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

[Brian Norris <briannorris@chromium.org>]

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