From mboxrd@z Thu Jan 1 00:00:00 1970 From: Jean-Jacques Hiblot Date: Mon, 17 Jun 2019 11:09:40 +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> 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 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. > >> If we could get this tested OK on most of the platforms that support >> HS200, I suggest that we remove this limitation. > Yes, but not this close to the release. It might work on TI board(s), > but it could very well break on other boards which we cannot test. I had > stability issues with those HS200/HS400 modes, so I am seriously > concerned about removing this limitation this close to the release. I agree. I would rather keep the limitation in place for this release as well When all tests are in place, we will be able to confidently remove then during the next cycle. > > I would much rather see a release where the eMMCs work stable, albeit > slower, possibly with a downstream patch in some BSP , then a release > where the eMMCs randomly fail when loading data from them . > > I am happy to reword the commit message if that helps ? >