[U-Boot] [PATCH] mmc: sdhci: consider SDHCI_QUIRK_NO_HISPD_BIT in host_caps
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 oe5hpm@oevsv.at
---
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;
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 oe5hpm@oevsv.at
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;
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 oe5hpm@oevsv.at
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 */ 1851 card_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
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 oe5hpm@oevsv.at
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 */ 1851 card_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
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/59999.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
participants (3)
-
Hannes Schmelzer -
Hannes Schmelzer -
Jaehoon Chung