* [PATCH 1/4] b43: HT-PHY: fix masks in radio ctl
@ 2011-07-17 8:30 Rafał Miłecki
2011-07-17 8:43 ` Michael Büsch
0 siblings, 1 reply; 6+ messages in thread
From: Rafał Miłecki @ 2011-07-17 8:30 UTC (permalink / raw)
To: linux-wireless, John W. Linville; +Cc: b43-dev, Rafał Miłecki
Old masks were causing ugly, delayed lock ups.
Signed-off-by: Rafa? Mi?ecki <zajec5@gmail.com>
---
drivers/net/wireless/b43/phy_ht.c | 10 +++++-----
1 files changed, 5 insertions(+), 5 deletions(-)
diff --git a/drivers/net/wireless/b43/phy_ht.c b/drivers/net/wireless/b43/phy_ht.c
index 2982103..0cc293a 100644
--- a/drivers/net/wireless/b43/phy_ht.c
+++ b/drivers/net/wireless/b43/phy_ht.c
@@ -277,12 +277,12 @@ static void b43_phy_ht_op_software_rfkill(struct b43_wldev *dev,
b43err(dev->wl, "MAC not suspended\n");
if (blocked) {
- b43_phy_mask(dev, B43_PHY_HT_RF_CTL1, ~0);
+ b43_phy_mask(dev, B43_PHY_HT_RF_CTL1, 0);
} else {
- b43_phy_mask(dev, B43_PHY_HT_RF_CTL1, ~0);
- b43_phy_maskset(dev, B43_PHY_HT_RF_CTL1, ~0, 0x1);
- b43_phy_mask(dev, B43_PHY_HT_RF_CTL1, ~0);
- b43_phy_maskset(dev, B43_PHY_HT_RF_CTL1, ~0, 0x2);
+ b43_phy_mask(dev, B43_PHY_HT_RF_CTL1, 0);
+ b43_phy_maskset(dev, B43_PHY_HT_RF_CTL1, 0, 0x1);
+ b43_phy_mask(dev, B43_PHY_HT_RF_CTL1, 0);
+ b43_phy_maskset(dev, B43_PHY_HT_RF_CTL1, 0, 0x2);
if (dev->phy.radio_ver == 0x2059)
b43_radio_2059_init(dev);
--
1.7.3.4
^ permalink raw reply related [flat|nested] 6+ messages in thread
* [PATCH 1/4] b43: HT-PHY: fix masks in radio ctl
2011-07-17 8:30 Rafał Miłecki
@ 2011-07-17 8:43 ` Michael Büsch
2011-07-17 15:05 ` Larry Finger
` (2 more replies)
0 siblings, 3 replies; 6+ messages in thread
From: Michael Büsch @ 2011-07-17 8:43 UTC (permalink / raw)
To: Rafał Miłecki; +Cc: linux-wireless, John W. Linville, b43-dev
On Sun, 17 Jul 2011 10:30:31 +0200
Rafa? Mi?ecki <zajec5@gmail.com> wrote:
> Old masks were causing ugly, delayed lock ups.
>
> Signed-off-by: Rafa? Mi?ecki <zajec5@gmail.com>
> ---
> drivers/net/wireless/b43/phy_ht.c | 10 +++++-----
> 1 files changed, 5 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/net/wireless/b43/phy_ht.c b/drivers/net/wireless/b43/phy_ht.c
> index 2982103..0cc293a 100644
> --- a/drivers/net/wireless/b43/phy_ht.c
> +++ b/drivers/net/wireless/b43/phy_ht.c
> @@ -277,12 +277,12 @@ static void b43_phy_ht_op_software_rfkill(struct b43_wldev *dev,
> b43err(dev->wl, "MAC not suspended\n");
>
> if (blocked) {
> - b43_phy_mask(dev, B43_PHY_HT_RF_CTL1, ~0);
> + b43_phy_mask(dev, B43_PHY_HT_RF_CTL1, 0);
> } else {
> - b43_phy_mask(dev, B43_PHY_HT_RF_CTL1, ~0);
> - b43_phy_maskset(dev, B43_PHY_HT_RF_CTL1, ~0, 0x1);
> - b43_phy_mask(dev, B43_PHY_HT_RF_CTL1, ~0);
> - b43_phy_maskset(dev, B43_PHY_HT_RF_CTL1, ~0, 0x2);
> + b43_phy_mask(dev, B43_PHY_HT_RF_CTL1, 0);
> + b43_phy_maskset(dev, B43_PHY_HT_RF_CTL1, 0, 0x1);
> + b43_phy_mask(dev, B43_PHY_HT_RF_CTL1, 0);
> + b43_phy_maskset(dev, B43_PHY_HT_RF_CTL1, 0, 0x2);
>
> if (dev->phy.radio_ver == 0x2059)
> b43_radio_2059_init(dev);
Why are we using mask/maskset here at all, if the mask is zero?
You could just use phy_write.
(And mask() with ~0 should always ring a bell anyway, because it does
nothing except for possible side effects of the register read/write).
^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH 1/4] b43: HT-PHY: fix masks in radio ctl
2011-07-17 8:43 ` Michael Büsch
@ 2011-07-17 15:05 ` Larry Finger
2011-07-17 16:42 ` Rafał Miłecki
2011-07-17 16:56 ` Rafał Miłecki
2 siblings, 0 replies; 6+ messages in thread
From: Larry Finger @ 2011-07-17 15:05 UTC (permalink / raw)
To: Michael Büsch
Cc: Rafał Miłecki, linux-wireless, John W. Linville,
b43-dev
On 07/17/2011 03:43 AM, Michael B?sch wrote:
> On Sun, 17 Jul 2011 10:30:31 +0200
> Rafa? Mi?ecki<zajec5@gmail.com> wrote:
>
>> Old masks were causing ugly, delayed lock ups.
>>
>> Signed-off-by: Rafa? Mi?ecki<zajec5@gmail.com>
>> ---
>> drivers/net/wireless/b43/phy_ht.c | 10 +++++-----
>> 1 files changed, 5 insertions(+), 5 deletions(-)
>>
>> diff --git a/drivers/net/wireless/b43/phy_ht.c b/drivers/net/wireless/b43/phy_ht.c
>> index 2982103..0cc293a 100644
>> --- a/drivers/net/wireless/b43/phy_ht.c
>> +++ b/drivers/net/wireless/b43/phy_ht.c
>> @@ -277,12 +277,12 @@ static void b43_phy_ht_op_software_rfkill(struct b43_wldev *dev,
>> b43err(dev->wl, "MAC not suspended\n");
>>
>> if (blocked) {
>> - b43_phy_mask(dev, B43_PHY_HT_RF_CTL1, ~0);
>> + b43_phy_mask(dev, B43_PHY_HT_RF_CTL1, 0);
>> } else {
>> - b43_phy_mask(dev, B43_PHY_HT_RF_CTL1, ~0);
>> - b43_phy_maskset(dev, B43_PHY_HT_RF_CTL1, ~0, 0x1);
>> - b43_phy_mask(dev, B43_PHY_HT_RF_CTL1, ~0);
>> - b43_phy_maskset(dev, B43_PHY_HT_RF_CTL1, ~0, 0x2);
>> + b43_phy_mask(dev, B43_PHY_HT_RF_CTL1, 0);
>> + b43_phy_maskset(dev, B43_PHY_HT_RF_CTL1, 0, 0x1);
>> + b43_phy_mask(dev, B43_PHY_HT_RF_CTL1, 0);
>> + b43_phy_maskset(dev, B43_PHY_HT_RF_CTL1, 0, 0x2);
>>
>> if (dev->phy.radio_ver == 0x2059)
>> b43_radio_2059_init(dev);
>
> Why are we using mask/maskset here at all, if the mask is zero?
> You could just use phy_write.
>
> (And mask() with ~0 should always ring a bell anyway, because it does
> nothing except for possible side effects of the register read/write).
All of the Broadcom mask and maskset routines are called with the complement of
the mask. It has been a constant battle during the RE work to keep that straight.
Larry
^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH 1/4] b43: HT-PHY: fix masks in radio ctl
2011-07-17 8:43 ` Michael Büsch
2011-07-17 15:05 ` Larry Finger
@ 2011-07-17 16:42 ` Rafał Miłecki
2011-07-17 16:56 ` Rafał Miłecki
2 siblings, 0 replies; 6+ messages in thread
From: Rafał Miłecki @ 2011-07-17 16:42 UTC (permalink / raw)
To: Michael Büsch; +Cc: linux-wireless, John W. Linville, b43-dev
W dniu 17 lipca 2011 10:43 u?ytkownik Michael B?sch <m@bues.ch> napisa?:
> On Sun, 17 Jul 2011 10:30:31 +0200
> Rafa? Mi?ecki <zajec5@gmail.com> wrote:
>
>> Old masks were causing ugly, delayed lock ups.
>>
>> Signed-off-by: Rafa? Mi?ecki <zajec5@gmail.com>
>> ---
>> ?drivers/net/wireless/b43/phy_ht.c | ? 10 +++++-----
>> ?1 files changed, 5 insertions(+), 5 deletions(-)
>>
>> diff --git a/drivers/net/wireless/b43/phy_ht.c b/drivers/net/wireless/b43/phy_ht.c
>> index 2982103..0cc293a 100644
>> --- a/drivers/net/wireless/b43/phy_ht.c
>> +++ b/drivers/net/wireless/b43/phy_ht.c
>> @@ -277,12 +277,12 @@ static void b43_phy_ht_op_software_rfkill(struct b43_wldev *dev,
>> ? ? ? ? ? ? ? b43err(dev->wl, "MAC not suspended\n");
>>
>> ? ? ? if (blocked) {
>> - ? ? ? ? ? ? b43_phy_mask(dev, B43_PHY_HT_RF_CTL1, ~0);
>> + ? ? ? ? ? ? b43_phy_mask(dev, B43_PHY_HT_RF_CTL1, 0);
>> ? ? ? } else {
>> - ? ? ? ? ? ? b43_phy_mask(dev, B43_PHY_HT_RF_CTL1, ~0);
>> - ? ? ? ? ? ? b43_phy_maskset(dev, B43_PHY_HT_RF_CTL1, ~0, 0x1);
>> - ? ? ? ? ? ? b43_phy_mask(dev, B43_PHY_HT_RF_CTL1, ~0);
>> - ? ? ? ? ? ? b43_phy_maskset(dev, B43_PHY_HT_RF_CTL1, ~0, 0x2);
>> + ? ? ? ? ? ? b43_phy_mask(dev, B43_PHY_HT_RF_CTL1, 0);
>> + ? ? ? ? ? ? b43_phy_maskset(dev, B43_PHY_HT_RF_CTL1, 0, 0x1);
>> + ? ? ? ? ? ? b43_phy_mask(dev, B43_PHY_HT_RF_CTL1, 0);
>> + ? ? ? ? ? ? b43_phy_maskset(dev, B43_PHY_HT_RF_CTL1, 0, 0x2);
>>
>> ? ? ? ? ? ? ? if (dev->phy.radio_ver == 0x2059)
>> ? ? ? ? ? ? ? ? ? ? ? b43_radio_2059_init(dev);
>
> Why are we using mask/maskset here at all, if the mask is zero?
> You could just use phy_write.
>
> (And mask() with ~0 should always ring a bell anyway, because it does
> nothing except for possible side effects of the register read/write).
Yeah, I implemented it this way as the result of my rush in writing HT
code. We can ignore wl's ops and be smarter in such a cases.
--
Rafa?
^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH 1/4] b43: HT-PHY: fix masks in radio ctl
2011-07-17 8:43 ` Michael Büsch
2011-07-17 15:05 ` Larry Finger
2011-07-17 16:42 ` Rafał Miłecki
@ 2011-07-17 16:56 ` Rafał Miłecki
2 siblings, 0 replies; 6+ messages in thread
From: Rafał Miłecki @ 2011-07-17 16:56 UTC (permalink / raw)
To: Michael Büsch; +Cc: linux-wireless, John W. Linville, b43-dev
W dniu 17 lipca 2011 10:43 u?ytkownik Michael B?sch <m@bues.ch> napisa?:
> On Sun, 17 Jul 2011 10:30:31 +0200
> Rafa? Mi?ecki <zajec5@gmail.com> wrote:
>
>> Old masks were causing ugly, delayed lock ups.
>>
>> Signed-off-by: Rafa? Mi?ecki <zajec5@gmail.com>
>> ---
>> ?drivers/net/wireless/b43/phy_ht.c | ? 10 +++++-----
>> ?1 files changed, 5 insertions(+), 5 deletions(-)
>>
>> diff --git a/drivers/net/wireless/b43/phy_ht.c b/drivers/net/wireless/b43/phy_ht.c
>> index 2982103..0cc293a 100644
>> --- a/drivers/net/wireless/b43/phy_ht.c
>> +++ b/drivers/net/wireless/b43/phy_ht.c
>> @@ -277,12 +277,12 @@ static void b43_phy_ht_op_software_rfkill(struct b43_wldev *dev,
>> ? ? ? ? ? ? ? b43err(dev->wl, "MAC not suspended\n");
>>
>> ? ? ? if (blocked) {
>> - ? ? ? ? ? ? b43_phy_mask(dev, B43_PHY_HT_RF_CTL1, ~0);
>> + ? ? ? ? ? ? b43_phy_mask(dev, B43_PHY_HT_RF_CTL1, 0);
>> ? ? ? } else {
>> - ? ? ? ? ? ? b43_phy_mask(dev, B43_PHY_HT_RF_CTL1, ~0);
>> - ? ? ? ? ? ? b43_phy_maskset(dev, B43_PHY_HT_RF_CTL1, ~0, 0x1);
>> - ? ? ? ? ? ? b43_phy_mask(dev, B43_PHY_HT_RF_CTL1, ~0);
>> - ? ? ? ? ? ? b43_phy_maskset(dev, B43_PHY_HT_RF_CTL1, ~0, 0x2);
>> + ? ? ? ? ? ? b43_phy_mask(dev, B43_PHY_HT_RF_CTL1, 0);
>> + ? ? ? ? ? ? b43_phy_maskset(dev, B43_PHY_HT_RF_CTL1, 0, 0x1);
>> + ? ? ? ? ? ? b43_phy_mask(dev, B43_PHY_HT_RF_CTL1, 0);
>> + ? ? ? ? ? ? b43_phy_maskset(dev, B43_PHY_HT_RF_CTL1, 0, 0x2);
>>
>> ? ? ? ? ? ? ? if (dev->phy.radio_ver == 0x2059)
>> ? ? ? ? ? ? ? ? ? ? ? b43_radio_2059_init(dev);
>
> Why are we using mask/maskset here at all, if the mask is zero?
> You could just use phy_write.
>
> (And mask() with ~0 should always ring a bell anyway, because it does
> nothing except for possible side effects of the register read/write).
Well, after re-thinking it, I've some doubts.
Thanks to using maskset we got read after every write. We have no idea
it this is needed or no. What we are really sure is that this part of
code is really sensitive. Writing 0x3 cause horrible lock ups, really
ugly to track.
Should we use read-after-write then? Or maybe just keep wl's dummy way
of operating on this register?
--
Rafa?
^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH 1/4] b43: HT-PHY: fix masks in radio ctl
@ 2011-07-18 0:13 Rafał Miłecki
0 siblings, 0 replies; 6+ messages in thread
From: Rafał Miłecki @ 2011-07-18 0:13 UTC (permalink / raw)
To: linux-wireless, John W. Linville; +Cc: b43-dev, Rafał Miłecki
Old masks were causing ugly, delayed lock ups.
Signed-off-by: Rafa? Mi?ecki <zajec5@gmail.com>
---
V2: Explain why we do such a dummy operations like: write(read() & 0).
Put cleaning this as TODO. We should wait with cleaning until having BCM4331
working at all.
---
drivers/net/wireless/b43/phy_ht.c | 15 ++++++++++-----
1 files changed, 10 insertions(+), 5 deletions(-)
diff --git a/drivers/net/wireless/b43/phy_ht.c b/drivers/net/wireless/b43/phy_ht.c
index 2982103..85a30af 100644
--- a/drivers/net/wireless/b43/phy_ht.c
+++ b/drivers/net/wireless/b43/phy_ht.c
@@ -276,13 +276,18 @@ static void b43_phy_ht_op_software_rfkill(struct b43_wldev *dev,
if (b43_read32(dev, B43_MMIO_MACCTL) & B43_MACCTL_ENABLED)
b43err(dev->wl, "MAC not suspended\n");
+ /* In the following PHY ops we copy wl's dummy behaviour.
+ * TODO: Find out if reads (currently hidden in masks/masksets) are
+ * needed and replace following ops with just writes or w&r.
+ * Note: B43_PHY_HT_RF_CTL1 register is tricky, wrong operation can
+ * cause delayed (!) machine lock up. */
if (blocked) {
- b43_phy_mask(dev, B43_PHY_HT_RF_CTL1, ~0);
+ b43_phy_mask(dev, B43_PHY_HT_RF_CTL1, 0);
} else {
- b43_phy_mask(dev, B43_PHY_HT_RF_CTL1, ~0);
- b43_phy_maskset(dev, B43_PHY_HT_RF_CTL1, ~0, 0x1);
- b43_phy_mask(dev, B43_PHY_HT_RF_CTL1, ~0);
- b43_phy_maskset(dev, B43_PHY_HT_RF_CTL1, ~0, 0x2);
+ b43_phy_mask(dev, B43_PHY_HT_RF_CTL1, 0);
+ b43_phy_maskset(dev, B43_PHY_HT_RF_CTL1, 0, 0x1);
+ b43_phy_mask(dev, B43_PHY_HT_RF_CTL1, 0);
+ b43_phy_maskset(dev, B43_PHY_HT_RF_CTL1, 0, 0x2);
if (dev->phy.radio_ver == 0x2059)
b43_radio_2059_init(dev);
--
1.7.3.4
^ permalink raw reply related [flat|nested] 6+ messages in thread
end of thread, other threads:[~2011-07-18 0:13 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2011-07-18 0:13 [PATCH 1/4] b43: HT-PHY: fix masks in radio ctl Rafał Miłecki
-- strict thread matches above, loose matches on Subject: below --
2011-07-17 8:30 Rafał Miłecki
2011-07-17 8:43 ` Michael Büsch
2011-07-17 15:05 ` Larry Finger
2011-07-17 16:42 ` Rafał Miłecki
2011-07-17 16:56 ` Rafał Miłecki
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).