diff mbox

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

Message ID 1431674695.13197.14.camel@transmode.se (mailing list archive)
State New, archived
Headers show

Commit Message

Joakim Tjernlund May 15, 2015, 7:24 a.m. UTC 
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

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

 Jocke--
To unsubscribe from this list: send the line "unsubscribe linux-mmc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Comments

Kevin Hao May 15, 2015, 7:42 a.m. UTC | #1 
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
Joakim Tjernlund May 15, 2015, 8 a.m. UTC | #2 
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--
To unsubscribe from this list: send the line "unsubscribe linux-mmc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
diff mbox

Patch

--- 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,
 };