openbmc.lists.ozlabs.org archive mirror
 help / color / mirror / Atom feed
From: "Andrew Jeffery" <andrew@aj.id.au>
To: "Chin-Ting Kuo" <chin-ting_kuo@aspeedtech.com>,
	"Rob Herring" <robh+dt@kernel.org>,
	"Joel Stanley" <joel@jms.id.au>,
	"Michael Turquette" <mturquette@baylibre.com>,
	"Stephen Boyd" <sboyd@kernel.org>,
	"Adrian Hunter" <adrian.hunter@intel.com>,
	 "linux-aspeed@lists.ozlabs.org" <linux-aspeed@lists.ozlabs.org>,
	"openbmc@lists.ozlabs.org" <openbmc@lists.ozlabs.org>,
	linux-mmc <linux-mmc@vger.kernel.org>,
	"devicetree@vger.kernel.org" <devicetree@vger.kernel.org>,
	"linux-arm-kernel@lists.infradead.org"
	<linux-arm-kernel@lists.infradead.org>,
	 "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
	"linux-clk@vger.kernel.org" <linux-clk@vger.kernel.org>
Cc: BMC-SW <BMC-SW@aspeedtech.com>, Steven Lee <steven_lee@aspeedtech.com>
Subject: Re: [PATCH 05/10] mmc: aspeed: Adjust delay taps calculation method
Date: Mon, 08 Nov 2021 10:12:38 +1030	[thread overview]
Message-ID: <30748da1-0fdb-40c1-bf28-8682d3a89c73@www.fastmail.com> (raw)
In-Reply-To: <HK0PR06MB2786C1ED2463764EAAEA7166B28F9@HK0PR06MB2786.apcprd06.prod.outlook.com>

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

  reply	other threads:[~2021-11-07 23:43 UTC|newest]

Thread overview: 31+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-09-22 10:31 [PATCH 00/10] ASPEED SD/eMMC controller clock configuration Chin-Ting Kuo
2021-09-22 10:31 ` [PATCH 01/10] clk: aspeed: ast2600: Porting sdhci clock source Chin-Ting Kuo
2021-09-23  0:02   ` Joel Stanley
2021-09-23  5:31     ` Chin-Ting Kuo
2021-10-26  6:10   ` Paul Menzel
2021-11-26  2:27     ` Chin-Ting Kuo
2021-09-22 10:31 ` [PATCH 02/10] sdhci: aspeed: Add SDR50 support Chin-Ting Kuo
2021-10-26  0:31   ` Andrew Jeffery
2021-11-06 10:01     ` Chin-Ting Kuo
2021-09-22 10:31 ` [PATCH 03/10] dts: aspeed: ast2600: Support SDR50 for SD device Chin-Ting Kuo
2021-10-26  0:42   ` Andrew Jeffery
2021-11-06 10:01     ` Chin-Ting Kuo
2021-09-22 10:31 ` [PATCH 04/10] mmc: Add invert flag for clock phase signedness Chin-Ting Kuo
2021-10-26  0:52   ` Andrew Jeffery
2021-11-06 10:02     ` Chin-Ting Kuo
2021-11-08  0:21       ` Andrew Jeffery
2021-09-22 10:31 ` [PATCH 05/10] mmc: aspeed: Adjust delay taps calculation method Chin-Ting Kuo
2021-10-26  3:10   ` Andrew Jeffery
2021-11-06 10:05     ` Chin-Ting Kuo
2021-11-07 23:42       ` Andrew Jeffery [this message]
2021-09-22 10:31 ` [PATCH 06/10] arm: dts: aspeed: Change eMMC device compatible Chin-Ting Kuo
2021-09-22 10:31 ` [PATCH 07/10] arm: dts: aspeed: Adjust clock phase parameter Chin-Ting Kuo
2021-09-22 10:31 ` [PATCH 08/10] arm: dts: ibm: " Chin-Ting Kuo
2021-09-22 10:31 ` [PATCH 09/10] dt-bindings: mmc: aspeed: Add max-tap-delay property Chin-Ting Kuo
2021-09-27 18:40   ` Rob Herring
2021-09-28  2:50     ` Chin-Ting Kuo
2021-09-22 10:31 ` [PATCH 10/10] dt-bindings: mmc: aspeed: Add a new compatible string Chin-Ting Kuo
2021-09-27 18:59   ` Rob Herring
2021-09-28  2:50     ` Chin-Ting Kuo
2021-09-28 22:28       ` Rob Herring
2021-09-29  3:03         ` Chin-Ting Kuo

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=30748da1-0fdb-40c1-bf28-8682d3a89c73@www.fastmail.com \
    --to=andrew@aj.id.au \
    --cc=BMC-SW@aspeedtech.com \
    --cc=adrian.hunter@intel.com \
    --cc=chin-ting_kuo@aspeedtech.com \
    --cc=devicetree@vger.kernel.org \
    --cc=joel@jms.id.au \
    --cc=linux-arm-kernel@lists.infradead.org \
    --cc=linux-aspeed@lists.ozlabs.org \
    --cc=linux-clk@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-mmc@vger.kernel.org \
    --cc=mturquette@baylibre.com \
    --cc=openbmc@lists.ozlabs.org \
    --cc=robh+dt@kernel.org \
    --cc=sboyd@kernel.org \
    --cc=steven_lee@aspeedtech.com \
    --subject='Re: [PATCH 05/10] mmc: aspeed: Adjust delay taps calculation method' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).