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! -- Best regards, Marek Vasut _______________________________________________ U-Boot mailing list U-Boot@lists.denx.de https://lists.denx.de/listinfo/u-boot
