[PATCH net-next v2] net: macb: In shared MDIO usecase make MDIO producer ethernet node to probe first

[PATCH net-next v2] net: macb: In shared MDIO usecase make MDIO producer ethernet node to probe first

[Radhey Shyam Pandey]

In shared MDIO suspend/resume usecase for ex. with MDIO producer
(0xff0c0000) eth1 and MDIO consumer(0xff0b0000) eth0 there is a
constraint that ethernet interface(ff0c0000) MDIO bus producer
has to be resumed before the consumer ethernet interface(ff0b0000).

However above constraint is not met when GEM0(ff0b0000) is resumed first.
There is phy_error on GEM0 and interface becomes non-functional on resume.

suspend:
[ 46.477795] macb ff0c0000.ethernet eth1: Link is Down
[ 46.483058] macb ff0c0000.ethernet: gem-ptp-timer ptp clock unregistered.
[ 46.490097] macb ff0b0000.ethernet eth0: Link is Down
[ 46.495298] macb ff0b0000.ethernet: gem-ptp-timer ptp clock unregistered.

resume:
[ 46.633840] macb ff0b0000.ethernet eth0: configuring for phy/sgmii link mode
macb_mdio_read -> pm_runtime_get_sync(GEM1) it return -EACCES error.

The suspend/resume is dependent on probe order so to fix this dependency
ensure that MDIO producer ethernet node is always probed first followed
by MDIO consumer ethernet node.

During MDIO registration find out if MDIO bus is shared and check if MDIO
producer platform node(traverse by 'phy-handle' property) is bound. If not
bound then defer the MDIO consumer ethernet node probe. Doing it ensures
that in suspend/resume MDIO producer is resumed followed by MDIO consumer
ethernet node.

Signed-off-by: Radhey Shyam Pandey <radhey.shyam.pandey@amd.com>
---
Changes for v2:
- check presence of drvdata instead of using device_is_bound()
  to fix module compilation. Idea derived from ongoing
  onboard_usb_hub driver series[1].
  [1]: https://lore.kernel.org/all/20220622144857.v23.2.I7c9a1f1d6ced41dd8310e8a03da666a32364e790@changeid
  Listed in v21 changes.
- CC Saravana, Greg, Rafael and ethernet phy maintainers.

Background notes:
As an alternative to this defer probe approach i also explored using
devicelink framework in ndo_open and create a link between consumer and
producer and that solves suspend/resume issue but incase MDIO producer
probe fails MDIO consumer ethernet node remain non-functional. So a
simpler solution seem to defer MDIO consumer ethernet probe till all
dependencies are met.

Please suggest if there is better of solving MDIO producer dependency.
Copied below DTS snippet for reference.

ethernet@ff0b0000 {
    is-internal-pcspma;
    phy-handle = <0x8f>;
    phys = <0x17 0x0 0x8 0x0 0x0>;
    compatible = "cdns,zynqmp-gem", "cdns,gem";
    status = "okay";
	<snip>
    xlnx,ptp-enet-clock = <0x0>;
    phandle = <0x58>;
};

ethernet@ff0c0000 {
    phy-handle = <0x91>;
    pinctrl-0 = <0x90>;
    pinctrl-names = "default";
    compatible = "cdns,zynqmp-gem", "cdns,gem";
    status = "okay";
    <snip>
	mdio {
        phandle = <0x99>;
        #size-cells = <0x0>;
        #address-cells = <0x1>;

        ethernet-phy@8 {
        phandle = <0x91>;
        reset-gpios = <0x8b 0x6 0x1>;
        reset-deassert-us = <0x118>;
        reset-assert-us = <0x64>;
        ti,dp83867-rxctrl-strap-quirk;
        ti,fifo-depth = <0x1>;
        ti,tx-internal-delay = <0xa>;
        ti,rx-internal-delay = <0x8>;
        reg = <0x8>;
        compatible = "ethernet-phy-id2000.a231";
        #phy-cells = <0x1>;
        };

        ethernet-phy@4 {
            phandle = <0x8f>;
            reset-gpios = <0x8b 0x5 0x1>;
            reset-deassert-us = <0x118>;
            reset-assert-us = <0x64>;
            ti,dp83867-rxctrl-strap-quirk;
            ti,fifo-depth = <0x1>;
            ti,tx-internal-delay = <0xa>;
            ti,rx-internal-delay = <0x8>;
            reg = <0x4>;
            compatible = "ethernet-phy-id2000.a231";
            #phy-cells = <0x1>;
            };
        };
};
---
 drivers/net/ethernet/cadence/macb_main.c | 23 ++++++++++++++++++++++-
 1 file changed, 22 insertions(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/cadence/macb_main.c b/drivers/net/ethernet/cadence/macb_main.c
index d0ea8dbfa213..88b95d4cacaf 100644
--- a/drivers/net/ethernet/cadence/macb_main.c
+++ b/drivers/net/ethernet/cadence/macb_main.c
@@ -853,7 +853,8 @@ static int macb_mii_probe(struct net_device *dev)
 
 static int macb_mdiobus_register(struct macb *bp)
 {
-	struct device_node *child, *np = bp->pdev->dev.of_node;
+	struct device_node *child, *np = bp->pdev->dev.of_node, *mdio_np, *dev_np;
+	struct platform_device *mdio_pdev;
 
 	/* If we have a child named mdio, probe it instead of looking for PHYs
 	 * directly under the MAC node
@@ -884,6 +885,26 @@ static int macb_mdiobus_register(struct macb *bp)
 			return of_mdiobus_register(bp->mii_bus, np);
 		}
 
+	/* For shared MDIO usecases find out MDIO producer platform
+	 * device node by traversing through phy-handle DT property.
+	 */
+	np = of_parse_phandle(np, "phy-handle", 0);
+	mdio_np = of_get_parent(np);
+	of_node_put(np);
+	dev_np = of_get_parent(mdio_np);
+	of_node_put(mdio_np);
+	mdio_pdev = of_find_device_by_node(dev_np);
+	of_node_put(dev_np);
+
+	/* Check MDIO producer device driver data to see if it's probed */
+	if (mdio_pdev && !dev_get_drvdata(&mdio_pdev->dev)) {
+		platform_device_put(mdio_pdev);
+		netdev_info(bp->dev, "Defer probe as mdio producer %s is not probed\n",
+			    dev_name(&mdio_pdev->dev));
+		return -EPROBE_DEFER;
+	}
+
+	platform_device_put(mdio_pdev);
 	return mdiobus_register(bp->mii_bus);
 }
 
-- 
2.1.1

Re: [PATCH net-next v2] net: macb: In shared MDIO usecase make MDIO producer ethernet node to probe first

[Andrew Lunn]

On Fri, Jul 01, 2022 at 01:25:06AM +0530, Radhey Shyam Pandey wrote:
> In shared MDIO suspend/resume usecase for ex. with MDIO producer
> (0xff0c0000) eth1 and MDIO consumer(0xff0b0000) eth0 there is a
> constraint that ethernet interface(ff0c0000) MDIO bus producer
> has to be resumed before the consumer ethernet interface(ff0b0000).
> 
> However above constraint is not met when GEM0(ff0b0000) is resumed first.
> There is phy_error on GEM0 and interface becomes non-functional on resume.
> 
> suspend:
> [ 46.477795] macb ff0c0000.ethernet eth1: Link is Down
> [ 46.483058] macb ff0c0000.ethernet: gem-ptp-timer ptp clock unregistered.
> [ 46.490097] macb ff0b0000.ethernet eth0: Link is Down
> [ 46.495298] macb ff0b0000.ethernet: gem-ptp-timer ptp clock unregistered.
> 
> resume:
> [ 46.633840] macb ff0b0000.ethernet eth0: configuring for phy/sgmii link mode
> macb_mdio_read -> pm_runtime_get_sync(GEM1) it return -EACCES error.
> 
> The suspend/resume is dependent on probe order so to fix this dependency
> ensure that MDIO producer ethernet node is always probed first followed
> by MDIO consumer ethernet node.
> 
> During MDIO registration find out if MDIO bus is shared and check if MDIO
> producer platform node(traverse by 'phy-handle' property) is bound. If not
> bound then defer the MDIO consumer ethernet node probe. Doing it ensures
> that in suspend/resume MDIO producer is resumed followed by MDIO consumer
> ethernet node.

I don't think there is anything specific to MACB here. There are
Freescale boards which have an MDIO bus shared by two interfaces etc.

Please try to solve this in a generic way, not specific to one MAC and
MDIO combination.

     Andrew

RE: [PATCH net-next v2] net: macb: In shared MDIO usecase make MDIO producer ethernet node to probe first

[Pandey, Radhey Shyam]

> -----Original Message-----
> From: Andrew Lunn <andrew@lunn.ch>
> Sent: Friday, July 1, 2022 2:44 PM
> To: Pandey, Radhey Shyam <radhey.shyam.pandey@amd.com>
> Cc: nicolas.ferre@microchip.com; claudiu.beznea@microchip.com;
> davem@davemloft.net; edumazet@google.com; kuba@kernel.org;
> pabeni@redhat.com; hkallweit1@gmail.com; linux@armlinux.org.uk;
> gregkh@linuxfoundation.org; rafael@kernel.org; saravanak@google.com;
> netdev@vger.kernel.org; linux-kernel@vger.kernel.org; git (AMD-Xilinx)
> <git@amd.com>
> Subject: Re: [PATCH net-next v2] net: macb: In shared MDIO usecase make
> MDIO producer ethernet node to probe first
> 
> On Fri, Jul 01, 2022 at 01:25:06AM +0530, Radhey Shyam Pandey wrote:
> > In shared MDIO suspend/resume usecase for ex. with MDIO producer
> > (0xff0c0000) eth1 and MDIO consumer(0xff0b0000) eth0 there is a
> > constraint that ethernet interface(ff0c0000) MDIO bus producer has to
> > be resumed before the consumer ethernet interface(ff0b0000).
> >
> > However above constraint is not met when GEM0(ff0b0000) is resumed first.
> > There is phy_error on GEM0 and interface becomes non-functional on
> resume.
> >
> > suspend:
> > [ 46.477795] macb ff0c0000.ethernet eth1: Link is Down [ 46.483058]
> > macb ff0c0000.ethernet: gem-ptp-timer ptp clock unregistered.
> > [ 46.490097] macb ff0b0000.ethernet eth0: Link is Down [ 46.495298]
> > macb ff0b0000.ethernet: gem-ptp-timer ptp clock unregistered.
> >
> > resume:
> > [ 46.633840] macb ff0b0000.ethernet eth0: configuring for phy/sgmii
> > link mode macb_mdio_read -> pm_runtime_get_sync(GEM1) it return -
> EACCES error.
> >
> > The suspend/resume is dependent on probe order so to fix this
> > dependency ensure that MDIO producer ethernet node is always probed
> > first followed by MDIO consumer ethernet node.
> >
> > During MDIO registration find out if MDIO bus is shared and check if
> > MDIO producer platform node(traverse by 'phy-handle' property) is
> > bound. If not bound then defer the MDIO consumer ethernet node probe.
> > Doing it ensures that in suspend/resume MDIO producer is resumed
> > followed by MDIO consumer ethernet node.
> 
> I don't think there is anything specific to MACB here. There are Freescale
> boards which have an MDIO bus shared by two interfaces etc.
> 
> Please try to solve this in a generic way, not specific to one MAC and MDIO
> combination.

Thanks for the review.  I want to get your thoughts on the outline of
the generic solution. Is the current approach fine and we can extend it
for all shared MDIO use cases/ or do we see any limitations?
 
a) Figure out if the MDIO bus is shared.  (new binding or reuse existing)
b) If the MDIO bus is shared based on DT property then figure out if the 
MDIO producer platform device is probed. If not, defer MDIO consumer
MDIO bus registration.

> 
>      Andrew

Re: [PATCH net-next v2] net: macb: In shared MDIO usecase make MDIO producer ethernet node to probe first

[Saravana Kannan]

On Tue, Jul 5, 2022 at 11:49 AM Pandey, Radhey Shyam
<radhey.shyam.pandey@amd.com> wrote:
>
> > -----Original Message-----
> > From: Andrew Lunn <andrew@lunn.ch>
> > Sent: Friday, July 1, 2022 2:44 PM
> > To: Pandey, Radhey Shyam <radhey.shyam.pandey@amd.com>
> > Cc: nicolas.ferre@microchip.com; claudiu.beznea@microchip.com;
> > davem@davemloft.net; edumazet@google.com; kuba@kernel.org;
> > pabeni@redhat.com; hkallweit1@gmail.com; linux@armlinux.org.uk;
> > gregkh@linuxfoundation.org; rafael@kernel.org; saravanak@google.com;
> > netdev@vger.kernel.org; linux-kernel@vger.kernel.org; git (AMD-Xilinx)
> > <git@amd.com>
> > Subject: Re: [PATCH net-next v2] net: macb: In shared MDIO usecase make
> > MDIO producer ethernet node to probe first
> >
> > On Fri, Jul 01, 2022 at 01:25:06AM +0530, Radhey Shyam Pandey wrote:
> > > In shared MDIO suspend/resume usecase for ex. with MDIO producer
> > > (0xff0c0000) eth1 and MDIO consumer(0xff0b0000) eth0 there is a
> > > constraint that ethernet interface(ff0c0000) MDIO bus producer has to
> > > be resumed before the consumer ethernet interface(ff0b0000).
> > >
> > > However above constraint is not met when GEM0(ff0b0000) is resumed first.
> > > There is phy_error on GEM0 and interface becomes non-functional on
> > resume.
> > >
> > > suspend:
> > > [ 46.477795] macb ff0c0000.ethernet eth1: Link is Down [ 46.483058]
> > > macb ff0c0000.ethernet: gem-ptp-timer ptp clock unregistered.
> > > [ 46.490097] macb ff0b0000.ethernet eth0: Link is Down [ 46.495298]
> > > macb ff0b0000.ethernet: gem-ptp-timer ptp clock unregistered.
> > >
> > > resume:
> > > [ 46.633840] macb ff0b0000.ethernet eth0: configuring for phy/sgmii
> > > link mode macb_mdio_read -> pm_runtime_get_sync(GEM1) it return -
> > EACCES error.
> > >
> > > The suspend/resume is dependent on probe order so to fix this
> > > dependency ensure that MDIO producer ethernet node is always probed
> > > first followed by MDIO consumer ethernet node.
> > >
> > > During MDIO registration find out if MDIO bus is shared and check if
> > > MDIO producer platform node(traverse by 'phy-handle' property) is
> > > bound. If not bound then defer the MDIO consumer ethernet node probe.
> > > Doing it ensures that in suspend/resume MDIO producer is resumed
> > > followed by MDIO consumer ethernet node.
> >
> > I don't think there is anything specific to MACB here. There are Freescale
> > boards which have an MDIO bus shared by two interfaces etc.
> >
> > Please try to solve this in a generic way, not specific to one MAC and MDIO
> > combination.
>
> Thanks for the review.  I want to get your thoughts on the outline of
> the generic solution. Is the current approach fine and we can extend it
> for all shared MDIO use cases/ or do we see any limitations?
>
> a) Figure out if the MDIO bus is shared.  (new binding or reuse existing)
> b) If the MDIO bus is shared based on DT property then figure out if the
> MDIO producer platform device is probed. If not, defer MDIO consumer
> MDIO bus registration.

Radhey,

I think Andrew added me because he's pointing you towards fw_devlink.

Andrew,

I have intentionally not added phy-handle support to fw_devlink
because it would also prevent the generic driver from binding/cause
issues with DSA. I have some high level ideas on fixing that but
haven't gotten around to it yet.

-Saravana

Re: [PATCH net-next v2] net: macb: In shared MDIO usecase make MDIO producer ethernet node to probe first

[Andrew Lunn]

> Thanks for the review.  I want to get your thoughts on the outline of
> the generic solution. Is the current approach fine and we can extend it
> for all shared MDIO use cases/ or do we see any limitations?
>  
> a) Figure out if the MDIO bus is shared.  (new binding or reuse existing)
> b) If the MDIO bus is shared based on DT property then figure out if the 
> MDIO producer platform device is probed. If not, defer MDIO consumer
> MDIO bus registration.

I actually think you need to talk to the core device model people and
those who support suspend/resume.

It seems like there should be a way to declare a dependency, probably
a probe time, so the core will just do the right things. I don't see
why MDIO busses should be the first to have this problem, so i expect
there already exists a solution.

	Andrew

Re: [PATCH net-next v2] net: macb: In shared MDIO usecase make MDIO producer ethernet node to probe first

[Andrew Lunn]

> > Thanks for the review.  I want to get your thoughts on the outline of
> > the generic solution. Is the current approach fine and we can extend it
> > for all shared MDIO use cases/ or do we see any limitations?
> >
> > a) Figure out if the MDIO bus is shared.  (new binding or reuse existing)
> > b) If the MDIO bus is shared based on DT property then figure out if the
> > MDIO producer platform device is probed. If not, defer MDIO consumer
> > MDIO bus registration.
> 
> Radhey,
> 
> I think Andrew added me because he's pointing you towards fw_devlink.
> 
> Andrew,
> 
> I have intentionally not added phy-handle support to fw_devlink
> because it would also prevent the generic driver from binding/cause
> issues with DSA. I have some high level ideas on fixing that but
> haven't gotten around to it yet.

I took a quick look at macb, and i think it is actually broken in
other ways. If you where to use NFS root, i suspect it would also
fail.

This also has nothing to do with shared MDIO busses as such. All it
requires is some other MDIO bus, not the MACs own MDIO bus.

It is also that we cannot return -EPROBE_DEFER when trying to connect
the PHY, because it is not performed in the context of the probe, but
the open.

fw_dewlink might help solve this, bit it is not going to be easy. We
can also split this into two problems;

1) probe time
2) suspend/resume

macb does seem to probe, for most use cases. So we can probably ignore
that for now. So we can concentrate on suspend/resume. You say
suspend/resume is based on probe order. So it must build some sort of
tree. Can we make phy_attach_direct add an additional link to this
tree when a MAC device is link to a PHY? Is this what
device_link_add() is about?

     Andrew

RE: [PATCH net-next v2] net: macb: In shared MDIO usecase make MDIO producer ethernet node to probe first

[Pandey, Radhey Shyam]

> -----Original Message-----
> From: Saravana Kannan <saravanak@google.com>
> Sent: Wednesday, July 6, 2022 12:28 AM
> To: Pandey, Radhey Shyam <radhey.shyam.pandey@amd.com>
> Cc: Andrew Lunn <andrew@lunn.ch>; nicolas.ferre@microchip.com;
> claudiu.beznea@microchip.com; davem@davemloft.net;
> edumazet@google.com; kuba@kernel.org; pabeni@redhat.com;
> hkallweit1@gmail.com; linux@armlinux.org.uk;
> gregkh@linuxfoundation.org; rafael@kernel.org; netdev@vger.kernel.org;
> linux-kernel@vger.kernel.org; git (AMD-Xilinx) <git@amd.com>
> Subject: Re: [PATCH net-next v2] net: macb: In shared MDIO usecase make
> MDIO producer ethernet node to probe first
> 
> On Tue, Jul 5, 2022 at 11:49 AM Pandey, Radhey Shyam
> <radhey.shyam.pandey@amd.com> wrote:
> >
> > > -----Original Message-----
> > > From: Andrew Lunn <andrew@lunn.ch>
> > > Sent: Friday, July 1, 2022 2:44 PM
> > > To: Pandey, Radhey Shyam <radhey.shyam.pandey@amd.com>
> > > Cc: nicolas.ferre@microchip.com; claudiu.beznea@microchip.com;
> > > davem@davemloft.net; edumazet@google.com; kuba@kernel.org;
> > > pabeni@redhat.com; hkallweit1@gmail.com; linux@armlinux.org.uk;
> > > gregkh@linuxfoundation.org; rafael@kernel.org;
> saravanak@google.com;
> > > netdev@vger.kernel.org; linux-kernel@vger.kernel.org; git
> > > (AMD-Xilinx) <git@amd.com>
> > > Subject: Re: [PATCH net-next v2] net: macb: In shared MDIO usecase
> > > make MDIO producer ethernet node to probe first
> > >
> > > On Fri, Jul 01, 2022 at 01:25:06AM +0530, Radhey Shyam Pandey wrote:
> > > > In shared MDIO suspend/resume usecase for ex. with MDIO producer
> > > > (0xff0c0000) eth1 and MDIO consumer(0xff0b0000) eth0 there is a
> > > > constraint that ethernet interface(ff0c0000) MDIO bus producer has
> > > > to be resumed before the consumer ethernet interface(ff0b0000).
> > > >
> > > > However above constraint is not met when GEM0(ff0b0000) is resumed
> first.
> > > > There is phy_error on GEM0 and interface becomes non-functional on
> > > resume.
> > > >
> > > > suspend:
> > > > [ 46.477795] macb ff0c0000.ethernet eth1: Link is Down [
> > > > 46.483058] macb ff0c0000.ethernet: gem-ptp-timer ptp clock
> unregistered.
> > > > [ 46.490097] macb ff0b0000.ethernet eth0: Link is Down [
> > > > 46.495298] macb ff0b0000.ethernet: gem-ptp-timer ptp clock
> unregistered.
> > > >
> > > > resume:
> > > > [ 46.633840] macb ff0b0000.ethernet eth0: configuring for
> > > > phy/sgmii link mode macb_mdio_read -> pm_runtime_get_sync(GEM1)
> it
> > > > return -
> > > EACCES error.
> > > >
> > > > The suspend/resume is dependent on probe order so to fix this
> > > > dependency ensure that MDIO producer ethernet node is always
> > > > probed first followed by MDIO consumer ethernet node.
> > > >
> > > > During MDIO registration find out if MDIO bus is shared and check
> > > > if MDIO producer platform node(traverse by 'phy-handle' property)
> > > > is bound. If not bound then defer the MDIO consumer ethernet node
> probe.
> > > > Doing it ensures that in suspend/resume MDIO producer is resumed
> > > > followed by MDIO consumer ethernet node.
> > >
> > > I don't think there is anything specific to MACB here. There are
> > > Freescale boards which have an MDIO bus shared by two interfaces etc.
> > >
> > > Please try to solve this in a generic way, not specific to one MAC
> > > and MDIO combination.
> >
> > Thanks for the review.  I want to get your thoughts on the outline of
> > the generic solution. Is the current approach fine and we can extend
> > it for all shared MDIO use cases/ or do we see any limitations?
> >
> > a) Figure out if the MDIO bus is shared.  (new binding or reuse
> > existing)
> > b) If the MDIO bus is shared based on DT property then figure out if
> > the MDIO producer platform device is probed. If not, defer MDIO
> > consumer MDIO bus registration.
> 
> Radhey,
> 
> I think Andrew added me because he's pointing you towards fw_devlink.
> 
> Andrew,
> 
> I have intentionally not added phy-handle support to fw_devlink because it
> would also prevent the generic driver from binding/cause issues with DSA. I
> have some high level ideas on fixing that but haven't gotten around to it yet.
Thanks, just want to understand on implementation when phy-handle support is
added to fw_devlink. Does it ensure that supplier node is probed first? Or it uses
device_link framework to specify suspend/resume dependency and don't care
on consumer/producer probe order.
 
> 
> -Saravana

Re: [PATCH net-next v2] net: macb: In shared MDIO usecase make MDIO producer ethernet node to probe first

[Saravana Kannan]

On Tue, Jul 5, 2022 at 12:49 PM Andrew Lunn <andrew@lunn.ch> wrote:
>
> > > Thanks for the review.  I want to get your thoughts on the outline of
> > > the generic solution. Is the current approach fine and we can extend it
> > > for all shared MDIO use cases/ or do we see any limitations?
> > >
> > > a) Figure out if the MDIO bus is shared.  (new binding or reuse existing)
> > > b) If the MDIO bus is shared based on DT property then figure out if the
> > > MDIO producer platform device is probed. If not, defer MDIO consumer
> > > MDIO bus registration.
> >
> > Radhey,
> >
> > I think Andrew added me because he's pointing you towards fw_devlink.
> >
> > Andrew,
> >
> > I have intentionally not added phy-handle support to fw_devlink
> > because it would also prevent the generic driver from binding/cause
> > issues with DSA. I have some high level ideas on fixing that but
> > haven't gotten around to it yet.
>
> I took a quick look at macb, and i think it is actually broken in
> other ways. If you where to use NFS root, i suspect it would also
> fail.
>
> This also has nothing to do with shared MDIO busses as such. All it
> requires is some other MDIO bus, not the MACs own MDIO bus.
>
> It is also that we cannot return -EPROBE_DEFER when trying to connect
> the PHY, because it is not performed in the context of the probe, but
> the open.
>
> fw_dewlink might help solve this, bit it is not going to be easy. We
> can also split this into two problems;
>
> 1) probe time
> 2) suspend/resume
>
> macb does seem to probe, for most use cases. So we can probably ignore
> that for now. So we can concentrate on suspend/resume. You say
> suspend/resume is based on probe order. So it must build some sort of
> tree. Can we make phy_attach_direct add an additional link to this
> tree when a MAC device is link to a PHY? Is this what
> device_link_add() is about?

Based on the flags you pass, you can tell device link to only enforce
suspend/resume ordering or also enforce probe ordering.

-Saravana

Re: [PATCH net-next v2] net: macb: In shared MDIO usecase make MDIO producer ethernet node to probe first

[Saravana Kannan]

On Fri, Jul 15, 2022 at 12:00 PM Pandey, Radhey Shyam
<radhey.shyam.pandey@amd.com> wrote:
>
> > -----Original Message-----
> > From: Saravana Kannan <saravanak@google.com>
> > Sent: Wednesday, July 6, 2022 12:28 AM
> > To: Pandey, Radhey Shyam <radhey.shyam.pandey@amd.com>
> > Cc: Andrew Lunn <andrew@lunn.ch>; nicolas.ferre@microchip.com;
> > claudiu.beznea@microchip.com; davem@davemloft.net;
> > edumazet@google.com; kuba@kernel.org; pabeni@redhat.com;
> > hkallweit1@gmail.com; linux@armlinux.org.uk;
> > gregkh@linuxfoundation.org; rafael@kernel.org; netdev@vger.kernel.org;
> > linux-kernel@vger.kernel.org; git (AMD-Xilinx) <git@amd.com>
> > Subject: Re: [PATCH net-next v2] net: macb: In shared MDIO usecase make
> > MDIO producer ethernet node to probe first
> >
> > On Tue, Jul 5, 2022 at 11:49 AM Pandey, Radhey Shyam
> > <radhey.shyam.pandey@amd.com> wrote:
> > >
> > > > -----Original Message-----
> > > > From: Andrew Lunn <andrew@lunn.ch>
> > > > Sent: Friday, July 1, 2022 2:44 PM
> > > > To: Pandey, Radhey Shyam <radhey.shyam.pandey@amd.com>
> > > > Cc: nicolas.ferre@microchip.com; claudiu.beznea@microchip.com;
> > > > davem@davemloft.net; edumazet@google.com; kuba@kernel.org;
> > > > pabeni@redhat.com; hkallweit1@gmail.com; linux@armlinux.org.uk;
> > > > gregkh@linuxfoundation.org; rafael@kernel.org;
> > saravanak@google.com;
> > > > netdev@vger.kernel.org; linux-kernel@vger.kernel.org; git
> > > > (AMD-Xilinx) <git@amd.com>
> > > > Subject: Re: [PATCH net-next v2] net: macb: In shared MDIO usecase
> > > > make MDIO producer ethernet node to probe first
> > > >
> > > > On Fri, Jul 01, 2022 at 01:25:06AM +0530, Radhey Shyam Pandey wrote:
> > > > > In shared MDIO suspend/resume usecase for ex. with MDIO producer
> > > > > (0xff0c0000) eth1 and MDIO consumer(0xff0b0000) eth0 there is a
> > > > > constraint that ethernet interface(ff0c0000) MDIO bus producer has
> > > > > to be resumed before the consumer ethernet interface(ff0b0000).
> > > > >
> > > > > However above constraint is not met when GEM0(ff0b0000) is resumed
> > first.
> > > > > There is phy_error on GEM0 and interface becomes non-functional on
> > > > resume.
> > > > >
> > > > > suspend:
> > > > > [ 46.477795] macb ff0c0000.ethernet eth1: Link is Down [
> > > > > 46.483058] macb ff0c0000.ethernet: gem-ptp-timer ptp clock
> > unregistered.
> > > > > [ 46.490097] macb ff0b0000.ethernet eth0: Link is Down [
> > > > > 46.495298] macb ff0b0000.ethernet: gem-ptp-timer ptp clock
> > unregistered.
> > > > >
> > > > > resume:
> > > > > [ 46.633840] macb ff0b0000.ethernet eth0: configuring for
> > > > > phy/sgmii link mode macb_mdio_read -> pm_runtime_get_sync(GEM1)
> > it
> > > > > return -
> > > > EACCES error.
> > > > >
> > > > > The suspend/resume is dependent on probe order so to fix this
> > > > > dependency ensure that MDIO producer ethernet node is always
> > > > > probed first followed by MDIO consumer ethernet node.
> > > > >
> > > > > During MDIO registration find out if MDIO bus is shared and check
> > > > > if MDIO producer platform node(traverse by 'phy-handle' property)
> > > > > is bound. If not bound then defer the MDIO consumer ethernet node
> > probe.
> > > > > Doing it ensures that in suspend/resume MDIO producer is resumed
> > > > > followed by MDIO consumer ethernet node.
> > > >
> > > > I don't think there is anything specific to MACB here. There are
> > > > Freescale boards which have an MDIO bus shared by two interfaces etc.
> > > >
> > > > Please try to solve this in a generic way, not specific to one MAC
> > > > and MDIO combination.
> > >
> > > Thanks for the review.  I want to get your thoughts on the outline of
> > > the generic solution. Is the current approach fine and we can extend
> > > it for all shared MDIO use cases/ or do we see any limitations?
> > >
> > > a) Figure out if the MDIO bus is shared.  (new binding or reuse
> > > existing)
> > > b) If the MDIO bus is shared based on DT property then figure out if
> > > the MDIO producer platform device is probed. If not, defer MDIO
> > > consumer MDIO bus registration.
> >
> > Radhey,
> >
> > I think Andrew added me because he's pointing you towards fw_devlink.
> >
> > Andrew,
> >
> > I have intentionally not added phy-handle support to fw_devlink because it
> > would also prevent the generic driver from binding/cause issues with DSA. I
> > have some high level ideas on fixing that but haven't gotten around to it yet.
>
> Thanks, just want to understand on implementation when phy-handle support is
> added to fw_devlink. Does it ensure that supplier node is probed first? Or it uses
> device_link framework to specify suspend/resume dependency and don't care
> on consumer/producer probe order.

fw_devlink will enforce probe ordering and suspend/resume ordering.
Btw, fw_devlink uses device links underneath. It just used the
firmware (Eg: DT) to figure out the dependencies. That's why it's
called fw_devlink.

-Saravana

RE: [PATCH net-next v2] net: macb: In shared MDIO usecase make MDIO producer ethernet node to probe first

[Pandey, Radhey Shyam]

> -----Original Message-----
> From: Saravana Kannan <saravanak@google.com>
> Sent: Saturday, July 16, 2022 12:39 AM
> To: Pandey, Radhey Shyam <radhey.shyam.pandey@amd.com>
> Cc: Andrew Lunn <andrew@lunn.ch>; nicolas.ferre@microchip.com;
> claudiu.beznea@microchip.com; davem@davemloft.net;
> edumazet@google.com; kuba@kernel.org; pabeni@redhat.com;
> hkallweit1@gmail.com; linux@armlinux.org.uk;
> gregkh@linuxfoundation.org; rafael@kernel.org; netdev@vger.kernel.org;
> linux-kernel@vger.kernel.org; git (AMD-Xilinx) <git@amd.com>
> Subject: Re: [PATCH net-next v2] net: macb: In shared MDIO usecase make
> MDIO producer ethernet node to probe first
> 
> On Fri, Jul 15, 2022 at 12:00 PM Pandey, Radhey Shyam
> <radhey.shyam.pandey@amd.com> wrote:
> >
> > > -----Original Message-----
> > > From: Saravana Kannan <saravanak@google.com>
> > > Sent: Wednesday, July 6, 2022 12:28 AM
> > > To: Pandey, Radhey Shyam <radhey.shyam.pandey@amd.com>
> > > Cc: Andrew Lunn <andrew@lunn.ch>; nicolas.ferre@microchip.com;
> > > claudiu.beznea@microchip.com; davem@davemloft.net;
> > > edumazet@google.com; kuba@kernel.org; pabeni@redhat.com;
> > > hkallweit1@gmail.com; linux@armlinux.org.uk;
> > > gregkh@linuxfoundation.org; rafael@kernel.org;
> > > netdev@vger.kernel.org; linux-kernel@vger.kernel.org; git
> > > (AMD-Xilinx) <git@amd.com>
> > > Subject: Re: [PATCH net-next v2] net: macb: In shared MDIO usecase
> > > make MDIO producer ethernet node to probe first
> > >
> > > On Tue, Jul 5, 2022 at 11:49 AM Pandey, Radhey Shyam
> > > <radhey.shyam.pandey@amd.com> wrote:
> > > >
> > > > > -----Original Message-----
> > > > > From: Andrew Lunn <andrew@lunn.ch>
> > > > > Sent: Friday, July 1, 2022 2:44 PM
> > > > > To: Pandey, Radhey Shyam <radhey.shyam.pandey@amd.com>
> > > > > Cc: nicolas.ferre@microchip.com; claudiu.beznea@microchip.com;
> > > > > davem@davemloft.net; edumazet@google.com; kuba@kernel.org;
> > > > > pabeni@redhat.com; hkallweit1@gmail.com; linux@armlinux.org.uk;
> > > > > gregkh@linuxfoundation.org; rafael@kernel.org;
> > > saravanak@google.com;
> > > > > netdev@vger.kernel.org; linux-kernel@vger.kernel.org; git
> > > > > (AMD-Xilinx) <git@amd.com>
> > > > > Subject: Re: [PATCH net-next v2] net: macb: In shared MDIO
> > > > > usecase make MDIO producer ethernet node to probe first
> > > > >
> > > > > On Fri, Jul 01, 2022 at 01:25:06AM +0530, Radhey Shyam Pandey
> wrote:
> > > > > > In shared MDIO suspend/resume usecase for ex. with MDIO
> > > > > > producer
> > > > > > (0xff0c0000) eth1 and MDIO consumer(0xff0b0000) eth0 there is
> > > > > > a constraint that ethernet interface(ff0c0000) MDIO bus
> > > > > > producer has to be resumed before the consumer ethernet
> interface(ff0b0000).
> > > > > >
> > > > > > However above constraint is not met when GEM0(ff0b0000) is
> > > > > > resumed
> > > first.
> > > > > > There is phy_error on GEM0 and interface becomes
> > > > > > non-functional on
> > > > > resume.
> > > > > >
> > > > > > suspend:
> > > > > > [ 46.477795] macb ff0c0000.ethernet eth1: Link is Down [
> > > > > > 46.483058] macb ff0c0000.ethernet: gem-ptp-timer ptp clock
> > > unregistered.
> > > > > > [ 46.490097] macb ff0b0000.ethernet eth0: Link is Down [
> > > > > > 46.495298] macb ff0b0000.ethernet: gem-ptp-timer ptp clock
> > > unregistered.
> > > > > >
> > > > > > resume:
> > > > > > [ 46.633840] macb ff0b0000.ethernet eth0: configuring for
> > > > > > phy/sgmii link mode macb_mdio_read ->
> > > > > > pm_runtime_get_sync(GEM1)
> > > it
> > > > > > return -
> > > > > EACCES error.
> > > > > >
> > > > > > The suspend/resume is dependent on probe order so to fix this
> > > > > > dependency ensure that MDIO producer ethernet node is always
> > > > > > probed first followed by MDIO consumer ethernet node.
> > > > > >
> > > > > > During MDIO registration find out if MDIO bus is shared and
> > > > > > check if MDIO producer platform node(traverse by 'phy-handle'
> > > > > > property) is bound. If not bound then defer the MDIO consumer
> > > > > > ethernet node
> > > probe.
> > > > > > Doing it ensures that in suspend/resume MDIO producer is
> > > > > > resumed followed by MDIO consumer ethernet node.
> > > > >
> > > > > I don't think there is anything specific to MACB here. There are
> > > > > Freescale boards which have an MDIO bus shared by two interfaces
> etc.
> > > > >
> > > > > Please try to solve this in a generic way, not specific to one
> > > > > MAC and MDIO combination.
> > > >
> > > > Thanks for the review.  I want to get your thoughts on the outline
> > > > of the generic solution. Is the current approach fine and we can
> > > > extend it for all shared MDIO use cases/ or do we see any limitations?
> > > >
> > > > a) Figure out if the MDIO bus is shared.  (new binding or reuse
> > > > existing)
> > > > b) If the MDIO bus is shared based on DT property then figure out
> > > > if the MDIO producer platform device is probed. If not, defer MDIO
> > > > consumer MDIO bus registration.
> > >
> > > Radhey,
> > >
> > > I think Andrew added me because he's pointing you towards fw_devlink.
> > >
> > > Andrew,
> > >
> > > I have intentionally not added phy-handle support to fw_devlink
> > > because it would also prevent the generic driver from binding/cause
> > > issues with DSA. I have some high level ideas on fixing that but haven't
> gotten around to it yet.
> >
> > Thanks, just want to understand on implementation when phy-handle
> > support is added to fw_devlink. Does it ensure that supplier node is
> > probed first? Or it uses device_link framework to specify
> > suspend/resume dependency and don't care on consumer/producer probe
> order.
> 
> fw_devlink will enforce probe ordering and suspend/resume ordering.
> Btw, fw_devlink uses device links underneath. It just used the firmware (Eg:
> DT) to figure out the dependencies. That's why it's called fw_devlink.
Thanks! Forgot to ask earlier, when are you planning to add phy-handle support 
to fw_devlink ? seems like we have a dependency on this feature to make
shared MDIO use case work in a generic way.