[PATCH v2 1/3] dt-bindings: mmc: sdhci-of-aspeed: Add description for AST2600 EVB.

[PATCH v2 1/3] dt-bindings: mmc: sdhci-of-aspeed: Add description for AST2600 EVB.

[Steven Lee]

Add the description for describing the AST 2600 EVB reference design of
GPIO regulators and provide the example in the document.

AST2600-A2 EVB has the reference design for enabling SD bus
power and toggling SD bus signal voltage by GPIO pins.

In the reference design, GPIOV0 of AST2600-A2 EVB is connected to
power load switch that providing 3.3v to SD1 bus vdd. GPIOV1 is
connected to a 1.8v and a 3.3v power load switch that providing
signal voltage to
SD1 bus.

If GPIOV0 is active high, SD1 bus is enabled. Otherwise, SD1 bus is
disabled.
If GPIOV1 is active high, 3.3v power load switch is enabled, SD1
signal voltage is 3.3v. Otherwise, 1.8v power load switch will be
enabled, SD1 signal voltage becomes 1.8v.

AST2600-A2 EVB also support toggling signal voltage for SD2 bus.
The design is the same as SD1 bus. It uses GPIOV2 as power-gpio and
GPIOV3 as power-switch-gpio.

Signed-off-by: Steven Lee <steven_lee@aspeedtech.com>
---
 .../devicetree/bindings/mmc/aspeed,sdhci.yaml | 99 +++++++++++++++++++
 1 file changed, 99 insertions(+)

diff --git a/Documentation/devicetree/bindings/mmc/aspeed,sdhci.yaml b/Documentation/devicetree/bindings/mmc/aspeed,sdhci.yaml
index 987b287f3bff..dd894aba0bb7 100644
--- a/Documentation/devicetree/bindings/mmc/aspeed,sdhci.yaml
+++ b/Documentation/devicetree/bindings/mmc/aspeed,sdhci.yaml
@@ -20,6 +20,19 @@ description: |+
   the slots are dependent on the common configuration area, they are described
   as child nodes.
 
+  The signal voltage of SDHCIs on AST2600-A2 EVB is able to be toggled by GPIO
+  pins. In the reference design, GPIOV0 of AST2600-A2 EVB is connected to the
+  power load switch that providing 3.3v to SD1 bus vdd, GPIOV1 is connected to
+  a 1.8v and a 3.3v power load switch that providing signal voltage to
+  SD1 bus.
+  If GPIOV0 is active high, SD1 bus is enabled. Otherwise, SD1 bus is
+  disabled. If GPIOV1 is active high, 3.3v power load switch is enabled, SD1
+  signal voltage is 3.3v. Otherwise, 1.8v power load switch will be enabled, SD1
+  signal voltage becomes 1.8v.
+  AST2600-A2 EVB also support toggling signal voltage for SD2 bus.
+  The design is the same as SD1 bus. It uses GPIOV2 as power-gpio and GPIOV3
+  as power-switch-gpio.
+
 properties:
   compatible:
     enum:
@@ -78,6 +91,7 @@ required:
   - clocks
 
 examples:
+  //Example 1
   - |
     #include <dt-bindings/clock/aspeed-clock.h>
     sdc@1e740000 {
@@ -104,3 +118,88 @@ examples:
                     clocks = <&syscon ASPEED_CLK_SDIO>;
             };
     };
+
+  //Example 2 (AST2600EVB with GPIO regulator)
+  - |
+    #include <dt-bindings/clock/aspeed-clock.h>
+    #include <dt-bindings/gpio/aspeed-gpio.h>
+    vcc_sdhci0: regulator-vcc-sdhci0 {
+            compatible = "regulator-fixed";
+
+            regulator-name = "SDHCI0 Vcc";
+            regulator-min-microvolt = <3300000>;
+            regulator-max-microvolt = <3300000>;
+            gpios = <&gpio0 ASPEED_GPIO(V, 0)
+                            GPIO_ACTIVE_HIGH>;
+            enable-active-high;
+    };
+
+    vccq_sdhci0: regulator-vccq-sdhci0 {
+            compatible = "regulator-gpio";
+
+            regulator-name = "SDHCI0 VccQ";
+            regulator-min-microvolt = <1800000>;
+            regulator-max-microvolt = <3300000>;
+            gpios = <&gpio0 ASPEED_GPIO(V, 1)
+                            GPIO_ACTIVE_HIGH>;
+            gpios-states = <1>;
+            states = <3300000 1
+                      1800000 0>;
+    };
+
+    vcc_sdhci1: regulator-vcc-sdhci1 {
+            compatible = "regulator-fixed";
+
+            regulator-name = "SDHCI1 Vcc";
+            regulator-min-microvolt = <3300000>;
+            regulator-max-microvolt = <3300000>;
+            gpios = <&gpio0 ASPEED_GPIO(V, 2)
+                            GPIO_ACTIVE_HIGH>;
+            enable-active-high;
+    };
+
+    vccq_sdhci1: regulator-vccq-sdhci1 {
+            compatible = "regulator-gpio";
+
+            regulator-name = "SDHCI1 VccQ";
+            regulator-min-microvolt = <1800000>;
+            regulator-max-microvolt = <3300000>;
+            gpios = <&gpio0 ASPEED_GPIO(V, 3)
+                            GPIO_ACTIVE_HIGH>;
+            gpios-states = <1>;
+            states = <3300000 1
+                      1800000 0>;
+    };
+
+    sdc@1e740000 {
+            compatible = "aspeed,ast2600-sd-controller";
+            reg = <0x1e740000 0x100>;
+            #address-cells = <1>;
+            #size-cells = <1>;
+            ranges = <0 0x1e740000 0x20000>;
+            clocks = <&syscon ASPEED_CLK_GATE_SDCLK>;
+
+            sdhci0: sdhci@100 {
+                    compatible = "aspeed,ast2600-sdhci", "sdhci";
+                    reg = <0x100 0x100>;
+                    interrupts = <GIC_SPI 43 IRQ_TYPE_LEVEL_HIGH>;
+                    sdhci,auto-cmd12;
+                    clocks = <&syscon ASPEED_CLK_SDIO>;
+                    vmmc-supply = <&vcc_sdhci0>;
+                    vqmmc-supply = <&vccq_sdhci0>;
+                    sd-uhs-sdr104;
+                    clk-phase-uhs-sdr104 = <180>, <180>;
+            };
+
+            sdhci1: sdhci@200 {
+                    compatible = "aspeed,ast2600-sdhci", "sdhci";
+                    reg = <0x200 0x100>;
+                    interrupts = <GIC_SPI 43 IRQ_TYPE_LEVEL_HIGH>;
+                    sdhci,auto-cmd12;
+                    clocks = <&syscon ASPEED_CLK_SDIO>;
+                    vmmc-supply = <&vcc_sdhci1>;
+                    vqmmc-supply = <&vccq_sdhci1>;
+                    sd-uhs-sdr104;
+                    clk-phase-uhs-sdr104 = <0>, <0>;
+            };
+    };
-- 
2.17.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v2 1/3] dt-bindings: mmc: sdhci-of-aspeed: Add description for AST2600 EVB.

[Andrew Jeffery]

Hi Steven,

On Mon, 3 May 2021, at 11:13, Steven Lee wrote:
> Add the description for describing the AST 2600 EVB reference design of
> GPIO regulators and provide the example in the document.
> 
> AST2600-A2 EVB has the reference design for enabling SD bus
> power and toggling SD bus signal voltage by GPIO pins.
> 
> In the reference design, GPIOV0 of AST2600-A2 EVB is connected to
> power load switch that providing 3.3v to SD1 bus vdd. GPIOV1 is
> connected to a 1.8v and a 3.3v power load switch that providing
> signal voltage to
> SD1 bus.
> 
> If GPIOV0 is active high, SD1 bus is enabled. Otherwise, SD1 bus is
> disabled.
> If GPIOV1 is active high, 3.3v power load switch is enabled, SD1
> signal voltage is 3.3v. Otherwise, 1.8v power load switch will be
> enabled, SD1 signal voltage becomes 1.8v.
> 
> AST2600-A2 EVB also support toggling signal voltage for SD2 bus.
> The design is the same as SD1 bus. It uses GPIOV2 as power-gpio and
> GPIOV3 as power-switch-gpio.
> 
> Signed-off-by: Steven Lee <steven_lee@aspeedtech.com>
> ---
>  .../devicetree/bindings/mmc/aspeed,sdhci.yaml | 99 +++++++++++++++++++
>  1 file changed, 99 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/mmc/aspeed,sdhci.yaml 
> b/Documentation/devicetree/bindings/mmc/aspeed,sdhci.yaml
> index 987b287f3bff..dd894aba0bb7 100644
> --- a/Documentation/devicetree/bindings/mmc/aspeed,sdhci.yaml
> +++ b/Documentation/devicetree/bindings/mmc/aspeed,sdhci.yaml
> @@ -20,6 +20,19 @@ description: |+
>    the slots are dependent on the common configuration area, they are 
> described
>    as child nodes.
>  
> +  The signal voltage of SDHCIs on AST2600-A2 EVB is able to be toggled 
> by GPIO
> +  pins. In the reference design, GPIOV0 of AST2600-A2 EVB is connected 
> to the
> +  power load switch that providing 3.3v to SD1 bus vdd, GPIOV1 is 
> connected to
> +  a 1.8v and a 3.3v power load switch that providing signal voltage to
> +  SD1 bus.
> +  If GPIOV0 is active high, SD1 bus is enabled. Otherwise, SD1 bus is
> +  disabled. If GPIOV1 is active high, 3.3v power load switch is 
> enabled, SD1
> +  signal voltage is 3.3v. Otherwise, 1.8v power load switch will be 
> enabled, SD1
> +  signal voltage becomes 1.8v.
> +  AST2600-A2 EVB also support toggling signal voltage for SD2 bus.
> +  The design is the same as SD1 bus. It uses GPIOV2 as power-gpio and 
> GPIOV3
> +  as power-switch-gpio.

I don't think we should be describing design-specific details in the 
binding document. However, I think this would be a great comment in the 
AST2600 EVB devicetree. Can you please move it there?

> +
>  properties:
>    compatible:
>      enum:
> @@ -78,6 +91,7 @@ required:
>    - clocks
>  
>  examples:
> +  //Example 1
>    - |
>      #include <dt-bindings/clock/aspeed-clock.h>
>      sdc@1e740000 {
> @@ -104,3 +118,88 @@ examples:
>                      clocks = <&syscon ASPEED_CLK_SDIO>;
>              };
>      };
> +
> +  //Example 2 (AST2600EVB with GPIO regulator)

I feel you didn't test this with `make dt_binding_check` as `//` isn't
a valid YAML comment token. You need to use `#` for comments (
https://yaml.org/spec/1.2/spec.html#id2780069 ).

> +  - |
> +    #include <dt-bindings/clock/aspeed-clock.h>
> +    #include <dt-bindings/gpio/aspeed-gpio.h>
> +    vcc_sdhci0: regulator-vcc-sdhci0 {
> +            compatible = "regulator-fixed";
> +
> +            regulator-name = "SDHCI0 Vcc";
> +            regulator-min-microvolt = <3300000>;
> +            regulator-max-microvolt = <3300000>;
> +            gpios = <&gpio0 ASPEED_GPIO(V, 0)
> +                            GPIO_ACTIVE_HIGH>;
> +            enable-active-high;
> +    };
> +
> +    vccq_sdhci0: regulator-vccq-sdhci0 {
> +            compatible = "regulator-gpio";
> +
> +            regulator-name = "SDHCI0 VccQ";
> +            regulator-min-microvolt = <1800000>;
> +            regulator-max-microvolt = <3300000>;
> +            gpios = <&gpio0 ASPEED_GPIO(V, 1)
> +                            GPIO_ACTIVE_HIGH>;
> +            gpios-states = <1>;
> +            states = <3300000 1
> +                      1800000 0>;
> +    };
> +
> +    vcc_sdhci1: regulator-vcc-sdhci1 {
> +            compatible = "regulator-fixed";
> +
> +            regulator-name = "SDHCI1 Vcc";
> +            regulator-min-microvolt = <3300000>;
> +            regulator-max-microvolt = <3300000>;
> +            gpios = <&gpio0 ASPEED_GPIO(V, 2)
> +                            GPIO_ACTIVE_HIGH>;
> +            enable-active-high;
> +    };
> +
> +    vccq_sdhci1: regulator-vccq-sdhci1 {
> +            compatible = "regulator-gpio";
> +
> +            regulator-name = "SDHCI1 VccQ";
> +            regulator-min-microvolt = <1800000>;
> +            regulator-max-microvolt = <3300000>;
> +            gpios = <&gpio0 ASPEED_GPIO(V, 3)
> +                            GPIO_ACTIVE_HIGH>;
> +            gpios-states = <1>;
> +            states = <3300000 1
> +                      1800000 0>;
> +    };
> +
> +    sdc@1e740000 {
> +            compatible = "aspeed,ast2600-sd-controller";
> +            reg = <0x1e740000 0x100>;
> +            #address-cells = <1>;
> +            #size-cells = <1>;
> +            ranges = <0 0x1e740000 0x20000>;
> +            clocks = <&syscon ASPEED_CLK_GATE_SDCLK>;
> +
> +            sdhci0: sdhci@100 {
> +                    compatible = "aspeed,ast2600-sdhci", "sdhci";
> +                    reg = <0x100 0x100>;
> +                    interrupts = <GIC_SPI 43 IRQ_TYPE_LEVEL_HIGH>;
> +                    sdhci,auto-cmd12;
> +                    clocks = <&syscon ASPEED_CLK_SDIO>;
> +                    vmmc-supply = <&vcc_sdhci0>;
> +                    vqmmc-supply = <&vccq_sdhci0>;
> +                    sd-uhs-sdr104;
> +                    clk-phase-uhs-sdr104 = <180>, <180>;
> +            };
> +
> +            sdhci1: sdhci@200 {
> +                    compatible = "aspeed,ast2600-sdhci", "sdhci";
> +                    reg = <0x200 0x100>;
> +                    interrupts = <GIC_SPI 43 IRQ_TYPE_LEVEL_HIGH>;
> +                    sdhci,auto-cmd12;
> +                    clocks = <&syscon ASPEED_CLK_SDIO>;
> +                    vmmc-supply = <&vcc_sdhci1>;
> +                    vqmmc-supply = <&vccq_sdhci1>;
> +                    sd-uhs-sdr104;
> +                    clk-phase-uhs-sdr104 = <0>, <0>;
> +            };
> +    };

This is a good example, so can we keep this and just drop the comment 
from the binding document?

Cheers,

Andrew

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v2 1/3] dt-bindings: mmc: sdhci-of-aspeed: Add description for AST2600 EVB.

[Steven Lee]

The 05/03/2021 12:19, Andrew Jeffery wrote:
> Hi Steven,
> 
> On Mon, 3 May 2021, at 11:13, Steven Lee wrote:
> > Add the description for describing the AST 2600 EVB reference design of
> > GPIO regulators and provide the example in the document.
> > 
> > AST2600-A2 EVB has the reference design for enabling SD bus
> > power and toggling SD bus signal voltage by GPIO pins.
> > 
> > In the reference design, GPIOV0 of AST2600-A2 EVB is connected to
> > power load switch that providing 3.3v to SD1 bus vdd. GPIOV1 is
> > connected to a 1.8v and a 3.3v power load switch that providing
> > signal voltage to
> > SD1 bus.
> > 
> > If GPIOV0 is active high, SD1 bus is enabled. Otherwise, SD1 bus is
> > disabled.
> > If GPIOV1 is active high, 3.3v power load switch is enabled, SD1
> > signal voltage is 3.3v. Otherwise, 1.8v power load switch will be
> > enabled, SD1 signal voltage becomes 1.8v.
> > 
> > AST2600-A2 EVB also support toggling signal voltage for SD2 bus.
> > The design is the same as SD1 bus. It uses GPIOV2 as power-gpio and
> > GPIOV3 as power-switch-gpio.
> > 
> > Signed-off-by: Steven Lee <steven_lee@aspeedtech.com>
> > ---
> >  .../devicetree/bindings/mmc/aspeed,sdhci.yaml | 99 +++++++++++++++++++
> >  1 file changed, 99 insertions(+)
> > 
> > diff --git a/Documentation/devicetree/bindings/mmc/aspeed,sdhci.yaml 
> > b/Documentation/devicetree/bindings/mmc/aspeed,sdhci.yaml
> > index 987b287f3bff..dd894aba0bb7 100644
> > --- a/Documentation/devicetree/bindings/mmc/aspeed,sdhci.yaml
> > +++ b/Documentation/devicetree/bindings/mmc/aspeed,sdhci.yaml
> > @@ -20,6 +20,19 @@ description: |+
> >    the slots are dependent on the common configuration area, they are 
> > described
> >    as child nodes.
> >  
> > +  The signal voltage of SDHCIs on AST2600-A2 EVB is able to be toggled 
> > by GPIO
> > +  pins. In the reference design, GPIOV0 of AST2600-A2 EVB is connected 
> > to the
> > +  power load switch that providing 3.3v to SD1 bus vdd, GPIOV1 is 
> > connected to
> > +  a 1.8v and a 3.3v power load switch that providing signal voltage to
> > +  SD1 bus.
> > +  If GPIOV0 is active high, SD1 bus is enabled. Otherwise, SD1 bus is
> > +  disabled. If GPIOV1 is active high, 3.3v power load switch is 
> > enabled, SD1
> > +  signal voltage is 3.3v. Otherwise, 1.8v power load switch will be 
> > enabled, SD1
> > +  signal voltage becomes 1.8v.
> > +  AST2600-A2 EVB also support toggling signal voltage for SD2 bus.
> > +  The design is the same as SD1 bus. It uses GPIOV2 as power-gpio and 
> > GPIOV3
> > +  as power-switch-gpio.
> 
> I don't think we should be describing design-specific details in the 
> binding document. However, I think this would be a great comment in the 
> AST2600 EVB devicetree. Can you please move it there?
> 

Ok, I will move it to the device tree.

I was wondering if the following place is a good place to put the
comment

at line 534 of aspeed-g6.dtsi
sdc: sdc@1e740000 {
	// Comment here...

	compatible = "aspeed,ast2600-sd-controller";
	reg = <0x1e740000 0x100>;

	sdhci0: sdhci@1e740100 {
		compatible = "aspeed,ast2600-sdhci", "sdhci";
		reg = <0x100 0x100>;
		interrupts = <GIC_SPI 43 IRQ_TYPE_LEVEL_HIGH>;
...
}

> > +
> >  properties:
> >    compatible:
> >      enum:
> > @@ -78,6 +91,7 @@ required:
> >    - clocks
> >  
> >  examples:
> > +  //Example 1
> >    - |
> >      #include <dt-bindings/clock/aspeed-clock.h>
> >      sdc@1e740000 {
> > @@ -104,3 +118,88 @@ examples:
> >                      clocks = <&syscon ASPEED_CLK_SDIO>;
> >              };
> >      };
> > +
> > +  //Example 2 (AST2600EVB with GPIO regulator)
> 
> I feel you didn't test this with `make dt_binding_check` as `//` isn't
> a valid YAML comment token. You need to use `#` for comments (
> https://yaml.org/spec/1.2/spec.html#id2780069 ).
> 

Sorry, I don't know that there is a binding check command for valiating
YAML document.
Regardless, thanks for the reference link.
I will test with dt_binding_check.

> > +  - |
> > +    #include <dt-bindings/clock/aspeed-clock.h>
> > +    #include <dt-bindings/gpio/aspeed-gpio.h>
> > +    vcc_sdhci0: regulator-vcc-sdhci0 {
> > +            compatible = "regulator-fixed";
> > +
> > +            regulator-name = "SDHCI0 Vcc";
> > +            regulator-min-microvolt = <3300000>;
> > +            regulator-max-microvolt = <3300000>;
> > +            gpios = <&gpio0 ASPEED_GPIO(V, 0)
> > +                            GPIO_ACTIVE_HIGH>;
> > +            enable-active-high;
> > +    };
> > +
> > +    vccq_sdhci0: regulator-vccq-sdhci0 {
> > +            compatible = "regulator-gpio";
> > +
> > +            regulator-name = "SDHCI0 VccQ";
> > +            regulator-min-microvolt = <1800000>;
> > +            regulator-max-microvolt = <3300000>;
> > +            gpios = <&gpio0 ASPEED_GPIO(V, 1)
> > +                            GPIO_ACTIVE_HIGH>;
> > +            gpios-states = <1>;
> > +            states = <3300000 1
> > +                      1800000 0>;
> > +    };
> > +
> > +    vcc_sdhci1: regulator-vcc-sdhci1 {
> > +            compatible = "regulator-fixed";
> > +
> > +            regulator-name = "SDHCI1 Vcc";
> > +            regulator-min-microvolt = <3300000>;
> > +            regulator-max-microvolt = <3300000>;
> > +            gpios = <&gpio0 ASPEED_GPIO(V, 2)
> > +                            GPIO_ACTIVE_HIGH>;
> > +            enable-active-high;
> > +    };
> > +
> > +    vccq_sdhci1: regulator-vccq-sdhci1 {
> > +            compatible = "regulator-gpio";
> > +
> > +            regulator-name = "SDHCI1 VccQ";
> > +            regulator-min-microvolt = <1800000>;
> > +            regulator-max-microvolt = <3300000>;
> > +            gpios = <&gpio0 ASPEED_GPIO(V, 3)
> > +                            GPIO_ACTIVE_HIGH>;
> > +            gpios-states = <1>;
> > +            states = <3300000 1
> > +                      1800000 0>;
> > +    };
> > +
> > +    sdc@1e740000 {
> > +            compatible = "aspeed,ast2600-sd-controller";
> > +            reg = <0x1e740000 0x100>;
> > +            #address-cells = <1>;
> > +            #size-cells = <1>;
> > +            ranges = <0 0x1e740000 0x20000>;
> > +            clocks = <&syscon ASPEED_CLK_GATE_SDCLK>;
> > +
> > +            sdhci0: sdhci@100 {
> > +                    compatible = "aspeed,ast2600-sdhci", "sdhci";
> > +                    reg = <0x100 0x100>;
> > +                    interrupts = <GIC_SPI 43 IRQ_TYPE_LEVEL_HIGH>;
> > +                    sdhci,auto-cmd12;
> > +                    clocks = <&syscon ASPEED_CLK_SDIO>;
> > +                    vmmc-supply = <&vcc_sdhci0>;
> > +                    vqmmc-supply = <&vccq_sdhci0>;
> > +                    sd-uhs-sdr104;
> > +                    clk-phase-uhs-sdr104 = <180>, <180>;
> > +            };
> > +
> > +            sdhci1: sdhci@200 {
> > +                    compatible = "aspeed,ast2600-sdhci", "sdhci";
> > +                    reg = <0x200 0x100>;
> > +                    interrupts = <GIC_SPI 43 IRQ_TYPE_LEVEL_HIGH>;
> > +                    sdhci,auto-cmd12;
> > +                    clocks = <&syscon ASPEED_CLK_SDIO>;
> > +                    vmmc-supply = <&vcc_sdhci1>;
> > +                    vqmmc-supply = <&vccq_sdhci1>;
> > +                    sd-uhs-sdr104;
> > +                    clk-phase-uhs-sdr104 = <0>, <0>;
> > +            };
> > +    };
> 
> This is a good example, so can we keep this and just drop the comment 
> from the binding document?

Ok, I will remove the comment.

> 
> Cheers,
> 
> Andrew

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v2 1/3] dt-bindings: mmc: sdhci-of-aspeed: Add description for AST2600 EVB.

[Andrew Jeffery]

On Mon, 3 May 2021, at 19:10, Steven Lee wrote:
> The 05/03/2021 12:19, Andrew Jeffery wrote:
> > Hi Steven,
> > 
> > On Mon, 3 May 2021, at 11:13, Steven Lee wrote:
> > > Add the description for describing the AST 2600 EVB reference design of
> > > GPIO regulators and provide the example in the document.
> > > 
> > > AST2600-A2 EVB has the reference design for enabling SD bus
> > > power and toggling SD bus signal voltage by GPIO pins.
> > > 
> > > In the reference design, GPIOV0 of AST2600-A2 EVB is connected to
> > > power load switch that providing 3.3v to SD1 bus vdd. GPIOV1 is
> > > connected to a 1.8v and a 3.3v power load switch that providing
> > > signal voltage to
> > > SD1 bus.
> > > 
> > > If GPIOV0 is active high, SD1 bus is enabled. Otherwise, SD1 bus is
> > > disabled.
> > > If GPIOV1 is active high, 3.3v power load switch is enabled, SD1
> > > signal voltage is 3.3v. Otherwise, 1.8v power load switch will be
> > > enabled, SD1 signal voltage becomes 1.8v.
> > > 
> > > AST2600-A2 EVB also support toggling signal voltage for SD2 bus.
> > > The design is the same as SD1 bus. It uses GPIOV2 as power-gpio and
> > > GPIOV3 as power-switch-gpio.
> > > 
> > > Signed-off-by: Steven Lee <steven_lee@aspeedtech.com>
> > > ---
> > >  .../devicetree/bindings/mmc/aspeed,sdhci.yaml | 99 +++++++++++++++++++
> > >  1 file changed, 99 insertions(+)
> > > 
> > > diff --git a/Documentation/devicetree/bindings/mmc/aspeed,sdhci.yaml 
> > > b/Documentation/devicetree/bindings/mmc/aspeed,sdhci.yaml
> > > index 987b287f3bff..dd894aba0bb7 100644
> > > --- a/Documentation/devicetree/bindings/mmc/aspeed,sdhci.yaml
> > > +++ b/Documentation/devicetree/bindings/mmc/aspeed,sdhci.yaml
> > > @@ -20,6 +20,19 @@ description: |+
> > >    the slots are dependent on the common configuration area, they are 
> > > described
> > >    as child nodes.
> > >  
> > > +  The signal voltage of SDHCIs on AST2600-A2 EVB is able to be toggled 
> > > by GPIO
> > > +  pins. In the reference design, GPIOV0 of AST2600-A2 EVB is connected 
> > > to the
> > > +  power load switch that providing 3.3v to SD1 bus vdd, GPIOV1 is 
> > > connected to
> > > +  a 1.8v and a 3.3v power load switch that providing signal voltage to
> > > +  SD1 bus.
> > > +  If GPIOV0 is active high, SD1 bus is enabled. Otherwise, SD1 bus is
> > > +  disabled. If GPIOV1 is active high, 3.3v power load switch is 
> > > enabled, SD1
> > > +  signal voltage is 3.3v. Otherwise, 1.8v power load switch will be 
> > > enabled, SD1
> > > +  signal voltage becomes 1.8v.
> > > +  AST2600-A2 EVB also support toggling signal voltage for SD2 bus.
> > > +  The design is the same as SD1 bus. It uses GPIOV2 as power-gpio and 
> > > GPIOV3
> > > +  as power-switch-gpio.
> > 
> > I don't think we should be describing design-specific details in the 
> > binding document. However, I think this would be a great comment in the 
> > AST2600 EVB devicetree. Can you please move it there?
> > 
> 
> Ok, I will move it to the device tree.
> 
> I was wondering if the following place is a good place to put the
> comment
> 
> at line 534 of aspeed-g6.dtsi

What you're describing is specific to the AST2600 EVB, so I suggest you 
put it in the EVB dts, e.g. at:

https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/arch/arm/boot/dts/aspeed-ast2600-evb.dts#n103

> sdc: sdc@1e740000 {
> 	// Comment here...
> 
> 	compatible = "aspeed,ast2600-sd-controller";
> 	reg = <0x1e740000 0x100>;
> 
> 	sdhci0: sdhci@1e740100 {
> 		compatible = "aspeed,ast2600-sdhci", "sdhci";
> 		reg = <0x100 0x100>;
> 		interrupts = <GIC_SPI 43 IRQ_TYPE_LEVEL_HIGH>;
> ...
> }
> 
> > > +
> > >  properties:
> > >    compatible:
> > >      enum:
> > > @@ -78,6 +91,7 @@ required:
> > >    - clocks
> > >  
> > >  examples:
> > > +  //Example 1
> > >    - |
> > >      #include <dt-bindings/clock/aspeed-clock.h>
> > >      sdc@1e740000 {
> > > @@ -104,3 +118,88 @@ examples:
> > >                      clocks = <&syscon ASPEED_CLK_SDIO>;
> > >              };
> > >      };
> > > +
> > > +  //Example 2 (AST2600EVB with GPIO regulator)
> > 
> > I feel you didn't test this with `make dt_binding_check` as `//` isn't
> > a valid YAML comment token. You need to use `#` for comments (
> > https://yaml.org/spec/1.2/spec.html#id2780069 ).
> > 
> 
> Sorry, I don't know that there is a binding check command for valiating
> YAML document.

No worries! There's also `make dtbs_check` to validate the devicetree files
against the bindings. It's useful to run both, as usually when you're adding to
the binding you're modifying a devicetree as well.

Unfortunately we need to do a bit of a cleanup of the Aspeed dts files, they
generate a number of warnings right now.

> Regardless, thanks for the reference link.
> I will test with dt_binding_check.
> 
> > > +  - |
> > > +    #include <dt-bindings/clock/aspeed-clock.h>
> > > +    #include <dt-bindings/gpio/aspeed-gpio.h>
> > > +    vcc_sdhci0: regulator-vcc-sdhci0 {
> > > +            compatible = "regulator-fixed";
> > > +
> > > +            regulator-name = "SDHCI0 Vcc";
> > > +            regulator-min-microvolt = <3300000>;
> > > +            regulator-max-microvolt = <3300000>;
> > > +            gpios = <&gpio0 ASPEED_GPIO(V, 0)
> > > +                            GPIO_ACTIVE_HIGH>;
> > > +            enable-active-high;
> > > +    };
> > > +
> > > +    vccq_sdhci0: regulator-vccq-sdhci0 {
> > > +            compatible = "regulator-gpio";
> > > +
> > > +            regulator-name = "SDHCI0 VccQ";
> > > +            regulator-min-microvolt = <1800000>;
> > > +            regulator-max-microvolt = <3300000>;
> > > +            gpios = <&gpio0 ASPEED_GPIO(V, 1)
> > > +                            GPIO_ACTIVE_HIGH>;
> > > +            gpios-states = <1>;
> > > +            states = <3300000 1
> > > +                      1800000 0>;
> > > +    };
> > > +
> > > +    vcc_sdhci1: regulator-vcc-sdhci1 {
> > > +            compatible = "regulator-fixed";
> > > +
> > > +            regulator-name = "SDHCI1 Vcc";
> > > +            regulator-min-microvolt = <3300000>;
> > > +            regulator-max-microvolt = <3300000>;
> > > +            gpios = <&gpio0 ASPEED_GPIO(V, 2)
> > > +                            GPIO_ACTIVE_HIGH>;
> > > +            enable-active-high;
> > > +    };
> > > +
> > > +    vccq_sdhci1: regulator-vccq-sdhci1 {
> > > +            compatible = "regulator-gpio";
> > > +
> > > +            regulator-name = "SDHCI1 VccQ";
> > > +            regulator-min-microvolt = <1800000>;
> > > +            regulator-max-microvolt = <3300000>;
> > > +            gpios = <&gpio0 ASPEED_GPIO(V, 3)
> > > +                            GPIO_ACTIVE_HIGH>;
> > > +            gpios-states = <1>;
> > > +            states = <3300000 1
> > > +                      1800000 0>;
> > > +    };
> > > +
> > > +    sdc@1e740000 {
> > > +            compatible = "aspeed,ast2600-sd-controller";
> > > +            reg = <0x1e740000 0x100>;
> > > +            #address-cells = <1>;
> > > +            #size-cells = <1>;
> > > +            ranges = <0 0x1e740000 0x20000>;
> > > +            clocks = <&syscon ASPEED_CLK_GATE_SDCLK>;
> > > +
> > > +            sdhci0: sdhci@100 {
> > > +                    compatible = "aspeed,ast2600-sdhci", "sdhci";
> > > +                    reg = <0x100 0x100>;
> > > +                    interrupts = <GIC_SPI 43 IRQ_TYPE_LEVEL_HIGH>;
> > > +                    sdhci,auto-cmd12;
> > > +                    clocks = <&syscon ASPEED_CLK_SDIO>;
> > > +                    vmmc-supply = <&vcc_sdhci0>;
> > > +                    vqmmc-supply = <&vccq_sdhci0>;
> > > +                    sd-uhs-sdr104;
> > > +                    clk-phase-uhs-sdr104 = <180>, <180>;
> > > +            };
> > > +
> > > +            sdhci1: sdhci@200 {
> > > +                    compatible = "aspeed,ast2600-sdhci", "sdhci";
> > > +                    reg = <0x200 0x100>;
> > > +                    interrupts = <GIC_SPI 43 IRQ_TYPE_LEVEL_HIGH>;
> > > +                    sdhci,auto-cmd12;
> > > +                    clocks = <&syscon ASPEED_CLK_SDIO>;
> > > +                    vmmc-supply = <&vcc_sdhci1>;
> > > +                    vqmmc-supply = <&vccq_sdhci1>;
> > > +                    sd-uhs-sdr104;
> > > +                    clk-phase-uhs-sdr104 = <0>, <0>;
> > > +            };
> > > +    };
> > 
> > This is a good example, so can we keep this and just drop the comment 
> > from the binding document?
> 
> Ok, I will remove the comment.

Thanks.

Andrew

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v2 1/3] dt-bindings: mmc: sdhci-of-aspeed: Add description for AST2600 EVB.

[Steven Lee]

The 05/03/2021 18:32, Andrew Jeffery wrote:
> On Mon, 3 May 2021, at 19:10, Steven Lee wrote:
> > The 05/03/2021 12:19, Andrew Jeffery wrote:
> > > Hi Steven,
> > > 
> > > On Mon, 3 May 2021, at 11:13, Steven Lee wrote:
> > > > Add the description for describing the AST 2600 EVB reference design of
> > > > GPIO regulators and provide the example in the document.
> > > > 
> > > > AST2600-A2 EVB has the reference design for enabling SD bus
> > > > power and toggling SD bus signal voltage by GPIO pins.
> > > > 
> > > > In the reference design, GPIOV0 of AST2600-A2 EVB is connected to
> > > > power load switch that providing 3.3v to SD1 bus vdd. GPIOV1 is
> > > > connected to a 1.8v and a 3.3v power load switch that providing
> > > > signal voltage to
> > > > SD1 bus.
> > > > 
> > > > If GPIOV0 is active high, SD1 bus is enabled. Otherwise, SD1 bus is
> > > > disabled.
> > > > If GPIOV1 is active high, 3.3v power load switch is enabled, SD1
> > > > signal voltage is 3.3v. Otherwise, 1.8v power load switch will be
> > > > enabled, SD1 signal voltage becomes 1.8v.
> > > > 
> > > > AST2600-A2 EVB also support toggling signal voltage for SD2 bus.
> > > > The design is the same as SD1 bus. It uses GPIOV2 as power-gpio and
> > > > GPIOV3 as power-switch-gpio.
> > > > 
> > > > Signed-off-by: Steven Lee <steven_lee@aspeedtech.com>
> > > > ---
> > > >  .../devicetree/bindings/mmc/aspeed,sdhci.yaml | 99 +++++++++++++++++++
> > > >  1 file changed, 99 insertions(+)
> > > > 
> > > > diff --git a/Documentation/devicetree/bindings/mmc/aspeed,sdhci.yaml 
> > > > b/Documentation/devicetree/bindings/mmc/aspeed,sdhci.yaml
> > > > index 987b287f3bff..dd894aba0bb7 100644
> > > > --- a/Documentation/devicetree/bindings/mmc/aspeed,sdhci.yaml
> > > > +++ b/Documentation/devicetree/bindings/mmc/aspeed,sdhci.yaml
> > > > @@ -20,6 +20,19 @@ description: |+
> > > >    the slots are dependent on the common configuration area, they are 
> > > > described
> > > >    as child nodes.
> > > >  
> > > > +  The signal voltage of SDHCIs on AST2600-A2 EVB is able to be toggled 
> > > > by GPIO
> > > > +  pins. In the reference design, GPIOV0 of AST2600-A2 EVB is connected 
> > > > to the
> > > > +  power load switch that providing 3.3v to SD1 bus vdd, GPIOV1 is 
> > > > connected to
> > > > +  a 1.8v and a 3.3v power load switch that providing signal voltage to
> > > > +  SD1 bus.
> > > > +  If GPIOV0 is active high, SD1 bus is enabled. Otherwise, SD1 bus is
> > > > +  disabled. If GPIOV1 is active high, 3.3v power load switch is 
> > > > enabled, SD1
> > > > +  signal voltage is 3.3v. Otherwise, 1.8v power load switch will be 
> > > > enabled, SD1
> > > > +  signal voltage becomes 1.8v.
> > > > +  AST2600-A2 EVB also support toggling signal voltage for SD2 bus.
> > > > +  The design is the same as SD1 bus. It uses GPIOV2 as power-gpio and 
> > > > GPIOV3
> > > > +  as power-switch-gpio.
> > > 
> > > I don't think we should be describing design-specific details in the 
> > > binding document. However, I think this would be a great comment in the 
> > > AST2600 EVB devicetree. Can you please move it there?
> > > 
> > 
> > Ok, I will move it to the device tree.
> > 
> > I was wondering if the following place is a good place to put the
> > comment
> > 
> > at line 534 of aspeed-g6.dtsi
> 
> What you're describing is specific to the AST2600 EVB, so I suggest you 
> put it in the EVB dts, e.g. at:
> 
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/arch/arm/boot/dts/aspeed-ast2600-evb.dts#n103
> 

Got it!
Thanks for the prompt reply.

> > sdc: sdc@1e740000 {
> > 	// Comment here...
> > 
> > 	compatible = "aspeed,ast2600-sd-controller";
> > 	reg = <0x1e740000 0x100>;
> > 
> > 	sdhci0: sdhci@1e740100 {
> > 		compatible = "aspeed,ast2600-sdhci", "sdhci";
> > 		reg = <0x100 0x100>;
> > 		interrupts = <GIC_SPI 43 IRQ_TYPE_LEVEL_HIGH>;
> > ...
> > }
> > 
> > > > +
> > > >  properties:
> > > >    compatible:
> > > >      enum:
> > > > @@ -78,6 +91,7 @@ required:
> > > >    - clocks
> > > >  
> > > >  examples:
> > > > +  //Example 1
> > > >    - |
> > > >      #include <dt-bindings/clock/aspeed-clock.h>
> > > >      sdc@1e740000 {
> > > > @@ -104,3 +118,88 @@ examples:
> > > >                      clocks = <&syscon ASPEED_CLK_SDIO>;
> > > >              };
> > > >      };
> > > > +
> > > > +  //Example 2 (AST2600EVB with GPIO regulator)
> > > 
> > > I feel you didn't test this with `make dt_binding_check` as `//` isn't
> > > a valid YAML comment token. You need to use `#` for comments (
> > > https://yaml.org/spec/1.2/spec.html#id2780069 ).
> > > 
> > 
> > Sorry, I don't know that there is a binding check command for valiating
> > YAML document.
> 
> No worries! There's also `make dtbs_check` to validate the devicetree files
> against the bindings. It's useful to run both, as usually when you're adding to
> the binding you're modifying a devicetree as well.
> 

Thanks, I will to these checks.

> Unfortunately we need to do a bit of a cleanup of the Aspeed dts files, they
> generate a number of warnings right now.
> 
> > Regardless, thanks for the reference link.
> > I will test with dt_binding_check.
> > 
> > > > +  - |
> > > > +    #include <dt-bindings/clock/aspeed-clock.h>
> > > > +    #include <dt-bindings/gpio/aspeed-gpio.h>
> > > > +    vcc_sdhci0: regulator-vcc-sdhci0 {
> > > > +            compatible = "regulator-fixed";
> > > > +
> > > > +            regulator-name = "SDHCI0 Vcc";
> > > > +            regulator-min-microvolt = <3300000>;
> > > > +            regulator-max-microvolt = <3300000>;
> > > > +            gpios = <&gpio0 ASPEED_GPIO(V, 0)
> > > > +                            GPIO_ACTIVE_HIGH>;
> > > > +            enable-active-high;
> > > > +    };
> > > > +
> > > > +    vccq_sdhci0: regulator-vccq-sdhci0 {
> > > > +            compatible = "regulator-gpio";
> > > > +
> > > > +            regulator-name = "SDHCI0 VccQ";
> > > > +            regulator-min-microvolt = <1800000>;
> > > > +            regulator-max-microvolt = <3300000>;
> > > > +            gpios = <&gpio0 ASPEED_GPIO(V, 1)
> > > > +                            GPIO_ACTIVE_HIGH>;
> > > > +            gpios-states = <1>;
> > > > +            states = <3300000 1
> > > > +                      1800000 0>;
> > > > +    };
> > > > +
> > > > +    vcc_sdhci1: regulator-vcc-sdhci1 {
> > > > +            compatible = "regulator-fixed";
> > > > +
> > > > +            regulator-name = "SDHCI1 Vcc";
> > > > +            regulator-min-microvolt = <3300000>;
> > > > +            regulator-max-microvolt = <3300000>;
> > > > +            gpios = <&gpio0 ASPEED_GPIO(V, 2)
> > > > +                            GPIO_ACTIVE_HIGH>;
> > > > +            enable-active-high;
> > > > +    };
> > > > +
> > > > +    vccq_sdhci1: regulator-vccq-sdhci1 {
> > > > +            compatible = "regulator-gpio";
> > > > +
> > > > +            regulator-name = "SDHCI1 VccQ";
> > > > +            regulator-min-microvolt = <1800000>;
> > > > +            regulator-max-microvolt = <3300000>;
> > > > +            gpios = <&gpio0 ASPEED_GPIO(V, 3)
> > > > +                            GPIO_ACTIVE_HIGH>;
> > > > +            gpios-states = <1>;
> > > > +            states = <3300000 1
> > > > +                      1800000 0>;
> > > > +    };
> > > > +
> > > > +    sdc@1e740000 {
> > > > +            compatible = "aspeed,ast2600-sd-controller";
> > > > +            reg = <0x1e740000 0x100>;
> > > > +            #address-cells = <1>;
> > > > +            #size-cells = <1>;
> > > > +            ranges = <0 0x1e740000 0x20000>;
> > > > +            clocks = <&syscon ASPEED_CLK_GATE_SDCLK>;
> > > > +
> > > > +            sdhci0: sdhci@100 {
> > > > +                    compatible = "aspeed,ast2600-sdhci", "sdhci";
> > > > +                    reg = <0x100 0x100>;
> > > > +                    interrupts = <GIC_SPI 43 IRQ_TYPE_LEVEL_HIGH>;
> > > > +                    sdhci,auto-cmd12;
> > > > +                    clocks = <&syscon ASPEED_CLK_SDIO>;
> > > > +                    vmmc-supply = <&vcc_sdhci0>;
> > > > +                    vqmmc-supply = <&vccq_sdhci0>;
> > > > +                    sd-uhs-sdr104;
> > > > +                    clk-phase-uhs-sdr104 = <180>, <180>;
> > > > +            };
> > > > +
> > > > +            sdhci1: sdhci@200 {
> > > > +                    compatible = "aspeed,ast2600-sdhci", "sdhci";
> > > > +                    reg = <0x200 0x100>;
> > > > +                    interrupts = <GIC_SPI 43 IRQ_TYPE_LEVEL_HIGH>;
> > > > +                    sdhci,auto-cmd12;
> > > > +                    clocks = <&syscon ASPEED_CLK_SDIO>;
> > > > +                    vmmc-supply = <&vcc_sdhci1>;
> > > > +                    vqmmc-supply = <&vccq_sdhci1>;
> > > > +                    sd-uhs-sdr104;
> > > > +                    clk-phase-uhs-sdr104 = <0>, <0>;
> > > > +            };
> > > > +    };
> > > 
> > > This is a good example, so can we keep this and just drop the comment 
> > > from the binding document?
> > 
> > Ok, I will remove the comment.
> 
> Thanks.
> 
> Andrew

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v2 1/3] dt-bindings: mmc: sdhci-of-aspeed: Add description for AST2600 EVB.

[Rob Herring]

On Mon, 03 May 2021 09:43:34 +0800, Steven Lee wrote:
> Add the description for describing the AST 2600 EVB reference design of
> GPIO regulators and provide the example in the document.
> 
> AST2600-A2 EVB has the reference design for enabling SD bus
> power and toggling SD bus signal voltage by GPIO pins.
> 
> In the reference design, GPIOV0 of AST2600-A2 EVB is connected to
> power load switch that providing 3.3v to SD1 bus vdd. GPIOV1 is
> connected to a 1.8v and a 3.3v power load switch that providing
> signal voltage to
> SD1 bus.
> 
> If GPIOV0 is active high, SD1 bus is enabled. Otherwise, SD1 bus is
> disabled.
> If GPIOV1 is active high, 3.3v power load switch is enabled, SD1
> signal voltage is 3.3v. Otherwise, 1.8v power load switch will be
> enabled, SD1 signal voltage becomes 1.8v.
> 
> AST2600-A2 EVB also support toggling signal voltage for SD2 bus.
> The design is the same as SD1 bus. It uses GPIOV2 as power-gpio and
> GPIOV3 as power-switch-gpio.
> 
> Signed-off-by: Steven Lee <steven_lee@aspeedtech.com>
> ---
>  .../devicetree/bindings/mmc/aspeed,sdhci.yaml | 99 +++++++++++++++++++
>  1 file changed, 99 insertions(+)
> 

My bot found errors running 'make DT_CHECKER_FLAGS=-m dt_binding_check'
on your patch (DT_CHECKER_FLAGS is new in v5.13):

yamllint warnings/errors:
./Documentation/devicetree/bindings/mmc/aspeed,sdhci.yaml:97:5: [error] syntax error: expected <block end>, but found '<scalar>' (syntax)

dtschema/dtc warnings/errors:
make[1]: *** Deleting file 'Documentation/devicetree/bindings/mmc/aspeed,sdhci.example.dts'
Traceback (most recent call last):
  File "/usr/local/bin/dt-extract-example", line 45, in <module>
    binding = yaml.load(open(args.yamlfile, encoding='utf-8').read())
  File "/usr/local/lib/python3.8/dist-packages/ruamel/yaml/main.py", line 421, in load
    return constructor.get_single_data()
  File "/usr/local/lib/python3.8/dist-packages/ruamel/yaml/constructor.py", line 109, in get_single_data
    node = self.composer.get_single_node()
  File "_ruamel_yaml.pyx", line 706, in _ruamel_yaml.CParser.get_single_node
  File "_ruamel_yaml.pyx", line 724, in _ruamel_yaml.CParser._compose_document
  File "_ruamel_yaml.pyx", line 775, in _ruamel_yaml.CParser._compose_node
  File "_ruamel_yaml.pyx", line 891, in _ruamel_yaml.CParser._compose_mapping_node
  File "_ruamel_yaml.pyx", line 904, in _ruamel_yaml.CParser._parse_next_event
ruamel.yaml.parser.ParserError: while parsing a block mapping
  in "<unicode string>", line 5, column 1
did not find expected key
  in "<unicode string>", line 97, column 5
make[1]: *** [Documentation/devicetree/bindings/Makefile:20: Documentation/devicetree/bindings/mmc/aspeed,sdhci.example.dts] Error 1
make[1]: *** Waiting for unfinished jobs....
./Documentation/devicetree/bindings/mmc/aspeed,sdhci.yaml:  while parsing a block mapping
  in "<unicode string>", line 5, column 1
did not find expected key
  in "<unicode string>", line 97, column 5
/builds/robherring/linux-dt-review/Documentation/devicetree/bindings/mmc/aspeed,sdhci.yaml: ignoring, error parsing file
warning: no schema found in file: ./Documentation/devicetree/bindings/mmc/aspeed,sdhci.yaml
make: *** [Makefile:1414: dt_binding_check] Error 2

See https://patchwork.ozlabs.org/patch/1472993

This check can fail if there are any dependencies. The base for a patch
series is generally the most recent rc1.

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.


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v2 1/3] dt-bindings: mmc: sdhci-of-aspeed: Add description for AST2600 EVB.

[Steven Lee]

The 05/03/2021 23:20, Rob Herring wrote:
> On Mon, 03 May 2021 09:43:34 +0800, Steven Lee wrote:
> > Add the description for describing the AST 2600 EVB reference design of
> > GPIO regulators and provide the example in the document.
> > 
> > AST2600-A2 EVB has the reference design for enabling SD bus
> > power and toggling SD bus signal voltage by GPIO pins.
> > 
> > In the reference design, GPIOV0 of AST2600-A2 EVB is connected to
> > power load switch that providing 3.3v to SD1 bus vdd. GPIOV1 is
> > connected to a 1.8v and a 3.3v power load switch that providing
> > signal voltage to
> > SD1 bus.
> > 
> > If GPIOV0 is active high, SD1 bus is enabled. Otherwise, SD1 bus is
> > disabled.
> > If GPIOV1 is active high, 3.3v power load switch is enabled, SD1
> > signal voltage is 3.3v. Otherwise, 1.8v power load switch will be
> > enabled, SD1 signal voltage becomes 1.8v.
> > 
> > AST2600-A2 EVB also support toggling signal voltage for SD2 bus.
> > The design is the same as SD1 bus. It uses GPIOV2 as power-gpio and
> > GPIOV3 as power-switch-gpio.
> > 
> > Signed-off-by: Steven Lee <steven_lee@aspeedtech.com>
> > ---
> >  .../devicetree/bindings/mmc/aspeed,sdhci.yaml | 99 +++++++++++++++++++
> >  1 file changed, 99 insertions(+)
> > 
> 
> My bot found errors running 'make DT_CHECKER_FLAGS=-m dt_binding_check'
> on your patch (DT_CHECKER_FLAGS is new in v5.13):
> 
> yamllint warnings/errors:
> ./Documentation/devicetree/bindings/mmc/aspeed,sdhci.yaml:97:5: [error] syntax error: expected <block end>, but found '<scalar>' (syntax)
> 
> dtschema/dtc warnings/errors:
> make[1]: *** Deleting file 'Documentation/devicetree/bindings/mmc/aspeed,sdhci.example.dts'
> Traceback (most recent call last):
>   File "/usr/local/bin/dt-extract-example", line 45, in <module>
>     binding = yaml.load(open(args.yamlfile, encoding='utf-8').read())
>   File "/usr/local/lib/python3.8/dist-packages/ruamel/yaml/main.py", line 421, in load
>     return constructor.get_single_data()
>   File "/usr/local/lib/python3.8/dist-packages/ruamel/yaml/constructor.py", line 109, in get_single_data
>     node = self.composer.get_single_node()
>   File "_ruamel_yaml.pyx", line 706, in _ruamel_yaml.CParser.get_single_node
>   File "_ruamel_yaml.pyx", line 724, in _ruamel_yaml.CParser._compose_document
>   File "_ruamel_yaml.pyx", line 775, in _ruamel_yaml.CParser._compose_node
>   File "_ruamel_yaml.pyx", line 891, in _ruamel_yaml.CParser._compose_mapping_node
>   File "_ruamel_yaml.pyx", line 904, in _ruamel_yaml.CParser._parse_next_event
> ruamel.yaml.parser.ParserError: while parsing a block mapping
>   in "<unicode string>", line 5, column 1
> did not find expected key
>   in "<unicode string>", line 97, column 5
> make[1]: *** [Documentation/devicetree/bindings/Makefile:20: Documentation/devicetree/bindings/mmc/aspeed,sdhci.example.dts] Error 1
> make[1]: *** Waiting for unfinished jobs....
> ./Documentation/devicetree/bindings/mmc/aspeed,sdhci.yaml:  while parsing a block mapping
>   in "<unicode string>", line 5, column 1
> did not find expected key
>   in "<unicode string>", line 97, column 5
> /builds/robherring/linux-dt-review/Documentation/devicetree/bindings/mmc/aspeed,sdhci.yaml: ignoring, error parsing file
> warning: no schema found in file: ./Documentation/devicetree/bindings/mmc/aspeed,sdhci.yaml
> make: *** [Makefile:1414: dt_binding_check] Error 2
> 
> See https://patchwork.ozlabs.org/patch/1472993
> 
> This check can fail if there are any dependencies. The base for a patch
> series is generally the most recent rc1.
> 
> 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.
> 

Thanks for the log and the information, I will install the package
and do the check before re-submiting the patch.


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH v2 2/3] ARM: dts: aspeed: ast2600evb: Add timing-phase property for eMMC controller

[Steven Lee]

Set eMMC input clock phase to 3, which is more stable on AST2600 EVBs.

Signed-off-by: Steven Lee <steven_lee@aspeedtech.com>
---
 arch/arm/boot/dts/aspeed-ast2600-evb.dts | 1 +
 1 file changed, 1 insertion(+)

diff --git a/arch/arm/boot/dts/aspeed-ast2600-evb.dts b/arch/arm/boot/dts/aspeed-ast2600-evb.dts
index 2772796e215e..7a93317e27dc 100644
--- a/arch/arm/boot/dts/aspeed-ast2600-evb.dts
+++ b/arch/arm/boot/dts/aspeed-ast2600-evb.dts
@@ -102,6 +102,7 @@
 
 &emmc_controller {
 	status = "okay";
+	timing-phase = <0x300FF>;
 };
 
 &emmc {
-- 
2.17.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v2 2/3] ARM: dts: aspeed: ast2600evb: Add timing-phase property for eMMC controller

[Andrew Jeffery]

Hi Steven,

On Mon, 3 May 2021, at 11:13, Steven Lee wrote:
> Set eMMC input clock phase to 3, which is more stable on AST2600 EVBs.
> 
> Signed-off-by: Steven Lee <steven_lee@aspeedtech.com>
> ---
>  arch/arm/boot/dts/aspeed-ast2600-evb.dts | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/arch/arm/boot/dts/aspeed-ast2600-evb.dts 
> b/arch/arm/boot/dts/aspeed-ast2600-evb.dts
> index 2772796e215e..7a93317e27dc 100644
> --- a/arch/arm/boot/dts/aspeed-ast2600-evb.dts
> +++ b/arch/arm/boot/dts/aspeed-ast2600-evb.dts
> @@ -102,6 +102,7 @@
>  
>  &emmc_controller {
>  	status = "okay";
> +	timing-phase = <0x300FF>;

Please use the existing binding for phase corrections. The existing 
binding is already supported by the driver (added in v5.12).

Andrew

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v2 2/3] ARM: dts: aspeed: ast2600evb: Add timing-phase property for eMMC controller

[Steven Lee]

The 05/03/2021 13:07, Andrew Jeffery wrote:
> Hi Steven,
> 
> On Mon, 3 May 2021, at 11:13, Steven Lee wrote:
> > Set eMMC input clock phase to 3, which is more stable on AST2600 EVBs.
> > 
> > Signed-off-by: Steven Lee <steven_lee@aspeedtech.com>
> > ---
> >  arch/arm/boot/dts/aspeed-ast2600-evb.dts | 1 +
> >  1 file changed, 1 insertion(+)
> > 
> > diff --git a/arch/arm/boot/dts/aspeed-ast2600-evb.dts 
> > b/arch/arm/boot/dts/aspeed-ast2600-evb.dts
> > index 2772796e215e..7a93317e27dc 100644
> > --- a/arch/arm/boot/dts/aspeed-ast2600-evb.dts
> > +++ b/arch/arm/boot/dts/aspeed-ast2600-evb.dts
> > @@ -102,6 +102,7 @@
> >  
> >  &emmc_controller {
> >  	status = "okay";
> > +	timing-phase = <0x300FF>;
> 
> Please use the existing binding for phase corrections. The existing 
> binding is already supported by the driver (added in v5.12).
> 

Hi Andrew,

Thanks for the review.
I will add the following settings from aspeed-bmc-ibm-rainier.dts
instead of adding timing-phase in device tree.

        clk-phase-mmc-hs200 = <N> <N>;

> Andrew

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH v2 3/3] mmc: sdhci-of-aspeed: Sync capabilities from device tree to ast2600 SoC registers

[Steven Lee]

Sync Capbility Registers(SDIO140, SDIO144, SDIO240, SDIO244) of ast2600
SoC from the device tree.
The bit 26(Voltage Support 1.8v) of SDIO140/SDIO240 is set to 1 if
"mmc-hs200-1_8v" or "sd-uhs-sdr104" is added in the device tree.
The bit 1(SDR104 Supported) of SDR144/SDR244 is set to 1 if "sd-uhs-sdr104"
is added in the device tree.
"timing-phase" is synced to SDIO0F4(Colock Phase Control)

Signed-off-by: Steven Lee <steven_lee@aspeedtech.com>
---
 drivers/mmc/host/sdhci-of-aspeed.c | 107 ++++++++++++++++++++++++++---
 1 file changed, 98 insertions(+), 9 deletions(-)

diff --git a/drivers/mmc/host/sdhci-of-aspeed.c b/drivers/mmc/host/sdhci-of-aspeed.c
index 7d8692e90996..2d755bac777a 100644
--- a/drivers/mmc/host/sdhci-of-aspeed.c
+++ b/drivers/mmc/host/sdhci-of-aspeed.c
@@ -13,6 +13,7 @@
 #include <linux/of.h>
 #include <linux/of_platform.h>
 #include <linux/platform_device.h>
+#include <linux/reset.h>
 #include <linux/spinlock.h>
 
 #include "sdhci-pltfm.h"
@@ -30,10 +31,18 @@
 #define   ASPEED_SDC_S0_PHASE_IN_EN	BIT(2)
 #define   ASPEED_SDC_S0_PHASE_OUT_EN	GENMASK(1, 0)
 #define   ASPEED_SDC_PHASE_MAX		31
+#define ASPEED_SDC_CAP1_1_8V           BIT(26)
+#define ASPEED_SDC_CAP2_SDR104         BIT(1)
+#define PROBE_AFTER_ASSET_DEASSERT     0x1
+
+struct aspeed_sdc_info {
+	u32 flag;
+};
 
 struct aspeed_sdc {
 	struct clk *clk;
 	struct resource *res;
+	struct reset_control *rst;
 
 	spinlock_t lock;
 	void __iomem *regs;
@@ -72,6 +81,44 @@ struct aspeed_sdhci {
 	const struct aspeed_sdhci_phase_desc *phase_desc;
 };
 
+struct aspeed_sdc_info ast2600_sdc_info = {
+	.flag = PROBE_AFTER_ASSET_DEASSERT
+};
+
+/*
+ * The function sets the mirror register for updating
+ * capbilities of the current slot.
+ *
+ *   slot | cap_idx | caps_reg | mirror_reg
+ *   -----|---------|----------|------------
+ *     0  |    0    | SDIO140  |   SDIO10
+ *     0  |    1    | SDIO144  |   SDIO14
+ *     1  |    0    | SDIO240  |   SDIO20
+ *     1  |    1    | SDIO244  |   SDIO24
+ */
+static void aspeed_sdc_set_slot_capability(struct sdhci_host *host,
+					   struct aspeed_sdc *sdc,
+					   u32 reg_val,
+					   u8 slot,
+					   u8 cap_idx)
+{
+	u8 caps_reg_offset;
+	u32 caps_reg;
+	u32 mirror_reg_offset;
+	u32 caps_val;
+
+	if (cap_idx > 1 || slot > 1)
+		return;
+
+	caps_reg_offset = (cap_idx == 0) ? 0 : 4;
+	caps_reg = 0x40 + caps_reg_offset;
+	caps_val = sdhci_readl(host, caps_reg);
+	caps_val |= reg_val;
+	mirror_reg_offset = (slot == 0) ? 0x10 : 0x20;
+	mirror_reg_offset += caps_reg_offset;
+	writel(caps_val, sdc->regs + mirror_reg_offset);
+}
+
 static void aspeed_sdc_configure_8bit_mode(struct aspeed_sdc *sdc,
 					   struct aspeed_sdhci *sdhci,
 					   bool bus8)
@@ -329,9 +376,11 @@ static int aspeed_sdhci_probe(struct platform_device *pdev)
 {
 	const struct aspeed_sdhci_pdata *aspeed_pdata;
 	struct sdhci_pltfm_host *pltfm_host;
+	struct device_node *np = pdev->dev.of_node;
 	struct aspeed_sdhci *dev;
 	struct sdhci_host *host;
 	struct resource *res;
+	u32 reg_val;
 	int slot;
 	int ret;
 
@@ -372,6 +421,21 @@ static int aspeed_sdhci_probe(struct platform_device *pdev)
 
 	sdhci_get_of_property(pdev);
 
+	if (of_property_read_bool(np, "mmc-hs200-1_8v") ||
+	    of_property_read_bool(np, "sd-uhs-sdr104"))
+		aspeed_sdc_set_slot_capability(host,
+					       dev->parent,
+					       ASPEED_SDC_CAP1_1_8V,
+					       slot,
+					       0);
+
+	if (of_property_read_bool(np, "sd-uhs-sdr104"))
+		aspeed_sdc_set_slot_capability(host,
+					       dev->parent,
+					       ASPEED_SDC_CAP2_SDR104,
+					       slot,
+					       1);
+
 	pltfm_host->clk = devm_clk_get(&pdev->dev, NULL);
 	if (IS_ERR(pltfm_host->clk))
 		return PTR_ERR(pltfm_host->clk);
@@ -476,12 +540,25 @@ static struct platform_driver aspeed_sdhci_driver = {
 	.remove		= aspeed_sdhci_remove,
 };
 
+static const struct of_device_id aspeed_sdc_of_match[] = {
+	{ .compatible = "aspeed,ast2400-sd-controller", },
+	{ .compatible = "aspeed,ast2500-sd-controller", },
+	{ .compatible = "aspeed,ast2600-sd-controller", .data = &ast2600_sdc_info},
+	{ }
+};
+
+MODULE_DEVICE_TABLE(of, aspeed_sdc_of_match);
+
 static int aspeed_sdc_probe(struct platform_device *pdev)
 
 {
 	struct device_node *parent, *child;
 	struct aspeed_sdc *sdc;
+	const struct of_device_id *match = NULL;
+	const struct aspeed_sdc_info *info = NULL;
+
 	int ret;
+	u32 timing_phase;
 
 	sdc = devm_kzalloc(&pdev->dev, sizeof(*sdc), GFP_KERNEL);
 	if (!sdc)
@@ -489,6 +566,23 @@ static int aspeed_sdc_probe(struct platform_device *pdev)
 
 	spin_lock_init(&sdc->lock);
 
+	match = of_match_device(aspeed_sdc_of_match, &pdev->dev);
+	if (!match)
+		return -ENODEV;
+
+	if (match->data)
+		info = match->data;
+
+	if (info) {
+		if (info->flag & PROBE_AFTER_ASSET_DEASSERT) {
+			sdc->rst = devm_reset_control_get(&pdev->dev, NULL);
+			if (!IS_ERR(sdc->rst)) {
+				reset_control_assert(sdc->rst);
+				reset_control_deassert(sdc->rst);
+			}
+		}
+	}
+
 	sdc->clk = devm_clk_get(&pdev->dev, NULL);
 	if (IS_ERR(sdc->clk))
 		return PTR_ERR(sdc->clk);
@@ -506,6 +600,10 @@ static int aspeed_sdc_probe(struct platform_device *pdev)
 		goto err_clk;
 	}
 
+	if (!of_property_read_u32(pdev->dev.of_node,
+				  "timing-phase", &timing_phase))
+		writel(timing_phase, sdc->regs + ASPEED_SDC_PHASE);
+
 	dev_set_drvdata(&pdev->dev, sdc);
 
 	parent = pdev->dev.of_node;
@@ -536,15 +634,6 @@ static int aspeed_sdc_remove(struct platform_device *pdev)
 	return 0;
 }
 
-static const struct of_device_id aspeed_sdc_of_match[] = {
-	{ .compatible = "aspeed,ast2400-sd-controller", },
-	{ .compatible = "aspeed,ast2500-sd-controller", },
-	{ .compatible = "aspeed,ast2600-sd-controller", },
-	{ }
-};
-
-MODULE_DEVICE_TABLE(of, aspeed_sdc_of_match);
-
 static struct platform_driver aspeed_sdc_driver = {
 	.driver		= {
 		.name	= "sd-controller-aspeed",
-- 
2.17.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v2 3/3] mmc: sdhci-of-aspeed: Sync capabilities from device tree to ast2600 SoC registers

[Andrew Jeffery]

Hi Steven,

On Mon, 3 May 2021, at 11:13, Steven Lee wrote:
> Sync Capbility Registers(SDIO140, SDIO144, SDIO240, SDIO244) of ast2600
> SoC from the device tree.
> The bit 26(Voltage Support 1.8v) of SDIO140/SDIO240 is set to 1 if
> "mmc-hs200-1_8v" or "sd-uhs-sdr104" is added in the device tree.
> The bit 1(SDR104 Supported) of SDR144/SDR244 is set to 1 if "sd-uhs-sdr104"
> is added in the device tree.
> "timing-phase" is synced to SDIO0F4(Colock Phase Control)
> 
> Signed-off-by: Steven Lee <steven_lee@aspeedtech.com>
> ---
>  drivers/mmc/host/sdhci-of-aspeed.c | 107 ++++++++++++++++++++++++++---
>  1 file changed, 98 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/mmc/host/sdhci-of-aspeed.c 
> b/drivers/mmc/host/sdhci-of-aspeed.c
> index 7d8692e90996..2d755bac777a 100644
> --- a/drivers/mmc/host/sdhci-of-aspeed.c
> +++ b/drivers/mmc/host/sdhci-of-aspeed.c
> @@ -13,6 +13,7 @@
>  #include <linux/of.h>
>  #include <linux/of_platform.h>
>  #include <linux/platform_device.h>
> +#include <linux/reset.h>
>  #include <linux/spinlock.h>
>  
>  #include "sdhci-pltfm.h"
> @@ -30,10 +31,18 @@
>  #define   ASPEED_SDC_S0_PHASE_IN_EN	BIT(2)
>  #define   ASPEED_SDC_S0_PHASE_OUT_EN	GENMASK(1, 0)
>  #define   ASPEED_SDC_PHASE_MAX		31
> +#define ASPEED_SDC_CAP1_1_8V           BIT(26)
> +#define ASPEED_SDC_CAP2_SDR104         BIT(1)
> +#define PROBE_AFTER_ASSET_DEASSERT     0x1
> +
> +struct aspeed_sdc_info {
> +	u32 flag;
> +};
>  
>  struct aspeed_sdc {
>  	struct clk *clk;
>  	struct resource *res;
> +	struct reset_control *rst;
>  
>  	spinlock_t lock;
>  	void __iomem *regs;
> @@ -72,6 +81,44 @@ struct aspeed_sdhci {
>  	const struct aspeed_sdhci_phase_desc *phase_desc;
>  };
>  
> +struct aspeed_sdc_info ast2600_sdc_info = {
> +	.flag = PROBE_AFTER_ASSET_DEASSERT
> +};
> +
> +/*
> + * The function sets the mirror register for updating
> + * capbilities of the current slot.
> + *
> + *   slot | cap_idx | caps_reg | mirror_reg
> + *   -----|---------|----------|------------
> + *     0  |    0    | SDIO140  |   SDIO10
> + *     0  |    1    | SDIO144  |   SDIO14
> + *     1  |    0    | SDIO240  |   SDIO20
> + *     1  |    1    | SDIO244  |   SDIO24
> + */
> +static void aspeed_sdc_set_slot_capability(struct sdhci_host *host,
> +					   struct aspeed_sdc *sdc,
> +					   u32 reg_val,
> +					   u8 slot,
> +					   u8 cap_idx)

Having thought about this some more now we have code, I wonder if we can get
rid of `cap_idx` as a parameter. Something like:

static void aspeed_sdc_set_slot_capability(struct sdhci_host *host,
    struct aspeed_sdc *sdc, int capability, bool enable, u8 slot);

From there, instead of

#define ASPEED_SDC_CAP1_1_8V           BIT(26)
#define ASPEED_SDC_CAP2_SDR104         BIT(1)

We do

/* SDIO{10,20} */
#define ASPEED_SDC_CAP1_1_8V           (0 * 32 + 26)
/* SDIO{14,24} */
#define ASPEED_SDC_CAP2_SDR104         (1 * 32 + 1)

Then in the implementation of aspeed_sdc_set_slot_capability() we 
derive cap_idx and reg_val:

u8 reg_val = enable * BIT(capability % 32);
u8 cap_reg = capability / 32;

That way we get rid of the 0 and 1 magic values for cap_idx when 
invoking aspeed_sdc_set_slot_capability() and the caller can't
accidentally pass the wrong value.

> +{
> +	u8 caps_reg_offset;
> +	u32 caps_reg;
> +	u32 mirror_reg_offset;
> +	u32 caps_val;
> +
> +	if (cap_idx > 1 || slot > 1)
> +		return;
> +
> +	caps_reg_offset = (cap_idx == 0) ? 0 : 4;
> +	caps_reg = 0x40 + caps_reg_offset;
> +	caps_val = sdhci_readl(host, caps_reg);

Hmm, I think you used sdhci_readl() because I commented on that last 
time. If the global-area registers are truly mirrored we could read 
from them as well right? In which case we could just use 
readl(sdc->regs + mirror_reg_offset)? If so we can drop the host 
parameter and (incorporating my suggestion above) just have:

static void aspeed_sdc_set_slot_capability(struct aspeed_sdc *sdc,
    int capability, bool enable, u8 slot);

Sorry if I've sort of flip-flopped on that, but I think originally you 
were still reading from the SDHCI (read-only) address?

> +	caps_val |= reg_val;
> +	mirror_reg_offset = (slot == 0) ? 0x10 : 0x20;
> +	mirror_reg_offset += caps_reg_offset;
> +	writel(caps_val, sdc->regs + mirror_reg_offset);
> +}
> +
>  static void aspeed_sdc_configure_8bit_mode(struct aspeed_sdc *sdc,
>  					   struct aspeed_sdhci *sdhci,
>  					   bool bus8)
> @@ -329,9 +376,11 @@ static int aspeed_sdhci_probe(struct platform_device *pdev)
>  {
>  	const struct aspeed_sdhci_pdata *aspeed_pdata;
>  	struct sdhci_pltfm_host *pltfm_host;
> +	struct device_node *np = pdev->dev.of_node;
>  	struct aspeed_sdhci *dev;
>  	struct sdhci_host *host;
>  	struct resource *res;
> +	u32 reg_val;
>  	int slot;
>  	int ret;
>  
> @@ -372,6 +421,21 @@ static int aspeed_sdhci_probe(struct platform_device *pdev)
>  
>  	sdhci_get_of_property(pdev);
>  
> +	if (of_property_read_bool(np, "mmc-hs200-1_8v") ||
> +	    of_property_read_bool(np, "sd-uhs-sdr104"))
> +		aspeed_sdc_set_slot_capability(host,
> +					       dev->parent,
> +					       ASPEED_SDC_CAP1_1_8V,
> +					       slot,
> +					       0);

See the discussion above about reworking aspeed_sdc_set_slot_capability.

> +
> +	if (of_property_read_bool(np, "sd-uhs-sdr104"))
> +		aspeed_sdc_set_slot_capability(host,
> +					       dev->parent,
> +					       ASPEED_SDC_CAP2_SDR104,
> +					       slot,
> +					       1);

Again here.

> +
>  	pltfm_host->clk = devm_clk_get(&pdev->dev, NULL);
>  	if (IS_ERR(pltfm_host->clk))
>  		return PTR_ERR(pltfm_host->clk);
> @@ -476,12 +540,25 @@ static struct platform_driver aspeed_sdhci_driver = {
>  	.remove		= aspeed_sdhci_remove,
>  };
>  
> +static const struct of_device_id aspeed_sdc_of_match[] = {
> +	{ .compatible = "aspeed,ast2400-sd-controller", },
> +	{ .compatible = "aspeed,ast2500-sd-controller", },
> +	{ .compatible = "aspeed,ast2600-sd-controller", .data = &ast2600_sdc_info},
> +	{ }
> +};
> +
> +MODULE_DEVICE_TABLE(of, aspeed_sdc_of_match);
> +
>  static int aspeed_sdc_probe(struct platform_device *pdev)
>  
>  {
>  	struct device_node *parent, *child;
>  	struct aspeed_sdc *sdc;
> +	const struct of_device_id *match = NULL;
> +	const struct aspeed_sdc_info *info = NULL;
> +
>  	int ret;
> +	u32 timing_phase;
>  
>  	sdc = devm_kzalloc(&pdev->dev, sizeof(*sdc), GFP_KERNEL);
>  	if (!sdc)
> @@ -489,6 +566,23 @@ static int aspeed_sdc_probe(struct platform_device *pdev)
>  
>  	spin_lock_init(&sdc->lock);
>  
> +	match = of_match_device(aspeed_sdc_of_match, &pdev->dev);
> +	if (!match)
> +		return -ENODEV;
> +
> +	if (match->data)
> +		info = match->data;
> +
> +	if (info) {
> +		if (info->flag & PROBE_AFTER_ASSET_DEASSERT) {
> +			sdc->rst = devm_reset_control_get(&pdev->dev, NULL);
> +			if (!IS_ERR(sdc->rst)) {
> +				reset_control_assert(sdc->rst);
> +				reset_control_deassert(sdc->rst);
> +			}
> +		}
> +	}

I think this should be a separate patch.

From the code it seems that this is necessary for just the 2600? Where 
is this documented?

> +
>  	sdc->clk = devm_clk_get(&pdev->dev, NULL);
>  	if (IS_ERR(sdc->clk))
>  		return PTR_ERR(sdc->clk);
> @@ -506,6 +600,10 @@ static int aspeed_sdc_probe(struct platform_device *pdev)
>  		goto err_clk;
>  	}
>  
> +	if (!of_property_read_u32(pdev->dev.of_node,
> +				  "timing-phase", &timing_phase))
> +		writel(timing_phase, sdc->regs + ASPEED_SDC_PHASE);

I asked on v1 that you use the phase support already in the bindings 
and in the driver. The example you added in the binding document[1] 
used the existing devicetree properties but it seems you haven't fixed 
the code.

Please drop your phase implementation from the patch.

[1] https://lore.kernel.org/lkml/20210503014336.20256-2-steven_lee@aspeedtech.com/

Cheers,

Andrew

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v2 3/3] mmc: sdhci-of-aspeed: Sync capabilities from device tree to ast2600 SoC registers

[Steven Lee]

The 05/03/2021 13:04, Andrew Jeffery wrote:
> Hi Steven,
> 
> On Mon, 3 May 2021, at 11:13, Steven Lee wrote:
> > Sync Capbility Registers(SDIO140, SDIO144, SDIO240, SDIO244) of ast2600
> > SoC from the device tree.
> > The bit 26(Voltage Support 1.8v) of SDIO140/SDIO240 is set to 1 if
> > "mmc-hs200-1_8v" or "sd-uhs-sdr104" is added in the device tree.
> > The bit 1(SDR104 Supported) of SDR144/SDR244 is set to 1 if "sd-uhs-sdr104"
> > is added in the device tree.
> > "timing-phase" is synced to SDIO0F4(Colock Phase Control)
> > 
> > Signed-off-by: Steven Lee <steven_lee@aspeedtech.com>
> > ---
> >  drivers/mmc/host/sdhci-of-aspeed.c | 107 ++++++++++++++++++++++++++---
> >  1 file changed, 98 insertions(+), 9 deletions(-)
> > 
> > diff --git a/drivers/mmc/host/sdhci-of-aspeed.c 
> > b/drivers/mmc/host/sdhci-of-aspeed.c
> > index 7d8692e90996..2d755bac777a 100644
> > --- a/drivers/mmc/host/sdhci-of-aspeed.c
> > +++ b/drivers/mmc/host/sdhci-of-aspeed.c
> > @@ -13,6 +13,7 @@
> >  #include <linux/of.h>
> >  #include <linux/of_platform.h>
> >  #include <linux/platform_device.h>
> > +#include <linux/reset.h>
> >  #include <linux/spinlock.h>
> >  
> >  #include "sdhci-pltfm.h"
> > @@ -30,10 +31,18 @@
> >  #define   ASPEED_SDC_S0_PHASE_IN_EN	BIT(2)
> >  #define   ASPEED_SDC_S0_PHASE_OUT_EN	GENMASK(1, 0)
> >  #define   ASPEED_SDC_PHASE_MAX		31
> > +#define ASPEED_SDC_CAP1_1_8V           BIT(26)
> > +#define ASPEED_SDC_CAP2_SDR104         BIT(1)
> > +#define PROBE_AFTER_ASSET_DEASSERT     0x1
> > +
> > +struct aspeed_sdc_info {
> > +	u32 flag;
> > +};
> >  
> >  struct aspeed_sdc {
> >  	struct clk *clk;
> >  	struct resource *res;
> > +	struct reset_control *rst;
> >  
> >  	spinlock_t lock;
> >  	void __iomem *regs;
> > @@ -72,6 +81,44 @@ struct aspeed_sdhci {
> >  	const struct aspeed_sdhci_phase_desc *phase_desc;
> >  };
> >  
> > +struct aspeed_sdc_info ast2600_sdc_info = {
> > +	.flag = PROBE_AFTER_ASSET_DEASSERT
> > +};
> > +
> > +/*
> > + * The function sets the mirror register for updating
> > + * capbilities of the current slot.
> > + *
> > + *   slot | cap_idx | caps_reg | mirror_reg
> > + *   -----|---------|----------|------------
> > + *     0  |    0    | SDIO140  |   SDIO10
> > + *     0  |    1    | SDIO144  |   SDIO14
> > + *     1  |    0    | SDIO240  |   SDIO20
> > + *     1  |    1    | SDIO244  |   SDIO24
> > + */
> > +static void aspeed_sdc_set_slot_capability(struct sdhci_host *host,
> > +					   struct aspeed_sdc *sdc,
> > +					   u32 reg_val,
> > +					   u8 slot,
> > +					   u8 cap_idx)
> 
> Having thought about this some more now we have code, I wonder if we can get
> rid of `cap_idx` as a parameter. Something like:
> 
> static void aspeed_sdc_set_slot_capability(struct sdhci_host *host,
>     struct aspeed_sdc *sdc, int capability, bool enable, u8 slot);
> 
> From there, instead of
> 
> #define ASPEED_SDC_CAP1_1_8V           BIT(26)
> #define ASPEED_SDC_CAP2_SDR104         BIT(1)
> 
> We do
> 
> /* SDIO{10,20} */
> #define ASPEED_SDC_CAP1_1_8V           (0 * 32 + 26)
> /* SDIO{14,24} */
> #define ASPEED_SDC_CAP2_SDR104         (1 * 32 + 1)
> 
> Then in the implementation of aspeed_sdc_set_slot_capability() we 
> derive cap_idx and reg_val:
> 
> u8 reg_val = enable * BIT(capability % 32);
> u8 cap_reg = capability / 32;
> 
> That way we get rid of the 0 and 1 magic values for cap_idx when 
> invoking aspeed_sdc_set_slot_capability() and the caller can't
> accidentally pass the wrong value.
> 

Thanks for the detailed suggestion, I will modify the function as you
suggested.

> > +{
> > +	u8 caps_reg_offset;
> > +	u32 caps_reg;
> > +	u32 mirror_reg_offset;
> > +	u32 caps_val;
> > +
> > +	if (cap_idx > 1 || slot > 1)
> > +		return;
> > +
> > +	caps_reg_offset = (cap_idx == 0) ? 0 : 4;
> > +	caps_reg = 0x40 + caps_reg_offset;
> > +	caps_val = sdhci_readl(host, caps_reg);
> 
> Hmm, I think you used sdhci_readl() because I commented on that last 
> time. If the global-area registers are truly mirrored we could read 
> from them as well right? In which case we could just use 
> readl(sdc->regs + mirror_reg_offset)? If so we can drop the host 
> parameter and (incorporating my suggestion above) just have:
> 
> static void aspeed_sdc_set_slot_capability(struct aspeed_sdc *sdc,
>     int capability, bool enable, u8 slot);
> 
> Sorry if I've sort of flip-flopped on that, but I think originally you 
> were still reading from the SDHCI (read-only) address?
> 

Yes, mirror registers are used to update the capability register, it returns
zero if we read the mirror register.
The test result is as follows:

# devmem 0x1e740010 32             // Read SDIO010(Mirror of SDIO140)
0x00000000

# devmem 0x1e740140 32             // Read capability
0x07FC0080

# devmem 0x1e740010 32 0x07fb0080  // Write mirror

# devmem 0x1e740010 32             // Read mirror
0x00000000

# devmem 0x1e740140 32             // Read capability
0x07FB0080

> > +	caps_val |= reg_val;
> > +	mirror_reg_offset = (slot == 0) ? 0x10 : 0x20;
> > +	mirror_reg_offset += caps_reg_offset;
> > +	writel(caps_val, sdc->regs + mirror_reg_offset);
> > +}
> > +
> >  static void aspeed_sdc_configure_8bit_mode(struct aspeed_sdc *sdc,
> >  					   struct aspeed_sdhci *sdhci,
> >  					   bool bus8)
> > @@ -329,9 +376,11 @@ static int aspeed_sdhci_probe(struct platform_device *pdev)
> >  {
> >  	const struct aspeed_sdhci_pdata *aspeed_pdata;
> >  	struct sdhci_pltfm_host *pltfm_host;
> > +	struct device_node *np = pdev->dev.of_node;
> >  	struct aspeed_sdhci *dev;
> >  	struct sdhci_host *host;
> >  	struct resource *res;
> > +	u32 reg_val;
> >  	int slot;
> >  	int ret;
> >  
> > @@ -372,6 +421,21 @@ static int aspeed_sdhci_probe(struct platform_device *pdev)
> >  
> >  	sdhci_get_of_property(pdev);
> >  
> > +	if (of_property_read_bool(np, "mmc-hs200-1_8v") ||
> > +	    of_property_read_bool(np, "sd-uhs-sdr104"))
> > +		aspeed_sdc_set_slot_capability(host,
> > +					       dev->parent,
> > +					       ASPEED_SDC_CAP1_1_8V,
> > +					       slot,
> > +					       0);
> 
> See the discussion above about reworking aspeed_sdc_set_slot_capability.
> 

Will do it.

> > +
> > +	if (of_property_read_bool(np, "sd-uhs-sdr104"))
> > +		aspeed_sdc_set_slot_capability(host,
> > +					       dev->parent,
> > +					       ASPEED_SDC_CAP2_SDR104,
> > +					       slot,
> > +					       1);
> 
> Again here.
> 

Will do it.

> > +
> >  	pltfm_host->clk = devm_clk_get(&pdev->dev, NULL);
> >  	if (IS_ERR(pltfm_host->clk))
> >  		return PTR_ERR(pltfm_host->clk);
> > @@ -476,12 +540,25 @@ static struct platform_driver aspeed_sdhci_driver = {
> >  	.remove		= aspeed_sdhci_remove,
> >  };
> >  
> > +static const struct of_device_id aspeed_sdc_of_match[] = {
> > +	{ .compatible = "aspeed,ast2400-sd-controller", },
> > +	{ .compatible = "aspeed,ast2500-sd-controller", },
> > +	{ .compatible = "aspeed,ast2600-sd-controller", .data = &ast2600_sdc_info},
> > +	{ }
> > +};
> > +
> > +MODULE_DEVICE_TABLE(of, aspeed_sdc_of_match);
> > +
> >  static int aspeed_sdc_probe(struct platform_device *pdev)
> >  
> >  {
> >  	struct device_node *parent, *child;
> >  	struct aspeed_sdc *sdc;
> > +	const struct of_device_id *match = NULL;
> > +	const struct aspeed_sdc_info *info = NULL;
> > +
> >  	int ret;
> > +	u32 timing_phase;
> >  
> >  	sdc = devm_kzalloc(&pdev->dev, sizeof(*sdc), GFP_KERNEL);
> >  	if (!sdc)
> > @@ -489,6 +566,23 @@ static int aspeed_sdc_probe(struct platform_device *pdev)
> >  
> >  	spin_lock_init(&sdc->lock);
> >  
> > +	match = of_match_device(aspeed_sdc_of_match, &pdev->dev);
> > +	if (!match)
> > +		return -ENODEV;
> > +
> > +	if (match->data)
> > +		info = match->data;
> > +
> > +	if (info) {
> > +		if (info->flag & PROBE_AFTER_ASSET_DEASSERT) {
> > +			sdc->rst = devm_reset_control_get(&pdev->dev, NULL);
> > +			if (!IS_ERR(sdc->rst)) {
> > +				reset_control_assert(sdc->rst);
> > +				reset_control_deassert(sdc->rst);
> > +			}
> > +		}
> > +	}
> 
> I think this should be a separate patch.
> 
> From the code it seems that this is necessary for just the 2600? Where 
> is this documented?
> 

Yes it is just for 2600. The patch is suggested by our chip designer and
is used for cleaning up MMC controller.
Currently, there is no document describes this changes.

And I have a question regarding the "separate patch", does it mean I should
create another patch set or I can add a new patch in the current
patch set?

> > +
> >  	sdc->clk = devm_clk_get(&pdev->dev, NULL);
> >  	if (IS_ERR(sdc->clk))
> >  		return PTR_ERR(sdc->clk);
> > @@ -506,6 +600,10 @@ static int aspeed_sdc_probe(struct platform_device *pdev)
> >  		goto err_clk;
> >  	}
> >  
> > +	if (!of_property_read_u32(pdev->dev.of_node,
> > +				  "timing-phase", &timing_phase))
> > +		writel(timing_phase, sdc->regs + ASPEED_SDC_PHASE);
> 
> I asked on v1 that you use the phase support already in the bindings 
> and in the driver. The example you added in the binding document[1] 
> used the existing devicetree properties but it seems you haven't fixed 
> the code.
> 
> Please drop your phase implementation from the patch.
> 

Sorry, I misunderstand the comment in the v1 patch. I thought that I should use
the exists ASPEED_SDC_PHASE for timing-phase.

Now I think I understand what you mean in the previous review.
I will remove the implementation you mentioned and add the following setting in
the device tree to verify again.

 clk-phase-mmc-hs200 = <N>, <N>;


> [1] https://lore.kernel.org/lkml/20210503014336.20256-2-steven_lee@aspeedtech.com/
> 
> Cheers,
> 
> Andrew

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v2 3/3] mmc: sdhci-of-aspeed: Sync capabilities from device tree to ast2600 SoC registers

[Andrew Jeffery]

On Mon, 3 May 2021, at 20:22, Steven Lee wrote:
> The 05/03/2021 13:04, Andrew Jeffery wrote:
> > Hi Steven,
> > 
> > On Mon, 3 May 2021, at 11:13, Steven Lee wrote:
> > > Sync Capbility Registers(SDIO140, SDIO144, SDIO240, SDIO244) of ast2600
> > > SoC from the device tree.
> > > The bit 26(Voltage Support 1.8v) of SDIO140/SDIO240 is set to 1 if
> > > "mmc-hs200-1_8v" or "sd-uhs-sdr104" is added in the device tree.
> > > The bit 1(SDR104 Supported) of SDR144/SDR244 is set to 1 if "sd-uhs-sdr104"
> > > is added in the device tree.
> > > "timing-phase" is synced to SDIO0F4(Colock Phase Control)
> > > 
> > > Signed-off-by: Steven Lee <steven_lee@aspeedtech.com>
> > > ---
> > >  drivers/mmc/host/sdhci-of-aspeed.c | 107 ++++++++++++++++++++++++++---
> > >  1 file changed, 98 insertions(+), 9 deletions(-)
> > > 
> > > diff --git a/drivers/mmc/host/sdhci-of-aspeed.c 
> > > b/drivers/mmc/host/sdhci-of-aspeed.c
> > > index 7d8692e90996..2d755bac777a 100644
> > > --- a/drivers/mmc/host/sdhci-of-aspeed.c
> > > +++ b/drivers/mmc/host/sdhci-of-aspeed.c
> > > @@ -13,6 +13,7 @@
> > >  #include <linux/of.h>
> > >  #include <linux/of_platform.h>
> > >  #include <linux/platform_device.h>
> > > +#include <linux/reset.h>
> > >  #include <linux/spinlock.h>
> > >  
> > >  #include "sdhci-pltfm.h"
> > > @@ -30,10 +31,18 @@
> > >  #define   ASPEED_SDC_S0_PHASE_IN_EN	BIT(2)
> > >  #define   ASPEED_SDC_S0_PHASE_OUT_EN	GENMASK(1, 0)
> > >  #define   ASPEED_SDC_PHASE_MAX		31
> > > +#define ASPEED_SDC_CAP1_1_8V           BIT(26)
> > > +#define ASPEED_SDC_CAP2_SDR104         BIT(1)
> > > +#define PROBE_AFTER_ASSET_DEASSERT     0x1
> > > +
> > > +struct aspeed_sdc_info {
> > > +	u32 flag;
> > > +};
> > >  
> > >  struct aspeed_sdc {
> > >  	struct clk *clk;
> > >  	struct resource *res;
> > > +	struct reset_control *rst;
> > >  
> > >  	spinlock_t lock;
> > >  	void __iomem *regs;
> > > @@ -72,6 +81,44 @@ struct aspeed_sdhci {
> > >  	const struct aspeed_sdhci_phase_desc *phase_desc;
> > >  };
> > >  
> > > +struct aspeed_sdc_info ast2600_sdc_info = {
> > > +	.flag = PROBE_AFTER_ASSET_DEASSERT
> > > +};
> > > +
> > > +/*
> > > + * The function sets the mirror register for updating
> > > + * capbilities of the current slot.
> > > + *
> > > + *   slot | cap_idx | caps_reg | mirror_reg
> > > + *   -----|---------|----------|------------
> > > + *     0  |    0    | SDIO140  |   SDIO10
> > > + *     0  |    1    | SDIO144  |   SDIO14
> > > + *     1  |    0    | SDIO240  |   SDIO20
> > > + *     1  |    1    | SDIO244  |   SDIO24
> > > + */
> > > +static void aspeed_sdc_set_slot_capability(struct sdhci_host *host,
> > > +					   struct aspeed_sdc *sdc,
> > > +					   u32 reg_val,
> > > +					   u8 slot,
> > > +					   u8 cap_idx)
> > 
> > Having thought about this some more now we have code, I wonder if we can get
> > rid of `cap_idx` as a parameter. Something like:
> > 
> > static void aspeed_sdc_set_slot_capability(struct sdhci_host *host,
> >     struct aspeed_sdc *sdc, int capability, bool enable, u8 slot);
> > 
> > From there, instead of
> > 
> > #define ASPEED_SDC_CAP1_1_8V           BIT(26)
> > #define ASPEED_SDC_CAP2_SDR104         BIT(1)
> > 
> > We do
> > 
> > /* SDIO{10,20} */
> > #define ASPEED_SDC_CAP1_1_8V           (0 * 32 + 26)
> > /* SDIO{14,24} */
> > #define ASPEED_SDC_CAP2_SDR104         (1 * 32 + 1)
> > 
> > Then in the implementation of aspeed_sdc_set_slot_capability() we 
> > derive cap_idx and reg_val:
> > 
> > u8 reg_val = enable * BIT(capability % 32);
> > u8 cap_reg = capability / 32;
> > 
> > That way we get rid of the 0 and 1 magic values for cap_idx when 
> > invoking aspeed_sdc_set_slot_capability() and the caller can't
> > accidentally pass the wrong value.
> > 
> 
> Thanks for the detailed suggestion, I will modify the function as you
> suggested.

Great!

> 
> > > +{
> > > +	u8 caps_reg_offset;
> > > +	u32 caps_reg;
> > > +	u32 mirror_reg_offset;
> > > +	u32 caps_val;
> > > +
> > > +	if (cap_idx > 1 || slot > 1)
> > > +		return;
> > > +
> > > +	caps_reg_offset = (cap_idx == 0) ? 0 : 4;
> > > +	caps_reg = 0x40 + caps_reg_offset;
> > > +	caps_val = sdhci_readl(host, caps_reg);
> > 
> > Hmm, I think you used sdhci_readl() because I commented on that last 
> > time. If the global-area registers are truly mirrored we could read 
> > from them as well right? In which case we could just use 
> > readl(sdc->regs + mirror_reg_offset)? If so we can drop the host 
> > parameter and (incorporating my suggestion above) just have:
> > 
> > static void aspeed_sdc_set_slot_capability(struct aspeed_sdc *sdc,
> >     int capability, bool enable, u8 slot);
> > 
> > Sorry if I've sort of flip-flopped on that, but I think originally you 
> > were still reading from the SDHCI (read-only) address?
> > 
> 
> Yes, mirror registers are used to update the capability register, it returns
> zero if we read the mirror register.
> The test result is as follows:
> 
> # devmem 0x1e740010 32             // Read SDIO010(Mirror of SDIO140)
> 0x00000000
> 
> # devmem 0x1e740140 32             // Read capability
> 0x07FC0080
> 
> # devmem 0x1e740010 32 0x07fb0080  // Write mirror
> 
> # devmem 0x1e740010 32             // Read mirror
> 0x00000000
> 
> # devmem 0x1e740140 32             // Read capability
> 0x07FB0080

Ah well, I guess we continue to pass the struct sdhci_host pointer then.

> 
> > > +	caps_val |= reg_val;
> > > +	mirror_reg_offset = (slot == 0) ? 0x10 : 0x20;
> > > +	mirror_reg_offset += caps_reg_offset;
> > > +	writel(caps_val, sdc->regs + mirror_reg_offset);
> > > +}
> > > +
> > >  static void aspeed_sdc_configure_8bit_mode(struct aspeed_sdc *sdc,
> > >  					   struct aspeed_sdhci *sdhci,
> > >  					   bool bus8)
> > > @@ -329,9 +376,11 @@ static int aspeed_sdhci_probe(struct platform_device *pdev)
> > >  {
> > >  	const struct aspeed_sdhci_pdata *aspeed_pdata;
> > >  	struct sdhci_pltfm_host *pltfm_host;
> > > +	struct device_node *np = pdev->dev.of_node;
> > >  	struct aspeed_sdhci *dev;
> > >  	struct sdhci_host *host;
> > >  	struct resource *res;
> > > +	u32 reg_val;
> > >  	int slot;
> > >  	int ret;
> > >  
> > > @@ -372,6 +421,21 @@ static int aspeed_sdhci_probe(struct platform_device *pdev)
> > >  
> > >  	sdhci_get_of_property(pdev);
> > >  
> > > +	if (of_property_read_bool(np, "mmc-hs200-1_8v") ||
> > > +	    of_property_read_bool(np, "sd-uhs-sdr104"))
> > > +		aspeed_sdc_set_slot_capability(host,
> > > +					       dev->parent,
> > > +					       ASPEED_SDC_CAP1_1_8V,
> > > +					       slot,
> > > +					       0);
> > 
> > See the discussion above about reworking aspeed_sdc_set_slot_capability.
> > 
> 
> Will do it.
> 
> > > +
> > > +	if (of_property_read_bool(np, "sd-uhs-sdr104"))
> > > +		aspeed_sdc_set_slot_capability(host,
> > > +					       dev->parent,
> > > +					       ASPEED_SDC_CAP2_SDR104,
> > > +					       slot,
> > > +					       1);
> > 
> > Again here.
> > 
> 
> Will do it.
> 
> > > +
> > >  	pltfm_host->clk = devm_clk_get(&pdev->dev, NULL);
> > >  	if (IS_ERR(pltfm_host->clk))
> > >  		return PTR_ERR(pltfm_host->clk);
> > > @@ -476,12 +540,25 @@ static struct platform_driver aspeed_sdhci_driver = {
> > >  	.remove		= aspeed_sdhci_remove,
> > >  };
> > >  
> > > +static const struct of_device_id aspeed_sdc_of_match[] = {
> > > +	{ .compatible = "aspeed,ast2400-sd-controller", },
> > > +	{ .compatible = "aspeed,ast2500-sd-controller", },
> > > +	{ .compatible = "aspeed,ast2600-sd-controller", .data = &ast2600_sdc_info},
> > > +	{ }
> > > +};
> > > +
> > > +MODULE_DEVICE_TABLE(of, aspeed_sdc_of_match);
> > > +
> > >  static int aspeed_sdc_probe(struct platform_device *pdev)
> > >  
> > >  {
> > >  	struct device_node *parent, *child;
> > >  	struct aspeed_sdc *sdc;
> > > +	const struct of_device_id *match = NULL;
> > > +	const struct aspeed_sdc_info *info = NULL;
> > > +
> > >  	int ret;
> > > +	u32 timing_phase;
> > >  
> > >  	sdc = devm_kzalloc(&pdev->dev, sizeof(*sdc), GFP_KERNEL);
> > >  	if (!sdc)
> > > @@ -489,6 +566,23 @@ static int aspeed_sdc_probe(struct platform_device *pdev)
> > >  
> > >  	spin_lock_init(&sdc->lock);
> > >  
> > > +	match = of_match_device(aspeed_sdc_of_match, &pdev->dev);
> > > +	if (!match)
> > > +		return -ENODEV;
> > > +
> > > +	if (match->data)
> > > +		info = match->data;
> > > +
> > > +	if (info) {
> > > +		if (info->flag & PROBE_AFTER_ASSET_DEASSERT) {
> > > +			sdc->rst = devm_reset_control_get(&pdev->dev, NULL);
> > > +			if (!IS_ERR(sdc->rst)) {
> > > +				reset_control_assert(sdc->rst);
> > > +				reset_control_deassert(sdc->rst);
> > > +			}
> > > +		}
> > > +	}
> > 
> > I think this should be a separate patch.
> > 
> > From the code it seems that this is necessary for just the 2600? Where 
> > is this documented?
> > 
> 
> Yes it is just for 2600. The patch is suggested by our chip designer and
> is used for cleaning up MMC controller.
> Currently, there is no document describes this changes.
> 
> And I have a question regarding the "separate patch", does it mean I should
> create another patch set or I can add a new patch in the current
> patch set?

Depends what you mean by this :)

It's kind-of awkward to send another patch as part of the existing v2 
of the series, as you'll wind up with what is effectively patch 4/3. 
It's less confusing to just send a v3 with all 4 patches.

However, in general if patches don't depend on each other it's good to 
send them as separate series, that way the series can be applied in any 
order.

> 
> > > +
> > >  	sdc->clk = devm_clk_get(&pdev->dev, NULL);
> > >  	if (IS_ERR(sdc->clk))
> > >  		return PTR_ERR(sdc->clk);
> > > @@ -506,6 +600,10 @@ static int aspeed_sdc_probe(struct platform_device *pdev)
> > >  		goto err_clk;
> > >  	}
> > >  
> > > +	if (!of_property_read_u32(pdev->dev.of_node,
> > > +				  "timing-phase", &timing_phase))
> > > +		writel(timing_phase, sdc->regs + ASPEED_SDC_PHASE);
> > 
> > I asked on v1 that you use the phase support already in the bindings 
> > and in the driver. The example you added in the binding document[1] 
> > used the existing devicetree properties but it seems you haven't fixed 
> > the code.
> > 
> > Please drop your phase implementation from the patch.
> > 
> 
> Sorry, I misunderstand the comment in the v1 patch. I thought that I should use
> the exists ASPEED_SDC_PHASE for timing-phase.

Ah!

> 
> Now I think I understand what you mean in the previous review.
> I will remove the implementation you mentioned and add the following setting in
> the device tree to verify again.
> 
>  clk-phase-mmc-hs200 = <N>, <N>;

Right, that's what I was suggesting. We have support for most of the 
other speeds and as well (not just HS200, just that HS200 is what 
Rainier cares about), see:

https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/mmc/core/host.c?h=v5.12#n181

Cheers,

Andrew

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel