On Fri, May 12, 2023 at 05:28:47PM +0800, Yan Wang wrote:
As Andrew has told you twice:
On 5/12/2023 5:02 PM, Russell King (Oracle) wrote:
On Fri, May 12, 2023 at 03:08:53PM +0800, Yan Wang wrote:I'm very sorry, I didn't have their previous intention.
+ gpiod_set_value_cansleep(reset, gpiod_is_active_low(reset));Andrew, one of the phylib maintainers and thus is responsible for code
+ fsleep(reset_assert_delay);
+ gpiod_set_value_cansleep(reset, !gpiod_is_active_low(reset));
in the area you are touching. Andrew has complained about the above
which asserts and then deasserts reset on two occasions now, explained
why it is wrong, but still the code persists in doing this.
I am going to add my voice as another phylib maintainer to this and say
NO to this code, for the exact same reasons that Andrew has given.
You now have two people responsible for the code in question telling
you that this is the wrong approach.
Until this is addressed in some way, it is pointless you posting
another version of this patch.
Thanks.
The meaning of the two assertions is reset and reset release.
If you believe this is the wrong method, please ignore it.
We do not want to be resetting the PHY while we are probing the bus,
and he has given one reason for it.
The reason Andrew gave is that hardware resetting a PHY that was not
already in reset means that any link is immediately terminated, and
the PHY has to renegotiate with its link partner when your code
subsequently releases the reset signal. This is *not* the behaviour
that phylib maintainers want to see.
The second problem that Andrew didn't mention is that always hardware
resetting the PHY will clear out any firmware setup that has happened
before the kernel has been booted. Again, that's a no-no.
The final issue I have is that your patch is described as "add a
function do *DEASSERT* reset" not "add a function to *ALWAYS* *RESET*"
which is what you are actually doing here. So the commit message and
the code disagree with what's going on - the summary line is at best
misleading.
If your hardware case is that the PHY is already in reset, then of
course you don't see any of the above as a problem, but that is not
universally true - and that is exactly why Andrew is bringing this
up. There are platforms out there where the reset is described in
the firmware hardware description, *but* when the kernel boots, the
reset signal is already deasserted. Raising it during kernel boot as
you are doing will terminate the PHY's link with the remote end,
and then deasserting it will cause it to renegotiate.
Thanks.