Re: [PATCH] arm: stm32mp15x: Move mmc aliases to board files

[Ahmad Fatoum <a.fatoum@pengutronix.de>]

Hello Uwe,

On 07.03.22 13:04, Uwe Kleine-König wrote:
> Hello Ahmad,
> 
> On Mon, Mar 07, 2022 at 12:34:19PM +0100, Ahmad Fatoum wrote:
>> On 07.03.22 12:23, Uwe Kleine-König wrote:
>>> Having the mmc aliases in stm32mp151.dtsi is surprising as depending on
>>> the order of includes these override the mmc ordering in
>>> <arm/stm32mp157c-myboard.dts>. Also the ordering of mmc devices is actually
>>> board specific, so it's also right to have this in the board.dts files.
>>
>> NACK.
> 
> I feared you'd oppose. :-\
> 
> My motivation to deviate from the default ordering is that I want to
> have eMMC as first device and SD as second, so on the board I have here
> I'd like to have
> 
> 	mmc0 = &sdmmc2; /* eMMC */
> 	mmc1 = &sdmmc1; /* µSD */
> 
> However in combination with arch/arm/dts/stm32mp151.dtsi this results in
> 
> 	mmc0 = &sdmmc2; /* eMMC */
> 	mmc1 = &sdmmc1; /* µSD */
> 	mmc2 = &sdmmc3;
> 
> which is a bit ugly because sdmmc3 isn't used at all on the board in
> question. Doing a /delete-property/mmc2 isn't that nice, too. Open for
> alternatives ...

Why though? Is this for compatibility with older/other hardware?

Renumbering is what I'd suggest though.

>> MMC IPs have fixed numbering, because TF-A (and BootROM before it)
>> report to barebox the number of the MMC device that it succesfully booted from.
>> The aliases map these IDs to device tree nodes, so barebox can fix up a correct
>> /chosen/bootsource.
> 
> And mapping the number from TF-A (or BootROM) depends on the aliases
> defined in the dts? Sounds like a bug to me.

We can change of_alias_get_id to lookup /aliases/barebox,$alias as a fallback.
Then add a new function that looks up barebox alias first and then non-barebox
adorned as fallback and use that for bootsource calculation. Afterwards, we can
prefix SoC level MMC aliases with barebox, and boards can specify their
aliases as they please. This may be the most straight-forward way
to decouple. Having mapping tables in SoC support is not my favorite.

>> Additionally having any alias at all ensures fixed naming that's
>> not dependent on probe order.
> 
> Fine for me. And if the board doesn't define the aliases, you get random
> ordering.

I prefer sane defaults.

>> I know that Linux maintainers seem to disagree with this, but as far
>> as barebox is concerned, aliases are SoC-specific, not board-specific
>> in general. You can override this board-level if you like, but the
>> default should remain.
>>
>>> There is no (relevant) change intended by this patch.
>>
>> I have non-upstream boards that would be broken by this.
> 
> This is not a reason to not take this patch, is it?

I think most people involved have boards (often with trivial board support)
that are not upstream. I do think we should avoid breaking them for no good
reason.

>> There's also a Phytec board in next that would be broken by this.
>> Whereas they had a fixed mmcX before, they would now have disk0, disk1
>> or disk2 depending on probe order with this patch applied.
> 
> Agreed, code in next should be adapted.
Cheers,
Ahmad

> 
> Best regards
> Uwe
> 


-- 
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-5555 |

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox