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-abrod...@synopsys.com> you
> wrote:
> > Signed-off-by: Alexey Brodkin <abrod...@synopsys.com>
> >
> > Cc: Mischa Jonker <mjon...@synopsys.com>
> > Cc: Francois Bedard <fbed...@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
_______________________________________________
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
