Message-ID: <KL1PR01MB5448242DBD4EA7170341A418E6759@KL1PR01MB5448.apcprd01.prod.exchangelabs.com>
Date:   Sat, 13 May 2023 00:30:09 +0800
From:   Yan Wang <rk.code@...look.com>
To:     Alexander Stein <alexander.stein@...tq-group.com>
Cc:     "Russell King (Oracle)" <linux@...linux.org.uk>,
        Andrew Lunn <andrew@...n.ch>, hkallweit1@...il.com,
        davem@...emloft.net, edumazet@...gle.com, kuba@...nel.org,
        pabeni@...hat.com, netdev@...r.kernel.org,
        open list <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v5] net: mdiobus: Add a function to deassert reset



> On May 12, 2023, at 21:37, Alexander Stein <alexander.stein@...tq-group.com> wrote:
> 
> 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));
>>>> 
>>>> 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.
>>>> 
>>>> 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.
>>> 
>>> 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.
>> 
>> As Andrew has told you twice:
>> 
>> 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.
> 
> I am a bit confused by your statement regarding always resetting a PHY is a 
> no-no. Isn't mdiobus_register_device() exactly doing this for PHYs? Using 
> either a GPIO or reset-controller.
> Thats's also what I see on our boards. During startup while device probing 
> there is a PHY reset, including the link reset.
> And yes, that clears settings done by the firmware, e.g. setting PHY's LED 
> configuration.
> 



What he expressed is that the phy has been linked before the kernel has been booted, 
and at this point, resetting the phy hardware will lose its original configuration. 
The main focus is on fast links, resetting phy, and renegotiate

I am not sure if my statement is correct

the mdiobus_ register_ Device (), I didn't understand it either In the following example, 
I submitted a patch and did the same thing.

int phy_device_register(struct phy_device *phydev)
{
	int err;

	*err = mdiobus_register_device(&phydev->mdio);*
	if (err)
		return err;

	/* Deassert the reset signal */
	*phy_device_reset(phydev, 0);*

	/* Run all of the fixups for this PHY */
	err = phy_scan_fixups(phydev);
	if (err) {
		phydev_err(phydev, "failed to initialize\n");
		goto out;
	}

	err = device_add(&phydev->mdio.dev);
	if (err) {
		phydev_err(phydev, "failed to add\n");
		goto out;
	}

	return 0;

 out:
	/* Assert the reset signal */
	phy_device_reset(phydev, 1);

	mdiobus_unregister_device(&phydev->mdio);
	return err;
}

Firstly, I think this operation is too late.

Secondly, it was in the boot program that I did not reset my phy and was unable to detect it, 
so I submitted a patch, which caused trouble for maintenance personnel.


> 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.
>> 
>> 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.
> 
> 
> -- 
> TQ-Systems GmbH | Mühlstraße 2, Gut Delling | 82229 Seefeld, Germany
> Amtsgericht München, HRB 105018
> Geschäftsführer: Detlef Schneider, Rüdiger Stahl, Stefan Schneider
> http://www.tq-group.com/

Powered by blists - more mailing lists