[U-Boot] [PATCH 1/5] x86: ich-spi: Remove spi_write_protect_region()

[U-Boot] [PATCH 1/5] x86: ich-spi: Remove spi_write_protect_region()

[Bin Meng]

This routine is not called anywhere.

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

 drivers/spi/ich.c | 50 --------------------------------------------------
 1 file changed, 50 deletions(-)

diff --git a/drivers/spi/ich.c b/drivers/spi/ich.c
index bf2e99b..46dd9a8 100644
--- a/drivers/spi/ich.c
+++ b/drivers/spi/ich.c
@@ -539,56 +539,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
- * don't actually take effect until the HSFS[FLOCKDN] bit is set, but that's
- * done elsewhere.
- */
-int spi_write_protect_region(struct udevice *dev, uint32_t lower_limit,
-			     uint32_t length, int hint)
-{
-	struct udevice *bus = dev->parent;
-	struct ich_spi_priv *ctlr = dev_get_priv(bus);
-	uint32_t tmplong;
-	uint32_t upper_limit;
-
-	if (!ctlr->pr) {
-		printf("%s: operation not supported on this chipset\n",
-		       __func__);
-		return -ENOSYS;
-	}
-
-	if (length == 0 ||
-	    lower_limit > (0xFFFFFFFFUL - length) + 1 ||
-	    hint < 0 || hint > 4) {
-		printf("%s(0x%x, 0x%x, %d): invalid args\n", __func__,
-		       lower_limit, length, hint);
-		return -EPERM;
-	}
-
-	upper_limit = lower_limit + length - 1;
-
-	/*
-	 * Determine bits to write, as follows:
-	 *  31     Write-protection enable (includes erase operation)
-	 *  30:29  reserved
-	 *  28:16  Upper Limit (FLA address bits 24:12, with 11:0 == 0xfff)
-	 *  15     Read-protection enable
-	 *  14:13  reserved
-	 *  12:0   Lower Limit (FLA address bits 24:12, with 11:0 == 0x000)
-	 */
-	tmplong = 0x80000000 |
-		((upper_limit & 0x01fff000) << 4) |
-		((lower_limit & 0x01fff000) >> 12);
-
-	printf("%s: writing 0x%08x to %p\n", __func__, tmplong,
-	       &ctlr->pr[hint]);
-	ctlr->pr[hint] = tmplong;
-
-	return 0;
-}
-
 static int ich_spi_probe(struct udevice *dev)
 {
 	struct ich_spi_platdata *plat = dev_get_platdata(dev);
-- 
2.9.2

[U-Boot] [PATCH 2/5] x86: ich-spi: Remove unnecessary assignment in ich_init_controller()

[Bin Meng]

There is no need to do another assignment to ich7_spi.

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

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

diff --git a/drivers/spi/ich.c b/drivers/spi/ich.c
index 46dd9a8..909eefc 100644
--- a/drivers/spi/ich.c
+++ b/drivers/spi/ich.c
@@ -126,7 +126,6 @@ static int ich_init_controller(struct udevice *dev,
 	if (plat->ich_version == ICHV_7) {
 		struct ich7_spi_regs *ich7_spi = sbase;
 
-		ich7_spi = (struct ich7_spi_regs *)sbase;
 		ctlr->ichspi_lock = readw(&ich7_spi->spis) & SPIS_LOCK;
 		ctlr->opmenu = offsetof(struct ich7_spi_regs, opmenu);
 		ctlr->menubytes = sizeof(ich7_spi->opmenu);
-- 
2.9.2

[U-Boot] [PATCH 3/5] x86: ich-spi: Don't read cached lock status

[Bin Meng]

At present the ICH SPI controller driver reads the controller lock
status from its register in the probe routine and saves the lock
status to a member of priv. Later the driver uses the cached status
from priv to judge whether the controller setting is locked and do
different setup.

But such logic is only valid when there is only the SPI controller
driver that touches the SPI hardware. In fact the lock status change
can be trigged outside the driver, eg: during the fsp_notify() call
when Intel FSP is used.

This changes the driver to read the lock status every time when an
SPI transfer is initiated instead of reading the cached one.

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

 drivers/spi/ich.c | 29 +++++++++++++++++++++++------
 drivers/spi/ich.h |  2 --
 2 files changed, 23 insertions(+), 8 deletions(-)

diff --git a/drivers/spi/ich.c b/drivers/spi/ich.c
index 909eefc..d4888f5 100644
--- a/drivers/spi/ich.c
+++ b/drivers/spi/ich.c
@@ -126,7 +126,6 @@ static int ich_init_controller(struct udevice *dev,
 	if (plat->ich_version == ICHV_7) {
 		struct ich7_spi_regs *ich7_spi = sbase;
 
-		ctlr->ichspi_lock = readw(&ich7_spi->spis) & SPIS_LOCK;
 		ctlr->opmenu = offsetof(struct ich7_spi_regs, opmenu);
 		ctlr->menubytes = sizeof(ich7_spi->opmenu);
 		ctlr->optype = offsetof(struct ich7_spi_regs, optype);
@@ -141,7 +140,6 @@ static int ich_init_controller(struct udevice *dev,
 	} else if (plat->ich_version == ICHV_9) {
 		struct ich9_spi_regs *ich9_spi = sbase;
 
-		ctlr->ichspi_lock = readw(&ich9_spi->hsfs) & HSFS_FLOCKDN;
 		ctlr->opmenu = offsetof(struct ich9_spi_regs, opmenu);
 		ctlr->menubytes = sizeof(ich9_spi->opmenu);
 		ctlr->optype = offsetof(struct ich9_spi_regs, optype);
@@ -186,6 +184,23 @@ static inline void spi_use_in(struct spi_trans *trans, unsigned bytes)
 	trans->bytesin -= bytes;
 }
 
+static bool spi_lock_status(struct ich_spi_platdata *plat, void *sbase)
+{
+	int lock = 0;
+
+	if (plat->ich_version == ICHV_7) {
+		struct ich7_spi_regs *ich7_spi = sbase;
+
+		lock = readw(&ich7_spi->spis) & SPIS_LOCK;
+	} else if (plat->ich_version == ICHV_9) {
+		struct ich9_spi_regs *ich9_spi = sbase;
+
+		lock = readw(&ich9_spi->hsfs) & HSFS_FLOCKDN;
+	}
+
+	return lock != 0;
+}
+
 static void spi_setup_type(struct spi_trans *trans, int data_bytes)
 {
 	trans->type = 0xFF;
@@ -219,14 +234,15 @@ static void spi_setup_type(struct spi_trans *trans, int data_bytes)
 	}
 }
 
-static int spi_setup_opcode(struct ich_spi_priv *ctlr, struct spi_trans *trans)
+static int spi_setup_opcode(struct ich_spi_priv *ctlr, struct spi_trans *trans,
+			    bool lock)
 {
 	uint16_t optypes;
 	uint8_t opmenu[ctlr->menubytes];
 
 	trans->opcode = trans->out[0];
 	spi_use_out(trans, 1);
-	if (!ctlr->ichspi_lock) {
+	if (!lock) {
 		/* The lock is off, so just use index 0. */
 		ich_writeb(ctlr, trans->opcode, ctlr->opmenu);
 		optypes = ich_readw(ctlr, ctlr->optype);
@@ -336,6 +352,7 @@ static int ich_spi_xfer(struct udevice *dev, unsigned int bitlen,
 	struct spi_trans *trans = &ctlr->trans;
 	unsigned type = flags & (SPI_XFER_BEGIN | SPI_XFER_END);
 	int using_cmd = 0;
+	bool lock = spi_lock_status(plat, ctlr->base);
 	int ret;
 
 	/* We don't support writing partial bytes */
@@ -399,7 +416,7 @@ static int ich_spi_xfer(struct udevice *dev, unsigned int bitlen,
 		ich_writeb(ctlr, SPIS_CDS | SPIS_FCERR, ctlr->status);
 
 	spi_setup_type(trans, using_cmd ? bytes : 0);
-	opcode_index = spi_setup_opcode(ctlr, trans);
+	opcode_index = spi_setup_opcode(ctlr, trans, lock);
 	if (opcode_index < 0)
 		return -EINVAL;
 	with_address = spi_setup_offset(trans);
@@ -412,7 +429,7 @@ static int ich_spi_xfer(struct udevice *dev, unsigned int bitlen,
 		 * in order to prevent the Management Engine from
 		 * issuing a transaction between WREN and DATA.
 		 */
-		if (!ctlr->ichspi_lock)
+		if (!lock)
 			ich_writew(ctlr, trans->opcode, ctlr->preop);
 		return 0;
 	}
diff --git a/drivers/spi/ich.h b/drivers/spi/ich.h
index dcb8a90..c867c57 100644
--- a/drivers/spi/ich.h
+++ b/drivers/spi/ich.h
@@ -177,8 +177,6 @@ struct ich_spi_platdata {
 };
 
 struct ich_spi_priv {
-	int ichspi_lock;
-	int locked;
 	int opmenu;
 	int menubytes;
 	void *base;		/* Base of register set */
-- 
2.9.2

[U-Boot] [PATCH 4/5] x86: ich-spi: Move opcode registers configuration to another routine

[Bin Meng]

At present the ICH SPI opcode registers configuration is done in the
ich_spi_remove() routine, a little bit weird but that's how current.
Linux MTD driver works. This changes to move the opcode registers
configuration to a separate routine ich_spi_config_opcode() which
might be called by U-Boot itself as well.

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

 drivers/spi/ich.c | 22 ++++++++++++++++------
 1 file changed, 16 insertions(+), 6 deletions(-)

diff --git a/drivers/spi/ich.c b/drivers/spi/ich.c
index d4888f5..373bc26 100644
--- a/drivers/spi/ich.c
+++ b/drivers/spi/ich.c
@@ -338,6 +338,21 @@ static int ich_status_poll(struct ich_spi_priv *ctlr, u16 bitmask,
 	return -ETIMEDOUT;
 }
 
+void ich_spi_config_opcode(struct udevice *dev)
+{
+	struct ich_spi_priv *ctlr = dev_get_priv(dev);
+
+	/*
+	 * PREOP, OPTYPE, OPMENU1/OPMENU2 registers can be locked down
+	 * to prevent accidental or intentional writes. Before they get
+	 * locked down, these registers should be initialized properly.
+	 */
+	ich_writew(ctlr, SPI_OPPREFIX, ctlr->preop);
+	ich_writew(ctlr, SPI_OPTYPE, ctlr->optype);
+	ich_writel(ctlr, SPI_OPMENU_LOWER, ctlr->opmenu);
+	ich_writel(ctlr, SPI_OPMENU_UPPER, ctlr->opmenu + sizeof(u32));
+}
+
 static int ich_spi_xfer(struct udevice *dev, unsigned int bitlen,
 			const void *dout, void *din, unsigned long flags)
 {
@@ -585,16 +600,11 @@ static int ich_spi_probe(struct udevice *dev)
 
 static int ich_spi_remove(struct udevice *bus)
 {
-	struct ich_spi_priv *ctlr = dev_get_priv(bus);
-
 	/*
 	 * Configure SPI controller so that the Linux MTD driver can fully
 	 * access the SPI NOR chip
 	 */
-	ich_writew(ctlr, SPI_OPPREFIX, ctlr->preop);
-	ich_writew(ctlr, SPI_OPTYPE, ctlr->optype);
-	ich_writel(ctlr, SPI_OPMENU_LOWER, ctlr->opmenu);
-	ich_writel(ctlr, SPI_OPMENU_UPPER, ctlr->opmenu + sizeof(u32));
+	ich_spi_config_opcode(bus);
 
 	return 0;
 }
-- 
2.9.2

[U-Boot] [PATCH 5/5] x86: fsp: Configure SPI opcode registers before SPI is locked down

[Bin Meng]

Some Intel FSP (like Braswell) does SPI lock-down during the call
to fsp_notify(INIT_PHASE_BOOT). But before SPI lock-down is done,
it's bootloader's responsibility to configure the SPI controller's
opcode registers properly otherwise SPI controller driver doesn't
know how to communicate with the SPI flash device.

This introduces a Kconfig option CONFIG_FSP_LOCKDOWN_SPI for such
FSPs. When it is on, U-Boot will configure the SPI opcode registers
before the lock-down.

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

 arch/x86/Kconfig              |  9 +++++++++
 arch/x86/lib/fsp/fsp_common.c | 24 ++++++++++++++++++++++++
 2 files changed, 33 insertions(+)

diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig
index c26710b..5373082 100644
--- a/arch/x86/Kconfig
+++ b/arch/x86/Kconfig
@@ -401,6 +401,15 @@ config FSP_BROKEN_HOB
 	  do not overwrite the important boot service data which is used by
 	  FSP, otherwise the subsequent call to fsp_notify() will fail.
 
+config FSP_LOCKDOWN_SPI
+	bool
+	depends on HAVE_FSP
+	help
+	  Some Intel FSP (like Braswell) does SPI lock-down during the call
+	  to fsp_notify(INIT_PHASE_BOOT). This option should be turned on
+	  for such FSP and U-Boot will configure the SPI opcode registers
+	  before the lock-down.
+
 config ENABLE_MRC_CACHE
 	bool "Enable MRC cache"
 	depends on !EFI && !SYS_COREBOOT
diff --git a/arch/x86/lib/fsp/fsp_common.c b/arch/x86/lib/fsp/fsp_common.c
index 3397bb8..320d87d 100644
--- a/arch/x86/lib/fsp/fsp_common.c
+++ b/arch/x86/lib/fsp/fsp_common.c
@@ -19,6 +19,8 @@
 
 DECLARE_GLOBAL_DATA_PTR;
 
+extern void ich_spi_config_opcode(struct udevice *dev);
+
 int checkcpu(void)
 {
 	return 0;
@@ -49,6 +51,28 @@ void board_final_cleanup(void)
 {
 	u32 status;
 
+#ifdef CONFIG_FSP_LOCKDOWN_SPI
+	struct udevice *dev;
+
+	/*
+	 * Some Intel FSP (like Braswell) does SPI lock-down during the call
+	 * to fsp_notify(INIT_PHASE_BOOT). But before SPI lock-down is done,
+	 * it's bootloader's responsibility to configure the SPI controller's
+	 * opcode registers properly otherwise SPI controller driver doesn't
+	 * know how to communicate with the SPI flash device.
+	 *
+	 * Note we cannot do such configuration elsewhere (eg: during the SPI
+	 * controller driver's probe() routine), becasue:
+	 *
+	 * 1). U-Boot SPI controller driver does not set the lock-down bit
+	 * 2). Any SPI transfer will corrupt the contents of these registers
+	 *
+	 * Hence we have to do it right here before SPI lock-down bit is set.
+	 */
+	if (!uclass_first_device_err(UCLASS_SPI, &dev))
+		ich_spi_config_opcode(dev);
+#endif
+
 	/* call into FspNotify */
 	debug("Calling into FSP (notify phase INIT_PHASE_BOOT): ");
 	status = fsp_notify(NULL, INIT_PHASE_BOOT);
-- 
2.9.2

[U-Boot] [PATCH 1/5] x86: ich-spi: Remove spi_write_protect_region()

[Stefan Roese]

On 16.08.2017 07:38, Bin Meng wrote:
> This routine is not called anywhere.
> 
> Signed-off-by: Bin Meng <bmeng.cn@gmail.com>
> ---
> 
>   drivers/spi/ich.c | 50 --------------------------------------------------
>   1 file changed, 50 deletions(-)
> 
> diff --git a/drivers/spi/ich.c b/drivers/spi/ich.c
> index bf2e99b..46dd9a8 100644
> --- a/drivers/spi/ich.c
> +++ b/drivers/spi/ich.c
> @@ -539,56 +539,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
> - * don't actually take effect until the HSFS[FLOCKDN] bit is set, but that's
> - * done elsewhere.
> - */
> -int spi_write_protect_region(struct udevice *dev, uint32_t lower_limit,
> -			     uint32_t length, int hint)
> -{
> -	struct udevice *bus = dev->parent;
> -	struct ich_spi_priv *ctlr = dev_get_priv(bus);
> -	uint32_t tmplong;
> -	uint32_t upper_limit;
> -
> -	if (!ctlr->pr) {
> -		printf("%s: operation not supported on this chipset\n",
> -		       __func__);
> -		return -ENOSYS;
> -	}
> -
> -	if (length == 0 ||
> -	    lower_limit > (0xFFFFFFFFUL - length) + 1 ||
> -	    hint < 0 || hint > 4) {
> -		printf("%s(0x%x, 0x%x, %d): invalid args\n", __func__,
> -		       lower_limit, length, hint);
> -		return -EPERM;
> -	}
> -
> -	upper_limit = lower_limit + length - 1;
> -
> -	/*
> -	 * Determine bits to write, as follows:
> -	 *  31     Write-protection enable (includes erase operation)
> -	 *  30:29  reserved
> -	 *  28:16  Upper Limit (FLA address bits 24:12, with 11:0 == 0xfff)
> -	 *  15     Read-protection enable
> -	 *  14:13  reserved
> -	 *  12:0   Lower Limit (FLA address bits 24:12, with 11:0 == 0x000)
> -	 */
> -	tmplong = 0x80000000 |
> -		((upper_limit & 0x01fff000) << 4) |
> -		((lower_limit & 0x01fff000) >> 12);
> -
> -	printf("%s: writing 0x%08x to %p\n", __func__, tmplong,
> -	       &ctlr->pr[hint]);
> -	ctlr->pr[hint] = tmplong;
> -
> -	return 0;
> -}
> -
>   static int ich_spi_probe(struct udevice *dev)
>   {
>   	struct ich_spi_platdata *plat = dev_get_platdata(dev);
> 

Reviewed-by: Stefan Roese <sr@denx.de>

Thanks,
Stefan

[U-Boot] [PATCH 2/5] x86: ich-spi: Remove unnecessary assignment in ich_init_controller()

[Stefan Roese]

On 16.08.2017 07:38, Bin Meng wrote:
> There is no need to do another assignment to ich7_spi.
> 
> Signed-off-by: Bin Meng <bmeng.cn@gmail.com>
> ---
> 
>   drivers/spi/ich.c | 1 -
>   1 file changed, 1 deletion(-)
> 
> diff --git a/drivers/spi/ich.c b/drivers/spi/ich.c
> index 46dd9a8..909eefc 100644
> --- a/drivers/spi/ich.c
> +++ b/drivers/spi/ich.c
> @@ -126,7 +126,6 @@ static int ich_init_controller(struct udevice *dev,
>   	if (plat->ich_version == ICHV_7) {
>   		struct ich7_spi_regs *ich7_spi = sbase;
>   
> -		ich7_spi = (struct ich7_spi_regs *)sbase;
>   		ctlr->ichspi_lock = readw(&ich7_spi->spis) & SPIS_LOCK;
>   		ctlr->opmenu = offsetof(struct ich7_spi_regs, opmenu);
>   		ctlr->menubytes = sizeof(ich7_spi->opmenu);
> 

Reviewed-by: Stefan Roese <sr@denx.de>

Thanks,
Stefan

[U-Boot] [PATCH 3/5] x86: ich-spi: Don't read cached lock status

[Stefan Roese]

On 16.08.2017 07:38, Bin Meng wrote:
> At present the ICH SPI controller driver reads the controller lock
> status from its register in the probe routine and saves the lock
> status to a member of priv. Later the driver uses the cached status
> from priv to judge whether the controller setting is locked and do
> different setup.
> 
> But such logic is only valid when there is only the SPI controller
> driver that touches the SPI hardware. In fact the lock status change
> can be trigged outside the driver, eg: during the fsp_notify() call
> when Intel FSP is used.
> 
> This changes the driver to read the lock status every time when an
> SPI transfer is initiated instead of reading the cached one.
> 
> Signed-off-by: Bin Meng <bmeng.cn@gmail.com>
> ---
> 
>   drivers/spi/ich.c | 29 +++++++++++++++++++++++------
>   drivers/spi/ich.h |  2 --
>   2 files changed, 23 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/spi/ich.c b/drivers/spi/ich.c
> index 909eefc..d4888f5 100644
> --- a/drivers/spi/ich.c
> +++ b/drivers/spi/ich.c
> @@ -126,7 +126,6 @@ static int ich_init_controller(struct udevice *dev,
>   	if (plat->ich_version == ICHV_7) {
>   		struct ich7_spi_regs *ich7_spi = sbase;
>   
> -		ctlr->ichspi_lock = readw(&ich7_spi->spis) & SPIS_LOCK;
>   		ctlr->opmenu = offsetof(struct ich7_spi_regs, opmenu);
>   		ctlr->menubytes = sizeof(ich7_spi->opmenu);
>   		ctlr->optype = offsetof(struct ich7_spi_regs, optype);
> @@ -141,7 +140,6 @@ static int ich_init_controller(struct udevice *dev,
>   	} else if (plat->ich_version == ICHV_9) {
>   		struct ich9_spi_regs *ich9_spi = sbase;
>   
> -		ctlr->ichspi_lock = readw(&ich9_spi->hsfs) & HSFS_FLOCKDN;
>   		ctlr->opmenu = offsetof(struct ich9_spi_regs, opmenu);
>   		ctlr->menubytes = sizeof(ich9_spi->opmenu);
>   		ctlr->optype = offsetof(struct ich9_spi_regs, optype);
> @@ -186,6 +184,23 @@ static inline void spi_use_in(struct spi_trans *trans, unsigned bytes)
>   	trans->bytesin -= bytes;
>   }
>   
> +static bool spi_lock_status(struct ich_spi_platdata *plat, void *sbase)
> +{
> +	int lock = 0;
> +
> +	if (plat->ich_version == ICHV_7) {
> +		struct ich7_spi_regs *ich7_spi = sbase;
> +
> +		lock = readw(&ich7_spi->spis) & SPIS_LOCK;
> +	} else if (plat->ich_version == ICHV_9) {
> +		struct ich9_spi_regs *ich9_spi = sbase;
> +
> +		lock = readw(&ich9_spi->hsfs) & HSFS_FLOCKDN;
> +	}
> +
> +	return lock != 0;
> +}
> +
>   static void spi_setup_type(struct spi_trans *trans, int data_bytes)
>   {
>   	trans->type = 0xFF;
> @@ -219,14 +234,15 @@ static void spi_setup_type(struct spi_trans *trans, int data_bytes)
>   	}
>   }
>   
> -static int spi_setup_opcode(struct ich_spi_priv *ctlr, struct spi_trans *trans)
> +static int spi_setup_opcode(struct ich_spi_priv *ctlr, struct spi_trans *trans,
> +			    bool lock)
>   {
>   	uint16_t optypes;
>   	uint8_t opmenu[ctlr->menubytes];
>   
>   	trans->opcode = trans->out[0];
>   	spi_use_out(trans, 1);
> -	if (!ctlr->ichspi_lock) {
> +	if (!lock) {
>   		/* The lock is off, so just use index 0. */
>   		ich_writeb(ctlr, trans->opcode, ctlr->opmenu);
>   		optypes = ich_readw(ctlr, ctlr->optype);
> @@ -336,6 +352,7 @@ static int ich_spi_xfer(struct udevice *dev, unsigned int bitlen,
>   	struct spi_trans *trans = &ctlr->trans;
>   	unsigned type = flags & (SPI_XFER_BEGIN | SPI_XFER_END);
>   	int using_cmd = 0;
> +	bool lock = spi_lock_status(plat, ctlr->base);
>   	int ret;
>   
>   	/* We don't support writing partial bytes */
> @@ -399,7 +416,7 @@ static int ich_spi_xfer(struct udevice *dev, unsigned int bitlen,
>   		ich_writeb(ctlr, SPIS_CDS | SPIS_FCERR, ctlr->status);
>   
>   	spi_setup_type(trans, using_cmd ? bytes : 0);
> -	opcode_index = spi_setup_opcode(ctlr, trans);
> +	opcode_index = spi_setup_opcode(ctlr, trans, lock);
>   	if (opcode_index < 0)
>   		return -EINVAL;
>   	with_address = spi_setup_offset(trans);
> @@ -412,7 +429,7 @@ static int ich_spi_xfer(struct udevice *dev, unsigned int bitlen,
>   		 * in order to prevent the Management Engine from
>   		 * issuing a transaction between WREN and DATA.
>   		 */
> -		if (!ctlr->ichspi_lock)
> +		if (!lock)
>   			ich_writew(ctlr, trans->opcode, ctlr->preop);
>   		return 0;
>   	}
> diff --git a/drivers/spi/ich.h b/drivers/spi/ich.h
> index dcb8a90..c867c57 100644
> --- a/drivers/spi/ich.h
> +++ b/drivers/spi/ich.h
> @@ -177,8 +177,6 @@ struct ich_spi_platdata {
>   };
>   
>   struct ich_spi_priv {
> -	int ichspi_lock;
> -	int locked;
>   	int opmenu;
>   	int menubytes;
>   	void *base;		/* Base of register set */
> 

Reviewed-by: Stefan Roese <sr@denx.de>

Thanks,
Stefan

[U-Boot] [PATCH 4/5] x86: ich-spi: Move opcode registers configuration to another routine

[Stefan Roese]

On 16.08.2017 07:38, Bin Meng wrote:
> At present the ICH SPI opcode registers configuration is done in the
> ich_spi_remove() routine, a little bit weird but that's how current.
> Linux MTD driver works. This changes to move the opcode registers
> configuration to a separate routine ich_spi_config_opcode() which
> might be called by U-Boot itself as well.
> 
> Signed-off-by: Bin Meng <bmeng.cn@gmail.com>
> ---
> 
>   drivers/spi/ich.c | 22 ++++++++++++++++------
>   1 file changed, 16 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/spi/ich.c b/drivers/spi/ich.c
> index d4888f5..373bc26 100644
> --- a/drivers/spi/ich.c
> +++ b/drivers/spi/ich.c
> @@ -338,6 +338,21 @@ static int ich_status_poll(struct ich_spi_priv *ctlr, u16 bitmask,
>   	return -ETIMEDOUT;
>   }
>   
> +void ich_spi_config_opcode(struct udevice *dev)
> +{
> +	struct ich_spi_priv *ctlr = dev_get_priv(dev);
> +
> +	/*
> +	 * PREOP, OPTYPE, OPMENU1/OPMENU2 registers can be locked down
> +	 * to prevent accidental or intentional writes. Before they get
> +	 * locked down, these registers should be initialized properly.
> +	 */
> +	ich_writew(ctlr, SPI_OPPREFIX, ctlr->preop);
> +	ich_writew(ctlr, SPI_OPTYPE, ctlr->optype);
> +	ich_writel(ctlr, SPI_OPMENU_LOWER, ctlr->opmenu);
> +	ich_writel(ctlr, SPI_OPMENU_UPPER, ctlr->opmenu + sizeof(u32));
> +}
> +
>   static int ich_spi_xfer(struct udevice *dev, unsigned int bitlen,
>   			const void *dout, void *din, unsigned long flags)
>   {
> @@ -585,16 +600,11 @@ static int ich_spi_probe(struct udevice *dev)
>   
>   static int ich_spi_remove(struct udevice *bus)
>   {
> -	struct ich_spi_priv *ctlr = dev_get_priv(bus);
> -
>   	/*
>   	 * Configure SPI controller so that the Linux MTD driver can fully
>   	 * access the SPI NOR chip
>   	 */
> -	ich_writew(ctlr, SPI_OPPREFIX, ctlr->preop);
> -	ich_writew(ctlr, SPI_OPTYPE, ctlr->optype);
> -	ich_writel(ctlr, SPI_OPMENU_LOWER, ctlr->opmenu);
> -	ich_writel(ctlr, SPI_OPMENU_UPPER, ctlr->opmenu + sizeof(u32));
> +	ich_spi_config_opcode(bus);
>   
>   	return 0;
>   }
> 

Reviewed-by: Stefan Roese <sr@denx.de>

Thanks,
Stefan

[U-Boot] [PATCH 5/5] x86: fsp: Configure SPI opcode registers before SPI is locked down

[Stefan Roese]

On 16.08.2017 07:38, Bin Meng wrote:
> Some Intel FSP (like Braswell) does SPI lock-down during the call
> to fsp_notify(INIT_PHASE_BOOT). But before SPI lock-down is done,
> it's bootloader's responsibility to configure the SPI controller's
> opcode registers properly otherwise SPI controller driver doesn't
> know how to communicate with the SPI flash device.
> 
> This introduces a Kconfig option CONFIG_FSP_LOCKDOWN_SPI for such
> FSPs. When it is on, U-Boot will configure the SPI opcode registers
> before the lock-down.
> 
> Signed-off-by: Bin Meng <bmeng.cn@gmail.com>
> ---
> 
>   arch/x86/Kconfig              |  9 +++++++++
>   arch/x86/lib/fsp/fsp_common.c | 24 ++++++++++++++++++++++++
>   2 files changed, 33 insertions(+)
> 
> diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig
> index c26710b..5373082 100644
> --- a/arch/x86/Kconfig
> +++ b/arch/x86/Kconfig
> @@ -401,6 +401,15 @@ config FSP_BROKEN_HOB
>   	  do not overwrite the important boot service data which is used by
>   	  FSP, otherwise the subsequent call to fsp_notify() will fail.
>   
> +config FSP_LOCKDOWN_SPI
> +	bool
> +	depends on HAVE_FSP
> +	help
> +	  Some Intel FSP (like Braswell) does SPI lock-down during the call
> +	  to fsp_notify(INIT_PHASE_BOOT). This option should be turned on
> +	  for such FSP and U-Boot will configure the SPI opcode registers
> +	  before the lock-down.
> +
>   config ENABLE_MRC_CACHE
>   	bool "Enable MRC cache"
>   	depends on !EFI && !SYS_COREBOOT
> diff --git a/arch/x86/lib/fsp/fsp_common.c b/arch/x86/lib/fsp/fsp_common.c
> index 3397bb8..320d87d 100644
> --- a/arch/x86/lib/fsp/fsp_common.c
> +++ b/arch/x86/lib/fsp/fsp_common.c
> @@ -19,6 +19,8 @@
>   
>   DECLARE_GLOBAL_DATA_PTR;
>   
> +extern void ich_spi_config_opcode(struct udevice *dev);
> +
>   int checkcpu(void)
>   {
>   	return 0;
> @@ -49,6 +51,28 @@ void board_final_cleanup(void)
>   {
>   	u32 status;
>   
> +#ifdef CONFIG_FSP_LOCKDOWN_SPI
> +	struct udevice *dev;
> +
> +	/*
> +	 * Some Intel FSP (like Braswell) does SPI lock-down during the call
> +	 * to fsp_notify(INIT_PHASE_BOOT). But before SPI lock-down is done,
> +	 * it's bootloader's responsibility to configure the SPI controller's
> +	 * opcode registers properly otherwise SPI controller driver doesn't
> +	 * know how to communicate with the SPI flash device.
> +	 *
> +	 * Note we cannot do such configuration elsewhere (eg: during the SPI
> +	 * controller driver's probe() routine), becasue:

because

> +	 *
> +	 * 1). U-Boot SPI controller driver does not set the lock-down bit
> +	 * 2). Any SPI transfer will corrupt the contents of these registers
> +	 *
> +	 * Hence we have to do it right here before SPI lock-down bit is set.
> +	 */
> +	if (!uclass_first_device_err(UCLASS_SPI, &dev))
> +		ich_spi_config_opcode(dev);
> +#endif
> +
>   	/* call into FspNotify */
>   	debug("Calling into FSP (notify phase INIT_PHASE_BOOT): ");
>   	status = fsp_notify(NULL, INIT_PHASE_BOOT);
> 

Reviewed-by: Stefan Roese <sr@denx.de>

Thanks,
Stefan

[U-Boot] [PATCH 1/5] x86: ich-spi: Remove spi_write_protect_region()

[Bin Meng]

On Wed, Aug 16, 2017 at 2:18 PM, Stefan Roese <sr@denx.de> wrote:
> On 16.08.2017 07:38, Bin Meng wrote:
>>
>> This routine is not called anywhere.
>>
>> Signed-off-by: Bin Meng <bmeng.cn@gmail.com>
>> ---
>>
>>   drivers/spi/ich.c | 50
>> --------------------------------------------------
>>   1 file changed, 50 deletions(-)
>>
>> diff --git a/drivers/spi/ich.c b/drivers/spi/ich.c
>> index bf2e99b..46dd9a8 100644
>> --- a/drivers/spi/ich.c
>> +++ b/drivers/spi/ich.c
>> @@ -539,56 +539,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
>> - * don't actually take effect until the HSFS[FLOCKDN] bit is set, but
>> that's
>> - * done elsewhere.
>> - */
>> -int spi_write_protect_region(struct udevice *dev, uint32_t lower_limit,
>> -                            uint32_t length, int hint)
>> -{
>> -       struct udevice *bus = dev->parent;
>> -       struct ich_spi_priv *ctlr = dev_get_priv(bus);
>> -       uint32_t tmplong;
>> -       uint32_t upper_limit;
>> -
>> -       if (!ctlr->pr) {
>> -               printf("%s: operation not supported on this chipset\n",
>> -                      __func__);
>> -               return -ENOSYS;
>> -       }
>> -
>> -       if (length == 0 ||
>> -           lower_limit > (0xFFFFFFFFUL - length) + 1 ||
>> -           hint < 0 || hint > 4) {
>> -               printf("%s(0x%x, 0x%x, %d): invalid args\n", __func__,
>> -                      lower_limit, length, hint);
>> -               return -EPERM;
>> -       }
>> -
>> -       upper_limit = lower_limit + length - 1;
>> -
>> -       /*
>> -        * Determine bits to write, as follows:
>> -        *  31     Write-protection enable (includes erase operation)
>> -        *  30:29  reserved
>> -        *  28:16  Upper Limit (FLA address bits 24:12, with 11:0 ==
>> 0xfff)
>> -        *  15     Read-protection enable
>> -        *  14:13  reserved
>> -        *  12:0   Lower Limit (FLA address bits 24:12, with 11:0 ==
>> 0x000)
>> -        */
>> -       tmplong = 0x80000000 |
>> -               ((upper_limit & 0x01fff000) << 4) |
>> -               ((lower_limit & 0x01fff000) >> 12);
>> -
>> -       printf("%s: writing 0x%08x to %p\n", __func__, tmplong,
>> -              &ctlr->pr[hint]);
>> -       ctlr->pr[hint] = tmplong;
>> -
>> -       return 0;
>> -}
>> -
>>   static int ich_spi_probe(struct udevice *dev)
>>   {
>>         struct ich_spi_platdata *plat = dev_get_platdata(dev);
>>
>
> Reviewed-by: Stefan Roese <sr@denx.de>

applied to u-boot-x86, thanks!

[U-Boot] [PATCH 2/5] x86: ich-spi: Remove unnecessary assignment in ich_init_controller()

[Bin Meng]

On Wed, Aug 16, 2017 at 2:18 PM, Stefan Roese <sr@denx.de> wrote:
> On 16.08.2017 07:38, Bin Meng wrote:
>>
>> There is no need to do another assignment to ich7_spi.
>>
>> Signed-off-by: Bin Meng <bmeng.cn@gmail.com>
>> ---
>>
>>   drivers/spi/ich.c | 1 -
>>   1 file changed, 1 deletion(-)
>>
>> diff --git a/drivers/spi/ich.c b/drivers/spi/ich.c
>> index 46dd9a8..909eefc 100644
>> --- a/drivers/spi/ich.c
>> +++ b/drivers/spi/ich.c
>> @@ -126,7 +126,6 @@ static int ich_init_controller(struct udevice *dev,
>>         if (plat->ich_version == ICHV_7) {
>>                 struct ich7_spi_regs *ich7_spi = sbase;
>>   -             ich7_spi = (struct ich7_spi_regs *)sbase;
>>                 ctlr->ichspi_lock = readw(&ich7_spi->spis) & SPIS_LOCK;
>>                 ctlr->opmenu = offsetof(struct ich7_spi_regs, opmenu);
>>                 ctlr->menubytes = sizeof(ich7_spi->opmenu);
>>
>
> Reviewed-by: Stefan Roese <sr@denx.de>

applied to u-boot-x86, thanks!

[U-Boot] [PATCH 3/5] x86: ich-spi: Don't read cached lock status

[Bin Meng]

On Wed, Aug 16, 2017 at 2:19 PM, Stefan Roese <sr@denx.de> wrote:
> On 16.08.2017 07:38, Bin Meng wrote:
>>
>> At present the ICH SPI controller driver reads the controller lock
>> status from its register in the probe routine and saves the lock
>> status to a member of priv. Later the driver uses the cached status
>> from priv to judge whether the controller setting is locked and do
>> different setup.
>>
>> But such logic is only valid when there is only the SPI controller
>> driver that touches the SPI hardware. In fact the lock status change
>> can be trigged outside the driver, eg: during the fsp_notify() call
>> when Intel FSP is used.
>>
>> This changes the driver to read the lock status every time when an
>> SPI transfer is initiated instead of reading the cached one.
>>
>> Signed-off-by: Bin Meng <bmeng.cn@gmail.com>
>> ---
>>
>>   drivers/spi/ich.c | 29 +++++++++++++++++++++++------
>>   drivers/spi/ich.h |  2 --
>>   2 files changed, 23 insertions(+), 8 deletions(-)
>>
>> diff --git a/drivers/spi/ich.c b/drivers/spi/ich.c
>> index 909eefc..d4888f5 100644
>> --- a/drivers/spi/ich.c
>> +++ b/drivers/spi/ich.c
>> @@ -126,7 +126,6 @@ static int ich_init_controller(struct udevice *dev,
>>         if (plat->ich_version == ICHV_7) {
>>                 struct ich7_spi_regs *ich7_spi = sbase;
>>   -             ctlr->ichspi_lock = readw(&ich7_spi->spis) & SPIS_LOCK;
>>                 ctlr->opmenu = offsetof(struct ich7_spi_regs, opmenu);
>>                 ctlr->menubytes = sizeof(ich7_spi->opmenu);
>>                 ctlr->optype = offsetof(struct ich7_spi_regs, optype);
>> @@ -141,7 +140,6 @@ static int ich_init_controller(struct udevice *dev,
>>         } else if (plat->ich_version == ICHV_9) {
>>                 struct ich9_spi_regs *ich9_spi = sbase;
>>   -             ctlr->ichspi_lock = readw(&ich9_spi->hsfs) & HSFS_FLOCKDN;
>>                 ctlr->opmenu = offsetof(struct ich9_spi_regs, opmenu);
>>                 ctlr->menubytes = sizeof(ich9_spi->opmenu);
>>                 ctlr->optype = offsetof(struct ich9_spi_regs, optype);
>> @@ -186,6 +184,23 @@ static inline void spi_use_in(struct spi_trans
>> *trans, unsigned bytes)
>>         trans->bytesin -= bytes;
>>   }
>>   +static bool spi_lock_status(struct ich_spi_platdata *plat, void *sbase)
>> +{
>> +       int lock = 0;
>> +
>> +       if (plat->ich_version == ICHV_7) {
>> +               struct ich7_spi_regs *ich7_spi = sbase;
>> +
>> +               lock = readw(&ich7_spi->spis) & SPIS_LOCK;
>> +       } else if (plat->ich_version == ICHV_9) {
>> +               struct ich9_spi_regs *ich9_spi = sbase;
>> +
>> +               lock = readw(&ich9_spi->hsfs) & HSFS_FLOCKDN;
>> +       }
>> +
>> +       return lock != 0;
>> +}
>> +
>>   static void spi_setup_type(struct spi_trans *trans, int data_bytes)
>>   {
>>         trans->type = 0xFF;
>> @@ -219,14 +234,15 @@ static void spi_setup_type(struct spi_trans *trans,
>> int data_bytes)
>>         }
>>   }
>>   -static int spi_setup_opcode(struct ich_spi_priv *ctlr, struct spi_trans
>> *trans)
>> +static int spi_setup_opcode(struct ich_spi_priv *ctlr, struct spi_trans
>> *trans,
>> +                           bool lock)
>>   {
>>         uint16_t optypes;
>>         uint8_t opmenu[ctlr->menubytes];
>>         trans->opcode = trans->out[0];
>>         spi_use_out(trans, 1);
>> -       if (!ctlr->ichspi_lock) {
>> +       if (!lock) {
>>                 /* The lock is off, so just use index 0. */
>>                 ich_writeb(ctlr, trans->opcode, ctlr->opmenu);
>>                 optypes = ich_readw(ctlr, ctlr->optype);
>> @@ -336,6 +352,7 @@ static int ich_spi_xfer(struct udevice *dev, unsigned
>> int bitlen,
>>         struct spi_trans *trans = &ctlr->trans;
>>         unsigned type = flags & (SPI_XFER_BEGIN | SPI_XFER_END);
>>         int using_cmd = 0;
>> +       bool lock = spi_lock_status(plat, ctlr->base);
>>         int ret;
>>         /* We don't support writing partial bytes */
>> @@ -399,7 +416,7 @@ static int ich_spi_xfer(struct udevice *dev, unsigned
>> int bitlen,
>>                 ich_writeb(ctlr, SPIS_CDS | SPIS_FCERR, ctlr->status);
>>         spi_setup_type(trans, using_cmd ? bytes : 0);
>> -       opcode_index = spi_setup_opcode(ctlr, trans);
>> +       opcode_index = spi_setup_opcode(ctlr, trans, lock);
>>         if (opcode_index < 0)
>>                 return -EINVAL;
>>         with_address = spi_setup_offset(trans);
>> @@ -412,7 +429,7 @@ static int ich_spi_xfer(struct udevice *dev, unsigned
>> int bitlen,
>>                  * in order to prevent the Management Engine from
>>                  * issuing a transaction between WREN and DATA.
>>                  */
>> -               if (!ctlr->ichspi_lock)
>> +               if (!lock)
>>                         ich_writew(ctlr, trans->opcode, ctlr->preop);
>>                 return 0;
>>         }
>> diff --git a/drivers/spi/ich.h b/drivers/spi/ich.h
>> index dcb8a90..c867c57 100644
>> --- a/drivers/spi/ich.h
>> +++ b/drivers/spi/ich.h
>> @@ -177,8 +177,6 @@ struct ich_spi_platdata {
>>   };
>>     struct ich_spi_priv {
>> -       int ichspi_lock;
>> -       int locked;
>>         int opmenu;
>>         int menubytes;
>>         void *base;             /* Base of register set */
>>
>
> Reviewed-by: Stefan Roese <sr@denx.de>
>

applied to u-boot-x86, thanks!

[U-Boot] [PATCH 4/5] x86: ich-spi: Move opcode registers configuration to another routine

[Bin Meng]

On Wed, Aug 16, 2017 at 2:19 PM, Stefan Roese <sr@denx.de> wrote:
> On 16.08.2017 07:38, Bin Meng wrote:
>>
>> At present the ICH SPI opcode registers configuration is done in the
>> ich_spi_remove() routine, a little bit weird but that's how current.
>> Linux MTD driver works. This changes to move the opcode registers
>> configuration to a separate routine ich_spi_config_opcode() which
>> might be called by U-Boot itself as well.
>>
>> Signed-off-by: Bin Meng <bmeng.cn@gmail.com>
>> ---
>>
>>   drivers/spi/ich.c | 22 ++++++++++++++++------
>>   1 file changed, 16 insertions(+), 6 deletions(-)
>>
>> diff --git a/drivers/spi/ich.c b/drivers/spi/ich.c
>> index d4888f5..373bc26 100644
>> --- a/drivers/spi/ich.c
>> +++ b/drivers/spi/ich.c
>> @@ -338,6 +338,21 @@ static int ich_status_poll(struct ich_spi_priv *ctlr,
>> u16 bitmask,
>>         return -ETIMEDOUT;
>>   }
>>   +void ich_spi_config_opcode(struct udevice *dev)
>> +{
>> +       struct ich_spi_priv *ctlr = dev_get_priv(dev);
>> +
>> +       /*
>> +        * PREOP, OPTYPE, OPMENU1/OPMENU2 registers can be locked down
>> +        * to prevent accidental or intentional writes. Before they get
>> +        * locked down, these registers should be initialized properly.
>> +        */
>> +       ich_writew(ctlr, SPI_OPPREFIX, ctlr->preop);
>> +       ich_writew(ctlr, SPI_OPTYPE, ctlr->optype);
>> +       ich_writel(ctlr, SPI_OPMENU_LOWER, ctlr->opmenu);
>> +       ich_writel(ctlr, SPI_OPMENU_UPPER, ctlr->opmenu + sizeof(u32));
>> +}
>> +
>>   static int ich_spi_xfer(struct udevice *dev, unsigned int bitlen,
>>                         const void *dout, void *din, unsigned long flags)
>>   {
>> @@ -585,16 +600,11 @@ static int ich_spi_probe(struct udevice *dev)
>>     static int ich_spi_remove(struct udevice *bus)
>>   {
>> -       struct ich_spi_priv *ctlr = dev_get_priv(bus);
>> -
>>         /*
>>          * Configure SPI controller so that the Linux MTD driver can fully
>>          * access the SPI NOR chip
>>          */
>> -       ich_writew(ctlr, SPI_OPPREFIX, ctlr->preop);
>> -       ich_writew(ctlr, SPI_OPTYPE, ctlr->optype);
>> -       ich_writel(ctlr, SPI_OPMENU_LOWER, ctlr->opmenu);
>> -       ich_writel(ctlr, SPI_OPMENU_UPPER, ctlr->opmenu + sizeof(u32));
>> +       ich_spi_config_opcode(bus);
>>         return 0;
>>   }
>>
>
> Reviewed-by: Stefan Roese <sr@denx.de>

applied to u-boot-x86, thanks!

[U-Boot] [PATCH 5/5] x86: fsp: Configure SPI opcode registers before SPI is locked down

[Bin Meng]

On Wed, Aug 16, 2017 at 2:21 PM, Stefan Roese <sr@denx.de> wrote:
> On 16.08.2017 07:38, Bin Meng wrote:
>>
>> Some Intel FSP (like Braswell) does SPI lock-down during the call
>> to fsp_notify(INIT_PHASE_BOOT). But before SPI lock-down is done,
>> it's bootloader's responsibility to configure the SPI controller's
>> opcode registers properly otherwise SPI controller driver doesn't
>> know how to communicate with the SPI flash device.
>>
>> This introduces a Kconfig option CONFIG_FSP_LOCKDOWN_SPI for such
>> FSPs. When it is on, U-Boot will configure the SPI opcode registers
>> before the lock-down.
>>
>> Signed-off-by: Bin Meng <bmeng.cn@gmail.com>
>> ---
>>
>>   arch/x86/Kconfig              |  9 +++++++++
>>   arch/x86/lib/fsp/fsp_common.c | 24 ++++++++++++++++++++++++
>>   2 files changed, 33 insertions(+)
>>
>> diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig
>> index c26710b..5373082 100644
>> --- a/arch/x86/Kconfig
>> +++ b/arch/x86/Kconfig
>> @@ -401,6 +401,15 @@ config FSP_BROKEN_HOB
>>           do not overwrite the important boot service data which is used
>> by
>>           FSP, otherwise the subsequent call to fsp_notify() will fail.
>>   +config FSP_LOCKDOWN_SPI
>> +       bool
>> +       depends on HAVE_FSP
>> +       help
>> +         Some Intel FSP (like Braswell) does SPI lock-down during the
>> call
>> +         to fsp_notify(INIT_PHASE_BOOT). This option should be turned on
>> +         for such FSP and U-Boot will configure the SPI opcode registers
>> +         before the lock-down.
>> +
>>   config ENABLE_MRC_CACHE
>>         bool "Enable MRC cache"
>>         depends on !EFI && !SYS_COREBOOT
>> diff --git a/arch/x86/lib/fsp/fsp_common.c b/arch/x86/lib/fsp/fsp_common.c
>> index 3397bb8..320d87d 100644
>> --- a/arch/x86/lib/fsp/fsp_common.c
>> +++ b/arch/x86/lib/fsp/fsp_common.c
>> @@ -19,6 +19,8 @@
>>     DECLARE_GLOBAL_DATA_PTR;
>>   +extern void ich_spi_config_opcode(struct udevice *dev);
>> +
>>   int checkcpu(void)
>>   {
>>         return 0;
>> @@ -49,6 +51,28 @@ void board_final_cleanup(void)
>>   {
>>         u32 status;
>>   +#ifdef CONFIG_FSP_LOCKDOWN_SPI
>> +       struct udevice *dev;
>> +
>> +       /*
>> +        * Some Intel FSP (like Braswell) does SPI lock-down during the
>> call
>> +        * to fsp_notify(INIT_PHASE_BOOT). But before SPI lock-down is
>> done,
>> +        * it's bootloader's responsibility to configure the SPI
>> controller's
>> +        * opcode registers properly otherwise SPI controller driver
>> doesn't
>> +        * know how to communicate with the SPI flash device.
>> +        *
>> +        * Note we cannot do such configuration elsewhere (eg: during the
>> SPI
>> +        * controller driver's probe() routine), becasue:
>
>
> because

Fixed this, and

>
>> +        *
>> +        * 1). U-Boot SPI controller driver does not set the lock-down bit
>> +        * 2). Any SPI transfer will corrupt the contents of these
>> registers
>> +        *
>> +        * Hence we have to do it right here before SPI lock-down bit is
>> set.
>> +        */
>> +       if (!uclass_first_device_err(UCLASS_SPI, &dev))
>> +               ich_spi_config_opcode(dev);
>> +#endif
>> +
>>         /* call into FspNotify */
>>         debug("Calling into FSP (notify phase INIT_PHASE_BOOT): ");
>>         status = fsp_notify(NULL, INIT_PHASE_BOOT);
>>
>
> Reviewed-by: Stefan Roese <sr@denx.de>

applied to u-boot-x86, thanks!

[U-Boot] [PATCH 5/5] x86: fsp: Configure SPI opcode registers before SPI is locked down

[Simon Glass]

Hi Bin,

On 15 August 2017 at 23:38, Bin Meng <bmeng.cn@gmail.com> wrote:
> Some Intel FSP (like Braswell) does SPI lock-down during the call
> to fsp_notify(INIT_PHASE_BOOT). But before SPI lock-down is done,
> it's bootloader's responsibility to configure the SPI controller's
> opcode registers properly otherwise SPI controller driver doesn't
> know how to communicate with the SPI flash device.
>
> This introduces a Kconfig option CONFIG_FSP_LOCKDOWN_SPI for such
> FSPs. When it is on, U-Boot will configure the SPI opcode registers
> before the lock-down.
>
> Signed-off-by: Bin Meng <bmeng.cn@gmail.com>
> ---
>
>  arch/x86/Kconfig              |  9 +++++++++
>  arch/x86/lib/fsp/fsp_common.c | 24 ++++++++++++++++++++++++
>  2 files changed, 33 insertions(+)
>
> diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig
> index c26710b..5373082 100644
> --- a/arch/x86/Kconfig
> +++ b/arch/x86/Kconfig
> @@ -401,6 +401,15 @@ config FSP_BROKEN_HOB
>           do not overwrite the important boot service data which is used by
>           FSP, otherwise the subsequent call to fsp_notify() will fail.
>
> +config FSP_LOCKDOWN_SPI
> +       bool
> +       depends on HAVE_FSP
> +       help
> +         Some Intel FSP (like Braswell) does SPI lock-down during the call
> +         to fsp_notify(INIT_PHASE_BOOT). This option should be turned on
> +         for such FSP and U-Boot will configure the SPI opcode registers
> +         before the lock-down.
> +
>  config ENABLE_MRC_CACHE
>         bool "Enable MRC cache"
>         depends on !EFI && !SYS_COREBOOT
> diff --git a/arch/x86/lib/fsp/fsp_common.c b/arch/x86/lib/fsp/fsp_common.c
> index 3397bb8..320d87d 100644
> --- a/arch/x86/lib/fsp/fsp_common.c
> +++ b/arch/x86/lib/fsp/fsp_common.c
> @@ -19,6 +19,8 @@
>
>  DECLARE_GLOBAL_DATA_PTR;
>
> +extern void ich_spi_config_opcode(struct udevice *dev);
> +
>  int checkcpu(void)
>  {
>         return 0;
> @@ -49,6 +51,28 @@ void board_final_cleanup(void)
>  {
>         u32 status;
>
> +#ifdef CONFIG_FSP_LOCKDOWN_SPI
> +       struct udevice *dev;
> +
> +       /*
> +        * Some Intel FSP (like Braswell) does SPI lock-down during the call
> +        * to fsp_notify(INIT_PHASE_BOOT). But before SPI lock-down is done,
> +        * it's bootloader's responsibility to configure the SPI controller's
> +        * opcode registers properly otherwise SPI controller driver doesn't
> +        * know how to communicate with the SPI flash device.
> +        *
> +        * Note we cannot do such configuration elsewhere (eg: during the SPI
> +        * controller driver's probe() routine), becasue:
> +        *
> +        * 1). U-Boot SPI controller driver does not set the lock-down bit
> +        * 2). Any SPI transfer will corrupt the contents of these registers
> +        *
> +        * Hence we have to do it right here before SPI lock-down bit is set.
> +        */
> +       if (!uclass_first_device_err(UCLASS_SPI, &dev))
> +               ich_spi_config_opcode(dev);

I wonder if we could do this by using an operation instead of directly
calling a function in the driver?

> +#endif
> +
>         /* call into FspNotify */
>         debug("Calling into FSP (notify phase INIT_PHASE_BOOT): ");
>         status = fsp_notify(NULL, INIT_PHASE_BOOT);
> --
> 2.9.2
>

Regards,
Simon

[U-Boot] [PATCH 5/5] x86: fsp: Configure SPI opcode registers before SPI is locked down

[Bin Meng]

Hi Simon,

On Sat, Aug 26, 2017 at 9:39 PM, Simon Glass <sjg@chromium.org> wrote:
> Hi Bin,
>
> On 15 August 2017 at 23:38, Bin Meng <bmeng.cn@gmail.com> wrote:
>> Some Intel FSP (like Braswell) does SPI lock-down during the call
>> to fsp_notify(INIT_PHASE_BOOT). But before SPI lock-down is done,
>> it's bootloader's responsibility to configure the SPI controller's
>> opcode registers properly otherwise SPI controller driver doesn't
>> know how to communicate with the SPI flash device.
>>
>> This introduces a Kconfig option CONFIG_FSP_LOCKDOWN_SPI for such
>> FSPs. When it is on, U-Boot will configure the SPI opcode registers
>> before the lock-down.
>>
>> Signed-off-by: Bin Meng <bmeng.cn@gmail.com>
>> ---
>>
>>  arch/x86/Kconfig              |  9 +++++++++
>>  arch/x86/lib/fsp/fsp_common.c | 24 ++++++++++++++++++++++++
>>  2 files changed, 33 insertions(+)
>>
>> diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig
>> index c26710b..5373082 100644
>> --- a/arch/x86/Kconfig
>> +++ b/arch/x86/Kconfig
>> @@ -401,6 +401,15 @@ config FSP_BROKEN_HOB
>>           do not overwrite the important boot service data which is used by
>>           FSP, otherwise the subsequent call to fsp_notify() will fail.
>>
>> +config FSP_LOCKDOWN_SPI
>> +       bool
>> +       depends on HAVE_FSP
>> +       help
>> +         Some Intel FSP (like Braswell) does SPI lock-down during the call
>> +         to fsp_notify(INIT_PHASE_BOOT). This option should be turned on
>> +         for such FSP and U-Boot will configure the SPI opcode registers
>> +         before the lock-down.
>> +
>>  config ENABLE_MRC_CACHE
>>         bool "Enable MRC cache"
>>         depends on !EFI && !SYS_COREBOOT
>> diff --git a/arch/x86/lib/fsp/fsp_common.c b/arch/x86/lib/fsp/fsp_common.c
>> index 3397bb8..320d87d 100644
>> --- a/arch/x86/lib/fsp/fsp_common.c
>> +++ b/arch/x86/lib/fsp/fsp_common.c
>> @@ -19,6 +19,8 @@
>>
>>  DECLARE_GLOBAL_DATA_PTR;
>>
>> +extern void ich_spi_config_opcode(struct udevice *dev);
>> +
>>  int checkcpu(void)
>>  {
>>         return 0;
>> @@ -49,6 +51,28 @@ void board_final_cleanup(void)
>>  {
>>         u32 status;
>>
>> +#ifdef CONFIG_FSP_LOCKDOWN_SPI
>> +       struct udevice *dev;
>> +
>> +       /*
>> +        * Some Intel FSP (like Braswell) does SPI lock-down during the call
>> +        * to fsp_notify(INIT_PHASE_BOOT). But before SPI lock-down is done,
>> +        * it's bootloader's responsibility to configure the SPI controller's
>> +        * opcode registers properly otherwise SPI controller driver doesn't
>> +        * know how to communicate with the SPI flash device.
>> +        *
>> +        * Note we cannot do such configuration elsewhere (eg: during the SPI
>> +        * controller driver's probe() routine), becasue:
>> +        *
>> +        * 1). U-Boot SPI controller driver does not set the lock-down bit
>> +        * 2). Any SPI transfer will corrupt the contents of these registers
>> +        *
>> +        * Hence we have to do it right here before SPI lock-down bit is set.
>> +        */
>> +       if (!uclass_first_device_err(UCLASS_SPI, &dev))
>> +               ich_spi_config_opcode(dev);
>
> I wonder if we could do this by using an operation instead of directly
> calling a function in the driver?
>

Do you mean adding one operation to dm_spi_ops?

>> +#endif
>> +
>>         /* call into FspNotify */
>>         debug("Calling into FSP (notify phase INIT_PHASE_BOOT): ");
>>         status = fsp_notify(NULL, INIT_PHASE_BOOT);
>> --

Regards,
Bin

[U-Boot] [PATCH 5/5] x86: fsp: Configure SPI opcode registers before SPI is locked down

[Simon Glass]

Hi Bin,

On 26 August 2017 at 07:58, Bin Meng <bmeng.cn@gmail.com> wrote:
> Hi Simon,
>
> On Sat, Aug 26, 2017 at 9:39 PM, Simon Glass <sjg@chromium.org> wrote:
>> Hi Bin,
>>
>> On 15 August 2017 at 23:38, Bin Meng <bmeng.cn@gmail.com> wrote:
>>> Some Intel FSP (like Braswell) does SPI lock-down during the call
>>> to fsp_notify(INIT_PHASE_BOOT). But before SPI lock-down is done,
>>> it's bootloader's responsibility to configure the SPI controller's
>>> opcode registers properly otherwise SPI controller driver doesn't
>>> know how to communicate with the SPI flash device.
>>>
>>> This introduces a Kconfig option CONFIG_FSP_LOCKDOWN_SPI for such
>>> FSPs. When it is on, U-Boot will configure the SPI opcode registers
>>> before the lock-down.
>>>
>>> Signed-off-by: Bin Meng <bmeng.cn@gmail.com>
>>> ---
>>>
>>>  arch/x86/Kconfig              |  9 +++++++++
>>>  arch/x86/lib/fsp/fsp_common.c | 24 ++++++++++++++++++++++++
>>>  2 files changed, 33 insertions(+)
>>>
>>> diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig
>>> index c26710b..5373082 100644
>>> --- a/arch/x86/Kconfig
>>> +++ b/arch/x86/Kconfig
>>> @@ -401,6 +401,15 @@ config FSP_BROKEN_HOB
>>>           do not overwrite the important boot service data which is used by
>>>           FSP, otherwise the subsequent call to fsp_notify() will fail.
>>>
>>> +config FSP_LOCKDOWN_SPI
>>> +       bool
>>> +       depends on HAVE_FSP
>>> +       help
>>> +         Some Intel FSP (like Braswell) does SPI lock-down during the call
>>> +         to fsp_notify(INIT_PHASE_BOOT). This option should be turned on
>>> +         for such FSP and U-Boot will configure the SPI opcode registers
>>> +         before the lock-down.
>>> +
>>>  config ENABLE_MRC_CACHE
>>>         bool "Enable MRC cache"
>>>         depends on !EFI && !SYS_COREBOOT
>>> diff --git a/arch/x86/lib/fsp/fsp_common.c b/arch/x86/lib/fsp/fsp_common.c
>>> index 3397bb8..320d87d 100644
>>> --- a/arch/x86/lib/fsp/fsp_common.c
>>> +++ b/arch/x86/lib/fsp/fsp_common.c
>>> @@ -19,6 +19,8 @@
>>>
>>>  DECLARE_GLOBAL_DATA_PTR;
>>>
>>> +extern void ich_spi_config_opcode(struct udevice *dev);
>>> +
>>>  int checkcpu(void)
>>>  {
>>>         return 0;
>>> @@ -49,6 +51,28 @@ void board_final_cleanup(void)
>>>  {
>>>         u32 status;
>>>
>>> +#ifdef CONFIG_FSP_LOCKDOWN_SPI
>>> +       struct udevice *dev;
>>> +
>>> +       /*
>>> +        * Some Intel FSP (like Braswell) does SPI lock-down during the call
>>> +        * to fsp_notify(INIT_PHASE_BOOT). But before SPI lock-down is done,
>>> +        * it's bootloader's responsibility to configure the SPI controller's
>>> +        * opcode registers properly otherwise SPI controller driver doesn't
>>> +        * know how to communicate with the SPI flash device.
>>> +        *
>>> +        * Note we cannot do such configuration elsewhere (eg: during the SPI
>>> +        * controller driver's probe() routine), becasue:
>>> +        *
>>> +        * 1). U-Boot SPI controller driver does not set the lock-down bit
>>> +        * 2). Any SPI transfer will corrupt the contents of these registers
>>> +        *
>>> +        * Hence we have to do it right here before SPI lock-down bit is set.
>>> +        */
>>> +       if (!uclass_first_device_err(UCLASS_SPI, &dev))
>>> +               ich_spi_config_opcode(dev);
>>
>> I wonder if we could do this by using an operation instead of directly
>> calling a function in the driver?
>>
>
> Do you mean adding one operation to dm_spi_ops?

Yes I think that would be better.

>
>>> +#endif
>>> +
>>>         /* call into FspNotify */
>>>         debug("Calling into FSP (notify phase INIT_PHASE_BOOT): ");
>>>         status = fsp_notify(NULL, INIT_PHASE_BOOT);
>>> --
>
> Regards,
> Bin

Regards,
Simon

[U-Boot] [PATCH 5/5] x86: fsp: Configure SPI opcode registers before SPI is locked down

[Bin Meng]

Hi Simon,

On Sun, Aug 27, 2017 at 6:40 AM, Simon Glass <sjg@chromium.org> wrote:
> Hi Bin,
>
> On 26 August 2017 at 07:58, Bin Meng <bmeng.cn@gmail.com> wrote:
>> Hi Simon,
>>
>> On Sat, Aug 26, 2017 at 9:39 PM, Simon Glass <sjg@chromium.org> wrote:
>>> Hi Bin,
>>>
>>> On 15 August 2017 at 23:38, Bin Meng <bmeng.cn@gmail.com> wrote:
>>>> Some Intel FSP (like Braswell) does SPI lock-down during the call
>>>> to fsp_notify(INIT_PHASE_BOOT). But before SPI lock-down is done,
>>>> it's bootloader's responsibility to configure the SPI controller's
>>>> opcode registers properly otherwise SPI controller driver doesn't
>>>> know how to communicate with the SPI flash device.
>>>>
>>>> This introduces a Kconfig option CONFIG_FSP_LOCKDOWN_SPI for such
>>>> FSPs. When it is on, U-Boot will configure the SPI opcode registers
>>>> before the lock-down.
>>>>
>>>> Signed-off-by: Bin Meng <bmeng.cn@gmail.com>
>>>> ---
>>>>
>>>>  arch/x86/Kconfig              |  9 +++++++++
>>>>  arch/x86/lib/fsp/fsp_common.c | 24 ++++++++++++++++++++++++
>>>>  2 files changed, 33 insertions(+)
>>>>
>>>> diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig
>>>> index c26710b..5373082 100644
>>>> --- a/arch/x86/Kconfig
>>>> +++ b/arch/x86/Kconfig
>>>> @@ -401,6 +401,15 @@ config FSP_BROKEN_HOB
>>>>           do not overwrite the important boot service data which is used by
>>>>           FSP, otherwise the subsequent call to fsp_notify() will fail.
>>>>
>>>> +config FSP_LOCKDOWN_SPI
>>>> +       bool
>>>> +       depends on HAVE_FSP
>>>> +       help
>>>> +         Some Intel FSP (like Braswell) does SPI lock-down during the call
>>>> +         to fsp_notify(INIT_PHASE_BOOT). This option should be turned on
>>>> +         for such FSP and U-Boot will configure the SPI opcode registers
>>>> +         before the lock-down.
>>>> +
>>>>  config ENABLE_MRC_CACHE
>>>>         bool "Enable MRC cache"
>>>>         depends on !EFI && !SYS_COREBOOT
>>>> diff --git a/arch/x86/lib/fsp/fsp_common.c b/arch/x86/lib/fsp/fsp_common.c
>>>> index 3397bb8..320d87d 100644
>>>> --- a/arch/x86/lib/fsp/fsp_common.c
>>>> +++ b/arch/x86/lib/fsp/fsp_common.c
>>>> @@ -19,6 +19,8 @@
>>>>
>>>>  DECLARE_GLOBAL_DATA_PTR;
>>>>
>>>> +extern void ich_spi_config_opcode(struct udevice *dev);
>>>> +
>>>>  int checkcpu(void)
>>>>  {
>>>>         return 0;
>>>> @@ -49,6 +51,28 @@ void board_final_cleanup(void)
>>>>  {
>>>>         u32 status;
>>>>
>>>> +#ifdef CONFIG_FSP_LOCKDOWN_SPI
>>>> +       struct udevice *dev;
>>>> +
>>>> +       /*
>>>> +        * Some Intel FSP (like Braswell) does SPI lock-down during the call
>>>> +        * to fsp_notify(INIT_PHASE_BOOT). But before SPI lock-down is done,
>>>> +        * it's bootloader's responsibility to configure the SPI controller's
>>>> +        * opcode registers properly otherwise SPI controller driver doesn't
>>>> +        * know how to communicate with the SPI flash device.
>>>> +        *
>>>> +        * Note we cannot do such configuration elsewhere (eg: during the SPI
>>>> +        * controller driver's probe() routine), becasue:
>>>> +        *
>>>> +        * 1). U-Boot SPI controller driver does not set the lock-down bit
>>>> +        * 2). Any SPI transfer will corrupt the contents of these registers
>>>> +        *
>>>> +        * Hence we have to do it right here before SPI lock-down bit is set.
>>>> +        */
>>>> +       if (!uclass_first_device_err(UCLASS_SPI, &dev))
>>>> +               ich_spi_config_opcode(dev);
>>>
>>> I wonder if we could do this by using an operation instead of directly
>>> calling a function in the driver?
>>>
>>
>> Do you mean adding one operation to dm_spi_ops?
>
> Yes I think that would be better.
>

Since this is x86-specific, I would hesitate to add one.

Regards,
Bin

[U-Boot] [PATCH 5/5] x86: fsp: Configure SPI opcode registers before SPI is locked down

[Simon Glass]

Hi Bin,

On 26 August 2017 at 18:12, Bin Meng <bmeng.cn@gmail.com> wrote:
> Hi Simon,
>
> On Sun, Aug 27, 2017 at 6:40 AM, Simon Glass <sjg@chromium.org> wrote:
>> Hi Bin,
>>
>> On 26 August 2017 at 07:58, Bin Meng <bmeng.cn@gmail.com> wrote:
>>> Hi Simon,
>>>
>>> On Sat, Aug 26, 2017 at 9:39 PM, Simon Glass <sjg@chromium.org> wrote:
>>>> Hi Bin,
>>>>
>>>> On 15 August 2017 at 23:38, Bin Meng <bmeng.cn@gmail.com> wrote:
>>>>> Some Intel FSP (like Braswell) does SPI lock-down during the call
>>>>> to fsp_notify(INIT_PHASE_BOOT). But before SPI lock-down is done,
>>>>> it's bootloader's responsibility to configure the SPI controller's
>>>>> opcode registers properly otherwise SPI controller driver doesn't
>>>>> know how to communicate with the SPI flash device.
>>>>>
>>>>> This introduces a Kconfig option CONFIG_FSP_LOCKDOWN_SPI for such
>>>>> FSPs. When it is on, U-Boot will configure the SPI opcode registers
>>>>> before the lock-down.
>>>>>
>>>>> Signed-off-by: Bin Meng <bmeng.cn@gmail.com>
>>>>> ---
>>>>>
>>>>>  arch/x86/Kconfig              |  9 +++++++++
>>>>>  arch/x86/lib/fsp/fsp_common.c | 24 ++++++++++++++++++++++++
>>>>>  2 files changed, 33 insertions(+)
>>>>>
>>>>> diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig
>>>>> index c26710b..5373082 100644
>>>>> --- a/arch/x86/Kconfig
>>>>> +++ b/arch/x86/Kconfig
>>>>> @@ -401,6 +401,15 @@ config FSP_BROKEN_HOB
>>>>>           do not overwrite the important boot service data which is used by
>>>>>           FSP, otherwise the subsequent call to fsp_notify() will fail.
>>>>>
>>>>> +config FSP_LOCKDOWN_SPI
>>>>> +       bool
>>>>> +       depends on HAVE_FSP
>>>>> +       help
>>>>> +         Some Intel FSP (like Braswell) does SPI lock-down during the call
>>>>> +         to fsp_notify(INIT_PHASE_BOOT). This option should be turned on
>>>>> +         for such FSP and U-Boot will configure the SPI opcode registers
>>>>> +         before the lock-down.
>>>>> +
>>>>>  config ENABLE_MRC_CACHE
>>>>>         bool "Enable MRC cache"
>>>>>         depends on !EFI && !SYS_COREBOOT
>>>>> diff --git a/arch/x86/lib/fsp/fsp_common.c b/arch/x86/lib/fsp/fsp_common.c
>>>>> index 3397bb8..320d87d 100644
>>>>> --- a/arch/x86/lib/fsp/fsp_common.c
>>>>> +++ b/arch/x86/lib/fsp/fsp_common.c
>>>>> @@ -19,6 +19,8 @@
>>>>>
>>>>>  DECLARE_GLOBAL_DATA_PTR;
>>>>>
>>>>> +extern void ich_spi_config_opcode(struct udevice *dev);
>>>>> +
>>>>>  int checkcpu(void)
>>>>>  {
>>>>>         return 0;
>>>>> @@ -49,6 +51,28 @@ void board_final_cleanup(void)
>>>>>  {
>>>>>         u32 status;
>>>>>
>>>>> +#ifdef CONFIG_FSP_LOCKDOWN_SPI
>>>>> +       struct udevice *dev;
>>>>> +
>>>>> +       /*
>>>>> +        * Some Intel FSP (like Braswell) does SPI lock-down during the call
>>>>> +        * to fsp_notify(INIT_PHASE_BOOT). But before SPI lock-down is done,
>>>>> +        * it's bootloader's responsibility to configure the SPI controller's
>>>>> +        * opcode registers properly otherwise SPI controller driver doesn't
>>>>> +        * know how to communicate with the SPI flash device.
>>>>> +        *
>>>>> +        * Note we cannot do such configuration elsewhere (eg: during the SPI
>>>>> +        * controller driver's probe() routine), becasue:
>>>>> +        *
>>>>> +        * 1). U-Boot SPI controller driver does not set the lock-down bit
>>>>> +        * 2). Any SPI transfer will corrupt the contents of these registers
>>>>> +        *
>>>>> +        * Hence we have to do it right here before SPI lock-down bit is set.
>>>>> +        */
>>>>> +       if (!uclass_first_device_err(UCLASS_SPI, &dev))
>>>>> +               ich_spi_config_opcode(dev);
>>>>
>>>> I wonder if we could do this by using an operation instead of directly
>>>> calling a function in the driver?
>>>>
>>>
>>> Do you mean adding one operation to dm_spi_ops?
>>
>> Yes I think that would be better.
>>
>
> Since this is x86-specific, I would hesitate to add one.
>

Yes I understand that. Still I don't think we should call directly
into drivers. What do you suggest?

- add some sort of DM event system to which drivers can attach for notifications
- add an ioctl() method to the SPI uclass where we can put random
calls like this
- set up a new MISC device as a child of SPI which includes an ioctl
for this operation
- something else?

Regards,
Simon

[U-Boot] [PATCH 5/5] x86: fsp: Configure SPI opcode registers before SPI is locked down

[Bin Meng]

Hi Simon,

On Wed, Sep 6, 2017 at 9:39 AM, Simon Glass <sjg@chromium.org> wrote:
> Hi Bin,
>
> On 26 August 2017 at 18:12, Bin Meng <bmeng.cn@gmail.com> wrote:
>> Hi Simon,
>>
>> On Sun, Aug 27, 2017 at 6:40 AM, Simon Glass <sjg@chromium.org> wrote:
>>> Hi Bin,
>>>
>>> On 26 August 2017 at 07:58, Bin Meng <bmeng.cn@gmail.com> wrote:
>>>> Hi Simon,
>>>>
>>>> On Sat, Aug 26, 2017 at 9:39 PM, Simon Glass <sjg@chromium.org> wrote:
>>>>> Hi Bin,
>>>>>
>>>>> On 15 August 2017 at 23:38, Bin Meng <bmeng.cn@gmail.com> wrote:
>>>>>> Some Intel FSP (like Braswell) does SPI lock-down during the call
>>>>>> to fsp_notify(INIT_PHASE_BOOT). But before SPI lock-down is done,
>>>>>> it's bootloader's responsibility to configure the SPI controller's
>>>>>> opcode registers properly otherwise SPI controller driver doesn't
>>>>>> know how to communicate with the SPI flash device.
>>>>>>
>>>>>> This introduces a Kconfig option CONFIG_FSP_LOCKDOWN_SPI for such
>>>>>> FSPs. When it is on, U-Boot will configure the SPI opcode registers
>>>>>> before the lock-down.
>>>>>>
>>>>>> Signed-off-by: Bin Meng <bmeng.cn@gmail.com>
>>>>>> ---
>>>>>>
>>>>>>  arch/x86/Kconfig              |  9 +++++++++
>>>>>>  arch/x86/lib/fsp/fsp_common.c | 24 ++++++++++++++++++++++++
>>>>>>  2 files changed, 33 insertions(+)
>>>>>>
>>>>>> diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig
>>>>>> index c26710b..5373082 100644
>>>>>> --- a/arch/x86/Kconfig
>>>>>> +++ b/arch/x86/Kconfig
>>>>>> @@ -401,6 +401,15 @@ config FSP_BROKEN_HOB
>>>>>>           do not overwrite the important boot service data which is used by
>>>>>>           FSP, otherwise the subsequent call to fsp_notify() will fail.
>>>>>>
>>>>>> +config FSP_LOCKDOWN_SPI
>>>>>> +       bool
>>>>>> +       depends on HAVE_FSP
>>>>>> +       help
>>>>>> +         Some Intel FSP (like Braswell) does SPI lock-down during the call
>>>>>> +         to fsp_notify(INIT_PHASE_BOOT). This option should be turned on
>>>>>> +         for such FSP and U-Boot will configure the SPI opcode registers
>>>>>> +         before the lock-down.
>>>>>> +
>>>>>>  config ENABLE_MRC_CACHE
>>>>>>         bool "Enable MRC cache"
>>>>>>         depends on !EFI && !SYS_COREBOOT
>>>>>> diff --git a/arch/x86/lib/fsp/fsp_common.c b/arch/x86/lib/fsp/fsp_common.c
>>>>>> index 3397bb8..320d87d 100644
>>>>>> --- a/arch/x86/lib/fsp/fsp_common.c
>>>>>> +++ b/arch/x86/lib/fsp/fsp_common.c
>>>>>> @@ -19,6 +19,8 @@
>>>>>>
>>>>>>  DECLARE_GLOBAL_DATA_PTR;
>>>>>>
>>>>>> +extern void ich_spi_config_opcode(struct udevice *dev);
>>>>>> +
>>>>>>  int checkcpu(void)
>>>>>>  {
>>>>>>         return 0;
>>>>>> @@ -49,6 +51,28 @@ void board_final_cleanup(void)
>>>>>>  {
>>>>>>         u32 status;
>>>>>>
>>>>>> +#ifdef CONFIG_FSP_LOCKDOWN_SPI
>>>>>> +       struct udevice *dev;
>>>>>> +
>>>>>> +       /*
>>>>>> +        * Some Intel FSP (like Braswell) does SPI lock-down during the call
>>>>>> +        * to fsp_notify(INIT_PHASE_BOOT). But before SPI lock-down is done,
>>>>>> +        * it's bootloader's responsibility to configure the SPI controller's
>>>>>> +        * opcode registers properly otherwise SPI controller driver doesn't
>>>>>> +        * know how to communicate with the SPI flash device.
>>>>>> +        *
>>>>>> +        * Note we cannot do such configuration elsewhere (eg: during the SPI
>>>>>> +        * controller driver's probe() routine), becasue:
>>>>>> +        *
>>>>>> +        * 1). U-Boot SPI controller driver does not set the lock-down bit
>>>>>> +        * 2). Any SPI transfer will corrupt the contents of these registers
>>>>>> +        *
>>>>>> +        * Hence we have to do it right here before SPI lock-down bit is set.
>>>>>> +        */
>>>>>> +       if (!uclass_first_device_err(UCLASS_SPI, &dev))
>>>>>> +               ich_spi_config_opcode(dev);
>>>>>
>>>>> I wonder if we could do this by using an operation instead of directly
>>>>> calling a function in the driver?
>>>>>
>>>>
>>>> Do you mean adding one operation to dm_spi_ops?
>>>
>>> Yes I think that would be better.
>>>
>>
>> Since this is x86-specific, I would hesitate to add one.
>>
>
> Yes I understand that. Still I don't think we should call directly
> into drivers. What do you suggest?
>
> - add some sort of DM event system to which drivers can attach for notifications
> - add an ioctl() method to the SPI uclass where we can put random
> calls like this
> - set up a new MISC device as a child of SPI which includes an ioctl
> for this operation
> - something else?
>

These are maybe too complicated to solve this little specific issue.
I've thought about this, and looks we can add a simple DTS property
"intel,spi-lock-down" and let the driver call this function.

Regards,
Bin

[U-Boot] [PATCH 5/5] x86: fsp: Configure SPI opcode registers before SPI is locked down

[Simon Glass]

Hi Bin,

On 12 September 2017 at 07:53, Bin Meng <bmeng.cn@gmail.com> wrote:
> Hi Simon,
>
> On Wed, Sep 6, 2017 at 9:39 AM, Simon Glass <sjg@chromium.org> wrote:
>> Hi Bin,
>>
>> On 26 August 2017 at 18:12, Bin Meng <bmeng.cn@gmail.com> wrote:
>>> Hi Simon,
>>>
>>> On Sun, Aug 27, 2017 at 6:40 AM, Simon Glass <sjg@chromium.org> wrote:
>>>> Hi Bin,
>>>>
>>>> On 26 August 2017 at 07:58, Bin Meng <bmeng.cn@gmail.com> wrote:
>>>>> Hi Simon,
>>>>>
>>>>> On Sat, Aug 26, 2017 at 9:39 PM, Simon Glass <sjg@chromium.org> wrote:
>>>>>> Hi Bin,
>>>>>>
>>>>>> On 15 August 2017 at 23:38, Bin Meng <bmeng.cn@gmail.com> wrote:
>>>>>>> Some Intel FSP (like Braswell) does SPI lock-down during the call
>>>>>>> to fsp_notify(INIT_PHASE_BOOT). But before SPI lock-down is done,
>>>>>>> it's bootloader's responsibility to configure the SPI controller's
>>>>>>> opcode registers properly otherwise SPI controller driver doesn't
>>>>>>> know how to communicate with the SPI flash device.
>>>>>>>
>>>>>>> This introduces a Kconfig option CONFIG_FSP_LOCKDOWN_SPI for such
>>>>>>> FSPs. When it is on, U-Boot will configure the SPI opcode registers
>>>>>>> before the lock-down.
>>>>>>>
>>>>>>> Signed-off-by: Bin Meng <bmeng.cn@gmail.com>
>>>>>>> ---
>>>>>>>
>>>>>>>  arch/x86/Kconfig              |  9 +++++++++
>>>>>>>  arch/x86/lib/fsp/fsp_common.c | 24 ++++++++++++++++++++++++
>>>>>>>  2 files changed, 33 insertions(+)
>>>>>>>
>>>>>>> diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig
>>>>>>> index c26710b..5373082 100644
>>>>>>> --- a/arch/x86/Kconfig
>>>>>>> +++ b/arch/x86/Kconfig
>>>>>>> @@ -401,6 +401,15 @@ config FSP_BROKEN_HOB
>>>>>>>           do not overwrite the important boot service data which is used by
>>>>>>>           FSP, otherwise the subsequent call to fsp_notify() will fail.
>>>>>>>
>>>>>>> +config FSP_LOCKDOWN_SPI
>>>>>>> +       bool
>>>>>>> +       depends on HAVE_FSP
>>>>>>> +       help
>>>>>>> +         Some Intel FSP (like Braswell) does SPI lock-down during the call
>>>>>>> +         to fsp_notify(INIT_PHASE_BOOT). This option should be turned on
>>>>>>> +         for such FSP and U-Boot will configure the SPI opcode registers
>>>>>>> +         before the lock-down.
>>>>>>> +
>>>>>>>  config ENABLE_MRC_CACHE
>>>>>>>         bool "Enable MRC cache"
>>>>>>>         depends on !EFI && !SYS_COREBOOT
>>>>>>> diff --git a/arch/x86/lib/fsp/fsp_common.c b/arch/x86/lib/fsp/fsp_common.c
>>>>>>> index 3397bb8..320d87d 100644
>>>>>>> --- a/arch/x86/lib/fsp/fsp_common.c
>>>>>>> +++ b/arch/x86/lib/fsp/fsp_common.c
>>>>>>> @@ -19,6 +19,8 @@
>>>>>>>
>>>>>>>  DECLARE_GLOBAL_DATA_PTR;
>>>>>>>
>>>>>>> +extern void ich_spi_config_opcode(struct udevice *dev);
>>>>>>> +
>>>>>>>  int checkcpu(void)
>>>>>>>  {
>>>>>>>         return 0;
>>>>>>> @@ -49,6 +51,28 @@ void board_final_cleanup(void)
>>>>>>>  {
>>>>>>>         u32 status;
>>>>>>>
>>>>>>> +#ifdef CONFIG_FSP_LOCKDOWN_SPI
>>>>>>> +       struct udevice *dev;
>>>>>>> +
>>>>>>> +       /*
>>>>>>> +        * Some Intel FSP (like Braswell) does SPI lock-down during the call
>>>>>>> +        * to fsp_notify(INIT_PHASE_BOOT). But before SPI lock-down is done,
>>>>>>> +        * it's bootloader's responsibility to configure the SPI controller's
>>>>>>> +        * opcode registers properly otherwise SPI controller driver doesn't
>>>>>>> +        * know how to communicate with the SPI flash device.
>>>>>>> +        *
>>>>>>> +        * Note we cannot do such configuration elsewhere (eg: during the SPI
>>>>>>> +        * controller driver's probe() routine), becasue:
>>>>>>> +        *
>>>>>>> +        * 1). U-Boot SPI controller driver does not set the lock-down bit
>>>>>>> +        * 2). Any SPI transfer will corrupt the contents of these registers
>>>>>>> +        *
>>>>>>> +        * Hence we have to do it right here before SPI lock-down bit is set.
>>>>>>> +        */
>>>>>>> +       if (!uclass_first_device_err(UCLASS_SPI, &dev))
>>>>>>> +               ich_spi_config_opcode(dev);
>>>>>>
>>>>>> I wonder if we could do this by using an operation instead of directly
>>>>>> calling a function in the driver?
>>>>>>
>>>>>
>>>>> Do you mean adding one operation to dm_spi_ops?
>>>>
>>>> Yes I think that would be better.
>>>>
>>>
>>> Since this is x86-specific, I would hesitate to add one.
>>>
>>
>> Yes I understand that. Still I don't think we should call directly
>> into drivers. What do you suggest?
>>
>> - add some sort of DM event system to which drivers can attach for notifications
>> - add an ioctl() method to the SPI uclass where we can put random
>> calls like this
>> - set up a new MISC device as a child of SPI which includes an ioctl
>> for this operation
>> - something else?
>>
>
> These are maybe too complicated to solve this little specific issue.
> I've thought about this, and looks we can add a simple DTS property
> "intel,spi-lock-down" and let the driver call this function.

OK, sounds good so long as it knows when to call it.

Regards,
Simon