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

- 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

- 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.

Why are we renaming this? It looks to cause a bit of noise in the diff.

+
 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.

 {
 	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?

-	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.

+	if (invert) {
+		if (phase_deg >= 90)
+			phase_deg -= 90;
+		else
+			phase_deg = 0;

Why are we throwing away information?

 	}

-	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

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.

This needs to be documented (and binding documentation patches need to 
be the first patches in the series). 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?

Andrew