Re: [PATCH v2] mci: add Atmel AT91 MCI driver

[Sascha Hauer <s.hauer@pengutronix.de>]

On Mon, Jun 06, 2011 at 09:43:49AM +0200, Hubert Feurstein wrote:
> 2011/6/2 Jean-Christophe PLAGNIOL-VILLARD <plagnioj@jcrosoft.com>:
> > On 13:48 Wed 01 Jun     , Hubert Feurstein wrote:
> [snip]
> > I've test it on at91sam9263ek and work fine
> Great, good to hear ;) I'll add also support for at91sam9m10g45ek
> later in an extra commit.
> >
> > please fix the following comments
> > Acked-by: Jean-Christophe PLAGNIOL-VILLARD <plagnioj@jcrosoft.com>
> >
> [snip]
> >> +/* Multimedia Card Interface */
> >> +struct atmel_mci_platform_data {
> >> +     unsigned bus_width;
> >> +     unsigned host_caps; /* MCI_MODE_* from mci.h */
> >> +     unsigned detect_pin;
> >> +     unsigned wp_pin;
> >> +};
> >we can have 2 slot but you allow only one
> Would it be ok to add the second slot in an extra commit?
> 
> [snip]
> >> +/*
> >> + * Structure for struct SoC access.
> >> + * Names starting with '_' are fillers.
> >> + */
> >> +struct atmel_mci_regs {
> >> +     /*      reg     Offset */
> >> +     u32     cr;     /* 0x00 */
> >> +     u32     mr;     /* 0x04 */
> >> +     u32     dtor;   /* 0x08 */
> >> +     u32     sdcr;   /* 0x0c */
> >> +     u32     argr;   /* 0x10 */
> >> +     u32     cmdr;   /* 0x14 */
> >> +     u32     blkr;   /* 0x18 */
> >> +     u32     _1c;    /* 0x1c */
> >> +     u32     rspr;   /* 0x20 */
> >> +     u32     rspr1;  /* 0x24 */
> >> +     u32     rspr2;  /* 0x28 */
> >> +     u32     rspr3;  /* 0x2c */
> >> +     u32     rdr;    /* 0x30 */
> >> +     u32     tdr;    /* 0x34 */
> >> +     u32     _38;    /* 0x38 */
> >> +     u32     _3c;    /* 0x3c */
> >> +     u32     sr;     /* 0x40 */
> >> +     u32     ier;    /* 0x44 */
> >> +     u32     idr;    /* 0x48 */
> >> +     u32     imr;    /* 0x4c */
> >> +};
> > please use the same as the kernel here
> > offset not a struct
> Of course I could change it to offset, but I thought this is the way
> how it is done in barebox.

Nope, it's prefered in U-Boot but not barebox.

> What is Sascha's opinion on that?

Personally I prefer having defines instead of struct types for
variables. Reasons for this are:

- defines make the actual register offset clear without having to put
  comments after them. Also, you can't make mistakes while numbering
  the registers.
- Type safety is often an argument for using struct types, but 16 bit
  registers with 32bit alignment are just too ugly to describe in struct
  types. Also, if used in another SoC the registers might have a
  different alignment which is also quite ugly to describe in structs.

I know there are other opinions and I won't nack patches just because
of this. It's probably best to just use the way the drivers does you
copied this one from.


> >> +
> >> +struct atmel_mci_host {
> >> +     struct mci_host mci;
> >> +     struct atmel_mci_regs volatile __iomem *base;
> >> +     struct device_d *hw_dev;
> >> +     struct clk *clk;
> >> +
> >> +     u32 datasize;
> >> +     struct mci_cmd *cmd;
> >> +     struct mci_data *data;
> >> +};
> >> +
> >
> >> +static int atmel_pull(struct atmel_mci_host *host, void *_buf, int bytes)
> >> +{
> >> +     unsigned int stat;
> >> +     u32 *buf = _buf;
> >> +
> >> +     while (bytes > 3) {
> >> +             stat = atmel_poll_status(host, AT91_MCI_RXRDY);
> >> +             if (stat)
> >> +                     return stat;
> >> +
> >> +             *buf++ = readl(&host->base->rdr);
> >> +             bytes -= 4;
> >> +     }
> >> +
> >> +     if (bytes) {
> >> +             u8 *b = (u8 *)buf;
> >> +             u32 tmp;
> >> +
> >> +             stat = atmel_poll_status(host, AT91_MCI_RXRDY);
> >> +             if (stat)
> >> +                     return stat;
> >> +
> >> +             tmp = readl(&host->base->rdr);
> >> +             memcpy(b, &tmp, bytes);
> > please use __iowrite32 to speedup the copy

Ah, here is the potential __iowrite32 user ;)

> I think memcpy is alright here. Usually this code-path shouldn't be
> executed anyway, because the mci-core always
> requests multiples of 512 bytes, and here we copy only the last
> remaining _three_ bytes.

One thing to consider when using memcpy for io accesses is that it
is not specified which type of accesses are used. For example on arm,
when assembler optimized string functions are used, the code will
use 32 bit accesses when possible. If not, memcpy will be a simple
byte copy loop.
I really doubt this code works as expected. Does your hardware support
byte accesses? Might be better to just add a WARN_ON(bytes) instead
of introducing this kind of untested code.

Sascha

-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox