* [PATCH 2/2] mmc: sdhci-pci-gli: GL9767: Fix initializing the UHS-II interface during a power-on
@ 2025-09-01 9:42 Ben Chuang
2025-09-01 17:01 ` Adrian Hunter
0 siblings, 1 reply; 6+ messages in thread
From: Ben Chuang @ 2025-09-01 9:42 UTC (permalink / raw)
To: adrian.hunter, ulf.hansson
Cc: victor.shih, ben.chuang, HL.Liu, SeanHY.Chen, benchuanggli,
victorshihgli, linux-mmc, linux-kernel, stable
From: Ben Chuang <ben.chuang@genesyslogic.com.tw>
According to the power structure of IC hardware design for UHS-II
interface, reset control and timing must be added to the initialization
process of powering on the UHS-II interface.
Fixes: 27dd3b82557a ("mmc: sdhci-pci-gli: enable UHS-II mode for GL9767")
Cc: stable@vger.kernel.org # v6.13+
Signed-off-by: Ben Chuang <ben.chuang@genesyslogic.com.tw>
---
drivers/mmc/host/sdhci-pci-gli.c | 71 +++++++++++++++++++++++++++++++-
1 file changed, 70 insertions(+), 1 deletion(-)
diff --git a/drivers/mmc/host/sdhci-pci-gli.c b/drivers/mmc/host/sdhci-pci-gli.c
index 3a1de477e9af..85d0d7e6169c 100644
--- a/drivers/mmc/host/sdhci-pci-gli.c
+++ b/drivers/mmc/host/sdhci-pci-gli.c
@@ -283,6 +283,8 @@
#define PCIE_GLI_9767_UHS2_CTL2_ZC_VALUE 0xb
#define PCIE_GLI_9767_UHS2_CTL2_ZC_CTL BIT(6)
#define PCIE_GLI_9767_UHS2_CTL2_ZC_CTL_VALUE 0x1
+#define PCIE_GLI_9767_UHS2_CTL2_FORCE_PHY_RESETN BIT(13)
+#define PCIE_GLI_9767_UHS2_CTL2_FORCE_RESETN_VALUE BIT(14)
#define GLI_MAX_TUNING_LOOP 40
@@ -1179,6 +1181,69 @@ static void gl9767_set_low_power_negotiation(struct pci_dev *pdev, bool enable)
gl9767_vhs_read(pdev);
}
+static void sdhci_gl9767_uhs2_phy_reset_assert(struct sdhci_host *host)
+{
+ struct sdhci_pci_slot *slot = sdhci_priv(host);
+ struct pci_dev *pdev = slot->chip->pdev;
+ u32 value;
+
+ gl9767_vhs_write(pdev);
+ pci_read_config_dword(pdev, PCIE_GLI_9767_UHS2_CTL2, &value);
+ value |= PCIE_GLI_9767_UHS2_CTL2_FORCE_PHY_RESETN;
+ pci_write_config_dword(pdev, PCIE_GLI_9767_UHS2_CTL2, value);
+ value &= ~PCIE_GLI_9767_UHS2_CTL2_FORCE_RESETN_VALUE;
+ pci_write_config_dword(pdev, PCIE_GLI_9767_UHS2_CTL2, value);
+ gl9767_vhs_read(pdev);
+}
+
+static void sdhci_gl9767_uhs2_phy_reset_deassert(struct sdhci_host *host)
+{
+ struct sdhci_pci_slot *slot = sdhci_priv(host);
+ struct pci_dev *pdev = slot->chip->pdev;
+ u32 value;
+
+ gl9767_vhs_write(pdev);
+ pci_read_config_dword(pdev, PCIE_GLI_9767_UHS2_CTL2, &value);
+ value |= PCIE_GLI_9767_UHS2_CTL2_FORCE_RESETN_VALUE;
+ pci_write_config_dword(pdev, PCIE_GLI_9767_UHS2_CTL2, value);
+ value &= ~PCIE_GLI_9767_UHS2_CTL2_FORCE_PHY_RESETN;
+ pci_write_config_dword(pdev, PCIE_GLI_9767_UHS2_CTL2, value);
+ gl9767_vhs_read(pdev);
+}
+
+static void __gl9767_uhs2_set_power(struct sdhci_host *host, unsigned char mode, unsigned short vdd)
+{
+ u8 pwr = 0;
+
+ if (mode != MMC_POWER_OFF) {
+ pwr = sdhci_get_vdd_value(vdd);
+ if (!pwr)
+ WARN(1, "%s: Invalid vdd %#x\n",
+ mmc_hostname(host->mmc), vdd);
+ pwr |= SDHCI_VDD2_POWER_180;
+ }
+
+ if (host->pwr == pwr)
+ return;
+
+ host->pwr = pwr;
+
+ if (pwr == 0) {
+ sdhci_writeb(host, 0, SDHCI_POWER_CONTROL);
+ } else {
+ sdhci_writeb(host, 0, SDHCI_POWER_CONTROL);
+
+ pwr |= SDHCI_POWER_ON;
+ sdhci_writeb(host, pwr & 0xf, SDHCI_POWER_CONTROL);
+ mdelay(5);
+
+ sdhci_gl9767_uhs2_phy_reset_assert(host);
+ pwr |= SDHCI_VDD2_POWER_ON;
+ sdhci_writeb(host, pwr, SDHCI_POWER_CONTROL);
+ mdelay(5);
+ }
+}
+
static void sdhci_gl9767_set_clock(struct sdhci_host *host, unsigned int clock)
{
struct sdhci_pci_slot *slot = sdhci_priv(host);
@@ -1205,6 +1270,10 @@ static void sdhci_gl9767_set_clock(struct sdhci_host *host, unsigned int clock)
}
sdhci_enable_clk(host, clk);
+
+ if (mmc_card_uhs2(host->mmc))
+ sdhci_gl9767_uhs2_phy_reset_deassert(host);
+
gl9767_set_low_power_negotiation(pdev, true);
}
@@ -1476,7 +1545,7 @@ static void sdhci_gl9767_set_power(struct sdhci_host *host, unsigned char mode,
gl9767_vhs_read(pdev);
sdhci_gli_overcurrent_event_enable(host, false);
- sdhci_uhs2_set_power(host, mode, vdd);
+ __gl9767_uhs2_set_power(host, mode, vdd);
sdhci_gli_overcurrent_event_enable(host, true);
} else {
gl9767_vhs_write(pdev);
--
2.51.0
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH 2/2] mmc: sdhci-pci-gli: GL9767: Fix initializing the UHS-II interface during a power-on
2025-09-01 9:42 [PATCH 2/2] mmc: sdhci-pci-gli: GL9767: Fix initializing the UHS-II interface during a power-on Ben Chuang
@ 2025-09-01 17:01 ` Adrian Hunter
2025-09-02 6:33 ` Ben Chuang
0 siblings, 1 reply; 6+ messages in thread
From: Adrian Hunter @ 2025-09-01 17:01 UTC (permalink / raw)
To: Ben Chuang, ulf.hansson
Cc: victor.shih, ben.chuang, HL.Liu, SeanHY.Chen, victorshihgli,
linux-mmc, linux-kernel, stable
On 01/09/2025 12:42, Ben Chuang wrote:
> From: Ben Chuang <ben.chuang@genesyslogic.com.tw>
>
> According to the power structure of IC hardware design for UHS-II
> interface, reset control and timing must be added to the initialization
> process of powering on the UHS-II interface.
>
> Fixes: 27dd3b82557a ("mmc: sdhci-pci-gli: enable UHS-II mode for GL9767")
> Cc: stable@vger.kernel.org # v6.13+
> Signed-off-by: Ben Chuang <ben.chuang@genesyslogic.com.tw>
> ---
> drivers/mmc/host/sdhci-pci-gli.c | 71 +++++++++++++++++++++++++++++++-
> 1 file changed, 70 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/mmc/host/sdhci-pci-gli.c b/drivers/mmc/host/sdhci-pci-gli.c
> index 3a1de477e9af..85d0d7e6169c 100644
> --- a/drivers/mmc/host/sdhci-pci-gli.c
> +++ b/drivers/mmc/host/sdhci-pci-gli.c
> @@ -283,6 +283,8 @@
> #define PCIE_GLI_9767_UHS2_CTL2_ZC_VALUE 0xb
> #define PCIE_GLI_9767_UHS2_CTL2_ZC_CTL BIT(6)
> #define PCIE_GLI_9767_UHS2_CTL2_ZC_CTL_VALUE 0x1
> +#define PCIE_GLI_9767_UHS2_CTL2_FORCE_PHY_RESETN BIT(13)
> +#define PCIE_GLI_9767_UHS2_CTL2_FORCE_RESETN_VALUE BIT(14)
>
> #define GLI_MAX_TUNING_LOOP 40
>
> @@ -1179,6 +1181,69 @@ static void gl9767_set_low_power_negotiation(struct pci_dev *pdev, bool enable)
> gl9767_vhs_read(pdev);
> }
>
> +static void sdhci_gl9767_uhs2_phy_reset_assert(struct sdhci_host *host)
> +{
> + struct sdhci_pci_slot *slot = sdhci_priv(host);
> + struct pci_dev *pdev = slot->chip->pdev;
> + u32 value;
> +
> + gl9767_vhs_write(pdev);
> + pci_read_config_dword(pdev, PCIE_GLI_9767_UHS2_CTL2, &value);
> + value |= PCIE_GLI_9767_UHS2_CTL2_FORCE_PHY_RESETN;
> + pci_write_config_dword(pdev, PCIE_GLI_9767_UHS2_CTL2, value);
> + value &= ~PCIE_GLI_9767_UHS2_CTL2_FORCE_RESETN_VALUE;
> + pci_write_config_dword(pdev, PCIE_GLI_9767_UHS2_CTL2, value);
> + gl9767_vhs_read(pdev);
> +}
> +
> +static void sdhci_gl9767_uhs2_phy_reset_deassert(struct sdhci_host *host)
> +{
> + struct sdhci_pci_slot *slot = sdhci_priv(host);
> + struct pci_dev *pdev = slot->chip->pdev;
> + u32 value;
> +
> + gl9767_vhs_write(pdev);
> + pci_read_config_dword(pdev, PCIE_GLI_9767_UHS2_CTL2, &value);
> + value |= PCIE_GLI_9767_UHS2_CTL2_FORCE_RESETN_VALUE;
Maybe add a small comment about PCIE_GLI_9767_UHS2_CTL2_FORCE_RESETN_VALUE
and PCIE_GLI_9767_UHS2_CTL2_FORCE_PHY_RESETN being updated separately.
> + pci_write_config_dword(pdev, PCIE_GLI_9767_UHS2_CTL2, value);
> + value &= ~PCIE_GLI_9767_UHS2_CTL2_FORCE_PHY_RESETN;
> + pci_write_config_dword(pdev, PCIE_GLI_9767_UHS2_CTL2, value);
> + gl9767_vhs_read(pdev);
> +}
sdhci_gl9767_uhs2_phy_reset_assert() and sdhci_gl9767_uhs2_phy_reset_deassert()
are fairly similar. Maybe consider:
static void sdhci_gl9767_uhs2_phy_reset(struct sdhci_host *host, bool assert)
{
struct sdhci_pci_slot *slot = sdhci_priv(host);
struct pci_dev *pdev = slot->chip->pdev;
u32 value, set, clr;
if (assert) {
set = PCIE_GLI_9767_UHS2_CTL2_FORCE_PHY_RESETN;
clr = PCIE_GLI_9767_UHS2_CTL2_FORCE_RESETN_VALUE;
} else {
set = PCIE_GLI_9767_UHS2_CTL2_FORCE_RESETN_VALUE;
clr = PCIE_GLI_9767_UHS2_CTL2_FORCE_PHY_RESETN;
}
gl9767_vhs_write(pdev);
pci_read_config_dword(pdev, PCIE_GLI_9767_UHS2_CTL2, &value);
value |= set;
pci_write_config_dword(pdev, PCIE_GLI_9767_UHS2_CTL2, value);
value &= ~clr;
pci_write_config_dword(pdev, PCIE_GLI_9767_UHS2_CTL2, value);
gl9767_vhs_read(pdev);
}
> +
> +static void __gl9767_uhs2_set_power(struct sdhci_host *host, unsigned char mode, unsigned short vdd)
> +{
> + u8 pwr = 0;
> +
> + if (mode != MMC_POWER_OFF) {
> + pwr = sdhci_get_vdd_value(vdd);
> + if (!pwr)
> + WARN(1, "%s: Invalid vdd %#x\n",
> + mmc_hostname(host->mmc), vdd);
> + pwr |= SDHCI_VDD2_POWER_180;
> + }
> +
> + if (host->pwr == pwr)
> + return;
> +
> + host->pwr = pwr;
> +
> + if (pwr == 0) {
> + sdhci_writeb(host, 0, SDHCI_POWER_CONTROL);
> + } else {
> + sdhci_writeb(host, 0, SDHCI_POWER_CONTROL);
> +
> + pwr |= SDHCI_POWER_ON;
> + sdhci_writeb(host, pwr & 0xf, SDHCI_POWER_CONTROL);
> + mdelay(5);
Can be mmc_delay(5)
> +
> + sdhci_gl9767_uhs2_phy_reset_assert(host);
> + pwr |= SDHCI_VDD2_POWER_ON;
> + sdhci_writeb(host, pwr, SDHCI_POWER_CONTROL);
> + mdelay(5);
Can be mmc_delay(5)
> + }
> +}
> +
> static void sdhci_gl9767_set_clock(struct sdhci_host *host, unsigned int clock)
> {
> struct sdhci_pci_slot *slot = sdhci_priv(host);
> @@ -1205,6 +1270,10 @@ static void sdhci_gl9767_set_clock(struct sdhci_host *host, unsigned int clock)
> }
>
> sdhci_enable_clk(host, clk);
> +
> + if (mmc_card_uhs2(host->mmc))
> + sdhci_gl9767_uhs2_phy_reset_deassert(host);
> +
> gl9767_set_low_power_negotiation(pdev, true);
> }
>
> @@ -1476,7 +1545,7 @@ static void sdhci_gl9767_set_power(struct sdhci_host *host, unsigned char mode,
> gl9767_vhs_read(pdev);
>
> sdhci_gli_overcurrent_event_enable(host, false);
> - sdhci_uhs2_set_power(host, mode, vdd);
> + __gl9767_uhs2_set_power(host, mode, vdd);
> sdhci_gli_overcurrent_event_enable(host, true);
> } else {
> gl9767_vhs_write(pdev);
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 2/2] mmc: sdhci-pci-gli: GL9767: Fix initializing the UHS-II interface during a power-on
2025-09-01 17:01 ` Adrian Hunter
@ 2025-09-02 6:33 ` Ben Chuang
2025-09-02 7:12 ` Ben Chuang
0 siblings, 1 reply; 6+ messages in thread
From: Ben Chuang @ 2025-09-02 6:33 UTC (permalink / raw)
To: Adrian Hunter
Cc: ulf.hansson, victor.shih, ben.chuang, HL.Liu, SeanHY.Chen,
linux-mmc, linux-kernel, stable
On Tue, Sep 2, 2025 at 1:02 AM Adrian Hunter <adrian.hunter@intel.com> wrote:
>
> On 01/09/2025 12:42, Ben Chuang wrote:
> > From: Ben Chuang <ben.chuang@genesyslogic.com.tw>
> >
> > According to the power structure of IC hardware design for UHS-II
> > interface, reset control and timing must be added to the initialization
> > process of powering on the UHS-II interface.
> >
> > Fixes: 27dd3b82557a ("mmc: sdhci-pci-gli: enable UHS-II mode for GL9767")
> > Cc: stable@vger.kernel.org # v6.13+
> > Signed-off-by: Ben Chuang <ben.chuang@genesyslogic.com.tw>
> > ---
> > drivers/mmc/host/sdhci-pci-gli.c | 71 +++++++++++++++++++++++++++++++-
> > 1 file changed, 70 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/mmc/host/sdhci-pci-gli.c b/drivers/mmc/host/sdhci-pci-gli.c
> > index 3a1de477e9af..85d0d7e6169c 100644
> > --- a/drivers/mmc/host/sdhci-pci-gli.c
> > +++ b/drivers/mmc/host/sdhci-pci-gli.c
> > @@ -283,6 +283,8 @@
> > #define PCIE_GLI_9767_UHS2_CTL2_ZC_VALUE 0xb
> > #define PCIE_GLI_9767_UHS2_CTL2_ZC_CTL BIT(6)
> > #define PCIE_GLI_9767_UHS2_CTL2_ZC_CTL_VALUE 0x1
> > +#define PCIE_GLI_9767_UHS2_CTL2_FORCE_PHY_RESETN BIT(13)
> > +#define PCIE_GLI_9767_UHS2_CTL2_FORCE_RESETN_VALUE BIT(14)
> >
> > #define GLI_MAX_TUNING_LOOP 40
> >
> > @@ -1179,6 +1181,69 @@ static void gl9767_set_low_power_negotiation(struct pci_dev *pdev, bool enable)
> > gl9767_vhs_read(pdev);
> > }
> >
> > +static void sdhci_gl9767_uhs2_phy_reset_assert(struct sdhci_host *host)
> > +{
> > + struct sdhci_pci_slot *slot = sdhci_priv(host);
> > + struct pci_dev *pdev = slot->chip->pdev;
> > + u32 value;
> > +
> > + gl9767_vhs_write(pdev);
> > + pci_read_config_dword(pdev, PCIE_GLI_9767_UHS2_CTL2, &value);
> > + value |= PCIE_GLI_9767_UHS2_CTL2_FORCE_PHY_RESETN;
> > + pci_write_config_dword(pdev, PCIE_GLI_9767_UHS2_CTL2, value);
> > + value &= ~PCIE_GLI_9767_UHS2_CTL2_FORCE_RESETN_VALUE;
> > + pci_write_config_dword(pdev, PCIE_GLI_9767_UHS2_CTL2, value);
> > + gl9767_vhs_read(pdev);
> > +}
> > +
> > +static void sdhci_gl9767_uhs2_phy_reset_deassert(struct sdhci_host *host)
> > +{
> > + struct sdhci_pci_slot *slot = sdhci_priv(host);
> > + struct pci_dev *pdev = slot->chip->pdev;
> > + u32 value;
> > +
> > + gl9767_vhs_write(pdev);
> > + pci_read_config_dword(pdev, PCIE_GLI_9767_UHS2_CTL2, &value);
> > + value |= PCIE_GLI_9767_UHS2_CTL2_FORCE_RESETN_VALUE;
>
> Maybe add a small comment about PCIE_GLI_9767_UHS2_CTL2_FORCE_RESETN_VALUE
> and PCIE_GLI_9767_UHS2_CTL2_FORCE_PHY_RESETN being updated separately.
>
> > + pci_write_config_dword(pdev, PCIE_GLI_9767_UHS2_CTL2, value);
> > + value &= ~PCIE_GLI_9767_UHS2_CTL2_FORCE_PHY_RESETN;
> > + pci_write_config_dword(pdev, PCIE_GLI_9767_UHS2_CTL2, value);
> > + gl9767_vhs_read(pdev);
> > +}
>
> sdhci_gl9767_uhs2_phy_reset_assert() and sdhci_gl9767_uhs2_phy_reset_deassert()
> are fairly similar. Maybe consider:
>
> static void sdhci_gl9767_uhs2_phy_reset(struct sdhci_host *host, bool assert)
> {
> struct sdhci_pci_slot *slot = sdhci_priv(host);
> struct pci_dev *pdev = slot->chip->pdev;
> u32 value, set, clr;
>
> if (assert) {
> set = PCIE_GLI_9767_UHS2_CTL2_FORCE_PHY_RESETN;
> clr = PCIE_GLI_9767_UHS2_CTL2_FORCE_RESETN_VALUE;
> } else {
> set = PCIE_GLI_9767_UHS2_CTL2_FORCE_RESETN_VALUE;
> clr = PCIE_GLI_9767_UHS2_CTL2_FORCE_PHY_RESETN;
> }
>
> gl9767_vhs_write(pdev);
> pci_read_config_dword(pdev, PCIE_GLI_9767_UHS2_CTL2, &value);
> value |= set;
> pci_write_config_dword(pdev, PCIE_GLI_9767_UHS2_CTL2, value);
> value &= ~clr;
> pci_write_config_dword(pdev, PCIE_GLI_9767_UHS2_CTL2, value);
> gl9767_vhs_read(pdev);
> }
>
OK, I will update it. Thank you.
>
> > +
> > +static void __gl9767_uhs2_set_power(struct sdhci_host *host, unsigned char mode, unsigned short vdd)
> > +{
> > + u8 pwr = 0;
> > +
> > + if (mode != MMC_POWER_OFF) {
> > + pwr = sdhci_get_vdd_value(vdd);
> > + if (!pwr)
> > + WARN(1, "%s: Invalid vdd %#x\n",
> > + mmc_hostname(host->mmc), vdd);
> > + pwr |= SDHCI_VDD2_POWER_180;
> > + }
> > +
> > + if (host->pwr == pwr)
> > + return;
> > +
> > + host->pwr = pwr;
> > +
> > + if (pwr == 0) {
> > + sdhci_writeb(host, 0, SDHCI_POWER_CONTROL);
> > + } else {
> > + sdhci_writeb(host, 0, SDHCI_POWER_CONTROL);
> > +
> > + pwr |= SDHCI_POWER_ON;
> > + sdhci_writeb(host, pwr & 0xf, SDHCI_POWER_CONTROL);
> > + mdelay(5);
>
> Can be mmc_delay(5)
>
> > +
> > + sdhci_gl9767_uhs2_phy_reset_assert(host);
> > + pwr |= SDHCI_VDD2_POWER_ON;
> > + sdhci_writeb(host, pwr, SDHCI_POWER_CONTROL);
> > + mdelay(5);
>
> Can be mmc_delay(5)
I may not modify it now.
mmc_delay() is in "drivers/mmc/core/core.h".
If sdhci-pci-gli.c only includes "../core/core.h", the compiler will
report some errors.
<snippet>
IIn file included from host/sdhci-pci-gli.c:17:
host/../core/core.h:131:52: warning: ‘struct mmc_ctx’ declared inside
parameter list will not be visible outside of this definition or
declaration
131 | int __mmc_claim_host(struct mmc_host *host, struct mmc_ctx *ctx,
| ^~~~~~~
host/../core/core.h:134:49: warning: ‘struct mmc_ctx’ declared inside
parameter list will not be visible outside of this definition or
declaration
134 | void mmc_get_card(struct mmc_card *card, struct mmc_ctx *ctx);
| ^~~~~~~
host/../core/core.h:135:49: warning: ‘struct mmc_ctx’ declared inside
parameter list will not be visible outside of this definition or
declaration
135 | void mmc_put_card(struct mmc_card *card, struct mmc_ctx *ctx);
| ^~~~~~~
host/../core/core.h: In function ‘mmc_pre_req’:
host/../core/core.h:165:17: error: invalid use of undefined type
‘struct mmc_host’
165 | if (host->ops->pre_req)
| ^~
>
> > + }
> > +}
> > +
> > static void sdhci_gl9767_set_clock(struct sdhci_host *host, unsigned int clock)
> > {
> > struct sdhci_pci_slot *slot = sdhci_priv(host);
> > @@ -1205,6 +1270,10 @@ static void sdhci_gl9767_set_clock(struct sdhci_host *host, unsigned int clock)
> > }
> >
> > sdhci_enable_clk(host, clk);
> > +
> > + if (mmc_card_uhs2(host->mmc))
> > + sdhci_gl9767_uhs2_phy_reset_deassert(host);
> > +
> > gl9767_set_low_power_negotiation(pdev, true);
> > }
> >
> > @@ -1476,7 +1545,7 @@ static void sdhci_gl9767_set_power(struct sdhci_host *host, unsigned char mode,
> > gl9767_vhs_read(pdev);
> >
> > sdhci_gli_overcurrent_event_enable(host, false);
> > - sdhci_uhs2_set_power(host, mode, vdd);
> > + __gl9767_uhs2_set_power(host, mode, vdd);
> > sdhci_gli_overcurrent_event_enable(host, true);
> > } else {
> > gl9767_vhs_write(pdev);
>
Best regards,
Ben Chuang
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 2/2] mmc: sdhci-pci-gli: GL9767: Fix initializing the UHS-II interface during a power-on
2025-09-02 6:33 ` Ben Chuang
@ 2025-09-02 7:12 ` Ben Chuang
2025-09-02 7:47 ` Adrian Hunter
0 siblings, 1 reply; 6+ messages in thread
From: Ben Chuang @ 2025-09-02 7:12 UTC (permalink / raw)
To: Adrian Hunter
Cc: ulf.hansson, victor.shih, ben.chuang, HL.Liu, SeanHY.Chen,
linux-mmc, linux-kernel, stable
On Tue, Sep 2, 2025 at 2:33 PM Ben Chuang <benchuanggli@gmail.com> wrote:
>
> On Tue, Sep 2, 2025 at 1:02 AM Adrian Hunter <adrian.hunter@intel.com> wrote:
> >
> > On 01/09/2025 12:42, Ben Chuang wrote:
> > > From: Ben Chuang <ben.chuang@genesyslogic.com.tw>
> > >
> > > According to the power structure of IC hardware design for UHS-II
> > > interface, reset control and timing must be added to the initialization
> > > process of powering on the UHS-II interface.
> > >
> > > Fixes: 27dd3b82557a ("mmc: sdhci-pci-gli: enable UHS-II mode for GL9767")
> > > Cc: stable@vger.kernel.org # v6.13+
> > > Signed-off-by: Ben Chuang <ben.chuang@genesyslogic.com.tw>
> > > ---
> > > drivers/mmc/host/sdhci-pci-gli.c | 71 +++++++++++++++++++++++++++++++-
> > > 1 file changed, 70 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/drivers/mmc/host/sdhci-pci-gli.c b/drivers/mmc/host/sdhci-pci-gli.c
> > > index 3a1de477e9af..85d0d7e6169c 100644
> > > --- a/drivers/mmc/host/sdhci-pci-gli.c
> > > +++ b/drivers/mmc/host/sdhci-pci-gli.c
> > > @@ -283,6 +283,8 @@
> > > #define PCIE_GLI_9767_UHS2_CTL2_ZC_VALUE 0xb
> > > #define PCIE_GLI_9767_UHS2_CTL2_ZC_CTL BIT(6)
> > > #define PCIE_GLI_9767_UHS2_CTL2_ZC_CTL_VALUE 0x1
> > > +#define PCIE_GLI_9767_UHS2_CTL2_FORCE_PHY_RESETN BIT(13)
> > > +#define PCIE_GLI_9767_UHS2_CTL2_FORCE_RESETN_VALUE BIT(14)
> > >
> > > #define GLI_MAX_TUNING_LOOP 40
> > >
> > > @@ -1179,6 +1181,69 @@ static void gl9767_set_low_power_negotiation(struct pci_dev *pdev, bool enable)
> > > gl9767_vhs_read(pdev);
> > > }
> > >
> > > +static void sdhci_gl9767_uhs2_phy_reset_assert(struct sdhci_host *host)
> > > +{
> > > + struct sdhci_pci_slot *slot = sdhci_priv(host);
> > > + struct pci_dev *pdev = slot->chip->pdev;
> > > + u32 value;
> > > +
> > > + gl9767_vhs_write(pdev);
> > > + pci_read_config_dword(pdev, PCIE_GLI_9767_UHS2_CTL2, &value);
> > > + value |= PCIE_GLI_9767_UHS2_CTL2_FORCE_PHY_RESETN;
> > > + pci_write_config_dword(pdev, PCIE_GLI_9767_UHS2_CTL2, value);
> > > + value &= ~PCIE_GLI_9767_UHS2_CTL2_FORCE_RESETN_VALUE;
> > > + pci_write_config_dword(pdev, PCIE_GLI_9767_UHS2_CTL2, value);
> > > + gl9767_vhs_read(pdev);
> > > +}
> > > +
> > > +static void sdhci_gl9767_uhs2_phy_reset_deassert(struct sdhci_host *host)
> > > +{
> > > + struct sdhci_pci_slot *slot = sdhci_priv(host);
> > > + struct pci_dev *pdev = slot->chip->pdev;
> > > + u32 value;
> > > +
> > > + gl9767_vhs_write(pdev);
> > > + pci_read_config_dword(pdev, PCIE_GLI_9767_UHS2_CTL2, &value);
> > > + value |= PCIE_GLI_9767_UHS2_CTL2_FORCE_RESETN_VALUE;
> >
> > Maybe add a small comment about PCIE_GLI_9767_UHS2_CTL2_FORCE_RESETN_VALUE
> > and PCIE_GLI_9767_UHS2_CTL2_FORCE_PHY_RESETN being updated separately.
> >
> > > + pci_write_config_dword(pdev, PCIE_GLI_9767_UHS2_CTL2, value);
> > > + value &= ~PCIE_GLI_9767_UHS2_CTL2_FORCE_PHY_RESETN;
> > > + pci_write_config_dword(pdev, PCIE_GLI_9767_UHS2_CTL2, value);
> > > + gl9767_vhs_read(pdev);
> > > +}
> >
> > sdhci_gl9767_uhs2_phy_reset_assert() and sdhci_gl9767_uhs2_phy_reset_deassert()
> > are fairly similar. Maybe consider:
> >
> > static void sdhci_gl9767_uhs2_phy_reset(struct sdhci_host *host, bool assert)
> > {
> > struct sdhci_pci_slot *slot = sdhci_priv(host);
> > struct pci_dev *pdev = slot->chip->pdev;
> > u32 value, set, clr;
> >
> > if (assert) {
> > set = PCIE_GLI_9767_UHS2_CTL2_FORCE_PHY_RESETN;
> > clr = PCIE_GLI_9767_UHS2_CTL2_FORCE_RESETN_VALUE;
> > } else {
> > set = PCIE_GLI_9767_UHS2_CTL2_FORCE_RESETN_VALUE;
> > clr = PCIE_GLI_9767_UHS2_CTL2_FORCE_PHY_RESETN;
> > }
> >
> > gl9767_vhs_write(pdev);
> > pci_read_config_dword(pdev, PCIE_GLI_9767_UHS2_CTL2, &value);
> > value |= set;
> > pci_write_config_dword(pdev, PCIE_GLI_9767_UHS2_CTL2, value);
> > value &= ~clr;
> > pci_write_config_dword(pdev, PCIE_GLI_9767_UHS2_CTL2, value);
> > gl9767_vhs_read(pdev);
> > }
> >
>
> OK, I will update it. Thank you.
>
> >
> > > +
> > > +static void __gl9767_uhs2_set_power(struct sdhci_host *host, unsigned char mode, unsigned short vdd)
> > > +{
> > > + u8 pwr = 0;
> > > +
> > > + if (mode != MMC_POWER_OFF) {
> > > + pwr = sdhci_get_vdd_value(vdd);
> > > + if (!pwr)
> > > + WARN(1, "%s: Invalid vdd %#x\n",
> > > + mmc_hostname(host->mmc), vdd);
> > > + pwr |= SDHCI_VDD2_POWER_180;
> > > + }
> > > +
> > > + if (host->pwr == pwr)
> > > + return;
> > > +
> > > + host->pwr = pwr;
> > > +
> > > + if (pwr == 0) {
> > > + sdhci_writeb(host, 0, SDHCI_POWER_CONTROL);
> > > + } else {
> > > + sdhci_writeb(host, 0, SDHCI_POWER_CONTROL);
> > > +
> > > + pwr |= SDHCI_POWER_ON;
> > > + sdhci_writeb(host, pwr & 0xf, SDHCI_POWER_CONTROL);
> > > + mdelay(5);
> >
> > Can be mmc_delay(5)
> >
> > > +
> > > + sdhci_gl9767_uhs2_phy_reset_assert(host);
> > > + pwr |= SDHCI_VDD2_POWER_ON;
> > > + sdhci_writeb(host, pwr, SDHCI_POWER_CONTROL);
> > > + mdelay(5);
> >
> > Can be mmc_delay(5)
>
> I may not modify it now.
> mmc_delay() is in "drivers/mmc/core/core.h".
> If sdhci-pci-gli.c only includes "../core/core.h", the compiler will
> report some errors.
Ah, just add another headersand it will build.
--- a/drivers/mmc/host/sdhci-pci-gli.c
+++ b/drivers/mmc/host/sdhci-pci-gli.c
@@ -14,6 +14,8 @@
#include <linux/delay.h>
#include <linux/of.h>
#include <linux/iopoll.h>
+#include <linux/mmc/host.h>
+#include "../core/core.h"
#include "sdhci.h"
#include "sdhci-cqhci.h"
#include "sdhci-pci.h"
@@ -968,10 +970,10 @@ static void gl9755_set_power(struct sdhci_host
*host, unsigned char mode,
sdhci_writeb(host, pwr & 0xf, SDHCI_POWER_CONTROL);
/* wait stable */
- mdelay(5);
+ mmc_delay(5);
sdhci_writeb(host, pwr, SDHCI_POWER_CONTROL);
/* wait stable */
- mdelay(5);
+ mmc_delay(5);
sdhci_gli_overcurrent_event_enable(host, true);
}
>
> <snippet>
> IIn file included from host/sdhci-pci-gli.c:17:
> host/../core/core.h:131:52: warning: ‘struct mmc_ctx’ declared inside
> parameter list will not be visible outside of this definition or
> declaration
> 131 | int __mmc_claim_host(struct mmc_host *host, struct mmc_ctx *ctx,
> | ^~~~~~~
> host/../core/core.h:134:49: warning: ‘struct mmc_ctx’ declared inside
> parameter list will not be visible outside of this definition or
> declaration
> 134 | void mmc_get_card(struct mmc_card *card, struct mmc_ctx *ctx);
> | ^~~~~~~
> host/../core/core.h:135:49: warning: ‘struct mmc_ctx’ declared inside
> parameter list will not be visible outside of this definition or
> declaration
> 135 | void mmc_put_card(struct mmc_card *card, struct mmc_ctx *ctx);
> | ^~~~~~~
> host/../core/core.h: In function ‘mmc_pre_req’:
> host/../core/core.h:165:17: error: invalid use of undefined type
> ‘struct mmc_host’
> 165 | if (host->ops->pre_req)
> | ^~
>
> >
> > > + }
> > > +}
> > > +
> > > static void sdhci_gl9767_set_clock(struct sdhci_host *host, unsigned int clock)
> > > {
> > > struct sdhci_pci_slot *slot = sdhci_priv(host);
> > > @@ -1205,6 +1270,10 @@ static void sdhci_gl9767_set_clock(struct sdhci_host *host, unsigned int clock)
> > > }
> > >
> > > sdhci_enable_clk(host, clk);
> > > +
> > > + if (mmc_card_uhs2(host->mmc))
> > > + sdhci_gl9767_uhs2_phy_reset_deassert(host);
> > > +
> > > gl9767_set_low_power_negotiation(pdev, true);
> > > }
> > >
> > > @@ -1476,7 +1545,7 @@ static void sdhci_gl9767_set_power(struct sdhci_host *host, unsigned char mode,
> > > gl9767_vhs_read(pdev);
> > >
> > > sdhci_gli_overcurrent_event_enable(host, false);
> > > - sdhci_uhs2_set_power(host, mode, vdd);
> > > + __gl9767_uhs2_set_power(host, mode, vdd);
> > > sdhci_gli_overcurrent_event_enable(host, true);
> > > } else {
> > > gl9767_vhs_write(pdev);
> >
>
> Best regards,
> Ben Chuang
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 2/2] mmc: sdhci-pci-gli: GL9767: Fix initializing the UHS-II interface during a power-on
2025-09-02 7:12 ` Ben Chuang
@ 2025-09-02 7:47 ` Adrian Hunter
2025-09-02 8:54 ` Ben Chuang
0 siblings, 1 reply; 6+ messages in thread
From: Adrian Hunter @ 2025-09-02 7:47 UTC (permalink / raw)
To: Ben Chuang
Cc: ulf.hansson, victor.shih, ben.chuang, HL.Liu, SeanHY.Chen,
linux-mmc, linux-kernel, stable
On 02/09/2025 10:12, Ben Chuang wrote:
> On Tue, Sep 2, 2025 at 2:33 PM Ben Chuang <benchuanggli@gmail.com> wrote:
>>
>> On Tue, Sep 2, 2025 at 1:02 AM Adrian Hunter <adrian.hunter@intel.com> wrote:
>>>
>>> On 01/09/2025 12:42, Ben Chuang wrote:
>>>> From: Ben Chuang <ben.chuang@genesyslogic.com.tw>
>>>>
>>>> According to the power structure of IC hardware design for UHS-II
>>>> interface, reset control and timing must be added to the initialization
>>>> process of powering on the UHS-II interface.
>>>>
>>>> Fixes: 27dd3b82557a ("mmc: sdhci-pci-gli: enable UHS-II mode for GL9767")
>>>> Cc: stable@vger.kernel.org # v6.13+
>>>> Signed-off-by: Ben Chuang <ben.chuang@genesyslogic.com.tw>
>>>> ---
>>>> drivers/mmc/host/sdhci-pci-gli.c | 71 +++++++++++++++++++++++++++++++-
>>>> 1 file changed, 70 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/drivers/mmc/host/sdhci-pci-gli.c b/drivers/mmc/host/sdhci-pci-gli.c
>>>> index 3a1de477e9af..85d0d7e6169c 100644
>>>> --- a/drivers/mmc/host/sdhci-pci-gli.c
>>>> +++ b/drivers/mmc/host/sdhci-pci-gli.c
>>>> @@ -283,6 +283,8 @@
>>>> #define PCIE_GLI_9767_UHS2_CTL2_ZC_VALUE 0xb
>>>> #define PCIE_GLI_9767_UHS2_CTL2_ZC_CTL BIT(6)
>>>> #define PCIE_GLI_9767_UHS2_CTL2_ZC_CTL_VALUE 0x1
>>>> +#define PCIE_GLI_9767_UHS2_CTL2_FORCE_PHY_RESETN BIT(13)
>>>> +#define PCIE_GLI_9767_UHS2_CTL2_FORCE_RESETN_VALUE BIT(14)
>>>>
>>>> #define GLI_MAX_TUNING_LOOP 40
>>>>
>>>> @@ -1179,6 +1181,69 @@ static void gl9767_set_low_power_negotiation(struct pci_dev *pdev, bool enable)
>>>> gl9767_vhs_read(pdev);
>>>> }
>>>>
>>>> +static void sdhci_gl9767_uhs2_phy_reset_assert(struct sdhci_host *host)
>>>> +{
>>>> + struct sdhci_pci_slot *slot = sdhci_priv(host);
>>>> + struct pci_dev *pdev = slot->chip->pdev;
>>>> + u32 value;
>>>> +
>>>> + gl9767_vhs_write(pdev);
>>>> + pci_read_config_dword(pdev, PCIE_GLI_9767_UHS2_CTL2, &value);
>>>> + value |= PCIE_GLI_9767_UHS2_CTL2_FORCE_PHY_RESETN;
>>>> + pci_write_config_dword(pdev, PCIE_GLI_9767_UHS2_CTL2, value);
>>>> + value &= ~PCIE_GLI_9767_UHS2_CTL2_FORCE_RESETN_VALUE;
>>>> + pci_write_config_dword(pdev, PCIE_GLI_9767_UHS2_CTL2, value);
>>>> + gl9767_vhs_read(pdev);
>>>> +}
>>>> +
>>>> +static void sdhci_gl9767_uhs2_phy_reset_deassert(struct sdhci_host *host)
>>>> +{
>>>> + struct sdhci_pci_slot *slot = sdhci_priv(host);
>>>> + struct pci_dev *pdev = slot->chip->pdev;
>>>> + u32 value;
>>>> +
>>>> + gl9767_vhs_write(pdev);
>>>> + pci_read_config_dword(pdev, PCIE_GLI_9767_UHS2_CTL2, &value);
>>>> + value |= PCIE_GLI_9767_UHS2_CTL2_FORCE_RESETN_VALUE;
>>>
>>> Maybe add a small comment about PCIE_GLI_9767_UHS2_CTL2_FORCE_RESETN_VALUE
>>> and PCIE_GLI_9767_UHS2_CTL2_FORCE_PHY_RESETN being updated separately.
>>>
>>>> + pci_write_config_dword(pdev, PCIE_GLI_9767_UHS2_CTL2, value);
>>>> + value &= ~PCIE_GLI_9767_UHS2_CTL2_FORCE_PHY_RESETN;
>>>> + pci_write_config_dword(pdev, PCIE_GLI_9767_UHS2_CTL2, value);
>>>> + gl9767_vhs_read(pdev);
>>>> +}
>>>
>>> sdhci_gl9767_uhs2_phy_reset_assert() and sdhci_gl9767_uhs2_phy_reset_deassert()
>>> are fairly similar. Maybe consider:
>>>
>>> static void sdhci_gl9767_uhs2_phy_reset(struct sdhci_host *host, bool assert)
>>> {
>>> struct sdhci_pci_slot *slot = sdhci_priv(host);
>>> struct pci_dev *pdev = slot->chip->pdev;
>>> u32 value, set, clr;
>>>
>>> if (assert) {
>>> set = PCIE_GLI_9767_UHS2_CTL2_FORCE_PHY_RESETN;
>>> clr = PCIE_GLI_9767_UHS2_CTL2_FORCE_RESETN_VALUE;
>>> } else {
>>> set = PCIE_GLI_9767_UHS2_CTL2_FORCE_RESETN_VALUE;
>>> clr = PCIE_GLI_9767_UHS2_CTL2_FORCE_PHY_RESETN;
>>> }
>>>
>>> gl9767_vhs_write(pdev);
>>> pci_read_config_dword(pdev, PCIE_GLI_9767_UHS2_CTL2, &value);
>>> value |= set;
>>> pci_write_config_dword(pdev, PCIE_GLI_9767_UHS2_CTL2, value);
>>> value &= ~clr;
>>> pci_write_config_dword(pdev, PCIE_GLI_9767_UHS2_CTL2, value);
>>> gl9767_vhs_read(pdev);
>>> }
>>>
>>
>> OK, I will update it. Thank you.
>>
>>>
>>>> +
>>>> +static void __gl9767_uhs2_set_power(struct sdhci_host *host, unsigned char mode, unsigned short vdd)
>>>> +{
>>>> + u8 pwr = 0;
>>>> +
>>>> + if (mode != MMC_POWER_OFF) {
>>>> + pwr = sdhci_get_vdd_value(vdd);
>>>> + if (!pwr)
>>>> + WARN(1, "%s: Invalid vdd %#x\n",
>>>> + mmc_hostname(host->mmc), vdd);
>>>> + pwr |= SDHCI_VDD2_POWER_180;
>>>> + }
>>>> +
>>>> + if (host->pwr == pwr)
>>>> + return;
>>>> +
>>>> + host->pwr = pwr;
>>>> +
>>>> + if (pwr == 0) {
>>>> + sdhci_writeb(host, 0, SDHCI_POWER_CONTROL);
>>>> + } else {
>>>> + sdhci_writeb(host, 0, SDHCI_POWER_CONTROL);
>>>> +
>>>> + pwr |= SDHCI_POWER_ON;
>>>> + sdhci_writeb(host, pwr & 0xf, SDHCI_POWER_CONTROL);
>>>> + mdelay(5);
>>>
>>> Can be mmc_delay(5)
>>>
>>>> +
>>>> + sdhci_gl9767_uhs2_phy_reset_assert(host);
>>>> + pwr |= SDHCI_VDD2_POWER_ON;
>>>> + sdhci_writeb(host, pwr, SDHCI_POWER_CONTROL);
>>>> + mdelay(5);
>>>
>>> Can be mmc_delay(5)
>>
>> I may not modify it now.
>> mmc_delay() is in "drivers/mmc/core/core.h".
>> If sdhci-pci-gli.c only includes "../core/core.h", the compiler will
>> report some errors.
>
> Ah, just add another headersand it will build.
>
> --- a/drivers/mmc/host/sdhci-pci-gli.c
> +++ b/drivers/mmc/host/sdhci-pci-gli.c
> @@ -14,6 +14,8 @@
> #include <linux/delay.h>
> #include <linux/of.h>
> #include <linux/iopoll.h>
> +#include <linux/mmc/host.h>
> +#include "../core/core.h"
> #include "sdhci.h"
> #include "sdhci-cqhci.h"
> #include "sdhci-pci.h"
> @@ -968,10 +970,10 @@ static void gl9755_set_power(struct sdhci_host
> *host, unsigned char mode,
>
> sdhci_writeb(host, pwr & 0xf, SDHCI_POWER_CONTROL);
> /* wait stable */
> - mdelay(5);
> + mmc_delay(5);
> sdhci_writeb(host, pwr, SDHCI_POWER_CONTROL);
> /* wait stable */
> - mdelay(5);
> + mmc_delay(5);
> sdhci_gli_overcurrent_event_enable(host, true);
> }
It seems mmc_delay() is for core mmc code only, not host drivers.
But the issue with mdelay is that it does not sleep. Other options
are msleep() or usleep_range().
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 2/2] mmc: sdhci-pci-gli: GL9767: Fix initializing the UHS-II interface during a power-on
2025-09-02 7:47 ` Adrian Hunter
@ 2025-09-02 8:54 ` Ben Chuang
0 siblings, 0 replies; 6+ messages in thread
From: Ben Chuang @ 2025-09-02 8:54 UTC (permalink / raw)
To: Adrian Hunter
Cc: ulf.hansson, victor.shih, ben.chuang, HL.Liu, SeanHY.Chen,
linux-mmc, linux-kernel, stable
On Tue, Sep 2, 2025 at 3:47 PM Adrian Hunter <adrian.hunter@intel.com> wrote:
>
> On 02/09/2025 10:12, Ben Chuang wrote:
> > On Tue, Sep 2, 2025 at 2:33 PM Ben Chuang <benchuanggli@gmail.com> wrote:
> >>
> >> On Tue, Sep 2, 2025 at 1:02 AM Adrian Hunter <adrian.hunter@intel.com> wrote:
> >>>
> >>> On 01/09/2025 12:42, Ben Chuang wrote:
> >>>> From: Ben Chuang <ben.chuang@genesyslogic.com.tw>
> >>>>
> >>>> According to the power structure of IC hardware design for UHS-II
> >>>> interface, reset control and timing must be added to the initialization
> >>>> process of powering on the UHS-II interface.
> >>>>
> >>>> Fixes: 27dd3b82557a ("mmc: sdhci-pci-gli: enable UHS-II mode for GL9767")
> >>>> Cc: stable@vger.kernel.org # v6.13+
> >>>> Signed-off-by: Ben Chuang <ben.chuang@genesyslogic.com.tw>
> >>>> ---
> >>>> drivers/mmc/host/sdhci-pci-gli.c | 71 +++++++++++++++++++++++++++++++-
> >>>> 1 file changed, 70 insertions(+), 1 deletion(-)
> >>>>
> >>>> diff --git a/drivers/mmc/host/sdhci-pci-gli.c b/drivers/mmc/host/sdhci-pci-gli.c
> >>>> index 3a1de477e9af..85d0d7e6169c 100644
> >>>> --- a/drivers/mmc/host/sdhci-pci-gli.c
> >>>> +++ b/drivers/mmc/host/sdhci-pci-gli.c
> >>>> @@ -283,6 +283,8 @@
> >>>> #define PCIE_GLI_9767_UHS2_CTL2_ZC_VALUE 0xb
> >>>> #define PCIE_GLI_9767_UHS2_CTL2_ZC_CTL BIT(6)
> >>>> #define PCIE_GLI_9767_UHS2_CTL2_ZC_CTL_VALUE 0x1
> >>>> +#define PCIE_GLI_9767_UHS2_CTL2_FORCE_PHY_RESETN BIT(13)
> >>>> +#define PCIE_GLI_9767_UHS2_CTL2_FORCE_RESETN_VALUE BIT(14)
> >>>>
> >>>> #define GLI_MAX_TUNING_LOOP 40
> >>>>
> >>>> @@ -1179,6 +1181,69 @@ static void gl9767_set_low_power_negotiation(struct pci_dev *pdev, bool enable)
> >>>> gl9767_vhs_read(pdev);
> >>>> }
> >>>>
> >>>> +static void sdhci_gl9767_uhs2_phy_reset_assert(struct sdhci_host *host)
> >>>> +{
> >>>> + struct sdhci_pci_slot *slot = sdhci_priv(host);
> >>>> + struct pci_dev *pdev = slot->chip->pdev;
> >>>> + u32 value;
> >>>> +
> >>>> + gl9767_vhs_write(pdev);
> >>>> + pci_read_config_dword(pdev, PCIE_GLI_9767_UHS2_CTL2, &value);
> >>>> + value |= PCIE_GLI_9767_UHS2_CTL2_FORCE_PHY_RESETN;
> >>>> + pci_write_config_dword(pdev, PCIE_GLI_9767_UHS2_CTL2, value);
> >>>> + value &= ~PCIE_GLI_9767_UHS2_CTL2_FORCE_RESETN_VALUE;
> >>>> + pci_write_config_dword(pdev, PCIE_GLI_9767_UHS2_CTL2, value);
> >>>> + gl9767_vhs_read(pdev);
> >>>> +}
> >>>> +
> >>>> +static void sdhci_gl9767_uhs2_phy_reset_deassert(struct sdhci_host *host)
> >>>> +{
> >>>> + struct sdhci_pci_slot *slot = sdhci_priv(host);
> >>>> + struct pci_dev *pdev = slot->chip->pdev;
> >>>> + u32 value;
> >>>> +
> >>>> + gl9767_vhs_write(pdev);
> >>>> + pci_read_config_dword(pdev, PCIE_GLI_9767_UHS2_CTL2, &value);
> >>>> + value |= PCIE_GLI_9767_UHS2_CTL2_FORCE_RESETN_VALUE;
> >>>
> >>> Maybe add a small comment about PCIE_GLI_9767_UHS2_CTL2_FORCE_RESETN_VALUE
> >>> and PCIE_GLI_9767_UHS2_CTL2_FORCE_PHY_RESETN being updated separately.
> >>>
> >>>> + pci_write_config_dword(pdev, PCIE_GLI_9767_UHS2_CTL2, value);
> >>>> + value &= ~PCIE_GLI_9767_UHS2_CTL2_FORCE_PHY_RESETN;
> >>>> + pci_write_config_dword(pdev, PCIE_GLI_9767_UHS2_CTL2, value);
> >>>> + gl9767_vhs_read(pdev);
> >>>> +}
> >>>
> >>> sdhci_gl9767_uhs2_phy_reset_assert() and sdhci_gl9767_uhs2_phy_reset_deassert()
> >>> are fairly similar. Maybe consider:
> >>>
> >>> static void sdhci_gl9767_uhs2_phy_reset(struct sdhci_host *host, bool assert)
> >>> {
> >>> struct sdhci_pci_slot *slot = sdhci_priv(host);
> >>> struct pci_dev *pdev = slot->chip->pdev;
> >>> u32 value, set, clr;
> >>>
> >>> if (assert) {
> >>> set = PCIE_GLI_9767_UHS2_CTL2_FORCE_PHY_RESETN;
> >>> clr = PCIE_GLI_9767_UHS2_CTL2_FORCE_RESETN_VALUE;
> >>> } else {
> >>> set = PCIE_GLI_9767_UHS2_CTL2_FORCE_RESETN_VALUE;
> >>> clr = PCIE_GLI_9767_UHS2_CTL2_FORCE_PHY_RESETN;
> >>> }
> >>>
> >>> gl9767_vhs_write(pdev);
> >>> pci_read_config_dword(pdev, PCIE_GLI_9767_UHS2_CTL2, &value);
> >>> value |= set;
> >>> pci_write_config_dword(pdev, PCIE_GLI_9767_UHS2_CTL2, value);
> >>> value &= ~clr;
> >>> pci_write_config_dword(pdev, PCIE_GLI_9767_UHS2_CTL2, value);
> >>> gl9767_vhs_read(pdev);
> >>> }
> >>>
> >>
> >> OK, I will update it. Thank you.
> >>
> >>>
> >>>> +
> >>>> +static void __gl9767_uhs2_set_power(struct sdhci_host *host, unsigned char mode, unsigned short vdd)
> >>>> +{
> >>>> + u8 pwr = 0;
> >>>> +
> >>>> + if (mode != MMC_POWER_OFF) {
> >>>> + pwr = sdhci_get_vdd_value(vdd);
> >>>> + if (!pwr)
> >>>> + WARN(1, "%s: Invalid vdd %#x\n",
> >>>> + mmc_hostname(host->mmc), vdd);
> >>>> + pwr |= SDHCI_VDD2_POWER_180;
> >>>> + }
> >>>> +
> >>>> + if (host->pwr == pwr)
> >>>> + return;
> >>>> +
> >>>> + host->pwr = pwr;
> >>>> +
> >>>> + if (pwr == 0) {
> >>>> + sdhci_writeb(host, 0, SDHCI_POWER_CONTROL);
> >>>> + } else {
> >>>> + sdhci_writeb(host, 0, SDHCI_POWER_CONTROL);
> >>>> +
> >>>> + pwr |= SDHCI_POWER_ON;
> >>>> + sdhci_writeb(host, pwr & 0xf, SDHCI_POWER_CONTROL);
> >>>> + mdelay(5);
> >>>
> >>> Can be mmc_delay(5)
> >>>
> >>>> +
> >>>> + sdhci_gl9767_uhs2_phy_reset_assert(host);
> >>>> + pwr |= SDHCI_VDD2_POWER_ON;
> >>>> + sdhci_writeb(host, pwr, SDHCI_POWER_CONTROL);
> >>>> + mdelay(5);
> >>>
> >>> Can be mmc_delay(5)
> >>
> >> I may not modify it now.
> >> mmc_delay() is in "drivers/mmc/core/core.h".
> >> If sdhci-pci-gli.c only includes "../core/core.h", the compiler will
> >> report some errors.
> >
> > Ah, just add another headersand it will build.
> >
> > --- a/drivers/mmc/host/sdhci-pci-gli.c
> > +++ b/drivers/mmc/host/sdhci-pci-gli.c
> > @@ -14,6 +14,8 @@
> > #include <linux/delay.h>
> > #include <linux/of.h>
> > #include <linux/iopoll.h>
> > +#include <linux/mmc/host.h>
> > +#include "../core/core.h"
> > #include "sdhci.h"
> > #include "sdhci-cqhci.h"
> > #include "sdhci-pci.h"
> > @@ -968,10 +970,10 @@ static void gl9755_set_power(struct sdhci_host
> > *host, unsigned char mode,
> >
> > sdhci_writeb(host, pwr & 0xf, SDHCI_POWER_CONTROL);
> > /* wait stable */
> > - mdelay(5);
> > + mmc_delay(5);
> > sdhci_writeb(host, pwr, SDHCI_POWER_CONTROL);
> > /* wait stable */
> > - mdelay(5);
> > + mmc_delay(5);
> > sdhci_gli_overcurrent_event_enable(host, true);
> > }
>
> It seems mmc_delay() is for core mmc code only, not host drivers.
> But the issue with mdelay is that it does not sleep. Other options
> are msleep() or usleep_range().
>
Ok, I will use usleep_range() instead of mdelay().
Thanks for the suggestion.
Best regards,
Ben Chuang
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2025-09-02 8:55 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2025-09-01 9:42 [PATCH 2/2] mmc: sdhci-pci-gli: GL9767: Fix initializing the UHS-II interface during a power-on Ben Chuang
2025-09-01 17:01 ` Adrian Hunter
2025-09-02 6:33 ` Ben Chuang
2025-09-02 7:12 ` Ben Chuang
2025-09-02 7:47 ` Adrian Hunter
2025-09-02 8:54 ` Ben Chuang
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).