Messages in this thread | | | Date | Fri, 20 Jan 2023 20:43:30 +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: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; >>> + }; >>> }; >> >
| |