[rt2x00-users] [PATCH] Rt2x00 : Fix the MCU request and status interaction .

[Ivo van Doorn]

Hi,

> This patch is about fixing pci mcu_status failing most of the time (at boot time all calls fails,
> afterwards the first one pass sometimes).
> May be of interest for usb too ...

Does this by any chance also enables this TX/RX operations of the device?

> This is done one way at boot time by stopping IRQ and setting the Host
> 2 MCU MAILBOX and BBP to 0 while setting the wake state.
> For later command (eg. config_ps) simply resetting the MCU CID and STATUS
> before each request does the job.

Just curious, are these changes based on experimentation or knowlegde from the legacy driver?

>      if (WAIT_FOR_MCU(rt2x00dev, &reg)) {
> +        rt2x00pci_register_write(rt2x00dev, H2M_MAILBOX_STATUS, ~0);
> +        rt2x00pci_register_write(rt2x00dev, H2M_MAILBOX_CID, ~0);
>          rt2x00_set_field32(&reg, H2M_MAILBOX_CSR_OWNER, 1);
>          rt2x00_set_field32(&reg, H2M_MAILBOX_CSR_CMD_TOKEN, token);
>          rt2x00_set_field32(&reg, H2M_MAILBOX_CSR_ARG0, arg0);

Can this upset calls to rt2800pci_mcu_status() when MCU is accessed in multiple threads?

>      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(&reg, INT_MASK_CSR_AC1_DMA_DONE, mask);
>      rt2x00_set_field32(&reg, INT_MASK_CSR_AC2_DMA_DONE, mask);
>      rt2x00_set_field32(&reg, INT_MASK_CSR_AC3_DMA_DONE, mask);
> -    rt2x00_set_field32(&reg, INT_MASK_CSR_HCCA_DMA_DONE, mask);
> -    rt2x00_set_field32(&reg, INT_MASK_CSR_MGMT_DMA_DONE, mask);
> +    if (state != STATE_RADIO_IRQ_ON) {
> +        rt2x00_set_field32(&reg, INT_MASK_CSR_HCCA_DMA_DONE, mask);
> +        rt2x00_set_field32(&reg, 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.

> @@ -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.

> @@ -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, &reg);
> +            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?

>          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.

>          msleep(1);
>          retval = rt2800pci_enable_radio(rt2x00dev);
>          break;

Thanks,

Ivo