| Message ID | 20231223005253.17891-9-luizluca@gmail.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | net: dsa: realtek: variants to drivers, interfaces to a common module | expand |
On 23.12.2023 03:46, Luiz Angelo Daros de Luca wrote: > This reverts commit fe7324b932222574a0721b80e72c6c5fe57960d1. > > The use of user_mii_bus is inappropriate when the hardware is described > with a device-tree [1]. > > Since all drivers currently implementing ds_switch_ops.phy_{read,write} > were not updated to utilize the MDIO information from OF with the > generic "dsa user mii", they might not be affected by this change. > > [1] https://lkml.kernel.org/netdev/20231213120656.x46fyad6ls7sqyzv@skbuf/T/#u This doesn't make sense to me: "The use of user_mii_bus is inappropriate when the hardware is described with a device-tree." I think this is the correct statement: "The use of user_mii_bus is inappropriate when the MDIO bus of the switch is described on the device-tree." The patch log is also not clear on why this leads to the removal of OF-based registration from the DSA core driver. I would change the patch log here to something like this: net: dsa: do not populate user_mii_bus when switch MDIO bus is described The use of ds->user_mii_bus is inappropriate when the MDIO bus of the switch is described on the device-tree [1]. To keep things simple, make [all subdrivers that control a switch [with MDIO bus] probed on OF] register the bus on the subdriver. The subdrivers that control switches [with MDIO bus] probed on OF must follow this logic to support all cases properly: No switch MDIO bus defined: Populate ds->user_mii_bus, register the MDIO bus, set the interrupts for PHYs if "interrupt-controller" is defined at the switch node. Switch MDIO bus defined: Don't populate ds->user_mii_bus, register the MDIO bus, set the interrupts for PHYs if ["interrupt-controller" is defined at the switch node and "interrupts" is defined at the PHY nodes under the switch MDIO bus node]. There can be a case where ds->user_mii_bus is not populated, but ds_switch_ops.phy_{read,write} is. That would mean the subdriver controls a switch probed on OF, and it lets the DSA core driver to populate ds->user_mii_bus and register the bus. We don't want this to happen. Therefore, ds_switch_ops.phy_{read,write} should be only used on the subdrivers that control switches probed on platform_data. With that, the "!ds->user_mii_bus && ds->ops->phy_read" check under dsa_switch_setup() remains in use only for switches probed on platform_data. Therefore, remove OF-based registration as it's useless now. --- I think we should do all this in a single patch. I've done it on the MT7530 DSA subdriver which I maintain. diff --git a/drivers/net/dsa/mt7530.c b/drivers/net/dsa/mt7530.c index 391c4dbdff42..bbd230a73ead 100644 --- a/drivers/net/dsa/mt7530.c +++ b/drivers/net/dsa/mt7530.c @@ -2155,15 +2155,21 @@ mt7530_setup_mdio(struct mt7530_priv *priv) { struct dsa_switch *ds = priv->ds; struct device *dev = priv->dev; + struct device_node *np, *mnp; struct mii_bus *bus; static int idx; int ret; + np = priv->dev->of_node; + mnp = of_get_child_by_name(np, "mdio"); + bus = devm_mdiobus_alloc(dev); if (!bus) return -ENOMEM; - ds->user_mii_bus = bus; + if (mnp == NULL) + ds->user_mii_bus = bus; + bus->priv = priv; bus->name = KBUILD_MODNAME "-mii"; snprintf(bus->id, MII_BUS_ID_SIZE, KBUILD_MODNAME "-%d", idx++); @@ -2174,10 +2180,11 @@ mt7530_setup_mdio(struct mt7530_priv *priv) bus->parent = dev; bus->phy_mask = ~ds->phys_mii_mask; - if (priv->irq) + if (priv->irq && mnp == NULL) mt7530_setup_mdio_irq(priv); - ret = devm_mdiobus_register(dev, bus); + ret = devm_of_mdiobus_register(dev, bus, mnp); + of_node_put(mnp); if (ret) { dev_err(dev, "failed to register MDIO bus: %d\n", ret); if (priv->irq) Arınç
On 30.12.2023 10:18, Arınç ÜNAL wrote: > On 23.12.2023 03:46, Luiz Angelo Daros de Luca wrote: >> This reverts commit fe7324b932222574a0721b80e72c6c5fe57960d1. >> >> The use of user_mii_bus is inappropriate when the hardware is described >> with a device-tree [1]. >> >> Since all drivers currently implementing ds_switch_ops.phy_{read,write} >> were not updated to utilize the MDIO information from OF with the >> generic "dsa user mii", they might not be affected by this change. >> >> [1] https://lkml.kernel.org/netdev/20231213120656.x46fyad6ls7sqyzv@skbuf/T/#u > > This doesn't make sense to me: "The use of user_mii_bus is inappropriate > when the hardware is described with a device-tree." I think this is the > correct statement: "The use of user_mii_bus is inappropriate when the MDIO > bus of the switch is described on the device-tree." > > The patch log is also not clear on why this leads to the removal of > OF-based registration from the DSA core driver. > > I would change the patch log here to something like this: > > net: dsa: do not populate user_mii_bus when switch MDIO bus is described > > The use of ds->user_mii_bus is inappropriate when the MDIO bus of the > switch is described on the device-tree [1]. > > To keep things simple, make [all subdrivers that control a switch [with > MDIO bus] probed on OF] register the bus on the subdriver. > > The subdrivers that control switches [with MDIO bus] probed on OF must > follow this logic to support all cases properly: > > No switch MDIO bus defined: Populate ds->user_mii_bus, register the MDIO > bus, set the interrupts for PHYs if "interrupt-controller" is defined at > the switch node. > > Switch MDIO bus defined: Don't populate ds->user_mii_bus, register the MDIO > bus, set the interrupts for PHYs if ["interrupt-controller" is defined at > the switch node and "interrupts" is defined at the PHY nodes under the > switch MDIO bus node]. > > There can be a case where ds->user_mii_bus is not populated, but > ds_switch_ops.phy_{read,write} is. That would mean the subdriver controls a > switch probed on OF, and it lets the DSA core driver to populate > ds->user_mii_bus and register the bus. We don't want this to happen. > Therefore, ds_switch_ops.phy_{read,write} should be only used on the > subdrivers that control switches probed on platform_data. > > With that, the "!ds->user_mii_bus && ds->ops->phy_read" check under > dsa_switch_setup() remains in use only for switches probed on > platform_data. Therefore, remove OF-based registration as it's useless now. > > --- > > I think we should do all this in a single patch. I've done it on the MT7530 > DSA subdriver which I maintain. Actually, there's no need to drag this patch further by including the improvement of handling the MDIO bus on all relevant subdrivers. That said, I'd like to submit this patch myself, if it is OK by everyone here. Here's the patch log I've prepared: net: dsa: do not populate user_mii_bus when switch MDIO bus is described The use of ds->user_mii_bus is inappropriate when the MDIO bus of the switch is described on the device-tree [1]. To keep things simple, make [all subdrivers that control switches [with MDIO bus] probed on OF] register the bus on the subdriver. This is already the case on all of these subdrivers. There can be a case where ds->user_mii_bus is not populated, but ds_switch_ops.phy_{read,write} is. That would mean the subdriver controls switches probed on OF, and it lets the DSA core driver to populate ds->user_mii_bus and register the bus. We don't want this to happen. Therefore, ds_switch_ops.phy_{read,write} should be only used on the subdrivers that control switches probed on platform_data. With that, the "!ds->user_mii_bus && ds->ops->phy_read" check under dsa_switch_setup() remains in use only for switches probed on platform_data. Therefore, remove OF-based registration as it's useless now. Currently, none of the subdrivers that control switches [with MDIO bus] probed on OF implements ds_switch_ops.phy_{read,write} so no subdriver will be affected by this patch. The subdrivers that control switches [with MDIO bus] probed on OF must follow this logic to support all cases properly: No switch MDIO bus defined: Populate ds->user_mii_bus, register the MDIO bus, set the interrupts for PHYs if "interrupt-controller" is defined at the switch node. This case should only be covered for the switches which their dt-bindings documentation didn't document the MDIO bus from the start. This is to keep supporting the device trees that do not describe the MDIO bus on the device tree but the MDIO bus is being used nonetheless. Switch MDIO bus defined: Don't populate ds->user_mii_bus, register the MDIO bus, set the interrupts for PHYs if ["interrupt-controller" is defined at the switch node and "interrupts" is defined at the PHY nodes under the switch MDIO bus node]. Let's leave the improvement of handling the MDIO bus on relevant subdrivers to future patches. Arınç
On 30.12.2023 18:56, Arınç ÜNAL wrote: > On 30.12.2023 10:18, Arınç ÜNAL wrote: >> I think we should do all this in a single patch. I've done it on the MT7530 >> DSA subdriver which I maintain. > > Actually, there's no need to drag this patch further by including the > improvement of handling the MDIO bus on all relevant subdrivers. > > That said, I'd like to submit this patch myself, if it is OK by everyone > here. > > Here's the patch log I've prepared: > > net: dsa: do not populate user_mii_bus when switch MDIO bus is described > > The use of ds->user_mii_bus is inappropriate when the MDIO bus of the > switch is described on the device-tree [1]. > > To keep things simple, make [all subdrivers that control switches [with > MDIO bus] probed on OF] register the bus on the subdriver. This is already > the case on all of these subdrivers. > > There can be a case where ds->user_mii_bus is not populated, but > ds_switch_ops.phy_{read,write} is. That would mean the subdriver controls > switches probed on OF, and it lets the DSA core driver to populate > ds->user_mii_bus and register the bus. We don't want this to happen. > Therefore, ds_switch_ops.phy_{read,write} should be only used on the > subdrivers that control switches probed on platform_data. > > With that, the "!ds->user_mii_bus && ds->ops->phy_read" check under > dsa_switch_setup() remains in use only for switches probed on > platform_data. Therefore, remove OF-based registration as it's useless now. > > Currently, none of the subdrivers that control switches [with MDIO bus] > probed on OF implements ds_switch_ops.phy_{read,write} so no subdriver will > be affected by this patch. It looks like this patch will cause the MDIO bus of the switches probed on OF which are controlled by these subdrivers to only be registered non-OF-based. drivers/net/dsa/b53/b53_common.c drivers/net/dsa/lan9303-core.c drivers/net/dsa/vitesse-vsc73xx-core.c These subdrivers let the DSA driver register the bus OF-based or non-OF-based: - ds->ops->phy_read() and ds->ops->phy_write() are present. - ds->user_mii_bus is not populated. Not being able to register the bus OF-based may cause issues. There is an example for the switch on the MT7988 SoC which is controlled by the MT7530 DSA subdriver. Being able to reference the PHYs on the switch MDIO bus is mandatory on MT7988 as calibration data from NVMEM for each PHY is required. I suggest that we hold off on this patch until these subdrivers are made to be capable of registering the MDIO bus as OF-based on their own. Arınç
Hi Arinç, > It looks like this patch will cause the MDIO bus of the switches probed on > OF which are controlled by these subdrivers to only be registered > non-OF-based. > > drivers/net/dsa/b53/b53_common.c > drivers/net/dsa/lan9303-core.c > drivers/net/dsa/vitesse-vsc73xx-core.c > > These subdrivers let the DSA driver register the bus OF-based or > non-OF-based: > - ds->ops->phy_read() and ds->ops->phy_write() are present. > - ds->user_mii_bus is not populated. I checked the changes on those drivers since fe7324b932222574a0721b80e72c6c5fe57960d1 and nothing indicates that they were changing anything related to the user mii bus. I also checked bindings for the mdio node requirement. None of them mentioned the mdio node. > Not being able to register the bus OF-based may cause issues. There is an > example for the switch on the MT7988 SoC which is controlled by the MT7530 > DSA subdriver. Being able to reference the PHYs on the switch MDIO bus is > mandatory on MT7988 as calibration data from NVMEM for each PHY is > required. > > I suggest that we hold off on this patch until these subdrivers are made to > be capable of registering the MDIO bus as OF-based on their own. We might be over cautious keeping this for more time after the realtek refactoring gets merged. The using OF with the generic user mii bus driver is just a broken design and probably not in use. Anyway, it is not a requirement for the series. If there is no objection, I can drop it. I would like to send v4 with the OF node handling simplified by the change in the MDIO API. However, I'm reluctant to send mostly the same code without any reviews. Regards, Luiz
On 8.01.2024 07:44, Luiz Angelo Daros de Luca wrote: > Hi Arinç, > >> It looks like this patch will cause the MDIO bus of the switches probed on >> OF which are controlled by these subdrivers to only be registered >> non-OF-based. >> >> drivers/net/dsa/b53/b53_common.c >> drivers/net/dsa/lan9303-core.c >> drivers/net/dsa/vitesse-vsc73xx-core.c >> >> These subdrivers let the DSA driver register the bus OF-based or >> non-OF-based: >> - ds->ops->phy_read() and ds->ops->phy_write() are present. >> - ds->user_mii_bus is not populated. > > I checked the changes on those drivers since > fe7324b932222574a0721b80e72c6c5fe57960d1 and nothing indicates that > they were changing anything related to the user mii bus. I also > checked bindings for the mdio node requirement. None of them mentioned > the mdio node. Ok, we need to specifically mention the latter on the patch log. > >> Not being able to register the bus OF-based may cause issues. There is an >> example for the switch on the MT7988 SoC which is controlled by the MT7530 >> DSA subdriver. Being able to reference the PHYs on the switch MDIO bus is >> mandatory on MT7988 as calibration data from NVMEM for each PHY is >> required. >> >> I suggest that we hold off on this patch until these subdrivers are made to >> be capable of registering the MDIO bus as OF-based on their own. > > We might be over cautious keeping this for more time after the realtek > refactoring gets merged. The using OF with the generic user mii bus > driver is just a broken design and probably not in use. Anyway, it is > not a requirement for the series. If there is no objection, I can drop > it. Sounds good to me. I'd like to take this patch off your hands. > > I would like to send v4 with the OF node handling simplified by the > change in the MDIO API. However, I'm reluctant to send mostly the same > code without any reviews. I suppose our conversation here will remind Vladimir and Alvin to review. Arınç
diff --git a/net/dsa/dsa.c b/net/dsa/dsa.c index ac7be864e80d..09d2f5d4b3dd 100644 --- a/net/dsa/dsa.c +++ b/net/dsa/dsa.c @@ -15,7 +15,6 @@ #include <linux/slab.h> #include <linux/rtnetlink.h> #include <linux/of.h> -#include <linux/of_mdio.h> #include <linux/of_net.h> #include <net/dsa_stubs.h> #include <net/sch_generic.h> @@ -626,7 +625,6 @@ static void dsa_switch_teardown_tag_protocol(struct dsa_switch *ds) static int dsa_switch_setup(struct dsa_switch *ds) { - struct device_node *dn; int err; if (ds->setup) @@ -666,10 +664,7 @@ static int dsa_switch_setup(struct dsa_switch *ds) dsa_user_mii_bus_init(ds); - dn = of_get_child_by_name(ds->dev->of_node, "mdio"); - - err = of_mdiobus_register(ds->user_mii_bus, dn); - of_node_put(dn); + err = mdiobus_register(ds->user_mii_bus); if (err < 0) goto free_user_mii_bus; }
This reverts commit fe7324b932222574a0721b80e72c6c5fe57960d1. The use of user_mii_bus is inappropriate when the hardware is described with a device-tree [1]. Since all drivers currently implementing ds_switch_ops.phy_{read,write} were not updated to utilize the MDIO information from OF with the generic "dsa user mii", they might not be affected by this change. [1] https://lkml.kernel.org/netdev/20231213120656.x46fyad6ls7sqyzv@skbuf/T/#u Signed-off-by: Luiz Angelo Daros de Luca <luizluca@gmail.com> --- net/dsa/dsa.c | 7 +------ 1 file changed, 1 insertion(+), 6 deletions(-)