[PATCH] mmc: sdhci-pci-gli: Disable LPM during initialization

[PATCH] mmc: sdhci-pci-gli: Disable LPM during initialization

[Kornel Dulęba]

To address IO performance commit f9e5b33934ce
("mmc: host: Improve I/O read/write performance for GL9763E")
limited LPM negotiation to runtime suspend state.
The problem is that it only flips the switch in the runtime PM
resume/suspend logic.

Disable LPM negotiation in gl9763e_add_host.
This helps in two ways:
1. It was found that the LPM switch stays in the same position after
   warm reboot. Having it set in init helps with consistency.
2. Disabling LPM during the first runtime resume leaves us susceptible
   to the performance issue in the time window between boot and the
   first runtime suspend.

Fixes: f9e5b33934ce ("mmc: host: Improve I/O read/write performance for GL9763E")
Cc: stable@vger.kernel.org
Signed-off-by: Kornel Dulęba <korneld@chromium.org>
---
 drivers/mmc/host/sdhci-pci-gli.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/drivers/mmc/host/sdhci-pci-gli.c b/drivers/mmc/host/sdhci-pci-gli.c
index d83261e857a5..ce91d1e63a8e 100644
--- a/drivers/mmc/host/sdhci-pci-gli.c
+++ b/drivers/mmc/host/sdhci-pci-gli.c
@@ -220,6 +220,9 @@
 
 #define GLI_MAX_TUNING_LOOP 40
 
+static void gl9763e_set_low_power_negotiation(struct sdhci_pci_slot *slot,
+					      bool enable);
+
 /* Genesys Logic chipset */
 static inline void gl9750_wt_on(struct sdhci_host *host)
 {
@@ -1281,6 +1284,9 @@ static int gl9763e_add_host(struct sdhci_pci_slot *slot)
 	if (ret)
 		goto cleanup;
 
+	/* Disable LPM negotiation to avoid entering L1 state. */
+	gl9763e_set_low_power_negotiation(slot, false);
+
 	return 0;
 
 cleanup:
@@ -1323,7 +1329,6 @@ static void gli_set_gl9763e(struct sdhci_pci_slot *slot)
 	pci_write_config_dword(pdev, PCIE_GLI_9763E_VHS, value);
 }
 
-#ifdef CONFIG_PM
 static void gl9763e_set_low_power_negotiation(struct sdhci_pci_slot *slot, bool enable)
 {
 	struct pci_dev *pdev = slot->chip->pdev;
@@ -1349,6 +1354,7 @@ static void gl9763e_set_low_power_negotiation(struct sdhci_pci_slot *slot, bool
 	pci_write_config_dword(pdev, PCIE_GLI_9763E_VHS, value);
 }
 
+#ifdef CONFIG_PM
 static int gl9763e_runtime_suspend(struct sdhci_pci_chip *chip)
 {
 	struct sdhci_pci_slot *slot = chip->slots[0];
-- 
2.42.0.869.gea05f2083d-goog

Re: [PATCH] mmc: sdhci-pci-gli: Disable LPM during initialization

[Sven van Ashbrook]

Hi Kornel, see below.

On Thu, Nov 9, 2023 at 6:20 AM Kornel Dulęba <korneld@chromium.org> wrote:
>
> To address IO performance commit f9e5b33934ce
> ("mmc: host: Improve I/O read/write performance for GL9763E")
> limited LPM negotiation to runtime suspend state.
> The problem is that it only flips the switch in the runtime PM
> resume/suspend logic.
>
> Disable LPM negotiation in gl9763e_add_host.
> This helps in two ways:
> 1. It was found that the LPM switch stays in the same position after
>    warm reboot. Having it set in init helps with consistency.
> 2. Disabling LPM during the first runtime resume leaves us susceptible
>    to the performance issue in the time window between boot and the
>    first runtime suspend.
>
> Fixes: f9e5b33934ce ("mmc: host: Improve I/O read/write performance for GL9763E")
> Cc: stable@vger.kernel.org
> Signed-off-by: Kornel Dulęba <korneld@chromium.org>
> ---
>  drivers/mmc/host/sdhci-pci-gli.c | 8 +++++++-
>  1 file changed, 7 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/mmc/host/sdhci-pci-gli.c b/drivers/mmc/host/sdhci-pci-gli.c
> index d83261e857a5..ce91d1e63a8e 100644
> --- a/drivers/mmc/host/sdhci-pci-gli.c
> +++ b/drivers/mmc/host/sdhci-pci-gli.c
> @@ -220,6 +220,9 @@
>
>  #define GLI_MAX_TUNING_LOOP 40
>
> +static void gl9763e_set_low_power_negotiation(struct sdhci_pci_slot *slot,
> +                                             bool enable);
> +
>  /* Genesys Logic chipset */
>  static inline void gl9750_wt_on(struct sdhci_host *host)
>  {
> @@ -1281,6 +1284,9 @@ static int gl9763e_add_host(struct sdhci_pci_slot *slot)
>         if (ret)
>                 goto cleanup;
>
> +       /* Disable LPM negotiation to avoid entering L1 state. */
> +       gl9763e_set_low_power_negotiation(slot, false);
> +
>         return 0;

What happens if the bridge is not driving the system rootfs? Imagine
the case where
the bridge is used to drive an auxiliary eMMC, unused until a few hours
after boot. After this patch, the bridge may remain active (not-L1)
for the entire time,
although it's not being used...

I suspect we want the following:
1. consistency - LPM register setting and runtime_pm state must agree
2. power-efficient initial state - bridge must come out of probe
runtime-suspended
and LPM must be enabled

I suspect the above will be fulfilled if we do

+ /* Bring to consistent runtime suspended state with LPM negotiation enabled */
+ gl9763e_set_low_power_negotiation(slot, false);
+ pm_runtime_set_suspended(dev);

WDYT?

>
>  cleanup:
> @@ -1323,7 +1329,6 @@ static void gli_set_gl9763e(struct sdhci_pci_slot *slot)
>         pci_write_config_dword(pdev, PCIE_GLI_9763E_VHS, value);
>  }
>
> -#ifdef CONFIG_PM
>  static void gl9763e_set_low_power_negotiation(struct sdhci_pci_slot *slot, bool enable)
>  {
>         struct pci_dev *pdev = slot->chip->pdev;
> @@ -1349,6 +1354,7 @@ static void gl9763e_set_low_power_negotiation(struct sdhci_pci_slot *slot, bool
>         pci_write_config_dword(pdev, PCIE_GLI_9763E_VHS, value);
>  }
>
> +#ifdef CONFIG_PM
>  static int gl9763e_runtime_suspend(struct sdhci_pci_chip *chip)
>  {
>         struct sdhci_pci_slot *slot = chip->slots[0];
> --
> 2.42.0.869.gea05f2083d-goog
>

Re: [PATCH] mmc: sdhci-pci-gli: Disable LPM during initialization

[Kornel Dulęba]

Hi Sven,

>Hi Kornel, see below.
>
>On Thu, Nov 9, 2023 at 6:20 AM Kornel Dulęba <korneld@chromium.org> wrote:
>>
>> To address IO performance commit f9e5b33934ce
>> ("mmc: host: Improve I/O read/write performance for GL9763E")
>> limited LPM negotiation to runtime suspend state.
>> The problem is that it only flips the switch in the runtime PM
>> resume/suspend logic.
>>
>> Disable LPM negotiation in gl9763e_add_host.
>> This helps in two ways:
>> 1. It was found that the LPM switch stays in the same position after
>>    warm reboot. Having it set in init helps with consistency.
>> 2. Disabling LPM during the first runtime resume leaves us susceptible
>>    to the performance issue in the time window between boot and the
>>    first runtime suspend.
>>
>> Fixes: f9e5b33934ce ("mmc: host: Improve I/O read/write performance for GL9763E")
>> Cc: stable@vger.kernel.org
>> Signed-off-by: Kornel Dulęba <korneld@chromium.org>
>> ---
>>  drivers/mmc/host/sdhci-pci-gli.c | 8 +++++++-
>>  1 file changed, 7 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/mmc/host/sdhci-pci-gli.c b/drivers/mmc/host/sdhci-pci-gli.c
>> index d83261e857a5..ce91d1e63a8e 100644
>> --- a/drivers/mmc/host/sdhci-pci-gli.c
>> +++ b/drivers/mmc/host/sdhci-pci-gli.c
>> @@ -220,6 +220,9 @@
>>
>>  #define GLI_MAX_TUNING_LOOP 40
>>
>> +static void gl9763e_set_low_power_negotiation(struct sdhci_pci_slot *slot,
>> +                                             bool enable);
>> +
>>  /* Genesys Logic chipset */
>>  static inline void gl9750_wt_on(struct sdhci_host *host)
>>  {
>> @@ -1281,6 +1284,9 @@ static int gl9763e_add_host(struct sdhci_pci_slot *slot)
>>         if (ret)
>>                 goto cleanup;
>>
>> +       /* Disable LPM negotiation to avoid entering L1 state. */
>> +       gl9763e_set_low_power_negotiation(slot, false);
>> +
>>         return 0;
>
>What happens if the bridge is not driving the system rootfs? Imagine
>the case where
>the bridge is used to drive an auxiliary eMMC, unused until a few hours
>after boot. After this patch, the bridge may remain active (not-L1)
>for the entire time,
>although it's not being used...

That's already addressed by runtime PM. LPM negotiation will be
re-enabled duing the first runtime suspend. The default autosuspend
delay for all PCI MMC controllers is 50ms, so I think that's fine.
The only scenario where LPM will never be entered is if the user
explicitly disabled runtime PM for the controller. In that case however,
it's arguably better to have the LPM negotiation disabled for the sake 
of performance.

>
>I suspect we want the following:
>1. consistency - LPM register setting and runtime_pm state must agree
>2. power-efficient initial state - bridge must come out of probe
>runtime-suspended
>and LPM must be enabled
>
>I suspect the above will be fulfilled if we do
>
>+ /* Bring to consistent runtime suspended state with LPM negotiation enabled */
>+ gl9763e_set_low_power_negotiation(slot, false);
>+ pm_runtime_set_suspended(dev);
>
>WDYT?

I don't think this is something that we want do to. Apart from my
argument above there is one more thing to consider.
During runtime PM initialization in sdhci_pci_runtime_pm_allow
the usage counter is dropped using pm_runtime_put_noidle,
which doesn't trigger the machinery to suspend the device.
According to the comment that's because the mmc core logic will shortly
talk to the device, probably to initialize the eMMC card itself.

>
>>
>>  cleanup:
>> @@ -1323,7 +1329,6 @@ static void gli_set_gl9763e(struct sdhci_pci_slot *slot)
>>         pci_write_config_dword(pdev, PCIE_GLI_9763E_VHS, value);
>>  }
>>
>> -#ifdef CONFIG_PM
>>  static void gl9763e_set_low_power_negotiation(struct sdhci_pci_slot *slot, bool enable)
>>  {
>>         struct pci_dev *pdev = slot->chip->pdev;
>> @@ -1349,6 +1354,7 @@ static void gl9763e_set_low_power_negotiation(struct sdhci_pci_slot *slot, bool
>>         pci_write_config_dword(pdev, PCIE_GLI_9763E_VHS, value);
>>  }
>>
>> +#ifdef CONFIG_PM
>>  static int gl9763e_runtime_suspend(struct sdhci_pci_chip *chip)
>>  {
>>         struct sdhci_pci_slot *slot = chip->slots[0];
>> --
>> 2.42.0.869.gea05f2083d-goog
>>

Re: [PATCH] mmc: sdhci-pci-gli: Disable LPM during initialization

[Sven van Ashbrook]

There's something happening in this driver that doesn't
make much sense to me.

According to the pm runtime docs [1] the initial runtime pm
status of all devices is 'suspended'. Which I presume, means:
if the driver doesn't use any of the pm_runtime_*() functions
to tell the core "actually, I am active after probe", then the
device remains suspended until explicitly going active, at which
point the runtime_resume() callback is invoked.

That's the theory. In practice, what do I see on a device
containing this bridge?
Intel SoC <-> PCIe bus <-> gl9763e bridge <-> eMMC bus <-> eMMC drive

at probe() (does not exist in this driver so I stubbed it):
[ 0.601542] runtime pm is enabled = 1 (disable_depth == 0)
[ 0.601552] runtime pm is active = 2 (usage_count)

at probe_slot():
[ 0.602024] runtime pm is enabled = 1
[ 0.602027] runtime pm is active = 2

At add_host():
[ 0.602804] runtime pm is enabled = 1
[ 0.602809] runtime pm is active = 3

It looks like:
- nowhere does the gl9763e driver inform runtime pm it's active
- the device is active in probe(), probe_slot() and add_host()
- the runtime_resume() callback did not get called before
probe(), probe_slot(), or add_host().

Why is the runtime_resume() callback not invoked?
Does the driver have a runtime_pm misconfiguration issue here?

Perhaps Rafael could clarify?

[1] https://git.kernel.org/pub/scm/linux/kernel/git/stable/linux.git/tree/Documentation/power/runtime_pm.rst?h=v6.6.1#n563

Re: [PATCH] mmc: sdhci-pci-gli: Disable LPM during initialization

[Adrian Hunter]

On 10/11/23 18:58, Sven van Ashbrook wrote:
> There's something happening in this driver that doesn't
> make much sense to me.
> 
> According to the pm runtime docs [1] the initial runtime pm
> status of all devices is 'suspended'. Which I presume, means:
> if the driver doesn't use any of the pm_runtime_*() functions
> to tell the core "actually, I am active after probe", then the
> device remains suspended until explicitly going active, at which
> point the runtime_resume() callback is invoked.
> 
> That's the theory. In practice, what do I see on a device
> containing this bridge?
> Intel SoC <-> PCIe bus <-> gl9763e bridge <-> eMMC bus <-> eMMC drive
> 
> at probe() (does not exist in this driver so I stubbed it):
> [ 0.601542] runtime pm is enabled = 1 (disable_depth == 0)
> [ 0.601552] runtime pm is active = 2 (usage_count)
> 
> at probe_slot():
> [ 0.602024] runtime pm is enabled = 1
> [ 0.602027] runtime pm is active = 2
> 
> At add_host():
> [ 0.602804] runtime pm is enabled = 1
> [ 0.602809] runtime pm is active = 3
> 
> It looks like:
> - nowhere does the gl9763e driver inform runtime pm it's active

PCI subsystem does it in pci_pm_init()

> - the device is active in probe(), probe_slot() and add_host()
> - the runtime_resume() callback did not get called before
> probe(), probe_slot(), or add_host().
> 
> Why is the runtime_resume() callback not invoked?

Most drivers expect the device to be active at probe().  How it
gets that way is up to the bus.  Note, the driver may call 
pm_runtime_set_active() but that doesn't call runtime_resume().

> Does the driver have a runtime_pm misconfiguration issue here?

No

> 
> Perhaps Rafael could clarify?
> 
> [1] https://git.kernel.org/pub/scm/linux/kernel/git/stable/linux.git/tree/Documentation/power/runtime_pm.rst?h=v6.6.1#n563

Re: [PATCH] mmc: sdhci-pci-gli: Disable LPM during initialization

[Sven van Ashbrook]

Copypasta issue, I really meant to write:

+ /* Bring to consistent runtime suspended state with LPM negotiation enabled */
+ gl9763e_set_low_power_negotiation(slot, true);
+ pm_runtime_set_suspended(dev);

Re: [PATCH] mmc: sdhci-pci-gli: Disable LPM during initialization

[Adrian Hunter]

On 9/11/23 13:19, Kornel Dulęba wrote:
> To address IO performance commit f9e5b33934ce
> ("mmc: host: Improve I/O read/write performance for GL9763E")
> limited LPM negotiation to runtime suspend state.
> The problem is that it only flips the switch in the runtime PM
> resume/suspend logic.
> 
> Disable LPM negotiation in gl9763e_add_host.
> This helps in two ways:
> 1. It was found that the LPM switch stays in the same position after
>    warm reboot. Having it set in init helps with consistency.
> 2. Disabling LPM during the first runtime resume leaves us susceptible
>    to the performance issue in the time window between boot and the
>    first runtime suspend.
> 
> Fixes: f9e5b33934ce ("mmc: host: Improve I/O read/write performance for GL9763E")
> Cc: stable@vger.kernel.org
> Signed-off-by: Kornel Dulęba <korneld@chromium.org>
> ---
>  drivers/mmc/host/sdhci-pci-gli.c | 8 +++++++-
>  1 file changed, 7 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/mmc/host/sdhci-pci-gli.c b/drivers/mmc/host/sdhci-pci-gli.c
> index d83261e857a5..ce91d1e63a8e 100644
> --- a/drivers/mmc/host/sdhci-pci-gli.c
> +++ b/drivers/mmc/host/sdhci-pci-gli.c
> @@ -220,6 +220,9 @@
>  
>  #define GLI_MAX_TUNING_LOOP 40
>  
> +static void gl9763e_set_low_power_negotiation(struct sdhci_pci_slot *slot,
> +					      bool enable);

Kernel-style is to move the whole function to prevent the need
for forward declaration.

> +
>  /* Genesys Logic chipset */
>  static inline void gl9750_wt_on(struct sdhci_host *host)
>  {
> @@ -1281,6 +1284,9 @@ static int gl9763e_add_host(struct sdhci_pci_slot *slot)
>  	if (ret)
>  		goto cleanup;
>  
> +	/* Disable LPM negotiation to avoid entering L1 state. */
> +	gl9763e_set_low_power_negotiation(slot, false);
> +
>  	return 0;
>  
>  cleanup:
> @@ -1323,7 +1329,6 @@ static void gli_set_gl9763e(struct sdhci_pci_slot *slot)
>  	pci_write_config_dword(pdev, PCIE_GLI_9763E_VHS, value);
>  }
>  
> -#ifdef CONFIG_PM
>  static void gl9763e_set_low_power_negotiation(struct sdhci_pci_slot *slot, bool enable)
>  {
>  	struct pci_dev *pdev = slot->chip->pdev;
> @@ -1349,6 +1354,7 @@ static void gl9763e_set_low_power_negotiation(struct sdhci_pci_slot *slot, bool
>  	pci_write_config_dword(pdev, PCIE_GLI_9763E_VHS, value);
>  }
>  
> +#ifdef CONFIG_PM
>  static int gl9763e_runtime_suspend(struct sdhci_pci_chip *chip)
>  {
>  	struct sdhci_pci_slot *slot = chip->slots[0];