Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
From:   Alexander Stein <alexander.stein@ew.tq-group.com>
To:     Yan Wang <rk.code@outlook.com>,
        "Russell King (Oracle)" <linux@armlinux.org.uk>
Cc:     andrew@lunn.ch, hkallweit1@gmail.com, davem@davemloft.net,
        edumazet@google.com, kuba@kernel.org, pabeni@redhat.com,
        netdev@vger.kernel.org, linux-kernel@vger.kernel.org
Subject: Re: [PATCH v5] net: mdiobus: Add a function to deassert reset
Date:   Fri, 12 May 2023 15:37:51 +0200
Message-ID: <1828875.atdPhlSkOF@steina-w>
Organization: TQ-Systems GmbH
In-Reply-To: <ZF4J1VqEqbnE6JG9@shell.armlinux.org.uk>
References: <KL1PR01MB54486A247214CC72CAB5A433E6759@KL1PR01MB5448.apcprd01.prod.exchangelabs.com> <KL1PR01MB54488021E5650ED8A203057FE6759@KL1PR01MB5448.apcprd01.prod.exchangelabs.com> <ZF4J1VqEqbnE6JG9@shell.armlinux.org.uk>
MIME-Version: 1.0
Content-Transfer-Encoding: quoted-printable
Content-Type: text/plain; charset="iso-8859-1"
Precedence: bulk

Hi Russel,

Am Freitag, 12. Mai 2023, 11:41:41 CEST schrieb Russell King (Oracle):
> On Fri, May 12, 2023 at 05:28:47PM +0800, Yan Wang wrote:
> > On 5/12/2023 5:02 PM, Russell King (Oracle) wrote:
> > > On Fri, May 12, 2023 at 03:08:53PM +0800, Yan Wang wrote:
> > > > +	gpiod_set_value_cansleep(reset, gpiod_is_active_low(reset));
> > > > +	fsleep(reset_assert_delay);
> > > > +	gpiod_set_value_cansleep(reset, !gpiod_is_active_low(reset));
> > >=20
> > > Andrew, one of the phylib maintainers and thus is responsible for code
> > > 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.
> > >=20
> > > I am going to add my voice as another phylib maintainer to this and s=
ay
> > > NO to this code, for the exact same reasons that Andrew has given.
> > >=20
> > > You now have two people responsible for the code in question telling
> > > you that this is the wrong approach.
> > >=20
> > > Until this is addressed in some way, it is pointless you posting
> > > another version of this patch.
> > >=20
> > > Thanks.
> >=20
> > I'm very sorry, I didn't have their previous intention.
> > The meaning of the two assertions is reset and reset release.
> > If you believe this is the wrong method, please ignore it.
>=20
> As Andrew has told you twice:
>=20
> We do not want to be resetting the PHY while we are probing the bus,
> and he has given one reason for it.
>=20
> 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.
>=20
> 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.

I am a bit confused by your statement regarding always resetting a PHY is a=
=20
no-no. Isn't mdiobus_register_device() exactly doing this for PHYs? Using=20
either a GPIO or reset-controller.
Thats's also what I see on our boards. During startup while device probing=
=20
there is a PHY reset, including the link reset.
And yes, that clears settings done by the firmware, e.g. setting PHY's LED=
=20
configuration.

Best
Alexander

> 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.
>=20
> 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.
>=20
> Thanks.


=2D-=20
TQ-Systems GmbH | M=FChlstra=DFe 2, Gut Delling | 82229 Seefeld, Germany
Amtsgericht M=FCnchen, HRB 105018
Gesch=E4ftsf=FChrer: Detlef Schneider, R=FCdiger Stahl, Stefan Schneider
http://www.tq-group.com/