Re: [PATCH 3/3] arm64: dts: qcom: sm6115: Add USB SS qmp phy node

[Konrad Dybcio <konrad.dybcio@linaro.org>]

On 14.12.2022 06:20, Bhupesh Sharma wrote:
> On Wed, 14 Dec 2022 at 00:29, Krzysztof Kozlowski
> <krzysztof.kozlowski@linaro.org> wrote:
>>
>> On 13/12/2022 19:52, Bhupesh Sharma wrote:
>>> Hi Krzysztof,
>>>
>>> On Tue, 13 Dec 2022 at 18:26, Krzysztof Kozlowski
>>> <krzysztof.kozlowski@linaro.org> wrote:
>>>>
>>>> On 13/12/2022 13:38, Bhupesh Sharma wrote:
>>>>> Add USB superspeed qmp phy node to dtsi.
>>>>>
>>>>> Signed-off-by: Bhupesh Sharma <bhupesh.sharma@linaro.org>
>>>>> ---
>>>>>  arch/arm64/boot/dts/qcom/sm6115.dtsi | 38 ++++++++++++++++++++++++++--
>>>>>  1 file changed, 36 insertions(+), 2 deletions(-)
>>>>>
>>>>> diff --git a/arch/arm64/boot/dts/qcom/sm6115.dtsi b/arch/arm64/boot/dts/qcom/sm6115.dtsi
>>>>> index e4ce135264f3d..9c5c024919f92 100644
>>>>> --- a/arch/arm64/boot/dts/qcom/sm6115.dtsi
>>>>> +++ b/arch/arm64/boot/dts/qcom/sm6115.dtsi
>>>>> @@ -579,6 +579,40 @@ usb_hsphy: phy@1613000 {
>>>>>                       status = "disabled";
>>>>>               };
>>>>>
>>>>> +             usb_qmpphy: phy@1615000 {
>>>>> +                     compatible = "qcom,sm6115-qmp-usb3-phy";
>>>>> +                     reg = <0x01615000 0x200>;
>>>>> +                     #clock-cells = <1>;
>>>>> +                     #address-cells = <1>;
>>>>> +                     #size-cells = <1>;
>>>>> +                     ranges;
>>>>> +                     clocks = <&gcc GCC_USB3_PRIM_PHY_COM_AUX_CLK>,
>>>>> +                              <&gcc GCC_USB3_PRIM_CLKREF_CLK>,
>>>>> +                              <&gcc GCC_AHB2PHY_USB_CLK>;
>>>>> +                     clock-names = "com_aux",
>>>>> +                                   "ref",
>>>>> +                                   "cfg_ahb";
>>>>> +                     resets = <&gcc GCC_USB3_PHY_PRIM_SP0_BCR>,
>>>>> +                              <&gcc GCC_USB3PHY_PHY_PRIM_SP0_BCR>;
>>>>> +                     reset-names = "phy", "phy_phy";
>>>>> +                     status = "disabled";
>>>>
>>>> Hm, you add a disabled PHY which is used by existing controller. The
>>>> controller is enabled in board DTS, but new PHY node isn't. Aren't you
>>>> now breaking it?
>>>
>>> The USB controller is connected to two PHYs - one is HS PHY and the other is SS
>>> QMP Phy. So while the exiting board dts describes and uses only the HS
>>> PHY, newer
>>> board dts files (which will soon be sent out as a separate patch),
>>
>> Then I miss how do you narrow the existing DTS to use only one PHY. I
>> don't see anything in this patchset.
>>
>>> will use both the HS and SS
>>> USB PHYs.
>>>
>>> So, this will not break the existing board dts files.
>>
>> I still think it will be. The board boots with USB with one phy enabled
>> and one disabled. The driver gets phys unconditionally and one of them
>> is disabled.
>>
>> Even if Linux implementation will work (devm_usb_get_phy_by_phandle will
>> return -ENXIO or -ENODEV for disabled node), it is still a bit confusing
>> and I wonder how other users of such DTS should behave. Although it will
>> affect only one board, so maybe there are no other users?
> 
> Ah, now I get your point. So how does the following fix in
> sm4250-oneplus-billie2.dts look like. It allows the base dtsi to carry
> the usb nodes as exposed by the SoC and allows other board dts files
> to use both the USB2 and UBS3 PHYs.
> 
> Please let me know.
> 
> --- a/arch/arm64/boot/dts/qcom/sm4250-oneplus-billie2.dts
> +++ b/arch/arm64/boot/dts/qcom/sm4250-oneplus-billie2.dts
> @@ -232,6 +232,9 @@ &usb {
>  &usb_dwc3 {
>         maximum-speed = "high-speed";
>         dr_mode = "peripheral";
> +
> +       phys = <&usb_hsphy>;
> +       phy-names = "usb2-phy";
>  };
Looks good now!

Konrad
> 
> Thanks.