Hello,
This series implements support for the MMC core clk-phase-* devicetree bindings
in the Aspeed SD/eMMC driver. The relevant register was exposed on the AST2600
and is present for both the SD/MMC controller and the dedicated eMMC
controller.
v5 fixes some build issues identified by the kernel test robot.
v4 can be found here:
https://lore.kernel.org/linux-mmc/[email protected]/
The series has had light testing on an AST2600-based platform which requires
180deg of input and output clock phase correction at HS200, as well as some
synthetic testing under qemu and KUnit.
Please review!
Cheers,
Andrew
Andrew Jeffery (6):
mmc: core: Add helper for parsing clock phase properties
mmc: sdhci-of-aspeed: Expose clock phase controls
mmc: sdhci-of-aspeed: Add AST2600 bus clock support
mmc: sdhci-of-aspeed: Add KUnit tests for phase calculations
MAINTAINERS: Add entry for the ASPEED SD/MMC driver
ARM: dts: rainier: Add eMMC clock phase compensation
MAINTAINERS | 9 +
arch/arm/boot/dts/aspeed-bmc-ibm-rainier.dts | 1 +
drivers/mmc/core/host.c | 44 ++++
drivers/mmc/host/Kconfig | 14 ++
drivers/mmc/host/Makefile | 1 +
drivers/mmc/host/sdhci-of-aspeed-test.c | 100 ++++++++
drivers/mmc/host/sdhci-of-aspeed.c | 251 ++++++++++++++++++-
include/linux/mmc/host.h | 17 ++
8 files changed, 426 insertions(+), 11 deletions(-)
create mode 100644 drivers/mmc/host/sdhci-of-aspeed-test.c
--
2.27.0
Drivers for MMC hosts that accept phase corrections can take advantage
of the helper by embedding a mmc_clk_phase_map_t object in their
private data and invoking mmc_of_parse_clk_phase() to extract phase
parameters. It is the responsibility of the host driver to translate and
apply the extracted values to hardware as required.
Signed-off-by: Andrew Jeffery <[email protected]>
---
drivers/mmc/core/host.c | 44 ++++++++++++++++++++++++++++++++++++++++
include/linux/mmc/host.h | 17 ++++++++++++++++
2 files changed, 61 insertions(+)
diff --git a/drivers/mmc/core/host.c b/drivers/mmc/core/host.c
index 96b2ca1f1b06..b1697f00c4b5 100644
--- a/drivers/mmc/core/host.c
+++ b/drivers/mmc/core/host.c
@@ -163,6 +163,50 @@ static void mmc_retune_timer(struct timer_list *t)
mmc_retune_needed(host);
}
+static void mmc_of_parse_timing_phase(struct device *dev, const char *prop,
+ struct mmc_clk_phase *phase)
+{
+ int degrees[2] = {0};
+ int rc;
+
+ rc = device_property_read_u32_array(dev, prop, degrees, 2);
+ phase->valid = !rc;
+ if (phase->valid) {
+ phase->in_deg = degrees[0];
+ phase->out_deg = degrees[1];
+ }
+}
+
+void
+mmc_of_parse_clk_phase(struct mmc_host *host, mmc_clk_phase_map_t map)
+{
+ struct device *dev = host->parent;
+
+ mmc_of_parse_timing_phase(dev, "clk-phase-legacy",
+ &map[MMC_TIMING_LEGACY]);
+ mmc_of_parse_timing_phase(dev, "clk-phase-mmc-hs",
+ &map[MMC_TIMING_MMC_HS]);
+ mmc_of_parse_timing_phase(dev, "clk-phase-sd-hs",
+ &map[MMC_TIMING_SD_HS]);
+ mmc_of_parse_timing_phase(dev, "clk-phase-uhs-sdr12",
+ &map[MMC_TIMING_UHS_SDR12]);
+ mmc_of_parse_timing_phase(dev, "clk-phase-uhs-sdr25",
+ &map[MMC_TIMING_UHS_SDR25]);
+ mmc_of_parse_timing_phase(dev, "clk-phase-uhs-sdr50",
+ &map[MMC_TIMING_UHS_SDR50]);
+ mmc_of_parse_timing_phase(dev, "clk-phase-uhs-sdr104",
+ &map[MMC_TIMING_UHS_SDR104]);
+ mmc_of_parse_timing_phase(dev, "clk-phase-uhs-ddr50",
+ &map[MMC_TIMING_UHS_DDR50]);
+ mmc_of_parse_timing_phase(dev, "clk-phase-mmc-ddr52",
+ &map[MMC_TIMING_MMC_DDR52]);
+ mmc_of_parse_timing_phase(dev, "clk-phase-mmc-hs200",
+ &map[MMC_TIMING_MMC_HS200]);
+ mmc_of_parse_timing_phase(dev, "clk-phase-mmc-hs400",
+ &map[MMC_TIMING_MMC_HS400]);
+}
+EXPORT_SYMBOL(mmc_of_parse_clk_phase);
+
/**
* mmc_of_parse() - parse host's device-tree node
* @host: host whose node should be parsed.
diff --git a/include/linux/mmc/host.h b/include/linux/mmc/host.h
index 01bba36545c5..bc4731c9738f 100644
--- a/include/linux/mmc/host.h
+++ b/include/linux/mmc/host.h
@@ -79,6 +79,22 @@ struct mmc_ios {
bool enhanced_strobe; /* hs400es selection */
};
+struct mmc_clk_phase {
+ bool valid;
+ u16 in_deg;
+ u16 out_deg;
+};
+
+/*
+ * Define a type to map between bus timings and phase correction values. To
+ * avoid bloat in struct mmc_host we leave it to the host driver to define the
+ * phase map object in its private data if it supports phase correction.
+ * However, mmc_of_parse_clk_phase() is provided by the mmc core and needs the
+ * provided array to be correctly sized, so typedef an appropriately sized
+ * array to minimise the chance that the wrong size object is passed.
+ */
+typedef struct mmc_clk_phase mmc_clk_phase_map_t[MMC_TIMING_MMC_HS400 + 1];
+
struct mmc_host;
struct mmc_host_ops {
@@ -490,6 +506,7 @@ struct mmc_host *mmc_alloc_host(int extra, struct device *);
int mmc_add_host(struct mmc_host *);
void mmc_remove_host(struct mmc_host *);
void mmc_free_host(struct mmc_host *);
+void mmc_of_parse_clk_phase(struct mmc_host *host, mmc_clk_phase_map_t map);
int mmc_of_parse(struct mmc_host *host);
int mmc_of_parse_voltage(struct device_node *np, u32 *mask);
--
2.27.0
On Tue, 8 Dec 2020 at 02:26, Andrew Jeffery <[email protected]> wrote:
>
> Drivers for MMC hosts that accept phase corrections can take advantage
> of the helper by embedding a mmc_clk_phase_map_t object in their
> private data and invoking mmc_of_parse_clk_phase() to extract phase
> parameters. It is the responsibility of the host driver to translate and
> apply the extracted values to hardware as required.
>
> Signed-off-by: Andrew Jeffery <[email protected]>
> ---
> drivers/mmc/core/host.c | 44 ++++++++++++++++++++++++++++++++++++++++
> include/linux/mmc/host.h | 17 ++++++++++++++++
> 2 files changed, 61 insertions(+)
>
> diff --git a/drivers/mmc/core/host.c b/drivers/mmc/core/host.c
> index 96b2ca1f1b06..b1697f00c4b5 100644
> --- a/drivers/mmc/core/host.c
> +++ b/drivers/mmc/core/host.c
> @@ -163,6 +163,50 @@ static void mmc_retune_timer(struct timer_list *t)
> mmc_retune_needed(host);
> }
>
> +static void mmc_of_parse_timing_phase(struct device *dev, const char *prop,
> + struct mmc_clk_phase *phase)
> +{
> + int degrees[2] = {0};
> + int rc;
> +
> + rc = device_property_read_u32_array(dev, prop, degrees, 2);
> + phase->valid = !rc;
> + if (phase->valid) {
> + phase->in_deg = degrees[0];
> + phase->out_deg = degrees[1];
> + }
> +}
> +
> +void
> +mmc_of_parse_clk_phase(struct mmc_host *host, mmc_clk_phase_map_t map)
Would you mind to change to pass a "struct mmc_clk_phase_map *map" to this?
See more comments below.
> +{
> + struct device *dev = host->parent;
> +
> + mmc_of_parse_timing_phase(dev, "clk-phase-legacy",
> + &map[MMC_TIMING_LEGACY]);
> + mmc_of_parse_timing_phase(dev, "clk-phase-mmc-hs",
> + &map[MMC_TIMING_MMC_HS]);
> + mmc_of_parse_timing_phase(dev, "clk-phase-sd-hs",
> + &map[MMC_TIMING_SD_HS]);
> + mmc_of_parse_timing_phase(dev, "clk-phase-uhs-sdr12",
> + &map[MMC_TIMING_UHS_SDR12]);
> + mmc_of_parse_timing_phase(dev, "clk-phase-uhs-sdr25",
> + &map[MMC_TIMING_UHS_SDR25]);
> + mmc_of_parse_timing_phase(dev, "clk-phase-uhs-sdr50",
> + &map[MMC_TIMING_UHS_SDR50]);
> + mmc_of_parse_timing_phase(dev, "clk-phase-uhs-sdr104",
> + &map[MMC_TIMING_UHS_SDR104]);
> + mmc_of_parse_timing_phase(dev, "clk-phase-uhs-ddr50",
> + &map[MMC_TIMING_UHS_DDR50]);
> + mmc_of_parse_timing_phase(dev, "clk-phase-mmc-ddr52",
> + &map[MMC_TIMING_MMC_DDR52]);
> + mmc_of_parse_timing_phase(dev, "clk-phase-mmc-hs200",
> + &map[MMC_TIMING_MMC_HS200]);
> + mmc_of_parse_timing_phase(dev, "clk-phase-mmc-hs400",
> + &map[MMC_TIMING_MMC_HS400]);
> +}
> +EXPORT_SYMBOL(mmc_of_parse_clk_phase);
> +
> /**
> * mmc_of_parse() - parse host's device-tree node
> * @host: host whose node should be parsed.
> diff --git a/include/linux/mmc/host.h b/include/linux/mmc/host.h
> index 01bba36545c5..bc4731c9738f 100644
> --- a/include/linux/mmc/host.h
> +++ b/include/linux/mmc/host.h
> @@ -79,6 +79,22 @@ struct mmc_ios {
> bool enhanced_strobe; /* hs400es selection */
> };
>
> +struct mmc_clk_phase {
> + bool valid;
> + u16 in_deg;
> + u16 out_deg;
> +};
> +
> +/*
> + * Define a type to map between bus timings and phase correction values. To
> + * avoid bloat in struct mmc_host we leave it to the host driver to define the
> + * phase map object in its private data if it supports phase correction.
> + * However, mmc_of_parse_clk_phase() is provided by the mmc core and needs the
> + * provided array to be correctly sized, so typedef an appropriately sized
> + * array to minimise the chance that the wrong size object is passed.
> + */
> +typedef struct mmc_clk_phase mmc_clk_phase_map_t[MMC_TIMING_MMC_HS400 + 1];
> +
Nitpick: I would appreciate if we could avoid using "typedefs", as I
think they in many cases makes the code harder to read. How about
doing this instead?
#define MMC_NUM_CLK_PHASES (MMC_TIMING_MMC_HS400 + 1)
struct mmc_clk_phase_map {
struct mmc_clk_phase phase[MMC_NUM_CLK_PHASES];
};
[...]
Kind regards
Uffe
On Tue, 8 Dec 2020 at 02:26, Andrew Jeffery <[email protected]> wrote:
>
> Hello,
>
> This series implements support for the MMC core clk-phase-* devicetree bindings
> in the Aspeed SD/eMMC driver. The relevant register was exposed on the AST2600
> and is present for both the SD/MMC controller and the dedicated eMMC
> controller.
>
> v5 fixes some build issues identified by the kernel test robot.
>
> v4 can be found here:
>
> https://lore.kernel.org/linux-mmc/[email protected]/
>
> The series has had light testing on an AST2600-based platform which requires
> 180deg of input and output clock phase correction at HS200, as well as some
> synthetic testing under qemu and KUnit.
>
> Please review!
FYI, other than the comment I had on patch1, I think the series looks
good to me.
[...]
Kind regards
Uffe
On Tue, 15 Dec 2020, at 02:18, Ulf Hansson wrote:
> On Tue, 8 Dec 2020 at 02:26, Andrew Jeffery <[email protected]> wrote:
> >
> > Drivers for MMC hosts that accept phase corrections can take advantage
> > of the helper by embedding a mmc_clk_phase_map_t object in their
> > private data and invoking mmc_of_parse_clk_phase() to extract phase
> > parameters. It is the responsibility of the host driver to translate and
> > apply the extracted values to hardware as required.
> >
> > Signed-off-by: Andrew Jeffery <[email protected]>
> > ---
> > drivers/mmc/core/host.c | 44 ++++++++++++++++++++++++++++++++++++++++
> > include/linux/mmc/host.h | 17 ++++++++++++++++
> > 2 files changed, 61 insertions(+)
> >
> > diff --git a/drivers/mmc/core/host.c b/drivers/mmc/core/host.c
> > index 96b2ca1f1b06..b1697f00c4b5 100644
> > --- a/drivers/mmc/core/host.c
> > +++ b/drivers/mmc/core/host.c
> > @@ -163,6 +163,50 @@ static void mmc_retune_timer(struct timer_list *t)
> > mmc_retune_needed(host);
> > }
> >
> > +static void mmc_of_parse_timing_phase(struct device *dev, const char *prop,
> > + struct mmc_clk_phase *phase)
> > +{
> > + int degrees[2] = {0};
> > + int rc;
> > +
> > + rc = device_property_read_u32_array(dev, prop, degrees, 2);
> > + phase->valid = !rc;
> > + if (phase->valid) {
> > + phase->in_deg = degrees[0];
> > + phase->out_deg = degrees[1];
> > + }
> > +}
> > +
> > +void
> > +mmc_of_parse_clk_phase(struct mmc_host *host, mmc_clk_phase_map_t map)
>
> Would you mind to change to pass a "struct mmc_clk_phase_map *map" to this?
>
> See more comments below.
>
> > +{
> > + struct device *dev = host->parent;
> > +
> > + mmc_of_parse_timing_phase(dev, "clk-phase-legacy",
> > + &map[MMC_TIMING_LEGACY]);
> > + mmc_of_parse_timing_phase(dev, "clk-phase-mmc-hs",
> > + &map[MMC_TIMING_MMC_HS]);
> > + mmc_of_parse_timing_phase(dev, "clk-phase-sd-hs",
> > + &map[MMC_TIMING_SD_HS]);
> > + mmc_of_parse_timing_phase(dev, "clk-phase-uhs-sdr12",
> > + &map[MMC_TIMING_UHS_SDR12]);
> > + mmc_of_parse_timing_phase(dev, "clk-phase-uhs-sdr25",
> > + &map[MMC_TIMING_UHS_SDR25]);
> > + mmc_of_parse_timing_phase(dev, "clk-phase-uhs-sdr50",
> > + &map[MMC_TIMING_UHS_SDR50]);
> > + mmc_of_parse_timing_phase(dev, "clk-phase-uhs-sdr104",
> > + &map[MMC_TIMING_UHS_SDR104]);
> > + mmc_of_parse_timing_phase(dev, "clk-phase-uhs-ddr50",
> > + &map[MMC_TIMING_UHS_DDR50]);
> > + mmc_of_parse_timing_phase(dev, "clk-phase-mmc-ddr52",
> > + &map[MMC_TIMING_MMC_DDR52]);
> > + mmc_of_parse_timing_phase(dev, "clk-phase-mmc-hs200",
> > + &map[MMC_TIMING_MMC_HS200]);
> > + mmc_of_parse_timing_phase(dev, "clk-phase-mmc-hs400",
> > + &map[MMC_TIMING_MMC_HS400]);
> > +}
> > +EXPORT_SYMBOL(mmc_of_parse_clk_phase);
> > +
> > /**
> > * mmc_of_parse() - parse host's device-tree node
> > * @host: host whose node should be parsed.
> > diff --git a/include/linux/mmc/host.h b/include/linux/mmc/host.h
> > index 01bba36545c5..bc4731c9738f 100644
> > --- a/include/linux/mmc/host.h
> > +++ b/include/linux/mmc/host.h
> > @@ -79,6 +79,22 @@ struct mmc_ios {
> > bool enhanced_strobe; /* hs400es selection */
> > };
> >
> > +struct mmc_clk_phase {
> > + bool valid;
> > + u16 in_deg;
> > + u16 out_deg;
> > +};
> > +
> > +/*
> > + * Define a type to map between bus timings and phase correction values. To
> > + * avoid bloat in struct mmc_host we leave it to the host driver to define the
> > + * phase map object in its private data if it supports phase correction.
> > + * However, mmc_of_parse_clk_phase() is provided by the mmc core and needs the
> > + * provided array to be correctly sized, so typedef an appropriately sized
> > + * array to minimise the chance that the wrong size object is passed.
> > + */
> > +typedef struct mmc_clk_phase mmc_clk_phase_map_t[MMC_TIMING_MMC_HS400 + 1];
> > +
>
> Nitpick: I would appreciate if we could avoid using "typedefs", as I
> think they in many cases makes the code harder to read. How about
> doing this instead?
>
> #define MMC_NUM_CLK_PHASES (MMC_TIMING_MMC_HS400 + 1)
>
> struct mmc_clk_phase_map {
> struct mmc_clk_phase phase[MMC_NUM_CLK_PHASES];
> };
>
> [...]
Right; I experimented with that approach and felt it was kinda clunky (hence
the typedef), but I'll respin the series doing as such.
Thanks,
Andrew