[U-Boot] [PATCH 2/9 V3] add c structures for SoC access

[Jens Scharsig]

Tom wrote:
>>
> 
> I was expecting this patch to convert existing #define's to c struct's
> I was not expecting it to add new features.
> The features should be broken out into their own patch.
> Specifically at91_matrix
> 
> These are significant enough changes that you should
> append a copyright to them.
> 
> ....
> This looks like a new feature that needs its own patch

The at91_matrix holds c stuctures for all derivates together, so i put
the code in the new file.
> 
> This is also used in at91_pdc.h
> Multiple files in the next patches use it as well.
> Please review the above reference to memsetup.S

I will be fix this copy and paste problems  
> 
>> +#else
>> +#if (AT91_MATRIX_MASTERS < 16)
>> +	u32		reserve1[16 - AT91_MATRIX_MASTERS];
>> +#endif
> 
> This #if () construct is awkward.
>  From above it does ot look like either the AT91_MATRIX_MASTER or
> AT91_MATRIX_SLAVE is ever >= 16.
> These should be reduced

OK, we can remove the #if. AT91_MATRIX_MASTER and AT91_MATRIX_SLAVE
can only have an invalid value, if defines above are wrong.

> 
> The board specific element names are also awkward.
> Please try to reduce this.
> 
> Maybe have seperate headers per board?
> This is the way the kernel does it.

You're right, but we have many files with the same function.
What file names will have the file name, when several 
soc are supported (e.g SAM9260 and 9G20 use the same matrix hw)?

To get better readable Code, we can do following:

#if defined(CONFIG_AT91SAM...1) || defined(CONFIG_AT91SAM...1) 
typedef struct at91_matrix {
...
} at91_matrix_t;
#elseif defined(CONFIG_AT91SAM...1) 
typedef struct at91_matrix {
...
} at91_matrix_t;
#else
#error #error CPU not supported. Please update at91_matrix.h
#endif

and we can also remove the AT91_MATRIX_MASTER and AT91_MATRIX_SLAVE
defines

>> +#ifndef AT91_PDC_H
>> +#define AT91_PDC_H
>> +
>> +typedef struct at91_pdc {
...
>> +} at91_pdc_t;
>> +
> 
> This is only used by at91_spi.h's c structure.
> It seems like these elements should just be appended to the at91_spi_t
> Please move them.
> 

PDC is the peripherial DMA controller. Currently only SPI use in SPI
Thats right, but some other peripherials (currently not needed in u-boot)
needs it to (eg. AC91 controller, SCC ...).
>> +#define AT91_PIO_PORTS	5
>> +#else
>> +#define AT91_PIO_PORTS	4
>> +#endif
> 
> It would be better if the '4' case was an explicit list of
> boards like 3 & 5 and the #else case was
> #error "Error unsupported AT91_PIO_PORTS"
> or something like this.
> 
Will be add

>> +
> 
> this could be moved to gpio patch.
> 
>> +#endif
>> +
>> +#define AT91_PIN_TO_MASK(x)	(1<<x)
>> +#define AT91_PORTPIN(PORT, PIN)	((0x0##PORT - 9) * 32 + ((PIN) & 0x1F))
>> +#define	AT91_PIO_PORTA		0x0
>> +#define	AT91_PIO_PORTB		0x1
>> +#define	AT91_PIO_PORTC		0x2
>> +#define	AT91_PIO_PORTD		0x3
>> +#define	AT91_PIO_PORTE		0x4
>> +
> 
> Is this necessary?
I think, yes.
> You are packing arguments that you are passing to the gpio functions.
> It would be better if there were seperate port, pin parameters than
> combining and then uncombining.
> Please change this.
Serveral drivers (e.g nand and I2C) uses this packed pins for arguments. 
> 
> These are already defined in the legacy case
> Pull the legacy ones into the common part of the file
OK
> 
>> +#define		AT91_PIT_MR_PIV_MASK	(x & 0x000fffff)
>> +#define		AT91_PIT_MR_PIV(x)	(x & AT91_PIT_MR_PIV_MASK)
> 
> Where are these used ?
Curently, nowhere
> Remove if unused.
It's defined only to complete mr register flags.
> 
>>  #endif
>> diff --git a/include/asm-arm/arch-at91/at91_pmc.h b/include/asm-arm/arch-at91/at91_pmc.h
>> index 9fe94c7..9a0e1d2 100644
>> --- a/include/asm-arm/arch-at91/at91_pmc.h
>> +++ b/include/asm-arm/arch-at91/at91_pmc.h
>> @@ -16,6 +16,105 @@
>>  #ifndef AT91_PMC_H
>>  #define AT91_PMC_H
>>  
>> +#ifdef __ASSEMBLY__
>> +
>> +#define	AT91_ASM_PMC_MOR	(AT91_PMC_BASE + 0x20)
>> +#define	AT91_ASM_PMC_PLLAR	(AT91_PMC_BASE + 0x28)
>> +#define	AT91_ASM_PMC_PLLBR	(AT91_PMC_BASE + 0x2c)
>> +#define AT91_ASM_PMC_MCKR	(AT91_PMC_BASE + 0x30)
>> +#define AT91_ASM_PMC_SR		(AT91_PMC_BASE + 0x68)
>> +
> 
> These could always be defined and the
> #else turned into
> #ifndef __ASSEMBLY__
> 

OK
>> +#ifdef CONFIG_AT91SAM9
>> +	u32	pllicpr;	/* 0x80 Change Pump Current Register */
>> +#else
>> +	u32	reserved5;
>> +#endif
> 
> board specific elements is awkward.
> Please combine these element names
> Similar below.
> 
>> +	u32	reserved6[21];
>> +#ifdef CONFIG_AT91CAP9
>> +	u32	wpmr;		/* 0xE4 Write Protect Mode Register */
>> +	u32	wpsr;		/* 0xE8 Write Protect Status Register */
>> +#else
>> +	u32	reserved7[2];
Do you think we should define registers, even if they are only used by 
a particular cpu type?
> 
> Change mixed case in macro to all upper.
> Also make sure macros are not already defined in legacy
OK
> 

regards Jens