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'
Signed-off-by: Abhishek Kumar <[email protected]>
---
drivers/net/wireless/ath/ath10k/core.c | 45 +++++++++++++++++++-------
1 file changed, 34 insertions(+), 11 deletions(-)
diff --git a/drivers/net/wireless/ath/ath10k/core.c b/drivers/net/wireless/ath/ath10k/core.c
index a3f15cc89a10..767bb9d6a197 100644
--- a/drivers/net/wireless/ath/ath10k/core.c
+++ b/drivers/net/wireless/ath/ath10k/core.c
@@ -1343,7 +1343,8 @@ static int ath10k_core_search_bd(struct ath10k *ar,
static int ath10k_core_fetch_board_data_api_n(struct ath10k *ar,
const char *boardname,
- const char *fallback_boardname,
+ const char *fallback_boardname1,
+ const char *fallback_boardname2,
const char *filename)
{
size_t len, magic_len;
@@ -1392,8 +1393,11 @@ static int ath10k_core_fetch_board_data_api_n(struct ath10k *ar,
ret = ath10k_core_search_bd(ar, boardname, data, len);
/* if we didn't find it and have a fallback name, try that */
- if (ret == -ENOENT && fallback_boardname)
- ret = ath10k_core_search_bd(ar, fallback_boardname, data, len);
+ if (ret == -ENOENT && fallback_boardname1)
+ ret = ath10k_core_search_bd(ar, fallback_boardname1, data, len);
+
+ if (ret == -ENOENT && fallback_boardname2)
+ ret = ath10k_core_search_bd(ar, fallback_boardname2, data, len);
if (ret == -ENOENT) {
ath10k_err(ar,
@@ -1413,7 +1417,8 @@ 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_variant,
+ bool with_chip_id)
{
/* strlen(',variant=') + strlen(ar->id.bdf_ext) */
char variant[9 + ATH10K_SMBIOS_BDF_EXT_STR_LENGTH] = { 0 };
@@ -1432,12 +1437,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_variant && ar->id.bdf_ext[0] != '\0' && with_chip_id)
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_chip_id)
+ 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",
@@ -1476,21 +1486,33 @@ static int ath10k_core_create_eboard_name(struct ath10k *ar, char *name,
int ath10k_core_fetch_board_file(struct ath10k *ar, int bd_ie_type)
{
- char boardname[100], fallback_boardname[100];
+ char boardname[100], fallback_boardname1[100], fallback_boardname2[100];
int ret;
if (bd_ie_type == ATH10K_BD_IE_BOARD) {
+ /* With variant and chip id */
ret = ath10k_core_create_board_name(ar, boardname,
- sizeof(boardname), true);
+ sizeof(boardname), true, true);
if (ret) {
ath10k_err(ar, "failed to create board name: %d", ret);
return ret;
}
- ret = ath10k_core_create_board_name(ar, fallback_boardname,
- sizeof(boardname), false);
+ /* Without variant and only chip-id */
+ ret = ath10k_core_create_board_name(ar, fallback_boardname1,
+ sizeof(boardname), false,
+ true);
+ if (ret) {
+ ath10k_err(ar, "failed to create 1st fallback board name: %d", ret);
+ return ret;
+ }
+
+ /* Without variant and without chip-id */
+ ret = ath10k_core_create_board_name(ar, fallback_boardname2,
+ sizeof(boardname), false,
+ false);
if (ret) {
- ath10k_err(ar, "failed to create fallback board name: %d", ret);
+ ath10k_err(ar, "failed to create 2nd fallback board name: %d", ret);
return ret;
}
} else if (bd_ie_type == ATH10K_BD_IE_BOARD_EXT) {
@@ -1504,7 +1526,8 @@ int ath10k_core_fetch_board_file(struct ath10k *ar, int bd_ie_type)
ar->bd_api = 2;
ret = ath10k_core_fetch_board_data_api_n(ar, boardname,
- fallback_boardname,
+ fallback_boardname1,
+ fallback_boardname2,
ATH10K_BOARD_API2_FILE);
if (!ret)
goto success;
--
2.29.0.rc1.297.gfa9743e501-goog
Hi,
On Mon, Oct 19, 2020 at 5:51 PM Abhishek Kumar <[email protected]> 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'
>
> Signed-off-by: Abhishek Kumar <[email protected]>
> ---
>
> drivers/net/wireless/ath/ath10k/core.c | 45 +++++++++++++++++++-------
> 1 file changed, 34 insertions(+), 11 deletions(-)
You might want to think a little more about who you're CCing when you
post a patch. I would have imagined that Rakesh should have been
CCed. Probably Brian and I, too? Luckily I knew to look for it.
> int ath10k_core_fetch_board_file(struct ath10k *ar, int bd_ie_type)
> {
> - char boardname[100], fallback_boardname[100];
> + char boardname[100], fallback_boardname1[100], fallback_boardname2[100];
> int ret;
>
> if (bd_ie_type == ATH10K_BD_IE_BOARD) {
> + /* With variant and chip id */
> ret = ath10k_core_create_board_name(ar, boardname,
> - sizeof(boardname), true);
> + sizeof(boardname), true, true);
I don't know if it's worth spinning the patch just for this, but it's
weird that the indentation of the 2nd line above doesn't match the
indentation of the similar calls below.
Other than the tiny nit, this seems sane to me.
Reviewed-by: Douglas Anderson <[email protected]>
I've also confirmed that the patch is behaving properly on two boards
I have with slightly different chip-ids. I tried poking with various
combinations of specifying a variant in dts and not specifying one.
Board 1:
qmi chip_id 0x320 chip_family 0x4001 board_id 0xff soc_id 0x400c0000
Board 2:
qmi chip_id 0x4320 chip_family 0x4001 board_id 0xff soc_id 0x400c0000
Both boards report "WCN3990 hw1.0 SNOC", so I guess the "Tested-on"
tag should be:
Tested-on: WCN3990 hw1.0 SNOC WLAN.HL.3.2.2-00696-QCAHLSWMTPL-1
Tested-by: Douglas Anderson <[email protected]>
-Doug
Additionally tested on ath10k based non-QMI platform
Tested-on: QCA6174 HW3.2 WLAN.RM.4.4.1-00157-QCARMSWPZ-1
Tested-by: Abhishek Kumar <[email protected]>
-Abhishek
Hi,
On Sat, Oct 24, 2020 at 9:40 AM Rakesh Pillai <[email protected]> wrote:
>
> > if (bd_ie_type == ATH10K_BD_IE_BOARD) {
> > + /* With variant and chip id */
> > ret = ath10k_core_create_board_name(ar, boardname,
> > - sizeof(boardname), true);
> > + sizeof(boardname), true, true);
>
> Instead of adding a lot of code to generate a second fallback name, its better to just modify the condition inside the function “ath10k_core_create_board_name” to allow the generation of BDF tag using chip id, even “if ar->id.bdf_ext[0] == '\0 “.
>
> This will make sure that the variant string is NULL, and just board-id and chip-id is used. This will help avoid most of the code changes.
> The code would look as shown below
>
> @@ -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_variant)
Wouldn't the above just be "if (with_chip_id)" instead? ...but yeah,
that would be a cleaner way to do this. Abhishek: do you want to post
a v2?
-Doug
> -----Original Message-----
> From: Doug Anderson <[email protected]>
> Sent: Tuesday, October 27, 2020 4:21 AM
> To: Rakesh Pillai <[email protected]>
> Cc: Abhishek Kumar <[email protected]>; Kalle Valo
> <[email protected]>; ath10k <[email protected]>; LKML
> <[email protected]>; linux-wireless <linux-
> [email protected]>; Brian Norris <[email protected]>
> Subject: Re: [PATCH] ath10k: add option for chip-id based BDF selection
>
> Hi,
>
> On Sat, Oct 24, 2020 at 9:40 AM Rakesh Pillai <[email protected]> wrote:
> >
> > > if (bd_ie_type == ATH10K_BD_IE_BOARD) {
> > > + /* With variant and chip id */
> > > ret = ath10k_core_create_board_name(ar, boardname,
> > > - sizeof(boardname), true);
> > > + sizeof(boardname), true, true);
> >
> > Instead of adding a lot of code to generate a second fallback name, its
> better to just modify the condition inside the function
> “ath10k_core_create_board_name” to allow the generation of BDF tag using
> chip id, even “if ar->id.bdf_ext[0] == '\0 “.
> >
> > This will make sure that the variant string is NULL, and just board-id and
> chip-id is used. This will help avoid most of the code changes.
> > The code would look as shown below
> >
> > @@ -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_variant)
>
> Wouldn't the above just be "if (with_chip_id)" instead? ...but yeah,
> that would be a cleaner way to do this. Abhishek: do you want to post
> a v2?
The parameter name passed to this function is "with_variant", since other non-qmi targets (eg QCA6174) use this as a flag to just add the variant field.
This can be renamed to something meaningful for both qmi and non-qmi targets.
>
> -Doug
> -----Original Message-----
> From: Doug Anderson <[email protected]>
> Sent: Tuesday, October 27, 2020 8:26 PM
> To: Rakesh Pillai <[email protected]>
> Cc: Abhishek Kumar <[email protected]>; Kalle Valo
> <[email protected]>; ath10k <[email protected]>; LKML
> <[email protected]>; linux-wireless <linux-
> [email protected]>; Brian Norris <[email protected]>
> Subject: Re: [PATCH] ath10k: add option for chip-id based BDF selection
>
> Hi,
>
> On Mon, Oct 26, 2020 at 10:18 PM Rakesh Pillai <[email protected]>
> wrote:
> >
> >
> >
> > > -----Original Message-----
> > > From: Doug Anderson <[email protected]>
> > > Sent: Tuesday, October 27, 2020 4:21 AM
> > > To: Rakesh Pillai <[email protected]>
> > > Cc: Abhishek Kumar <[email protected]>; Kalle Valo
> > > <[email protected]>; ath10k <[email protected]>; LKML
> > > <[email protected]>; linux-wireless <linux-
> > > [email protected]>; Brian Norris <[email protected]>
> > > Subject: Re: [PATCH] ath10k: add option for chip-id based BDF selection
> > >
> > > Hi,
> > >
> > > On Sat, Oct 24, 2020 at 9:40 AM Rakesh Pillai <[email protected]>
> wrote:
> > > >
> > > > > if (bd_ie_type == ATH10K_BD_IE_BOARD) {
> > > > > + /* With variant and chip id */
> > > > > ret = ath10k_core_create_board_name(ar, boardname,
> > > > > - sizeof(boardname), true);
> > > > > + sizeof(boardname), true, true);
> > > >
> > > > Instead of adding a lot of code to generate a second fallback name, its
> > > better to just modify the condition inside the function
> > > “ath10k_core_create_board_name” to allow the generation of BDF tag
> using
> > > chip id, even “if ar->id.bdf_ext[0] == '\0 “.
> > > >
> > > > This will make sure that the variant string is NULL, and just board-id and
> > > chip-id is used. This will help avoid most of the code changes.
> > > > The code would look as shown below
> > > >
> > > > @@ -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_variant)
> > >
> > > Wouldn't the above just be "if (with_chip_id)" instead? ...but yeah,
> > > that would be a cleaner way to do this. Abhishek: do you want to post
> > > a v2?
> >
> >
> > The parameter name passed to this function is "with_variant", since other
> non-qmi targets (eg QCA6174) use this as a flag to just add the variant field.
> > This can be renamed to something meaningful for both qmi and non-qmi
> targets.
>
> I think we still need Abhishek's change to have two booleans passed to
> this function, though, right? Thus, it'll be called 3 times:
>
> * with_chip_id = false, with_variant = false
> * with_chip_id = true, with_variant = true
> * with_chip_id = true, with_variant = false
>
> The two cases you want to combine are both with "with_chip_id = true",
> right? The "with_variant" variable being false will make the variant
> string empty.
I meant that we can use the 4th argument passed to the function " ath10k_core_create_board_name" (currently named as with_variant) as an indication to use the BDF name with variant.
But even if with_variant=true, we allow the variant string to be NULL, thereby allowing us to generate a boardname with the format "bus=snoc,qmi-board-id=0xab,qmi-chip-id=0xcd"
The combinations of args/variant-strings for generating different board names will be as follows:
1) with_variant=false : "bus=snoc,qmi-board-id=0xab"
2) with_variant=true, variant_string=NULL : " bus=snoc,qmi-board-id=0xab,qmi-chip-id=0xcd"
3) with_variant=true, variant_string="variant_xyz" : " bus=snoc,qmi-board-id=0xab,qmi-chip-id=0xcd,variant=variant_xyz"
This will minimize the code changes.
>
> -Doug
Abhishek Kumar <[email protected]> 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'
>
> Signed-off-by: Abhishek Kumar <[email protected]>
> Reviewed-by: Douglas Anderson <[email protected]>
> Tested-by: Douglas Anderson <[email protected]>
> Tested-by: Abhishek Kumar <[email protected]>
There were few checkpatch warnings which I fixed:
$ ath10k-check
drivers/net/wireless/ath/ath10k/core.c:1501: Alignment should match open parenthesis
drivers/net/wireless/ath/ath10k/core.c:1512: line length of 92 exceeds 90 columns
drivers/net/wireless/ath/ath10k/core.c:1521: line length of 92 exceeds 90 columns
The first one was also what Doug commented. I also added Tested-on tags,
thanks for those. The updated patch is in pending branch (soon).
But is this patch ok to take now? I didn't quite get the conclusion of the
discussion.
--
https://patchwork.kernel.org/project/linux-wireless/patch/20201020000506.1.Ifbc28707942179f1cefc7491e995814564495270@changeid/
https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches
Apologies for the delay, was busy so could not work on V2 . I have
started working on V2 patch. Will upload by today/tomorrow.
Abhishek
On Thu, Nov 5, 2020 at 11:11 PM Kalle Valo <[email protected]> wrote:
>
> Abhishek Kumar <[email protected]> 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'
> >
> > Signed-off-by: Abhishek Kumar <[email protected]>
> > Reviewed-by: Douglas Anderson <[email protected]>
> > Tested-by: Douglas Anderson <[email protected]>
> > Tested-by: Abhishek Kumar <[email protected]>
>
> There were few checkpatch warnings which I fixed:
>
> $ ath10k-check
> drivers/net/wireless/ath/ath10k/core.c:1501: Alignment should match open parenthesis
> drivers/net/wireless/ath/ath10k/core.c:1512: line length of 92 exceeds 90 columns
> drivers/net/wireless/ath/ath10k/core.c:1521: line length of 92 exceeds 90 columns
>
> The first one was also what Doug commented. I also added Tested-on tags,
> thanks for those. The updated patch is in pending branch (soon).
>
> But is this patch ok to take now? I didn't quite get the conclusion of the
> discussion.
>
> --
> https://patchwork.kernel.org/project/linux-wireless/patch/20201020000506.1.Ifbc28707942179f1cefc7491e995814564495270@changeid/
>
> https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches
>