Re: [PATCH v4] ARM: rpi: parse useful data from vc fdt
On Fri, Jun 17, 2022 at 11:58:11PM +0200, Daniel Brát wrote:
> Videocore first-stage loader on rpi passes us many useful information
> inside the vc fdt, including the real value of PM_RSTS register, not
> easily available by other means and which we can use to determine
> the reset cause.
> Also make the relevant funtions just print error/warning and continue
> in case of some errors, since the fdt from vc is now optional for
> barebox's basic function.
>
> Signed-off-by: Daniel Brát
> ---
> arch/arm/boards/raspberry-pi/rpi-common.c | 183 +-
> drivers/watchdog/bcm2835_wdt.c| 23 +--
> include/soc/bcm283x/wdt.h | 40 +
> 3 files changed, 182 insertions(+), 64 deletions(-)
> create mode 100644 include/soc/bcm283x/wdt.h
Applied, thanks
Sascha
>
> diff --git a/arch/arm/boards/raspberry-pi/rpi-common.c
> b/arch/arm/boards/raspberry-pi/rpi-common.c
> index 12a4f4a0a..eaca4edef 100644
> --- a/arch/arm/boards/raspberry-pi/rpi-common.c
> +++ b/arch/arm/boards/raspberry-pi/rpi-common.c
> @@ -23,13 +23,28 @@
> #include
> #include
> #include
> +#include
>
> #include
> #include
> #include
>
> +#include
> +
> #include "lowlevel.h"
>
> +//https://www.raspberrypi.com/documentation/computers/raspberry-pi.html#BOOT_ORDER
> +static const char * const boot_mode_names[] = {
> + [0x0] = "unknown",
> + [0x1] = "sd",
> + [0x2] = "net",
> + [0x3] = "rpiboot",
> + [0x4] = "usbmsd",
> + [0x5] = "usbc",
> + [0x6] = "nvme",
> + [0x7] = "http",
> +};
> +
> struct rpi_priv;
> struct rpi_machine_data {
> int (*init)(struct rpi_priv *priv);
> @@ -186,54 +201,131 @@ static int rpi_env_init(void)
> return 0;
> }
>
> -/* Extract /chosen/bootargs from the VideoCore FDT into vc.bootargs
> - * global variable. */
> -static int rpi_vc_fdt_bootargs(void *fdt)
> +/* Some string properties in fdt passed to us from vc may be
> + * malformed by not being null terminated. In that case, create and
> + * return a null terminated copy.
> + */
> +static char *of_read_vc_string(struct device_node *node,
> + const char *prop_name,
> + bool *is_dyn)
> +{
> + int len;
> + char *str;
> +
> + str = (char *)of_get_property(node, prop_name, &len);
> + if (!str || len <= 0)
> + return NULL;
> + if (is_dyn)
> + *is_dyn = str[len - 1] != '\0';
> + return str[len - 1] == '\0' ? str : xstrndup(str, len);
> +}
> +
> +static u32 rpi_boot_mode, rpi_boot_part;
> +/* Extract useful information from the VideoCore FDT we got.
> + * Some parameters are defined here:
> + *
> https://www.raspberrypi.com/documentation/computers/configuration.html#part4
> + */
> +static void rpi_vc_fdt_parse(void *fdt)
> {
> - int ret = 0;
> - struct device_node *root = NULL, *node;
> - const char *cmdline;
> + int ret;
> + struct device_node *root, *chosen, *bootloader;
> + enum reset_src_type rst_src = RESET_UKWN;
> + char *str;
> + bool is_dyn;
> + u32 pm_rsts;
>
> root = of_unflatten_dtb(fdt, INT_MAX);
> - if (IS_ERR(root)) {
> - ret = PTR_ERR(root);
> - root = NULL;
> - goto out;
> + if (IS_ERR(root))
> + return;
> +
> + str = of_read_vc_string(root, "serial-number", &is_dyn);
> + if (str) {
> + barebox_set_serial_number(str);
> + if (is_dyn)
> + free(str);
> + } else {
> + pr_warn("no 'serial-number' property found in vc fdt root\n");
> }
>
> - node = of_find_node_by_path_from(root, "/chosen");
> - if (!node) {
> - pr_err("no /chosen node\n");
> - ret = -ENOENT;
> - goto out;
> + str = of_read_vc_string(root, "model", &is_dyn);
> + if (str) {
> + barebox_set_model(str);
> + if (is_dyn)
> + free(str);
> + } else {
> + pr_warn("no 'model' property found in vc fdt root\n");
> }
>
> - cmdline = of_get_property(node, "bootargs", NULL);
> - if (!cmdline) {
> - pr_err("no bootargs property in the /chosen node\n");
> - ret = -ENOENT;
> + chosen = of_find_node_by_path_from(root, "/chosen");
> + if (!chosen) {
> + pr_err("no '/chosen' node found in vc fdt\n");
> goto out;
> }
>
> - globalvar_add_simple("vc.bootargs", cmdline);
> + bootloader = of_find_node_by_name(chosen, "bootloader");
> +
> + str = of_read_vc_string(chosen, "bootargs", NULL);
> + if (str)
> + globalvar_add_simple("vc.bootargs", str);
> + else
> + pr_warn("no 'bootargs' property found in vc fdt '/chosen'\n");
> +
> + str = of_read_vc_string(chosen, "overlay_prefix", NULL);
> + if (str)
> + globalvar_add_simple("vc.overlay_prefix", str);
> + else
> +
Re: Boot hangs during sdhci_transfer_data_dma
Hi Robin, On Mon, Jun 20, 2022 at 04:33:02PM +0200, Robin van der Gracht wrote: > Hi, > > Today I tried to run barebox with CONFIG_KEYBOARD_GPIO=y added to my config. > and noticed my board hangs during boot. When I modify the probe function to > run without registering the poller[1] it boots as expected. > > I started digging into the code to see how far the boot gets when I do > register the poller. I found that Barebox hangs in a do/while loop in > sdhci_transfer_data_dma[2]. > > The contents of the interrupt status (SDHCI_INT_STATUS) is 0 and stays that > way forever trapping the process in the loop. > > Call stack: > > initcall -> barebox_of_populate > state_probe drivers/misc/state.c > state_new_from_nodecommon/state/state.c > of_find_path_by_node drivers/of/of_path.c > __of_find_path drivers/of/of_path.c > device_detectdrivers/base/driver.c > mci_detect_carddrivers/mci/mci-core.c > mci_card_probe drivers/mci/mci-core.c > mci_startupdrivers/mci/mci-core.c > mci_startup_mmc drivers/mci/mci-core.c > mmc_compare_ext_csds drivers/mci/mci-core.c > mci_send_ext_csd drivers/mci/mci-core.c > mci_send_cmd drivers/mci/mci-core.c > esdhc_send_cmd > drivers/mci/imx-esdhc.c > __esdhc_send_cmd > drivers/mci/imx-esdhc-common.c > sdhci_transfer_data_dma drivers/mci/sdhci.c > > > I'm not sure how this happens. It's not the first transfer taking place. I > figured that mayby the poller[1] just adds some cpu load that opens up a > window for this to occur. > > Maybe something else cleared the status register right before we entered the > loop. Thats when I spotted this read/write construction[3]. It's executed > right before we enter the do/while loop and (over)writes to the irq status > register. > > I removed the line with the write command[3] and my board boots as expected. > Why are we (over)writing the status register right after reading it? The idea is likely that we clear the interrupts that we just handled. It seems by the time the status register is overwritten the DMA transfer is already ongoing, and in your case even already done. We should only ever clear the bits we have handled, like sdhci_transfer_data_dma() does with sdhci_write32(sdhci, SDHCI_INT_STATUS, SDHCI_INT_DMA); Apart from this line the code never checks the same bit twice, so clearing anything shoudn't be necessary. Clearing the status register once either at the start or the end of the function should be enough. I think the right thing to do is just to remove the erroneous status register write like you already did. Sascha -- Pengutronix e.K. | | Steuerwalder Str. 21 | http://www.pengutronix.de/ | 31137 Hildesheim, Germany | Phone: +49-5121-206917-0| Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917- |
[PATCH] arm: rockchip: radxa-rock3: enable deep probe support
Enable deep probe support on the Radxa ROCK3 boards.
Signed-off-by: Michael Riesch
---
arch/arm/boards/radxa-rock3/board.c | 3 +++
1 file changed, 3 insertions(+)
diff --git a/arch/arm/boards/radxa-rock3/board.c
b/arch/arm/boards/radxa-rock3/board.c
index 30ad594d0a..aef5ec5df6 100644
--- a/arch/arm/boards/radxa-rock3/board.c
+++ b/arch/arm/boards/radxa-rock3/board.c
@@ -1,6 +1,7 @@
// SPDX-License-Identifier: GPL-2.0-only
#include
#include
+#include
#include
#include
@@ -50,3 +51,5 @@ static struct driver_d rock3_board_driver = {
.of_compatible = rock3_of_match,
};
coredevice_platform_driver(rock3_board_driver);
+
+BAREBOX_DEEP_PROBE_ENABLE(rock3_of_match);
--
2.30.2
Re: Boot hangs during sdhci_transfer_data_dma
Hi Sascha, On 2022-06-21 09:46, Sascha Hauer wrote: Hi Robin, On Mon, Jun 20, 2022 at 04:33:02PM +0200, Robin van der Gracht wrote: Hi, Today I tried to run barebox with CONFIG_KEYBOARD_GPIO=y added to my config. and noticed my board hangs during boot. When I modify the probe function to run without registering the poller[1] it boots as expected. I started digging into the code to see how far the boot gets when I do register the poller. I found that Barebox hangs in a do/while loop in sdhci_transfer_data_dma[2]. The contents of the interrupt status (SDHCI_INT_STATUS) is 0 and stays that way forever trapping the process in the loop. Call stack: initcall -> barebox_of_populate state_probe drivers/misc/state.c state_new_from_nodecommon/state/state.c of_find_path_by_node drivers/of/of_path.c __of_find_path drivers/of/of_path.c device_detect drivers/base/driver.c mci_detect_card drivers/mci/mci-core.c mci_card_probe drivers/mci/mci-core.c mci_startup drivers/mci/mci-core.c mci_startup_mmc drivers/mci/mci-core.c mmc_compare_ext_csds drivers/mci/mci-core.c mci_send_ext_csd drivers/mci/mci-core.c mci_send_cmd drivers/mci/mci-core.c esdhc_send_cmd drivers/mci/imx-esdhc.c __esdhc_send_cmd drivers/mci/imx-esdhc-common.c sdhci_transfer_data_dma drivers/mci/sdhci.c I'm not sure how this happens. It's not the first transfer taking place. I figured that mayby the poller[1] just adds some cpu load that opens up a window for this to occur. Maybe something else cleared the status register right before we entered the loop. Thats when I spotted this read/write construction[3]. It's executed right before we enter the do/while loop and (over)writes to the irq status register. I removed the line with the write command[3] and my board boots as expected. Why are we (over)writing the status register right after reading it? The idea is likely that we clear the interrupts that we just handled. It seems by the time the status register is overwritten the DMA transfer is already ongoing, and in your case even already done. I can confirm this. When the data transfer is still ongoing the status register holds 0x1 (SDHCI_INT_CMD_COMPLETE). A few lines above the write there is a busy wait for this bit to be set. When my board hangs the status register holds 0x000b at the timen it is cleared. Which means the DMA engine has stopped on a buffer boundary. Apparently this can sometimes happen shortly after starting. We should only ever clear the bits we have handled, like sdhci_transfer_data_dma() does with sdhci_write32(sdhci, SDHCI_INT_STATUS, SDHCI_INT_DMA); Ack. This would suffice: sdhci_write32(&host->sdhci, SDHCI_INT_STATUS, SDHCI_INT_CMD_COMPLETE); Apart from this line the code never checks the same bit twice, so clearing anything shoudn't be necessary. Clearing the status register once either at the start or the end of the function should be enough. I think the right thing to do is just to remove the erroneous status register write like you already did. I'll submit a patch that removes the write. Thank you for thinking along. - Robin
Re: Boot hangs during sdhci_transfer_data_dma
On 2022-06-21 09:46, Sascha Hauer wrote: Hi Robin, ... We should only ever clear the bits we have handled, like sdhci_transfer_data_dma() does with sdhci_write32(sdhci, SDHCI_INT_STATUS, SDHCI_INT_DMA); I just noticed that the tegra-sdmmc mci driver might have the same issue. https://git.pengutronix.de/cgit/barebox/tree/drivers/mci/tegra-sdmmc.c#n149 It can prevent the following conditional from ever evaluating true trapping the process in the while loop. https://git.pengutronix.de/cgit/barebox/tree/drivers/mci/tegra-sdmmc.c#n203 This driver clears the status register on error (not on start/end of function) with an old value 'val' which can be lacking status bits that popped up along the way... I can't test code changes for this platform. - Robin
[PATCH] mci: imx-esdhc-common: Don't clear unhandled status bits
A DMA cmd + data transfer can finish or stop (i.e. on a block gap) before the status register is cleared. In that case we'll lose track of state causing sdhci_transfer_data_dma() to loop forever waiting for status bits that are already cleared. Clearing SDHCI_INT_CMD_COMPLETE should suffice here. Since it's not evaluated a second time, clearing it at the start of the function is sufficient so we can just remove the erroneous status write. Signed-off-by: Robin van der Gracht --- drivers/mci/imx-esdhc-common.c | 1 - 1 file changed, 1 deletion(-) diff --git a/drivers/mci/imx-esdhc-common.c b/drivers/mci/imx-esdhc-common.c index a12db43c88..27293e44b7 100644 --- a/drivers/mci/imx-esdhc-common.c +++ b/drivers/mci/imx-esdhc-common.c @@ -176,7 +176,6 @@ int __esdhc_send_cmd(struct fsl_esdhc_host *host, struct mci_cmd *cmd, } irqstat = sdhci_read32(&host->sdhci, SDHCI_INT_STATUS); - sdhci_write32(&host->sdhci, SDHCI_INT_STATUS, irqstat); if (irqstat & CMD_ERR) return -EIO; -- 2.34.1
Re: [PATCH] arm: imx: mmdc_size: Increase row_max for imx8m
Hi!
Am Montag, dem 20.06.2022 um 15:19 +0200 schrieb Ahmad Fatoum:
> Hi,
>
> On 20.06.22 14:47, Teresa Remmet wrote:
> > Am Montag, dem 20.06.2022 um 14:38 +0200 schrieb Ahmad Fatoum:
> > > On 20.06.22 14:27, Teresa Remmet wrote:
> > > > I have set the DDRC_ADDRMAP7 register manually in the RAM
> > > > configuration
> > > > in such a case. As I don't see a solution that fits for all.
> > > > But
> > > > would
> > > > be happy for one. :)
> > >
> > > What would the 'neutral' value to write into this register be?
> > > zero
> > > seems to not be it.
> >
> > it's
> >
> > 0xf0f for ADDRMAP7.
> >
> > Reference Manual says: "If set to 15, row address bit X is set to
> > 0"
>
> Do newer spreadsheets always generate ADDRMAP7 writes even if the
> value is zero? If so, we could perhaps initialize it to 0xf0f before
> running ddr_cfg_umctl2(). The DDRC seems to be in reset while the
> registers are being written, so this might just work.
this is not so easy to figure out because the spread sheet does
not generate the c headers directly.
But it is true for other ADDRMAP registers where the reset value is also zero.
So I would give it a try.
>
> As 0 is a valid value, I am wondering if this snippet introduced with
> 42d45ef380c5 ("ARM: imx: Add imx8 support for SDRAM with two or more
> bank groups")
> is correct:
>
> if (addrmap[8]) {
> if (FIELD_GET(DDRC_ADDRMAP8_BG_B0, addrmap[8]) != 0b1)
> banks++;
> if (FIELD_GET(DDRC_ADDRMAP8_BG_B1, addrmap[8]) != 0b11)
> banks++;
> }
yes, this is wrong. LPDDR4 is no problem at the moment as the
spreadsheet does not set the value and the reset value here is zero.
But DDR4 uses it and there could be a "real" zero set which is then
ignored.
Regards,
Teresa
>
> Thanks,
> Ahmad
>
> > Regards,
> > Teresa
> >
> >
> > > Thanks,
> > > Ahmad
> > >
> > > > Regards,
> > > > Teresa
> > > >
> > > > > > ---
> > > > > > arch/arm/mach-imx/esdctl.c | 2 +-
> > > > > > 1 file changed, 1 insertion(+), 1 deletion(-)
> > > > > >
> > > > > > diff --git a/arch/arm/mach-imx/esdctl.c b/arch/arm/mach-
> > > > > > imx/esdctl.c
> > > > > > index 8dd0ddbbc965..b070ebc62a45 100644
> > > > > > --- a/arch/arm/mach-imx/esdctl.c
> > > > > > +++ b/arch/arm/mach-imx/esdctl.c
> > > > > > @@ -488,7 +488,7 @@ static resource_size_t
> > > > > > imx8m_ddrc_sdram_size(void __iomem *ddrc)
> > > > > >
> > > > > > return imx_ddrc_sdram_size(ddrc, addrmap,
> > > > > >12, ARRAY_AND_SIZE(col_b),
> > > > > > - 16, ARRAY_AND_SIZE(row_b),
> > > > > > + 18, ARRAY_AND_SIZE(row_b),
> > > > > >reduced_adress_space, true);
> > > > > > }
> > > > > >
>
>
--
PHYTEC Messtechnik GmbH | Robert-Koch-Str. 39 | 55129 Mainz, Germany
Geschäftsführer: Dipl.-Ing. Michael Mitezki, Dipl.-Ing. Bodo Huber |
Handelsregister Mainz HRB 4656 | Finanzamt Mainz | St.Nr. 266500608, DE
149059855
[PATCH] ARM: rpi: cleanup parsing of vc fdt
Refactoring and cleanup of vc fdt parsing based on advice
from Ahmad Fatoum.
Signed-off-by: Daniel Brát
---
I originally wanted to send these as part of v5 of
'ARM: rpi: parse useful data from vc fdt' patch, but you
were quicker with accepting the v4 :)
Hope that's not a problem.
arch/arm/boards/raspberry-pi/rpi-common.c | 114 +++---
1 file changed, 58 insertions(+), 56 deletions(-)
diff --git a/arch/arm/boards/raspberry-pi/rpi-common.c
b/arch/arm/boards/raspberry-pi/rpi-common.c
index eaca4edef..77935e5c8 100644
--- a/arch/arm/boards/raspberry-pi/rpi-common.c
+++ b/arch/arm/boards/raspberry-pi/rpi-common.c
@@ -202,22 +202,52 @@ static int rpi_env_init(void)
}
/* Some string properties in fdt passed to us from vc may be
- * malformed by not being null terminated. In that case, create and
- * return a null terminated copy.
+ * malformed by not being null terminated, so just create and
+ * return a fixed copy.
*/
static char *of_read_vc_string(struct device_node *node,
-const char *prop_name,
-bool *is_dyn)
+ const char *prop_name)
{
int len;
- char *str;
+ const char *str;
- str = (char *)of_get_property(node, prop_name, &len);
- if (!str || len <= 0)
+ str = of_get_property(node, prop_name, &len);
+ if (!str) {
+ pr_warn("no property '%s' found in vc fdt's '%s' node\n",
+ prop_name, node->full_name);
return NULL;
- if (is_dyn)
- *is_dyn = str[len - 1] != '\0';
- return str[len - 1] == '\0' ? str : xstrndup(str, len);
+ }
+ return xstrndup(str, len);
+}
+
+static enum reset_src_type rpi_decode_pm_rsts(struct device_node *chosen,
+ struct device_node *bootloader)
+{
+ u32 pm_rsts;
+ int ret;
+
+ ret = of_property_read_u32(chosen, "pm_rsts", &pm_rsts);
+ if (ret && bootloader)
+ret = of_property_read_u32(bootloader, "rsts", &pm_rsts);
+
+ if (ret) {
+ pr_warn("'pm_rsts' value not found in vc fdt\n");
+ return RESET_UKWN;
+ }
+ /*
+* https://github.com/raspberrypi/linux/issues/932#issuecomment-93989581
+*/
+
+ if (pm_rsts & PM_RSTS_HADPOR_SET)
+ return RESET_POR;
+ if (pm_rsts & PM_RSTS_HADDR_SET)
+ return RESET_JTAG;
+ if (pm_rsts & PM_RSTS_HADWR_SET)
+ return RESET_WDG;
+ if (pm_rsts & PM_RSTS_HADSR_SET)
+ return RESET_RST;
+
+ return RESET_UKWN;
}
static u32 rpi_boot_mode, rpi_boot_part;
@@ -229,31 +259,22 @@ static void rpi_vc_fdt_parse(void *fdt)
{
int ret;
struct device_node *root, *chosen, *bootloader;
- enum reset_src_type rst_src = RESET_UKWN;
char *str;
- bool is_dyn;
- u32 pm_rsts;
root = of_unflatten_dtb(fdt, INT_MAX);
if (IS_ERR(root))
return;
- str = of_read_vc_string(root, "serial-number", &is_dyn);
+ str = of_read_vc_string(root, "serial-number");
if (str) {
barebox_set_serial_number(str);
- if (is_dyn)
- free(str);
- } else {
- pr_warn("no 'serial-number' property found in vc fdt root\n");
+ free(str);
}
- str = of_read_vc_string(root, "model", &is_dyn);
+ str = of_read_vc_string(root, "model");
if (str) {
barebox_set_model(str);
- if (is_dyn)
- free(str);
- } else {
- pr_warn("no 'model' property found in vc fdt root\n");
+ free(str);
}
chosen = of_find_node_by_path_from(root, "/chosen");
@@ -264,23 +285,23 @@ static void rpi_vc_fdt_parse(void *fdt)
bootloader = of_find_node_by_name(chosen, "bootloader");
- str = of_read_vc_string(chosen, "bootargs", NULL);
- if (str)
+ str = of_read_vc_string(chosen, "bootargs");
+ if (str) {
globalvar_add_simple("vc.bootargs", str);
- else
- pr_warn("no 'bootargs' property found in vc fdt '/chosen'\n");
+ free(str);
+ }
- str = of_read_vc_string(chosen, "overlay_prefix", NULL);
- if (str)
+ str = of_read_vc_string(chosen, "overlay_prefix");
+ if (str) {
globalvar_add_simple("vc.overlay_prefix", str);
- else
- pr_warn("no 'overlay_prefix' property found in vc fdt
'/chosen'\n");
+ free(str);
+ }
- str = of_read_vc_string(chosen, "os_prefix", NULL);
- if (str)
+ str = of_read_vc_string(chosen, "os_prefix");
+ if (str) {
globalvar_add_simple("vc.os_prefix", str);
- else
- pr_warn("no 'os_prefix' property found in vc fdt '
