linux-kernel - Re: [PATCH 08/13] phy: qcom-qmp-pcie: drop power-down delay config

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <Y0ZhC0apqGoAyeII@hovoldconsulting.com>
Date:   Wed, 12 Oct 2022 08:39:07 +0200
From:   Johan Hovold <johan@...nel.org>
To:     Dmitry Baryshkov <dmitry.baryshkov@...aro.org>
Cc:     Johan Hovold <johan+linaro@...nel.org>,
        Vinod Koul <vkoul@...nel.org>, Andy Gross <agross@...nel.org>,
        Bjorn Andersson <andersson@...nel.org>,
        Konrad Dybcio <konrad.dybcio@...ainline.org>,
        Kishon Vijay Abraham I <kishon@...com>,
        linux-arm-msm@...r.kernel.org, linux-phy@...ts.infradead.org,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH 08/13] phy: qcom-qmp-pcie: drop power-down delay config

On Tue, Oct 11, 2022 at 05:41:28PM +0300, Dmitry Baryshkov wrote:
> On 11/10/2022 17:17, Johan Hovold wrote:

> > Yeah, I noticed that ipq8074 was the first to abuse the prwdn_delay
> > and possibly because of it starting the PHY already in its PCS table
> > (which it never should have).
> > 
> > I'm talking about the intent of pwrdn_delay which was to add a delay
> > after powering-on the phy and before starting it.
> > 
> > The vendor driver has a 1 ms delay after starting the PHY and before it
> > starts polling as the PHY on newer SoC tend to take > 1 ms before they
> > are ready.
> > 
> > So, I still claim that that delay in the vendor driver is a different
> > one entirely.
> > 
> >> Thus, I'd say, the PCIe delay should be moved after the registers
> >> programming.
> > 
> > No, not necessarily. Again, that's an optimisation in the vendor driver
> > to avoid polling so many times. Since I can say for sure that there are
> > no PHY that start in less than 1 ms, I wouldn't add it unconditionally.
> 
> I don't think it's an optimization. For me it looks like some kind of 
> stabilization delay before touching pipe clocks.

I meant that it's effectively an optimisation as the driver still works
without that unconditional delay after starting the PHY and before
polling its status. And the mainline poll-loop takes care of waiting
also for that 1 ms of "stabilisation".

But I guess you could be right in that later contributors have seen that
delay in the vendor driver and thought that prwdn_delay corresponds to
it without noticing that they are not at all equivalent.

As the current delay is pretty much pointless (you can wait for 20 ms if
you want, it doesn't matter as the PHY hasn't been started yet) I guess
we can move it and avoid a few loops when polling for the status.

[ The next batch of clean ups also increases that silly 10 us polling
period. ]

> > Either way, separate change.
> >   
> >>>> I think we can either drop this delay completely, or move it before
> >>>> read_poll_timeout().
> >>>
> >>> It definitely shouldn't be used for any new platforms, but I opted for
> >>> the conservative route of keeping it in case some of the older platforms
> >>> actually do need it.
> >>>
> >>> My bet is that this is all copy-paste cruft that could be removed, but
> >>> I'd rather do that as a separate follow-on change. Perhaps after testing
> >>> some more SoC after removing the delay.
> >>>
> >>> SC8280XP certainly doesn't need it.
> >>
> >> I think in our case this delay just falls into status polling. We'd
> >> probably need it, if we'd add the noretain handling.
> > 
> > I'm not sure I understand what you're referring to here ("noretain
> > handling")?
> 
>  From what I see in the downstream (4.19 at hand), the sequence is the 
> following:
> 
> program_phy_config() // including SW_RESET & START_CTRL
> 
> delay
> 
> for pipe clocks:
> clk_set_flags(info->hdl, CLKFLAG_NORETAIN_MEM)
> clk_set_flags(info->hdl, CLKFLAG_NORETAIN_PERIPH)

Heh. Crazy vendor-kernel magic.

> set clock rates, prepare & enable pipe clocks
> 
> wmb()
> 
> poll for the PHY STATUS

But 5.4 has something similar:

	program + start
	delay
	enable pipe clock
	poll for status

Johan