| Message ID | 1492066102-31251-4-git-send-email-huxinming820@gmail.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Kalle Valo |
| Headers | show |
Xinming Hu <huxinming820@gmail.com> writes: > From: Xinming Hu <huxm@marvell.com> > > A seprate wifi-only firmware was download during pcie function level reset. > It is in fact the tail part of wifi/bt combo firmware. Per Brian's and > Dmitry's suggestion, this patch extract the wifi part from combo firmware. > > After that, we can discard the redudant image in linux-firmware repo. If you mean with discarding removing the actual image from linux-firmware, then you cannot do that. The image is still needed so that linux-firmware works with older kernels.
Hi Xinming, On Thu, Apr 13, 2017 at 06:48:22AM +0000, Xinming Hu wrote: > From: Xinming Hu <huxm@marvell.com> > > A seprate wifi-only firmware was download during pcie function level reset. > It is in fact the tail part of wifi/bt combo firmware. Per Brian's and > Dmitry's suggestion, this patch extract the wifi part from combo firmware. > > After that, we can discard the redudant image in linux-firmware repo. > > Signed-off-by: Xinming Hu <huxm@marvell.com> > Signed-off-by: Ganapathi Bhat <gbhat@marvell.com> > Signed-off-by: Cathy Luo <cluo@marvell.com> > Reviewed-by: Brian Norris <briannorris@chromium.org> Please don't add my Reviewed-by tag unless I've provided it explicitly. And especially not if you have to change the patch significantly. (It's fair to carry an old tag I've provided, if you only have to make minor changes, or if I suggest *exactly* what needs changed, along with a conditional 'Reviewed-by'.) As it stands, this patch is worse than the previous one. I replied to some of your commnets on v3, but I'll carry some here. > --- > v2: extract wifi part from combo firmware(Dimtry and Brain) > add more description(Kalle) > v3: same as v2 > v4: add sequence comments, code enhance(Brain) > --- > drivers/net/wireless/marvell/mwifiex/fw.h | 18 ++++++ > drivers/net/wireless/marvell/mwifiex/pcie.c | 93 ++++++++++++++++++++++++++--- > drivers/net/wireless/marvell/mwifiex/pcie.h | 3 +- > 3 files changed, 106 insertions(+), 8 deletions(-) > > diff --git a/drivers/net/wireless/marvell/mwifiex/fw.h b/drivers/net/wireless/marvell/mwifiex/fw.h > index 0b68374..6cf9ab9 100644 > --- a/drivers/net/wireless/marvell/mwifiex/fw.h > +++ b/drivers/net/wireless/marvell/mwifiex/fw.h > @@ -43,6 +43,24 @@ struct tx_packet_hdr { > struct rfc_1042_hdr rfc1042_hdr; > } __packed; > > +struct mwifiex_fw_header { > + __le32 dnld_cmd; > + __le32 base_addr; > + __le32 data_length; > + __le32 crc; > +} __packed; > + > +struct mwifiex_fw_data { > + struct mwifiex_fw_header header; > + __le32 seq_num; > + u8 data[1]; > +} __packed; > + > +#define MWIFIEX_FW_DNLD_CMD_1 0x1 > +#define MWIFIEX_FW_DNLD_CMD_5 0x5 > +#define MWIFIEX_FW_DNLD_CMD_6 0x6 > +#define MWIFIEX_FW_DNLD_CMD_7 0x7 > + > #define B_SUPPORTED_RATES 5 > #define G_SUPPORTED_RATES 9 > #define BG_SUPPORTED_RATES 13 > diff --git a/drivers/net/wireless/marvell/mwifiex/pcie.c b/drivers/net/wireless/marvell/mwifiex/pcie.c > index a07cb0a..7b24bb4 100644 > --- a/drivers/net/wireless/marvell/mwifiex/pcie.c > +++ b/drivers/net/wireless/marvell/mwifiex/pcie.c > @@ -1956,6 +1956,73 @@ static int mwifiex_pcie_event_complete(struct mwifiex_adapter *adapter, > return ret; > } > > +/* Combo firmware image is a combination of > + * (1) combo crc heaer, start with CMD5 > + * (2) bluetooth image, start with CMD7, end with CMD6, data wrapped in CMD1. > + * (3) wifi image. > + * > + * This function bypass the header and bluetooth part, return > + * the offset of tail wifi-only part. > + */ > + > +static int mwifiex_extract_wifi_fw(struct mwifiex_adapter *adapter, > + const void *firmware, u32 firmware_len) { > + const struct mwifiex_fw_data *fwdata; > + u32 offset = 0, data_len, dnld_cmd; > + int ret = 0; > + bool cmd7_before = false; > + > + while (1) { > + if (offset >= firmware_len) { This is worse than what you used to have. This should still be: if (offset + sizeof(fwdata->header) >= firmware_len) { And you might add an overflow check here too, so: /* Check for integer and buffer overflow */ if (offset + sizeof(fwdata->header) < sizeof(fwdata->header) || offset + sizeof(fwdata->header) >= firmware_len) { > + mwifiex_dbg(adapter, ERROR, > + "extract wifi-only fw failure!"); Needs a '\n'. > + ret = -1; > + goto done; > + } > + > + fwdata = firmware + offset; > + dnld_cmd = le32_to_cpu(fwdata->header.dnld_cmd); > + data_len = le32_to_cpu(fwdata->header.data_length); > + Then: offset += sizeof(fwdata->header); > + switch (dnld_cmd) { > + case MWIFIEX_FW_DNLD_CMD_1: > + if (!cmd7_before) { > + mwifiex_dbg(adapter, ERROR, > + "no cmd7 before cmd1!"); Needs a '\n'. > + ret = -1; > + goto done; > + } > + offset += data_len + sizeof(fwdata->header); Replace with: if (offset + data_len < data_len) { mwifiex_dbg(adapter, ERROR, "bad firmware\n"); ret = -1; goto done; } offset += data_len; > + break; > + case MWIFIEX_FW_DNLD_CMD_5: > + offset += data_len + sizeof(fwdata->header); Replace with: if (offset + data_len < data_len) { mwifiex_dbg(adapter, ERROR, "bad firmware\n"); ret = -1; goto done; } offset += data_len; > + break; > + case MWIFIEX_FW_DNLD_CMD_6: > + offset += data_len + sizeof(fwdata->header); Replace with: if (offset + data_len < data_len) { mwifiex_dbg(adapter, ERROR, "bad firmware\n"); ret = -1; goto done; } offset += data_len; > + if (offset >= firmware_len) { > + mwifiex_dbg(adapter, ERROR, > + "extract wifi-only fw failure!"); Needs a '\n'. > + ret = -1; > + } else { > + ret = offset; > + } > + goto done; > + case MWIFIEX_FW_DNLD_CMD_7: > + cmd7_before = true; > + offset += sizeof(fwdata->header); Now you can delete this line. > + break; > + default: > + mwifiex_dbg(adapter, ERROR, "unknown dnld_cmd %d\n", > + dnld_cmd); > + ret = -1; > + goto done; > + } > + } > + > +done: > + return ret; > +} > + > /* > * This function downloads the firmware to the card. > * [...] I might just rewrite this and send it myself, if I get the time. Brian
Hi Kalle, > -----Original Message----- > From: Kalle Valo [mailto:kvalo@codeaurora.org] > Sent: 2017年4月13日 21:59 > To: Xinming Hu > Cc: Linux Wireless; Brian Norris; Dmitry Torokhov; rajatja@google.com; > Amitkumar Karwar; Cathy Luo; Xinming Hu; Ganapathi Bhat > Subject: [EXT] Re: [PATCH v4 4/4] mwifiex: pcie: extract wifi part from combo > firmware during function level reset > > External Email > > ---------------------------------------------------------------------- > Xinming Hu <huxinming820@gmail.com> writes: > > > From: Xinming Hu <huxm@marvell.com> > > > > A seprate wifi-only firmware was download during pcie function level reset. > > It is in fact the tail part of wifi/bt combo firmware. Per Brian's and > > Dmitry's suggestion, this patch extract the wifi part from combo firmware. > > > > After that, we can discard the redudant image in linux-firmware repo. > > If you mean with discarding removing the actual image from linux-firmware, > then you cannot do that. The image is still needed so that linux-firmware > works with older kernels. > Yes, we will keep it. Thanks, Simon > -- > Kalle Valo
diff --git a/drivers/net/wireless/marvell/mwifiex/fw.h b/drivers/net/wireless/marvell/mwifiex/fw.h index 0b68374..6cf9ab9 100644 --- a/drivers/net/wireless/marvell/mwifiex/fw.h +++ b/drivers/net/wireless/marvell/mwifiex/fw.h @@ -43,6 +43,24 @@ struct tx_packet_hdr { struct rfc_1042_hdr rfc1042_hdr; } __packed; +struct mwifiex_fw_header { + __le32 dnld_cmd; + __le32 base_addr; + __le32 data_length; + __le32 crc; +} __packed; + +struct mwifiex_fw_data { + struct mwifiex_fw_header header; + __le32 seq_num; + u8 data[1]; +} __packed; + +#define MWIFIEX_FW_DNLD_CMD_1 0x1 +#define MWIFIEX_FW_DNLD_CMD_5 0x5 +#define MWIFIEX_FW_DNLD_CMD_6 0x6 +#define MWIFIEX_FW_DNLD_CMD_7 0x7 + #define B_SUPPORTED_RATES 5 #define G_SUPPORTED_RATES 9 #define BG_SUPPORTED_RATES 13 diff --git a/drivers/net/wireless/marvell/mwifiex/pcie.c b/drivers/net/wireless/marvell/mwifiex/pcie.c index a07cb0a..7b24bb4 100644 --- a/drivers/net/wireless/marvell/mwifiex/pcie.c +++ b/drivers/net/wireless/marvell/mwifiex/pcie.c @@ -1956,6 +1956,73 @@ static int mwifiex_pcie_event_complete(struct mwifiex_adapter *adapter, return ret; } +/* Combo firmware image is a combination of + * (1) combo crc heaer, start with CMD5 + * (2) bluetooth image, start with CMD7, end with CMD6, data wrapped in CMD1. + * (3) wifi image. + * + * This function bypass the header and bluetooth part, return + * the offset of tail wifi-only part. + */ + +static int mwifiex_extract_wifi_fw(struct mwifiex_adapter *adapter, + const void *firmware, u32 firmware_len) { + const struct mwifiex_fw_data *fwdata; + u32 offset = 0, data_len, dnld_cmd; + int ret = 0; + bool cmd7_before = false; + + while (1) { + if (offset >= firmware_len) { + mwifiex_dbg(adapter, ERROR, + "extract wifi-only fw failure!"); + ret = -1; + goto done; + } + + fwdata = firmware + offset; + dnld_cmd = le32_to_cpu(fwdata->header.dnld_cmd); + data_len = le32_to_cpu(fwdata->header.data_length); + + switch (dnld_cmd) { + case MWIFIEX_FW_DNLD_CMD_1: + if (!cmd7_before) { + mwifiex_dbg(adapter, ERROR, + "no cmd7 before cmd1!"); + ret = -1; + goto done; + } + offset += data_len + sizeof(fwdata->header); + break; + case MWIFIEX_FW_DNLD_CMD_5: + offset += data_len + sizeof(fwdata->header); + break; + case MWIFIEX_FW_DNLD_CMD_6: + offset += data_len + sizeof(fwdata->header); + if (offset >= firmware_len) { + mwifiex_dbg(adapter, ERROR, + "extract wifi-only fw failure!"); + ret = -1; + } else { + ret = offset; + } + goto done; + case MWIFIEX_FW_DNLD_CMD_7: + cmd7_before = true; + offset += sizeof(fwdata->header); + break; + default: + mwifiex_dbg(adapter, ERROR, "unknown dnld_cmd %d\n", + dnld_cmd); + ret = -1; + goto done; + } + } + +done: + return ret; +} + /* * This function downloads the firmware to the card. * @@ -1971,7 +2038,7 @@ static int mwifiex_prog_fw_w_helper(struct mwifiex_adapter *adapter, u32 firmware_len = fw->fw_len; u32 offset = 0; struct sk_buff *skb; - u32 txlen, tx_blocks = 0, tries, len; + u32 txlen, tx_blocks = 0, tries, len, val; u32 block_retry_cnt = 0; struct pcie_service_card *card = adapter->card; const struct mwifiex_pcie_card_reg *reg = card->pcie.reg; @@ -1998,6 +2065,24 @@ static int mwifiex_prog_fw_w_helper(struct mwifiex_adapter *adapter, goto done; } + ret = mwifiex_read_reg(adapter, PCIE_SCRATCH_13_REG, &val); + if (ret) { + mwifiex_dbg(adapter, FATAL, "Failed to read scratch register 13\n"); + goto done; + } + + /* PCIE FLR case: extract wifi part from combo firmware*/ + if (val == MWIFIEX_PCIE_FLR_HAPPENS) { + ret = mwifiex_extract_wifi_fw(adapter, firmware, firmware_len); + if (ret < 0) { + mwifiex_dbg(adapter, ERROR, "Failed to extract wifi fw\n"); + goto done; + } + offset = ret; + mwifiex_dbg(adapter, MSG, + "info: dnld wifi firmware from %d bytes\n", offset); + } + /* Perform firmware data transfer */ do { u32 ireg_intr = 0; @@ -3060,12 +3145,6 @@ static void mwifiex_pcie_up_dev(struct mwifiex_adapter *adapter) struct pci_dev *pdev = card->dev; const struct mwifiex_pcie_card_reg *reg = card->pcie.reg; - /* Bluetooth is not on pcie interface. Download Wifi only firmware - * during pcie FLR, so that bluetooth part of firmware which is - * already running doesn't get affected. - */ - strcpy(adapter->fw_name, PCIE8997_DEFAULT_WIFIFW_NAME); - /* tx_buf_size might be changed to 3584 by firmware during * data transfer, we should reset it to default size. */ diff --git a/drivers/net/wireless/marvell/mwifiex/pcie.h b/drivers/net/wireless/marvell/mwifiex/pcie.h index 7e2450c..f7ce9b6 100644 --- a/drivers/net/wireless/marvell/mwifiex/pcie.h +++ b/drivers/net/wireless/marvell/mwifiex/pcie.h @@ -35,7 +35,6 @@ #define PCIE8897_B0_FW_NAME "mrvl/pcie8897_uapsta.bin" #define PCIEUART8997_FW_NAME_V4 "mrvl/pcieuart8997_combo_v4.bin" #define PCIEUSB8997_FW_NAME_V4 "mrvl/pcieusb8997_combo_v4.bin" -#define PCIE8997_DEFAULT_WIFIFW_NAME "mrvl/pcie8997_wlan_v4.bin" #define PCIE_VENDOR_ID_MARVELL (0x11ab) #define PCIE_VENDOR_ID_V2_MARVELL (0x1b4b) @@ -120,6 +119,8 @@ #define MWIFIEX_SLEEP_COOKIE_SIZE 4 #define MWIFIEX_MAX_DELAY_COUNT 100 +#define MWIFIEX_PCIE_FLR_HAPPENS 0xFEDCBABA + struct mwifiex_pcie_card_reg { u16 cmd_addr_lo; u16 cmd_addr_hi;