Re: [PATCH v3] mmc: renesas_sdhi: Fix change point of data handling

[Wolfram Sang <wsa+renesas@sang-engineering.com>]

[-- Attachment #1: Type: text/plain, Size: 3284 bytes --]

Hi Claudiu,

I got more information about SMPCMP now. I had a misunderstanding there.
According to your patch description, you might have the same
misunderstanding? Let me quote again:

===
RZ hardware manual are similar on this chapter), at the time of tuning,
data is captured by the previous and next TAPs and the result is stored in
the SMPCMP register (previous TAP in bits 22..16, next TAP in bits 7..0).
===

It is not the previous and next TAP but the previous and next clock
cycle using the *same* TAP. And the bits in the register describe if
there was a mismatch in the data bits across these clock cycles.

So, we really want SMPCMP to be 0 because the data should be stable
across all three clock cycles of the same TAP.

> As of my understanding the TAP where cmpngu = 0x0e and cmpngd=0x0e is not
> considered change point of the input data. For that to happen it would mean
> that cmpngu != cmpngd.

I am not sure you can assume that cmpngu != cmpngd is always true for a
change point. I'd think it is likely often the case. But always? I am
not convinced. But I am convinced that if SMPCMP is 0, this is a good
TAP because it was stable over these clock cycles.

> From this snapshot, datasheet and our discussions:
> 
> i=0, cmpngu=00000000, cmpngd=00000000, smpcmp=00000000
> i=1, cmpngu=00000000, cmpngd=00000000, smpcmp=00000000
> i=2, cmpngu=0000000e, cmpngd=0000000e, smpcmp=000e000e
> i=3, cmpngu=00000000, cmpngd=00000000, smpcmp=00000000
> *i=4, cmpngu=00000000, cmpngd=00000002, smpcmp=00000002*
> *i=5, cmpngu=00000000, cmpngd=000000ff, smpcmp=000001ff*
> *i=6, cmpngu=000000ff, cmpngd=00000000, smpcmp=01ff0000*
> i=7, cmpngu=00000000, cmpngd=00000000, smpcmp=00000000
> i=8, cmpngu=00000000, cmpngd=00000000, smpcmp=00000000
> i=9, cmpngu=00000000, cmpngd=00000000, smpcmp=00000000
> i=10, cmpngu=00000000, cmpngd=00000000, smpcmp=00000000
> i=11, cmpngu=00000000, cmpngd=00000000, smpcmp=00000000
> *i=12, cmpngu=00000000, cmpngd=00000002, smpcmp=00000002*
> *i=13, cmpngu=00000000, cmpngd=000000ff, smpcmp=000001ff*
> *i=14, cmpngu=000000ff, cmpngd=00000000, smpcmp=01ff0000*
> i=15, cmpngu=00000000, cmpngd=00000000, smpcmp=00000000
> 
> I understand that TAP4,5,6 are change point of the input data and
> TAP8,0,1,2,3 are candidates for being selected, TAP 1,2 being the best
> (please correct me if I'm wrong).

I agree that TAP4-6 are the change point. TAP2 could be a candidate. I
dunno why SMPCMP is non-zero at i == 2, maybe some glitch due to noise
on the board?

I do really wonder why probing failed, though? TAP1 sounds like a good
choice as well. I mean we consider SMPCMP only if all TAPs are good. So,
if probing fails, that means that SMPCMP was non-zero all the time?

That being said, our code to select the best TAP from SMPCMP is really
not considering the change point :( It just picks the first one where
SMPCMP is 0. We are not checking where the change point is and try to be
as far away as possible.

> root@smarc-rzg3s:~# md5sum out test
> b053723af63801e665959d48cb7bd8e6  out
> b053723af63801e665959d48cb7bd8e6  test
> 
> Do yo consider this enough?

Yes, if done 100 times ;)

I hope this mail was helpful?

Thanks and happy hacking,

   Wolfram


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]