Received-SPF: pass (google.com: domain of linux-kernel+bounces-55025-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45e3:2400::1 as permitted sender) client-ip=2604:1380:45e3:2400::1;
Message-ID: <63e1eabd-a484-48ee-b8db-1e460bce70ab@tuxon.dev>
Date: Tue, 6 Feb 2024 15:35:19 +0200
Precedence: bulk
MIME-Version: 1.0
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH v3] mmc: renesas_sdhi: Fix change point of data handling
Content-Language: en-US
To: Wolfram Sang <wsa+renesas@sang-engineering.com>, ulf.hansson@linaro.org,
 yoshihiro.shimoda.uh@renesas.com, masaharu.hayakawa.ry@renesas.com,
 takeshi.saito.xv@renesas.com, linux-mmc@vger.kernel.org,
 linux-renesas-soc@vger.kernel.org, linux-kernel@vger.kernel.org,
 Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
References: <20240205112702.213050-1-claudiu.beznea.uj@bp.renesas.com>
 <ZcDdn2AVz8FIXzak@shikoro> <237bd5c8-184d-4e46-ba66-253e3ef0c895@tuxon.dev>
 <ZcD17mTRnfIaueAW@shikoro>
From: claudiu beznea <claudiu.beznea@tuxon.dev>
In-Reply-To: <ZcD17mTRnfIaueAW@shikoro>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 7bit

Hi, Wolfram,

On 05.02.2024 16:51, Wolfram Sang wrote:
> 
>>> According to my understanding, we should only mark this TAP good if it
>>> is in the range 5-7. I need to double check with Renesas, though.
>>
>> OK, my understanding is that it should be in the middle (beginning being
>> the tap that triggered change point of the input data, end being the next
>> tap with the same ID). This is what I understand from this: "As the width
>> of the input data is 1 (UI), select TAP6 or TAP7 which is
>>
>> *the median* of next TAP3 from TAP3."
> 
> Yes, I agree. With 0x0e, that means TAP1+2+3 are changing points and we
> should be far away from them, like 5-7.

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.

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

> 
> But: I am still waiting for Renesas to answer my questions regarding
> SMPCMP. I'd like to get that first, so we have clear facts then.
> 
>>> Boot failure is one test. Read/write tests should be another, I think.
>>
>> OK, I'll try also read/write. Do you have in mind something particular?
> 
> Nope. Just consistency checks.
> 
>>> Because if we select a bad TAP, bad things might happen later. To reduce
>>> the amount of testing, read/write testing could only be triggered if the
>>> new code path was excecuted?
>>
>> I'm not sure how to trigger that (or maybe I haven't understood your
>> statement...)
> 
> I thought something in the lines of:
> 
> - print out when you needed SMPCMP to select a TAP

On my device (RZ/G3S) that triggered initially "mmc0: tuning execution
failed" at probe, with this patch (when doing read/write tests) I have a
lot of moment when cmpngu == cmpngd and thus the smpcmp bitmask is populated.

With RZ/G3S+rootfs on eMMC and this patch I did the following read/write test:

root@smarc-rzg3s:~# dd if=/dev/random of=out bs=1024 count=1048576
1048576+0 records in
1048576+0 records out
root@smarc-rzg3s:~#
root@smarc-rzg3s:~# dd if=out of=test bs=1024 count=1048576
1048576+0 records in
1048576+0 records out
root@smarc-rzg3s:~#
root@smarc-rzg3s:~#
root@smarc-rzg3s:~#
root@smarc-rzg3s:~# md5sum out test
b053723af63801e665959d48cb7bd8e6  out
b053723af63801e665959d48cb7bd8e6  test

Do yo consider this enough?

Thank you,
Claudiu Beznea

> - check the log for that printout
> - if (printout) do read_write_tests
> 
> Dunno if that makes sense with your test setup.
>