On Mon, Mar 21, 2022 at 05:10:03PM +0100, Francesco Dolcini wrote: > From: Andrejs Cainikovs <andrejs.cainikovs@xxxxxxxxxxx> > > 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@xxxxxxxxxxx> > Signed-off-by: Francesco Dolcini <francesco.dolcini@xxxxxxxxxxx> > --- > 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@xxxxxxxxxxxxxxx/ 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
