This patch series aims to configure SD and eMMC controllers' clock
frequency and clock phase parameters. The main modification is the
clock phase calculation method which has been checked with the HW IP
designer. Also, the clock source detection method is updated for
AST2600-A2/A3. This patch series has been verified on AST2600-A3 EVB.
Chin-Ting Kuo (10):
clk: aspeed: ast2600: Porting sdhci clock source
sdhci: aspeed: Add SDR50 support
dts: aspeed: ast2600: Support SDR50 for SD device
mmc: Add invert flag for clock phase signedness
mmc: aspeed: Adjust delay taps calculation method
arm: dts: aspeed: Change eMMC device compatible
arm: dts: aspeed: Adjust clock phase parameter
arm: dts: ibm: Adjust clock phase parameter
dt-bindings: mmc: aspeed: Add max-tap-delay property
dt-bindings: mmc: aspeed: Add a new compatible string
.../devicetree/bindings/mmc/aspeed,sdhci.yaml | 4 +
arch/arm/boot/dts/aspeed-ast2600-evb-a1.dts | 8 ++
arch/arm/boot/dts/aspeed-ast2600-evb.dts | 11 +-
arch/arm/boot/dts/aspeed-bmc-ibm-everest.dts | 3 +-
arch/arm/boot/dts/aspeed-bmc-ibm-rainier.dts | 3 +-
arch/arm/boot/dts/aspeed-bmc-opp-tacoma.dts | 3 +-
arch/arm/boot/dts/aspeed-g6.dtsi | 2 +-
drivers/clk/clk-ast2600.c | 69 ++++++++--
drivers/mmc/core/host.c | 10 +-
drivers/mmc/host/sdhci-of-aspeed.c | 123 ++++++++++++++----
include/linux/mmc/host.h | 2 +
11 files changed, 193 insertions(+), 45 deletions(-)
--
2.17.1
Add max-tap-delay proptery in order to record the maximum
tap delay on different platforms.
Signed-off-by: Chin-Ting Kuo <[email protected]>
---
Documentation/devicetree/bindings/mmc/aspeed,sdhci.yaml | 3 +++
1 file changed, 3 insertions(+)
diff --git a/Documentation/devicetree/bindings/mmc/aspeed,sdhci.yaml b/Documentation/devicetree/bindings/mmc/aspeed,sdhci.yaml
index 987b287f3bff..5bb66849df65 100644
--- a/Documentation/devicetree/bindings/mmc/aspeed,sdhci.yaml
+++ b/Documentation/devicetree/bindings/mmc/aspeed,sdhci.yaml
@@ -37,6 +37,9 @@ properties:
clocks:
maxItems: 1
description: The SD/SDIO controller clock gate
+ max-tap-delay:
+ maxItems: 1
+ description: The maximum delay in picosecond for SD/SDIO controller
patternProperties:
"^sdhci@[0-9a-f]+$":
--
2.17.1
Add "aspeed,ast2600-emmc" compatible string for the sake of
distinguishing between SD and eMMC device.
Signed-off-by: Chin-Ting Kuo <[email protected]>
---
Documentation/devicetree/bindings/mmc/aspeed,sdhci.yaml | 1 +
1 file changed, 1 insertion(+)
diff --git a/Documentation/devicetree/bindings/mmc/aspeed,sdhci.yaml b/Documentation/devicetree/bindings/mmc/aspeed,sdhci.yaml
index 5bb66849df65..41105cd104c6 100644
--- a/Documentation/devicetree/bindings/mmc/aspeed,sdhci.yaml
+++ b/Documentation/devicetree/bindings/mmc/aspeed,sdhci.yaml
@@ -52,6 +52,7 @@ patternProperties:
- aspeed,ast2400-sdhci
- aspeed,ast2500-sdhci
- aspeed,ast2600-sdhci
+ - aspeed,ast2600-emmc
reg:
maxItems: 1
description: The SDHCI registers
--
2.17.1
- The maximum tap delay may be slightly different on
different platforms. It may also be different due to
different SoC processes or different manufacturers.
Thus, the maximum tap delay should be gotten from the
device tree through max-tap-delay property.
- The delay time for each tap is an absolute value which
is independent of clock frequency. But, in order to combine
this principle with "phase" concept, clock frequency is took
into consideration during calculating delay taps.
- The delay cell of eMMC device is non-uniform.
The time period of the first tap is two times of others.
- The clock phase degree range is from -360 to 360.
But, if the clock phase signedness is negative, clock signal
is output from the falling edge first by default and thus, clock
signal is leading to data signal by 90 degrees at least.
Signed-off-by: Chin-Ting Kuo <[email protected]>
---
drivers/mmc/host/sdhci-of-aspeed.c | 115 ++++++++++++++++++++++-------
1 file changed, 89 insertions(+), 26 deletions(-)
diff --git a/drivers/mmc/host/sdhci-of-aspeed.c b/drivers/mmc/host/sdhci-of-aspeed.c
index c6eaeb02e3f9..739c9503a5ed 100644
--- a/drivers/mmc/host/sdhci-of-aspeed.c
+++ b/drivers/mmc/host/sdhci-of-aspeed.c
@@ -44,6 +44,7 @@ struct aspeed_sdc {
spinlock_t lock;
void __iomem *regs;
+ u32 max_tap_delay_ps;
};
struct aspeed_sdhci_tap_param {
@@ -63,6 +64,7 @@ struct aspeed_sdhci_tap_desc {
struct aspeed_sdhci_phase_desc {
struct aspeed_sdhci_tap_desc in;
struct aspeed_sdhci_tap_desc out;
+ u32 nr_taps;
};
struct aspeed_sdhci_pdata {
@@ -158,43 +160,60 @@ aspeed_sdc_set_phase_taps(struct aspeed_sdc *sdc,
}
#define PICOSECONDS_PER_SECOND 1000000000000ULL
-#define ASPEED_SDHCI_NR_TAPS 15
-/* Measured value with *handwave* environmentals and static loading */
-#define ASPEED_SDHCI_MAX_TAP_DELAY_PS 1253
+#define ASPEED_SDHCI_MAX_TAPS 15
+
static int aspeed_sdhci_phase_to_tap(struct device *dev, unsigned long rate_hz,
- int phase_deg)
+ bool invert, int phase_deg, u32 nr_taps)
{
u64 phase_period_ps;
u64 prop_delay_ps;
u64 clk_period_ps;
- unsigned int tap;
- u8 inverted;
+ u32 tap = 0;
+ struct aspeed_sdc *sdc = dev_get_drvdata(dev->parent);
- phase_deg %= 360;
+ if (sdc->max_tap_delay_ps == 0)
+ return 0;
- if (phase_deg >= 180) {
- inverted = ASPEED_SDHCI_TAP_PARAM_INVERT_CLK;
- phase_deg -= 180;
- dev_dbg(dev,
- "Inverting clock to reduce phase correction from %d to %d degrees\n",
- phase_deg + 180, phase_deg);
- } else {
- inverted = 0;
+ prop_delay_ps = sdc->max_tap_delay_ps / nr_taps;
+ clk_period_ps = div_u64(PICOSECONDS_PER_SECOND, (u64)rate_hz);
+
+ /*
+ * For ast2600, if clock phase degree is negative, clock signal is
+ * output from falling edge first by default. Namely, clock signal
+ * is leading to data signal by 90 degrees at least.
+ */
+ if (invert) {
+ if (phase_deg >= 90)
+ phase_deg -= 90;
+ else
+ phase_deg = 0;
}
- prop_delay_ps = ASPEED_SDHCI_MAX_TAP_DELAY_PS / ASPEED_SDHCI_NR_TAPS;
- clk_period_ps = div_u64(PICOSECONDS_PER_SECOND, (u64)rate_hz);
phase_period_ps = div_u64((u64)phase_deg * clk_period_ps, 360ULL);
- tap = div_u64(phase_period_ps, prop_delay_ps);
- if (tap > ASPEED_SDHCI_NR_TAPS) {
+ /*
+ * The delay cell is non-uniform for eMMC controller.
+ * The time period of the first tap is two times of others.
+ */
+ if (nr_taps == 16 && phase_period_ps > prop_delay_ps * 2) {
+ phase_period_ps -= prop_delay_ps * 2;
+ tap++;
+ }
+
+ tap += div_u64(phase_period_ps, prop_delay_ps);
+ if (tap > ASPEED_SDHCI_MAX_TAPS) {
dev_dbg(dev,
"Requested out of range phase tap %d for %d degrees of phase compensation at %luHz, clamping to tap %d\n",
- tap, phase_deg, rate_hz, ASPEED_SDHCI_NR_TAPS);
- tap = ASPEED_SDHCI_NR_TAPS;
+ tap, phase_deg, rate_hz, ASPEED_SDHCI_MAX_TAPS);
+ tap = ASPEED_SDHCI_MAX_TAPS;
}
- return inverted | tap;
+ if (invert) {
+ dev_info(dev, "invert the clock\n");
+ tap |= ASPEED_SDHCI_TAP_PARAM_INVERT_CLK;
+ }
+
+ return tap;
}
static void
@@ -202,13 +221,19 @@ aspeed_sdhci_phases_to_taps(struct device *dev, unsigned long rate,
const struct mmc_clk_phase *phases,
struct aspeed_sdhci_tap_param *taps)
{
+ struct sdhci_host *host = dev->driver_data;
+ struct aspeed_sdhci *sdhci;
+
+ sdhci = sdhci_pltfm_priv(sdhci_priv(host));
taps->valid = phases->valid;
if (!phases->valid)
return;
- taps->in = aspeed_sdhci_phase_to_tap(dev, rate, phases->in_deg);
- taps->out = aspeed_sdhci_phase_to_tap(dev, rate, phases->out_deg);
+ taps->in = aspeed_sdhci_phase_to_tap(dev, rate, phases->inv_in_deg,
+ phases->in_deg, sdhci->phase_desc->nr_taps);
+ taps->out = aspeed_sdhci_phase_to_tap(dev, rate, phases->inv_out_deg,
+ phases->out_deg, sdhci->phase_desc->nr_taps);
}
static void
@@ -230,8 +255,8 @@ aspeed_sdhci_configure_phase(struct sdhci_host *host, unsigned long rate)
aspeed_sdc_set_phase_taps(sdhci->parent, sdhci->phase_desc, taps);
dev_dbg(dev,
"Using taps [%d, %d] for [%d, %d] degrees of phase correction at %luHz (%d)\n",
- taps->in & ASPEED_SDHCI_NR_TAPS,
- taps->out & ASPEED_SDHCI_NR_TAPS,
+ taps->in & ASPEED_SDHCI_MAX_TAPS,
+ taps->out & ASPEED_SDHCI_MAX_TAPS,
params->in_deg, params->out_deg, rate, host->timing);
}
@@ -493,6 +518,7 @@ static const struct aspeed_sdhci_phase_desc ast2600_sdhci_phase[] = {
.enable_mask = ASPEED_SDC_S0_PHASE_OUT_EN,
.enable_value = 3,
},
+ .nr_taps = 15,
},
/* SDHCI/Slot 1 */
[1] = {
@@ -506,6 +532,31 @@ static const struct aspeed_sdhci_phase_desc ast2600_sdhci_phase[] = {
.enable_mask = ASPEED_SDC_S1_PHASE_OUT_EN,
.enable_value = 3,
},
+ .nr_taps = 15,
+ },
+};
+
+static const struct aspeed_sdhci_phase_desc ast2600_emmc_phase[] = {
+ /* eMMC slot 0 */
+ [0] = {
+ .in = {
+ .tap_mask = ASPEED_SDC_S0_PHASE_IN,
+ .enable_mask = ASPEED_SDC_S0_PHASE_IN_EN,
+ .enable_value = 1,
+ },
+ .out = {
+ .tap_mask = ASPEED_SDC_S0_PHASE_OUT,
+ .enable_mask = ASPEED_SDC_S0_PHASE_OUT_EN,
+ .enable_value = 3,
+ },
+
+ /*
+ * There are 15 taps recorded in AST2600 datasheet.
+ * But, actually, the time period of the first tap
+ * is two times of others. Thus, 16 tap is used to
+ * emulate this situation.
+ */
+ .nr_taps = 16,
},
};
@@ -515,10 +566,17 @@ static const struct aspeed_sdhci_pdata ast2600_sdhci_pdata = {
.nr_phase_descs = ARRAY_SIZE(ast2600_sdhci_phase),
};
+static const struct aspeed_sdhci_pdata ast2600_emmc_pdata = {
+ .clk_div_start = 1,
+ .phase_desc = ast2600_emmc_phase,
+ .nr_phase_descs = ARRAY_SIZE(ast2600_emmc_phase),
+};
+
static const struct of_device_id aspeed_sdhci_of_match[] = {
{ .compatible = "aspeed,ast2400-sdhci", .data = &ast2400_sdhci_pdata, },
{ .compatible = "aspeed,ast2500-sdhci", .data = &ast2400_sdhci_pdata, },
{ .compatible = "aspeed,ast2600-sdhci", .data = &ast2600_sdhci_pdata, },
+ { .compatible = "aspeed,ast2600-emmc", .data = &ast2600_emmc_pdata, },
{ }
};
@@ -562,6 +620,11 @@ static int aspeed_sdc_probe(struct platform_device *pdev)
goto err_clk;
}
+ ret = of_property_read_u32(pdev->dev.of_node, "max-tap-delay",
+ &sdc->max_tap_delay_ps);
+ if (ret)
+ sdc->max_tap_delay_ps = 0;
+
dev_set_drvdata(&pdev->dev, sdc);
parent = pdev->dev.of_node;
--
2.17.1
Since the eMMC device's delay parameters are different from
the SD's, a new compatible should be used to distinguish
between eMMC and SD device.
Signed-off-by: Chin-Ting Kuo <[email protected]>
---
arch/arm/boot/dts/aspeed-g6.dtsi | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/arch/arm/boot/dts/aspeed-g6.dtsi b/arch/arm/boot/dts/aspeed-g6.dtsi
index 1b47be1704f8..d083de1d6567 100644
--- a/arch/arm/boot/dts/aspeed-g6.dtsi
+++ b/arch/arm/boot/dts/aspeed-g6.dtsi
@@ -597,7 +597,7 @@
status = "disabled";
emmc: sdhci@1e750100 {
- compatible = "aspeed,ast2600-sdhci";
+ compatible = "aspeed,ast2600-emmc";
reg = <0x100 0x100>;
sdhci,auto-cmd12;
interrupts = <GIC_SPI 15 IRQ_TYPE_LEVEL_HIGH>;
--
2.17.1
Change clock phase degree for AST2600 EVB.
These parameter has been verified with 100MHz
clock frequency for eMMC and SD controllers.
Signed-off-by: Chin-Ting Kuo <[email protected]>
---
arch/arm/boot/dts/aspeed-ast2600-evb-a1.dts | 8 ++++++++
arch/arm/boot/dts/aspeed-ast2600-evb.dts | 9 ++++++---
2 files changed, 14 insertions(+), 3 deletions(-)
diff --git a/arch/arm/boot/dts/aspeed-ast2600-evb-a1.dts b/arch/arm/boot/dts/aspeed-ast2600-evb-a1.dts
index dd7148060c4a..2d83617dc436 100644
--- a/arch/arm/boot/dts/aspeed-ast2600-evb-a1.dts
+++ b/arch/arm/boot/dts/aspeed-ast2600-evb-a1.dts
@@ -13,3 +13,11 @@
};
/delete-node/ &sdc;
+
+&emmc_controller {
+ max-tap-delay = <706>;
+};
+
+&emmc {
+ clk-phase-mmc-hs200 = <0 13>, <1 103>;
+};
diff --git a/arch/arm/boot/dts/aspeed-ast2600-evb.dts b/arch/arm/boot/dts/aspeed-ast2600-evb.dts
index 4551dba499c2..f728b9d9b4cf 100644
--- a/arch/arm/boot/dts/aspeed-ast2600-evb.dts
+++ b/arch/arm/boot/dts/aspeed-ast2600-evb.dts
@@ -143,13 +143,15 @@
&emmc_controller {
status = "okay";
+ /* Measured value with *handwave* environmentals and static loading */
+ max-tap-delay = <736>;
};
&emmc {
non-removable;
bus-width = <4>;
max-frequency = <100000000>;
- clk-phase-mmc-hs200 = <9>, <225>;
+ clk-phase-mmc-hs200 = <0 27>, <1 95>;
};
&rtc {
@@ -260,6 +262,7 @@
&sdc {
status = "okay";
+ max-tap-delay = <9000>;
};
/*
@@ -287,7 +290,7 @@
sdhci,wp-inverted;
vmmc-supply = <&vcc_sdhci0>;
vqmmc-supply = <&vccq_sdhci0>;
- clk-phase-sd-hs = <7>, <200>;
+ clk-phase-uhs-sdr50 = <0 130>, <0 238>;
};
&sdhci1 {
@@ -300,5 +303,5 @@
sdhci,wp-inverted;
vmmc-supply = <&vcc_sdhci1>;
vqmmc-supply = <&vccq_sdhci1>;
- clk-phase-sd-hs = <7>, <200>;
+ clk-phase-uhs-sdr50 = <0 130>, <0 130>;
};
--
2.17.1
- Add max-tap-delay property for eMMC controller.
- Change clock phase degree for AST2600 on IBM platforms.
Signed-off-by: Chin-Ting Kuo <[email protected]>
---
arch/arm/boot/dts/aspeed-bmc-ibm-everest.dts | 3 ++-
arch/arm/boot/dts/aspeed-bmc-ibm-rainier.dts | 3 ++-
arch/arm/boot/dts/aspeed-bmc-opp-tacoma.dts | 3 ++-
3 files changed, 6 insertions(+), 3 deletions(-)
diff --git a/arch/arm/boot/dts/aspeed-bmc-ibm-everest.dts b/arch/arm/boot/dts/aspeed-bmc-ibm-everest.dts
index 2efd70666738..eccb4749755a 100644
--- a/arch/arm/boot/dts/aspeed-bmc-ibm-everest.dts
+++ b/arch/arm/boot/dts/aspeed-bmc-ibm-everest.dts
@@ -2824,6 +2824,7 @@
&emmc_controller {
status = "okay";
+ max-tap-delay = <1253>;
};
&pinctrl_emmc_default {
@@ -2832,7 +2833,7 @@
&emmc {
status = "okay";
- clk-phase-mmc-hs200 = <210>, <228>;
+ clk-phase-mmc-hs200 = <1 124>, <1 141>;
};
&fsim0 {
diff --git a/arch/arm/boot/dts/aspeed-bmc-ibm-rainier.dts b/arch/arm/boot/dts/aspeed-bmc-ibm-rainier.dts
index 6419c9762c0b..2138a8a10d6e 100644
--- a/arch/arm/boot/dts/aspeed-bmc-ibm-rainier.dts
+++ b/arch/arm/boot/dts/aspeed-bmc-ibm-rainier.dts
@@ -299,6 +299,7 @@
&emmc_controller {
status = "okay";
+ max-tap-delay = <1253>;
};
&pinctrl_emmc_default {
@@ -307,7 +308,7 @@
&emmc {
status = "okay";
- clk-phase-mmc-hs200 = <180>, <180>;
+ clk-phase-mmc-hs200 = <1 90>, <1 90>;
};
&fsim0 {
diff --git a/arch/arm/boot/dts/aspeed-bmc-opp-tacoma.dts b/arch/arm/boot/dts/aspeed-bmc-opp-tacoma.dts
index e39f310d55eb..7427809354cc 100644
--- a/arch/arm/boot/dts/aspeed-bmc-opp-tacoma.dts
+++ b/arch/arm/boot/dts/aspeed-bmc-opp-tacoma.dts
@@ -182,11 +182,12 @@
&emmc_controller {
status = "okay";
+ max-tap-delay = <1253>;
};
&emmc {
status = "okay";
- clk-phase-mmc-hs200 = <36>, <270>;
+ clk-phase-mmc-hs200 = <0 40>, <1 181>;
};
&fsim0 {
--
2.17.1
On Wed, Sep 22, 2021 at 06:31:15PM +0800, Chin-Ting Kuo wrote:
> Add max-tap-delay proptery in order to record the maximum
> tap delay on different platforms.
>
> Signed-off-by: Chin-Ting Kuo <[email protected]>
> ---
> Documentation/devicetree/bindings/mmc/aspeed,sdhci.yaml | 3 +++
> 1 file changed, 3 insertions(+)
>
> diff --git a/Documentation/devicetree/bindings/mmc/aspeed,sdhci.yaml b/Documentation/devicetree/bindings/mmc/aspeed,sdhci.yaml
> index 987b287f3bff..5bb66849df65 100644
> --- a/Documentation/devicetree/bindings/mmc/aspeed,sdhci.yaml
> +++ b/Documentation/devicetree/bindings/mmc/aspeed,sdhci.yaml
> @@ -37,6 +37,9 @@ properties:
> clocks:
> maxItems: 1
> description: The SD/SDIO controller clock gate
> + max-tap-delay:
> + maxItems: 1
An array?
> + description: The maximum delay in picosecond for SD/SDIO controller
Properties with a unit should have a standard unit suffix.
Should be common property? If not, needs a vendor prefix.
>
> patternProperties:
> "^sdhci@[0-9a-f]+$":
> --
> 2.17.1
>
>
On Wed, Sep 22, 2021 at 06:31:16PM +0800, Chin-Ting Kuo wrote:
> Add "aspeed,ast2600-emmc" compatible string for the sake of
> distinguishing between SD and eMMC device.
Why?
Is the h/w block different? We already have properties to handle some of
the eMMC specifics. Also, you can have a child node for the eMMC device
if you need that.
>
> Signed-off-by: Chin-Ting Kuo <[email protected]>
> ---
> Documentation/devicetree/bindings/mmc/aspeed,sdhci.yaml | 1 +
> 1 file changed, 1 insertion(+)
>
> diff --git a/Documentation/devicetree/bindings/mmc/aspeed,sdhci.yaml b/Documentation/devicetree/bindings/mmc/aspeed,sdhci.yaml
> index 5bb66849df65..41105cd104c6 100644
> --- a/Documentation/devicetree/bindings/mmc/aspeed,sdhci.yaml
> +++ b/Documentation/devicetree/bindings/mmc/aspeed,sdhci.yaml
> @@ -52,6 +52,7 @@ patternProperties:
> - aspeed,ast2400-sdhci
> - aspeed,ast2500-sdhci
> - aspeed,ast2600-sdhci
> + - aspeed,ast2600-emmc
> reg:
> maxItems: 1
> description: The SDHCI registers
> --
> 2.17.1
>
>
Hi Rob,
Thanks for your review.
> -----Original Message-----
> From: Rob Herring <[email protected]>
> Sent: Tuesday, September 28, 2021 2:41 AM
> To: Chin-Ting Kuo <[email protected]>
> Subject: Re: [PATCH 09/10] dt-bindings: mmc: aspeed: Add max-tap-delay
> property
>
> On Wed, Sep 22, 2021 at 06:31:15PM +0800, Chin-Ting Kuo wrote:
> > Add max-tap-delay proptery in order to record the maximum tap delay on
> > different platforms.
> >
> > Signed-off-by: Chin-Ting Kuo <[email protected]>
> > ---
> > Documentation/devicetree/bindings/mmc/aspeed,sdhci.yaml | 3 +++
> > 1 file changed, 3 insertions(+)
> >
> > diff --git a/Documentation/devicetree/bindings/mmc/aspeed,sdhci.yaml
> > b/Documentation/devicetree/bindings/mmc/aspeed,sdhci.yaml
> > index 987b287f3bff..5bb66849df65 100644
> > --- a/Documentation/devicetree/bindings/mmc/aspeed,sdhci.yaml
> > +++ b/Documentation/devicetree/bindings/mmc/aspeed,sdhci.yaml
> > @@ -37,6 +37,9 @@ properties:
> > clocks:
> > maxItems: 1
> > description: The SD/SDIO controller clock gate
> > + max-tap-delay:
> > + maxItems: 1
>
> An array?
No, it is an "int" value and this will be fixed in the next patch version.
> > + description: The maximum delay in picosecond for SD/SDIO
> > + controller
>
> Properties with a unit should have a standard unit suffix.
>
> Should be common property? If not, needs a vendor prefix.
Okay, a vender prefix and an unit suffix will be add in the next patch version.
>
> >
> > patternProperties:
> > "^sdhci@[0-9a-f]+$":
> > --
> > 2.17.1
> >
> >
Hi Rob,
> -----Original Message-----
> From: Rob Herring <[email protected]>
> Sent: Tuesday, September 28, 2021 2:59 AM
> To: Chin-Ting Kuo <[email protected]>
> Subject: Re: [PATCH 10/10] dt-bindings: mmc: aspeed: Add a new compatible
> string
>
> On Wed, Sep 22, 2021 at 06:31:16PM +0800, Chin-Ting Kuo wrote:
> > Add "aspeed,ast2600-emmc" compatible string for the sake of
> > distinguishing between SD and eMMC device.
>
> Why?
>
> Is the h/w block different? We already have properties to handle some of the
> eMMC specifics. Also, you can have a child node for the eMMC device if you
> need that.
There are two SD/SDIO controllers in a AST2600 SoC.
One is for SD card and the other is for eMMC.
Although both of them are embedded in the same SoC, the design of delay cell and
the manufacture process are different. The delay phase is definitely different and, thus,
we need a flag, compatible, to distinguish the device, SD or eMMC.
Without "aspeed,ast2600-emmc" compatible, of course, eMMC device can work with original
sdhci driver and device tree setting. But, for ultra-speed or HS200 case, AST2600 SoC needs some
phase delay which (maximum) value is different between SD and eMMC device.
>
> >
> > Signed-off-by: Chin-Ting Kuo <[email protected]>
> > ---
> > Documentation/devicetree/bindings/mmc/aspeed,sdhci.yaml | 1 +
> > 1 file changed, 1 insertion(+)
> >
> > diff --git a/Documentation/devicetree/bindings/mmc/aspeed,sdhci.yaml
> > b/Documentation/devicetree/bindings/mmc/aspeed,sdhci.yaml
> > index 5bb66849df65..41105cd104c6 100644
> > --- a/Documentation/devicetree/bindings/mmc/aspeed,sdhci.yaml
> > +++ b/Documentation/devicetree/bindings/mmc/aspeed,sdhci.yaml
> > @@ -52,6 +52,7 @@ patternProperties:
> > - aspeed,ast2400-sdhci
> > - aspeed,ast2500-sdhci
> > - aspeed,ast2600-sdhci
> > + - aspeed,ast2600-emmc
> > reg:
> > maxItems: 1
> > description: The SDHCI registers
> > --
> > 2.17.1
> >
> >
On Mon, Sep 27, 2021 at 9:51 PM Chin-Ting Kuo
<[email protected]> wrote:
>
> Hi Rob,
>
> > -----Original Message-----
> > From: Rob Herring <[email protected]>
> > Sent: Tuesday, September 28, 2021 2:59 AM
> > To: Chin-Ting Kuo <[email protected]>
> > Subject: Re: [PATCH 10/10] dt-bindings: mmc: aspeed: Add a new compatible
> > string
> >
> > On Wed, Sep 22, 2021 at 06:31:16PM +0800, Chin-Ting Kuo wrote:
> > > Add "aspeed,ast2600-emmc" compatible string for the sake of
> > > distinguishing between SD and eMMC device.
> >
> > Why?
> >
> > Is the h/w block different? We already have properties to handle some of the
> > eMMC specifics. Also, you can have a child node for the eMMC device if you
> > need that.
>
> There are two SD/SDIO controllers in a AST2600 SoC.
> One is for SD card and the other is for eMMC.
> Although both of them are embedded in the same SoC, the design of delay cell and
> the manufacture process are different. The delay phase is definitely different and, thus,
> we need a flag, compatible, to distinguish the device, SD or eMMC.
>
> Without "aspeed,ast2600-emmc" compatible, of course, eMMC device can work with original
> sdhci driver and device tree setting. But, for ultra-speed or HS200 case, AST2600 SoC needs some
> phase delay which (maximum) value is different between SD and eMMC device.
This is quite common as tweaking the timing is also need per board.
Look at what other bindings have done. A property is more appropriate
here.
Rob
Hi Rob
> -----Original Message-----
> From: Rob Herring <[email protected]>
> Sent: Wednesday, September 29, 2021 6:28 AM
> To: Chin-Ting Kuo <[email protected]>
> Subject: Re: [PATCH 10/10] dt-bindings: mmc: aspeed: Add a new compatible
> string
>
> On Mon, Sep 27, 2021 at 9:51 PM Chin-Ting Kuo
> <[email protected]> wrote:
> >
> > Hi Rob,
> >
> > > -----Original Message-----
> > > From: Rob Herring <[email protected]>
> > > Sent: Tuesday, September 28, 2021 2:59 AM
> > > To: Chin-Ting Kuo <[email protected]>
> > > Subject: Re: [PATCH 10/10] dt-bindings: mmc: aspeed: Add a new
> > > compatible string
> > >
> > > On Wed, Sep 22, 2021 at 06:31:16PM +0800, Chin-Ting Kuo wrote:
> > > > Add "aspeed,ast2600-emmc" compatible string for the sake of
> > > > distinguishing between SD and eMMC device.
> > >
> > > Why?
> > >
> > > Is the h/w block different? We already have properties to handle
> > > some of the eMMC specifics. Also, you can have a child node for the
> > > eMMC device if you need that.
> >
> > There are two SD/SDIO controllers in a AST2600 SoC.
> > One is for SD card and the other is for eMMC.
> > Although both of them are embedded in the same SoC, the design of
> > delay cell and the manufacture process are different. The delay phase
> > is definitely different and, thus, we need a flag, compatible, to distinguish the
> device, SD or eMMC.
> >
> > Without "aspeed,ast2600-emmc" compatible, of course, eMMC device can
> > work with original sdhci driver and device tree setting. But, for
> > ultra-speed or HS200 case, AST2600 SoC needs some phase delay which
> (maximum) value is different between SD and eMMC device.
>
> This is quite common as tweaking the timing is also need per board.
> Look at what other bindings have done. A property is more appropriate here.
Okay, I will try to check whether there is an existing binding which can achieve this purpose.
Or, maybe, as you said, a property is better since this phase delay is a proprietary
HW design and is different between each chipset version.
>
> Rob
Chin-Ting
Hi Chin-Ting,
I think we can split this up a bit:
On Wed, 22 Sep 2021, at 20:01, Chin-Ting Kuo wrote:
> - The maximum tap delay may be slightly different on
> different platforms. It may also be different due to
> different SoC processes or different manufacturers.
> Thus, the maximum tap delay should be gotten from the
> device tree through max-tap-delay property.
I think this could be a patch on its own
> - The delay time for each tap is an absolute value which
> is independent of clock frequency. But, in order to combine
> this principle with "phase" concept, clock frequency is took
> into consideration during calculating delay taps.
> - The delay cell of eMMC device is non-uniform.
> The time period of the first tap is two times of others.
Again, this could be a patch of its own
> - The clock phase degree range is from -360 to 360.
> But, if the clock phase signedness is negative, clock signal
> is output from the falling edge first by default and thus, clock
> signal is leading to data signal by 90 degrees at least.
This line of development is impacted by my comment on an earlier patch
in the series, so should be its own patch.
>
> Signed-off-by: Chin-Ting Kuo <[email protected]>
> ---
> drivers/mmc/host/sdhci-of-aspeed.c | 115 ++++++++++++++++++++++-------
> 1 file changed, 89 insertions(+), 26 deletions(-)
>
> diff --git a/drivers/mmc/host/sdhci-of-aspeed.c
> b/drivers/mmc/host/sdhci-of-aspeed.c
> index c6eaeb02e3f9..739c9503a5ed 100644
> --- a/drivers/mmc/host/sdhci-of-aspeed.c
> +++ b/drivers/mmc/host/sdhci-of-aspeed.c
> @@ -44,6 +44,7 @@ struct aspeed_sdc {
>
> spinlock_t lock;
> void __iomem *regs;
> + u32 max_tap_delay_ps;
> };
>
> struct aspeed_sdhci_tap_param {
> @@ -63,6 +64,7 @@ struct aspeed_sdhci_tap_desc {
> struct aspeed_sdhci_phase_desc {
> struct aspeed_sdhci_tap_desc in;
> struct aspeed_sdhci_tap_desc out;
> + u32 nr_taps;
> };
>
> struct aspeed_sdhci_pdata {
> @@ -158,43 +160,60 @@ aspeed_sdc_set_phase_taps(struct aspeed_sdc *sdc,
> }
>
> #define PICOSECONDS_PER_SECOND 1000000000000ULL
> -#define ASPEED_SDHCI_NR_TAPS 15
> -/* Measured value with *handwave* environmentals and static loading */
> -#define ASPEED_SDHCI_MAX_TAP_DELAY_PS 1253
> +#define ASPEED_SDHCI_MAX_TAPS 15
Why are we renaming this? It looks to cause a bit of noise in the diff.
> +
> static int aspeed_sdhci_phase_to_tap(struct device *dev, unsigned long rate_hz,
> - int phase_deg)
> + bool invert, int phase_deg, u32 nr_taps)
Hmm.
> {
> u64 phase_period_ps;
> u64 prop_delay_ps;
> u64 clk_period_ps;
> - unsigned int tap;
> - u8 inverted;
> + u32 tap = 0;
> + struct aspeed_sdc *sdc = dev_get_drvdata(dev->parent);
>
> - phase_deg %= 360;
> + if (sdc->max_tap_delay_ps == 0)
> + return 0;
I don't think just silently returning 0 here is the right thing to do.
What about -EINVAL, or printing a warning and using the old hard-coded
value?
>
> - if (phase_deg >= 180) {
> - inverted = ASPEED_SDHCI_TAP_PARAM_INVERT_CLK;
> - phase_deg -= 180;
> - dev_dbg(dev,
> - "Inverting clock to reduce phase correction from %d to %d degrees\n",
> - phase_deg + 180, phase_deg);
> - } else {
> - inverted = 0;
> + prop_delay_ps = sdc->max_tap_delay_ps / nr_taps;
> + clk_period_ps = div_u64(PICOSECONDS_PER_SECOND, (u64)rate_hz);
> +
> + /*
> + * For ast2600, if clock phase degree is negative, clock signal is
> + * output from falling edge first by default. Namely, clock signal
> + * is leading to data signal by 90 degrees at least.
> + */
Have I missed something about a asymmetric clock timings? Otherwise the
falling edge is 180 degrees away from the rising edge? I'm still not
clear on why 90 degrees is used here.
> + if (invert) {
> + if (phase_deg >= 90)
> + phase_deg -= 90;
> + else
> + phase_deg = 0;
Why are we throwing away information?
> }
>
> - prop_delay_ps = ASPEED_SDHCI_MAX_TAP_DELAY_PS / ASPEED_SDHCI_NR_TAPS;
> - clk_period_ps = div_u64(PICOSECONDS_PER_SECOND, (u64)rate_hz);
> phase_period_ps = div_u64((u64)phase_deg * clk_period_ps, 360ULL);
>
> - tap = div_u64(phase_period_ps, prop_delay_ps);
> - if (tap > ASPEED_SDHCI_NR_TAPS) {
> + /*
> + * The delay cell is non-uniform for eMMC controller.
> + * The time period of the first tap is two times of others.
> + */
> + if (nr_taps == 16 && phase_period_ps > prop_delay_ps * 2) {
> + phase_period_ps -= prop_delay_ps * 2;
> + tap++;
> + }
> +
> + tap += div_u64(phase_period_ps, prop_delay_ps);
> + if (tap > ASPEED_SDHCI_MAX_TAPS) {
> dev_dbg(dev,
> "Requested out of range phase tap %d for %d degrees of phase
> compensation at %luHz, clamping to tap %d\n",
> - tap, phase_deg, rate_hz, ASPEED_SDHCI_NR_TAPS);
> - tap = ASPEED_SDHCI_NR_TAPS;
> + tap, phase_deg, rate_hz, ASPEED_SDHCI_MAX_TAPS);
> + tap = ASPEED_SDHCI_MAX_TAPS;
> }
>
> - return inverted | tap;
> + if (invert) {
> + dev_info(dev, "invert the clock\n");
I prefer we drop this message
> + tap |= ASPEED_SDHCI_TAP_PARAM_INVERT_CLK;
> + }
> +
> + return tap;
> }
>
> static void
> @@ -202,13 +221,19 @@ aspeed_sdhci_phases_to_taps(struct device *dev,
> unsigned long rate,
> const struct mmc_clk_phase *phases,
> struct aspeed_sdhci_tap_param *taps)
> {
> + struct sdhci_host *host = dev->driver_data;
> + struct aspeed_sdhci *sdhci;
> +
> + sdhci = sdhci_pltfm_priv(sdhci_priv(host));
> taps->valid = phases->valid;
>
> if (!phases->valid)
> return;
>
> - taps->in = aspeed_sdhci_phase_to_tap(dev, rate, phases->in_deg);
> - taps->out = aspeed_sdhci_phase_to_tap(dev, rate, phases->out_deg);
> + taps->in = aspeed_sdhci_phase_to_tap(dev, rate, phases->inv_in_deg,
> + phases->in_deg, sdhci->phase_desc->nr_taps);
> + taps->out = aspeed_sdhci_phase_to_tap(dev, rate, phases->inv_out_deg,
> + phases->out_deg, sdhci->phase_desc->nr_taps);
> }
>
> static void
> @@ -230,8 +255,8 @@ aspeed_sdhci_configure_phase(struct sdhci_host
> *host, unsigned long rate)
> aspeed_sdc_set_phase_taps(sdhci->parent, sdhci->phase_desc, taps);
> dev_dbg(dev,
> "Using taps [%d, %d] for [%d, %d] degrees of phase correction at
> %luHz (%d)\n",
> - taps->in & ASPEED_SDHCI_NR_TAPS,
> - taps->out & ASPEED_SDHCI_NR_TAPS,
> + taps->in & ASPEED_SDHCI_MAX_TAPS,
> + taps->out & ASPEED_SDHCI_MAX_TAPS,
> params->in_deg, params->out_deg, rate, host->timing);
> }
>
> @@ -493,6 +518,7 @@ static const struct aspeed_sdhci_phase_desc
> ast2600_sdhci_phase[] = {
> .enable_mask = ASPEED_SDC_S0_PHASE_OUT_EN,
> .enable_value = 3,
> },
> + .nr_taps = 15,
> },
> /* SDHCI/Slot 1 */
> [1] = {
> @@ -506,6 +532,31 @@ static const struct aspeed_sdhci_phase_desc
> ast2600_sdhci_phase[] = {
> .enable_mask = ASPEED_SDC_S1_PHASE_OUT_EN,
> .enable_value = 3,
> },
> + .nr_taps = 15,
> + },
> +};
> +
> +static const struct aspeed_sdhci_phase_desc ast2600_emmc_phase[] = {
> + /* eMMC slot 0 */
> + [0] = {
> + .in = {
> + .tap_mask = ASPEED_SDC_S0_PHASE_IN,
> + .enable_mask = ASPEED_SDC_S0_PHASE_IN_EN,
> + .enable_value = 1,
> + },
> + .out = {
> + .tap_mask = ASPEED_SDC_S0_PHASE_OUT,
> + .enable_mask = ASPEED_SDC_S0_PHASE_OUT_EN,
> + .enable_value = 3,
> + },
> +
> + /*
> + * There are 15 taps recorded in AST2600 datasheet.
> + * But, actually, the time period of the first tap
> + * is two times of others. Thus, 16 tap is used to
> + * emulate this situation.
> + */
> + .nr_taps = 16,
I think this is a very indirect way to communicate the problem. The
only time we look at nr_taps is in a test that explicitly compensates
for the non-uniform delay. I think we should just have a boolean struct
member called 'non_uniform_delay' rather than 'nr_taps', as the number
of taps isn't what's changing. But also see the discussion below about
a potential aspeed,tap-delays property.
> },
> };
>
> @@ -515,10 +566,17 @@ static const struct aspeed_sdhci_pdata
> ast2600_sdhci_pdata = {
> .nr_phase_descs = ARRAY_SIZE(ast2600_sdhci_phase),
> };
>
> +static const struct aspeed_sdhci_pdata ast2600_emmc_pdata = {
> + .clk_div_start = 1,
> + .phase_desc = ast2600_emmc_phase,
> + .nr_phase_descs = ARRAY_SIZE(ast2600_emmc_phase),
> +};
> +
> static const struct of_device_id aspeed_sdhci_of_match[] = {
> { .compatible = "aspeed,ast2400-sdhci", .data = &ast2400_sdhci_pdata, },
> { .compatible = "aspeed,ast2500-sdhci", .data = &ast2400_sdhci_pdata, },
> { .compatible = "aspeed,ast2600-sdhci", .data = &ast2600_sdhci_pdata, },
> + { .compatible = "aspeed,ast2600-emmc", .data = &ast2600_emmc_pdata, },
This needs to be documented (and binding documentation patches need to
be the first patches in the series). Something further to consider is
whether we separate the compatibles or add e.g. an aspeed,tap-delays
property that specifies the specific delay of each logic element. This
might take the place of e.g. the max-tap-delay property?
Andrew
> { }
> };
>
> @@ -562,6 +620,11 @@ static int aspeed_sdc_probe(struct platform_device *pdev)
> goto err_clk;
> }
>
> + ret = of_property_read_u32(pdev->dev.of_node, "max-tap-delay",
> + &sdc->max_tap_delay_ps);
> + if (ret)
> + sdc->max_tap_delay_ps = 0;
> +
> dev_set_drvdata(&pdev->dev, sdc);
>
> parent = pdev->dev.of_node;
> --
> 2.17.1
Hi Andrew,
> -----Original Message-----
> From: Andrew Jeffery <[email protected]>
> Sent: Tuesday, October 26, 2021 11:10 AM
> Subject: Re: [PATCH 05/10] mmc: aspeed: Adjust delay taps calculation method
>
> Hi Chin-Ting,
>
> I think we can split this up a bit:
>
> On Wed, 22 Sep 2021, at 20:01, Chin-Ting Kuo wrote:
> > - The maximum tap delay may be slightly different on
> > different platforms. It may also be different due to
> > different SoC processes or different manufacturers.
> > Thus, the maximum tap delay should be gotten from the
> > device tree through max-tap-delay property.
>
> I think this could be a patch on its own
Okay.
>
> > - The delay time for each tap is an absolute value which
> > is independent of clock frequency. But, in order to combine
> > this principle with "phase" concept, clock frequency is took
> > into consideration during calculating delay taps.
> > - The delay cell of eMMC device is non-uniform.
> > The time period of the first tap is two times of others.
>
> Again, this could be a patch of its own
Okay.
>
> > - The clock phase degree range is from -360 to 360.
> > But, if the clock phase signedness is negative, clock signal
> > is output from the falling edge first by default and thus, clock
> > signal is leading to data signal by 90 degrees at least.
>
> This line of development is impacted by my comment on an earlier patch in
> the series, so should be its own patch.
>
Okay.
> >
> > Signed-off-by: Chin-Ting Kuo <[email protected]>
> > ---
> > drivers/mmc/host/sdhci-of-aspeed.c | 115
> > ++++++++++++++++++++++-------
> > 1 file changed, 89 insertions(+), 26 deletions(-)
> >
> > diff --git a/drivers/mmc/host/sdhci-of-aspeed.c
> > b/drivers/mmc/host/sdhci-of-aspeed.c
> > index c6eaeb02e3f9..739c9503a5ed 100644
> > --- a/drivers/mmc/host/sdhci-of-aspeed.c
> > +++ b/drivers/mmc/host/sdhci-of-aspeed.c
> > @@ -44,6 +44,7 @@ struct aspeed_sdc {
> >
> > spinlock_t lock;
> > void __iomem *regs;
> > + u32 max_tap_delay_ps;
> > };
> >
> > struct aspeed_sdhci_tap_param {
> > @@ -63,6 +64,7 @@ struct aspeed_sdhci_tap_desc { struct
> > aspeed_sdhci_phase_desc {
> > struct aspeed_sdhci_tap_desc in;
> > struct aspeed_sdhci_tap_desc out;
> > + u32 nr_taps;
> > };
> >
> > struct aspeed_sdhci_pdata {
> > @@ -158,43 +160,60 @@ aspeed_sdc_set_phase_taps(struct aspeed_sdc
> > *sdc, }
> >
> > #define PICOSECONDS_PER_SECOND 1000000000000ULL
> > -#define ASPEED_SDHCI_NR_TAPS 15
> > -/* Measured value with *handwave* environmentals and static loading */
> > -#define ASPEED_SDHCI_MAX_TAP_DELAY_PS 1253
> > +#define ASPEED_SDHCI_MAX_TAPS 15
>
> Why are we renaming this? It looks to cause a bit of noise in the diff.
>
Okay, it can be changed back to the original one in the next patch version.
> > +
> > static int aspeed_sdhci_phase_to_tap(struct device *dev, unsigned long
> rate_hz,
> > - int phase_deg)
> > + bool invert, int phase_deg, u32 nr_taps)
>
> Hmm.
>
It will also be modified.
> > {
> > u64 phase_period_ps;
> > u64 prop_delay_ps;
> > u64 clk_period_ps;
> > - unsigned int tap;
> > - u8 inverted;
> > + u32 tap = 0;
> > + struct aspeed_sdc *sdc = dev_get_drvdata(dev->parent);
> >
> > - phase_deg %= 360;
> > + if (sdc->max_tap_delay_ps == 0)
> > + return 0;
>
> I don't think just silently returning 0 here is the right thing to do.
>
> What about -EINVAL, or printing a warning and using the old hard-coded
> value?
>
Agree, both -EINVAL and printing a warning are better.
> >
> > - if (phase_deg >= 180) {
> > - inverted = ASPEED_SDHCI_TAP_PARAM_INVERT_CLK;
> > - phase_deg -= 180;
> > - dev_dbg(dev,
> > - "Inverting clock to reduce phase correction from %d to %d
> degrees\n",
> > - phase_deg + 180, phase_deg);
> > - } else {
> > - inverted = 0;
> > + prop_delay_ps = sdc->max_tap_delay_ps / nr_taps;
> > + clk_period_ps = div_u64(PICOSECONDS_PER_SECOND, (u64)rate_hz);
> > +
> > + /*
> > + * For ast2600, if clock phase degree is negative, clock signal is
> > + * output from falling edge first by default. Namely, clock signal
> > + * is leading to data signal by 90 degrees at least.
> > + */
>
> Have I missed something about a asymmetric clock timings? Otherwise the
> falling edge is 180 degrees away from the rising edge? I'm still not clear on
> why 90 degrees is used here.
>
Oh, you are right. It should be 180 degrees.
> > + if (invert) {
> > + if (phase_deg >= 90)
> > + phase_deg -= 90;
> > + else
> > + phase_deg = 0;
>
> Why are we throwing away information?
>
With the above correction, it should be modified as below.
If the "invert" is needed, we expect that its value should be greater than 180
degrees. We clear "phase_deg" if its value is unexpected. Maybe, a warning
should be shown and -EINVAL can be returned.
if (invert) {
if (phase_deg >= 180)
phase_deg -= 180;
else
phase_deg = 0;
}
> > }
> >
> > - prop_delay_ps = ASPEED_SDHCI_MAX_TAP_DELAY_PS /
> ASPEED_SDHCI_NR_TAPS;
> > - clk_period_ps = div_u64(PICOSECONDS_PER_SECOND, (u64)rate_hz);
> > phase_period_ps = div_u64((u64)phase_deg * clk_period_ps, 360ULL);
> >
> > - tap = div_u64(phase_period_ps, prop_delay_ps);
> > - if (tap > ASPEED_SDHCI_NR_TAPS) {
> > + /*
> > + * The delay cell is non-uniform for eMMC controller.
> > + * The time period of the first tap is two times of others.
> > + */
> > + if (nr_taps == 16 && phase_period_ps > prop_delay_ps * 2) {
> > + phase_period_ps -= prop_delay_ps * 2;
> > + tap++;
> > + }
> > +
> > + tap += div_u64(phase_period_ps, prop_delay_ps);
> > + if (tap > ASPEED_SDHCI_MAX_TAPS) {
> > dev_dbg(dev,
> > "Requested out of range phase tap %d for %d degrees of phase
> > compensation at %luHz, clamping to tap %d\n",
> > - tap, phase_deg, rate_hz, ASPEED_SDHCI_NR_TAPS);
> > - tap = ASPEED_SDHCI_NR_TAPS;
> > + tap, phase_deg, rate_hz, ASPEED_SDHCI_MAX_TAPS);
> > + tap = ASPEED_SDHCI_MAX_TAPS;
> > }
> >
> > - return inverted | tap;
> > + if (invert) {
> > + dev_info(dev, "invert the clock\n");
>
> I prefer we drop this message
>
Okay.
> > + tap |= ASPEED_SDHCI_TAP_PARAM_INVERT_CLK;
> > + }
> > +
> > + return tap;
> > }
> >
> > static void
> > @@ -202,13 +221,19 @@ aspeed_sdhci_phases_to_taps(struct device *dev,
> > unsigned long rate,
> > const struct mmc_clk_phase *phases,
> > struct aspeed_sdhci_tap_param *taps) {
> > + struct sdhci_host *host = dev->driver_data;
> > + struct aspeed_sdhci *sdhci;
> > +
> > + sdhci = sdhci_pltfm_priv(sdhci_priv(host));
> > taps->valid = phases->valid;
> >
> > if (!phases->valid)
> > return;
> >
> > - taps->in = aspeed_sdhci_phase_to_tap(dev, rate, phases->in_deg);
> > - taps->out = aspeed_sdhci_phase_to_tap(dev, rate, phases->out_deg);
> > + taps->in = aspeed_sdhci_phase_to_tap(dev, rate, phases->inv_in_deg,
> > + phases->in_deg, sdhci->phase_desc->nr_taps);
> > + taps->out = aspeed_sdhci_phase_to_tap(dev, rate, phases->inv_out_deg,
> > + phases->out_deg, sdhci->phase_desc->nr_taps);
> > }
> >
> > static void
> > @@ -230,8 +255,8 @@ aspeed_sdhci_configure_phase(struct sdhci_host
> > *host, unsigned long rate)
> > aspeed_sdc_set_phase_taps(sdhci->parent, sdhci->phase_desc, taps);
> > dev_dbg(dev,
> > "Using taps [%d, %d] for [%d, %d] degrees of phase correction at
> > %luHz (%d)\n",
> > - taps->in & ASPEED_SDHCI_NR_TAPS,
> > - taps->out & ASPEED_SDHCI_NR_TAPS,
> > + taps->in & ASPEED_SDHCI_MAX_TAPS,
> > + taps->out & ASPEED_SDHCI_MAX_TAPS,
> > params->in_deg, params->out_deg, rate, host->timing); }
> >
> > @@ -493,6 +518,7 @@ static const struct aspeed_sdhci_phase_desc
> > ast2600_sdhci_phase[] = {
> > .enable_mask = ASPEED_SDC_S0_PHASE_OUT_EN,
> > .enable_value = 3,
> > },
> > + .nr_taps = 15,
> > },
> > /* SDHCI/Slot 1 */
> > [1] = {
> > @@ -506,6 +532,31 @@ static const struct aspeed_sdhci_phase_desc
> > ast2600_sdhci_phase[] = {
> > .enable_mask = ASPEED_SDC_S1_PHASE_OUT_EN,
> > .enable_value = 3,
> > },
> > + .nr_taps = 15,
> > + },
> > +};
> > +
> > +static const struct aspeed_sdhci_phase_desc ast2600_emmc_phase[] = {
> > + /* eMMC slot 0 */
> > + [0] = {
> > + .in = {
> > + .tap_mask = ASPEED_SDC_S0_PHASE_IN,
> > + .enable_mask = ASPEED_SDC_S0_PHASE_IN_EN,
> > + .enable_value = 1,
> > + },
> > + .out = {
> > + .tap_mask = ASPEED_SDC_S0_PHASE_OUT,
> > + .enable_mask = ASPEED_SDC_S0_PHASE_OUT_EN,
> > + .enable_value = 3,
> > + },
> > +
> > + /*
> > + * There are 15 taps recorded in AST2600 datasheet.
> > + * But, actually, the time period of the first tap
> > + * is two times of others. Thus, 16 tap is used to
> > + * emulate this situation.
> > + */
> > + .nr_taps = 16,
>
> I think this is a very indirect way to communicate the problem. The only time
> we look at nr_taps is in a test that explicitly compensates for the non-uniform
> delay. I think we should just have a boolean struct member called
> 'non_uniform_delay' rather than 'nr_taps', as the number of taps isn't what's
> changing. But also see the discussion below about a potential
> aspeed,tap-delays property.
>
A new property may be the better choice.
> > },
> > };
> >
> > @@ -515,10 +566,17 @@ static const struct aspeed_sdhci_pdata
> > ast2600_sdhci_pdata = {
> > .nr_phase_descs = ARRAY_SIZE(ast2600_sdhci_phase), };
> >
> > +static const struct aspeed_sdhci_pdata ast2600_emmc_pdata = {
> > + .clk_div_start = 1,
> > + .phase_desc = ast2600_emmc_phase,
> > + .nr_phase_descs = ARRAY_SIZE(ast2600_emmc_phase), };
> > +
> > static const struct of_device_id aspeed_sdhci_of_match[] = {
> > { .compatible = "aspeed,ast2400-sdhci", .data = &ast2400_sdhci_pdata, },
> > { .compatible = "aspeed,ast2500-sdhci", .data = &ast2400_sdhci_pdata, },
> > { .compatible = "aspeed,ast2600-sdhci", .data =
> > &ast2600_sdhci_pdata, },
> > + { .compatible = "aspeed,ast2600-emmc", .data = &ast2600_emmc_pdata,
> > +},
>
> This needs to be documented (and binding documentation patches need to be
> the first patches in the series).
Okay.
> Something further to consider is whether we
> separate the compatibles or add e.g. an aspeed,tap-delays property that
> specifies the specific delay of each logic element. This might take the place of
> e.g. the max-tap-delay property?
>
Yes, an additional property may be better.
> Andrew
>
> > { }
> > };
> >
> > @@ -562,6 +620,11 @@ static int aspeed_sdc_probe(struct platform_device
> *pdev)
> > goto err_clk;
> > }
> >
> > + ret = of_property_read_u32(pdev->dev.of_node, "max-tap-delay",
> > + &sdc->max_tap_delay_ps);
> > + if (ret)
> > + sdc->max_tap_delay_ps = 0;
> > +
> > dev_set_drvdata(&pdev->dev, sdc);
> >
> > parent = pdev->dev.of_node;
> > --
> > 2.17.1
Chin-Ting
Hi Chin-Ting,
I've had another think about this, see my comments below.
On Sat, 6 Nov 2021, at 20:35, Chin-Ting Kuo wrote:
> Hi Andrew,
>> > struct aspeed_sdhci_pdata {
>> > @@ -158,43 +160,60 @@ aspeed_sdc_set_phase_taps(struct aspeed_sdc
>> > *sdc, }
>> >
>> > #define PICOSECONDS_PER_SECOND 1000000000000ULL
>> > -#define ASPEED_SDHCI_NR_TAPS 15
>> > -/* Measured value with *handwave* environmentals and static loading */
>> > -#define ASPEED_SDHCI_MAX_TAP_DELAY_PS 1253
>> > +#define ASPEED_SDHCI_MAX_TAPS 15
>>
>> Why are we renaming this? It looks to cause a bit of noise in the diff.
>>
>
> Okay, it can be changed back to the original one in the next patch version.
Well, maybe it makes sense, but I think we have to take into account
how we describe the taps in the discussion below.
>> > - if (phase_deg >= 180) {
>> > - inverted = ASPEED_SDHCI_TAP_PARAM_INVERT_CLK;
>> > - phase_deg -= 180;
>> > - dev_dbg(dev,
>> > - "Inverting clock to reduce phase correction from %d to %d
>> degrees\n",
>> > - phase_deg + 180, phase_deg);
>> > - } else {
>> > - inverted = 0;
>> > + prop_delay_ps = sdc->max_tap_delay_ps / nr_taps;
>> > + clk_period_ps = div_u64(PICOSECONDS_PER_SECOND, (u64)rate_hz);
>> > +
>> > + /*
>> > + * For ast2600, if clock phase degree is negative, clock signal is
>> > + * output from falling edge first by default. Namely, clock signal
>> > + * is leading to data signal by 90 degrees at least.
>> > + */
>>
>> Have I missed something about a asymmetric clock timings? Otherwise the
>> falling edge is 180 degrees away from the rising edge? I'm still not clear on
>> why 90 degrees is used here.
>>
>
> Oh, you are right. It should be 180 degrees.
>
>> > + if (invert) {
>> > + if (phase_deg >= 90)
>> > + phase_deg -= 90;
>> > + else
>> > + phase_deg = 0;
>>
>> Why are we throwing away information?
>>
>
> With the above correction, it should be modified as below.
> If the "invert" is needed, we expect that its value should be greater than 180
> degrees. We clear "phase_deg" if its value is unexpected. Maybe, a warning
> should be shown and -EINVAL can be returned.
>
> if (invert) {
> if (phase_deg >= 180)
> phase_deg -= 180;
> else
> phase_deg = 0;
Though we want this to return the EINVAL right?
\>> > + /*
>> > + * There are 15 taps recorded in AST2600 datasheet.
>> > + * But, actually, the time period of the first tap
>> > + * is two times of others. Thus, 16 tap is used to
>> > + * emulate this situation.
>> > + */
>> > + .nr_taps = 16,
>>
>> I think this is a very indirect way to communicate the problem. The only time
>> we look at nr_taps is in a test that explicitly compensates for the non-uniform
>> delay. I think we should just have a boolean struct member called
>> 'non_uniform_delay' rather than 'nr_taps', as the number of taps isn't what's
>> changing. But also see the discussion below about a potential
>> aspeed,tap-delays property.
>>
>
> A new property may be the better choice.
I think I'm changing my mind on this sorry.
I'm think that aiming for fewer custom devicetree properties is better.
Using SoC-specific and device-specific compatibles (i.e. to
differentiate between the eMMC and SD controllers in the 2600) we can
just encode this data straight in the driver using the OF match data.
Rob and/or Joel: Thoughts?
>
>> Something further to consider is whether we
>> separate the compatibles or add e.g. an aspeed,tap-delays property that
>> specifies the specific delay of each logic element. This might take the place of
>> e.g. the max-tap-delay property?
>>
>
> Yes, an additional property may be better.
Again, flip-flopping on this a bit, the aspeed,ast2600-emmc compatible
is probably fine, and we add the tap delays in the OF match data for
the compatible. This means we get rid of any aspeed-specific devictree
properties with respect to the delays.
Andrew