[PATCH 1/2] mmc: sdhci-pci-gli: GL9767: Fix low power mode on the set clock function

[PATCH 1/2] mmc: sdhci-pci-gli: GL9767: Fix low power mode on the set clock function

[Ben Chuang]

From: Ben Chuang <ben.chuang@genesyslogic.com.tw>

On sdhci_gl9767_set_clock(), the vendor header space(VHS) is read-only
after calling gl9767_disable_ssc_pll() and gl9767_set_ssc_pll_205mhz().
So the low power negotiation mode cannot be enabled again.
Introduce gl9767_set_low_power_negotiation() function to fix it.

The explanation process is as below.

static void sdhci_gl9767_set_clock()
{
	...
        gl9767_vhs_write();
        ...
	value |= PCIE_GLI_9767_CFG_LOW_PWR_OFF;
        pci_write_config_dword(pdev, PCIE_GLI_9767_CFG, value); <--- (a)

        gl9767_disable_ssc_pll(); <--- (b)
        sdhci_writew(host, 0, SDHCI_CLOCK_CONTROL);

        if (clock == 0)
                return;  <-- (I)

	...
        if (clock == 200000000 && ios->timing == MMC_TIMING_UHS_SDR104) {
		...
                gl9767_set_ssc_pll_205mhz(); <--- (c)
        }
	...
	value &= ~PCIE_GLI_9767_CFG_LOW_PWR_OFF;
        pci_write_config_dword(pdev, PCIE_GLI_9767_CFG, value); <-- (II)
        gl9767_vhs_read();
}

(a) disable low power negotiation mode. When return on (I), the low power
mode is disabled.  After (b) and (c), VHS is read-only, the low power mode
cannot be enabled on (II).

Fixes: d2754355512e ("mmc: sdhci-pci-gli: Set SDR104's clock to 205MHz and enable SSC for GL9767")
Signed-off-by: Ben Chuang <benchuanggli@gmail.com>
Signed-off-by: Ben Chuang <ben.chuang@genesyslogic.com.tw>
---
 drivers/mmc/host/sdhci-pci-gli.c | 35 +++++++++++++++++++-------------
 1 file changed, 21 insertions(+), 14 deletions(-)

diff --git a/drivers/mmc/host/sdhci-pci-gli.c b/drivers/mmc/host/sdhci-pci-gli.c
index 0f81586a19df..22a927ce2c88 100644
--- a/drivers/mmc/host/sdhci-pci-gli.c
+++ b/drivers/mmc/host/sdhci-pci-gli.c
@@ -892,28 +892,40 @@ static void gl9767_disable_ssc_pll(struct pci_dev *pdev)
 	gl9767_vhs_read(pdev);
 }
 
+static void gl9767_set_low_power_negotiation(struct pci_dev *pdev, bool enable)
+{
+	u32 value;
+
+	gl9767_vhs_write(pdev);
+
+	pci_read_config_dword(pdev, PCIE_GLI_9767_CFG, &value);
+	if (enable)
+		value &= ~PCIE_GLI_9767_CFG_LOW_PWR_OFF;
+	else
+		value |= PCIE_GLI_9767_CFG_LOW_PWR_OFF;
+	pci_write_config_dword(pdev, PCIE_GLI_9767_CFG, value);
+
+	gl9767_vhs_read(pdev);
+}
+
 static void sdhci_gl9767_set_clock(struct sdhci_host *host, unsigned int clock)
 {
 	struct sdhci_pci_slot *slot = sdhci_priv(host);
 	struct mmc_ios *ios = &host->mmc->ios;
 	struct pci_dev *pdev;
-	u32 value;
 	u16 clk;
 
 	pdev = slot->chip->pdev;
 	host->mmc->actual_clock = 0;
 
-	gl9767_vhs_write(pdev);
-
-	pci_read_config_dword(pdev, PCIE_GLI_9767_CFG, &value);
-	value |= PCIE_GLI_9767_CFG_LOW_PWR_OFF;
-	pci_write_config_dword(pdev, PCIE_GLI_9767_CFG, value);
-
+	gl9767_set_low_power_negotiation(pdev, false);
 	gl9767_disable_ssc_pll(pdev);
 	sdhci_writew(host, 0, SDHCI_CLOCK_CONTROL);
 
-	if (clock == 0)
+	if (clock == 0) {
+		gl9767_set_low_power_negotiation(pdev, true);
 		return;
+	}
 
 	clk = sdhci_calc_clk(host, clock, &host->mmc->actual_clock);
 	if (clock == 200000000 && ios->timing == MMC_TIMING_UHS_SDR104) {
@@ -922,12 +934,7 @@ static void sdhci_gl9767_set_clock(struct sdhci_host *host, unsigned int clock)
 	}
 
 	sdhci_enable_clk(host, clk);
-
-	pci_read_config_dword(pdev, PCIE_GLI_9767_CFG, &value);
-	value &= ~PCIE_GLI_9767_CFG_LOW_PWR_OFF;
-	pci_write_config_dword(pdev, PCIE_GLI_9767_CFG, value);
-
-	gl9767_vhs_read(pdev);
+	gl9767_set_low_power_negotiation(pdev, true);
 }
 
 static void gli_set_9767(struct sdhci_host *host)
-- 
2.47.0

[PATCH 2/2] mmc: sdhci-pci-gli: GL9767: Fix low power mode in the SD Express process

[Ben Chuang]

From: Ben Chuang <ben.chuang@genesyslogic.com.tw>

When starting the SD Express process, the low power negotiation mode will
be disabled, so we need to re-enable it after switching back to SD mode.

Fixes: 0e92aec2efa0 ("mmc: sdhci-pci-gli: Add support SD Express card for GL9767")
Signed-off-by: Ben Chuang <benchuanggli@gmail.com>
Signed-off-by: Ben Chuang <ben.chuang@genesyslogic.com.tw>
---
 drivers/mmc/host/sdhci-pci-gli.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/mmc/host/sdhci-pci-gli.c b/drivers/mmc/host/sdhci-pci-gli.c
index 22a927ce2c88..68ce4920e01e 100644
--- a/drivers/mmc/host/sdhci-pci-gli.c
+++ b/drivers/mmc/host/sdhci-pci-gli.c
@@ -1068,6 +1068,9 @@ static int gl9767_init_sd_express(struct mmc_host *mmc, struct mmc_ios *ios)
 		sdhci_writew(host, value, SDHCI_CLOCK_CONTROL);
 	}
 
+	pci_read_config_dword(pdev, PCIE_GLI_9767_CFG, &value);
+	value &= ~PCIE_GLI_9767_CFG_LOW_PWR_OFF;
+	pci_write_config_dword(pdev, PCIE_GLI_9767_CFG, value);
 	gl9767_vhs_read(pdev);
 
 	return 0;
-- 
2.47.0

Re: [PATCH 1/2] mmc: sdhci-pci-gli: GL9767: Fix low power mode on the set clock function

[Ulf Hansson]

+ Georg

On Fri, 25 Oct 2024 at 08:01, Ben Chuang <benchuanggli@gmail.com> wrote:
>
> From: Ben Chuang <ben.chuang@genesyslogic.com.tw>
>
> On sdhci_gl9767_set_clock(), the vendor header space(VHS) is read-only
> after calling gl9767_disable_ssc_pll() and gl9767_set_ssc_pll_205mhz().
> So the low power negotiation mode cannot be enabled again.
> Introduce gl9767_set_low_power_negotiation() function to fix it.
>
> The explanation process is as below.
>
> static void sdhci_gl9767_set_clock()
> {
>         ...
>         gl9767_vhs_write();
>         ...
>         value |= PCIE_GLI_9767_CFG_LOW_PWR_OFF;
>         pci_write_config_dword(pdev, PCIE_GLI_9767_CFG, value); <--- (a)
>
>         gl9767_disable_ssc_pll(); <--- (b)
>         sdhci_writew(host, 0, SDHCI_CLOCK_CONTROL);
>
>         if (clock == 0)
>                 return;  <-- (I)
>
>         ...
>         if (clock == 200000000 && ios->timing == MMC_TIMING_UHS_SDR104) {
>                 ...
>                 gl9767_set_ssc_pll_205mhz(); <--- (c)
>         }
>         ...
>         value &= ~PCIE_GLI_9767_CFG_LOW_PWR_OFF;
>         pci_write_config_dword(pdev, PCIE_GLI_9767_CFG, value); <-- (II)
>         gl9767_vhs_read();
> }
>
> (a) disable low power negotiation mode. When return on (I), the low power
> mode is disabled.  After (b) and (c), VHS is read-only, the low power mode
> cannot be enabled on (II).
>
> Fixes: d2754355512e ("mmc: sdhci-pci-gli: Set SDR104's clock to 205MHz and enable SSC for GL9767")

Is this the same problem as being reported in
https://lore.kernel.org/all/41c1c88a-b2c9-4c05-863a-467785027f49@tuxedocomputers.com/

?

> Signed-off-by: Ben Chuang <benchuanggli@gmail.com>

Not sure the above SoB makes sense. The below is perfectly sufficient, right?

> Signed-off-by: Ben Chuang <ben.chuang@genesyslogic.com.tw>

Kind regards
Uffe


> ---
>  drivers/mmc/host/sdhci-pci-gli.c | 35 +++++++++++++++++++-------------
>  1 file changed, 21 insertions(+), 14 deletions(-)
>
> diff --git a/drivers/mmc/host/sdhci-pci-gli.c b/drivers/mmc/host/sdhci-pci-gli.c
> index 0f81586a19df..22a927ce2c88 100644
> --- a/drivers/mmc/host/sdhci-pci-gli.c
> +++ b/drivers/mmc/host/sdhci-pci-gli.c
> @@ -892,28 +892,40 @@ static void gl9767_disable_ssc_pll(struct pci_dev *pdev)
>         gl9767_vhs_read(pdev);
>  }
>
> +static void gl9767_set_low_power_negotiation(struct pci_dev *pdev, bool enable)
> +{
> +       u32 value;
> +
> +       gl9767_vhs_write(pdev);
> +
> +       pci_read_config_dword(pdev, PCIE_GLI_9767_CFG, &value);
> +       if (enable)
> +               value &= ~PCIE_GLI_9767_CFG_LOW_PWR_OFF;
> +       else
> +               value |= PCIE_GLI_9767_CFG_LOW_PWR_OFF;
> +       pci_write_config_dword(pdev, PCIE_GLI_9767_CFG, value);
> +
> +       gl9767_vhs_read(pdev);
> +}
> +
>  static void sdhci_gl9767_set_clock(struct sdhci_host *host, unsigned int clock)
>  {
>         struct sdhci_pci_slot *slot = sdhci_priv(host);
>         struct mmc_ios *ios = &host->mmc->ios;
>         struct pci_dev *pdev;
> -       u32 value;
>         u16 clk;
>
>         pdev = slot->chip->pdev;
>         host->mmc->actual_clock = 0;
>
> -       gl9767_vhs_write(pdev);
> -
> -       pci_read_config_dword(pdev, PCIE_GLI_9767_CFG, &value);
> -       value |= PCIE_GLI_9767_CFG_LOW_PWR_OFF;
> -       pci_write_config_dword(pdev, PCIE_GLI_9767_CFG, value);
> -
> +       gl9767_set_low_power_negotiation(pdev, false);
>         gl9767_disable_ssc_pll(pdev);
>         sdhci_writew(host, 0, SDHCI_CLOCK_CONTROL);
>
> -       if (clock == 0)
> +       if (clock == 0) {
> +               gl9767_set_low_power_negotiation(pdev, true);
>                 return;
> +       }
>
>         clk = sdhci_calc_clk(host, clock, &host->mmc->actual_clock);
>         if (clock == 200000000 && ios->timing == MMC_TIMING_UHS_SDR104) {
> @@ -922,12 +934,7 @@ static void sdhci_gl9767_set_clock(struct sdhci_host *host, unsigned int clock)
>         }
>
>         sdhci_enable_clk(host, clk);
> -
> -       pci_read_config_dword(pdev, PCIE_GLI_9767_CFG, &value);
> -       value &= ~PCIE_GLI_9767_CFG_LOW_PWR_OFF;
> -       pci_write_config_dword(pdev, PCIE_GLI_9767_CFG, value);
> -
> -       gl9767_vhs_read(pdev);
> +       gl9767_set_low_power_negotiation(pdev, true);
>  }
>
>  static void gli_set_9767(struct sdhci_host *host)
> --
> 2.47.0
>

Re: [PATCH 1/2] mmc: sdhci-pci-gli: GL9767: Fix low power mode on the set clock function

[Georg Gottleuber]

Hello Ben, hello Uffe,

thank you for this fix.

Am 25.10.24 um 15:22 schrieb Ulf Hansson:
> + Georg
> 
> On Fri, 25 Oct 2024 at 08:01, Ben Chuang <benchuanggli@gmail.com> wrote:
>>
>> From: Ben Chuang <ben.chuang@genesyslogic.com.tw>
>>
>> On sdhci_gl9767_set_clock(), the vendor header space(VHS) is read-only
>> after calling gl9767_disable_ssc_pll() and gl9767_set_ssc_pll_205mhz().
>> So the low power negotiation mode cannot be enabled again.
>> Introduce gl9767_set_low_power_negotiation() function to fix it.
>>
>> The explanation process is as below.
>>
>> static void sdhci_gl9767_set_clock()
>> {
>>         ...
>>         gl9767_vhs_write();
>>         ...
>>         value |= PCIE_GLI_9767_CFG_LOW_PWR_OFF;
>>         pci_write_config_dword(pdev, PCIE_GLI_9767_CFG, value); <--- (a)
>>
>>         gl9767_disable_ssc_pll(); <--- (b)
>>         sdhci_writew(host, 0, SDHCI_CLOCK_CONTROL);
>>
>>         if (clock == 0)
>>                 return;  <-- (I)
>>
>>         ...
>>         if (clock == 200000000 && ios->timing == MMC_TIMING_UHS_SDR104) {
>>                 ...
>>                 gl9767_set_ssc_pll_205mhz(); <--- (c)
>>         }
>>         ...
>>         value &= ~PCIE_GLI_9767_CFG_LOW_PWR_OFF;
>>         pci_write_config_dword(pdev, PCIE_GLI_9767_CFG, value); <-- (II)
>>         gl9767_vhs_read();
>> }
>>
>> (a) disable low power negotiation mode. When return on (I), the low power
>> mode is disabled.  After (b) and (c), VHS is read-only, the low power mode
>> cannot be enabled on (II).
>>
>> Fixes: d2754355512e ("mmc: sdhci-pci-gli: Set SDR104's clock to 205MHz and enable SSC for GL9767")
> 
> Is this the same problem as being reported in
> https://lore.kernel.org/all/41c1c88a-b2c9-4c05-863a-467785027f49@tuxedocomputers.com/
> 
> ?

Yes, this patch fixes
https://bugzilla.kernel.org/show_bug.cgi?id=219284

This makes my patch obsolete.

Kind regards,
Georg

Re: [PATCH 1/2] mmc: sdhci-pci-gli: GL9767: Fix low power mode on the set clock function

[Ben Chuang]

On Fri, Oct 25, 2024 at 9:23 PM Ulf Hansson <ulf.hansson@linaro.org> wrote:
>
> + Georg
>
> On Fri, 25 Oct 2024 at 08:01, Ben Chuang <benchuanggli@gmail.com> wrote:
> >
> > From: Ben Chuang <ben.chuang@genesyslogic.com.tw>
> >
> > On sdhci_gl9767_set_clock(), the vendor header space(VHS) is read-only
> > after calling gl9767_disable_ssc_pll() and gl9767_set_ssc_pll_205mhz().
> > So the low power negotiation mode cannot be enabled again.
> > Introduce gl9767_set_low_power_negotiation() function to fix it.
> >
> > The explanation process is as below.
> >
> > static void sdhci_gl9767_set_clock()
> > {
> >         ...
> >         gl9767_vhs_write();
> >         ...
> >         value |= PCIE_GLI_9767_CFG_LOW_PWR_OFF;
> >         pci_write_config_dword(pdev, PCIE_GLI_9767_CFG, value); <--- (a)
> >
> >         gl9767_disable_ssc_pll(); <--- (b)
> >         sdhci_writew(host, 0, SDHCI_CLOCK_CONTROL);
> >
> >         if (clock == 0)
> >                 return;  <-- (I)
> >
> >         ...
> >         if (clock == 200000000 && ios->timing == MMC_TIMING_UHS_SDR104) {
> >                 ...
> >                 gl9767_set_ssc_pll_205mhz(); <--- (c)
> >         }
> >         ...
> >         value &= ~PCIE_GLI_9767_CFG_LOW_PWR_OFF;
> >         pci_write_config_dword(pdev, PCIE_GLI_9767_CFG, value); <-- (II)
> >         gl9767_vhs_read();
> > }
> >
> > (a) disable low power negotiation mode. When return on (I), the low power
> > mode is disabled.  After (b) and (c), VHS is read-only, the low power mode
> > cannot be enabled on (II).
> >
> > Fixes: d2754355512e ("mmc: sdhci-pci-gli: Set SDR104's clock to 205MHz and enable SSC for GL9767")
>
> Is this the same problem as being reported in
> https://lore.kernel.org/all/41c1c88a-b2c9-4c05-863a-467785027f49@tuxedocomputers.com/
>
> ?
>
> > Signed-off-by: Ben Chuang <benchuanggli@gmail.com>
>
> Not sure the above SoB makes sense. The below is perfectly sufficient, right?

Yes, just keep the company's SOB. Thanks.

Best regards,
Ben Chuang

>
> > Signed-off-by: Ben Chuang <ben.chuang@genesyslogic.com.tw>
>
> Kind regards
> Uffe
>
>
> > ---
> >  drivers/mmc/host/sdhci-pci-gli.c | 35 +++++++++++++++++++-------------
> >  1 file changed, 21 insertions(+), 14 deletions(-)
> >
> > diff --git a/drivers/mmc/host/sdhci-pci-gli.c b/drivers/mmc/host/sdhci-pci-gli.c
> > index 0f81586a19df..22a927ce2c88 100644
> > --- a/drivers/mmc/host/sdhci-pci-gli.c
> > +++ b/drivers/mmc/host/sdhci-pci-gli.c
> > @@ -892,28 +892,40 @@ static void gl9767_disable_ssc_pll(struct pci_dev *pdev)
> >         gl9767_vhs_read(pdev);
> >  }
> >
> > +static void gl9767_set_low_power_negotiation(struct pci_dev *pdev, bool enable)
> > +{
> > +       u32 value;
> > +
> > +       gl9767_vhs_write(pdev);
> > +
> > +       pci_read_config_dword(pdev, PCIE_GLI_9767_CFG, &value);
> > +       if (enable)
> > +               value &= ~PCIE_GLI_9767_CFG_LOW_PWR_OFF;
> > +       else
> > +               value |= PCIE_GLI_9767_CFG_LOW_PWR_OFF;
> > +       pci_write_config_dword(pdev, PCIE_GLI_9767_CFG, value);
> > +
> > +       gl9767_vhs_read(pdev);
> > +}
> > +
> >  static void sdhci_gl9767_set_clock(struct sdhci_host *host, unsigned int clock)
> >  {
> >         struct sdhci_pci_slot *slot = sdhci_priv(host);
> >         struct mmc_ios *ios = &host->mmc->ios;
> >         struct pci_dev *pdev;
> > -       u32 value;
> >         u16 clk;
> >
> >         pdev = slot->chip->pdev;
> >         host->mmc->actual_clock = 0;
> >
> > -       gl9767_vhs_write(pdev);
> > -
> > -       pci_read_config_dword(pdev, PCIE_GLI_9767_CFG, &value);
> > -       value |= PCIE_GLI_9767_CFG_LOW_PWR_OFF;
> > -       pci_write_config_dword(pdev, PCIE_GLI_9767_CFG, value);
> > -
> > +       gl9767_set_low_power_negotiation(pdev, false);
> >         gl9767_disable_ssc_pll(pdev);
> >         sdhci_writew(host, 0, SDHCI_CLOCK_CONTROL);
> >
> > -       if (clock == 0)
> > +       if (clock == 0) {
> > +               gl9767_set_low_power_negotiation(pdev, true);
> >                 return;
> > +       }
> >
> >         clk = sdhci_calc_clk(host, clock, &host->mmc->actual_clock);
> >         if (clock == 200000000 && ios->timing == MMC_TIMING_UHS_SDR104) {
> > @@ -922,12 +934,7 @@ static void sdhci_gl9767_set_clock(struct sdhci_host *host, unsigned int clock)
> >         }
> >
> >         sdhci_enable_clk(host, clk);
> > -
> > -       pci_read_config_dword(pdev, PCIE_GLI_9767_CFG, &value);
> > -       value &= ~PCIE_GLI_9767_CFG_LOW_PWR_OFF;
> > -       pci_write_config_dword(pdev, PCIE_GLI_9767_CFG, value);
> > -
> > -       gl9767_vhs_read(pdev);
> > +       gl9767_set_low_power_negotiation(pdev, true);
> >  }
> >
> >  static void gli_set_9767(struct sdhci_host *host)
> > --
> > 2.47.0
> >

Re: [PATCH 1/2] mmc: sdhci-pci-gli: GL9767: Fix low power mode on the set clock function

[Ben Chuang]

Hi Georg,

Thanks for your test.
If there is no confidential information,
Can you share some pci/acpi configuration or the s0ix self test log on
AMD/Intel laptop?

Best regards,
Ben Chuang

On Fri, Oct 25, 2024 at 11:40 PM Georg Gottleuber
<ggo@tuxedocomputers.com> wrote:
>
> Hello Ben, hello Uffe,
>
> thank you for this fix.
>
> Am 25.10.24 um 15:22 schrieb Ulf Hansson:
> > + Georg
> >
> > On Fri, 25 Oct 2024 at 08:01, Ben Chuang <benchuanggli@gmail.com> wrote:
> >>
> >> From: Ben Chuang <ben.chuang@genesyslogic.com.tw>
> >>
> >> On sdhci_gl9767_set_clock(), the vendor header space(VHS) is read-only
> >> after calling gl9767_disable_ssc_pll() and gl9767_set_ssc_pll_205mhz().
> >> So the low power negotiation mode cannot be enabled again.
> >> Introduce gl9767_set_low_power_negotiation() function to fix it.
> >>
> >> The explanation process is as below.
> >>
> >> static void sdhci_gl9767_set_clock()
> >> {
> >>         ...
> >>         gl9767_vhs_write();
> >>         ...
> >>         value |= PCIE_GLI_9767_CFG_LOW_PWR_OFF;
> >>         pci_write_config_dword(pdev, PCIE_GLI_9767_CFG, value); <--- (a)
> >>
> >>         gl9767_disable_ssc_pll(); <--- (b)
> >>         sdhci_writew(host, 0, SDHCI_CLOCK_CONTROL);
> >>
> >>         if (clock == 0)
> >>                 return;  <-- (I)
> >>
> >>         ...
> >>         if (clock == 200000000 && ios->timing == MMC_TIMING_UHS_SDR104) {
> >>                 ...
> >>                 gl9767_set_ssc_pll_205mhz(); <--- (c)
> >>         }
> >>         ...
> >>         value &= ~PCIE_GLI_9767_CFG_LOW_PWR_OFF;
> >>         pci_write_config_dword(pdev, PCIE_GLI_9767_CFG, value); <-- (II)
> >>         gl9767_vhs_read();
> >> }
> >>
> >> (a) disable low power negotiation mode. When return on (I), the low power
> >> mode is disabled.  After (b) and (c), VHS is read-only, the low power mode
> >> cannot be enabled on (II).
> >>
> >> Fixes: d2754355512e ("mmc: sdhci-pci-gli: Set SDR104's clock to 205MHz and enable SSC for GL9767")
> >
> > Is this the same problem as being reported in
> > https://lore.kernel.org/all/41c1c88a-b2c9-4c05-863a-467785027f49@tuxedocomputers.com/
> >
> > ?
>
> Yes, this patch fixes
> https://bugzilla.kernel.org/show_bug.cgi?id=219284
>
> This makes my patch obsolete.
>
> Kind regards,
> Georg

Re: [PATCH 1/2] mmc: sdhci-pci-gli: GL9767: Fix low power mode on the set clock function

[Ulf Hansson]

On Fri, 25 Oct 2024 at 17:40, Georg Gottleuber <ggo@tuxedocomputers.com> wrote:
>
> Hello Ben, hello Uffe,
>
> thank you for this fix.
>
> Am 25.10.24 um 15:22 schrieb Ulf Hansson:
> > + Georg
> >
> > On Fri, 25 Oct 2024 at 08:01, Ben Chuang <benchuanggli@gmail.com> wrote:
> >>
> >> From: Ben Chuang <ben.chuang@genesyslogic.com.tw>
> >>
> >> On sdhci_gl9767_set_clock(), the vendor header space(VHS) is read-only
> >> after calling gl9767_disable_ssc_pll() and gl9767_set_ssc_pll_205mhz().
> >> So the low power negotiation mode cannot be enabled again.
> >> Introduce gl9767_set_low_power_negotiation() function to fix it.
> >>
> >> The explanation process is as below.
> >>
> >> static void sdhci_gl9767_set_clock()
> >> {
> >>         ...
> >>         gl9767_vhs_write();
> >>         ...
> >>         value |= PCIE_GLI_9767_CFG_LOW_PWR_OFF;
> >>         pci_write_config_dword(pdev, PCIE_GLI_9767_CFG, value); <--- (a)
> >>
> >>         gl9767_disable_ssc_pll(); <--- (b)
> >>         sdhci_writew(host, 0, SDHCI_CLOCK_CONTROL);
> >>
> >>         if (clock == 0)
> >>                 return;  <-- (I)
> >>
> >>         ...
> >>         if (clock == 200000000 && ios->timing == MMC_TIMING_UHS_SDR104) {
> >>                 ...
> >>                 gl9767_set_ssc_pll_205mhz(); <--- (c)
> >>         }
> >>         ...
> >>         value &= ~PCIE_GLI_9767_CFG_LOW_PWR_OFF;
> >>         pci_write_config_dword(pdev, PCIE_GLI_9767_CFG, value); <-- (II)
> >>         gl9767_vhs_read();
> >> }
> >>
> >> (a) disable low power negotiation mode. When return on (I), the low power
> >> mode is disabled.  After (b) and (c), VHS is read-only, the low power mode
> >> cannot be enabled on (II).
> >>
> >> Fixes: d2754355512e ("mmc: sdhci-pci-gli: Set SDR104's clock to 205MHz and enable SSC for GL9767")
> >
> > Is this the same problem as being reported in
> > https://lore.kernel.org/all/41c1c88a-b2c9-4c05-863a-467785027f49@tuxedocomputers.com/
> >
> > ?
>
> Yes, this patch fixes
> https://bugzilla.kernel.org/show_bug.cgi?id=219284
>
> This makes my patch obsolete.

Thanks to both of you for helping out and fixing the problem!

I added Georg's reported/tested-by tag when applying and queued this
up as a fix with a stable tag.

Kind regards
Uffe

Re: [PATCH 2/2] mmc: sdhci-pci-gli: GL9767: Fix low power mode in the SD Express process

[Ulf Hansson]

On Fri, 25 Oct 2024 at 08:01, Ben Chuang <benchuanggli@gmail.com> wrote:
>
> From: Ben Chuang <ben.chuang@genesyslogic.com.tw>
>
> When starting the SD Express process, the low power negotiation mode will
> be disabled, so we need to re-enable it after switching back to SD mode.
>
> Fixes: 0e92aec2efa0 ("mmc: sdhci-pci-gli: Add support SD Express card for GL9767")
> Signed-off-by: Ben Chuang <benchuanggli@gmail.com>

I am dropping this SoB, as we agreed for patch1 too.

> Signed-off-by: Ben Chuang <ben.chuang@genesyslogic.com.tw>

Applied for fixes and by adding a stable tag, thanks!

Kind regards
Uffe


> ---
>  drivers/mmc/host/sdhci-pci-gli.c | 3 +++
>  1 file changed, 3 insertions(+)
>
> diff --git a/drivers/mmc/host/sdhci-pci-gli.c b/drivers/mmc/host/sdhci-pci-gli.c
> index 22a927ce2c88..68ce4920e01e 100644
> --- a/drivers/mmc/host/sdhci-pci-gli.c
> +++ b/drivers/mmc/host/sdhci-pci-gli.c
> @@ -1068,6 +1068,9 @@ static int gl9767_init_sd_express(struct mmc_host *mmc, struct mmc_ios *ios)
>                 sdhci_writew(host, value, SDHCI_CLOCK_CONTROL);
>         }
>
> +       pci_read_config_dword(pdev, PCIE_GLI_9767_CFG, &value);
> +       value &= ~PCIE_GLI_9767_CFG_LOW_PWR_OFF;
> +       pci_write_config_dword(pdev, PCIE_GLI_9767_CFG, value);
>         gl9767_vhs_read(pdev);
>
>         return 0;
> --
> 2.47.0
>

Re: [PATCH 1/2] mmc: sdhci-pci-gli: GL9767: Fix low power mode on the set clock function

[Ben Chuang]

On Mon, Oct 28, 2024 at 7:44 PM Ulf Hansson <ulf.hansson@linaro.org> wrote:
>
> On Fri, 25 Oct 2024 at 17:40, Georg Gottleuber <ggo@tuxedocomputers.com> wrote:
> >
> > Hello Ben, hello Uffe,
> >
> > thank you for this fix.
> >
> > Am 25.10.24 um 15:22 schrieb Ulf Hansson:
> > > + Georg
> > >
> > > On Fri, 25 Oct 2024 at 08:01, Ben Chuang <benchuanggli@gmail.com> wrote:
> > >>
> > >> From: Ben Chuang <ben.chuang@genesyslogic.com.tw>
> > >>
> > >> On sdhci_gl9767_set_clock(), the vendor header space(VHS) is read-only
> > >> after calling gl9767_disable_ssc_pll() and gl9767_set_ssc_pll_205mhz().
> > >> So the low power negotiation mode cannot be enabled again.
> > >> Introduce gl9767_set_low_power_negotiation() function to fix it.
> > >>
> > >> The explanation process is as below.
> > >>
> > >> static void sdhci_gl9767_set_clock()
> > >> {
> > >>         ...
> > >>         gl9767_vhs_write();
> > >>         ...
> > >>         value |= PCIE_GLI_9767_CFG_LOW_PWR_OFF;
> > >>         pci_write_config_dword(pdev, PCIE_GLI_9767_CFG, value); <--- (a)
> > >>
> > >>         gl9767_disable_ssc_pll(); <--- (b)
> > >>         sdhci_writew(host, 0, SDHCI_CLOCK_CONTROL);
> > >>
> > >>         if (clock == 0)
> > >>                 return;  <-- (I)
> > >>
> > >>         ...
> > >>         if (clock == 200000000 && ios->timing == MMC_TIMING_UHS_SDR104) {
> > >>                 ...
> > >>                 gl9767_set_ssc_pll_205mhz(); <--- (c)
> > >>         }
> > >>         ...
> > >>         value &= ~PCIE_GLI_9767_CFG_LOW_PWR_OFF;
> > >>         pci_write_config_dword(pdev, PCIE_GLI_9767_CFG, value); <-- (II)
> > >>         gl9767_vhs_read();
> > >> }
> > >>
> > >> (a) disable low power negotiation mode. When return on (I), the low power
> > >> mode is disabled.  After (b) and (c), VHS is read-only, the low power mode
> > >> cannot be enabled on (II).
> > >>
> > >> Fixes: d2754355512e ("mmc: sdhci-pci-gli: Set SDR104's clock to 205MHz and enable SSC for GL9767")
> > >
> > > Is this the same problem as being reported in
> > > https://lore.kernel.org/all/41c1c88a-b2c9-4c05-863a-467785027f49@tuxedocomputers.com/
> > >
> > > ?
> >
> > Yes, this patch fixes
> > https://bugzilla.kernel.org/show_bug.cgi?id=219284
> >
> > This makes my patch obsolete.
>
> Thanks to both of you for helping out and fixing the problem!
>
> I added Georg's reported/tested-by tag when applying and queued this
> up as a fix with a stable tag.
>
> Kind regards
> Uffe

Hi Georg and Uffe,

The original test reported on bugzilla that only Intel laptops had issues.
AMD laptops without this patch seem to have no impact.

I'm not sure if this patch is related to the originally reported issue.
Thanks for the help.

Best regards,
Ben Chuang