Re: [PATCH 07/28] nios2: I/O Mapping

[Ley Foon Tan]

On Tue, Apr 22, 2014 at 9:59 PM, Arnd Bergmann <arnd@xxxxxxxx> wrote:
> On Friday 18 April 2014, Ley Foon Tan wrote:
>
>> +
>> +#include <asm/pgtable-bits.h>
>> +
>> +#define IO_SPACE_LIMIT 0xffffffff
>
> Please use 0xffff here, this should work for almost any PCI bus.
Ah, CONFIG_PCI is not enable in nios2. So, I think IO_SPACE_LIMIT
should set to 0.


>> +
>> +/* Use "Duff's Device" to unroll the loops. */
>> +#define __IO_OUT_LOOP(a, b, l)                               \
>> +     do {                                            \
>> +             if (l > 0) {                            \
>> +                     int _n = (l + 7) / 8;           \
>> +                     switch (l % 8) {                \
>> +                     case 0:                         \
>> +                             do {                    \
>> +                                     *a = *b++;      \
>
> I would recommend just doing all of this in out-of-line implementations
> rather than macros and inline functions.
Okay. Will move the macros to out-of-line function.


>
>> +
>> +/*
>> + *   make the short names macros so specific devices
>> + *   can override them as required
>> + */
>> +#define inb(addr)            readb(addr)
>> +#define inw(addr)            readw(addr)
>> +#define inl(addr)            readl(addr)
>> +#define outb(x, addr)                ((void) writeb(x, addr))
>> +#define outw(x, addr)                ((void) writew(x, addr))
>> +#define outl(x, addr)                ((void) writel(x, addr))
>
> This makes no sense: the legacy PC I/O accessors take a completely
> different argument type: I would recommend to make both inline
> functions so you can ensure that readl() gets passed an __iomem
> pointer, while inl() gets an integer number.
>
> Please see the asm-generic version for an example. You should also
> define a non-NULL PCI_IOBASE to which the PCI I/O space gets mapped.
I think these macros can be removed if PCI is not enabled in Nios2.

>
>> +#define virt_to_bus virt_to_phys
>> +#define bus_to_virt phys_to_virt
>
> Please drop these, and the CONFIG_VIRT_TO_BUS Kconfig option,
> and fix all drivers that rely on it.
Okay. Will remove these.

>
>> +#define ioport_map(port, nr) ioremap(port, nr)
>> +#define ioport_unmap(port)   iounmap(port)
>
> Use this one instead:
>
> #ifdef CONFIG_PCI
> static inline void __iomem *ioport_map(unsigned long port, unsigned int nr)
> {
>         return PCI_IOBASE + port;
> }
>
> static inline void __iomem *ioport_unmap(void __iomem *)
> {
> }
> #endif
These also can be removed.

>
>> +/* Macros used for smc91x.c driver */
>> +#define readsb(p, d, l)              insb(p, d, l)
>> +#define readsw(p, d, l)              insw(p, d, l)
>> +#define readsl(p, d, l)              insl(p, d, l)
>> +#define writesb(p, d, l)     outsb(p, d, l)
>> +#define writesw(p, d, l)     outsw(p, d, l)
>> +#define writesl(p, d, l)     outsl(p, d, l)
>
> These should of course not fall back to the PCI I/O space functions.
> You can do it the other way round.
Okay, these will change to something like:
#define readsb(p, d, l)              io_insb(p, d, l)
#define readsw(p, d, l)              io_insw(p, d, l)
(etc)

Regards
Ley Foon
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/