Re: [PATCH 2/3] sdhci: sparx5: Add Sparx5 SoC eMMC driver

[Adrian Hunter <adrian.hunter@intel.com>]

On 20/05/20 2:14 pm, Lars Povlsen wrote:
> 
> Lars Povlsen writes:
> 
>> Adrian Hunter writes:
>>
>>> On 13/05/20 4:31 pm, Lars Povlsen wrote:
>>>> This adds the eMMC driver for the Sparx5 SoC. It is based upon the
>>>> designware IP, but requires some extra initialization and quirks.
>>>>
>>>> Reviewed-by: Alexandre Belloni <alexandre.belloni@bootlin.com>
>>>> Signed-off-by: Lars Povlsen <lars.povlsen@microchip.com>
>>>> ---
> {Snip]
>>>> +};
>>>> +
>>>> +static const struct sdhci_pltfm_data sdhci_sparx5_pdata = {
>>>> +     .quirks  = 0,
>>>> +     .quirks2 = SDHCI_QUIRK2_HOST_NO_CMD23 | /* Card quirk */
>>>
>>> If this is a card quirk then it should be in drivers/mmc/core/quirks.h not here.
>>
> 
> Adrian, I had a go at changing the controller quirk to a card quirk.
> 
> Unfortunately, SDHCI_QUIRK2_HOST_NO_CMD23 does not directly translate to
> MMC_QUIRK_BLK_NO_CMD23, as for 'do_rel_wr' in mmc_blk_rw_rq_prep(), it
> will *still* use MMC_SET_BLOCK_COUNT (cmd23), causing the issue.
> 
> We are using a ISSI "IS004G" device, and so I have gone through the
> motions of adding it to quirks.h. The comment before the list of devices
> using MMC_QUIRK_BLK_NO_CMD23 suggest working around a performance issue,
> which is not exactly the issue I'm seeing. I'm seeing combinations of
> CMD_TOUT_ERR, DATA_CRC_ERR and DATA_END_BIT_ERR whenever a cmd23 is
> issued.
> 
> I have not been able to test the controller with another eMMC device
> yet, but I expect its not the controller at fault.
> 
> So, I'm a little bit in doubt of how to proceed - either keep the quirk
> as a controller quirk - or make a *new* card quirk (with
> SDHCI_QUIRK2_HOST_NO_CMD23 semantics)?
> 
> Anybody else have had experience with ISSI eMMC devices?
> 
> I have also tried to use DT sdhci-caps-mask, but MMC_CAP_CMD23 is not
> read from the controller just (unconditionally) set in sdhci.c - so that
> doesn't fly either.
> 
> Any suggestions?

It is up to you.  In the future, you may want to distinguish devices that
have this problem from ones that do not.

If you are not sure it is the ISSI eMMC, and maybe not the host controller,
then might it be the board?  Perhaps make SDHCI_QUIRK2_HOST_NO_CMD23
conditional on the particular compatibility string?

At a minimum, change the "/* Card quirk */" comment to a fuller explanation.

> 
>> Yes, its supposedly a card quirk. I'll see to use the card quirks
>> methods in place.
>>
>