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

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

[Andrew Jeffery]

Hello,

This series exposes some devicetree properties for tuning sample phase
delay in the Aspeed SD/eMMC controllers. The relevant register was
introduced on the AST2600 and is present for both the SD/MMC controller
and the dedicated eMMC controller.

Please review!

Joel: If Rob's happy with the binding change can you take the dts patch
through the aspeed dt tree?

Cheers,

Andrew

Andrew Jeffery (3):
  dt: bindings: mmc: Add phase control properties for the Aspeed SDHCI
  mmc: sdhci-of-aspeed: Expose data sample phase delay tuning
  ARM: dts: tacoma: Add data sample phase delay for eMMC

 .../devicetree/bindings/mmc/aspeed,sdhci.yaml |   8 +
 arch/arm/boot/dts/aspeed-bmc-opp-tacoma.dts   |   2 +
 drivers/mmc/host/sdhci-of-aspeed.c            | 137 +++++++++++++++++-
 3 files changed, 142 insertions(+), 5 deletions(-)

-- 
2.25.1

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

[Andrew Jeffery]

Hello,

This series exposes some devicetree properties for tuning sample phase
delay in the Aspeed SD/eMMC controllers. The relevant register was
introduced on the AST2600 and is present for both the SD/MMC controller
and the dedicated eMMC controller.

Please review!

Joel: If Rob's happy with the binding change can you take the dts patch
through the aspeed dt tree?

Cheers,

Andrew

Andrew Jeffery (3):
  dt: bindings: mmc: Add phase control properties for the Aspeed SDHCI
  mmc: sdhci-of-aspeed: Expose data sample phase delay tuning
  ARM: dts: tacoma: Add data sample phase delay for eMMC

 .../devicetree/bindings/mmc/aspeed,sdhci.yaml |   8 +
 arch/arm/boot/dts/aspeed-bmc-opp-tacoma.dts   |   2 +
 drivers/mmc/host/sdhci-of-aspeed.c            | 137 +++++++++++++++++-
 3 files changed, 142 insertions(+), 5 deletions(-)

-- 
2.25.1


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

[PATCH 1/3] dt: bindings: mmc: Add phase control properties for the Aspeed SDHCI

[Andrew Jeffery]

Add properties to control the phase delay for input and output data
sampling.

Signed-off-by: Andrew Jeffery <andrew@aj.id.au>
---
 Documentation/devicetree/bindings/mmc/aspeed,sdhci.yaml | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/Documentation/devicetree/bindings/mmc/aspeed,sdhci.yaml b/Documentation/devicetree/bindings/mmc/aspeed,sdhci.yaml
index 987b287f3bff..75effd411554 100644
--- a/Documentation/devicetree/bindings/mmc/aspeed,sdhci.yaml
+++ b/Documentation/devicetree/bindings/mmc/aspeed,sdhci.yaml
@@ -61,6 +61,14 @@ patternProperties:
       sdhci,auto-cmd12:
         type: boolean
         description: Specifies that controller should use auto CMD12
+      "aspeed,input-phase":
+        $ref: '/schemas/types.yaml#/definitions/uint32'
+        description:
+          The input clock phase delay value.
+      "aspeed,output-phase":
+        $ref: '/schemas/types.yaml#/definitions/uint32'
+        description:
+          The output clock phase delay value.
     required:
       - compatible
       - reg
-- 
2.25.1

[PATCH 1/3] dt: bindings: mmc: Add phase control properties for the Aspeed SDHCI

[Andrew Jeffery]

Add properties to control the phase delay for input and output data
sampling.

Signed-off-by: Andrew Jeffery <andrew@aj.id.au>
---
 Documentation/devicetree/bindings/mmc/aspeed,sdhci.yaml | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/Documentation/devicetree/bindings/mmc/aspeed,sdhci.yaml b/Documentation/devicetree/bindings/mmc/aspeed,sdhci.yaml
index 987b287f3bff..75effd411554 100644
--- a/Documentation/devicetree/bindings/mmc/aspeed,sdhci.yaml
+++ b/Documentation/devicetree/bindings/mmc/aspeed,sdhci.yaml
@@ -61,6 +61,14 @@ patternProperties:
       sdhci,auto-cmd12:
         type: boolean
         description: Specifies that controller should use auto CMD12
+      "aspeed,input-phase":
+        $ref: '/schemas/types.yaml#/definitions/uint32'
+        description:
+          The input clock phase delay value.
+      "aspeed,output-phase":
+        $ref: '/schemas/types.yaml#/definitions/uint32'
+        description:
+          The output clock phase delay value.
     required:
       - compatible
       - reg
-- 
2.25.1


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

[PATCH 2/3] mmc: sdhci-of-aspeed: Expose data sample phase delay tuning

[Andrew Jeffery]

Allow sample phase adjustment to deal with layout or tolerance issues.

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

diff --git a/drivers/mmc/host/sdhci-of-aspeed.c b/drivers/mmc/host/sdhci-of-aspeed.c
index 4f008ba3280e..641accbfcde4 100644
--- a/drivers/mmc/host/sdhci-of-aspeed.c
+++ b/drivers/mmc/host/sdhci-of-aspeed.c
@@ -16,9 +16,18 @@
 
 #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)
 
 struct aspeed_sdc {
 	struct clk *clk;
@@ -28,9 +37,21 @@ struct aspeed_sdc {
 	void __iomem *regs;
 };
 
+struct aspeed_sdhci_phase_desc {
+	u32 value_mask;
+	u32 enable_mask;
+	u8 enable_value;
+};
+
+struct aspeed_sdhci_phase {
+	struct aspeed_sdhci_phase_desc in;
+	struct aspeed_sdhci_phase_desc out;
+};
+
 struct aspeed_sdhci {
 	struct aspeed_sdc *parent;
 	u32 width_mask;
+	const struct aspeed_sdhci_phase *phase;
 };
 
 static void aspeed_sdc_configure_8bit_mode(struct aspeed_sdc *sdc,
@@ -50,6 +71,25 @@ static void aspeed_sdc_configure_8bit_mode(struct aspeed_sdc *sdc,
 	spin_unlock(&sdc->lock);
 }
 
+static void
+aspeed_sdc_configure_phase(struct aspeed_sdc *sdc,
+			   const struct aspeed_sdhci_phase_desc *phase,
+			   uint8_t value, bool enable)
+{
+	u32 reg;
+
+	spin_lock(&sdc->lock);
+	reg = readl(sdc->regs + ASPEED_SDC_PHASE);
+	reg &= ~phase->enable_mask;
+	if (enable) {
+		reg &= ~phase->value_mask;
+		reg |= value << __ffs(phase->value_mask);
+		reg |= phase->enable_value << __ffs(phase->enable_mask);
+	}
+	writel(reg, sdc->regs + ASPEED_SDC_PHASE);
+	spin_unlock(&sdc->lock);
+}
+
 static void aspeed_sdhci_set_clock(struct sdhci_host *host, unsigned int clock)
 {
 	struct sdhci_pltfm_host *pltfm_host;
@@ -155,8 +195,58 @@ static inline int aspeed_sdhci_calculate_slot(struct aspeed_sdhci *dev,
 	return (delta / 0x100) - 1;
 }
 
+static int aspeed_sdhci_configure_of(struct platform_device *pdev,
+				     struct aspeed_sdhci *sdhci)
+{
+	u32 iphase, ophase;
+	struct device_node *np;
+	struct device *dev;
+	int ret;
+
+	if (!sdhci->phase)
+		return 0;
+
+	dev = &pdev->dev;
+	np = dev->of_node;
+
+	ret = of_property_read_u32(np, "aspeed,input-phase", &iphase);
+	if (ret < 0) {
+		aspeed_sdc_configure_phase(sdhci->parent, &sdhci->phase->in, 0,
+					   false);
+		dev_dbg(dev, "Input phase configuration disabled");
+	} else if (iphase >= (1 << 5)) {
+		dev_err(dev,
+			"Input phase value exceeds field range (5 bits): %u",
+			iphase);
+		return -ERANGE;
+	} else {
+		aspeed_sdc_configure_phase(sdhci->parent, &sdhci->phase->in,
+					   iphase, true);
+		dev_info(dev, "Input phase relationship: %u", iphase);
+	}
+
+	ret = of_property_read_u32(np, "aspeed,output-phase", &ophase);
+	if (ret < 0) {
+		aspeed_sdc_configure_phase(sdhci->parent, &sdhci->phase->out, 0,
+					   false);
+		dev_dbg(dev, "Output phase configuration disabled");
+	} else if (ophase >= (1 << 5)) {
+		dev_err(dev,
+			"Output phase value exceeds field range (5 bits): %u",
+			iphase);
+		return -ERANGE;
+	} else {
+		aspeed_sdc_configure_phase(sdhci->parent, &sdhci->phase->out,
+					   ophase, true);
+		dev_info(dev, "Output phase relationship: %u", ophase);
+	}
+
+	return 0;
+}
+
 static int aspeed_sdhci_probe(struct platform_device *pdev)
 {
+	const struct aspeed_sdhci_phase *phase;
 	struct sdhci_pltfm_host *pltfm_host;
 	struct aspeed_sdhci *dev;
 	struct sdhci_host *host;
@@ -181,7 +271,10 @@ static int aspeed_sdhci_probe(struct platform_device *pdev)
 		return -EINVAL;
 
 	dev_info(&pdev->dev, "Configuring for slot %d\n", slot);
-	dev->width_mask = !slot ? ASPEED_SDC_S0MMC8 : ASPEED_SDC_S1MMC8;
+	dev->width_mask = !slot ? ASPEED_SDC_S0_MMC8 : ASPEED_SDC_S1_MMC8;
+	phase = of_device_get_match_data(&pdev->dev);
+	if (phase)
+		dev->phase = &phase[slot];
 
 	sdhci_get_of_property(pdev);
 
@@ -195,6 +288,10 @@ static int aspeed_sdhci_probe(struct platform_device *pdev)
 		goto err_pltfm_free;
 	}
 
+	ret = aspeed_sdhci_configure_of(pdev, dev);
+	if (ret)
+		goto err_sdhci_add;
+
 	ret = mmc_of_parse(host->mmc);
 	if (ret)
 		goto err_sdhci_add;
@@ -230,10 +327,40 @@ static int aspeed_sdhci_remove(struct platform_device *pdev)
 	return 0;
 }
 
+static const struct aspeed_sdhci_phase ast2600_sdhci_phase[] = {
+	/* SDHCI/Slot 0 */
+	[0] = {
+		.in = {
+			.value_mask = ASPEED_SDC_S0_PHASE_IN,
+			.enable_mask = ASPEED_SDC_S0_PHASE_IN_EN,
+			.enable_value = 1,
+		},
+		.out = {
+			.value_mask = ASPEED_SDC_S0_PHASE_OUT,
+			.enable_mask = ASPEED_SDC_S0_PHASE_OUT_EN,
+			.enable_value = 3,
+		},
+	},
+	/* SDHCI/Slot 1 */
+	[1] = {
+		.in = {
+			.value_mask = ASPEED_SDC_S1_PHASE_IN,
+			.enable_mask = ASPEED_SDC_S1_PHASE_IN_EN,
+			.enable_value = 1,
+		},
+		.out = {
+			.value_mask = ASPEED_SDC_S1_PHASE_OUT,
+			.enable_mask = ASPEED_SDC_S1_PHASE_OUT_EN,
+			.enable_value = 3,
+		},
+	},
+};
+
+/* If supported, phase adjustment fields are stored in the data pointer */
 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_phase },
 	{ }
 };
 
-- 
2.25.1

[PATCH 2/3] mmc: sdhci-of-aspeed: Expose data sample phase delay tuning

[Andrew Jeffery]

Allow sample phase adjustment to deal with layout or tolerance issues.

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

diff --git a/drivers/mmc/host/sdhci-of-aspeed.c b/drivers/mmc/host/sdhci-of-aspeed.c
index 4f008ba3280e..641accbfcde4 100644
--- a/drivers/mmc/host/sdhci-of-aspeed.c
+++ b/drivers/mmc/host/sdhci-of-aspeed.c
@@ -16,9 +16,18 @@
 
 #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)
 
 struct aspeed_sdc {
 	struct clk *clk;
@@ -28,9 +37,21 @@ struct aspeed_sdc {
 	void __iomem *regs;
 };
 
+struct aspeed_sdhci_phase_desc {
+	u32 value_mask;
+	u32 enable_mask;
+	u8 enable_value;
+};
+
+struct aspeed_sdhci_phase {
+	struct aspeed_sdhci_phase_desc in;
+	struct aspeed_sdhci_phase_desc out;
+};
+
 struct aspeed_sdhci {
 	struct aspeed_sdc *parent;
 	u32 width_mask;
+	const struct aspeed_sdhci_phase *phase;
 };
 
 static void aspeed_sdc_configure_8bit_mode(struct aspeed_sdc *sdc,
@@ -50,6 +71,25 @@ static void aspeed_sdc_configure_8bit_mode(struct aspeed_sdc *sdc,
 	spin_unlock(&sdc->lock);
 }
 
+static void
+aspeed_sdc_configure_phase(struct aspeed_sdc *sdc,
+			   const struct aspeed_sdhci_phase_desc *phase,
+			   uint8_t value, bool enable)
+{
+	u32 reg;
+
+	spin_lock(&sdc->lock);
+	reg = readl(sdc->regs + ASPEED_SDC_PHASE);
+	reg &= ~phase->enable_mask;
+	if (enable) {
+		reg &= ~phase->value_mask;
+		reg |= value << __ffs(phase->value_mask);
+		reg |= phase->enable_value << __ffs(phase->enable_mask);
+	}
+	writel(reg, sdc->regs + ASPEED_SDC_PHASE);
+	spin_unlock(&sdc->lock);
+}
+
 static void aspeed_sdhci_set_clock(struct sdhci_host *host, unsigned int clock)
 {
 	struct sdhci_pltfm_host *pltfm_host;
@@ -155,8 +195,58 @@ static inline int aspeed_sdhci_calculate_slot(struct aspeed_sdhci *dev,
 	return (delta / 0x100) - 1;
 }
 
+static int aspeed_sdhci_configure_of(struct platform_device *pdev,
+				     struct aspeed_sdhci *sdhci)
+{
+	u32 iphase, ophase;
+	struct device_node *np;
+	struct device *dev;
+	int ret;
+
+	if (!sdhci->phase)
+		return 0;
+
+	dev = &pdev->dev;
+	np = dev->of_node;
+
+	ret = of_property_read_u32(np, "aspeed,input-phase", &iphase);
+	if (ret < 0) {
+		aspeed_sdc_configure_phase(sdhci->parent, &sdhci->phase->in, 0,
+					   false);
+		dev_dbg(dev, "Input phase configuration disabled");
+	} else if (iphase >= (1 << 5)) {
+		dev_err(dev,
+			"Input phase value exceeds field range (5 bits): %u",
+			iphase);
+		return -ERANGE;
+	} else {
+		aspeed_sdc_configure_phase(sdhci->parent, &sdhci->phase->in,
+					   iphase, true);
+		dev_info(dev, "Input phase relationship: %u", iphase);
+	}
+
+	ret = of_property_read_u32(np, "aspeed,output-phase", &ophase);
+	if (ret < 0) {
+		aspeed_sdc_configure_phase(sdhci->parent, &sdhci->phase->out, 0,
+					   false);
+		dev_dbg(dev, "Output phase configuration disabled");
+	} else if (ophase >= (1 << 5)) {
+		dev_err(dev,
+			"Output phase value exceeds field range (5 bits): %u",
+			iphase);
+		return -ERANGE;
+	} else {
+		aspeed_sdc_configure_phase(sdhci->parent, &sdhci->phase->out,
+					   ophase, true);
+		dev_info(dev, "Output phase relationship: %u", ophase);
+	}
+
+	return 0;
+}
+
 static int aspeed_sdhci_probe(struct platform_device *pdev)
 {
+	const struct aspeed_sdhci_phase *phase;
 	struct sdhci_pltfm_host *pltfm_host;
 	struct aspeed_sdhci *dev;
 	struct sdhci_host *host;
@@ -181,7 +271,10 @@ static int aspeed_sdhci_probe(struct platform_device *pdev)
 		return -EINVAL;
 
 	dev_info(&pdev->dev, "Configuring for slot %d\n", slot);
-	dev->width_mask = !slot ? ASPEED_SDC_S0MMC8 : ASPEED_SDC_S1MMC8;
+	dev->width_mask = !slot ? ASPEED_SDC_S0_MMC8 : ASPEED_SDC_S1_MMC8;
+	phase = of_device_get_match_data(&pdev->dev);
+	if (phase)
+		dev->phase = &phase[slot];
 
 	sdhci_get_of_property(pdev);
 
@@ -195,6 +288,10 @@ static int aspeed_sdhci_probe(struct platform_device *pdev)
 		goto err_pltfm_free;
 	}
 
+	ret = aspeed_sdhci_configure_of(pdev, dev);
+	if (ret)
+		goto err_sdhci_add;
+
 	ret = mmc_of_parse(host->mmc);
 	if (ret)
 		goto err_sdhci_add;
@@ -230,10 +327,40 @@ static int aspeed_sdhci_remove(struct platform_device *pdev)
 	return 0;
 }
 
+static const struct aspeed_sdhci_phase ast2600_sdhci_phase[] = {
+	/* SDHCI/Slot 0 */
+	[0] = {
+		.in = {
+			.value_mask = ASPEED_SDC_S0_PHASE_IN,
+			.enable_mask = ASPEED_SDC_S0_PHASE_IN_EN,
+			.enable_value = 1,
+		},
+		.out = {
+			.value_mask = ASPEED_SDC_S0_PHASE_OUT,
+			.enable_mask = ASPEED_SDC_S0_PHASE_OUT_EN,
+			.enable_value = 3,
+		},
+	},
+	/* SDHCI/Slot 1 */
+	[1] = {
+		.in = {
+			.value_mask = ASPEED_SDC_S1_PHASE_IN,
+			.enable_mask = ASPEED_SDC_S1_PHASE_IN_EN,
+			.enable_value = 1,
+		},
+		.out = {
+			.value_mask = ASPEED_SDC_S1_PHASE_OUT,
+			.enable_mask = ASPEED_SDC_S1_PHASE_OUT_EN,
+			.enable_value = 3,
+		},
+	},
+};
+
+/* If supported, phase adjustment fields are stored in the data pointer */
 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_phase },
 	{ }
 };
 
-- 
2.25.1


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

[PATCH 3/3] ARM: dts: tacoma: Add data sample phase delay for eMMC

[Andrew Jeffery]

Adjust the phase delay to avoid data timeout splats like the following:

[  731.368601] mmc0: Timeout waiting for hardware interrupt.
[  731.374644] mmc0: sdhci: ============ SDHCI REGISTER DUMP ===========
[  731.381828] mmc0: sdhci: Sys addr:  0x00000020 | Version:  0x00000002
[  731.389012] mmc0: sdhci: Blk size:  0x00007200 | Blk cnt:  0x00000020
[  731.396194] mmc0: sdhci: Argument:  0x00462a18 | Trn mode: 0x0000002b
[  731.403377] mmc0: sdhci: Present:   0x01f70106 | Host ctl: 0x00000017
[  731.410559] mmc0: sdhci: Power:     0x0000000f | Blk gap:  0x00000000
[  731.417733] mmc0: sdhci: Wake-up:   0x00000000 | Clock:    0x00000107
[  731.424915] mmc0: sdhci: Timeout:   0x0000000e | Int stat: 0x00000000
[  731.432098] mmc0: sdhci: Int enab:  0x03ff008b | Sig enab: 0x03ff008b
[  731.439282] mmc0: sdhci: ACmd stat: 0x00000000 | Slot int: 0x00000000
[  731.446464] mmc0: sdhci: Caps:      0x01f80080 | Caps_1:   0x00000007
[  731.453647] mmc0: sdhci: Cmd:       0x0000193a | Max curr: 0x001f0f08
[  731.460829] mmc0: sdhci: Resp[0]:   0x00000900 | Resp[1]:  0xffffffff
[  731.468013] mmc0: sdhci: Resp[2]:   0x320f5913 | Resp[3]:  0x00000900
[  731.475195] mmc0: sdhci: Host ctl2: 0x0000008b
[  731.480139] mmc0: sdhci: ADMA Err:  0x00000000 | ADMA Ptr: 0xbe040200
[  731.487321] mmc0: sdhci: ============================================

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

diff --git a/arch/arm/boot/dts/aspeed-bmc-opp-tacoma.dts b/arch/arm/boot/dts/aspeed-bmc-opp-tacoma.dts
index 5f4ee67ac787..94ec301ceb73 100644
--- a/arch/arm/boot/dts/aspeed-bmc-opp-tacoma.dts
+++ b/arch/arm/boot/dts/aspeed-bmc-opp-tacoma.dts
@@ -179,6 +179,8 @@ &emmc_controller {
 
 &emmc {
 	status = "okay";
+	aspeed,input-phase = <0x7>;
+	aspeed,output-phase = <0x1f>;
 };
 
 &fsim0 {
-- 
2.25.1

[PATCH 3/3] ARM: dts: tacoma: Add data sample phase delay for eMMC

[Andrew Jeffery]

Adjust the phase delay to avoid data timeout splats like the following:

[  731.368601] mmc0: Timeout waiting for hardware interrupt.
[  731.374644] mmc0: sdhci: ============ SDHCI REGISTER DUMP ===========
[  731.381828] mmc0: sdhci: Sys addr:  0x00000020 | Version:  0x00000002
[  731.389012] mmc0: sdhci: Blk size:  0x00007200 | Blk cnt:  0x00000020
[  731.396194] mmc0: sdhci: Argument:  0x00462a18 | Trn mode: 0x0000002b
[  731.403377] mmc0: sdhci: Present:   0x01f70106 | Host ctl: 0x00000017
[  731.410559] mmc0: sdhci: Power:     0x0000000f | Blk gap:  0x00000000
[  731.417733] mmc0: sdhci: Wake-up:   0x00000000 | Clock:    0x00000107
[  731.424915] mmc0: sdhci: Timeout:   0x0000000e | Int stat: 0x00000000
[  731.432098] mmc0: sdhci: Int enab:  0x03ff008b | Sig enab: 0x03ff008b
[  731.439282] mmc0: sdhci: ACmd stat: 0x00000000 | Slot int: 0x00000000
[  731.446464] mmc0: sdhci: Caps:      0x01f80080 | Caps_1:   0x00000007
[  731.453647] mmc0: sdhci: Cmd:       0x0000193a | Max curr: 0x001f0f08
[  731.460829] mmc0: sdhci: Resp[0]:   0x00000900 | Resp[1]:  0xffffffff
[  731.468013] mmc0: sdhci: Resp[2]:   0x320f5913 | Resp[3]:  0x00000900
[  731.475195] mmc0: sdhci: Host ctl2: 0x0000008b
[  731.480139] mmc0: sdhci: ADMA Err:  0x00000000 | ADMA Ptr: 0xbe040200
[  731.487321] mmc0: sdhci: ============================================

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

diff --git a/arch/arm/boot/dts/aspeed-bmc-opp-tacoma.dts b/arch/arm/boot/dts/aspeed-bmc-opp-tacoma.dts
index 5f4ee67ac787..94ec301ceb73 100644
--- a/arch/arm/boot/dts/aspeed-bmc-opp-tacoma.dts
+++ b/arch/arm/boot/dts/aspeed-bmc-opp-tacoma.dts
@@ -179,6 +179,8 @@ &emmc_controller {
 
 &emmc {
 	status = "okay";
+	aspeed,input-phase = <0x7>;
+	aspeed,output-phase = <0x1f>;
 };
 
 &fsim0 {
-- 
2.25.1


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

Re: [PATCH 2/3] mmc: sdhci-of-aspeed: Expose data sample phase delay tuning

[Joel Stanley]

On Thu, 10 Sep 2020 at 10:55, Andrew Jeffery <andrew@aj.id.au> wrote:
>
> Allow sample phase adjustment to deal with layout or tolerance issues.
>
> Signed-off-by: Andrew Jeffery <andrew@aj.id.au>
> ---
>  drivers/mmc/host/sdhci-of-aspeed.c | 137 +++++++++++++++++++++++++++--
>  1 file changed, 132 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/mmc/host/sdhci-of-aspeed.c b/drivers/mmc/host/sdhci-of-aspeed.c
> index 4f008ba3280e..641accbfcde4 100644
> --- a/drivers/mmc/host/sdhci-of-aspeed.c
> +++ b/drivers/mmc/host/sdhci-of-aspeed.c
> @@ -16,9 +16,18 @@
>
>  #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)
>
>  struct aspeed_sdc {
>         struct clk *clk;
> @@ -28,9 +37,21 @@ struct aspeed_sdc {
>         void __iomem *regs;
>  };
>
> +struct aspeed_sdhci_phase_desc {
> +       u32 value_mask;
> +       u32 enable_mask;
> +       u8 enable_value;
> +};
> +
> +struct aspeed_sdhci_phase {
> +       struct aspeed_sdhci_phase_desc in;
> +       struct aspeed_sdhci_phase_desc out;
> +};
> +
>  struct aspeed_sdhci {
>         struct aspeed_sdc *parent;
>         u32 width_mask;
> +       const struct aspeed_sdhci_phase *phase;
>  };
>
>  static void aspeed_sdc_configure_8bit_mode(struct aspeed_sdc *sdc,
> @@ -50,6 +71,25 @@ static void aspeed_sdc_configure_8bit_mode(struct aspeed_sdc *sdc,
>         spin_unlock(&sdc->lock);
>  }
>
> +static void
> +aspeed_sdc_configure_phase(struct aspeed_sdc *sdc,
> +                          const struct aspeed_sdhci_phase_desc *phase,
> +                          uint8_t value, bool enable)
> +{
> +       u32 reg;
> +
> +       spin_lock(&sdc->lock);

What is the lock protecting against?

We call this in the ->probe, so there should be no concurrent access going on.


> +       reg = readl(sdc->regs + ASPEED_SDC_PHASE);
> +       reg &= ~phase->enable_mask;
> +       if (enable) {
> +               reg &= ~phase->value_mask;
> +               reg |= value << __ffs(phase->value_mask);
> +               reg |= phase->enable_value << __ffs(phase->enable_mask);
> +       }
> +       writel(reg, sdc->regs + ASPEED_SDC_PHASE);
> +       spin_unlock(&sdc->lock);
> +}
> +
>  static void aspeed_sdhci_set_clock(struct sdhci_host *host, unsigned int clock)
>  {
>         struct sdhci_pltfm_host *pltfm_host;
> @@ -155,8 +195,58 @@ static inline int aspeed_sdhci_calculate_slot(struct aspeed_sdhci *dev,
>         return (delta / 0x100) - 1;
>  }
>
> +static int aspeed_sdhci_configure_of(struct platform_device *pdev,
> +                                    struct aspeed_sdhci *sdhci)
> +{
> +       u32 iphase, ophase;
> +       struct device_node *np;
> +       struct device *dev;
> +       int ret;
> +
> +       if (!sdhci->phase)
> +               return 0;
> +
> +       dev = &pdev->dev;
> +       np = dev->of_node;
> +
> +       ret = of_property_read_u32(np, "aspeed,input-phase", &iphase);
> +       if (ret < 0) {
> +               aspeed_sdc_configure_phase(sdhci->parent, &sdhci->phase->in, 0,
> +                                          false);

Will this clear any value that eg. u-boot writes?

The register should be left alone if the kernel doesn't have a
configuration of it's own, otherwise we may end up breaking an
otherwise working system.

> +               dev_dbg(dev, "Input phase configuration disabled");
> +       } else if (iphase >= (1 << 5)) {
> +               dev_err(dev,
> +                       "Input phase value exceeds field range (5 bits): %u",
> +                       iphase);
> +               return -ERANGE;
> +       } else {
> +               aspeed_sdc_configure_phase(sdhci->parent, &sdhci->phase->in,
> +                                          iphase, true);
> +               dev_info(dev, "Input phase relationship: %u", iphase);

Make theis _dbg, on a normal boot we don't need this chatter in the logs.

The same comments apply for the output.

> +       }
> +
> +       ret = of_property_read_u32(np, "aspeed,output-phase", &ophase);
> +       if (ret < 0) {
> +               aspeed_sdc_configure_phase(sdhci->parent, &sdhci->phase->out, 0,
> +                                          false);
> +               dev_dbg(dev, "Output phase configuration disabled");
> +       } else if (ophase >= (1 << 5)) {
> +               dev_err(dev,
> +                       "Output phase value exceeds field range (5 bits): %u",
> +                       iphase);
> +               return -ERANGE;

This will cause the driver to fail to probe. I think skipping setting
of the phase is a better option.


> +       } else {
> +               aspeed_sdc_configure_phase(sdhci->parent, &sdhci->phase->out,
> +                                          ophase, true);
> +               dev_info(dev, "Output phase relationship: %u", ophase);
> +       }
> +
> +       return 0;
> +}
> +
>  static int aspeed_sdhci_probe(struct platform_device *pdev)
>  {
> +       const struct aspeed_sdhci_phase *phase;
>         struct sdhci_pltfm_host *pltfm_host;
>         struct aspeed_sdhci *dev;
>         struct sdhci_host *host;
> @@ -181,7 +271,10 @@ static int aspeed_sdhci_probe(struct platform_device *pdev)
>                 return -EINVAL;
>
>         dev_info(&pdev->dev, "Configuring for slot %d\n", slot);
> -       dev->width_mask = !slot ? ASPEED_SDC_S0MMC8 : ASPEED_SDC_S1MMC8;
> +       dev->width_mask = !slot ? ASPEED_SDC_S0_MMC8 : ASPEED_SDC_S1_MMC8;
> +       phase = of_device_get_match_data(&pdev->dev);
> +       if (phase)
> +               dev->phase = &phase[slot];
>
>         sdhci_get_of_property(pdev);
>
> @@ -195,6 +288,10 @@ static int aspeed_sdhci_probe(struct platform_device *pdev)
>                 goto err_pltfm_free;
>         }
>
> +       ret = aspeed_sdhci_configure_of(pdev, dev);
> +       if (ret)
> +               goto err_sdhci_add;
> +
>         ret = mmc_of_parse(host->mmc);
>         if (ret)
>                 goto err_sdhci_add;
> @@ -230,10 +327,40 @@ static int aspeed_sdhci_remove(struct platform_device *pdev)
>         return 0;
>  }
>
> +static const struct aspeed_sdhci_phase ast2600_sdhci_phase[] = {
> +       /* SDHCI/Slot 0 */
> +       [0] = {
> +               .in = {
> +                       .value_mask = ASPEED_SDC_S0_PHASE_IN,
> +                       .enable_mask = ASPEED_SDC_S0_PHASE_IN_EN,
> +                       .enable_value = 1,
> +               },
> +               .out = {
> +                       .value_mask = ASPEED_SDC_S0_PHASE_OUT,
> +                       .enable_mask = ASPEED_SDC_S0_PHASE_OUT_EN,
> +                       .enable_value = 3,
> +               },
> +       },
> +       /* SDHCI/Slot 1 */
> +       [1] = {
> +               .in = {
> +                       .value_mask = ASPEED_SDC_S1_PHASE_IN,
> +                       .enable_mask = ASPEED_SDC_S1_PHASE_IN_EN,
> +                       .enable_value = 1,
> +               },
> +               .out = {
> +                       .value_mask = ASPEED_SDC_S1_PHASE_OUT,
> +                       .enable_mask = ASPEED_SDC_S1_PHASE_OUT_EN,

Is there any value in splitting the input and output phase values
up? (instead of taking the property from the device tree and putting
it in the hardware).

> +                       .enable_value = 3,
> +               },
> +       },
> +};
> +
> +/* If supported, phase adjustment fields are stored in the data pointer */
>  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_phase },
>         { }
>  };
>
> --
> 2.25.1
>

Re: [PATCH 2/3] mmc: sdhci-of-aspeed: Expose data sample phase delay tuning

[Joel Stanley]

On Thu, 10 Sep 2020 at 10:55, Andrew Jeffery <andrew@aj.id.au> wrote:
>
> Allow sample phase adjustment to deal with layout or tolerance issues.
>
> Signed-off-by: Andrew Jeffery <andrew@aj.id.au>
> ---
>  drivers/mmc/host/sdhci-of-aspeed.c | 137 +++++++++++++++++++++++++++--
>  1 file changed, 132 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/mmc/host/sdhci-of-aspeed.c b/drivers/mmc/host/sdhci-of-aspeed.c
> index 4f008ba3280e..641accbfcde4 100644
> --- a/drivers/mmc/host/sdhci-of-aspeed.c
> +++ b/drivers/mmc/host/sdhci-of-aspeed.c
> @@ -16,9 +16,18 @@
>
>  #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)
>
>  struct aspeed_sdc {
>         struct clk *clk;
> @@ -28,9 +37,21 @@ struct aspeed_sdc {
>         void __iomem *regs;
>  };
>
> +struct aspeed_sdhci_phase_desc {
> +       u32 value_mask;
> +       u32 enable_mask;
> +       u8 enable_value;
> +};
> +
> +struct aspeed_sdhci_phase {
> +       struct aspeed_sdhci_phase_desc in;
> +       struct aspeed_sdhci_phase_desc out;
> +};
> +
>  struct aspeed_sdhci {
>         struct aspeed_sdc *parent;
>         u32 width_mask;
> +       const struct aspeed_sdhci_phase *phase;
>  };
>
>  static void aspeed_sdc_configure_8bit_mode(struct aspeed_sdc *sdc,
> @@ -50,6 +71,25 @@ static void aspeed_sdc_configure_8bit_mode(struct aspeed_sdc *sdc,
>         spin_unlock(&sdc->lock);
>  }
>
> +static void
> +aspeed_sdc_configure_phase(struct aspeed_sdc *sdc,
> +                          const struct aspeed_sdhci_phase_desc *phase,
> +                          uint8_t value, bool enable)
> +{
> +       u32 reg;
> +
> +       spin_lock(&sdc->lock);

What is the lock protecting against?

We call this in the ->probe, so there should be no concurrent access going on.


> +       reg = readl(sdc->regs + ASPEED_SDC_PHASE);
> +       reg &= ~phase->enable_mask;
> +       if (enable) {
> +               reg &= ~phase->value_mask;
> +               reg |= value << __ffs(phase->value_mask);
> +               reg |= phase->enable_value << __ffs(phase->enable_mask);
> +       }
> +       writel(reg, sdc->regs + ASPEED_SDC_PHASE);
> +       spin_unlock(&sdc->lock);
> +}
> +
>  static void aspeed_sdhci_set_clock(struct sdhci_host *host, unsigned int clock)
>  {
>         struct sdhci_pltfm_host *pltfm_host;
> @@ -155,8 +195,58 @@ static inline int aspeed_sdhci_calculate_slot(struct aspeed_sdhci *dev,
>         return (delta / 0x100) - 1;
>  }
>
> +static int aspeed_sdhci_configure_of(struct platform_device *pdev,
> +                                    struct aspeed_sdhci *sdhci)
> +{
> +       u32 iphase, ophase;
> +       struct device_node *np;
> +       struct device *dev;
> +       int ret;
> +
> +       if (!sdhci->phase)
> +               return 0;
> +
> +       dev = &pdev->dev;
> +       np = dev->of_node;
> +
> +       ret = of_property_read_u32(np, "aspeed,input-phase", &iphase);
> +       if (ret < 0) {
> +               aspeed_sdc_configure_phase(sdhci->parent, &sdhci->phase->in, 0,
> +                                          false);

Will this clear any value that eg. u-boot writes?

The register should be left alone if the kernel doesn't have a
configuration of it's own, otherwise we may end up breaking an
otherwise working system.

> +               dev_dbg(dev, "Input phase configuration disabled");
> +       } else if (iphase >= (1 << 5)) {
> +               dev_err(dev,
> +                       "Input phase value exceeds field range (5 bits): %u",
> +                       iphase);
> +               return -ERANGE;
> +       } else {
> +               aspeed_sdc_configure_phase(sdhci->parent, &sdhci->phase->in,
> +                                          iphase, true);
> +               dev_info(dev, "Input phase relationship: %u", iphase);

Make theis _dbg, on a normal boot we don't need this chatter in the logs.

The same comments apply for the output.

> +       }
> +
> +       ret = of_property_read_u32(np, "aspeed,output-phase", &ophase);
> +       if (ret < 0) {
> +               aspeed_sdc_configure_phase(sdhci->parent, &sdhci->phase->out, 0,
> +                                          false);
> +               dev_dbg(dev, "Output phase configuration disabled");
> +       } else if (ophase >= (1 << 5)) {
> +               dev_err(dev,
> +                       "Output phase value exceeds field range (5 bits): %u",
> +                       iphase);
> +               return -ERANGE;

This will cause the driver to fail to probe. I think skipping setting
of the phase is a better option.


> +       } else {
> +               aspeed_sdc_configure_phase(sdhci->parent, &sdhci->phase->out,
> +                                          ophase, true);
> +               dev_info(dev, "Output phase relationship: %u", ophase);
> +       }
> +
> +       return 0;
> +}
> +
>  static int aspeed_sdhci_probe(struct platform_device *pdev)
>  {
> +       const struct aspeed_sdhci_phase *phase;
>         struct sdhci_pltfm_host *pltfm_host;
>         struct aspeed_sdhci *dev;
>         struct sdhci_host *host;
> @@ -181,7 +271,10 @@ static int aspeed_sdhci_probe(struct platform_device *pdev)
>                 return -EINVAL;
>
>         dev_info(&pdev->dev, "Configuring for slot %d\n", slot);
> -       dev->width_mask = !slot ? ASPEED_SDC_S0MMC8 : ASPEED_SDC_S1MMC8;
> +       dev->width_mask = !slot ? ASPEED_SDC_S0_MMC8 : ASPEED_SDC_S1_MMC8;
> +       phase = of_device_get_match_data(&pdev->dev);
> +       if (phase)
> +               dev->phase = &phase[slot];
>
>         sdhci_get_of_property(pdev);
>
> @@ -195,6 +288,10 @@ static int aspeed_sdhci_probe(struct platform_device *pdev)
>                 goto err_pltfm_free;
>         }
>
> +       ret = aspeed_sdhci_configure_of(pdev, dev);
> +       if (ret)
> +               goto err_sdhci_add;
> +
>         ret = mmc_of_parse(host->mmc);
>         if (ret)
>                 goto err_sdhci_add;
> @@ -230,10 +327,40 @@ static int aspeed_sdhci_remove(struct platform_device *pdev)
>         return 0;
>  }
>
> +static const struct aspeed_sdhci_phase ast2600_sdhci_phase[] = {
> +       /* SDHCI/Slot 0 */
> +       [0] = {
> +               .in = {
> +                       .value_mask = ASPEED_SDC_S0_PHASE_IN,
> +                       .enable_mask = ASPEED_SDC_S0_PHASE_IN_EN,
> +                       .enable_value = 1,
> +               },
> +               .out = {
> +                       .value_mask = ASPEED_SDC_S0_PHASE_OUT,
> +                       .enable_mask = ASPEED_SDC_S0_PHASE_OUT_EN,
> +                       .enable_value = 3,
> +               },
> +       },
> +       /* SDHCI/Slot 1 */
> +       [1] = {
> +               .in = {
> +                       .value_mask = ASPEED_SDC_S1_PHASE_IN,
> +                       .enable_mask = ASPEED_SDC_S1_PHASE_IN_EN,
> +                       .enable_value = 1,
> +               },
> +               .out = {
> +                       .value_mask = ASPEED_SDC_S1_PHASE_OUT,
> +                       .enable_mask = ASPEED_SDC_S1_PHASE_OUT_EN,

Is there any value in splitting the input and output phase values
up? (instead of taking the property from the device tree and putting
it in the hardware).

> +                       .enable_value = 3,
> +               },
> +       },
> +};
> +
> +/* If supported, phase adjustment fields are stored in the data pointer */
>  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_phase },
>         { }
>  };
>
> --
> 2.25.1
>

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

Re: [PATCH 3/3] ARM: dts: tacoma: Add data sample phase delay for eMMC

[Joel Stanley]

On Thu, 10 Sep 2020 at 10:55, Andrew Jeffery <andrew@aj.id.au> wrote:
>
> Adjust the phase delay to avoid data timeout splats like the following:
>
> [  731.368601] mmc0: Timeout waiting for hardware interrupt.
> [  731.374644] mmc0: sdhci: ============ SDHCI REGISTER DUMP ===========
> [  731.381828] mmc0: sdhci: Sys addr:  0x00000020 | Version:  0x00000002
> [  731.389012] mmc0: sdhci: Blk size:  0x00007200 | Blk cnt:  0x00000020
> [  731.396194] mmc0: sdhci: Argument:  0x00462a18 | Trn mode: 0x0000002b
> [  731.403377] mmc0: sdhci: Present:   0x01f70106 | Host ctl: 0x00000017
> [  731.410559] mmc0: sdhci: Power:     0x0000000f | Blk gap:  0x00000000
> [  731.417733] mmc0: sdhci: Wake-up:   0x00000000 | Clock:    0x00000107
> [  731.424915] mmc0: sdhci: Timeout:   0x0000000e | Int stat: 0x00000000
> [  731.432098] mmc0: sdhci: Int enab:  0x03ff008b | Sig enab: 0x03ff008b
> [  731.439282] mmc0: sdhci: ACmd stat: 0x00000000 | Slot int: 0x00000000
> [  731.446464] mmc0: sdhci: Caps:      0x01f80080 | Caps_1:   0x00000007
> [  731.453647] mmc0: sdhci: Cmd:       0x0000193a | Max curr: 0x001f0f08
> [  731.460829] mmc0: sdhci: Resp[0]:   0x00000900 | Resp[1]:  0xffffffff
> [  731.468013] mmc0: sdhci: Resp[2]:   0x320f5913 | Resp[3]:  0x00000900
> [  731.475195] mmc0: sdhci: Host ctl2: 0x0000008b
> [  731.480139] mmc0: sdhci: ADMA Err:  0x00000000 | ADMA Ptr: 0xbe040200
> [  731.487321] mmc0: sdhci: ============================================
>
> Signed-off-by: Andrew Jeffery <andrew@aj.id.au>

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

> ---
>  arch/arm/boot/dts/aspeed-bmc-opp-tacoma.dts | 2 ++
>  1 file changed, 2 insertions(+)
>
> diff --git a/arch/arm/boot/dts/aspeed-bmc-opp-tacoma.dts b/arch/arm/boot/dts/aspeed-bmc-opp-tacoma.dts
> index 5f4ee67ac787..94ec301ceb73 100644
> --- a/arch/arm/boot/dts/aspeed-bmc-opp-tacoma.dts
> +++ b/arch/arm/boot/dts/aspeed-bmc-opp-tacoma.dts
> @@ -179,6 +179,8 @@ &emmc_controller {
>
>  &emmc {
>         status = "okay";
> +       aspeed,input-phase = <0x7>;
> +       aspeed,output-phase = <0x1f>;
>  };
>
>  &fsim0 {
> --
> 2.25.1
>

Re: [PATCH 3/3] ARM: dts: tacoma: Add data sample phase delay for eMMC

[Joel Stanley]

On Thu, 10 Sep 2020 at 10:55, Andrew Jeffery <andrew@aj.id.au> wrote:
>
> Adjust the phase delay to avoid data timeout splats like the following:
>
> [  731.368601] mmc0: Timeout waiting for hardware interrupt.
> [  731.374644] mmc0: sdhci: ============ SDHCI REGISTER DUMP ===========
> [  731.381828] mmc0: sdhci: Sys addr:  0x00000020 | Version:  0x00000002
> [  731.389012] mmc0: sdhci: Blk size:  0x00007200 | Blk cnt:  0x00000020
> [  731.396194] mmc0: sdhci: Argument:  0x00462a18 | Trn mode: 0x0000002b
> [  731.403377] mmc0: sdhci: Present:   0x01f70106 | Host ctl: 0x00000017
> [  731.410559] mmc0: sdhci: Power:     0x0000000f | Blk gap:  0x00000000
> [  731.417733] mmc0: sdhci: Wake-up:   0x00000000 | Clock:    0x00000107
> [  731.424915] mmc0: sdhci: Timeout:   0x0000000e | Int stat: 0x00000000
> [  731.432098] mmc0: sdhci: Int enab:  0x03ff008b | Sig enab: 0x03ff008b
> [  731.439282] mmc0: sdhci: ACmd stat: 0x00000000 | Slot int: 0x00000000
> [  731.446464] mmc0: sdhci: Caps:      0x01f80080 | Caps_1:   0x00000007
> [  731.453647] mmc0: sdhci: Cmd:       0x0000193a | Max curr: 0x001f0f08
> [  731.460829] mmc0: sdhci: Resp[0]:   0x00000900 | Resp[1]:  0xffffffff
> [  731.468013] mmc0: sdhci: Resp[2]:   0x320f5913 | Resp[3]:  0x00000900
> [  731.475195] mmc0: sdhci: Host ctl2: 0x0000008b
> [  731.480139] mmc0: sdhci: ADMA Err:  0x00000000 | ADMA Ptr: 0xbe040200
> [  731.487321] mmc0: sdhci: ============================================
>
> Signed-off-by: Andrew Jeffery <andrew@aj.id.au>

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

> ---
>  arch/arm/boot/dts/aspeed-bmc-opp-tacoma.dts | 2 ++
>  1 file changed, 2 insertions(+)
>
> diff --git a/arch/arm/boot/dts/aspeed-bmc-opp-tacoma.dts b/arch/arm/boot/dts/aspeed-bmc-opp-tacoma.dts
> index 5f4ee67ac787..94ec301ceb73 100644
> --- a/arch/arm/boot/dts/aspeed-bmc-opp-tacoma.dts
> +++ b/arch/arm/boot/dts/aspeed-bmc-opp-tacoma.dts
> @@ -179,6 +179,8 @@ &emmc_controller {
>
>  &emmc {
>         status = "okay";
> +       aspeed,input-phase = <0x7>;
> +       aspeed,output-phase = <0x1f>;
>  };
>
>  &fsim0 {
> --
> 2.25.1
>

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

Re: [PATCH 2/3] mmc: sdhci-of-aspeed: Expose data sample phase delay tuning

[Andrew Jeffery]

On Fri, 11 Sep 2020, at 11:32, Joel Stanley wrote:
> On Thu, 10 Sep 2020 at 10:55, Andrew Jeffery <andrew@aj.id.au> wrote:
> >
> > Allow sample phase adjustment to deal with layout or tolerance issues.
> >
> > Signed-off-by: Andrew Jeffery <andrew@aj.id.au>
> > ---
> >  drivers/mmc/host/sdhci-of-aspeed.c | 137 +++++++++++++++++++++++++++--
> >  1 file changed, 132 insertions(+), 5 deletions(-)
> >
> > diff --git a/drivers/mmc/host/sdhci-of-aspeed.c b/drivers/mmc/host/sdhci-of-aspeed.c
> > index 4f008ba3280e..641accbfcde4 100644
> > --- a/drivers/mmc/host/sdhci-of-aspeed.c
> > +++ b/drivers/mmc/host/sdhci-of-aspeed.c
> > @@ -16,9 +16,18 @@
> >
> >  #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)
> >
> >  struct aspeed_sdc {
> >         struct clk *clk;
> > @@ -28,9 +37,21 @@ struct aspeed_sdc {
> >         void __iomem *regs;
> >  };
> >
> > +struct aspeed_sdhci_phase_desc {
> > +       u32 value_mask;
> > +       u32 enable_mask;
> > +       u8 enable_value;
> > +};
> > +
> > +struct aspeed_sdhci_phase {
> > +       struct aspeed_sdhci_phase_desc in;
> > +       struct aspeed_sdhci_phase_desc out;
> > +};
> > +
> >  struct aspeed_sdhci {
> >         struct aspeed_sdc *parent;
> >         u32 width_mask;
> > +       const struct aspeed_sdhci_phase *phase;
> >  };
> >
> >  static void aspeed_sdc_configure_8bit_mode(struct aspeed_sdc *sdc,
> > @@ -50,6 +71,25 @@ static void aspeed_sdc_configure_8bit_mode(struct aspeed_sdc *sdc,
> >         spin_unlock(&sdc->lock);
> >  }
> >
> > +static void
> > +aspeed_sdc_configure_phase(struct aspeed_sdc *sdc,
> > +                          const struct aspeed_sdhci_phase_desc *phase,
> > +                          uint8_t value, bool enable)
> > +{
> > +       u32 reg;
> > +
> > +       spin_lock(&sdc->lock);
> 
> What is the lock protecting against?
> 
> We call this in the ->probe, so there should be no concurrent access going on.

Because the register is in the "global" part of the SD/MMC controller address 
space (it's not part of the SDHCI), and there are multiple slots that may have 
a driver probed concurrently.

> 
> 
> > +       reg = readl(sdc->regs + ASPEED_SDC_PHASE);
> > +       reg &= ~phase->enable_mask;
> > +       if (enable) {
> > +               reg &= ~phase->value_mask;
> > +               reg |= value << __ffs(phase->value_mask);
> > +               reg |= phase->enable_value << __ffs(phase->enable_mask);
> > +       }
> > +       writel(reg, sdc->regs + ASPEED_SDC_PHASE);
> > +       spin_unlock(&sdc->lock);
> > +}
> > +
> >  static void aspeed_sdhci_set_clock(struct sdhci_host *host, unsigned int clock)
> >  {
> >         struct sdhci_pltfm_host *pltfm_host;
> > @@ -155,8 +195,58 @@ static inline int aspeed_sdhci_calculate_slot(struct aspeed_sdhci *dev,
> >         return (delta / 0x100) - 1;
> >  }
> >
> > +static int aspeed_sdhci_configure_of(struct platform_device *pdev,
> > +                                    struct aspeed_sdhci *sdhci)
> > +{
> > +       u32 iphase, ophase;
> > +       struct device_node *np;
> > +       struct device *dev;
> > +       int ret;
> > +
> > +       if (!sdhci->phase)
> > +               return 0;
> > +
> > +       dev = &pdev->dev;
> > +       np = dev->of_node;
> > +
> > +       ret = of_property_read_u32(np, "aspeed,input-phase", &iphase);
> > +       if (ret < 0) {
> > +               aspeed_sdc_configure_phase(sdhci->parent, &sdhci->phase->in, 0,
> > +                                          false);
> 
> Will this clear any value that eg. u-boot writes?

No, see the 'enable' test in aspeed_sdc_configure_phase()

> 
> The register should be left alone if the kernel doesn't have a
> configuration of it's own, otherwise we may end up breaking an
> otherwise working system.

Right, I can rework that.

> 
> > +               dev_dbg(dev, "Input phase configuration disabled");
> > +       } else if (iphase >= (1 << 5)) {
> > +               dev_err(dev,
> > +                       "Input phase value exceeds field range (5 bits): %u",
> > +                       iphase);
> > +               return -ERANGE;
> > +       } else {
> > +               aspeed_sdc_configure_phase(sdhci->parent, &sdhci->phase->in,
> > +                                          iphase, true);
> > +               dev_info(dev, "Input phase relationship: %u", iphase);
> 
> Make theis _dbg, on a normal boot we don't need this chatter in the logs.
> 
> The same comments apply for the output.

Yes, I actually meant to do this before I sent the patches but clearly forgot :)

> 
> > +       }
> > +
> > +       ret = of_property_read_u32(np, "aspeed,output-phase", &ophase);
> > +       if (ret < 0) {
> > +               aspeed_sdc_configure_phase(sdhci->parent, &sdhci->phase->out, 0,
> > +                                          false);
> > +               dev_dbg(dev, "Output phase configuration disabled");
> > +       } else if (ophase >= (1 << 5)) {
> > +               dev_err(dev,
> > +                       "Output phase value exceeds field range (5 bits): %u",
> > +                       iphase);
> > +               return -ERANGE;
> 
> This will cause the driver to fail to probe. I think skipping setting
> of the phase is a better option.

Well, maybe? If the phase is specified but invalid then chances are you'll hit 
system instability by continuing to probe the driver. I think it's better to 
fail in an obvious way, it's not as if it's incidentally incorrect.

> 
> 
> > +       } else {
> > +               aspeed_sdc_configure_phase(sdhci->parent, &sdhci->phase->out,
> > +                                          ophase, true);
> > +               dev_info(dev, "Output phase relationship: %u", ophase);
> > +       }
> > +
> > +       return 0;
> > +}
> > +
> >  static int aspeed_sdhci_probe(struct platform_device *pdev)
> >  {
> > +       const struct aspeed_sdhci_phase *phase;
> >         struct sdhci_pltfm_host *pltfm_host;
> >         struct aspeed_sdhci *dev;
> >         struct sdhci_host *host;
> > @@ -181,7 +271,10 @@ static int aspeed_sdhci_probe(struct platform_device *pdev)
> >                 return -EINVAL;
> >
> >         dev_info(&pdev->dev, "Configuring for slot %d\n", slot);
> > -       dev->width_mask = !slot ? ASPEED_SDC_S0MMC8 : ASPEED_SDC_S1MMC8;
> > +       dev->width_mask = !slot ? ASPEED_SDC_S0_MMC8 : ASPEED_SDC_S1_MMC8;
> > +       phase = of_device_get_match_data(&pdev->dev);
> > +       if (phase)
> > +               dev->phase = &phase[slot];
> >
> >         sdhci_get_of_property(pdev);
> >
> > @@ -195,6 +288,10 @@ static int aspeed_sdhci_probe(struct platform_device *pdev)
> >                 goto err_pltfm_free;
> >         }
> >
> > +       ret = aspeed_sdhci_configure_of(pdev, dev);
> > +       if (ret)
> > +               goto err_sdhci_add;
> > +
> >         ret = mmc_of_parse(host->mmc);
> >         if (ret)
> >                 goto err_sdhci_add;
> > @@ -230,10 +327,40 @@ static int aspeed_sdhci_remove(struct platform_device *pdev)
> >         return 0;
> >  }
> >
> > +static const struct aspeed_sdhci_phase ast2600_sdhci_phase[] = {
> > +       /* SDHCI/Slot 0 */
> > +       [0] = {
> > +               .in = {
> > +                       .value_mask = ASPEED_SDC_S0_PHASE_IN,
> > +                       .enable_mask = ASPEED_SDC_S0_PHASE_IN_EN,
> > +                       .enable_value = 1,
> > +               },
> > +               .out = {
> > +                       .value_mask = ASPEED_SDC_S0_PHASE_OUT,
> > +                       .enable_mask = ASPEED_SDC_S0_PHASE_OUT_EN,
> > +                       .enable_value = 3,
> > +               },
> > +       },
> > +       /* SDHCI/Slot 1 */
> > +       [1] = {
> > +               .in = {
> > +                       .value_mask = ASPEED_SDC_S1_PHASE_IN,
> > +                       .enable_mask = ASPEED_SDC_S1_PHASE_IN_EN,
> > +                       .enable_value = 1,
> > +               },
> > +               .out = {
> > +                       .value_mask = ASPEED_SDC_S1_PHASE_OUT,
> > +                       .enable_mask = ASPEED_SDC_S1_PHASE_OUT_EN,
> 
> Is there any value in splitting the input and output phase values
> up? (instead of taking the property from the device tree and putting
> it in the hardware).

One register contains both settings for both slots. We can't just blast the 
full register value for one of the slots into it. Further, the fields for the 
slots are interleaved, so it's not like we can just associate the top or bottom 
16 bits with a particular slot.

Cheers,

Andrew

Re: [PATCH 2/3] mmc: sdhci-of-aspeed: Expose data sample phase delay tuning

[Andrew Jeffery]

On Fri, 11 Sep 2020, at 11:32, Joel Stanley wrote:
> On Thu, 10 Sep 2020 at 10:55, Andrew Jeffery <andrew@aj.id.au> wrote:
> >
> > Allow sample phase adjustment to deal with layout or tolerance issues.
> >
> > Signed-off-by: Andrew Jeffery <andrew@aj.id.au>
> > ---
> >  drivers/mmc/host/sdhci-of-aspeed.c | 137 +++++++++++++++++++++++++++--
> >  1 file changed, 132 insertions(+), 5 deletions(-)
> >
> > diff --git a/drivers/mmc/host/sdhci-of-aspeed.c b/drivers/mmc/host/sdhci-of-aspeed.c
> > index 4f008ba3280e..641accbfcde4 100644
> > --- a/drivers/mmc/host/sdhci-of-aspeed.c
> > +++ b/drivers/mmc/host/sdhci-of-aspeed.c
> > @@ -16,9 +16,18 @@
> >
> >  #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)
> >
> >  struct aspeed_sdc {
> >         struct clk *clk;
> > @@ -28,9 +37,21 @@ struct aspeed_sdc {
> >         void __iomem *regs;
> >  };
> >
> > +struct aspeed_sdhci_phase_desc {
> > +       u32 value_mask;
> > +       u32 enable_mask;
> > +       u8 enable_value;
> > +};
> > +
> > +struct aspeed_sdhci_phase {
> > +       struct aspeed_sdhci_phase_desc in;
> > +       struct aspeed_sdhci_phase_desc out;
> > +};
> > +
> >  struct aspeed_sdhci {
> >         struct aspeed_sdc *parent;
> >         u32 width_mask;
> > +       const struct aspeed_sdhci_phase *phase;
> >  };
> >
> >  static void aspeed_sdc_configure_8bit_mode(struct aspeed_sdc *sdc,
> > @@ -50,6 +71,25 @@ static void aspeed_sdc_configure_8bit_mode(struct aspeed_sdc *sdc,
> >         spin_unlock(&sdc->lock);
> >  }
> >
> > +static void
> > +aspeed_sdc_configure_phase(struct aspeed_sdc *sdc,
> > +                          const struct aspeed_sdhci_phase_desc *phase,
> > +                          uint8_t value, bool enable)
> > +{
> > +       u32 reg;
> > +
> > +       spin_lock(&sdc->lock);
> 
> What is the lock protecting against?
> 
> We call this in the ->probe, so there should be no concurrent access going on.

Because the register is in the "global" part of the SD/MMC controller address 
space (it's not part of the SDHCI), and there are multiple slots that may have 
a driver probed concurrently.

> 
> 
> > +       reg = readl(sdc->regs + ASPEED_SDC_PHASE);
> > +       reg &= ~phase->enable_mask;
> > +       if (enable) {
> > +               reg &= ~phase->value_mask;
> > +               reg |= value << __ffs(phase->value_mask);
> > +               reg |= phase->enable_value << __ffs(phase->enable_mask);
> > +       }
> > +       writel(reg, sdc->regs + ASPEED_SDC_PHASE);
> > +       spin_unlock(&sdc->lock);
> > +}
> > +
> >  static void aspeed_sdhci_set_clock(struct sdhci_host *host, unsigned int clock)
> >  {
> >         struct sdhci_pltfm_host *pltfm_host;
> > @@ -155,8 +195,58 @@ static inline int aspeed_sdhci_calculate_slot(struct aspeed_sdhci *dev,
> >         return (delta / 0x100) - 1;
> >  }
> >
> > +static int aspeed_sdhci_configure_of(struct platform_device *pdev,
> > +                                    struct aspeed_sdhci *sdhci)
> > +{
> > +       u32 iphase, ophase;
> > +       struct device_node *np;
> > +       struct device *dev;
> > +       int ret;
> > +
> > +       if (!sdhci->phase)
> > +               return 0;
> > +
> > +       dev = &pdev->dev;
> > +       np = dev->of_node;
> > +
> > +       ret = of_property_read_u32(np, "aspeed,input-phase", &iphase);
> > +       if (ret < 0) {
> > +               aspeed_sdc_configure_phase(sdhci->parent, &sdhci->phase->in, 0,
> > +                                          false);
> 
> Will this clear any value that eg. u-boot writes?

No, see the 'enable' test in aspeed_sdc_configure_phase()

> 
> The register should be left alone if the kernel doesn't have a
> configuration of it's own, otherwise we may end up breaking an
> otherwise working system.

Right, I can rework that.

> 
> > +               dev_dbg(dev, "Input phase configuration disabled");
> > +       } else if (iphase >= (1 << 5)) {
> > +               dev_err(dev,
> > +                       "Input phase value exceeds field range (5 bits): %u",
> > +                       iphase);
> > +               return -ERANGE;
> > +       } else {
> > +               aspeed_sdc_configure_phase(sdhci->parent, &sdhci->phase->in,
> > +                                          iphase, true);
> > +               dev_info(dev, "Input phase relationship: %u", iphase);
> 
> Make theis _dbg, on a normal boot we don't need this chatter in the logs.
> 
> The same comments apply for the output.

Yes, I actually meant to do this before I sent the patches but clearly forgot :)

> 
> > +       }
> > +
> > +       ret = of_property_read_u32(np, "aspeed,output-phase", &ophase);
> > +       if (ret < 0) {
> > +               aspeed_sdc_configure_phase(sdhci->parent, &sdhci->phase->out, 0,
> > +                                          false);
> > +               dev_dbg(dev, "Output phase configuration disabled");
> > +       } else if (ophase >= (1 << 5)) {
> > +               dev_err(dev,
> > +                       "Output phase value exceeds field range (5 bits): %u",
> > +                       iphase);
> > +               return -ERANGE;
> 
> This will cause the driver to fail to probe. I think skipping setting
> of the phase is a better option.

Well, maybe? If the phase is specified but invalid then chances are you'll hit 
system instability by continuing to probe the driver. I think it's better to 
fail in an obvious way, it's not as if it's incidentally incorrect.

> 
> 
> > +       } else {
> > +               aspeed_sdc_configure_phase(sdhci->parent, &sdhci->phase->out,
> > +                                          ophase, true);
> > +               dev_info(dev, "Output phase relationship: %u", ophase);
> > +       }
> > +
> > +       return 0;
> > +}
> > +
> >  static int aspeed_sdhci_probe(struct platform_device *pdev)
> >  {
> > +       const struct aspeed_sdhci_phase *phase;
> >         struct sdhci_pltfm_host *pltfm_host;
> >         struct aspeed_sdhci *dev;
> >         struct sdhci_host *host;
> > @@ -181,7 +271,10 @@ static int aspeed_sdhci_probe(struct platform_device *pdev)
> >                 return -EINVAL;
> >
> >         dev_info(&pdev->dev, "Configuring for slot %d\n", slot);
> > -       dev->width_mask = !slot ? ASPEED_SDC_S0MMC8 : ASPEED_SDC_S1MMC8;
> > +       dev->width_mask = !slot ? ASPEED_SDC_S0_MMC8 : ASPEED_SDC_S1_MMC8;
> > +       phase = of_device_get_match_data(&pdev->dev);
> > +       if (phase)
> > +               dev->phase = &phase[slot];
> >
> >         sdhci_get_of_property(pdev);
> >
> > @@ -195,6 +288,10 @@ static int aspeed_sdhci_probe(struct platform_device *pdev)
> >                 goto err_pltfm_free;
> >         }
> >
> > +       ret = aspeed_sdhci_configure_of(pdev, dev);
> > +       if (ret)
> > +               goto err_sdhci_add;
> > +
> >         ret = mmc_of_parse(host->mmc);
> >         if (ret)
> >                 goto err_sdhci_add;
> > @@ -230,10 +327,40 @@ static int aspeed_sdhci_remove(struct platform_device *pdev)
> >         return 0;
> >  }
> >
> > +static const struct aspeed_sdhci_phase ast2600_sdhci_phase[] = {
> > +       /* SDHCI/Slot 0 */
> > +       [0] = {
> > +               .in = {
> > +                       .value_mask = ASPEED_SDC_S0_PHASE_IN,
> > +                       .enable_mask = ASPEED_SDC_S0_PHASE_IN_EN,
> > +                       .enable_value = 1,
> > +               },
> > +               .out = {
> > +                       .value_mask = ASPEED_SDC_S0_PHASE_OUT,
> > +                       .enable_mask = ASPEED_SDC_S0_PHASE_OUT_EN,
> > +                       .enable_value = 3,
> > +               },
> > +       },
> > +       /* SDHCI/Slot 1 */
> > +       [1] = {
> > +               .in = {
> > +                       .value_mask = ASPEED_SDC_S1_PHASE_IN,
> > +                       .enable_mask = ASPEED_SDC_S1_PHASE_IN_EN,
> > +                       .enable_value = 1,
> > +               },
> > +               .out = {
> > +                       .value_mask = ASPEED_SDC_S1_PHASE_OUT,
> > +                       .enable_mask = ASPEED_SDC_S1_PHASE_OUT_EN,
> 
> Is there any value in splitting the input and output phase values
> up? (instead of taking the property from the device tree and putting
> it in the hardware).

One register contains both settings for both slots. We can't just blast the 
full register value for one of the slots into it. Further, the fields for the 
slots are interleaved, so it's not like we can just associate the top or bottom 
16 bits with a particular slot.

Cheers,

Andrew

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

Re: [PATCH 2/3] mmc: sdhci-of-aspeed: Expose data sample phase delay tuning

[Joel Stanley]

On Fri, 11 Sep 2020 at 02:49, Andrew Jeffery <andrew@aj.id.au> wrote:
>
>
>
> On Fri, 11 Sep 2020, at 11:32, Joel Stanley wrote:
> > On Thu, 10 Sep 2020 at 10:55, Andrew Jeffery <andrew@aj.id.au> wrote:
> > >
> > > Allow sample phase adjustment to deal with layout or tolerance issues.
> > >
> > > Signed-off-by: Andrew Jeffery <andrew@aj.id.au>
> > > ---
> > >  drivers/mmc/host/sdhci-of-aspeed.c | 137 +++++++++++++++++++++++++++--
> > >  1 file changed, 132 insertions(+), 5 deletions(-)
> > >
> > > diff --git a/drivers/mmc/host/sdhci-of-aspeed.c b/drivers/mmc/host/sdhci-of-aspeed.c
> > > index 4f008ba3280e..641accbfcde4 100644
> > > --- a/drivers/mmc/host/sdhci-of-aspeed.c
> > > +++ b/drivers/mmc/host/sdhci-of-aspeed.c

> > > +static void
> > > +aspeed_sdc_configure_phase(struct aspeed_sdc *sdc,
> > > +                          const struct aspeed_sdhci_phase_desc *phase,
> > > +                          uint8_t value, bool enable)
> > > +{
> > > +       u32 reg;
> > > +
> > > +       spin_lock(&sdc->lock);
> >
> > What is the lock protecting against?
> >
> > We call this in the ->probe, so there should be no concurrent access going on.
>
> Because the register is in the "global" part of the SD/MMC controller address
> space (it's not part of the SDHCI), and there are multiple slots that may have
> a driver probed concurrently.

That points to having the property be part of the "global" device tree
node. This would simplify the code; you wouldn't need the locking
either.

>
> >
> >
> > > +       reg = readl(sdc->regs + ASPEED_SDC_PHASE);
> > > +       reg &= ~phase->enable_mask;
> > > +       if (enable) {
> > > +               reg &= ~phase->value_mask;
> > > +               reg |= value << __ffs(phase->value_mask);
> > > +               reg |= phase->enable_value << __ffs(phase->enable_mask);
> > > +       }
> > > +       writel(reg, sdc->regs + ASPEED_SDC_PHASE);
> > > +       spin_unlock(&sdc->lock);
> > > +}
> > > +
> > >  static void aspeed_sdhci_set_clock(struct sdhci_host *host, unsigned int clock)
> > >  {
> > >         struct sdhci_pltfm_host *pltfm_host;
> > > @@ -155,8 +195,58 @@ static inline int aspeed_sdhci_calculate_slot(struct aspeed_sdhci *dev,
> > >         return (delta / 0x100) - 1;
> > >  }
> > >
> > > +static int aspeed_sdhci_configure_of(struct platform_device *pdev,
> > > +                                    struct aspeed_sdhci *sdhci)
> > > +{
> > > +       u32 iphase, ophase;
> > > +       struct device_node *np;
> > > +       struct device *dev;
> > > +       int ret;
> > > +
> > > +       if (!sdhci->phase)
> > > +               return 0;
> > > +
> > > +       dev = &pdev->dev;
> > > +       np = dev->of_node;
> > > +
> > > +       ret = of_property_read_u32(np, "aspeed,input-phase", &iphase);
> > > +       if (ret < 0) {
> > > +               aspeed_sdc_configure_phase(sdhci->parent, &sdhci->phase->in, 0,
> > > +                                          false);
> >
> > Will this clear any value that eg. u-boot writes?
>
> No, see the 'enable' test in aspeed_sdc_configure_phase()

OK, so this branch will never cause any change in the register? Best
to drop it then.

>
> >
> > The register should be left alone if the kernel doesn't have a
> > configuration of it's own, otherwise we may end up breaking an
> > otherwise working system.
>
> Right, I can rework that.

Re: [PATCH 2/3] mmc: sdhci-of-aspeed: Expose data sample phase delay tuning

[Joel Stanley]

On Fri, 11 Sep 2020 at 02:49, Andrew Jeffery <andrew@aj.id.au> wrote:
>
>
>
> On Fri, 11 Sep 2020, at 11:32, Joel Stanley wrote:
> > On Thu, 10 Sep 2020 at 10:55, Andrew Jeffery <andrew@aj.id.au> wrote:
> > >
> > > Allow sample phase adjustment to deal with layout or tolerance issues.
> > >
> > > Signed-off-by: Andrew Jeffery <andrew@aj.id.au>
> > > ---
> > >  drivers/mmc/host/sdhci-of-aspeed.c | 137 +++++++++++++++++++++++++++--
> > >  1 file changed, 132 insertions(+), 5 deletions(-)
> > >
> > > diff --git a/drivers/mmc/host/sdhci-of-aspeed.c b/drivers/mmc/host/sdhci-of-aspeed.c
> > > index 4f008ba3280e..641accbfcde4 100644
> > > --- a/drivers/mmc/host/sdhci-of-aspeed.c
> > > +++ b/drivers/mmc/host/sdhci-of-aspeed.c

> > > +static void
> > > +aspeed_sdc_configure_phase(struct aspeed_sdc *sdc,
> > > +                          const struct aspeed_sdhci_phase_desc *phase,
> > > +                          uint8_t value, bool enable)
> > > +{
> > > +       u32 reg;
> > > +
> > > +       spin_lock(&sdc->lock);
> >
> > What is the lock protecting against?
> >
> > We call this in the ->probe, so there should be no concurrent access going on.
>
> Because the register is in the "global" part of the SD/MMC controller address
> space (it's not part of the SDHCI), and there are multiple slots that may have
> a driver probed concurrently.

That points to having the property be part of the "global" device tree
node. This would simplify the code; you wouldn't need the locking
either.

>
> >
> >
> > > +       reg = readl(sdc->regs + ASPEED_SDC_PHASE);
> > > +       reg &= ~phase->enable_mask;
> > > +       if (enable) {
> > > +               reg &= ~phase->value_mask;
> > > +               reg |= value << __ffs(phase->value_mask);
> > > +               reg |= phase->enable_value << __ffs(phase->enable_mask);
> > > +       }
> > > +       writel(reg, sdc->regs + ASPEED_SDC_PHASE);
> > > +       spin_unlock(&sdc->lock);
> > > +}
> > > +
> > >  static void aspeed_sdhci_set_clock(struct sdhci_host *host, unsigned int clock)
> > >  {
> > >         struct sdhci_pltfm_host *pltfm_host;
> > > @@ -155,8 +195,58 @@ static inline int aspeed_sdhci_calculate_slot(struct aspeed_sdhci *dev,
> > >         return (delta / 0x100) - 1;
> > >  }
> > >
> > > +static int aspeed_sdhci_configure_of(struct platform_device *pdev,
> > > +                                    struct aspeed_sdhci *sdhci)
> > > +{
> > > +       u32 iphase, ophase;
> > > +       struct device_node *np;
> > > +       struct device *dev;
> > > +       int ret;
> > > +
> > > +       if (!sdhci->phase)
> > > +               return 0;
> > > +
> > > +       dev = &pdev->dev;
> > > +       np = dev->of_node;
> > > +
> > > +       ret = of_property_read_u32(np, "aspeed,input-phase", &iphase);
> > > +       if (ret < 0) {
> > > +               aspeed_sdc_configure_phase(sdhci->parent, &sdhci->phase->in, 0,
> > > +                                          false);
> >
> > Will this clear any value that eg. u-boot writes?
>
> No, see the 'enable' test in aspeed_sdc_configure_phase()

OK, so this branch will never cause any change in the register? Best
to drop it then.

>
> >
> > The register should be left alone if the kernel doesn't have a
> > configuration of it's own, otherwise we may end up breaking an
> > otherwise working system.
>
> Right, I can rework that.

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

Re: [PATCH 2/3] mmc: sdhci-of-aspeed: Expose data sample phase delay tuning

[Andrew Jeffery]

On Fri, 11 Sep 2020, at 13:03, Joel Stanley wrote:
> On Fri, 11 Sep 2020 at 02:49, Andrew Jeffery <andrew@aj.id.au> wrote:
> >
> >
> >
> > On Fri, 11 Sep 2020, at 11:32, Joel Stanley wrote:
> > > On Thu, 10 Sep 2020 at 10:55, Andrew Jeffery <andrew@aj.id.au> wrote:
> > > >
> > > > Allow sample phase adjustment to deal with layout or tolerance issues.
> > > >
> > > > Signed-off-by: Andrew Jeffery <andrew@aj.id.au>
> > > > ---
> > > >  drivers/mmc/host/sdhci-of-aspeed.c | 137 +++++++++++++++++++++++++++--
> > > >  1 file changed, 132 insertions(+), 5 deletions(-)
> > > >
> > > > diff --git a/drivers/mmc/host/sdhci-of-aspeed.c b/drivers/mmc/host/sdhci-of-aspeed.c
> > > > index 4f008ba3280e..641accbfcde4 100644
> > > > --- a/drivers/mmc/host/sdhci-of-aspeed.c
> > > > +++ b/drivers/mmc/host/sdhci-of-aspeed.c
> 
> > > > +static void
> > > > +aspeed_sdc_configure_phase(struct aspeed_sdc *sdc,
> > > > +                          const struct aspeed_sdhci_phase_desc *phase,
> > > > +                          uint8_t value, bool enable)
> > > > +{
> > > > +       u32 reg;
> > > > +
> > > > +       spin_lock(&sdc->lock);
> > >
> > > What is the lock protecting against?
> > >
> > > We call this in the ->probe, so there should be no concurrent access going on.
> >
> > Because the register is in the "global" part of the SD/MMC controller address
> > space (it's not part of the SDHCI), and there are multiple slots that may have
> > a driver probed concurrently.
> 
> That points to having the property be part of the "global" device tree
> node.

Not really. The settings are slot-specific. The only reason it's in the global
register space is that the settings cannot be part of the SDHCI. That Aspeed
chose to pack them in the same register, and _interleaved_ at that, is
unfortunate.

As the settings are slot-specific they should be associated with each slot's
node. We should concentrate on representing the intent of the controls and
not tie the devicetree representation to the register layout that Aspeed came
up with.

>  This would simplify the code; you wouldn't need the locking
> either.

IMO this is a loss for readability, so I'm not convinced it should be changed.
The outcome is some opaque register value that is shoved in the devicetree,
and given the baffling interleaving and choices of field sizes that's not a place
I want to be.

> 
> >
> > >
> > >
> > > > +       reg = readl(sdc->regs + ASPEED_SDC_PHASE);
> > > > +       reg &= ~phase->enable_mask;
> > > > +       if (enable) {
> > > > +               reg &= ~phase->value_mask;
> > > > +               reg |= value << __ffs(phase->value_mask);
> > > > +               reg |= phase->enable_value << __ffs(phase->enable_mask);
> > > > +       }
> > > > +       writel(reg, sdc->regs + ASPEED_SDC_PHASE);
> > > > +       spin_unlock(&sdc->lock);
> > > > +}
> > > > +
> > > >  static void aspeed_sdhci_set_clock(struct sdhci_host *host, unsigned int clock)
> > > >  {
> > > >         struct sdhci_pltfm_host *pltfm_host;
> > > > @@ -155,8 +195,58 @@ static inline int aspeed_sdhci_calculate_slot(struct aspeed_sdhci *dev,
> > > >         return (delta / 0x100) - 1;
> > > >  }
> > > >
> > > > +static int aspeed_sdhci_configure_of(struct platform_device *pdev,
> > > > +                                    struct aspeed_sdhci *sdhci)
> > > > +{
> > > > +       u32 iphase, ophase;
> > > > +       struct device_node *np;
> > > > +       struct device *dev;
> > > > +       int ret;
> > > > +
> > > > +       if (!sdhci->phase)
> > > > +               return 0;
> > > > +
> > > > +       dev = &pdev->dev;
> > > > +       np = dev->of_node;
> > > > +
> > > > +       ret = of_property_read_u32(np, "aspeed,input-phase", &iphase);
> > > > +       if (ret < 0) {
> > > > +               aspeed_sdc_configure_phase(sdhci->parent, &sdhci->phase->in, 0,
> > > > +                                          false);
> > >
> > > Will this clear any value that eg. u-boot writes?
> >
> > No, see the 'enable' test in aspeed_sdc_configure_phase()
> 
> OK, so this branch will never cause any change in the register? Best
> to drop it then.

So there are two parts to the phase configuration, the phase adjustment
value, and a switch to turn phase adjustment on or off. Both fields exist
for both in and out phase adjustments for both slots.

So this branch will cause the phase control to be disabled, but it won't
change the phase value that was originally programmed. If we maintain
the original semantics it shouldn't be dropped.

However, below you suggest we maintain the configuration (both
enable and value state) in the absence of the property, so the code
needs to be reworked to uphold suggestion.

> 
> >
> > >
> > > The register should be left alone if the kernel doesn't have a
> > > configuration of it's own, otherwise we may end up breaking an
> > > otherwise working system.
> >
> > Right, I can rework that.
>

Re: [PATCH 2/3] mmc: sdhci-of-aspeed: Expose data sample phase delay tuning

[Andrew Jeffery]

On Fri, 11 Sep 2020, at 13:03, Joel Stanley wrote:
> On Fri, 11 Sep 2020 at 02:49, Andrew Jeffery <andrew@aj.id.au> wrote:
> >
> >
> >
> > On Fri, 11 Sep 2020, at 11:32, Joel Stanley wrote:
> > > On Thu, 10 Sep 2020 at 10:55, Andrew Jeffery <andrew@aj.id.au> wrote:
> > > >
> > > > Allow sample phase adjustment to deal with layout or tolerance issues.
> > > >
> > > > Signed-off-by: Andrew Jeffery <andrew@aj.id.au>
> > > > ---
> > > >  drivers/mmc/host/sdhci-of-aspeed.c | 137 +++++++++++++++++++++++++++--
> > > >  1 file changed, 132 insertions(+), 5 deletions(-)
> > > >
> > > > diff --git a/drivers/mmc/host/sdhci-of-aspeed.c b/drivers/mmc/host/sdhci-of-aspeed.c
> > > > index 4f008ba3280e..641accbfcde4 100644
> > > > --- a/drivers/mmc/host/sdhci-of-aspeed.c
> > > > +++ b/drivers/mmc/host/sdhci-of-aspeed.c
> 
> > > > +static void
> > > > +aspeed_sdc_configure_phase(struct aspeed_sdc *sdc,
> > > > +                          const struct aspeed_sdhci_phase_desc *phase,
> > > > +                          uint8_t value, bool enable)
> > > > +{
> > > > +       u32 reg;
> > > > +
> > > > +       spin_lock(&sdc->lock);
> > >
> > > What is the lock protecting against?
> > >
> > > We call this in the ->probe, so there should be no concurrent access going on.
> >
> > Because the register is in the "global" part of the SD/MMC controller address
> > space (it's not part of the SDHCI), and there are multiple slots that may have
> > a driver probed concurrently.
> 
> That points to having the property be part of the "global" device tree
> node.

Not really. The settings are slot-specific. The only reason it's in the global
register space is that the settings cannot be part of the SDHCI. That Aspeed
chose to pack them in the same register, and _interleaved_ at that, is
unfortunate.

As the settings are slot-specific they should be associated with each slot's
node. We should concentrate on representing the intent of the controls and
not tie the devicetree representation to the register layout that Aspeed came
up with.

>  This would simplify the code; you wouldn't need the locking
> either.

IMO this is a loss for readability, so I'm not convinced it should be changed.
The outcome is some opaque register value that is shoved in the devicetree,
and given the baffling interleaving and choices of field sizes that's not a place
I want to be.

> 
> >
> > >
> > >
> > > > +       reg = readl(sdc->regs + ASPEED_SDC_PHASE);
> > > > +       reg &= ~phase->enable_mask;
> > > > +       if (enable) {
> > > > +               reg &= ~phase->value_mask;
> > > > +               reg |= value << __ffs(phase->value_mask);
> > > > +               reg |= phase->enable_value << __ffs(phase->enable_mask);
> > > > +       }
> > > > +       writel(reg, sdc->regs + ASPEED_SDC_PHASE);
> > > > +       spin_unlock(&sdc->lock);
> > > > +}
> > > > +
> > > >  static void aspeed_sdhci_set_clock(struct sdhci_host *host, unsigned int clock)
> > > >  {
> > > >         struct sdhci_pltfm_host *pltfm_host;
> > > > @@ -155,8 +195,58 @@ static inline int aspeed_sdhci_calculate_slot(struct aspeed_sdhci *dev,
> > > >         return (delta / 0x100) - 1;
> > > >  }
> > > >
> > > > +static int aspeed_sdhci_configure_of(struct platform_device *pdev,
> > > > +                                    struct aspeed_sdhci *sdhci)
> > > > +{
> > > > +       u32 iphase, ophase;
> > > > +       struct device_node *np;
> > > > +       struct device *dev;
> > > > +       int ret;
> > > > +
> > > > +       if (!sdhci->phase)
> > > > +               return 0;
> > > > +
> > > > +       dev = &pdev->dev;
> > > > +       np = dev->of_node;
> > > > +
> > > > +       ret = of_property_read_u32(np, "aspeed,input-phase", &iphase);
> > > > +       if (ret < 0) {
> > > > +               aspeed_sdc_configure_phase(sdhci->parent, &sdhci->phase->in, 0,
> > > > +                                          false);
> > >
> > > Will this clear any value that eg. u-boot writes?
> >
> > No, see the 'enable' test in aspeed_sdc_configure_phase()
> 
> OK, so this branch will never cause any change in the register? Best
> to drop it then.

So there are two parts to the phase configuration, the phase adjustment
value, and a switch to turn phase adjustment on or off. Both fields exist
for both in and out phase adjustments for both slots.

So this branch will cause the phase control to be disabled, but it won't
change the phase value that was originally programmed. If we maintain
the original semantics it shouldn't be dropped.

However, below you suggest we maintain the configuration (both
enable and value state) in the absence of the property, so the code
needs to be reworked to uphold suggestion.

> 
> >
> > >
> > > The register should be left alone if the kernel doesn't have a
> > > configuration of it's own, otherwise we may end up breaking an
> > > otherwise working system.
> >
> > Right, I can rework that.
>

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

Re: [PATCH 1/3] dt: bindings: mmc: Add phase control properties for the Aspeed SDHCI

[Ulf Hansson]

On Thu, 10 Sep 2020 at 12:54, Andrew Jeffery <andrew@aj.id.au> wrote:
>
> Add properties to control the phase delay for input and output data
> sampling.
>
> Signed-off-by: Andrew Jeffery <andrew@aj.id.au>
> ---
>  Documentation/devicetree/bindings/mmc/aspeed,sdhci.yaml | 8 ++++++++
>  1 file changed, 8 insertions(+)
>
> diff --git a/Documentation/devicetree/bindings/mmc/aspeed,sdhci.yaml b/Documentation/devicetree/bindings/mmc/aspeed,sdhci.yaml
> index 987b287f3bff..75effd411554 100644
> --- a/Documentation/devicetree/bindings/mmc/aspeed,sdhci.yaml
> +++ b/Documentation/devicetree/bindings/mmc/aspeed,sdhci.yaml
> @@ -61,6 +61,14 @@ patternProperties:
>        sdhci,auto-cmd12:
>          type: boolean
>          description: Specifies that controller should use auto CMD12
> +      "aspeed,input-phase":
> +        $ref: '/schemas/types.yaml#/definitions/uint32'
> +        description:
> +          The input clock phase delay value.
> +      "aspeed,output-phase":
> +        $ref: '/schemas/types.yaml#/definitions/uint32'
> +        description:
> +          The output clock phase delay value.

We already have a common mmc clk-phase* binding, see
mmc-controller.yaml. As matter of fact, there is one binding per speed
mode.

Could that work for this case as well?

Kind regards
Uffe

Re: [PATCH 1/3] dt: bindings: mmc: Add phase control properties for the Aspeed SDHCI

[Ulf Hansson]

On Thu, 10 Sep 2020 at 12:54, Andrew Jeffery <andrew@aj.id.au> wrote:
>
> Add properties to control the phase delay for input and output data
> sampling.
>
> Signed-off-by: Andrew Jeffery <andrew@aj.id.au>
> ---
>  Documentation/devicetree/bindings/mmc/aspeed,sdhci.yaml | 8 ++++++++
>  1 file changed, 8 insertions(+)
>
> diff --git a/Documentation/devicetree/bindings/mmc/aspeed,sdhci.yaml b/Documentation/devicetree/bindings/mmc/aspeed,sdhci.yaml
> index 987b287f3bff..75effd411554 100644
> --- a/Documentation/devicetree/bindings/mmc/aspeed,sdhci.yaml
> +++ b/Documentation/devicetree/bindings/mmc/aspeed,sdhci.yaml
> @@ -61,6 +61,14 @@ patternProperties:
>        sdhci,auto-cmd12:
>          type: boolean
>          description: Specifies that controller should use auto CMD12
> +      "aspeed,input-phase":
> +        $ref: '/schemas/types.yaml#/definitions/uint32'
> +        description:
> +          The input clock phase delay value.
> +      "aspeed,output-phase":
> +        $ref: '/schemas/types.yaml#/definitions/uint32'
> +        description:
> +          The output clock phase delay value.

We already have a common mmc clk-phase* binding, see
mmc-controller.yaml. As matter of fact, there is one binding per speed
mode.

Could that work for this case as well?

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 1/3] dt: bindings: mmc: Add phase control properties for the Aspeed SDHCI

[Andrew Jeffery]

On Mon, 14 Sep 2020, at 19:11, Ulf Hansson wrote:
> On Thu, 10 Sep 2020 at 12:54, Andrew Jeffery <andrew@aj.id.au> wrote:
> >
> > Add properties to control the phase delay for input and output data
> > sampling.
> >
> > Signed-off-by: Andrew Jeffery <andrew@aj.id.au>
> > ---
> >  Documentation/devicetree/bindings/mmc/aspeed,sdhci.yaml | 8 ++++++++
> >  1 file changed, 8 insertions(+)
> >
> > diff --git a/Documentation/devicetree/bindings/mmc/aspeed,sdhci.yaml b/Documentation/devicetree/bindings/mmc/aspeed,sdhci.yaml
> > index 987b287f3bff..75effd411554 100644
> > --- a/Documentation/devicetree/bindings/mmc/aspeed,sdhci.yaml
> > +++ b/Documentation/devicetree/bindings/mmc/aspeed,sdhci.yaml
> > @@ -61,6 +61,14 @@ patternProperties:
> >        sdhci,auto-cmd12:
> >          type: boolean
> >          description: Specifies that controller should use auto CMD12
> > +      "aspeed,input-phase":
> > +        $ref: '/schemas/types.yaml#/definitions/uint32'
> > +        description:
> > +          The input clock phase delay value.
> > +      "aspeed,output-phase":
> > +        $ref: '/schemas/types.yaml#/definitions/uint32'
> > +        description:
> > +          The output clock phase delay value.
> 
> We already have a common mmc clk-phase* binding, see
> mmc-controller.yaml. As matter of fact, there is one binding per speed
> mode.
> 
> Could that work for this case as well?

Ah, great, I think so. Sorry for overlooking that. I just need to extract from 
Aspeed what units the damn register fields are using :/

Andrew

Re: [PATCH 1/3] dt: bindings: mmc: Add phase control properties for the Aspeed SDHCI

[Andrew Jeffery]

On Mon, 14 Sep 2020, at 19:11, Ulf Hansson wrote:
> On Thu, 10 Sep 2020 at 12:54, Andrew Jeffery <andrew@aj.id.au> wrote:
> >
> > Add properties to control the phase delay for input and output data
> > sampling.
> >
> > Signed-off-by: Andrew Jeffery <andrew@aj.id.au>
> > ---
> >  Documentation/devicetree/bindings/mmc/aspeed,sdhci.yaml | 8 ++++++++
> >  1 file changed, 8 insertions(+)
> >
> > diff --git a/Documentation/devicetree/bindings/mmc/aspeed,sdhci.yaml b/Documentation/devicetree/bindings/mmc/aspeed,sdhci.yaml
> > index 987b287f3bff..75effd411554 100644
> > --- a/Documentation/devicetree/bindings/mmc/aspeed,sdhci.yaml
> > +++ b/Documentation/devicetree/bindings/mmc/aspeed,sdhci.yaml
> > @@ -61,6 +61,14 @@ patternProperties:
> >        sdhci,auto-cmd12:
> >          type: boolean
> >          description: Specifies that controller should use auto CMD12
> > +      "aspeed,input-phase":
> > +        $ref: '/schemas/types.yaml#/definitions/uint32'
> > +        description:
> > +          The input clock phase delay value.
> > +      "aspeed,output-phase":
> > +        $ref: '/schemas/types.yaml#/definitions/uint32'
> > +        description:
> > +          The output clock phase delay value.
> 
> We already have a common mmc clk-phase* binding, see
> mmc-controller.yaml. As matter of fact, there is one binding per speed
> mode.
> 
> Could that work for this case as well?

Ah, great, I think so. Sorry for overlooking that. I just need to extract from 
Aspeed what units the damn register fields are using :/

Andrew

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