[U-Boot] [PATCH v2 0/7] dm: x86: PCI/SPI fixes for minnowboard MAX
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.
Changes in v2: - Continue to use writew for ICH7 - Use ich_read/write() for BIOS protection update - Fix typos in README.x86 - Rename the ops and ids arrays for consistency - Drop the coreboot PCI driver which is no-longer needed - Only limit the PCI system memory region on x86 machines
Simon Glass (7): 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: x86: baytrail: Correct PCI region 3 when driver model is used
arch/x86/cpu/baytrail/Makefile | 1 - arch/x86/cpu/baytrail/pci.c | 46 --------------------------------------- arch/x86/cpu/coreboot/pci.c | 21 ------------------ arch/x86/cpu/cpu.c | 1 + arch/x86/dts/minnowmax.dts | 10 +++++++++ common/board_f.c | 4 ++++ configs/minnowmax_defconfig | 1 + doc/README.x86 | 17 +++++++++++++++ drivers/pci/pci-uclass.c | 8 +++++-- drivers/pci/pci_x86.c | 13 ++++++----- drivers/spi/ich.c | 15 ++++++++----- include/asm-generic/global_data.h | 1 + include/configs/minnowmax.h | 6 ++--- 13 files changed, 60 insertions(+), 84 deletions(-) delete mode 100644 arch/x86/cpu/baytrail/pci.c
The status register on ICH9 is a single byte, so use byte access when writing to it, to avoid updating the control register also.
Signed-off-by: Simon Glass sjg@chromium.org ---
Changes in v2: - Continue to use writew for ICH7
drivers/spi/ich.c | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-)
diff --git a/drivers/spi/ich.c b/drivers/spi/ich.c index 6b6cfbf..66a5cba 100644 --- a/drivers/spi/ich.c +++ b/drivers/spi/ich.c @@ -411,6 +411,7 @@ static int ich_spi_xfer(struct udevice *dev, unsigned int bitlen, const void *dout, void *din, unsigned long flags) { struct udevice *bus = dev_get_parent(dev); + struct ich_spi_platdata *plat = dev_get_platdata(bus); struct ich_spi_priv *ctlr = dev_get_priv(bus); uint16_t control; int16_t opcode_index; @@ -477,7 +478,10 @@ static int ich_spi_xfer(struct udevice *dev, unsigned int bitlen, if (ret < 0) return ret;
- ich_writew(ctlr, SPIS_CDS | SPIS_FCERR, ctlr->status); + if (plat->ich_version == 7) + ich_writew(ctlr, SPIS_CDS | SPIS_FCERR, ctlr->status); + else + ich_writeb(ctlr, SPIS_CDS | SPIS_FCERR, ctlr->status);
spi_setup_type(trans, using_cmd ? bytes : 0); opcode_index = spi_setup_opcode(ctlr, trans);
On Fri, Jun 26, 2015 at 1:55 AM, Simon Glass sjg@chromium.org wrote:
The status register on ICH9 is a single byte, so use byte access when writing to it, to avoid updating the control register also.
Signed-off-by: Simon Glass sjg@chromium.org
Changes in v2:
- Continue to use writew for ICH7
drivers/spi/ich.c | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-)
diff --git a/drivers/spi/ich.c b/drivers/spi/ich.c index 6b6cfbf..66a5cba 100644 --- a/drivers/spi/ich.c +++ b/drivers/spi/ich.c @@ -411,6 +411,7 @@ static int ich_spi_xfer(struct udevice *dev, unsigned int bitlen, const void *dout, void *din, unsigned long flags) { struct udevice *bus = dev_get_parent(dev);
struct ich_spi_platdata *plat = dev_get_platdata(bus); struct ich_spi_priv *ctlr = dev_get_priv(bus); uint16_t control; int16_t opcode_index;@@ -477,7 +478,10 @@ static int ich_spi_xfer(struct udevice *dev, unsigned int bitlen, if (ret < 0) return ret;
ich_writew(ctlr, SPIS_CDS | SPIS_FCERR, ctlr->status);
if (plat->ich_version == 7)ich_writew(ctlr, SPIS_CDS | SPIS_FCERR, ctlr->status);elseich_writeb(ctlr, SPIS_CDS | SPIS_FCERR, ctlr->status); spi_setup_type(trans, using_cmd ? bytes : 0); opcode_index = spi_setup_opcode(ctlr, trans);--
Reviewed-by: Bin Meng bmeng.cn@gmail.com
On 29 June 2015 at 14:33, Bin Meng bmeng.cn@gmail.com wrote:
On Fri, Jun 26, 2015 at 1:55 AM, Simon Glass sjg@chromium.org wrote:
The status register on ICH9 is a single byte, so use byte access when writing to it, to avoid updating the control register also.
Signed-off-by: Simon Glass sjg@chromium.org
Changes in v2:
- Continue to use writew for ICH7
drivers/spi/ich.c | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-)
diff --git a/drivers/spi/ich.c b/drivers/spi/ich.c index 6b6cfbf..66a5cba 100644 --- a/drivers/spi/ich.c +++ b/drivers/spi/ich.c @@ -411,6 +411,7 @@ static int ich_spi_xfer(struct udevice *dev, unsigned int bitlen, const void *dout, void *din, unsigned long flags) { struct udevice *bus = dev_get_parent(dev);
struct ich_spi_platdata *plat = dev_get_platdata(bus); struct ich_spi_priv *ctlr = dev_get_priv(bus); uint16_t control; int16_t opcode_index;@@ -477,7 +478,10 @@ static int ich_spi_xfer(struct udevice *dev, unsigned int bitlen, if (ret < 0) return ret;
ich_writew(ctlr, SPIS_CDS | SPIS_FCERR, ctlr->status);
if (plat->ich_version == 7)ich_writew(ctlr, SPIS_CDS | SPIS_FCERR, ctlr->status);elseich_writeb(ctlr, SPIS_CDS | SPIS_FCERR, ctlr->status); spi_setup_type(trans, using_cmd ? bytes : 0); opcode_index = spi_setup_opcode(ctlr, trans);--
Reviewed-by: Bin Meng bmeng.cn@gmail.com
Reviewed-by: Jagan Teki jteki@openedev.com
thanks!
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 sjg@chromium.org ---
Changes in v2: - Use ich_read/write() for BIOS protection update
drivers/spi/ich.c | 9 ++++----- 1 file changed, 4 insertions(+), 5 deletions(-)
diff --git a/drivers/spi/ich.c b/drivers/spi/ich.c index 66a5cba..f68e07b 100644 --- a/drivers/spi/ich.c +++ b/drivers/spi/ich.c @@ -40,6 +40,7 @@ struct ich_spi_priv { int status; int control; int bbar; + int bcr; uint32_t *pr; /* only for ich9 */ int speed; /* pointer to speed control */ ulong max_speed; /* Maximum bus speed in MHz */ @@ -239,6 +240,7 @@ static int ich_init_controller(struct ich_spi_platdata *plat, ctlr->speed = ctlr->control + 2; ctlr->bbar = offsetof(struct ich9_spi_regs, bbar); ctlr->preop = offsetof(struct ich9_spi_regs, preop); + ctlr->bcr = offsetof(struct ich9_spi_regs, bcr); ctlr->pr = &ich9_spi->pr[0]; ctlr->base = ich9_spi; } else { @@ -688,13 +690,10 @@ static int ich_spi_probe(struct udevice *bus) * v9, deassert SMM BIOS Write Protect Disable. */ if (plat->use_sbase) { - struct ich9_spi_regs *ich9_spi; - - ich9_spi = priv->base; - bios_cntl = ich_readb(priv, ich9_spi->bcr); + bios_cntl = ich_readb(priv, priv->bcr); bios_cntl &= ~(1 << 5); /* clear Enable InSMM_STS (EISS) */ bios_cntl |= 1; /* Write Protect Disable (WPD) */ - ich_writeb(priv, bios_cntl, ich9_spi->bcr); + ich_writeb(priv, priv->bcr, bios_cntl); } else { pci_read_config_byte(plat->dev, 0xdc, &bios_cntl); if (plat->ich_version == 9)
On Fri, Jun 26, 2015 at 1:55 AM, Simon Glass sjg@chromium.org wrote:
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 sjg@chromium.org
Changes in v2:
- Use ich_read/write() for BIOS protection update
drivers/spi/ich.c | 9 ++++----- 1 file changed, 4 insertions(+), 5 deletions(-)
diff --git a/drivers/spi/ich.c b/drivers/spi/ich.c index 66a5cba..f68e07b 100644 --- a/drivers/spi/ich.c +++ b/drivers/spi/ich.c @@ -40,6 +40,7 @@ struct ich_spi_priv { int status; int control; int bbar;
int bcr; uint32_t *pr; /* only for ich9 */ int speed; /* pointer to speed control */ ulong max_speed; /* Maximum bus speed in MHz */@@ -239,6 +240,7 @@ static int ich_init_controller(struct ich_spi_platdata *plat, ctlr->speed = ctlr->control + 2; ctlr->bbar = offsetof(struct ich9_spi_regs, bbar); ctlr->preop = offsetof(struct ich9_spi_regs, preop);
ctlr->bcr = offsetof(struct ich9_spi_regs, bcr); ctlr->pr = &ich9_spi->pr[0]; ctlr->base = ich9_spi; } else {@@ -688,13 +690,10 @@ static int ich_spi_probe(struct udevice *bus) * v9, deassert SMM BIOS Write Protect Disable. */ if (plat->use_sbase) {
struct ich9_spi_regs *ich9_spi;ich9_spi = priv->base;bios_cntl = ich_readb(priv, ich9_spi->bcr);
bios_cntl = ich_readb(priv, priv->bcr); bios_cntl &= ~(1 << 5); /* clear Enable InSMM_STS (EISS) */ bios_cntl |= 1; /* Write Protect Disable (WPD) */
ich_writeb(priv, bios_cntl, ich9_spi->bcr);
ich_writeb(priv, priv->bcr, bios_cntl); } else { pci_read_config_byte(plat->dev, 0xdc, &bios_cntl); if (plat->ich_version == 9)--
Reviewed-by: Bin Meng bmeng.cn@gmail.com
On 29 June 2015 at 14:36, Bin Meng bmeng.cn@gmail.com wrote:
On Fri, Jun 26, 2015 at 1:55 AM, Simon Glass sjg@chromium.org wrote:
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 sjg@chromium.org
Changes in v2:
- Use ich_read/write() for BIOS protection update
drivers/spi/ich.c | 9 ++++----- 1 file changed, 4 insertions(+), 5 deletions(-)
diff --git a/drivers/spi/ich.c b/drivers/spi/ich.c index 66a5cba..f68e07b 100644 --- a/drivers/spi/ich.c +++ b/drivers/spi/ich.c @@ -40,6 +40,7 @@ struct ich_spi_priv { int status; int control; int bbar;
int bcr; uint32_t *pr; /* only for ich9 */ int speed; /* pointer to speed control */ ulong max_speed; /* Maximum bus speed in MHz */@@ -239,6 +240,7 @@ static int ich_init_controller(struct ich_spi_platdata *plat, ctlr->speed = ctlr->control + 2; ctlr->bbar = offsetof(struct ich9_spi_regs, bbar); ctlr->preop = offsetof(struct ich9_spi_regs, preop);
ctlr->bcr = offsetof(struct ich9_spi_regs, bcr); ctlr->pr = &ich9_spi->pr[0]; ctlr->base = ich9_spi; } else {@@ -688,13 +690,10 @@ static int ich_spi_probe(struct udevice *bus) * v9, deassert SMM BIOS Write Protect Disable. */ if (plat->use_sbase) {
struct ich9_spi_regs *ich9_spi;ich9_spi = priv->base;bios_cntl = ich_readb(priv, ich9_spi->bcr);
bios_cntl = ich_readb(priv, priv->bcr); bios_cntl &= ~(1 << 5); /* clear Enable InSMM_STS (EISS) */ bios_cntl |= 1; /* Write Protect Disable (WPD) */
ich_writeb(priv, bios_cntl, ich9_spi->bcr);
ich_writeb(priv, priv->bcr, bios_cntl); } else { pci_read_config_byte(plat->dev, 0xdc, &bios_cntl); if (plat->ich_version == 9)--
Reviewed-by: Bin Meng bmeng.cn@gmail.com
Reviewed-by: Jagan Teki jteki@openedev.com
thanks!
Enable a SPI environment and store it in a suitable place.
Signed-off-by: Simon Glass sjg@chromium.org Reviewed-by: Bin Meng bmeng.cn@gmail.com Reviewed-by: Jagan Teki jteki@openedev.com ---
Changes in v2: None
include/configs/minnowmax.h | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-)
diff --git a/include/configs/minnowmax.h b/include/configs/minnowmax.h index 547765d..d4d28a7 100644 --- a/include/configs/minnowmax.h +++ b/include/configs/minnowmax.h @@ -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
#endif /* __CONFIG_H */
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 sjg@chromium.org ---
Changes in v2: - Fix typos in README.x86
doc/README.x86 | 17 +++++++++++++++++ 1 file changed, 17 insertions(+)
diff --git a/doc/README.x86 b/doc/README.x86 index 49d6e83..e58ca19 100644 --- a/doc/README.x86 +++ b/doc/README.x86 @@ -160,6 +160,23 @@ Now you can build U-Boot and obtain u-boot.rom $ make minnowmax_defconfig $ make all
+The ROM image is broken up into these parts: + +Offset Description Controlling config +------------------------------------------------------------ +000000 descriptor.bin Hard-coded to 0 in ifdtool +001000 me.bin Set by the descriptor +500000 <spare> +700000 u-boot-dtb.bin CONFIG_SYS_TEXT_BASE +790000 vga.bin CONFIG_X86_OPTION_ROM_ADDR +7c0000 fsp.bin CONFIG_FSP_ADDR +7f8000 <spare> (depends on size of fsp.bin) +7fe000 Environment CONFIG_ENV_OFFSET +7ff800 U-Boot 16-bit boot CONFIG_SYS_X86_START16 + +Overall ROM image size is controlled by CONFIG_ROM_SIZE. + + Intel Galileo instructions:
Only one binary blob is needed for Remote Management Unit (RMU) within Intel
On Fri, Jun 26, 2015 at 1:55 AM, Simon Glass sjg@chromium.org wrote:
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 sjg@chromium.org
Changes in v2:
- Fix typos in README.x86
doc/README.x86 | 17 +++++++++++++++++ 1 file changed, 17 insertions(+)
diff --git a/doc/README.x86 b/doc/README.x86 index 49d6e83..e58ca19 100644 --- a/doc/README.x86 +++ b/doc/README.x86 @@ -160,6 +160,23 @@ Now you can build U-Boot and obtain u-boot.rom $ make minnowmax_defconfig $ make all
+The ROM image is broken up into these parts:
+Offset Description Controlling config +------------------------------------------------------------ +000000 descriptor.bin Hard-coded to 0 in ifdtool +001000 me.bin Set by the descriptor +500000 <spare> +700000 u-boot-dtb.bin CONFIG_SYS_TEXT_BASE +790000 vga.bin CONFIG_X86_OPTION_ROM_ADDR +7c0000 fsp.bin CONFIG_FSP_ADDR +7f8000 <spare> (depends on size of fsp.bin) +7fe000 Environment CONFIG_ENV_OFFSET +7ff800 U-Boot 16-bit boot CONFIG_SYS_X86_START16
+Overall ROM image size is controlled by CONFIG_ROM_SIZE.
Intel Galileo instructions:
Only one binary blob is needed for Remote Management Unit (RMU) within Intel
Reviewed-by: Bin Meng bmeng.cn@gmail.com
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 sjg@chromium.org ---
Changes in v2: - Rename the ops and ids arrays for consistency - Drop the coreboot PCI driver which is no-longer needed
arch/x86/cpu/coreboot/pci.c | 21 --------------------- drivers/pci/pci_x86.c | 13 ++++++++----- 2 files changed, 8 insertions(+), 26 deletions(-)
diff --git a/arch/x86/cpu/coreboot/pci.c b/arch/x86/cpu/coreboot/pci.c index 67eb14c..af9f391 100644 --- a/arch/x86/cpu/coreboot/pci.c +++ b/arch/x86/cpu/coreboot/pci.c @@ -11,30 +11,9 @@
#include <common.h> #include <dm.h> -#include <errno.h> #include <pci.h> -#include <asm/io.h> #include <asm/pci.h>
-DECLARE_GLOBAL_DATA_PTR; - -static const struct dm_pci_ops pci_x86_ops = { - .read_config = pci_x86_read_config, - .write_config = pci_x86_write_config, -}; - -static const struct udevice_id pci_x86_ids[] = { - { .compatible = "pci-x86" }, - { } -}; - -U_BOOT_DRIVER(pci_x86_drv) = { - .name = "pci_x86", - .id = UCLASS_PCI, - .of_match = pci_x86_ids, - .ops = &pci_x86_ops, -}; - static const struct udevice_id generic_pch_ids[] = { { .compatible = "intel,pch" }, { } diff --git a/drivers/pci/pci_x86.c b/drivers/pci/pci_x86.c index 901bdca..89e8c11 100644 --- a/drivers/pci/pci_x86.c +++ b/drivers/pci/pci_x86.c @@ -7,18 +7,21 @@ #include <common.h> #include <dm.h> #include <pci.h> +#include <asm/pci.h>
-static const struct dm_pci_ops x86_pci_ops = { +static const struct dm_pci_ops pci_x86_ops = { + .read_config = pci_x86_read_config, + .write_config = pci_x86_write_config, };
-static const struct udevice_id x86_pci_ids[] = { - { .compatible = "x86,pci" }, +static const struct udevice_id pci_x86_ids[] = { + { .compatible = "pci-x86" }, { } };
U_BOOT_DRIVER(pci_x86) = { .name = "pci_x86", .id = UCLASS_PCI, - .of_match = x86_pci_ids, - .ops = &x86_pci_ops, + .of_match = pci_x86_ids, + .ops = &pci_x86_ops, };
Hi Simon,
On Fri, Jun 26, 2015 at 1:55 AM, Simon Glass sjg@chromium.org wrote:
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 sjg@chromium.org
Changes in v2:
- Rename the ops and ids arrays for consistency
- Drop the coreboot PCI driver which is no-longer needed
arch/x86/cpu/coreboot/pci.c | 21 --------------------- drivers/pci/pci_x86.c | 13 ++++++++----- 2 files changed, 8 insertions(+), 26 deletions(-)
diff --git a/arch/x86/cpu/coreboot/pci.c b/arch/x86/cpu/coreboot/pci.c index 67eb14c..af9f391 100644 --- a/arch/x86/cpu/coreboot/pci.c +++ b/arch/x86/cpu/coreboot/pci.c @@ -11,30 +11,9 @@
#include <common.h> #include <dm.h> -#include <errno.h> #include <pci.h> -#include <asm/io.h> #include <asm/pci.h>
Should <pci.h> and <asm/pci.h> be removed too?
-DECLARE_GLOBAL_DATA_PTR;
-static const struct dm_pci_ops pci_x86_ops = {
.read_config = pci_x86_read_config,.write_config = pci_x86_write_config,-};
-static const struct udevice_id pci_x86_ids[] = {
{ .compatible = "pci-x86" },{ }-};
-U_BOOT_DRIVER(pci_x86_drv) = {
.name = "pci_x86",.id = UCLASS_PCI,.of_match = pci_x86_ids,.ops = &pci_x86_ops,-};
static const struct udevice_id generic_pch_ids[] = { { .compatible = "intel,pch" }, { }
What's this? Is this required by some board?
diff --git a/drivers/pci/pci_x86.c b/drivers/pci/pci_x86.c index 901bdca..89e8c11 100644 --- a/drivers/pci/pci_x86.c +++ b/drivers/pci/pci_x86.c @@ -7,18 +7,21 @@ #include <common.h> #include <dm.h> #include <pci.h> +#include <asm/pci.h>
-static const struct dm_pci_ops x86_pci_ops = { +static const struct dm_pci_ops pci_x86_ops = {
.read_config = pci_x86_read_config,.write_config = pci_x86_write_config,};
As I mentioned in previous discussion, I still think it's better to move the implementation of pci_x86_read_config() and pci_x86_write_config() from arch/x86/cpu/pci.c to this file (drivers/pci/pci_x86.c). I don't think calling arch/x86/cpu/ivybridge/pci.c would be a problem. The arch/x86/cpu/pci.c provides generic x86 pci config access methods which can be utilized by some variants of x86 pci controller (like ivybridge). And I also looked the arch/x86/cpu/ivybridge/pci.c driver and just wonder why should we create that ivybridge-specific driver? Can we move pci_ivybridge_probe() from that file to arch/x86/cpu/ivybridge/bd82x6x.c? This way, we no longer need the ivybridge-specific pci driver as x86 pci controller should be generic enough.
-static const struct udevice_id x86_pci_ids[] = {
{ .compatible = "x86,pci" },+static const struct udevice_id pci_x86_ids[] = {
{ .compatible = "pci-x86" }, { }};
U_BOOT_DRIVER(pci_x86) = { .name = "pci_x86", .id = UCLASS_PCI,
.of_match = x86_pci_ids,.ops = &x86_pci_ops,
.of_match = pci_x86_ids,.ops = &pci_x86_ops,};
Regards, Bin
Hi Bin,
On 29 June 2015 at 03:21, Bin Meng bmeng.cn@gmail.com wrote:
Hi Simon,
On Fri, Jun 26, 2015 at 1:55 AM, Simon Glass sjg@chromium.org wrote:
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 sjg@chromium.org
Changes in v2:
- Rename the ops and ids arrays for consistency
- Drop the coreboot PCI driver which is no-longer needed
arch/x86/cpu/coreboot/pci.c | 21 --------------------- drivers/pci/pci_x86.c | 13 ++++++++----- 2 files changed, 8 insertions(+), 26 deletions(-)
diff --git a/arch/x86/cpu/coreboot/pci.c b/arch/x86/cpu/coreboot/pci.c index 67eb14c..af9f391 100644 --- a/arch/x86/cpu/coreboot/pci.c +++ b/arch/x86/cpu/coreboot/pci.c @@ -11,30 +11,9 @@
#include <common.h> #include <dm.h> -#include <errno.h> #include <pci.h> -#include <asm/io.h> #include <asm/pci.h>
Should <pci.h> and <asm/pci.h> be removed too?
OK.
-DECLARE_GLOBAL_DATA_PTR;
-static const struct dm_pci_ops pci_x86_ops = {
.read_config = pci_x86_read_config,.write_config = pci_x86_write_config,-};
-static const struct udevice_id pci_x86_ids[] = {
{ .compatible = "pci-x86" },{ }-};
-U_BOOT_DRIVER(pci_x86_drv) = {
.name = "pci_x86",.id = UCLASS_PCI,.of_match = pci_x86_ids,.ops = &pci_x86_ops,-};
static const struct udevice_id generic_pch_ids[] = { { .compatible = "intel,pch" }, { }
What's this? Is this required by some board?
Yes, it is needed for ivybridge (when booted from coreboot). The PCH contains the SPI and LPC peripherals so we need a driver to make those visible.
diff --git a/drivers/pci/pci_x86.c b/drivers/pci/pci_x86.c index 901bdca..89e8c11 100644 --- a/drivers/pci/pci_x86.c +++ b/drivers/pci/pci_x86.c @@ -7,18 +7,21 @@ #include <common.h> #include <dm.h> #include <pci.h> +#include <asm/pci.h>
-static const struct dm_pci_ops x86_pci_ops = { +static const struct dm_pci_ops pci_x86_ops = {
.read_config = pci_x86_read_config,.write_config = pci_x86_write_config,};
As I mentioned in previous discussion, I still think it's better to move the implementation of pci_x86_read_config() and pci_x86_write_config() from arch/x86/cpu/pci.c to this file (drivers/pci/pci_x86.c). I don't think calling arch/x86/cpu/ivybridge/pci.c would be a problem. The arch/x86/cpu/pci.c provides generic x86 pci config access methods which can be utilized by some variants of x86 pci controller (like ivybridge). And I also looked the arch/x86/cpu/ivybridge/pci.c driver and just wonder why should we create that ivybridge-specific driver? Can we move pci_ivybridge_probe() from that file to arch/x86/cpu/ivybridge/bd82x6x.c? This way, we no longer need the ivybridge-specific pci driver as x86 pci controller should be generic enough.
But pci_ivybridge_probe() calls the code in bd82x6x.c.
With ivybridge we attach all the device init to the PCI probe function. It should be possible to move more of this to driver model, but not in this patch.
-static const struct udevice_id x86_pci_ids[] = {
{ .compatible = "x86,pci" },+static const struct udevice_id pci_x86_ids[] = {
{ .compatible = "pci-x86" }, { }};
U_BOOT_DRIVER(pci_x86) = { .name = "pci_x86", .id = UCLASS_PCI,
.of_match = x86_pci_ids,.ops = &x86_pci_ops,
.of_match = pci_x86_ids,.ops = &pci_x86_ops,};
Regards, Simon
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.
Reviewed-by: Bin Meng bmeng.cn@gmail.com Signed-off-by: Simon Glass sjg@chromium.org ---
Changes in v2: None
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
diff --git a/arch/x86/cpu/baytrail/Makefile b/arch/x86/cpu/baytrail/Makefile index c78b644..5be5491 100644 --- a/arch/x86/cpu/baytrail/Makefile +++ b/arch/x86/cpu/baytrail/Makefile @@ -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 diff --git a/arch/x86/cpu/baytrail/pci.c b/arch/x86/cpu/baytrail/pci.c deleted file mode 100644 index 48409de..0000000 --- a/arch/x86/cpu/baytrail/pci.c +++ /dev/null @@ -1,46 +0,0 @@ -/* - * Copyright (C) 2014, Bin Meng bmeng.cn@gmail.com - * - * 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; -} diff --git a/arch/x86/dts/minnowmax.dts b/arch/x86/dts/minnowmax.dts index bd21bfb..0e59b18 100644 --- a/arch/x86/dts/minnowmax.dts +++ b/arch/x86/dts/minnowmax.dts @@ -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>; diff --git a/configs/minnowmax_defconfig b/configs/minnowmax_defconfig index 744aca3..ff2bfda 100644 --- a/configs/minnowmax_defconfig +++ b/configs/minnowmax_defconfig @@ -12,3 +12,4 @@ CONFIG_CMD_CPU=y CONFIG_CMD_NET=y CONFIG_OF_CONTROL=y CONFIG_CPU=y +CONFIG_DM_PCI=y diff --git a/include/configs/minnowmax.h b/include/configs/minnowmax.h index d4d28a7..41653ba 100644 --- a/include/configs/minnowmax.h +++ b/include/configs/minnowmax.h @@ -32,6 +32,7 @@ #define CONFIG_PCI_IO_PHYS CONFIG_PCI_IO_BUS #define CONFIG_PCI_IO_SIZE 0xe000
+#define CONFIG_PCI_CONFIG_HOST_BRIDGE #define CONFIG_SYS_EARLY_PCI_INIT #define CONFIG_PCI_PNP #define CONFIG_RTL8169
Commit afbbd413a fixed this for non-driver-model. Make sure that the driver model code handles this also.
Signed-off-by: Simon Glass sjg@chromium.org ---
Changes in v2: - Only limit the PCI system memory region on x86 machines
arch/x86/cpu/cpu.c | 1 + common/board_f.c | 4 ++++ drivers/pci/pci-uclass.c | 8 ++++++-- include/asm-generic/global_data.h | 1 + 4 files changed, 12 insertions(+), 2 deletions(-)
diff --git a/arch/x86/cpu/cpu.c b/arch/x86/cpu/cpu.c index d108ee5..936b6ee 100644 --- a/arch/x86/cpu/cpu.c +++ b/arch/x86/cpu/cpu.c @@ -351,6 +351,7 @@ int x86_cpu_init_f(void)
gd->arch.has_mtrr = has_mtrr(); } + gd->pci_ram_top = 0x80000000U;
return 0; } diff --git a/common/board_f.c b/common/board_f.c index 21be26f..cb85382 100644 --- a/common/board_f.c +++ b/common/board_f.c @@ -350,6 +350,10 @@ static int setup_dest_addr(void) debug("Reserving MP boot page to %08lx\n", gd->relocaddr); } #endif +#ifdef CONFIG_PCI + gd->pci_ram_top = gd->ram_top; +#endif + return 0; }
diff --git a/drivers/pci/pci-uclass.c b/drivers/pci/pci-uclass.c index edec93f..5b91fe3 100644 --- a/drivers/pci/pci-uclass.c +++ b/drivers/pci/pci-uclass.c @@ -444,6 +444,7 @@ static int decode_regions(struct pci_controller *hose, const void *blob, { int pci_addr_cells, addr_cells, size_cells; int cells_per_record; + phys_addr_t addr; const u32 *prop; int len; int i; @@ -494,8 +495,11 @@ 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); + addr = gd->ram_size; + if (gd->pci_ram_top && gd->pci_ram_top < addr) + addr = gd->pci_ram_top; + pci_set_region(hose->regions + hose->region_count++, 0, 0, addr, + PCI_REGION_MEM | PCI_REGION_SYS_MEMORY);
return 0; } diff --git a/include/asm-generic/global_data.h b/include/asm-generic/global_data.h index 6747619..db0550b 100644 --- a/include/asm-generic/global_data.h +++ b/include/asm-generic/global_data.h @@ -93,6 +93,7 @@ typedef struct global_data { #endif #ifdef CONFIG_PCI struct pci_controller *hose; /* PCI hose for early use */ + phys_addr_t pci_ram_top; /* top of region accessible to PCI */ #endif #ifdef CONFIG_PCI_BOOTDELAY int pcidelay_done;
On Fri, Jun 26, 2015 at 1:55 AM, Simon Glass sjg@chromium.org wrote:
Commit afbbd413a fixed this for non-driver-model. Make sure that the driver model code handles this also.
Signed-off-by: Simon Glass sjg@chromium.org
Changes in v2:
- Only limit the PCI system memory region on x86 machines
arch/x86/cpu/cpu.c | 1 + common/board_f.c | 4 ++++ drivers/pci/pci-uclass.c | 8 ++++++-- include/asm-generic/global_data.h | 1 + 4 files changed, 12 insertions(+), 2 deletions(-)
diff --git a/arch/x86/cpu/cpu.c b/arch/x86/cpu/cpu.c index d108ee5..936b6ee 100644 --- a/arch/x86/cpu/cpu.c +++ b/arch/x86/cpu/cpu.c @@ -351,6 +351,7 @@ int x86_cpu_init_f(void)
gd->arch.has_mtrr = has_mtrr(); }
gd->pci_ram_top = 0x80000000U; return 0;} diff --git a/common/board_f.c b/common/board_f.c index 21be26f..cb85382 100644 --- a/common/board_f.c +++ b/common/board_f.c @@ -350,6 +350,10 @@ static int setup_dest_addr(void) debug("Reserving MP boot page to %08lx\n", gd->relocaddr); } #endif +#ifdef CONFIG_PCI
gd->pci_ram_top = gd->ram_top;+#endif
return 0;}
diff --git a/drivers/pci/pci-uclass.c b/drivers/pci/pci-uclass.c index edec93f..5b91fe3 100644 --- a/drivers/pci/pci-uclass.c +++ b/drivers/pci/pci-uclass.c @@ -444,6 +444,7 @@ static int decode_regions(struct pci_controller *hose, const void *blob, { int pci_addr_cells, addr_cells, size_cells; int cells_per_record;
phys_addr_t addr; const u32 *prop; int len; int i;@@ -494,8 +495,11 @@ 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);
addr = gd->ram_size;if (gd->pci_ram_top && gd->pci_ram_top < addr)addr = gd->pci_ram_top;pci_set_region(hose->regions + hose->region_count++, 0, 0, addr,PCI_REGION_MEM | PCI_REGION_SYS_MEMORY); return 0;} diff --git a/include/asm-generic/global_data.h b/include/asm-generic/global_data.h index 6747619..db0550b 100644 --- a/include/asm-generic/global_data.h +++ b/include/asm-generic/global_data.h @@ -93,6 +93,7 @@ typedef struct global_data { #endif #ifdef CONFIG_PCI struct pci_controller *hose; /* PCI hose for early use */
phys_addr_t pci_ram_top; /* top of region accessible to PCI */#endif #ifdef CONFIG_PCI_BOOTDELAY int pcidelay_done; --
Reviewed-by: Bin Meng bmeng.cn@gmail.com
Hi Simon,
Probably I'm not fully understanding the startup flow and how PCI fits into it yet, so can you please help me to understand this patch a little more?
On 06/25 11:55, Simon Glass wrote:
Commit afbbd413a fixed this for non-driver-model. Make sure that the driver model code handles this also.
Signed-off-by: Simon Glass sjg@chromium.org
Changes in v2:
- Only limit the PCI system memory region on x86 machines
arch/x86/cpu/cpu.c | 1 + common/board_f.c | 4 ++++ drivers/pci/pci-uclass.c | 8 ++++++-- include/asm-generic/global_data.h | 1 + 4 files changed, 12 insertions(+), 2 deletions(-)
diff --git a/arch/x86/cpu/cpu.c b/arch/x86/cpu/cpu.c index d108ee5..936b6ee 100644 --- a/arch/x86/cpu/cpu.c +++ b/arch/x86/cpu/cpu.c @@ -351,6 +351,7 @@ int x86_cpu_init_f(void)
gd->arch.has_mtrr = has_mtrr();}
- gd->pci_ram_top = 0x80000000U;
So this sets gd->pci_ram_top to 2 GiB, then...
return 0; } diff --git a/common/board_f.c b/common/board_f.c index 21be26f..cb85382 100644 --- a/common/board_f.c +++ b/common/board_f.c @@ -350,6 +350,10 @@ static int setup_dest_addr(void) debug("Reserving MP boot page to %08lx\n", gd->relocaddr); } #endif +#ifdef CONFIG_PCI
- gd->pci_ram_top = gd->ram_top;
This can set gd->pci_ram_top to be the actual size of memory later in boot, which on E3800 with 4 GiB of SDRAM will be 4 GiB (right?), then...
+#endif
- return 0;
}
diff --git a/drivers/pci/pci-uclass.c b/drivers/pci/pci-uclass.c index edec93f..5b91fe3 100644 --- a/drivers/pci/pci-uclass.c +++ b/drivers/pci/pci-uclass.c @@ -444,6 +444,7 @@ static int decode_regions(struct pci_controller *hose, const void *blob, { int pci_addr_cells, addr_cells, size_cells; int cells_per_record;
- phys_addr_t addr; const u32 *prop; int len; int i;
@@ -494,8 +495,11 @@ 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);
- addr = gd->ram_size;
- if (gd->pci_ram_top && gd->pci_ram_top < addr)
addr = gd->pci_ram_top;
This sets addr to be the lesser of gd->ram_size and gd->pci_ram_top when we enumerate a PCI device. Right?
What is the flow if, for example, a Baytrail E3800 has 4 GiB of RAM (and the physical memory hole from 2 GiB to 4 GiB exists), then does this code here actually set addr to be 2 GiB like it should?
I don't fear that memory sizes of 2 GiB or less will work fine but I don't quite understand if this patch works how I expect for memory sizes larger than 2 GiB if the processor has a physical memory hole like E3800 does.
Why wouldn't the assignment in board_f.c interfere and set gd->pci_ram_top to be 4 GiB instead of 2 GiB like it should be?
Thanks, Andrew
Hi Andrew,
On 29 June 2015 at 12:44, Andrew Bradford andrew@bradfordembedded.com wrote:
Hi Simon,
Probably I'm not fully understanding the startup flow and how PCI fits into it yet, so can you please help me to understand this patch a little more?
On 06/25 11:55, Simon Glass wrote:
Commit afbbd413a fixed this for non-driver-model. Make sure that the driver model code handles this also.
Signed-off-by: Simon Glass sjg@chromium.org
Changes in v2:
- Only limit the PCI system memory region on x86 machines
arch/x86/cpu/cpu.c | 1 + common/board_f.c | 4 ++++ drivers/pci/pci-uclass.c | 8 ++++++-- include/asm-generic/global_data.h | 1 + 4 files changed, 12 insertions(+), 2 deletions(-)
diff --git a/arch/x86/cpu/cpu.c b/arch/x86/cpu/cpu.c index d108ee5..936b6ee 100644 --- a/arch/x86/cpu/cpu.c +++ b/arch/x86/cpu/cpu.c @@ -351,6 +351,7 @@ int x86_cpu_init_f(void)
gd->arch.has_mtrr = has_mtrr(); }
gd->pci_ram_top = 0x80000000U;So this sets gd->pci_ram_top to 2 GiB, then...
return 0;} diff --git a/common/board_f.c b/common/board_f.c index 21be26f..cb85382 100644 --- a/common/board_f.c +++ b/common/board_f.c @@ -350,6 +350,10 @@ static int setup_dest_addr(void) debug("Reserving MP boot page to %08lx\n", gd->relocaddr); } #endif +#ifdef CONFIG_PCI
gd->pci_ram_top = gd->ram_top;This can set gd->pci_ram_top to be the actual size of memory later in boot, which on E3800 with 4 GiB of SDRAM will be 4 GiB (right?), then...
+#endif
return 0;}
diff --git a/drivers/pci/pci-uclass.c b/drivers/pci/pci-uclass.c index edec93f..5b91fe3 100644 --- a/drivers/pci/pci-uclass.c +++ b/drivers/pci/pci-uclass.c @@ -444,6 +444,7 @@ static int decode_regions(struct pci_controller *hose, const void *blob, { int pci_addr_cells, addr_cells, size_cells; int cells_per_record;
phys_addr_t addr; const u32 *prop; int len; int i;@@ -494,8 +495,11 @@ 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);
addr = gd->ram_size;if (gd->pci_ram_top && gd->pci_ram_top < addr)addr = gd->pci_ram_top;This sets addr to be the lesser of gd->ram_size and gd->pci_ram_top when we enumerate a PCI device. Right?
What is the flow if, for example, a Baytrail E3800 has 4 GiB of RAM (and the physical memory hole from 2 GiB to 4 GiB exists), then does this code here actually set addr to be 2 GiB like it should?
I don't fear that memory sizes of 2 GiB or less will work fine but I don't quite understand if this patch works how I expect for memory sizes larger than 2 GiB if the processor has a physical memory hole like E3800 does.
Why wouldn't the assignment in board_f.c interfere and set gd->pci_ram_top to be 4 GiB instead of 2 GiB like it should be?
I think the assignment in board_f.c should be removed. Since there is a zero check, it is not needed, and it will interfere as you say. Thanks for picking this up. I'll respin it.
Regards, Simon
On 06/25 11:55, Simon Glass wrote:
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.
Changes in v2:
- Continue to use writew for ICH7
- Use ich_read/write() for BIOS protection update
- Fix typos in README.x86
- Rename the ops and ids arrays for consistency
- Drop the coreboot PCI driver which is no-longer needed
- Only limit the PCI system memory region on x86 machines
Simon Glass (7): 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: x86: baytrail: Correct PCI region 3 when driver model is used
arch/x86/cpu/baytrail/Makefile | 1 - arch/x86/cpu/baytrail/pci.c | 46 --------------------------------------- arch/x86/cpu/coreboot/pci.c | 21 ------------------ arch/x86/cpu/cpu.c | 1 + arch/x86/dts/minnowmax.dts | 10 +++++++++ common/board_f.c | 4 ++++ configs/minnowmax_defconfig | 1 + doc/README.x86 | 17 +++++++++++++++ drivers/pci/pci-uclass.c | 8 +++++-- drivers/pci/pci_x86.c | 13 ++++++----- drivers/spi/ich.c | 15 ++++++++----- include/asm-generic/global_data.h | 1 + include/configs/minnowmax.h | 6 ++--- 13 files changed, 60 insertions(+), 84 deletions(-) delete mode 100644 arch/x86/cpu/baytrail/pci.c
Tested-by: Andrew Bradford andrew.bradford@kodakalaris.com
I don't actually have a Minnowmax board or any E3800 board that has the type of SPI flash that's on Minnowmax, but when I try to do an `env save` it just seems to hang unless I interrupt it with ^C (output below). Even waiting 15 minutes for the env write to complete doesn't help (I don't currently have a logic analyzer hooked up, sorry).
I'm running the minnowmax_defconfig but adjusted only to handle that I have a SODIMM installed so telling FSP to set memory_down to 0 (hence 1 GB of memory). My board is close enough to Minnow Max to be useful (patches to support it hopefully coming real soon now).
But the PCI bits seem to be working for me! :)
Thanks! -Andrew
Output from doing `env save`:
U-Boot 2015.07-rc2-00176-gc3dd276 (Jun 29 2015 - 08:17:03 -0400)
CPU: x86_64, vendor Intel, device 30679h DRAM: 1 GiB Warning: MP init failure MMC: ValleyView SDHCI: 0, ValleyView SDHCI: 1 SF: Detected W25Q64DW with page size 256 Bytes, erase size 4 KiB, total 8 MiB, mapped at ff800000 *** Warning - bad CRC, using default environment
Video: 1280x1024x16 Model: Intel Minnowboard Max SCSI: SATA link 0 timeout. SATA link 1 timeout. AHCI 0001.0300 32 slots 2 ports 3 Gbps 0x3 impl SATA mode flags: 64bit ncq stag pm led clo pio slum part sxs scanning bus for devices... Found 0 device(s). Net: RTL8169#0 Error: RTL8169#0 address not set.
=> env print baudrate=115200 bootargs=root=/dev/sdb3 init=/sbin/init rootwait ro bootcmd=ext2load scsi 0:3 01000000 /boot/vmlinuz; zboot 01000000 bootfile=bzImage consoledev=ttyS0 ethact=RTL8169#0 hostname="x86" loadaddr=0x1000000 netdev=eth0 nfsboot=setenv bootargs root=/dev/nfs rw nfsroot=$serverip:$rootpath ip=$ipaddr:$serverip:$gatewayip:$netmask:$hostname:$netdev:off console=$consoledev,$baudrate $othbootargs;tftpboot $loadaddr $bootfile;zboot $loadaddr othbootargs=acpi=off pciconfighost=1 ramboot=setenv bootargs root=/dev/ram rw ip=$ipaddr:$serverip:$gatewayip:$netmask:$hostname:$netdev:off console=$consoledev,$baudrate $othbootargs;tftpboot $loadaddr $bootfile;tftpboot $ramdiskaddr $ramdiskfile;zboot $loadaddr 0 $ramdiskaddr $filesize ramdiskaddr=0x2000000 ramdiskfile=initramfs.gz rootpath=/opt/nfsroot scsidevs=0 stderr=vga,serial stdin=usbkbd,vga,serial stdout=vga,serial
Environment size: 920/4092 bytes => env set simon says => env save Saving Environment to SPI Flash... SF: Detected W25Q64DW with page size 256 Bytes, erase size 4 KiB, total 8 MiB, mapped at ff800000 Erasing SPI flash...Writing to SPI flash...=> <INTERRUPT> =>
Hi Simon,
On 06/29 09:07, Andrew Bradford wrote:
On 06/25 11:55, Simon Glass wrote:
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.
Changes in v2:
- Continue to use writew for ICH7
- Use ich_read/write() for BIOS protection update
- Fix typos in README.x86
- Rename the ops and ids arrays for consistency
- Drop the coreboot PCI driver which is no-longer needed
- Only limit the PCI system memory region on x86 machines
Simon Glass (7): 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: x86: baytrail: Correct PCI region 3 when driver model is used
arch/x86/cpu/baytrail/Makefile | 1 - arch/x86/cpu/baytrail/pci.c | 46 --------------------------------------- arch/x86/cpu/coreboot/pci.c | 21 ------------------ arch/x86/cpu/cpu.c | 1 + arch/x86/dts/minnowmax.dts | 10 +++++++++ common/board_f.c | 4 ++++ configs/minnowmax_defconfig | 1 + doc/README.x86 | 17 +++++++++++++++ drivers/pci/pci-uclass.c | 8 +++++-- drivers/pci/pci_x86.c | 13 ++++++----- drivers/spi/ich.c | 15 ++++++++----- include/asm-generic/global_data.h | 1 + include/configs/minnowmax.h | 6 ++--- 13 files changed, 60 insertions(+), 84 deletions(-) delete mode 100644 arch/x86/cpu/baytrail/pci.c
Tested-by: Andrew Bradford andrew.bradford@kodakalaris.com
I don't actually have a Minnowmax board or any E3800 board that has the type of SPI flash that's on Minnowmax, but when I try to do an `env save` it just seems to hang unless I interrupt it with ^C (output below). Even waiting 15 minutes for the env write to complete doesn't help (I don't currently have a logic analyzer hooked up, sorry).
I'm running the minnowmax_defconfig but adjusted only to handle that I have a SODIMM installed so telling FSP to set memory_down to 0 (hence 1 GB of memory). My board is close enough to Minnow Max to be useful (patches to support it hopefully coming real soon now).
But the PCI bits seem to be working for me! :)
<snip>
I've enabled debug output and with this patchset from you I believe I'm able to erase and read the SPI flash fine, I just cannot write to it. That I seem to be able to do erases but not writes confuses me.
This is tested on an E3845 based board that's fairly similar to Minnowmax but uses a different SPI flash. I know my SPI flash is OK, I'm able to program it with a Dediprog SF100 programmer without issue.
A debug output enabled log is below. It seems that my writes are resulting in the FCERR bit being set in the Software Sequencing Flash Status Register, indicating that either I'm violating a protection policy or I'm writing to a programmed cycle register when a programming access is already in progress. I do not believe I'm writing to any registers during a programming operation, so it seems I'm violating some kind of protection but all of the protections I see in the data sheet disallow writes *AND* erases, so I'm a bit confused.
drivers/spi/ich.c and the sequence of events to do writes all seems correct to me as I understand the Bay Trail datasheet.
Any pointers on what I could look at to try to better understand why FCERR is getting set during writes?
Thanks, Andrew
Below is example of doing probe, erase, write, and then test. In both the write and test cases, writes fail due to FCERR being set:
=> sf probe uclass_find_device_by_seq: 0 0 - 0 0 - found spi_find_chip_select: plat=3b845528, cs=0 uclass_find_device_by_seq: 0 0 - 0 0 - found spi_get_bus_and_cs: Binding new device 'spi_flash@0:0', busnum=0, cs=0, driver=spi_flash_std Bound device spi_flash@0:0 to spi uclass_find_device_by_seq: 0 -1 uclass_find_device_by_seq: 0 0 - -1 -1 - not found spi_flash_std_probe: slave=3b847b68, cs=0 ich_spi_set_mode: mode=3 ICH SPI: Saved 1 bytes ICH SPI: Using 1 bytes read 0080 from 0090 wrote 0c to 0090 wrote 9f to 0098 read 0001 from 0096 wrote 0000 to 0096 read 0000 from 0091 read 0000 from 0094 wrote 00000000 to 0008 wrote 4402 to 0091 read 0084 from 0090 wrote 0004 to 0090 wrote 0000 to 0094 SF: Got idcodes 00000000: ef 60 17 00 00 .`... read 0080 from 0090 wrote 0c to 0090 wrote 06 to 0098 read 0000 from 0096 wrote 0001 to 0096 wrote 0006 to 0094 ICH SPI: Saved 1 bytes ICH SPI: Using 1 bytes read 0080 from 0090 wrote 0c to 0090 wrote 01 to 0098 read 0001 from 0096 wrote 0001 to 0096 ICH SPI: Moving to data, 1 bytes read 4400 from 0091 read 0006 from 0094 wrote 00000000 to 0008 wrote 4006 to 0091 read 0481 from 0090 read 0481 from 0090 read 0484 from 0090 wrote 0004 to 0090 wrote 0000 to 0094 ICH SPI: Saved 1 bytes ICH SPI: Using 1 bytes read 0080 from 0090 wrote 0c to 0090 wrote 05 to 0098 read 0001 from 0096 wrote 0000 to 0096 read 4000 from 0091 read 0000 from 0094 wrote 00000000 to 0008 wrote 4002 to 0091 read 0084 from 0090 wrote 0004 to 0090 wrote 0000 to 0094 fdtdec_get_addr_size: memory-map: addr=ff800000, size=00800000 SF: Detected W25Q64DW with page size 256 Bytes, erase size 4 KiB, total 8 MiB, mapped at ff800000 ich_spi_set_mode: mode=3 spi_get_bus_and_cs: bus=3b845448, slave=3b847b68
=> sf erase 7fe000 1000 SF: erase 20 7f e0 0 (7fe000) read 0080 from 0090 wrote 0c to 0090 wrote 06 to 0098 read 0000 from 0096 wrote 0001 to 0096 wrote 0006 to 0094 read 0080 from 0090 wrote 0c to 0090 wrote 20 to 0098 read 0001 from 0096 wrote 0001 to 0096 read 4000 from 0091 read 0006 from 0094 wrote 00000000 to 0008 wrote 4206 to 0091 read 0481 from 0090 read 0481 from 0090 read 0484 from 0090 wrote 0004 to 0090 wrote 0000 to 0094 ICH SPI: Saved 1 bytes ICH SPI: Using 1 bytes read 0080 from 0090 wrote 0c to 0090 wrote 05 to 0098 read 0001 from 0096 wrote 0000 to 0096 read 4200 from 0091 read 0000 from 0094 wrote 00000000 to 0008 wrote 4002 to 0091 read 0084 from 0090 wrote 0004 to 0090 wrote 0000 to 0094 SF: 4096 bytes @ 0x7fe000 Erased: OK
=> sf write $loadaddr 7fe000 1000 SF: 0x01000000 => cmd = { 0x02 0x7fe000 } chunk_len = 64 read 0080 from 0090 wrote 0c to 0090 wrote 06 to 0098 read 0000 from 0096 wrote 0001 to 0096 wrote 0006 to 0094 ICH SPI: Saved 4 bytes ICH SPI: Using 4 bytes read 0080 from 0090 wrote 0c to 0090 wrote 02 to 0098 read 0001 from 0096 wrote 0003 to 0096 ICH SPI: Moving to data, 64 bytes read 4000 from 0091 read 0006 from 0094 wrote 007fe000 to 0008 wrote 7f06 to 0091 read 048c from 0090 wrote 000c to 0090 ICH SPI: Data transaction error 48c SF: Failed to transfer 64 bytes of data: -5 SF: write cmd failed SF: write failed SF: 4096 bytes @ 0x7fe000 Written: ERROR -5 Command failed, result=1
=> sf test 7fe000 1000 SPI flash test: SF: erase 20 7f e0 0 (7fe000) read 0080 from 0090 wrote 0c to 0090 wrote 06 to 0098 read 0000 from 0096 wrote 0001 to 0096 wrote 0006 to 0094 read 0080 from 0090 wrote 0c to 0090 wrote 20 to 0098 read 0001 from 0096 wrote 0001 to 0096 read 4000 from 0091 read 0006 from 0094 wrote 00000000 to 0008 wrote 4206 to 0091 read 0481 from 0090 read 0481 from 0090 read 0481 from 0090 read 0481 from 0090 read 0484 from 0090 wrote 0004 to 0090 wrote 0000 to 0094 ICH SPI: Saved 1 bytes ICH SPI: Using 1 bytes read 0080 from 0090 wrote 0c to 0090 wrote 05 to 0098 read 0001 from 0096 wrote 0000 to 0096 read 4200 from 0091 read 0000 from 0094 wrote 00000000 to 0008 wrote 4002 to 0091 read 0084 from 0090 wrote 0004 to 0090 wrote 0000 to 0094 0 erase: 501 ticks, 7 KiB/s 0.056 Mbps ICH SPI: Using 1 bytes read 0080 from 0090 wrote 0c to 0090 wrote 05 to 0098 read 0000 from 0096 wrote 0001 to 0096 read 4000 from 0091 read 0000 from 0094 wrote 0002 to 0091 read 0084 from 0090 wrote 0004 to 0090 ICH SPI: Using 1 bytes read 0080 from 0090 wrote 0c to 0090 wrote 05 to 0098 read 0001 from 0096 wrote 0001 to 0096 read 0000 from 0091 read 0000 from 0094 wrote 0002 to 0091 read 0084 from 0090 wrote 0004 to 0090 1 check: 333 ticks, 12 KiB/s 0.096 Mbps SF: 0x3b84afc0 => cmd = { 0x02 0x7fe000 } chunk_len = 64 read 0080 from 0090 wrote 0c to 0090 wrote 06 to 0098 read 0001 from 0096 wrote 0001 to 0096 wrote 0006 to 0094 ICH SPI: Saved 4 bytes ICH SPI: Using 4 bytes read 0080 from 0090 wrote 0c to 0090 wrote 02 to 0098 read 0001 from 0096 wrote 0003 to 0096 ICH SPI: Moving to data, 64 bytes read 0000 from 0091 read 0006 from 0094 wrote 007fe000 to 0008 wrote 7f06 to 0091 read 048c from 0090 wrote 000c to 0090 ICH SPI: Data transaction error 48c SF: Failed to transfer 64 bytes of data: -5 SF: write cmd failed SF: write failed Write failed Test failed Command failed, result=1
Hi Andrew,
On 2 July 2015 at 13:59, Andrew Bradford andrew@bradfordembedded.com wrote:
Hi Simon,
On 06/29 09:07, Andrew Bradford wrote:
On 06/25 11:55, Simon Glass wrote:
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.
Changes in v2:
- Continue to use writew for ICH7
- Use ich_read/write() for BIOS protection update
- Fix typos in README.x86
- Rename the ops and ids arrays for consistency
- Drop the coreboot PCI driver which is no-longer needed
- Only limit the PCI system memory region on x86 machines
Simon Glass (7): 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: x86: baytrail: Correct PCI region 3 when driver model is used
arch/x86/cpu/baytrail/Makefile | 1 - arch/x86/cpu/baytrail/pci.c | 46 --------------------------------------- arch/x86/cpu/coreboot/pci.c | 21 ------------------ arch/x86/cpu/cpu.c | 1 + arch/x86/dts/minnowmax.dts | 10 +++++++++ common/board_f.c | 4 ++++ configs/minnowmax_defconfig | 1 + doc/README.x86 | 17 +++++++++++++++ drivers/pci/pci-uclass.c | 8 +++++-- drivers/pci/pci_x86.c | 13 ++++++----- drivers/spi/ich.c | 15 ++++++++----- include/asm-generic/global_data.h | 1 + include/configs/minnowmax.h | 6 ++--- 13 files changed, 60 insertions(+), 84 deletions(-) delete mode 100644 arch/x86/cpu/baytrail/pci.c
Tested-by: Andrew Bradford andrew.bradford@kodakalaris.com
I don't actually have a Minnowmax board or any E3800 board that has the type of SPI flash that's on Minnowmax, but when I try to do an `env save` it just seems to hang unless I interrupt it with ^C (output below). Even waiting 15 minutes for the env write to complete doesn't help (I don't currently have a logic analyzer hooked up, sorry).
I'm running the minnowmax_defconfig but adjusted only to handle that I have a SODIMM installed so telling FSP to set memory_down to 0 (hence 1 GB of memory). My board is close enough to Minnow Max to be useful (patches to support it hopefully coming real soon now).
But the PCI bits seem to be working for me! :)
<snip>
I've enabled debug output and with this patchset from you I believe I'm able to erase and read the SPI flash fine, I just cannot write to it. That I seem to be able to do erases but not writes confuses me.
This is tested on an E3845 based board that's fairly similar to Minnowmax but uses a different SPI flash. I know my SPI flash is OK, I'm able to program it with a Dediprog SF100 programmer without issue.
A debug output enabled log is below. It seems that my writes are resulting in the FCERR bit being set in the Software Sequencing Flash Status Register, indicating that either I'm violating a protection policy or I'm writing to a programmed cycle register when a programming access is already in progress. I do not believe I'm writing to any registers during a programming operation, so it seems I'm violating some kind of protection but all of the protections I see in the data sheet disallow writes *AND* erases, so I'm a bit confused.
drivers/spi/ich.c and the sequence of events to do writes all seems correct to me as I understand the Bay Trail datasheet.
Any pointers on what I could look at to try to better understand why FCERR is getting set during writes?
I saw the same thing but put it down to the protection still being in place in certain areas - it has hit me before and I went through a similar process as you did here. But actually it's a bug - I switched the parameters of the function around. I'll send an update series soon. Thanks for testing this and finding the problem.
Thanks, Andrew
[snip]
participants (4)
-
Andrew Bradford -
Bin Meng -
Jagan Teki -
Simon Glass