[PATCH 0/5] mmc: sdhci-brcmstb: host controller clock enhancements

[PATCH 0/5] mmc: sdhci-brcmstb: host controller clock enhancements

[Kamal Dasu]

This change set contains broadcom settop sdhci controller clock related
improvements.

Al Cooper (4):
  mmc: sdhci-brcmstb: "mmc1: Internal clock never stabilised." seen on
    72113
  mmc: sdhci-brcmstb: Re-organize flags
  mmc: sdhci-brcmstb: Enable Clock Gating to save power
  mmc: sdhci-brcmstb: Add ability to increase max clock rate for 72116b0

Kamal Dasu (1):
  dt-bindings: mmc: Add Broadcom optional sdio_freq clock

 .../bindings/mmc/brcm,sdhci-brcmstb.yaml      | 29 +++++-
 drivers/mmc/host/sdhci-brcmstb.c              | 96 +++++++++++++++----
 2 files changed, 103 insertions(+), 22 deletions(-)

-- 
2.17.1

[PATCH 1/5] mmc: sdhci-brcmstb: "mmc1: Internal clock never stabilised." seen on 72113

[Kamal Dasu]

From: Al Cooper <alcooperx@gmail.com>

The problem is in the .shutdown callback that was added to the
sdhci-iproc and sdhci-brcmstb drivers to save power in S5. The
shutdown callback will just call the sdhci_pltfm_suspend() function
to suspend the lower level driver and then stop the sdhci system
clock. The problem is that in some cases there can be a worker
thread in the "system_freezable_wq" work queue that is scanning
for a device every second. In normal system suspend, this queue
is suspended before the driver suspend is called. In shutdown the
queue is not suspended and the thread my run after we stop the
sdhci clock in the shutdown callback which will cause the "clock
never stabilised" error. The solution will be to have the shutdown
callback cancel the worker thread before calling suspend (and
stopping the sdhci clock).

NOTE: This is only happening on systems with the Legacy RPi SDIO
core because that's the only controller that doesn't have the
presence signal and needs to use a worker thread to do a 1 second
poll loop.

Fixes: 5b191dcba719 ("mmc: sdhci-brcmstb: Fix mmc timeout errors on S5 suspend")
Signed-off-by: Al Cooper <alcooperx@gmail.com>
Signed-off-by: Kamal Dasu <kdasu.kdev@gmail.com>
---
 drivers/mmc/host/sdhci-brcmstb.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/mmc/host/sdhci-brcmstb.c b/drivers/mmc/host/sdhci-brcmstb.c
index f24623aac2db..11037cd14cfa 100644
--- a/drivers/mmc/host/sdhci-brcmstb.c
+++ b/drivers/mmc/host/sdhci-brcmstb.c
@@ -313,6 +313,10 @@ static int sdhci_brcmstb_probe(struct platform_device *pdev)
 
 static void sdhci_brcmstb_shutdown(struct platform_device *pdev)
 {
+	struct sdhci_host *host = platform_get_drvdata(pdev);
+
+	/* Cancel possible rescan worker thread */
+	cancel_delayed_work_sync(&host->mmc->detect);
 	sdhci_pltfm_suspend(&pdev->dev);
 }
 
-- 
2.17.1

Re: [PATCH 1/5] mmc: sdhci-brcmstb: "mmc1: Internal clock never stabilised." seen on 72113

[Florian Fainelli]

On 4/21/22 11:27, Kamal Dasu wrote:
> From: Al Cooper <alcooperx@gmail.com>
> 
> The problem is in the .shutdown callback that was added to the
> sdhci-iproc and sdhci-brcmstb drivers to save power in S5. The
> shutdown callback will just call the sdhci_pltfm_suspend() function
> to suspend the lower level driver and then stop the sdhci system
> clock. The problem is that in some cases there can be a worker
> thread in the "system_freezable_wq" work queue that is scanning
> for a device every second. In normal system suspend, this queue
> is suspended before the driver suspend is called. In shutdown the
> queue is not suspended and the thread my run after we stop the
> sdhci clock in the shutdown callback which will cause the "clock
> never stabilised" error. The solution will be to have the shutdown
> callback cancel the worker thread before calling suspend (and
> stopping the sdhci clock).
> 
> NOTE: This is only happening on systems with the Legacy RPi SDIO
> core because that's the only controller that doesn't have the
> presence signal and needs to use a worker thread to do a 1 second
> poll loop.
> 
> Fixes: 5b191dcba719 ("mmc: sdhci-brcmstb: Fix mmc timeout errors on S5 suspend")
> 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>
-- 
Florian

Re: [PATCH 1/5] mmc: sdhci-brcmstb: "mmc1: Internal clock never stabilised." seen on 72113

[Adrian Hunter]

On 21/04/22 21:27, Kamal Dasu wrote:
> From: Al Cooper <alcooperx@gmail.com>
> 
> The problem is in the .shutdown callback that was added to the
> sdhci-iproc and sdhci-brcmstb drivers to save power in S5. The
> shutdown callback will just call the sdhci_pltfm_suspend() function
> to suspend the lower level driver and then stop the sdhci system
> clock. The problem is that in some cases there can be a worker
> thread in the "system_freezable_wq" work queue that is scanning
> for a device every second. In normal system suspend, this queue
> is suspended before the driver suspend is called. In shutdown the
> queue is not suspended and the thread my run after we stop the
> sdhci clock in the shutdown callback which will cause the "clock
> never stabilised" error. The solution will be to have the shutdown
> callback cancel the worker thread before calling suspend (and
> stopping the sdhci clock).
> 
> NOTE: This is only happening on systems with the Legacy RPi SDIO
> core because that's the only controller that doesn't have the
> presence signal and needs to use a worker thread to do a 1 second
> poll loop.
> 
> Fixes: 5b191dcba719 ("mmc: sdhci-brcmstb: Fix mmc timeout errors on S5 suspend")
> Signed-off-by: Al Cooper <alcooperx@gmail.com>
> Signed-off-by: Kamal Dasu <kdasu.kdev@gmail.com>
> ---
>  drivers/mmc/host/sdhci-brcmstb.c | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/drivers/mmc/host/sdhci-brcmstb.c b/drivers/mmc/host/sdhci-brcmstb.c
> index f24623aac2db..11037cd14cfa 100644
> --- a/drivers/mmc/host/sdhci-brcmstb.c
> +++ b/drivers/mmc/host/sdhci-brcmstb.c
> @@ -313,6 +313,10 @@ static int sdhci_brcmstb_probe(struct platform_device *pdev)
>  
>  static void sdhci_brcmstb_shutdown(struct platform_device *pdev)
>  {
> +	struct sdhci_host *host = platform_get_drvdata(pdev);
> +
> +	/* Cancel possible rescan worker thread */
> +	cancel_delayed_work_sync(&host->mmc->detect);
>  	sdhci_pltfm_suspend(&pdev->dev);
>  }
>  

I think we fixed that already with the commit below:


commit 66c915d09b942fb3b2b0cb2f56562180901fba17
Author: Ulf Hansson <ulf.hansson@linaro.org>
Date:   Fri Dec 3 15:15:54 2021 +0100

    mmc: core: Disable card detect during shutdown
    
    It's seems prone to problems by allowing card detect and its corresponding
    mmc_rescan() work to run, during platform shutdown. For example, we may end
    up turning off the power while initializing a card, which potentially could
    damage it.
    
    To avoid this scenario, let's add ->shutdown_pre() callback for the mmc host
    class device and then turn of the card detect from there.
    
    Reported-by: Al Cooper <alcooperx@gmail.com>
    Suggested-by: Adrian Hunter <adrian.hunter@intel.com>
    Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
    Cc: stable@vger.kernel.org
    Link: https://lore.kernel.org/r/20211203141555.105351-1-ulf.hansson@linaro.org

[PATCH 2/5] mmc: sdhci-brcmstb: Re-organize flags

[Kamal Dasu]

From: Al Cooper <alcooperx@gmail.com>

Re-organize the flags by basing the bit names on the flag that they
apply to. Also change the "flags" member in the "brcmstb_match_priv"
struct to const.

Signed-off-by: Al Cooper <alcooperx@gmail.com>
Signed-off-by: Kamal Dasu <kdasu.kdev@gmail.com>
---
 drivers/mmc/host/sdhci-brcmstb.c | 32 ++++++++++++++++----------------
 1 file changed, 16 insertions(+), 16 deletions(-)

diff --git a/drivers/mmc/host/sdhci-brcmstb.c b/drivers/mmc/host/sdhci-brcmstb.c
index 11037cd14cfa..f32aa045c26d 100644
--- a/drivers/mmc/host/sdhci-brcmstb.c
+++ b/drivers/mmc/host/sdhci-brcmstb.c
@@ -18,20 +18,22 @@
 #define SDHCI_VENDOR 0x78
 #define  SDHCI_VENDOR_ENHANCED_STRB 0x1
 
-#define BRCMSTB_PRIV_FLAGS_NO_64BIT		BIT(0)
-#define BRCMSTB_PRIV_FLAGS_BROKEN_TIMEOUT	BIT(1)
+#define BRCMSTB_MATCH_FLAGS_NO_64BIT		BIT(0)
+#define BRCMSTB_MATCH_FLAGS_BROKEN_TIMEOUT	BIT(1)
+
+#define BRCMSTB_PRIV_FLAGS_HAS_CQE		BIT(0)
 
 #define SDHCI_ARASAN_CQE_BASE_ADDR		0x200
 
 struct sdhci_brcmstb_priv {
 	void __iomem *cfg_regs;
-	bool has_cqe;
+	unsigned int flags;
 };
 
 struct brcmstb_match_priv {
 	void (*hs400es)(struct mmc_host *mmc, struct mmc_ios *ios);
 	struct sdhci_ops *ops;
-	unsigned int flags;
+	const unsigned int flags;
 };
 
 static void sdhci_brcmstb_hs400es(struct mmc_host *mmc, struct mmc_ios *ios)
@@ -134,13 +136,13 @@ static struct sdhci_ops sdhci_brcmstb_ops_7216 = {
 };
 
 static struct brcmstb_match_priv match_priv_7425 = {
-	.flags = BRCMSTB_PRIV_FLAGS_NO_64BIT |
-	BRCMSTB_PRIV_FLAGS_BROKEN_TIMEOUT,
+	.flags = BRCMSTB_MATCH_FLAGS_NO_64BIT |
+	BRCMSTB_MATCH_FLAGS_BROKEN_TIMEOUT,
 	.ops = &sdhci_brcmstb_ops,
 };
 
 static struct brcmstb_match_priv match_priv_7445 = {
-	.flags = BRCMSTB_PRIV_FLAGS_BROKEN_TIMEOUT,
+	.flags = BRCMSTB_MATCH_FLAGS_BROKEN_TIMEOUT,
 	.ops = &sdhci_brcmstb_ops,
 };
 
@@ -176,7 +178,7 @@ static int sdhci_brcmstb_add_host(struct sdhci_host *host,
 	bool dma64;
 	int ret;
 
-	if (!priv->has_cqe)
+	if ((priv->flags & BRCMSTB_PRIV_FLAGS_HAS_CQE) == 0)
 		return sdhci_add_host(host);
 
 	dev_dbg(mmc_dev(host->mmc), "CQE is enabled\n");
@@ -225,7 +227,6 @@ static int sdhci_brcmstb_probe(struct platform_device *pdev)
 	struct sdhci_brcmstb_priv *priv;
 	struct sdhci_host *host;
 	struct resource *iomem;
-	bool has_cqe = false;
 	struct clk *clk;
 	int res;
 
@@ -244,10 +245,6 @@ static int sdhci_brcmstb_probe(struct platform_device *pdev)
 		return res;
 
 	memset(&brcmstb_pdata, 0, sizeof(brcmstb_pdata));
-	if (device_property_read_bool(&pdev->dev, "supports-cqe")) {
-		has_cqe = true;
-		match_priv->ops->irq = sdhci_brcmstb_cqhci_irq;
-	}
 	brcmstb_pdata.ops = match_priv->ops;
 	host = sdhci_pltfm_init(pdev, &brcmstb_pdata,
 				sizeof(struct sdhci_brcmstb_priv));
@@ -258,7 +255,10 @@ static int sdhci_brcmstb_probe(struct platform_device *pdev)
 
 	pltfm_host = sdhci_priv(host);
 	priv = sdhci_pltfm_priv(pltfm_host);
-	priv->has_cqe = has_cqe;
+	if (device_property_read_bool(&pdev->dev, "supports-cqe")) {
+		priv->flags |= BRCMSTB_PRIV_FLAGS_HAS_CQE;
+		match_priv->ops->irq = sdhci_brcmstb_cqhci_irq;
+	}
 
 	/* Map in the non-standard CFG registers */
 	iomem = platform_get_resource(pdev, IORESOURCE_MEM, 1);
@@ -287,14 +287,14 @@ static int sdhci_brcmstb_probe(struct platform_device *pdev)
 	 * properties through mmc_of_parse().
 	 */
 	host->caps = sdhci_readl(host, SDHCI_CAPABILITIES);
-	if (match_priv->flags & BRCMSTB_PRIV_FLAGS_NO_64BIT)
+	if (match_priv->flags & BRCMSTB_MATCH_FLAGS_NO_64BIT)
 		host->caps &= ~SDHCI_CAN_64BIT;
 	host->caps1 = sdhci_readl(host, SDHCI_CAPABILITIES_1);
 	host->caps1 &= ~(SDHCI_SUPPORT_SDR50 | SDHCI_SUPPORT_SDR104 |
 			 SDHCI_SUPPORT_DDR50);
 	host->quirks |= SDHCI_QUIRK_MISSING_CAPS;
 
-	if (match_priv->flags & BRCMSTB_PRIV_FLAGS_BROKEN_TIMEOUT)
+	if (match_priv->flags & BRCMSTB_MATCH_FLAGS_BROKEN_TIMEOUT)
 		host->quirks |= SDHCI_QUIRK_BROKEN_TIMEOUT_VAL;
 
 	res = sdhci_brcmstb_add_host(host, priv);
-- 
2.17.1

Re: [PATCH 2/5] mmc: sdhci-brcmstb: Re-organize flags

[Florian Fainelli]

On 4/21/22 11:28, Kamal Dasu wrote:
> From: Al Cooper <alcooperx@gmail.com>
> 
> Re-organize the flags by basing the bit names on the flag that they
> apply to. Also change the "flags" member in the "brcmstb_match_priv"
> struct to const.
> 
> 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>
-- 
Florian

Re: [PATCH 2/5] mmc: sdhci-brcmstb: Re-organize flags

[Adrian Hunter]

On 21/04/22 21:28, Kamal Dasu wrote:
> From: Al Cooper <alcooperx@gmail.com>
> 
> Re-organize the flags by basing the bit names on the flag that they
> apply to. Also change the "flags" member in the "brcmstb_match_priv"
> struct to const.
> 
> Signed-off-by: Al Cooper <alcooperx@gmail.com>
> Signed-off-by: Kamal Dasu <kdasu.kdev@gmail.com>

Acked-by: Adrian Hunter <adrian.hunter@intel.com>

> ---
>  drivers/mmc/host/sdhci-brcmstb.c | 32 ++++++++++++++++----------------
>  1 file changed, 16 insertions(+), 16 deletions(-)
> 
> diff --git a/drivers/mmc/host/sdhci-brcmstb.c b/drivers/mmc/host/sdhci-brcmstb.c
> index 11037cd14cfa..f32aa045c26d 100644
> --- a/drivers/mmc/host/sdhci-brcmstb.c
> +++ b/drivers/mmc/host/sdhci-brcmstb.c
> @@ -18,20 +18,22 @@
>  #define SDHCI_VENDOR 0x78
>  #define  SDHCI_VENDOR_ENHANCED_STRB 0x1
>  
> -#define BRCMSTB_PRIV_FLAGS_NO_64BIT		BIT(0)
> -#define BRCMSTB_PRIV_FLAGS_BROKEN_TIMEOUT	BIT(1)
> +#define BRCMSTB_MATCH_FLAGS_NO_64BIT		BIT(0)
> +#define BRCMSTB_MATCH_FLAGS_BROKEN_TIMEOUT	BIT(1)
> +
> +#define BRCMSTB_PRIV_FLAGS_HAS_CQE		BIT(0)
>  
>  #define SDHCI_ARASAN_CQE_BASE_ADDR		0x200
>  
>  struct sdhci_brcmstb_priv {
>  	void __iomem *cfg_regs;
> -	bool has_cqe;
> +	unsigned int flags;
>  };
>  
>  struct brcmstb_match_priv {
>  	void (*hs400es)(struct mmc_host *mmc, struct mmc_ios *ios);
>  	struct sdhci_ops *ops;
> -	unsigned int flags;
> +	const unsigned int flags;
>  };
>  
>  static void sdhci_brcmstb_hs400es(struct mmc_host *mmc, struct mmc_ios *ios)
> @@ -134,13 +136,13 @@ static struct sdhci_ops sdhci_brcmstb_ops_7216 = {
>  };
>  
>  static struct brcmstb_match_priv match_priv_7425 = {
> -	.flags = BRCMSTB_PRIV_FLAGS_NO_64BIT |
> -	BRCMSTB_PRIV_FLAGS_BROKEN_TIMEOUT,
> +	.flags = BRCMSTB_MATCH_FLAGS_NO_64BIT |
> +	BRCMSTB_MATCH_FLAGS_BROKEN_TIMEOUT,
>  	.ops = &sdhci_brcmstb_ops,
>  };
>  
>  static struct brcmstb_match_priv match_priv_7445 = {
> -	.flags = BRCMSTB_PRIV_FLAGS_BROKEN_TIMEOUT,
> +	.flags = BRCMSTB_MATCH_FLAGS_BROKEN_TIMEOUT,
>  	.ops = &sdhci_brcmstb_ops,
>  };
>  
> @@ -176,7 +178,7 @@ static int sdhci_brcmstb_add_host(struct sdhci_host *host,
>  	bool dma64;
>  	int ret;
>  
> -	if (!priv->has_cqe)
> +	if ((priv->flags & BRCMSTB_PRIV_FLAGS_HAS_CQE) == 0)
>  		return sdhci_add_host(host);
>  
>  	dev_dbg(mmc_dev(host->mmc), "CQE is enabled\n");
> @@ -225,7 +227,6 @@ static int sdhci_brcmstb_probe(struct platform_device *pdev)
>  	struct sdhci_brcmstb_priv *priv;
>  	struct sdhci_host *host;
>  	struct resource *iomem;
> -	bool has_cqe = false;
>  	struct clk *clk;
>  	int res;
>  
> @@ -244,10 +245,6 @@ static int sdhci_brcmstb_probe(struct platform_device *pdev)
>  		return res;
>  
>  	memset(&brcmstb_pdata, 0, sizeof(brcmstb_pdata));
> -	if (device_property_read_bool(&pdev->dev, "supports-cqe")) {
> -		has_cqe = true;
> -		match_priv->ops->irq = sdhci_brcmstb_cqhci_irq;
> -	}
>  	brcmstb_pdata.ops = match_priv->ops;
>  	host = sdhci_pltfm_init(pdev, &brcmstb_pdata,
>  				sizeof(struct sdhci_brcmstb_priv));
> @@ -258,7 +255,10 @@ static int sdhci_brcmstb_probe(struct platform_device *pdev)
>  
>  	pltfm_host = sdhci_priv(host);
>  	priv = sdhci_pltfm_priv(pltfm_host);
> -	priv->has_cqe = has_cqe;
> +	if (device_property_read_bool(&pdev->dev, "supports-cqe")) {
> +		priv->flags |= BRCMSTB_PRIV_FLAGS_HAS_CQE;
> +		match_priv->ops->irq = sdhci_brcmstb_cqhci_irq;
> +	}
>  
>  	/* Map in the non-standard CFG registers */
>  	iomem = platform_get_resource(pdev, IORESOURCE_MEM, 1);
> @@ -287,14 +287,14 @@ static int sdhci_brcmstb_probe(struct platform_device *pdev)
>  	 * properties through mmc_of_parse().
>  	 */
>  	host->caps = sdhci_readl(host, SDHCI_CAPABILITIES);
> -	if (match_priv->flags & BRCMSTB_PRIV_FLAGS_NO_64BIT)
> +	if (match_priv->flags & BRCMSTB_MATCH_FLAGS_NO_64BIT)
>  		host->caps &= ~SDHCI_CAN_64BIT;
>  	host->caps1 = sdhci_readl(host, SDHCI_CAPABILITIES_1);
>  	host->caps1 &= ~(SDHCI_SUPPORT_SDR50 | SDHCI_SUPPORT_SDR104 |
>  			 SDHCI_SUPPORT_DDR50);
>  	host->quirks |= SDHCI_QUIRK_MISSING_CAPS;
>  
> -	if (match_priv->flags & BRCMSTB_PRIV_FLAGS_BROKEN_TIMEOUT)
> +	if (match_priv->flags & BRCMSTB_MATCH_FLAGS_BROKEN_TIMEOUT)
>  		host->quirks |= SDHCI_QUIRK_BROKEN_TIMEOUT_VAL;
>  
>  	res = sdhci_brcmstb_add_host(host, priv);

[PATCH 3/5] mmc: sdhci-brcmstb: Enable Clock Gating to save power

[Kamal Dasu]

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>
---
 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 f32aa045c26d..d5cb3e8978b2 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

Re: [PATCH 3/5] mmc: sdhci-brcmstb: Enable Clock Gating to save power

[Florian Fainelli]

On 4/21/22 11:28, Kamal Dasu 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>
-- 
Florian

Re: [PATCH 3/5] mmc: sdhci-brcmstb: Enable Clock Gating to save power

[Adrian Hunter]

On 21/04/22 21:28, Kamal Dasu 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: 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 f32aa045c26d..d5cb3e8978b2 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

[PATCH 4/5] dt-bindings: mmc: Add Broadcom optional sdio_freq clock

[Kamal Dasu]

The 72116B0 has improved SDIO controllers that allow the max clock
rate to be increased from a max of 100MHz to a max of 150MHz.
Optional "sdio_freq" clock is used to drive the bus clock if present
optional property "clock-frequency" specifies a base clock frequency
in Hz that overrides the base clock frequency in the CAPS registers.

Signed-off-by: Kamal Dasu <kdasu.kdev@gmail.com>
---
 .../bindings/mmc/brcm,sdhci-brcmstb.yaml      | 29 +++++++++++++++----
 1 file changed, 24 insertions(+), 5 deletions(-)

diff --git a/Documentation/devicetree/bindings/mmc/brcm,sdhci-brcmstb.yaml b/Documentation/devicetree/bindings/mmc/brcm,sdhci-brcmstb.yaml
index dccd5ad96981..1b45a918400a 100644
--- a/Documentation/devicetree/bindings/mmc/brcm,sdhci-brcmstb.yaml
+++ b/Documentation/devicetree/bindings/mmc/brcm,sdhci-brcmstb.yaml
@@ -10,8 +10,6 @@ maintainers:
   - Al Cooper <alcooperx@gmail.com>
   - Florian Fainelli <f.fainelli@gmail.com>
 
-allOf:
-  - $ref: mmc-controller.yaml#
 
 properties:
   compatible:
@@ -42,23 +40,44 @@ properties:
     maxItems: 1
 
   clocks:
-    maxItems: 1
-    description:
-      handle to core clock for the sdhci controller.
+    minItems: 1
+    items:
+      - description: handle to core clock for the sdhci controller
+      - description: improved 150Mhz clock for sdhci controller (Optional clock)
 
   clock-names:
+    minItems: 1
     items:
       - const: sw_sdio
+      - const: sdio_freq # Optional clock
+
+  clock-frequency:
+    description: Should be the frequency (in Hz) of the base controller clock
+    minimum: 400000
+    maximum: 150000000
 
   sdhci,auto-cmd12:
     type: boolean
     description: Specifies that controller should use auto CMD12
 
+allOf:
+  - $ref: mmc-controller.yaml#
+  - if:
+      properties:
+        clock-names:
+          contains:
+            const: sdio_freq
+
+  - then:
+      required:
+        - clock-frequency
+
 required:
   - compatible
   - reg
   - interrupts
   - clocks
+  - clock-names
 
 unevaluatedProperties: false
 
-- 
2.17.1

Re: [PATCH 4/5] dt-bindings: mmc: Add Broadcom optional sdio_freq clock

[Krzysztof Kozlowski]

On 21/04/2022 20:28, Kamal Dasu wrote:
> The 72116B0 has improved SDIO controllers that allow the max clock
> rate to be increased from a max of 100MHz to a max of 150MHz.
> Optional "sdio_freq" clock is used to drive the bus clock if present
> optional property "clock-frequency" specifies a base clock frequency
> in Hz that overrides the base clock frequency in the CAPS registers.
> 
> Signed-off-by: Kamal Dasu <kdasu.kdev@gmail.com>
> ---
>  .../bindings/mmc/brcm,sdhci-brcmstb.yaml      | 29 +++++++++++++++----
>  1 file changed, 24 insertions(+), 5 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/mmc/brcm,sdhci-brcmstb.yaml b/Documentation/devicetree/bindings/mmc/brcm,sdhci-brcmstb.yaml
> index dccd5ad96981..1b45a918400a 100644
> --- a/Documentation/devicetree/bindings/mmc/brcm,sdhci-brcmstb.yaml
> +++ b/Documentation/devicetree/bindings/mmc/brcm,sdhci-brcmstb.yaml
> @@ -10,8 +10,6 @@ maintainers:
>    - Al Cooper <alcooperx@gmail.com>
>    - Florian Fainelli <f.fainelli@gmail.com>
>  
> -allOf:
> -  - $ref: mmc-controller.yaml#
>  
>  properties:
>    compatible:
> @@ -42,23 +40,44 @@ properties:
>      maxItems: 1
>  
>    clocks:
> -    maxItems: 1
> -    description:
> -      handle to core clock for the sdhci controller.
> +    minItems: 1
> +    items:
> +      - description: handle to core clock for the sdhci controller
> +      - description: improved 150Mhz clock for sdhci controller (Optional clock)
>  
>    clock-names:
> +    minItems: 1
>      items:
>        - const: sw_sdio
> +      - const: sdio_freq # Optional clock
> +
> +  clock-frequency:
> +    description: Should be the frequency (in Hz) of the base controller clock
> +    minimum: 400000
> +    maximum: 150000000

Why do you need this property? The mmc-controller.yaml already has
max-frequency, so maybe that suits your needs?


Best regards,
Krzysztof

Re: [PATCH 4/5] dt-bindings: mmc: Add Broadcom optional sdio_freq clock

[Kamal Dasu]

On Fri, Apr 22, 2022 at 1:07 PM Krzysztof Kozlowski
<krzysztof.kozlowski@linaro.org> wrote:
>
> On 21/04/2022 20:28, Kamal Dasu wrote:
> > The 72116B0 has improved SDIO controllers that allow the max clock
> > rate to be increased from a max of 100MHz to a max of 150MHz.
> > Optional "sdio_freq" clock is used to drive the bus clock if present
> > optional property "clock-frequency" specifies a base clock frequency
> > in Hz that overrides the base clock frequency in the CAPS registers.
> >
> > Signed-off-by: Kamal Dasu <kdasu.kdev@gmail.com>
> > ---
> >  .../bindings/mmc/brcm,sdhci-brcmstb.yaml      | 29 +++++++++++++++----
> >  1 file changed, 24 insertions(+), 5 deletions(-)
> >
> > diff --git a/Documentation/devicetree/bindings/mmc/brcm,sdhci-brcmstb.yaml b/Documentation/devicetree/bindings/mmc/brcm,sdhci-brcmstb.yaml
> > index dccd5ad96981..1b45a918400a 100644
> > --- a/Documentation/devicetree/bindings/mmc/brcm,sdhci-brcmstb.yaml
> > +++ b/Documentation/devicetree/bindings/mmc/brcm,sdhci-brcmstb.yaml
> > @@ -10,8 +10,6 @@ maintainers:
> >    - Al Cooper <alcooperx@gmail.com>
> >    - Florian Fainelli <f.fainelli@gmail.com>
> >
> > -allOf:
> > -  - $ref: mmc-controller.yaml#
> >
> >  properties:
> >    compatible:
> > @@ -42,23 +40,44 @@ properties:
> >      maxItems: 1
> >
> >    clocks:
> > -    maxItems: 1
> > -    description:
> > -      handle to core clock for the sdhci controller.
> > +    minItems: 1
> > +    items:
> > +      - description: handle to core clock for the sdhci controller
> > +      - description: improved 150Mhz clock for sdhci controller (Optional clock)
> >
> >    clock-names:
> > +    minItems: 1
> >      items:
> >        - const: sw_sdio
> > +      - const: sdio_freq # Optional clock
> > +
> > +  clock-frequency:
> > +    description: Should be the frequency (in Hz) of the base controller clock
> > +    minimum: 400000
> > +    maximum: 150000000
>
> Why do you need this property? The mmc-controller.yaml already has
> max-frequency, so maybe that suits your needs?
>

Yes I can use max-frequency, I will make the changes to patch 4/5 and
change 5/5 and send a v2 patchset.

>
> Best regards,
> Krzysztof

Thanks
Kamal

[PATCH 5/5] mmc: sdhci-brcmstb: Add ability to increase max clock rate for 72116b0

[Kamal Dasu]

From: Al Cooper <alcooperx@gmail.com>

The 72116B0 has improved SDIO controllers that allow the max clock
rate to be increased from a max of 100MHz to a max of 150MHz. The
driver will need to get the clock and increase it's default rate
and override the caps register, that still indicates a max of 100MHz.
The new clock will be named "sdio_freq" in the DT node's "clock-names"
list. The driver will use a DT property, "clock-frequency", to
enable this functionality and will get the actual rate in MHz
from the property to allow various speeds to be requested.

Signed-off-by: Al Cooper <alcooperx@gmail.com>
Signed-off-by: Kamal Dasu <kdasu.kdev@gmail.com>
---
 drivers/mmc/host/sdhci-brcmstb.c | 25 +++++++++++++++++++++++++
 1 file changed, 25 insertions(+)

diff --git a/drivers/mmc/host/sdhci-brcmstb.c b/drivers/mmc/host/sdhci-brcmstb.c
index d5cb3e8978b2..4f3629a8e70a 100644
--- a/drivers/mmc/host/sdhci-brcmstb.c
+++ b/drivers/mmc/host/sdhci-brcmstb.c
@@ -250,6 +250,7 @@ static int sdhci_brcmstb_probe(struct platform_device *pdev)
 	struct sdhci_pltfm_host *pltfm_host;
 	const struct of_device_id *match;
 	struct sdhci_brcmstb_priv *priv;
+	uint32_t base_clock_hz = 0;
 	struct sdhci_host *host;
 	struct resource *iomem;
 	struct clk *clk;
@@ -330,6 +331,30 @@ static int sdhci_brcmstb_probe(struct platform_device *pdev)
 	if (match_priv->flags & BRCMSTB_MATCH_FLAGS_BROKEN_TIMEOUT)
 		host->quirks |= SDHCI_QUIRK_BROKEN_TIMEOUT_VAL;
 
+	/* Change the base clock frequency if the DT property exists */
+	if (device_property_read_u32(&pdev->dev, "clock-frequency",
+				     &base_clock_hz) == 0) {
+		struct clk *master_clk;
+		u32 actual_clock_mhz;
+
+		master_clk = devm_clk_get(&pdev->dev, "sdio_freq");
+		if (IS_ERR(master_clk)) {
+			dev_warn(&pdev->dev,
+				 "Clock for \"sdio_freq\" was not found\n");
+		} else {
+			clk_set_rate(master_clk, base_clock_hz);
+			actual_clock_mhz = clk_get_rate(master_clk) / 1000000;
+
+			host->caps &= ~SDHCI_CLOCK_V3_BASE_MASK;
+			host->caps |=
+				(actual_clock_mhz << SDHCI_CLOCK_BASE_SHIFT);
+			/* Disable presets because they are now incorrect */
+			host->quirks2 |= SDHCI_QUIRK2_PRESET_VALUE_BROKEN;
+			dev_dbg(&pdev->dev,
+				"Base Clock Frequency changed to %dMHz\n",
+				actual_clock_mhz);
+		}
+	}
 	res = sdhci_brcmstb_add_host(host, priv);
 	if (res)
 		goto err;
-- 
2.17.1

Re: [PATCH 5/5] mmc: sdhci-brcmstb: Add ability to increase max clock rate for 72116b0

[Adrian Hunter]

On 21/04/22 21:28, Kamal Dasu wrote:
> From: Al Cooper <alcooperx@gmail.com>
> 
> The 72116B0 has improved SDIO controllers that allow the max clock
> rate to be increased from a max of 100MHz to a max of 150MHz. The
> driver will need to get the clock and increase it's default rate
> and override the caps register, that still indicates a max of 100MHz.
> The new clock will be named "sdio_freq" in the DT node's "clock-names"
> list. The driver will use a DT property, "clock-frequency", to
> enable this functionality and will get the actual rate in MHz
> from the property to allow various speeds to be requested.
> 
> Signed-off-by: Al Cooper <alcooperx@gmail.com>
> Signed-off-by: Kamal Dasu <kdasu.kdev@gmail.com>
> ---
>  drivers/mmc/host/sdhci-brcmstb.c | 25 +++++++++++++++++++++++++
>  1 file changed, 25 insertions(+)
> 
> diff --git a/drivers/mmc/host/sdhci-brcmstb.c b/drivers/mmc/host/sdhci-brcmstb.c
> index d5cb3e8978b2..4f3629a8e70a 100644
> --- a/drivers/mmc/host/sdhci-brcmstb.c
> +++ b/drivers/mmc/host/sdhci-brcmstb.c
> @@ -250,6 +250,7 @@ static int sdhci_brcmstb_probe(struct platform_device *pdev)
>  	struct sdhci_pltfm_host *pltfm_host;
>  	const struct of_device_id *match;
>  	struct sdhci_brcmstb_priv *priv;
> +	uint32_t base_clock_hz = 0;

Prefer u32 i.e. uint32_t -> u32

>  	struct sdhci_host *host;
>  	struct resource *iomem;
>  	struct clk *clk;
> @@ -330,6 +331,30 @@ static int sdhci_brcmstb_probe(struct platform_device *pdev)
>  	if (match_priv->flags & BRCMSTB_MATCH_FLAGS_BROKEN_TIMEOUT)
>  		host->quirks |= SDHCI_QUIRK_BROKEN_TIMEOUT_VAL;
>  
> +	/* Change the base clock frequency if the DT property exists */
> +	if (device_property_read_u32(&pdev->dev, "clock-frequency",
> +				     &base_clock_hz) == 0) {
> +		struct clk *master_clk;
> +		u32 actual_clock_mhz;
> +
> +		master_clk = devm_clk_get(&pdev->dev, "sdio_freq");
> +		if (IS_ERR(master_clk)) {
> +			dev_warn(&pdev->dev,
> +				 "Clock for \"sdio_freq\" was not found\n");
> +		} else {
> +			clk_set_rate(master_clk, base_clock_hz);
> +			actual_clock_mhz = clk_get_rate(master_clk) / 1000000;
> +
> +			host->caps &= ~SDHCI_CLOCK_V3_BASE_MASK;
> +			host->caps |=
> +				(actual_clock_mhz << SDHCI_CLOCK_BASE_SHIFT);
> +			/* Disable presets because they are now incorrect */
> +			host->quirks2 |= SDHCI_QUIRK2_PRESET_VALUE_BROKEN;
> +			dev_dbg(&pdev->dev,
> +				"Base Clock Frequency changed to %dMHz\n",
> +				actual_clock_mhz);
> +		}
> +	}
>  	res = sdhci_brcmstb_add_host(host, priv);
>  	if (res)
>  		goto err;