| Message ID | 20210920123441.9088-1-zajec5@gmail.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [net-next] net: bgmac: support MDIO described in DT | expand |
| Context | Check | Description |
|---|---|---|
| netdev/cover_letter | success | Link |
| netdev/fixes_present | success | Link |
| netdev/patch_count | success | Link |
| netdev/tree_selection | success | Clearly marked for net-next |
| netdev/subject_prefix | success | Link |
| netdev/cc_maintainers | success | CCed 5 of 5 maintainers |
| netdev/source_inline | success | Was 0 now: 0 |
| netdev/verify_signedoff | success | Link |
| netdev/module_param | success | Was 0 now: 0 |
| netdev/build_32bit | success | Errors and warnings before: 0 this patch: 0 |
| netdev/kdoc | success | Errors and warnings before: 0 this patch: 0 |
| netdev/verify_fixes | success | Link |
| netdev/checkpatch | success | total: 0 errors, 0 warnings, 0 checks, 24 lines checked |
| netdev/build_allmodconfig_warn | success | Errors and warnings before: 0 this patch: 0 |
| netdev/header_inline | success | Link |
On 9/20/21 5:34 AM, Rafał Miłecki wrote: > From: Rafał Miłecki <rafal@milecki.pl> > > Check ethernet controller DT node for "mdio" subnode and use it with > of_mdiobus_register() when present. That allows specifying MDIO and its > PHY devices in a standard DT based way. > > This is required for BCM53573 SoC support which has an MDIO attached > switch. > > Signed-off-by: Rafał Miłecki <rafal@milecki.pl> > --- > drivers/net/ethernet/broadcom/bgmac-bcma-mdio.c | 6 +++++- > 1 file changed, 5 insertions(+), 1 deletion(-) > > diff --git a/drivers/net/ethernet/broadcom/bgmac-bcma-mdio.c b/drivers/net/ethernet/broadcom/bgmac-bcma-mdio.c > index 6ce80cbcb48e..086739e4f40a 100644 > --- a/drivers/net/ethernet/broadcom/bgmac-bcma-mdio.c > +++ b/drivers/net/ethernet/broadcom/bgmac-bcma-mdio.c > @@ -10,6 +10,7 @@ > > #include <linux/bcma/bcma.h> > #include <linux/brcmphy.h> > +#include <linux/of_mdio.h> > #include "bgmac.h" > > static bool bcma_mdio_wait_value(struct bcma_device *core, u16 reg, u32 mask, > @@ -211,6 +212,7 @@ struct mii_bus *bcma_mdio_mii_register(struct bgmac *bgmac) > { > struct bcma_device *core = bgmac->bcma.core; > struct mii_bus *mii_bus; > + struct device_node *np; > int err; > > mii_bus = mdiobus_alloc(); > @@ -229,7 +231,9 @@ struct mii_bus *bcma_mdio_mii_register(struct bgmac *bgmac) > mii_bus->parent = &core->dev; > mii_bus->phy_mask = ~(1 << bgmac->phyaddr); > > - err = mdiobus_register(mii_bus); > + np = of_get_child_by_name(core->dev.of_node, "mdio"); I believe this leaks np and the use case is not exactly clear to me here. AFAICT the Northstar SoCs have two MDIO controllers: one for internal PHYs and one for external PHYs which how you would attach a switch to the chip (in chipcommonA). Is 53573 somewhat different here? What is the MDIO bus driver that is being used?
On 20.09.2021 18:11, Florian Fainelli wrote: > On 9/20/21 5:34 AM, Rafał Miłecki wrote: >> From: Rafał Miłecki <rafal@milecki.pl> >> >> Check ethernet controller DT node for "mdio" subnode and use it with >> of_mdiobus_register() when present. That allows specifying MDIO and its >> PHY devices in a standard DT based way. >> >> This is required for BCM53573 SoC support which has an MDIO attached >> switch. >> >> Signed-off-by: Rafał Miłecki <rafal@milecki.pl> >> --- >> drivers/net/ethernet/broadcom/bgmac-bcma-mdio.c | 6 +++++- >> 1 file changed, 5 insertions(+), 1 deletion(-) >> >> diff --git a/drivers/net/ethernet/broadcom/bgmac-bcma-mdio.c b/drivers/net/ethernet/broadcom/bgmac-bcma-mdio.c >> index 6ce80cbcb48e..086739e4f40a 100644 >> --- a/drivers/net/ethernet/broadcom/bgmac-bcma-mdio.c >> +++ b/drivers/net/ethernet/broadcom/bgmac-bcma-mdio.c >> @@ -10,6 +10,7 @@ >> >> #include <linux/bcma/bcma.h> >> #include <linux/brcmphy.h> >> +#include <linux/of_mdio.h> >> #include "bgmac.h" >> >> static bool bcma_mdio_wait_value(struct bcma_device *core, u16 reg, u32 mask, >> @@ -211,6 +212,7 @@ struct mii_bus *bcma_mdio_mii_register(struct bgmac *bgmac) >> { >> struct bcma_device *core = bgmac->bcma.core; >> struct mii_bus *mii_bus; >> + struct device_node *np; >> int err; >> >> mii_bus = mdiobus_alloc(); >> @@ -229,7 +231,9 @@ struct mii_bus *bcma_mdio_mii_register(struct bgmac *bgmac) >> mii_bus->parent = &core->dev; >> mii_bus->phy_mask = ~(1 << bgmac->phyaddr); >> >> - err = mdiobus_register(mii_bus); >> + np = of_get_child_by_name(core->dev.of_node, "mdio"); > > I believe this leaks np and the use case is not exactly clear to me > here. AFAICT the Northstar SoCs have two MDIO controllers: one for > internal PHYs and one for external PHYs which how you would attach a > switch to the chip (in chipcommonA). Is 53573 somewhat different here? > What is the MDIO bus driver that is being used? of_get_child_by_name() doesn't seem to increase refcount or anything and I think it's how most drivers handle it. I don't think it should leak. BCM53573 is a built with some older blocks. Please check: 4ebd50472899 ("ARM: BCM53573: Initial support for Broadcom BCM53573 SoCs") BCM53573 series is a new family with embedded wireless. By marketing people it's sometimes called Northstar but it uses different CPU and has different architecture so we need a new symbol for it. Fortunately it shares some peripherals with other iProc based SoCs so we will be able to reuse some drivers/bindings. e90d2d51c412 ("ARM: BCM5301X: Add basic dts for BCM53573 based Tenda AC9") BCM53573 seems to be low priced alternative for Northstar chipsts. It uses single core Cortex-A7 and doesn't have SDU or local (TWD) timer. It was also stripped out of independent SPI controller and 2 GMACs. Northstar uses SRAB which is some memory based (0x18007000) access to switch register space. BCM53573 uses different blocks & mappings and it doesn't include SRAB at 0x18007000. Accessing switch registers is handled over MDIO.
On 20.09.2021 19:57, Rafał Miłecki wrote: > On 20.09.2021 18:11, Florian Fainelli wrote: >> I believe this leaks np and the use case is not exactly clear to me >> here. AFAICT the Northstar SoCs have two MDIO controllers: one for >> internal PHYs and one for external PHYs which how you would attach a >> switch to the chip (in chipcommonA). Is 53573 somewhat different here? >> What is the MDIO bus driver that is being used? > > of_get_child_by_name() doesn't seem to increase refcount or anything and > I think it's how most drivers handle it. I don't think it should leak. > > BCM53573 is a built with some older blocks. Please check: > > 4ebd50472899 ("ARM: BCM53573: Initial support for Broadcom BCM53573 SoCs") > BCM53573 series is a new family with embedded wireless. By marketing > people it's sometimes called Northstar but it uses different CPU and has > different architecture so we need a new symbol for it. > Fortunately it shares some peripherals with other iProc based SoCs so we > will be able to reuse some drivers/bindings. > > e90d2d51c412 ("ARM: BCM5301X: Add basic dts for BCM53573 based Tenda AC9") > BCM53573 seems to be low priced alternative for Northstar chipsts. It > uses single core Cortex-A7 and doesn't have SDU or local (TWD) timer. It > was also stripped out of independent SPI controller and 2 GMACs. > > Northstar uses SRAB which is some memory based (0x18007000) access to > switch register space. > BCM53573 uses different blocks & mappings and it doesn't include SRAB at > 0x18007000. Accessing switch registers is handled over MDIO. Florian: did my explanations help reviewing this patch? Would you ack it now?
On 9/30/21 7:29 AM, Rafał Miłecki wrote: > On 20.09.2021 19:57, Rafał Miłecki wrote: >> On 20.09.2021 18:11, Florian Fainelli wrote: >>> I believe this leaks np and the use case is not exactly clear to me >>> here. AFAICT the Northstar SoCs have two MDIO controllers: one for >>> internal PHYs and one for external PHYs which how you would attach a >>> switch to the chip (in chipcommonA). Is 53573 somewhat different here? >>> What is the MDIO bus driver that is being used? >> >> of_get_child_by_name() doesn't seem to increase refcount or anything and >> I think it's how most drivers handle it. I don't think it should leak. >> >> BCM53573 is a built with some older blocks. Please check: >> >> 4ebd50472899 ("ARM: BCM53573: Initial support for Broadcom BCM53573 >> SoCs") >> BCM53573 series is a new family with embedded wireless. By marketing >> people it's sometimes called Northstar but it uses different CPU >> and has >> different architecture so we need a new symbol for it. >> Fortunately it shares some peripherals with other iProc based >> SoCs so we >> will be able to reuse some drivers/bindings. >> >> e90d2d51c412 ("ARM: BCM5301X: Add basic dts for BCM53573 based Tenda >> AC9") >> BCM53573 seems to be low priced alternative for Northstar >> chipsts. It >> uses single core Cortex-A7 and doesn't have SDU or local (TWD) >> timer. It >> was also stripped out of independent SPI controller and 2 GMACs. >> >> Northstar uses SRAB which is some memory based (0x18007000) access to >> switch register space. >> BCM53573 uses different blocks & mappings and it doesn't include SRAB at >> 0x18007000. Accessing switch registers is handled over MDIO. > > Florian: did my explanations help reviewing this patch? Would you ack it > now? Thanks for providing the background. You still appear to be needing an of_node_put() after of_mdiobus_register() because that function does increase the reference count.
On 01.10.2021 01:04, Florian Fainelli wrote: > On 9/30/21 7:29 AM, Rafał Miłecki wrote: >> On 20.09.2021 19:57, Rafał Miłecki wrote: >>> On 20.09.2021 18:11, Florian Fainelli wrote: >>>> I believe this leaks np and the use case is not exactly clear to me >>>> here. AFAICT the Northstar SoCs have two MDIO controllers: one for >>>> internal PHYs and one for external PHYs which how you would attach a >>>> switch to the chip (in chipcommonA). Is 53573 somewhat different here? >>>> What is the MDIO bus driver that is being used? >>> >>> of_get_child_by_name() doesn't seem to increase refcount or anything and >>> I think it's how most drivers handle it. I don't think it should leak. >>> >>> BCM53573 is a built with some older blocks. Please check: >>> >>> 4ebd50472899 ("ARM: BCM53573: Initial support for Broadcom BCM53573 >>> SoCs") >>> BCM53573 series is a new family with embedded wireless. By marketing >>> people it's sometimes called Northstar but it uses different CPU >>> and has >>> different architecture so we need a new symbol for it. >>> Fortunately it shares some peripherals with other iProc based >>> SoCs so we >>> will be able to reuse some drivers/bindings. >>> >>> e90d2d51c412 ("ARM: BCM5301X: Add basic dts for BCM53573 based Tenda >>> AC9") >>> BCM53573 seems to be low priced alternative for Northstar >>> chipsts. It >>> uses single core Cortex-A7 and doesn't have SDU or local (TWD) >>> timer. It >>> was also stripped out of independent SPI controller and 2 GMACs. >>> >>> Northstar uses SRAB which is some memory based (0x18007000) access to >>> switch register space. >>> BCM53573 uses different blocks & mappings and it doesn't include SRAB at >>> 0x18007000. Accessing switch registers is handled over MDIO. >> >> Florian: did my explanations help reviewing this patch? Would you ack it >> now? > > Thanks for providing the background. > > You still appear to be needing an of_node_put() after > of_mdiobus_register() because that function does increase the reference > count. I really can't find code increasing refcount. I even attempted to runtime test it and I still can't see a leaking ref. See: [ 1.168863] bgmac_bcma bcma0:5: [bcma_mdio_mii_register] BEFORE count:2 [ 1.176235] libphy: bcma_mdio mii bus: probed [ 1.181513] bcm53xx bcma_mdio-0-0:1e: found switch: BCM53125, rev 4 [ 1.187936] bcm53xx bcma_mdio-0-0:1e: failed to register switch: -517 [ 1.194610] bgmac_bcma bcma0:5: [bcma_mdio_mii_register] AFTER count:2 diff --git a/drivers/net/ethernet/broadcom/bgmac-bcma-mdio.c b/drivers/net/ethernet/broadcom/bgmac-bcma-mdio.c index 086739e4f..e52a3d8b7 100644 --- a/drivers/net/ethernet/broadcom/bgmac-bcma-mdio.c +++ b/drivers/net/ethernet/broadcom/bgmac-bcma-mdio.c @@ -233,11 +233,14 @@ struct mii_bus *bcma_mdio_mii_register(struct bgmac *bgmac) np = of_get_child_by_name(core->dev.of_node, "mdio"); + + dev_info(&core->dev, "[%s] BEFORE count:%d\n", __func__, refcount_read(&np->kobj.kref.refcount)); err = of_mdiobus_register(mii_bus, np); if (err) { dev_err(&core->dev, "Registration of mii bus failed\n"); goto err_free_bus; } + dev_info(&core->dev, "[%s] AFTER count:%d\n", __func__, refcount_read(&np->kobj.kref.refcount)); return mii_bus;
diff --git a/drivers/net/ethernet/broadcom/bgmac-bcma-mdio.c b/drivers/net/ethernet/broadcom/bgmac-bcma-mdio.c index 6ce80cbcb48e..086739e4f40a 100644 --- a/drivers/net/ethernet/broadcom/bgmac-bcma-mdio.c +++ b/drivers/net/ethernet/broadcom/bgmac-bcma-mdio.c @@ -10,6 +10,7 @@ #include <linux/bcma/bcma.h> #include <linux/brcmphy.h> +#include <linux/of_mdio.h> #include "bgmac.h" static bool bcma_mdio_wait_value(struct bcma_device *core, u16 reg, u32 mask, @@ -211,6 +212,7 @@ struct mii_bus *bcma_mdio_mii_register(struct bgmac *bgmac) { struct bcma_device *core = bgmac->bcma.core; struct mii_bus *mii_bus; + struct device_node *np; int err; mii_bus = mdiobus_alloc(); @@ -229,7 +231,9 @@ struct mii_bus *bcma_mdio_mii_register(struct bgmac *bgmac) mii_bus->parent = &core->dev; mii_bus->phy_mask = ~(1 << bgmac->phyaddr); - err = mdiobus_register(mii_bus); + np = of_get_child_by_name(core->dev.of_node, "mdio"); + + err = of_mdiobus_register(mii_bus, np); if (err) { dev_err(&core->dev, "Registration of mii bus failed\n"); goto err_free_bus;