[PATCH v1] arm: dts: st: stm32mp151a-prtt1l: Fix QSPI configuration

[PATCH v1] arm: dts: st: stm32mp151a-prtt1l: Fix QSPI configuration

[Oleksij Rempel]

Rename 'pins1' to 'pins' in the qspi_bk1_pins_a node to correct the
subnode name. The previous name caused the configuration to be
applied to the wrong subnode, resulting in QSPI not working properly.

Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de>
---
 arch/arm/boot/dts/st/stm32mp151a-prtt1l.dtsi | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/arm/boot/dts/st/stm32mp151a-prtt1l.dtsi b/arch/arm/boot/dts/st/stm32mp151a-prtt1l.dtsi
index 3938d357e198f..4db684478c320 100644
--- a/arch/arm/boot/dts/st/stm32mp151a-prtt1l.dtsi
+++ b/arch/arm/boot/dts/st/stm32mp151a-prtt1l.dtsi
@@ -123,7 +123,7 @@ flash@0 {
 };
 
 &qspi_bk1_pins_a {
-	pins1 {
+	pins {
 		bias-pull-up;
 		drive-push-pull;
 		slew-rate = <1>;
-- 
2.39.2

Re: [PATCH v1] arm: dts: st: stm32mp151a-prtt1l: Fix QSPI configuration

[Ahmad Fatoum]

Hello Oleksij,

On 06.08.24 14:05, Oleksij Rempel wrote:
> Rename 'pins1' to 'pins' in the qspi_bk1_pins_a node to correct the
> subnode name. The previous name caused the configuration to be
> applied to the wrong subnode, resulting in QSPI not working properly.
> 
> Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de>
> ---
>  arch/arm/boot/dts/st/stm32mp151a-prtt1l.dtsi | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/arch/arm/boot/dts/st/stm32mp151a-prtt1l.dtsi b/arch/arm/boot/dts/st/stm32mp151a-prtt1l.dtsi
> index 3938d357e198f..4db684478c320 100644
> --- a/arch/arm/boot/dts/st/stm32mp151a-prtt1l.dtsi
> +++ b/arch/arm/boot/dts/st/stm32mp151a-prtt1l.dtsi
> @@ -123,7 +123,7 @@ flash@0 {
>  };
>  
>  &qspi_bk1_pins_a {
> -	pins1 {
> +	pins {

As you have seen such device tree overriding is error prone and would
be entirely avoidable if specifying full board-specific pinctrl groups
was allowed for the stm32 platforms instead of override-and-pray.

Anyways, there's better syntax for such overriding now:

  &{qspi_blk1_pins_a/pins}

which would cause a compilation error if pins was renamed again.

>  		bias-pull-up;

There's bias-disable in stm32mp15-pinctrl.dtsi. You may want to add
a /delete-property/ for that to make sure, it's not up to the driver
which one has priority.

>  		drive-push-pull;
>  		slew-rate = <1>;

These are already in qspi_bk1_pins_a. If repeating those is ok, why
not go a step further and just duplicate the pinmux property and stay
clear of this issue altogether, provided Alex is amenable to changing
his mind regarding pinctrl groups in board device trees.


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 |

Re: [PATCH v1] arm: dts: st: stm32mp151a-prtt1l: Fix QSPI configuration

[Alexandre TORGUE]

Hi

On 8/7/24 11:38, Ahmad Fatoum wrote:
> Hello Oleksij,
> 
> On 06.08.24 14:05, Oleksij Rempel wrote:
>> Rename 'pins1' to 'pins' in the qspi_bk1_pins_a node to correct the
>> subnode name. The previous name caused the configuration to be
>> applied to the wrong subnode, resulting in QSPI not working properly.
>>
>> Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de>
>> ---
>>   arch/arm/boot/dts/st/stm32mp151a-prtt1l.dtsi | 2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/arch/arm/boot/dts/st/stm32mp151a-prtt1l.dtsi b/arch/arm/boot/dts/st/stm32mp151a-prtt1l.dtsi
>> index 3938d357e198f..4db684478c320 100644
>> --- a/arch/arm/boot/dts/st/stm32mp151a-prtt1l.dtsi
>> +++ b/arch/arm/boot/dts/st/stm32mp151a-prtt1l.dtsi
>> @@ -123,7 +123,7 @@ flash@0 {
>>   };
>>   
>>   &qspi_bk1_pins_a {
>> -	pins1 {
>> +	pins {
> 
> As you have seen such device tree overriding is error prone and would
> be entirely avoidable if specifying full board-specific pinctrl groups
> was allowed for the stm32 platforms instead of override-and-pray.

You can create your own pin group in stm32mp15-pinctlr.dtsi. What is the 
issue ? Do I miss something ? It will avoid to overwrite an existing 
configuration

regards
alex


> Anyways, there's better syntax for such overriding now:
> 
>    &{qspi_blk1_pins_a/pins}
> 
> which would cause a compilation error if pins was renamed again.
> 
>>   		bias-pull-up;
> 
> There's bias-disable in stm32mp15-pinctrl.dtsi. You may want to add
> a /delete-property/ for that to make sure, it's not up to the driver
> which one has priority.
> 
>>   		drive-push-pull;
>>   		slew-rate = <1>;
> 
> These are already in qspi_bk1_pins_a. If repeating those is ok, why
> not go a step further and just duplicate the pinmux property and stay
> clear of this issue altogether, provided Alex is amenable to changing
> his mind regarding pinctrl groups in board device trees.
> 
> 
> Cheers,
> Ahmad
> 

Re: [PATCH v1] arm: dts: st: stm32mp151a-prtt1l: Fix QSPI configuration

[Oleksij Rempel]

Hi Alexandre,

On Tue, Aug 27, 2024 at 10:07:10AM +0200, Alexandre TORGUE wrote:
> Hi
> 
> On 8/7/24 11:38, Ahmad Fatoum wrote:
> > Hello Oleksij,
> > 
> > On 06.08.24 14:05, Oleksij Rempel wrote:
> > > Rename 'pins1' to 'pins' in the qspi_bk1_pins_a node to correct the
> > > subnode name. The previous name caused the configuration to be
> > > applied to the wrong subnode, resulting in QSPI not working properly.
> > > 
> > > Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de>
> > > ---
> > >   arch/arm/boot/dts/st/stm32mp151a-prtt1l.dtsi | 2 +-
> > >   1 file changed, 1 insertion(+), 1 deletion(-)
> > > 
> > > diff --git a/arch/arm/boot/dts/st/stm32mp151a-prtt1l.dtsi b/arch/arm/boot/dts/st/stm32mp151a-prtt1l.dtsi
> > > index 3938d357e198f..4db684478c320 100644
> > > --- a/arch/arm/boot/dts/st/stm32mp151a-prtt1l.dtsi
> > > +++ b/arch/arm/boot/dts/st/stm32mp151a-prtt1l.dtsi
> > > @@ -123,7 +123,7 @@ flash@0 {
> > >   };
> > >   &qspi_bk1_pins_a {
> > > -	pins1 {
> > > +	pins {
> > 
> > As you have seen such device tree overriding is error prone and would
> > be entirely avoidable if specifying full board-specific pinctrl groups
> > was allowed for the stm32 platforms instead of override-and-pray.
> 
> You can create your own pin group in stm32mp15-pinctlr.dtsi. What is the
> issue ? Do I miss something ? It will avoid to overwrite an existing
> configuration

There are mostly one one properly change (pull up), not really worth adding a
new sub node.

Regards,
Oleksij
-- 
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 |

Re: [PATCH v1] arm: dts: st: stm32mp151a-prtt1l: Fix QSPI configuration

[Alexandre TORGUE]

Hi Ahmad

On 8/7/24 11:38, Ahmad Fatoum wrote:
> Hello Oleksij,
> 
> On 06.08.24 14:05, Oleksij Rempel wrote:
>> Rename 'pins1' to 'pins' in the qspi_bk1_pins_a node to correct the
>> subnode name. The previous name caused the configuration to be
>> applied to the wrong subnode, resulting in QSPI not working properly.
>>
>> Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de>
>> ---
>>   arch/arm/boot/dts/st/stm32mp151a-prtt1l.dtsi | 2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/arch/arm/boot/dts/st/stm32mp151a-prtt1l.dtsi b/arch/arm/boot/dts/st/stm32mp151a-prtt1l.dtsi
>> index 3938d357e198f..4db684478c320 100644
>> --- a/arch/arm/boot/dts/st/stm32mp151a-prtt1l.dtsi
>> +++ b/arch/arm/boot/dts/st/stm32mp151a-prtt1l.dtsi
>> @@ -123,7 +123,7 @@ flash@0 {
>>   };
>>   
>>   &qspi_bk1_pins_a {
>> -	pins1 {
>> +	pins {
> 
> As you have seen such device tree overriding is error prone and would
> be entirely avoidable if specifying full board-specific pinctrl groups
> was allowed for the stm32 platforms instead of override-and-pray.
> 
> Anyways, there's better syntax for such overriding now:
> 
>    &{qspi_blk1_pins_a/pins}
> 
> which would cause a compilation error if pins was renamed again.
> 
>>   		bias-pull-up;
> 
> There's bias-disable in stm32mp15-pinctrl.dtsi. You may want to add
> a /delete-property/ for that to make sure, it's not up to the driver
> which one has priority.
> 
>>   		drive-push-pull;
>>   		slew-rate = <1>;
> 
> These are already in qspi_bk1_pins_a. If repeating those is ok, why
> not go a step further and just duplicate the pinmux property and stay
> clear of this issue altogether, provided Alex is amenable to changing
> his mind regarding pinctrl groups in board device trees.

I still prefer to have pin configuration defined in pinctrl dtsi file 
and I'll continue like this for ST board. The reason is that we try to 
reuse as much as possible pins when we create a new board and so it is 
easier to maintain if we declare them only one time.

But, I'm not blocked for "other" boards based on STM32 SoC. I mean, if 
it is simpler for you and above all if it avoid issues/complexities 
then, you can declare some pin groups in your board dts file. In this 
case we need to take care of the IO groups label name.

regards
alex

> 
> 
> Cheers,
> Ahmad
> 

Re: [PATCH v1] arm: dts: st: stm32mp151a-prtt1l: Fix QSPI configuration

[Ahmad Fatoum]

Hello Alex,

On 29.10.24 16:39, Alexandre TORGUE wrote:
> On 8/7/24 11:38, Ahmad Fatoum wrote:
>> Hello Oleksij,
>>
>> On 06.08.24 14:05, Oleksij Rempel wrote:
>> There's bias-disable in stm32mp15-pinctrl.dtsi. You may want to add
>> a /delete-property/ for that to make sure, it's not up to the driver
>> which one has priority.
>>
>>>           drive-push-pull;
>>>           slew-rate = <1>;
>>
>> These are already in qspi_bk1_pins_a. If repeating those is ok, why
>> not go a step further and just duplicate the pinmux property and stay
>> clear of this issue altogether, provided Alex is amenable to changing
>> his mind regarding pinctrl groups in board device trees.
> 
> I still prefer to have pin configuration defined in pinctrl dtsi file and I'll continue like this for ST board. The reason is that we try to reuse as much as possible pins when we create a new board and so it is easier to maintain if we declare them only one time.

I can see the point for ST evaluation kits as ST customer hardware
will often copy the reference designs.

> But, I'm not blocked for "other" boards based on STM32 SoC. I mean, if it is simpler for you and above all if it avoid issues/complexities then, you can declare some pin groups in your board dts file. In this case we need to take care of the IO groups label name.

That's good to hear and what I wanted to clarify. Especially for things like
ADCs, there are so many possible permutations that there is no point for
boards to add their pinctrl group to the main DTSI instead of just listing
their specific pin configuration in the board DTS.

Thanks,
Ahmad

> 
> regards
> alex
> 
>>
>>
>> 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 |