Re: [PATCH 08/10] add TI AR7 support

[Florian Fainelli <florian@openwrt.org>]

Hallo Ralf,

Le Tuesday 02 June 2009 14:51:59 Ralf Baechle, vous avez écrit :
[snip]
> > +
> > +	writel(((prediv - 1) << PREDIV_SHIFT) | (postdiv - 1), &clock->ctrl);
> > +	mdelay(1);
> > +	writel(4, &clock->pll);
> > +	while (readl(&clock->pll) & PLL_STATUS)
> > +		;
> > +	writel(((mul - 1) << MUL_SHIFT) | (0xff << 3) | 0x0e, &clock->pll);
> > +	mdelay(75);
>
> These calls to mdelay seem to be done very early before BogoMIPS has been
> calibrated?

Yes, is there a prefered way than using mdelay here ?

>
>
> Can you elaborate?

I do not think there has been AR7 devices having non-8250 compatible UARTs out 
there thus this code is simply useful. What is meant by the comment is that 
the UART will be reported by Linux as  ttyS0 .... is a Xscale. But this 
should not happen since there are only 8250 UARTs.

> If you don't have a MAC address, either use random_ether_addr() or don't
> bring up the network interface at all.  Multiple interfaces with the same
> MAC can cause chaos on a network to better avoid that.

Thanks for spotting this.

>
> > +struct psp_env_chunk {
> > +	u8 num;
> > +	u8 ctrl;
> > +	u16 csum;
> > +	u8 len;
> > +	char data[11];
> > +} __attribute__ ((packed));
>
> Afair historic versions of this code were totally polluted with
> __attribute__((packed)).  Thanks for cleaning that.
>
> Btw - Linux coding style: No space between __attribute__ and ((packed)).
>
> > +#include <asm/reboot.h>
>
> You get a false warning from checkpatch.pl here.  Which probably means
> either we should teach checkpatch.pl to check if <linux/reboot.h> is
> actually including <asm/reboot.h> or rename <asm/reboot.h> to something
> which would also help to avoid confusion.

<linux/reboot.h> is not including <asm/reboot.h> so renaming <asm/reboot.h> 
sounds like a good solution.

>
> > +++ b/arch/mips/include/asm/mach-ar7/spaces.h
>
> You can cut down this header file to something like:
>
> /* Copyright blurb */
> #ifndef _ASM_AR7_SPACES_H
> #define _ASM_AR7_SPACES_H
>
> /*
>  * This handles the memory map.
>  * We handle pages at KSEG0 for kernels with 32 bit address space.
>  */
> #define PAGE_OFFSET		0x94000000UL
> #define PHYS_OFFSET		0x14000000UL
>
> #include <asm/mach-generic/spaces.h>

Fixed, thanks !
-- 
Best regards, Florian Fainelli
Email : florian@openwrt.org
http://openwrt.org
-------------------------------