RE: [PATCH 05/10] mmc: aspeed: Adjust delay taps calculation method

From: Chin-Ting Kuo
Date: Sat Nov 06 2021 - 06:05:26 EST

Next message: Kieran Bingham: "Re: [PATCH v2] Input: add 'safe' user switch codes"
Previous message: Chin-Ting Kuo: "RE: [PATCH 04/10] mmc: Add invert flag for clock phase signedness"
Next in thread: Andrew Jeffery: "Re: [PATCH 05/10] mmc: aspeed: Adjust delay taps calculation method"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

Hi Andrew,

> -----Original Message-----
> From: Andrew Jeffery <andrew@xxxxxxxx>
> 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 <chin-ting_kuo@xxxxxxxxxxxxxx>
> > ---
> > 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

Next message: Kieran Bingham: "Re: [PATCH v2] Input: add 'safe' user switch codes"
Previous message: Chin-Ting Kuo: "RE: [PATCH 04/10] mmc: Add invert flag for clock phase signedness"
Next in thread: Andrew Jeffery: "Re: [PATCH 05/10] mmc: aspeed: Adjust delay taps calculation method"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]