* [PATCH net-next] net: bgmac: support MDIO described in DT
@ 2021-09-20 12:34 Rafał Miłecki
2021-09-20 16:11 ` Florian Fainelli
0 siblings, 1 reply; 6+ messages in thread
From: Rafał Miłecki @ 2021-09-20 12:34 UTC (permalink / raw)
To: David S . Miller, Jakub Kicinski
Cc: netdev, Florian Fainelli, devicetree, bcm-kernel-feedback-list,
Rafał Miłecki
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");
+
+ err = of_mdiobus_register(mii_bus, np);
if (err) {
dev_err(&core->dev, "Registration of mii bus failed\n");
goto err_free_bus;
--
2.26.2
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH net-next] net: bgmac: support MDIO described in DT
2021-09-20 12:34 [PATCH net-next] net: bgmac: support MDIO described in DT Rafał Miłecki
@ 2021-09-20 16:11 ` Florian Fainelli
2021-09-20 17:57 ` Rafał Miłecki
0 siblings, 1 reply; 6+ messages in thread
From: Florian Fainelli @ 2021-09-20 16:11 UTC (permalink / raw)
To: Rafał Miłecki, David S . Miller, Jakub Kicinski
Cc: netdev, devicetree, bcm-kernel-feedback-list, Rafał Miłecki
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?
--
Florian
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH net-next] net: bgmac: support MDIO described in DT
2021-09-20 16:11 ` Florian Fainelli
@ 2021-09-20 17:57 ` Rafał Miłecki
2021-09-30 14:29 ` Rafał Miłecki
0 siblings, 1 reply; 6+ messages in thread
From: Rafał Miłecki @ 2021-09-20 17:57 UTC (permalink / raw)
To: Florian Fainelli, David S . Miller, Jakub Kicinski
Cc: netdev, devicetree, bcm-kernel-feedback-list, Rafał Miłecki
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.
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH net-next] net: bgmac: support MDIO described in DT
2021-09-20 17:57 ` Rafał Miłecki
@ 2021-09-30 14:29 ` Rafał Miłecki
2021-09-30 23:04 ` Florian Fainelli
0 siblings, 1 reply; 6+ messages in thread
From: Rafał Miłecki @ 2021-09-30 14:29 UTC (permalink / raw)
To: Florian Fainelli, David S . Miller, Jakub Kicinski
Cc: netdev, devicetree, bcm-kernel-feedback-list, Rafał Miłecki
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?
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH net-next] net: bgmac: support MDIO described in DT
2021-09-30 14:29 ` Rafał Miłecki
@ 2021-09-30 23:04 ` Florian Fainelli
2021-10-02 11:46 ` Rafał Miłecki
0 siblings, 1 reply; 6+ messages in thread
From: Florian Fainelli @ 2021-09-30 23:04 UTC (permalink / raw)
To: Rafał Miłecki, Florian Fainelli, David S . Miller,
Jakub Kicinski
Cc: netdev, devicetree, bcm-kernel-feedback-list, Rafał Miłecki
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.
--
Florian
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH net-next] net: bgmac: support MDIO described in DT
2021-09-30 23:04 ` Florian Fainelli
@ 2021-10-02 11:46 ` Rafał Miłecki
0 siblings, 0 replies; 6+ messages in thread
From: Rafał Miłecki @ 2021-10-02 11:46 UTC (permalink / raw)
To: Florian Fainelli, David S . Miller, Jakub Kicinski
Cc: netdev, devicetree, bcm-kernel-feedback-list, Rafał Miłecki
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;
^ permalink raw reply related [flat|nested] 6+ messages in thread
end of thread, other threads:[~2021-10-02 11:46 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-09-20 12:34 [PATCH net-next] net: bgmac: support MDIO described in DT Rafał Miłecki
2021-09-20 16:11 ` Florian Fainelli
2021-09-20 17:57 ` Rafał Miłecki
2021-09-30 14:29 ` Rafał Miłecki
2021-09-30 23:04 ` Florian Fainelli
2021-10-02 11:46 ` Rafał Miłecki
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.