On Mon, Mar 17, 2014 at 4:38 AM, Ian Campbell <i...@hellion.org.uk> wrote:
> 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.
You can drop the gpio ones in favor of using the sunxi gpio driver.
>> [...]
>> > + 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.
Should be something like this:
for (pin = SUNXI_GPI(4); pin <= SUNXI_GPI(9); pin++) {
sunxi_gpio_set_cfgpin(pin, SUNXI_GPI_SDC3);
sunxi_gpio_set_drv(pin, 1);
sunxi_gpio_set_pull(pin, SUNXI_PULL_UP);
}
Note that SUNXI_GPI_* and SUNXI_PULL_* have not been defined.
I will send a patch for both the macros and MMC pinmux setting.
[..]
Thanks for working on this!
Cheers
ChenYu
_______________________________________________
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
