* Re: [PATCH] Revert "sdhci-of-esdhc: Support 8BIT bus width."
2015-05-15 7:42 ` Kevin Hao
@ 2015-05-15 8:00 ` Joakim Tjernlund
2015-05-15 8:20 ` Joakim Tjernlund
2015-05-15 12:40 ` Joakim Tjernlund
2 siblings, 0 replies; 20+ messages in thread
From: Joakim Tjernlund @ 2015-05-15 8:00 UTC (permalink / raw)
To: haokexin@gmail.com; +Cc: ulf.hansson@linaro.org, linux-mmc@vger.kernel.org
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
^ permalink raw reply [flat|nested] 20+ messages in thread* Re: [PATCH] Revert "sdhci-of-esdhc: Support 8BIT bus width."
2015-05-15 7:42 ` Kevin Hao
2015-05-15 8:00 ` Joakim Tjernlund
@ 2015-05-15 8:20 ` Joakim Tjernlund
2015-05-15 12:40 ` Joakim Tjernlund
2 siblings, 0 replies; 20+ messages in thread
From: Joakim Tjernlund @ 2015-05-15 8:20 UTC (permalink / raw)
To: haokexin@gmail.com; +Cc: ulf.hansson@linaro.org, linux-mmc@vger.kernel.org
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,
};
^ permalink raw reply related [flat|nested] 20+ messages in thread* Re: [PATCH] Revert "sdhci-of-esdhc: Support 8BIT bus width."
2015-05-15 7:42 ` Kevin Hao
2015-05-15 8:00 ` Joakim Tjernlund
2015-05-15 8:20 ` Joakim Tjernlund
@ 2015-05-15 12:40 ` Joakim Tjernlund
2015-05-17 5:06 ` Kevin Hao
2 siblings, 1 reply; 20+ messages in thread
From: Joakim Tjernlund @ 2015-05-15 12:40 UTC (permalink / raw)
To: haokexin@gmail.com; +Cc: ulf.hansson@linaro.org, linux-mmc@vger.kernel.org
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
^ permalink raw reply related [flat|nested] 20+ messages in thread* Re: [PATCH] Revert "sdhci-of-esdhc: Support 8BIT bus width."
2015-05-15 12:40 ` Joakim Tjernlund
@ 2015-05-17 5:06 ` Kevin Hao
2015-05-17 8:36 ` Joakim Tjernlund
0 siblings, 1 reply; 20+ messages in thread
From: Kevin Hao @ 2015-05-17 5:06 UTC (permalink / raw)
To: Joakim Tjernlund; +Cc: ulf.hansson@linaro.org, linux-mmc@vger.kernel.org
[-- 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 --]
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH] Revert "sdhci-of-esdhc: Support 8BIT bus width."
2015-05-17 5:06 ` Kevin Hao
@ 2015-05-17 8:36 ` Joakim Tjernlund
2015-05-18 7:58 ` Ulf Hansson
2015-05-19 9:20 ` Kevin Hao
0 siblings, 2 replies; 20+ messages in thread
From: Joakim Tjernlund @ 2015-05-17 8:36 UTC (permalink / raw)
To: haokexin@gmail.com; +Cc: ulf.hansson@linaro.org, linux-mmc@vger.kernel.org
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
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH] Revert "sdhci-of-esdhc: Support 8BIT bus width."
2015-05-17 8:36 ` Joakim Tjernlund
@ 2015-05-18 7:58 ` Ulf Hansson
2015-05-19 9:20 ` Kevin Hao
1 sibling, 0 replies; 20+ messages in thread
From: Ulf Hansson @ 2015-05-18 7:58 UTC (permalink / raw)
To: Joakim Tjernlund, haokexin@gmail.com; +Cc: linux-mmc@vger.kernel.org
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
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH] Revert "sdhci-of-esdhc: Support 8BIT bus width."
2015-05-17 8:36 ` Joakim Tjernlund
2015-05-18 7:58 ` Ulf Hansson
@ 2015-05-19 9:20 ` Kevin Hao
2015-05-20 14:54 ` Joakim Tjernlund
1 sibling, 1 reply; 20+ messages in thread
From: Kevin Hao @ 2015-05-19 9:20 UTC (permalink / raw)
To: Joakim Tjernlund; +Cc: ulf.hansson@linaro.org, linux-mmc@vger.kernel.org
[-- 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 --]
^ permalink raw reply [flat|nested] 20+ messages in thread* Re: [PATCH] Revert "sdhci-of-esdhc: Support 8BIT bus width."
2015-05-19 9:20 ` Kevin Hao
@ 2015-05-20 14:54 ` Joakim Tjernlund
2015-05-21 1:07 ` Kevin Hao
0 siblings, 1 reply; 20+ messages in thread
From: Joakim Tjernlund @ 2015-05-20 14:54 UTC (permalink / raw)
To: haokexin@gmail.com; +Cc: ulf.hansson@linaro.org, linux-mmc@vger.kernel.org
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
^ permalink raw reply [flat|nested] 20+ messages in thread* Re: [PATCH] Revert "sdhci-of-esdhc: Support 8BIT bus width."
2015-05-20 14:54 ` Joakim Tjernlund
@ 2015-05-21 1:07 ` Kevin Hao
2015-05-21 9:24 ` Joakim Tjernlund
0 siblings, 1 reply; 20+ messages in thread
From: Kevin Hao @ 2015-05-21 1:07 UTC (permalink / raw)
To: Joakim Tjernlund; +Cc: ulf.hansson@linaro.org, linux-mmc@vger.kernel.org
[-- 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 --]
^ permalink raw reply [flat|nested] 20+ messages in thread* Re: [PATCH] Revert "sdhci-of-esdhc: Support 8BIT bus width."
2015-05-21 1:07 ` Kevin Hao
@ 2015-05-21 9:24 ` Joakim Tjernlund
2015-05-21 10:56 ` Kevin Hao
0 siblings, 1 reply; 20+ messages in thread
From: Joakim Tjernlund @ 2015-05-21 9:24 UTC (permalink / raw)
To: haokexin@gmail.com; +Cc: ulf.hansson@linaro.org, linux-mmc@vger.kernel.org
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
^ permalink raw reply [flat|nested] 20+ messages in thread* Re: [PATCH] Revert "sdhci-of-esdhc: Support 8BIT bus width."
2015-05-21 9:24 ` Joakim Tjernlund
@ 2015-05-21 10:56 ` Kevin Hao
2015-05-21 11:45 ` Joakim Tjernlund
0 siblings, 1 reply; 20+ messages in thread
From: Kevin Hao @ 2015-05-21 10:56 UTC (permalink / raw)
To: Joakim Tjernlund; +Cc: ulf.hansson@linaro.org, linux-mmc@vger.kernel.org
[-- 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 --]
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH] Revert "sdhci-of-esdhc: Support 8BIT bus width."
2015-05-21 10:56 ` Kevin Hao
@ 2015-05-21 11:45 ` Joakim Tjernlund
2015-05-22 13:46 ` Ulf Hansson
0 siblings, 1 reply; 20+ messages in thread
From: Joakim Tjernlund @ 2015-05-21 11:45 UTC (permalink / raw)
To: haokexin@gmail.com; +Cc: ulf.hansson@linaro.org, linux-mmc@vger.kernel.org
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
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH] Revert "sdhci-of-esdhc: Support 8BIT bus width."
2015-05-21 11:45 ` Joakim Tjernlund
@ 2015-05-22 13:46 ` Ulf Hansson
0 siblings, 0 replies; 20+ messages in thread
From: Ulf Hansson @ 2015-05-22 13:46 UTC (permalink / raw)
To: Joakim Tjernlund; +Cc: haokexin@gmail.com, linux-mmc@vger.kernel.org
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
^ permalink raw reply [flat|nested] 20+ messages in thread