RE: [PATCH] ARM: dts: imx6qdl-dhcom: Add USB overcurrent pin on SoM layer

[Christoph Niedermaier <cniedermaier@dh-electronics.com>]

From: Marek Vasut
Sent: Monday, December 13, 2021 12:25 AM
> On 12/10/21 08:58, Christoph Niedermaier wrote:
>> From: Marek Vasut [mailto:marex@denx.de]
>> Sent: Thursday, December 9, 2021 11:26 PM
>>> On 12/9/21 10:54, Christoph Niedermaier wrote:
>>>> From: Marek Vasut
>>>> Sent: Thursday, December 9, 2021 1:23 AM
>>>>>
>>>>> On 12/8/21 16:15, Christoph Niedermaier wrote:
>>>>>> Add USB overcurrent pin muxing on SoM layer, but disable it
>>>>>> by default. If a board has connected this pin like the
>>>>>> picoITX, this property should be removed in the board file.
>>>>>>
>>>>>> Signed-off-by: Christoph Niedermaier <cniedermaier@dh-electronics.com>
>>>>>> Cc: Shawn Guo <shawnguo@kernel.org>
>>>>>> Cc: Fabio Estevam <festevam@gmail.com>
>>>>>> Cc: Marek Vasut <marex@denx.de>
>>>>>> Cc: NXP Linux Team <linux-imx@nxp.com>
>>>>>> Cc: kernel@dh-electronics.com
>>>>>> To: linux-arm-kernel@lists.infradead.org
>>>>>> ---
>>>>>>     arch/arm/boot/dts/imx6qdl-dhcom-picoitx.dtsi | 4 ++++
>>>>>>     arch/arm/boot/dts/imx6qdl-dhcom-som.dtsi     | 2 ++
>>>>>>     2 files changed, 6 insertions(+)
>>>>>>
>>>>>> diff --git a/arch/arm/boot/dts/imx6qdl-dhcom-picoitx.dtsi b/arch/arm/boot/dts/imx6qdl-dhcom-picoitx.dtsi
>>>>>> index 4cd4cb9543c8..a67682bfe7bd 100644
>>>>>> --- a/arch/arm/boot/dts/imx6qdl-dhcom-picoitx.dtsi
>>>>>> +++ b/arch/arm/boot/dts/imx6qdl-dhcom-picoitx.dtsi
>>>>>> @@ -48,6 +48,10 @@
>>>>>>                 "", "", "", "", "", "", "", "";
>>>>>>     };
>>>>>>
>>>>>> +&usbh1 { /* USB overcurrent pin is connected */
>>>>>> +     /delete-property/ disable-over-current;
>>>>>> +};
>>>>>> +
>>>>>>     &iomuxc {
>>>>>>         pinctrl-0 = <
>>>>>>                         /*
>>>>>> diff --git a/arch/arm/boot/dts/imx6qdl-dhcom-som.dtsi b/arch/arm/boot/dts/imx6qdl-dhcom-som.dtsi
>>>>>> index 5d10c40313cb..e4fdce016c34 100644
>>>>>> --- a/arch/arm/boot/dts/imx6qdl-dhcom-som.dtsi
>>>>>> +++ b/arch/arm/boot/dts/imx6qdl-dhcom-som.dtsi
>>>>>> @@ -385,6 +385,7 @@
>>>>>>     };
>>>>>>
>>>>>>     &usbh1 {
>>>>>> +     disable-over-current;
>>>>>>         dr_mode = "host";
>>>>>>         pinctrl-0 = <&pinctrl_usbh1>;
>>>>>>         pinctrl-names = "default";
>>>>>> @@ -728,6 +729,7 @@
>>>>>>         pinctrl_usbh1: usbh1-grp {
>>>>>>                 fsl,pins = <
>>>>>>                        MX6QDL_PAD_EIM_D31__GPIO3_IO31          0x120b0
>>>>>> +                     MX6QDL_PAD_EIM_D30__USB_H1_OC           0x1b0b1
>>>>>>                 >;
>>>>>>         };
>>>>>
>>>>> Shouldn't this be the other way around -- boards with unused USB
>>>>> overcurrent detection should disable it ? In this case, PDK2 and DRC02
>>>>> should add the 'disable-over-current' DT property and pinmux, while
>>>>> picoitx should add the USB OC pinmux .
>>>>
>>>> This pin is defined by the DHCOM standard, therefore no other function
>>>> can be used on this pin. That is why it should be configured in the SoM
>>>> file.
>>>
>>> Then the pinmux in the SoM dtsi is OK.
>>>
>>>> The first internal version was the other way around, but then we had a
>>>> discussion about accidentally enabling the overcurrent detection, because
>>>> it is enabled by default. Since most customers do not use overcurrent
>>>> detection, we have decided to disable it by default. So if a customer
>>>> uses that pin, he has to actively remove the DT property as for example
>>>> shown in the PicoITX board file. In this way, the source of issues should
>>>> be reduced.
>>>
>>> It seems to me that if the SoM has a dedicated pin for USB OC, then the
>>> SoM dtsi should keep the default configuration of USB OC (i.e. enabled).
>>> If a board does not use the USB OC (e.g. because there is a USB hub on
>>> it), then the board should add the 'disable-over-current' property,
>>> because this is clearly a board property, not a SoM property.
>>>
>>> Besides, on systems without a USB hub, you likely want to make sure the
>>> OC detection is not accidentally forgotten disabled, as that might lead
>>> to damage to the port.
>>>
>>> So I would say, keep the pinmux settings in the SoM dtsi, and add
>>> disable-over-current property on board level dts.
>>
>> I am with you, it is a board property. But I don't want to enable it by
>> default, because here I rate the accidental damage of the port higher.
>> So if you need it you can enable it on board layer. Because of the negative
>> logic have to do it this way. What is the argument against it?
> 
> Per the DHCOM specification, there is a dedicated USB OC input pin on
> the SoM. This would imply the SoM deliberately exposes a functionality
> and therefore that functionality should be enabled by default, or left
> in default setting (which is enabled) -- that means putting only the
> pinmux settings into the SoM DT, the OC input is enabled by default by
> the CI HDRC driver, so no need for extra SoM level properties.
> 
> Next, if a board does not use the OC input of the SoM, that is a board
> property, so the board should add 'disable-over-current' property in the
> board DT. The recommended configuration of boards is to connect the OC
> input, so in case there is an OC condition on a port, the CI HDRC driver
> would be notified and can turn off Vbus too e.g. via regulator to avoid
> stressing those Vbus power distribution switches on the board.
> 
> There is also an additional benefit of not adding the
> 'disable-over-current' property into SoM DT when it comes to DTOs, which
> are used on some DHSOM. A DTO can add a new DT property, but it is not
> capable of deleting an existing property in the base DT.

I will send a version 2.

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