[U-Boot] [PATCH 1/4] dm: device_remove: Don't return in device_chld_remove() upon error
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; }
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, };
/**
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
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
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)
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
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,
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!
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
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
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
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
participants (3)
-
Jagan Teki -
Simon Glass -
Stefan Roese