2011/6/2 Jean-Christophe PLAGNIOL-VILLARD <plagnioj@xxxxxxxxxxxx>:
> 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@xxxxxxxxxxxx>
>
[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@xxxxxxxxxxxxxxxxxxx
http://lists.infradead.org/mailman/listinfo/barebox
