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

[Marek Vasut]

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!

-- 
Best regards,
Marek Vasut