| Message ID | 20220321161003.39214-1-francesco.dolcini@toradex.com (mailing list archive) |
|---|---|
| State | RFC |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [RFC] mwifiex: Select firmware based on strapping | expand |
| Context | Check | Description |
|---|---|---|
| netdev/tree_selection | success | Not a local patch |
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
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.
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;