[U-Boot] [PATCH v3 5/5] New board support: Nokia RX-51 aka N900

Pali Rohár
2012-10-16T14:43:54Z

On Sunday 14 October 2012 02:06:49 Marek Vasut wrote:
> >
> > diff --git a/MAINTAINERS b/MAINTAINERS
> > index 971235b..613d8cd 100644
> > --- a/MAINTAINERS
> > +++ b/MAINTAINERS
> > @@ -1009,6 +1009,10 @@ Nobuhiro Iwamatsu
> > <nobuhiro.iwamatsu.yj@renesas.com>>
> > armadillo-800eva R8A7740 (RMOBILE SoC)
> >
> > +Pali Roh?r <pali.rohar@gmail.com>
> > +
> > + nokia_rx51 ARM ARMV7 (OMAP3xx SoC)
>
> OMAP3xxx or OMAP34xx
>

Ok.

>
> > +relocaddr: /* address of this relocaddr section after
> > coping */ + .word . /* address of section (calculated
at
> > compile time) */ +
> > +startaddr: /* address of u-boot after copying */
> > + .word CONFIG_SYS_TEXT_BASE
> > +
> > +kernaddr: /* address of kernel after copying */
> > + .word KERNEL_ADDRESS
> > +
> > +kernsize: /* maximal size of kernel image */
> > + .word KERNEL_MAXSIZE
> > +
> > +kernoffs: /* offset of kernel image in loaded u-boot
*/
> > + .word KERNEL_OFFSET
> > +
> > +imagesize: /* maximal size of image */
> > + .word IMAGE_MAXSIZE
> > +
> > +ih_magic: /* IH_MAGIC in big endian from
include/image.h */
> > + .word 0x56190527
> > +
> > +/*
>
> Try using the new kerneldoc style, the tools are now in.
>
> See eg. here about the annotations:
> http://permalink.gmane.org/gmane.comp.boot-loaders.u-boot/14417
> 3
>
> Also see:
> http://git.kernel.org/?p=linux/kernel/git/torvalds/linux-2.6.gi
> t;a=blob;f=Documentation/kernel- doc-nano-HOWTO.txt
>

Ok.

> > + * Routine: save_boot_params (called after reset from
> > start.S) + * Description: Copy attached kernel to address
> > KERNEL_ADDRESS + * Copy u-boot to address
> > CONFIG_SYS_TEXT_BASE + * Return to copied
> > u-boot address
> > + */
> > +
> > +.global save_boot_params
>
> > +save_boot_params:
> [...]
>
> How much of the assembler crap can be made C?
>

Nothing. This code must be in assembler because stack is not
initialized. Also we must be sure that U-Boot will not overwrite
attached kernel (with can be in U-Boot malloc/monitor area...)

>
> > +static char *boot_reason_ptr;
> > +static char *hw_build_ptr;
> > +static char *nolo_version_ptr;
> > +static char *boot_mode_ptr;
> > +
> > +/*
> > + * Routine: init_omap_tags
> > + * Description: Initialize pointers to values in tag_omap
> > + */
> > +static void init_omap_tags(void)
> > +{
> > + char *component;
> > + char *version;
> > + int i = 0;
> > + while (omap[i].hdr.tag) {
> > + switch (omap[i].hdr.tag) {
> > + case OMAP_TAG_BOOT_REASON:
> > + boot_reason_ptr =
omap[i].u.boot_reason.reason_str;
> > + break;
> > + case OMAP_TAG_VERSION_STR:
> > + component = omap[i].u.version.component;
> > + version = omap[i].u.version.version;
> > + if (strcmp(component, "hw-build") == 0)
> > + hw_build_ptr = version;
> > + else if (strcmp(component, "nolo") == 0)
> > + nolo_version_ptr = version;
> > + else if (strcmp(component, "boot-mode") == 0)
> > + boot_mode_ptr = version;
> > + break;
>
> default: missing.
>

Is really needed? (if yes, I can add default: break;)

> > + }
> > + ++i;
>
> i++;
>

Reason? ++i and i++ are same (for this situation).

> > + /* append omap atag only if env setup_omap_atag is set to
1
> > */ + str = getenv("setup_omap_atag");
> > + if (!str || strcmp(str, "1") != 0)
>
> str[0] == '1' ? But still, you only want to check if it's
> defined, no?
>

Hm, I'm checking if setup_omap_atag is 1. But is it problem?

>
> > +/*
> > + * Routine: twl4030_regulator_set_mode
> > + * Description: Set twl4030 regulator mode over i2c
> > powerbus.
> > + */
> > +static void twl4030_regulator_set_mode(u8 id, u8 mode)
> > +{
> > + u16 msg = MSG_SINGULAR(DEV_GRP_P1, id, mode);
> > + twl4030_i2c_write_u8(TWL4030_CHIP_PM_MASTER, msg >> 8,
> > + TWL4030_PM_MASTER_PB_WORD_MSB);
> > + twl4030_i2c_write_u8(TWL4030_CHIP_PM_MASTER, msg & 0xff,
> > + TWL4030_PM_MASTER_PB_WORD_LSB);
>
> Uh, is this somehow special that you can't do longer transfer?
>

I do not know how, because registers are different (MSB and LSB).

>
> > + /* set env variable attkernaddr for relocated kernel */
> > + sprintf(buf, "%#x", KERNEL_ADDRESS);
> > + setenv("attkernaddr", buf);
>
> Uhhh ? This definitelly isn't right! What are you trying to
> achieve here?

I think it is right. I want to store address of kernel (in hex
with leading 0x) to env attkernaddr. And %#x is doing it.

> > +
> > +static unsigned long int twl_wd_time; /* last time of
> > watchdog reset */ +static unsigned long int twl_i2c_lock;
>
> Are you sure you want to use global vars for these? These won't
> work before reloc!
>

Why it does not work before reloc? U-Boot is on n900 always
started from RAM.

>
> > +
> > +static u8 keys[8];
> > +static u8 old_keys[8] = {0, 0, 0, 0, 0, 0, 0, 0};
> > +#define KEYBUF_SIZE 32
> > +static u8 keybuf[KEYBUF_SIZE];
> > +static u8 keybuf_head;
> > +static u8 keybuf_tail;
>
> How much of this can be made const ?
>

Nothing. All is for keyboard and is changed by keyboard functions
(tstc, getc)

>
> This magic needs at least _some_ documentation.
>
> > + if (!(mods & 2) && (k == 18 || k == 31 || k == 33 || k ==
> > 34)) { + /* cursor keys, without fn */
> > + keybuf[keybuf_tail++] = '\e';
>

there is "cursor keys without fn" (fn is "meta" key on n900). So
this condition check if some cursor key (without fn) was pressed.

>
> > +int rx51_kp_tstc(void)
> > +{
> > + u8 c, r, dk, i;
> > + u8 intr;
> > + u8 mods;
> > +
> > + /* localy lock twl4030 i2c bus */
> > + if (test_and_set_bit(0, &twl_i2c_lock))
> > + return 0;
> > +
> > + /* twl4030 remembers up to 2 events */
> > + for (i = 0; i < 2; i++) {
> > +
> > + /* check interrupt register for events */
> > + twl4030_i2c_read_u8(TWL4030_CHIP_KEYPAD, &intr,
> > + TWL4030_KEYPAD_KEYP_ISR1+(2*i));
> > +
> > + if (intr&1) { /* got an event */
>
> I will let you think about how to optimize the indent depth
> here ...

Ok, I will optimize indentation. This code (keyboard support) was
written by Alistair Buxton and I reused it with minimal changes.

>
> > +#define tostring(s) #s
> > +#define stringify(s) tostring(s)
>
> We do have __stringify(), use that!

Ok, in April (when I wrote this part of code) __stringify was not
in U-Boot (I checked it).

--
Pali Roh?r
pali.rohar at gmail.com
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 198 bytes
Desc: This is a digitally signed message part.
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20121016/cc3cdeb0/attachment.pgp>