openbmc.lists.ozlabs.org archive mirror
 help / color / mirror / Atom feed
* [PATCH 00/10] ASPEED SD/eMMC controller clock configuration
@ 2021-09-22 10:31 Chin-Ting Kuo
  2021-09-22 10:31 ` [PATCH 01/10] clk: aspeed: ast2600: Porting sdhci clock source Chin-Ting Kuo
                   ` (9 more replies)
  0 siblings, 10 replies; 31+ messages in thread
From: Chin-Ting Kuo @ 2021-09-22 10:31 UTC (permalink / raw)
  To: robh+dt, joel, mturquette, sboyd, adrian.hunter, linux-aspeed,
	openbmc, linux-mmc, devicetree, linux-arm-kernel, linux-kernel,
	linux-clk, andrew
  Cc: BMC-SW, steven_lee

This patch series aims to configure SD and eMMC controllers' clock
frequency and clock phase parameters. The main modification is the
clock phase calculation method which has been checked with the HW IP
designer. Also, the clock source detection method is updated for
AST2600-A2/A3. This patch series has been verified on AST2600-A3 EVB.

Chin-Ting Kuo (10):
  clk: aspeed: ast2600: Porting sdhci clock source
  sdhci: aspeed: Add SDR50 support
  dts: aspeed: ast2600: Support SDR50 for SD device
  mmc: Add invert flag for clock phase signedness
  mmc: aspeed: Adjust delay taps calculation method
  arm: dts: aspeed: Change eMMC device compatible
  arm: dts: aspeed: Adjust clock phase parameter
  arm: dts: ibm: Adjust clock phase parameter
  dt-bindings: mmc: aspeed: Add max-tap-delay property
  dt-bindings: mmc: aspeed: Add a new compatible string

 .../devicetree/bindings/mmc/aspeed,sdhci.yaml |   4 +
 arch/arm/boot/dts/aspeed-ast2600-evb-a1.dts   |   8 ++
 arch/arm/boot/dts/aspeed-ast2600-evb.dts      |  11 +-
 arch/arm/boot/dts/aspeed-bmc-ibm-everest.dts  |   3 +-
 arch/arm/boot/dts/aspeed-bmc-ibm-rainier.dts  |   3 +-
 arch/arm/boot/dts/aspeed-bmc-opp-tacoma.dts   |   3 +-
 arch/arm/boot/dts/aspeed-g6.dtsi              |   2 +-
 drivers/clk/clk-ast2600.c                     |  69 ++++++++--
 drivers/mmc/core/host.c                       |  10 +-
 drivers/mmc/host/sdhci-of-aspeed.c            | 123 ++++++++++++++----
 include/linux/mmc/host.h                      |   2 +
 11 files changed, 193 insertions(+), 45 deletions(-)

-- 
2.17.1


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

* [PATCH 01/10] clk: aspeed: ast2600: Porting sdhci clock source
  2021-09-22 10:31 [PATCH 00/10] ASPEED SD/eMMC controller clock configuration Chin-Ting Kuo
@ 2021-09-22 10:31 ` Chin-Ting Kuo
  2021-09-23  0:02   ` Joel Stanley
  2021-10-26  6:10   ` Paul Menzel
  2021-09-22 10:31 ` [PATCH 02/10] sdhci: aspeed: Add SDR50 support Chin-Ting Kuo
                   ` (8 subsequent siblings)
  9 siblings, 2 replies; 31+ messages in thread
From: Chin-Ting Kuo @ 2021-09-22 10:31 UTC (permalink / raw)
  To: robh+dt, joel, mturquette, sboyd, adrian.hunter, linux-aspeed,
	openbmc, linux-mmc, devicetree, linux-arm-kernel, linux-kernel,
	linux-clk, andrew
  Cc: BMC-SW, steven_lee

- There are two clock sources used to generate
  SD/SDIO clock, APLL clock and HCLK (200MHz).
  User can select which clock source should be used
  by configuring SCU310[8].
- The SD/SDIO clock divider selection table SCU310[30:28]
  is different between AST2600-A1 and AST2600-A2/A3.
  For AST2600-A1, 200MHz SD/SDIO clock cannot be
  gotten by the dividers in SCU310[30:28] if APLL
  is not the multiple of 200MHz and HCLK is 200MHz.
  For AST2600-A2/A3, a new divider, "1", is added and
  200MHz SD/SDIO clock can be obtained by adopting HCLK
  as clock source and setting SCU310[30:28] to 3b'111.

Signed-off-by: Chin-Ting Kuo <chin-ting_kuo@aspeedtech.com>
---
 drivers/clk/clk-ast2600.c | 69 ++++++++++++++++++++++++++++++++++-----
 1 file changed, 61 insertions(+), 8 deletions(-)

diff --git a/drivers/clk/clk-ast2600.c b/drivers/clk/clk-ast2600.c
index bc3be5f3eae1..a6778c18274a 100644
--- a/drivers/clk/clk-ast2600.c
+++ b/drivers/clk/clk-ast2600.c
@@ -168,6 +168,30 @@ static const struct clk_div_table ast2600_div_table[] = {
 	{ 0 }
 };
 
+static const struct clk_div_table ast2600_sd_div_a1_table[] = {
+	{ 0x0, 2 },
+	{ 0x1, 4 },
+	{ 0x2, 6 },
+	{ 0x3, 8 },
+	{ 0x4, 10 },
+	{ 0x5, 12 },
+	{ 0x6, 14 },
+	{ 0x7, 16 },
+	{ 0 }
+};
+
+static const struct clk_div_table ast2600_sd_div_a2_table[] = {
+	{ 0x0, 2 },
+	{ 0x1, 4 },
+	{ 0x2, 6 },
+	{ 0x3, 8 },
+	{ 0x4, 10 },
+	{ 0x5, 12 },
+	{ 0x6, 14 },
+	{ 0x7, 1 },
+	{ 0 }
+};
+
 /* For hpll/dpll/epll/mpll */
 static struct clk_hw *ast2600_calc_pll(const char *name, u32 val)
 {
@@ -424,6 +448,11 @@ static const char *const emmc_extclk_parent_names[] = {
 	"mpll",
 };
 
+static const char *const sd_extclk_parent_names[] = {
+	"hclk",
+	"apll",
+};
+
 static const char * const vclk_parent_names[] = {
 	"dpll",
 	"d1pll",
@@ -523,18 +552,42 @@ static int aspeed_g6_clk_probe(struct platform_device *pdev)
 		return PTR_ERR(hw);
 	aspeed_g6_clk_data->hws[ASPEED_CLK_EMMC] = hw;
 
-	/* SD/SDIO clock divider and gate */
-	hw = clk_hw_register_gate(dev, "sd_extclk_gate", "hpll", 0,
-			scu_g6_base + ASPEED_G6_CLK_SELECTION4, 31, 0,
-			&aspeed_g6_clk_lock);
+	clk_hw_register_fixed_rate(NULL, "hclk", NULL, 0, 200000000);
+
+	regmap_read(map, 0x310, &val);
+	hw = clk_hw_register_mux(dev, "sd_extclk_mux",
+				 sd_extclk_parent_names,
+				 ARRAY_SIZE(sd_extclk_parent_names), 0,
+				 scu_g6_base + ASPEED_G6_CLK_SELECTION4, 8, 1,
+				 0, &aspeed_g6_clk_lock);
 	if (IS_ERR(hw))
 		return PTR_ERR(hw);
-	hw = clk_hw_register_divider_table(dev, "sd_extclk", "sd_extclk_gate",
-			0, scu_g6_base + ASPEED_G6_CLK_SELECTION4, 28, 3, 0,
-			ast2600_div_table,
-			&aspeed_g6_clk_lock);
+
+	hw = clk_hw_register_gate(dev, "sd_extclk_gate", "sd_extclk_mux",
+				  0, scu_g6_base + ASPEED_G6_CLK_SELECTION4,
+				  31, 0, &aspeed_g6_clk_lock);
 	if (IS_ERR(hw))
 		return PTR_ERR(hw);
+
+	regmap_read(map, 0x14, &val);
+	/* AST2600-A2/A3 clock divisor is different from AST2600-A1 */
+	if (((val & GENMASK(23, 16)) >> 16) >= 2) {
+		/* AST2600-A2/A3 */
+		hw = clk_hw_register_divider_table(dev, "sd_extclk", "sd_extclk_gate",
+					0, scu_g6_base + ASPEED_G6_CLK_SELECTION4, 28, 3, 0,
+					ast2600_sd_div_a2_table,
+					&aspeed_g6_clk_lock);
+		if (IS_ERR(hw))
+			return PTR_ERR(hw);
+	} else {
+		/* AST2600-A1 */
+		hw = clk_hw_register_divider_table(dev, "sd_extclk", "sd_extclk_gate",
+					0, scu_g6_base + ASPEED_G6_CLK_SELECTION4, 28, 3, 0,
+					ast2600_sd_div_a1_table,
+					&aspeed_g6_clk_lock);
+		if (IS_ERR(hw))
+			return PTR_ERR(hw);
+	}
 	aspeed_g6_clk_data->hws[ASPEED_CLK_SDIO] = hw;
 
 	/* MAC1/2 RMII 50MHz RCLK */
-- 
2.17.1


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

* [PATCH 02/10] sdhci: aspeed: Add SDR50 support
  2021-09-22 10:31 [PATCH 00/10] ASPEED SD/eMMC controller clock configuration Chin-Ting Kuo
  2021-09-22 10:31 ` [PATCH 01/10] clk: aspeed: ast2600: Porting sdhci clock source Chin-Ting Kuo
@ 2021-09-22 10:31 ` Chin-Ting Kuo
  2021-10-26  0:31   ` Andrew Jeffery
  2021-09-22 10:31 ` [PATCH 03/10] dts: aspeed: ast2600: Support SDR50 for SD device Chin-Ting Kuo
                   ` (7 subsequent siblings)
  9 siblings, 1 reply; 31+ messages in thread
From: Chin-Ting Kuo @ 2021-09-22 10:31 UTC (permalink / raw)
  To: robh+dt, joel, mturquette, sboyd, adrian.hunter, linux-aspeed,
	openbmc, linux-mmc, devicetree, linux-arm-kernel, linux-kernel,
	linux-clk, andrew
  Cc: BMC-SW, steven_lee

From the analog waveform analysis result, SD/SDIO controller
of AST2600 cannot always work well with 200MHz. The upper bound
stable frequency for SD/SDIO controller is 100MHz. Thus, SDR50
supported bit, instead of SDR104, in capability 2 register
should be set in advance.

Signed-off-by: Chin-Ting Kuo <chin-ting_kuo@aspeedtech.com>
---
 drivers/mmc/host/sdhci-of-aspeed.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/drivers/mmc/host/sdhci-of-aspeed.c b/drivers/mmc/host/sdhci-of-aspeed.c
index 6e4e132903a6..c6eaeb02e3f9 100644
--- a/drivers/mmc/host/sdhci-of-aspeed.c
+++ b/drivers/mmc/host/sdhci-of-aspeed.c
@@ -35,6 +35,8 @@
 #define ASPEED_SDC_CAP1_1_8V           (0 * 32 + 26)
 /* SDIO{14,24} */
 #define ASPEED_SDC_CAP2_SDR104         (1 * 32 + 1)
+/* SDIO{14,24} */
+#define ASPEED_SDC_CAP2_SDR50          (1 * 32 + 0)
 
 struct aspeed_sdc {
 	struct clk *clk;
@@ -410,11 +412,17 @@ static int aspeed_sdhci_probe(struct platform_device *pdev)
 	sdhci_get_of_property(pdev);
 
 	if (of_property_read_bool(np, "mmc-hs200-1_8v") ||
+		of_property_read_bool(np, "sd-uhs-sdr50") ||
 	    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-sdr50")) {
+		aspeed_sdc_set_slot_capability(host, dev->parent, ASPEED_SDC_CAP2_SDR50,
+					       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);
-- 
2.17.1


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

* [PATCH 03/10] dts: aspeed: ast2600: Support SDR50 for SD device
  2021-09-22 10:31 [PATCH 00/10] ASPEED SD/eMMC controller clock configuration Chin-Ting Kuo
  2021-09-22 10:31 ` [PATCH 01/10] clk: aspeed: ast2600: Porting sdhci clock source Chin-Ting Kuo
  2021-09-22 10:31 ` [PATCH 02/10] sdhci: aspeed: Add SDR50 support Chin-Ting Kuo
@ 2021-09-22 10:31 ` Chin-Ting Kuo
  2021-10-26  0:42   ` Andrew Jeffery
  2021-09-22 10:31 ` [PATCH 04/10] mmc: Add invert flag for clock phase signedness Chin-Ting Kuo
                   ` (6 subsequent siblings)
  9 siblings, 1 reply; 31+ messages in thread
From: Chin-Ting Kuo @ 2021-09-22 10:31 UTC (permalink / raw)
  To: robh+dt, joel, mturquette, sboyd, adrian.hunter, linux-aspeed,
	openbmc, linux-mmc, devicetree, linux-arm-kernel, linux-kernel,
	linux-clk, andrew
  Cc: BMC-SW, steven_lee

The maximum frequency for SD controller on AST2600 EVB is
100MHz. In order to achieve 100MHz, sd-uhs-sdr50 property
should be added and the driver will set the SDR50 supported
bit in capability 2 register during probing stage.

Signed-off-by: Chin-Ting Kuo <chin-ting_kuo@aspeedtech.com>
---
 arch/arm/boot/dts/aspeed-ast2600-evb.dts | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/arch/arm/boot/dts/aspeed-ast2600-evb.dts b/arch/arm/boot/dts/aspeed-ast2600-evb.dts
index b7eb552640cb..4551dba499c2 100644
--- a/arch/arm/boot/dts/aspeed-ast2600-evb.dts
+++ b/arch/arm/boot/dts/aspeed-ast2600-evb.dts
@@ -280,6 +280,7 @@
 &sdhci0 {
 	status = "okay";
 	bus-width = <4>;
+	sd-uhs-sdr50;
 	max-frequency = <100000000>;
 	sdhci-drive-type = /bits/ 8 <3>;
 	sdhci-caps-mask = <0x7 0x0>;
@@ -292,6 +293,7 @@
 &sdhci1 {
 	status = "okay";
 	bus-width = <4>;
+	sd-uhs-sdr50;
 	max-frequency = <100000000>;
 	sdhci-drive-type = /bits/ 8 <3>;
 	sdhci-caps-mask = <0x7 0x0>;
-- 
2.17.1


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

* [PATCH 04/10] mmc: Add invert flag for clock phase signedness
  2021-09-22 10:31 [PATCH 00/10] ASPEED SD/eMMC controller clock configuration Chin-Ting Kuo
                   ` (2 preceding siblings ...)
  2021-09-22 10:31 ` [PATCH 03/10] dts: aspeed: ast2600: Support SDR50 for SD device Chin-Ting Kuo
@ 2021-09-22 10:31 ` Chin-Ting Kuo
  2021-10-26  0:52   ` Andrew Jeffery
  2021-09-22 10:31 ` [PATCH 05/10] mmc: aspeed: Adjust delay taps calculation method Chin-Ting Kuo
                   ` (5 subsequent siblings)
  9 siblings, 1 reply; 31+ messages in thread
From: Chin-Ting Kuo @ 2021-09-22 10:31 UTC (permalink / raw)
  To: robh+dt, joel, mturquette, sboyd, adrian.hunter, linux-aspeed,
	openbmc, linux-mmc, devicetree, linux-arm-kernel, linux-kernel,
	linux-clk, andrew
  Cc: BMC-SW, steven_lee

The clock phase degree may be between -360 to 360.
If the data signal is leading to the clock, the signedness
of clock phase is postive, otherwise, the signedness
is negative.

Signed-off-by: Chin-Ting Kuo <chin-ting_kuo@aspeedtech.com>
---
 drivers/mmc/core/host.c  | 10 ++++++----
 include/linux/mmc/host.h |  2 ++
 2 files changed, 8 insertions(+), 4 deletions(-)

diff --git a/drivers/mmc/core/host.c b/drivers/mmc/core/host.c
index d4683b1d263f..c2de7cbc7838 100644
--- a/drivers/mmc/core/host.c
+++ b/drivers/mmc/core/host.c
@@ -214,14 +214,16 @@ static void mmc_retune_timer(struct timer_list *t)
 static void mmc_of_parse_timing_phase(struct device *dev, const char *prop,
 				      struct mmc_clk_phase *phase)
 {
-	int degrees[2] = {0};
+	int degrees[4] = {0};
 	int rc;
 
-	rc = device_property_read_u32_array(dev, prop, degrees, 2);
+	rc = device_property_read_u32_array(dev, prop, degrees, 4);
 	phase->valid = !rc;
 	if (phase->valid) {
-		phase->in_deg = degrees[0];
-		phase->out_deg = degrees[1];
+		phase->inv_in_deg = degrees[0] ? true : false;
+		phase->in_deg = degrees[1];
+		phase->inv_out_deg = degrees[2] ? true : false;
+		phase->out_deg = degrees[3];
 	}
 }
 
diff --git a/include/linux/mmc/host.h b/include/linux/mmc/host.h
index 0c0c9a0fdf57..3c13010683e0 100644
--- a/include/linux/mmc/host.h
+++ b/include/linux/mmc/host.h
@@ -82,7 +82,9 @@ struct mmc_ios {
 
 struct mmc_clk_phase {
 	bool valid;
+	bool inv_in_deg;
 	u16 in_deg;
+	bool inv_out_deg;
 	u16 out_deg;
 };
 
-- 
2.17.1


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

* [PATCH 05/10] mmc: aspeed: Adjust delay taps calculation method
  2021-09-22 10:31 [PATCH 00/10] ASPEED SD/eMMC controller clock configuration Chin-Ting Kuo
                   ` (3 preceding siblings ...)
  2021-09-22 10:31 ` [PATCH 04/10] mmc: Add invert flag for clock phase signedness Chin-Ting Kuo
@ 2021-09-22 10:31 ` Chin-Ting Kuo
  2021-10-26  3:10   ` Andrew Jeffery
  2021-09-22 10:31 ` [PATCH 06/10] arm: dts: aspeed: Change eMMC device compatible Chin-Ting Kuo
                   ` (4 subsequent siblings)
  9 siblings, 1 reply; 31+ messages in thread
From: Chin-Ting Kuo @ 2021-09-22 10:31 UTC (permalink / raw)
  To: robh+dt, joel, mturquette, sboyd, adrian.hunter, linux-aspeed,
	openbmc, linux-mmc, devicetree, linux-arm-kernel, linux-kernel,
	linux-clk, andrew
  Cc: BMC-SW, steven_lee

- The maximum tap delay may be slightly different on
  different platforms. It may also be different due to
  different SoC processes or different manufacturers.
  Thus, the maximum tap delay should be gotten from the
  device tree through max-tap-delay property.
- The delay time for each tap is an absolute value which
  is independent of clock frequency. But, in order to combine
  this principle with "phase" concept, clock frequency is took
  into consideration during calculating delay taps.
- The delay cell of eMMC device is non-uniform.
  The time period of the first tap is two times of others.
- The clock phase degree range is from -360 to 360.
  But, if the clock phase signedness is negative, clock signal
  is output from the falling edge first by default and thus, clock
  signal is leading to data signal by 90 degrees at least.

Signed-off-by: Chin-Ting Kuo <chin-ting_kuo@aspeedtech.com>
---
 drivers/mmc/host/sdhci-of-aspeed.c | 115 ++++++++++++++++++++++-------
 1 file changed, 89 insertions(+), 26 deletions(-)

diff --git a/drivers/mmc/host/sdhci-of-aspeed.c b/drivers/mmc/host/sdhci-of-aspeed.c
index c6eaeb02e3f9..739c9503a5ed 100644
--- a/drivers/mmc/host/sdhci-of-aspeed.c
+++ b/drivers/mmc/host/sdhci-of-aspeed.c
@@ -44,6 +44,7 @@ struct aspeed_sdc {
 
 	spinlock_t lock;
 	void __iomem *regs;
+	u32 max_tap_delay_ps;
 };
 
 struct aspeed_sdhci_tap_param {
@@ -63,6 +64,7 @@ struct aspeed_sdhci_tap_desc {
 struct aspeed_sdhci_phase_desc {
 	struct aspeed_sdhci_tap_desc in;
 	struct aspeed_sdhci_tap_desc out;
+	u32 nr_taps;
 };
 
 struct aspeed_sdhci_pdata {
@@ -158,43 +160,60 @@ aspeed_sdc_set_phase_taps(struct aspeed_sdc *sdc,
 }
 
 #define PICOSECONDS_PER_SECOND		1000000000000ULL
-#define ASPEED_SDHCI_NR_TAPS		15
-/* Measured value with *handwave* environmentals and static loading */
-#define ASPEED_SDHCI_MAX_TAP_DELAY_PS	1253
+#define ASPEED_SDHCI_MAX_TAPS		15
+
 static int aspeed_sdhci_phase_to_tap(struct device *dev, unsigned long rate_hz,
-				     int phase_deg)
+				     bool invert, int phase_deg, u32 nr_taps)
 {
 	u64 phase_period_ps;
 	u64 prop_delay_ps;
 	u64 clk_period_ps;
-	unsigned int tap;
-	u8 inverted;
+	u32 tap = 0;
+	struct aspeed_sdc *sdc = dev_get_drvdata(dev->parent);
 
-	phase_deg %= 360;
+	if (sdc->max_tap_delay_ps == 0)
+		return 0;
 
-	if (phase_deg >= 180) {
-		inverted = ASPEED_SDHCI_TAP_PARAM_INVERT_CLK;
-		phase_deg -= 180;
-		dev_dbg(dev,
-			"Inverting clock to reduce phase correction from %d to %d degrees\n",
-			phase_deg + 180, phase_deg);
-	} else {
-		inverted = 0;
+	prop_delay_ps = sdc->max_tap_delay_ps / nr_taps;
+	clk_period_ps = div_u64(PICOSECONDS_PER_SECOND, (u64)rate_hz);
+
+	/*
+	 * For ast2600, if clock phase degree is negative, clock signal is
+	 * output from falling edge first by default. Namely, clock signal
+	 * is leading to data signal by 90 degrees at least.
+	 */
+	if (invert) {
+		if (phase_deg >= 90)
+			phase_deg -= 90;
+		else
+			phase_deg = 0;
 	}
 
-	prop_delay_ps = ASPEED_SDHCI_MAX_TAP_DELAY_PS / ASPEED_SDHCI_NR_TAPS;
-	clk_period_ps = div_u64(PICOSECONDS_PER_SECOND, (u64)rate_hz);
 	phase_period_ps = div_u64((u64)phase_deg * clk_period_ps, 360ULL);
 
-	tap = div_u64(phase_period_ps, prop_delay_ps);
-	if (tap > ASPEED_SDHCI_NR_TAPS) {
+	/*
+	 * The delay cell is non-uniform for eMMC controller.
+	 * The time period of the first tap is two times of others.
+	 */
+	if (nr_taps == 16 && phase_period_ps > prop_delay_ps * 2) {
+		phase_period_ps -= prop_delay_ps * 2;
+		tap++;
+	}
+
+	tap += div_u64(phase_period_ps, prop_delay_ps);
+	if (tap > ASPEED_SDHCI_MAX_TAPS) {
 		dev_dbg(dev,
 			 "Requested out of range phase tap %d for %d degrees of phase compensation at %luHz, clamping to tap %d\n",
-			 tap, phase_deg, rate_hz, ASPEED_SDHCI_NR_TAPS);
-		tap = ASPEED_SDHCI_NR_TAPS;
+			 tap, phase_deg, rate_hz, ASPEED_SDHCI_MAX_TAPS);
+		tap = ASPEED_SDHCI_MAX_TAPS;
 	}
 
-	return inverted | tap;
+	if (invert) {
+		dev_info(dev, "invert the clock\n");
+		tap |= ASPEED_SDHCI_TAP_PARAM_INVERT_CLK;
+	}
+
+	return tap;
 }
 
 static void
@@ -202,13 +221,19 @@ aspeed_sdhci_phases_to_taps(struct device *dev, unsigned long rate,
 			    const struct mmc_clk_phase *phases,
 			    struct aspeed_sdhci_tap_param *taps)
 {
+	struct sdhci_host *host = dev->driver_data;
+	struct aspeed_sdhci *sdhci;
+
+	sdhci = sdhci_pltfm_priv(sdhci_priv(host));
 	taps->valid = phases->valid;
 
 	if (!phases->valid)
 		return;
 
-	taps->in = aspeed_sdhci_phase_to_tap(dev, rate, phases->in_deg);
-	taps->out = aspeed_sdhci_phase_to_tap(dev, rate, phases->out_deg);
+	taps->in = aspeed_sdhci_phase_to_tap(dev, rate, phases->inv_in_deg,
+				phases->in_deg, sdhci->phase_desc->nr_taps);
+	taps->out = aspeed_sdhci_phase_to_tap(dev, rate, phases->inv_out_deg,
+				phases->out_deg, sdhci->phase_desc->nr_taps);
 }
 
 static void
@@ -230,8 +255,8 @@ aspeed_sdhci_configure_phase(struct sdhci_host *host, unsigned long rate)
 	aspeed_sdc_set_phase_taps(sdhci->parent, sdhci->phase_desc, taps);
 	dev_dbg(dev,
 		"Using taps [%d, %d] for [%d, %d] degrees of phase correction at %luHz (%d)\n",
-		taps->in & ASPEED_SDHCI_NR_TAPS,
-		taps->out & ASPEED_SDHCI_NR_TAPS,
+		taps->in & ASPEED_SDHCI_MAX_TAPS,
+		taps->out & ASPEED_SDHCI_MAX_TAPS,
 		params->in_deg, params->out_deg, rate, host->timing);
 }
 
@@ -493,6 +518,7 @@ static const struct aspeed_sdhci_phase_desc ast2600_sdhci_phase[] = {
 			.enable_mask = ASPEED_SDC_S0_PHASE_OUT_EN,
 			.enable_value = 3,
 		},
+		.nr_taps = 15,
 	},
 	/* SDHCI/Slot 1 */
 	[1] = {
@@ -506,6 +532,31 @@ static const struct aspeed_sdhci_phase_desc ast2600_sdhci_phase[] = {
 			.enable_mask = ASPEED_SDC_S1_PHASE_OUT_EN,
 			.enable_value = 3,
 		},
+		.nr_taps = 15,
+	},
+};
+
+static const struct aspeed_sdhci_phase_desc ast2600_emmc_phase[] = {
+	/* eMMC slot 0 */
+	[0] = {
+		.in = {
+			.tap_mask = ASPEED_SDC_S0_PHASE_IN,
+			.enable_mask = ASPEED_SDC_S0_PHASE_IN_EN,
+			.enable_value = 1,
+		},
+		.out = {
+			.tap_mask = ASPEED_SDC_S0_PHASE_OUT,
+			.enable_mask = ASPEED_SDC_S0_PHASE_OUT_EN,
+			.enable_value = 3,
+		},
+
+		/*
+		 * There are 15 taps recorded in AST2600 datasheet.
+		 * But, actually, the time period of the first tap
+		 * is two times of others. Thus, 16 tap is used to
+		 * emulate this situation.
+		 */
+		.nr_taps = 16,
 	},
 };
 
@@ -515,10 +566,17 @@ static const struct aspeed_sdhci_pdata ast2600_sdhci_pdata = {
 	.nr_phase_descs = ARRAY_SIZE(ast2600_sdhci_phase),
 };
 
+static const struct aspeed_sdhci_pdata ast2600_emmc_pdata = {
+	.clk_div_start = 1,
+	.phase_desc = ast2600_emmc_phase,
+	.nr_phase_descs = ARRAY_SIZE(ast2600_emmc_phase),
+};
+
 static const struct of_device_id aspeed_sdhci_of_match[] = {
 	{ .compatible = "aspeed,ast2400-sdhci", .data = &ast2400_sdhci_pdata, },
 	{ .compatible = "aspeed,ast2500-sdhci", .data = &ast2400_sdhci_pdata, },
 	{ .compatible = "aspeed,ast2600-sdhci", .data = &ast2600_sdhci_pdata, },
+	{ .compatible = "aspeed,ast2600-emmc", .data = &ast2600_emmc_pdata, },
 	{ }
 };
 
@@ -562,6 +620,11 @@ static int aspeed_sdc_probe(struct platform_device *pdev)
 		goto err_clk;
 	}
 
+	ret = of_property_read_u32(pdev->dev.of_node, "max-tap-delay",
+			&sdc->max_tap_delay_ps);
+	if (ret)
+		sdc->max_tap_delay_ps = 0;
+
 	dev_set_drvdata(&pdev->dev, sdc);
 
 	parent = pdev->dev.of_node;
-- 
2.17.1


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

* [PATCH 06/10] arm: dts: aspeed: Change eMMC device compatible
  2021-09-22 10:31 [PATCH 00/10] ASPEED SD/eMMC controller clock configuration Chin-Ting Kuo
                   ` (4 preceding siblings ...)
  2021-09-22 10:31 ` [PATCH 05/10] mmc: aspeed: Adjust delay taps calculation method Chin-Ting Kuo
@ 2021-09-22 10:31 ` Chin-Ting Kuo
  2021-09-22 10:31 ` [PATCH 07/10] arm: dts: aspeed: Adjust clock phase parameter Chin-Ting Kuo
                   ` (3 subsequent siblings)
  9 siblings, 0 replies; 31+ messages in thread
From: Chin-Ting Kuo @ 2021-09-22 10:31 UTC (permalink / raw)
  To: robh+dt, joel, mturquette, sboyd, adrian.hunter, linux-aspeed,
	openbmc, linux-mmc, devicetree, linux-arm-kernel, linux-kernel,
	linux-clk, andrew
  Cc: BMC-SW, steven_lee

Since the eMMC device's delay parameters are different from
the SD's, a new compatible should be used to distinguish
between eMMC and SD device.

Signed-off-by: Chin-Ting Kuo <chin-ting_kuo@aspeedtech.com>
---
 arch/arm/boot/dts/aspeed-g6.dtsi | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/arm/boot/dts/aspeed-g6.dtsi b/arch/arm/boot/dts/aspeed-g6.dtsi
index 1b47be1704f8..d083de1d6567 100644
--- a/arch/arm/boot/dts/aspeed-g6.dtsi
+++ b/arch/arm/boot/dts/aspeed-g6.dtsi
@@ -597,7 +597,7 @@
 				status = "disabled";
 
 				emmc: sdhci@1e750100 {
-					compatible = "aspeed,ast2600-sdhci";
+					compatible = "aspeed,ast2600-emmc";
 					reg = <0x100 0x100>;
 					sdhci,auto-cmd12;
 					interrupts = <GIC_SPI 15 IRQ_TYPE_LEVEL_HIGH>;
-- 
2.17.1


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

* [PATCH 07/10] arm: dts: aspeed: Adjust clock phase parameter
  2021-09-22 10:31 [PATCH 00/10] ASPEED SD/eMMC controller clock configuration Chin-Ting Kuo
                   ` (5 preceding siblings ...)
  2021-09-22 10:31 ` [PATCH 06/10] arm: dts: aspeed: Change eMMC device compatible Chin-Ting Kuo
@ 2021-09-22 10:31 ` Chin-Ting Kuo
  2021-09-22 10:31 ` [PATCH 08/10] arm: dts: ibm: " Chin-Ting Kuo
                   ` (2 subsequent siblings)
  9 siblings, 0 replies; 31+ messages in thread
From: Chin-Ting Kuo @ 2021-09-22 10:31 UTC (permalink / raw)
  To: robh+dt, joel, mturquette, sboyd, adrian.hunter, linux-aspeed,
	openbmc, linux-mmc, devicetree, linux-arm-kernel, linux-kernel,
	linux-clk, andrew
  Cc: BMC-SW, steven_lee

Change clock phase degree for AST2600 EVB.
These parameter has been verified with 100MHz
clock frequency for eMMC and SD controllers.

Signed-off-by: Chin-Ting Kuo <chin-ting_kuo@aspeedtech.com>
---
 arch/arm/boot/dts/aspeed-ast2600-evb-a1.dts | 8 ++++++++
 arch/arm/boot/dts/aspeed-ast2600-evb.dts    | 9 ++++++---
 2 files changed, 14 insertions(+), 3 deletions(-)

diff --git a/arch/arm/boot/dts/aspeed-ast2600-evb-a1.dts b/arch/arm/boot/dts/aspeed-ast2600-evb-a1.dts
index dd7148060c4a..2d83617dc436 100644
--- a/arch/arm/boot/dts/aspeed-ast2600-evb-a1.dts
+++ b/arch/arm/boot/dts/aspeed-ast2600-evb-a1.dts
@@ -13,3 +13,11 @@
 };
 
 /delete-node/ &sdc;
+
+&emmc_controller {
+	max-tap-delay = <706>;
+};
+
+&emmc {
+	clk-phase-mmc-hs200 = <0 13>, <1 103>;
+};
diff --git a/arch/arm/boot/dts/aspeed-ast2600-evb.dts b/arch/arm/boot/dts/aspeed-ast2600-evb.dts
index 4551dba499c2..f728b9d9b4cf 100644
--- a/arch/arm/boot/dts/aspeed-ast2600-evb.dts
+++ b/arch/arm/boot/dts/aspeed-ast2600-evb.dts
@@ -143,13 +143,15 @@
 
 &emmc_controller {
 	status = "okay";
+	/* Measured value with *handwave* environmentals and static loading */
+	max-tap-delay = <736>;
 };
 
 &emmc {
 	non-removable;
 	bus-width = <4>;
 	max-frequency = <100000000>;
-	clk-phase-mmc-hs200 = <9>, <225>;
+	clk-phase-mmc-hs200 = <0 27>, <1 95>;
 };
 
 &rtc {
@@ -260,6 +262,7 @@
 
 &sdc {
 	status = "okay";
+	max-tap-delay = <9000>;
 };
 
 /*
@@ -287,7 +290,7 @@
 	sdhci,wp-inverted;
 	vmmc-supply = <&vcc_sdhci0>;
 	vqmmc-supply = <&vccq_sdhci0>;
-	clk-phase-sd-hs = <7>, <200>;
+	clk-phase-uhs-sdr50 = <0 130>, <0 238>;
 };
 
 &sdhci1 {
@@ -300,5 +303,5 @@
 	sdhci,wp-inverted;
 	vmmc-supply = <&vcc_sdhci1>;
 	vqmmc-supply = <&vccq_sdhci1>;
-	clk-phase-sd-hs = <7>, <200>;
+	clk-phase-uhs-sdr50 = <0 130>, <0 130>;
 };
-- 
2.17.1


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

* [PATCH 08/10] arm: dts: ibm: Adjust clock phase parameter
  2021-09-22 10:31 [PATCH 00/10] ASPEED SD/eMMC controller clock configuration Chin-Ting Kuo
                   ` (6 preceding siblings ...)
  2021-09-22 10:31 ` [PATCH 07/10] arm: dts: aspeed: Adjust clock phase parameter Chin-Ting Kuo
@ 2021-09-22 10:31 ` Chin-Ting Kuo
  2021-09-22 10:31 ` [PATCH 09/10] dt-bindings: mmc: aspeed: Add max-tap-delay property Chin-Ting Kuo
  2021-09-22 10:31 ` [PATCH 10/10] dt-bindings: mmc: aspeed: Add a new compatible string Chin-Ting Kuo
  9 siblings, 0 replies; 31+ messages in thread
From: Chin-Ting Kuo @ 2021-09-22 10:31 UTC (permalink / raw)
  To: robh+dt, joel, mturquette, sboyd, adrian.hunter, linux-aspeed,
	openbmc, linux-mmc, devicetree, linux-arm-kernel, linux-kernel,
	linux-clk, andrew
  Cc: BMC-SW, steven_lee

- Add max-tap-delay property for eMMC controller.
- Change clock phase degree for AST2600 on IBM platforms.

Signed-off-by: Chin-Ting Kuo <chin-ting_kuo@aspeedtech.com>
---
 arch/arm/boot/dts/aspeed-bmc-ibm-everest.dts | 3 ++-
 arch/arm/boot/dts/aspeed-bmc-ibm-rainier.dts | 3 ++-
 arch/arm/boot/dts/aspeed-bmc-opp-tacoma.dts  | 3 ++-
 3 files changed, 6 insertions(+), 3 deletions(-)

diff --git a/arch/arm/boot/dts/aspeed-bmc-ibm-everest.dts b/arch/arm/boot/dts/aspeed-bmc-ibm-everest.dts
index 2efd70666738..eccb4749755a 100644
--- a/arch/arm/boot/dts/aspeed-bmc-ibm-everest.dts
+++ b/arch/arm/boot/dts/aspeed-bmc-ibm-everest.dts
@@ -2824,6 +2824,7 @@
 
 &emmc_controller {
 	status = "okay";
+	max-tap-delay = <1253>;
 };
 
 &pinctrl_emmc_default {
@@ -2832,7 +2833,7 @@
 
 &emmc {
 	status = "okay";
-	clk-phase-mmc-hs200 = <210>, <228>;
+	clk-phase-mmc-hs200 = <1 124>, <1 141>;
 };
 
 &fsim0 {
diff --git a/arch/arm/boot/dts/aspeed-bmc-ibm-rainier.dts b/arch/arm/boot/dts/aspeed-bmc-ibm-rainier.dts
index 6419c9762c0b..2138a8a10d6e 100644
--- a/arch/arm/boot/dts/aspeed-bmc-ibm-rainier.dts
+++ b/arch/arm/boot/dts/aspeed-bmc-ibm-rainier.dts
@@ -299,6 +299,7 @@
 
 &emmc_controller {
 	status = "okay";
+	max-tap-delay = <1253>;
 };
 
 &pinctrl_emmc_default {
@@ -307,7 +308,7 @@
 
 &emmc {
 	status = "okay";
-	clk-phase-mmc-hs200 = <180>, <180>;
+	clk-phase-mmc-hs200 = <1 90>, <1 90>;
 };
 
 &fsim0 {
diff --git a/arch/arm/boot/dts/aspeed-bmc-opp-tacoma.dts b/arch/arm/boot/dts/aspeed-bmc-opp-tacoma.dts
index e39f310d55eb..7427809354cc 100644
--- a/arch/arm/boot/dts/aspeed-bmc-opp-tacoma.dts
+++ b/arch/arm/boot/dts/aspeed-bmc-opp-tacoma.dts
@@ -182,11 +182,12 @@
 
 &emmc_controller {
 	status = "okay";
+	max-tap-delay = <1253>;
 };
 
 &emmc {
 	status = "okay";
-	clk-phase-mmc-hs200 = <36>, <270>;
+	clk-phase-mmc-hs200 = <0 40>, <1 181>;
 };
 
 &fsim0 {
-- 
2.17.1


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

* [PATCH 09/10] dt-bindings: mmc: aspeed: Add max-tap-delay property
  2021-09-22 10:31 [PATCH 00/10] ASPEED SD/eMMC controller clock configuration Chin-Ting Kuo
                   ` (7 preceding siblings ...)
  2021-09-22 10:31 ` [PATCH 08/10] arm: dts: ibm: " Chin-Ting Kuo
@ 2021-09-22 10:31 ` Chin-Ting Kuo
  2021-09-27 18:40   ` Rob Herring
  2021-09-22 10:31 ` [PATCH 10/10] dt-bindings: mmc: aspeed: Add a new compatible string Chin-Ting Kuo
  9 siblings, 1 reply; 31+ messages in thread
From: Chin-Ting Kuo @ 2021-09-22 10:31 UTC (permalink / raw)
  To: robh+dt, joel, mturquette, sboyd, adrian.hunter, linux-aspeed,
	openbmc, linux-mmc, devicetree, linux-arm-kernel, linux-kernel,
	linux-clk, andrew
  Cc: BMC-SW, steven_lee

Add max-tap-delay proptery in order to record the maximum
tap delay on different platforms.

Signed-off-by: Chin-Ting Kuo <chin-ting_kuo@aspeedtech.com>
---
 Documentation/devicetree/bindings/mmc/aspeed,sdhci.yaml | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/Documentation/devicetree/bindings/mmc/aspeed,sdhci.yaml b/Documentation/devicetree/bindings/mmc/aspeed,sdhci.yaml
index 987b287f3bff..5bb66849df65 100644
--- a/Documentation/devicetree/bindings/mmc/aspeed,sdhci.yaml
+++ b/Documentation/devicetree/bindings/mmc/aspeed,sdhci.yaml
@@ -37,6 +37,9 @@ properties:
   clocks:
     maxItems: 1
     description: The SD/SDIO controller clock gate
+  max-tap-delay:
+    maxItems: 1
+    description: The maximum delay in picosecond for SD/SDIO controller
 
 patternProperties:
   "^sdhci@[0-9a-f]+$":
-- 
2.17.1


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

* [PATCH 10/10] dt-bindings: mmc: aspeed: Add a new compatible string
  2021-09-22 10:31 [PATCH 00/10] ASPEED SD/eMMC controller clock configuration Chin-Ting Kuo
                   ` (8 preceding siblings ...)
  2021-09-22 10:31 ` [PATCH 09/10] dt-bindings: mmc: aspeed: Add max-tap-delay property Chin-Ting Kuo
@ 2021-09-22 10:31 ` Chin-Ting Kuo
  2021-09-27 18:59   ` Rob Herring
  9 siblings, 1 reply; 31+ messages in thread
From: Chin-Ting Kuo @ 2021-09-22 10:31 UTC (permalink / raw)
  To: robh+dt, joel, mturquette, sboyd, adrian.hunter, linux-aspeed,
	openbmc, linux-mmc, devicetree, linux-arm-kernel, linux-kernel,
	linux-clk, andrew
  Cc: BMC-SW, steven_lee

Add "aspeed,ast2600-emmc" compatible string for the sake of
distinguishing between SD and eMMC device.

Signed-off-by: Chin-Ting Kuo <chin-ting_kuo@aspeedtech.com>
---
 Documentation/devicetree/bindings/mmc/aspeed,sdhci.yaml | 1 +
 1 file changed, 1 insertion(+)

diff --git a/Documentation/devicetree/bindings/mmc/aspeed,sdhci.yaml b/Documentation/devicetree/bindings/mmc/aspeed,sdhci.yaml
index 5bb66849df65..41105cd104c6 100644
--- a/Documentation/devicetree/bindings/mmc/aspeed,sdhci.yaml
+++ b/Documentation/devicetree/bindings/mmc/aspeed,sdhci.yaml
@@ -52,6 +52,7 @@ patternProperties:
           - aspeed,ast2400-sdhci
           - aspeed,ast2500-sdhci
           - aspeed,ast2600-sdhci
+          - aspeed,ast2600-emmc
       reg:
         maxItems: 1
         description: The SDHCI registers
-- 
2.17.1


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

* Re: [PATCH 01/10] clk: aspeed: ast2600: Porting sdhci clock source
  2021-09-22 10:31 ` [PATCH 01/10] clk: aspeed: ast2600: Porting sdhci clock source Chin-Ting Kuo
@ 2021-09-23  0:02   ` Joel Stanley
  2021-09-23  5:31     ` Chin-Ting Kuo
  2021-10-26  6:10   ` Paul Menzel
  1 sibling, 1 reply; 31+ messages in thread
From: Joel Stanley @ 2021-09-23  0:02 UTC (permalink / raw)
  To: Chin-Ting Kuo
  Cc: devicetree, linux-clk, linux-aspeed, BMC-SW, Stephen Boyd,
	Steven Lee, Michael Turquette, linux-mmc, Adrian Hunter,
	Linux Kernel Mailing List, Andrew Jeffery, Rob Herring,
	OpenBMC Maillist, Linux ARM

On Wed, 22 Sept 2021 at 10:31, Chin-Ting Kuo
<chin-ting_kuo@aspeedtech.com> wrote:
>
> - There are two clock sources used to generate
>   SD/SDIO clock, APLL clock and HCLK (200MHz).
>   User can select which clock source should be used
>   by configuring SCU310[8].
> - The SD/SDIO clock divider selection table SCU310[30:28]
>   is different between AST2600-A1 and AST2600-A2/A3.
>   For AST2600-A1, 200MHz SD/SDIO clock cannot be
>   gotten by the dividers in SCU310[30:28] if APLL
>   is not the multiple of 200MHz and HCLK is 200MHz.
>   For AST2600-A2/A3, a new divider, "1", is added and
>   200MHz SD/SDIO clock can be obtained by adopting HCLK
>   as clock source and setting SCU310[30:28] to 3b'111.
>
> Signed-off-by: Chin-Ting Kuo <chin-ting_kuo@aspeedtech.com>
> ---
>  drivers/clk/clk-ast2600.c | 69 ++++++++++++++++++++++++++++++++++-----
>  1 file changed, 61 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/clk/clk-ast2600.c b/drivers/clk/clk-ast2600.c
> index bc3be5f3eae1..a6778c18274a 100644
> --- a/drivers/clk/clk-ast2600.c
> +++ b/drivers/clk/clk-ast2600.c
> @@ -168,6 +168,30 @@ static const struct clk_div_table ast2600_div_table[] = {
>         { 0 }
>  };
>
> +static const struct clk_div_table ast2600_sd_div_a1_table[] = {

Let's put the revision next to the ast2600 like the other tables:

ast2600_a1_sd_div_table

> +       { 0x0, 2 },
> +       { 0x1, 4 },
> +       { 0x2, 6 },
> +       { 0x3, 8 },
> +       { 0x4, 10 },
> +       { 0x5, 12 },
> +       { 0x6, 14 },
> +       { 0x7, 16 },
> +       { 0 }
> +};
> +
> +static const struct clk_div_table ast2600_sd_div_a2_table[] = {

For naming; can I propose we omit the revision for the A2/A3+ case? So
this one would be called:

ast2600_sd_div_table

> +       { 0x0, 2 },
> +       { 0x1, 4 },
> +       { 0x2, 6 },
> +       { 0x3, 8 },
> +       { 0x4, 10 },
> +       { 0x5, 12 },
> +       { 0x6, 14 },
> +       { 0x7, 1 },
> +       { 0 }
> +};
> +
>  /* For hpll/dpll/epll/mpll */
>  static struct clk_hw *ast2600_calc_pll(const char *name, u32 val)
>  {
> @@ -424,6 +448,11 @@ static const char *const emmc_extclk_parent_names[] = {
>         "mpll",
>  };
>
> +static const char *const sd_extclk_parent_names[] = {
> +       "hclk",
> +       "apll",
> +};
> +
>  static const char * const vclk_parent_names[] = {
>         "dpll",
>         "d1pll",
> @@ -523,18 +552,42 @@ static int aspeed_g6_clk_probe(struct platform_device *pdev)
>                 return PTR_ERR(hw);
>         aspeed_g6_clk_data->hws[ASPEED_CLK_EMMC] = hw;
>
> -       /* SD/SDIO clock divider and gate */
> -       hw = clk_hw_register_gate(dev, "sd_extclk_gate", "hpll", 0,
> -                       scu_g6_base + ASPEED_G6_CLK_SELECTION4, 31, 0,
> -                       &aspeed_g6_clk_lock);
> +       clk_hw_register_fixed_rate(NULL, "hclk", NULL, 0, 200000000);
> +
> +       regmap_read(map, 0x310, &val);

Use the #defines for the register number.

> +       hw = clk_hw_register_mux(dev, "sd_extclk_mux",
> +                                sd_extclk_parent_names,
> +                                ARRAY_SIZE(sd_extclk_parent_names), 0,
> +                                scu_g6_base + ASPEED_G6_CLK_SELECTION4, 8, 1,
> +                                0, &aspeed_g6_clk_lock);
>         if (IS_ERR(hw))
>                 return PTR_ERR(hw);
> -       hw = clk_hw_register_divider_table(dev, "sd_extclk", "sd_extclk_gate",
> -                       0, scu_g6_base + ASPEED_G6_CLK_SELECTION4, 28, 3, 0,
> -                       ast2600_div_table,
> -                       &aspeed_g6_clk_lock);
> +
> +       hw = clk_hw_register_gate(dev, "sd_extclk_gate", "sd_extclk_mux",
> +                                 0, scu_g6_base + ASPEED_G6_CLK_SELECTION4,
> +                                 31, 0, &aspeed_g6_clk_lock);
>         if (IS_ERR(hw))
>                 return PTR_ERR(hw);
> +
> +       regmap_read(map, 0x14, &val);
> +       /* AST2600-A2/A3 clock divisor is different from AST2600-A1 */
> +       if (((val & GENMASK(23, 16)) >> 16) >= 2) {

I've got a little patch that I recommend you base your series on (feel
free to include it in your series when posting v2 to make it
self-contained):

https://lore.kernel.org/all/20210922235449.213631-1-joel@jms.id.au/

With this one you can do:

const struct clk_div_table* table;

 if (soc_rev >= 2)
   table = ast2600_sd_div_table;
else
   table = ast2600_a1_sd_div_table;

Then you don't need to duplicate the registration for each case:

               hw = clk_hw_register_divider_table(dev, "sd_extclk",
"sd_extclk_gate",
                                       0, scu_g6_base +
ASPEED_G6_CLK_SELECTION4, 28, 3, 0,
                                       table,
                                       &aspeed_g6_clk_lock);
               if (IS_ERR(hw))
                       return PTR_ERR(hw);

> +               /* AST2600-A2/A3 */
> +               hw = clk_hw_register_divider_table(dev, "sd_extclk", "sd_extclk_gate",
> +                                       0, scu_g6_base + ASPEED_G6_CLK_SELECTION4, 28, 3, 0,
> +                                       ast2600_sd_div_a2_table,
> +                                       &aspeed_g6_clk_lock);
> +               if (IS_ERR(hw))
> +                       return PTR_ERR(hw);
> +       } else {
> +               /* AST2600-A1 */
> +               hw = clk_hw_register_divider_table(dev, "sd_extclk", "sd_extclk_gate",
> +                                       0, scu_g6_base + ASPEED_G6_CLK_SELECTION4, 28, 3, 0,
> +                                       ast2600_sd_div_a1_table,
> +                                       &aspeed_g6_clk_lock);
> +               if (IS_ERR(hw))
> +                       return PTR_ERR(hw);
> +       }
>         aspeed_g6_clk_data->hws[ASPEED_CLK_SDIO] = hw;
>
>         /* MAC1/2 RMII 50MHz RCLK */
> --
> 2.17.1
>

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

* RE: [PATCH 01/10] clk: aspeed: ast2600: Porting sdhci clock source
  2021-09-23  0:02   ` Joel Stanley
@ 2021-09-23  5:31     ` Chin-Ting Kuo
  0 siblings, 0 replies; 31+ messages in thread
From: Chin-Ting Kuo @ 2021-09-23  5:31 UTC (permalink / raw)
  To: Joel Stanley
  Cc: devicetree, linux-clk, linux-aspeed, BMC-SW, Stephen Boyd,
	Steven Lee, Michael Turquette, linux-mmc, Adrian Hunter,
	Linux Kernel Mailing List, Andrew Jeffery, Rob Herring,
	OpenBMC Maillist, Linux ARM

Hi Joel,

Thanks for the review.

> -----Original Message-----
> From: Joel Stanley <joel@jms.id.au>
> Sent: Thursday, September 23, 2021 8:02 AM
> To: Chin-Ting Kuo <chin-ting_kuo@aspeedtech.com>
> Subject: Re: [PATCH 01/10] clk: aspeed: ast2600: Porting sdhci clock source
> 
> On Wed, 22 Sept 2021 at 10:31, Chin-Ting Kuo
> <chin-ting_kuo@aspeedtech.com> wrote:
> >
> > - There are two clock sources used to generate
> >   SD/SDIO clock, APLL clock and HCLK (200MHz).
> >   User can select which clock source should be used
> >   by configuring SCU310[8].
> > - The SD/SDIO clock divider selection table SCU310[30:28]
> >   is different between AST2600-A1 and AST2600-A2/A3.
> >   For AST2600-A1, 200MHz SD/SDIO clock cannot be
> >   gotten by the dividers in SCU310[30:28] if APLL
> >   is not the multiple of 200MHz and HCLK is 200MHz.
> >   For AST2600-A2/A3, a new divider, "1", is added and
> >   200MHz SD/SDIO clock can be obtained by adopting HCLK
> >   as clock source and setting SCU310[30:28] to 3b'111.
> >
> > Signed-off-by: Chin-Ting Kuo <chin-ting_kuo@aspeedtech.com>
> > ---
> >  drivers/clk/clk-ast2600.c | 69
> > ++++++++++++++++++++++++++++++++++-----
> >  1 file changed, 61 insertions(+), 8 deletions(-)
> >
> > diff --git a/drivers/clk/clk-ast2600.c b/drivers/clk/clk-ast2600.c
> > index bc3be5f3eae1..a6778c18274a 100644
> > --- a/drivers/clk/clk-ast2600.c
> > +++ b/drivers/clk/clk-ast2600.c
> > @@ -168,6 +168,30 @@ static const struct clk_div_table ast2600_div_table[]
> = {
> >         { 0 }
> >  };
> >
> > +static const struct clk_div_table ast2600_sd_div_a1_table[] = {
> 
> Let's put the revision next to the ast2600 like the other tables:
> 
> ast2600_a1_sd_div_table
> 
> > +       { 0x0, 2 },
> > +       { 0x1, 4 },
> > +       { 0x2, 6 },
> > +       { 0x3, 8 },
> > +       { 0x4, 10 },
> > +       { 0x5, 12 },
> > +       { 0x6, 14 },
> > +       { 0x7, 16 },
> > +       { 0 }
> > +};
> > +
> > +static const struct clk_div_table ast2600_sd_div_a2_table[] = {
> 

Okay, this will be updated in the next patch version.

> For naming; can I propose we omit the revision for the A2/A3+ case? So this
> one would be called:
> 
> ast2600_sd_div_table
> 

Okay, this will also be updated in the next patch version.

> > +       { 0x0, 2 },
> > +       { 0x1, 4 },
> > +       { 0x2, 6 },
> > +       { 0x3, 8 },
> > +       { 0x4, 10 },
> > +       { 0x5, 12 },
> > +       { 0x6, 14 },
> > +       { 0x7, 1 },
> > +       { 0 }
> > +};
> > +
> >  /* For hpll/dpll/epll/mpll */
> >  static struct clk_hw *ast2600_calc_pll(const char *name, u32 val)  {
> > @@ -424,6 +448,11 @@ static const char *const
> emmc_extclk_parent_names[] = {
> >         "mpll",
> >  };
> >
> > +static const char *const sd_extclk_parent_names[] = {
> > +       "hclk",
> > +       "apll",
> > +};
> > +
> >  static const char * const vclk_parent_names[] = {
> >         "dpll",
> >         "d1pll",
> > @@ -523,18 +552,42 @@ static int aspeed_g6_clk_probe(struct
> platform_device *pdev)
> >                 return PTR_ERR(hw);
> >         aspeed_g6_clk_data->hws[ASPEED_CLK_EMMC] = hw;
> >
> > -       /* SD/SDIO clock divider and gate */
> > -       hw = clk_hw_register_gate(dev, "sd_extclk_gate", "hpll", 0,
> > -                       scu_g6_base + ASPEED_G6_CLK_SELECTION4,
> 31, 0,
> > -                       &aspeed_g6_clk_lock);
> > +       clk_hw_register_fixed_rate(NULL, "hclk", NULL, 0, 200000000);
> > +
> > +       regmap_read(map, 0x310, &val);
> 
> Use the #defines for the register number.

Okay.

> 
> > +       hw = clk_hw_register_mux(dev, "sd_extclk_mux",
> > +                                sd_extclk_parent_names,
> > +
> ARRAY_SIZE(sd_extclk_parent_names), 0,
> > +                                scu_g6_base +
> ASPEED_G6_CLK_SELECTION4, 8, 1,
> > +                                0, &aspeed_g6_clk_lock);
> >         if (IS_ERR(hw))
> >                 return PTR_ERR(hw);
> > -       hw = clk_hw_register_divider_table(dev, "sd_extclk",
> "sd_extclk_gate",
> > -                       0, scu_g6_base + ASPEED_G6_CLK_SELECTION4,
> 28, 3, 0,
> > -                       ast2600_div_table,
> > -                       &aspeed_g6_clk_lock);
> > +
> > +       hw = clk_hw_register_gate(dev, "sd_extclk_gate", "sd_extclk_mux",
> > +                                 0, scu_g6_base +
> ASPEED_G6_CLK_SELECTION4,
> > +                                 31, 0, &aspeed_g6_clk_lock);
> >         if (IS_ERR(hw))
> >                 return PTR_ERR(hw);
> > +
> > +       regmap_read(map, 0x14, &val);
> > +       /* AST2600-A2/A3 clock divisor is different from AST2600-A1 */
> > +       if (((val & GENMASK(23, 16)) >> 16) >= 2) {
> 
> I've got a little patch that I recommend you base your series on (feel free to
> include it in your series when posting v2 to make it
> self-contained):
> 
> https://lore.kernel.org/all/20210922235449.213631-1-joel@jms.id.au/
> 
> With this one you can do:
> 
> const struct clk_div_table* table;
> 
>  if (soc_rev >= 2)
>    table = ast2600_sd_div_table;
> else
>    table = ast2600_a1_sd_div_table;
> 
> Then you don't need to duplicate the registration for each case:
> 
>                hw = clk_hw_register_divider_table(dev, "sd_extclk",
> "sd_extclk_gate",
>                                        0, scu_g6_base +
> ASPEED_G6_CLK_SELECTION4, 28, 3, 0,
>                                        table,
>                                        &aspeed_g6_clk_lock);
>                if (IS_ERR(hw))
>                        return PTR_ERR(hw);
> 

Okay, I will include your patch into this patch series when posting v2.

> > +               /* AST2600-A2/A3 */
> > +               hw = clk_hw_register_divider_table(dev, "sd_extclk",
> "sd_extclk_gate",
> > +                                       0, scu_g6_base +
> ASPEED_G6_CLK_SELECTION4, 28, 3, 0,
> > +                                       ast2600_sd_div_a2_table,
> > +                                       &aspeed_g6_clk_lock);
> > +               if (IS_ERR(hw))
> > +                       return PTR_ERR(hw);
> > +       } else {
> > +               /* AST2600-A1 */
> > +               hw = clk_hw_register_divider_table(dev, "sd_extclk",
> "sd_extclk_gate",
> > +                                       0, scu_g6_base +
> ASPEED_G6_CLK_SELECTION4, 28, 3, 0,
> > +                                       ast2600_sd_div_a1_table,
> > +                                       &aspeed_g6_clk_lock);
> > +               if (IS_ERR(hw))
> > +                       return PTR_ERR(hw);
> > +       }
> >         aspeed_g6_clk_data->hws[ASPEED_CLK_SDIO] = hw;
> >
> >         /* MAC1/2 RMII 50MHz RCLK */
> > --
> > 2.17.1
> >

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

* Re: [PATCH 09/10] dt-bindings: mmc: aspeed: Add max-tap-delay property
  2021-09-22 10:31 ` [PATCH 09/10] dt-bindings: mmc: aspeed: Add max-tap-delay property Chin-Ting Kuo
@ 2021-09-27 18:40   ` Rob Herring
  2021-09-28  2:50     ` Chin-Ting Kuo
  0 siblings, 1 reply; 31+ messages in thread
From: Rob Herring @ 2021-09-27 18:40 UTC (permalink / raw)
  To: Chin-Ting Kuo
  Cc: devicetree, linux-clk, linux-aspeed, BMC-SW, sboyd, steven_lee,
	openbmc, linux-mmc, adrian.hunter, linux-kernel, andrew,
	mturquette, linux-arm-kernel

On Wed, Sep 22, 2021 at 06:31:15PM +0800, Chin-Ting Kuo wrote:
> Add max-tap-delay proptery in order to record the maximum
> tap delay on different platforms.
> 
> Signed-off-by: Chin-Ting Kuo <chin-ting_kuo@aspeedtech.com>
> ---
>  Documentation/devicetree/bindings/mmc/aspeed,sdhci.yaml | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/mmc/aspeed,sdhci.yaml b/Documentation/devicetree/bindings/mmc/aspeed,sdhci.yaml
> index 987b287f3bff..5bb66849df65 100644
> --- a/Documentation/devicetree/bindings/mmc/aspeed,sdhci.yaml
> +++ b/Documentation/devicetree/bindings/mmc/aspeed,sdhci.yaml
> @@ -37,6 +37,9 @@ properties:
>    clocks:
>      maxItems: 1
>      description: The SD/SDIO controller clock gate
> +  max-tap-delay:
> +    maxItems: 1

An array?

> +    description: The maximum delay in picosecond for SD/SDIO controller

Properties with a unit should have a standard unit suffix.

Should be common property? If not, needs a vendor prefix.

>  
>  patternProperties:
>    "^sdhci@[0-9a-f]+$":
> -- 
> 2.17.1
> 
> 

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

* Re: [PATCH 10/10] dt-bindings: mmc: aspeed: Add a new compatible string
  2021-09-22 10:31 ` [PATCH 10/10] dt-bindings: mmc: aspeed: Add a new compatible string Chin-Ting Kuo
@ 2021-09-27 18:59   ` Rob Herring
  2021-09-28  2:50     ` Chin-Ting Kuo
  0 siblings, 1 reply; 31+ messages in thread
From: Rob Herring @ 2021-09-27 18:59 UTC (permalink / raw)
  To: Chin-Ting Kuo
  Cc: devicetree, linux-clk, linux-aspeed, BMC-SW, sboyd, steven_lee,
	openbmc, linux-mmc, adrian.hunter, linux-kernel, andrew,
	mturquette, linux-arm-kernel

On Wed, Sep 22, 2021 at 06:31:16PM +0800, Chin-Ting Kuo wrote:
> Add "aspeed,ast2600-emmc" compatible string for the sake of
> distinguishing between SD and eMMC device.

Why?

Is the h/w block different? We already have properties to handle some of 
the eMMC specifics. Also, you can have a child node for the eMMC device 
if you need that.

> 
> Signed-off-by: Chin-Ting Kuo <chin-ting_kuo@aspeedtech.com>
> ---
>  Documentation/devicetree/bindings/mmc/aspeed,sdhci.yaml | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/Documentation/devicetree/bindings/mmc/aspeed,sdhci.yaml b/Documentation/devicetree/bindings/mmc/aspeed,sdhci.yaml
> index 5bb66849df65..41105cd104c6 100644
> --- a/Documentation/devicetree/bindings/mmc/aspeed,sdhci.yaml
> +++ b/Documentation/devicetree/bindings/mmc/aspeed,sdhci.yaml
> @@ -52,6 +52,7 @@ patternProperties:
>            - aspeed,ast2400-sdhci
>            - aspeed,ast2500-sdhci
>            - aspeed,ast2600-sdhci
> +          - aspeed,ast2600-emmc
>        reg:
>          maxItems: 1
>          description: The SDHCI registers
> -- 
> 2.17.1
> 
> 

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

* RE: [PATCH 09/10] dt-bindings: mmc: aspeed: Add max-tap-delay property
  2021-09-27 18:40   ` Rob Herring
@ 2021-09-28  2:50     ` Chin-Ting Kuo
  0 siblings, 0 replies; 31+ messages in thread
From: Chin-Ting Kuo @ 2021-09-28  2:50 UTC (permalink / raw)
  To: Rob Herring
  Cc: devicetree, linux-clk, linux-aspeed, BMC-SW, sboyd, Steven Lee,
	openbmc, linux-mmc, adrian.hunter, linux-kernel, andrew,
	mturquette, linux-arm-kernel

Hi Rob,

Thanks for your review.

> -----Original Message-----
> From: Rob Herring <robh@kernel.org>
> Sent: Tuesday, September 28, 2021 2:41 AM
> To: Chin-Ting Kuo <chin-ting_kuo@aspeedtech.com>
> Subject: Re: [PATCH 09/10] dt-bindings: mmc: aspeed: Add max-tap-delay
> property
> 
> On Wed, Sep 22, 2021 at 06:31:15PM +0800, Chin-Ting Kuo wrote:
> > Add max-tap-delay proptery in order to record the maximum tap delay on
> > different platforms.
> >
> > Signed-off-by: Chin-Ting Kuo <chin-ting_kuo@aspeedtech.com>
> > ---
> >  Documentation/devicetree/bindings/mmc/aspeed,sdhci.yaml | 3 +++
> >  1 file changed, 3 insertions(+)
> >
> > diff --git a/Documentation/devicetree/bindings/mmc/aspeed,sdhci.yaml
> > b/Documentation/devicetree/bindings/mmc/aspeed,sdhci.yaml
> > index 987b287f3bff..5bb66849df65 100644
> > --- a/Documentation/devicetree/bindings/mmc/aspeed,sdhci.yaml
> > +++ b/Documentation/devicetree/bindings/mmc/aspeed,sdhci.yaml
> > @@ -37,6 +37,9 @@ properties:
> >    clocks:
> >      maxItems: 1
> >      description: The SD/SDIO controller clock gate
> > +  max-tap-delay:
> > +    maxItems: 1
> 
> An array?

No, it is an "int" value and this will be fixed in the next patch version.

> > +    description: The maximum delay in picosecond for SD/SDIO
> > + controller
> 
> Properties with a unit should have a standard unit suffix.
> 
> Should be common property? If not, needs a vendor prefix.

Okay, a vender prefix and an unit suffix will be add in the next patch version.

> 
> >
> >  patternProperties:
> >    "^sdhci@[0-9a-f]+$":
> > --
> > 2.17.1
> >
> >

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

* RE: [PATCH 10/10] dt-bindings: mmc: aspeed: Add a new compatible string
  2021-09-27 18:59   ` Rob Herring
@ 2021-09-28  2:50     ` Chin-Ting Kuo
  2021-09-28 22:28       ` Rob Herring
  0 siblings, 1 reply; 31+ messages in thread
From: Chin-Ting Kuo @ 2021-09-28  2:50 UTC (permalink / raw)
  To: Rob Herring
  Cc: devicetree, linux-clk, linux-aspeed, BMC-SW, sboyd, Steven Lee,
	openbmc, linux-mmc, adrian.hunter, linux-kernel, andrew,
	mturquette, linux-arm-kernel

Hi Rob,

> -----Original Message-----
> From: Rob Herring <robh@kernel.org>
> Sent: Tuesday, September 28, 2021 2:59 AM
> To: Chin-Ting Kuo <chin-ting_kuo@aspeedtech.com>
> Subject: Re: [PATCH 10/10] dt-bindings: mmc: aspeed: Add a new compatible
> string
> 
> On Wed, Sep 22, 2021 at 06:31:16PM +0800, Chin-Ting Kuo wrote:
> > Add "aspeed,ast2600-emmc" compatible string for the sake of
> > distinguishing between SD and eMMC device.
> 
> Why?
> 
> Is the h/w block different? We already have properties to handle some of the
> eMMC specifics. Also, you can have a child node for the eMMC device if you
> need that.

There are two SD/SDIO controllers in a AST2600 SoC.
One is for SD card and the other is for eMMC.
Although both of them are embedded in the same SoC, the design of delay cell and
the manufacture process are different. The delay phase is definitely different and, thus,
we need a flag, compatible, to distinguish the device, SD or eMMC.

Without "aspeed,ast2600-emmc" compatible, of course, eMMC device can work with original
sdhci driver and device tree setting. But, for ultra-speed or HS200 case, AST2600 SoC needs some
phase delay which (maximum) value is different between SD and eMMC device.

> 
> >
> > Signed-off-by: Chin-Ting Kuo <chin-ting_kuo@aspeedtech.com>
> > ---
> >  Documentation/devicetree/bindings/mmc/aspeed,sdhci.yaml | 1 +
> >  1 file changed, 1 insertion(+)
> >
> > diff --git a/Documentation/devicetree/bindings/mmc/aspeed,sdhci.yaml
> > b/Documentation/devicetree/bindings/mmc/aspeed,sdhci.yaml
> > index 5bb66849df65..41105cd104c6 100644
> > --- a/Documentation/devicetree/bindings/mmc/aspeed,sdhci.yaml
> > +++ b/Documentation/devicetree/bindings/mmc/aspeed,sdhci.yaml
> > @@ -52,6 +52,7 @@ patternProperties:
> >            - aspeed,ast2400-sdhci
> >            - aspeed,ast2500-sdhci
> >            - aspeed,ast2600-sdhci
> > +          - aspeed,ast2600-emmc
> >        reg:
> >          maxItems: 1
> >          description: The SDHCI registers
> > --
> > 2.17.1
> >
> >

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

* Re: [PATCH 10/10] dt-bindings: mmc: aspeed: Add a new compatible string
  2021-09-28  2:50     ` Chin-Ting Kuo
@ 2021-09-28 22:28       ` Rob Herring
  2021-09-29  3:03         ` Chin-Ting Kuo
  0 siblings, 1 reply; 31+ messages in thread
From: Rob Herring @ 2021-09-28 22:28 UTC (permalink / raw)
  To: Chin-Ting Kuo
  Cc: devicetree, linux-clk, linux-aspeed, BMC-SW, sboyd, Steven Lee,
	openbmc, linux-mmc, adrian.hunter, linux-kernel, andrew,
	mturquette, linux-arm-kernel

On Mon, Sep 27, 2021 at 9:51 PM Chin-Ting Kuo
<chin-ting_kuo@aspeedtech.com> wrote:
>
> Hi Rob,
>
> > -----Original Message-----
> > From: Rob Herring <robh@kernel.org>
> > Sent: Tuesday, September 28, 2021 2:59 AM
> > To: Chin-Ting Kuo <chin-ting_kuo@aspeedtech.com>
> > Subject: Re: [PATCH 10/10] dt-bindings: mmc: aspeed: Add a new compatible
> > string
> >
> > On Wed, Sep 22, 2021 at 06:31:16PM +0800, Chin-Ting Kuo wrote:
> > > Add "aspeed,ast2600-emmc" compatible string for the sake of
> > > distinguishing between SD and eMMC device.
> >
> > Why?
> >
> > Is the h/w block different? We already have properties to handle some of the
> > eMMC specifics. Also, you can have a child node for the eMMC device if you
> > need that.
>
> There are two SD/SDIO controllers in a AST2600 SoC.
> One is for SD card and the other is for eMMC.
> Although both of them are embedded in the same SoC, the design of delay cell and
> the manufacture process are different. The delay phase is definitely different and, thus,
> we need a flag, compatible, to distinguish the device, SD or eMMC.
>
> Without "aspeed,ast2600-emmc" compatible, of course, eMMC device can work with original
> sdhci driver and device tree setting. But, for ultra-speed or HS200 case, AST2600 SoC needs some
> phase delay which (maximum) value is different between SD and eMMC device.

This is quite common as tweaking the timing is also need per board.
Look at what other bindings have done. A property is more appropriate
here.

Rob

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

* RE: [PATCH 10/10] dt-bindings: mmc: aspeed: Add a new compatible string
  2021-09-28 22:28       ` Rob Herring
@ 2021-09-29  3:03         ` Chin-Ting Kuo
  0 siblings, 0 replies; 31+ messages in thread
From: Chin-Ting Kuo @ 2021-09-29  3:03 UTC (permalink / raw)
  To: Rob Herring
  Cc: devicetree, linux-clk, linux-aspeed, BMC-SW, sboyd, Steven Lee,
	openbmc, linux-mmc, adrian.hunter, linux-kernel, andrew,
	mturquette, linux-arm-kernel

Hi Rob

> -----Original Message-----
> From: Rob Herring <robh@kernel.org>
> Sent: Wednesday, September 29, 2021 6:28 AM
> To: Chin-Ting Kuo <chin-ting_kuo@aspeedtech.com>
> Subject: Re: [PATCH 10/10] dt-bindings: mmc: aspeed: Add a new compatible
> string
> 
> On Mon, Sep 27, 2021 at 9:51 PM Chin-Ting Kuo
> <chin-ting_kuo@aspeedtech.com> wrote:
> >
> > Hi Rob,
> >
> > > -----Original Message-----
> > > From: Rob Herring <robh@kernel.org>
> > > Sent: Tuesday, September 28, 2021 2:59 AM
> > > To: Chin-Ting Kuo <chin-ting_kuo@aspeedtech.com>
> > > Subject: Re: [PATCH 10/10] dt-bindings: mmc: aspeed: Add a new
> > > compatible string
> > >
> > > On Wed, Sep 22, 2021 at 06:31:16PM +0800, Chin-Ting Kuo wrote:
> > > > Add "aspeed,ast2600-emmc" compatible string for the sake of
> > > > distinguishing between SD and eMMC device.
> > >
> > > Why?
> > >
> > > Is the h/w block different? We already have properties to handle
> > > some of the eMMC specifics. Also, you can have a child node for the
> > > eMMC device if you need that.
> >
> > There are two SD/SDIO controllers in a AST2600 SoC.
> > One is for SD card and the other is for eMMC.
> > Although both of them are embedded in the same SoC, the design of
> > delay cell and the manufacture process are different. The delay phase
> > is definitely different and, thus, we need a flag, compatible, to distinguish the
> device, SD or eMMC.
> >
> > Without "aspeed,ast2600-emmc" compatible, of course, eMMC device can
> > work with original sdhci driver and device tree setting. But, for
> > ultra-speed or HS200 case, AST2600 SoC needs some phase delay which
> (maximum) value is different between SD and eMMC device.
> 
> This is quite common as tweaking the timing is also need per board.
> Look at what other bindings have done. A property is more appropriate here.

Okay, I will try to check whether there is an existing binding which can achieve this purpose.
Or, maybe, as you said, a property is better since this phase delay is a proprietary
HW design and is different between each chipset version.

> 
> Rob

Chin-Ting

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

* Re: [PATCH 02/10] sdhci: aspeed: Add SDR50 support
  2021-09-22 10:31 ` [PATCH 02/10] sdhci: aspeed: Add SDR50 support Chin-Ting Kuo
@ 2021-10-26  0:31   ` Andrew Jeffery
  2021-11-06 10:01     ` Chin-Ting Kuo
  0 siblings, 1 reply; 31+ messages in thread
From: Andrew Jeffery @ 2021-10-26  0:31 UTC (permalink / raw)
  To: Chin-Ting Kuo, Rob Herring, Joel Stanley, Michael Turquette,
	Stephen Boyd, Adrian Hunter, linux-aspeed, openbmc, linux-mmc,
	devicetree, linux-arm-kernel, linux-kernel, linux-clk
  Cc: BMC-SW, Steven Lee

Hi Chin-Ting,

Sorry for the delay in looking at your series.

On Wed, 22 Sep 2021, at 20:01, Chin-Ting Kuo wrote:
> From the analog waveform analysis result, SD/SDIO controller
> of AST2600 cannot always work well with 200MHz. The upper bound
> stable frequency for SD/SDIO controller is 100MHz. Thus, SDR50
> supported bit, instead of SDR104, in capability 2 register
> should be set in advance.
>
> Signed-off-by: Chin-Ting Kuo <chin-ting_kuo@aspeedtech.com>
> ---
>  drivers/mmc/host/sdhci-of-aspeed.c | 8 ++++++++
>  1 file changed, 8 insertions(+)
>
> diff --git a/drivers/mmc/host/sdhci-of-aspeed.c 
> b/drivers/mmc/host/sdhci-of-aspeed.c
> index 6e4e132903a6..c6eaeb02e3f9 100644
> --- a/drivers/mmc/host/sdhci-of-aspeed.c
> +++ b/drivers/mmc/host/sdhci-of-aspeed.c
> @@ -35,6 +35,8 @@
>  #define ASPEED_SDC_CAP1_1_8V           (0 * 32 + 26)
>  /* SDIO{14,24} */
>  #define ASPEED_SDC_CAP2_SDR104         (1 * 32 + 1)
> +/* SDIO{14,24} */

I don't think we need to duplicate this comment.

> +#define ASPEED_SDC_CAP2_SDR50          (1 * 32 + 0)

Can we keep the defines in increasing bit order (i.e. put 
ASPEED_SDC_CAP2_SDR50 above ASPEED_SDC_CAP2_SDR104)?

> 
>  struct aspeed_sdc {
>  	struct clk *clk;
> @@ -410,11 +412,17 @@ static int aspeed_sdhci_probe(struct 
> platform_device *pdev)
>  	sdhci_get_of_property(pdev);
> 
>  	if (of_property_read_bool(np, "mmc-hs200-1_8v") ||
> +		of_property_read_bool(np, "sd-uhs-sdr50") ||

Minor formatting issue, but can you make sure all the conditions are 
aligned vertically from the left?

>  	    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-sdr50")) {
> +		aspeed_sdc_set_slot_capability(host, dev->parent, ASPEED_SDC_CAP2_SDR50,
> +					       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);
> -- 
> 2.17.1

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

* Re: [PATCH 03/10] dts: aspeed: ast2600: Support SDR50 for SD device
  2021-09-22 10:31 ` [PATCH 03/10] dts: aspeed: ast2600: Support SDR50 for SD device Chin-Ting Kuo
@ 2021-10-26  0:42   ` Andrew Jeffery
  2021-11-06 10:01     ` Chin-Ting Kuo
  0 siblings, 1 reply; 31+ messages in thread
From: Andrew Jeffery @ 2021-10-26  0:42 UTC (permalink / raw)
  To: Chin-Ting Kuo, Rob Herring, Joel Stanley, Michael Turquette,
	Stephen Boyd, Adrian Hunter, linux-aspeed, openbmc, linux-mmc,
	devicetree, linux-arm-kernel, linux-kernel, linux-clk
  Cc: BMC-SW, Steven Lee



On Wed, 22 Sep 2021, at 20:01, Chin-Ting Kuo wrote:
> The maximum frequency for SD controller on AST2600 EVB is
> 100MHz. In order to achieve 100MHz, sd-uhs-sdr50 property
> should be added and the driver will set the SDR50 supported
> bit in capability 2 register during probing stage.
>
> Signed-off-by: Chin-Ting Kuo <chin-ting_kuo@aspeedtech.com>

As this is a limitation of the SoC it should be done in aspeed-g6.dtsi. 
Unless I've misunderstood?

Andrew

> ---
>  arch/arm/boot/dts/aspeed-ast2600-evb.dts | 2 ++
>  1 file changed, 2 insertions(+)
>
> diff --git a/arch/arm/boot/dts/aspeed-ast2600-evb.dts 
> b/arch/arm/boot/dts/aspeed-ast2600-evb.dts
> index b7eb552640cb..4551dba499c2 100644
> --- a/arch/arm/boot/dts/aspeed-ast2600-evb.dts
> +++ b/arch/arm/boot/dts/aspeed-ast2600-evb.dts
> @@ -280,6 +280,7 @@
>  &sdhci0 {
>  	status = "okay";
>  	bus-width = <4>;
> +	sd-uhs-sdr50;
>  	max-frequency = <100000000>;
>  	sdhci-drive-type = /bits/ 8 <3>;
>  	sdhci-caps-mask = <0x7 0x0>;
> @@ -292,6 +293,7 @@
>  &sdhci1 {
>  	status = "okay";
>  	bus-width = <4>;
> +	sd-uhs-sdr50;
>  	max-frequency = <100000000>;
>  	sdhci-drive-type = /bits/ 8 <3>;
>  	sdhci-caps-mask = <0x7 0x0>;
> -- 
> 2.17.1

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

* Re: [PATCH 04/10] mmc: Add invert flag for clock phase signedness
  2021-09-22 10:31 ` [PATCH 04/10] mmc: Add invert flag for clock phase signedness Chin-Ting Kuo
@ 2021-10-26  0:52   ` Andrew Jeffery
  2021-11-06 10:02     ` Chin-Ting Kuo
  0 siblings, 1 reply; 31+ messages in thread
From: Andrew Jeffery @ 2021-10-26  0:52 UTC (permalink / raw)
  To: Chin-Ting Kuo, Rob Herring, Joel Stanley, Michael Turquette,
	Stephen Boyd, Adrian Hunter, linux-aspeed, openbmc, linux-mmc,
	devicetree, linux-arm-kernel, linux-kernel, linux-clk
  Cc: BMC-SW, Steven Lee

On Wed, 22 Sep 2021, at 20:01, Chin-Ting Kuo wrote:
> The clock phase degree may be between -360 to 360.
> If the data signal is leading to the clock, the signedness
> of clock phase is postive, otherwise, the signedness
> is negative.
>
> Signed-off-by: Chin-Ting Kuo <chin-ting_kuo@aspeedtech.com>

The implementation here can't be changed without a change to the 
binding documentation:

https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/devicetree/bindings/mmc/mmc-controller.yaml?h=v5.15-rc7#n345

> ---
>  drivers/mmc/core/host.c  | 10 ++++++----
>  include/linux/mmc/host.h |  2 ++
>  2 files changed, 8 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/mmc/core/host.c b/drivers/mmc/core/host.c
> index d4683b1d263f..c2de7cbc7838 100644
> --- a/drivers/mmc/core/host.c
> +++ b/drivers/mmc/core/host.c
> @@ -214,14 +214,16 @@ static void mmc_retune_timer(struct timer_list *t)
>  static void mmc_of_parse_timing_phase(struct device *dev, const char *prop,
>  				      struct mmc_clk_phase *phase)
>  {
> -	int degrees[2] = {0};
> +	int degrees[4] = {0};
>  	int rc;
> 
> -	rc = device_property_read_u32_array(dev, prop, degrees, 2);
> +	rc = device_property_read_u32_array(dev, prop, degrees, 4);
>  	phase->valid = !rc;
>  	if (phase->valid) {
> -		phase->in_deg = degrees[0];
> -		phase->out_deg = degrees[1];
> +		phase->inv_in_deg = degrees[0] ? true : false;
> +		phase->in_deg = degrees[1];
> +		phase->inv_out_deg = degrees[2] ? true : false;
> +		phase->out_deg = degrees[3];

This fundamentally breaks any in-tree users. We can't do this.

In terms of the binding, if negative phase values are something we must 
do, we can just extend the value range to include [-359, -1] right? 
That avoids changing the type of the value positions in the manner this 
patch does.

Andrew

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

* Re: [PATCH 05/10] mmc: aspeed: Adjust delay taps calculation method
  2021-09-22 10:31 ` [PATCH 05/10] mmc: aspeed: Adjust delay taps calculation method Chin-Ting Kuo
@ 2021-10-26  3:10   ` Andrew Jeffery
  2021-11-06 10:05     ` Chin-Ting Kuo
  0 siblings, 1 reply; 31+ messages in thread
From: Andrew Jeffery @ 2021-10-26  3:10 UTC (permalink / raw)
  To: Chin-Ting Kuo, Rob Herring, Joel Stanley, Michael Turquette,
	Stephen Boyd, Adrian Hunter, linux-aspeed, openbmc, linux-mmc,
	devicetree, linux-arm-kernel, linux-kernel, linux-clk
  Cc: BMC-SW, Steven Lee

Hi Chin-Ting,

I think we can split this up a bit:

On Wed, 22 Sep 2021, at 20:01, Chin-Ting Kuo wrote:
> - The maximum tap delay may be slightly different on
>   different platforms. It may also be different due to
>   different SoC processes or different manufacturers.
>   Thus, the maximum tap delay should be gotten from the
>   device tree through max-tap-delay property.

I think this could be a patch on its own

> - The delay time for each tap is an absolute value which
>   is independent of clock frequency. But, in order to combine
>   this principle with "phase" concept, clock frequency is took
>   into consideration during calculating delay taps.
> - The delay cell of eMMC device is non-uniform.
>   The time period of the first tap is two times of others.

Again, this could be a patch of its own

> - The clock phase degree range is from -360 to 360.
>   But, if the clock phase signedness is negative, clock signal
>   is output from the falling edge first by default and thus, clock
>   signal is leading to data signal by 90 degrees at least.

This line of development is impacted by my comment on an earlier patch 
in the series, so should be its own patch.

>
> Signed-off-by: Chin-Ting Kuo <chin-ting_kuo@aspeedtech.com>
> ---
>  drivers/mmc/host/sdhci-of-aspeed.c | 115 ++++++++++++++++++++++-------
>  1 file changed, 89 insertions(+), 26 deletions(-)
>
> diff --git a/drivers/mmc/host/sdhci-of-aspeed.c 
> b/drivers/mmc/host/sdhci-of-aspeed.c
> index c6eaeb02e3f9..739c9503a5ed 100644
> --- a/drivers/mmc/host/sdhci-of-aspeed.c
> +++ b/drivers/mmc/host/sdhci-of-aspeed.c
> @@ -44,6 +44,7 @@ struct aspeed_sdc {
> 
>  	spinlock_t lock;
>  	void __iomem *regs;
> +	u32 max_tap_delay_ps;
>  };
> 
>  struct aspeed_sdhci_tap_param {
> @@ -63,6 +64,7 @@ struct aspeed_sdhci_tap_desc {
>  struct aspeed_sdhci_phase_desc {
>  	struct aspeed_sdhci_tap_desc in;
>  	struct aspeed_sdhci_tap_desc out;
> +	u32 nr_taps;
>  };
> 
>  struct aspeed_sdhci_pdata {
> @@ -158,43 +160,60 @@ aspeed_sdc_set_phase_taps(struct aspeed_sdc *sdc,
>  }
> 
>  #define PICOSECONDS_PER_SECOND		1000000000000ULL
> -#define ASPEED_SDHCI_NR_TAPS		15
> -/* Measured value with *handwave* environmentals and static loading */
> -#define ASPEED_SDHCI_MAX_TAP_DELAY_PS	1253
> +#define ASPEED_SDHCI_MAX_TAPS		15

Why are we renaming this? It looks to cause a bit of noise in the diff.

> +
>  static int aspeed_sdhci_phase_to_tap(struct device *dev, unsigned long rate_hz,
> -				     int phase_deg)
> +				     bool invert, int phase_deg, u32 nr_taps)

Hmm.

>  {
>  	u64 phase_period_ps;
>  	u64 prop_delay_ps;
>  	u64 clk_period_ps;
> -	unsigned int tap;
> -	u8 inverted;
> +	u32 tap = 0;
> +	struct aspeed_sdc *sdc = dev_get_drvdata(dev->parent);
> 
> -	phase_deg %= 360;
> +	if (sdc->max_tap_delay_ps == 0)
> +		return 0;

I don't think just silently returning 0 here is the right thing to do.

What about -EINVAL, or printing a warning and using the old hard-coded 
value?

> 
> -	if (phase_deg >= 180) {
> -		inverted = ASPEED_SDHCI_TAP_PARAM_INVERT_CLK;
> -		phase_deg -= 180;
> -		dev_dbg(dev,
> -			"Inverting clock to reduce phase correction from %d to %d degrees\n",
> -			phase_deg + 180, phase_deg);
> -	} else {
> -		inverted = 0;
> +	prop_delay_ps = sdc->max_tap_delay_ps / nr_taps;
> +	clk_period_ps = div_u64(PICOSECONDS_PER_SECOND, (u64)rate_hz);
> +
> +	/*
> +	 * For ast2600, if clock phase degree is negative, clock signal is
> +	 * output from falling edge first by default. Namely, clock signal
> +	 * is leading to data signal by 90 degrees at least.
> +	 */

Have I missed something about a asymmetric clock timings? Otherwise the 
falling edge is 180 degrees away from the rising edge? I'm still not 
clear on why 90 degrees is used here.

> +	if (invert) {
> +		if (phase_deg >= 90)
> +			phase_deg -= 90;
> +		else
> +			phase_deg = 0;

Why are we throwing away information?

>  	}
> 
> -	prop_delay_ps = ASPEED_SDHCI_MAX_TAP_DELAY_PS / ASPEED_SDHCI_NR_TAPS;
> -	clk_period_ps = div_u64(PICOSECONDS_PER_SECOND, (u64)rate_hz);
>  	phase_period_ps = div_u64((u64)phase_deg * clk_period_ps, 360ULL);
> 
> -	tap = div_u64(phase_period_ps, prop_delay_ps);
> -	if (tap > ASPEED_SDHCI_NR_TAPS) {
> +	/*
> +	 * The delay cell is non-uniform for eMMC controller.
> +	 * The time period of the first tap is two times of others.
> +	 */
> +	if (nr_taps == 16 && phase_period_ps > prop_delay_ps * 2) {
> +		phase_period_ps -= prop_delay_ps * 2;
> +		tap++;
> +	}
> +
> +	tap += div_u64(phase_period_ps, prop_delay_ps);
> +	if (tap > ASPEED_SDHCI_MAX_TAPS) {
>  		dev_dbg(dev,
>  			 "Requested out of range phase tap %d for %d degrees of phase 
> compensation at %luHz, clamping to tap %d\n",
> -			 tap, phase_deg, rate_hz, ASPEED_SDHCI_NR_TAPS);
> -		tap = ASPEED_SDHCI_NR_TAPS;
> +			 tap, phase_deg, rate_hz, ASPEED_SDHCI_MAX_TAPS);
> +		tap = ASPEED_SDHCI_MAX_TAPS;
>  	}
> 
> -	return inverted | tap;
> +	if (invert) {
> +		dev_info(dev, "invert the clock\n");

I prefer we drop this message

> +		tap |= ASPEED_SDHCI_TAP_PARAM_INVERT_CLK;
> +	}
> +
> +	return tap;
>  }
> 
>  static void
> @@ -202,13 +221,19 @@ aspeed_sdhci_phases_to_taps(struct device *dev, 
> unsigned long rate,
>  			    const struct mmc_clk_phase *phases,
>  			    struct aspeed_sdhci_tap_param *taps)
>  {
> +	struct sdhci_host *host = dev->driver_data;
> +	struct aspeed_sdhci *sdhci;
> +
> +	sdhci = sdhci_pltfm_priv(sdhci_priv(host));
>  	taps->valid = phases->valid;
> 
>  	if (!phases->valid)
>  		return;
> 
> -	taps->in = aspeed_sdhci_phase_to_tap(dev, rate, phases->in_deg);
> -	taps->out = aspeed_sdhci_phase_to_tap(dev, rate, phases->out_deg);
> +	taps->in = aspeed_sdhci_phase_to_tap(dev, rate, phases->inv_in_deg,
> +				phases->in_deg, sdhci->phase_desc->nr_taps);
> +	taps->out = aspeed_sdhci_phase_to_tap(dev, rate, phases->inv_out_deg,
> +				phases->out_deg, sdhci->phase_desc->nr_taps);
>  }
> 
>  static void
> @@ -230,8 +255,8 @@ aspeed_sdhci_configure_phase(struct sdhci_host 
> *host, unsigned long rate)
>  	aspeed_sdc_set_phase_taps(sdhci->parent, sdhci->phase_desc, taps);
>  	dev_dbg(dev,
>  		"Using taps [%d, %d] for [%d, %d] degrees of phase correction at 
> %luHz (%d)\n",
> -		taps->in & ASPEED_SDHCI_NR_TAPS,
> -		taps->out & ASPEED_SDHCI_NR_TAPS,
> +		taps->in & ASPEED_SDHCI_MAX_TAPS,
> +		taps->out & ASPEED_SDHCI_MAX_TAPS,
>  		params->in_deg, params->out_deg, rate, host->timing);
>  }
> 
> @@ -493,6 +518,7 @@ static const struct aspeed_sdhci_phase_desc 
> ast2600_sdhci_phase[] = {
>  			.enable_mask = ASPEED_SDC_S0_PHASE_OUT_EN,
>  			.enable_value = 3,
>  		},
> +		.nr_taps = 15,
>  	},
>  	/* SDHCI/Slot 1 */
>  	[1] = {
> @@ -506,6 +532,31 @@ static const struct aspeed_sdhci_phase_desc 
> ast2600_sdhci_phase[] = {
>  			.enable_mask = ASPEED_SDC_S1_PHASE_OUT_EN,
>  			.enable_value = 3,
>  		},
> +		.nr_taps = 15,
> +	},
> +};
> +
> +static const struct aspeed_sdhci_phase_desc ast2600_emmc_phase[] = {
> +	/* eMMC slot 0 */
> +	[0] = {
> +		.in = {
> +			.tap_mask = ASPEED_SDC_S0_PHASE_IN,
> +			.enable_mask = ASPEED_SDC_S0_PHASE_IN_EN,
> +			.enable_value = 1,
> +		},
> +		.out = {
> +			.tap_mask = ASPEED_SDC_S0_PHASE_OUT,
> +			.enable_mask = ASPEED_SDC_S0_PHASE_OUT_EN,
> +			.enable_value = 3,
> +		},
> +
> +		/*
> +		 * There are 15 taps recorded in AST2600 datasheet.
> +		 * But, actually, the time period of the first tap
> +		 * is two times of others. Thus, 16 tap is used to
> +		 * emulate this situation.
> +		 */
> +		.nr_taps = 16,

I think this is a very indirect way to communicate the problem. The 
only time we look at nr_taps is in a test that explicitly compensates 
for the non-uniform delay. I think we should just have a boolean struct 
member called 'non_uniform_delay' rather than 'nr_taps', as the number 
of taps isn't what's changing. But also see the discussion below about 
a potential aspeed,tap-delays property.

>  	},
>  };
> 
> @@ -515,10 +566,17 @@ static const struct aspeed_sdhci_pdata 
> ast2600_sdhci_pdata = {
>  	.nr_phase_descs = ARRAY_SIZE(ast2600_sdhci_phase),
>  };
> 
> +static const struct aspeed_sdhci_pdata ast2600_emmc_pdata = {
> +	.clk_div_start = 1,
> +	.phase_desc = ast2600_emmc_phase,
> +	.nr_phase_descs = ARRAY_SIZE(ast2600_emmc_phase),
> +};
> +
>  static const struct of_device_id aspeed_sdhci_of_match[] = {
>  	{ .compatible = "aspeed,ast2400-sdhci", .data = &ast2400_sdhci_pdata, },
>  	{ .compatible = "aspeed,ast2500-sdhci", .data = &ast2400_sdhci_pdata, },
>  	{ .compatible = "aspeed,ast2600-sdhci", .data = &ast2600_sdhci_pdata, },
> +	{ .compatible = "aspeed,ast2600-emmc", .data = &ast2600_emmc_pdata, },

This needs to be documented (and binding documentation patches need to 
be the first patches in the series). Something further to consider is 
whether we separate the compatibles or add e.g. an aspeed,tap-delays 
property that specifies the specific delay of each logic element. This 
might take the place of e.g. the max-tap-delay property?

Andrew

>  	{ }
>  };
> 
> @@ -562,6 +620,11 @@ static int aspeed_sdc_probe(struct platform_device *pdev)
>  		goto err_clk;
>  	}
> 
> +	ret = of_property_read_u32(pdev->dev.of_node, "max-tap-delay",
> +			&sdc->max_tap_delay_ps);
> +	if (ret)
> +		sdc->max_tap_delay_ps = 0;
> +
>  	dev_set_drvdata(&pdev->dev, sdc);
> 
>  	parent = pdev->dev.of_node;
> -- 
> 2.17.1

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

* Re: [PATCH 01/10] clk: aspeed: ast2600: Porting sdhci clock source
  2021-09-22 10:31 ` [PATCH 01/10] clk: aspeed: ast2600: Porting sdhci clock source Chin-Ting Kuo
  2021-09-23  0:02   ` Joel Stanley
@ 2021-10-26  6:10   ` Paul Menzel
  2021-11-26  2:27     ` Chin-Ting Kuo
  1 sibling, 1 reply; 31+ messages in thread
From: Paul Menzel @ 2021-10-26  6:10 UTC (permalink / raw)
  To: Chin-Ting Kuo
  Cc: devicetree, linux-clk, linux-aspeed, BMC-SW, sboyd, steven_lee,
	mturquette, linux-mmc, adrian.hunter, linux-kernel, andrew,
	robh+dt, openbmc, linux-arm-kernel

Dear Chin-Ting,


Thank you for your patch. Some small things.

Please use imperative mood in the commit messages summary [1]:

clk: aspeed: ast2600: Port SDHCI clock source

On 22.09.21 12:31, Chin-Ting Kuo wrote:
> - There are two clock sources used to generate
>    SD/SDIO clock, APLL clock and HCLK (200MHz).
>    User can select which clock source should be used
>    by configuring SCU310[8].
> - The SD/SDIO clock divider selection table SCU310[30:28]
>    is different between AST2600-A1 and AST2600-A2/A3.
>    For AST2600-A1, 200MHz SD/SDIO clock cannot be
>    gotten by the dividers in SCU310[30:28] if APLL
>    is not the multiple of 200MHz and HCLK is 200MHz.
>    For AST2600-A2/A3, a new divider, "1", is added and
>    200MHz SD/SDIO clock can be obtained by adopting HCLK
>    as clock source and setting SCU310[30:28] to 3b'111.

Please reference the datasheet name and version, and please reflow the 
commit message for 75 characters per line.

> Signed-off-by: Chin-Ting Kuo <chin-ting_kuo@aspeedtech.com>
> ---
>   drivers/clk/clk-ast2600.c | 69 ++++++++++++++++++++++++++++++++++-----
>   1 file changed, 61 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/clk/clk-ast2600.c b/drivers/clk/clk-ast2600.c
> index bc3be5f3eae1..a6778c18274a 100644
> --- a/drivers/clk/clk-ast2600.c
> +++ b/drivers/clk/clk-ast2600.c
> @@ -168,6 +168,30 @@ static const struct clk_div_table ast2600_div_table[] = {
>   	{ 0 }
>   };
>   
> +static const struct clk_div_table ast2600_sd_div_a1_table[] = {
> +	{ 0x0, 2 },
> +	{ 0x1, 4 },
> +	{ 0x2, 6 },
> +	{ 0x3, 8 },
> +	{ 0x4, 10 },
> +	{ 0x5, 12 },
> +	{ 0x6, 14 },
> +	{ 0x7, 16 },
> +	{ 0 }
> +};
> +
> +static const struct clk_div_table ast2600_sd_div_a2_table[] = {
> +	{ 0x0, 2 },
> +	{ 0x1, 4 },
> +	{ 0x2, 6 },
> +	{ 0x3, 8 },
> +	{ 0x4, 10 },
> +	{ 0x5, 12 },
> +	{ 0x6, 14 },
> +	{ 0x7, 1 },
> +	{ 0 }
> +};
> +
>   /* For hpll/dpll/epll/mpll */
>   static struct clk_hw *ast2600_calc_pll(const char *name, u32 val)
>   {
> @@ -424,6 +448,11 @@ static const char *const emmc_extclk_parent_names[] = {
>   	"mpll",
>   };
>   
> +static const char *const sd_extclk_parent_names[] = {
> +	"hclk",
> +	"apll",
> +};
> +
>   static const char * const vclk_parent_names[] = {
>   	"dpll",
>   	"d1pll",
> @@ -523,18 +552,42 @@ static int aspeed_g6_clk_probe(struct platform_device *pdev)
>   		return PTR_ERR(hw);
>   	aspeed_g6_clk_data->hws[ASPEED_CLK_EMMC] = hw;
>   
> -	/* SD/SDIO clock divider and gate */
> -	hw = clk_hw_register_gate(dev, "sd_extclk_gate", "hpll", 0,
> -			scu_g6_base + ASPEED_G6_CLK_SELECTION4, 31, 0,
> -			&aspeed_g6_clk_lock);
> +	clk_hw_register_fixed_rate(NULL, "hclk", NULL, 0, 200000000);
> +
> +	regmap_read(map, 0x310, &val);
> +	hw = clk_hw_register_mux(dev, "sd_extclk_mux",
> +				 sd_extclk_parent_names,
> +				 ARRAY_SIZE(sd_extclk_parent_names), 0,
> +				 scu_g6_base + ASPEED_G6_CLK_SELECTION4, 8, 1,
> +				 0, &aspeed_g6_clk_lock);
>   	if (IS_ERR(hw))
>   		return PTR_ERR(hw);
> -	hw = clk_hw_register_divider_table(dev, "sd_extclk", "sd_extclk_gate",
> -			0, scu_g6_base + ASPEED_G6_CLK_SELECTION4, 28, 3, 0,
> -			ast2600_div_table,
> -			&aspeed_g6_clk_lock);
> +
> +	hw = clk_hw_register_gate(dev, "sd_extclk_gate", "sd_extclk_mux",
> +				  0, scu_g6_base + ASPEED_G6_CLK_SELECTION4,
> +				  31, 0, &aspeed_g6_clk_lock);
>   	if (IS_ERR(hw))
>   		return PTR_ERR(hw);
> +
> +	regmap_read(map, 0x14, &val);
> +	/* AST2600-A2/A3 clock divisor is different from AST2600-A1 */
> +	if (((val & GENMASK(23, 16)) >> 16) >= 2) {
> +		/* AST2600-A2/A3 */
> +		hw = clk_hw_register_divider_table(dev, "sd_extclk", "sd_extclk_gate",
> +					0, scu_g6_base + ASPEED_G6_CLK_SELECTION4, 28, 3, 0,
> +					ast2600_sd_div_a2_table,
> +					&aspeed_g6_clk_lock);
> +		if (IS_ERR(hw))
> +			return PTR_ERR(hw);
> +	} else {
> +		/* AST2600-A1 */
> +		hw = clk_hw_register_divider_table(dev, "sd_extclk", "sd_extclk_gate",
> +					0, scu_g6_base + ASPEED_G6_CLK_SELECTION4, 28, 3, 0,
> +					ast2600_sd_div_a1_table,
> +					&aspeed_g6_clk_lock);
> +		if (IS_ERR(hw))
> +			return PTR_ERR(hw);
> +	}
>   	aspeed_g6_clk_data->hws[ASPEED_CLK_SDIO] = hw;
>   
>   	/* MAC1/2 RMII 50MHz RCLK */
> 

Does Linux already log, if A1 or A2/A3 is detected?

Should a debug message be added, what clock divisor is used?


Kind regards,

Paul

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

* RE: [PATCH 02/10] sdhci: aspeed: Add SDR50 support
  2021-10-26  0:31   ` Andrew Jeffery
@ 2021-11-06 10:01     ` Chin-Ting Kuo
  0 siblings, 0 replies; 31+ messages in thread
From: Chin-Ting Kuo @ 2021-11-06 10:01 UTC (permalink / raw)
  To: Andrew Jeffery, Rob Herring, Joel Stanley, Michael Turquette,
	Stephen Boyd, Adrian Hunter, linux-aspeed, openbmc, linux-mmc,
	devicetree, linux-arm-kernel, linux-kernel, linux-clk
  Cc: BMC-SW, Steven Lee

Hi Andrew,

Thanks for the review.

> -----Original Message-----
> From: Andrew Jeffery <andrew@aj.id.au>
> Sent: Tuesday, October 26, 2021 8:31 AM
> Subject: Re: [PATCH 02/10] sdhci: aspeed: Add SDR50 support
> 
> Hi Chin-Ting,
> 
> Sorry for the delay in looking at your series.
> 
> On Wed, 22 Sep 2021, at 20:01, Chin-Ting Kuo wrote:
> > From the analog waveform analysis result, SD/SDIO controller of
> > AST2600 cannot always work well with 200MHz. The upper bound stable
> > frequency for SD/SDIO controller is 100MHz. Thus, SDR50 supported bit,
> > instead of SDR104, in capability 2 register should be set in advance.
> >
> > Signed-off-by: Chin-Ting Kuo <chin-ting_kuo@aspeedtech.com>
> > ---
> >  drivers/mmc/host/sdhci-of-aspeed.c | 8 ++++++++
> >  1 file changed, 8 insertions(+)
> >
> > diff --git a/drivers/mmc/host/sdhci-of-aspeed.c
> > b/drivers/mmc/host/sdhci-of-aspeed.c
> > index 6e4e132903a6..c6eaeb02e3f9 100644
> > --- a/drivers/mmc/host/sdhci-of-aspeed.c
> > +++ b/drivers/mmc/host/sdhci-of-aspeed.c
> > @@ -35,6 +35,8 @@
> >  #define ASPEED_SDC_CAP1_1_8V           (0 * 32 + 26)
> >  /* SDIO{14,24} */
> >  #define ASPEED_SDC_CAP2_SDR104         (1 * 32 + 1)
> > +/* SDIO{14,24} */
> 
> I don't think we need to duplicate this comment.

Okay, it will be modified in the next patch version.

> 
> > +#define ASPEED_SDC_CAP2_SDR50          (1 * 32 + 0)
> 
> Can we keep the defines in increasing bit order (i.e. put
> ASPEED_SDC_CAP2_SDR50 above ASPEED_SDC_CAP2_SDR104)?
> 

Okay.

> >
> >  struct aspeed_sdc {
> >  	struct clk *clk;
> > @@ -410,11 +412,17 @@ static int aspeed_sdhci_probe(struct
> > platform_device *pdev)
> >  	sdhci_get_of_property(pdev);
> >
> >  	if (of_property_read_bool(np, "mmc-hs200-1_8v") ||
> > +		of_property_read_bool(np, "sd-uhs-sdr50") ||
> 
> Minor formatting issue, but can you make sure all the conditions are aligned
> vertically from the left?
> 

It will also be updated in the next patch version.

> >  	    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-sdr50")) {
> > +		aspeed_sdc_set_slot_capability(host, dev->parent,
> ASPEED_SDC_CAP2_SDR50,
> > +					       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);
> > --
> > 2.17.1

Chin-Ting

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

* RE: [PATCH 03/10] dts: aspeed: ast2600: Support SDR50 for SD device
  2021-10-26  0:42   ` Andrew Jeffery
@ 2021-11-06 10:01     ` Chin-Ting Kuo
  0 siblings, 0 replies; 31+ messages in thread
From: Chin-Ting Kuo @ 2021-11-06 10:01 UTC (permalink / raw)
  To: Andrew Jeffery, Rob Herring, Joel Stanley, Michael Turquette,
	Stephen Boyd, Adrian Hunter, linux-aspeed, openbmc, linux-mmc,
	devicetree, linux-arm-kernel, linux-kernel, linux-clk
  Cc: BMC-SW, Steven Lee

> -----Original Message-----
> From: Andrew Jeffery <andrew@aj.id.au>
> Sent: Tuesday, October 26, 2021 8:43 AM
> Subject: Re: [PATCH 03/10] dts: aspeed: ast2600: Support SDR50 for SD device
> 
> 
> 
> On Wed, 22 Sep 2021, at 20:01, Chin-Ting Kuo wrote:
> > The maximum frequency for SD controller on AST2600 EVB is 100MHz. In
> > order to achieve 100MHz, sd-uhs-sdr50 property should be added and the
> > driver will set the SDR50 supported bit in capability 2 register
> > during probing stage.
> >
> > Signed-off-by: Chin-Ting Kuo <chin-ting_kuo@aspeedtech.com>
> 
> As this is a limitation of the SoC it should be done in aspeed-g6.dtsi.
> Unless I've misunderstood?
> 

Okay, it will be updated in the next patch version.

> Andrew
> 
> > ---
> >  arch/arm/boot/dts/aspeed-ast2600-evb.dts | 2 ++
> >  1 file changed, 2 insertions(+)
> >
> > diff --git a/arch/arm/boot/dts/aspeed-ast2600-evb.dts
> > b/arch/arm/boot/dts/aspeed-ast2600-evb.dts
> > index b7eb552640cb..4551dba499c2 100644
> > --- a/arch/arm/boot/dts/aspeed-ast2600-evb.dts
> > +++ b/arch/arm/boot/dts/aspeed-ast2600-evb.dts
> > @@ -280,6 +280,7 @@
> >  &sdhci0 {
> >  	status = "okay";
> >  	bus-width = <4>;
> > +	sd-uhs-sdr50;
> >  	max-frequency = <100000000>;
> >  	sdhci-drive-type = /bits/ 8 <3>;
> >  	sdhci-caps-mask = <0x7 0x0>;
> > @@ -292,6 +293,7 @@
> >  &sdhci1 {
> >  	status = "okay";
> >  	bus-width = <4>;
> > +	sd-uhs-sdr50;
> >  	max-frequency = <100000000>;
> >  	sdhci-drive-type = /bits/ 8 <3>;
> >  	sdhci-caps-mask = <0x7 0x0>;
> > --
> > 2.17.1

Chin-Ting

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

* RE: [PATCH 04/10] mmc: Add invert flag for clock phase signedness
  2021-10-26  0:52   ` Andrew Jeffery
@ 2021-11-06 10:02     ` Chin-Ting Kuo
  2021-11-08  0:21       ` Andrew Jeffery
  0 siblings, 1 reply; 31+ messages in thread
From: Chin-Ting Kuo @ 2021-11-06 10:02 UTC (permalink / raw)
  To: Andrew Jeffery, Rob Herring, Joel Stanley, Michael Turquette,
	Stephen Boyd, Adrian Hunter, linux-aspeed, openbmc, linux-mmc,
	devicetree, linux-arm-kernel, linux-kernel, linux-clk
  Cc: BMC-SW, Steven Lee

Hi Andrew,

> -----Original Message-----
> From: Andrew Jeffery <andrew@aj.id.au>
> Sent: Tuesday, October 26, 2021 8:53 AM
> Subject: Re: [PATCH 04/10] mmc: Add invert flag for clock phase signedness
> 
> On Wed, 22 Sep 2021, at 20:01, Chin-Ting Kuo wrote:
> > The clock phase degree may be between -360 to 360.
> > If the data signal is leading to the clock, the signedness of clock
> > phase is postive, otherwise, the signedness is negative.
> >
> > Signed-off-by: Chin-Ting Kuo <chin-ting_kuo@aspeedtech.com>
> 
> The implementation here can't be changed without a change to the binding
> documentation:
> 

Okay, the binding document will be changed in the next patch version.

> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Docume
> ntation/devicetree/bindings/mmc/mmc-controller.yaml?h=v5.15-rc7#n345
> 
> > ---
> >  drivers/mmc/core/host.c  | 10 ++++++----  include/linux/mmc/host.h |
> > 2 ++
> >  2 files changed, 8 insertions(+), 4 deletions(-)
> >
> > diff --git a/drivers/mmc/core/host.c b/drivers/mmc/core/host.c index
> > d4683b1d263f..c2de7cbc7838 100644
> > --- a/drivers/mmc/core/host.c
> > +++ b/drivers/mmc/core/host.c
> > @@ -214,14 +214,16 @@ static void mmc_retune_timer(struct timer_list
> > *t)  static void mmc_of_parse_timing_phase(struct device *dev, const char
> *prop,
> >  				      struct mmc_clk_phase *phase)  {
> > -	int degrees[2] = {0};
> > +	int degrees[4] = {0};
> >  	int rc;
> >
> > -	rc = device_property_read_u32_array(dev, prop, degrees, 2);
> > +	rc = device_property_read_u32_array(dev, prop, degrees, 4);
> >  	phase->valid = !rc;
> >  	if (phase->valid) {
> > -		phase->in_deg = degrees[0];
> > -		phase->out_deg = degrees[1];
> > +		phase->inv_in_deg = degrees[0] ? true : false;
> > +		phase->in_deg = degrees[1];
> > +		phase->inv_out_deg = degrees[2] ? true : false;
> > +		phase->out_deg = degrees[3];
> 
> This fundamentally breaks any in-tree users. We can't do this.
> 
> In terms of the binding, if negative phase values are something we must do,
> we can just extend the value range to include [-359, -1] right?

Yes, agree it and I tried it before. But, it seems that the device tree doesn't support
negative value with "-" prefixed and there is no device tree related API used to get
the negative value from .dts. Thus, I tried to add an additional flag to present
negative value.

> That avoids changing the type of the value positions in the manner this patch
> does.
> 
> Andrew

Chin-Ting

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

* RE: [PATCH 05/10] mmc: aspeed: Adjust delay taps calculation method
  2021-10-26  3:10   ` Andrew Jeffery
@ 2021-11-06 10:05     ` Chin-Ting Kuo
  2021-11-07 23:42       ` Andrew Jeffery
  0 siblings, 1 reply; 31+ messages in thread
From: Chin-Ting Kuo @ 2021-11-06 10:05 UTC (permalink / raw)
  To: Andrew Jeffery, Rob Herring, Joel Stanley, Michael Turquette,
	Stephen Boyd, Adrian Hunter, linux-aspeed, openbmc, linux-mmc,
	devicetree, linux-arm-kernel, linux-kernel, linux-clk
  Cc: BMC-SW, Steven Lee

Hi Andrew,

> -----Original Message-----
> From: Andrew Jeffery <andrew@aj.id.au>
> Sent: Tuesday, October 26, 2021 11:10 AM
> Subject: Re: [PATCH 05/10] mmc: aspeed: Adjust delay taps calculation method
> 
> Hi Chin-Ting,
> 
> I think we can split this up a bit:
> 
> On Wed, 22 Sep 2021, at 20:01, Chin-Ting Kuo wrote:
> > - The maximum tap delay may be slightly different on
> >   different platforms. It may also be different due to
> >   different SoC processes or different manufacturers.
> >   Thus, the maximum tap delay should be gotten from the
> >   device tree through max-tap-delay property.
> 
> I think this could be a patch on its own

Okay.

> 
> > - The delay time for each tap is an absolute value which
> >   is independent of clock frequency. But, in order to combine
> >   this principle with "phase" concept, clock frequency is took
> >   into consideration during calculating delay taps.
> > - The delay cell of eMMC device is non-uniform.
> >   The time period of the first tap is two times of others.
> 
> Again, this could be a patch of its own

Okay.

> 
> > - The clock phase degree range is from -360 to 360.
> >   But, if the clock phase signedness is negative, clock signal
> >   is output from the falling edge first by default and thus, clock
> >   signal is leading to data signal by 90 degrees at least.
> 
> This line of development is impacted by my comment on an earlier patch in
> the series, so should be its own patch.
> 

Okay.

> >
> > Signed-off-by: Chin-Ting Kuo <chin-ting_kuo@aspeedtech.com>
> > ---
> >  drivers/mmc/host/sdhci-of-aspeed.c | 115
> > ++++++++++++++++++++++-------
> >  1 file changed, 89 insertions(+), 26 deletions(-)
> >
> > diff --git a/drivers/mmc/host/sdhci-of-aspeed.c
> > b/drivers/mmc/host/sdhci-of-aspeed.c
> > index c6eaeb02e3f9..739c9503a5ed 100644
> > --- a/drivers/mmc/host/sdhci-of-aspeed.c
> > +++ b/drivers/mmc/host/sdhci-of-aspeed.c
> > @@ -44,6 +44,7 @@ struct aspeed_sdc {
> >
> >  	spinlock_t lock;
> >  	void __iomem *regs;
> > +	u32 max_tap_delay_ps;
> >  };
> >
> >  struct aspeed_sdhci_tap_param {
> > @@ -63,6 +64,7 @@ struct aspeed_sdhci_tap_desc {  struct
> > aspeed_sdhci_phase_desc {
> >  	struct aspeed_sdhci_tap_desc in;
> >  	struct aspeed_sdhci_tap_desc out;
> > +	u32 nr_taps;
> >  };
> >
> >  struct aspeed_sdhci_pdata {
> > @@ -158,43 +160,60 @@ aspeed_sdc_set_phase_taps(struct aspeed_sdc
> > *sdc,  }
> >
> >  #define PICOSECONDS_PER_SECOND		1000000000000ULL
> > -#define ASPEED_SDHCI_NR_TAPS		15
> > -/* Measured value with *handwave* environmentals and static loading */
> > -#define ASPEED_SDHCI_MAX_TAP_DELAY_PS	1253
> > +#define ASPEED_SDHCI_MAX_TAPS		15
> 
> Why are we renaming this? It looks to cause a bit of noise in the diff.
> 

Okay, it can be changed back to the original one in the next patch version.

> > +
> >  static int aspeed_sdhci_phase_to_tap(struct device *dev, unsigned long
> rate_hz,
> > -				     int phase_deg)
> > +				     bool invert, int phase_deg, u32 nr_taps)
> 
> Hmm.
> 

It will also be modified.

> >  {
> >  	u64 phase_period_ps;
> >  	u64 prop_delay_ps;
> >  	u64 clk_period_ps;
> > -	unsigned int tap;
> > -	u8 inverted;
> > +	u32 tap = 0;
> > +	struct aspeed_sdc *sdc = dev_get_drvdata(dev->parent);
> >
> > -	phase_deg %= 360;
> > +	if (sdc->max_tap_delay_ps == 0)
> > +		return 0;
> 
> I don't think just silently returning 0 here is the right thing to do.
> 
> What about -EINVAL, or printing a warning and using the old hard-coded
> value?
> 

Agree, both -EINVAL and printing a warning are better.

> >
> > -	if (phase_deg >= 180) {
> > -		inverted = ASPEED_SDHCI_TAP_PARAM_INVERT_CLK;
> > -		phase_deg -= 180;
> > -		dev_dbg(dev,
> > -			"Inverting clock to reduce phase correction from %d to %d
> degrees\n",
> > -			phase_deg + 180, phase_deg);
> > -	} else {
> > -		inverted = 0;
> > +	prop_delay_ps = sdc->max_tap_delay_ps / nr_taps;
> > +	clk_period_ps = div_u64(PICOSECONDS_PER_SECOND, (u64)rate_hz);
> > +
> > +	/*
> > +	 * For ast2600, if clock phase degree is negative, clock signal is
> > +	 * output from falling edge first by default. Namely, clock signal
> > +	 * is leading to data signal by 90 degrees at least.
> > +	 */
> 
> Have I missed something about a asymmetric clock timings? Otherwise the
> falling edge is 180 degrees away from the rising edge? I'm still not clear on
> why 90 degrees is used here.
> 

Oh, you are right. It should be 180 degrees.

> > +	if (invert) {
> > +		if (phase_deg >= 90)
> > +			phase_deg -= 90;
> > +		else
> > +			phase_deg = 0;
> 
> Why are we throwing away information?
> 

With the above correction, it should be modified as below.
If the "invert" is needed, we expect that its value should be greater than 180
degrees. We clear "phase_deg" if its value is unexpected. Maybe, a warning
should be shown and -EINVAL can be returned.

if (invert) {
	if (phase_deg >= 180)
		phase_deg -= 180;
	else
		phase_deg = 0;
}

> >  	}
> >
> > -	prop_delay_ps = ASPEED_SDHCI_MAX_TAP_DELAY_PS /
> ASPEED_SDHCI_NR_TAPS;
> > -	clk_period_ps = div_u64(PICOSECONDS_PER_SECOND, (u64)rate_hz);
> >  	phase_period_ps = div_u64((u64)phase_deg * clk_period_ps, 360ULL);
> >
> > -	tap = div_u64(phase_period_ps, prop_delay_ps);
> > -	if (tap > ASPEED_SDHCI_NR_TAPS) {
> > +	/*
> > +	 * The delay cell is non-uniform for eMMC controller.
> > +	 * The time period of the first tap is two times of others.
> > +	 */
> > +	if (nr_taps == 16 && phase_period_ps > prop_delay_ps * 2) {
> > +		phase_period_ps -= prop_delay_ps * 2;
> > +		tap++;
> > +	}
> > +
> > +	tap += div_u64(phase_period_ps, prop_delay_ps);
> > +	if (tap > ASPEED_SDHCI_MAX_TAPS) {
> >  		dev_dbg(dev,
> >  			 "Requested out of range phase tap %d for %d degrees of phase
> > compensation at %luHz, clamping to tap %d\n",
> > -			 tap, phase_deg, rate_hz, ASPEED_SDHCI_NR_TAPS);
> > -		tap = ASPEED_SDHCI_NR_TAPS;
> > +			 tap, phase_deg, rate_hz, ASPEED_SDHCI_MAX_TAPS);
> > +		tap = ASPEED_SDHCI_MAX_TAPS;
> >  	}
> >
> > -	return inverted | tap;
> > +	if (invert) {
> > +		dev_info(dev, "invert the clock\n");
> 
> I prefer we drop this message
> 

Okay.

> > +		tap |= ASPEED_SDHCI_TAP_PARAM_INVERT_CLK;
> > +	}
> > +
> > +	return tap;
> >  }
> >
> >  static void
> > @@ -202,13 +221,19 @@ aspeed_sdhci_phases_to_taps(struct device *dev,
> > unsigned long rate,
> >  			    const struct mmc_clk_phase *phases,
> >  			    struct aspeed_sdhci_tap_param *taps)  {
> > +	struct sdhci_host *host = dev->driver_data;
> > +	struct aspeed_sdhci *sdhci;
> > +
> > +	sdhci = sdhci_pltfm_priv(sdhci_priv(host));
> >  	taps->valid = phases->valid;
> >
> >  	if (!phases->valid)
> >  		return;
> >
> > -	taps->in = aspeed_sdhci_phase_to_tap(dev, rate, phases->in_deg);
> > -	taps->out = aspeed_sdhci_phase_to_tap(dev, rate, phases->out_deg);
> > +	taps->in = aspeed_sdhci_phase_to_tap(dev, rate, phases->inv_in_deg,
> > +				phases->in_deg, sdhci->phase_desc->nr_taps);
> > +	taps->out = aspeed_sdhci_phase_to_tap(dev, rate, phases->inv_out_deg,
> > +				phases->out_deg, sdhci->phase_desc->nr_taps);
> >  }
> >
> >  static void
> > @@ -230,8 +255,8 @@ aspeed_sdhci_configure_phase(struct sdhci_host
> > *host, unsigned long rate)
> >  	aspeed_sdc_set_phase_taps(sdhci->parent, sdhci->phase_desc, taps);
> >  	dev_dbg(dev,
> >  		"Using taps [%d, %d] for [%d, %d] degrees of phase correction at
> > %luHz (%d)\n",
> > -		taps->in & ASPEED_SDHCI_NR_TAPS,
> > -		taps->out & ASPEED_SDHCI_NR_TAPS,
> > +		taps->in & ASPEED_SDHCI_MAX_TAPS,
> > +		taps->out & ASPEED_SDHCI_MAX_TAPS,
> >  		params->in_deg, params->out_deg, rate, host->timing);  }
> >
> > @@ -493,6 +518,7 @@ static const struct aspeed_sdhci_phase_desc
> > ast2600_sdhci_phase[] = {
> >  			.enable_mask = ASPEED_SDC_S0_PHASE_OUT_EN,
> >  			.enable_value = 3,
> >  		},
> > +		.nr_taps = 15,
> >  	},
> >  	/* SDHCI/Slot 1 */
> >  	[1] = {
> > @@ -506,6 +532,31 @@ static const struct aspeed_sdhci_phase_desc
> > ast2600_sdhci_phase[] = {
> >  			.enable_mask = ASPEED_SDC_S1_PHASE_OUT_EN,
> >  			.enable_value = 3,
> >  		},
> > +		.nr_taps = 15,
> > +	},
> > +};
> > +
> > +static const struct aspeed_sdhci_phase_desc ast2600_emmc_phase[] = {
> > +	/* eMMC slot 0 */
> > +	[0] = {
> > +		.in = {
> > +			.tap_mask = ASPEED_SDC_S0_PHASE_IN,
> > +			.enable_mask = ASPEED_SDC_S0_PHASE_IN_EN,
> > +			.enable_value = 1,
> > +		},
> > +		.out = {
> > +			.tap_mask = ASPEED_SDC_S0_PHASE_OUT,
> > +			.enable_mask = ASPEED_SDC_S0_PHASE_OUT_EN,
> > +			.enable_value = 3,
> > +		},
> > +
> > +		/*
> > +		 * There are 15 taps recorded in AST2600 datasheet.
> > +		 * But, actually, the time period of the first tap
> > +		 * is two times of others. Thus, 16 tap is used to
> > +		 * emulate this situation.
> > +		 */
> > +		.nr_taps = 16,
> 
> I think this is a very indirect way to communicate the problem. The only time
> we look at nr_taps is in a test that explicitly compensates for the non-uniform
> delay. I think we should just have a boolean struct member called
> 'non_uniform_delay' rather than 'nr_taps', as the number of taps isn't what's
> changing. But also see the discussion below about a potential
> aspeed,tap-delays property.
> 

A new property may be the better choice.

> >  	},
> >  };
> >
> > @@ -515,10 +566,17 @@ static const struct aspeed_sdhci_pdata
> > ast2600_sdhci_pdata = {
> >  	.nr_phase_descs = ARRAY_SIZE(ast2600_sdhci_phase),  };
> >
> > +static const struct aspeed_sdhci_pdata ast2600_emmc_pdata = {
> > +	.clk_div_start = 1,
> > +	.phase_desc = ast2600_emmc_phase,
> > +	.nr_phase_descs = ARRAY_SIZE(ast2600_emmc_phase), };
> > +
> >  static const struct of_device_id aspeed_sdhci_of_match[] = {
> >  	{ .compatible = "aspeed,ast2400-sdhci", .data = &ast2400_sdhci_pdata, },
> >  	{ .compatible = "aspeed,ast2500-sdhci", .data = &ast2400_sdhci_pdata, },
> >  	{ .compatible = "aspeed,ast2600-sdhci", .data =
> > &ast2600_sdhci_pdata, },
> > +	{ .compatible = "aspeed,ast2600-emmc", .data = &ast2600_emmc_pdata,
> > +},
> 
> This needs to be documented (and binding documentation patches need to be
> the first patches in the series). 

Okay.

> Something further to consider is whether we
> separate the compatibles or add e.g. an aspeed,tap-delays property that
> specifies the specific delay of each logic element. This might take the place of
> e.g. the max-tap-delay property?
> 

Yes, an additional property may be better.

> Andrew
> 
> >  	{ }
> >  };
> >
> > @@ -562,6 +620,11 @@ static int aspeed_sdc_probe(struct platform_device
> *pdev)
> >  		goto err_clk;
> >  	}
> >
> > +	ret = of_property_read_u32(pdev->dev.of_node, "max-tap-delay",
> > +			&sdc->max_tap_delay_ps);
> > +	if (ret)
> > +		sdc->max_tap_delay_ps = 0;
> > +
> >  	dev_set_drvdata(&pdev->dev, sdc);
> >
> >  	parent = pdev->dev.of_node;
> > --
> > 2.17.1

Chin-Ting

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

* Re: [PATCH 05/10] mmc: aspeed: Adjust delay taps calculation method
  2021-11-06 10:05     ` Chin-Ting Kuo
@ 2021-11-07 23:42       ` Andrew Jeffery
  0 siblings, 0 replies; 31+ messages in thread
From: Andrew Jeffery @ 2021-11-07 23:42 UTC (permalink / raw)
  To: Chin-Ting Kuo, Rob Herring, Joel Stanley, Michael Turquette,
	Stephen Boyd, Adrian Hunter, linux-aspeed, openbmc, linux-mmc,
	devicetree, linux-arm-kernel, linux-kernel, linux-clk
  Cc: BMC-SW, Steven Lee

Hi Chin-Ting,

I've had another think about this, see my comments below.

On Sat, 6 Nov 2021, at 20:35, Chin-Ting Kuo wrote:
> Hi Andrew,
>> >  struct aspeed_sdhci_pdata {
>> > @@ -158,43 +160,60 @@ aspeed_sdc_set_phase_taps(struct aspeed_sdc
>> > *sdc,  }
>> >
>> >  #define PICOSECONDS_PER_SECOND		1000000000000ULL
>> > -#define ASPEED_SDHCI_NR_TAPS		15
>> > -/* Measured value with *handwave* environmentals and static loading */
>> > -#define ASPEED_SDHCI_MAX_TAP_DELAY_PS	1253
>> > +#define ASPEED_SDHCI_MAX_TAPS		15
>> 
>> Why are we renaming this? It looks to cause a bit of noise in the diff.
>> 
>
> Okay, it can be changed back to the original one in the next patch version.

Well, maybe it makes sense, but I think we have to take into account 
how we describe the taps in the discussion below.

>> > -	if (phase_deg >= 180) {
>> > -		inverted = ASPEED_SDHCI_TAP_PARAM_INVERT_CLK;
>> > -		phase_deg -= 180;
>> > -		dev_dbg(dev,
>> > -			"Inverting clock to reduce phase correction from %d to %d
>> degrees\n",
>> > -			phase_deg + 180, phase_deg);
>> > -	} else {
>> > -		inverted = 0;
>> > +	prop_delay_ps = sdc->max_tap_delay_ps / nr_taps;
>> > +	clk_period_ps = div_u64(PICOSECONDS_PER_SECOND, (u64)rate_hz);
>> > +
>> > +	/*
>> > +	 * For ast2600, if clock phase degree is negative, clock signal is
>> > +	 * output from falling edge first by default. Namely, clock signal
>> > +	 * is leading to data signal by 90 degrees at least.
>> > +	 */
>> 
>> Have I missed something about a asymmetric clock timings? Otherwise the
>> falling edge is 180 degrees away from the rising edge? I'm still not clear on
>> why 90 degrees is used here.
>> 
>
> Oh, you are right. It should be 180 degrees.
>
>> > +	if (invert) {
>> > +		if (phase_deg >= 90)
>> > +			phase_deg -= 90;
>> > +		else
>> > +			phase_deg = 0;
>> 
>> Why are we throwing away information?
>> 
>
> With the above correction, it should be modified as below.
> If the "invert" is needed, we expect that its value should be greater than 180
> degrees. We clear "phase_deg" if its value is unexpected. Maybe, a warning
> should be shown and -EINVAL can be returned.
>
> if (invert) {
> 	if (phase_deg >= 180)
> 		phase_deg -= 180;
> 	else
> 		phase_deg = 0;

Though we want this to return the EINVAL right?

\>> > +		/*
>> > +		 * There are 15 taps recorded in AST2600 datasheet.
>> > +		 * But, actually, the time period of the first tap
>> > +		 * is two times of others. Thus, 16 tap is used to
>> > +		 * emulate this situation.
>> > +		 */
>> > +		.nr_taps = 16,
>> 
>> I think this is a very indirect way to communicate the problem. The only time
>> we look at nr_taps is in a test that explicitly compensates for the non-uniform
>> delay. I think we should just have a boolean struct member called
>> 'non_uniform_delay' rather than 'nr_taps', as the number of taps isn't what's
>> changing. But also see the discussion below about a potential
>> aspeed,tap-delays property.
>> 
>
> A new property may be the better choice.

I think I'm changing my mind on this sorry.

I'm think that aiming for fewer custom devicetree properties is better.

Using SoC-specific and device-specific compatibles (i.e. to 
differentiate between the eMMC and SD controllers in the 2600) we can 
just encode this data straight in the driver using the OF match data.

Rob and/or Joel: Thoughts?

>
>> Something further to consider is whether we
>> separate the compatibles or add e.g. an aspeed,tap-delays property that
>> specifies the specific delay of each logic element. This might take the place of
>> e.g. the max-tap-delay property?
>> 
>
> Yes, an additional property may be better.

Again, flip-flopping on this a bit, the aspeed,ast2600-emmc compatible 
is probably fine, and we add the tap delays in the OF match data for 
the compatible. This means we get rid of any aspeed-specific devictree 
properties with respect to the delays.

Andrew

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

* Re: [PATCH 04/10] mmc: Add invert flag for clock phase signedness
  2021-11-06 10:02     ` Chin-Ting Kuo
@ 2021-11-08  0:21       ` Andrew Jeffery
  0 siblings, 0 replies; 31+ messages in thread
From: Andrew Jeffery @ 2021-11-08  0:21 UTC (permalink / raw)
  To: Chin-Ting Kuo, Rob Herring, Joel Stanley, Ulf Hansson
  Cc: BMC-SW, linux-clk, linux-aspeed, devicetree, Stephen Boyd,
	linux-mmc, Michael Turquette, Steven Lee, Adrian Hunter,
	linux-kernel, openbmc, linux-arm-kernel

On Sat, 6 Nov 2021, at 20:32, Chin-Ting Kuo wrote:
> Hi Andrew,
>>> > -	rc = device_property_read_u32_array(dev, prop, degrees, 2);
>> > +	rc = device_property_read_u32_array(dev, prop, degrees, 4);
>> >  	phase->valid = !rc;
>> >  	if (phase->valid) {
>> > -		phase->in_deg = degrees[0];
>> > -		phase->out_deg = degrees[1];
>> > +		phase->inv_in_deg = degrees[0] ? true : false;
>> > +		phase->in_deg = degrees[1];
>> > +		phase->inv_out_deg = degrees[2] ? true : false;
>> > +		phase->out_deg = degrees[3];
>> 
>> This fundamentally breaks any in-tree users. We can't do this.
>> 
>> In terms of the binding, if negative phase values are something we must do,
>> we can just extend the value range to include [-359, -1] right?
>
> Yes, agree it and I tried it before. But, it seems that the device tree 
> doesn't support
> negative value with "-" prefixed and there is no device tree related 
> API used to get
> the negative value from .dts. Thus, I tried to add an additional flag 
> to present
> negative value.
>

Hmm. Still, I don't think we can break the binding this way.

Rob, Ulf, Adrian: What are your thoughts on handling phase offsets in 
[-360, 360] in the binding? Do we append the flag field? Add a separate 
property? I don't think interleaving the flags is desirable, though 
interested in your thoughts.

Andrew

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

* RE: [PATCH 01/10] clk: aspeed: ast2600: Porting sdhci clock source
  2021-10-26  6:10   ` Paul Menzel
@ 2021-11-26  2:27     ` Chin-Ting Kuo
  0 siblings, 0 replies; 31+ messages in thread
From: Chin-Ting Kuo @ 2021-11-26  2:27 UTC (permalink / raw)
  To: Paul Menzel
  Cc: devicetree, linux-clk, linux-aspeed, BMC-SW, sboyd, Steven Lee,
	mturquette, linux-mmc, adrian.hunter, linux-kernel, andrew,
	robh+dt, openbmc, linux-arm-kernel

Hi Paul,

> -----Original Message-----
> From: Paul Menzel <pmenzel@molgen.mpg.de>
> Sent: Tuesday, October 26, 2021 2:10 PM
> To: Chin-Ting Kuo <chin-ting_kuo@aspeedtech.com>
> Cc: robh+dt@kernel.org; joel@jms.id.au; mturquette@baylibre.com;
> sboyd@kernel.org; adrian.hunter@intel.com; linux-aspeed@lists.ozlabs.org;
> openbmc@lists.ozlabs.org; linux-mmc@vger.kernel.org;
> devicetree@vger.kernel.org; linux-arm-kernel@lists.infradead.org;
> linux-kernel@vger.kernel.org; linux-clk@vger.kernel.org; andrew@aj.id.au;
> BMC-SW <BMC-SW@aspeedtech.com>; Steven Lee
> <steven_lee@aspeedtech.com>
> Subject: Re: [PATCH 01/10] clk: aspeed: ast2600: Porting sdhci clock source
> 
> Dear Chin-Ting,
> 
> 
> Thank you for your patch. Some small things.
> 
> Please use imperative mood in the commit messages summary [1]:
> 
> clk: aspeed: ast2600: Port SDHCI clock source

Okay.

> On 22.09.21 12:31, Chin-Ting Kuo wrote:
> > - There are two clock sources used to generate
> >    SD/SDIO clock, APLL clock and HCLK (200MHz).
> >    User can select which clock source should be used
> >    by configuring SCU310[8].
> > - The SD/SDIO clock divider selection table SCU310[30:28]
> >    is different between AST2600-A1 and AST2600-A2/A3.
> >    For AST2600-A1, 200MHz SD/SDIO clock cannot be
> >    gotten by the dividers in SCU310[30:28] if APLL
> >    is not the multiple of 200MHz and HCLK is 200MHz.
> >    For AST2600-A2/A3, a new divider, "1", is added and
> >    200MHz SD/SDIO clock can be obtained by adopting HCLK
> >    as clock source and setting SCU310[30:28] to 3b'111.
> 
> Please reference the datasheet name and version, and please reflow the
> commit message for 75 characters per line.
> 

Okay, it will be updated in the next patch version.

> > Signed-off-by: Chin-Ting Kuo <chin-ting_kuo@aspeedtech.com>
> > ---
> >   drivers/clk/clk-ast2600.c | 69
> ++++++++++++++++++++++++++++++++++-----
> >   1 file changed, 61 insertions(+), 8 deletions(-)
> >
> > diff --git a/drivers/clk/clk-ast2600.c b/drivers/clk/clk-ast2600.c
> > index bc3be5f3eae1..a6778c18274a 100644
> > --- a/drivers/clk/clk-ast2600.c
> > +++ b/drivers/clk/clk-ast2600.c
> > @@ -168,6 +168,30 @@ static const struct clk_div_table ast2600_div_table[]
> = {
> >   	{ 0 }
> >   };
> >
> > +static const struct clk_div_table ast2600_sd_div_a1_table[] = {
> > +	{ 0x0, 2 },
> > +	{ 0x1, 4 },
> > +	{ 0x2, 6 },
> > +	{ 0x3, 8 },
> > +	{ 0x4, 10 },
> > +	{ 0x5, 12 },
> > +	{ 0x6, 14 },
> > +	{ 0x7, 16 },
> > +	{ 0 }
> > +};
> > +
> > +static const struct clk_div_table ast2600_sd_div_a2_table[] = {
> > +	{ 0x0, 2 },
> > +	{ 0x1, 4 },
> > +	{ 0x2, 6 },
> > +	{ 0x3, 8 },
> > +	{ 0x4, 10 },
> > +	{ 0x5, 12 },
> > +	{ 0x6, 14 },
> > +	{ 0x7, 1 },
> > +	{ 0 }
> > +};
> > +
> >   /* For hpll/dpll/epll/mpll */
> >   static struct clk_hw *ast2600_calc_pll(const char *name, u32 val)
> >   {
> > @@ -424,6 +448,11 @@ static const char *const
> emmc_extclk_parent_names[] = {
> >   	"mpll",
> >   };
> >
> > +static const char *const sd_extclk_parent_names[] = {
> > +	"hclk",
> > +	"apll",
> > +};
> > +
> >   static const char * const vclk_parent_names[] = {
> >   	"dpll",
> >   	"d1pll",
> > @@ -523,18 +552,42 @@ static int aspeed_g6_clk_probe(struct
> platform_device *pdev)
> >   		return PTR_ERR(hw);
> >   	aspeed_g6_clk_data->hws[ASPEED_CLK_EMMC] = hw;
> >
> > -	/* SD/SDIO clock divider and gate */
> > -	hw = clk_hw_register_gate(dev, "sd_extclk_gate", "hpll", 0,
> > -			scu_g6_base + ASPEED_G6_CLK_SELECTION4, 31, 0,
> > -			&aspeed_g6_clk_lock);
> > +	clk_hw_register_fixed_rate(NULL, "hclk", NULL, 0, 200000000);
> > +
> > +	regmap_read(map, 0x310, &val);
> > +	hw = clk_hw_register_mux(dev, "sd_extclk_mux",
> > +				 sd_extclk_parent_names,
> > +				 ARRAY_SIZE(sd_extclk_parent_names), 0,
> > +				 scu_g6_base + ASPEED_G6_CLK_SELECTION4, 8, 1,
> > +				 0, &aspeed_g6_clk_lock);
> >   	if (IS_ERR(hw))
> >   		return PTR_ERR(hw);
> > -	hw = clk_hw_register_divider_table(dev, "sd_extclk", "sd_extclk_gate",
> > -			0, scu_g6_base + ASPEED_G6_CLK_SELECTION4, 28, 3, 0,
> > -			ast2600_div_table,
> > -			&aspeed_g6_clk_lock);
> > +
> > +	hw = clk_hw_register_gate(dev, "sd_extclk_gate", "sd_extclk_mux",
> > +				  0, scu_g6_base + ASPEED_G6_CLK_SELECTION4,
> > +				  31, 0, &aspeed_g6_clk_lock);
> >   	if (IS_ERR(hw))
> >   		return PTR_ERR(hw);
> > +
> > +	regmap_read(map, 0x14, &val);
> > +	/* AST2600-A2/A3 clock divisor is different from AST2600-A1 */
> > +	if (((val & GENMASK(23, 16)) >> 16) >= 2) {
> > +		/* AST2600-A2/A3 */
> > +		hw = clk_hw_register_divider_table(dev, "sd_extclk",
> "sd_extclk_gate",
> > +					0, scu_g6_base + ASPEED_G6_CLK_SELECTION4, 28, 3,
> 0,
> > +					ast2600_sd_div_a2_table,
> > +					&aspeed_g6_clk_lock);
> > +		if (IS_ERR(hw))
> > +			return PTR_ERR(hw);
> > +	} else {
> > +		/* AST2600-A1 */
> > +		hw = clk_hw_register_divider_table(dev, "sd_extclk",
> "sd_extclk_gate",
> > +					0, scu_g6_base + ASPEED_G6_CLK_SELECTION4, 28, 3,
> 0,
> > +					ast2600_sd_div_a1_table,
> > +					&aspeed_g6_clk_lock);
> > +		if (IS_ERR(hw))
> > +			return PTR_ERR(hw);
> > +	}
> >   	aspeed_g6_clk_data->hws[ASPEED_CLK_SDIO] = hw;
> >
> >   	/* MAC1/2 RMII 50MHz RCLK */
> >
> 
> Does Linux already log, if A1 or A2/A3 is detected?
> 

No, but Joel provided a related patch for SoC version in clk-ast2600.c.

> Should a debug message be added, what clock divisor is used?
> 

Okay, the debug message for clock divisor will be added in the next patch version.

> 
> Kind regards,
> 
> Paul


Best Wishes,
Chin-Ting

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

end of thread, other threads:[~2021-11-26  2:28 UTC | newest]

Thread overview: 31+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-09-22 10:31 [PATCH 00/10] ASPEED SD/eMMC controller clock configuration Chin-Ting Kuo
2021-09-22 10:31 ` [PATCH 01/10] clk: aspeed: ast2600: Porting sdhci clock source Chin-Ting Kuo
2021-09-23  0:02   ` Joel Stanley
2021-09-23  5:31     ` Chin-Ting Kuo
2021-10-26  6:10   ` Paul Menzel
2021-11-26  2:27     ` Chin-Ting Kuo
2021-09-22 10:31 ` [PATCH 02/10] sdhci: aspeed: Add SDR50 support Chin-Ting Kuo
2021-10-26  0:31   ` Andrew Jeffery
2021-11-06 10:01     ` Chin-Ting Kuo
2021-09-22 10:31 ` [PATCH 03/10] dts: aspeed: ast2600: Support SDR50 for SD device Chin-Ting Kuo
2021-10-26  0:42   ` Andrew Jeffery
2021-11-06 10:01     ` Chin-Ting Kuo
2021-09-22 10:31 ` [PATCH 04/10] mmc: Add invert flag for clock phase signedness Chin-Ting Kuo
2021-10-26  0:52   ` Andrew Jeffery
2021-11-06 10:02     ` Chin-Ting Kuo
2021-11-08  0:21       ` Andrew Jeffery
2021-09-22 10:31 ` [PATCH 05/10] mmc: aspeed: Adjust delay taps calculation method Chin-Ting Kuo
2021-10-26  3:10   ` Andrew Jeffery
2021-11-06 10:05     ` Chin-Ting Kuo
2021-11-07 23:42       ` Andrew Jeffery
2021-09-22 10:31 ` [PATCH 06/10] arm: dts: aspeed: Change eMMC device compatible Chin-Ting Kuo
2021-09-22 10:31 ` [PATCH 07/10] arm: dts: aspeed: Adjust clock phase parameter Chin-Ting Kuo
2021-09-22 10:31 ` [PATCH 08/10] arm: dts: ibm: " Chin-Ting Kuo
2021-09-22 10:31 ` [PATCH 09/10] dt-bindings: mmc: aspeed: Add max-tap-delay property Chin-Ting Kuo
2021-09-27 18:40   ` Rob Herring
2021-09-28  2:50     ` Chin-Ting Kuo
2021-09-22 10:31 ` [PATCH 10/10] dt-bindings: mmc: aspeed: Add a new compatible string Chin-Ting Kuo
2021-09-27 18:59   ` Rob Herring
2021-09-28  2:50     ` Chin-Ting Kuo
2021-09-28 22:28       ` Rob Herring
2021-09-29  3:03         ` Chin-Ting Kuo

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