* [PATCH v2 0/1] This patch address comments on patch v1 @ 2020-11-12 20:09 Abhishek Kumar 2020-11-12 20:09 ` [PATCH v2 1/1] ath10k: add option for chip-id based BDF selection Abhishek Kumar 0 siblings, 1 reply; 10+ messages in thread From: Abhishek Kumar @ 2020-11-12 20:09 UTC (permalink / raw) To: kvalo, pillair Cc: dianders, linux-kernel, ath10k, briannorris, linux-wireless, Abhishek Kumar, David S. Miller, Jakub Kicinski, netdev This patch extends ath10k_core_create_board_name function to support chip id based BDF selection and not add provision for fallback_boardname2, thus introducing lesser lines of code. (no changes since v1) Abhishek Kumar (1): ath10k: add option for chip-id based BDF selection drivers/net/wireless/ath/ath10k/core.c | 12 +++++++++--- 1 file changed, 9 insertions(+), 3 deletions(-) -- 2.29.2.222.g5d2a92d10f8-goog ^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH v2 1/1] ath10k: add option for chip-id based BDF selection 2020-11-12 20:09 [PATCH v2 0/1] This patch address comments on patch v1 Abhishek Kumar @ 2020-11-12 20:09 ` Abhishek Kumar 2020-11-24 0:56 ` Doug Anderson 0 siblings, 1 reply; 10+ messages in thread From: Abhishek Kumar @ 2020-11-12 20:09 UTC (permalink / raw) To: kvalo, pillair Cc: dianders, linux-kernel, ath10k, briannorris, linux-wireless, Abhishek Kumar, David S. Miller, Jakub Kicinski, netdev 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(-) 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", -- 2.29.2.222.g5d2a92d10f8-goog ^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH v2 1/1] ath10k: add option for chip-id based BDF selection 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 0 siblings, 1 reply; 10+ messages in thread From: Doug Anderson @ 2020-11-24 0:56 UTC (permalink / raw) To: Abhishek Kumar Cc: Kalle Valo, Rakesh Pillai, LKML, ath10k, Brian Norris, linux-wireless, David S. Miller, Jakub Kicinski, netdev 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 ^ permalink raw reply [flat|nested] 10+ messages in thread
* RE: [PATCH v2 1/1] ath10k: add option for chip-id based BDF selection 2020-11-24 0:56 ` Doug Anderson @ 2020-11-24 9:18 ` Rakesh Pillai 2020-11-24 16:26 ` Doug Anderson 0 siblings, 1 reply; 10+ messages in thread From: Rakesh Pillai @ 2020-11-24 9:18 UTC (permalink / raw) To: 'Doug Anderson', 'Abhishek Kumar' Cc: 'Kalle Valo', 'LKML', 'ath10k', 'Brian Norris', 'linux-wireless', 'David S. Miller', 'Jakub Kicinski', 'netdev' > -----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. ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v2 1/1] ath10k: add option for chip-id based BDF selection 2020-11-24 9:18 ` Rakesh Pillai @ 2020-11-24 16:26 ` Doug Anderson 2020-11-25 3:44 ` Rakesh Pillai 0 siblings, 1 reply; 10+ messages in thread From: Doug Anderson @ 2020-11-24 16:26 UTC (permalink / raw) To: Rakesh Pillai Cc: Abhishek Kumar, Kalle Valo, LKML, ath10k, Brian Norris, linux-wireless, David S. Miller, Jakub Kicinski, netdev 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 ^ permalink raw reply [flat|nested] 10+ messages in thread
* RE: [PATCH v2 1/1] ath10k: add option for chip-id based BDF selection 2020-11-24 16:26 ` Doug Anderson @ 2020-11-25 3:44 ` Rakesh Pillai 2020-11-30 19:18 ` Doug Anderson 0 siblings, 1 reply; 10+ messages in thread From: Rakesh Pillai @ 2020-11-25 3:44 UTC (permalink / raw) To: 'Doug Anderson' Cc: 'Abhishek Kumar', 'Kalle Valo', 'LKML', 'ath10k', 'Brian Norris', 'linux-wireless', 'David S. Miller', 'Jakub Kicinski', 'netdev' > -----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 ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v2 1/1] ath10k: add option for chip-id based BDF selection 2020-11-25 3:44 ` Rakesh Pillai @ 2020-11-30 19:18 ` Doug Anderson 2020-12-03 11:33 ` Rakesh Pillai 0 siblings, 1 reply; 10+ messages in thread From: Doug Anderson @ 2020-11-30 19:18 UTC (permalink / raw) To: Rakesh Pillai Cc: Abhishek Kumar, Kalle Valo, LKML, ath10k, Brian Norris, linux-wireless, David S. Miller, Jakub Kicinski, netdev 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 ^ permalink raw reply [flat|nested] 10+ messages in thread
* RE: [PATCH v2 1/1] ath10k: add option for chip-id based BDF selection 2020-11-30 19:18 ` Doug Anderson @ 2020-12-03 11:33 ` Rakesh Pillai 2020-12-03 15:33 ` Doug Anderson 0 siblings, 1 reply; 10+ messages in thread From: Rakesh Pillai @ 2020-12-03 11:33 UTC (permalink / raw) To: 'Doug Anderson' Cc: 'Abhishek Kumar', 'Kalle Valo', 'LKML', 'ath10k', 'Brian Norris', 'linux-wireless', 'David S. Miller', 'Jakub Kicinski', 'netdev' > -----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 ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v2 1/1] ath10k: add option for chip-id based BDF selection 2020-12-03 11:33 ` Rakesh Pillai @ 2020-12-03 15:33 ` Doug Anderson 2020-12-07 18:14 ` Abhishek Kumar 0 siblings, 1 reply; 10+ messages in thread From: Doug Anderson @ 2020-12-03 15:33 UTC (permalink / raw) To: Rakesh Pillai Cc: Abhishek Kumar, Kalle Valo, LKML, ath10k, Brian Norris, linux-wireless, David S. Miller, Jakub Kicinski, netdev 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 ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v2 1/1] ath10k: add option for chip-id based BDF selection 2020-12-03 15:33 ` Doug Anderson @ 2020-12-07 18:14 ` Abhishek Kumar 0 siblings, 0 replies; 10+ messages in thread From: Abhishek Kumar @ 2020-12-07 18:14 UTC (permalink / raw) To: Doug Anderson Cc: Rakesh Pillai, Kalle Valo, LKML, ath10k, Brian Norris, linux-wireless, David S. Miller, Jakub Kicinski, netdev 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 ^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2020-12-07 18:15 UTC | newest] Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 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 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
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).