[U-Boot] [PATCH 1/5 v2] dm: device_remove: Don't return in device_chld_remove() upon error

[U-Boot] [PATCH 1/5 v2] dm: device_remove: Don't return in device_chld_remove() upon error

[Stefan Roese]

On my x86 platform I've noticed, that calling dm_uninit() or the new
function dm_remove_devices_flags() does not remove the desired device at
all. Debugging showed, that the serial uclass returns -EPERM in
serial_pre_remove() and this leads to a complete stop of the device
removal pretty early, as the serial device is one of the first ones in
the DM. Here the dm tree output:

=> dm tree
 Class       Probed   Name
----------------------------------------
 root        [ + ]    root_driver
 rsa_mod_exp [   ]    |-- mod_exp_sw
 serial      [ + ]    |-- serial
 rtc         [   ]    |-- rtc
 timer       [ + ]    |-- tsc-timer
 syscon      [ + ]    |-- pch_pinctrl
 ...

In this example, device_remove(root) will stop directly after trying to
remove the "serial" device.

To solve this problem, this patch removes the return upon error check in
the device_remove() call in device_chld_remove(). This leads to
device_chld_remove() continuing with the device_remove() call to the
following child devices.

Signed-off-by: Stefan Roese <sr@denx.de>
Cc: Simon Glass <sjg@chromium.org>
Cc: Bin Meng <bmeng.cn@gmail.com>
---
v2:
- Add debug() output in error case

 drivers/core/device-remove.c | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

diff --git a/drivers/core/device-remove.c b/drivers/core/device-remove.c
index cc0043b990..a1c0103af0 100644
--- a/drivers/core/device-remove.c
+++ b/drivers/core/device-remove.c
@@ -58,8 +58,14 @@ static int device_chld_remove(struct udevice *dev, uint flags)
 
 	list_for_each_entry_safe(pos, n, &dev->child_head, sibling_node) {
 		ret = device_remove(pos, flags);
-		if (ret)
-			return ret;
+		/*
+		 * Don't stop on error here, the remaining child devices still
+		 * need to get removed.
+		 */
+		if (ret) {
+			debug("%s: device_remove(%s) returned %d\n",
+			      __func__, pos->name, ret);
+		}
 	}
 
 	return 0;
-- 
2.12.2

[U-Boot] [PATCH 2/5 v2] serial: serial-uclass: Use force parameter in stdio_deregister_dev()

[Stefan Roese]

On my x86 platform I've noticed, that calling dm_uninit() or the new
function dm_remove_devices_flags() does not remove the desired device at
all. Debugging showed, that the serial uclass returns -EPERM in
serial_pre_remove(). This patch sets the force parameter when calling
stdio_deregister_dev() resulting in a removal of the device.

Signed-off-by: Stefan Roese <sr@denx.de>
Cc: Simon Glass <sjg@chromium.org>
Cc: Bin Meng <bmeng.cn@gmail.com>
---
v2:
- New patch

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

diff --git a/drivers/serial/serial-uclass.c b/drivers/serial/serial-uclass.c
index 43c028ebe6..c2b9c5f12f 100644
--- a/drivers/serial/serial-uclass.c
+++ b/drivers/serial/serial-uclass.c
@@ -349,7 +349,7 @@ static int serial_pre_remove(struct udevice *dev)
 #if CONFIG_IS_ENABLED(SYS_STDIO_DEREGISTER)
 	struct serial_dev_priv *upriv = dev_get_uclass_priv(dev);
 
-	if (stdio_deregister_dev(upriv->sdev, 0))
+	if (stdio_deregister_dev(upriv->sdev, true))
 		return -EPERM;
 #endif
 
-- 
2.12.2

[U-Boot] [PATCH 3/5 v2] dm: core: Add DM_FLAG_OS_PREPARE flag

[Stefan Roese]

This new flag can be added to DM device drivers, which need to do some
final configuration before U-Boot exits and the OS (e.g. Linux) is
started. The remove functions of those drivers will get called at
this stage to do these last-stage configuration steps.

Signed-off-by: Stefan Roese <sr@denx.de>
Reviewed-by: Simon Glass <sjg@chromium.org>
Cc: Bin Meng <bmeng.cn@gmail.com>
---
- Renamed flag to DM_FLAG_OS_PREPARE

 drivers/core/device-remove.c | 16 +++++++++++-----
 include/dm/device.h          | 11 ++++++++++-
 2 files changed, 21 insertions(+), 6 deletions(-)

diff --git a/drivers/core/device-remove.c b/drivers/core/device-remove.c
index a1c0103af0..1ea2b0ae00 100644
--- a/drivers/core/device-remove.c
+++ b/drivers/core/device-remove.c
@@ -158,6 +158,15 @@ void device_free(struct udevice *dev)
 	devres_release_probe(dev);
 }
 
+static bool flags_remove(uint flags, uint drv_flags)
+{
+	if ((flags & DM_REMOVE_NORMAL) ||
+	    (flags & (drv_flags & (DM_FLAG_ACTIVE_DMA | DM_FLAG_OS_PREPARE))))
+		return true;
+
+	return false;
+}
+
 int device_remove(struct udevice *dev, uint flags)
 {
 	const struct driver *drv;
@@ -184,9 +193,7 @@ int device_remove(struct udevice *dev, uint flags)
 	 * Remove the device if called with the "normal" remove flag set,
 	 * or if the remove flag matches any of the drivers remove flags
 	 */
-	if (drv->remove &&
-	    ((flags & DM_REMOVE_NORMAL) ||
-	     (flags & (drv->flags & DM_FLAG_ACTIVE_DMA)))) {
+	if (drv->remove && flags_remove(flags, drv->flags)) {
 		ret = drv->remove(dev);
 		if (ret)
 			goto err_remove;
@@ -200,8 +207,7 @@ int device_remove(struct udevice *dev, uint flags)
 		}
 	}
 
-	if ((flags & DM_REMOVE_NORMAL) ||
-	    (flags & (drv->flags & DM_FLAG_ACTIVE_DMA))) {
+	if (flags_remove(flags, drv->flags)) {
 		device_free(dev);
 
 		dev->seq = -1;
diff --git a/include/dm/device.h b/include/dm/device.h
index 079ec57003..df02e41df3 100644
--- a/include/dm/device.h
+++ b/include/dm/device.h
@@ -55,6 +55,12 @@ struct driver_info;
 #define DM_FLAG_ACTIVE_DMA		(1 << 9)
 
 /*
+ * Call driver remove function to do some final configuration, before
+ * U-Boot exits and the OS is started
+ */
+#define DM_FLAG_OS_PREPARE		(1 << 10)
+
+/*
  * One or multiple of these flags are passed to device_remove() so that
  * a selective device removal as specified by the remove-stage and the
  * driver flags can be done.
@@ -66,10 +72,13 @@ enum {
 	/* Remove devices with active DMA */
 	DM_REMOVE_ACTIVE_DMA = DM_FLAG_ACTIVE_DMA,
 
+	/* Remove devices which need some final OS preparation steps */
+	DM_REMOVE_OS_PREPARE = DM_FLAG_OS_PREPARE,
+
 	/* Add more use cases here */
 
 	/* Remove devices with any active flag */
-	DM_REMOVE_ACTIVE_ALL = DM_REMOVE_ACTIVE_DMA,
+	DM_REMOVE_ACTIVE_ALL = DM_REMOVE_ACTIVE_DMA | DM_REMOVE_OS_PREPARE,
 };
 
 /**
-- 
2.12.2

[U-Boot] [PATCH 4/5 v2] x86: bootm: Add dm_remove_devices_flags() call to bootm_announce_and_cleanup()

[Stefan Roese]

This patch adds a call to dm_remove_devices_flags() to
bootm_announce_and_cleanup() so that drivers that have one of the removal
flags set (e.g. DM_FLAG_ACTIVE_DMA_REMOVE) in their driver struct, may
do some last-stage cleanup before the OS is started.

Signed-off-by: Stefan Roese <sr@denx.de>
Cc: Bin Meng <bmeng.cn@gmail.com>
Reviewed-by: Simon Glass <sjg@chromium.org>
---
v2:
- Added Simons RB line

 arch/x86/lib/bootm.c | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/arch/x86/lib/bootm.c b/arch/x86/lib/bootm.c
index 75bab90225..ecd4f4e6c6 100644
--- a/arch/x86/lib/bootm.c
+++ b/arch/x86/lib/bootm.c
@@ -10,6 +10,8 @@
 
 #include <common.h>
 #include <command.h>
+#include <dm/device.h>
+#include <dm/root.h>
 #include <errno.h>
 #include <fdt_support.h>
 #include <image.h>
@@ -46,6 +48,13 @@ void bootm_announce_and_cleanup(void)
 #ifdef CONFIG_BOOTSTAGE_REPORT
 	bootstage_report();
 #endif
+
+	/*
+	 * Call remove function of all devices with a removal flag set.
+	 * This may be useful for last-stage operations, like cancelling
+	 * of DMA operation or releasing device internal buffers.
+	 */
+	dm_remove_devices_flags(DM_REMOVE_ACTIVE_ALL);
 }
 
 #if defined(CONFIG_OF_LIBFDT) && !defined(CONFIG_OF_NO_KERNEL)
-- 
2.12.2

[U-Boot] [PATCH 5/5 v2] spi: ich: Configure SPI BIOS parameters for Linux upon U-Boot exit

[Stefan Roese]

This patch adds a remove function to the Intel ICH SPI driver, that will
be called upon U-Boot exit, directly before the OS (Linux) is started.
This function takes care of configuring the BIOS registers in the SPI
controller (similar to what a "standard" BIOS or coreboot does), so that
the Linux MTD device driver is able to correctly read/write to the SPI
NOR chip. Without this, the chip is not detected at all.

Signed-off-by: Stefan Roese <sr@denx.de>
Reviewed-by: Simon Glass <sjg@chromium.org>
Cc: Bin Meng <bmeng.cn@gmail.com>
Cc: Jagan Teki <jteki@openedev.com>
---
v2:
- Added Simons RB line

 drivers/spi/ich.c | 18 ++++++++++++++++++
 drivers/spi/ich.h | 54 +++++++++++++++++++++++++++++++++++++++++++++++-------
 2 files changed, 65 insertions(+), 7 deletions(-)

diff --git a/drivers/spi/ich.c b/drivers/spi/ich.c
index 893fe33b66..bf2e99b5cc 100644
--- a/drivers/spi/ich.c
+++ b/drivers/spi/ich.c
@@ -617,6 +617,22 @@ static int ich_spi_probe(struct udevice *dev)
 	return 0;
 }
 
+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));
+
+	return 0;
+}
+
 static int ich_spi_set_speed(struct udevice *bus, uint speed)
 {
 	struct ich_spi_priv *priv = dev_get_priv(bus);
@@ -700,4 +716,6 @@ U_BOOT_DRIVER(ich_spi) = {
 	.priv_auto_alloc_size = sizeof(struct ich_spi_priv),
 	.child_pre_probe = ich_spi_child_pre_probe,
 	.probe	= ich_spi_probe,
+	.remove	= ich_spi_remove,
+	.flags	= DM_FLAG_OS_PREPARE,
 };
diff --git a/drivers/spi/ich.h b/drivers/spi/ich.h
index bd0a820809..dcb8a9048f 100644
--- a/drivers/spi/ich.h
+++ b/drivers/spi/ich.h
@@ -102,13 +102,6 @@ enum {
 };
 
 enum {
-	SPI_OPCODE_TYPE_READ_NO_ADDRESS =	0,
-	SPI_OPCODE_TYPE_WRITE_NO_ADDRESS =	1,
-	SPI_OPCODE_TYPE_READ_WITH_ADDRESS =	2,
-	SPI_OPCODE_TYPE_WRITE_WITH_ADDRESS =	3
-};
-
-enum {
 	ICH_MAX_CMD_LEN		= 5,
 };
 
@@ -124,8 +117,55 @@ struct spi_trans {
 	uint32_t offset;
 };
 
+#define SPI_OPCODE_WRSR		0x01
+#define SPI_OPCODE_PAGE_PROGRAM	0x02
+#define SPI_OPCODE_READ		0x03
+#define SPI_OPCODE_WRDIS	0x04
+#define SPI_OPCODE_RDSR		0x05
 #define SPI_OPCODE_WREN		0x06
 #define SPI_OPCODE_FAST_READ	0x0b
+#define SPI_OPCODE_ERASE_SECT	0x20
+#define SPI_OPCODE_READ_ID	0x9f
+#define SPI_OPCODE_ERASE_BLOCK	0xd8
+
+#define SPI_OPCODE_TYPE_READ_NO_ADDRESS		0
+#define SPI_OPCODE_TYPE_WRITE_NO_ADDRESS	1
+#define SPI_OPCODE_TYPE_READ_WITH_ADDRESS	2
+#define SPI_OPCODE_TYPE_WRITE_WITH_ADDRESS	3
+
+#define SPI_OPMENU_0	SPI_OPCODE_WRSR
+#define SPI_OPTYPE_0	SPI_OPCODE_TYPE_WRITE_NO_ADDRESS
+
+#define SPI_OPMENU_1	SPI_OPCODE_PAGE_PROGRAM
+#define SPI_OPTYPE_1	SPI_OPCODE_TYPE_WRITE_WITH_ADDRESS
+
+#define SPI_OPMENU_2	SPI_OPCODE_READ
+#define SPI_OPTYPE_2	SPI_OPCODE_TYPE_READ_WITH_ADDRESS
+
+#define SPI_OPMENU_3	SPI_OPCODE_RDSR
+#define SPI_OPTYPE_3	SPI_OPCODE_TYPE_READ_NO_ADDRESS
+
+#define SPI_OPMENU_4	SPI_OPCODE_ERASE_SECT
+#define SPI_OPTYPE_4	SPI_OPCODE_TYPE_WRITE_WITH_ADDRESS
+
+#define SPI_OPMENU_5	SPI_OPCODE_READ_ID
+#define SPI_OPTYPE_5	SPI_OPCODE_TYPE_READ_NO_ADDRESS
+
+#define SPI_OPMENU_6	SPI_OPCODE_ERASE_BLOCK
+#define SPI_OPTYPE_6	SPI_OPCODE_TYPE_WRITE_WITH_ADDRESS
+
+#define SPI_OPMENU_7	SPI_OPCODE_FAST_READ
+#define SPI_OPTYPE_7	SPI_OPCODE_TYPE_READ_WITH_ADDRESS
+
+#define SPI_OPPREFIX	((SPI_OPCODE_WREN << 8) | SPI_OPCODE_WREN)
+#define SPI_OPTYPE	((SPI_OPTYPE_7 << 14) | (SPI_OPTYPE_6 << 12) | \
+			 (SPI_OPTYPE_5 << 10) | (SPI_OPTYPE_4 <<  8) | \
+			 (SPI_OPTYPE_3 <<  6) | (SPI_OPTYPE_2 <<  4) | \
+			 (SPI_OPTYPE_1 <<  2) | (SPI_OPTYPE_0 <<  0))
+#define SPI_OPMENU_UPPER ((SPI_OPMENU_7 << 24) | (SPI_OPMENU_6 << 16) | \
+			  (SPI_OPMENU_5 <<  8) | (SPI_OPMENU_4 <<  0))
+#define SPI_OPMENU_LOWER ((SPI_OPMENU_3 << 24) | (SPI_OPMENU_2 << 16) | \
+			  (SPI_OPMENU_1 <<  8) | (SPI_OPMENU_0 <<  0))
 
 enum ich_version {
 	ICHV_7,
-- 
2.12.2

[U-Boot] [PATCH 2/5 v2] serial: serial-uclass: Use force parameter in stdio_deregister_dev()

[Simon Glass]

On 24 April 2017 at 01:48, Stefan Roese <sr@denx.de> wrote:
> On my x86 platform I've noticed, that calling dm_uninit() or the new
> function dm_remove_devices_flags() does not remove the desired device at
> all. Debugging showed, that the serial uclass returns -EPERM in
> serial_pre_remove(). This patch sets the force parameter when calling
> stdio_deregister_dev() resulting in a removal of the device.
>
> Signed-off-by: Stefan Roese <sr@denx.de>
> Cc: Simon Glass <sjg@chromium.org>
> Cc: Bin Meng <bmeng.cn@gmail.com>
> ---
> v2:
> - New patch
>
>  drivers/serial/serial-uclass.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)

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

[U-Boot] [PATCH 1/5 v2] dm: device_remove: Don't return in device_chld_remove() upon error

[Simon Glass]

Hi Stefan,

On 24 April 2017 at 01:48, Stefan Roese <sr@denx.de> wrote:
> On my x86 platform I've noticed, that calling dm_uninit() or the new
> function dm_remove_devices_flags() does not remove the desired device at
> all. Debugging showed, that the serial uclass returns -EPERM in
> serial_pre_remove() and this leads to a complete stop of the device
> removal pretty early, as the serial device is one of the first ones in
> the DM. Here the dm tree output:
>
> => dm tree
>  Class       Probed   Name
> ----------------------------------------
>  root        [ + ]    root_driver
>  rsa_mod_exp [   ]    |-- mod_exp_sw
>  serial      [ + ]    |-- serial
>  rtc         [   ]    |-- rtc
>  timer       [ + ]    |-- tsc-timer
>  syscon      [ + ]    |-- pch_pinctrl
>  ...
>
> In this example, device_remove(root) will stop directly after trying to
> remove the "serial" device.
>
> To solve this problem, this patch removes the return upon error check in
> the device_remove() call in device_chld_remove(). This leads to
> device_chld_remove() continuing with the device_remove() call to the
> following child devices.
>
> Signed-off-by: Stefan Roese <sr@denx.de>
> Cc: Simon Glass <sjg@chromium.org>
> Cc: Bin Meng <bmeng.cn@gmail.com>
> ---
> v2:
> - Add debug() output in error case
>
>  drivers/core/device-remove.c | 10 ++++++++--
>  1 file changed, 8 insertions(+), 2 deletions(-)

I thought that your change to force removal of the stdio dev would
make this change unnecessary?

I really would rather fix the root cause if we can.

>
> diff --git a/drivers/core/device-remove.c b/drivers/core/device-remove.c
> index cc0043b990..a1c0103af0 100644
> --- a/drivers/core/device-remove.c
> +++ b/drivers/core/device-remove.c
> @@ -58,8 +58,14 @@ static int device_chld_remove(struct udevice *dev, uint flags)
>
>         list_for_each_entry_safe(pos, n, &dev->child_head, sibling_node) {
>                 ret = device_remove(pos, flags);
> -               if (ret)
> -                       return ret;
> +               /*
> +                * Don't stop on error here, the remaining child devices still
> +                * need to get removed.
> +                */
> +               if (ret) {
> +                       debug("%s: device_remove(%s) returned %d\n",
> +                             __func__, pos->name, ret);
> +               }
>         }
>
>         return 0;
> --
> 2.12.2
>

Regards,
Simon

[U-Boot] [PATCH 1/5 v2] dm: device_remove: Don't return in device_chld_remove() upon error

[Stefan Roese]

Hi Simon,

On 29.04.2017 02:26, Simon Glass wrote:
> On 24 April 2017 at 01:48, Stefan Roese <sr@denx.de> wrote:
>> On my x86 platform I've noticed, that calling dm_uninit() or the new
>> function dm_remove_devices_flags() does not remove the desired device at
>> all. Debugging showed, that the serial uclass returns -EPERM in
>> serial_pre_remove() and this leads to a complete stop of the device
>> removal pretty early, as the serial device is one of the first ones in
>> the DM. Here the dm tree output:
>>
>> => dm tree
>>  Class       Probed   Name
>> ----------------------------------------
>>  root        [ + ]    root_driver
>>  rsa_mod_exp [   ]    |-- mod_exp_sw
>>  serial      [ + ]    |-- serial
>>  rtc         [   ]    |-- rtc
>>  timer       [ + ]    |-- tsc-timer
>>  syscon      [ + ]    |-- pch_pinctrl
>>  ...
>>
>> In this example, device_remove(root) will stop directly after trying to
>> remove the "serial" device.
>>
>> To solve this problem, this patch removes the return upon error check in
>> the device_remove() call in device_chld_remove(). This leads to
>> device_chld_remove() continuing with the device_remove() call to the
>> following child devices.
>>
>> Signed-off-by: Stefan Roese <sr@denx.de>
>> Cc: Simon Glass <sjg@chromium.org>
>> Cc: Bin Meng <bmeng.cn@gmail.com>
>> ---
>> v2:
>> - Add debug() output in error case
>>
>>  drivers/core/device-remove.c | 10 ++++++++--
>>  1 file changed, 8 insertions(+), 2 deletions(-)
>
> I thought that your change to force removal of the stdio dev would
> make this change unnecessary?

Yes, the force removal made this change unnecessary in this specific
case. But...

> I really would rather fix the root cause if we can.

... the current implementation to exit the loop over all children
upon error and not remove the remaining children is wrong IMO. All
devices should at least be tried to get removed, even if one fails
to get removed. This is what this patch makes sure of.

Thanks,
Stefan

[U-Boot] [PATCH 1/5 v2] dm: device_remove: Don't return in device_chld_remove() upon error

[Simon Glass]

Hi Stefan,

On 1 May 2017 at 00:14, Stefan Roese <sr@denx.de> wrote:
> Hi Simon,
>
>
> On 29.04.2017 02:26, Simon Glass wrote:
>>
>> On 24 April 2017 at 01:48, Stefan Roese <sr@denx.de> wrote:
>>>
>>> On my x86 platform I've noticed, that calling dm_uninit() or the new
>>> function dm_remove_devices_flags() does not remove the desired device at
>>> all. Debugging showed, that the serial uclass returns -EPERM in
>>> serial_pre_remove() and this leads to a complete stop of the device
>>> removal pretty early, as the serial device is one of the first ones in
>>> the DM. Here the dm tree output:
>>>
>>> => dm tree
>>>  Class       Probed   Name
>>> ----------------------------------------
>>>  root        [ + ]    root_driver
>>>  rsa_mod_exp [   ]    |-- mod_exp_sw
>>>  serial      [ + ]    |-- serial
>>>  rtc         [   ]    |-- rtc
>>>  timer       [ + ]    |-- tsc-timer
>>>  syscon      [ + ]    |-- pch_pinctrl
>>>  ...
>>>
>>> In this example, device_remove(root) will stop directly after trying to
>>> remove the "serial" device.
>>>
>>> To solve this problem, this patch removes the return upon error check in
>>> the device_remove() call in device_chld_remove(). This leads to
>>> device_chld_remove() continuing with the device_remove() call to the
>>> following child devices.
>>>
>>> Signed-off-by: Stefan Roese <sr@denx.de>
>>> Cc: Simon Glass <sjg@chromium.org>
>>> Cc: Bin Meng <bmeng.cn@gmail.com>
>>> ---
>>> v2:
>>> - Add debug() output in error case
>>>
>>>  drivers/core/device-remove.c | 10 ++++++++--
>>>  1 file changed, 8 insertions(+), 2 deletions(-)
>>
>>
>> I thought that your change to force removal of the stdio dev would
>> make this change unnecessary?
>
>
> Yes, the force removal made this change unnecessary in this specific
> case. But...
>
>> I really would rather fix the root cause if we can.
>
>
> ... the current implementation to exit the loop over all children
> upon error and not remove the remaining children is wrong IMO. All
> devices should at least be tried to get removed, even if one fails
> to get removed. This is what this patch makes sure of.

Yes I see that, but not being able to remove is actually an error. In
the normal course of events, a device that will not remove itself is
likely buggy.

What do you think about adding a new remove flag to indicate that
failures should be skipped?

Regards,
Simon

[U-Boot] [PATCH 1/5 v2] dm: device_remove: Don't return in device_chld_remove() upon error

[Stefan Roese]

Hi Simon,

On 02.05.2017 13:27, Simon Glass wrote:
> On 1 May 2017 at 00:14, Stefan Roese <sr@denx.de> wrote:
>> Hi Simon,
>>
>>
>> On 29.04.2017 02:26, Simon Glass wrote:
>>>
>>> On 24 April 2017 at 01:48, Stefan Roese <sr@denx.de> wrote:
>>>>
>>>> On my x86 platform I've noticed, that calling dm_uninit() or the new
>>>> function dm_remove_devices_flags() does not remove the desired device at
>>>> all. Debugging showed, that the serial uclass returns -EPERM in
>>>> serial_pre_remove() and this leads to a complete stop of the device
>>>> removal pretty early, as the serial device is one of the first ones in
>>>> the DM. Here the dm tree output:
>>>>
>>>> => dm tree
>>>>  Class       Probed   Name
>>>> ----------------------------------------
>>>>  root        [ + ]    root_driver
>>>>  rsa_mod_exp [   ]    |-- mod_exp_sw
>>>>  serial      [ + ]    |-- serial
>>>>  rtc         [   ]    |-- rtc
>>>>  timer       [ + ]    |-- tsc-timer
>>>>  syscon      [ + ]    |-- pch_pinctrl
>>>>  ...
>>>>
>>>> In this example, device_remove(root) will stop directly after trying to
>>>> remove the "serial" device.
>>>>
>>>> To solve this problem, this patch removes the return upon error check in
>>>> the device_remove() call in device_chld_remove(). This leads to
>>>> device_chld_remove() continuing with the device_remove() call to the
>>>> following child devices.
>>>>
>>>> Signed-off-by: Stefan Roese <sr@denx.de>
>>>> Cc: Simon Glass <sjg@chromium.org>
>>>> Cc: Bin Meng <bmeng.cn@gmail.com>
>>>> ---
>>>> v2:
>>>> - Add debug() output in error case
>>>>
>>>>  drivers/core/device-remove.c | 10 ++++++++--
>>>>  1 file changed, 8 insertions(+), 2 deletions(-)
>>>
>>>
>>> I thought that your change to force removal of the stdio dev would
>>> make this change unnecessary?
>>
>>
>> Yes, the force removal made this change unnecessary in this specific
>> case. But...
>>
>>> I really would rather fix the root cause if we can.
>>
>>
>> ... the current implementation to exit the loop over all children
>> upon error and not remove the remaining children is wrong IMO. All
>> devices should at least be tried to get removed, even if one fails
>> to get removed. This is what this patch makes sure of.
>
> Yes I see that, but not being able to remove is actually an error. In
> the normal course of events, a device that will not remove itself is
> likely buggy.

Isn't it enough then to just print an error message in this case
in this loop - change debug() to printf() in this current patch
version? Then "users" of this code will be aware of such remove
failures and can take appropriate actions (fix bug etc in their
setup).

> What do you think about adding a new remove flag to indicate that
> failures should be skipped?

I'm a bit afraid that this makes the code overly complex. But if
you prefer to have it this way, then I can come up with such a
version as well. Just let me know.

Thanks,
Stefan

[U-Boot] [PATCH 1/5 v2] dm: device_remove: Don't return in device_chld_remove() upon error

[Simon Glass]

Hi Stefan,

On 2 May 2017 at 05:45, Stefan Roese <sr@denx.de> wrote:
> Hi Simon,
>
>
> On 02.05.2017 13:27, Simon Glass wrote:
>>
>> On 1 May 2017 at 00:14, Stefan Roese <sr@denx.de> wrote:
>>>
>>> Hi Simon,
>>>
>>>
>>> On 29.04.2017 02:26, Simon Glass wrote:
>>>>
>>>>
>>>> On 24 April 2017 at 01:48, Stefan Roese <sr@denx.de> wrote:
>>>>>
>>>>>
>>>>> On my x86 platform I've noticed, that calling dm_uninit() or the new
>>>>> function dm_remove_devices_flags() does not remove the desired device
>>>>> at
>>>>> all. Debugging showed, that the serial uclass returns -EPERM in
>>>>> serial_pre_remove() and this leads to a complete stop of the device
>>>>> removal pretty early, as the serial device is one of the first ones in
>>>>> the DM. Here the dm tree output:
>>>>>
>>>>> => dm tree
>>>>>  Class       Probed   Name
>>>>> ----------------------------------------
>>>>>  root        [ + ]    root_driver
>>>>>  rsa_mod_exp [   ]    |-- mod_exp_sw
>>>>>  serial      [ + ]    |-- serial
>>>>>  rtc         [   ]    |-- rtc
>>>>>  timer       [ + ]    |-- tsc-timer
>>>>>  syscon      [ + ]    |-- pch_pinctrl
>>>>>  ...
>>>>>
>>>>> In this example, device_remove(root) will stop directly after trying to
>>>>> remove the "serial" device.
>>>>>
>>>>> To solve this problem, this patch removes the return upon error check
>>>>> in
>>>>> the device_remove() call in device_chld_remove(). This leads to
>>>>> device_chld_remove() continuing with the device_remove() call to the
>>>>> following child devices.
>>>>>
>>>>> Signed-off-by: Stefan Roese <sr@denx.de>
>>>>> Cc: Simon Glass <sjg@chromium.org>
>>>>> Cc: Bin Meng <bmeng.cn@gmail.com>
>>>>> ---
>>>>> v2:
>>>>> - Add debug() output in error case
>>>>>
>>>>>  drivers/core/device-remove.c | 10 ++++++++--
>>>>>  1 file changed, 8 insertions(+), 2 deletions(-)
>>>>
>>>>
>>>>
>>>> I thought that your change to force removal of the stdio dev would
>>>> make this change unnecessary?
>>>
>>>
>>>
>>> Yes, the force removal made this change unnecessary in this specific
>>> case. But...
>>>
>>>> I really would rather fix the root cause if we can.
>>>
>>>
>>>
>>> ... the current implementation to exit the loop over all children
>>> upon error and not remove the remaining children is wrong IMO. All
>>> devices should at least be tried to get removed, even if one fails
>>> to get removed. This is what this patch makes sure of.
>>
>>
>> Yes I see that, but not being able to remove is actually an error. In
>> the normal course of events, a device that will not remove itself is
>> likely buggy.
>
>
> Isn't it enough then to just print an error message in this case
> in this loop - change debug() to printf() in this current patch
> version? Then "users" of this code will be aware of such remove
> failures and can take appropriate actions (fix bug etc in their
> setup).

Possibly, but programmatically it becomes impossible to detect a
failure. Say the USB fails to stop its DMA, we might want to reboot
rather than continue to boot Linux and crash.

>
>> What do you think about adding a new remove flag to indicate that
>> failures should be skipped?
>
>
> I'm a bit afraid that this makes the code overly complex. But if
> you prefer to have it this way, then I can come up with such a
> version as well. Just let me know.

I don't think the complexity is too great. It does need a new test.
But I think we should hold the line on error checking even with
remove(), by default.

>
> Thanks,
> Stefan

Regards,
Simon

[U-Boot] [PATCH 1/5 v2] dm: device_remove: Don't return in device_chld_remove() upon error

[Stefan Roese]

Hi Simon,

On 04.05.2017 18:50, Simon Glass wrote:

<snip>

>>>> ... the current implementation to exit the loop over all children
>>>> upon error and not remove the remaining children is wrong IMO. All
>>>> devices should at least be tried to get removed, even if one fails
>>>> to get removed. This is what this patch makes sure of.
>>>
>>>
>>> Yes I see that, but not being able to remove is actually an error. In
>>> the normal course of events, a device that will not remove itself is
>>> likely buggy.
>>
>>
>> Isn't it enough then to just print an error message in this case
>> in this loop - change debug() to printf() in this current patch
>> version? Then "users" of this code will be aware of such remove
>> failures and can take appropriate actions (fix bug etc in their
>> setup).
>
> Possibly, but programmatically it becomes impossible to detect a
> failure. Say the USB fails to stop its DMA, we might want to reboot
> rather than continue to boot Linux and crash.

Okay, I see your point.

>>
>>> What do you think about adding a new remove flag to indicate that
>>> failures should be skipped?
>>
>>
>> I'm a bit afraid that this makes the code overly complex. But if
>> you prefer to have it this way, then I can come up with such a
>> version as well. Just let me know.
>
> I don't think the complexity is too great. It does need a new test.
> But I think we should hold the line on error checking even with
> remove(), by default.

Understood. How about doing it this way: You drop this patch from
the series for now (it still works for us with the force remove of
the serial driver) and apply the remaining patches. I'll try to get
back to this skip-failures flag implementation in a few days / weeks.

Is this okay for you?

Thanks,
Stefan

[U-Boot] [PATCH 1/5 v2] dm: device_remove: Don't return in device_chld_remove() upon error

[Simon Glass]

Hi Stefan,

On 8 May 2017 at 01:35, Stefan Roese <sr@denx.de> wrote:
> Hi Simon,
>
> On 04.05.2017 18:50, Simon Glass wrote:
>
> <snip>
>
>>>>> ... the current implementation to exit the loop over all children
>>>>> upon error and not remove the remaining children is wrong IMO. All
>>>>> devices should at least be tried to get removed, even if one fails
>>>>> to get removed. This is what this patch makes sure of.
>>>>
>>>>
>>>>
>>>> Yes I see that, but not being able to remove is actually an error. In
>>>> the normal course of events, a device that will not remove itself is
>>>> likely buggy.
>>>
>>>
>>>
>>> Isn't it enough then to just print an error message in this case
>>> in this loop - change debug() to printf() in this current patch
>>> version? Then "users" of this code will be aware of such remove
>>> failures and can take appropriate actions (fix bug etc in their
>>> setup).
>>
>>
>> Possibly, but programmatically it becomes impossible to detect a
>> failure. Say the USB fails to stop its DMA, we might want to reboot
>> rather than continue to boot Linux and crash.
>
>
> Okay, I see your point.
>
>>>
>>>> What do you think about adding a new remove flag to indicate that
>>>> failures should be skipped?
>>>
>>>
>>>
>>> I'm a bit afraid that this makes the code overly complex. But if
>>> you prefer to have it this way, then I can come up with such a
>>> version as well. Just let me know.
>>
>>
>> I don't think the complexity is too great. It does need a new test.
>> But I think we should hold the line on error checking even with
>> remove(), by default.
>
>
> Understood. How about doing it this way: You drop this patch from
> the series for now (it still works for us with the force remove of
> the serial driver) and apply the remaining patches. I'll try to get
> back to this skip-failures flag implementation in a few days / weeks.
>
> Is this okay for you?

Yes that's fine.

>
> Thanks,
> Stefan

Regards,
Simon

[U-Boot] [PATCH 2/5 v2] serial: serial-uclass: Use force parameter in stdio_deregister_dev()

[Bin Meng]

On Sat, Apr 29, 2017 at 8:26 AM, Simon Glass <sjg@chromium.org> wrote:
> On 24 April 2017 at 01:48, Stefan Roese <sr@denx.de> wrote:
>> On my x86 platform I've noticed, that calling dm_uninit() or the new
>> function dm_remove_devices_flags() does not remove the desired device at
>> all. Debugging showed, that the serial uclass returns -EPERM in
>> serial_pre_remove(). This patch sets the force parameter when calling
>> stdio_deregister_dev() resulting in a removal of the device.
>>
>> Signed-off-by: Stefan Roese <sr@denx.de>
>> Cc: Simon Glass <sjg@chromium.org>
>> Cc: Bin Meng <bmeng.cn@gmail.com>
>> ---
>> v2:
>> - New patch
>>
>>  drivers/serial/serial-uclass.c | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> Reviewed-by: Simon Glass <sjg@chromium.org>

applied to u-boot-x86, thanks!

[U-Boot] [PATCH 3/5 v2] dm: core: Add DM_FLAG_OS_PREPARE flag

[Bin Meng]

On Mon, Apr 24, 2017 at 3:48 PM, Stefan Roese <sr@denx.de> wrote:
> This new flag can be added to DM device drivers, which need to do some
> final configuration before U-Boot exits and the OS (e.g. Linux) is
> started. The remove functions of those drivers will get called at
> this stage to do these last-stage configuration steps.
>
> Signed-off-by: Stefan Roese <sr@denx.de>
> Reviewed-by: Simon Glass <sjg@chromium.org>
> Cc: Bin Meng <bmeng.cn@gmail.com>
> ---
> - Renamed flag to DM_FLAG_OS_PREPARE
>
>  drivers/core/device-remove.c | 16 +++++++++++-----
>  include/dm/device.h          | 11 ++++++++++-
>  2 files changed, 21 insertions(+), 6 deletions(-)
>

applied to u-boot-x86, thanks!

[U-Boot] [PATCH 4/5 v2] x86: bootm: Add dm_remove_devices_flags() call to bootm_announce_and_cleanup()

[Bin Meng]

On Mon, Apr 24, 2017 at 3:48 PM, Stefan Roese <sr@denx.de> wrote:
> This patch adds a call to dm_remove_devices_flags() to
> bootm_announce_and_cleanup() so that drivers that have one of the removal
> flags set (e.g. DM_FLAG_ACTIVE_DMA_REMOVE) in their driver struct, may
> do some last-stage cleanup before the OS is started.
>
> Signed-off-by: Stefan Roese <sr@denx.de>
> Cc: Bin Meng <bmeng.cn@gmail.com>
> Reviewed-by: Simon Glass <sjg@chromium.org>
> ---
> v2:
> - Added Simons RB line
>
>  arch/x86/lib/bootm.c | 9 +++++++++
>  1 file changed, 9 insertions(+)
>

applied to u-boot-x86, thanks!

[U-Boot] [PATCH 5/5 v2] spi: ich: Configure SPI BIOS parameters for Linux upon U-Boot exit

[Bin Meng]

On Mon, Apr 24, 2017 at 3:48 PM, Stefan Roese <sr@denx.de> wrote:
> This patch adds a remove function to the Intel ICH SPI driver, that will
> be called upon U-Boot exit, directly before the OS (Linux) is started.
> This function takes care of configuring the BIOS registers in the SPI
> controller (similar to what a "standard" BIOS or coreboot does), so that
> the Linux MTD device driver is able to correctly read/write to the SPI
> NOR chip. Without this, the chip is not detected at all.
>
> Signed-off-by: Stefan Roese <sr@denx.de>
> Reviewed-by: Simon Glass <sjg@chromium.org>
> Cc: Bin Meng <bmeng.cn@gmail.com>
> Cc: Jagan Teki <jteki@openedev.com>
> ---
> v2:
> - Added Simons RB line
>
>  drivers/spi/ich.c | 18 ++++++++++++++++++
>  drivers/spi/ich.h | 54 +++++++++++++++++++++++++++++++++++++++++++++++-------
>  2 files changed, 65 insertions(+), 7 deletions(-)
>

applied to u-boot-x86, thanks!

[U-Boot] [PATCH 5/5 v2] spi: ich: Configure SPI BIOS parameters for Linux upon U-Boot exit

[Jagan Teki]

On Mon, Apr 24, 2017 at 1:18 PM, Stefan Roese <sr@denx.de> wrote:
> This patch adds a remove function to the Intel ICH SPI driver, that will
> be called upon U-Boot exit, directly before the OS (Linux) is started.
> This function takes care of configuring the BIOS registers in the SPI
> controller (similar to what a "standard" BIOS or coreboot does), so that
> the Linux MTD device driver is able to correctly read/write to the SPI
> NOR chip. Without this, the chip is not detected at all.
>
> Signed-off-by: Stefan Roese <sr@denx.de>
> Reviewed-by: Simon Glass <sjg@chromium.org>
> Cc: Bin Meng <bmeng.cn@gmail.com>
> Cc: Jagan Teki <jteki@openedev.com>
> ---
> v2:
> - Added Simons RB line
>
>  drivers/spi/ich.c | 18 ++++++++++++++++++
>  drivers/spi/ich.h | 54 +++++++++++++++++++++++++++++++++++++++++++++++-------
>  2 files changed, 65 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/spi/ich.c b/drivers/spi/ich.c
> index 893fe33b66..bf2e99b5cc 100644
> --- a/drivers/spi/ich.c
> +++ b/drivers/spi/ich.c
> @@ -617,6 +617,22 @@ static int ich_spi_probe(struct udevice *dev)
>         return 0;
>  }
>
> +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));
> +
> +       return 0;
> +}
> +
>  static int ich_spi_set_speed(struct udevice *bus, uint speed)
>  {
>         struct ich_spi_priv *priv = dev_get_priv(bus);
> @@ -700,4 +716,6 @@ U_BOOT_DRIVER(ich_spi) = {
>         .priv_auto_alloc_size = sizeof(struct ich_spi_priv),
>         .child_pre_probe = ich_spi_child_pre_probe,
>         .probe  = ich_spi_probe,
> +       .remove = ich_spi_remove,
> +       .flags  = DM_FLAG_OS_PREPARE,
>  };
> diff --git a/drivers/spi/ich.h b/drivers/spi/ich.h
> index bd0a820809..dcb8a9048f 100644
> --- a/drivers/spi/ich.h
> +++ b/drivers/spi/ich.h
> @@ -102,13 +102,6 @@ enum {
>  };
>
>  enum {
> -       SPI_OPCODE_TYPE_READ_NO_ADDRESS =       0,
> -       SPI_OPCODE_TYPE_WRITE_NO_ADDRESS =      1,
> -       SPI_OPCODE_TYPE_READ_WITH_ADDRESS =     2,
> -       SPI_OPCODE_TYPE_WRITE_WITH_ADDRESS =    3
> -};
> -
> -enum {
>         ICH_MAX_CMD_LEN         = 5,
>  };
>
> @@ -124,8 +117,55 @@ struct spi_trans {
>         uint32_t offset;
>  };
>
> +#define SPI_OPCODE_WRSR                0x01
> +#define SPI_OPCODE_PAGE_PROGRAM        0x02
> +#define SPI_OPCODE_READ                0x03
> +#define SPI_OPCODE_WRDIS       0x04
> +#define SPI_OPCODE_RDSR                0x05
>  #define SPI_OPCODE_WREN                0x06
>  #define SPI_OPCODE_FAST_READ   0x0b
> +#define SPI_OPCODE_ERASE_SECT  0x20
> +#define SPI_OPCODE_READ_ID     0x9f
> +#define SPI_OPCODE_ERASE_BLOCK 0xd8

Wonder why the flash part should be part of SPI, can't we use existing
spi_flash through command interface if there is specific stuff like
this?

Same I've commented on previous version, no response either but applied?

thanks!
-- 
Jagan Teki
Free Software Engineer | www.openedev.com
U-Boot, Linux | Upstream Maintainer
Hyderabad, India.

[U-Boot] [PATCH 5/5 v2] spi: ich: Configure SPI BIOS parameters for Linux upon U-Boot exit

[Stefan Roese]

(Added Simon to Cc)

On 09.05.2017 13:14, Jagan Teki wrote:
> On Mon, Apr 24, 2017 at 1:18 PM, Stefan Roese <sr@denx.de> wrote:
>> This patch adds a remove function to the Intel ICH SPI driver, that will
>> be called upon U-Boot exit, directly before the OS (Linux) is started.
>> This function takes care of configuring the BIOS registers in the SPI
>> controller (similar to what a "standard" BIOS or coreboot does), so that
>> the Linux MTD device driver is able to correctly read/write to the SPI
>> NOR chip. Without this, the chip is not detected at all.
>>
>> Signed-off-by: Stefan Roese <sr@denx.de>
>> Reviewed-by: Simon Glass <sjg@chromium.org>
>> Cc: Bin Meng <bmeng.cn@gmail.com>
>> Cc: Jagan Teki <jteki@openedev.com>
>> ---
>> v2:
>> - Added Simons RB line
>>
>>  drivers/spi/ich.c | 18 ++++++++++++++++++
>>  drivers/spi/ich.h | 54 +++++++++++++++++++++++++++++++++++++++++++++++-------
>>  2 files changed, 65 insertions(+), 7 deletions(-)
>>
>> diff --git a/drivers/spi/ich.c b/drivers/spi/ich.c
>> index 893fe33b66..bf2e99b5cc 100644
>> --- a/drivers/spi/ich.c
>> +++ b/drivers/spi/ich.c
>> @@ -617,6 +617,22 @@ static int ich_spi_probe(struct udevice *dev)
>>         return 0;
>>  }
>>
>> +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));
>> +
>> +       return 0;
>> +}
>> +
>>  static int ich_spi_set_speed(struct udevice *bus, uint speed)
>>  {
>>         struct ich_spi_priv *priv = dev_get_priv(bus);
>> @@ -700,4 +716,6 @@ U_BOOT_DRIVER(ich_spi) = {
>>         .priv_auto_alloc_size = sizeof(struct ich_spi_priv),
>>         .child_pre_probe = ich_spi_child_pre_probe,
>>         .probe  = ich_spi_probe,
>> +       .remove = ich_spi_remove,
>> +       .flags  = DM_FLAG_OS_PREPARE,
>>  };
>> diff --git a/drivers/spi/ich.h b/drivers/spi/ich.h
>> index bd0a820809..dcb8a9048f 100644
>> --- a/drivers/spi/ich.h
>> +++ b/drivers/spi/ich.h
>> @@ -102,13 +102,6 @@ enum {
>>  };
>>
>>  enum {
>> -       SPI_OPCODE_TYPE_READ_NO_ADDRESS =       0,
>> -       SPI_OPCODE_TYPE_WRITE_NO_ADDRESS =      1,
>> -       SPI_OPCODE_TYPE_READ_WITH_ADDRESS =     2,
>> -       SPI_OPCODE_TYPE_WRITE_WITH_ADDRESS =    3
>> -};
>> -
>> -enum {
>>         ICH_MAX_CMD_LEN         = 5,
>>  };
>>
>> @@ -124,8 +117,55 @@ struct spi_trans {
>>         uint32_t offset;
>>  };
>>
>> +#define SPI_OPCODE_WRSR                0x01
>> +#define SPI_OPCODE_PAGE_PROGRAM        0x02
>> +#define SPI_OPCODE_READ                0x03
>> +#define SPI_OPCODE_WRDIS       0x04
>> +#define SPI_OPCODE_RDSR                0x05
>>  #define SPI_OPCODE_WREN                0x06
>>  #define SPI_OPCODE_FAST_READ   0x0b
>> +#define SPI_OPCODE_ERASE_SECT  0x20
>> +#define SPI_OPCODE_READ_ID     0x9f
>> +#define SPI_OPCODE_ERASE_BLOCK 0xd8
>
> Wonder why the flash part should be part of SPI, can't we use existing
> spi_flash through command interface if there is specific stuff like
> this?

This patch only changes some defines here and passes some allowed
opcodes via some configuration registers to the Linux driver.

I didn't look closely into this U-Boot driver and how it interacts
with the SPI NOR. Simon is most likely the best person to answer
on your questions regarding the usage of spi_flash. Simon could
you please answer Jagan's questions?

Thanks,
Stefan

[U-Boot] [PATCH 5/5 v2] spi: ich: Configure SPI BIOS parameters for Linux upon U-Boot exit

[Bin Meng]

On Tue, May 9, 2017 at 7:20 PM, Stefan Roese <sr@denx.de> wrote:
> (Added Simon to Cc)

Really added Simon :-)

>
>
> On 09.05.2017 13:14, Jagan Teki wrote:
>>
>> On Mon, Apr 24, 2017 at 1:18 PM, Stefan Roese <sr@denx.de> wrote:
>>>
>>> This patch adds a remove function to the Intel ICH SPI driver, that will
>>> be called upon U-Boot exit, directly before the OS (Linux) is started.
>>> This function takes care of configuring the BIOS registers in the SPI
>>> controller (similar to what a "standard" BIOS or coreboot does), so that
>>> the Linux MTD device driver is able to correctly read/write to the SPI
>>> NOR chip. Without this, the chip is not detected at all.
>>>
>>> Signed-off-by: Stefan Roese <sr@denx.de>
>>> Reviewed-by: Simon Glass <sjg@chromium.org>
>>> Cc: Bin Meng <bmeng.cn@gmail.com>
>>> Cc: Jagan Teki <jteki@openedev.com>
>>> ---
>>> v2:
>>> - Added Simons RB line
>>>
>>>  drivers/spi/ich.c | 18 ++++++++++++++++++
>>>  drivers/spi/ich.h | 54
>>> +++++++++++++++++++++++++++++++++++++++++++++++-------
>>>  2 files changed, 65 insertions(+), 7 deletions(-)
>>>
>>> diff --git a/drivers/spi/ich.c b/drivers/spi/ich.c
>>> index 893fe33b66..bf2e99b5cc 100644
>>> --- a/drivers/spi/ich.c
>>> +++ b/drivers/spi/ich.c
>>> @@ -617,6 +617,22 @@ static int ich_spi_probe(struct udevice *dev)
>>>         return 0;
>>>  }
>>>
>>> +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));
>>> +
>>> +       return 0;
>>> +}
>>> +
>>>  static int ich_spi_set_speed(struct udevice *bus, uint speed)
>>>  {
>>>         struct ich_spi_priv *priv = dev_get_priv(bus);
>>> @@ -700,4 +716,6 @@ U_BOOT_DRIVER(ich_spi) = {
>>>         .priv_auto_alloc_size = sizeof(struct ich_spi_priv),
>>>         .child_pre_probe = ich_spi_child_pre_probe,
>>>         .probe  = ich_spi_probe,
>>> +       .remove = ich_spi_remove,
>>> +       .flags  = DM_FLAG_OS_PREPARE,
>>>  };
>>> diff --git a/drivers/spi/ich.h b/drivers/spi/ich.h
>>> index bd0a820809..dcb8a9048f 100644
>>> --- a/drivers/spi/ich.h
>>> +++ b/drivers/spi/ich.h
>>> @@ -102,13 +102,6 @@ enum {
>>>  };
>>>
>>>  enum {
>>> -       SPI_OPCODE_TYPE_READ_NO_ADDRESS =       0,
>>> -       SPI_OPCODE_TYPE_WRITE_NO_ADDRESS =      1,
>>> -       SPI_OPCODE_TYPE_READ_WITH_ADDRESS =     2,
>>> -       SPI_OPCODE_TYPE_WRITE_WITH_ADDRESS =    3
>>> -};
>>> -
>>> -enum {
>>>         ICH_MAX_CMD_LEN         = 5,
>>>  };
>>>
>>> @@ -124,8 +117,55 @@ struct spi_trans {
>>>         uint32_t offset;
>>>  };
>>>
>>> +#define SPI_OPCODE_WRSR                0x01
>>> +#define SPI_OPCODE_PAGE_PROGRAM        0x02
>>> +#define SPI_OPCODE_READ                0x03
>>> +#define SPI_OPCODE_WRDIS       0x04
>>> +#define SPI_OPCODE_RDSR                0x05
>>>  #define SPI_OPCODE_WREN                0x06
>>>  #define SPI_OPCODE_FAST_READ   0x0b
>>> +#define SPI_OPCODE_ERASE_SECT  0x20
>>> +#define SPI_OPCODE_READ_ID     0x9f
>>> +#define SPI_OPCODE_ERASE_BLOCK 0xd8
>>
>>
>> Wonder why the flash part should be part of SPI, can't we use existing
>> spi_flash through command interface if there is specific stuff like
>> this?
>

These flash commands need to be programmed to SPI controller register,
by Intel's design, to make Linux MTD driver happy. Possibly we may do
like this, like get such value dynamically from spi_flash to program
this to SPI controller? I once asked a question about this: if some
other flash part that does not have the exact same command set as what
was programmed to the SPI controller, will Linux MTD driver still
work?

>
> This patch only changes some defines here and passes some allowed
> opcodes via some configuration registers to the Linux driver.
>
> I didn't look closely into this U-Boot driver and how it interacts
> with the SPI NOR. Simon is most likely the best person to answer
> on your questions regarding the usage of spi_flash. Simon could
> you please answer Jagan's questions?

Regards,
Bin

[U-Boot] [PATCH 5/5 v2] spi: ich: Configure SPI BIOS parameters for Linux upon U-Boot exit

[Stefan Roese]

Hi Bin,

On 10.05.2017 02:47, Bin Meng wrote:
> On Tue, May 9, 2017 at 7:20 PM, Stefan Roese <sr@denx.de> wrote:
>> (Added Simon to Cc)
>
> Really added Simon :-)

I did in my mail as well. Sometimes the ML removes recipients from
the list as it seems.

>>
>>
>> On 09.05.2017 13:14, Jagan Teki wrote:
>>>
>>> On Mon, Apr 24, 2017 at 1:18 PM, Stefan Roese <sr@denx.de> wrote:
>>>>
>>>> This patch adds a remove function to the Intel ICH SPI driver, that will
>>>> be called upon U-Boot exit, directly before the OS (Linux) is started.
>>>> This function takes care of configuring the BIOS registers in the SPI
>>>> controller (similar to what a "standard" BIOS or coreboot does), so that
>>>> the Linux MTD device driver is able to correctly read/write to the SPI
>>>> NOR chip. Without this, the chip is not detected at all.
>>>>
>>>> Signed-off-by: Stefan Roese <sr@denx.de>
>>>> Reviewed-by: Simon Glass <sjg@chromium.org>
>>>> Cc: Bin Meng <bmeng.cn@gmail.com>
>>>> Cc: Jagan Teki <jteki@openedev.com>
>>>> ---
>>>> v2:
>>>> - Added Simons RB line
>>>>
>>>>  drivers/spi/ich.c | 18 ++++++++++++++++++
>>>>  drivers/spi/ich.h | 54
>>>> +++++++++++++++++++++++++++++++++++++++++++++++-------
>>>>  2 files changed, 65 insertions(+), 7 deletions(-)
>>>>
>>>> diff --git a/drivers/spi/ich.c b/drivers/spi/ich.c
>>>> index 893fe33b66..bf2e99b5cc 100644
>>>> --- a/drivers/spi/ich.c
>>>> +++ b/drivers/spi/ich.c
>>>> @@ -617,6 +617,22 @@ static int ich_spi_probe(struct udevice *dev)
>>>>         return 0;
>>>>  }
>>>>
>>>> +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));
>>>> +
>>>> +       return 0;
>>>> +}
>>>> +
>>>>  static int ich_spi_set_speed(struct udevice *bus, uint speed)
>>>>  {
>>>>         struct ich_spi_priv *priv = dev_get_priv(bus);
>>>> @@ -700,4 +716,6 @@ U_BOOT_DRIVER(ich_spi) = {
>>>>         .priv_auto_alloc_size = sizeof(struct ich_spi_priv),
>>>>         .child_pre_probe = ich_spi_child_pre_probe,
>>>>         .probe  = ich_spi_probe,
>>>> +       .remove = ich_spi_remove,
>>>> +       .flags  = DM_FLAG_OS_PREPARE,
>>>>  };
>>>> diff --git a/drivers/spi/ich.h b/drivers/spi/ich.h
>>>> index bd0a820809..dcb8a9048f 100644
>>>> --- a/drivers/spi/ich.h
>>>> +++ b/drivers/spi/ich.h
>>>> @@ -102,13 +102,6 @@ enum {
>>>>  };
>>>>
>>>>  enum {
>>>> -       SPI_OPCODE_TYPE_READ_NO_ADDRESS =       0,
>>>> -       SPI_OPCODE_TYPE_WRITE_NO_ADDRESS =      1,
>>>> -       SPI_OPCODE_TYPE_READ_WITH_ADDRESS =     2,
>>>> -       SPI_OPCODE_TYPE_WRITE_WITH_ADDRESS =    3
>>>> -};
>>>> -
>>>> -enum {
>>>>         ICH_MAX_CMD_LEN         = 5,
>>>>  };
>>>>
>>>> @@ -124,8 +117,55 @@ struct spi_trans {
>>>>         uint32_t offset;
>>>>  };
>>>>
>>>> +#define SPI_OPCODE_WRSR                0x01
>>>> +#define SPI_OPCODE_PAGE_PROGRAM        0x02
>>>> +#define SPI_OPCODE_READ                0x03
>>>> +#define SPI_OPCODE_WRDIS       0x04
>>>> +#define SPI_OPCODE_RDSR                0x05
>>>>  #define SPI_OPCODE_WREN                0x06
>>>>  #define SPI_OPCODE_FAST_READ   0x0b
>>>> +#define SPI_OPCODE_ERASE_SECT  0x20
>>>> +#define SPI_OPCODE_READ_ID     0x9f
>>>> +#define SPI_OPCODE_ERASE_BLOCK 0xd8
>>>
>>>
>>> Wonder why the flash part should be part of SPI, can't we use existing
>>> spi_flash through command interface if there is specific stuff like
>>> this?
>>
>
> These flash commands need to be programmed to SPI controller register,
> by Intel's design, to make Linux MTD driver happy. Possibly we may do
> like this, like get such value dynamically from spi_flash to program
> this to SPI controller? I once asked a question about this: if some
> other flash part that does not have the exact same command set as what
> was programmed to the SPI controller, will Linux MTD driver still
> work?

Yes, we discussed this a few months before already. I've not seen
any other values used in coreboot - so these values seem to be
quite "safe" for now. Once we have such an issue with incompatible
SPI NOR chips on x86, we will find a way to solve this in a board
specific way. But for now, its very helpful to have access from the
Linux MTD subsystem to the NOR chips by using these patches.

Thanks,
Stefan

[U-Boot] [PATCH 5/5 v2] spi: ich: Configure SPI BIOS parameters for Linux upon U-Boot exit

[Simon Glass]

Hi Jagan,

On 9 May 2017 at 18:47, Bin Meng <bmeng.cn@gmail.com> wrote:
> On Tue, May 9, 2017 at 7:20 PM, Stefan Roese <sr@denx.de> wrote:
>> (Added Simon to Cc)
>
> Really added Simon :-)
>
>>
>>
>> On 09.05.2017 13:14, Jagan Teki wrote:
>>>
>>> On Mon, Apr 24, 2017 at 1:18 PM, Stefan Roese <sr@denx.de> wrote:
>>>>
>>>> This patch adds a remove function to the Intel ICH SPI driver, that will
>>>> be called upon U-Boot exit, directly before the OS (Linux) is started.
>>>> This function takes care of configuring the BIOS registers in the SPI
>>>> controller (similar to what a "standard" BIOS or coreboot does), so that
>>>> the Linux MTD device driver is able to correctly read/write to the SPI
>>>> NOR chip. Without this, the chip is not detected at all.
>>>>
>>>> Signed-off-by: Stefan Roese <sr@denx.de>
>>>> Reviewed-by: Simon Glass <sjg@chromium.org>
>>>> Cc: Bin Meng <bmeng.cn@gmail.com>
>>>> Cc: Jagan Teki <jteki@openedev.com>
>>>> ---
>>>> v2:
>>>> - Added Simons RB line
>>>>
>>>>  drivers/spi/ich.c | 18 ++++++++++++++++++
>>>>  drivers/spi/ich.h | 54
>>>> +++++++++++++++++++++++++++++++++++++++++++++++-------
>>>>  2 files changed, 65 insertions(+), 7 deletions(-)
>>>>
>>>> diff --git a/drivers/spi/ich.c b/drivers/spi/ich.c
>>>> index 893fe33b66..bf2e99b5cc 100644
>>>> --- a/drivers/spi/ich.c
>>>> +++ b/drivers/spi/ich.c
>>>> @@ -617,6 +617,22 @@ static int ich_spi_probe(struct udevice *dev)
>>>>         return 0;
>>>>  }
>>>>
>>>> +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));
>>>> +
>>>> +       return 0;
>>>> +}
>>>> +
>>>>  static int ich_spi_set_speed(struct udevice *bus, uint speed)
>>>>  {
>>>>         struct ich_spi_priv *priv = dev_get_priv(bus);
>>>> @@ -700,4 +716,6 @@ U_BOOT_DRIVER(ich_spi) = {
>>>>         .priv_auto_alloc_size = sizeof(struct ich_spi_priv),
>>>>         .child_pre_probe = ich_spi_child_pre_probe,
>>>>         .probe  = ich_spi_probe,
>>>> +       .remove = ich_spi_remove,
>>>> +       .flags  = DM_FLAG_OS_PREPARE,
>>>>  };
>>>> diff --git a/drivers/spi/ich.h b/drivers/spi/ich.h
>>>> index bd0a820809..dcb8a9048f 100644
>>>> --- a/drivers/spi/ich.h
>>>> +++ b/drivers/spi/ich.h
>>>> @@ -102,13 +102,6 @@ enum {
>>>>  };
>>>>
>>>>  enum {
>>>> -       SPI_OPCODE_TYPE_READ_NO_ADDRESS =       0,
>>>> -       SPI_OPCODE_TYPE_WRITE_NO_ADDRESS =      1,
>>>> -       SPI_OPCODE_TYPE_READ_WITH_ADDRESS =     2,
>>>> -       SPI_OPCODE_TYPE_WRITE_WITH_ADDRESS =    3
>>>> -};
>>>> -
>>>> -enum {
>>>>         ICH_MAX_CMD_LEN         = 5,
>>>>  };
>>>>
>>>> @@ -124,8 +117,55 @@ struct spi_trans {
>>>>         uint32_t offset;
>>>>  };
>>>>
>>>> +#define SPI_OPCODE_WRSR                0x01
>>>> +#define SPI_OPCODE_PAGE_PROGRAM        0x02
>>>> +#define SPI_OPCODE_READ                0x03
>>>> +#define SPI_OPCODE_WRDIS       0x04
>>>> +#define SPI_OPCODE_RDSR                0x05
>>>>  #define SPI_OPCODE_WREN                0x06
>>>>  #define SPI_OPCODE_FAST_READ   0x0b
>>>> +#define SPI_OPCODE_ERASE_SECT  0x20
>>>> +#define SPI_OPCODE_READ_ID     0x9f
>>>> +#define SPI_OPCODE_ERASE_BLOCK 0xd8
>>>
>>>
>>> Wonder why the flash part should be part of SPI, can't we use existing
>>> spi_flash through command interface if there is specific stuff like
>>> this?

This driver is odd in that it tries to decode low-level SPI requests
coming in via spi_xfer() and issue high-level (SPI flash) commands to
the controller. The controller does not actually support generic SPI
operation.

>>
>
> These flash commands need to be programmed to SPI controller register,
> by Intel's design, to make Linux MTD driver happy. Possibly we may do
> like this, like get such value dynamically from spi_flash to program
> this to SPI controller? I once asked a question about this: if some
> other flash part that does not have the exact same command set as what
> was programmed to the SPI controller, will Linux MTD driver still
> work?
>
>>
>> This patch only changes some defines here and passes some allowed
>> opcodes via some configuration registers to the Linux driver.
>>
>> I didn't look closely into this U-Boot driver and how it interacts
>> with the SPI NOR. Simon is most likely the best person to answer
>> on your questions regarding the usage of spi_flash. Simon could
>> you please answer Jagan's questions?
>
> Regards,
> Bin

Regards,
Simon