| 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: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 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 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 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 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 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
On 5/26/2023 8:25 AM, Bjorn Andersson 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++) { >> >> 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). > Hi Bjorn, Johan, 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. 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. For the first idea, I tried it out as follows: diff --git a/drivers/usb/dwc3/dwc3-qcom.c b/drivers/usb/dwc3/dwc3-qcom.c index 959fc925ca7c..ce2f867d7c9a 100644 --- a/drivers/usb/dwc3/dwc3-qcom.c +++ b/drivers/usb/dwc3/dwc3-qcom.c @@ -91,6 +91,9 @@ struct dwc3_qcom { bool pm_suspended; struct icc_path *icc_path_ddr; struct icc_path *icc_path_apps; + + bool in_host_mode; + struct notifier_block xhci_nb; }; static inline void dwc3_qcom_setbits(void __iomem *base, u32 offset, u32 val) @@ -305,14 +308,6 @@ static void dwc3_qcom_interconnect_exit(struct dwc3_qcom *qcom) icc_put(qcom->icc_path_apps); } -/* Only usable in contexts where the role can not change. */ -static bool dwc3_qcom_is_host(struct dwc3_qcom *qcom) -{ - struct dwc3 *dwc = platform_get_drvdata(qcom->dwc3); - - return dwc->xhci; -} - static enum usb_device_speed dwc3_qcom_read_usb2_speed(struct dwc3_qcom *qcom) { struct dwc3 *dwc = platform_get_drvdata(qcom->dwc3); @@ -432,7 +427,7 @@ static int dwc3_qcom_suspend(struct dwc3_qcom *qcom, bool wakeup) * The role is stable during suspend as role switching is done from a * freezable workqueue. */ - if (dwc3_qcom_is_host(qcom) && wakeup) { + if (qcom->in_host_mode && wakeup) { qcom->usb2_speed = dwc3_qcom_read_usb2_speed(qcom); dwc3_qcom_enable_interrupts(qcom); } @@ -450,7 +445,7 @@ static int dwc3_qcom_resume(struct dwc3_qcom *qcom, bool wakeup) if (!qcom->is_suspended) return 0; - if (dwc3_qcom_is_host(qcom) && wakeup) + if (qcom->in_host_mode && wakeup) dwc3_qcom_disable_interrupts(qcom); for (i = 0; i < qcom->num_clocks; i++) { @@ -488,7 +483,7 @@ static irqreturn_t qcom_dwc3_resume_irq(int irq, void *data) * This is safe as role switching is done from a freezable workqueue * and the wakeup interrupts are disabled as part of resume. */ - if (dwc3_qcom_is_host(qcom)) + if (qcom->in_host_mode) pm_runtime_resume(&dwc->xhci->dev); return IRQ_HANDLED; @@ -785,6 +780,41 @@ dwc3_qcom_create_urs_usb_platdev(struct device *dev) return acpi_create_platform_device(adev, NULL); } +static int dwc3_xhci_event_notifier(struct notifier_block *nb, + unsigned long event, void *ptr) +{ + struct dwc3_qcom *qcom = container_of(nb, struct dwc3_qcom, xhci_nb); + struct usb_bus *ubus = ptr; + struct usb_hcd *hcd; + + if (event != USB_BUS_ADD && event != USB_BUS_REMOVE) + return NOTIFY_DONE; + + //TODO: Check whether event generated is for this qcom controller or not + + hcd = bus_to_hcd(ubus); + if (event == USB_BUS_ADD) { + /* + * Assuming both usb2 and usb3 roothubs wil + * be registered, wait for shared hcd to be + * registered to ensure that we are in host mode + * completely. + */ + if (!usb_hcd_is_primary_hcd(hcd)) + qcom->in_host_mode = true; + } else if (event == USB_BUS_REMOVE) { + /* + * While exiting host mode, primary hcd is + * removed at the end. Use it as indication + * that host stack has been removed successfully. + */ + if (usb_hcd_is_primary_hcd(hcd)) + qcom->in_host_mode = false; + } + + return 0; +} + static int dwc3_qcom_probe(struct platform_device *pdev) { struct device_node *np = pdev->dev.of_node; @@ -884,6 +914,9 @@ static int dwc3_qcom_probe(struct platform_device *pdev) if (ignore_pipe_clk) dwc3_qcom_select_utmi_clk(qcom); + qcom->xhci_nb.notifier_call = dwc3_xhci_event_notifier; + usb_register_notify(&qcom->xhci_nb); + if (np) ret = dwc3_qcom_of_register_core(pdev); else @@ -923,6 +956,7 @@ static int dwc3_qcom_probe(struct platform_device *pdev) interconnect_exit: dwc3_qcom_interconnect_exit(qcom); depopulate: + usb_unregister_notify(&qcom->xhci_nb); if (np) of_platform_depopulate(&pdev->dev); I tested the patch on sc7280 and see that wakeup from system suspend works fine: [ 3.807449] K: Before notify register [ 3.810774] K: After notify register [ 3.814006] K: calling dwc3_qcom_of_register_core [ 3.818467] dwc3 a600000.usb: Adding to iommu group 8 [ 3.840031] K: before plat adde [ 3.849151] K: before add hcd [ 3.852219] xhci-hcd xhci-hcd.12.auto: xHCI Host Controller [ 3.857956] K: usb_notify_add_bus: Before notify add bus event: 3 [ 3.866481] K: dwc3_xhci_event_notifier recevied event: 3 [ 3.874007] K: dwc3_xhci_event_notifier: its a bus add/remove event [ 3.880451] K: dwc3_xhci_event_notifier hcd added [ 3.885301] K: in host mode: 0 [ 3.888441] K: usb_notify_add_bus: After notify add bus [ 3.893815] xhci-hcd xhci-hcd.12.auto: new USB bus registered, assigned bus number 1 [ 3.903799] xhci-hcd xhci-hcd.12.auto: hcc params 0x0230fe65 hci version 0x110 quirks 0x0000000000010010 [ 3.913564] xhci-hcd xhci-hcd.12.auto: irq 214, io mem 0x0a600000 [ 3.919938] K: after add hcd [ 3.922913] K: before add shared hcd [ 3.926589] xhci-hcd xhci-hcd.12.auto: xHCI Host Controller [ 3.932327] K: usb_notify_add_bus: Before notify add bus event: 3 [ 3.940973] K: dwc3_xhci_event_notifier recevied event: 3 [ 3.948492] K: dwc3_xhci_event_notifier: its a bus add/remove event [ 3.954936] K: dwc3_xhci_event_notifier hcd added [ 3.959770] K: dwc3_xhci_event_notifier: Its shared hcd [ 3.965143] K: in host mode: 1 [ 3.968283] K: usb_notify_add_bus: After notify add bus [ 3.973675] xhci-hcd xhci-hcd.12.auto: new USB bus registered, assigned bus number 2 [ 3.981645] xhci-hcd xhci-hcd.12.auto: Host supports USB 3.0 SuperSpeed [ 3.988537] usb usb1: New USB device found, idVendor=1d6b, idProduct=0002, bcdDevice= 5.15 [ 3.997024] usb usb1: New USB device strings: Mfr=3, Product=2, SerialNumber=1 [ 4.004438] usb usb1: Product: xHCI Host Controller [ 4.009459] usb usb1: Manufacturer: Linux 5.15.90-25532-g92932f8e612f-dirty xhci-hcd [ 4.017420] usb usb1: SerialNumber: xhci-hcd.12.auto ..... [ 90.809188] Resume caused by IRQ 211, qcom_dwc3 DP_HS Attached the patch file as well as in mail. Let me know if this is a good enough way to fix the layering violations. Regards, Krishna, From 3a318ab38c4959b21df3cce6b933b6d0abe5eb4b Mon Sep 17 00:00:00 2001 From: Krishna Kurapati <quic_kriskura@quicinc.com> Date: Fri, 26 May 2023 15:03:07 +0530 Subject: [PATCH] usb: dwc3: qcom: Cleanup layering violations in dwc3 qcom Implement host notifier in qcom to remove layering violations Signed-off-by: Krishna Kurapati <quic_kriskura@quicinc.com> --- drivers/usb/dwc3/dwc3-qcom.c | 56 +++++++++++++++++++++++++++++------- 1 file changed, 45 insertions(+), 11 deletions(-) diff --git a/drivers/usb/dwc3/dwc3-qcom.c b/drivers/usb/dwc3/dwc3-qcom.c index 959fc925ca7c..ce2f867d7c9a 100644 --- a/drivers/usb/dwc3/dwc3-qcom.c +++ b/drivers/usb/dwc3/dwc3-qcom.c @@ -91,6 +91,9 @@ struct dwc3_qcom { bool pm_suspended; struct icc_path *icc_path_ddr; struct icc_path *icc_path_apps; + + bool in_host_mode; + struct notifier_block xhci_nb; }; static inline void dwc3_qcom_setbits(void __iomem *base, u32 offset, u32 val) @@ -305,14 +308,6 @@ static void dwc3_qcom_interconnect_exit(struct dwc3_qcom *qcom) icc_put(qcom->icc_path_apps); } -/* Only usable in contexts where the role can not change. */ -static bool dwc3_qcom_is_host(struct dwc3_qcom *qcom) -{ - struct dwc3 *dwc = platform_get_drvdata(qcom->dwc3); - - return dwc->xhci; -} - static enum usb_device_speed dwc3_qcom_read_usb2_speed(struct dwc3_qcom *qcom) { struct dwc3 *dwc = platform_get_drvdata(qcom->dwc3); @@ -432,7 +427,7 @@ static int dwc3_qcom_suspend(struct dwc3_qcom *qcom, bool wakeup) * The role is stable during suspend as role switching is done from a * freezable workqueue. */ - if (dwc3_qcom_is_host(qcom) && wakeup) { + if (qcom->in_host_mode && wakeup) { qcom->usb2_speed = dwc3_qcom_read_usb2_speed(qcom); dwc3_qcom_enable_interrupts(qcom); } @@ -450,7 +445,7 @@ static int dwc3_qcom_resume(struct dwc3_qcom *qcom, bool wakeup) if (!qcom->is_suspended) return 0; - if (dwc3_qcom_is_host(qcom) && wakeup) + if (qcom->in_host_mode && wakeup) dwc3_qcom_disable_interrupts(qcom); for (i = 0; i < qcom->num_clocks; i++) { @@ -488,7 +483,7 @@ static irqreturn_t qcom_dwc3_resume_irq(int irq, void *data) * This is safe as role switching is done from a freezable workqueue * and the wakeup interrupts are disabled as part of resume. */ - if (dwc3_qcom_is_host(qcom)) + if (qcom->in_host_mode) pm_runtime_resume(&dwc->xhci->dev); return IRQ_HANDLED; @@ -785,6 +780,41 @@ dwc3_qcom_create_urs_usb_platdev(struct device *dev) return acpi_create_platform_device(adev, NULL); } +static int dwc3_xhci_event_notifier(struct notifier_block *nb, + unsigned long event, void *ptr) +{ + struct dwc3_qcom *qcom = container_of(nb, struct dwc3_qcom, xhci_nb); + struct usb_bus *ubus = ptr; + struct usb_hcd *hcd; + + if (event != USB_BUS_ADD && event != USB_BUS_REMOVE) + return NOTIFY_DONE; + + //TODO: Check whether event generated is for this qcom controller or not + + hcd = bus_to_hcd(ubus); + if (event == USB_BUS_ADD) { + /* + * Assuming both usb2 and usb3 roothubs wil + * be registered, wait for shared hcd to be + * registered to ensure that we are in host mode + * completely. + */ + if (!usb_hcd_is_primary_hcd(hcd)) + qcom->in_host_mode = true; + } else if (event == USB_BUS_REMOVE) { + /* + * While exiting host mode, primary hcd is + * removed at the end. Use it as indication + * that host stack has been removed successfully. + */ + if (usb_hcd_is_primary_hcd(hcd)) + qcom->in_host_mode = false; + } + + return 0; +} + static int dwc3_qcom_probe(struct platform_device *pdev) { struct device_node *np = pdev->dev.of_node; @@ -884,6 +914,9 @@ static int dwc3_qcom_probe(struct platform_device *pdev) if (ignore_pipe_clk) dwc3_qcom_select_utmi_clk(qcom); + qcom->xhci_nb.notifier_call = dwc3_xhci_event_notifier; + usb_register_notify(&qcom->xhci_nb); + if (np) ret = dwc3_qcom_of_register_core(pdev); else @@ -923,6 +956,7 @@ static int dwc3_qcom_probe(struct platform_device *pdev) interconnect_exit: dwc3_qcom_interconnect_exit(qcom); depopulate: + usb_unregister_notify(&qcom->xhci_nb); if (np) of_platform_depopulate(&pdev->dev); else
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 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
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
Currently the DWC3 driver supports only single port controller which requires at most two PHYs ie HS and SS PHYs. There are SoCs that has DWC3 controller with multiple ports that can operate in host mode. Some of the port supports both SS+HS and other port supports only HS mode. This change primarily refactors the Phy logic in core driver to allow multiport support with Generic Phy's. Chananges have been tested on QCOM SoC SA8295P which has 4 ports (2 are HS+SS capable and 2 are HS only capable). Changes in v8: Reorganised code in patch-5 Fixed nitpicks in code according to comments received on v7 Fixed indentation in DT patches Added drive strength for pinctrl nodes in SA8295 DT Changes in v7: Added power event irq's for Multiport controller. Udpated commit text for patch-9 (adding DT changes for enabling first port of multiport controller on sa8540-ride). Fixed check-patch warnings for driver code. Fixed DT binding errors for changes in snps,dwc3.yaml Reabsed code on top of usb-next Changes in v6: Updated comments in code after. Updated variables names appropriately as per review comments. Updated commit text in patch-2 and added additional info as per review comments. The patch header in v5 doesn't have "PATHCH v5" notation present. Corrected it in this version. Changes in v5: Added DT support for first port of Teritiary USB controller on SA8540-Ride Added support for reading port info from XHCI Extended Params registers. Changes in RFC v4: Added DT support for SA8295p. Changes in RFC v3: Incase any PHY init fails, then clear/exit the PHYs that are already initialized. Changes in RFC v2: Changed dwc3_count_phys to return the number of PHY Phandles in the node. This will be used now in dwc3_extract_num_phys to increment num_usb2_phy and num_usb3_phy. Added new parameter "ss_idx" in dwc3_core_get_phy_ny_node and changed its structure such that the first half is for HS-PHY and second half is for SS-PHY. In dwc3_core_get_phy, for multiport controller, only if SS-PHY phandle is present, pass proper SS_IDX else pass -1. Link to v7: https://lore.kernel.org/all/20230501143445.3851-1-quic_kriskura@quicinc.com/ Link to v6: https://lore.kernel.org/all/20230405125759.4201-1-quic_kriskura@quicinc.com/ Link to v5: https://lore.kernel.org/all/20230310163420.7582-1-quic_kriskura@quicinc.com/ Link to RFC v4: https://lore.kernel.org/all/20230115114146.12628-1-quic_kriskura@quicinc.com/ Link to RFC v3: https://lore.kernel.org/all/1654709787-23686-1-git-send-email-quic_harshq@quicinc.com/#r Link to RFC v2: https://lore.kernel.org/all/1653560029-6937-1-git-send-email-quic_harshq@quicinc.com/#r Test results: Bus 3/4 represent multiport controller having 4 HS ports and 2 SS ports. / # dmesg |grep hub [ 0.029029] usbcore: registered new interface driver hub [ 1.372812] hub 1-0:1.0: USB hub found [ 1.389142] hub 1-0:1.0: 1 port detected [ 1.414721] hub 2-0:1.0: USB hub found [ 1.427669] hub 2-0:1.0: 1 port detected [ 2.931465] hub 3-0:1.0: USB hub found [ 2.935340] hub 3-0:1.0: 4 ports detected [ 2.948721] hub 4-0:1.0: USB hub found [ 2.952604] hub 4-0:1.0: 2 ports detected / # / # lsusb Bus 003 Device 001: ID 1d6b:0002 Bus 001 Device 001: ID 1d6b:0002 Bus 003 Device 005: ID 0b0e:0300 Bus 003 Device 002: ID 046d:c077 Bus 004 Device 001: ID 1d6b:0003 Bus 002 Device 001: ID 1d6b:0003 Bus 003 Device 004: ID 03f0:0024 Bus 003 Device 003: ID 046d:c016 Krishna Kurapati (9): dt-bindings: usb: qcom,dwc3: Add bindings for SC8280 Multiport dt-bindings: usb: Add bindings for multiport properties on DWC3 controller usb: dwc3: core: Access XHCI address space temporarily to read port info usb: dwc3: core: Skip setting event buffers for host only controllers usb: dwc3: core: Refactor PHY logic to support Multiport Controller usb: dwc3: qcom: Add multiport controller support for qcom wrapper arm64: dts: qcom: sc8280xp: Add multiport controller node for SC8280 arm64: dts: qcom: sa8295p: Enable tertiary controller and its 4 USB ports arm64: dts: qcom: sa8540-ride: Enable first port of tertiary usb controller .../devicetree/bindings/usb/qcom,dwc3.yaml | 22 + .../devicetree/bindings/usb/snps,dwc3.yaml | 13 +- arch/arm64/boot/dts/qcom/sa8295p-adp.dts | 52 +++ arch/arm64/boot/dts/qcom/sa8540p-ride.dts | 22 + arch/arm64/boot/dts/qcom/sc8280xp.dtsi | 66 +++ drivers/usb/dwc3/core.c | 389 +++++++++++++++--- drivers/usb/dwc3/core.h | 28 +- drivers/usb/dwc3/drd.c | 13 +- drivers/usb/dwc3/dwc3-qcom.c | 28 +- 9 files changed, 543 insertions(+), 90 deletions(-)