[PATCH] mx6qsabrelite: increase offset for environment on SPI

[PATCH] mx6qsabrelite: increase offset for environment on SPI

[Denis Pynkin]

The size of the binary created with the default U-boot config is much
greater than the default offset for environment `0x60000`. If the new
version is flashed onto SPI it overlaps with the stored environment.
This leads to U-Boot corruption in case of saving environment onto
SPI and non-bootable SabreLite board.

Signed-off-by: Denis Pynkin <denis.pynkin@collabora.com>
---
 configs/mx6qsabrelite_defconfig | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/configs/mx6qsabrelite_defconfig b/configs/mx6qsabrelite_defconfig
index 008fcfe04c..6e7193e411 100644
--- a/configs/mx6qsabrelite_defconfig
+++ b/configs/mx6qsabrelite_defconfig
@@ -4,7 +4,7 @@ CONFIG_SYS_TEXT_BASE=0x17800000
 CONFIG_SYS_MALLOC_F_LEN=0x4000
 CONFIG_NR_DRAM_BANKS=1
 CONFIG_ENV_SIZE=0x2000
-CONFIG_ENV_OFFSET=0x60000
+CONFIG_ENV_OFFSET=0xC0000
 CONFIG_MX6Q=y
 CONFIG_TARGET_NITROGEN6X=y
 CONFIG_DM_GPIO=y
-- 
2.25.4

[PATCH] mx6qsabrelite: increase offset for environment on SPI

[Stefano Babic]

On 14.09.20 11:56, Denis Pynkin wrote:
> The size of the binary created with the default U-boot config is much
> greater than the default offset for environment `0x60000`. If the new
> version is flashed onto SPI it overlaps with the stored environment.
> This leads to U-Boot corruption in case of saving environment onto
> SPI and non-bootable SabreLite board.
> 
> Signed-off-by: Denis Pynkin <denis.pynkin@collabora.com>
> ---
>  configs/mx6qsabrelite_defconfig | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/configs/mx6qsabrelite_defconfig b/configs/mx6qsabrelite_defconfig
> index 008fcfe04c..6e7193e411 100644
> --- a/configs/mx6qsabrelite_defconfig
> +++ b/configs/mx6qsabrelite_defconfig
> @@ -4,7 +4,7 @@ CONFIG_SYS_TEXT_BASE=0x17800000
>  CONFIG_SYS_MALLOC_F_LEN=0x4000
>  CONFIG_NR_DRAM_BANKS=1
>  CONFIG_ENV_SIZE=0x2000
> -CONFIG_ENV_OFFSET=0x60000
> +CONFIG_ENV_OFFSET=0xC0000
>  CONFIG_MX6Q=y
>  CONFIG_TARGET_NITROGEN6X=y
>  CONFIG_DM_GPIO=y
> 

Reviewed-by: Stefano Babic <sbabic@denx.de>

Best regards,
Stefano Babic

-- 
=====================================================================
DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: +49-8142-66989-53 Fax: +49-8142-66989-80 Email: sbabic at denx.de
=====================================================================

[PATCH] mx6qsabrelite: increase offset for environment on SPI

[Fabio Estevam]

Hi Denis,

On Mon, Sep 14, 2020 at 6:56 AM Denis Pynkin <denis.pynkin@collabora.com> wrote:
>
> The size of the binary created with the default U-boot config is much
> greater than the default offset for environment `0x60000`. If the new
> version is flashed onto SPI it overlaps with the stored environment.
> This leads to U-Boot corruption in case of saving environment onto
> SPI and non-bootable SabreLite board.
>
> Signed-off-by: Denis Pynkin <denis.pynkin@collabora.com>

Reviewed-by: Fabio Estevam <festevam@gmail.com>

In case you want to detect errors like this again in the future, you could add
CONFIG_BOARD_SIZE_LIMIT that detects such overlaps in build-time.

Please check commit 033f6ea5fa5f ("mx53loco: Fix U-Boot corruption
after saving the environment")

The addition of CONFIG_BOARD_SIZE_LIMIT can be a separate patch though.

[PATCH] mx6qsabrelite: increase offset for environment on SPI

[Tom Rini]

On Mon, Sep 14, 2020 at 08:03:33AM -0300, Fabio Estevam wrote:
> Hi Denis,
> 
> On Mon, Sep 14, 2020 at 6:56 AM Denis Pynkin <denis.pynkin@collabora.com> wrote:
> >
> > The size of the binary created with the default U-boot config is much
> > greater than the default offset for environment `0x60000`. If the new
> > version is flashed onto SPI it overlaps with the stored environment.
> > This leads to U-Boot corruption in case of saving environment onto
> > SPI and non-bootable SabreLite board.
> >
> > Signed-off-by: Denis Pynkin <denis.pynkin@collabora.com>
> 
> Reviewed-by: Fabio Estevam <festevam@gmail.com>
> 
> In case you want to detect errors like this again in the future, you could add
> CONFIG_BOARD_SIZE_LIMIT that detects such overlaps in build-time.
> 
> Please check commit 033f6ea5fa5f ("mx53loco: Fix U-Boot corruption
> after saving the environment")
> 
> The addition of CONFIG_BOARD_SIZE_LIMIT can be a separate patch though.

Hold on.  Can we shrink the board back down so that we don't need to
move the environment?  Moving the environment is bad as it will also
break existing users.  Thanks!

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 659 bytes
Desc: not available
URL: <https://lists.denx.de/pipermail/u-boot/attachments/20200914/76207c23/attachment.sig>

[PATCH] mx6qsabrelite: increase offset for environment on SPI

[Denis Pynkin]

14.09.2020 15:33, Tom Rini wrote?:
> On Mon, Sep 14, 2020 at 08:03:33AM -0300, Fabio Estevam wrote:

>>> The size of the binary created with the default U-boot config is much
>>> greater than the default offset for environment `0x60000`. If the new
>>> version is flashed onto SPI it overlaps with the stored environment.
>>> This leads to U-Boot corruption in case of saving environment onto
>>> SPI and non-bootable SabreLite board.
>>>
>>> Signed-off-by: Denis Pynkin <denis.pynkin@collabora.com>
>>
>> Reviewed-by: Fabio Estevam <festevam@gmail.com>
>>
>> In case you want to detect errors like this again in the future, you could add
>> CONFIG_BOARD_SIZE_LIMIT that detects such overlaps in build-time.
>>
>> Please check commit 033f6ea5fa5f ("mx53loco: Fix U-Boot corruption
>> after saving the environment")
>>
>> The addition of CONFIG_BOARD_SIZE_LIMIT can be a separate patch though.
> 
> Hold on.  Can we shrink the board back down so that we don't need to
> move the environment?  Moving the environment is bad as it will also
> break existing users.  Thanks!

I don't think so to be honest.
Current sizes even for `u-boot-nodtb.imx` in my builds are:
- 595308 (0x9156C) -- cross-compilation
- 470780 (0x72EFC) -- native build
which is much larger than current offset 393216 (0x60000)

The offset for environment `CONFIG_ENV_OFFSET=0x60000` were added in
commit a09fea1 just a year ago.
Not sure if it was tested with SabreLite board tbh -- this is a bulk
commit aimed for ARMv8 and sizes of binaries produced with pre-a09fea1
commit are also larger than the current offset.

-- 
wbr, Denis

[PATCH] mx6qsabrelite: increase offset for environment on SPI

[Tom Rini]

On Mon, Sep 14, 2020 at 04:20:20PM +0300, Denis Pynkin wrote:
> 14.09.2020 15:33, Tom Rini wrote?:
> > On Mon, Sep 14, 2020 at 08:03:33AM -0300, Fabio Estevam wrote:
> 
> >>> The size of the binary created with the default U-boot config is much
> >>> greater than the default offset for environment `0x60000`. If the new
> >>> version is flashed onto SPI it overlaps with the stored environment.
> >>> This leads to U-Boot corruption in case of saving environment onto
> >>> SPI and non-bootable SabreLite board.
> >>>
> >>> Signed-off-by: Denis Pynkin <denis.pynkin@collabora.com>
> >>
> >> Reviewed-by: Fabio Estevam <festevam@gmail.com>
> >>
> >> In case you want to detect errors like this again in the future, you could add
> >> CONFIG_BOARD_SIZE_LIMIT that detects such overlaps in build-time.
> >>
> >> Please check commit 033f6ea5fa5f ("mx53loco: Fix U-Boot corruption
> >> after saving the environment")
> >>
> >> The addition of CONFIG_BOARD_SIZE_LIMIT can be a separate patch though.
> > 
> > Hold on.  Can we shrink the board back down so that we don't need to
> > move the environment?  Moving the environment is bad as it will also
> > break existing users.  Thanks!
> 
> I don't think so to be honest.
> Current sizes even for `u-boot-nodtb.imx` in my builds are:
> - 595308 (0x9156C) -- cross-compilation
> - 470780 (0x72EFC) -- native build

I'd like to have one thread where we see what on earth is going on
there.  That's a rather crazy size difference and very troubling.

> which is much larger than current offset 393216 (0x60000)
> 
> The offset for environment `CONFIG_ENV_OFFSET=0x60000` were added in
> commit a09fea1 just a year ago.
> Not sure if it was tested with SabreLite board tbh -- this is a bulk
> commit aimed for ARMv8 and sizes of binaries produced with pre-a09fea1
> commit are also larger than the current offset.

Ah, so here's what's going on then.  Commit a09fea1 wasn't about ARMv8,
it was about migrating options to the defconfig files.  In this case, it
should have been set to the value you're changing it to now, at least
from a read of the patch (include/configs/mx6sabre_common.h would set it
to 0xC0000 if CONFIG_ENV_IS_IN_MMC), but I thought I had done that
migration with my hack to make a tool that had the in-use value be
printed.  So I'm going to re-check that whole thing to see what else
might be wrong as well.  Thanks!

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 659 bytes
Desc: not available
URL: <https://lists.denx.de/pipermail/u-boot/attachments/20200914/8ada3a7b/attachment.sig>

[PATCH] mx6qsabrelite: increase offset for environment on SPI

[Tom Rini]

On Mon, Sep 14, 2020 at 09:29:18AM -0400, Tom Rini wrote:
> On Mon, Sep 14, 2020 at 04:20:20PM +0300, Denis Pynkin wrote:
> > 14.09.2020 15:33, Tom Rini wrote?:
> > > On Mon, Sep 14, 2020 at 08:03:33AM -0300, Fabio Estevam wrote:
> > 
> > >>> The size of the binary created with the default U-boot config is much
> > >>> greater than the default offset for environment `0x60000`. If the new
> > >>> version is flashed onto SPI it overlaps with the stored environment.
> > >>> This leads to U-Boot corruption in case of saving environment onto
> > >>> SPI and non-bootable SabreLite board.
> > >>>
> > >>> Signed-off-by: Denis Pynkin <denis.pynkin@collabora.com>
> > >>
> > >> Reviewed-by: Fabio Estevam <festevam@gmail.com>
> > >>
> > >> In case you want to detect errors like this again in the future, you could add
> > >> CONFIG_BOARD_SIZE_LIMIT that detects such overlaps in build-time.
> > >>
> > >> Please check commit 033f6ea5fa5f ("mx53loco: Fix U-Boot corruption
> > >> after saving the environment")
> > >>
> > >> The addition of CONFIG_BOARD_SIZE_LIMIT can be a separate patch though.
> > > 
> > > Hold on.  Can we shrink the board back down so that we don't need to
> > > move the environment?  Moving the environment is bad as it will also
> > > break existing users.  Thanks!
> > 
> > I don't think so to be honest.
> > Current sizes even for `u-boot-nodtb.imx` in my builds are:
> > - 595308 (0x9156C) -- cross-compilation
> > - 470780 (0x72EFC) -- native build
> 
> I'd like to have one thread where we see what on earth is going on
> there.  That's a rather crazy size difference and very troubling.
> 
> > which is much larger than current offset 393216 (0x60000)
> > 
> > The offset for environment `CONFIG_ENV_OFFSET=0x60000` were added in
> > commit a09fea1 just a year ago.
> > Not sure if it was tested with SabreLite board tbh -- this is a bulk
> > commit aimed for ARMv8 and sizes of binaries produced with pre-a09fea1
> > commit are also larger than the current offset.
> 
> Ah, so here's what's going on then.  Commit a09fea1 wasn't about ARMv8,
> it was about migrating options to the defconfig files.  In this case, it
> should have been set to the value you're changing it to now, at least
> from a read of the patch (include/configs/mx6sabre_common.h would set it
> to 0xC0000 if CONFIG_ENV_IS_IN_MMC), but I thought I had done that
> migration with my hack to make a tool that had the in-use value be
> printed.  So I'm going to re-check that whole thing to see what else
> might be wrong as well.  Thanks!

Ah, so, mx6qsabrelite falls in to the "nitrogen6x" family and not the
rest of the "sabre" board families.  As such, it's always had the env
offset of 0x60000.  Jumping back somewhat arbitrarily to v2014.10, I see
with gcc-4.9 a size of 404480 on u-boot.imx which is still bigger.

The commit message isn't clear however, as environment is in MMC and not
SPI, so SPI booting should be fine.  But MMC/eMMC is where this case can
happen (I'm not sure which device is SD slot and which is eMMC on this
hardware off-hand).  Thanks!

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 659 bytes
Desc: not available
URL: <https://lists.denx.de/pipermail/u-boot/attachments/20200914/190444d7/attachment.sig>

[PATCH] mx6qsabrelite: increase offset for environment on SPI

[Denis Pynkin]

14.09.2020 17:11, Tom Rini wrote:
> On Mon, Sep 14, 2020 at 09:29:18AM -0400, Tom Rini wrote:
>> On Mon, Sep 14, 2020 at 04:20:20PM +0300, Denis Pynkin wrote:
>>> 14.09.2020 15:33, Tom Rini wrote?:
>>>> On Mon, Sep 14, 2020 at 08:03:33AM -0300, Fabio Estevam wrote:
>>>
>>>>>> The size of the binary created with the default U-boot config is much
>>>>>> greater than the default offset for environment `0x60000`. If the new
>>>>>> version is flashed onto SPI it overlaps with the stored environment.
>>>>>> This leads to U-Boot corruption in case of saving environment onto
>>>>>> SPI and non-bootable SabreLite board.
>>>>>>
>>>>>> Signed-off-by: Denis Pynkin <denis.pynkin@collabora.com>
>>>>>
>>>>> Reviewed-by: Fabio Estevam <festevam@gmail.com>
>>>>>
>>>>> In case you want to detect errors like this again in the future, you could add
>>>>> CONFIG_BOARD_SIZE_LIMIT that detects such overlaps in build-time.
>>>>>
>>>>> Please check commit 033f6ea5fa5f ("mx53loco: Fix U-Boot corruption
>>>>> after saving the environment")
>>>>>
>>>>> The addition of CONFIG_BOARD_SIZE_LIMIT can be a separate patch though.
>>>>
>>>> Hold on.  Can we shrink the board back down so that we don't need to
>>>> move the environment?  Moving the environment is bad as it will also
>>>> break existing users.  Thanks!
>>>
>>> I don't think so to be honest.
>>> Current sizes even for `u-boot-nodtb.imx` in my builds are:
>>> - 595308 (0x9156C) -- cross-compilation
>>> - 470780 (0x72EFC) -- native build
>>
>> I'd like to have one thread where we see what on earth is going on
>> there.  That's a rather crazy size difference and very troubling.
>>
>>> which is much larger than current offset 393216 (0x60000)
>>>
>>> The offset for environment `CONFIG_ENV_OFFSET=0x60000` were added in
>>> commit a09fea1 just a year ago.
>>> Not sure if it was tested with SabreLite board tbh -- this is a bulk
>>> commit aimed for ARMv8 and sizes of binaries produced with pre-a09fea1
>>> commit are also larger than the current offset.
>>
>> Ah, so here's what's going on then.  Commit a09fea1 wasn't about ARMv8,
>> it was about migrating options to the defconfig files.  In this case, it
>> should have been set to the value you're changing it to now, at least
>> from a read of the patch (include/configs/mx6sabre_common.h would set it
>> to 0xC0000 if CONFIG_ENV_IS_IN_MMC), but I thought I had done that
>> migration with my hack to make a tool that had the in-use value be
>> printed.  So I'm going to re-check that whole thing to see what else
>> might be wrong as well.  Thanks!
> 
> Ah, so, mx6qsabrelite falls in to the "nitrogen6x" family and not the
> rest of the "sabre" board families.  As such, it's always had the env
> offset of 0x60000.  Jumping back somewhat arbitrarily to v2014.10, I see
> with gcc-4.9 a size of 404480 on u-boot.imx which is still bigger.
> 
> The commit message isn't clear however, as environment is in MMC and not
> SPI, so SPI booting should be fine.  But MMC/eMMC is where this case can
> happen (I'm not sure which device is SD slot and which is eMMC on this
> hardware off-hand).  Thanks!

To my shame -- I didn't even thought about the booting from MMC, but yes
-- should be the same issue in case if U-Boot is placed on SD-card.

This device have 2MB SPI NOR flash there the U-Boot could be flashed
with an environment. We are using this approach, so I attempt to
describe the issue from this point of view.

Should I try to re-word the description? Or may I ask you to add a
better description?

-- 
wbr, Denis

[PATCH] mx6qsabrelite: increase offset for environment on SPI

[Tom Rini]

On Mon, Sep 14, 2020 at 08:08:48PM +0300, Denis Pynkin wrote:
> 14.09.2020 17:11, Tom Rini wrote:
> > On Mon, Sep 14, 2020 at 09:29:18AM -0400, Tom Rini wrote:
> >> On Mon, Sep 14, 2020 at 04:20:20PM +0300, Denis Pynkin wrote:
> >>> 14.09.2020 15:33, Tom Rini wrote?:
> >>>> On Mon, Sep 14, 2020 at 08:03:33AM -0300, Fabio Estevam wrote:
> >>>
> >>>>>> The size of the binary created with the default U-boot config is much
> >>>>>> greater than the default offset for environment `0x60000`. If the new
> >>>>>> version is flashed onto SPI it overlaps with the stored environment.
> >>>>>> This leads to U-Boot corruption in case of saving environment onto
> >>>>>> SPI and non-bootable SabreLite board.
> >>>>>>
> >>>>>> Signed-off-by: Denis Pynkin <denis.pynkin@collabora.com>
> >>>>>
> >>>>> Reviewed-by: Fabio Estevam <festevam@gmail.com>
> >>>>>
> >>>>> In case you want to detect errors like this again in the future, you could add
> >>>>> CONFIG_BOARD_SIZE_LIMIT that detects such overlaps in build-time.
> >>>>>
> >>>>> Please check commit 033f6ea5fa5f ("mx53loco: Fix U-Boot corruption
> >>>>> after saving the environment")
> >>>>>
> >>>>> The addition of CONFIG_BOARD_SIZE_LIMIT can be a separate patch though.
> >>>>
> >>>> Hold on.  Can we shrink the board back down so that we don't need to
> >>>> move the environment?  Moving the environment is bad as it will also
> >>>> break existing users.  Thanks!
> >>>
> >>> I don't think so to be honest.
> >>> Current sizes even for `u-boot-nodtb.imx` in my builds are:
> >>> - 595308 (0x9156C) -- cross-compilation
> >>> - 470780 (0x72EFC) -- native build
> >>
> >> I'd like to have one thread where we see what on earth is going on
> >> there.  That's a rather crazy size difference and very troubling.
> >>
> >>> which is much larger than current offset 393216 (0x60000)
> >>>
> >>> The offset for environment `CONFIG_ENV_OFFSET=0x60000` were added in
> >>> commit a09fea1 just a year ago.
> >>> Not sure if it was tested with SabreLite board tbh -- this is a bulk
> >>> commit aimed for ARMv8 and sizes of binaries produced with pre-a09fea1
> >>> commit are also larger than the current offset.
> >>
> >> Ah, so here's what's going on then.  Commit a09fea1 wasn't about ARMv8,
> >> it was about migrating options to the defconfig files.  In this case, it
> >> should have been set to the value you're changing it to now, at least
> >> from a read of the patch (include/configs/mx6sabre_common.h would set it
> >> to 0xC0000 if CONFIG_ENV_IS_IN_MMC), but I thought I had done that
> >> migration with my hack to make a tool that had the in-use value be
> >> printed.  So I'm going to re-check that whole thing to see what else
> >> might be wrong as well.  Thanks!
> > 
> > Ah, so, mx6qsabrelite falls in to the "nitrogen6x" family and not the
> > rest of the "sabre" board families.  As such, it's always had the env
> > offset of 0x60000.  Jumping back somewhat arbitrarily to v2014.10, I see
> > with gcc-4.9 a size of 404480 on u-boot.imx which is still bigger.
> > 
> > The commit message isn't clear however, as environment is in MMC and not
> > SPI, so SPI booting should be fine.  But MMC/eMMC is where this case can
> > happen (I'm not sure which device is SD slot and which is eMMC on this
> > hardware off-hand).  Thanks!
> 
> To my shame -- I didn't even thought about the booting from MMC, but yes
> -- should be the same issue in case if U-Boot is placed on SD-card.
> 
> This device have 2MB SPI NOR flash there the U-Boot could be flashed
> with an environment. We are using this approach, so I attempt to
> describe the issue from this point of view.
> 
> Should I try to re-word the description? Or may I ask you to add a
> better description?

Well, there's a few things going on here then.  The defconfig has
ENV_IS_IN_MMC, not ENV_IS_IN_SPI.  So SPI booting should be fine we're
putting U-Boot and environment on entirely different media.  There's no
conflict.  Booting from the first MMC device can be broken as that's
where we say the environment should be and 'saveenv' will scribble all
over it.  So can you confirm what the breaking condition you see is?
The commit message talks about SPI, but SPI should work as-is.

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 659 bytes
Desc: not available
URL: <https://lists.denx.de/pipermail/u-boot/attachments/20200914/b1a8d569/attachment.sig>

[PATCH] mx6qsabrelite: increase offset for environment on SPI

[Denis Pynkin]

14.09.2020 21:50, Tom Rini wrote:
> On Mon, Sep 14, 2020 at 08:08:48PM +0300, Denis Pynkin wrote:
>> 14.09.2020 17:11, Tom Rini wrote:
>>> On Mon, Sep 14, 2020 at 09:29:18AM -0400, Tom Rini wrote:
>>>> On Mon, Sep 14, 2020 at 04:20:20PM +0300, Denis Pynkin wrote:
>>>>> 14.09.2020 15:33, Tom Rini wrote?:
>>>>>> On Mon, Sep 14, 2020 at 08:03:33AM -0300, Fabio Estevam wrote:
>>>>>
>>>>>>>> The size of the binary created with the default U-boot config is much
>>>>>>>> greater than the default offset for environment `0x60000`. If the new
>>>>>>>> version is flashed onto SPI it overlaps with the stored environment.
>>>>>>>> This leads to U-Boot corruption in case of saving environment onto
>>>>>>>> SPI and non-bootable SabreLite board.
>>>>>>>>
>>>>>>>> Signed-off-by: Denis Pynkin <denis.pynkin@collabora.com>
>>>>>>>
>>>>>>> Reviewed-by: Fabio Estevam <festevam@gmail.com>
>>>>>>>
>>>>>>> In case you want to detect errors like this again in the future, you could add
>>>>>>> CONFIG_BOARD_SIZE_LIMIT that detects such overlaps in build-time.
>>>>>>>
>>>>>>> Please check commit 033f6ea5fa5f ("mx53loco: Fix U-Boot corruption
>>>>>>> after saving the environment")
>>>>>>>
>>>>>>> The addition of CONFIG_BOARD_SIZE_LIMIT can be a separate patch though.
>>>>>>
>>>>>> Hold on.  Can we shrink the board back down so that we don't need to
>>>>>> move the environment?  Moving the environment is bad as it will also
>>>>>> break existing users.  Thanks!
>>>>>
>>>>> I don't think so to be honest.
>>>>> Current sizes even for `u-boot-nodtb.imx` in my builds are:
>>>>> - 595308 (0x9156C) -- cross-compilation
>>>>> - 470780 (0x72EFC) -- native build
>>>>
>>>> I'd like to have one thread where we see what on earth is going on
>>>> there.  That's a rather crazy size difference and very troubling.
>>>>
>>>>> which is much larger than current offset 393216 (0x60000)
>>>>>
>>>>> The offset for environment `CONFIG_ENV_OFFSET=0x60000` were added in
>>>>> commit a09fea1 just a year ago.
>>>>> Not sure if it was tested with SabreLite board tbh -- this is a bulk
>>>>> commit aimed for ARMv8 and sizes of binaries produced with pre-a09fea1
>>>>> commit are also larger than the current offset.
>>>>
>>>> Ah, so here's what's going on then.  Commit a09fea1 wasn't about ARMv8,
>>>> it was about migrating options to the defconfig files.  In this case, it
>>>> should have been set to the value you're changing it to now, at least
>>>> from a read of the patch (include/configs/mx6sabre_common.h would set it
>>>> to 0xC0000 if CONFIG_ENV_IS_IN_MMC), but I thought I had done that
>>>> migration with my hack to make a tool that had the in-use value be
>>>> printed.  So I'm going to re-check that whole thing to see what else
>>>> might be wrong as well.  Thanks!
>>>
>>> Ah, so, mx6qsabrelite falls in to the "nitrogen6x" family and not the
>>> rest of the "sabre" board families.  As such, it's always had the env
>>> offset of 0x60000.  Jumping back somewhat arbitrarily to v2014.10, I see
>>> with gcc-4.9 a size of 404480 on u-boot.imx which is still bigger.
>>>
>>> The commit message isn't clear however, as environment is in MMC and not
>>> SPI, so SPI booting should be fine.  But MMC/eMMC is where this case can
>>> happen (I'm not sure which device is SD slot and which is eMMC on this
>>> hardware off-hand).  Thanks!
>>
>> To my shame -- I didn't even thought about the booting from MMC, but yes
>> -- should be the same issue in case if U-Boot is placed on SD-card.
>>
>> This device have 2MB SPI NOR flash there the U-Boot could be flashed
>> with an environment. We are using this approach, so I attempt to
>> describe the issue from this point of view.
>>
>> Should I try to re-word the description? Or may I ask you to add a
>> better description?
> 
> Well, there's a few things going on here then.  The defconfig has
> ENV_IS_IN_MMC, not ENV_IS_IN_SPI.  So SPI booting should be fine we're
> putting U-Boot and environment on entirely different media.  There's no
> conflict.  Booting from the first MMC device can be broken as that's
> where we say the environment should be and 'saveenv' will scribble all
> over it.  So can you confirm what the breaking condition you see is?
> The commit message talks about SPI, but SPI should work as-is.

You are absolutely right.
My fault -- copy-paste the patch from my project as is without
additional testing.
Did an additional tests with default value 0x60000:
- works well with the combination U-Boot on SPI NOR flash + environment
on MMC
- corrupted U-Boot on MMC after `saveenv` if used U-Boot on MMC
- corrupted U-Boot after `saveenv` if it is configured
  with `CONFIG_ENV_IS_IN_SPI_FLASH=y` and flashed on SPI

The value `0xC0000` fixes the issue for both corrupted cases.

Thanks a lot for pointing to incorrect description and misunderstanding!

-- 
wbr, Denis

[PATCH v2] mx6qsabrelite: increase the environment offset

[Denis Pynkin]

The size of the binary created with the default U-boot config is much
greater than the default offset for environment `0x60000`.
In case if that binary is used for booting via MMC it is overlapped with
the environment stored on MMC.
This leads to U-Boot corruption while saving environment with `saveenv`
command and non-bootable SabreLite board.

The offset for environment `CONFIG_ENV_OFFSET=0x60000` was added in
commit a09fea1 but did not count in the change to `0xC0000` if option
`CONFIG_ENV_IS_IN_MMC` is used.

The offset is also used for variant with environment saving onto SPI NOR
flash (with enabled option `CONFIG_ENV_IS_IN_SPI_FLASH`). In that case the
U-Boot binary flashed on SPI NOR is also corrupted after environment
saving with the original 0x60000 offset.

Signed-off-by: Denis Pynkin <denis.pynkin@collabora.com>
---
 configs/mx6qsabrelite_defconfig | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/configs/mx6qsabrelite_defconfig b/configs/mx6qsabrelite_defconfig
index 008fcfe04c..6e7193e411 100644
--- a/configs/mx6qsabrelite_defconfig
+++ b/configs/mx6qsabrelite_defconfig
@@ -4,7 +4,7 @@ CONFIG_SYS_TEXT_BASE=0x17800000
 CONFIG_SYS_MALLOC_F_LEN=0x4000
 CONFIG_NR_DRAM_BANKS=1
 CONFIG_ENV_SIZE=0x2000
-CONFIG_ENV_OFFSET=0x60000
+CONFIG_ENV_OFFSET=0xC0000
 CONFIG_MX6Q=y
 CONFIG_TARGET_NITROGEN6X=y
 CONFIG_DM_GPIO=y
-- 
2.25.4

[PATCH v2] mx6qsabrelite: increase the environment offset

[Tom Rini]

On Tue, Sep 15, 2020 at 02:37:14PM +0300, Denis Pynkin wrote:

> The size of the binary created with the default U-boot config is much
> greater than the default offset for environment `0x60000`.
> In case if that binary is used for booting via MMC it is overlapped with
> the environment stored on MMC.
> This leads to U-Boot corruption while saving environment with `saveenv`
> command and non-bootable SabreLite board.
> 
> The offset for environment `CONFIG_ENV_OFFSET=0x60000` was added in
> commit a09fea1 but did not count in the change to `0xC0000` if option
> `CONFIG_ENV_IS_IN_MMC` is used.
> 
> The offset is also used for variant with environment saving onto SPI NOR
> flash (with enabled option `CONFIG_ENV_IS_IN_SPI_FLASH`). In that case the
> U-Boot binary flashed on SPI NOR is also corrupted after environment
> saving with the original 0x60000 offset.
> 
> Signed-off-by: Denis Pynkin <denis.pynkin@collabora.com>

Thanks for rewording.

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

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 659 bytes
Desc: not available
URL: <https://lists.denx.de/pipermail/u-boot/attachments/20200915/0ce4521e/attachment.sig>

[PATCH v2] mx6qsabrelite: increase the environment offset

[sbabic at denx.de]

> The size of the binary created with the default U-boot config is much
> greater than the default offset for environment `0x60000`.
> In case if that binary is used for booting via MMC it is overlapped with
> the environment stored on MMC.
> This leads to U-Boot corruption while saving environment with `saveenv`
> command and non-bootable SabreLite board.
> The offset for environment `CONFIG_ENV_OFFSET=0x60000` was added in
> commit a09fea1 but did not count in the change to `0xC0000` if option
> `CONFIG_ENV_IS_IN_MMC` is used.
> The offset is also used for variant with environment saving onto SPI NOR
> flash (with enabled option `CONFIG_ENV_IS_IN_SPI_FLASH`). In that case the
> U-Boot binary flashed on SPI NOR is also corrupted after environment
> saving with the original 0x60000 offset.
> Signed-off-by: Denis Pynkin <denis.pynkin@collabora.com>
> Reviewed-by: Tom Rini <trini@konsulko.com>
Applied to u-boot-imx, master, thanks !

Best regards,
Stefano Babic

-- 
=====================================================================
DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: +49-8142-66989-53 Fax: +49-8142-66989-80 Email: sbabic at denx.de
=====================================================================