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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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




[Index of Archives]     [Linux Memonry Technology]     [Linux USB Devel]     [Linux Media]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]

  Powered by Linux