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

["Pali Rohár" <pali.rohar@gmail.com>]

On Tuesday 16 October 2012 16:55:20 Marek Vasut wrote:
> Dear Pali Roh?r,
> 
> [...]
> 
> > 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...)
> Don't we have something that can reserve a memory area?
> 

I'm using CONFIG_PRAM and KERNEL_ADDRESS is calculated from 
protected ram. But previous bootloader (NOLO) loading u-boot (+ 
attached kernel) at random address, so it can be in u-boot malloc 
area. That assembler code is called after reset and copy attached 
kernel to protected ram (so U-Boot will not touch it) and U-Boot 
to correct address.

> > > > +		}
> > > > +		++i;
> > > 
> > > i++;
> > 
> > Reason? ++i and i++ are same (for this situation).
> 
> On arm, yes, on intel, no in certain cases. Also, to keep the
> code consistent, go with postdecrement.
> 

What is different on intel (without assigning value of i)?

> > > > +	/* 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?
> 
> Then why not str[0] == '1' ?
> 

Ok.

> > > > +/*
> > > > + * 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).
> Investigate ;-)
> 

I think it is not possible. Registers are in bad order. (So on 
some high endian system it could be possible)

Also in linux twl regulator driver is used two calls see:
https://git.kernel.org/?p=linux/kernel/git/torvalds/linux.git;a=blob;f=drivers/regulator/twl-
regulator.c;hb=HEAD#l327

> > > > +
> > > > +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.
> 
> The BSS isn't cleared though.
> 

So how to fix it? I need to share these vars in more functions.

> > > > +
> > > > +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.
> Good, document it.
> 

Ok.

-- 
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/686e9247/attachment.pgp>