Re: MDIO over I2C driver driver probe dependency issue

Re: MDIO over I2C driver driver probe dependency issue

[Florian Fainelli]

On 1/7/2021 6:22 PM, Brian Silverman wrote:
> I've written a very small generic MDIO driver that uses the existing
> mdio-i2c.c library in drivers/net/phy.  The driver allows
> communication to the PHY's MDIO interface as using I2C, as supported by
> PHYs like the BCM54616S.  This is working on my hardware.  
> 
> The one issue I have is that I2C is not up and available (i.e. probed)
> at the time that the MDIO interface comes up.  To fix this, I've changed
> the device order in drivers/Makefile to put "obj-y += i2c/"
> before "obj-y += net/".
> 
> While that works, I prefer not to have to keep that difference from
> mainline Linux.  Also, I don't understand why i2c drivers occur
> arbitrarily late in the Makefile - surely there are other devices
> drivers that need i2c to be enabled when they are probed?
> 
> Is there a way to do this that doesn't change probe order?  Or is there
> a way to change probe order without patching mainline Linux?

Linux supports probe deferral so when a consumer of a resource finds
that said resource's provider is not available, it should return
-EPROBE_DEFER which puts the driver's probe routine onto a list of
driver's probe function to retry at a later time.

In your case the GEM Ethernet driver should get an -EPROBE_DEFER while
the Ethernet PHY device tree node is looked up via
phylink_of_phy_connect() because the mdio-i2c-gen i2c client has not had
a chance to register the MDIO bus yet. Have you figured out the call
path that does not work for you?

Which version of the kernel are you using? What I am referring to is
assuming mainline, but maybe this is not your case?

> 
> 
> ---
> 
> For reference, here's the driver (excluding headers and footers):
> 
> static int mdio_i2c_gen_probe(struct i2c_client *client,
>     const struct i2c_device_id *id)
> {
> struct mii_bus *bus;
> 
> bus = mdio_i2c_alloc(&client->dev, client->adapter);
> if (IS_ERR(bus)){
> return PTR_ERR(bus);
> }
> bus->name = "Generic MDIO bus over I2C";
> bus->parent = &client->dev;
> 
> return of_mdiobus_register(bus, client->dev.of_node);
> }
> 
> static int mdio_i2c_gen_remove(struct i2c_client *client)
> {
> return 0;
> }
> 
> static const struct of_device_id mdio_i2c_gen_of_match[] = {
> { .compatible = "mdio-i2c-gen", },
> { }
> };
> MODULE_DEVICE_TABLE(of, mdio_i2c_gen_of_match);
> 
> static struct i2c_driver mdio_i2c_gen_driver = {
> .driver = {
> .name= "mdio-i2c-gen",
> .of_match_table = of_match_ptr(mdio_i2c_gen_of_match),
> },
> .probe= mdio_i2c_gen_probe,
> .remove= mdio_i2c_gen_remove,
> };
> 
> module_i2c_driver(mdio_i2c_gen_driver);
> 
> 
> ---
> 
> And here's a device-tree snippet:
> 
> &gem3 {
>     status = "okay";
>     phy-handle =  <&phy0>;
> };
> 
> &i2c0 {
>     mdio@40 {
>         compatible = "mdio-i2c-gen";
>         reg = <0x40>;
>         #address-cells = <1>;
>         #size-cells = <0>;
> 
>         phy0: phy@0 {
>             reg = <0>;
>         };
>     };
> };
> 
> 
> 

-- 
Florian

Re: MDIO over I2C driver driver probe dependency issue

[Andrew Lunn]

On Thu, Jan 07, 2021 at 07:05:21PM -0800, Florian Fainelli wrote:
> 
> 
> On 1/7/2021 6:22 PM, Brian Silverman wrote:
> > I've written a very small generic MDIO driver that uses the existing
> > mdio-i2c.c library in drivers/net/phy.  The driver allows
> > communication to the PHY's MDIO interface as using I2C, as supported by
> > PHYs like the BCM54616S.  This is working on my hardware.  
> > 
> > The one issue I have is that I2C is not up and available (i.e. probed)
> > at the time that the MDIO interface comes up.  To fix this, I've changed
> > the device order in drivers/Makefile to put "obj-y += i2c/"
> > before "obj-y += net/".
> > 
> > While that works, I prefer not to have to keep that difference from
> > mainline Linux.  Also, I don't understand why i2c drivers occur
> > arbitrarily late in the Makefile - surely there are other devices
> > drivers that need i2c to be enabled when they are probed?
> > 
> > Is there a way to do this that doesn't change probe order?  Or is there
> > a way to change probe order without patching mainline Linux?
> 
> Linux supports probe deferral so when a consumer of a resource finds
> that said resource's provider is not available, it should return
> -EPROBE_DEFER which puts the driver's probe routine onto a list of
> driver's probe function to retry at a later time.
> 
> In your case the GEM Ethernet driver should get an -EPROBE_DEFER while
> the Ethernet PHY device tree node is looked up via
> phylink_of_phy_connect() because the mdio-i2c-gen i2c client has not had
> a chance to register the MDIO bus yet. Have you figured out the call
> path that does not work for you?

Just adding to this. The way the current mdio-i2c code is used, we
have already talked to the SFP to get its EEPROM contents. So we know
I2C works at the time the mdio-i2c bus is instantiated.

You are using the code slightly differently which might be why the
code is not correctly handling EPROBE_DEFER where it should. Patches
to add this are likely to be accepted.

   Andrew

Re: MDIO over I2C driver driver probe dependency issue

[Andrew Lunn]

On Fri, Jan 08, 2021 at 03:02:52PM -0500, Brian Silverman wrote:
> Thanks for the responses - I now have a more clear picture of what's going on.
>  (Note: I'm using Xilinx's 2019.2 kernel (based off 4.19).  I believe it would
> be similar to latest kernels, but I could be wrong.)

Hi Brian

macb_main has had a lot of changes with respect to PHYs. Please try
something modern, like 5.10.

	  Andrew

Re: MDIO over I2C driver driver probe dependency issue

[Florian Fainelli]

On 1/8/2021 1:04 PM, Andrew Lunn wrote:
> On Fri, Jan 08, 2021 at 03:02:52PM -0500, Brian Silverman wrote:
>> Thanks for the responses - I now have a more clear picture of what's going on.
>>  (Note: I'm using Xilinx's 2019.2 kernel (based off 4.19).  I believe it would
>> be similar to latest kernels, but I could be wrong.)
> 
> Hi Brian
> 
> macb_main has had a lot of changes with respect to PHYs. Please try
> something modern, like 5.10.

It does not seem to me like 5.10 will be much better, because we have
the following in PHYLINK:

int phylink_of_phy_connect(struct phylink *pl, struct device_node *dn,
                             u32 flags)
...
          phy_dev = of_phy_find_device(phy_node);
          /* We're done with the phy_node handle */
          of_node_put(phy_node);
          if (!phy_dev)
                  return -ENODEV;

Given Brian's configuration we should be returning -EPROBE_DEFER here,
but doing that would likely break a number of systems that do expect
-ENODEV to be returned. However there may be hope with fw_devlink to
create an appropriate graph of probing orders and solve the
consumer/provider order generically.

Up until now we did not really have a situation like this one where the
MDIO/PHY subsystem depended upon an another one to be available. The
problem does exist, however it is not clear to me yet how to best solve it.
-- 
Florian

Re: MDIO over I2C driver driver probe dependency issue

[Brian Silverman]

Thanks - I will have to keep a local patch to fix this for my needs
for the time being.

If in the future if I'm able to do something better (e.g. with a
better fw_devlink, which looks interesting), I'll post a patch.  But I
generally lag the latest kernels by a couple years, so...  not any
time soon.

<and hopefully this time my post doesn't bounce as I've turned off HTML>



On Fri, Jan 8, 2021 at 5:11 PM Florian Fainelli <f.fainelli@gmail.com> wrote:
>
>
>
> On 1/8/2021 1:04 PM, Andrew Lunn wrote:
> > On Fri, Jan 08, 2021 at 03:02:52PM -0500, Brian Silverman wrote:
> >> Thanks for the responses - I now have a more clear picture of what's going on.
> >>  (Note: I'm using Xilinx's 2019.2 kernel (based off 4.19).  I believe it would
> >> be similar to latest kernels, but I could be wrong.)
> >
> > Hi Brian
> >
> > macb_main has had a lot of changes with respect to PHYs. Please try
> > something modern, like 5.10.
>
> It does not seem to me like 5.10 will be much better, because we have
> the following in PHYLINK:
>
> int phylink_of_phy_connect(struct phylink *pl, struct device_node *dn,
>                              u32 flags)
> ...
>           phy_dev = of_phy_find_device(phy_node);
>           /* We're done with the phy_node handle */
>           of_node_put(phy_node);
>           if (!phy_dev)
>                   return -ENODEV;
>
> Given Brian's configuration we should be returning -EPROBE_DEFER here,
> but doing that would likely break a number of systems that do expect
> -ENODEV to be returned. However there may be hope with fw_devlink to
> create an appropriate graph of probing orders and solve the
> consumer/provider order generically.
>
> Up until now we did not really have a situation like this one where the
> MDIO/PHY subsystem depended upon an another one to be available. The
> problem does exist, however it is not clear to me yet how to best solve it.
> --
> Florian

Re: MDIO over I2C driver driver probe dependency issue

[Andrew Lunn]

On Fri, Jan 08, 2021 at 02:11:31PM -0800, Florian Fainelli wrote:
> 
> 
> On 1/8/2021 1:04 PM, Andrew Lunn wrote:
> > On Fri, Jan 08, 2021 at 03:02:52PM -0500, Brian Silverman wrote:
> >> Thanks for the responses - I now have a more clear picture of what's going on.
> >>  (Note: I'm using Xilinx's 2019.2 kernel (based off 4.19).  I believe it would
> >> be similar to latest kernels, but I could be wrong.)
> > 
> > Hi Brian
> > 
> > macb_main has had a lot of changes with respect to PHYs. Please try
> > something modern, like 5.10.
> 
> It does not seem to me like 5.10 will be much better, because we have
> the following in PHYLINK:
> 
> int phylink_of_phy_connect(struct phylink *pl, struct device_node *dn,
>                              u32 flags)
> ...
>           phy_dev = of_phy_find_device(phy_node);
>           /* We're done with the phy_node handle */
>           of_node_put(phy_node);
>           if (!phy_dev)
>                   return -ENODEV;
> 
> Given Brian's configuration we should be returning -EPROBE_DEFER here,
> but doing that would likely break a number of systems that do expect
> -ENODEV to be returned. However there may be hope with fw_devlink to
> create an appropriate graph of probing orders and solve the
> consumer/provider order generically.
> 
> Up until now we did not really have a situation like this one where the
> MDIO/PHY subsystem depended upon an another one to be available. The
> problem does exist, however it is not clear to me yet how to best solve it.

Hi Florian

But we do have situations where the MDIO bus driver is independent of
the MAC driver. And mdio-i2c is not that different to mdio-gpio, where
we need a GPIO driver to load before mdio-gpio is usable. Are all this
also broken?

     Andrew

Re: MDIO over I2C driver driver probe dependency issue

[Andrew Lunn]

On Fri, Jan 08, 2021 at 02:11:31PM -0800, Florian Fainelli wrote:
> On 1/8/2021 1:04 PM, Andrew Lunn wrote:
> > On Fri, Jan 08, 2021 at 03:02:52PM -0500, Brian Silverman wrote:
> >> Thanks for the responses - I now have a more clear picture of what's going on.
> >>  (Note: I'm using Xilinx's 2019.2 kernel (based off 4.19).  I believe it would
> >> be similar to latest kernels, but I could be wrong.)
> > 
> > Hi Brian
> > 
> > macb_main has had a lot of changes with respect to PHYs. Please try
> > something modern, like 5.10.
> 
> It does not seem to me like 5.10 will be much better, because we have
> the following in PHYLINK:
> 
> int phylink_of_phy_connect(struct phylink *pl, struct device_node *dn,
>                              u32 flags)
> ...
>           phy_dev = of_phy_find_device(phy_node);
>           /* We're done with the phy_node handle */
>           of_node_put(phy_node);
>           if (!phy_dev)
>                   return -ENODEV;
> 
> Given Brian's configuration we should be returning -EPROBE_DEFER here,
> but doing that would likely break a number of systems that do expect
> -ENODEV to be returned.

I just looked through the current users of phylink_of_phy_connect().
Most simply do a netdev_err() and return the error code to higher
levels. So apart from the spurious netdev_err() is -EPROBE_DEFER is
returned, they should do the right thing.

mvneta actually looks broken, it prints the error, but keeps going,
plays with WOL setings on the phy and device_set_wake() then returns
the error code.

macb is a bit more complex, but if i'm understanding it correctly, it
should handle -EPROBE_DEFER already, but you will get a spurious
netdev_err() for the -EPROBE_DEFER.

So i think we can fix this, and we should probably do it before there
are more users.

Brian, can you run a modern kernel to test patches, or do you need to
use the Xilinx fork?

    Andrew