Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
MIME-Version: 1.0
References: <20220107200417.1.Ie4dcc45b0bf365077303c596891d460d716bb4c5@changeid>
 <CAD=FV=W5fHP8K-PcoYWxYHiDWnPUVQQzOzw=REbuJSSqGeVVfg@mail.gmail.com>
 <87sfrqqfzy.fsf@kernel.org> <CAD=FV=U0Qw-OnKJg8SWk9=e=B0qgqnaTHpuR0cRA0WCmSHSJYQ@mail.gmail.com>
In-Reply-To: <CAD=FV=U0Qw-OnKJg8SWk9=e=B0qgqnaTHpuR0cRA0WCmSHSJYQ@mail.gmail.com>
From:   Abhishek Kumar <kuabhs@chromium.org>
Date:   Mon, 11 Apr 2022 16:25:10 -0700
Message-ID: <CACTWRwtpYBokTehRE0_zSdSjio6Ga1yqdCfj1TNck7SqOT8o_Q@mail.gmail.com>
Subject: Re: [PATCH 1/2] ath10k: search for default BDF name provided in DT
To:     Doug Anderson <dianders@chromium.org>
Cc:     Kalle Valo <kvalo@kernel.org>,
        Rakesh Pillai <pillair@codeaurora.org>,
        LKML <linux-kernel@vger.kernel.org>,
        linux-wireless <linux-wireless@vger.kernel.org>,
        "David S. Miller" <davem@davemloft.net>,
        Jakub Kicinski <kuba@kernel.org>,
        ath10k <ath10k@lists.infradead.org>,
        netdev <netdev@vger.kernel.org>
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

Hi All,

Apologies for the late reply, too many things at the same time. Just a
quick note, Qcomm provided a new BDF file with support for
'bus=snoc,qmi-board-id=ff' as well, so even without this patch, the
non-programmed devices(with board-id=0xff) would work. However, for
optimization of the BDF search strategy, the above mentioned strategy
would still not work:
- The stripping of full Board name would not work if board-id itself
is not programmed i.e. =0xff
e.g. for
 'bus=snoc,qmi-board-id=ff,qmi-chip-id=320,variant=GO_LAZOR' => no match
 'bus=snoc,qmi-board-id=ff,qmi-chip-id=320' => no match
 'bus=snoc,qmi-board-id=ff' => no match
 'bus=snoc' => no match
because all the BDFs contains board-id=67

So with this DT patch specifically for case 'bus=snoc,qmi-board-id=ff'
=> no match, we fallback to default case ( the one provided in DT)
i.e. 'bus=snoc,qmi-board-id=67' => match . I do not see how exactly
the driver can know that it should check for a board-id of 67.

However, to still remove dependency on the DT, we can make the
board-id as no-op if it is not programmed i.e. if the board-id=ff then
we would pick any BDF that match 'bus=snoc,qmi-board-id=<XX>' where XX
can be any arbitrary number. Thoughts ?

-Abhishek

On Thu, Mar 10, 2022 at 4:28 PM Doug Anderson <dianders@chromium.org> wrote:
>
> Hi,
>
> On Thu, Mar 10, 2022 at 2:06 AM Kalle Valo <kvalo@kernel.org> wrote:
> >
> > Doug Anderson <dianders@chromium.org> writes:
> >
> > > Hi,
> > >
> > > On Fri, Jan 7, 2022 at 12:05 PM Abhishek Kumar <kuabhs@chromium.org> wrote:
> > >>
> > >> There can be cases where the board-2.bin does not contain
> > >> any BDF matching the chip-id+board-id+variant combination.
> > >> This causes the wlan probe to fail and renders wifi unusable.
> > >> For e.g. if the board-2.bin has default BDF as:
> > >> bus=snoc,qmi-board-id=67 but for some reason the board-id
> > >> on the wlan chip is not programmed and read 0xff as the
> > >> default value. In such cases there won't be any matching BDF
> > >> because the board-2.bin will be searched with following:
> > >> bus=snoc,qmi-board-id=ff
> >
> > I just checked, in ath10k-firmware WCN3990/hw1.0/board-2.bin we have
> > that entry:
> >
> > BoardNames[1]: 'bus=snoc,qmi-board-id=ff'
> >
> > >> To address these scenarios, there can be an option to provide
> > >> fallback default BDF name in the device tree. If none of the
> > >> BDF names match then the board-2.bin file can be searched with
> > >> default BDF names assigned in the device tree.
> > >>
> > >> The default BDF name can be set as:
> > >> wifi@a000000 {
> > >>         status = "okay";
> > >>         qcom,ath10k-default-bdf = "bus=snoc,qmi-board-id=67";
> > >
> > > Rather than add a new device tree property, wouldn't it be good enough
> > > to leverage the existing variant?
> >
> > I'm not thrilled either adding this to Device Tree, we should keep the
> > bindings as simple as possible.
> >
> > > Right now I think that the board file contains:
> > >
> > > 'bus=snoc,qmi-board-id=67.bin'
> > > 'bus=snoc,qmi-board-id=67,qmi-chip-id=320,variant=GO_LAZOR.bin'
> > > 'bus=snoc,qmi-board-id=67,qmi-chip-id=320,variant=GO_POMPOM.bin'
> > > 'bus=snoc,qmi-board-id=67,qmi-chip-id=4320,variant=GO_LAZOR.bin'
> > > 'bus=snoc,qmi-board-id=67,qmi-chip-id=4320,variant=GO_POMPOM.bin'
> > >
> > > ...and, on lazor for instance, we have:
> > >
> > > qcom,ath10k-calibration-variant = "GO_LAZOR";
> > >
> > > The problem you're trying to solve is that on some early lazor
> > > prototype hardware we didn't have the "board-id" programmed we could
> > > get back 0xff from the hardware. As I understand it 0xff always means
> > > "unprogrammed".
> > >
> > > It feels like you could just have a special case such that if the
> > > hardware reports board ID of 0xff and you don't get a "match" that you
> > > could just treat 0xff as a wildcard. That means that you'd see the
> > > "variant" in the device tree and pick one of the "GO_LAZOR" entries.
> > >
> > > Anyway, I guess it's up to the people who spend more time in this file
> > > which they'd prefer, but that seems like it'd be simple and wouldn't
> > > require a bindings addition...
> >
> > In ath11k we need something similar for that I have been thinking like
> > this:
> >
> > 'bus=snoc,qmi-board-id=67,qmi-chip-id=320,variant=GO_LAZOR'
> >
> > 'bus=snoc,qmi-board-id=67,qmi-chip-id=320'
> >
> > 'bus=snoc,qmi-board-id=67'
> >
> > 'bus=snoc'
> >
> > Ie. we drop one attribute at a time and try to find a suitable board
> > file. Though I'm not sure if it's possible to find a board file which
> > works with many different hardware, but the principle would be at least
> > that. Would something like that work in your case?
>
> I'll leave it for Abhishek to comment for sure since he's been the one
> actively involved in keeping track of our board-2.bin file. As far as
> I know:
>
> * Mostly we need this for pre-production hardware that developers
> inside Google and Qualcomm still have sitting around on their desks. I
> guess some people are even still using this pre-production hardware to
> surf the web?
>
> * In the ideal world, we think that the best calibration would be to
> use the board-specific one in these cases. AKA if board_id is 0xff
> (unprogrammed) and variant is "GO_LAZOR" then the best solution would
> be to use the settings for board id 0x67 and variant "GO_LAZOR". This
> _ought_ to be better settings than the default 0xff settings without
> the "GO_LAZOR".
>
> * In reality, we're probably OK as long as _some_ reasonable settings
> get picked. WiFi might not be super range optimized but at least it
> will still come up and work.