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

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

[Uwe Kleine-König]

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 = &ethernet0;
+		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

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

[Ahmad Fatoum]

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 = &ethernet0;
> +		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

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

[Uwe Kleine-König]

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

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

[Ahmad Fatoum]

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

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

[Uwe Kleine-König]

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

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

[Ahmad Fatoum]

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