| Message ID | 20230514054917.21318-1-quic_kriskura@quicinc.com |
|---|---|
| Headers | show |
| Series | Add multiport support for DWC3 controllers | expand |
On Sun, May 14, 2023 at 11:19:08AM +0530, Krishna Kurapati wrote: > Currently the DWC3 driver supports only single port controller which > requires at most two PHYs ie HS and SS PHYs. There are SoCs that has > DWC3 controller with multiple ports that can operate in host mode. > Some of the port supports both SS+HS and other port supports only HS > mode. > > This change primarily refactors the Phy logic in core driver to allow > multiport support with Generic Phy's. > > Chananges have been tested on QCOM SoC SA8295P which has 4 ports (2 > are HS+SS capable and 2 are HS only capable). > I'm able to detect my USB stick on all 4 ports on the sa8295p adp. Tested-by: Bjorn Andersson <andersson@kernel.org> Thanks, Bjorn > Changes in v8: > Reorganised code in patch-5 > Fixed nitpicks in code according to comments received on v7 > Fixed indentation in DT patches > Added drive strength for pinctrl nodes in SA8295 DT > > Changes in v7: > Added power event irq's for Multiport controller. > Udpated commit text for patch-9 (adding DT changes for enabling first > port of multiport controller on sa8540-ride). > Fixed check-patch warnings for driver code. > Fixed DT binding errors for changes in snps,dwc3.yaml > Reabsed code on top of usb-next > > Changes in v6: > Updated comments in code after. > Updated variables names appropriately as per review comments. > Updated commit text in patch-2 and added additional info as per review > comments. > The patch header in v5 doesn't have "PATHCH v5" notation present. Corrected > it in this version. > > Changes in v5: > Added DT support for first port of Teritiary USB controller on SA8540-Ride > Added support for reading port info from XHCI Extended Params registers. > > Changes in RFC v4: > Added DT support for SA8295p. > > Changes in RFC v3: > Incase any PHY init fails, then clear/exit the PHYs that > are already initialized. > > Changes in RFC v2: > Changed dwc3_count_phys to return the number of PHY Phandles in the node. > This will be used now in dwc3_extract_num_phys to increment num_usb2_phy > and num_usb3_phy. > > Added new parameter "ss_idx" in dwc3_core_get_phy_ny_node and changed its > structure such that the first half is for HS-PHY and second half is for > SS-PHY. > > In dwc3_core_get_phy, for multiport controller, only if SS-PHY phandle is > present, pass proper SS_IDX else pass -1. > > Link to v7: https://lore.kernel.org/all/20230501143445.3851-1-quic_kriskura@quicinc.com/ > Link to v6: https://lore.kernel.org/all/20230405125759.4201-1-quic_kriskura@quicinc.com/ > Link to v5: https://lore.kernel.org/all/20230310163420.7582-1-quic_kriskura@quicinc.com/ > Link to RFC v4: https://lore.kernel.org/all/20230115114146.12628-1-quic_kriskura@quicinc.com/ > Link to RFC v3: https://lore.kernel.org/all/1654709787-23686-1-git-send-email-quic_harshq@quicinc.com/#r > Link to RFC v2: https://lore.kernel.org/all/1653560029-6937-1-git-send-email-quic_harshq@quicinc.com/#r > > Test results: > > Bus 3/4 represent multiport controller having 4 HS ports and 2 SS ports. > > / # dmesg |grep hub > [ 0.029029] usbcore: registered new interface driver hub > [ 1.372812] hub 1-0:1.0: USB hub found > [ 1.389142] hub 1-0:1.0: 1 port detected > [ 1.414721] hub 2-0:1.0: USB hub found > [ 1.427669] hub 2-0:1.0: 1 port detected > [ 2.931465] hub 3-0:1.0: USB hub found > [ 2.935340] hub 3-0:1.0: 4 ports detected > [ 2.948721] hub 4-0:1.0: USB hub found > [ 2.952604] hub 4-0:1.0: 2 ports detected > / # > / # lsusb > Bus 003 Device 001: ID 1d6b:0002 > Bus 001 Device 001: ID 1d6b:0002 > Bus 003 Device 005: ID 0b0e:0300 > Bus 003 Device 002: ID 046d:c077 > Bus 004 Device 001: ID 1d6b:0003 > Bus 002 Device 001: ID 1d6b:0003 > Bus 003 Device 004: ID 03f0:0024 > Bus 003 Device 003: ID 046d:c016 > > Krishna Kurapati (9): > dt-bindings: usb: qcom,dwc3: Add bindings for SC8280 Multiport > dt-bindings: usb: Add bindings for multiport properties on DWC3 > controller > usb: dwc3: core: Access XHCI address space temporarily to read port > info > usb: dwc3: core: Skip setting event buffers for host only controllers > usb: dwc3: core: Refactor PHY logic to support Multiport Controller > usb: dwc3: qcom: Add multiport controller support for qcom wrapper > arm64: dts: qcom: sc8280xp: Add multiport controller node for SC8280 > arm64: dts: qcom: sa8295p: Enable tertiary controller and its 4 USB > ports > arm64: dts: qcom: sa8540-ride: Enable first port of tertiary usb > controller > > .../devicetree/bindings/usb/qcom,dwc3.yaml | 22 + > .../devicetree/bindings/usb/snps,dwc3.yaml | 13 +- > arch/arm64/boot/dts/qcom/sa8295p-adp.dts | 52 +++ > arch/arm64/boot/dts/qcom/sa8540p-ride.dts | 22 + > arch/arm64/boot/dts/qcom/sc8280xp.dtsi | 66 +++ > drivers/usb/dwc3/core.c | 389 +++++++++++++++--- > drivers/usb/dwc3/core.h | 28 +- > drivers/usb/dwc3/drd.c | 13 +- > drivers/usb/dwc3/dwc3-qcom.c | 28 +- > 9 files changed, 543 insertions(+), 90 deletions(-) > > -- > 2.40.0 >
On Sun, May 14, 2023 at 11:19:12AM +0530, Krishna Kurapati wrote: > On some SoC's like SA8295P where the tertiary controller is host-only Please add "Qualcomm" before SA8295P. > capable, GEVTADDRHI/LO, GEVTSIZ, GEVTCOUNT registers are not accessible. > Trying to setup them up during core_init leads to a crash. s/setup/access/ (or "write to"?) and presumably this is a problem beyond core_init, so I would suggest dropping "up during core_init" from the sentence. > > For DRD/Peripheral supported controllers, event buffer setup is done > again in gadget_pullup. Skip setup or cleanup of event buffers if > controller is host-only capable. > With that, this looks reasonable to me. Reviewed-by: Bjorn Andersson <andersson@kernel.org> Regards, Bjorn > Signed-off-by: Krishna Kurapati <quic_kriskura@quicinc.com> > --- > drivers/usb/dwc3/core.c | 10 ++++++++++ > 1 file changed, 10 insertions(+) > > diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c > index e983aef1fb93..46192d08d1b6 100644 > --- a/drivers/usb/dwc3/core.c > +++ b/drivers/usb/dwc3/core.c > @@ -505,6 +505,11 @@ static int dwc3_alloc_event_buffers(struct dwc3 *dwc, unsigned int length) > int dwc3_event_buffers_setup(struct dwc3 *dwc) > { > struct dwc3_event_buffer *evt; > + unsigned int hw_mode; > + > + hw_mode = DWC3_GHWPARAMS0_MODE(dwc->hwparams.hwparams0); > + if (hw_mode == DWC3_GHWPARAMS0_MODE_HOST) > + return 0; > > evt = dwc->ev_buf; > evt->lpos = 0; > @@ -522,6 +527,11 @@ int dwc3_event_buffers_setup(struct dwc3 *dwc) > void dwc3_event_buffers_cleanup(struct dwc3 *dwc) > { > struct dwc3_event_buffer *evt; > + unsigned int hw_mode; > + > + hw_mode = DWC3_GHWPARAMS0_MODE(dwc->hwparams.hwparams0); > + if (hw_mode == DWC3_GHWPARAMS0_MODE_HOST) > + return; > > evt = dwc->ev_buf; > > -- > 2.40.0 >
On Sun, May 14, 2023 at 11:19:14AM +0530, Krishna Kurapati wrote: > QCOM SoC SA8295P's tertiary quad port controller supports 2 HS+SS > ports and 2 HS only ports. Add support for configuring PWR_EVENT_IRQ's > for all the ports during suspend/resume. > > Signed-off-by: Krishna Kurapati <quic_kriskura@quicinc.com> > --- > drivers/usb/dwc3/dwc3-qcom.c | 28 ++++++++++++++++++++++------ > 1 file changed, 22 insertions(+), 6 deletions(-) > > diff --git a/drivers/usb/dwc3/dwc3-qcom.c b/drivers/usb/dwc3/dwc3-qcom.c > index 959fc925ca7c..7a9bce66295d 100644 > --- a/drivers/usb/dwc3/dwc3-qcom.c > +++ b/drivers/usb/dwc3/dwc3-qcom.c > @@ -37,7 +37,10 @@ > #define PIPE3_PHYSTATUS_SW BIT(3) > #define PIPE_UTMI_CLK_DIS BIT(8) > > -#define PWR_EVNT_IRQ_STAT_REG 0x58 > +#define PWR_EVNT_IRQ1_STAT_REG 0x58 > +#define PWR_EVNT_IRQ2_STAT_REG 0x1dc > +#define PWR_EVNT_IRQ3_STAT_REG 0x228 > +#define PWR_EVNT_IRQ4_STAT_REG 0x238 > #define PWR_EVNT_LPM_IN_L2_MASK BIT(4) > #define PWR_EVNT_LPM_OUT_L2_MASK BIT(5) > > @@ -93,6 +96,13 @@ struct dwc3_qcom { > struct icc_path *icc_path_apps; > }; > > +static u32 pwr_evnt_irq_stat_reg_offset[4] = { > + PWR_EVNT_IRQ1_STAT_REG, > + PWR_EVNT_IRQ2_STAT_REG, > + PWR_EVNT_IRQ3_STAT_REG, > + PWR_EVNT_IRQ4_STAT_REG, Seems to be excessive indentation of these... Can you also please confirm that these should be counted starting at 1 - given that you otherwise talk about port0..N-1? > +}; > + > static inline void dwc3_qcom_setbits(void __iomem *base, u32 offset, u32 val) > { > u32 reg; > @@ -413,13 +423,16 @@ static int dwc3_qcom_suspend(struct dwc3_qcom *qcom, bool wakeup) > { > u32 val; > int i, ret; > + struct dwc3 *dwc = platform_get_drvdata(qcom->dwc3); > > if (qcom->is_suspended) > return 0; > > - val = readl(qcom->qscratch_base + PWR_EVNT_IRQ_STAT_REG); > - if (!(val & PWR_EVNT_LPM_IN_L2_MASK)) > - dev_err(qcom->dev, "HS-PHY not in L2\n"); > + for (i = 0; i < dwc->num_usb2_ports; i++) { In the event that the dwc3 core fails to acquire or enable e.g. clocks its drvdata will be NULL. If you then hit a runtime pm transition in the dwc3-qcom glue you will dereference NULL here. (You can force this issue by e.g. returning -EINVAL from dwc3_clk_enable()). So if you're peaking into qcom->dwc3 you need to handle the fact that dwc might be NULL, here and in resume below. Regards, Bjorn
On 5/16/2023 3:57 AM, Bjorn Andersson wrote: > On Sun, May 14, 2023 at 11:19:14AM +0530, Krishna Kurapati wrote: >> QCOM SoC SA8295P's tertiary quad port controller supports 2 HS+SS >> ports and 2 HS only ports. Add support for configuring PWR_EVENT_IRQ's >> for all the ports during suspend/resume. >> >> Signed-off-by: Krishna Kurapati <quic_kriskura@quicinc.com> >> --- >> drivers/usb/dwc3/dwc3-qcom.c | 28 ++++++++++++++++++++++------ >> 1 file changed, 22 insertions(+), 6 deletions(-) >> >> diff --git a/drivers/usb/dwc3/dwc3-qcom.c b/drivers/usb/dwc3/dwc3-qcom.c >> index 959fc925ca7c..7a9bce66295d 100644 >> --- a/drivers/usb/dwc3/dwc3-qcom.c >> +++ b/drivers/usb/dwc3/dwc3-qcom.c >> @@ -37,7 +37,10 @@ >> #define PIPE3_PHYSTATUS_SW BIT(3) >> #define PIPE_UTMI_CLK_DIS BIT(8) >> >> -#define PWR_EVNT_IRQ_STAT_REG 0x58 >> +#define PWR_EVNT_IRQ1_STAT_REG 0x58 >> +#define PWR_EVNT_IRQ2_STAT_REG 0x1dc >> +#define PWR_EVNT_IRQ3_STAT_REG 0x228 >> +#define PWR_EVNT_IRQ4_STAT_REG 0x238 >> #define PWR_EVNT_LPM_IN_L2_MASK BIT(4) >> #define PWR_EVNT_LPM_OUT_L2_MASK BIT(5) >> >> @@ -93,6 +96,13 @@ struct dwc3_qcom { >> struct icc_path *icc_path_apps; >> }; >> >> +static u32 pwr_evnt_irq_stat_reg_offset[4] = { >> + PWR_EVNT_IRQ1_STAT_REG, >> + PWR_EVNT_IRQ2_STAT_REG, >> + PWR_EVNT_IRQ3_STAT_REG, >> + PWR_EVNT_IRQ4_STAT_REG, > > Seems to be excessive indentation of these... > > Can you also please confirm that these should be counted starting at 1 - > given that you otherwise talk about port0..N-1? > Hi Bjorn, I am fine with either way. Since this just denoted 4 different ports, I named them starting with 1. Either ways, we will run through array from (0-3), so we must be fine. >> +}; >> + >> static inline void dwc3_qcom_setbits(void __iomem *base, u32 offset, u32 val) >> { >> u32 reg; >> @@ -413,13 +423,16 @@ static int dwc3_qcom_suspend(struct dwc3_qcom *qcom, bool wakeup) >> { >> u32 val; >> int i, ret; >> + struct dwc3 *dwc = platform_get_drvdata(qcom->dwc3); >> >> if (qcom->is_suspended) >> return 0; >> >> - val = readl(qcom->qscratch_base + PWR_EVNT_IRQ_STAT_REG); >> - if (!(val & PWR_EVNT_LPM_IN_L2_MASK)) >> - dev_err(qcom->dev, "HS-PHY not in L2\n"); >> + for (i = 0; i < dwc->num_usb2_ports; i++) { > > In the event that the dwc3 core fails to acquire or enable e.g. clocks > its drvdata will be NULL. If you then hit a runtime pm transition in the > dwc3-qcom glue you will dereference NULL here. (You can force this issue > by e.g. returning -EINVAL from dwc3_clk_enable()). > > So if you're peaking into qcom->dwc3 you need to handle the fact that > dwc might be NULL, here and in resume below. > Thanks for catching this. You are right, there were instances where the we saw probe for dwc3 being deferred while the probe for dwc3-qcom was still successful [1]. In this case, if the dwc3 probe never happened and system tries to enter suspend, we might hit a NULL pointer dereference. I will fix this in v9. [1]: https://patchwork.kernel.org/project/linux-usb/patch/1657809067-25815-1-git-send-email-quic_kriskura@quicinc.com/ Regards, Krishna,
On Sun, May 14, 2023 at 11:19:09AM +0530, Krishna Kurapati wrote: > Add the compatible string for SC8280 Multiport USB controller from > Qualcomm. > > There are 4 power event irq interrupts supported by this controller > (one for each port of multiport). Added all the 4 as non-optional > interrupts for SC8280XP-MP > > Signed-off-by: Krishna Kurapati <quic_kriskura@quicinc.com> > --- > .../devicetree/bindings/usb/qcom,dwc3.yaml | 22 +++++++++++++++++++ > 1 file changed, 22 insertions(+) > + - if: > + properties: > + compatible: > + contains: > + enum: > + - qcom,sc8280xp-dwc3-mp > + then: > + properties: > + interrupts: > + maxItems: 7 > + interrupt-names: > + items: > + - const: dp_hs_phy_irq > + - const: dm_hs_phy_irq > + - const: ss_phy_irq I assume that these are only for the first port, and that you need to define these interrupts also for ports 2-4. > + - const: pwr_event_1 > + - const: pwr_event_2 > + - const: pwr_event_3 > + - const: pwr_event_4 > + > additionalProperties: false > > examples: Johan
On Sun, May 14, 2023 at 11:19:12AM +0530, Krishna Kurapati wrote: > On some SoC's like SA8295P where the tertiary controller is host-only > capable, GEVTADDRHI/LO, GEVTSIZ, GEVTCOUNT registers are not accessible. > Trying to setup them up during core_init leads to a crash. > > For DRD/Peripheral supported controllers, event buffer setup is done > again in gadget_pullup. Skip setup or cleanup of event buffers if > controller is host-only capable. > > Signed-off-by: Krishna Kurapati <quic_kriskura@quicinc.com> > --- > drivers/usb/dwc3/core.c | 10 ++++++++++ > 1 file changed, 10 insertions(+) > > diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c > index e983aef1fb93..46192d08d1b6 100644 > --- a/drivers/usb/dwc3/core.c > +++ b/drivers/usb/dwc3/core.c > @@ -505,6 +505,11 @@ static int dwc3_alloc_event_buffers(struct dwc3 *dwc, unsigned int length) > int dwc3_event_buffers_setup(struct dwc3 *dwc) > { > struct dwc3_event_buffer *evt; > + unsigned int hw_mode; > + > + hw_mode = DWC3_GHWPARAMS0_MODE(dwc->hwparams.hwparams0); > + if (hw_mode == DWC3_GHWPARAMS0_MODE_HOST) > + return 0; > > evt = dwc->ev_buf; How about adding this check to dwc3_alloc_event_buffers() instead as there should be no need to allocate buffer that you never use? Then you can just check dwc->ev_buf here and elsewhere. Johan
On 5/16/2023 5:47 PM, Johan Hovold wrote: > On Sun, May 14, 2023 at 11:19:12AM +0530, Krishna Kurapati wrote: >> On some SoC's like SA8295P where the tertiary controller is host-only >> capable, GEVTADDRHI/LO, GEVTSIZ, GEVTCOUNT registers are not accessible. >> Trying to setup them up during core_init leads to a crash. >> >> For DRD/Peripheral supported controllers, event buffer setup is done >> again in gadget_pullup. Skip setup or cleanup of event buffers if >> controller is host-only capable. >> >> Signed-off-by: Krishna Kurapati <quic_kriskura@quicinc.com> >> --- >> drivers/usb/dwc3/core.c | 10 ++++++++++ >> 1 file changed, 10 insertions(+) >> >> diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c >> index e983aef1fb93..46192d08d1b6 100644 >> --- a/drivers/usb/dwc3/core.c >> +++ b/drivers/usb/dwc3/core.c >> @@ -505,6 +505,11 @@ static int dwc3_alloc_event_buffers(struct dwc3 *dwc, unsigned int length) >> int dwc3_event_buffers_setup(struct dwc3 *dwc) >> { >> struct dwc3_event_buffer *evt; >> + unsigned int hw_mode; >> + >> + hw_mode = DWC3_GHWPARAMS0_MODE(dwc->hwparams.hwparams0); >> + if (hw_mode == DWC3_GHWPARAMS0_MODE_HOST) >> + return 0; >> >> evt = dwc->ev_buf; > > How about adding this check to dwc3_alloc_event_buffers() instead as > there should be no need to allocate buffer that you never use? > > Then you can just check dwc->ev_buf here and elsewhere. > Thanks for this idea. We can save 4096 bytes from being allocated this way. Will get this in next version. Regards, Krishna,
On Wed, May 17, 2023 at 05:49:13PM +0530, Krishna Kurapati PSSNV wrote: > On 5/17/2023 5:14 PM, Johan Hovold wrote: > > On Wed, May 17, 2023 at 04:40:11PM +0530, Krishna Kurapati PSSNV wrote: > >> On 5/16/2023 4:29 PM, Johan Hovold wrote: > >>> On Sun, May 14, 2023 at 11:19:09AM +0530, Krishna Kurapati wrote: > > > >>>> + interrupts: > >>>> + maxItems: 7 > >>>> + interrupt-names: > >>>> + items: > >>>> + - const: dp_hs_phy_irq > >>>> + - const: dm_hs_phy_irq > >>>> + - const: ss_phy_irq > >>> > >>> I assume that these are only for the first port, and that you need to > >>> define these interrupts also for ports 2-4. > > > >> I wanted to add them when wakeup-source is enabled but since you > >> mentioned that these must be added now and driver support can be added > >> later, I will make a patch separately for this in v9. > > > >> Can I use the following notation for the new interrupts ? > >> > >> dp_hs_port2_irq > >> dm_hs_port2_irq > >> dp_hs_port3_irq > >> dm_hs_port3_irq > >> dp_hs_port4_irq > >> dm_hs_port4_irq > >> > >> > >> That way the interrupt names for first port will be same as ones for > >> single port. > > > > For consistency, I'd say: use the same scheme also for port1. Perhaps > > "port" is unnecessary too. > > > > And since these are getting new names, you can drop the redundant "_irq" > > suffix as you did for the power-event lines. > The reason I wanted to mark it as dp_hs_portX_irq is to keep code > changes to driver simple. The existing code to read current IRQ's can > stay as it. Only need to add changes for reading IRQ's of new ports. I understand why you want to do it this way, but again, the devicetree binding is supposed to be hardware description that is independent from any particular implementation. This is also why I said that it may be preferable/easier to just implement wakeup for MP from the start. > > For example: > > > > pwr_event_1 > > dp_hs_phy_1 > > dm_hs_phy_1 > > ss_phy_1 > > ... Johan
On 6/27/2023 9:13 PM, Johan Hovold wrote: > Hi Krishna, > > On Wed, Jun 07, 2023 at 02:16:37PM +0200, Johan Hovold wrote: >> On Sun, May 14, 2023 at 11:19:14AM +0530, Krishna Kurapati wrote: > >>> static inline void dwc3_qcom_setbits(void __iomem *base, u32 offset, u32 val) >>> { >>> u32 reg; >>> @@ -413,13 +423,16 @@ static int dwc3_qcom_suspend(struct dwc3_qcom *qcom, bool wakeup) >>> { >>> u32 val; >>> int i, ret; >>> + struct dwc3 *dwc = platform_get_drvdata(qcom->dwc3); >>> >>> if (qcom->is_suspended) >>> return 0; >>> >>> - val = readl(qcom->qscratch_base + PWR_EVNT_IRQ_STAT_REG); >>> - if (!(val & PWR_EVNT_LPM_IN_L2_MASK)) >>> - dev_err(qcom->dev, "HS-PHY not in L2\n"); >>> + for (i = 0; i < dwc->num_usb2_ports; i++) { >>> + val = readl(qcom->qscratch_base + pwr_evnt_irq_stat_reg_offset[i]); >>> + if (!(val & PWR_EVNT_LPM_IN_L2_MASK)) >>> + dev_err(qcom->dev, "HS-PHY%d not in L2\n", i); >>> + } > >> When testing this on the X13s I get: >> >> dwc3-qcom a4f8800.usb: HS-PHY2 not in L2 >> >> for the third port, whose status registers always seems to return zero >> (e.g. as if we're checking the wrong register?): >> >> dwc3-qcom a4f8800.usb: dwc3_qcom_suspend - phy 0, pwr_event_stat = 38103c >> dwc3-qcom a4f8800.usb: dwc3_qcom_suspend - phy 1, pwr_event_stat = 38103c >> dwc3-qcom a4f8800.usb: dwc3_qcom_suspend - phy 2, pwr_event_stat = 00 >> dwc3-qcom a4f8800.usb: dwc3_qcom_suspend - phy 3, pwr_event_stat = 140030 >> >> I verified that everything appears to work as expected on sa8295p-adp. >> >> Do you have any idea of what may be causing this? > > You never replied to this; do you have any idea why the status register > for the second port seemingly always read back as 0 on the X13s? > > Johan Hi Johan, Missed this mail. This never popped up on my system. So no idea what is different in Lenovo X13s. Might need to check with team internally. Regards, Krishna,
On 21.07.2023 14:54, Johan Hovold wrote: > On Fri, Jul 21, 2023 at 02:10:07PM +0200, Konrad Dybcio wrote: >> On 21.07.2023 13:16, Johan Hovold wrote: >>> On Fri, Jul 14, 2023 at 04:08:45PM +0530, Krishna Kurapati PSSNV wrote: >>>> On 7/14/2023 2:30 PM, Johan Hovold wrote: >>>>> On Mon, Jul 03, 2023 at 12:35:48AM +0530, Krishna Kurapati PSSNV wrote: >>>>>> On 6/27/2023 9:13 PM, Johan Hovold wrote: >>>>>>> On Wed, Jun 07, 2023 at 02:16:37PM +0200, Johan Hovold wrote: >>>>>>>> On Sun, May 14, 2023 at 11:19:14AM +0530, Krishna Kurapati wrote: >>>>> >>>>>>>>> - val = readl(qcom->qscratch_base + PWR_EVNT_IRQ_STAT_REG); >>>>>>>>> - if (!(val & PWR_EVNT_LPM_IN_L2_MASK)) >>>>>>>>> - dev_err(qcom->dev, "HS-PHY not in L2\n"); >>>>>>>>> + for (i = 0; i < dwc->num_usb2_ports; i++) { >>>>>>>>> + val = readl(qcom->qscratch_base + pwr_evnt_irq_stat_reg_offset[i]); >>>>>>>>> + if (!(val & PWR_EVNT_LPM_IN_L2_MASK)) >>>>>>>>> + dev_err(qcom->dev, "HS-PHY%d not in L2\n", i); >>>>>>>>> + } >>>>>>> >>>>>>>> When testing this on the X13s I get: >>>>>>>> >>>>>>>> dwc3-qcom a4f8800.usb: HS-PHY2 not in L2 > >> Sidenote, I get this on any Qcom device on any platform I try >> to enter suspend on, without these MP patches. > > Ok, that might provide some hint. But on sc8280xp (X13s) we only get it > on one of the four MP ports (i.e. on one out of six ports in total). > > While on sa8295p-adp there are no such errors on any port. I've been playing with 8450 and it looks like snps,dis_u2_susphy_quirk causes this error. The downstream tree contains this property and I'm inclined to believe it means that this platforms should define it (as the devicetrees are machine-generated to a degree, AFAIK), especially since this quirk does the exact same thing on a known-working downstream, namely unsetting DWC3_GUSB2PHYCFG_SUSPHY. Digging a bit deeper, dwc3-msm-core [1], the downstream version of dwc3-qcom performs a bit of a dance in a couple of places.. Look for that register name. Unfortunately I have little idea what the "USB2 suspend phy" is.. is it a PHY used in suspend? Is it the suspension of the USB2 PHY? No clue. [1] https://git.codelinaro.org/clo/la/kernel/msm-5.10/-/blob/KERNEL.PLATFORM.1.0.r2-08800-WAIPIOLE.0/drivers/usb/dwc3/dwc3-msm-core.c Konrad
Currently the DWC3 driver supports only single port controller which requires at most two PHYs ie HS and SS PHYs. There are SoCs that has DWC3 controller with multiple ports that can operate in host mode. Some of the port supports both SS+HS and other port supports only HS mode. This change primarily refactors the Phy logic in core driver to allow multiport support with Generic Phy's. Chananges have been tested on QCOM SoC SA8295P which has 4 ports (2 are HS+SS capable and 2 are HS only capable). Changes in v8: Reorganised code in patch-5 Fixed nitpicks in code according to comments received on v7 Fixed indentation in DT patches Added drive strength for pinctrl nodes in SA8295 DT Changes in v7: Added power event irq's for Multiport controller. Udpated commit text for patch-9 (adding DT changes for enabling first port of multiport controller on sa8540-ride). Fixed check-patch warnings for driver code. Fixed DT binding errors for changes in snps,dwc3.yaml Reabsed code on top of usb-next Changes in v6: Updated comments in code after. Updated variables names appropriately as per review comments. Updated commit text in patch-2 and added additional info as per review comments. The patch header in v5 doesn't have "PATHCH v5" notation present. Corrected it in this version. Changes in v5: Added DT support for first port of Teritiary USB controller on SA8540-Ride Added support for reading port info from XHCI Extended Params registers. Changes in RFC v4: Added DT support for SA8295p. Changes in RFC v3: Incase any PHY init fails, then clear/exit the PHYs that are already initialized. Changes in RFC v2: Changed dwc3_count_phys to return the number of PHY Phandles in the node. This will be used now in dwc3_extract_num_phys to increment num_usb2_phy and num_usb3_phy. Added new parameter "ss_idx" in dwc3_core_get_phy_ny_node and changed its structure such that the first half is for HS-PHY and second half is for SS-PHY. In dwc3_core_get_phy, for multiport controller, only if SS-PHY phandle is present, pass proper SS_IDX else pass -1. Link to v7: https://lore.kernel.org/all/20230501143445.3851-1-quic_kriskura@quicinc.com/ Link to v6: https://lore.kernel.org/all/20230405125759.4201-1-quic_kriskura@quicinc.com/ Link to v5: https://lore.kernel.org/all/20230310163420.7582-1-quic_kriskura@quicinc.com/ Link to RFC v4: https://lore.kernel.org/all/20230115114146.12628-1-quic_kriskura@quicinc.com/ Link to RFC v3: https://lore.kernel.org/all/1654709787-23686-1-git-send-email-quic_harshq@quicinc.com/#r Link to RFC v2: https://lore.kernel.org/all/1653560029-6937-1-git-send-email-quic_harshq@quicinc.com/#r Test results: Bus 3/4 represent multiport controller having 4 HS ports and 2 SS ports. / # dmesg |grep hub [ 0.029029] usbcore: registered new interface driver hub [ 1.372812] hub 1-0:1.0: USB hub found [ 1.389142] hub 1-0:1.0: 1 port detected [ 1.414721] hub 2-0:1.0: USB hub found [ 1.427669] hub 2-0:1.0: 1 port detected [ 2.931465] hub 3-0:1.0: USB hub found [ 2.935340] hub 3-0:1.0: 4 ports detected [ 2.948721] hub 4-0:1.0: USB hub found [ 2.952604] hub 4-0:1.0: 2 ports detected / # / # lsusb Bus 003 Device 001: ID 1d6b:0002 Bus 001 Device 001: ID 1d6b:0002 Bus 003 Device 005: ID 0b0e:0300 Bus 003 Device 002: ID 046d:c077 Bus 004 Device 001: ID 1d6b:0003 Bus 002 Device 001: ID 1d6b:0003 Bus 003 Device 004: ID 03f0:0024 Bus 003 Device 003: ID 046d:c016 Krishna Kurapati (9): dt-bindings: usb: qcom,dwc3: Add bindings for SC8280 Multiport dt-bindings: usb: Add bindings for multiport properties on DWC3 controller usb: dwc3: core: Access XHCI address space temporarily to read port info usb: dwc3: core: Skip setting event buffers for host only controllers usb: dwc3: core: Refactor PHY logic to support Multiport Controller usb: dwc3: qcom: Add multiport controller support for qcom wrapper arm64: dts: qcom: sc8280xp: Add multiport controller node for SC8280 arm64: dts: qcom: sa8295p: Enable tertiary controller and its 4 USB ports arm64: dts: qcom: sa8540-ride: Enable first port of tertiary usb controller .../devicetree/bindings/usb/qcom,dwc3.yaml | 22 + .../devicetree/bindings/usb/snps,dwc3.yaml | 13 +- arch/arm64/boot/dts/qcom/sa8295p-adp.dts | 52 +++ arch/arm64/boot/dts/qcom/sa8540p-ride.dts | 22 + arch/arm64/boot/dts/qcom/sc8280xp.dtsi | 66 +++ drivers/usb/dwc3/core.c | 389 +++++++++++++++--- drivers/usb/dwc3/core.h | 28 +- drivers/usb/dwc3/drd.c | 13 +- drivers/usb/dwc3/dwc3-qcom.c | 28 +- 9 files changed, 543 insertions(+), 90 deletions(-)