Re: [PATCH 08/13] phy: qcom-qmp-pcie: drop power-down delay config
From: Dmitry Baryshkov
Date: Tue Oct 11 2022 - 10:41:38 EST
On 11/10/2022 17:17, Johan Hovold wrote:
On Tue, Oct 11, 2022 at 05:04:04PM +0300, Dmitry Baryshkov wrote:
On 11/10/2022 16:53, Johan Hovold wrote:
On Tue, Oct 11, 2022 at 04:46:53PM +0300, Dmitry Baryshkov wrote:
On 11/10/2022 16:14, Johan Hovold wrote:
The power-down delay was included in the first version of the QMP driver
as an optional delay after powering on the PHY (using
POWER_DOWN_CONTROL) and just before starting it. Later changes modified
this sequence by powering on before initialising the PHY, but the
optional delay stayed where it was (i.e. before starting the PHY).
The vendor driver does not use a delay before starting the PHY and this
is likely not needed on any platform unless there is a corresponding
delay in the vendor kernel init sequence tables (i.e. in devicetree).
Let's keep the delay for now, but drop the redundant delay period
configuration while increasing the unnecessarily low timer slack
somewhat.
Actually, the vendor driver does this 995..1005 sleep. But contrary to
our driver it does that after programming whole PHY init sequence, which
includes SW_RESET / START_CTL, but before programming the pipe clocks.
Right, it does it after starting the PHY which means that you don't have
to poll for as long for the PHY status.
It's a different delay entirely.
No-no-no. The 995-1005 delay was added guess for which SoC? For ipq8074,
where the config tables contain the ugly CFG_L writes for SW_RESET /
START_CTRL. So, it is the same delay, but added by somebody who didn't
care enough. The original 10-11 delay is a completely different story,
you are correct here.
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.
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)
set clock rates, prepare & enable pipe clocks
wmb()
poll for the PHY STATUS
--
With best wishes
Dmitry