From: Doug Anderson <dianders@chromium.org>
To: Rakesh Pillai <pillair@codeaurora.org>
Cc: Abhishek Kumar <kuabhs@chromium.org>,
Kalle Valo <kvalo@codeaurora.org>,
LKML <linux-kernel@vger.kernel.org>,
ath10k <ath10k@lists.infradead.org>,
Brian Norris <briannorris@chromium.org>,
linux-wireless <linux-wireless@vger.kernel.org>,
"David S. Miller" <davem@davemloft.net>,
Jakub Kicinski <kuba@kernel.org>, netdev <netdev@vger.kernel.org>
Subject: Re: [PATCH v2 1/1] ath10k: add option for chip-id based BDF selection
Date: Tue, 24 Nov 2020 08:26:14 -0800 [thread overview]
Message-ID: <CAD=FV=UnecON-M9eZVQePuNpdygN_E9OtLN495Xe1GL_PA94DQ@mail.gmail.com> (raw)
In-Reply-To: <002401d6c242$d78f2140$86ad63c0$@codeaurora.org>
Hi,
On Tue, Nov 24, 2020 at 1:19 AM Rakesh Pillai <pillair@codeaurora.org> wrote:
>
> > -----Original Message-----
> > From: Doug Anderson <dianders@chromium.org>
> > Sent: Tuesday, November 24, 2020 6:27 AM
> > To: Abhishek Kumar <kuabhs@chromium.org>
> > Cc: Kalle Valo <kvalo@codeaurora.org>; Rakesh Pillai
> > <pillair@codeaurora.org>; LKML <linux-kernel@vger.kernel.org>; ath10k
> > <ath10k@lists.infradead.org>; Brian Norris <briannorris@chromium.org>;
> > linux-wireless <linux-wireless@vger.kernel.org>; David S. Miller
> > <davem@davemloft.net>; Jakub Kicinski <kuba@kernel.org>; netdev
> > <netdev@vger.kernel.org>
> > Subject: Re: [PATCH v2 1/1] ath10k: add option for chip-id based BDF
> > selection
> >
> > Hi,
> >
> > On Thu, Nov 12, 2020 at 12:09 PM Abhishek Kumar <kuabhs@chromium.org>
> > wrote:
> > >
> > > In some devices difference in chip-id should be enough to pick
> > > the right BDF. Add another support for chip-id based BDF selection.
> > > With this new option, ath10k supports 2 fallback options.
> > >
> > > The board name with chip-id as option looks as follows
> > > board name 'bus=snoc,qmi-board-id=ff,qmi-chip-id=320'
> > >
> > > Tested-on: WCN3990 hw1.0 SNOC WLAN.HL.3.2.2-00696-QCAHLSWMTPL-1
> > > Tested-on: QCA6174 HW3.2 WLAN.RM.4.4.1-00157-QCARMSWPZ-1
> > > Signed-off-by: Abhishek Kumar <kuabhs@chromium.org>
> > > ---
> > >
> > > (no changes since v1)
> >
> > I think you need to work on the method you're using to generate your
> > patches. There are most definitely changes since v1. You described
> > them in your cover letter (which you don't really need for a singleton
> > patch) instead of here.
> >
> >
> > > @@ -1438,12 +1439,17 @@ static int
> > ath10k_core_create_board_name(struct ath10k *ar, char *name,
> > > }
> > >
> > > if (ar->id.qmi_ids_valid) {
> > > - if (with_variant && ar->id.bdf_ext[0] != '\0')
> > > + if (with_additional_params && ar->id.bdf_ext[0] != '\0')
> > > scnprintf(name, name_len,
> > > "bus=%s,qmi-board-id=%x,qmi-chip-id=%x%s",
> > > ath10k_bus_str(ar->hif.bus),
> > > ar->id.qmi_board_id, ar->id.qmi_chip_id,
> > > variant);
> > > + else if (with_additional_params)
> > > + scnprintf(name, name_len,
> > > + "bus=%s,qmi-board-id=%x,qmi-chip-id=%x",
> > > + ath10k_bus_str(ar->hif.bus),
> > > + ar->id.qmi_board_id, ar->id.qmi_chip_id);
> >
> > I believe this is exactly opposite of what Rakesh was requesting.
> > Specifically, he was trying to eliminate the extra scnprintf() but I
> > think he still agreed that it was a good idea to generate 3 different
> > strings. I believe the proper diff to apply to v1 is:
> >
> > https://crrev.com/c/255643
Wow, I seem to have deleted the last digit from my URL. Should have been:
https://crrev.com/c/2556437
> >
> > -Doug
>
> Hi Abhishek/Doug,
>
> I missed on reviewing this change. Also I agree with Doug that this is not the change I was looking for.
>
> The argument "with_variant" can be renamed to "with_extra_params". There is no need for any new argument to this function.
> Case 1: with_extra_params=0, ar->id.bdf_ext[0] = 0 -> The default name will be used (bus=snoc,qmi_board_id=0xab)
> Case 2: with_extra_params=1, ar->id.bdf_ext[0] = 0 -> bus=snoc,qmi_board_id=0xab,qmi_chip_id=0xcd
> Case 3: with_extra_params=1, ar->id.bdf_ext[0] = "xyz" -> bus=snoc,qmi_board_id=0xab,qmi_chip_id=0xcd,variant=xyz
>
> ar->id.bdf_ext[0] depends on the DT entry for variant field.
I'm confused about your suggestion. Maybe you can help clarify. Are
you suggesting:
a) Only two calls to ath10k_core_create_board_name()
I'm pretty sure this will fail in some cases. Specifically consider
the case where the device tree has a "variant" defined but the BRD
file only has one entry for (board-id) and one for (board-id +
chip-id) but no entry for (board-id + chip-id + variant). If you are
only making two calls then I don't think you'll pick the right one.
Said another way...
If the device tree has a variant:
1. We should prefer a BRD entry that has board-id + chip-id + variant
2. If #1 isn't there, we should prefer a BRD entry that has board-id + chip-id
3. If #1 and #2 aren't there we fall back to a BRD entry that has board-id.
...without 3 calls to ath10k_core_create_board_name() we can't handle
all 3 cases.
b) Three calls to ath10k_core_create_board_name() but the caller
manually whacks "ar->id.bdf_ext[0]" for one of the calls
This doesn't look like it's a clean solution, but maybe I'm missing something.
-Doug
next prev parent reply other threads:[~2020-11-24 16:27 UTC|newest]
Thread overview: 10+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-11-12 20:09 [PATCH v2 0/1] This patch address comments on patch v1 Abhishek Kumar
2020-11-12 20:09 ` [PATCH v2 1/1] ath10k: add option for chip-id based BDF selection Abhishek Kumar
2020-11-24 0:56 ` Doug Anderson
2020-11-24 9:18 ` Rakesh Pillai
2020-11-24 16:26 ` Doug Anderson [this message]
2020-11-25 3:44 ` Rakesh Pillai
2020-11-30 19:18 ` Doug Anderson
2020-12-03 11:33 ` Rakesh Pillai
2020-12-03 15:33 ` Doug Anderson
2020-12-07 18:14 ` Abhishek Kumar
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to='CAD=FV=UnecON-M9eZVQePuNpdygN_E9OtLN495Xe1GL_PA94DQ@mail.gmail.com' \
--to=dianders@chromium.org \
--cc=ath10k@lists.infradead.org \
--cc=briannorris@chromium.org \
--cc=davem@davemloft.net \
--cc=kuabhs@chromium.org \
--cc=kuba@kernel.org \
--cc=kvalo@codeaurora.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-wireless@vger.kernel.org \
--cc=netdev@vger.kernel.org \
--cc=pillair@codeaurora.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).