Re: [PATCH 1/2] [v3][POWERPC] refactor dcr code

["Grant Likely" <grant.likely@secretlab.ca>]

On Fri, Apr 18, 2008 at 3:55 PM, Stephen Neuendorffer
<stephen.neuendorffer@xilinx.com> wrote:
> Previously, dcr support was configured at compile time to either using
>  MMIO or native dcr instructions.  Although this works for most
>  platforms, it fails on FPGA platforms:
>
>  1) Systems may include more than one dcr bus.
>  2) Systems may be native dcr capable and still use memory mapped dcr interface.
>
>  This patch provides runtime support based on the device trees for the
>  case where CONFIG_PPC_DCR_MMIO and CONFIG_PPC_DCR_NATIVE are both
>  selected.  Previously, this was a poorly defined configuration, which
>  happened to provide NATIVE support.  The runtime selection is made
>  based on the dcr controller having a 'dcr-access-method' attribute
>
> in the device tree.  If only one of the above options is selected,
>  then the code uses #defines to select only the used code in order to
>  avoid introducing overhead in existing usage.

I'm not really qualified to ack this patch, but it looks right to me;
except for one comment below (sorry I didn't get to looking at this
earlier).


>  diff --git a/include/asm-powerpc/dcr.h b/include/asm-powerpc/dcr.h
>  index 9338d50..6b86322 100644
>  --- a/include/asm-powerpc/dcr.h
>  +++ b/include/asm-powerpc/dcr.h
>  @@ -20,14 +20,47 @@
>   #ifndef _ASM_POWERPC_DCR_H
>   #define _ASM_POWERPC_DCR_H
>   #ifdef __KERNEL__
>  +#ifndef __ASSEMBLY__
>   #ifdef CONFIG_PPC_DCR
>
>   #ifdef CONFIG_PPC_DCR_NATIVE
>   #include <asm/dcr-native.h>
>  -#else
>  +#endif
>  +
>  +#ifdef CONFIG_PPC_DCR_MMIO
>   #include <asm/dcr-mmio.h>
>   #endif
>
>  +#if defined(CONFIG_PPC_DCR_NATIVE) && defined(CONFIG_PPC_DCR_MMIO)
>  +
>  +#include <asm/dcr-generic.h>
>  +
>  +#define DCR_MAP_OK(host)       dcr_map_ok_generic(host)
>  +#define dcr_map(dev, dcr_n, dcr_c) dcr_map_generic(dev, dcr_n, dcr_c)
>  +#define dcr_unmap(host, dcr_c) dcr_unmap_generic(host, dcr_c)
>  +#define dcr_read(host, dcr_n) dcr_read_generic(host, dcr_n)
>  +#define dcr_write(host, dcr_n, value) dcr_write_generic(host, dcr_n, value)
>  +
>  +#else
>  +
>  +#ifdef CONFIG_PPC_DCR_NATIVE
>  +typedef dcr_host_native_t dcr_host_t;
>  +#define DCR_MAP_OK(host)       dcr_map_ok_native(host)
>  +#define dcr_map(dev, dcr_n, dcr_c) dcr_map_native(dev, dcr_n, dcr_c)
>  +#define dcr_unmap(host, dcr_c) dcr_unmap_native(host, dcr_c)
>  +#define dcr_read(host, dcr_n) dcr_read_native(host, dcr_n)
>  +#define dcr_write(host, dcr_n, value) dcr_write_native(host, dcr_n, value)
>  +#else
>  +typedef dcr_host_mmio_t dcr_host_t;
>  +#define DCR_MAP_OK(host)       dcr_map_ok_mmio(host)
>  +#define dcr_map(dev, dcr_n, dcr_c) dcr_map_mmio(dev, dcr_n, dcr_c)
>  +#define dcr_unmap(host, dcr_c) dcr_unmap_mmio(host, dcr_c)
>  +#define dcr_read(host, dcr_n) dcr_read_mmio(host, dcr_n)
>  +#define dcr_write(host, dcr_n, value) dcr_write_mmio(host, dcr_n, value)
>  +#endif
>  +
>  +#endif /* defined(CONFIG_PPC_DCR_NATIVE) && defined(CONFIG_PPC_DCR_MMIO) */
>  +

By current conventions; these should probably be static functions (but
don't make them inline).  The compiler will do the right thing with
them.  Functions are easier to validate by the compiler and sparse
than #defines.

Cheers,
g.

-- 
Grant Likely, B.Sc., P.Eng.
Secret Lab Technologies Ltd.