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.
_______________________________________________
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
