[U-Boot] [PATCH 1/4] arm: iproc: Initial commit of iproc architecture code

[Steve Rae]

On 14-07-20 12:46 AM, Wolfgang Denk wrote:
> Dear Steve Rae,
>
> In message <1405733854-20194-2-git-send-email-srae at broadcom.com> you wrote:
>> From: Scott Branden <sbranden at broadcom.com>
>>
>> The iproc architecture code is present in several Broadcom
>> chip architectures, including Cygnus and NSP.
> ...
>> +	writel(IPROC_CLKCT_HDELAY_SW_EN, IHOST_PROC_CLK_CORE0_CLKGATE);
>> +	writel(IPROC_CLKCT_HDELAY_SW_EN, IHOST_PROC_CLK_CORE1_CLKGATE);
>> +	writel(IPROC_CLKCT_HDELAY_SW_EN, IHOST_PROC_CLK_ARM_SWITCH_CLKGATE);
>> +	writel(IPROC_CLKCT_HDELAY_SW_EN, IHOST_PROC_CLK_ARM_PERIPH_CLKGATE);
>> +	writel(IPROC_CLKCT_HDELAY_SW_EN, IHOST_PROC_CLK_APB0_CLKGATE);
>
> Instead of using #defines for IHOST_PROC_CLK_CORE0_CLKGATE etc. it
> would be better to use a C struct to describe the register map.

In our situation, the register map is an automatically generated list of 
#defines (which actually comes directly from another department within 
the company)... It might be better to use a C struct, but to be able to 
use this generated file is more accurate.

>
>> +		count_h = readl(IPROC_PERIPH_GLB_TIM_REG_BASE +
>> +				TIMER_GLB_HI_OFFSET);
>> +		count_l = readl(IPROC_PERIPH_GLB_TIM_REG_BASE +
>> +				TIMER_GLB_LOW_OFFSET);
>> +		cur_tick = readl(IPROC_PERIPH_GLB_TIM_REG_BASE +
>> +				 TIMER_GLB_HI_OFFSET);
>
> NAK.  We do not support accessing device registers through a "base
> address + offset" notation.  Please use a C struct instead.

Please clarify -- does the "readl()" (and "writel()") have issues with 
this "base + offset" notation?  We have used this extensively (in the 
non-upstreamed code), and we would like to upstream this.
( ...looking at the existing U-Boot code, this notation is used 
elsewhere... )

>
> Please fix globally.
>
> ...
>> +#define IHOST_PROC_CLK_WR_ACCESS				0X19000000
>> +#define IHOST_PROC_CLK_POLICY_FREQ				0X19000008
> ...
>> +#define IHOST_PROC_CLK_POLICY_CTL				0X1900000C
> ...
>
> Make C struct?
(automatically generated code)

>
>> +/* ARM A9 Private Timer */
>> +#define TIMER_PVT_LOAD_OFFSET			0x00000000
>> +#define TIMER_PVT_COUNTER_OFFSET		0x00000004
>> +#define TIMER_PVT_CTRL_OFFSET			0x00000008
>> +#define TIMER_PVT_STATUS_OFFSET			0x0000000C
> ...
>> +#define TIMER_GLB_LOW_OFFSET			0x00000000
>> +#define TIMER_GLB_HI_OFFSET			0x00000004
>> +#define TIMER_GLB_CTRL_OFFSET			0x00000008
>
> Please make C struct !!!
(automatically generated code)

>
> Best regards,
>
> Wolfgang Denk
>