Messages in this thread | | | Date | Fri, 20 Jan 2023 20:48:55 +0530 | Subject | Re: [RFC v4 2/5] usb: dwc3: core: Refactor PHY logic to support Multiport Controller | From | Krishna Kurapati PSSNV <> |
| |
On 1/20/2023 8:43 PM, Krishna Kurapati PSSNV wrote: > > > 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
Typo in the previous mail. Correcting it here.
> working. Agreed, The dt-patch you provided will be a good working example of getting just a single port working for a multiport controller.
Regards, Krishna,
>>> >>>> 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; >>>> + }; >>>> }; >>> >>
| |