| Message ID | 20230621043628.21485-7-quic_kriskura@quicinc.com (mailing list archive) |
|---|---|
| State | Superseded |
| 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 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 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 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,
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 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: >>> 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 Hi Johan, How about the implementation in the attached patches. This way we don't need to know if we are multiport capable or not. Regards, Krishna, From c5bf1235d7d1c1b629fda7f321b33671d9705b1f Mon Sep 17 00:00:00 2001 From: Krishna Kurapati <quic_kriskura@quicinc.com> Date: Wed, 19 Jul 2023 19:29:10 +0530 Subject: [PATCH 06/11] usb: dwc3: qcom: Refactor IRQ handling in QCOM Glue driver Refactor setup_irq call to facilitate reading multiport IRQ's along with non mulitport ones. Signed-off-by: Krishna Kurapati <quic_kriskura@quicinc.com> --- drivers/usb/dwc3/dwc3-qcom.c | 196 ++++++++++++++++++++++++----------- 1 file changed, 137 insertions(+), 59 deletions(-) diff --git a/drivers/usb/dwc3/dwc3-qcom.c b/drivers/usb/dwc3/dwc3-qcom.c index 3de43df6bbe8..ab8664ced255 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[DWC3_MAX_PORTS]; + int dm_hs_phy_irq[DWC3_MAX_PORTS]; + int ss_phy_irq[DWC3_MAX_PORTS]; enum usb_device_speed usb2_speed; struct extcon_dev *edev; @@ -93,6 +93,42 @@ struct dwc3_qcom { struct icc_path *icc_path_apps; }; +struct dwc3_qcom_irq_info { + char *dt_name; + char *disp_name; + bool acpi_variant_present; +}; + +const struct dwc3_qcom_irq_info non_mp_irq_info[4] = { + { "hs_phy_irq", "qcom_dwc3 HS", true, }, + { "dp_hs_phy_irq", "qcom_dwc3 DP_HS", true, }, + { "dm_hs_phy_irq", "qcom_dwc3 DM_HS", true, }, + { "ss_phy_irq", "qcom_dwc3 SS", true, }, +}; + +const struct dwc3_qcom_irq_info mp_irq_info[3][DWC3_MAX_PORTS] = { + { + { "dp_hs_phy_1", "qcom_dwc3 DP_HS1", false }, + { "dp_hs_phy_2", "qcom_dwc3 DP_HS2", false }, + { "dp_hs_phy_3", "qcom_dwc3 DP_HS3", false }, + { "dp_hs_phy_4", "qcom_dwc3 DP_HS4", false }, + }, + + { + { "dm_hs_phy_1", "qcom_dwc3 DM_HS1", false }, + { "dm_hs_phy_2", "qcom_dwc3 DM_HS2", false }, + { "dm_hs_phy_3", "qcom_dwc3 DM_HS3", false }, + { "dm_hs_phy_4", "qcom_dwc3 DM_HS4", false }, + }, + + { + { "ss_phy_1", "qcom_dwc3 SS1", false }, + { "ss_phy_2", "qcom_dwc3 SS2", false }, + { "ss_phy_3", "qcom_dwc3 SS3", false }, + { "ss_phy_4", "qcom_dwc3 SS4", false }, + }, +}; + static inline void dwc3_qcom_setbits(void __iomem *base, u32 offset, u32 val) { u32 reg; @@ -375,16 +411,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 +437,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,72 +571,114 @@ static int dwc3_qcom_get_irq(struct platform_device *pdev, return ret; } +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(qcom->dev, irq, NULL, + qcom_dwc3_resume_irq, + IRQF_TRIGGER_HIGH | IRQF_ONESHOT, + disp_name, qcom); + 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) { struct dwc3_qcom *qcom = platform_get_drvdata(pdev); const struct dwc3_acpi_pdata *pdata = qcom->acpi_pdata; + char *disp_name; + char *dt_name; + int index; int irq; int ret; + int i; - irq = dwc3_qcom_get_irq(pdev, "hs_phy_irq", + irq = dwc3_qcom_get_irq(pdev, non_mp_irq_info[0].dt_name, pdata ? pdata->hs_phy_irq_index : -1); if (irq > 0) { - /* Keep wakeup interrupts disabled until suspend */ - 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, - "qcom_dwc3 HS", qcom); - if (ret) { - dev_err(qcom->dev, "hs_phy_irq failed: %d\n", ret); + ret = dwc3_qcom_prep_irq(qcom, non_mp_irq_info[0].dt_name, + non_mp_irq_info[0].disp_name, irq); + if (ret) return ret; - } qcom->hs_phy_irq = irq; } - irq = dwc3_qcom_get_irq(pdev, "dp_hs_phy_irq", - pdata ? pdata->dp_hs_phy_irq_index : -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, - "qcom_dwc3 DP_HS", qcom); - if (ret) { - dev_err(qcom->dev, "dp_hs_phy_irq failed: %d\n", ret); - return ret; + for (i = 0; i < DWC3_MAX_PORTS; i++) { + dt_name = mp_irq_info[0][i].dt_name; + disp_name = mp_irq_info[0][i].disp_name; + index = (mp_irq_info[0][i].acpi_variant_present && pdata) ? + pdata->dp_hs_phy_irq_index : -1; + + irq = dwc3_qcom_get_irq(pdev, dt_name, index); + if ((irq < 0) && (i == 0)) { + dt_name = non_mp_irq_info[1].dt_name; + disp_name = non_mp_irq_info[1].disp_name; + index = (non_mp_irq_info[1].acpi_variant_present && pdata) ? + pdata->dp_hs_phy_irq_index : -1; + + irq = dwc3_qcom_get_irq(pdev, dt_name, index); + } + + if (irq > 0) { + ret = dwc3_qcom_prep_irq(qcom, dt_name, disp_name, irq); + if (ret) + return ret; + qcom->dp_hs_phy_irq[i] = irq; } - qcom->dp_hs_phy_irq = irq; } - irq = dwc3_qcom_get_irq(pdev, "dm_hs_phy_irq", - pdata ? pdata->dm_hs_phy_irq_index : -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, - "qcom_dwc3 DM_HS", qcom); - if (ret) { - dev_err(qcom->dev, "dm_hs_phy_irq failed: %d\n", ret); - return ret; + for (i = 0; i < DWC3_MAX_PORTS; i++) { + dt_name = mp_irq_info[1][i].dt_name; + disp_name = mp_irq_info[1][i].disp_name; + index = (mp_irq_info[1][i].acpi_variant_present && pdata) ? + pdata->dm_hs_phy_irq_index : -1; + + irq = dwc3_qcom_get_irq(pdev, dt_name, index); + if ((irq < 0) && (i == 0)) { + dt_name = non_mp_irq_info[2].dt_name; + disp_name = non_mp_irq_info[2].disp_name; + index = (non_mp_irq_info[2].acpi_variant_present && pdata) ? + pdata->dp_hs_phy_irq_index : -1; + + irq = dwc3_qcom_get_irq(pdev, dt_name, index); + } + + if (irq > 0) { + ret = dwc3_qcom_prep_irq(qcom, dt_name, disp_name, irq); + if (ret) + return ret; + qcom->dm_hs_phy_irq[i] = irq; } - qcom->dm_hs_phy_irq = irq; } - irq = dwc3_qcom_get_irq(pdev, "ss_phy_irq", - pdata ? pdata->ss_phy_irq_index : -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, - "qcom_dwc3 SS", qcom); - if (ret) { - dev_err(qcom->dev, "ss_phy_irq failed: %d\n", ret); - return ret; + for (i = 0; i < DWC3_MAX_PORTS; i++) { + dt_name = mp_irq_info[2][i].dt_name; + disp_name = mp_irq_info[2][i].disp_name; + index = (mp_irq_info[2][i].acpi_variant_present && pdata) ? + pdata->ss_phy_irq_index : -1; + + irq = dwc3_qcom_get_irq(pdev, dt_name, index); + if ((irq < 0) && (i == 0)) { + dt_name = non_mp_irq_info[3].dt_name; + disp_name = non_mp_irq_info[3].disp_name; + index = (non_mp_irq_info[3].acpi_variant_present && pdata) ? + pdata->dp_hs_phy_irq_index : -1; + + irq = dwc3_qcom_get_irq(pdev, dt_name, index); + } + + if (irq > 0) { + ret = dwc3_qcom_prep_irq(qcom, dt_name, disp_name, irq); + if (ret) + return ret; + qcom->ss_phy_irq[i] = irq; } - qcom->ss_phy_irq = irq; } return 0;
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
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]; 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)
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(-)