Re: [PATCH] mmc: rpmb: add quirk MMC_QUIRK_BROKEN_RPMB_RETUNE

[Christian Loehle <christian.loehle@arm.com>]

On 29/11/2023 14:42, Jorge Ramirez-Ortiz, Foundries wrote:
> On 29/11/23 10:32:41, Christian Loehle wrote:
>> On 29/11/2023 09:43, Jorge Ramirez-Ortiz wrote:
>>> On the eMMC SanDisk iNAND 7250 configured with HS200, requesting a
>>> re-tune before switching to the RPMB partition would randomly cause
>>> subsequent RPMB requests to fail with EILSEQ:
>>> * data error -84, tigggered in __mmc_blk_ioctl_cmd()
>>>
>>> This commit skips the retune when switching to RPMB.
>>> Tested over several days with per minute RPMB reads.
>>>
>>> Signed-off-by: Jorge Ramirez-Ortiz <jorge@foundries.io>
>>> ---
>>>  drivers/mmc/core/block.c  | 6 +++++-
>>>  drivers/mmc/core/card.h   | 7 +++++++
>>>  drivers/mmc/core/quirks.h | 7 +++++++
>>>  include/linux/mmc/card.h  | 1 +
>>>  4 files changed, 20 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/mmc/core/block.c b/drivers/mmc/core/block.c
>>> index 152dfe593c43..9b7ba6562a3b 100644
>>> --- a/drivers/mmc/core/block.c
>>> +++ b/drivers/mmc/core/block.c
>>> @@ -860,6 +860,11 @@ static int mmc_blk_part_switch_pre(struct mmc_card *card,
>>>  				return ret;
>>>  		}
>>>  		mmc_retune_pause(card->host);
>>> +
>>> +		/* Do not force retune before RPMB switch */
>>> +		if (mmc_can_retune(card->host) &&
>>> +		    mmc_card_broken_rpmb_retune(card))
>>> +			card->host->need_retune = 0;
>>>  	}
>>
>> Is this just an issue for rpmb switches or did you try other non-rpmb
>> partition switches, too?
> 
> I only tried the reported failure case (ie, RPMB switch).This failure
> case came from a product verification team so I believe all the other
> scenarios should be functional - other than RPMB accesses (these tests
> started a few months ago), no other issues have been reported from eMMC
> validation in the last couple of years.
> 
> Is there some additional check/test you would like to see done? if so,
> please could you let me know how to trigger them?

That's fine with me, probably just a very interesting bug then.

> 
>> And only HS200 or that's just what you tested with?
> 
> Yes that is just the product configuration; so the only one I tested.
> 
> The host controller is sdhci-of-arasan for xlnx,zynqmp-8.9a. I inspected
> the driver history and auto-tuning was extended to support ZynqMP (DLL
> reset) some years ago so I believe tuning itself should be fine.
> 
> I am referring to commit 8d2e334377dbe645415fbe031711324bc2281907 "mmc:
> sdhci-of-arasan: Add support for DLL reset for ZynqMP platforms "
> 
>>
>>>
>>>  	return ret;
>>> @@ -3143,4 +3148,3 @@ module_exit(mmc_blk_exit);
>>>
>>>  MODULE_LICENSE("GPL");
>>>  MODULE_DESCRIPTION("Multimedia Card (MMC) block device driver");
>>> -
>>> diff --git a/drivers/mmc/core/card.h b/drivers/mmc/core/card.h
>>> index b7754a1b8d97..1e1555a15de9 100644
>>> --- a/drivers/mmc/core/card.h
>>> +++ b/drivers/mmc/core/card.h
>>> @@ -85,6 +85,7 @@ struct mmc_fixup {
>>>  #define CID_MANFID_MICRON       0x13
>>>  #define CID_MANFID_SAMSUNG      0x15
>>>  #define CID_MANFID_APACER       0x27
> 
> I had to add another ID for SanDisk - maybe 0x45 is for Industrial
> controllers? do you know?

I don't have access to JEDEC internal docs anymore, but since Avri
probably wants to ACK this potential bug anyway, he should confirm.