All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH net-next] net: dsa: realtek: rtl8365mb: irq with realtek-mdio
@ 2022-02-09 22:45 Luiz Angelo Daros de Luca
  2022-02-10  3:30 ` Florian Fainelli
  0 siblings, 1 reply; 9+ messages in thread
From: Luiz Angelo Daros de Luca @ 2022-02-09 22:45 UTC (permalink / raw)
  To: netdev
  Cc: linus.walleij, andrew, vivien.didelot, f.fainelli, olteanv,
	davem, kuba, alsi, arinc.unal, Luiz Angelo Daros de Luca

realtek-smi creates a custom ds->slave_mii_bus and uses a mdio
device-tree subnode to associates the interrupt-controller to each port.
However, with realtek-mdio, ds->slave_mii_bus is created and configured
by the switch with no device-tree settings. With no interruptions, the
switch falls back to polling the port status.

This patch adds a new ds_ops->port_setup() to configure each phy_device
interruption. It is only used by realtek-mdio but it could probably be
used by realtek-smi as well, removing the need for a mdio subnode in the
realtek device-tree node.

Signed-off-by: Luiz Angelo Daros de Luca <luizluca@gmail.com>
---
 drivers/net/dsa/realtek/rtl8365mb.c | 18 ++++++++++++++++++
 1 file changed, 18 insertions(+)

diff --git a/drivers/net/dsa/realtek/rtl8365mb.c b/drivers/net/dsa/realtek/rtl8365mb.c
index 2ed592147c20..45afe57a5d31 100644
--- a/drivers/net/dsa/realtek/rtl8365mb.c
+++ b/drivers/net/dsa/realtek/rtl8365mb.c
@@ -1053,6 +1053,23 @@ static void rtl8365mb_phylink_mac_link_up(struct dsa_switch *ds, int port,
 	}
 }
 
+static int rtl8365mb_port_setup(struct dsa_switch *ds, int port)
+{
+	struct realtek_priv *priv = ds->priv;
+	struct phy_device *phydev;
+
+	if (priv->irqdomain && ds->slave_mii_bus->irq[port] == PHY_POLL) {
+		phydev = mdiobus_get_phy(ds->slave_mii_bus, port);
+		if (!phydev)
+			return 0;
+
+		phydev->irq = irq_find_mapping(priv->irqdomain, port);
+		ds->slave_mii_bus->irq[port] = phydev->irq;
+	}
+
+	return 0;
+}
+
 static void rtl8365mb_port_stp_state_set(struct dsa_switch *ds, int port,
 					 u8 state)
 {
@@ -2022,6 +2039,7 @@ static const struct dsa_switch_ops rtl8365mb_switch_ops_mdio = {
 	.phylink_mac_link_up = rtl8365mb_phylink_mac_link_up,
 	.phy_read = rtl8365mb_dsa_phy_read,
 	.phy_write = rtl8365mb_dsa_phy_write,
+	.port_setup = rtl8365mb_port_setup,
 	.port_stp_state_set = rtl8365mb_port_stp_state_set,
 	.get_strings = rtl8365mb_get_strings,
 	.get_ethtool_stats = rtl8365mb_get_ethtool_stats,
-- 
2.35.1


^ permalink raw reply related	[flat|nested] 9+ messages in thread

* Re: [PATCH net-next] net: dsa: realtek: rtl8365mb: irq with realtek-mdio
  2022-02-09 22:45 [PATCH net-next] net: dsa: realtek: rtl8365mb: irq with realtek-mdio Luiz Angelo Daros de Luca
@ 2022-02-10  3:30 ` Florian Fainelli
  2022-02-10 22:15   ` Luiz Angelo Daros de Luca
  0 siblings, 1 reply; 9+ messages in thread
From: Florian Fainelli @ 2022-02-10  3:30 UTC (permalink / raw)
  To: Luiz Angelo Daros de Luca, netdev
  Cc: linus.walleij, andrew, vivien.didelot, olteanv, davem, kuba,
	alsi, arinc.unal



On 2/9/2022 2:45 PM, Luiz Angelo Daros de Luca wrote:
> realtek-smi creates a custom ds->slave_mii_bus and uses a mdio
> device-tree subnode to associates the interrupt-controller to each port.
> However, with realtek-mdio, ds->slave_mii_bus is created and configured
> by the switch with no device-tree settings. With no interruptions, the
> switch falls back to polling the port status.
> 
> This patch adds a new ds_ops->port_setup() to configure each phy_device
> interruption. It is only used by realtek-mdio but it could probably be
> used by realtek-smi as well, removing the need for a mdio subnode in the
> realtek device-tree node.
> 
> Signed-off-by: Luiz Angelo Daros de Luca <luizluca@gmail.com>
> ---
>   drivers/net/dsa/realtek/rtl8365mb.c | 18 ++++++++++++++++++
>   1 file changed, 18 insertions(+)
> 
> diff --git a/drivers/net/dsa/realtek/rtl8365mb.c b/drivers/net/dsa/realtek/rtl8365mb.c
> index 2ed592147c20..45afe57a5d31 100644
> --- a/drivers/net/dsa/realtek/rtl8365mb.c
> +++ b/drivers/net/dsa/realtek/rtl8365mb.c
> @@ -1053,6 +1053,23 @@ static void rtl8365mb_phylink_mac_link_up(struct dsa_switch *ds, int port,
>   	}
>   }
>   
> +static int rtl8365mb_port_setup(struct dsa_switch *ds, int port)
> +{
> +	struct realtek_priv *priv = ds->priv;
> +	struct phy_device *phydev;
> +
> +	if (priv->irqdomain && ds->slave_mii_bus->irq[port] == PHY_POLL) {
> +		phydev = mdiobus_get_phy(ds->slave_mii_bus, port);

This assumes a 1:1 mapping between the port number and its PHY address 
on the internal MDIO bus, is that always true?

It seems to me like we are resisting as much as possible the creating of 
the MDIO bus using of_mdiobus_register() and that seems to be forcing 
you to jump through hoops to get your per-port PHY interrupts mapped.

Maybe this needs to be re-considered and you should just create that 
internal MDIO bus without the help of the DSA framework and reap the 
benefits? We could also change the DSA framework's way of creating the 
MDIO bus so as to be OF-aware.
-- 
Florian

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH net-next] net: dsa: realtek: rtl8365mb: irq with realtek-mdio
  2022-02-10  3:30 ` Florian Fainelli
@ 2022-02-10 22:15   ` Luiz Angelo Daros de Luca
  2022-02-10 23:48     ` Alvin Šipraga
  0 siblings, 1 reply; 9+ messages in thread
From: Luiz Angelo Daros de Luca @ 2022-02-10 22:15 UTC (permalink / raw)
  To: Florian Fainelli
  Cc: open list:NETWORKING DRIVERS, Linus Walleij, Andrew Lunn,
	Vivien Didelot, Vladimir Oltean, David S. Miller, Jakub Kicinski,
	Alvin Šipraga, Arınç ÜNAL

> This assumes a 1:1 mapping between the port number and its PHY address
> on the internal MDIO bus, is that always true?

Thanks Florian,

As far as I know, for supported models, yes. I'm not sure about models
rtl8363nb and rtl8364nb because they have only 2 user ports at 1 and
3.
Anyway, they are not supported yet.

> It seems to me like we are resisting as much as possible the creating of
> the MDIO bus using of_mdiobus_register() and that seems to be forcing
> you to jump through hoops to get your per-port PHY interrupts mapped.
>
> Maybe this needs to be re-considered and you should just create that
> internal MDIO bus without the help of the DSA framework and reap the
> benefits? We could also change the DSA framework's way of creating the
> MDIO bus so as to be OF-aware.

That looks like a nice idea.

I do not have any problem duplicating the mdio setup from realtek-smi
into realtek-mdio.
However, it is just 3 copies of the same code (and I believe there are
a couple more of them):

1) dsa_switch_setup()+dsa_slave_mii_bus_init()
2) realtek_smi_setup_mdio()
3) realtek_mdio_setup_mdio() (NEW)

And realtek_smi_setup_mdio only exists as a way to reference the
OF-node. And OF-node is only needed because it needs to associate the
interrupt-parent and interrupts with each phy.
I think the best solution would be a way that the
dsa_slave_mii_bus_init could look for a specific subnode. Something
like:

dsa_slave_mii_bus_init(struct dsa_switch *ds)
{
        struct device_node *dn;
...
        dn = of_get_child_by_name(ds->dn, "slave_mii_bus");
        if (dn) {
                ds->slave_mii_bus->dev.of_node = dn;
        }
...
}

It would remove the realtek_smi_setup_mdio().

If possible, I would like to define safe default values (like assuming
1:1 mapping between the port number and its PHY address) for this
driver when interrupt-controller is present but
slave_mii_bus node is missing.

Does it sound ok?

--
Luiz

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH net-next] net: dsa: realtek: rtl8365mb: irq with realtek-mdio
  2022-02-10 22:15   ` Luiz Angelo Daros de Luca
@ 2022-02-10 23:48     ` Alvin Šipraga
  2022-02-11  4:51       ` Luiz Angelo Daros de Luca
  0 siblings, 1 reply; 9+ messages in thread
From: Alvin Šipraga @ 2022-02-10 23:48 UTC (permalink / raw)
  To: Luiz Angelo Daros de Luca
  Cc: Florian Fainelli, open list:NETWORKING DRIVERS, Linus Walleij,
	Andrew Lunn, Vivien Didelot, Vladimir Oltean, David S. Miller,
	Jakub Kicinski, Arınç ÜNAL

Luiz Angelo Daros de Luca <luizluca@gmail.com> writes:

>> This assumes a 1:1 mapping between the port number and its PHY address
>> on the internal MDIO bus, is that always true?
>
> Thanks Florian,
>
> As far as I know, for supported models, yes. I'm not sure about models
> rtl8363nb and rtl8364nb because they have only 2 user ports at 1 and
> 3.
> Anyway, they are not supported yet.

I think the port number as defined in the device tree is always going to
be the same as its PHY address on the internal bus. I had a look at the
Realtek code and this seems to be the assumption there too.

>
>> It seems to me like we are resisting as much as possible the creating of
>> the MDIO bus using of_mdiobus_register() and that seems to be forcing
>> you to jump through hoops to get your per-port PHY interrupts mapped.
>>
>> Maybe this needs to be re-considered and you should just create that
>> internal MDIO bus without the help of the DSA framework and reap the
>> benefits? We could also change the DSA framework's way of creating the
>> MDIO bus so as to be OF-aware.
>
> That looks like a nice idea.
>
> I do not have any problem duplicating the mdio setup from realtek-smi
> into realtek-mdio.
> However, it is just 3 copies of the same code (and I believe there are
> a couple more of them):
>
> 1) dsa_switch_setup()+dsa_slave_mii_bus_init()
> 2) realtek_smi_setup_mdio()
> 3) realtek_mdio_setup_mdio() (NEW)
>
> And realtek_smi_setup_mdio only exists as a way to reference the
> OF-node. And OF-node is only needed because it needs to associate the
> interrupt-parent and interrupts with each phy.
> I think the best solution would be a way that the
> dsa_slave_mii_bus_init could look for a specific subnode. Something
> like:
>
> dsa_slave_mii_bus_init(struct dsa_switch *ds)
> {
>         struct device_node *dn;
> ...
>         dn = of_get_child_by_name(ds->dn, "slave_mii_bus");
>         if (dn) {
>                 ds->slave_mii_bus->dev.of_node = dn;
>         }
> ...
> }
>
> It would remove the realtek_smi_setup_mdio().

We are not the only ones doing this. mv88e6xxx is another example. So
Florian's suggestion seems like a good one, but we should be careful to
maintain compatibility with older device trees. In some cases it is
based on child node name (e.g. "mdio"), in others it is based on the
child node compatible string (e.g. "realtek,smi-mdio",
"marvell,mv88e6xxx-mdio-external"). 

>
> If possible, I would like to define safe default values (like assuming
> 1:1 mapping between the port number and its PHY address) for this
> driver when interrupt-controller is present but
> slave_mii_bus node is missing.

You could just require the phy nodes to be described in the device
tree. Then you don't need this extra port_setup code. Seems better IMO,
or am I missing something?

Kind regards,
Alvin

>
> Does it sound ok?
>
> --
> Luiz

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH net-next] net: dsa: realtek: rtl8365mb: irq with realtek-mdio
  2022-02-10 23:48     ` Alvin Šipraga
@ 2022-02-11  4:51       ` Luiz Angelo Daros de Luca
  2022-02-11  5:11         ` [PATCH net-next] net: dsa: OF-ware slave_mii_bus (RFC) Luiz Angelo Daros de Luca
  2022-02-11  9:03         ` [PATCH net-next] net: dsa: realtek: rtl8365mb: irq with realtek-mdio Alvin Šipraga
  0 siblings, 2 replies; 9+ messages in thread
From: Luiz Angelo Daros de Luca @ 2022-02-11  4:51 UTC (permalink / raw)
  To: Alvin Šipraga
  Cc: Florian Fainelli, open list:NETWORKING DRIVERS, Linus Walleij,
	Andrew Lunn, Vivien Didelot, Vladimir Oltean, David S. Miller,
	Jakub Kicinski, Arınç ÜNAL

Thanks Alvin,

> > As far as I know, for supported models, yes. I'm not sure about models
> > rtl8363nb and rtl8364nb because they have only 2 user ports at 1 and
> > 3.
> > Anyway, they are not supported yet.
>
> I think the port number as defined in the device tree is always going to
> be the same as its PHY address on the internal bus. I had a look at the
> Realtek code and this seems to be the assumption there too.

One of the realtek-smi.txt examples (that I also copied to
realtek.yaml) does not respect that:

phy4: phy@4 {
   reg = <4>;
   interrupt-parent = <&switch_intc>;
   interrupts = <12>;
};

I don't know if 12 is a typo here.

It would only matter if I do create a default association when the
specific device tree-entry is missing. It wasn't supposed to
completely remove the device-tree declaration but to make it optional.
For now, I'll put this option aside.

> >> We could also change the DSA framework's way of creating the
> >> MDIO bus so as to be OF-aware.

It worked like a charm. I'll send it in reply to this email. I still
have some questions about it.

> We are not the only ones doing this. mv88e6xxx is another example. So
> Florian's suggestion seems like a good one, but we should be careful to
> maintain compatibility with older device trees. In some cases it is
> based on child node name (e.g. "mdio"), in others it is based on the
> child node compatible string (e.g. "realtek,smi-mdio",
> "marvell,mv88e6xxx-mdio-external").

The name "mdio" seems to be the de facto name. I'll use it. However,
it might be confusing with mdio-connected switches as you'll have an
mdio inside a switch inside another mdio. But it is exactly what it
is.

It would not affect drivers that are already allocating slave_mii_bus
by themselves. If the driver is fine with that, that's the end of the
case.

However, if a driver doesn't need any special properties inside the
mdio node, it might want to migrate to the default dsa slave_mii_bus.
For those already using "mdio" node name, they just need to drop their
code and move phy_read/write to dsa_switch_ops. Now, those using
different node names (like when they check the compatible strings)
will have a little more job. I believe we cannot rename a node "on the
fly". So, if the matched node name is not mdio, they still need to
allocate the slave_mii_bus. They will also need a different
dsa_switch_ops for each case because dsa_switch_ops->phy_read cannot
coexist with an externally allocated slave_mii_bus. Each driver needs
to plan their own migration path if they want to migrate.

For realtek-smi, the code does not require the node to be named "mdio"
but doc "realtek-smi.txt" and my new "realtek.yaml" does require it.
If I assume that, I could simply drop the code and migrate to
read/write to dsa_switch_ops.
If not, we need to maintain both code paths and warn the user for a
couple of releases until we drop the compatible string match.

> > If possible, I would like to define safe default values (like assuming
> > 1:1 mapping between the port number and its PHY address) for this
> > driver when interrupt-controller is present but
> > slave_mii_bus node is missing.
>
> You could just require the phy nodes to be described in the device
> tree. Then you don't need this extra port_setup code. Seems better IMO,
> or am I missing something?

Upstream devs seem to prefer more code than more device-tree confs. I
just wanted to reduce some device-tree copy/paste. I'm ok with using a
device-tree node.

---
Luiz

^ permalink raw reply	[flat|nested] 9+ messages in thread

* [PATCH net-next] net: dsa: OF-ware slave_mii_bus (RFC)
  2022-02-11  4:51       ` Luiz Angelo Daros de Luca
@ 2022-02-11  5:11         ` Luiz Angelo Daros de Luca
  2022-02-11  9:35           ` Alvin Šipraga
  2022-02-11  9:03         ` [PATCH net-next] net: dsa: realtek: rtl8365mb: irq with realtek-mdio Alvin Šipraga
  1 sibling, 1 reply; 9+ messages in thread
From: Luiz Angelo Daros de Luca @ 2022-02-11  5:11 UTC (permalink / raw)
  To: netdev
  Cc: linus.walleij, andrew, vivien.didelot, f.fainelli, olteanv,
	davem, kuba, alsi, arinc.unal, Luiz Angelo Daros de Luca

If found, register the DSA internal allocated slave_mii_bus with an OF
"mdio" child object. It can save some drivers from creating their
internal MDIO bus.

Some doubts:
1) is there any special reason for the absence of a "device_node dn" in
   dsa_switch? Is there any constraint on where to place it?
2) Is looking for "mdio" the best solution?

Signed-off-by: Luiz Angelo Daros de Luca <luizluca@gmail.com>
---
 include/net/dsa.h | 2 ++
 net/dsa/dsa2.c    | 8 +++++++-
 2 files changed, 9 insertions(+), 1 deletion(-)

diff --git a/include/net/dsa.h b/include/net/dsa.h
index b688ced04b0e..c01c059c5335 100644
--- a/include/net/dsa.h
+++ b/include/net/dsa.h
@@ -421,6 +421,8 @@ struct dsa_switch {
 	u32			phys_mii_mask;
 	struct mii_bus		*slave_mii_bus;
 
+	struct device_node	*dn;
+
 	/* Ageing Time limits in msecs */
 	unsigned int ageing_time_min;
 	unsigned int ageing_time_max;
diff --git a/net/dsa/dsa2.c b/net/dsa/dsa2.c
index 909b045c9b11..db1aeb6b8352 100644
--- a/net/dsa/dsa2.c
+++ b/net/dsa/dsa2.c
@@ -13,6 +13,7 @@
 #include <linux/slab.h>
 #include <linux/rtnetlink.h>
 #include <linux/of.h>
+#include <linux/of_mdio.h>
 #include <linux/of_net.h>
 #include <net/devlink.h>
 #include <net/sch_generic.h>
@@ -869,6 +870,7 @@ static int dsa_switch_setup_tag_protocol(struct dsa_switch *ds)
 static int dsa_switch_setup(struct dsa_switch *ds)
 {
 	struct dsa_devlink_priv *dl_priv;
+	struct device_node *dn;
 	struct dsa_port *dp;
 	int err;
 
@@ -924,7 +926,9 @@ static int dsa_switch_setup(struct dsa_switch *ds)
 
 		dsa_slave_mii_bus_init(ds);
 
-		err = mdiobus_register(ds->slave_mii_bus);
+		dn = of_get_child_by_name(ds->dn, "mdio");
+
+		err = of_mdiobus_register(ds->slave_mii_bus, dn);
 		if (err < 0)
 			goto free_slave_mii_bus;
 	}
@@ -1610,6 +1614,8 @@ static int dsa_switch_parse_of(struct dsa_switch *ds, struct device_node *dn)
 {
 	int err;
 
+	ds->dn = dn;
+
 	err = dsa_switch_parse_member_of(ds, dn);
 	if (err)
 		return err;
-- 
2.35.1


^ permalink raw reply related	[flat|nested] 9+ messages in thread

* Re: [PATCH net-next] net: dsa: realtek: rtl8365mb: irq with realtek-mdio
  2022-02-11  4:51       ` Luiz Angelo Daros de Luca
  2022-02-11  5:11         ` [PATCH net-next] net: dsa: OF-ware slave_mii_bus (RFC) Luiz Angelo Daros de Luca
@ 2022-02-11  9:03         ` Alvin Šipraga
  1 sibling, 0 replies; 9+ messages in thread
From: Alvin Šipraga @ 2022-02-11  9:03 UTC (permalink / raw)
  To: Luiz Angelo Daros de Luca
  Cc: Florian Fainelli, open list:NETWORKING DRIVERS, Linus Walleij,
	Andrew Lunn, Vivien Didelot, Vladimir Oltean, David S. Miller,
	Jakub Kicinski, Arınç ÜNAL

Hi Luiz,

Luiz Angelo Daros de Luca <luizluca@gmail.com> writes:

> Thanks Alvin,
>
>> > As far as I know, for supported models, yes. I'm not sure about models
>> > rtl8363nb and rtl8364nb because they have only 2 user ports at 1 and
>> > 3.
>> > Anyway, they are not supported yet.
>>
>> I think the port number as defined in the device tree is always going to
>> be the same as its PHY address on the internal bus. I had a look at the
>> Realtek code and this seems to be the assumption there too.
>
> One of the realtek-smi.txt examples (that I also copied to
> realtek.yaml) does not respect that:
>
> phy4: phy@4 {
>    reg = <4>;
>    interrupt-parent = <&switch_intc>;
>    interrupts = <12>;
> };
>
> I don't know if 12 is a typo here.

Oh sorry, I thought we were talking about the rtl8365mb driver and the
family it supports. I did not check datasheets for RTL8366RB and
such. My statement was only regarding switches supported by the '65mb
driver, where I still believe this is the case based on my reading of
the Realtek vendor code.

>
> It would only matter if I do create a default association when the
> specific device tree-entry is missing. It wasn't supposed to
> completely remove the device-tree declaration but to make it optional.
> For now, I'll put this option aside.
>
>> >> We could also change the DSA framework's way of creating the
>> >> MDIO bus so as to be OF-aware.
>
> It worked like a charm. I'll send it in reply to this email. I still
> have some questions about it.
>
>> We are not the only ones doing this. mv88e6xxx is another example. So
>> Florian's suggestion seems like a good one, but we should be careful to
>> maintain compatibility with older device trees. In some cases it is
>> based on child node name (e.g. "mdio"), in others it is based on the
>> child node compatible string (e.g. "realtek,smi-mdio",
>> "marvell,mv88e6xxx-mdio-external").
>
> The name "mdio" seems to be the de facto name. I'll use it. However,
> it might be confusing with mdio-connected switches as you'll have an
> mdio inside a switch inside another mdio. But it is exactly what it
> is.
>
> It would not affect drivers that are already allocating slave_mii_bus
> by themselves. If the driver is fine with that, that's the end of the
> case.
>
> However, if a driver doesn't need any special properties inside the
> mdio node, it might want to migrate to the default dsa slave_mii_bus.
> For those already using "mdio" node name, they just need to drop their
> code and move phy_read/write to dsa_switch_ops. Now, those using
> different node names (like when they check the compatible strings)
> will have a little more job. I believe we cannot rename a node "on the
> fly". So, if the matched node name is not mdio, they still need to
> allocate the slave_mii_bus. They will also need a different
> dsa_switch_ops for each case because dsa_switch_ops->phy_read cannot
> coexist with an externally allocated slave_mii_bus. Each driver needs
> to plan their own migration path if they want to migrate.
>
> For realtek-smi, the code does not require the node to be named "mdio"
> but doc "realtek-smi.txt" and my new "realtek.yaml" does require it.

The fact that the required property is documented as "mdio" probably
lets us do away with the compatible string parsing and switch to a
generic implementation in the realtek drivers - although I'm no device
tree lawyer, so I could be wrong here. I agree with your analysis.

> If I assume that, I could simply drop the code and migrate to
> read/write to dsa_switch_ops.
> If not, we need to maintain both code paths and warn the user for a
> couple of releases until we drop the compatible string match.
>
>> > If possible, I would like to define safe default values (like assuming
>> > 1:1 mapping between the port number and its PHY address) for this
>> > driver when interrupt-controller is present but
>> > slave_mii_bus node is missing.
>>
>> You could just require the phy nodes to be described in the device
>> tree. Then you don't need this extra port_setup code. Seems better IMO,
>> or am I missing something?
>
> Upstream devs seem to prefer more code than more device-tree confs. I
> just wanted to reduce some device-tree copy/paste. I'm ok with using a
> device-tree node.

Do you have an example of such a statement from an upstream dev? I am
asking for my own education :-)

If we require an mdio node to begin with it also obviates the whole
discussion about 1:1 mapping between DSA port number and PHY address.

Kind regards,
Alvin

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH net-next] net: dsa: OF-ware slave_mii_bus (RFC)
  2022-02-11  5:11         ` [PATCH net-next] net: dsa: OF-ware slave_mii_bus (RFC) Luiz Angelo Daros de Luca
@ 2022-02-11  9:35           ` Alvin Šipraga
  2022-02-12  3:39             ` Luiz Angelo Daros de Luca
  0 siblings, 1 reply; 9+ messages in thread
From: Alvin Šipraga @ 2022-02-11  9:35 UTC (permalink / raw)
  To: Luiz Angelo Daros de Luca
  Cc: netdev, linus.walleij, andrew, vivien.didelot, f.fainelli,
	olteanv, davem, kuba, arinc.unal

Luiz Angelo Daros de Luca <luizluca@gmail.com> writes:

> If found, register the DSA internal allocated slave_mii_bus with an OF
> "mdio" child object. It can save some drivers from creating their
> internal MDIO bus.
>
> Some doubts:
> 1) is there any special reason for the absence of a "device_node dn" in
>    dsa_switch? Is there any constraint on where to place it?
> 2) Is looking for "mdio" the best solution?
>
> Signed-off-by: Luiz Angelo Daros de Luca <luizluca@gmail.com>
> ---
>  include/net/dsa.h | 2 ++
>  net/dsa/dsa2.c    | 8 +++++++-
>  2 files changed, 9 insertions(+), 1 deletion(-)
>
> diff --git a/include/net/dsa.h b/include/net/dsa.h
> index b688ced04b0e..c01c059c5335 100644
> --- a/include/net/dsa.h
> +++ b/include/net/dsa.h
> @@ -421,6 +421,8 @@ struct dsa_switch {
>  	u32			phys_mii_mask;
>  	struct mii_bus		*slave_mii_bus;
>  
> +	struct device_node	*dn;
> +
>  	/* Ageing Time limits in msecs */
>  	unsigned int ageing_time_min;
>  	unsigned int ageing_time_max;
> diff --git a/net/dsa/dsa2.c b/net/dsa/dsa2.c
> index 909b045c9b11..db1aeb6b8352 100644
> --- a/net/dsa/dsa2.c
> +++ b/net/dsa/dsa2.c
> @@ -13,6 +13,7 @@
>  #include <linux/slab.h>
>  #include <linux/rtnetlink.h>
>  #include <linux/of.h>
> +#include <linux/of_mdio.h>
>  #include <linux/of_net.h>
>  #include <net/devlink.h>
>  #include <net/sch_generic.h>
> @@ -869,6 +870,7 @@ static int dsa_switch_setup_tag_protocol(struct dsa_switch *ds)
>  static int dsa_switch_setup(struct dsa_switch *ds)
>  {
>  	struct dsa_devlink_priv *dl_priv;
> +	struct device_node *dn;
>  	struct dsa_port *dp;
>  	int err;
>  
> @@ -924,7 +926,9 @@ static int dsa_switch_setup(struct dsa_switch *ds)
>  
>  		dsa_slave_mii_bus_init(ds);
>  
> -		err = mdiobus_register(ds->slave_mii_bus);
> +		dn = of_get_child_by_name(ds->dn, "mdio");

of_node_put(dn) after registration? Or else who will put it?

> +
> +		err = of_mdiobus_register(ds->slave_mii_bus, dn);
>  		if (err < 0)
>  			goto free_slave_mii_bus;
>  	}
> @@ -1610,6 +1614,8 @@ static int dsa_switch_parse_of(struct dsa_switch *ds, struct device_node *dn)
>  {
>  	int err;
>  
> +	ds->dn = dn;
> +
>  	err = dsa_switch_parse_member_of(ds, dn);
>  	if (err)
>  		return err;

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH net-next] net: dsa: OF-ware slave_mii_bus (RFC)
  2022-02-11  9:35           ` Alvin Šipraga
@ 2022-02-12  3:39             ` Luiz Angelo Daros de Luca
  0 siblings, 0 replies; 9+ messages in thread
From: Luiz Angelo Daros de Luca @ 2022-02-12  3:39 UTC (permalink / raw)
  To: Alvin Šipraga
  Cc: netdev, linus.walleij, andrew, vivien.didelot, f.fainelli,
	olteanv, davem, kuba, arinc.unal

> > @@ -924,7 +926,9 @@ static int dsa_switch_setup(struct dsa_switch *ds)
> >
> >               dsa_slave_mii_bus_init(ds);
> >
> > -             err = mdiobus_register(ds->slave_mii_bus);
> > +             dn = of_get_child_by_name(ds->dn, "mdio");
>
> of_node_put(dn) after registration? Or else who will put it?

Thanks Alvin, I'll need some help here.

Is it expected for every code that receives a device_node and keeps a
reference to it to call of_node_get()?  I checked of_get_child_by_name
and it seems it just parses the device_node but does not save it.
I'll put it after the registration.

I got confused looking at the dsa_port->dn usage. It is initialized at
dsa_switch_parse_ports_of() with:

for_each_available_child_of_node() {
      ...
      dsa_port_parse_of(dp, port);
}

dsa_port_parse_of(dp, dn) {
      dp->dn = dn;
      ...
}

But nobody calls of_node_put(port) when there is no error.

Should it have used a of_node_put(port) after dsa_port_parse_of() is
called and 'dp->dn = of_node_get(dn)' with a corresponding
of_node_put() when the port is destroyed?

And I dropped "ds->dn" as I can use "ds->dev->of_node". The code
indicates it can be null but it looks like all methods I use seem to
play nice with null values. It's getting even smaller:

 #include <linux/slab.h>
#include <linux/rtnetlink.h>
#include <linux/of.h>
+#include <linux/of_mdio.h>
#include <linux/of_net.h>
#include <net/devlink.h>
#include <net/sch_generic.h>
@@ -869,6 +870,7 @@ static int dsa_switch_setup_tag_protocol(struct
dsa_switch *ds)
static int dsa_switch_setup(struct dsa_switch *ds)
{
       struct dsa_devlink_priv *dl_priv;
+       struct device_node *dn;
       struct dsa_port *dp;
       int err;

@@ -924,7 +926,10 @@ static int dsa_switch_setup(struct dsa_switch *ds)

               dsa_slave_mii_bus_init(ds);

-               err = mdiobus_register(ds->slave_mii_bus);
+               dn = of_get_child_by_name(ds->dev->of_node, "mdio");
+
+               err = of_mdiobus_register(ds->slave_mii_bus, dn);
+               of_node_put(dn);
               if (err < 0)
                       goto free_slave_mii_bus;
       }

Regards,

Luiz

^ permalink raw reply	[flat|nested] 9+ messages in thread

end of thread, other threads:[~2022-02-12  3:39 UTC | newest]

Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-02-09 22:45 [PATCH net-next] net: dsa: realtek: rtl8365mb: irq with realtek-mdio Luiz Angelo Daros de Luca
2022-02-10  3:30 ` Florian Fainelli
2022-02-10 22:15   ` Luiz Angelo Daros de Luca
2022-02-10 23:48     ` Alvin Šipraga
2022-02-11  4:51       ` Luiz Angelo Daros de Luca
2022-02-11  5:11         ` [PATCH net-next] net: dsa: OF-ware slave_mii_bus (RFC) Luiz Angelo Daros de Luca
2022-02-11  9:35           ` Alvin Šipraga
2022-02-12  3:39             ` Luiz Angelo Daros de Luca
2022-02-11  9:03         ` [PATCH net-next] net: dsa: realtek: rtl8365mb: irq with realtek-mdio Alvin Šipraga

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.