On Friday, September 09, 2011 10:23:09 AM Stefano Babic wrote:
> On 09/08/2011 10:42 PM, Marek Vasut wrote:
> > This patch supports:
> > - Timers
> > - Debug UART
> > - Clock
>
> Hi Marek,
>
> a general remark. It seems to me that your patchset comes directly from
> your development, as it looks like what I normally have when I develop a
> new board ;-)
>
> However, to put into mainline and to make review easier, because this is
> a porting to a new SOC, I am expecting that all patches related to a new
> file are squashed together. It makes no sense to have several patches
> regarding, for example, m28evk.h, because this is a new file.
Done, I'll retest and send V2 of the series
>
> Another general remark here: we tend to have the same structure and the
> same files for all IMX SOC. This means that all IMX SOC have a
> imx-regs.h that contain the required register definitions (really a
> subset what we have in kernel). Is it really necessary to split the
> definitions in several small files ?
Just like Heiko said ... it'd produce terribly big and messy file. I'd prefer
to
avoid that.
>
> > Signed-off-by: Marek Vasut <marek.va...@gmail.com>
> > Cc: Stefano Babic <sba...@denx.de>
> > Cc: Wolfgang Denk <w...@denx.de>
> > Cc: Detlev Zundel <d...@denx.de>
> > ---
> >
> > arch/arm/cpu/arm926ejs/mx28/Makefile | 46 +++
> > arch/arm/cpu/arm926ejs/mx28/clock.c | 359
> > ++++++++++++++++++++++ arch/arm/cpu/arm926ejs/mx28/mx28.c |
> > 131 ++++++++
> > arch/arm/cpu/arm926ejs/mx28/timer.c | 143 +++++++++
> > arch/arm/include/asm/arch-mx28/clock.h | 48 +++
> > arch/arm/include/asm/arch-mx28/imx-regs.h | 33 ++
> > arch/arm/include/asm/arch-mx28/mx28.h | 30 ++
> > arch/arm/include/asm/arch-mx28/regs-base.h | 88 ++++++
> > arch/arm/include/asm/arch-mx28/regs-clkctrl.h | 308 +++++++++++++++++++
> > arch/arm/include/asm/arch-mx28/regs-common.h | 66 ++++
> > arch/arm/include/asm/arch-mx28/regs-power.h | 409
> > +++++++++++++++++++++++++ arch/arm/include/asm/arch-mx28/regs-ssp.h
> > | 345 +++++++++++++++++++++
> > arch/arm/include/asm/arch-mx28/regs-timrot.h | 167 ++++++++++
> > arch/arm/include/asm/arch-mx28/regs-uartdbg.h | 182 +++++++++++ 14
> > files changed, 2355 insertions(+), 0 deletions(-)
> > create mode 100644 arch/arm/cpu/arm926ejs/mx28/Makefile
> > create mode 100644 arch/arm/cpu/arm926ejs/mx28/clock.c
> > create mode 100644 arch/arm/cpu/arm926ejs/mx28/mx28.c
> > create mode 100644 arch/arm/cpu/arm926ejs/mx28/timer.c
> > create mode 100644 arch/arm/include/asm/arch-mx28/clock.h
> > create mode 100644 arch/arm/include/asm/arch-mx28/imx-regs.h
> > create mode 100644 arch/arm/include/asm/arch-mx28/mx28.h
> > create mode 100644 arch/arm/include/asm/arch-mx28/regs-base.h
> > create mode 100644 arch/arm/include/asm/arch-mx28/regs-clkctrl.h
> > create mode 100644 arch/arm/include/asm/arch-mx28/regs-common.h
> > create mode 100644 arch/arm/include/asm/arch-mx28/regs-power.h
> > create mode 100644 arch/arm/include/asm/arch-mx28/regs-ssp.h
> > create mode 100644 arch/arm/include/asm/arch-mx28/regs-timrot.h
> > create mode 100644 arch/arm/include/asm/arch-mx28/regs-uartdbg.h
>
> regs-uartdbg.h is used only by the driver, right ? The driver should be
> in the driver directory, and also its header. See drivers/serial/
Honestly, I need to check if the file is used at all. We replaced the original
driver with pl011 driver.
>
> > +
> > +#include <common.h>
> > +#include <asm/errno.h>
> > +#include <asm/io.h>
> > +#include <asm/arch/clock.h>
> > +#include <asm/arch/regs-common.h>
> > +#include <asm/arch/regs-base.h>
> > +#include <asm/arch/regs-clkctrl.h>
> > +#include <asm/arch/regs-ssp.h>
>
> Again, regarding file splitting for register. If you prefer to have
> several files, I think it is better that imx-regs.h include them. Then
> we have still a common header imx-regs.h that contains all needed
> register definitions. This is a better interface for a board maintainer.
Won't it make the compiler slower at compile time if it has to go through all
of
the included files instead of a subset ?
>
> > +
> > +/* The PLL frequency is always 480MHz, see section 10.2 in iMX28
> > datasheet. */ +#define PLL_FREQ_KHZ 480000
> > +#define PLL_FREQ_COEF 18
> > +/* The XTAL frequency is always 24MHz, see section 10.2 in iMX28
> > datasheet. */ +#define XTAL_FREQ_KHZ 24000
> > +
> > +#define PLL_FREQ_MHZ (PLL_FREQ_KHZ / 1000)
> > +#define XTAL_FREQ_MHZ (XTAL_FREQ_KHZ / 1000)
> > +
> > +uint32_t mx28_get_pclk(void)
>
> This function must be static.
>
[...]
Fixed all the stuff
> > +/* Lowlevel init isn't used on i.MX28, so just have a dummy here */
> > +inline void lowlevel_init(void) {}
>
> Ok.
>
> > +
> > +void reset_cpu(ulong ignored) __attribute__((noreturn));
> > +
> > +void reset_cpu(ulong ignored)
> > +{
> > + struct mx28_clkctrl_regs *clkctrl_regs =
> > + (struct mx28_clkctrl_regs *)MXS_CLKCTRL_BASE;
> > + struct mx28_power_regs *power_regs =
> > + (struct mx28_power_regs *)MXS_POWER_BASE;
> > +
> > + writel(0, &power_regs->hw_power_charge);
> > + writel(0, &power_regs->hw_power_minpwr);
> > + writel(CLKCTRL_RESET_DIG, &clkctrl_regs->hw_clkctrl_reset);
> > +
> > + /* Endless loop, reset will exit from here */
> > + for (;;)
> > + ;
>
> To understand: does a reset mean on the i.MX28 a power off ? Do you turn
> off the power ? If this is the case, some features are not possible (as
> PRAM, for example) on this SOC.
This should just reset the chip. No poweroff.
[...]
>
> > +ulong get_timer(ulong base)
> > +{
> > + struct mx28_timrot_regs *timrot_regs =
> > + (struct mx28_timrot_regs *)MXS_TIMROT_BASE;
> > +
> > + /* Current tick value */
> > + uint32_t now = readl(&timrot_regs->hw_timrot_running_count0);
> > +
> > + if (lastdec >= now) {
> > + /*
> > + * normal mode (non roll)
> > + * move stamp forward with absolut diff ticks
> > + */
> > + timestamp += (lastdec - now);
> > + } else {
> > + /* we have rollover of decrementer */
> > + timestamp += (TIMER_LOAD_VAL - now) + lastdec;
> > +
> > + }
> > + lastdec = now;
> > +
> > + return tick_to_time(timestamp) - base;
> > +}
>
> Not sure, but it seems to me this implementation does not follow the
> last changes to this kind of interface. We should implement
> get_timer_masked(), but then get_timer is simply:
>
> ulong get_timer(ulong base)
> {
> return get_timer_masked() - base;
> }
>
> as I see in most SOC. However, Heiko (in CC) knows more deeply this
> stuff as me, and he can better explain us if this implementation is
> correct.
>
> I know that other i.MX are look like your implementation, but they are
> quite old...
I'll probably wait for someone to clearly say how this should be to avoid
reworking it for the fourth time.
[...]
> Best regards,
> Stefano Babic
Cheers
_______________________________________________
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
