Patchwork [8/8] b44: add dummy PHY device if we do not find any

login
register
mail settings
Submitter Hauke Mehrtens
Date Dec. 15, 2013, 6:42 p.m.
Message ID <1387132925-18651-9-git-send-email-hauke@hauke-m.de>
Download mbox | patch
Permalink /patch/301378/
State Changes Requested
Delegated to: David Miller
Headers show

Comments

Hauke Mehrtens - Dec. 15, 2013, 6:42 p.m.
The ADM6996L switches used on some routers do not return a valid value
when reading the PHY id register and Linux thinks there is not PHY at
all, but that is wrong. This created a dummy PHY and uses that instead.

Signed-off-by: Hauke Mehrtens <hauke@hauke-m.de>
---
 drivers/net/ethernet/broadcom/b44.c |   23 +++++++++++++++++++----
 drivers/net/ethernet/broadcom/b44.h |    3 +++
 2 files changed, 22 insertions(+), 4 deletions(-)
Florian Fainelli - Dec. 15, 2013, 9:26 p.m.
Hi Hauke,

Le dimanche 15 décembre 2013, 19:42:05 Hauke Mehrtens a écrit :
> The ADM6996L switches used on some routers do not return a valid value
> when reading the PHY id register and Linux thinks there is not PHY at
> all, but that is wrong. This created a dummy PHY and uses that instead.

As far as I read it from the ADM6996L datasheet; the management interface on 
these switches is via a serial EEPROM which was usually wired up to the 
BCM47xx SoC to some GPIOs (kmod-switch driver in OpenWrt). If MII_PHYSID1 and 
MII_PHYSID2 present bad values, I would assume that MII_BMSR would also return 
bad values too and that if you are given a valid link speed/duplex this might 
just be because all bits are set?

In any case this is a case where you should register a fixed PHY device instead 
of creating such a dummy device which will still make the mdiobus driver/PHY 
state machine issue real reads/writes on the MDIO bus to a non-existent PHY.

> 
> Signed-off-by: Hauke Mehrtens <hauke@hauke-m.de>
> ---
>  drivers/net/ethernet/broadcom/b44.c |   23 +++++++++++++++++++----
>  drivers/net/ethernet/broadcom/b44.h |    3 +++
>  2 files changed, 22 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/net/ethernet/broadcom/b44.c
> b/drivers/net/ethernet/broadcom/b44.c index b65a463..07e58c2 100644
> --- a/drivers/net/ethernet/broadcom/b44.c
> +++ b/drivers/net/ethernet/broadcom/b44.c
> @@ -2233,6 +2233,8 @@ static int b44_register_phy_one(struct b44 *bp)
>  	struct ssb_device *sdev = bp->sdev;
>  	struct phy_device *phydev;
>  	int err;
> +	struct phy_c45_device_ids c45_ids = {0};
> +	struct ssb_sprom *sprom = &sdev->bus->sprom;
> 
>  	mii_bus = mdiobus_alloc();
>  	if (!mii_bus) {
> @@ -2266,10 +2268,23 @@ static int b44_register_phy_one(struct b44 *bp)
>  	}
> 
>  	phydev = bp->mii_bus->phy_map[bp->phy_addr];
> -	if (!phydev) {
> -		dev_err(sdev->dev, "could not find PHY at %i\n", bp->phy_addr);
> -		err = -ENODEV;
> -		goto err_out_mdiobus_unregister;
> +	if (!phydev &&
> +	    (sprom->boardflags_lo & (B44_BOARDFLAG_ROBO | B44_BOARDFLAG_ADM))) {
> +		dev_info(sdev->dev, "could not find PHY at %i, create dummy one\n",
> +			 bp->phy_addr);

Your commit subject says ADM6996L but here you are also treating Broadcom 
switches that way, this might deserve a comment to explain that some BCM53xx 
switches might be SPI/GPIO connected.

> +
> +		phydev = phy_device_create(bp->mii_bus, bp->phy_addr, 0x0,
> +					   false, &c45_ids);
> +		if (IS_ERR(phydev)) {
> +			err = PTR_ERR(phydev);
> +			dev_err(sdev->dev, "Can not create dummy PHY\n");
> +			goto err_out_mdiobus_unregister;
> +		}
> +		err = phy_device_register(phydev);
> +		if (err) {
> +			dev_err(sdev->dev, "failed to register MII bus\n");
> +			goto err_out_mdiobus_unregister;
> +		}
>  	}
> 
>  	err = phy_connect_direct(bp->dev, phydev, &b44_adjust_link,
> diff --git a/drivers/net/ethernet/broadcom/b44.h
> b/drivers/net/ethernet/broadcom/b44.h index c5ec9b4..e6780fe 100644
> --- a/drivers/net/ethernet/broadcom/b44.h
> +++ b/drivers/net/ethernet/broadcom/b44.h
> @@ -345,6 +345,9 @@ B44_STAT_REG_DECLARE
>  	struct u64_stats_sync	syncp;
>  };
> 
> +#define	B44_BOARDFLAG_ROBO		0x0010  /* Board has robo switch */
> +#define	B44_BOARDFLAG_ADM		0x0080  /* Board has ADMtek switch */
> +
>  struct ssb_device;
> 
>  struct b44 {
Hauke Mehrtens - Dec. 15, 2013, 11:31 p.m.
On 12/15/2013 10:26 PM, Florian Fainelli wrote:
> Hi Hauke,
> 
> Le dimanche 15 décembre 2013, 19:42:05 Hauke Mehrtens a écrit :
>> The ADM6996L switches used on some routers do not return a valid value
>> when reading the PHY id register and Linux thinks there is not PHY at
>> all, but that is wrong. This created a dummy PHY and uses that instead.
> 
> As far as I read it from the ADM6996L datasheet; the management interface on 
> these switches is via a serial EEPROM which was usually wired up to the 
> BCM47xx SoC to some GPIOs (kmod-switch driver in OpenWrt). If MII_PHYSID1 and 
> MII_PHYSID2 present bad values, I would assume that MII_BMSR would also return 
> bad values too and that if you are given a valid link speed/duplex this might 
> just be because all bits are set?

Yes these registers are returning 0xffff.

> In any case this is a case where you should register a fixed PHY device instead 
> of creating such a dummy device which will still make the mdiobus driver/PHY 
> state machine issue real reads/writes on the MDIO bus to a non-existent PHY.

Yes I will try that.

>> Signed-off-by: Hauke Mehrtens <hauke@hauke-m.de>
>> ---
>>  drivers/net/ethernet/broadcom/b44.c |   23 +++++++++++++++++++----
>>  drivers/net/ethernet/broadcom/b44.h |    3 +++
>>  2 files changed, 22 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/net/ethernet/broadcom/b44.c
>> b/drivers/net/ethernet/broadcom/b44.c index b65a463..07e58c2 100644
>> --- a/drivers/net/ethernet/broadcom/b44.c
>> +++ b/drivers/net/ethernet/broadcom/b44.c
>> @@ -2233,6 +2233,8 @@ static int b44_register_phy_one(struct b44 *bp)
>>  	struct ssb_device *sdev = bp->sdev;
>>  	struct phy_device *phydev;
>>  	int err;
>> +	struct phy_c45_device_ids c45_ids = {0};
>> +	struct ssb_sprom *sprom = &sdev->bus->sprom;
>>
>>  	mii_bus = mdiobus_alloc();
>>  	if (!mii_bus) {
>> @@ -2266,10 +2268,23 @@ static int b44_register_phy_one(struct b44 *bp)
>>  	}
>>
>>  	phydev = bp->mii_bus->phy_map[bp->phy_addr];
>> -	if (!phydev) {
>> -		dev_err(sdev->dev, "could not find PHY at %i\n", bp->phy_addr);
>> -		err = -ENODEV;
>> -		goto err_out_mdiobus_unregister;
>> +	if (!phydev &&
>> +	    (sprom->boardflags_lo & (B44_BOARDFLAG_ROBO | B44_BOARDFLAG_ADM))) {
>> +		dev_info(sdev->dev, "could not find PHY at %i, create dummy one\n",
>> +			 bp->phy_addr);
> 
> Your commit subject says ADM6996L but here you are also treating Broadcom 
> switches that way, this might deserve a comment to explain that some BCM53xx 
> switches might be SPI/GPIO connected.

This is also needed for some Broadcom switches. The Broadcom BCM5325F
switch has two MII interfaces and the BCM4704 has two MACs which are
both connected to one of the MII interfaces of the switch. On such
devices we do get a PHY ID for the first MII interface used for the LAN
ports, but when trying to read the PHY ID of the second MII interface
used for the WAN port it just returns 0xffff on all addresses. When
registering this dummy phy we are able to talk to the interface.

This WAN port seams to be hard wired to the second MII interface in the
switch.

>> +
>> +		phydev = phy_device_create(bp->mii_bus, bp->phy_addr, 0x0,
>> +					   false, &c45_ids);
>> +		if (IS_ERR(phydev)) {
>> +			err = PTR_ERR(phydev);
>> +			dev_err(sdev->dev, "Can not create dummy PHY\n");
>> +			goto err_out_mdiobus_unregister;
>> +		}
>> +		err = phy_device_register(phydev);
>> +		if (err) {
>> +			dev_err(sdev->dev, "failed to register MII bus\n");
>> +			goto err_out_mdiobus_unregister;
>> +		}
>>  	}
>>
>>  	err = phy_connect_direct(bp->dev, phydev, &b44_adjust_link,
>> diff --git a/drivers/net/ethernet/broadcom/b44.h
>> b/drivers/net/ethernet/broadcom/b44.h index c5ec9b4..e6780fe 100644
>> --- a/drivers/net/ethernet/broadcom/b44.h
>> +++ b/drivers/net/ethernet/broadcom/b44.h
>> @@ -345,6 +345,9 @@ B44_STAT_REG_DECLARE
>>  	struct u64_stats_sync	syncp;
>>  };
>>
>> +#define	B44_BOARDFLAG_ROBO		0x0010  /* Board has robo switch */
>> +#define	B44_BOARDFLAG_ADM		0x0080  /* Board has ADMtek switch */
>> +
>>  struct ssb_device;
>>
>>  struct b44 {
> 

--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Florian Fainelli - Dec. 16, 2013, 3:42 a.m.
Le lundi 16 décembre 2013, 00:31:34 Hauke Mehrtens a écrit :
> On 12/15/2013 10:26 PM, Florian Fainelli wrote:
> > Hi Hauke,
> > 
> > Le dimanche 15 décembre 2013, 19:42:05 Hauke Mehrtens a écrit :
> >> The ADM6996L switches used on some routers do not return a valid value
> >> when reading the PHY id register and Linux thinks there is not PHY at
> >> all, but that is wrong. This created a dummy PHY and uses that instead.
> > 
> > As far as I read it from the ADM6996L datasheet; the management interface
> > on these switches is via a serial EEPROM which was usually wired up to
> > the BCM47xx SoC to some GPIOs (kmod-switch driver in OpenWrt). If
> > MII_PHYSID1 and MII_PHYSID2 present bad values, I would assume that
> > MII_BMSR would also return bad values too and that if you are given a
> > valid link speed/duplex this might just be because all bits are set?
> 
> Yes these registers are returning 0xffff.

Ok, yes that's pretty typical of the absence of a physical connection between 
the MAC and PHY/switch.

> 
> > In any case this is a case where you should register a fixed PHY device
> > instead of creating such a dummy device which will still make the mdiobus
> > driver/PHY state machine issue real reads/writes on the MDIO bus to a
> > non-existent PHY.
> Yes I will try that.

Thanks! Note that a fixed PHY must be registered before the fixed MDIO bus is 
probe, you can take a look at how AR7 does it for instance 
(arch/mips/ar7/platform.c and drivers/net/ethernet/ti/cpmac.c). We might want 
to be able to change that in the future and re-scan the fixed MDIO bus though.

> 
> >> Signed-off-by: Hauke Mehrtens <hauke@hauke-m.de>
> >> ---
> >> 
> >>  drivers/net/ethernet/broadcom/b44.c |   23 +++++++++++++++++++----
> >>  drivers/net/ethernet/broadcom/b44.h |    3 +++
> >>  2 files changed, 22 insertions(+), 4 deletions(-)
> >> 
> >> diff --git a/drivers/net/ethernet/broadcom/b44.c
> >> b/drivers/net/ethernet/broadcom/b44.c index b65a463..07e58c2 100644
> >> --- a/drivers/net/ethernet/broadcom/b44.c
> >> +++ b/drivers/net/ethernet/broadcom/b44.c
> >> @@ -2233,6 +2233,8 @@ static int b44_register_phy_one(struct b44 *bp)
> >> 
> >>  	struct ssb_device *sdev = bp->sdev;
> >>  	struct phy_device *phydev;
> >>  	int err;
> >> 
> >> +	struct phy_c45_device_ids c45_ids = {0};
> >> +	struct ssb_sprom *sprom = &sdev->bus->sprom;
> >> 
> >>  	mii_bus = mdiobus_alloc();
> >>  	if (!mii_bus) {
> >> 
> >> @@ -2266,10 +2268,23 @@ static int b44_register_phy_one(struct b44 *bp)
> >> 
> >>  	}
> >>  	
> >>  	phydev = bp->mii_bus->phy_map[bp->phy_addr];
> >> 
> >> -	if (!phydev) {
> >> -		dev_err(sdev->dev, "could not find PHY at %i\n", bp->phy_addr);
> >> -		err = -ENODEV;
> >> -		goto err_out_mdiobus_unregister;
> >> +	if (!phydev &&
> >> +	    (sprom->boardflags_lo & (B44_BOARDFLAG_ROBO | B44_BOARDFLAG_ADM)))
> >> {
> >> +		dev_info(sdev->dev, "could not find PHY at %i, create dummy one\n",
> >> +			 bp->phy_addr);
> > 
> > Your commit subject says ADM6996L but here you are also treating Broadcom
> > switches that way, this might deserve a comment to explain that some
> > BCM53xx switches might be SPI/GPIO connected.
> 
> This is also needed for some Broadcom switches. The Broadcom BCM5325F
> switch has two MII interfaces and the BCM4704 has two MACs which are
> both connected to one of the MII interfaces of the switch. On such
> devices we do get a PHY ID for the first MII interface used for the LAN
> ports, but when trying to read the PHY ID of the second MII interface
> used for the WAN port it just returns 0xffff on all addresses. When
> registering this dummy phy we are able to talk to the interface.

Most likely you get 0xffff because the switch is only connected to the first 
Ethernet MAC MDIO bus.

In that specific case it might be better to make the second Ethernet MAC probe 
for PHYs on the first Ethernet MAC mdio bus or simply register a fixed PHY 
device for the WAN port since you only want one Ethernet MAC to be in control 
of the switch at a time.

> 
> This WAN port seams to be hard wired to the second MII interface in the
> switch.

This is quite likely, most Broadcom switches support one or two IMP (CPU port) 
configurations.

Patch

diff --git a/drivers/net/ethernet/broadcom/b44.c b/drivers/net/ethernet/broadcom/b44.c
index b65a463..07e58c2 100644
--- a/drivers/net/ethernet/broadcom/b44.c
+++ b/drivers/net/ethernet/broadcom/b44.c
@@ -2233,6 +2233,8 @@  static int b44_register_phy_one(struct b44 *bp)
 	struct ssb_device *sdev = bp->sdev;
 	struct phy_device *phydev;
 	int err;
+	struct phy_c45_device_ids c45_ids = {0};
+	struct ssb_sprom *sprom = &sdev->bus->sprom;
 
 	mii_bus = mdiobus_alloc();
 	if (!mii_bus) {
@@ -2266,10 +2268,23 @@  static int b44_register_phy_one(struct b44 *bp)
 	}
 
 	phydev = bp->mii_bus->phy_map[bp->phy_addr];
-	if (!phydev) {
-		dev_err(sdev->dev, "could not find PHY at %i\n", bp->phy_addr);
-		err = -ENODEV;
-		goto err_out_mdiobus_unregister;
+	if (!phydev &&
+	    (sprom->boardflags_lo & (B44_BOARDFLAG_ROBO | B44_BOARDFLAG_ADM))) {
+		dev_info(sdev->dev, "could not find PHY at %i, create dummy one\n",
+			 bp->phy_addr);
+
+		phydev = phy_device_create(bp->mii_bus, bp->phy_addr, 0x0,
+					   false, &c45_ids);
+		if (IS_ERR(phydev)) {
+			err = PTR_ERR(phydev);
+			dev_err(sdev->dev, "Can not create dummy PHY\n");
+			goto err_out_mdiobus_unregister;
+		}
+		err = phy_device_register(phydev);
+		if (err) {
+			dev_err(sdev->dev, "failed to register MII bus\n");
+			goto err_out_mdiobus_unregister;
+		}
 	}
 
 	err = phy_connect_direct(bp->dev, phydev, &b44_adjust_link,
diff --git a/drivers/net/ethernet/broadcom/b44.h b/drivers/net/ethernet/broadcom/b44.h
index c5ec9b4..e6780fe 100644
--- a/drivers/net/ethernet/broadcom/b44.h
+++ b/drivers/net/ethernet/broadcom/b44.h
@@ -345,6 +345,9 @@  B44_STAT_REG_DECLARE
 	struct u64_stats_sync	syncp;
 };
 
+#define	B44_BOARDFLAG_ROBO		0x0010  /* Board has robo switch */
+#define	B44_BOARDFLAG_ADM		0x0080  /* Board has ADMtek switch */
+
 struct ssb_device;
 
 struct b44 {