[PATCH v3 0/3] mmc: sdhci-of-aspeed: Expose phase delay tuning

[PATCH v3 0/3] mmc: sdhci-of-aspeed: Expose phase delay tuning

[Andrew Jeffery]

Hello,

This series implements support for the MMC core clk-phase-* devicetree bindings
in the Aspeed SD/eMMC driver. The relevant register was introduced on the
AST2600 and is present for both the SD/MMC controller and the dedicated eMMC
controller.

Previously, v1 and v2 of the series implemented custom bindings. Thanks to Ulf
for pointing out that this functionality already existed in the core bindings.
For historical reference, v2 can be found here:

https://lore.kernel.org/linux-arm-kernel/20200911074452.3148259-1-andrew@aj.id.au/

The series has had light testing on an AST2600-based platform which requires
180deg of input and output clock phase correction at HS200, as well as some
synthetic testing under qemu.

Please review!

Cheers,

Andrew

Andrew Jeffery (3):
  mmc: sdhci-of-aspeed: Expose phase delay tuning
  mmc: sdhci-of-aspeed: Add AST2600 bus clock support
  ARM: dts: rainier: Add eMMC clock phase compensation

 arch/arm/boot/dts/aspeed-bmc-ibm-rainier.dts |   1 +
 drivers/mmc/host/sdhci-of-aspeed.c           | 310 ++++++++++++++++++-
 2 files changed, 300 insertions(+), 11 deletions(-)

-- 
2.27.0


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

[PATCH v3 1/3] mmc: sdhci-of-aspeed: Expose phase delay tuning

[Andrew Jeffery]

The Aspeed SD/eMMC controllers feature up to two SDHCIs alongside a
a set of "global" configuration registers. The global configuration
registers house controller-specific settings that aren't exposed by the
SDHCI, one example being a register for phase tuning.

The phase tuning feature is new in the AST2600 design. It's exposed as a
single register in the global register set and controls both the input
and output phase adjustment for each slot. As the settings are
slot-specific, the values to program are extracted from properties in
the SDHCI devicetree nodes.

Signed-off-by: Andrew Jeffery <andrew@aj.id.au>
---
 drivers/mmc/host/sdhci-of-aspeed.c | 275 ++++++++++++++++++++++++++++-
 1 file changed, 267 insertions(+), 8 deletions(-)

diff --git a/drivers/mmc/host/sdhci-of-aspeed.c b/drivers/mmc/host/sdhci-of-aspeed.c
index 4f008ba3280e..9fda2e7c1d78 100644
--- a/drivers/mmc/host/sdhci-of-aspeed.c
+++ b/drivers/mmc/host/sdhci-of-aspeed.c
@@ -6,6 +6,7 @@
 #include <linux/delay.h>
 #include <linux/device.h>
 #include <linux/io.h>
+#include <linux/math64.h>
 #include <linux/mmc/host.h>
 #include <linux/module.h>
 #include <linux/of_address.h>
@@ -16,9 +17,19 @@
 
 #include "sdhci-pltfm.h"
 
-#define ASPEED_SDC_INFO		0x00
-#define   ASPEED_SDC_S1MMC8	BIT(25)
-#define   ASPEED_SDC_S0MMC8	BIT(24)
+#define ASPEED_SDC_INFO			0x00
+#define   ASPEED_SDC_S1_MMC8		BIT(25)
+#define   ASPEED_SDC_S0_MMC8		BIT(24)
+#define ASPEED_SDC_PHASE		0xf4
+#define   ASPEED_SDC_S1_PHASE_IN	GENMASK(25, 21)
+#define   ASPEED_SDC_S0_PHASE_IN	GENMASK(20, 16)
+#define   ASPEED_SDC_S1_PHASE_OUT	GENMASK(15, 11)
+#define   ASPEED_SDC_S1_PHASE_IN_EN	BIT(10)
+#define   ASPEED_SDC_S1_PHASE_OUT_EN	GENMASK(9, 8)
+#define   ASPEED_SDC_S0_PHASE_OUT	GENMASK(7, 3)
+#define   ASPEED_SDC_S0_PHASE_IN_EN	BIT(2)
+#define   ASPEED_SDC_S0_PHASE_OUT_EN	GENMASK(1, 0)
+#define   ASPEED_SDC_PHASE_MAX		31
 
 struct aspeed_sdc {
 	struct clk *clk;
@@ -28,9 +39,42 @@ struct aspeed_sdc {
 	void __iomem *regs;
 };
 
+struct aspeed_sdhci_tap_param {
+	bool set;
+
+#define ASPEED_SDHCI_TAP_PARAM_INVERT_CLK	BIT(4)
+	u8 in;
+	u8 out;
+};
+
+struct aspeed_sdhci_phase_tap_desc {
+	u32 tap_mask;
+	u32 enable_mask;
+	u8 enable_value;
+};
+
+struct aspeed_sdhci_phase_desc {
+	struct aspeed_sdhci_phase_tap_desc in;
+	struct aspeed_sdhci_phase_tap_desc out;
+};
+
+struct aspeed_sdhci_phase_param {
+	bool set;
+	u8 in_deg;
+	u8 out_deg;
+};
+
+struct aspeed_sdhci_pdata {
+	const struct aspeed_sdhci_phase_desc *phase_desc;
+	size_t nr_phase_descs;
+};
+
 struct aspeed_sdhci {
+	const struct aspeed_sdhci_pdata *pdata;
 	struct aspeed_sdc *parent;
 	u32 width_mask;
+	const struct aspeed_sdhci_phase_desc *phase_desc;
+	struct aspeed_sdhci_phase_param phase_param[MMC_TIMING_MMC_HS200 + 1];
 };
 
 static void aspeed_sdc_configure_8bit_mode(struct aspeed_sdc *sdc,
@@ -50,10 +94,119 @@ static void aspeed_sdc_configure_8bit_mode(struct aspeed_sdc *sdc,
 	spin_unlock(&sdc->lock);
 }
 
+static u32
+aspeed_sdc_set_phase_tap(const struct aspeed_sdhci_phase_tap_desc *desc,
+			 u8 tap, bool enable, u32 reg)
+{
+	reg &= ~(desc->enable_mask | desc->tap_mask);
+	if (enable) {
+		reg |= tap << __ffs(desc->tap_mask);
+		reg |= desc->enable_value << __ffs(desc->enable_mask);
+	}
+
+	return reg;
+}
+
+static void
+aspeed_sdc_set_phase_taps(struct aspeed_sdc *sdc,
+			  const struct aspeed_sdhci_phase_desc *desc,
+			  const struct aspeed_sdhci_tap_param *taps)
+{
+	u32 reg;
+
+	spin_lock(&sdc->lock);
+	reg = readl(sdc->regs + ASPEED_SDC_PHASE);
+
+	reg = aspeed_sdc_set_phase_tap(&desc->in, taps->in, taps->set, reg);
+	reg = aspeed_sdc_set_phase_tap(&desc->out, taps->out, taps->set, reg);
+
+	writel(reg, sdc->regs + ASPEED_SDC_PHASE);
+	spin_unlock(&sdc->lock);
+}
+
+#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
+static int aspeed_sdhci_phase_to_tap(struct sdhci_host *host,
+				     unsigned long rate_hz, int phase_deg)
+{
+	u64 phase_period_ps;
+	struct device *dev;
+	u64 prop_delay_ps;
+	u64 clk_period_ps;
+	unsigned int taps;
+	u8 inverted;
+
+	dev = host->mmc->parent;
+
+	phase_deg %= 360;
+
+	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 = 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);
+
+	taps = div_u64(phase_period_ps, prop_delay_ps);
+	if (taps > ASPEED_SDHCI_NR_TAPS) {
+		dev_warn(dev,
+			 "Requested out of range phase tap %d for %d degrees of phase compensation at %luHz, clamping to tap %d\n",
+			 taps, phase_deg, rate_hz, ASPEED_SDHCI_NR_TAPS);
+		taps = ASPEED_SDHCI_NR_TAPS;
+	}
+
+	return inverted | taps;
+}
+
+static void
+aspeed_sdhci_phases_to_taps(struct sdhci_host *host, unsigned long rate,
+			    const struct aspeed_sdhci_phase_param *phases,
+			    struct aspeed_sdhci_tap_param *taps)
+{
+	taps->set = phases->set;
+
+	if (!phases->set)
+		return;
+
+	taps->in = aspeed_sdhci_phase_to_tap(host, rate, phases->in_deg);
+	taps->out = aspeed_sdhci_phase_to_tap(host, rate, phases->out_deg);
+}
+
+static void
+aspeed_sdhci_configure_phase(struct sdhci_host *host, unsigned long rate)
+{
+	struct aspeed_sdhci_tap_param _taps = {0}, *taps = &_taps;
+	struct aspeed_sdhci_phase_param *phases;
+	struct aspeed_sdhci *sdhci;
+
+	sdhci = sdhci_pltfm_priv(sdhci_priv(host));
+
+	if (!sdhci->phase_desc)
+		return;
+
+	phases = &sdhci->phase_param[host->timing];
+	aspeed_sdhci_phases_to_taps(host, rate, phases, taps);
+	aspeed_sdc_set_phase_taps(sdhci->parent, sdhci->phase_desc, taps);
+	dev_dbg(host->mmc->parent,
+		"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,
+		phases->in_deg, phases->out_deg, rate, host->timing);
+}
+
 static void aspeed_sdhci_set_clock(struct sdhci_host *host, unsigned int clock)
 {
 	struct sdhci_pltfm_host *pltfm_host;
-	unsigned long parent;
+	unsigned long parent, bus;
 	int div;
 	u16 clk;
 
@@ -69,13 +222,17 @@ static void aspeed_sdhci_set_clock(struct sdhci_host *host, unsigned int clock)
 		clock = host->max_clk;
 
 	for (div = 2; div < 256; div *= 2) {
-		if ((parent / div) <= clock)
+		bus = parent / div;
+		if (bus <= clock)
 			break;
 	}
+
 	div >>= 1;
 
 	clk = div << SDHCI_DIVIDER_SHIFT;
 
+	aspeed_sdhci_configure_phase(host, bus);
+
 	sdhci_enable_clk(host, clk);
 }
 
@@ -155,8 +312,60 @@ static inline int aspeed_sdhci_calculate_slot(struct aspeed_sdhci *dev,
 	return (delta / 0x100) - 1;
 }
 
+static void
+aspeed_sdhci_of_parse_phase(struct device_node *np, const char *prop,
+			    struct aspeed_sdhci_phase_param *phase)
+{
+	int degrees[2] = {0};
+	int rc;
+
+	rc = of_property_read_variable_u32_array(np, prop, degrees, 2, 0);
+	phase->set = rc == 2;
+	if (phase->set) {
+		phase->in_deg = degrees[0];
+		phase->out_deg = degrees[1];
+	}
+}
+
+static int aspeed_sdhci_of_parse(struct platform_device *pdev,
+				 struct aspeed_sdhci *sdhci)
+{
+	struct device_node *np;
+	struct device *dev;
+
+	if (!sdhci->phase_desc)
+		return 0;
+
+	dev = &pdev->dev;
+	np = dev->of_node;
+
+	aspeed_sdhci_of_parse_phase(np, "clk-phase-legacy",
+				    &sdhci->phase_param[MMC_TIMING_LEGACY]);
+	aspeed_sdhci_of_parse_phase(np, "clk-phase-mmc-hs",
+				    &sdhci->phase_param[MMC_TIMING_MMC_HS]);
+	aspeed_sdhci_of_parse_phase(np, "clk-phase-sd-hs",
+				    &sdhci->phase_param[MMC_TIMING_SD_HS]);
+	aspeed_sdhci_of_parse_phase(np, "clk-phase-uhs-sdr12",
+				    &sdhci->phase_param[MMC_TIMING_UHS_SDR12]);
+	aspeed_sdhci_of_parse_phase(np, "clk-phase-uhs-sdr25",
+				    &sdhci->phase_param[MMC_TIMING_UHS_SDR25]);
+	aspeed_sdhci_of_parse_phase(np, "clk-phase-uhs-sdr50",
+				    &sdhci->phase_param[MMC_TIMING_UHS_SDR50]);
+	aspeed_sdhci_of_parse_phase(np, "clk-phase-uhs-sdr104",
+				    &sdhci->phase_param[MMC_TIMING_UHS_SDR104]);
+	aspeed_sdhci_of_parse_phase(np, "clk-phase-uhs-ddr50",
+				    &sdhci->phase_param[MMC_TIMING_UHS_DDR50]);
+	aspeed_sdhci_of_parse_phase(np, "clk-phase-mmc-ddr52",
+				    &sdhci->phase_param[MMC_TIMING_MMC_DDR52]);
+	aspeed_sdhci_of_parse_phase(np, "clk-phase-mmc-hs200",
+				    &sdhci->phase_param[MMC_TIMING_MMC_HS200]);
+
+	return 0;
+}
+
 static int aspeed_sdhci_probe(struct platform_device *pdev)
 {
+	const struct aspeed_sdhci_pdata *aspeed_pdata;
 	struct sdhci_pltfm_host *pltfm_host;
 	struct aspeed_sdhci *dev;
 	struct sdhci_host *host;
@@ -164,12 +373,15 @@ static int aspeed_sdhci_probe(struct platform_device *pdev)
 	int slot;
 	int ret;
 
+	aspeed_pdata = of_device_get_match_data(&pdev->dev);
+
 	host = sdhci_pltfm_init(pdev, &aspeed_sdhci_pdata, sizeof(*dev));
 	if (IS_ERR(host))
 		return PTR_ERR(host);
 
 	pltfm_host = sdhci_priv(host);
 	dev = sdhci_pltfm_priv(pltfm_host);
+	dev->pdata = aspeed_pdata;
 	dev->parent = dev_get_drvdata(pdev->dev.parent);
 
 	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
@@ -180,8 +392,17 @@ static int aspeed_sdhci_probe(struct platform_device *pdev)
 	else if (slot >= 2)
 		return -EINVAL;
 
-	dev_info(&pdev->dev, "Configuring for slot %d\n", slot);
-	dev->width_mask = !slot ? ASPEED_SDC_S0MMC8 : ASPEED_SDC_S1MMC8;
+	if (dev->pdata && slot < dev->pdata->nr_phase_descs) {
+		dev->phase_desc = &dev->pdata->phase_desc[slot];
+	} else {
+		dev_info(&pdev->dev,
+			 "Phase control not supported for slot %d\n", slot);
+		dev->phase_desc = NULL;
+	}
+
+	dev->width_mask = !slot ? ASPEED_SDC_S0_MMC8 : ASPEED_SDC_S1_MMC8;
+
+	dev_info(&pdev->dev, "Configured for slot %d\n", slot);
 
 	sdhci_get_of_property(pdev);
 
@@ -195,6 +416,10 @@ static int aspeed_sdhci_probe(struct platform_device *pdev)
 		goto err_pltfm_free;
 	}
 
+	ret = aspeed_sdhci_of_parse(pdev, dev);
+	if (ret)
+		goto err_sdhci_add;
+
 	ret = mmc_of_parse(host->mmc);
 	if (ret)
 		goto err_sdhci_add;
@@ -230,10 +455,44 @@ static int aspeed_sdhci_remove(struct platform_device *pdev)
 	return 0;
 }
 
+static const struct aspeed_sdhci_phase_desc ast2600_sdhci_phase[] = {
+	/* SDHCI/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,
+		},
+	},
+	/* SDHCI/Slot 1 */
+	[1] = {
+		.in = {
+			.tap_mask = ASPEED_SDC_S1_PHASE_IN,
+			.enable_mask = ASPEED_SDC_S1_PHASE_IN_EN,
+			.enable_value = 1,
+		},
+		.out = {
+			.tap_mask = ASPEED_SDC_S1_PHASE_OUT,
+			.enable_mask = ASPEED_SDC_S1_PHASE_OUT_EN,
+			.enable_value = 3,
+		},
+	},
+};
+
+static const struct aspeed_sdhci_pdata ast2600_sdhci_pdata = {
+	.phase_desc = ast2600_sdhci_phase,
+	.nr_phase_descs = ARRAY_SIZE(ast2600_sdhci_phase),
+};
+
 static const struct of_device_id aspeed_sdhci_of_match[] = {
 	{ .compatible = "aspeed,ast2400-sdhci", },
 	{ .compatible = "aspeed,ast2500-sdhci", },
-	{ .compatible = "aspeed,ast2600-sdhci", },
+	{ .compatible = "aspeed,ast2600-sdhci", .data = &ast2600_sdhci_pdata, },
 	{ }
 };
 
-- 
2.27.0


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

[PATCH v3 2/3] mmc: sdhci-of-aspeed: Add AST2600 bus clock support

[Andrew Jeffery]

The AST2600 can achieve HS200 speeds with a change to the bus clock
divisor behaviour. The divisor can also be more accurate with respect
to the requested clock rate, but keep the one-hot behaviour for
backwards compatibility with the AST2400 and AST2500.

Signed-off-by: Andrew Jeffery <andrew@aj.id.au>
---
 drivers/mmc/host/sdhci-of-aspeed.c | 37 ++++++++++++++++++++++++++----
 1 file changed, 33 insertions(+), 4 deletions(-)

diff --git a/drivers/mmc/host/sdhci-of-aspeed.c b/drivers/mmc/host/sdhci-of-aspeed.c
index 9fda2e7c1d78..52179b800e6c 100644
--- a/drivers/mmc/host/sdhci-of-aspeed.c
+++ b/drivers/mmc/host/sdhci-of-aspeed.c
@@ -65,6 +65,7 @@ struct aspeed_sdhci_phase_param {
 };
 
 struct aspeed_sdhci_pdata {
+	unsigned int clk_div_start;
 	const struct aspeed_sdhci_phase_desc *phase_desc;
 	size_t nr_phase_descs;
 };
@@ -207,10 +208,13 @@ static void aspeed_sdhci_set_clock(struct sdhci_host *host, unsigned int clock)
 {
 	struct sdhci_pltfm_host *pltfm_host;
 	unsigned long parent, bus;
+	struct aspeed_sdhci *sdhci;
 	int div;
 	u16 clk;
 
 	pltfm_host = sdhci_priv(host);
+	sdhci = sdhci_pltfm_priv(pltfm_host);
+
 	parent = clk_get_rate(pltfm_host->clk);
 
 	sdhci_writew(host, 0, SDHCI_CLOCK_CONTROL);
@@ -221,7 +225,23 @@ static void aspeed_sdhci_set_clock(struct sdhci_host *host, unsigned int clock)
 	if (WARN_ON(clock > host->max_clk))
 		clock = host->max_clk;
 
-	for (div = 2; div < 256; div *= 2) {
+	/*
+	 * Regarding the AST2600:
+	 *
+	 * If (EMMC12C[7:6], EMMC12C[15:8] == 0) then
+	 *   period of SDCLK = period of SDMCLK.
+	 *
+	 * If (EMMC12C[7:6], EMMC12C[15:8] != 0) then
+	 *   period of SDCLK = period of SDMCLK * 2 * (EMMC12C[7:6], EMMC[15:8])
+	 *
+	 * If you keep EMMC12C[7:6] = 0 and EMMC12C[15:8] as one-hot,
+	 * 0x1/0x2/0x4/etc, you will find it is compatible to AST2400 or AST2500
+	 *
+	 * Keep the one-hot behaviour for backwards compatibility except for
+	 * supporting the value 0 in (EMMC12C[7:6], EMMC12C[15:8]), and capture
+	 * the 0-value capability in clk_div_start.
+	 */
+	for (div = sdhci->pdata->clk_div_start; div < 256; div *= 2) {
 		bus = parent / div;
 		if (bus <= clock)
 			break;
@@ -374,6 +394,10 @@ static int aspeed_sdhci_probe(struct platform_device *pdev)
 	int ret;
 
 	aspeed_pdata = of_device_get_match_data(&pdev->dev);
+	if (!aspeed_pdata) {
+		dev_err(&pdev->dev, "Missing platform configuration data\n");
+		return -EINVAL;
+	}
 
 	host = sdhci_pltfm_init(pdev, &aspeed_sdhci_pdata, sizeof(*dev));
 	if (IS_ERR(host))
@@ -392,7 +416,7 @@ static int aspeed_sdhci_probe(struct platform_device *pdev)
 	else if (slot >= 2)
 		return -EINVAL;
 
-	if (dev->pdata && slot < dev->pdata->nr_phase_descs) {
+	if (slot < dev->pdata->nr_phase_descs) {
 		dev->phase_desc = &dev->pdata->phase_desc[slot];
 	} else {
 		dev_info(&pdev->dev,
@@ -455,6 +479,10 @@ static int aspeed_sdhci_remove(struct platform_device *pdev)
 	return 0;
 }
 
+static const struct aspeed_sdhci_pdata ast2400_sdhci_pdata = {
+	.clk_div_start = 2,
+};
+
 static const struct aspeed_sdhci_phase_desc ast2600_sdhci_phase[] = {
 	/* SDHCI/Slot 0 */
 	[0] = {
@@ -485,13 +513,14 @@ static const struct aspeed_sdhci_phase_desc ast2600_sdhci_phase[] = {
 };
 
 static const struct aspeed_sdhci_pdata ast2600_sdhci_pdata = {
+	.clk_div_start = 1,
 	.phase_desc = ast2600_sdhci_phase,
 	.nr_phase_descs = ARRAY_SIZE(ast2600_sdhci_phase),
 };
 
 static const struct of_device_id aspeed_sdhci_of_match[] = {
-	{ .compatible = "aspeed,ast2400-sdhci", },
-	{ .compatible = "aspeed,ast2500-sdhci", },
+	{ .compatible = "aspeed,ast2400-sdhci", .data = &ast2400_sdhci_pdata, },
+	{ .compatible = "aspeed,ast2500-sdhci", .data = &ast2400_sdhci_pdata, },
 	{ .compatible = "aspeed,ast2600-sdhci", .data = &ast2600_sdhci_pdata, },
 	{ }
 };
-- 
2.27.0


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

[PATCH v3 3/3] ARM: dts: rainier: Add eMMC clock phase compensation

[Andrew Jeffery]

Determined by scope measurements at speed.

Signed-off-by: Andrew Jeffery <andrew@aj.id.au>
---
 arch/arm/boot/dts/aspeed-bmc-ibm-rainier.dts | 1 +
 1 file changed, 1 insertion(+)

diff --git a/arch/arm/boot/dts/aspeed-bmc-ibm-rainier.dts b/arch/arm/boot/dts/aspeed-bmc-ibm-rainier.dts
index 21ae880c7530..ab8d37d49f30 100644
--- a/arch/arm/boot/dts/aspeed-bmc-ibm-rainier.dts
+++ b/arch/arm/boot/dts/aspeed-bmc-ibm-rainier.dts
@@ -186,6 +186,7 @@ &pinctrl_emmc_default {
 
 &emmc {
 	status = "okay";
+	clk-phase-mmc-hs200 = <180>, <180>;
 };
 
 &fsim0 {
-- 
2.27.0


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

Re: [PATCH v3 1/3] mmc: sdhci-of-aspeed: Expose phase delay tuning

[Ulf Hansson]

On Mon, 23 Nov 2020 at 07:30, Andrew Jeffery <andrew@aj.id.au> wrote:
>
> The Aspeed SD/eMMC controllers feature up to two SDHCIs alongside a
> a set of "global" configuration registers. The global configuration
> registers house controller-specific settings that aren't exposed by the
> SDHCI, one example being a register for phase tuning.
>
> The phase tuning feature is new in the AST2600 design. It's exposed as a
> single register in the global register set and controls both the input
> and output phase adjustment for each slot. As the settings are
> slot-specific, the values to program are extracted from properties in
> the SDHCI devicetree nodes.
>
> Signed-off-by: Andrew Jeffery <andrew@aj.id.au>

[...]

>
> +static void
> +aspeed_sdhci_of_parse_phase(struct device_node *np, const char *prop,
> +                           struct aspeed_sdhci_phase_param *phase)
> +{
> +       int degrees[2] = {0};
> +       int rc;
> +
> +       rc = of_property_read_variable_u32_array(np, prop, degrees, 2, 0);
> +       phase->set = rc == 2;
> +       if (phase->set) {
> +               phase->in_deg = degrees[0];
> +               phase->out_deg = degrees[1];
> +       }
> +}
> +
> +static int aspeed_sdhci_of_parse(struct platform_device *pdev,
> +                                struct aspeed_sdhci *sdhci)
> +{
> +       struct device_node *np;
> +       struct device *dev;
> +
> +       if (!sdhci->phase_desc)
> +               return 0;
> +
> +       dev = &pdev->dev;
> +       np = dev->of_node;
> +
> +       aspeed_sdhci_of_parse_phase(np, "clk-phase-legacy",
> +                                   &sdhci->phase_param[MMC_TIMING_LEGACY]);
> +       aspeed_sdhci_of_parse_phase(np, "clk-phase-mmc-hs",
> +                                   &sdhci->phase_param[MMC_TIMING_MMC_HS]);
> +       aspeed_sdhci_of_parse_phase(np, "clk-phase-sd-hs",
> +                                   &sdhci->phase_param[MMC_TIMING_SD_HS]);
> +       aspeed_sdhci_of_parse_phase(np, "clk-phase-uhs-sdr12",
> +                                   &sdhci->phase_param[MMC_TIMING_UHS_SDR12]);
> +       aspeed_sdhci_of_parse_phase(np, "clk-phase-uhs-sdr25",
> +                                   &sdhci->phase_param[MMC_TIMING_UHS_SDR25]);
> +       aspeed_sdhci_of_parse_phase(np, "clk-phase-uhs-sdr50",
> +                                   &sdhci->phase_param[MMC_TIMING_UHS_SDR50]);
> +       aspeed_sdhci_of_parse_phase(np, "clk-phase-uhs-sdr104",
> +                                   &sdhci->phase_param[MMC_TIMING_UHS_SDR104]);
> +       aspeed_sdhci_of_parse_phase(np, "clk-phase-uhs-ddr50",
> +                                   &sdhci->phase_param[MMC_TIMING_UHS_DDR50]);
> +       aspeed_sdhci_of_parse_phase(np, "clk-phase-mmc-ddr52",
> +                                   &sdhci->phase_param[MMC_TIMING_MMC_DDR52]);
> +       aspeed_sdhci_of_parse_phase(np, "clk-phase-mmc-hs200",
> +                                   &sdhci->phase_param[MMC_TIMING_MMC_HS200]);
> +
> +       return 0;
> +}

If it's not too much to ask, would you mind adding a helper function
to the mmc core, as to let us avoid open coding? Then we should be
able to move the sdhci-of-arasan driver to use this as well.

Perhaps the definition of the helper could look something like this:
int mmc_of_parse_clk_phase(struct mmc_host *host, struct mmc_clk_phase
*phases) (or something along those lines)

I think the struct mmc_clk_phase could be something that is stored in
the host specific struct, rather than in the common struct mmc_host
(to avoid sprinkle it with unnecessary data).

Moreover, we should probably use the device_property_* APIs instead of
the DT specific of_property_*.

[...]

Kind regards
Uffe

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

Re: [PATCH v3 1/3] mmc: sdhci-of-aspeed: Expose phase delay tuning

[Andrew Jeffery]

On Wed, 25 Nov 2020, at 00:42, Ulf Hansson wrote:
> On Mon, 23 Nov 2020 at 07:30, Andrew Jeffery <andrew@aj.id.au> wrote:
> >
> > The Aspeed SD/eMMC controllers feature up to two SDHCIs alongside a
> > a set of "global" configuration registers. The global configuration
> > registers house controller-specific settings that aren't exposed by the
> > SDHCI, one example being a register for phase tuning.
> >
> > The phase tuning feature is new in the AST2600 design. It's exposed as a
> > single register in the global register set and controls both the input
> > and output phase adjustment for each slot. As the settings are
> > slot-specific, the values to program are extracted from properties in
> > the SDHCI devicetree nodes.
> >
> > Signed-off-by: Andrew Jeffery <andrew@aj.id.au>
> 
> [...]
> 
> >
> > +static void
> > +aspeed_sdhci_of_parse_phase(struct device_node *np, const char *prop,
> > +                           struct aspeed_sdhci_phase_param *phase)
> > +{
> > +       int degrees[2] = {0};
> > +       int rc;
> > +
> > +       rc = of_property_read_variable_u32_array(np, prop, degrees, 2, 0);
> > +       phase->set = rc == 2;
> > +       if (phase->set) {
> > +               phase->in_deg = degrees[0];
> > +               phase->out_deg = degrees[1];
> > +       }
> > +}
> > +
> > +static int aspeed_sdhci_of_parse(struct platform_device *pdev,
> > +                                struct aspeed_sdhci *sdhci)
> > +{
> > +       struct device_node *np;
> > +       struct device *dev;
> > +
> > +       if (!sdhci->phase_desc)
> > +               return 0;
> > +
> > +       dev = &pdev->dev;
> > +       np = dev->of_node;
> > +
> > +       aspeed_sdhci_of_parse_phase(np, "clk-phase-legacy",
> > +                                   &sdhci->phase_param[MMC_TIMING_LEGACY]);
> > +       aspeed_sdhci_of_parse_phase(np, "clk-phase-mmc-hs",
> > +                                   &sdhci->phase_param[MMC_TIMING_MMC_HS]);
> > +       aspeed_sdhci_of_parse_phase(np, "clk-phase-sd-hs",
> > +                                   &sdhci->phase_param[MMC_TIMING_SD_HS]);
> > +       aspeed_sdhci_of_parse_phase(np, "clk-phase-uhs-sdr12",
> > +                                   &sdhci->phase_param[MMC_TIMING_UHS_SDR12]);
> > +       aspeed_sdhci_of_parse_phase(np, "clk-phase-uhs-sdr25",
> > +                                   &sdhci->phase_param[MMC_TIMING_UHS_SDR25]);
> > +       aspeed_sdhci_of_parse_phase(np, "clk-phase-uhs-sdr50",
> > +                                   &sdhci->phase_param[MMC_TIMING_UHS_SDR50]);
> > +       aspeed_sdhci_of_parse_phase(np, "clk-phase-uhs-sdr104",
> > +                                   &sdhci->phase_param[MMC_TIMING_UHS_SDR104]);
> > +       aspeed_sdhci_of_parse_phase(np, "clk-phase-uhs-ddr50",
> > +                                   &sdhci->phase_param[MMC_TIMING_UHS_DDR50]);
> > +       aspeed_sdhci_of_parse_phase(np, "clk-phase-mmc-ddr52",
> > +                                   &sdhci->phase_param[MMC_TIMING_MMC_DDR52]);
> > +       aspeed_sdhci_of_parse_phase(np, "clk-phase-mmc-hs200",
> > +                                   &sdhci->phase_param[MMC_TIMING_MMC_HS200]);
> > +
> > +       return 0;
> > +}
> 
> If it's not too much to ask, would you mind adding a helper function
> to the mmc core, as to let us avoid open coding? Then we should be
> able to move the sdhci-of-arasan driver to use this as well.

Yes, I can look at it and send a v4.

> 
> Perhaps the definition of the helper could look something like this:
> int mmc_of_parse_clk_phase(struct mmc_host *host, struct mmc_clk_phase
> *phases) (or something along those lines)
> 
> I think the struct mmc_clk_phase could be something that is stored in
> the host specific struct, rather than in the common struct mmc_host
> (to avoid sprinkle it with unnecessary data).
> 
> Moreover, we should probably use the device_property_* APIs instead of
> the DT specific of_property_*.

Yep, thanks for the pointers.

Andrew

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