Hi
Le Wednesday 24 June 2009 13:28:56 Alexander Clouter, vous avez écrit :
> Hi,
>
> Florian Fainelli <florian@xxxxxxxxxxx> wrote:
> > Le Tuesday 23 June 2009 20:15:09 Ralf Baechle, vous avez écrit :
> >> On Tue, Jun 23, 2009 at 11:28:27AM +0200, Florian Fainelli wrote:
> >>
> >> AR7 time again - the platform pending longest ... Let's see:
> >
> > Thank you very much for your comments Ralf, please find below an updated
> > version which addresses all of your comments. I hope this time we are
> > going to make it ;)
>
> I am hoping someone can have a tackle of the lzma/bzip2 kernel/initramfs
> generic compression code myself, but I guess one thing at a time. :) If
> you have a simple way for me to produce a LZMA'd image, I'll test it
> this on my WAG54Gv2 (I need the image to be less than 700kB).
>
> My comments, for what they are worth, below:
> > From: Florian Fainelli <florian@xxxxxxxxxxx>
> > Subject: Add support for Texas Instruments AR7 System-on-a-Chip
> >
> > This patch adds support for the Texas Instruments AR7 System-on-a-Chip.
> > It supports the TNETD7100, 7200 and 7300 versions of the SoC.
> >
> > Changes from v4:
> > [snipped]
> >
> > Signed-off-by: Matteo Croce <matteo@xxxxxxxxxxx>
> > Signed-off-by: Felix Fietkau <nbd@xxxxxxxxxxx>
> > Signed-off-by: Eugene Konev <ejka@xxxxxxxxxxx>
> > Signed-off-by: Nicolas Thill <nico@xxxxxxxxxxx>
> > Signed-off-by: Florian Fainelli <florian@xxxxxxxxxxx>
> > --
> > [snipped]
> >
> > diff --git a/arch/mips/ar7/clock.c b/arch/mips/ar7/clock.c
> > new file mode 100644
> > index 0000000..7ce5f07
> > --- /dev/null
> > +++ b/arch/mips/ar7/clock.c
> > @@ -0,0 +1,450 @@
> > [snipped]
> > +static void __init tnetd7300_init_clocks(void)
> > +{
> > + u32 *bootcr = (u32 *)ioremap_nocache(AR7_REGS_DCL, 4);
> > + struct tnetd7300_clocks *clocks =
> > + (struct tnetd7300_clocks *)
> > + ioremap_nocache(AR7_REGS_POWER +
> > 0x20, + sizeof(struct
> > tnetd7300_clocks)); +
>
> Needless cast'ing and also should you not check that NULL is not
> returned for both of these ioremap's?
As Ralf explained, ioremap will not fail here.
>
> > + ar7_bus_clock = tnetd7300_get_clock(BUS_PLL_SOURCE_SHIFT,
> > + &clocks->bus, bootcr, AR7_AFE_CLOCK);
> > +
> > + if (*bootcr & BOOT_PLL_ASYNC_MODE)
> > + ar7_cpu_clock = tnetd7300_get_clock(CPU_PLL_SOURCE_SHIFT,
> > + &clocks->cpu, bootcr, AR7_AFE_CLOCK);
> > + else
> > + ar7_cpu_clock = ar7_bus_clock;
> > +/*
> > + tnetd7300_set_clock(USB_PLL_SOURCE_SHIFT, &clocks->usb,
> > + bootcr, 48000000);
> > +*/
>
> Dead code? Feels that way with what then follows.
Not really, but not used yet since we have not ported the USB HCI driver yet.
>
> > + if (ar7_dsp_clock == 250000000)
> > + tnetd7300_set_clock(DSP_PLL_SOURCE_SHIFT, &clocks->dsp,
> > + bootcr, ar7_dsp_clock);
> > +
> > + iounmap(clocks);
> > + iounmap(bootcr);
> > +}
> >
> > [snipped]
> >
> > +static void tnetd7200_set_clock(int base, struct tnetd7200_clock *clock,
> > + int prediv, int postdiv, int postdiv2, int mul, u32 frequency)
> > +{
> > + printk(KERN_INFO
> > + "Clocks: base = %d, frequency = %u, prediv = %d, "
> > + "postdiv = %d, postdiv2 = %d, mul = %d\n",
> > + base, frequency, prediv, postdiv, postdiv2, mul);
> > +
> > + writel(0, &clock->ctrl);
> > + writel(DIVISOR_ENABLE_MASK | ((prediv - 1) & 0x1F),
> > &clock->prediv); + writel((mul - 1) & 0xF, &clock->mul);
> > +
> > + for (mul = 0; mul < 2000; mul++)
> > + ; /* nop */
> > +
>
> Only for furthering my know how, this because the timing is changing so
> you cannot msleep() or whatever? Can we not jump straight into the
> while() instead and wait it out? Alternatively start with a:
> ----
> while (!readl(&clock->status) & 0x1)
> ;
> ----
>
> and then lead into the following?
Yes, we can keep busy waiting on the status register, this works too.
>
> > + while (readl(&clock->status) & 0x1)
> > + ; /* nop */
> > +
> > + writel(DIVISOR_ENABLE_MASK | ((postdiv - 1) & 0x1F),
> > &clock->postdiv); +
> > + writel(readl(&clock->cmden) | 1, &clock->cmden);
> > + writel(readl(&clock->cmd) | 1, &clock->cmd);
> > +
> > + while (readl(&clock->status) & 0x1)
> > + ; /* nop */
> > +
> > + writel(DIVISOR_ENABLE_MASK | ((postdiv2 - 1) & 0x1F),
> > &clock->postdiv2); +
> > + writel(readl(&clock->cmden) | 1, &clock->cmden);
> > + writel(readl(&clock->cmd) | 1, &clock->cmd);
> > +
> > + while (readl(&clock->status) & 0x1)
> > + ; /* nop */
> > +
> > + writel(readl(&clock->ctrl) | 1, &clock->ctrl);
> > +}
> >
> > [snipped]
> >
> > +static void __init tnetd7200_init_clocks(void)
> > +{
> > + u32 *bootcr = (u32 *)ioremap_nocache(AR7_REGS_DCL, 4);
> > + struct tnetd7200_clocks *clocks =
> > + (struct tnetd7200_clocks *)
> > + ioremap_nocache(AR7_REGS_POWER +
> > 0x80, + sizeof(struct
> > tnetd7200_clocks));
>
> casting and NULL checks?
>
> > +int __init ar7_init_clocks(void)
> > +{
> > + switch (ar7_chip_id()) {
> > + case AR7_CHIP_7100:
> > + tnetd7200_init_clocks();
> > + break;
> > + case AR7_CHIP_7200:
> > + tnetd7200_init_clocks();
> > + break;
>
> Change this to the following maybe?
> ----
> switch (ar7_chip_id()) {
> case AR7_CHIP_7100:
> case AR7_CHIP_7200:
> tnetd7200_init_clocks();
> break;
> ----
>
> > + case AR7_CHIP_7300:
> > + ar7_dsp_clock = tnetd7300_dsp_clock();
> > + tnetd7300_init_clocks();
> > + break;
> > + default:
> > + break;
> > + }
> > +
> > + return 0;
> > +}
> > +arch_initcall(ar7_init_clocks);
> >
> > diff --git a/arch/mips/ar7/gpio.c b/arch/mips/ar7/gpio.c
>
> Too late now but gpiolib is rather nice to look into at some stage.
Yes, I will submit that later when AR7 is already included.
>
> > diff --git a/arch/mips/ar7/setup.c b/arch/mips/ar7/setup.c
> > [snipped]
> > +#include <linux/version.h>
> > +#include <linux/init.h>
> > +#include <linux/ioport.h>
> > +#include <linux/pm.h>
> > +#include <linux/time.h>
> > +
> > +#include <asm/reboot.h>
> > +#include <asm/mach-ar7/ar7.h>
> > +#include <asm/mach-ar7/prom.h>
> > +
> > +static void ar7_machine_restart(char *command);
> > +static void ar7_machine_halt(void);
> > +static void ar7_machine_power_off(void);
> > +
>
> I don't think these are needed as you have the following in the
> 'correct' order.
Indeed.
>
> > +static void ar7_machine_restart(char *command)
> > +{
> > + u32 *softres_reg = ioremap(AR7_REGS_RESET +
> > + AR7_RESET_SOFTWARE, 1);
> > + writel(1, softres_reg);
> > +}
> > +
> > +static void ar7_machine_halt(void)
> > +{
> > + while (1)
> > + ;
> > +}
> > +
> > +static void ar7_machine_power_off(void)
> > +{
> > + u32 *power_reg = (u32 *)ioremap(AR7_REGS_POWER, 1);
> > + u32 power_state = readl(power_reg) | (3 << 30);
> > + writel(power_state, power_reg);
> > + ar7_machine_halt();
> > +}
> >
> > +const char *get_system_type(void)
> > +{
> > + u16 chip_id = ar7_chip_id();
> > + switch (chip_id) {
> > + case AR7_CHIP_7300:
> > + return "TI AR7 (TNETD7300)";
> > + case AR7_CHIP_7100:
> > + return "TI AR7 (TNETD7100)";
> > + case AR7_CHIP_7200:
> > + return "TI AR7 (TNETD7200)";
> > + default:
> > + return "TI AR7 (Unknown)";
> > + }
> > +}
> > +
> > +static int __init ar7_init_console(void)
> > +{
> > + return 0;
> > +}
> > +console_initcall(ar7_init_console);
> > +
>
> Does this need to be called?
>
> > +/*
> > + * Initializes basic routines and structures pointers, memory size (as
> > + * given by the bios and saves the command line.
> > + */
> > +
> > +void __init plat_mem_setup(void)
> > +{
> > + unsigned long io_base;
> > +
> > + _machine_restart = ar7_machine_restart;
> > + _machine_halt = ar7_machine_halt;
> > + pm_power_off = ar7_machine_power_off;
> > + panic_timeout = 3;
> > +
> > + io_base = (unsigned long)ioremap(AR7_REGS_BASE, 0x10000);
> > + if (!io_base)
> > + panic("Can't remap IO base!\n");
> > + set_io_port_base(io_base);
> > +
>
> Casting a pointer to a unsigned long...hmmmm.
>
> > + prom_meminit();
> > +
> > + printk(KERN_INFO "%s, ID: 0x%04x, Revision: 0x%02x\n",
> > + get_system_type(),
> > + ar7_chip_id(), ar7_chip_rev());
> > +}
> > diff --git a/arch/mips/ar7/time.c b/arch/mips/ar7/time.c
> > new file mode 100644
> > index 0000000..a1fba89
> > --- /dev/null
> > +++ b/arch/mips/ar7/time.c
> > @@ -0,0 +1,30 @@
> > +/*
> > + * Carsten Langgaard, carstenl@xxxxxxxx
> > + * Copyright (C) 1999,2000 MIPS Technologies, Inc. All rights reserved.
> > + *
> > + * This program is free software; you can distribute it and/or modify
> > it + * under the terms of the GNU General Public License (Version 2) as
> > + * published by the Free Software Foundation.
> > + *
> > + * This program is distributed in the hope it will be useful, but
> > WITHOUT + * ANY WARRANTY; without even the implied warranty of
> > MERCHANTABILITY or + * FITNESS FOR A PARTICULAR PURPOSE. See the GNU
> > General Public License + * for more details.
> > + *
> > + * You should have received a copy of the GNU General Public License
> > along + * with this program; if not, write to the Free Software
> > Foundation, Inc., + * 59 Temple Place - Suite 330, Boston MA 02111-1307,
> > USA.
> > + *
> > + * Setting up the clock on the MIPS boards.
> > + */
> > +
> > +#include <linux/init.h>
> > +#include <linux/time.h>
> > +
> > +#include <asm/time.h>
> > +#include <asm/mach-ar7/ar7.h>
> > +
> > +void __init plat_time_init(void)
> > +{
> > + mips_hpt_frequency = ar7_cpu_freq() / 2;
> > +}
> >
> > diff --git a/arch/mips/include/asm/mach-ar7/ar7.h
> > b/arch/mips/include/asm/mach-ar7/ar7.h [snipped]
> > +#ifndef __AR7_H__
> > +#define __AR7_H__
> > +
> > +#include <linux/delay.h>
> > +#include <asm/addrspace.h>
> > +#include <linux/io.h>
> > +
> > +#define AR7_SDRAM_BASE 0x14000000
> > +
> > +#define AR7_REGS_BASE 0x08610000
> > +
> > +#define AR7_REGS_MAC0 (AR7_REGS_BASE + 0x0000)
> > +#define AR7_REGS_GPIO (AR7_REGS_BASE + 0x0900)
> > +/* 0x08610A00 - 0x08610BFF (512 bytes, 128 bytes / clock) */
> > +#define AR7_REGS_POWER (AR7_REGS_BASE + 0x0a00)
> > +#define AR7_REGS_UART0 (AR7_REGS_BASE + 0x0e00)
> > +#define AR7_REGS_USB (AR7_REGS_BASE + 0x1200)
> > +#define AR7_REGS_RESET (AR7_REGS_BASE + 0x1600)
> > +#define AR7_REGS_VLYNQ0 (AR7_REGS_BASE + 0x1800)
> > +#define AR7_REGS_DCL (AR7_REGS_BASE + 0x1a00)
> > +#define AR7_REGS_VLYNQ1 (AR7_REGS_BASE + 0x1c00)
> > +#define AR7_REGS_MDIO (AR7_REGS_BASE + 0x1e00)
> > +#define AR7_REGS_IRQ (AR7_REGS_BASE + 0x2400)
> > +#define AR7_REGS_MAC1 (AR7_REGS_BASE + 0x2800)
> > +
> > +#define AR7_REGS_WDT (AR7_REGS_BASE + 0x1f00)
> > +#define UR8_REGS_WDT (AR7_REGS_BASE + 0x0b00)
> > +#define UR8_REGS_UART1 (AR7_REGS_BASE + 0x0f00)
> > +
>
> I guess it's style, but I have preferred '(<blah> | <offset>)'. You
> might want to add a few more for unlabelled offsets such as
> "AR7_REGS_POWER + 0x80" to make things more self documenting?
Right, added too.
>
> > [snipped]
> > +extern int ar7_cpu_clock, ar7_bus_clock, ar7_dsp_clock;
> > +
> > +static inline u16 ar7_chip_id(void)
> > +{
> > + return readl((void *)KSEG1ADDR(AR7_REGS_GPIO + 0x14)) & 0xffff;
> > +}
> > +
> > +static inline u8 ar7_chip_rev(void)
> > +{
> > + return (readl((void *)KSEG1ADDR(AR7_REGS_GPIO + 0x14)) >> 16) &
> > 0xff; +}
>
> unneeded casting?
>
> > [snipped]
> > +
> > +static inline int ar7_has_high_cpmac(void)
> > +{
> > + u16 chip_id = ar7_chip_id();
> > + switch (chip_id) {
> > + case AR7_CHIP_7100:
> > + case AR7_CHIP_7200:
> > + return 0;
> > + default:
> > + return 1;
> > + }
> > +}
>
> I would probably recommend as a default to return ENXIO or EINVAL maybe?
> Then for AR7_CHIP_7300 you return 1, I'm guessing?
Yes, thanks.
>
> I have been slightly tracking the ar7 code for a while and I have to say
> it is really looking much nicer now-a-days. Well done! If you ever are
> in London, I'll buy you a beer.
I am in Paris at the moment, but you can also come here ;)
--
Best regards, Florian Fainelli
Email : florian@xxxxxxxxxxx
http://openwrt.org
-------------------------------
