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

2008-04-19 Thread Josh Boyer
On Fri, 18 Apr 2008 14:55:03 -0700
Stephen Neuendorffer [EMAIL PROTECTED] 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 [EMAIL PROTECTED]

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 000..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
___
Linuxppc-dev mailing list
Linuxppc-dev@ozlabs.org
https://ozlabs.org/mailman/listinfo/linuxppc-dev


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

2008-04-18 Thread Stephen Neuendorffer
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 [EMAIL PROTECTED]
---
 arch/powerpc/sysdev/dcr.c |   91 -
 include/asm-powerpc/dcr-generic.h |   49 
 include/asm-powerpc/dcr-mmio.h|   20 +---
 include/asm-powerpc/dcr-native.h  |   16 ---
 include/asm-powerpc/dcr.h |   36 ++-
 5 files changed, 186 insertions(+), 26 deletions(-)
 create mode 100644 include/asm-powerpc/dcr-generic.h

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);
+}
+EXPORT_SYMBOL_GPL(dcr_unmap_generic);
+
+u32 dcr_read_generic(dcr_host_t host, unsigned int dcr_n)
+{
+   if (host.type == NATIVE)
+   return dcr_read_native(host.host.native, dcr_n);
+   else
+   return dcr_read_mmio(host.host.mmio, dcr_n);
+}
+EXPORT_SYMBOL_GPL(dcr_read_generic);
+
+void dcr_write_generic(dcr_host_t host, unsigned int dcr_n, u32 value)
+{
+   if (host.type == NATIVE)
+   dcr_write_native(host.host.native, dcr_n, value);
+   else
+   dcr_write_mmio(host.host.mmio, dcr_n, value);
+}
+EXPORT_SYMBOL_GPL(dcr_write_generic);
+
+#endif /* defined(CONFIG_PPC_DCR_NATIVE)  defined(CONFIG_PPC_DCR_MMIO) */
+
 unsigned int dcr_resource_start(struct device_node *np, unsigned int index)
 {
unsigned int ds;
@@ -47,7 +109,7 @@ unsigned int dcr_resource_len(struct device_node *np, 
unsigned int index)
 }
 EXPORT_SYMBOL_GPL(dcr_resource_len);
 
-#ifndef CONFIG_PPC_DCR_NATIVE
+#ifdef CONFIG_PPC_DCR_MMIO
 
 static struct device_node * find_dcr_parent(struct device_node * node)
 {
@@ -101,18 +163,19 @@ u64 of_translate_dcr_address(struct device_node *dev,
return ret;
 }
 
-dcr_host_t dcr_map(struct device_node *dev, unsigned int dcr_n,
-  unsigned int dcr_c)
+dcr_host_mmio_t dcr_map_mmio(struct device_node *dev,
+unsigned int dcr_n,
+unsigned int dcr_c)
 {
-   dcr_host_t ret = { .token = NULL, .stride = 0, .base = dcr_n };
+   dcr_host_mmio_t ret = { .token = NULL, .stride = 0, .base = dcr_n };
u64 addr;
 
pr_debug(dcr_map(%s, 0x%x, 0x%x)\n,
 dev-full_name, dcr_n, dcr_c);
 
addr = of_translate_dcr_address(dev, dcr_n, ret.stride);
-   pr_debug(translates to addr: 0x%lx, stride: 0x%x\n,
-addr, ret.stride);
+   pr_debug(translates to addr: 0x%llx, stride: 0x%x\n,
+(unsigned long long) addr, ret.stride);
if (addr == OF_BAD_ADDR)
return ret;
pr_debug(mapping 0x%x bytes\n, dcr_c * ret.stride);
@@ -124,11 +187,11 @@ dcr_host_t dcr_map(struct device_node *dev, unsigned int 
dcr_n,
ret.token -= dcr_n * ret.stride;
  

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

2008-04-18 Thread Benjamin Herrenschmidt

 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.

Not necessarily... yes we tend to prefer functions, but in that case
which is 100% a renaming trick, macros are fine and somewhat simpler.

There is no type difference and the macro will not prevent type checking
here.

Cheers,
Ben.

___
Linuxppc-dev mailing list
Linuxppc-dev@ozlabs.org
https://ozlabs.org/mailman/listinfo/linuxppc-dev