From mboxrd@z Thu Jan 1 00:00:00 1970 From: Heiko Schocher Date: Wed, 29 Jan 2014 10:04:55 +0100 Subject: [U-Boot] [PATCH 1/9] arc: add architecture header files In-Reply-To: <1390985824.2540.15.camel@abrodkin-8560l> References: <1390950580-10672-1-git-send-email-abrodkin@synopsys.com> <1390950580-10672-2-git-send-email-abrodkin@synopsys.com> <52E89526.4040009@denx.de> <1390985824.2540.15.camel@abrodkin-8560l> Message-ID: <52E8C437.1080400@denx.de> List-Id: MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: u-boot@lists.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 >> >> 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