From mboxrd@z Thu Jan  1 00:00:00 1970
From: Ilya Yanok <yanok@emcraft.com>
Date: Thu, 14 May 2009 02:54:18 +0400
Subject: [U-Boot] [PATCH 01/10] mx27: basic cpu support
In-Reply-To: <20090506211604.289C483420E8@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>
Message-ID: <4A0B4F9A.8030503@emcraft.com>
List-Id: <u-boot.lists.denx.de>
MIME-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: 7bit
To: u-boot@lists.denx.de

Hi Wolfgang,

Wolfgang Denk wrote:
>> +static ulong clk_in_26m(void)
>> +{
>> +	if (CSCR & CSCR_OSC26M_DIV1P5) {
>> +		/* divide by 1.5 */
>> +		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.

>> +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... On the other hand Linux uses offsets for registers
definitions so converting them to C structs makes porting and
maintaining ported drivers harder...

> 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.

>> +#define IMX_IO_BASE		0x10000000
>> +
>> +#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.

Regards, Ilya.