[PATCH] b43: ensure ext PA lines are enabled for BCM4331

[PATCH] b43: ensure ext PA lines are enabled for BCM4331

[Seth Forshee]

Some MacBook Pro models with BCM4331 wireless will stop transmitting
after resuming from S3 without external power attached. This is fixed by
ensuring that the ext PA lines are enabled in BCMA_CC_CHIPCTL. Export
the function in bcma which does this for use by b43 and enable the ext
PA lines during wireless core initialization.

BugLink: http://bugs.launchpad.net/bugs/925577
Cc: Arend van Spriel <arend@broadcom.com>
Cc: stable@vger.kernel.org
Signed-off-by: Seth Forshee <seth.forshee@canonical.com>
---
 drivers/bcma/driver_chipcommon_pmu.c |    1 +
 drivers/net/wireless/b43/main.c      |    7 +++++++
 2 files changed, 8 insertions(+)

diff --git a/drivers/bcma/driver_chipcommon_pmu.c b/drivers/bcma/driver_chipcommon_pmu.c
index a058842..332d7fb 100644
--- a/drivers/bcma/driver_chipcommon_pmu.c
+++ b/drivers/bcma/driver_chipcommon_pmu.c
@@ -129,6 +129,7 @@ void bcma_chipco_bcm4331_ext_pa_lines_ctl(struct bcma_drv_cc *cc, bool enable)
 	}
 	bcma_cc_write32(cc, BCMA_CC_CHIPCTL, val);
 }
+EXPORT_SYMBOL_GPL(bcma_chipco_bcm4331_ext_pa_lines_ctl);
 
 void bcma_pmu_workarounds(struct bcma_drv_cc *cc)
 {
diff --git a/drivers/net/wireless/b43/main.c b/drivers/net/wireless/b43/main.c
index 5a39b22..6bac5cf 100644
--- a/drivers/net/wireless/b43/main.c
+++ b/drivers/net/wireless/b43/main.c
@@ -4713,6 +4713,13 @@ static int b43_wireless_core_init(struct b43_wldev *dev)
 	b43_upload_card_macaddress(dev);
 	b43_security_init(dev);
 
+#ifdef CONFIG_B43_BCMA
+	/* Required for tx to work on BCM4331 */
+	if (dev->dev->bus_type == B43_BUS_BCMA && dev->dev->chip_id == 0x4331)
+		bcma_chipco_bcm4331_ext_pa_lines_ctl(&dev->dev->bdev->bus->drv_cc,
+						     true);
+#endif
+
 	ieee80211_wake_queues(dev->wl->hw);
 
 	b43_set_status(dev, B43_STAT_INITIALIZED);
-- 
1.7.9.5

Re: [PATCH] b43: ensure ext PA lines are enabled for BCM4331

[Hauke Mehrtens]

On 05/31/2012 03:49 PM, Seth Forshee wrote:
> Some MacBook Pro models with BCM4331 wireless will stop transmitting
> after resuming from S3 without external power attached. This is fixed by
> ensuring that the ext PA lines are enabled in BCMA_CC_CHIPCTL. Export
> the function in bcma which does this for use by b43 and enable the ext
> PA lines during wireless core initialization.
> 
> BugLink: http://bugs.launchpad.net/bugs/925577
> Cc: Arend van Spriel <arend@broadcom.com>
> Cc: stable@vger.kernel.org
> Signed-off-by: Seth Forshee <seth.forshee@canonical.com>
> ---
>  drivers/bcma/driver_chipcommon_pmu.c |    1 +
>  drivers/net/wireless/b43/main.c      |    7 +++++++
>  2 files changed, 8 insertions(+)
> 
> diff --git a/drivers/bcma/driver_chipcommon_pmu.c b/drivers/bcma/driver_chipcommon_pmu.c
> index a058842..332d7fb 100644
> --- a/drivers/bcma/driver_chipcommon_pmu.c
> +++ b/drivers/bcma/driver_chipcommon_pmu.c
> @@ -129,6 +129,7 @@ void bcma_chipco_bcm4331_ext_pa_lines_ctl(struct bcma_drv_cc *cc, bool enable)
>  	}
>  	bcma_cc_write32(cc, BCMA_CC_CHIPCTL, val);
>  }
> +EXPORT_SYMBOL_GPL(bcma_chipco_bcm4331_ext_pa_lines_ctl);
>  
>  void bcma_pmu_workarounds(struct bcma_drv_cc *cc)
>  {
> diff --git a/drivers/net/wireless/b43/main.c b/drivers/net/wireless/b43/main.c
> index 5a39b22..6bac5cf 100644
> --- a/drivers/net/wireless/b43/main.c
> +++ b/drivers/net/wireless/b43/main.c
> @@ -4713,6 +4713,13 @@ static int b43_wireless_core_init(struct b43_wldev *dev)
>  	b43_upload_card_macaddress(dev);
>  	b43_security_init(dev);
>  
> +#ifdef CONFIG_B43_BCMA
> +	/* Required for tx to work on BCM4331 */
> +	if (dev->dev->bus_type == B43_BUS_BCMA && dev->dev->chip_id == 0x4331)
> +		bcma_chipco_bcm4331_ext_pa_lines_ctl(&dev->dev->bdev->bus->drv_cc,
> +						     true);
> +#endif
> +
>  	ieee80211_wake_queues(dev->wl->hw);
>  
>  	b43_set_status(dev, B43_STAT_INITIALIZED);

Hi Seth,

why don't you call this from bcma_pmu_workarounds() in
drivers/bcma/driver_chipcommon_pmu.c instead of calling this from b43? I
think it looks better to call some workarounds on chip common from bcma
and not from b43.
According to some Broadcom code this should also be called for chip_id
43431 when turning it on and in the sprom code.

Hauke

Re: [PATCH] b43: ensure ext PA lines are enabled for BCM4331

[Seth Forshee]

On Thu, May 31, 2012 at 04:16:05PM +0200, Hauke Mehrtens wrote:
> Hi Seth,
> 
> why don't you call this from bcma_pmu_workarounds() in
> drivers/bcma/driver_chipcommon_pmu.c instead of calling this from b43? I
> think it looks better to call some workarounds on chip common from bcma
> and not from b43.

Arend recommended calling it from within b43's start op, but I'm not
sure of the reason. Arend?

Agreed though that if there's no need to run it every time the interface
is started then bcma_pmu_workarounds() would be a nicer place for it.

> According to some Broadcom code this should also be called for chip_id
> 43431 when turning it on and in the sprom code.

I'm having trouble parsing this, specifically the "and in the sprom
code" part. Can you clarify?

Thanks,
Seth

Re: [PATCH] b43: ensure ext PA lines are enabled for BCM4331

[Hauke Mehrtens]

On 05/31/2012 04:26 PM, Seth Forshee wrote:
> On Thu, May 31, 2012 at 04:16:05PM +0200, Hauke Mehrtens wrote:
>> Hi Seth,
>>
>> why don't you call this from bcma_pmu_workarounds() in
>> drivers/bcma/driver_chipcommon_pmu.c instead of calling this from b43? I
>> think it looks better to call some workarounds on chip common from bcma
>> and not from b43.
> 
> Arend recommended calling it from within b43's start op, but I'm not
> sure of the reason. Arend?
> 
> Agreed though that if there's no need to run it every time the interface
> is started then bcma_pmu_workarounds() would be a nicer place for it.

brcmsmac calls the chip specific the workarounds in ai_doattach(), but
the ones for BCM4313 and BCM43224 are also in bcma_pmu_workarounds(),
this should be cleaned up to have them just at one place. In some old
version of brcmsmac the workaround for the BCM4331 was also called from
ai_doattach() function, but later removed likely because this devices is
not supported by brcmsmac.
So if there is not better reason as, the proprietary Broadcom driver
does so, I would like to see this call in bcma_pmu_workarounds().

>> According to some Broadcom code this should also be called for chip_id
>> 43431 when turning it on and in the sprom code.
> 
> I'm having trouble parsing this, specifically the "and in the sprom
> code" part. Can you clarify?

In the Broadcom SDK code si_chipcontrl_epa4331(), the same function as
bcma_chipco_bcm4331_ext_pa_lines_ctl() in bcma, gets called for devices
with a chip id of 0x4331 and 43431, both seam to be BCM4331 devices. We
should also call our workarounds for both chip ids.

Hauke

Re: [PATCH] b43: ensure ext PA lines are enabled for BCM4331

[Arend van Spriel]

On 05/31/2012 04:26 PM, Seth Forshee wrote:
> On Thu, May 31, 2012 at 04:16:05PM +0200, Hauke Mehrtens wrote:
>> > Hi Seth,
>> > 
>> > why don't you call this from bcma_pmu_workarounds() in
>> > drivers/bcma/driver_chipcommon_pmu.c instead of calling this from b43? I
>> > think it looks better to call some workarounds on chip common from bcma
>> > and not from b43.
> Arend recommended calling it from within b43's start op, but I'm not
> sure of the reason. Arend?

Well, my comment on start op was regarding your observation of CHIPCTL
being zero. As the start op does reinitialize the hardware you should
check CHIPCTL register at the end of it. This is just to confirm the
register contains valid bits.

> Agreed though that if there's no need to run it every time the interface
> is started then bcma_pmu_workarounds() would be a nicer place for it.
> 

So I agree with Hauke and have this code in bcma. Just not sure whether
bcma_pmu_workarounds() is the place to put it. This is not a pmu related
register, but as the function was already there I will not stress to
move it to driver_chipcommon.c ;-)

Gr. AvS

Re: [PATCH] b43: ensure ext PA lines are enabled for BCM4331

[Seth Forshee]

On Thu, May 31, 2012 at 06:23:25PM +0200, Hauke Mehrtens wrote:
> On 05/31/2012 04:26 PM, Seth Forshee wrote:
> > On Thu, May 31, 2012 at 04:16:05PM +0200, Hauke Mehrtens wrote:
> >> Hi Seth,
> >>
> >> why don't you call this from bcma_pmu_workarounds() in
> >> drivers/bcma/driver_chipcommon_pmu.c instead of calling this from b43? I
> >> think it looks better to call some workarounds on chip common from bcma
> >> and not from b43.
> > 
> > Arend recommended calling it from within b43's start op, but I'm not
> > sure of the reason. Arend?
> > 
> > Agreed though that if there's no need to run it every time the interface
> > is started then bcma_pmu_workarounds() would be a nicer place for it.
> 
> brcmsmac calls the chip specific the workarounds in ai_doattach(), but
> the ones for BCM4313 and BCM43224 are also in bcma_pmu_workarounds(),
> this should be cleaned up to have them just at one place. In some old
> version of brcmsmac the workaround for the BCM4331 was also called from
> ai_doattach() function, but later removed likely because this devices is
> not supported by brcmsmac.
> So if there is not better reason as, the proprietary Broadcom driver
> does so, I would like to see this call in bcma_pmu_workarounds().

I found the SDK code from a router source package, and oddly that code
is clearing those bits in si_doattach(). In fact si_chipcontrl_epa4331()
is never called to enable the ext PA lines, only to disable them. The
sprom code reads the value before disabling them and restores it after
reading from the sprom. Enabling the ext PA pins is what fixes things
for me. Predictably, duplicating this in bcma makes wireless broken all
the time. 

At any rate, it sounds like we're agreed to add the workaround in
bcma_pmu_workarounds(), so I'll redo the patch.

> >> According to some Broadcom code this should also be called for chip_id
> >> 43431 when turning it on and in the sprom code.
> > 
> > I'm having trouble parsing this, specifically the "and in the sprom
> > code" part. Can you clarify?
> 
> In the Broadcom SDK code si_chipcontrl_epa4331(), the same function as
> bcma_chipco_bcm4331_ext_pa_lines_ctl() in bcma, gets called for devices
> with a chip id of 0x4331 and 43431, both seam to be BCM4331 devices. We
> should also call our workarounds for both chip ids.

I'll fix this too.

Thanks,
Seth

Re: [PATCH] b43: ensure ext PA lines are enabled for BCM4331

[Hauke Mehrtens]

On 06/01/2012 12:06 AM, Seth Forshee wrote:
> On Thu, May 31, 2012 at 06:23:25PM +0200, Hauke Mehrtens wrote:
>> On 05/31/2012 04:26 PM, Seth Forshee wrote:
>>> On Thu, May 31, 2012 at 04:16:05PM +0200, Hauke Mehrtens wrote:
>>>> Hi Seth,
>>>>
>>>> why don't you call this from bcma_pmu_workarounds() in
>>>> drivers/bcma/driver_chipcommon_pmu.c instead of calling this from b43? I
>>>> think it looks better to call some workarounds on chip common from bcma
>>>> and not from b43.
>>>
>>> Arend recommended calling it from within b43's start op, but I'm not
>>> sure of the reason. Arend?
>>>
>>> Agreed though that if there's no need to run it every time the interface
>>> is started then bcma_pmu_workarounds() would be a nicer place for it.
>>
>> brcmsmac calls the chip specific the workarounds in ai_doattach(), but
>> the ones for BCM4313 and BCM43224 are also in bcma_pmu_workarounds(),
>> this should be cleaned up to have them just at one place. In some old
>> version of brcmsmac the workaround for the BCM4331 was also called from
>> ai_doattach() function, but later removed likely because this devices is
>> not supported by brcmsmac.
>> So if there is not better reason as, the proprietary Broadcom driver
>> does so, I would like to see this call in bcma_pmu_workarounds().
> 
> I found the SDK code from a router source package, and oddly that code
> is clearing those bits in si_doattach(). In fact si_chipcontrl_epa4331()
> is never called to enable the ext PA lines, only to disable them. The
> sprom code reads the value before disabling them and restores it after
> reading from the sprom. Enabling the ext PA pins is what fixes things
> for me. Predictably, duplicating this in bcma makes wireless broken all
> the time.

The open source SDK just contains some of the code, the wireless part is
closed source and I haven't seen any call which enables this in the open
part too. The initial commit of brcm80211 to the kernel staging area
contains a call to this function with true given.
It is commit a9533e7ea3c410fed2f4cd8b3e1e213e48529b75 if you are interested.

> At any rate, it sounds like we're agreed to add the workaround in
> bcma_pmu_workarounds(), so I'll redo the patch.
> 
>>>> According to some Broadcom code this should also be called for chip_id
>>>> 43431 when turning it on and in the sprom code.
>>>
>>> I'm having trouble parsing this, specifically the "and in the sprom
>>> code" part. Can you clarify?
>>
>> In the Broadcom SDK code si_chipcontrl_epa4331(), the same function as
>> bcma_chipco_bcm4331_ext_pa_lines_ctl() in bcma, gets called for devices
>> with a chip id of 0x4331 and 43431, both seam to be BCM4331 devices. We
>> should also call our workarounds for both chip ids.
> 
> I'll fix this too.
> 
> Thanks,
> Seth
>