[PATCH] increase rtl8xxxu polling timeout for firmware startup

[PATCH] increase rtl8xxxu polling timeout for firmware startup

[Dan Lenski]

Here is the patch to increase the polling timeout for rtl8xxxu firmware
startup, and to make it configurable, as referred to in my previous message.

Dan Lenski (1):
  Make firmware startup polling timeout configurable, and increase
    default

 drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu.c | 11 +++++++----
 drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu.h |  2 +-
 2 files changed, 8 insertions(+), 5 deletions(-)

-- 
2.8.2

[PATCH] Make firmware startup polling timeout configurable, and increase default

[Dan Lenski]

This patch:

- increases the default value for the maximum number of polling loops to
  wait for the rtl8xxxu MCU to start after the firmware is loaded (from
  1000 to 5000)
- makes this a configurable module parameter

With RTL8723AU chipset, I frequently encounter "Firmware failed to start"
errors from rtl8xxxu after a cold boot.

It appears that other chipsets supported by the driver have the same
problem. Here are a couple of relevant bug reports:
- http://ubuntuforums.org/showthread.php?t=2321756
- https://www.mail-archive.com/ubuntu-bugs-nLRlyDuq1AZFpShjVBNYrg <at> public.gmane.org/msg4942468.html

This issue seems to occur because RTL8XXXU_FIRMWARE_POLL_MAX (1000) is
too short, and the MCU fails to start up as quickly as expected.

With a longer value (5000), the driver starts up consistently and
successfully after cold-boot.
---
 drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu.c | 11 +++++++----
 drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu.h |  2 +-
 2 files changed, 8 insertions(+), 5 deletions(-)

diff --git a/drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu.c b/drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu.c
index 6aed923..a1efb2c 100644
--- a/drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu.c
+++ b/drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu.c
@@ -44,6 +44,7 @@
 
 static int rtl8xxxu_debug;
 static bool rtl8xxxu_ht40_2g;
+static int rtl8xxxu_firmware_poll_max = RTL8XXXU_FIRMWARE_POLL_MAX;
 
 MODULE_AUTHOR("Jes Sorensen <Jes.Sorensen@redhat.com>");
 MODULE_DESCRIPTION("RTL8XXXu USB mac80211 Wireless LAN Driver");
@@ -59,6 +60,8 @@ module_param_named(debug, rtl8xxxu_debug, int, 0600);
 MODULE_PARM_DESC(debug, "Set debug mask");
 module_param_named(ht40_2g, rtl8xxxu_ht40_2g, bool, 0600);
 MODULE_PARM_DESC(ht40_2g, "Enable HT40 support on the 2.4GHz band");
+module_param_named(firmware_poll_max, rtl8xxxu_firmware_poll_max, int, 0600);
+MODULE_PARM_DESC(firmware_poll_max, "Maximum polling count for firmware startup (increase if firmware fails to start)");
 
 #define USB_VENDOR_ID_REALTEK		0x0bda
 /* Minimum IEEE80211_MAX_FRAME_LEN */
@@ -2050,13 +2053,13 @@ static int rtl8xxxu_start_firmware(struct rtl8xxxu_priv *priv)
 	u32 val32;
 
 	/* Poll checksum report */
-	for (i = 0; i < RTL8XXXU_FIRMWARE_POLL_MAX; i++) {
+	for (i = 0; i < rtl8xxxu_firmware_poll_max; i++) {
 		val32 = rtl8xxxu_read32(priv, REG_MCU_FW_DL);
 		if (val32 & MCU_FW_DL_CSUM_REPORT)
 			break;
 	}
 
-	if (i == RTL8XXXU_FIRMWARE_POLL_MAX) {
+	if (i == rtl8xxxu_firmware_poll_max) {
 		dev_warn(dev, "Firmware checksum poll timed out\n");
 		ret = -EAGAIN;
 		goto exit;
@@ -2068,7 +2071,7 @@ static int rtl8xxxu_start_firmware(struct rtl8xxxu_priv *priv)
 	rtl8xxxu_write32(priv, REG_MCU_FW_DL, val32);
 
 	/* Wait for firmware to become ready */
-	for (i = 0; i < RTL8XXXU_FIRMWARE_POLL_MAX; i++) {
+	for (i = 0; i < rtl8xxxu_firmware_poll_max; i++) {
 		val32 = rtl8xxxu_read32(priv, REG_MCU_FW_DL);
 		if (val32 & MCU_WINT_INIT_READY)
 			break;
@@ -2076,7 +2079,7 @@ static int rtl8xxxu_start_firmware(struct rtl8xxxu_priv *priv)
 		udelay(100);
 	}
 
-	if (i == RTL8XXXU_FIRMWARE_POLL_MAX) {
+	if (i == rtl8xxxu_firmware_poll_max) {
 		dev_warn(dev, "Firmware failed to start\n");
 		ret = -EAGAIN;
 		goto exit;
diff --git a/drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu.h b/drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu.h
index f2a1bac..f2838e1 100644
--- a/drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu.h
+++ b/drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu.h
@@ -49,7 +49,7 @@
 #define TX_PAGE_NUM_NORM_PQ		0x02
 
 #define RTL_FW_PAGE_SIZE		4096
-#define RTL8XXXU_FIRMWARE_POLL_MAX	1000
+#define RTL8XXXU_FIRMWARE_POLL_MAX	5000
 
 #define RTL8723A_CHANNEL_GROUPS		3
 #define RTL8723A_MAX_RF_PATHS		2
-- 
2.8.2

Re: [PATCH] Make firmware startup polling timeout configurable, and increase default

[Julian Calaby]

Hi Dan,

Add a "rtl8xxxu:" prefix to the patch title. This makes it easier to
determine which patch is for which driver when only the titles are
listed.

On Wed, May 18, 2016 at 8:48 AM, Dan Lenski <dlenski@gmail.com> wrote:
> This patch:
>
> - increases the default value for the maximum number of polling loops to
>   wait for the rtl8xxxu MCU to start after the firmware is loaded (from
>   1000 to 5000)
> - makes this a configurable module parameter

Split this into two patches, one to make it configurable and one to
increase the default.

> With RTL8723AU chipset, I frequently encounter "Firmware failed to start"
> errors from rtl8xxxu after a cold boot.
>
> It appears that other chipsets supported by the driver have the same
> problem. Here are a couple of relevant bug reports:
> - http://ubuntuforums.org/showthread.php?t=2321756
> - https://www.mail-archive.com/ubuntu-bugs-nLRlyDuq1AZFpShjVBNYrg <at> public.gmane.org/msg4942468.html
>
> This issue seems to occur because RTL8XXXU_FIRMWARE_POLL_MAX (1000) is
> too short, and the MCU fails to start up as quickly as expected.
>
> With a longer value (5000), the driver starts up consistently and
> successfully after cold-boot.

No signed-off-by.

> ---
>  drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu.c | 11 +++++++----
>  drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu.h |  2 +-
>  2 files changed, 8 insertions(+), 5 deletions(-)

Other than those things, the actual changes look fine to me.

Thanks,

-- 
Julian Calaby

Email: julian.calaby@gmail.com
Profile: http://www.google.com/profiles/julian.calaby/

Re: [PATCH] Make firmware startup polling timeout configurable, and increase default

[Jes Sorensen]

Dan Lenski <dlenski@gmail.com> writes:
> This patch:
>
> - increases the default value for the maximum number of polling loops to
>   wait for the rtl8xxxu MCU to start after the firmware is loaded (from
>   1000 to 5000)
> - makes this a configurable module parameter
>
> With RTL8723AU chipset, I frequently encounter "Firmware failed to start"
> errors from rtl8xxxu after a cold boot.
>
> It appears that other chipsets supported by the driver have the same
> problem. Here are a couple of relevant bug reports:
> - http://ubuntuforums.org/showthread.php?t=2321756
> - https://www.mail-archive.com/ubuntu-bugs-nLRlyDuq1AZFpShjVBNYrg <at> public.gmane.org/msg4942468.html
>
> This issue seems to occur because RTL8XXXU_FIRMWARE_POLL_MAX (1000) is
> too short, and the MCU fails to start up as quickly as expected.
>
> With a longer value (5000), the driver starts up consistently and
> successfully after cold-boot.

I am not against increasing the maximum value here, however I would like
more details about the system where you see these problems. I have used
this driver extensively for a long time with my Lenovo Yoga 13 laptop
and not seen this issue.

Second, I really don't think this warrants a module parameter. Bumping
the value should suffice.

Please clean up your patch message to have the required information
included, as previously pointed out on the list.

Note that the patch should at least be relative to
wireless-drivers-next, or preferably my rtl8xxxu-devel tree. In both of
these trees, the rtl8xxxu driver has been split into multiple files and
your patch will not apply.

Last, neither of the links you included for external bug reports work.

Jes

Re: [PATCH] Make firmware startup polling timeout configurable, and increase default

[Daniel Lenski]

Jes Sorensen <Jes.Sorensen@redhat.com> writes:
>
> Dan Lenski <dlenski@gmail.com> writes:
> >
> > This issue seems to occur because RTL8XXXU_FIRMWARE_POLL_MAX (1000) is
> > too short, and the MCU fails to start up as quickly as expected.
> >
> > With a longer value (5000), the driver starts up consistently and
> > successfully after cold-boot.
>
> I am not against increasing the maximum value here, however I would like
> more details about the system where you see these problems. I have used
> this driver extensively for a long time with my Lenovo Yoga 13 laptop
> and not seen this issue.

Mine is also a Yoga 13.

Here is the chipset description that the rtl8xxxu driver prints on load:

[    8.097402] usb 1-1.4: RTL8723AU rev B (TSMC) 1T1R, TX queues 2,
WiFi=1, BT=1, GPS=0, HI PA=0

What other details should I add about it?

> Second, I really don't think this warrants a module parameter. Bumping
> the value should suffice.

Okay. Adding a module parameter was useful for me in debugging, and I
thought it might
be a useful backup option for other end-users who may find that the
default value doesn't
suffice.

It seems plausible to me that the delay is due to waiting for some
kind of analog
circuit to stabilize (PLL, maybe?) and that there could be a large
variation among
devices.

> Note that the patch should at least be relative to
> wireless-drivers-next, or preferably my rtl8xxxu-devel tree. In both of
> these trees, the rtl8xxxu driver has been split into multiple files and
> your patch will not apply.
>
> Last, neither of the links you included for external bug reports work.

Are you still unable to follow them in the second version of the patches that
I submitted? (It appears that they were mangled by Gmane the first
time I posted them.)

Here they are again:

http://ubuntuforums.org/showthread.php?t=2321756

That is, thread #2321756 at ubuntuforums.org

https://www.mail-archive.com/ubuntu-bugs@lists.ubuntu.com/msg4942468.html

That is, Ubuntu bug #1574622

Thanks,
Dan

Re: [PATCH] Make firmware startup polling timeout configurable, and increase default

[Jes Sorensen]

Daniel Lenski <dlenski@gmail.com> writes:
> Jes Sorensen <Jes.Sorensen@redhat.com> writes:
>>
>> Dan Lenski <dlenski@gmail.com> writes:
>> >
>> > This issue seems to occur because RTL8XXXU_FIRMWARE_POLL_MAX (1000) is
>> > too short, and the MCU fails to start up as quickly as expected.
>> >
>> > With a longer value (5000), the driver starts up consistently and
>> > successfully after cold-boot.
>>
>> I am not against increasing the maximum value here, however I would like
>> more details about the system where you see these problems. I have used
>> this driver extensively for a long time with my Lenovo Yoga 13 laptop
>> and not seen this issue.
>
> Mine is also a Yoga 13.

Interesting, I thought it might have been an ARM board and it could be
something else that wasn't happening correctly. So far I only know of
the Yogas and certain ARM boards that have the 8723au.

>> Second, I really don't think this warrants a module parameter. Bumping
>> the value should suffice.
>
> Okay. Adding a module parameter was useful for me in debugging, and I
> thought it might be a useful backup option for other end-users who may
> find that the default value doesn't suffice.
>
> It seems plausible to me that the delay is due to waiting for some
> kind of analog circuit to stabilize (PLL, maybe?) and that there could
> be a large variation among devices.

I am surprised your system is that different from mine, but I am fine
with bumping it. I don't really like to clutter the module parameters
unnecessarily, and if we bump this to 5000, I think it is unlikely
anyone else will hit this problem. If they do, I think there are other
issues at play.

>> Note that the patch should at least be relative to
>> wireless-drivers-next, or preferably my rtl8xxxu-devel tree. In both of
>> these trees, the rtl8xxxu driver has been split into multiple files and
>> your patch will not apply.
>>
>> Last, neither of the links you included for external bug reports work.
>
> Are you still unable to follow them in the second version of the patches that
> I submitted? (It appears that they were mangled by Gmane the first
> time I posted them.)
>
> Here they are again:
>
> http://ubuntuforums.org/showthread.php?t=2321756
>
> That is, thread #2321756 at ubuntuforums.org
>
> https://www.mail-archive.com/ubuntu-bugs@lists.ubuntu.com/msg4942468.html
>
> That is, Ubuntu bug #1574622

I found them from your second post.

Cheers,
Jes