[PATCH 0/2] mmc: sdhci-pci-gli: Remove unnecessary device-dependent code

[PATCH 0/2] mmc: sdhci-pci-gli: Remove unnecessary device-dependent code

[Bjorn Helgaas]

From: Bjorn Helgaas <bhelgaas@google.com>

Previously the driver hard-coded the config space locations of the Power
Management and the AER Capabilities and included device-specific #defines
for bits defined by the PCI spec.  This replaces those with the equivalents
from the PCI core.

This also replace hard-coded power state changes (to D3hot and back to D0)
with the pci_set_power_state() interface, which takes care of the required
delays after these transitions.

Bjorn Helgaas (2):
  mmc: sdhci-pci-gli: Use PCI AER definitions, not hard-coded values
  mmc: sdhci-pci-gli: Use pci_set_power_state(), not direct PMCSR writes

 drivers/mmc/host/sdhci-pci-gli.c | 46 +++++++++++++-------------------
 1 file changed, 18 insertions(+), 28 deletions(-)

-- 
2.34.1

[PATCH 1/2] mmc: sdhci-pci-gli: Use PCI AER definitions, not hard-coded values

[Bjorn Helgaas]

From: Bjorn Helgaas <bhelgaas@google.com>

015c9cbcf0ad ("mmc: sdhci-pci-gli: GL9750: Mask the replay timer timeout of
AER") added PCI_GLI_9750_CORRERR_MASK, the offset of the AER Capability in
config space, and PCI_GLI_9750_CORRERR_MASK_REPLAY_TIMER_TIMEOUT, the
Replay Timer Timeout bit in the AER Correctable Error Status register.

Use pci_find_ext_capability() to locate the AER Capability and use the
existing PCI_ERR_COR_REP_TIMER definition to mask the bit.

This removes a little bit of unnecessarily device-specific code and makes
AER-related things more greppable.

Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
---
 drivers/mmc/host/sdhci-pci-gli.c | 26 ++++++++++++++------------
 1 file changed, 14 insertions(+), 12 deletions(-)

diff --git a/drivers/mmc/host/sdhci-pci-gli.c b/drivers/mmc/host/sdhci-pci-gli.c
index 77911a57b12c..3d5543581537 100644
--- a/drivers/mmc/host/sdhci-pci-gli.c
+++ b/drivers/mmc/host/sdhci-pci-gli.c
@@ -28,9 +28,6 @@
 #define PCI_GLI_9750_PM_CTRL	0xFC
 #define   PCI_GLI_9750_PM_STATE	  GENMASK(1, 0)
 
-#define PCI_GLI_9750_CORRERR_MASK				0x214
-#define   PCI_GLI_9750_CORRERR_MASK_REPLAY_TIMER_TIMEOUT	  BIT(12)
-
 #define SDHCI_GLI_9750_CFG2          0x848
 #define   SDHCI_GLI_9750_CFG2_L1DLY    GENMASK(28, 24)
 #define   GLI_9750_CFG2_L1DLY_VALUE    0x1F
@@ -155,9 +152,6 @@
 #define PCI_GLI_9755_PM_CTRL     0xFC
 #define   PCI_GLI_9755_PM_STATE    GENMASK(1, 0)
 
-#define PCI_GLI_9755_CORRERR_MASK				0x214
-#define   PCI_GLI_9755_CORRERR_MASK_REPLAY_TIMER_TIMEOUT	  BIT(12)
-
 #define SDHCI_GLI_9767_GM_BURST_SIZE			0x510
 #define   SDHCI_GLI_9767_GM_BURST_SIZE_AXI_ALWAYS_SET	  BIT(8)
 
@@ -547,6 +541,7 @@ static void gl9750_hw_setting(struct sdhci_host *host)
 {
 	struct sdhci_pci_slot *slot = sdhci_priv(host);
 	struct pci_dev *pdev;
+	int aer;
 	u32 value;
 
 	pdev = slot->chip->pdev;
@@ -568,9 +563,12 @@ static void gl9750_hw_setting(struct sdhci_host *host)
 	pci_write_config_dword(pdev, PCI_GLI_9750_PM_CTRL, value);
 
 	/* mask the replay timer timeout of AER */
-	pci_read_config_dword(pdev, PCI_GLI_9750_CORRERR_MASK, &value);
-	value |= PCI_GLI_9750_CORRERR_MASK_REPLAY_TIMER_TIMEOUT;
-	pci_write_config_dword(pdev, PCI_GLI_9750_CORRERR_MASK, value);
+	aer = pci_find_ext_capability(pdev, PCI_EXT_CAP_ID_ERR);
+	if (aer) {
+		pci_read_config_dword(pdev, aer + PCI_ERR_COR_MASK, &value);
+		value |= PCI_ERR_COR_REP_TIMER;
+		pci_write_config_dword(pdev, aer + PCI_ERR_COR_MASK, value);
+	}
 
 	gl9750_wt_off(host);
 }
@@ -745,6 +743,7 @@ static void sdhci_gl9755_set_clock(struct sdhci_host *host, unsigned int clock)
 static void gl9755_hw_setting(struct sdhci_pci_slot *slot)
 {
 	struct pci_dev *pdev = slot->chip->pdev;
+	int aer;
 	u32 value;
 
 	gl9755_wt_on(pdev);
@@ -782,9 +781,12 @@ static void gl9755_hw_setting(struct sdhci_pci_slot *slot)
 	pci_write_config_dword(pdev, PCI_GLI_9755_PM_CTRL, value);
 
 	/* mask the replay timer timeout of AER */
-	pci_read_config_dword(pdev, PCI_GLI_9755_CORRERR_MASK, &value);
-	value |= PCI_GLI_9755_CORRERR_MASK_REPLAY_TIMER_TIMEOUT;
-	pci_write_config_dword(pdev, PCI_GLI_9755_CORRERR_MASK, value);
+	aer = pci_find_ext_capability(pdev, PCI_EXT_CAP_ID_ERR);
+	if (aer) {
+		pci_read_config_dword(pdev, aer + PCI_ERR_COR_MASK, &value);
+		value |= PCI_ERR_COR_REP_TIMER;
+		pci_write_config_dword(pdev, aer + PCI_ERR_COR_MASK, value);
+	}
 
 	gl9755_wt_off(pdev);
 }
-- 
2.34.1

[PATCH 2/2] mmc: sdhci-pci-gli: Use pci_set_power_state(), not direct PMCSR writes

[Bjorn Helgaas]

From: Bjorn Helgaas <bhelgaas@google.com>

d7133797e9e1 ("mmc: sdhci-pci-gli: A workaround to allow GL9750 to enter
ASPM L1.2") and 36ed2fd32b2c ("mmc: sdhci-pci-gli: A workaround to allow
GL9755 to enter ASPM L1.2") added writes to the Control register in the
Power Management Capability to put the device in D3hot and back to D0.

Use the pci_set_power_state() interface instead because these are generic
operations that don't need to be driver-specific.  Also, the PCI spec
requires some delays after these power transitions, and
pci_set_power_state() takes care of those, while d7133797e9e1 and
36ed2fd32b2c did not.

Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
---
 drivers/mmc/host/sdhci-pci-gli.c | 20 ++++----------------
 1 file changed, 4 insertions(+), 16 deletions(-)

diff --git a/drivers/mmc/host/sdhci-pci-gli.c b/drivers/mmc/host/sdhci-pci-gli.c
index 3d5543581537..0f81586a19df 100644
--- a/drivers/mmc/host/sdhci-pci-gli.c
+++ b/drivers/mmc/host/sdhci-pci-gli.c
@@ -25,9 +25,6 @@
 #define   GLI_9750_WT_EN_ON	    0x1
 #define   GLI_9750_WT_EN_OFF	    0x0
 
-#define PCI_GLI_9750_PM_CTRL	0xFC
-#define   PCI_GLI_9750_PM_STATE	  GENMASK(1, 0)
-
 #define SDHCI_GLI_9750_CFG2          0x848
 #define   SDHCI_GLI_9750_CFG2_L1DLY    GENMASK(28, 24)
 #define   GLI_9750_CFG2_L1DLY_VALUE    0x1F
@@ -149,9 +146,6 @@
 #define PCI_GLI_9755_MISC	    0x78
 #define   PCI_GLI_9755_MISC_SSC_OFF    BIT(26)
 
-#define PCI_GLI_9755_PM_CTRL     0xFC
-#define   PCI_GLI_9755_PM_STATE    GENMASK(1, 0)
-
 #define SDHCI_GLI_9767_GM_BURST_SIZE			0x510
 #define   SDHCI_GLI_9767_GM_BURST_SIZE_AXI_ALWAYS_SET	  BIT(8)
 
@@ -556,11 +550,8 @@ static void gl9750_hw_setting(struct sdhci_host *host)
 	sdhci_writel(host, value, SDHCI_GLI_9750_CFG2);
 
 	/* toggle PM state to allow GL9750 to enter ASPM L1.2 */
-	pci_read_config_dword(pdev, PCI_GLI_9750_PM_CTRL, &value);
-	value |= PCI_GLI_9750_PM_STATE;
-	pci_write_config_dword(pdev, PCI_GLI_9750_PM_CTRL, value);
-	value &= ~PCI_GLI_9750_PM_STATE;
-	pci_write_config_dword(pdev, PCI_GLI_9750_PM_CTRL, value);
+	pci_set_power_state(pdev, PCI_D3hot);
+	pci_set_power_state(pdev, PCI_D0);
 
 	/* mask the replay timer timeout of AER */
 	aer = pci_find_ext_capability(pdev, PCI_EXT_CAP_ID_ERR);
@@ -774,11 +765,8 @@ static void gl9755_hw_setting(struct sdhci_pci_slot *slot)
 	pci_write_config_dword(pdev, PCI_GLI_9755_CFG2, value);
 
 	/* toggle PM state to allow GL9755 to enter ASPM L1.2 */
-	pci_read_config_dword(pdev, PCI_GLI_9755_PM_CTRL, &value);
-	value |= PCI_GLI_9755_PM_STATE;
-	pci_write_config_dword(pdev, PCI_GLI_9755_PM_CTRL, value);
-	value &= ~PCI_GLI_9755_PM_STATE;
-	pci_write_config_dword(pdev, PCI_GLI_9755_PM_CTRL, value);
+	pci_set_power_state(pdev, PCI_D3hot);
+	pci_set_power_state(pdev, PCI_D0);
 
 	/* mask the replay timer timeout of AER */
 	aer = pci_find_ext_capability(pdev, PCI_EXT_CAP_ID_ERR);
-- 
2.34.1

Re: [PATCH 2/2] mmc: sdhci-pci-gli: Use pci_set_power_state(), not direct PMCSR writes

[Ben Chuang]

On Thu, Mar 28, 2024 at 5:49 AM Bjorn Helgaas <helgaas@kernel.org> wrote:
>
> From: Bjorn Helgaas <bhelgaas@google.com>
>
> d7133797e9e1 ("mmc: sdhci-pci-gli: A workaround to allow GL9750 to enter
> ASPM L1.2") and 36ed2fd32b2c ("mmc: sdhci-pci-gli: A workaround to allow
> GL9755 to enter ASPM L1.2") added writes to the Control register in the
> Power Management Capability to put the device in D3hot and back to D0.
>
> Use the pci_set_power_state() interface instead because these are generic
> operations that don't need to be driver-specific.  Also, the PCI spec
> requires some delays after these power transitions, and
> pci_set_power_state() takes care of those, while d7133797e9e1 and
> 36ed2fd32b2c did not.
>
> Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>

Hi Bjorn,

Thanks. It looks better than the vendor specific.

Best regards,
Ben Chuang

> ---
>  drivers/mmc/host/sdhci-pci-gli.c | 20 ++++----------------
>  1 file changed, 4 insertions(+), 16 deletions(-)
>
> diff --git a/drivers/mmc/host/sdhci-pci-gli.c b/drivers/mmc/host/sdhci-pci-gli.c
> index 3d5543581537..0f81586a19df 100644
> --- a/drivers/mmc/host/sdhci-pci-gli.c
> +++ b/drivers/mmc/host/sdhci-pci-gli.c
> @@ -25,9 +25,6 @@
>  #define   GLI_9750_WT_EN_ON        0x1
>  #define   GLI_9750_WT_EN_OFF       0x0
>
> -#define PCI_GLI_9750_PM_CTRL   0xFC
> -#define   PCI_GLI_9750_PM_STATE          GENMASK(1, 0)
> -
>  #define SDHCI_GLI_9750_CFG2          0x848
>  #define   SDHCI_GLI_9750_CFG2_L1DLY    GENMASK(28, 24)
>  #define   GLI_9750_CFG2_L1DLY_VALUE    0x1F
> @@ -149,9 +146,6 @@
>  #define PCI_GLI_9755_MISC          0x78
>  #define   PCI_GLI_9755_MISC_SSC_OFF    BIT(26)
>
> -#define PCI_GLI_9755_PM_CTRL     0xFC
> -#define   PCI_GLI_9755_PM_STATE    GENMASK(1, 0)
> -
>  #define SDHCI_GLI_9767_GM_BURST_SIZE                   0x510
>  #define   SDHCI_GLI_9767_GM_BURST_SIZE_AXI_ALWAYS_SET    BIT(8)
>
> @@ -556,11 +550,8 @@ static void gl9750_hw_setting(struct sdhci_host *host)
>         sdhci_writel(host, value, SDHCI_GLI_9750_CFG2);
>
>         /* toggle PM state to allow GL9750 to enter ASPM L1.2 */
> -       pci_read_config_dword(pdev, PCI_GLI_9750_PM_CTRL, &value);
> -       value |= PCI_GLI_9750_PM_STATE;
> -       pci_write_config_dword(pdev, PCI_GLI_9750_PM_CTRL, value);
> -       value &= ~PCI_GLI_9750_PM_STATE;
> -       pci_write_config_dword(pdev, PCI_GLI_9750_PM_CTRL, value);
> +       pci_set_power_state(pdev, PCI_D3hot);
> +       pci_set_power_state(pdev, PCI_D0);
>
>         /* mask the replay timer timeout of AER */
>         aer = pci_find_ext_capability(pdev, PCI_EXT_CAP_ID_ERR);
> @@ -774,11 +765,8 @@ static void gl9755_hw_setting(struct sdhci_pci_slot *slot)
>         pci_write_config_dword(pdev, PCI_GLI_9755_CFG2, value);
>
>         /* toggle PM state to allow GL9755 to enter ASPM L1.2 */
> -       pci_read_config_dword(pdev, PCI_GLI_9755_PM_CTRL, &value);
> -       value |= PCI_GLI_9755_PM_STATE;
> -       pci_write_config_dword(pdev, PCI_GLI_9755_PM_CTRL, value);
> -       value &= ~PCI_GLI_9755_PM_STATE;
> -       pci_write_config_dword(pdev, PCI_GLI_9755_PM_CTRL, value);
> +       pci_set_power_state(pdev, PCI_D3hot);
> +       pci_set_power_state(pdev, PCI_D0);
>
>         /* mask the replay timer timeout of AER */
>         aer = pci_find_ext_capability(pdev, PCI_EXT_CAP_ID_ERR);
> --
> 2.34.1
>
>

Re: [PATCH 2/2] mmc: sdhci-pci-gli: Use pci_set_power_state(), not direct PMCSR writes

[Ulf Hansson]

On Thu, 28 Mar 2024 at 02:01, Ben Chuang <benchuanggli@gmail.com> wrote:
>
> On Thu, Mar 28, 2024 at 5:49 AM Bjorn Helgaas <helgaas@kernel.org> wrote:
> >
> > From: Bjorn Helgaas <bhelgaas@google.com>
> >
> > d7133797e9e1 ("mmc: sdhci-pci-gli: A workaround to allow GL9750 to enter
> > ASPM L1.2") and 36ed2fd32b2c ("mmc: sdhci-pci-gli: A workaround to allow
> > GL9755 to enter ASPM L1.2") added writes to the Control register in the
> > Power Management Capability to put the device in D3hot and back to D0.
> >
> > Use the pci_set_power_state() interface instead because these are generic
> > operations that don't need to be driver-specific.  Also, the PCI spec
> > requires some delays after these power transitions, and
> > pci_set_power_state() takes care of those, while d7133797e9e1 and
> > 36ed2fd32b2c did not.
> >
> > Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
>
> Hi Bjorn,
>
> Thanks. It looks better than the vendor specific.
>
> Best regards,
> Ben Chuang

Hi Ben,

I assume I can consider your reply as a reviewed-by tag. If not,
please let me know.

Kind regards
Uffe


>
> > ---
> >  drivers/mmc/host/sdhci-pci-gli.c | 20 ++++----------------
> >  1 file changed, 4 insertions(+), 16 deletions(-)
> >
> > diff --git a/drivers/mmc/host/sdhci-pci-gli.c b/drivers/mmc/host/sdhci-pci-gli.c
> > index 3d5543581537..0f81586a19df 100644
> > --- a/drivers/mmc/host/sdhci-pci-gli.c
> > +++ b/drivers/mmc/host/sdhci-pci-gli.c
> > @@ -25,9 +25,6 @@
> >  #define   GLI_9750_WT_EN_ON        0x1
> >  #define   GLI_9750_WT_EN_OFF       0x0
> >
> > -#define PCI_GLI_9750_PM_CTRL   0xFC
> > -#define   PCI_GLI_9750_PM_STATE          GENMASK(1, 0)
> > -
> >  #define SDHCI_GLI_9750_CFG2          0x848
> >  #define   SDHCI_GLI_9750_CFG2_L1DLY    GENMASK(28, 24)
> >  #define   GLI_9750_CFG2_L1DLY_VALUE    0x1F
> > @@ -149,9 +146,6 @@
> >  #define PCI_GLI_9755_MISC          0x78
> >  #define   PCI_GLI_9755_MISC_SSC_OFF    BIT(26)
> >
> > -#define PCI_GLI_9755_PM_CTRL     0xFC
> > -#define   PCI_GLI_9755_PM_STATE    GENMASK(1, 0)
> > -
> >  #define SDHCI_GLI_9767_GM_BURST_SIZE                   0x510
> >  #define   SDHCI_GLI_9767_GM_BURST_SIZE_AXI_ALWAYS_SET    BIT(8)
> >
> > @@ -556,11 +550,8 @@ static void gl9750_hw_setting(struct sdhci_host *host)
> >         sdhci_writel(host, value, SDHCI_GLI_9750_CFG2);
> >
> >         /* toggle PM state to allow GL9750 to enter ASPM L1.2 */
> > -       pci_read_config_dword(pdev, PCI_GLI_9750_PM_CTRL, &value);
> > -       value |= PCI_GLI_9750_PM_STATE;
> > -       pci_write_config_dword(pdev, PCI_GLI_9750_PM_CTRL, value);
> > -       value &= ~PCI_GLI_9750_PM_STATE;
> > -       pci_write_config_dword(pdev, PCI_GLI_9750_PM_CTRL, value);
> > +       pci_set_power_state(pdev, PCI_D3hot);
> > +       pci_set_power_state(pdev, PCI_D0);
> >
> >         /* mask the replay timer timeout of AER */
> >         aer = pci_find_ext_capability(pdev, PCI_EXT_CAP_ID_ERR);
> > @@ -774,11 +765,8 @@ static void gl9755_hw_setting(struct sdhci_pci_slot *slot)
> >         pci_write_config_dword(pdev, PCI_GLI_9755_CFG2, value);
> >
> >         /* toggle PM state to allow GL9755 to enter ASPM L1.2 */
> > -       pci_read_config_dword(pdev, PCI_GLI_9755_PM_CTRL, &value);
> > -       value |= PCI_GLI_9755_PM_STATE;
> > -       pci_write_config_dword(pdev, PCI_GLI_9755_PM_CTRL, value);
> > -       value &= ~PCI_GLI_9755_PM_STATE;
> > -       pci_write_config_dword(pdev, PCI_GLI_9755_PM_CTRL, value);
> > +       pci_set_power_state(pdev, PCI_D3hot);
> > +       pci_set_power_state(pdev, PCI_D0);
> >
> >         /* mask the replay timer timeout of AER */
> >         aer = pci_find_ext_capability(pdev, PCI_EXT_CAP_ID_ERR);
> > --
> > 2.34.1
> >
> >

Re: [PATCH 0/2] mmc: sdhci-pci-gli: Remove unnecessary device-dependent code

[Ulf Hansson]

On Wed, 27 Mar 2024 at 22:48, Bjorn Helgaas <helgaas@kernel.org> wrote:
>
> From: Bjorn Helgaas <bhelgaas@google.com>
>
> Previously the driver hard-coded the config space locations of the Power
> Management and the AER Capabilities and included device-specific #defines
> for bits defined by the PCI spec.  This replaces those with the equivalents
> from the PCI core.
>
> This also replace hard-coded power state changes (to D3hot and back to D0)
> with the pci_set_power_state() interface, which takes care of the required
> delays after these transitions.
>
> Bjorn Helgaas (2):
>   mmc: sdhci-pci-gli: Use PCI AER definitions, not hard-coded values
>   mmc: sdhci-pci-gli: Use pci_set_power_state(), not direct PMCSR writes
>
>  drivers/mmc/host/sdhci-pci-gli.c | 46 +++++++++++++-------------------
>  1 file changed, 18 insertions(+), 28 deletions(-)
>

The series applied for next, thanks!

Kind regards
Uffe