On 14-07-20 12:46 AM, Wolfgang Denk wrote:
Dear Steve Rae,

In message <[email protected]> you wrote:
From: Scott Branden <[email protected]>

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

_______________________________________________
U-Boot mailing list
[email protected]
http://lists.denx.de/mailman/listinfo/u-boot

Reply via email to