From: Marek Vasut <marek.vasut@gmail.com>
To: u-boot@lists.denx.de
Subject: [U-Boot] [PATCH 01/31] iMX28: Initial support for iMX28 CPU
Date: Fri, 9 Sep 2011 12:12:08 +0200 [thread overview]
Message-ID: <201109091212.09015.marek.vasut@gmail.com> (raw)
In-Reply-To: <4E69CCED.8050504@denx.de>
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.vasut@gmail.com>
> > Cc: Stefano Babic <sbabic@denx.de>
> > Cc: Wolfgang Denk <wd@denx.de>
> > Cc: Detlev Zundel <dzu@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
next prev parent reply other threads:[~2011-09-09 10:12 UTC|newest]
Thread overview: 68+ messages / expand[flat|nested] mbox.gz Atom feed top
2011-09-08 20:42 [U-Boot] [PATCH 00/31] Support for the DENX M28 SoM Marek Vasut
2011-09-08 20:42 ` [U-Boot] [PATCH 01/31] iMX28: Initial support for iMX28 CPU Marek Vasut
2011-09-09 8:23 ` Stefano Babic
2011-09-09 8:54 ` Heiko Schocher
2011-09-09 10:12 ` Marek Vasut [this message]
2011-09-09 10:29 ` Stefano Babic
2011-09-09 10:50 ` Marek Vasut
2011-09-09 11:04 ` Stefano Babic
2011-09-08 20:42 ` [U-Boot] [PATCH 02/31] iMX28: Add basic support for DENX M28EVK board Marek Vasut
2011-09-09 7:13 ` Igor Grinberg
2011-09-09 8:26 ` Stefano Babic
2011-09-08 20:42 ` [U-Boot] [PATCH 03/31] iMX28: Add support for SSP MMC Marek Vasut
2011-09-08 20:42 ` [U-Boot] [PATCH 04/31] M28EVK: Enable MMC support Marek Vasut
2011-09-09 8:28 ` Stefano Babic
2011-09-08 20:42 ` [U-Boot] [PATCH 05/31] iMX28: Add pinctrl and ocotp register definitions Marek Vasut
2011-09-09 8:30 ` Stefano Babic
2011-09-08 20:42 ` [U-Boot] [PATCH 06/31] FEC: Add support for iMX28 quirks Marek Vasut
2011-09-08 20:42 ` [U-Boot] [PATCH 07/31] iMX28: Add CPU-specific FEC ethernet init Marek Vasut
2011-09-08 20:42 ` [U-Boot] [PATCH 08/31] M28EVK: Enable FEC0 and FEC1 Marek Vasut
2011-09-08 20:42 ` [U-Boot] [PATCH 09/31] iMX28: Add PINMUX control Marek Vasut
2011-09-08 20:42 ` [U-Boot] [PATCH 10/31] iMX28: Add RTC register definitions Marek Vasut
2011-09-08 20:42 ` [U-Boot] [PATCH 11/31] iMX28: Add I2C " Marek Vasut
2011-09-09 5:44 ` Heiko Schocher
2011-09-08 20:42 ` [U-Boot] [PATCH 12/31] iMX28: Add I2C bus driver Marek Vasut
2011-09-09 5:45 ` Heiko Schocher
2011-09-09 11:18 ` Marek Vasut
2011-09-09 11:26 ` Stefan Roese
2011-09-09 11:52 ` Marek Vasut
2011-09-08 20:42 ` [U-Boot] [PATCH 13/31] M28: Enable I2C, EEPROM and RTC Marek Vasut
2011-09-09 5:45 ` Heiko Schocher
2011-09-08 20:42 ` [U-Boot] [PATCH 14/31] iMX28: Add MMC SPL Marek Vasut
2011-09-09 5:44 ` Chander Kashyap
2011-09-09 7:33 ` Wolfgang Denk
[not found] ` <CANuQgHH_p-_HRf_jBKiPZz=br0+aCyWROWzHDZgV47xWg97aiQ@mail.gmail.com>
2011-09-09 12:49 ` Marek Vasut
2011-09-09 13:14 ` Chander Kashyap
2011-09-10 20:58 ` Marek Vasut
2011-09-10 21:04 ` Marek Vasut
2011-09-08 20:42 ` [U-Boot] [PATCH 15/31] M28: Enable " Marek Vasut
2011-09-08 20:42 ` [U-Boot] [PATCH 16/31] iMX28: Add GPIO control Marek Vasut
2011-09-09 8:46 ` Stefano Babic
2011-09-08 20:42 ` [U-Boot] [PATCH 17/31] M28: Enable use of GPIO configurators Marek Vasut
2011-09-08 20:42 ` [U-Boot] [PATCH 18/31] iMX28: Add SPI driver Marek Vasut
2011-09-09 8:51 ` Stefano Babic
2011-09-09 11:50 ` Marek Vasut
2011-09-09 12:34 ` Stefano Babic
2011-09-09 12:50 ` Marek Vasut
2011-09-08 20:42 ` [U-Boot] [PATCH 19/31] M28EVK: Enable SPI and SPI flash Marek Vasut
2011-09-08 20:42 ` [U-Boot] [PATCH 20/31] iMX28: Add APBH DMA driver Marek Vasut
2011-09-08 20:42 ` [U-Boot] [PATCH 21/31] iMX28: Add BCH and GPMI register definitions Marek Vasut
2011-09-08 20:42 ` [U-Boot] [PATCH 22/31] iMX28: Add GPMI NAND driver Marek Vasut
2011-09-08 20:42 ` [U-Boot] [PATCH 23/31] M28: Enable NAND Marek Vasut
2011-09-08 20:42 ` [U-Boot] [PATCH 24/31] iMX28: Add driver for internal RTC Marek Vasut
2011-09-08 20:42 ` [U-Boot] [PATCH 25/31] M28: Enable the internal RTC instead of the M41T62 Marek Vasut
2011-09-09 12:45 ` Stefano Babic
2011-09-09 12:51 ` Marek Vasut
2011-09-09 12:55 ` Stefano Babic
2011-09-08 20:42 ` [U-Boot] [PATCH 26/31] M28: Enable UBI and UBIFS Marek Vasut
2011-09-08 20:42 ` [U-Boot] [PATCH 27/31] M28: Save environment in NAND Marek Vasut
2011-09-16 19:15 ` Scott Wood
2011-09-17 8:20 ` Marek Vasut
2011-09-19 17:54 ` Scott Wood
2011-09-17 7:10 ` Fabio Estevam
2011-09-08 20:42 ` [U-Boot] [PATCH 28/31] iMX28: Add image header generator tool Marek Vasut
2011-09-09 8:58 ` Stefano Babic
2011-09-09 11:51 ` Marek Vasut
2011-09-08 20:42 ` [U-Boot] [PATCH 29/31] M28: Add NAND update scripts Marek Vasut
2011-09-08 20:42 ` [U-Boot] [PATCH 30/31] i.MX28: Add u-boot.sb target to Makefile Marek Vasut
2011-09-08 20:42 ` [U-Boot] [PATCH 31/31] M28: Add doc/README.m28 documentation Marek Vasut
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=201109091212.09015.marek.vasut@gmail.com \
--to=marek.vasut@gmail.com \
--cc=u-boot@lists.denx.de \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox