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.
_______________________________________________
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
