BCM4331 tx failures after S3

BCM4331 tx failures after S3

[Arend van Spriel]

On 05/25/2012 08:34 PM, Seth Forshee wrote:
> On Fri, May 25, 2012 at 06:47:27PM +0200, Arend van Spriel wrote:
>> On 05/25/2012 04:13 PM, Seth Forshee wrote:
>>> On Thu, May 24, 2012 at 11:34:37PM +0200, Hauke Mehrtens wrote:
>>>> Hi Seth,
>>>>
>>>> rmmod and insmod of b43 does not help but doing this with b43 and bcma
>>>> works is that correct?
>>>> Have you tried to add this code from bcma_host_pci_probe() to resume:
>>>>
>>>> 	/* Disable the RETRY_TIMEOUT register (0x41) to keep
>>>> 	 * PCI Tx retries from interfering with C3 CPU state */
>>>> 	pci_read_config_dword(dev, 0x40, &val);
>>>> 	if ((val & 0x0000ff00) != 0)
>>>> 		pci_write_config_dword(dev, 0x40, val & 0xffff00ff);
>>>>
>>>> Could you also try to run bcma_sprom_get() after resume.
>>>
>>> Okay, this one fixes it. The part that's needed seems to be:
>>>
>>>         if (bus->chipinfo.id == 0x4331)
>>>                 bcma_chipco_bcm4331_ext_pa_lines_ctl(&bus->drv_cc, true);
>>>
>>> I could have sworn I tried adding this to resume before, but apparently
>>> I didn't.
>>>
>>> Hmm. It turns out that BCMA_CC_CHIPCTL is completely reset to 0 when on
>>> S3 without AC power, meaning all other fields in the register get
>>> cleared as well. On this MBP that's affecting
>>> BCMA_CHIPCTL_4331_BT_COEXIST. So we could save the value at suspend and
>>> restore it during resume, but maybe the initialization of this register
>>> also needs to be improved. Thoughts?
>>>
>>> Thanks,
>>> Seth
>>>
>>>
>>
>> Well let me explain some behaviour. Upon suspend mac80211 takes down the
>> driver with the last callback being .stop(). Upon resume mac80211 calls
>> the driver with .start() callback.
>>
>> The brcmsmac driver has two bool states that are in play here: driver_up
>> and hw_up. Upon .stop() callback the driver_up is set to false, but
>> hw_up remains true. A subsequent .start() will perform some
>> reconfiguration and driver_up will be made true.
>>
>> The suspend/resume scenario is different for brcmsmac. bcma will call
>> brcmsmac .suspend() callback in which we set hw_up to false. This
>> changes the behaviour of the .start() callback after resume. Basically,
>> it reinitializes the hardware.
> 
> I'm having a bit of a hard time understanding the relevance of your
> description, since we're talking about b43. Are you suggesting that b43
> should do something similar? But even then I don't see any of the
> reinitialization done by brcmsmac setting BCMA_CC_CHIPCTL with the
> needed value. Am I missing something?
> 
> Seth
> 

Yes, that is my suggestion. I dived into the b43 driver, but I do not
see a .resume callback for bcma:

static struct bcma_driver b43_bcma_driver = {
	.name		= KBUILD_MODNAME,
	.id_table	= b43_bcma_tbl,
	.probe		= b43_bcma_probe,
	.remove		= b43_bcma_remove,
};

So upon suspend/resume b43 seems to take no extra steps, but I may be
missing it as I am not a b43 specialist (let me CC the b43 developer
list :-) ).

In brcmsmac the brcms_b_hw_up() is called in brcms_c_up() when the flag
hw_up is false:

int brcms_c_up(struct brcms_c_info *wlc)
{
	if (!wlc->pub->hw_up) {
		brcms_b_hw_up(wlc->hw);
		wlc->pub->hw_up = true;
	}

/* Initialize just the hardware when coming out of POR or S3/S5 system
states */
static void brcms_b_hw_up(struct brcms_hardware *wlc_hw)

You are right that this particular code does not fiddle with
BCMA_CC_CHIPCTL. Where in the code did you print the value of this
register? Before or after the mac80211 .start() callback has been called?

Gr. AvS

BCM4331 tx failures after S3

[Seth Forshee]

On Fri, May 25, 2012 at 10:19:43PM +0200, Arend van Spriel wrote:
> >> Well let me explain some behaviour. Upon suspend mac80211 takes down the
> >> driver with the last callback being .stop(). Upon resume mac80211 calls
> >> the driver with .start() callback.
> >>
> >> The brcmsmac driver has two bool states that are in play here: driver_up
> >> and hw_up. Upon .stop() callback the driver_up is set to false, but
> >> hw_up remains true. A subsequent .start() will perform some
> >> reconfiguration and driver_up will be made true.
> >>
> >> The suspend/resume scenario is different for brcmsmac. bcma will call
> >> brcmsmac .suspend() callback in which we set hw_up to false. This
> >> changes the behaviour of the .start() callback after resume. Basically,
> >> it reinitializes the hardware.
> > 
> > I'm having a bit of a hard time understanding the relevance of your
> > description, since we're talking about b43. Are you suggesting that b43
> > should do something similar? But even then I don't see any of the
> > reinitialization done by brcmsmac setting BCMA_CC_CHIPCTL with the
> > needed value. Am I missing something?
> > 
> > Seth
> > 
> 
> Yes, that is my suggestion. I dived into the b43 driver, but I do not
> see a .resume callback for bcma:
> 
> static struct bcma_driver b43_bcma_driver = {
> 	.name		= KBUILD_MODNAME,
> 	.id_table	= b43_bcma_tbl,
> 	.probe		= b43_bcma_probe,
> 	.remove		= b43_bcma_remove,
> };
> 
> So upon suspend/resume b43 seems to take no extra steps, but I may be
> missing it as I am not a b43 specialist (let me CC the b43 developer
> list :-) ).

I don't think your missing it. There doesn't seem to be a resume handler
in b43.

> In brcmsmac the brcms_b_hw_up() is called in brcms_c_up() when the flag
> hw_up is false:
> 
> int brcms_c_up(struct brcms_c_info *wlc)
> {
> 	if (!wlc->pub->hw_up) {
> 		brcms_b_hw_up(wlc->hw);
> 		wlc->pub->hw_up = true;
> 	}
> 
> /* Initialize just the hardware when coming out of POR or S3/S5 system
> states */
> static void brcms_b_hw_up(struct brcms_hardware *wlc_hw)
> 
> You are right that this particular code does not fiddle with
> BCMA_CC_CHIPCTL. Where in the code did you print the value of this
> register? Before or after the mac80211 .start() callback has been called?

Before. I printed the value from bcma_host_pci_resume().

I can easily make a patch that fixes the chipctl register up as needed
during resume to fix my problem. But based on the fact that other fields
in the register are also changing value, I'm wondering if more thorough
handling is needed for the register. And I'm also a little fuzzy on
exactly who should be responsible for what in the relationship between
b43 and the wireless drivers.

Thanks,
Seth

BCM4331 tx failures after S3

[Arend van Spriel]

On 05/25/2012 10:44 PM, Seth Forshee wrote:
> Before. I printed the value from bcma_host_pci_resume().
> 
> I can easily make a patch that fixes the chipctl register up as needed
> during resume to fix my problem. But based on the fact that other fields
> in the register are also changing value, I'm wondering if more thorough
> handling is needed for the register.

Ok. I dived into b43 and it does disable the wireless core in the
.stop() callback. So I believe it does reinitialize the device when
.start() callback is being called after resume. So I think you should
check the chipctl register at the end of the .start() callback.

> And I'm also a little fuzzy on
> exactly who should be responsible for what in the relationship between
> b43 and the wireless drivers.

I assume you mean bcma instead of b43, right? I admit it is a bit shady,
because bcma is more than just a bus driver. It also contains drivers
for the chipcommon and pcie cores on the device.

Gr. AvS