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

[Peng Fan]

> 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?

Thanks,
Peng.

> 
> --
> Best regards,
> Marek Vasut