[U-Boot] [PATCH v2 2/9] arc: add cpu files

[Alexey Brodkin]

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 at synopsys.com> you wrote:
> > Signed-off-by: Alexey Brodkin <abrodkin at synopsys.com>
> > 
> > Cc: Mischa Jonker <mjonker at synopsys.com>
> > Cc: Francois Bedard <fbedard at 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