[U-Boot] [PATCH v1 7/9] sunxi: mmc support

[Ian Campbell]

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.