diff mbox series

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

Message ID 20210503014336.20256-2-steven_lee@aspeedtech.com
State New
Headers show
Series [v2,1/3] dt-bindings: mmc: sdhci-of-aspeed: Add description for AST2600 EVB. | expand

Commit Message

Steven Lee May 3, 2021, 1:43 a.m. UTC
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(+)

Comments

Andrew Jeffery May 3, 2021, 4:19 a.m. UTC | #1
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
Steven Lee May 3, 2021, 9:40 a.m. UTC | #2
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
Andrew Jeffery May 3, 2021, 10:32 a.m. UTC | #3
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
Rob Herring (Arm) May 3, 2021, 3:20 p.m. UTC | #4
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.
Steven Lee May 4, 2021, 1:46 a.m. UTC | #5
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.
diff mbox series

Patch

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>;
+            };
+    };