Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::3:2 as permitted sender) client-ip=2620:137:e000::3:2;
Message-ID: <516aeb55-69ad-460c-9757-6ad8a203b693@quicinc.com>
Date:   Fri, 8 Dec 2023 17:44:41 +0530
MIME-Version: 1.0
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH v2 2/6] usb: dwc3: qcom: Rename hs_phy_irq to
 qusb2_phy_irq
To:     Johan Hovold <johan@kernel.org>
CC:     Thinh Nguyen <Thinh.Nguyen@synopsys.com>,
        Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
        Andy Gross <agross@kernel.org>,
        Bjorn Andersson <andersson@kernel.org>,
        Konrad Dybcio <konrad.dybcio@linaro.org>,
        Rob Herring <robh+dt@kernel.org>,
        Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org>,
        Wesley Cheng <quic_wcheng@quicinc.com>,
        Conor Dooley <conor+dt@kernel.org>,
        <cros-qcom-dts-watchers@chromium.org>, <linux-usb@vger.kernel.org>,
        <linux-kernel@vger.kernel.org>, <linux-arm-msm@vger.kernel.org>,
        <devicetree@vger.kernel.org>, <quic_ppratap@quicinc.com>,
        <quic_jackp@quicinc.com>
References: <20231204100950.28712-1-quic_kriskura@quicinc.com>
 <20231204100950.28712-3-quic_kriskura@quicinc.com>
 <ZXHkpwji_AzXMjfm@hovoldconsulting.com>
 <9d52fa8c-41d1-46a7-be89-5c1c11ca09b4@quicinc.com>
 <ZXHz-HYgVLbgFp2k@hovoldconsulting.com>
Content-Language: en-US
From:   Krishna Kurapati PSSNV <quic_kriskura@quicinc.com>
In-Reply-To: <ZXHz-HYgVLbgFp2k@hovoldconsulting.com>
Content-Type: text/plain; charset="UTF-8"; format=flowed
Content-Transfer-Encoding: 7bit
Precedence: bulk


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,