Patchwork eSDHC: Access Freescale eSDHC registers by 32-bit

login
register
mail settings
Submitter Zang Roy-R61911
Date Sept. 9, 2011, 12:05 p.m.
Message ID <1315569946-21386-1-git-send-email-tie-fei.zang@freescale.com>
Download mbox | patch
Permalink /patch/114084/
State Not Applicable
Headers show

Comments

Wolfram Sang - Sept. 9, 2011, 11:40 a.m.
Hi,

On Fri, Sep 09, 2011 at 08:05:46PM +0800, Roy Zang wrote:
> From: Xu lei <B33228@freescale.com>
> 
> Freescale eSDHC registers only support 32-bit accesses,
> this patch ensures that all Freescale eSDHC register accesses
> are 32-bit.

So, what about the byte-swapping that Anton needed? You are simply
removing that if I am not mistaken.

> Signed-off-by: Xu lei <B33228@freescale.com>
> Signed-off-by: Roy Zang <tie-fei.zang@freescale.com>
> Signed-off-by: Kumar Gala <galak@kernel.crashing.org>
> ---
> This is a patch resend 
> http://patchwork.ozlabs.org/patch/106245/
> based on latest Linus tree.
> 
> Andrew,
> Could you help to  pick up this patch first?

mmc-list is not dead, it must go via Chris, I'd think.

>  drivers/mmc/host/sdhci-of-esdhc.c |   18 ++++++++++++++----
>  1 files changed, 14 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/mmc/host/sdhci-of-esdhc.c b/drivers/mmc/host/sdhci-of-esdhc.c
> index fe604df..40036f6 100644
> --- a/drivers/mmc/host/sdhci-of-esdhc.c
> +++ b/drivers/mmc/host/sdhci-of-esdhc.c
> @@ -1,7 +1,7 @@
>  /*
>   * Freescale eSDHC controller driver.
>   *
> - * Copyright (c) 2007 Freescale Semiconductor, Inc.
> + * Copyright (c) 2007, 2010 Freescale Semiconductor, Inc.

I wonder if such a small change has impact on the copyright.

Regards,

   Wolfram
Zang Roy-R61911 - Sept. 9, 2011, 12:05 p.m.
From: Xu lei <B33228@freescale.com>

Freescale eSDHC registers only support 32-bit accesses,
this patch ensures that all Freescale eSDHC register accesses
are 32-bit.

Signed-off-by: Xu lei <B33228@freescale.com>
Signed-off-by: Roy Zang <tie-fei.zang@freescale.com>
Signed-off-by: Kumar Gala <galak@kernel.crashing.org>
---
This is a patch resend 
http://patchwork.ozlabs.org/patch/106245/
based on latest Linus tree.

Andrew,
Could you help to  pick up this patch first?
Thanks.
Roy

 drivers/mmc/host/sdhci-of-esdhc.c |   18 ++++++++++++++----
 1 files changed, 14 insertions(+), 4 deletions(-)
Zang Roy-R61911 - Sept. 9, 2011, 12:16 p.m.
> -----Original Message-----
> From: Wolfram Sang [mailto:w.sang@pengutronix.de]
> Sent: Friday, September 09, 2011 19:40 PM
> To: Zang Roy-R61911
> Cc: linux-mmc@vger.kernel.org; linuxppc-dev@lists.ozlabs.org; akpm@linux-
> foundation.org; cbouatmailru@gmail.com; Xu Lei-B33228; Kumar Gala
> Subject: Re: [PATCH] eSDHC: Access Freescale eSDHC registers by 32-bit
> 
> Hi,
> 
> On Fri, Sep 09, 2011 at 08:05:46PM +0800, Roy Zang wrote:
> > From: Xu lei <B33228@freescale.com>
> >
> > Freescale eSDHC registers only support 32-bit accesses,
> > this patch ensures that all Freescale eSDHC register accesses
> > are 32-bit.
> 
> So, what about the byte-swapping that Anton needed? You are simply
> removing that if I am not mistaken.
> 
> > Signed-off-by: Xu lei <B33228@freescale.com>
> > Signed-off-by: Roy Zang <tie-fei.zang@freescale.com>
> > Signed-off-by: Kumar Gala <galak@kernel.crashing.org>
> > ---
> > This is a patch resend
> > http://patchwork.ozlabs.org/patch/106245/
> > based on latest Linus tree.
> >
> > Andrew,
> > Could you help to  pick up this patch first?
> 
> mmc-list is not dead, it must go via Chris, I'd think.
This is the third time that I re-send this patch to the list.  There is no comment.
That is why I ask Andrew's help.
> 
> >  drivers/mmc/host/sdhci-of-esdhc.c |   18 ++++++++++++++----
> >  1 files changed, 14 insertions(+), 4 deletions(-)
> >
> > diff --git a/drivers/mmc/host/sdhci-of-esdhc.c b/drivers/mmc/host/sdhci-of-
> esdhc.c
> > index fe604df..40036f6 100644
> > --- a/drivers/mmc/host/sdhci-of-esdhc.c
> > +++ b/drivers/mmc/host/sdhci-of-esdhc.c
> > @@ -1,7 +1,7 @@
> >  /*
> >   * Freescale eSDHC controller driver.
> >   *
> > - * Copyright (c) 2007 Freescale Semiconductor, Inc.
> > + * Copyright (c) 2007, 2010 Freescale Semiconductor, Inc.
> 
> I wonder if such a small change has impact on the copyright.
I just update the year for tracking. Anything wrong?
Roy
Zang Roy-R61911 - Sept. 9, 2011, 12:23 p.m.
> -----Original Message-----
> From: Wolfram Sang [mailto:w.sang@pengutronix.de]
> Sent: Friday, September 09, 2011 19:40 PM
> To: Zang Roy-R61911
> Cc: linux-mmc@vger.kernel.org; linuxppc-dev@lists.ozlabs.org; akpm@linux-
> foundation.org; cbouatmailru@gmail.com; Xu Lei-B33228; Kumar Gala
> Subject: Re: [PATCH] eSDHC: Access Freescale eSDHC registers by 32-bit
> 
> Hi,
> 
> On Fri, Sep 09, 2011 at 08:05:46PM +0800, Roy Zang wrote:
> > From: Xu lei <B33228@freescale.com>
> >
> > Freescale eSDHC registers only support 32-bit accesses,
> > this patch ensures that all Freescale eSDHC register accesses
> > are 32-bit.
> 
> So, what about the byte-swapping that Anton needed? You are simply
> removing that if I am not mistaken.
I do not see any comment for this patch from Anton. You may miss the patch. Please confirm.
I add Chris in the loop.
Thanks.
Roy
Wolfram Sang - Sept. 9, 2011, 12:33 p.m.
On Fri, Sep 09, 2011 at 12:23:05PM +0000, Zang Roy-R61911 wrote:

> > > Freescale eSDHC registers only support 32-bit accesses,
> > > this patch ensures that all Freescale eSDHC register accesses
> > > are 32-bit.
> > 
> > So, what about the byte-swapping that Anton needed? You are simply
> > removing that if I am not mistaken.
> I do not see any comment for this patch from Anton.

Maybe he is busy?

> You may miss the patch. Please confirm.

I don't understand, what needs to be confirmed? I am simply wondering
what happens to the byteswapping that Anton (back then) intentionally
implemented.

Regards,

   Wolfram
Zang Roy-R61911 - Sept. 9, 2011, 12:48 p.m.
> -----Original Message-----
> From: Wolfram Sang [mailto:w.sang@pengutronix.de]
> Sent: Friday, September 09, 2011 20:33 PM
> To: Zang Roy-R61911
> Cc: cjb@laptop.org; linux-mmc@vger.kernel.org; linuxppc-dev@lists.ozlabs.org;
> akpm@linux-foundation.org; cbouatmailru@gmail.com; Xu Lei-B33228; Kumar Gala
> Subject: Re: [PATCH] eSDHC: Access Freescale eSDHC registers by 32-bit
> 
> On Fri, Sep 09, 2011 at 12:23:05PM +0000, Zang Roy-R61911 wrote:
> 
> > > > Freescale eSDHC registers only support 32-bit accesses,
> > > > this patch ensures that all Freescale eSDHC register accesses
> > > > are 32-bit.
> > >
> > > So, what about the byte-swapping that Anton needed? You are simply
> > > removing that if I am not mistaken.
> > I do not see any comment for this patch from Anton.
> 
> Maybe he is busy?
Sounds fair.
> 
> > You may miss the patch. Please confirm.
> 
> I don't understand, what needs to be confirmed?
Confirm: there is no comment about this patch. 
Previously, I sent this patch together  with another one. But technically they are separated patches.  I got some comments from Anton about the patch. I will try to address the comment. but for this one, it is a stand along patch. I'd like to push it first.

> I am simply wondering
> what happens to the byteswapping that Anton (back then) intentionally
> implemented.
I do not see any comment about byte swapping about this patch.
you may have mis-understanding here.
Roy
Wolfram Sang - Sept. 9, 2011, 1:08 p.m.
> Previously, I sent this patch together  with another one. But
> technically they are separated patches.  I got some comments from
> Anton about the patch. I will try to address the comment. but for this
> one, it is a stand along patch. I'd like to push it first.

Okay.

> I do not see any comment about byte swapping about this patch.
> you may have mis-understanding here.

I was misguided, yes, sorry. BE/LE issues with additional byteswapping
are a mind twister :/ Having a second look, it looks okay to me. One
could think about putting this into sdhci_platform as well, but this can
be done later if needed. Sadly, I currently don't have hardware to test
it.

Regards,

   Wolfram
Anton Vorontsov - Sept. 9, 2011, 2:07 p.m.
On Fri, Sep 09, 2011 at 08:05:46PM +0800, Roy Zang wrote:
> From: Xu lei <B33228@freescale.com>
> 
> Freescale eSDHC registers only support 32-bit accesses,
> this patch ensures that all Freescale eSDHC register accesses
> are 32-bit.
> 
> Signed-off-by: Xu lei <B33228@freescale.com>
> Signed-off-by: Roy Zang <tie-fei.zang@freescale.com>
> Signed-off-by: Kumar Gala <galak@kernel.crashing.org>
> ---

The patch looks OK.

Acked-by: Anton Vorontsov <cbouatmailru@gmail.com>

[...]
> +static u8 esdhc_readb(struct sdhci_host *host, int reg)
> +{
> +	int base = reg & ~0x3;
> +	int shift = (reg & 0x3) * 8;
> +	u8 ret = (in_be32(host->ioaddr + base) >> shift) & 0xff;
>  	return ret;
>  }

Though, I wonder if we could change sdhci_be32bs_read{b,w}, instead
of making this local to eSDHC.

The thing is: sdhci_be32bs_writeb() is using clrsetbits_be32,
so the write variant already uses 32-bit accessors, so nothing should
break if we switch sdhci_be32bs_readb() to in_be32().

But maybe it's safer if we do this in a separate patch, so that it
could be easily reverted without impacting eSDHC if something actually
breaks.

You decide. :-)

Thanks!
Zang Roy-R61911 - Sept. 9, 2011, 4 p.m.
> -----Original Message-----
> From: Anton Vorontsov [mailto:cbouatmailru@gmail.com]
> Sent: Friday, September 09, 2011 22:07 PM
> To: Zang Roy-R61911
> Cc: linux-mmc@vger.kernel.org; linuxppc-dev@lists.ozlabs.org; akpm@linux-
> foundation.org; Xu Lei-B33228; Kumar Gala
> Subject: Re: [PATCH] eSDHC: Access Freescale eSDHC registers by 32-bit
> 
> On Fri, Sep 09, 2011 at 08:05:46PM +0800, Roy Zang wrote:
> > From: Xu lei <B33228@freescale.com>
> >
> > Freescale eSDHC registers only support 32-bit accesses,
> > this patch ensures that all Freescale eSDHC register accesses
> > are 32-bit.
> >
> > Signed-off-by: Xu lei <B33228@freescale.com>
> > Signed-off-by: Roy Zang <tie-fei.zang@freescale.com>
> > Signed-off-by: Kumar Gala <galak@kernel.crashing.org>
> > ---
> 
> The patch looks OK.
> 
> Acked-by: Anton Vorontsov <cbouatmailru@gmail.com>
Thanks.
Add Chris.

> 
> [...]
> > +static u8 esdhc_readb(struct sdhci_host *host, int reg)
> > +{
> > +	int base = reg & ~0x3;
> > +	int shift = (reg & 0x3) * 8;
> > +	u8 ret = (in_be32(host->ioaddr + base) >> shift) & 0xff;
> >  	return ret;
> >  }
> 
> Though, I wonder if we could change sdhci_be32bs_read{b,w}, instead
> of making this local to eSDHC.
> 
> The thing is: sdhci_be32bs_writeb() is using clrsetbits_be32,
> so the write variant already uses 32-bit accessors, so nothing should
> break if we switch sdhci_be32bs_readb() to in_be32().
> 
> But maybe it's safer if we do this in a separate patch, so that it
> could be easily reverted without impacting eSDHC if something actually
> breaks.
> 
> You decide. :-)
> 
> Thanks!
The suggestion makes sense. I'd like to do this in a separate patch after this patch applied.
Thanks.
Roy
Zang Roy-R61911 - Sept. 13, 2011, 4:10 a.m.
> -----Original Message-----
> From: linux-mmc-owner@vger.kernel.org [mailto:linux-mmc-owner@vger.kernel.org]
> On Behalf Of Wolfram Sang
> Sent: Friday, September 09, 2011 21:09 PM
> To: Zang Roy-R61911
> Cc: cjb@laptop.org; linux-mmc@vger.kernel.org; linuxppc-dev@lists.ozlabs.org;
> akpm@linux-foundation.org; cbouatmailru@gmail.com; Xu Lei-B33228; Kumar Gala
> Subject: Re: [PATCH] eSDHC: Access Freescale eSDHC registers by 32-bit
> 
> 
> > Previously, I sent this patch together  with another one. But
> > technically they are separated patches.  I got some comments from
> > Anton about the patch. I will try to address the comment. but for this
> > one, it is a stand along patch. I'd like to push it first.
> 
> Okay.
Good.
> 
> > I do not see any comment about byte swapping about this patch.
> > you may have mis-understanding here.
> 
> I was misguided, yes, sorry. BE/LE issues with additional byteswapping
> are a mind twister :/ Having a second look, it looks okay to me. One
> could think about putting this into sdhci_platform as well, but this can
> be done later if needed. Sadly, I currently don't have hardware to test
> it.
I tested on 8536 and P4080/P5020/P3041 platform.
Anyone can help to pull it in?
Thanks.
Roy
Chris Ball - Sept. 21, 2011, 5:53 p.m.
Hi Roy,

On Fri, Sep 09 2011, Roy Zang wrote:
> From: Xu lei <B33228@freescale.com>
>
> Freescale eSDHC registers only support 32-bit accesses,
> this patch ensures that all Freescale eSDHC register accesses
> are 32-bit.
>
> Signed-off-by: Xu lei <B33228@freescale.com>
> Signed-off-by: Roy Zang <tie-fei.zang@freescale.com>
> Signed-off-by: Kumar Gala <galak@kernel.crashing.org>

Pushed to mmc-next for 3.2 with Anton's ACK now, thanks.

- Chris.

Patch

diff --git a/drivers/mmc/host/sdhci-of-esdhc.c b/drivers/mmc/host/sdhci-of-esdhc.c
index fe604df..40036f6 100644
--- a/drivers/mmc/host/sdhci-of-esdhc.c
+++ b/drivers/mmc/host/sdhci-of-esdhc.c
@@ -1,7 +1,7 @@ 
 /*
  * Freescale eSDHC controller driver.
  *
- * Copyright (c) 2007 Freescale Semiconductor, Inc.
+ * Copyright (c) 2007, 2010 Freescale Semiconductor, Inc.
  * Copyright (c) 2009 MontaVista Software, Inc.
  *
  * Authors: Xiaobo Xie <X.Xie@freescale.com>
@@ -22,11 +22,21 @@ 
 static u16 esdhc_readw(struct sdhci_host *host, int reg)
 {
 	u16 ret;
+	int base = reg & ~0x3;
+	int shift = (reg & 0x2) * 8;
 
 	if (unlikely(reg == SDHCI_HOST_VERSION))
-		ret = in_be16(host->ioaddr + reg);
+		ret = in_be32(host->ioaddr + base) & 0xffff;
 	else
-		ret = sdhci_be32bs_readw(host, reg);
+		ret = (in_be32(host->ioaddr + base) >> shift) & 0xffff;
+	return ret;
+}
+
+static u8 esdhc_readb(struct sdhci_host *host, int reg)
+{
+	int base = reg & ~0x3;
+	int shift = (reg & 0x3) * 8;
+	u8 ret = (in_be32(host->ioaddr + base) >> shift) & 0xff;
 	return ret;
 }
 
@@ -74,7 +84,7 @@  static unsigned int esdhc_of_get_min_clock(struct sdhci_host *host)
 static struct sdhci_ops sdhci_esdhc_ops = {
 	.read_l = sdhci_be32bs_readl,
 	.read_w = esdhc_readw,
-	.read_b = sdhci_be32bs_readb,
+	.read_b = esdhc_readb,
 	.write_l = sdhci_be32bs_writel,
 	.write_w = esdhc_writew,
 	.write_b = esdhc_writeb,