linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH 1/2] ath10k: search for default BDF name provided in DT
@ 2022-01-07 20:04 Abhishek Kumar
  2022-01-07 20:04 ` [PATCH 2/2] dt: bindings: add dt entry for ath10k default BDF name Abhishek Kumar
                   ` (2 more replies)
  0 siblings, 3 replies; 10+ messages in thread
From: Abhishek Kumar @ 2022-01-07 20:04 UTC (permalink / raw)
  To: kvalo, dianders
  Cc: pillair, kuabhs, linux-kernel, linux-wireless, David S. Miller,
	Jakub Kicinski, Kalle Valo, ath10k, netdev

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(+)

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);
 }
 
-- 
2.34.1.575.g55b058a8bb-goog


^ permalink raw reply related	[flat|nested] 10+ messages in thread

* [PATCH 2/2] dt: bindings: add dt entry for ath10k default BDF name
  2022-01-07 20:04 [PATCH 1/2] ath10k: search for default BDF name provided in DT Abhishek Kumar
@ 2022-01-07 20:04 ` Abhishek Kumar
  2022-01-10 10:18   ` Kalle Valo
  2022-01-07 21:22 ` [PATCH 1/2] ath10k: search for default BDF name provided in DT Doug Anderson
  2022-01-08  3:14 ` kernel test robot
  2 siblings, 1 reply; 10+ messages in thread
From: Abhishek Kumar @ 2022-01-07 20:04 UTC (permalink / raw)
  To: kvalo, dianders
  Cc: pillair, kuabhs, linux-kernel, linux-wireless, David S. Miller,
	Jakub Kicinski, Kalle Valo, Rob Herring, devicetree, netdev

It is possible that BDF name with board-id+chip-id+variant
combination is not found in the board-2.bin. Such cases can
cause wlan probe to fail and completely break wifi. In such
case there can be an optional property to define a default
BDF name to search for in the board-2.bin file when none of
the combinations (board-id,chip-id,variant) match.
To address the above concern provide an optional proptery:
qcom,ath10k-default-bdf

Signed-off-by: Abhishek Kumar <kuabhs@chromium.org>
---

 .../devicetree/bindings/net/wireless/qcom,ath10k.txt          | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/Documentation/devicetree/bindings/net/wireless/qcom,ath10k.txt b/Documentation/devicetree/bindings/net/wireless/qcom,ath10k.txt
index b61c2d5a0ff7..d76d1392863d 100644
--- a/Documentation/devicetree/bindings/net/wireless/qcom,ath10k.txt
+++ b/Documentation/devicetree/bindings/net/wireless/qcom,ath10k.txt
@@ -63,6 +63,10 @@ Optional properties:
 				 hw versions.
 - qcom,ath10k-pre-calibration-data : pre calibration data as an array,
 				     the length can vary between hw versions.
+- qcom,ath10k-default-bdf : default board data file name to be searched in
+			    board-2.bin. This is searched if no BDF is found
+			    in board-2.bin that matches, chip-id, board-id and
+			    variant combination
 - <supply-name>-supply: handle to the regulator device tree node
 			   optional "supply-name" are "vdd-0.8-cx-mx",
 			   "vdd-1.8-xo", "vdd-1.3-rfa", "vdd-3.3-ch0",
-- 
2.34.1.575.g55b058a8bb-goog


^ permalink raw reply related	[flat|nested] 10+ messages in thread

* Re: [PATCH 1/2] ath10k: search for default BDF name provided in DT
  2022-01-07 20:04 [PATCH 1/2] ath10k: search for default BDF name provided in DT Abhishek Kumar
  2022-01-07 20:04 ` [PATCH 2/2] dt: bindings: add dt entry for ath10k default BDF name Abhishek Kumar
@ 2022-01-07 21:22 ` Doug Anderson
  2022-03-10 10:05   ` Kalle Valo
  2022-01-08  3:14 ` kernel test robot
  2 siblings, 1 reply; 10+ messages in thread
From: Doug Anderson @ 2022-01-07 21:22 UTC (permalink / raw)
  To: Abhishek Kumar
  Cc: Kalle Valo, Rakesh Pillai, LKML, linux-wireless, David S. Miller,
	Jakub Kicinski, Kalle Valo, ath10k, netdev

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
> 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? 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...

-Doug

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH 1/2] ath10k: search for default BDF name provided in DT
  2022-01-07 20:04 [PATCH 1/2] ath10k: search for default BDF name provided in DT Abhishek Kumar
  2022-01-07 20:04 ` [PATCH 2/2] dt: bindings: add dt entry for ath10k default BDF name Abhishek Kumar
  2022-01-07 21:22 ` [PATCH 1/2] ath10k: search for default BDF name provided in DT Doug Anderson
@ 2022-01-08  3:14 ` kernel test robot
  2 siblings, 0 replies; 10+ messages in thread
From: kernel test robot @ 2022-01-08  3:14 UTC (permalink / raw)
  To: Abhishek Kumar, kvalo, dianders
  Cc: kbuild-all, pillair, kuabhs, linux-kernel, linux-wireless,
	Jakub Kicinski, Kalle Valo, ath10k, netdev

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

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH 2/2] dt: bindings: add dt entry for ath10k default BDF name
  2022-01-07 20:04 ` [PATCH 2/2] dt: bindings: add dt entry for ath10k default BDF name Abhishek Kumar
@ 2022-01-10 10:18   ` Kalle Valo
  0 siblings, 0 replies; 10+ messages in thread
From: Kalle Valo @ 2022-01-10 10:18 UTC (permalink / raw)
  To: Abhishek Kumar
  Cc: dianders, pillair, linux-kernel, linux-wireless, David S. Miller,
	Jakub Kicinski, Rob Herring, devicetree, netdev, ath10k

Abhishek Kumar <kuabhs@chromium.org> writes:

> It is possible that BDF name with board-id+chip-id+variant
> combination is not found in the board-2.bin. Such cases can
> cause wlan probe to fail and completely break wifi. In such
> case there can be an optional property to define a default
> BDF name to search for in the board-2.bin file when none of
> the combinations (board-id,chip-id,variant) match.
> To address the above concern provide an optional proptery:
> qcom,ath10k-default-bdf
>
> Signed-off-by: Abhishek Kumar <kuabhs@chromium.org>
> ---
>
>  .../devicetree/bindings/net/wireless/qcom,ath10k.txt          | 4 ++++
>  1 file changed, 4 insertions(+)

Please CC ath10k list on all ath10k related patches.

-- 
https://patchwork.kernel.org/project/linux-wireless/list/

https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH 1/2] ath10k: search for default BDF name provided in DT
  2022-01-07 21:22 ` [PATCH 1/2] ath10k: search for default BDF name provided in DT Doug Anderson
@ 2022-03-10 10:05   ` Kalle Valo
  2022-03-11  0:27     ` Doug Anderson
  0 siblings, 1 reply; 10+ messages in thread
From: Kalle Valo @ 2022-03-10 10:05 UTC (permalink / raw)
  To: Doug Anderson
  Cc: Abhishek Kumar, Rakesh Pillai, LKML, linux-wireless,
	David S. Miller, Jakub Kicinski, ath10k, netdev

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?

-- 
https://patchwork.kernel.org/project/linux-wireless/list/

https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH 1/2] ath10k: search for default BDF name provided in DT
  2022-03-10 10:05   ` Kalle Valo
@ 2022-03-11  0:27     ` Doug Anderson
  2022-04-11 23:25       ` Abhishek Kumar
  0 siblings, 1 reply; 10+ messages in thread
From: Doug Anderson @ 2022-03-11  0:27 UTC (permalink / raw)
  To: Kalle Valo
  Cc: Abhishek Kumar, Rakesh Pillai, LKML, linux-wireless,
	David S. Miller, Jakub Kicinski, ath10k, netdev

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

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH 1/2] ath10k: search for default BDF name provided in DT
  2022-03-11  0:27     ` Doug Anderson
@ 2022-04-11 23:25       ` Abhishek Kumar
  2022-04-12 10:46         ` Kalle Valo
  0 siblings, 1 reply; 10+ messages in thread
From: Abhishek Kumar @ 2022-04-11 23:25 UTC (permalink / raw)
  To: Doug Anderson
  Cc: Kalle Valo, Rakesh Pillai, LKML, linux-wireless, David S. Miller,
	Jakub Kicinski, ath10k, netdev

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.

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH 1/2] ath10k: search for default BDF name provided in DT
  2022-04-11 23:25       ` Abhishek Kumar
@ 2022-04-12 10:46         ` Kalle Valo
  2022-04-20  0:43           ` Abhishek Kumar
  0 siblings, 1 reply; 10+ messages in thread
From: Kalle Valo @ 2022-04-12 10:46 UTC (permalink / raw)
  To: Abhishek Kumar
  Cc: Doug Anderson, Rakesh Pillai, LKML, linux-wireless,
	David S. Miller, Jakub Kicinski, ath10k, netdev

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'?
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.

-- 
https://patchwork.kernel.org/project/linux-wireless/list/

https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH 1/2] ath10k: search for default BDF name provided in DT
  2022-04-12 10:46         ` Kalle Valo
@ 2022-04-20  0:43           ` Abhishek Kumar
  0 siblings, 0 replies; 10+ messages in thread
From: Abhishek Kumar @ 2022-04-20  0:43 UTC (permalink / raw)
  To: Kalle Valo
  Cc: Doug Anderson, Rakesh Pillai, LKML, linux-wireless,
	David S. Miller, Jakub Kicinski, ath10k, netdev

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

^ permalink raw reply	[flat|nested] 10+ messages in thread

end of thread, other threads:[~2022-04-20  0:43 UTC | newest]

Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-01-07 20:04 [PATCH 1/2] ath10k: search for default BDF name provided in DT Abhishek Kumar
2022-01-07 20:04 ` [PATCH 2/2] dt: bindings: add dt entry for ath10k default BDF name Abhishek Kumar
2022-01-10 10:18   ` Kalle Valo
2022-01-07 21:22 ` [PATCH 1/2] ath10k: search for default BDF name provided in DT Doug Anderson
2022-03-10 10:05   ` Kalle Valo
2022-03-11  0:27     ` Doug Anderson
2022-04-11 23:25       ` Abhishek Kumar
2022-04-12 10:46         ` Kalle Valo
2022-04-20  0:43           ` Abhishek Kumar
2022-01-08  3:14 ` kernel test robot

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).