Message ID | 20240408132925.1880571-1-quic_kriskura@quicinc.com |
---|---|
Headers | show |
Series | Add multiport support for DWC3 controllers | expand |
On Mon, Apr 08, 2024, Krishna Kurapati wrote: > Currently the DWC3 driver supports only single port controller > which requires at least one HS PHY and at most 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 PHYs supported > by a multiport controller. Limit support to multiport controllers > with up to four ports for now (e.g. as needed for SC8280XP). > > Signed-off-by: Krishna Kurapati <quic_kriskura@quicinc.com> > Reviewed-by: Johan Hovold <johan+linaro@kernel.org> > --- > drivers/usb/dwc3/core.c | 251 ++++++++++++++++++++++++++++------------ > drivers/usb/dwc3/core.h | 14 ++- > drivers/usb/dwc3/drd.c | 15 ++- > 3 files changed, 193 insertions(+), 87 deletions(-) > <snip> > @@ -1937,6 +2020,10 @@ static int dwc3_get_num_ports(struct dwc3 *dwc) > > iounmap(base); > > + if (dwc->num_usb2_ports > DWC3_MAX_PORTS || > + dwc->num_usb3_ports > DWC3_MAX_PORTS) > + return -ENOMEM; This should be -EINVAL. > + > return 0; > } <snip> > diff --git a/drivers/usb/dwc3/core.h b/drivers/usb/dwc3/core.h > index 341e4c73cb2e..df2e111aa848 100644 > --- a/drivers/usb/dwc3/core.h > +++ b/drivers/usb/dwc3/core.h > @@ -33,6 +33,12 @@ > > #include <linux/power_supply.h> > > +/* > + * Maximum number of ports currently supported for multiport > + * controllers. This macro here is being used per USB2 vs USB3 ports rather than USB2 + USB3, unlike the xHCI MAXPORTS. You can clarify in the comment and rename the macro to avoid any confusion. You can also create 2 separate macros for number of USB2 and USB3 ports even if they share the same value. As noted[*], we support have different max number of usb2 ports vs usb3 ports. I would suggest splitting the macros. [*] https://lore.kernel.org/linux-usb/20230801013031.ft3zpoatiyfegmh6@synopsys.com/ > + */ > +#define DWC3_MAX_PORTS 4 > + > But it's not a big issue whether you decided to push a new version or a create a separate patch for the comments above. Here's my Ack: Acked-by: Thinh Nguyen <Thinh.Nguyen@synopsys.com> Thanks, Thinh
On Mon, Apr 08, 2024, Krishna Kurapati wrote: > On multiport supported controllers, each port has its own DP/DM > and SS (if super speed capable) interrupts. As per the bindings, > their interrupt names differ from standard ones having "_x" added > as suffix (x indicates port number). Identify from the interrupt > names whether the controller is a multiport controller or not. > Refactor dwc3_qcom_setup_irq() call to parse multiport interrupts > along with non-multiport ones accordingly.. > > Signed-off-by: Krishna Kurapati <quic_kriskura@quicinc.com> > Reviewed-by: Johan Hovold <johan+linaro@kernel.org> > --- > drivers/usb/dwc3/dwc3-qcom.c | 137 ++++++++++++++++++++++++++--------- > 1 file changed, 103 insertions(+), 34 deletions(-) > > diff --git a/drivers/usb/dwc3/dwc3-qcom.c b/drivers/usb/dwc3/dwc3-qcom.c > index cae5dab8fcfc..35eb338514bc 100644 > --- a/drivers/usb/dwc3/dwc3-qcom.c > +++ b/drivers/usb/dwc3/dwc3-qcom.c > @@ -52,6 +52,13 @@ > #define APPS_USB_AVG_BW 0 > #define APPS_USB_PEAK_BW MBps_to_icc(40) > > +struct dwc3_qcom_port { > + int qusb2_phy_irq; > + int dp_hs_phy_irq; > + int dm_hs_phy_irq; > + int ss_phy_irq; > +}; > + > struct dwc3_qcom { > struct device *dev; > void __iomem *qscratch_base; > @@ -59,11 +66,8 @@ struct dwc3_qcom { > struct clk **clks; > int num_clocks; > struct reset_control *resets; > - > - int qusb2_phy_irq; > - int dp_hs_phy_irq; > - int dm_hs_phy_irq; > - int ss_phy_irq; > + struct dwc3_qcom_port ports[DWC3_MAX_PORTS]; > + u8 num_ports; > enum usb_device_speed usb2_speed; > > struct extcon_dev *edev; > @@ -354,24 +358,24 @@ static void dwc3_qcom_disable_wakeup_irq(int irq) > > static void dwc3_qcom_disable_interrupts(struct dwc3_qcom *qcom) > { > - dwc3_qcom_disable_wakeup_irq(qcom->qusb2_phy_irq); > + dwc3_qcom_disable_wakeup_irq(qcom->ports[0].qusb2_phy_irq); > > if (qcom->usb2_speed == USB_SPEED_LOW) { > - dwc3_qcom_disable_wakeup_irq(qcom->dm_hs_phy_irq); > + dwc3_qcom_disable_wakeup_irq(qcom->ports[0].dm_hs_phy_irq); > } else if ((qcom->usb2_speed == USB_SPEED_HIGH) || > (qcom->usb2_speed == USB_SPEED_FULL)) { > - dwc3_qcom_disable_wakeup_irq(qcom->dp_hs_phy_irq); > + dwc3_qcom_disable_wakeup_irq(qcom->ports[0].dp_hs_phy_irq); > } else { > - dwc3_qcom_disable_wakeup_irq(qcom->dp_hs_phy_irq); > - dwc3_qcom_disable_wakeup_irq(qcom->dm_hs_phy_irq); > + dwc3_qcom_disable_wakeup_irq(qcom->ports[0].dp_hs_phy_irq); > + dwc3_qcom_disable_wakeup_irq(qcom->ports[0].dm_hs_phy_irq); > } > > - dwc3_qcom_disable_wakeup_irq(qcom->ss_phy_irq); > + dwc3_qcom_disable_wakeup_irq(qcom->ports[0].ss_phy_irq); > } > > static void dwc3_qcom_enable_interrupts(struct dwc3_qcom *qcom) > { > - dwc3_qcom_enable_wakeup_irq(qcom->qusb2_phy_irq, 0); > + dwc3_qcom_enable_wakeup_irq(qcom->ports[0].qusb2_phy_irq, 0); > > /* > * Configure DP/DM line interrupts based on the USB2 device attached to > @@ -383,20 +387,20 @@ static void dwc3_qcom_enable_interrupts(struct dwc3_qcom *qcom) > */ > > if (qcom->usb2_speed == USB_SPEED_LOW) { > - dwc3_qcom_enable_wakeup_irq(qcom->dm_hs_phy_irq, > - IRQ_TYPE_EDGE_FALLING); > + dwc3_qcom_enable_wakeup_irq(qcom->ports[0].dm_hs_phy_irq, > + IRQ_TYPE_EDGE_FALLING); > } else if ((qcom->usb2_speed == USB_SPEED_HIGH) || > (qcom->usb2_speed == USB_SPEED_FULL)) { > - dwc3_qcom_enable_wakeup_irq(qcom->dp_hs_phy_irq, > - IRQ_TYPE_EDGE_FALLING); > + dwc3_qcom_enable_wakeup_irq(qcom->ports[0].dp_hs_phy_irq, > + IRQ_TYPE_EDGE_FALLING); > } else { > - dwc3_qcom_enable_wakeup_irq(qcom->dp_hs_phy_irq, > - IRQ_TYPE_EDGE_RISING); > - dwc3_qcom_enable_wakeup_irq(qcom->dm_hs_phy_irq, > - IRQ_TYPE_EDGE_RISING); > + dwc3_qcom_enable_wakeup_irq(qcom->ports[0].dp_hs_phy_irq, > + IRQ_TYPE_EDGE_RISING); > + dwc3_qcom_enable_wakeup_irq(qcom->ports[0].dm_hs_phy_irq, > + IRQ_TYPE_EDGE_RISING); > } > > - dwc3_qcom_enable_wakeup_irq(qcom->ss_phy_irq, 0); > + dwc3_qcom_enable_wakeup_irq(qcom->ports[0].ss_phy_irq, 0); > } > > static int dwc3_qcom_suspend(struct dwc3_qcom *qcom, bool wakeup) > @@ -517,42 +521,107 @@ static int dwc3_qcom_request_irq(struct dwc3_qcom *qcom, int irq, > return ret; > } > > -static int dwc3_qcom_setup_irq(struct platform_device *pdev) > +static int dwc3_qcom_setup_port_irq(struct platform_device *pdev, int port_index, bool is_multiport) > { > struct dwc3_qcom *qcom = platform_get_drvdata(pdev); > + const char *irq_name; > int irq; > int ret; > > - irq = platform_get_irq_byname_optional(pdev, "qusb2_phy"); > + if (is_multiport) > + irq_name = devm_kasprintf(&pdev->dev, GFP_KERNEL, "dp_hs_phy_%d", port_index + 1); > + else > + irq_name = devm_kasprintf(&pdev->dev, GFP_KERNEL, "dp_hs_phy_irq"); > + if (!irq_name) > + return -ENOMEM; > + > + irq = platform_get_irq_byname_optional(pdev, irq_name); > if (irq > 0) { > - ret = dwc3_qcom_request_irq(qcom, irq, "qusb2_phy"); > + ret = dwc3_qcom_request_irq(qcom, irq, irq_name); > if (ret) > return ret; > - qcom->qusb2_phy_irq = irq; > + qcom->ports[port_index].dp_hs_phy_irq = irq; > } > > - irq = platform_get_irq_byname_optional(pdev, "dp_hs_phy_irq"); > + if (is_multiport) > + irq_name = devm_kasprintf(&pdev->dev, GFP_KERNEL, "dm_hs_phy_%d", port_index + 1); > + else > + irq_name = devm_kasprintf(&pdev->dev, GFP_KERNEL, "dm_hs_phy_irq"); > + if (!irq_name) > + return -ENOMEM; > + > + irq = platform_get_irq_byname_optional(pdev, irq_name); > if (irq > 0) { > - ret = dwc3_qcom_request_irq(qcom, irq, "dp_hs_phy_irq"); > + ret = dwc3_qcom_request_irq(qcom, irq, irq_name); > if (ret) > return ret; > - qcom->dp_hs_phy_irq = irq; > + qcom->ports[port_index].dm_hs_phy_irq = irq; > } > > - irq = platform_get_irq_byname_optional(pdev, "dm_hs_phy_irq"); > + if (is_multiport) > + irq_name = devm_kasprintf(&pdev->dev, GFP_KERNEL, "ss_phy_%d", port_index + 1); > + else > + irq_name = devm_kasprintf(&pdev->dev, GFP_KERNEL, "ss_phy_irq"); > + if (!irq_name) > + return -ENOMEM; > + > + irq = platform_get_irq_byname_optional(pdev, irq_name); > if (irq > 0) { > - ret = dwc3_qcom_request_irq(qcom, irq, "dm_hs_phy_irq"); > + ret = dwc3_qcom_request_irq(qcom, irq, irq_name); > if (ret) > return ret; > - qcom->dm_hs_phy_irq = irq; > + qcom->ports[port_index].ss_phy_irq = irq; > } > > - irq = platform_get_irq_byname_optional(pdev, "ss_phy_irq"); > + if (is_multiport) > + return 0; > + > + irq = platform_get_irq_byname_optional(pdev, "qusb2_phy"); > if (irq > 0) { > - ret = dwc3_qcom_request_irq(qcom, irq, "ss_phy_irq"); > + ret = dwc3_qcom_request_irq(qcom, irq, "qusb2_phy"); > + if (ret) > + return ret; > + qcom->ports[port_index].qusb2_phy_irq = irq; > + } > + > + return 0; > +} > + > +static int dwc3_qcom_find_num_ports(struct platform_device *pdev) > +{ > + char irq_name[14]; > + int port_num; > + int irq; > + > + irq = platform_get_irq_byname_optional(pdev, "dp_hs_phy_1"); > + if (irq <= 0) > + return 1; > + > + for (port_num = 2; port_num <= DWC3_MAX_PORTS; port_num++) { > + sprintf(irq_name, "dp_hs_phy_%d", port_num); > + > + irq = platform_get_irq_byname_optional(pdev, irq_name); > + if (irq <= 0) > + return port_num - 1; > + } > + > + return DWC3_MAX_PORTS; > +} > + > +static int dwc3_qcom_setup_irq(struct platform_device *pdev) > +{ > + struct dwc3_qcom *qcom = platform_get_drvdata(pdev); > + bool is_multiport; > + int ret; > + int i; > + > + qcom->num_ports = dwc3_qcom_find_num_ports(pdev); > + is_multiport = (qcom->num_ports > 1); > + > + for (i = 0; i < qcom->num_ports; i++) { > + ret = dwc3_qcom_setup_port_irq(pdev, i, is_multiport); > if (ret) > return ret; > - qcom->ss_phy_irq = irq; > } > > return 0; > -- > 2.34.1 > Acked-by: Thinh Nguyen <Thinh.Nguyen@synopsys.com> Thanks, Thinh
On 4/9/2024 6:41 AM, Thinh Nguyen wrote: > On Mon, Apr 08, 2024, Krishna Kurapati wrote: >> Currently the DWC3 driver supports only single port controller >> which requires at least one HS PHY and at most 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 PHYs supported >> by a multiport controller. Limit support to multiport controllers >> with up to four ports for now (e.g. as needed for SC8280XP). >> >> Signed-off-by: Krishna Kurapati <quic_kriskura@quicinc.com> >> Reviewed-by: Johan Hovold <johan+linaro@kernel.org> >> --- >> drivers/usb/dwc3/core.c | 251 ++++++++++++++++++++++++++++------------ >> drivers/usb/dwc3/core.h | 14 ++- >> drivers/usb/dwc3/drd.c | 15 ++- >> 3 files changed, 193 insertions(+), 87 deletions(-) >> > > <snip> > >> @@ -1937,6 +2020,10 @@ static int dwc3_get_num_ports(struct dwc3 *dwc) >> >> iounmap(base); >> >> + if (dwc->num_usb2_ports > DWC3_MAX_PORTS || >> + dwc->num_usb3_ports > DWC3_MAX_PORTS) >> + return -ENOMEM; > > This should be -EINVAL. > >> + >> return 0; >> } > > <snip> > >> diff --git a/drivers/usb/dwc3/core.h b/drivers/usb/dwc3/core.h >> index 341e4c73cb2e..df2e111aa848 100644 >> --- a/drivers/usb/dwc3/core.h >> +++ b/drivers/usb/dwc3/core.h >> @@ -33,6 +33,12 @@ >> >> #include <linux/power_supply.h> >> >> +/* >> + * Maximum number of ports currently supported for multiport >> + * controllers. > > This macro here is being used per USB2 vs USB3 ports rather than USB2 + > USB3, unlike the xHCI MAXPORTS. You can clarify in the comment and > rename the macro to avoid any confusion. You can also create 2 separate > macros for number of USB2 and USB3 ports even if they share the same > value. > > As noted[*], we support have different max number of usb2 ports vs usb3 > ports. I would suggest splitting the macros. > Hi Thinh, This macro was intended only to identify how many USB2 (or USB3) Phy's were serviced/operated by this driver, not how many logical ports present (like in xHCI). I don't think it would be confusing currently given that it is only used to identify number of generic phy instances to allocate and not used for any other purpose. Once the num_usb2_ports and num_usb3_ports are read by get_num_ports(...) call, they directly indicate how many ports are HS and SS respectively. Keeping the same in mind, I returned ENOMEM above (as you mentioned) because we don't allocate more than DWC3_MAX_PORTS and if the number of hs or ss ports is more than that, we simply return ENOMEM saying the driver doesn't support operating those many phy's. > [*] https://lore.kernel.org/linux-usb/20230801013031.ft3zpoatiyfegmh6@synopsys.com/ > >> + */ >> +#define DWC3_MAX_PORTS 4 >> + >> > > But it's not a big issue whether you decided to push a new version or a > create a separate patch for the comments above. Here's my Ack: > Since this is not a bug, I would prefer to make a separate patch to rename the macros. (If that is fine). > Acked-by: Thinh Nguyen <Thinh.Nguyen@synopsys.com> > > Thanks, > Thinh