From mboxrd@z Thu Jan 1 00:00:00 1970 From: Steve Rae Date: Mon, 21 Jul 2014 17:27:41 -0700 Subject: [U-Boot] [PATCH 1/4] arm: iproc: Initial commit of iproc architecture code In-Reply-To: <20140720074648.53819380316@gemini.denx.de> References: <1405733854-20194-1-git-send-email-srae@broadcom.com> <1405733854-20194-2-git-send-email-srae@broadcom.com> <20140720074648.53819380316@gemini.denx.de> Message-ID: <53CDAFFD.80909@broadcom.com> List-Id: MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: u-boot@lists.denx.de On 14-07-20 12:46 AM, Wolfgang Denk wrote: > Dear Steve Rae, > > In message <1405733854-20194-2-git-send-email-srae@broadcom.com> you wrote: >> From: Scott Branden >> >> 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 >