by Judith Mendez

[permalink] [raw]

Subject: Re: [PATCH v2 1/7] mmc: sdhci_am654: Add tuning algorithm for delay chain

Hello Adrian,

On 2/28/24 7:21 AM, Adrian Hunter wrote:
> On 20/02/24 22:10, Judith Mendez wrote:
>> On 2/16/24 11:09 AM, Adrian Hunter wrote:
>>> On 7/02/24 03:15, Judith Mendez wrote:
>>>> +
>>>> +    if (!num_fails)
>>>> +        return ITAPDLY_LAST_INDEX >> 1;
>>>> +
>>>> +    if (fail_window->length == ITAPDLY_LENGTH) {
>>>> +        dev_err(dev, "No passing ITAPDLY, return 0\n");
>>>> +        return 0;
>>>> +    }
>>>> +
>>>> +    first_fail_start = fail_window->start;
>>>> +    last_fail_end = fail_window[num_fails - 1].end;
>>>> +
>>>> +    for (i = 0; i < num_fails; i++) {
>>>> +        start_fail = fail_window[i].start;
>>>> +        end_fail = fail_window[i].end;
>>>> +        pass_length = start_fail - (prev_fail_end + 1);
>>>> +
>>>> +        if (pass_length > pass_window.length) {
>>>> +            pass_window.start = prev_fail_end + 1;
>>>> +            pass_window.length = pass_length;
>>>> +        }
>>>> +        prev_fail_end = end_fail;
>>>> +    }
>>>> +
>>>> +    if (!circular_buffer)
>>>> +        pass_length = ITAPDLY_LAST_INDEX - last_fail_end;
>>>> +    else
>>>> +        pass_length = ITAPDLY_LAST_INDEX - last_fail_end + first_fail_start;
>>>> +
>>>> +    if (pass_length > pass_window.length) {
>>>> +        pass_window.start = last_fail_end + 1;
>>>> +        pass_window.length = pass_length;
>>>> +    }
>>>> +
>>>> +    if (!circular_buffer)
>>>> +        itap = pass_window.start + (pass_window.length >> 1);
>>>> +    else
>>>> +        itap = (pass_window.start + (pass_window.length >> 1)) % ITAPDLY_LENGTH;
>>>> +
>>>> +    return (itap < 0 || itap > ITAPDLY_LAST_INDEX ? 0 : itap);
>>>
>>> Parentheses are not needed where they are but putting
>>> them around the condition would make it more readable e.g.
>>>
>>>     return (itap < 0 || itap > ITAPDLY_LAST_INDEX) ? 0 : itap;
>>>
>>> However (itap < 0) is not possible because itap is an unsigned type
>>> and if (itap > ITAPDLY_LAST_INDEX) then maybe it would be better
>>> to return ITAPDLY_LAST_INDEX
>>
>> You are right about itap < 0, thanks will fix.
>>
>> About itap > ITAPDLY_LAST_INDEX, this is an error. Why
>> return ITAPDLY_LAST_INDEX instead of 0?
>
> It doesn't matter. Just if a value has a better chance to work
> if the calculation fails, like maybe ITAPDLY_LAST_INDEX / 2, but
> presumably it should not fail.

Ok, ITAPDLY_LAST_INDEX / sounds good to me, I will add this instead.

Thanks,
~ Judith

2024-02-28 15:42:00

by Judith Mendez

[permalink] [raw]

Subject: Re: [PATCH v2 2/7] mmc: sdhci_am654: Write ITAPDLY for DDR52 timing

Hi Adrian,

On 2/28/24 7:21 AM, Adrian Hunter wrote:
> On 20/02/24 23:05, Judith Mendez wrote:
>> Hi Adrian,
>>
>> On 2/16/24 11:09 AM, Adrian Hunter wrote:
>>> On 7/02/24 03:15, Judith Mendez wrote:
>>>> For DDR52 timing, DLL is enabled but tuning is not carried
>>>> out, therefore the ITAPDLY value in PHY CTRL 4 register is
>>>> not correct. Fix this by writing ITAPDLY after enabling DLL.
>>>>
>>>> Fixes: a161c45f2979 ("mmc: sdhci_am654: Enable DLL only for some speed modes")
>>>
>>> Note that the Fixes tags make a different ordering
>>> than the patch order i.e.
>>>
>>> Patch    Fixes        in version
>>> 1     13ebeae68ac9     v5.10-rc1
>>> 2     a161c45f2979     v5.7-rc1
>>> 3     8ee5fc0e0b3b     v5.7-rc1
>>> 4     8ee5fc0e0b3b     v5.7-rc1
>>> 4     a0a62497f6aa     v5.10-rc1
>>> 5     fe52e2fbc6ef     v5.9-rc1
>>> 6     1accbced1c32     v5.3-rc1
>>> 7     a161c45f2979     v5.7-rc1
>>>
>>> That might make backporting these patches more challenging.
>>
>> Are you suggesting to remove the fixes tag here?
>
> No, it is just something to think about if you intend to
> backport these patches to older kernels.

Thanks, Ill keep this in mind for v3.

>
>>
>>>
>>>> Signed-off-by: Judith Mendez <[email protected]>
>>>> ---
>>>> Changelog:
>>>> v1->v2:
>>>> - Call sdhci_am654_write_itapdly() in sdhci_am654_set_clock()
>>>> instead of sdhci_am654_setup_dll()
>>>> ---
>>>> drivers/mmc/host/sdhci_am654.c | 1 +
>>>> 1 file changed, 1 insertion(+)
>>>>
>>>> diff --git a/drivers/mmc/host/sdhci_am654.c b/drivers/mmc/host/sdhci_am654.c
>>>> index 2c66a965c225..b50db5d4a452 100644
>>>> --- a/drivers/mmc/host/sdhci_am654.c
>>>> +++ b/drivers/mmc/host/sdhci_am654.c
>>>> @@ -299,6 +299,7 @@ static void sdhci_am654_set_clock(struct sdhci_host *host, unsigned int clock)
>>>>       if (timing > MMC_TIMING_UHS_SDR25 && clock >= CLOCK_TOO_SLOW_HZ) {
>>>>           sdhci_am654_setup_dll(host, clock);
>>>> +        sdhci_am654_write_itapdly(sdhci_am654, sdhci_am654->itap_del_sel[timing]);
>>>>           sdhci_am654->dll_enable = true;
>>>>       } else {
>>>>           sdhci_am654_setup_delay_chain(sdhci_am654, timing);
>>>
>>
>> ~ Judith
>