Received-SPF: pass (google.com: domain of linux-wireless-owner@vger.kernel.org designates 2620:137:e000::3:4 as permitted sender) client-ip=2620:137:e000::3:4;
MIME-Version: 1.0
References: <cover.1697650207.git.dberlin@dberlin.org> <0b95944fcf047b3ec83cecb0c65ca24de43810fd.1697650207.git.dberlin@dberlin.org>
 <d55c968a-d953-67d7-cdff-f856422607f5@broadcom.com>
In-Reply-To: <d55c968a-d953-67d7-cdff-f856422607f5@broadcom.com>
From:   Daniel Berlin <dberlin@dberlin.org>
Date:   Fri, 20 Oct 2023 12:36:01 -0400
Message-ID: <CAF4BwTWDAV0_iecq3Z7iOxe9UejYkYpDrfMqBXA=hq_ouEePKw@mail.gmail.com>
Subject: Re: [PATCH 1/5] [brcmfmac] Add support for encoding/decoding 6g chanspecs
To:     Arend van Spriel <arend.vanspriel@broadcom.com>
Cc:     Arend van Spriel <aspriel@gmail.com>,
        Franky Lin <franky.lin@broadcom.com>,
        Hante Meuleman <hante.meuleman@broadcom.com>,
        linux-wireless@vger.kernel.org,
        brcm80211-dev-list.pdl@broadcom.com,
        SHA-cyfmac-dev-list@infineon.com, linux-kernel@vger.kernel.org
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Precedence: bulk

I'll remove the extra prints, and the 320MHZ.

On Fri, Oct 20, 2023 at 5:57=E2=80=AFAM Arend van Spriel
<arend.vanspriel@broadcom.com> wrote:
>
> On 10/19/2023 3:42 AM, Daniel Berlin wrote:
> > This patch adds support for 6G chanspecs, as part of adding 6G and
> > 802.11ax support.
> >
> > I added the correct values for the upcoming 320mhz as well so that the
> > info is complete.
>
> Reviewed-by: Arend van Spriel <arend.vanspriel@broadcom.com>>
> Signed-off-by: Daniel Berlin <dberlin@dberlin.org>
> > ---
> >   .../broadcom/brcm80211/brcmutil/d11.c         | 46 +++++++++++++++---=
-
> >   .../broadcom/brcm80211/include/brcmu_d11.h    | 46 +++++++++++++-----=
-
> >   .../broadcom/brcm80211/include/brcmu_wifi.h   | 27 ++++++++---
> >   3 files changed, 89 insertions(+), 30 deletions(-)
> >
> > diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmutil/d11.c b/d=
rivers/net/wireless/broadcom/brcm80211/brcmutil/d11.c
> > index 1e2b1e487eb7..faf7eeeeb2d5 100644
> > --- a/drivers/net/wireless/broadcom/brcm80211/brcmutil/d11.c
> > +++ b/drivers/net/wireless/broadcom/brcm80211/brcmutil/d11.c
>
> [...]
>
> > @@ -117,7 +127,9 @@ static void brcmu_d11n_decchspec(struct brcmu_chan =
*ch)
> >               }
> >               break;
> >       default:
> > -             WARN_ONCE(1, "Invalid chanspec 0x%04x\n", ch->chspec);
> > +             WARN_ONCE(1,
> > +                       "Invalid chanspec - unknown 11n bandwidth 0x%04=
x\n",
> > +                       ch->chspec);
>
> I don't think this provides more info. The WARN_ONCE() already prints
> code location and stack trace.
>
> >               break;
> >       }
> >
> > @@ -129,7 +141,8 @@ static void brcmu_d11n_decchspec(struct brcmu_chan =
*ch)
> >               ch->band =3D BRCMU_CHAN_BAND_2G;
> >               break;
> >       default:
> > -             WARN_ONCE(1, "Invalid chanspec 0x%04x\n", ch->chspec);
> > +             WARN_ONCE(1, "Invalid chanspec - unknown 11n band 0x%04x\=
n",
> > +                       ch->chspec);
>
> ditto
>
> >               break;
> >       }
> >   }
> > @@ -156,7 +169,9 @@ static void brcmu_d11ac_decchspec(struct brcmu_chan=
 *ch)
> >                       ch->sb =3D BRCMU_CHAN_SB_U;
> >                       ch->control_ch_num +=3D CH_10MHZ_APART;
> >               } else {
> > -                     WARN_ONCE(1, "Invalid chanspec 0x%04x\n", ch->chs=
pec);
> > +                     WARN_ONCE(1,
> > +                               "Invalid chanspec - unknown 11ac channe=
l distance 0x%04x\n",
> > +                               ch->chspec);
>
> ditto
>
> >               }
> >               break;
> >       case BRCMU_CHSPEC_D11AC_BW_80:
> > @@ -177,7 +192,9 @@ static void brcmu_d11ac_decchspec(struct brcmu_chan=
 *ch)
> >                       ch->control_ch_num +=3D CH_30MHZ_APART;
> >                       break;
> >               default:
> > -                     WARN_ONCE(1, "Invalid chanspec 0x%04x\n", ch->chs=
pec);
> > +                     WARN_ONCE(1,
> > +                               "Invalid chanspec - unknown 11ac channe=
l distance 0x%04x\n",
> > +                               ch->chspec);
>
> ditto
>
> >                       break;
> >               }
> >               break;
> > @@ -211,17 +228,24 @@ static void brcmu_d11ac_decchspec(struct brcmu_ch=
an *ch)
> >                       ch->control_ch_num +=3D CH_70MHZ_APART;
> >                       break;
> >               default:
> > -                     WARN_ONCE(1, "Invalid chanspec 0x%04x\n", ch->chs=
pec);
> > +                     WARN_ONCE(1,
> > +                               "Invalid chanspec - unknown 11ac channe=
l distance 0x%04x\n",
> > +                               ch->chspec);
>
> ditto
>
> >                       break;
> >               }
> >               break;
> >       case BRCMU_CHSPEC_D11AC_BW_8080:
> >       default:
> > -             WARN_ONCE(1, "Invalid chanspec 0x%04x\n", ch->chspec);
> > +             WARN_ONCE(1,
> > +                       "Invalid chanspec - unknown 11ac channel bandwi=
dth 0x%04x\n",
> > +                       ch->chspec);
> >               break;
> >       }
> >
> >       switch (ch->chspec & BRCMU_CHSPEC_D11AC_BND_MASK) {
> > +     case BRCMU_CHSPEC_D11AC_BND_6G:
> > +             ch->band =3D BRCMU_CHAN_BAND_6G;
> > +             break;
> >       case BRCMU_CHSPEC_D11AC_BND_5G:
> >               ch->band =3D BRCMU_CHAN_BAND_5G;
> >               break;
>
> [...]
>
> > diff --git a/drivers/net/wireless/broadcom/brcm80211/include/brcmu_d11.=
h b/drivers/net/wireless/broadcom/brcm80211/include/brcmu_d11.h
> > index f6344023855c..bb48b7442062 100644
> > --- a/drivers/net/wireless/broadcom/brcm80211/include/brcmu_d11.h
> > +++ b/drivers/net/wireless/broadcom/brcm80211/include/brcmu_d11.h
> > @@ -69,24 +69,44 @@
> >   #define  BRCMU_CHSPEC_D11AC_SB_UU   BRCMU_CHSPEC_D11AC_SB_LUU
> >   #define  BRCMU_CHSPEC_D11AC_SB_L    BRCMU_CHSPEC_D11AC_SB_LLL
> >   #define  BRCMU_CHSPEC_D11AC_SB_U    BRCMU_CHSPEC_D11AC_SB_LLU
> > +/* channel sideband indication for frequency >=3D 240MHz */
> > +#define BRCMU_CHSPEC_D11AC_320_SB_MASK       0x0780
> > +#define BRCMU_CHSPEC_D11AC_320_SB_SHIFT      7
> > +#define BRCMU_CHSPEC_D11AC_SB_LLLL   0x0000
> > +#define BRCMU_CHSPEC_D11AC_SB_LLLU   0x0080
> > +#define BRCMU_CHSPEC_D11AC_SB_LLUL   0x0100
> > +#define BRCMU_CHSPEC_D11AC_SB_LLUU   0x0180
> > +#define BRCMU_CHSPEC_D11AC_SB_LULL   0x0200
> > +#define BRCMU_CHSPEC_D11AC_SB_LULU   0x0280
> > +#define BRCMU_CHSPEC_D11AC_SB_LUUL   0x0300
> > +#define BRCMU_CHSPEC_D11AC_SB_LUUU   0x0380
> > +#define BRCMU_CHSPEC_D11AC_SB_ULLL   0x0400
> > +#define BRCMU_CHSPEC_D11AC_SB_ULLU   0x0480
> > +#define BRCMU_CHSPEC_D11AC_SB_ULUL   0x0500
> > +#define BRCMU_CHSPEC_D11AC_SB_ULUU   0x0580
> > +#define BRCMU_CHSPEC_D11AC_SB_UULL   0x0600
> > +#define BRCMU_CHSPEC_D11AC_SB_UULU   0x0680
> > +#define BRCMU_CHSPEC_D11AC_SB_UUUL   0x0700
> > +#define BRCMU_CHSPEC_D11AC_SB_UUUU   0x0780
>
> These are WCC specific, but I think it is okay to have these definitions
> here.
>
> >   #define BRCMU_CHSPEC_D11AC_BW_MASK  0x3800
> >   #define BRCMU_CHSPEC_D11AC_BW_SHIFT 11
> > -#define  BRCMU_CHSPEC_D11AC_BW_5     0x0000
> > -#define  BRCMU_CHSPEC_D11AC_BW_10    0x0800
> > -#define  BRCMU_CHSPEC_D11AC_BW_20    0x1000
> > -#define  BRCMU_CHSPEC_D11AC_BW_40    0x1800
> > -#define  BRCMU_CHSPEC_D11AC_BW_80    0x2000
> > -#define  BRCMU_CHSPEC_D11AC_BW_160   0x2800
> > -#define  BRCMU_CHSPEC_D11AC_BW_8080  0x3000
> > -#define BRCMU_CHSPEC_D11AC_BND_MASK  0xc000
> > -#define BRCMU_CHSPEC_D11AC_BND_SHIFT 14
> > -#define  BRCMU_CHSPEC_D11AC_BND_2G   0x0000
> > -#define  BRCMU_CHSPEC_D11AC_BND_3G   0x4000
> > -#define  BRCMU_CHSPEC_D11AC_BND_4G   0x8000
> > -#define  BRCMU_CHSPEC_D11AC_BND_5G   0xc000
> > +#define BRCMU_CHSPEC_D11AC_BW_10    0x0800
> > +#define BRCMU_CHSPEC_D11AC_BW_20    0x1000
> > +#define BRCMU_CHSPEC_D11AC_BW_40    0x1800
> > +#define BRCMU_CHSPEC_D11AC_BW_80    0x2000
> > +#define BRCMU_CHSPEC_D11AC_BW_160   0x2800
> > +#define BRCMU_CHSPEC_D11AC_BW_320   0x0000
>
> 320MHz is 802.11be. No need to add this already, but not a biggy.
>
> > +#define BRCMU_CHSPEC_D11AC_BW_8080  0x3000
> > +#define BRCMU_CHSPEC_D11AC_BND_MASK 0xc000
> > +#define BRCMU_CHSPEC_D11AC_BND_SHIFT 14
> > +#define BRCMU_CHSPEC_D11AC_BND_2G   0x0000
> > +#define BRCMU_CHSPEC_D11AC_BND_4G   0x8000
> > +#define BRCMU_CHSPEC_D11AC_BND_5G   0xc000
> > +#define BRCMU_CHSPEC_D11AC_BND_6G   0x4000
> >
> >   #define BRCMU_CHAN_BAND_2G          0
> >   #define BRCMU_CHAN_BAND_5G          1
> > +#define BRCMU_CHAN_BAND_6G           2
> >
> >   enum brcmu_chan_bw {
> >       BRCMU_CHAN_BW_20,
>