From mboxrd@z Thu Jan 1 00:00:00 1970 From: Peng Fan Date: Wed, 19 Jun 2019 05:42:59 +0000 Subject: [U-Boot] [PATCH] mmc: Avoid HS400 mode when accessing boot partitions In-Reply-To: References: <20190531132244.29719-1-marek.vasut+renesas@gmail.com> <43c3efe1-55cf-91e3-34c3-c509d0a7207d@ti.com> <3e2e9363-bf4d-84db-867b-130c3709bbb3@gmail.com> <39df182a-dbc8-b688-fa5a-43907bffc5a3@ti.com> <722b00d2-f1d2-3382-9e6c-41d1e4aa5f20@gmail.com> <84b514e9-c5ec-cf22-e2c6-1bc2d14447cf@ti.com> <90fb1578-0ccb-081c-cb6c-c04473618e80@ti.com> <61d6a62a-2ddd-c483-8a55-f70c1878a65e@gmail.com> <7cdb2f59-a851-ba5f-ddc1-e37960a380d3@ti.com> Message-ID: List-Id: MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: u-boot@lists.denx.de gauri.org>; Marek Vasut > > 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 Cc: Jean-Jacques Hiblot Cc: Nobuhiro Iwamatsu Cc: Peng Fan Reviewed-by: Peng Fan " Regards, Peng. > > Thanks, > Faiz