[U-Boot] [PATCH] configs: Remove CONFIG_SPL_FS_EXT4 for all MX6 platforms

Sun May 26 17:30:16 UTC 2019

On 5/26/19 6:18 PM, Ezequiel Garcia wrote:
> On Sun, 26 May 2019 at 12:05, Lukasz Majewski <lukma at denx.de> wrote:
>>
>> Hi Tom,
>>
>>> On Sun, May 26, 2019 at 01:46:53PM +0200, Lukasz Majewski wrote:
>>>> Hi Tom,
>>>>
>>>>> On Sun, May 26, 2019 at 10:22:00AM +0200, Lukasz Majewski wrote:
>>>>>> Dear Marek, Tom,
>>>>>>
>>>>>>> On 5/26/19 1:23 AM, Tom Rini wrote:
>>>>>>>> On Sun, May 26, 2019 at 01:20:34AM +0200, Marek Vasut
>>>>>>>> wrote:
>>>>>>>>> On 5/26/19 1:08 AM, Tom Rini wrote:
>>>>>>>>>> On Sun, May 26, 2019 at 12:57:08AM +0200, Marek Vasut
>>>>>>>>>> wrote:
>>>>>>>>>>> On 5/26/19 12:45 AM, Ezequiel Garcia wrote:
>>>>>>>>>>>> On Sun, 2019-05-26 at 00:24 +0200, Marek Vasut
>>>>>>>>>>>> wrote:
>>>>>>>>>>>>> On 5/25/19 11:47 PM, Ezequiel Garcia wrote:
>>>>>>>>>>>>>> On Sat, 2019-05-25 at 22:15 +0200, Marek Vasut
>>>>>>>>>>>>>> wrote:
>>>>>>>>>>>>>>> On 5/25/19 6:49 PM, Ezequiel Garcia wrote:
>>>>>>>>>>>>>>>> i.MX6 platforms boot U-Boot second-stage from
>>>>>>>>>>>>>>>> unformatted space, and should not need Ext
>>>>>>>>>>>>>>>> filesystem support on SPL.
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>> The commit was generated with:
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>> git grep -l MX6 -- configs/ | xargs grep -l
>>>>>>>>>>>>>>>> SPL_FS_EXT4 | xargs sed -i -e
>>>>>>>>>>>>>>>> '/CONFIG_SPL_FS_EXT4=y/d'
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>> This change has a dramatic impact on SPL size:
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>> ./scripts/bloat-o-meter old new
>>>>>>>>>>>>>>>> add/remove: 0/59 grow/shrink: 0/3 up/down: 0/-8674
>>>>>>>>>>>>>>>> (-8674) [..]
>>>>>>>>>>>>>>>> Total: Before=38320, After=29646, chg -22.64%
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>> Cc: Otavio Salvador <otavio at ossystems.com.br>
>>>>>>>>>>>>>>>> Cc: Fabio Estevam <fabio.estevam at nxp.com>
>>>>>>>>>>>>>>>> Cc: Peng Fan <peng.fan at nxp.com>
>>>>>>>>>>>>>>>> Cc: Marek Vasut <marex at denx.de>
>>>>>>>>>>>>>>>> Cc: Stefano Babic <sbabic at denx.de>
>>>>>>>>>>>>>>>> Cc: Stefan Roese <sr at denx.de>
>>>>>>>>>>>>>>>> Cc: "Eric Bénard" <eric at eukrea.com>
>>>>>>>>>>>>>>>> Cc: Breno Lima <breno.lima at nxp.com>
>>>>>>>>>>>>>>>> Cc: Francesco Montefoschi
>>>>>>>>>>>>>>>> <francesco.montefoschi at udoo.org> Signed-off-by:
>>>>>>>>>>>>>>>> Ezequiel Garcia <ezequiel at collabora.com> ---
>>>>>>>>>>>>>>>> Tested on Wandboard only. Maintainers, please ACK or
>>>>>>>>>>>>>>>> NAK!
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>  configs/cgtqmx6eval_defconfig       | 1 -
>>>>>>>>>>>>>>>>  configs/mx6cuboxi_defconfig         | 1 -
>>>>>>>>>>>>>>>>  configs/mx6sabreauto_defconfig      | 1 -
>>>>>>>>>>>>>>>>  configs/mx6sabresd_defconfig        | 1 -
>>>>>>>>>>>>>>>>  configs/mx6slevk_spl_defconfig      | 1 -
>>>>>>>>>>>>>>>>  configs/mx6sxsabresd_spl_defconfig  | 1 -
>>>>>>>>>>>>>>>>  configs/mx6ul_14x14_evk_defconfig   | 1 -
>>>>>>>>>>>>>>>>  configs/mx6ul_9x9_evk_defconfig     | 1 -
>>>>>>>>>>>>>>>>  configs/novena_defconfig            | 1 -
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> NAK, I boot my Novena from ext4 and this just broke
>>>>>>>>>>>>>>> it.
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> And also, NAK, this removes functionality from SPL
>>>>>>>>>>>>>>> which worked fine before.
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> I'll drop from Novena, but I think the patch still
>>>>>>>>>>>>>> makes some sense, why do you want Ext4 on SPL?
>>>>>>>>>>>>>
>>>>>>>>>>>>> What other filesystem is available in SPL and doesn't
>>>>>>>>>>>>> have patent problems ?
>>>>>>>>>>>>
>>>>>>>>>>>> Sorry for not being clear. I am asking why turn on a
>>>>>>>>>>>> feature that is so heavy, on a system that won't need
>>>>>>>>>>>> it (such as Sabre* boards, Wandboard and similar)?
>>>>>>>>>>>
>>>>>>>>>>> Two reasons:
>>>>>>>>>>> 1) It was enabled, disabling it means removing
>>>>>>>>>>> functionality for no good reason (oops, bloat, is not a
>>>>>>>>>>> good reason), and that is not desired. 2) Booting from
>>>>>>>>>>> block device implies booting from a filesystem,
>>>>>>>>>>> otherwise you might overwrite various things on the
>>>>>>>>>>> block device when updating the file (u-boot image).
>>>>>>>>>>
>>>>>>>>>> So, are you using SPL to load something from ext4 or
>>>>>>>>>> not?
>>>>>>>>>
>>>>>>>>> Yes, I think that's what I said.
>>>>>>>>>
>>>>>>>>>> There are
>>>>>>>>>> setups where people have configured the system such that
>>>>>>>>>> SPL loads something from ext4 and that's why we have it
>>>>>>>>>> available.  Is anyone doing that on Novena?  Or any iMX
>>>>>>>>>> system?
>>>>>>>>>
>>>>>>>>> Quoting my first response in this thread:
>>>>>>>>> "
>>>>>>>>> NAK, I boot my Novena from ext4 and this just broke it.
>>>>>>>>> "
>>>>>>>>
>>>>>>>> Actually, I wasn't sure from your first response if you're
>>>>>>>> using SPL to load u-boot from EXT4 or not.  So, Novena is a
>>>>>>>> no and we should wait for more board maintainers to speak
>>>>>>>> up to see if they use it or not, thanks!
>>>>>>>
>>>>>>> Novena is certainly a no. Since I use a couple of wandboards,
>>>>>>> those are no as well.
>>>>>>>
>>>>>>> But I do not want to see useful functionality removed from SPL
>>>>>>> only to make space for useless DM/DT bloat. Temporarily
>>>>>>> band-aiding this real problem by removing functionality is a
>>>>>>> no-go, no matter how you slice it. The real fix is to fix the
>>>>>>> DM/DT and figure out a way to reduce it's size and _retain_
>>>>>>> _all_ the functionality.
>>>>>>
>>>>>> I fully agree with Marek here - the DM/DT SPL support (in the
>>>>>> form as it is now) is a bloat. The SPL size increases
>>>>>> considerably.
>>>>>>
>>>>>> The _real_ issue is the lack of OF_PLATDATA support for DM/DTS
>>>>>> drivers, which are used when we convert u-boot to DM/DTS.
>>>>>>
>>>>>> For boards, which have enough internal on-chip RAM (OCRAM in
>>>>>> case of imx6q) it is doable to convert them to DM/DTS in SPL.
>>>>>>
>>>>>> However, nobody wants to say it loud, but the footprint
>>>>>> considerably increases in SPL - for example:
>>>>>>
>>>>>> SPL (and I only use eMMC there - no SPI, no I2C):
>>>>>> ------------------------------------------------
>>>>>> Before conversion:                              35 KiB
>>>>>> DM conversion:                                  47 KiB
>>>>>> (around ~25% increase)
>>>>>>
>>>>>>
>>>>>> For u-boot proper:
>>>>>> ----------------------------
>>>>>> Footprint changes (u-boot.imx):
>>>>>>
>>>>>> Before conversion:                              345 KiB
>>>>>> DM conversion:                    415 KiB
>>>>>> (around + 17% increase)
>>>>>>
>>>>>>
>>>>>> This has several implications:
>>>>>>
>>>>>> 1. We replace small, robust SPL (I speak only for i.MX) with
>>>>>> something which is XX% larger and hence boot time increases. In
>>>>>> short - customers are/will not be happy. We do introduce
>>>>>> regression here.
>>>>>> fdtdec_get_chosen_node
>>>>>> 2. To make it usable (with the size comparable to what we do
>>>>>> have now) we need to add OF_PLATDATA support to eMMC, SPI, I2C,
>>>>>> etc. drivers. Test them and validate.
>>>>>>
>>>>>> That is why we now "just" convert only U-Boot proper to DM/DTS.
>>>>>>
>>>>>> As a follow up - it seems pragmatic to not touch SPL for now,
>>>>>> and start the conversion ONLY when necessary drivers gain
>>>>>> support for OF_PLATDATA (so we don't need DTS support in SPL).
>>>>>>
>>>>>> Unfortunately, it would take some time.
>>>>>
>>>>> Right, and again, DM support in SPL is not required as we haven't
>>>>> sorted out some of the issues such as making sure it is still
>>>>> small enough.
>>>>
>>>> Ok, so we do have agreement here. Good.
>>>>
>>>>>
>>>>> Which is why I'm quite frustrated at the moment about "just now"
>>>>> starting to convert these things for i.MX rather than several
>>>>> years ago,
>>>>
>>>> Frankly speaking - I do prefer to use older, well tested and stable
>>>> SW.
>>>>
>>>> And such situation we had with i.MX port. It was (and still is)
>>>> working, being there before the whole DM/DTS stuff.
>>>>
>>>> I can understand that we advised new ports/archs to use DM/DTS. But
>>>> i.MX has a lot of legacy code, which is already deployed. Changing
>>>> it because "we move to DM/DTS" immediately is not practical as we
>>>> loose the code base developed, tested and validated for years.
>>>
>>> DM/DT has been around for over 7 years now, so it too is "legacy
>>> code". So it's not "immediately", it's been several years now of
>>> trying to get people to migrate and introducing more firm deadlines
>>> about it.
>>
>> I'm not talking about generic DM/DTS adoption - this goes pretty well.
>>
>> I'm specific about OF_PLATDATA in SPL, which is lagging behind in the
>> whole U-Boot.
>> To efficiently convert SPL on i.MX we do need it.
>>
>>>
>>> And I'm _still_ not removing code that's close to a full year past the
>>> published "We're going to remove this now" date.
>>
>> And this is a good approach. This code is a legacy one, but still
>> working and sometimes used.
>>
>>>
>>>>> when the first "hey, this might be too big for some platforms"
>>>>> conversations started and we changed from "OF_PLATDATA is a
>>>>> stop-gap" to "OF_PLATDATA is a required feature or we'll never be
>>>>> small enough".
>>>>
>>>> Please consider that we even now have issues with some non-converted
>>>> drivers. Some drivers were "under conversion" for years (spi
>>>> framework, sf command). Please also note how many drivers actually
>>>> support OF_PLATDATA?
>>>
>>> Yes, it's been hard to get some drivers converted because just not
>>> doing it had no consequences.  And yes, I'm sure drivers you need
>>> will need OF_PLATDATA hooks added.
>>
>> Yes, certainly.
>>
>>>
>>>>> And it's still unrelated to removing unused features from a build,
>>>>> which this thread is about.  If platforms aren't loading U-Boot
>>>>> from EXT4, that's just unused bloat.
>>>>
>>>> I do agree with the above sentence. If the feature is not used - it
>>>> shall be removed. However, if somebody deployed it already - then -
>>>> well it must be supported or other, acceptable solution needs to be
>>>> used (e.g. EXT4 -> VFAT conversion).
>>>
>>> Exactly.  If people are using EXT4 on iMX6 then it should stay.  We
>>> dropped it from am335x_evm over a year ago because it wasn't used.
>>>
>>
>> Yes. I also do agree.
>>
> 
> I don't want to get into the DM discussion, the intention of this patch was
> to reduce SPL size, not make room for a future increase.

SPL size should not be reduced at the expense of useful functionality
though.

> That said, I am surprised it is so controversial to slim down a Defconfig,
> which (at least in my mind) is just the "Maintainer Recommended Starting
> Point". By removing Ext4 we stating that it's no needed for the board
> "typical" use.

Maybe you can remove VFAT first, why ext4 in the first place ?

> Of course, anyone can still turn Ext4 -and many other features- on.

I was under the impression that mainline should be as useful as
possible. Maybe I'm mistaken and carrying random downstream patches is
now the suggested approach ?

> Once again, this is only possible if we maintain SPL "starting point"
> size as small as possible, which probably excludes DM or any other
> form of bloat.
> 
> In fact, if I can keep removing features and still boot the board with
> the typical setup, I'll be happy to keep suggesting such patches.

That's fine, unless it breaks other peoples' existing setups .

-- 
Best regards,
Marek Vasut