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. >> >>> 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.
All right, so how shall we proceed ? -- Best regards, Marek Vasut _______________________________________________ U-Boot mailing list U-Boot@lists.denx.de https://lists.denx.de/listinfo/u-boot
