[PATCH v2 1/3] mx6sabresd: Fix U-Boot corruption after saving the environment

[PATCH v2 1/3] mx6sabresd: Fix U-Boot corruption after saving the environment

[Fabio Estevam]

From: Fabio Estevam <festevam@denx.de>

U-Boot binary has grown in such a way that it goes beyond the reserved
area for the environment variables.
    
Running "saveenv" and rebooting the board causes U-Boot to hang because
of this overlap.
    
Fix this problem by selecting CONFIG_LTO so that the U-Boot proper
size can be reduced.
    
Also, to prevent this same problem to happen in the future, use
CONFIG_BOARD_SIZE_LIMIT, which can detect the overlap in build-time.
    
CONFIG_BOARD_SIZE_LIMIT is calculated as follows:

CONFIG_BOARD_SIZE_LIMIT = CONFIG_ENV_OFFSET - u-boot-img.dtb offset
CONFIG_BOARD_SIZE_LIMIT = 0xc000 - 69 * 1024
CONFIG_BOARD_SIZE_LIMIT = 715766

Signed-off-by: Fabio Estevam <festevam@denx.de>
---
Changes since v1:
- Select LTO. (Tom)

 configs/mx6sabresd_defconfig | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/configs/mx6sabresd_defconfig b/configs/mx6sabresd_defconfig
index a90efe4a7786..d4fb55622cf1 100644
--- a/configs/mx6sabresd_defconfig
+++ b/configs/mx6sabresd_defconfig
@@ -21,6 +21,9 @@ CONFIG_SPL_SERIAL=y
 CONFIG_SPL=y
 CONFIG_SPL_LIBDISK_SUPPORT=y
 CONFIG_PCI=y
+CONFIG_LTO=y
+CONFIG_HAS_BOARD_SIZE_LIMIT=y
+CONFIG_BOARD_SIZE_LIMIT=715766
 CONFIG_FIT=y
 CONFIG_SPL_FIT_PRINT=y
 CONFIG_SPL_LOAD_FIT=y
-- 
2.34.1

[PATCH v2 2/3] mx6sabresd: Convert to watchdog driver model

[Fabio Estevam]

From: Fabio Estevam <festevam@denx.de>

Commit 68dcbdd594d4 ("ARM: imx: Add weak default reset_cpu()") caused
the 'reset' command in U-Boot to not cause a board reset.

Fix it by switching to the watchdog driver model via sysreset, which
is the preferred method for implementing the watchdog reset.

Signed-off-by: Fabio Estevam <festevam@denx.de>
---
Changes since v1:
- None.

 arch/arm/dts/imx6qdl-sabresd-u-boot.dtsi | 9 +++++++++
 configs/mx6sabresd_defconfig             | 3 +++
 2 files changed, 12 insertions(+)

diff --git a/arch/arm/dts/imx6qdl-sabresd-u-boot.dtsi b/arch/arm/dts/imx6qdl-sabresd-u-boot.dtsi
index 5c4101b76da2..9e9c4422f00e 100644
--- a/arch/arm/dts/imx6qdl-sabresd-u-boot.dtsi
+++ b/arch/arm/dts/imx6qdl-sabresd-u-boot.dtsi
@@ -9,6 +9,11 @@
 	aliases {
 		mmc1 = &usdhc3;
 	};
+	wdt-reboot {
+		compatible = "wdt-reboot";
+		wdt = <&wdog2>;
+		bootph-pre-ram;
+	};
 };
 
 &usdhc3 {
@@ -18,3 +23,7 @@
 &pinctrl_usdhc3 {
 	bootph-pre-ram;
 };
+
+&wdog2 {
+	bootph-pre-ram;
+};
diff --git a/configs/mx6sabresd_defconfig b/configs/mx6sabresd_defconfig
index f19df607e7ae..69685f12eb10 100644
--- a/configs/mx6sabresd_defconfig
+++ b/configs/mx6sabresd_defconfig
@@ -103,6 +103,8 @@ CONFIG_DM_REGULATOR_FIXED=y
 CONFIG_DM_REGULATOR_GPIO=y
 CONFIG_DM_SERIAL=y
 CONFIG_MXC_UART=y
+CONFIG_SYSRESET=y
+CONFIG_SYSRESET_WATCHDOG=y
 CONFIG_SPI=y
 CONFIG_DM_SPI=y
 CONFIG_MXC_SPI=y
@@ -129,3 +131,4 @@ CONFIG_IMX_HDMI=y
 CONFIG_SPLASH_SCREEN=y
 CONFIG_SPLASH_SCREEN_ALIGN=y
 CONFIG_BMP_16BPP=y
+CONFIG_IMX_WATCHDOG=y
-- 
2.34.1

Re: [PATCH v2 2/3] mx6sabresd: Convert to watchdog driver model

[Tom Rini]

[-- Attachment #1: Type: text/plain, Size: 1246 bytes --]

On Fri, Feb 02, 2024 at 01:04:04PM -0300, Fabio Estevam wrote:

> From: Fabio Estevam <festevam@denx.de>
> 
> Commit 68dcbdd594d4 ("ARM: imx: Add weak default reset_cpu()") caused
> the 'reset' command in U-Boot to not cause a board reset.
> 
> Fix it by switching to the watchdog driver model via sysreset, which
> is the preferred method for implementing the watchdog reset.
> 
> Signed-off-by: Fabio Estevam <festevam@denx.de>
> ---
> Changes since v1:
> - None.
> 
>  arch/arm/dts/imx6qdl-sabresd-u-boot.dtsi | 9 +++++++++
>  configs/mx6sabresd_defconfig             | 3 +++
>  2 files changed, 12 insertions(+)
> 
> diff --git a/arch/arm/dts/imx6qdl-sabresd-u-boot.dtsi b/arch/arm/dts/imx6qdl-sabresd-u-boot.dtsi
> index 5c4101b76da2..9e9c4422f00e 100644
> --- a/arch/arm/dts/imx6qdl-sabresd-u-boot.dtsi
> +++ b/arch/arm/dts/imx6qdl-sabresd-u-boot.dtsi
> @@ -9,6 +9,11 @@
>  	aliases {
>  		mmc1 = &usdhc3;
>  	};
> +	wdt-reboot {
> +		compatible = "wdt-reboot";
> +		wdt = <&wdog2>;
> +		bootph-pre-ram;
> +	};
>  };
>  
>  &usdhc3 {
> @@ -18,3 +23,7 @@
>  &pinctrl_usdhc3 {
>  	bootph-pre-ram;
>  };
> +
> +&wdog2 {
> +	bootph-pre-ram;
> +};

Can all of this be upstreamed as well?

-- 
Tom

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]

Re: [PATCH v2 2/3] mx6sabresd: Convert to watchdog driver model

[Fabio Estevam]

On Fri, Feb 2, 2024 at 1:07 PM Tom Rini <trini@konsulko.com> wrote:

> Can all of this be upstreamed as well?

I can try it, but that would go into kernel 6.9 in the best case.

For now, let's keep it in u-boot.dtsi.

Re: [PATCH v2 2/3] mx6sabresd: Convert to watchdog driver model

[Tom Rini]

[-- Attachment #1: Type: text/plain, Size: 498 bytes --]

On Fri, Feb 02, 2024 at 01:17:06PM -0300, Fabio Estevam wrote:
> On Fri, Feb 2, 2024 at 1:07 PM Tom Rini <trini@konsulko.com> wrote:
> 
> > Can all of this be upstreamed as well?
> 
> I can try it, but that would go into kernel 6.9 in the best case.
> 
> For now, let's keep it in u-boot.dtsi.

Yes, sorry, I'm not saying don't do this here, but can you please submit
it so that long term this can be converted to OF_UPSTREAM and also not
have a -u-boot.dtsi potentially.

-- 
Tom

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]

[PATCH v2 3/3] mx6sabresd: Remove board_phy_config()

[Fabio Estevam]

From: Fabio Estevam <festevam@denx.de>

With Ethernet DM in place, there is no longer the need for having
the board_phy_config() anymore.
    
Remove it.

Confirmed that TFTP transfer still works fine without board_phy_config().

Signed-off-by: Fabio Estevam <festevam@denx.de>
---
Changes since v1:
- Newly introduced.

 board/freescale/mx6sabresd/mx6sabresd.c | 34 -------------------------
 1 file changed, 34 deletions(-)

diff --git a/board/freescale/mx6sabresd/mx6sabresd.c b/board/freescale/mx6sabresd/mx6sabresd.c
index b558a596dff8..bb066a5d36c3 100644
--- a/board/freescale/mx6sabresd/mx6sabresd.c
+++ b/board/freescale/mx6sabresd/mx6sabresd.c
@@ -7,7 +7,6 @@
 
 #include <image.h>
 #include <init.h>
-#include <net.h>
 #include <asm/arch/clock.h>
 #include <asm/arch/imx-regs.h>
 #include <asm/arch/iomux.h>
@@ -254,39 +253,6 @@ int board_mmc_init(struct bd_info *bis)
 }
 #endif
 
-static int ar8031_phy_fixup(struct phy_device *phydev)
-{
-	unsigned short val;
-
-	/* To enable AR8031 ouput a 125MHz clk from CLK_25M */
-	phy_write(phydev, MDIO_DEVAD_NONE, 0xd, 0x7);
-	phy_write(phydev, MDIO_DEVAD_NONE, 0xe, 0x8016);
-	phy_write(phydev, MDIO_DEVAD_NONE, 0xd, 0x4007);
-
-	val = phy_read(phydev, MDIO_DEVAD_NONE, 0xe);
-	val &= 0xffe3;
-	val |= 0x18;
-	phy_write(phydev, MDIO_DEVAD_NONE, 0xe, val);
-
-	/* introduce tx clock delay */
-	phy_write(phydev, MDIO_DEVAD_NONE, 0x1d, 0x5);
-	val = phy_read(phydev, MDIO_DEVAD_NONE, 0x1e);
-	val |= 0x0100;
-	phy_write(phydev, MDIO_DEVAD_NONE, 0x1e, val);
-
-	return 0;
-}
-
-int board_phy_config(struct phy_device *phydev)
-{
-	ar8031_phy_fixup(phydev);
-
-	if (phydev->drv->config)
-		phydev->drv->config(phydev);
-
-	return 0;
-}
-
 #if defined(CONFIG_VIDEO_IPUV3)
 static void disable_lvds(struct display_info_t const *dev)
 {
-- 
2.34.1

Re: [PATCH v2 3/3] mx6sabresd: Remove board_phy_config()

[Igor Opaniuk]

Hello Fabio,

On Fri, Feb 2, 2024 at 5:04 PM Fabio Estevam <festevam@gmail.com> wrote:
>
> From: Fabio Estevam <festevam@denx.de>
>
> With Ethernet DM in place, there is no longer the need for having
> the board_phy_config() anymore.
>
> Remove it.
>
> Confirmed that TFTP transfer still works fine without board_phy_config().
>
> Signed-off-by: Fabio Estevam <festevam@denx.de>
> ---
> Changes since v1:
> - Newly introduced.
>
>  board/freescale/mx6sabresd/mx6sabresd.c | 34 -------------------------
>  1 file changed, 34 deletions(-)
>
> diff --git a/board/freescale/mx6sabresd/mx6sabresd.c b/board/freescale/mx6sabresd/mx6sabresd.c
> index b558a596dff8..bb066a5d36c3 100644
> --- a/board/freescale/mx6sabresd/mx6sabresd.c
> +++ b/board/freescale/mx6sabresd/mx6sabresd.c
> @@ -7,7 +7,6 @@
>
>  #include <image.h>
>  #include <init.h>
> -#include <net.h>
>  #include <asm/arch/clock.h>
>  #include <asm/arch/imx-regs.h>
>  #include <asm/arch/iomux.h>
> @@ -254,39 +253,6 @@ int board_mmc_init(struct bd_info *bis)
>  }
>  #endif
>
> -static int ar8031_phy_fixup(struct phy_device *phydev)
> -{
> -       unsigned short val;
> -
> -       /* To enable AR8031 ouput a 125MHz clk from CLK_25M */
> -       phy_write(phydev, MDIO_DEVAD_NONE, 0xd, 0x7);
> -       phy_write(phydev, MDIO_DEVAD_NONE, 0xe, 0x8016);
> -       phy_write(phydev, MDIO_DEVAD_NONE, 0xd, 0x4007);
> -
> -       val = phy_read(phydev, MDIO_DEVAD_NONE, 0xe);
> -       val &= 0xffe3;
> -       val |= 0x18;
> -       phy_write(phydev, MDIO_DEVAD_NONE, 0xe, val);
> -
> -       /* introduce tx clock delay */
> -       phy_write(phydev, MDIO_DEVAD_NONE, 0x1d, 0x5);
> -       val = phy_read(phydev, MDIO_DEVAD_NONE, 0x1e);
> -       val |= 0x0100;
> -       phy_write(phydev, MDIO_DEVAD_NONE, 0x1e, val);
> -
> -       return 0;
> -}
> -
> -int board_phy_config(struct phy_device *phydev)
> -{
> -       ar8031_phy_fixup(phydev);
> -
> -       if (phydev->drv->config)
> -               phydev->drv->config(phydev);
> -
> -       return 0;
> -}
> -
>  #if defined(CONFIG_VIDEO_IPUV3)
>  static void disable_lvds(struct display_info_t const *dev)
>  {
> --
> 2.34.1
>

Reviewed-by: Igor Opaniuk <igor.opaniuk@foundries.io>

-- 
Best regards - Freundliche Grüsse - Meilleures salutations

Igor Opaniuk
Senior Software Engineer, Embedded & Security
E: igor.opaniuk@foundries.io
W: www.foundries.io

Re: [PATCH v2 1/3] mx6sabresd: Fix U-Boot corruption after saving the environment

[Tom Rini]

[-- Attachment #1: Type: text/plain, Size: 921 bytes --]

On Fri, Feb 02, 2024 at 01:04:03PM -0300, Fabio Estevam wrote:

> From: Fabio Estevam <festevam@denx.de>
> 
> U-Boot binary has grown in such a way that it goes beyond the reserved
> area for the environment variables.
>     
> Running "saveenv" and rebooting the board causes U-Boot to hang because
> of this overlap.
>     
> Fix this problem by selecting CONFIG_LTO so that the U-Boot proper
> size can be reduced.
>     
> Also, to prevent this same problem to happen in the future, use
> CONFIG_BOARD_SIZE_LIMIT, which can detect the overlap in build-time.
>     
> CONFIG_BOARD_SIZE_LIMIT is calculated as follows:
> 
> CONFIG_BOARD_SIZE_LIMIT = CONFIG_ENV_OFFSET - u-boot-img.dtb offset
> CONFIG_BOARD_SIZE_LIMIT = 0xc000 - 69 * 1024
> CONFIG_BOARD_SIZE_LIMIT = 715766
> 
> Signed-off-by: Fabio Estevam <festevam@denx.de>

Thanks!

Reviewed-by: Tom Rini <trini@konsulko.com>

-- 
Tom

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]

Re: [PATCH v2 1/3] mx6sabresd: Fix U-Boot corruption after saving the environment

[Igor Opaniuk]

On Fri, Feb 2, 2024 at 5:04 PM Fabio Estevam <festevam@gmail.com> wrote:
>
> From: Fabio Estevam <festevam@denx.de>
>
> U-Boot binary has grown in such a way that it goes beyond the reserved
> area for the environment variables.
>
> Running "saveenv" and rebooting the board causes U-Boot to hang because
> of this overlap.
>
> Fix this problem by selecting CONFIG_LTO so that the U-Boot proper
> size can be reduced.
>
> Also, to prevent this same problem to happen in the future, use
> CONFIG_BOARD_SIZE_LIMIT, which can detect the overlap in build-time.
>
> CONFIG_BOARD_SIZE_LIMIT is calculated as follows:
>
> CONFIG_BOARD_SIZE_LIMIT = CONFIG_ENV_OFFSET - u-boot-img.dtb offset
> CONFIG_BOARD_SIZE_LIMIT = 0xc000 - 69 * 1024
> CONFIG_BOARD_SIZE_LIMIT = 715766
>
> Signed-off-by: Fabio Estevam <festevam@denx.de>
> ---
> Changes since v1:
> - Select LTO. (Tom)
>
>  configs/mx6sabresd_defconfig | 3 +++
>  1 file changed, 3 insertions(+)
>
> diff --git a/configs/mx6sabresd_defconfig b/configs/mx6sabresd_defconfig
> index a90efe4a7786..d4fb55622cf1 100644
> --- a/configs/mx6sabresd_defconfig
> +++ b/configs/mx6sabresd_defconfig
> @@ -21,6 +21,9 @@ CONFIG_SPL_SERIAL=y
>  CONFIG_SPL=y
>  CONFIG_SPL_LIBDISK_SUPPORT=y
>  CONFIG_PCI=y
> +CONFIG_LTO=y
> +CONFIG_HAS_BOARD_SIZE_LIMIT=y
> +CONFIG_BOARD_SIZE_LIMIT=715766
>  CONFIG_FIT=y
>  CONFIG_SPL_FIT_PRINT=y
>  CONFIG_SPL_LOAD_FIT=y
> --
> 2.34.1
>
Reviewed-by: Igor Opaniuk <igor.opaniuk@foundries.io>

btw, from my experience, the same issue will probably occur again in future,
especially when the current U-Boot size is already close to the defined limit
(yes, with this patch it will be reported during compile time, but still).

Have you considered adjusting the boot image layout instead
(moving CONFIG_ENV_OFFSET, so unlikely
U-Boot image will overlap again) to tackle feature problems?

-- 
Best regards - Freundliche Grüsse - Meilleures salutations

Igor Opaniuk
Senior Software Engineer, Embedded & Security
E: igor.opaniuk@foundries.io
W: www.foundries.io

Re: [PATCH v2 1/3] mx6sabresd: Fix U-Boot corruption after saving the environment

[Igor Opaniuk]

On Fri, Feb 2, 2024 at 9:34 PM Igor Opaniuk <igor.opaniuk@foundries.io> wrote:
>
> On Fri, Feb 2, 2024 at 5:04 PM Fabio Estevam <festevam@gmail.com> wrote:
> >
> > From: Fabio Estevam <festevam@denx.de>
> >
> > U-Boot binary has grown in such a way that it goes beyond the reserved
> > area for the environment variables.
> >
> > Running "saveenv" and rebooting the board causes U-Boot to hang because
> > of this overlap.
> >
> > Fix this problem by selecting CONFIG_LTO so that the U-Boot proper
> > size can be reduced.
> >
> > Also, to prevent this same problem to happen in the future, use
> > CONFIG_BOARD_SIZE_LIMIT, which can detect the overlap in build-time.
> >
> > CONFIG_BOARD_SIZE_LIMIT is calculated as follows:
> >
> > CONFIG_BOARD_SIZE_LIMIT = CONFIG_ENV_OFFSET - u-boot-img.dtb offset
> > CONFIG_BOARD_SIZE_LIMIT = 0xc000 - 69 * 1024
> > CONFIG_BOARD_SIZE_LIMIT = 715766
> >
> > Signed-off-by: Fabio Estevam <festevam@denx.de>
> > ---
> > Changes since v1:
> > - Select LTO. (Tom)
> >
> >  configs/mx6sabresd_defconfig | 3 +++
> >  1 file changed, 3 insertions(+)
> >
> > diff --git a/configs/mx6sabresd_defconfig b/configs/mx6sabresd_defconfig
> > index a90efe4a7786..d4fb55622cf1 100644
> > --- a/configs/mx6sabresd_defconfig
> > +++ b/configs/mx6sabresd_defconfig
> > @@ -21,6 +21,9 @@ CONFIG_SPL_SERIAL=y
> >  CONFIG_SPL=y
> >  CONFIG_SPL_LIBDISK_SUPPORT=y
> >  CONFIG_PCI=y
> > +CONFIG_LTO=y
> > +CONFIG_HAS_BOARD_SIZE_LIMIT=y
> > +CONFIG_BOARD_SIZE_LIMIT=715766
> >  CONFIG_FIT=y
> >  CONFIG_SPL_FIT_PRINT=y
> >  CONFIG_SPL_LOAD_FIT=y
> > --
> > 2.34.1
> >
> Reviewed-by: Igor Opaniuk <igor.opaniuk@foundries.io>
>
> btw, from my experience, the same issue will probably occur again in future,
> especially when the current U-Boot size is already close to the defined limit
> (yes, with this patch it will be reported during compile time, but still).
>
> Have you considered adjusting the boot image layout instead
> (moving CONFIG_ENV_OFFSET, so unlikely
> U-Boot image will overlap again) to tackle feature problems?
I've just checked the previous patch version, where
you addressed that exactly with CONFIG_ENV_OFFSET change and
following suggestions/objections from Tom, so sorry for the noise :)

>
> --
> Best regards - Freundliche Grüsse - Meilleures salutations
>
> Igor Opaniuk
> Senior Software Engineer, Embedded & Security
> E: igor.opaniuk@foundries.io
> W: www.foundries.io


-- 
Best regards - Freundliche Grüsse - Meilleures salutations

Igor Opaniuk
Senior Software Engineer, Embedded & Security
E: igor.opaniuk@foundries.io
W: www.foundries.io

Re: [PATCH v2 1/3] mx6sabresd: Fix U-Boot corruption after saving the environment

[Fabio Estevam]

On Fri, Feb 2, 2024 at 1:04 PM Fabio Estevam <festevam@gmail.com> wrote:
>
> From: Fabio Estevam <festevam@denx.de>
>
> U-Boot binary has grown in such a way that it goes beyond the reserved
> area for the environment variables.
>
> Running "saveenv" and rebooting the board causes U-Boot to hang because
> of this overlap.
>
> Fix this problem by selecting CONFIG_LTO so that the U-Boot proper
> size can be reduced.
>
> Also, to prevent this same problem to happen in the future, use
> CONFIG_BOARD_SIZE_LIMIT, which can detect the overlap in build-time.
>
> CONFIG_BOARD_SIZE_LIMIT is calculated as follows:
>
> CONFIG_BOARD_SIZE_LIMIT = CONFIG_ENV_OFFSET - u-boot-img.dtb offset
> CONFIG_BOARD_SIZE_LIMIT = 0xc000 - 69 * 1024
> CONFIG_BOARD_SIZE_LIMIT = 715766
>
> Signed-off-by: Fabio Estevam <festevam@denx.de>

Applied all, thanks.