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

[Hubert Feurstein <h.feurstein@gmail.com>]

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.
What is Sascha's opinion on that?

>> +
>> +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
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.

>> +     }
>> +
>> +     return 0;
>> +}
>> +
[snip]
>> +
>> +     if (bus_width == 8)
>> +             writel(AT91_MCI_SDCBUS_8BIT, &host->base->sdcr);
>> +     if (bus_width == 4)
>> +             writel(AT91_MCI_SDCBUS_4BIT, &host->base->sdcr);
>> +     else
>> +             writel(AT91_MCI_SDCBUS_1BIT, &host->base->sdcr);
> switch here
OK

[snip]
>> +
>> +static int mci_probe(struct device_d *hw_dev)
>> +{
>> +     unsigned long clk_rate;
>> +     struct atmel_mci_host *host;
>> +     struct atmel_mci_platform_data *pd = hw_dev->platform_data;
>> +
>> +     if (pd == NULL) {
> if (!pd)
OK
>> +             dev_err(hw_dev, "missing platform data\n");
>> +             return -EINVAL;
>> +     }
>> +
>> +     host = xzalloc(sizeof(*host));
>> +     host->mci.send_cmd = mci_request;
>> +     host->mci.set_ios = mci_set_ios;
>> +     host->mci.init = mci_reset;
>> +
>
> Best Regards,
> J.
>
Best Regards and thank you for the support.
Hubert

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