From mboxrd@z Thu Jan 1 00:00:00 1970 From: Sascha Hauer Date: Thu, 14 May 2009 15:40:55 +0200 Subject: [U-Boot] [PATCH 01/10] mx27: basic cpu support In-Reply-To: <20090514081007.AF05C832E416@gemini.denx.de> References: <1241634633-13917-1-git-send-email-yanok@emcraft.com> <1241634633-13917-2-git-send-email-yanok@emcraft.com> <20090506211604.289C483420E8@gemini.denx.de> <4A0B4F9A.8030503@emcraft.com> <20090514081007.AF05C832E416@gemini.denx.de> Message-ID: <20090514134055.GA29278@pengutronix.de> List-Id: MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: u-boot@lists.denx.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 |