[U-Boot] [PATCH 01/10] mx27: basic cpu support

[Sascha Hauer <s.hauer@pengutronix.de>]

On Thu, May 14, 2009 at 10:10:07AM +0200, Wolfgang Denk wrote:
> Dear Ilya Yanok,
> 
> In message <4A0B4F9A.8030503@emcraft.com> you wrote:
> > 
> > >> +		return 26000000 / 1.5;
> > >
> > > We definitely do not allow any FP use in U-Boot.
> > 
> > This will be actually converted to an integer at the compile time.
> 
> Maybe. But it's also trivial not to use any FP calculations at all.
> 
> > >> +void imx_gpio_mode(int gpio_mode)
> > >> +{
> > >> +	unsigned int pin = gpio_mode & GPIO_PIN_MASK;
> > >> +	unsigned int port = (gpio_mode & GPIO_PORT_MASK) >> GPIO_PORT_SHIFT;
> > >> +	unsigned int ocr = (gpio_mode & GPIO_OCR_MASK) >> GPIO_OCR_SHIFT;
> > >> +	unsigned int aout = (gpio_mode & GPIO_AOUT_MASK) >> GPIO_AOUT_SHIFT;
> > >> +	unsigned int bout = (gpio_mode & GPIO_BOUT_MASK) >> GPIO_BOUT_SHIFT;
> > >> +	unsigned int tmp;
> > >> +
> > >> +	/* Pullup enable */
> > >> +	if(gpio_mode & GPIO_PUEN)
> > >> +		PUEN(port) |= (1 << pin);
> > >> +	else
> > >> +		PUEN(port) &= ~(1 << pin);
> > >>     
> > >
> > > This smells as if these were pointer accesses using register offsets
> > > instead of I/O accessor calls using C structs?
> > >   
> > 
> > Ok, I really like using accessor calls instead of pointer accesses but I
> > don't really understand the reason for using C structs here... I
> > remember I've already asked you about that and you told me that it's for
> > type safety... But we loose this type-safety when we are using I/O
> > accessor functions! All pointers are just silently converted to the
> > needed type... 
> 
> They are not _silently_ converted. They raise compiler warnings. If,
> for example, I try to access a 32 bit register using a 16 bit I/O
> accessor function as simulated by this (bogus) change:
> 
> -       out_be32(&fec->eth->r_des_active, 0x01000000);
> +       out_be16(&fec->eth->r_des_active, 0x01000000);
> 
> then the compiler will complain:
> 
> mpc512x_fec.c: In function 'mpc512x_fec_rbd_clean':
> mpc512x_fec.c:125: warning: passing argument 1 of 'out_be16' from incompatible pointer type
> 
> I never understood why you claim such type checking would not
> happen...
> 
> >            ... On the other hand Linux uses offsets for registers
> > definitions so converting them to C structs makes porting and
> > maintaining ported drivers harder...
> 
> It is incorrect to state that "Linux uses offsets for registers". The
> Linux code for ARM may do this, and I consider this one of the major
> deficiencies of the ARM code in Linux. Other architectures (like
> PowerPC) deprecated this long ago.

Can you provide some pointers to a discussion? I sometimes google for
this topic, but I never found something relevant.

> 
> And in U-Boot we also don't accept this any more.
> 
> > > You probably want to use the respective clrbits*() / setbits*() macros
> > > here?
> > 
> > I can see these macros defined only for ppc arch... And I really prefer
> > generic writel(readl() | something) here... The reason is the same: to
> > preserve as much code as it possible in drivers ported from Linux.
> 
> Yes, I am aware of this. This is another area where cleanup is needed.

The good thing about writel(readl() | something) is that this makes it
clear that this operation is not atomic.

> 
> Note that I'm not sure if readl() can be considered "generic" across
> architectures. If my understanding is correct, then in Linux the
> ioread() / iowrite() are considered portable and should be used, i. e.
> ioread16(), ioread16be(), ioread32(), ioread32be() etc., with the
> plain ioread*() [i. e. without the "be" part] being little-endian on
> all architectures.
> 
> > >> +#define IMX_AIPI1_BASE             (0x00000 + IMX_IO_BASE)
> > >> +#define IMX_WDT_BASE               (0x02000 + IMX_IO_BASE)
> > >> +#define IMX_TIM1_BASE              (0x03000 + IMX_IO_BASE)
> > >> +#define IMX_TIM2_BASE              (0x04000 + IMX_IO_BASE)
> > >> +#define IMX_TIM3_BASE              (0x05000 + IMX_IO_BASE)
> > >> +#define IMX_UART1_BASE             (0x0a000 + IMX_IO_BASE)
> > >> +#define IMX_UART2_BASE             (0x0b000 + IMX_IO_BASE)
> > >> +#define IMX_UART3_BASE             (0x0c000 + IMX_IO_BASE)
> > >> +#define IMX_UART4_BASE             (0x0d000 + IMX_IO_BASE)
> > >> +#define IMX_I2C1_BASE              (0x12000 + IMX_IO_BASE)
> > >> +#define IMX_GPIO_BASE              (0x15000 + IMX_IO_BASE)
> > >> +#define IMX_TIM4_BASE              (0x19000 + IMX_IO_BASE)
> > >> +#define IMX_TIM5_BASE              (0x1a000 + IMX_IO_BASE)
> > >> +#define IMX_UART5_BASE             (0x1b000 + IMX_IO_BASE)
> > >> +#define IMX_UART6_BASE             (0x1c000 + IMX_IO_BASE)
> > >> +#define IMX_I2C2_BASE              (0x1D000 + IMX_IO_BASE)
> > >> +#define IMX_TIM6_BASE              (0x1f000 + IMX_IO_BASE)
> > >> +#define IMX_AIPI2_BASE             (0x20000 + IMX_IO_BASE)
> > >> +#define IMX_PLL_BASE               (0x27000 + IMX_IO_BASE)
> > >> +#define IMX_SYSTEM_CTL_BASE        (0x27800 + IMX_IO_BASE)
> > >> +#define IMX_IIM_BASE               (0x28000 + IMX_IO_BASE)
> > >> +#define IMX_FEC_BASE               (0x2b000 + IMX_IO_BASE)
> > >>     
> > >
> > > NAK. We do not accept device I/O through pointers; please use C
> > > structs to describe the hardware and use I/O accessor calls.
> > >   
> > 
> > These are actually base addresses. I don't think we can make use of C
> > structs here.
> 
> Well, each of these addresses points to some device registers that
> shoud be described by a C struct. Of course it is possible to sum
> these structs up into a big struct like it is done with the IMMR
> structs for PowerPC. It has been discussed if this makes sense,
> especially when there should be huge gaps in such a stuct, but this is
> obviously not the case here. So what prevents you from doung something
> like
> 
> 	struct imx_io {
> 		struct ... imx_aipi1;
> 		struct ... imx_wdt;
> 		struct ... imx_tim[3];
> 		struct ... imx_uart[4];
> 		struct ... imx_i2c1;
> 		...
> 	};
> ?

Sorry, but IMO structs over registers simply suck. I never found
typechecking an issue when searching for bugs, but what I found an issue
several times is to check which register is accessed, especially when
the names in structs differ from those in the datasheet. I have often
enough counted the members in a struct to determine the offset of a
register.

I wouldn't enforce people using structs over hardware registers. This
really hurts portability between U-Boot and the Kernel.

> 
> Then just a single base address (IMX_IO_BASE) is left [and I wonder if
> you cannot read this from some register.]

Arm processors usually have their registers on a fixed address. You
can't map them to another place.

Sascha

-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |