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

Wed Jun 19 05:42:59 UTC 2019

gauri.org>; Marek Vasut
> <marek.vasut+renesas at gmail.com>
> Subject: Re: [U-Boot] [PATCH] mmc: Avoid HS400 mode when accessing boot
> partitions
> 
> 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.

Since gitlab not ready, applied to https://github.com/MrVan/u-boot/releases/tag/mmc-6-19
With commit log modified as below:
"
U-Boot code currently only applies this restriction to HS200 mode,
extend this to HS400 mode as well.

Currently U-Boot code not support accessing boot partition in HS200/400
mode. This needs more check.

Signed-off-by: Marek Vasut <marek.vasut+renesas at gmail.com>
Cc: Jean-Jacques Hiblot <jjhiblot at ti.com>
Cc: Nobuhiro Iwamatsu <iwamatsu at nigauri.org>
Cc: Peng Fan <peng.fan at nxp.com>
Reviewed-by: Peng Fan <peng.fan at nxp.com> 
"

Regards,
Peng.

> 
> Thanks,
> Faiz