[U-Boot] [PATCH] mmc: Avoid HS400 mode when accessing boot partitions

[U-Boot] [PATCH] mmc: Avoid HS400 mode when accessing boot partitions

[Marek Vasut]

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.

Signed-off-by: Marek Vasut <marek.vasut+renesas@gmail.com>
Cc: Jean-Jacques Hiblot <jjhiblot@ti.com>
Cc: Nobuhiro Iwamatsu <iwamatsu@nigauri.org>
Cc: Peng Fan <peng.fan@nxp.com>
---
 drivers/mmc/mmc.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/mmc/mmc.c b/drivers/mmc/mmc.c
index 456c1b4cc9..71b52c6cf2 100644
--- a/drivers/mmc/mmc.c
+++ b/drivers/mmc/mmc.c
@@ -905,14 +905,14 @@ static int mmc_set_capacity(struct mmc *mmc, int part_num)
 	return 0;
 }
 
-#if CONFIG_IS_ENABLED(MMC_HS200_SUPPORT)
+#if CONFIG_IS_ENABLED(MMC_HS200_SUPPORT) || CONFIG_IS_ENABLED(MMC_HS400_SUPPORT)
 static int mmc_boot_part_access_chk(struct mmc *mmc, unsigned int part_num)
 {
 	int forbidden = 0;
 	bool change = false;
 
 	if (part_num & PART_ACCESS_MASK)
-		forbidden = MMC_CAP(MMC_HS_200);
+		forbidden = MMC_CAP(MMC_HS_200) | MMC_CAP(MMC_HS_400);
 
 	if (MMC_CAP(mmc->selected_mode) & forbidden) {
 		pr_debug("selected mode (%s) is forbidden for part %d\n",
-- 
2.20.1

[U-Boot] [PATCH] mmc: Avoid HS400 mode when accessing boot partitions

[Peng Fan]

> 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.
> 
> Signed-off-by: Marek Vasut <marek.vasut+renesas@gmail.com>
> Cc: Jean-Jacques Hiblot <jjhiblot@ti.com>
> Cc: Nobuhiro Iwamatsu <iwamatsu@nigauri.org>
> Cc: Peng Fan <peng.fan@nxp.com>
> ---
>  drivers/mmc/mmc.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/mmc/mmc.c b/drivers/mmc/mmc.c index
> 456c1b4cc9..71b52c6cf2 100644
> --- a/drivers/mmc/mmc.c
> +++ b/drivers/mmc/mmc.c
> @@ -905,14 +905,14 @@ static int mmc_set_capacity(struct mmc *mmc, int
> part_num)
>  	return 0;
>  }
> 
> -#if CONFIG_IS_ENABLED(MMC_HS200_SUPPORT)
> +#if CONFIG_IS_ENABLED(MMC_HS200_SUPPORT) ||
> +CONFIG_IS_ENABLED(MMC_HS400_SUPPORT)
>  static int mmc_boot_part_access_chk(struct mmc *mmc, unsigned int
> part_num)  {
>  	int forbidden = 0;
>  	bool change = false;
> 
>  	if (part_num & PART_ACCESS_MASK)
> -		forbidden = MMC_CAP(MMC_HS_200);
> +		forbidden = MMC_CAP(MMC_HS_200) | MMC_CAP(MMC_HS_400);
> 
>  	if (MMC_CAP(mmc->selected_mode) & forbidden) {
>  		pr_debug("selected mode (%s) is forbidden for part %d\n",

Reviewed-by: Peng Fan <peng.fan@nxp.com>

> --
> 2.20.1

[U-Boot] [PATCH] mmc: Avoid HS400 mode when accessing boot partitions

[Faiz Abbas]

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.

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.

Thanks,
Faiz

[U-Boot] [PATCH] mmc: Avoid HS400 mode when accessing boot partitions

[Marek Vasut]

On 6/4/19 1:22 PM, Faiz Abbas wrote:
> Hi Marek, Peng,

Hi,

> 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.
> 
> 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.

Did you practically verify this ? In my case, the boot partition access
fails in HS200/HS400 mode (samsung and sandisk emmc, but I'd have to
check the exact part number).

commit 01298da31d92ecc46cf9130d8cff68bc51698197
    mmc: Change mode when switching to a boot partition
seems to confirm that too.

If nothing else, this patch should go in for the sake of consistency.

-- 
Best regards,
Marek Vasut

[U-Boot] [PATCH] mmc: Avoid HS400 mode when accessing boot partitions

[Faiz Abbas]

Hi Marek,

On 04/06/19 6:56 PM, Marek Vasut wrote:
> On 6/4/19 1:22 PM, Faiz Abbas wrote:
>> Hi Marek, Peng,
> 
> Hi,
> 
>> 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.
>>
>> 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.
> 
> Did you practically verify this ? In my case, the boot partition access
> fails in HS200/HS400 mode (samsung and sandisk emmc, but I'd have to
> check the exact part number).
> 
> commit 01298da31d92ecc46cf9130d8cff68bc51698197
>     mmc: Change mode when switching to a boot partition
> seems to confirm that too.
> 

I had tried to raise these concerns for that patch as well
(https://www.spinics.net/lists/linux-mmc/msg50432.html) but it was
missed at that time.

I did verify that kernel is not switching speeds on a dra7xx board. JJ
and I have seen failures similar to yours in u-boot (which led to this
patch) but not in kernel which makes me think that the fix was wrong.
Have you verified with your setup in kernel?

Thanks,
Faiz

[U-Boot] [PATCH] mmc: Avoid HS400 mode when accessing boot partitions

[Marek Vasut]

On 6/4/19 3:34 PM, Faiz Abbas wrote:
> Hi Marek,
> 
> On 04/06/19 6:56 PM, Marek Vasut wrote:
>> On 6/4/19 1:22 PM, Faiz Abbas wrote:
>>> Hi Marek, Peng,
>>
>> Hi,
>>
>>> 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.
>>>
>>> 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.
>>
>> Did you practically verify this ? In my case, the boot partition access
>> fails in HS200/HS400 mode (samsung and sandisk emmc, but I'd have to
>> check the exact part number).
>>
>> commit 01298da31d92ecc46cf9130d8cff68bc51698197
>>     mmc: Change mode when switching to a boot partition
>> seems to confirm that too.
>>
> 
> I had tried to raise these concerns for that patch as well
> (https://www.spinics.net/lists/linux-mmc/msg50432.html) but it was
> missed at that time.
> 
> I did verify that kernel is not switching speeds on a dra7xx board. JJ
> and I have seen failures similar to yours in u-boot (which led to this
> patch) but not in kernel which makes me think that the fix was wrong.
> Have you verified with your setup in kernel?

No, but I'll add it to the list of things to look into.

In the meantime, this should go in anyway, to make the code consistent.

-- 
Best regards,
Marek Vasut

[U-Boot] [PATCH] mmc: Avoid HS400 mode when accessing boot partitions

[Faiz Abbas]

Hi,

+ Kernel MMC maintainer

On 04/06/19 7:08 PM, Marek Vasut wrote:
> On 6/4/19 3:34 PM, Faiz Abbas wrote:
>> Hi Marek,
>>
>> On 04/06/19 6:56 PM, Marek Vasut wrote:
>>> On 6/4/19 1:22 PM, Faiz Abbas wrote:
>>>> Hi Marek, Peng,
>>>
>>> Hi,
>>>
>>>> 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.
>>>>
>>>> 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.
>>>
>>> Did you practically verify this ? In my case, the boot partition access
>>> fails in HS200/HS400 mode (samsung and sandisk emmc, but I'd have to
>>> check the exact part number).
>>>
>>> commit 01298da31d92ecc46cf9130d8cff68bc51698197
>>>     mmc: Change mode when switching to a boot partition
>>> seems to confirm that too.
>>>
>>
>> I had tried to raise these concerns for that patch as well
>> (https://www.spinics.net/lists/linux-mmc/msg50432.html) but it was
>> missed at that time.
>>
>> I did verify that kernel is not switching speeds on a dra7xx board. JJ
>> and I have seen failures similar to yours in u-boot (which led to this
>> patch) but not in kernel which makes me think that the fix was wrong.
>> Have you verified with your setup in kernel?
> 
> No, but I'll add it to the list of things to look into.
> 
> In the meantime, this should go in anyway, to make the code consistent.
> 

If we keep covering up the issue then it will never be solved. We need
to get more data points and more expert opinions on this and take the
pains to fix it.

@Ulf we are seeing some failures when trying to access eMMC boot
partitions at HS200/HS400 speed modes.

Do you know if there are limitations on accessing eMMC boot partitions
at HS200/HS400 speed modes in kernel? Do you agree with my reading of
the spec above?

Thanks,
Faiz

[U-Boot] [PATCH] mmc: Avoid HS400 mode when accessing boot partitions

[Marek Vasut]

On 6/7/19 9:53 AM, Faiz Abbas wrote:
> Hi,
> 
> + Kernel MMC maintainer
> 
> On 04/06/19 7:08 PM, Marek Vasut wrote:
>> On 6/4/19 3:34 PM, Faiz Abbas wrote:
>>> Hi Marek,
>>>
>>> On 04/06/19 6:56 PM, Marek Vasut wrote:
>>>> On 6/4/19 1:22 PM, Faiz Abbas wrote:
>>>>> Hi Marek, Peng,
>>>>
>>>> Hi,
>>>>
>>>>> 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.
>>>>>
>>>>> 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.
>>>>
>>>> Did you practically verify this ? In my case, the boot partition access
>>>> fails in HS200/HS400 mode (samsung and sandisk emmc, but I'd have to
>>>> check the exact part number).
>>>>
>>>> commit 01298da31d92ecc46cf9130d8cff68bc51698197
>>>>     mmc: Change mode when switching to a boot partition
>>>> seems to confirm that too.
>>>>
>>>
>>> I had tried to raise these concerns for that patch as well
>>> (https://www.spinics.net/lists/linux-mmc/msg50432.html) but it was
>>> missed at that time.
>>>
>>> I did verify that kernel is not switching speeds on a dra7xx board. JJ
>>> and I have seen failures similar to yours in u-boot (which led to this
>>> patch) but not in kernel which makes me think that the fix was wrong.
>>> Have you verified with your setup in kernel?
>>
>> No, but I'll add it to the list of things to look into.
>>
>> In the meantime, this should go in anyway, to make the code consistent.
>>
> 
> If we keep covering up the issue then it will never be solved. We need
> to get more data points and more expert opinions on this and take the
> pains to fix it.

The release is weeks away, this fix is trivial. If we want to do more
intrusive changes, that's for next release anyway.

> @Ulf we are seeing some failures when trying to access eMMC boot
> partitions at HS200/HS400 speed modes.
> 
> Do you know if there are limitations on accessing eMMC boot partitions
> at HS200/HS400 speed modes in kernel? Do you agree with my reading of
> the spec above?
> 
> Thanks,
> Faiz
> 


-- 
Best regards,
Marek Vasut

[U-Boot] [PATCH] mmc: Avoid HS400 mode when accessing boot partitions

[Peng Fan]

> 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.

Regards,
Peng.

> 
> Thanks,
> Faiz
> 

[U-Boot] [PATCH] mmc: Avoid HS400 mode when accessing boot partitions

[Marek Vasut]

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 would also like to hear from Jean why he originally introduced this
for HS200 mode.

-- 
Best regards,
Marek Vasut

[U-Boot] [PATCH] mmc: Avoid HS400 mode when accessing boot partitions

[Peng Fan]

> 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.

Regards,
Peng.

> 
> I would also like to hear from Jean why he originally introduced this for HS200
> mode.
> 
> --
> Best regards,
> Marek Vasut

[U-Boot] [PATCH] mmc: Avoid HS400 mode when accessing boot partitions

[Marek Vasut]

On 6/11/19 3:17 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.

Right.

But I would still like to know whether we can access the HW partitions
in HS modes or not. And I would also like to know why we originally
prevented it.

-- 
Best regards,
Marek Vasut

[U-Boot] [PATCH] mmc: Avoid HS400 mode when accessing boot partitions

[Faiz Abbas]

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?

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.

Thanks,
Faiz

[U-Boot] [PATCH] mmc: Avoid HS400 mode when accessing boot partitions

[Marek Vasut]

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.

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.

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.

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.

> 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 ?

-- 
Best regards,
Marek Vasut

[U-Boot] [PATCH] mmc: Avoid HS400 mode when accessing boot partitions

[Faiz Abbas]

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.

Thanks,
Faiz

[U-Boot] [PATCH] mmc: Avoid HS400 mode when accessing boot partitions

[Jean-Jacques Hiblot]

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.


If we could get this tested OK on most of the platforms that support 
HS200, I suggest that we remove this limitation.

JJ


>
> Thanks,
> Faiz
>
>
>

[U-Boot] [PATCH] mmc: Avoid HS400 mode when accessing boot partitions

[Marek Vasut]

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 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 ?

-- 
Best regards,
Marek Vasut

[U-Boot] [PATCH] mmc: Avoid HS400 mode when accessing boot partitions

[Jean-Jacques Hiblot]

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 ?
>

[U-Boot] [PATCH] mmc: Avoid HS400 mode when accessing boot partitions

[Marek Vasut]

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] [PATCH] mmc: Avoid HS400 mode when accessing boot partitions

[Jean-Jacques Hiblot]

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.

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.

JJ


>>>
>>>> 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 ?

>

[U-Boot] [PATCH] mmc: Avoid HS400 mode when accessing boot partitions

[Marek Vasut]

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] [PATCH] mmc: Avoid HS400 mode when accessing boot partitions

[Peng Fan]

> 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

[U-Boot] [PATCH] mmc: Avoid HS400 mode when accessing boot partitions

[Faiz Abbas]

Hi Peng, Marek,

On 18/06/19 10:33 AM, 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?
> 

I agree with the condition that the patch description should record the
history of why HS200 was disabled and be clear that HS400 is being
disabled for consistency and not because the spec says it.

Thanks,
Faiz

[U-Boot] [PATCH] mmc: Avoid HS400 mode when accessing boot partitions

[Peng Fan]

gauri.org>; Marek Vasut
> <marek.vasut+renesas@gmail.com>
> Subject: Re: [U-Boot] [PATCH] mmc: Avoid HS400 mode when accessing boot
> partitions
> 
> Hi Peng, Marek,
> 
> On 18/06/19 10:33 AM, 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?
> >
> 
> I agree with the condition that the patch description should record the history
> of why HS200 was disabled and be clear that HS400 is being disabled for
> consistency and not because the spec says it.

Since gitlab not ready, applied to https://github.com/MrVan/u-boot/releases/tag/mmc-6-19
With commit log modified as below:
"
U-Boot code currently only applies this restriction to HS200 mode,
extend this to HS400 mode as well.

Currently U-Boot code not support accessing boot partition in HS200/400
mode. This needs more check.

Signed-off-by: Marek Vasut <marek.vasut+renesas@gmail.com>
Cc: Jean-Jacques Hiblot <jjhiblot@ti.com>
Cc: Nobuhiro Iwamatsu <iwamatsu@nigauri.org>
Cc: Peng Fan <peng.fan@nxp.com>
Reviewed-by: Peng Fan <peng.fan@nxp.com> 
"

Regards,
Peng.

> 
> Thanks,
> Faiz

[U-Boot] [PATCH] mmc: Avoid HS400 mode when accessing boot partitions

[Marek Vasut]

On 6/18/19 7:03 AM, 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?

I think so.

-- 
Best regards,
Marek Vasut

[U-Boot] [PATCH] mmc: Avoid HS400 mode when accessing boot partitions

[Jean-Jacques Hiblot]

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

[U-Boot] [PATCH] mmc: Avoid HS400 mode when accessing boot partitions

[Marek Vasut]

On 6/11/19 5:59 PM, 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.

Currently, I am not banking either way. If it's done one way or the
other in some other project, that does not imply it's correct.

I would suggest you send a patch, we queue it for next release and test
it on all available hardware. Then we'll see whether something breaks or
not. This would give us ~3-4 months of time to test this extensively.
Can you do that ?

>> 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.

We are not sure whether this is a real error or not ... yet.

>> 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.

And that is better than non-working boot altogether.

>> 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.

I can reword the commit message, got any suggestions ?

>>> 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.

Sorry, I cannot agree on this. Right now, we block boot partition access
in one HS mode and not the other (HS200 and not HS400). That's clearly
an omission when the HS400 mode was added and has to be fixed. I am more
concerned about stability this close to the end of release cycle, than I
am concerned about performance.

Please send a patch for next to remove this limitation and then I can
test it on the boards I have available. Then we can collect data points
and decide whether to leave that patch in next or not.

-- 
Best regards,
Marek Vasut

[U-Boot] [PATCH] mmc: Avoid HS400 mode when accessing boot partitions

[Jean-Jacques Hiblot]

+ Jaehoon


Hi Marek,

On 10/06/2019 13:33, Marek Vasut wrote:
> 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 would also like to hear from Jean why he originally introduced this
> for HS200 mode.

Well I didn't look into the specs when working on this part.

I introduced it because of Jaehoon's remark on this thread: 
https://lists.denx.de/pipermail/u-boot/2017-January/278672.html

Maybe Jaehoon can help us here understand why it is needed.

JJ

>

[U-Boot] [PATCH] mmc: Avoid HS400 mode when accessing boot partitions

[Marek Vasut]

On 6/11/19 5:25 PM, Jean-Jacques Hiblot wrote:
> + Jaehoon
> 
> 
> Hi Marek,
> 
> On 10/06/2019 13:33, Marek Vasut wrote:
>> 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 would also like to hear from Jean why he originally introduced this
>> for HS200 mode.
> 
> Well I didn't look into the specs when working on this part.
> 
> I introduced it because of Jaehoon's remark on this thread:
> https://lists.denx.de/pipermail/u-boot/2017-January/278672.html

Aha

> Maybe Jaehoon can help us here understand why it is needed.

I'm afraid Jaehoon was MIA for a while :(

-- 
Best regards,
Marek Vasut