[PATCH v4 0/3] mmc: sdhci-of-aspeed: Support toggling SD bus signal

[PATCH v4 0/3] mmc: sdhci-of-aspeed: Support toggling SD bus signal

[Steven Lee]

AST2600-A2 EVB has the reference design for enabling SD bus
power and toggling SD bus signal voltage between 3.3v and 1.8v by
GPIO regulators.
This patch series adds sdhci node and gpio regulators in a new dts file
for AST2600-A2 EVB.
The description of the reference design of AST2600-A2 EVB is added
in the new dts file.

This patch also include a helper for updating AST2600 sdhci capability
registers.

Changes from v3:
* Remove the example of gpio regulator from dt-bindings.
* Add sdhci node and gpio regulators to a new dts file.
* Move the comment of the reference design to the new
  dts file.
* Modify commit message of sdhci-of-aspeed.c.
* Fix coding style issues of sdhci-of-aspeed.c.
* Remove the implementation of eMMC resetc since it has no relevance to
  the goal that this patch series want to achieve and it may needs further
  discussion about the design of reset behavior.

Changes from v2:
* Move the comment of the reference design from dt-bindings to device tree.
* Add clk-phase binding for eMMC controller.
* Reimplement aspeed_sdc_set_slot_capability().
* Separate the implementation of eMMC reset to another patch file.
* Fix yaml document error per the report of dt_binding_check and
  dtbs_check.

Changes from v1:
* Add the device tree example for AST2600 A2 EVB in dt-bindings
  document
* Add timing-phase for eMMC controller.
* Remove power-gpio and power-switch-gpio from sdhci driver, they should
  be handled by regulator.
* Add a helper to update capability registers in the driver.
* Sync sdhci settings from device tree to SoC capability registers.
* Sync timing-phase from device tree to SoC Clock Phase Control
  register

Please help to review.

Regards,
Steven

Steven Lee (3):
  ARM: dts: aspeed: ast2600evb: Add sdhci node and gpio regulator for A2
    evb.
  ARM: dts: aspeed: ast2600evb: Add phase correction for emmc
    controller.
  mmc: sdhci-of-aspeed: Configure the SDHCIs as specified by the
    devicetree.

 arch/arm/boot/dts/aspeed-ast2600-evb-a2.dts | 98 +++++++++++++++++++++
 arch/arm/boot/dts/aspeed-ast2600-evb.dts    |  3 +-
 drivers/mmc/host/sdhci-of-aspeed.c          | 48 ++++++++++
 3 files changed, 148 insertions(+), 1 deletion(-)
 create mode 100644 arch/arm/boot/dts/aspeed-ast2600-evb-a2.dts

-- 
2.17.1


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

[PATCH v4 1/3] ARM: dts: aspeed: ast2600evb: Add sdhci node and gpio regulator for A2 evb.

[Steven Lee]

AST2600 A2(or newer) EVB has gpio regulators for toggling signal voltage
between 3.3v and 1.8v, the patch adds sdhci node and gpio regulator in the
new dts file and adds commment for describing the reference design.

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

diff --git a/arch/arm/boot/dts/aspeed-ast2600-evb-a2.dts b/arch/arm/boot/dts/aspeed-ast2600-evb-a2.dts
new file mode 100644
index 000000000000..d581e8069a82
--- /dev/null
+++ b/arch/arm/boot/dts/aspeed-ast2600-evb-a2.dts
@@ -0,0 +1,98 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
+// Copyright 2021 IBM Corp.
+
+#include "aspeed-ast2600-evb.dts"
+#include <dt-bindings/gpio/gpio.h>
+
+/ {
+	model = "AST2600 A2 EVB";
+	compatible = "aspeed,ast2600";
+
+	vcc_sdhci0: regulator-vcc-sdhci0 {
+		compatible = "regulator-fixed";
+		regulator-name = "SDHCI0 Vcc";
+		regulator-min-microvolt = <3300000>;
+		regulator-max-microvolt = <3300000>;
+		gpios = <&gpio0 168
+			 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 169
+			 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 170
+			 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 171
+			 GPIO_ACTIVE_HIGH>;
+		gpios-states = <1>;
+		states = <3300000 1>,
+			 <1800000 0>;
+	};
+};
+
+&sdc {
+	status = "okay";
+};
+
+/*
+ * The signal voltage of sdhci0 and sdhci1 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 sdhci0 vdd, GPIOV1 is connected to
+ * a 1.8v and a 3.3v power load switch that providing signal voltage to
+ * sdhci0 bus.
+ * If GPIOV0 is active high, sdhci0 is enabled, otherwise, sdhci0 is disabled.
+ * If GPIOV1 is active high, 3.3v power load switch is enabled, sdhci0 signal
+ * voltage is 3.3v, otherwise, 1.8v power load switch will be enabled,
+ * sdhci0 signal voltage becomes 1.8v.
+ * AST2600-A2 EVB also support toggling signal voltage for sdhci1.
+ * The design is the same as sdhci0, it uses GPIOV2 as power-gpio and GPIOV3
+ * as power-switch-gpio.
+ */
+&sdhci0 {
+	status = "okay";
+	bus-width = <4>;
+	max-frequency = <100000000>;
+	sdhci-drive-type = /bits/ 8 <3>;
+	sdhci-caps-mask = <0x7 0x0>;
+	sdhci,wp-inverted;
+	vmmc-supply = <&vcc_sdhci0>;
+	vqmmc-supply = <&vccq_sdhci0>;
+	clk-phase-sd-hs = <7>, <200>;
+};
+
+&sdhci1 {
+	status = "okay";
+	bus-width = <4>;
+	max-frequency = <100000000>;
+	sdhci-drive-type = /bits/ 8 <3>;
+	sdhci-caps-mask = <0x7 0x0>;
+	sdhci,wp-inverted;
+	vmmc-supply = <&vcc_sdhci1>;
+	vqmmc-supply = <&vccq_sdhci1>;
+	clk-phase-sd-hs = <7>, <200>;
+};
+
-- 
2.17.1


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

Re: [PATCH v4 1/3] ARM: dts: aspeed: ast2600evb: Add sdhci node and gpio regulator for A2 evb.

[Joel Stanley]

Hi Steven,

On Thu, 20 May 2021 at 10:16, Steven Lee <steven_lee@aspeedtech.com> wrote:
>
> AST2600 A2(or newer) EVB has gpio regulators for toggling signal voltage
> between 3.3v and 1.8v, the patch adds sdhci node and gpio regulator in the
> new dts file and adds commment for describing the reference design.

spelling: comment

I need you to justify the separate dts for the A2 EVB.

What would happen if this device tree was loaded on to an A1 or A0?

Would this device tree be used for the A3 (and any future revision?)

An alternative proposal: we modify the ast2600-evb.dts to support the
A2 (which I assume would also support the A3).

If we need a separate board file for the A0 and A1 EVB, we add a new
one that supports these earlier revisions. Or we decide to only
support the latest revision in mainline.

>
> Signed-off-by: Steven Lee <steven_lee@aspeedtech.com>
> ---
>  arch/arm/boot/dts/aspeed-ast2600-evb-a2.dts | 98 +++++++++++++++++++++
>  1 file changed, 98 insertions(+)
>  create mode 100644 arch/arm/boot/dts/aspeed-ast2600-evb-a2.dts
>
> diff --git a/arch/arm/boot/dts/aspeed-ast2600-evb-a2.dts b/arch/arm/boot/dts/aspeed-ast2600-evb-a2.dts
> new file mode 100644
> index 000000000000..d581e8069a82
> --- /dev/null
> +++ b/arch/arm/boot/dts/aspeed-ast2600-evb-a2.dts
> @@ -0,0 +1,98 @@
> +// SPDX-License-Identifier: GPL-2.0-or-later
> +// Copyright 2021 IBM Corp.
> +
> +#include "aspeed-ast2600-evb.dts"
> +#include <dt-bindings/gpio/gpio.h>
> +
> +/ {
> +       model = "AST2600 A2 EVB";
> +       compatible = "aspeed,ast2600";

Will this override the "aspeed,ast2600-evb" compatible in the dts? I
think you can drop the compatible string here and just use the one
from the DTS.

> +
> +       vcc_sdhci0: regulator-vcc-sdhci0 {
> +               compatible = "regulator-fixed";
> +               regulator-name = "SDHCI0 Vcc";
> +               regulator-min-microvolt = <3300000>;
> +               regulator-max-microvolt = <3300000>;
> +               gpios = <&gpio0 168

We have macros for describing the GPIOs:

ASPEED_GPIO(V, 0)

> +                        GPIO_ACTIVE_HIGH>;

This can go on one line.

> +               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 169
> +                        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 170
> +                        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 171
> +                        GPIO_ACTIVE_HIGH>;
> +               gpios-states = <1>;
> +               states = <3300000 1>,
> +                        <1800000 0>;
> +       };
> +};
> +
> +&sdc {
> +       status = "okay";
> +};
> +
> +/*
> + * The signal voltage of sdhci0 and sdhci1 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 sdhci0 vdd, GPIOV1 is connected to
> + * a 1.8v and a 3.3v power load switch that providing signal voltage to

nit: provides

> + * sdhci0 bus.
> + * If GPIOV0 is active high, sdhci0 is enabled, otherwise, sdhci0 is disabled.
> + * If GPIOV1 is active high, 3.3v power load switch is enabled, sdhci0 signal
> + * voltage is 3.3v, otherwise, 1.8v power load switch will be enabled,
> + * sdhci0 signal voltage becomes 1.8v.
> + * AST2600-A2 EVB also support toggling signal voltage for sdhci1.

nit: supports

> + * The design is the same as sdhci0, it uses GPIOV2 as power-gpio and GPIOV3
> + * as power-switch-gpio.
> + */
> +&sdhci0 {
> +       status = "okay";
> +       bus-width = <4>;
> +       max-frequency = <100000000>;
> +       sdhci-drive-type = /bits/ 8 <3>;
> +       sdhci-caps-mask = <0x7 0x0>;
> +       sdhci,wp-inverted;
> +       vmmc-supply = <&vcc_sdhci0>;
> +       vqmmc-supply = <&vccq_sdhci0>;
> +       clk-phase-sd-hs = <7>, <200>;
> +};
> +
> +&sdhci1 {
> +       status = "okay";
> +       bus-width = <4>;
> +       max-frequency = <100000000>;
> +       sdhci-drive-type = /bits/ 8 <3>;
> +       sdhci-caps-mask = <0x7 0x0>;
> +       sdhci,wp-inverted;
> +       vmmc-supply = <&vcc_sdhci1>;
> +       vqmmc-supply = <&vccq_sdhci1>;
> +       clk-phase-sd-hs = <7>, <200>;
> +};
> +
> --
> 2.17.1
>

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

Re: [PATCH v4 1/3] ARM: dts: aspeed: ast2600evb: Add sdhci node and gpio regulator for A2 evb.

[Steven Lee]

The 05/21/2021 09:25, Joel Stanley wrote:
> Hi Steven,
> 
> On Thu, 20 May 2021 at 10:16, Steven Lee <steven_lee@aspeedtech.com> wrote:
> >
> > AST2600 A2(or newer) EVB has gpio regulators for toggling signal voltage
> > between 3.3v and 1.8v, the patch adds sdhci node and gpio regulator in the
> > new dts file and adds commment for describing the reference design.
> 
> spelling: comment
> 

Thanks, will correct the typo.

> I need you to justify the separate dts for the A2 EVB.
> 
> What would happen if this device tree was loaded on to an A1 or A0?
> 

Since the clock default value(SCU210) of A1 and A0 are different to A2,
the following error would happen if A2 device tree was loaded on A1/A0.

```
[  133.179825] mmc1: Reset 0x4 never completed.
[  133.184599] mmc1: sdhci: ============ SDHCI REGISTER DUMP ===========
[  133.191786] mmc1: sdhci: Sys addr:  0x00000000 | Version:  0x00000002
[  133.198972] mmc1: sdhci: Blk size:  0x00007008 | Blk cnt:  0x00000001
[  133.206158] mmc1: sdhci: Argument:  0x00000c00 | Trn mode: 0x00000013
[  133.213343] mmc1: sdhci: Present:   0x01f70001 | Host ctl: 0x00000011
[  133.220528] mmc1: sdhci: Power:     0x0000000f | Blk gap:  0x00000000
[  133.227713] mmc1: sdhci: Wake-up:   0x00000000 | Clock:    0x00008007
[  133.234898] mmc1: sdhci: Timeout:   0x0000000b | Int stat: 0x00000000
[  133.242083] mmc1: sdhci: Int enab:  0x00ff0083 | Sig enab: 0x00ff0083
[  133.249268] mmc1: sdhci: ACmd stat: 0x00000000 | Slot int: 0x00000000
[  133.256453] mmc1: sdhci: Caps:      0x07f80080 | Caps_1:   0x00000007
[  133.263638] mmc1: sdhci: Cmd:       0x0000341a | Max curr: 0x001f0f08
[  133.270824] mmc1: sdhci: Resp[0]:   0x00000000 | Resp[1]:  0x01dd7f7f
[  133.278009] mmc1: sdhci: Resp[2]:   0x325b5900 | Resp[3]:  0x00400e00
[  133.285193] mmc1: sdhci: Host ctl2: 0x00000000
[  133.290148] mmc1: sdhci: ADMA Err:  0x00000000 | ADMA Ptr: 0xbe041200
[  133.297332] mmc1: sdhci: ============================================

```

Besides, A1/A0 EVBs don't have regulator, vmmc and vqmmc should be
removed from sdhci node of A1/A0 dts.

> Would this device tree be used for the A3 (and any future revision?)
> 

Yes, A3 can use the A2 dts.

> An alternative proposal: we modify the ast2600-evb.dts to support the
> A2 (which I assume would also support the A3).
> 
> If we need a separate board file for the A0 and A1 EVB, we add a new
> one that supports these earlier revisions. Or we decide to only
> support the latest revision in mainline.
> 

In this patch, I add a new dts to support A2 sdhci, and include the
original dts since the other settings can be loaded on A2.
Do you mean creating a new file(e.g. aspeed-ast2600-evb-a1.dts) for A1,
and modifying the original aspeed-ast2600-evb.dts for supporting A2?

If we decide to only support the latest version in mainline, users
should mark vmmc and vqmmc as comment and modify clk-phase manually
for supporting A1.

> >
> > Signed-off-by: Steven Lee <steven_lee@aspeedtech.com>
> > ---
> >  arch/arm/boot/dts/aspeed-ast2600-evb-a2.dts | 98 +++++++++++++++++++++
> >  1 file changed, 98 insertions(+)
> >  create mode 100644 arch/arm/boot/dts/aspeed-ast2600-evb-a2.dts
> >
> > diff --git a/arch/arm/boot/dts/aspeed-ast2600-evb-a2.dts b/arch/arm/boot/dts/aspeed-ast2600-evb-a2.dts
> > new file mode 100644
> > index 000000000000..d581e8069a82
> > --- /dev/null
> > +++ b/arch/arm/boot/dts/aspeed-ast2600-evb-a2.dts
> > @@ -0,0 +1,98 @@
> > +// SPDX-License-Identifier: GPL-2.0-or-later
> > +// Copyright 2021 IBM Corp.
> > +
> > +#include "aspeed-ast2600-evb.dts"
> > +#include <dt-bindings/gpio/gpio.h>
> > +
> > +/ {
> > +       model = "AST2600 A2 EVB";
> > +       compatible = "aspeed,ast2600";
> 
> Will this override the "aspeed,ast2600-evb" compatible in the dts? I
> think you can drop the compatible string here and just use the one
> from the DTS.
> 

Thanks for review, I will remove it.

> > +
> > +       vcc_sdhci0: regulator-vcc-sdhci0 {
> > +               compatible = "regulator-fixed";
> > +               regulator-name = "SDHCI0 Vcc";
> > +               regulator-min-microvolt = <3300000>;
> > +               regulator-max-microvolt = <3300000>;
> > +               gpios = <&gpio0 168
> 
> We have macros for describing the GPIOs:
> 
> ASPEED_GPIO(V, 0)
> 
> > +                        GPIO_ACTIVE_HIGH>;
> 
> This can go on one line.
> 
> > +               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 169
> > +                        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 170
> > +                        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 171
> > +                        GPIO_ACTIVE_HIGH>;
> > +               gpios-states = <1>;
> > +               states = <3300000 1>,
> > +                        <1800000 0>;
> > +       };
> > +};
> > +
> > +&sdc {
> > +       status = "okay";
> > +};
> > +
> > +/*
> > + * The signal voltage of sdhci0 and sdhci1 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 sdhci0 vdd, GPIOV1 is connected to
> > + * a 1.8v and a 3.3v power load switch that providing signal voltage to
> 
> nit: provides
> 

Will modify it.

> > + * sdhci0 bus.
> > + * If GPIOV0 is active high, sdhci0 is enabled, otherwise, sdhci0 is disabled.
> > + * If GPIOV1 is active high, 3.3v power load switch is enabled, sdhci0 signal
> > + * voltage is 3.3v, otherwise, 1.8v power load switch will be enabled,
> > + * sdhci0 signal voltage becomes 1.8v.
> > + * AST2600-A2 EVB also support toggling signal voltage for sdhci1.
> 
> nit: supports
> 

Will modify it.

> > + * The design is the same as sdhci0, it uses GPIOV2 as power-gpio and GPIOV3
> > + * as power-switch-gpio.
> > + */
> > +&sdhci0 {
> > +       status = "okay";
> > +       bus-width = <4>;
> > +       max-frequency = <100000000>;
> > +       sdhci-drive-type = /bits/ 8 <3>;
> > +       sdhci-caps-mask = <0x7 0x0>;
> > +       sdhci,wp-inverted;
> > +       vmmc-supply = <&vcc_sdhci0>;
> > +       vqmmc-supply = <&vccq_sdhci0>;
> > +       clk-phase-sd-hs = <7>, <200>;
> > +};
> > +
> > +&sdhci1 {
> > +       status = "okay";
> > +       bus-width = <4>;
> > +       max-frequency = <100000000>;
> > +       sdhci-drive-type = /bits/ 8 <3>;
> > +       sdhci-caps-mask = <0x7 0x0>;
> > +       sdhci,wp-inverted;
> > +       vmmc-supply = <&vcc_sdhci1>;
> > +       vqmmc-supply = <&vccq_sdhci1>;
> > +       clk-phase-sd-hs = <7>, <200>;
> > +};
> > +
> > --
> > 2.17.1
> >

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

Re: [PATCH v4 1/3] ARM: dts: aspeed: ast2600evb: Add sdhci node and gpio regulator for A2 evb.

[Joel Stanley]

On Mon, 24 May 2021 at 02:35, Steven Lee <steven_lee@aspeedtech.com> wrote:
>
> The 05/21/2021 09:25, Joel Stanley wrote:
> > Hi Steven,
> >
> > On Thu, 20 May 2021 at 10:16, Steven Lee <steven_lee@aspeedtech.com> wrote:
> > >
> > > AST2600 A2(or newer) EVB has gpio regulators for toggling signal voltage
> > > between 3.3v and 1.8v, the patch adds sdhci node and gpio regulator in the
> > > new dts file and adds commment for describing the reference design.
> >
> > spelling: comment
> >
>
> Thanks, will correct the typo.
>
> > I need you to justify the separate dts for the A2 EVB.
> >
> > What would happen if this device tree was loaded on to an A1 or A0?
> >
>
> Since the clock default value(SCU210) of A1 and A0 are different to A2,
> the following error would happen if A2 device tree was loaded on A1/A0.
>
> ```
> [  133.179825] mmc1: Reset 0x4 never completed.
> [  133.184599] mmc1: sdhci: ============ SDHCI REGISTER DUMP ===========
> [  133.191786] mmc1: sdhci: Sys addr:  0x00000000 | Version:  0x00000002
> [  133.198972] mmc1: sdhci: Blk size:  0x00007008 | Blk cnt:  0x00000001
> [  133.206158] mmc1: sdhci: Argument:  0x00000c00 | Trn mode: 0x00000013
> [  133.213343] mmc1: sdhci: Present:   0x01f70001 | Host ctl: 0x00000011
> [  133.220528] mmc1: sdhci: Power:     0x0000000f | Blk gap:  0x00000000
> [  133.227713] mmc1: sdhci: Wake-up:   0x00000000 | Clock:    0x00008007
> [  133.234898] mmc1: sdhci: Timeout:   0x0000000b | Int stat: 0x00000000
> [  133.242083] mmc1: sdhci: Int enab:  0x00ff0083 | Sig enab: 0x00ff0083
> [  133.249268] mmc1: sdhci: ACmd stat: 0x00000000 | Slot int: 0x00000000
> [  133.256453] mmc1: sdhci: Caps:      0x07f80080 | Caps_1:   0x00000007
> [  133.263638] mmc1: sdhci: Cmd:       0x0000341a | Max curr: 0x001f0f08
> [  133.270824] mmc1: sdhci: Resp[0]:   0x00000000 | Resp[1]:  0x01dd7f7f
> [  133.278009] mmc1: sdhci: Resp[2]:   0x325b5900 | Resp[3]:  0x00400e00
> [  133.285193] mmc1: sdhci: Host ctl2: 0x00000000
> [  133.290148] mmc1: sdhci: ADMA Err:  0x00000000 | ADMA Ptr: 0xbe041200
> [  133.297332] mmc1: sdhci: ============================================
>
> ```
>
> Besides, A1/A0 EVBs don't have regulator, vmmc and vqmmc should be
> removed from sdhci node of A1/A0 dts.
>
> > Would this device tree be used for the A3 (and any future revision?)
> >
>
> Yes, A3 can use the A2 dts.
>
> > An alternative proposal: we modify the ast2600-evb.dts to support the
> > A2 (which I assume would also support the A3).
> >
> > If we need a separate board file for the A0 and A1 EVB, we add a new
> > one that supports these earlier revisions. Or we decide to only
> > support the latest revision in mainline.
> >
>
> In this patch, I add a new dts to support A2 sdhci, and include the
> original dts since the other settings can be loaded on A2.
> Do you mean creating a new file(e.g. aspeed-ast2600-evb-a1.dts) for A1,
> and modifying the original aspeed-ast2600-evb.dts for supporting A2?

Yes, that would be my suggestion. The aspeed-ast2600-evb-a1.dts could
include the aspeed-ast2600-evb.dts.

> If we decide to only support the latest version in mainline, users
> should mark vmmc and vqmmc as comment and modify clk-phase manually
> for supporting A1.

If you believe there will be users of the A1 for some time, then I
think it makes sense to support both A1 and future boards in mainline.

Cheers,

Joel

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

Re: [PATCH v4 1/3] ARM: dts: aspeed: ast2600evb: Add sdhci node and gpio regulator for A2 evb.

[Steven Lee]

The 05/24/2021 10:46, Joel Stanley wrote:
> On Mon, 24 May 2021 at 02:35, Steven Lee <steven_lee@aspeedtech.com> wrote:
> >
> > The 05/21/2021 09:25, Joel Stanley wrote:
> > > Hi Steven,
> > >
> > > On Thu, 20 May 2021 at 10:16, Steven Lee <steven_lee@aspeedtech.com> wrote:
> > > >
> > > > AST2600 A2(or newer) EVB has gpio regulators for toggling signal voltage
> > > > between 3.3v and 1.8v, the patch adds sdhci node and gpio regulator in the
> > > > new dts file and adds commment for describing the reference design.
> > >
> > > spelling: comment
> > >
> >
> > Thanks, will correct the typo.
> >
> > > I need you to justify the separate dts for the A2 EVB.
> > >
> > > What would happen if this device tree was loaded on to an A1 or A0?
> > >
> >
> > Since the clock default value(SCU210) of A1 and A0 are different to A2,
> > the following error would happen if A2 device tree was loaded on A1/A0.
> >
> > ```
> > [  133.179825] mmc1: Reset 0x4 never completed.
> > [  133.184599] mmc1: sdhci: ============ SDHCI REGISTER DUMP ===========
> > [  133.191786] mmc1: sdhci: Sys addr:  0x00000000 | Version:  0x00000002
> > [  133.198972] mmc1: sdhci: Blk size:  0x00007008 | Blk cnt:  0x00000001
> > [  133.206158] mmc1: sdhci: Argument:  0x00000c00 | Trn mode: 0x00000013
> > [  133.213343] mmc1: sdhci: Present:   0x01f70001 | Host ctl: 0x00000011
> > [  133.220528] mmc1: sdhci: Power:     0x0000000f | Blk gap:  0x00000000
> > [  133.227713] mmc1: sdhci: Wake-up:   0x00000000 | Clock:    0x00008007
> > [  133.234898] mmc1: sdhci: Timeout:   0x0000000b | Int stat: 0x00000000
> > [  133.242083] mmc1: sdhci: Int enab:  0x00ff0083 | Sig enab: 0x00ff0083
> > [  133.249268] mmc1: sdhci: ACmd stat: 0x00000000 | Slot int: 0x00000000
> > [  133.256453] mmc1: sdhci: Caps:      0x07f80080 | Caps_1:   0x00000007
> > [  133.263638] mmc1: sdhci: Cmd:       0x0000341a | Max curr: 0x001f0f08
> > [  133.270824] mmc1: sdhci: Resp[0]:   0x00000000 | Resp[1]:  0x01dd7f7f
> > [  133.278009] mmc1: sdhci: Resp[2]:   0x325b5900 | Resp[3]:  0x00400e00
> > [  133.285193] mmc1: sdhci: Host ctl2: 0x00000000
> > [  133.290148] mmc1: sdhci: ADMA Err:  0x00000000 | ADMA Ptr: 0xbe041200
> > [  133.297332] mmc1: sdhci: ============================================
> >
> > ```
> >
> > Besides, A1/A0 EVBs don't have regulator, vmmc and vqmmc should be
> > removed from sdhci node of A1/A0 dts.
> >
> > > Would this device tree be used for the A3 (and any future revision?)
> > >
> >
> > Yes, A3 can use the A2 dts.
> >
> > > An alternative proposal: we modify the ast2600-evb.dts to support the
> > > A2 (which I assume would also support the A3).
> > >
> > > If we need a separate board file for the A0 and A1 EVB, we add a new
> > > one that supports these earlier revisions. Or we decide to only
> > > support the latest revision in mainline.
> > >
> >
> > In this patch, I add a new dts to support A2 sdhci, and include the
> > original dts since the other settings can be loaded on A2.
> > Do you mean creating a new file(e.g. aspeed-ast2600-evb-a1.dts) for A1,
> > and modifying the original aspeed-ast2600-evb.dts for supporting A2?
> 
> Yes, that would be my suggestion. The aspeed-ast2600-evb-a1.dts could
> include the aspeed-ast2600-evb.dts.
> 

Thanks for the prompt reply, I will add aspeed-ast2600-evb-a1.dts for
A1 and A0.

> > If we decide to only support the latest version in mainline, users
> > should mark vmmc and vqmmc as comment and modify clk-phase manually
> > for supporting A1.
> 
> If you believe there will be users of the A1 for some time, then I
> think it makes sense to support both A1 and future boards in mainline.
> 
> Cheers,
> 
> Joel

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

[PATCH v4 2/3] ARM: dts: aspeed: ast2600evb: Add phase correction for emmc controller.

[Steven Lee]

Set MMC timing-phase register by adding the phase correction binding in the
device tree.

Signed-off-by: Steven Lee <steven_lee@aspeedtech.com>
Acked-by: Andrew Jeffery <andrew@aj.id.au>
---
 arch/arm/boot/dts/aspeed-ast2600-evb.dts | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/arch/arm/boot/dts/aspeed-ast2600-evb.dts b/arch/arm/boot/dts/aspeed-ast2600-evb.dts
index 2772796e215e..2b8634513fe6 100644
--- a/arch/arm/boot/dts/aspeed-ast2600-evb.dts
+++ b/arch/arm/boot/dts/aspeed-ast2600-evb.dts
@@ -107,7 +107,8 @@
 &emmc {
 	non-removable;
 	bus-width = <4>;
-	max-frequency = <52000000>;
+	max-frequency = <100000000>;
+	clk-phase-mmc-hs200 = <9>, <225>;
 };
 
 &rtc {
-- 
2.17.1


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

Re: [PATCH v4 2/3] ARM: dts: aspeed: ast2600evb: Add phase correction for emmc controller.

[Joel Stanley]

On Thu, 20 May 2021 at 10:16, Steven Lee <steven_lee@aspeedtech.com> wrote:
>
> Set MMC timing-phase register by adding the phase correction binding in the
> device tree.
>
> Signed-off-by: Steven Lee <steven_lee@aspeedtech.com>
> Acked-by: Andrew Jeffery <andrew@aj.id.au>

As the aspeed maintainer:

Reviewed-by: Joel Stanley <joel@jms.id.au>

I don't mind if this gets applied directly by the mmc maintainers (I
do not anticpiate any conflicts).

Cheers,

Joel

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

[PATCH v4 3/3] mmc: sdhci-of-aspeed: Configure the SDHCIs as specified by the devicetree.

[Steven Lee]

The hardware provides capability configuration registers for each SDHCI
in the global configuration space for the SD controller. Writes to the
global capability registers are mirrored to the capability registers in
the associated SDHCI. Configuration of the capabilities must be written
through the mirror registers prior to initialisation of the SDHCI.

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

diff --git a/drivers/mmc/host/sdhci-of-aspeed.c b/drivers/mmc/host/sdhci-of-aspeed.c
index d001c51074a0..65b5685f6c15 100644
--- a/drivers/mmc/host/sdhci-of-aspeed.c
+++ b/drivers/mmc/host/sdhci-of-aspeed.c
@@ -31,6 +31,11 @@
 #define   ASPEED_SDC_S0_PHASE_OUT_EN	GENMASK(1, 0)
 #define   ASPEED_SDC_PHASE_MAX		31
 
+/* SDIO{10,20} */
+#define ASPEED_SDC_CAP1_1_8V           (0 * 32 + 26)
+/* SDIO{14,24} */
+#define ASPEED_SDC_CAP2_SDR104         (1 * 32 + 1)
+
 struct aspeed_sdc {
 	struct clk *clk;
 	struct resource *res;
@@ -72,6 +77,37 @@ struct aspeed_sdhci {
 	const struct aspeed_sdhci_phase_desc *phase_desc;
 };
 
+/*
+ * The function sets the mirror register for updating
+ * capbilities of the current slot.
+ *
+ *   slot | capability  | caps_reg | mirror_reg
+ *   -----|-------------|----------|------------
+ *     0  | CAP1_1_8V   | SDIO140  |   SDIO10
+ *     0  | CAP2_SDR104 | SDIO144  |   SDIO14
+ *     1  | CAP1_1_8V   | SDIO240  |   SDIO20
+ *     1  | CAP2_SDR104 | SDIO244  |   SDIO24
+ */
+static void aspeed_sdc_set_slot_capability(struct sdhci_host *host, struct aspeed_sdc *sdc,
+					   int capability, bool enable, u8 slot)
+{
+	u32 mirror_reg_offset;
+	u32 cap_val;
+	u8 cap_reg;
+
+	if (slot > 1)
+		return;
+
+	cap_reg = capability / 32;
+	cap_val = sdhci_readl(host, 0x40 + (cap_reg * 4));
+	if (enable)
+		cap_val |= BIT(capability % 32);
+	else
+		cap_val &= ~BIT(capability % 32);
+	mirror_reg_offset = ((slot + 1) * 0x10) + (cap_reg * 4);
+	writel(cap_val, sdc->regs + mirror_reg_offset);
+}
+
 static void aspeed_sdc_configure_8bit_mode(struct aspeed_sdc *sdc,
 					   struct aspeed_sdhci *sdhci,
 					   bool bus8)
@@ -328,6 +364,7 @@ static inline int aspeed_sdhci_calculate_slot(struct aspeed_sdhci *dev,
 static int aspeed_sdhci_probe(struct platform_device *pdev)
 {
 	const struct aspeed_sdhci_pdata *aspeed_pdata;
+	struct device_node *np = pdev->dev.of_node;
 	struct sdhci_pltfm_host *pltfm_host;
 	struct aspeed_sdhci *dev;
 	struct sdhci_host *host;
@@ -372,6 +409,17 @@ 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,
+					       true, slot);
+	}
+
+	if (of_property_read_bool(np, "sd-uhs-sdr104")) {
+		aspeed_sdc_set_slot_capability(host, dev->parent, ASPEED_SDC_CAP2_SDR104,
+					       true, slot);
+	}
+
 	pltfm_host->clk = devm_clk_get(&pdev->dev, NULL);
 	if (IS_ERR(pltfm_host->clk))
 		return PTR_ERR(pltfm_host->clk);
-- 
2.17.1


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

Re: [PATCH v4 3/3] mmc: sdhci-of-aspeed: Configure the SDHCIs as specified by the devicetree.

[Andrew Jeffery]

On Thu, 20 May 2021, at 19:43, Steven Lee wrote:
> The hardware provides capability configuration registers for each SDHCI
> in the global configuration space for the SD controller. Writes to the
> global capability registers are mirrored to the capability registers in
> the associated SDHCI. Configuration of the capabilities must be written
> through the mirror registers prior to initialisation of the SDHCI.
> 
> Signed-off-by: Steven Lee <steven_lee@aspeedtech.com>

Reviewed-by: Andrew Jeffery <andrew@aj.id.au>

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

Re: [PATCH v4 3/3] mmc: sdhci-of-aspeed: Configure the SDHCIs as specified by the devicetree.

[Joel Stanley]

On Thu, 20 May 2021 at 10:16, Steven Lee <steven_lee@aspeedtech.com> wrote:
>
> The hardware provides capability configuration registers for each SDHCI
> in the global configuration space for the SD controller. Writes to the
> global capability registers are mirrored to the capability registers in
> the associated SDHCI. Configuration of the capabilities must be written
> through the mirror registers prior to initialisation of the SDHCI.
>
> Signed-off-by: Steven Lee <steven_lee@aspeedtech.com>

Reviewed-by: Joel Stanley <joel@jms.id.au>

MMC maintainers, please ignore my other mail about taking the dts
patch through your tree. I didn't realise there were two dts patches;
I'll take them both through the aspeed tree once they are ready.
Please don't let that review stop you from applying this patch.

Cheers,

Joel

> ---
>  drivers/mmc/host/sdhci-of-aspeed.c | 48 ++++++++++++++++++++++++++++++
>  1 file changed, 48 insertions(+)
>
> diff --git a/drivers/mmc/host/sdhci-of-aspeed.c b/drivers/mmc/host/sdhci-of-aspeed.c
> index d001c51074a0..65b5685f6c15 100644
> --- a/drivers/mmc/host/sdhci-of-aspeed.c
> +++ b/drivers/mmc/host/sdhci-of-aspeed.c
> @@ -31,6 +31,11 @@
>  #define   ASPEED_SDC_S0_PHASE_OUT_EN   GENMASK(1, 0)
>  #define   ASPEED_SDC_PHASE_MAX         31
>
> +/* SDIO{10,20} */
> +#define ASPEED_SDC_CAP1_1_8V           (0 * 32 + 26)
> +/* SDIO{14,24} */
> +#define ASPEED_SDC_CAP2_SDR104         (1 * 32 + 1)
> +
>  struct aspeed_sdc {
>         struct clk *clk;
>         struct resource *res;
> @@ -72,6 +77,37 @@ struct aspeed_sdhci {
>         const struct aspeed_sdhci_phase_desc *phase_desc;
>  };
>
> +/*
> + * The function sets the mirror register for updating
> + * capbilities of the current slot.
> + *
> + *   slot | capability  | caps_reg | mirror_reg
> + *   -----|-------------|----------|------------
> + *     0  | CAP1_1_8V   | SDIO140  |   SDIO10
> + *     0  | CAP2_SDR104 | SDIO144  |   SDIO14
> + *     1  | CAP1_1_8V   | SDIO240  |   SDIO20
> + *     1  | CAP2_SDR104 | SDIO244  |   SDIO24
> + */
> +static void aspeed_sdc_set_slot_capability(struct sdhci_host *host, struct aspeed_sdc *sdc,
> +                                          int capability, bool enable, u8 slot)
> +{
> +       u32 mirror_reg_offset;
> +       u32 cap_val;
> +       u8 cap_reg;
> +
> +       if (slot > 1)
> +               return;
> +
> +       cap_reg = capability / 32;
> +       cap_val = sdhci_readl(host, 0x40 + (cap_reg * 4));
> +       if (enable)
> +               cap_val |= BIT(capability % 32);
> +       else
> +               cap_val &= ~BIT(capability % 32);
> +       mirror_reg_offset = ((slot + 1) * 0x10) + (cap_reg * 4);
> +       writel(cap_val, sdc->regs + mirror_reg_offset);
> +}
> +
>  static void aspeed_sdc_configure_8bit_mode(struct aspeed_sdc *sdc,
>                                            struct aspeed_sdhci *sdhci,
>                                            bool bus8)
> @@ -328,6 +364,7 @@ static inline int aspeed_sdhci_calculate_slot(struct aspeed_sdhci *dev,
>  static int aspeed_sdhci_probe(struct platform_device *pdev)
>  {
>         const struct aspeed_sdhci_pdata *aspeed_pdata;
> +       struct device_node *np = pdev->dev.of_node;
>         struct sdhci_pltfm_host *pltfm_host;
>         struct aspeed_sdhci *dev;
>         struct sdhci_host *host;
> @@ -372,6 +409,17 @@ 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,
> +                                              true, slot);
> +       }
> +
> +       if (of_property_read_bool(np, "sd-uhs-sdr104")) {
> +               aspeed_sdc_set_slot_capability(host, dev->parent, ASPEED_SDC_CAP2_SDR104,
> +                                              true, slot);
> +       }
> +
>         pltfm_host->clk = devm_clk_get(&pdev->dev, NULL);
>         if (IS_ERR(pltfm_host->clk))
>                 return PTR_ERR(pltfm_host->clk);
> --
> 2.17.1
>

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