[U-Boot] [PATCH 01/10] mx27: basic cpu support
Sascha Hauer
s.hauer at pengutronix.de
Thu May 14 15:40:55 CEST 2009
On Thu, May 14, 2009 at 10:10:07AM +0200, Wolfgang Denk wrote:
> 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.
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 |
More information about the U-Boot
mailing list