[PATCH] ARM: dts: stm32: Consistently enable internal pull-ups for SD bus

[PATCH] ARM: dts: stm32: Consistently enable internal pull-ups for SD bus

[Marek Vasut]

The default state of SD bus and clock line is logical HI. SD card IO is
open-drain and pulls the bus lines LO. Always enable the SD bus pull ups
to guarantee this behavior. Note that on systems with bus voltage level
shifter on the SD bus, the pull ups might also be built into the level
shifter, however that should have no negative impact.

Signed-off-by: Marek Vasut <marex@denx.de>
Cc: Alexandre Torgue <alexandre.torgue@st.com>
Cc: Maxime Coquelin <mcoquelin.stm32@gmail.com>
Cc: Patrice Chotard <patrice.chotard@st.com>
Cc: Patrick Delaunay <patrick.delaunay@st.com>
Cc: linux-stm32@st-md-mailman.stormreply.com
To: linux-arm-kernel@lists.infradead.org
---
 arch/arm/boot/dts/stm32mp15-pinctrl.dtsi | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/arch/arm/boot/dts/stm32mp15-pinctrl.dtsi b/arch/arm/boot/dts/stm32mp15-pinctrl.dtsi
index b2d19583450c..73d9a5b7f5ba 100644
--- a/arch/arm/boot/dts/stm32mp15-pinctrl.dtsi
+++ b/arch/arm/boot/dts/stm32mp15-pinctrl.dtsi
@@ -1291,13 +1291,13 @@ pins1 {
 				 <STM32_PINMUX('D', 2, AF12)>; /* SDMMC1_CMD */
 			slew-rate = <1>;
 			drive-push-pull;
-			bias-disable;
+			bias-pull-up;
 		};
 		pins2 {
 			pinmux = <STM32_PINMUX('C', 12, AF12)>; /* SDMMC1_CK */
 			slew-rate = <2>;
 			drive-push-pull;
-			bias-disable;
+			bias-pull-up;
 		};
 	};
 
@@ -1447,13 +1447,13 @@ pins1 {
 				 <STM32_PINMUX('G', 6, AF10)>; /* SDMMC2_CMD */
 			slew-rate = <1>;
 			drive-push-pull;
-			bias-disable;
+			bias-pull-up;
 		};
 		pins2 {
 			pinmux = <STM32_PINMUX('E', 3, AF9)>; /* SDMMC2_CK */
 			slew-rate = <2>;
 			drive-push-pull;
-			bias-disable;
+			bias-pull-up;
 		};
 	};
 
@@ -1510,7 +1510,7 @@ pins {
 				 <STM32_PINMUX('C', 7, AF10)>; /* SDMMC2_D7 */
 			slew-rate = <1>;
 			drive-push-pull;
-			bias-disable;
+			bias-pull-up;
 		};
 	};
 
-- 
2.28.0


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [Linux-stm32] [PATCH] ARM: dts: stm32: Consistently enable internal pull-ups for SD bus

[Yann GAUTIER]

On 10/9/20 11:08 PM, Marek Vasut wrote:
> The default state of SD bus and clock line is logical HI. SD card IO is
> open-drain and pulls the bus lines LO. Always enable the SD bus pull ups
> to guarantee this behavior. Note that on systems with bus voltage level
> shifter on the SD bus, the pull ups might also be built into the level
> shifter, however that should have no negative impact.
> 
> Signed-off-by: Marek Vasut <marex@denx.de>
> Cc: Alexandre Torgue <alexandre.torgue@st.com>
> Cc: Maxime Coquelin <mcoquelin.stm32@gmail.com>
> Cc: Patrice Chotard <patrice.chotard@st.com>
> Cc: Patrick Delaunay <patrick.delaunay@st.com>
> Cc: linux-stm32@st-md-mailman.stormreply.com
> To: linux-arm-kernel@lists.infradead.org
> ---
>   arch/arm/boot/dts/stm32mp15-pinctrl.dtsi | 10 +++++-----
>   1 file changed, 5 insertions(+), 5 deletions(-)
> 
> diff --git a/arch/arm/boot/dts/stm32mp15-pinctrl.dtsi b/arch/arm/boot/dts/stm32mp15-pinctrl.dtsi
> index b2d19583450c..73d9a5b7f5ba 100644
> --- a/arch/arm/boot/dts/stm32mp15-pinctrl.dtsi
> +++ b/arch/arm/boot/dts/stm32mp15-pinctrl.dtsi
> @@ -1291,13 +1291,13 @@ pins1 {
>   				 <STM32_PINMUX('D', 2, AF12)>; /* SDMMC1_CMD */
>   			slew-rate = <1>;
>   			drive-push-pull;
> -			bias-disable;
> +			bias-pull-up;
Hi Marek,

This pin config is used by ST board, where we have a level shifter.
This shouldn't be changed. We discussed this with Alex, and a new group 
should be added in this case.

>   		};
>   		pins2 {
>   			pinmux = <STM32_PINMUX('C', 12, AF12)>; /* SDMMC1_CK */
>   			slew-rate = <2>;
>   			drive-push-pull;
> -			bias-disable;
> +			bias-pull-up;
Same case for this one.
>   		};
>   	};
>   
> @@ -1447,13 +1447,13 @@ pins1 {
>   				 <STM32_PINMUX('G', 6, AF10)>; /* SDMMC2_CMD */
>   			slew-rate = <1>;
>   			drive-push-pull;
> -			bias-disable;
> +			bias-pull-up;
This node sdmmc2_b4_pins_b should be used by ST board DK2, which has 
external pull-ups. So this shouldn't be changed as well.
>   		};
>   		pins2 {
>   			pinmux = <STM32_PINMUX('E', 3, AF9)>; /* SDMMC2_CK */
>   			slew-rate = <2>;
>   			drive-push-pull;
> -			bias-disable;
> +			bias-pull-up;
Same here.
>   		};
>   	};
>   
> @@ -1510,7 +1510,7 @@ pins {
>   				 <STM32_PINMUX('C', 7, AF10)>; /* SDMMC2_D7 */
>   			slew-rate = <1>;
>   			drive-push-pull;
> -			bias-disable;
> +			bias-pull-up;
This one is also used on Linux Automation MC-1 board, Ahmad, is it OK 
for you?


Best regards,
Yann
>   		};
>   	};
>   
> 
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [Linux-stm32] [PATCH] ARM: dts: stm32: Consistently enable internal pull-ups for SD bus

[Marek Vasut]

On 10/14/20 3:26 PM, Yann GAUTIER wrote:
[...]
>> diff --git a/arch/arm/boot/dts/stm32mp15-pinctrl.dtsi b/arch/arm/boot/dts/stm32mp15-pinctrl.dtsi
>> index b2d19583450c..73d9a5b7f5ba 100644
>> --- a/arch/arm/boot/dts/stm32mp15-pinctrl.dtsi
>> +++ b/arch/arm/boot/dts/stm32mp15-pinctrl.dtsi
>> @@ -1291,13 +1291,13 @@ pins1 {
>>   				 <STM32_PINMUX('D', 2, AF12)>; /* SDMMC1_CMD */
>>   			slew-rate = <1>;
>>   			drive-push-pull;
>> -			bias-disable;
>> +			bias-pull-up;
> Hi Marek,

Hi,

> This pin config is used by ST board, where we have a level shifter.
> This shouldn't be changed. We discussed this with Alex, and a new group 
> should be added in this case.

Is it a problem if we enable the pulls up unconditionally with the level
shifter present, to make the properties of the SD bus consistent ?

>>   		};
>>   		pins2 {
>>   			pinmux = <STM32_PINMUX('C', 12, AF12)>; /* SDMMC1_CK */
>>   			slew-rate = <2>;
>>   			drive-push-pull;
>> -			bias-disable;
>> +			bias-pull-up;
> Same case for this one.
>>   		};
>>   	};

[...]

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [Linux-stm32] [PATCH] ARM: dts: stm32: Consistently enable internal pull-ups for SD bus

[Yann GAUTIER]

On 10/14/20 3:30 PM, Marek Vasut wrote:
> On 10/14/20 3:26 PM, Yann GAUTIER wrote:
> [...]
>>> diff --git a/arch/arm/boot/dts/stm32mp15-pinctrl.dtsi b/arch/arm/boot/dts/stm32mp15-pinctrl.dtsi
>>> index b2d19583450c..73d9a5b7f5ba 100644
>>> --- a/arch/arm/boot/dts/stm32mp15-pinctrl.dtsi
>>> +++ b/arch/arm/boot/dts/stm32mp15-pinctrl.dtsi
>>> @@ -1291,13 +1291,13 @@ pins1 {
>>>    				 <STM32_PINMUX('D', 2, AF12)>; /* SDMMC1_CMD */
>>>    			slew-rate = <1>;
>>>    			drive-push-pull;
>>> -			bias-disable;
>>> +			bias-pull-up;
>> Hi Marek,
> 
> Hi,
> 
>> This pin config is used by ST board, where we have a level shifter.
>> This shouldn't be changed. We discussed this with Alex, and a new group
>> should be added in this case.
> 
> Is it a problem if we enable the pulls up unconditionally with the level
> shifter present, to make the properties of the SD bus consistent ?
> 
The risk of having 2 parallel pull-ups is that the resulting value could 
be below the lower acceptable value for SD. I'll check if that can occur.


Yann
>>>    		};
>>>    		pins2 {
>>>    			pinmux = <STM32_PINMUX('C', 12, AF12)>; /* SDMMC1_CK */
>>>    			slew-rate = <2>;
>>>    			drive-push-pull;
>>> -			bias-disable;
>>> +			bias-pull-up;
>> Same case for this one.
>>>    		};
>>>    	};
> 
> [...]
> 
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [Linux-stm32] [PATCH] ARM: dts: stm32: Consistently enable internal pull-ups for SD bus

[Marek Vasut]

On 10/14/20 4:52 PM, Yann GAUTIER wrote:
> 
> 
> On 10/14/20 3:30 PM, Marek Vasut wrote:
>> On 10/14/20 3:26 PM, Yann GAUTIER wrote:
>> [...]
>>>> diff --git a/arch/arm/boot/dts/stm32mp15-pinctrl.dtsi b/arch/arm/boot/dts/stm32mp15-pinctrl.dtsi
>>>> index b2d19583450c..73d9a5b7f5ba 100644
>>>> --- a/arch/arm/boot/dts/stm32mp15-pinctrl.dtsi
>>>> +++ b/arch/arm/boot/dts/stm32mp15-pinctrl.dtsi
>>>> @@ -1291,13 +1291,13 @@ pins1 {
>>>>    				 <STM32_PINMUX('D', 2, AF12)>; /* SDMMC1_CMD */
>>>>    			slew-rate = <1>;
>>>>    			drive-push-pull;
>>>> -			bias-disable;
>>>> +			bias-pull-up;
>>> Hi Marek,
>>
>> Hi,
>>
>>> This pin config is used by ST board, where we have a level shifter.
>>> This shouldn't be changed. We discussed this with Alex, and a new group
>>> should be added in this case.
>>
>> Is it a problem if we enable the pulls up unconditionally with the level
>> shifter present, to make the properties of the SD bus consistent ?
>>
> The risk of having 2 parallel pull-ups is that the resulting value could 
> be below the lower acceptable value for SD. I'll check if that can occur.

That's a good idea, thank you!

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [Linux-stm32] [PATCH] ARM: dts: stm32: Consistently enable internal pull-ups for SD bus

[Ahmad Fatoum]

Hello Yann,

thanks for pinging me.

On 10/14/20 3:26 PM, Yann GAUTIER wrote:
> On 10/9/20 11:08 PM, Marek Vasut wrote:
>> @@ -1510,7 +1510,7 @@ pins {
>>   				 <STM32_PINMUX('C', 7, AF10)>; /* SDMMC2_D7 */
>>   			slew-rate = <1>;
>>   			drive-push-pull;
>> -			bias-disable;
>> +			bias-pull-up;
> This one is also used on Linux Automation MC-1 board, Ahmad, is it OK 
> for you?

Hello Marek,

We already have 47K external pull-ups on all the SDMMC2's data lanes and we
don't need the SoC-internal pull-up there as well.

On the SDMMC1 we lack them, so they were added in the board DTS:

  /* stm32mp157c-lxa-mc1.dts */
  &sdmmc1_b4_pins_a {
          /*
           * board lacks external pull-ups on SDMMC lines. Class 10 SD refuses to
           * work, thus enable internal pull-ups.
           */
          pins1 {
                  /delete-property/ bias-disable;
                  bias-pull-up;
          };
          pins2 {
                  /delete-property/ bias-disable;
                  bias-pull-up;
          };
  };

I don't mind the central pinctrl settings changed to make them more widely applicable
if current users get such override nodes inserted to maintain their existing settings.

(My favorite would of course be to allow board DTS to just keep their own pinctrl
 nodes outside stm32mp15-pinctrl.dtsi.)

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 |

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [Linux-stm32] [PATCH] ARM: dts: stm32: Consistently enable internal pull-ups for SD bus

[Marek Vasut]

On 10/15/20 2:44 PM, Ahmad Fatoum wrote:

Hi,

[...]

> We already have 47K external pull-ups on all the SDMMC2's data lanes and we
> don't need the SoC-internal pull-up there as well.
> 
> On the SDMMC1 we lack them, so they were added in the board DTS:

You do need pullups on SD bus by default. Currently the
stm32mp15-pinmux.dtsi is not consistent in that aspect. I am not arguing
about any one single board, but about the dtsi, I suspect by default the
pull ups should be enabled, and possibly on board-DT-level they should
be disabled if not needed instead ?

[...]

> I don't mind the central pinctrl settings changed to make them more widely applicable
> if current users get such override nodes inserted to maintain their existing settings.
> 
> (My favorite would of course be to allow board DTS to just keep their own pinctrl
>  nodes outside stm32mp15-pinctrl.dtsi.)

I agree, the current state is just heading toward combinatorial
explosion of pinmux entries.

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [Linux-stm32] [PATCH] ARM: dts: stm32: Consistently enable internal pull-ups for SD bus

[Ahmad Fatoum]

Hello Marek,

On 10/15/20 2:52 PM, Marek Vasut wrote:
> On 10/15/20 2:44 PM, Ahmad Fatoum wrote:
> 
> Hi,
> 
> [...]
> 
>> We already have 47K external pull-ups on all the SDMMC2's data lanes and we
>> don't need the SoC-internal pull-up there as well.
>>
>> On the SDMMC1 we lack them, so they were added in the board DTS:
> 
> You do need pullups on SD bus by default.

Yes, we are aware of this now :-)

> Currently the
> stm32mp15-pinmux.dtsi is not consistent in that aspect. I am not arguing
> about any one single board, but about the dtsi, I suspect by default the
> pull ups should be enabled, and possibly on board-DT-level they should
> be disabled if not needed instead ?

I think this is a good idea. But existing boards should be fixed up so that
their old behavior is maintained.

> [...]
> 
>> I don't mind the central pinctrl settings changed to make them more widely applicable
>> if current users get such override nodes inserted to maintain their existing settings.
>>
>> (My favorite would of course be to allow board DTS to just keep their own pinctrl
>>  nodes outside stm32mp15-pinctrl.dtsi.)
> 
> I agree, the current state is just heading toward combinatorial
> explosion of pinmux entries.


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 |

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [Linux-stm32] [PATCH] ARM: dts: stm32: Consistently enable internal pull-ups for SD bus

[Marek Vasut]

On 10/15/20 3:18 PM, Ahmad Fatoum wrote:
> Hello Marek,

Hi,

> On 10/15/20 2:52 PM, Marek Vasut wrote:
>> On 10/15/20 2:44 PM, Ahmad Fatoum wrote:
>>
>> Hi,
>>
>> [...]
>>
>>> We already have 47K external pull-ups on all the SDMMC2's data lanes and we
>>> don't need the SoC-internal pull-up there as well.
>>>
>>> On the SDMMC1 we lack them, so they were added in the board DTS:
>>
>> You do need pullups on SD bus by default.
> 
> Yes, we are aware of this now :-)
> 
>> Currently the
>> stm32mp15-pinmux.dtsi is not consistent in that aspect. I am not arguing
>> about any one single board, but about the dtsi, I suspect by default the
>> pull ups should be enabled, and possibly on board-DT-level they should
>> be disabled if not needed instead ?
> 
> I think this is a good idea. But existing boards should be fixed up so that
> their old behavior is maintained.

So, which boards do we fix. The automation-1 board and the ST ones ?

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [Linux-stm32] [PATCH] ARM: dts: stm32: Consistently enable internal pull-ups for SD bus

[Ahmad Fatoum]

Hello,

On 10/15/20 3:40 PM, Marek Vasut wrote:
>>> Currently the
>>> stm32mp15-pinmux.dtsi is not consistent in that aspect. I am not arguing
>>> about any one single board, but about the dtsi, I suspect by default the
>>> pull ups should be enabled, and possibly on board-DT-level they should
>>> be disabled if not needed instead ?
>>
>> I think this is a good idea. But existing boards should be fixed up so that
>> their old behavior is maintained.
> 
> So, which boards do we fix. The automation-1 board and the ST ones ?

fixed up, not fixed. I don't know if ST is fine with the changes, for
the MC-1, please do:

-------------- 8< -------------
diff --git a/arch/arm/boot/dts/stm32mp157c-lxa-mc1.dts b/arch/arm/boot/dts/stm32mp157c-lxa-mc1.dts
index 5700e6b700d3..7134466256b3 100644
--- a/arch/arm/boot/dts/stm32mp157c-lxa-mc1.dts
+++ b/arch/arm/boot/dts/stm32mp157c-lxa-mc1.dts
@@ -208,6 +208,14 @@ pins2 {
        };
 };

+&sdmmc2_d47_pins_b {
+       /* board already has external 47K pull-ups */
+
+       pins {
+               /delete-property/ bias-pull-up;
+               bias-disable;
+       };
+};
+
 &sdmmc2 {
        pinctrl-names = "default", "opendrain", "sleep";
        pinctrl-0 = <&sdmmc2_b4_pins_a &sdmmc2_d47_pins_b>;


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

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [Linux-stm32] [PATCH] ARM: dts: stm32: Consistently enable internal pull-ups for SD bus

[Alexandre Torgue]

Hi

On 10/15/20 3:47 PM, Ahmad Fatoum wrote:
> Hello,
> 
> On 10/15/20 3:40 PM, Marek Vasut wrote:
>>>> Currently the
>>>> stm32mp15-pinmux.dtsi is not consistent in that aspect. I am not arguing
>>>> about any one single board, but about the dtsi, I suspect by default the
>>>> pull ups should be enabled, and possibly on board-DT-level they should
>>>> be disabled if not needed instead ?
>>>
>>> I think this is a good idea. But existing boards should be fixed up so that
>>> their old behavior is maintained.
>>
>> So, which boards do we fix. The automation-1 board and the ST ones ?
> 
> fixed up, not fixed. I don't know if ST is fine with the changes, for
> the MC-1, please do:
> 
> -------------- 8< -------------
> diff --git a/arch/arm/boot/dts/stm32mp157c-lxa-mc1.dts b/arch/arm/boot/dts/stm32mp157c-lxa-mc1.dts
> index 5700e6b700d3..7134466256b3 100644
> --- a/arch/arm/boot/dts/stm32mp157c-lxa-mc1.dts
> +++ b/arch/arm/boot/dts/stm32mp157c-lxa-mc1.dts
> @@ -208,6 +208,14 @@ pins2 {
>          };
>   };
> 
> +&sdmmc2_d47_pins_b {
> +       /* board already has external 47K pull-ups */
> +
> +       pins {
> +               /delete-property/ bias-pull-up;
> +               bias-disable;
> +       };
> +};
> +

This proposition remind me an old discussion we got with Marek about 
"where pin definitions has to be done ?". My approach is to not define 
pins groups inside board dts file mainly because pinmux is a SoC 
configuration and a board only use one of those configurations blablabla 
:). But counter-argument is that pull-up/pull-down, Open-drain push-pull 
settings are "driven" by the board configuration and I agree.

We are exactly in this case here but before to rework all STM32 DT to 
split pin config I propose to keep what we have today, and to enable 
bias-pull-up, (or other settings) in boards that need it.

Marek, what's your feeling ?

cheers
alex



>   &sdmmc2 {
>          pinctrl-names = "default", "opendrain", "sleep";
>          pinctrl-0 = <&sdmmc2_b4_pins_a &sdmmc2_d47_pins_b>;
> 
> 

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [Linux-stm32] [PATCH] ARM: dts: stm32: Consistently enable internal pull-ups for SD bus

[Marek Vasut]

On 10/15/20 5:51 PM, Alexandre Torgue wrote:

Hi,

[...]

>>>>> Currently the
>>>>> stm32mp15-pinmux.dtsi is not consistent in that aspect. I am not
>>>>> arguing
>>>>> about any one single board, but about the dtsi, I suspect by
>>>>> default the
>>>>> pull ups should be enabled, and possibly on board-DT-level they should
>>>>> be disabled if not needed instead ?
>>>>
>>>> I think this is a good idea. But existing boards should be fixed up
>>>> so that
>>>> their old behavior is maintained.
>>>
>>> So, which boards do we fix. The automation-1 board and the ST ones ?
>>
>> fixed up, not fixed. I don't know if ST is fine with the changes, for
>> the MC-1, please do:
>>
>> -------------- 8< -------------
>> diff --git a/arch/arm/boot/dts/stm32mp157c-lxa-mc1.dts
>> b/arch/arm/boot/dts/stm32mp157c-lxa-mc1.dts
>> index 5700e6b700d3..7134466256b3 100644
>> --- a/arch/arm/boot/dts/stm32mp157c-lxa-mc1.dts
>> +++ b/arch/arm/boot/dts/stm32mp157c-lxa-mc1.dts
>> @@ -208,6 +208,14 @@ pins2 {
>>          };
>>   };
>>
>> +&sdmmc2_d47_pins_b {
>> +       /* board already has external 47K pull-ups */
>> +
>> +       pins {
>> +               /delete-property/ bias-pull-up;
>> +               bias-disable;
>> +       };
>> +};
>> +
> 
> This proposition remind me an old discussion we got with Marek about
> "where pin definitions has to be done ?". My approach is to not define
> pins groups inside board dts file mainly because pinmux is a SoC
> configuration and a board only use one of those configurations blablabla
> :). But counter-argument is that pull-up/pull-down, Open-drain push-pull
> settings are "driven" by the board configuration and I agree.
> 
> We are exactly in this case here but before to rework all STM32 DT to
> split pin config I propose to keep what we have today, and to enable
> bias-pull-up, (or other settings) in boards that need it.

I think this is not an easy question to answer.

Either you end up with boards patching some common pinmux file, which
would then make it difficult to update the pinmux in that file if needed
be, because you have subtle dependencies all over. (that's the current
proposal)

OR

You end up with multiple copies of similar-but-not-the-same pinmux in
every board DT, and it will not be clear which one is the right one.
(that's the i.MX case)

I really don't know which one is better (or worse).

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [Linux-stm32] [PATCH] ARM: dts: stm32: Consistently enable internal pull-ups for SD bus

[Ahmad Fatoum]

Hello Alex,

On 10/15/20 5:51 PM, Alexandre Torgue wrote:
> On 10/15/20 3:47 PM, Ahmad Fatoum wrote:
>> fixed up, not fixed. I don't know if ST is fine with the changes, for
>> the MC-1, please do:
>>
>> -------------- 8< -------------
>> diff --git a/arch/arm/boot/dts/stm32mp157c-lxa-mc1.dts b/arch/arm/boot/dts/stm32mp157c-lxa-mc1.dts
>> index 5700e6b700d3..7134466256b3 100644
>> --- a/arch/arm/boot/dts/stm32mp157c-lxa-mc1.dts
>> +++ b/arch/arm/boot/dts/stm32mp157c-lxa-mc1.dts
>> @@ -208,6 +208,14 @@ pins2 {
>>          };
>>   };
>>
>> +&sdmmc2_d47_pins_b {
>> +       /* board already has external 47K pull-ups */
>> +
>> +       pins {
>> +               /delete-property/ bias-pull-up;
>> +               bias-disable;
>> +       };
>> +};
>> +
> 
> This proposition remind me an old discussion we got with Marek about "where pin definitions has to be done ?". My approach is to not define pins groups inside board dts file mainly because pinmux is a SoC configuration and a board only use one of those configurations blablabla :). But counter-argument is that pull-up/pull-down, Open-drain push-pull settings are "driven" by the board configuration and I agree.

We can split pinmux away from pinconf and still have a header to be included everywhere
for pin muxing like the i.MX is doing, e.g.

/* stm32mp15-pinmux.h */
#define STM32MP15_PC8__SDMMC1_D0	<STM32_PINMUX('C',  8, AF12)>
#define STM32MP15_PC9__SDMMC1_D1	<STM32_PINMUX('C',  9, AF12)>
#define STM32MP15_PC10__SDMMC1_D2	<STM32_PINMUX('C', 10, AF12)>
#define STM32MP15_PC11__SDMMC1_D3	<STM32_PINMUX('C', 11, AF12)>

The s/stm32mp15-pinctrl.dtsi/stm32mp15-pinconf.dtsi/ can be made to use it and define
the pin configurations that are in use on the ST boards and that most of the boards will
probably copy off.

Boards DTS can then pick and choose which pinctrl groups to take from the ST one.
If they have custom needs they just define their own groups inline
(I know I repeat myself, https://lore.kernel.org/linux-arm-kernel/310aa3a3-09ce-42ef-d1ea-b653163d1d72@pengutronix.de/)

Cheers
Ahmad

> 
> We are exactly in this case here but before to rework all STM32 DT to split pin config I propose to keep what we have today, and to enable bias-pull-up, (or other settings) in boards that need it.
> 
> Marek, what's your feeling ?
> 
> cheers
> alex
> 
> 
> 
>>   &sdmmc2 {
>>          pinctrl-names = "default", "opendrain", "sleep";
>>          pinctrl-0 = <&sdmmc2_b4_pins_a &sdmmc2_d47_pins_b>;
>>
>>
> 

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

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [Linux-stm32] [PATCH] ARM: dts: stm32: Consistently enable internal pull-ups for SD bus

[Marek Vasut]

On 10/16/20 8:19 AM, Ahmad Fatoum wrote:
> Hello Alex,

Hi,

> On 10/15/20 5:51 PM, Alexandre Torgue wrote:
>> On 10/15/20 3:47 PM, Ahmad Fatoum wrote:
>>> fixed up, not fixed. I don't know if ST is fine with the changes, for
>>> the MC-1, please do:
>>>
>>> -------------- 8< -------------
>>> diff --git a/arch/arm/boot/dts/stm32mp157c-lxa-mc1.dts b/arch/arm/boot/dts/stm32mp157c-lxa-mc1.dts
>>> index 5700e6b700d3..7134466256b3 100644
>>> --- a/arch/arm/boot/dts/stm32mp157c-lxa-mc1.dts
>>> +++ b/arch/arm/boot/dts/stm32mp157c-lxa-mc1.dts
>>> @@ -208,6 +208,14 @@ pins2 {
>>>          };
>>>   };
>>>
>>> +&sdmmc2_d47_pins_b {
>>> +       /* board already has external 47K pull-ups */
>>> +
>>> +       pins {
>>> +               /delete-property/ bias-pull-up;
>>> +               bias-disable;
>>> +       };
>>> +};
>>> +
>>
>> This proposition remind me an old discussion we got with Marek about "where pin definitions has to be done ?". My approach is to not define pins groups inside board dts file mainly because pinmux is a SoC configuration and a board only use one of those configurations blablabla :). But counter-argument is that pull-up/pull-down, Open-drain push-pull settings are "driven" by the board configuration and I agree.
> 
> We can split pinmux away from pinconf and still have a header to be included everywhere
> for pin muxing like the i.MX is doing, e.g.
> 
> /* stm32mp15-pinmux.h */
> #define STM32MP15_PC8__SDMMC1_D0	<STM32_PINMUX('C',  8, AF12)>
> #define STM32MP15_PC9__SDMMC1_D1	<STM32_PINMUX('C',  9, AF12)>
> #define STM32MP15_PC10__SDMMC1_D2	<STM32_PINMUX('C', 10, AF12)>
> #define STM32MP15_PC11__SDMMC1_D3	<STM32_PINMUX('C', 11, AF12)>
> 
> The s/stm32mp15-pinctrl.dtsi/stm32mp15-pinconf.dtsi/ can be made to use it and define
> the pin configurations that are in use on the ST boards and that most of the boards will
> probably copy off.
> 
> Boards DTS can then pick and choose which pinctrl groups to take from the ST one.
> If they have custom needs they just define their own groups inline
> (I know I repeat myself, https://lore.kernel.org/linux-arm-kernel/310aa3a3-09ce-42ef-d1ea-b653163d1d72@pengutronix.de/)

The downside of this is that you will end up with a lot of duplication
of "pin groups" in board files.

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel