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

[Ilya Yanok]

Hi Wolfgang,

Wolfgang Denk wrote:
>> 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...
>   

Well, out_be32() and friends don't convert pointer, you are right. But
these functions are not really generic, they can be found only on couple
of archs. And writel() and friends (which are generic accessor functions
for MMIO) do silent pointer conversion...

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

I see. PowerPC in Linux uses C structs too. But there are still a lot of
code that uses registers offsets in Linux, so my arguments are the same:
requirement to convert offsets to C struct brings additional
difficulties to porting (and maintaining already ported) drivers from Linux.

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

Ok, we can do that. But for what reason? I don't think this improves
readability...

Regards, Ilya.