On Fri, 2014-03-14 at 17:36 +0200, Pantelis Antoniou wrote:
[...]
Thanks for your review. It seems there are still quite a few issues dating back to the original allwinner dumps here.
@linux-sunxi: if anyone wants to volunteer to help cleanup this particular driver I'd be very happy -- there's a lot of it!
+static void dumpmmcreg(struct sunxi_mmc *reg) +{
- debug("dump mmc registers:\n");
- debug("gctrl 0x%08x\n", reg->gctrl);
- debug("clkcr 0x%08x\n", reg->clkcr);
- debug("timeout 0x%08x\n", reg->timeout);
- debug("width 0x%08x\n", reg->width);
- debug("blksz 0x%08x\n", reg->blksz);
[...] lots more debug(foo)
+}
^^^ #ifdef DEBUG here?
I can if you prefer but debug() itself effectively includes the same ifdef so the end result is already the same.
[...]
+/* support 4 mmc hosts */ +struct mmc mmc_dev[4]; +struct sunxi_mmc_host mmc_host[4];
^ hosts & mmc structs can be allocated even for SPL now
Can be or must be?
- struct sunxi_mmc_host *mmchost = &mmc_host[sdc_no];
- static struct sunxi_gpio *gpio_c =
&((struct sunxi_gpio_reg *)SUNXI_PIO_BASE)->gpio_bank[SUNXI_GPIO_C];- static struct sunxi_gpio *gpio_f =
&((struct sunxi_gpio_reg *)SUNXI_PIO_BASE)->gpio_bank[SUNXI_GPIO_F];+#if CONFIG_MMC1_PG
- static struct sunxi_gpio *gpio_g =
&((struct sunxi_gpio_reg *)SUNXI_PIO_BASE)->gpio_bank[SUNXI_GPIO_G];+#endif
- static struct sunxi_gpio *gpio_h =
&((struct sunxi_gpio_reg *)SUNXI_PIO_BASE)->gpio_bank[SUNXI_GPIO_H];- static struct sunxi_gpio *gpio_i =
&((struct sunxi_gpio_reg *)SUNXI_PIO_BASE)->gpio_bank[SUNXI_GPIO_I];- struct sunxi_ccm_reg *ccm = (struct sunxi_ccm_reg *)SUNXI_CCM_BASE;
^ Castings are ugly; rework with a temporary variable.
Ack.
The static's here are odd too and date back to the original alwinner code dumps. I'll get rid of them too.
[...]
- case 3:
/* PI4-CMD, PI5-CLK, PI6~9-D0~D3 : 2 */writel(0x2222 << 16, &gpio_i->cfg[0]);writel(0x22, &gpio_i->cfg[1]);writel(0x555 << 8, &gpio_i->pull[0]);writel(0x555 << 8, &gpio_i->drv[0]);break;- default:
return -1;- }
Lots of magic constants. I have no idea what's going on here. Use a few defines.
Right. These came from the original allwinner dumps so I was worried that they might be undocumented magic, but actually since the are gpio frobbing I reckon I can figure them out.
[...]> + cmd = (0x1 << 31) | (0x1 << 21) | (0x1 << 13);
- writel(cmd, &mmchost->reg->cmd);
- while ((readl(&mmchost->reg->cmd) & (0x1 << 31)) && timeout--);
- if (!timeout)
return -1;^ Eeek! Use udelay and a time based timeout.
Ack.
- writel(readl(&mmchost->reg->rint), &mmchost->reg->rint);
^ Same here - Not even a timeout?
No loop here though? [...]
- rval |= (0x1 << 16);
#define ?
Ack
[...]
Ambiguous formatting. Use { }
Ack
[...]
- /* Reset controller */
- writel(0x7, &mmchost->reg->gctrl);
Magic value again.
The sum total of the docs for this one are: * GCTRLREG * GCTRL[2] : DMA reset * GCTRL[5] : DMA enable
But I'll see what I can do.
[...]
- } else {
buff = (unsigned int *)data->src;for (i = 0; i < (byte_cnt >> 2); i++) {while (--timeout &&(readl(&mmchost->reg->status) & (0x1 << 3)));if (timeout <= 0)goto out;writel(buff[i], mmchost->database);timeout = 0xfffff;}- }
^ Timeouts using time values? udelay? See above.
Ack.
[....]
- writel(rval, &mmchost->reg->idie);
- writel((u32) pdes, &mmchost->reg->dlba);
- writel((0x2 << 28) | (0x7 << 16) | (0x01 << 3),
&mmchost->reg->ftrglevel);^ #define again?
Some of these (ftrgllevel) have no docs whatsoever, but I'll do what I can.
[...]
- timeout = 0xfffff;
- do {
status = readl(&mmchost->reg->rint);if (!timeout-- || (status & 0xbfc2)) {error = status & 0xbfc2;debug("cmd timeout %x\n", error);error = TIMEOUT;goto out;}^ Again timeouts without using time values.
I'm getting the picture ;-)
[...]
+int sunxi_mmc_init(int sdc_no) +{
- struct mmc *mmc;
- memset(&mmc_dev[sdc_no], 0, sizeof(struct mmc));
- memset(&mmc_host[sdc_no], 0, sizeof(struct sunxi_mmc_host));
- mmc = &mmc_dev[sdc_no];
- sprintf(mmc->name, "SUNXI SD/MMC");
- mmc->priv = &mmc_host[sdc_no];
- mmc->send_cmd = mmc_send_cmd;
- mmc->set_ios = mmc_set_ios;
- mmc->init = mmc_core_init;
- mmc->voltages = MMC_VDD_32_33 | MMC_VDD_33_34;
- mmc->host_caps = MMC_MODE_4BIT;
- mmc->host_caps |= MMC_MODE_HS_52MHz | MMC_MODE_HS;
- mmc->f_min = 400000;
- mmc->f_max = 52000000;
- mmc_resource_init(sdc_no);
- mmc_clk_io_on(sdc_no);
- mmc_register(mmc);
^ The mmc_registeration sequence has changed, but not landed at master yet.
Do you have a reference handy for this?
Thanks again, sorry this driver is such a mess (I must confess I didn't look at it that closely when I cherry picked it).
Ian.