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

[Heiko Schocher <hs@denx.de>]

Hello Alexey,

Am 29.01.2014 09:57, schrieb 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@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?

Maybe this site helps you:

http://www.denx.de/wiki/view/U-Boot/Patches#Attributing_Code_Copyrights_Sign

> 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.

Hmm.. at least you should an comment in this file, why it is necessary.

>>> 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.

Ah, ok, so this is ok I think.

>>> 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.

Great!

>>> 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.

Ok, hope to find time for your other patches ...

bye,
Heiko
-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany