Re: [U-Boot] [PATCH v3 1/4] arm: Tegra2: Add basic NVIDIA Tegra2 SoC support
Dear Tom Warren, In message you wrote: > > >> >> +#define NV_PA_APB_UARTD_BASE (NV_PA_APB_MISC_BASE + 0x6300) > >> >> +#define NV_PA_APB_UARTE_BASE (NV_PA_APB_MISC_BASE + 0x6400) > >> >> +#define NV_PA_PMC_BASE 0x7000E400 > >> > > >> > what is the purpose of NV_PA prefix here? > >> NV_Physical_Address - a base address of a HW block (Power Management > >> Cntrlr, etc.) > > > > Well, the NV_ part is not needed, right? > True. I can remove it, but why? It designates this as a > NVIDIA-specific define. I see the same thing > in AT91, OMAP, NetARM, DaVinci, IMX files, etc. etc. OK, I don't insist. Best regards, Wolfgang Denk -- DENX Software Engineering GmbH, MD: Wolfgang Denk & Detlev Zundel HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: w...@denx.de A weak mind is like a microscope, which magnifies trifling things, but cannot receive great ones. -- Philip Earl of Chesterfield ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH v3 1/4] arm: Tegra2: Add basic NVIDIA Tegra2 SoC support
Wolfgang (& Mike), On Mon, Jan 24, 2011 at 12:00 PM, Wolfgang Denk wrote: > Dear Tom Warren, > > In message you > wrote: >> > ... >> >> +#define NV_PA_APB_UARTD_BASE (NV_PA_APB_MISC_BASE + 0x6300) >> >> +#define NV_PA_APB_UARTE_BASE (NV_PA_APB_MISC_BASE + 0x6400) >> >> +#define NV_PA_PMC_BASE 0x7000E400 >> > >> > what is the purpose of NV_PA prefix here? >> NV_Physical_Address - a base address of a HW block (Power Management >> Cntrlr, etc.) > > Well, the NV_ part is not needed, right? True. I can remove it, but why? It designates this as a NVIDIA-specific define. I see the same thing in AT91, OMAP, NetARM, DaVinci, IMX files, etc. etc. > > > Best regards, > > Wolfgang Denk Thanks, Tom > > -- > DENX Software Engineering GmbH, MD: Wolfgang Denk & Detlev Zundel > HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany > Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: w...@denx.de > Writing a book is like washing an elephant: there's no good place to > begin or end, and it's hard to keep track of what you've already > covered. > ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH v3 1/4] arm: Tegra2: Add basic NVIDIA Tegra2 SoC support
Dear Tom Warren, In message you wrote: > ... > >> +#define NV_PA_APB_UARTD_BASE (NV_PA_APB_MISC_BASE + 0x6300) > >> +#define NV_PA_APB_UARTE_BASE (NV_PA_APB_MISC_BASE + 0x6400) > >> +#define NV_PA_PMC_BASE 0x7000E400 > > > > what is the purpose of NV_PA prefix here? > NV_Physical_Address - a base address of a HW block (Power Management > Cntrlr, etc.) Well, the NV_ part is not needed, right? Best regards, Wolfgang Denk -- DENX Software Engineering GmbH, MD: Wolfgang Denk & Detlev Zundel HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: w...@denx.de Writing a book is like washing an elephant: there's no good place to begin or end, and it's hard to keep track of what you've already covered. ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH v3 1/4] arm: Tegra2: Add basic NVIDIA Tegra2 SoC support
Dear Mike Rapoport, In message <4d3d68a9.4040...@compulab.co.il> you wrote: > > Besides, since you're using I/O accessors anyway, the struct can replaces with > base address and offset definitions. We do not allow such construtcs in U-Boot. With C structs, you can have proper type checking by the compiler (well, at least assuming you have proper I/O accessors in place). > > +#define NV_PA_APB_UARTC_BASE (NV_PA_APB_MISC_BASE + 0x6200) > > +#define NV_PA_APB_UARTD_BASE (NV_PA_APB_MISC_BASE + 0x6300) > > +#define NV_PA_APB_UARTE_BASE (NV_PA_APB_MISC_BASE + 0x6400) > > +#define NV_PA_PMC_BASE 0x7000E400 > > what is the purpose of NV_PA prefix here? Good catch. Best regards, Wolfgang Denk -- DENX Software Engineering GmbH, MD: Wolfgang Denk & Detlev Zundel HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: w...@denx.de When the bosses talk about improving productivity, they are never talking about themselves. ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH v3 1/4] arm: Tegra2: Add basic NVIDIA Tegra2 SoC support
Mike,
On Mon, Jan 24, 2011 at 4:55 AM, Mike Rapoport wrote:
> On 01/19/11 23:19, Tom Warren wrote:
>> Signed-off-by: Tom Warren
>> ---
>> Changes for V2:
>> - Coding style cleanup
>> - Move serial driver changes to separate patch
>> - Use board/nvidia/ instead of /board/tegra
>> - Remove TRUE/FALSE defines
>> - Use standard NS16550 register/bit defines in UART init
>>
>> Changes for V3:
>> - Use I/O accessors for Tegra2 HW MMIO register access
>> - Allow conditional compile of UARTA/UARTD code to save space
>>
>> arch/arm/cpu/armv7/tegra2/Makefile | 48 +
>> arch/arm/cpu/armv7/tegra2/board.c | 91 ++
>> arch/arm/cpu/armv7/tegra2/config.mk | 28 +++
>> arch/arm/cpu/armv7/tegra2/lowlevel_init.S | 66 +++
>> arch/arm/cpu/armv7/tegra2/sys_info.c | 35
>> arch/arm/cpu/armv7/tegra2/timer.c | 122 +
>> arch/arm/include/asm/arch-tegra2/clk_rst.h | 155
>> arch/arm/include/asm/arch-tegra2/pinmux.h | 52 ++
>> arch/arm/include/asm/arch-tegra2/pmc.h | 125 +
>> arch/arm/include/asm/arch-tegra2/sys_proto.h | 33
>> arch/arm/include/asm/arch-tegra2/tegra2.h | 49 +
>> arch/arm/include/asm/arch-tegra2/uart.h | 45 +
>> board/nvidia/common/board.c | 249
>> ++
>> board/nvidia/common/board.h | 57 ++
>> 14 files changed, 1155 insertions(+), 0 deletions(-)
>> create mode 100644 arch/arm/cpu/armv7/tegra2/Makefile
>> create mode 100644 arch/arm/cpu/armv7/tegra2/board.c
>> create mode 100644 arch/arm/cpu/armv7/tegra2/config.mk
>> create mode 100644 arch/arm/cpu/armv7/tegra2/lowlevel_init.S
>> create mode 100644 arch/arm/cpu/armv7/tegra2/sys_info.c
>> create mode 100644 arch/arm/cpu/armv7/tegra2/timer.c
>> create mode 100644 arch/arm/include/asm/arch-tegra2/clk_rst.h
>> create mode 100644 arch/arm/include/asm/arch-tegra2/pinmux.h
>> create mode 100644 arch/arm/include/asm/arch-tegra2/pmc.h
>> create mode 100644 arch/arm/include/asm/arch-tegra2/sys_proto.h
>> create mode 100644 arch/arm/include/asm/arch-tegra2/tegra2.h
>> create mode 100644 arch/arm/include/asm/arch-tegra2/uart.h
>> create mode 100644 board/nvidia/common/board.c
>> create mode 100644 board/nvidia/common/board.h
>
> [ snip ]
>
>> + */
>> +
>> +#ifndef _CLK_RST_H_
>> +#define _CLK_RST_H_
>> +
>> +/* Clock/Reset Controller (CLK_RST_CONTROLLER_) regs */
>> +
>> +typedef volatile struct clk_rst_ctlr {
>
> Is it necessary to use the structure to map the clocks and reset controller?
> Wouldn't be better to port Linux implementation of Tegra2 clocks to U-Boot as
> well?
> Besides, since you're using I/O accessors anyway, the struct can replaces with
> base address and offset definitions.
I asked Wolfgang to pre-review the original patch, and this is what he
said about original
base+offset register access code:
Wolfgang> We do not allow this in U-Boot. Please turn all offset
tables into C structs, and
Wolfgang> create a set of I/O accessor functions (or macros) as needed
to provide the needed
Wolfgang> memory barriers on your architecture.
Using structs seems like a natural way to map HW MMIO regs, and is
done throughout U-Boot.
The structs are already written, contain just the members needed for
U-Boot (to a large degree),
and as Wolfgang has said in the past, U-Boot is not Linux, so I see no
reason to bring in the
Linux Tegra2 structs for any of these HW blocks. When I start posting
the drivers (SPI, USB,
etc.), then it might make sense to use (copy w/edits) the Linux data
structs, etc.
>
>> + uint crc_rst_src; /* _RST_SOURCE_0, 0x00*/
>> + uint crc_rst_dev_l; /* _RST_DEVICES_L_0, 0x04*/
>> + uint crc_rst_dev_h; /* _RST_DEVICES_H_0, 0x08*/
>> + uint crc_rst_dev_u; /* _RST_DEVICES_U_0, 0x0C*/
>> + uint crc_clk_out_enb_l; /* _CLK_OUT_ENB_L_0, 0x10*/
>> + uint crc_clk_out_enb_h; /* _CLK_OUT_ENB_H_0, 0x14*/
>
> [ snip ]
>
>> +
>> +#ifndef _PINMUX_H_
>> +#define _PINMUX_H_
>> +
>> +/* APB MISC Pin Mux and Tristate (APB_MISC_PP_) registers */
>> +
>> +typedef volatile struct pinmux_tri_ctlr {
>
> The same comment is valid also for the pin multiplexing registers...
>
>> + uint pmt_reserved0; /* ABP_MISC_PP_ reserved offset 00 */
>> + uint pmt_reserved1; /* ABP_MISC_PP_ reserved offset 04 */
>> + uint pmt_strap_opt_a; /* _STRAPPING_OPT_A_0, offset 08 */
>> +
>> +#ifndef _PMC_H_
>> +#define _PMC_H_
>> +
>> +/* Power Management Controller (APBDEV_PMC_) registers */
>> +
>> +typedef volatile struct pmc_ctlr {
>
> And for the PMC registers as well.
>
>> + uint pmc_cntrl; /* _CNTRL_0, offset 00 */
>> + uint pmc_sec_disable; /* _SEC_DISABLE_0, offset 04 */
>> + uint pmc_pmc_swrst;
Re: [U-Boot] [PATCH v3 1/4] arm: Tegra2: Add basic NVIDIA Tegra2 SoC support
On 01/19/11 23:19, Tom Warren wrote:
> Signed-off-by: Tom Warren
> ---
> Changes for V2:
> - Coding style cleanup
> - Move serial driver changes to separate patch
> - Use board/nvidia/ instead of /board/tegra
> - Remove TRUE/FALSE defines
> - Use standard NS16550 register/bit defines in UART init
>
> Changes for V3:
> - Use I/O accessors for Tegra2 HW MMIO register access
> - Allow conditional compile of UARTA/UARTD code to save space
>
> arch/arm/cpu/armv7/tegra2/Makefile | 48 +
> arch/arm/cpu/armv7/tegra2/board.c| 91 ++
> arch/arm/cpu/armv7/tegra2/config.mk | 28 +++
> arch/arm/cpu/armv7/tegra2/lowlevel_init.S| 66 +++
> arch/arm/cpu/armv7/tegra2/sys_info.c | 35
> arch/arm/cpu/armv7/tegra2/timer.c| 122 +
> arch/arm/include/asm/arch-tegra2/clk_rst.h | 155
> arch/arm/include/asm/arch-tegra2/pinmux.h| 52 ++
> arch/arm/include/asm/arch-tegra2/pmc.h | 125 +
> arch/arm/include/asm/arch-tegra2/sys_proto.h | 33
> arch/arm/include/asm/arch-tegra2/tegra2.h| 49 +
> arch/arm/include/asm/arch-tegra2/uart.h | 45 +
> board/nvidia/common/board.c | 249
> ++
> board/nvidia/common/board.h | 57 ++
> 14 files changed, 1155 insertions(+), 0 deletions(-)
> create mode 100644 arch/arm/cpu/armv7/tegra2/Makefile
> create mode 100644 arch/arm/cpu/armv7/tegra2/board.c
> create mode 100644 arch/arm/cpu/armv7/tegra2/config.mk
> create mode 100644 arch/arm/cpu/armv7/tegra2/lowlevel_init.S
> create mode 100644 arch/arm/cpu/armv7/tegra2/sys_info.c
> create mode 100644 arch/arm/cpu/armv7/tegra2/timer.c
> create mode 100644 arch/arm/include/asm/arch-tegra2/clk_rst.h
> create mode 100644 arch/arm/include/asm/arch-tegra2/pinmux.h
> create mode 100644 arch/arm/include/asm/arch-tegra2/pmc.h
> create mode 100644 arch/arm/include/asm/arch-tegra2/sys_proto.h
> create mode 100644 arch/arm/include/asm/arch-tegra2/tegra2.h
> create mode 100644 arch/arm/include/asm/arch-tegra2/uart.h
> create mode 100644 board/nvidia/common/board.c
> create mode 100644 board/nvidia/common/board.h
[ snip ]
> + */
> +
> +#ifndef _CLK_RST_H_
> +#define _CLK_RST_H_
> +
> +/* Clock/Reset Controller (CLK_RST_CONTROLLER_) regs */
> +
> +typedef volatile struct clk_rst_ctlr {
Is it necessary to use the structure to map the clocks and reset controller?
Wouldn't be better to port Linux implementation of Tegra2 clocks to U-Boot as
well?
Besides, since you're using I/O accessors anyway, the struct can replaces with
base address and offset definitions.
> + uint crc_rst_src; /* _RST_SOURCE_0, 0x00*/
> + uint crc_rst_dev_l; /* _RST_DEVICES_L_0,0x04*/
> + uint crc_rst_dev_h; /* _RST_DEVICES_H_0,0x08*/
> + uint crc_rst_dev_u; /* _RST_DEVICES_U_0,0x0C*/
> + uint crc_clk_out_enb_l; /* _CLK_OUT_ENB_L_0,0x10*/
> + uint crc_clk_out_enb_h; /* _CLK_OUT_ENB_H_0,0x14*/
[ snip ]
> +
> +#ifndef _PINMUX_H_
> +#define _PINMUX_H_
> +
> +/* APB MISC Pin Mux and Tristate (APB_MISC_PP_) registers */
> +
> +typedef volatile struct pinmux_tri_ctlr {
The same comment is valid also for the pin multiplexing registers...
> + uint pmt_reserved0; /* ABP_MISC_PP_ reserved offset 00 */
> + uint pmt_reserved1; /* ABP_MISC_PP_ reserved offset 04 */
> + uint pmt_strap_opt_a; /* _STRAPPING_OPT_A_0, offset 08 */
> +
> +#ifndef _PMC_H_
> +#define _PMC_H_
> +
> +/* Power Management Controller (APBDEV_PMC_) registers */
> +
> +typedef volatile struct pmc_ctlr {
And for the PMC registers as well.
> + uint pmc_cntrl; /* _CNTRL_0, offset 00 */
> + uint pmc_sec_disable; /* _SEC_DISABLE_0, offset 04 */
> + uint pmc_pmc_swrst; /* _PMC_SWRST_0, offset 08 */
> + uint pmc_wake_mask; /* _WAKE_MASK_0, offset 0C */
> + uint pmc_wake_lvl; /* _WAKE_LVL_0, offset 10 */
[ snip ]
> +#ifndef _TEGRA2_H_
> +#define _TEGRA2_H_
> +
> +#define NV_PA_SDRAM_BASE 0x
> +#define NV_PA_TMRUS_BASE 0x60005010
> +#define NV_PA_CLK_RST_BASE 0x60006000
> +#define NV_PA_APB_MISC_BASE 0x7000
> +#define NV_PA_APB_UARTA_BASE (NV_PA_APB_MISC_BASE + 0x6000)
> +#define NV_PA_APB_UARTB_BASE (NV_PA_APB_MISC_BASE + 0x6040)
> +#define NV_PA_APB_UARTC_BASE (NV_PA_APB_MISC_BASE + 0x6200)
> +#define NV_PA_APB_UARTD_BASE (NV_PA_APB_MISC_BASE + 0x6300)
> +#define NV_PA_APB_UARTE_BASE (NV_PA_APB_MISC_BASE + 0x6400)
> +#define NV_PA_PMC_BASE 0x7000E400
what is the purpose of NV_PA prefix here?
> +#define TEGRA2_SDRC_CS0 NV_PA_SDRAM_BASE
> +#define LOW_LEVEL_SRAM_STACK 0x4000FFFC
> +
> +#ifndef __ASSEMBLY__
> +typedef volatile struct timerus {
Re: [U-Boot] [PATCH v3 1/4] arm: Tegra2: Add basic NVIDIA Tegra2 SoC support
On Fri, Jan 21, 2011 at 9:50 AM, Wolfgang Denk wrote:
> Dear Tom Warren,
>
> In message you
> wrote:
>>
>> I'll take a look at the ARM asm code generated, but you are probably right.
>> But shouldn't the compiler have complained if I wasn't passing the
>> struct address?
>
> I'm surprised about this, too. But then, current mainline code still
> has the horrible "(*(volatile unsigned int *)(a) = (v))" definition,
> so the cast will eat all potential warnings :-(
>
Yes, I noticed this with x86 - I can do something like the following
without the compiler warning me:
typdef struct blah {
u32 foo;
u16 bar;
} blah_t;
blah_t *fred = 0x1000;
writel(1, &fred->foo);
writel(1, &fred->bar);
writew(1, &fred->foo);
writew(1, &fred->bar);
This is particularly nasty with the sc520's Memory Mapped Control
Registers - I have found a few here and there where longs were being
written to words and visa-versa
Regards,
Graeme
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH v3 1/4] arm: Tegra2: Add basic NVIDIA Tegra2 SoC support
Dear Tom Warren, In message you wrote: > > I run checkpatch.pl (v 0.31) on every patch before I submit it, and I > did see 12 warnings but > no errors. The warnings were minor - new typedefs and volatile > structs. Could you please > provide the text of the checkpatch.pl output so I can see what the > errors might be? Sorry for the false alarms, I was using an older version of checkpatch. Best regards, Wolfgang Denk -- DENX Software Engineering GmbH, MD: Wolfgang Denk & Detlev Zundel HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: w...@denx.de It took him several minutes to understand any new idea put to him, and this is a very valuable trait in a leader, because anything any- one is still trying to explain to you after two minutes is probably important and anything they give up after a mere minute or so is al- most certainly something they shouldn't have been bothering you with in the first place. - Terry Pratchett, _Reaper Man_ ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH v3 1/4] arm: Tegra2: Add basic NVIDIA Tegra2 SoC support
Dear Tom Warren, In message you wrote: > > I'll take a look at the ARM asm code generated, but you are probably right. > But shouldn't the compiler have complained if I wasn't passing the > struct address? I'm surprised about this, too. But then, current mainline code still has the horrible "(*(volatile unsigned int *)(a) = (v))" definition, so the cast will eat all potential warnings :-( Best regards, Wolfgang Denk -- DENX Software Engineering GmbH, MD: Wolfgang Denk & Detlev Zundel HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: w...@denx.de "...all the good computer designs are bootlegged; the formally planned products, if they are built at all, are dogs!" - David E. Lundstrom, "A Few Good Men From Univac", MIT Press, 1987 ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH v3 1/4] arm: Tegra2: Add basic NVIDIA Tegra2 SoC support
Dear Tom Warren, In message you wrote: ... > > Are all these uart functions board-specific? They look more > > CPU-specific. If that's the case they should be moved somewhere in > > arch/arm/*. Other boards that use the Tegra2 don't want to duplicate > > this code or link into Nvidia's board/nvidia directory. > It's Tegra2 SoC-specific - that's not the CPU, per se. I guess I could move > it to arch/arm/cpu/armv7/tegra2, if you think it's important enough. Yes, please do. Best regards, Wolfgang Denk -- DENX Software Engineering GmbH, MD: Wolfgang Denk & Detlev Zundel HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: w...@denx.de Q: How many DEC repairman does it take to fix a flat ? A: Five; four to hold the car up and one to swap tires. ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH v3 1/4] arm: Tegra2: Add basic NVIDIA Tegra2 SoC support
On Thu, 2011-01-20 at 09:41 -0700, Tom Warren wrote:
> On Wed, Jan 19, 2011 at 5:04 PM, Peter Tyser wrote:
> > Hi Tom,
> > Some last minutes nits:
> >
> > It looks like some of the new functions can be declared statically.
> > It'd be nice to do so where possible.
> Which functions, Peter? Please point them out specifically, thanks.
Any function that won't be called from outside the scope of the file.
Eg it looks like init_uart() and setup_uart() are local functions and
should be static. Those are the 2 that jumped out initially, but you
should review to see if there are others.
> >> +/*
> >> + * Routine: uart_clock_init
> >> + * Description: init the PLL and clock for the UART in uart_num
> >> + */
> >> +void uart_clock_init(int uart_num)
> >> +{
> >
> > Are all these uart functions board-specific? They look more
> > CPU-specific. If that's the case they should be moved somewhere in
> > arch/arm/*. Other boards that use the Tegra2 don't want to duplicate
> > this code or link into Nvidia's board/nvidia directory.
> It's Tegra2 SoC-specific - that's not the CPU, per se. I guess I could move
> it to arch/arm/cpu/armv7/tegra2, if you think it's important enough.
I think they should be moved. If they aren't, the next board vendor (eg
my company) that uses the Tegra2 will copy your board.[ch] into their
board/ directory and use them as a starting point, which is a
large duplication of code. Moving it somewhere in arch/arm is the
"right" thing to do and will make every future tegra2 board port cleaner
and easier.
Best,
Peter
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH v3 1/4] arm: Tegra2: Add basic NVIDIA Tegra2 SoC support
Wolfgang, On Thu, Jan 20, 2011 at 1:40 AM, Wolfgang Denk wrote: > Dear Tom Warren, > > In message <1295471986-2395-2-git-send-email-twar...@nvidia.com> you wrote: >> Signed-off-by: Tom Warren > > checkpatch.pl reports: > > total: 6 errors, 12 warnings, 1155 lines checked > > /tmp/patch has style problems, please review. > > Please clean up. I run checkpatch.pl (v 0.31) on every patch before I submit it, and I did see 12 warnings but no errors. The warnings were minor - new typedefs and volatile structs. Could you please provide the text of the checkpatch.pl output so I can see what the errors might be? Thanks. > > Best regards, > > Wolfgang Denk > > -- > DENX Software Engineering GmbH, MD: Wolfgang Denk & Detlev Zundel > HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany > Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: w...@denx.de > "Where shall I begin, please your Majesty?" he asked. "Begin at the > beginning," the King said, gravely, "and go on till you come to the > end: then stop." - Alice's Adventures in Wonderland, Lewis Carroll > ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH v3 1/4] arm: Tegra2: Add basic NVIDIA Tegra2 SoC support
Graeme,
On Wed, Jan 19, 2011 at 5:20 PM, Graeme Russ wrote:
> On Thu, Jan 20, 2011 at 8:19 AM, Tom Warren wrote:
>
>> +
>> +/*
>> + * Routine: uart_clock_init
>> + * Description: init the PLL and clock for the UART in uart_num
>> + */
>> +void uart_clock_init(int uart_num)
>> +{
>> + clk_rst_ctlr *const clkrst = (clk_rst_ctlr *)NV_PA_CLK_RST_BASE;
>> + static int pllp_init_done;
>> + u32 reg;
>> +
>> + if (!pllp_init_done) {
>> +
>> + /* Override pllp setup for 216MHz operation. */
>> + reg = (PLL_BYPASS | PLL_BASE_OVRRIDE | PLL_DIVP);
>> + reg |= (((NVRM_PLLP_FIXED_FREQ_KHZ/500) << 8) | PLL_DIVM);
>> + writel(reg, clkrst->crc_pllp_base);
>> +
>> + reg |= PLL_ENABLE;
>> + writel(reg, clkrst->crc_pllp_base);
>
> Is this correct? Should it not be writel(reg, &clkrst->crc_pllp_base);
Well, the PLLs, UART and device clocks that I'm writing all seem to work OK.
I'll take a look at the ARM asm code generated, but you are probably right.
But shouldn't the compiler have complained if I wasn't passing the
struct address?
>
> Similarly for other readl()'s and writel()'s
>
> Regards,
>
> Graeme
>
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH v3 1/4] arm: Tegra2: Add basic NVIDIA Tegra2 SoC support
On Wed, Jan 19, 2011 at 5:04 PM, Peter Tyser wrote:
> Hi Tom,
> Some last minutes nits:
>
> It looks like some of the new functions can be declared statically.
> It'd be nice to do so where possible.
Which functions, Peter? Please point them out specifically, thanks.
>
>
>
>> --- /dev/null
>> +++ b/arch/arm/cpu/armv7/tegra2/lowlevel_init.S
>> @@ -0,0 +1,66 @@
>> +/*
>> + * Board specific setup info
>
> This is CPU-specific code, correct?
Yes - I'll change the comment.
>
>> + *
>> + * (C) Copyright 2010,2011
>> + * NVIDIA Corporation
>> + *
>> + * See file CREDITS for list of people who contributed to this
>> + * project.
>> + *
>> + * This program is free software; you can redistribute it and/or
>> + * modify it under the terms of the GNU General Public License as
>> + * published by the Free Software Foundation; either version 2 of
>> + * the License, or (at your option) any later version.
>> + *
>> + * This program is distributed in the hope that 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
>> + */
>> +
>> +#include
>> +#include
>> +
>> +_TEXT_BASE:
>> + .word CONFIG_SYS_TEXT_BASE @ sdram load addr from config file
>> +
>> +.global invalidate_dcache
>> +invalidate_dcache:
>> + mov pc, lr
>> +
>> +
>> + .align 5
>> +.global reset_cpu
>> +reset_cpu:
>> + ldr r1, rstctl @ get addr for global reset
>> + @ reg
>> + ldr r3, [r1]
>> + orr r3, r3, #0x10
>> + str r3, [r1] @ force reset
>> + mov r0, r0
>> +_loop_forever:
>> + b _loop_forever
>> +rstctl:
>> + .word PRM_RSTCTRL
>> +
>> +.globl lowlevel_init
>> +lowlevel_init:
>> + ldr sp, SRAM_STACK
>> + str ip, [sp]
>> + mov ip, lr
>> + bl s_init @ go setup pll, mux & memory
>> + ldr ip, [sp]
>> + mov lr, ip
>> +
>> + mov pc, lr @ back to arch calling code
>> +
>> + @ the literal pools origin
>> + .ltorg
>> +
>> +SRAM_STACK:
>> + .word LOW_LEVEL_SRAM_STACK
>> diff --git a/arch/arm/cpu/armv7/tegra2/sys_info.c
>> b/arch/arm/cpu/armv7/tegra2/sys_info.c
>> new file mode 100644
>> index 000..6d11dc1
>> --- /dev/null
>> +++ b/arch/arm/cpu/armv7/tegra2/sys_info.c
>> @@ -0,0 +1,35 @@
>> +/*
>> + * (C) Copyright 2010,2011
>> + * NVIDIA Corporation
>> + *
>> + * See file CREDITS for list of people who contributed to this
>> + * project.
>> + *
>> + * This program is free software; you can redistribute it and/or
>> + * modify it under the terms of the GNU General Public License as
>> + * published by the Free Software Foundation; either version 2 of
>> + * the License, or (at your option) any later version.
>> + *
>> + * This program is distributed in the hope that 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
>> + */
>> +
>> +#include
>> +
>> +#ifdef CONFIG_DISPLAY_CPUINFO
>> +/* Print CPU information */
>> +int print_cpuinfo(void)
>> +{
>> + puts("TEGRA2\n");
>> +
>> + /* TBD: Add printf of major/minor rev info, stepping, etc. */
>> + return 0;
>> +}
>> +#endif /* CONFIG_DISPLAY_CPUINFO */
>> diff --git a/arch/arm/cpu/armv7/tegra2/timer.c
>> b/arch/arm/cpu/armv7/tegra2/timer.c
>> new file mode 100644
>> index 000..858af0f
>> --- /dev/null
>> +++ b/arch/arm/cpu/armv7/tegra2/timer.c
>> @@ -0,0 +1,122 @@
>> +/*
>> + * (C) Copyright 2010,2011
>> + * NVIDIA Corporation
>> + *
>> + * (C) Copyright 2008
>> + * Texas Instruments
>> + *
>> + * Richard Woodruff
>> + * Syed Moahmmed Khasim
>> + *
>> + * (C) Copyright 2002
>> + * Sysgo Real-Time Solutions, GmbH
>> + * Marius Groeger
>> + * Alex Zuepke
>> + *
>> + * (C) Copyright 2002
>> + * Gary Jennejohn, DENX Software Engineering,
>> + *
>> + * See file CREDITS for list of people who contributed to this
>> + * project.
>> + *
>> + * This program is free software; you can redistribute it and/or
>> + * modify it under the terms of the GNU General Public License as
>> + * published by the Free Software Foundation; either version 2 of
>> + * the License, or (at your option) any later version.
>> + *
>> + * This program is distributed in th
Re: [U-Boot] [PATCH v3 1/4] arm: Tegra2: Add basic NVIDIA Tegra2 SoC support
Dear Tom Warren, In message <1295471986-2395-2-git-send-email-twar...@nvidia.com> you wrote: > Signed-off-by: Tom Warren checkpatch.pl reports: total: 6 errors, 12 warnings, 1155 lines checked /tmp/patch has style problems, please review. Please clean up. Best regards, Wolfgang Denk -- DENX Software Engineering GmbH, MD: Wolfgang Denk & Detlev Zundel HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: w...@denx.de "Where shall I begin, please your Majesty?" he asked. "Begin at the beginning," the King said, gravely, "and go on till you come to the end: then stop."- Alice's Adventures in Wonderland, Lewis Carroll ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH v3 1/4] arm: Tegra2: Add basic NVIDIA Tegra2 SoC support
On Thu, Jan 20, 2011 at 8:19 AM, Tom Warren wrote:
> +
> +/*
> + * Routine: uart_clock_init
> + * Description: init the PLL and clock for the UART in uart_num
> + */
> +void uart_clock_init(int uart_num)
> +{
> + clk_rst_ctlr *const clkrst = (clk_rst_ctlr *)NV_PA_CLK_RST_BASE;
> + static int pllp_init_done;
> + u32 reg;
> +
> + if (!pllp_init_done) {
> +
> + /* Override pllp setup for 216MHz operation. */
> + reg = (PLL_BYPASS | PLL_BASE_OVRRIDE | PLL_DIVP);
> + reg |= (((NVRM_PLLP_FIXED_FREQ_KHZ/500) << 8) | PLL_DIVM);
> + writel(reg, clkrst->crc_pllp_base);
> +
> + reg |= PLL_ENABLE;
> + writel(reg, clkrst->crc_pllp_base);
Is this correct? Should it not be writel(reg, &clkrst->crc_pllp_base);
Similarly for other readl()'s and writel()'s
Regards,
Graeme
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH v3 1/4] arm: Tegra2: Add basic NVIDIA Tegra2 SoC support
Hi Tom,
Some last minutes nits:
It looks like some of the new functions can be declared statically.
It'd be nice to do so where possible.
> --- /dev/null
> +++ b/arch/arm/cpu/armv7/tegra2/lowlevel_init.S
> @@ -0,0 +1,66 @@
> +/*
> + * Board specific setup info
This is CPU-specific code, correct?
> + *
> + * (C) Copyright 2010,2011
> + * NVIDIA Corporation
> + *
> + * See file CREDITS for list of people who contributed to this
> + * project.
> + *
> + * This program is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU General Public License as
> + * published by the Free Software Foundation; either version 2 of
> + * the License, or (at your option) any later version.
> + *
> + * This program is distributed in the hope that 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
> + */
> +
> +#include
> +#include
> +
> +_TEXT_BASE:
> + .word CONFIG_SYS_TEXT_BASE@ sdram load addr from config file
> +
> +.global invalidate_dcache
> +invalidate_dcache:
> + mov pc, lr
> +
> +
> + .align 5
> +.global reset_cpu
> +reset_cpu:
> + ldr r1, rstctl @ get addr for global reset
> + @ reg
> + ldr r3, [r1]
> + orr r3, r3, #0x10
> + str r3, [r1]@ force reset
> + mov r0, r0
> +_loop_forever:
> + b _loop_forever
> +rstctl:
> + .word PRM_RSTCTRL
> +
> +.globl lowlevel_init
> +lowlevel_init:
> + ldr sp, SRAM_STACK
> + str ip, [sp]
> + mov ip, lr
> + bl s_init @ go setup pll, mux & memory
> + ldr ip, [sp]
> + mov lr, ip
> +
> + mov pc, lr @ back to arch calling code
> +
> + @ the literal pools origin
> + .ltorg
> +
> +SRAM_STACK:
> + .word LOW_LEVEL_SRAM_STACK
> diff --git a/arch/arm/cpu/armv7/tegra2/sys_info.c
> b/arch/arm/cpu/armv7/tegra2/sys_info.c
> new file mode 100644
> index 000..6d11dc1
> --- /dev/null
> +++ b/arch/arm/cpu/armv7/tegra2/sys_info.c
> @@ -0,0 +1,35 @@
> +/*
> + * (C) Copyright 2010,2011
> + * NVIDIA Corporation
> + *
> + * See file CREDITS for list of people who contributed to this
> + * project.
> + *
> + * This program is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU General Public License as
> + * published by the Free Software Foundation; either version 2 of
> + * the License, or (at your option) any later version.
> + *
> + * This program is distributed in the hope that 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
> + */
> +
> +#include
> +
> +#ifdef CONFIG_DISPLAY_CPUINFO
> +/* Print CPU information */
> +int print_cpuinfo(void)
> +{
> + puts("TEGRA2\n");
> +
> + /* TBD: Add printf of major/minor rev info, stepping, etc. */
> + return 0;
> +}
> +#endif /* CONFIG_DISPLAY_CPUINFO */
> diff --git a/arch/arm/cpu/armv7/tegra2/timer.c
> b/arch/arm/cpu/armv7/tegra2/timer.c
> new file mode 100644
> index 000..858af0f
> --- /dev/null
> +++ b/arch/arm/cpu/armv7/tegra2/timer.c
> @@ -0,0 +1,122 @@
> +/*
> + * (C) Copyright 2010,2011
> + * NVIDIA Corporation
> + *
> + * (C) Copyright 2008
> + * Texas Instruments
> + *
> + * Richard Woodruff
> + * Syed Moahmmed Khasim
> + *
> + * (C) Copyright 2002
> + * Sysgo Real-Time Solutions, GmbH
> + * Marius Groeger
> + * Alex Zuepke
> + *
> + * (C) Copyright 2002
> + * Gary Jennejohn, DENX Software Engineering,
> + *
> + * See file CREDITS for list of people who contributed to this
> + * project.
> + *
> + * This program is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU General Public License as
> + * published by the Free Software Foundation; either version 2 of
> + * the License, or (at your option) any later version.
> + *
> + * This program is distributed in the hope that 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 th
[U-Boot] [PATCH v3 1/4] arm: Tegra2: Add basic NVIDIA Tegra2 SoC support
Signed-off-by: Tom Warren --- Changes for V2: - Coding style cleanup - Move serial driver changes to separate patch - Use board/nvidia/ instead of /board/tegra - Remove TRUE/FALSE defines - Use standard NS16550 register/bit defines in UART init Changes for V3: - Use I/O accessors for Tegra2 HW MMIO register access - Allow conditional compile of UARTA/UARTD code to save space arch/arm/cpu/armv7/tegra2/Makefile | 48 + arch/arm/cpu/armv7/tegra2/board.c| 91 ++ arch/arm/cpu/armv7/tegra2/config.mk | 28 +++ arch/arm/cpu/armv7/tegra2/lowlevel_init.S| 66 +++ arch/arm/cpu/armv7/tegra2/sys_info.c | 35 arch/arm/cpu/armv7/tegra2/timer.c| 122 + arch/arm/include/asm/arch-tegra2/clk_rst.h | 155 arch/arm/include/asm/arch-tegra2/pinmux.h| 52 ++ arch/arm/include/asm/arch-tegra2/pmc.h | 125 + arch/arm/include/asm/arch-tegra2/sys_proto.h | 33 arch/arm/include/asm/arch-tegra2/tegra2.h| 49 + arch/arm/include/asm/arch-tegra2/uart.h | 45 + board/nvidia/common/board.c | 249 ++ board/nvidia/common/board.h | 57 ++ 14 files changed, 1155 insertions(+), 0 deletions(-) create mode 100644 arch/arm/cpu/armv7/tegra2/Makefile create mode 100644 arch/arm/cpu/armv7/tegra2/board.c create mode 100644 arch/arm/cpu/armv7/tegra2/config.mk create mode 100644 arch/arm/cpu/armv7/tegra2/lowlevel_init.S create mode 100644 arch/arm/cpu/armv7/tegra2/sys_info.c create mode 100644 arch/arm/cpu/armv7/tegra2/timer.c create mode 100644 arch/arm/include/asm/arch-tegra2/clk_rst.h create mode 100644 arch/arm/include/asm/arch-tegra2/pinmux.h create mode 100644 arch/arm/include/asm/arch-tegra2/pmc.h create mode 100644 arch/arm/include/asm/arch-tegra2/sys_proto.h create mode 100644 arch/arm/include/asm/arch-tegra2/tegra2.h create mode 100644 arch/arm/include/asm/arch-tegra2/uart.h create mode 100644 board/nvidia/common/board.c create mode 100644 board/nvidia/common/board.h diff --git a/arch/arm/cpu/armv7/tegra2/Makefile b/arch/arm/cpu/armv7/tegra2/Makefile new file mode 100644 index 000..75fba0b --- /dev/null +++ b/arch/arm/cpu/armv7/tegra2/Makefile @@ -0,0 +1,48 @@ +# +# (C) Copyright 2010,2011 Nvidia Corporation. +# +# (C) Copyright 2000-2003 +# Wolfgang Denk, DENX Software Engineering, w...@denx.de. +# +# See file CREDITS for list of people who contributed to this +# project. +# +# This program is free software; you can redistribute it and/or +# modify it under the terms of the GNU General Public License as +# published by the Free Software Foundation; either version 2 of +# the License, or (at your option) any later version. +# +# This program is distributed in the hope that 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 +# + +include $(TOPDIR)/config.mk + +LIB= $(obj)lib$(SOC).o + +SOBJS := lowlevel_init.o +COBJS := sys_info.o board.o timer.o + +SRCS := $(SOBJS:.o=.S) $(COBJS:.o=.c) +OBJS := $(addprefix $(obj),$(COBJS) $(SOBJS)) + +all:$(obj).depend $(LIB) + +$(LIB):$(OBJS) + $(AR) $(ARFLAGS) $@ $(OBJS) + +# + +# defines $(obj).depend target +include $(SRCTREE)/rules.mk + +sinclude $(obj).depend + +# diff --git a/arch/arm/cpu/armv7/tegra2/board.c b/arch/arm/cpu/armv7/tegra2/board.c new file mode 100644 index 000..1e92d98 --- /dev/null +++ b/arch/arm/cpu/armv7/tegra2/board.c @@ -0,0 +1,91 @@ +/* + * (C) Copyright 2010,2011 + * NVIDIA Corporation + * + * See file CREDITS for list of people who contributed to this + * project. + * + * This program is free software; you can redistribute it and/or + * modify it under the terms of the GNU General Public License as + * published by the Free Software Foundation; either version 2 of + * the License, or (at your option) any later version. + * + * This program is distributed in the hope that 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 + */ + +#include +#include +#include +#include +#inc
