[U-Boot] [PATCH 0/5] Fix Falcon Boot from internal eMMC on RK3288 Vyasa
This patchest fixes booting Linux Kernel in falcon mode for Vyasa board. What happen is that CPU hangs on SPL while loading kernel from internal eMMC.
This issue has been already addressed here [1] but with a wrong approach: it is a clear case of memory corruption and first patch of this serie helps a lot in profiling the same issue in the future.
[1]: https://patchwork.ozlabs.org/patch/833821/
Alberto Panizzo (5): mmc: dw_mmc: prevent silent memory corruption when stack and heap are too small mmc: dw_mmc: increase cmd timeout to fix eMMC enumeration error rockchip: rk3288-vyasa: increase heap space after relocation rk3288: vyasa: Allow booting from internal eMMC rk3288: vyasa: Fixup indentation
board/amarula/vyasa-rk3288/vyasa-rk3288.c | 8 +++---- configs/vyasa-rk3288_defconfig | 8 ++++--- drivers/mmc/dw_mmc.c | 35 +++++++++++++++++++++++-------- include/configs/vyasa-rk3288.h | 1 + 4 files changed, 36 insertions(+), 16 deletions(-)
ALLOC_CACHE_ALIGN_BUFFER was called here in a way to alloc in stack a possible huge quantity of memory depending on data transer size.
Es: loading kernel 8MB from eMMC we have Transfer size: 0x800000 Block size: 0x200 Transfer blocks: 0x4000 struct size: 0x10 Stack allocation: ((0x200 / 8) + 1) * 0x10 = 0x8010 (~32KB)
Since this allocation is done on stack, there is no current way to get an error on stack memory limit exceeded, overlapping heap space on environments with very strict stack + heap limits like TPL or SPL (where malloc size can be 16KB). Results are silent corruptions of heap on mmc transfer and random errors or CPU hang.
Using malloc_cache_aligned() we will alloc slightly bigger buffers but we do have evidence about memory allocation failure allowing developer to recognize the issue and take actions.
Signed-off-by: Alberto Panizzo alberto@amarulasolutions.com --- drivers/mmc/dw_mmc.c | 33 +++++++++++++++++++++++++-------- 1 file changed, 25 insertions(+), 8 deletions(-)
diff --git a/drivers/mmc/dw_mmc.c b/drivers/mmc/dw_mmc.c index 13180fc..0126563 100644 --- a/drivers/mmc/dw_mmc.c +++ b/drivers/mmc/dw_mmc.c @@ -194,8 +194,9 @@ static int dwmci_send_cmd(struct mmc *mmc, struct mmc_cmd *cmd, { #endif struct dwmci_host *host = mmc->priv; - ALLOC_CACHE_ALIGN_BUFFER(struct dwmci_idmac, cur_idmac, - data ? DIV_ROUND_UP(data->blocks, 8) : 0); + struct dwmci_idmac *cur_idmac = + malloc_cache_aligned(sizeof(struct dwmci_idmac) * + (1 + (data ? DIV_ROUND_UP(data->blocks, 8) : 0))); int ret = 0, flags = 0, i; unsigned int timeout = 500; u32 retry = 100000; @@ -203,10 +204,18 @@ static int dwmci_send_cmd(struct mmc *mmc, struct mmc_cmd *cmd, ulong start = get_timer(0); struct bounce_buffer bbstate;
+ if (!cur_idmac) { + debug("%s: Cannot allocate 0x%x bytes\n", __func__, + sizeof(struct dwmci_idmac) * + (1 + (data ? DIV_ROUND_UP(data->blocks, 8) : 0))); + return -ENOMEM; + } + while (dwmci_readl(host, DWMCI_STATUS) & DWMCI_BUSY) { if (get_timer(start) > timeout) { debug("%s: Timeout on data busy\n", __func__); - return -ETIMEDOUT; + ret = -ETIMEDOUT; + goto free_idmac; } }
@@ -238,8 +247,10 @@ static int dwmci_send_cmd(struct mmc *mmc, struct mmc_cmd *cmd, if (data) flags = dwmci_set_transfer_mode(host, data);
- if ((cmd->resp_type & MMC_RSP_136) && (cmd->resp_type & MMC_RSP_BUSY)) - return -1; + if ((cmd->resp_type & MMC_RSP_136) && (cmd->resp_type & MMC_RSP_BUSY)) { + ret = -EIO; + goto free_idmac; + }
if (cmd->cmdidx == MMC_CMD_STOP_TRANSMISSION) flags |= DWMCI_CMD_ABORT_STOP; @@ -272,7 +283,8 @@ static int dwmci_send_cmd(struct mmc *mmc, struct mmc_cmd *cmd,
if (i == retry) { debug("%s: Timeout.\n", __func__); - return -ETIMEDOUT; + ret = -ETIMEDOUT; + goto free_idmac; }
if (mask & DWMCI_INTMSK_RTO) { @@ -285,10 +297,12 @@ static int dwmci_send_cmd(struct mmc *mmc, struct mmc_cmd *cmd, * CMD8, please keep that in mind. */ debug("%s: Response Timeout.\n", __func__); - return -ETIMEDOUT; + ret = -ETIMEDOUT; + goto free_idmac; } else if (mask & DWMCI_INTMSK_RE) { debug("%s: Response Error.\n", __func__); - return -EIO; + ret = -EIO; + goto free_idmac; }
@@ -317,6 +331,9 @@ static int dwmci_send_cmd(struct mmc *mmc, struct mmc_cmd *cmd,
udelay(100);
+free_idmac: + free(cur_idmac); + return ret; }
ALLOC_CACHE_ALIGN_BUFFER was called here in a way to alloc in stack a possible huge quantity of memory depending on data transer size.
Es: loading kernel 8MB from eMMC we have Transfer size: 0x800000 Block size: 0x200 Transfer blocks: 0x4000 struct size: 0x10 Stack allocation: ((0x200 / 8) + 1) * 0x10 = 0x8010 (~32KB)
Since this allocation is done on stack, there is no current way to get an error on stack memory limit exceeded, overlapping heap space on environments with very strict stack + heap limits like TPL or SPL (where malloc size can be 16KB). Results are silent corruptions of heap on mmc transfer and random errors or CPU hang.
Using malloc_cache_aligned() we will alloc slightly bigger buffers but we do have evidence about memory allocation failure allowing developer to recognize the issue and take actions.
Signed-off-by: Alberto Panizzo alberto@amarulasolutions.com
drivers/mmc/dw_mmc.c | 33 +++++++++++++++++++++++++-------- 1 file changed, 25 insertions(+), 8 deletions(-)
Reviewed-by: Philipp Tomsich philipp.tomsich@theobroma-systems.com
Errors are reported to happen running U-Boot after SPL kernel load failure. In this case mmc host is not clean, and card enumeration timeouts do happen frequently.
Signed-off-by: Alberto Panizzo alberto@amarulasolutions.com --- drivers/mmc/dw_mmc.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/mmc/dw_mmc.c b/drivers/mmc/dw_mmc.c index 0126563..b6890d3 100644 --- a/drivers/mmc/dw_mmc.c +++ b/drivers/mmc/dw_mmc.c @@ -199,7 +199,7 @@ static int dwmci_send_cmd(struct mmc *mmc, struct mmc_cmd *cmd, (1 + (data ? DIV_ROUND_UP(data->blocks, 8) : 0))); int ret = 0, flags = 0, i; unsigned int timeout = 500; - u32 retry = 100000; + u32 retry = 1000000; u32 mask, ctrl; ulong start = get_timer(0); struct bounce_buffer bbstate;
On Wed, 4 Jul 2018, Alberto Panizzo wrote:
Errors are reported to happen running U-Boot after SPL kernel load failure. In this case mmc host is not clean, and card enumeration timeouts do happen frequently.
Signed-off-by: Alberto Panizzo alberto@amarulasolutions.com
See below for requested changes.
drivers/mmc/dw_mmc.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/mmc/dw_mmc.c b/drivers/mmc/dw_mmc.c index 0126563..b6890d3 100644 --- a/drivers/mmc/dw_mmc.c +++ b/drivers/mmc/dw_mmc.c @@ -199,7 +199,7 @@ static int dwmci_send_cmd(struct mmc *mmc, struct mmc_cmd *cmd, (1 + (data ? DIV_ROUND_UP(data->blocks, 8) : 0))); int ret = 0, flags = 0, i; unsigned int timeout = 500;
- u32 retry = 100000;
- u32 retry = 1000000;
I feel rather uneasy about this, especially as it affects all devices (not just limited to Rockchip) that include a Designware MMC controller.
This affects code that effectivly 'retries' reading the status register to wait for a DONE bit: for (i = 0; i < retry; i++) { status = readl(...); if (status & BITMASK) { writel(...); break; // done } }
if (i == retry) // a timeout happened
There's two main issues I have: (a) the number of retries is an implicit timeout (i.e. depending on the CPU frequency, instruction-set, whether the i-cache is enabled, ...) a different number of microseconds will expire per pass through the loop (b) you commit message states (I am paraphrasing here) 'because the BootROM has left the state dirty', we need to increase the timeout
So there's two things (addressing these two issues) I'd like to see: (a) the use of a polling loop that uses a timer where possible (I don't know whether there's an execution context in someone's SPL or TPL that doesn't have a timebase running, though... so: caveat emptor) --- this might even lend itself to the use of include/linux/iopoll.h macros. (b) a clean reset of the MMC controller for your/our platform in case we expect the BootROM having left things in disarray.
u32 mask, ctrl; ulong start = get_timer(0); struct bounce_buffer bbstate;
Before relocation (TPL or early SPL) we are executing in internal SDRAM with limited space. As soon as SPL is relocated increase malloc size, big enough to manage loading kernel images from eMMC
Signed-off-by: Alberto Panizzo alberto@amarulasolutions.com --- configs/vyasa-rk3288_defconfig | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-)
diff --git a/configs/vyasa-rk3288_defconfig b/configs/vyasa-rk3288_defconfig index e721d07..ed5b7c5 100644 --- a/configs/vyasa-rk3288_defconfig +++ b/configs/vyasa-rk3288_defconfig @@ -3,19 +3,21 @@ CONFIG_ARM=y # CONFIG_SPL_USE_ARCH_MEMSET is not set CONFIG_ARCH_ROCKCHIP=y CONFIG_SYS_TEXT_BASE=0x00100000 -CONFIG_SYS_MALLOC_F_LEN=0x2000 +CONFIG_SYS_MALLOC_F_LEN=0x4000 +CONFIG_TPL_SYS_MALLOC_F_LEN=0x4000 +CONFIG_SPL_SYS_MALLOC_F_LEN=0x4000 CONFIG_ROCKCHIP_RK3288=y CONFIG_TARGET_VYASA_RK3288=y CONFIG_DEBUG_UART_BASE=0xff690000 CONFIG_DEBUG_UART_CLOCK=24000000 -CONFIG_SPL_STACK_R_ADDR=0x80000 +CONFIG_SPL_STACK_R_ADDR=0x180000 CONFIG_DEFAULT_DEVICE_TREE="rk3288-vyasa" CONFIG_DEBUG_UART=y CONFIG_SILENT_CONSOLE=y # CONFIG_DISPLAY_CPUINFO is not set CONFIG_DISPLAY_BOARDINFO_LATE=y CONFIG_SPL_STACK_R=y -CONFIG_SPL_STACK_R_MALLOC_SIMPLE_LEN=0x2000 +CONFIG_SPL_STACK_R_MALLOC_SIMPLE_LEN=0x100000 CONFIG_CMD_GPIO=y CONFIG_CMD_GPT=y CONFIG_CMD_I2C=y
Before relocation (TPL or early SPL) we are executing in internal SDRAM with limited space. As soon as SPL is relocated increase malloc size, big enough to manage loading kernel images from eMMC
Signed-off-by: Alberto Panizzo alberto@amarulasolutions.com
configs/vyasa-rk3288_defconfig | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-)
Reviewed-by: Philipp Tomsich philipp.tomsich@theobroma-systems.com
Keeping SD-Card as priority for easy board recovery
Signed-off-by: Alberto Panizzo alberto@amarulasolutions.com --- include/configs/vyasa-rk3288.h | 1 + 1 file changed, 1 insertion(+)
diff --git a/include/configs/vyasa-rk3288.h b/include/configs/vyasa-rk3288.h index 382fdac..4114bf0 100644 --- a/include/configs/vyasa-rk3288.h +++ b/include/configs/vyasa-rk3288.h @@ -15,6 +15,7 @@
#define BOOT_TARGET_DEVICES(func) \ func(MMC, mmc, 1) \ + func(MMC, mmc, 0)
#define CONFIG_SYS_MMC_ENV_DEV 1 #undef CONFIG_CMD_USB_MASS_STORAGE
On Thu, Jul 5, 2018 at 1:11 AM, Alberto Panizzo alberto@amarulasolutions.com wrote:
Keeping SD-Card as priority for easy board recovery
Signed-off-by: Alberto Panizzo alberto@amarulasolutions.com
Reviewed-by: Jagan Teki jagan@amarulasolutions.com
include/configs/vyasa-rk3288.h | 1 + 1 file changed, 1 insertion(+)
diff --git a/include/configs/vyasa-rk3288.h b/include/configs/vyasa-rk3288.h index 382fdac..4114bf0 100644 --- a/include/configs/vyasa-rk3288.h +++ b/include/configs/vyasa-rk3288.h @@ -15,6 +15,7 @@
#define BOOT_TARGET_DEVICES(func) \ func(MMC, mmc, 1) \
func(MMC, mmc, 0)#define CONFIG_SYS_MMC_ENV_DEV 1
I think we even need to take care env device wrt boot mode.
Keeping SD-Card as priority for easy board recovery
Signed-off-by: Alberto Panizzo alberto@amarulasolutions.com Reviewed-by: Jagan Teki jagan@amarulasolutions.com
include/configs/vyasa-rk3288.h | 1 + 1 file changed, 1 insertion(+)
Reviewed-by: Philipp Tomsich philipp.tomsich@theobroma-systems.com
Indent file using tabs
Signed-off-by: Alberto Panizzo alberto@amarulasolutions.com --- board/amarula/vyasa-rk3288/vyasa-rk3288.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/board/amarula/vyasa-rk3288/vyasa-rk3288.c b/board/amarula/vyasa-rk3288/vyasa-rk3288.c index 2b509f5..4367ed2 100644 --- a/board/amarula/vyasa-rk3288/vyasa-rk3288.c +++ b/board/amarula/vyasa-rk3288/vyasa-rk3288.c @@ -17,10 +17,10 @@ void board_boot_order(u32 *spl_boot_list)
int spl_start_uboot(void) { - /* break into full u-boot on 'c' */ - if (serial_tstc() && serial_getc() == 'c') - return 1; + /* break into full u-boot on 'c' */ + if (serial_tstc() && serial_getc() == 'c') + return 1;
- return 0; + return 0; } #endif
On Thu, Jul 5, 2018 at 1:11 AM, Alberto Panizzo alberto@amarulasolutions.com wrote:
Indent file using tabs
Signed-off-by: Alberto Panizzo alberto@amarulasolutions.com
Reviewed-by: Jagan Teki jagan@amarulasolutions.com
Indent file using tabs
Signed-off-by: Alberto Panizzo alberto@amarulasolutions.com
board/amarula/vyasa-rk3288/vyasa-rk3288.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-)
Reviewed-by: Philipp Tomsich philipp.tomsich@theobroma-systems.com
This patchest fixes booting Linux Kernel in falcon mode for Vyasa board. What happen is that CPU hangs on SPL while loading kernel from internal eMMC.
This issue has been already addressed here [1] but with a wrong approach: it is a clear case of memory corruption and first patch of this serie helps a lot in profiling the same issue in the future.
Alberto Panizzo (5): mmc: dw_mmc: prevent silent memory corruption when stack and heap are too small mmc: dw_mmc: increase cmd timeout to fix eMMC enumeration error rockchip: rk3288-vyasa: increase heap space after relocation rk3288: vyasa: Allow booting from internal eMMC rk3288: vyasa: Fixup indentation
Tested-by: Shyam Saini shyam@amarulasolutions.com
participants (4)
-
Alberto Panizzo -
Jagan Teki -
Philipp Tomsich -
Shyam Saini