Hello Wolfgang,
On Wed, 2014-01-29 at 21:06 +0100, Wolfgang Denk wrote:
Dear Alexey Brodkin,
In message 1391011745-22239-3-git-send-email-abrodkin@synopsys.com you wrote:
Signed-off-by: Alexey Brodkin abrodkin@synopsys.com
Cc: Mischa Jonker mjonker@synopsys.com Cc: Francois Bedard fbedard@synopsys.com
No changes for v2.
This belongs into the comment section (below the "---" line), not into the commit message.
Good point, will fix next time.
...
+int icache_status(void) +{
- return (read_aux_reg(ARC_REG_IC_CTRL) & IC_CTRL_CACHE_DISABLE) !=
IC_CTRL_CACHE_DISABLE;+}
+void icache_enable(void) +{
- write_aux_reg(ARC_REG_IC_CTRL, read_aux_reg(ARC_REG_IC_CTRL) &
~IC_CTRL_CACHE_DISABLE);+}
I missed this in patch # 1 - why do you need these read_aux_reg() / write_aux_reg() macros? Why cannot you use standard I/O accessors instead?
This is already answered by Mischa.
+/* STATUS32 register bits related to Interrupt Handling */ +#define STATUS_E1_BIT 1 /* Int 1 enable */ +#define STATUS_E2_BIT 2 /* Int 2 enable */ +#define STATUS_A1_BIT 3 /* Int 1 active */ +#define STATUS_A2_BIT 4 /* Int 2 active */
+#define STATUS_E1_MASK (1<<STATUS_E1_BIT) +#define STATUS_E2_MASK (1<<STATUS_E2_BIT) +#define STATUS_A1_MASK (1<<STATUS_A1_BIT) +#define STATUS_A2_MASK (1<<STATUS_A2_BIT)
See before.
As I answered in comments for the first patch - most of STATUS_xx_BITs are defined in "arcregs.h" which is a copy of kernel's one.
In kernel sources for some reason A1, A2, E1 and E2 bits are defined in separate header "irqflags.h" which as a whole substance we don't need in U-Boot. So I defined these flags in "arch/arc/cpu/arc700/interrupts.c" and did it in the same manner as they are in "arcregs.h".
Moreover it is expected that these mask defines all look the same way STATUS_xx_MASK by handy macro: ==== #define STS_BIT(r, bit) r->status32 & STATUS_##bit##_MASK ? #bit : "" ==== So we may easily access each and every status bit.
Also IMHO it's good to keep the same defines in both Linux kernel and in U-Boot so reader familiar with Linux for ARC understands what happens in U-Boot and vise versa.
That's my motivation here.
+int disable_interrupts(void) +{
- int status = read_aux_reg(ARC_REG_STATUS32);
- int state = (status | STATUS_E1_MASK | STATUS_E2_MASK) ? 1 : 0;
- status &= ~(STATUS_E1_MASK | STATUS_E2_MASK);
- __asm__("flag %0" : : "r" (status));
- return state;
+}
Please insert blankk line between declarations and code. Please fix globally.
+void enable_interrupts(void) +{
- unsigned int status = read_aux_reg(ARC_REG_STATUS32);
- status |= STATUS_E1_MASK | STATUS_E2_MASK;
- __asm__("flag %0" : : "r" (status));
+}
+/*
- Common routine to print scratch regs (r0-r12) or callee regs (r13-r25)
- -Prints 3 regs per line and a CR.
Comment and code disagree: You print "\n", not "\r" !
Agree, so I'll remove entire comment before function and put small comments within function itself - IMHO will be easier to understand a logic.
Regards, Alexey