[PATCH] b43legacy: Fix machine check errors for PPC architecture with BCM4306/2

[PATCH] b43legacy: Fix machine check errors for PPC architecture with BCM4306/2

[Larry Finger]

In b43, the variable gmode merely indicates whether the given core
has a GPHY or a BPHY. In b43legacy, this is always true; however,
on a BCM4306/2 it must be set only when the PHY is connected to the
ssb backplane, otherwise, reads of the PHY registers are invalid.
For x86 architecture, these read failures cause no problems; however,
on the ppc architecture, they cause a machine check. This patch has been
tested on an i386 platform using special code to detect these invalid
reads, and on two different Powerbooks.
 
Signed-off-by: Larry Finger <Larry.Finger@lwfinger.net>
---

John,

This patch fixes a problem with the Fedora Rawhide kernel reported by David
Woodhouse on the bcm43xx mailing list and by Will Woods at 
https://bugzilla.redhat.com/show_bug.cgi?id=233011.

Larry

 drivers/net/wireless/b43legacy/main.c |    5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

Index: wireless-dev/drivers/net/wireless/b43legacy/main.c
===================================================================
--- wireless-dev.orig/drivers/net/wireless/b43legacy/main.c
+++ wireless-dev/drivers/net/wireless/b43legacy/main.c
@@ -738,8 +738,11 @@ void b43legacy_wireless_core_reset(struc
 
 	macctl = b43legacy_read32(dev, B43legacy_MMIO_MACCTL);
 	macctl &= ~B43legacy_MACCTL_GMODE;
-	if (flags & B43legacy_TMSLOW_GMODE)
+	if (flags & B43legacy_TMSLOW_GMODE) {
 		macctl |= B43legacy_MACCTL_GMODE;
+		dev->phy.gmode = 1;
+	} else
+		dev->phy.gmode = 0;
 	macctl |= B43legacy_MACCTL_IHR_ENABLED;
 	b43legacy_write32(dev, B43legacy_MMIO_MACCTL, macctl);
 }
@@ -3424,7 +3427,6 @@ static int b43legacy_wireless_core_attac
 	int err;
 	int have_bphy = 0;
 	int have_gphy = 0;
-	u32 tmp;
 
 	/* Do NOT do any device initialization here.
 	 * Do it in wireless_core_init() instead.
@@ -3456,9 +3458,7 @@ static int b43legacy_wireless_core_attac
 	if (err)
 		goto err_powerdown;
 
-	dev->phy.gmode = (have_gphy || have_bphy);
-	tmp = dev->phy.gmode ? B43legacy_TMSLOW_GMODE : 0;
-	b43legacy_wireless_core_reset(dev, tmp);
+	b43legacy_wireless_core_reset(dev, B43legacy_TMSLOW_GMODE);
 
 	err = b43legacy_phy_versioning(dev);
 	if (err)
@@ -3482,9 +3482,7 @@ static int b43legacy_wireless_core_attac
 			B43legacy_BUG_ON(1);
 		}
 	}
-	dev->phy.gmode = (have_gphy || have_bphy);
-	tmp = dev->phy.gmode ? B43legacy_TMSLOW_GMODE : 0;
-	b43legacy_wireless_core_reset(dev, tmp);
+	b43legacy_wireless_core_reset(dev, B43legacy_TMSLOW_GMODE);
 
 	err = b43legacy_validate_chipaccess(dev);
 	if (err)
Index: wireless-dev/drivers/net/wireless/b43legacy/b43legacy.h
===================================================================
--- wireless-dev.orig/drivers/net/wireless/b43legacy/b43legacy.h
+++ wireless-dev/drivers/net/wireless/b43legacy/b43legacy.h
@@ -389,7 +389,7 @@ struct b43legacy_lopair {
 struct b43legacy_phy {
 	/* Possible PHYMODEs on this PHY */
 	u8 possible_phymodes;
-	/* GMODE bit enabled? */
+	/* True if PHY connected to ssb backplane */
 	bool gmode;
 	/* Possible ieee80211 subsystem hwmodes for this PHY.
 	 * Which mode is selected, depends on thr GMODE enabled bit */
Index: wireless-dev/drivers/net/wireless/b43legacy/xmit.c
===================================================================
--- wireless-dev.orig/drivers/net/wireless/b43legacy/xmit.c
+++ wireless-dev/drivers/net/wireless/b43legacy/xmit.c
@@ -49,8 +49,8 @@ static u8 b43legacy_plcp_get_bitrate_cck
 	case 0x6E:
 		return B43legacy_CCK_RATE_11MB;
 	}
-	B43legacy_BUG_ON(1);
-	return 0;
+	printk(KERN_INFO "b43legacy: Invalid CCK bitrate of 0x%X\n", plcp->raw[0]);
+	return B43legacy_CCK_RATE_1MB;
 }
 
 /* Extract the bitrate out of an OFDM PLCP header. */
@@ -74,8 +74,8 @@ static u8 b43legacy_plcp_get_bitrate_ofd
 	case 0xC:
 		return B43legacy_OFDM_RATE_54MB;
 	}
-	B43legacy_BUG_ON(1);
-	return 0;
+	printk(KERN_INFO "b43legacy: Invalid OFDM bitrate of 0x%X\n", plcp->raw[0] & 0xF);
+	return B43legacy_OFDM_RATE_6MB;
 }
 
 u8 b43legacy_plcp_get_ratecode_cck(const u8 bitrate)
@@ -90,8 +90,8 @@ u8 b43legacy_plcp_get_ratecode_cck(const
 	case B43legacy_CCK_RATE_11MB:
 		return 0x6E;
 	}
-	B43legacy_BUG_ON(1);
-	return 0;
+	printk(KERN_INFO "b43legacy: Invalid CCK ratecode of 0x%X\n", bitrate);
+	return 0x0A;
 }
 
 u8 b43legacy_plcp_get_ratecode_ofdm(const u8 bitrate)
@@ -114,8 +114,8 @@ u8 b43legacy_plcp_get_ratecode_ofdm(cons
 	case B43legacy_OFDM_RATE_54MB:
 		return 0xC;
 	}
-	B43legacy_BUG_ON(1);
-	return 0;
+	printk(KERN_INFO "b43legacy: Invalid OFDM ratecode of 0x%X\n", bitrate);
+	return 0x0B;
 }
 
 void b43legacy_generate_plcp_hdr(struct b43legacy_plcp_hdr4 *plcp,

Re: [PATCH] b43legacy: Fix machine check errors for PPC architecture with BCM4306/2

[Michael Buesch]

On Saturday 15 September 2007 19:39:34 Larry Finger wrote:
> In b43, the variable gmode merely indicates whether the given core
> has a GPHY or a BPHY. In b43legacy, this is always true; however,
> on a BCM4306/2 it must be set only when the PHY is connected to the
> ssb backplane, otherwise, reads of the PHY registers are invalid.
> For x86 architecture, these read failures cause no problems; however,
> on the ppc architecture, they cause a machine check. This patch has been
> tested on an i386 platform using special code to detect these invalid
> reads, and on two different Powerbooks.
>  
> Signed-off-by: Larry Finger <Larry.Finger@lwfinger.net>
> ---
> 
> John,
> 
> This patch fixes a problem with the Fedora Rawhide kernel reported by David
> Woodhouse on the bcm43xx mailing list and by Will Woods at 
> https://bugzilla.redhat.com/show_bug.cgi?id=233011.
> 
> Larry
> 
>  drivers/net/wireless/b43legacy/main.c |    5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
> 
> Index: wireless-dev/drivers/net/wireless/b43legacy/main.c
> ===================================================================
> --- wireless-dev.orig/drivers/net/wireless/b43legacy/main.c
> +++ wireless-dev/drivers/net/wireless/b43legacy/main.c
> @@ -738,8 +738,11 @@ void b43legacy_wireless_core_reset(struc
>  
>  	macctl = b43legacy_read32(dev, B43legacy_MMIO_MACCTL);
>  	macctl &= ~B43legacy_MACCTL_GMODE;
> -	if (flags & B43legacy_TMSLOW_GMODE)
> +	if (flags & B43legacy_TMSLOW_GMODE) {
>  		macctl |= B43legacy_MACCTL_GMODE;
> +		dev->phy.gmode = 1;
> +	} else
> +		dev->phy.gmode = 0;
>  	macctl |= B43legacy_MACCTL_IHR_ENABLED;
>  	b43legacy_write32(dev, B43legacy_MMIO_MACCTL, macctl);
>  }
> @@ -3424,7 +3427,6 @@ static int b43legacy_wireless_core_attac
>  	int err;
>  	int have_bphy = 0;
>  	int have_gphy = 0;
> -	u32 tmp;
>  
>  	/* Do NOT do any device initialization here.
>  	 * Do it in wireless_core_init() instead.
> @@ -3456,9 +3458,7 @@ static int b43legacy_wireless_core_attac
>  	if (err)
>  		goto err_powerdown;
>  
> -	dev->phy.gmode = (have_gphy || have_bphy);
> -	tmp = dev->phy.gmode ? B43legacy_TMSLOW_GMODE : 0;
> -	b43legacy_wireless_core_reset(dev, tmp);
> +	b43legacy_wireless_core_reset(dev, B43legacy_TMSLOW_GMODE);
>  
>  	err = b43legacy_phy_versioning(dev);
>  	if (err)
> @@ -3482,9 +3482,7 @@ static int b43legacy_wireless_core_attac
>  			B43legacy_BUG_ON(1);
>  		}
>  	}
> -	dev->phy.gmode = (have_gphy || have_bphy);
> -	tmp = dev->phy.gmode ? B43legacy_TMSLOW_GMODE : 0;
> -	b43legacy_wireless_core_reset(dev, tmp);
> +	b43legacy_wireless_core_reset(dev, B43legacy_TMSLOW_GMODE);
>  
>  	err = b43legacy_validate_chipaccess(dev);
>  	if (err)
> Index: wireless-dev/drivers/net/wireless/b43legacy/b43legacy.h
> ===================================================================
> --- wireless-dev.orig/drivers/net/wireless/b43legacy/b43legacy.h
> +++ wireless-dev/drivers/net/wireless/b43legacy/b43legacy.h
> @@ -389,7 +389,7 @@ struct b43legacy_lopair {
>  struct b43legacy_phy {
>  	/* Possible PHYMODEs on this PHY */
>  	u8 possible_phymodes;
> -	/* GMODE bit enabled? */
> +	/* True if PHY connected to ssb backplane */
>  	bool gmode;

Nono, wait. That "PHY Connected" bit never existed. It was a bug
in the specification. It really is "Drive the PHY in G-mode".
It has nothing to do with "connecting" something.
So your changes above are, strictly said, also wrong.
The bit must be enabled, if we want to use the G-mode PHY on the
device and disabled otherwise. This is for multi-phy devices with
an A PHY and a B/G PHY.
So, well. As you won't ever support an A-PHY in the driver, I am
probably OK with always forcing the GMODE bit on (which your above code does,
but I don't see how this could fix anything). Better, yet, fix the _real_
cause why gmode wasn't set (that's what you are trying to fix, no?)
And please don't break the comment above. :)
It _really_ is a "GMODE bit enabled" bit.

Again: This has nothing to do with connecting some PHY to anything.

>  	/* Possible ieee80211 subsystem hwmodes for this PHY.
>  	 * Which mode is selected, depends on thr GMODE enabled bit */
> Index: wireless-dev/drivers/net/wireless/b43legacy/xmit.c
> ===================================================================
> --- wireless-dev.orig/drivers/net/wireless/b43legacy/xmit.c
> +++ wireless-dev/drivers/net/wireless/b43legacy/xmit.c
> @@ -49,8 +49,8 @@ static u8 b43legacy_plcp_get_bitrate_cck
>  	case 0x6E:
>  		return B43legacy_CCK_RATE_11MB;
>  	}
> -	B43legacy_BUG_ON(1);
> -	return 0;
> +	printk(KERN_INFO "b43legacy: Invalid CCK bitrate of 0x%X\n", plcp->raw[0]);
> +	return B43legacy_CCK_RATE_1MB;

Please ratelimit this message, if you really want a message here.

>  }
>  
>  /* Extract the bitrate out of an OFDM PLCP header. */
> @@ -74,8 +74,8 @@ static u8 b43legacy_plcp_get_bitrate_ofd
>  	case 0xC:
>  		return B43legacy_OFDM_RATE_54MB;
>  	}
> -	B43legacy_BUG_ON(1);
> -	return 0;
> +	printk(KERN_INFO "b43legacy: Invalid OFDM bitrate of 0x%X\n", plcp->raw[0] & 0xF);
> +	return B43legacy_OFDM_RATE_6MB;
>  }

Same.

>  u8 b43legacy_plcp_get_ratecode_cck(const u8 bitrate)
> @@ -90,8 +90,8 @@ u8 b43legacy_plcp_get_ratecode_cck(const
>  	case B43legacy_CCK_RATE_11MB:
>  		return 0x6E;
>  	}
> -	B43legacy_BUG_ON(1);
> -	return 0;
> +	printk(KERN_INFO "b43legacy: Invalid CCK ratecode of 0x%X\n", bitrate);
> +	return 0x0A;
>  }

Same.

>  u8 b43legacy_plcp_get_ratecode_ofdm(const u8 bitrate)
> @@ -114,8 +114,8 @@ u8 b43legacy_plcp_get_ratecode_ofdm(cons
>  	case B43legacy_OFDM_RATE_54MB:
>  		return 0xC;
>  	}
> -	B43legacy_BUG_ON(1);
> -	return 0;
> +	printk(KERN_INFO "b43legacy: Invalid OFDM ratecode of 0x%X\n", bitrate);
> +	return 0x0B;
>  }

Same.

-- 
Greetings Michael.

Re: [PATCH] b43legacy: Fix machine check errors for PPC architecture with BCM4306/2

[Larry Finger]

Michael Buesch wrote:
> On Saturday 15 September 2007 19:39:34 Larry Finger wrote:
>> In b43, the variable gmode merely indicates whether the given core
>> has a GPHY or a BPHY. In b43legacy, this is always true; however,
>> on a BCM4306/2 it must be set only when the PHY is connected to the
>> ssb backplane, otherwise, reads of the PHY registers are invalid.
>> For x86 architecture, these read failures cause no problems; however,
>> on the ppc architecture, they cause a machine check. This patch has been
>> tested on an i386 platform using special code to detect these invalid
>> reads, and on two different Powerbooks.
>>  
>> Signed-off-by: Larry Finger <Larry.Finger@lwfinger.net>
>> ---
>>
>> John,
>>
>> This patch fixes a problem with the Fedora Rawhide kernel reported by David
>> Woodhouse on the bcm43xx mailing list and by Will Woods at 
>> https://bugzilla.redhat.com/show_bug.cgi?id=233011.
>>
>> Larry
>>
>>  drivers/net/wireless/b43legacy/main.c |    5 ++++-
>>  1 file changed, 4 insertions(+), 1 deletion(-)
>>
>> Index: wireless-dev/drivers/net/wireless/b43legacy/main.c
>> ===================================================================
>> --- wireless-dev.orig/drivers/net/wireless/b43legacy/main.c
>> +++ wireless-dev/drivers/net/wireless/b43legacy/main.c
>> @@ -738,8 +738,11 @@ void b43legacy_wireless_core_reset(struc
>>  
>>  	macctl = b43legacy_read32(dev, B43legacy_MMIO_MACCTL);
>>  	macctl &= ~B43legacy_MACCTL_GMODE;
>> -	if (flags & B43legacy_TMSLOW_GMODE)
>> +	if (flags & B43legacy_TMSLOW_GMODE) {
>>  		macctl |= B43legacy_MACCTL_GMODE;
>> +		dev->phy.gmode = 1;
>> +	} else
>> +		dev->phy.gmode = 0;
>>  	macctl |= B43legacy_MACCTL_IHR_ENABLED;
>>  	b43legacy_write32(dev, B43legacy_MMIO_MACCTL, macctl);
>>  }
>> @@ -3424,7 +3427,6 @@ static int b43legacy_wireless_core_attac
>>  	int err;
>>  	int have_bphy = 0;
>>  	int have_gphy = 0;
>> -	u32 tmp;
>>  
>>  	/* Do NOT do any device initialization here.
>>  	 * Do it in wireless_core_init() instead.
>> @@ -3456,9 +3458,7 @@ static int b43legacy_wireless_core_attac
>>  	if (err)
>>  		goto err_powerdown;
>>  
>> -	dev->phy.gmode = (have_gphy || have_bphy);
>> -	tmp = dev->phy.gmode ? B43legacy_TMSLOW_GMODE : 0;
>> -	b43legacy_wireless_core_reset(dev, tmp);
>> +	b43legacy_wireless_core_reset(dev, B43legacy_TMSLOW_GMODE);
>>  
>>  	err = b43legacy_phy_versioning(dev);
>>  	if (err)
>> @@ -3482,9 +3482,7 @@ static int b43legacy_wireless_core_attac
>>  			B43legacy_BUG_ON(1);
>>  		}
>>  	}
>> -	dev->phy.gmode = (have_gphy || have_bphy);
>> -	tmp = dev->phy.gmode ? B43legacy_TMSLOW_GMODE : 0;
>> -	b43legacy_wireless_core_reset(dev, tmp);
>> +	b43legacy_wireless_core_reset(dev, B43legacy_TMSLOW_GMODE);
>>  
>>  	err = b43legacy_validate_chipaccess(dev);
>>  	if (err)
>> Index: wireless-dev/drivers/net/wireless/b43legacy/b43legacy.h
>> ===================================================================
>> --- wireless-dev.orig/drivers/net/wireless/b43legacy/b43legacy.h
>> +++ wireless-dev/drivers/net/wireless/b43legacy/b43legacy.h
>> @@ -389,7 +389,7 @@ struct b43legacy_lopair {
>>  struct b43legacy_phy {
>>  	/* Possible PHYMODEs on this PHY */
>>  	u8 possible_phymodes;
>> -	/* GMODE bit enabled? */
>> +	/* True if PHY connected to ssb backplane */
>>  	bool gmode;
> 
> Nono, wait. That "PHY Connected" bit never existed. It was a bug
> in the specification. It really is "Drive the PHY in G-mode".
> It has nothing to do with "connecting" something.
> So your changes above are, strictly said, also wrong.
> The bit must be enabled, if we want to use the G-mode PHY on the
> device and disabled otherwise. This is for multi-phy devices with
> an A PHY and a B/G PHY.
> So, well. As you won't ever support an A-PHY in the driver, I am
> probably OK with always forcing the GMODE bit on (which your above code does,
> but I don't see how this could fix anything). Better, yet, fix the _real_
> cause why gmode wasn't set (that's what you are trying to fix, no?)
> And please don't break the comment above. :)
> It _really_ is a "GMODE bit enabled" bit.
> 
> Again: This has nothing to do with connecting some PHY to anything.

Your comments may be correct; however, the fact remains that with a rev 4 BCM4306, the original code
generates many, many machine checks on PPC architecture, and I get reads with all ones on i386
indicating that the read is invalid. These all occur before the firmware is loaded. Making the
"gmode" bit mimic the behavior of the phy_connected variable of bcm43xx gets rid of the problems on
both platforms. The registers that error are not just the extended GPHY registers that caused
problems in the past, but all the PHY registers.

If you can suggest a reordering of the initialization that delays PHY setup until the "magic step"
that eliminates this problem is done on/to the ssb backplane, I will be happy to test it. I was
unable to find such a solution.

Larry

Re: [PATCH] b43legacy: Fix machine check errors for PPC architecture with BCM4306/2

[Michael Buesch]

On Saturday 15 September 2007 20:07:22 Larry Finger wrote:
> Your comments may be correct; however, the fact remains that with a rev 4 BCM4306, the original code
> generates many, many machine checks on PPC architecture, and I get reads with all ones on i386
> indicating that the read is invalid. These all occur before the firmware is loaded. Making the
> "gmode" bit mimic the behavior of the phy_connected variable of bcm43xx gets rid of the problems on
> both platforms.

Sure. But if you workaround this like that, you could
as well get completely rid of the gmode bit, as you always force it on.
But I think that's not what we want. The real fix is simple. See below.

> The registers that error are not just the extended GPHY registers that caused 
> problems in the past, but all the PHY registers.

Nobody is talking about the extG registers.

> If you can suggest a reordering of the initialization that delays PHY setup until the "magic step"
> that eliminates this problem is done on/to the ssb backplane, I will be happy to test it. I was
> unable to find such a solution.

There is no reordering needed.
You just need to find out why phy->gmode was not set.
I think it's set based on the have_gphy and have_bphy bits.
So you need to look there why one of these is not set. That is the real
cause of the issue.

-- 
Greetings Michael.

Re: [PATCH] b43legacy: Fix machine check errors for PPC architecture with BCM4306/2

[Larry Finger]

Michael Buesch wrote:
> On Saturday 15 September 2007 20:07:22 Larry Finger wrote:
>> Your comments may be correct; however, the fact remains that with a rev 4 BCM4306, the original code
>> generates many, many machine checks on PPC architecture, and I get reads with all ones on i386
>> indicating that the read is invalid. These all occur before the firmware is loaded. Making the
>> "gmode" bit mimic the behavior of the phy_connected variable of bcm43xx gets rid of the problems on
>> both platforms.
> 
> Sure. But if you workaround this like that, you could
> as well get completely rid of the gmode bit, as you always force it on.
> But I think that's not what we want. The real fix is simple. See below.
> 
> 
>> If you can suggest a reordering of the initialization that delays PHY setup until the "magic step"
>> that eliminates this problem is done on/to the ssb backplane, I will be happy to test it. I was
>> unable to find such a solution.
> 
> There is no reordering needed.
> You just need to find out why phy->gmode was not set.
> I think it's set based on the have_gphy and have_bphy bits.
> So you need to look there why one of these is not set. That is the real
> cause of the issue.

In the original code, have_gphy was true, and phy->gmode was set. These variables were not the cause.

In bcm43xx, the second argument to wireless_core_reset controls the value of phy->connected at exit.
This behavior is missing in the original b43legacy (and in b43). For b43, it seems to have no
consequence, but for the early PHYs, phy->connected controls when certain registers can be
read/written and affects the program flow through the phy and radio initialization. The lack of the
ability to toggle gmode is what leads to the register errors, and this is what my change restores.
The name could be changed, but that seems more intrusive than necessary.

Larry

Re: [PATCH] b43legacy: Fix machine check errors for PPC architecture with BCM4306/2

[Michael Buesch]

On Saturday 15 September 2007 21:12:04 Larry Finger wrote:
> Michael Buesch wrote:
> > On Saturday 15 September 2007 20:07:22 Larry Finger wrote:
> >> Your comments may be correct; however, the fact remains that with a rev 4 BCM4306, the original code
> >> generates many, many machine checks on PPC architecture, and I get reads with all ones on i386
> >> indicating that the read is invalid. These all occur before the firmware is loaded. Making the
> >> "gmode" bit mimic the behavior of the phy_connected variable of bcm43xx gets rid of the problems on
> >> both platforms.
> > 
> > Sure. But if you workaround this like that, you could
> > as well get completely rid of the gmode bit, as you always force it on.
> > But I think that's not what we want. The real fix is simple. See below.
> > 
> > 
> >> If you can suggest a reordering of the initialization that delays PHY setup until the "magic step"
> >> that eliminates this problem is done on/to the ssb backplane, I will be happy to test it. I was
> >> unable to find such a solution.
> > 
> > There is no reordering needed.
> > You just need to find out why phy->gmode was not set.
> > I think it's set based on the have_gphy and have_bphy bits.
> > So you need to look there why one of these is not set. That is the real
> > cause of the issue.
> 
> In the original code, have_gphy was true, and phy->gmode was set. These variables were not the cause.
> 
> In bcm43xx, the second argument to wireless_core_reset controls the value of phy->connected at exit.
> This behavior is missing in the original b43legacy (and in b43). For b43, it seems to have no
> consequence, but for the early PHYs, phy->connected controls when certain registers can be
> read/written and affects the program flow through the phy and radio initialization. The lack of the
> ability to toggle gmode is what leads to the register errors, and this is what my change restores.
> The name could be changed, but that seems more intrusive than necessary.

No, your patch doesn't restore anything. It rips out the gmode logic.
I just want to make sure you understand what you are actually doing there.
the old phy->connected semantics were completely broken. Forget about them.
The problem here is, that at some point phy->gmode is not set, where
it should be set (or the other way around).
So it's not about renaming or something.

With your patch you remove the "gmode" semantics, so you could
as well remove the whole variable and always assume it's set.
But I don't recommend to go that way. I'd suggest you find the
place in the code where phy->gmode is not set but it should be.
_that_ is the place where the real bug is.

Just want to make sure you understand the consequences of your patch.
You remove a feature with that.
If that feature is really needed in legacy; that is another question.
It's needed if you want to drive APHY devices.

-- 
Greetings Michael.

Re: [PATCH] b43legacy: Fix machine check errors for PPC architecture with BCM4306/2

[Larry Finger]

Michael Buesch wrote:
> With your patch you remove the "gmode" semantics, so you could
> as well remove the whole variable and always assume it's set.
> But I don't recommend to go that way. I'd suggest you find the
> place in the code where phy->gmode is not set but it should be.
> _that_ is the place where the real bug is.
> 
> Just want to make sure you understand the consequences of your patch.
> You remove a feature with that.
> If that feature is really needed in legacy; that is another question.
> It's needed if you want to drive APHY devices.

All of the devices that b43legacy drives have either a BPHY or a GPHY. The current logic always sets
gmode and that feature is meaningless. Anyone overriding the rules that select this driver in order
to operate with an APHY device will hit a BUG_ON in wireless_core_attach.

I have generated and tested a patch that changes back to the old 'connected' variable. That will
avoid the semantics argument and match the V3 specs.

Larry

Re: [PATCH] b43legacy: Fix machine check errors for PPC architecture with BCM4306/2

[Michael Buesch]

On Saturday 15 September 2007 23:01:56 Larry Finger wrote:
> I have generated and tested a patch that changes back to the old 'connected' variable. That will
> avoid the semantics argument and match the V3 specs.

There is no such thing as "connected".
The bit is called "gmode". Why do you change it back to the wrong name?
The V3 specs are simply _wrong_. It has nothing to do with the backplane
or anything else like that. The bit just selects between the A-PHY and
the BG-PHY. Of course, if there is no A-PHY it will obviously machine-check
on access, if it's selected.

V3 specs assume that the meaning is "connected", because it was misinterpreted
to be a "PHY connect" flag. But it really is a "PHY select" flag, to select
between two different PHYs. If it's enabled, the G-PHY will be selected (and
the A-PHY otherwise).

Btw, while we are at it: The V3 specs are wrong regarding the gmode (or connected)
bit in other areas as well, because there was even more confusion with
other PHY flags. The PHY specification is pretty broken in the v3 specs.

-- 
Greetings Michael.

Re: [PATCH] b43legacy: Fix machine check errors for PPC architecture with BCM4306/2

[Larry Finger]

Michael Buesch wrote:
> 
> There is no such thing as "connected".
> The bit is called "gmode". Why do you change it back to the wrong name?
> The V3 specs are simply _wrong_. It has nothing to do with the backplane
> or anything else like that. The bit just selects between the A-PHY and
> the BG-PHY. Of course, if there is no A-PHY it will obviously machine-check
> on access, if it's selected.
> 
> V3 specs assume that the meaning is "connected", because it was misinterpreted
> to be a "PHY connect" flag. But it really is a "PHY select" flag, to select
> between two different PHYs. If it's enabled, the G-PHY will be selected (and
> the A-PHY otherwise).
> 
> Btw, while we are at it: The V3 specs are wrong regarding the gmode (or connected)
> bit in other areas as well, because there was even more confusion with
> other PHY flags. The PHY specification is pretty broken in the v3 specs.

The critical part of the legacy code in this regard is from phy_calibrate():

        if (phy->type == B43legacy_PHYTYPE_G && phy->rev == 1) {
                b43legacy_wireless_core_reset(dev, 0);
                b43legacy_phy_initg(dev);
                b43legacy_wireless_core_reset(dev, B43legacy_TMSLOW_GMODE);
        }

If phy_initg is going to work correctly, then gmode has to follow the state of the MACCTL_GMODE bit
in MMIO_MACCTL. Apparently GPHYs with rev > 1 do not care about this bit, but the BCM4306/2 does.
The changes can be accomplished by the following patch:

Index: wireless-dev/drivers/net/wireless/b43legacy/main.c
===================================================================
--- wireless-dev.orig/drivers/net/wireless/b43legacy/main.c
+++ wireless-dev/drivers/net/wireless/b43legacy/main.c
@@ -738,8 +738,11 @@ void b43legacy_wireless_core_reset(struc

        macctl = b43legacy_read32(dev, B43legacy_MMIO_MACCTL);
        macctl &= ~B43legacy_MACCTL_GMODE;
-       if (flags & B43legacy_TMSLOW_GMODE)
+       if (flags & B43legacy_TMSLOW_GMODE) {
                macctl |= B43legacy_MACCTL_GMODE;
+               dev->phy.gmode = 1;
+       } else
+               dev->phy.gmode = 0;
        macctl |= B43legacy_MACCTL_IHR_ENABLED;
        b43legacy_write32(dev, B43legacy_MMIO_MACCTL, macctl);
 }

The above patch and a revised comment for the gmode variable will be the contents of V3 of my patch.
I hope that you find it acceptable. In my position as maintainer, I can push stuff through, but I
prefer that you are in agreement.

Larry

Re: [PATCH] b43legacy: Fix machine check errors for PPC architecture with BCM4306/2

[Michael Buesch]

On Sunday 16 September 2007 01:26:29 Larry Finger wrote:
> Michael Buesch wrote:
> > 
> > There is no such thing as "connected".
> > The bit is called "gmode". Why do you change it back to the wrong name?
> > The V3 specs are simply _wrong_. It has nothing to do with the backplane
> > or anything else like that. The bit just selects between the A-PHY and
> > the BG-PHY. Of course, if there is no A-PHY it will obviously machine-check
> > on access, if it's selected.
> > 
> > V3 specs assume that the meaning is "connected", because it was misinterpreted
> > to be a "PHY connect" flag. But it really is a "PHY select" flag, to select
> > between two different PHYs. If it's enabled, the G-PHY will be selected (and
> > the A-PHY otherwise).
> > 
> > Btw, while we are at it: The V3 specs are wrong regarding the gmode (or connected)
> > bit in other areas as well, because there was even more confusion with
> > other PHY flags. The PHY specification is pretty broken in the v3 specs.
> 
> The critical part of the legacy code in this regard is from phy_calibrate():
> 
>         if (phy->type == B43legacy_PHYTYPE_G && phy->rev == 1) {
>                 b43legacy_wireless_core_reset(dev, 0);
>                 b43legacy_phy_initg(dev);
>                 b43legacy_wireless_core_reset(dev, B43legacy_TMSLOW_GMODE);
>         }

Here's how this was solved in bcm43xx-mac80211:

	if (phy->type == BCM43xx_PHYTYPE_G && phy->rev == 1) {
		/* Workaround: Temporarly disable gmode through the early init
		 * phase, as the gmode stuff is not needed for phy rev 1 */
		phy->gmode = 0;
		bcm43xx_wireless_core_reset(dev, 0);
		bcm43xx_phy_initg(dev);
		phy->gmode = 1;
		bcm43xx_wireless_core_reset(dev, BCM43xx_TMSLOW_GMODE);
	}

> If phy_initg is going to work correctly, then gmode has to follow the state of the MACCTL_GMODE bit
> in MMIO_MACCTL. Apparently GPHYs with rev > 1 do not care about this bit, but the BCM4306/2 does.
> The changes can be accomplished by the following patch:
> 
> Index: wireless-dev/drivers/net/wireless/b43legacy/main.c
> ===================================================================
> --- wireless-dev.orig/drivers/net/wireless/b43legacy/main.c
> +++ wireless-dev/drivers/net/wireless/b43legacy/main.c
> @@ -738,8 +738,11 @@ void b43legacy_wireless_core_reset(struc
> 
>         macctl = b43legacy_read32(dev, B43legacy_MMIO_MACCTL);
>         macctl &= ~B43legacy_MACCTL_GMODE;
> -       if (flags & B43legacy_TMSLOW_GMODE)
> +       if (flags & B43legacy_TMSLOW_GMODE) {
>                 macctl |= B43legacy_MACCTL_GMODE;
> +               dev->phy.gmode = 1;
> +       } else
> +               dev->phy.gmode = 0;
>         macctl |= B43legacy_MACCTL_IHR_ENABLED;
>         b43legacy_write32(dev, B43legacy_MMIO_MACCTL, macctl);
>  }

I think I am OK with both, this and the old bcm43xx-mac80211 fix I pasted above.

> The above patch and a revised comment for the gmode variable will be the contents of V3 of my patch.
> I hope that you find it acceptable. In my position as maintainer, I can push stuff through, but I
> prefer that you are in agreement.

Ok. Anyway. In the end you must work with that stuff and understand it. Not me.
So if you don't like "gmode" for whatever reason, rename it to something you like.
Just keep in mind please, that v4 PHY-specs (which mostly apply to your driver, too)
correctly talk about "gmode" and not "connected" anymore.
Also keep in mind that v3 specs are/were wrong in parts where it says to
check (phy->type == TYPE_G) where it should really test "gmode".
So if you change that back to "connected" the confusion gets even more complicated. :)
At least to me. All that stuff already screwed me a lot, so I am thankful that
it's gone and working correctly (at least for phy.rev > 1 cards) by now.
That's why I asked you to not add that crap back. :)

-- 
Greetings Michael.