[PATCH] Revert "sdhci-of-esdhc: Support 8BIT bus width."

[PATCH] Revert "sdhci-of-esdhc: Support 8BIT bus width."

[Kevin Hao]

This reverts commit 459fe0cfda71835eacc0b24571e8425cea975688.
It causes kernel panic due to a null pointer reference to .set_bus_width
on fsl p2020rdb board.
  Unable to handle kernel paging request for instruction fetch
  Faulting instruction address: 0x00000000
  Oops: Kernel access of bad area, sig: 11 [#1]
  SMP NR_CPUS=8 P2020 RDB
  Modules linked in:
  CPU: 1 PID: 1 Comm: swapper/0 Not tainted 4.1.0-rc3-next-20150514-00001-g76c7a15bee83 #103
  task: ea858000 ti: ea846000 task.ti: ea846000
  NIP: 00000000 LR: c048036c CTR: 00000000
  REGS: ea847c70 TRAP: 0400   Not tainted  (4.1.0-rc3-next-20150514-00001-g76c7a15bee83)
  MSR: 00021000 <CE,ME>  CR: 22010022  XER: 00000000

  GPR00: c04802c4 ea847d20 ea858000 eaa3ab00 00000000 00000f20 00000002 00000000
  GPR08: 00000000 00000000 00000000 90000000 22010022 00000000 c0002a68 00000000
  GPR16: 00000000 00000000 00000000 00000000 00000000 00000000 c08a64e0 000000e9
  GPR24: c080f398 c08a0000 00000000 0000000e 00029000 eaa3ab28 eaa3a9f0 eaa3ab00
  NIP [00000000]   (null)
  LR [c048036c] sdhci_do_set_ios+0x164/0x824
  Call Trace:
  [ea847d20] [c04802c4] sdhci_do_set_ios+0xbc/0x824 (unreliable)
  [ea847d40] [c0480a60] sdhci_set_ios+0x34/0x4c
  [ea847d50] [c046c96c] mmc_power_up.part.16+0x3c/0x120
  [ea847d70] [c046da10] mmc_start_host+0x50/0xa4
  [ea847d80] [c046ee28] mmc_add_host+0x50/0x78
  [ea847d90] [c0481438] sdhci_add_host+0x8c4/0xcc0
  [ea847db0] [c0485824] sdhci_esdhc_probe+0xa8/0xc8
  [ea847dd0] [c032ed28] platform_drv_probe+0x3c/0xa4
  [ea847df0] [c032d170] driver_probe_device+0x1f0/0x2c4
  [ea847e10] [c032d370] __driver_attach+0xbc/0xc0
  [ea847e30] [c032b178] bus_for_each_dev+0x6c/0xb8
  [ea847e60] [c032c6b8] bus_add_driver+0x168/0x220
  [ea847e80] [c032da70] driver_register+0x88/0x130
  [ea847e90] [c000234c] do_one_initcall+0x8c/0x1e0
  [ea847f00] [c0815b08] kernel_init_freeable+0x138/0x1d4
  [ea847f30] [c0002a7c] kernel_init+0x14/0x100
  [ea847f40] [c000e838] ret_from_kernel_thread+0x5c/0x64

Signed-off-by: Kevin Hao <haokexin@gmail.com>
---
 drivers/mmc/host/sdhci-of-esdhc.c | 31 +++++++++++++++++++++++--------
 1 file changed, 23 insertions(+), 8 deletions(-)

diff --git a/drivers/mmc/host/sdhci-of-esdhc.c b/drivers/mmc/host/sdhci-of-esdhc.c
index 7a98a2207976..1804bdbdb145 100644
--- a/drivers/mmc/host/sdhci-of-esdhc.c
+++ b/drivers/mmc/host/sdhci-of-esdhc.c
@@ -82,10 +82,6 @@ static u8 esdhc_readb(struct sdhci_host *host, int reg)
 		/* fixup the result */
 		ret &= ~SDHCI_CTRL_DMA_MASK;
 		ret |= dma_bits;
-
-		/* 8BIT is bit 29 in Control register */
-		ret |= ((ret << 3) & SDHCI_CTRL_8BITBUS);
-		ret &= ~(SDHCI_CTRL_8BITBUS >> 3);
 	}
 
 	return ret;
@@ -138,10 +134,6 @@ static void esdhc_writeb(struct sdhci_host *host, u8 val, int reg)
 			dma_bits);
 		val &= ~SDHCI_CTRL_DMA_MASK;
 		val |= in_be32(host->ioaddr + reg) & SDHCI_CTRL_DMA_MASK;
-
-		/* 8BIT is bit 29 in Control register */
-		val |= ((val & SDHCI_CTRL_8BITBUS) >> 3);
-		val = (val & ~SDHCI_CTRL_8BITBUS);
 	}
 
 	/* Prevent SDHCI core from writing reserved bits (e.g. HISPD). */
@@ -262,6 +254,28 @@ static void esdhc_of_platform_init(struct sdhci_host *host)
 		host->quirks &= ~SDHCI_QUIRK_NO_BUSY_IRQ;
 }
 
+static void esdhc_pltfm_set_bus_width(struct sdhci_host *host, int width)
+{
+	u32 ctrl;
+
+	switch (width) {
+	case MMC_BUS_WIDTH_8:
+		ctrl = ESDHC_CTRL_8BITBUS;
+		break;
+
+	case MMC_BUS_WIDTH_4:
+		ctrl = ESDHC_CTRL_4BITBUS;
+		break;
+
+	default:
+		ctrl = 0;
+		break;
+	}
+
+	clrsetbits_be32(host->ioaddr + SDHCI_HOST_CONTROL,
+			ESDHC_CTRL_BUSWIDTH_MASK, ctrl);
+}
+
 static void esdhc_reset(struct sdhci_host *host, u8 mask)
 {
 	sdhci_reset(host, mask);
@@ -283,6 +297,7 @@ static const struct sdhci_ops sdhci_esdhc_ops = {
 	.get_min_clock = esdhc_of_get_min_clock,
 	.platform_init = esdhc_of_platform_init,
 	.adma_workaround = esdhci_of_adma_workaround,
+	.set_bus_width = esdhc_pltfm_set_bus_width,
 	.reset = esdhc_reset,
 	.set_uhs_signaling = sdhci_set_uhs_signaling,
 };
-- 
2.1.0

Re: [PATCH] Revert "sdhci-of-esdhc: Support 8BIT bus width."

[Joakim Tjernlund]

On Fri, 2015-05-15 at 14:29 +0800, Kevin Hao wrote:
> This reverts commit 459fe0cfda71835eacc0b24571e8425cea975688.
> It causes kernel panic due to a null pointer reference to .set_bus_width
> on fsl p2020rdb board.
>   Unable to handle kernel paging request for instruction fetch
>   Faulting instruction address: 0x00000000
>   Oops: Kernel access of bad area, sig: 11 [#1]
>   SMP NR_CPUS=8 P2020 RDB
>   Modules linked in:
>   CPU: 1 PID: 1 Comm: swapper/0 Not tainted 4.1.0-rc3-next-20150514-00001-g76c7a15bee83 #103
>   task: ea858000 ti: ea846000 task.ti: ea846000
>   NIP: 00000000 LR: c048036c CTR: 00000000
>   REGS: ea847c70 TRAP: 0400   Not tainted  (4.1.0-rc3-next-20150514-00001-g76c7a15bee83)
>   MSR: 00021000 <CE,ME>  CR: 22010022  XER: 00000000
> 
>   GPR00: c04802c4 ea847d20 ea858000 eaa3ab00 00000000 00000f20 00000002 00000000
>   GPR08: 00000000 00000000 00000000 90000000 22010022 00000000 c0002a68 00000000
>   GPR16: 00000000 00000000 00000000 00000000 00000000 00000000 c08a64e0 000000e9
>   GPR24: c080f398 c08a0000 00000000 0000000e 00029000 eaa3ab28 eaa3a9f0 eaa3ab00
>   NIP [00000000]   (null)
>   LR [c048036c] sdhci_do_set_ios+0x164/0x824
>   Call Trace:
>   [ea847d20] [c04802c4] sdhci_do_set_ios+0xbc/0x824 (unreliable)
>   [ea847d40] [c0480a60] sdhci_set_ios+0x34/0x4c
>   [ea847d50] [c046c96c] mmc_power_up.part.16+0x3c/0x120
>   [ea847d70] [c046da10] mmc_start_host+0x50/0xa4
>   [ea847d80] [c046ee28] mmc_add_host+0x50/0x78
>   [ea847d90] [c0481438] sdhci_add_host+0x8c4/0xcc0
>   [ea847db0] [c0485824] sdhci_esdhc_probe+0xa8/0xc8
>   [ea847dd0] [c032ed28] platform_drv_probe+0x3c/0xa4
>   [ea847df0] [c032d170] driver_probe_device+0x1f0/0x2c4
>   [ea847e10] [c032d370] __driver_attach+0xbc/0xc0
>   [ea847e30] [c032b178] bus_for_each_dev+0x6c/0xb8
>   [ea847e60] [c032c6b8] bus_add_driver+0x168/0x220
>   [ea847e80] [c032da70] driver_register+0x88/0x130
>   [ea847e90] [c000234c] do_one_initcall+0x8c/0x1e0
>   [ea847f00] [c0815b08] kernel_init_freeable+0x138/0x1d4
>   [ea847f30] [c0002a7c] kernel_init+0x14/0x100
>   [ea847f40] [c000e838] ret_from_kernel_thread+0x5c/0x64
> 
> Signed-off-by: Kevin Hao <haokexin@gmail.com>

Reverting this seem a bit to much. Looking in the log it seems commit
 2317f56c055fcad524bf6a873df48a754e7ebc4d 
introduced the error by not checking for host->ops->set_bus_width before
calling it.

How does this work for you?

diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
index c80287a..23603b2 100644
--- a/drivers/mmc/host/sdhci.c
+++ b/drivers/mmc/host/sdhci.c
@@ -1528,7 +1528,10 @@ static void sdhci_do_set_ios(struct sdhci_host *host, struct mmc_ios *ios)
        if (host->ops->platform_send_init_74_clocks)
                host->ops->platform_send_init_74_clocks(host, ios->power_mode);
 
-       host->ops->set_bus_width(host, ios->bus_width);
+       if (host->ops->set_bus_width)
+               host->ops->set_bus_width(host, ios->bus_width);
+       else
+               sdhci_set_bus_width(host, ios->bus_width);

Re: [PATCH] Revert "sdhci-of-esdhc: Support 8BIT bus width."

[Joakim Tjernlund]

On Fri, 2015-05-15 at 09:02 +0200, an unknown sender wrote:
> On Fri, 2015-05-15 at 14:29 +0800, Kevin Hao wrote:
> > This reverts commit 459fe0cfda71835eacc0b24571e8425cea975688.
> > It causes kernel panic due to a null pointer reference to .set_bus_width
> > on fsl p2020rdb board.
> >   Unable to handle kernel paging request for instruction fetch
> >   Faulting instruction address: 0x00000000
> >   Oops: Kernel access of bad area, sig: 11 [#1]
> >   SMP NR_CPUS=8 P2020 RDB
> >   Modules linked in:
> >   CPU: 1 PID: 1 Comm: swapper/0 Not tainted 4.1.0-rc3-next-20150514-00001-g76c7a15bee83 #103
> >   task: ea858000 ti: ea846000 task.ti: ea846000
> >   NIP: 00000000 LR: c048036c CTR: 00000000
> >   REGS: ea847c70 TRAP: 0400   Not tainted  (4.1.0-rc3-next-20150514-00001-g76c7a15bee83)
> >   MSR: 00021000 <CE,ME>  CR: 22010022  XER: 00000000
> > 
> >   GPR00: c04802c4 ea847d20 ea858000 eaa3ab00 00000000 00000f20 00000002 00000000
> >   GPR08: 00000000 00000000 00000000 90000000 22010022 00000000 c0002a68 00000000
> >   GPR16: 00000000 00000000 00000000 00000000 00000000 00000000 c08a64e0 000000e9
> >   GPR24: c080f398 c08a0000 00000000 0000000e 00029000 eaa3ab28 eaa3a9f0 eaa3ab00
> >   NIP [00000000]   (null)
> >   LR [c048036c] sdhci_do_set_ios+0x164/0x824
> >   Call Trace:
> >   [ea847d20] [c04802c4] sdhci_do_set_ios+0xbc/0x824 (unreliable)
> >   [ea847d40] [c0480a60] sdhci_set_ios+0x34/0x4c
> >   [ea847d50] [c046c96c] mmc_power_up.part.16+0x3c/0x120
> >   [ea847d70] [c046da10] mmc_start_host+0x50/0xa4
> >   [ea847d80] [c046ee28] mmc_add_host+0x50/0x78
> >   [ea847d90] [c0481438] sdhci_add_host+0x8c4/0xcc0
> >   [ea847db0] [c0485824] sdhci_esdhc_probe+0xa8/0xc8
> >   [ea847dd0] [c032ed28] platform_drv_probe+0x3c/0xa4
> >   [ea847df0] [c032d170] driver_probe_device+0x1f0/0x2c4
> >   [ea847e10] [c032d370] __driver_attach+0xbc/0xc0
> >   [ea847e30] [c032b178] bus_for_each_dev+0x6c/0xb8
> >   [ea847e60] [c032c6b8] bus_add_driver+0x168/0x220
> >   [ea847e80] [c032da70] driver_register+0x88/0x130
> >   [ea847e90] [c000234c] do_one_initcall+0x8c/0x1e0
> >   [ea847f00] [c0815b08] kernel_init_freeable+0x138/0x1d4
> >   [ea847f30] [c0002a7c] kernel_init+0x14/0x100
> >   [ea847f40] [c000e838] ret_from_kernel_thread+0x5c/0x64
> > 
> > Signed-off-by: Kevin Hao <haokexin@gmail.com>
> 
> Reverting this seem a bit to much. Looking in the log it seems commit
>  2317f56c055fcad524bf6a873df48a754e7ebc4d 
> introduced the error by not checking for host->ops->set_bus_width before
> calling it.
> 
> How does this work for you?
> 
> diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
> index c80287a..23603b2 100644
> --- a/drivers/mmc/host/sdhci.c
> +++ b/drivers/mmc/host/sdhci.c
> @@ -1528,7 +1528,10 @@ static void sdhci_do_set_ios(struct sdhci_host *host, struct mmc_ios *ios)
>         if (host->ops->platform_send_init_74_clocks)
>                 host->ops->platform_send_init_74_clocks(host, ios->power_mode);
>  
> -       host->ops->set_bus_width(host, ios->bus_width);
> +       if (host->ops->set_bus_width)
> +               host->ops->set_bus_width(host, ios->bus_width);
> +       else
> +               sdhci_set_bus_width(host, ios->bus_width);

Ahh, now I see. Drivers are supposed to call sdhci_set_bus_width instead of NULL:
Instead of reverting this add:

 diff --git a/drivers/mmc/host/sdhci-of-esdhc.c b/drivers/mmc/host/sdhci-of-esdhc.c
index 7a98a22..07b9df2 100644
--- a/drivers/mmc/host/sdhci-of-esdhc.c
+++ b/drivers/mmc/host/sdhci-of-esdhc.c
@@ -283,6 +283,7 @@ static const struct sdhci_ops sdhci_esdhc_ops = {
        .get_min_clock = esdhc_of_get_min_clock,
        .platform_init = esdhc_of_platform_init,
        .adma_workaround = esdhci_of_adma_workaround,
+       .set_bus_width = sdhci_set_bus_width,
        .reset = esdhc_reset,
        .set_uhs_signaling = sdhci_set_uhs_signaling,
 };

Should I repost the full "sdhci-of-esdhc: Support 8BIT bus width." with this fix added
of just the above fix?

 Jocke

Re: [PATCH] Revert "sdhci-of-esdhc: Support 8BIT bus width."

[Kevin Hao]

[-- Attachment #1: Type: text/plain, Size: 1305 bytes --]

On Fri, May 15, 2015 at 07:02:44AM +0000, Joakim Tjernlund wrote:
> Reverting this seem a bit to much. Looking in the log it seems commit
>  2317f56c055fcad524bf6a873df48a754e7ebc4d 
> introduced the error by not checking for host->ops->set_bus_width before
> calling it.
> 
> How does this work for you?

No. My sd card still don't work with this change.

> 
> diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
> index c80287a..23603b2 100644
> --- a/drivers/mmc/host/sdhci.c
> +++ b/drivers/mmc/host/sdhci.c
> @@ -1528,7 +1528,10 @@ static void sdhci_do_set_ios(struct sdhci_host *host, struct mmc_ios *ios)
>         if (host->ops->platform_send_init_74_clocks)
>                 host->ops->platform_send_init_74_clocks(host, ios->power_mode);
>  
> -       host->ops->set_bus_width(host, ios->bus_width);
> +       if (host->ops->set_bus_width)
> +               host->ops->set_bus_width(host, ios->bus_width);
> +       else
> +               sdhci_set_bus_width(host, ios->bus_width);

The default sdhci_set_bus_width() doesn't apply to the esdhc controller on my
board (p2020rdb). This controller is compatible with the sd host specification
2.0 and the implementation of sdhci_set_bus_width() can't set the bus width
correctly for it.

Thanks,
Kevin

[-- Attachment #2: Type: application/pgp-signature, Size: 473 bytes --]

Re: [PATCH] Revert "sdhci-of-esdhc: Support 8BIT bus width."

[Kevin Hao]

[-- Attachment #1: Type: text/plain, Size: 947 bytes --]

On Fri, May 15, 2015 at 07:24:55AM +0000, Joakim Tjernlund wrote:
> Ahh, now I see. Drivers are supposed to call sdhci_set_bus_width instead of NULL:
> Instead of reverting this add:
> 
>  diff --git a/drivers/mmc/host/sdhci-of-esdhc.c b/drivers/mmc/host/sdhci-of-esdhc.c
> index 7a98a22..07b9df2 100644
> --- a/drivers/mmc/host/sdhci-of-esdhc.c
> +++ b/drivers/mmc/host/sdhci-of-esdhc.c
> @@ -283,6 +283,7 @@ static const struct sdhci_ops sdhci_esdhc_ops = {
>         .get_min_clock = esdhc_of_get_min_clock,
>         .platform_init = esdhc_of_platform_init,
>         .adma_workaround = esdhci_of_adma_workaround,
> +       .set_bus_width = sdhci_set_bus_width,
>         .reset = esdhc_reset,
>         .set_uhs_signaling = sdhci_set_uhs_signaling,
>  };
> 
> Should I repost the full "sdhci-of-esdhc: Support 8BIT bus width." with this fix added
> of just the above fix?

Sorry, this still don't work.

Thanks,
Kevin

[-- Attachment #2: Type: application/pgp-signature, Size: 473 bytes --]

Re: [PATCH] Revert "sdhci-of-esdhc: Support 8BIT bus width."

[Joakim Tjernlund]

On Fri, 2015-05-15 at 15:36 +0800, Kevin Hao wrote:
> On Fri, May 15, 2015 at 07:02:44AM +0000, Joakim Tjernlund wrote:
> > Reverting this seem a bit to much. Looking in the log it seems commit
> >  2317f56c055fcad524bf6a873df48a754e7ebc4d 
> > introduced the error by not checking for host->ops->set_bus_width before
> > calling it.
> > 
> > How does this work for you?
> 
> No. My sd card still don't work with this change.
> 
> > 
> > diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
> > index c80287a..23603b2 100644
> > --- a/drivers/mmc/host/sdhci.c
> > +++ b/drivers/mmc/host/sdhci.c
> > @@ -1528,7 +1528,10 @@ static void sdhci_do_set_ios(struct sdhci_host *host, struct mmc_ios *ios)
> >         if (host->ops->platform_send_init_74_clocks)
> >                 host->ops->platform_send_init_74_clocks(host, ios->power_mode);
> >  
> > -       host->ops->set_bus_width(host, ios->bus_width);
> > +       if (host->ops->set_bus_width)
> > +               host->ops->set_bus_width(host, ios->bus_width);
> > +       else
> > +               sdhci_set_bus_width(host, ios->bus_width);
> 
> The default sdhci_set_bus_width() doesn't apply to the esdhc controller on my
> board (p2020rdb). This controller is compatible with the sd host specification
> 2.0 and the implementation of sdhci_set_bus_width() can't set the bus width
> correctly for it.

But the controller is the same as for T1042 I presume? The bus width is in SDHCI_HOST_CONTROL
and the 8BIT is bit 29 in the register?

What bus width do you use and what is the failure?

 Jocke
 

Re: [PATCH] Revert "sdhci-of-esdhc: Support 8BIT bus width."

[Joakim Tjernlund]

On Fri, 2015-05-15 at 15:42 +0800, Kevin Hao wrote:
> On Fri, May 15, 2015 at 07:24:55AM +0000, Joakim Tjernlund wrote:
> > Ahh, now I see. Drivers are supposed to call sdhci_set_bus_width instead of NULL:
> > Instead of reverting this add:
> > 
> >  diff --git a/drivers/mmc/host/sdhci-of-esdhc.c b/drivers/mmc/host/sdhci-of-esdhc.c
> > index 7a98a22..07b9df2 100644
> > --- a/drivers/mmc/host/sdhci-of-esdhc.c
> > +++ b/drivers/mmc/host/sdhci-of-esdhc.c
> > @@ -283,6 +283,7 @@ static const struct sdhci_ops sdhci_esdhc_ops = {
> >         .get_min_clock = esdhc_of_get_min_clock,
> >         .platform_init = esdhc_of_platform_init,
> >         .adma_workaround = esdhci_of_adma_workaround,
> > +       .set_bus_width = sdhci_set_bus_width,
> >         .reset = esdhc_reset,
> >         .set_uhs_signaling = sdhci_set_uhs_signaling,
> >  };
> > 
> > Should I repost the full "sdhci-of-esdhc: Support 8BIT bus width." with this fix added
> > of just the above fix?
> 
> Sorry, this still don't work.

hmm, looking at
sdhci_set_bus_width(struct sdhci_host *host, int width)
{
        u8 ctrl;

        ctrl = sdhci_readb(host, SDHCI_HOST_CONTROL);
        if (width == MMC_BUS_WIDTH_8) {
                ctrl &= ~SDHCI_CTRL_4BITBUS;
                if (host->version >= SDHCI_SPEC_300)
                        ctrl |= SDHCI_CTRL_8BITBUS;

I suspect SDHCI_SPEC_300 is not set for you? I see that 
sdhci-esdhc-imx.c has:
static u16 esdhc_readw_le(struct sdhci_host *host, int reg)
{
        struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
        struct pltfm_imx_data *imx_data = pltfm_host->priv;
        u16 ret = 0;
        u32 val;

        if (unlikely(reg == SDHCI_HOST_VERSION)) {
                reg ^= 2;
                if (esdhc_is_usdhc(imx_data)) {
                        /*
                         * The usdhc register returns a wrong host version.
                         * Correct it here.
                         */
                        return SDHCI_SPEC_300;
                }

so I am guessing that our driver needs either needs something similar or
just an own impl of sdhci_set_bus_width:

void sdhci_set_bus_width(struct sdhci_host *host, int width)
{
        u8 ctrl;

        ctrl = sdhci_readb(host, SDHCI_HOST_CONTROL);
        if (width == MMC_BUS_WIDTH_8) {
                ctrl &= ~SDHCI_CTRL_4BITBUS;
                ctrl |= SDHCI_CTRL_8BITBUS;
        } else {
                ctrl &= ~SDHCI_CTRL_8BITBUS;
                if (width == MMC_BUS_WIDTH_4)
                        ctrl |= SDHCI_CTRL_4BITBUS;
                else
                        ctrl &= ~SDHCI_CTRL_4BITBUS;
        }
        sdhci_writeb(host, ctrl, SDHCI_HOST_CONTROL);
}

 Jocke

Re: [PATCH] Revert "sdhci-of-esdhc: Support 8BIT bus width."

[Joakim Tjernlund]

On Fri, 2015-05-15 at 15:42 +0800, Kevin Hao wrote:
> On Fri, May 15, 2015 at 07:24:55AM +0000, Joakim Tjernlund wrote:
> > Ahh, now I see. Drivers are supposed to call sdhci_set_bus_width instead of NULL:
> > Instead of reverting this add:
> > 
> >  diff --git a/drivers/mmc/host/sdhci-of-esdhc.c b/drivers/mmc/host/sdhci-of-esdhc.c
> > index 7a98a22..07b9df2 100644
> > --- a/drivers/mmc/host/sdhci-of-esdhc.c
> > +++ b/drivers/mmc/host/sdhci-of-esdhc.c
> > @@ -283,6 +283,7 @@ static const struct sdhci_ops sdhci_esdhc_ops = {
> >         .get_min_clock = esdhc_of_get_min_clock,
> >         .platform_init = esdhc_of_platform_init,
> >         .adma_workaround = esdhci_of_adma_workaround,
> > +       .set_bus_width = sdhci_set_bus_width,
> >         .reset = esdhc_reset,
> >         .set_uhs_signaling = sdhci_set_uhs_signaling,
> >  };
> > 
> > Should I repost the full "sdhci-of-esdhc: Support 8BIT bus width." with this fix added
> > of just the above fix?
> 
> Sorry, this still don't work.

What about this then:

diff --git a/drivers/mmc/host/sdhci-of-esdhc.c b/drivers/mmc/host/sdhci-of-esdhc.c
index 7a98a22..569fce1 100644
--- a/drivers/mmc/host/sdhci-of-esdhc.c
+++ b/drivers/mmc/host/sdhci-of-esdhc.c
@@ -262,6 +262,24 @@ static void esdhc_of_platform_init(struct sdhci_host *host)
                host->quirks &= ~SDHCI_QUIRK_NO_BUSY_IRQ;
 }
 
+static void esdhc_pltfm_set_bus_width(struct sdhci_host *host, int width)
+{
+       u8 ctrl;
+
+       ctrl = sdhci_readb(host, SDHCI_HOST_CONTROL);
+       if (width == MMC_BUS_WIDTH_8) {
+               ctrl &= ~SDHCI_CTRL_4BITBUS;
+               ctrl |= SDHCI_CTRL_8BITBUS;
+       } else {
+               ctrl &= ~SDHCI_CTRL_8BITBUS;
+               if (width == MMC_BUS_WIDTH_4)
+                       ctrl |= SDHCI_CTRL_4BITBUS;
+               else
+                       ctrl &= ~SDHCI_CTRL_4BITBUS;
+       }
+       sdhci_writeb(host, ctrl, SDHCI_HOST_CONTROL);
+}
+
 static void esdhc_reset(struct sdhci_host *host, u8 mask)
 {
        sdhci_reset(host, mask);
@@ -283,6 +301,7 @@ static const struct sdhci_ops sdhci_esdhc_ops = {
        .get_min_clock = esdhc_of_get_min_clock,
        .platform_init = esdhc_of_platform_init,
        .adma_workaround = esdhci_of_adma_workaround,
+       .set_bus_width = esdhc_pltfm_set_bus_width,
        .reset = esdhc_reset,
        .set_uhs_signaling = sdhci_set_uhs_signaling,
 };

Re: [PATCH] Revert "sdhci-of-esdhc: Support 8BIT bus width."

[Joakim Tjernlund]

On Fri, 2015-05-15 at 15:42 +0800, Kevin Hao wrote:
> On Fri, May 15, 2015 at 07:24:55AM +0000, Joakim Tjernlund wrote:
> > Ahh, now I see. Drivers are supposed to call sdhci_set_bus_width instead of NULL:
> > Instead of reverting this add:
> > 
> >  diff --git a/drivers/mmc/host/sdhci-of-esdhc.c b/drivers/mmc/host/sdhci-of-esdhc.c
> > index 7a98a22..07b9df2 100644
> > --- a/drivers/mmc/host/sdhci-of-esdhc.c
> > +++ b/drivers/mmc/host/sdhci-of-esdhc.c
> > @@ -283,6 +283,7 @@ static const struct sdhci_ops sdhci_esdhc_ops = {
> >         .get_min_clock = esdhc_of_get_min_clock,
> >         .platform_init = esdhc_of_platform_init,
> >         .adma_workaround = esdhci_of_adma_workaround,
> > +       .set_bus_width = sdhci_set_bus_width,
> >         .reset = esdhc_reset,
> >         .set_uhs_signaling = sdhci_set_uhs_signaling,
> >  };
> > 
> > Should I repost the full "sdhci-of-esdhc: Support 8BIT bus width." with this fix added
> > of just the above fix?
> 
> Sorry, this still don't work.

How about this one:

>From af6b18c056b6064424bd2ab1f9989bbadae5e701 Mon Sep 17 00:00:00 2001
From: Joakim Tjernlund <joakim.tjernlund@transmode.se>
Date: Mon, 20 Apr 2015 22:36:55 +0200
Subject: [PATCHv3] sdhci-of-esdhc: Support 8BIT bus width.

esdhc_readb()/esdhc_writeb() did not adjust for 8BIT.

Signed-off-by: Joakim Tjernlund <joakim.tjernlund@transmode.se>
---
 v3 - v2 had a few misunderstandings in it w.r.t bus_with handling.
      sdhci-of-esdhc.c appears to be a bit of a mess w.r.t to this so
      here is v3 which only touchs esdhc_readb()/esdhc_writeb() so
      8BIT is preserverd.

 v2 - I had fogotten to add bus-width = <8> to my DTS.
      Adding this simplified things
 drivers/mmc/host/sdhci-of-esdhc.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/drivers/mmc/host/sdhci-of-esdhc.c b/drivers/mmc/host/sdhci-of-esdhc.c
index 1804bdb..233e38d 100644
--- a/drivers/mmc/host/sdhci-of-esdhc.c
+++ b/drivers/mmc/host/sdhci-of-esdhc.c
@@ -82,6 +82,10 @@ static u8 esdhc_readb(struct sdhci_host *host, int reg)
                /* fixup the result */
                ret &= ~SDHCI_CTRL_DMA_MASK;
                ret |= dma_bits;
+
+               /* 8BIT is bit 29 in Control register */
+               ret |= ((ret << 3) & SDHCI_CTRL_8BITBUS);
+               ret &= ~(SDHCI_CTRL_8BITBUS >> 3);
        }
 
        return ret;
@@ -134,6 +138,10 @@ static void esdhc_writeb(struct sdhci_host *host, u8 val, int reg)
                        dma_bits);
                val &= ~SDHCI_CTRL_DMA_MASK;
                val |= in_be32(host->ioaddr + reg) & SDHCI_CTRL_DMA_MASK;
+
+               /* 8BIT is bit 29 in Control register */
+               val |= ((val & SDHCI_CTRL_8BITBUS) >> 3);
+               val = (val & ~SDHCI_CTRL_8BITBUS);
        }
 
        /* Prevent SDHCI core from writing reserved bits (e.g. HISPD). */
-- 
2.3.6

Re: [PATCH] Revert "sdhci-of-esdhc: Support 8BIT bus width."

[Kevin Hao]

[-- Attachment #1: Type: text/plain, Size: 2030 bytes --]

On Fri, May 15, 2015 at 07:49:29AM +0000, Joakim Tjernlund wrote:
> On Fri, 2015-05-15 at 15:36 +0800, Kevin Hao wrote:
> > On Fri, May 15, 2015 at 07:02:44AM +0000, Joakim Tjernlund wrote:
> > > Reverting this seem a bit to much. Looking in the log it seems commit
> > >  2317f56c055fcad524bf6a873df48a754e7ebc4d 
> > > introduced the error by not checking for host->ops->set_bus_width before
> > > calling it.
> > > 
> > > How does this work for you?
> > 
> > No. My sd card still don't work with this change.
> > 
> > > 
> > > diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
> > > index c80287a..23603b2 100644
> > > --- a/drivers/mmc/host/sdhci.c
> > > +++ b/drivers/mmc/host/sdhci.c
> > > @@ -1528,7 +1528,10 @@ static void sdhci_do_set_ios(struct sdhci_host *host, struct mmc_ios *ios)
> > >         if (host->ops->platform_send_init_74_clocks)
> > >                 host->ops->platform_send_init_74_clocks(host, ios->power_mode);
> > >  
> > > -       host->ops->set_bus_width(host, ios->bus_width);
> > > +       if (host->ops->set_bus_width)
> > > +               host->ops->set_bus_width(host, ios->bus_width);
> > > +       else
> > > +               sdhci_set_bus_width(host, ios->bus_width);
> > 
> > The default sdhci_set_bus_width() doesn't apply to the esdhc controller on my
> > board (p2020rdb). This controller is compatible with the sd host specification
> > 2.0 and the implementation of sdhci_set_bus_width() can't set the bus width
> > correctly for it.
> 
> But the controller is the same as for T1042 I presume?

No. As I know the controller on t1042 is compatible with sd host specification 3.

> The bus width is in SDHCI_HOST_CONTROL
> and the 8BIT is bit 29 in the register?

Yes, that is true.

> 
> What bus width do you use and what is the failure?

4 bit sd card. I got some error like this:
mmcblk0: error -84 transferring data, sector 3911672, nr 8, cmd response 0x900, card status 0xb0

Thanks,
Kevin

> 
>nk  Jocke
>  
> 

[-- Attachment #2: Type: application/pgp-signature, Size: 473 bytes --]

Re: [PATCH] Revert "sdhci-of-esdhc: Support 8BIT bus width."

[Kevin Hao]

[-- Attachment #1: Type: text/plain, Size: 561 bytes --]

> How about this one:
> 
> From af6b18c056b6064424bd2ab1f9989bbadae5e701 Mon Sep 17 00:00:00 2001
> From: Joakim Tjernlund <joakim.tjernlund@transmode.se>
> Date: Mon, 20 Apr 2015 22:36:55 +0200
> Subject: [PATCHv3] sdhci-of-esdhc: Support 8BIT bus width.
> 
> esdhc_readb()/esdhc_writeb() did not adjust for 8BIT.

Do we really need this for the 8bit bus support? There is already a specific
API for setting the bus width, this change seems unnecessary to me. That is
also why I choose to revert that patch. Did I miss something?

Thanks,
Kevin

[-- Attachment #2: Type: application/pgp-signature, Size: 473 bytes --]

Re: [PATCH] Revert "sdhci-of-esdhc: Support 8BIT bus width."

[Joakim Tjernlund]

On Sun, 2015-05-17 at 13:06 +0800, Kevin Hao wrote:
> > 
> > How about this one:
> > 
> > From af6b18c056b6064424bd2ab1f9989bbadae5e701 Mon Sep 17 00:00:00 2001
> > From: Joakim Tjernlund <joakim.tjernlund@transmode.se>
> > Date: Mon, 20 Apr 2015 22:36:55 +0200
> > Subject: [PATCHv3] sdhci-of-esdhc: Support 8BIT bus width.
> > 
> > esdhc_readb()/esdhc_writeb() did not adjust for 8BIT.
> 
> Do we really need this for the 8bit bus support? There is already a specific
> API for setting the bus width, this change seems unnecessary to me. That is
> also why I choose to revert that patch. Did I miss something?

We do, the bus API really only works well when the bus bits are in another 
register but the HOST_CONTROL register.
The only reason 4BIT works is because its bit placement is where 
SDHCI expects it to be. 8BIT is not, so unless readb/writeb funktions compensate
for that they will overwrite what the bus API set earlier.

Atleast this is my understanding, Ulf?

Didn't this patch work for you either?

 Jocke

Re: [PATCH] Revert "sdhci-of-esdhc: Support 8BIT bus width."

[Ulf Hansson]

On 17 May 2015 at 10:36, Joakim Tjernlund <joakim.tjernlund@transmode.se> wrote:
> On Sun, 2015-05-17 at 13:06 +0800, Kevin Hao wrote:
>> >
>> > How about this one:
>> >
>> > From af6b18c056b6064424bd2ab1f9989bbadae5e701 Mon Sep 17 00:00:00 2001
>> > From: Joakim Tjernlund <joakim.tjernlund@transmode.se>
>> > Date: Mon, 20 Apr 2015 22:36:55 +0200
>> > Subject: [PATCHv3] sdhci-of-esdhc: Support 8BIT bus width.
>> >
>> > esdhc_readb()/esdhc_writeb() did not adjust for 8BIT.
>>
>> Do we really need this for the 8bit bus support? There is already a specific
>> API for setting the bus width, this change seems unnecessary to me. That is
>> also why I choose to revert that patch. Did I miss something?
>
> We do, the bus API really only works well when the bus bits are in another
> register but the HOST_CONTROL register.
> The only reason 4BIT works is because its bit placement is where
> SDHCI expects it to be. 8BIT is not, so unless readb/writeb funktions compensate
> for that they will overwrite what the bus API set earlier.
>
> Atleast this is my understanding, Ulf?

Sorry, I don't have enough knowledge about sdhci to give you good answer.

>
> Didn't this patch work for you either?

I have dropped the "sdhci-of-edsdhc: Support 8-bit..." from my next
branch for now.

A "Tested-by" confirmation of a new version of $subject patch, would
help me to know when it can go in.

Kind regards
Uffe

Re: [PATCH] Revert "sdhci-of-esdhc: Support 8BIT bus width."

[Kevin Hao]

[-- Attachment #1: Type: text/plain, Size: 1933 bytes --]

On Sun, May 17, 2015 at 08:36:07AM +0000, Joakim Tjernlund wrote:
> On Sun, 2015-05-17 at 13:06 +0800, Kevin Hao wrote:
> > > 
> > > How about this one:
> > > 
> > > From af6b18c056b6064424bd2ab1f9989bbadae5e701 Mon Sep 17 00:00:00 2001
> > > From: Joakim Tjernlund <joakim.tjernlund@transmode.se>
> > > Date: Mon, 20 Apr 2015 22:36:55 +0200
> > > Subject: [PATCHv3] sdhci-of-esdhc: Support 8BIT bus width.
> > > 
> > > esdhc_readb()/esdhc_writeb() did not adjust for 8BIT.
> > 
> > Do we really need this for the 8bit bus support? There is already a specific
> > API for setting the bus width, this change seems unnecessary to me. That is
> > also why I choose to revert that patch. Did I miss something?
> 
> We do, the bus API really only works well when the bus bits are in another 
> register but the HOST_CONTROL register.

Sorry, I didn't get what you mean. Could you elaborate a bit more?

> The only reason 4BIT works is because its bit placement is where 
> SDHCI expects it to be. 8BIT is not, so unless readb/writeb funktions compensate
> for that they will overwrite what the bus API set earlier.

But the implementation of esdhc_pltfm_set_bus_width() already take care about
this. And in the current kernel, we only set the bus width via this API. So why
do you think that it can be overwrote?

static void esdhc_pltfm_set_bus_width(struct sdhci_host *host, int width)
{
	u32 ctrl;

	switch (width) {
	case MMC_BUS_WIDTH_8:
		ctrl = ESDHC_CTRL_8BITBUS;
		break;

	case MMC_BUS_WIDTH_4:
		ctrl = ESDHC_CTRL_4BITBUS;
		break;

	default:
		ctrl = 0;
		break;
	}

	clrsetbits_be32(host->ioaddr + SDHCI_HOST_CONTROL,
			ESDHC_CTRL_BUSWIDTH_MASK, ctrl);
}

> 
> Atleast this is my understanding, Ulf?
> 
> Didn't this patch work for you either?

I don't have a 8bit card at hand. For 4bit card, it works with or without this
change.

Thanks,
Kevin

> 
>  Jocke

[-- Attachment #2: Type: application/pgp-signature, Size: 473 bytes --]

Re: [PATCH] Revert "sdhci-of-esdhc: Support 8BIT bus width."

[Joakim Tjernlund]

On Tue, 2015-05-19 at 17:20 +0800, Kevin Hao wrote:
> On Sun, May 17, 2015 at 08:36:07AM +0000, Joakim Tjernlund wrote:
> > On Sun, 2015-05-17 at 13:06 +0800, Kevin Hao wrote:
> > > > 
> > > > How about this one:
> > > > 
> > > > From af6b18c056b6064424bd2ab1f9989bbadae5e701 Mon Sep 17 00:00:00 2001
> > > > From: Joakim Tjernlund <joakim.tjernlund@transmode.se>
> > > > Date: Mon, 20 Apr 2015 22:36:55 +0200
> > > > Subject: [PATCHv3] sdhci-of-esdhc: Support 8BIT bus width.
> > > > 
> > > > esdhc_readb()/esdhc_writeb() did not adjust for 8BIT.
> > > 
> > > Do we really need this for the 8bit bus support? There is already a specific
> > > API for setting the bus width, this change seems unnecessary to me. That is
> > > also why I choose to revert that patch. Did I miss something?
> > 
> > We do, the bus API really only works well when the bus bits are in another 
> > register but the HOST_CONTROL register.
> 
> Sorry, I didn't get what you mean. Could you elaborate a bit more?
  clrsetbits_be32(host->ioaddr + SDHCI_HOST_CONTROL,
                        ESDHC_CTRL_BUSWIDTH_MASK, ctrl);
and 
  esdhc_writeb(struct sdhci_host *host, int reg) with reg ==  SDHCI_HOST_CONTROL
is the same register so esdhc_writeb() will stomp on the settings esdhc_pltfm_set_bus_width()
did earlier.

> 
> > The only reason 4BIT works is because its bit placement is where 
> > SDHCI expects it to be. 8BIT is not, so unless readb/writeb funktions compensate
> > for that they will overwrite what the bus API set earlier.
> 
> But the implementation of esdhc_pltfm_set_bus_width() already take care about
> this. And in the current kernel, we only set the bus width via this API. So why
> do you think that it can be overwrote?
> 
> static void esdhc_pltfm_set_bus_width(struct sdhci_host *host, int width)
> {
>       u32 ctrl;
> 
>       switch (width) {
>       case MMC_BUS_WIDTH_8:
>               ctrl = ESDHC_CTRL_8BITBUS;
>               break;
> 
>       case MMC_BUS_WIDTH_4:
>               ctrl = ESDHC_CTRL_4BITBUS;
>               break;
> 
>       default:
>               ctrl = 0;
>               break;
>       }
> 
>       clrsetbits_be32(host->ioaddr + SDHCI_HOST_CONTROL,
>                       ESDHC_CTRL_BUSWIDTH_MASK, ctrl);
> }
> 
> > 
> > Atleast this is my understanding, Ulf?
> > 
> > Didn't this patch work for you either?
> 
> I don't have a 8bit card at hand. For 4bit card, it works with or without this
> change.

Sure it does, that was not the question though:
Does it still work with this version of the patch?

 Jocke

Re: [PATCH] Revert "sdhci-of-esdhc: Support 8BIT bus width."

[Kevin Hao]

[-- Attachment #1: Type: text/plain, Size: 3236 bytes --]

On Wed, May 20, 2015 at 02:54:27PM +0000, Joakim Tjernlund wrote:
> On Tue, 2015-05-19 at 17:20 +0800, Kevin Hao wrote:
> > On Sun, May 17, 2015 at 08:36:07AM +0000, Joakim Tjernlund wrote:
> > > On Sun, 2015-05-17 at 13:06 +0800, Kevin Hao wrote:
> > > > > 
> > > > > How about this one:
> > > > > 
> > > > > From af6b18c056b6064424bd2ab1f9989bbadae5e701 Mon Sep 17 00:00:00 2001
> > > > > From: Joakim Tjernlund <joakim.tjernlund@transmode.se>
> > > > > Date: Mon, 20 Apr 2015 22:36:55 +0200
> > > > > Subject: [PATCHv3] sdhci-of-esdhc: Support 8BIT bus width.
> > > > > 
> > > > > esdhc_readb()/esdhc_writeb() did not adjust for 8BIT.
> > > > 
> > > > Do we really need this for the 8bit bus support? There is already a specific
> > > > API for setting the bus width, this change seems unnecessary to me. That is
> > > > also why I choose to revert that patch. Did I miss something?
> > > 
> > > We do, the bus API really only works well when the bus bits are in another 
> > > register but the HOST_CONTROL register.
> > 
> > Sorry, I didn't get what you mean. Could you elaborate a bit more?
>   clrsetbits_be32(host->ioaddr + SDHCI_HOST_CONTROL,
>                         ESDHC_CTRL_BUSWIDTH_MASK, ctrl);
> and 
>   esdhc_writeb(struct sdhci_host *host, int reg) with reg ==  SDHCI_HOST_CONTROL
> is the same register so esdhc_writeb() will stomp on the settings esdhc_pltfm_set_bus_width()
> did earlier.

The .set_bus_width() is the only API we use to set the bus width. We never do
something like this:
	esdhc_writeb(host, SDHCI_CTRL_8BITBUS, SDHCI_HOST_CONTROL);

So the scenario you mentioned above should not happen. That is also why I think
your changes is not necessary.

> 
> > 
> > > The only reason 4BIT works is because its bit placement is where 
> > > SDHCI expects it to be. 8BIT is not, so unless readb/writeb funktions compensate
> > > for that they will overwrite what the bus API set earlier.
> > 
> > But the implementation of esdhc_pltfm_set_bus_width() already take care about
> > this. And in the current kernel, we only set the bus width via this API. So why
> > do you think that it can be overwrote?
> > 
> > static void esdhc_pltfm_set_bus_width(struct sdhci_host *host, int width)
> > {
> >       u32 ctrl;
> > 
> >       switch (width) {
> >       case MMC_BUS_WIDTH_8:
> >               ctrl = ESDHC_CTRL_8BITBUS;
> >               break;
> > 
> >       case MMC_BUS_WIDTH_4:
> >               ctrl = ESDHC_CTRL_4BITBUS;
> >               break;
> > 
> >       default:
> >               ctrl = 0;
> >               break;
> >       }
> > 
> >       clrsetbits_be32(host->ioaddr + SDHCI_HOST_CONTROL,
> >                       ESDHC_CTRL_BUSWIDTH_MASK, ctrl);
> > }
> > 
> > > 
> > > Atleast this is my understanding, Ulf?
> > > 
> > > Didn't this patch work for you either?
> > 
> > I don't have a 8bit card at hand. For 4bit card, it works with or without this
> > change.
> 
> Sure it does, that was not the question though:
> Does it still work with this version of the patch?

I think I already answered your question. For 4bit card, it works with or
without this version of patch.

Thanks,
Kevin
> 
>  Jocke

[-- Attachment #2: Type: application/pgp-signature, Size: 473 bytes --]

Re: [PATCH] Revert "sdhci-of-esdhc: Support 8BIT bus width."

[Joakim Tjernlund]

On Thu, 2015-05-21 at 09:07 +0800, Kevin Hao wrote:
> On Wed, May 20, 2015 at 02:54:27PM +0000, Joakim Tjernlund wrote:
> > On Tue, 2015-05-19 at 17:20 +0800, Kevin Hao wrote:
> > > On Sun, May 17, 2015 at 08:36:07AM +0000, Joakim Tjernlund wrote:
> > > > On Sun, 2015-05-17 at 13:06 +0800, Kevin Hao wrote:
> > > > > > 
> > > > > > How about this one:
> > > > > > 
> > > > > > From af6b18c056b6064424bd2ab1f9989bbadae5e701 Mon Sep 17 00:00:00 2001
> > > > > > From: Joakim Tjernlund <joakim.tjernlund@transmode.se>
> > > > > > Date: Mon, 20 Apr 2015 22:36:55 +0200
> > > > > > Subject: [PATCHv3] sdhci-of-esdhc: Support 8BIT bus width.
> > > > > > 
> > > > > > esdhc_readb()/esdhc_writeb() did not adjust for 8BIT.
> > > > > 
> > > > > Do we really need this for the 8bit bus support? There is already a specific
> > > > > API for setting the bus width, this change seems unnecessary to me. That is
> > > > > also why I choose to revert that patch. Did I miss something?
> > > > 
> > > > We do, the bus API really only works well when the bus bits are in another 
> > > > register but the HOST_CONTROL register.
> > > 
> > > Sorry, I didn't get what you mean. Could you elaborate a bit more?
> >   clrsetbits_be32(host->ioaddr + SDHCI_HOST_CONTROL,
> >                         ESDHC_CTRL_BUSWIDTH_MASK, ctrl);
> > and 
> >   esdhc_writeb(struct sdhci_host *host, int reg) with reg ==  SDHCI_HOST_CONTROL
> > is the same register so esdhc_writeb() will stomp on the settings esdhc_pltfm_set_bus_width()
> > did earlier.
> 
> The .set_bus_width() is the only API we use to set the bus width. We never do
> something like this:
>       esdhc_writeb(host, SDHCI_CTRL_8BITBUS, SDHCI_HOST_CONTROL);

ATM, you might at some point.

> 
> So the scenario you mentioned above should not happen. That is also why I think
> your changes is not necessary.

The HW 8BIT can be confused with SDHCI_CTRL_HISPD(0x04) as it is the same bit.

However, now I see that  esdhc_writeb() has
  /* Prevent SDHCI core from writing reserved bits (e.g. HISPD). */
        if (reg == SDHCI_HOST_CONTROL)
                val &= ~ESDHC_HOST_CONTROL_RES;
and
   #define ESDHC_HOST_CONTROL_RES       0x05

so any esdhc_writeb() to SDHCI_HOST_CONTROL will clear the HW 8BIT.
What a mess.

> 
> > 
> > > 
> > > > The only reason 4BIT works is because its bit placement is where 
> > > > SDHCI expects it to be. 8BIT is not, so unless readb/writeb funktions compensate
> > > > for that they will overwrite what the bus API set earlier.
> > > 
> > > But the implementation of esdhc_pltfm_set_bus_width() already take care about
> > > this. And in the current kernel, we only set the bus width via this API. So why
> > > do you think that it can be overwrote?
> > > 
> > > static void esdhc_pltfm_set_bus_width(struct sdhci_host *host, int width)
> > > {
> > >       u32 ctrl;
> > > 
> > >       switch (width) {
> > >       case MMC_BUS_WIDTH_8:
> > >               ctrl = ESDHC_CTRL_8BITBUS;
> > >               break;
> > > 
> > >       case MMC_BUS_WIDTH_4:
> > >               ctrl = ESDHC_CTRL_4BITBUS;
> > >               break;
> > > 
> > >       default:
> > >               ctrl = 0;
> > >               break;
> > >       }
> > > 
> > >       clrsetbits_be32(host->ioaddr + SDHCI_HOST_CONTROL,
> > >                       ESDHC_CTRL_BUSWIDTH_MASK, ctrl);
> > > }
> > > 
> > > > 
> > > > Atleast this is my understanding, Ulf?
> > > > 
> > > > Didn't this patch work for you either?
> > > 
> > > I don't have a 8bit card at hand. For 4bit card, it works with or without this
> > > change.
> > 
> > Sure it does, that was not the question though:
> > Does it still work with this version of the patch?
> 
> I think I already answered your question. For 4bit card, it works with or
> without this version of patch.

Ahh, I misread. Sorry for the confusion.

 Jocke

Re: [PATCH] Revert "sdhci-of-esdhc: Support 8BIT bus width."

[Kevin Hao]

[-- Attachment #1: Type: text/plain, Size: 571 bytes --]

On Thu, May 21, 2015 at 09:24:43AM +0000, Joakim Tjernlund wrote:
> The HW 8BIT can be confused with SDHCI_CTRL_HISPD(0x04) as it is the same bit.
> 
> However, now I see that  esdhc_writeb() has
>   /* Prevent SDHCI core from writing reserved bits (e.g. HISPD). */
>         if (reg == SDHCI_HOST_CONTROL)
>                 val &= ~ESDHC_HOST_CONTROL_RES;
> and
>    #define ESDHC_HOST_CONTROL_RES       0x05
> 
> so any esdhc_writeb() to SDHCI_HOST_CONTROL will clear the HW 8BIT.
> What a mess.

Aha, I see. Thanks for the explanation.

Thanks,
Kevin

[-- Attachment #2: Type: application/pgp-signature, Size: 473 bytes --]

Re: [PATCH] Revert "sdhci-of-esdhc: Support 8BIT bus width."

[Joakim Tjernlund]

On Thu, 2015-05-21 at 18:56 +0800, Kevin Hao wrote:
> On Thu, May 21, 2015 at 09:24:43AM +0000, Joakim Tjernlund wrote:
> > The HW 8BIT can be confused with SDHCI_CTRL_HISPD(0x04) as it is the same bit.
> > 
> > However, now I see that  esdhc_writeb() has
> >   /* Prevent SDHCI core from writing reserved bits (e.g. HISPD). */
> >         if (reg == SDHCI_HOST_CONTROL)
> >                 val &= ~ESDHC_HOST_CONTROL_RES;
> > and
> >    #define ESDHC_HOST_CONTROL_RES       0x05
> > 
> > so any esdhc_writeb() to SDHCI_HOST_CONTROL will clear the HW 8BIT.
> > What a mess.
> 
> Aha, I see. Thanks for the explanation.

There is also the common patten of
 val = esdhc_readb(..)
 val |= some bit
 esdhc_writeb(val)
This will also stomp on 8BIT

I cannot find a Maintainer, is there one?
This driver really needs a Freescale maintainer who can navigate
the subtile differences between Freescale SOCs

 Jocke

Re: [PATCH] Revert "sdhci-of-esdhc: Support 8BIT bus width."

[Ulf Hansson]

On 21 May 2015 at 13:45, Joakim Tjernlund <joakim.tjernlund@transmode.se> wrote:
> On Thu, 2015-05-21 at 18:56 +0800, Kevin Hao wrote:
>> On Thu, May 21, 2015 at 09:24:43AM +0000, Joakim Tjernlund wrote:
>> > The HW 8BIT can be confused with SDHCI_CTRL_HISPD(0x04) as it is the same bit.
>> >
>> > However, now I see that  esdhc_writeb() has
>> >   /* Prevent SDHCI core from writing reserved bits (e.g. HISPD). */
>> >         if (reg == SDHCI_HOST_CONTROL)
>> >                 val &= ~ESDHC_HOST_CONTROL_RES;
>> > and
>> >    #define ESDHC_HOST_CONTROL_RES       0x05
>> >
>> > so any esdhc_writeb() to SDHCI_HOST_CONTROL will clear the HW 8BIT.
>> > What a mess.
>>
>> Aha, I see. Thanks for the explanation.
>
> There is also the common patten of
>  val = esdhc_readb(..)
>  val |= some bit
>  esdhc_writeb(val)
> This will also stomp on 8BIT
>
> I cannot find a Maintainer, is there one?

Nope.

> This driver really needs a Freescale maintainer who can navigate
> the subtile differences between Freescale SOCs

I certainly would welcome that!

Kind regards
Uffe