Hello Wolfgang, On Fri, 2014-01-31 at 00:10 +0100, Wolfgang Denk wrote: > Dear Alexey, > > In message <[email protected]> > you wrote: > > > > 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? > > This is difficult to impossible for me to decide yet. MY general > feeling is that there is an overwhelming amount of code that is > completely unused in U-Boot. That should really be dropped. > > Also, it would help if you could give reasons (I mean, other than > "this comes form Linux, so it must be good") why this code should be > written as it is. > > Or, for example, if it is realistic to expect that both BE and LE > configurations of your systems will be supported in U-Boot, etc.
Your comments and reasons make perfect sense so I think I'll do a massive clean-up of those headers. It will definitely benefit U-Boot and hopefully then similar clean-up will be accepted in Linux - everybody will be happy. And indeed I do expect to support both LE and BE flavors of ARC CPUs. Because right away one of the active users (I mean company here not individual) use BE ARC CPU. > > 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/byteor> > > der.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")? > > IIRC it might go away in the context of the Kconfig changes. Good. I fixed mine, hope others will follow. > > > > +#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? > > It should be sufficient to use inline functions instead of macros; for > example something like > > extern inline u8 readb(const volatile unsigned char __iomem *addr) > { > u8 val; > > __asm__ __volatile__( > "ldb.di %0,[%1]" : "=&r" ((val)) : "r" ((addr))); > > return val; > } Good suggestion. Will definitely replace macros with in-lined functions. Still I don't quite understand how this implementation helps with data type checking. I may feed any address and compiler happily puts 1 byte aligned address as argument for word load/store instruction. I'm not sure if this is a flaw of our GCC port or I'm doing something wrong here. > > > > +/* > > > > + * 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. > > Are you sure all of these are actually used? Well, most of these are used here and there. And the problem I see is we have many very similar accessors in U-Boot. And people randomly (or based on their preference) use either of them. Let's look at "outb". Different arches define it as an alias for "writeb", "__raw_writeb", "out_8" etc. Now I see "outb" is used in "drivers/serial/ns16550.c" as I mentioned. And this is one of the most common serial port devices right? "writeb" is used everywhere. "__raw_writeb" is used in "drivers/mtd/cfi_flash.c" And the same picture with other accessors. It would be good if we re-visit this topic and come up with some guide that explains which accessers could be used and in which cases. In current situation it looks like every arch sort of emulates accessors of other platform. For example this is a comment from "io.h" for "SH" architecture: ===== The {in,out}[bwl] macros are for emulating x86-style PCI/ISA IO space ===== And indeed I may remove these "in/out" accessors from "ARC" port. But as a maintainer/submitter of ARC port I'd like my users to have access to all (or at least most of) already existing and working drivers and common things (like commands etc). But IMHO it will be unfair. ARC users will complain on "buggy"/obsolete drivers while users of all other architectures will rest in pease and just use existing code-base. That's why I would prefer to have an accessors clean-up as a separate movement among all arches at once - so everybody gets affected and many hands will start fixing stuff all together. > > > > +/* > > > > + * 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()". > > So drop the unused, please... Already did. > > > > +/* 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. > > It appears you avoid to answer my questions. Referring to existing > sub-optimal code is not really an answer. But you are right, if othe > rarchitectures include such stuff, they should be cleaned up, too. > > Patches are always welcome. Sure there're questions I don't have a good technical answer for. I have to confess that I'm not an expert of each and every tiny bit of U-boot. But I see that those types in Linux sources are defined in single one header "include/linux/types.h". And all arches use it. >From this I can make a conclusion that each working implementation rom other architecture will work for ARC. So did I - copied and it worked. I would say that in U-Boot we need to unify lots of things - for example unified init sequences is a good movement but even there I see tons of ifdefs for each and other corner case. So if I want to fix every flaw of existing U-Boot that I met during implementation of my port I would spend many months on this work while ARC port still won't be in upstream. That's why I decided to fix non-ARC sources that block ARC port form being useful (more than 10 patches were accepted) and then submit ARC part which looks as much as possible similarly to existing architectures. This similarity will allow even automatic batch fixing of things. Otherwise each time you'll need to figure out what happen here and there and why there're different implementations of similar or even same things. Saying all this I'm not withdrawing from doing all these fixes anytime later. As an active user and contributor I'm interested in having U-boot sources in good shape - it benefits me in the end. > > > 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? > > No. Everybody can make mistakes. But to knowingly copy poor code > would be... well, no. Understood. > > > > +#endif /* __ASM_ARC_POSIX_TYPES_H */ > > > > diff --git a/arch/arc/include/asm/ptrace.h > > > > b/arch/arc/include/asm/ptrac> e.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. > > Maybe we can take only the parts that are actually needed or useful? As with "arcregs.h" I'll clean this one up heavily so it has only useful pieces. Regard, Alexey _______________________________________________ U-Boot mailing list [email protected] http://lists.denx.de/mailman/listinfo/u-boot
