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:15AM +0530, Krishna Kurapati wrote: > Add USB and DWC3 node for tertiary port of SC8280 along with multiport > IRQ's and phy's. This will be used as a base for SA8295P and SA8295-Ride > platforms. > > Signed-off-by: Krishna Kurapati <quic_kriskura@quicinc.com> > --- > arch/arm64/boot/dts/qcom/sc8280xp.dtsi | 66 ++++++++++++++++++++++++++ > 1 file changed, 66 insertions(+) > > diff --git a/arch/arm64/boot/dts/qcom/sc8280xp.dtsi b/arch/arm64/boot/dts/qcom/sc8280xp.dtsi > index 8fa9fbfe5d00..50f6a8424537 100644 > --- a/arch/arm64/boot/dts/qcom/sc8280xp.dtsi > +++ b/arch/arm64/boot/dts/qcom/sc8280xp.dtsi > @@ -3133,6 +3133,72 @@ usb_1_role_switch: endpoint { > }; > }; > > + usb_2: usb@a4f8800 { As I believe someone already pointed out, this node is not in sort order (i.e. it should go before usb@a6f8800). > + compatible = "qcom,sc8280xp-dwc3-mp", "qcom,dwc3"; > + reg = <0 0x0a4f8800 0 0x400>; > + #address-cells = <2>; > + #size-cells = <2>; > + ranges; > + > + clocks = <&gcc GCC_CFG_NOC_USB3_MP_AXI_CLK>, > + <&gcc GCC_USB30_MP_MASTER_CLK>, > + <&gcc GCC_AGGRE_USB3_MP_AXI_CLK>, > + <&gcc GCC_USB30_MP_SLEEP_CLK>, > + <&gcc GCC_USB30_MP_MOCK_UTMI_CLK>, > + <&gcc GCC_AGGRE_USB_NOC_AXI_CLK>, > + <&gcc GCC_AGGRE_USB_NOC_NORTH_AXI_CLK>, > + <&gcc GCC_AGGRE_USB_NOC_SOUTH_AXI_CLK>, > + <&gcc GCC_SYS_NOC_USB_AXI_CLK>; > + clock-names = "cfg_noc", "core", "iface", "sleep", "mock_utmi", > + "noc_aggr", "noc_aggr_north", "noc_aggr_south", "noc_sys"; > + > + assigned-clocks = <&gcc GCC_USB30_MP_MOCK_UTMI_CLK>, > + <&gcc GCC_USB30_MP_MASTER_CLK>; > + assigned-clock-rates = <19200000>, <200000000>; > + > + interrupts-extended = <&pdc 127 IRQ_TYPE_EDGE_RISING>, > + <&pdc 126 IRQ_TYPE_EDGE_RISING>, > + <&pdc 16 IRQ_TYPE_LEVEL_HIGH>, > + <&intc GIC_SPI 130 IRQ_TYPE_LEVEL_HIGH>, > + <&intc GIC_SPI 135 IRQ_TYPE_LEVEL_HIGH>, > + <&intc GIC_SPI 857 IRQ_TYPE_LEVEL_HIGH>, > + <&intc GIC_SPI 856 IRQ_TYPE_LEVEL_HIGH>; > + > + interrupt-names = "dp_hs_phy_irq", > + "dm_hs_phy_irq", > + "ss_phy_irq", > + "pwr_event_1", > + "pwr_event_2", > + "pwr_event_3", > + "pwr_event_4"; > + > + power-domains = <&gcc USB30_MP_GDSC>; > + required-opps = <&rpmhpd_opp_nom>; > + > + resets = <&gcc GCC_USB30_MP_BCR>; > + > + interconnects = <&aggre1_noc MASTER_USB3_1 0 &mc_virt SLAVE_EBI1 0>, > + <&gem_noc MASTER_APPSS_PROC 0 &config_noc SLAVE_USB3_1 0>; This is not the correct interconnect master and slave; it should be MASTER_USB3_MP and SLAVE_USB3_MP. > + interconnect-names = "usb-ddr", "apps-usb"; Looks like 'wakeup-source' is missing here too. > + > + status = "disabled"; > + > + 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>; > + phys = <&usb_2_hsphy0>, <&usb_2_qmpphy0>, > + <&usb_2_hsphy1>, <&usb_2_qmpphy1>, > + <&usb_2_hsphy2>, > + <&usb_2_hsphy3>; > + phy-names = "usb2-port0", "usb3-port0", > + "usb2-port1", "usb3-port1", > + "usb2-port2", > + "usb2-port3"; The phys and phy-names continuation lines above are still not aligned. > + }; > + }; > + > mdss0: display-subsystem@ae00000 { > compatible = "qcom,sc8280xp-mdss"; > reg = <0 0x0ae00000 0 0x1000>; Johan
On 5/15/2023 7:56 PM, Johan Hovold wrote: > On Sun, May 14, 2023 at 11:19:15AM +0530, Krishna Kurapati wrote: >> Add USB and DWC3 node for tertiary port of SC8280 along with multiport >> IRQ's and phy's. This will be used as a base for SA8295P and SA8295-Ride >> platforms. >> >> Signed-off-by: Krishna Kurapati <quic_kriskura@quicinc.com> >> --- >> arch/arm64/boot/dts/qcom/sc8280xp.dtsi | 66 ++++++++++++++++++++++++++ >> 1 file changed, 66 insertions(+) >> >> diff --git a/arch/arm64/boot/dts/qcom/sc8280xp.dtsi b/arch/arm64/boot/dts/qcom/sc8280xp.dtsi >> index 8fa9fbfe5d00..50f6a8424537 100644 >> --- a/arch/arm64/boot/dts/qcom/sc8280xp.dtsi >> +++ b/arch/arm64/boot/dts/qcom/sc8280xp.dtsi >> @@ -3133,6 +3133,72 @@ usb_1_role_switch: endpoint { >> }; >> }; >> >> + usb_2: usb@a4f8800 { > > As I believe someone already pointed out, this node is not in sort order > (i.e. it should go before usb@a6f8800). > Hi Johan, I missed that message, but since I named it usb_2, so I placed it in order after usb_1. Hope that is fine !! >> + compatible = "qcom,sc8280xp-dwc3-mp", "qcom,dwc3"; >> + reg = <0 0x0a4f8800 0 0x400>; >> + #address-cells = <2>; >> + #size-cells = <2>; >> + ranges; >> + >> + clocks = <&gcc GCC_CFG_NOC_USB3_MP_AXI_CLK>, >> + <&gcc GCC_USB30_MP_MASTER_CLK>, >> + <&gcc GCC_AGGRE_USB3_MP_AXI_CLK>, >> + <&gcc GCC_USB30_MP_SLEEP_CLK>, >> + <&gcc GCC_USB30_MP_MOCK_UTMI_CLK>, >> + <&gcc GCC_AGGRE_USB_NOC_AXI_CLK>, >> + <&gcc GCC_AGGRE_USB_NOC_NORTH_AXI_CLK>, >> + <&gcc GCC_AGGRE_USB_NOC_SOUTH_AXI_CLK>, >> + <&gcc GCC_SYS_NOC_USB_AXI_CLK>; >> + clock-names = "cfg_noc", "core", "iface", "sleep", "mock_utmi", >> + "noc_aggr", "noc_aggr_north", "noc_aggr_south", "noc_sys"; >> + >> + assigned-clocks = <&gcc GCC_USB30_MP_MOCK_UTMI_CLK>, >> + <&gcc GCC_USB30_MP_MASTER_CLK>; >> + assigned-clock-rates = <19200000>, <200000000>; >> + >> + interrupts-extended = <&pdc 127 IRQ_TYPE_EDGE_RISING>, >> + <&pdc 126 IRQ_TYPE_EDGE_RISING>, >> + <&pdc 16 IRQ_TYPE_LEVEL_HIGH>, >> + <&intc GIC_SPI 130 IRQ_TYPE_LEVEL_HIGH>, >> + <&intc GIC_SPI 135 IRQ_TYPE_LEVEL_HIGH>, >> + <&intc GIC_SPI 857 IRQ_TYPE_LEVEL_HIGH>, >> + <&intc GIC_SPI 856 IRQ_TYPE_LEVEL_HIGH>; >> + >> + interrupt-names = "dp_hs_phy_irq", >> + "dm_hs_phy_irq", >> + "ss_phy_irq", >> + "pwr_event_1", >> + "pwr_event_2", >> + "pwr_event_3", >> + "pwr_event_4"; >> + >> + power-domains = <&gcc USB30_MP_GDSC>; >> + required-opps = <&rpmhpd_opp_nom>; >> + >> + resets = <&gcc GCC_USB30_MP_BCR>; >> + >> + interconnects = <&aggre1_noc MASTER_USB3_1 0 &mc_virt SLAVE_EBI1 0>, >> + <&gem_noc MASTER_APPSS_PROC 0 &config_noc SLAVE_USB3_1 0>; > > This is not the correct interconnect master and slave; it should be > MASTER_USB3_MP and SLAVE_USB3_MP. > Thanks for pointing it out. I need to check how it was working all these days. (Probably since both of them vote for the same NOC, it didn't show any affect) >> + interconnect-names = "usb-ddr", "apps-usb"; > > Looks like 'wakeup-source' is missing here too. > I believe this property was added to enable wakeup from system suspend in host mode. I didn't add this property as currently I don't need to support wakeup. If any requirement comes in future, then I might need to add dp/dm interrupts (if any) for other ports as well and then need to change driver code to enable/disable them on suspend/resume. >> + >> + status = "disabled"; >> + >> + 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>; >> + phys = <&usb_2_hsphy0>, <&usb_2_qmpphy0>, >> + <&usb_2_hsphy1>, <&usb_2_qmpphy1>, >> + <&usb_2_hsphy2>, >> + <&usb_2_hsphy3>; >> + phy-names = "usb2-port0", "usb3-port0", >> + "usb2-port1", "usb3-port1", >> + "usb2-port2", >> + "usb2-port3"; > > The phys and phy-names continuation lines above are still not aligned. > Missed it. Will fix it in next version. Thanks, Krishna, >> + }; >> + }; >> + >> mdss0: display-subsystem@ae00000 { >> compatible = "qcom,sc8280xp-mdss"; >> reg = <0 0x0ae00000 0 0x1000>; > > Johan
On Sun, May 14, 2023 at 11:19:11AM +0530, Krishna Kurapati wrote: > Currently host-only capable DWC3 controllers support Multiport. > Temporarily map XHCI address space for host-only controllers and parse > XHCI Extended Capabilities registers to read number of usb2 ports and > usb3 ports present on multiport controller. Each USB Port is at least HS > capable. > > The port info for usb2 and usb3 phy are identified as num_usb2_ports > and num_usb3_ports. The intention is as follows: > > Wherever we need to perform phy operations like: > > LOOP_OVER_NUMBER_OF_AVAILABLE_PORTS() > { > phy_set_mode(dwc->usb2_generic_phy[i], PHY_MODE_USB_HOST); > phy_set_mode(dwc->usb3_generic_phy[i], PHY_MODE_USB_HOST); > } > > If number of usb2 ports is 3, loop can go from index 0-2 for > usb2_generic_phy. If number of usb3-ports is 2, we don't know for sure, > if the first 2 ports are SS capable or some other ports like (2 and 3) > are SS capable. So instead, num_usb2_ports is used to loop around all > phy's (both hs and ss) for performing phy operations. If any > usb3_generic_phy turns out to be NULL, phy operation just bails out. > > num_usb3_ports is used to modify GUSB3PIPECTL registers while setting up > phy's as we need to know how many SS capable ports are there for this. > > Signed-off-by: Krishna Kurapati <quic_kriskura@quicinc.com> > --- > drivers/usb/dwc3/core.c | 113 ++++++++++++++++++++++++++++++++++++++++ > drivers/usb/dwc3/core.h | 17 +++++- > 2 files changed, 129 insertions(+), 1 deletion(-) > > diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c > index 0beaab932e7d..e983aef1fb93 100644 > --- a/drivers/usb/dwc3/core.c > +++ b/drivers/usb/dwc3/core.c > @@ -1767,6 +1767,104 @@ static int dwc3_get_clocks(struct dwc3 *dwc) > return 0; > } > > +/** > + * dwc3_xhci_find_next_ext_cap - Find the offset of the extended capabilities > + * with capability ID id. () after function name in kernel-doc > + * > + * @base: PCI MMIO registers base address. Should this be "XHCI MMIO..."? > + * @start: address at which to start looking, (0 or HCC_PARAMS to start at > + * beginning of list) > + * @id: Extended capability ID to search for, or 0 for the next > + * capability > + * > + * Returns the offset of the next matching extended capability structure. Return: The offset... Per https://www.kernel.org/doc/html/next/doc-guide/kernel-doc.html. > + * Some capabilities can occur several times, e.g., the XHCI_EXT_CAPS_PROTOCOL, > + * and this provides a way to find them all. > + */ > +static int dwc3_xhci_find_next_ext_cap(void __iomem *base, u32 start, int id) > +{ > + u32 val; > + u32 next; > + u32 offset; > + > + offset = start; > + if (!start || start == XHCI_HCC_PARAMS_OFFSET) { > + val = readl(base + XHCI_HCC_PARAMS_OFFSET); > + if (val == ~0) > + return 0; > + offset = XHCI_HCC_EXT_CAPS(val) << 2; > + if (!offset) > + return 0; > + } > + do { > + val = readl(base + offset); > + if (val == ~0) > + return 0; > + if (offset != start && (id == 0 || XHCI_EXT_CAPS_ID(val) == id)) > + return offset; > + > + next = XHCI_EXT_CAPS_NEXT(val); > + offset += next << 2; > + } while (next); > + > + return 0; > +} > + > +static int dwc3_read_port_info(struct dwc3 *dwc) > +{ > + void __iomem *regs; > + u32 offset; > + u32 temp; > + u8 major_revision; > + int ret = 0; Please drop the spacing between type and variable name here, if nothing else it's inconsistent with the previous function. Regards, Bjorn > + > + /* > + * Remap xHCI address space to access XHCI ext cap regs, > + * since it is needed to get port info. > + */ > + regs = ioremap(dwc->xhci_resources[0].start, > + resource_size(&dwc->xhci_resources[0])); > + if (IS_ERR(regs)) > + return PTR_ERR(regs); > + > + offset = dwc3_xhci_find_next_ext_cap(regs, 0, > + XHCI_EXT_CAPS_PROTOCOL); > + while (offset) { > + temp = readl(regs + offset); > + major_revision = XHCI_EXT_PORT_MAJOR(temp); > + > + temp = readl(regs + offset + 0x08); > + if (major_revision == 0x03) { > + dwc->num_usb3_ports += XHCI_EXT_PORT_COUNT(temp); > + } else if (major_revision <= 0x02) { > + dwc->num_usb2_ports += XHCI_EXT_PORT_COUNT(temp); > + } else { > + dev_err(dwc->dev, > + "Unrecognized port major revision %d\n", major_revision); > + ret = -EINVAL; > + goto unmap_reg; > + } > + > + offset = dwc3_xhci_find_next_ext_cap(regs, offset, > + XHCI_EXT_CAPS_PROTOCOL); > + } > + > + temp = readl(regs + DWC3_XHCI_HCSPARAMS1); > + if (HCS_MAX_PORTS(temp) != (dwc->num_usb3_ports + dwc->num_usb2_ports)) { > + dev_err(dwc->dev, > + "Mismatched reported MAXPORTS (%d)\n", HCS_MAX_PORTS(temp)); > + ret = -EINVAL; > + goto unmap_reg; > + } > + > + dev_dbg(dwc->dev, > + "hs-ports: %d ss-ports: %d\n", dwc->num_usb2_ports, dwc->num_usb3_ports); > + > +unmap_reg: > + iounmap(regs); > + return ret; > +} > + > static int dwc3_probe(struct platform_device *pdev) > { > struct device *dev = &pdev->dev; > @@ -1774,6 +1872,7 @@ static int dwc3_probe(struct platform_device *pdev) > void __iomem *regs; > struct dwc3 *dwc; > int ret; > + unsigned int hw_mode; > > dwc = devm_kzalloc(dev, sizeof(*dwc), GFP_KERNEL); > if (!dwc) > @@ -1843,6 +1942,20 @@ static int dwc3_probe(struct platform_device *pdev) > goto err_disable_clks; > } > > + /* > + * Currently DWC3 controllers that are host-only capable > + * support Multiport > + */ > + hw_mode = DWC3_GHWPARAMS0_MODE(dwc->hwparams.hwparams0); > + if (hw_mode == DWC3_GHWPARAMS0_MODE_HOST) { > + ret = dwc3_read_port_info(dwc); > + if (ret) > + goto err_disable_clks; > + } else { > + dwc->num_usb2_ports = 1; > + dwc->num_usb3_ports = 1; > + } > + > spin_lock_init(&dwc->lock); > mutex_init(&dwc->mutex); > > diff --git a/drivers/usb/dwc3/core.h b/drivers/usb/dwc3/core.h > index d56457c02996..d3401963bc27 100644 > --- a/drivers/usb/dwc3/core.h > +++ b/drivers/usb/dwc3/core.h > @@ -35,6 +35,17 @@ > > #define DWC3_MSG_MAX 500 > > +/* Define XHCI Extcap register offsets for getting multiport info */ > +#define XHCI_HCC_PARAMS_OFFSET 0x10 > +#define DWC3_XHCI_HCSPARAMS1 0x04 > +#define XHCI_EXT_CAPS_PROTOCOL 2 > +#define XHCI_HCC_EXT_CAPS(x) (((x) >> 16) & 0xffff) > +#define XHCI_EXT_CAPS_ID(x) (((x) >> 0) & 0xff) > +#define XHCI_EXT_CAPS_NEXT(x) (((x) >> 8) & 0xff) > +#define XHCI_EXT_PORT_MAJOR(x) (((x) >> 24) & 0xff) > +#define XHCI_EXT_PORT_COUNT(x) (((x) >> 8) & 0xff) > +#define HCS_MAX_PORTS(x) (((x) >> 24) & 0x7f) > + > /* Global constants */ > #define DWC3_PULL_UP_TIMEOUT 500 /* ms */ > #define DWC3_BOUNCE_SIZE 1024 /* size of a superspeed bulk */ > @@ -1025,6 +1036,8 @@ struct dwc3_scratchpad_array { > * @usb_psy: pointer to power supply interface. > * @usb2_phy: pointer to USB2 PHY > * @usb3_phy: pointer to USB3 PHY > + * @num_usb2_ports: number of usb2 ports. > + * @num_usb3_ports: number of usb3 ports. > * @usb2_generic_phy: pointer to USB2 PHY > * @usb3_generic_phy: pointer to USB3 PHY > * @phys_ready: flag to indicate that PHYs are ready > @@ -1162,6 +1175,9 @@ struct dwc3 { > struct usb_phy *usb2_phy; > struct usb_phy *usb3_phy; > > + u8 num_usb2_ports; > + u8 num_usb3_ports; > + > struct phy *usb2_generic_phy; > struct phy *usb3_generic_phy; > > @@ -1649,5 +1665,4 @@ static inline int dwc3_ulpi_init(struct dwc3 *dwc) > static inline void dwc3_ulpi_exit(struct dwc3 *dwc) > { } > #endif > - > #endif /* __DRIVERS_USB_DWC3_CORE_H */ > -- > 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:13AM +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. > > Co-developed-by: Harsh Agarwal <quic_harshq@quicinc.com> Please include Harsh's signed-off-by as well here, to clarify that you both certify the origin of this patch. > Signed-off-by: Krishna Kurapati <quic_kriskura@quicinc.com> > --- > drivers/usb/dwc3/core.c | 266 ++++++++++++++++++++++++++++++---------- > drivers/usb/dwc3/core.h | 11 +- > drivers/usb/dwc3/drd.c | 13 +- > 3 files changed, 213 insertions(+), 77 deletions(-) > > diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c [..] > @@ -744,22 +777,38 @@ static int dwc3_phy_setup(struct dwc3 *dwc) > static int dwc3_phy_init(struct dwc3 *dwc) > { > int ret; > + int i; > + int j; > > usb_phy_init(dwc->usb2_phy); > usb_phy_init(dwc->usb3_phy); > > - ret = phy_init(dwc->usb2_generic_phy); > - if (ret < 0) > - goto err_shutdown_usb3_phy; > + for (i = 0; i < dwc->num_usb2_ports; i++) { > + ret = phy_init(dwc->usb2_generic_phy[i]); > + if (ret < 0) { > + /* clean up prior initialized HS PHYs */ > + for (j = 0; j < i; j++) > + phy_exit(dwc->usb2_generic_phy[j]); > + goto err_shutdown_usb3_phy; The idiomatic form is to goto err_exit_usb2_phy and let it phy_exit() from i - 1 to 0. That would avoid duplicating this snippet. > + } > + } > > - ret = phy_init(dwc->usb3_generic_phy); > - if (ret < 0) > - goto err_exit_usb2_phy; > + for (i = 0; i < dwc->num_usb2_ports; i++) { When you call dwc3_ss_phy_setup() the index refer to port0, port1... but when you refer to the phys you consistently loops over num_usb2_ports. The only case I can think of where this would be useful is if it's not the first N ports that are SS-capable (e.g. port0 and port can do SS). If this is the case, is it correct that this should not be reflected in the index passed to e.g. dwc3_ss_phy_setup()? If this is not the case, could you please transition these SS-related loops to iterate over usb3_generic_phy[0..num_usb3_ports)? > + ret = phy_init(dwc->usb3_generic_phy[i]); > + if (ret < 0) { > + /* clean up prior initialized SS PHYs */ > + for (j = 0; j < i; j++) > + phy_exit(dwc->usb3_generic_phy[j]); > + goto err_exit_usb2_phy; For the purpose of symmetry, same suggestion as above. phy_exit() i - 1 through 0, then reset j to dwc->num_usb2_ports and fall through to err_exit_usb2_phy. > + } > + } > > return 0; > > err_exit_usb2_phy: > - phy_exit(dwc->usb2_generic_phy); > + for (i = 0; i < dwc->num_usb2_ports; i++) > + phy_exit(dwc->usb2_generic_phy[i]); > + > err_shutdown_usb3_phy: > usb_phy_shutdown(dwc->usb3_phy); > usb_phy_shutdown(dwc->usb2_phy); > @@ -769,8 +818,12 @@ static int dwc3_phy_init(struct dwc3 *dwc) [..] > static int dwc3_phy_power_on(struct dwc3 *dwc) > { > int ret; > + int i; > + int j; > > usb_phy_set_suspend(dwc->usb2_phy, 0); > usb_phy_set_suspend(dwc->usb3_phy, 0); > > - ret = phy_power_on(dwc->usb2_generic_phy); > - if (ret < 0) > - goto err_suspend_usb3_phy; > + for (i = 0; i < dwc->num_usb2_ports; i++) { > + ret = phy_power_on(dwc->usb2_generic_phy[i]); > + if (ret < 0) { > + /* Turn off prior ON'ed HS Phy's */ > + for (j = 0; j < i; j++) > + phy_power_off(dwc->usb2_generic_phy[j]); > + goto err_suspend_usb3_phy; As above, I'd prefer that you don't duplicate the phy_power_off() loop. > + } > + } > > - ret = phy_power_on(dwc->usb3_generic_phy); > - if (ret < 0) > - goto err_power_off_usb2_phy; > + for (i = 0; i < dwc->num_usb2_ports; i++) { > + ret = phy_power_on(dwc->usb3_generic_phy[i]); > + if (ret < 0) { > + /* Turn of prior ON'ed SS Phy's */ > + for (j = 0; j < i; j++) > + phy_power_off(dwc->usb3_generic_phy[j]); > + goto err_power_off_usb2_phy; > + } > + } > > return 0; [..] > +static int dwc3_get_multiport_phys(struct dwc3 *dwc) > +{ > + int ret; > + struct device *dev = dwc->dev; > + int i; > + char phy_name[11]; It would be prettier if you sorted these lines by length, longest first... [..] > diff --git a/drivers/usb/dwc3/core.h b/drivers/usb/dwc3/core.h > index d3401963bc27..84f6303922aa 100644 > --- a/drivers/usb/dwc3/core.h > +++ b/drivers/usb/dwc3/core.h > @@ -35,6 +35,9 @@ > > #define DWC3_MSG_MAX 500 > > +/* Number of ports supported by a multiport controller */ > +#define MAX_PORTS_SUPPORTED 4 I think it would be preferred to prefix this DWC3_ (so perhaps just DWC3_MAX_PORTS, to keep it shorter) Regards, Bjorn > + > /* Define XHCI Extcap register offsets for getting multiport info */ > #define XHCI_HCC_PARAMS_OFFSET 0x10 > #define DWC3_XHCI_HCSPARAMS1 0x04 > @@ -1038,8 +1041,8 @@ struct dwc3_scratchpad_array { > * @usb3_phy: pointer to USB3 PHY > * @num_usb2_ports: number of usb2 ports. > * @num_usb3_ports: number of usb3 ports. > - * @usb2_generic_phy: pointer to USB2 PHY > - * @usb3_generic_phy: pointer to USB3 PHY > + * @usb2_generic_phy: pointer to array of USB2 PHY > + * @usb3_generic_phy: pointer to array of USB3 PHY > * @phys_ready: flag to indicate that PHYs are ready > * @ulpi: pointer to ulpi interface > * @ulpi_ready: flag to indicate that ULPI is initialized > @@ -1178,8 +1181,8 @@ struct dwc3 { > u8 num_usb2_ports; > u8 num_usb3_ports; > > - struct phy *usb2_generic_phy; > - struct phy *usb3_generic_phy; > + struct phy *usb2_generic_phy[MAX_PORTS_SUPPORTED]; > + struct phy *usb3_generic_phy[MAX_PORTS_SUPPORTED]; > > bool phys_ready;
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 2:38 AM, Bjorn Andersson wrote: > On Sun, May 14, 2023 at 11:19:11AM +0530, Krishna Kurapati wrote: >> Currently host-only capable DWC3 controllers support Multiport. >> Temporarily map XHCI address space for host-only controllers and parse >> XHCI Extended Capabilities registers to read number of usb2 ports and >> usb3 ports present on multiport controller. Each USB Port is at least HS >> capable. >> >> The port info for usb2 and usb3 phy are identified as num_usb2_ports >> and num_usb3_ports. The intention is as follows: >> >> Wherever we need to perform phy operations like: >> >> LOOP_OVER_NUMBER_OF_AVAILABLE_PORTS() >> { >> phy_set_mode(dwc->usb2_generic_phy[i], PHY_MODE_USB_HOST); >> phy_set_mode(dwc->usb3_generic_phy[i], PHY_MODE_USB_HOST); >> } >> >> If number of usb2 ports is 3, loop can go from index 0-2 for >> usb2_generic_phy. If number of usb3-ports is 2, we don't know for sure, >> if the first 2 ports are SS capable or some other ports like (2 and 3) >> are SS capable. So instead, num_usb2_ports is used to loop around all >> phy's (both hs and ss) for performing phy operations. If any >> usb3_generic_phy turns out to be NULL, phy operation just bails out. >> >> num_usb3_ports is used to modify GUSB3PIPECTL registers while setting up >> phy's as we need to know how many SS capable ports are there for this. >> >> Signed-off-by: Krishna Kurapati <quic_kriskura@quicinc.com> >> --- >> drivers/usb/dwc3/core.c | 113 ++++++++++++++++++++++++++++++++++++++++ >> drivers/usb/dwc3/core.h | 17 +++++- >> 2 files changed, 129 insertions(+), 1 deletion(-) >> >> diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c >> index 0beaab932e7d..e983aef1fb93 100644 >> --- a/drivers/usb/dwc3/core.c >> +++ b/drivers/usb/dwc3/core.c >> @@ -1767,6 +1767,104 @@ static int dwc3_get_clocks(struct dwc3 *dwc) >> return 0; >> } >> >> +/** >> + * dwc3_xhci_find_next_ext_cap - Find the offset of the extended capabilities >> + * with capability ID id. > > () after function name in kernel-doc > >> + * >> + * @base: PCI MMIO registers base address. > > Should this be "XHCI MMIO..."? Hi Bjorn, I copied this code from xhci-ext-caps.h. The documentation of this function mentioned PCI in that file. May be Thinh/Mathias can correct us if this is wrong. > >> + * @start: address at which to start looking, (0 or HCC_PARAMS to start at >> + * beginning of list) >> + * @id: Extended capability ID to search for, or 0 for the next >> + * capability >> + * >> + * Returns the offset of the next matching extended capability structure. > > Return: The offset... > > Per https://www.kernel.org/doc/html/next/doc-guide/kernel-doc.html. > I executed the following command and it didn't give any errors: ./scripts/kernel-doc -none -Werror -function dwc3_xhci_find_next_ext_cap drivers/usb/dwc3/core.c I see that even for dwc3_core_init the comments are the same: /** * dwc3_core_init - Low-level initialization of DWC3 Core * @dwc: Pointer to our controller context structure * * Returns 0 on success otherwise negative errno. */ >> + * Some capabilities can occur several times, e.g., the XHCI_EXT_CAPS_PROTOCOL, >> + * and this provides a way to find them all. >> + */ >> +static int dwc3_xhci_find_next_ext_cap(void __iomem *base, u32 start, int id) >> +{ >> + u32 val; >> + u32 next; >> + u32 offset; >> + >> + offset = start; >> + if (!start || start == XHCI_HCC_PARAMS_OFFSET) { >> + val = readl(base + XHCI_HCC_PARAMS_OFFSET); >> + if (val == ~0) >> + return 0; >> + offset = XHCI_HCC_EXT_CAPS(val) << 2; >> + if (!offset) >> + return 0; >> + } >> + do { >> + val = readl(base + offset); >> + if (val == ~0) >> + return 0; >> + if (offset != start && (id == 0 || XHCI_EXT_CAPS_ID(val) == id)) >> + return offset; >> + >> + next = XHCI_EXT_CAPS_NEXT(val); >> + offset += next << 2; >> + } while (next); >> + >> + return 0; >> +} >> + >> +static int dwc3_read_port_info(struct dwc3 *dwc) >> +{ >> + void __iomem *regs; >> + u32 offset; >> + u32 temp; >> + u8 major_revision; >> + int ret = 0; > > Please drop the spacing between type and variable name here, if nothing > else it's inconsistent with the previous function. > Sure, will fix this nit. Thanks, Krishna, >> + >> + /* >> + * Remap xHCI address space to access XHCI ext cap regs, >> + * since it is needed to get port info. >> + */ >> + regs = ioremap(dwc->xhci_resources[0].start, >> + resource_size(&dwc->xhci_resources[0])); >> + if (IS_ERR(regs)) >> + return PTR_ERR(regs); >> + >> + offset = dwc3_xhci_find_next_ext_cap(regs, 0, >> + XHCI_EXT_CAPS_PROTOCOL); >> + while (offset) { >> + temp = readl(regs + offset); >> + major_revision = XHCI_EXT_PORT_MAJOR(temp); >> + >> + temp = readl(regs + offset + 0x08); >> + if (major_revision == 0x03) { >> + dwc->num_usb3_ports += XHCI_EXT_PORT_COUNT(temp); >> + } else if (major_revision <= 0x02) { >> + dwc->num_usb2_ports += XHCI_EXT_PORT_COUNT(temp); >> + } else { >> + dev_err(dwc->dev, >> + "Unrecognized port major revision %d\n", major_revision); >> + ret = -EINVAL; >> + goto unmap_reg; >> + } >> + >> + offset = dwc3_xhci_find_next_ext_cap(regs, offset, >> + XHCI_EXT_CAPS_PROTOCOL); >> + } >> + >> + temp = readl(regs + DWC3_XHCI_HCSPARAMS1); >> + if (HCS_MAX_PORTS(temp) != (dwc->num_usb3_ports + dwc->num_usb2_ports)) { >> + dev_err(dwc->dev, >> + "Mismatched reported MAXPORTS (%d)\n", HCS_MAX_PORTS(temp)); >> + ret = -EINVAL; >> + goto unmap_reg; >> + } >> + >> + dev_dbg(dwc->dev, >> + "hs-ports: %d ss-ports: %d\n", dwc->num_usb2_ports, dwc->num_usb3_ports); >> + >> +unmap_reg: >> + iounmap(regs); >> + return ret; >> +} >> + >> static int dwc3_probe(struct platform_device *pdev) >> { >> struct device *dev = &pdev->dev; >> @@ -1774,6 +1872,7 @@ static int dwc3_probe(struct platform_device *pdev) >> void __iomem *regs; >> struct dwc3 *dwc; >> int ret; >> + unsigned int hw_mode; >> >> dwc = devm_kzalloc(dev, sizeof(*dwc), GFP_KERNEL); >> if (!dwc) >> @@ -1843,6 +1942,20 @@ static int dwc3_probe(struct platform_device *pdev) >> goto err_disable_clks; >> } >> >> + /* >> + * Currently DWC3 controllers that are host-only capable >> + * support Multiport >> + */ >> + hw_mode = DWC3_GHWPARAMS0_MODE(dwc->hwparams.hwparams0); >> + if (hw_mode == DWC3_GHWPARAMS0_MODE_HOST) { >> + ret = dwc3_read_port_info(dwc); >> + if (ret) >> + goto err_disable_clks; >> + } else { >> + dwc->num_usb2_ports = 1; >> + dwc->num_usb3_ports = 1; >> + } >> + >> spin_lock_init(&dwc->lock); >> mutex_init(&dwc->mutex); >> >> diff --git a/drivers/usb/dwc3/core.h b/drivers/usb/dwc3/core.h >> index d56457c02996..d3401963bc27 100644 >> --- a/drivers/usb/dwc3/core.h >> +++ b/drivers/usb/dwc3/core.h >> @@ -35,6 +35,17 @@ >> >> #define DWC3_MSG_MAX 500 >> >> +/* Define XHCI Extcap register offsets for getting multiport info */ >> +#define XHCI_HCC_PARAMS_OFFSET 0x10 >> +#define DWC3_XHCI_HCSPARAMS1 0x04 >> +#define XHCI_EXT_CAPS_PROTOCOL 2 >> +#define XHCI_HCC_EXT_CAPS(x) (((x) >> 16) & 0xffff) >> +#define XHCI_EXT_CAPS_ID(x) (((x) >> 0) & 0xff) >> +#define XHCI_EXT_CAPS_NEXT(x) (((x) >> 8) & 0xff) >> +#define XHCI_EXT_PORT_MAJOR(x) (((x) >> 24) & 0xff) >> +#define XHCI_EXT_PORT_COUNT(x) (((x) >> 8) & 0xff) >> +#define HCS_MAX_PORTS(x) (((x) >> 24) & 0x7f) >> + >> /* Global constants */ >> #define DWC3_PULL_UP_TIMEOUT 500 /* ms */ >> #define DWC3_BOUNCE_SIZE 1024 /* size of a superspeed bulk */ >> @@ -1025,6 +1036,8 @@ struct dwc3_scratchpad_array { >> * @usb_psy: pointer to power supply interface. >> * @usb2_phy: pointer to USB2 PHY >> * @usb3_phy: pointer to USB3 PHY >> + * @num_usb2_ports: number of usb2 ports. >> + * @num_usb3_ports: number of usb3 ports. >> * @usb2_generic_phy: pointer to USB2 PHY >> * @usb3_generic_phy: pointer to USB3 PHY >> * @phys_ready: flag to indicate that PHYs are ready >> @@ -1162,6 +1175,9 @@ struct dwc3 { >> struct usb_phy *usb2_phy; >> struct usb_phy *usb3_phy; >> >> + u8 num_usb2_ports; >> + u8 num_usb3_ports; >> + >> struct phy *usb2_generic_phy; >> struct phy *usb3_generic_phy; >> >> @@ -1649,5 +1665,4 @@ static inline int dwc3_ulpi_init(struct dwc3 *dwc) >> static inline void dwc3_ulpi_exit(struct dwc3 *dwc) >> { } >> #endif >> - >> #endif /* __DRIVERS_USB_DWC3_CORE_H */ >> -- >> 2.40.0 >>
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 5/16/2023 3:17 AM, Bjorn Andersson wrote: > On Sun, May 14, 2023 at 11:19:13AM +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. >> >> Co-developed-by: Harsh Agarwal <quic_harshq@quicinc.com> > > Please include Harsh's signed-off-by as well here, to clarify that you > both certify the origin of this patch. > >> Signed-off-by: Krishna Kurapati <quic_kriskura@quicinc.com> >> --- >> drivers/usb/dwc3/core.c | 266 ++++++++++++++++++++++++++++++---------- >> drivers/usb/dwc3/core.h | 11 +- >> drivers/usb/dwc3/drd.c | 13 +- >> 3 files changed, 213 insertions(+), 77 deletions(-) >> >> diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c > [..] >> @@ -744,22 +777,38 @@ static int dwc3_phy_setup(struct dwc3 *dwc) >> static int dwc3_phy_init(struct dwc3 *dwc) >> { >> int ret; >> + int i; >> + int j; >> >> usb_phy_init(dwc->usb2_phy); >> usb_phy_init(dwc->usb3_phy); >> >> - ret = phy_init(dwc->usb2_generic_phy); >> - if (ret < 0) >> - goto err_shutdown_usb3_phy; >> + for (i = 0; i < dwc->num_usb2_ports; i++) { >> + ret = phy_init(dwc->usb2_generic_phy[i]); >> + if (ret < 0) { >> + /* clean up prior initialized HS PHYs */ >> + for (j = 0; j < i; j++) >> + phy_exit(dwc->usb2_generic_phy[j]); >> + goto err_shutdown_usb3_phy; > > The idiomatic form is to goto err_exit_usb2_phy and let it phy_exit() > from i - 1 to 0. That would avoid duplicating this snippet. > >> + } >> + } >> >> - ret = phy_init(dwc->usb3_generic_phy); >> - if (ret < 0) >> - goto err_exit_usb2_phy; >> + for (i = 0; i < dwc->num_usb2_ports; i++) { > > When you call dwc3_ss_phy_setup() the index refer to port0, port1... but > when you refer to the phys you consistently loops over num_usb2_ports. > > The only case I can think of where this would be useful is if it's not > the first N ports that are SS-capable (e.g. port0 and port can do SS). > > If this is the case, is it correct that this should not be reflected in > the index passed to e.g. dwc3_ss_phy_setup()? > > If this is not the case, could you please transition these SS-related > loops to iterate over usb3_generic_phy[0..num_usb3_ports)? Hi Bjorn, The reason why I used num_usb2_ports here and num_usb3_ports in ss_phy_setup is that, there may be cases where not the ones in the beginning but the last few ports of multiport controller are SS capable. In that case trying to keep track of them and accordingly executing phy operations is difficult. Instead since all ports are atleast HS capable and calling a Phy op on a non-existent SS Phy is still harmless as the phy pointer is NULL and phy op's return 0. Coming to ss_phy_setup, we need to modify the GUSB3PIPECTL registers which are same as number of ss ports present. And modifying these registers whether or not the Phy is used is completely fine as it is done even in today's code (dwc3_phy_setup). In this case, if there are 2 SS Phy's we need to loop over and modify GUSB3PIPECTL(0) and GUSB3PIPECTL(1) and so we need to use num_usb3_ports. For the above two reasons I used num_usb2_ports wherever I did phy operations and num_usb3_ports wherever GUSB3PIPECTL is being modified like I did in dwc3_core_init as well. > >> + ret = phy_init(dwc->usb3_generic_phy[i]); >> + if (ret < 0) { >> + /* clean up prior initialized SS PHYs */ >> + for (j = 0; j < i; j++) >> + phy_exit(dwc->usb3_generic_phy[j]); >> + goto err_exit_usb2_phy; > > For the purpose of symmetry, same suggestion as above. phy_exit() i - 1 > through 0, then reset j to dwc->num_usb2_ports and fall through to > err_exit_usb2_phy. I will try to set i/j to proper values to cleanup code appropriately. But isn't this fine for now as I don't want to make too many changes in next version. I will make sure to cleanup this section in a separate patch after this series if that is fine. >> + } >> + } >> >> return 0; >> >> err_exit_usb2_phy: >> - phy_exit(dwc->usb2_generic_phy); >> + for (i = 0; i < dwc->num_usb2_ports; i++) >> + phy_exit(dwc->usb2_generic_phy[i]); >> + >> err_shutdown_usb3_phy: >> usb_phy_shutdown(dwc->usb3_phy); >> usb_phy_shutdown(dwc->usb2_phy); >> @@ -769,8 +818,12 @@ static int dwc3_phy_init(struct dwc3 *dwc) > [..] >> static int dwc3_phy_power_on(struct dwc3 *dwc) >> { >> int ret; >> + int i; >> + int j; >> >> usb_phy_set_suspend(dwc->usb2_phy, 0); >> usb_phy_set_suspend(dwc->usb3_phy, 0); >> >> - ret = phy_power_on(dwc->usb2_generic_phy); >> - if (ret < 0) >> - goto err_suspend_usb3_phy; >> + for (i = 0; i < dwc->num_usb2_ports; i++) { >> + ret = phy_power_on(dwc->usb2_generic_phy[i]); >> + if (ret < 0) { >> + /* Turn off prior ON'ed HS Phy's */ >> + for (j = 0; j < i; j++) >> + phy_power_off(dwc->usb2_generic_phy[j]); >> + goto err_suspend_usb3_phy; > > As above, I'd prefer that you don't duplicate the phy_power_off() loop. > >> + } >> + } >> >> - ret = phy_power_on(dwc->usb3_generic_phy); >> - if (ret < 0) >> - goto err_power_off_usb2_phy; >> + for (i = 0; i < dwc->num_usb2_ports; i++) { >> + ret = phy_power_on(dwc->usb3_generic_phy[i]); >> + if (ret < 0) { >> + /* Turn of prior ON'ed SS Phy's */ >> + for (j = 0; j < i; j++) >> + phy_power_off(dwc->usb3_generic_phy[j]); >> + goto err_power_off_usb2_phy; >> + } >> + } >> >> return 0; > [..] >> +static int dwc3_get_multiport_phys(struct dwc3 *dwc) >> +{ >> + int ret; >> + struct device *dev = dwc->dev; >> + int i; >> + char phy_name[11]; > > It would be prettier if you sorted these lines by length, longest > first... > Sure, will fix this nit. > [..] >> diff --git a/drivers/usb/dwc3/core.h b/drivers/usb/dwc3/core.h >> index d3401963bc27..84f6303922aa 100644 >> --- a/drivers/usb/dwc3/core.h >> +++ b/drivers/usb/dwc3/core.h >> @@ -35,6 +35,9 @@ >> >> #define DWC3_MSG_MAX 500 >> >> +/* Number of ports supported by a multiport controller */ >> +#define MAX_PORTS_SUPPORTED 4 > > I think it would be preferred to prefix this DWC3_ (so perhaps just > DWC3_MAX_PORTS, to keep it shorter) Sure, will fix this nit. Thanks, Krishna, > >> + >> /* Define XHCI Extcap register offsets for getting multiport info */ >> #define XHCI_HCC_PARAMS_OFFSET 0x10 >> #define DWC3_XHCI_HCSPARAMS1 0x04 >> @@ -1038,8 +1041,8 @@ struct dwc3_scratchpad_array { >> * @usb3_phy: pointer to USB3 PHY >> * @num_usb2_ports: number of usb2 ports. >> * @num_usb3_ports: number of usb3 ports. >> - * @usb2_generic_phy: pointer to USB2 PHY >> - * @usb3_generic_phy: pointer to USB3 PHY >> + * @usb2_generic_phy: pointer to array of USB2 PHY >> + * @usb3_generic_phy: pointer to array of USB3 PHY >> * @phys_ready: flag to indicate that PHYs are ready >> * @ulpi: pointer to ulpi interface >> * @ulpi_ready: flag to indicate that ULPI is initialized >> @@ -1178,8 +1181,8 @@ struct dwc3 { >> u8 num_usb2_ports; >> u8 num_usb3_ports; >> >> - struct phy *usb2_generic_phy; >> - struct phy *usb3_generic_phy; >> + struct phy *usb2_generic_phy[MAX_PORTS_SUPPORTED]; >> + struct phy *usb3_generic_phy[MAX_PORTS_SUPPORTED]; >> >> bool phys_ready; >
On Mon, May 15, 2023 at 09:02:13PM +0530, Krishna Kurapati PSSNV wrote: > On 5/15/2023 7:56 PM, Johan Hovold wrote: > > On Sun, May 14, 2023 at 11:19:15AM +0530, Krishna Kurapati wrote: > >> @@ -3133,6 +3133,72 @@ usb_1_role_switch: endpoint { > >> }; > >> }; > >> > >> + usb_2: usb@a4f8800 { > > > > As I believe someone already pointed out, this node is not in sort order > > (i.e. it should go before usb@a6f8800). > I missed that message, but since I named it usb_2, so I placed it in > order after usb_1. Hope that is fine !! No, the nodes should be sorted by unit address so you need to move it. > >> + interrupts-extended = <&pdc 127 IRQ_TYPE_EDGE_RISING>, > >> + <&pdc 126 IRQ_TYPE_EDGE_RISING>, > >> + <&pdc 16 IRQ_TYPE_LEVEL_HIGH>, > >> + <&intc GIC_SPI 130 IRQ_TYPE_LEVEL_HIGH>, > >> + <&intc GIC_SPI 135 IRQ_TYPE_LEVEL_HIGH>, > >> + <&intc GIC_SPI 857 IRQ_TYPE_LEVEL_HIGH>, > >> + <&intc GIC_SPI 856 IRQ_TYPE_LEVEL_HIGH>; > >> + > >> + interrupt-names = "dp_hs_phy_irq", > >> + "dm_hs_phy_irq", > >> + "ss_phy_irq", > >> + "pwr_event_1", > >> + "pwr_event_2", > >> + "pwr_event_3", > >> + "pwr_event_4"; > >> + interconnect-names = "usb-ddr", "apps-usb"; > > > > Looks like 'wakeup-source' is missing here too. > > > > I believe this property was added to enable wakeup from system suspend > in host mode. I didn't add this property as currently I don't need to > support wakeup. If any requirement comes in future, then I might need to > add dp/dm interrupts (if any) for other ports as well and then need to > change driver code to enable/disable them on suspend/resume. If there are dp/dm/ss interrupts per ports then those need to be defined in the binding and devicetree from the start. Similar for 'wakeup-source' which indicates that the controller *can* be used to wakeup the system from suspend (which those pdc interrupts indicates). Remember that the devicetree is supposed to describe the hardware, and which features are currently supported in some version of software is mostly irrelevant. Johan
On Sun, May 14, 2023 at 11:19:11AM +0530, Krishna Kurapati wrote: > Currently host-only capable DWC3 controllers support Multiport. > Temporarily map XHCI address space for host-only controllers and parse > XHCI Extended Capabilities registers to read number of usb2 ports and > usb3 ports present on multiport controller. Each USB Port is at least HS > capable. > > The port info for usb2 and usb3 phy are identified as num_usb2_ports > and num_usb3_ports. The intention is as follows: > > Wherever we need to perform phy operations like: > > LOOP_OVER_NUMBER_OF_AVAILABLE_PORTS() > { > phy_set_mode(dwc->usb2_generic_phy[i], PHY_MODE_USB_HOST); > phy_set_mode(dwc->usb3_generic_phy[i], PHY_MODE_USB_HOST); > } > > If number of usb2 ports is 3, loop can go from index 0-2 for > usb2_generic_phy. If number of usb3-ports is 2, we don't know for sure, > if the first 2 ports are SS capable or some other ports like (2 and 3) > are SS capable. So instead, num_usb2_ports is used to loop around all > phy's (both hs and ss) for performing phy operations. If any > usb3_generic_phy turns out to be NULL, phy operation just bails out. > > num_usb3_ports is used to modify GUSB3PIPECTL registers while setting up > phy's as we need to know how many SS capable ports are there for this. > > Signed-off-by: Krishna Kurapati <quic_kriskura@quicinc.com> > --- > drivers/usb/dwc3/core.c | 113 ++++++++++++++++++++++++++++++++++++++++ > drivers/usb/dwc3/core.h | 17 +++++- > 2 files changed, 129 insertions(+), 1 deletion(-) > > diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c > index 0beaab932e7d..e983aef1fb93 100644 > --- a/drivers/usb/dwc3/core.c > +++ b/drivers/usb/dwc3/core.c > @@ -1767,6 +1767,104 @@ static int dwc3_get_clocks(struct dwc3 *dwc) > return 0; > } > > +/** > + * dwc3_xhci_find_next_ext_cap - Find the offset of the extended capabilities > + * with capability ID id. > + * > + * @base: PCI MMIO registers base address. > + * @start: address at which to start looking, (0 or HCC_PARAMS to start at > + * beginning of list) > + * @id: Extended capability ID to search for, or 0 for the next > + * capability > + * > + * Returns the offset of the next matching extended capability structure. > + * Some capabilities can occur several times, e.g., the XHCI_EXT_CAPS_PROTOCOL, > + * and this provides a way to find them all. > + */ > +static int dwc3_xhci_find_next_ext_cap(void __iomem *base, u32 start, int id) > +{ > + u32 val; > + u32 next; > + u32 offset; > + > + offset = start; > + if (!start || start == XHCI_HCC_PARAMS_OFFSET) { > + val = readl(base + XHCI_HCC_PARAMS_OFFSET); > + if (val == ~0) > + return 0; > + offset = XHCI_HCC_EXT_CAPS(val) << 2; > + if (!offset) > + return 0; > + } > + do { > + val = readl(base + offset); > + if (val == ~0) > + return 0; > + if (offset != start && (id == 0 || XHCI_EXT_CAPS_ID(val) == id)) > + return offset; > + > + next = XHCI_EXT_CAPS_NEXT(val); > + offset += next << 2; > + } while (next); > + > + return 0; > +} You should not make another copy of xhci_find_next_ext_cap(), but rather use it directly. We already have drivers outside of usb/host using this function so it should be fine to do the same for now: #include "../host/xhci-ext-caps.h" > +static int dwc3_read_port_info(struct dwc3 *dwc) > +{ > + void __iomem *regs; Call this one 'base' instead. > + u32 offset; > + u32 temp; I see that the xhci driver use 'temp' for this, but I'd prefer 'val'. > + u8 major_revision; > + int ret = 0; > + > + /* > + * Remap xHCI address space to access XHCI ext cap regs, > + * since it is needed to get port info. > + */ > + regs = ioremap(dwc->xhci_resources[0].start, > + resource_size(&dwc->xhci_resources[0])); > + if (IS_ERR(regs)) > + return PTR_ERR(regs); > + > + offset = dwc3_xhci_find_next_ext_cap(regs, 0, > + XHCI_EXT_CAPS_PROTOCOL); > + while (offset) { This would be better implemented as a do-while loop (cf. xdbc_reset_debug_port()). > + temp = readl(regs + offset); > + major_revision = XHCI_EXT_PORT_MAJOR(temp); > + > + temp = readl(regs + offset + 0x08); We should try to avoid magic constants, but I see that we already have cases accessing these fields like this. > + if (major_revision == 0x03) { > + dwc->num_usb3_ports += XHCI_EXT_PORT_COUNT(temp); > + } else if (major_revision <= 0x02) { > + dwc->num_usb2_ports += XHCI_EXT_PORT_COUNT(temp); > + } else { > + dev_err(dwc->dev, > + "Unrecognized port major revision %d\n", major_revision); Please add a line break after the string. Perhaps this should be handles as in xhci core by simply warning and continuing instead. > + ret = -EINVAL; > + goto unmap_reg; > + } > + > + offset = dwc3_xhci_find_next_ext_cap(regs, offset, > + XHCI_EXT_CAPS_PROTOCOL); > + } > + > + temp = readl(regs + DWC3_XHCI_HCSPARAMS1); > + if (HCS_MAX_PORTS(temp) != (dwc->num_usb3_ports + dwc->num_usb2_ports)) { > + dev_err(dwc->dev, > + "Mismatched reported MAXPORTS (%d)\n", HCS_MAX_PORTS(temp)); > + ret = -EINVAL; > + goto unmap_reg; > + } Not sure this is needed either. Could this risk regressing platforms which does not have currently have all PHYs described in DT? You do however need to make sure that both num_usb<n>_ports is no larger than MAX_PORTS_SUPPORTED to avoid memory corruption when you're adding fixed sized arrays for the PHYs later in the series. > + > + dev_dbg(dwc->dev, > + "hs-ports: %d ss-ports: %d\n", dwc->num_usb2_ports, dwc->num_usb3_ports); Use %u for unsigned values. And please try to stay within 80 columns. > + > +unmap_reg: > + iounmap(regs); > + return ret; > +} > + > static int dwc3_probe(struct platform_device *pdev) > { > struct device *dev = &pdev->dev; > @@ -1774,6 +1872,7 @@ static int dwc3_probe(struct platform_device *pdev) > void __iomem *regs; > struct dwc3 *dwc; > int ret; > + unsigned int hw_mode; > > dwc = devm_kzalloc(dev, sizeof(*dwc), GFP_KERNEL); > if (!dwc) > @@ -1843,6 +1942,20 @@ static int dwc3_probe(struct platform_device *pdev) > goto err_disable_clks; > } > > + /* > + * Currently DWC3 controllers that are host-only capable > + * support Multiport Are you missing an "only" after "Currently" above? Please add a full stop. > + */ > + hw_mode = DWC3_GHWPARAMS0_MODE(dwc->hwparams.hwparams0); > + if (hw_mode == DWC3_GHWPARAMS0_MODE_HOST) { > + ret = dwc3_read_port_info(dwc); > + if (ret) > + goto err_disable_clks; > + } else { > + dwc->num_usb2_ports = 1; > + dwc->num_usb3_ports = 1; > + } > + > spin_lock_init(&dwc->lock); > mutex_init(&dwc->mutex); > > diff --git a/drivers/usb/dwc3/core.h b/drivers/usb/dwc3/core.h > index d56457c02996..d3401963bc27 100644 > --- a/drivers/usb/dwc3/core.h > +++ b/drivers/usb/dwc3/core.h > @@ -35,6 +35,17 @@ > > #define DWC3_MSG_MAX 500 > > +/* Define XHCI Extcap register offsets for getting multiport info */ > +#define XHCI_HCC_PARAMS_OFFSET 0x10 > +#define DWC3_XHCI_HCSPARAMS1 0x04 > +#define XHCI_EXT_CAPS_PROTOCOL 2 > +#define XHCI_HCC_EXT_CAPS(x) (((x) >> 16) & 0xffff) > +#define XHCI_EXT_CAPS_ID(x) (((x) >> 0) & 0xff) > +#define XHCI_EXT_CAPS_NEXT(x) (((x) >> 8) & 0xff) > +#define XHCI_EXT_PORT_MAJOR(x) (((x) >> 24) & 0xff) > +#define XHCI_EXT_PORT_COUNT(x) (((x) >> 8) & 0xff) > +#define HCS_MAX_PORTS(x) (((x) >> 24) & 0x7f) > + You should use the xhci defines instead of these copies too. > /* Global constants */ > #define DWC3_PULL_UP_TIMEOUT 500 /* ms */ > #define DWC3_BOUNCE_SIZE 1024 /* size of a superspeed bulk */ > @@ -1025,6 +1036,8 @@ struct dwc3_scratchpad_array { > * @usb_psy: pointer to power supply interface. > * @usb2_phy: pointer to USB2 PHY > * @usb3_phy: pointer to USB3 PHY > + * @num_usb2_ports: number of usb2 ports. > + * @num_usb3_ports: number of usb3 ports. Use upper case "USBn" and drop the full stops for consistency. Please move these after the PHY structures. > * @usb2_generic_phy: pointer to USB2 PHY > * @usb3_generic_phy: pointer to USB3 PHY > * @phys_ready: flag to indicate that PHYs are ready > @@ -1162,6 +1175,9 @@ struct dwc3 { > struct usb_phy *usb2_phy; > struct usb_phy *usb3_phy; > > + u8 num_usb2_ports; > + u8 num_usb3_ports; > + > struct phy *usb2_generic_phy; > struct phy *usb3_generic_phy; > > @@ -1649,5 +1665,4 @@ static inline int dwc3_ulpi_init(struct dwc3 *dwc) > static inline void dwc3_ulpi_exit(struct dwc3 *dwc) > { } > #endif > - This is an unrelated change that should be dropped. > #endif /* __DRIVERS_USB_DWC3_CORE_H */ 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 4:24 PM, Johan Hovold wrote: > On Mon, May 15, 2023 at 09:02:13PM +0530, Krishna Kurapati PSSNV wrote: >> On 5/15/2023 7:56 PM, Johan Hovold wrote: >>> On Sun, May 14, 2023 at 11:19:15AM +0530, Krishna Kurapati wrote: > >>>> @@ -3133,6 +3133,72 @@ usb_1_role_switch: endpoint { >>>> }; >>>> }; >>>> >>>> + usb_2: usb@a4f8800 { >>> >>> As I believe someone already pointed out, this node is not in sort order >>> (i.e. it should go before usb@a6f8800). > >> I missed that message, but since I named it usb_2, so I placed it in >> order after usb_1. Hope that is fine !! > > No, the nodes should be sorted by unit address so you need to move it. > Sure, in that case will put it in between usb_0 and usb_1 nodes. >>>> + interrupts-extended = <&pdc 127 IRQ_TYPE_EDGE_RISING>, >>>> + <&pdc 126 IRQ_TYPE_EDGE_RISING>, >>>> + <&pdc 16 IRQ_TYPE_LEVEL_HIGH>, >>>> + <&intc GIC_SPI 130 IRQ_TYPE_LEVEL_HIGH>, >>>> + <&intc GIC_SPI 135 IRQ_TYPE_LEVEL_HIGH>, >>>> + <&intc GIC_SPI 857 IRQ_TYPE_LEVEL_HIGH>, >>>> + <&intc GIC_SPI 856 IRQ_TYPE_LEVEL_HIGH>; >>>> + >>>> + interrupt-names = "dp_hs_phy_irq", >>>> + "dm_hs_phy_irq", >>>> + "ss_phy_irq", >>>> + "pwr_event_1", >>>> + "pwr_event_2", >>>> + "pwr_event_3", >>>> + "pwr_event_4"; > >>>> + interconnect-names = "usb-ddr", "apps-usb"; >>> >>> Looks like 'wakeup-source' is missing here too. >>> >> >> I believe this property was added to enable wakeup from system suspend >> in host mode. I didn't add this property as currently I don't need to >> support wakeup. If any requirement comes in future, then I might need to >> add dp/dm interrupts (if any) for other ports as well and then need to >> change driver code to enable/disable them on suspend/resume. > > If there are dp/dm/ss interrupts per ports then those need to be defined > in the binding and devicetree from the start. > > Similar for 'wakeup-source' which indicates that the controller *can* be > used to wakeup the system from suspend (which those pdc interrupts > indicates). > > Remember that the devicetree is supposed to describe the hardware, and > which features are currently supported in some version of software is > mostly irrelevant. > > Johan Can I take this up as a separate series (Wakeup support for multiport) once this series is merged. If I am adding interrupts for other ports, I can add driver code to handle those interrupts as well. Regards, Krishna,
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 Tue, May 16, 2023 at 07:54:00PM +0530, Krishna Kurapati PSSNV wrote: > > > On 5/16/2023 4:24 PM, Johan Hovold wrote: > > On Mon, May 15, 2023 at 09:02:13PM +0530, Krishna Kurapati PSSNV wrote: > >> On 5/15/2023 7:56 PM, Johan Hovold wrote: > >>> On Sun, May 14, 2023 at 11:19:15AM +0530, Krishna Kurapati wrote: > > > >>>> @@ -3133,6 +3133,72 @@ usb_1_role_switch: endpoint { > >>>> }; > >>>> }; > >>>> > >>>> + usb_2: usb@a4f8800 { > >>> > >>> As I believe someone already pointed out, this node is not in sort order > >>> (i.e. it should go before usb@a6f8800). > > > >> I missed that message, but since I named it usb_2, so I placed it in > >> order after usb_1. Hope that is fine !! > > > > No, the nodes should be sorted by unit address so you need to move it. > > > > Sure, in that case will put it in between usb_0 and usb_1 nodes. No, it goes before usb_0 on sc8280xp. usb_2: usb@a4f8800 { usb_0: usb@a6f8800 { usb_1: usb@a8f8800 { > >>>> + interrupts-extended = <&pdc 127 IRQ_TYPE_EDGE_RISING>, > >>>> + <&pdc 126 IRQ_TYPE_EDGE_RISING>, > >>>> + <&pdc 16 IRQ_TYPE_LEVEL_HIGH>, > >>>> + <&intc GIC_SPI 130 IRQ_TYPE_LEVEL_HIGH>, > >>>> + <&intc GIC_SPI 135 IRQ_TYPE_LEVEL_HIGH>, > >>>> + <&intc GIC_SPI 857 IRQ_TYPE_LEVEL_HIGH>, > >>>> + <&intc GIC_SPI 856 IRQ_TYPE_LEVEL_HIGH>; > >>>> + > >>>> + interrupt-names = "dp_hs_phy_irq", > >>>> + "dm_hs_phy_irq", > >>>> + "ss_phy_irq", > >>>> + "pwr_event_1", > >>>> + "pwr_event_2", > >>>> + "pwr_event_3", > >>>> + "pwr_event_4"; > > > >>>> + interconnect-names = "usb-ddr", "apps-usb"; > >>> > >>> Looks like 'wakeup-source' is missing here too. > >>> > >> > >> I believe this property was added to enable wakeup from system suspend > >> in host mode. I didn't add this property as currently I don't need to > >> support wakeup. If any requirement comes in future, then I might need to > >> add dp/dm interrupts (if any) for other ports as well and then need to > >> change driver code to enable/disable them on suspend/resume. > > > > If there are dp/dm/ss interrupts per ports then those need to be defined > > in the binding and devicetree from the start. > > > > Similar for 'wakeup-source' which indicates that the controller *can* be > > used to wakeup the system from suspend (which those pdc interrupts > > indicates). > > > > Remember that the devicetree is supposed to describe the hardware, and > > which features are currently supported in some version of software is > > mostly irrelevant. > Can I take this up as a separate series (Wakeup support for multiport) > once this series is merged. If I am adding interrupts for other ports, I > can add driver code to handle those interrupts as well. Nope. You can possibly add driver support later, but the binding and dtsi need to be correct from the start (and it may be easier to do it all at once). Johan
On 5/16/2023 8:12 PM, Johan Hovold wrote: > On Tue, May 16, 2023 at 07:54:00PM +0530, Krishna Kurapati PSSNV wrote: >> >> >> On 5/16/2023 4:24 PM, Johan Hovold wrote: >>> On Mon, May 15, 2023 at 09:02:13PM +0530, Krishna Kurapati PSSNV wrote: >>>> On 5/15/2023 7:56 PM, Johan Hovold wrote: >>>>> On Sun, May 14, 2023 at 11:19:15AM +0530, Krishna Kurapati wrote: >>> >>>>>> @@ -3133,6 +3133,72 @@ usb_1_role_switch: endpoint { >>>>>> }; >>>>>> }; >>>>>> >>>>>> + usb_2: usb@a4f8800 { >>>>> >>>>> As I believe someone already pointed out, this node is not in sort order >>>>> (i.e. it should go before usb@a6f8800). >>> >>>> I missed that message, but since I named it usb_2, so I placed it in >>>> order after usb_1. Hope that is fine !! >>> >>> No, the nodes should be sorted by unit address so you need to move it. >>> >> >> Sure, in that case will put it in between usb_0 and usb_1 nodes. > > No, it goes before usb_0 on sc8280xp. > > usb_2: usb@a4f8800 { > usb_0: usb@a6f8800 { > usb_1: usb@a8f8800 { > >>>>>> + interrupts-extended = <&pdc 127 IRQ_TYPE_EDGE_RISING>, >>>>>> + <&pdc 126 IRQ_TYPE_EDGE_RISING>, >>>>>> + <&pdc 16 IRQ_TYPE_LEVEL_HIGH>, >>>>>> + <&intc GIC_SPI 130 IRQ_TYPE_LEVEL_HIGH>, >>>>>> + <&intc GIC_SPI 135 IRQ_TYPE_LEVEL_HIGH>, >>>>>> + <&intc GIC_SPI 857 IRQ_TYPE_LEVEL_HIGH>, >>>>>> + <&intc GIC_SPI 856 IRQ_TYPE_LEVEL_HIGH>; >>>>>> + >>>>>> + interrupt-names = "dp_hs_phy_irq", >>>>>> + "dm_hs_phy_irq", >>>>>> + "ss_phy_irq", >>>>>> + "pwr_event_1", >>>>>> + "pwr_event_2", >>>>>> + "pwr_event_3", >>>>>> + "pwr_event_4"; >>> >>>>>> + interconnect-names = "usb-ddr", "apps-usb"; >>>>> >>>>> Looks like 'wakeup-source' is missing here too. >>>>> >>>> >>>> I believe this property was added to enable wakeup from system suspend >>>> in host mode. I didn't add this property as currently I don't need to >>>> support wakeup. If any requirement comes in future, then I might need to >>>> add dp/dm interrupts (if any) for other ports as well and then need to >>>> change driver code to enable/disable them on suspend/resume. >>> >>> If there are dp/dm/ss interrupts per ports then those need to be defined >>> in the binding and devicetree from the start. >>> >>> Similar for 'wakeup-source' which indicates that the controller *can* be >>> used to wakeup the system from suspend (which those pdc interrupts >>> indicates). >>> >>> Remember that the devicetree is supposed to describe the hardware, and >>> which features are currently supported in some version of software is >>> mostly irrelevant. > >> Can I take this up as a separate series (Wakeup support for multiport) >> once this series is merged. If I am adding interrupts for other ports, I >> can add driver code to handle those interrupts as well. > > Nope. You can possibly add driver support later, but the binding and > dtsi need to be correct from the start (and it may be easier to do it > all at once). > Ok, will add this in v9. Regards, Krishna,
On 5/16/2023 5:41 PM, Johan Hovold wrote: > On Sun, May 14, 2023 at 11:19:11AM +0530, Krishna Kurapati wrote: >> Currently host-only capable DWC3 controllers support Multiport. >> Temporarily map XHCI address space for host-only controllers and parse >> XHCI Extended Capabilities registers to read number of usb2 ports and >> usb3 ports present on multiport controller. Each USB Port is at least HS >> capable. >> >> The port info for usb2 and usb3 phy are identified as num_usb2_ports >> and num_usb3_ports. The intention is as follows: >> >> Wherever we need to perform phy operations like: >> >> LOOP_OVER_NUMBER_OF_AVAILABLE_PORTS() >> { >> phy_set_mode(dwc->usb2_generic_phy[i], PHY_MODE_USB_HOST); >> phy_set_mode(dwc->usb3_generic_phy[i], PHY_MODE_USB_HOST); >> } >> >> If number of usb2 ports is 3, loop can go from index 0-2 for >> usb2_generic_phy. If number of usb3-ports is 2, we don't know for sure, >> if the first 2 ports are SS capable or some other ports like (2 and 3) >> are SS capable. So instead, num_usb2_ports is used to loop around all >> phy's (both hs and ss) for performing phy operations. If any >> usb3_generic_phy turns out to be NULL, phy operation just bails out. >> >> num_usb3_ports is used to modify GUSB3PIPECTL registers while setting up >> phy's as we need to know how many SS capable ports are there for this. >> >> Signed-off-by: Krishna Kurapati <quic_kriskura@quicinc.com> >> --- >> drivers/usb/dwc3/core.c | 113 ++++++++++++++++++++++++++++++++++++++++ >> drivers/usb/dwc3/core.h | 17 +++++- >> 2 files changed, 129 insertions(+), 1 deletion(-) >> >> diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c >> index 0beaab932e7d..e983aef1fb93 100644 >> --- a/drivers/usb/dwc3/core.c >> +++ b/drivers/usb/dwc3/core.c >> @@ -1767,6 +1767,104 @@ static int dwc3_get_clocks(struct dwc3 *dwc) >> return 0; >> } >> >> +/** >> + * dwc3_xhci_find_next_ext_cap - Find the offset of the extended capabilities >> + * with capability ID id. >> + * >> + * @base: PCI MMIO registers base address. >> + * @start: address at which to start looking, (0 or HCC_PARAMS to start at >> + * beginning of list) >> + * @id: Extended capability ID to search for, or 0 for the next >> + * capability >> + * >> + * Returns the offset of the next matching extended capability structure. >> + * Some capabilities can occur several times, e.g., the XHCI_EXT_CAPS_PROTOCOL, >> + * and this provides a way to find them all. >> + */ >> +static int dwc3_xhci_find_next_ext_cap(void __iomem *base, u32 start, int id) >> +{ >> + u32 val; >> + u32 next; >> + u32 offset; >> + >> + offset = start; >> + if (!start || start == XHCI_HCC_PARAMS_OFFSET) { >> + val = readl(base + XHCI_HCC_PARAMS_OFFSET); >> + if (val == ~0) >> + return 0; >> + offset = XHCI_HCC_EXT_CAPS(val) << 2; >> + if (!offset) >> + return 0; >> + } >> + do { >> + val = readl(base + offset); >> + if (val == ~0) >> + return 0; >> + if (offset != start && (id == 0 || XHCI_EXT_CAPS_ID(val) == id)) >> + return offset; >> + >> + next = XHCI_EXT_CAPS_NEXT(val); >> + offset += next << 2; >> + } while (next); >> + >> + return 0; >> +} > > You should not make another copy of xhci_find_next_ext_cap(), but rather > use it directly. > > We already have drivers outside of usb/host using this function so it > should be fine to do the same for now: > > #include "../host/xhci-ext-caps.h" > Hi Johan, This was the approach which we followed when we first introduced the patch [1]. But Thinh suggested to duplicate code so that we can avoid any dependency on xhci (which seems to be right). So since its just one function, I duplicated it here. >> +static int dwc3_read_port_info(struct dwc3 *dwc) >> +{ >> + void __iomem *regs; > > Call this one 'base' instead. > >> + u32 offset; >> + u32 temp; > > I see that the xhci driver use 'temp' for this, but I'd prefer 'val'. > >> + u8 major_revision; >> + int ret = 0; >> + >> + /* >> + * Remap xHCI address space to access XHCI ext cap regs, >> + * since it is needed to get port info. >> + */ >> + regs = ioremap(dwc->xhci_resources[0].start, >> + resource_size(&dwc->xhci_resources[0])); >> + if (IS_ERR(regs)) >> + return PTR_ERR(regs); >> + >> + offset = dwc3_xhci_find_next_ext_cap(regs, 0, >> + XHCI_EXT_CAPS_PROTOCOL); >> + while (offset) { > > This would be better implemented as a do-while loop (cf. > xdbc_reset_debug_port()). > >> + temp = readl(regs + offset); >> + major_revision = XHCI_EXT_PORT_MAJOR(temp); >> + >> + temp = readl(regs + offset + 0x08); > > We should try to avoid magic constants, but I see that we already have > cases accessing these fields like this. > >> + if (major_revision == 0x03) { >> + dwc->num_usb3_ports += XHCI_EXT_PORT_COUNT(temp); >> + } else if (major_revision <= 0x02) { >> + dwc->num_usb2_ports += XHCI_EXT_PORT_COUNT(temp); >> + } else { >> + dev_err(dwc->dev, >> + "Unrecognized port major revision %d\n", major_revision); > > Please add a line break after the string. > > Perhaps this should be handles as in xhci core by simply warning and > continuing instead. > I broke the loop and went to unmap as we are not sure what values would be read. Any use of continuing ? >> + ret = -EINVAL; >> + goto unmap_reg; >> + } >> + >> + offset = dwc3_xhci_find_next_ext_cap(regs, offset, >> + XHCI_EXT_CAPS_PROTOCOL); >> + } >> + >> + temp = readl(regs + DWC3_XHCI_HCSPARAMS1); >> + if (HCS_MAX_PORTS(temp) != (dwc->num_usb3_ports + dwc->num_usb2_ports)) { >> + dev_err(dwc->dev, >> + "Mismatched reported MAXPORTS (%d)\n", HCS_MAX_PORTS(temp)); >> + ret = -EINVAL; >> + goto unmap_reg; >> + } > > Not sure this is needed either. > > Could this risk regressing platforms which does not have currently have > all PHYs described in DT? > No, it doesn't. AFAIK, this only tells how many ports are present as per the core consultant configuration of the device. I tried to explain what would happen incase phy's are not present in DT in [2] & [3]. > You do however need to make sure that both num_usb<n>_ports is no larger > than MAX_PORTS_SUPPORTED to avoid memory corruption when you're adding > fixed sized arrays for the PHYs later in the series. > >> + >> + dev_dbg(dwc->dev, >> + "hs-ports: %d ss-ports: %d\n", dwc->num_usb2_ports, dwc->num_usb3_ports); > > Use %u for unsigned values. > > And please try to stay within 80 columns. > Thanks for catching this potential bug. Will add that if check as well in v9. >> + >> +unmap_reg: >> + iounmap(regs); >> + return ret; >> +} >> + >> static int dwc3_probe(struct platform_device *pdev) >> { >> struct device *dev = &pdev->dev; >> @@ -1774,6 +1872,7 @@ static int dwc3_probe(struct platform_device *pdev) >> void __iomem *regs; >> struct dwc3 *dwc; >> int ret; >> + unsigned int hw_mode; >> >> dwc = devm_kzalloc(dev, sizeof(*dwc), GFP_KERNEL); >> if (!dwc) >> @@ -1843,6 +1942,20 @@ static int dwc3_probe(struct platform_device *pdev) >> goto err_disable_clks; >> } >> >> + /* >> + * Currently DWC3 controllers that are host-only capable >> + * support Multiport > > Are you missing an "only" after "Currently" above? > > Please add a full stop. > >> + */ >> + hw_mode = DWC3_GHWPARAMS0_MODE(dwc->hwparams.hwparams0); >> + if (hw_mode == DWC3_GHWPARAMS0_MODE_HOST) { >> + ret = dwc3_read_port_info(dwc); >> + if (ret) >> + goto err_disable_clks; >> + } else { >> + dwc->num_usb2_ports = 1; >> + dwc->num_usb3_ports = 1; >> + } >> + >> spin_lock_init(&dwc->lock); >> mutex_init(&dwc->mutex); >> >> diff --git a/drivers/usb/dwc3/core.h b/drivers/usb/dwc3/core.h >> index d56457c02996..d3401963bc27 100644 >> --- a/drivers/usb/dwc3/core.h >> +++ b/drivers/usb/dwc3/core.h >> @@ -35,6 +35,17 @@ >> >> #define DWC3_MSG_MAX 500 >> >> +/* Define XHCI Extcap register offsets for getting multiport info */ >> +#define XHCI_HCC_PARAMS_OFFSET 0x10 >> +#define DWC3_XHCI_HCSPARAMS1 0x04 >> +#define XHCI_EXT_CAPS_PROTOCOL 2 >> +#define XHCI_HCC_EXT_CAPS(x) (((x) >> 16) & 0xffff) >> +#define XHCI_EXT_CAPS_ID(x) (((x) >> 0) & 0xff) >> +#define XHCI_EXT_CAPS_NEXT(x) (((x) >> 8) & 0xff) >> +#define XHCI_EXT_PORT_MAJOR(x) (((x) >> 24) & 0xff) >> +#define XHCI_EXT_PORT_COUNT(x) (((x) >> 8) & 0xff) >> +#define HCS_MAX_PORTS(x) (((x) >> 24) & 0x7f) >> + > > You should use the xhci defines instead of these copies too. > >> /* Global constants */ >> #define DWC3_PULL_UP_TIMEOUT 500 /* ms */ >> #define DWC3_BOUNCE_SIZE 1024 /* size of a superspeed bulk */ >> @@ -1025,6 +1036,8 @@ struct dwc3_scratchpad_array { >> * @usb_psy: pointer to power supply interface. >> * @usb2_phy: pointer to USB2 PHY >> * @usb3_phy: pointer to USB3 PHY >> + * @num_usb2_ports: number of usb2 ports. >> + * @num_usb3_ports: number of usb3 ports. > > Use upper case "USBn" and drop the full stops for consistency. > > Please move these after the PHY structures. > >> * @usb2_generic_phy: pointer to USB2 PHY >> * @usb3_generic_phy: pointer to USB3 PHY >> * @phys_ready: flag to indicate that PHYs are ready >> @@ -1162,6 +1175,9 @@ struct dwc3 { >> struct usb_phy *usb2_phy; >> struct usb_phy *usb3_phy; >> >> + u8 num_usb2_ports; >> + u8 num_usb3_ports; >> + >> struct phy *usb2_generic_phy; >> struct phy *usb3_generic_phy; >> >> @@ -1649,5 +1665,4 @@ static inline int dwc3_ulpi_init(struct dwc3 *dwc) >> static inline void dwc3_ulpi_exit(struct dwc3 *dwc) >> { } >> #endif >> - > > This is an unrelated change that should be dropped. > >> #endif /* __DRIVERS_USB_DWC3_CORE_H */ > > Johan [1]: https://lore.kernel.org/all/20230310163420.7582-3-quic_kriskura@quicinc.com/ [2]: https://lore.kernel.org/all/4eb26a54-148b-942f-01c6-64e66541de8b@quicinc.com/ [3]: https://lore.kernel.org/all/966c1001-6d64-9163-0c07-96595156fc8c@quicinc.com/ Thanks for the review and comments 🙂. Will make sure to fix them in v9. Regards, Krishna,
On Tue, May 16, 2023, Krishna Kurapati PSSNV wrote: > > > On 5/16/2023 2:38 AM, Bjorn Andersson wrote: > > On Sun, May 14, 2023 at 11:19:11AM +0530, Krishna Kurapati wrote: > > > Currently host-only capable DWC3 controllers support Multiport. > > > Temporarily map XHCI address space for host-only controllers and parse > > > XHCI Extended Capabilities registers to read number of usb2 ports and > > > usb3 ports present on multiport controller. Each USB Port is at least HS > > > capable. > > > > > > The port info for usb2 and usb3 phy are identified as num_usb2_ports > > > and num_usb3_ports. The intention is as follows: > > > > > > Wherever we need to perform phy operations like: > > > > > > LOOP_OVER_NUMBER_OF_AVAILABLE_PORTS() > > > { > > > phy_set_mode(dwc->usb2_generic_phy[i], PHY_MODE_USB_HOST); > > > phy_set_mode(dwc->usb3_generic_phy[i], PHY_MODE_USB_HOST); > > > } > > > > > > If number of usb2 ports is 3, loop can go from index 0-2 for > > > usb2_generic_phy. If number of usb3-ports is 2, we don't know for sure, > > > if the first 2 ports are SS capable or some other ports like (2 and 3) > > > are SS capable. So instead, num_usb2_ports is used to loop around all > > > phy's (both hs and ss) for performing phy operations. If any > > > usb3_generic_phy turns out to be NULL, phy operation just bails out. > > > > > > num_usb3_ports is used to modify GUSB3PIPECTL registers while setting up > > > phy's as we need to know how many SS capable ports are there for this. > > > > > > Signed-off-by: Krishna Kurapati <quic_kriskura@quicinc.com> > > > --- > > > drivers/usb/dwc3/core.c | 113 ++++++++++++++++++++++++++++++++++++++++ > > > drivers/usb/dwc3/core.h | 17 +++++- > > > 2 files changed, 129 insertions(+), 1 deletion(-) > > > > > > diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c > > > index 0beaab932e7d..e983aef1fb93 100644 > > > --- a/drivers/usb/dwc3/core.c > > > +++ b/drivers/usb/dwc3/core.c > > > @@ -1767,6 +1767,104 @@ static int dwc3_get_clocks(struct dwc3 *dwc) > > > return 0; > > > } > > > +/** > > > + * dwc3_xhci_find_next_ext_cap - Find the offset of the extended capabilities > > > + * with capability ID id. > > > > () after function name in kernel-doc > > > > > + * > > > + * @base: PCI MMIO registers base address. > > > > Should this be "XHCI MMIO..."? > > Hi Bjorn, > > I copied this code from xhci-ext-caps.h. The documentation of this > function mentioned PCI in that file. May be Thinh/Mathias can correct us if > this is wrong. > It's the beginning of the xhci MMIO address space. You can refer to it as "xHCI MMIO base address". It's not specific to PCI xHCI. > > > > > + * @start: address at which to start looking, (0 or HCC_PARAMS to start at > > > + * beginning of list) > > > + * @id: Extended capability ID to search for, or 0 for the next > > > + * capability > > > + * > > > + * Returns the offset of the next matching extended capability structure. > > > > Return: The offset... > > > > Per https://urldefense.com/v3/__https://www.kernel.org/doc/html/next/doc-guide/kernel-doc.html__;!!A4F2R9G_pg!bEwblSKMcLvR5FA5HEYgV98KR4Vwjj9WnIKHsUa5udbp7YOBzLR77YzL5Ijqx41kce4DDcgUtSsFoS1Tn7inIPAQZFdVuw$ . > > > > I executed the following command and it didn't give any errors: > > ./scripts/kernel-doc -none -Werror -function dwc3_xhci_find_next_ext_cap > drivers/usb/dwc3/core.c > > I see that even for dwc3_core_init the comments are the same: > > /** > * dwc3_core_init - Low-level initialization of DWC3 Core > * @dwc: Pointer to our controller context structure > * > * Returns 0 on success otherwise negative errno. > */ The documentation Bjorn sent is correct. The script isn't smart enough to catch everything. Looks like we have a lot of kernel-doc mistakes in dwc3. > > > > + * Some capabilities can occur several times, e.g., the XHCI_EXT_CAPS_PROTOCOL, > > > + * and this provides a way to find them all. > > > + */ > > > +static int dwc3_xhci_find_next_ext_cap(void __iomem *base, u32 start, int id) > > > +{ > > > + u32 val; > > > + u32 next; > > > + u32 offset; > > > + > > > + offset = start; > > > + if (!start || start == XHCI_HCC_PARAMS_OFFSET) { > > > + val = readl(base + XHCI_HCC_PARAMS_OFFSET); > > > + if (val == ~0) > > > + return 0; > > > + offset = XHCI_HCC_EXT_CAPS(val) << 2; > > > + if (!offset) > > > + return 0; > > > + } > > > + do { > > > + val = readl(base + offset); > > > + if (val == ~0) > > > + return 0; > > > + if (offset != start && (id == 0 || XHCI_EXT_CAPS_ID(val) == id)) > > > + return offset; > > > + > > > + next = XHCI_EXT_CAPS_NEXT(val); > > > + offset += next << 2; > > > + } while (next); > > > + > > > + return 0; > > > +} > > > + > > > +static int dwc3_read_port_info(struct dwc3 *dwc) > > > +{ > > > + void __iomem *regs; > > > + u32 offset; > > > + u32 temp; > > > + u8 major_revision; > > > + int ret = 0; > > > > Please drop the spacing between type and variable name here, if nothing > > else it's inconsistent with the previous function. > > > > Sure, will fix this nit. > It's understandable why you had this in the beginning since it's common in different places within dwc3 driver. It's a bit difficult to enforce this, and it's just minor style issue. My only request is to keep it consistent throughout your changes. Thanks, Thinh > > > + > > > + /* > > > + * Remap xHCI address space to access XHCI ext cap regs, > > > + * since it is needed to get port info. > > > + */ > > > + regs = ioremap(dwc->xhci_resources[0].start, > > > + resource_size(&dwc->xhci_resources[0])); > > > + if (IS_ERR(regs)) > > > + return PTR_ERR(regs); > > > + > > > + offset = dwc3_xhci_find_next_ext_cap(regs, 0, > > > + XHCI_EXT_CAPS_PROTOCOL); > > > + while (offset) { > > > + temp = readl(regs + offset); > > > + major_revision = XHCI_EXT_PORT_MAJOR(temp); > > > + > > > + temp = readl(regs + offset + 0x08); > > > + if (major_revision == 0x03) { > > > + dwc->num_usb3_ports += XHCI_EXT_PORT_COUNT(temp); > > > + } else if (major_revision <= 0x02) { > > > + dwc->num_usb2_ports += XHCI_EXT_PORT_COUNT(temp); > > > + } else { > > > + dev_err(dwc->dev, > > > + "Unrecognized port major revision %d\n", major_revision); > > > + ret = -EINVAL; > > > + goto unmap_reg; > > > + } > > > + > > > + offset = dwc3_xhci_find_next_ext_cap(regs, offset, > > > + XHCI_EXT_CAPS_PROTOCOL); > > > + } > > > + > > > + temp = readl(regs + DWC3_XHCI_HCSPARAMS1); > > > + if (HCS_MAX_PORTS(temp) != (dwc->num_usb3_ports + dwc->num_usb2_ports)) { > > > + dev_err(dwc->dev, > > > + "Mismatched reported MAXPORTS (%d)\n", HCS_MAX_PORTS(temp)); > > > + ret = -EINVAL; > > > + goto unmap_reg; > > > + } > > > + > > > + dev_dbg(dwc->dev, > > > + "hs-ports: %d ss-ports: %d\n", dwc->num_usb2_ports, dwc->num_usb3_ports); > > > + > > > +unmap_reg: > > > + iounmap(regs); > > > + return ret; > > > +} > > > + > > > static int dwc3_probe(struct platform_device *pdev) > > > { > > > struct device *dev = &pdev->dev; > > > @@ -1774,6 +1872,7 @@ static int dwc3_probe(struct platform_device *pdev) > > > void __iomem *regs; > > > struct dwc3 *dwc; > > > int ret; > > > + unsigned int hw_mode; > > > dwc = devm_kzalloc(dev, sizeof(*dwc), GFP_KERNEL); > > > if (!dwc) > > > @@ -1843,6 +1942,20 @@ static int dwc3_probe(struct platform_device *pdev) > > > goto err_disable_clks; > > > } > > > + /* > > > + * Currently DWC3 controllers that are host-only capable > > > + * support Multiport > > > + */ > > > + hw_mode = DWC3_GHWPARAMS0_MODE(dwc->hwparams.hwparams0); > > > + if (hw_mode == DWC3_GHWPARAMS0_MODE_HOST) { > > > + ret = dwc3_read_port_info(dwc); > > > + if (ret) > > > + goto err_disable_clks; > > > + } else { > > > + dwc->num_usb2_ports = 1; > > > + dwc->num_usb3_ports = 1; > > > + } > > > + > > > spin_lock_init(&dwc->lock); > > > mutex_init(&dwc->mutex); > > > diff --git a/drivers/usb/dwc3/core.h b/drivers/usb/dwc3/core.h > > > index d56457c02996..d3401963bc27 100644 > > > --- a/drivers/usb/dwc3/core.h > > > +++ b/drivers/usb/dwc3/core.h > > > @@ -35,6 +35,17 @@ > > > #define DWC3_MSG_MAX 500 > > > +/* Define XHCI Extcap register offsets for getting multiport info */ > > > +#define XHCI_HCC_PARAMS_OFFSET 0x10 > > > +#define DWC3_XHCI_HCSPARAMS1 0x04 > > > +#define XHCI_EXT_CAPS_PROTOCOL 2 > > > +#define XHCI_HCC_EXT_CAPS(x) (((x) >> 16) & 0xffff) > > > +#define XHCI_EXT_CAPS_ID(x) (((x) >> 0) & 0xff) > > > +#define XHCI_EXT_CAPS_NEXT(x) (((x) >> 8) & 0xff) > > > +#define XHCI_EXT_PORT_MAJOR(x) (((x) >> 24) & 0xff) > > > +#define XHCI_EXT_PORT_COUNT(x) (((x) >> 8) & 0xff) > > > +#define HCS_MAX_PORTS(x) (((x) >> 24) & 0x7f) > > > + > > > /* Global constants */ > > > #define DWC3_PULL_UP_TIMEOUT 500 /* ms */ > > > #define DWC3_BOUNCE_SIZE 1024 /* size of a superspeed bulk */ > > > @@ -1025,6 +1036,8 @@ struct dwc3_scratchpad_array { > > > * @usb_psy: pointer to power supply interface. > > > * @usb2_phy: pointer to USB2 PHY > > > * @usb3_phy: pointer to USB3 PHY > > > + * @num_usb2_ports: number of usb2 ports. > > > + * @num_usb3_ports: number of usb3 ports. > > > * @usb2_generic_phy: pointer to USB2 PHY > > > * @usb3_generic_phy: pointer to USB3 PHY > > > * @phys_ready: flag to indicate that PHYs are ready > > > @@ -1162,6 +1175,9 @@ struct dwc3 { > > > struct usb_phy *usb2_phy; > > > struct usb_phy *usb3_phy; > > > + u8 num_usb2_ports; > > > + u8 num_usb3_ports; > > > + > > > struct phy *usb2_generic_phy; > > > struct phy *usb3_generic_phy; > > > @@ -1649,5 +1665,4 @@ static inline int dwc3_ulpi_init(struct dwc3 *dwc) > > > static inline void dwc3_ulpi_exit(struct dwc3 *dwc) > > > { } > > > #endif > > > - > > > #endif /* __DRIVERS_USB_DWC3_CORE_H */ > > > -- > > > 2.40.0 > > >
On 5/16/2023 8:32 PM, Krishna Kurapati PSSNV wrote: > > > On 5/16/2023 5:41 PM, Johan Hovold wrote: >> On Sun, May 14, 2023 at 11:19:11AM +0530, Krishna Kurapati wrote: >>> Currently host-only capable DWC3 controllers support Multiport. >>> Temporarily map XHCI address space for host-only controllers and parse >>> XHCI Extended Capabilities registers to read number of usb2 ports and >>> usb3 ports present on multiport controller. Each USB Port is at least HS >>> capable. >>> >>> The port info for usb2 and usb3 phy are identified as num_usb2_ports >>> and num_usb3_ports. The intention is as follows: >>> >>> Wherever we need to perform phy operations like: >>> >>> LOOP_OVER_NUMBER_OF_AVAILABLE_PORTS() >>> { >>> phy_set_mode(dwc->usb2_generic_phy[i], PHY_MODE_USB_HOST); >>> phy_set_mode(dwc->usb3_generic_phy[i], PHY_MODE_USB_HOST); >>> } >>> >>> If number of usb2 ports is 3, loop can go from index 0-2 for >>> usb2_generic_phy. If number of usb3-ports is 2, we don't know for sure, >>> if the first 2 ports are SS capable or some other ports like (2 and 3) >>> are SS capable. So instead, num_usb2_ports is used to loop around all >>> phy's (both hs and ss) for performing phy operations. If any >>> usb3_generic_phy turns out to be NULL, phy operation just bails out. >>> >>> num_usb3_ports is used to modify GUSB3PIPECTL registers while setting up >>> phy's as we need to know how many SS capable ports are there for this. >>> >>> Signed-off-by: Krishna Kurapati <quic_kriskura@quicinc.com> >>> --- >>> drivers/usb/dwc3/core.c | 113 ++++++++++++++++++++++++++++++++++++++++ >>> drivers/usb/dwc3/core.h | 17 +++++- >>> 2 files changed, 129 insertions(+), 1 deletion(-) >>> >>> diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c >>> index 0beaab932e7d..e983aef1fb93 100644 >>> --- a/drivers/usb/dwc3/core.c >>> +++ b/drivers/usb/dwc3/core.c >>> @@ -1767,6 +1767,104 @@ static int dwc3_get_clocks(struct dwc3 *dwc) >>> return 0; >>> } >>> +/** >>> + * dwc3_xhci_find_next_ext_cap - Find the offset of the extended >>> capabilities >>> + * with capability ID id. >>> + * >>> + * @base: PCI MMIO registers base address. >>> + * @start: address at which to start looking, (0 or HCC_PARAMS to >>> start at >>> + * beginning of list) >>> + * @id: Extended capability ID to search for, or 0 for the next >>> + * capability >>> + * >>> + * Returns the offset of the next matching extended capability >>> structure. >>> + * Some capabilities can occur several times, e.g., the >>> XHCI_EXT_CAPS_PROTOCOL, >>> + * and this provides a way to find them all. >>> + */ >>> +static int dwc3_xhci_find_next_ext_cap(void __iomem *base, u32 >>> start, int id) >>> +{ >>> + u32 val; >>> + u32 next; >>> + u32 offset; >>> + >>> + offset = start; >>> + if (!start || start == XHCI_HCC_PARAMS_OFFSET) { >>> + val = readl(base + XHCI_HCC_PARAMS_OFFSET); >>> + if (val == ~0) >>> + return 0; >>> + offset = XHCI_HCC_EXT_CAPS(val) << 2; >>> + if (!offset) >>> + return 0; >>> + } >>> + do { >>> + val = readl(base + offset); >>> + if (val == ~0) >>> + return 0; >>> + if (offset != start && (id == 0 || XHCI_EXT_CAPS_ID(val) == >>> id)) >>> + return offset; >>> + >>> + next = XHCI_EXT_CAPS_NEXT(val); >>> + offset += next << 2; >>> + } while (next); >>> + >>> + return 0; >>> +} >> >> You should not make another copy of xhci_find_next_ext_cap(), but rather >> use it directly. >> >> We already have drivers outside of usb/host using this function so it >> should be fine to do the same for now: >> >> #include "../host/xhci-ext-caps.h" >> > Hi Johan, > > This was the approach which we followed when we first introduced the > patch [1]. But Thinh suggested to duplicate code so that we can avoid > any dependency on xhci (which seems to be right). So since its just one > function, I duplicated it here. > Hi Thinh, Would like to know your opinion here on how to proceed further. Regards, Krishna, > >>> +static int dwc3_read_port_info(struct dwc3 *dwc) >>> +{ >>> + void __iomem *regs; >> >> Call this one 'base' instead. >> >>> + u32 offset; >>> + u32 temp; >> >> I see that the xhci driver use 'temp' for this, but I'd prefer 'val'. >> >>> + u8 major_revision; >>> + int ret = 0; >>> + >>> + /* >>> + * Remap xHCI address space to access XHCI ext cap regs, >>> + * since it is needed to get port info. >>> + */ >>> + regs = ioremap(dwc->xhci_resources[0].start, >>> + resource_size(&dwc->xhci_resources[0])); >>> + if (IS_ERR(regs)) >>> + return PTR_ERR(regs); >>> + >>> + offset = dwc3_xhci_find_next_ext_cap(regs, 0, >>> + XHCI_EXT_CAPS_PROTOCOL); >>> + while (offset) { >> >> This would be better implemented as a do-while loop (cf. >> xdbc_reset_debug_port()). >> >>> + temp = readl(regs + offset); >>> + major_revision = XHCI_EXT_PORT_MAJOR(temp); >>> + >>> + temp = readl(regs + offset + 0x08); >> >> We should try to avoid magic constants, but I see that we already have >> cases accessing these fields like this. >> >>> + if (major_revision == 0x03) { >>> + dwc->num_usb3_ports += XHCI_EXT_PORT_COUNT(temp); >>> + } else if (major_revision <= 0x02) { >>> + dwc->num_usb2_ports += XHCI_EXT_PORT_COUNT(temp); >>> + } else { >>> + dev_err(dwc->dev, >>> + "Unrecognized port major revision %d\n", >>> major_revision); >> >> Please add a line break after the string. >> >> Perhaps this should be handles as in xhci core by simply warning and >> continuing instead. >> > I broke the loop and went to unmap as we are not sure what values would > be read. Any use of continuing ? > >>> + ret = -EINVAL; >>> + goto unmap_reg; >>> + } >>> + >>> + offset = dwc3_xhci_find_next_ext_cap(regs, offset, >>> + XHCI_EXT_CAPS_PROTOCOL); >>> + } >>> + >>> + temp = readl(regs + DWC3_XHCI_HCSPARAMS1); >>> + if (HCS_MAX_PORTS(temp) != (dwc->num_usb3_ports + >>> dwc->num_usb2_ports)) { >>> + dev_err(dwc->dev, >>> + "Mismatched reported MAXPORTS (%d)\n", >>> HCS_MAX_PORTS(temp)); >>> + ret = -EINVAL; >>> + goto unmap_reg; >>> + } >> >> Not sure this is needed either. >> >> Could this risk regressing platforms which does not have currently have >> all PHYs described in DT? >> > No, it doesn't. AFAIK, this only tells how many ports are present as per > the core consultant configuration of the device. I tried to explain what > would happen incase phy's are not present in DT in [2] & [3]. > >> You do however need to make sure that both num_usb<n>_ports is no larger >> than MAX_PORTS_SUPPORTED to avoid memory corruption when you're adding >> fixed sized arrays for the PHYs later in the series. >> >>> + >>> + dev_dbg(dwc->dev, >>> + "hs-ports: %d ss-ports: %d\n", dwc->num_usb2_ports, >>> dwc->num_usb3_ports); >> >> Use %u for unsigned values. >> >> And please try to stay within 80 columns. >> > Thanks for catching this potential bug. Will add that if check as well > in v9. > >>> + >>> +unmap_reg: >>> + iounmap(regs); >>> + return ret; >>> +} >>> + >>> static int dwc3_probe(struct platform_device *pdev) >>> { >>> struct device *dev = &pdev->dev; >>> @@ -1774,6 +1872,7 @@ static int dwc3_probe(struct platform_device >>> *pdev) >>> void __iomem *regs; >>> struct dwc3 *dwc; >>> int ret; >>> + unsigned int hw_mode; >>> dwc = devm_kzalloc(dev, sizeof(*dwc), GFP_KERNEL); >>> if (!dwc) >>> @@ -1843,6 +1942,20 @@ static int dwc3_probe(struct platform_device >>> *pdev) >>> goto err_disable_clks; >>> } >>> + /* >>> + * Currently DWC3 controllers that are host-only capable >>> + * support Multiport >> >> Are you missing an "only" after "Currently" above? >> > Please add a full stop. >> >>> + */ >>> + hw_mode = DWC3_GHWPARAMS0_MODE(dwc->hwparams.hwparams0); >>> + if (hw_mode == DWC3_GHWPARAMS0_MODE_HOST) { >>> + ret = dwc3_read_port_info(dwc); >>> + if (ret) >>> + goto err_disable_clks; >>> + } else { >>> + dwc->num_usb2_ports = 1; >>> + dwc->num_usb3_ports = 1; >>> + } >>> + >>> spin_lock_init(&dwc->lock); >>> mutex_init(&dwc->mutex); >>> diff --git a/drivers/usb/dwc3/core.h b/drivers/usb/dwc3/core.h >>> index d56457c02996..d3401963bc27 100644 >>> --- a/drivers/usb/dwc3/core.h >>> +++ b/drivers/usb/dwc3/core.h >>> @@ -35,6 +35,17 @@ >>> #define DWC3_MSG_MAX 500 >>> +/* Define XHCI Extcap register offsets for getting multiport info */ >>> +#define XHCI_HCC_PARAMS_OFFSET 0x10 >>> +#define DWC3_XHCI_HCSPARAMS1 0x04 >>> +#define XHCI_EXT_CAPS_PROTOCOL 2 >>> +#define XHCI_HCC_EXT_CAPS(x) (((x) >> 16) & 0xffff) >>> +#define XHCI_EXT_CAPS_ID(x) (((x) >> 0) & 0xff) >>> +#define XHCI_EXT_CAPS_NEXT(x) (((x) >> 8) & 0xff) >>> +#define XHCI_EXT_PORT_MAJOR(x) (((x) >> 24) & 0xff) >>> +#define XHCI_EXT_PORT_COUNT(x) (((x) >> 8) & 0xff) >>> +#define HCS_MAX_PORTS(x) (((x) >> 24) & 0x7f) >>> + >> >> You should use the xhci defines instead of these copies too. >> >>> /* Global constants */ >>> #define DWC3_PULL_UP_TIMEOUT 500 /* ms */ >>> #define DWC3_BOUNCE_SIZE 1024 /* size of a superspeed bulk */ >>> @@ -1025,6 +1036,8 @@ struct dwc3_scratchpad_array { >>> * @usb_psy: pointer to power supply interface. >>> * @usb2_phy: pointer to USB2 PHY >>> * @usb3_phy: pointer to USB3 PHY >>> + * @num_usb2_ports: number of usb2 ports. >>> + * @num_usb3_ports: number of usb3 ports. >> >> Use upper case "USBn" and drop the full stops for consistency. >> >> Please move these after the PHY structures. >> >>> * @usb2_generic_phy: pointer to USB2 PHY >>> * @usb3_generic_phy: pointer to USB3 PHY >>> * @phys_ready: flag to indicate that PHYs are ready >>> @@ -1162,6 +1175,9 @@ struct dwc3 { >>> struct usb_phy *usb2_phy; >>> struct usb_phy *usb3_phy; >>> + u8 num_usb2_ports; >>> + u8 num_usb3_ports; >>> + >>> struct phy *usb2_generic_phy; >>> struct phy *usb3_generic_phy; >>> @@ -1649,5 +1665,4 @@ static inline int dwc3_ulpi_init(struct dwc3 *dwc) >>> static inline void dwc3_ulpi_exit(struct dwc3 *dwc) >>> { } >>> #endif >>> - >> >> This is an unrelated change that should be dropped. >> >>> #endif /* __DRIVERS_USB_DWC3_CORE_H */ >> >> Johan > > [1]: > https://lore.kernel.org/all/20230310163420.7582-3-quic_kriskura@quicinc.com/ > > [2]: > https://lore.kernel.org/all/4eb26a54-148b-942f-01c6-64e66541de8b@quicinc.com/ > > [3]: > https://lore.kernel.org/all/966c1001-6d64-9163-0c07-96595156fc8c@quicinc.com/ > > Thanks for the review and comments 🙂. Will make sure to fix them in v9. > > Regards, > Krishna,
On Wed, May 17, 2023, Krishna Kurapati PSSNV wrote: > > > On 5/16/2023 8:32 PM, Krishna Kurapati PSSNV wrote: > > > > > > On 5/16/2023 5:41 PM, Johan Hovold wrote: > > > On Sun, May 14, 2023 at 11:19:11AM +0530, Krishna Kurapati wrote: > > > > Currently host-only capable DWC3 controllers support Multiport. > > > > Temporarily map XHCI address space for host-only controllers and parse > > > > XHCI Extended Capabilities registers to read number of usb2 ports and > > > > usb3 ports present on multiport controller. Each USB Port is at least HS > > > > capable. > > > > > > > > The port info for usb2 and usb3 phy are identified as num_usb2_ports > > > > and num_usb3_ports. The intention is as follows: > > > > > > > > Wherever we need to perform phy operations like: > > > > > > > > LOOP_OVER_NUMBER_OF_AVAILABLE_PORTS() > > > > { > > > > phy_set_mode(dwc->usb2_generic_phy[i], PHY_MODE_USB_HOST); > > > > phy_set_mode(dwc->usb3_generic_phy[i], PHY_MODE_USB_HOST); > > > > } > > > > > > > > If number of usb2 ports is 3, loop can go from index 0-2 for > > > > usb2_generic_phy. If number of usb3-ports is 2, we don't know for sure, > > > > if the first 2 ports are SS capable or some other ports like (2 and 3) > > > > are SS capable. So instead, num_usb2_ports is used to loop around all > > > > phy's (both hs and ss) for performing phy operations. If any > > > > usb3_generic_phy turns out to be NULL, phy operation just bails out. > > > > > > > > num_usb3_ports is used to modify GUSB3PIPECTL registers while setting up > > > > phy's as we need to know how many SS capable ports are there for this. > > > > > > > > Signed-off-by: Krishna Kurapati <quic_kriskura@quicinc.com> > > > > --- > > > > drivers/usb/dwc3/core.c | 113 ++++++++++++++++++++++++++++++++++++++++ > > > > drivers/usb/dwc3/core.h | 17 +++++- > > > > 2 files changed, 129 insertions(+), 1 deletion(-) > > > > > > > > diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c > > > > index 0beaab932e7d..e983aef1fb93 100644 > > > > --- a/drivers/usb/dwc3/core.c > > > > +++ b/drivers/usb/dwc3/core.c > > > > @@ -1767,6 +1767,104 @@ static int dwc3_get_clocks(struct dwc3 *dwc) > > > > return 0; > > > > } > > > > +/** > > > > + * dwc3_xhci_find_next_ext_cap - Find the offset of the > > > > extended capabilities > > > > + * with capability ID id. > > > > + * > > > > + * @base: PCI MMIO registers base address. > > > > + * @start: address at which to start looking, (0 or > > > > HCC_PARAMS to start at > > > > + * beginning of list) > > > > + * @id: Extended capability ID to search for, or 0 for the next > > > > + * capability > > > > + * > > > > + * Returns the offset of the next matching extended capability > > > > structure. > > > > + * Some capabilities can occur several times, e.g., the > > > > XHCI_EXT_CAPS_PROTOCOL, > > > > + * and this provides a way to find them all. > > > > + */ > > > > +static int dwc3_xhci_find_next_ext_cap(void __iomem *base, u32 > > > > start, int id) > > > > +{ > > > > + u32 val; > > > > + u32 next; > > > > + u32 offset; > > > > + > > > > + offset = start; > > > > + if (!start || start == XHCI_HCC_PARAMS_OFFSET) { > > > > + val = readl(base + XHCI_HCC_PARAMS_OFFSET); > > > > + if (val == ~0) > > > > + return 0; > > > > + offset = XHCI_HCC_EXT_CAPS(val) << 2; > > > > + if (!offset) > > > > + return 0; > > > > + } > > > > + do { > > > > + val = readl(base + offset); > > > > + if (val == ~0) > > > > + return 0; > > > > + if (offset != start && (id == 0 || > > > > XHCI_EXT_CAPS_ID(val) == id)) > > > > + return offset; > > > > + > > > > + next = XHCI_EXT_CAPS_NEXT(val); > > > > + offset += next << 2; > > > > + } while (next); > > > > + > > > > + return 0; > > > > +} > > > > > > You should not make another copy of xhci_find_next_ext_cap(), but rather > > > use it directly. > > > > > > We already have drivers outside of usb/host using this function so it > > > should be fine to do the same for now: > > > > > > #include "../host/xhci-ext-caps.h" > > > > > Hi Johan, > > > > This was the approach which we followed when we first introduced the > > patch [1]. But Thinh suggested to duplicate code so that we can avoid > > any dependency on xhci (which seems to be right). So since its just one > > function, I duplicated it here. > > > Hi Thinh, > > Would like to know your opinion here on how to proceed further. > > Regards, > Krishna, Please keep them separated. The xhci-ext-caps.h is for xhci driver only. It's not meant to be exposed to other drivers. Same with other *.h files under drivers/usb/host. Thanks, Thinh
Hi Krishna, Please try to remember to trim unneeded context when replying so that it's easier to find your replies and also to catch up on threads (e.g. when later reading the mail archives). On Tue, May 16, 2023 at 08:32:00PM +0530, Krishna Kurapati PSSNV wrote: > On 5/16/2023 5:41 PM, Johan Hovold wrote: > > On Sun, May 14, 2023 at 11:19:11AM +0530, Krishna Kurapati wrote: > > You should not make another copy of xhci_find_next_ext_cap(), but rather > > use it directly. > > > > We already have drivers outside of usb/host using this function so it > > should be fine to do the same for now: > > > > #include "../host/xhci-ext-caps.h" > This was the approach which we followed when we first introduced the > patch [1]. But Thinh suggested to duplicate code so that we can avoid > any dependency on xhci (which seems to be right). So since its just one > function, I duplicated it here. Ok, fair enough. I still think we should not be duplicating the xhci definitions like this even if we were to copy the helper to avoid any future dependencies on xhci (it's currently an inline function, which is also not very nice). I'll take closer look at the rest of the series as there are a few more of these layering violations which we should try to avoid. > >> + offset = dwc3_xhci_find_next_ext_cap(regs, 0, > >> + XHCI_EXT_CAPS_PROTOCOL); > >> + while (offset) { > >> + temp = readl(regs + offset); > >> + major_revision = XHCI_EXT_PORT_MAJOR(temp); > >> + > >> + temp = readl(regs + offset + 0x08); > >> + if (major_revision == 0x03) { > >> + dwc->num_usb3_ports += XHCI_EXT_PORT_COUNT(temp); > >> + } else if (major_revision <= 0x02) { > >> + dwc->num_usb2_ports += XHCI_EXT_PORT_COUNT(temp); > >> + } else { > >> + dev_err(dwc->dev, > >> + "Unrecognized port major revision %d\n", major_revision); > > Perhaps this should be handles as in xhci core by simply warning and > > continuing instead. > > > I broke the loop and went to unmap as we are not sure what values would > be read. Any use of continuing ? Mostly to align with xhci core which currently handles this case. It would not not work unless you get rid of the max-ports check below though. > >> + ret = -EINVAL; > >> + goto unmap_reg; > >> + } > >> + > >> + offset = dwc3_xhci_find_next_ext_cap(regs, offset, > >> + XHCI_EXT_CAPS_PROTOCOL); > >> + } > >> + > >> + temp = readl(regs + DWC3_XHCI_HCSPARAMS1); > >> + if (HCS_MAX_PORTS(temp) != (dwc->num_usb3_ports + dwc->num_usb2_ports)) { > >> + dev_err(dwc->dev, > >> + "Mismatched reported MAXPORTS (%d)\n", HCS_MAX_PORTS(temp)); > >> + ret = -EINVAL; > >> + goto unmap_reg; > >> + } > > > > Not sure this is needed either. > > > > Could this risk regressing platforms which does not have currently have > > all PHYs described in DT? > > > No, it doesn't. AFAIK, this only tells how many ports are present as per > the core consultant configuration of the device. I tried to explain what > would happen incase phy's are not present in DT in [2] & [3]. Right, whether the PHYs are described in DT is not directly related to this. As long as HCS_MAX_PORTS by definition (assumption) is always (dwc->num_usb3_ports + dwc->num_usb2_ports) any such machines would continue to work. But if you want to catch machines where this assumption does not hold, you could also end up regressing machines which have so far been working despite these numbers not adding up. That may be acceptable, but I'm still not sure what the value of this check is (e.g. as xhci core will handle basic sanity checks like usb2 + usb3 <= max_ports). Johan
On Wed, May 17, 2023 at 03:21:24AM +0000, Thinh Nguyen wrote: > On Wed, May 17, 2023, Krishna Kurapati PSSNV wrote: > > On 5/16/2023 8:32 PM, Krishna Kurapati PSSNV wrote: > > > On 5/16/2023 5:41 PM, Johan Hovold wrote: > > > > You should not make another copy of xhci_find_next_ext_cap(), but rather > > > > use it directly. > > > > > > > > We already have drivers outside of usb/host using this function so it > > > > should be fine to do the same for now: > > > > > > > > #include "../host/xhci-ext-caps.h" > > > This was the approach which we followed when we first introduced the > > > patch [1]. But Thinh suggested to duplicate code so that we can avoid > > > any dependency on xhci (which seems to be right). So since its just one > > > function, I duplicated it here. > > Would like to know your opinion here on how to proceed further. > Please keep them separated. The xhci-ext-caps.h is for xhci driver only. > It's not meant to be exposed to other drivers. Same with other *.h files > under drivers/usb/host. As I mentioned earlier, it is already used by the xdbc earlyprintk driver which lives outside of drivers/usb/host, even if such a debug driver could be considered a special case. If it turns out that there's no way to avoid mapping those registers from the qcom glue driver, then I think at least the register defines need to be provided in a global header rather than being copied verbatim. But hopefully that can be avoided too as the xhci driver already parses these registers and stores the port information, even if accessing that data may require a bit more work currently. Johan
On 5/17/2023 1:05 PM, Johan Hovold wrote: >>> You should not make another copy of xhci_find_next_ext_cap(), but rather >>> use it directly. >>> >>> We already have drivers outside of usb/host using this function so it >>> should be fine to do the same for now: >>> >>> #include "../host/xhci-ext-caps.h" > >> This was the approach which we followed when we first introduced the >> patch [1]. But Thinh suggested to duplicate code so that we can avoid >> any dependency on xhci (which seems to be right). So since its just one >> function, I duplicated it here. > > Ok, fair enough. I still think we should not be duplicating the > xhci definitions like this even if we were to copy the helper to avoid > any future dependencies on xhci (it's currently an inline function, > which is also not very nice). > > I'll take closer look at the rest of the series as there are a few more > of these layering violations which we should try to avoid. > >>>> + offset = dwc3_xhci_find_next_ext_cap(regs, 0, >>>> + XHCI_EXT_CAPS_PROTOCOL); >>>> + while (offset) { > >>>> + temp = readl(regs + offset); >>>> + major_revision = XHCI_EXT_PORT_MAJOR(temp); >>>> + >>>> + temp = readl(regs + offset + 0x08); > >>>> + if (major_revision == 0x03) { >>>> + dwc->num_usb3_ports += XHCI_EXT_PORT_COUNT(temp); >>>> + } else if (major_revision <= 0x02) { >>>> + dwc->num_usb2_ports += XHCI_EXT_PORT_COUNT(temp); >>>> + } else { >>>> + dev_err(dwc->dev, >>>> + "Unrecognized port major revision %d\n", major_revision); > >>> Perhaps this should be handles as in xhci core by simply warning and >>> continuing instead. >>> >> I broke the loop and went to unmap as we are not sure what values would >> be read. Any use of continuing ? > > Mostly to align with xhci core which currently handles this case. It > would not not work unless you get rid of the max-ports check below > though. > >>>> + ret = -EINVAL; >>>> + goto unmap_reg; >>>> + } >>>> + >>>> + offset = dwc3_xhci_find_next_ext_cap(regs, offset, >>>> + XHCI_EXT_CAPS_PROTOCOL); >>>> + } >>>> + >>>> + temp = readl(regs + DWC3_XHCI_HCSPARAMS1); >>>> + if (HCS_MAX_PORTS(temp) != (dwc->num_usb3_ports + dwc->num_usb2_ports)) { >>>> + dev_err(dwc->dev, >>>> + "Mismatched reported MAXPORTS (%d)\n", HCS_MAX_PORTS(temp)); >>>> + ret = -EINVAL; >>>> + goto unmap_reg; >>>> + } >>> >>> Not sure this is needed either. >>> >>> Could this risk regressing platforms which does not have currently have >>> all PHYs described in DT? >>> >> No, it doesn't. AFAIK, this only tells how many ports are present as per >> the core consultant configuration of the device. I tried to explain what >> would happen incase phy's are not present in DT in [2] & [3]. > > Right, whether the PHYs are described in DT is not directly related to > this. > > As long as HCS_MAX_PORTS by definition (assumption) is always > (dwc->num_usb3_ports + dwc->num_usb2_ports) any such machines would > continue to work. > > But if you want to catch machines where this assumption does not hold, > you could also end up regressing machines which have so far been working > despite these numbers not adding up. > > That may be acceptable, but I'm still not sure what the value of this > check is (e.g. as xhci core will handle basic sanity checks like usb2 + > usb3 <= max_ports). > Hi Johan, Thanks for the review comments. Ideally the HCC_PARAMS1 must indicate total number of ports supported. If not then I believe the core consultant configuration is wrong. According to the spec: "The MaxPorts value in the HCSPARAMS1 register defines the number of Port Register Sets (e.g. PORTSC, PORTPMSC, and PORTLI register sets)." So shouldn't the (usb2+usb3 ports be equal to MaxPorts to ensure each port properly accesses the respective PortSC etc., ? Do we have any machines today that support HOST_ONLY_CONFIG indicated in HWPARAMS0 that support multiport and violate this rule ? Regards, Krishna,
On Wed, May 17, 2023 at 05:51:45PM +0530, Krishna Kurapati PSSNV wrote: > On 5/17/2023 1:05 PM, Johan Hovold wrote: > >>>> + temp = readl(regs + DWC3_XHCI_HCSPARAMS1); > >>>> + if (HCS_MAX_PORTS(temp) != (dwc->num_usb3_ports + dwc->num_usb2_ports)) { > >>>> + dev_err(dwc->dev, > >>>> + "Mismatched reported MAXPORTS (%d)\n", HCS_MAX_PORTS(temp)); > >>>> + ret = -EINVAL; > >>>> + goto unmap_reg; > >>>> + } > >>> > >>> Not sure this is needed either. > >>> > >>> Could this risk regressing platforms which does not have currently have > >>> all PHYs described in DT? > >>> > >> No, it doesn't. AFAIK, this only tells how many ports are present as per > >> the core consultant configuration of the device. I tried to explain what > >> would happen incase phy's are not present in DT in [2] & [3]. > > > > Right, whether the PHYs are described in DT is not directly related to > > this. > > > > As long as HCS_MAX_PORTS by definition (assumption) is always > > (dwc->num_usb3_ports + dwc->num_usb2_ports) any such machines would > > continue to work. > > > > But if you want to catch machines where this assumption does not hold, > > you could also end up regressing machines which have so far been working > > despite these numbers not adding up. > > > > That may be acceptable, but I'm still not sure what the value of this > > check is (e.g. as xhci core will handle basic sanity checks like usb2 + > > usb3 <= max_ports). > Thanks for the review comments. Ideally the HCC_PARAMS1 must indicate > total number of ports supported. If not then I believe the core > consultant configuration is wrong. > > According to the spec: > > "The MaxPorts value in the HCSPARAMS1 register defines the number of > Port Register Sets (e.g. PORTSC, PORTPMSC, and PORTLI register sets)." > > So shouldn't the (usb2+usb3 ports be equal to MaxPorts to ensure each > port properly accesses the respective PortSC etc., ? Sure, that's what is expected, but why do you need to add a check for this in the glue driver all of a sudden? Your series does not seem to rely on this. This is the xHCI driver's business (as is parsing these registers in the first place, really). Johan
On Sun, May 14, 2023 at 11:19:13AM +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. > > Co-developed-by: Harsh Agarwal <quic_harshq@quicinc.com> > Signed-off-by: Krishna Kurapati <quic_kriskura@quicinc.com> If Harsh is the primary author you need to add a From: line at the beginning of the patch. Either way, you need his SoB as well as your Co-developed-by tag. All this is documented under Documentation/process/ somewhere. > +/** > + * dwc3_phy_setup - Configure USB PHY Interface of DWC3 Core > + * @dwc: Pointer to our controller context structure > + * > + * Returns 0 on success. The USB PHY interfaces are configured but not > + * initialized. The PHY interfaces and the PHYs get initialized together with > + * the core in dwc3_core_init. > + */ > +static int dwc3_phy_setup(struct dwc3 *dwc) > +{ > + int i; > + int ret; Please try to use reverse xmas style for declaration throughout (i.e. place the longest declarations first). > + > + for (i = 0; i < dwc->num_usb3_ports; i++) { > + ret = dwc3_ss_phy_setup(dwc, i); > + if (ret) > + return ret; > + } > + > + for (i = 0; i < dwc->num_usb2_ports; i++) { > + ret = dwc3_hs_phy_setup(dwc, i); > + if (ret) > + return ret; > + } > > return 0; > } > @@ -744,22 +777,38 @@ static int dwc3_phy_setup(struct dwc3 *dwc) > static int dwc3_phy_init(struct dwc3 *dwc) > { > int ret; > + int i; > + int j; > > usb_phy_init(dwc->usb2_phy); > usb_phy_init(dwc->usb3_phy); > > - ret = phy_init(dwc->usb2_generic_phy); > - if (ret < 0) > - goto err_shutdown_usb3_phy; > + for (i = 0; i < dwc->num_usb2_ports; i++) { > + ret = phy_init(dwc->usb2_generic_phy[i]); > + if (ret < 0) { > + /* clean up prior initialized HS PHYs */ > + for (j = 0; j < i; j++) > + phy_exit(dwc->usb2_generic_phy[j]); > + goto err_shutdown_usb3_phy; > + } > + } > > - ret = phy_init(dwc->usb3_generic_phy); > - if (ret < 0) > - goto err_exit_usb2_phy; > + for (i = 0; i < dwc->num_usb2_ports; i++) { > + ret = phy_init(dwc->usb3_generic_phy[i]); > + if (ret < 0) { > + /* clean up prior initialized SS PHYs */ > + for (j = 0; j < i; j++) > + phy_exit(dwc->usb3_generic_phy[j]); > + goto err_exit_usb2_phy; > + } > + } The above is probably better implemented as a single loop over num_usb2_ports where you enable each USB2 and USB3 PHY. On errors you use the loop index to disable the already enabled PHYs in reverse order below (after disabling the USB2 PHY if USB3 phy init fails). > return 0; > > err_exit_usb2_phy: > - phy_exit(dwc->usb2_generic_phy); > + for (i = 0; i < dwc->num_usb2_ports; i++) > + phy_exit(dwc->usb2_generic_phy[i]); > + No need for a newline separator. > err_shutdown_usb3_phy: > usb_phy_shutdown(dwc->usb3_phy); > usb_phy_shutdown(dwc->usb2_phy); > @@ -769,8 +818,12 @@ static int dwc3_phy_init(struct dwc3 *dwc) > > static void dwc3_phy_exit(struct dwc3 *dwc) > { > - phy_exit(dwc->usb3_generic_phy); > - phy_exit(dwc->usb2_generic_phy); > + int i; > + > + for (i = 0; i < dwc->num_usb2_ports; i++) { > + phy_exit(dwc->usb3_generic_phy[i]); > + phy_exit(dwc->usb2_generic_phy[i]); > + } For symmetry, I'd probably do this in reverse order to. > usb_phy_shutdown(dwc->usb3_phy); > usb_phy_shutdown(dwc->usb2_phy); > @@ -779,22 +832,38 @@ static void dwc3_phy_exit(struct dwc3 *dwc) > static int dwc3_phy_power_on(struct dwc3 *dwc) > { > int ret; > + int i; > + int j; > > usb_phy_set_suspend(dwc->usb2_phy, 0); > usb_phy_set_suspend(dwc->usb3_phy, 0); > > - ret = phy_power_on(dwc->usb2_generic_phy); > - if (ret < 0) > - goto err_suspend_usb3_phy; > + for (i = 0; i < dwc->num_usb2_ports; i++) { > + ret = phy_power_on(dwc->usb2_generic_phy[i]); > + if (ret < 0) { > + /* Turn off prior ON'ed HS Phy's */ > + for (j = 0; j < i; j++) > + phy_power_off(dwc->usb2_generic_phy[j]); > + goto err_suspend_usb3_phy; > + } > + } > > - ret = phy_power_on(dwc->usb3_generic_phy); > - if (ret < 0) > - goto err_power_off_usb2_phy; > + for (i = 0; i < dwc->num_usb2_ports; i++) { > + ret = phy_power_on(dwc->usb3_generic_phy[i]); > + if (ret < 0) { > + /* Turn of prior ON'ed SS Phy's */ > + for (j = 0; j < i; j++) > + phy_power_off(dwc->usb3_generic_phy[j]); > + goto err_power_off_usb2_phy; > + } > + } These loops should be merged too as for phy_init. > return 0; > > err_power_off_usb2_phy: > - phy_power_off(dwc->usb2_generic_phy); > + for (i = 0; i < dwc->num_usb2_ports; i++) > + phy_power_off(dwc->usb2_generic_phy[i]); > + > err_suspend_usb3_phy: > usb_phy_set_suspend(dwc->usb3_phy, 1); > usb_phy_set_suspend(dwc->usb2_phy, 1); > +static int dwc3_get_multiport_phys(struct dwc3 *dwc) > +{ > + int ret; > + struct device *dev = dwc->dev; > + int i; > + char phy_name[11]; As an example, for reverse xmas style this should be struct device *dev = dwc->dev; char phy_name[11]; int ret; int i; which tends to be more readable. > + > + /* > + * Each port is at least HS capable. So loop over num_usb2_ports > + * to get available phy's. > + */ > + for (i = 0; i < dwc->num_usb2_ports; i++) { > + sprintf(phy_name, "usb2-port%d", i); > + dwc->usb2_generic_phy[i] = devm_phy_get(dev, phy_name); > + if (IS_ERR(dwc->usb2_generic_phy[i])) { > + ret = PTR_ERR(dwc->usb2_generic_phy[i]); > + if (ret == -ENOSYS || ret == -ENODEV) > + dwc->usb2_generic_phy[i] = NULL; > + else > + return dev_err_probe(dev, ret, "usb2 phy: %s not configured\n", phy_name); This can just be "phy %s not configured" or perhaps better "failed to lookup phy %s" > + } > + > + sprintf(phy_name, "usb3-port%d", i); > + dwc->usb3_generic_phy[i] = devm_phy_get(dev, phy_name); > + if (IS_ERR(dwc->usb3_generic_phy[i])) { > + ret = PTR_ERR(dwc->usb3_generic_phy[i]); > + if (ret == -ENOSYS || ret == -ENODEV) > + dwc->usb3_generic_phy[i] = NULL; > + else > + return dev_err_probe(dev, ret, "usb3 phy: %s not configured\n", phy_name); > + } > + } > + > + return 0; > +} I think you should drop this helper and use the same loop for both single and multiport controllers below. Just use the old phy names if num_usb2_ports is 1, for example: if (dwc->num_usb2_ports == 1) sprintf(phy_name, "usb2-phy"); else sprintf(phy_name, "usb2-port%d", i); > + > static int dwc3_core_get_phy(struct dwc3 *dwc) > { > struct device *dev = dwc->dev; > @@ -1314,20 +1428,23 @@ static int dwc3_core_get_phy(struct dwc3 *dwc) > return dev_err_probe(dev, ret, "no usb3 phy configured\n"); > } > > - dwc->usb2_generic_phy = devm_phy_get(dev, "usb2-phy"); > - if (IS_ERR(dwc->usb2_generic_phy)) { > - ret = PTR_ERR(dwc->usb2_generic_phy); > + if (dwc->num_usb2_ports > 1) > + return dwc3_get_multiport_phys(dwc); > + > + dwc->usb2_generic_phy[0] = devm_phy_get(dev, "usb2-phy"); > + if (IS_ERR(dwc->usb2_generic_phy[0])) { > + ret = PTR_ERR(dwc->usb2_generic_phy[0]); > if (ret == -ENOSYS || ret == -ENODEV) > - dwc->usb2_generic_phy = NULL; > + dwc->usb2_generic_phy[0] = NULL; > else > return dev_err_probe(dev, ret, "no usb2 phy configured\n"); > } > > - dwc->usb3_generic_phy = devm_phy_get(dev, "usb3-phy"); > - if (IS_ERR(dwc->usb3_generic_phy)) { > - ret = PTR_ERR(dwc->usb3_generic_phy); > + dwc->usb3_generic_phy[0] = devm_phy_get(dev, "usb3-phy"); > + if (IS_ERR(dwc->usb3_generic_phy[0])) { > + ret = PTR_ERR(dwc->usb3_generic_phy[0]); > if (ret == -ENOSYS || ret == -ENODEV) > - dwc->usb3_generic_phy = NULL; > + dwc->usb3_generic_phy[0] = NULL; > else > return dev_err_probe(dev, ret, "no usb3 phy configured\n"); > } > static int dwc3_suspend_common(struct dwc3 *dwc, pm_message_t msg) > { > + int i; Add this declaration last instead (and similar throughout). > unsigned long flags; > u32 reg; > diff --git a/drivers/usb/dwc3/core.h b/drivers/usb/dwc3/core.h > index d3401963bc27..84f6303922aa 100644 > --- a/drivers/usb/dwc3/core.h > +++ b/drivers/usb/dwc3/core.h > @@ -35,6 +35,9 @@ > > #define DWC3_MSG_MAX 500 > > +/* Number of ports supported by a multiport controller */ > +#define MAX_PORTS_SUPPORTED 4 This define should have a DWC3 prefix (e.g. DWC3_MAX_PORTS, "SUPPORTED" doesn't seem to add much). Is this just an arbitrary implementation limit? > + > /* Define XHCI Extcap register offsets for getting multiport info */ > #define XHCI_HCC_PARAMS_OFFSET 0x10 > #define DWC3_XHCI_HCSPARAMS1 0x04 > @@ -1038,8 +1041,8 @@ struct dwc3_scratchpad_array { > * @usb3_phy: pointer to USB3 PHY > * @num_usb2_ports: number of usb2 ports. > * @num_usb3_ports: number of usb3 ports. > - * @usb2_generic_phy: pointer to USB2 PHY > - * @usb3_generic_phy: pointer to USB3 PHY > + * @usb2_generic_phy: pointer to array of USB2 PHY > + * @usb3_generic_phy: pointer to array of USB3 PHY > * @phys_ready: flag to indicate that PHYs are ready > * @ulpi: pointer to ulpi interface > * @ulpi_ready: flag to indicate that ULPI is initialized > @@ -1178,8 +1181,8 @@ struct dwc3 { > u8 num_usb2_ports; > u8 num_usb3_ports; > > - struct phy *usb2_generic_phy; > - struct phy *usb3_generic_phy; > + struct phy *usb2_generic_phy[MAX_PORTS_SUPPORTED]; > + struct phy *usb3_generic_phy[MAX_PORTS_SUPPORTED]; As I mentioned in a comment to one of the earlier patches, you need to add a sanity check when parsing the port counts to avoid accessing data beyond these arrays when looping over the PHYs. > > bool phys_ready; Johan
On Tue, May 16, 2023 at 07:49:14AM +0530, Krishna Kurapati PSSNV wrote: > > > On 5/16/2023 3:57 AM, Bjorn Andersson wrote: > > On Sun, May 14, 2023 at 11:19:14AM +0530, Krishna Kurapati wrote: > >> -#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? > 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. Actually, the USB ports are indexed from 1, so the above naming may or may not be correct depending on how they are defined. > >> +}; > >> + > >> 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 don't think we should be adding more of these layering violations. A parent device driver has no business messing with the driver data for a child device which may or may not even have probed yet. I added a FIXME elsewhere in the driver about fixing up the current instances that have already snuck in (which in some sense is even worse by accessing driver data of a grandchild device). We really need to try sort this mess out and how to properly handle the interactions between these layers (e.g. glue, dwc3 core and xhci). This will likely involve adding callbacks from the child to the parent, for example, when the child is suspending. Johan
On Wed, May 17, 2023, Johan Hovold wrote: > On Wed, May 17, 2023 at 03:21:24AM +0000, Thinh Nguyen wrote: > > On Wed, May 17, 2023, Krishna Kurapati PSSNV wrote: > > > On 5/16/2023 8:32 PM, Krishna Kurapati PSSNV wrote: > > > > On 5/16/2023 5:41 PM, Johan Hovold wrote: > > > > > > You should not make another copy of xhci_find_next_ext_cap(), but rather > > > > > use it directly. > > > > > > > > > > We already have drivers outside of usb/host using this function so it > > > > > should be fine to do the same for now: > > > > > > > > > > #include "../host/xhci-ext-caps.h" > > > > > This was the approach which we followed when we first introduced the > > > > patch [1]. But Thinh suggested to duplicate code so that we can avoid > > > > any dependency on xhci (which seems to be right). So since its just one > > > > function, I duplicated it here. > > > > Would like to know your opinion here on how to proceed further. > > > Please keep them separated. The xhci-ext-caps.h is for xhci driver only. > > It's not meant to be exposed to other drivers. Same with other *.h files > > under drivers/usb/host. > > As I mentioned earlier, it is already used by the xdbc earlyprintk > driver which lives outside of drivers/usb/host, even if such a debug > driver could be considered a special case. > > If it turns out that there's no way to avoid mapping those registers > from the qcom glue driver, then I think at least the register defines > need to be provided in a global header rather than being copied > verbatim. It would be good to properly define the global header with common offset/interface that can be public for other drivers. > > But hopefully that can be avoided too as the xhci driver already parses > these registers and stores the port information, even if accessing that > data may require a bit more work currently. > Not many drivers outside of xhci should care about its registers except for some special cases. Even for those special cases, only a small subset of xhci registers are used. So we may not need a global header for that. What we may need is a global header that holds all the xhci quirks/configs with defined interface for drivers outside of xhci can use and pass those configs to xhci (such as from dwc3). This requires some work. Thanks, Thinh
On 5/17/2023 10:07 PM, Johan Hovold wrote: > On Tue, May 16, 2023 at 07:49:14AM +0530, Krishna Kurapati PSSNV wrote: >> >> >> On 5/16/2023 3:57 AM, Bjorn Andersson wrote: >>> On Sun, May 14, 2023 at 11:19:14AM +0530, Krishna Kurapati wrote: > >>>> -#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? > >> 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. > > Actually, the USB ports are indexed from 1, so the above naming may or > may not be correct depending on how they are defined. > Ok, will rename them as PWR_EVNT_IRQx_STAT_REG (x = 0,1,2,3) >>>> +}; >>>> + >>>> 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 don't think we should be adding more of these layering violations. A > parent device driver has no business messing with the driver data for a > child device which may or may not even have probed yet. > > I added a FIXME elsewhere in the driver about fixing up the current > instances that have already snuck in (which in some sense is even worse > by accessing driver data of a grandchild device). > > We really need to try sort this mess out and how to properly handle the > interactions between these layers (e.g. glue, dwc3 core and xhci). This > will likely involve adding callbacks from the child to the parent, for > example, when the child is suspending. > Hi Johan, I agree with you, but in this case I believe there is no other way we can find the number of ports present. Unless its a dt property which parent driver can access the child's of node and get the details. Like done in v4 [1]. But it would be adding redundant data into DT as pointed out by Rob and Krzysztof and so we removed these properties. Also, since this is a read only operation being done and no modifications are being done to driver data of child, is it still not acceptable as both dwc3-qcom and core are tightly coupled entities. [1]: https://lore.kernel.org/all/20230115114146.12628-2-quic_kriskura@quicinc.com/ Regards, Krishna,
On Mon, May 15, 2023 at 03:27:30PM -0700, 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? > > > +}; > > + > > 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()). > I looked at this once more, and realized that I missed the fact that dwc3_qcom_is_host() will happily dereference the drvdata() just a few lines further down... So this is already broken. > So if you're peaking into qcom->dwc3 you need to handle the fact that > dwc might be NULL, here and in resume below. > We need to fix the dwc3 glue design, so that the glue and the core can cooperate - and we have a few other use cases where this is needed (e.g. usb_role_switch propagation to the glue code). Regards, Bjorn > Regards, > Bjorn
Hi Krishna, and sorry about the delay in following up on this. As usual, there are just way too many threads and this one in particular requires a bit of thought. On Sat, May 20, 2023 at 11:18:52PM +0530, Krishna Kurapati PSSNV wrote: > On 5/17/2023 10:07 PM, Johan Hovold wrote: > > I don't think we should be adding more of these layering violations. A > > parent device driver has no business messing with the driver data for a > > child device which may or may not even have probed yet. > > > > I added a FIXME elsewhere in the driver about fixing up the current > > instances that have already snuck in (which in some sense is even worse > > by accessing driver data of a grandchild device). > > > > We really need to try sort this mess out and how to properly handle the > > interactions between these layers (e.g. glue, dwc3 core and xhci). This > > will likely involve adding callbacks from the child to the parent, for > > example, when the child is suspending. > I agree with you, but in this case I believe there is no other way we > can find the number of ports present. Unless its a dt property which > parent driver can access the child's of node and get the details. Like > done in v4 [1]. But it would be adding redundant data into DT as pointed > out by Rob and Krzysztof and so we removed these properties. So there at least two issues with this series: 1. accessing xhci registers from the dwc3 core 2. accessing driver data of a child device 1. The first part about accessing xhci registers goes against the clear separation between glue, core and xhci that Felipe tried to maintain. I'm not entirely against doing this from the core driver before registering the xhci platform device as the registers are unmapped afterwards. But if this is to be allowed, then the implementation should be shared with xhci rather than copied verbatim. The alternative that avoids this issue entirely could indeed be to simply count the number of PHYs described in DT as Rob initially suggested. Why would that not work? 2. The driver is already accessing driver data of the child device so arguably your series is not really making things much worse than they already are. I've just sent a couple of fixes to address some of the symptoms of this layering violation here: https://lore.kernel.org/lkml/20230607100540.31045-1-johan+linaro@kernel.org/ Getting this fixed properly is going to take a bit of work, and depending on how your multiport wake up implementation is going to look like, adding support for multiport controllers may not make this much harder to address. So perhaps we can indeed merge support for multiport and then get back to cleaning this up. Johan
On Fri, May 26, 2023 at 08:55:22PM +0530, Krishna Kurapati PSSNV wrote: > On 5/26/2023 8:25 AM, Bjorn Andersson wrote: > > We need to fix the dwc3 glue design, so that the glue and the core can > > cooperate - and we have a few other use cases where this is needed (e.g. > > usb_role_switch propagation to the glue code). > Thanks for the comments on this patch. I had some suggestions come in > from the team internally: > > 1. To use the notifier call available in drivers/usb/core/notify.c and > make sure that host mode is enabled. That way we can access dwc or xhci > without any issue. I don't think this is a good idea and instead the callbacks should be dedicated for the xhci and dwc3 drivers. A struct with callbacks can be passed down to the child devices, which call back into the drivers of their parents for notifications and when they need services from them (e.g. during suspend or on role changes). > 2. For this particular case where we are trying to get info on number of > ports present (dwc->num_usb2_ports), we can add compatible data for > sc8280-mp and provide input to driver telling num ports is 4. That may also work as a way to avoid parsing the xhci registers, but I'm still not sure why simply counting the PHYs in DT would not work. Johan
On Wed, May 17, 2023 at 11:21:50PM +0000, Thinh Nguyen wrote: > On Wed, May 17, 2023, Johan Hovold wrote: > > On Wed, May 17, 2023 at 03:21:24AM +0000, Thinh Nguyen wrote: > > > On Wed, May 17, 2023, Krishna Kurapati PSSNV wrote: > > > > On 5/16/2023 8:32 PM, Krishna Kurapati PSSNV wrote: > > > > > On 5/16/2023 5:41 PM, Johan Hovold wrote: > > > > > > > > You should not make another copy of xhci_find_next_ext_cap(), but rather > > > > > > use it directly. > > > > > > > > > > > > We already have drivers outside of usb/host using this function so it > > > > > > should be fine to do the same for now: > > > > > > > > > > > > #include "../host/xhci-ext-caps.h" > > > > > > > This was the approach which we followed when we first introduced the > > > > > patch [1]. But Thinh suggested to duplicate code so that we can avoid > > > > > any dependency on xhci (which seems to be right). So since its just one > > > > > function, I duplicated it here. > > > > > > Would like to know your opinion here on how to proceed further. > > > > > Please keep them separated. The xhci-ext-caps.h is for xhci driver only. > > > It's not meant to be exposed to other drivers. Same with other *.h files > > > under drivers/usb/host. > > > > As I mentioned earlier, it is already used by the xdbc earlyprintk > > driver which lives outside of drivers/usb/host, even if such a debug > > driver could be considered a special case. > > > > If it turns out that there's no way to avoid mapping those registers > > from the qcom glue driver, then I think at least the register defines > > need to be provided in a global header rather than being copied > > verbatim. > > It would be good to properly define the global header with common > offset/interface that can be public for other drivers. Yes, either drivers outside of xhci should be allowed to access this registers and then the defines should go in a public header or we need to find another way for drivers to get their hands on the corresponding information. I agree that accessing the header from inside the host directory is not very nice, but it looked we already had drivers violating this. If this turns out to be at all needed, it should even be possible to share the implementation even if that means adding an explicit dependency on xhci for host mode. The current inline function really is just a hack. Johan
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, > +}; Indentation is off, as I believe Bjorn pointed out. > 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); > + } You need check for NULL dwc as we just discussed and skip the above check if core has not probed yet. 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? Johan
On 6/7/2023 5:07 PM, Johan Hovold wrote: > Hi Krishna, > > and sorry about the delay in following up on this. As usual, there are > just way too many threads and this one in particular requires a bit of > thought. > Hi Johan, Thanks for taking the time out and reviewing the patches again. > On Sat, May 20, 2023 at 11:18:52PM +0530, Krishna Kurapati PSSNV wrote: >> On 5/17/2023 10:07 PM, Johan Hovold wrote: > >>> I don't think we should be adding more of these layering violations. A >>> parent device driver has no business messing with the driver data for a >>> child device which may or may not even have probed yet. >>> >>> I added a FIXME elsewhere in the driver about fixing up the current >>> instances that have already snuck in (which in some sense is even worse >>> by accessing driver data of a grandchild device). >>> >>> We really need to try sort this mess out and how to properly handle the >>> interactions between these layers (e.g. glue, dwc3 core and xhci). This >>> will likely involve adding callbacks from the child to the parent, for >>> example, when the child is suspending. > >> I agree with you, but in this case I believe there is no other way we >> can find the number of ports present. Unless its a dt property which >> parent driver can access the child's of node and get the details. Like >> done in v4 [1]. But it would be adding redundant data into DT as pointed >> out by Rob and Krzysztof and so we removed these properties. > > So there at least two issues with this series: > > 1. accessing xhci registers from the dwc3 core > 2. accessing driver data of a child device > > 1. The first part about accessing xhci registers goes against the clear > separation between glue, core and xhci that Felipe tried to maintain. > > I'm not entirely against doing this from the core driver before > registering the xhci platform device as the registers are unmapped > afterwards. But if this is to be allowed, then the implementation should > be shared with xhci rather than copied verbatim. > > The alternative that avoids this issue entirely could indeed be to > simply count the number of PHYs described in DT as Rob initially > suggested. Why would that not work? > The reason why I didn't want to read the Phy's from DT is explained in [1]. I felt it makes the code unreadable and its very tricky to read the phy's properly, so we decided we would initialize phy's for all ports and if a phy is missing in DT, the corresponding member in dwc->usbX_generic_phy[] would be NULL and any phy op on it would be a NOP. Also as per Krzysztof suggestion on [2], we can add a compatible to read number of phy's / ports present. This avoids accessing xhci members atleast in driver core. But the layering violations would still be present. > 2. The driver is already accessing driver data of the child device so > arguably your series is not really making things much worse than they > already are. > > I've just sent a couple of fixes to address some of the symptoms of > this layering violation here: > > https://lore.kernel.org/lkml/20230607100540.31045-1-johan+linaro@kernel.org/ > As you pointed out offline to me that using xhci event notifiers I mentioned on [3], if it is not acceptable to use them as notifications to check whether we are in host mode, I believe the only way would be to use the patches you pushed in. > Getting this fixed properly is going to take a bit of work, and > depending on how your multiport wake up implementation is going to look > like, adding support for multiport controllers may not make this much > harder to address. > > So perhaps we can indeed merge support for multiport and then get back > to cleaning this up. So, you are referring to use the patches you pushed today as a partial way to cleanup layering violations and merge multiport on top of it ? If so, I am fine with it. I can rebase my changes on top of them. [1]: https://lore.kernel.org/all/4eb26a54-148b-942f-01c6-64e66541de8b@quicinc.com/ [2]: https://lore.kernel.org/all/ca729f62-672e-d3de-4069-e2205c97e7d8@linaro.org/ [3]: https://lore.kernel.org/all/37fd026e-ecb1-3584-19f3-f8c1e5a9d20a@quicinc.com/ Regards, Krishna,
On 6/7/2023 5:14 PM, Johan Hovold wrote: > On Fri, May 26, 2023 at 08:55:22PM +0530, Krishna Kurapati PSSNV wrote: >> On 5/26/2023 8:25 AM, Bjorn Andersson wrote: > >>> We need to fix the dwc3 glue design, so that the glue and the core can >>> cooperate - and we have a few other use cases where this is needed (e.g. >>> usb_role_switch propagation to the glue code). > >> Thanks for the comments on this patch. I had some suggestions come in >> from the team internally: >> >> 1. To use the notifier call available in drivers/usb/core/notify.c and >> make sure that host mode is enabled. That way we can access dwc or xhci >> without any issue. > > I don't think this is a good idea and instead the callbacks should be > dedicated for the xhci and dwc3 drivers. A struct with callbacks can be > passed down to the child devices, which call back into the drivers of > their parents for notifications and when they need services from them > (e.g. during suspend or on role changes). > Hi Johan, While I agree with you that these notifications are to be used during role switch or suspend/resume, there is no restriction on using them for checking whether we are in host mode or not. IMO, it would be cleaner as we won't be dereferencing dwc driver data at all to check if we are in host mode or not. Regards, Krishna, >> 2. For this particular case where we are trying to get info on number of >> ports present (dwc->num_usb2_ports), we can add compatible data for >> sc8280-mp and provide input to driver telling num ports is 4. > > That may also work as a way to avoid parsing the xhci registers, but I'm > still not sure why simply counting the PHYs in DT would not work. > > Johan
On Thu, Jun 08, 2023 at 01:21:02AM +0530, Krishna Kurapati PSSNV wrote: > On 6/7/2023 5:07 PM, Johan Hovold wrote: > > So there at least two issues with this series: > > > > 1. accessing xhci registers from the dwc3 core > > 2. accessing driver data of a child device > > > > 1. The first part about accessing xhci registers goes against the clear > > separation between glue, core and xhci that Felipe tried to maintain. > > > > I'm not entirely against doing this from the core driver before > > registering the xhci platform device as the registers are unmapped > > afterwards. But if this is to be allowed, then the implementation should > > be shared with xhci rather than copied verbatim. > > > > The alternative that avoids this issue entirely could indeed be to > > simply count the number of PHYs described in DT as Rob initially > > suggested. Why would that not work? > > > The reason why I didn't want to read the Phy's from DT is explained in > [1]. I felt it makes the code unreadable and its very tricky to read the > phy's properly, so we decided we would initialize phy's for all ports > and if a phy is missing in DT, the corresponding member in > dwc->usbX_generic_phy[] would be NULL and any phy op on it would be a NOP. That doesn't sound too convincing. Can't you just iterate over the PHYs described in DT and determine the maximum port number used for HS and SS? > Also as per Krzysztof suggestion on [2], we can add a compatible to read > number of phy's / ports present. This avoids accessing xhci members > atleast in driver core. But the layering violations would still be present. Yes, but if the information is already available in DT it's better to use it rather than re-encode it in the driver. > > 2. The driver is already accessing driver data of the child device so > > arguably your series is not really making things much worse than they > > already are. > > > > I've just sent a couple of fixes to address some of the symptoms of > > this layering violation here: > > > > https://lore.kernel.org/lkml/20230607100540.31045-1-johan+linaro@kernel.org/ > > > > As you pointed out offline to me that using xhci event notifiers I > mentioned on [3], if it is not acceptable to use them as notifications > to check whether we are in host mode, I believe the only way would be to > use the patches you pushed in. I still think we'll end up using callbacks from the xhci/core into the glue driver, but dedicated ones rather than using usb_register_notify(). The fixes I posted can work as a stop-gap solution until then. > > Getting this fixed properly is going to take a bit of work, and > > depending on how your multiport wake up implementation is going to look > > like, adding support for multiport controllers may not make this much > > harder to address. > > > > So perhaps we can indeed merge support for multiport and then get back > > to cleaning this up. > So, you are referring to use the patches you pushed today as a partial > way to cleanup layering violations and merge multiport on top of it ? If > so, I am fine with it. I can rebase my changes on top of them. Right. A bit depending on how your wakeup implementation looks like, it seems we can merge multiport support and then address the layering issues which are already present in the driver. > [1]: > https://lore.kernel.org/all/4eb26a54-148b-942f-01c6-64e66541de8b@quicinc.com/ > [2]: > https://lore.kernel.org/all/ca729f62-672e-d3de-4069-e2205c97e7d8@linaro.org/ > [3]: > https://lore.kernel.org/all/37fd026e-ecb1-3584-19f3-f8c1e5a9d20a@quicinc.com/ Johan
On Thu, Jun 08, 2023 at 01:25:25AM +0530, Krishna Kurapati PSSNV wrote: > On 6/7/2023 5:14 PM, Johan Hovold wrote: > > On Fri, May 26, 2023 at 08:55:22PM +0530, Krishna Kurapati PSSNV wrote: > >> On 5/26/2023 8:25 AM, Bjorn Andersson wrote: > > > >>> We need to fix the dwc3 glue design, so that the glue and the core can > >>> cooperate - and we have a few other use cases where this is needed (e.g. > >>> usb_role_switch propagation to the glue code). > > > >> Thanks for the comments on this patch. I had some suggestions come in > >> from the team internally: > >> > >> 1. To use the notifier call available in drivers/usb/core/notify.c and > >> make sure that host mode is enabled. That way we can access dwc or xhci > >> without any issue. > > > > I don't think this is a good idea and instead the callbacks should be > > dedicated for the xhci and dwc3 drivers. A struct with callbacks can be > > passed down to the child devices, which call back into the drivers of > > their parents for notifications and when they need services from them > > (e.g. during suspend or on role changes). > While I agree with you that these notifications are to be used during > role switch or suspend/resume, there is no restriction on using them for > checking whether we are in host mode or not. IMO, it would be cleaner as > we won't be dereferencing dwc driver data at all to check if we are in > host mode or not. I'm not sure I understand what you're saying here. Could you try to rephrase it? Johan
On 6/8/2023 3:12 PM, Johan Hovold wrote: > On Thu, Jun 08, 2023 at 01:21:02AM +0530, Krishna Kurapati PSSNV wrote: >> On 6/7/2023 5:07 PM, Johan Hovold wrote: > >>> So there at least two issues with this series: >>> >>> 1. accessing xhci registers from the dwc3 core >>> 2. accessing driver data of a child device >>> >>> 1. The first part about accessing xhci registers goes against the clear >>> separation between glue, core and xhci that Felipe tried to maintain. >>> >>> I'm not entirely against doing this from the core driver before >>> registering the xhci platform device as the registers are unmapped >>> afterwards. But if this is to be allowed, then the implementation should >>> be shared with xhci rather than copied verbatim. >>> >>> The alternative that avoids this issue entirely could indeed be to >>> simply count the number of PHYs described in DT as Rob initially >>> suggested. Why would that not work? >>> >> The reason why I didn't want to read the Phy's from DT is explained in >> [1]. I felt it makes the code unreadable and its very tricky to read the >> phy's properly, so we decided we would initialize phy's for all ports >> and if a phy is missing in DT, the corresponding member in >> dwc->usbX_generic_phy[] would be NULL and any phy op on it would be a NOP. > > That doesn't sound too convincing. Can't you just iterate over the PHYs > described in DT and determine the maximum port number used for HS and > SS? > >> Also as per Krzysztof suggestion on [2], we can add a compatible to read >> number of phy's / ports present. This avoids accessing xhci members >> atleast in driver core. But the layering violations would still be present. > > Yes, but if the information is already available in DT it's better to use > it rather than re-encode it in the driver. > Hi Johan, Are you suggesting that we just do something like num_ports = max( highest usb2 portnum, highest usb3 port num) If so, incase the usb2 phy of quad port controller is missing in DT, we would still read num_usb2_ports as 4 but the usb2_generic_phy[1] would be NULL and any phy ops would still be NOP. But we would be getting rid of reading the xhci registers compeltely in core driver. Thinh, Bjorn, can you also let us know your views on this. 1. Read: num_usb3_ports = highest usb3 port index in DT num_usb2_ports = max( highest usb2 port index, num_usb3_ports) 2. Read the same by parsing xhci registers as done in recent versions of this series. Regards, Krishna, >>> 2. The driver is already accessing driver data of the child device so >>> arguably your series is not really making things much worse than they >>> already are. >>> >>> I've just sent a couple of fixes to address some of the symptoms of >>> this layering violation here: >>> >>> https://lore.kernel.org/lkml/20230607100540.31045-1-johan+linaro@kernel.org/ >>> >> >> As you pointed out offline to me that using xhci event notifiers I >> mentioned on [3], if it is not acceptable to use them as notifications >> to check whether we are in host mode, I believe the only way would be to >> use the patches you pushed in. > > I still think we'll end up using callbacks from the xhci/core into the > glue driver, but dedicated ones rather than using usb_register_notify(). > > The fixes I posted can work as a stop-gap solution until then. > >>> Getting this fixed properly is going to take a bit of work, and >>> depending on how your multiport wake up implementation is going to look >>> like, adding support for multiport controllers may not make this much >>> harder to address. >>> >>> So perhaps we can indeed merge support for multiport and then get back >>> to cleaning this up. >> So, you are referring to use the patches you pushed today as a partial >> way to cleanup layering violations and merge multiport on top of it ? If >> so, I am fine with it. I can rebase my changes on top of them. > > Right. A bit depending on how your wakeup implementation looks like, it > seems we can merge multiport support and then address the layering > issues which are already present in the driver. > >> [1]: >> https://lore.kernel.org/all/4eb26a54-148b-942f-01c6-64e66541de8b@quicinc.com/ >> [2]: >> https://lore.kernel.org/all/ca729f62-672e-d3de-4069-e2205c97e7d8@linaro.org/ >> [3]: >> https://lore.kernel.org/all/37fd026e-ecb1-3584-19f3-f8c1e5a9d20a@quicinc.com/ > > Johan
On Thu, Jun 08, 2023, Krishna Kurapati PSSNV wrote: > > > On 6/8/2023 3:12 PM, Johan Hovold wrote: > > On Thu, Jun 08, 2023 at 01:21:02AM +0530, Krishna Kurapati PSSNV wrote: > > > On 6/7/2023 5:07 PM, Johan Hovold wrote: > > > > > > So there at least two issues with this series: > > > > > > > > 1. accessing xhci registers from the dwc3 core > > > > 2. accessing driver data of a child device > > > > > > > > 1. The first part about accessing xhci registers goes against the clear > > > > separation between glue, core and xhci that Felipe tried to maintain. > > > > > > > > I'm not entirely against doing this from the core driver before > > > > registering the xhci platform device as the registers are unmapped > > > > afterwards. But if this is to be allowed, then the implementation should > > > > be shared with xhci rather than copied verbatim. The core will just be looking at the HW capability registers and accessing the ports capability. Our programming guide also listed the host capability registers in its documentation. We're not driving the xhci controller here. We're initializing some of the core configs base on its capability. We're duplicating the logic here and not exactly doing it verbatim. Let's try not to share the whole xhci header where we should not have visibility over. Perhaps it makes sense in some other driver, but let's not do it here. > > > > > > > > The alternative that avoids this issue entirely could indeed be to > > > > simply count the number of PHYs described in DT as Rob initially > > > > suggested. Why would that not work? See below. > > > > > > > The reason why I didn't want to read the Phy's from DT is explained in > > > [1]. I felt it makes the code unreadable and its very tricky to read the > > > phy's properly, so we decided we would initialize phy's for all ports > > > and if a phy is missing in DT, the corresponding member in > > > dwc->usbX_generic_phy[] would be NULL and any phy op on it would be a NOP. > > > > That doesn't sound too convincing. Can't you just iterate over the PHYs > > described in DT and determine the maximum port number used for HS and > > SS? > > > Also as per Krzysztof suggestion on [2], we can add a compatible to read > > > number of phy's / ports present. This avoids accessing xhci members > > > atleast in driver core. But the layering violations would still be present. > > > > Yes, but if the information is already available in DT it's better to use > > it rather than re-encode it in the driver. > > Hi Johan, > > Are you suggesting that we just do something like > num_ports = max( highest usb2 portnum, highest usb3 port num) Why do we want to do this? This makes num_ports ambiguous. Let's not sacrifice clarity for some lines of code. > > If so, incase the usb2 phy of quad port controller is missing in DT, we > would still read num_usb2_ports as 4 but the usb2_generic_phy[1] would be > NULL and any phy ops would still be NOP. But we would be getting rid of > reading the xhci registers compeltely in core driver. > > Thinh, Bjorn, can you also let us know your views on this. > > 1. Read: > num_usb3_ports = highest usb3 port index in DT > num_usb2_ports = max( highest usb2 port index, num_usb3_ports) > > 2. Read the same by parsing xhci registers as done in recent versions of > this series. > DT is not reliable to get this info. As noted, the DT may skip some ports and still be fine. However, the driver doesn't know which port reflects which port config index without the exact port count. More importantly, the host controller that lives on the PCI bus will not use DT. This can be useful for some re-configurations if the controller is a PCI device and that goes through the dwc3 code path. Thanks, Thinh
On Thu, Jun 08, 2023 at 05:57:23PM +0000, Thinh Nguyen wrote: > On Thu, Jun 08, 2023, Krishna Kurapati PSSNV wrote: > > On 6/8/2023 3:12 PM, Johan Hovold wrote: > > > On Thu, Jun 08, 2023 at 01:21:02AM +0530, Krishna Kurapati PSSNV wrote: > > > > On 6/7/2023 5:07 PM, Johan Hovold wrote: > > > > > > > > So there at least two issues with this series: > > > > > > > > > > 1. accessing xhci registers from the dwc3 core > > > > > 2. accessing driver data of a child device > > > > > > > > > > 1. The first part about accessing xhci registers goes against the clear > > > > > separation between glue, core and xhci that Felipe tried to maintain. > > > > > > > > > > I'm not entirely against doing this from the core driver before > > > > > registering the xhci platform device as the registers are unmapped > > > > > afterwards. But if this is to be allowed, then the implementation should > > > > > be shared with xhci rather than copied verbatim. > > The core will just be looking at the HW capability registers and > accessing the ports capability. Our programming guide also listed the > host capability registers in its documentation. We're not driving the > xhci controller here. We're initializing some of the core configs base > on its capability. > > We're duplicating the logic here and not exactly doing it verbatim. > Let's try not to share the whole xhci header where we should not have > visibility over. Perhaps it makes sense in some other driver, but let's > not do it here. The patch series even copied the kernel doc verbatim. This is just not the way things are supposed to be done upstream. We share defines and implementations all the time, but we should not be making copies of them. > > > > > > > > > > The alternative that avoids this issue entirely could indeed be to > > > > > simply count the number of PHYs described in DT as Rob initially > > > > > suggested. Why would that not work? > > See below. > > > > > > > > > > The reason why I didn't want to read the Phy's from DT is explained in > > > > [1]. I felt it makes the code unreadable and its very tricky to read the > > > > phy's properly, so we decided we would initialize phy's for all ports > > > > and if a phy is missing in DT, the corresponding member in > > > > dwc->usbX_generic_phy[] would be NULL and any phy op on it would be a NOP. > > > > > > That doesn't sound too convincing. Can't you just iterate over the PHYs > > > described in DT and determine the maximum port number used for HS and > > > SS? > > > > Also as per Krzysztof suggestion on [2], we can add a compatible to read > > > > number of phy's / ports present. This avoids accessing xhci members > > > > atleast in driver core. But the layering violations would still be present. > > > > > > Yes, but if the information is already available in DT it's better to use > > > it rather than re-encode it in the driver. > > Are you suggesting that we just do something like > > num_ports = max( highest usb2 portnum, highest usb3 port num) > > Why do we want to do this? This makes num_ports ambiguous. Let's not > sacrifice clarity for some lines of code. This is not about lines of code, but avoiding the bad practice of copying code around and, to some degree, maintaining the separation between the glue, core, and xhci which Felipe (perhaps mistakingly) has fought for. If you just need to know how many PHYs you have in DT so that you can iterate over that internal array, you can just look at the max index in DT where the indexes are specified in the first place. Don't get hung up on the current variable names, those can be renamed to match the implementation. Call it max_ports or whatever. > > If so, incase the usb2 phy of quad port controller is missing in DT, we > > would still read num_usb2_ports as 4 but the usb2_generic_phy[1] would be > > NULL and any phy ops would still be NOP. But we would be getting rid of > > reading the xhci registers compeltely in core driver. > > > > Thinh, Bjorn, can you also let us know your views on this. > > > > 1. Read: > > num_usb3_ports = highest usb3 port index in DT > > num_usb2_ports = max( highest usb2 port index, num_usb3_ports) > > > > 2. Read the same by parsing xhci registers as done in recent versions of > > this series. > > DT is not reliable to get this info. As noted, the DT may skip some > ports and still be fine. However, the driver doesn't know which port > reflects which port config index without the exact port count. That's not correct. DT provides the port indexes already, for example: phy-names = "usb2-port0", "usb3-port0", "usb2-port1", "usb3-port1", "usb2-port2", "usb2-port3"; So if you just need this to iterate over the PHYs all the information needed is here. If you need to access ports which do not have a PHY described in DT, then this is not going to suffice, but I have not seen anyone claim that that is needed yet. > More importantly, the host controller that lives on the PCI bus will not > use DT. This can be useful for some re-configurations if the controller > is a PCI device and that goes through the dwc3 code path. Ok, this is a bit hand wavy, but if this ever turns out to be needed it can also be implemented then. Or just generalise the xhci implementation for parsing these registers and reuse that from the start. (As a bonus you'd shrink the kernel text size by getting rid of that iffy inline implementation.) Johan
On Fri, Jun 09, 2023, Johan Hovold wrote: > On Thu, Jun 08, 2023 at 05:57:23PM +0000, Thinh Nguyen wrote: > > On Thu, Jun 08, 2023, Krishna Kurapati PSSNV wrote: > > > On 6/8/2023 3:12 PM, Johan Hovold wrote: > > > > On Thu, Jun 08, 2023 at 01:21:02AM +0530, Krishna Kurapati PSSNV wrote: > > > > > On 6/7/2023 5:07 PM, Johan Hovold wrote: > > > > > > > > > > So there at least two issues with this series: > > > > > > > > > > > > 1. accessing xhci registers from the dwc3 core > > > > > > 2. accessing driver data of a child device > > > > > > > > > > > > 1. The first part about accessing xhci registers goes against the clear > > > > > > separation between glue, core and xhci that Felipe tried to maintain. > > > > > > > > > > > > I'm not entirely against doing this from the core driver before > > > > > > registering the xhci platform device as the registers are unmapped > > > > > > afterwards. But if this is to be allowed, then the implementation should > > > > > > be shared with xhci rather than copied verbatim. > > > > The core will just be looking at the HW capability registers and > > accessing the ports capability. Our programming guide also listed the > > host capability registers in its documentation. We're not driving the > > xhci controller here. We're initializing some of the core configs base > > on its capability. > > > > We're duplicating the logic here and not exactly doing it verbatim. > > Let's try not to share the whole xhci header where we should not have > > visibility over. Perhaps it makes sense in some other driver, but let's > > not do it here. > > The patch series even copied the kernel doc verbatim. This is just not > the way things are supposed to be done upstream. We share defines and > implementations all the time, but we should not be making copies of > them. We had some fixes to the kernel doc as it's incorrect description. Perhaps we can fully rewrite the kernel-doc if that what makes it better. We can share define implementations if they are meant to be shared. However, with the current way xhci header is implemented, it's not meant to be shared with dwc3. You agreed that we are violating this in some driver, but you're also insistent that we should not duplicate the logic to avoid this violation. Perhaps I'm not a maintainer here long enough to know some violation is better kept. If sharing the xhci header is what it takes to get this through, then fine. > > > > > > > > > > > > > The alternative that avoids this issue entirely could indeed be to > > > > > > simply count the number of PHYs described in DT as Rob initially > > > > > > suggested. Why would that not work? > > > > See below. > > > > > > > > > > > > > The reason why I didn't want to read the Phy's from DT is explained in > > > > > [1]. I felt it makes the code unreadable and its very tricky to read the > > > > > phy's properly, so we decided we would initialize phy's for all ports > > > > > and if a phy is missing in DT, the corresponding member in > > > > > dwc->usbX_generic_phy[] would be NULL and any phy op on it would be a NOP. > > > > > > > > That doesn't sound too convincing. Can't you just iterate over the PHYs > > > > described in DT and determine the maximum port number used for HS and > > > > SS? > > > > > Also as per Krzysztof suggestion on [2], we can add a compatible to read > > > > > number of phy's / ports present. This avoids accessing xhci members > > > > > atleast in driver core. But the layering violations would still be present. > > > > > > > > Yes, but if the information is already available in DT it's better to use > > > > it rather than re-encode it in the driver. > > > > Are you suggesting that we just do something like > > > num_ports = max( highest usb2 portnum, highest usb3 port num) > > > > Why do we want to do this? This makes num_ports ambiguous. Let's not > > sacrifice clarity for some lines of code. > > This is not about lines of code, but avoiding the bad practice of > copying code around and, to some degree, maintaining the separation > between the glue, core, and xhci which Felipe (perhaps mistakingly) has > fought for. We're talking about combining num_usb3_ports and num_usb2_ports here, what does that have to do with layer separation? > > If you just need to know how many PHYs you have in DT so that you can > iterate over that internal array, you can just look at the max index in > DT where the indexes are specified in the first place. > > Don't get hung up on the current variable names, those can be renamed to > match the implementation. Call it max_ports or whatever. It doesn't matter what variable name is given, it doesn't change the fact that this "num_ports" or "max_ports" obfuscated usb2 vs usb3 ports just for this specific implementation. So, don't do that. > > > > If so, incase the usb2 phy of quad port controller is missing in DT, we > > > would still read num_usb2_ports as 4 but the usb2_generic_phy[1] would be > > > NULL and any phy ops would still be NOP. But we would be getting rid of > > > reading the xhci registers compeltely in core driver. > > > > > > Thinh, Bjorn, can you also let us know your views on this. > > > > > > 1. Read: > > > num_usb3_ports = highest usb3 port index in DT > > > num_usb2_ports = max( highest usb2 port index, num_usb3_ports) > > > > > > 2. Read the same by parsing xhci registers as done in recent versions of > > > this series. > > > > DT is not reliable to get this info. As noted, the DT may skip some > > ports and still be fine. However, the driver doesn't know which port > > reflects which port config index without the exact port count. > > That's not correct. DT provides the port indexes already, for example: > > phy-names = "usb2-port0", "usb3-port0", > "usb2-port1", "usb3-port1", > "usb2-port2", > "usb2-port3"; > > So if you just need this to iterate over the PHYs all the information > needed is here. > > If you need to access ports which do not have a PHY described in DT, > then this is not going to suffice, but I have not seen anyone claim that > that is needed yet. Perhaps I misunderstand the conversation. However, there isn't a method that everyone's agree on yet regarding DT [*]. Perhaps this indicates it may not be the best approach. You can resume the conversation if you want to: [*] https://lore.kernel.org/linux-usb/9671cade-1820-22e1-9db9-5c9836414908@quicinc.com/#t > > > More importantly, the host controller that lives on the PCI bus will not > > use DT. This can be useful for some re-configurations if the controller > > is a PCI device and that goes through the dwc3 code path. > > Ok, this is a bit hand wavy, but if this ever turns out to be needed it > can also be implemented then. What does hand wavy mean? We have case where it's useful outside of this, and it would be useful for PCI device too: https://lore.kernel.org/linux-usb/20230517233218.rjfmvptrexgkpam3@synopsys.com/ > > Or just generalise the xhci implementation for parsing these registers > and reuse that from the start. (As a bonus you'd shrink the kernel text > size by getting rid of that iffy inline implementation.) > I don't like the iffy inline function either. We changed that here. To rework the xhci header and define its global header seems a bit excessive just for dwc3 to get the port capability. Regardless, as I've said, if we _must_, perhaps we can just import xhci-ext-caps.h instead of the whole xhci.h. BR, Thinh
On 6/9/2023 11:46 PM, Thinh Nguyen wrote: > On Fri, Jun 09, 2023, Johan Hovold wrote: >> On Thu, Jun 08, 2023 at 05:57:23PM +0000, Thinh Nguyen wrote: >>> On Thu, Jun 08, 2023, Krishna Kurapati PSSNV wrote: >>>> On 6/8/2023 3:12 PM, Johan Hovold wrote: >>>>> On Thu, Jun 08, 2023 at 01:21:02AM +0530, Krishna Kurapati PSSNV wrote: >>>>>> On 6/7/2023 5:07 PM, Johan Hovold wrote: >>>>> >>>>>>> So there at least two issues with this series: >>>>>>> >>>>>>> 1. accessing xhci registers from the dwc3 core >>>>>>> 2. accessing driver data of a child device >>>>>>> >>>>>>> 1. The first part about accessing xhci registers goes against the clear >>>>>>> separation between glue, core and xhci that Felipe tried to maintain. >>>>>>> >>>>>>> I'm not entirely against doing this from the core driver before >>>>>>> registering the xhci platform device as the registers are unmapped >>>>>>> afterwards. But if this is to be allowed, then the implementation should >>>>>>> be shared with xhci rather than copied verbatim. >>> >>> The core will just be looking at the HW capability registers and >>> accessing the ports capability. Our programming guide also listed the >>> host capability registers in its documentation. We're not driving the >>> xhci controller here. We're initializing some of the core configs base >>> on its capability. >>> >>> We're duplicating the logic here and not exactly doing it verbatim. >>> Let's try not to share the whole xhci header where we should not have >>> visibility over. Perhaps it makes sense in some other driver, but let's >>> not do it here. >> >> The patch series even copied the kernel doc verbatim. This is just not >> the way things are supposed to be done upstream. We share defines and >> implementations all the time, but we should not be making copies of >> them. > > We had some fixes to the kernel doc as it's incorrect description. > Perhaps we can fully rewrite the kernel-doc if that what makes it > better. We can share define implementations if they are meant to be > shared. However, with the current way xhci header is implemented, it's > not meant to be shared with dwc3. You agreed that we are violating this > in some driver, but you're also insistent that we should not duplicate > the logic to avoid this violation. Perhaps I'm not a maintainer here > long enough to know some violation is better kept. If sharing the xhci > header is what it takes to get this through, then fine. > >> >>>>>>> >>>>>>> The alternative that avoids this issue entirely could indeed be to >>>>>>> simply count the number of PHYs described in DT as Rob initially >>>>>>> suggested. Why would that not work? >>> >>> See below. >>> >>>>>>> >>>>>> The reason why I didn't want to read the Phy's from DT is explained in >>>>>> [1]. I felt it makes the code unreadable and its very tricky to read the >>>>>> phy's properly, so we decided we would initialize phy's for all ports >>>>>> and if a phy is missing in DT, the corresponding member in >>>>>> dwc->usbX_generic_phy[] would be NULL and any phy op on it would be a NOP. >>>>> >>>>> That doesn't sound too convincing. Can't you just iterate over the PHYs >>>>> described in DT and determine the maximum port number used for HS and >>>>> SS? >>>>>> Also as per Krzysztof suggestion on [2], we can add a compatible to read >>>>>> number of phy's / ports present. This avoids accessing xhci members >>>>>> atleast in driver core. But the layering violations would still be present. >>>>> >>>>> Yes, but if the information is already available in DT it's better to use >>>>> it rather than re-encode it in the driver. >> >>>> Are you suggesting that we just do something like >>>> num_ports = max( highest usb2 portnum, highest usb3 port num) >>> >>> Why do we want to do this? This makes num_ports ambiguous. Let's not >>> sacrifice clarity for some lines of code. >> >> This is not about lines of code, but avoiding the bad practice of >> copying code around and, to some degree, maintaining the separation >> between the glue, core, and xhci which Felipe (perhaps mistakingly) has >> fought for. > > We're talking about combining num_usb3_ports and num_usb2_ports here, > what does that have to do with layer separation? > >> >> If you just need to know how many PHYs you have in DT so that you can >> iterate over that internal array, you can just look at the max index in >> DT where the indexes are specified in the first place. >> >> Don't get hung up on the current variable names, those can be renamed to >> match the implementation. Call it max_ports or whatever. > > It doesn't matter what variable name is given, it doesn't change the > fact that this "num_ports" or "max_ports" obfuscated usb2 vs usb3 ports > just for this specific implementation. So, don't do that. > >> >>>> If so, incase the usb2 phy of quad port controller is missing in DT, we >>>> would still read num_usb2_ports as 4 but the usb2_generic_phy[1] would be >>>> NULL and any phy ops would still be NOP. But we would be getting rid of >>>> reading the xhci registers compeltely in core driver. >>>> >>>> Thinh, Bjorn, can you also let us know your views on this. >>>> >>>> 1. Read: >>>> num_usb3_ports = highest usb3 port index in DT >>>> num_usb2_ports = max( highest usb2 port index, num_usb3_ports) >>>> >>>> 2. Read the same by parsing xhci registers as done in recent versions of >>>> this series. >>> >>> DT is not reliable to get this info. As noted, the DT may skip some >>> ports and still be fine. However, the driver doesn't know which port >>> reflects which port config index without the exact port count. >> >> That's not correct. DT provides the port indexes already, for example: >> >> phy-names = "usb2-port0", "usb3-port0", >> "usb2-port1", "usb3-port1", >> "usb2-port2", >> "usb2-port3"; >> >> So if you just need this to iterate over the PHYs all the information >> needed is here. >> >> If you need to access ports which do not have a PHY described in DT, >> then this is not going to suffice, but I have not seen anyone claim that >> that is needed yet. > > Perhaps I misunderstand the conversation. However, there isn't a method > that everyone's agree on yet regarding DT [*]. Perhaps this indicates it > may not be the best approach. You can resume the conversation if you > want to: > > [*] https://lore.kernel.org/linux-usb/9671cade-1820-22e1-9db9-5c9836414908@quicinc.com/#t > >> >>> More importantly, the host controller that lives on the PCI bus will not >>> use DT. This can be useful for some re-configurations if the controller >>> is a PCI device and that goes through the dwc3 code path. >> >> Ok, this is a bit hand wavy, but if this ever turns out to be needed it >> can also be implemented then. > > What does hand wavy mean? We have case where it's useful outside of > this, and it would be useful for PCI device too: > > https://lore.kernel.org/linux-usb/20230517233218.rjfmvptrexgkpam3@synopsys.com/ > >> >> Or just generalise the xhci implementation for parsing these registers >> and reuse that from the start. (As a bonus you'd shrink the kernel text >> size by getting rid of that iffy inline implementation.) >> > > I don't like the iffy inline function either. We changed that here. To > rework the xhci header and define its global header seems a bit > excessive just for dwc3 to get the port capability. Regardless, as I've > said, if we _must_, perhaps we can just import xhci-ext-caps.h instead > of the whole xhci.h. Hi Thinh, Johan, How about we add compatible data indicating the number of usb2/usb3 ports. That way we needn't parse the DT or read xhci registers atleast as a temporary solution to unblock other patches. Once this series is merged, we can get back to fixing the port count calculation. Does it seem fine ? Regards, Krishna,
On Thu, Jun 15, 2023, Krishna Kurapati PSSNV wrote: > > > On 6/9/2023 11:46 PM, Thinh Nguyen wrote: > > On Fri, Jun 09, 2023, Johan Hovold wrote: > > > On Thu, Jun 08, 2023 at 05:57:23PM +0000, Thinh Nguyen wrote: > > > > On Thu, Jun 08, 2023, Krishna Kurapati PSSNV wrote: > > > > > On 6/8/2023 3:12 PM, Johan Hovold wrote: > > > > > > On Thu, Jun 08, 2023 at 01:21:02AM +0530, Krishna Kurapati PSSNV wrote: > > > > > > > On 6/7/2023 5:07 PM, Johan Hovold wrote: > > > > > > > > > > > > > > So there at least two issues with this series: > > > > > > > > > > > > > > > > 1. accessing xhci registers from the dwc3 core > > > > > > > > 2. accessing driver data of a child device > > > > > > > > > > > > > > > > 1. The first part about accessing xhci registers goes against the clear > > > > > > > > separation between glue, core and xhci that Felipe tried to maintain. > > > > > > > > > > > > > > > > I'm not entirely against doing this from the core driver before > > > > > > > > registering the xhci platform device as the registers are unmapped > > > > > > > > afterwards. But if this is to be allowed, then the implementation should > > > > > > > > be shared with xhci rather than copied verbatim. > > > > > > > > The core will just be looking at the HW capability registers and > > > > accessing the ports capability. Our programming guide also listed the > > > > host capability registers in its documentation. We're not driving the > > > > xhci controller here. We're initializing some of the core configs base > > > > on its capability. > > > > > > > > We're duplicating the logic here and not exactly doing it verbatim. > > > > Let's try not to share the whole xhci header where we should not have > > > > visibility over. Perhaps it makes sense in some other driver, but let's > > > > not do it here. > > > > > > The patch series even copied the kernel doc verbatim. This is just not > > > the way things are supposed to be done upstream. We share defines and > > > implementations all the time, but we should not be making copies of > > > them. > > > > We had some fixes to the kernel doc as it's incorrect description. > > Perhaps we can fully rewrite the kernel-doc if that what makes it > > better. We can share define implementations if they are meant to be > > shared. However, with the current way xhci header is implemented, it's > > not meant to be shared with dwc3. You agreed that we are violating this > > in some driver, but you're also insistent that we should not duplicate > > the logic to avoid this violation. Perhaps I'm not a maintainer here > > long enough to know some violation is better kept. If sharing the xhci > > header is what it takes to get this through, then fine. > > > > > > > > > > > > > > > > > > > > > The alternative that avoids this issue entirely could indeed be to > > > > > > > > simply count the number of PHYs described in DT as Rob initially > > > > > > > > suggested. Why would that not work? > > > > > > > > See below. > > > > > > > > > > > > > > > > > > > The reason why I didn't want to read the Phy's from DT is explained in > > > > > > > [1]. I felt it makes the code unreadable and its very tricky to read the > > > > > > > phy's properly, so we decided we would initialize phy's for all ports > > > > > > > and if a phy is missing in DT, the corresponding member in > > > > > > > dwc->usbX_generic_phy[] would be NULL and any phy op on it would be a NOP. > > > > > > > > > > > > That doesn't sound too convincing. Can't you just iterate over the PHYs > > > > > > described in DT and determine the maximum port number used for HS and > > > > > > SS? > > > > > > > Also as per Krzysztof suggestion on [2], we can add a compatible to read > > > > > > > number of phy's / ports present. This avoids accessing xhci members > > > > > > > atleast in driver core. But the layering violations would still be present. > > > > > > > > > > > > Yes, but if the information is already available in DT it's better to use > > > > > > it rather than re-encode it in the driver. > > > > > > > > Are you suggesting that we just do something like > > > > > num_ports = max( highest usb2 portnum, highest usb3 port num) > > > > > > > > Why do we want to do this? This makes num_ports ambiguous. Let's not > > > > sacrifice clarity for some lines of code. > > > > > > This is not about lines of code, but avoiding the bad practice of > > > copying code around and, to some degree, maintaining the separation > > > between the glue, core, and xhci which Felipe (perhaps mistakingly) has > > > fought for. > > > > We're talking about combining num_usb3_ports and num_usb2_ports here, > > what does that have to do with layer separation? > > > > > > > > If you just need to know how many PHYs you have in DT so that you can > > > iterate over that internal array, you can just look at the max index in > > > DT where the indexes are specified in the first place. > > > > > > Don't get hung up on the current variable names, those can be renamed to > > > match the implementation. Call it max_ports or whatever. > > > > It doesn't matter what variable name is given, it doesn't change the > > fact that this "num_ports" or "max_ports" obfuscated usb2 vs usb3 ports > > just for this specific implementation. So, don't do that. > > > > > > > > > > If so, incase the usb2 phy of quad port controller is missing in DT, we > > > > > would still read num_usb2_ports as 4 but the usb2_generic_phy[1] would be > > > > > NULL and any phy ops would still be NOP. But we would be getting rid of > > > > > reading the xhci registers compeltely in core driver. > > > > > > > > > > Thinh, Bjorn, can you also let us know your views on this. > > > > > > > > > > 1. Read: > > > > > num_usb3_ports = highest usb3 port index in DT > > > > > num_usb2_ports = max( highest usb2 port index, num_usb3_ports) > > > > > > > > > > 2. Read the same by parsing xhci registers as done in recent versions of > > > > > this series. > > > > > > > > DT is not reliable to get this info. As noted, the DT may skip some > > > > ports and still be fine. However, the driver doesn't know which port > > > > reflects which port config index without the exact port count. > > > > > > That's not correct. DT provides the port indexes already, for example: > > > > > > phy-names = "usb2-port0", "usb3-port0", > > > "usb2-port1", "usb3-port1", > > > "usb2-port2", > > > "usb2-port3"; > > > > > > So if you just need this to iterate over the PHYs all the information > > > needed is here. > > > > > > If you need to access ports which do not have a PHY described in DT, > > > then this is not going to suffice, but I have not seen anyone claim that > > > that is needed yet. > > > > Perhaps I misunderstand the conversation. However, there isn't a method > > that everyone's agree on yet regarding DT [*]. Perhaps this indicates it > > may not be the best approach. You can resume the conversation if you > > want to: > > > > [*] https://urldefense.com/v3/__https://lore.kernel.org/linux-usb/9671cade-1820-22e1-9db9-5c9836414908@quicinc.com/*t__;Iw!!A4F2R9G_pg!YNb76pwkiNunnVGWfpM33LmCTJQNL7zPRooIIygA5rsUzkPGglyrsj5SLCy2raqkqwtjizd5js2wJ_OAP1Pp0N6mN4myMg$ > > > > > > More importantly, the host controller that lives on the PCI bus will not > > > > use DT. This can be useful for some re-configurations if the controller > > > > is a PCI device and that goes through the dwc3 code path. > > > > > > Ok, this is a bit hand wavy, but if this ever turns out to be needed it > > > can also be implemented then. > > > > What does hand wavy mean? We have case where it's useful outside of > > this, and it would be useful for PCI device too: > > > > https://urldefense.com/v3/__https://lore.kernel.org/linux-usb/20230517233218.rjfmvptrexgkpam3@synopsys.com/__;!!A4F2R9G_pg!YNb76pwkiNunnVGWfpM33LmCTJQNL7zPRooIIygA5rsUzkPGglyrsj5SLCy2raqkqwtjizd5js2wJ_OAP1Pp0N4CJPF7cQ$ > > > > > > > > Or just generalise the xhci implementation for parsing these registers > > > and reuse that from the start. (As a bonus you'd shrink the kernel text > > > size by getting rid of that iffy inline implementation.) > > > > > > > I don't like the iffy inline function either. We changed that here. To > > rework the xhci header and define its global header seems a bit > > excessive just for dwc3 to get the port capability. Regardless, as I've > > said, if we _must_, perhaps we can just import xhci-ext-caps.h instead > > of the whole xhci.h. > > Hi Thinh, Johan, > > How about we add compatible data indicating the number of usb2/usb3 ports. > That way we needn't parse the DT or read xhci registers atleast as a > temporary solution to unblock other patches. Once this series is merged, we > can get back to fixing the port count calculation. Does it seem fine ? > Temporary solution should not involve DT as it's not easily reverted or changed. Just include xhci-ext-caps.h and use the inline function. I think Johan is fine with that. If not, he can provide more feedback. Thanks, Thinh
Hi Thinh, and sorry about the late reply. Was on holiday last week. On Fri, Jun 09, 2023 at 06:16:13PM +0000, Thinh Nguyen wrote: > On Fri, Jun 09, 2023, Johan Hovold wrote: > > On Thu, Jun 08, 2023 at 05:57:23PM +0000, Thinh Nguyen wrote: > > > On Thu, Jun 08, 2023, Krishna Kurapati PSSNV wrote: > > > > On 6/8/2023 3:12 PM, Johan Hovold wrote: > > > > > On Thu, Jun 08, 2023 at 01:21:02AM +0530, Krishna Kurapati PSSNV wrote: > > > > > > On 6/7/2023 5:07 PM, Johan Hovold wrote: > > > > > > > > > > > > So there at least two issues with this series: > > > > > > > > > > > > > > 1. accessing xhci registers from the dwc3 core > > > > > > > 2. accessing driver data of a child device > > > > > > > > > > > > > > 1. The first part about accessing xhci registers goes against the clear > > > > > > > separation between glue, core and xhci that Felipe tried to maintain. > > > > > > > > > > > > > > I'm not entirely against doing this from the core driver before > > > > > > > registering the xhci platform device as the registers are unmapped > > > > > > > afterwards. But if this is to be allowed, then the implementation should > > > > > > > be shared with xhci rather than copied verbatim. > > > > > > The core will just be looking at the HW capability registers and > > > accessing the ports capability. Our programming guide also listed the > > > host capability registers in its documentation. We're not driving the > > > xhci controller here. We're initializing some of the core configs base > > > on its capability. > > > > > > We're duplicating the logic here and not exactly doing it verbatim. > > > Let's try not to share the whole xhci header where we should not have > > > visibility over. Perhaps it makes sense in some other driver, but let's > > > not do it here. > > > > The patch series even copied the kernel doc verbatim. This is just not > > the way things are supposed to be done upstream. We share defines and > > implementations all the time, but we should not be making copies of > > them. > > We had some fixes to the kernel doc as it's incorrect description. > Perhaps we can fully rewrite the kernel-doc if that what makes it > better. No, this is an example of why you should not copy code (and docs) around, for example, so you don't have to fix the same bug (or typo) in multiple places. > We can share define implementations if they are meant to be > shared. However, with the current way xhci header is implemented, it's > not meant to be shared with dwc3. You agreed that we are violating this > in some driver, but you're also insistent that we should not duplicate > the logic to avoid this violation. Perhaps I'm not a maintainer here > long enough to know some violation is better kept. If sharing the xhci > header is what it takes to get this through, then fine. Just because no-one has used these outside of xhci today, doesn't mean that you should copy the defines once they are needed outside that driver. And in fact, they are used outside of the xhci driver proper already today, only that people took the lazy approach and shared the inline helper for that. > > > > > > > > > > > > > > The alternative that avoids this issue entirely could indeed be to > > > > > > > simply count the number of PHYs described in DT as Rob initially > > > > > > > suggested. Why would that not work? > > > > > > See below. > > > > > > > > > > > > > > > > The reason why I didn't want to read the Phy's from DT is explained in > > > > > > [1]. I felt it makes the code unreadable and its very tricky to read the > > > > > > phy's properly, so we decided we would initialize phy's for all ports > > > > > > and if a phy is missing in DT, the corresponding member in > > > > > > dwc->usbX_generic_phy[] would be NULL and any phy op on it would be a NOP. > > > > > > > > > > That doesn't sound too convincing. Can't you just iterate over the PHYs > > > > > described in DT and determine the maximum port number used for HS and > > > > > SS? > > > > > > Also as per Krzysztof suggestion on [2], we can add a compatible to read > > > > > > number of phy's / ports present. This avoids accessing xhci members > > > > > > atleast in driver core. But the layering violations would still be present. > > > > > > > > > > Yes, but if the information is already available in DT it's better to use > > > > > it rather than re-encode it in the driver. > > > > > > Are you suggesting that we just do something like > > > > num_ports = max( highest usb2 portnum, highest usb3 port num) > > > > > > Why do we want to do this? This makes num_ports ambiguous. Let's not > > > sacrifice clarity for some lines of code. > > > > This is not about lines of code, but avoiding the bad practice of > > copying code around and, to some degree, maintaining the separation > > between the glue, core, and xhci which Felipe (perhaps mistakingly) has > > fought for. > > We're talking about combining num_usb3_ports and num_usb2_ports here, > what does that have to do with layer separation? I mentioned this is as an alternative if you're are hell-bent on not reusing the xhci register defines and parsing code. > > If you just need to know how many PHYs you have in DT so that you can > > iterate over that internal array, you can just look at the max index in > > DT where the indexes are specified in the first place. > > > > Don't get hung up on the current variable names, those can be renamed to > > match the implementation. Call it max_ports or whatever. > > It doesn't matter what variable name is given, it doesn't change the > fact that this "num_ports" or "max_ports" obfuscated usb2 vs usb3 ports > just for this specific implementation. So, don't do that. Ok, then make sure you reuse the xhci implementation for that then. > > > > If so, incase the usb2 phy of quad port controller is missing in DT, we > > > > would still read num_usb2_ports as 4 but the usb2_generic_phy[1] would be > > > > NULL and any phy ops would still be NOP. But we would be getting rid of > > > > reading the xhci registers compeltely in core driver. > > > > > > > > Thinh, Bjorn, can you also let us know your views on this. > > > > > > > > 1. Read: > > > > num_usb3_ports = highest usb3 port index in DT > > > > num_usb2_ports = max( highest usb2 port index, num_usb3_ports) > > > > > > > > 2. Read the same by parsing xhci registers as done in recent versions of > > > > this series. > > > > > > DT is not reliable to get this info. As noted, the DT may skip some > > > ports and still be fine. However, the driver doesn't know which port > > > reflects which port config index without the exact port count. > > > > That's not correct. DT provides the port indexes already, for example: > > > > phy-names = "usb2-port0", "usb3-port0", > > "usb2-port1", "usb3-port1", > > "usb2-port2", > > "usb2-port3"; > > > > So if you just need this to iterate over the PHYs all the information > > needed is here. > > > > If you need to access ports which do not have a PHY described in DT, > > then this is not going to suffice, but I have not seen anyone claim that > > that is needed yet. > > Perhaps I misunderstand the conversation. However, there isn't a method > that everyone's agree on yet regarding DT [*]. Perhaps this indicates it > may not be the best approach. You can resume the conversation if you > want to: > > [*] https://lore.kernel.org/linux-usb/9671cade-1820-22e1-9db9-5c9836414908@quicinc.com/#t This was the approach suggested by Rob, the DT maintainer, in that very thread IIRC. > > > More importantly, the host controller that lives on the PCI bus will not > > > use DT. This can be useful for some re-configurations if the controller > > > is a PCI device and that goes through the dwc3 code path. > > > > Ok, this is a bit hand wavy, but if this ever turns out to be needed it > > can also be implemented then. > > What does hand wavy mean? We have case where it's useful outside of > this, and it would be useful for PCI device too: > > https://lore.kernel.org/linux-usb/20230517233218.rjfmvptrexgkpam3@synopsys.com/ Hand-wavy as in lacking detail which makes it hard to evaluate the argument. > > Or just generalise the xhci implementation for parsing these registers > > and reuse that from the start. (As a bonus you'd shrink the kernel text > > size by getting rid of that iffy inline implementation.) > > > > I don't like the iffy inline function either. We changed that here. To > rework the xhci header and define its global header seems a bit > excessive just for dwc3 to get the port capability. Regardless, as I've > said, if we _must_, perhaps we can just import xhci-ext-caps.h instead > of the whole xhci.h. No one said anything about importing the whole of xhci.h. Johan
On Thu, Jun 15, 2023 at 09:08:01PM +0000, Thinh Nguyen wrote: > On Thu, Jun 15, 2023, Krishna Kurapati PSSNV wrote: > > How about we add compatible data indicating the number of usb2/usb3 ports. > > That way we needn't parse the DT or read xhci registers atleast as a > > temporary solution to unblock other patches. Once this series is merged, we > > can get back to fixing the port count calculation. Does it seem fine ? > > > > Temporary solution should not involve DT as it's not easily reverted or > changed. Just include xhci-ext-caps.h and use the inline function. I > think Johan is fine with that. If not, he can provide more feedback. Yes, I already suggested that as a quick way forward since it is already used this way by the xhci debug driver. Johan
On 6/21/2023 1:08 PM, Johan Hovold wrote: > On Thu, Jun 15, 2023 at 09:08:01PM +0000, Thinh Nguyen wrote: >> On Thu, Jun 15, 2023, Krishna Kurapati PSSNV wrote: > >>> How about we add compatible data indicating the number of usb2/usb3 ports. >>> That way we needn't parse the DT or read xhci registers atleast as a >>> temporary solution to unblock other patches. Once this series is merged, we >>> can get back to fixing the port count calculation. Does it seem fine ? >>> >> >> Temporary solution should not involve DT as it's not easily reverted or >> changed. Just include xhci-ext-caps.h and use the inline function. I >> think Johan is fine with that. If not, he can provide more feedback. > > Yes, I already suggested that as a quick way forward since it is already > used this way by the xhci debug driver. > > Johan Hi Johan, Thinh, Pushed a v9 following the above suggestion. Thanks, Krishna,
On Wed, Jun 21, 2023, Johan Hovold wrote: > Hi Thinh, > > and sorry about the late reply. Was on holiday last week. > > On Fri, Jun 09, 2023 at 06:16:13PM +0000, Thinh Nguyen wrote: > > On Fri, Jun 09, 2023, Johan Hovold wrote: > > > On Thu, Jun 08, 2023 at 05:57:23PM +0000, Thinh Nguyen wrote: > > > > On Thu, Jun 08, 2023, Krishna Kurapati PSSNV wrote: > > > > > On 6/8/2023 3:12 PM, Johan Hovold wrote: > > > > > > On Thu, Jun 08, 2023 at 01:21:02AM +0530, Krishna Kurapati PSSNV wrote: > > > > > > > On 6/7/2023 5:07 PM, Johan Hovold wrote: > > > > > > > > > > > > > > So there at least two issues with this series: > > > > > > > > > > > > > > > > 1. accessing xhci registers from the dwc3 core > > > > > > > > 2. accessing driver data of a child device > > > > > > > > > > > > > > > > 1. The first part about accessing xhci registers goes against the clear > > > > > > > > separation between glue, core and xhci that Felipe tried to maintain. > > > > > > > > > > > > > > > > I'm not entirely against doing this from the core driver before > > > > > > > > registering the xhci platform device as the registers are unmapped > > > > > > > > afterwards. But if this is to be allowed, then the implementation should > > > > > > > > be shared with xhci rather than copied verbatim. > > > > > > > > The core will just be looking at the HW capability registers and > > > > accessing the ports capability. Our programming guide also listed the > > > > host capability registers in its documentation. We're not driving the > > > > xhci controller here. We're initializing some of the core configs base > > > > on its capability. > > > > > > > > We're duplicating the logic here and not exactly doing it verbatim. > > > > Let's try not to share the whole xhci header where we should not have > > > > visibility over. Perhaps it makes sense in some other driver, but let's > > > > not do it here. > > > > > > The patch series even copied the kernel doc verbatim. This is just not > > > the way things are supposed to be done upstream. We share defines and > > > implementations all the time, but we should not be making copies of > > > them. > > > > We had some fixes to the kernel doc as it's incorrect description. > > Perhaps we can fully rewrite the kernel-doc if that what makes it > > better. > > No, this is an example of why you should not copy code (and docs) > around, for example, so you don't have to fix the same bug (or typo) in > multiple places. > > > We can share define implementations if they are meant to be > > shared. However, with the current way xhci header is implemented, it's > > not meant to be shared with dwc3. You agreed that we are violating this > > in some driver, but you're also insistent that we should not duplicate > > the logic to avoid this violation. Perhaps I'm not a maintainer here > > long enough to know some violation is better kept. If sharing the xhci > > header is what it takes to get this through, then fine. > > Just because no-one has used these outside of xhci today, doesn't mean > that you should copy the defines once they are needed outside that > driver. > > And in fact, they are used outside of the xhci driver proper already > today, only that people took the lazy approach and shared the inline > helper for that. > > > > > > > > > > > > > > > > > The alternative that avoids this issue entirely could indeed be to > > > > > > > > simply count the number of PHYs described in DT as Rob initially > > > > > > > > suggested. Why would that not work? > > > > > > > > See below. > > > > > > > > > > > > > > > > > > > The reason why I didn't want to read the Phy's from DT is explained in > > > > > > > [1]. I felt it makes the code unreadable and its very tricky to read the > > > > > > > phy's properly, so we decided we would initialize phy's for all ports > > > > > > > and if a phy is missing in DT, the corresponding member in > > > > > > > dwc->usbX_generic_phy[] would be NULL and any phy op on it would be a NOP. > > > > > > > > > > > > That doesn't sound too convincing. Can't you just iterate over the PHYs > > > > > > described in DT and determine the maximum port number used for HS and > > > > > > SS? > > > > > > > Also as per Krzysztof suggestion on [2], we can add a compatible to read > > > > > > > number of phy's / ports present. This avoids accessing xhci members > > > > > > > atleast in driver core. But the layering violations would still be present. > > > > > > > > > > > > Yes, but if the information is already available in DT it's better to use > > > > > > it rather than re-encode it in the driver. > > > > > > > > Are you suggesting that we just do something like > > > > > num_ports = max( highest usb2 portnum, highest usb3 port num) > > > > > > > > Why do we want to do this? This makes num_ports ambiguous. Let's not > > > > sacrifice clarity for some lines of code. > > > > > > This is not about lines of code, but avoiding the bad practice of > > > copying code around and, to some degree, maintaining the separation > > > between the glue, core, and xhci which Felipe (perhaps mistakingly) has > > > fought for. > > > > We're talking about combining num_usb3_ports and num_usb2_ports here, > > what does that have to do with layer separation? > > I mentioned this is as an alternative if you're are hell-bent on not > reusing the xhci register defines and parsing code. > > > > If you just need to know how many PHYs you have in DT so that you can > > > iterate over that internal array, you can just look at the max index in > > > DT where the indexes are specified in the first place. > > > > > > Don't get hung up on the current variable names, those can be renamed to > > > match the implementation. Call it max_ports or whatever. > > > > It doesn't matter what variable name is given, it doesn't change the > > fact that this "num_ports" or "max_ports" obfuscated usb2 vs usb3 ports > > just for this specific implementation. So, don't do that. > > Ok, then make sure you reuse the xhci implementation for that then. > > > > > > If so, incase the usb2 phy of quad port controller is missing in DT, we > > > > > would still read num_usb2_ports as 4 but the usb2_generic_phy[1] would be > > > > > NULL and any phy ops would still be NOP. But we would be getting rid of > > > > > reading the xhci registers compeltely in core driver. > > > > > > > > > > Thinh, Bjorn, can you also let us know your views on this. > > > > > > > > > > 1. Read: > > > > > num_usb3_ports = highest usb3 port index in DT > > > > > num_usb2_ports = max( highest usb2 port index, num_usb3_ports) > > > > > > > > > > 2. Read the same by parsing xhci registers as done in recent versions of > > > > > this series. > > > > > > > > DT is not reliable to get this info. As noted, the DT may skip some > > > > ports and still be fine. However, the driver doesn't know which port > > > > reflects which port config index without the exact port count. > > > > > > That's not correct. DT provides the port indexes already, for example: > > > > > > phy-names = "usb2-port0", "usb3-port0", > > > "usb2-port1", "usb3-port1", > > > "usb2-port2", > > > "usb2-port3"; > > > > > > So if you just need this to iterate over the PHYs all the information > > > needed is here. > > > > > > If you need to access ports which do not have a PHY described in DT, > > > then this is not going to suffice, but I have not seen anyone claim that > > > that is needed yet. > > > > Perhaps I misunderstand the conversation. However, there isn't a method > > that everyone's agree on yet regarding DT [*]. Perhaps this indicates it > > may not be the best approach. You can resume the conversation if you > > want to: > > > > [*] https://urldefense.com/v3/__https://lore.kernel.org/linux-usb/9671cade-1820-22e1-9db9-5c9836414908@quicinc.com/*t__;Iw!!A4F2R9G_pg!btc_fkrnAm1eFG5c5V0bWMnPnq4yvFyza1nDRJE8mPvSSrYIyxkUxjAARU51cRsoo2n0eLwTGt_SYvnQOaQ$ > > This was the approach suggested by Rob, the DT maintainer, in that very > thread IIRC. > > > > > More importantly, the host controller that lives on the PCI bus will not > > > > use DT. This can be useful for some re-configurations if the controller > > > > is a PCI device and that goes through the dwc3 code path. > > > > > > Ok, this is a bit hand wavy, but if this ever turns out to be needed it > > > can also be implemented then. > > > > What does hand wavy mean? We have case where it's useful outside of > > this, and it would be useful for PCI device too: > > > > https://urldefense.com/v3/__https://lore.kernel.org/linux-usb/20230517233218.rjfmvptrexgkpam3@synopsys.com/__;!!A4F2R9G_pg!btc_fkrnAm1eFG5c5V0bWMnPnq4yvFyza1nDRJE8mPvSSrYIyxkUxjAARU51cRsoo2n0eLwTGt_SqPJ_Tn0$ > > Hand-wavy as in lacking detail which makes it hard to evaluate the > argument. > > > > Or just generalise the xhci implementation for parsing these registers > > > and reuse that from the start. (As a bonus you'd shrink the kernel text > > > size by getting rid of that iffy inline implementation.) > > > > > > > I don't like the iffy inline function either. We changed that here. To > > rework the xhci header and define its global header seems a bit > > excessive just for dwc3 to get the port capability. Regardless, as I've > > said, if we _must_, perhaps we can just import xhci-ext-caps.h instead > > of the whole xhci.h. > > No one said anything about importing the whole of xhci.h. > This was how it was originally done in the earlier version of the series. Perhaps that's why I was against it. It looks wrong for dwc3 to know about xhci_hcd structure. Thanks, Thinh
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
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 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 > >> > >> 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? > 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. Did you hear anything back regarding the above? Could it even be that the register offset it not correct for sc8280xp? Johan
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 >>>> >>>> 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? > >> 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. > > Did you hear anything back regarding the above? > > Could it even be that the register offset it not correct for sc8280xp? > Hi Johan, No. I rechecked the register offsets and they are proper. (same as what we are using in downstream). Adding Jack and Wesley to help with any suggestions here. Regards, Krishna,
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 > >>>> > >>>> 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? > > > >> 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. > > > > Did you hear anything back regarding the above? > > > > Could it even be that the register offset it not correct for sc8280xp? > No. I rechecked the register offsets and they are proper. (same as what > we are using in downstream). > > Adding Jack and Wesley to help with any suggestions here. Still no idea as to why this appears to be broken on sc8280xp and triggers an error on every suspend? Johan
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. Konrad
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. Johan
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
On 8/11/2023 10:18 PM, Konrad Dybcio wrote: > 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 > The description for that bit (BIT(6)) as per the databook is as follows: --- 6 SUSPENDUSB20 R_W Suspend USB2.0 HS/FS/LS PHY (SusPHY) When set, USB2.0 PHY enters Suspend mode if Suspend conditions are valid. For DRD/OTG configurations, it is recommended that this bit is set to 0 during coreConsultant configuration. If it is set to 1, then the application must clear this bit after power-on reset. Application needs to set it to 1 after the core initialization completes. For all other configurations, this bit can be set to 1 during core configuration. Note: ■ In host mode, on reset, this bit is set to 1. Software can override this bit after reset. ■ In device mode, before issuing any device endpoint command when operating in 2.0 speeds, disable this bit and enable it after the command completes. If you issue a command without disabling this bit when the device is in L2 state and if mac2_clk (utmi_clk/ulpi_clk) is gated off, the command will not get completed --- "L2" is the term we say when PHY is suspended, i.e., the main PLL is shut off. Internally, I was able to find out that there are several conditions where phy can fail to enter L2. The entry into L2 is controlled by the USB controller itself, but can be limited by toggling GUSB2PHY SUSPENDABLE bit. if that bit is 0 then controller won't place HSPHY into L2. For the failure to enter L2, there can be several situations, like there may be some pending line state change that happened on the bus. But Johan's error seems to be different as the register itself reads zero which I don't understand. Regards, Krishna,