* [PATCH] arm: stm32mp15x: Move mmc aliases to board files @ 2022-03-07 11:23 Uwe Kleine-König 2022-03-07 11:34 ` Ahmad Fatoum 0 siblings, 1 reply; 6+ messages in thread From: Uwe Kleine-König @ 2022-03-07 11:23 UTC (permalink / raw) To: barebox; +Cc: Ahmad Fatoum 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. There is no (relevant) change intended by this patch. Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de> --- arch/arm/dts/stm32mp151-prtt1l.dtsi | 3 ++- arch/arm/dts/stm32mp151.dtsi | 3 --- arch/arm/dts/stm32mp157c-ev1.dts | 6 ++++++ arch/arm/dts/stm32mp157c-odyssey.dts | 5 +++++ arch/arm/dts/stm32mp15xx-dkx.dtsi | 5 +++++ 5 files changed, 18 insertions(+), 4 deletions(-) diff --git a/arch/arm/dts/stm32mp151-prtt1l.dtsi b/arch/arm/dts/stm32mp151-prtt1l.dtsi index 80ae72dee22a..6813cb1767f2 100644 --- a/arch/arm/dts/stm32mp151-prtt1l.dtsi +++ b/arch/arm/dts/stm32mp151-prtt1l.dtsi @@ -16,8 +16,9 @@ }; aliases { - serial0 = &uart4; ethernet0 = ðernet0; + mmc0 = &sdmmc1; + serial0 = &uart4; }; v3v3: fixed-regulator-v3v3 { diff --git a/arch/arm/dts/stm32mp151.dtsi b/arch/arm/dts/stm32mp151.dtsi index f1fd888fa1c6..61b022c6caba 100644 --- a/arch/arm/dts/stm32mp151.dtsi +++ b/arch/arm/dts/stm32mp151.dtsi @@ -13,9 +13,6 @@ gpio9 = &gpioj; gpio10 = &gpiok; gpio25 = &gpioz; - mmc0 = &sdmmc1; - mmc1 = &sdmmc2; - mmc2 = &sdmmc3; pwm1 = &{/soc/timer@44000000/pwm}; pwm2 = &{/soc/timer@40000000/pwm}; pwm3 = &{/soc/timer@40001000/pwm}; diff --git a/arch/arm/dts/stm32mp157c-ev1.dts b/arch/arm/dts/stm32mp157c-ev1.dts index 742eca7a33c9..1718e847a1f4 100644 --- a/arch/arm/dts/stm32mp157c-ev1.dts +++ b/arch/arm/dts/stm32mp157c-ev1.dts @@ -4,6 +4,12 @@ #include "stm32mp151.dtsi" / { + aliases { + mmc0 = &sdmmc1; + mmc1 = &sdmmc2; + mmc2 = &sdmmc3; + }; + chosen { environment-sd { compatible = "barebox,environment"; diff --git a/arch/arm/dts/stm32mp157c-odyssey.dts b/arch/arm/dts/stm32mp157c-odyssey.dts index 0e395bdec961..8a91575b472b 100644 --- a/arch/arm/dts/stm32mp157c-odyssey.dts +++ b/arch/arm/dts/stm32mp157c-odyssey.dts @@ -7,6 +7,11 @@ #include "stm32mp151.dtsi" / { + aliases { + mmc0 = &sdmmc1; + mmc1 = &sdmmc2; + }; + chosen { environment-sd { compatible = "barebox,environment"; diff --git a/arch/arm/dts/stm32mp15xx-dkx.dtsi b/arch/arm/dts/stm32mp15xx-dkx.dtsi index 173e64e04c1b..edb37970331a 100644 --- a/arch/arm/dts/stm32mp15xx-dkx.dtsi +++ b/arch/arm/dts/stm32mp15xx-dkx.dtsi @@ -8,6 +8,11 @@ #include <dt-bindings/gpio/gpio.h> / { + aliases { + mmc0 = &sdmmc1; + mmc2 = &sdmmc3; + }; + chosen { environment { compatible = "barebox,environment"; base-commit: 383f9f2d5d8db19dce05c982fa7fbfb44989ef53 -- 2.34.1 _______________________________________________ barebox mailing list barebox@lists.infradead.org http://lists.infradead.org/mailman/listinfo/barebox ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] arm: stm32mp15x: Move mmc aliases to board files 2022-03-07 11:23 [PATCH] arm: stm32mp15x: Move mmc aliases to board files Uwe Kleine-König @ 2022-03-07 11:34 ` Ahmad Fatoum 2022-03-07 12:04 ` Uwe Kleine-König 0 siblings, 1 reply; 6+ messages in thread From: Ahmad Fatoum @ 2022-03-07 11:34 UTC (permalink / raw) To: Uwe Kleine-König, barebox Hi, 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. 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. Additionally having any alias at all ensures fixed naming that's not dependent on probe order. 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. 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. Cheers, Ahmad > > Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de> > --- > arch/arm/dts/stm32mp151-prtt1l.dtsi | 3 ++- > arch/arm/dts/stm32mp151.dtsi | 3 --- > arch/arm/dts/stm32mp157c-ev1.dts | 6 ++++++ > arch/arm/dts/stm32mp157c-odyssey.dts | 5 +++++ > arch/arm/dts/stm32mp15xx-dkx.dtsi | 5 +++++ > 5 files changed, 18 insertions(+), 4 deletions(-) > > diff --git a/arch/arm/dts/stm32mp151-prtt1l.dtsi b/arch/arm/dts/stm32mp151-prtt1l.dtsi > index 80ae72dee22a..6813cb1767f2 100644 > --- a/arch/arm/dts/stm32mp151-prtt1l.dtsi > +++ b/arch/arm/dts/stm32mp151-prtt1l.dtsi > @@ -16,8 +16,9 @@ > }; > > aliases { > - serial0 = &uart4; > ethernet0 = ðernet0; > + mmc0 = &sdmmc1; > + serial0 = &uart4; > }; > > v3v3: fixed-regulator-v3v3 { > diff --git a/arch/arm/dts/stm32mp151.dtsi b/arch/arm/dts/stm32mp151.dtsi > index f1fd888fa1c6..61b022c6caba 100644 > --- a/arch/arm/dts/stm32mp151.dtsi > +++ b/arch/arm/dts/stm32mp151.dtsi > @@ -13,9 +13,6 @@ > gpio9 = &gpioj; > gpio10 = &gpiok; > gpio25 = &gpioz; > - mmc0 = &sdmmc1; > - mmc1 = &sdmmc2; > - mmc2 = &sdmmc3; > pwm1 = &{/soc/timer@44000000/pwm}; > pwm2 = &{/soc/timer@40000000/pwm}; > pwm3 = &{/soc/timer@40001000/pwm}; > diff --git a/arch/arm/dts/stm32mp157c-ev1.dts b/arch/arm/dts/stm32mp157c-ev1.dts > index 742eca7a33c9..1718e847a1f4 100644 > --- a/arch/arm/dts/stm32mp157c-ev1.dts > +++ b/arch/arm/dts/stm32mp157c-ev1.dts > @@ -4,6 +4,12 @@ > #include "stm32mp151.dtsi" > > / { > + aliases { > + mmc0 = &sdmmc1; > + mmc1 = &sdmmc2; > + mmc2 = &sdmmc3; > + }; > + > chosen { > environment-sd { > compatible = "barebox,environment"; > diff --git a/arch/arm/dts/stm32mp157c-odyssey.dts b/arch/arm/dts/stm32mp157c-odyssey.dts > index 0e395bdec961..8a91575b472b 100644 > --- a/arch/arm/dts/stm32mp157c-odyssey.dts > +++ b/arch/arm/dts/stm32mp157c-odyssey.dts > @@ -7,6 +7,11 @@ > #include "stm32mp151.dtsi" > > / { > + aliases { > + mmc0 = &sdmmc1; > + mmc1 = &sdmmc2; > + }; > + > chosen { > environment-sd { > compatible = "barebox,environment"; > diff --git a/arch/arm/dts/stm32mp15xx-dkx.dtsi b/arch/arm/dts/stm32mp15xx-dkx.dtsi > index 173e64e04c1b..edb37970331a 100644 > --- a/arch/arm/dts/stm32mp15xx-dkx.dtsi > +++ b/arch/arm/dts/stm32mp15xx-dkx.dtsi > @@ -8,6 +8,11 @@ > #include <dt-bindings/gpio/gpio.h> > > / { > + aliases { > + mmc0 = &sdmmc1; > + mmc2 = &sdmmc3; > + }; > + > chosen { > environment { > compatible = "barebox,environment"; > > base-commit: 383f9f2d5d8db19dce05c982fa7fbfb44989ef53 -- 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 ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] arm: stm32mp15x: Move mmc aliases to board files 2022-03-07 11:34 ` Ahmad Fatoum @ 2022-03-07 12:04 ` Uwe Kleine-König 2022-03-07 12:19 ` Ahmad Fatoum 0 siblings, 1 reply; 6+ messages in thread From: Uwe Kleine-König @ 2022-03-07 12:04 UTC (permalink / raw) To: Ahmad Fatoum; +Cc: barebox [-- Attachment #1.1: Type: text/plain, Size: 2426 bytes --] 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 ... > 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. > 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 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? > 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. Best regards Uwe -- Pengutronix e.K. | Uwe Kleine-König | Industrial Linux Solutions | https://www.pengutronix.de/ | [-- Attachment #1.2: signature.asc --] [-- Type: application/pgp-signature, Size: 488 bytes --] [-- Attachment #2: Type: text/plain, Size: 149 bytes --] _______________________________________________ barebox mailing list barebox@lists.infradead.org http://lists.infradead.org/mailman/listinfo/barebox ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] arm: stm32mp15x: Move mmc aliases to board files 2022-03-07 12:04 ` Uwe Kleine-König @ 2022-03-07 12:19 ` Ahmad Fatoum 2022-03-07 13:23 ` Uwe Kleine-König 0 siblings, 1 reply; 6+ messages in thread From: Ahmad Fatoum @ 2022-03-07 12:19 UTC (permalink / raw) To: Uwe Kleine-König; +Cc: barebox 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 ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] arm: stm32mp15x: Move mmc aliases to board files 2022-03-07 12:19 ` Ahmad Fatoum @ 2022-03-07 13:23 ` Uwe Kleine-König 2022-03-11 9:31 ` Ahmad Fatoum 0 siblings, 1 reply; 6+ messages in thread From: Uwe Kleine-König @ 2022-03-07 13:23 UTC (permalink / raw) To: Ahmad Fatoum; +Cc: barebox [-- Attachment #1.1: Type: text/plain, Size: 3984 bytes --] Hello Ahmad, On Mon, Mar 07, 2022 at 01:19:27PM +0100, Ahmad Fatoum wrote: > On 07.03.22 13:04, Uwe Kleine-König wrote: > > 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? It's for compatibility with developer's mind, where numbering starts with the always available devices :-) > 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. I would have gone with the mapping tables and I'd consider /aliases/barebox,mmc0 = ... more ugly. But I agree this to be a bit subjective. If the soc-code continues to depend on having the mmc aliases as defined in stm32mp151.dtsi, there should be a comment describing that IMHO. > >> 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 prefer sane defaults iff they can be easily adapted by board code. If you consider in board.dts: mmc0 = &sdmmc2; mmc1 = &sdmmc3; (for whatever reasons), you end up with mmc1 and mmc2 both pointing to sdmmc3. Ugly. > >> 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. I think these people should mainline their trivial board support if they want to be immune to such breaking. Best regards Uwe -- Pengutronix e.K. | Uwe Kleine-König | Industrial Linux Solutions | https://www.pengutronix.de/ | [-- Attachment #1.2: signature.asc --] [-- Type: application/pgp-signature, Size: 488 bytes --] [-- Attachment #2: Type: text/plain, Size: 149 bytes --] _______________________________________________ barebox mailing list barebox@lists.infradead.org http://lists.infradead.org/mailman/listinfo/barebox ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] arm: stm32mp15x: Move mmc aliases to board files 2022-03-07 13:23 ` Uwe Kleine-König @ 2022-03-11 9:31 ` Ahmad Fatoum 0 siblings, 0 replies; 6+ messages in thread From: Ahmad Fatoum @ 2022-03-11 9:31 UTC (permalink / raw) To: Uwe Kleine-König; +Cc: barebox Hello Uwe, On 07.03.22 14:23, Uwe Kleine-König wrote: > I would have gone with the mapping tables and I'd consider > /aliases/barebox,mmc0 = ... more ugly. But I agree this to be > a bit subjective. Mapping table will be more effort IMO. At least the `barebox,` prefix change would be mechanical and less error-prone. > If the soc-code continues to depend on having the mmc > aliases as defined in stm32mp151.dtsi, there should be a comment > describing that IMHO. It's generic code that depends on the aliases for fixup. >>>> 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 prefer sane defaults iff they can be easily adapted by board code. If > you consider in board.dts: > > mmc0 = &sdmmc2; > mmc1 = &sdmmc3; > > (for whatever reasons), you end up with mmc1 and mmc2 both pointing to > sdmmc3. Ugly. /delete-property/ is easily adaptable IMO, but yes, not particularly nice to look at. >> 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. > > I think these people should mainline their trivial board support if they > want to be immune to such breaking. One could hope. :-) Cheers, Ahmad -- 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 ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2022-03-11 9:33 UTC | newest] Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2022-03-07 11:23 [PATCH] arm: stm32mp15x: Move mmc aliases to board files Uwe Kleine-König 2022-03-07 11:34 ` Ahmad Fatoum 2022-03-07 12:04 ` Uwe Kleine-König 2022-03-07 12:19 ` Ahmad Fatoum 2022-03-07 13:23 ` Uwe Kleine-König 2022-03-11 9:31 ` Ahmad Fatoum
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox