Re: [PATCH v9 01/17] h8300: Assembly headers.

[Yoshinori Sato <ysato@users.sourceforge.jp>]

At Mon, 27 Apr 2015 10:40:51 +0200,
Arnd Bergmann wrote:
> 
> On Monday 27 April 2015 14:35:08 Yoshinori Sato wrote:
> > diff --git a/arch/h8300/include/asm/asm-offsets.h b/arch/h8300/include/asm/asm-offsets.h
> > new file mode 100644
> > index 0000000..d370ee3
> > --- /dev/null
> > +++ b/arch/h8300/include/asm/asm-offsets.h
> > @@ -0,0 +1 @@
> > +#include <generated/asm-offsets.h>
> 
> Could you add a file with these contents to include/asm-generic and use that
> instead of providing your own copy?

OK.

> > diff --git a/arch/h8300/include/asm/delay.h b/arch/h8300/include/asm/delay.h
> > new file mode 100644
> > index 0000000..2bdde59
> > --- /dev/null
> > +++ b/arch/h8300/include/asm/delay.h
> > @@ -0,0 +1,38 @@
> > +#ifndef _H8300_DELAY_H
> > +#define _H8300_DELAY_H
> > +
> > +#include <asm/param.h>
> > +
> > +/*
> > + * Copyright (C) 2002 Yoshinori Sato <ysato@sourceforge.jp>
> > + *
> > + * Delay routines, using a pre-computed "loops_per_second" value.
> > + */
> > +
> > +static inline void __delay(unsigned long loops)
> > +{
> > +	__asm__ __volatile__ ("1:\n\t"
> > +			      "dec.l #1,%0\n\t"
> > +			      "bne 1b"
> > +			      : "=r" (loops) : "0"(loops));
> > +}
> > +
> 
> This could be optimized by using the clocksource instead, if that is
> accurate enough. Doing so will speed up the boot as well, because you
> can avoid calibrating the delay loop.

OK.
remove it.

> 
> > +#endif /* _H8300_DELAY_H */
> > diff --git a/arch/h8300/include/asm/device.h b/arch/h8300/include/asm/device.h
> > new file mode 100644
> > index 0000000..06746c5
> > --- /dev/null
> > +++ b/arch/h8300/include/asm/device.h
> > @@ -0,0 +1,6 @@
> > +/*
> > + * Arch specific extensions to struct device
> > + *
> > + * This file is released under the GPLv2
> > + */
> > +#include <asm-generic/device.h>
> 
> This one can obviously just use 'generic-y' isntead of including the file.
> 
> > diff --git a/arch/h8300/include/asm/emergency-restart.h b/arch/h8300/include/asm/emergency-restart.h
> > new file mode 100644
> > index 0000000..108d8c4
> > --- /dev/null
> > +++ b/arch/h8300/include/asm/emergency-restart.h
> > @@ -0,0 +1,6 @@
> > +#ifndef _ASM_EMERGENCY_RESTART_H
> > +#define _ASM_EMERGENCY_RESTART_H
> > +
> > +#include <asm-generic/emergency-restart.h>
> > +
> > +#endif /* _ASM_EMERGENCY_RESTART_H */
> 
> Same here.

OK.

> > diff --git a/arch/h8300/include/asm/io.h b/arch/h8300/include/asm/io.h
> > new file mode 100644
> > index 0000000..51ee096
> > --- /dev/null
> > +++ b/arch/h8300/include/asm/io.h
> > @@ -0,0 +1,314 @@
> > +#ifndef _H8300_IO_H
> > +#define _H8300_IO_H
> > +
> > +#ifdef __KERNEL__
> > +
> > +#include <linux/types.h>
> > +
> > +#define __raw_readb(addr) ({ u8 __v = *(volatile u8 *)(addr); __v; })
> > +
> > +#define __raw_readw(addr) ({ u16 __v = *(volatile u16 *)(addr); __v; })
> > +
> > +#define __raw_readl(addr) ({ u32 __v = *(volatile u32 *)(addr); __v; })
> > +
> > +#define __raw_writeb(b, addr) (void)((*(volatile u8 *)(addr)) = (b))
> > +
> > +#define __raw_writew(b, addr) (void)((*(volatile u16 *)(addr)) = (b))
> > +
> > +#define __raw_writel(b, addr) (void)((*(volatile u32 *)(addr)) = (b))
> > +
> > +#define readb __raw_readb
> > +#define readw __raw_readw
> > +#define readl __raw_readl
> > +#define writeb __raw_writeb
> > +#define writew __raw_writew
> > +#define writel __raw_writel
> 
> We have recently changed this so you now have to provide readl_relaxed()
> and writel_relaxed() as well.

OK.

> As a side-note: The current definition here prevents you from ever
> using PCI devices, which are by definition little-endian. If there is
> any chance that you might need to support PCI devices later, better
> don't use the readl/writel family of accessors for platform specific
> drivers and use ioread_be32/iowrite_be32 (or an h8300-specific variant
> thereof) for any big-endian devices, and define read() etc to do a
> byte swap.

PCI is not supported.

> > +#if defined(CONFIG_H83069)
> > +#define ABWCR  0xFEE020
> > +#elif defined(CONFIG_H8S2678)
> > +#define ABWCR  0xFFFEC0
> > +#endif
> > +
> > +#ifdef CONFIG_H8300_BUBSSWAP
> > +#define _swapw(x) __builtin_bswap16(x)
> > +#define _swapl(x) __builtin_bswap32(x)
> > +#else
> > +#define _swapw(x) (x)
> > +#define _swapl(x) (x)
> > +#endif
> 
> Is this swapping configurable by software? The best way to do this
> is normally not to do any bus swapping in hardware at all but
> let the drivers take care of it, otherwise you will get things
> wrong in the end.
> 
> > +static inline void io_outsw(unsigned int addr, const void *buf, int len)
> > +{
> > +	volatile unsigned short *ap = (volatile unsigned short *) addr;
> > +	unsigned short *bp = (unsigned short *) buf;
> > +
> > +	while (len--)
> > +		*ap = _swapw(*bp++);
> > +}
> > +
> > +static inline void io_outsl(unsigned int addr, const void *buf, int len)
> > +{
> > +	volatile unsigned short *ap = (volatile unsigned short *) addr;
> > +	unsigned short *bp = (unsigned short *) buf;
> > +
> > +	while (len--) {
> > +		*(ap + 1) = _swapw(*(bp + 0));
> > +		*(ap + 0) = _swapw(*(bp + 1));
> > +		bp += 2;
> > +	}
> > +}
> 
> In particular, the outsw/insw/readsw/writesw/iowrite16_rep/ioread16_rep()
> family of function is assumed to never perform any byte swapping, because
> they are used to access FIFO registers from a lot of the drivers. These
> FIFOs normally contain byte streams in memory order, and Linux drivers
> are written to assume that normal mmio registers may need byte swaps while
> fifo registers don't.
> 
> What you have here is basically doing the opposite: you assume that
> mmio registers are configured to be the same endianess as the CPU
> by using an extra hardware bus swap, while the fifos will need to be
> swapped back as a side effect of the hardware swap.
> 
> This can work if none of your drivers are shared with other architectures,
> but the more you share, the more problems it will cause.

It not used functions.
Removed.

> > +
> > +#define inb(addr)    ((h8300_buswidth(addr)) ? \
> > +		      __raw_readw((addr) & ~1) & 0xff:__raw_readb(addr))
> > +#define inw(addr)    _swapw(__raw_readw(addr))
> > +#define inl(addr)    (_swapw(__raw_readw(addr) << 16 | \
> > +			     _swapw(__raw_readw(addr + 2))))
> > +#define outb(x, addr) ((void)((h8300_buswidth(addr) && ((addr) & 1)) ? \
> > +			      __raw_writeb(x, (addr) & ~1) : \
> > +			      __raw_writeb(x, addr)))
> > +#define outw(x, addr) ((void) __raw_writew(_swapw(x), addr))
> > +#define outl(x, addr) \
> > +		((void) __raw_writel(_swapw(x & 0xffff) | \
> > +				      _swapw(x >> 16) << 16, addr))
> > +
> > +#define inb_p(addr)    inb(addr)
> > +#define inw_p(addr)    inw(addr)
> > +#define inl_p(addr)    inl(addr)
> > +#define outb_p(x, addr) outb(x, addr)
> > +#define outw_p(x, addr) outw(x, addr)
> > +#define outl_p(x, addr) outl(x, addr)
> > +
> > +#define outsb(a, b, l) io_outsb(a, b, l)
> > +#define outsw(a, b, l) io_outsw(a, b, l)
> > +#define outsl(a, b, l) io_outsl(a, b, l)
> > +
> > +#define insb(a, b, l) io_insb(a, b, l)
> > +#define insw(a, b, l) io_insw(a, b, l)
> > +#define insl(a, b, l) io_insl(a, b, l)
> 
> It's probably better not to define these macros if you do not have
> PCI devices. They have a very specific meaning on PCI, and you
> don't seem to use them this way.

PCI not use.
Removed it.

> 
> > +#define IO_SPACE_LIMIT 0xffffff
> 
> In particular, you don't enforce the IO_SPACE_LIMIT for the
> addresses: What you normally have is one part of the physical
> address space that maps to PCI I/O ports, so your inb would look
> like
> 
> #define inb(addr) (readl(IO_SPACE_START + (addr & IO_SPACE_LIMIT))

OK.

> 
> > +#define ioread8(a)		__raw_readb(a)
> > +#define ioread16(a)		__raw_readw(a)
> > +#define ioread32(a)		__raw_readl(a)
> > +
> > +#define iowrite8(v, a)		__raw_writeb((v), (a))
> > +#define iowrite16(v, a)		__raw_writew((v), (a))
> > +#define iowrite32(v, a)		__raw_writel((v), (a))
> > +
> > +#define ioread8_rep(p, d, c)	insb(p, d, c)
> > +#define ioread16_rep(p, d, c)	insw(p, d, c)
> > +#define ioread32_rep(p, d, c)	insl(p, d, c)
> > +#define iowrite8_rep(p, s, c)	outsb(p, s, c)
> > +#define iowrite16_rep(p, s, c)	outsw(p, s, c)
> > +#define iowrite32_rep(p, s, c)	outsl(p, s, c)
> 
> 
> > diff --git a/arch/h8300/include/asm/smp.h b/arch/h8300/include/asm/smp.h
> > new file mode 100644
> > index 0000000..9e9bd7e
> > --- /dev/null
> > +++ b/arch/h8300/include/asm/smp.h
> > @@ -0,0 +1 @@
> > +/* nothing required here yet */
> > diff --git a/arch/h8300/include/asm/spinlock.h b/arch/h8300/include/asm/spinlock.h
> > new file mode 100644
> > index 0000000..d5407fa
> > --- /dev/null
> > +++ b/arch/h8300/include/asm/spinlock.h
> > @@ -0,0 +1,6 @@
> > +#ifndef __H8300_SPINLOCK_H
> > +#define __H8300_SPINLOCK_H
> > +
> > +#error "H8/300 doesn't do SMP yet"
> > +
> > +#endif
> 
> generic file?

Yes.

> 
> > diff --git a/arch/h8300/include/asm/topology.h b/arch/h8300/include/asm/topology.h
> > new file mode 100644
> > index 0000000..fdc1219
> > --- /dev/null
> > +++ b/arch/h8300/include/asm/topology.h
> > @@ -0,0 +1,6 @@
> > +#ifndef _ASM_H8300_TOPOLOGY_H
> > +#define _ASM_H8300_TOPOLOGY_H
> > +
> > +#include <asm-generic/topology.h>
> > +
> > +#endif /* _ASM_H8300_TOPOLOGY_H */
> 
> same here
> 
> 	Arnd
> 

-- 
Yoshinori Sato
<ysato@users.sourceforge.jp>