| Message ID | 1593193990-30366-1-git-send-email-pillair@codeaurora.org (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Kalle Valo |
| Headers | show |
| Series | ath10k: Use bdf calibration variant for snoc targets | expand |
Rakesh Pillai <pillair@codeaurora.org> writes: > Board Data File (BDF) is loaded upon driver boot-up procedure. > The right board data file is identified using bus and qmi-board-id. > > The problem, however, can occur when the (default) board data > file cannot fulfill with the vendor requirements and it is > necessary to use a different board data file. > > Add the support to get the variant field from DTSI and > use tht information to load the vendor specific BDF. > > The device tree requires addition strings to define the variant name > > wifi@a000000 { > status = "okay"; > qcom,ath10k-calibration-variant = "xyz-v2"; > }; > > wifi@a800000 { > status = "okay"; > qcom,ath10k-calibration-variant = "xyz-v1"; > }; > > This would create the boarddata identifiers for the board-2.bin search > > * bus=snoc,qmi-board-id=16,qmi-chip-id=0,variant=xyz-v1 > * bus=snoc,qmi-board-id=17,qmi-chip-id=0,variant=xyz-v2 You mention nothing about qmi-chip-id in the commit log. Please document what it is and also give some examples what kind of values there can be. > --- a/drivers/net/wireless/ath/ath10k/qmi.c > +++ b/drivers/net/wireless/ath/ath10k/qmi.c > @@ -576,6 +576,8 @@ static int ath10k_qmi_cap_send_sync_msg(struct ath10k_qmi *qmi) > if (resp->chip_info_valid) { > qmi->chip_info.chip_id = resp->chip_info.chip_id; > qmi->chip_info.chip_family = resp->chip_info.chip_family; > + } else { > + qmi->chip_info.chip_id = 0xFF; > } So you hard code chip_id to 0xff if it's not valid. Is it 100% guaranteed that there never will be a chip id with 0xff? > > if (resp->board_info_valid) > @@ -817,12 +819,18 @@ static void ath10k_qmi_event_server_arrive(struct ath10k_qmi *qmi) > static int ath10k_qmi_fetch_board_file(struct ath10k_qmi *qmi) > { > struct ath10k *ar = qmi->ar; > + int ret; > > ar->hif.bus = ATH10K_BUS_SNOC; > ar->id.qmi_ids_valid = true; > ar->id.qmi_board_id = qmi->board_info.board_id; > + ar->id.qmi_chip_id = qmi->chip_info.chip_id; To me a safer, and cleaner, option would be to have ar->id.qmi_chip_id_valid, and only add qmi-chip-id=%x to the board id if qmi_chip_id_valid is true. That way there's not this magic 0xff value hardcoded anywhere.
Hi Kalle, > -----Original Message----- > From: Kalle Valo <kvalo@codeaurora.org> > Sent: Wednesday, September 2, 2020 2:17 PM > To: Rakesh Pillai <pillair@codeaurora.org> > Cc: ath10k@lists.infradead.org; linux-wireless@vger.kernel.org; linux- > kernel@vger.kernel.org > Subject: Re: [PATCH] ath10k: Use bdf calibration variant for snoc targets > > Rakesh Pillai <pillair@codeaurora.org> writes: > > > Board Data File (BDF) is loaded upon driver boot-up procedure. > > The right board data file is identified using bus and qmi-board-id. > > > > The problem, however, can occur when the (default) board data > > file cannot fulfill with the vendor requirements and it is > > necessary to use a different board data file. > > > > Add the support to get the variant field from DTSI and > > use tht information to load the vendor specific BDF. > > > > The device tree requires addition strings to define the variant name > > > > wifi@a000000 { > > status = "okay"; > > qcom,ath10k-calibration-variant = "xyz-v2"; > > }; > > > > wifi@a800000 { > > status = "okay"; > > qcom,ath10k-calibration-variant = "xyz-v1"; > > }; > > > > This would create the boarddata identifiers for the board-2.bin search > > > > * bus=snoc,qmi-board-id=16,qmi-chip-id=0,variant=xyz-v1 > > * bus=snoc,qmi-board-id=17,qmi-chip-id=0,variant=xyz-v2 > > You mention nothing about qmi-chip-id in the commit log. Please document > what it is and also give some examples what kind of values there can be. Let me add a bit more details about the chip-id and send v2 for this change. > > > --- a/drivers/net/wireless/ath/ath10k/qmi.c > > +++ b/drivers/net/wireless/ath/ath10k/qmi.c > > @@ -576,6 +576,8 @@ static int ath10k_qmi_cap_send_sync_msg(struct > ath10k_qmi *qmi) > > if (resp->chip_info_valid) { > > qmi->chip_info.chip_id = resp->chip_info.chip_id; > > qmi->chip_info.chip_family = resp->chip_info.chip_family; > > + } else { > > + qmi->chip_info.chip_id = 0xFF; > > } > > So you hard code chip_id to 0xff if it's not valid. Is it 100% > guaranteed that there never will be a chip id with 0xff? 0x0 and 0xff are invalid chip id and are are not used. If the chip_id read fails, we fallback to the default board data. 0xff is used to go to the default board data (Also this is in alignment with the current implementation of board_id) Does that make sense ? > > > > > if (resp->board_info_valid) > > @@ -817,12 +819,18 @@ static void > ath10k_qmi_event_server_arrive(struct ath10k_qmi *qmi) > > static int ath10k_qmi_fetch_board_file(struct ath10k_qmi *qmi) > > { > > struct ath10k *ar = qmi->ar; > > + int ret; > > > > ar->hif.bus = ATH10K_BUS_SNOC; > > ar->id.qmi_ids_valid = true; > > ar->id.qmi_board_id = qmi->board_info.board_id; > > + ar->id.qmi_chip_id = qmi->chip_info.chip_id; > > To me a safer, and cleaner, option would be to have > ar->id.qmi_chip_id_valid, and only add qmi-chip-id=%x to the board id if > qmi_chip_id_valid is true. That way there's not this magic 0xff value > hardcoded anywhere. > > -- > https://wireless.wiki.kernel.org/en/developers/documentation/submittingp > atches
"Rakesh Pillai" <pillair@codeaurora.org> writes: >> > --- a/drivers/net/wireless/ath/ath10k/qmi.c >> > +++ b/drivers/net/wireless/ath/ath10k/qmi.c >> > @@ -576,6 +576,8 @@ static int ath10k_qmi_cap_send_sync_msg(struct >> ath10k_qmi *qmi) >> > if (resp->chip_info_valid) { >> > qmi->chip_info.chip_id = resp->chip_info.chip_id; >> > qmi->chip_info.chip_family = resp->chip_info.chip_family; >> > + } else { >> > + qmi->chip_info.chip_id = 0xFF; >> > } >> >> So you hard code chip_id to 0xff if it's not valid. Is it 100% >> guaranteed that there never will be a chip id with 0xff? > > 0x0 and 0xff are invalid chip id and are are not used. > If the chip_id read fails, we fallback to the default board data. > 0xff is used to go to the default board data (Also this is in alignment with > the current implementation of board_id) > > Does that make sense ? I'm a bit hesitant, over the years I have seen cases so many cases where "foo is not used anywhere" and later that foo is actually used somewhere. 0xff means that there's only 254 different values, but I guess there are not that many chip ids? So a chip id is very different from a board id, right?
> -----Original Message----- > From: Kalle Valo <kvalo@codeaurora.org> > Sent: Monday, September 7, 2020 9:48 PM > To: Rakesh Pillai <pillair@codeaurora.org> > Cc: linux-wireless@vger.kernel.org; linux-kernel@vger.kernel.org; > ath10k@lists.infradead.org > Subject: Re: [PATCH] ath10k: Use bdf calibration variant for snoc targets > > "Rakesh Pillai" <pillair@codeaurora.org> writes: > > >> > --- a/drivers/net/wireless/ath/ath10k/qmi.c > >> > +++ b/drivers/net/wireless/ath/ath10k/qmi.c > >> > @@ -576,6 +576,8 @@ static int > ath10k_qmi_cap_send_sync_msg(struct > >> ath10k_qmi *qmi) > >> > if (resp->chip_info_valid) { > >> > qmi->chip_info.chip_id = resp->chip_info.chip_id; > >> > qmi->chip_info.chip_family = resp->chip_info.chip_family; > >> > + } else { > >> > + qmi->chip_info.chip_id = 0xFF; > >> > } > >> > >> So you hard code chip_id to 0xff if it's not valid. Is it 100% > >> guaranteed that there never will be a chip id with 0xff? > > > > 0x0 and 0xff are invalid chip id and are are not used. > > If the chip_id read fails, we fallback to the default board data. > > 0xff is used to go to the default board data (Also this is in alignment with > > the current implementation of board_id) > > > > Does that make sense ? > > I'm a bit hesitant, over the years I have seen cases so many cases where > "foo is not used anywhere" and later that foo is actually used > somewhere. 0xff means that there's only 254 different values, but I > guess there are not that many chip ids? So a chip id is very different > from a board id, right? Yes that's correct. Chip id is already being used in case of qca6174 (pci bus) We are bringing this to snoc bus chipset (WCN3990) > > -- > https://wireless.wiki.kernel.org/en/developers/documentation/submittingp > atches
diff --git a/drivers/net/wireless/ath/ath10k/core.c b/drivers/net/wireless/ath/ath10k/core.c index 22b6937..bd60913 100644 --- a/drivers/net/wireless/ath/ath10k/core.c +++ b/drivers/net/wireless/ath/ath10k/core.c @@ -1024,7 +1024,7 @@ static int ath10k_core_check_smbios(struct ath10k *ar) return 0; } -static int ath10k_core_check_dt(struct ath10k *ar) +int ath10k_core_check_dt(struct ath10k *ar) { struct device_node *node; const char *variant = NULL; @@ -1045,6 +1045,7 @@ static int ath10k_core_check_dt(struct ath10k *ar) return 0; } +EXPORT_SYMBOL(ath10k_core_check_dt); static int ath10k_download_fw(struct ath10k *ar) { @@ -1439,10 +1440,17 @@ static int ath10k_core_create_board_name(struct ath10k *ar, char *name, } if (ar->id.qmi_ids_valid) { - scnprintf(name, name_len, - "bus=%s,qmi-board-id=%x", - ath10k_bus_str(ar->hif.bus), - ar->id.qmi_board_id); + if (with_variant && ar->id.bdf_ext[0] != '\0') + scnprintf(name, name_len, + "bus=%s,qmi-board-id=%x,qmi-chip-id=%x%s", + ath10k_bus_str(ar->hif.bus), + ar->id.qmi_board_id, ar->id.qmi_chip_id, + variant); + else + scnprintf(name, name_len, + "bus=%s,qmi-board-id=%x", + ath10k_bus_str(ar->hif.bus), + ar->id.qmi_board_id); goto out; } diff --git a/drivers/net/wireless/ath/ath10k/core.h b/drivers/net/wireless/ath/ath10k/core.h index 5c18f6c..62b1502 100644 --- a/drivers/net/wireless/ath/ath10k/core.h +++ b/drivers/net/wireless/ath/ath10k/core.h @@ -1056,6 +1056,7 @@ struct ath10k { bool bmi_ids_valid; bool qmi_ids_valid; u32 qmi_board_id; + u32 qmi_chip_id; u8 bmi_board_id; u8 bmi_eboard_id; u8 bmi_chip_id; @@ -1295,6 +1296,7 @@ int ath10k_core_register(struct ath10k *ar, const struct ath10k_bus_params *bus_params); 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); 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 5468a41..ae6b1f4 100644 --- a/drivers/net/wireless/ath/ath10k/qmi.c +++ b/drivers/net/wireless/ath/ath10k/qmi.c @@ -576,6 +576,8 @@ static int ath10k_qmi_cap_send_sync_msg(struct ath10k_qmi *qmi) if (resp->chip_info_valid) { qmi->chip_info.chip_id = resp->chip_info.chip_id; qmi->chip_info.chip_family = resp->chip_info.chip_family; + } else { + qmi->chip_info.chip_id = 0xFF; } if (resp->board_info_valid) @@ -817,12 +819,18 @@ static void ath10k_qmi_event_server_arrive(struct ath10k_qmi *qmi) static int ath10k_qmi_fetch_board_file(struct ath10k_qmi *qmi) { struct ath10k *ar = qmi->ar; + int ret; ar->hif.bus = ATH10K_BUS_SNOC; ar->id.qmi_ids_valid = true; ar->id.qmi_board_id = qmi->board_info.board_id; + ar->id.qmi_chip_id = qmi->chip_info.chip_id; ar->hw_params.fw.dir = WCN3990_HW_1_0_FW_DIR; + ret = ath10k_core_check_dt(ar); + if (ret) + ath10k_dbg(ar, ATH10K_DBG_QMI, "DT bdf variant name not set.\n"); + return ath10k_core_fetch_board_file(qmi->ar, ATH10K_BD_IE_BOARD); }
Board Data File (BDF) is loaded upon driver boot-up procedure. The right board data file is identified using bus and qmi-board-id. The problem, however, can occur when the (default) board data file cannot fulfill with the vendor requirements and it is necessary to use a different board data file. Add the support to get the variant field from DTSI and use tht information to load the vendor specific BDF. The device tree requires addition strings to define the variant name wifi@a000000 { status = "okay"; qcom,ath10k-calibration-variant = "xyz-v2"; }; wifi@a800000 { status = "okay"; qcom,ath10k-calibration-variant = "xyz-v1"; }; This would create the boarddata identifiers for the board-2.bin search * bus=snoc,qmi-board-id=16,qmi-chip-id=0,variant=xyz-v1 * bus=snoc,qmi-board-id=17,qmi-chip-id=0,variant=xyz-v2 Tested-on: WCN3990 hw1.0 SNOC WLAN.HL.3.1-01040-QCAHLSWMTPLZ-1 Signed-off-by: Rakesh Pillai <pillair@codeaurora.org> --- drivers/net/wireless/ath/ath10k/core.c | 18 +++++++++++++----- drivers/net/wireless/ath/ath10k/core.h | 2 ++ drivers/net/wireless/ath/ath10k/qmi.c | 8 ++++++++ 3 files changed, 23 insertions(+), 5 deletions(-)