dm: x86: PCI/SPI fixes for minnowboard MAX

Rather than just 'ERROR', display the error code, which may be useful, at least with driver model.

Signed-off-by Simon Glass <[email protected]>

---

 common/cmd_sf.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

common/cmd_sf.c +0 -2

index 342021d..668df69 100644
@@ -300,8 +300,12 @@ static int do_spi_flash_read_write(int argc, char * const argv[])
                else
                        ret = spi_flash_write(flash, offset, len, buf);
 
-               printf("SF: %zu bytes @ %#x %s: %s\n", (size_t)len, (u32)offset,
-                      read ? "Read" : "Written", ret ? "ERROR" : "OK");

Rather than just 'ERROR', display the error code, which may be useful, at least with driver model.

Signed-off-by Simon Glass <[email protected]>

Quoted 4 lines · Simon Glass

Rather than just 'ERROR', display the error code, which may be useful, at least with driver model.

Signed-off-by Simon Glass <[email protected]>

Acked-by Marek Vasut <[email protected]>

Quoted 7 lines · Marek Vasut

Quoted 4 lines · Simon Glass

Rather than just 'ERROR', display the error code, which may be useful, at least with driver model.

Signed-off-by Simon Glass <[email protected]>

Acked-by Marek Vasut <[email protected]>

Reviewed-by Jagan Teki <[email protected]>

Tidy up three minor problems in this file.

Signed-off-by Simon Glass <[email protected]>

---

 drivers/spi/ich.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

drivers/spi/ich.c +2 -2

index 50354fd..6b6cfbf 100644
@@ -422,7 +422,7 @@ static int ich_spi_xfer(struct udevice *dev, unsigned int bitlen,
        int using_cmd = 0;
        int ret;
 
-       /* Ee don't support writing partial bytes. */
+       /* We don't support writing partial bytes */
        if (bitlen % 8) {
                debug("ICH SPI: Accessing partial bytes not supported\n");
                return -EPROTONOSUPPORT;

@@ -601,7 +601,7 @@ static int ich_spi_xfer(struct udevice *dev, unsigned int bitlen,
                        return status;
 
                if (status & SPIS_FCERR) {
-                       debug("ICH SPI: Data transaction error\n");
+                       debug("ICH SPI: Data transaction error %x\n", status);
                        return -EIO;
                }
 

Quoted 34 lines · Simon Glass

Tidy up three minor problems in this file.

Signed-off-by Simon Glass <[email protected]>

---

 drivers/spi/ich.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

drivers/spi/ich.c +2 -2

index 50354fd..6b6cfbf 100644
@@ -422,7 +422,7 @@ static int ich_spi_xfer(struct udevice *dev, unsigned int bitlen,
        int using_cmd = 0;
        int ret;
 
-       /* Ee don't support writing partial bytes. */
+       /* We don't support writing partial bytes */
        if (bitlen % 8) {
                debug("ICH SPI: Accessing partial bytes not supported\n");
                return -EPROTONOSUPPORT;

@@ -601,7 +601,7 @@ static int ich_spi_xfer(struct udevice *dev, unsigned int bitlen,
                        return status;
 
                if (status & SPIS_FCERR) {
-                       debug("ICH SPI: Data transaction error\n");
+                       debug("ICH SPI: Data transaction error %x\n", status);
                        return -EIO;
                }
 

@@ -619,7 +619,6 @@ static int ich_spi_xfer(struct udevice *dev, unsigned int bitlen,
        return 0;
 }
 

drivers/spi/ich.c static int ich_spi_xfer(struct udevice *dev, unsigned int bitlen,

Quoted 5 lines

drivers/spi/ich.c +0 -2

-
 /*
  * This uses the SPI controller from the Intel Cougar Point and Panther Point
  * PCH to write-protect portions of the SPI flash until reboot. The changes
--

Reviewed-by Bin Meng <[email protected]>

Quoted 35 lines · Bin Meng

Quoted 34 lines · Simon Glass

Tidy up three minor problems in this file.

Signed-off-by Simon Glass <[email protected]>

---

 drivers/spi/ich.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

drivers/spi/ich.c +2 -2

index 50354fd..6b6cfbf 100644
@@ -422,7 +422,7 @@ static int ich_spi_xfer(struct udevice *dev, unsigned int bitlen,
        int using_cmd = 0;
        int ret;
 
-       /* Ee don't support writing partial bytes. */
+       /* We don't support writing partial bytes */
        if (bitlen % 8) {
                debug("ICH SPI: Accessing partial bytes not supported\n");
                return -EPROTONOSUPPORT;

@@ -601,7 +601,7 @@ static int ich_spi_xfer(struct udevice *dev, unsigned int bitlen,
                        return status;
 
                if (status & SPIS_FCERR) {
-                       debug("ICH SPI: Data transaction error\n");
+                       debug("ICH SPI: Data transaction error %x\n", status);
                        return -EIO;
                }
 

@@ -619,7 +619,6 @@ static int ich_spi_xfer(struct udevice *dev, unsigned int bitlen,
        return 0;
 }
 

Quoted 7 lines

drivers/spi/ich.c static int ich_spi_xfer(struct udevice *dev, unsigned int bitlen,

Quoted 5 lines

drivers/spi/ich.c +0 -2

-
 /*
  * This uses the SPI controller from the Intel Cougar Point and Panther Point
  * PCH to write-protect portions of the SPI flash until reboot. The changes
--

Reviewed-by Bin Meng <[email protected]>

Reviewed-by Jagan Teki <[email protected]>

The status register is a single byte, so use byte access when writing to it, to avoid updating the control register also.

Signed-off-by Simon Glass <[email protected]>

---

 drivers/spi/ich.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

drivers/spi/ich.c +0 -0

index 6b6cfbf..a8b4d0d 100644

The logic is incorrect and currently has no effect. Fix it so that we can write to SPI flash, since by default it is write-protected.

Signed-off-by Simon Glass <[email protected]>

---

 drivers/spi/ich.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

drivers/spi/ich.c +0 -0

index a8b4d0d..784320f 100644
@@ -687,10 +687,10 @@ static int ich_spi_probe(struct udevice *bus)
                struct ich9_spi_regs *ich9_spi;
 
                ich9_spi = priv->base;

The logic is incorrect and currently has no effect. Fix it so that we can write to SPI flash, since by default it is write-protected.

Signed-off-by Simon Glass <[email protected]>

---

 drivers/spi/ich.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

drivers/spi/ich.c +0 -0

index a8b4d0d..784320f 100644

drivers/spi/ich.c +1 -2

                bios_cntl &= ~(1 << 5); /* clear Enable InSMM_STS (EISS) */
                bios_cntl |= 1;         /* Write Protect Disable (WPD) */
-               ich_writeb(priv, bios_cntl, ich9_spi->bcr);
+               writeb(bios_cntl, &ich9_spi->bcr);
        } else {
                pci_read_config_byte(plat->dev, 0xdc, &bios_cntl);
                if (plat->ich_version == 9)
--

2.2.0.rc0.207.ga3a616c

Hi Simon,

Quoted 16 lines · Simon Glass

The logic is incorrect and currently has no effect. Fix it so that we can write to SPI flash, since by default it is write-protected.

Signed-off-by Simon Glass <[email protected]>

---

 drivers/spi/ich.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

drivers/spi/ich.c +0 -0

index a8b4d0d..784320f 100644
@@ -687,10 +687,10 @@ static int ich_spi_probe(struct udevice *bus)
                struct ich9_spi_regs *ich9_spi;
 

drivers/spi/ich.c static int ich_spi_probe(struct udevice *bus) · line 690

Quoted 3 lines

drivers/spi/ich.c +1 -1

                ich9_spi = priv->base;
-               bios_cntl = ich_readb(priv, ich9_spi->bcr);
+               bios_cntl = readb(&ich9_spi->bcr);

Couldn't understand based on the commit message, So for BIOS protection base shouldn't require or something?

It's not looks good to me to use generic io calls (readb|writeb) with in the functionality
code though we have a private calls defined on top to use generic ones.

drivers/spi/ich.c static int ich_spi_probe(struct udevice *bus)

Quoted 8 lines

drivers/spi/ich.c +1 -2

                bios_cntl &= ~(1 << 5); /* clear Enable InSMM_STS (EISS) */
                bios_cntl |= 1;         /* Write Protect Disable (WPD) */
-               ich_writeb(priv, bios_cntl, ich9_spi->bcr);
+               writeb(bios_cntl, &ich9_spi->bcr);
        } else {
                pci_read_config_byte(plat->dev, 0xdc, &bios_cntl);
                if (plat->ich_version == 9)
--

Enable a SPI environment and store it in a suitable place.

Signed-off-by Simon Glass <[email protected]>

---

 include/configs/minnowmax.h | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

include/configs/minnowmax.h +0 -0

index 547765d..d4d28a7 100644
@@ -65,8 +65,7 @@
 /* Avoid a warning in the Realtek Ethernet driver */

Quoted 22 lines · Simon Glass

Enable a SPI environment and store it in a suitable place.

Signed-off-by Simon Glass <[email protected]>

---

 include/configs/minnowmax.h | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

include/configs/minnowmax.h +2 -3

index 547765d..d4d28a7 100644
@@ -65,8 +65,7 @@
 /* Avoid a warning in the Realtek Ethernet driver */
 #define CONFIG_SYS_CACHELINE_SIZE 16
 
-/* Environment in SPI flash is unsupported for now */
-#undef CONFIG_ENV_IS_IN_SPI_FLASH
-#define CONFIG_ENV_IS_NOWHERE
+#define CONFIG_ENV_SECT_SIZE           0x1000
+#define CONFIG_ENV_OFFSET              0x007fe000
 

include/configs/minnowmax.h

Quoted 2 lines

include/configs/minnowmax.h +0 -1

 #endif /* __CONFIG_H */
--

Reviewed-by Bin Meng <[email protected]>

The layout of the ROM is a bit hard to discover by reading the code. Add a table to make it easier.

Signed-off-by Simon Glass <[email protected]>

---

 doc/README.x86 | 17 +++++++++++++++++
 1 file changed, 17 insertions(+)

doc/README.x86 +0 -0

index c19f4a0..596b73c 100644

This driver should use the x86 PCI configuration functions. Also adjust its compatible string to something generic (i.e. without a vendor name).

Signed-off-by Simon Glass <[email protected]>

---

 drivers/pci/pci_x86.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

drivers/pci/pci_x86.c +0 -0

index 901bdca..9f842c3 100644

Hi Simon,

Quoted 19 lines · Simon Glass

This driver should use the x86 PCI configuration functions. Also adjust its compatible string to something generic (i.e. without a vendor name).

Signed-off-by Simon Glass <[email protected]>

---

 drivers/pci/pci_x86.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

drivers/pci/pci_x86.c +1 -0

index 901bdca..9f842c3 100644
@@ -7,12 +7,15 @@
 #include <common.h>
 #include <dm.h>
 #include <pci.h>
+#include <asm/pci.h>
 

drivers/pci/pci_x86.c line 12

Quoted 1 line

drivers/pci/pci_x86.c +0 -0

 static const struct dm_pci_ops x86_pci_ops = {

To keep the consistent naming to match the driver name, can we rename this to pci_x86_ops?

drivers/pci/pci_x86.c line 14

Quoted 3 lines

drivers/pci/pci_x86.c +2 -0

+       .read_config    = pci_x86_read_config,
+       .write_config   = pci_x86_write_config,

Can we move pci_x86_read_config() and pci_x86_write_config() from arch/x86/cpu/pci.c to this file to make it a complete driver file? Also create a new header file pci_x86.h to declare these two so that it can be used by ivybridge.

drivers/pci/pci_x86.c line 17

Quoted 4 lines

drivers/pci/pci_x86.c +0 -0

 };
 
 static const struct udevice_id x86_pci_ids[] = {

Can we rename this to pci_x86_ids?

drivers/pci/pci_x86.c

Quoted 7 lines

drivers/pci/pci_x86.c +1 -2

-       { .compatible = "x86,pci" },
+       { .compatible = "pci-x86" },
        { }
 };
 
--

Hi Bin,

Quoted 22 lines · Bin Meng

Hi Simon,

Quoted 19 lines · Simon Glass

This driver should use the x86 PCI configuration functions. Also adjust its compatible string to something generic (i.e. without a vendor name).

Signed-off-by Simon Glass <[email protected]>

---

 drivers/pci/pci_x86.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

drivers/pci/pci_x86.c +1 -0

index 901bdca..9f842c3 100644
@@ -7,12 +7,15 @@
 #include <common.h>
 #include <dm.h>
 #include <pci.h>
+#include <asm/pci.h>
 

Quoted 4 lines

drivers/pci/pci_x86.c line 12

Quoted 1 line

drivers/pci/pci_x86.c +0 -0

 static const struct dm_pci_ops x86_pci_ops = {

To keep the consistent naming to match the driver name, can we rename this to pci_x86_ops?

OK

Quoted 8 lines

drivers/pci/pci_x86.c line 14

Quoted 3 lines

drivers/pci/pci_x86.c +2 -0

+       .read_config    = pci_x86_read_config,
+       .write_config   = pci_x86_write_config,

Can we move pci_x86_read_config() and pci_x86_write_config() from arch/x86/cpu/pci.c to this file to make it a complete driver file? Also create a new header file pci_x86.h to declare these two so that it can be used by ivybridge.

I can certainly drop the ivybridge duplication. But I don't think it is right to call directly into a driver in drivers/...

We should use driver model for this if we want to do it properly. I would like to continue the work to move x86 fully to driver model.

In the meantime I think that directly called functions should be in arch/x86.

Hi Simon,

Quoted 47 lines · Simon Glass

Hi Bin,

Quoted 22 lines · Bin Meng

Hi Simon,

Quoted 19 lines · Simon Glass

This driver should use the x86 PCI configuration functions. Also adjust its compatible string to something generic (i.e. without a vendor name).

Signed-off-by Simon Glass <[email protected]>

---

 drivers/pci/pci_x86.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

drivers/pci/pci_x86.c +1 -0

index 901bdca..9f842c3 100644
@@ -7,12 +7,15 @@
 #include <common.h>
 #include <dm.h>
 #include <pci.h>
+#include <asm/pci.h>
 

Quoted 4 lines

drivers/pci/pci_x86.c line 12

Quoted 1 line

drivers/pci/pci_x86.c +0 -0

 static const struct dm_pci_ops x86_pci_ops = {

To keep the consistent naming to match the driver name, can we rename this to pci_x86_ops?

OK

Quoted 8 lines

drivers/pci/pci_x86.c line 14

Quoted 3 lines

drivers/pci/pci_x86.c +2 -0

+       .read_config    = pci_x86_read_config,
+       .write_config   = pci_x86_write_config,

Can we move pci_x86_read_config() and pci_x86_write_config() from arch/x86/cpu/pci.c to this file to make it a complete driver file? Also create a new header file pci_x86.h to declare these two so that it can be used by ivybridge.

I can certainly drop the ivybridge duplication. But I don't think it is right to call directly into a driver in drivers/...

We should use driver model for this if we want to do it properly. I would like to continue the work to move x86 fully to driver model.

Quoted 2 lines

In the meantime I think that directly called functions should be in arch/x86.

Sorry I don't get it. I mean moving the implementation of pci_x86_read_config() and pci_x86_write_config() to drivers/pci/pci_x86.c. Do you have some concern about this?

[snip]

Hi Bin,

Quoted 57 lines · Bin Meng

Hi Simon,

Quoted 41 lines · Simon Glass

Hi Bin,

Quoted 22 lines · Bin Meng

Hi Simon,

Quoted 19 lines · Simon Glass

This driver should use the x86 PCI configuration functions. Also adjust its compatible string to something generic (i.e. without a vendor name).

Signed-off-by Simon Glass <[email protected]>

---

 drivers/pci/pci_x86.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

drivers/pci/pci_x86.c +1 -0

index 901bdca..9f842c3 100644
@@ -7,12 +7,15 @@
 #include <common.h>
 #include <dm.h>
 #include <pci.h>
+#include <asm/pci.h>
 

Quoted 4 lines

drivers/pci/pci_x86.c line 12

Quoted 1 line

drivers/pci/pci_x86.c +0 -0

 static const struct dm_pci_ops x86_pci_ops = {

To keep the consistent naming to match the driver name, can we rename this to pci_x86_ops?

OK

Quoted 8 lines

drivers/pci/pci_x86.c line 14

Quoted 3 lines

drivers/pci/pci_x86.c +2 -0

+       .read_config    = pci_x86_read_config,
+       .write_config   = pci_x86_write_config,

Can we move pci_x86_read_config() and pci_x86_write_config() from arch/x86/cpu/pci.c to this file to make it a complete driver file? Also create a new header file pci_x86.h to declare these two so that it can be used by ivybridge.

Quoted 8 lines

I can certainly drop the ivybridge duplication. But I don't think it is right to call directly into a driver in drivers/...

We should use driver model for this if we want to do it properly. I would like to continue the work to move x86 fully to driver model.

In the meantime I think that directly called functions should be in arch/x86.

Sorry I don't get it. I mean moving the implementation of pci_x86_read_config() and pci_x86_write_config() to drivers/pci/pci_x86.c. Do you have some concern about this?

Quoted 1 line

[snip]

Yes it is still used by arch/x86/cpu/coreboot/pci.c - and as I say I don't like the 'call directly into driver' idea. If we could remove the coreboot case then it would be fine.

Hi Simon,

Quoted 60 lines · Simon Glass

Hi Bin,

Quoted 57 lines · Bin Meng

Hi Simon,

Quoted 41 lines · Simon Glass

Hi Bin,

Quoted 22 lines · Bin Meng

Hi Simon,

Quoted 19 lines · Simon Glass

This driver should use the x86 PCI configuration functions. Also adjust its compatible string to something generic (i.e. without a vendor name).

Signed-off-by Simon Glass <[email protected]>

---

 drivers/pci/pci_x86.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

drivers/pci/pci_x86.c +1 -0

index 901bdca..9f842c3 100644
@@ -7,12 +7,15 @@
 #include <common.h>
 #include <dm.h>
 #include <pci.h>
+#include <asm/pci.h>
 

Quoted 4 lines

drivers/pci/pci_x86.c line 12

Quoted 1 line

drivers/pci/pci_x86.c +0 -0

 static const struct dm_pci_ops x86_pci_ops = {

To keep the consistent naming to match the driver name, can we rename this to pci_x86_ops?

OK

Quoted 8 lines

drivers/pci/pci_x86.c line 14

Quoted 3 lines

drivers/pci/pci_x86.c +2 -0

+       .read_config    = pci_x86_read_config,
+       .write_config   = pci_x86_write_config,

Can we move pci_x86_read_config() and pci_x86_write_config() from arch/x86/cpu/pci.c to this file to make it a complete driver file? Also create a new header file pci_x86.h to declare these two so that it can be used by ivybridge.

Quoted 8 lines

I can certainly drop the ivybridge duplication. But I don't think it is right to call directly into a driver in drivers/...

We should use driver model for this if we want to do it properly. I would like to continue the work to move x86 fully to driver model.

In the meantime I think that directly called functions should be in arch/x86.

Sorry I don't get it. I mean moving the implementation of pci_x86_read_config() and pci_x86_write_config() to drivers/pci/pci_x86.c. Do you have some concern about this?

Quoted 5 lines

Quoted 1 line

[snip]

Yes it is still used by arch/x86/cpu/coreboot/pci.c - and as I say I don't like the 'call directly into driver' idea. If we could remove the coreboot case then it would be fine.

Why not we drop this coreboot pci driver (arch/x86/cpu/coreboot/pci.c) and use the new one (drivers/pci/pci_x86.c) directly?

Hi Bin,

Quoted 65 lines · Bin Meng

Hi Simon,

Quoted 62 lines · Simon Glass

Hi Bin,

Quoted 58 lines · Bin Meng

Hi Simon,

Quoted 41 lines · Simon Glass

Hi Bin,

Quoted 22 lines · Bin Meng

Hi Simon,

Quoted 19 lines · Simon Glass

This driver should use the x86 PCI configuration functions. Also adjust its compatible string to something generic (i.e. without a vendor name).

Signed-off-by Simon Glass <[email protected]>

---

 drivers/pci/pci_x86.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

drivers/pci/pci_x86.c +1 -0

index 901bdca..9f842c3 100644
@@ -7,12 +7,15 @@
 #include <common.h>
 #include <dm.h>
 #include <pci.h>
+#include <asm/pci.h>
 

Quoted 4 lines

drivers/pci/pci_x86.c line 12

Quoted 1 line

drivers/pci/pci_x86.c +0 -0

 static const struct dm_pci_ops x86_pci_ops = {

To keep the consistent naming to match the driver name, can we rename this to pci_x86_ops?

OK

Quoted 8 lines

drivers/pci/pci_x86.c line 14

Quoted 3 lines

drivers/pci/pci_x86.c +2 -0

+       .read_config    = pci_x86_read_config,
+       .write_config   = pci_x86_write_config,

Can we move pci_x86_read_config() and pci_x86_write_config() from arch/x86/cpu/pci.c to this file to make it a complete driver file? Also create a new header file pci_x86.h to declare these two so that it can be used by ivybridge.

Quoted 8 lines

I can certainly drop the ivybridge duplication. But I don't think it is right to call directly into a driver in drivers/...

We should use driver model for this if we want to do it properly. I would like to continue the work to move x86 fully to driver model.

In the meantime I think that directly called functions should be in arch/x86.

Sorry I don't get it. I mean moving the implementation of pci_x86_read_config() and pci_x86_write_config() to drivers/pci/pci_x86.c. Do you have some concern about this?

[snip]

Quoted 6 lines

Quoted 3 lines

Yes it is still used by arch/x86/cpu/coreboot/pci.c - and as I say I don't like the 'call directly into driver' idea. If we could remove the coreboot case then it would be fine.

Why not we drop this coreboot pci driver (arch/x86/cpu/coreboot/pci.c) and use the new one (drivers/pci/pci_x86.c) directly?

Yes let's do that. I can't see any reason not to.

Adjust minnowmax to use driver model for PCI. This requires adding a device tree node to specify the ranges, removing the board-specific PCI code and ensuring that the host bridge is configured.

Signed-off-by Simon Glass <[email protected]>

---

 arch/x86/cpu/baytrail/Makefile |  1 -
 arch/x86/cpu/baytrail/pci.c    | 46 ------------------------------------------
 arch/x86/dts/minnowmax.dts     | 10 +++++++++
 configs/minnowmax_defconfig    |  1 +
 include/configs/minnowmax.h    |  1 +
 5 files changed, 12 insertions(+), 47 deletions(-)
 delete mode 100644 arch/x86/cpu/baytrail/pci.c

arch/x86/cpu/baytrail/Makefile +0 -1

index c78b644..5be5491 100644
@@ -7,5 +7,4 @@
 obj-y += cpu.o
 obj-y += early_uart.o
 obj-y += fsp_configs.o
-obj-y += pci.o
 obj-y += valleyview.o

arch/x86/cpu/baytrail/pci.c deleted +0 -46

deleted file mode 100644
index 48409de..0000000
@@ -1,46 +0,0 @@
-/*
- * Copyright (C) 2014, Bin Meng <[email protected]>
- *
- * SPDX-License-Identifier:    GPL-2.0+
- */
-
-#include <common.h>
-#include <pci.h>
-#include <asm/pci.h>
-#include <asm/fsp/fsp_support.h>
-
-DECLARE_GLOBAL_DATA_PTR;
-
-void board_pci_setup_hose(struct pci_controller *hose)
-{
-       hose->first_busno = 0;
-       hose->last_busno = 0;
-
-       /* PCI memory space */
-       pci_set_region(hose->regions + 0,
-                      CONFIG_PCI_MEM_BUS,
-                      CONFIG_PCI_MEM_PHYS,
-                      CONFIG_PCI_MEM_SIZE,
-                      PCI_REGION_MEM);
-
-       /* PCI IO space */
-       pci_set_region(hose->regions + 1,
-                      CONFIG_PCI_IO_BUS,
-                      CONFIG_PCI_IO_PHYS,
-                      CONFIG_PCI_IO_SIZE,
-                      PCI_REGION_IO);
-
-       pci_set_region(hose->regions + 2,
-                      CONFIG_PCI_PREF_BUS,
-                      CONFIG_PCI_PREF_PHYS,
-                      CONFIG_PCI_PREF_SIZE,
-                      PCI_REGION_PREFETCH);
-
-       pci_set_region(hose->regions + 3,
-                      0,
-                      0,
-                      gd->ram_size < 0x80000000 ? gd->ram_size : 0x80000000,
-                      PCI_REGION_MEM | PCI_REGION_SYS_MEMORY);
-
-       hose->region_count = 4;
-}

arch/x86/dts/minnowmax.dts +10 -0

index bd21bfb..0e59b18 100644
@@ -111,6 +111,16 @@
 
        };
 
+       pci {
+               compatible = "intel,pci-baytrail", "pci-x86";
+               #address-cells = <3>;
+               #size-cells = <2>;
+               u-boot,dm-pre-reloc;
+               ranges = <0x02000000 0x0 0xd0000000 0xd0000000 0 0x10000000
+                       0x42000000 0x0 0xc0000000 0xc0000000 0 0x10000000
+                       0x01000000 0x0 0x2000 0x2000 0 0xe000>;
+       };
+
        spi {
                #address-cells = <1>;
                #size-cells = <0>;

configs/minnowmax_defconfig +1 -0

index 744aca3..ff2bfda 100644
@@ -12,3 +12,4 @@ CONFIG_CMD_CPU=y
 CONFIG_CMD_NET=y
 CONFIG_OF_CONTROL=y
 CONFIG_CPU=y
+CONFIG_DM_PCI=y

include/configs/minnowmax.h +0 -0

index d4d28a7..41653ba 100644

Only the PCI controller has access to the PCI region information. Make sure to use the controller (rather than any attached bridges) when configuring devices.

This corrects a failure to scan and configure devices when driver model is enabled for PCI.

Also add a comment to explain the problem.

Signed-off-by Simon Glass <[email protected]>

---

 drivers/pci/pci-uclass.c |  6 +++++-
 drivers/pci/pci_common.c |  6 ++++++
 include/pci.h            | 10 ++++++++++
 3 files changed, 21 insertions(+), 1 deletion(-)

The sub-bus passed to pciauto_prescan_setup_bridge() is incorrect. Fix it so that sub-buses are numbered correctly.

Signed-off-by Simon Glass <[email protected]>

---

 drivers/pci/pci-uclass.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Commit afbbd413a fixed this for non-driver-model. Make sure that the driver model code handles this also.

Signed-off-by Simon Glass <[email protected]>

---

 drivers/pci/pci-uclass.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

drivers/pci/pci-uclass.c +0 -0

index edec93f..4255c02 100644

Hi Simon,

Quoted 21 lines · Simon Glass

Commit afbbd413a fixed this for non-driver-model. Make sure that the driver model code handles this also.

Signed-off-by Simon Glass <[email protected]>

---

 drivers/pci/pci-uclass.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

drivers/pci/pci-uclass.c +2 -1

index edec93f..4255c02 100644
@@ -495,7 +495,8 @@ static int decode_regions(struct pci_controller *hose, const void *blob,
 
        /* Add a region for our local memory */
        pci_set_region(hose->regions + hose->region_count++, 0, 0,
-                      gd->ram_size, PCI_REGION_MEM | PCI_REGION_SYS_MEMORY);
+                      gd->ram_size < 0x80000000U ? gd->ram_size : 0x80000000U,
+                      PCI_REGION_MEM | PCI_REGION_SYS_MEMORY);
 

drivers/pci/pci-uclass.c static int decode_regions(struct pci_controller *hose, const void *blob,

Quoted 3 lines

drivers/pci/pci-uclass.c +0 -1

        return 0;
 }
--

I think this is specific to baytrail fsp configuration. It should not be put into a common driver.

Hi Bin / Simon,

Quoted 24 lines · Bin Meng

Hi Simon,

Quoted 21 lines · Simon Glass

Commit afbbd413a fixed this for non-driver-model. Make sure that the driver model code handles this also.

Signed-off-by Simon Glass <[email protected]>

---

 drivers/pci/pci-uclass.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

drivers/pci/pci-uclass.c +2 -1

index edec93f..4255c02 100644
@@ -495,7 +495,8 @@ static int decode_regions(struct pci_controller *hose, const void *blob,
 
        /* Add a region for our local memory */
        pci_set_region(hose->regions + hose->region_count++, 0, 0,
-                      gd->ram_size, PCI_REGION_MEM | PCI_REGION_SYS_MEMORY);
+                      gd->ram_size < 0x80000000U ? gd->ram_size : 0x80000000U,
+                      PCI_REGION_MEM | PCI_REGION_SYS_MEMORY);
 

Quoted 6 lines

drivers/pci/pci-uclass.c static int decode_regions(struct pci_controller *hose, const void *blob,

Quoted 3 lines

drivers/pci/pci-uclass.c +0 -1

        return 0;
 }
--

I think this is specific to baytrail fsp configuration. It should not be put into a common driver.

Yes, I agree with Bin, this is likely only a Bay Trail (or maybe a very small number of other processors) configuration. I believe your change here will impact all PCI hosts which have > 2 GiB of RAM.

It might be a bit ugly to have an #ifdef in this file, it all seems like nice clean generic code. But maybe it's not a big deal to limit all u-boot to 2 GiB of RAM for this region?

Hi,

Quoted 41 lines · Andrew Bradford

Hi Bin / Simon,

Quoted 24 lines · Bin Meng

Hi Simon,

Quoted 21 lines · Simon Glass

Commit afbbd413a fixed this for non-driver-model. Make sure that the driver model code handles this also.

Signed-off-by Simon Glass <[email protected]>

---

 drivers/pci/pci-uclass.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

drivers/pci/pci-uclass.c +2 -1

index edec93f..4255c02 100644
@@ -495,7 +495,8 @@ static int decode_regions(struct pci_controller *hose, const void *blob,
 
        /* Add a region for our local memory */
        pci_set_region(hose->regions + hose->region_count++, 0, 0,
-                      gd->ram_size, PCI_REGION_MEM | PCI_REGION_SYS_MEMORY);
+                      gd->ram_size < 0x80000000U ? gd->ram_size : 0x80000000U,
+                      PCI_REGION_MEM | PCI_REGION_SYS_MEMORY);
 

Quoted 6 lines

drivers/pci/pci-uclass.c static int decode_regions(struct pci_controller *hose, const void *blob,

Quoted 3 lines

drivers/pci/pci-uclass.c +0 -1

        return 0;
 }
--

I think this is specific to baytrail fsp configuration. It should not be put into a common driver.

Yes, I agree with Bin, this is likely only a Bay Trail (or maybe a very small number of other processors) configuration. I believe your change here will impact all PCI hosts which have > 2 GiB of RAM.

It might be a bit ugly to have an #ifdef in this file, it all seems like nice clean generic code. But maybe it's not a big deal to limit all u-boot to 2 GiB of RAM for this region?

Yes, this will bite us when something else moves PCI to driver model and we may as well fix it now.

What is the best option? We could add a pci_max_addr to global_data perhaps? This could be set to ram_size for most machines, and adjusted for x86. I'd prefer to avoid #ifdef CONFIG_X86.

The SPI flash starts off protected on baytrail. The code which is supposed to fix this is broken. This series fixes that, enables the SPI environment and adds documentation.

Also when driver model is enabled for PCI some bugs appear. This series fixes those and enables driver model for PCI on minnowboard MAX.

Simon Glass (11):
spi: sf: Print the error code on failure
dm: spi: Correct minor nits in ICH driver
dm: spi: Correct status register access width
dm: spi: Correct BIOS protection logic for ICH9
dm: spi: Enable environment for minnowmax
x86: Add ROM image description for minnowmax
x86: pci: Tidy up the generic x86 PCI driver
dm: x86: minnowmax: Move PCI to use driver model
dm: pci: Use the correct hose when configuring devices
dm: pci: Correct bus number when scanning sub-buses
dm: x86: baytrail: Correct PCI region 3 when driver model is used