[U-Boot] [PATCH v2 1/9] arc: add architecture header files
Wolfgang Denk
wd at denx.de
Wed Jan 29 20:54:16 CET 2014
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
> +};
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.
> +/* DSP Options Ref Manual */
> +struct bcr_extn_mac_mul {
> +#ifdef CONFIG_CPU_BIG_ENDIAN
> + unsigned int pad:16, type:8, ver:8;
> +#else
> + unsigned int ver:8, type:8, pad:16;
> +#endif
> +};
> +
> +struct bcr_extn_xymem {
> +#ifdef CONFIG_CPU_BIG_ENDIAN
> + unsigned int ram_org:2, num_banks:4, bank_sz:4, ver:8;
> +#else
> + unsigned int ver:8, bank_sz:4, num_banks:4, ram_org:2;
> +#endif
> +};
..
etc.
With all these tons of bitfields the code will be a mess to read and
understand. Please really try and get rid of using bitfields!
> +/*
> + *******************************************************************
> + * Generic structures to hold build configuration used at runtime
> + */
Incorrect multiline comment style. Please fix globally.
> +struct cpuinfo_arc_mmu {
> + unsigned int ver, pg_sz, sets, ways, u_dtlb, u_itlb, num_tlb;
> +};
Please declare structs one field per line, and comment the meaning of
the fields. Please fix globally.
> +#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...
> +#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.
> +#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?
> +/*
> + * 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...
> +/*
> + * 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.
> +/* 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?
> +/* 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 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?
> +#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!
> +#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.
Best regards,
Wolfgang Denk
--
DENX Software Engineering GmbH, MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
The price of success is hard work, dedication to the job at hand, and
the determination that whether we win or lose, we have applied the
best of ourselves to the task at hand. - Vince Lombardi
More information about the U-Boot
mailing list