macb phy address bug?

macb phy address bug?

[Giulio Benetti]

[-- Attachment #1: Type: text/plain, Size: 192 bytes --]

PHYID returns 0xffff and not 0xffffffff when not found and in some 
case(at91sam9263) 0x0. Maybe this patch could be useful.



Signed-off-by: Giulio Benetti <giulio.benetti@micronovasrl.com>

[-- Attachment #2: macb.patch --]
[-- Type: text/x-diff, Size: 568 bytes --]

diff -urN -X exclude linux.orig/drivers/net/phy/phy_device.c linux/drivers/net/phy/phy_device.c
--- linux.orig/drivers/net/phy/phy_device.c	2008-11-11 17:52:13.000000000 +0100
+++ linux/drivers/net/phy/phy_device.c	2008-11-12 19:20:47.000000000 +0100
@@ -227,8 +227,8 @@
 	if (r)
 		return ERR_PTR(r);
 
-	/* If the phy_id is all Fs, there is no device there */
-	if (0xffffffff == phy_id)
+	/* If the phy_id is all Fs or all 0s, there is no device there */
+	if ((0xffff == phy_id) || (0x00 == phy_id))
 		return NULL;
 
 	dev = phy_device_create(bus, addr, phy_id);

Re: macb phy address bug?

[Giuseppe CAVALLARO]

Giulio Benetti wrote:
> PHYID returns 0xffff and not 0xffffffff when not found and in some 
> case(at91sam9263) 0x0. Maybe this patch could be useful.
>
>
>
> Signed-off-by: Giulio Benetti <giulio.benetti@micronovasrl.com>
>   
Hi Giulio,
it sounds like a work around for a hardware error.
Is my understanding that, broken hardware is sometimes missing the pull 
down resistor on the  MDIO line, which results in reads to non-existent 
devices returning  0 rather than 0xffff.
Your fix looks good for me, although, I don't know if some vendors 
actually use 0 as valid UID.
Moreover, when I met this problem, I decided to move just this hack 
within the MAC driver  (as soon as the PHY is connected to the driver) 
instead of patching the PAL code.
Hoping this could be useful.
Ciao
 Peppe

Re: macb phy address bug?

[David Miller]

I've applied this patch to net-2.6, thanks.

As mentioned there is some rare chance that the new
zero test could cause problems, in which case we'll
need to undo that part.

Re: macb phy address bug?

[Giuseppe CAVALLARO]

[-- Attachment #1: Type: text/plain, Size: 916 bytes --]

In my opinion, we need to rework this patch again for two reasons:
1) it doesn't cover the case when the PHYID is 0xffffffff.
2) we need to add an explicit comment on the PHYID check to highlight that
    this is a  work-around the broken hardware (in case we get 0xffff or 
0x0).
I've also tested it on several PHY devices: e.g. smsc lan 8700, ste101p, 
ste100p,
DP83865 National GPHY.
Where, for some of these, we need to treat the phyid=0 as wrong UID.
Please, review the attached patch.

Regards,
Peppe

David Miller wrote:
> I've applied this patch to net-2.6, thanks.
>
> As mentioned there is some rare chance that the new
> zero test could cause problems, in which case we'll
> need to undo that part.
> --
> 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
>
>   


[-- Attachment #2: phy_dev_uid_detection.patch --]
[-- Type: text/x-patch, Size: 819 bytes --]

Fix phy_id detection also for broken hardware.

Signed-off-by: Giuseppe Cavallaro <peppe.cavallaro@st.com>

--- phy_device.c.orig	2008-11-18 09:25:06.929041000 +0100
+++ phy_device.c	2008-11-18 09:27:15.876041000 +0100
@@ -227,7 +227,16 @@ struct phy_device * get_phy_device(struc
 	if (r)
 		return ERR_PTR(r);
 
-	/* If the phy_id is all Fs or all 0s, there is no device there */
+	/* If the phy_id is mostly Fs, there is no device there */
+	if ((phy_id & 0x1fffffff) == 0x1fffffff)
+		return NULL;
+
+	/*
+	 * Broken hardware is sometimes missing the pull down resistor on the
+	 * MDIO line, which results in reads to non-existent devices returning
+	 * 0 rather than 0xffff. Catch this here and treat 0 as a non-existent
+	 * device as well.
+	 */ 
 	if ((0xffff == phy_id) || (0x00 == phy_id))
 		return NULL;
 

Re: macb phy address bug?

[Giulio Benetti]

Giuseppe CAVALLARO wrote:

> In my opinion, we need to rework this patch again for two reasons:
> 1) it doesn't cover the case when the PHYID is 0xffffffff.
> 2) we need to add an explicit comment on the PHYID check to highlight that
>     this is a  work-around the broken hardware (in case we get 0xffff or
> 0x0).
> I've also tested it on several PHY devices: e.g. smsc lan 8700, ste101p,
> ste100p,
> DP83865 National GPHY.
> Where, for some of these, we need to treat the phyid=0 as wrong UID.
> Please, review the attached patch.
> 
> Regards,
> Peppe
> 
> David Miller wrote:
>> I've applied this patch to net-2.6, thanks.
>>
>> As mentioned there is some rare chance that the new
>> zero test could cause problems, in which case we'll
>> need to undo that part.
>> --
>> 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
>>
>>

You're right on hardware, it's my fault. The problem is on the primitives of 
mdio from atmel, I will patch them.

Giulio

Re: macb phy address bug?

[Giuseppe CAVALLARO]

[-- Attachment #1: Type: text/plain, Size: 83 bytes --]

Thanks Giulio for your feedback.

So I do think, this is the patch we need.

Peppe

[-- Attachment #2: phy_dev_uid_detection.patch --]
[-- Type: text/x-patch, Size: 920 bytes --]

Fix phy_id detection also for broken hardware.

Signed-off-by: Giuseppe Cavallaro <peppe.cavallaro@st.com>

--- a/drivers/net/phy/phy_device.c.orig	2008-11-18 09:25:06.929041000 +0100
+++ a/drivers/net/phy/phy_device.c	2008-11-18 16:34:12.929001000 +0100
@@ -227,8 +227,17 @@ struct phy_device * get_phy_device(struc
 	if (r)
 		return ERR_PTR(r);
 
-	/* If the phy_id is all Fs or all 0s, there is no device there */
-	if ((0xffff == phy_id) || (0x00 == phy_id))
+	/* If the phy_id is mostly Fs, there is no device there */
+	if ((phy_id & 0x1fffffff) == 0x1fffffff)
+		return NULL;
+
+	/*
+	 * Broken hardware is sometimes missing the pull down resistor on the
+	 * MDIO line, which results in reads to non-existent devices returning
+	 * 0 rather than 0xffff. Catch this here and treat 0 as a non-existent
+	 * device as well.
+	 */ 
+	if (phy_id == 0)
 		return NULL;
 
 	dev = phy_device_create(bus, addr, phy_id);

Re: macb phy address bug?

[David Miller]

From: Giuseppe CAVALLARO <peppe.cavallaro@st.com>
Date: Tue, 18 Nov 2008 09:50:17 +0100

> Fix phy_id detection also for broken hardware.
> 
> Signed-off-by: Giuseppe Cavallaro <peppe.cavallaro@st.com>
> 
> --- phy_device.c.orig	2008-11-18 09:25:06.929041000 +0100
> +++ phy_device.c	2008-11-18 09:27:15.876041000 +0100

Please -p1 base your patches with the top-level kernel tree,
as described in linux/Documentation/SubmittingPatches

Please make a full resubmission, with full changelog and
signoffs, when you correct this.  Don't just resend the
fixed patch.

Thank you.

Re: macb phy address bug?

[Giuseppe CAVALLARO]

I've just sent a new patch (named " phy: fix phy_id detection also for 
broken hardware.").

Peppe

David Miller wrote:
> From: Giuseppe CAVALLARO <peppe.cavallaro@st.com>
> Date: Tue, 18 Nov 2008 09:50:17 +0100
>
>   
>> Fix phy_id detection also for broken hardware.
>>
>> Signed-off-by: Giuseppe Cavallaro <peppe.cavallaro@st.com>
>>
>> --- phy_device.c.orig	2008-11-18 09:25:06.929041000 +0100
>> +++ phy_device.c	2008-11-18 09:27:15.876041000 +0100
>>     
>
> Please -p1 base your patches with the top-level kernel tree,
> as described in linux/Documentation/SubmittingPatches
>
> Please make a full resubmission, with full changelog and
> signoffs, when you correct this.  Don't just resend the
> fixed patch.
>
> Thank you.
> --
> 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
>
>