| Message ID | 20201020000506.1.Ifbc28707942179f1cefc7491e995814564495270@changeid (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Kalle Valo |
| Headers | show |
| Series | ath10k: add option for chip-id based BDF selection | expand |
Hi, On Mon, Oct 19, 2020 at 5:51 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' > > Signed-off-by: Abhishek Kumar <kuabhs@chromium.org> > --- > > 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 <dianders@chromium.org> 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 <dianders@chromium.org> -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 <kuabhs@chromium.org>
-Abhishek
Hi, On Sat, Oct 24, 2020 at 9:40 AM Rakesh Pillai <pillair@codeaurora.org> 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 <dianders@chromium.org> > Sent: Tuesday, October 27, 2020 4:21 AM > To: Rakesh Pillai <pillair@codeaurora.org> > Cc: Abhishek Kumar <kuabhs@chromium.org>; Kalle Valo > <kvalo@codeaurora.org>; ath10k <ath10k@lists.infradead.org>; LKML > <linux-kernel@vger.kernel.org>; linux-wireless <linux- > wireless@vger.kernel.org>; Brian Norris <briannorris@chromium.org> > Subject: Re: [PATCH] ath10k: add option for chip-id based BDF selection > > Hi, > > On Sat, Oct 24, 2020 at 9:40 AM Rakesh Pillai <pillair@codeaurora.org> 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
Hi, On Mon, Oct 26, 2020 at 10:18 PM Rakesh Pillai <pillair@codeaurora.org> wrote: > > > > > -----Original Message----- > > From: Doug Anderson <dianders@chromium.org> > > Sent: Tuesday, October 27, 2020 4:21 AM > > To: Rakesh Pillai <pillair@codeaurora.org> > > Cc: Abhishek Kumar <kuabhs@chromium.org>; Kalle Valo > > <kvalo@codeaurora.org>; ath10k <ath10k@lists.infradead.org>; LKML > > <linux-kernel@vger.kernel.org>; linux-wireless <linux- > > wireless@vger.kernel.org>; Brian Norris <briannorris@chromium.org> > > Subject: Re: [PATCH] ath10k: add option for chip-id based BDF selection > > > > Hi, > > > > On Sat, Oct 24, 2020 at 9:40 AM Rakesh Pillai <pillair@codeaurora.org> 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. -Doug
> -----Original Message----- > From: Doug Anderson <dianders@chromium.org> > Sent: Tuesday, October 27, 2020 8:26 PM > To: Rakesh Pillai <pillair@codeaurora.org> > Cc: Abhishek Kumar <kuabhs@chromium.org>; Kalle Valo > <kvalo@codeaurora.org>; ath10k <ath10k@lists.infradead.org>; LKML > <linux-kernel@vger.kernel.org>; linux-wireless <linux- > wireless@vger.kernel.org>; Brian Norris <briannorris@chromium.org> > Subject: Re: [PATCH] ath10k: add option for chip-id based BDF selection > > Hi, > > On Mon, Oct 26, 2020 at 10:18 PM Rakesh Pillai <pillair@codeaurora.org> > wrote: > > > > > > > > > -----Original Message----- > > > From: Doug Anderson <dianders@chromium.org> > > > Sent: Tuesday, October 27, 2020 4:21 AM > > > To: Rakesh Pillai <pillair@codeaurora.org> > > > Cc: Abhishek Kumar <kuabhs@chromium.org>; Kalle Valo > > > <kvalo@codeaurora.org>; ath10k <ath10k@lists.infradead.org>; LKML > > > <linux-kernel@vger.kernel.org>; linux-wireless <linux- > > > wireless@vger.kernel.org>; Brian Norris <briannorris@chromium.org> > > > Subject: Re: [PATCH] ath10k: add option for chip-id based BDF selection > > > > > > Hi, > > > > > > On Sat, Oct 24, 2020 at 9:40 AM Rakesh Pillai <pillair@codeaurora.org> > 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 <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' > > Signed-off-by: Abhishek Kumar <kuabhs@chromium.org> > Reviewed-by: Douglas Anderson <dianders@chromium.org> > Tested-by: Douglas Anderson <dianders@chromium.org> > Tested-by: Abhishek Kumar <kuabhs@chromium.org> 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.
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 <kvalo@codeaurora.org> wrote: > > 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' > > > > Signed-off-by: Abhishek Kumar <kuabhs@chromium.org> > > Reviewed-by: Douglas Anderson <dianders@chromium.org> > > Tested-by: Douglas Anderson <dianders@chromium.org> > > Tested-by: Abhishek Kumar <kuabhs@chromium.org> > > 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 >
Abhishek Kumar <kuabhs@chromium.org> writes: > Apologies for the delay, was busy so could not work on V2 . I have > started working on V2 patch. Will upload by today/tomorrow. Thanks, I'll then drop this v1.
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;
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 <kuabhs@chromium.org> --- drivers/net/wireless/ath/ath10k/core.c | 45 +++++++++++++++++++------- 1 file changed, 34 insertions(+), 11 deletions(-)