diff mbox series

[v5,1/6] mmc: core: Add helper for parsing clock phase properties

Message ID 20201208012615.2717412-2-andrew@aj.id.au (mailing list archive)
State New, archived
Headers show
Series mmc: sdhci-of-aspeed: Expose phase delay tuning | expand

Commit Message

Andrew Jeffery Dec. 8, 2020, 1:26 a.m. UTC
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 <andrew@aj.id.au>
---
 drivers/mmc/core/host.c  | 44 ++++++++++++++++++++++++++++++++++++++++
 include/linux/mmc/host.h | 17 ++++++++++++++++
 2 files changed, 61 insertions(+)

Comments

Ulf Hansson Dec. 14, 2020, 3:48 p.m. UTC | #1
On Tue, 8 Dec 2020 at 02:26, Andrew Jeffery <andrew@aj.id.au> 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 <andrew@aj.id.au>
> ---
>  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
Andrew Jeffery Dec. 14, 2020, 11:31 p.m. UTC | #2
On Tue, 15 Dec 2020, at 02:18, Ulf Hansson wrote:
> On Tue, 8 Dec 2020 at 02:26, Andrew Jeffery <andrew@aj.id.au> 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 <andrew@aj.id.au>
> > ---
> >  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
diff mbox series

Patch

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