Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <84ad5269-dd48-32ef-1313-6241980834bc@quicinc.com>
Date:   Fri, 20 Jan 2023 20:43:30 +0530
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:91.0) Gecko/20100101
 Thunderbird/91.12.0
Subject: Re: [RFC v4 2/5] usb: dwc3: core: Refactor PHY logic to support
 Multiport Controller
Content-Language: en-US
To:     Andrew Halaney <ahalaney@redhat.com>
CC:     Thinh Nguyen <Thinh.Nguyen@synopsys.com>,
        Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
        Philipp Zabel <p.zabel@pengutronix.de>,
        "Andy Gross" <agross@kernel.org>,
        Bjorn Andersson <andersson@kernel.org>,
        "Konrad Dybcio" <konrad.dybcio@linaro.org>,
        Rob Herring <robh+dt@kernel.org>,
        Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org>,
        Felipe Balbi <balbi@kernel.org>, <linux-usb@vger.kernel.org>,
        <linux-kernel@vger.kernel.org>, <linux-arm-msm@vger.kernel.org>,
        <devicetree@vger.kernel.org>, <quic_pkondeti@quicinc.com>,
        <quic_ppratap@quicinc.com>, <quic_wcheng@quicinc.com>,
        <quic_jackp@quicinc.com>, <quic_harshq@quicinc.com>,
        <quic_shazhuss@quicinc.com>
References: <20230115114146.12628-1-quic_kriskura@quicinc.com>
 <20230115114146.12628-3-quic_kriskura@quicinc.com>
 <20230119220942.ja5gbo3t3fl63gpy@halaney-x13s>
 <8f32c2e5-2743-1017-6a33-4849021c5287@quicinc.com>
 <20230120143717.ikbcb6x7wl4yy5d7@halaney-x13s>
From:   Krishna Kurapati PSSNV <quic_kriskura@quicinc.com>
In-Reply-To: <20230120143717.ikbcb6x7wl4yy5d7@halaney-x13s>
Content-Type: text/plain; charset="UTF-8"; format=flowed
Content-Transfer-Encoding: 7bit
Precedence: bulk


On 1/20/2023 8:07 PM, Andrew Halaney wrote:
> On Fri, Jan 20, 2023 at 07:25:57AM +0530, Krishna Kurapati PSSNV wrote:
>>
>>
>> On 1/20/2023 3:39 AM, Andrew Halaney wrote:
>>> On Sun, Jan 15, 2023 at 05:11:43PM +0530, Krishna Kurapati wrote:
>>>> Currently the DWC3 driver supports only single port controller
>>>> which requires at most one HS and one SS PHY.
>>>>
>>>> But the DWC3 USB controller can be connected to multiple ports and
>>>> each port can have their own PHYs. Each port of the multiport
>>>> controller can either be HS+SS capable or HS only capable
>>>> Proper quantification of them is required to modify GUSB2PHYCFG
>>>> and GUSB3PIPECTL registers appropriately.
>>>>
>>>> Add support for detecting, obtaining and configuring phy's supported
>>>> by a multiport controller and limit the max number of ports
>>>> supported to 4.
>>>>
>>>> Signed-off-by: Harsh Agarwal <quic_harshq@quicinc.com>
>>>> Signed-off-by: Krishna Kurapati <quic_kriskura@quicinc.com>
>>>> ---
>>>>    drivers/usb/dwc3/core.c | 304 +++++++++++++++++++++++++++++-----------
>>>>    drivers/usb/dwc3/core.h |  15 +-
>>>>    drivers/usb/dwc3/drd.c  |  14 +-
>>>>    3 files changed, 244 insertions(+), 89 deletions(-)
>>>>
>>>> diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
>>>> index 476b63618511..7e0a9a598dfd 100644
>>>> --- a/drivers/usb/dwc3/core.c
>>>> +++ b/drivers/usb/dwc3/core.c
>>>
>>> <snip>
>>>
>>>> @@ -1575,6 +1690,21 @@ static void dwc3_get_properties(struct dwc3 *dwc)
>>>>    	dwc->dis_split_quirk = device_property_read_bool(dev,
>>>>    				"snps,dis-split-quirk");
>>>> +
>>>> +	/*
>>>> +	 * If no mulitport properties are defined, default
>>>> +	 * the port count to '1'.
>>>> +	 */
>>>> +	ret = device_property_read_u32(dev, "num-ports",
>>>> +				&dwc->num_ports);
>>>> +	if (ret)
>>>> +		dwc->num_ports = 1;
>>>> +
>>>> +	ret = device_property_read_u32(dev, "num-ss-ports",
>>>> +				&dwc->num_ss_ports);
>>>> +	if (ret)
>>>> +		dwc->num_ss_ports = 1;
>>>
>>> By using this DT property instead of using the number of each phy type you
>>> find you can get into situations where you're writing DWC3_GUSB2PHYCFG, etc,
>>> when there's no phy to go along with it.
>>>
>> Hi Andrew,
>>
>>   Thanks for the review. Yes, this decoupling is still there and its fine I
>> believe.
>>
>>> I ran into this when testing on sa8540p-ride, which only uses one of the
>>> ports on the multiport controller. I didn't enable the other phys (not
>>> sure if that was smart or not) and overrode phy-names/phys, but did not
>>> override num-ports/num-ss-ports, which resulted in that. Nothing bad
>>> happened on a quick test.. but I thought I'd highlight that as another
>>> downside of decoupling this value from the number of phys you grab.
>>>
>> If we do not override phy-names or num-ports/num-ss-ports info in DT, they
>> are just defaulted to '1' and as per the current logic only port-1 registers
>> must be configured. Isn't that the case happening ?
>>
> 
> In my dts I'm inheriting from the sc8280xp.dtsi usb_2 phandle you've created!
> So unless I override them I get this from your sc8280xp.dtsi:
> 
> +                       usb_2_dwc3: usb@a400000 {
> +                               compatible = "snps,dwc3";
> +                               reg = <0 0x0a400000 0 0xcd00>;
> +                               interrupts = <GIC_SPI 133 IRQ_TYPE_LEVEL_HIGH>;
> +                               iommus = <&apps_smmu 0x800 0x0>;
> +                               num-ports = <4>;
> +                               num-ss-ports = <2>;
> +                               phys = <&usb_2_hsphy0>, <&usb_2_qmpphy0>,
> +                                       <&usb_2_hsphy1>, <&usb_2_qmpphy1>,
> +                                       <&usb_2_hsphy2>,
> +                                       <&usb_2_hsphy3>;
> +                               phy-names = "usb2-phy_port0", "usb3-phy_port0",
> +                                               "usb2-phy_port1", "usb3-phy_port1",
> +                                               "usb2-phy_port2",
> +                                               "usb2-phy_port3";
> +                       };
> 
> Since this board only uses one port of the multiport controller, I
> redefined phys/phy-names to indicate that. I figured that was more
> desireable than enabling unnecessary phys. Without overriding
> num-ports/num-ss-ports all the for loops in this patch would act like
> the values were 4 and 2 respectively, writing to DWC3_GUSB2PHYCFG
> multiple times etc as well as look for the multiport phy-names and fail
> to actually get any phys. Hope that makes sense!
> 
Hi Andrew,

  My Bad. I missed the fact that it was based on sc8280xp.dtsi. In that 
case it makes complete sense to override the num-ports and num-ss-ports 
to "1" and the usb phy-names.
>>> Here's a patch enabling sa8540p-ride, I'd love if you'd add it to the
>>> series (probably needs clean up after review, and will definitely need
>>> alteration after you update the dt-binding again). If not I'll continue
>>> to test/review so please CC me!:
>>>
>>>
>> Sure, I can add this patch (probably will add the other phy's too) during
>> the final submission.
> 
> I don't have a great understanding of the mapping of the phys to
> physical connections (as well as what registers like DWC3_GUSB2PHYCFG do),
> so if it makes more sense to enable all the relevant SoC phys, write
> those registers in the DWC3 IP, etc, and only use one of the actual
> board outputs then feel free. I think this is a good example of "what if
> a board designer only uses a single port of the multiport IP" imo.
> Agreed. This could be a good example of multi port with only single port 
working.

>>
>>>   From dcb27d07f079194ebd7efe1c9bec64da78beb290 Mon Sep 17 00:00:00 2001
>>> From: Andrew Halaney <ahalaney@redhat.com>
>>> Date: Thu, 19 Jan 2023 14:53:38 -0600
>>> Subject: [PATCH] arm64: dts: qcom: sa8540p-ride: Enable usb_2
>>> Content-type: text/plain
>>>
>>> There is now support for the multiport USB controller this uses
>>> so enable it.
>>>
>>> The board only has a single port hooked up (despite it being wired up to
>>> the multiport IP on the SoC). There's also a USB 2.0 mux hooked up,
>>> which by default on boot is selected to mux properly. Grab the gpio
>>> controlling that and ensure it stays in the right position so USB 2.0
>>> continues to be routed from the external port to the SoC.
>>>
>>> Signed-off-by: Andrew Halaney <ahalaney@redhat.com>
>>> ---
>>>    arch/arm64/boot/dts/qcom/sa8540p-ride.dts | 24 +++++++++++++++++++++++
>>>    1 file changed, 24 insertions(+)
>>>
>>> diff --git a/arch/arm64/boot/dts/qcom/sa8540p-ride.dts b/arch/arm64/boot/dts/qcom/sa8540p-ride.dts
>>> index 97957f3baa64..56d4f43faa1e 100644
>>> --- a/arch/arm64/boot/dts/qcom/sa8540p-ride.dts
>>> +++ b/arch/arm64/boot/dts/qcom/sa8540p-ride.dts
>>> @@ -246,6 +246,21 @@ &usb_0_qmpphy {
>>>    	status = "okay";
>>>    };
>>> +&usb_2 {
>>> +	pinctrl-names = "default";
>>> +	pinctrl-0 = <&usb2_en_state>;
>>> +
>>> +	status = "okay";
>>> +};
>>> +
>>> +&usb_2_dwc3 {
>>> +	dr_mode = "host";
>>> +	num-ports = <1>;
>>> +	num-ss-ports = <1>;
>>
>> More over, if this is a multiport controller and you are using only port-1,
>> it is as good as a single port controller I believe and the normal DT
>> convention must work. Adding these properties as "1" is not required as the
>> driver logic defaults them to "1" if they are not found.
> 
> See above comment about inheriting from sc8280xp.dtsi and needing to
> override their values.
> 
>>
>> Just to add a point here (as I was not clear in DT Binding description, My
>> bad), the num-ports and num-ss-ports must indicate the HS/SS Phys present on
>> HW whether they are used in DT or not. Just to cover all cases which user
>> can use [1].
>>
>> []1:
>> https://lore.kernel.org/all/4eb26a54-148b-942f-01c6-64e66541de8b@quicinc.com/
> 
> Ok, if you're going with that approach of "must indicate the HS/SS Phys
> present on HW whether they are used in the DT or not" (/me assumes DT
> here means on the board and not an incorrect coding of the DT) then I
> suppose I should not have overridden anything but phys/phy-names to
> indicate that I'm only using the first port (and used the multiport
> phy-names convention). It looks like in that link you also mention that
> it is ok to write to DWC3_GUSB2PHYCFG and friends even if the phy isn't
> defined, which was my concern and reasoning above for overriding
> num-ports/num-ss-ports.
> 
> Thanks,
> Andrew
> 
Actually, I was trying to mandate that rule to take care of cases where 
the phy's for say port2 or port3 are missing for a quad port controller 
in dtsi and we don't want to end up configuring wrong dwc3-phy regs.

For just the first port, the changes you have mentioned must be 
sufficient. (Furthermore, thanks for the review and testing it on 
sa8295-ride and confirming nothing breaks while the first port is enabled)

Regards,
Krishna,
>>
>> Regards,
>> Krishna,
>>
>>> +	phy-names = "usb2-phy", "usb3-phy";
>>> +	phys = <&usb_2_hsphy0>, <&usb_2_qmpphy0>;
>>> +};
>>> +
>>>    &usb_2_hsphy0 {
>>>    	vdda-pll-supply = <&vreg_l5a>;
>>>    	vdda18-supply = <&vreg_l7g>;
>>> @@ -313,4 +328,13 @@ wake-pins {
>>>    			bias-pull-up;
>>>    		};
>>>    	};
>>> +
>>> +	usb2_en_state: usb2-en-state {
>>> +		/* TS3USB221A USB2.0 mux select */
>>> +		pins = "gpio24";
>>> +		function = "gpio";
>>> +		drive-strength = <2>;
>>> +		bias-disable;
>>> +		output-low;
>>> +	};
>>>    };
>>
>