When the reset gpio is defined within the node of the device tree
describing the PHY, the reset is initialized and managed only after
calling the fwnode_mdiobus_phy_device_register function.
However, before calling it, the MDIO communication is checked by the
get_phy_device function.
When this happen and the reset GPIO was somehow previously set down,
the get_phy_device function fails, preventing the PHY detection.
These changes force the deassert of the MDIO reset signal before
checking the MDIO channel.
The PHY may require a minimum deassert time before being responsive:
use a reasonable sleep time after forcing the deassert of the MDIO
reset signal.
Once done, free the gpio descriptor to allow managing it later.
Signed-off-by: Pierluigi Passaro <[email protected]>
Signed-off-by: FrancescoFerraro <[email protected]>
---
drivers/net/mdio/fwnode_mdio.c | 25 +++++++++++++++++++++++++
1 file changed, 25 insertions(+)
diff --git a/drivers/net/mdio/fwnode_mdio.c b/drivers/net/mdio/fwnode_mdio.c
index b782c35c4ac1..1f4b8c4c1f60 100644
--- a/drivers/net/mdio/fwnode_mdio.c
+++ b/drivers/net/mdio/fwnode_mdio.c
@@ -8,6 +8,8 @@
#include <linux/acpi.h>
#include <linux/fwnode_mdio.h>
+#include <linux/gpio/consumer.h>
+#include <linux/gpio/driver.h>
#include <linux/of.h>
#include <linux/phy.h>
#include <linux/pse-pd/pse.h>
@@ -118,6 +120,8 @@ int fwnode_mdiobus_register_phy(struct mii_bus *bus,
bool is_c45 = false;
u32 phy_id;
int rc;
+ int reset_deassert_delay = 0;
+ struct gpio_desc *reset_gpio;
psec = fwnode_find_pse_control(child);
if (IS_ERR(psec))
@@ -134,10 +138,31 @@ int fwnode_mdiobus_register_phy(struct mii_bus *bus,
if (rc >= 0)
is_c45 = true;
+ reset_gpio = fwnode_gpiod_get_index(child, "reset", 0, GPIOD_OUT_LOW, "PHY reset");
+ if (reset_gpio == ERR_PTR(-EPROBE_DEFER)) {
+ dev_dbg(&bus->dev, "reset signal for PHY@%u not ready\n", addr);
+ return -EPROBE_DEFER;
+ } else if (IS_ERR(reset_gpio)) {
+ if (reset_gpio == ERR_PTR(-ENOENT))
+ dev_dbg(&bus->dev, "reset signal for PHY@%u not defined\n", addr);
+ else
+ dev_dbg(&bus->dev, "failed to request reset for PHY@%u, error %ld\n", addr, PTR_ERR(reset_gpio));
+ reset_gpio = NULL;
+ } else {
+ dev_dbg(&bus->dev, "deassert reset signal for PHY@%u\n", addr);
+ fwnode_property_read_u32(child, "reset-deassert-us",
+ &reset_deassert_delay);
+ if (reset_deassert_delay)
+ fsleep(reset_deassert_delay);
+ }
+
if (is_c45 || fwnode_get_phy_id(child, &phy_id))
phy = get_phy_device(bus, addr, is_c45);
else
phy = phy_device_create(bus, addr, phy_id, 0, NULL);
+
+ gpiochip_free_own_desc(reset_gpio);
+
if (IS_ERR(phy)) {
rc = PTR_ERR(phy);
goto clean_mii_ts;
--
2.37.2
On Sun, Jan 15, 2023 at 05:10:06PM +0100, Pierluigi Passaro wrote:
> When the reset gpio is defined within the node of the device tree
> describing the PHY, the reset is initialized and managed only after
> calling the fwnode_mdiobus_phy_device_register function.
> However, before calling it, the MDIO communication is checked by the
> get_phy_device function.
> When this happen and the reset GPIO was somehow previously set down,
> the get_phy_device function fails, preventing the PHY detection.
> These changes force the deassert of the MDIO reset signal before
> checking the MDIO channel.
> The PHY may require a minimum deassert time before being responsive:
> use a reasonable sleep time after forcing the deassert of the MDIO
> reset signal.
> Once done, free the gpio descriptor to allow managing it later.
This has been discussed before. The problem is, it is not just a reset
GPIO. There could also be a clock which needs turning on, a regulator,
and/or a linux reset controller. And what order do you turn these on?
The conclusions of the discussion is you assume the device cannot be
found by enumeration, and you put the ID in the compatible. That is
enough to get the driver to load, and the driver can then turn
everything on in the correct order, with the correct delays, etc.
I think there has been some work on generic power sequencing. I've not
being following it, so i've no idea how far it has got. If that could
be used to solve this problem for all the possible controls of a PHY,
i would be open for such patches.
Andrew
On Sun, Jan 15, 2023 at 6:08 PM Andrew Lunn <[email protected]> wrote:
> On Sun, Jan 15, 2023 at 05:10:06PM +0100, Pierluigi Passaro wrote:
> > When the reset gpio is defined within the node of the device tree
> > describing the PHY, the reset is initialized and managed only after
> > calling the fwnode_mdiobus_phy_device_register function.
> > However, before calling it, the MDIO communication is checked by the
> > get_phy_device function.
> > When this happens and the reset GPIO was somehow previously set down,
> > the get_phy_device function fails, preventing the PHY detection.
> > These changes force the deassert of the MDIO reset signal before
> > checking the MDIO channel.
> > The PHY may require a minimum deassert time before being responsive:
> > use a reasonable sleep time after forcing the deassert of the MDIO
> > reset signal.
> > Once done, free the gpio descriptor to allow managing it later.
>
> This has been discussed before. The problem is, it is not just a reset
> GPIO. There could also be a clock which needs turning on, a regulator,
> and/or a linux reset controller. And what order do you turn these on?
>
> The conclusion of the discussion is you assume the device cannot be
> found by enumeration, and you put the ID in the compatible. That is
> enough to get the driver to load, and the driver can then turn
> everything on in the correct order, with the correct delays, etc.
>
Can you provide any reference to this discussion?
From our investigation, the MDIO communication is checked before managing
the "reset" gpio .
This behaviour is generally not visible, but easily reproducible with all NXP
platforms with dual fec (iMX28, iMX6UL, iMX7, iMX8QM, iMX8QXP)
where the MDIO bus is owned by the 1st interface but shared with the 2nd.
When the 1st interface is probed, this causes the probe of the MDIO bus
when the 2nd interface is not yet set up.
Here a reference configuration
https://github.com/varigit/linux-imx/blob/5.15-2.0.x-imx_var01/arch/arm64/boot/dts/freescale/imx8qm-var-spear.dtsi#L168-L219
Without this patch, the above settings can fail simply forcing the reset GPIOs
to zero in u-boot.
Please let me know if further details are needed.
>
> I think there has been some work on generic power sequencing. I've not
> being following it, so I've no idea how far it has got. If that could
> be used to solve this problem for all the possible controls of a PHY,
> i would be open for such patches.
>
> Andrew
> This behaviour is generally not visible, but easily reproducible with all NXP
> platforms with dual fec (iMX28, iMX6UL, iMX7, iMX8QM, iMX8QXP)
> where the MDIO bus is owned by the 1st interface but shared with the 2nd.
> When the 1st interface is probed, this causes the probe of the MDIO bus
> when the 2nd interface is not yet set up.
This sounds like a different issue.
We need to split the problem up into two.
1) Does probing the MDIO bus find both PHYs?
2) Do the MACs get linked to the PHYs.
If the reset is asserted at the point the MDIO bus is probed, you
probably don't find the PHY because it does not respond to register
reads. Your patch probably ensures it is out of reset so it is
enumerated.
For fec1, if the PHY is found during probe, connecting to the PHY will
work without issues. However, fec2 can potentially have ordering
issues. Has the MDIO bus finished being probed by the time fec2 looks
for it? If it is not there you want to return -EPROBE_DEFERED so that
the driver code will try again later.
There have been patches to do with ordering recently, but they have
been more to do with suspend/resume. So please make sure you are
testing net-next, if ordering is your real problem. You also appear to
be missing a lot of stable patches, so please bring you kernel up to
date on the 5.15 branch, you are way out of date.
Andrew
On Sun, Jan 15, 2023 at 8:02 PM Andrew Lunn <[email protected]> wrote:
> > This behaviour is generally not visible, but easily reproducible with all NXP
> > platforms with dual fec (iMX28, iMX6UL, iMX7, iMX8QM, iMX8QXP)
> > where the MDIO bus is owned by the 1st interface but shared with the 2nd.
> > When the 1st interface is probed, this causes the probe of the MDIO bus
> > when the 2nd interface is not yet set up.
>
> This sounds like a different issue.
>
> We need to split the problem up into two.
>
> 1) Does probing the MDIO bus find both PHYs?
>
> 2) Do the MACs get linked to the PHYs.
>
> If the reset is asserted at the point the MDIO bus is probed, you
> probably don't find the PHY because it does not respond to register
> reads. Your patch probably ensures it is out of reset so it is
> enumerated.
>
You are perfectly right: this patch fixes only the 1st problem.
For the 2nd problem, I've already sent a dedicated patch:
https://lore.kernel.org/all/[email protected]/
>
> For fec1, if the PHY is found during probe, connecting to the PHY will
> work without issues. However, fec2 can potentially have ordering
> issues. Has the MDIO bus finished being probed by the time fec2 looks
> for it? If it is not there you want to return -EPROBE_DEFERED so that
> the driver code will try again later.
>
> There have been patches to do with ordering recently, but they have
> been more to do with suspend/resume. So please make sure you are
> testing net-next, if ordering is your real problem. You also appear to
> be missing a lot of stable patches, so please bring you kernel up to
> date on the 5.15 branch, you are way out of date.
>
> Andrew
On 1/15/23 09:08, Andrew Lunn wrote:
> On Sun, Jan 15, 2023 at 05:10:06PM +0100, Pierluigi Passaro wrote:
>> When the reset gpio is defined within the node of the device tree
>> describing the PHY, the reset is initialized and managed only after
>> calling the fwnode_mdiobus_phy_device_register function.
>> However, before calling it, the MDIO communication is checked by the
>> get_phy_device function.
>> When this happen and the reset GPIO was somehow previously set down,
>> the get_phy_device function fails, preventing the PHY detection.
>> These changes force the deassert of the MDIO reset signal before
>> checking the MDIO channel.
>> The PHY may require a minimum deassert time before being responsive:
>> use a reasonable sleep time after forcing the deassert of the MDIO
>> reset signal.
>> Once done, free the gpio descriptor to allow managing it later.
> This has been discussed before. The problem is, it is not just a reset
> GPIO. There could also be a clock which needs turning on, a regulator,
> and/or a linux reset controller. And what order do you turn these on?
>
> The conclusions of the discussion is you assume the device cannot be
> found by enumeration, and you put the ID in the compatible. That is
> enough to get the driver to load, and the driver can then turn
> everything on in the correct order, with the correct delays, etc.
I've been running into this same problem again and again over the past
years.
Specifying the ID as part of the compatible string works for clause 22
PHYs, but for clause 45 PHYs it does not work. The code always wants to
read the ID from the PHY itself. But I do not understand things well
enough to tell whether that's a fundamental restriction of C45 or just
our implementation and the implementation can be changed to fix it.
Do you have some thoughts on this?
On Sun, Jan 15, 2023 at 10:59 PM Lars-Peter Clausen <[email protected]> wrote:
> On 1/15/23 09:08, Andrew Lunn wrote:
> > On Sun, Jan 15, 2023 at 05:10:06PM +0100, Pierluigi Passaro wrote:
> >> When the reset gpio is defined within the node of the device tree
> >> describing the PHY, the reset is initialized and managed only after
> >> calling the fwnode_mdiobus_phy_device_register function.
> >> However, before calling it, the MDIO communication is checked by the
> >> get_phy_device function.
> >> When this happen and the reset GPIO was somehow previously set down,
> >> the get_phy_device function fails, preventing the PHY detection.
> >> These changes force the deassert of the MDIO reset signal before
> >> checking the MDIO channel.
> >> The PHY may require a minimum deassert time before being responsive:
> >> use a reasonable sleep time after forcing the deassert of the MDIO
> >> reset signal.
> >> Once done, free the gpio descriptor to allow managing it later.
> > This has been discussed before. The problem is, it is not just a reset
> > GPIO. There could also be a clock which needs turning on, a regulator,
> > and/or a linux reset controller. And what order do you turn these on?
> >
> > The conclusions of the discussion is you assume the device cannot be
> > found by enumeration, and you put the ID in the compatible. That is
> > enough to get the driver to load, and the driver can then turn
> > everything on in the correct order, with the correct delays, etc.
>
> I've been running into this same problem again and again over the past
> years.
>
> Specifying the ID as part of the compatible string works for clause 22
> PHYs, but for clause 45 PHYs it does not work. The code always wants to
> read the ID from the PHY itself. But I do not understand things well
> enough to tell whether that's a fundamental restriction of C45 or just
> our implementation and the implementation can be changed to fix it.
>
> Do you have some thoughts on this?
>
IMHO, since the framework allows defining the reset GPIO, it does not sound
reasonable to manage it only after checking if the PHY can communicate:
if the reset is asserted, the PHY cannot communicate at all.
This patch just ensures that, if the reset GPIO is defined, it's not asserted
while checking the communication.
On 1/15/23 14:33, Pierluigi Passaro wrote:
> On Sun, Jan 15, 2023 at 10:59 PM Lars-Peter Clausen <[email protected]> wrote:
>> On 1/15/23 09:08, Andrew Lunn wrote:
>>> On Sun, Jan 15, 2023 at 05:10:06PM +0100, Pierluigi Passaro wrote:
>>>> When the reset gpio is defined within the node of the device tree
>>>> describing the PHY, the reset is initialized and managed only after
>>>> calling the fwnode_mdiobus_phy_device_register function.
>>>> However, before calling it, the MDIO communication is checked by the
>>>> get_phy_device function.
>>>> When this happen and the reset GPIO was somehow previously set down,
>>>> the get_phy_device function fails, preventing the PHY detection.
>>>> These changes force the deassert of the MDIO reset signal before
>>>> checking the MDIO channel.
>>>> The PHY may require a minimum deassert time before being responsive:
>>>> use a reasonable sleep time after forcing the deassert of the MDIO
>>>> reset signal.
>>>> Once done, free the gpio descriptor to allow managing it later.
>>> This has been discussed before. The problem is, it is not just a reset
>>> GPIO. There could also be a clock which needs turning on, a regulator,
>>> and/or a linux reset controller. And what order do you turn these on?
>>>
>>> The conclusions of the discussion is you assume the device cannot be
>>> found by enumeration, and you put the ID in the compatible. That is
>>> enough to get the driver to load, and the driver can then turn
>>> everything on in the correct order, with the correct delays, etc.
>> I've been running into this same problem again and again over the past
>> years.
>>
>> Specifying the ID as part of the compatible string works for clause 22
>> PHYs, but for clause 45 PHYs it does not work. The code always wants to
>> read the ID from the PHY itself. But I do not understand things well
>> enough to tell whether that's a fundamental restriction of C45 or just
>> our implementation and the implementation can be changed to fix it.
>>
>> Do you have some thoughts on this?
>>
> IMHO, since the framework allows defining the reset GPIO, it does not sound
> reasonable to manage it only after checking if the PHY can communicate:
> if the reset is asserted, the PHY cannot communicate at all.
> This patch just ensures that, if the reset GPIO is defined, it's not asserted
> while checking the communication.
I fully agree with you and I think this is the right approach, cause it
is required to make systems work. But I've seen two attempts in the past
that did the very same thing and they always got rejected. I can't find
the patches anymore, but I think one was maybe 2 years ago.
On Sun, Jan 15, 2023 at 11:56 PM Lars-Peter Clausen <[email protected]> wrote:
> On 1/15/23 14:33, Pierluigi Passaro wrote:
> > On Sun, Jan 15, 2023 at 10:59 PM Lars-Peter Clausen <[email protected]> wrote:
> >> On 1/15/23 09:08, Andrew Lunn wrote:
> >>> On Sun, Jan 15, 2023 at 05:10:06PM +0100, Pierluigi Passaro wrote:
> >>>> When the reset gpio is defined within the node of the device tree
> >>>> describing the PHY, the reset is initialized and managed only after
> >>>> calling the fwnode_mdiobus_phy_device_register function.
> >>>> However, before calling it, the MDIO communication is checked by the
> >>>> get_phy_device function.
> >>>> When this happen and the reset GPIO was somehow previously set down,
> >>>> the get_phy_device function fails, preventing the PHY detection.
> >>>> These changes force the deassert of the MDIO reset signal before
> >>>> checking the MDIO channel.
> >>>> The PHY may require a minimum deassert time before being responsive:
> >>>> use a reasonable sleep time after forcing the deassert of the MDIO
> >>>> reset signal.
> >>>> Once done, free the gpio descriptor to allow managing it later.
> >>> This has been discussed before. The problem is, it is not just a reset
> >>> GPIO. There could also be a clock which needs turning on, a regulator,
> >>> and/or a linux reset controller. And what order do you turn these on?
> >>>
> >>> The conclusions of the discussion is you assume the device cannot be
> >>> found by enumeration, and you put the ID in the compatible. That is
> >>> enough to get the driver to load, and the driver can then turn
> >>> everything on in the correct order, with the correct delays, etc.
> >> I've been running into this same problem again and again over the past
> >> years.
> >>
> >> Specifying the ID as part of the compatible string works for clause 22
> >> PHYs, but for clause 45 PHYs it does not work. The code always wants to
> >> read the ID from the PHY itself. But I do not understand things well
> >> enough to tell whether that's a fundamental restriction of C45 or just
> >> our implementation and the implementation can be changed to fix it.
> >>
> >> Do you have some thoughts on this?
> >>
> > IMHO, since the framework allows defining the reset GPIO, it does not sound
> > reasonable to manage it only after checking if the PHY can communicate:
> > if the reset is asserted, the PHY cannot communicate at all.
> > This patch just ensures that, if the reset GPIO is defined, it's not asserted
> > while checking the communication.
>
> I fully agree with you and I think this is the right approach, cause it
> is required to make systems work. But I've seen two attempts in the past
> that did the very same thing and they always got rejected. I can't find
> the patches anymore, but I think one was maybe 2 years ago.
>
Rejection is always a chance ;)
As long I can understand the reasons, I can at least try improving this patch.
> Specifying the ID as part of the compatible string works for clause 22 PHYs,
> but for clause 45 PHYs it does not work. The code always wants to read the
> ID from the PHY itself. But I do not understand things well enough to tell
> whether that's a fundamental restriction of C45 or just our implementation
> and the implementation can be changed to fix it.
>
> Do you have some thoughts on this?
Do you have more details about what goes wrong? Which PHY driver is
it? What compatibles do you put into DT for the PHY?
To some extent, the ID is only used to find the driver. A C45 device
has a lot of ID register, and all of them are used by phy_bus_match()
to see if a driver matches. So you need to be careful which ID you
pick, it needs to match the driver.
It is the driver which decides to use C22 or C45 to talk to the PHY.
However, we do have:
static int phy_probe(struct device *dev)
{
...
else if (phydev->is_c45)
err = genphy_c45_pma_read_abilities(phydev);
else
err = genphy_read_abilities(phydev);
so it could be a C45 PHY probed using an ID does not have
phydev->is_c45 set, and so it looks in the wrong place for the
capabilities. Make sure you also have the compatible
"ethernet-phy-ieee802.3-c45" which i think should cause is_c45 to be
set.
There is no fundamental restriction that i know of here, it probably
just needs somebody to debug it and find where it goes wrong.
Ah!
int fwnode_mdiobus_register_phy(struct mii_bus *bus,
struct fwnode_handle *child, u32 addr)
{
...
rc = fwnode_property_match_string(child, "compatible",
"ethernet-phy-ieee802.3-c45");
if (rc >= 0)
is_c45 = true;
if (is_c45 || fwnode_get_phy_id(child, &phy_id))
phy = get_phy_device(bus, addr, is_c45);
else
phy = phy_device_create(bus, addr, phy_id, 0, NULL);
So compatible "ethernet-phy-ieee802.3-c45" results in is_c45 being set
true. The if (is_c45 || is then true, so it does not need to call
fwnode_get_phy_id(child, &phy_id) so ignores whatever ID is in DT and
asks the PHY.
Try this, totally untested:
diff --git a/drivers/net/mdio/fwnode_mdio.c b/drivers/net/mdio/fwnode_mdio.c
index b782c35c4ac1..13be23f8ac97 100644
--- a/drivers/net/mdio/fwnode_mdio.c
+++ b/drivers/net/mdio/fwnode_mdio.c
@@ -134,10 +134,10 @@ int fwnode_mdiobus_register_phy(struct mii_bus *bus,
if (rc >= 0)
is_c45 = true;
- if (is_c45 || fwnode_get_phy_id(child, &phy_id))
+ if (fwnode_get_phy_id (child, &phy_id))
phy = get_phy_device(bus, addr, is_c45);
else
- phy = phy_device_create(bus, addr, phy_id, 0, NULL);
+ phy = phy_device_create(bus, addr, phy_id, is_c45, NULL);
if (IS_ERR(phy)) {
rc = PTR_ERR(phy);
goto clean_mii_ts;
Andrew
> IMHO, since the framework allows defining the reset GPIO, it does not sound
> reasonable to manage it only after checking if the PHY can communicate:
> if the reset is asserted, the PHY cannot communicate at all.
> This patch just ensures that, if the reset GPIO is defined, it's not asserted
> while checking the communication.
The problem is, you are only solving 1/4 of the problem. What about
the clock the PHY needs? And the regulator, and the linux reset
controller? And what order to do enable these, and how long do you
wait between each one?
And why are you solving this purely for Ethernet PHYs when the same
problem probably affects other sorts of devices which have reset
GPIOs, regulators and clocks? It looks like MMC/SDIO devices have a
similar problem.
https://lwn.net/Articles/867740/
As i said, i've not been following this work. Has it got anywhere? Can
ethernet PHYs use it?
Andrew
On 1/15/23 15:55, Andrew Lunn wrote:
>> Specifying the ID as part of the compatible string works for clause 22 PHYs,
>> but for clause 45 PHYs it does not work. The code always wants to read the
>> ID from the PHY itself. But I do not understand things well enough to tell
>> whether that's a fundamental restriction of C45 or just our implementation
>> and the implementation can be changed to fix it.
>>
>> Do you have some thoughts on this?
> Do you have more details about what goes wrong? Which PHY driver is
> it? What compatibles do you put into DT for the PHY?
>
> To some extent, the ID is only used to find the driver. A C45 device
> has a lot of ID register, and all of them are used by phy_bus_match()
> to see if a driver matches. So you need to be careful which ID you
> pick, it needs to match the driver.
>
> It is the driver which decides to use C22 or C45 to talk to the PHY.
> However, we do have:
>
> static int phy_probe(struct device *dev)
> {
> ...
> else if (phydev->is_c45)
> err = genphy_c45_pma_read_abilities(phydev);
> else
> err = genphy_read_abilities(phydev);
>
> so it could be a C45 PHY probed using an ID does not have
> phydev->is_c45 set, and so it looks in the wrong place for the
> capabilities. Make sure you also have the compatible
> "ethernet-phy-ieee802.3-c45" which i think should cause is_c45 to be
> set.
>
> There is no fundamental restriction that i know of here, it probably
> just needs somebody to debug it and find where it goes wrong.
>
> Ah!
>
> int fwnode_mdiobus_register_phy(struct mii_bus *bus,
> struct fwnode_handle *child, u32 addr)
> {
> ...
> rc = fwnode_property_match_string(child, "compatible",
> "ethernet-phy-ieee802.3-c45");
> if (rc >= 0)
> is_c45 = true;
>
> if (is_c45 || fwnode_get_phy_id(child, &phy_id))
> phy = get_phy_device(bus, addr, is_c45);
> else
> phy = phy_device_create(bus, addr, phy_id, 0, NULL);
>
>
> So compatible "ethernet-phy-ieee802.3-c45" results in is_c45 being set
> true. The if (is_c45 || is then true, so it does not need to call
> fwnode_get_phy_id(child, &phy_id) so ignores whatever ID is in DT and
> asks the PHY.
>
> Try this, totally untested:
>
> diff --git a/drivers/net/mdio/fwnode_mdio.c b/drivers/net/mdio/fwnode_mdio.c
> index b782c35c4ac1..13be23f8ac97 100644
> --- a/drivers/net/mdio/fwnode_mdio.c
> +++ b/drivers/net/mdio/fwnode_mdio.c
> @@ -134,10 +134,10 @@ int fwnode_mdiobus_register_phy(struct mii_bus *bus,
> if (rc >= 0)
> is_c45 = true;
>
> - if (is_c45 || fwnode_get_phy_id(child, &phy_id))
> + if (fwnode_get_phy_id (child, &phy_id))
> phy = get_phy_device(bus, addr, is_c45);
> else
> - phy = phy_device_create(bus, addr, phy_id, 0, NULL);
> + phy = phy_device_create(bus, addr, phy_id, is_c45, NULL);
> if (IS_ERR(phy)) {
> rc = PTR_ERR(phy);
> goto clean_mii_ts;
>
I think part of the problem is that for C45 there are a few other fields
that get populated by the ID detection, such as devices_in_package and
mmds_present. Is this something we can do after running the PHY drivers
probe function? Or is it too late at that point?
Hi Pierluigi,
Thank you for the patch! Yet something to improve:
[auto build test ERROR on net-next/master]
[also build test ERROR on net/master linus/master v6.2-rc4 next-20230116]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]
url: https://github.com/intel-lab-lkp/linux/commits/Pierluigi-Passaro/net-mdio-force-deassert-MDIO-reset-signal/20230116-001044
patch link: https://lore.kernel.org/r/20230115161006.16431-1-pierluigi.p%40variscite.com
patch subject: [PATCH] net: mdio: force deassert MDIO reset signal
config: nios2-defconfig
compiler: nios2-linux-gcc (GCC) 12.1.0
reproduce (this is a W=1 build):
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# https://github.com/intel-lab-lkp/linux/commit/3f08f04af6947d4fce17b11443001c4e386ca66e
git remote add linux-review https://github.com/intel-lab-lkp/linux
git fetch --no-tags linux-review Pierluigi-Passaro/net-mdio-force-deassert-MDIO-reset-signal/20230116-001044
git checkout 3f08f04af6947d4fce17b11443001c4e386ca66e
# save the config file
mkdir build_dir && cp config build_dir/.config
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-12.1.0 make.cross W=1 O=build_dir ARCH=nios2 olddefconfig
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-12.1.0 make.cross W=1 O=build_dir ARCH=nios2 SHELL=/bin/bash
If you fix the issue, kindly add following tag where applicable
| Reported-by: kernel test robot <[email protected]>
All errors (new ones prefixed by >>):
nios2-linux-ld: drivers/net/mdio/fwnode_mdio.o: in function `fwnode_mdiobus_register_phy':
>> drivers/net/mdio/fwnode_mdio.c:164: undefined reference to `gpiochip_free_own_desc'
drivers/net/mdio/fwnode_mdio.c:164:(.text+0x230): relocation truncated to fit: R_NIOS2_CALL26 against `gpiochip_free_own_desc'
vim +164 drivers/net/mdio/fwnode_mdio.c
113
114 int fwnode_mdiobus_register_phy(struct mii_bus *bus,
115 struct fwnode_handle *child, u32 addr)
116 {
117 struct mii_timestamper *mii_ts = NULL;
118 struct pse_control *psec = NULL;
119 struct phy_device *phy;
120 bool is_c45 = false;
121 u32 phy_id;
122 int rc;
123 int reset_deassert_delay = 0;
124 struct gpio_desc *reset_gpio;
125
126 psec = fwnode_find_pse_control(child);
127 if (IS_ERR(psec))
128 return PTR_ERR(psec);
129
130 mii_ts = fwnode_find_mii_timestamper(child);
131 if (IS_ERR(mii_ts)) {
132 rc = PTR_ERR(mii_ts);
133 goto clean_pse;
134 }
135
136 rc = fwnode_property_match_string(child, "compatible",
137 "ethernet-phy-ieee802.3-c45");
138 if (rc >= 0)
139 is_c45 = true;
140
141 reset_gpio = fwnode_gpiod_get_index(child, "reset", 0, GPIOD_OUT_LOW, "PHY reset");
142 if (reset_gpio == ERR_PTR(-EPROBE_DEFER)) {
143 dev_dbg(&bus->dev, "reset signal for PHY@%u not ready\n", addr);
144 return -EPROBE_DEFER;
145 } else if (IS_ERR(reset_gpio)) {
146 if (reset_gpio == ERR_PTR(-ENOENT))
147 dev_dbg(&bus->dev, "reset signal for PHY@%u not defined\n", addr);
148 else
149 dev_dbg(&bus->dev, "failed to request reset for PHY@%u, error %ld\n", addr, PTR_ERR(reset_gpio));
150 reset_gpio = NULL;
151 } else {
152 dev_dbg(&bus->dev, "deassert reset signal for PHY@%u\n", addr);
153 fwnode_property_read_u32(child, "reset-deassert-us",
154 &reset_deassert_delay);
155 if (reset_deassert_delay)
156 fsleep(reset_deassert_delay);
157 }
158
159 if (is_c45 || fwnode_get_phy_id(child, &phy_id))
160 phy = get_phy_device(bus, addr, is_c45);
161 else
162 phy = phy_device_create(bus, addr, phy_id, 0, NULL);
163
> 164 gpiochip_free_own_desc(reset_gpio);
--
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests
On Mon, Jan 16, 2023 at 12:55 AM Andrew Lunn <[email protected]> wrote:
> > Specifying the ID as part of the compatible string works for clause 22 PHYs,
> > but for clause 45 PHYs it does not work. The code always wants to read the
> > ID from the PHY itself. But I do not understand things well enough to tell
> > whether that's a fundamental restriction of C45 or just our implementation
> > and the implementation can be changed to fix it.
> >
> > Do you have some thoughts on this?
>
> Do you have more details about what goes wrong? Which PHY driver is
> it? What compatibles do you put into DT for the PHY?
>
We use both AR8033 and ADIN1300: these are 10/100/1000 PHYs.
They are both probed after the MDIO bus, but skipped if the reset was
asserted while probing the MDIO bus.
However, for iMX6UL and iMX7 we use C22, not C45.
>
> To some extent, the ID is only used to find the driver. A C45 device
> has a lot of ID registers, and all of them are used by phy_bus_match()
> to see if a driver matches. So you need to be careful which ID you
> pick, it needs to match the driver.
>
> It is the driver which decides to use C22 or C45 to talk to the PHY.
> However, we do have:
>
> static int phy_probe(struct device *dev)
> {
> ...
> else if (phydev->is_c45)
> err = genphy_c45_pma_read_abilities(phydev);
> else
> err = genphy_read_abilities(phydev);
>
> so it could be a C45 PHY probed using an ID does not have
> phydev->is_c45 set, and so it looks in the wrong place for the
> capabilities. Make sure you also have the compatible
> "ethernet-phy-ieee802.3-c45" which i think should cause is_c45 to be
> set.
>
> There is no fundamental restriction that i know of here, it probably
> just needs somebody to debug it and find where it goes wrong.
>
> Ah!
>
> int fwnode_mdiobus_register_phy(struct mii_bus *bus,
> struct fwnode_handle *child, u32 addr)
> {
> ...
> rc = fwnode_property_match_string(child, "compatible",
> "ethernet-phy-ieee802.3-c45");
> if (rc >= 0)
> is_c45 = true;
>
> if (is_c45 || fwnode_get_phy_id(child, &phy_id))
> phy = get_phy_device(bus, addr, is_c45);
> else
> phy = phy_device_create(bus, addr, phy_id, 0, NULL);
>
>
> So compatible "ethernet-phy-ieee802.3-c45" results in is_c45 being set
> true. The if (is_c45 || is then true, so it does not need to call
> fwnode_get_phy_id(child, &phy_id) so ignores whatever ID is in DT and
> asks the PHY.
>
> Try this, totally untested:
>
> diff --git a/drivers/net/mdio/fwnode_mdio.c b/drivers/net/mdio/fwnode_mdio.c
> index b782c35c4ac1..13be23f8ac97 100644
> --- a/drivers/net/mdio/fwnode_mdio.c
> +++ b/drivers/net/mdio/fwnode_mdio.c
> @@ -134,10 +134,10 @@ int fwnode_mdiobus_register_phy(struct mii_bus *bus,
> if (rc >= 0)
> is_c45 = true;
>
> - if (is_c45 || fwnode_get_phy_id(child, &phy_id))
> + if (fwnode_get_phy_id (child, &phy_id))
> phy = get_phy_device(bus, addr, is_c45);
> else
> - phy = phy_device_create(bus, addr, phy_id, 0, NULL);
> + phy = phy_device_create(bus, addr, phy_id, is_c45, NULL);
> if (IS_ERR(phy)) {
> rc = PTR_ERR(phy);
> goto clean_mii_ts;
>
>
> Andrew
Unfortunately the above doesn't change the condition: this problem is not C45 specific.
The call fwnode_get_phy_id just parses the device tree and always passes.
This is a sample device tree
https://github.com/varigit/linux-imx/blob/5.15-2.0.x-imx_var01/arch/arm64/boot/dts/freescale/imx8qm-var-spear.dtsi#L168-L219
On Mon, Jan 16, 2023 at 1:11 AM Andrew Lunn <[email protected]> wrote:
> > IMHO, since the framework allows defining the reset GPIO, it does not sound
> > reasonable to manage it only after checking if the PHY can communicate:
> > if the reset is asserted, the PHY cannot communicate at all.
> > This patch just ensures that, if the reset GPIO is defined, it's not asserted
> > while checking the communication.
>
> The problem is, you are only solving 1/4 of the problem. What about
> the clock the PHY needs? And the regulator, and the linux reset
> controller? And what order to do enable these, and how long do you
> wait between each one?
>
Interesting point of view: I was thinking about solving one of 4 problems ;)
This problem affects all the platforms using the reset GPIO without
ensuring that either u-boot or the HW put a pull-up on it.
In our test, the problem is reproducible simply setting the reset to 0 from
u-boot and then use the GPIO reset as designed in the MDIO framework.
Is this approach?reasonable or a comprehensive?solution is expected to
cover all additional HW actors (clocks, regulators, ...) ?>
> And why are you solving this purely for Ethernet PHYs when the same
> problem probably affects other sorts of devices which have reset
> GPIOs, regulators and clocks? It looks like MMC/SDIO devices have a
> similar problem.
>
> https://lwn.net/Articles/867740/
>
> As i said, i've not been following this work. Has it got anywhere? Can
> ethernet PHYs use it?
>
> ? ? ? ? ?Andrew
I'm not that familiar with the article's implications, but it sounds like a
partial redesign of the framework is needed.
I'm not sure this is the real point.
Let's refer to I2C/SPI/USB busses, the sequence is something like
- probe and setup the bus
- once the bus is up & running, start probing the connected slaves
Apparently, in the MDIO framework there's an excessive coupling
between the MDIO bus and the PHYs.
I can't really understand why the MDIO bus must check the PHY presence.
Other busses try the communication only while probing the slaves,
never before.
On Mon, Jan 16, 2023 at 9:44 AM kernel test robot <[email protected]> wrote:
> Hi Pierluigi,
>
> Thank you for the patch! Yet something to improve:
>
> [auto build test ERROR on net-next/master]
> [also build test ERROR on net/master linus/master v6.2-rc4 next-20230116]
> [If your patch is applied to the wrong git tree, kindly drop us a note.
> And when submitting patch, we suggest to use '--base' as documented in
> https://git-scm.com/docs/git-format-patch#_base_tree_information]
>
> url: https://github.com/intel-lab-lkp/linux/commits/Pierluigi-Passaro/net-mdio-force-deassert-MDIO-reset-signal/20230116-001044
> patch link: https://lore.kernel.org/r/20230115161006.16431-1-pierluigi.p%40variscite.com
> patch subject: [PATCH] net: mdio: force deassert MDIO reset signal
> config: nios2-defconfig
> compiler: nios2-linux-gcc (GCC) 12.1.0
> reproduce (this is a W=1 build):
> wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
> chmod +x ~/bin/make.cross
> # https://github.com/intel-lab-lkp/linux/commit/3f08f04af6947d4fce17b11443001c4e386ca66e
> git remote add linux-review https://github.com/intel-lab-lkp/linux
> git fetch --no-tags linux-review Pierluigi-Passaro/net-mdio-force-deassert-MDIO-reset-signal/20230116-001044
> git checkout 3f08f04af6947d4fce17b11443001c4e386ca66e
> # save the config file
> mkdir build_dir && cp config build_dir/.config
> COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-12.1.0 make.cross W=1 O=build_dir ARCH=nios2 olddefconfig
> COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-12.1.0 make.cross W=1 O=build_dir ARCH=nios2 SHELL=/bin/bash
>
> If you fix the issue, kindly add following tag where applicable
> | Reported-by: kernel test robot <[email protected]>
>
The config file used to build this kernel disables CONFIG_GPIOLIB.
Is this intentional ?
If yes, I suppose the patch should fix the code here
https://github.com/intel-lab-lkp/linux/blob/3f08f04af6947d4fce17b11443001c4e386ca66e/include/linux/gpio/driver.h#L761-L798
with something like
#ifdef CONFIG_GPIOLIB
...
struct gpio_desc *gpiochip_request_own_desc(struct gpio_chip *gc,
unsigned int hwnum,
const char *label,
enum gpio_lookup_flags lflags,
enum gpiod_flags dflags);
void gpiochip_free_own_desc(struct gpio_desc *desc);
#else
...
static inline struct gpio_desc *gpiochip_request_own_desc(struct gpio_chip *gc,
unsigned int hwnum,
const char *label,
enum gpio_lookup_flags lflags,
enum gpiod_flags dflags)
{
/* GPIO can never have been requested */
WARN_ON(1);
return ERR_PTR(-ENODEV);
}
static inline void gpiochip_free_own_desc(struct gpio_desc *desc)
{
WARN_ON(1);
}
#endif /* CONFIG_GPIOLIB */
Do you agree ?
>
> All errors (new ones prefixed by >>):
>
> nios2-linux-ld: drivers/net/mdio/fwnode_mdio.o: in function `fwnode_mdiobus_register_phy':
> >> drivers/net/mdio/fwnode_mdio.c:164: undefined reference to `gpiochip_free_own_desc'
> drivers/net/mdio/fwnode_mdio.c:164:(.text+0x230): relocation truncated to fit: R_NIOS2_CALL26 against `gpiochip_free_own_desc'
>
>
> vim +164 drivers/net/mdio/fwnode_mdio.c
>
> 113
> 114 int fwnode_mdiobus_register_phy(struct mii_bus *bus,
> 115 struct fwnode_handle *child, u32 addr)
> 116 {
> 117 struct mii_timestamper *mii_ts = NULL;
> 118 struct pse_control *psec = NULL;
> 119 struct phy_device *phy;
> 120 bool is_c45 = false;
> 121 u32 phy_id;
> 122 int rc;
> 123 int reset_deassert_delay = 0;
> 124 struct gpio_desc *reset_gpio;
> 125
> 126 psec = fwnode_find_pse_control(child);
> 127 if (IS_ERR(psec))
> 128 return PTR_ERR(psec);
> 129
> 130 mii_ts = fwnode_find_mii_timestamper(child);
> 131 if (IS_ERR(mii_ts)) {
> 132 rc = PTR_ERR(mii_ts);
> 133 goto clean_pse;
> 134 }
> 135
> 136 rc = fwnode_property_match_string(child, "compatible",
> 137 "ethernet-phy-ieee802.3-c45");
> 138 if (rc >= 0)
> 139 is_c45 = true;
> 140
> 141 reset_gpio = fwnode_gpiod_get_index(child, "reset", 0, GPIOD_OUT_LOW, "PHY reset");
> 142 if (reset_gpio == ERR_PTR(-EPROBE_DEFER)) {
> 143 dev_dbg(&bus->dev, "reset signal for PHY@%u not ready\n", addr);
> 144 return -EPROBE_DEFER;
> 145 } else if (IS_ERR(reset_gpio)) {
> 146 if (reset_gpio == ERR_PTR(-ENOENT))
> 147 dev_dbg(&bus->dev, "reset signal for PHY@%u not defined\n", addr);
> 148 else
> 149 dev_dbg(&bus->dev, "failed to request reset for PHY@%u, error %ld\n", addr, PTR_ERR(reset_gpio));
> 150 reset_gpio = NULL;
> 151 } else {
> 152 dev_dbg(&bus->dev, "deassert reset signal for PHY@%u\n", addr);
> 153 fwnode_property_read_u32(child, "reset-deassert-us",
> 154 &reset_deassert_delay);
> 155 if (reset_deassert_delay)
> 156 fsleep(reset_deassert_delay);
> 157 }
> 158
> 159 if (is_c45 || fwnode_get_phy_id(child, &phy_id))
> 160 phy = get_phy_device(bus, addr, is_c45);
> 161 else
> 162 phy = phy_device_create(bus, addr, phy_id, 0, NULL);
> 163
> > 164 gpiochip_free_own_desc(reset_gpio);
>
> --
> 0-DAY CI Kernel Test Service
> https://github.com/intel/lkp-tests
On Mon, Jan 16, 2023 at 3:41 AM Lars-Peter Clausen <[email protected]> wrote:
> On 1/15/23 15:55, Andrew Lunn wrote:
> >> Specifying the ID as part of the compatible string works for clause 22 PHYs,
> >> but for clause 45 PHYs it does not work. The code always wants to read the
> >> ID from the PHY itself. But I do not understand things well enough to tell
> >> whether that's a fundamental restriction of C45 or just our implementation
> >> and the implementation can be changed to fix it.
> >>
> >> Do you have some thoughts on this?
> > Do you have more details about what goes wrong? Which PHY driver is
> > it? What compatibles do you put into DT for the PHY?
> >
> > To some extent, the ID is only used to find the driver. A C45 device
> > has a lot of ID register, and all of them are used by phy_bus_match()
> > to see if a driver matches. So you need to be careful which ID you
> > pick, it needs to match the driver.
> >
> > It is the driver which decides to use C22 or C45 to talk to the PHY.
> > However, we do have:
> >
> > static int phy_probe(struct device *dev)
> > {
> > ...
> > ? ? ? ? ?else if (phydev->is_c45)
> > ? ? ? ? ? ? ? ? ?err = genphy_c45_pma_read_abilities(phydev);
> > ? ? ? ? ?else
> > ? ? ? ? ? ? ? ? ?err = genphy_read_abilities(phydev);
> >
> > so it could be a C45 PHY probed using an ID does not have
> > phydev->is_c45 set, and so it looks in the wrong place for the
> > capabilities. Make sure you also have the compatible
> > "ethernet-phy-ieee802.3-c45" which i think should cause is_c45 to be
> > set.
> >
> > There is no fundamental restriction that i know of here, it probably
> > just needs somebody to debug it and find where it goes wrong.
> >
> > Ah!
> >
> > int fwnode_mdiobus_register_phy(struct mii_bus *bus,
> > ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?struct fwnode_handle *child, u32 addr)
> > {
> > ...
> > ? ? ? ? ?rc = fwnode_property_match_string(child, "compatible",
> > ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?"ethernet-phy-ieee802.3-c45");
> > ? ? ? ? ?if (rc >= 0)
> > ? ? ? ? ? ? ? ? ?is_c45 = true;
> >
> > ? ? ? ? ?if (is_c45 || fwnode_get_phy_id(child, &phy_id))
> > ? ? ? ? ? ? ? ? ?phy = get_phy_device(bus, addr, is_c45);
> > ? ? ? ? ?else
> > ? ? ? ? ? ? ? ? ?phy = phy_device_create(bus, addr, phy_id, 0, NULL);
> >
> >
> > So compatible "ethernet-phy-ieee802.3-c45" results in is_c45 being set
> > true. The if (is_c45 || is then true, so it does not need to call
> > fwnode_get_phy_id(child, &phy_id) so ignores whatever ID is in DT and
> > asks the PHY.
> >
> > Try this, totally untested:
> >
> > diff --git a/drivers/net/mdio/fwnode_mdio.c b/drivers/net/mdio/fwnode_mdio.c
> > index b782c35c4ac1..13be23f8ac97 100644
> > --- a/drivers/net/mdio/fwnode_mdio.c
> > +++ b/drivers/net/mdio/fwnode_mdio.c
> > @@ -134,10 +134,10 @@ int fwnode_mdiobus_register_phy(struct mii_bus *bus,
> > ? ? ? ? ?if (rc >= 0)
> > ? ? ? ? ? ? ? ? ?is_c45 = true;
> > ?
> > - ? ? ? if (is_c45 || fwnode_get_phy_id(child, &phy_id))
> > + ? ? ? if (fwnode_get_phy_id (child, &phy_id))
> > ? ? ? ? ? ? ? ? ?phy = get_phy_device(bus, addr, is_c45);
> > ? ? ? ? ?else
> > - ? ? ? ? ? ? ? phy = phy_device_create(bus, addr, phy_id, 0, NULL);
> > + ? ? ? ? ? ? ? phy = phy_device_create(bus, addr, phy_id, is_c45, NULL);
> > ? ? ? ? ?if (IS_ERR(phy)) {
> > ? ? ? ? ? ? ? ? ?rc = PTR_ERR(phy);
> > ? ? ? ? ? ? ? ? ?goto clean_mii_ts;
> >
> I think part of the problem is that for C45 there are a few other fields
> that get populated by the ID detection, such as devices_in_package and
> mmds_present. Is this something we can do after running the PHY drivers
> probe function? Or is it too late at that point?
>
Unfortunately the above doesn't change the condition: this problem
is not C45 specific.
The call fwnode_get_phy_id just parses the device tree and always
passes.
This is a sample device tree
https://github.com/varigit/linux-imx/blob/5.15-2.0.x-imx_var01/arch/arm64/boot/dts/freescale/imx8qm-var-spear.dtsi#L168-L219
On Mon, Jan 16, 2023 at 10:32:38AM +0000, Pierluigi Passaro wrote:
> The config file used to build this kernel disables CONFIG_GPIOLIB.
> Is this intentional ?
All configurations should build - and the build bot tries random
configurations. Your job is to fix the errors / warnings that it
finds as a result of your patch and submit fixes where appropriate.
Don't expect the build bot to provide any guidance or answer questions.
--
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!
On Mon, Jan 16, 2023 at 09:44:01AM +0000, Pierluigi Passaro wrote:
> I can't really understand why the MDIO bus must check the PHY presence.
> Other busses try the communication only while probing the slaves,
> never before.
That is not true.
Any bus that relies on hardware identification to bind drivers reads
the identifying information prior to probing. For example, PCI, USB,
AMBA to name a few. AMBA didn't used to have this problem, but does
now - but at least under AMBA there's a bit more standardisation to
what needs to be done. With ethernet PHYs, what's needed is highly
platform specific.
--
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!
On Mon, Jan 16, 2023 at 9:44 AM kernel test robot <[email protected]> wrote:
> Hi Pierluigi,
>
> Thank you for the patch! Yet something to improve:
>
> [auto build test ERROR on net-next/master]
> [also build test ERROR on net/master linus/master v6.2-rc4 next-20230116]
> [If your patch is applied to the wrong git tree, kindly drop us a note.
> And when submitting patch, we suggest to use '--base' as documented in
> https://git-scm.com/docs/git-format-patch#_base_tree_information]
>
> url: ? ?https://github.com/intel-lab-lkp/linux/commits/Pierluigi-Passaro/net-mdio-force-deassert-MDIO-reset-signal/20230116-001044
> patch link: ? ?https://lore.kernel.org/r/20230115161006.16431-1-pierluigi.p%40variscite.com
> patch subject: [PATCH] net: mdio: force deassert MDIO reset signal
> config: nios2-defconfig
> compiler: nios2-linux-gcc (GCC) 12.1.0
> reproduce (this is a W=1 build):
> ? ? ? ? wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
> ? ? ? ? chmod +x ~/bin/make.cross
> ? ? ? ? # https://github.com/intel-lab-lkp/linux/commit/3f08f04af6947d4fce17b11443001c4e386ca66e
> ? ? ? ? git remote add linux-review https://github.com/intel-lab-lkp/linux
> ? ? ? ? git fetch --no-tags linux-review Pierluigi-Passaro/net-mdio-force-deassert-MDIO-reset-signal/20230116-001044
> ? ? ? ? git checkout 3f08f04af6947d4fce17b11443001c4e386ca66e
> ? ? ? ? # save the config file
> ? ? ? ? mkdir build_dir && cp config build_dir/.config
> ? ? ? ? COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-12.1.0 make.cross W=1 O=build_dir ARCH=nios2 olddefconfig
> ? ? ? ? COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-12.1.0 make.cross W=1 O=build_dir ARCH=nios2 SHELL=/bin/bash
>
> If you fix the issue, kindly add following tag where applicable
> | Reported-by: kernel test robot <[email protected]>
>
patch available here
https://lore.kernel.org/all/[email protected]/
>
> All errors (new ones prefixed by >>):
>
> ? ?nios2-linux-ld: drivers/net/mdio/fwnode_mdio.o: in function `fwnode_mdiobus_register_phy':
> >> drivers/net/mdio/fwnode_mdio.c:164: undefined reference to `gpiochip_free_own_desc'
> ? ?drivers/net/mdio/fwnode_mdio.c:164:(.text+0x230): relocation truncated to fit: R_NIOS2_CALL26 against `gpiochip_free_own_desc'
>
>
> vim +164 drivers/net/mdio/fwnode_mdio.c
>
> ? ?113
> ? ?114 ?int fwnode_mdiobus_register_phy(struct mii_bus *bus,
> ? ?115 ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?struct fwnode_handle *child, u32 addr)
> ? ?116 ?{
> ? ?117 ? ? ? ? ?struct mii_timestamper *mii_ts = NULL;
> ? ?118 ? ? ? ? ?struct pse_control *psec = NULL;
> ? ?119 ? ? ? ? ?struct phy_device *phy;
> ? ?120 ? ? ? ? ?bool is_c45 = false;
> ? ?121 ? ? ? ? ?u32 phy_id;
> ? ?122 ? ? ? ? ?int rc;
> ? ?123 ? ? ? ? ?int reset_deassert_delay = 0;
> ? ?124 ? ? ? ? ?struct gpio_desc *reset_gpio;
> ? ?125
> ? ?126 ? ? ? ? ?psec = fwnode_find_pse_control(child);
> ? ?127 ? ? ? ? ?if (IS_ERR(psec))
> ? ?128 ? ? ? ? ? ? ? ? ?return PTR_ERR(psec);
> ? ?129
> ? ?130 ? ? ? ? ?mii_ts = fwnode_find_mii_timestamper(child);
> ? ?131 ? ? ? ? ?if (IS_ERR(mii_ts)) {
> ? ?132 ? ? ? ? ? ? ? ? ?rc = PTR_ERR(mii_ts);
> ? ?133 ? ? ? ? ? ? ? ? ?goto clean_pse;
> ? ?134 ? ? ? ? ?}
> ? ?135
> ? ?136 ? ? ? ? ?rc = fwnode_property_match_string(child, "compatible",
> ? ?137 ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?"ethernet-phy-ieee802.3-c45");
> ? ?138 ? ? ? ? ?if (rc >= 0)
> ? ?139 ? ? ? ? ? ? ? ? ?is_c45 = true;
> ? ?140
> ? ?141 ? ? ? ? ?reset_gpio = fwnode_gpiod_get_index(child, "reset", 0, GPIOD_OUT_LOW, "PHY reset");
> ? ?142 ? ? ? ? ?if (reset_gpio == ERR_PTR(-EPROBE_DEFER)) {
> ? ?143 ? ? ? ? ? ? ? ? ?dev_dbg(&bus->dev, "reset signal for PHY@%u not ready\n", addr);
> ? ?144 ? ? ? ? ? ? ? ? ?return -EPROBE_DEFER;
> ? ?145 ? ? ? ? ?} else if (IS_ERR(reset_gpio)) {
> ? ?146 ? ? ? ? ? ? ? ? ?if (reset_gpio == ERR_PTR(-ENOENT))
> ? ?147 ? ? ? ? ? ? ? ? ? ? ? ? ?dev_dbg(&bus->dev, "reset signal for PHY@%u not defined\n", addr);
> ? ?148 ? ? ? ? ? ? ? ? ?else
> ? ?149 ? ? ? ? ? ? ? ? ? ? ? ? ?dev_dbg(&bus->dev, "failed to request reset for PHY@%u, error %ld\n", addr, PTR_ERR(reset_gpio));
> ? ?150 ? ? ? ? ? ? ? ? ?reset_gpio = NULL;
> ? ?151 ? ? ? ? ?} else {
> ? ?152 ? ? ? ? ? ? ? ? ?dev_dbg(&bus->dev, "deassert reset signal for PHY@%u\n", addr);
> ? ?153 ? ? ? ? ? ? ? ? ?fwnode_property_read_u32(child, "reset-deassert-us",
> ? ?154 ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? &reset_deassert_delay);
> ? ?155 ? ? ? ? ? ? ? ? ?if (reset_deassert_delay)
> ? ?156 ? ? ? ? ? ? ? ? ? ? ? ? ?fsleep(reset_deassert_delay);
> ? ?157 ? ? ? ? ?}
> ? ?158
> ? ?159 ? ? ? ? ?if (is_c45 || fwnode_get_phy_id(child, &phy_id))
> ? ?160 ? ? ? ? ? ? ? ? ?phy = get_phy_device(bus, addr, is_c45);
> ? ?161 ? ? ? ? ?else
> ? ?162 ? ? ? ? ? ? ? ? ?phy = phy_device_create(bus, addr, phy_id, 0, NULL);
> ? ?163
> ?> 164 ? ? ? ? ?gpiochip_free_own_desc(reset_gpio);
>
> --
> 0-DAY CI Kernel Test Service
> https://github.com/intel/lkp-tests
> > So compatible "ethernet-phy-ieee802.3-c45" results in is_c45 being set
> > true. The if (is_c45 || is then true, so it does not need to call
> > fwnode_get_phy_id(child, &phy_id) so ignores whatever ID is in DT and
> > asks the PHY.
> >
> > Try this, totally untested:
> >
> > diff --git a/drivers/net/mdio/fwnode_mdio.c b/drivers/net/mdio/fwnode_mdio.c
> > index b782c35c4ac1..13be23f8ac97 100644
> > --- a/drivers/net/mdio/fwnode_mdio.c
> > +++ b/drivers/net/mdio/fwnode_mdio.c
> > @@ -134,10 +134,10 @@ int fwnode_mdiobus_register_phy(struct mii_bus *bus,
> > if (rc >= 0)
> > is_c45 = true;
> > - if (is_c45 || fwnode_get_phy_id(child, &phy_id))
> > + if (fwnode_get_phy_id (child, &phy_id))
> > phy = get_phy_device(bus, addr, is_c45);
> > else
> > - phy = phy_device_create(bus, addr, phy_id, 0, NULL);
> > + phy = phy_device_create(bus, addr, phy_id, is_c45, NULL);
> > if (IS_ERR(phy)) {
> > rc = PTR_ERR(phy);
> > goto clean_mii_ts;
> >
> I think part of the problem is that for C45 there are a few other fields
> that get populated by the ID detection, such as devices_in_package and
> mmds_present. Is this something we can do after running the PHY drivers
> probe function? Or is it too late at that point?
As i hinted, it needs somebody to actually debug this and figure out
why it does not work.
I think what i did above is part of the solution. You need to actually
read the ID from the DT, which if you never call fwnode_get_phy_id()
you never do.
You then need to look at phy_bus_match() and probably remove the
return 0;
} else {
so that C22 style ID matching is performed if matching via
c45_ids.device_ids fails.
Andrew
On Mon, Jan 16, 2023 at 08:39:31AM +0000, Pierluigi Passaro wrote:
> On Mon, Jan 16, 2023 at 12:55 AM Andrew Lunn <[email protected]> wrote:
> > > Specifying the ID as part of the compatible string works for clause 22 PHYs,
> > > but for clause 45 PHYs it does not work. The code always wants to read the
> > > ID from the PHY itself. But I do not understand things well enough to tell
> > > whether that's a fundamental restriction of C45 or just our implementation
> > > and the implementation can be changed to fix it.
> > >
> > > Do you have some thoughts on this?
> >
> > Do you have more details about what goes wrong? Which PHY driver is
> > it? What compatibles do you put into DT for the PHY?
> >
> We use both AR8033 and ADIN1300: these are 10/100/1000 PHYs.
> They are both probed after the MDIO bus, but skipped if the reset was
> asserted while probing the MDIO bus.
> However, for iMX6UL and iMX7 we use C22, not C45.
I never said it did. Please read the actual emails, then you would of
noticed we have go off on tangent and are trying to fix another issue.
Andrew
On 1/17/23 05:40, Andrew Lunn wrote:
>>> So compatible "ethernet-phy-ieee802.3-c45" results in is_c45 being set
>>> true. The if (is_c45 || is then true, so it does not need to call
>>> fwnode_get_phy_id(child, &phy_id) so ignores whatever ID is in DT and
>>> asks the PHY.
>>>
>>> Try this, totally untested:
>>>
>>> diff --git a/drivers/net/mdio/fwnode_mdio.c b/drivers/net/mdio/fwnode_mdio.c
>>> index b782c35c4ac1..13be23f8ac97 100644
>>> --- a/drivers/net/mdio/fwnode_mdio.c
>>> +++ b/drivers/net/mdio/fwnode_mdio.c
>>> @@ -134,10 +134,10 @@ int fwnode_mdiobus_register_phy(struct mii_bus *bus,
>>> if (rc >= 0)
>>> is_c45 = true;
>>> - if (is_c45 || fwnode_get_phy_id(child, &phy_id))
>>> + if (fwnode_get_phy_id (child, &phy_id))
>>> phy = get_phy_device(bus, addr, is_c45);
>>> else
>>> - phy = phy_device_create(bus, addr, phy_id, 0, NULL);
>>> + phy = phy_device_create(bus, addr, phy_id, is_c45, NULL);
>>> if (IS_ERR(phy)) {
>>> rc = PTR_ERR(phy);
>>> goto clean_mii_ts;
>>>
>> I think part of the problem is that for C45 there are a few other fields
>> that get populated by the ID detection, such as devices_in_package and
>> mmds_present. Is this something we can do after running the PHY drivers
>> probe function? Or is it too late at that point?
> As i hinted, it needs somebody to actually debug this and figure out
> why it does not work.
>
> I think what i did above is part of the solution. You need to actually
> read the ID from the DT, which if you never call fwnode_get_phy_id()
> you never do.
>
> You then need to look at phy_bus_match() and probably remove the
>
> return 0;
> } else {
>
> so that C22 style ID matching is performed if matching via
> c45_ids.device_ids fails.
Sorry, I've should have been more clear. I did try your proposed change
a while ago. The problem why it does not work is that there are other
fields in the phy data structure that get initialized when reading the
IDs for C45. Such as which MMD addresses are valid.
> > > I think part of the problem is that for C45 there are a few other fields
> > > that get populated by the ID detection, such as devices_in_package and
> > > mmds_present. Is this something we can do after running the PHY drivers
> > > probe function? Or is it too late at that point?
> > As i hinted, it needs somebody to actually debug this and figure out
> > why it does not work.
> >
> > I think what i did above is part of the solution. You need to actually
> > read the ID from the DT, which if you never call fwnode_get_phy_id()
> > you never do.
> >
> > You then need to look at phy_bus_match() and probably remove the
> >
> > return 0;
> > } else {
> >
> > so that C22 style ID matching is performed if matching via
> > c45_ids.device_ids fails.
>
> Sorry, I've should have been more clear. I did try your proposed change a
> while ago. The problem why it does not work is that there are other fields
> in the phy data structure that get initialized when reading the IDs for C45.
> Such as which MMD addresses are valid.
So lets take this one step at a time.
Does this change at least get the driver loaded?
There is some code in phy-c45.c which needs
phydev->c45_ids.mmds_present. So maybe after the driver has probed,
and the device should be accessible, we need to call get_phy_c45_ids()
to fill in the missing IDs?
Andrew
On Mon, Jan 16, 2023 at 09:44:01AM +0000, Pierluigi Passaro wrote:
> On Mon, Jan 16, 2023 at 1:11 AM Andrew Lunn <[email protected]> wrote:
> > > IMHO, since the framework allows defining the reset GPIO, it does not sound
> > > reasonable to manage it only after checking if the PHY can communicate:
> > > if the reset is asserted, the PHY cannot communicate at all.
> > > This patch just ensures that, if the reset GPIO is defined, it's not asserted
> > > while checking the communication.
> >
> > The problem is, you are only solving 1/4 of the problem. What about
> > the clock the PHY needs? And the regulator, and the linux reset
> > controller? And what order to do enable these, and how long do you
> > wait between each one?
> >
> Interesting point of view: I was thinking about solving one of 4 problems ;)
Lots of small incremental 'improvements' sometimes get you into a real
mess because you loose track of the big picture. And i do think we are
now in a mess. But i also think we have a better understanding of the
problem space. We know there can be arbitrate number of resources
which need to be enabled before you can enumerate the bus. We need a
generic solution to that problem. And Linux is good at solving a
problem once and reusing it other places. So the generic solution
should be applicable to other bus types.
We also have a well understood workaround, put the IDs in DT. So as
far as i'm concerned we don't need to add more incremental
'improvements', we can wait for somebody to put in the effort to solve
this properly with generic code.
So i don't want to merge this change. Sorry.
Andrew
On Tue, Jan 17, 2023 at 3:01 PM Andrew Lunn <[email protected]> wrote:
> On Mon, Jan 16, 2023 at 09:44:01AM +0000, Pierluigi Passaro wrote:
> > On Mon, Jan 16, 2023 at 1:11 AM Andrew Lunn <[email protected]> wrote:
> > > > IMHO, since the framework allows defining the reset GPIO, it does not sound
> > > > reasonable to manage it only after checking if the PHY can communicate:
> > > > if the reset is asserted, the PHY cannot communicate at all.
> > > > This patch just ensures that, if the reset GPIO is defined, it's not asserted
> > > > while checking the communication.
> > >
> > > The problem is, you are only solving 1/4 of the problem. What about
> > > the clock the PHY needs? And the regulator, and the linux reset
> > > controller? And what order to do enable these, and how long do you
> > > wait between each one?
> > >
> > Interesting point of view: I was thinking about solving one of 4 problems ;)
>
> Lots of small incremental 'improvements' sometimes get you into a real
> mess because you loose track of the big picture. And i do think we are
> now in a mess. But i also think we have a better understanding of the
> problem space. We know there can be arbitrate number of resources
> which need to be enabled before you can enumerate the bus. We need a
> generic solution to that problem. And Linux is good at solving a
> problem once and reusing it other places. So the generic solution
> should be applicable to other bus types.
>
> We also have a well understood workaround, put the IDs in DT. So as
> far as i'm concerned we don't need to add more incremental
> 'improvements', we can wait for somebody to put in the effort to solve
> this properly with generic code.
>
> So i don't want to merge this change. Sorry.
>
> ? ? ? ? Andrew
Hi Andrew,
I can understand your position and I apologize for the mess.
Thanks
Pier
Hi Pierluigi,
https://git-scm.com/docs/git-format-patch#_base_tree_information]
url: https://github.com/intel-lab-lkp/linux/commits/Pierluigi-Passaro/net-mdio-force-deassert-MDIO-reset-signal/20230116-001044
patch link: https://lore.kernel.org/r/20230115161006.16431-1-pierluigi.p%40variscite.com
patch subject: [PATCH] net: mdio: force deassert MDIO reset signal
config: xtensa-randconfig-m041-20230116
compiler: xtensa-linux-gcc (GCC) 12.1.0
If you fix the issue, kindly add following tag where applicable
| Reported-by: kernel test robot <[email protected]>
| Reported-by: Dan Carpenter <[email protected]>
smatch warnings:
drivers/net/mdio/fwnode_mdio.c:144 fwnode_mdiobus_register_phy() warn: missing unwind goto?
vim +144 drivers/net/mdio/fwnode_mdio.c
bc1bee3b87ee48 Calvin Johnson 2021-06-11 114 int fwnode_mdiobus_register_phy(struct mii_bus *bus,
bc1bee3b87ee48 Calvin Johnson 2021-06-11 115 struct fwnode_handle *child, u32 addr)
bc1bee3b87ee48 Calvin Johnson 2021-06-11 116 {
bc1bee3b87ee48 Calvin Johnson 2021-06-11 117 struct mii_timestamper *mii_ts = NULL;
5e82147de1cbd7 Oleksij Rempel 2022-10-03 118 struct pse_control *psec = NULL;
bc1bee3b87ee48 Calvin Johnson 2021-06-11 119 struct phy_device *phy;
bc1bee3b87ee48 Calvin Johnson 2021-06-11 120 bool is_c45 = false;
bc1bee3b87ee48 Calvin Johnson 2021-06-11 121 u32 phy_id;
bc1bee3b87ee48 Calvin Johnson 2021-06-11 122 int rc;
3f08f04af6947d Pierluigi Passaro 2023-01-15 123 int reset_deassert_delay = 0;
3f08f04af6947d Pierluigi Passaro 2023-01-15 124 struct gpio_desc *reset_gpio;
bc1bee3b87ee48 Calvin Johnson 2021-06-11 125
5e82147de1cbd7 Oleksij Rempel 2022-10-03 126 psec = fwnode_find_pse_control(child);
5e82147de1cbd7 Oleksij Rempel 2022-10-03 127 if (IS_ERR(psec))
5e82147de1cbd7 Oleksij Rempel 2022-10-03 128 return PTR_ERR(psec);
5e82147de1cbd7 Oleksij Rempel 2022-10-03 129
bc1bee3b87ee48 Calvin Johnson 2021-06-11 130 mii_ts = fwnode_find_mii_timestamper(child);
5e82147de1cbd7 Oleksij Rempel 2022-10-03 131 if (IS_ERR(mii_ts)) {
5e82147de1cbd7 Oleksij Rempel 2022-10-03 132 rc = PTR_ERR(mii_ts);
5e82147de1cbd7 Oleksij Rempel 2022-10-03 133 goto clean_pse;
^^^^^^^^^^^^^^^
5e82147de1cbd7 Oleksij Rempel 2022-10-03 134 }
bc1bee3b87ee48 Calvin Johnson 2021-06-11 135
bc1bee3b87ee48 Calvin Johnson 2021-06-11 136 rc = fwnode_property_match_string(child, "compatible",
bc1bee3b87ee48 Calvin Johnson 2021-06-11 137 "ethernet-phy-ieee802.3-c45");
bc1bee3b87ee48 Calvin Johnson 2021-06-11 138 if (rc >= 0)
bc1bee3b87ee48 Calvin Johnson 2021-06-11 139 is_c45 = true;
bc1bee3b87ee48 Calvin Johnson 2021-06-11 140
3f08f04af6947d Pierluigi Passaro 2023-01-15 141 reset_gpio = fwnode_gpiod_get_index(child, "reset", 0, GPIOD_OUT_LOW, "PHY reset");
3f08f04af6947d Pierluigi Passaro 2023-01-15 142 if (reset_gpio == ERR_PTR(-EPROBE_DEFER)) {
3f08f04af6947d Pierluigi Passaro 2023-01-15 143 dev_dbg(&bus->dev, "reset signal for PHY@%u not ready\n", addr);
3f08f04af6947d Pierluigi Passaro 2023-01-15 @144 return -EPROBE_DEFER;
^^^^^^^^^^^^^^^^^^^^
Looks like there needs to be some clean up done before the return.
(Sometimes the kbuild-bot doesn't include the whole function in these
emails. I'm not sure what the rules are.).
--
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests