Re: [PATCH] net: phy: micrel: remove suspend/resume

Re: [PATCH] net: phy: micrel: remove suspend/resume

[Florian Fainelli]

+others,

On 08/23/2016 04:13 AM, Christophe Leroy wrote:
> In ERRATA DS80000700A dated 05 May 2016, Microship recommends to
> not use software power down mode on KSZ8041 family.

s/Microship/Microchip/

> They say they have no plan to fix this ERRATA in future releases.

The errata applies to specific revisions, is this revision present in
the lower 4 bits of the MII_PHYSID2 register such that it could be used
to key the disabling of the power down?

> 
> http://ww1.microchip.com/downloads/en/DeviceDoc/80000700A.pdf
> 
> Signed-off-by: Christophe Leroy <christophe.leroy@c-s.fr>
> ---
>  drivers/net/phy/micrel.c | 4 ----
>  1 file changed, 4 deletions(-)
> 
> diff --git a/drivers/net/phy/micrel.c b/drivers/net/phy/micrel.c
> index 053e879..f456c55 100644
> --- a/drivers/net/phy/micrel.c
> +++ b/drivers/net/phy/micrel.c
> @@ -837,8 +837,6 @@ static struct phy_driver ksphy_driver[] = {
>  	.get_sset_count = kszphy_get_sset_count,
>  	.get_strings	= kszphy_get_strings,
>  	.get_stats	= kszphy_get_stats,
> -	.suspend	= genphy_suspend,
> -	.resume		= genphy_resume,
>  }, {
>  	.phy_id		= PHY_ID_KSZ8041RNLI,
>  	.phy_id_mask	= MICREL_PHY_ID_MASK,
> @@ -856,8 +854,6 @@ static struct phy_driver ksphy_driver[] = {
>  	.get_sset_count = kszphy_get_sset_count,
>  	.get_strings	= kszphy_get_strings,
>  	.get_stats	= kszphy_get_stats,
> -	.suspend	= genphy_suspend,
> -	.resume		= genphy_resume,
>  }, {
>  	.phy_id		= PHY_ID_KSZ8051,
>  	.phy_id_mask	= MICREL_PHY_ID_MASK,
> 


-- 
Florian

Re: [PATCH] net: phy: micrel: remove suspend/resume

[Xander Huff]

On 8/23/2016 2:03 PM, Florian Fainelli wrote:
> +others,
>
> On 08/23/2016 04:13 AM, Christophe Leroy wrote:
>> In ERRATA DS80000700A dated 05 May 2016, Microship recommends to
>> not use software power down mode on KSZ8041 family.
>
> s/Microship/Microchip/
>
>> They say they have no plan to fix this ERRATA in future releases.
>
> The errata applies to specific revisions, is this revision present in
> the lower 4 bits of the MII_PHYSID2 register such that it could be used
> to key the disabling of the power down?
>
>>
>> http://ww1.microchip.com/downloads/en/DeviceDoc/80000700A.pdf
>>
>> Signed-off-by: Christophe Leroy <christophe.leroy@c-s.fr>
>> ---
>>   drivers/net/phy/micrel.c | 4 ----
>>   1 file changed, 4 deletions(-)
>>
>> diff --git a/drivers/net/phy/micrel.c b/drivers/net/phy/micrel.c
>> index 053e879..f456c55 100644
>> --- a/drivers/net/phy/micrel.c
>> +++ b/drivers/net/phy/micrel.c
>> @@ -837,8 +837,6 @@ static struct phy_driver ksphy_driver[] = {
>>   	.get_sset_count = kszphy_get_sset_count,
>>   	.get_strings	= kszphy_get_strings,
>>   	.get_stats	= kszphy_get_stats,
>> -	.suspend	= genphy_suspend,
>> -	.resume		= genphy_resume,
>>   }, {
>>   	.phy_id		= PHY_ID_KSZ8041RNLI,
>>   	.phy_id_mask	= MICREL_PHY_ID_MASK,
>> @@ -856,8 +854,6 @@ static struct phy_driver ksphy_driver[] = {
>>   	.get_sset_count = kszphy_get_sset_count,
>>   	.get_strings	= kszphy_get_strings,
>>   	.get_stats	= kszphy_get_stats,
>> -	.suspend	= genphy_suspend,
>> -	.resume		= genphy_resume,
>>   }, {
>>   	.phy_id		= PHY_ID_KSZ8051,
>>   	.phy_id_mask	= MICREL_PHY_ID_MASK,
>>
>
>
It doesn't appear that KSZ9031 is affected by this errata; no mention of suspend:

http://ww1.microchip.com/downloads/en/DeviceDoc/KSZ9031MNX_Errata_Rev%20A_A2_032515.pdf

-- 
Xander Huff
Staff Software Engineer
National Instruments

Re: [PATCH] net: phy: micrel: remove suspend/resume

[Christophe Leroy]

Le 23/08/2016 à 21:03, Florian Fainelli a écrit :
> +others,
>
> On 08/23/2016 04:13 AM, Christophe Leroy wrote:
>> In ERRATA DS80000700A dated 05 May 2016, Microship recommends to
>> not use software power down mode on KSZ8041 family.
>
> s/Microship/Microchip/
>
>> They say they have no plan to fix this ERRATA in future releases.
>
> The errata applies to specific revisions, is this revision present in
> the lower 4 bits of the MII_PHYSID2 register such that it could be used
> to key the disabling of the power down?

It doesn't seem clear to me how this could/should be handled.

According to the documentation, all variants have the same ID 0x0022151x 
with revision x. A3 has ID 0x00221512 and A4 has 0x00221513.
According to the doc, the KSZ8041RNLI should has same ID. But according 
to micrel driver, it has ID 0x00221537. And the buggy revision of that 
one is rev A. Is it what the 7 means ?

The ERRATA applies to KSZ8041NL revision A4 and to KSZ8041NL-AM revision 
A3. My understanding it that both variants have ID 0x0022151x, ie 
KSZ8041NL-AM revision A3 has ID 0x00221512 and KSZ8041NL revision A4 has 
ID 0x00221513. But KSZ8041NL revision A3 also has ID 0x00221512 and the 
ERRATA doesn't apply to it.

So what can be done really ? Only apply the fix to ID 0x00221513 (which 
is what I need as I have KSZ8041NL revision A4 on my boards) ? Or apply 
it for all KSZ8041 and KSZ8041RNLI to be on the safe side ?

Christophe


>
>>
>> http://ww1.microchip.com/downloads/en/DeviceDoc/80000700A.pdf
>>
>> Signed-off-by: Christophe Leroy <christophe.leroy@c-s.fr>
>> ---
>>  drivers/net/phy/micrel.c | 4 ----
>>  1 file changed, 4 deletions(-)
>>
>> diff --git a/drivers/net/phy/micrel.c b/drivers/net/phy/micrel.c
>> index 053e879..f456c55 100644
>> --- a/drivers/net/phy/micrel.c
>> +++ b/drivers/net/phy/micrel.c
>> @@ -837,8 +837,6 @@ static struct phy_driver ksphy_driver[] = {
>>  	.get_sset_count = kszphy_get_sset_count,
>>  	.get_strings	= kszphy_get_strings,
>>  	.get_stats	= kszphy_get_stats,
>> -	.suspend	= genphy_suspend,
>> -	.resume		= genphy_resume,
>>  }, {
>>  	.phy_id		= PHY_ID_KSZ8041RNLI,
>>  	.phy_id_mask	= MICREL_PHY_ID_MASK,
>> @@ -856,8 +854,6 @@ static struct phy_driver ksphy_driver[] = {
>>  	.get_sset_count = kszphy_get_sset_count,
>>  	.get_strings	= kszphy_get_strings,
>>  	.get_stats	= kszphy_get_stats,
>> -	.suspend	= genphy_suspend,
>> -	.resume		= genphy_resume,
>>  }, {
>>  	.phy_id		= PHY_ID_KSZ8051,
>>  	.phy_id_mask	= MICREL_PHY_ID_MASK,
>>
>
>

Re: [PATCH] net: phy: micrel: remove suspend/resume

[Florian Fainelli]

Le 24/08/2016 à 07:14, Christophe Leroy a écrit :
> 
> 
> Le 23/08/2016 à 21:03, Florian Fainelli a écrit :
>> +others,
>>
>> On 08/23/2016 04:13 AM, Christophe Leroy wrote:
>>> In ERRATA DS80000700A dated 05 May 2016, Microship recommends to
>>> not use software power down mode on KSZ8041 family.
>>
>> s/Microship/Microchip/
>>
>>> They say they have no plan to fix this ERRATA in future releases.
>>
>> The errata applies to specific revisions, is this revision present in
>> the lower 4 bits of the MII_PHYSID2 register such that it could be used
>> to key the disabling of the power down?
> 
> It doesn't seem clear to me how this could/should be handled.
> 
> According to the documentation, all variants have the same ID 0x0022151x
> with revision x. A3 has ID 0x00221512 and A4 has 0x00221513.
> According to the doc, the KSZ8041RNLI should has same ID. But according
> to micrel driver, it has ID 0x00221537. And the buggy revision of that
> one is rev A. Is it what the 7 means ?

Humm the revision is typically stored on 4 bits, so 0x7 could mean
anything here, it really depends if how they are allocating their revision.

0b0000 -> A0
0b0001 -> A1
...
0b0110 -> A6
0b0111 -> A7?

Who knows.

> 
> The ERRATA applies to KSZ8041NL revision A4 and to KSZ8041NL-AM revision
> A3. My understanding it that both variants have ID 0x0022151x, ie
> KSZ8041NL-AM revision A3 has ID 0x00221512 and KSZ8041NL revision A4 has
> ID 0x00221513. But KSZ8041NL revision A3 also has ID 0x00221512 and the
> ERRATA doesn't apply to it.
> 
> So what can be done really ? Only apply the fix to ID 0x00221513 (which
> is what I need as I have KSZ8041NL revision A4 on my boards) ? Or apply
> it for all KSZ8041 and KSZ8041RNLI to be on the safe side ?

I would apply it to just the KSZ8041NL rev. A4 for now, ideally we would
want to track down the users of the KSZ8041RNLI and see if somebody
could test that, realistically, we won't be able to, so I would err on
the side of caution at the expense of slightly increased power
consumption for that particular PHY and have a broader match of all the
KSZ8041RNLI potentially affected.

Does that make sense?
-- 
Florian

Re: [PATCH] net: phy: micrel: remove suspend/resume

[Christophe Leroy]

Le 26/08/2016 à 06:35, Florian Fainelli a écrit :
> Le 24/08/2016 à 07:14, Christophe Leroy a écrit :
>>
>>
>> Le 23/08/2016 à 21:03, Florian Fainelli a écrit :
>>> +others,
>>>
>>> On 08/23/2016 04:13 AM, Christophe Leroy wrote:
>>>> In ERRATA DS80000700A dated 05 May 2016, Microship recommends to
>>>> not use software power down mode on KSZ8041 family.
>>>
>>> s/Microship/Microchip/
>>>
>>>> They say they have no plan to fix this ERRATA in future releases.
>>>
>>> The errata applies to specific revisions, is this revision present in
>>> the lower 4 bits of the MII_PHYSID2 register such that it could be used
>>> to key the disabling of the power down?
>>
>> It doesn't seem clear to me how this could/should be handled.
>>
>> According to the documentation, all variants have the same ID 0x0022151x
>> with revision x. A3 has ID 0x00221512 and A4 has 0x00221513.
>> According to the doc, the KSZ8041RNLI should has same ID. But according
>> to micrel driver, it has ID 0x00221537. And the buggy revision of that
>> one is rev A. Is it what the 7 means ?
>
> Humm the revision is typically stored on 4 bits, so 0x7 could mean
> anything here, it really depends if how they are allocating their revision.
>
> 0b0000 -> A0
> 0b0001 -> A1
> ...
> 0b0110 -> A6
> 0b0111 -> A7?
>
> Who knows.
>
>>
>> The ERRATA applies to KSZ8041NL revision A4 and to KSZ8041NL-AM revision
>> A3. My understanding it that both variants have ID 0x0022151x, ie
>> KSZ8041NL-AM revision A3 has ID 0x00221512 and KSZ8041NL revision A4 has
>> ID 0x00221513. But KSZ8041NL revision A3 also has ID 0x00221512 and the
>> ERRATA doesn't apply to it.
>>
>> So what can be done really ? Only apply the fix to ID 0x00221513 (which
>> is what I need as I have KSZ8041NL revision A4 on my boards) ? Or apply
>> it for all KSZ8041 and KSZ8041RNLI to be on the safe side ?
>
> I would apply it to just the KSZ8041NL rev. A4 for now, ideally we would
> want to track down the users of the KSZ8041RNLI and see if somebody
> could test that, realistically, we won't be able to, so I would err on
> the side of caution at the expense of slightly increased power
> consumption for that particular PHY and have a broader match of all the
> KSZ8041RNLI potentially affected.
>
> Does that make sense?
>

What about the KSZ8041NL-AM revision A3, which has the same PHY ID as 
the KSZ8041NL revision A3 ?
Shouldn't we also have a broader match on this one in order to cover all 
cases and also be on the side of caution ?

Christophe