From mboxrd@z Thu Jan 1 00:00:00 1970 From: Jean-Jacques Hiblot Date: Tue, 18 Jun 2019 16:38:56 +0200 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: <8d3f6293-172f-faf9-a0d4-79fa24c5eb70@ti.com> List-Id: MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: u-boot@lists.denx.de On 18/06/2019 07:03, 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? right. JJ > > Thanks, > Peng. > >> -- >> Best regards, >> Marek Vasut