[rt2x00-users] [PATCH] Rt2x00 : Fix the MCU request and status interaction .
DURST ...
dmk.mtd at hotmail.com
Sun Apr 19 06:49:39 CDT 2009
Hi man
thanks ...
Today thats oks =)
> From: ivdoorn at gmail.com
> To: prahal at yahoo.com
> Date: Sun, 19 Apr 2009 10:13:25 +0200
> CC: users at rt2x00.serialmonkey.com
> Subject: Re: [rt2x00-users] [PATCH] Rt2x00 : Fix the MCU request and status interaction .
>
> Hi,
>
> > I guesss somewhat in that I receive frames only they seems to be HT ones
> > and unable to be processed by the rt2x00lib_rxdone_read_signal in
> > rt2x00dev.c:
> > Apr 18 02:10:58 jason kernel: [ 1759.628394] phy0 ->
> > rt2x00lib_rxdone_read_signal: Warning - Frame received with
> > unrecognized signal, signal=0x0026, type=4
> > (...)
> > though most of the time it was signal 0x0075 with this RXDONE_SIGNAL_MCU
> > type too.
>
> Yeah Ralink reported problems with that rate reporting as well.
> It is quite odd, almost all frames are reported as CCK rates, and some values
> look quite odd. I almost suspect the wrong value to be read... :S
>
> > I also have warning that seems of minimal importance at this stage:
> > Apr 18 02:43:05 jason kernel: [ 3686.253422] WARNING: at
> > net/mac80211/rx.c:2483 __ieee80211_rx+0x663/0x670 [mac80211]()
>
> This is related to the above problem, we are sending the HT flag with an
> invalid HT rate to mac80211.
>
> > Scanning returns no results. Though I believe this is due to those
> > signal not being processed (0x0026, 0x0075, etc) .
>
> No, the rate warnings in rt2x00dev indicate that the wrong bitrate was
> detected, after which rt2x00 will set the bitrate to 1Mbs and forward the
> frame to mac80211 as usual. If the invalid rate is send to mac80211 only
> then the frame would be dropped.
> So for each frame which produces the rate warning in rt2x00 is actually send
> to mac80211 as normal.
>
> > The improvement for pci (cannot test usb) is that the behaviour is the
> > same at boot and at module reloading for radio state on, and the state
> > awake does not works once each two attempts. More on those follows.
>
> During my testing I haven't experienced MCU problems with rt2800usb,
> but perhaps your patch will fix some other issues which I haven't related
> to MCU yet. :)
>
> > >> if (WAIT_FOR_MCU(rt2x00dev, ®)) {
> > >> + rt2x00pci_register_write(rt2x00dev, H2M_MAILBOX_STATUS, ~0);
> > >> + rt2x00pci_register_write(rt2x00dev, H2M_MAILBOX_CID, ~0);
> > >> rt2x00_set_field32(®, H2M_MAILBOX_CSR_OWNER, 1);
> > >> rt2x00_set_field32(®, H2M_MAILBOX_CSR_CMD_TOKEN, token);
> > >> rt2x00_set_field32(®, H2M_MAILBOX_CSR_ARG0, arg0);
> > >>
> > >
> > > Can this upset calls to rt2800pci_mcu_status() when MCU is accessed in multiple threads?
> > >
> > >
> >
> > Gosh ."Could" but I hope not . Though this one was to avoid the need to
> > call set_state awake twice to have the status working afterwards. I
> > found out by experimentation that it was failing the first time because
> > the CID and STATUS were not reseted before sending the command and
> > getting the cid and status back. Thus I added it in the safest place
> > (where it would be executed only if the command was really be going to
> > be send) .
>
> If it is only needed for the wakeup, perhaps it is only needed to reset the MAILBOX_STATUS
> and MAILBOX_CID in set_state() only?
>
> > >> for (i = 0; i < REGISTER_BUSY_COUNT; i++) {
> > >> rt2800pci_bbp_read(rt2x00dev, 0, &value);
> > >> if ((value != 0xff) && (value != 0x00))
> > >> @@ -1956,8 +1950,10 @@ static void rt2800pci_toggle_irq(struct rt2x00_dev *rt2x00dev,
> > >> rt2x00_set_field32(®, INT_MASK_CSR_AC1_DMA_DONE, mask);
> > >> rt2x00_set_field32(®, INT_MASK_CSR_AC2_DMA_DONE, mask);
> > >> rt2x00_set_field32(®, INT_MASK_CSR_AC3_DMA_DONE, mask);
> > >> - rt2x00_set_field32(®, INT_MASK_CSR_HCCA_DMA_DONE, mask);
> > >> - rt2x00_set_field32(®, INT_MASK_CSR_MGMT_DMA_DONE, mask);
> > >> + if (state != STATE_RADIO_IRQ_ON) {
> > >> + rt2x00_set_field32(®, INT_MASK_CSR_HCCA_DMA_DONE, mask);
> > >> + rt2x00_set_field32(®, INT_MASK_CSR_MGMT_DMA_DONE, mask);
> > >> + }
> > >>
> > >
> > > This looks odd, doesn't this mean we should write (!mask) instead of the if-statement?
> > > And it might need a comment about why these 2 are special.
> > >
> > >
> > This one came from reading both the legacy driver and the
> > mac80211/rt2x00 one . I came to the conclusion that we don't create
> > queues for HCCA and MGMT opposed to what the legacy driver does.
> > I only prevent us from activating those . With this we can reload the
> > rt2800pci or even load the legacy driver after unloading the rt2800pci
> > one. Otherwise instant freeze happens which I feel is related to us
> > enabling those interrupts for queues that do not exists.
> > !mask may freeze the box when mask is 0 at toggle irq off. This needs
> > confirmation.
>
> Ok, so that suggests we always need to write 0 to it. That sounds quite logical,
> so that would be better then the if-statement and writing the mask to it. :)
>
> > >> @@ -2075,6 +2071,7 @@ static int rt2800pci_set_state(struct rt2x00_dev *rt2x00dev,
> > >> rt2x00pci_register_write(rt2x00dev, AUTOWAKEUP_CFG, 0);
> > >>
> > >> if (state == STATE_AWAKE) {
> > >> + rt2800pci_mcu_request(rt2x00dev, MCU_SLEEP, 0xff, 0xff, 0x02);
> > >> rt2800pci_mcu_request(rt2x00dev, MCU_WAKEUP, TOKEN_WAKUP, 0, 0);
> > >> rt2800pci_mcu_status(rt2x00dev, TOKEN_WAKUP);
> > >> } else
> > >>
> > >
> > > Not sure, Ralink hinted that calling rt2800pci_set_state(rt2x00dev, STATE_SLEEP); should
> > > be called from set_device_state() instead. But they were going to test this, and I haven't
> > > heard about the results yet.
> > >
> > >
> > This one is from the legacy driver cut'n paste . I added it because it
> > was there:
> > NICInitAsicFromEEPROM when they call 0x31 (MCU_WAKEUP) they call
> > MCU_SLEEP beforehand with 0xff, 0xff, 0x02 instead of 0xff, 0x00, 0x02.
> > This one is not needed I guess. I have not seen change in the behaviour
> > with or without it. Seems to me this is a safeguard in the legacy driver
> > .. Though I found a lot of dead code in the leagacy driver . Could well
> > be that even any of the commands sent to the mcu or firmware where not
> > really needed after all. Let s forget about this one.
>
> Well the only difference between your suggestion and Ralinks suggestion would result
> in only the extra call to:
> rt2x00pci_register_write(rt2x00dev, AUTOWAKEUP_CFG, 0)
> So I think your version is better since we indeed shouldn't need to reset the AUTOWAKEUP_CFG
> twice.
>
> > >> @@ -2087,6 +2084,8 @@ static int rt2800pci_set_device_state(struct rt2x00_dev *rt2x00dev,
> > >> enum dev_state state)
> > >> {
> > >> int retval = 0;
> > >> + int i =0;
> > >> + u32 reg;
> > >>
> > >> switch (state) {
> > >> case STATE_RADIO_ON:
> > >> @@ -2095,7 +2094,26 @@ static int rt2800pci_set_device_state(struct rt2x00_dev *rt2x00dev,
> > >> * to be woken up. After that it needs a bit of time
> > >> * to be fully awake and the radio can be enabled.
> > >> */
> > >> + rt2x00pci_register_write(rt2x00dev, H2M_BBP_AGENT, 0);
> > >> + rt2x00pci_register_write(rt2x00dev, H2M_MAILBOX_CSR, 0);
> > >> + /*
> > >> + * Wait for device to stabilize.
> > >> + */
> > >> + for (i = 0; i < REGISTER_BUSY_COUNT; i++) {
> > >> + rt2x00pci_register_read(rt2x00dev, PBF_SYS_CTRL, ®);
> > >> + if (rt2x00_get_field32(reg, PBF_SYS_CTRL_READY))
> > >> + break;
> > >> + msleep(1);
> > >> + }
> > >> +
> > >> + if (i == REGISTER_BUSY_COUNT) {
> > >> + ERROR(rt2x00dev, "PBF system register not ready.\n");
> > >> + return -EBUSY;
> > >> + }
> > >> +
> > >> + rt2x00dev->ops->lib->set_device_state(rt2x00dev, STATE_RADIO_IRQ_OFF);
> > >>
> > >
> > > This entire batch also happens during firmware loading. Is it required that it happens twice,
> > > must it be done after firmware loading (as it is now) or should it only be done when enabling
> > > the radio?
> > >
> > >
> >
> > This is the critical part to get the mcu status working at boot time and
> > before the full initialization happened.
> > Here I do it only to reinit everything before sending the MCU_WAKEUP and
> > reading the cid and status .
> > This need to be done just before the MCU_WAKEUP is called for the first
> > time at boot time (if before radio enable registry initialization).
> > I have not thought about trying to remove it from after the firmware
> > load beforehand.
>
> Could you please test that?
> Otherwise if both blocks are needed, it might need to be put into a seperate function,
> (at least the PBF_SYS_CTRL_READY loop).
>
> > >> rt2800pci_set_state(rt2x00dev, STATE_AWAKE);
> > >> + rt2x00dev->ops->lib->set_device_state(rt2x00dev, STATE_RADIO_IRQ_ON);
> > >>
> > >
> > > Is this a ordering thing? The set_device_state(rt2x00dev, STATE_RADIO_IRQ_ON); will
> > > be called by rt2x00dev after the radio has been enabled.
> > >
> > >
> > No, short sight of mine. I enable it back as soon as possible. I don't
> > remember testing without this irq toggles which looks not needed.
>
> Ok. :)
>
> Ivo
>
> _______________________________________________
> users mailing list
> users at rt2x00.serialmonkey.com
> http://rt2x00.serialmonkey.com/mailman/listinfo/users_rt2x00.serialmonkey.com
_________________________________________________________________
Messenger 2009: Instale já!
http://download.live.com
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://rt2x00.serialmonkey.com/pipermail/users_rt2x00.serialmonkey.com/attachments/20090419/8986edcd/attachment.html>
More information about the users
mailing list