[U-Boot] [PATCH 1/9] arc: add architecture header files

[Alexey Brodkin]

Hello Heiko,

On Wed, 2014-01-29 at 06:44 +0100, Heiko Schocher wrote:
> Hello Alexey,
> 
> Thanks for your patches, more or less just nitpicking comments ...

Thanks for prompt review.

> Am 29.01.2014 00:09, schrieb Alexey Brodkin:
> > Signed-off-by: Alexey Brodkin<abrodkin at synopsys.com>
> 
> No commit message, please add one. (Are this files from the Linux
> kernel? If so please add a comment in the commit message + add a
> hint which linux commit you used, thanks!)

I thought from commit subject it's already clear what's presented in
each particular patch so I left commit messages empty.
Frankly I'm not sure still what info may I put in commit messages except
mention of headers borrowed from Linux - or this is exactly what is
needed?

Agree I forgot to mention which header files came from Linux kernel.
Will add mentions.


> > diff --git a/arch/arc/include/asm/arch-arc700/hardware.h b/arch/arc/include/asm/arch-arc700/hardware.h
> > new file mode 100644
> > index 0000000..e69de29
> 
> Empty file ?

Right, it looks weird, but I had to add this empty file just because of
"designware_i2c" driver which refers to "asm/arch/hardware.h".

http://git.denx.de/?p=u-boot.git;a=blob;f=drivers/i2c/designware_i2c.c;h=9ed929521a8944dc870fc2eff546507632b6e86a;hb=HEAD

And to remove "asm/arch/hardware.h" I would need to modify
"arch/hardware.h" and "include/configs/..." files for platform/boards I
don't own.

Basically this is just a work-around that allows me to use
"designware_i2c" driver as it is.

There was a similar dependency ("asm/arch/clk.h") in "dw_mmc" but in
that case it was possible to just remove it - what I did -
http://git.denx.de/?p=u-boot.git;a=commit;h=ca6d4d0f8f0fb8ae09a7ba271177367bdfdf3136

So if you insist on removal of this file we would need to fix
"designware_i2c" first.

Please let me know what do you think about this item.

> > diff --git a/arch/arc/include/asm/arcregs.h b/arch/arc/include/asm/arcregs.h
> > new file mode 100644
> > index 0000000..87b0a60
...
> > +
> > +#ifndef __ASSEMBLY__
> > +
> > +/*
> > + ******************************************************************
> 
> Bad comment style
> 
> > + *      Inline ASM macros to read/write AUX Regs
> > + *      Essentially invocation of lr/sr insns from "C"
> > + */
> > +
> > +#if 1
> > +
> > +#define read_aux_reg(reg)	__builtin_arc_lr(reg)
> > +
> > +/* gcc builtin sr needs reg param to be long immediate */
> > +#define write_aux_reg(reg_immed, val)		\
> > +		__builtin_arc_sr((unsigned int)val, reg_immed)
> > +
> > +#else
> 
> Please remove dead code ...
> 
> > +
> > +#define read_aux_reg(reg)		\
...
> > +		bogus_undefined();			\
> > +	}						\
> > +}
> 
> Why do you use defines here instead of real functions?
> 
> > +
> > +#define WRITE_BCR(reg, into)				\
> > +{							\
> > +	unsigned int tmp;				\
> > +	if (sizeof(tmp) == sizeof(into)) {		\
> > +		tmp = (*(unsigned int *)(into));	\
> > +		write_aux_reg(reg, tmp);		\
> > +	} else  {					\
> > +		extern void bogus_undefined(void);	\
> > +		bogus_undefined();			\
> > +	}						\
> > +}
> 
> and here?

Ok, so this header is borrowed from Linux sources. That's why I didn't
do any modifications and took it as it is on kernel.org.

> > diff --git a/arch/arc/include/asm/processor.h b/arch/arc/include/asm/processor.h
> > new file mode 100644
> > index 0000000..e69de29
> 
> Hups, one more empty file ...

I thought it was required by some common file. Double-checked and now
see that it could be safely removed.

> > diff --git a/arch/arc/include/asm/ptrace.h b/arch/arc/include/asm/ptrace.h
> > new file mode 100644
> > index 0000000..3b2df87
> > --- /dev/null
> > +++ b/arch/arc/include/asm/ptrace.h
> > @@ -0,0 +1,101 @@
> > +/*
> > + * Copyright (C) 2004, 2007-2010, 2011-2012 Synopsys, Inc. All rights reserved.
> > + *
> > + * SPDX-License-Identifier:	GPL-2.0+
> > + */
> > +
> > +#ifndef __ASM_ARC_PTRACE_H
> > +#define __ASM_ARC_PTRACE_H
> > +
> > +#ifndef __ASSEMBLY__
> > +
> > +/* THE pt_regs: Defines how regs are saved during entry into kernel */
> 
> Is the "THE" a shortcut?
> 

Another copy from Linux sources - thus taken as it is.

> > diff --git a/arch/arc/include/asm/string.h b/arch/arc/include/asm/string.h
> > new file mode 100644
> > index 0000000..e69de29
> 
> empty file

Somehow I missed a fact that ARC already has optimized "string"
routines. Will add them in re-spin.

Will send a v2 series soon.

-Alexey