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

[U-Boot] [PATCH 1/4] 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>
---
 drivers/core/device-remove.c | 8 ++------
 1 file changed, 2 insertions(+), 6 deletions(-)

diff --git a/drivers/core/device-remove.c b/drivers/core/device-remove.c
index cc0043b990..8b46f3343e 100644
--- a/drivers/core/device-remove.c
+++ b/drivers/core/device-remove.c
@@ -52,15 +52,11 @@ static int device_chld_unbind(struct udevice *dev)
 static int device_chld_remove(struct udevice *dev, uint flags)
 {
 	struct udevice *pos, *n;
-	int ret;
 
 	assert(dev);
 
-	list_for_each_entry_safe(pos, n, &dev->child_head, sibling_node) {
-		ret = device_remove(pos, flags);
-		if (ret)
-			return ret;
-	}
+	list_for_each_entry_safe(pos, n, &dev->child_head, sibling_node)
+		device_remove(pos, flags);
 
 	return 0;
 }
-- 
2.12.2

[U-Boot] [PATCH 2/4] dm: core: Add DM_FLAG_PRE_OS_FINALIZE 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>
Cc: Simon Glass <sjg@chromium.org>
Cc: Bin Meng <bmeng.cn@gmail.com>
---
 drivers/core/device-remove.c | 17 ++++++++++++-----
 include/dm/device.h          | 11 ++++++++++-
 2 files changed, 22 insertions(+), 6 deletions(-)

diff --git a/drivers/core/device-remove.c b/drivers/core/device-remove.c
index 8b46f3343e..390be5a0d8 100644
--- a/drivers/core/device-remove.c
+++ b/drivers/core/device-remove.c
@@ -148,6 +148,16 @@ void device_free(struct udevice *dev)
 	devres_release_probe(dev);
 }
 
+static int flags_remove(uint flags, uint drv_flags)
+{
+	if ((flags & DM_REMOVE_NORMAL) ||
+	    (flags & (drv_flags &
+		      (DM_FLAG_ACTIVE_DMA | DM_FLAG_PRE_OS_FINALIZE))))
+		return 1;
+
+	return 0;
+}
+
 int device_remove(struct udevice *dev, uint flags)
 {
 	const struct driver *drv;
@@ -174,9 +184,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;
@@ -190,8 +198,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..c49b8e688e 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_PRE_OS_FINALIZE		(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 pre-OS finalization */
+	DM_REMOVE_PRE_OS_FINALIZE = DM_FLAG_PRE_OS_FINALIZE,
+
 	/* 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_PRE_OS_FINALIZE,
 };
 
 /**
-- 
2.12.2

[U-Boot] [PATCH 3/4] 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>
Cc: Simon Glass <sjg@chromium.org>
---
 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 4/4] 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>
Cc: Bin Meng <bmeng.cn@gmail.com>
Cc: Simon Glass <sjg@chromium.org>
Cc: Jagan Teki <jteki@openedev.com>
---
 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..60f40c5cf5 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_PRE_OS_FINALIZE,
 };
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 4/4] spi: ich: Configure SPI BIOS parameters for Linux upon U-Boot exit

[Jagan Teki]

On Thu, Apr 6, 2017 at 6:59 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>
> Cc: Bin Meng <bmeng.cn@gmail.com>
> Cc: Simon Glass <sjg@chromium.org>
> Cc: Jagan Teki <jteki@openedev.com>
> ---
>  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..60f40c5cf5 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_PRE_OS_FINALIZE,
>  };
> 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))

These look flash opcodes, shouldn't be part of spi

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

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

[Simon Glass]

Hi Stefan,

On 6 April 2017 at 07:29, 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.

I think the right solution is to find out why stdio_deregister_dev()
fails. It is probably because the device is in use within the stdio
variables. Perhaps you need to remove it first?

>
> Signed-off-by: Stefan Roese <sr@denx.de>
> Cc: Simon Glass <sjg@chromium.org>
> Cc: Bin Meng <bmeng.cn@gmail.com>
> ---
>  drivers/core/device-remove.c | 8 ++------
>  1 file changed, 2 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/core/device-remove.c b/drivers/core/device-remove.c
> index cc0043b990..8b46f3343e 100644
> --- a/drivers/core/device-remove.c
> +++ b/drivers/core/device-remove.c
> @@ -52,15 +52,11 @@ static int device_chld_unbind(struct udevice *dev)
>  static int device_chld_remove(struct udevice *dev, uint flags)
>  {
>         struct udevice *pos, *n;
> -       int ret;
>
>         assert(dev);
>
> -       list_for_each_entry_safe(pos, n, &dev->child_head, sibling_node) {
> -               ret = device_remove(pos, flags);
> -               if (ret)
> -                       return ret;
> -       }
> +       list_for_each_entry_safe(pos, n, &dev->child_head, sibling_node)
> +               device_remove(pos, flags);

I think we should keep the error checking here.

>
>         return 0;
>  }
> --
> 2.12.2
>

Regards,
Simon

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

[Simon Glass]

On 6 April 2017 at 07:29, 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>
> Cc: Simon Glass <sjg@chromium.org>
> ---
>  arch/x86/lib/bootm.c | 9 +++++++++
>  1 file changed, 9 insertions(+)

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

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

[Simon Glass]

On 6 April 2017 at 07:29, 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>
> Cc: Bin Meng <bmeng.cn@gmail.com>
> Cc: Simon Glass <sjg@chromium.org>
> Cc: Jagan Teki <jteki@openedev.com>
> ---
>  drivers/spi/ich.c | 18 ++++++++++++++++++
>  drivers/spi/ich.h | 54 +++++++++++++++++++++++++++++++++++++++++++++++-------
>  2 files changed, 65 insertions(+), 7 deletions(-)

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

[U-Boot] [PATCH 2/4] dm: core: Add DM_FLAG_PRE_OS_FINALIZE flag

[Simon Glass]

Hi Stefan,

On 6 April 2017 at 07:29, 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>
> Cc: Simon Glass <sjg@chromium.org>
> Cc: Bin Meng <bmeng.cn@gmail.com>
> ---
>  drivers/core/device-remove.c | 17 ++++++++++++-----
>  include/dm/device.h          | 11 ++++++++++-
>  2 files changed, 22 insertions(+), 6 deletions(-)

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

You could perhaps have a separate patch to move the code into
flags_remove(), but I suppose it isn't important.

nit below.

>
> diff --git a/drivers/core/device-remove.c b/drivers/core/device-remove.c
> index 8b46f3343e..390be5a0d8 100644
> --- a/drivers/core/device-remove.c
> +++ b/drivers/core/device-remove.c
> @@ -148,6 +148,16 @@ void device_free(struct udevice *dev)
>         devres_release_probe(dev);
>  }
>
> +static int flags_remove(uint flags, uint drv_flags)

Can this be bool, and return true/false?

> +{
> +       if ((flags & DM_REMOVE_NORMAL) ||
> +           (flags & (drv_flags &
> +                     (DM_FLAG_ACTIVE_DMA | DM_FLAG_PRE_OS_FINALIZE))))

What do you think about OS_PREPARE instead? It doesn't really finalize the OS...

> +               return 1;
> +
> +       return 0;
> +}
> +
>  int device_remove(struct udevice *dev, uint flags)
>  {
>         const struct driver *drv;
> @@ -174,9 +184,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;
> @@ -190,8 +198,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..c49b8e688e 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_PRE_OS_FINALIZE                (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 pre-OS finalization */
> +       DM_REMOVE_PRE_OS_FINALIZE = DM_FLAG_PRE_OS_FINALIZE,
> +
>         /* 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_PRE_OS_FINALIZE,
>  };
>
>  /**
> --
> 2.12.2
>

Regards,
Simon

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

[Stefan Roese]

Hi Simon,

sorry for the late replay - just back from vacation....

On 09.04.2017 21:28, Simon Glass wrote:
> Hi Stefan,
>
> On 6 April 2017 at 07:29, 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.
>
> I think the right solution is to find out why stdio_deregister_dev()
> fails. It is probably because the device is in use within the stdio
> variables.

This is most likely the case, yes.

> Perhaps you need to remove it first?

Not sure if this should / could be done in this general case of
removal of all devices (or all devices matching a remove-flag)?
Please think of dm_uninit() being called. This functions should
have no internal knowledge of usage of devices. One thing I could
do, probably best in a separate patch, is to use the force flag
of stdio_deregister_dev() in serial_pre_remove() to force the
serial device(s) to be removed in this case. What do you think?

>>
>> Signed-off-by: Stefan Roese <sr@denx.de>
>> Cc: Simon Glass <sjg@chromium.org>
>> Cc: Bin Meng <bmeng.cn@gmail.com>
>> ---
>>  drivers/core/device-remove.c | 8 ++------
>>  1 file changed, 2 insertions(+), 6 deletions(-)
>>
>> diff --git a/drivers/core/device-remove.c b/drivers/core/device-remove.c
>> index cc0043b990..8b46f3343e 100644
>> --- a/drivers/core/device-remove.c
>> +++ b/drivers/core/device-remove.c
>> @@ -52,15 +52,11 @@ static int device_chld_unbind(struct udevice *dev)
>>  static int device_chld_remove(struct udevice *dev, uint flags)
>>  {
>>         struct udevice *pos, *n;
>> -       int ret;
>>
>>         assert(dev);
>>
>> -       list_for_each_entry_safe(pos, n, &dev->child_head, sibling_node) {
>> -               ret = device_remove(pos, flags);
>> -               if (ret)
>> -                       return ret;
>> -       }
>> +       list_for_each_entry_safe(pos, n, &dev->child_head, sibling_node)
>> +               device_remove(pos, flags);
>
> I think we should keep the error checking here.

The main fix with this patch is, that removing of child devices does
not stop once an error is encountered. Even if an error occurs with
one child-device of a parent, all other child-devices of this
parent should still be removed - or at least tried to.

So what does this error code buy us here? Should I print a log message
here in this error case?

Thanks,
Stefan

[U-Boot] [PATCH 2/4] dm: core: Add DM_FLAG_PRE_OS_FINALIZE flag

[Stefan Roese]

Hi Simon,

On 09.04.2017 21:28, Simon Glass wrote:
> Hi Stefan,
>
> On 6 April 2017 at 07:29, 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>
>> Cc: Simon Glass <sjg@chromium.org>
>> Cc: Bin Meng <bmeng.cn@gmail.com>
>> ---
>>  drivers/core/device-remove.c | 17 ++++++++++++-----
>>  include/dm/device.h          | 11 ++++++++++-
>>  2 files changed, 22 insertions(+), 6 deletions(-)
>
> Reviewed-by: Simon Glass <sjg@chromium.org>
>
> You could perhaps have a separate patch to move the code into
> flags_remove(), but I suppose it isn't important.
>
> nit below.
>
>>
>> diff --git a/drivers/core/device-remove.c b/drivers/core/device-remove.c
>> index 8b46f3343e..390be5a0d8 100644
>> --- a/drivers/core/device-remove.c
>> +++ b/drivers/core/device-remove.c
>> @@ -148,6 +148,16 @@ void device_free(struct udevice *dev)
>>         devres_release_probe(dev);
>>  }
>>
>> +static int flags_remove(uint flags, uint drv_flags)
>
> Can this be bool, and return true/false?

I'm not a big fan of bool but I've no hard feelings here. I'll change
this if you prefer it this way in v2.

>> +{
>> +       if ((flags & DM_REMOVE_NORMAL) ||
>> +           (flags & (drv_flags &
>> +                     (DM_FLAG_ACTIVE_DMA | DM_FLAG_PRE_OS_FINALIZE))))
>
> What do you think about OS_PREPARE instead? It doesn't really finalize
> the OS...

Much better, thanks. Will change in v2.

Thanks,
Stefan

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

[Simon Glass]

Hi Stefan,

On 19 April 2017 at 03:27, Stefan Roese <sr@denx.de> wrote:
> Hi Simon,
>
> sorry for the late replay - just back from vacation....
>
>
> On 09.04.2017 21:28, Simon Glass wrote:
>>
>> Hi Stefan,
>>
>> On 6 April 2017 at 07:29, 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.
>>
>>
>> I think the right solution is to find out why stdio_deregister_dev()
>> fails. It is probably because the device is in use within the stdio
>> variables.
>
>
> This is most likely the case, yes.
>
>> Perhaps you need to remove it first?
>
>
> Not sure if this should / could be done in this general case of
> removal of all devices (or all devices matching a remove-flag)?
> Please think of dm_uninit() being called. This functions should
> have no internal knowledge of usage of devices. One thing I could
> do, probably best in a separate patch, is to use the force flag
> of stdio_deregister_dev() in serial_pre_remove() to force the
> serial device(s) to be removed in this case. What do you think?

I think that sounds reasonable. I don't know this area very well though.

>
>>>
>>> Signed-off-by: Stefan Roese <sr@denx.de>
>>> Cc: Simon Glass <sjg@chromium.org>
>>> Cc: Bin Meng <bmeng.cn@gmail.com>
>>> ---
>>>  drivers/core/device-remove.c | 8 ++------
>>>  1 file changed, 2 insertions(+), 6 deletions(-)
>>>
>>> diff --git a/drivers/core/device-remove.c b/drivers/core/device-remove.c
>>> index cc0043b990..8b46f3343e 100644
>>> --- a/drivers/core/device-remove.c
>>> +++ b/drivers/core/device-remove.c
>>> @@ -52,15 +52,11 @@ static int device_chld_unbind(struct udevice *dev)
>>>  static int device_chld_remove(struct udevice *dev, uint flags)
>>>  {
>>>         struct udevice *pos, *n;
>>> -       int ret;
>>>
>>>         assert(dev);
>>>
>>> -       list_for_each_entry_safe(pos, n, &dev->child_head, sibling_node)
>>> {
>>> -               ret = device_remove(pos, flags);
>>> -               if (ret)
>>> -                       return ret;
>>> -       }
>>> +       list_for_each_entry_safe(pos, n, &dev->child_head, sibling_node)
>>> +               device_remove(pos, flags);
>>
>>
>> I think we should keep the error checking here.
>
>
> The main fix with this patch is, that removing of child devices does
> not stop once an error is encountered. Even if an error occurs with
> one child-device of a parent, all other child-devices of this
> parent should still be removed - or at least tried to.
>
> So what does this error code buy us here? Should I print a log message
> here in this error case?

Yes just a debug() will do. I see what you are saying, I'm just
nervous of dropping error checking since devices really should remove
cleanly. Hopefully as above you can fix the error?

Regards,
Simon