| Message ID | 20220107200417.1.Ie4dcc45b0bf365077303c596891d460d716bb4c5@changeid |
|---|---|
| State | New |
| Headers | show |
| Series | [1/2] ath10k: search for default BDF name provided in DT | expand |
Hi Abhishek,
Thank you for the patch! Perhaps something to improve:
[auto build test WARNING on kvalo-ath/ath-next]
[also build test WARNING on kvalo-wireless-drivers-next/master kvalo-wireless-drivers/master v5.16-rc8 next-20220107]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]
url: https://github.com/0day-ci/linux/commits/Abhishek-Kumar/ath10k-search-for-default-BDF-name-provided-in-DT/20220108-040711
base: https://git.kernel.org/pub/scm/linux/kernel/git/kvalo/ath.git ath-next
config: alpha-allyesconfig (https://download.01.org/0day-ci/archive/20220108/202201081118.iN7Rcf4J-lkp@intel.com/config)
compiler: alpha-linux-gcc (GCC) 11.2.0
reproduce (this is a W=1 build):
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# https://github.com/0day-ci/linux/commit/98e7f008cb14b9c4038287915c271bb485a89346
git remote add linux-review https://github.com/0day-ci/linux
git fetch --no-tags linux-review Abhishek-Kumar/ath10k-search-for-default-BDF-name-provided-in-DT/20220108-040711
git checkout 98e7f008cb14b9c4038287915c271bb485a89346
# save the config file to linux build tree
mkdir build_dir
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-11.2.0 make.cross O=build_dir ARCH=alpha SHELL=/bin/bash drivers/net/wireless/ath/ath10k/
If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>
All warnings (new ones prefixed by >>):
drivers/net/wireless/ath/ath10k/core.c: In function 'ath10k_core_parse_default_bdf_dt':
>> drivers/net/wireless/ath/ath10k/core.c:1103:115: warning: format '%d' expects argument of type 'int', but argument 4 has type 'long unsigned int' [-Wformat=]
1103 | "default board name is longer than allocated buffer, board_name: %s; allocated size: %d\n",
| ~^
| |
| int
| %ld
1104 | board_name, sizeof(ar->id.default_bdf));
| ~~~~~~~~~~~~~~~~~~~~~~~~~~
| |
| long unsigned int
vim +1103 drivers/net/wireless/ath/ath10k/core.c
1083
1084 int ath10k_core_parse_default_bdf_dt(struct ath10k *ar)
1085 {
1086 struct device_node *node;
1087 const char *board_name = NULL;
1088
1089 ar->id.default_bdf[0] = '\0';
1090
1091 node = ar->dev->of_node;
1092 if (!node)
1093 return -ENOENT;
1094
1095 of_property_read_string(node, "qcom,ath10k-default-bdf",
1096 &board_name);
1097 if (!board_name)
1098 return -ENODATA;
1099
1100 if (strscpy(ar->id.default_bdf,
1101 board_name, sizeof(ar->id.default_bdf)) < 0)
1102 ath10k_warn(ar,
> 1103 "default board name is longer than allocated buffer, board_name: %s; allocated size: %d\n",
1104 board_name, sizeof(ar->id.default_bdf));
1105
1106 return 0;
1107 }
1108 EXPORT_SYMBOL(ath10k_core_parse_default_bdf_dt);
1109
---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org
Doug Anderson <dianders@chromium.org> writes: > Hi, > > On Fri, Jan 7, 2022 at 12:05 PM Abhishek Kumar <kuabhs@chromium.org> wrote: >> >> There can be cases where the board-2.bin does not contain >> any BDF matching the chip-id+board-id+variant combination. >> This causes the wlan probe to fail and renders wifi unusable. >> For e.g. if the board-2.bin has default BDF as: >> bus=snoc,qmi-board-id=67 but for some reason the board-id >> on the wlan chip is not programmed and read 0xff as the >> default value. In such cases there won't be any matching BDF >> because the board-2.bin will be searched with following: >> bus=snoc,qmi-board-id=ff I just checked, in ath10k-firmware WCN3990/hw1.0/board-2.bin we have that entry: BoardNames[1]: 'bus=snoc,qmi-board-id=ff' >> To address these scenarios, there can be an option to provide >> fallback default BDF name in the device tree. If none of the >> BDF names match then the board-2.bin file can be searched with >> default BDF names assigned in the device tree. >> >> The default BDF name can be set as: >> wifi@a000000 { >> status = "okay"; >> qcom,ath10k-default-bdf = "bus=snoc,qmi-board-id=67"; > > Rather than add a new device tree property, wouldn't it be good enough > to leverage the existing variant? I'm not thrilled either adding this to Device Tree, we should keep the bindings as simple as possible. > Right now I think that the board file contains: > > 'bus=snoc,qmi-board-id=67.bin' > 'bus=snoc,qmi-board-id=67,qmi-chip-id=320,variant=GO_LAZOR.bin' > 'bus=snoc,qmi-board-id=67,qmi-chip-id=320,variant=GO_POMPOM.bin' > 'bus=snoc,qmi-board-id=67,qmi-chip-id=4320,variant=GO_LAZOR.bin' > 'bus=snoc,qmi-board-id=67,qmi-chip-id=4320,variant=GO_POMPOM.bin' > > ...and, on lazor for instance, we have: > > qcom,ath10k-calibration-variant = "GO_LAZOR"; > > The problem you're trying to solve is that on some early lazor > prototype hardware we didn't have the "board-id" programmed we could > get back 0xff from the hardware. As I understand it 0xff always means > "unprogrammed". > > It feels like you could just have a special case such that if the > hardware reports board ID of 0xff and you don't get a "match" that you > could just treat 0xff as a wildcard. That means that you'd see the > "variant" in the device tree and pick one of the "GO_LAZOR" entries. > > Anyway, I guess it's up to the people who spend more time in this file > which they'd prefer, but that seems like it'd be simple and wouldn't > require a bindings addition... In ath11k we need something similar for that I have been thinking like this: 'bus=snoc,qmi-board-id=67,qmi-chip-id=320,variant=GO_LAZOR' 'bus=snoc,qmi-board-id=67,qmi-chip-id=320' 'bus=snoc,qmi-board-id=67' 'bus=snoc' Ie. we drop one attribute at a time and try to find a suitable board file. Though I'm not sure if it's possible to find a board file which works with many different hardware, but the principle would be at least that. Would something like that work in your case?
Hi, On Thu, Mar 10, 2022 at 2:06 AM Kalle Valo <kvalo@kernel.org> wrote: > > Doug Anderson <dianders@chromium.org> writes: > > > Hi, > > > > On Fri, Jan 7, 2022 at 12:05 PM Abhishek Kumar <kuabhs@chromium.org> wrote: > >> > >> There can be cases where the board-2.bin does not contain > >> any BDF matching the chip-id+board-id+variant combination. > >> This causes the wlan probe to fail and renders wifi unusable. > >> For e.g. if the board-2.bin has default BDF as: > >> bus=snoc,qmi-board-id=67 but for some reason the board-id > >> on the wlan chip is not programmed and read 0xff as the > >> default value. In such cases there won't be any matching BDF > >> because the board-2.bin will be searched with following: > >> bus=snoc,qmi-board-id=ff > > I just checked, in ath10k-firmware WCN3990/hw1.0/board-2.bin we have > that entry: > > BoardNames[1]: 'bus=snoc,qmi-board-id=ff' > > >> To address these scenarios, there can be an option to provide > >> fallback default BDF name in the device tree. If none of the > >> BDF names match then the board-2.bin file can be searched with > >> default BDF names assigned in the device tree. > >> > >> The default BDF name can be set as: > >> wifi@a000000 { > >> status = "okay"; > >> qcom,ath10k-default-bdf = "bus=snoc,qmi-board-id=67"; > > > > Rather than add a new device tree property, wouldn't it be good enough > > to leverage the existing variant? > > I'm not thrilled either adding this to Device Tree, we should keep the > bindings as simple as possible. > > > Right now I think that the board file contains: > > > > 'bus=snoc,qmi-board-id=67.bin' > > 'bus=snoc,qmi-board-id=67,qmi-chip-id=320,variant=GO_LAZOR.bin' > > 'bus=snoc,qmi-board-id=67,qmi-chip-id=320,variant=GO_POMPOM.bin' > > 'bus=snoc,qmi-board-id=67,qmi-chip-id=4320,variant=GO_LAZOR.bin' > > 'bus=snoc,qmi-board-id=67,qmi-chip-id=4320,variant=GO_POMPOM.bin' > > > > ...and, on lazor for instance, we have: > > > > qcom,ath10k-calibration-variant = "GO_LAZOR"; > > > > The problem you're trying to solve is that on some early lazor > > prototype hardware we didn't have the "board-id" programmed we could > > get back 0xff from the hardware. As I understand it 0xff always means > > "unprogrammed". > > > > It feels like you could just have a special case such that if the > > hardware reports board ID of 0xff and you don't get a "match" that you > > could just treat 0xff as a wildcard. That means that you'd see the > > "variant" in the device tree and pick one of the "GO_LAZOR" entries. > > > > Anyway, I guess it's up to the people who spend more time in this file > > which they'd prefer, but that seems like it'd be simple and wouldn't > > require a bindings addition... > > In ath11k we need something similar for that I have been thinking like > this: > > 'bus=snoc,qmi-board-id=67,qmi-chip-id=320,variant=GO_LAZOR' > > 'bus=snoc,qmi-board-id=67,qmi-chip-id=320' > > 'bus=snoc,qmi-board-id=67' > > 'bus=snoc' > > Ie. we drop one attribute at a time and try to find a suitable board > file. Though I'm not sure if it's possible to find a board file which > works with many different hardware, but the principle would be at least > that. Would something like that work in your case? I'll leave it for Abhishek to comment for sure since he's been the one actively involved in keeping track of our board-2.bin file. As far as I know: * Mostly we need this for pre-production hardware that developers inside Google and Qualcomm still have sitting around on their desks. I guess some people are even still using this pre-production hardware to surf the web? * In the ideal world, we think that the best calibration would be to use the board-specific one in these cases. AKA if board_id is 0xff (unprogrammed) and variant is "GO_LAZOR" then the best solution would be to use the settings for board id 0x67 and variant "GO_LAZOR". This _ought_ to be better settings than the default 0xff settings without the "GO_LAZOR". * In reality, we're probably OK as long as _some_ reasonable settings get picked. WiFi might not be super range optimized but at least it will still come up and work. -Doug
Hi All, Apologies for the late reply, too many things at the same time. Just a quick note, Qcomm provided a new BDF file with support for 'bus=snoc,qmi-board-id=ff' as well, so even without this patch, the non-programmed devices(with board-id=0xff) would work. However, for optimization of the BDF search strategy, the above mentioned strategy would still not work: - The stripping of full Board name would not work if board-id itself is not programmed i.e. =0xff e.g. for 'bus=snoc,qmi-board-id=ff,qmi-chip-id=320,variant=GO_LAZOR' => no match 'bus=snoc,qmi-board-id=ff,qmi-chip-id=320' => no match 'bus=snoc,qmi-board-id=ff' => no match 'bus=snoc' => no match because all the BDFs contains board-id=67 So with this DT patch specifically for case 'bus=snoc,qmi-board-id=ff' => no match, we fallback to default case ( the one provided in DT) i.e. 'bus=snoc,qmi-board-id=67' => match . I do not see how exactly the driver can know that it should check for a board-id of 67. However, to still remove dependency on the DT, we can make the board-id as no-op if it is not programmed i.e. if the board-id=ff then we would pick any BDF that match 'bus=snoc,qmi-board-id=<XX>' where XX can be any arbitrary number. Thoughts ? -Abhishek On Thu, Mar 10, 2022 at 4:28 PM Doug Anderson <dianders@chromium.org> wrote: > > Hi, > > On Thu, Mar 10, 2022 at 2:06 AM Kalle Valo <kvalo@kernel.org> wrote: > > > > Doug Anderson <dianders@chromium.org> writes: > > > > > Hi, > > > > > > On Fri, Jan 7, 2022 at 12:05 PM Abhishek Kumar <kuabhs@chromium.org> wrote: > > >> > > >> There can be cases where the board-2.bin does not contain > > >> any BDF matching the chip-id+board-id+variant combination. > > >> This causes the wlan probe to fail and renders wifi unusable. > > >> For e.g. if the board-2.bin has default BDF as: > > >> bus=snoc,qmi-board-id=67 but for some reason the board-id > > >> on the wlan chip is not programmed and read 0xff as the > > >> default value. In such cases there won't be any matching BDF > > >> because the board-2.bin will be searched with following: > > >> bus=snoc,qmi-board-id=ff > > > > I just checked, in ath10k-firmware WCN3990/hw1.0/board-2.bin we have > > that entry: > > > > BoardNames[1]: 'bus=snoc,qmi-board-id=ff' > > > > >> To address these scenarios, there can be an option to provide > > >> fallback default BDF name in the device tree. If none of the > > >> BDF names match then the board-2.bin file can be searched with > > >> default BDF names assigned in the device tree. > > >> > > >> The default BDF name can be set as: > > >> wifi@a000000 { > > >> status = "okay"; > > >> qcom,ath10k-default-bdf = "bus=snoc,qmi-board-id=67"; > > > > > > Rather than add a new device tree property, wouldn't it be good enough > > > to leverage the existing variant? > > > > I'm not thrilled either adding this to Device Tree, we should keep the > > bindings as simple as possible. > > > > > Right now I think that the board file contains: > > > > > > 'bus=snoc,qmi-board-id=67.bin' > > > 'bus=snoc,qmi-board-id=67,qmi-chip-id=320,variant=GO_LAZOR.bin' > > > 'bus=snoc,qmi-board-id=67,qmi-chip-id=320,variant=GO_POMPOM.bin' > > > 'bus=snoc,qmi-board-id=67,qmi-chip-id=4320,variant=GO_LAZOR.bin' > > > 'bus=snoc,qmi-board-id=67,qmi-chip-id=4320,variant=GO_POMPOM.bin' > > > > > > ...and, on lazor for instance, we have: > > > > > > qcom,ath10k-calibration-variant = "GO_LAZOR"; > > > > > > The problem you're trying to solve is that on some early lazor > > > prototype hardware we didn't have the "board-id" programmed we could > > > get back 0xff from the hardware. As I understand it 0xff always means > > > "unprogrammed". > > > > > > It feels like you could just have a special case such that if the > > > hardware reports board ID of 0xff and you don't get a "match" that you > > > could just treat 0xff as a wildcard. That means that you'd see the > > > "variant" in the device tree and pick one of the "GO_LAZOR" entries. > > > > > > Anyway, I guess it's up to the people who spend more time in this file > > > which they'd prefer, but that seems like it'd be simple and wouldn't > > > require a bindings addition... > > > > In ath11k we need something similar for that I have been thinking like > > this: > > > > 'bus=snoc,qmi-board-id=67,qmi-chip-id=320,variant=GO_LAZOR' > > > > 'bus=snoc,qmi-board-id=67,qmi-chip-id=320' > > > > 'bus=snoc,qmi-board-id=67' > > > > 'bus=snoc' > > > > Ie. we drop one attribute at a time and try to find a suitable board > > file. Though I'm not sure if it's possible to find a board file which > > works with many different hardware, but the principle would be at least > > that. Would something like that work in your case? > > I'll leave it for Abhishek to comment for sure since he's been the one > actively involved in keeping track of our board-2.bin file. As far as > I know: > > * Mostly we need this for pre-production hardware that developers > inside Google and Qualcomm still have sitting around on their desks. I > guess some people are even still using this pre-production hardware to > surf the web? > > * In the ideal world, we think that the best calibration would be to > use the board-specific one in these cases. AKA if board_id is 0xff > (unprogrammed) and variant is "GO_LAZOR" then the best solution would > be to use the settings for board id 0x67 and variant "GO_LAZOR". This > _ought_ to be better settings than the default 0xff settings without > the "GO_LAZOR". > > * In reality, we're probably OK as long as _some_ reasonable settings > get picked. WiFi might not be super range optimized but at least it > will still come up and work.
On Tue, Apr 12, 2022 at 3:47 AM Kalle Valo <kvalo@kernel.org> wrote: > > Abhishek Kumar <kuabhs@chromium.org> writes: > > > Hi All, > > > > Apologies for the late reply, too many things at the same time. > > Trust me, I know the feeling :) > > > Just a quick note, Qcomm provided a new BDF file with support for > > 'bus=snoc,qmi-board-id=ff' as well, so even without this patch, the > > non-programmed devices(with board-id=0xff) would work. However, for > > optimization of the BDF search strategy, the above mentioned strategy > > would still not work: - The stripping of full Board name would not > > work if board-id itself is not programmed i.e. =0xff e.g. for > > 'bus=snoc,qmi-board-id=ff,qmi-chip-id=320,variant=GO_LAZOR' => no > > match 'bus=snoc,qmi-board-id=ff,qmi-chip-id=320' => no match > > 'bus=snoc,qmi-board-id=ff' => no match 'bus=snoc' => no match because > > all the BDFs contains board-id=67 > > Sorry, not fully following your here. Are you saying that the problem is > that WCN3990/hw1.0/board-2.bin is missing board file for 'bus=snoc'? Ya, that is what I meant here, the board-2.bin file does not contain an entry for 'bus=snoc' and so if board-id=oxff then still there cannot be any BDF that matches. So adding BDF for 'bus=snoc' would simplify the approach. > That's easy to add, each board file within board-2.bin has multiple > names so we can easily select one board file for which we add > 'bus=snoc'. > > > So with this DT patch specifically for case 'bus=snoc,qmi-board-id=ff' > > => no match, we fallback to default case ( the one provided in DT) > > i.e. 'bus=snoc,qmi-board-id=67' => match . I do not see how exactly > > the driver can know that it should check for a board-id of 67. > > Sorry, not following you here either. Why would the driver need to use > board-id 67? > > > However, to still remove dependency on the DT, we can make the > > board-id as no-op if it is not programmed i.e. if the board-id=ff then > > we would pick any BDF that match 'bus=snoc,qmi-board-id=<XX>' where XX > > can be any arbitrary number. Thoughts ? > > To me using just 'bus=snoc' is more logical than picking up an arbitrary > number. But I might be missing something here. The reason I mentioned that if the board-id=oxff then pick any available BDF entry which matches 'bus=snoc,qmi-board-id=<XX>' is because: - This will atleast let the wlan chip to boot. - There is no BDF for 'bus=snoc' , so further stripping of boardname will not find any match, but as you mentioned that BDF for 'bus=snoc' can be added, then this will make the logic simpler and we don't have to pick 'bus=snoc,qmi-board-id=<XX>' with any arbitrary board-id. I will rollout a patch with this approach. > > -- > https://patchwork.kernel.org/project/linux-wireless/list/ > > https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches Thanks Abhishek
diff --git a/drivers/net/wireless/ath/ath10k/core.c b/drivers/net/wireless/ath/ath10k/core.c index 8f5b8eb368fa..a7008c853f0f 100644 --- a/drivers/net/wireless/ath/ath10k/core.c +++ b/drivers/net/wireless/ath/ath10k/core.c @@ -1081,6 +1081,32 @@ int ath10k_core_check_dt(struct ath10k *ar) } EXPORT_SYMBOL(ath10k_core_check_dt); +int ath10k_core_parse_default_bdf_dt(struct ath10k *ar) +{ + struct device_node *node; + const char *board_name = NULL; + + ar->id.default_bdf[0] = '\0'; + + node = ar->dev->of_node; + if (!node) + return -ENOENT; + + of_property_read_string(node, "qcom,ath10k-default-bdf", + &board_name); + if (!board_name) + return -ENODATA; + + if (strscpy(ar->id.default_bdf, + board_name, sizeof(ar->id.default_bdf)) < 0) + ath10k_warn(ar, + "default board name is longer than allocated buffer, board_name: %s; allocated size: %d\n", + board_name, sizeof(ar->id.default_bdf)); + + return 0; +} +EXPORT_SYMBOL(ath10k_core_parse_default_bdf_dt); + static int ath10k_download_fw(struct ath10k *ar) { u32 address, data_len; @@ -1441,6 +1467,10 @@ static int ath10k_core_fetch_board_data_api_n(struct ath10k *ar, if (ret == -ENOENT && fallback_boardname2) ret = ath10k_core_search_bd(ar, fallback_boardname2, data, len); + /* check default BDF name if provided in device tree */ + if (ret == -ENOENT && ar->id.default_bdf[0] != '\0') + ret = ath10k_core_search_bd(ar, ar->id.default_bdf, data, len); + if (ret == -ENOENT) { ath10k_err(ar, "failed to fetch board data for %s from %s/%s\n", diff --git a/drivers/net/wireless/ath/ath10k/core.h b/drivers/net/wireless/ath/ath10k/core.h index 9f6680b3be0a..1201bb7bb8ab 100644 --- a/drivers/net/wireless/ath/ath10k/core.h +++ b/drivers/net/wireless/ath/ath10k/core.h @@ -79,6 +79,9 @@ /* The magic used by QCA spec */ #define ATH10K_SMBIOS_BDF_EXT_MAGIC "BDF_" +/* Default BDF board name buffer size */ +#define ATH10K_DEFAULT_BDF_BUFFER_SIZE 0x40 + /* Default Airtime weight multipler (Tuned for multiclient performance) */ #define ATH10K_AIRTIME_WEIGHT_MULTIPLIER 4 @@ -1102,6 +1105,7 @@ struct ath10k { bool ext_bid_supported; char bdf_ext[ATH10K_SMBIOS_BDF_EXT_STR_LENGTH]; + char default_bdf[ATH10K_DEFAULT_BDF_BUFFER_SIZE]; } id; int fw_api; @@ -1342,6 +1346,7 @@ int ath10k_core_register(struct ath10k *ar, void ath10k_core_unregister(struct ath10k *ar); int ath10k_core_fetch_board_file(struct ath10k *ar, int bd_ie_type); int ath10k_core_check_dt(struct ath10k *ar); +int ath10k_core_parse_default_bdf_dt(struct ath10k *ar); void ath10k_core_free_board_files(struct ath10k *ar); #endif /* _CORE_H_ */ diff --git a/drivers/net/wireless/ath/ath10k/qmi.c b/drivers/net/wireless/ath/ath10k/qmi.c index 80fcb917fe4e..a57675308014 100644 --- a/drivers/net/wireless/ath/ath10k/qmi.c +++ b/drivers/net/wireless/ath/ath10k/qmi.c @@ -831,6 +831,10 @@ static int ath10k_qmi_fetch_board_file(struct ath10k_qmi *qmi) if (ret) ath10k_dbg(ar, ATH10K_DBG_QMI, "DT bdf variant name not set.\n"); + ret = ath10k_core_parse_default_bdf_dt(ar); + if (ret) + ath10k_dbg(ar, ATH10K_DBG_QMI, "Default BDF name not set in device tree.\n"); + return ath10k_core_fetch_board_file(qmi->ar, ATH10K_BD_IE_BOARD); }
There can be cases where the board-2.bin does not contain any BDF matching the chip-id+board-id+variant combination. This causes the wlan probe to fail and renders wifi unusable. For e.g. if the board-2.bin has default BDF as: bus=snoc,qmi-board-id=67 but for some reason the board-id on the wlan chip is not programmed and read 0xff as the default value. In such cases there won't be any matching BDF because the board-2.bin will be searched with following: bus=snoc,qmi-board-id=ff To address these scenarios, there can be an option to provide fallback default BDF name in the device tree. If none of the BDF names match then the board-2.bin file can be searched with default BDF names assigned in the device tree. The default BDF name can be set as: wifi@a000000 { status = "okay"; qcom,ath10k-default-bdf = "bus=snoc,qmi-board-id=67"; }; Tested-on: WCN3990 hw1.0 SNOC WLAN.HL.3.2.2-00696-QCAHLSWMTPL-1 Signed-off-by: Abhishek Kumar <kuabhs@chromium.org> --- drivers/net/wireless/ath/ath10k/core.c | 30 ++++++++++++++++++++++++++ drivers/net/wireless/ath/ath10k/core.h | 5 +++++ drivers/net/wireless/ath/ath10k/qmi.c | 4 ++++ 3 files changed, 39 insertions(+)