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

[Marek Vasut]

On 12/14/21 09:59, Christoph Niedermaier wrote:
> 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 at 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 at dh-electronics.com>
>>>>>>> Cc: Shawn Guo <shawnguo at kernel.org>
>>>>>>> Cc: Fabio Estevam <festevam at gmail.com>
>>>>>>> Cc: Marek Vasut <marex at denx.de>
>>>>>>> Cc: NXP Linux Team <linux-imx at nxp.com>
>>>>>>> Cc: kernel at dh-electronics.com
>>>>>>> To: linux-arm-kernel at 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.

Thanks