Re: [PATCH 02/11] sdhci: Add support for bus-specific IO memory accessors

2009-03-08 Thread Pierre Ossman 
On Wed, 4 Mar 2009 20:46:58 +0300
Anton Vorontsov avoront...@ru.mvista.com wrote:

 On Sat, Feb 21, 2009 at 04:57:57PM +0100, Pierre Ossman wrote:
  
  We can most likely do some micro-optimisation do make the compare part
  cheaper, but the point was to avoid a function call for all the
  properly implemented controllers out there. We could have a flag so
  that it only has to check host-flags, which will most likely be in the
  cache anyway.
  
  Overhead for eSDHC is not a concern in my book, what is interesting is
  how much this change slows things down for other controllers.
 
 OK, I see. Will the patch down below make you a little bit more happy
 wrt normal controllers? Two #ifdefs, but then there is absolutely
 zero overhead for the fully compliant SDHCI controllers.
 

I can't say this makes me happy either, but I think it's acceptable for
now so that we can move forward. I'd like a common code path for this
thing, but I think I'm going to have to put a bit more time into it
myself than I currently have available.

 (So far it's just on top of this series, but I can incorporate it
 into the sdhci: Add support for bus-specific IO memory accessors
 patch, if you like).
 

Please do. Have one patch add some code and another remove it in the
same set is just silly. :)

Rgds
-- 
 -- Pierre Ossman

  WARNING: This correspondence is being monitored by the
  Swedish government. Make sure your server uses encryption
  for SMTP traffic and consider using PGP for end-to-end
  encryption.


signature.asc
Description: PGP signature
___
Linuxppc-dev mailing list
Linuxppc-dev@ozlabs.org
https://ozlabs.org/mailman/listinfo/linuxppc-dev

Re: [PATCH 02/11] sdhci: Add support for bus-specific IO memory accessors

2009-03-04 Thread Anton Vorontsov 
On Sat, Feb 21, 2009 at 04:57:57PM +0100, Pierre Ossman wrote:
 On Fri, 13 Feb 2009 17:40:39 +0300
 Anton Vorontsov avoront...@ru.mvista.com wrote:
 
  
  No, on eSDHC the registers are big-endian, 32-bit width, with, for
  example, two 16-bit logical registers packed into it.
  
  That is,
  
   0x4  0x5 0x6  0x7
  |:|
  | BLKCNT : BLKSZ  |
  |:|
   31  0
  
  ( The register looks wrong, right? BLKSZ should be at 0x4. But imagine
that you swapped bytes in this 32 bit register... then the registers
and their byte addresses will look normal. )
  
  So if we try to issue readw(SDHCI_BLOCK_SIZE), i.e. readw(0x4):
  
  - We'll read BLKCNT, while we wanted BLKSZ. This is because the
address bits should be translated before we try word or byte
reads/writes.
  - On powerpc read{l,w}() convert the read value from little-endian
to big-endian byte order, which is wrong for our case (the
register is big-endian already).
  
  That means that we have to convert address, but we don't want to
  convert the result of read/write ops.
  
 
 *cries*

:-)

[...]
   as far as I'm
   concerned, so I'd prefer something like:
   
   if (!host-ops-writel)
 writel(host-ioaddr + reg, val);
   else
 host-ops-writel(host, val, reg);
  
  Surely the overhead isn't measurable... but why we purposely make
  things worse?
  
 
 We can most likely do some micro-optimisation do make the compare part
 cheaper, but the point was to avoid a function call for all the
 properly implemented controllers out there. We could have a flag so
 that it only has to check host-flags, which will most likely be in the
 cache anyway.
 
 Overhead for eSDHC is not a concern in my book, what is interesting is
 how much this change slows things down for other controllers.

OK, I see. Will the patch down below make you a little bit more happy
wrt normal controllers? Two #ifdefs, but then there is absolutely
zero overhead for the fully compliant SDHCI controllers.

(So far it's just on top of this series, but I can incorporate it
into the sdhci: Add support for bus-specific IO memory accessors
patch, if you like).

diff --git a/drivers/mmc/host/Kconfig b/drivers/mmc/host/Kconfig
index 73b79e1..69bd124 100644
--- a/drivers/mmc/host/Kconfig
+++ b/drivers/mmc/host/Kconfig
@@ -37,6 +37,13 @@ config MMC_SDHCI
 
  If unsure, say N.
 
+config MMC_SDHCI_IO_ACCESSORS
+   bool
+   depends on MMC_SDHCI
+   help
+ This is silent Kconfig symbol that is selected by the drivers that
+ need to overwrite SDHCI IO memory accessors.
+
 config MMC_SDHCI_PCI
tristate SDHCI support on PCI bus
depends on MMC_SDHCI  PCI
@@ -68,6 +75,7 @@ config MMC_RICOH_MMC
 config MMC_SDHCI_OF
tristate SDHCI support on OpenFirmware platforms
depends on MMC_SDHCI  PPC_OF
+   select MMC_SDHCI_IO_ACCESSORS
help
  This selects the OF support for Secure Digital Host Controller
  Interfaces. So far, only the Freescale eSDHC controller is known
diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
index 284bc5d..fb08d3b 100644
--- a/drivers/mmc/host/sdhci.c
+++ b/drivers/mmc/host/sdhci.c
@@ -88,60 +88,6 @@ static void sdhci_dumpregs(struct sdhci_host *host)
  *   *
 \*/
 
-void sdhci_writel(struct sdhci_host *host, u32 val, int reg)
-{
-   if (unlikely(host-ops-writel))
-   host-ops-writel(host, val, reg);
-   else
-   writel(val, host-ioaddr + reg);
-}
-EXPORT_SYMBOL_GPL(sdhci_writel);
-
-void sdhci_writew(struct sdhci_host *host, u16 val, int reg)
-{
-   if (unlikely(host-ops-writew))
-   host-ops-writew(host, val, reg);
-   else
-   writew(val, host-ioaddr + reg);
-}
-EXPORT_SYMBOL_GPL(sdhci_writew);
-
-void sdhci_writeb(struct sdhci_host *host, u8 val, int reg)
-{
-   if (unlikely(host-ops-writeb))
-   host-ops-writeb(host, val, reg);
-   else
-   writeb(val, host-ioaddr + reg);
-}
-EXPORT_SYMBOL_GPL(sdhci_writeb);
-
-u32 sdhci_readl(struct sdhci_host *host, int reg)
-{
-   if (unlikely(host-ops-readl))
-   return host-ops-readl(host, reg);
-   else
-   return readl(host-ioaddr + reg);
-}
-EXPORT_SYMBOL_GPL(sdhci_readl);
-
-u16 sdhci_readw(struct sdhci_host *host, int reg)
-{
-   if (unlikely(host-ops-readw))
-   return host-ops-readw(host, reg);
-   else
-   return readw(host-ioaddr + reg);
-}
-EXPORT_SYMBOL_GPL(sdhci_readw);
-
-u8 sdhci_readb(struct sdhci_host *host, int reg)
-{
-   if (unlikely(host-ops-readb))
-   return host-ops-readb(host, reg);
-   else
-   return readb(host-ioaddr + reg);
-}
-EXPORT_SYMBOL_GPL(sdhci_readb);
-
 static void sdhci_clear_set_irqs(struct sdhci_host *host,

Re: [PATCH 02/11] sdhci: Add support for bus-specific IO memory accessors

2009-02-21 Thread Pierre Ossman 
On Fri, 13 Feb 2009 17:40:39 +0300
Anton Vorontsov avoront...@ru.mvista.com wrote:

 
 No, on eSDHC the registers are big-endian, 32-bit width, with, for
 example, two 16-bit logical registers packed into it.
 
 That is,
 
  0x4  0x5 0x6  0x7
 |:|
 | BLKCNT : BLKSZ  |
 |:|
  31  0
 
 ( The register looks wrong, right? BLKSZ should be at 0x4. But imagine
   that you swapped bytes in this 32 bit register... then the registers
   and their byte addresses will look normal. )
 
 So if we try to issue readw(SDHCI_BLOCK_SIZE), i.e. readw(0x4):
 
 - We'll read BLKCNT, while we wanted BLKSZ. This is because the
   address bits should be translated before we try word or byte
   reads/writes.
 - On powerpc read{l,w}() convert the read value from little-endian
   to big-endian byte order, which is wrong for our case (the
   register is big-endian already).
 
 That means that we have to convert address, but we don't want to
 convert the result of read/write ops.
 

*cries*

Now this is just incredibly horrible. Why the hell did they try to use
the sdhci interface and then do stupid things like this?

   +static inline void sdhci_writel(struct sdhci_host *host, u32 val, int 
   reg)
   +{
   + host-writel(host, val, reg);
   +}
  
  Having to override these are worst case scenario
 
 Hm. It's not a worst case scenario, it's a normal scenario for
 eSDHC. Why should we treat eSDHC as a second-class citizen?
 

Because it's complete and utter crap. Freescale has completely ignored
the basic register interface requirements of the SDHCI spec. Treating
eSDHC as a second-class citizen is generous IMO.

  as far as I'm
  concerned, so I'd prefer something like:
  
  if (!host-ops-writel)
  writel(host-ioaddr + reg, val);
  else
  host-ops-writel(host, val, reg);
 
 Surely the overhead isn't measurable... but why we purposely make
 things worse?
 

We can most likely do some micro-optimisation do make the compare part
cheaper, but the point was to avoid a function call for all the
properly implemented controllers out there. We could have a flag so
that it only has to check host-flags, which will most likely be in the
cache anyway.

Overhead for eSDHC is not a concern in my book, what is interesting is
how much this change slows things down for other controllers.

Rgds
-- 
 -- Pierre Ossman

  WARNING: This correspondence is being monitored by the
  Swedish government. Make sure your server uses encryption
  for SMTP traffic and consider using PGP for end-to-end
  encryption.


signature.asc
Description: PGP signature
___
Linuxppc-dev mailing list
Linuxppc-dev@ozlabs.org
https://ozlabs.org/mailman/listinfo/linuxppc-dev

Re: [PATCH 02/11] sdhci: Add support for bus-specific IO memory accessors

2009-02-13 Thread Anton Vorontsov 
On Sun, Feb 08, 2009 at 09:50:20PM +0100, Pierre Ossman wrote:
 On Fri, 6 Feb 2009 21:06:45 +0300
 Anton Vorontsov avoront...@ru.mvista.com wrote:
  Currently the SDHCI driver works with PCI accessors (write{l,b,w} and
  read{l,b,w}).
  
  With this patch drivers may change memory accessors, so that we can
  support hosts with weird IO memory access requirments.
  
  For example, in FSL eSDHC SDHCI hardware all registers are 32 bit
  width, with big-endian addressing. That is, readb(0x2f) should turn
  into readb(0x2c), and readw(0x2c) should be translated to
  le16_to_cpu(readw(0x2e)).
  
  Signed-off-by: Anton Vorontsov avoront...@ru.mvista.com
  ---
 
 I was hoping we wouldn't have to do a lot of magic in the accessors
 since the spec is rather clear on the register interface. :/
 
 Let's see if I've understood this correctly.
 
 1. The CPU is big-endian but the register are little-endian (as the
 spec requires).

No, on eSDHC the registers are big-endian, 32-bit width, with, for
example, two 16-bit logical registers packed into it.

That is,

 0x4  0x5 0x6  0x7
|:|
| BLKCNT : BLKSZ  |
|:|
 31  0

( The register looks wrong, right? BLKSZ should be at 0x4. But imagine
  that you swapped bytes in this 32 bit register... then the registers
  and their byte addresses will look normal. )

So if we try to issue readw(SDHCI_BLOCK_SIZE), i.e. readw(0x4):

- We'll read BLKCNT, while we wanted BLKSZ. This is because the
  address bits should be translated before we try word or byte
  reads/writes.
- On powerpc read{l,w}() convert the read value from little-endian
  to big-endian byte order, which is wrong for our case (the
  register is big-endian already).

That means that we have to convert address, but we don't want to
convert the result of read/write ops.

 I was under the impression that the read*/write*
 accessor handled any endian conversion between the bus and the cpu? How
 do e.g. PCI work on Sparc?

read{l,w} are guaranteed to return values in CPU byte order, so
if CPU is in big-endian mode, then the PCI IO accessors should
convert values. And just as on PowerPC, Sparc's read*() accessors
swap bytes of a result:

static inline u32 __readl(const volatile void __iomem *addr)
{
return flip_dword(*(__force volatile u32 *)addr);
}

#define readl(__addr)   __readl(__addr)

 2. Register access must be done 32 bits at a time. Now this is just
 broken and might cause big problems as some registers cannot just be
 read and written back to.

We must only take special care when working with triggering
registers, and that's handled by the sdhci: Add support for hosts
with strict 32 bit addressing patch.

 OTOH you refer to readw() in your example,
 not readl(). What's the deal here?

readw() was just an example (most complicated one).

  +static inline void sdhci_writel(struct sdhci_host *host, u32 val, int reg)
  +{
  +   host-writel(host, val, reg);
  +}
 
 Having to override these are worst case scenario

Hm. It's not a worst case scenario, it's a normal scenario for
eSDHC. Why should we treat eSDHC as a second-class citizen?

 as far as I'm
 concerned, so I'd prefer something like:
 
 if (!host-ops-writel)
   writel(host-ioaddr + reg, val);
 else
   host-ops-writel(host, val, reg);

Hm.

-- What I purpose:

$ size drivers/mmc/host/sdhci.o
   textdata bss dec hex filename
  15173   8   4   151853b51 drivers/mmc/host/sdhci.o

And there is a minimum run-time overhead (dereference + branch).

+ no first/second-class citizen separation.

-- What you purpose (inlined):

$ size drivers/mmc/host/sdhci.o
   textdata bss dec hex filename
  17853   8   4   1786545c9 drivers/mmc/host/sdhci.o

Runtime overhead: dereference + dereference + compare +
(maybe)branch + larger code.

-- What you purpose (uninlined):

$ size drivers/mmc/host/sdhci.o
   textdata bss dec hex filename
  14692   8   4   147043970 drivers/mmc/host/sdhci.o

Better. But the runtime overhead: branch + dereference + dereference +
compare + (maybe)branch.


Surely the overhead isn't measurable... but why we purposely make
things worse?

Though, this is not something I'm going to argue about, I'll just
do it the way you prefer. ;-) For an updated patch set I took
the uninlined variant, hope this is OK.

Thanks for the review,

-- 
Anton Vorontsov
email: cbouatmai...@gmail.com
irc://irc.freenode.net/bd2
___
Linuxppc-dev mailing list
Linuxppc-dev@ozlabs.org
https://ozlabs.org/mailman/listinfo/linuxppc-dev

Re: [PATCH 02/11] sdhci: Add support for bus-specific IO memory accessors

2009-02-08 Thread Pierre Ossman 
On Fri, 6 Feb 2009 21:06:45 +0300
Anton Vorontsov avoront...@ru.mvista.com wrote:

 Currently the SDHCI driver works with PCI accessors (write{l,b,w} and
 read{l,b,w}).
 
 With this patch drivers may change memory accessors, so that we can
 support hosts with weird IO memory access requirments.
 
 For example, in FSL eSDHC SDHCI hardware all registers are 32 bit
 width, with big-endian addressing. That is, readb(0x2f) should turn
 into readb(0x2c), and readw(0x2c) should be translated to
 le16_to_cpu(readw(0x2e)).
 
 Signed-off-by: Anton Vorontsov avoront...@ru.mvista.com
 ---

I was hoping we wouldn't have to do a lot of magic in the accessors
since the spec is rather clear on the register interface. :/

Let's see if I've understood this correctly.

1. The CPU is big-endian but the register are little-endian (as the
spec requires). I was under the impression that the read*/write*
accessor handled any endian conversion between the bus and the cpu? How
do e.g. PCI work on Sparc?

2. Register access must be done 32 bits at a time. Now this is just
broken and might cause big problems as some registers cannot just be
read and written back to. OTOH you refer to readw() in your example,
not readl(). What's the deal here?

 +static inline void sdhci_writel(struct sdhci_host *host, u32 val, int reg)
 +{
 + host-writel(host, val, reg);
 +}

Having to override these are worst case scenario as far as I'm
concerned, so I'd prefer something like:

if (!host-ops-writel)
writel(host-ioaddr + reg, val);
else
host-ops-writel(host, val, reg);

and maybe even a likely() up there.

Rgds
-- 
 -- Pierre Ossman

  WARNING: This correspondence is being monitored by the
  Swedish government. Make sure your server uses encryption
  for SMTP traffic and consider using PGP for end-to-end
  encryption.


signature.asc
Description: PGP signature
___
Linuxppc-dev mailing list
Linuxppc-dev@ozlabs.org
https://ozlabs.org/mailman/listinfo/linuxppc-dev

[PATCH 02/11] sdhci: Add support for bus-specific IO memory accessors

2009-02-06 Thread Anton Vorontsov 
Currently the SDHCI driver works with PCI accessors (write{l,b,w} and
read{l,b,w}).

With this patch drivers may change memory accessors, so that we can
support hosts with weird IO memory access requirments.

For example, in FSL eSDHC SDHCI hardware all registers are 32 bit
width, with big-endian addressing. That is, readb(0x2f) should turn
into readb(0x2c), and readw(0x2c) should be translated to
le16_to_cpu(readw(0x2e)).

Signed-off-by: Anton Vorontsov avoront...@ru.mvista.com
---
 drivers/mmc/host/sdhci-pci.c |4 +-
 drivers/mmc/host/sdhci.c |  205 +-
 drivers/mmc/host/sdhci.h |   48 ++
 3 files changed, 174 insertions(+), 83 deletions(-)

diff --git a/drivers/mmc/host/sdhci-pci.c b/drivers/mmc/host/sdhci-pci.c
index f07255c..a3ced4d 100644
--- a/drivers/mmc/host/sdhci-pci.c
+++ b/drivers/mmc/host/sdhci-pci.c
@@ -212,7 +212,7 @@ static int jmicron_probe_slot(struct sdhci_pci_slot *slot)
if (slot-chip-pdev-revision == 0) {
u16 version;
 
-   version = readl(slot-host-ioaddr + SDHCI_HOST_VERSION);
+   version = sdhci_readl(slot-host, SDHCI_HOST_VERSION);
version = (version  SDHCI_VENDOR_VER_MASK) 
SDHCI_VENDOR_VER_SHIFT;
 
@@ -583,7 +583,7 @@ static void sdhci_pci_remove_slot(struct sdhci_pci_slot 
*slot)
u32 scratch;
 
dead = 0;
-   scratch = readl(slot-host-ioaddr + SDHCI_INT_STATUS);
+   scratch = sdhci_readl(slot-host, SDHCI_INT_STATUS);
if (scratch == (u32)-1)
dead = 1;
 
diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
index 0a1f5c5..8557521 100644
--- a/drivers/mmc/host/sdhci.c
+++ b/drivers/mmc/host/sdhci.c
@@ -48,35 +48,35 @@ static void sdhci_dumpregs(struct sdhci_host *host)
printk(KERN_DEBUG DRIVER_NAME : == REGISTER DUMP 
==\n);
 
printk(KERN_DEBUG DRIVER_NAME : Sys addr: 0x%08x | Version:  0x%08x\n,
-   readl(host-ioaddr + SDHCI_DMA_ADDRESS),
-   readw(host-ioaddr + SDHCI_HOST_VERSION));
+   sdhci_readl(host, SDHCI_DMA_ADDRESS),
+   sdhci_readw(host, SDHCI_HOST_VERSION));
printk(KERN_DEBUG DRIVER_NAME : Blk size: 0x%08x | Blk cnt:  0x%08x\n,
-   readw(host-ioaddr + SDHCI_BLOCK_SIZE),
-   readw(host-ioaddr + SDHCI_BLOCK_COUNT));
+   sdhci_readw(host, SDHCI_BLOCK_SIZE),
+   sdhci_readw(host, SDHCI_BLOCK_COUNT));
printk(KERN_DEBUG DRIVER_NAME : Argument: 0x%08x | Trn mode: 0x%08x\n,
-   readl(host-ioaddr + SDHCI_ARGUMENT),
-   readw(host-ioaddr + SDHCI_TRANSFER_MODE));
+   sdhci_readl(host, SDHCI_ARGUMENT),
+   sdhci_readw(host, SDHCI_TRANSFER_MODE));
printk(KERN_DEBUG DRIVER_NAME : Present:  0x%08x | Host ctl: 0x%08x\n,
-   readl(host-ioaddr + SDHCI_PRESENT_STATE),
-   readb(host-ioaddr + SDHCI_HOST_CONTROL));
+   sdhci_readl(host, SDHCI_PRESENT_STATE),
+   sdhci_readb(host, SDHCI_HOST_CONTROL));
printk(KERN_DEBUG DRIVER_NAME : Power:0x%08x | Blk gap:  0x%08x\n,
-   readb(host-ioaddr + SDHCI_POWER_CONTROL),
-   readb(host-ioaddr + SDHCI_BLOCK_GAP_CONTROL));
+   sdhci_readb(host, SDHCI_POWER_CONTROL),
+   sdhci_readb(host, SDHCI_BLOCK_GAP_CONTROL));
printk(KERN_DEBUG DRIVER_NAME : Wake-up:  0x%08x | Clock:0x%08x\n,
-   readb(host-ioaddr + SDHCI_WAKE_UP_CONTROL),
-   readw(host-ioaddr + SDHCI_CLOCK_CONTROL));
+   sdhci_readb(host, SDHCI_WAKE_UP_CONTROL),
+   sdhci_readw(host, SDHCI_CLOCK_CONTROL));
printk(KERN_DEBUG DRIVER_NAME : Timeout:  0x%08x | Int stat: 0x%08x\n,
-   readb(host-ioaddr + SDHCI_TIMEOUT_CONTROL),
-   readl(host-ioaddr + SDHCI_INT_STATUS));
+   sdhci_readb(host, SDHCI_TIMEOUT_CONTROL),
+   sdhci_readl(host, SDHCI_INT_STATUS));
printk(KERN_DEBUG DRIVER_NAME : Int enab: 0x%08x | Sig enab: 0x%08x\n,
-   readl(host-ioaddr + SDHCI_INT_ENABLE),
-   readl(host-ioaddr + SDHCI_SIGNAL_ENABLE));
+   sdhci_readl(host, SDHCI_INT_ENABLE),
+   sdhci_readl(host, SDHCI_SIGNAL_ENABLE));
printk(KERN_DEBUG DRIVER_NAME : AC12 err: 0x%08x | Slot int: 0x%08x\n,
-   readw(host-ioaddr + SDHCI_ACMD12_ERR),
-   readw(host-ioaddr + SDHCI_SLOT_INT_STATUS));
+   sdhci_readw(host, SDHCI_ACMD12_ERR),
+   sdhci_readw(host, SDHCI_SLOT_INT_STATUS));
printk(KERN_DEBUG DRIVER_NAME : Caps: 0x%08x | Max curr: 0x%08x\n,
-   readl(host-ioaddr + SDHCI_CAPABILITIES),
-   readl(host-ioaddr + SDHCI_MAX_CURRENT));
+   sdhci_readl(host, SDHCI_CAPABILITIES),
+   sdhci_readl(host, SDHCI_MAX_CURRENT));