From: Marcel Ziswiler <marcel@ziswiler.com>
To: David Lin <yu-hao.lin@nxp.com>, linux-wireless@vger.kernel.org
Cc: linux-kernel@vger.kernel.org, briannorris@chromium.org,
kvalo@kernel.org, francesco@dolcini.it,
tsung-hsien.hsieh@nxp.com, stable@vger.kernel.org
Subject: Re: [PATCH v3] wifi: mwifiex: add extra delay for firmware ready
Date: Thu, 14 Dec 2023 11:44:29 +0100 [thread overview]
Message-ID: <34f364a5ed0703b562b631efa1f3cd1fd8f3a93a.camel@ziswiler.com> (raw)
In-Reply-To: <20231208234029.2197-1-yu-hao.lin@nxp.com>
On Sat, 2023-12-09 at 07:40 +0800, David Lin wrote:
> For SDIO IW416, due to a bug, FW may return ready before complete
> full initialization.
BTW: What makes you think this issue is exclusive to the IW416?
We have also seen this in the past both on our Verdin iMX8M Mini (SDIO/SDIO) and Verdin iMX8M Plus (SDIO/UART)
with 88W8997.
good case:
[ 6.496541] mwifiex_sdio mmc0:0001:1: info: FW download over, size 594556 bytes
...
[ 7.272436] mwifiex_sdio mmc0:0001:1: WLAN FW is active
[ 7.314958] mwifiex_sdio mmc0:0001:1: Unknown api_id: 5
[ 7.347647] mwifiex_sdio mmc0:0001:1: info: MWIFIEX VERSION: mwifiex 1.0 (16.92.21.p55)
[ 7.355977] mwifiex_sdio mmc0:0001:1: driver_version = mwifiex 1.0 (16.92.21.p55)
bad case:
[ 8.720216] mwifiex_sdio mmc0:0001:1: info: FW download over, size 594556 bytes
...
[ 24.976699] mwifiex_sdio mmc0:0001:1: FW failed to be active in time
[ 24.983098] mwifiex_sdio mmc0:0001:1: info: _mwifiex_fw_dpc: unregister device
> Command timeout may occur at driver load after reboot.
> Workaround by adding 100ms delay at checking FW status.
>
> Signed-off-by: David Lin <yu-hao.lin@nxp.com>
> Cc: stable@vger.kernel.org
Tested-by: Marcel Ziswiler <marcel.ziswiler@toradex.com> # Verdin AM62 (IW416)
> ---
>
> v3:
> - v2 was a not finished patch that was send to the LKML by mistake
> - changed check condition for extra delay with clear comments.
> - added flag to struct mwifiex_sdio_device / mwifiex_sdio_sd8978 to
> enable extra delay only for IW416.
> ---
> drivers/net/wireless/marvell/mwifiex/sdio.c | 19 +++++++++++++++++++
> drivers/net/wireless/marvell/mwifiex/sdio.h | 2 ++
> 2 files changed, 21 insertions(+)
>
> diff --git a/drivers/net/wireless/marvell/mwifiex/sdio.c b/drivers/net/wireless/marvell/mwifiex/sdio.c
> index 6462a0ffe698..ef3e68d1059c 100644
> --- a/drivers/net/wireless/marvell/mwifiex/sdio.c
> +++ b/drivers/net/wireless/marvell/mwifiex/sdio.c
> @@ -331,6 +331,7 @@ static const struct mwifiex_sdio_device mwifiex_sdio_sd8786 = {
> .can_dump_fw = false,
> .can_auto_tdls = false,
> .can_ext_scan = false,
> + .fw_ready_extra_delay = false,
> };
>
> static const struct mwifiex_sdio_device mwifiex_sdio_sd8787 = {
> @@ -346,6 +347,7 @@ static const struct mwifiex_sdio_device mwifiex_sdio_sd8787 = {
> .can_dump_fw = false,
> .can_auto_tdls = false,
> .can_ext_scan = true,
> + .fw_ready_extra_delay = false,
> };
>
> static const struct mwifiex_sdio_device mwifiex_sdio_sd8797 = {
> @@ -361,6 +363,7 @@ static const struct mwifiex_sdio_device mwifiex_sdio_sd8797 = {
> .can_dump_fw = false,
> .can_auto_tdls = false,
> .can_ext_scan = true,
> + .fw_ready_extra_delay = false,
> };
>
> static const struct mwifiex_sdio_device mwifiex_sdio_sd8897 = {
> @@ -376,6 +379,7 @@ static const struct mwifiex_sdio_device mwifiex_sdio_sd8897 = {
> .can_dump_fw = true,
> .can_auto_tdls = false,
> .can_ext_scan = true,
> + .fw_ready_extra_delay = false,
> };
>
> static const struct mwifiex_sdio_device mwifiex_sdio_sd8977 = {
> @@ -392,6 +396,7 @@ static const struct mwifiex_sdio_device mwifiex_sdio_sd8977 = {
> .fw_dump_enh = true,
> .can_auto_tdls = false,
> .can_ext_scan = true,
> + .fw_ready_extra_delay = false,
> };
>
> static const struct mwifiex_sdio_device mwifiex_sdio_sd8978 = {
> @@ -408,6 +413,7 @@ static const struct mwifiex_sdio_device mwifiex_sdio_sd8978 = {
> .fw_dump_enh = true,
> .can_auto_tdls = false,
> .can_ext_scan = true,
> + .fw_ready_extra_delay = true,
> };
>
> static const struct mwifiex_sdio_device mwifiex_sdio_sd8997 = {
> @@ -425,6 +431,7 @@ static const struct mwifiex_sdio_device mwifiex_sdio_sd8997 = {
> .fw_dump_enh = true,
> .can_auto_tdls = false,
> .can_ext_scan = true,
> + .fw_ready_extra_delay = false,
> };
>
> static const struct mwifiex_sdio_device mwifiex_sdio_sd8887 = {
> @@ -440,6 +447,7 @@ static const struct mwifiex_sdio_device mwifiex_sdio_sd8887 = {
> .can_dump_fw = false,
> .can_auto_tdls = true,
> .can_ext_scan = true,
> + .fw_ready_extra_delay = false,
> };
>
> static const struct mwifiex_sdio_device mwifiex_sdio_sd8987 = {
> @@ -456,6 +464,7 @@ static const struct mwifiex_sdio_device mwifiex_sdio_sd8987 = {
> .fw_dump_enh = true,
> .can_auto_tdls = true,
> .can_ext_scan = true,
> + .fw_ready_extra_delay = false,
> };
>
> static const struct mwifiex_sdio_device mwifiex_sdio_sd8801 = {
> @@ -471,6 +480,7 @@ static const struct mwifiex_sdio_device mwifiex_sdio_sd8801 = {
> .can_dump_fw = false,
> .can_auto_tdls = false,
> .can_ext_scan = true,
> + .fw_ready_extra_delay = false,
> };
>
> static struct memory_type_mapping generic_mem_type_map[] = {
> @@ -563,6 +573,7 @@ mwifiex_sdio_probe(struct sdio_func *func, const struct sdio_device_id *id)
> card->fw_dump_enh = data->fw_dump_enh;
> card->can_auto_tdls = data->can_auto_tdls;
> card->can_ext_scan = data->can_ext_scan;
> + card->fw_ready_extra_delay = data->fw_ready_extra_delay;
> INIT_WORK(&card->work, mwifiex_sdio_work);
> }
>
> @@ -766,6 +777,7 @@ mwifiex_sdio_read_fw_status(struct mwifiex_adapter *adapter, u16 *dat)
> static int mwifiex_check_fw_status(struct mwifiex_adapter *adapter,
> u32 poll_num)
> {
> + struct sdio_mmc_card *card = adapter->card;
> int ret = 0;
> u16 firmware_stat;
> u32 tries;
> @@ -783,6 +795,13 @@ static int mwifiex_check_fw_status(struct mwifiex_adapter *adapter,
> ret = -1;
> }
>
> + if (card->fw_ready_extra_delay &&
> + firmware_stat == FIRMWARE_READY_SDIO)
> + /* firmware might pretend to be ready, when it's not.
> + * Wait a little bit more as a workaround.
> + */
> + msleep(100);
> +
> return ret;
> }
>
> diff --git a/drivers/net/wireless/marvell/mwifiex/sdio.h b/drivers/net/wireless/marvell/mwifiex/sdio.h
> index b86a9263a6a8..cb63ad55d675 100644
> --- a/drivers/net/wireless/marvell/mwifiex/sdio.h
> +++ b/drivers/net/wireless/marvell/mwifiex/sdio.h
> @@ -255,6 +255,7 @@ struct sdio_mmc_card {
> bool fw_dump_enh;
> bool can_auto_tdls;
> bool can_ext_scan;
> + bool fw_ready_extra_delay;
>
> struct mwifiex_sdio_mpa_tx mpa_tx;
> struct mwifiex_sdio_mpa_rx mpa_rx;
> @@ -278,6 +279,7 @@ struct mwifiex_sdio_device {
> bool fw_dump_enh;
> bool can_auto_tdls;
> bool can_ext_scan;
> + bool fw_ready_extra_delay;
> };
>
> /*
>
> base-commit: 783004b6dbda2cfe9a552a4cc9c1d168a2068f6c
next prev parent reply other threads:[~2023-12-14 10:49 UTC|newest]
Thread overview: 8+ messages / expand[flat|nested] mbox.gz Atom feed top
2023-12-08 23:40 [PATCH v3] wifi: mwifiex: add extra delay for firmware ready David Lin
2023-12-11 7:37 ` Francesco Dolcini
2023-12-14 1:32 ` Brian Norris
2023-12-14 10:44 ` Marcel Ziswiler [this message]
2023-12-14 11:46 ` [EXT] " David Lin
2023-12-14 13:35 ` Marcel Ziswiler
2023-12-14 14:27 ` David Lin
2023-12-15 13:24 ` [v3] " Kalle Valo
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=34f364a5ed0703b562b631efa1f3cd1fd8f3a93a.camel@ziswiler.com \
--to=marcel@ziswiler.com \
--cc=briannorris@chromium.org \
--cc=francesco@dolcini.it \
--cc=kvalo@kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-wireless@vger.kernel.org \
--cc=stable@vger.kernel.org \
--cc=tsung-hsien.hsieh@nxp.com \
--cc=yu-hao.lin@nxp.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.