| Message ID | 1490865547-10208-3-git-send-email-huxinming820@gmail.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Kalle Valo |
| Headers | show |
On Thu, Mar 30, 2017 at 2:19 AM, Xinming Hu <huxinming820@gmail.com> wrote: > From: Xinming Hu <huxm@marvell.com> > > Wifi-only firmware name should be chipset specific. > > Signed-off-by: Xinming Hu <huxm@marvell.com> > Signed-off-by: Amitkumar Karwar <akarwar@marvell.com> > --- > drivers/net/wireless/marvell/mwifiex/pcie.c | 8 +++++++- > drivers/net/wireless/marvell/mwifiex/pcie.h | 6 ++++++ > 2 files changed, 13 insertions(+), 1 deletion(-) > > diff --git a/drivers/net/wireless/marvell/mwifiex/pcie.c b/drivers/net/wireless/marvell/mwifiex/pcie.c > index 59cb01a..282aa9a 100644 > --- a/drivers/net/wireless/marvell/mwifiex/pcie.c > +++ b/drivers/net/wireless/marvell/mwifiex/pcie.c > @@ -351,6 +351,12 @@ static void mwifiex_pcie_reset_notify(struct pci_dev *pdev, bool prepare) > struct pcie_service_card *card = pci_get_drvdata(pdev); > struct mwifiex_adapter *adapter = card->adapter; > > + if (!card->pcie.flr_support) { > + pr_err("%s: FLR disabled in current chipset\n", __func__); > + return; > + } > + > + adapter = card->adapter; > if (!adapter) { > dev_err(&pdev->dev, "%s: adapter structure is not valid\n", > __func__); > @@ -3047,7 +3053,7 @@ static void mwifiex_pcie_up_dev(struct mwifiex_adapter *adapter) > * 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); > + strcpy(adapter->fw_name, card->pcie.wifi_fw_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 00e8ee5..d6c8526 100644 > --- a/drivers/net/wireless/marvell/mwifiex/pcie.h > +++ b/drivers/net/wireless/marvell/mwifiex/pcie.h > @@ -285,6 +285,8 @@ struct mwifiex_pcie_device { > struct memory_type_mapping *mem_type_mapping_tbl; > u8 num_mem_types; > bool can_ext_scan; > + u8 flr_support; This sounds like a boolean, but given that you can key off wifi_fw_name being NULL I am not sure it is needed. > + char *wifi_fw_name; const char *. That said, I consider the whole wifi-only firmware business is quite fragile. Can we have unified firmware and have driver figure out what part shoudl be [re]loaded? > }; > > static const struct mwifiex_pcie_device mwifiex_pcie8766 = { > @@ -293,6 +295,7 @@ struct mwifiex_pcie_device { > .tx_buf_size = MWIFIEX_TX_DATA_BUF_SIZE_2K, > .can_dump_fw = false, > .can_ext_scan = true, > + .flr_support = 0, > }; > > static const struct mwifiex_pcie_device mwifiex_pcie8897 = { > @@ -303,6 +306,7 @@ struct mwifiex_pcie_device { > .mem_type_mapping_tbl = mem_type_mapping_tbl_w8897, > .num_mem_types = ARRAY_SIZE(mem_type_mapping_tbl_w8897), > .can_ext_scan = true, > + .flr_support = 0, > }; > > static const struct mwifiex_pcie_device mwifiex_pcie8997 = { > @@ -313,6 +317,8 @@ struct mwifiex_pcie_device { > .mem_type_mapping_tbl = mem_type_mapping_tbl_w8997, > .num_mem_types = ARRAY_SIZE(mem_type_mapping_tbl_w8997), > .can_ext_scan = true, > + .flr_support = 1, > + .wifi_fw_name = "mrvl/pcie8997_wlan_v4.bin", > }; > > struct mwifiex_evt_buf_desc { > -- > 1.8.1.4 > Thanks.
On Fri, Mar 31, 2017 at 03:50:27PM -0700, Dmitry Torokhov wrote: > That said, I consider the whole wifi-only firmware business is quite > fragile. Can we have unified firmware and have driver figure out what > part shoudl be [re]loaded? +1. I think we should really give a stab at this first, and *then* see how we want to patch up the flagging of support on a per-chipset basis. As-is, you're wasting filesystem space on a duplicate firmware blob, that we have to make sure gets updated in sync with the combined firmware every time there's an update. Proof of the duplicate blob -- the latter portion of the combined FW is identical to the WLAN-only: $ cmp mrvl/pcie{usb8997_combo,8997_wlan}_v4.bin $((0x2919c)); echo $? 0 Brian
Xinming Hu <huxinming820@gmail.com> wrote: > From: Xinming Hu <huxm@marvell.com> > > Wifi-only firmware name should be chipset specific. > > Signed-off-by: Xinming Hu <huxm@marvell.com> > Signed-off-by: Amitkumar Karwar <akarwar@marvell.com> The commit log is not telling anything. Why are you changing this? And how does the functionality change from user space point of view?
Hi Dmitry, > -----Original Message----- > From: Dmitry Torokhov [mailto:dtor@google.com] > Sent: 2017年4月1日 6:50 > To: Xinming Hu > Cc: Linux Wireless; Kalle Valo; Brian Norris; Rajat Jain; Amitkumar Karwar; > Cathy Luo; Xinming Hu > Subject: [EXT] Re: [PATCH 3/3] mwifiex: pcie: avoid hardcode wifi-only > firmware name > > External Email > > ---------------------------------------------------------------------- > On Thu, Mar 30, 2017 at 2:19 AM, Xinming Hu <huxinming820@gmail.com> > wrote: > > From: Xinming Hu <huxm@marvell.com> > > > > Wifi-only firmware name should be chipset specific. > > > > Signed-off-by: Xinming Hu <huxm@marvell.com> > > Signed-off-by: Amitkumar Karwar <akarwar@marvell.com> > > --- > > drivers/net/wireless/marvell/mwifiex/pcie.c | 8 +++++++- > > drivers/net/wireless/marvell/mwifiex/pcie.h | 6 ++++++ > > 2 files changed, 13 insertions(+), 1 deletion(-) > > > > diff --git a/drivers/net/wireless/marvell/mwifiex/pcie.c > > b/drivers/net/wireless/marvell/mwifiex/pcie.c > > index 59cb01a..282aa9a 100644 > > --- a/drivers/net/wireless/marvell/mwifiex/pcie.c > > +++ b/drivers/net/wireless/marvell/mwifiex/pcie.c > > @@ -351,6 +351,12 @@ static void mwifiex_pcie_reset_notify(struct pci_dev > *pdev, bool prepare) > > struct pcie_service_card *card = pci_get_drvdata(pdev); > > struct mwifiex_adapter *adapter = card->adapter; > > > > + if (!card->pcie.flr_support) { > > + pr_err("%s: FLR disabled in current chipset\n", __func__); > > + return; > > + } > > + > > + adapter = card->adapter; > > if (!adapter) { > > dev_err(&pdev->dev, "%s: adapter structure is not > valid\n", > > __func__); > > @@ -3047,7 +3053,7 @@ static void mwifiex_pcie_up_dev(struct > mwifiex_adapter *adapter) > > * 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); > > + strcpy(adapter->fw_name, card->pcie.wifi_fw_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 00e8ee5..d6c8526 100644 > > --- a/drivers/net/wireless/marvell/mwifiex/pcie.h > > +++ b/drivers/net/wireless/marvell/mwifiex/pcie.h > > @@ -285,6 +285,8 @@ struct mwifiex_pcie_device { > > struct memory_type_mapping *mem_type_mapping_tbl; > > u8 num_mem_types; > > bool can_ext_scan; > > + u8 flr_support; > > This sounds like a boolean, but given that you can key off wifi_fw_name being > NULL I am not sure it is needed. > > > + char *wifi_fw_name; > > const char *. > > That said, I consider the whole wifi-only firmware business is quite fragile. Can > we have unified firmware and have driver figure out what part shoudl be > [re]loaded? > Thanks for the review, we have tried to extract wifi-only part from combo firmware. The new solution will be send in V2, in this way, we do not need separate wifi_fw_name any more. Thanks Simon > > }; > > > > static const struct mwifiex_pcie_device mwifiex_pcie8766 = { @@ > > -293,6 +295,7 @@ struct mwifiex_pcie_device { > > .tx_buf_size = MWIFIEX_TX_DATA_BUF_SIZE_2K, > > .can_dump_fw = false, > > .can_ext_scan = true, > > + .flr_support = 0, > > }; > > > > static const struct mwifiex_pcie_device mwifiex_pcie8897 = { @@ > > -303,6 +306,7 @@ struct mwifiex_pcie_device { > > .mem_type_mapping_tbl = mem_type_mapping_tbl_w8897, > > .num_mem_types = ARRAY_SIZE(mem_type_mapping_tbl_w8897), > > .can_ext_scan = true, > > + .flr_support = 0, > > }; > > > > static const struct mwifiex_pcie_device mwifiex_pcie8997 = { @@ > > -313,6 +317,8 @@ struct mwifiex_pcie_device { > > .mem_type_mapping_tbl = mem_type_mapping_tbl_w8997, > > .num_mem_types = ARRAY_SIZE(mem_type_mapping_tbl_w8997), > > .can_ext_scan = true, > > + .flr_support = 1, > > + .wifi_fw_name = "mrvl/pcie8997_wlan_v4.bin", > > }; > > > > struct mwifiex_evt_buf_desc { > > -- > > 1.8.1.4 > > > > Thanks. > > -- > Dmitry
Hi Brain, > -----Original Message----- > From: Brian Norris [mailto:briannorris@chromium.org] > Sent: 2017年4月4日 2:49 > To: Dmitry Torokhov > Cc: Xinming Hu; Linux Wireless; Kalle Valo; Rajat Jain; Amitkumar Karwar; Cathy > Luo; Xinming Hu > Subject: [EXT] Re: [PATCH 3/3] mwifiex: pcie: avoid hardcode wifi-only > firmware name > > External Email > > ---------------------------------------------------------------------- > On Fri, Mar 31, 2017 at 03:50:27PM -0700, Dmitry Torokhov wrote: > > That said, I consider the whole wifi-only firmware business is quite > > fragile. Can we have unified firmware and have driver figure out what > > part shoudl be [re]loaded? > > +1. I think we should really give a stab at this first, and *then* see > how we want to patch up the flagging of support on a per-chipset basis. > As-is, you're wasting filesystem space on a duplicate firmware blob, that we > have to make sure gets updated in sync with the combined firmware every > time there's an update. > > Proof of the duplicate blob -- the latter portion of the combined FW is identical > to the WLAN-only: > > $ cmp mrvl/pcie{usb8997_combo,8997_wlan}_v4.bin $((0x2919c)); echo $? > 0 Thanks for the review, we have a new solution for extracting wifi-only part from combo firmware, will send it in v2. Thanks Simon > > Brian
Hi Kalle, > -----Original Message----- > From: Kalle Valo [mailto:kvalo@codeaurora.org] > Sent: 2017年4月5日 18:47 > To: Xinming Hu > Cc: Linux Wireless; Kalle Valo; Brian Norris; Dmitry Torokhov; > rajatja@google.com; Amitkumar Karwar; Cathy Luo; Xinming Hu > Subject: [EXT] Re: [3/3] mwifiex: pcie: avoid hardcode wifi-only firmware name > > External Email > > ---------------------------------------------------------------------- > Xinming Hu <huxinming820@gmail.com> wrote: > > From: Xinming Hu <huxm@marvell.com> > > > > Wifi-only firmware name should be chipset specific. > > > > Signed-off-by: Xinming Hu <huxm@marvell.com> > > Signed-off-by: Amitkumar Karwar <akarwar@marvell.com> > > The commit log is not telling anything. Why are you changing this? And how > does the functionality change from user space point of view? > Thanks for the review, we have implemented a new way to extract wifi-only part from combo firmware, will add more description in v2 Thanks, Simon > -- > https://patchwork.kernel.org/patch/9653387/ > > https://wireless.wiki.kernel.org/en/developers/documentation/submittingpat > ches
diff --git a/drivers/net/wireless/marvell/mwifiex/pcie.c b/drivers/net/wireless/marvell/mwifiex/pcie.c index 59cb01a..282aa9a 100644 --- a/drivers/net/wireless/marvell/mwifiex/pcie.c +++ b/drivers/net/wireless/marvell/mwifiex/pcie.c @@ -351,6 +351,12 @@ static void mwifiex_pcie_reset_notify(struct pci_dev *pdev, bool prepare) struct pcie_service_card *card = pci_get_drvdata(pdev); struct mwifiex_adapter *adapter = card->adapter; + if (!card->pcie.flr_support) { + pr_err("%s: FLR disabled in current chipset\n", __func__); + return; + } + + adapter = card->adapter; if (!adapter) { dev_err(&pdev->dev, "%s: adapter structure is not valid\n", __func__); @@ -3047,7 +3053,7 @@ static void mwifiex_pcie_up_dev(struct mwifiex_adapter *adapter) * 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); + strcpy(adapter->fw_name, card->pcie.wifi_fw_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 00e8ee5..d6c8526 100644 --- a/drivers/net/wireless/marvell/mwifiex/pcie.h +++ b/drivers/net/wireless/marvell/mwifiex/pcie.h @@ -285,6 +285,8 @@ struct mwifiex_pcie_device { struct memory_type_mapping *mem_type_mapping_tbl; u8 num_mem_types; bool can_ext_scan; + u8 flr_support; + char *wifi_fw_name; }; static const struct mwifiex_pcie_device mwifiex_pcie8766 = { @@ -293,6 +295,7 @@ struct mwifiex_pcie_device { .tx_buf_size = MWIFIEX_TX_DATA_BUF_SIZE_2K, .can_dump_fw = false, .can_ext_scan = true, + .flr_support = 0, }; static const struct mwifiex_pcie_device mwifiex_pcie8897 = { @@ -303,6 +306,7 @@ struct mwifiex_pcie_device { .mem_type_mapping_tbl = mem_type_mapping_tbl_w8897, .num_mem_types = ARRAY_SIZE(mem_type_mapping_tbl_w8897), .can_ext_scan = true, + .flr_support = 0, }; static const struct mwifiex_pcie_device mwifiex_pcie8997 = { @@ -313,6 +317,8 @@ struct mwifiex_pcie_device { .mem_type_mapping_tbl = mem_type_mapping_tbl_w8997, .num_mem_types = ARRAY_SIZE(mem_type_mapping_tbl_w8997), .can_ext_scan = true, + .flr_support = 1, + .wifi_fw_name = "mrvl/pcie8997_wlan_v4.bin", }; struct mwifiex_evt_buf_desc {