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

[Alexey Brodkin]

Hello Wolfgang,

On Wed, 2014-01-29 at 20:54 +0100, Wolfgang Denk wrote:
> Dear Alexey Brodkin,
> 
> In message <1391011745-22239-2-git-send-email-abrodkin at synopsys.com> you wrote:
> > These are header files used by ARC700 architecture.
> ...
> > +/* Build Configuration Registers */
> > +#define ARC_REG_DCCMBASE_BCR	0x61	/* DCCM Base Addr */
> > +#define ARC_REG_CRC_BCR		0x62
> > +#define ARC_REG_DVFB_BCR	0x64
> > +#define ARC_REG_EXTARITH_BCR	0x65
> > +#define ARC_REG_VECBASE_BCR	0x68
> > +#define ARC_REG_PERIBASE_BCR	0x69
> > +#define ARC_REG_FP_BCR		0x6B	/* Single-Precision FPU */
> > +#define ARC_REG_DPFP_BCR	0x6C	/* Dbl Precision FPU */
> > +#define ARC_REG_DCCM_BCR	0x74	/* DCCM Present + SZ */
> > +#define ARC_REG_TIMERS_BCR	0x75
> > +#define ARC_REG_ICCM_BCR	0x78
> > +#define ARC_REG_XY_MEM_BCR	0x79
> > +#define ARC_REG_MAC_BCR		0x7a
> > +#define ARC_REG_MUL_BCR		0x7b
> > +#define ARC_REG_SWAP_BCR	0x7c
> > +#define ARC_REG_NORM_BCR	0x7d
> > +#define ARC_REG_MIXMAX_BCR	0x7e
> > +#define ARC_REG_BARREL_BCR	0x7f
> > +#define ARC_REG_D_UNCACH_BCR	0x6A
> 
> Are you sure that this should not become a C struct declaration?  This
> looks much like offsets, and we so not allow a base address + offset
> notation for device I/O ...
> 
> > +/* status32 Bits Positions */
> > +#define STATUS_AE_BIT		5	/* Exception active */
> > +#define STATUS_DE_BIT		6	/* PC is in delay slot */
> > +#define STATUS_U_BIT		7	/* User/Kernel mode */
> > +#define STATUS_L_BIT		12	/* Loop inhibit */
> 
> Please uses masks instead of bit numbers.  Bitfields are inherently
> non-portable and dangerous.
> 
> > +/* These masks correspond to the status word(STATUS_32) bits */
> > +#define STATUS_AE_MASK		(1<<STATUS_AE_BIT)
> > +#define STATUS_DE_MASK		(1<<STATUS_DE_BIT)
> > +#define STATUS_U_MASK		(1<<STATUS_U_BIT)
> > +#define STATUS_L_MASK		(1<<STATUS_L_BIT)
> 
> As the STATUS_??_BIT defiens should not be used anywhere else, you
> might drop them without loss of readability.
> 
> > +/*
> > + ******************************************************************
> > + *      Inline ASM macros to read/write AUX Regs
> > + *      Essentially invocation of lr/sr insns from "C"
> > + */
> 
> Can we drop this "*******************" line?
> 
> > +#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
> > +
> > +#define read_aux_reg(reg)		\
> ...
> > +#endif
> 
> This pretty long else branch is just dead code.  Please get rid of all
> such "#if 1" or "#if 0" blocks in your code, globally.
> 
> > +#define READ_BCR(reg, into)				\
> > +{							\
> > +	unsigned int tmp;				\
> > +	tmp = read_aux_reg(reg);			\
> > +	if (sizeof(tmp) == sizeof(into)) {		\
> > +		into = *((typeof(into) *)&tmp);		\
> > +	} else {					\
> > +		extern void bogus_undefined(void);	\
> > +		bogus_undefined();			\
> > +	}						\
> > +}
> > +
> > +#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();			\
> > +	}						\
> > +}
> 
> Do we really need this?  No other architecture does anything like
> that.  Can you not just use standard I/O accessors?
> 
> > +/* Helpers */
> > +#define TO_KB(bytes)		((bytes) >> 10)
> > +#define TO_MB(bytes)		(TO_KB(bytes) >> 10)
> > +#define PAGES_TO_KB(n_pages)	((n_pages) << (PAGE_SHIFT - 10))
> > +#define PAGES_TO_MB(n_pages)	(PAGES_TO_KB(n_pages) >> 10)
> 
> Please drop these.  The just make the code harder to read.
> 
> > +struct bcr_identity {
> > +#ifdef CONFIG_CPU_BIG_ENDIAN
> > +	unsigned int chip_id:16, cpu_id:8, family:8;
> > +#else
> > +	unsigned int family:8, cpu_id:8, chip_id:16;
> > +#endif
> > +};

As it is clearly mentioned in commit message "arcregs.h" came from Linux
sources and that's why kept as it was. You may refer to it here -
https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/arch/arc/include/asm/arcregs.h?id=v3.11

So I'm wondering if you insist on fixing all mentioned items in
"arcregs.h" or I may keep it as it is still?

> Arghh... Please by all means try to avoid bitfields!  They are pure
> evil and are strongly discouraged!
> 
> > +struct bcr_extn {
> > +#ifdef CONFIG_CPU_BIG_ENDIAN
> > +	unsigned int pad:20, crc:1, ext_arith:2, mul:2, barrel:2, minmax:2,
> > +		     norm:2, swap:1;
> > +#else
> > +	unsigned int swap:1, norm:2, minmax:2, barrel:2, mul:2, ext_arith:2,
> > +		     crc:1, pad:20;
> > +#endif
> > +};
> 
> Do you really need your own definition CONFIG_CPU_BIG_ENDIAN here?
> Can you not use the existing standard defines instead?  If you do,
> please keep inmind that all new CONFIG_ optins must be explained in
> the README.

Makes perfect sense. I missed the fact that similar defines have a bit
different names in Linux/U-boot.

It's CONFIG_CPU_BIG_ENDIAN in Linux while CONFIG_SYS_BIG_ENDIAN in
U-Boot. So I'll substitute it here with proper CONFIG_SYS_BIG_ENDIAN.

> > +#if !defined(__STRICT_ANSI__) || defined(__KERNEL__)
> > +	#define __BYTEORDER_HAS_U64__
> > +	#define __SWAB_64_THRU_32__
> > +#endif
> 
> We don't have __KERNEL__ much longer, I think...

Ok, I copy-pasted it from "arch/arm/include/asm/byteorder.h".
And it still has __KERNEL__ -
http://git.denx.de/?p=u-boot.git;a=blob;f=arch/arm/include/asm/byteorder.h;h=20cce7657e1059a170fd5ef32688cb96064fcd7f;hb=HEAD

Are we going to fix it for existing arches (note all arches except M68K,
PowerPC and SPARC have it in "byteorder.h")?

So do I need to eliminate all checks for __KERNEL__ then?

> > +#define IO_WRITE32(val, addr) ({__asm__ __volatile__ ("st.di %0,[%1]" : : \
> > +	"r" ((val)) , "r" ((addr))); })
> > +
> > +#define IO_READ32(addr) ({unsigned int val = 0; __asm__ __volatile__ \
> > +	("ld.di %0,[%1]" : "=&r" ((val)) : "r" ((addr))); val; })
> > +
> > +#define IO_WRITE8(val, addr) ({__asm__ __volatile__ ("stb.di %0,[%1]" : : \
> > +	"r" ((val)), "r" ((addr))); })
> > +
> > +#define IO_READ8(addr) ({unsigned int val = 0; __asm__ __volatile__ \
> > +	("ldb.di %0,[%1]" : "=&r" ((val)) : "r" ((addr))); val; })
> 
> Why would these IOREAD*/IO_WRITE* macros be needed in addition to the
> read*/write* accessors?  Their definitions look pretty much identical
> to me.

Indeed they are obsolete and I'll remove them completely.

> > +#define writeb(val, addr) ({ __asm__ __volatile__ ("stb.di %0,[%1]" : :\
> > +	"r" ((val)), "r" ((addr))); })
> > +
> > +#define writew(val, addr) ({ __asm__ __volatile__ ("stw.di %0,[%1]" : :\
> > +	"r" ((val)), "r" ((addr))); })
> > +
> > +#define writel(val, addr) ({ __asm__ __volatile__ ("st.di %0,[%1]" : :\
> > +	"r" ((val)), "r" ((addr))); })
> > +
> > +#define readb(addr)	({unsigned int val = 0; __asm__ __volatile__ \
> > +	("ldb.di %0,[%1]" : "=&r" ((val)) : "r" ((addr))); val; })
> > +
> > +#define readw(addr) ({unsigned int val = 0; __asm__ __volatile__ \
> > +	("ldw.di %0,[%1]" : "=&r" ((val)) : "r" ((addr))); val; })
> > +
> > +#define readl(addr) ({unsigned int val = 0; __asm__ __volatile__ \
> > +	("ld.di %0,[%1]" : "=&r" ((val)) : "r"((addr))); val; })
> 
> As far as I can tell there is no type checking for the arguments
> passed here.  Can this please be added, so the compiler can catch when
> you for example try to use a writel() on a address that points to char
> only?

Good suggestion, but I'm wondering if this kind of checks are already
implemented in other arches - may I have a reference?

> > +/*
> > + * Generic virtual read/write
> > + */
> > +#define iomem_valid_addr(iomem, sz) (1)
> > +#define iomem_to_phys(iomem)       (iomem)
> > +
> > +#ifdef __io
> > +#define outb(v, p)               __raw_writeb(v, __io(p))
> > +#define outw(v, p)               __raw_writew(cpu_to_le16(v), __io(p))
> > +#define outl(v, p)               __raw_writel(cpu_to_le32(v), __io(p))
> > +
> > +#define inb(p) ({ unsigned int __v = __raw_readb(__io(p)); __v; })
> > +#define inw(p) ({ unsigned int __v = le16_to_cpu(__raw_readw(__io(p))); __v; })
> > +#define inl(p) ({ unsigned int __v = le32_to_cpu(__raw_readl(__io(p))); __v; })
> > +
> > +#define outsb(p, d, l)            __raw_writesb(__io(p), d, l)
> > +#define outsw(p, d, l)            __raw_writesw(__io(p), d, l)
> > +#define outsl(p, d, l)            __raw_writesl(__io(p), d, l)
> > +
> > +#define insb(p, d, l)             __raw_readsb(__io(p), d, l)
> > +#define insw(p, d, l)             __raw_readsw(__io(p), d, l)
> > +#define insl(p, d, l)             __raw_readsl(__io(p), d, l)
> > +#endif
> > +
> > +#define outb_p(val, port)       outb((val), (port))
> > +#define outw_p(val, port)       outw((val), (port))
> > +#define outl_p(val, port)       outl((val), (port))
> > +
> > +#define inb_p(port)            inb((port))
> > +#define inw_p(port)            inw((port))
> > +#define inl_p(port)            inl((port))
> > +
> > +#define outsb_p(port, from, len)  outsb(port, from, len)
> > +#define outsw_p(port, from, len)  outsw(port, from, len)
> > +#define outsl_p(port, from, len)  outsl(port, from, len)
> > +
> > +#define insb_p(port, to, len)     insb(port, to, len)
> > +#define insw_p(port, to, len)     insw(port, to, len)
> > +#define insl_p(port, to, len)     insl(port, to, len)
> 
> Do we actually need any of this in U-Boot?
> 
> Please clean up and remove unused stuff...

Well for example "inb"/"outb" is used in "drivers/serial/ns16550.c" and
in many other places. So we need to clean drivers and common files
first.

> > +/*
> > + * Given a physical address and a length, return a virtual address
> > + * that can be used to access the memory range with the caching
> > + * properties specified by "flags".
> > + */
> > +#define MAP_NOCACHE   (0)
> > +#define MAP_WRCOMBINE (0)
> > +#define MAP_WRBACK    (0)
> > +#define MAP_WRTHROUGH (0)
> 
> Needed in U-Boot?  Please fix globally.

MAP_WRCOMBINE is not used really.
While MAP_NOCACHE and MAP_WRBACK are still in use in couple of places
together with "map_physmem()".

> > +/* Written to pacify arch indepeandant code.
> > + * Not used by ARC I/O
> > + */
> > +#define _inb inb
> > +#define _outb outb
> > +
> > +#define IO_SPACE_LIMIT 0xffff
> > +
> > +#define IOMAP_FULL_CACHING   0
> > +#define IOMAP_NOCACHE_SER    1
> > +#define IOMAP_NOCACHE_NONSER 2
> > +#define IOMAP_WRITETHROUGH   3
> 
> I think you can drop that, too?

will do.
> 
> > +/* Can't use the ENTRY macro in linux/linkage.h
> > + * gas considers ';' as comment vs. newline
> > + */
> > +.macro ARC_ENTRY name
> > +	.global \name
> > +	.align 4
> > +	\name:
> > +.endm
> 
> Is this really correct sytax for this architecture?  I would expect
> that the "\n" means a newline character ;-)

This is GNU Assembler macro syntax -
https://sourceware.org/binutils/docs/as/Macro.html#Macro
That's how you use parameters passed to macro.
So here parameter name is "name" and you use it as "\name" in macro's
body.

And it works which is proven by built and executed both Linux kernel and
U-boot on ARC CPUs.

> > +/*
> > + * This file is generally used by user-level software, so you need to
> > + * be a little careful about namespace pollution etc.  Also, we cannot
> > + * assume GCC is being used.
> > + */
> > +
> > +typedef unsigned short		__kernel_dev_t;
> > +typedef unsigned long		__kernel_ino_t;
> > +typedef unsigned short		__kernel_mode_t;
> > +typedef unsigned short		__kernel_nlink_t;
> > +typedef long			__kernel_off_t;
> > +typedef int			__kernel_pid_t;
> > +typedef unsigned short		__kernel_ipc_pid_t;
> > +typedef unsigned short		__kernel_uid_t;
> > +typedef unsigned short		__kernel_gid_t;
> > +typedef unsigned int		__kernel_size_t;
> > +typedef int			__kernel_ssize_t;
> > +typedef int			__kernel_ptrdiff_t;
> > +typedef long			__kernel_time_t;
> > +typedef long			__kernel_suseconds_t;
> > +typedef long			__kernel_clock_t;
> > +typedef int			__kernel_daddr_t;
> > +typedef char *			__kernel_caddr_t;
> > +typedef unsigned short		__kernel_uid16_t;
> > +typedef unsigned short		__kernel_gid16_t;
> > +typedef unsigned int		__kernel_uid32_t;
> > +typedef unsigned int		__kernel_gid32_t;
> > +
> > +typedef unsigned short		__kernel_old_uid_t;
> > +typedef unsigned short		__kernel_old_gid_t;
> > +
> > +#ifdef __GNUC__
> > +typedef long long		__kernel_loff_t;
> > +#endif
> > +
> > +typedef struct {
> > +#if defined(__KERNEL__) || defined(__USE_ALL)
> > +	int	val[2];
> > +#else /* !defined(__KERNEL__) && !defined(__USE_ALL) */
> > +	int	__val[2];
> > +#endif /* !defined(__KERNEL__) && !defined(__USE_ALL) */
> > +} __kernel_fsid_t;
> > +
> > +#if defined(__KERNEL__) || !defined(__GLIBC__) || (__GLIBC__ < 2)
> > +
> > +#undef	__FD_SET
> > +#define __FD_SET(fd, fdsetp) \
> > +		(((fd_set *)fdsetp)->fds_bits[fd >> 5] |= (1<<(fd & 31)))
> > +
> > +#undef	__FD_CLR
> > +#define __FD_CLR(fd, fdsetp) \
> > +		(((fd_set *)fdsetp)->fds_bits[fd >> 5] &= ~(1<<(fd & 31)))
> > +
> > +#undef	__FD_ISSET
> > +#define __FD_ISSET(fd, fdsetp) \
> > +		((((fd_set *)fdsetp)->fds_bits[fd >> 5] & (1<<(fd & 31))) != 0)
> > +
> 
> Is any of this actually relevant in U-Boot?

Once again - "posix_types.h" are very similar in all arches now and so I
took ARM's one. If everything or some parts there are useless we may
want to clean-up this globally.

But I see that in "include/linux/types.h" "__kernel_XXX_t" types are
used.

> > +#undef	__FD_ZERO
> > +#define __FD_ZERO(fdsetp) \
> > +		(memset (fdsetp, 0, sizeof (*(fd_set *)fdsetp)))
> > +
> > +#endif
> 
> Note that checkpatch throws warnings here:
> 
> WARNING: space prohibited between function name and open parenthesis '('
> 
> Please make sure to run all your patches through checkpatch and fix
> such errors and warnings!

As I mentioned I took this one from ARM so I left it as it is.
I might just add explicit mention that my file is a copy.
Will it work?

> > +#endif	/* __ASM_ARC_POSIX_TYPES_H */
> > 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 */
> 
> Needed?
> 
> > +/* return 1 if user mode or 0 if kernel mode */
> > +#define user_mode(regs) (regs->status32 & STATUS_U_MASK)
> 
> Certainly not needed, right?
> 
> etc. etc.

This is another file taken from Linux kernel source that's why I didn't
touch it at all.

Regards,
Alexey