Re: [PATCH 1/2] dt-bindings: phy: mediatek: tphy: add a property for force-mode switch

[Krzysztof Kozlowski]

On 27/11/2023 08:09, Macpaul Lin wrote:
> On 11/25/23 18:37, Krzysztof Kozlowski and Chunfeng Yun wrote:
>> 	
>>
>> External email : Please do not click links or open attachments until you 
>> have verified the sender or the content.
>>
>> On 25/11/2023 02:23, Chunfeng Yun wrote:
>>> Due to some old SoCs with shared t-phy only support force-mode switch, and
>>> can't use compatible to distinguish between shared and non-shared t-phy,
>>> add a property to supported it.
>>> But now prefer to use "mediatek,syscon-type" on new SoC as far as possible.
>>>
>>> Signed-off-by: Chunfeng Yun <chunfeng.yun@xxxxxxxxxxxx>
>>> ---
>>>  Documentation/devicetree/bindings/phy/mediatek,tphy.yaml | 6 ++++++
>>>  1 file changed, 6 insertions(+)
>>>
>>> diff --git a/Documentation/devicetree/bindings/phy/mediatek,tphy.yaml b/Documentation/devicetree/bindings/phy/mediatek,tphy.yaml
>>> index 2bb91542e984..eedba5b7025e 100644
>>> --- a/Documentation/devicetree/bindings/phy/mediatek,tphy.yaml
>>> +++ b/Documentation/devicetree/bindings/phy/mediatek,tphy.yaml
>>> @@ -235,6 +235,12 @@ patternProperties:
>>>            Specify the flag to enable BC1.2 if support it
>>>          type: boolean
>>>  
>>> +      mediatek,force-mode:
>>> +        description:
>>> +          Use force mode to switch shared phy mode, perfer to use the bellow
>>
>> I still do not understand what is the "force mode" you want to use. What
>> modes do you have? What are the characteristics of force mode?
>>
>> Also, please run spellcheck.
>>
>> Best regards,
>> Krzysztof
>>
> 
> Thanks!
> 
> 1. I've tested this patch and it could solve the clock unstable for
> XHCI1 on mt8195 or mt8395 during system boot up or during
> unload/reload the phy driver.
> 
> The error message has been listed as follows.
> 
> [   13.849936][   T72] xhci-mtk 11290000.usb: supply vbus not found, 
> using dummy regulator
> [   13.851300][   T72] xhci-mtk 11290000.usb: uwk - reg:0x400, version:104
> [   13.852624][   T72] xhci-mtk 11290000.usb: xHCI Host Controller
> [   13.853393][   T72] xhci-mtk 11290000.usb: new USB bus registered, 
> assigned bus number 3
> [   13.874490][   T72] xhci-mtk 11290000.usb: clocks are not stable (0x3d0f)
> [   13.875369][   T72] xhci-mtk 11290000.usb: can't setup: -110
> [   13.876091][   T72] xhci-mtk 11290000.usb: USB bus 3 deregistered
> [   13.877081][   T72] xhci-mtk: probe of 11290000.usb failed with error 
> -110
> 
> 2. This is a fix patch to XHCI1 since MT8195 has been upstream.
> Please add "Fixes:" tags and "Cc: stable@xxxxxxxxxx" to back ward
> porting to previous stable trees.
> 
> For example, add
> Fixes: 6b5ef194611e5 ("phy: mediatek: tphy: remove macros to prepare 
> bitfield")
> is suggested since the force-mode was missing in the previous
> implementation which causes hardware function was abnormal.
> However, add
> Fixes: 33d18746fa514 ("phy: phy-mtk-tphy: use new io helpers to access 
> register")
> will be better since the USB support for mt8195 is already enabled in 
> late 2021.
> 
> 3. How about we revise the description as follows for more precisely?
> 
> mediatek,force-mode:
>    description:
>      The force mode is used to manually switch the shared PHY mode
>      between USB and PCIe. When force-mode is set, the USB 3.0 mode
>      will be selected. This is typically required for older SoCs
>      that do not automatically manage PHY mode switching.
>      For newer SoCs that support it, it is preferable to use the
>      "mediatek,syscon-type" property instead.
>    type: boolean

Again, what is force-mode? It looks like you wrote bindings for the
driver behavior. Bindings describe hardware, not how the driver should
behave. The property might be reasonable, but you must describe here
hardware characteristics/issue/etc.

Also, your driver change suggests this is compatible specific, so why it
cannot be deduced from compatible?

Best regards,
Krzysztof