linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: "Pali Rohár" <pali@kernel.org>
To: "Jonas Dreßler" <verdre@v0yd.nl>
Cc: Amitkumar Karwar <amitkarwar@gmail.com>,
	Ganapathi Bhat <ganapathi017@gmail.com>,
	Xinming Hu <huxinming820@gmail.com>,
	Kalle Valo <kvalo@codeaurora.org>,
	"David S. Miller" <davem@davemloft.net>,
	Jakub Kicinski <kuba@kernel.org>,
	Tsuchiya Yuto <kitakar@gmail.com>,
	linux-wireless@vger.kernel.org, netdev@vger.kernel.org,
	linux-kernel@vger.kernel.org, linux-pci@vger.kernel.org,
	Maximilian Luz <luzmaximilian@gmail.com>,
	Andy Shevchenko <andriy.shevchenko@linux.intel.com>,
	Bjorn Helgaas <bhelgaas@google.com>,
	Heiner Kallweit <hkallweit1@gmail.com>,
	Johannes Berg <johannes@sipsolutions.net>,
	Brian Norris <briannorris@chromium.org>,
	David Laight <David.Laight@ACULAB.COM>
Subject: Re: [PATCH] mwifiex: Add quirk resetting the PCI bridge on MS Surface devices
Date: Mon, 11 Oct 2021 19:02:15 +0200	[thread overview]
Message-ID: <20211011170215.3bnmi6sa5yqux2r7@pali> (raw)
In-Reply-To: <20211011134238.16551-1-verdre@v0yd.nl>

On Monday 11 October 2021 15:42:38 Jonas Dreßler wrote:
> The most recent firmware (15.68.19.p21) of the 88W8897 PCIe+USB card
> reports a hardcoded LTR value to the system during initialization,
> probably as an (unsuccessful) attempt of the developers to fix firmware
> crashes. This LTR value prevents most of the Microsoft Surface devices
> from entering deep powersaving states (either platform C-State 10 or
> S0ix state), because the exit latency of that state would be higher than
> what the card can tolerate.

This description looks like a generic issue in 88W8897 chip or its
firmware and not something to Surface PCIe controller or Surface HW. But
please correct me if I'm wrong here.

Has somebody 88W8897-based PCIe card in non-Surface device and can check
or verify if this issue happens also outside of the Surface device?

It would be really nice to know if this is issue in Surface or in 8897.

> Turns out the card works just the same (including the firmware crashes)
> no matter if that hardcoded LTR value is reported or not, so it's kind
> of useless and only prevents us from saving power.
> 
> To get rid of those hardcoded LTR requirements, it's possible to reset
> the PCI bridge device after initializing the cards firmware. I'm not
> exactly sure why that works, maybe the power management subsystem of the
> PCH resets its stored LTR values when doing a function level reset of
> the bridge device. Doing the reset once after starting the wifi firmware
> works very well, probably because the firmware only reports that LTR
> value a single time during firmware startup.
> 
> Signed-off-by: Jonas Dreßler <verdre@v0yd.nl>
> ---
>  drivers/net/wireless/marvell/mwifiex/pcie.c   | 12 +++++++++
>  .../wireless/marvell/mwifiex/pcie_quirks.c    | 26 +++++++++++++------
>  .../wireless/marvell/mwifiex/pcie_quirks.h    |  1 +
>  3 files changed, 31 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/net/wireless/marvell/mwifiex/pcie.c b/drivers/net/wireless/marvell/mwifiex/pcie.c
> index c6ccce426b49..2506e7e49f0c 100644
> --- a/drivers/net/wireless/marvell/mwifiex/pcie.c
> +++ b/drivers/net/wireless/marvell/mwifiex/pcie.c
> @@ -1748,9 +1748,21 @@ mwifiex_pcie_send_boot_cmd(struct mwifiex_adapter *adapter, struct sk_buff *skb)
>  static int mwifiex_pcie_init_fw_port(struct mwifiex_adapter *adapter)
>  {
>  	struct pcie_service_card *card = adapter->card;
> +	struct pci_dev *pdev = card->dev;
> +	struct pci_dev *parent_pdev = pci_upstream_bridge(pdev);
>  	const struct mwifiex_pcie_card_reg *reg = card->pcie.reg;
>  	int tx_wrap = card->txbd_wrptr & reg->tx_wrap_mask;
>  
> +	/* Trigger a function level reset of the PCI bridge device, this makes
> +	 * the firmware (latest version 15.68.19.p21) of the 88W8897 PCIe+USB
> +	 * card stop reporting a fixed LTR value that prevents the system from
> +	 * entering package C10 and S0ix powersaving states.
> +	 * We need to do it here because it must happen after firmware
> +	 * initialization and this function is called right after that is done.
> +	 */
> +	if (card->quirks & QUIRK_DO_FLR_ON_BRIDGE)
> +		pci_reset_function(parent_pdev);
> +
>  	/* Write the RX ring read pointer in to reg->rx_rdptr */
>  	if (mwifiex_write_reg(adapter, reg->rx_rdptr, card->rxbd_rdptr |
>  			      tx_wrap)) {
> diff --git a/drivers/net/wireless/marvell/mwifiex/pcie_quirks.c b/drivers/net/wireless/marvell/mwifiex/pcie_quirks.c
> index 0234cf3c2974..cbf0565353ae 100644
> --- a/drivers/net/wireless/marvell/mwifiex/pcie_quirks.c
> +++ b/drivers/net/wireless/marvell/mwifiex/pcie_quirks.c
> @@ -27,7 +27,8 @@ static const struct dmi_system_id mwifiex_quirk_table[] = {
>  			DMI_EXACT_MATCH(DMI_SYS_VENDOR, "Microsoft Corporation"),
>  			DMI_EXACT_MATCH(DMI_PRODUCT_NAME, "Surface Pro 4"),
>  		},
> -		.driver_data = (void *)QUIRK_FW_RST_D3COLD,
> +		.driver_data = (void *)(QUIRK_FW_RST_D3COLD |
> +					QUIRK_DO_FLR_ON_BRIDGE),
>  	},
>  	{
>  		.ident = "Surface Pro 5",
> @@ -36,7 +37,8 @@ static const struct dmi_system_id mwifiex_quirk_table[] = {
>  			DMI_EXACT_MATCH(DMI_SYS_VENDOR, "Microsoft Corporation"),
>  			DMI_EXACT_MATCH(DMI_PRODUCT_SKU, "Surface_Pro_1796"),
>  		},
> -		.driver_data = (void *)QUIRK_FW_RST_D3COLD,
> +		.driver_data = (void *)(QUIRK_FW_RST_D3COLD |
> +					QUIRK_DO_FLR_ON_BRIDGE),
>  	},
>  	{
>  		.ident = "Surface Pro 5 (LTE)",
> @@ -45,7 +47,8 @@ static const struct dmi_system_id mwifiex_quirk_table[] = {
>  			DMI_EXACT_MATCH(DMI_SYS_VENDOR, "Microsoft Corporation"),
>  			DMI_EXACT_MATCH(DMI_PRODUCT_SKU, "Surface_Pro_1807"),
>  		},
> -		.driver_data = (void *)QUIRK_FW_RST_D3COLD,
> +		.driver_data = (void *)(QUIRK_FW_RST_D3COLD |
> +					QUIRK_DO_FLR_ON_BRIDGE),
>  	},
>  	{
>  		.ident = "Surface Pro 6",
> @@ -53,7 +56,8 @@ static const struct dmi_system_id mwifiex_quirk_table[] = {
>  			DMI_EXACT_MATCH(DMI_SYS_VENDOR, "Microsoft Corporation"),
>  			DMI_EXACT_MATCH(DMI_PRODUCT_NAME, "Surface Pro 6"),
>  		},
> -		.driver_data = (void *)QUIRK_FW_RST_D3COLD,
> +		.driver_data = (void *)(QUIRK_FW_RST_D3COLD |
> +					QUIRK_DO_FLR_ON_BRIDGE),
>  	},
>  	{
>  		.ident = "Surface Book 1",
> @@ -61,7 +65,8 @@ static const struct dmi_system_id mwifiex_quirk_table[] = {
>  			DMI_EXACT_MATCH(DMI_SYS_VENDOR, "Microsoft Corporation"),
>  			DMI_EXACT_MATCH(DMI_PRODUCT_NAME, "Surface Book"),
>  		},
> -		.driver_data = (void *)QUIRK_FW_RST_D3COLD,
> +		.driver_data = (void *)(QUIRK_FW_RST_D3COLD |
> +					QUIRK_DO_FLR_ON_BRIDGE),
>  	},
>  	{
>  		.ident = "Surface Book 2",
> @@ -69,7 +74,8 @@ static const struct dmi_system_id mwifiex_quirk_table[] = {
>  			DMI_EXACT_MATCH(DMI_SYS_VENDOR, "Microsoft Corporation"),
>  			DMI_EXACT_MATCH(DMI_PRODUCT_NAME, "Surface Book 2"),
>  		},
> -		.driver_data = (void *)QUIRK_FW_RST_D3COLD,
> +		.driver_data = (void *)(QUIRK_FW_RST_D3COLD |
> +					QUIRK_DO_FLR_ON_BRIDGE),
>  	},
>  	{
>  		.ident = "Surface Laptop 1",
> @@ -77,7 +83,8 @@ static const struct dmi_system_id mwifiex_quirk_table[] = {
>  			DMI_EXACT_MATCH(DMI_SYS_VENDOR, "Microsoft Corporation"),
>  			DMI_EXACT_MATCH(DMI_PRODUCT_NAME, "Surface Laptop"),
>  		},
> -		.driver_data = (void *)QUIRK_FW_RST_D3COLD,
> +		.driver_data = (void *)(QUIRK_FW_RST_D3COLD |
> +					QUIRK_DO_FLR_ON_BRIDGE),
>  	},
>  	{
>  		.ident = "Surface Laptop 2",
> @@ -85,7 +92,8 @@ static const struct dmi_system_id mwifiex_quirk_table[] = {
>  			DMI_EXACT_MATCH(DMI_SYS_VENDOR, "Microsoft Corporation"),
>  			DMI_EXACT_MATCH(DMI_PRODUCT_NAME, "Surface Laptop 2"),
>  		},
> -		.driver_data = (void *)QUIRK_FW_RST_D3COLD,
> +		.driver_data = (void *)(QUIRK_FW_RST_D3COLD |
> +					QUIRK_DO_FLR_ON_BRIDGE),
>  	},
>  	{}
>  };
> @@ -103,6 +111,8 @@ void mwifiex_initialize_quirks(struct pcie_service_card *card)
>  		dev_info(&pdev->dev, "no quirks enabled\n");
>  	if (card->quirks & QUIRK_FW_RST_D3COLD)
>  		dev_info(&pdev->dev, "quirk reset_d3cold enabled\n");
> +	if (card->quirks & QUIRK_DO_FLR_ON_BRIDGE)
> +		dev_info(&pdev->dev, "quirk do_flr_on_bridge enabled\n");
>  }
>  
>  static void mwifiex_pcie_set_power_d3cold(struct pci_dev *pdev)
> diff --git a/drivers/net/wireless/marvell/mwifiex/pcie_quirks.h b/drivers/net/wireless/marvell/mwifiex/pcie_quirks.h
> index 8ec4176d698f..f8d463f4269a 100644
> --- a/drivers/net/wireless/marvell/mwifiex/pcie_quirks.h
> +++ b/drivers/net/wireless/marvell/mwifiex/pcie_quirks.h
> @@ -18,6 +18,7 @@
>  #include "pcie.h"
>  
>  #define QUIRK_FW_RST_D3COLD	BIT(0)
> +#define QUIRK_DO_FLR_ON_BRIDGE	BIT(1)
>  
>  void mwifiex_initialize_quirks(struct pcie_service_card *card);
>  int mwifiex_pcie_reset_d3cold_quirk(struct pci_dev *pdev);
> -- 
> 2.31.1
> 

  parent reply	other threads:[~2021-10-11 17:02 UTC|newest]

Thread overview: 16+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-10-11 13:42 [PATCH] mwifiex: Add quirk resetting the PCI bridge on MS Surface devices Jonas Dreßler
2021-10-11 16:53 ` Bjorn Helgaas
2021-10-12  8:48   ` Jonas Dreßler
2021-10-12  9:00     ` Pali Rohár
2021-10-12 11:20       ` Jonas Dreßler
2021-10-12 11:29     ` Bjorn Helgaas
2021-10-13 21:35       ` Jonas Dreßler
2021-10-11 17:02 ` Pali Rohár [this message]
2021-10-12  8:55   ` Jonas Dreßler
2021-10-12 15:39     ` Bjorn Helgaas
2021-10-13 22:08       ` Jonas Dreßler
2021-10-18 15:35         ` Bjorn Helgaas
2021-10-25 16:45           ` Jonas Dreßler
2021-10-25 23:56             ` Bjorn Helgaas
2021-11-04 13:57               ` Jonas Dreßler
2021-10-18 12:27 ` 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=20211011170215.3bnmi6sa5yqux2r7@pali \
    --to=pali@kernel.org \
    --cc=David.Laight@ACULAB.COM \
    --cc=amitkarwar@gmail.com \
    --cc=andriy.shevchenko@linux.intel.com \
    --cc=bhelgaas@google.com \
    --cc=briannorris@chromium.org \
    --cc=davem@davemloft.net \
    --cc=ganapathi017@gmail.com \
    --cc=hkallweit1@gmail.com \
    --cc=huxinming820@gmail.com \
    --cc=johannes@sipsolutions.net \
    --cc=kitakar@gmail.com \
    --cc=kuba@kernel.org \
    --cc=kvalo@codeaurora.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-pci@vger.kernel.org \
    --cc=linux-wireless@vger.kernel.org \
    --cc=luzmaximilian@gmail.com \
    --cc=netdev@vger.kernel.org \
    --cc=verdre@v0yd.nl \
    /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 a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).