| Message ID | 20230621043628.21485-1-quic_kriskura@quicinc.com |
|---|---|
| Headers | show |
| Series | Add multiport support for DWC3 controllers | expand |
On 21.06.2023 06:36, Krishna Kurapati wrote: > Add support to read Multiport IRQ's related to quad port controller > of SA8295 Device. > > Signed-off-by: Krishna Kurapati <quic_kriskura@quicinc.com> > --- > drivers/usb/dwc3/dwc3-qcom.c | 108 +++++++++++++++++++++++++++++------ > 1 file changed, 91 insertions(+), 17 deletions(-) > > diff --git a/drivers/usb/dwc3/dwc3-qcom.c b/drivers/usb/dwc3/dwc3-qcom.c > index 3de43df6bbe8..3ab48a6925fe 100644 > --- a/drivers/usb/dwc3/dwc3-qcom.c > +++ b/drivers/usb/dwc3/dwc3-qcom.c > @@ -74,9 +74,9 @@ struct dwc3_qcom { > struct reset_control *resets; > > int hs_phy_irq; > - int dp_hs_phy_irq; > - int dm_hs_phy_irq; > - int ss_phy_irq; > + int dp_hs_phy_irq[4]; > + int dm_hs_phy_irq[4]; > + int ss_phy_irq[2]; Not sure if that's been raised previously, but having raw numbers here is not very descriptive.. MAX_NUM_MP_HSPHY or something would be helpful for readability.. Konrad > enum usb_device_speed usb2_speed; > > struct extcon_dev *edev; > @@ -375,16 +375,16 @@ static void dwc3_qcom_disable_interrupts(struct dwc3_qcom *qcom) > dwc3_qcom_disable_wakeup_irq(qcom->hs_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->dm_hs_phy_irq[0]); > } 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->dp_hs_phy_irq[0]); > } 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->dp_hs_phy_irq[0]); > + dwc3_qcom_disable_wakeup_irq(qcom->dm_hs_phy_irq[0]); > } > > - dwc3_qcom_disable_wakeup_irq(qcom->ss_phy_irq); > + dwc3_qcom_disable_wakeup_irq(qcom->ss_phy_irq[0]); > } > > static void dwc3_qcom_enable_interrupts(struct dwc3_qcom *qcom) > @@ -401,20 +401,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, > + dwc3_qcom_enable_wakeup_irq(qcom->dm_hs_phy_irq[0], > 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, > + dwc3_qcom_enable_wakeup_irq(qcom->dp_hs_phy_irq[0], > IRQ_TYPE_EDGE_FALLING); > } else { > - dwc3_qcom_enable_wakeup_irq(qcom->dp_hs_phy_irq, > + dwc3_qcom_enable_wakeup_irq(qcom->dp_hs_phy_irq[0], > IRQ_TYPE_EDGE_RISING); > - dwc3_qcom_enable_wakeup_irq(qcom->dm_hs_phy_irq, > + dwc3_qcom_enable_wakeup_irq(qcom->dm_hs_phy_irq[0], > IRQ_TYPE_EDGE_RISING); > } > > - dwc3_qcom_enable_wakeup_irq(qcom->ss_phy_irq, 0); > + dwc3_qcom_enable_wakeup_irq(qcom->ss_phy_irq[0], 0); > } > > static int dwc3_qcom_suspend(struct dwc3_qcom *qcom, bool wakeup) > @@ -535,6 +535,80 @@ static int dwc3_qcom_get_irq(struct platform_device *pdev, > return ret; > } > > +static int dwc3_qcom_setup_mp_irq(struct platform_device *pdev) > +{ > + struct dwc3_qcom *qcom = platform_get_drvdata(pdev); > + char irq_name[15]; > + int irq; > + int ret; > + int i; > + > + for (i = 0; i < 4; i++) { > + if (qcom->dp_hs_phy_irq[i]) > + continue; > + > + sprintf(irq_name, "dp%d_hs_phy_irq", i+1); > + irq = dwc3_qcom_get_irq(pdev, irq_name, -1); > + if (irq > 0) { > + irq_set_status_flags(irq, IRQ_NOAUTOEN); > + ret = devm_request_threaded_irq(qcom->dev, irq, NULL, > + qcom_dwc3_resume_irq, > + IRQF_TRIGGER_HIGH | IRQF_ONESHOT, > + irq_name, qcom); > + if (ret) { > + dev_err(qcom->dev, "%s failed: %d\n", irq_name, ret); > + return ret; > + } > + } > + > + qcom->dp_hs_phy_irq[i] = irq; > + } > + > + for (i = 0; i < 4; i++) { > + if (qcom->dm_hs_phy_irq[i]) > + continue; > + > + sprintf(irq_name, "dm%d_hs_phy_irq", i+1); > + irq = dwc3_qcom_get_irq(pdev, irq_name, -1); > + if (irq > 0) { > + irq_set_status_flags(irq, IRQ_NOAUTOEN); > + ret = devm_request_threaded_irq(qcom->dev, irq, NULL, > + qcom_dwc3_resume_irq, > + IRQF_TRIGGER_HIGH | IRQF_ONESHOT, > + irq_name, qcom); > + if (ret) { > + dev_err(qcom->dev, "%s failed: %d\n", irq_name, ret); > + return ret; > + } > + } > + > + qcom->dm_hs_phy_irq[i] = irq; > + } > + > + for (i = 0; i < 2; i++) { > + if (qcom->ss_phy_irq[i]) > + continue; > + > + sprintf(irq_name, "ss%d_phy_irq", i+1); > + irq = dwc3_qcom_get_irq(pdev, irq_name, -1); > + if (irq > 0) { > + irq_set_status_flags(irq, IRQ_NOAUTOEN); > + ret = devm_request_threaded_irq(qcom->dev, irq, NULL, > + qcom_dwc3_resume_irq, > + IRQF_TRIGGER_HIGH | IRQF_ONESHOT, > + irq_name, qcom); > + if (ret) { > + dev_err(qcom->dev, "%s failed: %d\n", irq_name, ret); > + return ret; > + } > + } > + > + qcom->ss_phy_irq[i] = irq; > + } > + > + return 0; > +} > + > static int dwc3_qcom_setup_irq(struct platform_device *pdev) > { > struct dwc3_qcom *qcom = platform_get_drvdata(pdev); > @@ -570,7 +644,7 @@ static int dwc3_qcom_setup_irq(struct platform_device *pdev) > dev_err(qcom->dev, "dp_hs_phy_irq failed: %d\n", ret); > return ret; > } > - qcom->dp_hs_phy_irq = irq; > + qcom->dp_hs_phy_irq[0] = irq; > } > > irq = dwc3_qcom_get_irq(pdev, "dm_hs_phy_irq", > @@ -585,7 +659,7 @@ static int dwc3_qcom_setup_irq(struct platform_device *pdev) > dev_err(qcom->dev, "dm_hs_phy_irq failed: %d\n", ret); > return ret; > } > - qcom->dm_hs_phy_irq = irq; > + qcom->dm_hs_phy_irq[0] = irq; > } > > irq = dwc3_qcom_get_irq(pdev, "ss_phy_irq", > @@ -600,10 +674,10 @@ static int dwc3_qcom_setup_irq(struct platform_device *pdev) > dev_err(qcom->dev, "ss_phy_irq failed: %d\n", ret); > return ret; > } > - qcom->ss_phy_irq = irq; > + qcom->ss_phy_irq[0] = irq; > } > > - return 0; > + return dwc3_qcom_setup_mp_irq(pdev);; > } > > static int dwc3_qcom_clk_init(struct dwc3_qcom *qcom, int count)
On 6/21/2023 3:35 PM, Konrad Dybcio wrote: > On 21.06.2023 06:36, Krishna Kurapati wrote: >> Add support to read Multiport IRQ's related to quad port controller >> of SA8295 Device. >> >> Signed-off-by: Krishna Kurapati <quic_kriskura@quicinc.com> >> --- >> drivers/usb/dwc3/dwc3-qcom.c | 108 +++++++++++++++++++++++++++++------ >> 1 file changed, 91 insertions(+), 17 deletions(-) >> >> diff --git a/drivers/usb/dwc3/dwc3-qcom.c b/drivers/usb/dwc3/dwc3-qcom.c >> index 3de43df6bbe8..3ab48a6925fe 100644 >> --- a/drivers/usb/dwc3/dwc3-qcom.c >> +++ b/drivers/usb/dwc3/dwc3-qcom.c >> @@ -74,9 +74,9 @@ struct dwc3_qcom { >> struct reset_control *resets; >> >> int hs_phy_irq; >> - int dp_hs_phy_irq; >> - int dm_hs_phy_irq; >> - int ss_phy_irq; >> + int dp_hs_phy_irq[4]; >> + int dm_hs_phy_irq[4]; >> + int ss_phy_irq[2]; > Not sure if that's been raised previously, but having raw numbers here > is not very descriptive.. MAX_NUM_MP_HSPHY or something would be helpful > for readability.. > > Konrad Hi Konrad, This has been implented in v9. Wasn't there till v8. Yes, will replace numbers with Macros. Regards, Krishna, >> enum usb_device_speed usb2_speed; >> >> struct extcon_dev *edev; >> @@ -375,16 +375,16 @@ static void dwc3_qcom_disable_interrupts(struct dwc3_qcom *qcom) >> dwc3_qcom_disable_wakeup_irq(qcom->hs_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->dm_hs_phy_irq[0]); >> } 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->dp_hs_phy_irq[0]); >> } 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->dp_hs_phy_irq[0]); >> + dwc3_qcom_disable_wakeup_irq(qcom->dm_hs_phy_irq[0]); >> } >> >> - dwc3_qcom_disable_wakeup_irq(qcom->ss_phy_irq); >> + dwc3_qcom_disable_wakeup_irq(qcom->ss_phy_irq[0]); >> } >> >> static void dwc3_qcom_enable_interrupts(struct dwc3_qcom *qcom) >> @@ -401,20 +401,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, >> + dwc3_qcom_enable_wakeup_irq(qcom->dm_hs_phy_irq[0], >> 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, >> + dwc3_qcom_enable_wakeup_irq(qcom->dp_hs_phy_irq[0], >> IRQ_TYPE_EDGE_FALLING); >> } else { >> - dwc3_qcom_enable_wakeup_irq(qcom->dp_hs_phy_irq, >> + dwc3_qcom_enable_wakeup_irq(qcom->dp_hs_phy_irq[0], >> IRQ_TYPE_EDGE_RISING); >> - dwc3_qcom_enable_wakeup_irq(qcom->dm_hs_phy_irq, >> + dwc3_qcom_enable_wakeup_irq(qcom->dm_hs_phy_irq[0], >> IRQ_TYPE_EDGE_RISING); >> } >> >> - dwc3_qcom_enable_wakeup_irq(qcom->ss_phy_irq, 0); >> + dwc3_qcom_enable_wakeup_irq(qcom->ss_phy_irq[0], 0); >> } >> >> static int dwc3_qcom_suspend(struct dwc3_qcom *qcom, bool wakeup) >> @@ -535,6 +535,80 @@ static int dwc3_qcom_get_irq(struct platform_device *pdev, >> return ret; >> } >> >> +static int dwc3_qcom_setup_mp_irq(struct platform_device *pdev) >> +{ >> + struct dwc3_qcom *qcom = platform_get_drvdata(pdev); >> + char irq_name[15]; >> + int irq; >> + int ret; >> + int i; >> + >> + for (i = 0; i < 4; i++) { >> + if (qcom->dp_hs_phy_irq[i]) >> + continue; >> + >> + sprintf(irq_name, "dp%d_hs_phy_irq", i+1); >> + irq = dwc3_qcom_get_irq(pdev, irq_name, -1); >> + if (irq > 0) { >> + irq_set_status_flags(irq, IRQ_NOAUTOEN); >> + ret = devm_request_threaded_irq(qcom->dev, irq, NULL, >> + qcom_dwc3_resume_irq, >> + IRQF_TRIGGER_HIGH | IRQF_ONESHOT, >> + irq_name, qcom); >> + if (ret) { >> + dev_err(qcom->dev, "%s failed: %d\n", irq_name, ret); >> + return ret; >> + } >> + } >> + >> + qcom->dp_hs_phy_irq[i] = irq; >> + } >> + >> + for (i = 0; i < 4; i++) { >> + if (qcom->dm_hs_phy_irq[i]) >> + continue; >> + >> + sprintf(irq_name, "dm%d_hs_phy_irq", i+1); >> + irq = dwc3_qcom_get_irq(pdev, irq_name, -1); >> + if (irq > 0) { >> + irq_set_status_flags(irq, IRQ_NOAUTOEN); >> + ret = devm_request_threaded_irq(qcom->dev, irq, NULL, >> + qcom_dwc3_resume_irq, >> + IRQF_TRIGGER_HIGH | IRQF_ONESHOT, >> + irq_name, qcom); >> + if (ret) { >> + dev_err(qcom->dev, "%s failed: %d\n", irq_name, ret); >> + return ret; >> + } >> + } >> + >> + qcom->dm_hs_phy_irq[i] = irq; >> + } >> + >> + for (i = 0; i < 2; i++) { >> + if (qcom->ss_phy_irq[i]) >> + continue; >> + >> + sprintf(irq_name, "ss%d_phy_irq", i+1); >> + irq = dwc3_qcom_get_irq(pdev, irq_name, -1); >> + if (irq > 0) { >> + irq_set_status_flags(irq, IRQ_NOAUTOEN); >> + ret = devm_request_threaded_irq(qcom->dev, irq, NULL, >> + qcom_dwc3_resume_irq, >> + IRQF_TRIGGER_HIGH | IRQF_ONESHOT, >> + irq_name, qcom); >> + if (ret) { >> + dev_err(qcom->dev, "%s failed: %d\n", irq_name, ret); >> + return ret; >> + } >> + } >> + >> + qcom->ss_phy_irq[i] = irq; >> + } >> + >> + return 0; >> +} >> + >> static int dwc3_qcom_setup_irq(struct platform_device *pdev) >> { >> struct dwc3_qcom *qcom = platform_get_drvdata(pdev); >> @@ -570,7 +644,7 @@ static int dwc3_qcom_setup_irq(struct platform_device *pdev) >> dev_err(qcom->dev, "dp_hs_phy_irq failed: %d\n", ret); >> return ret; >> } >> - qcom->dp_hs_phy_irq = irq; >> + qcom->dp_hs_phy_irq[0] = irq; >> } >> >> irq = dwc3_qcom_get_irq(pdev, "dm_hs_phy_irq", >> @@ -585,7 +659,7 @@ static int dwc3_qcom_setup_irq(struct platform_device *pdev) >> dev_err(qcom->dev, "dm_hs_phy_irq failed: %d\n", ret); >> return ret; >> } >> - qcom->dm_hs_phy_irq = irq; >> + qcom->dm_hs_phy_irq[0] = irq; >> } >> >> irq = dwc3_qcom_get_irq(pdev, "ss_phy_irq", >> @@ -600,10 +674,10 @@ static int dwc3_qcom_setup_irq(struct platform_device *pdev) >> dev_err(qcom->dev, "ss_phy_irq failed: %d\n", ret); >> return ret; >> } >> - qcom->ss_phy_irq = irq; >> + qcom->ss_phy_irq[0] = irq; >> } >> >> - return 0; >> + return dwc3_qcom_setup_mp_irq(pdev);; >> } >> >> static int dwc3_qcom_clk_init(struct dwc3_qcom *qcom, int count)
On Wed, Jun 21, 2023, Krishna Kurapati wrote: > Currently host-only capable DWC3 controllers support Multiport. > Temporarily map XHCI address space for host-only controllers and parse > XHCI Extended Capabilities registers to read number of usb2 ports and > usb3 ports present on multiport controller. Each USB Port is at least HS > capable. > > The port info for usb2 and usb3 phy are identified as num_usb2_ports > and num_usb3_ports. The intention is as follows: > > Wherever we need to perform phy operations like: > > LOOP_OVER_NUMBER_OF_AVAILABLE_PORTS() > { > phy_set_mode(dwc->usb2_generic_phy[i], PHY_MODE_USB_HOST); > phy_set_mode(dwc->usb3_generic_phy[i], PHY_MODE_USB_HOST); > } > > If number of usb2 ports is 3, loop can go from index 0-2 for > usb2_generic_phy. If number of usb3-ports is 2, we don't know for sure, > if the first 2 ports are SS capable or some other ports like (2 and 3) > are SS capable. So instead, num_usb2_ports is used to loop around all > phy's (both hs and ss) for performing phy operations. If any > usb3_generic_phy turns out to be NULL, phy operation just bails out. > > num_usb3_ports is used to modify GUSB3PIPECTL registers while setting up > phy's as we need to know how many SS capable ports are there for this. > > Signed-off-by: Krishna Kurapati <quic_kriskura@quicinc.com> > --- > drivers/usb/dwc3/core.c | 62 +++++++++++++++++++++++++++++++++++++++++ > drivers/usb/dwc3/core.h | 9 ++++++ > 2 files changed, 71 insertions(+) > > diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c > index f6689b731718..32ec05fc242b 100644 > --- a/drivers/usb/dwc3/core.c > +++ b/drivers/usb/dwc3/core.c > @@ -39,6 +39,7 @@ > #include "io.h" > > #include "debug.h" > +#include "../host/xhci-ext-caps.h" > > #define DWC3_DEFAULT_AUTOSUSPEND_DELAY 5000 /* ms */ > > @@ -1767,6 +1768,52 @@ static int dwc3_get_clocks(struct dwc3 *dwc) > return 0; > } > > +static int dwc3_read_port_info(struct dwc3 *dwc) > +{ > + void __iomem *base; > + u8 major_revision; > + u32 offset = 0; > + int ret = 0; > + u32 val; > + > + /* > + * Remap xHCI address space to access XHCI ext cap regs, > + * since it is needed to get port info. > + */ > + base = ioremap(dwc->xhci_resources[0].start, > + resource_size(&dwc->xhci_resources[0])); > + if (IS_ERR(base)) > + return PTR_ERR(base); > + > + do { > + offset = xhci_find_next_ext_cap(base, offset, > + XHCI_EXT_CAPS_PROTOCOL); > + > + if (!offset) > + break; > + > + val = readl(base + offset); > + major_revision = XHCI_EXT_PORT_MAJOR(val); > + > + val = readl(base + offset + 0x08); > + if (major_revision == 0x03) { > + dwc->num_usb3_ports += XHCI_EXT_PORT_COUNT(val); > + } else if (major_revision <= 0x02) { > + dwc->num_usb2_ports += XHCI_EXT_PORT_COUNT(val); > + } else { > + dev_err(dwc->dev, > + "Unrecognized port major revision %d\n", > + major_revision); > + } > + } while (1); > + > + dev_dbg(dwc->dev, "hs-ports: %u ss-ports: %u\n", > + dwc->num_usb2_ports, dwc->num_usb3_ports); > + > + iounmap(base); > + return ret; > +} > + > static int dwc3_probe(struct platform_device *pdev) > { > struct device *dev = &pdev->dev; > @@ -1774,6 +1821,7 @@ static int dwc3_probe(struct platform_device *pdev) > void __iomem *regs; > struct dwc3 *dwc; > int ret; > + unsigned int hw_mode; > > dwc = devm_kzalloc(dev, sizeof(*dwc), GFP_KERNEL); > if (!dwc) > @@ -1854,6 +1902,20 @@ static int dwc3_probe(struct platform_device *pdev) > goto err_disable_clks; > } > > + /* > + * Currently only DWC3 controllers that are host-only capable > + * support Multiport. > + */ > + hw_mode = DWC3_GHWPARAMS0_MODE(dwc->hwparams.hwparams0); > + if (hw_mode == DWC3_GHWPARAMS0_MODE_HOST) { > + ret = dwc3_read_port_info(dwc); > + if (ret) > + goto err_disable_clks; > + } else { > + dwc->num_usb2_ports = 1; > + dwc->num_usb3_ports = 1; > + } > + > spin_lock_init(&dwc->lock); > mutex_init(&dwc->mutex); > > diff --git a/drivers/usb/dwc3/core.h b/drivers/usb/dwc3/core.h > index 8b1295e4dcdd..42fb17aa66fa 100644 > --- a/drivers/usb/dwc3/core.h > +++ b/drivers/usb/dwc3/core.h > @@ -33,6 +33,10 @@ > > #include <linux/power_supply.h> > > +#define XHCI_EXT_PORT_MAJOR(x) (((x) >> 24) & 0xff) > +#define XHCI_EXT_PORT_MINOR(x) (((x) >> 16) & 0xff) > +#define XHCI_EXT_PORT_COUNT(x) (((x) >> 8) & 0xff) > + > #define DWC3_MSG_MAX 500 > > /* Global constants */ > @@ -1029,6 +1033,8 @@ struct dwc3_scratchpad_array { > * @usb3_phy: pointer to USB3 PHY > * @usb2_generic_phy: pointer to USB2 PHY > * @usb3_generic_phy: pointer to USB3 PHY > + * @num_usb2_ports: number of USB2 ports. > + * @num_usb3_ports: number of USB3 ports. > * @phys_ready: flag to indicate that PHYs are ready > * @ulpi: pointer to ulpi interface > * @ulpi_ready: flag to indicate that ULPI is initialized > @@ -1168,6 +1174,9 @@ struct dwc3 { > struct phy *usb2_generic_phy; > struct phy *usb3_generic_phy; > > + u8 num_usb2_ports; > + u8 num_usb3_ports; > + > bool phys_ready; > > struct ulpi *ulpi; > -- > 2.40.0 > Acked-by: Thinh Nguyen <Thinh.Nguyen@synopsys.com> Thanks, Thinh
On Wed, Jun 21, 2023, Krishna Kurapati wrote: > On some SoC's like SA8295P where the tertiary controller is host-only > capable, GEVTADDRHI/LO, GEVTSIZ, GEVTCOUNT registers are not accessible. > Trying to access them leads to a crash. > > For DRD/Peripheral supported controllers, event buffer setup is done > again in gadget_pullup. Skip setup or cleanup of event buffers if > controller is host-only capable. > > Suggested-by: Johan Hovold <johan@kernel.org> > Signed-off-by: Krishna Kurapati <quic_kriskura@quicinc.com> > --- > drivers/usb/dwc3/core.c | 11 +++++++++++ > 1 file changed, 11 insertions(+) > > diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c > index 32ec05fc242b..e1ebae54454f 100644 > --- a/drivers/usb/dwc3/core.c > +++ b/drivers/usb/dwc3/core.c > @@ -486,6 +486,11 @@ static void dwc3_free_event_buffers(struct dwc3 *dwc) > static int dwc3_alloc_event_buffers(struct dwc3 *dwc, unsigned int length) > { > struct dwc3_event_buffer *evt; > + unsigned int hw_mode; > + > + hw_mode = DWC3_GHWPARAMS0_MODE(dwc->hwparams.hwparams0); > + if (hw_mode == DWC3_GHWPARAMS0_MODE_HOST) > + return 0; This is a little awkward. Returning 0 here indicates that this function was successful, and the event buffers were allocated based on the function name. Do this check outside of dwc3_alloc_one_event_buffer() and specifically set dwc->ev_buf = NULL if that's the case. Thanks, Thinh > > evt = dwc3_alloc_one_event_buffer(dwc, length); > if (IS_ERR(evt)) { > @@ -507,6 +512,9 @@ int dwc3_event_buffers_setup(struct dwc3 *dwc) > { > struct dwc3_event_buffer *evt; > > + if (!dwc->ev_buf) > + return 0; > + > evt = dwc->ev_buf; > evt->lpos = 0; > dwc3_writel(dwc->regs, DWC3_GEVNTADRLO(0), > @@ -524,6 +532,9 @@ void dwc3_event_buffers_cleanup(struct dwc3 *dwc) > { > struct dwc3_event_buffer *evt; > > + if (!dwc->ev_buf) > + return; > + > evt = dwc->ev_buf; > > evt->lpos = 0; > -- > 2.40.0 >
On 21.06.2023 06:36, Krishna Kurapati wrote: > Add USB and DWC3 node for tertiary port of SC8280 along with multiport > IRQ's and phy's. This will be used as a base for SA8295P and SA8295-Ride > platforms. > > Signed-off-by: Krishna Kurapati <quic_kriskura@quicinc.com> > --- > arch/arm64/boot/dts/qcom/sc8280xp.dtsi | 77 ++++++++++++++++++++++++++ > 1 file changed, 77 insertions(+) > > diff --git a/arch/arm64/boot/dts/qcom/sc8280xp.dtsi b/arch/arm64/boot/dts/qcom/sc8280xp.dtsi > index 8fa9fbfe5d00..0dfa350ea3b3 100644 > --- a/arch/arm64/boot/dts/qcom/sc8280xp.dtsi > +++ b/arch/arm64/boot/dts/qcom/sc8280xp.dtsi > @@ -3013,6 +3013,83 @@ system-cache-controller@9200000 { > interrupts = <GIC_SPI 582 IRQ_TYPE_LEVEL_HIGH>; > }; > > + usb_2: usb@a4f8800 { > + compatible = "qcom,sc8280xp-dwc3-mp", "qcom,dwc3"; > + reg = <0 0x0a4f8800 0 0x400>; > + #address-cells = <2>; > + #size-cells = <2>; > + ranges; These three properties, please stick just before status > + > + 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"; Please make it one per line > + > + assigned-clocks = <&gcc GCC_USB30_MP_MOCK_UTMI_CLK>, > + <&gcc GCC_USB30_MP_MASTER_CLK>; > + assigned-clock-rates = <19200000>, <200000000>; And here > + > + interrupts-extended = <&pdc 127 IRQ_TYPE_EDGE_RISING>, > + <&pdc 126 IRQ_TYPE_EDGE_RISING>, > + <&pdc 129 IRQ_TYPE_EDGE_RISING>, > + <&pdc 128 IRQ_TYPE_EDGE_RISING>, > + <&pdc 131 IRQ_TYPE_EDGE_RISING>, > + <&pdc 130 IRQ_TYPE_EDGE_RISING>, > + <&pdc 133 IRQ_TYPE_EDGE_RISING>, > + <&pdc 132 IRQ_TYPE_EDGE_RISING>, > + <&pdc 16 IRQ_TYPE_LEVEL_HIGH>, > + <&pdc 17 IRQ_TYPE_LEVEL_HIGH>, Not a comment to the patch, but very nice that Qcom ensured every endpoint is wakeup-capable, this used not to be the case before :D > + <&intc GIC_SPI 130 IRQ_TYPE_LEVEL_HIGH>, > + <&intc GIC_SPI 135 IRQ_TYPE_LEVEL_HIGH>, > + <&intc GIC_SPI 857 IRQ_TYPE_LEVEL_HIGH>, > + <&intc GIC_SPI 856 IRQ_TYPE_LEVEL_HIGH>; > + Remove this newline > + interrupt-names = "dp1_hs_phy_irq", "dm1_hs_phy_irq", > + "dp2_hs_phy_irq", "dm2_hs_phy_irq", > + "dp3_hs_phy_irq", "dm3_hs_phy_irq", > + "dp4_hs_phy_irq", "dm4_hs_phy_irq", > + "ss1_phy_irq", "ss2_phy_irq", > + "pwr_event_1", > + "pwr_event_2", > + "pwr_event_3", > + "pwr_event_4"; Please make it one per line > + > + power-domains = <&gcc USB30_MP_GDSC>; > + required-opps = <&rpmhpd_opp_nom>; > + > + resets = <&gcc GCC_USB30_MP_BCR>; > + > + interconnects = <&aggre1_noc MASTER_USB3_MP 0 &mc_virt SLAVE_EBI1 0>, > + <&gem_noc MASTER_APPSS_PROC 0 &config_noc SLAVE_USB3_MP 0>; > + interconnect-names = "usb-ddr", "apps-usb"; > + > + wakeup-source; > + > + status = "disabled"; > + > + usb_2_dwc3: usb@a400000 { > + compatible = "snps,dwc3"; > + reg = <0 0x0a400000 0 0xcd00>; > + interrupts = <GIC_SPI 133 IRQ_TYPE_LEVEL_HIGH>; > + iommus = <&apps_smmu 0x800 0x0>; > + phys = <&usb_2_hsphy0>, <&usb_2_qmpphy0>, > + <&usb_2_hsphy1>, <&usb_2_qmpphy1>, > + <&usb_2_hsphy2>, > + <&usb_2_hsphy3>; And here > + phy-names = "usb2-port0", "usb3-port0", > + "usb2-port1", "usb3-port1", > + "usb2-port2", > + "usb2-port3"; And here Thanks for working on this! Konrad > + }; > + }; > + > usb_0: usb@a6f8800 { > compatible = "qcom,sc8280xp-dwc3", "qcom,dwc3"; > reg = <0 0x0a6f8800 0 0x400>;
On 21.06.2023 06:36, Krishna Kurapati wrote: > Enable tertiary controller for SA8295P (based on SC8280XP). > Add pinctrl support for usb ports to provide VBUS to connected peripherals. > > Signed-off-by: Krishna Kurapati <quic_kriskura@quicinc.com> > --- > arch/arm64/boot/dts/qcom/sa8295p-adp.dts | 53 ++++++++++++++++++++++++ > 1 file changed, 53 insertions(+) > > diff --git a/arch/arm64/boot/dts/qcom/sa8295p-adp.dts b/arch/arm64/boot/dts/qcom/sa8295p-adp.dts > index fd253942e5e5..8b24b3c55769 100644 > --- a/arch/arm64/boot/dts/qcom/sa8295p-adp.dts > +++ b/arch/arm64/boot/dts/qcom/sa8295p-adp.dts > @@ -9,6 +9,7 @@ > #include <dt-bindings/gpio/gpio.h> > #include <dt-bindings/regulator/qcom,rpmh-regulator.h> > #include <dt-bindings/spmi/spmi.h> > +#include <dt-bindings/pinctrl/qcom,pmic-gpio.h> > > #include "sa8540p.dtsi" > #include "sa8540p-pmics.dtsi" > @@ -584,6 +585,20 @@ &usb_1_qmpphy { > status = "okay"; > }; > > +&usb_2 { > + pinctrl-names = "default"; > + pinctrl-0 = <&usb2_en_state>, > + <&usb3_en_state>, > + <&usb4_en_state>, > + <&usb5_en_state>; Please put property-names after property > + > + status = "okay"; > +}; > + > +&usb_2_dwc3 { > + dr_mode = "host"; > +}; If I understood correctly, the MP controller is host-only. In that case, I'd say that putting this property in the SoC-dtsi node with an appropriate comment would be beneficial. lgtm otherwise Konrad > + > &usb_2_hsphy0 { > vdda-pll-supply = <&vreg_l5a>; > vdda18-supply = <&vreg_l7g>; > @@ -729,3 +744,41 @@ wake-n-pins { > }; > }; > }; > + > +&pmm8540c_gpios { > + usb2_en_state: usb2-en-state { > + pins = "gpio9"; > + function = "normal"; > + qcom,drive-strength = <PMIC_GPIO_STRENGTH_HIGH>; > + output-high; > + power-source = <0>; > + }; > +}; > + > +&pmm8540e_gpios { > + usb3_en_state: usb3-en-state { > + pins = "gpio5"; > + function = "normal"; > + qcom,drive-strength = <PMIC_GPIO_STRENGTH_HIGH>; > + output-high; > + power-source = <0>; > + }; > +}; > + > +&pmm8540g_gpios { > + usb4_en_state: usb4-en-state { > + pins = "gpio5"; > + function = "normal"; > + qcom,drive-strength = <PMIC_GPIO_STRENGTH_HIGH>; > + output-high; > + power-source = <0>; > + }; > + > + usb5_en_state: usb5-en-state { > + pins = "gpio9"; > + function = "normal"; > + qcom,drive-strength = <PMIC_GPIO_STRENGTH_HIGH>; > + output-high; > + power-source = <0>; > + }; > +};
On 21.06.2023 06:36, Krishna Kurapati wrote: > 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> > [Krishna: Rebased on top of usb-next] If that's your only change to this patch, you should have kept the Author: field unchanged. > Signed-off-by: Krishna Kurapati <quic_kriskura@quicinc.com> > --- Same comments as patch 9 Konrad > arch/arm64/boot/dts/qcom/sa8540p-ride.dts | 22 ++++++++++++++++++++++ > 1 file changed, 22 insertions(+) > > diff --git a/arch/arm64/boot/dts/qcom/sa8540p-ride.dts b/arch/arm64/boot/dts/qcom/sa8540p-ride.dts > index 24fa449d48a6..53d47593306e 100644 > --- a/arch/arm64/boot/dts/qcom/sa8540p-ride.dts > +++ b/arch/arm64/boot/dts/qcom/sa8540p-ride.dts > @@ -309,6 +309,19 @@ &usb_2_qmpphy0 { > status = "okay"; > }; > > +&usb_2 { > + pinctrl-names = "default"; > + pinctrl-0 = <&usb2_en_state>; > + > + status = "okay"; > +}; > + > +&usb_2_dwc3 { > + dr_mode = "host"; > + phy-names = "usb2-port0", "usb3-port0"; > + phys = <&usb_2_hsphy0>, <&usb_2_qmpphy0>; > +}; > + > &xo_board_clk { > clock-frequency = <38400000>; > }; > @@ -401,4 +414,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, Jun 21, 2023, 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> > [Krishna: Modifed logic for generic phy and rebased the patch] > Signed-off-by: Krishna Kurapati <quic_kriskura@quicinc.com> > --- > drivers/usb/dwc3/core.c | 252 ++++++++++++++++++++++++++++------------ > drivers/usb/dwc3/core.h | 11 +- > drivers/usb/dwc3/drd.c | 15 ++- > 3 files changed, 192 insertions(+), 86 deletions(-) > > diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c > index e1ebae54454f..2ac72525de7d 100644 > --- a/drivers/usb/dwc3/core.c > +++ b/drivers/usb/dwc3/core.c > @@ -120,10 +120,11 @@ void dwc3_set_prtcap(struct dwc3 *dwc, u32 mode) > static void __dwc3_set_mode(struct work_struct *work) > { > struct dwc3 *dwc = work_to_dwc(work); > + u32 desired_dr_role; > unsigned long flags; > int ret; > u32 reg; > - u32 desired_dr_role; > + int i; > > mutex_lock(&dwc->mutex); > spin_lock_irqsave(&dwc->lock, flags); > @@ -201,8 +202,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_usb2_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; > @@ -217,8 +220,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) > @@ -587,22 +590,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 > @@ -657,9 +652,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); > + > + return 0; > +} > + > +static int dwc3_hs_phy_setup(struct dwc3 *dwc, int index) > +{ > + unsigned int hw_mode; > + u32 reg; > > - reg = dwc3_readl(dwc->regs, DWC3_GUSB2PHYCFG(0)); > + 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)) { > @@ -671,7 +676,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)) > @@ -738,7 +743,35 @@ static int dwc3_phy_setup(struct dwc3 *dwc) > if (dwc->ulpi_ext_vbus_drv) > reg |= DWC3_GUSB2PHYCFG_ULPIEXTVBUSDRV; > > - 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_usb3_ports; i++) { > + ret = dwc3_ss_phy_setup(dwc, i); > + if (ret) > + return ret; > + } > + > + for (i = 0; i < dwc->num_usb2_ports; i++) { > + ret = dwc3_hs_phy_setup(dwc, i); > + if (ret) > + return ret; > + } > > return 0; > } > @@ -746,23 +779,34 @@ static int dwc3_phy_setup(struct dwc3 *dwc) > static int dwc3_phy_init(struct dwc3 *dwc) > { > int ret; > + int i; > + int j; > > usb_phy_init(dwc->usb2_phy); > usb_phy_init(dwc->usb3_phy); > > - ret = phy_init(dwc->usb2_generic_phy); > - if (ret < 0) > - goto err_shutdown_usb3_phy; > + for (i = 0; i < dwc->num_usb2_ports; i++) { > + ret = phy_init(dwc->usb2_generic_phy[i]); > + if (ret < 0) > + goto err_exit_usb2_phy; > + } > > - ret = phy_init(dwc->usb3_generic_phy); > - if (ret < 0) > - goto err_exit_usb2_phy; > + for (i = 0; i < dwc->num_usb2_ports; i++) { > + ret = phy_init(dwc->usb3_generic_phy[i]); > + if (ret < 0) > + goto err_exit_usb3_phy; > + } > > return 0; > > +err_exit_usb3_phy: > + for (j = i-1; j >= 0; j--) Minor nits, can we add spacing around the '-' for here and other places. Checkpatch should be able to catch this. > + phy_exit(dwc->usb3_generic_phy[j]); > + i = dwc->num_usb2_ports; > err_exit_usb2_phy: > - phy_exit(dwc->usb2_generic_phy); > -err_shutdown_usb3_phy: > + for (j = i-1; j >= 0; j--) > + phy_exit(dwc->usb2_generic_phy[j]); > + > usb_phy_shutdown(dwc->usb3_phy); > usb_phy_shutdown(dwc->usb2_phy); > > @@ -771,8 +815,12 @@ static int dwc3_phy_init(struct dwc3 *dwc) > > static void dwc3_phy_exit(struct dwc3 *dwc) > { > - phy_exit(dwc->usb3_generic_phy); > - phy_exit(dwc->usb2_generic_phy); > + int i; > + > + for (i = 0; i < dwc->num_usb2_ports; i++) { > + phy_exit(dwc->usb3_generic_phy[i]); > + phy_exit(dwc->usb2_generic_phy[i]); > + } > > usb_phy_shutdown(dwc->usb3_phy); > usb_phy_shutdown(dwc->usb2_phy); > @@ -781,23 +829,34 @@ static void dwc3_phy_exit(struct dwc3 *dwc) > static int dwc3_phy_power_on(struct dwc3 *dwc) > { > int ret; > + int i; > + int j; > > usb_phy_set_suspend(dwc->usb2_phy, 0); > usb_phy_set_suspend(dwc->usb3_phy, 0); > > - ret = phy_power_on(dwc->usb2_generic_phy); > - if (ret < 0) > - goto err_suspend_usb3_phy; > + for (i = 0; i < dwc->num_usb2_ports; i++) { > + ret = phy_power_on(dwc->usb2_generic_phy[i]); > + if (ret < 0) > + goto err_power_off_usb2_phy; > + } > > - ret = phy_power_on(dwc->usb3_generic_phy); > - if (ret < 0) > - goto err_power_off_usb2_phy; > + for (i = 0; i < dwc->num_usb2_ports; i++) { > + ret = phy_power_on(dwc->usb3_generic_phy[i]); > + if (ret < 0) > + goto err_power_off_usb3_phy; > + } > > return 0; > > +err_power_off_usb3_phy: > + for (j = i-1; j >= 0; j--) > + phy_power_off(dwc->usb3_generic_phy[i]); > + i = dwc->num_usb2_ports; > err_power_off_usb2_phy: > - phy_power_off(dwc->usb2_generic_phy); > -err_suspend_usb3_phy: > + for (j = i-1; j >= 0; j--) > + phy_power_off(dwc->usb2_generic_phy[i]); > + > usb_phy_set_suspend(dwc->usb3_phy, 1); > usb_phy_set_suspend(dwc->usb2_phy, 1); > > @@ -806,8 +865,12 @@ static int dwc3_phy_power_on(struct dwc3 *dwc) > > static void dwc3_phy_power_off(struct dwc3 *dwc) > { > - phy_power_off(dwc->usb3_generic_phy); > - phy_power_off(dwc->usb2_generic_phy); > + int i; > + > + for (i = 0; i < dwc->num_usb2_ports; i++) { > + phy_power_off(dwc->usb3_generic_phy[i]); > + phy_power_off(dwc->usb2_generic_phy[i]); > + } > > usb_phy_set_suspend(dwc->usb3_phy, 1); > usb_phy_set_suspend(dwc->usb2_phy, 1); > @@ -1080,6 +1143,7 @@ static int dwc3_core_init(struct dwc3 *dwc) > unsigned int hw_mode; > u32 reg; > int ret; > + int i; > > hw_mode = DWC3_GHWPARAMS0_MODE(dwc->hwparams.hwparams0); > > @@ -1123,15 +1187,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_usb3_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_usb2_ports; i++) { > + reg = dwc3_readl(dwc->regs, DWC3_GUSB2PHYCFG(i)); > + reg |= DWC3_GUSB2PHYCFG_SUSPHY; > + dwc3_writel(dwc->regs, DWC3_GUSB2PHYCFG(i), reg); > + } > } > } > > @@ -1290,7 +1358,9 @@ static int dwc3_core_get_phy(struct dwc3 *dwc) > { > struct device *dev = dwc->dev; > struct device_node *node = dev->of_node; > + char phy_name[11]; > int ret; > + int i; > > if (node) { > dwc->usb2_phy = devm_usb_get_phy_by_phandle(dev, "usb-phy", 0); > @@ -1316,22 +1386,36 @@ 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 (ret == -ENOSYS || ret == -ENODEV) > - dwc->usb2_generic_phy = NULL; > + for (i = 0; i < dwc->num_usb2_ports; i++) { > + if (dwc->num_usb2_ports == 1) > + sprintf(phy_name, "usb2-phy"); > else > - return dev_err_probe(dev, ret, "no usb2 phy configured\n"); > - } > + sprintf(phy_name, "usb2-port%d", i); > + > + dwc->usb2_generic_phy[i] = devm_phy_get(dev, phy_name); > + if (IS_ERR(dwc->usb2_generic_phy[i])) { > + ret = PTR_ERR(dwc->usb2_generic_phy[i]); > + if (ret == -ENOSYS || ret == -ENODEV) > + dwc->usb2_generic_phy[i] = NULL; > + else > + return dev_err_probe(dev, ret, > + "no %s phy configured\n", phy_name); > + } > > - dwc->usb3_generic_phy = devm_phy_get(dev, "usb3-phy"); > - if (IS_ERR(dwc->usb3_generic_phy)) { > - ret = PTR_ERR(dwc->usb3_generic_phy); > - if (ret == -ENOSYS || ret == -ENODEV) > - dwc->usb3_generic_phy = NULL; > + if (dwc->num_usb2_ports == 1) > + sprintf(phy_name, "usb3-phy"); > else > - return dev_err_probe(dev, ret, "no usb3 phy configured\n"); > + sprintf(phy_name, "usb3-port%d", i); > + > + dwc->usb3_generic_phy[i] = devm_phy_get(dev, phy_name); > + if (IS_ERR(dwc->usb3_generic_phy[i])) { > + ret = PTR_ERR(dwc->usb3_generic_phy[i]); > + if (ret == -ENOSYS || ret == -ENODEV) > + dwc->usb3_generic_phy[i] = NULL; > + else > + return dev_err_probe(dev, ret, > + "no %s phy configured\n", phy_name); > + } > } > > return 0; > @@ -1341,6 +1425,7 @@ static int dwc3_core_init_mode(struct dwc3 *dwc) > { > struct device *dev = dwc->dev; > int ret; > + int i; > > switch (dwc->dr_mode) { > case USB_DR_MODE_PERIPHERAL: > @@ -1348,8 +1433,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) > @@ -1360,8 +1445,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_usb2_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) > @@ -1821,6 +1908,9 @@ static int dwc3_read_port_info(struct dwc3 *dwc) > dev_dbg(dwc->dev, "hs-ports: %u ss-ports: %u\n", > dwc->num_usb2_ports, dwc->num_usb3_ports); > > + if (dwc->num_usb2_ports > DWC3_MAX_PORTS) > + ret = -ENOMEM; > + > iounmap(base); > return ret; > } > @@ -2057,6 +2147,7 @@ static int dwc3_suspend_common(struct dwc3 *dwc, pm_message_t msg) > { > unsigned long flags; > u32 reg; > + int i; > > switch (dwc->current_dr_role) { > case DWC3_GCTL_PRTCAP_DEVICE: > @@ -2075,17 +2166,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_usb2_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_usb2_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 */ > @@ -2115,6 +2210,7 @@ static int dwc3_resume_common(struct dwc3 *dwc, pm_message_t msg) > unsigned long flags; > int ret; > u32 reg; > + int i; > > switch (dwc->current_dr_role) { > case DWC3_GCTL_PRTCAP_DEVICE: > @@ -2134,17 +2230,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_usb2_ports; i++) { Nit: Extra spacing before the "for"? > + 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_usb2_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 42fb17aa66fa..b2bab23ca22b 100644 > --- a/drivers/usb/dwc3/core.h > +++ b/drivers/usb/dwc3/core.h > @@ -37,6 +37,9 @@ > #define XHCI_EXT_PORT_MINOR(x) (((x) >> 16) & 0xff) > #define XHCI_EXT_PORT_COUNT(x) (((x) >> 8) & 0xff) > > +/* Number of ports supported by a multiport controller */ > +#define DWC3_MAX_PORTS 4 > + > #define DWC3_MSG_MAX 500 > > /* Global constants */ > @@ -1031,8 +1034,8 @@ 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 > + * @usb2_generic_phy: pointer to array of USB2 PHY > + * @usb3_generic_phy: pointer to array of USB3 PHY > * @num_usb2_ports: number of USB2 ports. > * @num_usb3_ports: number of USB3 ports. > * @phys_ready: flag to indicate that PHYs are ready > @@ -1171,8 +1174,8 @@ struct dwc3 { > struct usb_phy *usb2_phy; > struct usb_phy *usb3_phy; > > - struct phy *usb2_generic_phy; > - struct phy *usb3_generic_phy; > + struct phy *usb2_generic_phy[DWC3_MAX_PORTS]; > + struct phy *usb3_generic_phy[DWC3_MAX_PORTS]; > > u8 num_usb2_ports; > u8 num_usb3_ports; > diff --git a/drivers/usb/dwc3/drd.c b/drivers/usb/dwc3/drd.c > index 039bf241769a..18a247ff75ac 100644 > --- a/drivers/usb/dwc3/drd.c > +++ b/drivers/usb/dwc3/drd.c > @@ -327,10 +327,11 @@ static void dwc3_otg_device_exit(struct dwc3 *dwc) > > void dwc3_otg_update(struct dwc3 *dwc, bool ignore_idstatus) > { > + unsigned long flags; > int ret; > u32 reg; > int id; > - unsigned long flags; > + int i; > > if (dwc->dr_mode != USB_DR_MODE_OTG) > return; > @@ -386,9 +387,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_usb2_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 +403,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.40.0 > After fixing the minor nits mentioned above, you can add my Ack: Acked-by: Thinh Nguyen <Thinh.Nguyen@synopsys.com> Thanks, Thinh
On 6/24/2023 4:12 AM, Konrad Dybcio wrote: > On 21.06.2023 06:36, Krishna Kurapati wrote: >> 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> >> [Krishna: Rebased on top of usb-next] > If that's your only change to this patch, you should have kept the > Author: field unchanged. > Sure, will do reset-author and resubmit the patch next version. >> Signed-off-by: Krishna Kurapati <quic_kriskura@quicinc.com> >> --- > Same comments as patch 9 > > Konrad Thanks for the review. Regards, Krishna, >> arch/arm64/boot/dts/qcom/sa8540p-ride.dts | 22 ++++++++++++++++++++++ >> 1 file changed, 22 insertions(+) >> >> diff --git a/arch/arm64/boot/dts/qcom/sa8540p-ride.dts b/arch/arm64/boot/dts/qcom/sa8540p-ride.dts >> index 24fa449d48a6..53d47593306e 100644 >> --- a/arch/arm64/boot/dts/qcom/sa8540p-ride.dts >> +++ b/arch/arm64/boot/dts/qcom/sa8540p-ride.dts >> @@ -309,6 +309,19 @@ &usb_2_qmpphy0 { >> status = "okay"; >> }; >> >> +&usb_2 { >> + pinctrl-names = "default"; >> + pinctrl-0 = <&usb2_en_state>; >> + >> + status = "okay"; >> +}; >> + >> +&usb_2_dwc3 { >> + dr_mode = "host"; >> + phy-names = "usb2-port0", "usb3-port0"; >> + phys = <&usb_2_hsphy0>, <&usb_2_qmpphy0>; >> +}; >> + >> &xo_board_clk { >> clock-frequency = <38400000>; >> }; >> @@ -401,4 +414,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 6/24/2023 4:09 AM, Konrad Dybcio wrote: > On 21.06.2023 06:36, Krishna Kurapati wrote: >> Add USB and DWC3 node for tertiary port of SC8280 along with multiport >> IRQ's and phy's. This will be used as a base for SA8295P and SA8295-Ride >> platforms. >> >> Signed-off-by: Krishna Kurapati <quic_kriskura@quicinc.com> >> --- >> arch/arm64/boot/dts/qcom/sc8280xp.dtsi | 77 ++++++++++++++++++++++++++ >> 1 file changed, 77 insertions(+) >> >> diff --git a/arch/arm64/boot/dts/qcom/sc8280xp.dtsi b/arch/arm64/boot/dts/qcom/sc8280xp.dtsi >> index 8fa9fbfe5d00..0dfa350ea3b3 100644 >> --- a/arch/arm64/boot/dts/qcom/sc8280xp.dtsi >> +++ b/arch/arm64/boot/dts/qcom/sc8280xp.dtsi >> @@ -3013,6 +3013,83 @@ system-cache-controller@9200000 { >> interrupts = <GIC_SPI 582 IRQ_TYPE_LEVEL_HIGH>; >> }; >> >> + usb_2: usb@a4f8800 { >> + compatible = "qcom,sc8280xp-dwc3-mp", "qcom,dwc3"; >> + reg = <0 0x0a4f8800 0 0x400>; > >> + #address-cells = <2>; >> + #size-cells = <2>; >> + ranges; > These three properties, please stick just before status > >> + >> + 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"; > Please make it one per line > >> + >> + assigned-clocks = <&gcc GCC_USB30_MP_MOCK_UTMI_CLK>, >> + <&gcc GCC_USB30_MP_MASTER_CLK>; >> + assigned-clock-rates = <19200000>, <200000000>; > And here > >> + >> + interrupts-extended = <&pdc 127 IRQ_TYPE_EDGE_RISING>, >> + <&pdc 126 IRQ_TYPE_EDGE_RISING>, >> + <&pdc 129 IRQ_TYPE_EDGE_RISING>, >> + <&pdc 128 IRQ_TYPE_EDGE_RISING>, >> + <&pdc 131 IRQ_TYPE_EDGE_RISING>, >> + <&pdc 130 IRQ_TYPE_EDGE_RISING>, >> + <&pdc 133 IRQ_TYPE_EDGE_RISING>, >> + <&pdc 132 IRQ_TYPE_EDGE_RISING>, >> + <&pdc 16 IRQ_TYPE_LEVEL_HIGH>, >> + <&pdc 17 IRQ_TYPE_LEVEL_HIGH>, > Not a comment to the patch, but very nice that Qcom ensured every > endpoint is wakeup-capable, this used not to be the case before :D Yes wakeup is supported by all ports now, but I didn't make those changes now as I wanted to keep driver code diff minimal and don't need wakeup support for the product currently. But for sure, will update driver code to handle wakeup on all ports in near future. Regards, Krishna, > >> + <&intc GIC_SPI 130 IRQ_TYPE_LEVEL_HIGH>, >> + <&intc GIC_SPI 135 IRQ_TYPE_LEVEL_HIGH>, >> + <&intc GIC_SPI 857 IRQ_TYPE_LEVEL_HIGH>, >> + <&intc GIC_SPI 856 IRQ_TYPE_LEVEL_HIGH>; >> + > Remove this newline > >> + interrupt-names = "dp1_hs_phy_irq", "dm1_hs_phy_irq", >> + "dp2_hs_phy_irq", "dm2_hs_phy_irq", >> + "dp3_hs_phy_irq", "dm3_hs_phy_irq", >> + "dp4_hs_phy_irq", "dm4_hs_phy_irq", >> + "ss1_phy_irq", "ss2_phy_irq", >> + "pwr_event_1", >> + "pwr_event_2", >> + "pwr_event_3", >> + "pwr_event_4"; > Please make it one per line > >> + >> + power-domains = <&gcc USB30_MP_GDSC>; >> + required-opps = <&rpmhpd_opp_nom>; >> + >> + resets = <&gcc GCC_USB30_MP_BCR>; >> + >> + interconnects = <&aggre1_noc MASTER_USB3_MP 0 &mc_virt SLAVE_EBI1 0>, >> + <&gem_noc MASTER_APPSS_PROC 0 &config_noc SLAVE_USB3_MP 0>; >> + interconnect-names = "usb-ddr", "apps-usb"; >> + >> + wakeup-source; >> + >> + status = "disabled"; >> + >> + usb_2_dwc3: usb@a400000 { >> + compatible = "snps,dwc3"; >> + reg = <0 0x0a400000 0 0xcd00>; >> + interrupts = <GIC_SPI 133 IRQ_TYPE_LEVEL_HIGH>; >> + iommus = <&apps_smmu 0x800 0x0>; >> + phys = <&usb_2_hsphy0>, <&usb_2_qmpphy0>, >> + <&usb_2_hsphy1>, <&usb_2_qmpphy1>, >> + <&usb_2_hsphy2>, >> + <&usb_2_hsphy3>; > And here >> + phy-names = "usb2-port0", "usb3-port0", >> + "usb2-port1", "usb3-port1", >> + "usb2-port2", >> + "usb2-port3"; > And here > > Thanks for working on this! > > Konrad >> + }; >> + }; >> + >> usb_0: usb@a6f8800 { >> compatible = "qcom,sc8280xp-dwc3", "qcom,dwc3"; >> reg = <0 0x0a6f8800 0 0x400>;
On 6/24/2023 4:25 AM, Thinh Nguyen wrote: > On Wed, Jun 21, 2023, 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> >> [Krishna: Modifed logic for generic phy and rebased the patch] >> Signed-off-by: Krishna Kurapati <quic_kriskura@quicinc.com> >> --- >> drivers/usb/dwc3/core.c | 252 ++++++++++++++++++++++++++++------------ >> drivers/usb/dwc3/core.h | 11 +- >> drivers/usb/dwc3/drd.c | 15 ++- >> 3 files changed, 192 insertions(+), 86 deletions(-) >> >> diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c >> index e1ebae54454f..2ac72525de7d 100644 >> --- a/drivers/usb/dwc3/core.c >> +++ b/drivers/usb/dwc3/core.c >> @@ -120,10 +120,11 @@ void dwc3_set_prtcap(struct dwc3 *dwc, u32 mode) >> static void __dwc3_set_mode(struct work_struct *work) >> { >> struct dwc3 *dwc = work_to_dwc(work); >> + u32 desired_dr_role; >> unsigned long flags; >> int ret; >> u32 reg; >> - u32 desired_dr_role; >> + int i; >> >> mutex_lock(&dwc->mutex); >> spin_lock_irqsave(&dwc->lock, flags); >> @@ -201,8 +202,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_usb2_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; >> @@ -217,8 +220,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) >> @@ -587,22 +590,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 >> @@ -657,9 +652,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); >> + >> + return 0; >> +} >> + >> +static int dwc3_hs_phy_setup(struct dwc3 *dwc, int index) >> +{ >> + unsigned int hw_mode; >> + u32 reg; >> >> - reg = dwc3_readl(dwc->regs, DWC3_GUSB2PHYCFG(0)); >> + 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)) { >> @@ -671,7 +676,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)) >> @@ -738,7 +743,35 @@ static int dwc3_phy_setup(struct dwc3 *dwc) >> if (dwc->ulpi_ext_vbus_drv) >> reg |= DWC3_GUSB2PHYCFG_ULPIEXTVBUSDRV; >> >> - 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_usb3_ports; i++) { >> + ret = dwc3_ss_phy_setup(dwc, i); >> + if (ret) >> + return ret; >> + } >> + >> + for (i = 0; i < dwc->num_usb2_ports; i++) { >> + ret = dwc3_hs_phy_setup(dwc, i); >> + if (ret) >> + return ret; >> + } >> >> return 0; >> } >> @@ -746,23 +779,34 @@ static int dwc3_phy_setup(struct dwc3 *dwc) >> static int dwc3_phy_init(struct dwc3 *dwc) >> { >> int ret; >> + int i; >> + int j; >> >> usb_phy_init(dwc->usb2_phy); >> usb_phy_init(dwc->usb3_phy); >> >> - ret = phy_init(dwc->usb2_generic_phy); >> - if (ret < 0) >> - goto err_shutdown_usb3_phy; >> + for (i = 0; i < dwc->num_usb2_ports; i++) { >> + ret = phy_init(dwc->usb2_generic_phy[i]); >> + if (ret < 0) >> + goto err_exit_usb2_phy; >> + } >> >> - ret = phy_init(dwc->usb3_generic_phy); >> - if (ret < 0) >> - goto err_exit_usb2_phy; >> + for (i = 0; i < dwc->num_usb2_ports; i++) { >> + ret = phy_init(dwc->usb3_generic_phy[i]); >> + if (ret < 0) >> + goto err_exit_usb3_phy; >> + } >> >> return 0; >> >> +err_exit_usb3_phy: >> + for (j = i-1; j >= 0; j--) > > Minor nits, can we add spacing around the '-' for here and other places. > Checkpatch should be able to catch this. > Hi Thinh, Checkpatch actually caught this, but the only reason I didn't fix this single nit is it looked like too much spaces in a single for loop, (I know it doesn't sound convincing). I will fix this nit in next version. Regards, Krishna, >> + phy_exit(dwc->usb3_generic_phy[j]); >> + i = dwc->num_usb2_ports; >> err_exit_usb2_phy: >> - phy_exit(dwc->usb2_generic_phy); >> -err_shutdown_usb3_phy: >> + for (j = i-1; j >= 0; j--) >> + phy_exit(dwc->usb2_generic_phy[j]); >> + >> usb_phy_shutdown(dwc->usb3_phy); >> usb_phy_shutdown(dwc->usb2_phy); >> >> @@ -771,8 +815,12 @@ static int dwc3_phy_init(struct dwc3 *dwc) >> >> static void dwc3_phy_exit(struct dwc3 *dwc) >> { >> - phy_exit(dwc->usb3_generic_phy); >> - phy_exit(dwc->usb2_generic_phy); >> + int i; >> + >> + for (i = 0; i < dwc->num_usb2_ports; i++) { >> + phy_exit(dwc->usb3_generic_phy[i]); >> + phy_exit(dwc->usb2_generic_phy[i]); >> + } >> >> usb_phy_shutdown(dwc->usb3_phy); >> usb_phy_shutdown(dwc->usb2_phy); >> @@ -781,23 +829,34 @@ static void dwc3_phy_exit(struct dwc3 *dwc) >> static int dwc3_phy_power_on(struct dwc3 *dwc) >> { >> int ret; >> + int i; >> + int j; >> >> usb_phy_set_suspend(dwc->usb2_phy, 0); >> usb_phy_set_suspend(dwc->usb3_phy, 0); >> >> - ret = phy_power_on(dwc->usb2_generic_phy); >> - if (ret < 0) >> - goto err_suspend_usb3_phy; >> + for (i = 0; i < dwc->num_usb2_ports; i++) { >> + ret = phy_power_on(dwc->usb2_generic_phy[i]); >> + if (ret < 0) >> + goto err_power_off_usb2_phy; >> + } >> >> - ret = phy_power_on(dwc->usb3_generic_phy); >> - if (ret < 0) >> - goto err_power_off_usb2_phy; >> + for (i = 0; i < dwc->num_usb2_ports; i++) { >> + ret = phy_power_on(dwc->usb3_generic_phy[i]); >> + if (ret < 0) >> + goto err_power_off_usb3_phy; >> + } >> >> return 0; >> >> +err_power_off_usb3_phy: >> + for (j = i-1; j >= 0; j--) >> + phy_power_off(dwc->usb3_generic_phy[i]); >> + i = dwc->num_usb2_ports; >> err_power_off_usb2_phy: >> - phy_power_off(dwc->usb2_generic_phy); >> -err_suspend_usb3_phy: >> + for (j = i-1; j >= 0; j--) >> + phy_power_off(dwc->usb2_generic_phy[i]); >> + >> usb_phy_set_suspend(dwc->usb3_phy, 1); >> usb_phy_set_suspend(dwc->usb2_phy, 1); >> >> @@ -806,8 +865,12 @@ static int dwc3_phy_power_on(struct dwc3 *dwc) >> >> static void dwc3_phy_power_off(struct dwc3 *dwc) >> { >> - phy_power_off(dwc->usb3_generic_phy); >> - phy_power_off(dwc->usb2_generic_phy); >> + int i; >> + >> + for (i = 0; i < dwc->num_usb2_ports; i++) { >> + phy_power_off(dwc->usb3_generic_phy[i]); >> + phy_power_off(dwc->usb2_generic_phy[i]); >> + } >> >> usb_phy_set_suspend(dwc->usb3_phy, 1); >> usb_phy_set_suspend(dwc->usb2_phy, 1); >> @@ -1080,6 +1143,7 @@ static int dwc3_core_init(struct dwc3 *dwc) >> unsigned int hw_mode; >> u32 reg; >> int ret; >> + int i; >> >> hw_mode = DWC3_GHWPARAMS0_MODE(dwc->hwparams.hwparams0); >> >> @@ -1123,15 +1187,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_usb3_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_usb2_ports; i++) { >> + reg = dwc3_readl(dwc->regs, DWC3_GUSB2PHYCFG(i)); >> + reg |= DWC3_GUSB2PHYCFG_SUSPHY; >> + dwc3_writel(dwc->regs, DWC3_GUSB2PHYCFG(i), reg); >> + } >> } >> } >> >> @@ -1290,7 +1358,9 @@ static int dwc3_core_get_phy(struct dwc3 *dwc) >> { >> struct device *dev = dwc->dev; >> struct device_node *node = dev->of_node; >> + char phy_name[11]; >> int ret; >> + int i; >> >> if (node) { >> dwc->usb2_phy = devm_usb_get_phy_by_phandle(dev, "usb-phy", 0); >> @@ -1316,22 +1386,36 @@ 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 (ret == -ENOSYS || ret == -ENODEV) >> - dwc->usb2_generic_phy = NULL; >> + for (i = 0; i < dwc->num_usb2_ports; i++) { >> + if (dwc->num_usb2_ports == 1) >> + sprintf(phy_name, "usb2-phy"); >> else >> - return dev_err_probe(dev, ret, "no usb2 phy configured\n"); >> - } >> + sprintf(phy_name, "usb2-port%d", i); >> + >> + dwc->usb2_generic_phy[i] = devm_phy_get(dev, phy_name); >> + if (IS_ERR(dwc->usb2_generic_phy[i])) { >> + ret = PTR_ERR(dwc->usb2_generic_phy[i]); >> + if (ret == -ENOSYS || ret == -ENODEV) >> + dwc->usb2_generic_phy[i] = NULL; >> + else >> + return dev_err_probe(dev, ret, >> + "no %s phy configured\n", phy_name); >> + } >> >> - dwc->usb3_generic_phy = devm_phy_get(dev, "usb3-phy"); >> - if (IS_ERR(dwc->usb3_generic_phy)) { >> - ret = PTR_ERR(dwc->usb3_generic_phy); >> - if (ret == -ENOSYS || ret == -ENODEV) >> - dwc->usb3_generic_phy = NULL; >> + if (dwc->num_usb2_ports == 1) >> + sprintf(phy_name, "usb3-phy"); >> else >> - return dev_err_probe(dev, ret, "no usb3 phy configured\n"); >> + sprintf(phy_name, "usb3-port%d", i); >> + >> + dwc->usb3_generic_phy[i] = devm_phy_get(dev, phy_name); >> + if (IS_ERR(dwc->usb3_generic_phy[i])) { >> + ret = PTR_ERR(dwc->usb3_generic_phy[i]); >> + if (ret == -ENOSYS || ret == -ENODEV) >> + dwc->usb3_generic_phy[i] = NULL; >> + else >> + return dev_err_probe(dev, ret, >> + "no %s phy configured\n", phy_name); >> + } >> } >> >> return 0; >> @@ -1341,6 +1425,7 @@ static int dwc3_core_init_mode(struct dwc3 *dwc) >> { >> struct device *dev = dwc->dev; >> int ret; >> + int i; >> >> switch (dwc->dr_mode) { >> case USB_DR_MODE_PERIPHERAL: >> @@ -1348,8 +1433,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) >> @@ -1360,8 +1445,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_usb2_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) >> @@ -1821,6 +1908,9 @@ static int dwc3_read_port_info(struct dwc3 *dwc) >> dev_dbg(dwc->dev, "hs-ports: %u ss-ports: %u\n", >> dwc->num_usb2_ports, dwc->num_usb3_ports); >> >> + if (dwc->num_usb2_ports > DWC3_MAX_PORTS) >> + ret = -ENOMEM; >> + >> iounmap(base); >> return ret; >> } >> @@ -2057,6 +2147,7 @@ static int dwc3_suspend_common(struct dwc3 *dwc, pm_message_t msg) >> { >> unsigned long flags; >> u32 reg; >> + int i; >> >> switch (dwc->current_dr_role) { >> case DWC3_GCTL_PRTCAP_DEVICE: >> @@ -2075,17 +2166,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_usb2_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_usb2_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 */ >> @@ -2115,6 +2210,7 @@ static int dwc3_resume_common(struct dwc3 *dwc, pm_message_t msg) >> unsigned long flags; >> int ret; >> u32 reg; >> + int i; >> >> switch (dwc->current_dr_role) { >> case DWC3_GCTL_PRTCAP_DEVICE: >> @@ -2134,17 +2230,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_usb2_ports; i++) { > > Nit: Extra spacing before the "for"? > >> + 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_usb2_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 42fb17aa66fa..b2bab23ca22b 100644 >> --- a/drivers/usb/dwc3/core.h >> +++ b/drivers/usb/dwc3/core.h >> @@ -37,6 +37,9 @@ >> #define XHCI_EXT_PORT_MINOR(x) (((x) >> 16) & 0xff) >> #define XHCI_EXT_PORT_COUNT(x) (((x) >> 8) & 0xff) >> >> +/* Number of ports supported by a multiport controller */ >> +#define DWC3_MAX_PORTS 4 >> + >> #define DWC3_MSG_MAX 500 >> >> /* Global constants */ >> @@ -1031,8 +1034,8 @@ 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 >> + * @usb2_generic_phy: pointer to array of USB2 PHY >> + * @usb3_generic_phy: pointer to array of USB3 PHY >> * @num_usb2_ports: number of USB2 ports. >> * @num_usb3_ports: number of USB3 ports. >> * @phys_ready: flag to indicate that PHYs are ready >> @@ -1171,8 +1174,8 @@ struct dwc3 { >> struct usb_phy *usb2_phy; >> struct usb_phy *usb3_phy; >> >> - struct phy *usb2_generic_phy; >> - struct phy *usb3_generic_phy; >> + struct phy *usb2_generic_phy[DWC3_MAX_PORTS]; >> + struct phy *usb3_generic_phy[DWC3_MAX_PORTS]; >> >> u8 num_usb2_ports; >> u8 num_usb3_ports; >> diff --git a/drivers/usb/dwc3/drd.c b/drivers/usb/dwc3/drd.c >> index 039bf241769a..18a247ff75ac 100644 >> --- a/drivers/usb/dwc3/drd.c >> +++ b/drivers/usb/dwc3/drd.c >> @@ -327,10 +327,11 @@ static void dwc3_otg_device_exit(struct dwc3 *dwc) >> >> void dwc3_otg_update(struct dwc3 *dwc, bool ignore_idstatus) >> { >> + unsigned long flags; >> int ret; >> u32 reg; >> int id; >> - unsigned long flags; >> + int i; >> >> if (dwc->dr_mode != USB_DR_MODE_OTG) >> return; >> @@ -386,9 +387,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_usb2_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 +403,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.40.0 >> > > After fixing the minor nits mentioned above, you can add my Ack: > > Acked-by: Thinh Nguyen <Thinh.Nguyen@synopsys.com> > > Thanks, > Thinh
On 6/24/2023 3:57 AM, Thinh Nguyen wrote: > On Wed, Jun 21, 2023, Krishna Kurapati wrote: >> On some SoC's like SA8295P where the tertiary controller is host-only >> capable, GEVTADDRHI/LO, GEVTSIZ, GEVTCOUNT registers are not accessible. >> Trying to access them leads to a crash. >> >> For DRD/Peripheral supported controllers, event buffer setup is done >> again in gadget_pullup. Skip setup or cleanup of event buffers if >> controller is host-only capable. >> >> Suggested-by: Johan Hovold <johan@kernel.org> >> Signed-off-by: Krishna Kurapati <quic_kriskura@quicinc.com> >> --- >> drivers/usb/dwc3/core.c | 11 +++++++++++ >> 1 file changed, 11 insertions(+) >> >> diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c >> index 32ec05fc242b..e1ebae54454f 100644 >> --- a/drivers/usb/dwc3/core.c >> +++ b/drivers/usb/dwc3/core.c >> @@ -486,6 +486,11 @@ static void dwc3_free_event_buffers(struct dwc3 *dwc) >> static int dwc3_alloc_event_buffers(struct dwc3 *dwc, unsigned int length) >> { >> struct dwc3_event_buffer *evt; >> + unsigned int hw_mode; >> + >> + hw_mode = DWC3_GHWPARAMS0_MODE(dwc->hwparams.hwparams0); >> + if (hw_mode == DWC3_GHWPARAMS0_MODE_HOST) >> + return 0; > > This is a little awkward. Returning 0 here indicates that this function > was successful, and the event buffers were allocated based on the > function name. Do this check outside of dwc3_alloc_one_event_buffer() > and specifically set dwc->ev_buf = NULL if that's the case. > Hi Thinh, Apologies, I didn't understand the comment properly. I thought we were supposed to return 0 here if we fulfill the goal of this function (allocate if we are drd/gadget and don't allocate if we are host mode only). If we return a non zero error here, probe would fail as this call will be done only for host only controllers during probe and nowhere else. Are you suggesting we move this check to dwc3_alloc_one_event_buffer call ? Regards, Krishna, >> >> evt = dwc3_alloc_one_event_buffer(dwc, length); >> if (IS_ERR(evt)) { >> @@ -507,6 +512,9 @@ int dwc3_event_buffers_setup(struct dwc3 *dwc) >> { >> struct dwc3_event_buffer *evt; >> >> + if (!dwc->ev_buf) >> + return 0; >> + >> evt = dwc->ev_buf; >> evt->lpos = 0; >> dwc3_writel(dwc->regs, DWC3_GEVNTADRLO(0), >> @@ -524,6 +532,9 @@ void dwc3_event_buffers_cleanup(struct dwc3 *dwc) >> { >> struct dwc3_event_buffer *evt; >> >> + if (!dwc->ev_buf) >> + return; >> + >> evt = dwc->ev_buf; >> >> evt->lpos = 0; >> -- >> 2.40.0
On Mon, Jun 26, 2023, Thinh Nguyen wrote: > On Sat, Jun 24, 2023, Krishna Kurapati PSSNV wrote: > > > > > > On 6/24/2023 3:57 AM, Thinh Nguyen wrote: > > > On Wed, Jun 21, 2023, Krishna Kurapati wrote: > > > > On some SoC's like SA8295P where the tertiary controller is host-only > > > > capable, GEVTADDRHI/LO, GEVTSIZ, GEVTCOUNT registers are not accessible. > > > > Trying to access them leads to a crash. > > > > > > > > For DRD/Peripheral supported controllers, event buffer setup is done > > > > again in gadget_pullup. Skip setup or cleanup of event buffers if > > > > controller is host-only capable. > > > > > > > > Suggested-by: Johan Hovold <johan@kernel.org> > > > > Signed-off-by: Krishna Kurapati <quic_kriskura@quicinc.com> > > > > --- > > > > drivers/usb/dwc3/core.c | 11 +++++++++++ > > > > 1 file changed, 11 insertions(+) > > > > > > > > diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c > > > > index 32ec05fc242b..e1ebae54454f 100644 > > > > --- a/drivers/usb/dwc3/core.c > > > > +++ b/drivers/usb/dwc3/core.c > > > > @@ -486,6 +486,11 @@ static void dwc3_free_event_buffers(struct dwc3 *dwc) > > > > static int dwc3_alloc_event_buffers(struct dwc3 *dwc, unsigned int length) > > > > { > > > > struct dwc3_event_buffer *evt; > > > > + unsigned int hw_mode; > > > > + > > > > + hw_mode = DWC3_GHWPARAMS0_MODE(dwc->hwparams.hwparams0); > > > > + if (hw_mode == DWC3_GHWPARAMS0_MODE_HOST) > > > > + return 0; > > > > > > This is a little awkward. Returning 0 here indicates that this function > > > was successful, and the event buffers were allocated based on the > > > function name. Do this check outside of dwc3_alloc_one_event_buffer() > > > and specifically set dwc->ev_buf = NULL if that's the case. > > > > > Hi Thinh, > > > > Apologies, I didn't understand the comment properly. > > > > I thought we were supposed to return 0 here if we fulfill the goal of this > > function (allocate if we are drd/gadget and don't allocate if we are host > > mode only). > > The name of the function implies that it returns 0 if it allocated the > event buffer. If there are multiple conditions to the function returning > 0 here, then we should document it. > > > > > If we return a non zero error here, probe would fail as this call will be > > done only for host only controllers during probe and nowhere else. > > > > Are you suggesting we move this check to dwc3_alloc_one_event_buffer call > > ? > > > > Regards, > > Krishna, > > > > I'm suggesting to move the check to the caller of > dwc3_alloc_one_event_buffer(). Something like this so that it's > self-documented: > > diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c > index 0beaab932e7d..bba82792bf6f 100644 > --- a/drivers/usb/dwc3/core.c > +++ b/drivers/usb/dwc3/core.c > @@ -1773,6 +1773,7 @@ static int dwc3_probe(struct platform_device *pdev) > struct resource *res, dwc_res; > void __iomem *regs; > struct dwc3 *dwc; > + unsigned int hw_mode; > int ret; > > dwc = devm_kzalloc(dev, sizeof(*dwc), GFP_KERNEL); > @@ -1854,11 +1855,16 @@ static int dwc3_probe(struct platform_device *pdev) > > pm_runtime_forbid(dev); > > - ret = dwc3_alloc_event_buffers(dwc, DWC3_EVENT_BUFFERS_SIZE); > - if (ret) { > - dev_err(dwc->dev, "failed to allocate event buffers\n"); > - ret = -ENOMEM; > - goto err_allow_rpm; > + hw_mode = DWC3_GHWPARAMS0_MODE(dwc->hwparams.hwparams0); > + if (hw_mode == DWC3_GHWPARAMS0_MODE_HOST) { > + dwc->ev_buf = NULL; > + } else { > + ret = dwc3_alloc_event_buffers(dwc, DWC3_EVENT_BUFFERS_SIZE); > + if (ret) { > + dev_err(dwc->dev, "failed to allocate event buffers\n"); > + ret = -ENOMEM; > + goto err_allow_rpm; > + } > } > > dwc->edev = dwc3_get_extcon(dwc); > Actually, please ignore. there's already a document there, I missed that for some reason. What you did is fine. Though, I don't see the condition for ev_buf = NULL anywhere. Can you add that for clarity? Thanks, Thinh
On Wed, Jun 21, 2023 at 10:06:21AM +0530, Krishna Kurapati wrote: > Currently host-only capable DWC3 controllers support Multiport. > Temporarily map XHCI address space for host-only controllers and parse > XHCI Extended Capabilities registers to read number of usb2 ports and > usb3 ports present on multiport controller. Each USB Port is at least HS > capable. > > The port info for usb2 and usb3 phy are identified as num_usb2_ports > and num_usb3_ports. The intention is as follows: > > Wherever we need to perform phy operations like: > > LOOP_OVER_NUMBER_OF_AVAILABLE_PORTS() > { > phy_set_mode(dwc->usb2_generic_phy[i], PHY_MODE_USB_HOST); > phy_set_mode(dwc->usb3_generic_phy[i], PHY_MODE_USB_HOST); > } > > If number of usb2 ports is 3, loop can go from index 0-2 for > usb2_generic_phy. If number of usb3-ports is 2, we don't know for sure, > if the first 2 ports are SS capable or some other ports like (2 and 3) > are SS capable. So instead, num_usb2_ports is used to loop around all > phy's (both hs and ss) for performing phy operations. If any > usb3_generic_phy turns out to be NULL, phy operation just bails out. > > num_usb3_ports is used to modify GUSB3PIPECTL registers while setting up > phy's as we need to know how many SS capable ports are there for this. > > Signed-off-by: Krishna Kurapati <quic_kriskura@quicinc.com> > --- > drivers/usb/dwc3/core.c | 62 +++++++++++++++++++++++++++++++++++++++++ > drivers/usb/dwc3/core.h | 9 ++++++ > 2 files changed, 71 insertions(+) > > diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c > index f6689b731718..32ec05fc242b 100644 > --- a/drivers/usb/dwc3/core.c > +++ b/drivers/usb/dwc3/core.c > @@ -39,6 +39,7 @@ > #include "io.h" > > #include "debug.h" > +#include "../host/xhci-ext-caps.h" > > #define DWC3_DEFAULT_AUTOSUSPEND_DELAY 5000 /* ms */ > > @@ -1767,6 +1768,52 @@ static int dwc3_get_clocks(struct dwc3 *dwc) > return 0; > } > > +static int dwc3_read_port_info(struct dwc3 *dwc) > +{ > + void __iomem *base; > + u8 major_revision; > + u32 offset = 0; > + int ret = 0; ret is never modified, so drop and return 0 unconditionally below. You can add it back later in the series when you start using it. > + u32 val; > + > + /* > + * Remap xHCI address space to access XHCI ext cap regs, > + * since it is needed to get port info. > + */ > + base = ioremap(dwc->xhci_resources[0].start, > + resource_size(&dwc->xhci_resources[0])); > + if (IS_ERR(base)) > + return PTR_ERR(base); > + > + do { > + offset = xhci_find_next_ext_cap(base, offset, > + XHCI_EXT_CAPS_PROTOCOL); > + You can drop this newline. > + if (!offset) > + break; > + > + val = readl(base + offset); > + major_revision = XHCI_EXT_PORT_MAJOR(val); > + > + val = readl(base + offset + 0x08); > + if (major_revision == 0x03) { > + dwc->num_usb3_ports += XHCI_EXT_PORT_COUNT(val); > + } else if (major_revision <= 0x02) { > + dwc->num_usb2_ports += XHCI_EXT_PORT_COUNT(val); > + } else { > + dev_err(dwc->dev, > + "Unrecognized port major revision %d\n", > + major_revision); > + } > + } while (1); > + > + dev_dbg(dwc->dev, "hs-ports: %u ss-ports: %u\n", > + dwc->num_usb2_ports, dwc->num_usb3_ports); > + > + iounmap(base); Nit: I'd add a newline here. > + return ret; > +} > + > static int dwc3_probe(struct platform_device *pdev) > { > struct device *dev = &pdev->dev; > @@ -1774,6 +1821,7 @@ static int dwc3_probe(struct platform_device *pdev) > void __iomem *regs; > struct dwc3 *dwc; > int ret; > + unsigned int hw_mode; > > dwc = devm_kzalloc(dev, sizeof(*dwc), GFP_KERNEL); > if (!dwc) > @@ -1854,6 +1902,20 @@ static int dwc3_probe(struct platform_device *pdev) > goto err_disable_clks; > } > > + /* > + * Currently only DWC3 controllers that are host-only capable > + * support Multiport. > + */ > + hw_mode = DWC3_GHWPARAMS0_MODE(dwc->hwparams.hwparams0); > + if (hw_mode == DWC3_GHWPARAMS0_MODE_HOST) { > + ret = dwc3_read_port_info(dwc); > + if (ret) > + goto err_disable_clks; > + } else { > + dwc->num_usb2_ports = 1; > + dwc->num_usb3_ports = 1; > + } > + > spin_lock_init(&dwc->lock); > mutex_init(&dwc->mutex); > > diff --git a/drivers/usb/dwc3/core.h b/drivers/usb/dwc3/core.h > index 8b1295e4dcdd..42fb17aa66fa 100644 > --- a/drivers/usb/dwc3/core.h > +++ b/drivers/usb/dwc3/core.h > @@ -33,6 +33,10 @@ > > #include <linux/power_supply.h> > > +#define XHCI_EXT_PORT_MAJOR(x) (((x) >> 24) & 0xff) > +#define XHCI_EXT_PORT_MINOR(x) (((x) >> 16) & 0xff) > +#define XHCI_EXT_PORT_COUNT(x) (((x) >> 8) & 0xff) Again, don't copy defines from xhci. Looks like these should be moved to the xhci-ext-caps.h header along with struct xhci_protocol_caps. > + > #define DWC3_MSG_MAX 500 > > /* Global constants */ > @@ -1029,6 +1033,8 @@ struct dwc3_scratchpad_array { > * @usb3_phy: pointer to USB3 PHY > * @usb2_generic_phy: pointer to USB2 PHY > * @usb3_generic_phy: pointer to USB3 PHY > + * @num_usb2_ports: number of USB2 ports. > + * @num_usb3_ports: number of USB3 ports. Again, please drop the full stops ('.'). Johan
On Wed, Jun 21, 2023 at 10:06:23AM +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> > [Krishna: Modifed logic for generic phy and rebased the patch] > Signed-off-by: Krishna Kurapati <quic_kriskura@quicinc.com> As I already said: If Harsh is the primary author you need to add a From: line at the beginning of the patch. Either way, you need his SoB as well as your Co-developed-by tag. All this is documented under Documentation/process/ somewhere. The above is missing a From line and two Co-developed-by tags at least. > --- > drivers/usb/dwc3/core.c | 252 ++++++++++++++++++++++++++++------------ > drivers/usb/dwc3/core.h | 11 +- > drivers/usb/dwc3/drd.c | 15 ++- > 3 files changed, 192 insertions(+), 86 deletions(-) > > diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c > index e1ebae54454f..2ac72525de7d 100644 > --- a/drivers/usb/dwc3/core.c > +++ b/drivers/usb/dwc3/core.c > @@ -120,10 +120,11 @@ void dwc3_set_prtcap(struct dwc3 *dwc, u32 mode) > static void __dwc3_set_mode(struct work_struct *work) > { > struct dwc3 *dwc = work_to_dwc(work); > + u32 desired_dr_role; > unsigned long flags; > int ret; > u32 reg; > - u32 desired_dr_role; This is an unrelated change. Just add int i at the end. > + int i; > > mutex_lock(&dwc->mutex); > spin_lock_irqsave(&dwc->lock, flags); > @@ -746,23 +779,34 @@ static int dwc3_phy_setup(struct dwc3 *dwc) > static int dwc3_phy_init(struct dwc3 *dwc) > { > int ret; > + int i; > + int j; > > usb_phy_init(dwc->usb2_phy); > usb_phy_init(dwc->usb3_phy); > > - ret = phy_init(dwc->usb2_generic_phy); > - if (ret < 0) > - goto err_shutdown_usb3_phy; > + for (i = 0; i < dwc->num_usb2_ports; i++) { > + ret = phy_init(dwc->usb2_generic_phy[i]); > + if (ret < 0) > + goto err_exit_usb2_phy; > + } > > - ret = phy_init(dwc->usb3_generic_phy); > - if (ret < 0) > - goto err_exit_usb2_phy; > + for (i = 0; i < dwc->num_usb2_ports; i++) { > + ret = phy_init(dwc->usb3_generic_phy[i]); > + if (ret < 0) > + goto err_exit_usb3_phy; > + } > > return 0; > > +err_exit_usb3_phy: > + for (j = i-1; j >= 0; j--) Missing spaces around - here and below. > + phy_exit(dwc->usb3_generic_phy[j]); > + i = dwc->num_usb2_ports; > err_exit_usb2_phy: > - phy_exit(dwc->usb2_generic_phy); > -err_shutdown_usb3_phy: > + for (j = i-1; j >= 0; j--) > + phy_exit(dwc->usb2_generic_phy[j]); > + Again: The above is probably better implemented as a *single* loop over num_usb2_ports where you enable each USB2 and USB3 PHY. On errors you use the loop index to disable the already enabled PHYs in reverse order below (after disabling the USB2 PHY if USB3 phy init fails). with emphasis on "single" added. > usb_phy_shutdown(dwc->usb3_phy); > usb_phy_shutdown(dwc->usb2_phy); > @@ -781,23 +829,34 @@ static void dwc3_phy_exit(struct dwc3 *dwc) > static int dwc3_phy_power_on(struct dwc3 *dwc) > { > int ret; > + int i; > + int j; > > usb_phy_set_suspend(dwc->usb2_phy, 0); > usb_phy_set_suspend(dwc->usb3_phy, 0); > > - ret = phy_power_on(dwc->usb2_generic_phy); > - if (ret < 0) > - goto err_suspend_usb3_phy; > + for (i = 0; i < dwc->num_usb2_ports; i++) { > + ret = phy_power_on(dwc->usb2_generic_phy[i]); > + if (ret < 0) > + goto err_power_off_usb2_phy; > + } > > - ret = phy_power_on(dwc->usb3_generic_phy); > - if (ret < 0) > - goto err_power_off_usb2_phy; > + for (i = 0; i < dwc->num_usb2_ports; i++) { > + ret = phy_power_on(dwc->usb3_generic_phy[i]); > + if (ret < 0) > + goto err_power_off_usb3_phy; > + } Again: These loops should be merged too as for phy_init. > return 0; > > +err_power_off_usb3_phy: > + for (j = i-1; j >= 0; j--) > + phy_power_off(dwc->usb3_generic_phy[i]); > + i = dwc->num_usb2_ports; > err_power_off_usb2_phy: > - phy_power_off(dwc->usb2_generic_phy); > -err_suspend_usb3_phy: > + for (j = i-1; j >= 0; j--) > + phy_power_off(dwc->usb2_generic_phy[i]); > + > usb_phy_set_suspend(dwc->usb3_phy, 1); > usb_phy_set_suspend(dwc->usb2_phy, 1); > > @@ -1290,7 +1358,9 @@ static int dwc3_core_get_phy(struct dwc3 *dwc) > { > struct device *dev = dwc->dev; > struct device_node *node = dev->of_node; > + char phy_name[11]; > int ret; > + int i; > > if (node) { > dwc->usb2_phy = devm_usb_get_phy_by_phandle(dev, "usb-phy", 0); > @@ -1316,22 +1386,36 @@ 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 (ret == -ENOSYS || ret == -ENODEV) > - dwc->usb2_generic_phy = NULL; > + for (i = 0; i < dwc->num_usb2_ports; i++) { > + if (dwc->num_usb2_ports == 1) > + sprintf(phy_name, "usb2-phy"); > else > - return dev_err_probe(dev, ret, "no usb2 phy configured\n"); > - } > + sprintf(phy_name, "usb2-port%d", i); > + > + dwc->usb2_generic_phy[i] = devm_phy_get(dev, phy_name); > + if (IS_ERR(dwc->usb2_generic_phy[i])) { > + ret = PTR_ERR(dwc->usb2_generic_phy[i]); > + if (ret == -ENOSYS || ret == -ENODEV) > + dwc->usb2_generic_phy[i] = NULL; > + else > + return dev_err_probe(dev, ret, > + "no %s phy configured\n", phy_name); I still believe "failed to lookup phy %s" is better. > + } > > - dwc->usb3_generic_phy = devm_phy_get(dev, "usb3-phy"); > - if (IS_ERR(dwc->usb3_generic_phy)) { > - ret = PTR_ERR(dwc->usb3_generic_phy); > - if (ret == -ENOSYS || ret == -ENODEV) > - dwc->usb3_generic_phy = NULL; > + if (dwc->num_usb2_ports == 1) > + sprintf(phy_name, "usb3-phy"); > else > - return dev_err_probe(dev, ret, "no usb3 phy configured\n"); > + sprintf(phy_name, "usb3-port%d", i); > + > + dwc->usb3_generic_phy[i] = devm_phy_get(dev, phy_name); > + if (IS_ERR(dwc->usb3_generic_phy[i])) { > + ret = PTR_ERR(dwc->usb3_generic_phy[i]); > + if (ret == -ENOSYS || ret == -ENODEV) > + dwc->usb3_generic_phy[i] = NULL; > + else > + return dev_err_probe(dev, ret, > + "no %s phy configured\n", phy_name); Same here. > + } > } > > return 0; > @@ -1821,6 +1908,9 @@ static int dwc3_read_port_info(struct dwc3 *dwc) > dev_dbg(dwc->dev, "hs-ports: %u ss-ports: %u\n", > dwc->num_usb2_ports, dwc->num_usb3_ports); > > + if (dwc->num_usb2_ports > DWC3_MAX_PORTS) > + ret = -ENOMEM; You also need to add a check for num_usb3_ports as I already mentioned. > + > iounmap(base); > return ret; > } > diff --git a/drivers/usb/dwc3/core.h b/drivers/usb/dwc3/core.h > index 42fb17aa66fa..b2bab23ca22b 100644 > --- a/drivers/usb/dwc3/core.h > +++ b/drivers/usb/dwc3/core.h > @@ -37,6 +37,9 @@ > #define XHCI_EXT_PORT_MINOR(x) (((x) >> 16) & 0xff) > #define XHCI_EXT_PORT_COUNT(x) (((x) >> 8) & 0xff) > > +/* Number of ports supported by a multiport controller */ > +#define DWC3_MAX_PORTS 4 You did not answer my question about whether this was an arbitrary implementation limit (i.e. just reflecting the only currently supported multiport controller)? > + > #define DWC3_MSG_MAX 500 > > /* Global constants */ > @@ -1031,8 +1034,8 @@ 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 > + * @usb2_generic_phy: pointer to array of USB2 PHY > + * @usb3_generic_phy: pointer to array of USB3 PHY > * @num_usb2_ports: number of USB2 ports. > * @num_usb3_ports: number of USB3 ports. > * @phys_ready: flag to indicate that PHYs are ready > @@ -1171,8 +1174,8 @@ struct dwc3 { > struct usb_phy *usb2_phy; > struct usb_phy *usb3_phy; > > - struct phy *usb2_generic_phy; > - struct phy *usb3_generic_phy; > + struct phy *usb2_generic_phy[DWC3_MAX_PORTS]; > + struct phy *usb3_generic_phy[DWC3_MAX_PORTS]; > > u8 num_usb2_ports; > u8 num_usb3_ports; > diff --git a/drivers/usb/dwc3/drd.c b/drivers/usb/dwc3/drd.c > index 039bf241769a..18a247ff75ac 100644 > --- a/drivers/usb/dwc3/drd.c > +++ b/drivers/usb/dwc3/drd.c > @@ -327,10 +327,11 @@ static void dwc3_otg_device_exit(struct dwc3 *dwc) > > void dwc3_otg_update(struct dwc3 *dwc, bool ignore_idstatus) > { > + unsigned long flags; > int ret; > u32 reg; > int id; > - unsigned long flags; > + int i; > > if (dwc->dr_mode != USB_DR_MODE_OTG) > return; > @@ -386,9 +387,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_usb2_ports; i++) { > + if (dwc->usb2_generic_phy[i]) > + phy_set_mode(dwc->usb2_generic_phy[i], > + PHY_MODE_USB_HOST); While not strictly necessary, adding bracket around multiline statements is usually preferred as it improves readability. > + } > } > break; > case DWC3_OTG_ROLE_DEVICE: > @@ -400,8 +403,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); Same here, but this is probably better just left at 85 columns after removing the line break. > ret = dwc3_gadget_init(dwc); > if (ret) Johan
On Wed, Jun 21, 2023 at 10:06:24AM +0530, Krishna Kurapati wrote: > Add support to read Multiport IRQ's related to quad port controller > of SA8295 Device. Please use a more descriptive summary and commit message; "read" is to vague. You're looking up interrupts from the devicetree. Also this should not just be about SA8295. > Signed-off-by: Krishna Kurapati <quic_kriskura@quicinc.com> > --- > drivers/usb/dwc3/dwc3-qcom.c | 108 +++++++++++++++++++++++++++++------ > 1 file changed, 91 insertions(+), 17 deletions(-) > > diff --git a/drivers/usb/dwc3/dwc3-qcom.c b/drivers/usb/dwc3/dwc3-qcom.c > index 3de43df6bbe8..3ab48a6925fe 100644 > --- a/drivers/usb/dwc3/dwc3-qcom.c > +++ b/drivers/usb/dwc3/dwc3-qcom.c > @@ -74,9 +74,9 @@ struct dwc3_qcom { > struct reset_control *resets; > > int hs_phy_irq; > - int dp_hs_phy_irq; > - int dm_hs_phy_irq; > - int ss_phy_irq; > + int dp_hs_phy_irq[4]; > + int dm_hs_phy_irq[4]; > + int ss_phy_irq[2]; As has already been pointed out, you should use a define for these. And you already have DWC3_MAX_PORTS. The driver should not be hardcoding the fact that there are only two SS ports on this particular SoC that you're interested in. > enum usb_device_speed usb2_speed; > > struct extcon_dev *edev; > @@ -535,6 +535,80 @@ static int dwc3_qcom_get_irq(struct platform_device *pdev, > return ret; > } > > +static int dwc3_qcom_setup_mp_irq(struct platform_device *pdev) > +{ > + struct dwc3_qcom *qcom = platform_get_drvdata(pdev); > + char irq_name[15]; > + int irq; > + int ret; > + int i; > + > + for (i = 0; i < 4; i++) { DWC3_MAX_PORTS here too and similar below. > + if (qcom->dp_hs_phy_irq[i]) > + continue; This is not very nice. You should try to integrate the current lookup code as I told you to do with the PHY lookups. That is, use a single loop for all HS/SS IRQs, and pick the legacy name if the number of ports is 1. Of course, you added the xhci capability parsing to the core driver so that information is not yet available, but you need it in the glue driver also... As I mentioned earlier, you can infer the number of ports from the interrupt names. Alternatively, you can infer it from the compatible string. In any case, you should not need to ways to determine the same information in the glue driver, then in the core part, and then yet again in the xhci driver... > + > + sprintf(irq_name, "dp%d_hs_phy_irq", i+1); Spaces around binary operators. Does not checkpatch warn about that? > + irq = dwc3_qcom_get_irq(pdev, irq_name, -1); > + if (irq > 0) { > + irq_set_status_flags(irq, IRQ_NOAUTOEN); > + ret = devm_request_threaded_irq(qcom->dev, irq, NULL, > + qcom_dwc3_resume_irq, > + IRQF_TRIGGER_HIGH | IRQF_ONESHOT, > + irq_name, qcom); > + if (ret) { > + dev_err(qcom->dev, "%s failed: %d\n", irq_name, ret); > + return ret; > + } > + } > + > + qcom->dp_hs_phy_irq[i] = irq; > + } > + > + for (i = 0; i < 4; i++) { > + if (qcom->dm_hs_phy_irq[i]) > + continue; > + > + sprintf(irq_name, "dm%d_hs_phy_irq", i+1); > + irq = dwc3_qcom_get_irq(pdev, irq_name, -1); > + if (irq > 0) { > + irq_set_status_flags(irq, IRQ_NOAUTOEN); > + ret = devm_request_threaded_irq(qcom->dev, irq, NULL, > + qcom_dwc3_resume_irq, > + IRQF_TRIGGER_HIGH | IRQF_ONESHOT, > + irq_name, qcom); > + if (ret) { > + dev_err(qcom->dev, "%s failed: %d\n", irq_name, ret); > + return ret; > + } > + } > + > + qcom->dm_hs_phy_irq[i] = irq; > + } > + > + for (i = 0; i < 2; i++) { > + if (qcom->ss_phy_irq[i]) > + continue; > + > + sprintf(irq_name, "ss%d_phy_irq", i+1); > + irq = dwc3_qcom_get_irq(pdev, irq_name, -1); > + if (irq > 0) { > + irq_set_status_flags(irq, IRQ_NOAUTOEN); > + ret = devm_request_threaded_irq(qcom->dev, irq, NULL, > + qcom_dwc3_resume_irq, > + IRQF_TRIGGER_HIGH | IRQF_ONESHOT, > + irq_name, qcom); > + if (ret) { > + dev_err(qcom->dev, "%s failed: %d\n", irq_name, ret); > + return ret; > + } > + } > + > + qcom->ss_phy_irq[i] = irq; > + } So the above should all be merged in either a single helper looking up all the interrupts for a port and resused for the non-MP case. > + > + return 0; > +} > + > static int dwc3_qcom_setup_irq(struct platform_device *pdev) > { > struct dwc3_qcom *qcom = platform_get_drvdata(pdev); > @@ -570,7 +644,7 @@ static int dwc3_qcom_setup_irq(struct platform_device *pdev) > dev_err(qcom->dev, "dp_hs_phy_irq failed: %d\n", ret); > return ret; > } > - qcom->dp_hs_phy_irq = irq; > + qcom->dp_hs_phy_irq[0] = irq; > } > > irq = dwc3_qcom_get_irq(pdev, "dm_hs_phy_irq", > @@ -585,7 +659,7 @@ static int dwc3_qcom_setup_irq(struct platform_device *pdev) > dev_err(qcom->dev, "dm_hs_phy_irq failed: %d\n", ret); > return ret; > } > - qcom->dm_hs_phy_irq = irq; > + qcom->dm_hs_phy_irq[0] = irq; > } > > irq = dwc3_qcom_get_irq(pdev, "ss_phy_irq", > @@ -600,10 +674,10 @@ static int dwc3_qcom_setup_irq(struct platform_device *pdev) > dev_err(qcom->dev, "ss_phy_irq failed: %d\n", ret); > return ret; > } > - qcom->ss_phy_irq = irq; > + qcom->ss_phy_irq[0] = irq; > } > > - return 0; > + return dwc3_qcom_setup_mp_irq(pdev);; Stray ; > } > > static int dwc3_qcom_clk_init(struct dwc3_qcom *qcom, int count) Johan
On Wed, Jun 21, 2023 at 10:06:25AM +0530, Krishna Kurapati wrote: > QCOM SoC SA8295P's tertiary quad port controller supports 2 HS+SS > ports and 2 HS only ports. Add support for configuring PWR_EVENT_IRQ's > for all the ports during suspend/resume. Please be more specific here. You don't seem to be configuring anything. > Signed-off-by: Krishna Kurapati <quic_kriskura@quicinc.com> > --- > drivers/usb/dwc3/dwc3-qcom.c | 48 +++++++++++++++++++++++++++++++----- > 1 file changed, 42 insertions(+), 6 deletions(-) > > diff --git a/drivers/usb/dwc3/dwc3-qcom.c b/drivers/usb/dwc3/dwc3-qcom.c > index 3ab48a6925fe..699485a85233 100644 > --- a/drivers/usb/dwc3/dwc3-qcom.c > +++ b/drivers/usb/dwc3/dwc3-qcom.c > @@ -37,7 +37,11 @@ > #define PIPE3_PHYSTATUS_SW BIT(3) > #define PIPE_UTMI_CLK_DIS BIT(8) > > -#define PWR_EVNT_IRQ_STAT_REG 0x58 > +#define PWR_EVNT_IRQ1_STAT_REG 0x58 > +#define PWR_EVNT_IRQ2_STAT_REG 0x1dc > +#define PWR_EVNT_IRQ3_STAT_REG 0x228 > +#define PWR_EVNT_IRQ4_STAT_REG 0x238 > + > #define PWR_EVNT_LPM_IN_L2_MASK BIT(4) > #define PWR_EVNT_LPM_OUT_L2_MASK BIT(5) > > @@ -93,6 +97,13 @@ struct dwc3_qcom { > struct icc_path *icc_path_apps; > }; > > +static u32 pwr_evnt_irq_stat_reg_offset[4] = { > + PWR_EVNT_IRQ1_STAT_REG, > + PWR_EVNT_IRQ2_STAT_REG, > + PWR_EVNT_IRQ3_STAT_REG, > + PWR_EVNT_IRQ4_STAT_REG, > +}; > + > static inline void dwc3_qcom_setbits(void __iomem *base, u32 offset, u32 val) > { > u32 reg; > @@ -417,17 +428,37 @@ static void dwc3_qcom_enable_interrupts(struct dwc3_qcom *qcom) > dwc3_qcom_enable_wakeup_irq(qcom->ss_phy_irq[0], 0); > } > > +static u8 dwc3_qcom_get_port_info(struct dwc3_qcom *qcom) "port_info" is not very specific, call it get_num_ports() or similar. > +{ > + struct dwc3 __maybe_unused *dwc = platform_get_drvdata(qcom->dwc3); __maybe unused makes no sense here. > + > + if (dwc3_qcom_is_host(qcom)) > + return dwc->num_usb2_ports; Here you're accessing the core driver data again, which we want to avoid going forward so this at least warrants a FIXME to rework this as well. > + > + /* > + * If not in host mode, either dwc was not probed > + * or we are in device mode, either case checking for > + * only first pwr event irq would suffice. Rewrap comment at 80 columns. > + */ > + > + return 1; > +} > + > static int dwc3_qcom_suspend(struct dwc3_qcom *qcom, bool wakeup) > { > u32 val; > int i, ret; > + u8 num_ports; Move first. > if (qcom->is_suspended) > return 0; > > - val = readl(qcom->qscratch_base + PWR_EVNT_IRQ_STAT_REG); > - if (!(val & PWR_EVNT_LPM_IN_L2_MASK)) > + num_ports = dwc3_qcom_get_port_info(qcom); > + for (i = 0; i < num_ports; i++) { > + val = readl(qcom->qscratch_base + pwr_evnt_irq_stat_reg_offset[i]); > + if (!(val & PWR_EVNT_LPM_IN_L2_MASK)) > dev_err(qcom->dev, "HS-PHY not in L2\n"); This line is not indented properly. Make sure to run checkpatch before submitting so that I don't have to point out things like this again. > + } > > for (i = qcom->num_clocks - 1; i >= 0; i--) > clk_disable_unprepare(qcom->clks[i]); > @@ -452,12 +483,14 @@ static int dwc3_qcom_suspend(struct dwc3_qcom *qcom, bool wakeup) > > static int dwc3_qcom_resume(struct dwc3_qcom *qcom, bool wakeup) > { > + int num_ports; > int ret; > int i; > > if (!qcom->is_suspended) > return 0; > > + num_ports = dwc3_qcom_get_port_info(qcom); Move below to where you use num_ports. > if (dwc3_qcom_is_host(qcom) && wakeup) > dwc3_qcom_disable_interrupts(qcom); > > @@ -474,9 +507,12 @@ static int dwc3_qcom_resume(struct dwc3_qcom *qcom, bool wakeup) > if (ret) > dev_warn(qcom->dev, "failed to enable interconnect: %d\n", ret); > > - /* Clear existing events from PHY related to L2 in/out */ No need to move the comment. > - dwc3_qcom_setbits(qcom->qscratch_base, PWR_EVNT_IRQ_STAT_REG, > - PWR_EVNT_LPM_IN_L2_MASK | PWR_EVNT_LPM_OUT_L2_MASK); > + for (i = 0; i < num_ports; i++) { > + /* Clear existing events from PHY related to L2 in/out */ > + dwc3_qcom_setbits(qcom->qscratch_base, > + pwr_evnt_irq_stat_reg_offset[i], > + PWR_EVNT_LPM_IN_L2_MASK | PWR_EVNT_LPM_OUT_L2_MASK); Indent continuation lines at least two tabs further than the previous line. > + } > > qcom->is_suspended = false; Johan
On Sat, Jun 24, 2023 at 12:39:36AM +0200, Konrad Dybcio wrote: > On 21.06.2023 06:36, Krishna Kurapati wrote: > > Add USB and DWC3 node for tertiary port of SC8280 along with multiport > > IRQ's and phy's. This will be used as a base for SA8295P and SA8295-Ride > > platforms. > > > > Signed-off-by: Krishna Kurapati <quic_kriskura@quicinc.com> > > --- > > arch/arm64/boot/dts/qcom/sc8280xp.dtsi | 77 ++++++++++++++++++++++++++ > > 1 file changed, 77 insertions(+) > > > > diff --git a/arch/arm64/boot/dts/qcom/sc8280xp.dtsi b/arch/arm64/boot/dts/qcom/sc8280xp.dtsi > > index 8fa9fbfe5d00..0dfa350ea3b3 100644 > > --- a/arch/arm64/boot/dts/qcom/sc8280xp.dtsi > > +++ b/arch/arm64/boot/dts/qcom/sc8280xp.dtsi > > @@ -3013,6 +3013,83 @@ system-cache-controller@9200000 { > > interrupts = <GIC_SPI 582 IRQ_TYPE_LEVEL_HIGH>; > > }; > > > > + usb_2: usb@a4f8800 { > > + compatible = "qcom,sc8280xp-dwc3-mp", "qcom,dwc3"; > > + reg = <0 0x0a4f8800 0 0x400>; > > > + #address-cells = <2>; > > + #size-cells = <2>; > > + ranges; > These three properties, please stick just before status No, please keep them were they are for consistency with the rest of the file. > > + > > + 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"; > Please make it one per line Also not needed for the same reason. > > > + > > + assigned-clocks = <&gcc GCC_USB30_MP_MOCK_UTMI_CLK>, > > + <&gcc GCC_USB30_MP_MASTER_CLK>; > > + assigned-clock-rates = <19200000>, <200000000>; > And here Same here. Johan
On Sat, Jun 24, 2023 at 12:43:23PM +0530, Krishna Kurapati PSSNV wrote: > > On 21.06.2023 06:36, Krishna Kurapati wrote: > >> Add USB and DWC3 node for tertiary port of SC8280 along with multiport > >> IRQ's and phy's. This will be used as a base for SA8295P and SA8295-Ride > >> platforms. > >> > >> Signed-off-by: Krishna Kurapati <quic_kriskura@quicinc.com> > > Not a comment to the patch, but very nice that Qcom ensured every > > endpoint is wakeup-capable, this used not to be the case before :D > Yes wakeup is supported by all ports now, but I didn't make those > changes now as I wanted to keep driver code diff minimal and don't need > wakeup support for the product currently. But for sure, will update > driver code to handle wakeup on all ports in near future. Why didn't you include it in v9? I thought you had a working implementation for this? Since wakeup will be another case where glue and core need to interact, it's good to have the wakeup implementation from the start to be able to evaluate your multiport implementation properly. Right now it looks like you only added wakeup interrupt lookup and request, but then you never actually enable them which is not very nice. Johan
On Wed, Jun 21, 2023 at 10:06:18AM +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). > > Changes in v9: > Added IRQ support for DP/DM/SS MP Irq's of SC8280 > Refactored code to read port count by accessing xhci registers > You obviously did many more changes in v9. Please amend this list for v9 and be more specific when submitting v10. Johan
On 6/27/2023 5:16 AM, Thinh Nguyen wrote: > On Mon, Jun 26, 2023, Thinh Nguyen wrote: >> On Sat, Jun 24, 2023, Krishna Kurapati PSSNV wrote: >>> >>> >>> On 6/24/2023 3:57 AM, Thinh Nguyen wrote: >>>> On Wed, Jun 21, 2023, Krishna Kurapati wrote: >>>>> On some SoC's like SA8295P where the tertiary controller is host-only >>>>> capable, GEVTADDRHI/LO, GEVTSIZ, GEVTCOUNT registers are not accessible. >>>>> Trying to access them leads to a crash. >>>>> >>>>> For DRD/Peripheral supported controllers, event buffer setup is done >>>>> again in gadget_pullup. Skip setup or cleanup of event buffers if >>>>> controller is host-only capable. >>>>> >>>>> Suggested-by: Johan Hovold <johan@kernel.org> >>>>> Signed-off-by: Krishna Kurapati <quic_kriskura@quicinc.com> >>>>> --- >>>>> drivers/usb/dwc3/core.c | 11 +++++++++++ >>>>> 1 file changed, 11 insertions(+) >>>>> >>>>> diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c >>>>> index 32ec05fc242b..e1ebae54454f 100644 >>>>> --- a/drivers/usb/dwc3/core.c >>>>> +++ b/drivers/usb/dwc3/core.c >>>>> @@ -486,6 +486,11 @@ static void dwc3_free_event_buffers(struct dwc3 *dwc) >>>>> static int dwc3_alloc_event_buffers(struct dwc3 *dwc, unsigned int length) >>>>> { >>>>> struct dwc3_event_buffer *evt; >>>>> + unsigned int hw_mode; >>>>> + >>>>> + hw_mode = DWC3_GHWPARAMS0_MODE(dwc->hwparams.hwparams0); >>>>> + if (hw_mode == DWC3_GHWPARAMS0_MODE_HOST) >>>>> + return 0; >>>> >>>> This is a little awkward. Returning 0 here indicates that this function >>>> was successful, and the event buffers were allocated based on the >>>> function name. Do this check outside of dwc3_alloc_one_event_buffer() >>>> and specifically set dwc->ev_buf = NULL if that's the case. >>>> >>> Hi Thinh, >>> >>> Apologies, I didn't understand the comment properly. >>> >>> I thought we were supposed to return 0 here if we fulfill the goal of this >>> function (allocate if we are drd/gadget and don't allocate if we are host >>> mode only). >> >> The name of the function implies that it returns 0 if it allocated the >> event buffer. If there are multiple conditions to the function returning >> 0 here, then we should document it. >> >>> >>> If we return a non zero error here, probe would fail as this call will be >>> done only for host only controllers during probe and nowhere else. >>> >>> Are you suggesting we move this check to dwc3_alloc_one_event_buffer call >>> ? >>> >>> Regards, >>> Krishna, >>> >> >> I'm suggesting to move the check to the caller of >> dwc3_alloc_one_event_buffer(). Something like this so that it's >> self-documented: >> >> diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c >> index 0beaab932e7d..bba82792bf6f 100644 >> --- a/drivers/usb/dwc3/core.c >> +++ b/drivers/usb/dwc3/core.c >> @@ -1773,6 +1773,7 @@ static int dwc3_probe(struct platform_device *pdev) >> struct resource *res, dwc_res; >> void __iomem *regs; >> struct dwc3 *dwc; >> + unsigned int hw_mode; >> int ret; >> >> dwc = devm_kzalloc(dev, sizeof(*dwc), GFP_KERNEL); >> @@ -1854,11 +1855,16 @@ static int dwc3_probe(struct platform_device *pdev) >> >> pm_runtime_forbid(dev); >> >> - ret = dwc3_alloc_event_buffers(dwc, DWC3_EVENT_BUFFERS_SIZE); >> - if (ret) { >> - dev_err(dwc->dev, "failed to allocate event buffers\n"); >> - ret = -ENOMEM; >> - goto err_allow_rpm; >> + hw_mode = DWC3_GHWPARAMS0_MODE(dwc->hwparams.hwparams0); >> + if (hw_mode == DWC3_GHWPARAMS0_MODE_HOST) { >> + dwc->ev_buf = NULL; >> + } else { >> + ret = dwc3_alloc_event_buffers(dwc, DWC3_EVENT_BUFFERS_SIZE); >> + if (ret) { >> + dev_err(dwc->dev, "failed to allocate event buffers\n"); >> + ret = -ENOMEM; >> + goto err_allow_rpm; >> + } >> } >> >> dwc->edev = dwc3_get_extcon(dwc); >> > > Actually, please ignore. there's already a document there, I missed that > for some reason. What you did is fine. Though, I don't see the condition > for ev_buf = NULL anywhere. Can you add that for clarity? > > Thanks, > Thinh Hi Thinh, Did you mean adding "dwc->ev_buf = NULL" specifically ? Regards, Krishna,
On 6/27/2023 5:15 PM, Johan Hovold wrote: > On Wed, Jun 21, 2023 at 10:06:21AM +0530, Krishna Kurapati wrote: >> Currently host-only capable DWC3 controllers support Multiport. >> Temporarily map XHCI address space for host-only controllers and parse >> XHCI Extended Capabilities registers to read number of usb2 ports and >> usb3 ports present on multiport controller. Each USB Port is at least HS >> capable. >> >> The port info for usb2 and usb3 phy are identified as num_usb2_ports >> and num_usb3_ports. The intention is as follows: >> >> Wherever we need to perform phy operations like: >> >> LOOP_OVER_NUMBER_OF_AVAILABLE_PORTS() >> { >> phy_set_mode(dwc->usb2_generic_phy[i], PHY_MODE_USB_HOST); >> phy_set_mode(dwc->usb3_generic_phy[i], PHY_MODE_USB_HOST); >> } >> >> If number of usb2 ports is 3, loop can go from index 0-2 for >> usb2_generic_phy. If number of usb3-ports is 2, we don't know for sure, >> if the first 2 ports are SS capable or some other ports like (2 and 3) >> are SS capable. So instead, num_usb2_ports is used to loop around all >> phy's (both hs and ss) for performing phy operations. If any >> usb3_generic_phy turns out to be NULL, phy operation just bails out. >> >> num_usb3_ports is used to modify GUSB3PIPECTL registers while setting up >> phy's as we need to know how many SS capable ports are there for this. >> >> Signed-off-by: Krishna Kurapati <quic_kriskura@quicinc.com> >> --- >> drivers/usb/dwc3/core.c | 62 +++++++++++++++++++++++++++++++++++++++++ >> drivers/usb/dwc3/core.h | 9 ++++++ >> 2 files changed, 71 insertions(+) >> >> diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c >> index f6689b731718..32ec05fc242b 100644 >> --- a/drivers/usb/dwc3/core.c >> +++ b/drivers/usb/dwc3/core.c >> @@ -39,6 +39,7 @@ >> #include "io.h" >> >> #include "debug.h" >> +#include "../host/xhci-ext-caps.h" >> >> #define DWC3_DEFAULT_AUTOSUSPEND_DELAY 5000 /* ms */ >> >> @@ -1767,6 +1768,52 @@ static int dwc3_get_clocks(struct dwc3 *dwc) >> return 0; >> } >> >> +static int dwc3_read_port_info(struct dwc3 *dwc) >> +{ >> + void __iomem *base; >> + u8 major_revision; >> + u32 offset = 0; >> + int ret = 0; > > ret is never modified, so drop and return 0 unconditionally below. > > You can add it back later in the series when you start using it. > >> + u32 val; >> + >> + /* >> + * Remap xHCI address space to access XHCI ext cap regs, >> + * since it is needed to get port info. >> + */ >> + base = ioremap(dwc->xhci_resources[0].start, >> + resource_size(&dwc->xhci_resources[0])); >> + if (IS_ERR(base)) >> + return PTR_ERR(base); >> + >> + do { >> + offset = xhci_find_next_ext_cap(base, offset, >> + XHCI_EXT_CAPS_PROTOCOL); >> + > > You can drop this newline. > >> + if (!offset) >> + break; >> + >> + val = readl(base + offset); >> + major_revision = XHCI_EXT_PORT_MAJOR(val); >> + >> + val = readl(base + offset + 0x08); >> + if (major_revision == 0x03) { >> + dwc->num_usb3_ports += XHCI_EXT_PORT_COUNT(val); >> + } else if (major_revision <= 0x02) { >> + dwc->num_usb2_ports += XHCI_EXT_PORT_COUNT(val); >> + } else { >> + dev_err(dwc->dev, >> + "Unrecognized port major revision %d\n", >> + major_revision); >> + } >> + } while (1); >> + >> + dev_dbg(dwc->dev, "hs-ports: %u ss-ports: %u\n", >> + dwc->num_usb2_ports, dwc->num_usb3_ports); >> + >> + iounmap(base); > > Nit: I'd add a newline here. > >> + return ret; >> +} >> + >> static int dwc3_probe(struct platform_device *pdev) >> { >> struct device *dev = &pdev->dev; >> @@ -1774,6 +1821,7 @@ static int dwc3_probe(struct platform_device *pdev) >> void __iomem *regs; >> struct dwc3 *dwc; >> int ret; >> + unsigned int hw_mode; >> >> dwc = devm_kzalloc(dev, sizeof(*dwc), GFP_KERNEL); >> if (!dwc) >> @@ -1854,6 +1902,20 @@ static int dwc3_probe(struct platform_device *pdev) >> goto err_disable_clks; >> } >> >> + /* >> + * Currently only DWC3 controllers that are host-only capable >> + * support Multiport. >> + */ >> + hw_mode = DWC3_GHWPARAMS0_MODE(dwc->hwparams.hwparams0); >> + if (hw_mode == DWC3_GHWPARAMS0_MODE_HOST) { >> + ret = dwc3_read_port_info(dwc); >> + if (ret) >> + goto err_disable_clks; >> + } else { >> + dwc->num_usb2_ports = 1; >> + dwc->num_usb3_ports = 1; >> + } >> + >> spin_lock_init(&dwc->lock); >> mutex_init(&dwc->mutex); >> >> diff --git a/drivers/usb/dwc3/core.h b/drivers/usb/dwc3/core.h >> index 8b1295e4dcdd..42fb17aa66fa 100644 >> --- a/drivers/usb/dwc3/core.h >> +++ b/drivers/usb/dwc3/core.h >> @@ -33,6 +33,10 @@ >> >> #include <linux/power_supply.h> >> >> +#define XHCI_EXT_PORT_MAJOR(x) (((x) >> 24) & 0xff) >> +#define XHCI_EXT_PORT_MINOR(x) (((x) >> 16) & 0xff) >> +#define XHCI_EXT_PORT_COUNT(x) (((x) >> 8) & 0xff) > > Again, don't copy defines from xhci. > > Looks like these should be moved to the xhci-ext-caps.h header along > with struct xhci_protocol_caps. > Can't we just give them an exception ? Modifying xhci stuff doesn't sound good. Can I just rename them and keep them here ? Regards, Krishna, >> + >> #define DWC3_MSG_MAX 500 >> >> /* Global constants */ >> @@ -1029,6 +1033,8 @@ struct dwc3_scratchpad_array { >> * @usb3_phy: pointer to USB3 PHY >> * @usb2_generic_phy: pointer to USB2 PHY >> * @usb3_generic_phy: pointer to USB3 PHY >> + * @num_usb2_ports: number of USB2 ports. >> + * @num_usb3_ports: number of USB3 ports. > > Again, please drop the full stops ('.'). > > Johan
On 6/27/2023 5:39 PM, Johan Hovold wrote: > On Wed, Jun 21, 2023 at 10:06:23AM +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> >> [Krishna: Modifed logic for generic phy and rebased the patch] >> Signed-off-by: Krishna Kurapati <quic_kriskura@quicinc.com> > > As I already said: > > If Harsh is the primary author you need to add a From: line at > the beginning of the patch. > > Either way, you need his SoB as well as your Co-developed-by tag. > > All this is documented under Documentation/process/ somewhere. > > The above is missing a From line and two Co-developed-by tags at least. > Hi Johan, I tried to follow the following commit: 8030cb9a5568 ("soc: qcom: aoss: remove spurious IRQF_ONESHOT flags") Let me know if that is not acceptable. >> --- >> drivers/usb/dwc3/core.c | 252 ++++++++++++++++++++++++++++------------ >> drivers/usb/dwc3/core.h | 11 +- >> drivers/usb/dwc3/drd.c | 15 ++- >> 3 files changed, 192 insertions(+), 86 deletions(-) >> >> diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c >> index e1ebae54454f..2ac72525de7d 100644 >> --- a/drivers/usb/dwc3/core.c >> +++ b/drivers/usb/dwc3/core.c >> @@ -120,10 +120,11 @@ void dwc3_set_prtcap(struct dwc3 *dwc, u32 mode) >> static void __dwc3_set_mode(struct work_struct *work) >> { >> struct dwc3 *dwc = work_to_dwc(work); >> + u32 desired_dr_role; >> unsigned long flags; >> int ret; >> u32 reg; >> - u32 desired_dr_role; > > This is an unrelated change. Just add int i at the end. > I was trying to keep the reverse xmas order of variables. >> + int i; >> >> mutex_lock(&dwc->mutex); >> spin_lock_irqsave(&dwc->lock, flags); > >> @@ -746,23 +779,34 @@ static int dwc3_phy_setup(struct dwc3 *dwc) >> static int dwc3_phy_init(struct dwc3 *dwc) >> { >> int ret; >> + int i; >> + int j; >> >> usb_phy_init(dwc->usb2_phy); >> usb_phy_init(dwc->usb3_phy); >> >> - ret = phy_init(dwc->usb2_generic_phy); >> - if (ret < 0) >> - goto err_shutdown_usb3_phy; >> + for (i = 0; i < dwc->num_usb2_ports; i++) { >> + ret = phy_init(dwc->usb2_generic_phy[i]); >> + if (ret < 0) >> + goto err_exit_usb2_phy; >> + } >> >> - ret = phy_init(dwc->usb3_generic_phy); >> - if (ret < 0) >> - goto err_exit_usb2_phy; >> + for (i = 0; i < dwc->num_usb2_ports; i++) { >> + ret = phy_init(dwc->usb3_generic_phy[i]); >> + if (ret < 0) >> + goto err_exit_usb3_phy; >> + } >> >> return 0; >> >> +err_exit_usb3_phy: >> + for (j = i-1; j >= 0; j--) > > Missing spaces around - here and below. > >> + phy_exit(dwc->usb3_generic_phy[j]); >> + i = dwc->num_usb2_ports; >> err_exit_usb2_phy: >> - phy_exit(dwc->usb2_generic_phy); >> -err_shutdown_usb3_phy: >> + for (j = i-1; j >= 0; j--) >> + phy_exit(dwc->usb2_generic_phy[j]); >> + > > Again: > > The above is probably better implemented as a *single* loop over > num_usb2_ports where you enable each USB2 and USB3 PHY. On > errors you use the loop index to disable the already enabled > PHYs in reverse order below (after disabling the USB2 PHY if > USB3 phy init fails). > > with emphasis on "single" added. > Oh, you mean something like this ? for (loop over num_ports) { ret = phy_init(dwc->usb3_generic_phy[i]); if (ret != 0) goto err_exit_phy; ret = phy_init(dwc->usb2_generic_phy[i]); if (ret != 0) goto err_exit_phy; } err_exit_phy: for (j = i-1; j >= 0; j--) { phy_exit(dwc->usb3_generic_phy[j]); phy_exit(dwc->usb2_generic_phy[j]); } >> usb_phy_shutdown(dwc->usb3_phy); >> usb_phy_shutdown(dwc->usb2_phy); > >> @@ -781,23 +829,34 @@ static void dwc3_phy_exit(struct dwc3 *dwc) >> static int dwc3_phy_power_on(struct dwc3 *dwc) >> { >> int ret; >> + int i; >> + int j; >> >> usb_phy_set_suspend(dwc->usb2_phy, 0); >> usb_phy_set_suspend(dwc->usb3_phy, 0); >> >> - ret = phy_power_on(dwc->usb2_generic_phy); >> - if (ret < 0) >> - goto err_suspend_usb3_phy; >> + for (i = 0; i < dwc->num_usb2_ports; i++) { >> + ret = phy_power_on(dwc->usb2_generic_phy[i]); >> + if (ret < 0) >> + goto err_power_off_usb2_phy; >> + } >> >> - ret = phy_power_on(dwc->usb3_generic_phy); >> - if (ret < 0) >> - goto err_power_off_usb2_phy; >> + for (i = 0; i < dwc->num_usb2_ports; i++) { >> + ret = phy_power_on(dwc->usb3_generic_phy[i]); >> + if (ret < 0) >> + goto err_power_off_usb3_phy; >> + } > > Again: These loops should be merged too as for phy_init. > >> return 0; >> >> +err_power_off_usb3_phy: >> + for (j = i-1; j >= 0; j--) >> + phy_power_off(dwc->usb3_generic_phy[i]); >> + i = dwc->num_usb2_ports; >> err_power_off_usb2_phy: >> - phy_power_off(dwc->usb2_generic_phy); >> -err_suspend_usb3_phy: >> + for (j = i-1; j >= 0; j--) >> + phy_power_off(dwc->usb2_generic_phy[i]); >> + >> usb_phy_set_suspend(dwc->usb3_phy, 1); >> usb_phy_set_suspend(dwc->usb2_phy, 1); >> > >> @@ -1290,7 +1358,9 @@ static int dwc3_core_get_phy(struct dwc3 *dwc) >> { >> struct device *dev = dwc->dev; >> struct device_node *node = dev->of_node; >> + char phy_name[11]; >> int ret; >> + int i; >> >> if (node) { >> dwc->usb2_phy = devm_usb_get_phy_by_phandle(dev, "usb-phy", 0); >> @@ -1316,22 +1386,36 @@ 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 (ret == -ENOSYS || ret == -ENODEV) >> - dwc->usb2_generic_phy = NULL; >> + for (i = 0; i < dwc->num_usb2_ports; i++) { >> + if (dwc->num_usb2_ports == 1) >> + sprintf(phy_name, "usb2-phy"); >> else >> - return dev_err_probe(dev, ret, "no usb2 phy configured\n"); >> - } >> + sprintf(phy_name, "usb2-port%d", i); >> + >> + dwc->usb2_generic_phy[i] = devm_phy_get(dev, phy_name); >> + if (IS_ERR(dwc->usb2_generic_phy[i])) { >> + ret = PTR_ERR(dwc->usb2_generic_phy[i]); >> + if (ret == -ENOSYS || ret == -ENODEV) >> + dwc->usb2_generic_phy[i] = NULL; >> + else >> + return dev_err_probe(dev, ret, >> + "no %s phy configured\n", phy_name); > > I still believe > > "failed to lookup phy %s" > > is better. > >> + } >> >> - dwc->usb3_generic_phy = devm_phy_get(dev, "usb3-phy"); >> - if (IS_ERR(dwc->usb3_generic_phy)) { >> - ret = PTR_ERR(dwc->usb3_generic_phy); >> - if (ret == -ENOSYS || ret == -ENODEV) >> - dwc->usb3_generic_phy = NULL; >> + if (dwc->num_usb2_ports == 1) >> + sprintf(phy_name, "usb3-phy"); >> else >> - return dev_err_probe(dev, ret, "no usb3 phy configured\n"); >> + sprintf(phy_name, "usb3-port%d", i); >> + >> + dwc->usb3_generic_phy[i] = devm_phy_get(dev, phy_name); >> + if (IS_ERR(dwc->usb3_generic_phy[i])) { >> + ret = PTR_ERR(dwc->usb3_generic_phy[i]); >> + if (ret == -ENOSYS || ret == -ENODEV) >> + dwc->usb3_generic_phy[i] = NULL; >> + else >> + return dev_err_probe(dev, ret, >> + "no %s phy configured\n", phy_name); > > Same here. > >> + } >> } >> >> return 0; > >> @@ -1821,6 +1908,9 @@ static int dwc3_read_port_info(struct dwc3 *dwc) >> dev_dbg(dwc->dev, "hs-ports: %u ss-ports: %u\n", >> dwc->num_usb2_ports, dwc->num_usb3_ports); >> >> + if (dwc->num_usb2_ports > DWC3_MAX_PORTS) >> + ret = -ENOMEM; > > You also need to add a check for num_usb3_ports as I already mentioned. > >> + >> iounmap(base); >> return ret; >> } > >> diff --git a/drivers/usb/dwc3/core.h b/drivers/usb/dwc3/core.h >> index 42fb17aa66fa..b2bab23ca22b 100644 >> --- a/drivers/usb/dwc3/core.h >> +++ b/drivers/usb/dwc3/core.h >> @@ -37,6 +37,9 @@ >> #define XHCI_EXT_PORT_MINOR(x) (((x) >> 16) & 0xff) >> #define XHCI_EXT_PORT_COUNT(x) (((x) >> 8) & 0xff) >> >> +/* Number of ports supported by a multiport controller */ >> +#define DWC3_MAX_PORTS 4 > > You did not answer my question about whether this was an arbitrary > implementation limit (i.e. just reflecting the only currently supported > multiport controller)? > I mentioned in commit text that it is limited to 4. Are you referring to state the reason why I chose the value 4 ? >> + >> #define DWC3_MSG_MAX 500 >> >> /* Global constants */ >> @@ -1031,8 +1034,8 @@ 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 >> + * @usb2_generic_phy: pointer to array of USB2 PHY >> + * @usb3_generic_phy: pointer to array of USB3 PHY >> * @num_usb2_ports: number of USB2 ports. >> * @num_usb3_ports: number of USB3 ports. >> * @phys_ready: flag to indicate that PHYs are ready >> @@ -1171,8 +1174,8 @@ struct dwc3 { >> struct usb_phy *usb2_phy; >> struct usb_phy *usb3_phy; >> >> - struct phy *usb2_generic_phy; >> - struct phy *usb3_generic_phy; >> + struct phy *usb2_generic_phy[DWC3_MAX_PORTS]; >> + struct phy *usb3_generic_phy[DWC3_MAX_PORTS]; >> >> u8 num_usb2_ports; >> u8 num_usb3_ports; >> diff --git a/drivers/usb/dwc3/drd.c b/drivers/usb/dwc3/drd.c >> index 039bf241769a..18a247ff75ac 100644 >> --- a/drivers/usb/dwc3/drd.c >> +++ b/drivers/usb/dwc3/drd.c >> @@ -327,10 +327,11 @@ static void dwc3_otg_device_exit(struct dwc3 *dwc) >> >> void dwc3_otg_update(struct dwc3 *dwc, bool ignore_idstatus) >> { >> + unsigned long flags; >> int ret; >> u32 reg; >> int id; >> - unsigned long flags; >> + int i; >> >> if (dwc->dr_mode != USB_DR_MODE_OTG) >> return; >> @@ -386,9 +387,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_usb2_ports; i++) { >> + if (dwc->usb2_generic_phy[i]) >> + phy_set_mode(dwc->usb2_generic_phy[i], >> + PHY_MODE_USB_HOST); > > While not strictly necessary, adding bracket around multiline statements > is usually preferred as it improves readability. > >> + } >> } >> break; >> case DWC3_OTG_ROLE_DEVICE: >> @@ -400,8 +403,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); > > Same here, but this is probably better just left at 85 columns after > removing the line break. > >> ret = dwc3_gadget_init(dwc); >> if (ret) > > Johan Thanks, Krishna,
On 6/27/2023 8:01 PM, Johan Hovold wrote: > On Wed, Jun 21, 2023 at 10:06:24AM +0530, Krishna Kurapati wrote: >> Add support to read Multiport IRQ's related to quad port controller >> of SA8295 Device. > > Please use a more descriptive summary and commit message; "read" is to > vague. You're looking up interrupts from the devicetree. Also this > should not just be about SA8295. > >> Signed-off-by: Krishna Kurapati <quic_kriskura@quicinc.com> >> --- >> drivers/usb/dwc3/dwc3-qcom.c | 108 +++++++++++++++++++++++++++++------ >> 1 file changed, 91 insertions(+), 17 deletions(-) >> >> diff --git a/drivers/usb/dwc3/dwc3-qcom.c b/drivers/usb/dwc3/dwc3-qcom.c >> index 3de43df6bbe8..3ab48a6925fe 100644 >> --- a/drivers/usb/dwc3/dwc3-qcom.c >> +++ b/drivers/usb/dwc3/dwc3-qcom.c >> @@ -74,9 +74,9 @@ struct dwc3_qcom { >> struct reset_control *resets; >> >> int hs_phy_irq; >> - int dp_hs_phy_irq; >> - int dm_hs_phy_irq; >> - int ss_phy_irq; >> + int dp_hs_phy_irq[4]; >> + int dm_hs_phy_irq[4]; >> + int ss_phy_irq[2]; > > As has already been pointed out, you should use a define for these. And > you already have DWC3_MAX_PORTS. > > The driver should not be hardcoding the fact that there are only two SS > ports on this particular SoC that you're interested in. > >> enum usb_device_speed usb2_speed; >> >> struct extcon_dev *edev; > >> @@ -535,6 +535,80 @@ static int dwc3_qcom_get_irq(struct platform_device *pdev, >> return ret; >> } >> >> +static int dwc3_qcom_setup_mp_irq(struct platform_device *pdev) >> +{ >> + struct dwc3_qcom *qcom = platform_get_drvdata(pdev); >> + char irq_name[15]; >> + int irq; >> + int ret; >> + int i; >> + >> + for (i = 0; i < 4; i++) { > > DWC3_MAX_PORTS here too and similar below. > >> + if (qcom->dp_hs_phy_irq[i]) >> + continue; > > This is not very nice. You should try to integrate the current lookup > code as I told you to do with the PHY lookups. That is, use a single > loop for all HS/SS IRQs, and pick the legacy name if the number of ports > is 1. > > Of course, you added the xhci capability parsing to the core driver so > that information is not yet available, but you need it in the glue > driver also... > > As I mentioned earlier, you can infer the number of ports from the > interrupt names. Alternatively, you can infer it from the compatible > string. In any case, you should not need to ways to determine the same > information in the glue driver, then in the core part, and then yet > again in the xhci driver... > Hi Johan, The reason why I didn't integrate this with the original function was the ACPI stuff. The MP devices have no ACPI variant. And I think for clarity sake its best to keep these two functions separated. Regards, Krishna, >> + >> + sprintf(irq_name, "dp%d_hs_phy_irq", i+1); > > Spaces around binary operators. Does not checkpatch warn about that? > >> + irq = dwc3_qcom_get_irq(pdev, irq_name, -1); >> + if (irq > 0) { >> + irq_set_status_flags(irq, IRQ_NOAUTOEN); >> + ret = devm_request_threaded_irq(qcom->dev, irq, NULL, >> + qcom_dwc3_resume_irq, >> + IRQF_TRIGGER_HIGH | IRQF_ONESHOT, >> + irq_name, qcom); >> + if (ret) { >> + dev_err(qcom->dev, "%s failed: %d\n", irq_name, ret); >> + return ret; >> + } >> + } >> + >> + qcom->dp_hs_phy_irq[i] = irq; >> + } >> + >> + for (i = 0; i < 4; i++) { >> + if (qcom->dm_hs_phy_irq[i]) >> + continue; >> + >> + sprintf(irq_name, "dm%d_hs_phy_irq", i+1); >> + irq = dwc3_qcom_get_irq(pdev, irq_name, -1); >> + if (irq > 0) { >> + irq_set_status_flags(irq, IRQ_NOAUTOEN); >> + ret = devm_request_threaded_irq(qcom->dev, irq, NULL, >> + qcom_dwc3_resume_irq, >> + IRQF_TRIGGER_HIGH | IRQF_ONESHOT, >> + irq_name, qcom); >> + if (ret) { >> + dev_err(qcom->dev, "%s failed: %d\n", irq_name, ret); >> + return ret; >> + } >> + } >> + >> + qcom->dm_hs_phy_irq[i] = irq; >> + } >> + >> + for (i = 0; i < 2; i++) { >> + if (qcom->ss_phy_irq[i]) >> + continue; >> + >> + sprintf(irq_name, "ss%d_phy_irq", i+1); >> + irq = dwc3_qcom_get_irq(pdev, irq_name, -1); >> + if (irq > 0) { >> + irq_set_status_flags(irq, IRQ_NOAUTOEN); >> + ret = devm_request_threaded_irq(qcom->dev, irq, NULL, >> + qcom_dwc3_resume_irq, >> + IRQF_TRIGGER_HIGH | IRQF_ONESHOT, >> + irq_name, qcom); >> + if (ret) { >> + dev_err(qcom->dev, "%s failed: %d\n", irq_name, ret); >> + return ret; >> + } >> + } >> + >> + qcom->ss_phy_irq[i] = irq; >> + } > > So the above should all be merged in either a single helper looking up > all the interrupts for a port and resused for the non-MP case. > I agree, Will merge all under some common helper function. Thanks, Krishna, >> + >> + return 0; >> +} >> + >> static int dwc3_qcom_setup_irq(struct platform_device *pdev) >> { >> struct dwc3_qcom *qcom = platform_get_drvdata(pdev); >> @@ -570,7 +644,7 @@ static int dwc3_qcom_setup_irq(struct platform_device *pdev) >> dev_err(qcom->dev, "dp_hs_phy_irq failed: %d\n", ret); >> return ret; >> } >> - qcom->dp_hs_phy_irq = irq; >> + qcom->dp_hs_phy_irq[0] = irq; >> } >> >> irq = dwc3_qcom_get_irq(pdev, "dm_hs_phy_irq", >> @@ -585,7 +659,7 @@ static int dwc3_qcom_setup_irq(struct platform_device *pdev) >> dev_err(qcom->dev, "dm_hs_phy_irq failed: %d\n", ret); >> return ret; >> } >> - qcom->dm_hs_phy_irq = irq; >> + qcom->dm_hs_phy_irq[0] = irq; >> } >> >> irq = dwc3_qcom_get_irq(pdev, "ss_phy_irq", >> @@ -600,10 +674,10 @@ static int dwc3_qcom_setup_irq(struct platform_device *pdev) >> dev_err(qcom->dev, "ss_phy_irq failed: %d\n", ret); >> return ret; >> } >> - qcom->ss_phy_irq = irq; >> + qcom->ss_phy_irq[0] = irq; >> } >> >> - return 0; >> + return dwc3_qcom_setup_mp_irq(pdev);; > > Stray ; > >> } >> >> static int dwc3_qcom_clk_init(struct dwc3_qcom *qcom, int count) > > Johan
On 6/27/2023 8:35 PM, Johan Hovold wrote: > On Wed, Jun 21, 2023 at 10:06:25AM +0530, Krishna Kurapati wrote: >> QCOM SoC SA8295P's tertiary quad port controller supports 2 HS+SS >> ports and 2 HS only ports. Add support for configuring PWR_EVENT_IRQ's >> for all the ports during suspend/resume. > > Please be more specific here. You don't seem to be configuring anything. > >> Signed-off-by: Krishna Kurapati <quic_kriskura@quicinc.com> >> --- >> drivers/usb/dwc3/dwc3-qcom.c | 48 +++++++++++++++++++++++++++++++----- >> 1 file changed, 42 insertions(+), 6 deletions(-) >> >> diff --git a/drivers/usb/dwc3/dwc3-qcom.c b/drivers/usb/dwc3/dwc3-qcom.c >> index 3ab48a6925fe..699485a85233 100644 >> --- a/drivers/usb/dwc3/dwc3-qcom.c >> +++ b/drivers/usb/dwc3/dwc3-qcom.c >> @@ -37,7 +37,11 @@ >> #define PIPE3_PHYSTATUS_SW BIT(3) >> #define PIPE_UTMI_CLK_DIS BIT(8) >> >> -#define PWR_EVNT_IRQ_STAT_REG 0x58 >> +#define PWR_EVNT_IRQ1_STAT_REG 0x58 >> +#define PWR_EVNT_IRQ2_STAT_REG 0x1dc >> +#define PWR_EVNT_IRQ3_STAT_REG 0x228 >> +#define PWR_EVNT_IRQ4_STAT_REG 0x238 >> + >> #define PWR_EVNT_LPM_IN_L2_MASK BIT(4) >> #define PWR_EVNT_LPM_OUT_L2_MASK BIT(5) >> >> @@ -93,6 +97,13 @@ struct dwc3_qcom { >> struct icc_path *icc_path_apps; >> }; >> >> +static u32 pwr_evnt_irq_stat_reg_offset[4] = { >> + PWR_EVNT_IRQ1_STAT_REG, >> + PWR_EVNT_IRQ2_STAT_REG, >> + PWR_EVNT_IRQ3_STAT_REG, >> + PWR_EVNT_IRQ4_STAT_REG, >> +}; >> + >> static inline void dwc3_qcom_setbits(void __iomem *base, u32 offset, u32 val) >> { >> u32 reg; >> @@ -417,17 +428,37 @@ static void dwc3_qcom_enable_interrupts(struct dwc3_qcom *qcom) >> dwc3_qcom_enable_wakeup_irq(qcom->ss_phy_irq[0], 0); >> } >> >> +static u8 dwc3_qcom_get_port_info(struct dwc3_qcom *qcom) > > "port_info" is not very specific, call it get_num_ports() or similar. > >> +{ >> + struct dwc3 __maybe_unused *dwc = platform_get_drvdata(qcom->dwc3); > > __maybe unused makes no sense here. > >> + >> + if (dwc3_qcom_is_host(qcom)) >> + return dwc->num_usb2_ports; > > Here you're accessing the core driver data again, which we want to > avoid going forward so this at least warrants a FIXME to rework this as > well. > Ok, will add a FIXME here. Thanks, Krishna, >> + >> + /* >> + * If not in host mode, either dwc was not probed >> + * or we are in device mode, either case checking for >> + * only first pwr event irq would suffice. > > Rewrap comment at 80 columns. > >> + */ >> + >> + return 1; >> +} >> + >> static int dwc3_qcom_suspend(struct dwc3_qcom *qcom, bool wakeup) >> { >> u32 val; >> int i, ret; >> + u8 num_ports; > > Move first. > >> if (qcom->is_suspended) >> return 0; >> >> - val = readl(qcom->qscratch_base + PWR_EVNT_IRQ_STAT_REG); >> - if (!(val & PWR_EVNT_LPM_IN_L2_MASK)) >> + num_ports = dwc3_qcom_get_port_info(qcom); >> + for (i = 0; i < num_ports; i++) { >> + val = readl(qcom->qscratch_base + pwr_evnt_irq_stat_reg_offset[i]); >> + if (!(val & PWR_EVNT_LPM_IN_L2_MASK)) >> dev_err(qcom->dev, "HS-PHY not in L2\n"); > > This line is not indented properly. > > Make sure to run checkpatch before submitting so that I don't have to > point out things like this again. > >> + } >> >> for (i = qcom->num_clocks - 1; i >= 0; i--) >> clk_disable_unprepare(qcom->clks[i]); >> @@ -452,12 +483,14 @@ static int dwc3_qcom_suspend(struct dwc3_qcom *qcom, bool wakeup) >> >> static int dwc3_qcom_resume(struct dwc3_qcom *qcom, bool wakeup) >> { >> + int num_ports; >> int ret; >> int i; >> >> if (!qcom->is_suspended) >> return 0; >> >> + num_ports = dwc3_qcom_get_port_info(qcom); > > Move below to where you use num_ports. > >> if (dwc3_qcom_is_host(qcom) && wakeup) >> dwc3_qcom_disable_interrupts(qcom); >> >> @@ -474,9 +507,12 @@ static int dwc3_qcom_resume(struct dwc3_qcom *qcom, bool wakeup) >> if (ret) >> dev_warn(qcom->dev, "failed to enable interconnect: %d\n", ret); >> >> - /* Clear existing events from PHY related to L2 in/out */ > > No need to move the comment. > >> - dwc3_qcom_setbits(qcom->qscratch_base, PWR_EVNT_IRQ_STAT_REG, >> - PWR_EVNT_LPM_IN_L2_MASK | PWR_EVNT_LPM_OUT_L2_MASK); >> + for (i = 0; i < num_ports; i++) { >> + /* Clear existing events from PHY related to L2 in/out */ >> + dwc3_qcom_setbits(qcom->qscratch_base, >> + pwr_evnt_irq_stat_reg_offset[i], >> + PWR_EVNT_LPM_IN_L2_MASK | PWR_EVNT_LPM_OUT_L2_MASK); > > Indent continuation lines at least two tabs further than the previous > line. > >> + } >> >> qcom->is_suspended = false; > > Johan
On 6/27/2023 8:46 PM, Johan Hovold wrote: > On Sat, Jun 24, 2023 at 12:43:23PM +0530, Krishna Kurapati PSSNV wrote: >>> On 21.06.2023 06:36, Krishna Kurapati wrote: >>>> Add USB and DWC3 node for tertiary port of SC8280 along with multiport >>>> IRQ's and phy's. This will be used as a base for SA8295P and SA8295-Ride >>>> platforms. >>>> >>>> Signed-off-by: Krishna Kurapati <quic_kriskura@quicinc.com> > >>> Not a comment to the patch, but very nice that Qcom ensured every >>> endpoint is wakeup-capable, this used not to be the case before :D > >> Yes wakeup is supported by all ports now, but I didn't make those >> changes now as I wanted to keep driver code diff minimal and don't need >> wakeup support for the product currently. But for sure, will update >> driver code to handle wakeup on all ports in near future. > > Why didn't you include it in v9? I thought you had a working > implementation for this? > > Since wakeup will be another case where glue and core need to interact, > it's good to have the wakeup implementation from the start to be able to > evaluate your multiport implementation properly. > > Right now it looks like you only added wakeup interrupt lookup and > request, but then you never actually enable them which is not very nice. > > Johan Hi Johan, As mentioned in one of my comments on earlier patches, wakeup is not a requirement I currently need to work on for the product. I added multiport IRQ support only because my pathces need to modify IRQ names. If there is a customer requirement I get in the future, I will definitely implement the wakeup part. But for now, I would like to stick to what is necessary for getting Multiport to work. Regards, Krishna,
On Mon, Jul 03, 2023, Krishna Kurapati PSSNV wrote: > > > On 6/27/2023 5:16 AM, Thinh Nguyen wrote: > > On Mon, Jun 26, 2023, Thinh Nguyen wrote: > > > On Sat, Jun 24, 2023, Krishna Kurapati PSSNV wrote: > > > > > > > > > > > > On 6/24/2023 3:57 AM, Thinh Nguyen wrote: > > > > > On Wed, Jun 21, 2023, Krishna Kurapati wrote: > > > > > > On some SoC's like SA8295P where the tertiary controller is host-only > > > > > > capable, GEVTADDRHI/LO, GEVTSIZ, GEVTCOUNT registers are not accessible. > > > > > > Trying to access them leads to a crash. > > > > > > > > > > > > For DRD/Peripheral supported controllers, event buffer setup is done > > > > > > again in gadget_pullup. Skip setup or cleanup of event buffers if > > > > > > controller is host-only capable. > > > > > > > > > > > > Suggested-by: Johan Hovold <johan@kernel.org> > > > > > > Signed-off-by: Krishna Kurapati <quic_kriskura@quicinc.com> > > > > > > --- > > > > > > drivers/usb/dwc3/core.c | 11 +++++++++++ > > > > > > 1 file changed, 11 insertions(+) > > > > > > > > > > > > diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c > > > > > > index 32ec05fc242b..e1ebae54454f 100644 > > > > > > --- a/drivers/usb/dwc3/core.c > > > > > > +++ b/drivers/usb/dwc3/core.c > > > > > > @@ -486,6 +486,11 @@ static void dwc3_free_event_buffers(struct dwc3 *dwc) > > > > > > static int dwc3_alloc_event_buffers(struct dwc3 *dwc, unsigned int length) > > > > > > { > > > > > > struct dwc3_event_buffer *evt; > > > > > > + unsigned int hw_mode; > > > > > > + > > > > > > + hw_mode = DWC3_GHWPARAMS0_MODE(dwc->hwparams.hwparams0); > > > > > > + if (hw_mode == DWC3_GHWPARAMS0_MODE_HOST) > > > > > > + return 0; > > > > > > > > > > This is a little awkward. Returning 0 here indicates that this function > > > > > was successful, and the event buffers were allocated based on the > > > > > function name. Do this check outside of dwc3_alloc_one_event_buffer() > > > > > and specifically set dwc->ev_buf = NULL if that's the case. > > > > > > > > > Hi Thinh, > > > > > > > > Apologies, I didn't understand the comment properly. > > > > > > > > I thought we were supposed to return 0 here if we fulfill the goal of this > > > > function (allocate if we are drd/gadget and don't allocate if we are host > > > > mode only). > > > > > > The name of the function implies that it returns 0 if it allocated the > > > event buffer. If there are multiple conditions to the function returning > > > 0 here, then we should document it. > > > > > > > > > > > If we return a non zero error here, probe would fail as this call will be > > > > done only for host only controllers during probe and nowhere else. > > > > > > > > Are you suggesting we move this check to dwc3_alloc_one_event_buffer call > > > > ? > > > > > > > > Regards, > > > > Krishna, > > > > > > > > > > I'm suggesting to move the check to the caller of > > > dwc3_alloc_one_event_buffer(). Something like this so that it's > > > self-documented: > > > > > > diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c > > > index 0beaab932e7d..bba82792bf6f 100644 > > > --- a/drivers/usb/dwc3/core.c > > > +++ b/drivers/usb/dwc3/core.c > > > @@ -1773,6 +1773,7 @@ static int dwc3_probe(struct platform_device *pdev) > > > struct resource *res, dwc_res; > > > void __iomem *regs; > > > struct dwc3 *dwc; > > > + unsigned int hw_mode; > > > int ret; > > > dwc = devm_kzalloc(dev, sizeof(*dwc), GFP_KERNEL); > > > @@ -1854,11 +1855,16 @@ static int dwc3_probe(struct platform_device *pdev) > > > pm_runtime_forbid(dev); > > > - ret = dwc3_alloc_event_buffers(dwc, DWC3_EVENT_BUFFERS_SIZE); > > > - if (ret) { > > > - dev_err(dwc->dev, "failed to allocate event buffers\n"); > > > - ret = -ENOMEM; > > > - goto err_allow_rpm; > > > + hw_mode = DWC3_GHWPARAMS0_MODE(dwc->hwparams.hwparams0); > > > + if (hw_mode == DWC3_GHWPARAMS0_MODE_HOST) { > > > + dwc->ev_buf = NULL; > > > + } else { > > > + ret = dwc3_alloc_event_buffers(dwc, DWC3_EVENT_BUFFERS_SIZE); > > > + if (ret) { > > > + dev_err(dwc->dev, "failed to allocate event buffers\n"); > > > + ret = -ENOMEM; > > > + goto err_allow_rpm; > > > + } > > > } > > > dwc->edev = dwc3_get_extcon(dwc); > > > > > > > Actually, please ignore. there's already a document there, I missed that > > for some reason. What you did is fine. Though, I don't see the condition > > for ev_buf = NULL anywhere. Can you add that for clarity? > > > > Thanks, > > Thinh > > Hi Thinh, > > Did you mean adding "dwc->ev_buf = NULL" specifically ? > Yes, please initialize dwc->ev_buf to NULL somewhere if it's host mode. Thanks, Thinh
On 7/3/2023 12:29 AM, Krishna Kurapati PSSNV wrote: > > > On 6/27/2023 8:01 PM, Johan Hovold wrote: >> On Wed, Jun 21, 2023 at 10:06:24AM +0530, Krishna Kurapati wrote: >>> Add support to read Multiport IRQ's related to quad port controller >>> of SA8295 Device. >> >> Please use a more descriptive summary and commit message; "read" is to >> vague. You're looking up interrupts from the devicetree. Also this >> should not just be about SA8295. >> >>> Signed-off-by: Krishna Kurapati <quic_kriskura@quicinc.com> >>> --- >>> drivers/usb/dwc3/dwc3-qcom.c | 108 +++++++++++++++++++++++++++++------ >>> 1 file changed, 91 insertions(+), 17 deletions(-) >>> >>> diff --git a/drivers/usb/dwc3/dwc3-qcom.c b/drivers/usb/dwc3/dwc3-qcom.c >>> index 3de43df6bbe8..3ab48a6925fe 100644 >>> --- a/drivers/usb/dwc3/dwc3-qcom.c >>> +++ b/drivers/usb/dwc3/dwc3-qcom.c >>> @@ -74,9 +74,9 @@ struct dwc3_qcom { >>> struct reset_control *resets; >>> int hs_phy_irq; >>> - int dp_hs_phy_irq; >>> - int dm_hs_phy_irq; >>> - int ss_phy_irq; >>> + int dp_hs_phy_irq[4]; >>> + int dm_hs_phy_irq[4]; >>> + int ss_phy_irq[2]; >> >> As has already been pointed out, you should use a define for these. And >> you already have DWC3_MAX_PORTS. >> >> The driver should not be hardcoding the fact that there are only two SS >> ports on this particular SoC that you're interested in. >> >>> enum usb_device_speed usb2_speed; >>> struct extcon_dev *edev; >> >>> @@ -535,6 +535,80 @@ static int dwc3_qcom_get_irq(struct >>> platform_device *pdev, >>> return ret; >>> } >>> +static int dwc3_qcom_setup_mp_irq(struct platform_device *pdev) >>> +{ >>> + struct dwc3_qcom *qcom = platform_get_drvdata(pdev); >>> + char irq_name[15]; >>> + int irq; >>> + int ret; >>> + int i; >>> + >>> + for (i = 0; i < 4; i++) { >> >> DWC3_MAX_PORTS here too and similar below. >> >>> + if (qcom->dp_hs_phy_irq[i]) >>> + continue; >> >> This is not very nice. You should try to integrate the current lookup >> code as I told you to do with the PHY lookups. That is, use a single >> loop for all HS/SS IRQs, and pick the legacy name if the number of ports >> is 1. >> >> Of course, you added the xhci capability parsing to the core driver so >> that information is not yet available, but you need it in the glue >> driver also... >> >> As I mentioned earlier, you can infer the number of ports from the >> interrupt names. Alternatively, you can infer it from the compatible >> string. In any case, you should not need to ways to determine the same >> information in the glue driver, then in the core part, and then yet >> again in the xhci driver... >> > Hi Johan, > > The reason why I didn't integrate this with the original function was > the ACPI stuff. The MP devices have no ACPI variant. And I think for > clarity sake its best to keep these two functions separated. > > Regards, > Krishna, > >>> + >>> + sprintf(irq_name, "dp%d_hs_phy_irq", i+1); >> >> Spaces around binary operators. Does not checkpatch warn about that? >> >>> + irq = dwc3_qcom_get_irq(pdev, irq_name, -1); >>> + if (irq > 0) { >>> + irq_set_status_flags(irq, IRQ_NOAUTOEN); >>> + ret = devm_request_threaded_irq(qcom->dev, irq, NULL, >>> + qcom_dwc3_resume_irq, >>> + IRQF_TRIGGER_HIGH | IRQF_ONESHOT, >>> + irq_name, qcom); >>> + if (ret) { >>> + dev_err(qcom->dev, "%s failed: %d\n", irq_name, ret); >>> + return ret; >>> + } >>> + } >>> + >>> + qcom->dp_hs_phy_irq[i] = irq; >>> + } >>> + >>> + for (i = 0; i < 4; i++) { >>> + if (qcom->dm_hs_phy_irq[i]) >>> + continue; >>> + >>> + sprintf(irq_name, "dm%d_hs_phy_irq", i+1); >>> + irq = dwc3_qcom_get_irq(pdev, irq_name, -1); >>> + if (irq > 0) { >>> + irq_set_status_flags(irq, IRQ_NOAUTOEN); >>> + ret = devm_request_threaded_irq(qcom->dev, irq, NULL, >>> + qcom_dwc3_resume_irq, >>> + IRQF_TRIGGER_HIGH | IRQF_ONESHOT, >>> + irq_name, qcom); >>> + if (ret) { >>> + dev_err(qcom->dev, "%s failed: %d\n", irq_name, ret); >>> + return ret; >>> + } >>> + } >>> + >>> + qcom->dm_hs_phy_irq[i] = irq; >>> + } >>> + >>> + for (i = 0; i < 2; i++) { >>> + if (qcom->ss_phy_irq[i]) >>> + continue; >>> + >>> + sprintf(irq_name, "ss%d_phy_irq", i+1); >>> + irq = dwc3_qcom_get_irq(pdev, irq_name, -1); >>> + if (irq > 0) { >>> + irq_set_status_flags(irq, IRQ_NOAUTOEN); >>> + ret = devm_request_threaded_irq(qcom->dev, irq, NULL, >>> + qcom_dwc3_resume_irq, >>> + IRQF_TRIGGER_HIGH | IRQF_ONESHOT, >>> + irq_name, qcom); >>> + if (ret) { >>> + dev_err(qcom->dev, "%s failed: %d\n", irq_name, ret); >>> + return ret; >>> + } >>> + } >>> + >>> + qcom->ss_phy_irq[i] = irq; >>> + } >> >> So the above should all be merged in either a single helper looking up >> all the interrupts for a port and resused for the non-MP case. >> > I agree, Will merge all under some common helper function. > Hi Johan, One more thought. To make one single function read all the interrupts (Single port or multi port), we need to provide proper inputs to get_irq call (i.e., "dp_hs_phy_irq" or dp_hs_phy_irq_X" and for that we need to know if device is multiport capable or not. Either of the following ways needs to be done: 1. Try getting all interrupts (MP or single port) and accordingly save it in qcom struct like done in this patch in which case setup_irq and get_mp_irq being seperated is the best option and we don't need to bother about whether we are MP capable or not. 2. Else, we need to find out if we are MP capable and read number of ports and accordingly get the IRQ's which will just complicate the code because of_platform_populate is done after setup_irq. Even if I move setup_irq to after of_platform_populate, what dwc3 probe was deferred or failed, we would not know what IRQ's to read. 3. If we want to know whether we are MP capable or not in dwc3-qcom even before of_platform_populate, we need to find out a way to get port info which will just be duplication of code or re-implementation of code done in core.c [1]. 4. One more option would be to defer qcom probe if dwc3 probe is not done and move setup_irq to be called after of_platform_populate. This way we can be sure we are not dereferencing dwc3->drvdata without knowing if it is NULL or not. Since qcom probe happened, we are sure dwc3 probe too happened. We would know if we are MP capable or not while setting up IRQ and we can read IRQ's appropriately. Logically, dwc3-qcom is nothing without dwc3 core getting probed and becoming active. So it would be better IMO to defer qcom probe if dwc3 probe doesn't happen and that way even the layering violations too would go away. I hope this idea appeals to the issues we are dealing with. [1]: https://lore.kernel.org/all/20230621043628.21485-4-quic_kriskura@quicinc.com/ Adding Bjorn and Konrad as well to know their thoughts on Point-4. Regards, Krishna, >>> + return 0; >>> +} >>> + >>> static int dwc3_qcom_setup_irq(struct platform_device *pdev) >>> { >>> struct dwc3_qcom *qcom = platform_get_drvdata(pdev); >>> @@ -570,7 +644,7 @@ static int dwc3_qcom_setup_irq(struct >>> platform_device *pdev) >>> dev_err(qcom->dev, "dp_hs_phy_irq failed: %d\n", ret); >>> return ret; >>> } >>> - qcom->dp_hs_phy_irq = irq; >>> + qcom->dp_hs_phy_irq[0] = irq; >>> } >>> irq = dwc3_qcom_get_irq(pdev, "dm_hs_phy_irq", >>> @@ -585,7 +659,7 @@ static int dwc3_qcom_setup_irq(struct >>> platform_device *pdev) >>> dev_err(qcom->dev, "dm_hs_phy_irq failed: %d\n", ret); >>> return ret; >>> } >>> - qcom->dm_hs_phy_irq = irq; >>> + qcom->dm_hs_phy_irq[0] = irq; >>> } >>> irq = dwc3_qcom_get_irq(pdev, "ss_phy_irq", >>> @@ -600,10 +674,10 @@ static int dwc3_qcom_setup_irq(struct >>> platform_device *pdev) >>> dev_err(qcom->dev, "ss_phy_irq failed: %d\n", ret); >>> return ret; >>> } >>> - qcom->ss_phy_irq = irq; >>> + qcom->ss_phy_irq[0] = irq; >>> } >>> - return 0; >>> + return dwc3_qcom_setup_mp_irq(pdev);; >> >> Stray ; >> >>> } >>> static int dwc3_qcom_clk_init(struct dwc3_qcom *qcom, int count) >> >> Johan
On Wed, Jun 21, 2023 at 10:06:24AM +0530, Krishna Kurapati wrote: > Add support to read Multiport IRQ's related to quad port controller > of SA8295 Device. > > Signed-off-by: Krishna Kurapati <quic_kriskura@quicinc.com> > --- > drivers/usb/dwc3/dwc3-qcom.c | 108 +++++++++++++++++++++++++++++------ > 1 file changed, 91 insertions(+), 17 deletions(-) > +static int dwc3_qcom_setup_mp_irq(struct platform_device *pdev) > +{ > + struct dwc3_qcom *qcom = platform_get_drvdata(pdev); > + char irq_name[15]; The interrupt device-name string can not be allocated on the stack or reused as it is stored directly in each irqaction structure. This can otherwise lead to random crashes when accessing /proc/interrupts: https://lore.kernel.org/lkml/ZK6IV_jJPICX5r53@hovoldconsulting.com/ > + int irq; > + int ret; > + int i; > + > + for (i = 0; i < 4; i++) { > + if (qcom->dp_hs_phy_irq[i]) > + continue; > + > + sprintf(irq_name, "dp%d_hs_phy_irq", i+1); > + irq = dwc3_qcom_get_irq(pdev, irq_name, -1); > + if (irq > 0) { > + irq_set_status_flags(irq, IRQ_NOAUTOEN); > + ret = devm_request_threaded_irq(qcom->dev, irq, NULL, > + qcom_dwc3_resume_irq, > + IRQF_TRIGGER_HIGH | IRQF_ONESHOT, > + irq_name, qcom); > + if (ret) { > + dev_err(qcom->dev, "%s failed: %d\n", irq_name, ret); > + return ret; > + } > + } > + > + qcom->dp_hs_phy_irq[i] = irq; > + } Johan
On 7/12/2023 5:42 PM, Johan Hovold wrote: > On Wed, Jun 21, 2023 at 10:06:24AM +0530, Krishna Kurapati wrote: >> Add support to read Multiport IRQ's related to quad port controller >> of SA8295 Device. >> >> Signed-off-by: Krishna Kurapati <quic_kriskura@quicinc.com> >> --- >> drivers/usb/dwc3/dwc3-qcom.c | 108 +++++++++++++++++++++++++++++------ >> 1 file changed, 91 insertions(+), 17 deletions(-) > >> +static int dwc3_qcom_setup_mp_irq(struct platform_device *pdev) >> +{ >> + struct dwc3_qcom *qcom = platform_get_drvdata(pdev); >> + char irq_name[15]; > > The interrupt device-name string can not be allocated on the stack or > reused as it is stored directly in each irqaction structure. > > This can otherwise lead to random crashes when accessing > /proc/interrupts: > > https://lore.kernel.org/lkml/ZK6IV_jJPICX5r53@hovoldconsulting.com/ > Hi Johan, Sure, will create a static array of names if possible in global section of file and use it to read interrupts. Are you fine with seperating out setup_irq and setup_mp_irq functions ? Can you please review comments and suggestion on [1]. [1]: https://lore.kernel.org/all/bf62bdf4-cc9e-ba7b-2078-cfd60f5dd237@quicinc.com/ Regards, Krishna, >> + int irq; >> + int ret; >> + int i; >> + >> + for (i = 0; i < 4; i++) { >> + if (qcom->dp_hs_phy_irq[i]) >> + continue; >> + >> + sprintf(irq_name, "dp%d_hs_phy_irq", i+1); >> + irq = dwc3_qcom_get_irq(pdev, irq_name, -1); >> + if (irq > 0) { >> + irq_set_status_flags(irq, IRQ_NOAUTOEN); >> + ret = devm_request_threaded_irq(qcom->dev, irq, NULL, >> + qcom_dwc3_resume_irq, >> + IRQF_TRIGGER_HIGH | IRQF_ONESHOT, >> + irq_name, qcom); >> + if (ret) { >> + dev_err(qcom->dev, "%s failed: %d\n", irq_name, ret); >> + return ret; >> + } >> + } >> + >> + qcom->dp_hs_phy_irq[i] = irq; >> + } > > Johan
On Wed, Jul 12, 2023 at 11:56:33PM +0530, Krishna Kurapati PSSNV wrote: > On 7/12/2023 5:42 PM, Johan Hovold wrote: > > On Wed, Jun 21, 2023 at 10:06:24AM +0530, Krishna Kurapati wrote: > >> Add support to read Multiport IRQ's related to quad port controller > >> of SA8295 Device. > >> > >> Signed-off-by: Krishna Kurapati <quic_kriskura@quicinc.com> > >> --- > >> drivers/usb/dwc3/dwc3-qcom.c | 108 +++++++++++++++++++++++++++++------ > >> 1 file changed, 91 insertions(+), 17 deletions(-) > > > >> +static int dwc3_qcom_setup_mp_irq(struct platform_device *pdev) > >> +{ > >> + struct dwc3_qcom *qcom = platform_get_drvdata(pdev); > >> + char irq_name[15]; > > > > The interrupt device-name string can not be allocated on the stack or > > reused as it is stored directly in each irqaction structure. > > > > This can otherwise lead to random crashes when accessing > > /proc/interrupts: > > > > https://lore.kernel.org/lkml/ZK6IV_jJPICX5r53@hovoldconsulting.com/ > Sure, will create a static array of names if possible in global > section of file and use it to read interrupts. Or just use devm_kasprintf(), which should allow for a cleaner implementation. I've fixed it up like this for my X13s wip branches: https://github.com/jhovold/linux/commit/0898b54456bc2f4bd4d420480db98e6758771ace > Are you fine with seperating out setup_irq and setup_mp_irq functions > ? Can you please review comments and suggestion on [1]. I haven't had time to look at your latest replies yet, but as I already said when reviewing v9, it seems you should be using a common helper for non-mp and mp. Johan
On 7/14/2023 2:35 PM, Johan Hovold wrote: > On Wed, Jul 12, 2023 at 11:56:33PM +0530, Krishna Kurapati PSSNV wrote: >> On 7/12/2023 5:42 PM, Johan Hovold wrote: >>> On Wed, Jun 21, 2023 at 10:06:24AM +0530, Krishna Kurapati wrote: >>>> Add support to read Multiport IRQ's related to quad port controller >>>> of SA8295 Device. >>>> >>>> Signed-off-by: Krishna Kurapati <quic_kriskura@quicinc.com> >>>> --- >>>> drivers/usb/dwc3/dwc3-qcom.c | 108 +++++++++++++++++++++++++++++------ >>>> 1 file changed, 91 insertions(+), 17 deletions(-) >>> >>>> +static int dwc3_qcom_setup_mp_irq(struct platform_device *pdev) >>>> +{ >>>> + struct dwc3_qcom *qcom = platform_get_drvdata(pdev); >>>> + char irq_name[15]; >>> >>> The interrupt device-name string can not be allocated on the stack or >>> reused as it is stored directly in each irqaction structure. >>> >>> This can otherwise lead to random crashes when accessing >>> /proc/interrupts: >>> >>> https://lore.kernel.org/lkml/ZK6IV_jJPICX5r53@hovoldconsulting.com/ > >> Sure, will create a static array of names if possible in global >> section of file and use it to read interrupts. > > Or just use devm_kasprintf(), which should allow for a cleaner > implementation. > > I've fixed it up like this for my X13s wip branches: > > https://github.com/jhovold/linux/commit/0898b54456bc2f4bd4d420480db98e6758771ace > >> Are you fine with seperating out setup_irq and setup_mp_irq functions >> ? Can you please review comments and suggestion on [1]. > > I haven't had time to look at your latest replies yet, but as I already > said when reviewing v9, it seems you should be using a common helper for > non-mp and mp. > Hi Johan, The gist of my mail was to see if I can defer qcom probe when dwc3 probe fails/or doesn't happen on of_plat_pop (which is logical) so that we can move setup_irq to after dwc3_register_core so that we know whether we are MP capable or not. This would help us move all IRQ reading into one function. Regards, Krishna,
On 7/14/2023 4:10 PM, Krishna Kurapati PSSNV wrote: > > > On 7/14/2023 2:35 PM, Johan Hovold wrote: >> On Wed, Jul 12, 2023 at 11:56:33PM +0530, Krishna Kurapati PSSNV wrote: >>> On 7/12/2023 5:42 PM, Johan Hovold wrote: >>>> On Wed, Jun 21, 2023 at 10:06:24AM +0530, Krishna Kurapati wrote: >>>>> Add support to read Multiport IRQ's related to quad port controller >>>>> of SA8295 Device. >>>>> >>>>> Signed-off-by: Krishna Kurapati <quic_kriskura@quicinc.com> >>>>> --- >>>>> drivers/usb/dwc3/dwc3-qcom.c | 108 >>>>> +++++++++++++++++++++++++++++------ >>>>> 1 file changed, 91 insertions(+), 17 deletions(-) >>>> >>>>> +static int dwc3_qcom_setup_mp_irq(struct platform_device *pdev) >>>>> +{ >>>>> + struct dwc3_qcom *qcom = platform_get_drvdata(pdev); >>>>> + char irq_name[15]; >>>> >>>> The interrupt device-name string can not be allocated on the stack or >>>> reused as it is stored directly in each irqaction structure. >>>> >>>> This can otherwise lead to random crashes when accessing >>>> /proc/interrupts: >>>> >>>> https://lore.kernel.org/lkml/ZK6IV_jJPICX5r53@hovoldconsulting.com/ >> >>> Sure, will create a static array of names if possible in global >>> section of file and use it to read interrupts. >> >> Or just use devm_kasprintf(), which should allow for a cleaner >> implementation. >> >> I've fixed it up like this for my X13s wip branches: >> >> https://github.com/jhovold/linux/commit/0898b54456bc2f4bd4d420480db98e6758771ace >>> Are you fine with seperating out setup_irq and setup_mp_irq >>> functions >>> ? Can you please review comments and suggestion on [1]. >> >> I haven't had time to look at your latest replies yet, but as I already >> said when reviewing v9, it seems you should be using a common helper for >> non-mp and mp. >> > Hi Johan, > > The gist of my mail was to see if I can defer qcom probe when dwc3 > probe fails/or doesn't happen on of_plat_pop (which is logical) so that > we can move setup_irq to after dwc3_register_core so that we know > whether we are MP capable or not. This would help us move all IRQ > reading into one function. > Hi Johan, I see it is difficult to write a common helper. To do so, we need to know whether the device is MP capable or not in advance. And since it is not possible to know it before of_plat_pop is done, I see only few ways to do it: 1. Based on qcom node compatible string, I can read whether the device is MP capable or not and get IRQ's accordingly. 2. Read the port_info in advance but it needs me to go through some DT props and try getting this info. Or read xhci regs like we are doing in core (which is not good). Also since some Dt props can be missing, is it difficult to get the MP capability info before of_plat_pop is done. 3. Remove IRQ handling completely. Just because the device has IRQ's present, I don't see a point in adding them to bindings, and because we added them to bindings, we are making a patch to read them (and since this is a little challenging, the whole of multiport series is blocked although I don't need wakeup support on these interrupts right away). Can't we let the rest of the patches go through and let interrupt handling for 2nd, 3rd and 4rth ports be taken care later ? I am asking this because I want the rest of the patches which are in good shape now (after fixing the nits mentioned) to get merged atleast. I will make sure to add interrupt handling later in a different series once this is merged once I send v10. Or if there is a simpler way to do it, I would be happy to take any suggestions and complete this missing part in this series itself. Regards, Krishna,
On 7/16/2023 12:31 AM, Krishna Kurapati PSSNV wrote: > > > On 7/14/2023 4:10 PM, Krishna Kurapati PSSNV wrote: >> >> >> On 7/14/2023 2:35 PM, Johan Hovold wrote: >>> On Wed, Jul 12, 2023 at 11:56:33PM +0530, Krishna Kurapati PSSNV wrote: >>>> On 7/12/2023 5:42 PM, Johan Hovold wrote: >>>>> On Wed, Jun 21, 2023 at 10:06:24AM +0530, Krishna Kurapati wrote: >>>>>> Add support to read Multiport IRQ's related to quad port controller >>>>>> of SA8295 Device. >>>>>> >>>>>> Signed-off-by: Krishna Kurapati <quic_kriskura@quicinc.com> >>>>>> --- >>>>>> drivers/usb/dwc3/dwc3-qcom.c | 108 >>>>>> +++++++++++++++++++++++++++++------ >>>>>> 1 file changed, 91 insertions(+), 17 deletions(-) >>>>> >>>>>> +static int dwc3_qcom_setup_mp_irq(struct platform_device *pdev) >>>>>> +{ >>>>>> + struct dwc3_qcom *qcom = platform_get_drvdata(pdev); >>>>>> + char irq_name[15]; >>>>> >>>>> The interrupt device-name string can not be allocated on the stack or >>>>> reused as it is stored directly in each irqaction structure. >>>>> >>>>> This can otherwise lead to random crashes when accessing >>>>> /proc/interrupts: >>>>> >>>>> https://lore.kernel.org/lkml/ZK6IV_jJPICX5r53@hovoldconsulting.com/ >>> >>>> Sure, will create a static array of names if possible in global >>>> section of file and use it to read interrupts. >>> >>> Or just use devm_kasprintf(), which should allow for a cleaner >>> implementation. >>> >>> I've fixed it up like this for my X13s wip branches: >>> >>> https://github.com/jhovold/linux/commit/0898b54456bc2f4bd4d420480db98e6758771ace >>>> Are you fine with seperating out setup_irq and setup_mp_irq >>>> functions >>>> ? Can you please review comments and suggestion on [1]. >>> >>> I haven't had time to look at your latest replies yet, but as I already >>> said when reviewing v9, it seems you should be using a common helper for >>> non-mp and mp. >>> >> Hi Johan, >> >> The gist of my mail was to see if I can defer qcom probe when dwc3 >> probe fails/or doesn't happen on of_plat_pop (which is logical) so >> that we can move setup_irq to after dwc3_register_core so that we know >> whether we are MP capable or not. This would help us move all IRQ >> reading into one function. >> > Hi Johan, > > I see it is difficult to write a common helper. To do so, we need to > know whether the device is MP capable or not in advance. And since it is > not possible to know it before of_plat_pop is done, I see only few ways > to do it: > > 1. Based on qcom node compatible string, I can read whether the device > is MP capable or not and get IRQ's accordingly. > > 2. Read the port_info in advance but it needs me to go through some DT > props and try getting this info. Or read xhci regs like we are doing in > core (which is not good). Also since some Dt props can be missing, is it > difficult to get the MP capability info before of_plat_pop is done. > > 3. Remove IRQ handling completely. Just because the device has IRQ's > present, I don't see a point in adding them to bindings, and because we > added them to bindings, we are making a patch to read them (and since > this is a little challenging, the whole of multiport series is blocked > although I don't need wakeup support on these interrupts right away). > > Can't we let the rest of the patches go through and let interrupt > handling for 2nd, 3rd and 4rth ports be taken care later ? I am asking > this because I want the rest of the patches which are in good shape now > (after fixing the nits mentioned) to get merged atleast. I will make > sure to add interrupt handling later in a different series once this is > merged once I send v10. > > Or if there is a simpler way to do it, I would be happy to take any > suggestions and complete this missing part in this series itself. > Hi Konrad, Bjorn, Can you also let me know your review on this. Can we add a compatible data to dwc3-qcom to identify whether we need to read MP irq's or non-NP irq's and also if it is better to split this series into two. (One for multiport and one for dwc3-qcom IRQ's) Regards, Krishna,
Hi Krishna, Please remember to trim unnecessary context from your replies (e.g. as it makes it easier to read your reply and later the entire thread in the mailing list archives). On Mon, Jul 03, 2023 at 12:18:14AM +0530, Krishna Kurapati PSSNV wrote: > On 6/27/2023 5:15 PM, Johan Hovold wrote: > > On Wed, Jun 21, 2023 at 10:06:21AM +0530, Krishna Kurapati wrote: > >> diff --git a/drivers/usb/dwc3/core.h b/drivers/usb/dwc3/core.h > >> index 8b1295e4dcdd..42fb17aa66fa 100644 > >> --- a/drivers/usb/dwc3/core.h > >> +++ b/drivers/usb/dwc3/core.h > >> @@ -33,6 +33,10 @@ > >> > >> #include <linux/power_supply.h> > >> > >> +#define XHCI_EXT_PORT_MAJOR(x) (((x) >> 24) & 0xff) > >> +#define XHCI_EXT_PORT_MINOR(x) (((x) >> 16) & 0xff) > >> +#define XHCI_EXT_PORT_COUNT(x) (((x) >> 8) & 0xff) > > > > Again, don't copy defines from xhci. > > > > Looks like these should be moved to the xhci-ext-caps.h header along > > with struct xhci_protocol_caps. > > > Can't we just give them an exception ? Modifying xhci stuff doesn't > sound good. Can I just rename them and keep them here ? Nope. This is mainline, not Qualcomm's vendor kernel. Johan
On Mon, Jul 03, 2023 at 12:26:26AM +0530, Krishna Kurapati PSSNV wrote: > On 6/27/2023 5:39 PM, Johan Hovold wrote: > > On Wed, Jun 21, 2023 at 10:06:23AM +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> > >> [Krishna: Modifed logic for generic phy and rebased the patch] > >> Signed-off-by: Krishna Kurapati <quic_kriskura@quicinc.com> > > > > As I already said: > > > > If Harsh is the primary author you need to add a From: line at > > the beginning of the patch. > > > > Either way, you need his SoB as well as your Co-developed-by tag. > > > > All this is documented under Documentation/process/ somewhere. > > > > The above is missing a From line and two Co-developed-by tags at least. > I tried to follow the following commit: > > 8030cb9a5568 ("soc: qcom: aoss: remove spurious IRQF_ONESHOT flags") > > Let me know if that is not acceptable. I don't see how that commit relevant to the discussion at hand. Please just fix your use of Signed-off-by and Co-developed-by tags that I've now pointed out repeatedly. If you can't figure it out by yourself after the feedback I've already given you need to ask someone inside Qualcomm. You work for a huge company that should provide resources for training it's developers in basic process issues like this. > >> @@ -120,10 +120,11 @@ void dwc3_set_prtcap(struct dwc3 *dwc, u32 mode) > >> static void __dwc3_set_mode(struct work_struct *work) > >> { > >> struct dwc3 *dwc = work_to_dwc(work); > >> + u32 desired_dr_role; > >> unsigned long flags; > >> int ret; > >> u32 reg; > >> - u32 desired_dr_role; > > > > This is an unrelated change. Just add int i at the end. > > > I was trying to keep the reverse xmas order of variables. That's generally good, but you should not change unrelated code as part of this patch. It's fine to leave this as is for now. > >> + int i; > >> > >> mutex_lock(&dwc->mutex); > >> spin_lock_irqsave(&dwc->lock, flags); > > > >> @@ -746,23 +779,34 @@ static int dwc3_phy_setup(struct dwc3 *dwc) > >> static int dwc3_phy_init(struct dwc3 *dwc) > >> { > >> int ret; > >> + int i; > >> + int j; > >> > >> usb_phy_init(dwc->usb2_phy); > >> usb_phy_init(dwc->usb3_phy); > >> > >> - ret = phy_init(dwc->usb2_generic_phy); > >> - if (ret < 0) > >> - goto err_shutdown_usb3_phy; > >> + for (i = 0; i < dwc->num_usb2_ports; i++) { > >> + ret = phy_init(dwc->usb2_generic_phy[i]); > >> + if (ret < 0) > >> + goto err_exit_usb2_phy; > >> + } > >> > >> - ret = phy_init(dwc->usb3_generic_phy); > >> - if (ret < 0) > >> - goto err_exit_usb2_phy; > >> + for (i = 0; i < dwc->num_usb2_ports; i++) { > >> + ret = phy_init(dwc->usb3_generic_phy[i]); > >> + if (ret < 0) > >> + goto err_exit_usb3_phy; > >> + } > >> > >> return 0; > >> > >> +err_exit_usb3_phy: > >> + for (j = i-1; j >= 0; j--) > > > > Missing spaces around - here and below. > > > >> + phy_exit(dwc->usb3_generic_phy[j]); > >> + i = dwc->num_usb2_ports; > >> err_exit_usb2_phy: > >> - phy_exit(dwc->usb2_generic_phy); > >> -err_shutdown_usb3_phy: > >> + for (j = i-1; j >= 0; j--) > >> + phy_exit(dwc->usb2_generic_phy[j]); > >> + > > > > Again: > > > > The above is probably better implemented as a *single* loop over > > num_usb2_ports where you enable each USB2 and USB3 PHY. On > > errors you use the loop index to disable the already enabled > > PHYs in reverse order below (after disabling the USB2 PHY if > > USB3 phy init fails). > > > > with emphasis on "single" added. > > > Oh, you mean something like this ? > > for (loop over num_ports) { > ret = phy_init(dwc->usb3_generic_phy[i]); > if (ret != 0) > goto err_exit_phy; > > ret = phy_init(dwc->usb2_generic_phy[i]); > if (ret != 0) > goto err_exit_phy; > } > > err_exit_phy: > for (j = i-1; j >= 0; j--) { > phy_exit(dwc->usb3_generic_phy[j]); > phy_exit(dwc->usb2_generic_phy[j]); > } Yeah, something like that, but you need to disable the usb3[i] phy when usb2[2] init fail above (and I'd also keep the order of initialising usb2 before usb3). > >> usb_phy_shutdown(dwc->usb3_phy); > >> usb_phy_shutdown(dwc->usb2_phy); > >> diff --git a/drivers/usb/dwc3/core.h b/drivers/usb/dwc3/core.h > >> index 42fb17aa66fa..b2bab23ca22b 100644 > >> --- a/drivers/usb/dwc3/core.h > >> +++ b/drivers/usb/dwc3/core.h > >> @@ -37,6 +37,9 @@ > >> #define XHCI_EXT_PORT_MINOR(x) (((x) >> 16) & 0xff) > >> #define XHCI_EXT_PORT_COUNT(x) (((x) >> 8) & 0xff) > >> > >> +/* Number of ports supported by a multiport controller */ > >> +#define DWC3_MAX_PORTS 4 > > > > You did not answer my question about whether this was an arbitrary > > implementation limit (i.e. just reflecting the only currently supported > > multiport controller)? > > > I mentioned in commit text that it is limited to 4. Are you referring to > state the reason why I chose the value 4 ? Yes, and to clarify whether this was an arbitrary limit you chose because it was all that was needed for the hw you care about, or if it's a more general limitation. Johan
On Mon, Jul 03, 2023 at 12:40:19AM +0530, Krishna Kurapati PSSNV wrote: > On 6/27/2023 8:46 PM, Johan Hovold wrote: > > On Sat, Jun 24, 2023 at 12:43:23PM +0530, Krishna Kurapati PSSNV wrote: > >>> On 21.06.2023 06:36, Krishna Kurapati wrote: > >>>> Add USB and DWC3 node for tertiary port of SC8280 along with multiport > >>>> IRQ's and phy's. This will be used as a base for SA8295P and SA8295-Ride > >>>> platforms. > >>>> > >>>> Signed-off-by: Krishna Kurapati <quic_kriskura@quicinc.com> > >> Yes wakeup is supported by all ports now, but I didn't make those > >> changes now as I wanted to keep driver code diff minimal and don't need > >> wakeup support for the product currently. But for sure, will update > >> driver code to handle wakeup on all ports in near future. > > > > Why didn't you include it in v9? I thought you had a working > > implementation for this? > > > > Since wakeup will be another case where glue and core need to interact, > > it's good to have the wakeup implementation from the start to be able to > > evaluate your multiport implementation properly. > > > > Right now it looks like you only added wakeup interrupt lookup and > > request, but then you never actually enable them which is not very nice. > As mentioned in one of my comments on earlier patches, wakeup is not a > requirement I currently need to work on for the product. I added > multiport IRQ support only because my pathces need to modify IRQ names. > If there is a customer requirement I get in the future, I will > definitely implement the wakeup part. But for now, I would like to stick > to what is necessary for getting Multiport to work. I think you need to implement this now as this is a basic features of any USB controller and one which is already supported by the driver you are changing. We've also had a long of history of Qualcomm pushing incomplete implementations upstream and then they move on to more pressing deadline and never actually complete the work. This very wakeup support is a good example of this as parts of it was merged years ago and when someone later tried to get it to actually work, it turned into a complete hack of an implementation as no one had thought about the overall design. Johan
On Mon, Jul 03, 2023 at 12:29:41AM +0530, Krishna Kurapati PSSNV wrote: > On 6/27/2023 8:01 PM, Johan Hovold wrote: > > On Wed, Jun 21, 2023 at 10:06:24AM +0530, Krishna Kurapati wrote: > >> +static int dwc3_qcom_setup_mp_irq(struct platform_device *pdev) > >> +{ > >> + struct dwc3_qcom *qcom = platform_get_drvdata(pdev); > >> + char irq_name[15]; > >> + int irq; > >> + int ret; > >> + int i; > >> + > >> + for (i = 0; i < 4; i++) { > > > > DWC3_MAX_PORTS here too and similar below. > > > >> + if (qcom->dp_hs_phy_irq[i]) > >> + continue; > > > > This is not very nice. You should try to integrate the current lookup > > code as I told you to do with the PHY lookups. That is, use a single > > loop for all HS/SS IRQs, and pick the legacy name if the number of ports > > is 1. > > > > Of course, you added the xhci capability parsing to the core driver so > > that information is not yet available, but you need it in the glue > > driver also... > > > > As I mentioned earlier, you can infer the number of ports from the > > interrupt names. Alternatively, you can infer it from the compatible > > string. In any case, you should not need to ways to determine the same > > information in the glue driver, then in the core part, and then yet > > again in the xhci driver... > The reason why I didn't integrate this with the original function was > the ACPI stuff. The MP devices have no ACPI variant. And I think for > clarity sake its best to keep these two functions separated. No. The ACPI stuff may make this a little harder to implement, but that's not a sufficient reason to not try to refactor things properly. > >> + > >> + sprintf(irq_name, "dp%d_hs_phy_irq", i+1); > > > > Spaces around binary operators. Does not checkpatch warn about that? > > > >> + irq = dwc3_qcom_get_irq(pdev, irq_name, -1); > >> + if (irq > 0) { > >> + irq_set_status_flags(irq, IRQ_NOAUTOEN); > >> + ret = devm_request_threaded_irq(qcom->dev, irq, NULL, > >> + qcom_dwc3_resume_irq, > >> + IRQF_TRIGGER_HIGH | IRQF_ONESHOT, > >> + irq_name, qcom); > >> + if (ret) { > >> + dev_err(qcom->dev, "%s failed: %d\n", irq_name, ret); > >> + return ret; > >> + } > >> + } > >> + > >> + qcom->dp_hs_phy_irq[i] = irq; > >> + } > >> + > >> + for (i = 0; i < 4; i++) { > >> + if (qcom->dm_hs_phy_irq[i]) > >> + continue; > >> + > >> + sprintf(irq_name, "dm%d_hs_phy_irq", i+1); > >> + irq = dwc3_qcom_get_irq(pdev, irq_name, -1); > >> + if (irq > 0) { > >> + irq_set_status_flags(irq, IRQ_NOAUTOEN); > >> + ret = devm_request_threaded_irq(qcom->dev, irq, NULL, > >> + qcom_dwc3_resume_irq, > >> + IRQF_TRIGGER_HIGH | IRQF_ONESHOT, > >> + irq_name, qcom); > >> + if (ret) { > >> + dev_err(qcom->dev, "%s failed: %d\n", irq_name, ret); > >> + return ret; > >> + } > >> + } > >> + > >> + qcom->dm_hs_phy_irq[i] = irq; > >> + } > >> + > >> + for (i = 0; i < 2; i++) { > >> + if (qcom->ss_phy_irq[i]) > >> + continue; > >> + > >> + sprintf(irq_name, "ss%d_phy_irq", i+1); > >> + irq = dwc3_qcom_get_irq(pdev, irq_name, -1); > >> + if (irq > 0) { > >> + irq_set_status_flags(irq, IRQ_NOAUTOEN); > >> + ret = devm_request_threaded_irq(qcom->dev, irq, NULL, > >> + qcom_dwc3_resume_irq, > >> + IRQF_TRIGGER_HIGH | IRQF_ONESHOT, > >> + irq_name, qcom); > >> + if (ret) { > >> + dev_err(qcom->dev, "%s failed: %d\n", irq_name, ret); > >> + return ret; > >> + } > >> + } > >> + > >> + qcom->ss_phy_irq[i] = irq; > >> + } > > > > So the above should all be merged in either a single helper looking up > > all the interrupts for a port and resused for the non-MP case. > > > I agree, Will merge all under some common helper function. Good. Johan
On Sun, Jul 16, 2023 at 12:31:05AM +0530, Krishna Kurapati PSSNV wrote: > On 7/14/2023 4:10 PM, Krishna Kurapati PSSNV wrote: > > On 7/14/2023 2:35 PM, Johan Hovold wrote: > >> I haven't had time to look at your latest replies yet, but as I already > >> said when reviewing v9, it seems you should be using a common helper for > >> non-mp and mp. > > The gist of my mail was to see if I can defer qcom probe when dwc3 > > probe fails/or doesn't happen on of_plat_pop (which is logical) so that > > we can move setup_irq to after dwc3_register_core so that we know > > whether we are MP capable or not. This would help us move all IRQ > > reading into one function. > I see it is difficult to write a common helper. To do so, we need to > know whether the device is MP capable or not in advance. And since it is > not possible to know it before of_plat_pop is done, I see only few ways > to do it: > > 1. Based on qcom node compatible string, I can read whether the device > is MP capable or not and get IRQ's accordingly. See, it's not impossible. You can also determine whether you have a multiport controller from looking at the interrupt names which are indexed and distinct for MP. > 2. Read the port_info in advance but it needs me to go through some DT > props and try getting this info. Or read xhci regs like we are doing in > core (which is not good). Also since some Dt props can be missing, is it > difficult to get the MP capability info before of_plat_pop is done. That seem unnecessary currently, but long term we probably need to fix the design of this driver and defer some setup using callbacks that are called when the core driver probes. Perhaps now is the time to add such functionality. > 3. Remove IRQ handling completely. Just because the device has IRQ's > present, I don't see a point in adding them to bindings, and because we > added them to bindings, we are making a patch to read them (and since > this is a little challenging, the whole of multiport series is blocked > although I don't need wakeup support on these interrupts right away). Again, no. The devicetree binding should describe the hardware capabilities and that has nothing to do with whether you need this for you current project or not. > Can't we let the rest of the patches go through and let interrupt > handling for 2nd, 3rd and 4rth ports be taken care later ? I am asking > this because I want the rest of the patches which are in good shape now > (after fixing the nits mentioned) to get merged atleast. I will make > sure to add interrupt handling later in a different series once this is > merged once I send v10. As I've explained in earlier mails, I don't think that is acceptable as you'd be dumping your technical debt on the community which will be left to clean up your mess. > Or if there is a simpler way to do it, I would be happy to take any > suggestions and complete this missing part in this series itself. Using the 'compatible' or 'interrupt-names' properties seems like the easiest way to determine whether you have an MP controller or not. Johan
On 7/21/2023 2:05 PM, Johan Hovold wrote: > On Sun, Jul 16, 2023 at 12:31:05AM +0530, Krishna Kurapati PSSNV wrote: >> On 7/14/2023 4:10 PM, Krishna Kurapati PSSNV wrote: >>> On 7/14/2023 2:35 PM, Johan Hovold wrote: > >>>> I haven't had time to look at your latest replies yet, but as I already >>>> said when reviewing v9, it seems you should be using a common helper for >>>> non-mp and mp. > >>> The gist of my mail was to see if I can defer qcom probe when dwc3 >>> probe fails/or doesn't happen on of_plat_pop (which is logical) so that >>> we can move setup_irq to after dwc3_register_core so that we know >>> whether we are MP capable or not. This would help us move all IRQ >>> reading into one function. > >> I see it is difficult to write a common helper. To do so, we need to >> know whether the device is MP capable or not in advance. And since it is >> not possible to know it before of_plat_pop is done, I see only few ways >> to do it: >> >> 1. Based on qcom node compatible string, I can read whether the device >> is MP capable or not and get IRQ's accordingly. > > See, it's not impossible. You can also determine whether you have a > multiport controller from looking at the interrupt names which are > indexed and distinct for MP. > >> 2. Read the port_info in advance but it needs me to go through some DT >> props and try getting this info. Or read xhci regs like we are doing in >> core (which is not good). Also since some Dt props can be missing, is it >> difficult to get the MP capability info before of_plat_pop is done. > > That seem unnecessary currently, but long term we probably need to fix > the design of this driver and defer some setup using callbacks that are > called when the core driver probes. Perhaps now is the time to add such > functionality. > >> 3. Remove IRQ handling completely. Just because the device has IRQ's >> present, I don't see a point in adding them to bindings, and because we >> added them to bindings, we are making a patch to read them (and since >> this is a little challenging, the whole of multiport series is blocked >> although I don't need wakeup support on these interrupts right away). > > Again, no. The devicetree binding should describe the hardware > capabilities and that has nothing to do with whether you need this for > you current project or not. > >> Can't we let the rest of the patches go through and let interrupt >> handling for 2nd, 3rd and 4rth ports be taken care later ? I am asking >> this because I want the rest of the patches which are in good shape now >> (after fixing the nits mentioned) to get merged atleast. I will make >> sure to add interrupt handling later in a different series once this is >> merged once I send v10. > > As I've explained in earlier mails, I don't think that is acceptable as > you'd be dumping your technical debt on the community which will be left > to clean up your mess. > >> Or if there is a simpler way to do it, I would be happy to take any >> suggestions and complete this missing part in this series itself. > Hi Johan, Thanks for these comments. > Using the 'compatible' or 'interrupt-names' properties seems like the > easiest way to determine whether you have an MP controller or not. > Yes, I can make a common helper to get IRQ's based on compatible. I also provided another implementation (which is more unambiguous and better I feel) on [1]. I will take one path forward based on your review of that patch as well. Thanks a lot again for the reviews ! [1]: https://lore.kernel.org/all/f6f2456d-0067-6cd6-3282-8cae7c47a2d3@quicinc.com/ Regards, Krishna,
Again, please remember to trim your replies. On Fri, Jul 21, 2023 at 01:49:37PM +0530, Krishna Kurapati PSSNV wrote: > On 7/21/2023 1:44 PM, Johan Hovold wrote: > > On Mon, Jul 03, 2023 at 12:29:41AM +0530, Krishna Kurapati PSSNV wrote: > >> On 6/27/2023 8:01 PM, Johan Hovold wrote: [...] > >>> So the above should all be merged in either a single helper looking up > >>> all the interrupts for a port and resused for the non-MP case. > How about the implementation in the attached patches. This way we > don't need to know if we are multiport capable or not. As I wrote above, I think you should instead add a common helper for setting up all the interrupts for a port. For example, along the lines of: dwc3_setup_port_irq(int index) { if (index == 0) try non-mp name else generate mp name lookup and request hs irqs lookup and request ss irq lookup and request power irq } dwc3_setup_irq() { determine if MP (num_ports) for each port dwc3_setup_port_irq(port index) } The port irq helper can either be told using a second argument that this is a non-mp controller, or you can first try looking up one of the non-mp names. My mailer discarded your second patch, but you cannot assume that the devices connected to each port are of the same type (e.g. HS or SS) based on what is connected to the first port. Johan
On 7/21/2023 2:51 PM, Johan Hovold wrote: > Again, please remember to trim your replies. > > On Fri, Jul 21, 2023 at 01:49:37PM +0530, Krishna Kurapati PSSNV wrote: >> On 7/21/2023 1:44 PM, Johan Hovold wrote: >>> On Mon, Jul 03, 2023 at 12:29:41AM +0530, Krishna Kurapati PSSNV wrote: >>>> On 6/27/2023 8:01 PM, Johan Hovold wrote: > > [...] > >>>>> So the above should all be merged in either a single helper looking up >>>>> all the interrupts for a port and resused for the non-MP case. > >> How about the implementation in the attached patches. This way we >> don't need to know if we are multiport capable or not. > > As I wrote above, I think you should instead add a common helper for > setting up all the interrupts for a port. For example, along the lines > of: > > dwc3_setup_port_irq(int index) > { > if (index == 0) > try non-mp name > else > generate mp name > > lookup and request hs irqs > lookup and request ss irq > lookup and request power irq > } > > dwc3_setup_irq() > { > determine if MP (num_ports) > > for each port > dwc3_setup_port_irq(port index) > } > The port irq helper can either be told using a second argument that this > is a non-mp controller, or you can first try looking up one of the > non-mp names. > I think I did something similar. I prepared a helper to request IRQ in the patch and the main logic would reside in setup_irq where i would try and get IRQ's. Irrespective of whether we are MP capable or not, how about we read all IRQ's like in the patch attached previously. And the implementation facilitates addition of ACPI to multiport also if required. I am just trying to cover all cases like this by declaring IRQ info in global section. static int dwc3_qcom_prep_irq(struct dwc3_qcom *qcom, char *irq_name, char *disp_name, int irq) { int ret; /* Keep wakeup interrupts disabled until suspend */ irq_set_status_flags(irq, IRQ_NOAUTOEN); ret = devm_request_threaded_irq(/* Give inouts here*/); if (ret) dev_err(qcom->dev, "%s failed: %d\n", irq_name, ret); return ret; } static int dwc3_qcom_setup_irq(struct platform_device *pdev) { for (DP_IRQ[ i = 0-3] { try getting dp_irq_i using MP_IRQ strings if ((ret < 0) and (i == 0)) try getting dp_irq_i using NON_MP_IRQ strings call prep_irq accordingly. } //Run same loop for DM and SS } The second patch was just enabling IRQ's for all ports to support wakeup. > My mailer discarded your second patch, but you cannot assume that the > devices connected to each port are of the same type (e.g. HS or SS) > based on what is connected to the first port. > Are you referring to enabling IRQ's for different ports before going to suspend ? Meaning get the speed of enum on each port and accordingly enable IRQ's ? Regards, Krishna,
On Fri, Jul 21, 2023 at 03:05:36PM +0530, Krishna Kurapati PSSNV wrote: > On 7/21/2023 2:51 PM, Johan Hovold wrote: > > As I wrote above, I think you should instead add a common helper for > > setting up all the interrupts for a port. For example, along the lines > > of: > > > > dwc3_setup_port_irq(int index) > > { > > if (index == 0) > > try non-mp name > > else > > generate mp name > > > > lookup and request hs irqs > > lookup and request ss irq > > lookup and request power irq > > } > > > > dwc3_setup_irq() > > { > > determine if MP (num_ports) > > > > for each port > > dwc3_setup_port_irq(port index) > > } > > The port irq helper can either be told using a second argument that this > > is a non-mp controller, or you can first try looking up one of the > > non-mp names. > I think I did something similar. I prepared a helper to request IRQ in > the patch and the main logic would reside in setup_irq where i would try > and get IRQ's. No, you only added a wrapper around request_irq() but no helper to setup all irqs for a port, so that's not really similar. > Irrespective of whether we are MP capable or not, how about we read all > IRQ's like in the patch attached previously. And the implementation > facilitates addition of ACPI to multiport also if required. I am just > trying to cover all cases like this by declaring IRQ info in global section. > > static int dwc3_qcom_prep_irq(struct dwc3_qcom *qcom, char *irq_name, > char *disp_name, int irq) > { > int ret; > > /* Keep wakeup interrupts disabled until suspend */ > irq_set_status_flags(irq, IRQ_NOAUTOEN); > ret = devm_request_threaded_irq(/* Give inouts here*/); > if (ret) > dev_err(qcom->dev, "%s failed: %d\n", irq_name, ret); > > return ret; > } > > > static int dwc3_qcom_setup_irq(struct platform_device *pdev) > { > for (DP_IRQ[ i = 0-3] { > try getting dp_irq_i using MP_IRQ strings > if ((ret < 0) and (i == 0)) > try getting dp_irq_i using NON_MP_IRQ strings > > call prep_irq accordingly. > } > > //Run same loop for DM and SS > } So again, the above is not setting up all irqs for a port in one place which seems like the natural way to add support for multiport. It should be possible to reuse a per-port setup function also for ACPI. > The second patch was just enabling IRQ's for all ports to support wakeup. > > > My mailer discarded your second patch, but you cannot assume that the > > devices connected to each port are of the same type (e.g. HS or SS) > > based on what is connected to the first port. > > > Are you referring to enabling IRQ's for different ports before going to > suspend ? Meaning get the speed of enum on each port and accordingly > enable IRQ's ? Exactly. That will be needed. Johan
On 6/27/2023 5:15 PM, Johan Hovold wrote: > On Wed, Jun 21, 2023 at 10:06:21AM +0530, Krishna Kurapati wrote: >> Currently host-only capable DWC3 controllers support Multiport. >> Temporarily map XHCI address space for host-only controllers and parse >> XHCI Extended Capabilities registers to read number of usb2 ports and >> usb3 ports present on multiport controller. Each USB Port is at least HS >> capable. >> >> The port info for usb2 and usb3 phy are identified as num_usb2_ports >> and num_usb3_ports. The intention is as follows: >> >> Wherever we need to perform phy operations like: >> >> LOOP_OVER_NUMBER_OF_AVAILABLE_PORTS() >> { >> phy_set_mode(dwc->usb2_generic_phy[i], PHY_MODE_USB_HOST); >> phy_set_mode(dwc->usb3_generic_phy[i], PHY_MODE_USB_HOST); >> } >> >> If number of usb2 ports is 3, loop can go from index 0-2 for >> usb2_generic_phy. If number of usb3-ports is 2, we don't know for sure, >> if the first 2 ports are SS capable or some other ports like (2 and 3) >> are SS capable. So instead, num_usb2_ports is used to loop around all >> phy's (both hs and ss) for performing phy operations. If any >> usb3_generic_phy turns out to be NULL, phy operation just bails out. >> >> num_usb3_ports is used to modify GUSB3PIPECTL registers while setting up >> phy's as we need to know how many SS capable ports are there for this. >> >> Signed-off-by: Krishna Kurapati <quic_kriskura@quicinc.com> >> --- >> drivers/usb/dwc3/core.c | 62 +++++++++++++++++++++++++++++++++++++++++ >> drivers/usb/dwc3/core.h | 9 ++++++ >> 2 files changed, 71 insertions(+) >> >> diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c >> index f6689b731718..32ec05fc242b 100644 >> --- a/drivers/usb/dwc3/core.c >> +++ b/drivers/usb/dwc3/core.c >> @@ -39,6 +39,7 @@ >> #include "io.h" >> >> #include "debug.h" >> +#include "../host/xhci-ext-caps.h" >> >> #define DWC3_DEFAULT_AUTOSUSPEND_DELAY 5000 /* ms */ >> >> @@ -1767,6 +1768,52 @@ static int dwc3_get_clocks(struct dwc3 *dwc) >> return 0; >> } >> >> +static int dwc3_read_port_info(struct dwc3 *dwc) >> +{ >> + void __iomem *base; >> + u8 major_revision; >> + u32 offset = 0; >> + int ret = 0; > > ret is never modified, so drop and return 0 unconditionally below. > > You can add it back later in the series when you start using it. > >> + u32 val; >> + >> + /* >> + * Remap xHCI address space to access XHCI ext cap regs, >> + * since it is needed to get port info. >> + */ >> + base = ioremap(dwc->xhci_resources[0].start, >> + resource_size(&dwc->xhci_resources[0])); >> + if (IS_ERR(base)) >> + return PTR_ERR(base); >> + >> + do { >> + offset = xhci_find_next_ext_cap(base, offset, >> + XHCI_EXT_CAPS_PROTOCOL); >> + > > You can drop this newline. > >> + if (!offset) >> + break; >> + >> + val = readl(base + offset); >> + major_revision = XHCI_EXT_PORT_MAJOR(val); >> + >> + val = readl(base + offset + 0x08); >> + if (major_revision == 0x03) { >> + dwc->num_usb3_ports += XHCI_EXT_PORT_COUNT(val); >> + } else if (major_revision <= 0x02) { >> + dwc->num_usb2_ports += XHCI_EXT_PORT_COUNT(val); >> + } else { >> + dev_err(dwc->dev, >> + "Unrecognized port major revision %d\n", >> + major_revision); >> + } >> + } while (1); >> + >> + dev_dbg(dwc->dev, "hs-ports: %u ss-ports: %u\n", >> + dwc->num_usb2_ports, dwc->num_usb3_ports); >> + >> + iounmap(base); > > Nit: I'd add a newline here. > >> + return ret; >> +} >> + >> static int dwc3_probe(struct platform_device *pdev) >> { >> struct device *dev = &pdev->dev; >> @@ -1774,6 +1821,7 @@ static int dwc3_probe(struct platform_device *pdev) >> void __iomem *regs; >> struct dwc3 *dwc; >> int ret; >> + unsigned int hw_mode; >> >> dwc = devm_kzalloc(dev, sizeof(*dwc), GFP_KERNEL); >> if (!dwc) >> @@ -1854,6 +1902,20 @@ static int dwc3_probe(struct platform_device *pdev) >> goto err_disable_clks; >> } >> >> + /* >> + * Currently only DWC3 controllers that are host-only capable >> + * support Multiport. >> + */ >> + hw_mode = DWC3_GHWPARAMS0_MODE(dwc->hwparams.hwparams0); >> + if (hw_mode == DWC3_GHWPARAMS0_MODE_HOST) { >> + ret = dwc3_read_port_info(dwc); >> + if (ret) >> + goto err_disable_clks; >> + } else { >> + dwc->num_usb2_ports = 1; >> + dwc->num_usb3_ports = 1; >> + } >> + >> spin_lock_init(&dwc->lock); >> mutex_init(&dwc->mutex); >> >> diff --git a/drivers/usb/dwc3/core.h b/drivers/usb/dwc3/core.h >> index 8b1295e4dcdd..42fb17aa66fa 100644 >> --- a/drivers/usb/dwc3/core.h >> +++ b/drivers/usb/dwc3/core.h >> @@ -33,6 +33,10 @@ >> >> #include <linux/power_supply.h> >> >> +#define XHCI_EXT_PORT_MAJOR(x) (((x) >> 24) & 0xff) >> +#define XHCI_EXT_PORT_MINOR(x) (((x) >> 16) & 0xff) >> +#define XHCI_EXT_PORT_COUNT(x) (((x) >> 8) & 0xff) > > Again, don't copy defines from xhci. > > Looks like these should be moved to the xhci-ext-caps.h header along > with struct xhci_protocol_caps. > Hi Johan, Moving the defines would be sufficient right ? Just wanted to know if there is any reason you are suggesting to move the structure as well so that I can update commit text accordingly. Regards, Krishna, >> + >> #define DWC3_MSG_MAX 500 >> >> /* Global constants */ >> @@ -1029,6 +1033,8 @@ struct dwc3_scratchpad_array { >> * @usb3_phy: pointer to USB3 PHY >> * @usb2_generic_phy: pointer to USB2 PHY >> * @usb3_generic_phy: pointer to USB3 PHY >> + * @num_usb2_ports: number of USB2 ports. >> + * @num_usb3_ports: number of USB3 ports. > > Again, please drop the full stops ('.'). > > Johan
[ Please remember to trim your replies. ] On Sun, Jul 23, 2023 at 08:29:47PM +0530, Krishna Kurapati PSSNV wrote: > On 6/27/2023 5:15 PM, Johan Hovold wrote: > > On Wed, Jun 21, 2023 at 10:06:21AM +0530, Krishna Kurapati wrote: > >> +#define XHCI_EXT_PORT_MAJOR(x) (((x) >> 24) & 0xff) > >> +#define XHCI_EXT_PORT_MINOR(x) (((x) >> 16) & 0xff) > >> +#define XHCI_EXT_PORT_COUNT(x) (((x) >> 8) & 0xff) > > > > Again, don't copy defines from xhci. > > > > Looks like these should be moved to the xhci-ext-caps.h header along > > with struct xhci_protocol_caps. > > Moving the defines would be sufficient right ? Just wanted to know if > there is any reason you are suggesting to move the structure as well so > that I can update commit text accordingly. The defines are used for parsing the members of struct xhci_protocol_caps and they belong together even if no driver has apparently ever used the structure. Johan
On 7/24/2023 9:11 PM, Johan Hovold wrote: > [ Please remember to trim your replies. ] > > On Sun, Jul 23, 2023 at 08:29:47PM +0530, Krishna Kurapati PSSNV wrote: >> On 6/27/2023 5:15 PM, Johan Hovold wrote: >>> On Wed, Jun 21, 2023 at 10:06:21AM +0530, Krishna Kurapati wrote: > >>>> +#define XHCI_EXT_PORT_MAJOR(x) (((x) >> 24) & 0xff) >>>> +#define XHCI_EXT_PORT_MINOR(x) (((x) >> 16) & 0xff) >>>> +#define XHCI_EXT_PORT_COUNT(x) (((x) >> 8) & 0xff) >>> >>> Again, don't copy defines from xhci. >>> >>> Looks like these should be moved to the xhci-ext-caps.h header along >>> with struct xhci_protocol_caps. >> >> Moving the defines would be sufficient right ? Just wanted to know if >> there is any reason you are suggesting to move the structure as well so >> that I can update commit text accordingly. > > The defines are used for parsing the members of struct > xhci_protocol_caps and they belong together even if no driver has > apparently ever used the structure. Hi Johan, Thanks for the suggestions. Will push out v10 today. Regards, Krishna,
On Fri, Jul 21, 2023, Johan Hovold wrote: > On Mon, Jul 03, 2023 at 12:26:26AM +0530, Krishna Kurapati PSSNV wrote: > > On 6/27/2023 5:39 PM, Johan Hovold wrote: > > > On Wed, Jun 21, 2023 at 10:06:23AM +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> > > >> [Krishna: Modifed logic for generic phy and rebased the patch] > > >> Signed-off-by: Krishna Kurapati <quic_kriskura@quicinc.com> > > > > > > As I already said: > > > > > > If Harsh is the primary author you need to add a From: line at > > > the beginning of the patch. > > > > > > Either way, you need his SoB as well as your Co-developed-by tag. > > > > > > All this is documented under Documentation/process/ somewhere. > > > > > > The above is missing a From line and two Co-developed-by tags at least. > > > I tried to follow the following commit: > > > > 8030cb9a5568 ("soc: qcom: aoss: remove spurious IRQF_ONESHOT flags") > > > > Let me know if that is not acceptable. > > I don't see how that commit relevant to the discussion at hand. > > Please just fix your use of Signed-off-by and Co-developed-by tags that > I've now pointed out repeatedly. > > If you can't figure it out by yourself after the feedback I've already > given you need to ask someone inside Qualcomm. You work for a huge > company that should provide resources for training it's developers in > basic process issues like this. > > > >> @@ -120,10 +120,11 @@ void dwc3_set_prtcap(struct dwc3 *dwc, u32 mode) > > >> static void __dwc3_set_mode(struct work_struct *work) > > >> { > > >> struct dwc3 *dwc = work_to_dwc(work); > > >> + u32 desired_dr_role; > > >> unsigned long flags; > > >> int ret; > > >> u32 reg; > > >> - u32 desired_dr_role; > > > > > > This is an unrelated change. Just add int i at the end. > > > > > I was trying to keep the reverse xmas order of variables. > > That's generally good, but you should not change unrelated code as part > of this patch. It's fine to leave this as is for now. > > > >> + int i; > > >> > > >> mutex_lock(&dwc->mutex); > > >> spin_lock_irqsave(&dwc->lock, flags); > > > > > >> @@ -746,23 +779,34 @@ static int dwc3_phy_setup(struct dwc3 *dwc) > > >> static int dwc3_phy_init(struct dwc3 *dwc) > > >> { > > >> int ret; > > >> + int i; > > >> + int j; > > >> > > >> usb_phy_init(dwc->usb2_phy); > > >> usb_phy_init(dwc->usb3_phy); > > >> > > >> - ret = phy_init(dwc->usb2_generic_phy); > > >> - if (ret < 0) > > >> - goto err_shutdown_usb3_phy; > > >> + for (i = 0; i < dwc->num_usb2_ports; i++) { > > >> + ret = phy_init(dwc->usb2_generic_phy[i]); > > >> + if (ret < 0) > > >> + goto err_exit_usb2_phy; > > >> + } > > >> > > >> - ret = phy_init(dwc->usb3_generic_phy); > > >> - if (ret < 0) > > >> - goto err_exit_usb2_phy; > > >> + for (i = 0; i < dwc->num_usb2_ports; i++) { > > >> + ret = phy_init(dwc->usb3_generic_phy[i]); > > >> + if (ret < 0) > > >> + goto err_exit_usb3_phy; > > >> + } > > >> > > >> return 0; > > >> > > >> +err_exit_usb3_phy: > > >> + for (j = i-1; j >= 0; j--) > > > > > > Missing spaces around - here and below. > > > > > >> + phy_exit(dwc->usb3_generic_phy[j]); > > >> + i = dwc->num_usb2_ports; > > >> err_exit_usb2_phy: > > >> - phy_exit(dwc->usb2_generic_phy); > > >> -err_shutdown_usb3_phy: > > >> + for (j = i-1; j >= 0; j--) > > >> + phy_exit(dwc->usb2_generic_phy[j]); > > >> + > > > > > > Again: > > > > > > The above is probably better implemented as a *single* loop over > > > num_usb2_ports where you enable each USB2 and USB3 PHY. On > > > errors you use the loop index to disable the already enabled > > > PHYs in reverse order below (after disabling the USB2 PHY if > > > USB3 phy init fails). > > > > > > with emphasis on "single" added. > > > > > Oh, you mean something like this ? > > > > for (loop over num_ports) { > > ret = phy_init(dwc->usb3_generic_phy[i]); > > if (ret != 0) > > goto err_exit_phy; > > > > ret = phy_init(dwc->usb2_generic_phy[i]); > > if (ret != 0) > > goto err_exit_phy; > > } > > > > err_exit_phy: > > for (j = i-1; j >= 0; j--) { > > phy_exit(dwc->usb3_generic_phy[j]); > > phy_exit(dwc->usb2_generic_phy[j]); > > } > > Yeah, something like that, but you need to disable the usb3[i] phy when > usb2[2] init fail above (and I'd also keep the order of initialising > usb2 before usb3). > > > >> usb_phy_shutdown(dwc->usb3_phy); > > >> usb_phy_shutdown(dwc->usb2_phy); > > > >> diff --git a/drivers/usb/dwc3/core.h b/drivers/usb/dwc3/core.h > > >> index 42fb17aa66fa..b2bab23ca22b 100644 > > >> --- a/drivers/usb/dwc3/core.h > > >> +++ b/drivers/usb/dwc3/core.h > > >> @@ -37,6 +37,9 @@ > > >> #define XHCI_EXT_PORT_MINOR(x) (((x) >> 16) & 0xff) > > >> #define XHCI_EXT_PORT_COUNT(x) (((x) >> 8) & 0xff) > > >> > > >> +/* Number of ports supported by a multiport controller */ > > >> +#define DWC3_MAX_PORTS 4 > > > > > > You did not answer my question about whether this was an arbitrary > > > implementation limit (i.e. just reflecting the only currently supported > > > multiport controller)? > > > > > I mentioned in commit text that it is limited to 4. Are you referring to > > state the reason why I chose the value 4 ? > > Yes, and to clarify whether this was an arbitrary limit you chose > because it was all that was needed for the hw you care about, or if it's > a more general limitation. > Note: We can support many, but we set the current limit to 4 usb3 ports and up to 15 usb2 ports. BR, Thinh
[ Digging through some old mails. ] On Tue, Aug 01, 2023 at 01:30:36AM +0000, Thinh Nguyen wrote: > On Fri, Jul 21, 2023, Johan Hovold wrote: > > On Mon, Jul 03, 2023 at 12:26:26AM +0530, Krishna Kurapati PSSNV wrote: > > > >> +/* Number of ports supported by a multiport controller */ > > > >> +#define DWC3_MAX_PORTS 4 > > > > > > > > You did not answer my question about whether this was an arbitrary > > > > implementation limit (i.e. just reflecting the only currently supported > > > > multiport controller)? > > > > > > > I mentioned in commit text that it is limited to 4. Are you referring to > > > state the reason why I chose the value 4 ? > > > > Yes, and to clarify whether this was an arbitrary limit you chose > > because it was all that was needed for the hw you care about, or if it's > > a more general limitation. > > > > Note: We can support many, but we set the current limit to 4 usb3 ports > and up to 15 usb2 ports. Thanks for clarifying. Johan
Currently the DWC3 driver supports only single port controller which requires at most two PHYs ie HS and SS PHYs. There are SoCs that has DWC3 controller with multiple ports that can operate in host mode. Some of the port supports both SS+HS and other port supports only HS mode. This change primarily refactors the Phy logic in core driver to allow multiport support with Generic Phy's. Chananges have been tested on QCOM SoC SA8295P which has 4 ports (2 are HS+SS capable and 2 are HS only capable). Changes in v9: Added IRQ support for DP/DM/SS MP Irq's of SC8280 Refactored code to read port count by accessing xhci registers Changes in v8: Reorganised code in patch-5 Fixed nitpicks in code according to comments received on v7 Fixed indentation in DT patches Added drive strength for pinctrl nodes in SA8295 DT Changes in v7: Added power event irq's for Multiport controller. Udpated commit text for patch-9 (adding DT changes for enabling first port of multiport controller on sa8540-ride). Fixed check-patch warnings for driver code. Fixed DT binding errors for changes in snps,dwc3.yaml Reabsed code on top of usb-next Changes in v6: Updated comments in code after. Updated variables names appropriately as per review comments. Updated commit text in patch-2 and added additional info as per review comments. The patch header in v5 doesn't have "PATHCH v5" notation present. Corrected it in this version. Changes in v5: Added DT support for first port of Teritiary USB controller on SA8540-Ride Added support for reading port info from XHCI Extended Params registers. Changes in RFC v4: Added DT support for SA8295p. Changes in RFC v3: Incase any PHY init fails, then clear/exit the PHYs that are already initialized. Changes in RFC v2: Changed dwc3_count_phys to return the number of PHY Phandles in the node. This will be used now in dwc3_extract_num_phys to increment num_usb2_phy and num_usb3_phy. Added new parameter "ss_idx" in dwc3_core_get_phy_ny_node and changed its structure such that the first half is for HS-PHY and second half is for SS-PHY. In dwc3_core_get_phy, for multiport controller, only if SS-PHY phandle is present, pass proper SS_IDX else pass -1. Test done on v9: Tested enum and wakeup on first port of quad port controller Tested enum and wakeup on SC7280 Chromebook Link to v8: https://lore.kernel.org/all/20230514054917.21318-1-quic_kriskura@quicinc.com/ Link to v7: https://lore.kernel.org/all/20230501143445.3851-1-quic_kriskura@quicinc.com/ Link to v6: https://lore.kernel.org/all/20230405125759.4201-1-quic_kriskura@quicinc.com/ Link to v5: https://lore.kernel.org/all/20230310163420.7582-1-quic_kriskura@quicinc.com/ Link to RFC v4: https://lore.kernel.org/all/20230115114146.12628-1-quic_kriskura@quicinc.com/ Link to RFC v3: https://lore.kernel.org/all/1654709787-23686-1-git-send-email-quic_harshq@quicinc.com/#r Link to RFC v2: https://lore.kernel.org/all/1653560029-6937-1-git-send-email-quic_harshq@quicinc.com/#r Krishna Kurapati (10): dt-bindings: usb: qcom,dwc3: Add bindings for SC8280 Multiport dt-bindings: usb: Add bindings for multiport properties on DWC3 controller usb: dwc3: core: Access XHCI address space temporarily to read port info usb: dwc3: core: Skip setting event buffers for host only controllers usb: dwc3: core: Refactor PHY logic to support Multiport Controller usb: dwc3: qcom: Add support to read IRQ's related to multiport usb: dwc3: qcom: Add multiport suspend/resume support for wrapper arm64: dts: qcom: sc8280xp: Add multiport controller node for SC8280 arm64: dts: qcom: sa8295p: Enable tertiary controller and its 4 USB ports arm64: dts: qcom: sa8540-ride: Enable first port of tertiary usb controller .../devicetree/bindings/usb/qcom,dwc3.yaml | 29 ++ .../devicetree/bindings/usb/snps,dwc3.yaml | 13 +- arch/arm64/boot/dts/qcom/sa8295p-adp.dts | 53 +++ arch/arm64/boot/dts/qcom/sa8540p-ride.dts | 22 ++ arch/arm64/boot/dts/qcom/sc8280xp.dtsi | 77 +++++ drivers/usb/dwc3/core.c | 325 ++++++++++++++---- drivers/usb/dwc3/core.h | 20 +- drivers/usb/dwc3/drd.c | 15 +- drivers/usb/dwc3/dwc3-qcom.c | 156 +++++++-- 9 files changed, 595 insertions(+), 115 deletions(-)