diff mbox series

[4/4] arm: mvebu: clearfog: Detect MMC vs SDHC and fixup fdt

Message ID 20230305113100.317824-5-martin.p.rowe@gmail.com
State Superseded
Delegated to: Stefan Roese
Headers show
Series arm: mvebu: clearfog: defconfig and eMMC updates | expand

Commit Message

Martin Rowe March 5, 2023, 11:31 a.m. UTC 
[upstream of vendor commit 19a96f7c40a8fc1d0a6546ac2418d966e5840a99]

The Clearfog devices have only one SDHC device. This is either eMMC if
it is populated on the SOM or SDHC if not. The Linux device tree assumes
the SDHC case. Detect if the device is an eMMC and fixup the device-tree
so it will be detected by Linux.

Ported from vendor repo at https://github.com/SolidRun/u-boot

Signed-off-by: Martin Rowe <martin.p.rowe@gmail.com>
---
 board/solidrun/clearfog/clearfog.c | 42 ++++++++++++++++++++++++++++++
 configs/clearfog_defconfig         |  1 +
 configs/clearfog_sata_defconfig    |  1 +
 configs/clearfog_spi_defconfig     |  1 +
 4 files changed, 45 insertions(+)

Comments

Pali Rohár March 5, 2023, 12:43 p.m. UTC | #1 
On Sunday 05 March 2023 21:31:00 Martin Rowe wrote:
> [upstream of vendor commit 19a96f7c40a8fc1d0a6546ac2418d966e5840a99]
> 
> The Clearfog devices have only one SDHC device. This is either eMMC if
> it is populated on the SOM or SDHC if not. The Linux device tree assumes
> the SDHC case. Detect if the device is an eMMC and fixup the device-tree
> so it will be detected by Linux.
> 
> Ported from vendor repo at https://github.com/SolidRun/u-boot
> 
> Signed-off-by: Martin Rowe <martin.p.rowe@gmail.com>
> ---
>  board/solidrun/clearfog/clearfog.c | 42 ++++++++++++++++++++++++++++++
>  configs/clearfog_defconfig         |  1 +
>  configs/clearfog_sata_defconfig    |  1 +
>  configs/clearfog_spi_defconfig     |  1 +
>  4 files changed, 45 insertions(+)
> 
> diff --git a/board/solidrun/clearfog/clearfog.c b/board/solidrun/clearfog/clearfog.c
> index 03adb591d8..cafead3a1a 100644
> --- a/board/solidrun/clearfog/clearfog.c
> +++ b/board/solidrun/clearfog/clearfog.c
> @@ -10,6 +10,7 @@
>  #include <miiphy.h>
>  #include <net.h>
>  #include <netdev.h>
> +#include <mmc.h>
>  #include <asm/global_data.h>
>  #include <asm/io.h>
>  #include <asm/arch/cpu.h>
> @@ -261,3 +262,44 @@ int board_late_init(void)
>  
>  	return 0;
>  }
> +
> +static bool has_emmc(void)
> +{
> +	struct mmc *mmc;
> +	mmc = find_mmc_device(0);
> +	if (!mmc)
> +		return 0;
> +	return (!mmc_init(mmc) && IS_MMC(mmc)) ? true : false;
> +}
> +
> +/*
> + * The Clearfog devices have only one SDHC device. This is either eMMC
> + * if it is populated on the SOM or SDHC if not. The Linux device tree
> + * assumes the SDHC case. Detect if the device is an eMMC and fixup the
> + * device-tree so it will be detected by Linux.
> + */
> +int ft_board_setup(void *blob, struct bd_info *bd)
> +{
> +	__maybe_unused int offset, rc;
> +
> +	if (has_emmc()) {
> +		puts("Patching FDT so that eMMC is detected by kernel\n");
> +		offset = fdt_path_offset(blob, "/soc/internal-regs/sdhci@d8000");

Do not use fixed DT paths when parsing Linux kernel DT files. Linux
developers often changes DT node names in their DTS files which cause
changing of DT paths and hence this code in bootloader stops working.

I would suggest to find node offset by compatible string
("marvell,armada-380-sdhci"). Look for inspiration what I done in past:
https://source.denx.de/u-boot/u-boot/-/commit/7cd67018dd7f754c66cf08a76397bbee254b32aa

> +		if (offset >= 0) {
> +			rc = fdt_delprop(blob, offset, "cd-gpios");
> +			if (rc == -FDT_ERR_NOTFOUND)
> +				return 0; /* Assume that the device tree is already customized for eMMC */

I would suggest to always add "non-removable" property when it does not
exist.

> +			if (rc) {
> +				printf("Unable to remove cd-gpios for eMMC, err=%s\n", fdt_strerror(rc));
> +				return rc;

Normally it is not a good idea to return non-zero value from
ft_board_setup() function because it "crashes" u-boot and stops booting
kernel.

> +			}
> +			rc = fdt_setprop_empty(blob, offset, "non-removable");
> +			if (rc) {
> +				printf("Unable to add non-removable for eMMC, err=%s\n", fdt_strerror(rc));
> +				return rc;
> +			}
> +		}
> +	}
> +
> +	return 0;
> +}
> diff --git a/configs/clearfog_defconfig b/configs/clearfog_defconfig
> index f9cfa94e77..45b093c6d8 100644
> --- a/configs/clearfog_defconfig
> +++ b/configs/clearfog_defconfig
> @@ -21,6 +21,7 @@ CONFIG_SYS_LOAD_ADDR=0x800000
>  CONFIG_DEBUG_UART=y
>  CONFIG_AHCI=y
>  CONFIG_DISTRO_DEFAULTS=y
> +CONFIG_OF_BOARD_SETUP=y

Hint: To avoid having CONFIG_OF_BOARD_SETUP=y in every defconfig file,
you can add "select OF_BOARD_SETUP" into "config TARGET_CLEARFOG"
section in arch/arm/mach-mvebu/Kconfig file.

>  CONFIG_BOOTDELAY=3
>  CONFIG_USE_PREBOOT=y
>  CONFIG_SYS_CONSOLE_INFO_QUIET=y
> diff --git a/configs/clearfog_sata_defconfig b/configs/clearfog_sata_defconfig
> index b11d8746b0..081ebd7e7a 100644
> --- a/configs/clearfog_sata_defconfig
> +++ b/configs/clearfog_sata_defconfig
> @@ -23,6 +23,7 @@ CONFIG_SYS_LOAD_ADDR=0x800000
>  CONFIG_DEBUG_UART=y
>  CONFIG_AHCI=y
>  CONFIG_DISTRO_DEFAULTS=y
> +CONFIG_OF_BOARD_SETUP=y
>  CONFIG_BOOTDELAY=3
>  CONFIG_USE_PREBOOT=y
>  CONFIG_SYS_CONSOLE_INFO_QUIET=y
> diff --git a/configs/clearfog_spi_defconfig b/configs/clearfog_spi_defconfig
> index d6695f31e6..6b9e817f68 100644
> --- a/configs/clearfog_spi_defconfig
> +++ b/configs/clearfog_spi_defconfig
> @@ -24,6 +24,7 @@ CONFIG_SYS_LOAD_ADDR=0x800000
>  CONFIG_DEBUG_UART=y
>  CONFIG_AHCI=y
>  CONFIG_DISTRO_DEFAULTS=y
> +CONFIG_OF_BOARD_SETUP=y
>  CONFIG_BOOTDELAY=3
>  CONFIG_USE_PREBOOT=y
>  CONFIG_SYS_CONSOLE_INFO_QUIET=y
> -- 
> 2.39.2
>
Martin Rowe March 6, 2023, 6:32 a.m. UTC | #2 
On Sun, 5 Mar 2023 at 12:43, Pali Rohár <pali@kernel.org> wrote:

> On Sunday 05 March 2023 21:31:00 Martin Rowe wrote:
> > [upstream of vendor commit 19a96f7c40a8fc1d0a6546ac2418d966e5840a99]
> >
> > The Clearfog devices have only one SDHC device. This is either eMMC if
> > it is populated on the SOM or SDHC if not. The Linux device tree assumes
> > the SDHC case. Detect if the device is an eMMC and fixup the device-tree
> > so it will be detected by Linux.
> >
> > Ported from vendor repo at https://github.com/SolidRun/u-boot
> >
> > Signed-off-by: Martin Rowe <martin.p.rowe@gmail.com>
> > ---
> >  board/solidrun/clearfog/clearfog.c | 42 ++++++++++++++++++++++++++++++
> >  configs/clearfog_defconfig         |  1 +
> >  configs/clearfog_sata_defconfig    |  1 +
> >  configs/clearfog_spi_defconfig     |  1 +
> >  4 files changed, 45 insertions(+)
> >
> > diff --git a/board/solidrun/clearfog/clearfog.c
> b/board/solidrun/clearfog/clearfog.c
> > index 03adb591d8..cafead3a1a 100644
> > --- a/board/solidrun/clearfog/clearfog.c
> > +++ b/board/solidrun/clearfog/clearfog.c
> > @@ -10,6 +10,7 @@
> >  #include <miiphy.h>
> >  #include <net.h>
> >  #include <netdev.h>
> > +#include <mmc.h>
> >  #include <asm/global_data.h>
> >  #include <asm/io.h>
> >  #include <asm/arch/cpu.h>
> > @@ -261,3 +262,44 @@ int board_late_init(void)
> >
> >       return 0;
> >  }
> > +
> > +static bool has_emmc(void)
> > +{
> > +     struct mmc *mmc;
> > +     mmc = find_mmc_device(0);
> > +     if (!mmc)
> > +             return 0;
> > +     return (!mmc_init(mmc) && IS_MMC(mmc)) ? true : false;
> > +}
> > +
> > +/*
> > + * The Clearfog devices have only one SDHC device. This is either eMMC
> > + * if it is populated on the SOM or SDHC if not. The Linux device tree
> > + * assumes the SDHC case. Detect if the device is an eMMC and fixup the
> > + * device-tree so it will be detected by Linux.
> > + */
> > +int ft_board_setup(void *blob, struct bd_info *bd)
> > +{
> > +     __maybe_unused int offset, rc;
> > +
> > +     if (has_emmc()) {
> > +             puts("Patching FDT so that eMMC is detected by kernel\n");
> > +             offset = fdt_path_offset(blob,
> "/soc/internal-regs/sdhci@d8000");
>
> Do not use fixed DT paths when parsing Linux kernel DT files. Linux
> developers often changes DT node names in their DTS files which cause
> changing of DT paths and hence this code in bootloader stops working.
>
> I would suggest to find node offset by compatible string
> ("marvell,armada-380-sdhci"). Look for inspiration what I done in past:
>
> https://source.denx.de/u-boot/u-boot/-/commit/7cd67018dd7f754c66cf08a76397bbee254b32aa
>

Makes sense, I'll rework the patch.


> > +             if (offset >= 0) {
> > +                     rc = fdt_delprop(blob, offset, "cd-gpios");
> > +                     if (rc == -FDT_ERR_NOTFOUND)
> > +                             return 0; /* Assume that the device tree
> is already customized for eMMC */
>
> I would suggest to always add "non-removable" property when it does not
> exist.
>

OK, that makes sense, too. I'll test that out.

> +                     if (rc) {
> > +                             printf("Unable to remove cd-gpios for
> eMMC, err=%s\n", fdt_strerror(rc));
> > +                             return rc;
>
> Normally it is not a good idea to return non-zero value from
> ft_board_setup() function because it "crashes" u-boot and stops booting
> kernel.
>

I think this might be an exception. If the board has eMMC there's a
reasonable chance that it's used for the root device. By the time Linux
tries and fails to mount the root device there are already reams of printks
on the serial console that make it hard to see the root cause all the way
back in u-boot. Failing early with an appropriate error message would be
more beneficial while troubleshooting this problem.

There's a corner case where you have an eMMC board and never use the eMMC
device (like my use case), so you wouldn't want to have it fail for
something that you don't use. I think we'll cause more issues for people
that can't mount their root device than for people that get an error for a
device they don't use.

Given I'm going to rework the logic here anyway it's likely this first
'return rc' will be removed, but I think it is worth retaining the one
below.

> +                     }
> > +                     rc = fdt_setprop_empty(blob, offset,
> "non-removable");
> > +                     if (rc) {
> > +                             printf("Unable to add non-removable for
> eMMC, err=%s\n", fdt_strerror(rc));
> > +                             return rc;
> > +                     }
> > +             }
> > +     }
> > +
> > +     return 0;
> > +}
> > diff --git a/configs/clearfog_defconfig b/configs/clearfog_defconfig
> > index f9cfa94e77..45b093c6d8 100644
> > --- a/configs/clearfog_defconfig
> > +++ b/configs/clearfog_defconfig
> > @@ -21,6 +21,7 @@ CONFIG_SYS_LOAD_ADDR=0x800000
> >  CONFIG_DEBUG_UART=y
> >  CONFIG_AHCI=y
> >  CONFIG_DISTRO_DEFAULTS=y
> > +CONFIG_OF_BOARD_SETUP=y
>
> Hint: To avoid having CONFIG_OF_BOARD_SETUP=y in every defconfig file,
> you can add "select OF_BOARD_SETUP" into "config TARGET_CLEARFOG"
> section in arch/arm/mach-mvebu/Kconfig file.
>

I'll fix that up.


> >  CONFIG_BOOTDELAY=3
> >  CONFIG_USE_PREBOOT=y
> >  CONFIG_SYS_CONSOLE_INFO_QUIET=y
> > diff --git a/configs/clearfog_sata_defconfig
> b/configs/clearfog_sata_defconfig
> > index b11d8746b0..081ebd7e7a 100644
> > --- a/configs/clearfog_sata_defconfig
> > +++ b/configs/clearfog_sata_defconfig
> > @@ -23,6 +23,7 @@ CONFIG_SYS_LOAD_ADDR=0x800000
> >  CONFIG_DEBUG_UART=y
> >  CONFIG_AHCI=y
> >  CONFIG_DISTRO_DEFAULTS=y
> > +CONFIG_OF_BOARD_SETUP=y
> >  CONFIG_BOOTDELAY=3
> >  CONFIG_USE_PREBOOT=y
> >  CONFIG_SYS_CONSOLE_INFO_QUIET=y
> > diff --git a/configs/clearfog_spi_defconfig
> b/configs/clearfog_spi_defconfig
> > index d6695f31e6..6b9e817f68 100644
> > --- a/configs/clearfog_spi_defconfig
> > +++ b/configs/clearfog_spi_defconfig
> > @@ -24,6 +24,7 @@ CONFIG_SYS_LOAD_ADDR=0x800000
> >  CONFIG_DEBUG_UART=y
> >  CONFIG_AHCI=y
> >  CONFIG_DISTRO_DEFAULTS=y
> > +CONFIG_OF_BOARD_SETUP=y
> >  CONFIG_BOOTDELAY=3
> >  CONFIG_USE_PREBOOT=y
> >  CONFIG_SYS_CONSOLE_INFO_QUIET=y
> > --
> > 2.39.2
> >
>
diff mbox series

Patch

diff --git a/board/solidrun/clearfog/clearfog.c b/board/solidrun/clearfog/clearfog.c
index 03adb591d8..cafead3a1a 100644
--- a/board/solidrun/clearfog/clearfog.c
+++ b/board/solidrun/clearfog/clearfog.c
@@ -10,6 +10,7 @@ 
 #include <miiphy.h>
 #include <net.h>
 #include <netdev.h>
+#include <mmc.h>
 #include <asm/global_data.h>
 #include <asm/io.h>
 #include <asm/arch/cpu.h>
@@ -261,3 +262,44 @@  int board_late_init(void)
 
 	return 0;
 }
+
+static bool has_emmc(void)
+{
+	struct mmc *mmc;
+	mmc = find_mmc_device(0);
+	if (!mmc)
+		return 0;
+	return (!mmc_init(mmc) && IS_MMC(mmc)) ? true : false;
+}
+
+/*
+ * The Clearfog devices have only one SDHC device. This is either eMMC
+ * if it is populated on the SOM or SDHC if not. The Linux device tree
+ * assumes the SDHC case. Detect if the device is an eMMC and fixup the
+ * device-tree so it will be detected by Linux.
+ */
+int ft_board_setup(void *blob, struct bd_info *bd)
+{
+	__maybe_unused int offset, rc;
+
+	if (has_emmc()) {
+		puts("Patching FDT so that eMMC is detected by kernel\n");
+		offset = fdt_path_offset(blob, "/soc/internal-regs/sdhci@d8000");
+		if (offset >= 0) {
+			rc = fdt_delprop(blob, offset, "cd-gpios");
+			if (rc == -FDT_ERR_NOTFOUND)
+				return 0; /* Assume that the device tree is already customized for eMMC */
+			if (rc) {
+				printf("Unable to remove cd-gpios for eMMC, err=%s\n", fdt_strerror(rc));
+				return rc;
+			}
+			rc = fdt_setprop_empty(blob, offset, "non-removable");
+			if (rc) {
+				printf("Unable to add non-removable for eMMC, err=%s\n", fdt_strerror(rc));
+				return rc;
+			}
+		}
+	}
+
+	return 0;
+}
diff --git a/configs/clearfog_defconfig b/configs/clearfog_defconfig
index f9cfa94e77..45b093c6d8 100644
--- a/configs/clearfog_defconfig
+++ b/configs/clearfog_defconfig
@@ -21,6 +21,7 @@  CONFIG_SYS_LOAD_ADDR=0x800000
 CONFIG_DEBUG_UART=y
 CONFIG_AHCI=y
 CONFIG_DISTRO_DEFAULTS=y
+CONFIG_OF_BOARD_SETUP=y
 CONFIG_BOOTDELAY=3
 CONFIG_USE_PREBOOT=y
 CONFIG_SYS_CONSOLE_INFO_QUIET=y
diff --git a/configs/clearfog_sata_defconfig b/configs/clearfog_sata_defconfig
index b11d8746b0..081ebd7e7a 100644
--- a/configs/clearfog_sata_defconfig
+++ b/configs/clearfog_sata_defconfig
@@ -23,6 +23,7 @@  CONFIG_SYS_LOAD_ADDR=0x800000
 CONFIG_DEBUG_UART=y
 CONFIG_AHCI=y
 CONFIG_DISTRO_DEFAULTS=y
+CONFIG_OF_BOARD_SETUP=y
 CONFIG_BOOTDELAY=3
 CONFIG_USE_PREBOOT=y
 CONFIG_SYS_CONSOLE_INFO_QUIET=y
diff --git a/configs/clearfog_spi_defconfig b/configs/clearfog_spi_defconfig
index d6695f31e6..6b9e817f68 100644
--- a/configs/clearfog_spi_defconfig
+++ b/configs/clearfog_spi_defconfig
@@ -24,6 +24,7 @@  CONFIG_SYS_LOAD_ADDR=0x800000
 CONFIG_DEBUG_UART=y
 CONFIG_AHCI=y
 CONFIG_DISTRO_DEFAULTS=y
+CONFIG_OF_BOARD_SETUP=y
 CONFIG_BOOTDELAY=3
 CONFIG_USE_PREBOOT=y
 CONFIG_SYS_CONSOLE_INFO_QUIET=y