[U-Boot] [PATCH 01/22] ARM: sunxi: Basic Allwinner A10/A13 (sun4i/sun5i) support

[Henrik Nordström]

sön 2012-11-25 klockan 19:06 +0100 skrev Marek Vasut:
> > +COBJS	+= timer.o
> > +COBJS	+= dram.o
> > +COBJS	+= board.o
> > +COBJS	+= clock.o
> > +COBJS	+= pinmux.o
> 
> COBJS = x.o y.o z.o
> works just fine

Does it matter?

+= is easier to deal with when adding/removing files.

> > +void enable_caches(void)
> > +{
> > +	/* Enable D-cache. I-cache is already enabled in start.S */
> > +	dcache_enable();
> 
> Why don't you enable them both? What if someone fiddled with them prior to 
> reset?

Because as the comment says, u-boot already enables I-cache in armv7
start.S.

> > +}
> > +#endif
> [...]
> 
> > +	cfg = readl(&pio->cfg[0] + index);
> > +	cfg &= ~(0xf << offset);
> > +	cfg |= val << offset;
> > +
> > +	writel(cfg, &pio->cfg[0] + index);
> 
> clrsetbits_le32()

Yes. Need to fix those in many places. Will do a full review.

> > +	return 0;
> > +}
> > +
> > +int sunxi_gpio_get_cfgpin(u32 pin)
> > +{
> > +	u32 cfg;
> > +	u32 bank = GPIO_BANK(pin);
> > +	u32 index = GPIO_CFG_INDEX(pin);
> > +	u32 offset = GPIO_CFG_OFFSET(pin);
> > +	struct sunxi_gpio *pio =
> > +	    &((struct sunxi_gpio_reg *)SUNXI_PIO_BASE)->gpio_bank[bank];
> > +
> > +	cfg = readl(&pio->cfg[0] + index);
> > +	cfg >>= offset;
> 
> I think you want to put this into drivers/gpio, no?

Why? It is accessing the pinmux setting of the pin, not GPIO value.

May want to rename the whole family of functions to _pinmux_

> > +++ b/arch/arm/cpu/armv7/sunxi/reset.S

> Put this into proper C code.

Agreed. Inherited it as asm but there is no need for asm here. Will
replace by parts from the watchdog code, which also sets the register
proper.

> > diff --git a/arch/arm/cpu/armv7/sunxi/timer.c
> > b/arch/arm/cpu/armv7/sunxi/timer.c new file mode 100644
> > index 0000000..e19df09
> > --- /dev/null
> > +++ b/arch/arm/cpu/armv7/sunxi/timer.c
> > @@ -0,0 +1,117 @@
> [...]
> 
> > +/* delay x useconds */
> 
> proper kerneldoc all around won't hurt.
>
>> +void __udelay(unsigned long usec)
> 

It's the same level of documentation or better as used pretty much
everywhere else. Most don't have a comment at all here.

Any example of the level of documentation you'd like to see?

Regards
Henrik