Re: [OpenBIOS] [PATCH v2] ppc: Convert I/O macros to inline functions

[Andreas Färber]

Am 21.11.2010 um 14:51 schrieb Alexander Graf:
> On 13.11.2010, at 00:54, Andreas Färber wrote:
> > Suggested by Blue.
> >
> > v2:
> > Make port uint16_t, suggested by Alex.
> > Adapt isa_io_base for ppc64.
> >
> > Cc: Blue Swirl 
> > Cc: Alexander Graf 
> > Signed-off-by: Andreas Färber
> >
> > arch/ppc/qemu/init.c  |    2 +-
> > include/arch/ppc/io.h |   68 +++++++++++++++++++++++++++++++++++ 
> > +------------
> > 2 files changed, 52 insertions(+), 18 deletions(-)
> >
> > diff --git a/arch/ppc/qemu/init.c b/arch/ppc/qemu/init.c
> > index ba215bc..ab1ac69 100644
> > --- a/arch/ppc/qemu/init.c
> > +++ b/arch/ppc/qemu/init.c
> > @@ -152,7 +152,7 @@ static const pci_arch_t known_arch[] = {
> >        .irqs = { 21, 22, 23, 24 }
> >    },
> > };
> > -uint32_t isa_io_base;
> > +uintptr_t isa_io_base;
> >
> > void
> > entry( void )
> > diff --git a/include/arch/ppc/io.h b/include/arch/ppc/io.h
> > index e5180f2..d8f9dcc 100644
> > --- a/include/arch/ppc/io.h
> > +++ b/include/arch/ppc/io.h
> > @@ -14,23 +14,7 @@ extern unsigned long virt_offset;
> >
> > #ifndef BOOTSTRAP
> >
> > -extern uint32_t isa_io_base;
> >
> > -/*
> > The insw/outsw/insl/outsl macros don't do byte-swapping.
> > They are only used in practice for transferring buffers which
> > are arrays of bytes, and byte-swapping is not appropriate in
> > that case.  - paulus
> > */
> >
> > -#define insw(port, buf, ns)	_insw((uint16_t *)(uintptr_t)((port) 
> > +isa_io_base), (buf), (ns))
> > -#define outsw(port, buf, ns)	_outsw((uint16_t *)(uintptr_t)((port) 
> > +isa_io_base), (buf), (ns))
> >
> > -#define inb(port)		in_8((uint8_t *)(uintptr_t)((port)+isa_io_base))
> > -#define outb(val, port)		out_8((uint8_t *)(uintptr_t)((port) 
> > +isa_io_base), (val))
> > -#define inw(port)		in_le16((uint16_t *)(uintptr_t)((port) 
> > +isa_io_base))
> > -#define outw(val, port)		out_le16((uint16_t *)(uintptr_t)((port) 
> > +isa_io_base), (val))
> > -#define inl(port)		in_le32((uint32_t *)(uintptr_t)((port) 
> > +isa_io_base))
> > -#define outl(val, port)		out_le32((uint32_t *)(uintptr_t)((port) 
> > +isa_io_base), (val))
> > +extern uintptr_t isa_io_base;
> >
> > /*
> > 8, 16 and 32 bit, big and little endian I/O operations, with
> >
> > barrier.
> > @@ -145,6 +129,56 @@ static inline void _outsw(volatile uint16_t *  
> > port, const void *buf,
> >    	ns--;
> >    }
> > }
> >
> > +/*
> > The insw/outsw/insl/outsl functions don't do byte-swapping.
> > They are only used in practice for transferring buffers which
> > are arrays of bytes, and byte-swapping is not appropriate in
> > that case.  - paulus
> > */
> >
> > +static inline void insw(uint16_t port, void *buf, int ns)
> > +{
> > _insw((uint16_t *)(port + isa_io_base), buf, ns);
> >
> > +}
> >
> > +static inline void outsw(uint16_t port, void *buf, int ns)
> > +{
> > _outsw((uint16_t *)(port + isa_io_base), buf, ns);
> >
> > +}
> >
> > +static inline int inb(uint16_t port)
>
> Shouldn't inb return an uint8_t?
> > +{
> > return in_8((uint8_t *)(port + isa_io_base));
> >
> > +}
> >
> > +static inline void outb(int val, uint16_t port)
> > +{
> > out_8((uint8_t *)(port + isa_io_base), val);
> >
> > +}
> >
> > +static inline int inw(uint16_t port)
>
> I'd expect uint16_t to be returned here.
> > +{
> > return in_le16((uint16_t *)(port + isa_io_base));
> >
> > +}
> >
> > +static inline void outw(int val, uint16_t port)
> > +{
> > out_le16((uint16_t *)(port + isa_io_base), val);
> >
> > +}
> >
> > +static inline unsigned inl(uint16_t port)
>
> and uint32_t here.
> > +{
> > return in_le32((uint32_t *)(port + isa_io_base));
> >
> > +}
> >
> > +static inline void outl(int val, uint16_t port)
>
> The same goes for the out values. They should only be as big as they  
> can really be and preferably unsigned :).

I used the types used by the functions my functions wrap. I noticed  
that myself but wanted to keep the patch as small as possible. I'll  
change the existing functions then.

Andreas
> The rest of it looks very nice!
>
> Alex