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

[Josh Boyer <jwboyer@linux.vnet.ibm.com>]

On Fri, 18 Apr 2008 14:55:03 -0700
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.
> 
> Signed-off-by: Stephen Neuendorffer <stephen.neuendorffer@xilinx.com>

Hi Stephen,

Sorry for the late review.  See some comments below.  Mostly minor
stuff and I think the general direction here is good.

> diff --git a/arch/powerpc/sysdev/dcr.c b/arch/powerpc/sysdev/dcr.c
> index 437e48d..d3de0ff 100644
> --- a/arch/powerpc/sysdev/dcr.c
> +++ b/arch/powerpc/sysdev/dcr.c
> @@ -23,6 +23,68 @@
>  #include <asm/prom.h>
>  #include <asm/dcr.h>
> 
> +#if defined(CONFIG_PPC_DCR_NATIVE) && defined(CONFIG_PPC_DCR_MMIO)
> +
> +bool dcr_map_ok_generic(dcr_host_t host)
> +{
> +	if (host.type == INVALID)
> +		return 0;
> +	else if (host.type == NATIVE)
> +		return dcr_map_ok_native(host.host.native);
> +	else
> +		return dcr_map_ok_mmio(host.host.mmio);
> +}
> +EXPORT_SYMBOL_GPL(dcr_map_ok_generic);
> +
> +dcr_host_t dcr_map_generic(struct device_node *dev,
> +			   unsigned int dcr_n,
> +			   unsigned int dcr_c)
> +{
> +	dcr_host_t host;
> +	const char *prop = of_get_property(dev, "dcr-access-method", NULL);
> +
> +	if (!strcmp(prop, "native")) {
> +		host.type = NATIVE;
> +		host.host.native = dcr_map_native(dev, dcr_n, dcr_c);
> +	} else if (!strcmp(prop, "mmio")) {
> +		host.type = MMIO;
> +		host.host.mmio = dcr_map_mmio(dev, dcr_n, dcr_c);
> +	} else
> +		host.type = INVALID;
> +
> +	return host;
> +}
> +EXPORT_SYMBOL_GPL(dcr_map_generic);
> +
> +void dcr_unmap_generic(dcr_host_t host, unsigned int dcr_c)
> +{
> +	if (host.type == NATIVE)
> +		dcr_unmap_native(host.host.native, dcr_c);
> +	else
> +		dcr_unmap_mmio(host.host.mmio, dcr_c);

What happens if host.type == INVALID?  Same question for the other
accessors in dcr_*_generic.

<snip>

> diff --git a/include/asm-powerpc/dcr-generic.h b/include/asm-powerpc/dcr-generic.h
> new file mode 100644
> index 0000000..0ee74fb
> --- /dev/null
> +++ b/include/asm-powerpc/dcr-generic.h
> @@ -0,0 +1,49 @@

<snip> 

> +enum host_type_t {MMIO, NATIVE, INVALID};

Should these be DCR_HOST_MMIO, DCR_HOST_NATIVE, DCR_HOST_INVALID?

I worry about the generic nature of the names.

josh