| Message ID | 20220209224538.9028-1-luizluca@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [net-next] net: dsa: realtek: rtl8365mb: irq with realtek-mdio | expand |
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.
> 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
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
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
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
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,
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(+)