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

[Chen-Yu Tsai]

On Mon, Mar 17, 2014 at 4:38 AM, Ian Campbell <ijc at 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