It is possible to mount multiple sub-devices on the mido bus.
The hardware power-on does not necessarily reset these devices.
The device may be in an uncertain state, causing the device's ID
to not be scanned.
So,before adding a reset to the scan, make sure the device is in
normal working mode.
Reported-by: kernel test robot <[email protected]>
Link: https://lore.kernel.org/oe-kbuild-all/[email protected]/
Signed-off-by: Yan Wang <[email protected]>
---
v5:
- fixed code style.
- Add fwnode_property_read_u32()'s return value processing.
v4: https://lore.kernel.org/all/KL1PR01MB54480925428513803DF3D03AE6749@KL1PR01MB5448.apcprd01.prod.exchangelabs.com/
- Get pulse width and settling time from the device tree
- Add logic for processing (PTR_ERR(reset) == -EPROBE_DEFER)
- included <linux/goio/consumer.h>
- fixed commit message
v3: https://lore.kernel.org/all/KL1PR01MB5448A33A549CDAD7D68945B9E6779@KL1PR01MB5448.apcprd01.prod.exchangelabs.com/
- fixed commit message
v2: https://lore.kernel.org/all/KL1PR01MB54482416A8BE0D80EA27223CE6779@KL1PR01MB5448.apcprd01.prod.exchangelabs.com/
- fixed commit message
- Using gpiod_ replace gpio_
v1: https://lore.kernel.org/all/KL1PR01MB5448631F2D6F71021602117FE6769@KL1PR01MB5448.apcprd01.prod.exchangelabs.com/
- Incorrect description of commit message.
- The gpio-api too old
---
drivers/net/mdio/fwnode_mdio.c | 48 ++++++++++++++++++++++++++++++++++
1 file changed, 48 insertions(+)
diff --git a/drivers/net/mdio/fwnode_mdio.c b/drivers/net/mdio/fwnode_mdio.c
index 1183ef5e203e..02e89e25c23d 100644
--- a/drivers/net/mdio/fwnode_mdio.c
+++ b/drivers/net/mdio/fwnode_mdio.c
@@ -11,6 +11,7 @@
#include <linux/of.h>
#include <linux/phy.h>
#include <linux/pse-pd/pse.h>
+#include <linux/gpio/consumer.h>
MODULE_AUTHOR("Calvin Johnson <[email protected]>");
MODULE_LICENSE("GPL");
@@ -57,6 +58,51 @@ fwnode_find_mii_timestamper(struct fwnode_handle *fwnode)
return register_mii_timestamper(arg.np, arg.args[0]);
}
+static void fwnode_mdiobus_pre_enable_phy(struct fwnode_handle *fwnode)
+{
+ unsigned int reset_deassert_delay;
+ unsigned int reset_assert_delay;
+ struct gpio_desc *reset;
+ int ret;
+
+ ret = fwnode_property_read_u32(fwnode, "reset-assert-us",
+ &reset_assert_delay);
+ if (ret) {
+ pr_err("%pOFn: %s : The reset-assert-us property missing\n",
+ to_of_node(fwnode), __func__);
+ return;
+ }
+
+ ret = fwnode_property_read_u32(fwnode, "reset-deassert-us",
+ &reset_deassert_delay);
+ if (ret) {
+ pr_err("%pOFn: %s : The reset-deassert-us property missing\n",
+ to_of_node(fwnode), __func__);
+ return;
+ }
+
+ reset = fwnode_gpiod_get_index(fwnode, "reset", 0, GPIOD_OUT_LOW, NULL);
+ if (IS_ERR(reset)) {
+ if (PTR_ERR(reset) == -EPROBE_DEFER)
+ pr_debug("%pOFn: %s: GPIOs not yet available, retry later\n",
+ to_of_node(fwnode), __func__);
+ else
+ pr_err("%pOFn: %s: Can't get reset line property\n",
+ to_of_node(fwnode), __func__);
+
+ return;
+ }
+
+ gpiod_set_value_cansleep(reset, gpiod_is_active_low(reset));
+ fsleep(reset_assert_delay);
+ gpiod_set_value_cansleep(reset, !gpiod_is_active_low(reset));
+ fsleep(reset_deassert_delay);
+ /*Release phy's reset line, mdiobus_register_gpiod() need to
+ *request it
+ */
+ gpiod_put(reset);
+}
+
int fwnode_mdiobus_phy_device_register(struct mii_bus *mdio,
struct phy_device *phy,
struct fwnode_handle *child, u32 addr)
@@ -119,6 +165,8 @@ int fwnode_mdiobus_register_phy(struct mii_bus *bus,
u32 phy_id;
int rc;
+ fwnode_mdiobus_pre_enable_phy(child);
+
psec = fwnode_find_pse_control(child);
if (IS_ERR(psec))
return PTR_ERR(psec);
--
2.17.1
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.
--
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!
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.
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.
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.
--
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!
On 5/12/2023 5:41 PM, Russell King (Oracle) wrote:
> 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.
>
> 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.
>
Thank you very much for your explanation. I apologize to Andrew
and I will handle it on my bootloader
Best Regards
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.
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/
> On May 12, 2023, at 21:37, Alexander Stein <[email protected]> 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/