Message ID | 20230727223307.8096-1-quic_kriskura@quicinc.com |
---|---|
Headers | show |
Series | Add multiport support for DWC3 controllers | expand |
On 28/07/2023 00:32, Krishna Kurapati wrote: > Add the compatible string for SC8280 Multiport USB controller from > Qualcomm. > > There are 4 power event irq interrupts supported by this controller > (one for each port of multiport). Added all the 4 as non-optional > interrupts for SC8280XP-MP > > Also each port of multiport has one DP and oen DM IRQ. Add all DP/DM > IRQ's related to 4 ports of SC8280XP Teritiary controller. > > Also added ss phy irq for both SS Ports. > > Signed-off-by: Krishna Kurapati <quic_kriskura@quicinc.com> > --- > .../devicetree/bindings/usb/qcom,dwc3.yaml | 29 +++++++++++++++++++ > 1 file changed, 29 insertions(+) Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> Best regards, Krzysztof
On Fri, Jul 28, 2023 at 04:03:02AM +0530, Krishna Kurapati wrote: > Refactor setup_irq call to facilitate reading multiport IRQ's along > with non mulitport ones. For SA8295, there are 4-DP/4-DM and 2-SS > IRQ's. Check whether device is multiport capable or not and read all > interrupts for DP/DM/SS on each port accordingly. > > Signed-off-by: Krishna Kurapati <quic_kriskura@quicinc.com> > --- > drivers/usb/dwc3/dwc3-qcom.c | 277 ++++++++++++++++++++++++----------- > 1 file changed, 190 insertions(+), 87 deletions(-) > > diff --git a/drivers/usb/dwc3/dwc3-qcom.c b/drivers/usb/dwc3/dwc3-qcom.c > index 3de43df6bbe8..ad89ded116d3 100644 > --- a/drivers/usb/dwc3/dwc3-qcom.c > +++ b/drivers/usb/dwc3/dwc3-qcom.c > @@ -64,33 +64,61 @@ struct dwc3_acpi_pdata { > bool is_urs; > }; > > +struct dwc3_qcom_of_match_data { > + u8 num_ports; > +}; > + > struct dwc3_qcom { > - struct device *dev; > - void __iomem *qscratch_base; > - struct platform_device *dwc3; > - struct platform_device *urs_usb; > - struct clk **clks; > - int num_clocks; > - struct reset_control *resets; > - > - int hs_phy_irq; > - int dp_hs_phy_irq; > - int dm_hs_phy_irq; > - int ss_phy_irq; > - enum usb_device_speed usb2_speed; > - > - struct extcon_dev *edev; > - struct extcon_dev *host_edev; > - struct notifier_block vbus_nb; > - struct notifier_block host_nb; > + struct device *dev; > + void __iomem *qscratch_base; > + struct platform_device *dwc3; > + struct platform_device *urs_usb; > + struct clk **clks; > + int num_clocks; > + struct reset_control *resets; > + > + int hs_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; > + struct extcon_dev *host_edev; > + struct notifier_block vbus_nb; > + struct notifier_block host_nb; > + > + const struct dwc3_acpi_pdata *acpi_pdata; > + > + enum usb_dr_mode mode; > + bool is_suspended; > + bool pm_suspended; > + struct icc_path *icc_path_ddr; > + struct icc_path *icc_path_apps; > + const struct dwc3_qcom_of_match_data *data; Please don't adjust indentation of unrelated code, it makes it hard to see what actually changed. > +}; > + > +static const struct dwc3_qcom_of_match_data qcom_dwc3 = { > + .num_ports = 1, > +}; > > - const struct dwc3_acpi_pdata *acpi_pdata; > +static const struct dwc3_qcom_of_match_data sx8280xp_qcom_dwc3 = { > + .num_ports = 4, > +}; > > - enum usb_dr_mode mode; > - bool is_suspended; > - bool pm_suspended; > - struct icc_path *icc_path_ddr; > - struct icc_path *icc_path_apps; > +/* > + * Driver needs to read HS/DP_HS/DM_HS/SS IRQ's. Currently, for > + * SA8295 which supports mutliport, thre are 4 DP/ 4 DM/ 2 SS IRQ's > + * and 1 HS IRQ present. So avoid trying to read HS_PHY_IRQ for 4 > + * ports of SA8295. > + */ The last part here is relevant information, but it doesn't seem to relate to this define. Also, does all platforms have this configuration of interrupts? > +#define MAX_PHY_IRQ 4 > + > +enum dwc3_qcom_phy_irq_identifier { > + HS_PHY_IRQ = 0, > + DP_HS_PHY_IRQ, > + DM_HS_PHY_IRQ, > + SS_PHY_IRQ, > }; This enum is unused. > [..] > +static int dwc3_get_acpi_index(const struct dwc3_acpi_pdata *pdata, int irq_index) > +{ > + int acpi_index = -1; > + > + if (!pdata) > + return -1; > + > + if (irq_index == DP_HS_PHY_IRQ) > + acpi_index = pdata->dp_hs_phy_irq_index; > + else if (irq_index == DM_HS_PHY_IRQ) > + acpi_index = pdata->dm_hs_phy_irq_index; > + else if (irq_index == SS_PHY_IRQ) > + acpi_index = pdata->ss_phy_irq_index; It looks favourable to put these in an array, instead of having to pull them out of 4 different variables conditionally. > + > + return acpi_index; > +} > + > +static int dwc3_get_port_irq(struct platform_device *pdev, u8 port_index) > +{ > + struct dwc3_qcom *qcom = platform_get_drvdata(pdev); > + bool is_mp_supported = (qcom->data->num_ports > 1) ? true : false; > + const struct dwc3_acpi_pdata *pdata = qcom->acpi_pdata; > + char *disp_name; > + int acpi_index; > + char *dt_name; > + int ret; > + int irq; > + int i; > + > + /* > + * We need to read only DP/DM/SS IRQ's here. > + * So loop over from 1->3 and accordingly modify respective phy_irq[]. > + */ > + for (i = 1; i < MAX_PHY_IRQ; i++) { > + > + if (!is_mp_supported && (port_index == 0)) { > + if (i == DP_HS_PHY_IRQ) { > + dt_name = devm_kasprintf(&pdev->dev, GFP_KERNEL, > + "dp_hs_phy_irq"); > + disp_name = devm_kasprintf(&pdev->dev, GFP_KERNEL, > + "qcom_dwc3 DP_HS"); > + } else if (i == DM_HS_PHY_IRQ) { > + dt_name = devm_kasprintf(&pdev->dev, GFP_KERNEL, > + "dm_hs_phy_irq"); > + disp_name = devm_kasprintf(&pdev->dev, GFP_KERNEL, > + "qcom_dwc3 DM_HS"); > + } else if (i == SS_PHY_IRQ) { > + dt_name = devm_kasprintf(&pdev->dev, GFP_KERNEL, > + "ss_phy_irq"); > + disp_name = devm_kasprintf(&pdev->dev, GFP_KERNEL, > + "qcom_dwc3 SS"); > + } > + } else { > + if (i == DP_HS_PHY_IRQ) { > + dt_name = devm_kasprintf(&pdev->dev, GFP_KERNEL, > + "dp_hs_phy_%d", port_index + 1); > + disp_name = devm_kasprintf(&pdev->dev, GFP_KERNEL, > + "qcom_dwc3 DP_HS%d", port_index + 1); > + } else if (i == DM_HS_PHY_IRQ) { > + dt_name = devm_kasprintf(&pdev->dev, GFP_KERNEL, > + "dm_hs_phy_%d", port_index + 1); > + disp_name = devm_kasprintf(&pdev->dev, GFP_KERNEL, > + "qcom_dwc3 DM_HS%d", port_index + 1); > + } else if (i == SS_PHY_IRQ) { > + dt_name = devm_kasprintf(&pdev->dev, GFP_KERNEL, > + "ss_phy_%d", port_index + 1); > + disp_name = devm_kasprintf(&pdev->dev, GFP_KERNEL, > + "qcom_dwc3 SS%d", port_index + 1); > + } There is too much repetition in this for my liking. > } > - 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; > + if (!dt_name || !disp_name) > + return -ENOMEM; > + > + acpi_index = !is_mp_supported ? dwc3_get_acpi_index(pdata, i) : -1; > + > + irq = dwc3_qcom_get_irq(pdev, dt_name, acpi_index); > + if (irq > 0) { > + ret = dwc3_qcom_prep_irq(qcom, dt_name, disp_name, irq); > + if (ret) > + return ret; > + > + if (i == DP_HS_PHY_IRQ) > + qcom->dp_hs_phy_irq[port_index] = irq; > + else if (i == DM_HS_PHY_IRQ) > + qcom->dm_hs_phy_irq[port_index] = irq; > + else if (i == SS_PHY_IRQ) > + qcom->ss_phy_irq[port_index] = 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); > + return 0; > +} > + > +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; > + int irq; > + int ret; > + int i; > + > + irq = dwc3_qcom_get_irq(pdev, "hs_phy_irq", > + pdata ? pdata->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); > + ret = dwc3_qcom_prep_irq(qcom, "hs_phy_irq", "qcom_dwc3 HS",irq); > + if (ret) It would be nice to have this refactored out in a separate commit. > return ret; > - } > - qcom->dm_hs_phy_irq = irq; > + qcom->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); > + for (i = 0; i < qcom->data->num_ports; i++) { > + ret = dwc3_get_port_irq(pdev, i); > + if (ret) > return ret; > - } > - qcom->ss_phy_irq = irq; > } > > return 0; > @@ -811,6 +905,8 @@ static int dwc3_qcom_probe(struct platform_device *pdev) > platform_set_drvdata(pdev, qcom); > qcom->dev = &pdev->dev; > > + qcom->data = of_device_get_match_data(qcom->dev); > + > if (has_acpi_companion(dev)) { > qcom->acpi_pdata = acpi_device_get_match_data(dev); > if (!qcom->acpi_pdata) { > @@ -1023,8 +1119,15 @@ static const struct dev_pm_ops dwc3_qcom_dev_pm_ops = { > }; > > static const struct of_device_id dwc3_qcom_of_match[] = { > - { .compatible = "qcom,dwc3" }, > - { } > + { > + .compatible = "qcom,dwc3", > + .data = &qcom_dwc3, > + }, > + { > + .compatible = "qcom,sc8280xp-dwc3-mp", > + .data = &sx8280xp_qcom_dwc3, > + }, I would prefer that we don't add a separate compatible, but rather just try to parse the interrupts for multiport and fall back to single port. If/when we figure out how to peak into the dwc3 core, we could potentially introduce a check to aid the developer. Regards, Bjorn > + { }, > }; > MODULE_DEVICE_TABLE(of, dwc3_qcom_of_match); > > -- > 2.40.0 >
On 8/11/2023 10:35 PM, Konrad Dybcio wrote: >>>>>> + >>>>>> +static int dwc3_get_port_irq(struct platform_device *pdev, u8 port_index) >>>>>> +{ >>>>>> + struct dwc3_qcom *qcom = platform_get_drvdata(pdev); >>>>>> + bool is_mp_supported = (qcom->data->num_ports > 1) ? true : false; >>>>>> + const struct dwc3_acpi_pdata *pdata = qcom->acpi_pdata; >>>>>> + char *disp_name; >>>>>> + int acpi_index; >>>>>> + char *dt_name; >>>>>> + int ret; >>>>>> + int irq; >>>>>> + int i; >>>>>> + >>>>>> + /* >>>>>> + * We need to read only DP/DM/SS IRQ's here. >>>>>> + * So loop over from 1->3 and accordingly modify respective phy_irq[]. >>>>>> + */ >>>>>> + for (i = 1; i < MAX_PHY_IRQ; i++) { >>>>>> + >>>>>> + if (!is_mp_supported && (port_index == 0)) { >>>>>> + if (i == DP_HS_PHY_IRQ) { >>>>>> + dt_name = devm_kasprintf(&pdev->dev, GFP_KERNEL, >>>>>> + "dp_hs_phy_irq"); >>>>>> + disp_name = devm_kasprintf(&pdev->dev, GFP_KERNEL, >>>>>> + "qcom_dwc3 DP_HS"); >>>>>> + } else if (i == DM_HS_PHY_IRQ) { >>>>>> + dt_name = devm_kasprintf(&pdev->dev, GFP_KERNEL, >>>>>> + "dm_hs_phy_irq"); >>>>>> + disp_name = devm_kasprintf(&pdev->dev, GFP_KERNEL, >>>>>> + "qcom_dwc3 DM_HS"); >>>>>> + } else if (i == SS_PHY_IRQ) { >>>>>> + dt_name = devm_kasprintf(&pdev->dev, GFP_KERNEL, >>>>>> + "ss_phy_irq"); >>>>>> + disp_name = devm_kasprintf(&pdev->dev, GFP_KERNEL, >>>>>> + "qcom_dwc3 SS"); >>>> Bjorn, Konrad, >>>> >>>> If we are to remove this repetitive loops, we might need to make a 2D array for all of Dp/Dm/Ss interrutps and make a global array of names to be used for irq lookup and use them to reduce the if-else-if stuff here. If that is fine, I can make those changes, else I would like to stick to this approach for now because if we don't add the global array of names, prepping them seperately for dp/dm/ss would again lead us to making if-else loops like above. >>>> >>>> Please let me know your thoughts on this. >>> Can we not just reuse the associated interrupt-names from the devicetree >>> if present? >>> >> Hi Konrad, >> >> Thanks for the comments but one more confirmation. >> We can read the interrupts from DT but I believe the compatible would still need to stay. We need the num_ports information not just for registering interrupts but for modifying the pwr_event_irq registers during suspend/resume. If we rely on the interrupts to find the number of ports, the user is free to remove any IRQ and we might end up in a situation where glue and core are not having same view of how many number of ports present. So I believe its best to keep the compatible and get num_ports info from there and rely on reading interrupt-names to get interrupts cleanly. Can you let me know your view on the same. > So is "is it okay to add SoC-specific compatibles and add the port number in > match data" what you're asking? > > If so, that doesn't seem right. > > The user should not "feel free to remove any IRQ", modifying the devicetree to > depict a subset of the hardware is not something we want to support. The driver > has to work with the "full" description in accordance with the bindings. > Hi Konrad, Johan, Thanks for the suggestions / review comments and taking the time out for reviewing them. Pushed v11 implementing the above suggestion of identifying port_count by reading through the interrupt-names. Please help let know your review on the same. Regards, Krishna,