Re: [PATCH v2 2/6] usb: dwc3: qcom: Rename hs_phy_irq to qusb2_phy_irq

[Krishna Kurapati PSSNV <quic_kriskura@quicinc.com>]

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,