| Message ID | 20230115114146.12628-1-quic_kriskura@quicinc.com |
|---|---|
| Headers | show |
| Series | Add multiport support for DWC3 controllers | expand |
On Sun, Jan 15, 2023 at 05:11:46PM +0530, Krishna Kurapati wrote: > Add USB and DWC3 node for teritiary port of SC8280 along > with multiport IRQ's and phy's. > Very nice. Please make the subject prefix "arm64: dts: qcom: sc8280xp:", to match other changes in the sc8280xp.dtsi. > Signed-off-by: Krishna Kurapati <quic_kriskura@quicinc.com> > --- > arch/arm64/boot/dts/qcom/sa8295p-adp.dts | 49 +++++++++++++++++++ > arch/arm64/boot/dts/qcom/sc8280xp.dtsi | 60 ++++++++++++++++++++++++ > 2 files changed, 109 insertions(+) > > diff --git a/arch/arm64/boot/dts/qcom/sa8295p-adp.dts b/arch/arm64/boot/dts/qcom/sa8295p-adp.dts > index 84cb6f3eeb56..f9eb854c3444 100644 > --- a/arch/arm64/boot/dts/qcom/sa8295p-adp.dts > +++ b/arch/arm64/boot/dts/qcom/sa8295p-adp.dts > @@ -422,6 +422,20 @@ &usb_1_qmpphy { > status = "okay"; > }; > > +&usb_2 { > + status = "okay"; Please the status property last in the node. > + > + pinctrl-names = "default"; > + pinctrl-0 = <&usb2_en_state>, > + <&usb3_en_state>, > + <&usb4_en_state>, > + <&usb5_en_state>; > +}; > + > +&usb_2_dwc3 { > + dr_mode = "host"; > +}; > + > &usb_2_hsphy0 { > vdda-pll-supply = <&vreg_l5a>; > vdda18-supply = <&vreg_l7g>; > @@ -472,6 +486,41 @@ &xo_board_clk { > clock-frequency = <38400000>; > }; > > +/* PINCTRL */ No need to repeat this comment, its purpose is to indicate that nodes above are sorted alphabetically and pinctrl-related nodes are kept here at the end of the file. Please place your nodes below the existing /* PINCTRL */ comment below. Thanks, Bjorn > +&pm8450c_gpios { > + usb2_en_state: usb2-en-state { > + pins = "gpio9"; > + function = "normal"; > + output-high; > + power-source = <0>; > + }; > +}; > + > +&pm8450e_gpios { > + usb3_en_state: usb3-en-state { > + pins = "gpio5"; > + function = "normal"; > + output-high; > + power-source = <0>; > + }; > +}; > + > +&pm8450g_gpios { > + usb4_en_state: usb4-en-state { > + pins = "gpio5"; > + function = "normal"; > + output-high; > + power-source = <0>; > + }; > + > + usb5_en_state: usb5-en-state { > + pins = "gpio9"; > + function = "normal"; > + output-high; > + power-source = <0>; > + }; > +}; > + > /* PINCTRL */ > > &tlmm { > diff --git a/arch/arm64/boot/dts/qcom/sc8280xp.dtsi b/arch/arm64/boot/dts/qcom/sc8280xp.dtsi > index 109c9d2b684d..e9866ab5c6e2 100644 > --- a/arch/arm64/boot/dts/qcom/sc8280xp.dtsi > +++ b/arch/arm64/boot/dts/qcom/sc8280xp.dtsi > @@ -1969,6 +1969,66 @@ usb_1_dwc3: usb@a800000 { > }; > }; > > + usb_2: usb@a4f8800 { > + compatible = "qcom,sc8280xp-dwc3", "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>; > + > + interrupt-names = "dp_hs_phy_irq", "dm_hs_phy_irq", > + "ss_phy_irq"; > + > + power-domains = <&gcc USB30_MP_GDSC>; > + > + 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>; > + interconnect-names = "usb-ddr", "apps-usb"; > + > + required-opps = <&rpmhpd_opp_nom>; > + > + 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>; > + num-ports = <4>; > + num-ss-ports = <2>; > + phys = <&usb_2_hsphy0>, <&usb_2_qmpphy0>, > + <&usb_2_hsphy1>, <&usb_2_qmpphy1>, > + <&usb_2_hsphy2>, > + <&usb_2_hsphy3>; > + phy-names = "usb2-phy_port0", "usb3-phy_port0", > + "usb2-phy_port1", "usb3-phy_port1", > + "usb2-phy_port2", > + "usb2-phy_port3"; > + }; > + }; > + > pdc: interrupt-controller@b220000 { > compatible = "qcom,sc8280xp-pdc", "qcom,pdc"; > reg = <0 0x0b220000 0 0x30000>, <0 0x17c000f0 0 0x60>; > -- > 2.39.0 >
On 1/18/2023 11:58 PM, Bjorn Andersson wrote: > On Sun, Jan 15, 2023 at 05:11:46PM +0530, Krishna Kurapati wrote: >> Add USB and DWC3 node for teritiary port of SC8280 along >> with multiport IRQ's and phy's. >> > > Very nice. > > Please make the subject prefix "arm64: dts: qcom: sc8280xp:", to match > other changes in the sc8280xp.dtsi. >> Signed-off-by: Krishna Kurapati <quic_kriskura@quicinc.com> >> --- >> arch/arm64/boot/dts/qcom/sa8295p-adp.dts | 49 +++++++++++++++++++ >> arch/arm64/boot/dts/qcom/sc8280xp.dtsi | 60 ++++++++++++++++++++++++ >> 2 files changed, 109 insertions(+) >> >> diff --git a/arch/arm64/boot/dts/qcom/sa8295p-adp.dts b/arch/arm64/boot/dts/qcom/sa8295p-adp.dts >> index 84cb6f3eeb56..f9eb854c3444 100644 >> --- a/arch/arm64/boot/dts/qcom/sa8295p-adp.dts >> +++ b/arch/arm64/boot/dts/qcom/sa8295p-adp.dts >> @@ -422,6 +422,20 @@ &usb_1_qmpphy { >> status = "okay"; >> }; >> >> +&usb_2 { >> + status = "okay"; > > Please the status property last in the node. > >> + >> + pinctrl-names = "default"; >> + pinctrl-0 = <&usb2_en_state>, >> + <&usb3_en_state>, >> + <&usb4_en_state>, >> + <&usb5_en_state>; >> +}; >> + >> +&usb_2_dwc3 { >> + dr_mode = "host"; >> +}; >> + >> &usb_2_hsphy0 { >> vdda-pll-supply = <&vreg_l5a>; >> vdda18-supply = <&vreg_l7g>; >> @@ -472,6 +486,41 @@ &xo_board_clk { >> clock-frequency = <38400000>; >> }; >> >> +/* PINCTRL */ > > No need to repeat this comment, its purpose is to indicate that nodes > above are sorted alphabetically and pinctrl-related nodes are kept here > at the end of the file. Please place your nodes below the existing /* > PINCTRL */ comment below. > > Thanks, > Bjorn > >> +&pm8450c_gpios { >> + usb2_en_state: usb2-en-state { >> + pins = "gpio9"; >> + function = "normal"; >> + output-high; >> + power-source = <0>; >> + }; >> +}; >> + >> +&pm8450e_gpios { >> + usb3_en_state: usb3-en-state { >> + pins = "gpio5"; >> + function = "normal"; >> + output-high; >> + power-source = <0>; >> + }; >> +}; >> + >> +&pm8450g_gpios { >> + usb4_en_state: usb4-en-state { >> + pins = "gpio5"; >> + function = "normal"; >> + output-high; >> + power-source = <0>; >> + }; >> + >> + usb5_en_state: usb5-en-state { >> + pins = "gpio9"; >> + function = "normal"; >> + output-high; >> + power-source = <0>; >> + }; >> +}; >> + >> /* PINCTRL */ >> >> &tlmm { >> diff --git a/arch/arm64/boot/dts/qcom/sc8280xp.dtsi b/arch/arm64/boot/dts/qcom/sc8280xp.dtsi >> index 109c9d2b684d..e9866ab5c6e2 100644 >> --- a/arch/arm64/boot/dts/qcom/sc8280xp.dtsi >> +++ b/arch/arm64/boot/dts/qcom/sc8280xp.dtsi >> @@ -1969,6 +1969,66 @@ usb_1_dwc3: usb@a800000 { >> }; >> }; >> >> + usb_2: usb@a4f8800 { >> + compatible = "qcom,sc8280xp-dwc3", "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>; >> + >> + interrupt-names = "dp_hs_phy_irq", "dm_hs_phy_irq", >> + "ss_phy_irq"; >> + >> + power-domains = <&gcc USB30_MP_GDSC>; >> + >> + 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>; >> + interconnect-names = "usb-ddr", "apps-usb"; >> + >> + required-opps = <&rpmhpd_opp_nom>; >> + >> + 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>; >> + num-ports = <4>; >> + num-ss-ports = <2>; >> + phys = <&usb_2_hsphy0>, <&usb_2_qmpphy0>, >> + <&usb_2_hsphy1>, <&usb_2_qmpphy1>, >> + <&usb_2_hsphy2>, >> + <&usb_2_hsphy3>; >> + phy-names = "usb2-phy_port0", "usb3-phy_port0", >> + "usb2-phy_port1", "usb3-phy_port1", >> + "usb2-phy_port2", >> + "usb2-phy_port3"; >> + }; >> + }; >> + >> pdc: interrupt-controller@b220000 { >> compatible = "qcom,sc8280xp-pdc", "qcom,pdc"; >> reg = <0 0x0b220000 0 0x30000>, <0 0x17c000f0 0 0x60>; >> -- >> 2.39.0 >> Hi Bjorn, Thanks for the review. Will make sure to incorporate these changes in the next version. Regards, Krishna,
Hi, On Sun, Jan 15, 2023, Krishna Kurapati wrote: > Currently the DWC3 driver supports only single port controller > which requires at most one HS and one SS PHY. Add note here that multi-port is for host mode for clarity. > > But the DWC3 USB controller can be connected to multiple ports and > each port can have their own PHYs. Each port of the multiport > controller can either be HS+SS capable or HS only capable > Proper quantification of them is required to modify GUSB2PHYCFG > and GUSB3PIPECTL registers appropriately. > > Add support for detecting, obtaining and configuring phy's supported > by a multiport controller and limit the max number of ports > supported to 4. > > Signed-off-by: Harsh Agarwal <quic_harshq@quicinc.com> > Signed-off-by: Krishna Kurapati <quic_kriskura@quicinc.com> > --- > drivers/usb/dwc3/core.c | 304 +++++++++++++++++++++++++++++----------- > drivers/usb/dwc3/core.h | 15 +- > drivers/usb/dwc3/drd.c | 14 +- > 3 files changed, 244 insertions(+), 89 deletions(-) > > diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c > index 476b63618511..7e0a9a598dfd 100644 > --- a/drivers/usb/dwc3/core.c > +++ b/drivers/usb/dwc3/core.c > @@ -120,7 +120,7 @@ static void __dwc3_set_mode(struct work_struct *work) > { > struct dwc3 *dwc = work_to_dwc(work); > unsigned long flags; > - int ret; > + int ret, i; Can we declare variables in separate lines here and other places. > u32 reg; > u32 desired_dr_role; > > @@ -200,8 +200,10 @@ static void __dwc3_set_mode(struct work_struct *work) > } else { > if (dwc->usb2_phy) > otg_set_vbus(dwc->usb2_phy->otg, true); > - phy_set_mode(dwc->usb2_generic_phy, PHY_MODE_USB_HOST); > - phy_set_mode(dwc->usb3_generic_phy, PHY_MODE_USB_HOST); > + for (i = 0; i < dwc->num_ports; i++) { > + 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 (dwc->dis_split_quirk) { > reg = dwc3_readl(dwc->regs, DWC3_GUCTL3); > reg |= DWC3_GUCTL3_SPLITDISABLE; > @@ -216,8 +218,8 @@ static void __dwc3_set_mode(struct work_struct *work) > > if (dwc->usb2_phy) > otg_set_vbus(dwc->usb2_phy->otg, false); > - phy_set_mode(dwc->usb2_generic_phy, PHY_MODE_USB_DEVICE); > - phy_set_mode(dwc->usb3_generic_phy, PHY_MODE_USB_DEVICE); > + phy_set_mode(dwc->usb2_generic_phy[0], PHY_MODE_USB_DEVICE); > + phy_set_mode(dwc->usb3_generic_phy[0], PHY_MODE_USB_DEVICE); > > ret = dwc3_gadget_init(dwc); > if (ret) > @@ -659,22 +661,14 @@ static int dwc3_core_ulpi_init(struct dwc3 *dwc) > return ret; > } > > -/** > - * dwc3_phy_setup - Configure USB PHY Interface of DWC3 Core > - * @dwc: Pointer to our controller context structure > - * > - * Returns 0 on success. The USB PHY interfaces are configured but not > - * initialized. The PHY interfaces and the PHYs get initialized together with > - * the core in dwc3_core_init. > - */ > -static int dwc3_phy_setup(struct dwc3 *dwc) > +static int dwc3_ss_phy_setup(struct dwc3 *dwc, int index) > { > unsigned int hw_mode; > u32 reg; > > hw_mode = DWC3_GHWPARAMS0_MODE(dwc->hwparams.hwparams0); > > - reg = dwc3_readl(dwc->regs, DWC3_GUSB3PIPECTL(0)); > + reg = dwc3_readl(dwc->regs, DWC3_GUSB3PIPECTL(index)); > > /* > * Make sure UX_EXIT_PX is cleared as that causes issues with some > @@ -729,9 +723,19 @@ static int dwc3_phy_setup(struct dwc3 *dwc) > if (dwc->dis_del_phy_power_chg_quirk) > reg &= ~DWC3_GUSB3PIPECTL_DEPOCHANGE; > > - dwc3_writel(dwc->regs, DWC3_GUSB3PIPECTL(0), reg); > + dwc3_writel(dwc->regs, DWC3_GUSB3PIPECTL(index), reg); > > - reg = dwc3_readl(dwc->regs, DWC3_GUSB2PHYCFG(0)); > + return 0; > +} > + > +static int dwc3_hs_phy_setup(struct dwc3 *dwc, int index) > +{ > + unsigned int hw_mode; > + u32 reg; > + > + hw_mode = DWC3_GHWPARAMS0_MODE(dwc->hwparams.hwparams0); > + > + reg = dwc3_readl(dwc->regs, DWC3_GUSB2PHYCFG(index)); > > /* Select the HS PHY interface */ > switch (DWC3_GHWPARAMS3_HSPHY_IFC(dwc->hwparams.hwparams3)) { > @@ -743,7 +747,7 @@ static int dwc3_phy_setup(struct dwc3 *dwc) > } else if (dwc->hsphy_interface && > !strncmp(dwc->hsphy_interface, "ulpi", 4)) { > reg |= DWC3_GUSB2PHYCFG_ULPI_UTMI; > - dwc3_writel(dwc->regs, DWC3_GUSB2PHYCFG(0), reg); > + dwc3_writel(dwc->regs, DWC3_GUSB2PHYCFG(index), reg); > } else { > /* Relying on default value. */ > if (!(reg & DWC3_GUSB2PHYCFG_ULPI_UTMI)) > @@ -800,7 +804,35 @@ static int dwc3_phy_setup(struct dwc3 *dwc) > if (dwc->dis_u2_freeclk_exists_quirk || dwc->gfladj_refclk_lpm_sel) > reg &= ~DWC3_GUSB2PHYCFG_U2_FREECLK_EXISTS; > > - dwc3_writel(dwc->regs, DWC3_GUSB2PHYCFG(0), reg); > + dwc3_writel(dwc->regs, DWC3_GUSB2PHYCFG(index), reg); > + > + return 0; > +} > + > +/** > + * dwc3_phy_setup - Configure USB PHY Interface of DWC3 Core > + * @dwc: Pointer to our controller context structure > + * > + * Returns 0 on success. The USB PHY interfaces are configured but not > + * initialized. The PHY interfaces and the PHYs get initialized together with > + * the core in dwc3_core_init. > + */ > +static int dwc3_phy_setup(struct dwc3 *dwc) > +{ > + int i; > + int ret; > + > + for (i = 0; i < dwc->num_ss_ports; i++) { > + ret = dwc3_ss_phy_setup(dwc, i); > + if (ret) > + return ret; > + } > + > + for (i = 0; i < dwc->num_ports; i++) { > + ret = dwc3_hs_phy_setup(dwc, i); > + if (ret) > + return ret; > + } > > return 0; > } > @@ -839,17 +871,25 @@ static void dwc3_clk_disable(struct dwc3 *dwc) > > static void dwc3_core_exit(struct dwc3 *dwc) > { > + int i; > + > dwc3_event_buffers_cleanup(dwc); > > usb_phy_set_suspend(dwc->usb2_phy, 1); > usb_phy_set_suspend(dwc->usb3_phy, 1); > - phy_power_off(dwc->usb2_generic_phy); > - phy_power_off(dwc->usb3_generic_phy); > + > + for (i = 0; i < dwc->num_ports; i++) { > + phy_power_off(dwc->usb2_generic_phy[i]); > + phy_power_off(dwc->usb3_generic_phy[i]); > + } > > usb_phy_shutdown(dwc->usb2_phy); > usb_phy_shutdown(dwc->usb3_phy); > - phy_exit(dwc->usb2_generic_phy); > - phy_exit(dwc->usb3_generic_phy); > + > + for (i = 0; i < dwc->num_ports; i++) { > + phy_exit(dwc->usb2_generic_phy[i]); > + phy_exit(dwc->usb3_generic_phy[i]); > + } > > dwc3_clk_disable(dwc); > reset_control_assert(dwc->reset); > @@ -1085,6 +1125,7 @@ static int dwc3_core_init(struct dwc3 *dwc) > unsigned int hw_mode; > u32 reg; > int ret; > + int i, j; > > hw_mode = DWC3_GHWPARAMS0_MODE(dwc->hwparams.hwparams0); > > @@ -1119,14 +1160,27 @@ static int dwc3_core_init(struct dwc3 *dwc) > > usb_phy_init(dwc->usb2_phy); > usb_phy_init(dwc->usb3_phy); > - ret = phy_init(dwc->usb2_generic_phy); > - if (ret < 0) > - goto err0a; > > - ret = phy_init(dwc->usb3_generic_phy); > - if (ret < 0) { > - phy_exit(dwc->usb2_generic_phy); > - goto err0a; > + for (i = 0; i < dwc->num_ports; i++) { > + ret = phy_init(dwc->usb2_generic_phy[i]); > + if (ret < 0) { > + /* clean up prior initialized HS PHYs */ > + for (j = 0; j < i; j++) > + phy_exit(dwc->usb2_generic_phy[j]); > + goto err0a; > + } > + } > + > + for (i = 0; i < dwc->num_ports; i++) { > + ret = phy_init(dwc->usb3_generic_phy[i]); > + if (ret < 0) { > + /* clean up prior initialized SS PHYs */ > + for (j = 0; j < i; j++) > + phy_exit(dwc->usb3_generic_phy[j]); > + for (j = 0; j < dwc->num_ports; j++) > + phy_exit(dwc->usb2_generic_phy[j]); > + goto err0a; > + } > } > > ret = dwc3_core_soft_reset(dwc); > @@ -1136,15 +1190,19 @@ static int dwc3_core_init(struct dwc3 *dwc) > if (hw_mode == DWC3_GHWPARAMS0_MODE_DRD && > !DWC3_VER_IS_WITHIN(DWC3, ANY, 194A)) { > if (!dwc->dis_u3_susphy_quirk) { > - reg = dwc3_readl(dwc->regs, DWC3_GUSB3PIPECTL(0)); > - reg |= DWC3_GUSB3PIPECTL_SUSPHY; > - dwc3_writel(dwc->regs, DWC3_GUSB3PIPECTL(0), reg); > + for (i = 0; i < dwc->num_ss_ports; i++) { > + reg = dwc3_readl(dwc->regs, DWC3_GUSB3PIPECTL(i)); > + reg |= DWC3_GUSB3PIPECTL_SUSPHY; > + dwc3_writel(dwc->regs, DWC3_GUSB3PIPECTL(i), reg); > + } > } > > if (!dwc->dis_u2_susphy_quirk) { > - reg = dwc3_readl(dwc->regs, DWC3_GUSB2PHYCFG(0)); > - reg |= DWC3_GUSB2PHYCFG_SUSPHY; > - dwc3_writel(dwc->regs, DWC3_GUSB2PHYCFG(0), reg); > + for (i = 0; i < dwc->num_ports; i++) { > + reg = dwc3_readl(dwc->regs, DWC3_GUSB2PHYCFG(i)); > + reg |= DWC3_GUSB2PHYCFG_SUSPHY; > + dwc3_writel(dwc->regs, DWC3_GUSB2PHYCFG(i), reg); > + } > } > } > > @@ -1168,13 +1226,25 @@ static int dwc3_core_init(struct dwc3 *dwc) > > usb_phy_set_suspend(dwc->usb2_phy, 0); > usb_phy_set_suspend(dwc->usb3_phy, 0); > - ret = phy_power_on(dwc->usb2_generic_phy); > - if (ret < 0) > - goto err2; > > - ret = phy_power_on(dwc->usb3_generic_phy); > - if (ret < 0) > - goto err3; > + > + for (i = 0; i < dwc->num_ports; i++) { > + ret = phy_power_on(dwc->usb2_generic_phy[i]); > + if (ret < 0) { > + for (j = 0; j < i; j++) > + phy_power_off(dwc->usb2_generic_phy[j]); > + goto err2; > + } > + } > + > + for (i = 0; i < dwc->num_ports; i++) { > + ret = phy_power_on(dwc->usb3_generic_phy[i]); > + if (ret < 0) { > + for (j = 0; j < i; j++) > + phy_power_off(dwc->usb3_generic_phy[j]); > + goto err3; > + } > + } > > ret = dwc3_event_buffers_setup(dwc); > if (ret) { > @@ -1297,10 +1367,12 @@ static int dwc3_core_init(struct dwc3 *dwc) > return 0; > > err4: > - phy_power_off(dwc->usb3_generic_phy); > + for (i = 0; i < dwc->num_ports; i++) > + phy_power_off(dwc->usb3_generic_phy[i]); > > err3: > - phy_power_off(dwc->usb2_generic_phy); > + for (i = 0; i < dwc->num_ports; i++) > + phy_power_off(dwc->usb2_generic_phy[i]); > > err2: > usb_phy_set_suspend(dwc->usb2_phy, 1); > @@ -1309,8 +1381,11 @@ static int dwc3_core_init(struct dwc3 *dwc) > err1: > usb_phy_shutdown(dwc->usb2_phy); > usb_phy_shutdown(dwc->usb3_phy); > - phy_exit(dwc->usb2_generic_phy); > - phy_exit(dwc->usb3_generic_phy); > + > + for (i = 0; i < dwc->num_ports; i++) { > + phy_exit(dwc->usb2_generic_phy[i]); > + phy_exit(dwc->usb3_generic_phy[i]); > + } > > err0a: > dwc3_ulpi_exit(dwc); > @@ -1319,6 +1394,38 @@ static int dwc3_core_init(struct dwc3 *dwc) > return ret; > } > > +static int dwc3_get_multiport_phys(struct dwc3 *dwc) > +{ > + int ret; > + struct device *dev = dwc->dev; > + int i; > + char phy_name[15]; > + > + for (i = 0; i < dwc->num_ports; i++) { > + sprintf(phy_name, "usb2-phy_port%d", i); > + dwc->usb2_generic_phy[i] = devm_phy_get(dev, phy_name); > + if (IS_ERR(dwc->usb2_generic_phy[i])) { > + ret = PTR_ERR(dwc->usb2_generic_phy[i]); > + if (ret == -ENOSYS || ret == -ENODEV) > + dwc->usb2_generic_phy[i] = NULL; > + else > + return dev_err_probe(dev, ret, "usb2 phy: %s not configured\n", phy_name); > + } > + > + sprintf(phy_name, "usb3-phy_port%d", i); > + dwc->usb3_generic_phy[i] = devm_phy_get(dev, phy_name); > + if (IS_ERR(dwc->usb3_generic_phy[i])) { > + ret = PTR_ERR(dwc->usb3_generic_phy[i]); > + if (ret == -ENOSYS || ret == -ENODEV) > + dwc->usb3_generic_phy[i] = NULL; > + else > + return dev_err_probe(dev, ret, "usb3 phy: %s not configured\n", phy_name); > + } > + } > + > + return 0; > +} > + > static int dwc3_core_get_phy(struct dwc3 *dwc) > { > struct device *dev = dwc->dev; > @@ -1349,31 +1456,37 @@ static int dwc3_core_get_phy(struct dwc3 *dwc) > return dev_err_probe(dev, ret, "no usb3 phy configured\n"); > } > > - dwc->usb2_generic_phy = devm_phy_get(dev, "usb2-phy"); > - if (IS_ERR(dwc->usb2_generic_phy)) { > - ret = PTR_ERR(dwc->usb2_generic_phy); > + if (dwc->num_ports > 1) > + goto get_multiport_phys; Can we avoid goto and just return dwc3_get_multiport_phys(dwc) here? It's easier to read IMO. > + > + dwc->usb2_generic_phy[0] = devm_phy_get(dev, "usb2-phy"); > + if (IS_ERR(dwc->usb2_generic_phy[0])) { > + ret = PTR_ERR(dwc->usb2_generic_phy[0]); > if (ret == -ENOSYS || ret == -ENODEV) > - dwc->usb2_generic_phy = NULL; > + dwc->usb2_generic_phy[0] = NULL; > else > return dev_err_probe(dev, ret, "no usb2 phy configured\n"); > } > > - dwc->usb3_generic_phy = devm_phy_get(dev, "usb3-phy"); > - if (IS_ERR(dwc->usb3_generic_phy)) { > - ret = PTR_ERR(dwc->usb3_generic_phy); > + dwc->usb3_generic_phy[0] = devm_phy_get(dev, "usb3-phy"); > + if (IS_ERR(dwc->usb3_generic_phy[0])) { > + ret = PTR_ERR(dwc->usb3_generic_phy[0]); > if (ret == -ENOSYS || ret == -ENODEV) > - dwc->usb3_generic_phy = NULL; > + dwc->usb3_generic_phy[0] = NULL; > else > return dev_err_probe(dev, ret, "no usb3 phy configured\n"); > } > > return 0; > + > +get_multiport_phys: > + return dwc3_get_multiport_phys(dwc); > } > > static int dwc3_core_init_mode(struct dwc3 *dwc) > { > struct device *dev = dwc->dev; > - int ret; > + int ret, i; > > switch (dwc->dr_mode) { > case USB_DR_MODE_PERIPHERAL: > @@ -1381,8 +1494,8 @@ static int dwc3_core_init_mode(struct dwc3 *dwc) > > if (dwc->usb2_phy) > otg_set_vbus(dwc->usb2_phy->otg, false); > - phy_set_mode(dwc->usb2_generic_phy, PHY_MODE_USB_DEVICE); > - phy_set_mode(dwc->usb3_generic_phy, PHY_MODE_USB_DEVICE); > + phy_set_mode(dwc->usb2_generic_phy[0], PHY_MODE_USB_DEVICE); > + phy_set_mode(dwc->usb3_generic_phy[0], PHY_MODE_USB_DEVICE); > > ret = dwc3_gadget_init(dwc); > if (ret) > @@ -1393,8 +1506,10 @@ static int dwc3_core_init_mode(struct dwc3 *dwc) > > if (dwc->usb2_phy) > otg_set_vbus(dwc->usb2_phy->otg, true); > - phy_set_mode(dwc->usb2_generic_phy, PHY_MODE_USB_HOST); > - phy_set_mode(dwc->usb3_generic_phy, PHY_MODE_USB_HOST); > + for (i = 0; i < dwc->num_ports; i++) { > + phy_set_mode(dwc->usb2_generic_phy[i], PHY_MODE_USB_HOST); > + phy_set_mode(dwc->usb3_generic_phy[i], PHY_MODE_USB_HOST); > + } > > ret = dwc3_host_init(dwc); > if (ret) > @@ -1575,6 +1690,21 @@ static void dwc3_get_properties(struct dwc3 *dwc) > dwc->dis_split_quirk = device_property_read_bool(dev, > "snps,dis-split-quirk"); > > + > + /* > + * If no mulitport properties are defined, default multi* > + * the port count to '1'. > + */ Can we initialize num_ports and num_ss_ports to 1 instead of setting it on error (similar to how we handle other properties). > + ret = device_property_read_u32(dev, "num-ports", > + &dwc->num_ports); > + if (ret) > + dwc->num_ports = 1; > + > + ret = device_property_read_u32(dev, "num-ss-ports", > + &dwc->num_ss_ports); > + if (ret) > + dwc->num_ss_ports = 1; > + > dwc->lpm_nyet_threshold = lpm_nyet_threshold; > dwc->tx_de_emphasis = tx_de_emphasis; > > @@ -1755,8 +1885,7 @@ static int dwc3_probe(struct platform_device *pdev) > struct device *dev = &pdev->dev; > struct resource *res, dwc_res; > struct dwc3 *dwc; > - > - int ret; > + int ret, i; > > void __iomem *regs; > > @@ -1933,17 +2062,24 @@ static int dwc3_probe(struct platform_device *pdev) > > err5: > dwc3_debugfs_exit(dwc); > + > dwc3_event_buffers_cleanup(dwc); > > usb_phy_set_suspend(dwc->usb2_phy, 1); > usb_phy_set_suspend(dwc->usb3_phy, 1); > - phy_power_off(dwc->usb2_generic_phy); > - phy_power_off(dwc->usb3_generic_phy); > + > + for (i = 0; i < dwc->num_ports; i++) { > + phy_power_off(dwc->usb2_generic_phy[i]); > + phy_power_off(dwc->usb3_generic_phy[i]); > + } > > usb_phy_shutdown(dwc->usb2_phy); > usb_phy_shutdown(dwc->usb3_phy); > - phy_exit(dwc->usb2_generic_phy); > - phy_exit(dwc->usb3_generic_phy); > + > + for (i = 0; i < dwc->num_ports; i++) { > + phy_exit(dwc->usb2_generic_phy[i]); > + phy_exit(dwc->usb3_generic_phy[i]); > + } > > dwc3_ulpi_exit(dwc); > > @@ -2025,6 +2161,7 @@ static int dwc3_core_init_for_resume(struct dwc3 *dwc) > > static int dwc3_suspend_common(struct dwc3 *dwc, pm_message_t msg) > { > + int i; > unsigned long flags; > u32 reg; > > @@ -2045,17 +2182,21 @@ static int dwc3_suspend_common(struct dwc3 *dwc, pm_message_t msg) > /* Let controller to suspend HSPHY before PHY driver suspends */ > if (dwc->dis_u2_susphy_quirk || > dwc->dis_enblslpm_quirk) { > - reg = dwc3_readl(dwc->regs, DWC3_GUSB2PHYCFG(0)); > - reg |= DWC3_GUSB2PHYCFG_ENBLSLPM | > - DWC3_GUSB2PHYCFG_SUSPHY; > - dwc3_writel(dwc->regs, DWC3_GUSB2PHYCFG(0), reg); > + for (i = 0; i < dwc->num_ports; i++) { > + reg = dwc3_readl(dwc->regs, DWC3_GUSB2PHYCFG(i)); > + reg |= DWC3_GUSB2PHYCFG_ENBLSLPM | > + DWC3_GUSB2PHYCFG_SUSPHY; > + dwc3_writel(dwc->regs, DWC3_GUSB2PHYCFG(i), reg); > + } > > /* Give some time for USB2 PHY to suspend */ > usleep_range(5000, 6000); > } > > - phy_pm_runtime_put_sync(dwc->usb2_generic_phy); > - phy_pm_runtime_put_sync(dwc->usb3_generic_phy); > + for (i = 0; i < dwc->num_ports; i++) { > + phy_pm_runtime_put_sync(dwc->usb2_generic_phy[i]); > + phy_pm_runtime_put_sync(dwc->usb3_generic_phy[i]); > + } > break; > case DWC3_GCTL_PRTCAP_OTG: > /* do nothing during runtime_suspend */ > @@ -2084,6 +2225,7 @@ static int dwc3_resume_common(struct dwc3 *dwc, pm_message_t msg) > { > unsigned long flags; > int ret; > + int i; > u32 reg; > > switch (dwc->current_dr_role) { > @@ -2104,17 +2246,21 @@ static int dwc3_resume_common(struct dwc3 *dwc, pm_message_t msg) > break; > } > /* Restore GUSB2PHYCFG bits that were modified in suspend */ > - reg = dwc3_readl(dwc->regs, DWC3_GUSB2PHYCFG(0)); > - if (dwc->dis_u2_susphy_quirk) > - reg &= ~DWC3_GUSB2PHYCFG_SUSPHY; > + for (i = 0; i < dwc->num_ports; i++) { > + reg = dwc3_readl(dwc->regs, DWC3_GUSB2PHYCFG(i)); > + if (dwc->dis_u2_susphy_quirk) > + reg &= ~DWC3_GUSB2PHYCFG_SUSPHY; > > - if (dwc->dis_enblslpm_quirk) > - reg &= ~DWC3_GUSB2PHYCFG_ENBLSLPM; > + if (dwc->dis_enblslpm_quirk) > + reg &= ~DWC3_GUSB2PHYCFG_ENBLSLPM; > > - dwc3_writel(dwc->regs, DWC3_GUSB2PHYCFG(0), reg); > + dwc3_writel(dwc->regs, DWC3_GUSB2PHYCFG(i), reg); > + } > > - phy_pm_runtime_get_sync(dwc->usb2_generic_phy); > - phy_pm_runtime_get_sync(dwc->usb3_generic_phy); > + for (i = 0; i < dwc->num_ports; i++) { > + phy_pm_runtime_get_sync(dwc->usb2_generic_phy[i]); > + phy_pm_runtime_get_sync(dwc->usb3_generic_phy[i]); > + } > break; > case DWC3_GCTL_PRTCAP_OTG: > /* nothing to do on runtime_resume */ > diff --git a/drivers/usb/dwc3/core.h b/drivers/usb/dwc3/core.h > index 8f9959ba9fd4..2f82eda9d44f 100644 > --- a/drivers/usb/dwc3/core.h > +++ b/drivers/usb/dwc3/core.h > @@ -35,6 +35,9 @@ > > #define DWC3_MSG_MAX 500 > > +/* Number of ports supported by a multiport controller */ > +#define MAX_PORTS_SUPPORTED 4 > + > /* Global constants */ > #define DWC3_PULL_UP_TIMEOUT 500 /* ms */ > #define DWC3_BOUNCE_SIZE 1024 /* size of a superspeed bulk */ > @@ -1023,8 +1026,10 @@ struct dwc3_scratchpad_array { > * @usb_psy: pointer to power supply interface. > * @usb2_phy: pointer to USB2 PHY > * @usb3_phy: pointer to USB3 PHY > - * @usb2_generic_phy: pointer to USB2 PHY > - * @usb3_generic_phy: pointer to USB3 PHY > + * @num_ports: Indicates number of usb ports supported by the controller. Note that this is the total number of usb ports including the SS capable ones. > + * @num_ss_ports: Indicates number of ss capable ports supported by controller > + * @usb2_generic_phy: pointer to array of USB2 PHY's > + * @usb3_generic_phy: pointer to array of USB3 PHY's > * @phys_ready: flag to indicate that PHYs are ready > * @ulpi: pointer to ulpi interface > * @ulpi_ready: flag to indicate that ULPI is initialized > @@ -1157,8 +1162,10 @@ struct dwc3 { > struct usb_phy *usb2_phy; > struct usb_phy *usb3_phy; > > - struct phy *usb2_generic_phy; > - struct phy *usb3_generic_phy; > + u32 num_ports; > + u32 num_ss_ports; > + struct phy *usb2_generic_phy[MAX_PORTS_SUPPORTED]; > + struct phy *usb3_generic_phy[MAX_PORTS_SUPPORTED]; > > bool phys_ready; > > diff --git a/drivers/usb/dwc3/drd.c b/drivers/usb/dwc3/drd.c > index 039bf241769a..ea86ff01433b 100644 > --- a/drivers/usb/dwc3/drd.c > +++ b/drivers/usb/dwc3/drd.c > @@ -327,7 +327,7 @@ static void dwc3_otg_device_exit(struct dwc3 *dwc) > > void dwc3_otg_update(struct dwc3 *dwc, bool ignore_idstatus) > { > - int ret; > + int ret, i; > u32 reg; > int id; > unsigned long flags; > @@ -386,9 +386,11 @@ void dwc3_otg_update(struct dwc3 *dwc, bool ignore_idstatus) > } else { > if (dwc->usb2_phy) > otg_set_vbus(dwc->usb2_phy->otg, true); > - if (dwc->usb2_generic_phy) > - phy_set_mode(dwc->usb2_generic_phy, > - PHY_MODE_USB_HOST); > + for (i = 0; i < dwc->num_ports; i++) { > + if (dwc->usb2_generic_phy[i]) > + phy_set_mode(dwc->usb2_generic_phy[i], > + PHY_MODE_USB_HOST); > + } > } > break; > case DWC3_OTG_ROLE_DEVICE: > @@ -400,8 +402,8 @@ void dwc3_otg_update(struct dwc3 *dwc, bool ignore_idstatus) > > if (dwc->usb2_phy) > otg_set_vbus(dwc->usb2_phy->otg, false); > - if (dwc->usb2_generic_phy) > - phy_set_mode(dwc->usb2_generic_phy, > + if (dwc->usb2_generic_phy[0]) > + phy_set_mode(dwc->usb2_generic_phy[0], > PHY_MODE_USB_DEVICE); > ret = dwc3_gadget_init(dwc); > if (ret) > -- > 2.39.0 > No major issue I see here. Just some minor nits. Once you feel it's ready, please drop the RFC tags on resubmission. Thanks, Thinh
On Sun, Jan 15, 2023, Krishna Kurapati wrote: > Multiport controllers being host-only capable do not have GEVNTADDR > HI/LO, SIZE, COUNT reigsters present. Accsesing them to setup event I think you should reword "present" to something else. They're still present, but those registers are to be set while operating in device mode. The rest looks fine. Thanks, Thinh > buffers during core_init can cause an SMMU Fault. Avoid event buffers > setup if the GHWPARAMS0 tells that the controller is host-only. > > Signed-off-by: Krishna Kurapati <quic_kriskura@quicinc.com> > --- > drivers/usb/dwc3/core.c | 23 +++++++++++++++-------- > 1 file changed, 15 insertions(+), 8 deletions(-) > > diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c > index 7e0a9a598dfd..f61ebddaecc0 100644 > --- a/drivers/usb/dwc3/core.c > +++ b/drivers/usb/dwc3/core.c > @@ -871,9 +871,12 @@ static void dwc3_clk_disable(struct dwc3 *dwc) > > static void dwc3_core_exit(struct dwc3 *dwc) > { > - int i; > + int i; > + unsigned int hw_mode; > > - dwc3_event_buffers_cleanup(dwc); > + hw_mode = DWC3_GHWPARAMS0_MODE(dwc->hwparams.hwparams0); > + if (hw_mode != DWC3_GHWPARAMS0_MODE_HOST) > + dwc3_event_buffers_cleanup(dwc); > > usb_phy_set_suspend(dwc->usb2_phy, 1); > usb_phy_set_suspend(dwc->usb3_phy, 1); > @@ -1246,10 +1249,12 @@ static int dwc3_core_init(struct dwc3 *dwc) > } > } > > - ret = dwc3_event_buffers_setup(dwc); > - if (ret) { > - dev_err(dwc->dev, "failed to setup event buffers\n"); > - goto err4; > + if (hw_mode != DWC3_GHWPARAMS0_MODE_HOST) { > + ret = dwc3_event_buffers_setup(dwc); > + if (ret) { > + dev_err(dwc->dev, "failed to setup event buffers\n"); > + goto err4; > + } > } > > /* > @@ -1886,7 +1891,7 @@ static int dwc3_probe(struct platform_device *pdev) > struct resource *res, dwc_res; > struct dwc3 *dwc; > int ret, i; > - > + unsigned int hw_mode; > void __iomem *regs; > > dwc = devm_kzalloc(dev, sizeof(*dwc), GFP_KERNEL); > @@ -2063,7 +2068,9 @@ static int dwc3_probe(struct platform_device *pdev) > err5: > dwc3_debugfs_exit(dwc); > > - dwc3_event_buffers_cleanup(dwc); > + hw_mode = DWC3_GHWPARAMS0_MODE(dwc->hwparams.hwparams0); > + if (hw_mode != DWC3_GHWPARAMS0_MODE_HOST) > + dwc3_event_buffers_cleanup(dwc); > > usb_phy_set_suspend(dwc->usb2_phy, 1); > usb_phy_set_suspend(dwc->usb3_phy, 1); > -- > 2.39.0 >
Hi Thinh, On Thu, Jan 19, 2023 at 12:38:51AM +0000, Thinh Nguyen wrote: > On Sun, Jan 15, 2023, Krishna Kurapati wrote: > > Multiport controllers being host-only capable do not have GEVNTADDR Multiport may not be relevant here. Host-only is though. > > HI/LO, SIZE, COUNT reigsters present. Accsesing them to setup event > > I think you should reword "present" to something else. They're still > present In our case we have an instance where the IP is statically configured via coreConsultant with DWC_USB31_MODE==1 (host only) and we did observe that none of the registers pertaining to device mode (including GEVNT* and of course all the D* ones) are even *present* in the register map. If we try to access them we encounter some kind of access error or stall (or translation fault as described). So the approach here is to first verify by checking the HWPARAMS0 register if the HW is even capable of device mode in the first place. > but those registers are to be set while operating in device > mode. The rest looks fine. Are you suggesting only touching the GEVNT* registers when *operating* in device mode, even in the case of a dual-role capable controller? In that case would it make more sense to additionally move the calls to dwc3_event_buffers_{setup,cleanup} out of core.c and into dwc3_gadget_{init,exit} perhaps? That way we avoid them completely unless and until we switch into peripheral mode (assuming controller supports that, which we should already have checks for). Moreover, if the devicetree dr_mode property is set to host-only we'd also avoid calling these. > > buffers during core_init can cause an SMMU Fault. Avoid event buffers > > setup if the GHWPARAMS0 tells that the controller is host-only. > > > > Signed-off-by: Krishna Kurapati <quic_kriskura@quicinc.com> > > --- > > drivers/usb/dwc3/core.c | 23 +++++++++++++++-------- > > 1 file changed, 15 insertions(+), 8 deletions(-) > > > > diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c > > index 7e0a9a598dfd..f61ebddaecc0 100644 > > --- a/drivers/usb/dwc3/core.c > > +++ b/drivers/usb/dwc3/core.c > > @@ -871,9 +871,12 @@ static void dwc3_clk_disable(struct dwc3 *dwc) > > > > static void dwc3_core_exit(struct dwc3 *dwc) > > { > > - int i; > > + int i; > > + unsigned int hw_mode; > > > > - dwc3_event_buffers_cleanup(dwc); > > + hw_mode = DWC3_GHWPARAMS0_MODE(dwc->hwparams.hwparams0); > > + if (hw_mode != DWC3_GHWPARAMS0_MODE_HOST) If we stick with this approach, we probably could just check dwc->dr_mode instead as probe should have already set that to be an intersection between the values given in devicetree "dr_mode" and the HWPARAMS0 capability. Thanks, Jack > > + dwc3_event_buffers_cleanup(dwc); > > > > usb_phy_set_suspend(dwc->usb2_phy, 1); > > usb_phy_set_suspend(dwc->usb3_phy, 1); > > @@ -1246,10 +1249,12 @@ static int dwc3_core_init(struct dwc3 *dwc) > > } > > } > > > > - ret = dwc3_event_buffers_setup(dwc); > > - if (ret) { > > - dev_err(dwc->dev, "failed to setup event buffers\n"); > > - goto err4; > > + if (hw_mode != DWC3_GHWPARAMS0_MODE_HOST) { > > + ret = dwc3_event_buffers_setup(dwc); > > + if (ret) { > > + dev_err(dwc->dev, "failed to setup event buffers\n"); > > + goto err4; > > + } > > } > > > > /* > > @@ -1886,7 +1891,7 @@ static int dwc3_probe(struct platform_device *pdev) > > struct resource *res, dwc_res; > > struct dwc3 *dwc; > > int ret, i; > > - > > + unsigned int hw_mode; > > void __iomem *regs; > > > > dwc = devm_kzalloc(dev, sizeof(*dwc), GFP_KERNEL); > > @@ -2063,7 +2068,9 @@ static int dwc3_probe(struct platform_device *pdev) > > err5: > > dwc3_debugfs_exit(dwc); > > > > - dwc3_event_buffers_cleanup(dwc); > > + hw_mode = DWC3_GHWPARAMS0_MODE(dwc->hwparams.hwparams0); > > + if (hw_mode != DWC3_GHWPARAMS0_MODE_HOST) > > + dwc3_event_buffers_cleanup(dwc); > > > > usb_phy_set_suspend(dwc->usb2_phy, 1); > > usb_phy_set_suspend(dwc->usb3_phy, 1); > > -- > > 2.39.0 > >
On Wed, Jan 18, 2023, Jack Pham wrote: > Hi Thinh, > > On Thu, Jan 19, 2023 at 12:38:51AM +0000, Thinh Nguyen wrote: > > On Sun, Jan 15, 2023, Krishna Kurapati wrote: > > > Multiport controllers being host-only capable do not have GEVNTADDR > > Multiport may not be relevant here. Host-only is though. > > > > HI/LO, SIZE, COUNT reigsters present. Accsesing them to setup event > > > > I think you should reword "present" to something else. They're still > > present > > In our case we have an instance where the IP is statically configured > via coreConsultant with DWC_USB31_MODE==1 (host only) and we did observe > that none of the registers pertaining to device mode (including GEVNT* > and of course all the D* ones) are even *present* in the register map. > If we try to access them we encounter some kind of access error or stall > (or translation fault as described). So the approach here is to first > verify by checking the HWPARAMS0 register if the HW is even capable of > device mode in the first place. I see. > > > but those registers are to be set while operating in device > > mode. The rest looks fine. > > Are you suggesting only touching the GEVNT* registers when *operating* > in device mode, even in the case of a dual-role capable controller? In > that case would it make more sense to additionally move the calls to > dwc3_event_buffers_{setup,cleanup} out of core.c and into > dwc3_gadget_{init,exit} perhaps? That way we avoid them completely While it shouldn't be a problem for DRD, it may be cleaner to do that. > unless and until we switch into peripheral mode (assuming controller > supports that, which we should already have checks for). Moreover, if > the devicetree dr_mode property is set to host-only we'd also avoid > calling these. > > > > buffers during core_init can cause an SMMU Fault. Avoid event buffers > > > setup if the GHWPARAMS0 tells that the controller is host-only. > > > > > > Signed-off-by: Krishna Kurapati <quic_kriskura@quicinc.com> > > > --- > > > drivers/usb/dwc3/core.c | 23 +++++++++++++++-------- > > > 1 file changed, 15 insertions(+), 8 deletions(-) > > > > > > diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c > > > index 7e0a9a598dfd..f61ebddaecc0 100644 > > > --- a/drivers/usb/dwc3/core.c > > > +++ b/drivers/usb/dwc3/core.c > > > @@ -871,9 +871,12 @@ static void dwc3_clk_disable(struct dwc3 *dwc) > > > > > > static void dwc3_core_exit(struct dwc3 *dwc) > > > { > > > - int i; > > > + int i; > > > + unsigned int hw_mode; > > > > > > - dwc3_event_buffers_cleanup(dwc); > > > + hw_mode = DWC3_GHWPARAMS0_MODE(dwc->hwparams.hwparams0); > > > + if (hw_mode != DWC3_GHWPARAMS0_MODE_HOST) > > If we stick with this approach, we probably could just check > dwc->dr_mode instead as probe should have already set that to be an > intersection between the values given in devicetree "dr_mode" and the > HWPARAMS0 capability. > What we have here should not break DRD, so it's fine either way. Thanks, Thinh
On 1/19/2023 6:06 AM, Thinh Nguyen wrote: > Hi, > > On Sun, Jan 15, 2023, Krishna Kurapati wrote: >> Currently the DWC3 driver supports only single port controller >> which requires at most one HS and one SS PHY. > > Add note here that multi-port is for host mode for clarity. > >> >> But the DWC3 USB controller can be connected to multiple ports and >> each port can have their own PHYs. Each port of the multiport >> controller can either be HS+SS capable or HS only capable >> Proper quantification of them is required to modify GUSB2PHYCFG >> and GUSB3PIPECTL registers appropriately. >> >> Add support for detecting, obtaining and configuring phy's supported >> by a multiport controller and limit the max number of ports >> supported to 4. >> >> Signed-off-by: Harsh Agarwal <quic_harshq@quicinc.com> >> Signed-off-by: Krishna Kurapati <quic_kriskura@quicinc.com> >> --- >> drivers/usb/dwc3/core.c | 304 +++++++++++++++++++++++++++++----------- >> drivers/usb/dwc3/core.h | 15 +- >> drivers/usb/dwc3/drd.c | 14 +- >> 3 files changed, 244 insertions(+), 89 deletions(-) >> >> diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c >> index 476b63618511..7e0a9a598dfd 100644 >> --- a/drivers/usb/dwc3/core.c >> +++ b/drivers/usb/dwc3/core.c >> @@ -120,7 +120,7 @@ static void __dwc3_set_mode(struct work_struct *work) >> { >> struct dwc3 *dwc = work_to_dwc(work); >> unsigned long flags; >> - int ret; >> + int ret, i; > > Can we declare variables in separate lines here and other places. > >> u32 reg; >> u32 desired_dr_role; >> >> @@ -200,8 +200,10 @@ static void __dwc3_set_mode(struct work_struct *work) >> } else { >> if (dwc->usb2_phy) >> otg_set_vbus(dwc->usb2_phy->otg, true); >> - phy_set_mode(dwc->usb2_generic_phy, PHY_MODE_USB_HOST); >> - phy_set_mode(dwc->usb3_generic_phy, PHY_MODE_USB_HOST); >> + for (i = 0; i < dwc->num_ports; i++) { >> + 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 (dwc->dis_split_quirk) { >> reg = dwc3_readl(dwc->regs, DWC3_GUCTL3); >> reg |= DWC3_GUCTL3_SPLITDISABLE; >> @@ -216,8 +218,8 @@ static void __dwc3_set_mode(struct work_struct *work) >> >> if (dwc->usb2_phy) >> otg_set_vbus(dwc->usb2_phy->otg, false); >> - phy_set_mode(dwc->usb2_generic_phy, PHY_MODE_USB_DEVICE); >> - phy_set_mode(dwc->usb3_generic_phy, PHY_MODE_USB_DEVICE); >> + phy_set_mode(dwc->usb2_generic_phy[0], PHY_MODE_USB_DEVICE); >> + phy_set_mode(dwc->usb3_generic_phy[0], PHY_MODE_USB_DEVICE); >> >> ret = dwc3_gadget_init(dwc); >> if (ret) >> @@ -659,22 +661,14 @@ static int dwc3_core_ulpi_init(struct dwc3 *dwc) >> return ret; >> } >> >> -/** >> - * dwc3_phy_setup - Configure USB PHY Interface of DWC3 Core >> - * @dwc: Pointer to our controller context structure >> - * >> - * Returns 0 on success. The USB PHY interfaces are configured but not >> - * initialized. The PHY interfaces and the PHYs get initialized together with >> - * the core in dwc3_core_init. >> - */ >> -static int dwc3_phy_setup(struct dwc3 *dwc) >> +static int dwc3_ss_phy_setup(struct dwc3 *dwc, int index) >> { >> unsigned int hw_mode; >> u32 reg; >> >> hw_mode = DWC3_GHWPARAMS0_MODE(dwc->hwparams.hwparams0); >> >> - reg = dwc3_readl(dwc->regs, DWC3_GUSB3PIPECTL(0)); >> + reg = dwc3_readl(dwc->regs, DWC3_GUSB3PIPECTL(index)); >> >> /* >> * Make sure UX_EXIT_PX is cleared as that causes issues with some >> @@ -729,9 +723,19 @@ static int dwc3_phy_setup(struct dwc3 *dwc) >> if (dwc->dis_del_phy_power_chg_quirk) >> reg &= ~DWC3_GUSB3PIPECTL_DEPOCHANGE; >> >> - dwc3_writel(dwc->regs, DWC3_GUSB3PIPECTL(0), reg); >> + dwc3_writel(dwc->regs, DWC3_GUSB3PIPECTL(index), reg); >> >> - reg = dwc3_readl(dwc->regs, DWC3_GUSB2PHYCFG(0)); >> + return 0; >> +} >> + >> +static int dwc3_hs_phy_setup(struct dwc3 *dwc, int index) >> +{ >> + unsigned int hw_mode; >> + u32 reg; >> + >> + hw_mode = DWC3_GHWPARAMS0_MODE(dwc->hwparams.hwparams0); >> + >> + reg = dwc3_readl(dwc->regs, DWC3_GUSB2PHYCFG(index)); >> >> /* Select the HS PHY interface */ >> switch (DWC3_GHWPARAMS3_HSPHY_IFC(dwc->hwparams.hwparams3)) { >> @@ -743,7 +747,7 @@ static int dwc3_phy_setup(struct dwc3 *dwc) >> } else if (dwc->hsphy_interface && >> !strncmp(dwc->hsphy_interface, "ulpi", 4)) { >> reg |= DWC3_GUSB2PHYCFG_ULPI_UTMI; >> - dwc3_writel(dwc->regs, DWC3_GUSB2PHYCFG(0), reg); >> + dwc3_writel(dwc->regs, DWC3_GUSB2PHYCFG(index), reg); >> } else { >> /* Relying on default value. */ >> if (!(reg & DWC3_GUSB2PHYCFG_ULPI_UTMI)) >> @@ -800,7 +804,35 @@ static int dwc3_phy_setup(struct dwc3 *dwc) >> if (dwc->dis_u2_freeclk_exists_quirk || dwc->gfladj_refclk_lpm_sel) >> reg &= ~DWC3_GUSB2PHYCFG_U2_FREECLK_EXISTS; >> >> - dwc3_writel(dwc->regs, DWC3_GUSB2PHYCFG(0), reg); >> + dwc3_writel(dwc->regs, DWC3_GUSB2PHYCFG(index), reg); >> + >> + return 0; >> +} >> + >> +/** >> + * dwc3_phy_setup - Configure USB PHY Interface of DWC3 Core >> + * @dwc: Pointer to our controller context structure >> + * >> + * Returns 0 on success. The USB PHY interfaces are configured but not >> + * initialized. The PHY interfaces and the PHYs get initialized together with >> + * the core in dwc3_core_init. >> + */ >> +static int dwc3_phy_setup(struct dwc3 *dwc) >> +{ >> + int i; >> + int ret; >> + >> + for (i = 0; i < dwc->num_ss_ports; i++) { >> + ret = dwc3_ss_phy_setup(dwc, i); >> + if (ret) >> + return ret; >> + } >> + >> + for (i = 0; i < dwc->num_ports; i++) { >> + ret = dwc3_hs_phy_setup(dwc, i); >> + if (ret) >> + return ret; >> + } >> >> return 0; >> } >> @@ -839,17 +871,25 @@ static void dwc3_clk_disable(struct dwc3 *dwc) >> >> static void dwc3_core_exit(struct dwc3 *dwc) >> { >> + int i; >> + >> dwc3_event_buffers_cleanup(dwc); >> >> usb_phy_set_suspend(dwc->usb2_phy, 1); >> usb_phy_set_suspend(dwc->usb3_phy, 1); >> - phy_power_off(dwc->usb2_generic_phy); >> - phy_power_off(dwc->usb3_generic_phy); >> + >> + for (i = 0; i < dwc->num_ports; i++) { >> + phy_power_off(dwc->usb2_generic_phy[i]); >> + phy_power_off(dwc->usb3_generic_phy[i]); >> + } >> >> usb_phy_shutdown(dwc->usb2_phy); >> usb_phy_shutdown(dwc->usb3_phy); >> - phy_exit(dwc->usb2_generic_phy); >> - phy_exit(dwc->usb3_generic_phy); >> + >> + for (i = 0; i < dwc->num_ports; i++) { >> + phy_exit(dwc->usb2_generic_phy[i]); >> + phy_exit(dwc->usb3_generic_phy[i]); >> + } >> >> dwc3_clk_disable(dwc); >> reset_control_assert(dwc->reset); >> @@ -1085,6 +1125,7 @@ static int dwc3_core_init(struct dwc3 *dwc) >> unsigned int hw_mode; >> u32 reg; >> int ret; >> + int i, j; >> >> hw_mode = DWC3_GHWPARAMS0_MODE(dwc->hwparams.hwparams0); >> >> @@ -1119,14 +1160,27 @@ static int dwc3_core_init(struct dwc3 *dwc) >> >> usb_phy_init(dwc->usb2_phy); >> usb_phy_init(dwc->usb3_phy); >> - ret = phy_init(dwc->usb2_generic_phy); >> - if (ret < 0) >> - goto err0a; >> >> - ret = phy_init(dwc->usb3_generic_phy); >> - if (ret < 0) { >> - phy_exit(dwc->usb2_generic_phy); >> - goto err0a; >> + for (i = 0; i < dwc->num_ports; i++) { >> + ret = phy_init(dwc->usb2_generic_phy[i]); >> + if (ret < 0) { >> + /* clean up prior initialized HS PHYs */ >> + for (j = 0; j < i; j++) >> + phy_exit(dwc->usb2_generic_phy[j]); >> + goto err0a; >> + } >> + } >> + >> + for (i = 0; i < dwc->num_ports; i++) { >> + ret = phy_init(dwc->usb3_generic_phy[i]); >> + if (ret < 0) { >> + /* clean up prior initialized SS PHYs */ >> + for (j = 0; j < i; j++) >> + phy_exit(dwc->usb3_generic_phy[j]); >> + for (j = 0; j < dwc->num_ports; j++) >> + phy_exit(dwc->usb2_generic_phy[j]); >> + goto err0a; >> + } >> } >> >> ret = dwc3_core_soft_reset(dwc); >> @@ -1136,15 +1190,19 @@ static int dwc3_core_init(struct dwc3 *dwc) >> if (hw_mode == DWC3_GHWPARAMS0_MODE_DRD && >> !DWC3_VER_IS_WITHIN(DWC3, ANY, 194A)) { >> if (!dwc->dis_u3_susphy_quirk) { >> - reg = dwc3_readl(dwc->regs, DWC3_GUSB3PIPECTL(0)); >> - reg |= DWC3_GUSB3PIPECTL_SUSPHY; >> - dwc3_writel(dwc->regs, DWC3_GUSB3PIPECTL(0), reg); >> + for (i = 0; i < dwc->num_ss_ports; i++) { >> + reg = dwc3_readl(dwc->regs, DWC3_GUSB3PIPECTL(i)); >> + reg |= DWC3_GUSB3PIPECTL_SUSPHY; >> + dwc3_writel(dwc->regs, DWC3_GUSB3PIPECTL(i), reg); >> + } >> } >> >> if (!dwc->dis_u2_susphy_quirk) { >> - reg = dwc3_readl(dwc->regs, DWC3_GUSB2PHYCFG(0)); >> - reg |= DWC3_GUSB2PHYCFG_SUSPHY; >> - dwc3_writel(dwc->regs, DWC3_GUSB2PHYCFG(0), reg); >> + for (i = 0; i < dwc->num_ports; i++) { >> + reg = dwc3_readl(dwc->regs, DWC3_GUSB2PHYCFG(i)); >> + reg |= DWC3_GUSB2PHYCFG_SUSPHY; >> + dwc3_writel(dwc->regs, DWC3_GUSB2PHYCFG(i), reg); >> + } >> } >> } >> >> @@ -1168,13 +1226,25 @@ static int dwc3_core_init(struct dwc3 *dwc) >> >> usb_phy_set_suspend(dwc->usb2_phy, 0); >> usb_phy_set_suspend(dwc->usb3_phy, 0); >> - ret = phy_power_on(dwc->usb2_generic_phy); >> - if (ret < 0) >> - goto err2; >> >> - ret = phy_power_on(dwc->usb3_generic_phy); >> - if (ret < 0) >> - goto err3; >> + >> + for (i = 0; i < dwc->num_ports; i++) { >> + ret = phy_power_on(dwc->usb2_generic_phy[i]); >> + if (ret < 0) { >> + for (j = 0; j < i; j++) >> + phy_power_off(dwc->usb2_generic_phy[j]); >> + goto err2; >> + } >> + } >> + >> + for (i = 0; i < dwc->num_ports; i++) { >> + ret = phy_power_on(dwc->usb3_generic_phy[i]); >> + if (ret < 0) { >> + for (j = 0; j < i; j++) >> + phy_power_off(dwc->usb3_generic_phy[j]); >> + goto err3; >> + } >> + } >> >> ret = dwc3_event_buffers_setup(dwc); >> if (ret) { >> @@ -1297,10 +1367,12 @@ static int dwc3_core_init(struct dwc3 *dwc) >> return 0; >> >> err4: >> - phy_power_off(dwc->usb3_generic_phy); >> + for (i = 0; i < dwc->num_ports; i++) >> + phy_power_off(dwc->usb3_generic_phy[i]); >> >> err3: >> - phy_power_off(dwc->usb2_generic_phy); >> + for (i = 0; i < dwc->num_ports; i++) >> + phy_power_off(dwc->usb2_generic_phy[i]); >> >> err2: >> usb_phy_set_suspend(dwc->usb2_phy, 1); >> @@ -1309,8 +1381,11 @@ static int dwc3_core_init(struct dwc3 *dwc) >> err1: >> usb_phy_shutdown(dwc->usb2_phy); >> usb_phy_shutdown(dwc->usb3_phy); >> - phy_exit(dwc->usb2_generic_phy); >> - phy_exit(dwc->usb3_generic_phy); >> + >> + for (i = 0; i < dwc->num_ports; i++) { >> + phy_exit(dwc->usb2_generic_phy[i]); >> + phy_exit(dwc->usb3_generic_phy[i]); >> + } >> >> err0a: >> dwc3_ulpi_exit(dwc); >> @@ -1319,6 +1394,38 @@ static int dwc3_core_init(struct dwc3 *dwc) >> return ret; >> } >> >> +static int dwc3_get_multiport_phys(struct dwc3 *dwc) >> +{ >> + int ret; >> + struct device *dev = dwc->dev; >> + int i; >> + char phy_name[15]; >> + >> + for (i = 0; i < dwc->num_ports; i++) { >> + sprintf(phy_name, "usb2-phy_port%d", i); >> + dwc->usb2_generic_phy[i] = devm_phy_get(dev, phy_name); >> + if (IS_ERR(dwc->usb2_generic_phy[i])) { >> + ret = PTR_ERR(dwc->usb2_generic_phy[i]); >> + if (ret == -ENOSYS || ret == -ENODEV) >> + dwc->usb2_generic_phy[i] = NULL; >> + else >> + return dev_err_probe(dev, ret, "usb2 phy: %s not configured\n", phy_name); >> + } >> + >> + sprintf(phy_name, "usb3-phy_port%d", i); >> + dwc->usb3_generic_phy[i] = devm_phy_get(dev, phy_name); >> + if (IS_ERR(dwc->usb3_generic_phy[i])) { >> + ret = PTR_ERR(dwc->usb3_generic_phy[i]); >> + if (ret == -ENOSYS || ret == -ENODEV) >> + dwc->usb3_generic_phy[i] = NULL; >> + else >> + return dev_err_probe(dev, ret, "usb3 phy: %s not configured\n", phy_name); >> + } >> + } >> + >> + return 0; >> +} >> + >> static int dwc3_core_get_phy(struct dwc3 *dwc) >> { >> struct device *dev = dwc->dev; >> @@ -1349,31 +1456,37 @@ static int dwc3_core_get_phy(struct dwc3 *dwc) >> return dev_err_probe(dev, ret, "no usb3 phy configured\n"); >> } >> >> - dwc->usb2_generic_phy = devm_phy_get(dev, "usb2-phy"); >> - if (IS_ERR(dwc->usb2_generic_phy)) { >> - ret = PTR_ERR(dwc->usb2_generic_phy); >> + if (dwc->num_ports > 1) >> + goto get_multiport_phys; > > Can we avoid goto and just return dwc3_get_multiport_phys(dwc) here? > It's easier to read IMO. > >> + >> + dwc->usb2_generic_phy[0] = devm_phy_get(dev, "usb2-phy"); >> + if (IS_ERR(dwc->usb2_generic_phy[0])) { >> + ret = PTR_ERR(dwc->usb2_generic_phy[0]); >> if (ret == -ENOSYS || ret == -ENODEV) >> - dwc->usb2_generic_phy = NULL; >> + dwc->usb2_generic_phy[0] = NULL; >> else >> return dev_err_probe(dev, ret, "no usb2 phy configured\n"); >> } >> >> - dwc->usb3_generic_phy = devm_phy_get(dev, "usb3-phy"); >> - if (IS_ERR(dwc->usb3_generic_phy)) { >> - ret = PTR_ERR(dwc->usb3_generic_phy); >> + dwc->usb3_generic_phy[0] = devm_phy_get(dev, "usb3-phy"); >> + if (IS_ERR(dwc->usb3_generic_phy[0])) { >> + ret = PTR_ERR(dwc->usb3_generic_phy[0]); >> if (ret == -ENOSYS || ret == -ENODEV) >> - dwc->usb3_generic_phy = NULL; >> + dwc->usb3_generic_phy[0] = NULL; >> else >> return dev_err_probe(dev, ret, "no usb3 phy configured\n"); >> } >> >> return 0; >> + >> +get_multiport_phys: >> + return dwc3_get_multiport_phys(dwc); >> } >> >> static int dwc3_core_init_mode(struct dwc3 *dwc) >> { >> struct device *dev = dwc->dev; >> - int ret; >> + int ret, i; >> >> switch (dwc->dr_mode) { >> case USB_DR_MODE_PERIPHERAL: >> @@ -1381,8 +1494,8 @@ static int dwc3_core_init_mode(struct dwc3 *dwc) >> >> if (dwc->usb2_phy) >> otg_set_vbus(dwc->usb2_phy->otg, false); >> - phy_set_mode(dwc->usb2_generic_phy, PHY_MODE_USB_DEVICE); >> - phy_set_mode(dwc->usb3_generic_phy, PHY_MODE_USB_DEVICE); >> + phy_set_mode(dwc->usb2_generic_phy[0], PHY_MODE_USB_DEVICE); >> + phy_set_mode(dwc->usb3_generic_phy[0], PHY_MODE_USB_DEVICE); >> >> ret = dwc3_gadget_init(dwc); >> if (ret) >> @@ -1393,8 +1506,10 @@ static int dwc3_core_init_mode(struct dwc3 *dwc) >> >> if (dwc->usb2_phy) >> otg_set_vbus(dwc->usb2_phy->otg, true); >> - phy_set_mode(dwc->usb2_generic_phy, PHY_MODE_USB_HOST); >> - phy_set_mode(dwc->usb3_generic_phy, PHY_MODE_USB_HOST); >> + for (i = 0; i < dwc->num_ports; i++) { >> + phy_set_mode(dwc->usb2_generic_phy[i], PHY_MODE_USB_HOST); >> + phy_set_mode(dwc->usb3_generic_phy[i], PHY_MODE_USB_HOST); >> + } >> >> ret = dwc3_host_init(dwc); >> if (ret) >> @@ -1575,6 +1690,21 @@ static void dwc3_get_properties(struct dwc3 *dwc) >> dwc->dis_split_quirk = device_property_read_bool(dev, >> "snps,dis-split-quirk"); >> >> + >> + /* >> + * If no mulitport properties are defined, default > > multi* > >> + * the port count to '1'. >> + */ > > Can we initialize num_ports and num_ss_ports to 1 instead of setting it > on error (similar to how we handle other properties). > Hi Thinh, Thanks for the review. On the bindings, Rob and Krzysztof have suggested to get the num-ports and num-ss-ports by counting the Phy-names in DT. Since there may be many cases where the user might skip giving any Phy's or even skip different ports in the DT if he doesn't want to use them, can we design/refactor the below logic as follows while mandating the fact that user must give the SS Phy's if any starting from Port-0.: num-ss-ports = max_port_index (usb3-portX) + 1 num-ports = max (max_port_index(usb2-portX), num-ss-ports) + 1 Ex: If there are 3 ports and only 1 is SS capable and user decides to skip port-2 HS Phy. case-1: phy-names = "usb2-port0", "usb3-port0", "usb2-port-1" case-2: phy-names = "usb2-port0", "usb2-port-1", "usb3-port1" In both cases, only one SS is present, just the order is changed. (Not sure if last few ports can be made SS Capable instead of the first ports on any HW) ? But according to the above formula: In case-1 : (num-ports = 2, num-ss-ports = 1) - This is correct In case-2: (num-ports = 2, num-ss-ports = 2) - This is wrong I believe this covers all cases and I can read this in get_properties function. Let me know your opinion if this design is good to proceed further. Regards, Krishna, >> + ret = device_property_read_u32(dev, "num-ports", >> + &dwc->num_ports); >> + if (ret) >> + dwc->num_ports = 1; >> + >> + ret = device_property_read_u32(dev, "num-ss-ports", >> + &dwc->num_ss_ports); >> + if (ret) >> + dwc->num_ss_ports = 1; >> + >> dwc->lpm_nyet_threshold = lpm_nyet_threshold; >> dwc->tx_de_emphasis = tx_de_emphasis; >> >> @@ -1755,8 +1885,7 @@ static int dwc3_probe(struct platform_device *pdev) >> struct device *dev = &pdev->dev; >> struct resource *res, dwc_res; >> struct dwc3 *dwc; >> - >> - int ret; >> + int ret, i; >> >> void __iomem *regs; >> >> @@ -1933,17 +2062,24 @@ static int dwc3_probe(struct platform_device *pdev) >> >> err5: >> dwc3_debugfs_exit(dwc); >> + >> dwc3_event_buffers_cleanup(dwc); >> >> usb_phy_set_suspend(dwc->usb2_phy, 1); >> usb_phy_set_suspend(dwc->usb3_phy, 1); >> - phy_power_off(dwc->usb2_generic_phy); >> - phy_power_off(dwc->usb3_generic_phy); >> + >> + for (i = 0; i < dwc->num_ports; i++) { >> + phy_power_off(dwc->usb2_generic_phy[i]); >> + phy_power_off(dwc->usb3_generic_phy[i]); >> + } >> >> usb_phy_shutdown(dwc->usb2_phy); >> usb_phy_shutdown(dwc->usb3_phy); >> - phy_exit(dwc->usb2_generic_phy); >> - phy_exit(dwc->usb3_generic_phy); >> + >> + for (i = 0; i < dwc->num_ports; i++) { >> + phy_exit(dwc->usb2_generic_phy[i]); >> + phy_exit(dwc->usb3_generic_phy[i]); >> + } >> >> dwc3_ulpi_exit(dwc); >> >> @@ -2025,6 +2161,7 @@ static int dwc3_core_init_for_resume(struct dwc3 *dwc) >> >> static int dwc3_suspend_common(struct dwc3 *dwc, pm_message_t msg) >> { >> + int i; >> unsigned long flags; >> u32 reg; >> >> @@ -2045,17 +2182,21 @@ static int dwc3_suspend_common(struct dwc3 *dwc, pm_message_t msg) >> /* Let controller to suspend HSPHY before PHY driver suspends */ >> if (dwc->dis_u2_susphy_quirk || >> dwc->dis_enblslpm_quirk) { >> - reg = dwc3_readl(dwc->regs, DWC3_GUSB2PHYCFG(0)); >> - reg |= DWC3_GUSB2PHYCFG_ENBLSLPM | >> - DWC3_GUSB2PHYCFG_SUSPHY; >> - dwc3_writel(dwc->regs, DWC3_GUSB2PHYCFG(0), reg); >> + for (i = 0; i < dwc->num_ports; i++) { >> + reg = dwc3_readl(dwc->regs, DWC3_GUSB2PHYCFG(i)); >> + reg |= DWC3_GUSB2PHYCFG_ENBLSLPM | >> + DWC3_GUSB2PHYCFG_SUSPHY; >> + dwc3_writel(dwc->regs, DWC3_GUSB2PHYCFG(i), reg); >> + } >> >> /* Give some time for USB2 PHY to suspend */ >> usleep_range(5000, 6000); >> } >> >> - phy_pm_runtime_put_sync(dwc->usb2_generic_phy); >> - phy_pm_runtime_put_sync(dwc->usb3_generic_phy); >> + for (i = 0; i < dwc->num_ports; i++) { >> + phy_pm_runtime_put_sync(dwc->usb2_generic_phy[i]); >> + phy_pm_runtime_put_sync(dwc->usb3_generic_phy[i]); >> + } >> break; >> case DWC3_GCTL_PRTCAP_OTG: >> /* do nothing during runtime_suspend */ >> @@ -2084,6 +2225,7 @@ static int dwc3_resume_common(struct dwc3 *dwc, pm_message_t msg) >> { >> unsigned long flags; >> int ret; >> + int i; >> u32 reg; >> >> switch (dwc->current_dr_role) { >> @@ -2104,17 +2246,21 @@ static int dwc3_resume_common(struct dwc3 *dwc, pm_message_t msg) >> break; >> } >> /* Restore GUSB2PHYCFG bits that were modified in suspend */ >> - reg = dwc3_readl(dwc->regs, DWC3_GUSB2PHYCFG(0)); >> - if (dwc->dis_u2_susphy_quirk) >> - reg &= ~DWC3_GUSB2PHYCFG_SUSPHY; >> + for (i = 0; i < dwc->num_ports; i++) { >> + reg = dwc3_readl(dwc->regs, DWC3_GUSB2PHYCFG(i)); >> + if (dwc->dis_u2_susphy_quirk) >> + reg &= ~DWC3_GUSB2PHYCFG_SUSPHY; >> >> - if (dwc->dis_enblslpm_quirk) >> - reg &= ~DWC3_GUSB2PHYCFG_ENBLSLPM; >> + if (dwc->dis_enblslpm_quirk) >> + reg &= ~DWC3_GUSB2PHYCFG_ENBLSLPM; >> >> - dwc3_writel(dwc->regs, DWC3_GUSB2PHYCFG(0), reg); >> + dwc3_writel(dwc->regs, DWC3_GUSB2PHYCFG(i), reg); >> + } >> >> - phy_pm_runtime_get_sync(dwc->usb2_generic_phy); >> - phy_pm_runtime_get_sync(dwc->usb3_generic_phy); >> + for (i = 0; i < dwc->num_ports; i++) { >> + phy_pm_runtime_get_sync(dwc->usb2_generic_phy[i]); >> + phy_pm_runtime_get_sync(dwc->usb3_generic_phy[i]); >> + } >> break; >> case DWC3_GCTL_PRTCAP_OTG: >> /* nothing to do on runtime_resume */ >> diff --git a/drivers/usb/dwc3/core.h b/drivers/usb/dwc3/core.h >> index 8f9959ba9fd4..2f82eda9d44f 100644 >> --- a/drivers/usb/dwc3/core.h >> +++ b/drivers/usb/dwc3/core.h >> @@ -35,6 +35,9 @@ >> >> #define DWC3_MSG_MAX 500 >> >> +/* Number of ports supported by a multiport controller */ >> +#define MAX_PORTS_SUPPORTED 4 >> + >> /* Global constants */ >> #define DWC3_PULL_UP_TIMEOUT 500 /* ms */ >> #define DWC3_BOUNCE_SIZE 1024 /* size of a superspeed bulk */ >> @@ -1023,8 +1026,10 @@ struct dwc3_scratchpad_array { >> * @usb_psy: pointer to power supply interface. >> * @usb2_phy: pointer to USB2 PHY >> * @usb3_phy: pointer to USB3 PHY >> - * @usb2_generic_phy: pointer to USB2 PHY >> - * @usb3_generic_phy: pointer to USB3 PHY >> + * @num_ports: Indicates number of usb ports supported by the controller. > > Note that this is the total number of usb ports including the SS capable > ones. > >> + * @num_ss_ports: Indicates number of ss capable ports supported by controller >> + * @usb2_generic_phy: pointer to array of USB2 PHY's >> + * @usb3_generic_phy: pointer to array of USB3 PHY's >> * @phys_ready: flag to indicate that PHYs are ready >> * @ulpi: pointer to ulpi interface >> * @ulpi_ready: flag to indicate that ULPI is initialized >> @@ -1157,8 +1162,10 @@ struct dwc3 { >> struct usb_phy *usb2_phy; >> struct usb_phy *usb3_phy; >> >> - struct phy *usb2_generic_phy; >> - struct phy *usb3_generic_phy; >> + u32 num_ports; >> + u32 num_ss_ports; >> + struct phy *usb2_generic_phy[MAX_PORTS_SUPPORTED]; >> + struct phy *usb3_generic_phy[MAX_PORTS_SUPPORTED]; >> >> bool phys_ready; >> >> diff --git a/drivers/usb/dwc3/drd.c b/drivers/usb/dwc3/drd.c >> index 039bf241769a..ea86ff01433b 100644 >> --- a/drivers/usb/dwc3/drd.c >> +++ b/drivers/usb/dwc3/drd.c >> @@ -327,7 +327,7 @@ static void dwc3_otg_device_exit(struct dwc3 *dwc) >> >> void dwc3_otg_update(struct dwc3 *dwc, bool ignore_idstatus) >> { >> - int ret; >> + int ret, i; >> u32 reg; >> int id; >> unsigned long flags; >> @@ -386,9 +386,11 @@ void dwc3_otg_update(struct dwc3 *dwc, bool ignore_idstatus) >> } else { >> if (dwc->usb2_phy) >> otg_set_vbus(dwc->usb2_phy->otg, true); >> - if (dwc->usb2_generic_phy) >> - phy_set_mode(dwc->usb2_generic_phy, >> - PHY_MODE_USB_HOST); >> + for (i = 0; i < dwc->num_ports; i++) { >> + if (dwc->usb2_generic_phy[i]) >> + phy_set_mode(dwc->usb2_generic_phy[i], >> + PHY_MODE_USB_HOST); >> + } >> } >> break; >> case DWC3_OTG_ROLE_DEVICE: >> @@ -400,8 +402,8 @@ void dwc3_otg_update(struct dwc3 *dwc, bool ignore_idstatus) >> >> if (dwc->usb2_phy) >> otg_set_vbus(dwc->usb2_phy->otg, false); >> - if (dwc->usb2_generic_phy) >> - phy_set_mode(dwc->usb2_generic_phy, >> + if (dwc->usb2_generic_phy[0]) >> + phy_set_mode(dwc->usb2_generic_phy[0], >> PHY_MODE_USB_DEVICE); >> ret = dwc3_gadget_init(dwc); >> if (ret) >> -- >> 2.39.0 >> > > No major issue I see here. Just some minor nits. Once you feel it's > ready, please drop the RFC tags on resubmission.
On Sun, Jan 15, 2023 at 05:11:41PM +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 can confirm that applying this series allow me to use all 6 USB ports on the ADP. Looking forward to v5. Thanks, Bjorn > Changes in v4: > Added DT support for SA8295p. > > Changes in v3: > Incase any PHY init fails, then clear/exit the PHYs that > are already initialized. > > Changes in 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 v3: https://lore.kernel.org/all/1654709787-23686-1-git-send-email-quic_harshq@quicinc.com/#r > Link to v2: https://lore.kernel.org/all/1653560029-6937-1-git-send-email-quic_harshq@quicinc.com/#r > > Krishna Kurapati (5): > dt-bindings: usb: Add bindings to support multiport properties > usb: dwc3: core: Refactor PHY logic to support Multiport Controller > usb: dwc3: core: Do not setup event buffers for host only controllers > usb: dwc3: qcom: Add multiport controller support for qcom wrapper > arm: dts: msm: Add multiport controller node for usb > > .../devicetree/bindings/usb/snps,dwc3.yaml | 42 ++- > arch/arm64/boot/dts/qcom/sa8295p-adp.dts | 49 +++ > arch/arm64/boot/dts/qcom/sc8280xp.dtsi | 60 ++++ > drivers/usb/dwc3/core.c | 325 +++++++++++++----- > drivers/usb/dwc3/core.h | 15 +- > drivers/usb/dwc3/drd.c | 14 +- > drivers/usb/dwc3/dwc3-qcom.c | 28 +- > 7 files changed, 429 insertions(+), 104 deletions(-) > > -- > 2.39.0 >
On 1/19/2023 9:13 AM, Bjorn Andersson wrote: > On Sun, Jan 15, 2023 at 05:11:41PM +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 can confirm that applying this series allow me to use all 6 USB ports > on the ADP. Looking forward to v5. > > Thanks, > Bjorn > Thanks Bjorn for testing it out. Regards, Krishna, >> Changes in v4: >> Added DT support for SA8295p. >> >> Changes in v3: >> Incase any PHY init fails, then clear/exit the PHYs that >> are already initialized. >> >> Changes in 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 v3: https://lore.kernel.org/all/1654709787-23686-1-git-send-email-quic_harshq@quicinc.com/#r >> Link to v2: https://lore.kernel.org/all/1653560029-6937-1-git-send-email-quic_harshq@quicinc.com/#r >> >> Krishna Kurapati (5): >> dt-bindings: usb: Add bindings to support multiport properties >> usb: dwc3: core: Refactor PHY logic to support Multiport Controller >> usb: dwc3: core: Do not setup event buffers for host only controllers >> usb: dwc3: qcom: Add multiport controller support for qcom wrapper >> arm: dts: msm: Add multiport controller node for usb >> >> .../devicetree/bindings/usb/snps,dwc3.yaml | 42 ++- >> arch/arm64/boot/dts/qcom/sa8295p-adp.dts | 49 +++ >> arch/arm64/boot/dts/qcom/sc8280xp.dtsi | 60 ++++ >> drivers/usb/dwc3/core.c | 325 +++++++++++++----- >> drivers/usb/dwc3/core.h | 15 +- >> drivers/usb/dwc3/drd.c | 14 +- >> drivers/usb/dwc3/dwc3-qcom.c | 28 +- >> 7 files changed, 429 insertions(+), 104 deletions(-) >> >> -- >> 2.39.0 >>
Hi, On Thu, Jan 19, 2023, Krishna Kurapati PSSNV wrote: > > > On 1/19/2023 6:06 AM, Thinh Nguyen wrote: > > Hi, > > > > On Sun, Jan 15, 2023, Krishna Kurapati wrote: > > > Currently the DWC3 driver supports only single port controller > > > which requires at most one HS and one SS PHY. > > > > Add note here that multi-port is for host mode for clarity. > > > > > > > > But the DWC3 USB controller can be connected to multiple ports and > > > each port can have their own PHYs. Each port of the multiport > > > controller can either be HS+SS capable or HS only capable > > > Proper quantification of them is required to modify GUSB2PHYCFG > > > and GUSB3PIPECTL registers appropriately. > > > > > > Add support for detecting, obtaining and configuring phy's supported > > > by a multiport controller and limit the max number of ports > > > supported to 4. > > > > > > Signed-off-by: Harsh Agarwal <quic_harshq@quicinc.com> > > > Signed-off-by: Krishna Kurapati <quic_kriskura@quicinc.com> > > > --- > > > drivers/usb/dwc3/core.c | 304 +++++++++++++++++++++++++++++----------- > > > drivers/usb/dwc3/core.h | 15 +- > > > drivers/usb/dwc3/drd.c | 14 +- > > > 3 files changed, 244 insertions(+), 89 deletions(-) > > > <snip> > > > @@ -1575,6 +1690,21 @@ static void dwc3_get_properties(struct dwc3 *dwc) > > > dwc->dis_split_quirk = device_property_read_bool(dev, > > > "snps,dis-split-quirk"); > > > + > > > + /* > > > + * If no mulitport properties are defined, default > > > > multi* > > > > > + * the port count to '1'. > > > + */ > > > > Can we initialize num_ports and num_ss_ports to 1 instead of setting it > > on error (similar to how we handle other properties). > > > Hi Thinh, > > Thanks for the review. On the bindings, Rob and Krzysztof have suggested > to get the num-ports and num-ss-ports by counting the Phy-names in DT. This may be a bit problematic for non-DT device. Currently pci devices pass fake DT properties to send these kinds of info. But that's fine, we can enhance dwc3 for non-DT devices later. > > Since there may be many cases where the user might skip giving any Phy's or > even skip different ports in the DT if he doesn't want to use them, can we > design/refactor the below logic as follows while mandating the fact that > user must give the SS Phy's if any starting from Port-0.: > > num-ss-ports = max_port_index (usb3-portX) + 1 > num-ports = max (max_port_index(usb2-portX), num-ss-ports) + 1 > > Ex: If there are 3 ports and only 1 is SS capable and user decides to skip > port-2 HS Phy. > > case-1: phy-names = "usb2-port0", "usb3-port0", "usb2-port-1" > case-2: phy-names = "usb2-port0", "usb2-port-1", "usb3-port1" > > In both cases, only one SS is present, just the order is changed. (Not sure > if last few ports can be made SS Capable instead of the first ports on any > HW) ? > > But according to the above formula: > > In case-1 : (num-ports = 2, num-ss-ports = 1) - This is correct > In case-2: (num-ports = 2, num-ss-ports = 2) - This is wrong > Can't we just walk through all the phy names to figure that out? Let's not require the user to specify Port-0 is SS capable if they can skip it. > I believe this covers all cases and I can read this in get_properties > function. Let me know your opinion if this design is good to proceed > further. > Thanks, Thinh
On 1/20/2023 6:32 AM, Thinh Nguyen wrote: > Hi, > > On Thu, Jan 19, 2023, Krishna Kurapati PSSNV wrote: >> >> >> On 1/19/2023 6:06 AM, Thinh Nguyen wrote: >>> Hi, >>> >>> On Sun, Jan 15, 2023, Krishna Kurapati wrote: >>>> Currently the DWC3 driver supports only single port controller >>>> which requires at most one HS and one SS PHY. >>> >>> Add note here that multi-port is for host mode for clarity. >>> >>>> >>>> But the DWC3 USB controller can be connected to multiple ports and >>>> each port can have their own PHYs. Each port of the multiport >>>> controller can either be HS+SS capable or HS only capable >>>> Proper quantification of them is required to modify GUSB2PHYCFG >>>> and GUSB3PIPECTL registers appropriately. >>>> >>>> Add support for detecting, obtaining and configuring phy's supported >>>> by a multiport controller and limit the max number of ports >>>> supported to 4. >>>> >>>> Signed-off-by: Harsh Agarwal <quic_harshq@quicinc.com> >>>> Signed-off-by: Krishna Kurapati <quic_kriskura@quicinc.com> >>>> --- >>>> drivers/usb/dwc3/core.c | 304 +++++++++++++++++++++++++++++----------- >>>> drivers/usb/dwc3/core.h | 15 +- >>>> drivers/usb/dwc3/drd.c | 14 +- >>>> 3 files changed, 244 insertions(+), 89 deletions(-) >>>> > > <snip> > >>>> @@ -1575,6 +1690,21 @@ static void dwc3_get_properties(struct dwc3 *dwc) >>>> dwc->dis_split_quirk = device_property_read_bool(dev, >>>> "snps,dis-split-quirk"); >>>> + >>>> + /* >>>> + * If no mulitport properties are defined, default >>> >>> multi* >>> >>>> + * the port count to '1'. >>>> + */ >>> >>> Can we initialize num_ports and num_ss_ports to 1 instead of setting it >>> on error (similar to how we handle other properties). >>> >> Hi Thinh, >> >> Thanks for the review. On the bindings, Rob and Krzysztof have suggested >> to get the num-ports and num-ss-ports by counting the Phy-names in DT. > > This may be a bit problematic for non-DT device. Currently pci devices > pass fake DT properties to send these kinds of info. But that's fine, > we can enhance dwc3 for non-DT devices later. > >> >> Since there may be many cases where the user might skip giving any Phy's or >> even skip different ports in the DT if he doesn't want to use them, can we >> design/refactor the below logic as follows while mandating the fact that >> user must give the SS Phy's if any starting from Port-0.: >> >> num-ss-ports = max_port_index (usb3-portX) + 1 >> num-ports = max (max_port_index(usb2-portX), num-ss-ports) + 1 >> >> Ex: If there are 3 ports and only 1 is SS capable and user decides to skip >> port-2 HS Phy. >> >> case-1: phy-names = "usb2-port0", "usb3-port0", "usb2-port-1" >> case-2: phy-names = "usb2-port0", "usb2-port-1", "usb3-port1" >> >> In both cases, only one SS is present, just the order is changed. (Not sure >> if last few ports can be made SS Capable instead of the first ports on any >> HW) ? >> >> But according to the above formula: >> >> In case-1 : (num-ports = 2, num-ss-ports = 1) - This is correct >> In case-2: (num-ports = 2, num-ss-ports = 2) - This is wrong >> > > Can't we just walk through all the phy names to figure that out? Let's > not require the user to specify Port-0 is SS capable if they can skip > it. > Hi Thinh, Thanks for the review. May be I wasn't able to convey my intention properly in my previous thread. The above suggested method doesn't tell that user must not skip any phy. Let us take the below case for 2 Port (HS+SS) capable controller. If the user skips ss-phy 2, then: phy-names = "usb2-port0", "usb3-port0", "usb2-port-1" We don't need to configure GUSB3PIPECTL2 (for port-2) register ere. If we parse phy-names and find it out, we see there are 2 hs-phy's and 1-ssphy and num-ports = 2 and num-ss-ports = 1. If the user skips ss-phy-1, then phy-names would be something like the below: phy-names = "usb2-port0", "usb2-port-1", "usb3-port1"; We need to handle two types of interpretations here while parsing the phy-names: a) There are 2 SS Phy's and we just skipped the first one. In this scenario, if we consider (num-ss-ports = 2) and (num-ports = 2) by using the above formula as reference, we configure both GUSB3PIPECTL registers present in the address map although we never use SS Phy-1 but nothing must break. All ports would still work as the user intends with the exception of GUSB3PIPECTL1 (for-port1) still being modified. b) The second interpretation is something like, port-1 is only HS capable and only Port-2 is SS Capable, and so in the phy-names only port-2 has been provided a SS Phy. Just by parsing through the phy-names, it would not be possible to get that info. So if we consider num-ss-ports as 2 in this scenario, we end up meddling with wrong registers (as there is only 1 GUSB3PIPECTL reg and we are assuing there are 2). I wanted to make sure that this scenario was not possible. num-ss-ports = max_port_index (usb3-portX) + 1 num-ports = max (max_port_index(usb2-portX), max_port_index(usb2-portX)) + 1 Taking case of a quad port controller with all ports SS Capable, if the user wants to completely skip port-4. Then with above formula, we get (num-ports = 3) and (num-ss-ports = 3) by parsing the phy-names and we will configure exactly those dwc3-phy registers and not touch the port-4 registers which is still fine. Please let me know if the above idea helps us in this scenario. Regards, Krishna,
On 1/20/2023 3:39 AM, Andrew Halaney wrote: > On Sun, Jan 15, 2023 at 05:11:43PM +0530, Krishna Kurapati wrote: >> Currently the DWC3 driver supports only single port controller >> which requires at most one HS and one SS PHY. >> >> But the DWC3 USB controller can be connected to multiple ports and >> each port can have their own PHYs. Each port of the multiport >> controller can either be HS+SS capable or HS only capable >> Proper quantification of them is required to modify GUSB2PHYCFG >> and GUSB3PIPECTL registers appropriately. >> >> Add support for detecting, obtaining and configuring phy's supported >> by a multiport controller and limit the max number of ports >> supported to 4. >> >> Signed-off-by: Harsh Agarwal <quic_harshq@quicinc.com> >> Signed-off-by: Krishna Kurapati <quic_kriskura@quicinc.com> >> --- >> drivers/usb/dwc3/core.c | 304 +++++++++++++++++++++++++++++----------- >> drivers/usb/dwc3/core.h | 15 +- >> drivers/usb/dwc3/drd.c | 14 +- >> 3 files changed, 244 insertions(+), 89 deletions(-) >> >> diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c >> index 476b63618511..7e0a9a598dfd 100644 >> --- a/drivers/usb/dwc3/core.c >> +++ b/drivers/usb/dwc3/core.c > > <snip> > >> @@ -1575,6 +1690,21 @@ static void dwc3_get_properties(struct dwc3 *dwc) >> dwc->dis_split_quirk = device_property_read_bool(dev, >> "snps,dis-split-quirk"); >> >> + >> + /* >> + * If no mulitport properties are defined, default >> + * the port count to '1'. >> + */ >> + ret = device_property_read_u32(dev, "num-ports", >> + &dwc->num_ports); >> + if (ret) >> + dwc->num_ports = 1; >> + >> + ret = device_property_read_u32(dev, "num-ss-ports", >> + &dwc->num_ss_ports); >> + if (ret) >> + dwc->num_ss_ports = 1; > > By using this DT property instead of using the number of each phy type you > find you can get into situations where you're writing DWC3_GUSB2PHYCFG, etc, > when there's no phy to go along with it. > Hi Andrew, Thanks for the review. Yes, this decoupling is still there and its fine I believe. > I ran into this when testing on sa8540p-ride, which only uses one of the > ports on the multiport controller. I didn't enable the other phys (not > sure if that was smart or not) and overrode phy-names/phys, but did not > override num-ports/num-ss-ports, which resulted in that. Nothing bad > happened on a quick test.. but I thought I'd highlight that as another > downside of decoupling this value from the number of phys you grab. > If we do not override phy-names or num-ports/num-ss-ports info in DT, they are just defaulted to '1' and as per the current logic only port-1 registers must be configured. Isn't that the case happening ? > Here's a patch enabling sa8540p-ride, I'd love if you'd add it to the > series (probably needs clean up after review, and will definitely need > alteration after you update the dt-binding again). If not I'll continue > to test/review so please CC me!: > > Sure, I can add this patch (probably will add the other phy's too) during the final submission. > From dcb27d07f079194ebd7efe1c9bec64da78beb290 Mon Sep 17 00:00:00 2001 > From: Andrew Halaney <ahalaney@redhat.com> > Date: Thu, 19 Jan 2023 14:53:38 -0600 > Subject: [PATCH] arm64: dts: qcom: sa8540p-ride: Enable usb_2 > Content-type: text/plain > > There is now support for the multiport USB controller this uses > so enable it. > > The board only has a single port hooked up (despite it being wired up to > the multiport IP on the SoC). There's also a USB 2.0 mux hooked up, > which by default on boot is selected to mux properly. Grab the gpio > controlling that and ensure it stays in the right position so USB 2.0 > continues to be routed from the external port to the SoC. > > Signed-off-by: Andrew Halaney <ahalaney@redhat.com> > --- > arch/arm64/boot/dts/qcom/sa8540p-ride.dts | 24 +++++++++++++++++++++++ > 1 file changed, 24 insertions(+) > > diff --git a/arch/arm64/boot/dts/qcom/sa8540p-ride.dts b/arch/arm64/boot/dts/qcom/sa8540p-ride.dts > index 97957f3baa64..56d4f43faa1e 100644 > --- a/arch/arm64/boot/dts/qcom/sa8540p-ride.dts > +++ b/arch/arm64/boot/dts/qcom/sa8540p-ride.dts > @@ -246,6 +246,21 @@ &usb_0_qmpphy { > status = "okay"; > }; > > +&usb_2 { > + pinctrl-names = "default"; > + pinctrl-0 = <&usb2_en_state>; > + > + status = "okay"; > +}; > + > +&usb_2_dwc3 { > + dr_mode = "host"; > + num-ports = <1>; > + num-ss-ports = <1>; More over, if this is a multiport controller and you are using only port-1, it is as good as a single port controller I believe and the normal DT convention must work. Adding these properties as "1" is not required as the driver logic defaults them to "1" if they are not found. Just to add a point here (as I was not clear in DT Binding description, My bad), the num-ports and num-ss-ports must indicate the HS/SS Phys present on HW whether they are used in DT or not. Just to cover all cases which user can use [1]. []1: https://lore.kernel.org/all/4eb26a54-148b-942f-01c6-64e66541de8b@quicinc.com/ Regards, Krishna, > + phy-names = "usb2-phy", "usb3-phy"; > + phys = <&usb_2_hsphy0>, <&usb_2_qmpphy0>; > +}; > + > &usb_2_hsphy0 { > vdda-pll-supply = <&vreg_l5a>; > vdda18-supply = <&vreg_l7g>; > @@ -313,4 +328,13 @@ wake-pins { > bias-pull-up; > }; > }; > + > + usb2_en_state: usb2-en-state { > + /* TS3USB221A USB2.0 mux select */ > + pins = "gpio24"; > + function = "gpio"; > + drive-strength = <2>; > + bias-disable; > + output-low; > + }; > };
On Fri, Jan 20, 2023 at 07:25:57AM +0530, Krishna Kurapati PSSNV wrote: > > > On 1/20/2023 3:39 AM, Andrew Halaney wrote: > > On Sun, Jan 15, 2023 at 05:11:43PM +0530, Krishna Kurapati wrote: > > > Currently the DWC3 driver supports only single port controller > > > which requires at most one HS and one SS PHY. > > > > > > But the DWC3 USB controller can be connected to multiple ports and > > > each port can have their own PHYs. Each port of the multiport > > > controller can either be HS+SS capable or HS only capable > > > Proper quantification of them is required to modify GUSB2PHYCFG > > > and GUSB3PIPECTL registers appropriately. > > > > > > Add support for detecting, obtaining and configuring phy's supported > > > by a multiport controller and limit the max number of ports > > > supported to 4. > > > > > > Signed-off-by: Harsh Agarwal <quic_harshq@quicinc.com> > > > Signed-off-by: Krishna Kurapati <quic_kriskura@quicinc.com> > > > --- > > > drivers/usb/dwc3/core.c | 304 +++++++++++++++++++++++++++++----------- > > > drivers/usb/dwc3/core.h | 15 +- > > > drivers/usb/dwc3/drd.c | 14 +- > > > 3 files changed, 244 insertions(+), 89 deletions(-) > > > > > > diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c > > > index 476b63618511..7e0a9a598dfd 100644 > > > --- a/drivers/usb/dwc3/core.c > > > +++ b/drivers/usb/dwc3/core.c > > > > <snip> > > > > > @@ -1575,6 +1690,21 @@ static void dwc3_get_properties(struct dwc3 *dwc) > > > dwc->dis_split_quirk = device_property_read_bool(dev, > > > "snps,dis-split-quirk"); > > > + > > > + /* > > > + * If no mulitport properties are defined, default > > > + * the port count to '1'. > > > + */ > > > + ret = device_property_read_u32(dev, "num-ports", > > > + &dwc->num_ports); > > > + if (ret) > > > + dwc->num_ports = 1; > > > + > > > + ret = device_property_read_u32(dev, "num-ss-ports", > > > + &dwc->num_ss_ports); > > > + if (ret) > > > + dwc->num_ss_ports = 1; > > > > By using this DT property instead of using the number of each phy type you > > find you can get into situations where you're writing DWC3_GUSB2PHYCFG, etc, > > when there's no phy to go along with it. > > > Hi Andrew, > > Thanks for the review. Yes, this decoupling is still there and its fine I > believe. > > > I ran into this when testing on sa8540p-ride, which only uses one of the > > ports on the multiport controller. I didn't enable the other phys (not > > sure if that was smart or not) and overrode phy-names/phys, but did not > > override num-ports/num-ss-ports, which resulted in that. Nothing bad > > happened on a quick test.. but I thought I'd highlight that as another > > downside of decoupling this value from the number of phys you grab. > > > If we do not override phy-names or num-ports/num-ss-ports info in DT, they > are just defaulted to '1' and as per the current logic only port-1 registers > must be configured. Isn't that the case happening ? > In my dts I'm inheriting from the sc8280xp.dtsi usb_2 phandle you've created! So unless I override them I get this from your sc8280xp.dtsi: + usb_2_dwc3: usb@a400000 { + compatible = "snps,dwc3"; + reg = <0 0x0a400000 0 0xcd00>; + interrupts = <GIC_SPI 133 IRQ_TYPE_LEVEL_HIGH>; + iommus = <&apps_smmu 0x800 0x0>; + num-ports = <4>; + num-ss-ports = <2>; + phys = <&usb_2_hsphy0>, <&usb_2_qmpphy0>, + <&usb_2_hsphy1>, <&usb_2_qmpphy1>, + <&usb_2_hsphy2>, + <&usb_2_hsphy3>; + phy-names = "usb2-phy_port0", "usb3-phy_port0", + "usb2-phy_port1", "usb3-phy_port1", + "usb2-phy_port2", + "usb2-phy_port3"; + }; Since this board only uses one port of the multiport controller, I redefined phys/phy-names to indicate that. I figured that was more desireable than enabling unnecessary phys. Without overriding num-ports/num-ss-ports all the for loops in this patch would act like the values were 4 and 2 respectively, writing to DWC3_GUSB2PHYCFG multiple times etc as well as look for the multiport phy-names and fail to actually get any phys. Hope that makes sense! > > Here's a patch enabling sa8540p-ride, I'd love if you'd add it to the > > series (probably needs clean up after review, and will definitely need > > alteration after you update the dt-binding again). If not I'll continue > > to test/review so please CC me!: > > > > > Sure, I can add this patch (probably will add the other phy's too) during > the final submission. I don't have a great understanding of the mapping of the phys to physical connections (as well as what registers like DWC3_GUSB2PHYCFG do), so if it makes more sense to enable all the relevant SoC phys, write those registers in the DWC3 IP, etc, and only use one of the actual board outputs then feel free. I think this is a good example of "what if a board designer only uses a single port of the multiport IP" imo. > > > From dcb27d07f079194ebd7efe1c9bec64da78beb290 Mon Sep 17 00:00:00 2001 > > From: Andrew Halaney <ahalaney@redhat.com> > > Date: Thu, 19 Jan 2023 14:53:38 -0600 > > Subject: [PATCH] arm64: dts: qcom: sa8540p-ride: Enable usb_2 > > Content-type: text/plain > > > > There is now support for the multiport USB controller this uses > > so enable it. > > > > The board only has a single port hooked up (despite it being wired up to > > the multiport IP on the SoC). There's also a USB 2.0 mux hooked up, > > which by default on boot is selected to mux properly. Grab the gpio > > controlling that and ensure it stays in the right position so USB 2.0 > > continues to be routed from the external port to the SoC. > > > > Signed-off-by: Andrew Halaney <ahalaney@redhat.com> > > --- > > arch/arm64/boot/dts/qcom/sa8540p-ride.dts | 24 +++++++++++++++++++++++ > > 1 file changed, 24 insertions(+) > > > > diff --git a/arch/arm64/boot/dts/qcom/sa8540p-ride.dts b/arch/arm64/boot/dts/qcom/sa8540p-ride.dts > > index 97957f3baa64..56d4f43faa1e 100644 > > --- a/arch/arm64/boot/dts/qcom/sa8540p-ride.dts > > +++ b/arch/arm64/boot/dts/qcom/sa8540p-ride.dts > > @@ -246,6 +246,21 @@ &usb_0_qmpphy { > > status = "okay"; > > }; > > +&usb_2 { > > + pinctrl-names = "default"; > > + pinctrl-0 = <&usb2_en_state>; > > + > > + status = "okay"; > > +}; > > + > > +&usb_2_dwc3 { > > + dr_mode = "host"; > > + num-ports = <1>; > > + num-ss-ports = <1>; > > More over, if this is a multiport controller and you are using only port-1, > it is as good as a single port controller I believe and the normal DT > convention must work. Adding these properties as "1" is not required as the > driver logic defaults them to "1" if they are not found. See above comment about inheriting from sc8280xp.dtsi and needing to override their values. > > Just to add a point here (as I was not clear in DT Binding description, My > bad), the num-ports and num-ss-ports must indicate the HS/SS Phys present on > HW whether they are used in DT or not. Just to cover all cases which user > can use [1]. > > []1: > https://lore.kernel.org/all/4eb26a54-148b-942f-01c6-64e66541de8b@quicinc.com/ Ok, if you're going with that approach of "must indicate the HS/SS Phys present on HW whether they are used in the DT or not" (/me assumes DT here means on the board and not an incorrect coding of the DT) then I suppose I should not have overridden anything but phys/phy-names to indicate that I'm only using the first port (and used the multiport phy-names convention). It looks like in that link you also mention that it is ok to write to DWC3_GUSB2PHYCFG and friends even if the phy isn't defined, which was my concern and reasoning above for overriding num-ports/num-ss-ports. Thanks, Andrew > > Regards, > Krishna, > > > + phy-names = "usb2-phy", "usb3-phy"; > > + phys = <&usb_2_hsphy0>, <&usb_2_qmpphy0>; > > +}; > > + > > &usb_2_hsphy0 { > > vdda-pll-supply = <&vreg_l5a>; > > vdda18-supply = <&vreg_l7g>; > > @@ -313,4 +328,13 @@ wake-pins { > > bias-pull-up; > > }; > > }; > > + > > + usb2_en_state: usb2-en-state { > > + /* TS3USB221A USB2.0 mux select */ > > + pins = "gpio24"; > > + function = "gpio"; > > + drive-strength = <2>; > > + bias-disable; > > + output-low; > > + }; > > }; >
On 1/20/2023 8:07 PM, Andrew Halaney wrote: > On Fri, Jan 20, 2023 at 07:25:57AM +0530, Krishna Kurapati PSSNV wrote: >> >> >> On 1/20/2023 3:39 AM, Andrew Halaney wrote: >>> On Sun, Jan 15, 2023 at 05:11:43PM +0530, Krishna Kurapati wrote: >>>> Currently the DWC3 driver supports only single port controller >>>> which requires at most one HS and one SS PHY. >>>> >>>> But the DWC3 USB controller can be connected to multiple ports and >>>> each port can have their own PHYs. Each port of the multiport >>>> controller can either be HS+SS capable or HS only capable >>>> Proper quantification of them is required to modify GUSB2PHYCFG >>>> and GUSB3PIPECTL registers appropriately. >>>> >>>> Add support for detecting, obtaining and configuring phy's supported >>>> by a multiport controller and limit the max number of ports >>>> supported to 4. >>>> >>>> Signed-off-by: Harsh Agarwal <quic_harshq@quicinc.com> >>>> Signed-off-by: Krishna Kurapati <quic_kriskura@quicinc.com> >>>> --- >>>> drivers/usb/dwc3/core.c | 304 +++++++++++++++++++++++++++++----------- >>>> drivers/usb/dwc3/core.h | 15 +- >>>> drivers/usb/dwc3/drd.c | 14 +- >>>> 3 files changed, 244 insertions(+), 89 deletions(-) >>>> >>>> diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c >>>> index 476b63618511..7e0a9a598dfd 100644 >>>> --- a/drivers/usb/dwc3/core.c >>>> +++ b/drivers/usb/dwc3/core.c >>> >>> <snip> >>> >>>> @@ -1575,6 +1690,21 @@ static void dwc3_get_properties(struct dwc3 *dwc) >>>> dwc->dis_split_quirk = device_property_read_bool(dev, >>>> "snps,dis-split-quirk"); >>>> + >>>> + /* >>>> + * If no mulitport properties are defined, default >>>> + * the port count to '1'. >>>> + */ >>>> + ret = device_property_read_u32(dev, "num-ports", >>>> + &dwc->num_ports); >>>> + if (ret) >>>> + dwc->num_ports = 1; >>>> + >>>> + ret = device_property_read_u32(dev, "num-ss-ports", >>>> + &dwc->num_ss_ports); >>>> + if (ret) >>>> + dwc->num_ss_ports = 1; >>> >>> By using this DT property instead of using the number of each phy type you >>> find you can get into situations where you're writing DWC3_GUSB2PHYCFG, etc, >>> when there's no phy to go along with it. >>> >> Hi Andrew, >> >> Thanks for the review. Yes, this decoupling is still there and its fine I >> believe. >> >>> I ran into this when testing on sa8540p-ride, which only uses one of the >>> ports on the multiport controller. I didn't enable the other phys (not >>> sure if that was smart or not) and overrode phy-names/phys, but did not >>> override num-ports/num-ss-ports, which resulted in that. Nothing bad >>> happened on a quick test.. but I thought I'd highlight that as another >>> downside of decoupling this value from the number of phys you grab. >>> >> If we do not override phy-names or num-ports/num-ss-ports info in DT, they >> are just defaulted to '1' and as per the current logic only port-1 registers >> must be configured. Isn't that the case happening ? >> > > In my dts I'm inheriting from the sc8280xp.dtsi usb_2 phandle you've created! > So unless I override them I get this from your sc8280xp.dtsi: > > + usb_2_dwc3: usb@a400000 { > + compatible = "snps,dwc3"; > + reg = <0 0x0a400000 0 0xcd00>; > + interrupts = <GIC_SPI 133 IRQ_TYPE_LEVEL_HIGH>; > + iommus = <&apps_smmu 0x800 0x0>; > + num-ports = <4>; > + num-ss-ports = <2>; > + phys = <&usb_2_hsphy0>, <&usb_2_qmpphy0>, > + <&usb_2_hsphy1>, <&usb_2_qmpphy1>, > + <&usb_2_hsphy2>, > + <&usb_2_hsphy3>; > + phy-names = "usb2-phy_port0", "usb3-phy_port0", > + "usb2-phy_port1", "usb3-phy_port1", > + "usb2-phy_port2", > + "usb2-phy_port3"; > + }; > > Since this board only uses one port of the multiport controller, I > redefined phys/phy-names to indicate that. I figured that was more > desireable than enabling unnecessary phys. Without overriding > num-ports/num-ss-ports all the for loops in this patch would act like > the values were 4 and 2 respectively, writing to DWC3_GUSB2PHYCFG > multiple times etc as well as look for the multiport phy-names and fail > to actually get any phys. Hope that makes sense! > Hi Andrew, My Bad. I missed the fact that it was based on sc8280xp.dtsi. In that case it makes complete sense to override the num-ports and num-ss-ports to "1" and the usb phy-names. >>> Here's a patch enabling sa8540p-ride, I'd love if you'd add it to the >>> series (probably needs clean up after review, and will definitely need >>> alteration after you update the dt-binding again). If not I'll continue >>> to test/review so please CC me!: >>> >>> >> Sure, I can add this patch (probably will add the other phy's too) during >> the final submission. > > I don't have a great understanding of the mapping of the phys to > physical connections (as well as what registers like DWC3_GUSB2PHYCFG do), > so if it makes more sense to enable all the relevant SoC phys, write > those registers in the DWC3 IP, etc, and only use one of the actual > board outputs then feel free. I think this is a good example of "what if > a board designer only uses a single port of the multiport IP" imo. > Agreed. This could be a good example of multi port with only single port working. >> >>> From dcb27d07f079194ebd7efe1c9bec64da78beb290 Mon Sep 17 00:00:00 2001 >>> From: Andrew Halaney <ahalaney@redhat.com> >>> Date: Thu, 19 Jan 2023 14:53:38 -0600 >>> Subject: [PATCH] arm64: dts: qcom: sa8540p-ride: Enable usb_2 >>> Content-type: text/plain >>> >>> There is now support for the multiport USB controller this uses >>> so enable it. >>> >>> The board only has a single port hooked up (despite it being wired up to >>> the multiport IP on the SoC). There's also a USB 2.0 mux hooked up, >>> which by default on boot is selected to mux properly. Grab the gpio >>> controlling that and ensure it stays in the right position so USB 2.0 >>> continues to be routed from the external port to the SoC. >>> >>> Signed-off-by: Andrew Halaney <ahalaney@redhat.com> >>> --- >>> arch/arm64/boot/dts/qcom/sa8540p-ride.dts | 24 +++++++++++++++++++++++ >>> 1 file changed, 24 insertions(+) >>> >>> diff --git a/arch/arm64/boot/dts/qcom/sa8540p-ride.dts b/arch/arm64/boot/dts/qcom/sa8540p-ride.dts >>> index 97957f3baa64..56d4f43faa1e 100644 >>> --- a/arch/arm64/boot/dts/qcom/sa8540p-ride.dts >>> +++ b/arch/arm64/boot/dts/qcom/sa8540p-ride.dts >>> @@ -246,6 +246,21 @@ &usb_0_qmpphy { >>> status = "okay"; >>> }; >>> +&usb_2 { >>> + pinctrl-names = "default"; >>> + pinctrl-0 = <&usb2_en_state>; >>> + >>> + status = "okay"; >>> +}; >>> + >>> +&usb_2_dwc3 { >>> + dr_mode = "host"; >>> + num-ports = <1>; >>> + num-ss-ports = <1>; >> >> More over, if this is a multiport controller and you are using only port-1, >> it is as good as a single port controller I believe and the normal DT >> convention must work. Adding these properties as "1" is not required as the >> driver logic defaults them to "1" if they are not found. > > See above comment about inheriting from sc8280xp.dtsi and needing to > override their values. > >> >> Just to add a point here (as I was not clear in DT Binding description, My >> bad), the num-ports and num-ss-ports must indicate the HS/SS Phys present on >> HW whether they are used in DT or not. Just to cover all cases which user >> can use [1]. >> >> []1: >> https://lore.kernel.org/all/4eb26a54-148b-942f-01c6-64e66541de8b@quicinc.com/ > > Ok, if you're going with that approach of "must indicate the HS/SS Phys > present on HW whether they are used in the DT or not" (/me assumes DT > here means on the board and not an incorrect coding of the DT) then I > suppose I should not have overridden anything but phys/phy-names to > indicate that I'm only using the first port (and used the multiport > phy-names convention). It looks like in that link you also mention that > it is ok to write to DWC3_GUSB2PHYCFG and friends even if the phy isn't > defined, which was my concern and reasoning above for overriding > num-ports/num-ss-ports. > > Thanks, > Andrew > Actually, I was trying to mandate that rule to take care of cases where the phy's for say port2 or port3 are missing for a quad port controller in dtsi and we don't want to end up configuring wrong dwc3-phy regs. For just the first port, the changes you have mentioned must be sufficient. (Furthermore, thanks for the review and testing it on sa8295-ride and confirming nothing breaks while the first port is enabled) Regards, Krishna, >> >> Regards, >> Krishna, >> >>> + phy-names = "usb2-phy", "usb3-phy"; >>> + phys = <&usb_2_hsphy0>, <&usb_2_qmpphy0>; >>> +}; >>> + >>> &usb_2_hsphy0 { >>> vdda-pll-supply = <&vreg_l5a>; >>> vdda18-supply = <&vreg_l7g>; >>> @@ -313,4 +328,13 @@ wake-pins { >>> bias-pull-up; >>> }; >>> }; >>> + >>> + usb2_en_state: usb2-en-state { >>> + /* TS3USB221A USB2.0 mux select */ >>> + pins = "gpio24"; >>> + function = "gpio"; >>> + drive-strength = <2>; >>> + bias-disable; >>> + output-low; >>> + }; >>> }; >> >
On 1/20/2023 8:43 PM, Krishna Kurapati PSSNV wrote: > > > On 1/20/2023 8:07 PM, Andrew Halaney wrote: >> On Fri, Jan 20, 2023 at 07:25:57AM +0530, Krishna Kurapati PSSNV wrote: >>> >>> >>> On 1/20/2023 3:39 AM, Andrew Halaney wrote: >>>> On Sun, Jan 15, 2023 at 05:11:43PM +0530, Krishna Kurapati wrote: >>>>> Currently the DWC3 driver supports only single port controller >>>>> which requires at most one HS and one SS PHY. >>>>> >>>>> But the DWC3 USB controller can be connected to multiple ports and >>>>> each port can have their own PHYs. Each port of the multiport >>>>> controller can either be HS+SS capable or HS only capable >>>>> Proper quantification of them is required to modify GUSB2PHYCFG >>>>> and GUSB3PIPECTL registers appropriately. >>>>> >>>>> Add support for detecting, obtaining and configuring phy's supported >>>>> by a multiport controller and limit the max number of ports >>>>> supported to 4. >>>>> >>>>> Signed-off-by: Harsh Agarwal <quic_harshq@quicinc.com> >>>>> Signed-off-by: Krishna Kurapati <quic_kriskura@quicinc.com> >>>>> --- >>>>> drivers/usb/dwc3/core.c | 304 >>>>> +++++++++++++++++++++++++++++----------- >>>>> drivers/usb/dwc3/core.h | 15 +- >>>>> drivers/usb/dwc3/drd.c | 14 +- >>>>> 3 files changed, 244 insertions(+), 89 deletions(-) >>>>> >>>>> diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c >>>>> index 476b63618511..7e0a9a598dfd 100644 >>>>> --- a/drivers/usb/dwc3/core.c >>>>> +++ b/drivers/usb/dwc3/core.c >>>> >>>> <snip> >>>> >>>>> @@ -1575,6 +1690,21 @@ static void dwc3_get_properties(struct dwc3 >>>>> *dwc) >>>>> dwc->dis_split_quirk = device_property_read_bool(dev, >>>>> "snps,dis-split-quirk"); >>>>> + >>>>> + /* >>>>> + * If no mulitport properties are defined, default >>>>> + * the port count to '1'. >>>>> + */ >>>>> + ret = device_property_read_u32(dev, "num-ports", >>>>> + &dwc->num_ports); >>>>> + if (ret) >>>>> + dwc->num_ports = 1; >>>>> + >>>>> + ret = device_property_read_u32(dev, "num-ss-ports", >>>>> + &dwc->num_ss_ports); >>>>> + if (ret) >>>>> + dwc->num_ss_ports = 1; >>>> >>>> By using this DT property instead of using the number of each phy >>>> type you >>>> find you can get into situations where you're writing >>>> DWC3_GUSB2PHYCFG, etc, >>>> when there's no phy to go along with it. >>>> >>> Hi Andrew, >>> >>> Thanks for the review. Yes, this decoupling is still there and its >>> fine I >>> believe. >>> >>>> I ran into this when testing on sa8540p-ride, which only uses one of >>>> the >>>> ports on the multiport controller. I didn't enable the other phys (not >>>> sure if that was smart or not) and overrode phy-names/phys, but did not >>>> override num-ports/num-ss-ports, which resulted in that. Nothing bad >>>> happened on a quick test.. but I thought I'd highlight that as another >>>> downside of decoupling this value from the number of phys you grab. >>>> >>> If we do not override phy-names or num-ports/num-ss-ports info in DT, >>> they >>> are just defaulted to '1' and as per the current logic only port-1 >>> registers >>> must be configured. Isn't that the case happening ? >>> >> >> In my dts I'm inheriting from the sc8280xp.dtsi usb_2 phandle you've >> created! >> So unless I override them I get this from your sc8280xp.dtsi: >> >> + usb_2_dwc3: usb@a400000 { >> + compatible = "snps,dwc3"; >> + reg = <0 0x0a400000 0 0xcd00>; >> + interrupts = <GIC_SPI 133 >> IRQ_TYPE_LEVEL_HIGH>; >> + iommus = <&apps_smmu 0x800 0x0>; >> + num-ports = <4>; >> + num-ss-ports = <2>; >> + phys = <&usb_2_hsphy0>, <&usb_2_qmpphy0>, >> + <&usb_2_hsphy1>, >> <&usb_2_qmpphy1>, >> + <&usb_2_hsphy2>, >> + <&usb_2_hsphy3>; >> + phy-names = "usb2-phy_port0", >> "usb3-phy_port0", >> + "usb2-phy_port1", >> "usb3-phy_port1", >> + "usb2-phy_port2", >> + "usb2-phy_port3"; >> + }; >> >> Since this board only uses one port of the multiport controller, I >> redefined phys/phy-names to indicate that. I figured that was more >> desireable than enabling unnecessary phys. Without overriding >> num-ports/num-ss-ports all the for loops in this patch would act like >> the values were 4 and 2 respectively, writing to DWC3_GUSB2PHYCFG >> multiple times etc as well as look for the multiport phy-names and fail >> to actually get any phys. Hope that makes sense! >> > Hi Andrew, > > My Bad. I missed the fact that it was based on sc8280xp.dtsi. In that > case it makes complete sense to override the num-ports and num-ss-ports > to "1" and the usb phy-names. >>>> Here's a patch enabling sa8540p-ride, I'd love if you'd add it to the >>>> series (probably needs clean up after review, and will definitely need >>>> alteration after you update the dt-binding again). If not I'll continue >>>> to test/review so please CC me!: >>>> >>>> >>> Sure, I can add this patch (probably will add the other phy's too) >>> during >>> the final submission. >> >> I don't have a great understanding of the mapping of the phys to >> physical connections (as well as what registers like DWC3_GUSB2PHYCFG >> do), >> so if it makes more sense to enable all the relevant SoC phys, write >> those registers in the DWC3 IP, etc, and only use one of the actual >> board outputs then feel free. I think this is a good example of "what if >> a board designer only uses a single port of the multiport IP" imo. >> Agreed. This could be a good example of multi port with only single port Typo in the previous mail. Correcting it here. > working. Agreed, The dt-patch you provided will be a good working example of getting just a single port working for a multiport controller. Regards, Krishna, >>> >>>> From dcb27d07f079194ebd7efe1c9bec64da78beb290 Mon Sep 17 00:00:00 >>>> 2001 >>>> From: Andrew Halaney <ahalaney@redhat.com> >>>> Date: Thu, 19 Jan 2023 14:53:38 -0600 >>>> Subject: [PATCH] arm64: dts: qcom: sa8540p-ride: Enable usb_2 >>>> Content-type: text/plain >>>> >>>> There is now support for the multiport USB controller this uses >>>> so enable it. >>>> >>>> The board only has a single port hooked up (despite it being wired >>>> up to >>>> the multiport IP on the SoC). There's also a USB 2.0 mux hooked up, >>>> which by default on boot is selected to mux properly. Grab the gpio >>>> controlling that and ensure it stays in the right position so USB 2.0 >>>> continues to be routed from the external port to the SoC. >>>> >>>> Signed-off-by: Andrew Halaney <ahalaney@redhat.com> >>>> --- >>>> arch/arm64/boot/dts/qcom/sa8540p-ride.dts | 24 >>>> +++++++++++++++++++++++ >>>> 1 file changed, 24 insertions(+) >>>> >>>> diff --git a/arch/arm64/boot/dts/qcom/sa8540p-ride.dts >>>> b/arch/arm64/boot/dts/qcom/sa8540p-ride.dts >>>> index 97957f3baa64..56d4f43faa1e 100644 >>>> --- a/arch/arm64/boot/dts/qcom/sa8540p-ride.dts >>>> +++ b/arch/arm64/boot/dts/qcom/sa8540p-ride.dts >>>> @@ -246,6 +246,21 @@ &usb_0_qmpphy { >>>> status = "okay"; >>>> }; >>>> +&usb_2 { >>>> + pinctrl-names = "default"; >>>> + pinctrl-0 = <&usb2_en_state>; >>>> + >>>> + status = "okay"; >>>> +}; >>>> + >>>> +&usb_2_dwc3 { >>>> + dr_mode = "host"; >>>> + num-ports = <1>; >>>> + num-ss-ports = <1>; >>> >>> More over, if this is a multiport controller and you are using only >>> port-1, >>> it is as good as a single port controller I believe and the normal DT >>> convention must work. Adding these properties as "1" is not required >>> as the >>> driver logic defaults them to "1" if they are not found. >> >> See above comment about inheriting from sc8280xp.dtsi and needing to >> override their values. >> >>> >>> Just to add a point here (as I was not clear in DT Binding >>> description, My >>> bad), the num-ports and num-ss-ports must indicate the HS/SS Phys >>> present on >>> HW whether they are used in DT or not. Just to cover all cases which >>> user >>> can use [1]. >>> >>> []1: >>> https://lore.kernel.org/all/4eb26a54-148b-942f-01c6-64e66541de8b@quicinc.com/ >>> >> >> Ok, if you're going with that approach of "must indicate the HS/SS Phys >> present on HW whether they are used in the DT or not" (/me assumes DT >> here means on the board and not an incorrect coding of the DT) then I >> suppose I should not have overridden anything but phys/phy-names to >> indicate that I'm only using the first port (and used the multiport >> phy-names convention). It looks like in that link you also mention that >> it is ok to write to DWC3_GUSB2PHYCFG and friends even if the phy isn't >> defined, which was my concern and reasoning above for overriding >> num-ports/num-ss-ports. >> >> Thanks, >> Andrew >> > Actually, I was trying to mandate that rule to take care of cases where > the phy's for say port2 or port3 are missing for a quad port controller > in dtsi and we don't want to end up configuring wrong dwc3-phy regs. > > For just the first port, the changes you have mentioned must be > sufficient. (Furthermore, thanks for the review and testing it on > sa8295-ride and confirming nothing breaks while the first port is enabled) > > Regards, > Krishna, >>> >>> Regards, >>> Krishna, >>> >>>> + phy-names = "usb2-phy", "usb3-phy"; >>>> + phys = <&usb_2_hsphy0>, <&usb_2_qmpphy0>; >>>> +}; >>>> + >>>> &usb_2_hsphy0 { >>>> vdda-pll-supply = <&vreg_l5a>; >>>> vdda18-supply = <&vreg_l7g>; >>>> @@ -313,4 +328,13 @@ wake-pins { >>>> bias-pull-up; >>>> }; >>>> }; >>>> + >>>> + usb2_en_state: usb2-en-state { >>>> + /* TS3USB221A USB2.0 mux select */ >>>> + pins = "gpio24"; >>>> + function = "gpio"; >>>> + drive-strength = <2>; >>>> + bias-disable; >>>> + output-low; >>>> + }; >>>> }; >>> >>
On Fri, Jan 20, 2023, Krishna Kurapati PSSNV wrote: > > > On 1/20/2023 6:32 AM, Thinh Nguyen wrote: > > Hi, > > > > On Thu, Jan 19, 2023, Krishna Kurapati PSSNV wrote: > > > > > > > > > On 1/19/2023 6:06 AM, Thinh Nguyen wrote: > > > > Hi, > > > > > > > > On Sun, Jan 15, 2023, Krishna Kurapati wrote: > > > > > Currently the DWC3 driver supports only single port controller > > > > > which requires at most one HS and one SS PHY. > > > > > > > > Add note here that multi-port is for host mode for clarity. > > > > > > > > > > > > > > But the DWC3 USB controller can be connected to multiple ports and > > > > > each port can have their own PHYs. Each port of the multiport > > > > > controller can either be HS+SS capable or HS only capable > > > > > Proper quantification of them is required to modify GUSB2PHYCFG > > > > > and GUSB3PIPECTL registers appropriately. > > > > > > > > > > Add support for detecting, obtaining and configuring phy's supported > > > > > by a multiport controller and limit the max number of ports > > > > > supported to 4. > > > > > > > > > > Signed-off-by: Harsh Agarwal <quic_harshq@quicinc.com> > > > > > Signed-off-by: Krishna Kurapati <quic_kriskura@quicinc.com> > > > > > --- > > > > > drivers/usb/dwc3/core.c | 304 +++++++++++++++++++++++++++++----------- > > > > > drivers/usb/dwc3/core.h | 15 +- > > > > > drivers/usb/dwc3/drd.c | 14 +- > > > > > 3 files changed, 244 insertions(+), 89 deletions(-) > > > > > > > > > <snip> > > > > > > > @@ -1575,6 +1690,21 @@ static void dwc3_get_properties(struct dwc3 *dwc) > > > > > dwc->dis_split_quirk = device_property_read_bool(dev, > > > > > "snps,dis-split-quirk"); > > > > > + > > > > > + /* > > > > > + * If no mulitport properties are defined, default > > > > > > > > multi* > > > > > > > > > + * the port count to '1'. > > > > > + */ > > > > > > > > Can we initialize num_ports and num_ss_ports to 1 instead of setting it > > > > on error (similar to how we handle other properties). > > > > > > > Hi Thinh, > > > > > > Thanks for the review. On the bindings, Rob and Krzysztof have suggested > > > to get the num-ports and num-ss-ports by counting the Phy-names in DT. > > > > This may be a bit problematic for non-DT device. Currently pci devices > > pass fake DT properties to send these kinds of info. But that's fine, > > we can enhance dwc3 for non-DT devices later. > > > > > > > > Since there may be many cases where the user might skip giving any Phy's or > > > even skip different ports in the DT if he doesn't want to use them, can we > > > design/refactor the below logic as follows while mandating the fact that > > > user must give the SS Phy's if any starting from Port-0.: > > > > > > num-ss-ports = max_port_index (usb3-portX) + 1 > > > num-ports = max (max_port_index(usb2-portX), num-ss-ports) + 1 > > > > > > Ex: If there are 3 ports and only 1 is SS capable and user decides to skip > > > port-2 HS Phy. > > > > > > case-1: phy-names = "usb2-port0", "usb3-port0", "usb2-port-1" > > > case-2: phy-names = "usb2-port0", "usb2-port-1", "usb3-port1" > > > > > > In both cases, only one SS is present, just the order is changed. (Not sure > > > if last few ports can be made SS Capable instead of the first ports on any > > > HW) ? > > > > > > But according to the above formula: > > > > > > In case-1 : (num-ports = 2, num-ss-ports = 1) - This is correct > > > In case-2: (num-ports = 2, num-ss-ports = 2) - This is wrong > > > > > > > Can't we just walk through all the phy names to figure that out? Let's > > not require the user to specify Port-0 is SS capable if they can skip > > it. > > > Hi Thinh, > > Thanks for the review. > > May be I wasn't able to convey my intention properly in my previous > thread. The above suggested method doesn't tell that user must not skip any > phy. > > Let us take the below case for 2 Port (HS+SS) capable controller. > If the user skips ss-phy 2, then: > > phy-names = "usb2-port0", "usb3-port0", "usb2-port-1" > > We don't need to configure GUSB3PIPECTL2 (for port-2) register ere. If we > parse phy-names and find it out, we see there are 2 hs-phy's and 1-ssphy and > num-ports = 2 and num-ss-ports = 1. > > If the user skips ss-phy-1, then phy-names would be something like the > below: > > phy-names = "usb2-port0", "usb2-port-1", "usb3-port1"; > > We need to handle two types of interpretations here while parsing the > phy-names: > > a) There are 2 SS Phy's and we just skipped the first one. In this scenario, > if we consider (num-ss-ports = 2) and (num-ports = 2) by using the above > formula as reference, we configure both GUSB3PIPECTL registers present in > the address map although we never use SS Phy-1 but nothing must break. All > ports would still work as the user intends with the exception of > GUSB3PIPECTL1 (for-port1) still being modified. > > b) The second interpretation is something like, port-1 is only HS capable > and only Port-2 is SS Capable, and so in the phy-names only port-2 has been > provided a SS Phy. Just by parsing through the phy-names, it would not be > possible to get that info. So if we consider num-ss-ports as 2 in this > scenario, we end up meddling with wrong registers (as there is only 1 > GUSB3PIPECTL reg and we are assuing there are 2). I wanted to make sure that > this scenario was not possible. > > num-ss-ports = max_port_index (usb3-portX) + 1 > num-ports = max (max_port_index(usb2-portX), max_port_index(usb2-portX)) + 1 > > Taking case of a quad port controller with all ports SS Capable, if the user > wants to completely skip port-4. Then with above formula, we get (num-ports > = 3) and (num-ss-ports = 3) by parsing the phy-names and we will configure > exactly those dwc3-phy registers and not touch the port-4 registers which is > still fine. > > Please let me know if the above idea helps us in this scenario. > This becomes rather more complicated because the user can skip certain port in the DT. We have access to the host registers. Can we just temporarily map and access HCSPARAMS1 to get the MAXPORTS and each port capability before handing control over to the xHCI driver. We would be able to get the num_ports and num_ss_ports then. Similarly, the xhci driver doesn't care whether the user skips certain port in the DT, it only checks and operates based on the capability registers. If we have the exact num_ports and num_ss_ports, we can be sure the setting to GUSB3PIPECTLn and GUSB2PHYCFGn are valid. Thanks, Thinh
On Thu, Jan 19, 2023, Thinh Nguyen wrote: > Hi, > > On Sun, Jan 15, 2023, Krishna Kurapati wrote: > > Currently the DWC3 driver supports only single port controller > > which requires at most one HS and one SS PHY. > > Add note here that multi-port is for host mode for clarity. > > > > > But the DWC3 USB controller can be connected to multiple ports and > > each port can have their own PHYs. Each port of the multiport > > controller can either be HS+SS capable or HS only capable > > Proper quantification of them is required to modify GUSB2PHYCFG > > and GUSB3PIPECTL registers appropriately. > > > > Add support for detecting, obtaining and configuring phy's supported > > by a multiport controller and limit the max number of ports > > supported to 4. > > > > Signed-off-by: Harsh Agarwal <quic_harshq@quicinc.com> > > Signed-off-by: Krishna Kurapati <quic_kriskura@quicinc.com> > > --- > > drivers/usb/dwc3/core.c | 304 +++++++++++++++++++++++++++++----------- > > drivers/usb/dwc3/core.h | 15 +- > > drivers/usb/dwc3/drd.c | 14 +- > > 3 files changed, 244 insertions(+), 89 deletions(-) > > > > diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c > > index 476b63618511..7e0a9a598dfd 100644 > > --- a/drivers/usb/dwc3/core.c > > +++ b/drivers/usb/dwc3/core.c > > @@ -120,7 +120,7 @@ static void __dwc3_set_mode(struct work_struct *work) > > { > > struct dwc3 *dwc = work_to_dwc(work); > > unsigned long flags; > > - int ret; > > + int ret, i; > > Can we declare variables in separate lines here and other places. > > > u32 reg; > > u32 desired_dr_role; > > > > @@ -200,8 +200,10 @@ static void __dwc3_set_mode(struct work_struct *work) > > } else { > > if (dwc->usb2_phy) > > otg_set_vbus(dwc->usb2_phy->otg, true); > > - phy_set_mode(dwc->usb2_generic_phy, PHY_MODE_USB_HOST); > > - phy_set_mode(dwc->usb3_generic_phy, PHY_MODE_USB_HOST); > > + for (i = 0; i < dwc->num_ports; i++) { BTW, is num_ports the total of usb2 + usb3 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 (dwc->dis_split_quirk) { Thanks, Thinh
On 1/21/2023 4:27 AM, Thinh Nguyen wrote: > On Thu, Jan 19, 2023, Thinh Nguyen wrote: >> Hi, >> >> On Sun, Jan 15, 2023, Krishna Kurapati wrote: >>> Currently the DWC3 driver supports only single port controller >>> which requires at most one HS and one SS PHY. >> >> Add note here that multi-port is for host mode for clarity. >> >>> >>> But the DWC3 USB controller can be connected to multiple ports and >>> each port can have their own PHYs. Each port of the multiport >>> controller can either be HS+SS capable or HS only capable >>> Proper quantification of them is required to modify GUSB2PHYCFG >>> and GUSB3PIPECTL registers appropriately. >>> >>> Add support for detecting, obtaining and configuring phy's supported >>> by a multiport controller and limit the max number of ports >>> supported to 4. >>> >>> Signed-off-by: Harsh Agarwal <quic_harshq@quicinc.com> >>> Signed-off-by: Krishna Kurapati <quic_kriskura@quicinc.com> >>> --- >>> drivers/usb/dwc3/core.c | 304 +++++++++++++++++++++++++++++----------- >>> drivers/usb/dwc3/core.h | 15 +- >>> drivers/usb/dwc3/drd.c | 14 +- >>> 3 files changed, 244 insertions(+), 89 deletions(-) >>> >>> diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c >>> index 476b63618511..7e0a9a598dfd 100644 >>> --- a/drivers/usb/dwc3/core.c >>> +++ b/drivers/usb/dwc3/core.c >>> @@ -120,7 +120,7 @@ static void __dwc3_set_mode(struct work_struct *work) >>> { >>> struct dwc3 *dwc = work_to_dwc(work); >>> unsigned long flags; >>> - int ret; >>> + int ret, i; >> >> Can we declare variables in separate lines here and other places. >> >>> u32 reg; >>> u32 desired_dr_role; >>> >>> @@ -200,8 +200,10 @@ static void __dwc3_set_mode(struct work_struct *work) >>> } else { >>> if (dwc->usb2_phy) >>> otg_set_vbus(dwc->usb2_phy->otg, true); >>> - phy_set_mode(dwc->usb2_generic_phy, PHY_MODE_USB_HOST); >>> - phy_set_mode(dwc->usb3_generic_phy, PHY_MODE_USB_HOST); >>> + for (i = 0; i < dwc->num_ports; i++) { > > BTW, is num_ports the total of usb2 + usb3 ports? Hi Thinh, No, num_ports is just the total number of hw usb ports present (presuming each port is hs capable, this is just the number of HS Phy's available). Regards, KRishna, > >>> + 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 (dwc->dis_split_quirk) { > > Thanks, > Thinh
On 1/21/2023 4:14 AM, Thinh Nguyen wrote: > On Fri, Jan 20, 2023, Krishna Kurapati PSSNV wrote: >> >> >> On 1/20/2023 6:32 AM, Thinh Nguyen wrote: >>> Hi, >>> >>> On Thu, Jan 19, 2023, Krishna Kurapati PSSNV wrote: >>>> >>>> >>>> On 1/19/2023 6:06 AM, Thinh Nguyen wrote: >>>>> Hi, >>>>> >>>>> On Sun, Jan 15, 2023, Krishna Kurapati wrote: >>>>>> Currently the DWC3 driver supports only single port controller >>>>>> which requires at most one HS and one SS PHY. >>>>> >>>>> Add note here that multi-port is for host mode for clarity. >>>>> >>>>>> >>>>>> But the DWC3 USB controller can be connected to multiple ports and >>>>>> each port can have their own PHYs. Each port of the multiport >>>>>> controller can either be HS+SS capable or HS only capable >>>>>> Proper quantification of them is required to modify GUSB2PHYCFG >>>>>> and GUSB3PIPECTL registers appropriately. >>>>>> >>>>>> Add support for detecting, obtaining and configuring phy's supported >>>>>> by a multiport controller and limit the max number of ports >>>>>> supported to 4. >>>>>> >>>>>> Signed-off-by: Harsh Agarwal <quic_harshq@quicinc.com> >>>>>> Signed-off-by: Krishna Kurapati <quic_kriskura@quicinc.com> >>>>>> --- >>>>>> drivers/usb/dwc3/core.c | 304 +++++++++++++++++++++++++++++----------- >>>>>> drivers/usb/dwc3/core.h | 15 +- >>>>>> drivers/usb/dwc3/drd.c | 14 +- >>>>>> 3 files changed, 244 insertions(+), 89 deletions(-) >>>>>> >>> >>> <snip> >>> >>>>>> @@ -1575,6 +1690,21 @@ static void dwc3_get_properties(struct dwc3 *dwc) >>>>>> dwc->dis_split_quirk = device_property_read_bool(dev, >>>>>> "snps,dis-split-quirk"); >>>>>> + >>>>>> + /* >>>>>> + * If no mulitport properties are defined, default >>>>> >>>>> multi* >>>>> >>>>>> + * the port count to '1'. >>>>>> + */ >>>>> >>>>> Can we initialize num_ports and num_ss_ports to 1 instead of setting it >>>>> on error (similar to how we handle other properties). >>>>> >>>> Hi Thinh, >>>> >>>> Thanks for the review. On the bindings, Rob and Krzysztof have suggested >>>> to get the num-ports and num-ss-ports by counting the Phy-names in DT. >>> >>> This may be a bit problematic for non-DT device. Currently pci devices >>> pass fake DT properties to send these kinds of info. But that's fine, >>> we can enhance dwc3 for non-DT devices later. >>> >>>> >>>> Since there may be many cases where the user might skip giving any Phy's or >>>> even skip different ports in the DT if he doesn't want to use them, can we >>>> design/refactor the below logic as follows while mandating the fact that >>>> user must give the SS Phy's if any starting from Port-0.: >>>> >>>> num-ss-ports = max_port_index (usb3-portX) + 1 >>>> num-ports = max (max_port_index(usb2-portX), num-ss-ports) + 1 >>>> >>>> Ex: If there are 3 ports and only 1 is SS capable and user decides to skip >>>> port-2 HS Phy. >>>> >>>> case-1: phy-names = "usb2-port0", "usb3-port0", "usb2-port-1" >>>> case-2: phy-names = "usb2-port0", "usb2-port-1", "usb3-port1" >>>> >>>> In both cases, only one SS is present, just the order is changed. (Not sure >>>> if last few ports can be made SS Capable instead of the first ports on any >>>> HW) ? >>>> >>>> But according to the above formula: >>>> >>>> In case-1 : (num-ports = 2, num-ss-ports = 1) - This is correct >>>> In case-2: (num-ports = 2, num-ss-ports = 2) - This is wrong >>>> >>> >>> Can't we just walk through all the phy names to figure that out? Let's >>> not require the user to specify Port-0 is SS capable if they can skip >>> it. >>> >> Hi Thinh, >> >> Thanks for the review. >> >> May be I wasn't able to convey my intention properly in my previous >> thread. The above suggested method doesn't tell that user must not skip any >> phy. >> >> Let us take the below case for 2 Port (HS+SS) capable controller. >> If the user skips ss-phy 2, then: >> >> phy-names = "usb2-port0", "usb3-port0", "usb2-port-1" >> >> We don't need to configure GUSB3PIPECTL2 (for port-2) register ere. If we >> parse phy-names and find it out, we see there are 2 hs-phy's and 1-ssphy and >> num-ports = 2 and num-ss-ports = 1. >> >> If the user skips ss-phy-1, then phy-names would be something like the >> below: >> >> phy-names = "usb2-port0", "usb2-port-1", "usb3-port1"; >> >> We need to handle two types of interpretations here while parsing the >> phy-names: >> >> a) There are 2 SS Phy's and we just skipped the first one. In this scenario, >> if we consider (num-ss-ports = 2) and (num-ports = 2) by using the above >> formula as reference, we configure both GUSB3PIPECTL registers present in >> the address map although we never use SS Phy-1 but nothing must break. All >> ports would still work as the user intends with the exception of >> GUSB3PIPECTL1 (for-port1) still being modified. >> >> b) The second interpretation is something like, port-1 is only HS capable >> and only Port-2 is SS Capable, and so in the phy-names only port-2 has been >> provided a SS Phy. Just by parsing through the phy-names, it would not be >> possible to get that info. So if we consider num-ss-ports as 2 in this >> scenario, we end up meddling with wrong registers (as there is only 1 >> GUSB3PIPECTL reg and we are assuing there are 2). I wanted to make sure that >> this scenario was not possible. >> >> num-ss-ports = max_port_index (usb3-portX) + 1 >> num-ports = max (max_port_index(usb2-portX), max_port_index(usb2-portX)) + 1 >> >> Taking case of a quad port controller with all ports SS Capable, if the user >> wants to completely skip port-4. Then with above formula, we get (num-ports >> = 3) and (num-ss-ports = 3) by parsing the phy-names and we will configure >> exactly those dwc3-phy registers and not touch the port-4 registers which is >> still fine. >> >> Please let me know if the above idea helps us in this scenario. >> > > This becomes rather more complicated because the user can skip certain > port in the DT. We have access to the host registers. Can we just > temporarily map and access HCSPARAMS1 to get the MAXPORTS and each port > capability before handing control over to the xHCI driver. We would be > able to get the num_ports and num_ss_ports then. > > Similarly, the xhci driver doesn't care whether the user skips certain > port in the DT, it only checks and operates based on the capability > registers. > > If we have the exact num_ports and num_ss_ports, we can be sure the > setting to GUSB3PIPECTLn and GUSB2PHYCFGn are valid. > Hi Thinh, Thanks for the review. Sure, I can explore this option and get the port info. This must make things easier. Regards, Krishna,
On Sat, Jan 21, 2023, Krishna Kurapati PSSNV wrote: > > > On 1/21/2023 4:27 AM, Thinh Nguyen wrote: > > On Thu, Jan 19, 2023, Thinh Nguyen wrote: > > > Hi, > > > > > > On Sun, Jan 15, 2023, Krishna Kurapati wrote: > > > > Currently the DWC3 driver supports only single port controller > > > > which requires at most one HS and one SS PHY. > > > > > > Add note here that multi-port is for host mode for clarity. > > > > > > > > > > > But the DWC3 USB controller can be connected to multiple ports and > > > > each port can have their own PHYs. Each port of the multiport > > > > controller can either be HS+SS capable or HS only capable > > > > Proper quantification of them is required to modify GUSB2PHYCFG > > > > and GUSB3PIPECTL registers appropriately. > > > > > > > > Add support for detecting, obtaining and configuring phy's supported > > > > by a multiport controller and limit the max number of ports > > > > supported to 4. > > > > > > > > Signed-off-by: Harsh Agarwal <quic_harshq@quicinc.com> > > > > Signed-off-by: Krishna Kurapati <quic_kriskura@quicinc.com> > > > > --- > > > > drivers/usb/dwc3/core.c | 304 +++++++++++++++++++++++++++++----------- > > > > drivers/usb/dwc3/core.h | 15 +- > > > > drivers/usb/dwc3/drd.c | 14 +- > > > > 3 files changed, 244 insertions(+), 89 deletions(-) > > > > > > > > diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c > > > > index 476b63618511..7e0a9a598dfd 100644 > > > > --- a/drivers/usb/dwc3/core.c > > > > +++ b/drivers/usb/dwc3/core.c > > > > @@ -120,7 +120,7 @@ static void __dwc3_set_mode(struct work_struct *work) > > > > { > > > > struct dwc3 *dwc = work_to_dwc(work); > > > > unsigned long flags; > > > > - int ret; > > > > + int ret, i; > > > > > > Can we declare variables in separate lines here and other places. > > > > > > > u32 reg; > > > > u32 desired_dr_role; > > > > @@ -200,8 +200,10 @@ static void __dwc3_set_mode(struct work_struct *work) > > > > } else { > > > > if (dwc->usb2_phy) > > > > otg_set_vbus(dwc->usb2_phy->otg, true); > > > > - phy_set_mode(dwc->usb2_generic_phy, PHY_MODE_USB_HOST); > > > > - phy_set_mode(dwc->usb3_generic_phy, PHY_MODE_USB_HOST); > > > > + for (i = 0; i < dwc->num_ports; i++) { > > > > BTW, is num_ports the total of usb2 + usb3 ports? > Hi Thinh, > > No, num_ports is just the total number of hw usb ports present (presuming > each port is hs capable, this is just the number of HS Phy's available). > I see, thanks. Can you also make this clear in its description. I got mixed up at some point for the equivalent of HCPARAMS1.MAXPORTS. Thanks, Thinh
On 1/21/2023 7:49 AM, Thinh Nguyen wrote: > On Sat, Jan 21, 2023, Krishna Kurapati PSSNV wrote: >> >> >> On 1/21/2023 4:27 AM, Thinh Nguyen wrote: >>> On Thu, Jan 19, 2023, Thinh Nguyen wrote: >>>> Hi, >>>> >>>> On Sun, Jan 15, 2023, Krishna Kurapati wrote: >>>>> Currently the DWC3 driver supports only single port controller >>>>> which requires at most one HS and one SS PHY. >>>> >>>> Add note here that multi-port is for host mode for clarity. >>>> >>>>> >>>>> But the DWC3 USB controller can be connected to multiple ports and >>>>> each port can have their own PHYs. Each port of the multiport >>>>> controller can either be HS+SS capable or HS only capable >>>>> Proper quantification of them is required to modify GUSB2PHYCFG >>>>> and GUSB3PIPECTL registers appropriately. >>>>> >>>>> Add support for detecting, obtaining and configuring phy's supported >>>>> by a multiport controller and limit the max number of ports >>>>> supported to 4. >>>>> >>>>> Signed-off-by: Harsh Agarwal <quic_harshq@quicinc.com> >>>>> Signed-off-by: Krishna Kurapati <quic_kriskura@quicinc.com> >>>>> --- >>>>> drivers/usb/dwc3/core.c | 304 +++++++++++++++++++++++++++++----------- >>>>> drivers/usb/dwc3/core.h | 15 +- >>>>> drivers/usb/dwc3/drd.c | 14 +- >>>>> 3 files changed, 244 insertions(+), 89 deletions(-) >>>>> >>>>> diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c >>>>> index 476b63618511..7e0a9a598dfd 100644 >>>>> --- a/drivers/usb/dwc3/core.c >>>>> +++ b/drivers/usb/dwc3/core.c >>>>> @@ -120,7 +120,7 @@ static void __dwc3_set_mode(struct work_struct *work) >>>>> { >>>>> struct dwc3 *dwc = work_to_dwc(work); >>>>> unsigned long flags; >>>>> - int ret; >>>>> + int ret, i; >>>> >>>> Can we declare variables in separate lines here and other places. >>>> >>>>> u32 reg; >>>>> u32 desired_dr_role; >>>>> @@ -200,8 +200,10 @@ static void __dwc3_set_mode(struct work_struct *work) >>>>> } else { >>>>> if (dwc->usb2_phy) >>>>> otg_set_vbus(dwc->usb2_phy->otg, true); >>>>> - phy_set_mode(dwc->usb2_generic_phy, PHY_MODE_USB_HOST); >>>>> - phy_set_mode(dwc->usb3_generic_phy, PHY_MODE_USB_HOST); >>>>> + for (i = 0; i < dwc->num_ports; i++) { >>> >>> BTW, is num_ports the total of usb2 + usb3 ports? >> Hi Thinh, >> >> No, num_ports is just the total number of hw usb ports present (presuming >> each port is hs capable, this is just the number of HS Phy's available). >> > > I see, thanks. Can you also make this clear in its description. I got > mixed up at some point for the equivalent of HCPARAMS1.MAXPORTS. > > Thanks, > Thinh Sure, Will add this to commit text. But as you rightly mentioned, HCSPARAMS1 gives the total number of HS+SS Phy's available (HCSPARAMS1.MAXPORTS). Is there a way to segregate this value (to just number of hs and ss). Regards, Krishna,
On Sat, Jan 21, 2023, Krishna Kurapati PSSNV wrote: > > > On 1/21/2023 7:49 AM, Thinh Nguyen wrote: > > On Sat, Jan 21, 2023, Krishna Kurapati PSSNV wrote: > > > > > > > > > On 1/21/2023 4:27 AM, Thinh Nguyen wrote: > > > > On Thu, Jan 19, 2023, Thinh Nguyen wrote: > > > > > Hi, > > > > > > > > > > On Sun, Jan 15, 2023, Krishna Kurapati wrote: > > > > > > Currently the DWC3 driver supports only single port controller > > > > > > which requires at most one HS and one SS PHY. > > > > > > > > > > Add note here that multi-port is for host mode for clarity. > > > > > > > > > > > > > > > > > But the DWC3 USB controller can be connected to multiple ports and > > > > > > each port can have their own PHYs. Each port of the multiport > > > > > > controller can either be HS+SS capable or HS only capable > > > > > > Proper quantification of them is required to modify GUSB2PHYCFG > > > > > > and GUSB3PIPECTL registers appropriately. > > > > > > > > > > > > Add support for detecting, obtaining and configuring phy's supported > > > > > > by a multiport controller and limit the max number of ports > > > > > > supported to 4. > > > > > > > > > > > > Signed-off-by: Harsh Agarwal <quic_harshq@quicinc.com> > > > > > > Signed-off-by: Krishna Kurapati <quic_kriskura@quicinc.com> > > > > > > --- > > > > > > drivers/usb/dwc3/core.c | 304 +++++++++++++++++++++++++++++----------- > > > > > > drivers/usb/dwc3/core.h | 15 +- > > > > > > drivers/usb/dwc3/drd.c | 14 +- > > > > > > 3 files changed, 244 insertions(+), 89 deletions(-) > > > > > > > > > > > > diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c > > > > > > index 476b63618511..7e0a9a598dfd 100644 > > > > > > --- a/drivers/usb/dwc3/core.c > > > > > > +++ b/drivers/usb/dwc3/core.c > > > > > > @@ -120,7 +120,7 @@ static void __dwc3_set_mode(struct work_struct *work) > > > > > > { > > > > > > struct dwc3 *dwc = work_to_dwc(work); > > > > > > unsigned long flags; > > > > > > - int ret; > > > > > > + int ret, i; > > > > > > > > > > Can we declare variables in separate lines here and other places. > > > > > > > > > > > u32 reg; > > > > > > u32 desired_dr_role; > > > > > > @@ -200,8 +200,10 @@ static void __dwc3_set_mode(struct work_struct *work) > > > > > > } else { > > > > > > if (dwc->usb2_phy) > > > > > > otg_set_vbus(dwc->usb2_phy->otg, true); > > > > > > - phy_set_mode(dwc->usb2_generic_phy, PHY_MODE_USB_HOST); > > > > > > - phy_set_mode(dwc->usb3_generic_phy, PHY_MODE_USB_HOST); > > > > > > + for (i = 0; i < dwc->num_ports; i++) { > > > > > > > > BTW, is num_ports the total of usb2 + usb3 ports? > > > Hi Thinh, > > > > > > No, num_ports is just the total number of hw usb ports present (presuming > > > each port is hs capable, this is just the number of HS Phy's available). > > > > > > > I see, thanks. Can you also make this clear in its description. I got > > mixed up at some point for the equivalent of HCPARAMS1.MAXPORTS. > > > > Thanks, > > Thinh > > Sure, Will add this to commit text. > But as you rightly mentioned, HCSPARAMS1 gives the total number of HS+SS > Phy's available (HCSPARAMS1.MAXPORTS). Is there a way to segregate this > value (to just number of hs and ss). > We need to walk through each port and check its capability, and we can check the port's major/minor revision to determine whether it's SS capable. See xhci driver's logic and how it calls xhci_add_in_port(). Thanks, Thinh
On 1/20/2023 8:48 PM, Krishna Kurapati PSSNV wrote: > > > On 1/20/2023 8:43 PM, Krishna Kurapati PSSNV wrote: >> >> >> On 1/20/2023 8:07 PM, Andrew Halaney wrote: >>> On Fri, Jan 20, 2023 at 07:25:57AM +0530, Krishna Kurapati PSSNV wrote: >>>> >>>> >>>> On 1/20/2023 3:39 AM, Andrew Halaney wrote: >>>>> On Sun, Jan 15, 2023 at 05:11:43PM +0530, Krishna Kurapati wrote: >>>>>> Currently the DWC3 driver supports only single port controller >>>>>> which requires at most one HS and one SS PHY. >>>>>> >>>>>> But the DWC3 USB controller can be connected to multiple ports and >>>>>> each port can have their own PHYs. Each port of the multiport >>>>>> controller can either be HS+SS capable or HS only capable >>>>>> Proper quantification of them is required to modify GUSB2PHYCFG >>>>>> and GUSB3PIPECTL registers appropriately. >>>>>> >>>>>> Add support for detecting, obtaining and configuring phy's supported >>>>>> by a multiport controller and limit the max number of ports >>>>>> supported to 4. >>>>>> >>>>>> Signed-off-by: Harsh Agarwal <quic_harshq@quicinc.com> >>>>>> Signed-off-by: Krishna Kurapati <quic_kriskura@quicinc.com> >>>>>> --- >>>>>> drivers/usb/dwc3/core.c | 304 >>>>>> +++++++++++++++++++++++++++++----------- >>>>>> drivers/usb/dwc3/core.h | 15 +- >>>>>> drivers/usb/dwc3/drd.c | 14 +- >>>>>> 3 files changed, 244 insertions(+), 89 deletions(-) >>>>>> >>>>>> diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c >>>>>> index 476b63618511..7e0a9a598dfd 100644 >>>>>> --- a/drivers/usb/dwc3/core.c >>>>>> +++ b/drivers/usb/dwc3/core.c >>>>> >>>>> <snip> >>>>> >>>>>> @@ -1575,6 +1690,21 @@ static void dwc3_get_properties(struct dwc3 >>>>>> *dwc) >>>>>> dwc->dis_split_quirk = device_property_read_bool(dev, >>>>>> "snps,dis-split-quirk"); >>>>>> + >>>>>> + /* >>>>>> + * If no mulitport properties are defined, default >>>>>> + * the port count to '1'. >>>>>> + */ >>>>>> + ret = device_property_read_u32(dev, "num-ports", >>>>>> + &dwc->num_ports); >>>>>> + if (ret) >>>>>> + dwc->num_ports = 1; >>>>>> + >>>>>> + ret = device_property_read_u32(dev, "num-ss-ports", >>>>>> + &dwc->num_ss_ports); >>>>>> + if (ret) >>>>>> + dwc->num_ss_ports = 1; >>>>> >>>>> By using this DT property instead of using the number of each phy >>>>> type you >>>>> find you can get into situations where you're writing >>>>> DWC3_GUSB2PHYCFG, etc, >>>>> when there's no phy to go along with it. >>>>> >>>> Hi Andrew, >>>> >>>> Thanks for the review. Yes, this decoupling is still there and its >>>> fine I >>>> believe. >>>> >>>>> I ran into this when testing on sa8540p-ride, which only uses one >>>>> of the >>>>> ports on the multiport controller. I didn't enable the other phys (not >>>>> sure if that was smart or not) and overrode phy-names/phys, but did >>>>> not >>>>> override num-ports/num-ss-ports, which resulted in that. Nothing bad >>>>> happened on a quick test.. but I thought I'd highlight that as another >>>>> downside of decoupling this value from the number of phys you grab. >>>>> >>>> If we do not override phy-names or num-ports/num-ss-ports info in >>>> DT, they >>>> are just defaulted to '1' and as per the current logic only port-1 >>>> registers >>>> must be configured. Isn't that the case happening ? >>>> >>> >>> In my dts I'm inheriting from the sc8280xp.dtsi usb_2 phandle you've >>> created! >>> So unless I override them I get this from your sc8280xp.dtsi: >>> >>> + usb_2_dwc3: usb@a400000 { >>> + compatible = "snps,dwc3"; >>> + reg = <0 0x0a400000 0 0xcd00>; >>> + interrupts = <GIC_SPI 133 >>> IRQ_TYPE_LEVEL_HIGH>; >>> + iommus = <&apps_smmu 0x800 0x0>; >>> + num-ports = <4>; >>> + num-ss-ports = <2>; >>> + phys = <&usb_2_hsphy0>, >>> <&usb_2_qmpphy0>, >>> + <&usb_2_hsphy1>, >>> <&usb_2_qmpphy1>, >>> + <&usb_2_hsphy2>, >>> + <&usb_2_hsphy3>; >>> + phy-names = "usb2-phy_port0", >>> "usb3-phy_port0", >>> + "usb2-phy_port1", >>> "usb3-phy_port1", >>> + "usb2-phy_port2", >>> + "usb2-phy_port3"; >>> + }; >>> >>> Since this board only uses one port of the multiport controller, I >>> redefined phys/phy-names to indicate that. I figured that was more >>> desireable than enabling unnecessary phys. Without overriding >>> num-ports/num-ss-ports all the for loops in this patch would act like >>> the values were 4 and 2 respectively, writing to DWC3_GUSB2PHYCFG >>> multiple times etc as well as look for the multiport phy-names and fail >>> to actually get any phys. Hope that makes sense! >>> >> Hi Andrew, >> >> My Bad. I missed the fact that it was based on sc8280xp.dtsi. In >> that case it makes complete sense to override the num-ports and >> num-ss-ports to "1" and the usb phy-names. >>>>> Here's a patch enabling sa8540p-ride, I'd love if you'd add it to the >>>>> series (probably needs clean up after review, and will definitely need >>>>> alteration after you update the dt-binding again). If not I'll >>>>> continue >>>>> to test/review so please CC me!: >>>>> >>>>> >>>> Sure, I can add this patch (probably will add the other phy's too) >>>> during >>>> the final submission. >>> >>> I don't have a great understanding of the mapping of the phys to >>> physical connections (as well as what registers like DWC3_GUSB2PHYCFG >>> do), >>> so if it makes more sense to enable all the relevant SoC phys, write >>> those registers in the DWC3 IP, etc, and only use one of the actual >>> board outputs then feel free. I think this is a good example of "what if >>> a board designer only uses a single port of the multiport IP" imo. >>> Agreed. This could be a good example of multi port with only single port > > Typo in the previous mail. Correcting it here. > >> working. > Agreed, The dt-patch you provided will be a good working example of > getting just a single port working for a multiport controller. > > Regards, > Krishna, > >>>> >>>>> From dcb27d07f079194ebd7efe1c9bec64da78beb290 Mon Sep 17 00:00:00 >>>>> 2001 >>>>> From: Andrew Halaney <ahalaney@redhat.com> >>>>> Date: Thu, 19 Jan 2023 14:53:38 -0600 >>>>> Subject: [PATCH] arm64: dts: qcom: sa8540p-ride: Enable usb_2 >>>>> Content-type: text/plain >>>>> >>>>> There is now support for the multiport USB controller this uses >>>>> so enable it. >>>>> >>>>> The board only has a single port hooked up (despite it being wired >>>>> up to >>>>> the multiport IP on the SoC). There's also a USB 2.0 mux hooked up, >>>>> which by default on boot is selected to mux properly. Grab the gpio >>>>> controlling that and ensure it stays in the right position so USB 2.0 >>>>> continues to be routed from the external port to the SoC. >>>>> >>>>> Signed-off-by: Andrew Halaney <ahalaney@redhat.com> >>>>> --- >>>>> arch/arm64/boot/dts/qcom/sa8540p-ride.dts | 24 >>>>> +++++++++++++++++++++++ >>>>> 1 file changed, 24 insertions(+) >>>>> >>>>> diff --git a/arch/arm64/boot/dts/qcom/sa8540p-ride.dts >>>>> b/arch/arm64/boot/dts/qcom/sa8540p-ride.dts >>>>> index 97957f3baa64..56d4f43faa1e 100644 >>>>> --- a/arch/arm64/boot/dts/qcom/sa8540p-ride.dts >>>>> +++ b/arch/arm64/boot/dts/qcom/sa8540p-ride.dts >>>>> @@ -246,6 +246,21 @@ &usb_0_qmpphy { >>>>> status = "okay"; >>>>> }; >>>>> +&usb_2 { >>>>> + pinctrl-names = "default"; >>>>> + pinctrl-0 = <&usb2_en_state>; >>>>> + >>>>> + status = "okay"; >>>>> +}; >>>>> + >>>>> +&usb_2_dwc3 { >>>>> + dr_mode = "host"; >>>>> + num-ports = <1>; >>>>> + num-ss-ports = <1>; >>>> >>>> More over, if this is a multiport controller and you are using only >>>> port-1, >>>> it is as good as a single port controller I believe and the normal DT >>>> convention must work. Adding these properties as "1" is not required >>>> as the >>>> driver logic defaults them to "1" if they are not found. >>> >>> See above comment about inheriting from sc8280xp.dtsi and needing to >>> override their values. >>> >>>> >>>> Just to add a point here (as I was not clear in DT Binding >>>> description, My >>>> bad), the num-ports and num-ss-ports must indicate the HS/SS Phys >>>> present on >>>> HW whether they are used in DT or not. Just to cover all cases which >>>> user >>>> can use [1]. >>>> >>>> []1: >>>> https://lore.kernel.org/all/4eb26a54-148b-942f-01c6-64e66541de8b@quicinc.com/ >>> >>> Ok, if you're going with that approach of "must indicate the HS/SS Phys >>> present on HW whether they are used in the DT or not" (/me assumes DT >>> here means on the board and not an incorrect coding of the DT) then I >>> suppose I should not have overridden anything but phys/phy-names to >>> indicate that I'm only using the first port (and used the multiport >>> phy-names convention). It looks like in that link you also mention that >>> it is ok to write to DWC3_GUSB2PHYCFG and friends even if the phy isn't >>> defined, which was my concern and reasoning above for overriding >>> num-ports/num-ss-ports. >>> >>> Thanks, >>> Andrew >>> >> Actually, I was trying to mandate that rule to take care of cases >> where the phy's for say port2 or port3 are missing for a quad port >> controller in dtsi and we don't want to end up configuring wrong >> dwc3-phy regs. >> >> For just the first port, the changes you have mentioned must be >> sufficient. (Furthermore, thanks for the review and testing it on >> sa8295-ride and confirming nothing breaks while the first port is >> enabled) I agree that for ride platform the changes provided with overridden phys in DT for the sa8540-ride.dts makes more sense with only having port0 of multi port on this platform. Looking forward to validate the next version on ride platform. Krishna, please keep me in cc in the next version :). -Shazad >> >> Regards, >> Krishna, >>>> >>>> Regards, >>>> Krishna, >>>> >>>>> + phy-names = "usb2-phy", "usb3-phy"; >>>>> + phys = <&usb_2_hsphy0>, <&usb_2_qmpphy0>; >>>>> +}; >>>>> + >>>>> &usb_2_hsphy0 { >>>>> vdda-pll-supply = <&vreg_l5a>; >>>>> vdda18-supply = <&vreg_l7g>; >>>>> @@ -313,4 +328,13 @@ wake-pins { >>>>> bias-pull-up; >>>>> }; >>>>> }; >>>>> + >>>>> + usb2_en_state: usb2-en-state { >>>>> + /* TS3USB221A USB2.0 mux select */ >>>>> + pins = "gpio24"; >>>>> + function = "gpio"; >>>>> + drive-strength = <2>; >>>>> + bias-disable; >>>>> + output-low; >>>>> + }; >>>>> }; >>>> >>>
On Wed, Jan 25, 2023, Krishna Kurapati PSSNV wrote: > > > On 1/21/2023 4:14 AM, Thinh Nguyen wrote: > > > > This becomes rather more complicated because the user can skip certain > > port in the DT. We have access to the host registers. Can we just > > temporarily map and access HCSPARAMS1 to get the MAXPORTS and each port > > capability before handing control over to the xHCI driver. We would be > > able to get the num_ports and num_ss_ports then. > > > > Similarly, the xhci driver doesn't care whether the user skips certain > > port in the DT, it only checks and operates based on the capability > > registers. > > > > If we have the exact num_ports and num_ss_ports, we can be sure the > > setting to GUSB3PIPECTLn and GUSB2PHYCFGn are valid. > > > > Hi Thinh, > > Thanks for the suggestion. Is the following diff / implementation good > enough ? I Wanted to get it clarified from upstream as I am using > *ioremap/iounmap* directly instead of *devm_* API's > > I tested it and it works fine on SA8295P. Will do some further testing on > other devices as well. > > > +static int dwc3_read_port_info(struct dwc3 *dwc, struct resource *res) > +{ > + void __iomem *regs; > + struct resource dwc_res; > + unsigned int hw_mode; > + u32 offset; > + u32 temp; > + u8 major_revision; > + u8 minor_revision; > + > + /* > + * Request memory region including xHCI regs, > + * since it is needed to get port info > + */ > + dwc_res = *res; > + dwc_res.start += 0; > + > + regs = ioremap(dwc_res.start, resource_size(&dwc_res)); > + if (IS_ERR(regs)) { > + return PTR_ERR(regs); > + } We don't need to ioremap the whole region. Just do it for the xhci_resources[0] > + > + /* > + * If the controller is not host-only, then it must be a > + * single port controller. > + */ > + temp = readl(regs + DWC3_GHWPARAMS0); > + hw_mode = DWC3_GHWPARAMS0_MODE(temp); > + if (hw_mode != DWC3_GHWPARAMS0_MODE_HOST) { > + dwc->num_ports = 1; > + dwc->num_ss_ports = 1; > + return 0; > + } This check should be done before we get into this function. > + > + offset = xhci_find_next_ext_cap(regs, 0, > + XHCI_EXT_CAPS_PROTOCOL); > + while (offset) { > + temp = readl(regs + offset); > + major_revision = XHCI_EXT_PORT_MAJOR(temp);; > + minor_revision = XHCI_EXT_PORT_MINOR(temp); We probably don't need minor revision. > + > + temp = readl(regs + offset + 0x08); > + if (major_revision == 0x03) { > + dwc->num_ss_ports += XHCI_EXT_PORT_COUNT(temp); > + } else if (major_revision <= 0x02) { > + dwc->num_ports += XHCI_EXT_PORT_COUNT(temp); > + } else { > + dev_err(dwc->dev, "revision gone wrong\n"); > + return -EINVAL; > + } > + > + offset = xhci_find_next_ext_cap(regs, offset, > + XHCI_EXT_CAPS_PROTOCOL); > + } > + > + temp = readl(regs + DWC3_XHCI_HCSPARAMS1_OFFSET); > + if (HCS_MAX_PORTS(temp) != (dwc->num_ss_ports + dwc->num_ports)) { > + dev_err(dwc->dev, "inconsistency in port info\n"); > + return -EINVAL; > + } > + > + dev_info(dwc->dev, "num_ports: %d, num_ss_ports: %d\n", > dwc->num_ports, dwc->num_ss_ports); > + iounmap(regs); Make sure to iounmap on all the early return/error cases. > + return 0; > +} > + > static int dwc3_probe(struct platform_device *pdev) > { > struct device *dev = &pdev->dev; > @@ -1912,6 +1964,10 @@ static int dwc3_probe(struct platform_device *pdev) > dwc->xhci_resources[0].flags = res->flags; > dwc->xhci_resources[0].name = res->name; > > + ret = dwc3_read_port_info(dwc, res); This should be called after some initializations to make sure some clocks are enabled. Otherwise some devices may not able to access the registers. Preferably after dwc3_cache_hwparams() but before dwc3_core_init(). > + if (ret) > + return ret; > + > /* > * Request memory region but exclude xHCI regs, > * since it will be requested by the xhci-plat driver. > diff --git a/drivers/usb/dwc3/core.h b/drivers/usb/dwc3/core.h > index 2f82eda9d44f..8535425b81d4 100644 > --- a/drivers/usb/dwc3/core.h > +++ b/drivers/usb/dwc3/core.h > @@ -38,6 +38,9 @@ > /* Numer of ports supported by a multiport controller */ > #define MAX_PORTS_SUPPORTED 4 > > +/* XHCI Reg constants */ > +#define DWC3_XHCI_HCSPARAMS1_OFFSET 0x04 Change to DWC3_XHCI_HCSPARAMS1 > + > /* Global constants */ > #define DWC3_PULL_UP_TIMEOUT 500 /* ms */ > #define DWC3_BOUNCE_SIZE 1024 /* size of a superspeed bulk */ > > > > Please let me know if this would be acceptable. > It looks fine to me. Please review the comments and remmove debug prints and any other cleanup for a proper patch. Thanks, Thinh
On Wed, Jan 25, 2023 at 07:08:10PM +0000, Thinh Nguyen wrote: <snip> > > + /* > > + * If the controller is not host-only, then it must be a > > + * single port controller. > > + */ Thinh, is this a correct assumption? Is it possible for the IP to be synthesized to support both dual-role and multiple ports? We know that when operating in device mode only the first port can be used but the additional ports would be usable when in host. Thanks, Jack > > + temp = readl(regs + DWC3_GHWPARAMS0); > > + hw_mode = DWC3_GHWPARAMS0_MODE(temp); > > + if (hw_mode != DWC3_GHWPARAMS0_MODE_HOST) { > > + dwc->num_ports = 1; > > + dwc->num_ss_ports = 1; > > + return 0; > > + } > > This check should be done before we get into this function.
On Wed, Jan 25, 2023, Jack Pham wrote: > On Wed, Jan 25, 2023 at 07:08:10PM +0000, Thinh Nguyen wrote: > > <snip> > > > > + /* > > > + * If the controller is not host-only, then it must be a > > > + * single port controller. > > > + */ > > Thinh, is this a correct assumption? Is it possible for the IP to be > synthesized to support both dual-role and multiple ports? We know that > when operating in device mode only the first port can be used but the > additional ports would be usable when in host. > Yes. Maybe we should rephrase it a bit. Perhaps something like this: "Currently, only host mode supports multi-port" In case this may change in the future. BR, Thinh > > > > + temp = readl(regs + DWC3_GHWPARAMS0); > > > + hw_mode = DWC3_GHWPARAMS0_MODE(temp); > > > + if (hw_mode != DWC3_GHWPARAMS0_MODE_HOST) { > > > + dwc->num_ports = 1; > > > + dwc->num_ss_ports = 1; > > > + return 0; > > > + } > > > > This check should be done before we get into this function.