Skip to contentSkip to conversation

mmc: sdhci-brcmstb: Add support for optional sdio_freq clock

PATCH Superseded Kamal Dasu 2 participants 5 messages May 17, 2022

Subsystems (5)

v2 of 3 Superseded Latest is v3 →

lore
Share & tools
lore

Kamal Dasu

author
#0 lore raw

v2 cover mmc: sdhci-brcmstb: Add support for optional sdio_freq clock

v2 changes :
- Added Reviewed-by tag to PATCH v2 1/2
- In PATCH v2 2/2 Used host->mmc->f_max instead of parsing the device tree again, /s/clock-frequency/max-frequency in commit message

v1 Changes :
Sending the remaining 2 patches separately after implementing review comments for the patches 3/4 and 4/4 as part of the following:
"mmc: sdhci-brcmstb: host controller clock enhancements"
https://lore.kernel.org/linux-arm-kernel/[email protected]/t/
changes are rebased over
git://git.kernel.org/pub/scm/linux/kernel/git/ulfh/mmc.git refs/heads/next

tested brcm,sdhci-brcmstb.yaml and driver with and without sdio_freq clock.

Al Cooper (1):
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      | 25 +++++++++++++----
 drivers/mmc/host/sdhci-brcmstb.c              | 28 +++++++++++++++++++
 2 files changed, 47 insertions(+), 6 deletions(-)
Signature · 1 line
2.17.1
author
#1 lore raw

PATCH v2 1/2 dt-bindings: mmc: Add Broadcom optional sdio_freq clock

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 "max-frequency" specifies a base clock frequency in Hz that overrides the base clock frequency in the CAPS registers.

---
 .../bindings/mmc/brcm,sdhci-brcmstb.yaml      | 25 ++++++++++++++-----
 1 file changed, 19 insertions(+), 6 deletions(-)
Documentation/devicetree/bindings/mmc/brcm,sdhci-brcmstb.yaml +19 -6
index b672202fff4e..3a4f6e75bf5e 100644
@@ -10,9 +10,6 @@ maintainers:
   - Al Cooper <[email protected]>
   - Florian Fainelli <[email protected]>
 
-allOf:
-  - $ref: mmc-controller.yaml#
-
 properties:
   compatible:
     oneOf:
@@ -42,23 +39,39 @@ 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: handle to improved 150Mhz clock for sdhci controller (Optional clock)
 
   clock-names:
+    minItems: 1
     items:
       - const: sw_sdio
+      - const: sdio_freq # Optional clock
 
   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:
+        - max-frequency
+
 required:
   - compatible
   - reg
   - interrupts
   - clocks
+  - clock-names
 
 unevaluatedProperties: false
 
Signature · 1 line
2.17.1
Acked-by on v2 1/2
#2 lore raw

Re: [PATCH v2 1/2] dt-bindings: mmc: Add Broadcom optional sdio_freq clock

Quoted 8 lines · 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 "max-frequency" specifies a base clock frequency in Hz that overrides the base clock frequency in the CAPS registers.

Signature · 1 line
Florian
author
#3 lore raw

PATCH v2 2/2 mmc: sdhci-brcmstb: Add ability to increase max clock rate for 72116b0

In reply to Kamal Dasu

From: Al Cooper <[email protected]>

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, "max-frequency", to enable this functionality and will get the actual rate in MHz from the property to allow various speeds to be requested.

---
 drivers/mmc/host/sdhci-brcmstb.c | 28 ++++++++++++++++++++++++++++
 1 file changed, 28 insertions(+)
drivers/mmc/host/sdhci-brcmstb.c +28 -0
index 8eb57de48e0c..bb614a5e1ea4 100644
@@ -250,6 +250,8 @@ 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;
+	struct clk *master_clk;
+	u32 actual_clock_mhz;
 	struct sdhci_host *host;
 	struct resource *iomem;
 	struct clk *clk;
@@ -330,6 +332,32 @@ 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 (!(host->mmc->f_max))
+		goto add_host;
+
+	master_clk = devm_clk_get(&pdev->dev, "sdio_freq");
+	if (IS_ERR(master_clk)) {
+		dev_warn(&pdev->dev, "Clock for \"sdio_freq\" not found\n");
+		goto add_host;
+	} else {
+		res = clk_prepare_enable(master_clk);
+		if (res)
+			goto err;
+	}
+
+	/* set improved clock rate */
+	clk_set_rate(master_clk, host->mmc->f_max);
+	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);
+
+add_host:
 	res = sdhci_brcmstb_add_host(host, priv);
 	if (res)
 		goto err;
Signature · 1 line
2.17.1
#4 lore raw

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

Hi Kamal,

Quoted 31 lines · Kamal Dasu

From: Al Cooper <[email protected]>

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, "max-frequency", to enable this functionality and will get the actual rate in MHz from the property to allow various speeds to be requested.

---
  drivers/mmc/host/sdhci-brcmstb.c | 28 ++++++++++++++++++++++++++++
  1 file changed, 28 insertions(+)
drivers/mmc/host/sdhci-brcmstb.c +2 -0
index 8eb57de48e0c..bb614a5e1ea4 100644
@@ -250,6 +250,8 @@ 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;
+	struct clk *master_clk;
+	u32 actual_clock_mhz;
  	struct sdhci_host *host;
  	struct resource *iomem;
  	struct clk *clk;

drivers/mmc/host/sdhci-brcmstb.c static int sdhci_brcmstb_probe(struct platform_device *pdev) · line 339

Quoted 8 lines
drivers/mmc/host/sdhci-brcmstb.c +5 -0
@@ -330,6 +332,32 @@ 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 (!(host->mmc->f_max))
+		goto add_host;
+
+	master_clk = devm_clk_get(&pdev->dev, "sdio_freq");

This looks like a candidate for devm_clk_get_optional() since the clock is optional. Then you can call clk_prepare_enable() unconditionally even if it is NULL/non-existent.

drivers/mmc/host/sdhci-brcmstb.c static int sdhci_brcmstb_probe(struct platform_device *pdev) · line 346

Quoted 7 lines
drivers/mmc/host/sdhci-brcmstb.c +7 -0
+	if (IS_ERR(master_clk)) {
+		dev_warn(&pdev->dev, "Clock for \"sdio_freq\" not found\n");
+		goto add_host;
+	} else {
+		res = clk_prepare_enable(master_clk);
+		if (res)
+			goto err;

It looks like we may be leaving the clock enabled even when we did not want to (e.g.: error path) and do not we need to turn if off, respectively turn it back on in .suspend() and .resume()?

Quoted 7 lines
drivers/mmc/host/sdhci-brcmstb.c +7 -0
+	}
+
+	/* set improved clock rate */
+	clk_set_rate(master_clk, host->mmc->f_max);
+	actual_clock_mhz = clk_get_rate(master_clk) / 1000000;
+
+	host->caps &= ~SDHCI_CLOCK_V3_BASE_MASK;

drivers/mmc/host/sdhci-brcmstb.c static int sdhci_brcmstb_probe(struct platform_device *pdev) · line 363

Quoted 10 lines
drivers/mmc/host/sdhci-brcmstb.c +7 -0
+	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);
+
+add_host:
  	res = sdhci_brcmstb_add_host(host, priv);
  	if (res)
  		goto err;

It looks like we would need to unwind the clk_prepare_enable(master_clk) in case of failures here.

Signature · 1 line
Florian