[U-Boot] [PATCH v2 1/9] arc: add architecture header files
Wolfgang Denk
wd at denx.de
Fri Jan 31 00:10:53 CET 2014
Dear Alexey,
In message <1391088780.3518.33.camel at abrodkin-8560l.internal.synopsys.com> 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.
> 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.
> > > +#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;
}
> > > +/*
> > > + * 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?
> > > +/*
> > > + * 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...
> > > +/* 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.
> > 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.
> > > +#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?
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
It's a small world, but I wouldn't want to paint it.
More information about the U-Boot
mailing list