| Message ID | 20241025060017.1663697-1-benchuanggli@gmail.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | [1/2] mmc: sdhci-pci-gli: GL9767: Fix low power mode on the set clock function | expand |
+ 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 >
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
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 > >
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
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
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
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)