diff mbox

[net-next,1/2] net: dsa: mv88e6xxx: Workaround missing PHY ID on mv88e6390

Message ID 1485309314-23942-2-git-send-email-andrew@lunn.ch
State Changes Requested, archived
Delegated to: David Miller
Headers show

Commit Message

Andrew Lunn Jan. 25, 2017, 1:55 a.m. UTC 
The internal PHYs of the mv88e6390 do not have a model ID. Trap any
calls to the ID register, and if it is zero, return the ID for the
mv88e6390. The Marvell PHY driver can then bind to this ID.

Signed-off-by: Andrew Lunn <andrew@lunn.ch>
Reviewed-by: Florian Fainelli <f.fainelli@gmail.com>
---
 drivers/net/dsa/mv88e6xxx/global2.c | 16 +++++++++++++++-
 1 file changed, 15 insertions(+), 1 deletion(-)

Comments

Gregory CLEMENT Jan. 25, 2017, 5:27 p.m. UTC | #1 
Hi Andrew,
 
 On mer., janv. 25 2017, Andrew Lunn <andrew@lunn.ch> wrote:

> The internal PHYs of the mv88e6390 do not have a model ID. Trap any
> calls to the ID register, and if it is zero, return the ID for the
> mv88e6390. The Marvell PHY driver can then bind to this ID.
>
> Signed-off-by: Andrew Lunn <andrew@lunn.ch>
> Reviewed-by: Florian Fainelli <f.fainelli@gmail.com>
> ---
>  drivers/net/dsa/mv88e6xxx/global2.c | 16 +++++++++++++++-
>  1 file changed, 15 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/net/dsa/mv88e6xxx/global2.c b/drivers/net/dsa/mv88e6xxx/global2.c
> index 353e26bea3c3..521a5511bd5f 100644
> --- a/drivers/net/dsa/mv88e6xxx/global2.c
> +++ b/drivers/net/dsa/mv88e6xxx/global2.c
> @@ -520,7 +520,21 @@ int mv88e6xxx_g2_smi_phy_read(struct mv88e6xxx_chip *chip,
>  	if (err)
>  		return err;
>  
> -	return mv88e6xxx_g2_read(chip, GLOBAL2_SMI_PHY_DATA, val);
> +	err = mv88e6xxx_g2_read(chip, GLOBAL2_SMI_PHY_DATA, val);
> +	if (err)
> +		return err;
> +
> +	if (reg == MII_PHYSID2) {
> +		/* The mv88e6390 internal PHYS don't have a model number.
> +		 * Use the switch family model number instead.
> +		 */
> +		if (!(*val & 0x3ff)) {

I tested this series on the Topaz switch but it failed because while I
said we read 0x1410C00 actually we read 0x01410C01. With the
MARVELL_PHY_ID_MASK we mask the 4 lower bits so that's why in my patch
"phy: marvell: Add support for the PHY embedded in the topaz switch" I
used the 0x01410C00 value for MARVELL_PHY_ID_88E6141.

However with the mask you use it doesn't work.

So this mask should be changed to 0x3f0 for the Topaz. Actually 0x3fe
would be enough but it seems more logical to use the same mask that for
MARVELL_PHY_ID_MASK.

We could either use the same mask for both family and still use 6390 as
they seem compatible or we use two different families based on the lower
bit.

Gregory

> +			if (chip->info->family == MV88E6XXX_FAMILY_6390)
> +				*val |= PORT_SWITCH_ID_PROD_NUM_6390;
> +		}
> +	}
> +
> +	return 0;
>  }
>  
>  int mv88e6xxx_g2_smi_phy_write(struct mv88e6xxx_chip *chip,
> -- 
> 2.11.0
>
Vivien Didelot Jan. 25, 2017, 5:45 p.m. UTC | #2 
Hi Gregory, Andrew,

Gregory CLEMENT <gregory.clement@free-electrons.com> writes:

>> +	if (reg == MII_PHYSID2) {
>> +		/* The mv88e6390 internal PHYS don't have a model number.
>> +		 * Use the switch family model number instead.
>> +		 */
>> +		if (!(*val & 0x3ff)) {
>
> I tested this series on the Topaz switch but it failed because while I
> said we read 0x1410C00 actually we read 0x01410C01. With the
> MARVELL_PHY_ID_MASK we mask the 4 lower bits so that's why in my patch
> "phy: marvell: Add support for the PHY embedded in the topaz switch" I
> used the 0x01410C00 value for MARVELL_PHY_ID_88E6141.
>
> However with the mask you use it doesn't work.
>
> So this mask should be changed to 0x3f0 for the Topaz. Actually 0x3fe
> would be enough but it seems more logical to use the same mask that for
> MARVELL_PHY_ID_MASK.
>
> We could either use the same mask for both family and still use 6390 as
> they seem compatible or we use two different families based on the lower
> bit.

Since several chips have this issue, we can introduce a u16 physid2_mask
member in the mv88e6xxx_info structure and move the check in
mv88e6xxx_phy_read() so that the logic of device (as in Global2) helpers
are not affected by such (necessary) hack. Something like:

    static int mv88e6xxx_phy_read(struct mv88e6xxx_chip *chip, int phy,
                                  int reg, u16 *val)
    {
        ...

        err = chip->info->ops->phy_read(chip, bus, addr, reg, val);
        if (err)
            return err;

        if (reg == MII_PHYSID2 && chip->info->physid2_mask) {
            /* Some internal PHYs don't have a model number,
             * so return the switch family model number directly.
             */
            if (!(*val & chip->info->physid2_mask))
                *val |= chip->info->prod_num;
        }

        return 0;
    }


Thanks,

        Vivien
Florian Fainelli Jan. 25, 2017, 5:51 p.m. UTC | #3 
On 01/25/2017 09:27 AM, Gregory CLEMENT wrote:
> Hi Andrew,
>  
>  On mer., janv. 25 2017, Andrew Lunn <andrew@lunn.ch> wrote:
> 
>> The internal PHYs of the mv88e6390 do not have a model ID. Trap any
>> calls to the ID register, and if it is zero, return the ID for the
>> mv88e6390. The Marvell PHY driver can then bind to this ID.
>>
>> Signed-off-by: Andrew Lunn <andrew@lunn.ch>
>> Reviewed-by: Florian Fainelli <f.fainelli@gmail.com>
>> ---
>>  drivers/net/dsa/mv88e6xxx/global2.c | 16 +++++++++++++++-
>>  1 file changed, 15 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/net/dsa/mv88e6xxx/global2.c b/drivers/net/dsa/mv88e6xxx/global2.c
>> index 353e26bea3c3..521a5511bd5f 100644
>> --- a/drivers/net/dsa/mv88e6xxx/global2.c
>> +++ b/drivers/net/dsa/mv88e6xxx/global2.c
>> @@ -520,7 +520,21 @@ int mv88e6xxx_g2_smi_phy_read(struct mv88e6xxx_chip *chip,
>>  	if (err)
>>  		return err;
>>  
>> -	return mv88e6xxx_g2_read(chip, GLOBAL2_SMI_PHY_DATA, val);
>> +	err = mv88e6xxx_g2_read(chip, GLOBAL2_SMI_PHY_DATA, val);
>> +	if (err)
>> +		return err;
>> +
>> +	if (reg == MII_PHYSID2) {
>> +		/* The mv88e6390 internal PHYS don't have a model number.
>> +		 * Use the switch family model number instead.
>> +		 */
>> +		if (!(*val & 0x3ff)) {
> 
> I tested this series on the Topaz switch but it failed because while I
> said we read 0x1410C00 actually we read 0x01410C01. With the
> MARVELL_PHY_ID_MASK we mask the 4 lower bits so that's why in my patch
> "phy: marvell: Add support for the PHY embedded in the topaz switch" I
> used the 0x01410C00 value for MARVELL_PHY_ID_88E6141.
> 
> However with the mask you use it doesn't work.
> 
> So this mask should be changed to 0x3f0 for the Topaz. Actually 0x3fe
> would be enough but it seems more logical to use the same mask that for
> MARVELL_PHY_ID_MASK.
> 
> We could either use the same mask for both family and still use 6390 as
> they seem compatible or we use two different families based on the lower
> bit.

By convention, the lower 4 bits are used to carry revision information,
which is why most drivers use 0xxxxx_fff0, can you try to use that here
for the PHY mask value?
Andrew Lunn Jan. 25, 2017, 5:52 p.m. UTC | #4 
> I tested this series on the Topaz switch but it failed because while I
> said we read 0x1410C00 actually we read 0x01410C01. With the
> MARVELL_PHY_ID_MASK we mask the 4 lower bits so that's why in my patch
> "phy: marvell: Add support for the PHY embedded in the topaz switch" I
> used the 0x01410C00 value for MARVELL_PHY_ID_88E6141.

O.K. The lower 4 bits seem to be the silicon revision. Marvells own
SDK ignores those bits. So lets do the same here, use
MARVELL_PHY_ID_MASK.

	Andrew
Andrew Lunn Jan. 25, 2017, 6 p.m. UTC | #5 
> Since several chips have this issue, we can introduce a u16 physid2_mask
> member in the mv88e6xxx_info structure and move the check in
> mv88e6xxx_phy_read() so that the logic of device (as in Global2) helpers
> are not affected by such (necessary) hack. Something like:
> 
>     static int mv88e6xxx_phy_read(struct mv88e6xxx_chip *chip, int phy,
>                                   int reg, u16 *val)
>     {
>         ...
> 
>         err = chip->info->ops->phy_read(chip, bus, addr, reg, val);
>         if (err)
>             return err;
> 
>         if (reg == MII_PHYSID2 && chip->info->physid2_mask) {
>             /* Some internal PHYs don't have a model number,
>              * so return the switch family model number directly.
>              */
>             if (!(*val & chip->info->physid2_mask))

Hi Vivien

I don't see the need to have per switch masks. Lets just hard code it
to ignore the lower 4 bits.

>                 *val |= chip->info->prod_num;

and this is not good. I deliberately picked the family num, not the
product num. Otherwise for the 6390 family, we have 6 different PHY
IDs. And two more for Gregorys two switches.

     Andrew
Vivien Didelot Jan. 25, 2017, 6:03 p.m. UTC | #6 
Andrew,

Vivien Didelot <vivien.didelot@savoirfairelinux.com> writes:

> Since several chips have this issue, we can introduce a u16 physid2_mask
> member in the mv88e6xxx_info structure and move the check in
> mv88e6xxx_phy_read() so that the logic of device (as in Global2) helpers
> are not affected by such (necessary) hack. Something like:
>
>     static int mv88e6xxx_phy_read(struct mv88e6xxx_chip *chip, int phy,
>                                   int reg, u16 *val)
>     {
>         ...
>
>         err = chip->info->ops->phy_read(chip, bus, addr, reg, val);
>         if (err)
>             return err;
>
>         if (reg == MII_PHYSID2 && chip->info->physid2_mask) {
>             /* Some internal PHYs don't have a model number,
>              * so return the switch family model number directly.
>              */
>             if (!(*val & chip->info->physid2_mask))
>                 *val |= chip->info->prod_num;

          if (reg == MII_PHYSID2 && (*val & 0xfff0) == 0)
              *val |= chip->info->prod_num << 4;

then. Do you agree?

>         }
>
>         return 0;
>     }

Thanks,

        Vivien
Andrew Lunn Jan. 25, 2017, 7:30 p.m. UTC | #7 
On Wed, Jan 25, 2017 at 01:03:43PM -0500, Vivien Didelot wrote:
> Andrew,
> 
> Vivien Didelot <vivien.didelot@savoirfairelinux.com> writes:
> 
> > Since several chips have this issue, we can introduce a u16 physid2_mask
> > member in the mv88e6xxx_info structure and move the check in
> > mv88e6xxx_phy_read() so that the logic of device (as in Global2) helpers
> > are not affected by such (necessary) hack. Something like:
> >
> >     static int mv88e6xxx_phy_read(struct mv88e6xxx_chip *chip, int phy,
> >                                   int reg, u16 *val)
> >     {
> >         ...
> >
> >         err = chip->info->ops->phy_read(chip, bus, addr, reg, val);
> >         if (err)
> >             return err;
> >
> >         if (reg == MII_PHYSID2 && chip->info->physid2_mask) {
> >             /* Some internal PHYs don't have a model number,
> >              * so return the switch family model number directly.
> >              */
> >             if (!(*val & chip->info->physid2_mask))
> >                 *val |= chip->info->prod_num;
> 
>           if (reg == MII_PHYSID2 && (*val & 0xfff0) == 0)

This should be 0x3f0. There are 10 bits for the device model, of which
Marvell uses the lowest 4 for version.

>               *val |= chip->info->prod_num << 4;

#define PORT_SWITCH_ID_PROD_NUM_6190    0x190
#define PORT_SWITCH_ID_PROD_NUM_6190X   0x0a0
#define PORT_SWITCH_ID_PROD_NUM_6191    0x191
#define PORT_SWITCH_ID_PROD_NUM_6290    0x290
#define PORT_SWITCH_ID_PROD_NUM_6390    0x390
#define PORT_SWITCH_ID_PROD_NUM_6390X   0x0a1

That still gives us 6 different PHY IDs, and the shift will cause is
to modify the OUI, so it is no longer a Marvell OUI.

   Andrew
Vivien Didelot Jan. 25, 2017, 7:37 p.m. UTC | #8 
Hi Andrew,

Andrew Lunn <andrew@lunn.ch> writes:

>>           if (reg == MII_PHYSID2 && (*val & 0xfff0) == 0)
>
> This should be 0x3f0. There are 10 bits for the device model, of which
> Marvell uses the lowest 4 for version.
>
>>               *val |= chip->info->prod_num << 4;
>
> #define PORT_SWITCH_ID_PROD_NUM_6190    0x190
> #define PORT_SWITCH_ID_PROD_NUM_6190X   0x0a0
> #define PORT_SWITCH_ID_PROD_NUM_6191    0x191
> #define PORT_SWITCH_ID_PROD_NUM_6290    0x290
> #define PORT_SWITCH_ID_PROD_NUM_6390    0x390
> #define PORT_SWITCH_ID_PROD_NUM_6390X   0x0a1
>
> That still gives us 6 different PHY IDs, and the shift will cause is
> to modify the OUI, so it is no longer a Marvell OUI.

OK, this is fine with the Marvell mask then. Can you move the check in
the higher mv88e6xxx_phy_read() function when you respin please?

Thanks!

        Vivien
Gregory CLEMENT Jan. 26, 2017, 12:06 p.m. UTC | #9 
Hi Andrew,

> -	return mv88e6xxx_g2_read(chip, GLOBAL2_SMI_PHY_DATA, val);
> +	err = mv88e6xxx_g2_read(chip, GLOBAL2_SMI_PHY_DATA, val);
> +	if (err)
> +		return err;
> +
> +	if (reg == MII_PHYSID2) {
> +		/* The mv88e6390 internal PHYS don't have a model number.
> +		 * Use the switch family model number instead.
> +		 */
> +		if (!(*val & 0x3ff)) {
> +			if (chip->info->family == MV88E6XXX_FAMILY_6390)

I needed to test the MV88E6XXX_FAMILY_6341 flag too. But this one have
to be done in my series because before it the flag is not introduced.

As pointed in the other patch, we still need to decide to use the sale
PHY ID or to use a new one.

Thanks,

Gregory


> +				*val |= PORT_SWITCH_ID_PROD_NUM_6390;
> +		}
> +	}
> +
> +	return 0;
>  }
>  
>  int mv88e6xxx_g2_smi_phy_write(struct mv88e6xxx_chip *chip,
> -- 
> 2.11.0
>
diff mbox

Patch

diff --git a/drivers/net/dsa/mv88e6xxx/global2.c b/drivers/net/dsa/mv88e6xxx/global2.c
index 353e26bea3c3..521a5511bd5f 100644
--- a/drivers/net/dsa/mv88e6xxx/global2.c
+++ b/drivers/net/dsa/mv88e6xxx/global2.c
@@ -520,7 +520,21 @@  int mv88e6xxx_g2_smi_phy_read(struct mv88e6xxx_chip *chip,
 	if (err)
 		return err;
 
-	return mv88e6xxx_g2_read(chip, GLOBAL2_SMI_PHY_DATA, val);
+	err = mv88e6xxx_g2_read(chip, GLOBAL2_SMI_PHY_DATA, val);
+	if (err)
+		return err;
+
+	if (reg == MII_PHYSID2) {
+		/* The mv88e6390 internal PHYS don't have a model number.
+		 * Use the switch family model number instead.
+		 */
+		if (!(*val & 0x3ff)) {
+			if (chip->info->family == MV88E6XXX_FAMILY_6390)
+				*val |= PORT_SWITCH_ID_PROD_NUM_6390;
+		}
+	}
+
+	return 0;
 }
 
 int mv88e6xxx_g2_smi_phy_write(struct mv88e6xxx_chip *chip,