Dear Alexey Brodkin,
In message 1391011745-22239-2-git-send-email-abrodkin@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