RE: [PATCH rtw-next v7 3/3] wifi: rtw88: time the countdown downloads from the TBTT

[Ping-Ke Shih]

Mehmet Fide <mehmet.fide@xxxxxxxxx> wrote:
+/* the beaconing interface is always on port 0 */

Move into function? Or just mention this in commit message.

+static u64 rtw_fw_read_tsf(struct rtw_dev *rtwdev)
+{
+       u32 hi, lo;
+
+       do {
+               hi = rtw_read32(rtwdev, REG_TSFTR + 4);
+               lo = rtw_read32(rtwdev, REG_TSFTR);
+       } while (hi != rtw_read32(rtwdev, REG_TSFTR + 4));

How about setting a retry limit? Consider if hardware goes wrong
suddenly returning random number, I'm afraid it becomes infinite
loop. As my thought, at most it needs one time retry.

+
+       return ((u64)hi << 32) | lo;
+}
+
>  static void rtw_fw_csa_queue(struct rtw_dev *rtwdev, struct ieee80211_vif *vif)
>  {
>         u32 interval = ieee80211_tu_to_usec(vif->bss_conf.beacon_int);
> +       u64 tsf = rtw_fw_read_tsf(rtwdev);
>         s64 delay;
> +       u32 rem;
> 
> -       rtwdev->csa_due = ktime_add_us(rtwdev->csa_due, interval);
> -       delay = ktime_us_delta(rtwdev->csa_due, ktime_get());
> +       rtwdev->csa_tbtt += interval;
> +       delay = rtwdev->csa_tbtt + RTW_CSA_TBTT_MARGIN_US - tsf;
> +       if (delay > interval + RTW_CSA_TBTT_MARGIN_US ||
> +           delay < -(s64)interval) {
> +               /* the TSF jumped: restart from the next TBTT */
> +               div_u64_rem(tsf, interval, &rem);
> +               rtwdev->csa_tbtt = tsf - rem + interval;
> +               delay = rtwdev->csa_tbtt + RTW_CSA_TBTT_MARGIN_US - tsf;
> +       }

an empty line

>         ieee80211_queue_delayed_work(rtwdev->hw, &rtwdev->csa_beacon_work,
>                                      usecs_to_jiffies(max(delay, 0)));
>  }
> 
>  void rtw_fw_csa_start(struct rtw_dev *rtwdev, struct ieee80211_vif *vif)
>  {
> +       u32 interval = ieee80211_tu_to_usec(vif->bss_conf.beacon_int);
> +       u32 rem;
> +
>         lockdep_assert_held(&rtwdev->mutex);
> 
>         if (rtwdev->csa_vif == vif)
>                 return;
> 
>         rtwdev->csa_vif = vif;

an empty line

> -       rtwdev->csa_due = ktime_get();
> +       rtwdev->csa_tbtt = rtw_fw_read_tsf(rtwdev);
> +       div_u64_rem(rtwdev->csa_tbtt, interval, &rem);
> +       rtwdev->csa_tbtt -= rem;
> +       /* a TBTT that just passed may have sent the first count already */
> +       if (rem < RTW_CSA_TBTT_MARGIN_US)
> +               rtwdev->csa_tbtt -= interval;

an empty line

>         rtw_fw_csa_queue(rtwdev, vif);

The csa_tbtt is just read from register and adjusted to what you want,
but in rtw_fw_csa_queue() it reads again and adjust. Can you reduce it?
How about adding an argument to tell rtw_fw_csa_queue() that this is first
time to initialize csa_tbtt (like the restart case)?

>  }