[U-Boot] [PATCH 00/11] dm: x86: PCI/SPI fixes for minnowboard MAX

[U-Boot] [PATCH 00/11] dm: x86: PCI/SPI fixes for minnowboard MAX

[Simon Glass]

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

 arch/x86/cpu/baytrail/Makefile |  1 -
 arch/x86/cpu/baytrail/pci.c    | 46 ------------------------------------------
 arch/x86/dts/minnowmax.dts     | 10 +++++++++
 common/cmd_sf.c                |  8 ++++++--
 configs/minnowmax_defconfig    |  1 +
 doc/README.x86                 | 17 ++++++++++++++++
 drivers/pci/pci-uclass.c       | 11 +++++++---
 drivers/pci/pci_common.c       |  6 ++++++
 drivers/pci/pci_x86.c          |  5 ++++-
 drivers/spi/ich.c              | 11 +++++-----
 include/configs/minnowmax.h    |  6 +++---
 include/pci.h                  | 10 +++++++++
 12 files changed, 70 insertions(+), 62 deletions(-)
 delete mode 100644 arch/x86/cpu/baytrail/pci.c

-- 
2.2.0.rc0.207.ga3a616c

[U-Boot] [PATCH 01/11] spi: sf: Print the error code on failure

[Simon Glass]

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

Signed-off-by: Simon Glass <sjg@chromium.org>
---

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

diff --git a/common/cmd_sf.c b/common/cmd_sf.c
index 342021d..668df69 100644
--- a/common/cmd_sf.c
+++ b/common/cmd_sf.c
@@ -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");
+		printf("SF: %zu bytes @ %#x %s: ", (size_t)len, (u32)offset,
+		       read ? "Read" : "Written");
+		if (ret)
+			printf("ERROR %d\n", ret);
+		else
+			printf("OK\n");
 	}
 
 	unmap_physmem(buf, len);
-- 
2.2.0.rc0.207.ga3a616c

[U-Boot] [PATCH 02/11] dm: spi: Correct minor nits in ICH driver

[Simon Glass]

Tidy up three minor problems in this file.

Signed-off-by: Simon Glass <sjg@chromium.org>
---

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

diff --git a/drivers/spi/ich.c b/drivers/spi/ich.c
index 50354fd..6b6cfbf 100644
--- a/drivers/spi/ich.c
+++ b/drivers/spi/ich.c
@@ -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;
 }
 
-
 /*
  * 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
-- 
2.2.0.rc0.207.ga3a616c

[U-Boot] [PATCH 03/11] dm: spi: Correct status register access width

[Simon Glass]

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 <sjg@chromium.org>
---

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

diff --git a/drivers/spi/ich.c b/drivers/spi/ich.c
index 6b6cfbf..a8b4d0d 100644
--- a/drivers/spi/ich.c
+++ b/drivers/spi/ich.c
@@ -477,7 +477,7 @@ 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);
+	ich_writeb(ctlr, SPIS_CDS | SPIS_FCERR, ctlr->status);
 
 	spi_setup_type(trans, using_cmd ? bytes : 0);
 	opcode_index = spi_setup_opcode(ctlr, trans);
-- 
2.2.0.rc0.207.ga3a616c

[U-Boot] [PATCH 04/11] dm: spi: Correct BIOS protection logic for ICH9

[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 <sjg@chromium.org>
---

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

diff --git a/drivers/spi/ich.c b/drivers/spi/ich.c
index a8b4d0d..784320f 100644
--- a/drivers/spi/ich.c
+++ b/drivers/spi/ich.c
@@ -687,10 +687,10 @@ static int ich_spi_probe(struct udevice *bus)
 		struct ich9_spi_regs *ich9_spi;
 
 		ich9_spi = priv->base;
-		bios_cntl = ich_readb(priv, ich9_spi->bcr);
+		bios_cntl = readb(&ich9_spi->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);
+		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

[U-Boot] [PATCH 05/11] dm: spi: Enable environment for minnowmax

[Simon Glass]

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

Signed-off-by: Simon Glass <sjg@chromium.org>
---

 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 */
-- 
2.2.0.rc0.207.ga3a616c

[U-Boot] [PATCH 06/11] x86: Add ROM image description for minnowmax

[Simon Glass]

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>
---

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

diff --git a/doc/README.x86 b/doc/README.x86
index c19f4a0..596b73c 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 iftool
+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   Emvironment         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
-- 
2.2.0.rc0.207.ga3a616c

[U-Boot] [PATCH 07/11] x86: pci: Tidy up the generic x86 PCI driver

[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 <sjg@chromium.org>
---

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

diff --git a/drivers/pci/pci_x86.c b/drivers/pci/pci_x86.c
index 901bdca..9f842c3 100644
--- a/drivers/pci/pci_x86.c
+++ b/drivers/pci/pci_x86.c
@@ -7,12 +7,15 @@
 #include <common.h>
 #include <dm.h>
 #include <pci.h>
+#include <asm/pci.h>
 
 static const struct dm_pci_ops x86_pci_ops = {
+	.read_config	= pci_x86_read_config,
+	.write_config	= pci_x86_write_config,
 };
 
 static const struct udevice_id x86_pci_ids[] = {
-	{ .compatible = "x86,pci" },
+	{ .compatible = "pci-x86" },
 	{ }
 };
 
-- 
2.2.0.rc0.207.ga3a616c

[U-Boot] [PATCH 08/11] dm: x86: minnowmax: Move PCI to use driver model

[Simon Glass]

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 <sjg@chromium.org>
---

 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
-- 
2.2.0.rc0.207.ga3a616c

[U-Boot] [PATCH 09/11] dm: pci: Use the correct hose when configuring devices

[Simon Glass]

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 <sjg@chromium.org>
---

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

diff --git a/drivers/pci/pci-uclass.c b/drivers/pci/pci-uclass.c
index de87505..41d19cb 100644
--- a/drivers/pci/pci-uclass.c
+++ b/drivers/pci/pci-uclass.c
@@ -296,6 +296,7 @@ int pci_auto_config_devices(struct udevice *bus)
 	     !ret && dev;
 	     ret = device_find_next_child(&dev)) {
 		struct pci_child_platdata *pplat;
+		struct pci_controller *ctlr_hose;
 
 		pplat = dev_get_parent_platdata(dev);
 		unsigned int max_bus;
@@ -303,7 +304,10 @@ int pci_auto_config_devices(struct udevice *bus)
 
 		bdf = PCI_ADD_BUS(bus->seq, pplat->devfn);
 		debug("%s: device %s\n", __func__, dev->name);
-		max_bus = pciauto_config_device(hose, bdf);
+
+		/* The root controller has the region information */
+		ctlr_hose = hose->ctlr->uclass_priv;
+		max_bus = pciauto_config_device(ctlr_hose, bdf);
 		sub_bus = max(sub_bus, max_bus);
 	}
 	debug("%s: done\n", __func__);
diff --git a/drivers/pci/pci_common.c b/drivers/pci/pci_common.c
index b9ff23f..f67c9c7 100644
--- a/drivers/pci/pci_common.c
+++ b/drivers/pci/pci_common.c
@@ -11,6 +11,7 @@
  */
 
 #include <common.h>
+#include <dm.h>
 #include <errno.h>
 #include <pci.h>
 #include <asm/io.h>
@@ -221,6 +222,11 @@ phys_addr_t pci_hose_bus_to_phys(struct pci_controller *hose,
 		return phys_addr;
 	}
 
+#ifdef CONFIG_DM_PCI
+	/* The root controller has the region information */
+	hose = hose->ctlr->uclass_priv;
+#endif
+
 	/*
 	 * if PCI_REGION_MEM is set we do a two pass search with preference
 	 * on matches that don't have PCI_REGION_SYS_MEMORY set
diff --git a/include/pci.h b/include/pci.h
index 07b1e9a..3af511b 100644
--- a/include/pci.h
+++ b/include/pci.h
@@ -513,6 +513,16 @@ struct pci_controller {
 
 	int indirect_type;
 
+	/*
+	 * TODO(sjg at chromium.org): With driver model we use struct
+	 * pci_controller for both the controller and any bridge devices
+	 * attached to it. But there is only one region list and it is in the
+	 * top-level controller.
+	 *
+	 * This could be changed so that struct pci_controller is only used
+	 * for PCI controllers and a separate UCLASS (or perhaps
+	 * UCLASS_PCI_GENERIC) is used for bridges.
+	 */
 	struct pci_region regions[MAX_PCI_REGIONS];
 	int region_count;
 
-- 
2.2.0.rc0.207.ga3a616c

[U-Boot] [PATCH 10/11] dm: pci: Correct bus number when scanning sub-buses

[Simon Glass]

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 <sjg@chromium.org>
---

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

diff --git a/drivers/pci/pci-uclass.c b/drivers/pci/pci-uclass.c
index 41d19cb..edec93f 100644
--- a/drivers/pci/pci-uclass.c
+++ b/drivers/pci/pci-uclass.c
@@ -334,7 +334,7 @@ int dm_pci_hose_probe_bus(struct pci_controller *hose, pci_dev_t bdf)
 
 	sub_bus = pci_get_bus_max() + 1;
 	debug("%s: bus = %d/%s\n", __func__, sub_bus, bus->name);
-	pciauto_prescan_setup_bridge(hose, bdf, bus->seq);
+	pciauto_prescan_setup_bridge(hose, bdf, sub_bus);
 
 	ret = device_probe(bus);
 	if (ret) {
-- 
2.2.0.rc0.207.ga3a616c

[U-Boot] [PATCH 11/11] dm: x86: baytrail: Correct PCI region 3 when driver model is used

[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 <sjg@chromium.org>
---

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

diff --git a/drivers/pci/pci-uclass.c b/drivers/pci/pci-uclass.c
index edec93f..4255c02 100644
--- a/drivers/pci/pci-uclass.c
+++ b/drivers/pci/pci-uclass.c
@@ -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);
 
 	return 0;
 }
-- 
2.2.0.rc0.207.ga3a616c

[U-Boot] [PATCH 01/11] spi: sf: Print the error code on failure

[Bin Meng]

On Sun, Jun 7, 2015 at 10:50 PM, Simon Glass <sjg@chromium.org> wrote:
> Rather than just 'ERROR', display the error code, which may be useful, at
> least with driver model.
>
> Signed-off-by: Simon Glass <sjg@chromium.org>
> ---
>
>  common/cmd_sf.c | 8 ++++++--
>  1 file changed, 6 insertions(+), 2 deletions(-)
>
> diff --git a/common/cmd_sf.c b/common/cmd_sf.c
> index 342021d..668df69 100644
> --- a/common/cmd_sf.c
> +++ b/common/cmd_sf.c
> @@ -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");
> +               printf("SF: %zu bytes @ %#x %s: ", (size_t)len, (u32)offset,
> +                      read ? "Read" : "Written");
> +               if (ret)
> +                       printf("ERROR %d\n", ret);
> +               else
> +                       printf("OK\n");
>         }
>
>         unmap_physmem(buf, len);
> --

Reviewed-by: Bin Meng <bmeng.cn@gmail.com>

[U-Boot] [PATCH 02/11] dm: spi: Correct minor nits in ICH driver

[Bin Meng]

On Sun, Jun 7, 2015 at 10:50 PM, Simon Glass <sjg@chromium.org> wrote:
> Tidy up three minor problems in this file.
>
> Signed-off-by: Simon Glass <sjg@chromium.org>
> ---
>
>  drivers/spi/ich.c | 5 ++---
>  1 file changed, 2 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/spi/ich.c b/drivers/spi/ich.c
> index 50354fd..6b6cfbf 100644
> --- a/drivers/spi/ich.c
> +++ b/drivers/spi/ich.c
> @@ -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;
>  }
>
> -
>  /*
>   * 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 <bmeng.cn@gmail.com>

[U-Boot] [PATCH 03/11] dm: spi: Correct status register access width

[Bin Meng]

Hi Simon,

On Sun, Jun 7, 2015 at 10:50 PM, Simon Glass <sjg@chromium.org> wrote:
> 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 <sjg@chromium.org>
> ---
>
>  drivers/spi/ich.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/spi/ich.c b/drivers/spi/ich.c
> index 6b6cfbf..a8b4d0d 100644
> --- a/drivers/spi/ich.c
> +++ b/drivers/spi/ich.c
> @@ -477,7 +477,7 @@ 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);
> +       ich_writeb(ctlr, SPIS_CDS | SPIS_FCERR, ctlr->status);

But for ICH7 the status is 16-bit. Please handle both cases.

>
>         spi_setup_type(trans, using_cmd ? bytes : 0);
>         opcode_index = spi_setup_opcode(ctlr, trans);
> --

Regards,
Bin

[U-Boot] [PATCH 04/11] dm: spi: Correct BIOS protection logic for ICH9

[Bin Meng]

On Sun, Jun 7, 2015 at 10:50 PM, 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>
> ---
>
>  drivers/spi/ich.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/spi/ich.c b/drivers/spi/ich.c
> index a8b4d0d..784320f 100644
> --- a/drivers/spi/ich.c
> +++ b/drivers/spi/ich.c
> @@ -687,10 +687,10 @@ static int ich_spi_probe(struct udevice *bus)
>                 struct ich9_spi_regs *ich9_spi;
>
>                 ich9_spi = priv->base;
> -               bios_cntl = ich_readb(priv, ich9_spi->bcr);
> +               bios_cntl = readb(&ich9_spi->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);
> +               writeb(bios_cntl, &ich9_spi->bcr);
>         } else {
>                 pci_read_config_byte(plat->dev, 0xdc, &bios_cntl);
>                 if (plat->ich_version == 9)
> --

Reviewed-by: Bin Meng <bmeng.cn@gmail.com>

[U-Boot] [PATCH 05/11] dm: spi: Enable environment for minnowmax

[Bin Meng]

On Sun, Jun 7, 2015 at 10:50 PM, Simon Glass <sjg@chromium.org> wrote:
> Enable a SPI environment and store it in a suitable place.
>
> Signed-off-by: Simon Glass <sjg@chromium.org>
> ---
>
>  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 */
> --

Reviewed-by: Bin Meng <bmeng.cn@gmail.com>

[U-Boot] [PATCH 06/11] x86: Add ROM image description for minnowmax

[Bin Meng]

Hi Simon,

On Sun, Jun 7, 2015 at 10:50 PM, 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>
> ---
>
>  doc/README.x86 | 17 +++++++++++++++++
>  1 file changed, 17 insertions(+)
>
> diff --git a/doc/README.x86 b/doc/README.x86
> index c19f4a0..596b73c 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 iftool

iftool -> 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   Emvironment         CONFIG_ENV_OFFSET

Environment

> +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
> --

Regards,
Bin

[U-Boot] [PATCH 07/11] x86: pci: Tidy up the generic x86 PCI driver

[Bin Meng]

Hi Simon,

On Sun, Jun 7, 2015 at 10:50 PM, 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>
> ---
>
>  drivers/pci/pci_x86.c | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/pci/pci_x86.c b/drivers/pci/pci_x86.c
> index 901bdca..9f842c3 100644
> --- a/drivers/pci/pci_x86.c
> +++ b/drivers/pci/pci_x86.c
> @@ -7,12 +7,15 @@
>  #include <common.h>
>  #include <dm.h>
>  #include <pci.h>
> +#include <asm/pci.h>
>
>  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?

> +       .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.

>  };
>
>  static const struct udevice_id x86_pci_ids[] = {

Can we rename this to pci_x86_ids?

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

Regards,
Bin

[U-Boot] [PATCH 08/11] dm: x86: minnowmax: Move PCI to use driver model

[Bin Meng]

On Sun, Jun 7, 2015 at 10:50 PM, Simon Glass <sjg@chromium.org> wrote:
> 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 <sjg@chromium.org>
> ---
>
>  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
> --

Reviewed-by: Bin Meng <bmeng.cn@gmail.com>

[U-Boot] [PATCH 11/11] dm: x86: baytrail: Correct PCI region 3 when driver model is used

[Bin Meng]

Hi Simon,

On Sun, Jun 7, 2015 at 10:50 PM, 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>
> ---
>
>  drivers/pci/pci-uclass.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/pci/pci-uclass.c b/drivers/pci/pci-uclass.c
> index edec93f..4255c02 100644
> --- a/drivers/pci/pci-uclass.c
> +++ b/drivers/pci/pci-uclass.c
> @@ -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);
>
>         return 0;
>  }
> --

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

Regards,
Bin

[U-Boot] [PATCH 00/11] dm: x86: PCI/SPI fixes for minnowboard MAX

[Bin Meng]

Hi Simon,

On Sun, Jun 7, 2015 at 10:50 PM, Simon Glass <sjg@chromium.org> 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.
>
>
> 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
>

I've finished review for the series except #9 and #10 which are driver
model related as I am not quite familiar with the dm stuff.

[snip]

Regards,
Bin

[U-Boot] [PATCH 01/11] spi: sf: Print the error code on failure

[Marek Vasut]

On Sunday, June 07, 2015 at 04:50:32 PM, Simon Glass wrote:
> Rather than just 'ERROR', display the error code, which may be useful, at
> least with driver model.
> 
> Signed-off-by: Simon Glass <sjg@chromium.org>

Acked-by: Marek Vasut <marex@denx.de>

Best regards,
Marek Vasut

[U-Boot] [PATCH 11/11] dm: x86: baytrail: Correct PCI region 3 when driver model is used

[Andrew Bradford]

Hi Bin / Simon,

On 06/08 10:57, Bin Meng wrote:
> Hi Simon,
> 
> On Sun, Jun 7, 2015 at 10:50 PM, 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>
> > ---
> >
> >  drivers/pci/pci-uclass.c | 3 ++-
> >  1 file changed, 2 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/pci/pci-uclass.c b/drivers/pci/pci-uclass.c
> > index edec93f..4255c02 100644
> > --- a/drivers/pci/pci-uclass.c
> > +++ b/drivers/pci/pci-uclass.c
> > @@ -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);
> >
> >         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?

Thanks,
Andrew

[U-Boot] [PATCH 01/11] spi: sf: Print the error code on failure

[Jagan Teki]

On 8 June 2015 at 13:55, Marek Vasut <marex@denx.de> wrote:
> On Sunday, June 07, 2015 at 04:50:32 PM, Simon Glass wrote:
>> Rather than just 'ERROR', display the error code, which may be useful, at
>> least with driver model.
>>
>> Signed-off-by: Simon Glass <sjg@chromium.org>
>
> Acked-by: Marek Vasut <marex@denx.de>

Reviewed-by: Jagan Teki <jteki@openedev.com>

thanks!
-- 
Jagan | Openedev.

[U-Boot] [PATCH 02/11] dm: spi: Correct minor nits in ICH driver

[Jagan Teki]

On 8 June 2015 at 06:58, Bin Meng <bmeng.cn@gmail.com> wrote:
> On Sun, Jun 7, 2015 at 10:50 PM, Simon Glass <sjg@chromium.org> wrote:
>> Tidy up three minor problems in this file.
>>
>> Signed-off-by: Simon Glass <sjg@chromium.org>
>> ---
>>
>>  drivers/spi/ich.c | 5 ++---
>>  1 file changed, 2 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/spi/ich.c b/drivers/spi/ich.c
>> index 50354fd..6b6cfbf 100644
>> --- a/drivers/spi/ich.c
>> +++ b/drivers/spi/ich.c
>> @@ -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;
>>  }
>>
>> -
>>  /*
>>   * 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 <bmeng.cn@gmail.com>

Reviewed-by: Jagan Teki <jteki@openedev.com>

thanks!
-- 
Jagan | Openedev.

[U-Boot] [PATCH 04/11] dm: spi: Correct BIOS protection logic for ICH9

[Jagan Teki]

Hi Simon,

On 7 June 2015 at 20:20, 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>
> ---
>
>  drivers/spi/ich.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/spi/ich.c b/drivers/spi/ich.c
> index a8b4d0d..784320f 100644
> --- a/drivers/spi/ich.c
> +++ b/drivers/spi/ich.c
> @@ -687,10 +687,10 @@ static int ich_spi_probe(struct udevice *bus)
>                 struct ich9_spi_regs *ich9_spi;
>
>                 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.

>                 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
>

thanks!
-- 
Jagan | Openedev.

[U-Boot] [PATCH 05/11] dm: spi: Enable environment for minnowmax

[Jagan Teki]

On 8 June 2015 at 07:29, Bin Meng <bmeng.cn@gmail.com> wrote:
> On Sun, Jun 7, 2015 at 10:50 PM, Simon Glass <sjg@chromium.org> wrote:
>> Enable a SPI environment and store it in a suitable place.
>>
>> Signed-off-by: Simon Glass <sjg@chromium.org>
>> ---
>>
>>  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 */
>> --
>
> Reviewed-by: Bin Meng <bmeng.cn@gmail.com>

Reviewed-by: Jagan Teki <jteki@openedev.com>

thanks!
-- 
Jagan | Openedev.

[U-Boot] [PATCH 04/11] dm: spi: Correct BIOS protection logic for ICH9

[Bin Meng]

Hi Jagan,

On Tue, Jun 9, 2015 at 1:58 AM, Jagan Teki <jteki@openedev.com> wrote:
> Hi Simon,
>
> On 7 June 2015 at 20:20, 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>
>> ---
>>
>>  drivers/spi/ich.c | 4 ++--
>>  1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/spi/ich.c b/drivers/spi/ich.c
>> index a8b4d0d..784320f 100644
>> --- a/drivers/spi/ich.c
>> +++ b/drivers/spi/ich.c
>> @@ -687,10 +687,10 @@ static int ich_spi_probe(struct udevice *bus)
>>                 struct ich9_spi_regs *ich9_spi;
>>
>>                 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?
>

This is because ich9_spi->bcr is the full address (i.e. not just
offset of the register like the others in this driver file)

> 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.
>

Then we need make ich9_spi->bcr become its offset, so the private
calls can be used.

>>                 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)
>> --

Regards,
Bin

[U-Boot] [PATCH 01/11] spi: sf: Print the error code on failure

[Simon Glass]

On 8 June 2015 at 11:49, Jagan Teki <jteki@openedev.com> wrote:
> On 8 June 2015 at 13:55, Marek Vasut <marex@denx.de> wrote:
>> On Sunday, June 07, 2015 at 04:50:32 PM, Simon Glass wrote:
>>> Rather than just 'ERROR', display the error code, which may be useful, at
>>> least with driver model.
>>>
>>> Signed-off-by: Simon Glass <sjg@chromium.org>
>>
>> Acked-by: Marek Vasut <marex@denx.de>
>
> Reviewed-by: Jagan Teki <jteki@openedev.com>

Since this is part of a bug-fix series I'll apply it now.

Applied to u-boot-x86.

[U-Boot] [PATCH 02/11] dm: spi: Correct minor nits in ICH driver

[Simon Glass]

On 8 June 2015 at 11:50, Jagan Teki <jteki@openedev.com> wrote:
> On 8 June 2015 at 06:58, Bin Meng <bmeng.cn@gmail.com> wrote:
>> On Sun, Jun 7, 2015 at 10:50 PM, Simon Glass <sjg@chromium.org> wrote:
>>> Tidy up three minor problems in this file.
>>>
>>> Signed-off-by: Simon Glass <sjg@chromium.org>
>>> ---
>>>
>>>  drivers/spi/ich.c | 5 ++---
>>>  1 file changed, 2 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/drivers/spi/ich.c b/drivers/spi/ich.c
>>> index 50354fd..6b6cfbf 100644
>>> --- a/drivers/spi/ich.c
>>> +++ b/drivers/spi/ich.c
>>> @@ -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;
>>>  }
>>>
>>> -
>>>  /*
>>>   * 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 <bmeng.cn@gmail.com>
>
> Reviewed-by: Jagan Teki <jteki@openedev.com>

Since this is part of a bug-fix series I'll apply it now.

Applied to u-boot-x86.

[U-Boot] [PATCH 07/11] x86: pci: Tidy up the generic x86 PCI driver

[Simon Glass]

Hi Bin,

On 7 June 2015 at 20:15, Bin Meng <bmeng.cn@gmail.com> wrote:
> Hi Simon,
>
> On Sun, Jun 7, 2015 at 10:50 PM, 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>
>> ---
>>
>>  drivers/pci/pci_x86.c | 5 ++++-
>>  1 file changed, 4 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/pci/pci_x86.c b/drivers/pci/pci_x86.c
>> index 901bdca..9f842c3 100644
>> --- a/drivers/pci/pci_x86.c
>> +++ b/drivers/pci/pci_x86.c
>> @@ -7,12 +7,15 @@
>>  #include <common.h>
>>  #include <dm.h>
>>  #include <pci.h>
>> +#include <asm/pci.h>
>>
>>  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

>
>> +       .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.

>
>>  };
>>
>>  static const struct udevice_id x86_pci_ids[] = {
>
> Can we rename this to pci_x86_ids?

OK

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

Regards,
Simon

[U-Boot] [PATCH 11/11] dm: x86: baytrail: Correct PCI region 3 when driver model is used

[Simon Glass]

Hi,

On 8 June 2015 at 06:32, Andrew Bradford <andrew@bradfordembedded.com> wrote:
> Hi Bin / Simon,
>
> On 06/08 10:57, Bin Meng wrote:
>> Hi Simon,
>>
>> On Sun, Jun 7, 2015 at 10:50 PM, 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>
>> > ---
>> >
>> >  drivers/pci/pci-uclass.c | 3 ++-
>> >  1 file changed, 2 insertions(+), 1 deletion(-)
>> >
>> > diff --git a/drivers/pci/pci-uclass.c b/drivers/pci/pci-uclass.c
>> > index edec93f..4255c02 100644
>> > --- a/drivers/pci/pci-uclass.c
>> > +++ b/drivers/pci/pci-uclass.c
>> > @@ -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);
>> >
>> >         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.

Regards,
Simon

[U-Boot] [PATCH 09/11] dm: pci: Use the correct hose when configuring devices

[Simon Glass]

On 7 June 2015 at 08:50, Simon Glass <sjg@chromium.org> wrote:
> 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 <sjg@chromium.org>
> ---
>
>  drivers/pci/pci-uclass.c |  6 +++++-
>  drivers/pci/pci_common.c |  6 ++++++
>  include/pci.h            | 10 ++++++++++
>  3 files changed, 21 insertions(+), 1 deletion(-)

Applied to u-boot-86.

[U-Boot] [PATCH 10/11] dm: pci: Correct bus number when scanning sub-buses

[Simon Glass]

On 7 June 2015 at 08:50, Simon Glass <sjg@chromium.org> wrote:
> 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 <sjg@chromium.org>
> ---
>
>  drivers/pci/pci-uclass.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)

Applied to u-boot-86.

[U-Boot] [PATCH 07/11] x86: pci: Tidy up the generic x86 PCI driver

[Bin Meng]

Hi Simon,

On Wed, Jun 24, 2015 at 11:18 AM, Simon Glass <sjg@chromium.org> wrote:
> Hi Bin,
>
> On 7 June 2015 at 20:15, Bin Meng <bmeng.cn@gmail.com> wrote:
>> Hi Simon,
>>
>> On Sun, Jun 7, 2015 at 10:50 PM, 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>
>>> ---
>>>
>>>  drivers/pci/pci_x86.c | 5 ++++-
>>>  1 file changed, 4 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/pci/pci_x86.c b/drivers/pci/pci_x86.c
>>> index 901bdca..9f842c3 100644
>>> --- a/drivers/pci/pci_x86.c
>>> +++ b/drivers/pci/pci_x86.c
>>> @@ -7,12 +7,15 @@
>>>  #include <common.h>
>>>  #include <dm.h>
>>>  #include <pci.h>
>>> +#include <asm/pci.h>
>>>
>>>  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
>
>>
>>> +       .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.
>

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]

Regards,
Bin

[U-Boot] [PATCH 07/11] x86: pci: Tidy up the generic x86 PCI driver

[Simon Glass]

Hi Bin,

On 23 June 2015 at 21:46, Bin Meng <bmeng.cn@gmail.com> wrote:
> Hi Simon,
>
> On Wed, Jun 24, 2015 at 11:18 AM, Simon Glass <sjg@chromium.org> wrote:
>> Hi Bin,
>>
>> On 7 June 2015 at 20:15, Bin Meng <bmeng.cn@gmail.com> wrote:
>>> Hi Simon,
>>>
>>> On Sun, Jun 7, 2015 at 10:50 PM, 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>
>>>> ---
>>>>
>>>>  drivers/pci/pci_x86.c | 5 ++++-
>>>>  1 file changed, 4 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/drivers/pci/pci_x86.c b/drivers/pci/pci_x86.c
>>>> index 901bdca..9f842c3 100644
>>>> --- a/drivers/pci/pci_x86.c
>>>> +++ b/drivers/pci/pci_x86.c
>>>> @@ -7,12 +7,15 @@
>>>>  #include <common.h>
>>>>  #include <dm.h>
>>>>  #include <pci.h>
>>>> +#include <asm/pci.h>
>>>>
>>>>  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
>>
>>>
>>>> +       .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.
>>
>
> 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]

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.

Regards,
Simon

[U-Boot] [PATCH 07/11] x86: pci: Tidy up the generic x86 PCI driver

[Bin Meng]

Hi Simon,

On Wed, Jun 24, 2015 at 11:54 AM, Simon Glass <sjg@chromium.org> wrote:
> Hi Bin,
>
> On 23 June 2015 at 21:46, Bin Meng <bmeng.cn@gmail.com> wrote:
>> Hi Simon,
>>
>> On Wed, Jun 24, 2015 at 11:18 AM, Simon Glass <sjg@chromium.org> wrote:
>>> Hi Bin,
>>>
>>> On 7 June 2015 at 20:15, Bin Meng <bmeng.cn@gmail.com> wrote:
>>>> Hi Simon,
>>>>
>>>> On Sun, Jun 7, 2015 at 10:50 PM, 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>
>>>>> ---
>>>>>
>>>>>  drivers/pci/pci_x86.c | 5 ++++-
>>>>>  1 file changed, 4 insertions(+), 1 deletion(-)
>>>>>
>>>>> diff --git a/drivers/pci/pci_x86.c b/drivers/pci/pci_x86.c
>>>>> index 901bdca..9f842c3 100644
>>>>> --- a/drivers/pci/pci_x86.c
>>>>> +++ b/drivers/pci/pci_x86.c
>>>>> @@ -7,12 +7,15 @@
>>>>>  #include <common.h>
>>>>>  #include <dm.h>
>>>>>  #include <pci.h>
>>>>> +#include <asm/pci.h>
>>>>>
>>>>>  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
>>>
>>>>
>>>>> +       .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.
>>>
>>
>> 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]
>
> 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?

Regards,
Bin

[U-Boot] [PATCH 11/11] dm: x86: baytrail: Correct PCI region 3 when driver model is used

[Bin Meng]

Hi Simon,

On Wed, Jun 24, 2015 at 11:23 AM, Simon Glass <sjg@chromium.org> wrote:
> Hi,
>
> On 8 June 2015 at 06:32, Andrew Bradford <andrew@bradfordembedded.com> wrote:
>> Hi Bin / Simon,
>>
>> On 06/08 10:57, Bin Meng wrote:
>>> Hi Simon,
>>>
>>> On Sun, Jun 7, 2015 at 10:50 PM, 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>
>>> > ---
>>> >
>>> >  drivers/pci/pci-uclass.c | 3 ++-
>>> >  1 file changed, 2 insertions(+), 1 deletion(-)
>>> >
>>> > diff --git a/drivers/pci/pci-uclass.c b/drivers/pci/pci-uclass.c
>>> > index edec93f..4255c02 100644
>>> > --- a/drivers/pci/pci-uclass.c
>>> > +++ b/drivers/pci/pci-uclass.c
>>> > @@ -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);
>>> >
>>> >         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?
>

Yep, maybe not a big deal, but it may also break some boards if the
allocated buffer for PCI device happens to be above 2 GiB, but that
case might be rare (ie: like 3 GiB memory is installed where on most
cases we only see boards with 1 GiB, 2 GiB, 4 GiB memory)

> 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.

Adding pci_max_addr to global_data sounds good if we want to avoid
#ifdefs. It would be good to hear more comments from other domains?

Regards,
Bin

[U-Boot] [PATCH 07/11] x86: pci: Tidy up the generic x86 PCI driver

[Simon Glass]

Hi Bin,

On 23 June 2015 at 21:59, Bin Meng <bmeng.cn@gmail.com> wrote:
> Hi Simon,
>
> On Wed, Jun 24, 2015 at 11:54 AM, Simon Glass <sjg@chromium.org> wrote:
>> Hi Bin,
>>
>> On 23 June 2015 at 21:46, Bin Meng <bmeng.cn@gmail.com> wrote:
>>> Hi Simon,
>>>
>>> On Wed, Jun 24, 2015 at 11:18 AM, Simon Glass <sjg@chromium.org> wrote:
>>>> Hi Bin,
>>>>
>>>> On 7 June 2015 at 20:15, Bin Meng <bmeng.cn@gmail.com> wrote:
>>>>> Hi Simon,
>>>>>
>>>>> On Sun, Jun 7, 2015 at 10:50 PM, 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>
>>>>>> ---
>>>>>>
>>>>>>  drivers/pci/pci_x86.c | 5 ++++-
>>>>>>  1 file changed, 4 insertions(+), 1 deletion(-)
>>>>>>
>>>>>> diff --git a/drivers/pci/pci_x86.c b/drivers/pci/pci_x86.c
>>>>>> index 901bdca..9f842c3 100644
>>>>>> --- a/drivers/pci/pci_x86.c
>>>>>> +++ b/drivers/pci/pci_x86.c
>>>>>> @@ -7,12 +7,15 @@
>>>>>>  #include <common.h>
>>>>>>  #include <dm.h>
>>>>>>  #include <pci.h>
>>>>>> +#include <asm/pci.h>
>>>>>>
>>>>>>  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
>>>>
>>>>>
>>>>>> +       .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.
>>>>
>>>
>>> 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]
>>
>> 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.

Regards,
Simon

[U-Boot] [PATCH 07/11] x86: pci: Tidy up the generic x86 PCI driver

[Simon Glass]

Hi Bin,

On 24 June 2015 at 09:15, Simon Glass <sjg@chromium.org> wrote:
> Hi Bin,
>
> On 23 June 2015 at 21:59, Bin Meng <bmeng.cn@gmail.com> wrote:
>> Hi Simon,
>>
>> On Wed, Jun 24, 2015 at 11:54 AM, Simon Glass <sjg@chromium.org> wrote:
>>> Hi Bin,
>>>
>>> On 23 June 2015 at 21:46, Bin Meng <bmeng.cn@gmail.com> wrote:
>>>> Hi Simon,
>>>>
>>>> On Wed, Jun 24, 2015 at 11:18 AM, Simon Glass <sjg@chromium.org> wrote:
>>>>> Hi Bin,
>>>>>
>>>>> On 7 June 2015 at 20:15, Bin Meng <bmeng.cn@gmail.com> wrote:
>>>>>> Hi Simon,
>>>>>>
>>>>>> On Sun, Jun 7, 2015 at 10:50 PM, 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>
>>>>>>> ---
>>>>>>>
>>>>>>>  drivers/pci/pci_x86.c | 5 ++++-
>>>>>>>  1 file changed, 4 insertions(+), 1 deletion(-)
>>>>>>>
>>>>>>> diff --git a/drivers/pci/pci_x86.c b/drivers/pci/pci_x86.c
>>>>>>> index 901bdca..9f842c3 100644
>>>>>>> --- a/drivers/pci/pci_x86.c
>>>>>>> +++ b/drivers/pci/pci_x86.c
>>>>>>> @@ -7,12 +7,15 @@
>>>>>>>  #include <common.h>
>>>>>>>  #include <dm.h>
>>>>>>>  #include <pci.h>
>>>>>>> +#include <asm/pci.h>
>>>>>>>
>>>>>>>  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
>>>>>
>>>>>>
>>>>>>> +       .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.
>>>>>
>>>>
>>>> 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]
>>>
>>> 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.

I think the problem is that cpu/ivybridge/pci.c has its own driver and
wants to call the pci_x86_read/write_config() functions. So they
really need to be in a generic area. I was thinking about coreboot
yesterday, but the problem is ivybridge.

Regards,
Simon