[U-Boot] [PATCH v2 1/9] arc: add architecture header files
Alexey Brodkin
abrodkin at synopsys.com
Fri Jan 31 16:57:47 CET 2014
Hello Wolfgang,
On Fri, 2014-01-31 at 00:10 +0100, Wolfgang Denk wrote:
> 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.
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.
More information about the U-Boot
mailing list