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

[Pierre Ossman <drzeus@drzeus.cx>]

[-- Attachment #1: Type: text/plain, Size: 2701 bytes --]

On Fri, 13 Feb 2009 17:40:39 +0300
Anton Vorontsov <avorontsov@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.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 198 bytes --]