diff mbox series

[v2,2/4] mmc: sdhci-brcmstb: Enable Clock Gating to save power

Message ID 20220427180853.35970-3-kdasu.kdev@gmail.com (mailing list archive)
State New, archived
Headers show
Series mmc: sdhci-brcmstb: host controller clock enhancements | expand

Commit Message

Kamal Dasu April 27, 2022, 6:08 p.m. UTC 
From: Al Cooper <alcooperx@gmail.com>

Enabling this feature will allow the controller to stop the bus
clock when the bus is idle. The feature is not part of the standard
and is unique to newer Arasan cores and is enabled with a bit in a
vendor specific register. This feature will only be enabled for
non-removable devices because they don't switch the voltage and
clock gating breaks SD Card volatge switching.

Signed-off-by: Al Cooper <alcooperx@gmail.com>
Signed-off-by: Kamal Dasu <kdasu.kdev@gmail.com>
Acked-by: Florian Fainelli <f.fainelli@gmail.com>
Acked-by: Adrian Hunter <adrian.hunter@intel.com>
---
 drivers/mmc/host/sdhci-brcmstb.c | 35 +++++++++++++++++++++++++++++++-
 1 file changed, 34 insertions(+), 1 deletion(-)

Comments

Ulf Hansson April 27, 2022, 9:39 p.m. UTC | #1 
On Wed, 27 Apr 2022 at 20:09, Kamal Dasu <kdasu.kdev@gmail.com> wrote:
>
> From: Al Cooper <alcooperx@gmail.com>
>
> Enabling this feature will allow the controller to stop the bus
> clock when the bus is idle. The feature is not part of the standard
> and is unique to newer Arasan cores and is enabled with a bit in a
> vendor specific register. This feature will only be enabled for
> non-removable devices because they don't switch the voltage and
> clock gating breaks SD Card volatge switching.

Rather than using a HW specific thing for this, it may be better to
use runtime PM. There are plenty of examples to get inspired from, so
it should be rather easy to implement, I think. More importantly, it
should work for both (e)MMC and SD cards, unless there are some
specific things to manage for this controller.

When it comes to SDIO, some driver simply bumps the runtime PM usage
count (pm_runtime_get_noresume()) to prevent the device from being
runtime suspended. There are ways to work around this, let me know if
you need some guidance around how to fix that too.

That said, I am not entirely opposed to $subject patch, but I wanted
to point out that there are better alternatives.

Kind regards
Uffe

>
> Signed-off-by: Al Cooper <alcooperx@gmail.com>
> Signed-off-by: Kamal Dasu <kdasu.kdev@gmail.com>
> Acked-by: Florian Fainelli <f.fainelli@gmail.com>
> Acked-by: Adrian Hunter <adrian.hunter@intel.com>
> ---
>  drivers/mmc/host/sdhci-brcmstb.c | 35 +++++++++++++++++++++++++++++++-
>  1 file changed, 34 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/mmc/host/sdhci-brcmstb.c b/drivers/mmc/host/sdhci-brcmstb.c
> index 244780481193..683d0c685748 100644
> --- a/drivers/mmc/host/sdhci-brcmstb.c
> +++ b/drivers/mmc/host/sdhci-brcmstb.c
> @@ -17,11 +17,14 @@
>
>  #define SDHCI_VENDOR 0x78
>  #define  SDHCI_VENDOR_ENHANCED_STRB 0x1
> +#define  SDHCI_VENDOR_GATE_SDCLK_EN 0x2
>
>  #define BRCMSTB_MATCH_FLAGS_NO_64BIT           BIT(0)
>  #define BRCMSTB_MATCH_FLAGS_BROKEN_TIMEOUT     BIT(1)
> +#define BRCMSTB_MATCH_FLAGS_HAS_CLOCK_GATE     BIT(2)
>
>  #define BRCMSTB_PRIV_FLAGS_HAS_CQE             BIT(0)
> +#define BRCMSTB_PRIV_FLAGS_GATE_CLOCK          BIT(1)
>
>  #define SDHCI_ARASAN_CQE_BASE_ADDR             0x200
>
> @@ -36,6 +39,27 @@ struct brcmstb_match_priv {
>         const unsigned int flags;
>  };
>
> +static inline void enable_clock_gating(struct sdhci_host *host)
> +{
> +       u32 reg;
> +
> +       reg = sdhci_readl(host, SDHCI_VENDOR);
> +       reg |= SDHCI_VENDOR_GATE_SDCLK_EN;
> +       sdhci_writel(host, reg, SDHCI_VENDOR);
> +}
> +
> +void brcmstb_reset(struct sdhci_host *host, u8 mask)
> +{
> +       struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
> +       struct sdhci_brcmstb_priv *priv = sdhci_pltfm_priv(pltfm_host);
> +
> +       sdhci_reset(host, mask);
> +
> +       /* Reset will clear this, so re-enable it */
> +       if (priv->flags & BRCMSTB_PRIV_FLAGS_GATE_CLOCK)
> +               enable_clock_gating(host);
> +}
> +
>  static void sdhci_brcmstb_hs400es(struct mmc_host *mmc, struct mmc_ios *ios)
>  {
>         struct sdhci_host *host = mmc_priv(mmc);
> @@ -131,7 +155,7 @@ static struct sdhci_ops sdhci_brcmstb_ops = {
>  static struct sdhci_ops sdhci_brcmstb_ops_7216 = {
>         .set_clock = sdhci_brcmstb_set_clock,
>         .set_bus_width = sdhci_set_bus_width,
> -       .reset = sdhci_reset,
> +       .reset = brcmstb_reset,
>         .set_uhs_signaling = sdhci_brcmstb_set_uhs_signaling,
>  };
>
> @@ -147,6 +171,7 @@ static struct brcmstb_match_priv match_priv_7445 = {
>  };
>
>  static const struct brcmstb_match_priv match_priv_7216 = {
> +       .flags = BRCMSTB_MATCH_FLAGS_HAS_CLOCK_GATE,
>         .hs400es = sdhci_brcmstb_hs400es,
>         .ops = &sdhci_brcmstb_ops_7216,
>  };
> @@ -273,6 +298,14 @@ static int sdhci_brcmstb_probe(struct platform_device *pdev)
>         if (res)
>                 goto err;
>
> +       /*
> +        * Automatic clock gating does not work for SD cards that may
> +        * voltage switch so only enable it for non-removable devices.
> +        */
> +       if ((match_priv->flags & BRCMSTB_MATCH_FLAGS_HAS_CLOCK_GATE) &&
> +           (host->mmc->caps & MMC_CAP_NONREMOVABLE))
> +               priv->flags |= BRCMSTB_PRIV_FLAGS_GATE_CLOCK;
> +
>         /*
>          * If the chip has enhanced strobe and it's enabled, add
>          * callback
> --
> 2.17.1
>
Florian Fainelli April 27, 2022, 9:43 p.m. UTC | #2 
On 4/27/22 14:39, Ulf Hansson wrote:
> On Wed, 27 Apr 2022 at 20:09, Kamal Dasu <kdasu.kdev@gmail.com> wrote:
>>
>> From: Al Cooper <alcooperx@gmail.com>
>>
>> Enabling this feature will allow the controller to stop the bus
>> clock when the bus is idle. The feature is not part of the standard
>> and is unique to newer Arasan cores and is enabled with a bit in a
>> vendor specific register. This feature will only be enabled for
>> non-removable devices because they don't switch the voltage and
>> clock gating breaks SD Card volatge switching.
> 
> Rather than using a HW specific thing for this, it may be better to
> use runtime PM. There are plenty of examples to get inspired from, so
> it should be rather easy to implement, I think. More importantly, it
> should work for both (e)MMC and SD cards, unless there are some
> specific things to manage for this controller.
> 
> When it comes to SDIO, some driver simply bumps the runtime PM usage
> count (pm_runtime_get_noresume()) to prevent the device from being
> runtime suspended. There are ways to work around this, let me know if
> you need some guidance around how to fix that too.
> 
> That said, I am not entirely opposed to $subject patch, but I wanted
> to point out that there are better alternatives.

This is a good suggestion, I would not consider runtime PM and enabling 
the clock gating as being alternatives to one another, but rather 
complementary.
Ulf Hansson May 4, 2022, 10:37 a.m. UTC | #3 
On Wed, 27 Apr 2022 at 20:09, Kamal Dasu <kdasu.kdev@gmail.com> wrote:
>
> From: Al Cooper <alcooperx@gmail.com>
>
> Enabling this feature will allow the controller to stop the bus
> clock when the bus is idle. The feature is not part of the standard
> and is unique to newer Arasan cores and is enabled with a bit in a
> vendor specific register. This feature will only be enabled for
> non-removable devices because they don't switch the voltage and
> clock gating breaks SD Card volatge switching.
>
> Signed-off-by: Al Cooper <alcooperx@gmail.com>
> Signed-off-by: Kamal Dasu <kdasu.kdev@gmail.com>
> Acked-by: Florian Fainelli <f.fainelli@gmail.com>
> Acked-by: Adrian Hunter <adrian.hunter@intel.com>

Applied for next, thanks!

Kind regards
Uffe


> ---
>  drivers/mmc/host/sdhci-brcmstb.c | 35 +++++++++++++++++++++++++++++++-
>  1 file changed, 34 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/mmc/host/sdhci-brcmstb.c b/drivers/mmc/host/sdhci-brcmstb.c
> index 244780481193..683d0c685748 100644
> --- a/drivers/mmc/host/sdhci-brcmstb.c
> +++ b/drivers/mmc/host/sdhci-brcmstb.c
> @@ -17,11 +17,14 @@
>
>  #define SDHCI_VENDOR 0x78
>  #define  SDHCI_VENDOR_ENHANCED_STRB 0x1
> +#define  SDHCI_VENDOR_GATE_SDCLK_EN 0x2
>
>  #define BRCMSTB_MATCH_FLAGS_NO_64BIT           BIT(0)
>  #define BRCMSTB_MATCH_FLAGS_BROKEN_TIMEOUT     BIT(1)
> +#define BRCMSTB_MATCH_FLAGS_HAS_CLOCK_GATE     BIT(2)
>
>  #define BRCMSTB_PRIV_FLAGS_HAS_CQE             BIT(0)
> +#define BRCMSTB_PRIV_FLAGS_GATE_CLOCK          BIT(1)
>
>  #define SDHCI_ARASAN_CQE_BASE_ADDR             0x200
>
> @@ -36,6 +39,27 @@ struct brcmstb_match_priv {
>         const unsigned int flags;
>  };
>
> +static inline void enable_clock_gating(struct sdhci_host *host)
> +{
> +       u32 reg;
> +
> +       reg = sdhci_readl(host, SDHCI_VENDOR);
> +       reg |= SDHCI_VENDOR_GATE_SDCLK_EN;
> +       sdhci_writel(host, reg, SDHCI_VENDOR);
> +}
> +
> +void brcmstb_reset(struct sdhci_host *host, u8 mask)
> +{
> +       struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
> +       struct sdhci_brcmstb_priv *priv = sdhci_pltfm_priv(pltfm_host);
> +
> +       sdhci_reset(host, mask);
> +
> +       /* Reset will clear this, so re-enable it */
> +       if (priv->flags & BRCMSTB_PRIV_FLAGS_GATE_CLOCK)
> +               enable_clock_gating(host);
> +}
> +
>  static void sdhci_brcmstb_hs400es(struct mmc_host *mmc, struct mmc_ios *ios)
>  {
>         struct sdhci_host *host = mmc_priv(mmc);
> @@ -131,7 +155,7 @@ static struct sdhci_ops sdhci_brcmstb_ops = {
>  static struct sdhci_ops sdhci_brcmstb_ops_7216 = {
>         .set_clock = sdhci_brcmstb_set_clock,
>         .set_bus_width = sdhci_set_bus_width,
> -       .reset = sdhci_reset,
> +       .reset = brcmstb_reset,
>         .set_uhs_signaling = sdhci_brcmstb_set_uhs_signaling,
>  };
>
> @@ -147,6 +171,7 @@ static struct brcmstb_match_priv match_priv_7445 = {
>  };
>
>  static const struct brcmstb_match_priv match_priv_7216 = {
> +       .flags = BRCMSTB_MATCH_FLAGS_HAS_CLOCK_GATE,
>         .hs400es = sdhci_brcmstb_hs400es,
>         .ops = &sdhci_brcmstb_ops_7216,
>  };
> @@ -273,6 +298,14 @@ static int sdhci_brcmstb_probe(struct platform_device *pdev)
>         if (res)
>                 goto err;
>
> +       /*
> +        * Automatic clock gating does not work for SD cards that may
> +        * voltage switch so only enable it for non-removable devices.
> +        */
> +       if ((match_priv->flags & BRCMSTB_MATCH_FLAGS_HAS_CLOCK_GATE) &&
> +           (host->mmc->caps & MMC_CAP_NONREMOVABLE))
> +               priv->flags |= BRCMSTB_PRIV_FLAGS_GATE_CLOCK;
> +
>         /*
>          * If the chip has enhanced strobe and it's enabled, add
>          * callback
> --
> 2.17.1
>
diff mbox series

Patch

diff --git a/drivers/mmc/host/sdhci-brcmstb.c b/drivers/mmc/host/sdhci-brcmstb.c
index 244780481193..683d0c685748 100644
--- a/drivers/mmc/host/sdhci-brcmstb.c
+++ b/drivers/mmc/host/sdhci-brcmstb.c
@@ -17,11 +17,14 @@ 
 
 #define SDHCI_VENDOR 0x78
 #define  SDHCI_VENDOR_ENHANCED_STRB 0x1
+#define  SDHCI_VENDOR_GATE_SDCLK_EN 0x2
 
 #define BRCMSTB_MATCH_FLAGS_NO_64BIT		BIT(0)
 #define BRCMSTB_MATCH_FLAGS_BROKEN_TIMEOUT	BIT(1)
+#define BRCMSTB_MATCH_FLAGS_HAS_CLOCK_GATE	BIT(2)
 
 #define BRCMSTB_PRIV_FLAGS_HAS_CQE		BIT(0)
+#define BRCMSTB_PRIV_FLAGS_GATE_CLOCK		BIT(1)
 
 #define SDHCI_ARASAN_CQE_BASE_ADDR		0x200
 
@@ -36,6 +39,27 @@  struct brcmstb_match_priv {
 	const unsigned int flags;
 };
 
+static inline void enable_clock_gating(struct sdhci_host *host)
+{
+	u32 reg;
+
+	reg = sdhci_readl(host, SDHCI_VENDOR);
+	reg |= SDHCI_VENDOR_GATE_SDCLK_EN;
+	sdhci_writel(host, reg, SDHCI_VENDOR);
+}
+
+void brcmstb_reset(struct sdhci_host *host, u8 mask)
+{
+	struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
+	struct sdhci_brcmstb_priv *priv = sdhci_pltfm_priv(pltfm_host);
+
+	sdhci_reset(host, mask);
+
+	/* Reset will clear this, so re-enable it */
+	if (priv->flags & BRCMSTB_PRIV_FLAGS_GATE_CLOCK)
+		enable_clock_gating(host);
+}
+
 static void sdhci_brcmstb_hs400es(struct mmc_host *mmc, struct mmc_ios *ios)
 {
 	struct sdhci_host *host = mmc_priv(mmc);
@@ -131,7 +155,7 @@  static struct sdhci_ops sdhci_brcmstb_ops = {
 static struct sdhci_ops sdhci_brcmstb_ops_7216 = {
 	.set_clock = sdhci_brcmstb_set_clock,
 	.set_bus_width = sdhci_set_bus_width,
-	.reset = sdhci_reset,
+	.reset = brcmstb_reset,
 	.set_uhs_signaling = sdhci_brcmstb_set_uhs_signaling,
 };
 
@@ -147,6 +171,7 @@  static struct brcmstb_match_priv match_priv_7445 = {
 };
 
 static const struct brcmstb_match_priv match_priv_7216 = {
+	.flags = BRCMSTB_MATCH_FLAGS_HAS_CLOCK_GATE,
 	.hs400es = sdhci_brcmstb_hs400es,
 	.ops = &sdhci_brcmstb_ops_7216,
 };
@@ -273,6 +298,14 @@  static int sdhci_brcmstb_probe(struct platform_device *pdev)
 	if (res)
 		goto err;
 
+	/*
+	 * Automatic clock gating does not work for SD cards that may
+	 * voltage switch so only enable it for non-removable devices.
+	 */
+	if ((match_priv->flags & BRCMSTB_MATCH_FLAGS_HAS_CLOCK_GATE) &&
+	    (host->mmc->caps & MMC_CAP_NONREMOVABLE))
+		priv->flags |= BRCMSTB_PRIV_FLAGS_GATE_CLOCK;
+
 	/*
 	 * If the chip has enhanced strobe and it's enabled, add
 	 * callback