diff mbox series

[v2,1/1] ath10k: add option for chip-id based BDF selection

Message ID 20201112200856.v2.1.Ia526132a366886e3b5cf72433d0d58bb7bb1be0f@changeid (mailing list archive)
State Awaiting Upstream
Delegated to: Netdev Maintainers
Headers show
Series [v2,1/1] ath10k: add option for chip-id based BDF selection | expand

Checks

Context Check Description
netdev/tree_selection success Not a local patch

Commit Message

Abhishek Kumar Nov. 12, 2020, 8:09 p.m. UTC
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)

 drivers/net/wireless/ath/ath10k/core.c | 12 +++++++++---
 1 file changed, 9 insertions(+), 3 deletions(-)

Comments

Doug Anderson Nov. 24, 2020, 12:56 a.m. UTC | #1
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

-Doug
Rakesh Pillai Nov. 24, 2020, 9:18 a.m. UTC | #2
> -----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
> 
> -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.

Thanks,
Rakesh Pillai.
Doug Anderson Nov. 24, 2020, 4:26 p.m. UTC | #3
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
Rakesh Pillai Nov. 25, 2020, 3:44 a.m. UTC | #4
> -----Original Message-----
> From: Doug Anderson <dianders@chromium.org>
> Sent: Tuesday, November 24, 2020 9:56 PM
> 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
> 
> 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.

This can be handled by two calls to ath10k_core_create_board_name
1) ath10k_core_create_board_name(ar, boardname, sizeof(boardname), true)   :  As per my suggestions, this can result in two possible board names
    a) If DT have the "variant" node, it outputs the #1 from your suggestion  (1. We should prefer a BRD entry that has board-id + chip-id + variant)
    b) If DT does not have the "variant" node, it outputs the #2 from your suggestion (2. If #1 isn't there, we should prefer a BRD entry that has board-id + chip-id)

2) ath10k_core_create_board_name(ar, boardname, sizeof(boardname), false)    :  This is the second call to this function and outputs the #3 from your suggestion (3. If #1 and #2 aren't there we fall back to a BRD entry that has board-id)


Below is the snippet of code change I am suggesting. 

 static int ath10k_core_create_board_name(struct ath10k *ar, char *name,
-                                        size_t name_len, bool with_variant)
+                                        size_t name_len, bool with_extra_params)
 {
        /* strlen(',variant=') + strlen(ar->id.bdf_ext) */
        char variant[9 + ATH10K_SMBIOS_BDF_EXT_STR_LENGTH] = { 0 };

-       if (with_variant && ar->id.bdf_ext[0] != '\0')
+       if (ar->id.bdf_ext[0] != '\0')
                scnprintf(variant, sizeof(variant), ",variant=%s",
                          ar->id.bdf_ext);

@@ -1493,7 +1493,7 @@ 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_extra_params)
                        scnprintf(name, name_len,
                                  "bus=%s,qmi-board-id=%x,qmi-chip-id=%x%s",
                                  ath10k_bus_str(ar->hif.bus),


Thanks,
Rakesh Pillai.

> 
> 
> 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
Doug Anderson Nov. 30, 2020, 7:18 p.m. UTC | #5
Hi,

On Tue, Nov 24, 2020 at 7:44 PM Rakesh Pillai <pillair@codeaurora.org> wrote:
>
> > > 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.
>
> This can be handled by two calls to ath10k_core_create_board_name
> 1) ath10k_core_create_board_name(ar, boardname, sizeof(boardname), true)   :  As per my suggestions, this can result in two possible board names
>     a) If DT have the "variant" node, it outputs the #1 from your suggestion  (1. We should prefer a BRD entry that has board-id + chip-id + variant)
>     b) If DT does not have the "variant" node, it outputs the #2 from your suggestion (2. If #1 isn't there, we should prefer a BRD entry that has board-id + chip-id)
>
> 2) ath10k_core_create_board_name(ar, boardname, sizeof(boardname), false)    :  This is the second call to this function and outputs the #3 from your suggestion (3. If #1 and #2 aren't there we fall back to a BRD entry that has board-id)

What I'm trying to say is this.  Imagine that:

a) the device tree has the "variant" property.

b) the BRD file has two entries, one for "board-id" (1) and one for
"board-id + chip-id" (2).  It doesn't have one for "board-id + chip-id
+ variant" (3).

With your suggestion we'll see the "variant" property in the device
tree.  That means we'll search for (1) and (3).  (3) isn't there, so
we'll pick (1).  ...but we really should have picked (2), right?

-Doug
Rakesh Pillai Dec. 3, 2020, 11:33 a.m. UTC | #6
> -----Original Message-----
> From: Doug Anderson <dianders@chromium.org>
> Sent: Tuesday, December 1, 2020 12:49 AM
> 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
> 
> Hi,
> 
> On Tue, Nov 24, 2020 at 7:44 PM Rakesh Pillai <pillair@codeaurora.org>
> wrote:
> >
> > > > 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.
> >
> > This can be handled by two calls to ath10k_core_create_board_name
> > 1) ath10k_core_create_board_name(ar, boardname, sizeof(boardname),
> true)   :  As per my suggestions, this can result in two possible board names
> >     a) If DT have the "variant" node, it outputs the #1 from your suggestion
> (1. We should prefer a BRD entry that has board-id + chip-id + variant)
> >     b) If DT does not have the "variant" node, it outputs the #2 from your
> suggestion (2. If #1 isn't there, we should prefer a BRD entry that has board-
> id + chip-id)
> >
> > 2) ath10k_core_create_board_name(ar, boardname, sizeof(boardname),
> false)    :  This is the second call to this function and outputs the #3 from your
> suggestion (3. If #1 and #2 aren't there we fall back to a BRD entry that has
> board-id)
> 
> What I'm trying to say is this.  Imagine that:
> 
> a) the device tree has the "variant" property.
> 
> b) the BRD file has two entries, one for "board-id" (1) and one for
> "board-id + chip-id" (2).  It doesn't have one for "board-id + chip-id
> + variant" (3).
> 
> With your suggestion we'll see the "variant" property in the device
> tree.  That means we'll search for (1) and (3).  (3) isn't there, so
> we'll pick (1).  ...but we really should have picked (2), right?


Do we expect board-2.bin to not be populated with the bdf with variant field (if its necessary ?)
Seems fine for me, if we have 2 fallback names if that is needed.

> 
> -Doug
Doug Anderson Dec. 3, 2020, 3:33 p.m. UTC | #7
Hi,

On Thu, Dec 3, 2020 at 3:33 AM Rakesh Pillai <pillair@codeaurora.org> wrote:
>
> > What I'm trying to say is this.  Imagine that:
> >
> > a) the device tree has the "variant" property.
> >
> > b) the BRD file has two entries, one for "board-id" (1) and one for
> > "board-id + chip-id" (2).  It doesn't have one for "board-id + chip-id
> > + variant" (3).
> >
> > With your suggestion we'll see the "variant" property in the device
> > tree.  That means we'll search for (1) and (3).  (3) isn't there, so
> > we'll pick (1).  ...but we really should have picked (2), right?
>
> Do we expect board-2.bin to not be populated with the bdf with variant field (if its necessary ?)

The whole fact that there is a fallback to begin with implies that
there can be a mismatch between the board-2.bin and the device tree
file.  Once we accept that there can be a mismatch, it seems good to
try all 3 fallbacks in order.


> Seems fine for me, if we have 2 fallback names if that is needed.

OK, sounds good.  So hopefully Abhishek can post a v3 based on what's
in <https://crrev.com/c/2556437> and you can confirm you're good with
it there?

-Doug
Abhishek Kumar Dec. 7, 2020, 6:14 p.m. UTC | #8
Hi,

> > (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.
I agree, this was not intentional, I will fix this in the upcoming patches.

On Thu, Dec 3, 2020 at 7:34 AM Doug Anderson <dianders@chromium.org> wrote:
>
> Hi,
>
> On Thu, Dec 3, 2020 at 3:33 AM Rakesh Pillai <pillair@codeaurora.org> wrote:
> >
> > > What I'm trying to say is this.  Imagine that:
> > >
> > > a) the device tree has the "variant" property.
> > >
> > > b) the BRD file has two entries, one for "board-id" (1) and one for
> > > "board-id + chip-id" (2).  It doesn't have one for "board-id + chip-id
> > > + variant" (3).
> > >
> > > With your suggestion we'll see the "variant" property in the device
> > > tree.  That means we'll search for (1) and (3).  (3) isn't there, so
> > > we'll pick (1).  ...but we really should have picked (2), right?
> >
> > Do we expect board-2.bin to not be populated with the bdf with variant field (if its necessary ?)
>
> The whole fact that there is a fallback to begin with implies that
> there can be a mismatch between the board-2.bin and the device tree
> file.  Once we accept that there can be a mismatch, it seems good to
> try all 3 fallbacks in order.
>
> > Seems fine for me, if we have 2 fallback names if that is needed.
> OK, sounds good.  So hopefully Abhishek can post a v3 based on what's
> in <https://crrev.com/c/2556437> and you can confirm you're good with
> it there?
I agree, with this patch there can be mismatch between what's provided
in the Board file and what required board name we are generating, so
three calls are needed. So in a sense, we want to keep the V1 patch
with fix to reuse the same BDF.

I am making V3 changes and will address and push that out.

Thanks
Abhishek
diff mbox series

Patch

diff --git a/drivers/net/wireless/ath/ath10k/core.c b/drivers/net/wireless/ath/ath10k/core.c
index d73ad60b571c..fa9e676b26d9 100644
--- a/drivers/net/wireless/ath/ath10k/core.c
+++ b/drivers/net/wireless/ath/ath10k/core.c
@@ -1419,12 +1419,13 @@  static int ath10k_core_fetch_board_data_api_n(struct ath10k *ar,
 }
 
 static int ath10k_core_create_board_name(struct ath10k *ar, char *name,
-					 size_t name_len, bool with_variant)
+					 size_t name_len,
+					 bool with_additional_params)
 {
 	/* strlen(',variant=') + strlen(ar->id.bdf_ext) */
 	char variant[9 + ATH10K_SMBIOS_BDF_EXT_STR_LENGTH] = { 0 };
 
-	if (with_variant && ar->id.bdf_ext[0] != '\0')
+	if (with_additional_params && ar->id.bdf_ext[0] != '\0')
 		scnprintf(variant, sizeof(variant), ",variant=%s",
 			  ar->id.bdf_ext);
 
@@ -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);
 		else
 			scnprintf(name, name_len,
 				  "bus=%s,qmi-board-id=%x",