* [PATCH v2 1/3] dt-bindings: mmc: sdhci-of-aspeed: Add description for AST2600 EVB. [not found] <20210503014336.20256-1-steven_lee@aspeedtech.com> @ 2021-05-03 1:43 ` Steven Lee 2021-05-03 4:19 ` Andrew Jeffery 2021-05-03 15:20 ` Rob Herring 2021-05-03 1:43 ` [PATCH v2 2/3] ARM: dts: aspeed: ast2600evb: Add timing-phase property for eMMC controller Steven Lee 2021-05-03 1:43 ` [PATCH v2 3/3] mmc: sdhci-of-aspeed: Sync capabilities from device tree to ast2600 SoC registers Steven Lee 2 siblings, 2 replies; 14+ messages in thread From: Steven Lee @ 2021-05-03 1:43 UTC (permalink / raw) To: Andrew Jeffery, Ulf Hansson, Rob Herring, Joel Stanley, Ryan Chen, moderated list:ASPEED SD/MMC DRIVER, moderated list:ASPEED SD/MMC DRIVER, open list:ASPEED SD/MMC DRIVER, open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS, moderated list:ARM/ASPEED MACHINE SUPPORT, open list Cc: Hongweiz, ryan_chen, chin-ting_kuo 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 ^ permalink raw reply related [flat|nested] 14+ messages in thread
* Re: [PATCH v2 1/3] dt-bindings: mmc: sdhci-of-aspeed: Add description for AST2600 EVB. 2021-05-03 1:43 ` [PATCH v2 1/3] dt-bindings: mmc: sdhci-of-aspeed: Add description for AST2600 EVB Steven Lee @ 2021-05-03 4:19 ` Andrew Jeffery 2021-05-03 9:40 ` Steven Lee 2021-05-03 15:20 ` Rob Herring 1 sibling, 1 reply; 14+ messages in thread From: Andrew Jeffery @ 2021-05-03 4:19 UTC (permalink / raw) To: Steven Lee, Ulf Hansson, Rob Herring, Joel Stanley, Ryan Chen, moderated list:ASPEED SD/MMC DRIVER, moderated list:ASPEED SD/MMC DRIVER, linux-mmc, open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS, moderated list:ARM/ASPEED MACHINE SUPPORT, open list Cc: Hongwei Zhang, Ryan Chen, Chin-Ting Kuo 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 ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v2 1/3] dt-bindings: mmc: sdhci-of-aspeed: Add description for AST2600 EVB. 2021-05-03 4:19 ` Andrew Jeffery @ 2021-05-03 9:40 ` Steven Lee 2021-05-03 10:32 ` Andrew Jeffery 0 siblings, 1 reply; 14+ messages in thread From: Steven Lee @ 2021-05-03 9:40 UTC (permalink / raw) To: Andrew Jeffery Cc: Ulf Hansson, Rob Herring, Joel Stanley, Ryan Chen, moderated list:ASPEED SD/MMC DRIVER, moderated list:ASPEED SD/MMC DRIVER, linux-mmc, open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS, moderated list:ARM/ASPEED MACHINE SUPPORT, open list, Hongwei Zhang, Ryan Chen, Chin-Ting Kuo 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 ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v2 1/3] dt-bindings: mmc: sdhci-of-aspeed: Add description for AST2600 EVB. 2021-05-03 9:40 ` Steven Lee @ 2021-05-03 10:32 ` Andrew Jeffery 2021-05-03 11:08 ` Steven Lee 0 siblings, 1 reply; 14+ messages in thread From: Andrew Jeffery @ 2021-05-03 10:32 UTC (permalink / raw) To: Steven Lee Cc: Ulf Hansson, Rob Herring, Joel Stanley, Ryan Chen, moderated list:ASPEED SD/MMC DRIVER, moderated list:ASPEED SD/MMC DRIVER, linux-mmc, open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS, moderated list:ARM/ASPEED MACHINE SUPPORT, open list, Hongwei Zhang, Ryan Chen, Chin-Ting Kuo 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 ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v2 1/3] dt-bindings: mmc: sdhci-of-aspeed: Add description for AST2600 EVB. 2021-05-03 10:32 ` Andrew Jeffery @ 2021-05-03 11:08 ` Steven Lee 0 siblings, 0 replies; 14+ messages in thread From: Steven Lee @ 2021-05-03 11:08 UTC (permalink / raw) To: Andrew Jeffery Cc: Ulf Hansson, Rob Herring, Joel Stanley, Ryan Chen, moderated list:ASPEED SD/MMC DRIVER, moderated list:ASPEED SD/MMC DRIVER, linux-mmc, open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS, moderated list:ARM/ASPEED MACHINE SUPPORT, open list, Hongwei Zhang, Ryan Chen, Chin-Ting Kuo 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 ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v2 1/3] dt-bindings: mmc: sdhci-of-aspeed: Add description for AST2600 EVB. 2021-05-03 1:43 ` [PATCH v2 1/3] dt-bindings: mmc: sdhci-of-aspeed: Add description for AST2600 EVB Steven Lee 2021-05-03 4:19 ` Andrew Jeffery @ 2021-05-03 15:20 ` Rob Herring 2021-05-04 1:46 ` Steven Lee 1 sibling, 1 reply; 14+ messages in thread From: Rob Herring @ 2021-05-03 15:20 UTC (permalink / raw) To: Steven Lee Cc: moderated list:ASPEED SD/MMC DRIVER, Ryan Chen, open list, open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS, ryan_chen, moderated list:ASPEED SD/MMC DRIVER, moderated list:ARM/ASPEED MACHINE SUPPORT, open list:ASPEED SD/MMC DRIVER, Andrew Jeffery, Rob Herring, chin-ting_kuo, Ulf Hansson, Joel Stanley, Hongweiz 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 ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v2 1/3] dt-bindings: mmc: sdhci-of-aspeed: Add description for AST2600 EVB. 2021-05-03 15:20 ` Rob Herring @ 2021-05-04 1:46 ` Steven Lee 0 siblings, 0 replies; 14+ messages in thread From: Steven Lee @ 2021-05-04 1:46 UTC (permalink / raw) To: Rob Herring Cc: moderated list:ASPEED SD/MMC DRIVER, Ryan Chen, open list, open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS, Ryan Chen, moderated list:ASPEED SD/MMC DRIVER, moderated list:ARM/ASPEED MACHINE SUPPORT, open list:ASPEED SD/MMC DRIVER, Andrew Jeffery, Rob Herring, Chin-Ting Kuo, Ulf Hansson, Joel Stanley, Hongweiz 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 ^ permalink raw reply [flat|nested] 14+ messages in thread
* [PATCH v2 2/3] ARM: dts: aspeed: ast2600evb: Add timing-phase property for eMMC controller [not found] <20210503014336.20256-1-steven_lee@aspeedtech.com> 2021-05-03 1:43 ` [PATCH v2 1/3] dt-bindings: mmc: sdhci-of-aspeed: Add description for AST2600 EVB Steven Lee @ 2021-05-03 1:43 ` Steven Lee 2021-05-03 5:07 ` Andrew Jeffery 2021-05-03 1:43 ` [PATCH v2 3/3] mmc: sdhci-of-aspeed: Sync capabilities from device tree to ast2600 SoC registers Steven Lee 2 siblings, 1 reply; 14+ messages in thread From: Steven Lee @ 2021-05-03 1:43 UTC (permalink / raw) To: Rob Herring, Joel Stanley, Andrew Jeffery, open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS, moderated list:ARM/ASPEED MACHINE SUPPORT, moderated list:ARM/ASPEED MACHINE SUPPORT, open list Cc: Hongweiz, ryan_chen, chin-ting_kuo 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 ^ permalink raw reply related [flat|nested] 14+ messages in thread
* Re: [PATCH v2 2/3] ARM: dts: aspeed: ast2600evb: Add timing-phase property for eMMC controller 2021-05-03 1:43 ` [PATCH v2 2/3] ARM: dts: aspeed: ast2600evb: Add timing-phase property for eMMC controller Steven Lee @ 2021-05-03 5:07 ` Andrew Jeffery 2021-05-03 10:58 ` Steven Lee 0 siblings, 1 reply; 14+ messages in thread From: Andrew Jeffery @ 2021-05-03 5:07 UTC (permalink / raw) To: Steven Lee, Rob Herring, Joel Stanley, open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS, moderated list:ARM/ASPEED MACHINE SUPPORT, moderated list:ARM/ASPEED MACHINE SUPPORT, open list Cc: Hongwei Zhang, Ryan Chen, Chin-Ting Kuo 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 ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v2 2/3] ARM: dts: aspeed: ast2600evb: Add timing-phase property for eMMC controller 2021-05-03 5:07 ` Andrew Jeffery @ 2021-05-03 10:58 ` Steven Lee 0 siblings, 0 replies; 14+ messages in thread From: Steven Lee @ 2021-05-03 10:58 UTC (permalink / raw) To: Andrew Jeffery Cc: Rob Herring, Joel Stanley, open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS, moderated list:ARM/ASPEED MACHINE SUPPORT, moderated list:ARM/ASPEED MACHINE SUPPORT, open list, Hongwei Zhang, Ryan Chen, Chin-Ting Kuo 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 ^ permalink raw reply [flat|nested] 14+ messages in thread
* [PATCH v2 3/3] mmc: sdhci-of-aspeed: Sync capabilities from device tree to ast2600 SoC registers [not found] <20210503014336.20256-1-steven_lee@aspeedtech.com> 2021-05-03 1:43 ` [PATCH v2 1/3] dt-bindings: mmc: sdhci-of-aspeed: Add description for AST2600 EVB Steven Lee 2021-05-03 1:43 ` [PATCH v2 2/3] ARM: dts: aspeed: ast2600evb: Add timing-phase property for eMMC controller Steven Lee @ 2021-05-03 1:43 ` Steven Lee 2021-05-03 5:04 ` Andrew Jeffery 2 siblings, 1 reply; 14+ messages in thread From: Steven Lee @ 2021-05-03 1:43 UTC (permalink / raw) To: Adrian Hunter, Andrew Jeffery, Ulf Hansson, Joel Stanley, Philipp Zabel, moderated list:ASPEED SD/MMC DRIVER, moderated list:ASPEED SD/MMC DRIVER, moderated list:ARM/ASPEED MACHINE SUPPORT, open list Cc: Hongweiz, ryan_chen, chin-ting_kuo 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 ^ permalink raw reply related [flat|nested] 14+ messages in thread
* Re: [PATCH v2 3/3] mmc: sdhci-of-aspeed: Sync capabilities from device tree to ast2600 SoC registers 2021-05-03 1:43 ` [PATCH v2 3/3] mmc: sdhci-of-aspeed: Sync capabilities from device tree to ast2600 SoC registers Steven Lee @ 2021-05-03 5:04 ` Andrew Jeffery 2021-05-03 10:52 ` Steven Lee 0 siblings, 1 reply; 14+ messages in thread From: Andrew Jeffery @ 2021-05-03 5:04 UTC (permalink / raw) To: Steven Lee, Adrian Hunter, Ulf Hansson, Joel Stanley, Philipp Zabel, moderated list:ASPEED SD/MMC DRIVER, moderated list:ASPEED SD/MMC DRIVER, moderated list:ARM/ASPEED MACHINE SUPPORT, open list Cc: Chin-Ting Kuo, Ryan Chen, Hongwei Zhang 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 ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v2 3/3] mmc: sdhci-of-aspeed: Sync capabilities from device tree to ast2600 SoC registers 2021-05-03 5:04 ` Andrew Jeffery @ 2021-05-03 10:52 ` Steven Lee 2021-05-03 11:15 ` Andrew Jeffery 0 siblings, 1 reply; 14+ messages in thread From: Steven Lee @ 2021-05-03 10:52 UTC (permalink / raw) To: Andrew Jeffery Cc: Adrian Hunter, Ulf Hansson, Joel Stanley, Philipp Zabel, moderated list:ASPEED SD/MMC DRIVER, moderated list:ASPEED SD/MMC DRIVER, moderated list:ARM/ASPEED MACHINE SUPPORT, open list, Chin-Ting Kuo, Ryan Chen, Hongwei Zhang 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 ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v2 3/3] mmc: sdhci-of-aspeed: Sync capabilities from device tree to ast2600 SoC registers 2021-05-03 10:52 ` Steven Lee @ 2021-05-03 11:15 ` Andrew Jeffery 0 siblings, 0 replies; 14+ messages in thread From: Andrew Jeffery @ 2021-05-03 11:15 UTC (permalink / raw) To: Steven Lee Cc: Adrian Hunter, Ulf Hansson, Joel Stanley, Philipp Zabel, moderated list:ASPEED SD/MMC DRIVER, moderated list:ASPEED SD/MMC DRIVER, moderated list:ARM/ASPEED MACHINE SUPPORT, open list, Chin-Ting Kuo, Ryan Chen, Hongwei Zhang 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 ^ permalink raw reply [flat|nested] 14+ messages in thread
end of thread, other threads:[~2021-05-04 1:49 UTC | newest] Thread overview: 14+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- [not found] <20210503014336.20256-1-steven_lee@aspeedtech.com> 2021-05-03 1:43 ` [PATCH v2 1/3] dt-bindings: mmc: sdhci-of-aspeed: Add description for AST2600 EVB Steven Lee 2021-05-03 4:19 ` Andrew Jeffery 2021-05-03 9:40 ` Steven Lee 2021-05-03 10:32 ` Andrew Jeffery 2021-05-03 11:08 ` Steven Lee 2021-05-03 15:20 ` Rob Herring 2021-05-04 1:46 ` Steven Lee 2021-05-03 1:43 ` [PATCH v2 2/3] ARM: dts: aspeed: ast2600evb: Add timing-phase property for eMMC controller Steven Lee 2021-05-03 5:07 ` Andrew Jeffery 2021-05-03 10:58 ` Steven Lee 2021-05-03 1:43 ` [PATCH v2 3/3] mmc: sdhci-of-aspeed: Sync capabilities from device tree to ast2600 SoC registers Steven Lee 2021-05-03 5:04 ` Andrew Jeffery 2021-05-03 10:52 ` Steven Lee 2021-05-03 11:15 ` Andrew Jeffery
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).