linux-arm-kernel.lists.infradead.org archive mirror
 help / color / mirror / Atom feed
* [PATCH v3 0/5] mmc: sdhci-of-aspeed: Support toggling SD bus signal
@ 2021-05-06 10:03 Steven Lee
  2021-05-06 10:03 ` [PATCH v3 1/5] dt-bindings: mmc: sdhci-of-aspeed: Add an example for AST2600-A2 EVB Steven Lee
                   ` (4 more replies)
  0 siblings, 5 replies; 28+ messages in thread
From: Steven Lee @ 2021-05-06 10:03 UTC (permalink / raw)
  To: Andrew Jeffery, Ulf Hansson, Rob Herring, Joel Stanley,
	Adrian Hunter, Philipp Zabel, 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: steven_lee, Hongweiz, ryan_chen, chin-ting_kuo

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 provides the example for enabling regulators and
supporting SDR104 mode on AST2600-A2 EVB.
The description of the reference design of AST2600-A2 EVB is added
in the dts file.

This patch also include a helper for updating AST2600 sdhci capability
registers, and assert/deassert the reset signal for cleaning up AST2600
eMMC controller before eMMC is probed.

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 (5):
  dt-bindings: mmc: sdhci-of-aspeed: Add an example for AST2600-A2 EVB
  ARM: dts: aspeed: ast2600evb: Add comment for gpio regulator of sdhci
  ARM: dts: aspeed: ast2600evb: Add phase correction for emmc
    controller.
  mmc: sdhci-of-aspeed: Add a helper for updating capability register.
  mmc: sdhci-of-aspeed: Assert/Deassert reset signal before probing eMMC

 .../devicetree/bindings/mmc/aspeed,sdhci.yaml | 101 ++++++++++++++++-
 arch/arm/boot/dts/aspeed-ast2600-evb.dts      |  18 ++-
 drivers/mmc/host/sdhci-of-aspeed.c            | 106 ++++++++++++++++--
 3 files changed, 211 insertions(+), 14 deletions(-)

-- 
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	[flat|nested] 28+ messages in thread

* [PATCH v3 1/5] dt-bindings: mmc: sdhci-of-aspeed: Add an example for AST2600-A2 EVB
  2021-05-06 10:03 [PATCH v3 0/5] mmc: sdhci-of-aspeed: Support toggling SD bus signal Steven Lee
@ 2021-05-06 10:03 ` Steven Lee
  2021-05-07  1:13   ` Rob Herring
  2021-05-06 10:03 ` [PATCH v3 2/5] ARM: dts: aspeed: ast2600evb: Add comment for gpio regulator of sdhci Steven Lee
                   ` (3 subsequent siblings)
  4 siblings, 1 reply; 28+ messages in thread
From: Steven Lee @ 2021-05-06 10:03 UTC (permalink / raw)
  To: Andrew Jeffery, Ulf Hansson, Rob Herring, Joel Stanley,
	Adrian Hunter, Philipp Zabel, 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: steven_lee, Hongweiz, ryan_chen, chin-ting_kuo

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 | 101 +++++++++++++++++-
 1 file changed, 97 insertions(+), 4 deletions(-)

diff --git a/Documentation/devicetree/bindings/mmc/aspeed,sdhci.yaml b/Documentation/devicetree/bindings/mmc/aspeed,sdhci.yaml
index 987b287f3bff..de7e61b3d37a 100644
--- a/Documentation/devicetree/bindings/mmc/aspeed,sdhci.yaml
+++ b/Documentation/devicetree/bindings/mmc/aspeed,sdhci.yaml
@@ -45,10 +45,16 @@ patternProperties:
 
     properties:
       compatible:
-        enum:
-          - aspeed,ast2400-sdhci
-          - aspeed,ast2500-sdhci
-          - aspeed,ast2600-sdhci
+        oneOf:
+          - items:
+              - enum:
+                  - aspeed,ast2400-sdhci
+                  - aspeed,ast2500-sdhci
+                  - aspeed,ast2600-sdhci
+          - items:
+              - enum:
+                  - aspeed,ast2600-sdhci
+              - const: sdhci
       reg:
         maxItems: 1
         description: The SDHCI registers
@@ -104,3 +110,90 @@ examples:
                     clocks = <&syscon ASPEED_CLK_SDIO>;
             };
     };
+
+  - |
+    #include <dt-bindings/gpio/gpio.h>
+    #include <dt-bindings/clock/aspeed-clock.h>
+    #include <dt-bindings/interrupt-controller/irq.h>
+    #include <dt-bindings/interrupt-controller/arm-gic.h>
+    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@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>;
+
+            sdhci@1e740100 {
+                    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>;
+            };
+
+            sdhci@1e740200 {
+                    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	[flat|nested] 28+ messages in thread

* [PATCH v3 2/5] ARM: dts: aspeed: ast2600evb: Add comment for gpio regulator of sdhci
  2021-05-06 10:03 [PATCH v3 0/5] mmc: sdhci-of-aspeed: Support toggling SD bus signal Steven Lee
  2021-05-06 10:03 ` [PATCH v3 1/5] dt-bindings: mmc: sdhci-of-aspeed: Add an example for AST2600-A2 EVB Steven Lee
@ 2021-05-06 10:03 ` Steven Lee
  2021-05-07  1:40   ` Andrew Jeffery
  2021-05-06 10:03 ` [PATCH v3 3/5] ARM: dts: aspeed: ast2600evb: Add phase correction for emmc controller Steven Lee
                   ` (2 subsequent siblings)
  4 siblings, 1 reply; 28+ messages in thread
From: Steven Lee @ 2021-05-06 10:03 UTC (permalink / raw)
  To: Andrew Jeffery, Ulf Hansson, Rob Herring, Joel Stanley,
	Adrian Hunter, Philipp Zabel, 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: steven_lee, Hongweiz, ryan_chen, chin-ting_kuo

Add the description for describing the AST2600-A2 EVB reference design of
GPIO regulators.

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

diff --git a/arch/arm/boot/dts/aspeed-ast2600-evb.dts b/arch/arm/boot/dts/aspeed-ast2600-evb.dts
index 2772796e215e..1ae0facc3d5f 100644
--- a/arch/arm/boot/dts/aspeed-ast2600-evb.dts
+++ b/arch/arm/boot/dts/aspeed-ast2600-evb.dts
@@ -104,6 +104,21 @@
 	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.
+ */
 &emmc {
 	non-removable;
 	bus-width = <4>;
-- 
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	[flat|nested] 28+ messages in thread

* [PATCH v3 3/5] ARM: dts: aspeed: ast2600evb: Add phase correction for emmc controller.
  2021-05-06 10:03 [PATCH v3 0/5] mmc: sdhci-of-aspeed: Support toggling SD bus signal Steven Lee
  2021-05-06 10:03 ` [PATCH v3 1/5] dt-bindings: mmc: sdhci-of-aspeed: Add an example for AST2600-A2 EVB Steven Lee
  2021-05-06 10:03 ` [PATCH v3 2/5] ARM: dts: aspeed: ast2600evb: Add comment for gpio regulator of sdhci Steven Lee
@ 2021-05-06 10:03 ` Steven Lee
  2021-05-07  1:34   ` Andrew Jeffery
  2021-05-06 10:03 ` [PATCH v3 4/5] mmc: sdhci-of-aspeed: Add a helper for updating capability register Steven Lee
  2021-05-06 10:03 ` [PATCH v3 5/5] mmc: sdhci-of-aspeed: Assert/Deassert reset signal before probing eMMC Steven Lee
  4 siblings, 1 reply; 28+ messages in thread
From: Steven Lee @ 2021-05-06 10:03 UTC (permalink / raw)
  To: Andrew Jeffery, Ulf Hansson, Rob Herring, Joel Stanley,
	Adrian Hunter, Philipp Zabel, 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: steven_lee, Hongweiz, ryan_chen, chin-ting_kuo

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

Signed-off-by: Steven Lee <steven_lee@aspeedtech.com>
---
 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 1ae0facc3d5f..fa63cb033c57 100644
--- a/arch/arm/boot/dts/aspeed-ast2600-evb.dts
+++ b/arch/arm/boot/dts/aspeed-ast2600-evb.dts
@@ -122,7 +122,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

^ permalink raw reply	[flat|nested] 28+ messages in thread

* [PATCH v3 4/5] mmc: sdhci-of-aspeed: Add a helper for updating capability register.
  2021-05-06 10:03 [PATCH v3 0/5] mmc: sdhci-of-aspeed: Support toggling SD bus signal Steven Lee
                   ` (2 preceding siblings ...)
  2021-05-06 10:03 ` [PATCH v3 3/5] ARM: dts: aspeed: ast2600evb: Add phase correction for emmc controller Steven Lee
@ 2021-05-06 10:03 ` Steven Lee
  2021-05-07  2:13   ` Andrew Jeffery
  2021-05-06 10:03 ` [PATCH v3 5/5] mmc: sdhci-of-aspeed: Assert/Deassert reset signal before probing eMMC Steven Lee
  4 siblings, 1 reply; 28+ messages in thread
From: Steven Lee @ 2021-05-06 10:03 UTC (permalink / raw)
  To: Andrew Jeffery, Ulf Hansson, Rob Herring, Joel Stanley,
	Adrian Hunter, Philipp Zabel, 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: steven_lee, Hongweiz, ryan_chen, chin-ting_kuo

The patch add a new function aspeed_sdc_set_slot_capability() for
updating sdhci capability register.

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

diff --git a/drivers/mmc/host/sdhci-of-aspeed.c b/drivers/mmc/host/sdhci-of-aspeed.c
index d001c51074a0..4979f98ffb52 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;
@@ -70,8 +75,42 @@ struct aspeed_sdhci {
 	u32 width_mask;
 	struct mmc_clk_phase_map phase_map;
 	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)
+{
+	u8 cap_reg;
+	u32 mirror_reg_offset, cap_val;
+
+	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)
@@ -329,6 +368,7 @@ 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;
@@ -372,6 +412,23 @@ 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

^ permalink raw reply	[flat|nested] 28+ messages in thread

* [PATCH v3 5/5] mmc: sdhci-of-aspeed: Assert/Deassert reset signal before probing eMMC
  2021-05-06 10:03 [PATCH v3 0/5] mmc: sdhci-of-aspeed: Support toggling SD bus signal Steven Lee
                   ` (3 preceding siblings ...)
  2021-05-06 10:03 ` [PATCH v3 4/5] mmc: sdhci-of-aspeed: Add a helper for updating capability register Steven Lee
@ 2021-05-06 10:03 ` Steven Lee
  2021-05-06 10:24   ` Philipp Zabel
  4 siblings, 1 reply; 28+ messages in thread
From: Steven Lee @ 2021-05-06 10:03 UTC (permalink / raw)
  To: Andrew Jeffery, Ulf Hansson, Rob Herring, Joel Stanley,
	Adrian Hunter, Philipp Zabel, 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: steven_lee, Hongweiz, ryan_chen, chin-ting_kuo

For cleaning up the AST2600 eMMC controller, the reset signal should be
asserted and deasserted before it is probed.

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

diff --git a/drivers/mmc/host/sdhci-of-aspeed.c b/drivers/mmc/host/sdhci-of-aspeed.c
index 4979f98ffb52..8ef06f32abff 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"
@@ -36,9 +37,16 @@
 /* SDIO{14,24} */
 #define ASPEED_SDC_CAP2_SDR104         (1 * 32 + 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;
@@ -78,6 +86,10 @@ struct aspeed_sdhci {
 
 };
 
+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.
@@ -533,11 +545,22 @@ 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;
 
 	sdc = devm_kzalloc(&pdev->dev, sizeof(*sdc), GFP_KERNEL);
@@ -546,6 +569,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);
@@ -593,15 +633,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	[flat|nested] 28+ messages in thread

* Re: [PATCH v3 5/5] mmc: sdhci-of-aspeed: Assert/Deassert reset signal before probing eMMC
  2021-05-06 10:03 ` [PATCH v3 5/5] mmc: sdhci-of-aspeed: Assert/Deassert reset signal before probing eMMC Steven Lee
@ 2021-05-06 10:24   ` Philipp Zabel
  2021-05-07  1:32     ` Andrew Jeffery
  2021-05-07  6:02     ` Steven Lee
  0 siblings, 2 replies; 28+ messages in thread
From: Philipp Zabel @ 2021-05-06 10:24 UTC (permalink / raw)
  To: Steven Lee
  Cc: Andrew Jeffery, Ulf Hansson, Rob Herring, Joel Stanley,
	Adrian Hunter, 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, Hongweiz,
	ryan_chen, chin-ting_kuo

Hi Steven,

On Thu, May 06, 2021 at 06:03:12PM +0800, Steven Lee wrote:
> For cleaning up the AST2600 eMMC controller, the reset signal should be
> asserted and deasserted before it is probed.
> 
> Signed-off-by: Steven Lee <steven_lee@aspeedtech.com>
> ---
>  drivers/mmc/host/sdhci-of-aspeed.c | 49 ++++++++++++++++++++++++------
>  1 file changed, 40 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/mmc/host/sdhci-of-aspeed.c b/drivers/mmc/host/sdhci-of-aspeed.c
> index 4979f98ffb52..8ef06f32abff 100644
> --- a/drivers/mmc/host/sdhci-of-aspeed.c
> +++ b/drivers/mmc/host/sdhci-of-aspeed.c
[...]
> @@ -533,11 +545,22 @@ 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;

There is no need to initialize these variables to NULL, see below:

>  	int ret;
>  
>  	sdc = devm_kzalloc(&pdev->dev, sizeof(*sdc), GFP_KERNEL);
> @@ -546,6 +569,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);

match is set unconditionally before it is used,

> +	if (!match)
> +		return -ENODEV;
> +
> +	if (match->data)
> +		info = match->data;

and info could be set unconditionally as well:

	info = match->data;

> +	if (info) {
> +		if (info->flag & PROBE_AFTER_ASSET_DEASSERT) {
> +			sdc->rst = devm_reset_control_get(&pdev->dev, NULL);

Please use devm_reset_control_get_exclusive() or
devm_reset_control_get_optional_exclusive().

> +			if (!IS_ERR(sdc->rst)) {

Please just return errors here instead of ignoring them.
The reset_control_get_optional variants return NULL in case the
device node doesn't contain a resets phandle, in case you really
consider this reset to be optional even though the flag is set?

> +				reset_control_assert(sdc->rst);
> +				reset_control_deassert(sdc->rst);

Is there no need for delays between assertion and deassertion or after
the reset is deasserted?

> +			}
> +		}
> +	}
> +
>  	sdc->clk = devm_clk_get(&pdev->dev, NULL);
>  	if (IS_ERR(sdc->clk))
>  		return PTR_ERR(sdc->clk);

In general, I would assert/deassert the reset only after all resources
are successfully acquired. This might avoid unnecessary resets in case
of probe deferrals.

regards
Philipp

_______________________________________________
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] 28+ messages in thread

* Re: [PATCH v3 1/5] dt-bindings: mmc: sdhci-of-aspeed: Add an example for AST2600-A2 EVB
  2021-05-06 10:03 ` [PATCH v3 1/5] dt-bindings: mmc: sdhci-of-aspeed: Add an example for AST2600-A2 EVB Steven Lee
@ 2021-05-07  1:13   ` Rob Herring
  2021-05-07  3:13     ` Steven Lee
  0 siblings, 1 reply; 28+ messages in thread
From: Rob Herring @ 2021-05-07  1:13 UTC (permalink / raw)
  To: Steven Lee
  Cc: Andrew Jeffery, Ulf Hansson, Joel Stanley, Adrian Hunter,
	Philipp Zabel, 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, Hongweiz,
	ryan_chen, chin-ting_kuo

On Thu, May 06, 2021 at 06:03:08PM +0800, Steven Lee wrote:
> 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 | 101 +++++++++++++++++-
>  1 file changed, 97 insertions(+), 4 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/mmc/aspeed,sdhci.yaml b/Documentation/devicetree/bindings/mmc/aspeed,sdhci.yaml
> index 987b287f3bff..de7e61b3d37a 100644
> --- a/Documentation/devicetree/bindings/mmc/aspeed,sdhci.yaml
> +++ b/Documentation/devicetree/bindings/mmc/aspeed,sdhci.yaml
> @@ -45,10 +45,16 @@ patternProperties:
>  
>      properties:
>        compatible:
> -        enum:
> -          - aspeed,ast2400-sdhci
> -          - aspeed,ast2500-sdhci
> -          - aspeed,ast2600-sdhci
> +        oneOf:
> +          - items:
> +              - enum:
> +                  - aspeed,ast2400-sdhci
> +                  - aspeed,ast2500-sdhci
> +                  - aspeed,ast2600-sdhci
> +          - items:
> +              - enum:
> +                  - aspeed,ast2600-sdhci
> +              - const: sdhci

Why are you adding 'sdhci'. That's not useful as a compatible given how 
many quirks different implementations have.


>        reg:
>          maxItems: 1
>          description: The SDHCI registers
> @@ -104,3 +110,90 @@ examples:
>                      clocks = <&syscon ASPEED_CLK_SDIO>;
>              };
>      };
> +
> +  - |

Why do we need another example?

> +    #include <dt-bindings/gpio/gpio.h>
> +    #include <dt-bindings/clock/aspeed-clock.h>
> +    #include <dt-bindings/interrupt-controller/irq.h>
> +    #include <dt-bindings/interrupt-controller/arm-gic.h>
> +    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@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>;
> +
> +            sdhci@1e740100 {
> +                    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>;
> +            };
> +
> +            sdhci@1e740200 {
> +                    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	[flat|nested] 28+ messages in thread

* Re: [PATCH v3 5/5] mmc: sdhci-of-aspeed: Assert/Deassert reset signal before probing eMMC
  2021-05-06 10:24   ` Philipp Zabel
@ 2021-05-07  1:32     ` Andrew Jeffery
  2021-05-07  6:24       ` Steven Lee
  2021-05-07  6:02     ` Steven Lee
  1 sibling, 1 reply; 28+ messages in thread
From: Andrew Jeffery @ 2021-05-07  1:32 UTC (permalink / raw)
  To: Philipp Zabel, Steven Lee
  Cc: Ulf Hansson, Rob Herring, Joel Stanley, Adrian Hunter, 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 Thu, 6 May 2021, at 19:54, Philipp Zabel wrote:
> Hi Steven,
> 
> On Thu, May 06, 2021 at 06:03:12PM +0800, Steven Lee wrote:
> > +	if (info) {
> > +		if (info->flag & PROBE_AFTER_ASSET_DEASSERT) {
> > +			sdc->rst = devm_reset_control_get(&pdev->dev, NULL);
> 
> Please use devm_reset_control_get_exclusive() or
> devm_reset_control_get_optional_exclusive().
> 
> > +			if (!IS_ERR(sdc->rst)) {
> 
> Please just return errors here instead of ignoring them.
> The reset_control_get_optional variants return NULL in case the
> device node doesn't contain a resets phandle, in case you really
> consider this reset to be optional even though the flag is set?

It feels like we should get rid of the flag and leave it to the 
devicetree.

I'm still kind of surprised it's not something we want to do for the 
2400 and 2500 as well.

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] 28+ messages in thread

* Re: [PATCH v3 3/5] ARM: dts: aspeed: ast2600evb: Add phase correction for emmc controller.
  2021-05-06 10:03 ` [PATCH v3 3/5] ARM: dts: aspeed: ast2600evb: Add phase correction for emmc controller Steven Lee
@ 2021-05-07  1:34   ` Andrew Jeffery
  0 siblings, 0 replies; 28+ messages in thread
From: Andrew Jeffery @ 2021-05-07  1:34 UTC (permalink / raw)
  To: Steven Lee, Ulf Hansson, Rob Herring, Joel Stanley,
	Adrian Hunter, Philipp Zabel, 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: Ryan Chen, Chin-Ting Kuo, Hongwei Zhang



On Thu, 6 May 2021, at 19:33, Steven Lee 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>

_______________________________________________
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] 28+ messages in thread

* Re: [PATCH v3 2/5] ARM: dts: aspeed: ast2600evb: Add comment for gpio regulator of sdhci
  2021-05-06 10:03 ` [PATCH v3 2/5] ARM: dts: aspeed: ast2600evb: Add comment for gpio regulator of sdhci Steven Lee
@ 2021-05-07  1:40   ` Andrew Jeffery
  2021-05-07  3:30     ` Steven Lee
  0 siblings, 1 reply; 28+ messages in thread
From: Andrew Jeffery @ 2021-05-07  1:40 UTC (permalink / raw)
  To: Steven Lee, Ulf Hansson, Rob Herring, Joel Stanley,
	Adrian Hunter, Philipp Zabel, 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: Ryan Chen, Chin-Ting Kuo, Hongwei Zhang



On Thu, 6 May 2021, at 19:33, Steven Lee wrote:
> Add the description for describing the AST2600-A2 EVB reference design of
> GPIO regulators.
> 
> Signed-off-by: Steven Lee <steven_lee@aspeedtech.com>
> ---
>  arch/arm/boot/dts/aspeed-ast2600-evb.dts | 15 +++++++++++++++
>  1 file changed, 15 insertions(+)
> 
> diff --git a/arch/arm/boot/dts/aspeed-ast2600-evb.dts 
> b/arch/arm/boot/dts/aspeed-ast2600-evb.dts
> index 2772796e215e..1ae0facc3d5f 100644
> --- a/arch/arm/boot/dts/aspeed-ast2600-evb.dts
> +++ b/arch/arm/boot/dts/aspeed-ast2600-evb.dts
> @@ -104,6 +104,21 @@
>  	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.
> + */

Okay, I think the comment is in the right place, but I feel this patch 
should also add the regulator nodes and hook them up to the SDHCIs.

Given Rob isn't super keen on a second example in the binding document 
I think you can just cut the example out and paste it in here.

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] 28+ messages in thread

* Re: [PATCH v3 4/5] mmc: sdhci-of-aspeed: Add a helper for updating capability register.
  2021-05-06 10:03 ` [PATCH v3 4/5] mmc: sdhci-of-aspeed: Add a helper for updating capability register Steven Lee
@ 2021-05-07  2:13   ` Andrew Jeffery
  2021-05-07  6:59     ` Steven Lee
  0 siblings, 1 reply; 28+ messages in thread
From: Andrew Jeffery @ 2021-05-07  2:13 UTC (permalink / raw)
  To: Steven Lee, Ulf Hansson, Rob Herring, Joel Stanley,
	Adrian Hunter, Philipp Zabel, 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,

I have some minor comments. I expect you're going to do a v4 of the 
series, so if you'd like to clean them up in the process I'd appreciate 
it.

However, from a pragmatic standpoint I think the patch is in good shape.

On Thu, 6 May 2021, at 19:33, Steven Lee wrote:
> The patch add a new function aspeed_sdc_set_slot_capability() for
> updating sdhci capability register.

The commit message should explain why the patch is necessary and not 
what it does, as what it does is contained in the diff.

It's okay to explain *how* the patch acheives its goals if the 
implementation is subtle or complex.

Maybe the commit message could be something like:


```
Configure the SDHCIs as specified by the devicetree.

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 | 57 ++++++++++++++++++++++++++++++
>  1 file changed, 57 insertions(+)
> 
> diff --git a/drivers/mmc/host/sdhci-of-aspeed.c 
> b/drivers/mmc/host/sdhci-of-aspeed.c
> index d001c51074a0..4979f98ffb52 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;
> @@ -70,8 +75,42 @@ struct aspeed_sdhci {
>  	u32 width_mask;
>  	struct mmc_clk_phase_map phase_map;
>  	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

It would be nice to align the columns to improve readability.

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

I prefer we don't take up so much vertical space here. I think this 
could be just a couple of lines with multiple variables per line. We 
can go to 100 chars per line.

> +{
> +	u8 cap_reg;
> +	u32 mirror_reg_offset, cap_val;

The rest of the driver follows "reverse christmas tree" (longest to 
shortest declaration) style, so I prefer we try to maintain consistency 
where we can. Essentially, declare them in this order:

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)
> @@ -329,6 +368,7 @@ 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;

Again here with the reverse-christmas-tree style, so:

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;
>  	struct resource *res;
> @@ -372,6 +412,23 @@ 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);

Again, this would be nicer if we compress it to as few lines as possible.

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

As above.

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] 28+ messages in thread

* Re: [PATCH v3 1/5] dt-bindings: mmc: sdhci-of-aspeed: Add an example for AST2600-A2 EVB
  2021-05-07  1:13   ` Rob Herring
@ 2021-05-07  3:13     ` Steven Lee
  2021-05-07 17:21       ` Rob Herring
  0 siblings, 1 reply; 28+ messages in thread
From: Steven Lee @ 2021-05-07  3:13 UTC (permalink / raw)
  To: Rob Herring
  Cc: Andrew Jeffery, Ulf Hansson, Joel Stanley, Adrian Hunter,
	Philipp Zabel, 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, Hongweiz,
	Ryan Chen, Chin-Ting Kuo

The 05/07/2021 09:13, Rob Herring wrote:
> On Thu, May 06, 2021 at 06:03:08PM +0800, Steven Lee wrote:
> > 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 | 101 +++++++++++++++++-
> >  1 file changed, 97 insertions(+), 4 deletions(-)
> > 
> > diff --git a/Documentation/devicetree/bindings/mmc/aspeed,sdhci.yaml b/Documentation/devicetree/bindings/mmc/aspeed,sdhci.yaml
> > index 987b287f3bff..de7e61b3d37a 100644
> > --- a/Documentation/devicetree/bindings/mmc/aspeed,sdhci.yaml
> > +++ b/Documentation/devicetree/bindings/mmc/aspeed,sdhci.yaml
> > @@ -45,10 +45,16 @@ patternProperties:
> >  
> >      properties:
> >        compatible:
> > -        enum:
> > -          - aspeed,ast2400-sdhci
> > -          - aspeed,ast2500-sdhci
> > -          - aspeed,ast2600-sdhci
> > +        oneOf:
> > +          - items:
> > +              - enum:
> > +                  - aspeed,ast2400-sdhci
> > +                  - aspeed,ast2500-sdhci
> > +                  - aspeed,ast2600-sdhci
> > +          - items:
> > +              - enum:
> > +                  - aspeed,ast2600-sdhci
> > +              - const: sdhci
> 
> Why are you adding 'sdhci'. That's not useful as a compatible given how 
> many quirks different implementations have.
> 
> 

It is for passing the dtbs_check of the second example.
Without this definition, many device trees have the following
error:

['aspeed,ast2600-sdhci', 'sdhci'] is too long
Additional items are not allowed ('sdhci' was unexpected)

Regardless, I will remove it, and move the new example to device
tree.

Thanks.

> >        reg:
> >          maxItems: 1
> >          description: The SDHCI registers
> > @@ -104,3 +110,90 @@ examples:
> >                      clocks = <&syscon ASPEED_CLK_SDIO>;
> >              };
> >      };
> > +
> > +  - |
> 
> Why do we need another example?
> 

The original example is for AST2500 which doesn't support UHS mode.
The new example teaches users how to enable sdhci with UHS mode, add
gpio regulators, and adjust clock phase for AST2600-A2.

I will move the new example to ast2600 device tree per the
review comment.
https://lkml.org/lkml/2021/5/6/968

> > +    #include <dt-bindings/gpio/gpio.h>
> > +    #include <dt-bindings/clock/aspeed-clock.h>
> > +    #include <dt-bindings/interrupt-controller/irq.h>
> > +    #include <dt-bindings/interrupt-controller/arm-gic.h>
> > +    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@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>;
> > +
> > +            sdhci@1e740100 {
> > +                    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>;
> > +            };
> > +
> > +            sdhci@1e740200 {
> > +                    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	[flat|nested] 28+ messages in thread

* Re: [PATCH v3 2/5] ARM: dts: aspeed: ast2600evb: Add comment for gpio regulator of sdhci
  2021-05-07  1:40   ` Andrew Jeffery
@ 2021-05-07  3:30     ` Steven Lee
  2021-05-07  3:42       ` Andrew Jeffery
  0 siblings, 1 reply; 28+ messages in thread
From: Steven Lee @ 2021-05-07  3:30 UTC (permalink / raw)
  To: Andrew Jeffery
  Cc: Ulf Hansson, Rob Herring, Joel Stanley, Adrian Hunter,
	Philipp Zabel, 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, Ryan Chen,
	Chin-Ting Kuo, Hongwei Zhang

The 05/07/2021 09:40, Andrew Jeffery wrote:
> 
> 
> On Thu, 6 May 2021, at 19:33, Steven Lee wrote:
> > Add the description for describing the AST2600-A2 EVB reference design of
> > GPIO regulators.
> > 
> > Signed-off-by: Steven Lee <steven_lee@aspeedtech.com>
> > ---
> >  arch/arm/boot/dts/aspeed-ast2600-evb.dts | 15 +++++++++++++++
> >  1 file changed, 15 insertions(+)
> > 
> > diff --git a/arch/arm/boot/dts/aspeed-ast2600-evb.dts 
> > b/arch/arm/boot/dts/aspeed-ast2600-evb.dts
> > index 2772796e215e..1ae0facc3d5f 100644
> > --- a/arch/arm/boot/dts/aspeed-ast2600-evb.dts
> > +++ b/arch/arm/boot/dts/aspeed-ast2600-evb.dts
> > @@ -104,6 +104,21 @@
> >  	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.
> > + */
> 
> Okay, I think the comment is in the right place, but I feel this patch 
> should also add the regulator nodes and hook them up to the SDHCIs.
> 
> Given Rob isn't super keen on a second example in the binding document 
> I think you can just cut the example out and paste it in here.
> 

Hi Andrew,

Since AST2600-A0 and AST2600-A1 don't have regulators, do you mean cut
the example from dt-binding and paste it to aspeed-ast2600-evb.dts but
comment out the example?

> 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] 28+ messages in thread

* Re: [PATCH v3 2/5] ARM: dts: aspeed: ast2600evb: Add comment for gpio regulator of sdhci
  2021-05-07  3:30     ` Steven Lee
@ 2021-05-07  3:42       ` Andrew Jeffery
  0 siblings, 0 replies; 28+ messages in thread
From: Andrew Jeffery @ 2021-05-07  3:42 UTC (permalink / raw)
  To: Steven Lee
  Cc: Ulf Hansson, Rob Herring, Joel Stanley, Adrian Hunter,
	Philipp Zabel, 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, Ryan Chen,
	Chin-Ting Kuo, Hongwei Zhang



On Fri, 7 May 2021, at 13:00, Steven Lee wrote:
> The 05/07/2021 09:40, Andrew Jeffery wrote:
> > 
> > 
> > On Thu, 6 May 2021, at 19:33, Steven Lee wrote:
> > > Add the description for describing the AST2600-A2 EVB reference design of
> > > GPIO regulators.
> > > 
> > > Signed-off-by: Steven Lee <steven_lee@aspeedtech.com>
> > > ---
> > >  arch/arm/boot/dts/aspeed-ast2600-evb.dts | 15 +++++++++++++++
> > >  1 file changed, 15 insertions(+)
> > > 
> > > diff --git a/arch/arm/boot/dts/aspeed-ast2600-evb.dts 
> > > b/arch/arm/boot/dts/aspeed-ast2600-evb.dts
> > > index 2772796e215e..1ae0facc3d5f 100644
> > > --- a/arch/arm/boot/dts/aspeed-ast2600-evb.dts
> > > +++ b/arch/arm/boot/dts/aspeed-ast2600-evb.dts
> > > @@ -104,6 +104,21 @@
> > >  	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.
> > > + */
> > 
> > Okay, I think the comment is in the right place, but I feel this patch 
> > should also add the regulator nodes and hook them up to the SDHCIs.
> > 
> > Given Rob isn't super keen on a second example in the binding document 
> > I think you can just cut the example out and paste it in here.
> > 
> 
> Hi Andrew,
> 
> Since AST2600-A0 and AST2600-A1 don't have regulators, do you mean cut
> the example from dt-binding and paste it to aspeed-ast2600-evb.dts but
> comment out the example?

If the board design varies with the silicon revision we should probably 
have devicetrees that are appropriate for each, so an 
aspeed-ast2600-evb-a2.dts

#include "aspeed-ast2600-evb.dts" at the top and go from there.

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] 28+ messages in thread

* Re: [PATCH v3 5/5] mmc: sdhci-of-aspeed: Assert/Deassert reset signal before probing eMMC
  2021-05-06 10:24   ` Philipp Zabel
  2021-05-07  1:32     ` Andrew Jeffery
@ 2021-05-07  6:02     ` Steven Lee
  1 sibling, 0 replies; 28+ messages in thread
From: Steven Lee @ 2021-05-07  6:02 UTC (permalink / raw)
  To: Philipp Zabel
  Cc: Andrew Jeffery, Ulf Hansson, Rob Herring, Joel Stanley,
	Adrian Hunter, 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, Hongweiz,
	Ryan Chen, Chin-Ting Kuo

The 05/06/2021 18:24, Philipp Zabel wrote:
> Hi Steven,
> 
> On Thu, May 06, 2021 at 06:03:12PM +0800, Steven Lee wrote:
> > For cleaning up the AST2600 eMMC controller, the reset signal should be
> > asserted and deasserted before it is probed.
> > 
> > Signed-off-by: Steven Lee <steven_lee@aspeedtech.com>
> > ---
> >  drivers/mmc/host/sdhci-of-aspeed.c | 49 ++++++++++++++++++++++++------
> >  1 file changed, 40 insertions(+), 9 deletions(-)
> > 
> > diff --git a/drivers/mmc/host/sdhci-of-aspeed.c b/drivers/mmc/host/sdhci-of-aspeed.c
> > index 4979f98ffb52..8ef06f32abff 100644
> > --- a/drivers/mmc/host/sdhci-of-aspeed.c
> > +++ b/drivers/mmc/host/sdhci-of-aspeed.c
> [...]
> > @@ -533,11 +545,22 @@ 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;
> 
> There is no need to initialize these variables to NULL, see below:
> 

Will modify it.

> >  	int ret;
> >  
> >  	sdc = devm_kzalloc(&pdev->dev, sizeof(*sdc), GFP_KERNEL);
> > @@ -546,6 +569,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);
> 
> match is set unconditionally before it is used,
> 
> > +	if (!match)
> > +		return -ENODEV;
> > +
> > +	if (match->data)
> > +		info = match->data;
> 
> and info could be set unconditionally as well:
> 
> 	info = match->data;
> 
> > +	if (info) {
> > +		if (info->flag & PROBE_AFTER_ASSET_DEASSERT) {
> > +			sdc->rst = devm_reset_control_get(&pdev->dev, NULL);
> 
> Please use devm_reset_control_get_exclusive() or
> devm_reset_control_get_optional_exclusive().
> 

Will modify as you suggest.

> > +			if (!IS_ERR(sdc->rst)) {
> 
> Please just return errors here instead of ignoring them.
> The reset_control_get_optional variants return NULL in case the
> device node doesn't contain a resets phandle, in case you really
> consider this reset to be optional even though the flag is set?
> 

Will return error here.

> > +				reset_control_assert(sdc->rst);
> > +				reset_control_deassert(sdc->rst);
> 
> Is there no need for delays between assertion and deassertion or after
> the reset is deasserted?
> 

Per the internal discussion, I Will add udelay(1).

> > +			}
> > +		}
> > +	}
> > +
> >  	sdc->clk = devm_clk_get(&pdev->dev, NULL);
> >  	if (IS_ERR(sdc->clk))
> >  		return PTR_ERR(sdc->clk);
> 
> In general, I would assert/deassert the reset only after all resources
> are successfully acquired. This might avoid unnecessary resets in case
> of probe deferrals.
> 

Thanks for the suggestion. I will try to move the implementation of
reset after devm_ioremap_resource().

> regards
> Philipp

_______________________________________________
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] 28+ messages in thread

* Re: [PATCH v3 5/5] mmc: sdhci-of-aspeed: Assert/Deassert reset signal before probing eMMC
  2021-05-07  1:32     ` Andrew Jeffery
@ 2021-05-07  6:24       ` Steven Lee
  2021-05-07  7:36         ` Andrew Jeffery
  0 siblings, 1 reply; 28+ messages in thread
From: Steven Lee @ 2021-05-07  6:24 UTC (permalink / raw)
  To: Andrew Jeffery
  Cc: Philipp Zabel, Ulf Hansson, Rob Herring, Joel Stanley,
	Adrian Hunter, 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/07/2021 09:32, Andrew Jeffery wrote:
> 
> 
> On Thu, 6 May 2021, at 19:54, Philipp Zabel wrote:
> > Hi Steven,
> > 
> > On Thu, May 06, 2021 at 06:03:12PM +0800, Steven Lee wrote:
> > > +	if (info) {
> > > +		if (info->flag & PROBE_AFTER_ASSET_DEASSERT) {
> > > +			sdc->rst = devm_reset_control_get(&pdev->dev, NULL);
> > 
> > Please use devm_reset_control_get_exclusive() or
> > devm_reset_control_get_optional_exclusive().
> > 
> > > +			if (!IS_ERR(sdc->rst)) {
> > 
> > Please just return errors here instead of ignoring them.
> > The reset_control_get_optional variants return NULL in case the
> > device node doesn't contain a resets phandle, in case you really
> > consider this reset to be optional even though the flag is set?
> 
> It feels like we should get rid of the flag and leave it to the 
> devicetree.
> 

Do you mean adding a flag, for instance, "mmc-reset" in the
device tree and call of_property_read_bool() in aspeed_sdc_probe()?

> I'm still kind of surprised it's not something we want to do for the 
> 2400 and 2500 as well.
> 

Per discussion with the chip designer, AST2400 and AST2500 doesn't need
this implementation since the chip design is different to AST2600.

> 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] 28+ messages in thread

* Re: [PATCH v3 4/5] mmc: sdhci-of-aspeed: Add a helper for updating capability register.
  2021-05-07  2:13   ` Andrew Jeffery
@ 2021-05-07  6:59     ` Steven Lee
  2021-05-07  7:07       ` Andrew Jeffery
  0 siblings, 1 reply; 28+ messages in thread
From: Steven Lee @ 2021-05-07  6:59 UTC (permalink / raw)
  To: Andrew Jeffery
  Cc: Ulf Hansson, Rob Herring, Joel Stanley, Adrian Hunter,
	Philipp Zabel, 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/07/2021 10:13, Andrew Jeffery wrote:
> Hi Steven,
> 
> I have some minor comments. I expect you're going to do a v4 of the 
> series, so if you'd like to clean them up in the process I'd appreciate 
> it.
> 

Yes, I am going to prepare v4 patch for meeting reviewer's expectation
including your comment in this patch.

I've learned a lot from your suggestion for driver upstream.
Many thanks!

> However, from a pragmatic standpoint I think the patch is in good shape.
> 
> On Thu, 6 May 2021, at 19:33, Steven Lee wrote:
> > The patch add a new function aspeed_sdc_set_slot_capability() for
> > updating sdhci capability register.
> 
> The commit message should explain why the patch is necessary and not 
> what it does, as what it does is contained in the diff.
> 
> It's okay to explain *how* the patch acheives its goals if the 
> implementation is subtle or complex.
> 
> Maybe the commit message could be something like:
> 
> 
> ```
> Configure the SDHCIs as specified by the devicetree.
> 
> 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.
> ```
> 

Thanks for the exmaple, I will modify my commit message.

> > 
> > Signed-off-by: Steven Lee <steven_lee@aspeedtech.com>
> > ---
> >  drivers/mmc/host/sdhci-of-aspeed.c | 57 ++++++++++++++++++++++++++++++
> >  1 file changed, 57 insertions(+)
> > 
> > diff --git a/drivers/mmc/host/sdhci-of-aspeed.c 
> > b/drivers/mmc/host/sdhci-of-aspeed.c
> > index d001c51074a0..4979f98ffb52 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;
> > @@ -70,8 +75,42 @@ struct aspeed_sdhci {
> >  	u32 width_mask;
> >  	struct mmc_clk_phase_map phase_map;
> >  	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
> 
> It would be nice to align the columns to improve readability.
> 

Columns seems are aligned in my mail client(mutt) and my editor(vim).
I paste the above comment in Notepad++, columns are aligned as well.

> > +static void aspeed_sdc_set_slot_capability(struct sdhci_host *host,
> > +					   struct aspeed_sdc *sdc,
> > +					   int capability,
> > +					   bool enable,
> > +					   u8 slot)
> 
> I prefer we don't take up so much vertical space here. I think this 
> could be just a couple of lines with multiple variables per line. We 
> can go to 100 chars per line.
> 

I will change the function as the follows:

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

> > +{
> > +	u8 cap_reg;
> > +	u32 mirror_reg_offset, cap_val;
> 
> The rest of the driver follows "reverse christmas tree" (longest to 
> shortest declaration) style, so I prefer we try to maintain consistency 
> where we can. Essentially, declare them in this order:
> 
> u32 mirror_reg_offset;
> u32 cap_val;
> u8 cap_reg;
> 

Will modify it.

> > +
> > +	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)
> > @@ -329,6 +368,7 @@ 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;
> 
> Again here with the reverse-christmas-tree style, so:
> 
> const struct aspeed_sdhci_pdata *aspeed_pdata;
> struct device_node *np = pdev->dev.of_node;
> struct sdhci_pltfm_host *pltfm_host;
> ...
> 

Will modify it.

> >  	struct aspeed_sdhci *dev;
> >  	struct sdhci_host *host;
> >  	struct resource *res;
> > @@ -372,6 +412,23 @@ 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);
> 
> Again, this would be nicer if we compress it to as few lines as possible.
> 

Will modify the function as follows:

		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);
> 
> As above.
> 

Will modify the function as follows:

		aspeed_sdc_set_slot_capability(host, dev->parent, ASPEED_SDC_CAP2_SDR104,
					       true, slot);

> 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] 28+ messages in thread

* Re: [PATCH v3 4/5] mmc: sdhci-of-aspeed: Add a helper for updating capability register.
  2021-05-07  6:59     ` Steven Lee
@ 2021-05-07  7:07       ` Andrew Jeffery
  0 siblings, 0 replies; 28+ messages in thread
From: Andrew Jeffery @ 2021-05-07  7:07 UTC (permalink / raw)
  To: Steven Lee
  Cc: Ulf Hansson, Rob Herring, Joel Stanley, Adrian Hunter,
	Philipp Zabel, 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 Fri, 7 May 2021, at 16:29, Steven Lee wrote:
> The 05/07/2021 10:13, Andrew Jeffery wrote:
> > > +/*
> > > + * 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
> > 
> > It would be nice to align the columns to improve readability.
> > 
> 
> Columns seems are aligned in my mail client(mutt) and my editor(vim).
> I paste the above comment in Notepad++, columns are aligned as well.
> 

Ah, it's probably my mail client then. Sorry for the noise!

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] 28+ messages in thread

* Re: [PATCH v3 5/5] mmc: sdhci-of-aspeed: Assert/Deassert reset signal before probing eMMC
  2021-05-07  6:24       ` Steven Lee
@ 2021-05-07  7:36         ` Andrew Jeffery
  2021-05-10  6:03           ` Steven Lee
  0 siblings, 1 reply; 28+ messages in thread
From: Andrew Jeffery @ 2021-05-07  7:36 UTC (permalink / raw)
  To: Steven Lee
  Cc: Philipp Zabel, Ulf Hansson, Rob Herring, Joel Stanley,
	Adrian Hunter, 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 Fri, 7 May 2021, at 15:54, Steven Lee wrote:
> The 05/07/2021 09:32, Andrew Jeffery wrote:
> > 
> > 
> > On Thu, 6 May 2021, at 19:54, Philipp Zabel wrote:
> > > Hi Steven,
> > > 
> > > On Thu, May 06, 2021 at 06:03:12PM +0800, Steven Lee wrote:
> > > > +	if (info) {
> > > > +		if (info->flag & PROBE_AFTER_ASSET_DEASSERT) {
> > > > +			sdc->rst = devm_reset_control_get(&pdev->dev, NULL);
> > > 
> > > Please use devm_reset_control_get_exclusive() or
> > > devm_reset_control_get_optional_exclusive().
> > > 
> > > > +			if (!IS_ERR(sdc->rst)) {
> > > 
> > > Please just return errors here instead of ignoring them.
> > > The reset_control_get_optional variants return NULL in case the
> > > device node doesn't contain a resets phandle, in case you really
> > > consider this reset to be optional even though the flag is set?
> > 
> > It feels like we should get rid of the flag and leave it to the 
> > devicetree.
> > 
> 
> Do you mean adding a flag, for instance, "mmc-reset" in the
> device tree and call of_property_read_bool() in aspeed_sdc_probe()?
> 
> > I'm still kind of surprised it's not something we want to do for the 
> > 2400 and 2500 as well.
> > 
> 
> Per discussion with the chip designer, AST2400 and AST2500 doesn't need
> this implementation since the chip design is different to AST2600.

So digging a bit more deeply on this, it looks like the reset is 
already taken care of by drivers/clk/clk-ast2600.c in the 
clk_prepare_enable() path.

clk-ast2600 handles resets when enabling the clock for most peripherals:

https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/clk/clk-ast2600.c?h=v5.12#n276

and this is true for both the SD controller and the eMMC controller:

https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/clk/clk-ast2600.c?h=v5.12#n94
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/clk/clk-ast2600.c?h=v5.12#n88

If this weren't the case you'd specify a reset property in the SD/eMMC 
devicetree nodes for the 2600 and then use 
devm_reset_control_get_optional_exclusive() as Philipp suggested. See 
the reset binding here:

https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/devicetree/bindings/reset/reset.txt?h=v5.12

So on the surface it seems the reset handling in this patch is 
unnecessary. Have you observed an issue with the SoC that means it's 
required?

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] 28+ messages in thread

* Re: [PATCH v3 1/5] dt-bindings: mmc: sdhci-of-aspeed: Add an example for AST2600-A2 EVB
  2021-05-07  3:13     ` Steven Lee
@ 2021-05-07 17:21       ` Rob Herring
  2021-05-10  2:32         ` Steven Lee
  0 siblings, 1 reply; 28+ messages in thread
From: Rob Herring @ 2021-05-07 17:21 UTC (permalink / raw)
  To: Steven Lee
  Cc: Andrew Jeffery, Ulf Hansson, Joel Stanley, Adrian Hunter,
	Philipp Zabel, 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, Hongweiz,
	Ryan Chen, Chin-Ting Kuo

On Thu, May 6, 2021 at 10:14 PM Steven Lee <steven_lee@aspeedtech.com> wrote:
>
> The 05/07/2021 09:13, Rob Herring wrote:
> > On Thu, May 06, 2021 at 06:03:08PM +0800, Steven Lee wrote:
> > > 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 | 101 +++++++++++++++++-
> > >  1 file changed, 97 insertions(+), 4 deletions(-)
> > >
> > > diff --git a/Documentation/devicetree/bindings/mmc/aspeed,sdhci.yaml b/Documentation/devicetree/bindings/mmc/aspeed,sdhci.yaml
> > > index 987b287f3bff..de7e61b3d37a 100644
> > > --- a/Documentation/devicetree/bindings/mmc/aspeed,sdhci.yaml
> > > +++ b/Documentation/devicetree/bindings/mmc/aspeed,sdhci.yaml
> > > @@ -45,10 +45,16 @@ patternProperties:
> > >
> > >      properties:
> > >        compatible:
> > > -        enum:
> > > -          - aspeed,ast2400-sdhci
> > > -          - aspeed,ast2500-sdhci
> > > -          - aspeed,ast2600-sdhci
> > > +        oneOf:
> > > +          - items:
> > > +              - enum:
> > > +                  - aspeed,ast2400-sdhci
> > > +                  - aspeed,ast2500-sdhci
> > > +                  - aspeed,ast2600-sdhci
> > > +          - items:
> > > +              - enum:
> > > +                  - aspeed,ast2600-sdhci
> > > +              - const: sdhci
> >
> > Why are you adding 'sdhci'. That's not useful as a compatible given how
> > many quirks different implementations have.
> >
> >
>
> It is for passing the dtbs_check of the second example.
> Without this definition, many device trees have the following
> error:
>
> ['aspeed,ast2600-sdhci', 'sdhci'] is too long
> Additional items are not allowed ('sdhci' was unexpected)

I would probably fix the dts files then. Does anything depend on 'sdhci'?

Rob

_______________________________________________
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] 28+ messages in thread

* Re: [PATCH v3 1/5] dt-bindings: mmc: sdhci-of-aspeed: Add an example for AST2600-A2 EVB
  2021-05-07 17:21       ` Rob Herring
@ 2021-05-10  2:32         ` Steven Lee
  0 siblings, 0 replies; 28+ messages in thread
From: Steven Lee @ 2021-05-10  2:32 UTC (permalink / raw)
  To: Rob Herring
  Cc: Andrew Jeffery, Ulf Hansson, Joel Stanley, Adrian Hunter,
	Philipp Zabel, 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, Hongweiz,
	Ryan Chen, Chin-Ting Kuo

The 05/08/2021 01:21, Rob Herring wrote:
> On Thu, May 6, 2021 at 10:14 PM Steven Lee <steven_lee@aspeedtech.com> wrote:
> >
> > The 05/07/2021 09:13, Rob Herring wrote:
> > > On Thu, May 06, 2021 at 06:03:08PM +0800, Steven Lee wrote:
> > > > 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 | 101 +++++++++++++++++-
> > > >  1 file changed, 97 insertions(+), 4 deletions(-)
> > > >
> > > > diff --git a/Documentation/devicetree/bindings/mmc/aspeed,sdhci.yaml b/Documentation/devicetree/bindings/mmc/aspeed,sdhci.yaml
> > > > index 987b287f3bff..de7e61b3d37a 100644
> > > > --- a/Documentation/devicetree/bindings/mmc/aspeed,sdhci.yaml
> > > > +++ b/Documentation/devicetree/bindings/mmc/aspeed,sdhci.yaml
> > > > @@ -45,10 +45,16 @@ patternProperties:
> > > >
> > > >      properties:
> > > >        compatible:
> > > > -        enum:
> > > > -          - aspeed,ast2400-sdhci
> > > > -          - aspeed,ast2500-sdhci
> > > > -          - aspeed,ast2600-sdhci
> > > > +        oneOf:
> > > > +          - items:
> > > > +              - enum:
> > > > +                  - aspeed,ast2400-sdhci
> > > > +                  - aspeed,ast2500-sdhci
> > > > +                  - aspeed,ast2600-sdhci
> > > > +          - items:
> > > > +              - enum:
> > > > +                  - aspeed,ast2600-sdhci
> > > > +              - const: sdhci
> > >
> > > Why are you adding 'sdhci'. That's not useful as a compatible given how
> > > many quirks different implementations have.
> > >
> > >
> >
> > It is for passing the dtbs_check of the second example.
> > Without this definition, many device trees have the following
> > error:
> >
> > ['aspeed,ast2600-sdhci', 'sdhci'] is too long
> > Additional items are not allowed ('sdhci' was unexpected)
> 
> I would probably fix the dts files then. Does anything depend on 'sdhci'?
> 

The build error is caused by my second example.
My example is refer to the aspeed-g6.dtsi to append "sdhci" after
"aspeed,ast2600-sdhci"
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/arch/arm/boot/dts/aspeed-g6.dtsi?h=v5.12#n561

As long as the second example is removed, the following error won't show in the dtbs_check result.

```
  SCHEMA  Documentation/devicetree/bindings/processed-schema.json
  DTC     arch/arm/boot/dts/aspeed-ast2500-evb.dt.yaml
  CHECK   arch/arm/boot/dts/aspeed-ast2500-evb.dt.yaml
  DTC     arch/arm/boot/dts/aspeed-ast2600-evb.dt.yaml
  CHECK   arch/arm/boot/dts/aspeed-ast2600-evb.dt.yaml
/home/slee/aspeed/patch_up/linux/arch/arm/boot/dts/aspeed-ast2600-evb.dt.yaml: sdc@1e740000: sdhci@1e740100:compatible: ['aspeed,ast2600-sdhci', 'sdhci'] is too long
        From schema: /home/slee/aspeed/patch_up/linux/Documentation/devicetree/bindings/mmc/aspeed,sdhci.yaml
/home/slee/aspeed/patch_up/linux/arch/arm/boot/dts/aspeed-ast2600-evb.dt.yaml: sdc@1e740000: sdhci@1e740100:compatible: Additional items are not allowed ('sdhci' was unexpected)
        From schema: /home/slee/aspeed/patch_up/linux/Documentation/devicetree/bindings/mmc/aspeed,sdhci.yaml
/home/slee/aspeed/patch_up/linux/arch/arm/boot/dts/aspeed-ast2600-evb.dt.yaml: sdc@1e740000: sdhci@1e740200:compatible: ['aspeed,ast2600-sdhci', 'sdhci'] is too long
        From schema: /home/slee/aspeed/patch_up/linux/Documentation/devicetree/bindings/mmc/aspeed,sdhci.yaml
/home/slee/aspeed/patch_up/linux/arch/arm/boot/dts/aspeed-ast2600-evb.dt.yaml: sdc@1e740000: sdhci@1e740200:compatible: Additional items are not allowed ('sdhci' was unexpected)
        From schema: /home/slee/aspeed/patch_up/linux/Documentation/devicetree/bindings/mmc/aspeed,sdhci.yaml

```

> Rob

_______________________________________________
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] 28+ messages in thread

* Re: [PATCH v3 5/5] mmc: sdhci-of-aspeed: Assert/Deassert reset signal before probing eMMC
  2021-05-07  7:36         ` Andrew Jeffery
@ 2021-05-10  6:03           ` Steven Lee
  2021-05-13  0:42             ` Andrew Jeffery
  0 siblings, 1 reply; 28+ messages in thread
From: Steven Lee @ 2021-05-10  6:03 UTC (permalink / raw)
  To: Andrew Jeffery
  Cc: Philipp Zabel, Ulf Hansson, Rob Herring, Joel Stanley,
	Adrian Hunter, 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/07/2021 15:36, Andrew Jeffery wrote:
> 
> 
> On Fri, 7 May 2021, at 15:54, Steven Lee wrote:
> > The 05/07/2021 09:32, Andrew Jeffery wrote:
> > > 
> > > 
> > > On Thu, 6 May 2021, at 19:54, Philipp Zabel wrote:
> > > > Hi Steven,
> > > > 
> > > > On Thu, May 06, 2021 at 06:03:12PM +0800, Steven Lee wrote:
> > > > > +	if (info) {
> > > > > +		if (info->flag & PROBE_AFTER_ASSET_DEASSERT) {
> > > > > +			sdc->rst = devm_reset_control_get(&pdev->dev, NULL);
> > > > 
> > > > Please use devm_reset_control_get_exclusive() or
> > > > devm_reset_control_get_optional_exclusive().
> > > > 
> > > > > +			if (!IS_ERR(sdc->rst)) {
> > > > 
> > > > Please just return errors here instead of ignoring them.
> > > > The reset_control_get_optional variants return NULL in case the
> > > > device node doesn't contain a resets phandle, in case you really
> > > > consider this reset to be optional even though the flag is set?
> > > 
> > > It feels like we should get rid of the flag and leave it to the 
> > > devicetree.
> > > 
> > 
> > Do you mean adding a flag, for instance, "mmc-reset" in the
> > device tree and call of_property_read_bool() in aspeed_sdc_probe()?
> > 
> > > I'm still kind of surprised it's not something we want to do for the 
> > > 2400 and 2500 as well.
> > > 
> > 
> > Per discussion with the chip designer, AST2400 and AST2500 doesn't need
> > this implementation since the chip design is different to AST2600.
> 
> So digging a bit more deeply on this, it looks like the reset is 
> already taken care of by drivers/clk/clk-ast2600.c in the 
> clk_prepare_enable() path.
> 
> clk-ast2600 handles resets when enabling the clock for most peripherals:
> 
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/clk/clk-ast2600.c?h=v5.12#n276
> 
> and this is true for both the SD controller and the eMMC controller:
> 
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/clk/clk-ast2600.c?h=v5.12#n94
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/clk/clk-ast2600.c?h=v5.12#n88
> 
> If this weren't the case you'd specify a reset property in the SD/eMMC 
> devicetree nodes for the 2600 and then use 
> devm_reset_control_get_optional_exclusive() as Philipp suggested. See 
> the reset binding here:
> 
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/devicetree/bindings/reset/reset.txt?h=v5.12
> 
> So on the surface it seems the reset handling in this patch is 
> unnecessary. Have you observed an issue with the SoC that means it's 
> required?
> 

Yes, you are right, aspeed_sdc_probe() calls clk_prepare_enable(),
aspeed_g6_clk_enable() does reset eMMC.

https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/mmc/host/sdhci-of-aspeed.c#n496

However, the clock of eMMC is enabled in my u-boot(2019.04).
So it is retruned in the condition of aspeed_g6_clk_is_enabled() below
and doesn't reset eMMC.

https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/clk/clk-ast2600.c?h=v5.12#n285


> 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] 28+ messages in thread

* Re: [PATCH v3 5/5] mmc: sdhci-of-aspeed: Assert/Deassert reset signal before probing eMMC
  2021-05-10  6:03           ` Steven Lee
@ 2021-05-13  0:42             ` Andrew Jeffery
  2021-05-14  2:09               ` Steven Lee
  0 siblings, 1 reply; 28+ messages in thread
From: Andrew Jeffery @ 2021-05-13  0:42 UTC (permalink / raw)
  To: Steven Lee
  Cc: Philipp Zabel, Ulf Hansson, Rob Herring, Joel Stanley,
	Adrian Hunter, 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, 10 May 2021, at 15:33, Steven Lee wrote:
> The 05/07/2021 15:36, Andrew Jeffery wrote:
> > 
> > 
> > On Fri, 7 May 2021, at 15:54, Steven Lee wrote:
> > > The 05/07/2021 09:32, Andrew Jeffery wrote:
> > > > 
> > > > 
> > > > On Thu, 6 May 2021, at 19:54, Philipp Zabel wrote:
> > > > > Hi Steven,
> > > > > 
> > > > > On Thu, May 06, 2021 at 06:03:12PM +0800, Steven Lee wrote:
> > > > > > +	if (info) {
> > > > > > +		if (info->flag & PROBE_AFTER_ASSET_DEASSERT) {
> > > > > > +			sdc->rst = devm_reset_control_get(&pdev->dev, NULL);
> > > > > 
> > > > > Please use devm_reset_control_get_exclusive() or
> > > > > devm_reset_control_get_optional_exclusive().
> > > > > 
> > > > > > +			if (!IS_ERR(sdc->rst)) {
> > > > > 
> > > > > Please just return errors here instead of ignoring them.
> > > > > The reset_control_get_optional variants return NULL in case the
> > > > > device node doesn't contain a resets phandle, in case you really
> > > > > consider this reset to be optional even though the flag is set?
> > > > 
> > > > It feels like we should get rid of the flag and leave it to the 
> > > > devicetree.
> > > > 
> > > 
> > > Do you mean adding a flag, for instance, "mmc-reset" in the
> > > device tree and call of_property_read_bool() in aspeed_sdc_probe()?
> > > 
> > > > I'm still kind of surprised it's not something we want to do for the 
> > > > 2400 and 2500 as well.
> > > > 
> > > 
> > > Per discussion with the chip designer, AST2400 and AST2500 doesn't need
> > > this implementation since the chip design is different to AST2600.
> > 
> > So digging a bit more deeply on this, it looks like the reset is 
> > already taken care of by drivers/clk/clk-ast2600.c in the 
> > clk_prepare_enable() path.
> > 
> > clk-ast2600 handles resets when enabling the clock for most peripherals:
> > 
> > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/clk/clk-ast2600.c?h=v5.12#n276
> > 
> > and this is true for both the SD controller and the eMMC controller:
> > 
> > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/clk/clk-ast2600.c?h=v5.12#n94
> > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/clk/clk-ast2600.c?h=v5.12#n88
> > 
> > If this weren't the case you'd specify a reset property in the SD/eMMC 
> > devicetree nodes for the 2600 and then use 
> > devm_reset_control_get_optional_exclusive() as Philipp suggested. See 
> > the reset binding here:
> > 
> > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/devicetree/bindings/reset/reset.txt?h=v5.12
> > 
> > So on the surface it seems the reset handling in this patch is 
> > unnecessary. Have you observed an issue with the SoC that means it's 
> > required?
> > 
> 
> Yes, you are right, aspeed_sdc_probe() calls clk_prepare_enable(),
> aspeed_g6_clk_enable() does reset eMMC.
> 
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/mmc/host/sdhci-of-aspeed.c#n496
> 
> However, the clock of eMMC is enabled in my u-boot(2019.04).
> So it is retruned in the condition of aspeed_g6_clk_is_enabled() below
> and doesn't reset eMMC.
> 
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/clk/clk-ast2600.c?h=v5.12#n285

Okay, so what's the issue that the patch addresses? Is there a bug? 
Presumably if u-boot isn't making use of the eMMC the clock won't be 
on, so we'll do the reset if the kernel wants to make use of the 
device. If u-boot _is_ using the eMMC, u-boot will have done the 
correct clock enable/reset sequence and so the controller should be 
ready to go?

The only potential issue remaining is u-boot leaving the controller in 
a configuration the kernel isn't expecting when handing over. If that's 
the issue then we've forgotten to do some specific initialisation (i.e. 
not just reset the entire thing) of the controller in the driver probe 
path, right?

FWIW I haven't recently seen any poor behaviour from the controller or 
driver. For us (IBM) it seems to be working well since we sorted out 
the phase configuration.

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] 28+ messages in thread

* Re: [PATCH v3 5/5] mmc: sdhci-of-aspeed: Assert/Deassert reset signal before probing eMMC
  2021-05-13  0:42             ` Andrew Jeffery
@ 2021-05-14  2:09               ` Steven Lee
  2021-05-14  2:37                 ` Andrew Jeffery
  0 siblings, 1 reply; 28+ messages in thread
From: Steven Lee @ 2021-05-14  2:09 UTC (permalink / raw)
  To: Andrew Jeffery
  Cc: Philipp Zabel, Ulf Hansson, Rob Herring, Joel Stanley,
	Adrian Hunter, 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/13/2021 08:42, Andrew Jeffery wrote:
> 
> 
> On Mon, 10 May 2021, at 15:33, Steven Lee wrote:
> > The 05/07/2021 15:36, Andrew Jeffery wrote:
> > > 
> > > 
> > > On Fri, 7 May 2021, at 15:54, Steven Lee wrote:
> > > > The 05/07/2021 09:32, Andrew Jeffery wrote:
> > > > > 
> > > > > 
> > > > > On Thu, 6 May 2021, at 19:54, Philipp Zabel wrote:
> > > > > > Hi Steven,
> > > > > > 
> > > > > > On Thu, May 06, 2021 at 06:03:12PM +0800, Steven Lee wrote:
> > > > > > > +	if (info) {
> > > > > > > +		if (info->flag & PROBE_AFTER_ASSET_DEASSERT) {
> > > > > > > +			sdc->rst = devm_reset_control_get(&pdev->dev, NULL);
> > > > > > 
> > > > > > Please use devm_reset_control_get_exclusive() or
> > > > > > devm_reset_control_get_optional_exclusive().
> > > > > > 
> > > > > > > +			if (!IS_ERR(sdc->rst)) {
> > > > > > 
> > > > > > Please just return errors here instead of ignoring them.
> > > > > > The reset_control_get_optional variants return NULL in case the
> > > > > > device node doesn't contain a resets phandle, in case you really
> > > > > > consider this reset to be optional even though the flag is set?
> > > > > 
> > > > > It feels like we should get rid of the flag and leave it to the 
> > > > > devicetree.
> > > > > 
> > > > 
> > > > Do you mean adding a flag, for instance, "mmc-reset" in the
> > > > device tree and call of_property_read_bool() in aspeed_sdc_probe()?
> > > > 
> > > > > I'm still kind of surprised it's not something we want to do for the 
> > > > > 2400 and 2500 as well.
> > > > > 
> > > > 
> > > > Per discussion with the chip designer, AST2400 and AST2500 doesn't need
> > > > this implementation since the chip design is different to AST2600.
> > > 
> > > So digging a bit more deeply on this, it looks like the reset is 
> > > already taken care of by drivers/clk/clk-ast2600.c in the 
> > > clk_prepare_enable() path.
> > > 
> > > clk-ast2600 handles resets when enabling the clock for most peripherals:
> > > 
> > > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/clk/clk-ast2600.c?h=v5.12#n276
> > > 
> > > and this is true for both the SD controller and the eMMC controller:
> > > 
> > > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/clk/clk-ast2600.c?h=v5.12#n94
> > > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/clk/clk-ast2600.c?h=v5.12#n88
> > > 
> > > If this weren't the case you'd specify a reset property in the SD/eMMC 
> > > devicetree nodes for the 2600 and then use 
> > > devm_reset_control_get_optional_exclusive() as Philipp suggested. See 
> > > the reset binding here:
> > > 
> > > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/devicetree/bindings/reset/reset.txt?h=v5.12
> > > 
> > > So on the surface it seems the reset handling in this patch is 
> > > unnecessary. Have you observed an issue with the SoC that means it's 
> > > required?
> > > 
> > 
> > Yes, you are right, aspeed_sdc_probe() calls clk_prepare_enable(),
> > aspeed_g6_clk_enable() does reset eMMC.
> > 
> > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/mmc/host/sdhci-of-aspeed.c#n496
> > 
> > However, the clock of eMMC is enabled in my u-boot(2019.04).
> > So it is retruned in the condition of aspeed_g6_clk_is_enabled() below
> > and doesn't reset eMMC.
> > 
> > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/clk/clk-ast2600.c?h=v5.12#n285
> 
> Okay, so what's the issue that the patch addresses? Is there a bug? 
> Presumably if u-boot isn't making use of the eMMC the clock won't be 
> on, so we'll do the reset if the kernel wants to make use of the 
> device. If u-boot _is_ using the eMMC, u-boot will have done the 
> correct clock enable/reset sequence and so the controller should be 
> ready to go?
> 
> The only potential issue remaining is u-boot leaving the controller in 
> a configuration the kernel isn't expecting when handing over. If that's 
> the issue then we've forgotten to do some specific initialisation (i.e. 
> not just reset the entire thing) of the controller in the driver probe 
> path, right?
> 

If DMA engine is used before probing eMMC in kernel stage,
eMMC controller may have unexpected behavior when re-exectuing
identifying process.
Thus, we need to reset at the beginning of kernel since
kernel is a new stage. We should not assume some one do something
before.

> FWIW I haven't recently seen any poor behaviour from the controller or 
> driver. For us (IBM) it seems to be working well since we sorted out 
> the phase configuration.
> 

Yes, you are right, everything work well currently. But, kernel is a new
stage, we cannot assume eMMC controller is at initial state when
entering kernel stage.

> 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] 28+ messages in thread

* Re: [PATCH v3 5/5] mmc: sdhci-of-aspeed: Assert/Deassert reset signal before probing eMMC
  2021-05-14  2:09               ` Steven Lee
@ 2021-05-14  2:37                 ` Andrew Jeffery
  2021-05-19 10:57                   ` Steven Lee
  0 siblings, 1 reply; 28+ messages in thread
From: Andrew Jeffery @ 2021-05-14  2:37 UTC (permalink / raw)
  To: Steven Lee
  Cc: Philipp Zabel, Ulf Hansson, Rob Herring, Joel Stanley,
	Adrian Hunter, 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 Fri, 14 May 2021, at 11:39, Steven Lee wrote:
> The 05/13/2021 08:42, Andrew Jeffery wrote:
> > 
> > 
> > On Mon, 10 May 2021, at 15:33, Steven Lee wrote:
> > > The 05/07/2021 15:36, Andrew Jeffery wrote:
> > > > 
> > > > 
> > > > On Fri, 7 May 2021, at 15:54, Steven Lee wrote:
> > > > > The 05/07/2021 09:32, Andrew Jeffery wrote:
> > > > > > 
> > > > > > 
> > > > > > On Thu, 6 May 2021, at 19:54, Philipp Zabel wrote:
> > > > > > > Hi Steven,
> > > > > > > 
> > > > > > > On Thu, May 06, 2021 at 06:03:12PM +0800, Steven Lee wrote:
> > > > > > > > +	if (info) {
> > > > > > > > +		if (info->flag & PROBE_AFTER_ASSET_DEASSERT) {
> > > > > > > > +			sdc->rst = devm_reset_control_get(&pdev->dev, NULL);
> > > > > > > 
> > > > > > > Please use devm_reset_control_get_exclusive() or
> > > > > > > devm_reset_control_get_optional_exclusive().
> > > > > > > 
> > > > > > > > +			if (!IS_ERR(sdc->rst)) {
> > > > > > > 
> > > > > > > Please just return errors here instead of ignoring them.
> > > > > > > The reset_control_get_optional variants return NULL in case the
> > > > > > > device node doesn't contain a resets phandle, in case you really
> > > > > > > consider this reset to be optional even though the flag is set?
> > > > > > 
> > > > > > It feels like we should get rid of the flag and leave it to the 
> > > > > > devicetree.
> > > > > > 
> > > > > 
> > > > > Do you mean adding a flag, for instance, "mmc-reset" in the
> > > > > device tree and call of_property_read_bool() in aspeed_sdc_probe()?
> > > > > 
> > > > > > I'm still kind of surprised it's not something we want to do for the 
> > > > > > 2400 and 2500 as well.
> > > > > > 
> > > > > 
> > > > > Per discussion with the chip designer, AST2400 and AST2500 doesn't need
> > > > > this implementation since the chip design is different to AST2600.
> > > > 
> > > > So digging a bit more deeply on this, it looks like the reset is 
> > > > already taken care of by drivers/clk/clk-ast2600.c in the 
> > > > clk_prepare_enable() path.
> > > > 
> > > > clk-ast2600 handles resets when enabling the clock for most peripherals:
> > > > 
> > > > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/clk/clk-ast2600.c?h=v5.12#n276
> > > > 
> > > > and this is true for both the SD controller and the eMMC controller:
> > > > 
> > > > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/clk/clk-ast2600.c?h=v5.12#n94
> > > > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/clk/clk-ast2600.c?h=v5.12#n88
> > > > 
> > > > If this weren't the case you'd specify a reset property in the SD/eMMC 
> > > > devicetree nodes for the 2600 and then use 
> > > > devm_reset_control_get_optional_exclusive() as Philipp suggested. See 
> > > > the reset binding here:
> > > > 
> > > > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/devicetree/bindings/reset/reset.txt?h=v5.12
> > > > 
> > > > So on the surface it seems the reset handling in this patch is 
> > > > unnecessary. Have you observed an issue with the SoC that means it's 
> > > > required?
> > > > 
> > > 
> > > Yes, you are right, aspeed_sdc_probe() calls clk_prepare_enable(),
> > > aspeed_g6_clk_enable() does reset eMMC.
> > > 
> > > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/mmc/host/sdhci-of-aspeed.c#n496
> > > 
> > > However, the clock of eMMC is enabled in my u-boot(2019.04).
> > > So it is retruned in the condition of aspeed_g6_clk_is_enabled() below
> > > and doesn't reset eMMC.
> > > 
> > > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/clk/clk-ast2600.c?h=v5.12#n285
> > 
> > Okay, so what's the issue that the patch addresses? Is there a bug? 
> > Presumably if u-boot isn't making use of the eMMC the clock won't be 
> > on, so we'll do the reset if the kernel wants to make use of the 
> > device. If u-boot _is_ using the eMMC, u-boot will have done the 
> > correct clock enable/reset sequence and so the controller should be 
> > ready to go?
> > 
> > The only potential issue remaining is u-boot leaving the controller in 
> > a configuration the kernel isn't expecting when handing over. If that's 
> > the issue then we've forgotten to do some specific initialisation (i.e. 
> > not just reset the entire thing) of the controller in the driver probe 
> > path, right?
> > 
> 
> If DMA engine is used before probing eMMC in kernel stage,
> eMMC controller may have unexpected behavior when re-exectuing
> identifying process.
> Thus, we need to reset at the beginning of kernel since
> kernel is a new stage. We should not assume some one do something
> before.
> 
> > FWIW I haven't recently seen any poor behaviour from the controller or 
> > driver. For us (IBM) it seems to be working well since we sorted out 
> > the phase configuration.
> > 
> 
> Yes, you are right, everything work well currently. But, kernel is a new
> stage, we cannot assume eMMC controller is at initial state when
> entering kernel stage.

Okay. That sounds true no matter what the hardware design though (going 
back to the difference between the 2400/2500 and 2600).

Given the reset is tied up in the clock gating, it would be nice if we 
could do the following in aspeed_sdc_probe():

```
/* Clean up the controller in case it wasn't left in a good state by the bootloader */
clock_disable_unprepare(...);

clock_prepare_enable(...);
```

But the enable_count tracked by clock_core_{en,dis}able() kills that 
idea.

This makes it seem like we need to break out the appropriate indexes 
to add `resets` properties in the devicetree. This will need some input 
from Joel, given the eMMC/SD resets can't currently be handled that way.

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] 28+ messages in thread

* Re: [PATCH v3 5/5] mmc: sdhci-of-aspeed: Assert/Deassert reset signal before probing eMMC
  2021-05-14  2:37                 ` Andrew Jeffery
@ 2021-05-19 10:57                   ` Steven Lee
  2021-05-19 23:09                     ` Andrew Jeffery
  0 siblings, 1 reply; 28+ messages in thread
From: Steven Lee @ 2021-05-19 10:57 UTC (permalink / raw)
  To: Andrew Jeffery
  Cc: Philipp Zabel, Ulf Hansson, Rob Herring, Joel Stanley,
	Adrian Hunter, 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/14/2021 10:37, Andrew Jeffery wrote:
> 
> 
> On Fri, 14 May 2021, at 11:39, Steven Lee wrote:
> > The 05/13/2021 08:42, Andrew Jeffery wrote:
> > > 
> > > 
> > > On Mon, 10 May 2021, at 15:33, Steven Lee wrote:
> > > > The 05/07/2021 15:36, Andrew Jeffery wrote:
> > > > > 
> > > > > 
> > > > > On Fri, 7 May 2021, at 15:54, Steven Lee wrote:
> > > > > > The 05/07/2021 09:32, Andrew Jeffery wrote:
> > > > > > > 
> > > > > > > 
> > > > > > > On Thu, 6 May 2021, at 19:54, Philipp Zabel wrote:
> > > > > > > > Hi Steven,
> > > > > > > > 
> > > > > > > > On Thu, May 06, 2021 at 06:03:12PM +0800, Steven Lee wrote:
> > > > > > > > > +	if (info) {
> > > > > > > > > +		if (info->flag & PROBE_AFTER_ASSET_DEASSERT) {
> > > > > > > > > +			sdc->rst = devm_reset_control_get(&pdev->dev, NULL);
> > > > > > > > 
> > > > > > > > Please use devm_reset_control_get_exclusive() or
> > > > > > > > devm_reset_control_get_optional_exclusive().
> > > > > > > > 
> > > > > > > > > +			if (!IS_ERR(sdc->rst)) {
> > > > > > > > 
> > > > > > > > Please just return errors here instead of ignoring them.
> > > > > > > > The reset_control_get_optional variants return NULL in case the
> > > > > > > > device node doesn't contain a resets phandle, in case you really
> > > > > > > > consider this reset to be optional even though the flag is set?
> > > > > > > 
> > > > > > > It feels like we should get rid of the flag and leave it to the 
> > > > > > > devicetree.
> > > > > > > 
> > > > > > 
> > > > > > Do you mean adding a flag, for instance, "mmc-reset" in the
> > > > > > device tree and call of_property_read_bool() in aspeed_sdc_probe()?
> > > > > > 
> > > > > > > I'm still kind of surprised it's not something we want to do for the 
> > > > > > > 2400 and 2500 as well.
> > > > > > > 
> > > > > > 
> > > > > > Per discussion with the chip designer, AST2400 and AST2500 doesn't need
> > > > > > this implementation since the chip design is different to AST2600.
> > > > > 
> > > > > So digging a bit more deeply on this, it looks like the reset is 
> > > > > already taken care of by drivers/clk/clk-ast2600.c in the 
> > > > > clk_prepare_enable() path.
> > > > > 
> > > > > clk-ast2600 handles resets when enabling the clock for most peripherals:
> > > > > 
> > > > > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/clk/clk-ast2600.c?h=v5.12#n276
> > > > > 
> > > > > and this is true for both the SD controller and the eMMC controller:
> > > > > 
> > > > > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/clk/clk-ast2600.c?h=v5.12#n94
> > > > > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/clk/clk-ast2600.c?h=v5.12#n88
> > > > > 
> > > > > If this weren't the case you'd specify a reset property in the SD/eMMC 
> > > > > devicetree nodes for the 2600 and then use 
> > > > > devm_reset_control_get_optional_exclusive() as Philipp suggested. See 
> > > > > the reset binding here:
> > > > > 
> > > > > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/devicetree/bindings/reset/reset.txt?h=v5.12
> > > > > 
> > > > > So on the surface it seems the reset handling in this patch is 
> > > > > unnecessary. Have you observed an issue with the SoC that means it's 
> > > > > required?
> > > > > 
> > > > 
> > > > Yes, you are right, aspeed_sdc_probe() calls clk_prepare_enable(),
> > > > aspeed_g6_clk_enable() does reset eMMC.
> > > > 
> > > > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/mmc/host/sdhci-of-aspeed.c#n496
> > > > 
> > > > However, the clock of eMMC is enabled in my u-boot(2019.04).
> > > > So it is retruned in the condition of aspeed_g6_clk_is_enabled() below
> > > > and doesn't reset eMMC.
> > > > 
> > > > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/clk/clk-ast2600.c?h=v5.12#n285
> > > 
> > > Okay, so what's the issue that the patch addresses? Is there a bug? 
> > > Presumably if u-boot isn't making use of the eMMC the clock won't be 
> > > on, so we'll do the reset if the kernel wants to make use of the 
> > > device. If u-boot _is_ using the eMMC, u-boot will have done the 
> > > correct clock enable/reset sequence and so the controller should be 
> > > ready to go?
> > > 
> > > The only potential issue remaining is u-boot leaving the controller in 
> > > a configuration the kernel isn't expecting when handing over. If that's 
> > > the issue then we've forgotten to do some specific initialisation (i.e. 
> > > not just reset the entire thing) of the controller in the driver probe 
> > > path, right?
> > > 
> > 
> > If DMA engine is used before probing eMMC in kernel stage,
> > eMMC controller may have unexpected behavior when re-exectuing
> > identifying process.
> > Thus, we need to reset at the beginning of kernel since
> > kernel is a new stage. We should not assume some one do something
> > before.
> > 
> > > FWIW I haven't recently seen any poor behaviour from the controller or 
> > > driver. For us (IBM) it seems to be working well since we sorted out 
> > > the phase configuration.
> > > 
> > 
> > Yes, you are right, everything work well currently. But, kernel is a new
> > stage, we cannot assume eMMC controller is at initial state when
> > entering kernel stage.
> 
> Okay. That sounds true no matter what the hardware design though (going 
> back to the difference between the 2400/2500 and 2600).
> 
> Given the reset is tied up in the clock gating, it would be nice if we 
> could do the following in aspeed_sdc_probe():
> 
> ```
> /* Clean up the controller in case it wasn't left in a good state by the bootloader */
> clock_disable_unprepare(...);
> 
> clock_prepare_enable(...);
> ```
> 
> But the enable_count tracked by clock_core_{en,dis}able() kills that 
> idea.
> 
> This makes it seem like we need to break out the appropriate indexes 
> to add `resets` properties in the devicetree. This will need some input 
> from Joel, given the eMMC/SD resets can't currently be handled that way.
> 

Hi Adnrew,

I was wondering if I should wait for Joel's comment, or

1. Drop this patch in this patch series, and prepare another patch
series for this issue in the future. Since sdhci works well as long as we
don't use DMA engine before kernel stage.

2. Modify the reset control according to Philipp and your comment.

Thanks and look forward to having your suggestion.

Steven

> 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] 28+ messages in thread

* Re: [PATCH v3 5/5] mmc: sdhci-of-aspeed: Assert/Deassert reset signal before probing eMMC
  2021-05-19 10:57                   ` Steven Lee
@ 2021-05-19 23:09                     ` Andrew Jeffery
  0 siblings, 0 replies; 28+ messages in thread
From: Andrew Jeffery @ 2021-05-19 23:09 UTC (permalink / raw)
  To: Steven Lee
  Cc: Philipp Zabel, Ulf Hansson, Rob Herring, Joel Stanley,
	Adrian Hunter, 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 Wed, 19 May 2021, at 20:27, Steven Lee wrote:
> The 05/14/2021 10:37, Andrew Jeffery wrote:
> > 
> > 
> > On Fri, 14 May 2021, at 11:39, Steven Lee wrote:
> > > The 05/13/2021 08:42, Andrew Jeffery wrote:
> > > > 
> > > > 
> > > > On Mon, 10 May 2021, at 15:33, Steven Lee wrote:
> > > > > The 05/07/2021 15:36, Andrew Jeffery wrote:
> > > > > > 
> > > > > > 
> > > > > > On Fri, 7 May 2021, at 15:54, Steven Lee wrote:
> > > > > > > The 05/07/2021 09:32, Andrew Jeffery wrote:
> > > > > > > > 
> > > > > > > > 
> > > > > > > > On Thu, 6 May 2021, at 19:54, Philipp Zabel wrote:
> > > > > > > > > Hi Steven,
> > > > > > > > > 
> > > > > > > > > On Thu, May 06, 2021 at 06:03:12PM +0800, Steven Lee wrote:
> > > > > > > > > > +	if (info) {
> > > > > > > > > > +		if (info->flag & PROBE_AFTER_ASSET_DEASSERT) {
> > > > > > > > > > +			sdc->rst = devm_reset_control_get(&pdev->dev, NULL);
> > > > > > > > > 
> > > > > > > > > Please use devm_reset_control_get_exclusive() or
> > > > > > > > > devm_reset_control_get_optional_exclusive().
> > > > > > > > > 
> > > > > > > > > > +			if (!IS_ERR(sdc->rst)) {
> > > > > > > > > 
> > > > > > > > > Please just return errors here instead of ignoring them.
> > > > > > > > > The reset_control_get_optional variants return NULL in case the
> > > > > > > > > device node doesn't contain a resets phandle, in case you really
> > > > > > > > > consider this reset to be optional even though the flag is set?
> > > > > > > > 
> > > > > > > > It feels like we should get rid of the flag and leave it to the 
> > > > > > > > devicetree.
> > > > > > > > 
> > > > > > > 
> > > > > > > Do you mean adding a flag, for instance, "mmc-reset" in the
> > > > > > > device tree and call of_property_read_bool() in aspeed_sdc_probe()?
> > > > > > > 
> > > > > > > > I'm still kind of surprised it's not something we want to do for the 
> > > > > > > > 2400 and 2500 as well.
> > > > > > > > 
> > > > > > > 
> > > > > > > Per discussion with the chip designer, AST2400 and AST2500 doesn't need
> > > > > > > this implementation since the chip design is different to AST2600.
> > > > > > 
> > > > > > So digging a bit more deeply on this, it looks like the reset is 
> > > > > > already taken care of by drivers/clk/clk-ast2600.c in the 
> > > > > > clk_prepare_enable() path.
> > > > > > 
> > > > > > clk-ast2600 handles resets when enabling the clock for most peripherals:
> > > > > > 
> > > > > > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/clk/clk-ast2600.c?h=v5.12#n276
> > > > > > 
> > > > > > and this is true for both the SD controller and the eMMC controller:
> > > > > > 
> > > > > > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/clk/clk-ast2600.c?h=v5.12#n94
> > > > > > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/clk/clk-ast2600.c?h=v5.12#n88
> > > > > > 
> > > > > > If this weren't the case you'd specify a reset property in the SD/eMMC 
> > > > > > devicetree nodes for the 2600 and then use 
> > > > > > devm_reset_control_get_optional_exclusive() as Philipp suggested. See 
> > > > > > the reset binding here:
> > > > > > 
> > > > > > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/devicetree/bindings/reset/reset.txt?h=v5.12
> > > > > > 
> > > > > > So on the surface it seems the reset handling in this patch is 
> > > > > > unnecessary. Have you observed an issue with the SoC that means it's 
> > > > > > required?
> > > > > > 
> > > > > 
> > > > > Yes, you are right, aspeed_sdc_probe() calls clk_prepare_enable(),
> > > > > aspeed_g6_clk_enable() does reset eMMC.
> > > > > 
> > > > > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/mmc/host/sdhci-of-aspeed.c#n496
> > > > > 
> > > > > However, the clock of eMMC is enabled in my u-boot(2019.04).
> > > > > So it is retruned in the condition of aspeed_g6_clk_is_enabled() below
> > > > > and doesn't reset eMMC.
> > > > > 
> > > > > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/clk/clk-ast2600.c?h=v5.12#n285
> > > > 
> > > > Okay, so what's the issue that the patch addresses? Is there a bug? 
> > > > Presumably if u-boot isn't making use of the eMMC the clock won't be 
> > > > on, so we'll do the reset if the kernel wants to make use of the 
> > > > device. If u-boot _is_ using the eMMC, u-boot will have done the 
> > > > correct clock enable/reset sequence and so the controller should be 
> > > > ready to go?
> > > > 
> > > > The only potential issue remaining is u-boot leaving the controller in 
> > > > a configuration the kernel isn't expecting when handing over. If that's 
> > > > the issue then we've forgotten to do some specific initialisation (i.e. 
> > > > not just reset the entire thing) of the controller in the driver probe 
> > > > path, right?
> > > > 
> > > 
> > > If DMA engine is used before probing eMMC in kernel stage,
> > > eMMC controller may have unexpected behavior when re-exectuing
> > > identifying process.
> > > Thus, we need to reset at the beginning of kernel since
> > > kernel is a new stage. We should not assume some one do something
> > > before.
> > > 
> > > > FWIW I haven't recently seen any poor behaviour from the controller or 
> > > > driver. For us (IBM) it seems to be working well since we sorted out 
> > > > the phase configuration.
> > > > 
> > > 
> > > Yes, you are right, everything work well currently. But, kernel is a new
> > > stage, we cannot assume eMMC controller is at initial state when
> > > entering kernel stage.
> > 
> > Okay. That sounds true no matter what the hardware design though (going 
> > back to the difference between the 2400/2500 and 2600).
> > 
> > Given the reset is tied up in the clock gating, it would be nice if we 
> > could do the following in aspeed_sdc_probe():
> > 
> > ```
> > /* Clean up the controller in case it wasn't left in a good state by the bootloader */
> > clock_disable_unprepare(...);
> > 
> > clock_prepare_enable(...);
> > ```
> > 
> > But the enable_count tracked by clock_core_{en,dis}able() kills that 
> > idea.
> > 
> > This makes it seem like we need to break out the appropriate indexes 
> > to add `resets` properties in the devicetree. This will need some input 
> > from Joel, given the eMMC/SD resets can't currently be handled that way.
> > 
> 
> Hi Adnrew,
> 
> I was wondering if I should wait for Joel's comment, or
> 
> 1. Drop this patch in this patch series, and prepare another patch
> series for this issue in the future. Since sdhci works well as long as we
> don't use DMA engine before kernel stage.

I think this is the way forward, it's kind of separate to what you're 
trying to achieve with the rest of the patches in this series.

I'll poke Joel.

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] 28+ messages in thread

end of thread, other threads:[~2021-05-19 23:12 UTC | newest]

Thread overview: 28+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-05-06 10:03 [PATCH v3 0/5] mmc: sdhci-of-aspeed: Support toggling SD bus signal Steven Lee
2021-05-06 10:03 ` [PATCH v3 1/5] dt-bindings: mmc: sdhci-of-aspeed: Add an example for AST2600-A2 EVB Steven Lee
2021-05-07  1:13   ` Rob Herring
2021-05-07  3:13     ` Steven Lee
2021-05-07 17:21       ` Rob Herring
2021-05-10  2:32         ` Steven Lee
2021-05-06 10:03 ` [PATCH v3 2/5] ARM: dts: aspeed: ast2600evb: Add comment for gpio regulator of sdhci Steven Lee
2021-05-07  1:40   ` Andrew Jeffery
2021-05-07  3:30     ` Steven Lee
2021-05-07  3:42       ` Andrew Jeffery
2021-05-06 10:03 ` [PATCH v3 3/5] ARM: dts: aspeed: ast2600evb: Add phase correction for emmc controller Steven Lee
2021-05-07  1:34   ` Andrew Jeffery
2021-05-06 10:03 ` [PATCH v3 4/5] mmc: sdhci-of-aspeed: Add a helper for updating capability register Steven Lee
2021-05-07  2:13   ` Andrew Jeffery
2021-05-07  6:59     ` Steven Lee
2021-05-07  7:07       ` Andrew Jeffery
2021-05-06 10:03 ` [PATCH v3 5/5] mmc: sdhci-of-aspeed: Assert/Deassert reset signal before probing eMMC Steven Lee
2021-05-06 10:24   ` Philipp Zabel
2021-05-07  1:32     ` Andrew Jeffery
2021-05-07  6:24       ` Steven Lee
2021-05-07  7:36         ` Andrew Jeffery
2021-05-10  6:03           ` Steven Lee
2021-05-13  0:42             ` Andrew Jeffery
2021-05-14  2:09               ` Steven Lee
2021-05-14  2:37                 ` Andrew Jeffery
2021-05-19 10:57                   ` Steven Lee
2021-05-19 23:09                     ` Andrew Jeffery
2021-05-07  6:02     ` Steven Lee

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).