On Tue, Oct 11, 2022 at 05:41:28PM +0300, Dmitry Baryshkov wrote:

On 11/10/2022 17:17, Johan Hovold wrote:

quoted

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.

quoted

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. ]

quoted

Either way, separate change.
  

quoted

quoted

quoted

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

-- 
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy