[U-Boot] [PATCH] mmc: Avoid HS400 mode when accessing boot partitions

[Faiz Abbas]

Hi Peng, Marek,

On 18/06/19 10:33 AM, Peng Fan wrote:
>> Subject: Re: [U-Boot] [PATCH] mmc: Avoid HS400 mode when accessing boot
>> partitions
>>
>> On 6/17/19 4:46 PM, Jean-Jacques Hiblot wrote:
>>>
>>> On 17/06/2019 12:34, Marek Vasut wrote:
>>>> On 6/17/19 11:09 AM, Jean-Jacques Hiblot wrote:
>>>>> On 15/06/2019 17:15, Marek Vasut wrote:
>>>>>> On 6/14/19 5:27 PM, Jean-Jacques Hiblot wrote:
>>>>>>> Marek, Faiz,
>>>>>>>
>>>>>>> On 11/06/2019 17:59, Faiz Abbas wrote:
>>>>>>>> Hi Marek,
>>>>>>>>
>>>>>>>> On 11/06/19 3:34 PM, Marek Vasut wrote:
>>>>>>>>> On 6/11/19 10:12 AM, Faiz Abbas wrote:
>>>>>>>>>> Peng, Marek,
>>>>>>>>>>
>>>>>>>>>> On 11/06/19 6:47 AM, Peng Fan wrote:
>>>>>>>>>>>> partitions
>>>>>>>>>>>>
>>>>>>>>>>>> On 6/10/19 7:59 AM, Peng Fan wrote:
>>>>>>>>>>>>>> Subject: Re: [U-Boot] [PATCH] mmc: Avoid HS400 mode when
>>>>>>>>>>>>>> accessing boot partitions
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> Hi Marek, Peng,
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> On 03/06/19 12:04 PM, Peng Fan wrote:
>>>>>>>>>>>>>>>> Subject: [PATCH] mmc: Avoid HS400 mode when accessing
>>>>>>>>>>>>>>>> boot partitions
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>> According to JEDEC JESD84-B51.pdf section 6.3.3 Boot
>>>>>>>>>>>>>>>> operation ,
>>>>>>>>>>>>>>>> HS200 & HS400 mode is not supported during boot
>> operation.
>>>>>>>>>>>>>>>> The
>>>>>>>>>>>>>>>> U-Boot code currently only applies this restriction to
>>>>>>>>>>>>>>>> HS200 mode, extend this to
>>>>>>>>>>>>>>>> HS400 mode as well.
>>>>>>>>>>>>>> The spec in section 6.3.3 (according to my understanding)
>>>>>>>>>>>>>> is talking about "boot operation" which is a way of getting
>>>>>>>>>>>>>> data from the the eMMC without going through the Device
>>>>>>>>>>>>>> identification mode (Section
>>>>>>>>>>>>>> 6.4.4) i.e. without sending any commands. All the host has
>>>>>>>>>>>>>> to do is hold the command line low in Pre-Idle mode to
>>>>>>>>>>>>>> automatically receive data at the preconfigured frequency
>>>>>>>>>>>>>> and bus width.
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> When U-boot is accessing the partition, it has already gone
>>>>>>>>>>>>>> through the Device identification mode and is in data
>>>>>>>>>>>>>> transfer mode (i.e. it needs to send commands for
>>>>>>>>>>>>>> read/write to happen). In this case, we need to switch the
>>>>>>>>>>>>>> partition in Extended CSD to access the boot partition
>>>>>>>>>>>>>> (Section 6.2.5). The spec doesn't say anything about
>>>>>>>>>>>>>> HS200 and
>>>>>>>>>>>> HS400 not being supported here.
>>>>>>>>>>>>> Yes, the spec does not mention this. It only mentions
>>>>>>>>>>>>> HS200/400 not supported during boot operation.
>>>>>>>>>>>>>
>>>>>>>>>>>>>> Also, I don't see linux kernel switching down speed when
>>>>>>>>>>>>>> trying to access a boot partition (unless its being very
>>>>>>>>>>>>>> sneaky about it). So if you are seeing issues with
>>>>>>>>>>>>>> accessing boot partitions at
>>>>>>>>>>>>>> HS200/HS400 then you should probably look at how linux code
>>>>>>>>>>>>>> is working
>>>>>>>>>>>> instead.
>>>>>>>>>>>>> There might be bug in U-Boot code.
>>>>>>>>>>>> So are we gonna leave this inconsistency in for current
>>>>>>>>>>>> release or what's it gonna be ? Like I said, we're in rc3,
>>>>>>>>>>>> it's fine to do bigger changes in next release, but we should
>>>>>>>>>>>> at least fix this in current release.
>>>>>>>>>>> I'll pick up your patch in this release.
>>>>>>>>>>>
>>>>>>>>>> The issue that Marek is facing is not a regression, is it? Are
>>>>>>>>>> we really going to merge something that we know to be wrong
>>>>>>>>>> just so we are consistently wrong?
>>>>>>>>> First of all, you established this is "wrong" without any real
>>>>>>>>> backing except for your interpretation of the specification. I
>>>>>>>>> would still like to hear from Jean the real reason why he added
>>>>>>>>> this filtering in the first place.
>>>>>>>> I think Peng agrees with my interpretation. The backing for it
>>>>>>>> being "right" is also JJ's and your interpretation of spec. The
>>>>>>>> additional justification that I am trying to give is that there
>>>>>>>> is no code to fallback in kernel and I have observed it working
>>>>>>>> in kernel with no issues. I needed your observations (with any
>>>>>>>> HS200/HS400 supporting
>>>>>>>> platform) in kernel for additional data points.
>>>>>>>>
>>>>>>>>> That said, we're in rc4 , the release is just around the corner.
>>>>>>>>> I would like to avoid big changes in the MMC subsystem , or any
>>>>>>>>> subsystem for that matter. That's for next release , and if you
>>>>>>>>> have a patch for next, please post it, I am happy to test it on
>>>>>>>>> the hardware I have available.
>>>>>>>> I am not saying we try to fix it before this release. All I am
>>>>>>>> saying is that we don't mask real errors (none of which are
>>>>>>>> regressions) with this "fix" that we are not even sure of.
>>>>>>>>
>>>>>>>>> Also note that this patch does not have any impact on general
>>>>>>>>> use case, the regular bulk of the eMMC can be accessed at
>>>>>>>>> HS200/HS400, it's just the boot partitions which are accessed in
>>>>>>>>> HS52 or lower.
>>>>>>>> Exceedingly, the general usecase is to put boot images in boot
>>>>>>>> partition and root filesystem in the user data area. In that
>>>>>>>> case, the boot area is all that will be accessed in SPL at HS52
>>>>>>>> even if
>>>>>>>> CONFIG_SPL_MMC_HS200/HS400 is enabled.
>>>>>>>>
>>>>>>>>> However, right now, the behavior is not consistent between HS200
>>>>>>>>> and
>>>>>>>>> HS400 modes, and I would very much like to have it consistent in
>>>>>>>>> the upcoming release.
>>>>>>>> I don't think consistency is a big enough reason to make this
>>>>>>>> change. If my interpretation is correct, you would be masking
>>>>>>>> real issues for everyone with this change and any platforms which
>>>>>>>> enable HS400/HS200 will be blissfully unaware that they are not
>>>>>>>> accessing data at the required speed. If things are failing for
>>>>>>>> others, we can get their datapoints in kernel as well.
>>>>>>>>
>>>>>>>> Having said that, if the maintainer still wants to pull this fix
>>>>>>>> as is, I would at least change the commit message to reflect our
>>>>>>>> uncertainty here so people are not mislead by this patch.
>>>>>>>>
>>>>>>>>>> Marek, I understand you do not want to debug this right now but
>>>>>>>>>> this patch will 1) Mislead people into thinking that we know
>>>>>>>>>> what we are doing (two patches went in with pretty confident
>>>>>>>>>> patch
>>>>>>>>>> descriptions)
>>>>>>>>>> and
>>>>>>>>>> 2) Mask issues for people who could take the time to help debug
>>>>>>>>>> this.
>>>>>>>>> Wrong, I want to debug this, I just don't want to do big changes
>>>>>>>>> in the upcoming release this late in the release cycle. But if
>>>>>>>>> you propose a patch for next, I am happy to test it on the
>>>>>>>>> hardware I have available.
>>>>>>>>> Can you send such a patch ?
>>>>>>>>>
>>>>>>>> Agreed on the no big changes this release. Hopefully we can also
>>>>>>>> agree on not having *this* change in the release either. I do not
>>>>>>>> have a fix yet but plan to look into this next week.
>>>>>>> Have you tried to use the boot partitions with HS200 lately ?
>>>>>>>
>>>>>>> I'm running a test on a DRA76 and haven't seen any issue. I wonder
>>>>>>> why it didn't work properly when I tested it back then.
>>>>>>>
>>>>>>> I also rand the same test with Linux and checked that the clock
>>>>>>> was also at 192 MHz
>>>>>>>
>>>>>>>
>>>>>>> test context:
>>>>>>>
>>>>>>> The boot partition (8MB) is accessed in HS200 mode (real freq is
>>>>>>> measured at 192 MHz with a scope)
>>>>>>>
>>>>>>> The data is fresh random data
>>>>>>>
>>>>>>> The test command is:
>>>>>>>
>>>>>>> setenv test_boot_part 'random 0x81000000 0x800000; mmc write
>>>>>>> 81000000 0
>>>>>>> 0x4000; mmc read 82000000 0 0x4000; cmp.b 81000000 82000000
>>>>>>> 0x800000' ; while run test_boot_part ; do echo -------------; done
>>>>>>>
>>>>>>> I'll post the patch for the 'random' command.
>>>>>> Do you think you can add a test.py test for this ? Then I can
>>>>>> continuously run this test on the boards I have available. It
>>>>>> should also propagate onto nvidia and xilinx boards, which I think
>>>>>> do HS modes too.
>>>
>>> I've worked on the python test for mmc writes today. I'll post it soon.
>>>
>>> It looks like HS200 for boot part is not working well yet, at least on
>>> our DRA7 platforms.
>>
>> That's what I was afraid of.
>>
>>> So IMO we should keep the limitation in place and add the limitation
>>> for
>>> HS400 and the python test in this release.
>>>
>>> Then we can start working on a real fix.
>>
>> Sounds good, thanks!
> 
> So we all agree to take this patch first, right?
> 

I agree with the condition that the patch description should record the
history of why HS200 was disabled and be clear that HS400 is being
disabled for consistency and not because the spec says it.

Thanks,
Faiz