From mboxrd@z Thu Jan 1 00:00:00 1970 From: Peng Fan Date: Tue, 18 Jun 2019 05:03:07 +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 > 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