2018-05-18 22:07:07

by Scott Branden

[permalink] [raw]
Subject: [PATCH 0/3] mmc: sdhci-iproc: UHS and 32bit access fixes

Collection of bug fixes for sdhci-iproc driver.
- fix for 32bit writes for TRANSFER_MODE register by correcting shadow
register logic
- fix for deep sleep mode by adding SDHCI_QUIRK2_HOST_OFF_CARD_ON
- remove hard coded mmc capability of 1.8V to allow boards to be supported
that do support 1.8V.


Corneliu Doban (2):
mmc: sdhci-iproc: fix 32bit writes for TRANSFER_MODE register
mmc: sdhci-iproc: add SDHCI_QUIRK2_HOST_OFF_CARD_ON for cygnus

Srinath Mannam (1):
mmc: sdhci-iproc: remove hard coded mmc cap 1.8v

drivers/mmc/host/sdhci-iproc.c | 33 ++++++++++++++++++++++++++-------
1 file changed, 26 insertions(+), 7 deletions(-)

--
2.5.0



2018-05-18 22:05:02

by Scott Branden

[permalink] [raw]
Subject: [PATCH 2/3] mmc: sdhci-iproc: fix 32bit writes for TRANSFER_MODE register

From: Corneliu Doban <[email protected]>

When the host controller accepts only 32bit writes, the value of the
16bit TRANSFER_MODE register, that has the same 32bit address as the
16bit COMMAND register, needs to be saved and it will be written
in a 32bit write together with the command as this will trigger the
host to send the command on the SD interface.
When sending the tuning command, TRANSFER_MODE is written and then
sdhci_set_transfer_mode reads it back to clear AUTO_CMD12 bit and
write it again resulting in wrong value to be written because the
initial write value was saved in a shadow and the read-back returned
a wrong value, from the register.
Fix sdhci_iproc_readw to return the saved value of TRANSFER_MODE
when a saved value exist.
Same fix for read of BLOCK_SIZE and BLOCK_COUNT registers, that are
saved for a different reason, although a scenario that will cause the
mentioned problem on this registers is not probable.

Fixes: b580c52d58d9 ("mmc: sdhci-iproc: add IPROC SDHCI driver")
Signed-off-by: Corneliu Doban <[email protected]>
Signed-off-by: Scott Branden <[email protected]>
---
drivers/mmc/host/sdhci-iproc.c | 30 +++++++++++++++++++++++++-----
1 file changed, 25 insertions(+), 5 deletions(-)

diff --git a/drivers/mmc/host/sdhci-iproc.c b/drivers/mmc/host/sdhci-iproc.c
index 6f430da..1f0ab08 100644
--- a/drivers/mmc/host/sdhci-iproc.c
+++ b/drivers/mmc/host/sdhci-iproc.c
@@ -33,6 +33,8 @@ struct sdhci_iproc_host {
const struct sdhci_iproc_data *data;
u32 shadow_cmd;
u32 shadow_blk;
+ bool is_cmd_shadowed;
+ bool is_blk_shadowed;
};

#define REG_OFFSET_IN_BITS(reg) ((reg) << 3 & 0x18)
@@ -48,8 +50,22 @@ static inline u32 sdhci_iproc_readl(struct sdhci_host *host, int reg)

static u16 sdhci_iproc_readw(struct sdhci_host *host, int reg)
{
- u32 val = sdhci_iproc_readl(host, (reg & ~3));
- u16 word = val >> REG_OFFSET_IN_BITS(reg) & 0xffff;
+ struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
+ struct sdhci_iproc_host *iproc_host = sdhci_pltfm_priv(pltfm_host);
+ u32 val;
+ u16 word;
+
+ if ((reg == SDHCI_TRANSFER_MODE) && iproc_host->is_cmd_shadowed) {
+ /* Get the saved transfer mode */
+ val = iproc_host->shadow_cmd;
+ } else if ((reg == SDHCI_BLOCK_SIZE || reg == SDHCI_BLOCK_COUNT) &&
+ iproc_host->is_blk_shadowed) {
+ /* Get the saved block info */
+ val = iproc_host->shadow_blk;
+ } else {
+ val = sdhci_iproc_readl(host, (reg & ~3));
+ }
+ word = val >> REG_OFFSET_IN_BITS(reg) & 0xffff;
return word;
}

@@ -105,13 +121,15 @@ static void sdhci_iproc_writew(struct sdhci_host *host, u16 val, int reg)

if (reg == SDHCI_COMMAND) {
/* Write the block now as we are issuing a command */
- if (iproc_host->shadow_blk != 0) {
+ if (iproc_host->is_blk_shadowed) {
sdhci_iproc_writel(host, iproc_host->shadow_blk,
SDHCI_BLOCK_SIZE);
- iproc_host->shadow_blk = 0;
+ iproc_host->is_blk_shadowed = false;
}
oldval = iproc_host->shadow_cmd;
- } else if (reg == SDHCI_BLOCK_SIZE || reg == SDHCI_BLOCK_COUNT) {
+ iproc_host->is_cmd_shadowed = false;
+ } else if ((reg == SDHCI_BLOCK_SIZE || reg == SDHCI_BLOCK_COUNT) &&
+ iproc_host->is_blk_shadowed) {
/* Block size and count are stored in shadow reg */
oldval = iproc_host->shadow_blk;
} else {
@@ -123,9 +141,11 @@ static void sdhci_iproc_writew(struct sdhci_host *host, u16 val, int reg)
if (reg == SDHCI_TRANSFER_MODE) {
/* Save the transfer mode until the command is issued */
iproc_host->shadow_cmd = newval;
+ iproc_host->is_cmd_shadowed = true;
} else if (reg == SDHCI_BLOCK_SIZE || reg == SDHCI_BLOCK_COUNT) {
/* Save the block info until the command is issued */
iproc_host->shadow_blk = newval;
+ iproc_host->is_blk_shadowed = true;
} else {
/* Command or other regular 32-bit write */
sdhci_iproc_writel(host, newval, reg & ~3);
--
2.5.0


2018-05-18 22:05:22

by Scott Branden

[permalink] [raw]
Subject: [PATCH 1/3] mmc: sdhci-iproc: remove hard coded mmc cap 1.8v

From: Srinath Mannam <[email protected]>

Remove hard coded mmc cap 1.8v from platform data as it is board specific.
The 1.8v DDR mmc caps can be enabled using DTS property for those
boards that support it.

Fixes: b17b4ab8ce38 ("mmc: sdhci-iproc: define MMC caps in platform data")
Signed-off-by: Srinath Mannam <[email protected]>
Signed-off-by: Scott Branden <[email protected]>
Reviewed-by: Ray Jui <[email protected]>
---
drivers/mmc/host/sdhci-iproc.c | 1 -
1 file changed, 1 deletion(-)

diff --git a/drivers/mmc/host/sdhci-iproc.c b/drivers/mmc/host/sdhci-iproc.c
index 0ef741b..6f430da 100644
--- a/drivers/mmc/host/sdhci-iproc.c
+++ b/drivers/mmc/host/sdhci-iproc.c
@@ -206,7 +206,6 @@ static const struct sdhci_iproc_data iproc_data = {
.caps1 = SDHCI_DRIVER_TYPE_C |
SDHCI_DRIVER_TYPE_D |
SDHCI_SUPPORT_DDR50,
- .mmc_caps = MMC_CAP_1_8V_DDR,
};

static const struct sdhci_pltfm_data sdhci_bcm2835_pltfm_data = {
--
2.5.0


2018-05-18 22:05:47

by Scott Branden

[permalink] [raw]
Subject: [PATCH 3/3] mmc: sdhci-iproc: add SDHCI_QUIRK2_HOST_OFF_CARD_ON for cygnus

From: Corneliu Doban <[email protected]>

The SDHCI_QUIRK2_HOST_OFF_CARD_ON is needed for the driver to
properly reset the host controller (reset all) on initialization
after exiting deep sleep.

Signed-off-by: Corneliu Doban <[email protected]>
Signed-off-by: Scott Branden <[email protected]>
Reviewed-by: Ray Jui <[email protected]>
Reviewed-by: Srinath Mannam <[email protected]>
---
drivers/mmc/host/sdhci-iproc.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/mmc/host/sdhci-iproc.c b/drivers/mmc/host/sdhci-iproc.c
index 1f0ab08..d0e83db 100644
--- a/drivers/mmc/host/sdhci-iproc.c
+++ b/drivers/mmc/host/sdhci-iproc.c
@@ -186,7 +186,7 @@ static const struct sdhci_ops sdhci_iproc_32only_ops = {

static const struct sdhci_pltfm_data sdhci_iproc_cygnus_pltfm_data = {
.quirks = SDHCI_QUIRK_DATA_TIMEOUT_USES_SDCLK,
- .quirks2 = SDHCI_QUIRK2_ACMD23_BROKEN,
+ .quirks2 = SDHCI_QUIRK2_ACMD23_BROKEN | SDHCI_QUIRK2_HOST_OFF_CARD_ON,
.ops = &sdhci_iproc_32only_ops,
};

--
2.5.0


2018-05-21 11:37:51

by Ulf Hansson

[permalink] [raw]
Subject: Re: [PATCH 1/3] mmc: sdhci-iproc: remove hard coded mmc cap 1.8v

On 19 May 2018 at 00:03, Scott Branden <[email protected]> wrote:
> From: Srinath Mannam <[email protected]>
>
> Remove hard coded mmc cap 1.8v from platform data as it is board specific.
> The 1.8v DDR mmc caps can be enabled using DTS property for those
> boards that support it.
>
> Fixes: b17b4ab8ce38 ("mmc: sdhci-iproc: define MMC caps in platform data")
> Signed-off-by: Srinath Mannam <[email protected]>
> Signed-off-by: Scott Branden <[email protected]>
> Reviewed-by: Ray Jui <[email protected]>

Thanks, applied for and by adding a stable tag.

Kind regards
Uffe

> ---
> drivers/mmc/host/sdhci-iproc.c | 1 -
> 1 file changed, 1 deletion(-)
>
> diff --git a/drivers/mmc/host/sdhci-iproc.c b/drivers/mmc/host/sdhci-iproc.c
> index 0ef741b..6f430da 100644
> --- a/drivers/mmc/host/sdhci-iproc.c
> +++ b/drivers/mmc/host/sdhci-iproc.c
> @@ -206,7 +206,6 @@ static const struct sdhci_iproc_data iproc_data = {
> .caps1 = SDHCI_DRIVER_TYPE_C |
> SDHCI_DRIVER_TYPE_D |
> SDHCI_SUPPORT_DDR50,
> - .mmc_caps = MMC_CAP_1_8V_DDR,
> };
>
> static const struct sdhci_pltfm_data sdhci_bcm2835_pltfm_data = {
> --
> 2.5.0
>

2018-05-21 11:37:59

by Ulf Hansson

[permalink] [raw]
Subject: Re: [PATCH 2/3] mmc: sdhci-iproc: fix 32bit writes for TRANSFER_MODE register

On 19 May 2018 at 00:03, Scott Branden <[email protected]> wrote:
> From: Corneliu Doban <[email protected]>
>
> When the host controller accepts only 32bit writes, the value of the
> 16bit TRANSFER_MODE register, that has the same 32bit address as the
> 16bit COMMAND register, needs to be saved and it will be written
> in a 32bit write together with the command as this will trigger the
> host to send the command on the SD interface.
> When sending the tuning command, TRANSFER_MODE is written and then
> sdhci_set_transfer_mode reads it back to clear AUTO_CMD12 bit and
> write it again resulting in wrong value to be written because the
> initial write value was saved in a shadow and the read-back returned
> a wrong value, from the register.
> Fix sdhci_iproc_readw to return the saved value of TRANSFER_MODE
> when a saved value exist.
> Same fix for read of BLOCK_SIZE and BLOCK_COUNT registers, that are
> saved for a different reason, although a scenario that will cause the
> mentioned problem on this registers is not probable.
>
> Fixes: b580c52d58d9 ("mmc: sdhci-iproc: add IPROC SDHCI driver")
> Signed-off-by: Corneliu Doban <[email protected]>
> Signed-off-by: Scott Branden <[email protected]>

Thanks, applied for fixes and by adding a stable tag!

Kind regards
Uffe

> ---
> drivers/mmc/host/sdhci-iproc.c | 30 +++++++++++++++++++++++++-----
> 1 file changed, 25 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/mmc/host/sdhci-iproc.c b/drivers/mmc/host/sdhci-iproc.c
> index 6f430da..1f0ab08 100644
> --- a/drivers/mmc/host/sdhci-iproc.c
> +++ b/drivers/mmc/host/sdhci-iproc.c
> @@ -33,6 +33,8 @@ struct sdhci_iproc_host {
> const struct sdhci_iproc_data *data;
> u32 shadow_cmd;
> u32 shadow_blk;
> + bool is_cmd_shadowed;
> + bool is_blk_shadowed;
> };
>
> #define REG_OFFSET_IN_BITS(reg) ((reg) << 3 & 0x18)
> @@ -48,8 +50,22 @@ static inline u32 sdhci_iproc_readl(struct sdhci_host *host, int reg)
>
> static u16 sdhci_iproc_readw(struct sdhci_host *host, int reg)
> {
> - u32 val = sdhci_iproc_readl(host, (reg & ~3));
> - u16 word = val >> REG_OFFSET_IN_BITS(reg) & 0xffff;
> + struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
> + struct sdhci_iproc_host *iproc_host = sdhci_pltfm_priv(pltfm_host);
> + u32 val;
> + u16 word;
> +
> + if ((reg == SDHCI_TRANSFER_MODE) && iproc_host->is_cmd_shadowed) {
> + /* Get the saved transfer mode */
> + val = iproc_host->shadow_cmd;
> + } else if ((reg == SDHCI_BLOCK_SIZE || reg == SDHCI_BLOCK_COUNT) &&
> + iproc_host->is_blk_shadowed) {
> + /* Get the saved block info */
> + val = iproc_host->shadow_blk;
> + } else {
> + val = sdhci_iproc_readl(host, (reg & ~3));
> + }
> + word = val >> REG_OFFSET_IN_BITS(reg) & 0xffff;
> return word;
> }
>
> @@ -105,13 +121,15 @@ static void sdhci_iproc_writew(struct sdhci_host *host, u16 val, int reg)
>
> if (reg == SDHCI_COMMAND) {
> /* Write the block now as we are issuing a command */
> - if (iproc_host->shadow_blk != 0) {
> + if (iproc_host->is_blk_shadowed) {
> sdhci_iproc_writel(host, iproc_host->shadow_blk,
> SDHCI_BLOCK_SIZE);
> - iproc_host->shadow_blk = 0;
> + iproc_host->is_blk_shadowed = false;
> }
> oldval = iproc_host->shadow_cmd;
> - } else if (reg == SDHCI_BLOCK_SIZE || reg == SDHCI_BLOCK_COUNT) {
> + iproc_host->is_cmd_shadowed = false;
> + } else if ((reg == SDHCI_BLOCK_SIZE || reg == SDHCI_BLOCK_COUNT) &&
> + iproc_host->is_blk_shadowed) {
> /* Block size and count are stored in shadow reg */
> oldval = iproc_host->shadow_blk;
> } else {
> @@ -123,9 +141,11 @@ static void sdhci_iproc_writew(struct sdhci_host *host, u16 val, int reg)
> if (reg == SDHCI_TRANSFER_MODE) {
> /* Save the transfer mode until the command is issued */
> iproc_host->shadow_cmd = newval;
> + iproc_host->is_cmd_shadowed = true;
> } else if (reg == SDHCI_BLOCK_SIZE || reg == SDHCI_BLOCK_COUNT) {
> /* Save the block info until the command is issued */
> iproc_host->shadow_blk = newval;
> + iproc_host->is_blk_shadowed = true;
> } else {
> /* Command or other regular 32-bit write */
> sdhci_iproc_writel(host, newval, reg & ~3);
> --
> 2.5.0
>

2018-05-21 11:38:09

by Ulf Hansson

[permalink] [raw]
Subject: Re: [PATCH 3/3] mmc: sdhci-iproc: add SDHCI_QUIRK2_HOST_OFF_CARD_ON for cygnus

On 19 May 2018 at 00:03, Scott Branden <[email protected]> wrote:
> From: Corneliu Doban <[email protected]>
>
> The SDHCI_QUIRK2_HOST_OFF_CARD_ON is needed for the driver to
> properly reset the host controller (reset all) on initialization
> after exiting deep sleep.
>
> Signed-off-by: Corneliu Doban <[email protected]>
> Signed-off-by: Scott Branden <[email protected]>
> Reviewed-by: Ray Jui <[email protected]>
> Reviewed-by: Srinath Mannam <[email protected]>

Applied for fixes, by adding a fixes+stable tag, thanks!

Fixes: c833e92bbb60 ("mmc: sdhci-iproc: support standard byte
register accesses")
Cc: [email protected] # v4.10+

Kind regards
Uffe

> ---
> drivers/mmc/host/sdhci-iproc.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/mmc/host/sdhci-iproc.c b/drivers/mmc/host/sdhci-iproc.c
> index 1f0ab08..d0e83db 100644
> --- a/drivers/mmc/host/sdhci-iproc.c
> +++ b/drivers/mmc/host/sdhci-iproc.c
> @@ -186,7 +186,7 @@ static const struct sdhci_ops sdhci_iproc_32only_ops = {
>
> static const struct sdhci_pltfm_data sdhci_iproc_cygnus_pltfm_data = {
> .quirks = SDHCI_QUIRK_DATA_TIMEOUT_USES_SDCLK,
> - .quirks2 = SDHCI_QUIRK2_ACMD23_BROKEN,
> + .quirks2 = SDHCI_QUIRK2_ACMD23_BROKEN | SDHCI_QUIRK2_HOST_OFF_CARD_ON,
> .ops = &sdhci_iproc_32only_ops,
> };
>
> --
> 2.5.0
>