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

Mon Jun 17 10:34:00 UTC 2019

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.
>>
>>> If we could get this tested OK on most of the platforms that support
>>> HS200, I suggest that we remove this limitation.
>> Yes, but not this close to the release. It might work on TI board(s),
>> but it could very well break on other boards which we cannot test. I had
>> stability issues with those HS200/HS400 modes, so I am seriously
>> concerned about removing this limitation this close to the release.
> 
> I agree. I would rather keep the limitation in place for this release as
> well
> 
> When all tests are in place, we will be able to confidently remove then
> during the next cycle.

All right, so how shall we proceed ?

-- 
Best regards,
Marek Vasut