| Message ID | 20240307062052.2319851-8-quic_kriskura@quicinc.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Add multiport support for DWC3 controllers | expand |
On 3/25/2024 6:18 PM, Johan Hovold wrote: > On Thu, Mar 07, 2024 at 11:50:50AM +0530, Krishna Kurapati wrote: >> On multiport supported controllers, each port has its own DP/DM >> and SS (if super speed capable) interrupts. As per the bindings, >> their interrupt names differ from standard ones having "_x" added >> as suffix (x indicates port number). Identify from the interrupt >> names whether the controller is a multiport controller or not. >> Refactor dwc3_qcom_setup_irq() call to parse multiport interrupts >> along with non-multiport ones accordingly.. >> >> Signed-off-by: Krishna Kurapati <quic_kriskura@quicinc.com> >> --- >> drivers/usb/dwc3/dwc3-qcom.c | 146 +++++++++++++++++++++++++++-------- >> 1 file changed, 112 insertions(+), 34 deletions(-) > > This is much better. Just a couple of nits below. > >> +static int dwc3_qcom_find_num_ports(struct platform_device *pdev) >> +{ >> + const char *irq_name; >> + int port_index; >> + int irq; >> + >> + irq = platform_get_irq_byname_optional(pdev, "qusb2_phy"); >> + if (irq > 0) >> + return 1; >> + >> + irq = platform_get_irq_byname_optional(pdev, "dp_hs_phy_irq"); >> + if (irq > 0) >> + return 1; >> + >> + for (port_index = 0; port_index < DWC3_MAX_PORTS; port_index++) { >> + irq_name = devm_kasprintf(&pdev->dev, GFP_KERNEL, "dp_hs_phy_%d", port_index + 1); > > Please use a stack allocated buffer for these strings as we don't need > them any more after this function returns. > Sure. Will replace this with "char irq_name[14]". >> + if (!irq_name) >> + return -ENOMEM; >> + >> + irq = platform_get_irq_byname_optional(pdev, irq_name); >> + if (irq <= 0) >> + return port_index; >> + } >> + >> + return port_index; > > I think explicitly returning DWC3_MAX_PORTS here would be more readable. > Will make this change. >> +} >> + >> +static int dwc3_qcom_setup_irq(struct platform_device *pdev) >> +{ >> + struct dwc3_qcom *qcom = platform_get_drvdata(pdev); >> + bool is_multiport; >> + int ret; >> + int i; >> + >> + qcom->num_ports = dwc3_qcom_find_num_ports(pdev); >> + if (qcom->num_ports < 0) >> + return -ENOMEM; > > Just return 'ret' directly. Sure, will init ret to -ENOMEM and return ret here. > >> + >> + is_multiport = (qcom->num_ports > 1) ? true : false; > > And no need for the ternary operator: ACK. > > is_multiport = (qcom->num_ports > 1); > >> + >> + for (i = 0; i < qcom->num_ports; i++) { >> + ret = dwc3_qcom_setup_port_irq(pdev, i, is_multiport); >> if (ret) >> return ret; >> - qcom->ss_phy_irq = irq; >> } >> >> return 0; > > With that fixed: > > Reviewed-by: Johan Hovold <johan+linaro@kernel.org> Thanks for the review Regards, Krishna,
On Mon, Mar 25, 2024 at 06:45:07PM +0530, Krishna Kurapati PSSNV wrote: > >> +static int dwc3_qcom_setup_irq(struct platform_device *pdev) > >> +{ > >> + struct dwc3_qcom *qcom = platform_get_drvdata(pdev); > >> + bool is_multiport; > >> + int ret; > >> + int i; > >> + > >> + qcom->num_ports = dwc3_qcom_find_num_ports(pdev); > >> + if (qcom->num_ports < 0) > >> + return -ENOMEM; > > > > Just return 'ret' directly. > > Sure, will init ret to -ENOMEM and return ret here. I meant that you should return whatever error dwc3_qcom_find_num_ports() returns, so perhaps something like: ret = dwc3_qcom_find_num_ports(pdev); if (ret < 0) return ret; qcom->num_ports = ret; It looks like dwc3_qcom_find_num_ports() can also return 0 (e.g. on malformed DT), which also needs to be handled somehow. I missed that earlier. Johan
On 3/25/2024 6:53 PM, Johan Hovold wrote: > On Mon, Mar 25, 2024 at 06:45:07PM +0530, Krishna Kurapati PSSNV wrote: >>>> +static int dwc3_qcom_setup_irq(struct platform_device *pdev) >>>> +{ >>>> + struct dwc3_qcom *qcom = platform_get_drvdata(pdev); >>>> + bool is_multiport; >>>> + int ret; >>>> + int i; >>>> + >>>> + qcom->num_ports = dwc3_qcom_find_num_ports(pdev); >>>> + if (qcom->num_ports < 0) >>>> + return -ENOMEM; >>> >>> Just return 'ret' directly. >> >> Sure, will init ret to -ENOMEM and return ret here. > Hi Johan, > I meant that you should return whatever error dwc3_qcom_find_num_ports() > returns, so perhaps something like: > Got it. Any error that might come up in interrupt reading as well. > > ret = dwc3_qcom_find_num_ports(pdev); > if (ret < 0) > return ret; > > qcom->num_ports = ret; > > It looks like dwc3_qcom_find_num_ports() can also return 0 (e.g. on > malformed DT), which also needs to be handled somehow. I missed that > earlier. > From what I remember, Konrad mentioned that we might not need to support incomplete or improper DT [1]. Also since this is close to getting merged, can we take up any changes for Malformed DT handling later given that only one or two devices are present and less likely to be given a malformed DT. [1]: https://lore.kernel.org/all/c8d77d4f-6696-4dc9-8030-daf1d10b114b@linaro.org/ Regards, Krishna,
On Mon, Mar 25, 2024 at 10:59:49PM +0530, Krishna Kurapati PSSNV wrote: > On 3/25/2024 6:53 PM, Johan Hovold wrote: > > ret = dwc3_qcom_find_num_ports(pdev); > > if (ret < 0) > > return ret; > > > > qcom->num_ports = ret; > > > > It looks like dwc3_qcom_find_num_ports() can also return 0 (e.g. on > > malformed DT), which also needs to be handled somehow. I missed that > > earlier. > > From what I remember, Konrad mentioned that we might not need to > support incomplete or improper DT [1]. You still need to make sure that the driver doesn't misbehave on malformed input. > Also since this is close to getting merged, can we take up any changes > for Malformed DT handling later given that only one or two devices are > present and less likely to be given a malformed DT. No, and I'm a bit disappointed that you're still trying to take short cuts to getting these patches merged after I've told you repeatedly that that is not how upstream works. Fortunately, this is easily fixed and would both simplify the code further and reduce the risk of breaking backwards compatibility. Just change the logic in dwc3_qcom_find_num_ports() so that it returns 1 if "dp_hs_phy_1" is missing, and otherwise you determine the number of ports by iterating from 2 to DWC3_MAX_PORTS - 1. Johan
On 3/26/2024 1:58 PM, Johan Hovold wrote: > On Tue, Mar 26, 2024 at 01:41:52PM +0530, Krishna Kurapati PSSNV wrote: >> On 3/26/2024 1:15 AM, Johan Hovold wrote: >> >>> Just change the logic in dwc3_qcom_find_num_ports() so that it returns 1 >>> if "dp_hs_phy_1" is missing, and otherwise you determine the number of >>> ports by iterating from 2 to DWC3_MAX_PORTS - 1. > >> I made this change and it works. Removed any return value check for the >> find_num_ports call as it can return only 1/2/3/4 now. >> >> --- >> irq = platform_get_irq_byname_optional(pdev, "qusb2_phy"); >> if (irq > 0) >> return 1; >> >> irq = platform_get_irq_byname_optional(pdev, "dp_hs_phy_irq"); >> if (irq > 0) >> return 1; > > As I mentioned above, these two lookups are no longer needed and should > be removed. > >> irq = platform_get_irq_byname_optional(pdev, "dp_hs_phy_1"); >> if (irq <= 0) >> return 1; > > Just assume it's a single port controller unless "dp_hs_phy_1" is > present. > >> for (port_index = 1; port_index < DWC3_MAX_PORTS - 1; >> port_index++) { > > I think this would be more readable if you use port (num) as iterator > (2..DWC3_MAX_PORTS) as you're returning a number of ports. > >> sprintf(irq_name, "dp_hs_phy_%d", port_index + 1); > > Then this would use just "port"; > >> >> irq = platform_get_irq_byname_optional(pdev, irq_name); >> if (irq <= 0) >> return port_index; > > And return "port - 1" here. > >> } >> >> return DWC3_MAX_PORTS; Ok,Thanks. With the above fixed, can I put your RB tag given on [1] or will you be providing another one. Asking because when you gave that RB, this malformed DT thing wasn't there. So wanted to check with you before pushing the code. [1]: https://lore.kernel.org/all/ZgFyukBXIIwZo7v-@hovoldconsulting.com/ Regards, Krishna,
On Tue, Mar 26, 2024 at 03:36:27PM +0530, Krishna Kurapati PSSNV wrote: > On 3/26/2024 1:58 PM, Johan Hovold wrote: > > On Tue, Mar 26, 2024 at 01:41:52PM +0530, Krishna Kurapati PSSNV wrote: > >> On 3/26/2024 1:15 AM, Johan Hovold wrote: > >> > >>> Just change the logic in dwc3_qcom_find_num_ports() so that it returns 1 > >>> if "dp_hs_phy_1" is missing, and otherwise you determine the number of > >>> ports by iterating from 2 to DWC3_MAX_PORTS - 1. > With the above fixed, can I put your RB tag given on [1] or will you be > providing another one. Asking because when you gave that RB, this > malformed DT thing wasn't there. So wanted to check with you before > pushing the code. Thanks for checking. I'm fine with you adding the tag after fixing the above now that we've discussed it. Johan
diff --git a/drivers/usb/dwc3/dwc3-qcom.c b/drivers/usb/dwc3/dwc3-qcom.c index cae5dab8fcfc..dcb3b3ded205 100644 --- a/drivers/usb/dwc3/dwc3-qcom.c +++ b/drivers/usb/dwc3/dwc3-qcom.c @@ -52,6 +52,13 @@ #define APPS_USB_AVG_BW 0 #define APPS_USB_PEAK_BW MBps_to_icc(40) +struct dwc3_qcom_port { + int qusb2_phy_irq; + int dp_hs_phy_irq; + int dm_hs_phy_irq; + int ss_phy_irq; +}; + struct dwc3_qcom { struct device *dev; void __iomem *qscratch_base; @@ -59,11 +66,8 @@ struct dwc3_qcom { struct clk **clks; int num_clocks; struct reset_control *resets; - - int qusb2_phy_irq; - int dp_hs_phy_irq; - int dm_hs_phy_irq; - int ss_phy_irq; + struct dwc3_qcom_port ports[DWC3_MAX_PORTS]; + u8 num_ports; enum usb_device_speed usb2_speed; struct extcon_dev *edev; @@ -354,24 +358,24 @@ static void dwc3_qcom_disable_wakeup_irq(int irq) static void dwc3_qcom_disable_interrupts(struct dwc3_qcom *qcom) { - dwc3_qcom_disable_wakeup_irq(qcom->qusb2_phy_irq); + dwc3_qcom_disable_wakeup_irq(qcom->ports[0].qusb2_phy_irq); if (qcom->usb2_speed == USB_SPEED_LOW) { - dwc3_qcom_disable_wakeup_irq(qcom->dm_hs_phy_irq); + dwc3_qcom_disable_wakeup_irq(qcom->ports[0].dm_hs_phy_irq); } else if ((qcom->usb2_speed == USB_SPEED_HIGH) || (qcom->usb2_speed == USB_SPEED_FULL)) { - dwc3_qcom_disable_wakeup_irq(qcom->dp_hs_phy_irq); + dwc3_qcom_disable_wakeup_irq(qcom->ports[0].dp_hs_phy_irq); } else { - dwc3_qcom_disable_wakeup_irq(qcom->dp_hs_phy_irq); - dwc3_qcom_disable_wakeup_irq(qcom->dm_hs_phy_irq); + dwc3_qcom_disable_wakeup_irq(qcom->ports[0].dp_hs_phy_irq); + dwc3_qcom_disable_wakeup_irq(qcom->ports[0].dm_hs_phy_irq); } - dwc3_qcom_disable_wakeup_irq(qcom->ss_phy_irq); + dwc3_qcom_disable_wakeup_irq(qcom->ports[0].ss_phy_irq); } static void dwc3_qcom_enable_interrupts(struct dwc3_qcom *qcom) { - dwc3_qcom_enable_wakeup_irq(qcom->qusb2_phy_irq, 0); + dwc3_qcom_enable_wakeup_irq(qcom->ports[0].qusb2_phy_irq, 0); /* * Configure DP/DM line interrupts based on the USB2 device attached to @@ -383,20 +387,20 @@ static void dwc3_qcom_enable_interrupts(struct dwc3_qcom *qcom) */ if (qcom->usb2_speed == USB_SPEED_LOW) { - dwc3_qcom_enable_wakeup_irq(qcom->dm_hs_phy_irq, - IRQ_TYPE_EDGE_FALLING); + dwc3_qcom_enable_wakeup_irq(qcom->ports[0].dm_hs_phy_irq, + IRQ_TYPE_EDGE_FALLING); } else if ((qcom->usb2_speed == USB_SPEED_HIGH) || (qcom->usb2_speed == USB_SPEED_FULL)) { - dwc3_qcom_enable_wakeup_irq(qcom->dp_hs_phy_irq, - IRQ_TYPE_EDGE_FALLING); + dwc3_qcom_enable_wakeup_irq(qcom->ports[0].dp_hs_phy_irq, + IRQ_TYPE_EDGE_FALLING); } else { - dwc3_qcom_enable_wakeup_irq(qcom->dp_hs_phy_irq, - IRQ_TYPE_EDGE_RISING); - dwc3_qcom_enable_wakeup_irq(qcom->dm_hs_phy_irq, - IRQ_TYPE_EDGE_RISING); + dwc3_qcom_enable_wakeup_irq(qcom->ports[0].dp_hs_phy_irq, + IRQ_TYPE_EDGE_RISING); + dwc3_qcom_enable_wakeup_irq(qcom->ports[0].dm_hs_phy_irq, + IRQ_TYPE_EDGE_RISING); } - dwc3_qcom_enable_wakeup_irq(qcom->ss_phy_irq, 0); + dwc3_qcom_enable_wakeup_irq(qcom->ports[0].ss_phy_irq, 0); } static int dwc3_qcom_suspend(struct dwc3_qcom *qcom, bool wakeup) @@ -517,42 +521,116 @@ static int dwc3_qcom_request_irq(struct dwc3_qcom *qcom, int irq, return ret; } -static int dwc3_qcom_setup_irq(struct platform_device *pdev) +static int dwc3_qcom_setup_port_irq(struct platform_device *pdev, int port_num, bool is_multiport) { struct dwc3_qcom *qcom = platform_get_drvdata(pdev); + const char *irq_name; int irq; int ret; - irq = platform_get_irq_byname_optional(pdev, "qusb2_phy"); + if (is_multiport) + irq_name = devm_kasprintf(&pdev->dev, GFP_KERNEL, "dp_hs_phy_%d", port_num + 1); + else + irq_name = devm_kasprintf(&pdev->dev, GFP_KERNEL, "dp_hs_phy_irq"); + if (!irq_name) + return -ENOMEM; + + irq = platform_get_irq_byname_optional(pdev, irq_name); if (irq > 0) { - ret = dwc3_qcom_request_irq(qcom, irq, "qusb2_phy"); + ret = dwc3_qcom_request_irq(qcom, irq, irq_name); if (ret) return ret; - qcom->qusb2_phy_irq = irq; + qcom->ports[port_num].dp_hs_phy_irq = irq; } - irq = platform_get_irq_byname_optional(pdev, "dp_hs_phy_irq"); + if (is_multiport) + irq_name = devm_kasprintf(&pdev->dev, GFP_KERNEL, "dm_hs_phy_%d", port_num + 1); + else + irq_name = devm_kasprintf(&pdev->dev, GFP_KERNEL, "dm_hs_phy_irq"); + if (!irq_name) + return -ENOMEM; + + irq = platform_get_irq_byname_optional(pdev, irq_name); if (irq > 0) { - ret = dwc3_qcom_request_irq(qcom, irq, "dp_hs_phy_irq"); + ret = dwc3_qcom_request_irq(qcom, irq, irq_name); if (ret) return ret; - qcom->dp_hs_phy_irq = irq; + qcom->ports[port_num].dm_hs_phy_irq = irq; } - irq = platform_get_irq_byname_optional(pdev, "dm_hs_phy_irq"); + if (is_multiport) + irq_name = devm_kasprintf(&pdev->dev, GFP_KERNEL, "ss_phy_%d", port_num + 1); + else + irq_name = devm_kasprintf(&pdev->dev, GFP_KERNEL, "ss_phy_irq"); + if (!irq_name) + return -ENOMEM; + + irq = platform_get_irq_byname_optional(pdev, irq_name); if (irq > 0) { - ret = dwc3_qcom_request_irq(qcom, irq, "dm_hs_phy_irq"); + ret = dwc3_qcom_request_irq(qcom, irq, irq_name); if (ret) return ret; - qcom->dm_hs_phy_irq = irq; + qcom->ports[port_num].ss_phy_irq = irq; } - irq = platform_get_irq_byname_optional(pdev, "ss_phy_irq"); + if (is_multiport) + return 0; + + irq = platform_get_irq_byname_optional(pdev, "qusb2_phy"); if (irq > 0) { - ret = dwc3_qcom_request_irq(qcom, irq, "ss_phy_irq"); + ret = dwc3_qcom_request_irq(qcom, irq, "qusb2_phy"); + if (ret) + return ret; + qcom->ports[port_num].qusb2_phy_irq = irq; + } + + return 0; +} + +static int dwc3_qcom_find_num_ports(struct platform_device *pdev) +{ + const char *irq_name; + int port_index; + int irq; + + irq = platform_get_irq_byname_optional(pdev, "qusb2_phy"); + if (irq > 0) + return 1; + + irq = platform_get_irq_byname_optional(pdev, "dp_hs_phy_irq"); + if (irq > 0) + return 1; + + for (port_index = 0; port_index < DWC3_MAX_PORTS; port_index++) { + irq_name = devm_kasprintf(&pdev->dev, GFP_KERNEL, "dp_hs_phy_%d", port_index + 1); + if (!irq_name) + return -ENOMEM; + + irq = platform_get_irq_byname_optional(pdev, irq_name); + if (irq <= 0) + return port_index; + } + + return port_index; +} + +static int dwc3_qcom_setup_irq(struct platform_device *pdev) +{ + struct dwc3_qcom *qcom = platform_get_drvdata(pdev); + bool is_multiport; + int ret; + int i; + + qcom->num_ports = dwc3_qcom_find_num_ports(pdev); + if (qcom->num_ports < 0) + return -ENOMEM; + + is_multiport = (qcom->num_ports > 1) ? true : false; + + for (i = 0; i < qcom->num_ports; i++) { + ret = dwc3_qcom_setup_port_irq(pdev, i, is_multiport); if (ret) return ret; - qcom->ss_phy_irq = irq; } return 0;
On multiport supported controllers, each port has its own DP/DM and SS (if super speed capable) interrupts. As per the bindings, their interrupt names differ from standard ones having "_x" added as suffix (x indicates port number). Identify from the interrupt names whether the controller is a multiport controller or not. Refactor dwc3_qcom_setup_irq() call to parse multiport interrupts along with non-multiport ones accordingly.. Signed-off-by: Krishna Kurapati <quic_kriskura@quicinc.com> --- drivers/usb/dwc3/dwc3-qcom.c | 146 +++++++++++++++++++++++++++-------- 1 file changed, 112 insertions(+), 34 deletions(-)