[PATCH 1/2] dt-bindings: mmc: mtk-sd: Add eMMC for AN7581

[PATCH 1/2] dt-bindings: mmc: mtk-sd: Add eMMC for AN7581

[Christian Marangi]

Document eMMC compatible for AN7581. This eMMC controller doesn't have
regulator exposed to the system and have a single clock only for source
clock and only default pintctrl.

Rework the schema to permit these new requirements and make supply
optional only for airoha,an7581-mmc compatible.

Also provide an example for airoha,an7581-mmc.

Signed-off-by: Christian Marangi <ansuelsmth@gmail.com>
---
This depends on patch merged in clk-next 
bfe257f9780d8f77045a7da6ec959ee0659d2f98

 .../devicetree/bindings/mmc/mtk-sd.yaml       | 64 +++++++++++++++++--
 1 file changed, 58 insertions(+), 6 deletions(-)

diff --git a/Documentation/devicetree/bindings/mmc/mtk-sd.yaml b/Documentation/devicetree/bindings/mmc/mtk-sd.yaml
index f86ebd81f5a5..6dad5455b369 100644
--- a/Documentation/devicetree/bindings/mmc/mtk-sd.yaml
+++ b/Documentation/devicetree/bindings/mmc/mtk-sd.yaml
@@ -27,6 +27,7 @@ properties:
           - mediatek,mt8183-mmc
           - mediatek,mt8196-mmc
           - mediatek,mt8516-mmc
+          - airoha,an7581-mmc
       - items:
           - const: mediatek,mt7623-mmc
           - const: mediatek,mt2701-mmc
@@ -48,11 +49,11 @@ properties:
   clocks:
     description:
       Should contain phandle for the clock feeding the MMC controller.
-    minItems: 2
+    minItems: 1
     maxItems: 7
 
   clock-names:
-    minItems: 2
+    minItems: 1
     maxItems: 7
 
   interrupts:
@@ -72,7 +73,7 @@ properties:
       Should at least contain default and state_uhs. To support SDIO in-band wakeup, dat1 pin
       will be switched between GPIO mode and SDIO DAT1 mode, state_eint is mandatory in this
       scenario.
-    minItems: 2
+    minItems: 1
     items:
       - const: default
       - const: state_uhs
@@ -170,9 +171,6 @@ required:
   - clock-names
   - pinctrl-names
   - pinctrl-0
-  - pinctrl-1
-  - vmmc-supply
-  - vqmmc-supply
 
 allOf:
   - $ref: mmc-controller.yaml#
@@ -335,6 +333,40 @@ allOf:
             - const: axi_cg
             - const: ahb_cg
 
+  - if:
+      properties:
+        compatible:
+          contains:
+            const: airoha,an7581-mmc
+    then:
+      properties:
+        clocks:
+          items:
+            - description: source clock
+
+        clock-names:
+          items:
+            - const: source
+
+        pinctrl-names:
+          items:
+            - const: default
+    else:
+      properties:
+        clocks:
+          minItems: 2
+
+        clock-names:
+          minItems: 2
+
+        pinctrl-names:
+          minItems: 2
+
+      required:
+        - pinctrl-1
+        - vmmc-supply
+        - vqmmc-supply
+
 unevaluatedProperties: false
 
 examples:
@@ -389,5 +421,25 @@ examples:
         vqmmc-supply = <&mt6397_vgp3_reg>;
         mmc-pwrseq = <&wifi_pwrseq>;
     };
+  - |
+    #include <dt-bindings/interrupt-controller/irq.h>
+    #include <dt-bindings/interrupt-controller/arm-gic.h>
+    #include <dt-bindings/clock/en7523-clk.h>
+    mmc@1fa0e000 {
+        compatible = "airoha,an7581-mmc";
+        reg = <0x1fa0e000 0x1000>,
+              <0x1fa0c000 0x60>;
+        clocks = <&scuclk EN7581_CLK_EMMC>;
+        clock-names = "source";
+        interrupts = <GIC_SPI 170 IRQ_TYPE_LEVEL_HIGH>;
+        pinctrl-names = "default";
+        pinctrl-0 = <&mmc_pins>;
+        bus-width = <4>;
+        max-frequency = <52000000>;
+        disable-wp;
+        cap-mmc-highspeed;
+        non-removable;
+
+    };
 
 ...
-- 
2.45.2

[PATCH 2/2] mmc: mtk-sd: add support for AN7581 MMC Host

[Christian Marangi]

Add support for AN7581 MMC Host. The MMC Host controller is based on
mt7622 with the difference of not having regulator supply and state_uhs
pins and hclk clock.

Some minor fixes are applied to check if the state_uhs pins are defined
and make hclk optional for the new airoha compatible.

Signed-off-by: Christian Marangi <ansuelsmth@gmail.com>
---
 drivers/mmc/host/mtk-sd.c | 55 ++++++++++++++++++++++++++++++++-------
 1 file changed, 46 insertions(+), 9 deletions(-)

diff --git a/drivers/mmc/host/mtk-sd.c b/drivers/mmc/host/mtk-sd.c
index efb0d2d5716b..9d6868883c91 100644
--- a/drivers/mmc/host/mtk-sd.c
+++ b/drivers/mmc/host/mtk-sd.c
@@ -666,6 +666,20 @@ static const struct mtk_mmc_compatible mt8196_compat = {
 	.support_new_rx = true,
 };
 
+static const struct mtk_mmc_compatible an7581_compat = {
+	.clk_div_bits = 12,
+	.recheck_sdio_irq = true,
+	.hs400_tune = false,
+	.pad_tune_reg = MSDC_PAD_TUNE0,
+	.async_fifo = true,
+	.data_tune = true,
+	.busy_check = true,
+	.stop_clk_fix = true,
+	.stop_dly_sel = 3,
+	.enhance_rx = true,
+	.support_64g = false,
+};
+
 static const struct of_device_id msdc_of_ids[] = {
 	{ .compatible = "mediatek,mt2701-mmc", .data = &mt2701_compat},
 	{ .compatible = "mediatek,mt2712-mmc", .data = &mt2712_compat},
@@ -680,7 +694,7 @@ static const struct of_device_id msdc_of_ids[] = {
 	{ .compatible = "mediatek,mt8183-mmc", .data = &mt8183_compat},
 	{ .compatible = "mediatek,mt8196-mmc", .data = &mt8196_compat},
 	{ .compatible = "mediatek,mt8516-mmc", .data = &mt8516_compat},
-
+	{ .compatible = "airoha,an7581-mmc", .data = &an7581_compat},
 	{}
 };
 MODULE_DEVICE_TABLE(of, msdc_of_ids);
@@ -1600,6 +1614,10 @@ static int msdc_ops_switch_volt(struct mmc_host *mmc, struct mmc_ios *ios)
 	struct msdc_host *host = mmc_priv(mmc);
 	int ret;
 
+	/* Skip setting supply if not supported */
+	if (!mmc->supply.vqmmc)
+		return 0;
+
 	if (!IS_ERR(mmc->supply.vqmmc)) {
 		if (ios->signal_voltage != MMC_SIGNAL_VOLTAGE_330 &&
 		    ios->signal_voltage != MMC_SIGNAL_VOLTAGE_180) {
@@ -1699,7 +1717,9 @@ static void msdc_enable_sdio_irq(struct mmc_host *mmc, int enb)
 				dev_dbg(host->dev, "SDIO eint irq: %d!\n", host->eint_irq);
 			}
 
-			pinctrl_select_state(host->pinctrl, host->pins_uhs);
+			/* Skip setting uhs pins if not supported */
+			if (host->pins_uhs)
+				pinctrl_select_state(host->pinctrl, host->pins_uhs);
 		} else {
 			dev_pm_clear_wake_irq(host->dev);
 		}
@@ -2036,6 +2056,10 @@ static void msdc_ops_set_ios(struct mmc_host *mmc, struct mmc_ios *ios)
 
 	msdc_set_buswidth(host, ios->bus_width);
 
+	/* Skip regulator if not supported */
+	if (!mmc->supply.vmmc)
+		goto skip_regulator;
+
 	/* Suspend/Resume will do power off/on */
 	switch (ios->power_mode) {
 	case MMC_POWER_UP:
@@ -2071,6 +2095,7 @@ static void msdc_ops_set_ios(struct mmc_host *mmc, struct mmc_ios *ios)
 		break;
 	}
 
+skip_regulator:
 	if (host->mclk != ios->clock || host->timing != ios->timing)
 		msdc_set_mclk(host, ios->timing, ios->clock);
 }
@@ -2816,9 +2841,12 @@ static int msdc_of_clock_parse(struct platform_device *pdev,
 	if (IS_ERR(host->src_clk))
 		return PTR_ERR(host->src_clk);
 
-	host->h_clk = devm_clk_get(&pdev->dev, "hclk");
-	if (IS_ERR(host->h_clk))
-		return PTR_ERR(host->h_clk);
+	/* AN7581 SoC doesn't have hclk */
+	if (!device_is_compatible(&pdev->dev, "airoha,an7581-mmc")) {
+		host->h_clk = devm_clk_get(&pdev->dev, "hclk");
+		if (IS_ERR(host->h_clk))
+			return PTR_ERR(host->h_clk);
+	}
 
 	host->bus_clk = devm_clk_get_optional(&pdev->dev, "bus_clk");
 	if (IS_ERR(host->bus_clk))
@@ -2926,10 +2954,13 @@ static int msdc_drv_probe(struct platform_device *pdev)
 		return PTR_ERR(host->pins_default);
 	}
 
-	host->pins_uhs = pinctrl_lookup_state(host->pinctrl, "state_uhs");
-	if (IS_ERR(host->pins_uhs)) {
-		dev_err(&pdev->dev, "Cannot find pinctrl uhs!\n");
-		return PTR_ERR(host->pins_uhs);
+	/* AN7581 doesn't have state_uhs pins */
+	if (!device_is_compatible(&pdev->dev, "airoha,an7581-mmc")) {
+		host->pins_uhs = pinctrl_lookup_state(host->pinctrl, "state_uhs");
+		if (IS_ERR(host->pins_uhs)) {
+			dev_err(&pdev->dev, "Cannot find pinctrl uhs!\n");
+			return PTR_ERR(host->pins_uhs);
+		}
 	}
 
 	/* Support for SDIO eint irq ? */
@@ -3010,6 +3041,12 @@ static int msdc_drv_probe(struct platform_device *pdev)
 		dev_err(&pdev->dev, "Cannot ungate clocks!\n");
 		goto release_clk;
 	}
+
+	/* AN7581 without regulator require tune to OCR values */
+	if (device_is_compatible(&pdev->dev, "airoha,an7581-mmc") &&
+	    !mmc->ocr_avail)
+		mmc->ocr_avail = MMC_VDD_32_33 | MMC_VDD_33_34;
+
 	msdc_init_hw(host);
 
 	if (mmc->caps2 & MMC_CAP2_CQE) {
-- 
2.45.2

Re: [PATCH 1/2] dt-bindings: mmc: mtk-sd: Add eMMC for AN7581

[Rob Herring (Arm)]

On Wed, 15 Jan 2025 08:29:50 +0100, Christian Marangi wrote:
> Document eMMC compatible for AN7581. This eMMC controller doesn't have
> regulator exposed to the system and have a single clock only for source
> clock and only default pintctrl.
> 
> Rework the schema to permit these new requirements and make supply
> optional only for airoha,an7581-mmc compatible.
> 
> Also provide an example for airoha,an7581-mmc.
> 
> Signed-off-by: Christian Marangi <ansuelsmth@gmail.com>
> ---
> This depends on patch merged in clk-next
> bfe257f9780d8f77045a7da6ec959ee0659d2f98
> 
>  .../devicetree/bindings/mmc/mtk-sd.yaml       | 64 +++++++++++++++++--
>  1 file changed, 58 insertions(+), 6 deletions(-)
> 

My bot found errors running 'make dt_binding_check' on your patch:

yamllint warnings/errors:

dtschema/dtc warnings/errors:
Error: Documentation/devicetree/bindings/mmc/mtk-sd.example.dts:104.31-32 syntax error
FATAL ERROR: Unable to parse input tree
make[2]: *** [scripts/Makefile.dtbs:131: Documentation/devicetree/bindings/mmc/mtk-sd.example.dtb] Error 1
make[2]: *** Waiting for unfinished jobs....
make[1]: *** [/builds/robherring/dt-review-ci/linux/Makefile:1506: dt_binding_check] Error 2
make: *** [Makefile:251: __sub-make] Error 2

doc reference errors (make refcheckdocs):

See https://patchwork.ozlabs.org/project/devicetree-bindings/patch/20250115073026.31552-1-ansuelsmth@gmail.com

The base for the series is generally the latest rc1. A different dependency
should be noted in *this* patch.

If you already ran 'make dt_binding_check' and didn't see the above
error(s), then make sure 'yamllint' is installed and dt-schema is up to
date:

pip3 install dtschema --upgrade

Please check and re-submit after running the above command yourself. Note
that DT_SCHEMA_FILES can be set to your schema file to speed up checking
your schema. However, it must be unset to test all examples with your schema.

Re: [PATCH 1/2] dt-bindings: mmc: mtk-sd: Add eMMC for AN7581

[Christian Marangi]

On Wed, Jan 15, 2025 at 02:37:50AM -0600, Rob Herring (Arm) wrote:
> 
> On Wed, 15 Jan 2025 08:29:50 +0100, Christian Marangi wrote:
> > Document eMMC compatible for AN7581. This eMMC controller doesn't have
> > regulator exposed to the system and have a single clock only for source
> > clock and only default pintctrl.
> > 
> > Rework the schema to permit these new requirements and make supply
> > optional only for airoha,an7581-mmc compatible.
> > 
> > Also provide an example for airoha,an7581-mmc.
> > 
> > Signed-off-by: Christian Marangi <ansuelsmth@gmail.com>
> > ---
> > This depends on patch merged in clk-next
> > bfe257f9780d8f77045a7da6ec959ee0659d2f98
> > 
> >  .../devicetree/bindings/mmc/mtk-sd.yaml       | 64 +++++++++++++++++--
> >  1 file changed, 58 insertions(+), 6 deletions(-)
> > 
> 
> My bot found errors running 'make dt_binding_check' on your patch:
> 
> yamllint warnings/errors:
> 
> dtschema/dtc warnings/errors:
> Error: Documentation/devicetree/bindings/mmc/mtk-sd.example.dts:104.31-32 syntax error
> FATAL ERROR: Unable to parse input tree
> make[2]: *** [scripts/Makefile.dtbs:131: Documentation/devicetree/bindings/mmc/mtk-sd.example.dtb] Error 1
> make[2]: *** Waiting for unfinished jobs....
> make[1]: *** [/builds/robherring/dt-review-ci/linux/Makefile:1506: dt_binding_check] Error 2
> make: *** [Makefile:251: __sub-make] Error 2
> 
> doc reference errors (make refcheckdocs):
> 
> See https://patchwork.ozlabs.org/project/devicetree-bindings/patch/20250115073026.31552-1-ansuelsmth@gmail.com
> 
> The base for the series is generally the latest rc1. A different dependency
> should be noted in *this* patch.
> 
> If you already ran 'make dt_binding_check' and didn't see the above
> error(s), then make sure 'yamllint' is installed and dt-schema is up to
> date:
> 
> pip3 install dtschema --upgrade
> 
> Please check and re-submit after running the above command yourself. Note
> that DT_SCHEMA_FILES can be set to your schema file to speed up checking
> your schema. However, it must be unset to test all examples with your schema.
>

Hi Rob, I think this fails as the CLK define is still not merged as said
this patch depends on bfe257f9780d8f77045a7da6ec959ee0659d2f98.

Any hint how to make your bot work with that?

-- 
	Ansuel

Re: [PATCH 1/2] dt-bindings: mmc: mtk-sd: Add eMMC for AN7581

[Krzysztof Kozlowski]

On Wed, Jan 15, 2025 at 09:39:34AM +0100, Christian Marangi wrote:
> > dtschema/dtc warnings/errors:
> > Error: Documentation/devicetree/bindings/mmc/mtk-sd.example.dts:104.31-32 syntax error
> > FATAL ERROR: Unable to parse input tree
> > make[2]: *** [scripts/Makefile.dtbs:131: Documentation/devicetree/bindings/mmc/mtk-sd.example.dtb] Error 1
> > make[2]: *** Waiting for unfinished jobs....
> > make[1]: *** [/builds/robherring/dt-review-ci/linux/Makefile:1506: dt_binding_check] Error 2
> > make: *** [Makefile:251: __sub-make] Error 2
> > 
> > doc reference errors (make refcheckdocs):
> > 
> > See https://patchwork.ozlabs.org/project/devicetree-bindings/patch/20250115073026.31552-1-ansuelsmth@gmail.com
> > 
> > The base for the series is generally the latest rc1. A different dependency
> > should be noted in *this* patch.
> > 
> > If you already ran 'make dt_binding_check' and didn't see the above
> > error(s), then make sure 'yamllint' is installed and dt-schema is up to
> > date:
> > 
> > pip3 install dtschema --upgrade
> > 
> > Please check and re-submit after running the above command yourself. Note
> > that DT_SCHEMA_FILES can be set to your schema file to speed up checking
> > your schema. However, it must be unset to test all examples with your schema.
> >
> 
> Hi Rob, I think this fails as the CLK define is still not merged as said
> this patch depends on bfe257f9780d8f77045a7da6ec959ee0659d2f98.
> 
> Any hint how to make your bot work with that?

Don't use the define, but raw number or just some fake phandle.

Best regards,
Krzysztof

Re: [PATCH 1/2] dt-bindings: mmc: mtk-sd: Add eMMC for AN7581

[Krzysztof Kozlowski]

On Wed, Jan 15, 2025 at 08:29:50AM +0100, Christian Marangi wrote:
> Document eMMC compatible for AN7581. This eMMC controller doesn't have
> regulator exposed to the system and have a single clock only for source
> clock and only default pintctrl.
> 
> Rework the schema to permit these new requirements and make supply
> optional only for airoha,an7581-mmc compatible.
> 
> Also provide an example for airoha,an7581-mmc.
> 
> Signed-off-by: Christian Marangi <ansuelsmth@gmail.com>
> ---
> This depends on patch merged in clk-next 
> bfe257f9780d8f77045a7da6ec959ee0659d2f98
> 
>  .../devicetree/bindings/mmc/mtk-sd.yaml       | 64 +++++++++++++++++--
>  1 file changed, 58 insertions(+), 6 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/mmc/mtk-sd.yaml b/Documentation/devicetree/bindings/mmc/mtk-sd.yaml
> index f86ebd81f5a5..6dad5455b369 100644
> --- a/Documentation/devicetree/bindings/mmc/mtk-sd.yaml
> +++ b/Documentation/devicetree/bindings/mmc/mtk-sd.yaml
> @@ -27,6 +27,7 @@ properties:
>            - mediatek,mt8183-mmc
>            - mediatek,mt8196-mmc
>            - mediatek,mt8516-mmc
> +          - airoha,an7581-mmc

Please keep things ordered. The list is nicely ordered so far.

>        - items:
>            - const: mediatek,mt7623-mmc
>            - const: mediatek,mt2701-mmc
> @@ -48,11 +49,11 @@ properties:
>    clocks:
>      description:
>        Should contain phandle for the clock feeding the MMC controller.
> -    minItems: 2
> +    minItems: 1
>      maxItems: 7
>  
>    clock-names:
> -    minItems: 2
> +    minItems: 1
>      maxItems: 7
>  
>    interrupts:
> @@ -72,7 +73,7 @@ properties:
>        Should at least contain default and state_uhs. To support SDIO in-band wakeup, dat1 pin
>        will be switched between GPIO mode and SDIO DAT1 mode, state_eint is mandatory in this
>        scenario.
> -    minItems: 2
> +    minItems: 1
>      items:
>        - const: default
>        - const: state_uhs
> @@ -170,9 +171,6 @@ required:
>    - clock-names
>    - pinctrl-names
>    - pinctrl-0
> -  - pinctrl-1
> -  - vmmc-supply
> -  - vqmmc-supply
>  
>  allOf:
>    - $ref: mmc-controller.yaml#
> @@ -335,6 +333,40 @@ allOf:
>              - const: axi_cg
>              - const: ahb_cg
>  
> +  - if:
> +      properties:
> +        compatible:
> +          contains:
> +            const: airoha,an7581-mmc
> +    then:
> +      properties:
> +        clocks:
> +          items:
> +            - description: source clock
> +
> +        clock-names:
> +          items:
> +            - const: source
> +
> +        pinctrl-names:
> +          items:
> +            - const: default
> +    else:

No else, you are now duplicating other "if:then" cases. There is already
such for "reg" so it does not really make it more readable. Just be sure
each variant has a strict, upper and lower, constraints.


> +      properties:
> +        clocks:
> +          minItems: 2
> +
> +        clock-names:
> +          minItems: 2
> +
> +        pinctrl-names:
> +          minItems: 2
> +
> +      required:
> +        - pinctrl-1
> +        - vmmc-supply
> +        - vqmmc-supply

These should go to each variant's if:then: or separate if:then: covering
all existing variants for the required fields.

> +
>  unevaluatedProperties: false
>  
>  examples:
> @@ -389,5 +421,25 @@ examples:
>          vqmmc-supply = <&mt6397_vgp3_reg>;
>          mmc-pwrseq = <&wifi_pwrseq>;
>      };
> +  - |
> +    #include <dt-bindings/interrupt-controller/irq.h>
> +    #include <dt-bindings/interrupt-controller/arm-gic.h>
> +    #include <dt-bindings/clock/en7523-clk.h>
> +    mmc@1fa0e000 {
> +        compatible = "airoha,an7581-mmc";
> +        reg = <0x1fa0e000 0x1000>,
> +              <0x1fa0c000 0x60>;
> +        clocks = <&scuclk EN7581_CLK_EMMC>;
> +        clock-names = "source";
> +        interrupts = <GIC_SPI 170 IRQ_TYPE_LEVEL_HIGH>;
> +        pinctrl-names = "default";
> +        pinctrl-0 = <&mmc_pins>;
> +        bus-width = <4>;
> +        max-frequency = <52000000>;
> +        disable-wp;
> +        cap-mmc-highspeed;
> +        non-removable;
> +

Drop blank line.


Best regards,
Krzysztof

Re: [PATCH 2/2] mmc: mtk-sd: add support for AN7581 MMC Host

[Wenbin Mei (梅文彬)]

On Wed, 2025-01-15 at 08:29 +0100, Christian Marangi wrote:
> External email : Please do not click links or open attachments until
> you have verified the sender or the content.
> 
> 
> Add support for AN7581 MMC Host. The MMC Host controller is based on
> mt7622 with the difference of not having regulator supply and
> state_uhs
> pins and hclk clock.
> 
> Some minor fixes are applied to check if the state_uhs pins are
> defined
> and make hclk optional for the new airoha compatible.
> 
> Signed-off-by: Christian Marangi <ansuelsmth@gmail.com>
> ---
>  drivers/mmc/host/mtk-sd.c | 55 ++++++++++++++++++++++++++++++++-----
> --
>  1 file changed, 46 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/mmc/host/mtk-sd.c b/drivers/mmc/host/mtk-sd.c
> index efb0d2d5716b..9d6868883c91 100644
> --- a/drivers/mmc/host/mtk-sd.c
> +++ b/drivers/mmc/host/mtk-sd.c
> @@ -666,6 +666,20 @@ static const struct mtk_mmc_compatible
> mt8196_compat = {
>         .support_new_rx = true,
>  };
> 
> +static const struct mtk_mmc_compatible an7581_compat = {
> +       .clk_div_bits = 12,
> +       .recheck_sdio_irq = true,
> +       .hs400_tune = false,
> +       .pad_tune_reg = MSDC_PAD_TUNE0,
> +       .async_fifo = true,
> +       .data_tune = true,
> +       .busy_check = true,
> +       .stop_clk_fix = true,
> +       .stop_dly_sel = 3,
> +       .enhance_rx = true,
> +       .support_64g = false,
> +};
> +
>  static const struct of_device_id msdc_of_ids[] = {
>         { .compatible = "mediatek,mt2701-mmc", .data =
> &mt2701_compat},
>         { .compatible = "mediatek,mt2712-mmc", .data =
> &mt2712_compat},
> @@ -680,7 +694,7 @@ static const struct of_device_id msdc_of_ids[] =
> {
>         { .compatible = "mediatek,mt8183-mmc", .data =
> &mt8183_compat},
>         { .compatible = "mediatek,mt8196-mmc", .data =
> &mt8196_compat},
>         { .compatible = "mediatek,mt8516-mmc", .data =
> &mt8516_compat},
> -
> +       { .compatible = "airoha,an7581-mmc", .data = &an7581_compat},
>         {}
>  };
>  MODULE_DEVICE_TABLE(of, msdc_of_ids);
> @@ -1600,6 +1614,10 @@ static int msdc_ops_switch_volt(struct
> mmc_host *mmc, struct mmc_ios *ios)
>         struct msdc_host *host = mmc_priv(mmc);
>         int ret;
> 
> +       /* Skip setting supply if not supported */
> +       if (!mmc->supply.vqmmc)
> +               return 0;
> +
>         if (!IS_ERR(mmc->supply.vqmmc)) {
>                 if (ios->signal_voltage != MMC_SIGNAL_VOLTAGE_330 &&
>                     ios->signal_voltage != MMC_SIGNAL_VOLTAGE_180) {
> @@ -1699,7 +1717,9 @@ static void msdc_enable_sdio_irq(struct
> mmc_host *mmc, int enb)
>                                 dev_dbg(host->dev, "SDIO eint irq:
> %d!\n", host->eint_irq);
>                         }
> 
> -                       pinctrl_select_state(host->pinctrl, host-
> >pins_uhs);
> +                       /* Skip setting uhs pins if not supported */
> +                       if (host->pins_uhs)
> +                               pinctrl_select_state(host->pinctrl,
> host->pins_uhs);
>                 } else {
>                         dev_pm_clear_wake_irq(host->dev);
>                 }
> @@ -2036,6 +2056,10 @@ static void msdc_ops_set_ios(struct mmc_host
> *mmc, struct mmc_ios *ios)
> 
>         msdc_set_buswidth(host, ios->bus_width);
> 
> +       /* Skip regulator if not supported */
> +       if (!mmc->supply.vmmc)
> +               goto skip_regulator;
> +
If power_mode is MMC_POWER_UP, we need to execute the related flow.
Now it will skip mmc_init_hw(host), which will cause problems.
BTW, can this be implemented using a fix regulator? if ok, no need
to modify here.
>         /* Suspend/Resume will do power off/on */
>         switch (ios->power_mode) {
>         case MMC_POWER_UP:
> @@ -2071,6 +2095,7 @@ static void msdc_ops_set_ios(struct mmc_host
> *mmc, struct mmc_ios *ios)
>                 break;
>         }
> 
> +skip_regulator:
>         if (host->mclk != ios->clock || host->timing != ios->timing)
>                 msdc_set_mclk(host, ios->timing, ios->clock);
>  }
> @@ -2816,9 +2841,12 @@ static int msdc_of_clock_parse(struct
> platform_device *pdev,
>         if (IS_ERR(host->src_clk))
>                 return PTR_ERR(host->src_clk);
> 
> -       host->h_clk = devm_clk_get(&pdev->dev, "hclk");
> -       if (IS_ERR(host->h_clk))
> -               return PTR_ERR(host->h_clk);
> +       /* AN7581 SoC doesn't have hclk */
> +       if (!device_is_compatible(&pdev->dev, "airoha,an7581-mmc")) {
> +               host->h_clk = devm_clk_get(&pdev->dev, "hclk");
> +               if (IS_ERR(host->h_clk))
> +                       return PTR_ERR(host->h_clk);
> +       }
> 
>         host->bus_clk = devm_clk_get_optional(&pdev->dev, "bus_clk");
>         if (IS_ERR(host->bus_clk))
> @@ -2926,10 +2954,13 @@ static int msdc_drv_probe(struct
> platform_device *pdev)
>                 return PTR_ERR(host->pins_default);
>         }
> 
> -       host->pins_uhs = pinctrl_lookup_state(host->pinctrl,
> "state_uhs");
> -       if (IS_ERR(host->pins_uhs)) {
> -               dev_err(&pdev->dev, "Cannot find pinctrl uhs!\n");
> -               return PTR_ERR(host->pins_uhs);
> +       /* AN7581 doesn't have state_uhs pins */
> +       if (!device_is_compatible(&pdev->dev, "airoha,an7581-mmc")) {
> +               host->pins_uhs = pinctrl_lookup_state(host->pinctrl,
> "state_uhs");
> +               if (IS_ERR(host->pins_uhs)) {
> +                       dev_err(&pdev->dev, "Cannot find pinctrl
> uhs!\n");
> +                       return PTR_ERR(host->pins_uhs);
> +               }
>         }
> 
>         /* Support for SDIO eint irq ? */
> @@ -3010,6 +3041,12 @@ static int msdc_drv_probe(struct
> platform_device *pdev)
>                 dev_err(&pdev->dev, "Cannot ungate clocks!\n");
>                 goto release_clk;
>         }
> +
> +       /* AN7581 without regulator require tune to OCR values */
> +       if (device_is_compatible(&pdev->dev, "airoha,an7581-mmc") &&
> +           !mmc->ocr_avail)
> +               mmc->ocr_avail = MMC_VDD_32_33 | MMC_VDD_33_34;
> +
>         msdc_init_hw(host);
> 
>         if (mmc->caps2 & MMC_CAP2_CQE) {
> --
> 2.45.2
> 

Re: [PATCH 2/2] mmc: mtk-sd: add support for AN7581 MMC Host

[Christian Marangi]

On Wed, Jan 15, 2025 at 09:33:35AM +0000, Wenbin Mei (梅文彬) wrote:
> On Wed, 2025-01-15 at 08:29 +0100, Christian Marangi wrote:
> > External email : Please do not click links or open attachments until
> > you have verified the sender or the content.
> > 
> > 
> > Add support for AN7581 MMC Host. The MMC Host controller is based on
> > mt7622 with the difference of not having regulator supply and
> > state_uhs
> > pins and hclk clock.
> > 
> > Some minor fixes are applied to check if the state_uhs pins are
> > defined
> > and make hclk optional for the new airoha compatible.
> > 
> > Signed-off-by: Christian Marangi <ansuelsmth@gmail.com>
> > ---
> >  drivers/mmc/host/mtk-sd.c | 55 ++++++++++++++++++++++++++++++++-----
> > --
> >  1 file changed, 46 insertions(+), 9 deletions(-)
> > 
> > diff --git a/drivers/mmc/host/mtk-sd.c b/drivers/mmc/host/mtk-sd.c
> > index efb0d2d5716b..9d6868883c91 100644
> > --- a/drivers/mmc/host/mtk-sd.c
> > +++ b/drivers/mmc/host/mtk-sd.c
> > @@ -666,6 +666,20 @@ static const struct mtk_mmc_compatible
> > mt8196_compat = {
> >         .support_new_rx = true,
> >  };
> > 
> > +static const struct mtk_mmc_compatible an7581_compat = {
> > +       .clk_div_bits = 12,
> > +       .recheck_sdio_irq = true,
> > +       .hs400_tune = false,
> > +       .pad_tune_reg = MSDC_PAD_TUNE0,
> > +       .async_fifo = true,
> > +       .data_tune = true,
> > +       .busy_check = true,
> > +       .stop_clk_fix = true,
> > +       .stop_dly_sel = 3,
> > +       .enhance_rx = true,
> > +       .support_64g = false,
> > +};
> > +
> >  static const struct of_device_id msdc_of_ids[] = {
> >         { .compatible = "mediatek,mt2701-mmc", .data =
> > &mt2701_compat},
> >         { .compatible = "mediatek,mt2712-mmc", .data =
> > &mt2712_compat},
> > @@ -680,7 +694,7 @@ static const struct of_device_id msdc_of_ids[] =
> > {
> >         { .compatible = "mediatek,mt8183-mmc", .data =
> > &mt8183_compat},
> >         { .compatible = "mediatek,mt8196-mmc", .data =
> > &mt8196_compat},
> >         { .compatible = "mediatek,mt8516-mmc", .data =
> > &mt8516_compat},
> > -
> > +       { .compatible = "airoha,an7581-mmc", .data = &an7581_compat},
> >         {}
> >  };
> >  MODULE_DEVICE_TABLE(of, msdc_of_ids);
> > @@ -1600,6 +1614,10 @@ static int msdc_ops_switch_volt(struct
> > mmc_host *mmc, struct mmc_ios *ios)
> >         struct msdc_host *host = mmc_priv(mmc);
> >         int ret;
> > 
> > +       /* Skip setting supply if not supported */
> > +       if (!mmc->supply.vqmmc)
> > +               return 0;
> > +
> >         if (!IS_ERR(mmc->supply.vqmmc)) {
> >                 if (ios->signal_voltage != MMC_SIGNAL_VOLTAGE_330 &&
> >                     ios->signal_voltage != MMC_SIGNAL_VOLTAGE_180) {
> > @@ -1699,7 +1717,9 @@ static void msdc_enable_sdio_irq(struct
> > mmc_host *mmc, int enb)
> >                                 dev_dbg(host->dev, "SDIO eint irq:
> > %d!\n", host->eint_irq);
> >                         }
> > 
> > -                       pinctrl_select_state(host->pinctrl, host-
> > >pins_uhs);
> > +                       /* Skip setting uhs pins if not supported */
> > +                       if (host->pins_uhs)
> > +                               pinctrl_select_state(host->pinctrl,
> > host->pins_uhs);
> >                 } else {
> >                         dev_pm_clear_wake_irq(host->dev);
> >                 }
> > @@ -2036,6 +2056,10 @@ static void msdc_ops_set_ios(struct mmc_host
> > *mmc, struct mmc_ios *ios)
> > 
> >         msdc_set_buswidth(host, ios->bus_width);
> > 
> > +       /* Skip regulator if not supported */
> > +       if (!mmc->supply.vmmc)
> > +               goto skip_regulator;
> > +
> If power_mode is MMC_POWER_UP, we need to execute the related flow.
> Now it will skip mmc_init_hw(host), which will cause problems.
> BTW, can this be implemented using a fix regulator? if ok, no need
> to modify here.

Yes I didn't think of that, I will test if this is possible and reply
back, thanks for the hint.

> >         /* Suspend/Resume will do power off/on */
> >         switch (ios->power_mode) {
> >         case MMC_POWER_UP:
> > @@ -2071,6 +2095,7 @@ static void msdc_ops_set_ios(struct mmc_host
> > *mmc, struct mmc_ios *ios)
> >                 break;
> >         }
> > 
> > +skip_regulator:
> >         if (host->mclk != ios->clock || host->timing != ios->timing)
> >                 msdc_set_mclk(host, ios->timing, ios->clock);
> >  }
> > @@ -2816,9 +2841,12 @@ static int msdc_of_clock_parse(struct
> > platform_device *pdev,
> >         if (IS_ERR(host->src_clk))
> >                 return PTR_ERR(host->src_clk);
> > 
> > -       host->h_clk = devm_clk_get(&pdev->dev, "hclk");
> > -       if (IS_ERR(host->h_clk))
> > -               return PTR_ERR(host->h_clk);
> > +       /* AN7581 SoC doesn't have hclk */
> > +       if (!device_is_compatible(&pdev->dev, "airoha,an7581-mmc")) {
> > +               host->h_clk = devm_clk_get(&pdev->dev, "hclk");
> > +               if (IS_ERR(host->h_clk))
> > +                       return PTR_ERR(host->h_clk);
> > +       }
> > 
> >         host->bus_clk = devm_clk_get_optional(&pdev->dev, "bus_clk");
> >         if (IS_ERR(host->bus_clk))
> > @@ -2926,10 +2954,13 @@ static int msdc_drv_probe(struct
> > platform_device *pdev)
> >                 return PTR_ERR(host->pins_default);
> >         }
> > 
> > -       host->pins_uhs = pinctrl_lookup_state(host->pinctrl,
> > "state_uhs");
> > -       if (IS_ERR(host->pins_uhs)) {
> > -               dev_err(&pdev->dev, "Cannot find pinctrl uhs!\n");
> > -               return PTR_ERR(host->pins_uhs);
> > +       /* AN7581 doesn't have state_uhs pins */
> > +       if (!device_is_compatible(&pdev->dev, "airoha,an7581-mmc")) {
> > +               host->pins_uhs = pinctrl_lookup_state(host->pinctrl,
> > "state_uhs");
> > +               if (IS_ERR(host->pins_uhs)) {
> > +                       dev_err(&pdev->dev, "Cannot find pinctrl
> > uhs!\n");
> > +                       return PTR_ERR(host->pins_uhs);
> > +               }
> >         }
> > 
> >         /* Support for SDIO eint irq ? */
> > @@ -3010,6 +3041,12 @@ static int msdc_drv_probe(struct
> > platform_device *pdev)
> >                 dev_err(&pdev->dev, "Cannot ungate clocks!\n");
> >                 goto release_clk;
> >         }
> > +
> > +       /* AN7581 without regulator require tune to OCR values */
> > +       if (device_is_compatible(&pdev->dev, "airoha,an7581-mmc") &&
> > +           !mmc->ocr_avail)
> > +               mmc->ocr_avail = MMC_VDD_32_33 | MMC_VDD_33_34;
> > +
> >         msdc_init_hw(host);
> > 
> >         if (mmc->caps2 & MMC_CAP2_CQE) {
> > --
> > 2.45.2
> > 

-- 
	Ansuel

Re: [PATCH 2/2] mmc: mtk-sd: add support for AN7581 MMC Host

[Andy-ld Lu (卢东)]

On Wed, 2025-01-15 at 08:29 +0100, Christian Marangi wrote:
> Add support for AN7581 MMC Host. The MMC Host controller is based on
> mt7622 with the difference of not having regulator supply and
> state_uhs
> pins and hclk clock.
> 
> Some minor fixes are applied to check if the state_uhs pins are
> defined
> and make hclk optional for the new airoha compatible.
> 
> Signed-off-by: Christian Marangi <ansuelsmth@gmail.com>
> ---
>  drivers/mmc/host/mtk-sd.c | 55 ++++++++++++++++++++++++++++++++-----
> --
>  1 file changed, 46 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/mmc/host/mtk-sd.c b/drivers/mmc/host/mtk-sd.c
> index efb0d2d5716b..9d6868883c91 100644
> --- a/drivers/mmc/host/mtk-sd.c
> +++ b/drivers/mmc/host/mtk-sd.c
> @@ -666,6 +666,20 @@ static const struct mtk_mmc_compatible
> mt8196_compat = {
>  	.support_new_rx = true,
>  };
>  
> +static const struct mtk_mmc_compatible an7581_compat = {
> +	.clk_div_bits = 12,
> +	.recheck_sdio_irq = true,
> +	.hs400_tune = false,
> +	.pad_tune_reg = MSDC_PAD_TUNE0,
> +	.async_fifo = true,
> +	.data_tune = true,
> +	.busy_check = true,
> +	.stop_clk_fix = true,
> +	.stop_dly_sel = 3,
> +	.enhance_rx = true,
> +	.support_64g = false,
> +};
> +
>  static const struct of_device_id msdc_of_ids[] = {
>  	{ .compatible = "mediatek,mt2701-mmc", .data = &mt2701_compat},
>  	{ .compatible = "mediatek,mt2712-mmc", .data = &mt2712_compat},
> @@ -680,7 +694,7 @@ static const struct of_device_id msdc_of_ids[] =
> {
>  	{ .compatible = "mediatek,mt8183-mmc", .data = &mt8183_compat},
>  	{ .compatible = "mediatek,mt8196-mmc", .data = &mt8196_compat},
>  	{ .compatible = "mediatek,mt8516-mmc", .data = &mt8516_compat},
> -
> +	{ .compatible = "airoha,an7581-mmc", .data = &an7581_compat},
>  	{}
>  };
>  MODULE_DEVICE_TABLE(of, msdc_of_ids);
> @@ -1600,6 +1614,10 @@ static int msdc_ops_switch_volt(struct
> mmc_host *mmc, struct mmc_ios *ios)
>  	struct msdc_host *host = mmc_priv(mmc);
>  	int ret;
>  
> +	/* Skip setting supply if not supported */
> +	if (!mmc->supply.vqmmc)
> +		return 0;
> +
Hi Christian,

I think here is no need. If you have not 'vqmmc' in the
dts, IS_ERR(mmc->supply.vqmmc) would be -ENODEV and the corresponding
flow would not be executed.

And another question, host->pins_default is just selected here, that
would be lost.
 
>  	if (!IS_ERR(mmc->supply.vqmmc)) {
>  		if (ios->signal_voltage != MMC_SIGNAL_VOLTAGE_330 &&
>  		    ios->signal_voltage != MMC_SIGNAL_VOLTAGE_180) {
> @@ -1699,7 +1717,9 @@ static void msdc_enable_sdio_irq(struct
> mmc_host *mmc, int enb)
>  				dev_dbg(host->dev, "SDIO eint irq:
> %d!\n", host->eint_irq);
>  			}
>  
> -			pinctrl_select_state(host->pinctrl, host-
> >pins_uhs);
> +			/* Skip setting uhs pins if not supported */
> +			if (host->pins_uhs)
> +				pinctrl_select_state(host->pinctrl,
> host->pins_uhs);
>  		} else {
>  			dev_pm_clear_wake_irq(host->dev);
>  		}
> @@ -2036,6 +2056,10 @@ static void msdc_ops_set_ios(struct mmc_host
> *mmc, struct mmc_ios *ios)
>  
>  	msdc_set_buswidth(host, ios->bus_width);
>  
> +	/* Skip regulator if not supported */
> +	if (!mmc->supply.vmmc)
> +		goto skip_regulator;
> +
No need too.

>  	/* Suspend/Resume will do power off/on */
>  	switch (ios->power_mode) {
>  	case MMC_POWER_UP:
> @@ -2071,6 +2095,7 @@ static void msdc_ops_set_ios(struct mmc_host
> *mmc, struct mmc_ios *ios)
>  		break;
>  	}
>  
> +skip_regulator:
>  	if (host->mclk != ios->clock || host->timing != ios->timing)
>  		msdc_set_mclk(host, ios->timing, ios->clock);
>  }
> @@ -2816,9 +2841,12 @@ static int msdc_of_clock_parse(struct
> platform_device *pdev,
>  	if (IS_ERR(host->src_clk))
>  		return PTR_ERR(host->src_clk);
>  
> -	host->h_clk = devm_clk_get(&pdev->dev, "hclk");
> -	if (IS_ERR(host->h_clk))
> -		return PTR_ERR(host->h_clk);
> +	/* AN7581 SoC doesn't have hclk */
> +	if (!device_is_compatible(&pdev->dev, "airoha,an7581-mmc")) {
> +		host->h_clk = devm_clk_get(&pdev->dev, "hclk");
> +		if (IS_ERR(host->h_clk))
> +			return PTR_ERR(host->h_clk);
> +	}
devm_clk_get_optional could be used to instead here, no need to use
compatible to distinguish.

>  	host->bus_clk = devm_clk_get_optional(&pdev->dev, "bus_clk");
>  	if (IS_ERR(host->bus_clk))
> @@ -2926,10 +2954,13 @@ static int msdc_drv_probe(struct
> platform_device *pdev)
>  		return PTR_ERR(host->pins_default);
>  	}
>  
> -	host->pins_uhs = pinctrl_lookup_state(host->pinctrl,
> "state_uhs");
> -	if (IS_ERR(host->pins_uhs)) {
> -		dev_err(&pdev->dev, "Cannot find pinctrl uhs!\n");
> -		return PTR_ERR(host->pins_uhs);
> +	/* AN7581 doesn't have state_uhs pins */
> +	if (!device_is_compatible(&pdev->dev, "airoha,an7581-mmc")) {
> +		host->pins_uhs = pinctrl_lookup_state(host->pinctrl,
> "state_uhs");
> +		if (IS_ERR(host->pins_uhs)) {
> +			dev_err(&pdev->dev, "Cannot find pinctrl
> uhs!\n");
> +			return PTR_ERR(host->pins_uhs);
> +		}
>  	}
Could you consider to set a dummy 'state_uhs' same as 'state_default'
in the dts, that you could not use compatible to distinguish here.

>  
>  	/* Support for SDIO eint irq ? */
> @@ -3010,6 +3041,12 @@ static int msdc_drv_probe(struct
> platform_device *pdev)
>  		dev_err(&pdev->dev, "Cannot ungate clocks!\n");
>  		goto release_clk;
>  	}
> +
> +	/* AN7581 without regulator require tune to OCR values */
> +	if (device_is_compatible(&pdev->dev, "airoha,an7581-mmc") &&
> +	    !mmc->ocr_avail)
> +		mmc->ocr_avail = MMC_VDD_32_33 | MMC_VDD_33_34;
> +
Maybe you could use regulator-fixed in the dts and configure min/max
voltage to get ocr_avail, no need to set hard code here. 

>  	msdc_init_hw(host);
>  
>  	if (mmc->caps2 & MMC_CAP2_CQE) {

Re: [PATCH 2/2] mmc: mtk-sd: add support for AN7581 MMC Host

[Christian Marangi]

On Thu, Jan 16, 2025 at 07:01:13AM +0000, Andy-ld Lu (卢东) wrote:
> On Wed, 2025-01-15 at 08:29 +0100, Christian Marangi wrote:
> > Add support for AN7581 MMC Host. The MMC Host controller is based on
> > mt7622 with the difference of not having regulator supply and
> > state_uhs
> > pins and hclk clock.
> > 
> > Some minor fixes are applied to check if the state_uhs pins are
> > defined
> > and make hclk optional for the new airoha compatible.
> > 
> > Signed-off-by: Christian Marangi <ansuelsmth@gmail.com>
> > ---
> >  drivers/mmc/host/mtk-sd.c | 55 ++++++++++++++++++++++++++++++++-----
> > --
> >  1 file changed, 46 insertions(+), 9 deletions(-)
> > 
> > diff --git a/drivers/mmc/host/mtk-sd.c b/drivers/mmc/host/mtk-sd.c
> > index efb0d2d5716b..9d6868883c91 100644
> > --- a/drivers/mmc/host/mtk-sd.c
> > +++ b/drivers/mmc/host/mtk-sd.c
> > @@ -666,6 +666,20 @@ static const struct mtk_mmc_compatible
> > mt8196_compat = {
> >  	.support_new_rx = true,
> >  };
> >  
> > +static const struct mtk_mmc_compatible an7581_compat = {
> > +	.clk_div_bits = 12,
> > +	.recheck_sdio_irq = true,
> > +	.hs400_tune = false,
> > +	.pad_tune_reg = MSDC_PAD_TUNE0,
> > +	.async_fifo = true,
> > +	.data_tune = true,
> > +	.busy_check = true,
> > +	.stop_clk_fix = true,
> > +	.stop_dly_sel = 3,
> > +	.enhance_rx = true,
> > +	.support_64g = false,
> > +};
> > +
> >  static const struct of_device_id msdc_of_ids[] = {
> >  	{ .compatible = "mediatek,mt2701-mmc", .data = &mt2701_compat},
> >  	{ .compatible = "mediatek,mt2712-mmc", .data = &mt2712_compat},
> > @@ -680,7 +694,7 @@ static const struct of_device_id msdc_of_ids[] =
> > {
> >  	{ .compatible = "mediatek,mt8183-mmc", .data = &mt8183_compat},
> >  	{ .compatible = "mediatek,mt8196-mmc", .data = &mt8196_compat},
> >  	{ .compatible = "mediatek,mt8516-mmc", .data = &mt8516_compat},
> > -
> > +	{ .compatible = "airoha,an7581-mmc", .data = &an7581_compat},
> >  	{}
> >  };
> >  MODULE_DEVICE_TABLE(of, msdc_of_ids);
> > @@ -1600,6 +1614,10 @@ static int msdc_ops_switch_volt(struct
> > mmc_host *mmc, struct mmc_ios *ios)
> >  	struct msdc_host *host = mmc_priv(mmc);
> >  	int ret;
> >  
> > +	/* Skip setting supply if not supported */
> > +	if (!mmc->supply.vqmmc)
> > +		return 0;
> > +
> Hi Christian,
> 
> I think here is no need. If you have not 'vqmmc' in the
> dts, IS_ERR(mmc->supply.vqmmc) would be -ENODEV and the corresponding
> flow would not be executed.
> 
> And another question, host->pins_default is just selected here, that
> would be lost.
>  
> >  	if (!IS_ERR(mmc->supply.vqmmc)) {
> >  		if (ios->signal_voltage != MMC_SIGNAL_VOLTAGE_330 &&
> >  		    ios->signal_voltage != MMC_SIGNAL_VOLTAGE_180) {
> > @@ -1699,7 +1717,9 @@ static void msdc_enable_sdio_irq(struct
> > mmc_host *mmc, int enb)
> >  				dev_dbg(host->dev, "SDIO eint irq:
> > %d!\n", host->eint_irq);
> >  			}
> >  
> > -			pinctrl_select_state(host->pinctrl, host-
> > >pins_uhs);
> > +			/* Skip setting uhs pins if not supported */
> > +			if (host->pins_uhs)
> > +				pinctrl_select_state(host->pinctrl,
> > host->pins_uhs);
> >  		} else {
> >  			dev_pm_clear_wake_irq(host->dev);
> >  		}
> > @@ -2036,6 +2056,10 @@ static void msdc_ops_set_ios(struct mmc_host
> > *mmc, struct mmc_ios *ios)
> >  
> >  	msdc_set_buswidth(host, ios->bus_width);
> >  
> > +	/* Skip regulator if not supported */
> > +	if (!mmc->supply.vmmc)
> > +		goto skip_regulator;
> > +
> No need too.
> 
> >  	/* Suspend/Resume will do power off/on */
> >  	switch (ios->power_mode) {
> >  	case MMC_POWER_UP:
> > @@ -2071,6 +2095,7 @@ static void msdc_ops_set_ios(struct mmc_host
> > *mmc, struct mmc_ios *ios)
> >  		break;
> >  	}
> >  
> > +skip_regulator:
> >  	if (host->mclk != ios->clock || host->timing != ios->timing)
> >  		msdc_set_mclk(host, ios->timing, ios->clock);
> >  }
> > @@ -2816,9 +2841,12 @@ static int msdc_of_clock_parse(struct
> > platform_device *pdev,
> >  	if (IS_ERR(host->src_clk))
> >  		return PTR_ERR(host->src_clk);
> >  
> > -	host->h_clk = devm_clk_get(&pdev->dev, "hclk");
> > -	if (IS_ERR(host->h_clk))
> > -		return PTR_ERR(host->h_clk);
> > +	/* AN7581 SoC doesn't have hclk */
> > +	if (!device_is_compatible(&pdev->dev, "airoha,an7581-mmc")) {
> > +		host->h_clk = devm_clk_get(&pdev->dev, "hclk");
> > +		if (IS_ERR(host->h_clk))
> > +			return PTR_ERR(host->h_clk);
> > +	}
> devm_clk_get_optional could be used to instead here, no need to use
> compatible to distinguish.
>

I can make the hclk optional but I think this would affect also every
other compatible by hiding broken clock configuration.

> >  	host->bus_clk = devm_clk_get_optional(&pdev->dev, "bus_clk");
> >  	if (IS_ERR(host->bus_clk))
> > @@ -2926,10 +2954,13 @@ static int msdc_drv_probe(struct
> > platform_device *pdev)
> >  		return PTR_ERR(host->pins_default);
> >  	}
> >  
> > -	host->pins_uhs = pinctrl_lookup_state(host->pinctrl,
> > "state_uhs");
> > -	if (IS_ERR(host->pins_uhs)) {
> > -		dev_err(&pdev->dev, "Cannot find pinctrl uhs!\n");
> > -		return PTR_ERR(host->pins_uhs);
> > +	/* AN7581 doesn't have state_uhs pins */
> > +	if (!device_is_compatible(&pdev->dev, "airoha,an7581-mmc")) {
> > +		host->pins_uhs = pinctrl_lookup_state(host->pinctrl,
> > "state_uhs");
> > +		if (IS_ERR(host->pins_uhs)) {
> > +			dev_err(&pdev->dev, "Cannot find pinctrl
> > uhs!\n");
> > +			return PTR_ERR(host->pins_uhs);
> > +		}
> >  	}
> Could you consider to set a dummy 'state_uhs' same as 'state_default'
> in the dts, that you could not use compatible to distinguish here.
> 

This is problematic, correct me if I'm wrong, you are suggesting to
assign the emmc pins to both default and uhs? This is problematic as the
pinctrl driver would complain that such pins are already assigned to
something. Also I don't think it's possible to assign these pins to a
dummy pin.

> >  
> >  	/* Support for SDIO eint irq ? */
> > @@ -3010,6 +3041,12 @@ static int msdc_drv_probe(struct
> > platform_device *pdev)
> >  		dev_err(&pdev->dev, "Cannot ungate clocks!\n");
> >  		goto release_clk;
> >  	}
> > +
> > +	/* AN7581 without regulator require tune to OCR values */
> > +	if (device_is_compatible(&pdev->dev, "airoha,an7581-mmc") &&
> > +	    !mmc->ocr_avail)
> > +		mmc->ocr_avail = MMC_VDD_32_33 | MMC_VDD_33_34;
> > +
> Maybe you could use regulator-fixed in the dts and configure min/max
> voltage to get ocr_avail, no need to set hard code here. 
> 

Also suggested by Wenbin Mei (梅文彬) and I just tried this.

This can't be done, fixed-regulator needs to have the same min and max
voltage or they fail to probe sooo fixed-regulator saddly can't be used
:(

I will send a new version of this with the other point corrected but I
think a compatible and these additional if is a must :(

> >  	msdc_init_hw(host);
> >  
> >  	if (mmc->caps2 & MMC_CAP2_CQE) {

-- 
	Ansuel

Re: [PATCH 2/2] mmc: mtk-sd: add support for AN7581 MMC Host

[Andy-ld Lu (卢东)]

On Mon, 2025-01-20 at 18:29 +0100, Christian Marangi wrote:
> External email : Please do not click links or open attachments until
> you have verified the sender or the content.
> 
> 
> On Thu, Jan 16, 2025 at 07:01:13AM +0000, Andy-ld Lu (卢东) wrote:
> > On Wed, 2025-01-15 at 08:29 +0100, Christian Marangi wrote:
> > > Add support for AN7581 MMC Host. The MMC Host controller is based
> > > on
> > > mt7622 with the difference of not having regulator supply and
> > > state_uhs
> > > pins and hclk clock.
> > > 
> > > Some minor fixes are applied to check if the state_uhs pins are
> > > defined
> > > and make hclk optional for the new airoha compatible.
> > > 
> > > Signed-off-by: Christian Marangi <ansuelsmth@gmail.com>
> > > ---
> > >  drivers/mmc/host/mtk-sd.c | 55 ++++++++++++++++++++++++++++++++-
> > > ----
> > > --
> > >  1 file changed, 46 insertions(+), 9 deletions(-)
> > > 
> > > diff --git a/drivers/mmc/host/mtk-sd.c b/drivers/mmc/host/mtk-
> > > sd.c
> > > index efb0d2d5716b..9d6868883c91 100644
> > > --- a/drivers/mmc/host/mtk-sd.c
> > > +++ b/drivers/mmc/host/mtk-sd.c
> > > @@ -666,6 +666,20 @@ static const struct mtk_mmc_compatible
> > > mt8196_compat = {
> > >     .support_new_rx = true,
> > >  };
> > > 
> > > +static const struct mtk_mmc_compatible an7581_compat = {
> > > +   .clk_div_bits = 12,
> > > +   .recheck_sdio_irq = true,
> > > +   .hs400_tune = false,
> > > +   .pad_tune_reg = MSDC_PAD_TUNE0,
> > > +   .async_fifo = true,
> > > +   .data_tune = true,
> > > +   .busy_check = true,
> > > +   .stop_clk_fix = true,
> > > +   .stop_dly_sel = 3,
> > > +   .enhance_rx = true,
> > > +   .support_64g = false,
> > > +};
> > > +
> > >  static const struct of_device_id msdc_of_ids[] = {
> > >     { .compatible = "mediatek,mt2701-mmc", .data =
> > > &mt2701_compat},
> > >     { .compatible = "mediatek,mt2712-mmc", .data =
> > > &mt2712_compat},
> > > @@ -680,7 +694,7 @@ static const struct of_device_id
> > > msdc_of_ids[] =
> > > {
> > >     { .compatible = "mediatek,mt8183-mmc", .data =
> > > &mt8183_compat},
> > >     { .compatible = "mediatek,mt8196-mmc", .data =
> > > &mt8196_compat},
> > >     { .compatible = "mediatek,mt8516-mmc", .data =
> > > &mt8516_compat},
> > > -
> > > +   { .compatible = "airoha,an7581-mmc", .data = &an7581_compat},
> > >     {}
> > >  };
> > >  MODULE_DEVICE_TABLE(of, msdc_of_ids);
> > > @@ -1600,6 +1614,10 @@ static int msdc_ops_switch_volt(struct
> > > mmc_host *mmc, struct mmc_ios *ios)
> > >     struct msdc_host *host = mmc_priv(mmc);
> > >     int ret;
> > > 
> > > +   /* Skip setting supply if not supported */
> > > +   if (!mmc->supply.vqmmc)
> > > +           return 0;
> > > +
> > 
> > Hi Christian,
> > 
> > I think here is no need. If you have not 'vqmmc' in the
> > dts, IS_ERR(mmc->supply.vqmmc) would be -ENODEV and the
> > corresponding
> > flow would not be executed.
> > 
> > And another question, host->pins_default is just selected here,
> > that
> > would be lost.
> > 
> > >     if (!IS_ERR(mmc->supply.vqmmc)) {
> > >             if (ios->signal_voltage != MMC_SIGNAL_VOLTAGE_330 &&
> > >                 ios->signal_voltage != MMC_SIGNAL_VOLTAGE_180) {
> > > @@ -1699,7 +1717,9 @@ static void msdc_enable_sdio_irq(struct
> > > mmc_host *mmc, int enb)
> > >                             dev_dbg(host->dev, "SDIO eint irq:
> > > %d!\n", host->eint_irq);
> > >                     }
> > > 
> > > -                   pinctrl_select_state(host->pinctrl, host-
> > > > pins_uhs);
> > > 
> > > +                   /* Skip setting uhs pins if not supported */
> > > +                   if (host->pins_uhs)
> > > +                           pinctrl_select_state(host->pinctrl,
> > > host->pins_uhs);
> > >             } else {
> > >                     dev_pm_clear_wake_irq(host->dev);
> > >             }
> > > @@ -2036,6 +2056,10 @@ static void msdc_ops_set_ios(struct
> > > mmc_host
> > > *mmc, struct mmc_ios *ios)
> > > 
> > >     msdc_set_buswidth(host, ios->bus_width);
> > > 
> > > +   /* Skip regulator if not supported */
> > > +   if (!mmc->supply.vmmc)
> > > +           goto skip_regulator;
> > > +
> > 
> > No need too.
> > 
> > >     /* Suspend/Resume will do power off/on */
> > >     switch (ios->power_mode) {
> > >     case MMC_POWER_UP:
> > > @@ -2071,6 +2095,7 @@ static void msdc_ops_set_ios(struct
> > > mmc_host
> > > *mmc, struct mmc_ios *ios)
> > >             break;
> > >     }
> > > 
> > > +skip_regulator:
> > >     if (host->mclk != ios->clock || host->timing != ios->timing)
> > >             msdc_set_mclk(host, ios->timing, ios->clock);
> > >  }
> > > @@ -2816,9 +2841,12 @@ static int msdc_of_clock_parse(struct
> > > platform_device *pdev,
> > >     if (IS_ERR(host->src_clk))
> > >             return PTR_ERR(host->src_clk);
> > > 
> > > -   host->h_clk = devm_clk_get(&pdev->dev, "hclk");
> > > -   if (IS_ERR(host->h_clk))
> > > -           return PTR_ERR(host->h_clk);
> > > +   /* AN7581 SoC doesn't have hclk */
> > > +   if (!device_is_compatible(&pdev->dev, "airoha,an7581-mmc")) {
> > > +           host->h_clk = devm_clk_get(&pdev->dev, "hclk");
> > > +           if (IS_ERR(host->h_clk))
> > > +                   return PTR_ERR(host->h_clk);
> > > +   }
> > 
> > devm_clk_get_optional could be used to instead here, no need to use
> > compatible to distinguish.
> > 
> 
> I can make the hclk optional but I think this would affect also every
> other compatible by hiding broken clock configuration.
> 
> > >     host->bus_clk = devm_clk_get_optional(&pdev->dev, "bus_clk");
> > >     if (IS_ERR(host->bus_clk))
> > > @@ -2926,10 +2954,13 @@ static int msdc_drv_probe(struct
> > > platform_device *pdev)
> > >             return PTR_ERR(host->pins_default);
> > >     }
> > > 
> > > -   host->pins_uhs = pinctrl_lookup_state(host->pinctrl,
> > > "state_uhs");
> > > -   if (IS_ERR(host->pins_uhs)) {
> > > -           dev_err(&pdev->dev, "Cannot find pinctrl uhs!\n");
> > > -           return PTR_ERR(host->pins_uhs);
> > > +   /* AN7581 doesn't have state_uhs pins */
> > > +   if (!device_is_compatible(&pdev->dev, "airoha,an7581-mmc")) {
> > > +           host->pins_uhs = pinctrl_lookup_state(host->pinctrl,
> > > "state_uhs");
> > > +           if (IS_ERR(host->pins_uhs)) {
> > > +                   dev_err(&pdev->dev, "Cannot find pinctrl
> > > uhs!\n");
> > > +                   return PTR_ERR(host->pins_uhs);
> > > +           }
> > >     }
> > 
> > Could you consider to set a dummy 'state_uhs' same as
> > 'state_default'
> > in the dts, that you could not use compatible to distinguish here.
> > 
> 
> This is problematic, correct me if I'm wrong, you are suggesting to
> assign the emmc pins to both default and uhs? This is problematic as
> the
> pinctrl driver would complain that such pins are already assigned to
> something. Also I don't think it's possible to assign these pins to a
> dummy pin.
> 
Maybe I have not expressed clearly...What I mean is that you could set
as below, and the content of &mmc_pins_uhs is just copied from
&mmc_pins_default.

mmc@1fa0e000 {
	...
	pinctrl-names = "default", "state_uhs";
	pinctrl-0 = <&mmc_pins_default>;
	pinctrl-1 = <&mmc_pins_uhs>;
}
> > > 
> > >     /* Support for SDIO eint irq ? */
> > > @@ -3010,6 +3041,12 @@ static int msdc_drv_probe(struct
> > > platform_device *pdev)
> > >             dev_err(&pdev->dev, "Cannot ungate clocks!\n");
> > >             goto release_clk;
> > >     }
> > > +
> > > +   /* AN7581 without regulator require tune to OCR values */
> > > +   if (device_is_compatible(&pdev->dev, "airoha,an7581-mmc") &&
> > > +       !mmc->ocr_avail)
> > > +           mmc->ocr_avail = MMC_VDD_32_33 | MMC_VDD_33_34;
> > > +
> > 
> > Maybe you could use regulator-fixed in the dts and configure
> > min/max
> > voltage to get ocr_avail, no need to set hard code here.
> > 
> 
> Also suggested by Wenbin Mei (梅文彬) and I just tried this.
> 
> This can't be done, fixed-regulator needs to have the same min and
> max
> voltage or they fail to probe sooo fixed-regulator saddly can't be
> used
> :(
> 
> I will send a new version of this with the other point corrected but
> I
> think a compatible and these additional if is a must :(
If use the fixed regulator such as below, you will get the same
ocr_avail as 'MMC_VDD_32_33 | MMC_VDD_33_34' through
mmc_regulator_get_ocrmask().

vmmc_3v3: regulator-vmmc-3v3 {
	compatible = "regulator-fixed";
	regulator-name = "vmmc";
	regulator-min-microvolt = <3300000>;
	regulator-max-microvolt = <3300000>;
	regulator-always-on;
}
> 
> > >     msdc_init_hw(host);
> > > 
> > >     if (mmc->caps2 & MMC_CAP2_CQE) {
> 
> --
>         Ansuel

Re: [PATCH 2/2] mmc: mtk-sd: add support for AN7581 MMC Host

[Christian Marangi]

On Tue, Jan 21, 2025 at 06:25:48AM +0000, Andy-ld Lu (卢东) wrote:
> On Mon, 2025-01-20 at 18:29 +0100, Christian Marangi wrote:
> > External email : Please do not click links or open attachments until
> > you have verified the sender or the content.
> > 
> > 
> > On Thu, Jan 16, 2025 at 07:01:13AM +0000, Andy-ld Lu (卢东) wrote:
> > > On Wed, 2025-01-15 at 08:29 +0100, Christian Marangi wrote:
> > > > Add support for AN7581 MMC Host. The MMC Host controller is based
> > > > on
> > > > mt7622 with the difference of not having regulator supply and
> > > > state_uhs
> > > > pins and hclk clock.
> > > > 
> > > > Some minor fixes are applied to check if the state_uhs pins are
> > > > defined
> > > > and make hclk optional for the new airoha compatible.
> > > > 
> > > > Signed-off-by: Christian Marangi <ansuelsmth@gmail.com>
> > > > ---
> > > >  drivers/mmc/host/mtk-sd.c | 55 ++++++++++++++++++++++++++++++++-
> > > > ----
> > > > --
> > > >  1 file changed, 46 insertions(+), 9 deletions(-)
> > > > 
> > > > diff --git a/drivers/mmc/host/mtk-sd.c b/drivers/mmc/host/mtk-
> > > > sd.c
> > > > index efb0d2d5716b..9d6868883c91 100644
> > > > --- a/drivers/mmc/host/mtk-sd.c
> > > > +++ b/drivers/mmc/host/mtk-sd.c
> > > > @@ -666,6 +666,20 @@ static const struct mtk_mmc_compatible
> > > > mt8196_compat = {
> > > >     .support_new_rx = true,
> > > >  };
> > > > 
> > > > +static const struct mtk_mmc_compatible an7581_compat = {
> > > > +   .clk_div_bits = 12,
> > > > +   .recheck_sdio_irq = true,
> > > > +   .hs400_tune = false,
> > > > +   .pad_tune_reg = MSDC_PAD_TUNE0,
> > > > +   .async_fifo = true,
> > > > +   .data_tune = true,
> > > > +   .busy_check = true,
> > > > +   .stop_clk_fix = true,
> > > > +   .stop_dly_sel = 3,
> > > > +   .enhance_rx = true,
> > > > +   .support_64g = false,
> > > > +};
> > > > +
> > > >  static const struct of_device_id msdc_of_ids[] = {
> > > >     { .compatible = "mediatek,mt2701-mmc", .data =
> > > > &mt2701_compat},
> > > >     { .compatible = "mediatek,mt2712-mmc", .data =
> > > > &mt2712_compat},
> > > > @@ -680,7 +694,7 @@ static const struct of_device_id
> > > > msdc_of_ids[] =
> > > > {
> > > >     { .compatible = "mediatek,mt8183-mmc", .data =
> > > > &mt8183_compat},
> > > >     { .compatible = "mediatek,mt8196-mmc", .data =
> > > > &mt8196_compat},
> > > >     { .compatible = "mediatek,mt8516-mmc", .data =
> > > > &mt8516_compat},
> > > > -
> > > > +   { .compatible = "airoha,an7581-mmc", .data = &an7581_compat},
> > > >     {}
> > > >  };
> > > >  MODULE_DEVICE_TABLE(of, msdc_of_ids);
> > > > @@ -1600,6 +1614,10 @@ static int msdc_ops_switch_volt(struct
> > > > mmc_host *mmc, struct mmc_ios *ios)
> > > >     struct msdc_host *host = mmc_priv(mmc);
> > > >     int ret;
> > > > 
> > > > +   /* Skip setting supply if not supported */
> > > > +   if (!mmc->supply.vqmmc)
> > > > +           return 0;
> > > > +
> > > 
> > > Hi Christian,
> > > 
> > > I think here is no need. If you have not 'vqmmc' in the
> > > dts, IS_ERR(mmc->supply.vqmmc) would be -ENODEV and the
> > > corresponding
> > > flow would not be executed.
> > > 
> > > And another question, host->pins_default is just selected here,
> > > that
> > > would be lost.
> > > 
> > > >     if (!IS_ERR(mmc->supply.vqmmc)) {
> > > >             if (ios->signal_voltage != MMC_SIGNAL_VOLTAGE_330 &&
> > > >                 ios->signal_voltage != MMC_SIGNAL_VOLTAGE_180) {
> > > > @@ -1699,7 +1717,9 @@ static void msdc_enable_sdio_irq(struct
> > > > mmc_host *mmc, int enb)
> > > >                             dev_dbg(host->dev, "SDIO eint irq:
> > > > %d!\n", host->eint_irq);
> > > >                     }
> > > > 
> > > > -                   pinctrl_select_state(host->pinctrl, host-
> > > > > pins_uhs);
> > > > 
> > > > +                   /* Skip setting uhs pins if not supported */
> > > > +                   if (host->pins_uhs)
> > > > +                           pinctrl_select_state(host->pinctrl,
> > > > host->pins_uhs);
> > > >             } else {
> > > >                     dev_pm_clear_wake_irq(host->dev);
> > > >             }
> > > > @@ -2036,6 +2056,10 @@ static void msdc_ops_set_ios(struct
> > > > mmc_host
> > > > *mmc, struct mmc_ios *ios)
> > > > 
> > > >     msdc_set_buswidth(host, ios->bus_width);
> > > > 
> > > > +   /* Skip regulator if not supported */
> > > > +   if (!mmc->supply.vmmc)
> > > > +           goto skip_regulator;
> > > > +
> > > 
> > > No need too.
> > > 
> > > >     /* Suspend/Resume will do power off/on */
> > > >     switch (ios->power_mode) {
> > > >     case MMC_POWER_UP:
> > > > @@ -2071,6 +2095,7 @@ static void msdc_ops_set_ios(struct
> > > > mmc_host
> > > > *mmc, struct mmc_ios *ios)
> > > >             break;
> > > >     }
> > > > 
> > > > +skip_regulator:
> > > >     if (host->mclk != ios->clock || host->timing != ios->timing)
> > > >             msdc_set_mclk(host, ios->timing, ios->clock);
> > > >  }
> > > > @@ -2816,9 +2841,12 @@ static int msdc_of_clock_parse(struct
> > > > platform_device *pdev,
> > > >     if (IS_ERR(host->src_clk))
> > > >             return PTR_ERR(host->src_clk);
> > > > 
> > > > -   host->h_clk = devm_clk_get(&pdev->dev, "hclk");
> > > > -   if (IS_ERR(host->h_clk))
> > > > -           return PTR_ERR(host->h_clk);
> > > > +   /* AN7581 SoC doesn't have hclk */
> > > > +   if (!device_is_compatible(&pdev->dev, "airoha,an7581-mmc")) {
> > > > +           host->h_clk = devm_clk_get(&pdev->dev, "hclk");
> > > > +           if (IS_ERR(host->h_clk))
> > > > +                   return PTR_ERR(host->h_clk);
> > > > +   }
> > > 
> > > devm_clk_get_optional could be used to instead here, no need to use
> > > compatible to distinguish.
> > > 
> > 
> > I can make the hclk optional but I think this would affect also every
> > other compatible by hiding broken clock configuration.
> > 
> > > >     host->bus_clk = devm_clk_get_optional(&pdev->dev, "bus_clk");
> > > >     if (IS_ERR(host->bus_clk))
> > > > @@ -2926,10 +2954,13 @@ static int msdc_drv_probe(struct
> > > > platform_device *pdev)
> > > >             return PTR_ERR(host->pins_default);
> > > >     }
> > > > 
> > > > -   host->pins_uhs = pinctrl_lookup_state(host->pinctrl,
> > > > "state_uhs");
> > > > -   if (IS_ERR(host->pins_uhs)) {
> > > > -           dev_err(&pdev->dev, "Cannot find pinctrl uhs!\n");
> > > > -           return PTR_ERR(host->pins_uhs);
> > > > +   /* AN7581 doesn't have state_uhs pins */
> > > > +   if (!device_is_compatible(&pdev->dev, "airoha,an7581-mmc")) {
> > > > +           host->pins_uhs = pinctrl_lookup_state(host->pinctrl,
> > > > "state_uhs");
> > > > +           if (IS_ERR(host->pins_uhs)) {
> > > > +                   dev_err(&pdev->dev, "Cannot find pinctrl
> > > > uhs!\n");
> > > > +                   return PTR_ERR(host->pins_uhs);
> > > > +           }
> > > >     }
> > > 
> > > Could you consider to set a dummy 'state_uhs' same as
> > > 'state_default'
> > > in the dts, that you could not use compatible to distinguish here.
> > > 
> > 
> > This is problematic, correct me if I'm wrong, you are suggesting to
> > assign the emmc pins to both default and uhs? This is problematic as
> > the
> > pinctrl driver would complain that such pins are already assigned to
> > something. Also I don't think it's possible to assign these pins to a
> > dummy pin.
> > 
> Maybe I have not expressed clearly...What I mean is that you could set
> as below, and the content of &mmc_pins_uhs is just copied from
> &mmc_pins_default.
> 
> mmc@1fa0e000 {
> 	...
> 	pinctrl-names = "default", "state_uhs";
> 	pinctrl-0 = <&mmc_pins_default>;
> 	pinctrl-1 = <&mmc_pins_uhs>;
> }

Ok my bad. I did declared the second pin to pinctrl-0 instead of adding
pinctrl-1. With that it does work correctly.

> > > > 
> > > >     /* Support for SDIO eint irq ? */
> > > > @@ -3010,6 +3041,12 @@ static int msdc_drv_probe(struct
> > > > platform_device *pdev)
> > > >             dev_err(&pdev->dev, "Cannot ungate clocks!\n");
> > > >             goto release_clk;
> > > >     }
> > > > +
> > > > +   /* AN7581 without regulator require tune to OCR values */
> > > > +   if (device_is_compatible(&pdev->dev, "airoha,an7581-mmc") &&
> > > > +       !mmc->ocr_avail)
> > > > +           mmc->ocr_avail = MMC_VDD_32_33 | MMC_VDD_33_34;
> > > > +
> > > 
> > > Maybe you could use regulator-fixed in the dts and configure
> > > min/max
> > > voltage to get ocr_avail, no need to set hard code here.
> > > 
> > 
> > Also suggested by Wenbin Mei (梅文彬) and I just tried this.
> > 
> > This can't be done, fixed-regulator needs to have the same min and
> > max
> > voltage or they fail to probe sooo fixed-regulator saddly can't be
> > used
> > :(
> > 
> > I will send a new version of this with the other point corrected but
> > I
> > think a compatible and these additional if is a must :(
> If use the fixed regulator such as below, you will get the same
> ocr_avail as 'MMC_VDD_32_33 | MMC_VDD_33_34' through
> mmc_regulator_get_ocrmask().
> 
> vmmc_3v3: regulator-vmmc-3v3 {
> 	compatible = "regulator-fixed";
> 	regulator-name = "vmmc";
> 	regulator-min-microvolt = <3300000>;
> 	regulator-max-microvolt = <3300000>;
> 	regulator-always-on;
> }

Ok the code was a bit confusing but yes I can confirm that a 3.3 fixed
regulator define those 2 flags so also this is OK.

There is still the discussion about clock. You are totally against a new
compatible for the hclk? 

> > 
> > > >     msdc_init_hw(host);
> > > > 
> > > >     if (mmc->caps2 & MMC_CAP2_CQE) {
> > 
> > --
> >         Ansuel

-- 
	Ansuel

Re: [PATCH 2/2] mmc: mtk-sd: add support for AN7581 MMC Host

[Andy-ld Lu (卢东)]

On Wed, 2025-01-22 at 10:32 +0100, Christian Marangi wrote:
> External email : Please do not click links or open attachments until
> you have verified the sender or the content.
> 
> 
> On Tue, Jan 21, 2025 at 06:25:48AM +0000, Andy-ld Lu (卢东) wrote:
> > On Mon, 2025-01-20 at 18:29 +0100, Christian Marangi wrote:
> > > External email : Please do not click links or open attachments
> > > until
> > > you have verified the sender or the content.
> > > 
> > > 
> > > On Thu, Jan 16, 2025 at 07:01:13AM +0000, Andy-ld Lu (卢东) wrote:
> > > > On Wed, 2025-01-15 at 08:29 +0100, Christian Marangi wrote:
> > > > > Add support for AN7581 MMC Host. The MMC Host controller is
> > > > > based
> > > > > on
> > > > > mt7622 with the difference of not having regulator supply and
> > > > > state_uhs
> > > > > pins and hclk clock.
> > > > > 
> > > > > Some minor fixes are applied to check if the state_uhs pins
> > > > > are
> > > > > defined
> > > > > and make hclk optional for the new airoha compatible.
> > > > > 
> > > > > Signed-off-by: Christian Marangi <ansuelsmth@gmail.com>
> > > > > ---
> > > > >  drivers/mmc/host/mtk-sd.c | 55
> > > > > ++++++++++++++++++++++++++++++++-
> > > > > ----
> > > > > --
> > > > >  1 file changed, 46 insertions(+), 9 deletions(-)
> > > > > 
> > > > > diff --git a/drivers/mmc/host/mtk-sd.c
> > > > > b/drivers/mmc/host/mtk-
> > > > > sd.c
> > > > > index efb0d2d5716b..9d6868883c91 100644
> > > > > --- a/drivers/mmc/host/mtk-sd.c
> > > > > +++ b/drivers/mmc/host/mtk-sd.c
> > > > > @@ -666,6 +666,20 @@ static const struct mtk_mmc_compatible
> > > > > mt8196_compat = {
> > > > >     .support_new_rx = true,
> > > > >  };
> > > > > 
> > > > > +static const struct mtk_mmc_compatible an7581_compat = {
> > > > > +   .clk_div_bits = 12,
> > > > > +   .recheck_sdio_irq = true,
> > > > > +   .hs400_tune = false,
> > > > > +   .pad_tune_reg = MSDC_PAD_TUNE0,
> > > > > +   .async_fifo = true,
> > > > > +   .data_tune = true,
> > > > > +   .busy_check = true,
> > > > > +   .stop_clk_fix = true,
> > > > > +   .stop_dly_sel = 3,
> > > > > +   .enhance_rx = true,
> > > > > +   .support_64g = false,
> > > > > +};
> > > > > +
> > > > >  static const struct of_device_id msdc_of_ids[] = {
> > > > >     { .compatible = "mediatek,mt2701-mmc", .data =
> > > > > &mt2701_compat},
> > > > >     { .compatible = "mediatek,mt2712-mmc", .data =
> > > > > &mt2712_compat},
> > > > > @@ -680,7 +694,7 @@ static const struct of_device_id
> > > > > msdc_of_ids[] =
> > > > > {
> > > > >     { .compatible = "mediatek,mt8183-mmc", .data =
> > > > > &mt8183_compat},
> > > > >     { .compatible = "mediatek,mt8196-mmc", .data =
> > > > > &mt8196_compat},
> > > > >     { .compatible = "mediatek,mt8516-mmc", .data =
> > > > > &mt8516_compat},
> > > > > -
> > > > > +   { .compatible = "airoha,an7581-mmc", .data =
> > > > > &an7581_compat},
> > > > >     {}
> > > > >  };
> > > > >  MODULE_DEVICE_TABLE(of, msdc_of_ids);
> > > > > @@ -1600,6 +1614,10 @@ static int msdc_ops_switch_volt(struct
> > > > > mmc_host *mmc, struct mmc_ios *ios)
> > > > >     struct msdc_host *host = mmc_priv(mmc);
> > > > >     int ret;
> > > > > 
> > > > > +   /* Skip setting supply if not supported */
> > > > > +   if (!mmc->supply.vqmmc)
> > > > > +           return 0;
> > > > > +
> > > > 
> > > > Hi Christian,
> > > > 
> > > > I think here is no need. If you have not 'vqmmc' in the
> > > > dts, IS_ERR(mmc->supply.vqmmc) would be -ENODEV and the
> > > > corresponding
> > > > flow would not be executed.
> > > > 
> > > > And another question, host->pins_default is just selected here,
> > > > that
> > > > would be lost.
> > > > 
> > > > >     if (!IS_ERR(mmc->supply.vqmmc)) {
> > > > >             if (ios->signal_voltage != MMC_SIGNAL_VOLTAGE_330
> > > > > &&
> > > > >                 ios->signal_voltage !=
> > > > > MMC_SIGNAL_VOLTAGE_180) {
> > > > > @@ -1699,7 +1717,9 @@ static void msdc_enable_sdio_irq(struct
> > > > > mmc_host *mmc, int enb)
> > > > >                             dev_dbg(host->dev, "SDIO eint
> > > > > irq:
> > > > > %d!\n", host->eint_irq);
> > > > >                     }
> > > > > 
> > > > > -                   pinctrl_select_state(host->pinctrl, host-
> > > > > > pins_uhs);
> > > > > 
> > > > > +                   /* Skip setting uhs pins if not supported
> > > > > */
> > > > > +                   if (host->pins_uhs)
> > > > > +                           pinctrl_select_state(host-
> > > > > >pinctrl,
> > > > > host->pins_uhs);
> > > > >             } else {
> > > > >                     dev_pm_clear_wake_irq(host->dev);
> > > > >             }
> > > > > @@ -2036,6 +2056,10 @@ static void msdc_ops_set_ios(struct
> > > > > mmc_host
> > > > > *mmc, struct mmc_ios *ios)
> > > > > 
> > > > >     msdc_set_buswidth(host, ios->bus_width);
> > > > > 
> > > > > +   /* Skip regulator if not supported */
> > > > > +   if (!mmc->supply.vmmc)
> > > > > +           goto skip_regulator;
> > > > > +
> > > > 
> > > > No need too.
> > > > 
> > > > >     /* Suspend/Resume will do power off/on */
> > > > >     switch (ios->power_mode) {
> > > > >     case MMC_POWER_UP:
> > > > > @@ -2071,6 +2095,7 @@ static void msdc_ops_set_ios(struct
> > > > > mmc_host
> > > > > *mmc, struct mmc_ios *ios)
> > > > >             break;
> > > > >     }
> > > > > 
> > > > > +skip_regulator:
> > > > >     if (host->mclk != ios->clock || host->timing != ios-
> > > > > >timing)
> > > > >             msdc_set_mclk(host, ios->timing, ios->clock);
> > > > >  }
> > > > > @@ -2816,9 +2841,12 @@ static int msdc_of_clock_parse(struct
> > > > > platform_device *pdev,
> > > > >     if (IS_ERR(host->src_clk))
> > > > >             return PTR_ERR(host->src_clk);
> > > > > 
> > > > > -   host->h_clk = devm_clk_get(&pdev->dev, "hclk");
> > > > > -   if (IS_ERR(host->h_clk))
> > > > > -           return PTR_ERR(host->h_clk);
> > > > > +   /* AN7581 SoC doesn't have hclk */
> > > > > +   if (!device_is_compatible(&pdev->dev, "airoha,an7581-
> > > > > mmc")) {
> > > > > +           host->h_clk = devm_clk_get(&pdev->dev, "hclk");
> > > > > +           if (IS_ERR(host->h_clk))
> > > > > +                   return PTR_ERR(host->h_clk);
> > > > > +   }
> > > > 
> > > > devm_clk_get_optional could be used to instead here, no need to
> > > > use
> > > > compatible to distinguish.
> > > > 
> > > 
> > > I can make the hclk optional but I think this would affect also
> > > every
> > > other compatible by hiding broken clock configuration.
> > > 
> > > > >     host->bus_clk = devm_clk_get_optional(&pdev->dev,
> > > > > "bus_clk");
> > > > >     if (IS_ERR(host->bus_clk))
> > > > > @@ -2926,10 +2954,13 @@ static int msdc_drv_probe(struct
> > > > > platform_device *pdev)
> > > > >             return PTR_ERR(host->pins_default);
> > > > >     }
> > > > > 
> > > > > -   host->pins_uhs = pinctrl_lookup_state(host->pinctrl,
> > > > > "state_uhs");
> > > > > -   if (IS_ERR(host->pins_uhs)) {
> > > > > -           dev_err(&pdev->dev, "Cannot find pinctrl
> > > > > uhs!\n");
> > > > > -           return PTR_ERR(host->pins_uhs);
> > > > > +   /* AN7581 doesn't have state_uhs pins */
> > > > > +   if (!device_is_compatible(&pdev->dev, "airoha,an7581-
> > > > > mmc")) {
> > > > > +           host->pins_uhs = pinctrl_lookup_state(host-
> > > > > >pinctrl,
> > > > > "state_uhs");
> > > > > +           if (IS_ERR(host->pins_uhs)) {
> > > > > +                   dev_err(&pdev->dev, "Cannot find pinctrl
> > > > > uhs!\n");
> > > > > +                   return PTR_ERR(host->pins_uhs);
> > > > > +           }
> > > > >     }
> > > > 
> > > > Could you consider to set a dummy 'state_uhs' same as
> > > > 'state_default'
> > > > in the dts, that you could not use compatible to distinguish
> > > > here.
> > > > 
> > > 
> > > This is problematic, correct me if I'm wrong, you are suggesting
> > > to
> > > assign the emmc pins to both default and uhs? This is problematic
> > > as
> > > the
> > > pinctrl driver would complain that such pins are already assigned
> > > to
> > > something. Also I don't think it's possible to assign these pins
> > > to a
> > > dummy pin.
> > > 
> > 
> > Maybe I have not expressed clearly...What I mean is that you could
> > set
> > as below, and the content of &mmc_pins_uhs is just copied from
> > &mmc_pins_default.
> > 
> > mmc@1fa0e000 {
> >       ...
> >       pinctrl-names = "default", "state_uhs";
> >       pinctrl-0 = <&mmc_pins_default>;
> >       pinctrl-1 = <&mmc_pins_uhs>;
> > }
> 
> Ok my bad. I did declared the second pin to pinctrl-0 instead of
> adding
> pinctrl-1. With that it does work correctly.
> 
> > > > > 
> > > > >     /* Support for SDIO eint irq ? */
> > > > > @@ -3010,6 +3041,12 @@ static int msdc_drv_probe(struct
> > > > > platform_device *pdev)
> > > > >             dev_err(&pdev->dev, "Cannot ungate clocks!\n");
> > > > >             goto release_clk;
> > > > >     }
> > > > > +
> > > > > +   /* AN7581 without regulator require tune to OCR values */
> > > > > +   if (device_is_compatible(&pdev->dev, "airoha,an7581-mmc") 
> > > > > &&
> > > > > +       !mmc->ocr_avail)
> > > > > +           mmc->ocr_avail = MMC_VDD_32_33 | MMC_VDD_33_34;
> > > > > +
> > > > 
> > > > Maybe you could use regulator-fixed in the dts and configure
> > > > min/max
> > > > voltage to get ocr_avail, no need to set hard code here.
> > > > 
> > > 
> > > Also suggested by Wenbin Mei (梅文彬) and I just tried this.
> > > 
> > > This can't be done, fixed-regulator needs to have the same min
> > > and
> > > max
> > > voltage or they fail to probe sooo fixed-regulator saddly can't
> > > be
> > > used
> > > :(
> > > 
> > > I will send a new version of this with the other point corrected
> > > but
> > > I
> > > think a compatible and these additional if is a must :(
> > 
> > If use the fixed regulator such as below, you will get the same
> > ocr_avail as 'MMC_VDD_32_33 | MMC_VDD_33_34' through
> > mmc_regulator_get_ocrmask().
> > 
> > vmmc_3v3: regulator-vmmc-3v3 {
> >       compatible = "regulator-fixed";
> >       regulator-name = "vmmc";
> >       regulator-min-microvolt = <3300000>;
> >       regulator-max-microvolt = <3300000>;
> >       regulator-always-on;
> > }
> 
> Ok the code was a bit confusing but yes I can confirm that a 3.3
> fixed
> regulator define those 2 flags so also this is OK.
> 
> There is still the discussion about clock. You are totally against a
> new
> compatible for the hclk?
> 
As the comment in the v2 patches, the better way is to add a bool
variable like 'needs_not_hclk' in the compat data, which is just true
for you, and use !host->dev_comp->needs_not_hclk as the condition to
get 'hclk'.

But I would like to confirm if the 'fixed-clock' could be supported in
your project, which is also used in mt7622.dtsi, you may align 'hclk'
to a dummy fixed-clock... 

> > > 
> > > > >     msdc_init_hw(host);
> > > > > 
> > > > >     if (mmc->caps2 & MMC_CAP2_CQE) {
> > > 
> > > --
> > >         Ansuel
> 
> --
>         Ansuel

Re: [PATCH 2/2] mmc: mtk-sd: add support for AN7581 MMC Host

[Christian Marangi]

On Thu, Jan 23, 2025 at 01:42:03AM +0000, Andy-ld Lu (卢东) wrote:
> On Wed, 2025-01-22 at 10:32 +0100, Christian Marangi wrote:
> > External email : Please do not click links or open attachments until
> > you have verified the sender or the content.
> > 
> > 
> > On Tue, Jan 21, 2025 at 06:25:48AM +0000, Andy-ld Lu (卢东) wrote:
> > > On Mon, 2025-01-20 at 18:29 +0100, Christian Marangi wrote:
> > > > External email : Please do not click links or open attachments
> > > > until
> > > > you have verified the sender or the content.
> > > > 
> > > > 
> > > > On Thu, Jan 16, 2025 at 07:01:13AM +0000, Andy-ld Lu (卢东) wrote:
> > > > > On Wed, 2025-01-15 at 08:29 +0100, Christian Marangi wrote:
> > > > > > Add support for AN7581 MMC Host. The MMC Host controller is
> > > > > > based
> > > > > > on
> > > > > > mt7622 with the difference of not having regulator supply and
> > > > > > state_uhs
> > > > > > pins and hclk clock.
> > > > > > 
> > > > > > Some minor fixes are applied to check if the state_uhs pins
> > > > > > are
> > > > > > defined
> > > > > > and make hclk optional for the new airoha compatible.
> > > > > > 
> > > > > > Signed-off-by: Christian Marangi <ansuelsmth@gmail.com>
> > > > > > ---
> > > > > >  drivers/mmc/host/mtk-sd.c | 55
> > > > > > ++++++++++++++++++++++++++++++++-
> > > > > > ----
> > > > > > --
> > > > > >  1 file changed, 46 insertions(+), 9 deletions(-)
> > > > > > 
> > > > > > diff --git a/drivers/mmc/host/mtk-sd.c
> > > > > > b/drivers/mmc/host/mtk-
> > > > > > sd.c
> > > > > > index efb0d2d5716b..9d6868883c91 100644
> > > > > > --- a/drivers/mmc/host/mtk-sd.c
> > > > > > +++ b/drivers/mmc/host/mtk-sd.c
> > > > > > @@ -666,6 +666,20 @@ static const struct mtk_mmc_compatible
> > > > > > mt8196_compat = {
> > > > > >     .support_new_rx = true,
> > > > > >  };
> > > > > > 
> > > > > > +static const struct mtk_mmc_compatible an7581_compat = {
> > > > > > +   .clk_div_bits = 12,
> > > > > > +   .recheck_sdio_irq = true,
> > > > > > +   .hs400_tune = false,
> > > > > > +   .pad_tune_reg = MSDC_PAD_TUNE0,
> > > > > > +   .async_fifo = true,
> > > > > > +   .data_tune = true,
> > > > > > +   .busy_check = true,
> > > > > > +   .stop_clk_fix = true,
> > > > > > +   .stop_dly_sel = 3,
> > > > > > +   .enhance_rx = true,
> > > > > > +   .support_64g = false,
> > > > > > +};
> > > > > > +
> > > > > >  static const struct of_device_id msdc_of_ids[] = {
> > > > > >     { .compatible = "mediatek,mt2701-mmc", .data =
> > > > > > &mt2701_compat},
> > > > > >     { .compatible = "mediatek,mt2712-mmc", .data =
> > > > > > &mt2712_compat},
> > > > > > @@ -680,7 +694,7 @@ static const struct of_device_id
> > > > > > msdc_of_ids[] =
> > > > > > {
> > > > > >     { .compatible = "mediatek,mt8183-mmc", .data =
> > > > > > &mt8183_compat},
> > > > > >     { .compatible = "mediatek,mt8196-mmc", .data =
> > > > > > &mt8196_compat},
> > > > > >     { .compatible = "mediatek,mt8516-mmc", .data =
> > > > > > &mt8516_compat},
> > > > > > -
> > > > > > +   { .compatible = "airoha,an7581-mmc", .data =
> > > > > > &an7581_compat},
> > > > > >     {}
> > > > > >  };
> > > > > >  MODULE_DEVICE_TABLE(of, msdc_of_ids);
> > > > > > @@ -1600,6 +1614,10 @@ static int msdc_ops_switch_volt(struct
> > > > > > mmc_host *mmc, struct mmc_ios *ios)
> > > > > >     struct msdc_host *host = mmc_priv(mmc);
> > > > > >     int ret;
> > > > > > 
> > > > > > +   /* Skip setting supply if not supported */
> > > > > > +   if (!mmc->supply.vqmmc)
> > > > > > +           return 0;
> > > > > > +
> > > > > 
> > > > > Hi Christian,
> > > > > 
> > > > > I think here is no need. If you have not 'vqmmc' in the
> > > > > dts, IS_ERR(mmc->supply.vqmmc) would be -ENODEV and the
> > > > > corresponding
> > > > > flow would not be executed.
> > > > > 
> > > > > And another question, host->pins_default is just selected here,
> > > > > that
> > > > > would be lost.
> > > > > 
> > > > > >     if (!IS_ERR(mmc->supply.vqmmc)) {
> > > > > >             if (ios->signal_voltage != MMC_SIGNAL_VOLTAGE_330
> > > > > > &&
> > > > > >                 ios->signal_voltage !=
> > > > > > MMC_SIGNAL_VOLTAGE_180) {
> > > > > > @@ -1699,7 +1717,9 @@ static void msdc_enable_sdio_irq(struct
> > > > > > mmc_host *mmc, int enb)
> > > > > >                             dev_dbg(host->dev, "SDIO eint
> > > > > > irq:
> > > > > > %d!\n", host->eint_irq);
> > > > > >                     }
> > > > > > 
> > > > > > -                   pinctrl_select_state(host->pinctrl, host-
> > > > > > > pins_uhs);
> > > > > > 
> > > > > > +                   /* Skip setting uhs pins if not supported
> > > > > > */
> > > > > > +                   if (host->pins_uhs)
> > > > > > +                           pinctrl_select_state(host-
> > > > > > >pinctrl,
> > > > > > host->pins_uhs);
> > > > > >             } else {
> > > > > >                     dev_pm_clear_wake_irq(host->dev);
> > > > > >             }
> > > > > > @@ -2036,6 +2056,10 @@ static void msdc_ops_set_ios(struct
> > > > > > mmc_host
> > > > > > *mmc, struct mmc_ios *ios)
> > > > > > 
> > > > > >     msdc_set_buswidth(host, ios->bus_width);
> > > > > > 
> > > > > > +   /* Skip regulator if not supported */
> > > > > > +   if (!mmc->supply.vmmc)
> > > > > > +           goto skip_regulator;
> > > > > > +
> > > > > 
> > > > > No need too.
> > > > > 
> > > > > >     /* Suspend/Resume will do power off/on */
> > > > > >     switch (ios->power_mode) {
> > > > > >     case MMC_POWER_UP:
> > > > > > @@ -2071,6 +2095,7 @@ static void msdc_ops_set_ios(struct
> > > > > > mmc_host
> > > > > > *mmc, struct mmc_ios *ios)
> > > > > >             break;
> > > > > >     }
> > > > > > 
> > > > > > +skip_regulator:
> > > > > >     if (host->mclk != ios->clock || host->timing != ios-
> > > > > > >timing)
> > > > > >             msdc_set_mclk(host, ios->timing, ios->clock);
> > > > > >  }
> > > > > > @@ -2816,9 +2841,12 @@ static int msdc_of_clock_parse(struct
> > > > > > platform_device *pdev,
> > > > > >     if (IS_ERR(host->src_clk))
> > > > > >             return PTR_ERR(host->src_clk);
> > > > > > 
> > > > > > -   host->h_clk = devm_clk_get(&pdev->dev, "hclk");
> > > > > > -   if (IS_ERR(host->h_clk))
> > > > > > -           return PTR_ERR(host->h_clk);
> > > > > > +   /* AN7581 SoC doesn't have hclk */
> > > > > > +   if (!device_is_compatible(&pdev->dev, "airoha,an7581-
> > > > > > mmc")) {
> > > > > > +           host->h_clk = devm_clk_get(&pdev->dev, "hclk");
> > > > > > +           if (IS_ERR(host->h_clk))
> > > > > > +                   return PTR_ERR(host->h_clk);
> > > > > > +   }
> > > > > 
> > > > > devm_clk_get_optional could be used to instead here, no need to
> > > > > use
> > > > > compatible to distinguish.
> > > > > 
> > > > 
> > > > I can make the hclk optional but I think this would affect also
> > > > every
> > > > other compatible by hiding broken clock configuration.
> > > > 
> > > > > >     host->bus_clk = devm_clk_get_optional(&pdev->dev,
> > > > > > "bus_clk");
> > > > > >     if (IS_ERR(host->bus_clk))
> > > > > > @@ -2926,10 +2954,13 @@ static int msdc_drv_probe(struct
> > > > > > platform_device *pdev)
> > > > > >             return PTR_ERR(host->pins_default);
> > > > > >     }
> > > > > > 
> > > > > > -   host->pins_uhs = pinctrl_lookup_state(host->pinctrl,
> > > > > > "state_uhs");
> > > > > > -   if (IS_ERR(host->pins_uhs)) {
> > > > > > -           dev_err(&pdev->dev, "Cannot find pinctrl
> > > > > > uhs!\n");
> > > > > > -           return PTR_ERR(host->pins_uhs);
> > > > > > +   /* AN7581 doesn't have state_uhs pins */
> > > > > > +   if (!device_is_compatible(&pdev->dev, "airoha,an7581-
> > > > > > mmc")) {
> > > > > > +           host->pins_uhs = pinctrl_lookup_state(host-
> > > > > > >pinctrl,
> > > > > > "state_uhs");
> > > > > > +           if (IS_ERR(host->pins_uhs)) {
> > > > > > +                   dev_err(&pdev->dev, "Cannot find pinctrl
> > > > > > uhs!\n");
> > > > > > +                   return PTR_ERR(host->pins_uhs);
> > > > > > +           }
> > > > > >     }
> > > > > 
> > > > > Could you consider to set a dummy 'state_uhs' same as
> > > > > 'state_default'
> > > > > in the dts, that you could not use compatible to distinguish
> > > > > here.
> > > > > 
> > > > 
> > > > This is problematic, correct me if I'm wrong, you are suggesting
> > > > to
> > > > assign the emmc pins to both default and uhs? This is problematic
> > > > as
> > > > the
> > > > pinctrl driver would complain that such pins are already assigned
> > > > to
> > > > something. Also I don't think it's possible to assign these pins
> > > > to a
> > > > dummy pin.
> > > > 
> > > 
> > > Maybe I have not expressed clearly...What I mean is that you could
> > > set
> > > as below, and the content of &mmc_pins_uhs is just copied from
> > > &mmc_pins_default.
> > > 
> > > mmc@1fa0e000 {
> > >       ...
> > >       pinctrl-names = "default", "state_uhs";
> > >       pinctrl-0 = <&mmc_pins_default>;
> > >       pinctrl-1 = <&mmc_pins_uhs>;
> > > }
> > 
> > Ok my bad. I did declared the second pin to pinctrl-0 instead of
> > adding
> > pinctrl-1. With that it does work correctly.
> > 
> > > > > > 
> > > > > >     /* Support for SDIO eint irq ? */
> > > > > > @@ -3010,6 +3041,12 @@ static int msdc_drv_probe(struct
> > > > > > platform_device *pdev)
> > > > > >             dev_err(&pdev->dev, "Cannot ungate clocks!\n");
> > > > > >             goto release_clk;
> > > > > >     }
> > > > > > +
> > > > > > +   /* AN7581 without regulator require tune to OCR values */
> > > > > > +   if (device_is_compatible(&pdev->dev, "airoha,an7581-mmc") 
> > > > > > &&
> > > > > > +       !mmc->ocr_avail)
> > > > > > +           mmc->ocr_avail = MMC_VDD_32_33 | MMC_VDD_33_34;
> > > > > > +
> > > > > 
> > > > > Maybe you could use regulator-fixed in the dts and configure
> > > > > min/max
> > > > > voltage to get ocr_avail, no need to set hard code here.
> > > > > 
> > > > 
> > > > Also suggested by Wenbin Mei (梅文彬) and I just tried this.
> > > > 
> > > > This can't be done, fixed-regulator needs to have the same min
> > > > and
> > > > max
> > > > voltage or they fail to probe sooo fixed-regulator saddly can't
> > > > be
> > > > used
> > > > :(
> > > > 
> > > > I will send a new version of this with the other point corrected
> > > > but
> > > > I
> > > > think a compatible and these additional if is a must :(
> > > 
> > > If use the fixed regulator such as below, you will get the same
> > > ocr_avail as 'MMC_VDD_32_33 | MMC_VDD_33_34' through
> > > mmc_regulator_get_ocrmask().
> > > 
> > > vmmc_3v3: regulator-vmmc-3v3 {
> > >       compatible = "regulator-fixed";
> > >       regulator-name = "vmmc";
> > >       regulator-min-microvolt = <3300000>;
> > >       regulator-max-microvolt = <3300000>;
> > >       regulator-always-on;
> > > }
> > 
> > Ok the code was a bit confusing but yes I can confirm that a 3.3
> > fixed
> > regulator define those 2 flags so also this is OK.
> > 
> > There is still the discussion about clock. You are totally against a
> > new
> > compatible for the hclk?
> > 
> As the comment in the v2 patches, the better way is to add a bool
> variable like 'needs_not_hclk' in the compat data, which is just true
> for you, and use !host->dev_comp->needs_not_hclk as the condition to
> get 'hclk'.
> 
> But I would like to confirm if the 'fixed-clock' could be supported in
> your project, which is also used in mt7622.dtsi, you may align 'hclk'
> to a dummy fixed-clock... 
>

Thanks I implemented with dummy clock + dummy regulator + dummy state so
we don't need an additional compatible. Thanks for the suggestions!

> > > > 
> > > > > >     msdc_init_hw(host);
> > > > > > 
> > > > > >     if (mmc->caps2 & MMC_CAP2_CQE) {
> > > > 
> > > > --
> > > >         Ansuel
> > 
> > --
> >         Ansuel

-- 
	Ansuel