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@xxxxxxxxxxxx>
> > > > > > 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
--
To unsubscribe from this list: send the line "unsubscribe linux-mmc" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
