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

[Wolfgang Denk]

Dear Ilya Yanok,

In message <4A0B4F9A.8030503 at 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.

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.

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;
		...
	};
?

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

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: wd at denx.de
In theory, there is no difference between  theory  and  practice.  In
practice, however, there is.