[PATCH v2] of: property: fw_devlink: Add support for "phy-handle" property

[PATCH v2] of: property: fw_devlink: Add support for "phy-handle" property

[Saravana Kannan]

Allows tracking dependencies between Ethernet PHYs and their consumers.

Cc: Andrew Lunn <andrew@lunn.ch>
Cc: netdev@vger.kernel.org
Signed-off-by: Saravana Kannan <saravanak@google.com>
---
v1 -> v2:
- Fixed patch to address my misunderstanding of how PHYs get
  initialized.

 drivers/of/property.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/of/property.c b/drivers/of/property.c
index 931340329414..0c0dc2e369c0 100644
--- a/drivers/of/property.c
+++ b/drivers/of/property.c
@@ -1291,6 +1291,7 @@ DEFINE_SIMPLE_PROP(pwms, "pwms", "#pwm-cells")
 DEFINE_SIMPLE_PROP(resets, "resets", "#reset-cells")
 DEFINE_SIMPLE_PROP(leds, "leds", NULL)
 DEFINE_SIMPLE_PROP(backlight, "backlight", NULL)
+DEFINE_SIMPLE_PROP(phy_handle, "phy-handle", NULL)
 DEFINE_SUFFIX_PROP(regulators, "-supply", NULL)
 DEFINE_SUFFIX_PROP(gpio, "-gpio", "#gpio-cells")
 
@@ -1379,6 +1380,7 @@ static const struct supplier_bindings of_supplier_bindings[] = {
 	{ .parse_prop = parse_resets, },
 	{ .parse_prop = parse_leds, },
 	{ .parse_prop = parse_backlight, },
+	{ .parse_prop = parse_phy_handle, },
 	{ .parse_prop = parse_gpio_compat, },
 	{ .parse_prop = parse_interrupts, },
 	{ .parse_prop = parse_regulators, },
-- 
2.33.0.rc1.237.g0d66db33f3-goog

Re: [PATCH v2] of: property: fw_devlink: Add support for "phy-handle" property

[Rob Herring]

On Tue, 17 Aug 2021 19:17:16 -0700, Saravana Kannan wrote:
> Allows tracking dependencies between Ethernet PHYs and their consumers.
> 
> Cc: Andrew Lunn <andrew@lunn.ch>
> Cc: netdev@vger.kernel.org
> Signed-off-by: Saravana Kannan <saravanak@google.com>
> ---
> v1 -> v2:
> - Fixed patch to address my misunderstanding of how PHYs get
>   initialized.
> 
>  drivers/of/property.c | 2 ++
>  1 file changed, 2 insertions(+)
> 

Applied, thanks!

Re: [PATCH v2] of: property: fw_devlink: Add support for "phy-handle" property

[Marek Szyprowski]

Hi,

On 18.08.2021 04:17, Saravana Kannan wrote:
> Allows tracking dependencies between Ethernet PHYs and their consumers.
>
> Cc: Andrew Lunn <andrew@lunn.ch>
> Cc: netdev@vger.kernel.org
> Signed-off-by: Saravana Kannan <saravanak@google.com>

This patch landed recently in linux-next as commit cf4b94c8530d ("of: 
property: fw_devlink: Add support for "phy-handle" property"). It breaks 
ethernet operation on my Amlogic-based ARM64 boards: Odroid C4 
(arm64/boot/dts/amlogic/meson-sm1-odroid-c4.dts) and N2 
(meson-g12b-odroid-n2.dts) as well as Khadas VIM3/VIM3l 
(meson-g12b-a311d-khadas-vim3.dts and meson-sm1-khadas-vim3l.dts).

In case of OdroidC4 I see the following entries in the 
/sys/kernel/debug/devices_deferred:

ff64c000.mdio-multiplexer
ff3f0000.ethernet

Let me know if there is anything I can check to help debugging this issue.

> ---
> v1 -> v2:
> - Fixed patch to address my misunderstanding of how PHYs get
>    initialized.
>
>   drivers/of/property.c | 2 ++
>   1 file changed, 2 insertions(+)
>
> diff --git a/drivers/of/property.c b/drivers/of/property.c
> index 931340329414..0c0dc2e369c0 100644
> --- a/drivers/of/property.c
> +++ b/drivers/of/property.c
> @@ -1291,6 +1291,7 @@ DEFINE_SIMPLE_PROP(pwms, "pwms", "#pwm-cells")
>   DEFINE_SIMPLE_PROP(resets, "resets", "#reset-cells")
>   DEFINE_SIMPLE_PROP(leds, "leds", NULL)
>   DEFINE_SIMPLE_PROP(backlight, "backlight", NULL)
> +DEFINE_SIMPLE_PROP(phy_handle, "phy-handle", NULL)
>   DEFINE_SUFFIX_PROP(regulators, "-supply", NULL)
>   DEFINE_SUFFIX_PROP(gpio, "-gpio", "#gpio-cells")
>   
> @@ -1379,6 +1380,7 @@ static const struct supplier_bindings of_supplier_bindings[] = {
>   	{ .parse_prop = parse_resets, },
>   	{ .parse_prop = parse_leds, },
>   	{ .parse_prop = parse_backlight, },
> +	{ .parse_prop = parse_phy_handle, },
>   	{ .parse_prop = parse_gpio_compat, },
>   	{ .parse_prop = parse_interrupts, },
>   	{ .parse_prop = parse_regulators, },

Best regards
-- 
Marek Szyprowski, PhD
Samsung R&D Institute Poland

Re: [PATCH v2] of: property: fw_devlink: Add support for "phy-handle" property

[Rob Herring]

On Mon, Aug 23, 2021 at 7:08 AM Marek Szyprowski
<m.szyprowski@samsung.com> wrote:
>
> Hi,
>
> On 18.08.2021 04:17, Saravana Kannan wrote:
> > Allows tracking dependencies between Ethernet PHYs and their consumers.
> >
> > Cc: Andrew Lunn <andrew@lunn.ch>
> > Cc: netdev@vger.kernel.org
> > Signed-off-by: Saravana Kannan <saravanak@google.com>
>
> This patch landed recently in linux-next as commit cf4b94c8530d ("of:
> property: fw_devlink: Add support for "phy-handle" property"). It breaks
> ethernet operation on my Amlogic-based ARM64 boards: Odroid C4
> (arm64/boot/dts/amlogic/meson-sm1-odroid-c4.dts) and N2
> (meson-g12b-odroid-n2.dts) as well as Khadas VIM3/VIM3l
> (meson-g12b-a311d-khadas-vim3.dts and meson-sm1-khadas-vim3l.dts).
>
> In case of OdroidC4 I see the following entries in the
> /sys/kernel/debug/devices_deferred:
>
> ff64c000.mdio-multiplexer
> ff3f0000.ethernet
>
> Let me know if there is anything I can check to help debugging this issue.

Looks to me like we need to handle 'mdio-parent-bus' dependency.

Rob

Re: [PATCH v2] of: property: fw_devlink: Add support for "phy-handle" property

[Andrew Lunn]

On Mon, Aug 23, 2021 at 02:08:48PM +0200, Marek Szyprowski wrote:
> Hi,
> 
> On 18.08.2021 04:17, Saravana Kannan wrote:
> > Allows tracking dependencies between Ethernet PHYs and their consumers.
> >
> > Cc: Andrew Lunn <andrew@lunn.ch>
> > Cc: netdev@vger.kernel.org
> > Signed-off-by: Saravana Kannan <saravanak@google.com>
> 
> This patch landed recently in linux-next as commit cf4b94c8530d ("of: 
> property: fw_devlink: Add support for "phy-handle" property"). It breaks 
> ethernet operation on my Amlogic-based ARM64 boards: Odroid C4 
> (arm64/boot/dts/amlogic/meson-sm1-odroid-c4.dts) and N2 
> (meson-g12b-odroid-n2.dts) as well as Khadas VIM3/VIM3l 
> (meson-g12b-a311d-khadas-vim3.dts and meson-sm1-khadas-vim3l.dts).
> 
> In case of OdroidC4 I see the following entries in the 
> /sys/kernel/debug/devices_deferred:
> 
> ff64c000.mdio-multiplexer
> ff3f0000.ethernet
> 
> Let me know if there is anything I can check to help debugging this issue.

Hi Marek

Please try this. Completetly untested, not even compile teseted:

diff --git a/drivers/of/property.c b/drivers/of/property.c
index 0c0dc2e369c0..7c4e257c0a81 100644
--- a/drivers/of/property.c
+++ b/drivers/of/property.c
@@ -1292,6 +1292,7 @@ DEFINE_SIMPLE_PROP(resets, "resets", "#reset-cells")
 DEFINE_SIMPLE_PROP(leds, "leds", NULL)
 DEFINE_SIMPLE_PROP(backlight, "backlight", NULL)
 DEFINE_SIMPLE_PROP(phy_handle, "phy-handle", NULL)
+DEFINE_SIMPLE_PROP(mdio_parent_bus, "mdio-parent-bus", NULL);
 DEFINE_SUFFIX_PROP(regulators, "-supply", NULL)
 DEFINE_SUFFIX_PROP(gpio, "-gpio", "#gpio-cells")
 
@@ -1381,6 +1382,7 @@ static const struct supplier_bindings of_supplier_bindings[] = {
        { .parse_prop = parse_leds, },
        { .parse_prop = parse_backlight, },
        { .parse_prop = parse_phy_handle, },
+       { .parse_prop = parse_mdio_parent_bus, },
        { .parse_prop = parse_gpio_compat, },
        { .parse_prop = parse_interrupts, },
        { .parse_prop = parse_regulators, },

	Andrew

Re: [PATCH v2] of: property: fw_devlink: Add support for "phy-handle" property

[Saravana Kannan]

On Mon, Aug 23, 2021 at 6:16 AM Andrew Lunn <andrew@lunn.ch> wrote:
>
> On Mon, Aug 23, 2021 at 02:08:48PM +0200, Marek Szyprowski wrote:
> > Hi,
> >
> > On 18.08.2021 04:17, Saravana Kannan wrote:
> > > Allows tracking dependencies between Ethernet PHYs and their consumers.
> > >
> > > Cc: Andrew Lunn <andrew@lunn.ch>
> > > Cc: netdev@vger.kernel.org
> > > Signed-off-by: Saravana Kannan <saravanak@google.com>
> >
> > This patch landed recently in linux-next as commit cf4b94c8530d ("of:
> > property: fw_devlink: Add support for "phy-handle" property"). It breaks
> > ethernet operation on my Amlogic-based ARM64 boards: Odroid C4
> > (arm64/boot/dts/amlogic/meson-sm1-odroid-c4.dts) and N2
> > (meson-g12b-odroid-n2.dts) as well as Khadas VIM3/VIM3l
> > (meson-g12b-a311d-khadas-vim3.dts and meson-sm1-khadas-vim3l.dts).
> >
> > In case of OdroidC4 I see the following entries in the
> > /sys/kernel/debug/devices_deferred:
> >
> > ff64c000.mdio-multiplexer
> > ff3f0000.ethernet
> >
> > Let me know if there is anything I can check to help debugging this issue.
>
> Hi Marek
>
> Please try this. Completetly untested, not even compile teseted:
>
> diff --git a/drivers/of/property.c b/drivers/of/property.c
> index 0c0dc2e369c0..7c4e257c0a81 100644
> --- a/drivers/of/property.c
> +++ b/drivers/of/property.c
> @@ -1292,6 +1292,7 @@ DEFINE_SIMPLE_PROP(resets, "resets", "#reset-cells")
>  DEFINE_SIMPLE_PROP(leds, "leds", NULL)
>  DEFINE_SIMPLE_PROP(backlight, "backlight", NULL)
>  DEFINE_SIMPLE_PROP(phy_handle, "phy-handle", NULL)
> +DEFINE_SIMPLE_PROP(mdio_parent_bus, "mdio-parent-bus", NULL);
>  DEFINE_SUFFIX_PROP(regulators, "-supply", NULL)
>  DEFINE_SUFFIX_PROP(gpio, "-gpio", "#gpio-cells")
>
> @@ -1381,6 +1382,7 @@ static const struct supplier_bindings of_supplier_bindings[] = {
>         { .parse_prop = parse_leds, },
>         { .parse_prop = parse_backlight, },
>         { .parse_prop = parse_phy_handle, },
> +       { .parse_prop = parse_mdio_parent_bus, },
>         { .parse_prop = parse_gpio_compat, },
>         { .parse_prop = parse_interrupts, },
>         { .parse_prop = parse_regulators, },

Looking at the code, I'm fairly certain that the device that
corresponds to a DT node pointed to by mdio-parent-bus will be a "bus"
device that's registered with the mdio_bus_class.

If my understanding is right, then Nak for this patch. It'll break a
lot of probes.

TL;DR is that stateful/managed device links don't make sense for
devices that are never probed/bound to a driver. I plan to improve
device links to try and accommodate these cases nicely, but that's in
my TO DO list. Until that's completed, this patch will break stuff.

-Saravana

Re: [PATCH v2] of: property: fw_devlink: Add support for "phy-handle" property

[Andrew Lunn]

On Mon, Aug 23, 2021 at 11:13:08AM -0700, Saravana Kannan wrote:
> On Mon, Aug 23, 2021 at 6:16 AM Andrew Lunn <andrew@lunn.ch> wrote:
> >
> > On Mon, Aug 23, 2021 at 02:08:48PM +0200, Marek Szyprowski wrote:
> > > Hi,
> > >
> > > On 18.08.2021 04:17, Saravana Kannan wrote:
> > > > Allows tracking dependencies between Ethernet PHYs and their consumers.
> > > >
> > > > Cc: Andrew Lunn <andrew@lunn.ch>
> > > > Cc: netdev@vger.kernel.org
> > > > Signed-off-by: Saravana Kannan <saravanak@google.com>
> > >
> > > This patch landed recently in linux-next as commit cf4b94c8530d ("of:
> > > property: fw_devlink: Add support for "phy-handle" property"). It breaks
> > > ethernet operation on my Amlogic-based ARM64 boards: Odroid C4
> > > (arm64/boot/dts/amlogic/meson-sm1-odroid-c4.dts) and N2
> > > (meson-g12b-odroid-n2.dts) as well as Khadas VIM3/VIM3l
> > > (meson-g12b-a311d-khadas-vim3.dts and meson-sm1-khadas-vim3l.dts).
> > >
> > > In case of OdroidC4 I see the following entries in the
> > > /sys/kernel/debug/devices_deferred:
> > >
> > > ff64c000.mdio-multiplexer
> > > ff3f0000.ethernet
> > >
> > > Let me know if there is anything I can check to help debugging this issue.
> >
> > Hi Marek
> >
> > Please try this. Completetly untested, not even compile teseted:
> >
> > diff --git a/drivers/of/property.c b/drivers/of/property.c
> > index 0c0dc2e369c0..7c4e257c0a81 100644
> > --- a/drivers/of/property.c
> > +++ b/drivers/of/property.c
> > @@ -1292,6 +1292,7 @@ DEFINE_SIMPLE_PROP(resets, "resets", "#reset-cells")
> >  DEFINE_SIMPLE_PROP(leds, "leds", NULL)
> >  DEFINE_SIMPLE_PROP(backlight, "backlight", NULL)
> >  DEFINE_SIMPLE_PROP(phy_handle, "phy-handle", NULL)
> > +DEFINE_SIMPLE_PROP(mdio_parent_bus, "mdio-parent-bus", NULL);
> >  DEFINE_SUFFIX_PROP(regulators, "-supply", NULL)
> >  DEFINE_SUFFIX_PROP(gpio, "-gpio", "#gpio-cells")
> >
> > @@ -1381,6 +1382,7 @@ static const struct supplier_bindings of_supplier_bindings[] = {
> >         { .parse_prop = parse_leds, },
> >         { .parse_prop = parse_backlight, },
> >         { .parse_prop = parse_phy_handle, },
> > +       { .parse_prop = parse_mdio_parent_bus, },
> >         { .parse_prop = parse_gpio_compat, },
> >         { .parse_prop = parse_interrupts, },
> >         { .parse_prop = parse_regulators, },
> 
> Looking at the code, I'm fairly certain that the device that
> corresponds to a DT node pointed to by mdio-parent-bus will be a "bus"
> device that's registered with the mdio_bus_class.
> 
> If my understanding is right, then Nak for this patch. It'll break a
> lot of probes.
> 
> TL;DR is that stateful/managed device links don't make sense for
> devices that are never probed/bound to a driver.

So some more background, which might help you get an idea what is
going on here, and what you will need to implement.

There are a number of different ways an mdio bus driver can come into
existence.

They can be classical devices, which are described in device tree and
probed in the normal way. Most of the mdio bus drivers in
driver/net/mdio are like this, and they have documented bindings, and
compatible strings, e.g. Documentation/devicetree/bindings/net/marvell-orion-mdio.txt

Multiplexers, which are probably a subclass of the above classical
devices. They have documented binds and compatible strings. They link
to another MDIO bus, and some other resource to switch the
multiplexor, e.g, GPIOs, a MMIO register, a Linux multiplexer.

They can be embedded inside some other device, typically an Ethernet
controller, but also a Ethernet switch. In this case, the parent
device should have an MDIO node in its device tree. An example would
be the freescale FEC
Documentation/devicetree/bindings/net/fsl,fec.yaml So if you are
trying to fulfil dependencies for this sort of mdio bus, you need to
probe the FEC driver, and as a side effect, the MDIO bus driver will
pop into existence.

    Andrew

Re: [PATCH v2] of: property: fw_devlink: Add support for "phy-handle" property

[Marek Szyprowski]

Hi Andrew,

On 23.08.2021 15:16, Andrew Lunn wrote:
> On Mon, Aug 23, 2021 at 02:08:48PM +0200, Marek Szyprowski wrote:
>> On 18.08.2021 04:17, Saravana Kannan wrote:
>>> Allows tracking dependencies between Ethernet PHYs and their consumers.
>>>
>>> Cc: Andrew Lunn <andrew@lunn.ch>
>>> Cc: netdev@vger.kernel.org
>>> Signed-off-by: Saravana Kannan <saravanak@google.com>
>> This patch landed recently in linux-next as commit cf4b94c8530d ("of:
>> property: fw_devlink: Add support for "phy-handle" property"). It breaks
>> ethernet operation on my Amlogic-based ARM64 boards: Odroid C4
>> (arm64/boot/dts/amlogic/meson-sm1-odroid-c4.dts) and N2
>> (meson-g12b-odroid-n2.dts) as well as Khadas VIM3/VIM3l
>> (meson-g12b-a311d-khadas-vim3.dts and meson-sm1-khadas-vim3l.dts).
>>
>> In case of OdroidC4 I see the following entries in the
>> /sys/kernel/debug/devices_deferred:
>>
>> ff64c000.mdio-multiplexer
>> ff3f0000.ethernet
>>
>> Let me know if there is anything I can check to help debugging this issue.
> Hi Marek
>
> Please try this. Completetly untested, not even compile teseted:

Nope, this doesn't help in this case.

> diff --git a/drivers/of/property.c b/drivers/of/property.c
> index 0c0dc2e369c0..7c4e257c0a81 100644
> --- a/drivers/of/property.c
> +++ b/drivers/of/property.c
> @@ -1292,6 +1292,7 @@ DEFINE_SIMPLE_PROP(resets, "resets", "#reset-cells")
>   DEFINE_SIMPLE_PROP(leds, "leds", NULL)
>   DEFINE_SIMPLE_PROP(backlight, "backlight", NULL)
>   DEFINE_SIMPLE_PROP(phy_handle, "phy-handle", NULL)
> +DEFINE_SIMPLE_PROP(mdio_parent_bus, "mdio-parent-bus", NULL);
>   DEFINE_SUFFIX_PROP(regulators, "-supply", NULL)
>   DEFINE_SUFFIX_PROP(gpio, "-gpio", "#gpio-cells")
>   
> @@ -1381,6 +1382,7 @@ static const struct supplier_bindings of_supplier_bindings[] = {
>          { .parse_prop = parse_leds, },
>          { .parse_prop = parse_backlight, },
>          { .parse_prop = parse_phy_handle, },
> +       { .parse_prop = parse_mdio_parent_bus, },
>          { .parse_prop = parse_gpio_compat, },
>          { .parse_prop = parse_interrupts, },
>          { .parse_prop = parse_regulators, },
>
> 	Andrew
>
Best regards
-- 
Marek Szyprowski, PhD
Samsung R&D Institute Poland

Re: [PATCH v2] of: property: fw_devlink: Add support for "phy-handle" property

[Saravana Kannan]

On Mon, Aug 23, 2021 at 5:08 AM Marek Szyprowski
<m.szyprowski@samsung.com> wrote:
>
> Hi,
>
> On 18.08.2021 04:17, Saravana Kannan wrote:
> > Allows tracking dependencies between Ethernet PHYs and their consumers.
> >
> > Cc: Andrew Lunn <andrew@lunn.ch>
> > Cc: netdev@vger.kernel.org
> > Signed-off-by: Saravana Kannan <saravanak@google.com>
>
> This patch landed recently in linux-next as commit cf4b94c8530d ("of:
> property: fw_devlink: Add support for "phy-handle" property"). It breaks
> ethernet operation on my Amlogic-based ARM64 boards: Odroid C4
> (arm64/boot/dts/amlogic/meson-sm1-odroid-c4.dts) and N2
> (meson-g12b-odroid-n2.dts) as well as Khadas VIM3/VIM3l
> (meson-g12b-a311d-khadas-vim3.dts and meson-sm1-khadas-vim3l.dts).
>
> In case of OdroidC4 I see the following entries in the
> /sys/kernel/debug/devices_deferred:
>
> ff64c000.mdio-multiplexer
> ff3f0000.ethernet
>
> Let me know if there is anything I can check to help debugging this issue.

I'm fairly certain you are hitting this issue because the PHY device
doesn't have a compatible property. And so the device link dependency
is propagated up to the mdio bus. But busses as suppliers aren't good
because busses never "probe".

PHY seems to be one of those cases where it's okay to have the
compatible property but also okay to not have it. You can confirm my
theory by checking for the list of suppliers under
ff64c000.mdio-multiplexer. You'd see mdio@0 (ext_mdio) and if you look
at the "status" file under the folder it should be "dormant". If you
add a compatible property that fits the formats a PHY node can have,
that should also fix your issue (not the solution though).

I'll send out a fix this week (once you confirm my analysis). Thanks
for reporting it.

-Saravana

>
> > ---
> > v1 -> v2:
> > - Fixed patch to address my misunderstanding of how PHYs get
> >    initialized.
> >
> >   drivers/of/property.c | 2 ++
> >   1 file changed, 2 insertions(+)
> >
> > diff --git a/drivers/of/property.c b/drivers/of/property.c
> > index 931340329414..0c0dc2e369c0 100644
> > --- a/drivers/of/property.c
> > +++ b/drivers/of/property.c
> > @@ -1291,6 +1291,7 @@ DEFINE_SIMPLE_PROP(pwms, "pwms", "#pwm-cells")
> >   DEFINE_SIMPLE_PROP(resets, "resets", "#reset-cells")
> >   DEFINE_SIMPLE_PROP(leds, "leds", NULL)
> >   DEFINE_SIMPLE_PROP(backlight, "backlight", NULL)
> > +DEFINE_SIMPLE_PROP(phy_handle, "phy-handle", NULL)
> >   DEFINE_SUFFIX_PROP(regulators, "-supply", NULL)
> >   DEFINE_SUFFIX_PROP(gpio, "-gpio", "#gpio-cells")
> >
> > @@ -1379,6 +1380,7 @@ static const struct supplier_bindings of_supplier_bindings[] = {
> >       { .parse_prop = parse_resets, },
> >       { .parse_prop = parse_leds, },
> >       { .parse_prop = parse_backlight, },
> > +     { .parse_prop = parse_phy_handle, },
> >       { .parse_prop = parse_gpio_compat, },
> >       { .parse_prop = parse_interrupts, },
> >       { .parse_prop = parse_regulators, },
>
> Best regards
> --
> Marek Szyprowski, PhD
> Samsung R&D Institute Poland
>

Re: [PATCH v2] of: property: fw_devlink: Add support for "phy-handle" property

[Andrew Lunn]

> PHY seems to be one of those cases where it's okay to have the
> compatible property but also okay to not have it.

Correct. They are like PCI or USB devices. You can ask it, what are
you? There are two registers in standard locations which give you a
vendor and product ID. We use that to find the correct driver.

You only need a compatible when things are not so simple.

1) The IDs are wrong. Some silicon vendors do stupid things

2) Chicken/egg problems, you cannot read the ID registers until you
   load the driver and some resource is enabled.

3) It is a C45 devices, e.g. part of clause 45 of 802.3, which
   requires a different protocol to be talked over the bus. So the
   compatible string tells you to talk C45 to get the IDs.

4) It is not a PHY, but some sort of other MDIO device, and hence
   there are no ID registers.

Andrew

Re: [PATCH v2] of: property: fw_devlink: Add support for "phy-handle" property

[Saravana Kannan]

On Mon, Aug 23, 2021 at 12:58 PM Andrew Lunn <andrew@lunn.ch> wrote:
>
> > PHY seems to be one of those cases where it's okay to have the
> > compatible property but also okay to not have it.
>
> Correct. They are like PCI or USB devices. You can ask it, what are
> you? There are two registers in standard locations which give you a
> vendor and product ID. We use that to find the correct driver.

For all the cases of PHYs that currently don't need any compatible
string, requiring a compatible string of type "ethernet-phy-standard"
would have been nice. That would have made PHYs consistent with the
general DT norm of "you need a compatible string to be matched with
the device". Anyway, it's too late to do that now. So I'll have to
deal with this some other way (I have a bunch of ideas, so it's not
the end of the world).

> You only need a compatible when things are not so simple.
>
> 1) The IDs are wrong. Some silicon vendors do stupid things
>
> 2) Chicken/egg problems, you cannot read the ID registers until you
>    load the driver and some resource is enabled.
>
> 3) It is a C45 devices, e.g. part of clause 45 of 802.3, which
>    requires a different protocol to be talked over the bus. So the
>    compatible string tells you to talk C45 to get the IDs.
>
> 4) It is not a PHY, but some sort of other MDIO device, and hence
>    there are no ID registers.

Yeah, I was digging through of_mdiobus_child_is_phy() when I was doing
the mdio-mux fixes and noticed this. But I missed/forgot the mdiobus
doesn't probe part when I sent out the phy-handle patch.

-Saravana

Re: [PATCH v2] of: property: fw_devlink: Add support for "phy-handle" property

[Andrew Lunn]

On Mon, Aug 23, 2021 at 01:48:23PM -0700, Saravana Kannan wrote:
> On Mon, Aug 23, 2021 at 12:58 PM Andrew Lunn <andrew@lunn.ch> wrote:
> >
> > > PHY seems to be one of those cases where it's okay to have the
> > > compatible property but also okay to not have it.
> >
> > Correct. They are like PCI or USB devices. You can ask it, what are
> > you? There are two registers in standard locations which give you a
> > vendor and product ID. We use that to find the correct driver.
> 
> For all the cases of PHYs that currently don't need any compatible
> string, requiring a compatible string of type "ethernet-phy-standard"
> would have been nice.

How does this help you? You cannot match that against anything?

How do you handle PCI and USB devices? e.g.

arch/arm/boot/dts/imx6qdl-zii-rdu2.dtsi

&pcie {
        pinctrl-names = "default";
        pinctrl-0 = <&pinctrl_pcie>;
        reset-gpio = <&gpio7 12 GPIO_ACTIVE_LOW>;
        status = "okay";

        host@0 {
                reg = <0 0 0 0 0>;

                #address-cells = <3>;
                #size-cells = <2>;

                i210: i210@0 {
                        reg = <0 0 0 0 0>;
                };
        };
};

There is an intel i210 Ethernet control on the PCIe bus. There is no
compatible string, none is needed. This is no different to a PHY.

	   Andrew

Re: [PATCH v2] of: property: fw_devlink: Add support for "phy-handle" property

[Rob Herring]

On Mon, Aug 23, 2021 at 3:49 PM Saravana Kannan <saravanak@google.com> wrote:
>
> On Mon, Aug 23, 2021 at 12:58 PM Andrew Lunn <andrew@lunn.ch> wrote:
> >
> > > PHY seems to be one of those cases where it's okay to have the
> > > compatible property but also okay to not have it.
> >
> > Correct. They are like PCI or USB devices. You can ask it, what are
> > you? There are two registers in standard locations which give you a
> > vendor and product ID. We use that to find the correct driver.
>
> For all the cases of PHYs that currently don't need any compatible
> string, requiring a compatible string of type "ethernet-phy-standard"
> would have been nice. That would have made PHYs consistent with the
> general DT norm of "you need a compatible string to be matched with
> the device". Anyway, it's too late to do that now. So I'll have to
> deal with this some other way (I have a bunch of ideas, so it's not
> the end of the world).

This is not the first time the need for compatible strings for MDIO
devices has come up, but MDIO devices are special (evidently). I
should have taken a harder stance on this which should be simply, if
your device requires having a node in DT, then it requires a
compatible string.

Rob

Re: [PATCH v2] of: property: fw_devlink: Add support for "phy-handle" property

[Marek Szyprowski]

Hi,

On 23.08.2021 20:22, Saravana Kannan wrote:
> On Mon, Aug 23, 2021 at 5:08 AM Marek Szyprowski
> <m.szyprowski@samsung.com> wrote:
>> On 18.08.2021 04:17, Saravana Kannan wrote:
>>> Allows tracking dependencies between Ethernet PHYs and their consumers.
>>>
>>> Cc: Andrew Lunn <andrew@lunn.ch>
>>> Cc: netdev@vger.kernel.org
>>> Signed-off-by: Saravana Kannan <saravanak@google.com>
>> This patch landed recently in linux-next as commit cf4b94c8530d ("of:
>> property: fw_devlink: Add support for "phy-handle" property"). It breaks
>> ethernet operation on my Amlogic-based ARM64 boards: Odroid C4
>> (arm64/boot/dts/amlogic/meson-sm1-odroid-c4.dts) and N2
>> (meson-g12b-odroid-n2.dts) as well as Khadas VIM3/VIM3l
>> (meson-g12b-a311d-khadas-vim3.dts and meson-sm1-khadas-vim3l.dts).
>>
>> In case of OdroidC4 I see the following entries in the
>> /sys/kernel/debug/devices_deferred:
>>
>> ff64c000.mdio-multiplexer
>> ff3f0000.ethernet
>>
>> Let me know if there is anything I can check to help debugging this issue.
> I'm fairly certain you are hitting this issue because the PHY device
> doesn't have a compatible property. And so the device link dependency
> is propagated up to the mdio bus. But busses as suppliers aren't good
> because busses never "probe".
>
> PHY seems to be one of those cases where it's okay to have the
> compatible property but also okay to not have it. You can confirm my
> theory by checking for the list of suppliers under
> ff64c000.mdio-multiplexer. You'd see mdio@0 (ext_mdio) and if you look
> at the "status" file under the folder it should be "dormant". If you
> add a compatible property that fits the formats a PHY node can have,
> that should also fix your issue (not the solution though).

Where should I look for the mentioned device links 'status' file?

# find /sys -name ff64c000.mdio-multiplexer
/sys/devices/platform/soc/ff600000.bus/ff64c000.mdio-multiplexer
/sys/bus/platform/devices/ff64c000.mdio-multiplexer

# ls -l /sys/devices/platform/soc/ff600000.bus/ff64c000.mdio-multiplexer
total 0
lrwxrwxrwx 1 root root    0 Jan  1 00:04 
consumer:platform:ff3f0000.ethernet -> 
../../../../virtual/devlink/platform:ff64c000.mdio-multiplexer--platform:ff3f0000.ethernet
-rw-r--r-- 1 root root 4096 Jan  1 00:04 driver_override
-r--r--r-- 1 root root 4096 Jan  1 00:04 modalias
lrwxrwxrwx 1 root root    0 Jan  1 00:04 of_node -> 
../../../../../firmware/devicetree/base/soc/bus@ff600000/mdio-multiplexer@4c000
drwxr-xr-x 2 root root    0 Jan  1 00:02 power
lrwxrwxrwx 1 root root    0 Jan  1 00:04 subsystem -> 
../../../../../bus/platform
lrwxrwxrwx 1 root root    0 Jan  1 00:04 
supplier:platform:ff63c000.system-controller:clock-controller -> 
../../../../virtual/devlink/platform:ff63c000.system-controller:clock-controller--platform:ff64c000.mdio-multiplexer
-rw-r--r-- 1 root root 4096 Jan  1 00:04 uevent
-r--r--r-- 1 root root 4096 Jan  1 00:04 waiting_for_supplier

# cat 
/sys/devices/platform/soc/ff600000.bus/ff64c000.mdio-multiplexer/waiting_for_supplier
0

I'm also not sure what compatible string should I add there.

> I'll send out a fix this week (once you confirm my analysis). Thanks
> for reporting it.

Best regards

-- 
Marek Szyprowski, PhD
Samsung R&D Institute Poland

Re: [PATCH v2] of: property: fw_devlink: Add support for "phy-handle" property

[Saravana Kannan]

On Tue, Aug 24, 2021 at 12:03 AM Marek Szyprowski
<m.szyprowski@samsung.com> wrote:
>
> Hi,
>
> On 23.08.2021 20:22, Saravana Kannan wrote:
> > On Mon, Aug 23, 2021 at 5:08 AM Marek Szyprowski
> > <m.szyprowski@samsung.com> wrote:
> >> On 18.08.2021 04:17, Saravana Kannan wrote:
> >>> Allows tracking dependencies between Ethernet PHYs and their consumers.
> >>>
> >>> Cc: Andrew Lunn <andrew@lunn.ch>
> >>> Cc: netdev@vger.kernel.org
> >>> Signed-off-by: Saravana Kannan <saravanak@google.com>
> >> This patch landed recently in linux-next as commit cf4b94c8530d ("of:
> >> property: fw_devlink: Add support for "phy-handle" property"). It breaks
> >> ethernet operation on my Amlogic-based ARM64 boards: Odroid C4
> >> (arm64/boot/dts/amlogic/meson-sm1-odroid-c4.dts) and N2
> >> (meson-g12b-odroid-n2.dts) as well as Khadas VIM3/VIM3l
> >> (meson-g12b-a311d-khadas-vim3.dts and meson-sm1-khadas-vim3l.dts).
> >>
> >> In case of OdroidC4 I see the following entries in the
> >> /sys/kernel/debug/devices_deferred:
> >>
> >> ff64c000.mdio-multiplexer
> >> ff3f0000.ethernet
> >>
> >> Let me know if there is anything I can check to help debugging this issue.
> > I'm fairly certain you are hitting this issue because the PHY device
> > doesn't have a compatible property. And so the device link dependency
> > is propagated up to the mdio bus. But busses as suppliers aren't good
> > because busses never "probe".
> >
> > PHY seems to be one of those cases where it's okay to have the
> > compatible property but also okay to not have it. You can confirm my
> > theory by checking for the list of suppliers under
> > ff64c000.mdio-multiplexer. You'd see mdio@0 (ext_mdio) and if you look
> > at the "status" file under the folder it should be "dormant". If you
> > add a compatible property that fits the formats a PHY node can have,
> > that should also fix your issue (not the solution though).
>
> Where should I look for the mentioned device links 'status' file?
>
> # find /sys -name ff64c000.mdio-multiplexer
> /sys/devices/platform/soc/ff600000.bus/ff64c000.mdio-multiplexer
> /sys/bus/platform/devices/ff64c000.mdio-multiplexer
>
> # ls -l /sys/devices/platform/soc/ff600000.bus/ff64c000.mdio-multiplexer
> total 0

This is the folder I wanted you to check.

> lrwxrwxrwx 1 root root    0 Jan  1 00:04
> consumer:platform:ff3f0000.ethernet ->
> ../../../../virtual/devlink/platform:ff64c000.mdio-multiplexer--platform:ff3f0000.ethernet

But I should have asked to look for the consumer list and not the
supplier list. In any case, we can see that the ethernet is marked as
the consumer of the mdio-multiplexer instead of the PHY device. So my
hunch seems to be right.

> -rw-r--r-- 1 root root 4096 Jan  1 00:04 driver_override
> -r--r--r-- 1 root root 4096 Jan  1 00:04 modalias
> lrwxrwxrwx 1 root root    0 Jan  1 00:04 of_node ->
> ../../../../../firmware/devicetree/base/soc/bus@ff600000/mdio-multiplexer@4c000
> drwxr-xr-x 2 root root    0 Jan  1 00:02 power
> lrwxrwxrwx 1 root root    0 Jan  1 00:04 subsystem ->
> ../../../../../bus/platform
> lrwxrwxrwx 1 root root    0 Jan  1 00:04
> supplier:platform:ff63c000.system-controller:clock-controller ->
> ../../../../virtual/devlink/platform:ff63c000.system-controller:clock-controller--platform:ff64c000.mdio-multiplexer
> -rw-r--r-- 1 root root 4096 Jan  1 00:04 uevent
> -r--r--r-- 1 root root 4096 Jan  1 00:04 waiting_for_supplier
>
> # cat
> /sys/devices/platform/soc/ff600000.bus/ff64c000.mdio-multiplexer/waiting_for_supplier
> 0
>
> I'm also not sure what compatible string should I add there.

It should have been added to external_phy: ethernet-phy@0. But don't
worry about it (because you need to use a specific format for the
compatible string).

-Saravana

>
> > I'll send out a fix this week (once you confirm my analysis). Thanks
> > for reporting it.
>
> Best regards
>
> --
> Marek Szyprowski, PhD
> Samsung R&D Institute Poland
>

Re: [PATCH v2] of: property: fw_devlink: Add support for "phy-handle" property

[Saravana Kannan]

On Tue, Aug 24, 2021 at 12:31 AM Saravana Kannan <saravanak@google.com> wrote:
>
> On Tue, Aug 24, 2021 at 12:03 AM Marek Szyprowski
> <m.szyprowski@samsung.com> wrote:
> >
> > Hi,
> >
> > On 23.08.2021 20:22, Saravana Kannan wrote:
> > > On Mon, Aug 23, 2021 at 5:08 AM Marek Szyprowski
> > > <m.szyprowski@samsung.com> wrote:
> > >> On 18.08.2021 04:17, Saravana Kannan wrote:
> > >>> Allows tracking dependencies between Ethernet PHYs and their consumers.
> > >>>
> > >>> Cc: Andrew Lunn <andrew@lunn.ch>
> > >>> Cc: netdev@vger.kernel.org
> > >>> Signed-off-by: Saravana Kannan <saravanak@google.com>
> > >> This patch landed recently in linux-next as commit cf4b94c8530d ("of:
> > >> property: fw_devlink: Add support for "phy-handle" property"). It breaks
> > >> ethernet operation on my Amlogic-based ARM64 boards: Odroid C4
> > >> (arm64/boot/dts/amlogic/meson-sm1-odroid-c4.dts) and N2
> > >> (meson-g12b-odroid-n2.dts) as well as Khadas VIM3/VIM3l
> > >> (meson-g12b-a311d-khadas-vim3.dts and meson-sm1-khadas-vim3l.dts).
> > >>
> > >> In case of OdroidC4 I see the following entries in the
> > >> /sys/kernel/debug/devices_deferred:
> > >>
> > >> ff64c000.mdio-multiplexer
> > >> ff3f0000.ethernet
> > >>
> > >> Let me know if there is anything I can check to help debugging this issue.
> > > I'm fairly certain you are hitting this issue because the PHY device
> > > doesn't have a compatible property. And so the device link dependency
> > > is propagated up to the mdio bus. But busses as suppliers aren't good
> > > because busses never "probe".
> > >
> > > PHY seems to be one of those cases where it's okay to have the
> > > compatible property but also okay to not have it. You can confirm my
> > > theory by checking for the list of suppliers under
> > > ff64c000.mdio-multiplexer. You'd see mdio@0 (ext_mdio) and if you look
> > > at the "status" file under the folder it should be "dormant". If you
> > > add a compatible property that fits the formats a PHY node can have,
> > > that should also fix your issue (not the solution though).
> >
> > Where should I look for the mentioned device links 'status' file?
> >
> > # find /sys -name ff64c000.mdio-multiplexer
> > /sys/devices/platform/soc/ff600000.bus/ff64c000.mdio-multiplexer
> > /sys/bus/platform/devices/ff64c000.mdio-multiplexer
> >
> > # ls -l /sys/devices/platform/soc/ff600000.bus/ff64c000.mdio-multiplexer
> > total 0
>
> This is the folder I wanted you to check.
>
> > lrwxrwxrwx 1 root root    0 Jan  1 00:04
> > consumer:platform:ff3f0000.ethernet ->
> > ../../../../virtual/devlink/platform:ff64c000.mdio-multiplexer--platform:ff3f0000.ethernet
>
> But I should have asked to look for the consumer list and not the
> supplier list. In any case, we can see that the ethernet is marked as
> the consumer of the mdio-multiplexer instead of the PHY device. So my
> hunch seems to be right.
>
> > -rw-r--r-- 1 root root 4096 Jan  1 00:04 driver_override
> > -r--r--r-- 1 root root 4096 Jan  1 00:04 modalias
> > lrwxrwxrwx 1 root root    0 Jan  1 00:04 of_node ->
> > ../../../../../firmware/devicetree/base/soc/bus@ff600000/mdio-multiplexer@4c000
> > drwxr-xr-x 2 root root    0 Jan  1 00:02 power
> > lrwxrwxrwx 1 root root    0 Jan  1 00:04 subsystem ->
> > ../../../../../bus/platform
> > lrwxrwxrwx 1 root root    0 Jan  1 00:04
> > supplier:platform:ff63c000.system-controller:clock-controller ->
> > ../../../../virtual/devlink/platform:ff63c000.system-controller:clock-controller--platform:ff64c000.mdio-multiplexer
> > -rw-r--r-- 1 root root 4096 Jan  1 00:04 uevent
> > -r--r--r-- 1 root root 4096 Jan  1 00:04 waiting_for_supplier
> >
> > # cat
> > /sys/devices/platform/soc/ff600000.bus/ff64c000.mdio-multiplexer/waiting_for_supplier
> > 0
> >
> > I'm also not sure what compatible string should I add there.
>
> It should have been added to external_phy: ethernet-phy@0. But don't
> worry about it (because you need to use a specific format for the
> compatible string).
>

Marek,

Can you give this a shot?
https://lore.kernel.org/lkml/20210831224510.703253-1-saravanak@google.com/

This is not the main fix for the case you brought up, but it should
fix your issue as a side effect of fixing another issue.

The main fix for your issue would be to teach fw_devlink that
phy-handle always points to the actual DT node that'll become a device
even if it doesn't have a compatible property. I'll send that out
later.

-Saravana

Re: [PATCH v2] of: property: fw_devlink: Add support for "phy-handle" property

[Marek Szyprowski]

Hi Saravana,

On 01.09.2021 04:37, Saravana Kannan wrote:
> On Tue, Aug 24, 2021 at 12:31 AM Saravana Kannan <saravanak@google.com> wrote:
>> On Tue, Aug 24, 2021 at 12:03 AM Marek Szyprowski
>> <m.szyprowski@samsung.com> wrote:
>>> On 23.08.2021 20:22, Saravana Kannan wrote:
>>>> On Mon, Aug 23, 2021 at 5:08 AM Marek Szyprowski
>>>> <m.szyprowski@samsung.com> wrote:
>>>>> On 18.08.2021 04:17, Saravana Kannan wrote:
>>>>>> Allows tracking dependencies between Ethernet PHYs and their consumers.
>>>>>>
>>>>>> Cc: Andrew Lunn <andrew@lunn.ch>
>>>>>> Cc: netdev@vger.kernel.org
>>>>>> Signed-off-by: Saravana Kannan <saravanak@google.com>
>>>>> This patch landed recently in linux-next as commit cf4b94c8530d ("of:
>>>>> property: fw_devlink: Add support for "phy-handle" property"). It breaks
>>>>> ethernet operation on my Amlogic-based ARM64 boards: Odroid C4
>>>>> (arm64/boot/dts/amlogic/meson-sm1-odroid-c4.dts) and N2
>>>>> (meson-g12b-odroid-n2.dts) as well as Khadas VIM3/VIM3l
>>>>> (meson-g12b-a311d-khadas-vim3.dts and meson-sm1-khadas-vim3l.dts).
>>>>>
>>>>> In case of OdroidC4 I see the following entries in the
>>>>> /sys/kernel/debug/devices_deferred:
>>>>>
>>>>> ff64c000.mdio-multiplexer
>>>>> ff3f0000.ethernet
>>>>>
>>>>> Let me know if there is anything I can check to help debugging this issue.
>>>> I'm fairly certain you are hitting this issue because the PHY device
>>>> doesn't have a compatible property. And so the device link dependency
>>>> is propagated up to the mdio bus. But busses as suppliers aren't good
>>>> because busses never "probe".
>>>>
>>>> PHY seems to be one of those cases where it's okay to have the
>>>> compatible property but also okay to not have it. You can confirm my
>>>> theory by checking for the list of suppliers under
>>>> ff64c000.mdio-multiplexer. You'd see mdio@0 (ext_mdio) and if you look
>>>> at the "status" file under the folder it should be "dormant". If you
>>>> add a compatible property that fits the formats a PHY node can have,
>>>> that should also fix your issue (not the solution though).
>>> Where should I look for the mentioned device links 'status' file?
>>>
>>> # find /sys -name ff64c000.mdio-multiplexer
>>> /sys/devices/platform/soc/ff600000.bus/ff64c000.mdio-multiplexer
>>> /sys/bus/platform/devices/ff64c000.mdio-multiplexer
>>>
>>> # ls -l /sys/devices/platform/soc/ff600000.bus/ff64c000.mdio-multiplexer
>>> total 0
>> This is the folder I wanted you to check.
>>
>>> lrwxrwxrwx 1 root root    0 Jan  1 00:04
>>> consumer:platform:ff3f0000.ethernet ->
>>> ../../../../virtual/devlink/platform:ff64c000.mdio-multiplexer--platform:ff3f0000.ethernet
>> But I should have asked to look for the consumer list and not the
>> supplier list. In any case, we can see that the ethernet is marked as
>> the consumer of the mdio-multiplexer instead of the PHY device. So my
>> hunch seems to be right.
>>
>>> -rw-r--r-- 1 root root 4096 Jan  1 00:04 driver_override
>>> -r--r--r-- 1 root root 4096 Jan  1 00:04 modalias
>>> lrwxrwxrwx 1 root root    0 Jan  1 00:04 of_node ->
>>> ../../../../../firmware/devicetree/base/soc/bus@ff600000/mdio-multiplexer@4c000
>>> drwxr-xr-x 2 root root    0 Jan  1 00:02 power
>>> lrwxrwxrwx 1 root root    0 Jan  1 00:04 subsystem ->
>>> ../../../../../bus/platform
>>> lrwxrwxrwx 1 root root    0 Jan  1 00:04
>>> supplier:platform:ff63c000.system-controller:clock-controller ->
>>> ../../../../virtual/devlink/platform:ff63c000.system-controller:clock-controller--platform:ff64c000.mdio-multiplexer
>>> -rw-r--r-- 1 root root 4096 Jan  1 00:04 uevent
>>> -r--r--r-- 1 root root 4096 Jan  1 00:04 waiting_for_supplier
>>>
>>> # cat
>>> /sys/devices/platform/soc/ff600000.bus/ff64c000.mdio-multiplexer/waiting_for_supplier
>>> 0
>>>
>>> I'm also not sure what compatible string should I add there.
>> It should have been added to external_phy: ethernet-phy@0. But don't
>> worry about it (because you need to use a specific format for the
>> compatible string).
>>
> Marek,
>
> Can you give this a shot?
> https://lore.kernel.org/lkml/20210831224510.703253-1-saravanak@google.com/
>
> This is not the main fix for the case you brought up, but it should
> fix your issue as a side effect of fixing another issue.

I've just checked it and it doesn't help in my case. 
ff64c000.mdio-multiplexer and ff3f0000.ethernet are still not probed 
after applying this patch.

> The main fix for your issue would be to teach fw_devlink that
> phy-handle always points to the actual DT node that'll become a device
> even if it doesn't have a compatible property. I'll send that out
> later.

I'm waiting for the proper fix then.

Best regards

-- 
Marek Szyprowski, PhD
Samsung R&D Institute Poland