[RFC PATCH] mwifiex: Select firmware based on strapping

[RFC PATCH] mwifiex: Select firmware based on strapping

[Francesco Dolcini]

From: Andrejs Cainikovs <andrejs.cainikovs@toradex.com>

Some WiFi/Bluetooth modules might have different host connection
options, allowing to either use SDIO for both WiFi and Bluetooth,
or SDIO for WiFi and UART for Bluetooth. It is possible to detect
whether a module has SDIO-SDIO or SDIO-UART connection by reading
its host strap register.

This change introduces a way to automatically select appropriate
firmware depending of the connection method, and removes a need
of symlinking or overwriting the original firmware file with a
required one.

Signed-off-by: Andrejs Cainikovs <andrejs.cainikovs@toradex.com>
Signed-off-by: Francesco Dolcini <francesco.dolcini@toradex.com>
---
Hi all,

Current mwifiex_sdio implementation does not have strapping detection, which
means there's no way system will automatically detect which firmware needs to
be picked depending of the strapping. SD8997, in particular, can be strapped
for sdiosdio (Wi-Fi over SDIO, Bluetooth over SDIO) or sdiouart (Wi-Fi over
SDIO, Bluetooth over UART). What we do now - simply replace the
original sdiosdio firmware file with the one supplied by NXP [1] for sdiouart.

Of course, this is not clean, and by submitting this patch I would like to
receive your comments regarding how it would be better to implement the
strapping detection.

[1] https://github.com/NXP/imx-firmware/blob/lf-5.10.52_2.1.0/nxp/FwImage_8997_SD/sdiouart8997_combo_v4.bin

Francesco & Andrejs

---
 drivers/net/wireless/marvell/mwifiex/sdio.c | 17 ++++++++++++++++-
 drivers/net/wireless/marvell/mwifiex/sdio.h |  6 ++++++
 2 files changed, 22 insertions(+), 1 deletion(-)

diff --git a/drivers/net/wireless/marvell/mwifiex/sdio.c b/drivers/net/wireless/marvell/mwifiex/sdio.c
index bde9e4bbfffe..8670ded74c27 100644
--- a/drivers/net/wireless/marvell/mwifiex/sdio.c
+++ b/drivers/net/wireless/marvell/mwifiex/sdio.c
@@ -182,6 +182,9 @@ static const struct mwifiex_sdio_card_reg mwifiex_reg_sd8997 = {
 	.host_int_rsr_reg = 0x4,
 	.host_int_status_reg = 0x0C,
 	.host_int_mask_reg = 0x08,
+	.host_strap_reg = 0xF4,
+	.host_strap_mask = 0x01,
+	.host_strap_value = 0x00,
 	.status_reg_0 = 0xE8,
 	.status_reg_1 = 0xE9,
 	.sdio_int_mask = 0xff,
@@ -402,6 +405,7 @@ static const struct mwifiex_sdio_device mwifiex_sdio_sd8977 = {
 
 static const struct mwifiex_sdio_device mwifiex_sdio_sd8997 = {
 	.firmware = SD8997_DEFAULT_FW_NAME,
+	.firmware_alt_strap = SD8997_SDIOUART_FW_NAME,
 	.reg = &mwifiex_reg_sd8997,
 	.max_ports = 32,
 	.mp_agg_pkt_limit = 16,
@@ -536,6 +540,7 @@ mwifiex_sdio_probe(struct sdio_func *func, const struct sdio_device_id *id)
 		struct mwifiex_sdio_device *data = (void *)id->driver_data;
 
 		card->firmware = data->firmware;
+		card->firmware_alt_strap = data->firmware_alt_strap;
 		card->reg = data->reg;
 		card->max_ports = data->max_ports;
 		card->mp_agg_pkt_limit = data->mp_agg_pkt_limit;
@@ -2439,6 +2444,7 @@ static int mwifiex_register_dev(struct mwifiex_adapter *adapter)
 	int ret;
 	struct sdio_mmc_card *card = adapter->card;
 	struct sdio_func *func = card->func;
+	const char *firmware = card->firmware;
 
 	/* save adapter pointer in card */
 	card->adapter = adapter;
@@ -2455,7 +2461,15 @@ static int mwifiex_register_dev(struct mwifiex_adapter *adapter)
 		return ret;
 	}
 
-	strcpy(adapter->fw_name, card->firmware);
+	/* Select alternative firmware based on the strapping options */
+	if (card->firmware_alt_strap) {
+		u8 val;
+		mwifiex_read_reg(adapter, card->reg->host_strap_reg, &val);
+		if ((val & card->reg->host_strap_mask) == card->reg->host_strap_value)
+			firmware = card->firmware_alt_strap;
+	}
+	strcpy(adapter->fw_name, firmware);
+
 	if (card->fw_dump_enh) {
 		adapter->mem_type_mapping_tbl = generic_mem_type_map;
 		adapter->num_mem_types = 1;
@@ -3157,3 +3171,4 @@ MODULE_FIRMWARE(SD8887_DEFAULT_FW_NAME);
 MODULE_FIRMWARE(SD8977_DEFAULT_FW_NAME);
 MODULE_FIRMWARE(SD8987_DEFAULT_FW_NAME);
 MODULE_FIRMWARE(SD8997_DEFAULT_FW_NAME);
+MODULE_FIRMWARE(SD8997_SDIOUART_FW_NAME);
diff --git a/drivers/net/wireless/marvell/mwifiex/sdio.h b/drivers/net/wireless/marvell/mwifiex/sdio.h
index 5648512c9300..bfea4d5998b7 100644
--- a/drivers/net/wireless/marvell/mwifiex/sdio.h
+++ b/drivers/net/wireless/marvell/mwifiex/sdio.h
@@ -39,6 +39,7 @@
 #define SD8977_DEFAULT_FW_NAME "mrvl/sdsd8977_combo_v2.bin"
 #define SD8987_DEFAULT_FW_NAME "mrvl/sd8987_uapsta.bin"
 #define SD8997_DEFAULT_FW_NAME "mrvl/sdsd8997_combo_v4.bin"
+#define SD8997_SDIOUART_FW_NAME "nxp/sdiouart8997_combo_v4.bin"
 
 #define BLOCK_MODE	1
 #define BYTE_MODE	0
@@ -196,6 +197,9 @@ struct mwifiex_sdio_card_reg {
 	u8 host_int_rsr_reg;
 	u8 host_int_status_reg;
 	u8 host_int_mask_reg;
+	u8 host_strap_reg;
+	u8 host_strap_mask;
+	u8 host_strap_value;
 	u8 status_reg_0;
 	u8 status_reg_1;
 	u8 sdio_int_mask;
@@ -241,6 +245,7 @@ struct sdio_mmc_card {
 
 	struct completion fw_done;
 	const char *firmware;
+	const char *firmware_alt_strap;
 	const struct mwifiex_sdio_card_reg *reg;
 	u8 max_ports;
 	u8 mp_agg_pkt_limit;
@@ -274,6 +279,7 @@ struct sdio_mmc_card {
 
 struct mwifiex_sdio_device {
 	const char *firmware;
+	const char *firmware_alt_strap;
 	const struct mwifiex_sdio_card_reg *reg;
 	u8 max_ports;
 	u8 mp_agg_pkt_limit;
-- 
2.25.1

Re: [RFC PATCH] mwifiex: Select firmware based on strapping

[Brian Norris]

On Mon, Mar 21, 2022 at 05:10:03PM +0100, Francesco Dolcini wrote:
> From: Andrejs Cainikovs <andrejs.cainikovs@toradex.com>
> 
> Some WiFi/Bluetooth modules might have different host connection
> options, allowing to either use SDIO for both WiFi and Bluetooth,
> or SDIO for WiFi and UART for Bluetooth. It is possible to detect
> whether a module has SDIO-SDIO or SDIO-UART connection by reading
> its host strap register.
> 
> This change introduces a way to automatically select appropriate
> firmware depending of the connection method, and removes a need
> of symlinking or overwriting the original firmware file with a
> required one.
> 
> Signed-off-by: Andrejs Cainikovs <andrejs.cainikovs@toradex.com>
> Signed-off-by: Francesco Dolcini <francesco.dolcini@toradex.com>
> ---
> Hi all,
> 
> Current mwifiex_sdio implementation does not have strapping detection, which
> means there's no way system will automatically detect which firmware needs to
> be picked depending of the strapping. SD8997, in particular, can be strapped
> for sdiosdio (Wi-Fi over SDIO, Bluetooth over SDIO) or sdiouart (Wi-Fi over
> SDIO, Bluetooth over UART). What we do now - simply replace the
> original sdiosdio firmware file with the one supplied by NXP [1] for sdiouart.
> 
> Of course, this is not clean, and by submitting this patch I would like to
> receive your comments regarding how it would be better to implement the
> strapping detection.
> 
> [1] https://github.com/NXP/imx-firmware/blob/lf-5.10.52_2.1.0/nxp/FwImage_8997_SD/sdiouart8997_combo_v4.bin
> 
> Francesco & Andrejs
> 
> ---
>  drivers/net/wireless/marvell/mwifiex/sdio.c | 17 ++++++++++++++++-
>  drivers/net/wireless/marvell/mwifiex/sdio.h |  6 ++++++
>  2 files changed, 22 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/net/wireless/marvell/mwifiex/sdio.c b/drivers/net/wireless/marvell/mwifiex/sdio.c
> index bde9e4bbfffe..8670ded74c27 100644
> --- a/drivers/net/wireless/marvell/mwifiex/sdio.c
> +++ b/drivers/net/wireless/marvell/mwifiex/sdio.c
> @@ -182,6 +182,9 @@ static const struct mwifiex_sdio_card_reg mwifiex_reg_sd8997 = {
>  	.host_int_rsr_reg = 0x4,
>  	.host_int_status_reg = 0x0C,
>  	.host_int_mask_reg = 0x08,
> +	.host_strap_reg = 0xF4,
> +	.host_strap_mask = 0x01,
> +	.host_strap_value = 0x00,

Do you have any documentation or sources (e.g., a vendor driver) that
describe this register? If we don't have documentation on what exactly
this register means, we might need to be a bit more conservative on
using it.

Previous thread:
https://lore.kernel.org/linux-wireless/87a6ejj2np.fsf@bang-olufsen.dk/
I guess this links to some driver sources with similar definitions?

Seems like we could still use a bit better naming/macros to explicitly
call these out as "UART" and "SDIO" options, instead of just
"alternate".

>  	.status_reg_0 = 0xE8,
>  	.status_reg_1 = 0xE9,
>  	.sdio_int_mask = 0xff,
> @@ -402,6 +405,7 @@ static const struct mwifiex_sdio_device mwifiex_sdio_sd8977 = {
>  
>  static const struct mwifiex_sdio_device mwifiex_sdio_sd8997 = {
>  	.firmware = SD8997_DEFAULT_FW_NAME,
> +	.firmware_alt_strap = SD8997_SDIOUART_FW_NAME,
>  	.reg = &mwifiex_reg_sd8997,
>  	.max_ports = 32,
>  	.mp_agg_pkt_limit = 16,
> @@ -536,6 +540,7 @@ mwifiex_sdio_probe(struct sdio_func *func, const struct sdio_device_id *id)
>  		struct mwifiex_sdio_device *data = (void *)id->driver_data;
>  
>  		card->firmware = data->firmware;
> +		card->firmware_alt_strap = data->firmware_alt_strap;
>  		card->reg = data->reg;
>  		card->max_ports = data->max_ports;
>  		card->mp_agg_pkt_limit = data->mp_agg_pkt_limit;
> @@ -2439,6 +2444,7 @@ static int mwifiex_register_dev(struct mwifiex_adapter *adapter)
>  	int ret;
>  	struct sdio_mmc_card *card = adapter->card;
>  	struct sdio_func *func = card->func;
> +	const char *firmware = card->firmware;
>  
>  	/* save adapter pointer in card */
>  	card->adapter = adapter;
> @@ -2455,7 +2461,15 @@ static int mwifiex_register_dev(struct mwifiex_adapter *adapter)
>  		return ret;
>  	}
>  
> -	strcpy(adapter->fw_name, card->firmware);
> +	/* Select alternative firmware based on the strapping options */
> +	if (card->firmware_alt_strap) {
> +		u8 val;
> +		mwifiex_read_reg(adapter, card->reg->host_strap_reg, &val);
> +		if ((val & card->reg->host_strap_mask) == card->reg->host_strap_value)
> +			firmware = card->firmware_alt_strap;
> +	}
> +	strcpy(adapter->fw_name, firmware);
> +
>  	if (card->fw_dump_enh) {
>  		adapter->mem_type_mapping_tbl = generic_mem_type_map;
>  		adapter->num_mem_types = 1;
> @@ -3157,3 +3171,4 @@ MODULE_FIRMWARE(SD8887_DEFAULT_FW_NAME);
>  MODULE_FIRMWARE(SD8977_DEFAULT_FW_NAME);
>  MODULE_FIRMWARE(SD8987_DEFAULT_FW_NAME);
>  MODULE_FIRMWARE(SD8997_DEFAULT_FW_NAME);
> +MODULE_FIRMWARE(SD8997_SDIOUART_FW_NAME);
> diff --git a/drivers/net/wireless/marvell/mwifiex/sdio.h b/drivers/net/wireless/marvell/mwifiex/sdio.h
> index 5648512c9300..bfea4d5998b7 100644
> --- a/drivers/net/wireless/marvell/mwifiex/sdio.h
> +++ b/drivers/net/wireless/marvell/mwifiex/sdio.h
> @@ -39,6 +39,7 @@
>  #define SD8977_DEFAULT_FW_NAME "mrvl/sdsd8977_combo_v2.bin"
>  #define SD8987_DEFAULT_FW_NAME "mrvl/sd8987_uapsta.bin"
>  #define SD8997_DEFAULT_FW_NAME "mrvl/sdsd8997_combo_v4.bin"
> +#define SD8997_SDIOUART_FW_NAME "nxp/sdiouart8997_combo_v4.bin"

This isn't your main issue, but just because companies buy and sell IP
doesn't mean we'll change the firmware paths. Qualcomm drivers still use
"ath" prefixes, for one ;)

Personally, I'd still keep the mrvl/ path. But that might be up to Kalle
and/or linux-firmware.git maintainers.

Brian

>  
>  #define BLOCK_MODE	1
>  #define BYTE_MODE	0

Re: [RFC PATCH] mwifiex: Select firmware based on strapping

[Kalle Valo]

Brian Norris <briannorris@chromium.org> writes:

>> --- a/drivers/net/wireless/marvell/mwifiex/sdio.h
>> +++ b/drivers/net/wireless/marvell/mwifiex/sdio.h
>> @@ -39,6 +39,7 @@
>>  #define SD8977_DEFAULT_FW_NAME "mrvl/sdsd8977_combo_v2.bin"
>>  #define SD8987_DEFAULT_FW_NAME "mrvl/sd8987_uapsta.bin"
>>  #define SD8997_DEFAULT_FW_NAME "mrvl/sdsd8997_combo_v4.bin"
>> +#define SD8997_SDIOUART_FW_NAME "nxp/sdiouart8997_combo_v4.bin"
>
> This isn't your main issue, but just because companies buy and sell IP
> doesn't mean we'll change the firmware paths. Qualcomm drivers still use
> "ath" prefixes, for one ;)
>
> Personally, I'd still keep the mrvl/ path. But that might be up to Kalle
> and/or linux-firmware.git maintainers.

I also prefer to have all the firmware files in the mrvl directory.

Actually I would prefer that each driver has it's own firmware
directory, but that's another topic.

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

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