| Message ID | 20231204100950.28712-3-quic_kriskura@quicinc.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | None | expand |
On Mon, Dec 04, 2023 at 03:39:46PM +0530, Krishna Kurapati wrote: > For wakeup to work, driver needs to enable interrupts that depict what is > happening on th DP/DM lines. On QUSB targets, this is identified by typo: the > qusb2_phy whereas on SoCs using Femto PHY, separate {dp,dm}_hs_phy_irq's > are used instead. > > The implementation incorrectly names qusb2_phy interrupts as "hs_phy_irq". > Clean this up so that driver would be using only qusb2/(dp & dm) for wakeup > purposes. Here too you should say something about why this won't break any systems booting using an older devicetree. Specifically, the QUSB2 PHY interrupt has never been armed on any system running mainline as those bits never made it upstream. So an alternative to this could also be to just drop the QUSB2 PHY interrupt handling from this driver for now. Johan
On 12/7/2023 8:58 PM, Johan Hovold wrote: > On Mon, Dec 04, 2023 at 03:39:46PM +0530, Krishna Kurapati wrote: >> For wakeup to work, driver needs to enable interrupts that depict what is >> happening on th DP/DM lines. On QUSB targets, this is identified by > > typo: the > >> qusb2_phy whereas on SoCs using Femto PHY, separate {dp,dm}_hs_phy_irq's >> are used instead. >> >> The implementation incorrectly names qusb2_phy interrupts as "hs_phy_irq". >> Clean this up so that driver would be using only qusb2/(dp & dm) for wakeup >> purposes. > > Here too you should say something about why this won't break any systems > booting using an older devicetree. Specifically, the QUSB2 PHY interrupt > has never been armed on any system running mainline as those bits never > made it upstream. > > So an alternative to this could also be to just drop the QUSB2 PHY > interrupt handling from this driver for now. > Hi Johan, So, are you suggesting that we drop the whole patch ? I assume if the older kernels are using old DT, they would be using an old driver version too right ? Is there a case where DT is not updated but driver is ? Because if we drop this patch from series, targets with updated DT's would break. Regards, Krishna,
On Thu, Dec 07, 2023 at 09:17:32PM +0530, Krishna Kurapati PSSNV wrote: > On 12/7/2023 8:58 PM, Johan Hovold wrote: > > Here too you should say something about why this won't break any systems > > booting using an older devicetree. Specifically, the QUSB2 PHY interrupt > > has never been armed on any system running mainline as those bits never > > made it upstream. > > > > So an alternative to this could also be to just drop the QUSB2 PHY > > interrupt handling from this driver for now. > > So, are you suggesting that we drop the whole patch ? No, I meant that an alternative could be to drop the current hs_phy_irq handling from the driver. > I assume if the older kernels are using old DT, they would be using an > old driver version too right ? No, and this is part of the devicetree ABI as we discussed the other week. You should generally be able to continue booting with an older devicetree on a newer kernel (even if newer functionality may not be enabled then). > Is there a case where DT is not updated but driver is ? Because if we > drop this patch from series, targets with updated DT's would break. Actually they would not due to the fact that the QUSB2 PHY interrupt is currently never armed in the PHY (and the interrupts are looked up by name and are considered optional by the driver). But simply dropping this patch is not an option here. I'm fine with this patch as it is, but the reason we can merge it is that those interrupts are currently not actually used. Otherwise, this would break older devicetrees. But this also means, we could consider dropping the current hs_phy_irq handling altogether. Hmm. Looking at the qusb2_phy_runtime_suspend() again now I see that the interrupt is actually armed on runtime suspend, it's just that it is configured incorrectly and would wakeup immediately if someone ever exercised this path. Specifically, the bits that would set those PHY_MODE_USB_HOST_HS modes (that should never have been merged) never made it upstream so this code is just dead code currently. I said before I'll look into ripping this out, but yeah, I'm swamped with work as usual (and it has been sitting there dead for years so there's no rush). So to summarise, the QUSB2 wakeup handling is incomplete and broken, so we won't actually make things worse by renaming the interrupts. If this was working, we would need to continue supporting the old names, though. Johan
On 12/7/2023 10:04 PM, Johan Hovold wrote: > On Thu, Dec 07, 2023 at 09:17:32PM +0530, Krishna Kurapati PSSNV wrote: >> On 12/7/2023 8:58 PM, Johan Hovold wrote: > >>> Here too you should say something about why this won't break any systems >>> booting using an older devicetree. Specifically, the QUSB2 PHY interrupt >>> has never been armed on any system running mainline as those bits never >>> made it upstream. >>> >>> So an alternative to this could also be to just drop the QUSB2 PHY >>> interrupt handling from this driver for now. > > >> So, are you suggesting that we drop the whole patch ? > > No, I meant that an alternative could be to drop the current hs_phy_irq > handling from the driver. > >> I assume if the older kernels are using old DT, they would be using an >> old driver version too right ? > > No, and this is part of the devicetree ABI as we discussed the other > week. > > You should generally be able to continue booting with an older devicetree > on a newer kernel (even if newer functionality may not be enabled then). > >> Is there a case where DT is not updated but driver is ? Because if we >> drop this patch from series, targets with updated DT's would break. > > Actually they would not due to the fact that the QUSB2 PHY interrupt is > currently never armed in the PHY (and the interrupts are looked up by > name and are considered optional by the driver). > > But simply dropping this patch is not an option here. I'm fine with this > patch as it is, but the reason we can merge it is that those interrupts > are currently not actually used. Otherwise, this would break older > devicetrees. > > But this also means, we could consider dropping the current hs_phy_irq > handling altogether. > > Hmm. Looking at the qusb2_phy_runtime_suspend() again now I see that the > interrupt is actually armed on runtime suspend, it's just that it is > configured incorrectly and would wakeup immediately if someone ever > exercised this path. > > Specifically, the bits that would set those PHY_MODE_USB_HOST_HS modes > (that should never have been merged) never made it upstream so this code > is just dead code currently. I said before I'll look into ripping this > out, but yeah, I'm swamped with work as usual (and it has been sitting > there dead for years so there's no rush). > > So to summarise, the QUSB2 wakeup handling is incomplete and broken, so > we won't actually make things worse by renaming the interrupts. If this > was working, we would need to continue supporting the old names, though. > Thanks for the review. Since renaming the interrupts won't be an issue, I will keep this patch as is in that case in v3. Regards, Krishna,
diff --git a/drivers/usb/dwc3/dwc3-qcom.c b/drivers/usb/dwc3/dwc3-qcom.c index fdf6d5d3c2ad..dbd6a5b2b289 100644 --- a/drivers/usb/dwc3/dwc3-qcom.c +++ b/drivers/usb/dwc3/dwc3-qcom.c @@ -57,7 +57,7 @@ struct dwc3_acpi_pdata { u32 qscratch_base_offset; u32 qscratch_base_size; u32 dwc3_core_base_size; - int hs_phy_irq_index; + int qusb2_phy_irq_index; int dp_hs_phy_irq_index; int dm_hs_phy_irq_index; int ss_phy_irq_index; @@ -73,7 +73,7 @@ struct dwc3_qcom { int num_clocks; struct reset_control *resets; - int hs_phy_irq; + int qusb2_phy_irq; int dp_hs_phy_irq; int dm_hs_phy_irq; int ss_phy_irq; @@ -372,7 +372,7 @@ 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->hs_phy_irq); + dwc3_qcom_disable_wakeup_irq(qcom->qusb2_phy_irq); if (qcom->usb2_speed == USB_SPEED_LOW) { dwc3_qcom_disable_wakeup_irq(qcom->dm_hs_phy_irq); @@ -389,7 +389,7 @@ static void dwc3_qcom_disable_interrupts(struct dwc3_qcom *qcom) static void dwc3_qcom_enable_interrupts(struct dwc3_qcom *qcom) { - dwc3_qcom_enable_wakeup_irq(qcom->hs_phy_irq, 0); + dwc3_qcom_enable_wakeup_irq(qcom->qusb2_phy_irq, 0); /* * Configure DP/DM line interrupts based on the USB2 device attached to @@ -542,19 +542,19 @@ static int dwc3_qcom_setup_irq(struct platform_device *pdev) int irq; int ret; - irq = dwc3_qcom_get_irq(pdev, "hs_phy_irq", - pdata ? pdata->hs_phy_irq_index : -1); + irq = dwc3_qcom_get_irq(pdev, "qusb2_phy", + pdata ? pdata->qusb2_phy_irq_index : -1); if (irq > 0) { /* Keep wakeup interrupts disabled until suspend */ ret = devm_request_threaded_irq(qcom->dev, irq, NULL, qcom_dwc3_resume_irq, IRQF_ONESHOT | IRQF_NO_AUTOEN, - "qcom_dwc3 HS", qcom); + "qcom_dwc3 QUSB2", qcom); if (ret) { - dev_err(qcom->dev, "hs_phy_irq failed: %d\n", ret); + dev_err(qcom->dev, "qusb2_phy_irq failed: %d\n", ret); return ret; } - qcom->hs_phy_irq = irq; + qcom->qusb2_phy_irq = irq; } irq = dwc3_qcom_get_irq(pdev, "dp_hs_phy_irq", @@ -1058,7 +1058,7 @@ static const struct dwc3_acpi_pdata sdm845_acpi_pdata = { .qscratch_base_offset = SDM845_QSCRATCH_BASE_OFFSET, .qscratch_base_size = SDM845_QSCRATCH_SIZE, .dwc3_core_base_size = SDM845_DWC3_CORE_SIZE, - .hs_phy_irq_index = 1, + .qusb2_phy_irq_index = 1, .dp_hs_phy_irq_index = 4, .dm_hs_phy_irq_index = 3, .ss_phy_irq_index = 2 @@ -1068,7 +1068,7 @@ static const struct dwc3_acpi_pdata sdm845_acpi_urs_pdata = { .qscratch_base_offset = SDM845_QSCRATCH_BASE_OFFSET, .qscratch_base_size = SDM845_QSCRATCH_SIZE, .dwc3_core_base_size = SDM845_DWC3_CORE_SIZE, - .hs_phy_irq_index = 1, + .qusb2_phy_irq_index = 1, .dp_hs_phy_irq_index = 4, .dm_hs_phy_irq_index = 3, .ss_phy_irq_index = 2,
For wakeup to work, driver needs to enable interrupts that depict what is happening on th DP/DM lines. On QUSB targets, this is identified by qusb2_phy whereas on SoCs using Femto PHY, separate {dp,dm}_hs_phy_irq's are used instead. The implementation incorrectly names qusb2_phy interrupts as "hs_phy_irq". Clean this up so that driver would be using only qusb2/(dp & dm) for wakeup purposes. Signed-off-by: Krishna Kurapati <quic_kriskura@quicinc.com> --- drivers/usb/dwc3/dwc3-qcom.c | 22 +++++++++++----------- 1 file changed, 11 insertions(+), 11 deletions(-)