[PATCH RFC 0/2] mmc: sdhci-pxav3: pinctrl setting for fast bus clocks

[PATCH RFC 0/2] mmc: sdhci-pxav3: pinctrl setting for fast bus clocks

[Duje Mihanović]

Hello,

This small series adds a pinctrl setting for fast MMC bus clocks to the
pxav3 driver. On bus clocks above 100 MHz, driving the data pins at a
higher current helps maintain signal quality.

This series is related to Marvell PXA1908 SoC support; the latest
version of that patchset (v16 as of now) can be found at
https://lore.kernel.org/20250708-pxa1908-lkml-v16-0-b4392c484180@dujemihanovic.xyz

The series is RFC because of the following:
* I'm unsure whether setting pinctrl-{names,1} to true in the top level
  of the binding is correct.
* Other mainline MMC drivers select between default and UHS states based
  on the signal voltage. The PXA1908 vendor kernel does it based on the
  bus clock. I followed the vendor kernel, but do not know whether this
  is bad practice and therefore the other mainline drivers should be
  followed instead.

Signed-off-by: Duje Mihanović <duje@dujemihanovic.xyz>
---
Duje Mihanović (2):
      dt-bindings: mmc: sdhci-pxa: add state_uhs pinctrl
      mmc: sdhci-pxav3: add state_uhs pinctrl setting

 .../devicetree/bindings/mmc/sdhci-pxa.yaml         | 45 +++++++++++++++++-----
 drivers/mmc/host/sdhci-pxav3.c                     | 30 ++++++++++++++-
 include/linux/platform_data/pxa_sdhci.h            |  7 ++++
 3 files changed, 71 insertions(+), 11 deletions(-)
---
base-commit: 347e9f5043c89695b01e66b3ed111755afcf1911
change-id: 20250718-pxav3-uhs-d956bfed13f0

Best regards,
-- 
Duje Mihanović <duje@dujemihanovic.xyz>

[PATCH RFC 1/2] dt-bindings: mmc: sdhci-pxa: add state_uhs pinctrl

[Duje Mihanović]

On the pxav3 controller, increasing the drive strength of the data pins
might be required to maintain stability on fast bus clocks (above 100
MHz). Add a state_uhs pinctrl to allow this.

The existing state_cmd_gpio pinctrl is changed to apply only on pxav1 as
it's unneeded on the other controllers.

Signed-off-by: Duje Mihanović <duje@dujemihanovic.xyz>
---
 .../devicetree/bindings/mmc/sdhci-pxa.yaml         | 45 +++++++++++++++++-----
 1 file changed, 35 insertions(+), 10 deletions(-)

diff --git a/Documentation/devicetree/bindings/mmc/sdhci-pxa.yaml b/Documentation/devicetree/bindings/mmc/sdhci-pxa.yaml
index 4869ddef36fd89265a1bfe96bb9663b553ac5084..7a9e2a63ac4351aea10b2763ca250ce4889df1eb 100644
--- a/Documentation/devicetree/bindings/mmc/sdhci-pxa.yaml
+++ b/Documentation/devicetree/bindings/mmc/sdhci-pxa.yaml
@@ -30,6 +30,39 @@ allOf:
           maxItems: 1
         reg-names:
           maxItems: 1
+  - if:
+      properties:
+        compatible:
+          contains:
+            const: mrvl,pxav1-mmc
+    then:
+      properties:
+        pinctrl-names:
+          description:
+            Optional for supporting PXA168 SDIO IRQ errata to switch CMD pin between
+            SDIO CMD and GPIO mode.
+          items:
+            - const: default
+            - const: state_cmd_gpio
+        pinctrl-1:
+          description:
+            Should switch CMD pin to GPIO mode as a high output.
+  - if:
+      properties:
+        compatible:
+          contains:
+            const: mrvl,pxav3-mmc
+    then:
+      properties:
+        pinctrl-names:
+          description:
+            Optional for increasing stability of the controller at fast bus clocks.
+          items:
+            - const: default
+            - const: state_uhs
+        pinctrl-1:
+          description:
+            Should switch the drive strength of the data pins to high.
 
 properties:
   compatible:
@@ -62,21 +95,13 @@ properties:
       - const: io
       - const: core
 
-  pinctrl-names:
-    description:
-      Optional for supporting PXA168 SDIO IRQ errata to switch CMD pin between
-      SDIO CMD and GPIO mode.
-    items:
-      - const: default
-      - const: state_cmd_gpio
+  pinctrl-names: true
 
   pinctrl-0:
     description:
       Should contain default pinctrl.
 
-  pinctrl-1:
-    description:
-      Should switch CMD pin to GPIO mode as a high output.
+  pinctrl-1: true
 
   mrvl,clk-delay-cycles:
     description: Specify a number of cycles to delay for tuning.

-- 
2.50.1

Re: [PATCH RFC 1/2] dt-bindings: mmc: sdhci-pxa: add state_uhs pinctrl

[Rob Herring]

On Fri, Jul 18, 2025 at 11:12:38PM +0200, Duje Mihanović wrote:
> On the pxav3 controller, increasing the drive strength of the data pins
> might be required to maintain stability on fast bus clocks (above 100
> MHz). Add a state_uhs pinctrl to allow this.
> 
> The existing state_cmd_gpio pinctrl is changed to apply only on pxav1 as
> it's unneeded on the other controllers.
> 
> Signed-off-by: Duje Mihanović <duje@dujemihanovic.xyz>
> ---
>  .../devicetree/bindings/mmc/sdhci-pxa.yaml         | 45 +++++++++++++++++-----
>  1 file changed, 35 insertions(+), 10 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/mmc/sdhci-pxa.yaml b/Documentation/devicetree/bindings/mmc/sdhci-pxa.yaml
> index 4869ddef36fd89265a1bfe96bb9663b553ac5084..7a9e2a63ac4351aea10b2763ca250ce4889df1eb 100644
> --- a/Documentation/devicetree/bindings/mmc/sdhci-pxa.yaml
> +++ b/Documentation/devicetree/bindings/mmc/sdhci-pxa.yaml
> @@ -30,6 +30,39 @@ allOf:
>            maxItems: 1
>          reg-names:
>            maxItems: 1
> +  - if:
> +      properties:
> +        compatible:
> +          contains:
> +            const: mrvl,pxav1-mmc
> +    then:
> +      properties:
> +        pinctrl-names:
> +          description:
> +            Optional for supporting PXA168 SDIO IRQ errata to switch CMD pin between
> +            SDIO CMD and GPIO mode.
> +          items:
> +            - const: default
> +            - const: state_cmd_gpio

blank line

> +        pinctrl-1:
> +          description:
> +            Should switch CMD pin to GPIO mode as a high output.
> +  - if:
> +      properties:
> +        compatible:
> +          contains:
> +            const: mrvl,pxav3-mmc
> +    then:
> +      properties:
> +        pinctrl-names:
> +          description:
> +            Optional for increasing stability of the controller at fast bus clocks.
> +          items:
> +            - const: default
> +            - const: state_uhs

blank line

> +        pinctrl-1:
> +          description:
> +            Should switch the drive strength of the data pins to high.
>  
>  properties:
>    compatible:
> @@ -62,21 +95,13 @@ properties:
>        - const: io
>        - const: core
>  
> -  pinctrl-names:
> -    description:
> -      Optional for supporting PXA168 SDIO IRQ errata to switch CMD pin between
> -      SDIO CMD and GPIO mode.
> -    items:
> -      - const: default
> -      - const: state_cmd_gpio
> +  pinctrl-names: true
>  
>    pinctrl-0:
>      description:
>        Should contain default pinctrl.
>  
> -  pinctrl-1:
> -    description:
> -      Should switch CMD pin to GPIO mode as a high output.
> +  pinctrl-1: true
>  
>    mrvl,clk-delay-cycles:
>      description: Specify a number of cycles to delay for tuning.
> 
> -- 
> 2.50.1
> 

Re: [PATCH RFC 1/2] dt-bindings: mmc: sdhci-pxa: add state_uhs pinctrl

[Duje Mihanović]

On Monday, 21 July 2025 16:35:41 Central European Summer Time Rob Herring 
wrote:
> On Fri, Jul 18, 2025 at 11:12:38PM +0200, Duje Mihanović wrote:
> > On the pxav3 controller, increasing the drive strength of the data pins
> > might be required to maintain stability on fast bus clocks (above 100
> > MHz). Add a state_uhs pinctrl to allow this.
> > 
> > The existing state_cmd_gpio pinctrl is changed to apply only on pxav1 as
> > it's unneeded on the other controllers.
> > 
> > Signed-off-by: Duje Mihanović <duje@dujemihanovic.xyz>
> > ---
> > 
> >  .../devicetree/bindings/mmc/sdhci-pxa.yaml         | 45
> >  +++++++++++++++++----- 1 file changed, 35 insertions(+), 10 deletions(-)
> > 
> > diff --git a/Documentation/devicetree/bindings/mmc/sdhci-pxa.yaml
> > b/Documentation/devicetree/bindings/mmc/sdhci-pxa.yaml index
> > 
4869ddef36fd89265a1bfe96bb9663b553ac5084..7a9e2a63ac4351aea10b2763ca250ce48
> > 89df1eb 100644 --- a/Documentation/devicetree/bindings/mmc/sdhci-pxa.yaml
> > +++ b/Documentation/devicetree/bindings/mmc/sdhci-pxa.yaml
> > 
> > @@ -30,6 +30,39 @@ allOf:
> >            maxItems: 1
> >          
> >          reg-names:
> >            maxItems: 1
> > 
> > +  - if:
> > +      properties:
> > +        compatible:
> > +          contains:
> > +            const: mrvl,pxav1-mmc
> > +    then:
> > +      properties:
> > +        pinctrl-names:
> > +          description:
> > +            Optional for supporting PXA168 SDIO IRQ errata to switch CMD
> > pin between +            SDIO CMD and GPIO mode.
> > +          items:
> > +            - const: default
> > +            - const: state_cmd_gpio
> 
> blank line

While at it, should I do the same with the properties: blocks in the existing 
if: block?

Regards,
--
Duje

[PATCH RFC 2/2] mmc: sdhci-pxav3: add state_uhs pinctrl setting

[Duje Mihanović]

Different bus clocks require different pinctrl states to remain stable.
Add support for selecting between a default and UHS state according to
the bus clock.

Signed-off-by: Duje Mihanović <duje@dujemihanovic.xyz>
---
 drivers/mmc/host/sdhci-pxav3.c          | 30 +++++++++++++++++++++++++++++-
 include/linux/platform_data/pxa_sdhci.h |  7 +++++++
 2 files changed, 36 insertions(+), 1 deletion(-)

diff --git a/drivers/mmc/host/sdhci-pxav3.c b/drivers/mmc/host/sdhci-pxav3.c
index 3fb56face3d81259b693c8569682d05c95be2880..0e1bd75a1252e5559deb0b3caabcdc75919aee3d 100644
--- a/drivers/mmc/host/sdhci-pxav3.c
+++ b/drivers/mmc/host/sdhci-pxav3.c
@@ -20,9 +20,11 @@
 #include <linux/module.h>
 #include <linux/of.h>
 #include <linux/of_device.h>
+#include <linux/pinctrl/consumer.h>
 #include <linux/pm.h>
 #include <linux/pm_runtime.h>
 #include <linux/mbus.h>
+#include <linux/units.h>
 
 #include "sdhci.h"
 #include "sdhci-pltfm.h"
@@ -313,8 +315,24 @@ static void pxav3_set_power(struct sdhci_host *host, unsigned char mode,
 		mmc_regulator_set_ocr(mmc, mmc->supply.vmmc, vdd);
 }
 
+static void pxav3_set_clock(struct sdhci_host *host, unsigned int clock)
+{
+	struct platform_device *pdev = to_platform_device(mmc_dev(host->mmc));
+	struct sdhci_pxa_platdata *pdata = pdev->dev.platform_data;
+
+	if (clock < 100 * HZ_PER_MHZ) {
+		if (!IS_ERR(pdata->pins_default))
+			pinctrl_select_state(pdata->pinctrl, pdata->pins_default);
+	} else {
+		if (!IS_ERR(pdata->pins_uhs))
+			pinctrl_select_state(pdata->pinctrl, pdata->pins_uhs);
+	}
+
+	sdhci_set_clock(host, clock);
+}
+
 static const struct sdhci_ops pxav3_sdhci_ops = {
-	.set_clock = sdhci_set_clock,
+	.set_clock = pxav3_set_clock,
 	.set_power = pxav3_set_power,
 	.platform_send_init_74_clocks = pxav3_gen_init_74_clocks,
 	.get_max_clock = sdhci_pltfm_clk_get_max_clock,
@@ -441,6 +459,16 @@ static int sdhci_pxav3_probe(struct platform_device *pdev)
 			host->mmc->pm_caps |= pdata->pm_caps;
 	}
 
+	pdata->pinctrl = devm_pinctrl_get(dev);
+	if (IS_ERR(pdata->pinctrl))
+		dev_warn(dev, "could not get pinctrl handle\n");
+	pdata->pins_default = pinctrl_lookup_state(pdata->pinctrl, "default");
+	if (IS_ERR(pdata->pins_default))
+		dev_warn(dev, "could not get default state\n");
+	pdata->pins_uhs = pinctrl_lookup_state(pdata->pinctrl, "state_uhs");
+	if (IS_ERR(pdata->pins_uhs))
+		dev_warn(dev, "could not get uhs state\n");
+
 	pm_runtime_get_noresume(&pdev->dev);
 	pm_runtime_set_active(&pdev->dev);
 	pm_runtime_set_autosuspend_delay(&pdev->dev, PXAV3_RPM_DELAY_MS);
diff --git a/include/linux/platform_data/pxa_sdhci.h b/include/linux/platform_data/pxa_sdhci.h
index 899457cee425d33f82606f0b8c280003bc73d48d..540aa36db11243719707bdf22db23a8e2035674d 100644
--- a/include/linux/platform_data/pxa_sdhci.h
+++ b/include/linux/platform_data/pxa_sdhci.h
@@ -35,6 +35,9 @@
  * @quirks: quirks of platfrom
  * @quirks2: quirks2 of platfrom
  * @pm_caps: pm_caps of platfrom
+ * @pinctrl: pinctrl handle
+ * @pins_default: default pinctrl state
+ * @pins_uhs: pinctrl state for fast (>100 MHz) bus clocks
  */
 struct sdhci_pxa_platdata {
 	unsigned int	flags;
@@ -47,5 +50,9 @@ struct sdhci_pxa_platdata {
 	unsigned int	quirks;
 	unsigned int	quirks2;
 	unsigned int	pm_caps;
+
+	struct pinctrl	     *pinctrl;
+	struct pinctrl_state *pins_default;
+	struct pinctrl_state *pins_uhs;
 };
 #endif /* _PXA_SDHCI_H_ */

-- 
2.50.1

Re: [PATCH RFC 2/2] mmc: sdhci-pxav3: add state_uhs pinctrl setting

[Adrian Hunter]

On 19/07/2025 00:12, Duje Mihanović wrote:
> Different bus clocks require different pinctrl states to remain stable.
> Add support for selecting between a default and UHS state according to
> the bus clock.
> 
> Signed-off-by: Duje Mihanović <duje@dujemihanovic.xyz>
> ---
>  drivers/mmc/host/sdhci-pxav3.c          | 30 +++++++++++++++++++++++++++++-
>  include/linux/platform_data/pxa_sdhci.h |  7 +++++++
>  2 files changed, 36 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/mmc/host/sdhci-pxav3.c b/drivers/mmc/host/sdhci-pxav3.c
> index 3fb56face3d81259b693c8569682d05c95be2880..0e1bd75a1252e5559deb0b3caabcdc75919aee3d 100644
> --- a/drivers/mmc/host/sdhci-pxav3.c
> +++ b/drivers/mmc/host/sdhci-pxav3.c
> @@ -20,9 +20,11 @@
>  #include <linux/module.h>
>  #include <linux/of.h>
>  #include <linux/of_device.h>
> +#include <linux/pinctrl/consumer.h>
>  #include <linux/pm.h>
>  #include <linux/pm_runtime.h>
>  #include <linux/mbus.h>
> +#include <linux/units.h>
>  
>  #include "sdhci.h"
>  #include "sdhci-pltfm.h"
> @@ -313,8 +315,24 @@ static void pxav3_set_power(struct sdhci_host *host, unsigned char mode,
>  		mmc_regulator_set_ocr(mmc, mmc->supply.vmmc, vdd);
>  }
>  
> +static void pxav3_set_clock(struct sdhci_host *host, unsigned int clock)
> +{
> +	struct platform_device *pdev = to_platform_device(mmc_dev(host->mmc));
> +	struct sdhci_pxa_platdata *pdata = pdev->dev.platform_data;
> +
> +	if (clock < 100 * HZ_PER_MHZ) {
> +		if (!IS_ERR(pdata->pins_default))
> +			pinctrl_select_state(pdata->pinctrl, pdata->pins_default);
> +	} else {
> +		if (!IS_ERR(pdata->pins_uhs))
> +			pinctrl_select_state(pdata->pinctrl, pdata->pins_uhs);
> +	}
> +
> +	sdhci_set_clock(host, clock);
> +}
> +
>  static const struct sdhci_ops pxav3_sdhci_ops = {
> -	.set_clock = sdhci_set_clock,
> +	.set_clock = pxav3_set_clock,
>  	.set_power = pxav3_set_power,
>  	.platform_send_init_74_clocks = pxav3_gen_init_74_clocks,
>  	.get_max_clock = sdhci_pltfm_clk_get_max_clock,
> @@ -441,6 +459,16 @@ static int sdhci_pxav3_probe(struct platform_device *pdev)
>  			host->mmc->pm_caps |= pdata->pm_caps;
>  	}
>  
> +	pdata->pinctrl = devm_pinctrl_get(dev);
> +	if (IS_ERR(pdata->pinctrl))
> +		dev_warn(dev, "could not get pinctrl handle\n");
> +	pdata->pins_default = pinctrl_lookup_state(pdata->pinctrl, "default");

Mustn't use pdata->pinctrl if it is not valid

> +	if (IS_ERR(pdata->pins_default))
> +		dev_warn(dev, "could not get default state\n");
> +	pdata->pins_uhs = pinctrl_lookup_state(pdata->pinctrl, "state_uhs");

Mustn't use pdata->pinctrl if it is not valid, and probably don't
want to change pin state unless both pdata->pins_default and
pdata->pins_uhs are valid

> +	if (IS_ERR(pdata->pins_uhs))
> +		dev_warn(dev, "could not get uhs state\n");

I'd suggest dev_dbg() rather than dev_warn() for all the
new dev_warn()s

> +
>  	pm_runtime_get_noresume(&pdev->dev);
>  	pm_runtime_set_active(&pdev->dev);
>  	pm_runtime_set_autosuspend_delay(&pdev->dev, PXAV3_RPM_DELAY_MS);
> diff --git a/include/linux/platform_data/pxa_sdhci.h b/include/linux/platform_data/pxa_sdhci.h
> index 899457cee425d33f82606f0b8c280003bc73d48d..540aa36db11243719707bdf22db23a8e2035674d 100644
> --- a/include/linux/platform_data/pxa_sdhci.h
> +++ b/include/linux/platform_data/pxa_sdhci.h
> @@ -35,6 +35,9 @@
>   * @quirks: quirks of platfrom
>   * @quirks2: quirks2 of platfrom
>   * @pm_caps: pm_caps of platfrom
> + * @pinctrl: pinctrl handle
> + * @pins_default: default pinctrl state
> + * @pins_uhs: pinctrl state for fast (>100 MHz) bus clocks
>   */
>  struct sdhci_pxa_platdata {
>  	unsigned int	flags;
> @@ -47,5 +50,9 @@ struct sdhci_pxa_platdata {
>  	unsigned int	quirks;
>  	unsigned int	quirks2;
>  	unsigned int	pm_caps;
> +
> +	struct pinctrl	     *pinctrl;
> +	struct pinctrl_state *pins_default;
> +	struct pinctrl_state *pins_uhs;
>  };
>  #endif /* _PXA_SDHCI_H_ */
>