Re: [U-Boot] [PATCH] mmc: sdhci: consider SDHCI_QUIRK_NO_HISPD_BIT in host_caps

[Hannes Schmelzer]

> 
> Dear Hannes,
Dear Jaehoon,

> >> Instead, its bit is used to other purpose.
> > 
> > Please explain me more about this other purpose,
> > maybe i'm missunterstand here something.
> 
> SD Host Controller has the HOST Controller register (Offset 0x28).
> BIT[2] is "High speed enable".
> - BIT[2] : 0 - Normal Speed mode
> - BIT[2] : 1 - High Speed mode.
> This bit is "RW" attribute.
> 
> And Samsung SoC also has the HOST Controller register (offset 0x28).
> BIT[2] is "Output Edge Inversion". 
> - BIT[2] : 0 - Rising edge output.
> - BIT[2] : 1 - Falling edge output.
> 
> "High speed enable" bit is standard. but Samsung SoC is using it for 
other purpose.
> So I had added the quirks for SDHCI_QUIRK_NO_HISPD_BIT.
> It means that there is no 'high speed bit'.

OK. Many thanks for clarifying this.
I took a look to the ZYNQ TRM and an the register 0x28 of the sdhci 
controller,
here we have standard behaviour where bit[2] controls the high speed 
stuff.

The conclusion of this is, that the "zynq_sdhci.c" uses the 
SDHCI_QUIRK_NO_HISPD_BIT
wrong and this must be fixed.

I did grep for the "CONFIG_ZYNQ_HISPD_BROKEN" within source tree and 
observed
that nobody or no boards except mine uses this #define.
This would explain that, i call it this bug, is unnoticed yet.

> 
> If your patch is applied, Samsung SoC can work the unexpected behavior.
> 
> If your ip also doesn't use this bit, then no problem. 
> But your patch tried to use the other purpose.

OK.

> 
> > 
> > Maybe i've to introduce some new quirk, which tells the ip-core and 
the 
> > other layers> above not using HIGH_SPEED. But i think still this is 
the right one ... 
> 
> Well, i don't want to add the new quirk, if there is a solution not to 
add the quriks.
> If really needs, you need to add the SDHCI_QUIRK_BROKEN_HISPD_MODE.
> 
> #define SDHCI_QUIRK_BROKEN_HISPD_MODE (1 << 5)

OK. But i fear that we've to introduce this new quirk.
Because in real life mistakes in hardware-design (in this case chip 
design) happen and
we software guys have to workaround them.

https://www.xilinx.com/support/answers/5.html

> 
> and change from SDHCI_QUIRK_NO_HSIPD_BIT to 
SDHCI_QUIRK_BROKEN_HISPD_MODE in 
> zynq_sdhci.c.

I will do so.

I will also send some V2 of my patch, with doing:

- add this new QUIRK
- fix the zynq_sdhci.c
- consider this quirk in the host_cap


> 
> I will update the descriptions about quirks. Because it can be confused 
what 
> purpose is used.

Great, so it will be more readable in the future.

> 
> Best Regards,
> Jaehoon Chung
cheers,
Hannes


___
U-Boot mailing list
U-Boot@lists.denx.de
https://lists.denx.de/listinfo/u-boot

Re: [U-Boot] [PATCH] mmc: sdhci: consider SDHCI_QUIRK_NO_HISPD_BIT in host_caps

[Jaehoon Chung]

Dear Hannes,

On 03/02/2018 04:16 PM, Hannes Schmelzer wrote:
>>
>> On 03/01/2018 11:36 PM, Hannes Schmelzer wrote:
>>> Some IP-core implementations of the SDHCI have different troubles on 
> the
>>> silicon where they are placed.
>>>
>>> On ZYNQ platform for example Xilinx doesn't accept the hold timing of 
> an
>>> eMMC chip which operates in High-Speed mode. Due to this fact the
>>> "SDHCI_QUIRK_NO_HISPD_BIT" quirk was born.
>>>
>>> This commit reflects this fact within the host-controller 
> capabilities,
>>> so that the layer above (mmc-driver) can setup the card correctly.
>>>
>>> Otherwise the MMC card will be switched into high-speed mode and 
> causes
>>> possible timing violation on the host-controller side.
>>>
>>> Signed-off-by: Hannes Schmelzer 
>>
>> SDHCI_QUIRK_NO_HISPD_BIT had added because of Samsung SoC.
>> (I had added this quirk, so i know exactly what purpose.)
> 
> Also on other platforms this bit is used, have look to the zynq_sdhci.c
> https://github.com/u-boot/u-boot/blob/master/drivers/mmc/zynq_sdhci.c
> 
> 56 #ifdef CONFIG_ZYNQ_HISPD_BROKEN 
> 57  host->quirks |= SDHCI_QUIRK_NO_HISPD_BIT; 
> 58 #endif 
> 
>> Samsung SoC didn't use the SDHCI_CTRL_HISPD bit at HOST_CNOTROL 
> register. 
>> Instead, its bit is used to other purpose.
> 
> Please explain me more about this other purpose,
> maybe i'm missunterstand here something.

SD Host Controller has the HOST Controller register (Offset 0x28).
BIT[2] is "High speed enable".
- BIT[2] : 0 - Normal Speed mode
- BIT[2] : 1 - High Speed mode.
This bit is "RW" attribute.

And Samsung SoC also has the HOST Controller register (offset 0x28).
BIT[2] is "Output Edge Inversion". 
- BIT[2] : 0 - Rising edge output.
- BIT[2] : 1 - Falling edge output.

"High speed enable" bit is standard. but Samsung SoC is using it for other 
purpose.
So I had added the quirks for SDHCI_QUIRK_NO_HISPD_BIT.
It means that there is no 'high speed bit'.

If your patch is applied, Samsung SoC can work the unexpected behavior.

If your ip also doesn't use this bit, then no problem. 
But your patch tried to use the other purpose.

> 
> Maybe i've to introduce some new quirk, which tells the ip-core and the 
> other layers> above not using HIGH_SPEED. But i think still this is the right 
> one ... 

Well, i don't want to add the new quirk, if there is a solution not to add the 
quriks.
If really needs, you need to add the SDHCI_QUIRK_BROKEN_HISPD_MODE.

#define SDHCI_QUIRK_BROKEN_HISPD_MODE (1 << 5)

and change from SDHCI_QUIRK_NO_HSIPD_BIT to SDHCI_QUIRK_BROKEN_HISPD_MODE in 
zynq_sdhci.c.

I will update the descriptions about quirks. Because it can be confused what 
purpose is used.

Best Regards,
Jaehoon Chung

> please have a
> look to:
> 
> https://github.com/u-boot/u-boot/blob/master/drivers/mmc/mmc.c
> 
> static int mmc_select_mode_and_width(struct mmc *mmc, uint card_caps)
> {
> 
> }
> 
> Here is the matching between host and card capabilities done:
> 1850/* Restrict card's capabilities by what the host can do */ 
> 1851card_caps &= mmc->host_caps; 
> 
> Today only the host side behaves correctly if "CONFIG_ZYNQ_HISPD_BROKEN" 
> is set.
> But the card still operates in HS-Mode and thats wrong.
> 
>> Best Regards,
>> Jaehoon Chung
> cheers,
> Hannes
> 
> 
> 
> 
> 

___
U-Boot mailing list
U-Boot@lists.denx.de
https://lists.denx.de/listinfo/u-boot

Re: [U-Boot] [PATCH] mmc: sdhci: consider SDHCI_QUIRK_NO_HISPD_BIT in host_caps

[Hannes Schmelzer]

> 
> On 03/01/2018 11:36 PM, Hannes Schmelzer wrote:
> > Some IP-core implementations of the SDHCI have different troubles on 
the
> > silicon where they are placed.
> > 
> > On ZYNQ platform for example Xilinx doesn't accept the hold timing of 
an
> > eMMC chip which operates in High-Speed mode. Due to this fact the
> > "SDHCI_QUIRK_NO_HISPD_BIT" quirk was born.
> > 
> > This commit reflects this fact within the host-controller 
capabilities,
> > so that the layer above (mmc-driver) can setup the card correctly.
> > 
> > Otherwise the MMC card will be switched into high-speed mode and 
causes
> > possible timing violation on the host-controller side.
> > 
> > Signed-off-by: Hannes Schmelzer 
> 
> SDHCI_QUIRK_NO_HISPD_BIT had added because of Samsung SoC.
> (I had added this quirk, so i know exactly what purpose.)

Also on other platforms this bit is used, have look to the zynq_sdhci.c
https://github.com/u-boot/u-boot/blob/master/drivers/mmc/zynq_sdhci.c

56 #ifdef CONFIG_ZYNQ_HISPD_BROKEN 
57  host->quirks |= SDHCI_QUIRK_NO_HISPD_BIT; 
58 #endif 

> Samsung SoC didn't use the SDHCI_CTRL_HISPD bit at HOST_CNOTROL 
register. 
> Instead, its bit is used to other purpose.

Please explain me more about this other purpose,
maybe i'm missunterstand here something.

Maybe i've to introduce some new quirk, which tells the ip-core and the 
other layers
above not using HIGH_SPEED. But i think still this is the right one ... 
please have a
look to:

https://github.com/u-boot/u-boot/blob/master/drivers/mmc/mmc.c

static int mmc_select_mode_and_width(struct mmc *mmc, uint card_caps)
{

}

Here is the matching between host and card capabilities done:
1850/* Restrict card's capabilities by what the host can do */ 
1851card_caps &= mmc->host_caps; 

Today only the host side behaves correctly if "CONFIG_ZYNQ_HISPD_BROKEN" 
is set.
But the card still operates in HS-Mode and thats wrong.

> Best Regards,
> Jaehoon Chung
cheers,
Hannes


___
U-Boot mailing list
U-Boot@lists.denx.de
https://lists.denx.de/listinfo/u-boot

Re: [U-Boot] [PATCH] mmc: sdhci: consider SDHCI_QUIRK_NO_HISPD_BIT in host_caps

[Jaehoon Chung]

On 03/01/2018 11:36 PM, Hannes Schmelzer wrote:
> Some IP-core implementations of the SDHCI have different troubles on the
> silicon where they are placed.
> 
> On ZYNQ platform for example Xilinx doesn't accept the hold timing of an
> eMMC chip which operates in High-Speed mode. Due to this fact the
> "SDHCI_QUIRK_NO_HISPD_BIT" quirk was born.
> 
> This commit reflects this fact within the host-controller capabilities,
> so that the layer above (mmc-driver) can setup the card correctly.
> 
> Otherwise the MMC card will be switched into high-speed mode and causes
> possible timing violation on the host-controller side.
> 
> Signed-off-by: Hannes Schmelzer 

SDHCI_QUIRK_NO_HISPD_BIT had added because of Samsung SoC.
(I had added this quirk, so i know exactly what purpose.)
Samsung SoC didn't use the SDHCI_CTRL_HISPD bit at HOST_CNOTROL register. 
Instead, its bit is used to other purpose.

NACK.

Best Regards,
Jaehoon Chung

> 
> ---
> 
>  drivers/mmc/sdhci.c | 5 +
>  1 file changed, 5 insertions(+)
> 
> diff --git a/drivers/mmc/sdhci.c b/drivers/mmc/sdhci.c
> index d31793a..defe6db 100644
> --- a/drivers/mmc/sdhci.c
> +++ b/drivers/mmc/sdhci.c
> @@ -602,6 +602,11 @@ int sdhci_setup_cfg(struct mmc_config *cfg, struct 
> sdhci_host *host,
>   cfg->host_caps &= ~MMC_MODE_8BIT;
>   }
>  
> + if (host->quirks & SDHCI_QUIRK_NO_HISPD_BIT) {
> + cfg->host_caps &= ~MMC_MODE_HS;
> + cfg->host_caps &= ~MMC_MODE_HS_52MHz;
> + }
> +
>   if (host->host_caps)
>   cfg->host_caps |= host->host_caps;
>  
> 

___
U-Boot mailing list
U-Boot@lists.denx.de
https://lists.denx.de/listinfo/u-boot