[PATCH] ath10k: add option for chip-id based BDF selection

[PATCH] ath10k: add option for chip-id based BDF selection

[Abhishek Kumar]

In some devices difference in chip-id should be enough to pick
the right BDF. Add another support for chip-id based BDF selection.
With this new option, ath10k supports 2 fallback options.

The board name with chip-id as option looks as follows
board name 'bus=snoc,qmi-board-id=ff,qmi-chip-id=320'

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

 drivers/net/wireless/ath/ath10k/core.c | 45 +++++++++++++++++++-------
 1 file changed, 34 insertions(+), 11 deletions(-)

diff --git a/drivers/net/wireless/ath/ath10k/core.c b/drivers/net/wireless/ath/ath10k/core.c
index a3f15cc89a10..767bb9d6a197 100644
--- a/drivers/net/wireless/ath/ath10k/core.c
+++ b/drivers/net/wireless/ath/ath10k/core.c
@@ -1343,7 +1343,8 @@ static int ath10k_core_search_bd(struct ath10k *ar,
 
 static int ath10k_core_fetch_board_data_api_n(struct ath10k *ar,
 					      const char *boardname,
-					      const char *fallback_boardname,
+					      const char *fallback_boardname1,
+					      const char *fallback_boardname2,
 					      const char *filename)
 {
 	size_t len, magic_len;
@@ -1392,8 +1393,11 @@ static int ath10k_core_fetch_board_data_api_n(struct ath10k *ar,
 	ret = ath10k_core_search_bd(ar, boardname, data, len);
 
 	/* if we didn't find it and have a fallback name, try that */
-	if (ret == -ENOENT && fallback_boardname)
-		ret = ath10k_core_search_bd(ar, fallback_boardname, data, len);
+	if (ret == -ENOENT && fallback_boardname1)
+		ret = ath10k_core_search_bd(ar, fallback_boardname1, data, len);
+
+	if (ret == -ENOENT && fallback_boardname2)
+		ret = ath10k_core_search_bd(ar, fallback_boardname2, data, len);
 
 	if (ret == -ENOENT) {
 		ath10k_err(ar,
@@ -1413,7 +1417,8 @@ static int ath10k_core_fetch_board_data_api_n(struct ath10k *ar,
 }
 
 static int ath10k_core_create_board_name(struct ath10k *ar, char *name,
-					 size_t name_len, bool with_variant)
+					 size_t name_len, bool with_variant,
+					 bool with_chip_id)
 {
 	/* strlen(',variant=') + strlen(ar->id.bdf_ext) */
 	char variant[9 + ATH10K_SMBIOS_BDF_EXT_STR_LENGTH] = { 0 };
@@ -1432,12 +1437,17 @@ static int ath10k_core_create_board_name(struct ath10k *ar, char *name,
 	}
 
 	if (ar->id.qmi_ids_valid) {
-		if (with_variant && ar->id.bdf_ext[0] != '\0')
+		if (with_variant && ar->id.bdf_ext[0] != '\0' && with_chip_id)
 			scnprintf(name, name_len,
 				  "bus=%s,qmi-board-id=%x,qmi-chip-id=%x%s",
 				  ath10k_bus_str(ar->hif.bus),
 				  ar->id.qmi_board_id, ar->id.qmi_chip_id,
 				  variant);
+		else if (with_chip_id)
+			scnprintf(name, name_len,
+				  "bus=%s,qmi-board-id=%x,qmi-chip-id=%x",
+				  ath10k_bus_str(ar->hif.bus),
+				  ar->id.qmi_board_id, ar->id.qmi_chip_id);
 		else
 			scnprintf(name, name_len,
 				  "bus=%s,qmi-board-id=%x",
@@ -1476,21 +1486,33 @@ static int ath10k_core_create_eboard_name(struct ath10k *ar, char *name,
 
 int ath10k_core_fetch_board_file(struct ath10k *ar, int bd_ie_type)
 {
-	char boardname[100], fallback_boardname[100];
+	char boardname[100], fallback_boardname1[100], fallback_boardname2[100];
 	int ret;
 
 	if (bd_ie_type == ATH10K_BD_IE_BOARD) {
+		/* With variant and chip id */
 		ret = ath10k_core_create_board_name(ar, boardname,
-						    sizeof(boardname), true);
+						sizeof(boardname), true, true);
 		if (ret) {
 			ath10k_err(ar, "failed to create board name: %d", ret);
 			return ret;
 		}
 
-		ret = ath10k_core_create_board_name(ar, fallback_boardname,
-						    sizeof(boardname), false);
+		/* Without variant and only chip-id */
+		ret = ath10k_core_create_board_name(ar, fallback_boardname1,
+						    sizeof(boardname), false,
+						    true);
+		if (ret) {
+			ath10k_err(ar, "failed to create 1st fallback board name: %d", ret);
+			return ret;
+		}
+
+		/* Without variant and without chip-id */
+		ret = ath10k_core_create_board_name(ar, fallback_boardname2,
+						    sizeof(boardname), false,
+						    false);
 		if (ret) {
-			ath10k_err(ar, "failed to create fallback board name: %d", ret);
+			ath10k_err(ar, "failed to create 2nd fallback board name: %d", ret);
 			return ret;
 		}
 	} else if (bd_ie_type == ATH10K_BD_IE_BOARD_EXT) {
@@ -1504,7 +1526,8 @@ int ath10k_core_fetch_board_file(struct ath10k *ar, int bd_ie_type)
 
 	ar->bd_api = 2;
 	ret = ath10k_core_fetch_board_data_api_n(ar, boardname,
-						 fallback_boardname,
+						 fallback_boardname1,
+						 fallback_boardname2,
 						 ATH10K_BOARD_API2_FILE);
 	if (!ret)
 		goto success;
-- 
2.29.0.rc1.297.gfa9743e501-goog


_______________________________________________
ath10k mailing list
ath10k@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/ath10k

[PATCH] ath10k: add option for chip-id based BDF selection

[Abhishek Kumar]

In some devices difference in chip-id should be enough to pick
the right BDF. Add another support for chip-id based BDF selection.
With this new option, ath10k supports 2 fallback options.

The board name with chip-id as option looks as follows
board name 'bus=snoc,qmi-board-id=ff,qmi-chip-id=320'

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

 drivers/net/wireless/ath/ath10k/core.c | 45 +++++++++++++++++++-------
 1 file changed, 34 insertions(+), 11 deletions(-)

diff --git a/drivers/net/wireless/ath/ath10k/core.c b/drivers/net/wireless/ath/ath10k/core.c
index a3f15cc89a10..767bb9d6a197 100644
--- a/drivers/net/wireless/ath/ath10k/core.c
+++ b/drivers/net/wireless/ath/ath10k/core.c
@@ -1343,7 +1343,8 @@ static int ath10k_core_search_bd(struct ath10k *ar,
 
 static int ath10k_core_fetch_board_data_api_n(struct ath10k *ar,
 					      const char *boardname,
-					      const char *fallback_boardname,
+					      const char *fallback_boardname1,
+					      const char *fallback_boardname2,
 					      const char *filename)
 {
 	size_t len, magic_len;
@@ -1392,8 +1393,11 @@ static int ath10k_core_fetch_board_data_api_n(struct ath10k *ar,
 	ret = ath10k_core_search_bd(ar, boardname, data, len);
 
 	/* if we didn't find it and have a fallback name, try that */
-	if (ret == -ENOENT && fallback_boardname)
-		ret = ath10k_core_search_bd(ar, fallback_boardname, data, len);
+	if (ret == -ENOENT && fallback_boardname1)
+		ret = ath10k_core_search_bd(ar, fallback_boardname1, data, len);
+
+	if (ret == -ENOENT && fallback_boardname2)
+		ret = ath10k_core_search_bd(ar, fallback_boardname2, data, len);
 
 	if (ret == -ENOENT) {
 		ath10k_err(ar,
@@ -1413,7 +1417,8 @@ static int ath10k_core_fetch_board_data_api_n(struct ath10k *ar,
 }
 
 static int ath10k_core_create_board_name(struct ath10k *ar, char *name,
-					 size_t name_len, bool with_variant)
+					 size_t name_len, bool with_variant,
+					 bool with_chip_id)
 {
 	/* strlen(',variant=') + strlen(ar->id.bdf_ext) */
 	char variant[9 + ATH10K_SMBIOS_BDF_EXT_STR_LENGTH] = { 0 };
@@ -1432,12 +1437,17 @@ static int ath10k_core_create_board_name(struct ath10k *ar, char *name,
 	}
 
 	if (ar->id.qmi_ids_valid) {
-		if (with_variant && ar->id.bdf_ext[0] != '\0')
+		if (with_variant && ar->id.bdf_ext[0] != '\0' && with_chip_id)
 			scnprintf(name, name_len,
 				  "bus=%s,qmi-board-id=%x,qmi-chip-id=%x%s",
 				  ath10k_bus_str(ar->hif.bus),
 				  ar->id.qmi_board_id, ar->id.qmi_chip_id,
 				  variant);
+		else if (with_chip_id)
+			scnprintf(name, name_len,
+				  "bus=%s,qmi-board-id=%x,qmi-chip-id=%x",
+				  ath10k_bus_str(ar->hif.bus),
+				  ar->id.qmi_board_id, ar->id.qmi_chip_id);
 		else
 			scnprintf(name, name_len,
 				  "bus=%s,qmi-board-id=%x",
@@ -1476,21 +1486,33 @@ static int ath10k_core_create_eboard_name(struct ath10k *ar, char *name,
 
 int ath10k_core_fetch_board_file(struct ath10k *ar, int bd_ie_type)
 {
-	char boardname[100], fallback_boardname[100];
+	char boardname[100], fallback_boardname1[100], fallback_boardname2[100];
 	int ret;
 
 	if (bd_ie_type == ATH10K_BD_IE_BOARD) {
+		/* With variant and chip id */
 		ret = ath10k_core_create_board_name(ar, boardname,
-						    sizeof(boardname), true);
+						sizeof(boardname), true, true);
 		if (ret) {
 			ath10k_err(ar, "failed to create board name: %d", ret);
 			return ret;
 		}
 
-		ret = ath10k_core_create_board_name(ar, fallback_boardname,
-						    sizeof(boardname), false);
+		/* Without variant and only chip-id */
+		ret = ath10k_core_create_board_name(ar, fallback_boardname1,
+						    sizeof(boardname), false,
+						    true);
+		if (ret) {
+			ath10k_err(ar, "failed to create 1st fallback board name: %d", ret);
+			return ret;
+		}
+
+		/* Without variant and without chip-id */
+		ret = ath10k_core_create_board_name(ar, fallback_boardname2,
+						    sizeof(boardname), false,
+						    false);
 		if (ret) {
-			ath10k_err(ar, "failed to create fallback board name: %d", ret);
+			ath10k_err(ar, "failed to create 2nd fallback board name: %d", ret);
 			return ret;
 		}
 	} else if (bd_ie_type == ATH10K_BD_IE_BOARD_EXT) {
@@ -1504,7 +1526,8 @@ int ath10k_core_fetch_board_file(struct ath10k *ar, int bd_ie_type)
 
 	ar->bd_api = 2;
 	ret = ath10k_core_fetch_board_data_api_n(ar, boardname,
-						 fallback_boardname,
+						 fallback_boardname1,
+						 fallback_boardname2,
 						 ATH10K_BOARD_API2_FILE);
 	if (!ret)
 		goto success;
-- 
2.29.0.rc1.297.gfa9743e501-goog

Re: [PATCH] ath10k: add option for chip-id based BDF selection

[Doug Anderson]

Hi,

On Mon, Oct 19, 2020 at 5:51 PM Abhishek Kumar <kuabhs@chromium.org> wrote:
>
> In some devices difference in chip-id should be enough to pick
> the right BDF. Add another support for chip-id based BDF selection.
> With this new option, ath10k supports 2 fallback options.
>
> The board name with chip-id as option looks as follows
> board name 'bus=snoc,qmi-board-id=ff,qmi-chip-id=320'
>
> Signed-off-by: Abhishek Kumar <kuabhs@chromium.org>
> ---
>
>  drivers/net/wireless/ath/ath10k/core.c | 45 +++++++++++++++++++-------
>  1 file changed, 34 insertions(+), 11 deletions(-)

You might want to think a little more about who you're CCing when you
post a patch.  I would have imagined that Rakesh should have been
CCed.  Probably Brian and I, too?  Luckily I knew to look for it.


>  int ath10k_core_fetch_board_file(struct ath10k *ar, int bd_ie_type)
>  {
> -       char boardname[100], fallback_boardname[100];
> +       char boardname[100], fallback_boardname1[100], fallback_boardname2[100];
>         int ret;
>
>         if (bd_ie_type == ATH10K_BD_IE_BOARD) {
> +               /* With variant and chip id */
>                 ret = ath10k_core_create_board_name(ar, boardname,
> -                                                   sizeof(boardname), true);
> +                                               sizeof(boardname), true, true);

I don't know if it's worth spinning the patch just for this, but it's
weird that the indentation of the 2nd line above doesn't match the
indentation of the similar calls below.

Other than the tiny nit, this seems sane to me.

Reviewed-by: Douglas Anderson <dianders@chromium.org>

I've also confirmed that the patch is behaving properly on two boards
I have with slightly different chip-ids.  I tried poking with various
combinations of specifying a variant in dts and not specifying one.

Board 1:
qmi chip_id 0x320 chip_family 0x4001 board_id 0xff soc_id 0x400c0000

Board 2:
qmi chip_id 0x4320 chip_family 0x4001 board_id 0xff soc_id 0x400c0000

Both boards report "WCN3990 hw1.0 SNOC", so I guess the "Tested-on"
tag should be:

Tested-on: WCN3990 hw1.0 SNOC WLAN.HL.3.2.2-00696-QCAHLSWMTPL-1

Tested-by: Douglas Anderson <dianders@chromium.org>

-Doug

_______________________________________________
ath10k mailing list
ath10k@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/ath10k

Re: [PATCH] ath10k: add option for chip-id based BDF selection

[Doug Anderson]

Hi,

On Mon, Oct 19, 2020 at 5:51 PM Abhishek Kumar <kuabhs@chromium.org> wrote:
>
> In some devices difference in chip-id should be enough to pick
> the right BDF. Add another support for chip-id based BDF selection.
> With this new option, ath10k supports 2 fallback options.
>
> The board name with chip-id as option looks as follows
> board name 'bus=snoc,qmi-board-id=ff,qmi-chip-id=320'
>
> Signed-off-by: Abhishek Kumar <kuabhs@chromium.org>
> ---
>
>  drivers/net/wireless/ath/ath10k/core.c | 45 +++++++++++++++++++-------
>  1 file changed, 34 insertions(+), 11 deletions(-)

You might want to think a little more about who you're CCing when you
post a patch.  I would have imagined that Rakesh should have been
CCed.  Probably Brian and I, too?  Luckily I knew to look for it.


>  int ath10k_core_fetch_board_file(struct ath10k *ar, int bd_ie_type)
>  {
> -       char boardname[100], fallback_boardname[100];
> +       char boardname[100], fallback_boardname1[100], fallback_boardname2[100];
>         int ret;
>
>         if (bd_ie_type == ATH10K_BD_IE_BOARD) {
> +               /* With variant and chip id */
>                 ret = ath10k_core_create_board_name(ar, boardname,
> -                                                   sizeof(boardname), true);
> +                                               sizeof(boardname), true, true);

I don't know if it's worth spinning the patch just for this, but it's
weird that the indentation of the 2nd line above doesn't match the
indentation of the similar calls below.

Other than the tiny nit, this seems sane to me.

Reviewed-by: Douglas Anderson <dianders@chromium.org>

I've also confirmed that the patch is behaving properly on two boards
I have with slightly different chip-ids.  I tried poking with various
combinations of specifying a variant in dts and not specifying one.

Board 1:
qmi chip_id 0x320 chip_family 0x4001 board_id 0xff soc_id 0x400c0000

Board 2:
qmi chip_id 0x4320 chip_family 0x4001 board_id 0xff soc_id 0x400c0000

Both boards report "WCN3990 hw1.0 SNOC", so I guess the "Tested-on"
tag should be:

Tested-on: WCN3990 hw1.0 SNOC WLAN.HL.3.2.2-00696-QCAHLSWMTPL-1

Tested-by: Douglas Anderson <dianders@chromium.org>

-Doug

Re: [PATCH] ath10k: add option for chip-id based BDF selection

[Abhishek Kumar]

Additionally tested on ath10k based non-QMI platform

Tested-on: QCA6174 HW3.2 WLAN.RM.4.4.1-00157-QCARMSWPZ-1
Tested-by: Abhishek Kumar <kuabhs@chromium.org>

-Abhishek

_______________________________________________
ath10k mailing list
ath10k@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/ath10k

Re: [PATCH] ath10k: add option for chip-id based BDF selection

[Abhishek Kumar]

Additionally tested on ath10k based non-QMI platform

Tested-on: QCA6174 HW3.2 WLAN.RM.4.4.1-00157-QCARMSWPZ-1
Tested-by: Abhishek Kumar <kuabhs@chromium.org>

-Abhishek

Re: [PATCH] ath10k: add option for chip-id based BDF selection

[Doug Anderson]

Hi,

On Sat, Oct 24, 2020 at 9:40 AM Rakesh Pillai <pillair@codeaurora.org> wrote:
>
> >         if (bd_ie_type == ATH10K_BD_IE_BOARD) {
> > +               /* With variant and chip id */
> >                 ret = ath10k_core_create_board_name(ar, boardname,
> > -                                                   sizeof(boardname), true);
> > +                                               sizeof(boardname), true, true);
>
> Instead of adding a lot of code to generate a second fallback name, its better to just modify the condition inside the function “ath10k_core_create_board_name” to allow the generation of BDF tag using chip id, even “if ar->id.bdf_ext[0] == '\0 “.
>
> This will make sure that the variant string is NULL, and just board-id and chip-id is used. This will help avoid most of the code changes.
> The code would look as shown below
>
> @@ -1493,7 +1493,7 @@ static int ath10k_core_create_board_name(struct ath10k *ar, char *name,
>         }
>
>         if (ar->id.qmi_ids_valid) {
> -               if (with_variant && ar->id.bdf_ext[0] != '\0')
> +               if (with_variant)

Wouldn't the above just be "if (with_chip_id)" instead?  ...but yeah,
that would be a cleaner way to do this.  Abhishek: do you want to post
a v2?

-Doug

_______________________________________________
ath10k mailing list
ath10k@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/ath10k

Re: [PATCH] ath10k: add option for chip-id based BDF selection

[Doug Anderson]

Hi,

On Sat, Oct 24, 2020 at 9:40 AM Rakesh Pillai <pillair@codeaurora.org> wrote:
>
> >         if (bd_ie_type == ATH10K_BD_IE_BOARD) {
> > +               /* With variant and chip id */
> >                 ret = ath10k_core_create_board_name(ar, boardname,
> > -                                                   sizeof(boardname), true);
> > +                                               sizeof(boardname), true, true);
>
> Instead of adding a lot of code to generate a second fallback name, its better to just modify the condition inside the function “ath10k_core_create_board_name” to allow the generation of BDF tag using chip id, even “if ar->id.bdf_ext[0] == '\0 “.
>
> This will make sure that the variant string is NULL, and just board-id and chip-id is used. This will help avoid most of the code changes.
> The code would look as shown below
>
> @@ -1493,7 +1493,7 @@ static int ath10k_core_create_board_name(struct ath10k *ar, char *name,
>         }
>
>         if (ar->id.qmi_ids_valid) {
> -               if (with_variant && ar->id.bdf_ext[0] != '\0')
> +               if (with_variant)

Wouldn't the above just be "if (with_chip_id)" instead?  ...but yeah,
that would be a cleaner way to do this.  Abhishek: do you want to post
a v2?

-Doug

RE: [PATCH] ath10k: add option for chip-id based BDF selection

[Rakesh Pillai]

> -----Original Message-----
> From: Doug Anderson <dianders@chromium.org>
> Sent: Tuesday, October 27, 2020 4:21 AM
> To: Rakesh Pillai <pillair@codeaurora.org>
> Cc: Abhishek Kumar <kuabhs@chromium.org>; Kalle Valo
> <kvalo@codeaurora.org>; ath10k <ath10k@lists.infradead.org>; LKML
> <linux-kernel@vger.kernel.org>; linux-wireless <linux-
> wireless@vger.kernel.org>; Brian Norris <briannorris@chromium.org>
> Subject: Re: [PATCH] ath10k: add option for chip-id based BDF selection
> 
> Hi,
> 
> On Sat, Oct 24, 2020 at 9:40 AM Rakesh Pillai <pillair@codeaurora.org> wrote:
> >
> > >         if (bd_ie_type == ATH10K_BD_IE_BOARD) {
> > > +               /* With variant and chip id */
> > >                 ret = ath10k_core_create_board_name(ar, boardname,
> > > -                                                   sizeof(boardname), true);
> > > +                                               sizeof(boardname), true, true);
> >
> > Instead of adding a lot of code to generate a second fallback name, its
> better to just modify the condition inside the function
> “ath10k_core_create_board_name” to allow the generation of BDF tag using
> chip id, even “if ar->id.bdf_ext[0] == '\0 “.
> >
> > This will make sure that the variant string is NULL, and just board-id and
> chip-id is used. This will help avoid most of the code changes.
> > The code would look as shown below
> >
> > @@ -1493,7 +1493,7 @@ static int ath10k_core_create_board_name(struct
> ath10k *ar, char *name,
> >         }
> >
> >         if (ar->id.qmi_ids_valid) {
> > -               if (with_variant && ar->id.bdf_ext[0] != '\0')
> > +               if (with_variant)
> 
> Wouldn't the above just be "if (with_chip_id)" instead?  ...but yeah,
> that would be a cleaner way to do this.  Abhishek: do you want to post
> a v2?


The parameter name passed to this function is "with_variant", since other non-qmi targets (eg QCA6174) use this as a flag to just add the variant field.
This can be renamed to something meaningful for both qmi and non-qmi targets.

> 
> -Doug


_______________________________________________
ath10k mailing list
ath10k@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/ath10k

RE: [PATCH] ath10k: add option for chip-id based BDF selection

[Rakesh Pillai]

> -----Original Message-----
> From: Doug Anderson <dianders@chromium.org>
> Sent: Tuesday, October 27, 2020 4:21 AM
> To: Rakesh Pillai <pillair@codeaurora.org>
> Cc: Abhishek Kumar <kuabhs@chromium.org>; Kalle Valo
> <kvalo@codeaurora.org>; ath10k <ath10k@lists.infradead.org>; LKML
> <linux-kernel@vger.kernel.org>; linux-wireless <linux-
> wireless@vger.kernel.org>; Brian Norris <briannorris@chromium.org>
> Subject: Re: [PATCH] ath10k: add option for chip-id based BDF selection
> 
> Hi,
> 
> On Sat, Oct 24, 2020 at 9:40 AM Rakesh Pillai <pillair@codeaurora.org> wrote:
> >
> > >         if (bd_ie_type == ATH10K_BD_IE_BOARD) {
> > > +               /* With variant and chip id */
> > >                 ret = ath10k_core_create_board_name(ar, boardname,
> > > -                                                   sizeof(boardname), true);
> > > +                                               sizeof(boardname), true, true);
> >
> > Instead of adding a lot of code to generate a second fallback name, its
> better to just modify the condition inside the function
> “ath10k_core_create_board_name” to allow the generation of BDF tag using
> chip id, even “if ar->id.bdf_ext[0] == '\0 “.
> >
> > This will make sure that the variant string is NULL, and just board-id and
> chip-id is used. This will help avoid most of the code changes.
> > The code would look as shown below
> >
> > @@ -1493,7 +1493,7 @@ static int ath10k_core_create_board_name(struct
> ath10k *ar, char *name,
> >         }
> >
> >         if (ar->id.qmi_ids_valid) {
> > -               if (with_variant && ar->id.bdf_ext[0] != '\0')
> > +               if (with_variant)
> 
> Wouldn't the above just be "if (with_chip_id)" instead?  ...but yeah,
> that would be a cleaner way to do this.  Abhishek: do you want to post
> a v2?


The parameter name passed to this function is "with_variant", since other non-qmi targets (eg QCA6174) use this as a flag to just add the variant field.
This can be renamed to something meaningful for both qmi and non-qmi targets.

> 
> -Doug

Re: [PATCH] ath10k: add option for chip-id based BDF selection

[Doug Anderson]

Hi,

On Mon, Oct 26, 2020 at 10:18 PM Rakesh Pillai <pillair@codeaurora.org> wrote:
>
>
>
> > -----Original Message-----
> > From: Doug Anderson <dianders@chromium.org>
> > Sent: Tuesday, October 27, 2020 4:21 AM
> > To: Rakesh Pillai <pillair@codeaurora.org>
> > Cc: Abhishek Kumar <kuabhs@chromium.org>; Kalle Valo
> > <kvalo@codeaurora.org>; ath10k <ath10k@lists.infradead.org>; LKML
> > <linux-kernel@vger.kernel.org>; linux-wireless <linux-
> > wireless@vger.kernel.org>; Brian Norris <briannorris@chromium.org>
> > Subject: Re: [PATCH] ath10k: add option for chip-id based BDF selection
> >
> > Hi,
> >
> > On Sat, Oct 24, 2020 at 9:40 AM Rakesh Pillai <pillair@codeaurora.org> wrote:
> > >
> > > >         if (bd_ie_type == ATH10K_BD_IE_BOARD) {
> > > > +               /* With variant and chip id */
> > > >                 ret = ath10k_core_create_board_name(ar, boardname,
> > > > -                                                   sizeof(boardname), true);
> > > > +                                               sizeof(boardname), true, true);
> > >
> > > Instead of adding a lot of code to generate a second fallback name, its
> > better to just modify the condition inside the function
> > “ath10k_core_create_board_name” to allow the generation of BDF tag using
> > chip id, even “if ar->id.bdf_ext[0] == '\0 “.
> > >
> > > This will make sure that the variant string is NULL, and just board-id and
> > chip-id is used. This will help avoid most of the code changes.
> > > The code would look as shown below
> > >
> > > @@ -1493,7 +1493,7 @@ static int ath10k_core_create_board_name(struct
> > ath10k *ar, char *name,
> > >         }
> > >
> > >         if (ar->id.qmi_ids_valid) {
> > > -               if (with_variant && ar->id.bdf_ext[0] != '\0')
> > > +               if (with_variant)
> >
> > Wouldn't the above just be "if (with_chip_id)" instead?  ...but yeah,
> > that would be a cleaner way to do this.  Abhishek: do you want to post
> > a v2?
>
>
> The parameter name passed to this function is "with_variant", since other non-qmi targets (eg QCA6174) use this as a flag to just add the variant field.
> This can be renamed to something meaningful for both qmi and non-qmi targets.

I think we still need Abhishek's change to have two booleans passed to
this function, though, right?  Thus, it'll be called 3 times:

* with_chip_id = false, with_variant = false
* with_chip_id = true, with_variant = true
* with_chip_id = true, with_variant = false

The two cases you want to combine are both with "with_chip_id = true",
right?  The "with_variant" variable being false will make the variant
string empty.

-Doug

_______________________________________________
ath10k mailing list
ath10k@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/ath10k

Re: [PATCH] ath10k: add option for chip-id based BDF selection

[Doug Anderson]

Hi,

On Mon, Oct 26, 2020 at 10:18 PM Rakesh Pillai <pillair@codeaurora.org> wrote:
>
>
>
> > -----Original Message-----
> > From: Doug Anderson <dianders@chromium.org>
> > Sent: Tuesday, October 27, 2020 4:21 AM
> > To: Rakesh Pillai <pillair@codeaurora.org>
> > Cc: Abhishek Kumar <kuabhs@chromium.org>; Kalle Valo
> > <kvalo@codeaurora.org>; ath10k <ath10k@lists.infradead.org>; LKML
> > <linux-kernel@vger.kernel.org>; linux-wireless <linux-
> > wireless@vger.kernel.org>; Brian Norris <briannorris@chromium.org>
> > Subject: Re: [PATCH] ath10k: add option for chip-id based BDF selection
> >
> > Hi,
> >
> > On Sat, Oct 24, 2020 at 9:40 AM Rakesh Pillai <pillair@codeaurora.org> wrote:
> > >
> > > >         if (bd_ie_type == ATH10K_BD_IE_BOARD) {
> > > > +               /* With variant and chip id */
> > > >                 ret = ath10k_core_create_board_name(ar, boardname,
> > > > -                                                   sizeof(boardname), true);
> > > > +                                               sizeof(boardname), true, true);
> > >
> > > Instead of adding a lot of code to generate a second fallback name, its
> > better to just modify the condition inside the function
> > “ath10k_core_create_board_name” to allow the generation of BDF tag using
> > chip id, even “if ar->id.bdf_ext[0] == '\0 “.
> > >
> > > This will make sure that the variant string is NULL, and just board-id and
> > chip-id is used. This will help avoid most of the code changes.
> > > The code would look as shown below
> > >
> > > @@ -1493,7 +1493,7 @@ static int ath10k_core_create_board_name(struct
> > ath10k *ar, char *name,
> > >         }
> > >
> > >         if (ar->id.qmi_ids_valid) {
> > > -               if (with_variant && ar->id.bdf_ext[0] != '\0')
> > > +               if (with_variant)
> >
> > Wouldn't the above just be "if (with_chip_id)" instead?  ...but yeah,
> > that would be a cleaner way to do this.  Abhishek: do you want to post
> > a v2?
>
>
> The parameter name passed to this function is "with_variant", since other non-qmi targets (eg QCA6174) use this as a flag to just add the variant field.
> This can be renamed to something meaningful for both qmi and non-qmi targets.

I think we still need Abhishek's change to have two booleans passed to
this function, though, right?  Thus, it'll be called 3 times:

* with_chip_id = false, with_variant = false
* with_chip_id = true, with_variant = true
* with_chip_id = true, with_variant = false

The two cases you want to combine are both with "with_chip_id = true",
right?  The "with_variant" variable being false will make the variant
string empty.

-Doug

RE: [PATCH] ath10k: add option for chip-id based BDF selection

[Rakesh Pillai]

> -----Original Message-----
> From: Doug Anderson <dianders@chromium.org>
> Sent: Tuesday, October 27, 2020 8:26 PM
> To: Rakesh Pillai <pillair@codeaurora.org>
> Cc: Abhishek Kumar <kuabhs@chromium.org>; Kalle Valo
> <kvalo@codeaurora.org>; ath10k <ath10k@lists.infradead.org>; LKML
> <linux-kernel@vger.kernel.org>; linux-wireless <linux-
> wireless@vger.kernel.org>; Brian Norris <briannorris@chromium.org>
> Subject: Re: [PATCH] ath10k: add option for chip-id based BDF selection
> 
> Hi,
> 
> On Mon, Oct 26, 2020 at 10:18 PM Rakesh Pillai <pillair@codeaurora.org>
> wrote:
> >
> >
> >
> > > -----Original Message-----
> > > From: Doug Anderson <dianders@chromium.org>
> > > Sent: Tuesday, October 27, 2020 4:21 AM
> > > To: Rakesh Pillai <pillair@codeaurora.org>
> > > Cc: Abhishek Kumar <kuabhs@chromium.org>; Kalle Valo
> > > <kvalo@codeaurora.org>; ath10k <ath10k@lists.infradead.org>; LKML
> > > <linux-kernel@vger.kernel.org>; linux-wireless <linux-
> > > wireless@vger.kernel.org>; Brian Norris <briannorris@chromium.org>
> > > Subject: Re: [PATCH] ath10k: add option for chip-id based BDF selection
> > >
> > > Hi,
> > >
> > > On Sat, Oct 24, 2020 at 9:40 AM Rakesh Pillai <pillair@codeaurora.org>
> wrote:
> > > >
> > > > >         if (bd_ie_type == ATH10K_BD_IE_BOARD) {
> > > > > +               /* With variant and chip id */
> > > > >                 ret = ath10k_core_create_board_name(ar, boardname,
> > > > > -                                                   sizeof(boardname), true);
> > > > > +                                               sizeof(boardname), true, true);
> > > >
> > > > Instead of adding a lot of code to generate a second fallback name, its
> > > better to just modify the condition inside the function
> > > “ath10k_core_create_board_name” to allow the generation of BDF tag
> using
> > > chip id, even “if ar->id.bdf_ext[0] == '\0 “.
> > > >
> > > > This will make sure that the variant string is NULL, and just board-id and
> > > chip-id is used. This will help avoid most of the code changes.
> > > > The code would look as shown below
> > > >
> > > > @@ -1493,7 +1493,7 @@ static int
> ath10k_core_create_board_name(struct
> > > ath10k *ar, char *name,
> > > >         }
> > > >
> > > >         if (ar->id.qmi_ids_valid) {
> > > > -               if (with_variant && ar->id.bdf_ext[0] != '\0')
> > > > +               if (with_variant)
> > >
> > > Wouldn't the above just be "if (with_chip_id)" instead?  ...but yeah,
> > > that would be a cleaner way to do this.  Abhishek: do you want to post
> > > a v2?
> >
> >
> > The parameter name passed to this function is "with_variant", since other
> non-qmi targets (eg QCA6174) use this as a flag to just add the variant field.
> > This can be renamed to something meaningful for both qmi and non-qmi
> targets.
> 
> I think we still need Abhishek's change to have two booleans passed to
> this function, though, right?  Thus, it'll be called 3 times:
> 
> * with_chip_id = false, with_variant = false
> * with_chip_id = true, with_variant = true
> * with_chip_id = true, with_variant = false
> 
> The two cases you want to combine are both with "with_chip_id = true",
> right?  The "with_variant" variable being false will make the variant
> string empty.


I meant that we can use the 4th argument passed to the function " ath10k_core_create_board_name" (currently named as with_variant) as an indication to use the BDF name with variant.
But even if with_variant=true, we allow the variant string to be NULL, thereby allowing us to generate a boardname with the format "bus=snoc,qmi-board-id=0xab,qmi-chip-id=0xcd"

The combinations of args/variant-strings for generating different board names will be as follows:
1) with_variant=false                                                     :      "bus=snoc,qmi-board-id=0xab"
2) with_variant=true, variant_string=NULL                :     " bus=snoc,qmi-board-id=0xab,qmi-chip-id=0xcd"
3) with_variant=true, variant_string="variant_xyz"  :     " bus=snoc,qmi-board-id=0xab,qmi-chip-id=0xcd,variant=variant_xyz"


This will minimize the code changes.

> 
> -Doug


_______________________________________________
ath10k mailing list
ath10k@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/ath10k

RE: [PATCH] ath10k: add option for chip-id based BDF selection

[Rakesh Pillai]

> -----Original Message-----
> From: Doug Anderson <dianders@chromium.org>
> Sent: Tuesday, October 27, 2020 8:26 PM
> To: Rakesh Pillai <pillair@codeaurora.org>
> Cc: Abhishek Kumar <kuabhs@chromium.org>; Kalle Valo
> <kvalo@codeaurora.org>; ath10k <ath10k@lists.infradead.org>; LKML
> <linux-kernel@vger.kernel.org>; linux-wireless <linux-
> wireless@vger.kernel.org>; Brian Norris <briannorris@chromium.org>
> Subject: Re: [PATCH] ath10k: add option for chip-id based BDF selection
> 
> Hi,
> 
> On Mon, Oct 26, 2020 at 10:18 PM Rakesh Pillai <pillair@codeaurora.org>
> wrote:
> >
> >
> >
> > > -----Original Message-----
> > > From: Doug Anderson <dianders@chromium.org>
> > > Sent: Tuesday, October 27, 2020 4:21 AM
> > > To: Rakesh Pillai <pillair@codeaurora.org>
> > > Cc: Abhishek Kumar <kuabhs@chromium.org>; Kalle Valo
> > > <kvalo@codeaurora.org>; ath10k <ath10k@lists.infradead.org>; LKML
> > > <linux-kernel@vger.kernel.org>; linux-wireless <linux-
> > > wireless@vger.kernel.org>; Brian Norris <briannorris@chromium.org>
> > > Subject: Re: [PATCH] ath10k: add option for chip-id based BDF selection
> > >
> > > Hi,
> > >
> > > On Sat, Oct 24, 2020 at 9:40 AM Rakesh Pillai <pillair@codeaurora.org>
> wrote:
> > > >
> > > > >         if (bd_ie_type == ATH10K_BD_IE_BOARD) {
> > > > > +               /* With variant and chip id */
> > > > >                 ret = ath10k_core_create_board_name(ar, boardname,
> > > > > -                                                   sizeof(boardname), true);
> > > > > +                                               sizeof(boardname), true, true);
> > > >
> > > > Instead of adding a lot of code to generate a second fallback name, its
> > > better to just modify the condition inside the function
> > > “ath10k_core_create_board_name” to allow the generation of BDF tag
> using
> > > chip id, even “if ar->id.bdf_ext[0] == '\0 “.
> > > >
> > > > This will make sure that the variant string is NULL, and just board-id and
> > > chip-id is used. This will help avoid most of the code changes.
> > > > The code would look as shown below
> > > >
> > > > @@ -1493,7 +1493,7 @@ static int
> ath10k_core_create_board_name(struct
> > > ath10k *ar, char *name,
> > > >         }
> > > >
> > > >         if (ar->id.qmi_ids_valid) {
> > > > -               if (with_variant && ar->id.bdf_ext[0] != '\0')
> > > > +               if (with_variant)
> > >
> > > Wouldn't the above just be "if (with_chip_id)" instead?  ...but yeah,
> > > that would be a cleaner way to do this.  Abhishek: do you want to post
> > > a v2?
> >
> >
> > The parameter name passed to this function is "with_variant", since other
> non-qmi targets (eg QCA6174) use this as a flag to just add the variant field.
> > This can be renamed to something meaningful for both qmi and non-qmi
> targets.
> 
> I think we still need Abhishek's change to have two booleans passed to
> this function, though, right?  Thus, it'll be called 3 times:
> 
> * with_chip_id = false, with_variant = false
> * with_chip_id = true, with_variant = true
> * with_chip_id = true, with_variant = false
> 
> The two cases you want to combine are both with "with_chip_id = true",
> right?  The "with_variant" variable being false will make the variant
> string empty.


I meant that we can use the 4th argument passed to the function " ath10k_core_create_board_name" (currently named as with_variant) as an indication to use the BDF name with variant.
But even if with_variant=true, we allow the variant string to be NULL, thereby allowing us to generate a boardname with the format "bus=snoc,qmi-board-id=0xab,qmi-chip-id=0xcd"

The combinations of args/variant-strings for generating different board names will be as follows:
1) with_variant=false                                                     :      "bus=snoc,qmi-board-id=0xab"
2) with_variant=true, variant_string=NULL                :     " bus=snoc,qmi-board-id=0xab,qmi-chip-id=0xcd"
3) with_variant=true, variant_string="variant_xyz"  :     " bus=snoc,qmi-board-id=0xab,qmi-chip-id=0xcd,variant=variant_xyz"


This will minimize the code changes.

> 
> -Doug

Re: [PATCH] ath10k: add option for chip-id based BDF selection

[Kalle Valo]

Abhishek Kumar <kuabhs@chromium.org> wrote:

> In some devices difference in chip-id should be enough to pick
> the right BDF. Add another support for chip-id based BDF selection.
> With this new option, ath10k supports 2 fallback options.
> 
> The board name with chip-id as option looks as follows
> board name 'bus=snoc,qmi-board-id=ff,qmi-chip-id=320'
> 
> Signed-off-by: Abhishek Kumar <kuabhs@chromium.org>
> Reviewed-by: Douglas Anderson <dianders@chromium.org>
> Tested-by: Douglas Anderson <dianders@chromium.org>
> Tested-by: Abhishek Kumar <kuabhs@chromium.org>

There were few checkpatch warnings which I fixed:

$ ath10k-check
drivers/net/wireless/ath/ath10k/core.c:1501: Alignment should match open parenthesis
drivers/net/wireless/ath/ath10k/core.c:1512: line length of 92 exceeds 90 columns
drivers/net/wireless/ath/ath10k/core.c:1521: line length of 92 exceeds 90 columns

The first one was also what Doug commented. I also added Tested-on tags,
thanks for those. The updated patch is in pending branch (soon).

But is this patch ok to take now? I didn't quite get the conclusion of the
discussion.

-- 
https://patchwork.kernel.org/project/linux-wireless/patch/20201020000506.1.Ifbc28707942179f1cefc7491e995814564495270@changeid/

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

Re: [PATCH] ath10k: add option for chip-id based BDF selection

[Abhishek Kumar]

Apologies for the delay, was busy so could not work on V2 . I have
started working on V2 patch. Will upload by today/tomorrow.

Abhishek


On Thu, Nov 5, 2020 at 11:11 PM Kalle Valo <kvalo@codeaurora.org> wrote:
>
> Abhishek Kumar <kuabhs@chromium.org> wrote:
>
> > In some devices difference in chip-id should be enough to pick
> > the right BDF. Add another support for chip-id based BDF selection.
> > With this new option, ath10k supports 2 fallback options.
> >
> > The board name with chip-id as option looks as follows
> > board name 'bus=snoc,qmi-board-id=ff,qmi-chip-id=320'
> >
> > Signed-off-by: Abhishek Kumar <kuabhs@chromium.org>
> > Reviewed-by: Douglas Anderson <dianders@chromium.org>
> > Tested-by: Douglas Anderson <dianders@chromium.org>
> > Tested-by: Abhishek Kumar <kuabhs@chromium.org>
>
> There were few checkpatch warnings which I fixed:
>
> $ ath10k-check
> drivers/net/wireless/ath/ath10k/core.c:1501: Alignment should match open parenthesis
> drivers/net/wireless/ath/ath10k/core.c:1512: line length of 92 exceeds 90 columns
> drivers/net/wireless/ath/ath10k/core.c:1521: line length of 92 exceeds 90 columns
>
> The first one was also what Doug commented. I also added Tested-on tags,
> thanks for those. The updated patch is in pending branch (soon).
>
> But is this patch ok to take now? I didn't quite get the conclusion of the
> discussion.
>
> --
> https://patchwork.kernel.org/project/linux-wireless/patch/20201020000506.1.Ifbc28707942179f1cefc7491e995814564495270@changeid/
>
> https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches
>

_______________________________________________
ath10k mailing list
ath10k@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/ath10k

Re: [PATCH] ath10k: add option for chip-id based BDF selection

[Abhishek Kumar]

Apologies for the delay, was busy so could not work on V2 . I have
started working on V2 patch. Will upload by today/tomorrow.

Abhishek


On Thu, Nov 5, 2020 at 11:11 PM Kalle Valo <kvalo@codeaurora.org> wrote:
>
> Abhishek Kumar <kuabhs@chromium.org> wrote:
>
> > In some devices difference in chip-id should be enough to pick
> > the right BDF. Add another support for chip-id based BDF selection.
> > With this new option, ath10k supports 2 fallback options.
> >
> > The board name with chip-id as option looks as follows
> > board name 'bus=snoc,qmi-board-id=ff,qmi-chip-id=320'
> >
> > Signed-off-by: Abhishek Kumar <kuabhs@chromium.org>
> > Reviewed-by: Douglas Anderson <dianders@chromium.org>
> > Tested-by: Douglas Anderson <dianders@chromium.org>
> > Tested-by: Abhishek Kumar <kuabhs@chromium.org>
>
> There were few checkpatch warnings which I fixed:
>
> $ ath10k-check
> drivers/net/wireless/ath/ath10k/core.c:1501: Alignment should match open parenthesis
> drivers/net/wireless/ath/ath10k/core.c:1512: line length of 92 exceeds 90 columns
> drivers/net/wireless/ath/ath10k/core.c:1521: line length of 92 exceeds 90 columns
>
> The first one was also what Doug commented. I also added Tested-on tags,
> thanks for those. The updated patch is in pending branch (soon).
>
> But is this patch ok to take now? I didn't quite get the conclusion of the
> discussion.
>
> --
> https://patchwork.kernel.org/project/linux-wireless/patch/20201020000506.1.Ifbc28707942179f1cefc7491e995814564495270@changeid/
>
> https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches
>

Re: [PATCH] ath10k: add option for chip-id based BDF selection

[Kalle Valo]

Abhishek Kumar <kuabhs@chromium.org> writes:

> Apologies for the delay, was busy so could not work on V2 . I have
> started working on V2 patch. Will upload by today/tomorrow.

Thanks, I'll then drop this v1.

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

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

_______________________________________________
ath10k mailing list
ath10k@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/ath10k

Re: [PATCH] ath10k: add option for chip-id based BDF selection

[Kalle Valo]

Abhishek Kumar <kuabhs@chromium.org> writes:

> Apologies for the delay, was busy so could not work on V2 . I have
> started working on V2 patch. Will upload by today/tomorrow.

Thanks, I'll then drop this v1.

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

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

Re: [PATCH] ath10k: add option for chip-id based BDF selection

[Kalle Valo]

Abhishek Kumar <kuabhs@chromium.org> wrote:

> In some devices difference in chip-id should be enough to pick
> the right BDF. Add another support for chip-id based BDF selection.
> With this new option, ath10k supports 2 fallback options.
> 
> The board name with chip-id as option looks as follows
> board name 'bus=snoc,qmi-board-id=ff,qmi-chip-id=320'
> 
> Signed-off-by: Abhishek Kumar <kuabhs@chromium.org>
> Reviewed-by: Douglas Anderson <dianders@chromium.org>
> Tested-by: Douglas Anderson <dianders@chromium.org>
> Tested-by: Abhishek Kumar <kuabhs@chromium.org>

There were few checkpatch warnings which I fixed:

$ ath10k-check
drivers/net/wireless/ath/ath10k/core.c:1501: Alignment should match open parenthesis
drivers/net/wireless/ath/ath10k/core.c:1512: line length of 92 exceeds 90 columns
drivers/net/wireless/ath/ath10k/core.c:1521: line length of 92 exceeds 90 columns

The first one was also what Doug commented. I also added Tested-on tags,
thanks for those. The updated patch is in pending branch (soon).

But is this patch ok to take now? I didn't quite get the conclusion of the
discussion.

-- 
https://patchwork.kernel.org/project/linux-wireless/patch/20201020000506.1.Ifbc28707942179f1cefc7491e995814564495270@changeid/

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


_______________________________________________
ath10k mailing list
ath10k@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/ath10k