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

Fri Jun 14 15:27:29 UTC 2019

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.

If we could get this tested OK on most of the platforms that support 
HS200, I suggest that we remove this limitation.

JJ

>
> Thanks,
> Faiz
>
>
>