* RE: [PATCH 1/5] [v2][POWERPC] refactor dcr code
2008-03-30 22:02 ` Benjamin Herrenschmidt
@ 2008-04-01 20:16 ` Stephen Neuendorffer
2008-04-04 22:02 ` Stephen Neuendorffer
` (3 subsequent siblings)
4 siblings, 0 replies; 45+ messages in thread
From: Stephen Neuendorffer @ 2008-04-01 20:16 UTC (permalink / raw)
To: benh; +Cc: linuxppc-dev
I considered function pointers as well, but I think something like
dcr-generic.h is still required in order to get the specialization if
only one is selected.
Also note that I neglected to change the patch description: The
dcr-access-method attribute is applied to the dcr-controller node, not
the dcr slave.
I'd particularly appreciate it if someone with access to PPC64 would
test this...
Steve
> -----Original Message-----
> From: Benjamin Herrenschmidt [mailto:benh@kernel.crashing.org]
> Sent: Sunday, March 30, 2008 3:03 PM
> To: Stephen Neuendorffer
> Cc: linuxppc-dev@ozlabs.org
> Subject: Re: [PATCH 1/5] [v2][POWERPC] refactor dcr code
>=20
>=20
> On Fri, 2008-03-28 at 09:23 -0700, Stephen Neuendorffer 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 slave device 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 interoducing overhead in existing usage.
> >
> > Signed-off-by: Stephen Neuendorffer
<stephen.neuendorffer@xilinx.com>
>=20
> Looks good. Haven't had a chance to test it yet, but tentatively
>=20
> Acked-by: Benjamin Herrenschmidt <benh@kernel.crashing.org>
>=20
> Initially, I thought about using function pointers but the if/else
will
> probably end up being more efficient. The only thing maybe to ponder
> is whether we could avoid the dcr-generic.h file completely, and have
> the generic wrappers be inline, though considering how slow DCRs are,
> it may not be that useful in practice and less clean...
>=20
> Cheers,
> Ben.
>=20
>=20
> > ---
> > 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 =3D=3D INVALID)
> > + return 0;
> > + else if (host.type =3D=3D 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 =3D of_get_property(dev, "dcr-access-method",
NULL);
> > +
> > + if (!strcmp(prop, "native")) {
> > + host.type =3D NATIVE;
> > + host.host.native =3D dcr_map_native(dev, dcr_n, dcr_c);
> > + } else if (!strcmp(prop, "mmio")) {
> > + host.type =3D MMIO;
> > + host.host.mmio =3D dcr_map_mmio(dev, dcr_n, dcr_c);
> > + } else
> > + host.type =3D INVALID;
> > +
> > + return host;
> > +}
> > +EXPORT_SYMBOL_GPL(dcr_map_generic);
> > +
> > +void dcr_unmap_generic(dcr_host_t host, unsigned int dcr_c)
> > +{
> > + if (host.type =3D=3D 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 =3D=3D 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 =3D=3D 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 =3D { .token =3D NULL, .stride =3D 0, .base =3D =
dcr_n };
> > + dcr_host_mmio_t ret =3D { .token =3D NULL, .stride =3D 0, .base =
=3D
dcr_n };
> > u64 addr;
> >
> > pr_debug("dcr_map(%s, 0x%x, 0x%x)\n",
> > dev->full_name, dcr_n, dcr_c);
> >
> > addr =3D 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 =3D=3D 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 -=3D dcr_n * ret.stride;
> > return ret;
> > }
> > -EXPORT_SYMBOL_GPL(dcr_map);
> > +EXPORT_SYMBOL_GPL(dcr_map_mmio);
> >
> > -void dcr_unmap(dcr_host_t host, unsigned int dcr_c)
> > +void dcr_unmap_mmio(dcr_host_mmio_t host, unsigned int dcr_c)
> > {
> > - dcr_host_t h =3D host;
> > + dcr_host_mmio_t h =3D host;
> >
> > if (h.token =3D=3D NULL)
> > return;
> > @@ -136,7 +199,11 @@ void dcr_unmap(dcr_host_t host, unsigned int
dcr_c)
> > iounmap(h.token);
> > h.token =3D NULL;
> > }
> > -EXPORT_SYMBOL_GPL(dcr_unmap);
> > -#else /* defined(CONFIG_PPC_DCR_NATIVE) */
> > +EXPORT_SYMBOL_GPL(dcr_unmap_mmio);
> > +
> > +#endif /* defined(CONFIG_PPC_DCR_MMIO) */
> > +
> > +#ifdef CONFIG_PPC_DCR_NATIVE
> > DEFINE_SPINLOCK(dcr_ind_lock);
> > -#endif /* !defined(CONFIG_PPC_DCR_NATIVE) */
> > +#endif /* defined(CONFIG_PPC_DCR_NATIVE) */
> > +
> > 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 @@
> > +/*
> > + * (c) Copyright 2006 Benjamin Herrenschmidt, IBM Corp.
> > + * <benh@kernel.crashing.org>
> > + *
> > + * This program is free software; you can redistribute it and/or
modify
> > + * it under the terms of the GNU General Public License as
published by
> > + * the Free Software Foundation; either version 2 of the License,
or
> > + * (at your option) any later version.
> > + *
> > + * This program is distributed in the hope that it will be
useful,
> > + * but WITHOUT ANY WARRANTY; without even the implied warranty
of
> > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See
> > + * the GNU General Public License for more details.
> > + *
> > + * You should have received a copy of the GNU General Public
License
> > + * along with this program; if not, write to the Free Software
> > + * Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA
02111-1307 USA
> > + */
> > +
> > +#ifndef _ASM_POWERPC_DCR_GENERIC_H
> > +#define _ASM_POWERPC_DCR_GENERIC_H
> > +#ifdef __KERNEL__
> > +#ifndef __ASSEMBLY__
> > +
> > +enum host_type_t {MMIO, NATIVE, INVALID};
> > +
> > +typedef struct {
> > + enum host_type_t type;
> > + union {
> > + dcr_host_mmio_t mmio;
> > + dcr_host_native_t native;
> > + } host;
> > +} dcr_host_t;
> > +
> > +extern bool dcr_map_ok_generic(dcr_host_t host);
> > +
> > +extern dcr_host_t dcr_map_generic(struct device_node *dev, unsigned
int dcr_n,
> > + unsigned int dcr_c);
> > +extern void dcr_unmap_generic(dcr_host_t host, unsigned int dcr_c);
> > +
> > +extern u32 dcr_read_generic(dcr_host_t host, unsigned int dcr_n);
> > +
> > +extern void dcr_write_generic(dcr_host_t host, unsigned int dcr_n,
u32 value);
> > +
> > +#endif /* __ASSEMBLY__ */
> > +#endif /* __KERNEL__ */
> > +#endif /* _ASM_POWERPC_DCR_GENERIC_H */
> > +
> > +
> > diff --git a/include/asm-powerpc/dcr-mmio.h
b/include/asm-powerpc/dcr-mmio.h
> > index 08532ff..acd491d 100644
> > --- a/include/asm-powerpc/dcr-mmio.h
> > +++ b/include/asm-powerpc/dcr-mmio.h
> > @@ -27,20 +27,26 @@ typedef struct {
> > void __iomem *token;
> > unsigned int stride;
> > unsigned int base;
> > -} dcr_host_t;
> > +} dcr_host_mmio_t;
> >
> > -#define DCR_MAP_OK(host) ((host).token !=3D NULL)
> > +static inline bool dcr_map_ok_mmio(dcr_host_mmio_t host)
> > +{
> > + return host.token !=3D NULL;
> > +}
> >
> > -extern dcr_host_t dcr_map(struct device_node *dev, unsigned int
dcr_n,
> > - unsigned int dcr_c);
> > -extern void dcr_unmap(dcr_host_t host, unsigned int dcr_c);
> > +extern dcr_host_mmio_t dcr_map_mmio(struct device_node *dev,
> > + unsigned int dcr_n,
> > + unsigned int dcr_c);
> > +extern void dcr_unmap_mmio(dcr_host_mmio_t host, unsigned int
dcr_c);
> >
> > -static inline u32 dcr_read(dcr_host_t host, unsigned int dcr_n)
> > +static inline u32 dcr_read_mmio(dcr_host_mmio_t host, unsigned int
dcr_n)
> > {
> > return in_be32(host.token + ((host.base + dcr_n) *
host.stride));
> > }
> >
> > -static inline void dcr_write(dcr_host_t host, unsigned int dcr_n,
u32 value)
> > +static inline void dcr_write_mmio(dcr_host_mmio_t host,
> > + unsigned int dcr_n,
> > + u32 value)
> > {
> > out_be32(host.token + ((host.base + dcr_n) * host.stride),
value);
> > }
> > diff --git a/include/asm-powerpc/dcr-native.h
b/include/asm-powerpc/dcr-native.h
> > index be6c879..67832e5 100644
> > --- a/include/asm-powerpc/dcr-native.h
> > +++ b/include/asm-powerpc/dcr-native.h
> > @@ -26,14 +26,18 @@
> >
> > typedef struct {
> > unsigned int base;
> > -} dcr_host_t;
> > +} dcr_host_native_t;
> >
> > -#define DCR_MAP_OK(host) (1)
> > +static inline bool dcr_map_ok_native(dcr_host_native_t host)
> > +{
> > + return 1;
> > +}
> >
> > -#define dcr_map(dev, dcr_n, dcr_c) ((dcr_host_t){ .base =3D (dcr_n)
})
> > -#define dcr_unmap(host, dcr_c) do {} while (0)
> > -#define dcr_read(host, dcr_n) mfdcr(dcr_n + host.base)
> > -#define dcr_write(host, dcr_n, value) mtdcr(dcr_n + host.base,
value)
> > +#define dcr_map_native(dev, dcr_n, dcr_c) \
> > + ((dcr_host_native_t){ .base =3D (dcr_n) })
> > +#define dcr_unmap_native(host, dcr_c) do {} while (0)
> > +#define dcr_read_native(host, dcr_n) mfdcr(dcr_n +
host.base)
> > +#define dcr_write_native(host, dcr_n, value) mtdcr(dcr_n +
host.base, value)
> >
> > /* Device Control Registers */
> > void __mtdcr(int reg, unsigned int val);
> > 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) */
> > +
> > /*
> > * On CONFIG_PPC_MERGE, we have additional helpers to read the DCR
> > * base from the device-tree
> > @@ -41,5 +74,6 @@ extern unsigned int dcr_resource_len(struct
device_node *np,
> > #endif /* CONFIG_PPC_MERGE */
> >
> > #endif /* CONFIG_PPC_DCR */
> > +#endif /* __ASSEMBLY__ */
> > #endif /* __KERNEL__ */
> > #endif /* _ASM_POWERPC_DCR_H */
>=20
^ permalink raw reply [flat|nested] 45+ messages in thread* RE: [PATCH 1/5] [v2][POWERPC] refactor dcr code
2008-03-30 22:02 ` Benjamin Herrenschmidt
2008-04-01 20:16 ` Stephen Neuendorffer
@ 2008-04-04 22:02 ` Stephen Neuendorffer
2008-04-04 22:10 ` Benjamin Herrenschmidt
2008-04-18 21:49 ` Stephen Neuendorffer
` (2 subsequent siblings)
4 siblings, 1 reply; 45+ messages in thread
From: Stephen Neuendorffer @ 2008-04-04 22:02 UTC (permalink / raw)
To: benh; +Cc: linuxppc-dev
Ben,
Any thoughts about device tree bindings for indirect DCR?
Steve
> -----Original Message-----
> From: Benjamin Herrenschmidt [mailto:benh@kernel.crashing.org]
> Sent: Sunday, March 30, 2008 3:03 PM
> To: Stephen Neuendorffer
> Cc: linuxppc-dev@ozlabs.org
> Subject: Re: [PATCH 1/5] [v2][POWERPC] refactor dcr code
>=20
>=20
> On Fri, 2008-03-28 at 09:23 -0700, Stephen Neuendorffer 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 slave device 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 interoducing overhead in existing usage.
> >
> > Signed-off-by: Stephen Neuendorffer
<stephen.neuendorffer@xilinx.com>
>=20
> Looks good. Haven't had a chance to test it yet, but tentatively
>=20
> Acked-by: Benjamin Herrenschmidt <benh@kernel.crashing.org>
>=20
> Initially, I thought about using function pointers but the if/else
will
> probably end up being more efficient. The only thing maybe to ponder
> is whether we could avoid the dcr-generic.h file completely, and have
> the generic wrappers be inline, though considering how slow DCRs are,
> it may not be that useful in practice and less clean...
>=20
> Cheers,
> Ben.
>=20
>=20
> > ---
> > 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 =3D=3D INVALID)
> > + return 0;
> > + else if (host.type =3D=3D 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 =3D of_get_property(dev, "dcr-access-method",
NULL);
> > +
> > + if (!strcmp(prop, "native")) {
> > + host.type =3D NATIVE;
> > + host.host.native =3D dcr_map_native(dev, dcr_n, dcr_c);
> > + } else if (!strcmp(prop, "mmio")) {
> > + host.type =3D MMIO;
> > + host.host.mmio =3D dcr_map_mmio(dev, dcr_n, dcr_c);
> > + } else
> > + host.type =3D INVALID;
> > +
> > + return host;
> > +}
> > +EXPORT_SYMBOL_GPL(dcr_map_generic);
> > +
> > +void dcr_unmap_generic(dcr_host_t host, unsigned int dcr_c)
> > +{
> > + if (host.type =3D=3D 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 =3D=3D 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 =3D=3D 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 =3D { .token =3D NULL, .stride =3D 0, .base =3D =
dcr_n };
> > + dcr_host_mmio_t ret =3D { .token =3D NULL, .stride =3D 0, .base =
=3D
dcr_n };
> > u64 addr;
> >
> > pr_debug("dcr_map(%s, 0x%x, 0x%x)\n",
> > dev->full_name, dcr_n, dcr_c);
> >
> > addr =3D 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 =3D=3D 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 -=3D dcr_n * ret.stride;
> > return ret;
> > }
> > -EXPORT_SYMBOL_GPL(dcr_map);
> > +EXPORT_SYMBOL_GPL(dcr_map_mmio);
> >
> > -void dcr_unmap(dcr_host_t host, unsigned int dcr_c)
> > +void dcr_unmap_mmio(dcr_host_mmio_t host, unsigned int dcr_c)
> > {
> > - dcr_host_t h =3D host;
> > + dcr_host_mmio_t h =3D host;
> >
> > if (h.token =3D=3D NULL)
> > return;
> > @@ -136,7 +199,11 @@ void dcr_unmap(dcr_host_t host, unsigned int
dcr_c)
> > iounmap(h.token);
> > h.token =3D NULL;
> > }
> > -EXPORT_SYMBOL_GPL(dcr_unmap);
> > -#else /* defined(CONFIG_PPC_DCR_NATIVE) */
> > +EXPORT_SYMBOL_GPL(dcr_unmap_mmio);
> > +
> > +#endif /* defined(CONFIG_PPC_DCR_MMIO) */
> > +
> > +#ifdef CONFIG_PPC_DCR_NATIVE
> > DEFINE_SPINLOCK(dcr_ind_lock);
> > -#endif /* !defined(CONFIG_PPC_DCR_NATIVE) */
> > +#endif /* defined(CONFIG_PPC_DCR_NATIVE) */
> > +
> > 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 @@
> > +/*
> > + * (c) Copyright 2006 Benjamin Herrenschmidt, IBM Corp.
> > + * <benh@kernel.crashing.org>
> > + *
> > + * This program is free software; you can redistribute it and/or
modify
> > + * it under the terms of the GNU General Public License as
published by
> > + * the Free Software Foundation; either version 2 of the License,
or
> > + * (at your option) any later version.
> > + *
> > + * This program is distributed in the hope that it will be
useful,
> > + * but WITHOUT ANY WARRANTY; without even the implied warranty
of
> > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See
> > + * the GNU General Public License for more details.
> > + *
> > + * You should have received a copy of the GNU General Public
License
> > + * along with this program; if not, write to the Free Software
> > + * Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA
02111-1307 USA
> > + */
> > +
> > +#ifndef _ASM_POWERPC_DCR_GENERIC_H
> > +#define _ASM_POWERPC_DCR_GENERIC_H
> > +#ifdef __KERNEL__
> > +#ifndef __ASSEMBLY__
> > +
> > +enum host_type_t {MMIO, NATIVE, INVALID};
> > +
> > +typedef struct {
> > + enum host_type_t type;
> > + union {
> > + dcr_host_mmio_t mmio;
> > + dcr_host_native_t native;
> > + } host;
> > +} dcr_host_t;
> > +
> > +extern bool dcr_map_ok_generic(dcr_host_t host);
> > +
> > +extern dcr_host_t dcr_map_generic(struct device_node *dev, unsigned
int dcr_n,
> > + unsigned int dcr_c);
> > +extern void dcr_unmap_generic(dcr_host_t host, unsigned int dcr_c);
> > +
> > +extern u32 dcr_read_generic(dcr_host_t host, unsigned int dcr_n);
> > +
> > +extern void dcr_write_generic(dcr_host_t host, unsigned int dcr_n,
u32 value);
> > +
> > +#endif /* __ASSEMBLY__ */
> > +#endif /* __KERNEL__ */
> > +#endif /* _ASM_POWERPC_DCR_GENERIC_H */
> > +
> > +
> > diff --git a/include/asm-powerpc/dcr-mmio.h
b/include/asm-powerpc/dcr-mmio.h
> > index 08532ff..acd491d 100644
> > --- a/include/asm-powerpc/dcr-mmio.h
> > +++ b/include/asm-powerpc/dcr-mmio.h
> > @@ -27,20 +27,26 @@ typedef struct {
> > void __iomem *token;
> > unsigned int stride;
> > unsigned int base;
> > -} dcr_host_t;
> > +} dcr_host_mmio_t;
> >
> > -#define DCR_MAP_OK(host) ((host).token !=3D NULL)
> > +static inline bool dcr_map_ok_mmio(dcr_host_mmio_t host)
> > +{
> > + return host.token !=3D NULL;
> > +}
> >
> > -extern dcr_host_t dcr_map(struct device_node *dev, unsigned int
dcr_n,
> > - unsigned int dcr_c);
> > -extern void dcr_unmap(dcr_host_t host, unsigned int dcr_c);
> > +extern dcr_host_mmio_t dcr_map_mmio(struct device_node *dev,
> > + unsigned int dcr_n,
> > + unsigned int dcr_c);
> > +extern void dcr_unmap_mmio(dcr_host_mmio_t host, unsigned int
dcr_c);
> >
> > -static inline u32 dcr_read(dcr_host_t host, unsigned int dcr_n)
> > +static inline u32 dcr_read_mmio(dcr_host_mmio_t host, unsigned int
dcr_n)
> > {
> > return in_be32(host.token + ((host.base + dcr_n) *
host.stride));
> > }
> >
> > -static inline void dcr_write(dcr_host_t host, unsigned int dcr_n,
u32 value)
> > +static inline void dcr_write_mmio(dcr_host_mmio_t host,
> > + unsigned int dcr_n,
> > + u32 value)
> > {
> > out_be32(host.token + ((host.base + dcr_n) * host.stride),
value);
> > }
> > diff --git a/include/asm-powerpc/dcr-native.h
b/include/asm-powerpc/dcr-native.h
> > index be6c879..67832e5 100644
> > --- a/include/asm-powerpc/dcr-native.h
> > +++ b/include/asm-powerpc/dcr-native.h
> > @@ -26,14 +26,18 @@
> >
> > typedef struct {
> > unsigned int base;
> > -} dcr_host_t;
> > +} dcr_host_native_t;
> >
> > -#define DCR_MAP_OK(host) (1)
> > +static inline bool dcr_map_ok_native(dcr_host_native_t host)
> > +{
> > + return 1;
> > +}
> >
> > -#define dcr_map(dev, dcr_n, dcr_c) ((dcr_host_t){ .base =3D (dcr_n)
})
> > -#define dcr_unmap(host, dcr_c) do {} while (0)
> > -#define dcr_read(host, dcr_n) mfdcr(dcr_n + host.base)
> > -#define dcr_write(host, dcr_n, value) mtdcr(dcr_n + host.base,
value)
> > +#define dcr_map_native(dev, dcr_n, dcr_c) \
> > + ((dcr_host_native_t){ .base =3D (dcr_n) })
> > +#define dcr_unmap_native(host, dcr_c) do {} while (0)
> > +#define dcr_read_native(host, dcr_n) mfdcr(dcr_n +
host.base)
> > +#define dcr_write_native(host, dcr_n, value) mtdcr(dcr_n +
host.base, value)
> >
> > /* Device Control Registers */
> > void __mtdcr(int reg, unsigned int val);
> > 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) */
> > +
> > /*
> > * On CONFIG_PPC_MERGE, we have additional helpers to read the DCR
> > * base from the device-tree
> > @@ -41,5 +74,6 @@ extern unsigned int dcr_resource_len(struct
device_node *np,
> > #endif /* CONFIG_PPC_MERGE */
> >
> > #endif /* CONFIG_PPC_DCR */
> > +#endif /* __ASSEMBLY__ */
> > #endif /* __KERNEL__ */
> > #endif /* _ASM_POWERPC_DCR_H */
>=20
^ permalink raw reply [flat|nested] 45+ messages in thread* RE: [PATCH 1/5] [v2][POWERPC] refactor dcr code
2008-03-30 22:02 ` Benjamin Herrenschmidt
2008-04-01 20:16 ` Stephen Neuendorffer
2008-04-04 22:02 ` Stephen Neuendorffer
@ 2008-04-18 21:49 ` Stephen Neuendorffer
2008-04-18 21:55 ` [PATCH 1/2] [v3][POWERPC] " Stephen Neuendorffer
[not found] ` <1208555704-8978-1-git-send-email-stephen.neuendorffer@xilinx.com>
4 siblings, 0 replies; 45+ messages in thread
From: Stephen Neuendorffer @ 2008-04-18 21:49 UTC (permalink / raw)
To: benh; +Cc: linuxppc-dev
Ben,
I'd like to get the first two patches tested and queued up for mainline
(although it's probably too late for this merge window, I suppose). I'm
going to punt on the ppc patches and put off fixing the tft driver until
ARCH=3Dppc goes away.
I've fixed a typo in patch description: The dcr-access-method attribute
is on the dcr-controller, not the dcr slave.
Steve
> -----Original Message-----
> From: Benjamin Herrenschmidt [mailto:benh@kernel.crashing.org]
> Sent: Sunday, March 30, 2008 3:03 PM
> To: Stephen Neuendorffer
> Cc: linuxppc-dev@ozlabs.org
> Subject: Re: [PATCH 1/5] [v2][POWERPC] refactor dcr code
>=20
>=20
> On Fri, 2008-03-28 at 09:23 -0700, Stephen Neuendorffer 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 slave device 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 interoducing overhead in existing usage.
> >
> > Signed-off-by: Stephen Neuendorffer
<stephen.neuendorffer@xilinx.com>
>=20
> Looks good. Haven't had a chance to test it yet, but tentatively
>=20
> Acked-by: Benjamin Herrenschmidt <benh@kernel.crashing.org>
>=20
> Initially, I thought about using function pointers but the if/else
will
> probably end up being more efficient. The only thing maybe to ponder
> is whether we could avoid the dcr-generic.h file completely, and have
> the generic wrappers be inline, though considering how slow DCRs are,
> it may not be that useful in practice and less clean...
>=20
> Cheers,
> Ben.
>=20
>=20
> > ---
> > 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 =3D=3D INVALID)
> > + return 0;
> > + else if (host.type =3D=3D 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 =3D of_get_property(dev, "dcr-access-method",
NULL);
> > +
> > + if (!strcmp(prop, "native")) {
> > + host.type =3D NATIVE;
> > + host.host.native =3D dcr_map_native(dev, dcr_n, dcr_c);
> > + } else if (!strcmp(prop, "mmio")) {
> > + host.type =3D MMIO;
> > + host.host.mmio =3D dcr_map_mmio(dev, dcr_n, dcr_c);
> > + } else
> > + host.type =3D INVALID;
> > +
> > + return host;
> > +}
> > +EXPORT_SYMBOL_GPL(dcr_map_generic);
> > +
> > +void dcr_unmap_generic(dcr_host_t host, unsigned int dcr_c)
> > +{
> > + if (host.type =3D=3D 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 =3D=3D 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 =3D=3D 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 =3D { .token =3D NULL, .stride =3D 0, .base =3D =
dcr_n };
> > + dcr_host_mmio_t ret =3D { .token =3D NULL, .stride =3D 0, .base =
=3D
dcr_n };
> > u64 addr;
> >
> > pr_debug("dcr_map(%s, 0x%x, 0x%x)\n",
> > dev->full_name, dcr_n, dcr_c);
> >
> > addr =3D 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 =3D=3D 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 -=3D dcr_n * ret.stride;
> > return ret;
> > }
> > -EXPORT_SYMBOL_GPL(dcr_map);
> > +EXPORT_SYMBOL_GPL(dcr_map_mmio);
> >
> > -void dcr_unmap(dcr_host_t host, unsigned int dcr_c)
> > +void dcr_unmap_mmio(dcr_host_mmio_t host, unsigned int dcr_c)
> > {
> > - dcr_host_t h =3D host;
> > + dcr_host_mmio_t h =3D host;
> >
> > if (h.token =3D=3D NULL)
> > return;
> > @@ -136,7 +199,11 @@ void dcr_unmap(dcr_host_t host, unsigned int
dcr_c)
> > iounmap(h.token);
> > h.token =3D NULL;
> > }
> > -EXPORT_SYMBOL_GPL(dcr_unmap);
> > -#else /* defined(CONFIG_PPC_DCR_NATIVE) */
> > +EXPORT_SYMBOL_GPL(dcr_unmap_mmio);
> > +
> > +#endif /* defined(CONFIG_PPC_DCR_MMIO) */
> > +
> > +#ifdef CONFIG_PPC_DCR_NATIVE
> > DEFINE_SPINLOCK(dcr_ind_lock);
> > -#endif /* !defined(CONFIG_PPC_DCR_NATIVE) */
> > +#endif /* defined(CONFIG_PPC_DCR_NATIVE) */
> > +
> > 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 @@
> > +/*
> > + * (c) Copyright 2006 Benjamin Herrenschmidt, IBM Corp.
> > + * <benh@kernel.crashing.org>
> > + *
> > + * This program is free software; you can redistribute it and/or
modify
> > + * it under the terms of the GNU General Public License as
published by
> > + * the Free Software Foundation; either version 2 of the License,
or
> > + * (at your option) any later version.
> > + *
> > + * This program is distributed in the hope that it will be
useful,
> > + * but WITHOUT ANY WARRANTY; without even the implied warranty
of
> > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See
> > + * the GNU General Public License for more details.
> > + *
> > + * You should have received a copy of the GNU General Public
License
> > + * along with this program; if not, write to the Free Software
> > + * Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA
02111-1307 USA
> > + */
> > +
> > +#ifndef _ASM_POWERPC_DCR_GENERIC_H
> > +#define _ASM_POWERPC_DCR_GENERIC_H
> > +#ifdef __KERNEL__
> > +#ifndef __ASSEMBLY__
> > +
> > +enum host_type_t {MMIO, NATIVE, INVALID};
> > +
> > +typedef struct {
> > + enum host_type_t type;
> > + union {
> > + dcr_host_mmio_t mmio;
> > + dcr_host_native_t native;
> > + } host;
> > +} dcr_host_t;
> > +
> > +extern bool dcr_map_ok_generic(dcr_host_t host);
> > +
> > +extern dcr_host_t dcr_map_generic(struct device_node *dev, unsigned
int dcr_n,
> > + unsigned int dcr_c);
> > +extern void dcr_unmap_generic(dcr_host_t host, unsigned int dcr_c);
> > +
> > +extern u32 dcr_read_generic(dcr_host_t host, unsigned int dcr_n);
> > +
> > +extern void dcr_write_generic(dcr_host_t host, unsigned int dcr_n,
u32 value);
> > +
> > +#endif /* __ASSEMBLY__ */
> > +#endif /* __KERNEL__ */
> > +#endif /* _ASM_POWERPC_DCR_GENERIC_H */
> > +
> > +
> > diff --git a/include/asm-powerpc/dcr-mmio.h
b/include/asm-powerpc/dcr-mmio.h
> > index 08532ff..acd491d 100644
> > --- a/include/asm-powerpc/dcr-mmio.h
> > +++ b/include/asm-powerpc/dcr-mmio.h
> > @@ -27,20 +27,26 @@ typedef struct {
> > void __iomem *token;
> > unsigned int stride;
> > unsigned int base;
> > -} dcr_host_t;
> > +} dcr_host_mmio_t;
> >
> > -#define DCR_MAP_OK(host) ((host).token !=3D NULL)
> > +static inline bool dcr_map_ok_mmio(dcr_host_mmio_t host)
> > +{
> > + return host.token !=3D NULL;
> > +}
> >
> > -extern dcr_host_t dcr_map(struct device_node *dev, unsigned int
dcr_n,
> > - unsigned int dcr_c);
> > -extern void dcr_unmap(dcr_host_t host, unsigned int dcr_c);
> > +extern dcr_host_mmio_t dcr_map_mmio(struct device_node *dev,
> > + unsigned int dcr_n,
> > + unsigned int dcr_c);
> > +extern void dcr_unmap_mmio(dcr_host_mmio_t host, unsigned int
dcr_c);
> >
> > -static inline u32 dcr_read(dcr_host_t host, unsigned int dcr_n)
> > +static inline u32 dcr_read_mmio(dcr_host_mmio_t host, unsigned int
dcr_n)
> > {
> > return in_be32(host.token + ((host.base + dcr_n) *
host.stride));
> > }
> >
> > -static inline void dcr_write(dcr_host_t host, unsigned int dcr_n,
u32 value)
> > +static inline void dcr_write_mmio(dcr_host_mmio_t host,
> > + unsigned int dcr_n,
> > + u32 value)
> > {
> > out_be32(host.token + ((host.base + dcr_n) * host.stride),
value);
> > }
> > diff --git a/include/asm-powerpc/dcr-native.h
b/include/asm-powerpc/dcr-native.h
> > index be6c879..67832e5 100644
> > --- a/include/asm-powerpc/dcr-native.h
> > +++ b/include/asm-powerpc/dcr-native.h
> > @@ -26,14 +26,18 @@
> >
> > typedef struct {
> > unsigned int base;
> > -} dcr_host_t;
> > +} dcr_host_native_t;
> >
> > -#define DCR_MAP_OK(host) (1)
> > +static inline bool dcr_map_ok_native(dcr_host_native_t host)
> > +{
> > + return 1;
> > +}
> >
> > -#define dcr_map(dev, dcr_n, dcr_c) ((dcr_host_t){ .base =3D (dcr_n)
})
> > -#define dcr_unmap(host, dcr_c) do {} while (0)
> > -#define dcr_read(host, dcr_n) mfdcr(dcr_n + host.base)
> > -#define dcr_write(host, dcr_n, value) mtdcr(dcr_n + host.base,
value)
> > +#define dcr_map_native(dev, dcr_n, dcr_c) \
> > + ((dcr_host_native_t){ .base =3D (dcr_n) })
> > +#define dcr_unmap_native(host, dcr_c) do {} while (0)
> > +#define dcr_read_native(host, dcr_n) mfdcr(dcr_n +
host.base)
> > +#define dcr_write_native(host, dcr_n, value) mtdcr(dcr_n +
host.base, value)
> >
> > /* Device Control Registers */
> > void __mtdcr(int reg, unsigned int val);
> > 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) */
> > +
> > /*
> > * On CONFIG_PPC_MERGE, we have additional helpers to read the DCR
> > * base from the device-tree
> > @@ -41,5 +74,6 @@ extern unsigned int dcr_resource_len(struct
device_node *np,
> > #endif /* CONFIG_PPC_MERGE */
> >
> > #endif /* CONFIG_PPC_DCR */
> > +#endif /* __ASSEMBLY__ */
> > #endif /* __KERNEL__ */
> > #endif /* _ASM_POWERPC_DCR_H */
>=20
^ permalink raw reply [flat|nested] 45+ messages in thread* [PATCH 1/2] [v3][POWERPC] refactor dcr code
2008-03-30 22:02 ` Benjamin Herrenschmidt
` (2 preceding siblings ...)
2008-04-18 21:49 ` Stephen Neuendorffer
@ 2008-04-18 21:55 ` Stephen Neuendorffer
2008-04-19 2:30 ` Grant Likely
2008-04-19 14:35 ` Josh Boyer
[not found] ` <1208555704-8978-1-git-send-email-stephen.neuendorffer@xilinx.com>
4 siblings, 2 replies; 45+ messages in thread
From: Stephen Neuendorffer @ 2008-04-18 21:55 UTC (permalink / raw)
To: benh, linuxppc-dev
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>
---
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;
return ret;
}
-EXPORT_SYMBOL_GPL(dcr_map);
+EXPORT_SYMBOL_GPL(dcr_map_mmio);
-void dcr_unmap(dcr_host_t host, unsigned int dcr_c)
+void dcr_unmap_mmio(dcr_host_mmio_t host, unsigned int dcr_c)
{
- dcr_host_t h = host;
+ dcr_host_mmio_t h = host;
if (h.token == NULL)
return;
@@ -136,7 +199,11 @@ void dcr_unmap(dcr_host_t host, unsigned int dcr_c)
iounmap(h.token);
h.token = NULL;
}
-EXPORT_SYMBOL_GPL(dcr_unmap);
-#else /* defined(CONFIG_PPC_DCR_NATIVE) */
+EXPORT_SYMBOL_GPL(dcr_unmap_mmio);
+
+#endif /* defined(CONFIG_PPC_DCR_MMIO) */
+
+#ifdef CONFIG_PPC_DCR_NATIVE
DEFINE_SPINLOCK(dcr_ind_lock);
-#endif /* !defined(CONFIG_PPC_DCR_NATIVE) */
+#endif /* defined(CONFIG_PPC_DCR_NATIVE) */
+
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 @@
+/*
+ * (c) Copyright 2006 Benjamin Herrenschmidt, IBM Corp.
+ * <benh@kernel.crashing.org>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See
+ * the GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program; if not, write to the Free Software
+ * Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA 02111-1307 USA
+ */
+
+#ifndef _ASM_POWERPC_DCR_GENERIC_H
+#define _ASM_POWERPC_DCR_GENERIC_H
+#ifdef __KERNEL__
+#ifndef __ASSEMBLY__
+
+enum host_type_t {MMIO, NATIVE, INVALID};
+
+typedef struct {
+ enum host_type_t type;
+ union {
+ dcr_host_mmio_t mmio;
+ dcr_host_native_t native;
+ } host;
+} dcr_host_t;
+
+extern bool dcr_map_ok_generic(dcr_host_t host);
+
+extern dcr_host_t dcr_map_generic(struct device_node *dev, unsigned int dcr_n,
+ unsigned int dcr_c);
+extern void dcr_unmap_generic(dcr_host_t host, unsigned int dcr_c);
+
+extern u32 dcr_read_generic(dcr_host_t host, unsigned int dcr_n);
+
+extern void dcr_write_generic(dcr_host_t host, unsigned int dcr_n, u32 value);
+
+#endif /* __ASSEMBLY__ */
+#endif /* __KERNEL__ */
+#endif /* _ASM_POWERPC_DCR_GENERIC_H */
+
+
diff --git a/include/asm-powerpc/dcr-mmio.h b/include/asm-powerpc/dcr-mmio.h
index 08532ff..acd491d 100644
--- a/include/asm-powerpc/dcr-mmio.h
+++ b/include/asm-powerpc/dcr-mmio.h
@@ -27,20 +27,26 @@ typedef struct {
void __iomem *token;
unsigned int stride;
unsigned int base;
-} dcr_host_t;
+} dcr_host_mmio_t;
-#define DCR_MAP_OK(host) ((host).token != NULL)
+static inline bool dcr_map_ok_mmio(dcr_host_mmio_t host)
+{
+ return host.token != NULL;
+}
-extern dcr_host_t dcr_map(struct device_node *dev, unsigned int dcr_n,
- unsigned int dcr_c);
-extern void dcr_unmap(dcr_host_t host, unsigned int dcr_c);
+extern dcr_host_mmio_t dcr_map_mmio(struct device_node *dev,
+ unsigned int dcr_n,
+ unsigned int dcr_c);
+extern void dcr_unmap_mmio(dcr_host_mmio_t host, unsigned int dcr_c);
-static inline u32 dcr_read(dcr_host_t host, unsigned int dcr_n)
+static inline u32 dcr_read_mmio(dcr_host_mmio_t host, unsigned int dcr_n)
{
return in_be32(host.token + ((host.base + dcr_n) * host.stride));
}
-static inline void dcr_write(dcr_host_t host, unsigned int dcr_n, u32 value)
+static inline void dcr_write_mmio(dcr_host_mmio_t host,
+ unsigned int dcr_n,
+ u32 value)
{
out_be32(host.token + ((host.base + dcr_n) * host.stride), value);
}
diff --git a/include/asm-powerpc/dcr-native.h b/include/asm-powerpc/dcr-native.h
index f8398ce..72d2b72 100644
--- a/include/asm-powerpc/dcr-native.h
+++ b/include/asm-powerpc/dcr-native.h
@@ -26,14 +26,18 @@
typedef struct {
unsigned int base;
-} dcr_host_t;
+} dcr_host_native_t;
-#define DCR_MAP_OK(host) (1)
+static inline bool dcr_map_ok_native(dcr_host_native_t host)
+{
+ return 1;
+}
-#define dcr_map(dev, dcr_n, dcr_c) ((dcr_host_t){ .base = (dcr_n) })
-#define dcr_unmap(host, dcr_c) do {} while (0)
-#define dcr_read(host, dcr_n) mfdcr(dcr_n + host.base)
-#define dcr_write(host, dcr_n, value) mtdcr(dcr_n + host.base, value)
+#define dcr_map_native(dev, dcr_n, dcr_c) \
+ ((dcr_host_native_t){ .base = (dcr_n) })
+#define dcr_unmap_native(host, dcr_c) do {} while (0)
+#define dcr_read_native(host, dcr_n) mfdcr(dcr_n + host.base)
+#define dcr_write_native(host, dcr_n, value) mtdcr(dcr_n + host.base, value)
/* Device Control Registers */
void __mtdcr(int reg, unsigned int val);
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) */
+
/*
* On CONFIG_PPC_MERGE, we have additional helpers to read the DCR
* base from the device-tree
@@ -41,5 +74,6 @@ extern unsigned int dcr_resource_len(struct device_node *np,
#endif /* CONFIG_PPC_MERGE */
#endif /* CONFIG_PPC_DCR */
+#endif /* __ASSEMBLY__ */
#endif /* __KERNEL__ */
#endif /* _ASM_POWERPC_DCR_H */
--
1.5.3.4-dirty
^ permalink raw reply related [flat|nested] 45+ messages in thread* Re: [PATCH 1/2] [v3][POWERPC] refactor dcr code
2008-04-18 21:55 ` [PATCH 1/2] [v3][POWERPC] " Stephen Neuendorffer
@ 2008-04-19 2:30 ` Grant Likely
2008-04-19 3:04 ` Benjamin Herrenschmidt
2008-04-19 14:35 ` Josh Boyer
1 sibling, 1 reply; 45+ messages in thread
From: Grant Likely @ 2008-04-19 2:30 UTC (permalink / raw)
To: Stephen Neuendorffer; +Cc: linuxppc-dev
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.
^ permalink raw reply [flat|nested] 45+ messages in thread
* Re: [PATCH 1/2] [v3][POWERPC] refactor dcr code
2008-04-19 2:30 ` Grant Likely
@ 2008-04-19 3:04 ` Benjamin Herrenschmidt
0 siblings, 0 replies; 45+ messages in thread
From: Benjamin Herrenschmidt @ 2008-04-19 3:04 UTC (permalink / raw)
To: Grant Likely; +Cc: linuxppc-dev
> 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.
^ permalink raw reply [flat|nested] 45+ messages in thread
* Re: [PATCH 1/2] [v3][POWERPC] refactor dcr code
2008-04-18 21:55 ` [PATCH 1/2] [v3][POWERPC] " Stephen Neuendorffer
2008-04-19 2:30 ` Grant Likely
@ 2008-04-19 14:35 ` Josh Boyer
2008-04-21 3:56 ` Stephen Neuendorffer
1 sibling, 1 reply; 45+ messages in thread
From: Josh Boyer @ 2008-04-19 14:35 UTC (permalink / raw)
To: Stephen Neuendorffer; +Cc: linuxppc-dev
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
^ permalink raw reply [flat|nested] 45+ messages in thread* RE: [PATCH 1/2] [v3][POWERPC] refactor dcr code
2008-04-19 14:35 ` Josh Boyer
@ 2008-04-21 3:56 ` Stephen Neuendorffer
2008-04-21 13:03 ` Josh Boyer
0 siblings, 1 reply; 45+ messages in thread
From: Stephen Neuendorffer @ 2008-04-21 3:56 UTC (permalink / raw)
To: Josh Boyer; +Cc: linuxppc-dev
[-- Attachment #1: Type: text/plain, Size: 1036 bytes --]
> > +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.
I guess looking back on it, I assumed that MAP_OK would return 0, meaning that behavior was undefined,
but I agree it's probably safer to have some error reporting there... There starts to become a speed tradeoff
at some point, which would make function pointers more attractive. If the ioremap does fail, or the
dcr-access-method can't be determined, then dcr_unmap_mmio would probably SEGV anyway, although that's
not something I'd really want to rely on. I'll put an error case in there.
> > +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.
Also seems reasonable,
Steve
[-- Attachment #2: Type: text/html, Size: 1748 bytes --]
^ permalink raw reply [flat|nested] 45+ messages in thread* Re: [PATCH 1/2] [v3][POWERPC] refactor dcr code
2008-04-21 3:56 ` Stephen Neuendorffer
@ 2008-04-21 13:03 ` Josh Boyer
2008-05-05 17:56 ` [PATCH 0/4] [v4][POWERPC] " Stephen Neuendorffer
[not found] ` <1210010201-28436-1-git-send-email-stephen.neuendorffer@xilinx.com>
0 siblings, 2 replies; 45+ messages in thread
From: Josh Boyer @ 2008-04-21 13:03 UTC (permalink / raw)
To: Stephen Neuendorffer; +Cc: linuxppc-dev
On Sun, 20 Apr 2008 20:56:20 -0700
"Stephen Neuendorffer" <stephen.neuendorffer@xilinx.com> wrote:
>
>
> > > +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.
>
> I guess looking back on it, I assumed that MAP_OK would return 0, meaning that behavior was undefined,
> but I agree it's probably safer to have some error reporting there... There starts to become a speed tradeoff
> at some point, which would make function pointers more attractive. If the ioremap does fail, or the
> dcr-access-method can't be determined, then dcr_unmap_mmio would probably SEGV anyway, although that's
> not something I'd really want to rely on. I'll put an error case in there.
Well, MAP_OK would return 0, and the caller of the original dcr_map
should probably return an error or something. But there's nothing to
enforce that. Which is true for the current code today as well, so
it's not something you've introduced.
I just thought that if you were going to go to the trouble of defining
an invalid host type, that you'd check for it in the accessor
functions. There is the concern of the speed tradeoffs. I suppose you
could just omit INVALID altogether and have it match the existing code
in behavior if it was too big of a deal.
Ben, any thoughts?
josh
^ permalink raw reply [flat|nested] 45+ messages in thread* [PATCH 0/4] [v4][POWERPC] refactor dcr code
2008-04-21 13:03 ` Josh Boyer
@ 2008-05-05 17:56 ` Stephen Neuendorffer
[not found] ` <1210010201-28436-1-git-send-email-stephen.neuendorffer@xilinx.com>
1 sibling, 0 replies; 45+ messages in thread
From: Stephen Neuendorffer @ 2008-05-05 17:56 UTC (permalink / raw)
To: jwboyer, benh, grant.likely, linuxppc-dev
I've modified these patches to include Josh's comments, and also
modified the tft patches to not assume that patches to ARCH=ppc are
made. This breaks platform_device support and hence ARCH=ppc support.
I'd appreciate it if these patches could get queued up for 2.6.27 (and
hence get built a bit before then), when ARCH=ppc is going away
anyway.
Steve
^ permalink raw reply [flat|nested] 45+ messages in thread[parent not found: <1210010201-28436-1-git-send-email-stephen.neuendorffer@xilinx.com>]
* [PATCH 1/4] [v4][POWERPC] refactor dcr code
[not found] ` <1210010201-28436-1-git-send-email-stephen.neuendorffer@xilinx.com>
@ 2008-05-05 17:56 ` Stephen Neuendorffer
2008-05-06 0:12 ` Benjamin Herrenschmidt
2008-05-06 3:40 ` Stephen Rothwell
[not found] ` <1210010201-28436-2-git-send-email-stephen.neuendorffer@xilinx.com>
1 sibling, 2 replies; 45+ messages in thread
From: Stephen Neuendorffer @ 2008-05-05 17:56 UTC (permalink / raw)
To: jwboyer, benh, grant.likely, linuxppc-dev
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>
--
[v4]
Converted flags to be DCR_HOST_*
Added WARN_ON in case someone forgets to call MAP_OK()
Fixed dealing with the dcr-access-method correctly, which was still
off in the wrong patch.
---
arch/powerpc/sysdev/dcr.c | 146 +++++++++++++++++++++++++++++--------
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 | 39 ++++++++++-
5 files changed, 225 insertions(+), 45 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..6984424 100644
--- a/arch/powerpc/sysdev/dcr.c
+++ b/arch/powerpc/sysdev/dcr.c
@@ -23,6 +23,104 @@
#include <asm/prom.h>
#include <asm/dcr.h>
+static struct device_node *find_dcr_parent(struct device_node *node)
+{
+ struct device_node *par, *tmp;
+ const u32 *p;
+
+ for (par = of_node_get(node); par;) {
+ if (of_get_property(par, "dcr-controller", NULL))
+ break;
+ p = of_get_property(par, "dcr-parent", NULL);
+ tmp = par;
+ if (p == NULL)
+ par = of_get_parent(par);
+ else
+ par = of_find_node_by_phandle(*p);
+ of_node_put(tmp);
+ }
+ return par;
+}
+
+#if defined(CONFIG_PPC_DCR_NATIVE) && defined(CONFIG_PPC_DCR_MMIO)
+
+bool dcr_map_ok_generic(dcr_host_t host)
+{
+ if (host.type == DCR_HOST_NATIVE)
+ return dcr_map_ok_native(host.host.native);
+ else if (host.type == DCR_HOST_MMIO)
+ return dcr_map_ok_mmio(host.host.mmio);
+ else
+ return 0;
+}
+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;
+ struct device_node *dp;
+ const char *prop;
+
+ host.type = DCR_HOST_INVALID;
+
+ dp = find_dcr_parent(dev);
+ if (dp == NULL)
+ return host;
+
+ prop = of_get_property(dp, "dcr-access-method", NULL);
+
+ pr_debug("dcr_map_generic(dcr-access-method = %s)\n", prop);
+
+ if (!strcmp(prop, "native")) {
+ host.type = DCR_HOST_NATIVE;
+ host.host.native = dcr_map_native(dev, dcr_n, dcr_c);
+ } else if (!strcmp(prop, "mmio")) {
+ host.type = DCR_HOST_MMIO;
+ host.host.mmio = dcr_map_mmio(dev, dcr_n, dcr_c);
+ }
+
+ return host;
+}
+EXPORT_SYMBOL_GPL(dcr_map_generic);
+
+void dcr_unmap_generic(dcr_host_t host, unsigned int dcr_c)
+{
+ if (host.type == DCR_HOST_NATIVE)
+ dcr_unmap_native(host.host.native, dcr_c);
+ else if (host.type == DCR_HOST_MMIO)
+ dcr_unmap_mmio(host.host.mmio, dcr_c);
+ else /* host.type == DCR_HOST_INVALID */
+ WARN_ON(true);
+}
+EXPORT_SYMBOL_GPL(dcr_unmap_generic);
+
+u32 dcr_read_generic(dcr_host_t host, unsigned int dcr_n)
+{
+ if (host.type == DCR_HOST_NATIVE)
+ return dcr_read_native(host.host.native, dcr_n);
+ else if (host.type == DCR_HOST_MMIO)
+ return dcr_read_mmio(host.host.mmio, dcr_n);
+ else /* host.type == DCR_HOST_INVALID */
+ WARN_ON(true);
+ return 0;
+}
+EXPORT_SYMBOL_GPL(dcr_read_generic);
+
+void dcr_write_generic(dcr_host_t host, unsigned int dcr_n, u32 value)
+{
+ if (host.type == DCR_HOST_NATIVE)
+ dcr_write_native(host.host.native, dcr_n, value);
+ else if (host.type == DCR_HOST_MMIO)
+ dcr_write_mmio(host.host.mmio, dcr_n, value);
+ else /* host.type == DCR_HOST_INVALID */
+ WARN_ON(true);
+}
+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,26 +145,7 @@ unsigned int dcr_resource_len(struct device_node *np, unsigned int index)
}
EXPORT_SYMBOL_GPL(dcr_resource_len);
-#ifndef CONFIG_PPC_DCR_NATIVE
-
-static struct device_node * find_dcr_parent(struct device_node * node)
-{
- struct device_node *par, *tmp;
- const u32 *p;
-
- for (par = of_node_get(node); par;) {
- if (of_get_property(par, "dcr-controller", NULL))
- break;
- p = of_get_property(par, "dcr-parent", NULL);
- tmp = par;
- if (p == NULL)
- par = of_get_parent(par);
- else
- par = of_find_node_by_phandle(*p);
- of_node_put(tmp);
- }
- return par;
-}
+#ifdef CONFIG_PPC_DCR_MMIO
u64 of_translate_dcr_address(struct device_node *dev,
unsigned int dcr_n,
@@ -101,18 +180,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 +204,11 @@ dcr_host_t dcr_map(struct device_node *dev, unsigned int dcr_n,
ret.token -= dcr_n * ret.stride;
return ret;
}
-EXPORT_SYMBOL_GPL(dcr_map);
+EXPORT_SYMBOL_GPL(dcr_map_mmio);
-void dcr_unmap(dcr_host_t host, unsigned int dcr_c)
+void dcr_unmap_mmio(dcr_host_mmio_t host, unsigned int dcr_c)
{
- dcr_host_t h = host;
+ dcr_host_mmio_t h = host;
if (h.token == NULL)
return;
@@ -136,7 +216,11 @@ void dcr_unmap(dcr_host_t host, unsigned int dcr_c)
iounmap(h.token);
h.token = NULL;
}
-EXPORT_SYMBOL_GPL(dcr_unmap);
-#else /* defined(CONFIG_PPC_DCR_NATIVE) */
+EXPORT_SYMBOL_GPL(dcr_unmap_mmio);
+
+#endif /* defined(CONFIG_PPC_DCR_MMIO) */
+
+#ifdef CONFIG_PPC_DCR_NATIVE
DEFINE_SPINLOCK(dcr_ind_lock);
-#endif /* !defined(CONFIG_PPC_DCR_NATIVE) */
+#endif /* defined(CONFIG_PPC_DCR_NATIVE) */
+
diff --git a/include/asm-powerpc/dcr-generic.h b/include/asm-powerpc/dcr-generic.h
new file mode 100644
index 0000000..35b7159
--- /dev/null
+++ b/include/asm-powerpc/dcr-generic.h
@@ -0,0 +1,49 @@
+/*
+ * (c) Copyright 2006 Benjamin Herrenschmidt, IBM Corp.
+ * <benh@kernel.crashing.org>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See
+ * the GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program; if not, write to the Free Software
+ * Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA 02111-1307 USA
+ */
+
+#ifndef _ASM_POWERPC_DCR_GENERIC_H
+#define _ASM_POWERPC_DCR_GENERIC_H
+#ifdef __KERNEL__
+#ifndef __ASSEMBLY__
+
+enum host_type_t {DCR_HOST_MMIO, DCR_HOST_NATIVE, DCR_HOST_INVALID};
+
+typedef struct {
+ enum host_type_t type;
+ union {
+ dcr_host_mmio_t mmio;
+ dcr_host_native_t native;
+ } host;
+} dcr_host_t;
+
+extern bool dcr_map_ok_generic(dcr_host_t host);
+
+extern dcr_host_t dcr_map_generic(struct device_node *dev, unsigned int dcr_n,
+ unsigned int dcr_c);
+extern void dcr_unmap_generic(dcr_host_t host, unsigned int dcr_c);
+
+extern u32 dcr_read_generic(dcr_host_t host, unsigned int dcr_n);
+
+extern void dcr_write_generic(dcr_host_t host, unsigned int dcr_n, u32 value);
+
+#endif /* __ASSEMBLY__ */
+#endif /* __KERNEL__ */
+#endif /* _ASM_POWERPC_DCR_GENERIC_H */
+
+
diff --git a/include/asm-powerpc/dcr-mmio.h b/include/asm-powerpc/dcr-mmio.h
index 08532ff..acd491d 100644
--- a/include/asm-powerpc/dcr-mmio.h
+++ b/include/asm-powerpc/dcr-mmio.h
@@ -27,20 +27,26 @@ typedef struct {
void __iomem *token;
unsigned int stride;
unsigned int base;
-} dcr_host_t;
+} dcr_host_mmio_t;
-#define DCR_MAP_OK(host) ((host).token != NULL)
+static inline bool dcr_map_ok_mmio(dcr_host_mmio_t host)
+{
+ return host.token != NULL;
+}
-extern dcr_host_t dcr_map(struct device_node *dev, unsigned int dcr_n,
- unsigned int dcr_c);
-extern void dcr_unmap(dcr_host_t host, unsigned int dcr_c);
+extern dcr_host_mmio_t dcr_map_mmio(struct device_node *dev,
+ unsigned int dcr_n,
+ unsigned int dcr_c);
+extern void dcr_unmap_mmio(dcr_host_mmio_t host, unsigned int dcr_c);
-static inline u32 dcr_read(dcr_host_t host, unsigned int dcr_n)
+static inline u32 dcr_read_mmio(dcr_host_mmio_t host, unsigned int dcr_n)
{
return in_be32(host.token + ((host.base + dcr_n) * host.stride));
}
-static inline void dcr_write(dcr_host_t host, unsigned int dcr_n, u32 value)
+static inline void dcr_write_mmio(dcr_host_mmio_t host,
+ unsigned int dcr_n,
+ u32 value)
{
out_be32(host.token + ((host.base + dcr_n) * host.stride), value);
}
diff --git a/include/asm-powerpc/dcr-native.h b/include/asm-powerpc/dcr-native.h
index f8398ce..72d2b72 100644
--- a/include/asm-powerpc/dcr-native.h
+++ b/include/asm-powerpc/dcr-native.h
@@ -26,14 +26,18 @@
typedef struct {
unsigned int base;
-} dcr_host_t;
+} dcr_host_native_t;
-#define DCR_MAP_OK(host) (1)
+static inline bool dcr_map_ok_native(dcr_host_native_t host)
+{
+ return 1;
+}
-#define dcr_map(dev, dcr_n, dcr_c) ((dcr_host_t){ .base = (dcr_n) })
-#define dcr_unmap(host, dcr_c) do {} while (0)
-#define dcr_read(host, dcr_n) mfdcr(dcr_n + host.base)
-#define dcr_write(host, dcr_n, value) mtdcr(dcr_n + host.base, value)
+#define dcr_map_native(dev, dcr_n, dcr_c) \
+ ((dcr_host_native_t){ .base = (dcr_n) })
+#define dcr_unmap_native(host, dcr_c) do {} while (0)
+#define dcr_read_native(host, dcr_n) mfdcr(dcr_n + host.base)
+#define dcr_write_native(host, dcr_n, value) mtdcr(dcr_n + host.base, value)
/* Device Control Registers */
void __mtdcr(int reg, unsigned int val);
diff --git a/include/asm-powerpc/dcr.h b/include/asm-powerpc/dcr.h
index 9338d50..53b2830 100644
--- a/include/asm-powerpc/dcr.h
+++ b/include/asm-powerpc/dcr.h
@@ -20,14 +20,50 @@
#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
+
+/* Indirection layer for providing both NATIVE and MMIO support. */
+
+#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) */
+
/*
* On CONFIG_PPC_MERGE, we have additional helpers to read the DCR
* base from the device-tree
@@ -41,5 +77,6 @@ extern unsigned int dcr_resource_len(struct device_node *np,
#endif /* CONFIG_PPC_MERGE */
#endif /* CONFIG_PPC_DCR */
+#endif /* __ASSEMBLY__ */
#endif /* __KERNEL__ */
#endif /* _ASM_POWERPC_DCR_H */
--
1.5.3.4-dirty
^ permalink raw reply related [flat|nested] 45+ messages in thread* Re: [PATCH 1/4] [v4][POWERPC] refactor dcr code
2008-05-05 17:56 ` [PATCH 1/4] " Stephen Neuendorffer
@ 2008-05-06 0:12 ` Benjamin Herrenschmidt
2008-05-06 3:40 ` Stephen Rothwell
1 sibling, 0 replies; 45+ messages in thread
From: Benjamin Herrenschmidt @ 2008-05-06 0:12 UTC (permalink / raw)
To: Stephen Neuendorffer; +Cc: linuxppc-dev
On Mon, 2008-05-05 at 10:56 -0700, Stephen Neuendorffer 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>
Quick look seems ok, but that was really only a quick look :-)
I'll rely on Josh to review further.
Cheers,
Ben.
^ permalink raw reply [flat|nested] 45+ messages in thread
* Re: [PATCH 1/4] [v4][POWERPC] refactor dcr code
2008-05-05 17:56 ` [PATCH 1/4] " Stephen Neuendorffer
2008-05-06 0:12 ` Benjamin Herrenschmidt
@ 2008-05-06 3:40 ` Stephen Rothwell
2008-05-06 4:02 ` Benjamin Herrenschmidt
2008-05-06 18:29 ` [PATCH 1/4] [v5][POWERPC] " Stephen Neuendorffer
1 sibling, 2 replies; 45+ messages in thread
From: Stephen Rothwell @ 2008-05-06 3:40 UTC (permalink / raw)
To: Stephen Neuendorffer; +Cc: linuxppc-dev
[-- Attachment #1: Type: text/plain, Size: 1619 bytes --]
Hi Stephen,
On Mon, 5 May 2008 10:56:38 -0700 Stephen Neuendorffer <stephen.neuendorffer@xilinx.com> wrote:
>
> +static struct device_node *find_dcr_parent(struct device_node *node)
> +{
> + struct device_node *par, *tmp;
> + const u32 *p;
> +
> + for (par = of_node_get(node); par;) {
> + if (of_get_property(par, "dcr-controller", NULL))
> + break;
> + p = of_get_property(par, "dcr-parent", NULL);
> + tmp = par;
> + if (p == NULL)
> + par = of_get_parent(par);
> + else
> + par = of_find_node_by_phandle(*p);
> + of_node_put(tmp);
> + }
> + return par;
> +}
.
.
> +dcr_host_t dcr_map_generic(struct device_node *dev,
> + unsigned int dcr_n,
> + unsigned int dcr_c)
> +{
> + dcr_host_t host;
> + struct device_node *dp;
> + const char *prop;
> +
> + host.type = DCR_HOST_INVALID;
> +
> + dp = find_dcr_parent(dev);
> + if (dp == NULL)
> + return host;
> +
> + prop = of_get_property(dp, "dcr-access-method", NULL);
> +
> + pr_debug("dcr_map_generic(dcr-access-method = %s)\n", prop);
> +
> + if (!strcmp(prop, "native")) {
> + host.type = DCR_HOST_NATIVE;
> + host.host.native = dcr_map_native(dev, dcr_n, dcr_c);
> + } else if (!strcmp(prop, "mmio")) {
> + host.type = DCR_HOST_MMIO;
> + host.host.mmio = dcr_map_mmio(dev, dcr_n, dcr_c);
> + }
> +
> + return host;
Since find_dcr_parent has done a of_node_get on its return value, you
leak a reference to dp here i.e. you need an of_node_put(dp) before you
return.
--
Cheers,
Stephen Rothwell sfr@canb.auug.org.au
http://www.canb.auug.org.au/~sfr/
[-- Attachment #2: Type: application/pgp-signature, Size: 189 bytes --]
^ permalink raw reply [flat|nested] 45+ messages in thread* Re: [PATCH 1/4] [v4][POWERPC] refactor dcr code
2008-05-06 3:40 ` Stephen Rothwell
@ 2008-05-06 4:02 ` Benjamin Herrenschmidt
2008-05-06 17:34 ` Stephen Neuendorffer
2008-05-06 18:29 ` [PATCH 1/4] [v5][POWERPC] " Stephen Neuendorffer
1 sibling, 1 reply; 45+ messages in thread
From: Benjamin Herrenschmidt @ 2008-05-06 4:02 UTC (permalink / raw)
To: Stephen Rothwell; +Cc: linuxppc-dev
On Tue, 2008-05-06 at 13:40 +1000, Stephen Rothwell wrote:
> Since find_dcr_parent has done a of_node_get on its return value, you
> leak a reference to dp here i.e. you need an of_node_put(dp) before
> you return.
He inherited that bug from other dcr.c functions I wrote, my fault.
Stephen (N. not R.), would you mind fixing them while at it or do you
want me to cook up a patch ?
Cheers,
Ben.
^ permalink raw reply [flat|nested] 45+ messages in thread
* RE: [PATCH 1/4] [v4][POWERPC] refactor dcr code
2008-05-06 4:02 ` Benjamin Herrenschmidt
@ 2008-05-06 17:34 ` Stephen Neuendorffer
2008-05-06 17:40 ` Josh Boyer
0 siblings, 1 reply; 45+ messages in thread
From: Stephen Neuendorffer @ 2008-05-06 17:34 UTC (permalink / raw)
To: benh, Stephen Rothwell; +Cc: linuxppc-dev
I'll fix it.
> -----Original Message-----
> From: Benjamin Herrenschmidt [mailto:benh@kernel.crashing.org]
> Sent: Monday, May 05, 2008 9:02 PM
> To: Stephen Rothwell
> Cc: Stephen Neuendorffer; jwboyer@linux.vnet.ibm.com;
grant.likely@secretlab.ca; linuxppc-
> dev@ozlabs.org
> Subject: Re: [PATCH 1/4] [v4][POWERPC] refactor dcr code
>=20
>=20
> On Tue, 2008-05-06 at 13:40 +1000, Stephen Rothwell wrote:
> > Since find_dcr_parent has done a of_node_get on its return value,
you
> > leak a reference to dp here i.e. you need an of_node_put(dp) before
> > you return.
>=20
> He inherited that bug from other dcr.c functions I wrote, my fault.
>=20
> Stephen (N. not R.), would you mind fixing them while at it or do you
> want me to cook up a patch ?
>=20
> Cheers,
> Ben.
>=20
>=20
^ permalink raw reply [flat|nested] 45+ messages in thread
* [PATCH 1/4] [v5][POWERPC] refactor dcr code
2008-05-06 3:40 ` Stephen Rothwell
2008-05-06 4:02 ` Benjamin Herrenschmidt
@ 2008-05-06 18:29 ` Stephen Neuendorffer
1 sibling, 0 replies; 45+ messages in thread
From: Stephen Neuendorffer @ 2008-05-06 18:29 UTC (permalink / raw)
To: jwboyer, benh, grant.likely, linuxppc-dev, sfr
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>
--
[v4]
Converted flags to be DCR_HOST_*
Added WARN_ON in case someone forgets to call MAP_OK()
Fixed dealing with the dcr-access-method correctly, which was still
off in the wrong patch.
[v5]
Fixed leaked reference to the device tree.
---
arch/powerpc/sysdev/dcr.c | 154 +++++++++++++++++++++++++++++--------
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 | 39 +++++++++-
5 files changed, 231 insertions(+), 47 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..5f39a79 100644
--- a/arch/powerpc/sysdev/dcr.c
+++ b/arch/powerpc/sysdev/dcr.c
@@ -23,6 +23,105 @@
#include <asm/prom.h>
#include <asm/dcr.h>
+static struct device_node *find_dcr_parent(struct device_node *node)
+{
+ struct device_node *par, *tmp;
+ const u32 *p;
+
+ for (par = of_node_get(node); par;) {
+ if (of_get_property(par, "dcr-controller", NULL))
+ break;
+ p = of_get_property(par, "dcr-parent", NULL);
+ tmp = par;
+ if (p == NULL)
+ par = of_get_parent(par);
+ else
+ par = of_find_node_by_phandle(*p);
+ of_node_put(tmp);
+ }
+ return par;
+}
+
+#if defined(CONFIG_PPC_DCR_NATIVE) && defined(CONFIG_PPC_DCR_MMIO)
+
+bool dcr_map_ok_generic(dcr_host_t host)
+{
+ if (host.type == DCR_HOST_NATIVE)
+ return dcr_map_ok_native(host.host.native);
+ else if (host.type == DCR_HOST_MMIO)
+ return dcr_map_ok_mmio(host.host.mmio);
+ else
+ return 0;
+}
+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;
+ struct device_node *dp;
+ const char *prop;
+
+ host.type = DCR_HOST_INVALID;
+
+ dp = find_dcr_parent(dev);
+ if (dp == NULL)
+ return host;
+
+ prop = of_get_property(dp, "dcr-access-method", NULL);
+
+ pr_debug("dcr_map_generic(dcr-access-method = %s)\n", prop);
+
+ if (!strcmp(prop, "native")) {
+ host.type = DCR_HOST_NATIVE;
+ host.host.native = dcr_map_native(dev, dcr_n, dcr_c);
+ } else if (!strcmp(prop, "mmio")) {
+ host.type = DCR_HOST_MMIO;
+ host.host.mmio = dcr_map_mmio(dev, dcr_n, dcr_c);
+ }
+
+ of_node_put(dp);
+ return host;
+}
+EXPORT_SYMBOL_GPL(dcr_map_generic);
+
+void dcr_unmap_generic(dcr_host_t host, unsigned int dcr_c)
+{
+ if (host.type == DCR_HOST_NATIVE)
+ dcr_unmap_native(host.host.native, dcr_c);
+ else if (host.type == DCR_HOST_MMIO)
+ dcr_unmap_mmio(host.host.mmio, dcr_c);
+ else /* host.type == DCR_HOST_INVALID */
+ WARN_ON(true);
+}
+EXPORT_SYMBOL_GPL(dcr_unmap_generic);
+
+u32 dcr_read_generic(dcr_host_t host, unsigned int dcr_n)
+{
+ if (host.type == DCR_HOST_NATIVE)
+ return dcr_read_native(host.host.native, dcr_n);
+ else if (host.type == DCR_HOST_MMIO)
+ return dcr_read_mmio(host.host.mmio, dcr_n);
+ else /* host.type == DCR_HOST_INVALID */
+ WARN_ON(true);
+ return 0;
+}
+EXPORT_SYMBOL_GPL(dcr_read_generic);
+
+void dcr_write_generic(dcr_host_t host, unsigned int dcr_n, u32 value)
+{
+ if (host.type == DCR_HOST_NATIVE)
+ dcr_write_native(host.host.native, dcr_n, value);
+ else if (host.type == DCR_HOST_MMIO)
+ dcr_write_mmio(host.host.mmio, dcr_n, value);
+ else /* host.type == DCR_HOST_INVALID */
+ WARN_ON(true);
+}
+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,26 +146,7 @@ unsigned int dcr_resource_len(struct device_node *np, unsigned int index)
}
EXPORT_SYMBOL_GPL(dcr_resource_len);
-#ifndef CONFIG_PPC_DCR_NATIVE
-
-static struct device_node * find_dcr_parent(struct device_node * node)
-{
- struct device_node *par, *tmp;
- const u32 *p;
-
- for (par = of_node_get(node); par;) {
- if (of_get_property(par, "dcr-controller", NULL))
- break;
- p = of_get_property(par, "dcr-parent", NULL);
- tmp = par;
- if (p == NULL)
- par = of_get_parent(par);
- else
- par = of_find_node_by_phandle(*p);
- of_node_put(tmp);
- }
- return par;
-}
+#ifdef CONFIG_PPC_DCR_MMIO
u64 of_translate_dcr_address(struct device_node *dev,
unsigned int dcr_n,
@@ -75,7 +155,7 @@ u64 of_translate_dcr_address(struct device_node *dev,
struct device_node *dp;
const u32 *p;
unsigned int stride;
- u64 ret;
+ u64 ret = OF_BAD_ADDR;
dp = find_dcr_parent(dev);
if (dp == NULL)
@@ -90,7 +170,7 @@ u64 of_translate_dcr_address(struct device_node *dev,
if (p == NULL)
p = of_get_property(dp, "dcr-mmio-space", NULL);
if (p == NULL)
- return OF_BAD_ADDR;
+ goto done;
/* Maybe could do some better range checking here */
ret = of_translate_address(dp, p);
@@ -98,21 +178,25 @@ u64 of_translate_dcr_address(struct device_node *dev,
ret += (u64)(stride) * (u64)dcr_n;
if (out_stride)
*out_stride = stride;
+
+ done:
+ of_node_put(dp);
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 +208,11 @@ dcr_host_t dcr_map(struct device_node *dev, unsigned int dcr_n,
ret.token -= dcr_n * ret.stride;
return ret;
}
-EXPORT_SYMBOL_GPL(dcr_map);
+EXPORT_SYMBOL_GPL(dcr_map_mmio);
-void dcr_unmap(dcr_host_t host, unsigned int dcr_c)
+void dcr_unmap_mmio(dcr_host_mmio_t host, unsigned int dcr_c)
{
- dcr_host_t h = host;
+ dcr_host_mmio_t h = host;
if (h.token == NULL)
return;
@@ -136,7 +220,11 @@ void dcr_unmap(dcr_host_t host, unsigned int dcr_c)
iounmap(h.token);
h.token = NULL;
}
-EXPORT_SYMBOL_GPL(dcr_unmap);
-#else /* defined(CONFIG_PPC_DCR_NATIVE) */
+EXPORT_SYMBOL_GPL(dcr_unmap_mmio);
+
+#endif /* defined(CONFIG_PPC_DCR_MMIO) */
+
+#ifdef CONFIG_PPC_DCR_NATIVE
DEFINE_SPINLOCK(dcr_ind_lock);
-#endif /* !defined(CONFIG_PPC_DCR_NATIVE) */
+#endif /* defined(CONFIG_PPC_DCR_NATIVE) */
+
diff --git a/include/asm-powerpc/dcr-generic.h b/include/asm-powerpc/dcr-generic.h
new file mode 100644
index 0000000..35b7159
--- /dev/null
+++ b/include/asm-powerpc/dcr-generic.h
@@ -0,0 +1,49 @@
+/*
+ * (c) Copyright 2006 Benjamin Herrenschmidt, IBM Corp.
+ * <benh@kernel.crashing.org>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See
+ * the GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program; if not, write to the Free Software
+ * Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA 02111-1307 USA
+ */
+
+#ifndef _ASM_POWERPC_DCR_GENERIC_H
+#define _ASM_POWERPC_DCR_GENERIC_H
+#ifdef __KERNEL__
+#ifndef __ASSEMBLY__
+
+enum host_type_t {DCR_HOST_MMIO, DCR_HOST_NATIVE, DCR_HOST_INVALID};
+
+typedef struct {
+ enum host_type_t type;
+ union {
+ dcr_host_mmio_t mmio;
+ dcr_host_native_t native;
+ } host;
+} dcr_host_t;
+
+extern bool dcr_map_ok_generic(dcr_host_t host);
+
+extern dcr_host_t dcr_map_generic(struct device_node *dev, unsigned int dcr_n,
+ unsigned int dcr_c);
+extern void dcr_unmap_generic(dcr_host_t host, unsigned int dcr_c);
+
+extern u32 dcr_read_generic(dcr_host_t host, unsigned int dcr_n);
+
+extern void dcr_write_generic(dcr_host_t host, unsigned int dcr_n, u32 value);
+
+#endif /* __ASSEMBLY__ */
+#endif /* __KERNEL__ */
+#endif /* _ASM_POWERPC_DCR_GENERIC_H */
+
+
diff --git a/include/asm-powerpc/dcr-mmio.h b/include/asm-powerpc/dcr-mmio.h
index 08532ff..acd491d 100644
--- a/include/asm-powerpc/dcr-mmio.h
+++ b/include/asm-powerpc/dcr-mmio.h
@@ -27,20 +27,26 @@ typedef struct {
void __iomem *token;
unsigned int stride;
unsigned int base;
-} dcr_host_t;
+} dcr_host_mmio_t;
-#define DCR_MAP_OK(host) ((host).token != NULL)
+static inline bool dcr_map_ok_mmio(dcr_host_mmio_t host)
+{
+ return host.token != NULL;
+}
-extern dcr_host_t dcr_map(struct device_node *dev, unsigned int dcr_n,
- unsigned int dcr_c);
-extern void dcr_unmap(dcr_host_t host, unsigned int dcr_c);
+extern dcr_host_mmio_t dcr_map_mmio(struct device_node *dev,
+ unsigned int dcr_n,
+ unsigned int dcr_c);
+extern void dcr_unmap_mmio(dcr_host_mmio_t host, unsigned int dcr_c);
-static inline u32 dcr_read(dcr_host_t host, unsigned int dcr_n)
+static inline u32 dcr_read_mmio(dcr_host_mmio_t host, unsigned int dcr_n)
{
return in_be32(host.token + ((host.base + dcr_n) * host.stride));
}
-static inline void dcr_write(dcr_host_t host, unsigned int dcr_n, u32 value)
+static inline void dcr_write_mmio(dcr_host_mmio_t host,
+ unsigned int dcr_n,
+ u32 value)
{
out_be32(host.token + ((host.base + dcr_n) * host.stride), value);
}
diff --git a/include/asm-powerpc/dcr-native.h b/include/asm-powerpc/dcr-native.h
index f8398ce..72d2b72 100644
--- a/include/asm-powerpc/dcr-native.h
+++ b/include/asm-powerpc/dcr-native.h
@@ -26,14 +26,18 @@
typedef struct {
unsigned int base;
-} dcr_host_t;
+} dcr_host_native_t;
-#define DCR_MAP_OK(host) (1)
+static inline bool dcr_map_ok_native(dcr_host_native_t host)
+{
+ return 1;
+}
-#define dcr_map(dev, dcr_n, dcr_c) ((dcr_host_t){ .base = (dcr_n) })
-#define dcr_unmap(host, dcr_c) do {} while (0)
-#define dcr_read(host, dcr_n) mfdcr(dcr_n + host.base)
-#define dcr_write(host, dcr_n, value) mtdcr(dcr_n + host.base, value)
+#define dcr_map_native(dev, dcr_n, dcr_c) \
+ ((dcr_host_native_t){ .base = (dcr_n) })
+#define dcr_unmap_native(host, dcr_c) do {} while (0)
+#define dcr_read_native(host, dcr_n) mfdcr(dcr_n + host.base)
+#define dcr_write_native(host, dcr_n, value) mtdcr(dcr_n + host.base, value)
/* Device Control Registers */
void __mtdcr(int reg, unsigned int val);
diff --git a/include/asm-powerpc/dcr.h b/include/asm-powerpc/dcr.h
index 9338d50..53b2830 100644
--- a/include/asm-powerpc/dcr.h
+++ b/include/asm-powerpc/dcr.h
@@ -20,14 +20,50 @@
#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
+
+/* Indirection layer for providing both NATIVE and MMIO support. */
+
+#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) */
+
/*
* On CONFIG_PPC_MERGE, we have additional helpers to read the DCR
* base from the device-tree
@@ -41,5 +77,6 @@ extern unsigned int dcr_resource_len(struct device_node *np,
#endif /* CONFIG_PPC_MERGE */
#endif /* CONFIG_PPC_DCR */
+#endif /* __ASSEMBLY__ */
#endif /* __KERNEL__ */
#endif /* _ASM_POWERPC_DCR_H */
--
1.5.3.4
^ permalink raw reply related [flat|nested] 45+ messages in thread
[parent not found: <1210010201-28436-2-git-send-email-stephen.neuendorffer@xilinx.com>]
* [PATCH 2/4] [POWERPC] Xilinx: Virtex: Enable dcr for MMIO and NATIVE
[not found] ` <1210010201-28436-2-git-send-email-stephen.neuendorffer@xilinx.com>
@ 2008-05-05 17:56 ` Stephen Neuendorffer
2008-05-06 0:11 ` Benjamin Herrenschmidt
[not found] ` <1210010201-28436-3-git-send-email-stephen.neuendorffer@xilinx.com>
1 sibling, 1 reply; 45+ messages in thread
From: Stephen Neuendorffer @ 2008-05-05 17:56 UTC (permalink / raw)
To: jwboyer, benh, grant.likely, linuxppc-dev
FPGA designs may have need of both MMIO-based and NATIVE-based dcr
interfaces.
Signed-off-by: Stephen Neuendorffer <stephen.neuendorffer@xilinx.com>
---
arch/powerpc/platforms/40x/Kconfig | 2 ++
1 files changed, 2 insertions(+), 0 deletions(-)
diff --git a/arch/powerpc/platforms/40x/Kconfig b/arch/powerpc/platforms/40x/Kconfig
index a9260e2..b8e06df 100644
--- a/arch/powerpc/platforms/40x/Kconfig
+++ b/arch/powerpc/platforms/40x/Kconfig
@@ -123,6 +123,8 @@ config 405GPR
config XILINX_VIRTEX
bool
+ select PPC_DCR_MMIO
+ select PPC_DCR_NATIVE
config XILINX_VIRTEX_II_PRO
bool
--
1.5.3.4-dirty
^ permalink raw reply related [flat|nested] 45+ messages in thread* Re: [PATCH 2/4] [POWERPC] Xilinx: Virtex: Enable dcr for MMIO and NATIVE
2008-05-05 17:56 ` [PATCH 2/4] [POWERPC] Xilinx: Virtex: Enable dcr for MMIO and NATIVE Stephen Neuendorffer
@ 2008-05-06 0:11 ` Benjamin Herrenschmidt
2008-05-06 17:33 ` [PATCH 2/4] [POWERPC] Xilinx: Virtex: Enable dcr for MMIO andNATIVE Stephen Neuendorffer
0 siblings, 1 reply; 45+ messages in thread
From: Benjamin Herrenschmidt @ 2008-05-06 0:11 UTC (permalink / raw)
To: Stephen Neuendorffer; +Cc: linuxppc-dev
On Mon, 2008-05-05 at 10:56 -0700, Stephen Neuendorffer wrote:
> FPGA designs may have need of both MMIO-based and NATIVE-based dcr
> interfaces.
You say _may_ ... wouldn't it be better if it was thus left to a given
virtex based platform to enable DCR_MMIO if it uses it and leave only
NATIVE by default ?
> Signed-off-by: Stephen Neuendorffer <stephen.neuendorffer@xilinx.com>
> ---
> arch/powerpc/platforms/40x/Kconfig | 2 ++
> 1 files changed, 2 insertions(+), 0 deletions(-)
>
> diff --git a/arch/powerpc/platforms/40x/Kconfig b/arch/powerpc/platforms/40x/Kconfig
> index a9260e2..b8e06df 100644
> --- a/arch/powerpc/platforms/40x/Kconfig
> +++ b/arch/powerpc/platforms/40x/Kconfig
> @@ -123,6 +123,8 @@ config 405GPR
>
> config XILINX_VIRTEX
> bool
> + select PPC_DCR_MMIO
> + select PPC_DCR_NATIVE
>
> config XILINX_VIRTEX_II_PRO
> bool
^ permalink raw reply [flat|nested] 45+ messages in thread
* RE: [PATCH 2/4] [POWERPC] Xilinx: Virtex: Enable dcr for MMIO andNATIVE
2008-05-06 0:11 ` Benjamin Herrenschmidt
@ 2008-05-06 17:33 ` Stephen Neuendorffer
0 siblings, 0 replies; 45+ messages in thread
From: Stephen Neuendorffer @ 2008-05-06 17:33 UTC (permalink / raw)
To: benh; +Cc: linuxppc-dev
Yes, but this is a symptom of a separate problem, which is that it is
sometimes valuable to generate an optimized kernel configuration for a
particular FPGA system. First order, I'm more concerned about getting a
kernel which is generic and can work with (more or less) any device
tree.
Steve
> -----Original Message-----
> From: Benjamin Herrenschmidt [mailto:benh@kernel.crashing.org]
> Sent: Monday, May 05, 2008 5:12 PM
> To: Stephen Neuendorffer
> Cc: jwboyer@linux.vnet.ibm.com; grant.likely@secretlab.ca;
linuxppc-dev@ozlabs.org
> Subject: Re: [PATCH 2/4] [POWERPC] Xilinx: Virtex: Enable dcr for MMIO
andNATIVE
>=20
>=20
> On Mon, 2008-05-05 at 10:56 -0700, Stephen Neuendorffer wrote:
> > FPGA designs may have need of both MMIO-based and NATIVE-based dcr
> > interfaces.
>=20
> You say _may_ ... wouldn't it be better if it was thus left to a given
> virtex based platform to enable DCR_MMIO if it uses it and leave only
> NATIVE by default ?
>=20
> > Signed-off-by: Stephen Neuendorffer
<stephen.neuendorffer@xilinx.com>
> > ---
> > arch/powerpc/platforms/40x/Kconfig | 2 ++
> > 1 files changed, 2 insertions(+), 0 deletions(-)
> >
> > diff --git a/arch/powerpc/platforms/40x/Kconfig
b/arch/powerpc/platforms/40x/Kconfig
> > index a9260e2..b8e06df 100644
> > --- a/arch/powerpc/platforms/40x/Kconfig
> > +++ b/arch/powerpc/platforms/40x/Kconfig
> > @@ -123,6 +123,8 @@ config 405GPR
> >
> > config XILINX_VIRTEX
> > bool
> > + select PPC_DCR_MMIO
> > + select PPC_DCR_NATIVE
> >
> > config XILINX_VIRTEX_II_PRO
> > bool
>=20
^ permalink raw reply [flat|nested] 45+ messages in thread
[parent not found: <1210010201-28436-3-git-send-email-stephen.neuendorffer@xilinx.com>]
* [PATCH 3/4] [POWERPC] Xilinx: Framebuffer: remove platform device support.
[not found] ` <1210010201-28436-3-git-send-email-stephen.neuendorffer@xilinx.com>
@ 2008-05-05 17:56 ` Stephen Neuendorffer
[not found] ` <1210010201-28436-4-git-send-email-stephen.neuendorffer@xilinx.com>
1 sibling, 0 replies; 45+ messages in thread
From: Stephen Neuendorffer @ 2008-05-05 17:56 UTC (permalink / raw)
To: jwboyer, benh, grant.likely, linuxppc-dev
With ARCH=ppc going away, we don't really need platform device support
anymore. In fact it is hard, if we want to use the generic dcr
infrastructure.
Signed-off-by: Stephen Neuendorffer <stephen.neuendorffer@xilinx.com>
---
drivers/video/xilinxfb.c | 63 +---------------------------------------------
1 files changed, 1 insertions(+), 62 deletions(-)
diff --git a/drivers/video/xilinxfb.c b/drivers/video/xilinxfb.c
index 7b3a842..4d64402 100644
--- a/drivers/video/xilinxfb.c
+++ b/drivers/video/xilinxfb.c
@@ -31,7 +31,6 @@
#include <linux/fb.h>
#include <linux/init.h>
#include <linux/dma-mapping.h>
-#include <linux/platform_device.h>
#if defined(CONFIG_OF)
#include <linux/of_device.h>
#include <linux/of_platform.h>
@@ -356,56 +355,6 @@ static int xilinxfb_release(struct device *dev)
}
/* ---------------------------------------------------------------------
- * Platform bus binding
- */
-
-static int
-xilinxfb_platform_probe(struct platform_device *pdev)
-{
- struct xilinxfb_platform_data *pdata;
- struct resource *res;
-
- /* Find the registers address */
- res = platform_get_resource(pdev, IORESOURCE_IO, 0);
- if (!res) {
- dev_err(&pdev->dev, "Couldn't get registers resource\n");
- return -ENODEV;
- }
-
- /* If a pdata structure is provided, then extract the parameters */
- pdata = &xilinx_fb_default_pdata;
- if (pdev->dev.platform_data) {
- pdata = pdev->dev.platform_data;
- if (!pdata->xres)
- pdata->xres = xilinx_fb_default_pdata.xres;
- if (!pdata->yres)
- pdata->yres = xilinx_fb_default_pdata.yres;
- if (!pdata->xvirt)
- pdata->xvirt = xilinx_fb_default_pdata.xvirt;
- if (!pdata->yvirt)
- pdata->yvirt = xilinx_fb_default_pdata.yvirt;
- }
-
- return xilinxfb_assign(&pdev->dev, res->start, pdata);
-}
-
-static int
-xilinxfb_platform_remove(struct platform_device *pdev)
-{
- return xilinxfb_release(&pdev->dev);
-}
-
-
-static struct platform_driver xilinxfb_platform_driver = {
- .probe = xilinxfb_platform_probe,
- .remove = xilinxfb_platform_remove,
- .driver = {
- .owner = THIS_MODULE,
- .name = DRIVER_NAME,
- },
-};
-
-/* ---------------------------------------------------------------------
* OF bus binding
*/
@@ -500,22 +449,12 @@ static inline void __exit xilinxfb_of_unregister(void) { }
static int __init
xilinxfb_init(void)
{
- int rc;
- rc = xilinxfb_of_register();
- if (rc)
- return rc;
-
- rc = platform_driver_register(&xilinxfb_platform_driver);
- if (rc)
- xilinxfb_of_unregister();
-
- return rc;
+ return xilinxfb_of_register();
}
static void __exit
xilinxfb_cleanup(void)
{
- platform_driver_unregister(&xilinxfb_platform_driver);
xilinxfb_of_unregister();
}
--
1.5.3.4-dirty
^ permalink raw reply related [flat|nested] 45+ messages in thread[parent not found: <1210010201-28436-4-git-send-email-stephen.neuendorffer@xilinx.com>]
* [PATCH 4/4] [POWERPC] Xilinx: Framebuffer: Use dcr infrastructure.
[not found] ` <1210010201-28436-4-git-send-email-stephen.neuendorffer@xilinx.com>
@ 2008-05-05 17:56 ` Stephen Neuendorffer
2008-05-06 4:55 ` Grant Likely
0 siblings, 1 reply; 45+ messages in thread
From: Stephen Neuendorffer @ 2008-05-05 17:56 UTC (permalink / raw)
To: jwboyer, benh, grant.likely, linuxppc-dev
This device contains a dcr interface. Previously, the dcr interface
was assumed to be used in mmio mode, and the register space of the dcr
interface was precomputed and stuffed in the device tree. This patch
makes use of the new dcr infrastructure to represent the dcr interface
as any other dcr interface in the device tree. This enables the dcr
interface to be connected directly to a native dcr interface in a
clean way.
In particular, the device tree expected looks like:
dcr_v29_0: dcr@0 {
#address-cells = <1>;
#size-cells = <1>;
compatible = "xlnx,dcr-v29-1.00.a";
VGA_FrameBuffer: tft@80 {
compatible = "xlnx,plb-tft-cntlr-ref-1.00.a";
dcr-parent = <&opb2dcr_bridge_0>;
dcr-reg = < 80 2 >;
xlnx,default-tft-base-addr = <7f>;
xlnx,dps-init = <1>;
xlnx,on-init = <1>;
xlnx,pixclk-is-busclk-divby4 = <1>;
} ;
} ;
opb2dcr_bridge_0: opb2dcr-bridge@40700000 {
compatible = "xlnx,opb2dcr-bridge-1.00.b";
dcr-access-method = "mmio";
dcr-controller ;
dcr-mmio-range = < 40700000 1000 >;
dcr-mmio-stride = <4>;
reg = < 40700000 1000 >;
xlnx,family = "virtex2p";
} ;
Signed-off-by: Stephen Neuendorffer <stephen.neuendorffer@xilinx.com>
---
drivers/video/xilinxfb.c | 54 +++++++++++++++++-----------------------------
1 files changed, 20 insertions(+), 34 deletions(-)
diff --git a/drivers/video/xilinxfb.c b/drivers/video/xilinxfb.c
index 4d64402..848752e 100644
--- a/drivers/video/xilinxfb.c
+++ b/drivers/video/xilinxfb.c
@@ -37,6 +37,7 @@
#endif
#include <asm/io.h>
#include <linux/xilinxfb.h>
+#include <asm/dcr.h>
#define DRIVER_NAME "xilinxfb"
#define DRIVER_DESCRIPTION "Xilinx TFT LCD frame buffer driver"
@@ -111,8 +112,9 @@ struct xilinxfb_drvdata {
struct fb_info info; /* FB driver info record */
- u32 regs_phys; /* phys. address of the control registers */
- u32 __iomem *regs; /* virt. address of the control registers */
+ dcr_host_t dcr_host;
+ unsigned int dcr_start;
+ unsigned int dcr_len;
void *fb_virt; /* virt. address of the frame buffer */
dma_addr_t fb_phys; /* phys. address of the frame buffer */
@@ -135,7 +137,7 @@ struct xilinxfb_drvdata {
* when it's needed.
*/
#define xilinx_fb_out_be32(driverdata, offset, val) \
- out_be32(driverdata->regs + offset, val)
+ dcr_write(driverdata->dcr_host, offset, val)
static int
xilinx_fb_setcolreg(unsigned regno, unsigned red, unsigned green, unsigned blue,
@@ -203,7 +205,8 @@ static struct fb_ops xilinxfb_ops =
* Bus independent setup/teardown
*/
-static int xilinxfb_assign(struct device *dev, unsigned long physaddr,
+static int xilinxfb_assign(struct device *dev, dcr_host_t dcr_host,
+ unsigned int dcr_start, unsigned int dcr_len,
struct xilinxfb_platform_data *pdata)
{
struct xilinxfb_drvdata *drvdata;
@@ -218,21 +221,9 @@ static int xilinxfb_assign(struct device *dev, unsigned long physaddr,
}
dev_set_drvdata(dev, drvdata);
- /* Map the control registers in */
- if (!request_mem_region(physaddr, 8, DRIVER_NAME)) {
- dev_err(dev, "Couldn't lock memory region at 0x%08lX\n",
- physaddr);
- rc = -ENODEV;
- goto err_region;
- }
- drvdata->regs_phys = physaddr;
- drvdata->regs = ioremap(physaddr, 8);
- if (!drvdata->regs) {
- dev_err(dev, "Couldn't lock memory region at 0x%08lX\n",
- physaddr);
- rc = -ENODEV;
- goto err_map;
- }
+ drvdata->dcr_start = dcr_start;
+ drvdata->dcr_len = dcr_len;
+ drvdata->dcr_host = dcr_host;
/* Allocate the framebuffer memory */
if (pdata->fb_phys) {
@@ -247,7 +238,7 @@ static int xilinxfb_assign(struct device *dev, unsigned long physaddr,
if (!drvdata->fb_virt) {
dev_err(dev, "Could not allocate frame buffer memory\n");
rc = -ENOMEM;
- goto err_fbmem;
+ goto err_region;
}
/* Clear (turn to black) the framebuffer */
@@ -297,7 +288,6 @@ static int xilinxfb_assign(struct device *dev, unsigned long physaddr,
}
/* Put a banner in the log (for DEBUG) */
- dev_dbg(dev, "regs: phys=%lx, virt=%p\n", physaddr, drvdata->regs);
dev_dbg(dev, "fb: phys=%p, virt=%p, size=%x\n",
(void*)drvdata->fb_phys, drvdata->fb_virt, fbsize);
@@ -313,12 +303,6 @@ err_cmap:
/* Turn off the display */
xilinx_fb_out_be32(drvdata, REG_CTRL, 0);
-err_fbmem:
- iounmap(drvdata->regs);
-
-err_map:
- release_mem_region(physaddr, 8);
-
err_region:
kfree(drvdata);
dev_set_drvdata(dev, NULL);
@@ -344,9 +328,8 @@ static int xilinxfb_release(struct device *dev)
/* Turn off the display */
xilinx_fb_out_be32(drvdata, REG_CTRL, 0);
- iounmap(drvdata->regs);
- release_mem_region(drvdata->regs_phys, 8);
+ dcr_unmap(drvdata->dcr_host, drvdata->dcr_len);
kfree(drvdata);
dev_set_drvdata(dev, NULL);
@@ -362,20 +345,23 @@ static int xilinxfb_release(struct device *dev)
static int __devinit
xilinxfb_of_probe(struct of_device *op, const struct of_device_id *match)
{
- struct resource res;
const u32 *prop;
struct xilinxfb_platform_data pdata;
int size, rc;
+ int start, len;
+ dcr_host_t dcr_host;
/* Copy with the default pdata (not a ptr reference!) */
pdata = xilinx_fb_default_pdata;
dev_dbg(&op->dev, "xilinxfb_of_probe(%p, %p)\n", op, match);
- rc = of_address_to_resource(op->node, 0, &res);
- if (rc) {
+ start = dcr_resource_start(op->node, 0);
+ len = dcr_resource_len(op->node, 0);
+ dcr_host = dcr_map(op->node, start, len);
+ if (!DCR_MAP_OK(dcr_host)) {
dev_err(&op->dev, "invalid address\n");
- return rc;
+ return -ENODEV;
}
prop = of_get_property(op->node, "phys-size", &size);
@@ -399,7 +385,7 @@ xilinxfb_of_probe(struct of_device *op, const struct of_device_id *match)
if (of_find_property(op->node, "rotate-display", NULL))
pdata.rotate_screen = 1;
- return xilinxfb_assign(&op->dev, res.start, &pdata);
+ return xilinxfb_assign(&op->dev, dcr_host, start, len, &pdata);
}
static int __devexit xilinxfb_of_remove(struct of_device *op)
--
1.5.3.4-dirty
^ permalink raw reply related [flat|nested] 45+ messages in thread* Re: [PATCH 4/4] [POWERPC] Xilinx: Framebuffer: Use dcr infrastructure.
2008-05-05 17:56 ` [PATCH 4/4] [POWERPC] Xilinx: Framebuffer: Use dcr infrastructure Stephen Neuendorffer
@ 2008-05-06 4:55 ` Grant Likely
2008-05-06 6:14 ` David Gibson
0 siblings, 1 reply; 45+ messages in thread
From: Grant Likely @ 2008-05-06 4:55 UTC (permalink / raw)
To: Stephen Neuendorffer; +Cc: linuxppc-dev
On Mon, May 5, 2008 at 11:56 AM, Stephen Neuendorffer
<stephen.neuendorffer@xilinx.com> wrote:
> This device contains a dcr interface. Previously, the dcr interface
> was assumed to be used in mmio mode, and the register space of the dcr
> interface was precomputed and stuffed in the device tree. This patch
> makes use of the new dcr infrastructure to represent the dcr interface
> as any other dcr interface in the device tree. This enables the dcr
> interface to be connected directly to a native dcr interface in a
> clean way.
>
> In particular, the device tree expected looks like:
>
> dcr_v29_0: dcr@0 {
> #address-cells = <1>;
> #size-cells = <1>;
> compatible = "xlnx,dcr-v29-1.00.a";
> VGA_FrameBuffer: tft@80 {
> compatible = "xlnx,plb-tft-cntlr-ref-1.00.a";
> dcr-parent = <&opb2dcr_bridge_0>;
> dcr-reg = < 80 2 >;
> xlnx,default-tft-base-addr = <7f>;
> xlnx,dps-init = <1>;
> xlnx,on-init = <1>;
> xlnx,pixclk-is-busclk-divby4 = <1>;
> } ;
> } ;
>
> opb2dcr_bridge_0: opb2dcr-bridge@40700000 {
> compatible = "xlnx,opb2dcr-bridge-1.00.b";
> dcr-access-method = "mmio";
> dcr-controller ;
> dcr-mmio-range = < 40700000 1000 >;
> dcr-mmio-stride = <4>;
> reg = < 40700000 1000 >;
> xlnx,family = "virtex2p";
> } ;
Hmmm, something doesn't quite feel right about this. The node
describing the tft device is a child of the dcr@0 node which is the
dcr bus. However, dcr bindings use dcr-bus and dcr-reg instead of
parent-child relationship to specify how to access the dcr registers.
So, in this example; if the device is described by tft@80, and the dcr
bus is described by opb2dcr-bridge@40700000, then what does dcr@0
describe? (I do understand what they really describe in EDK terms;
but I'm looking at it through device tree glasses).
I don't think the presence of a dcr@0 node is a problem, but in this
case #size/address-cells doesn't have any meaning (the child doesn't
have a reg property) and it looks like it should be a child of the
opb2dcr-bridge node (otherwise, what is it attached to?).
Cheers,
g.
>
> Signed-off-by: Stephen Neuendorffer <stephen.neuendorffer@xilinx.com>
> ---
> drivers/video/xilinxfb.c | 54 +++++++++++++++++-----------------------------
> 1 files changed, 20 insertions(+), 34 deletions(-)
>
> diff --git a/drivers/video/xilinxfb.c b/drivers/video/xilinxfb.c
> index 4d64402..848752e 100644
> --- a/drivers/video/xilinxfb.c
> +++ b/drivers/video/xilinxfb.c
> @@ -37,6 +37,7 @@
> #endif
> #include <asm/io.h>
> #include <linux/xilinxfb.h>
> +#include <asm/dcr.h>
>
> #define DRIVER_NAME "xilinxfb"
> #define DRIVER_DESCRIPTION "Xilinx TFT LCD frame buffer driver"
> @@ -111,8 +112,9 @@ struct xilinxfb_drvdata {
>
> struct fb_info info; /* FB driver info record */
>
> - u32 regs_phys; /* phys. address of the control registers */
> - u32 __iomem *regs; /* virt. address of the control registers */
> + dcr_host_t dcr_host;
> + unsigned int dcr_start;
> + unsigned int dcr_len;
>
> void *fb_virt; /* virt. address of the frame buffer */
> dma_addr_t fb_phys; /* phys. address of the frame buffer */
> @@ -135,7 +137,7 @@ struct xilinxfb_drvdata {
> * when it's needed.
> */
> #define xilinx_fb_out_be32(driverdata, offset, val) \
> - out_be32(driverdata->regs + offset, val)
> + dcr_write(driverdata->dcr_host, offset, val)
>
> static int
> xilinx_fb_setcolreg(unsigned regno, unsigned red, unsigned green, unsigned blue,
> @@ -203,7 +205,8 @@ static struct fb_ops xilinxfb_ops =
> * Bus independent setup/teardown
> */
>
> -static int xilinxfb_assign(struct device *dev, unsigned long physaddr,
> +static int xilinxfb_assign(struct device *dev, dcr_host_t dcr_host,
> + unsigned int dcr_start, unsigned int dcr_len,
> struct xilinxfb_platform_data *pdata)
> {
> struct xilinxfb_drvdata *drvdata;
> @@ -218,21 +221,9 @@ static int xilinxfb_assign(struct device *dev, unsigned long physaddr,
> }
> dev_set_drvdata(dev, drvdata);
>
> - /* Map the control registers in */
> - if (!request_mem_region(physaddr, 8, DRIVER_NAME)) {
> - dev_err(dev, "Couldn't lock memory region at 0x%08lX\n",
> - physaddr);
> - rc = -ENODEV;
> - goto err_region;
> - }
> - drvdata->regs_phys = physaddr;
> - drvdata->regs = ioremap(physaddr, 8);
> - if (!drvdata->regs) {
> - dev_err(dev, "Couldn't lock memory region at 0x%08lX\n",
> - physaddr);
> - rc = -ENODEV;
> - goto err_map;
> - }
> + drvdata->dcr_start = dcr_start;
> + drvdata->dcr_len = dcr_len;
> + drvdata->dcr_host = dcr_host;
>
> /* Allocate the framebuffer memory */
> if (pdata->fb_phys) {
> @@ -247,7 +238,7 @@ static int xilinxfb_assign(struct device *dev, unsigned long physaddr,
> if (!drvdata->fb_virt) {
> dev_err(dev, "Could not allocate frame buffer memory\n");
> rc = -ENOMEM;
> - goto err_fbmem;
> + goto err_region;
> }
>
> /* Clear (turn to black) the framebuffer */
> @@ -297,7 +288,6 @@ static int xilinxfb_assign(struct device *dev, unsigned long physaddr,
> }
>
> /* Put a banner in the log (for DEBUG) */
> - dev_dbg(dev, "regs: phys=%lx, virt=%p\n", physaddr, drvdata->regs);
> dev_dbg(dev, "fb: phys=%p, virt=%p, size=%x\n",
> (void*)drvdata->fb_phys, drvdata->fb_virt, fbsize);
>
> @@ -313,12 +303,6 @@ err_cmap:
> /* Turn off the display */
> xilinx_fb_out_be32(drvdata, REG_CTRL, 0);
>
> -err_fbmem:
> - iounmap(drvdata->regs);
> -
> -err_map:
> - release_mem_region(physaddr, 8);
> -
> err_region:
> kfree(drvdata);
> dev_set_drvdata(dev, NULL);
> @@ -344,9 +328,8 @@ static int xilinxfb_release(struct device *dev)
>
> /* Turn off the display */
> xilinx_fb_out_be32(drvdata, REG_CTRL, 0);
> - iounmap(drvdata->regs);
>
> - release_mem_region(drvdata->regs_phys, 8);
> + dcr_unmap(drvdata->dcr_host, drvdata->dcr_len);
>
> kfree(drvdata);
> dev_set_drvdata(dev, NULL);
> @@ -362,20 +345,23 @@ static int xilinxfb_release(struct device *dev)
> static int __devinit
> xilinxfb_of_probe(struct of_device *op, const struct of_device_id *match)
> {
> - struct resource res;
> const u32 *prop;
> struct xilinxfb_platform_data pdata;
> int size, rc;
> + int start, len;
> + dcr_host_t dcr_host;
>
> /* Copy with the default pdata (not a ptr reference!) */
> pdata = xilinx_fb_default_pdata;
>
> dev_dbg(&op->dev, "xilinxfb_of_probe(%p, %p)\n", op, match);
>
> - rc = of_address_to_resource(op->node, 0, &res);
> - if (rc) {
> + start = dcr_resource_start(op->node, 0);
> + len = dcr_resource_len(op->node, 0);
> + dcr_host = dcr_map(op->node, start, len);
> + if (!DCR_MAP_OK(dcr_host)) {
> dev_err(&op->dev, "invalid address\n");
> - return rc;
> + return -ENODEV;
> }
>
> prop = of_get_property(op->node, "phys-size", &size);
> @@ -399,7 +385,7 @@ xilinxfb_of_probe(struct of_device *op, const struct of_device_id *match)
> if (of_find_property(op->node, "rotate-display", NULL))
> pdata.rotate_screen = 1;
>
> - return xilinxfb_assign(&op->dev, res.start, &pdata);
> + return xilinxfb_assign(&op->dev, dcr_host, start, len, &pdata);
> }
>
> static int __devexit xilinxfb_of_remove(struct of_device *op)
> --
> 1.5.3.4-dirty
>
>
>
>
--
Grant Likely, B.Sc., P.Eng.
Secret Lab Technologies Ltd.
^ permalink raw reply [flat|nested] 45+ messages in thread* Re: [PATCH 4/4] [POWERPC] Xilinx: Framebuffer: Use dcr infrastructure.
2008-05-06 4:55 ` Grant Likely
@ 2008-05-06 6:14 ` David Gibson
2008-05-06 10:56 ` Segher Boessenkool
2008-05-06 17:43 ` [PATCH 4/4] [POWERPC] Xilinx: Framebuffer: Use dcrinfrastructure Stephen Neuendorffer
0 siblings, 2 replies; 45+ messages in thread
From: David Gibson @ 2008-05-06 6:14 UTC (permalink / raw)
To: Grant Likely; +Cc: linuxppc-dev
On Mon, May 05, 2008 at 10:55:53PM -0600, Grant Likely wrote:
> On Mon, May 5, 2008 at 11:56 AM, Stephen Neuendorffer
> <stephen.neuendorffer@xilinx.com> wrote:
> > This device contains a dcr interface. Previously, the dcr interface
> > was assumed to be used in mmio mode, and the register space of the dcr
> > interface was precomputed and stuffed in the device tree. This patch
> > makes use of the new dcr infrastructure to represent the dcr interface
> > as any other dcr interface in the device tree. This enables the dcr
> > interface to be connected directly to a native dcr interface in a
> > clean way.
> >
> > In particular, the device tree expected looks like:
> >
> > dcr_v29_0: dcr@0 {
> > #address-cells = <1>;
> > #size-cells = <1>;
> > compatible = "xlnx,dcr-v29-1.00.a";
> > VGA_FrameBuffer: tft@80 {
> > compatible = "xlnx,plb-tft-cntlr-ref-1.00.a";
> > dcr-parent = <&opb2dcr_bridge_0>;
> > dcr-reg = < 80 2 >;
> > xlnx,default-tft-base-addr = <7f>;
> > xlnx,dps-init = <1>;
> > xlnx,on-init = <1>;
> > xlnx,pixclk-is-busclk-divby4 = <1>;
> > } ;
> > } ;
> >
> > opb2dcr_bridge_0: opb2dcr-bridge@40700000 {
> > compatible = "xlnx,opb2dcr-bridge-1.00.b";
> > dcr-access-method = "mmio";
> > dcr-controller ;
> > dcr-mmio-range = < 40700000 1000 >;
> > dcr-mmio-stride = <4>;
> > reg = < 40700000 1000 >;
> > xlnx,family = "virtex2p";
> > } ;
>
> Hmmm, something doesn't quite feel right about this. The node
> describing the tft device is a child of the dcr@0 node which is the
> dcr bus. However, dcr bindings use dcr-bus and dcr-reg instead of
> parent-child relationship to specify how to access the dcr registers.
> So, in this example; if the device is described by tft@80, and the dcr
> bus is described by opb2dcr-bridge@40700000, then what does dcr@0
> describe? (I do understand what they really describe in EDK terms;
> but I'm looking at it through device tree glasses).
>
> I don't think the presence of a dcr@0 node is a problem, but in this
> case #size/address-cells doesn't have any meaning (the child doesn't
> have a reg property) and it looks like it should be a child of the
> opb2dcr-bridge node (otherwise, what is it attached to?).
Yes, indeed. If dcr@0 is representing the DCR bus / interface it
should really have the dcr-access-method property and have all the
dcr-parent handles point at it.
Current standard practice is not to represent the DCR bus as node with
subnodes for the DCR-controlled devices. That's because the DCR bus
tends to run in addition to other on-chip busses, and some things have
to go on another on-chip bus to make sense, but still have DCR control
registers (for example the internal bus bridges on 4xx).
Arguably for DCR-only devices we should instead have a node
representing the DCR bus and just put the devices under it with the
DCR number encoded in reg in the normal way. But then its
inconsistent with the devices that need the other DCR representation.
Segher and I did toss around some ideas for generalizing the DCR
representation to a way of representing that any node has some
presence on a bus other than its "primary" parent (e.g. other-bus-reg
= <&dcr-bus 0x0d0 0x010 &strange-i2c-control-bus 0xabc>). Then
DCR-only devices would use normal "reg", devices that sit on another
bus would sit on that bus and use this representation to show their
DCR control registers. Maybe one day.
--
David Gibson | I'll have my music baroque, and my code
david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_
| _way_ _around_!
http://www.ozlabs.org/~dgibson
^ permalink raw reply [flat|nested] 45+ messages in thread* Re: [PATCH 4/4] [POWERPC] Xilinx: Framebuffer: Use dcr infrastructure.
2008-05-06 6:14 ` David Gibson
@ 2008-05-06 10:56 ` Segher Boessenkool
2008-05-08 1:57 ` David Gibson
2008-05-06 17:43 ` [PATCH 4/4] [POWERPC] Xilinx: Framebuffer: Use dcrinfrastructure Stephen Neuendorffer
1 sibling, 1 reply; 45+ messages in thread
From: Segher Boessenkool @ 2008-05-06 10:56 UTC (permalink / raw)
To: David Gibson; +Cc: linuxppc-dev
> Current standard practice is not to represent the DCR bus as node with
> subnodes for the DCR-controlled devices. That's because the DCR bus
> tends to run in addition to other on-chip busses, and some things have
> to go on another on-chip bus to make sense, but still have DCR control
> registers (for example the internal bus bridges on 4xx).
Yeah.
> Arguably for DCR-only devices we should instead have a node
> representing the DCR bus and just put the devices under it with the
> DCR number encoded in reg in the normal way.
Right.
> But then its
> inconsistent with the devices that need the other DCR representation.
OTOH, it _is_ consistent with all other (non-DCR) devices that way.
What you could do right now is to give such DCR-only devices both
normal "reg" etc., and the "dcr-reg" etc. properties, containing
both the same info. If you do that, your device tree will be
correct (because you got all the standard stuff right), and the
kernel will like it as well (because it looks for the "dcr-reg"
stuff).
Then maybe later, if/when the kernel supports the standard addressing
for DCR as well, you could drop the "dcr-reg" things from your DTS.
Or you could just keep it.
David, will this work, do you think?
> Segher and I did toss around some ideas for generalizing the DCR
> representation to a way of representing that any node has some
> presence on a bus other than its "primary" parent (e.g. other-bus-reg
> = <&dcr-bus 0x0d0 0x010 &strange-i2c-control-bus 0xabc>). Then
> DCR-only devices would use normal "reg", devices that sit on another
> bus would sit on that bus and use this representation to show their
> DCR control registers. Maybe one day.
One day perhaps, yes :-)
It sounds cleaner to split such a prop into separate props per bus.
Maybe I said the opposite before, heh :-)
Segher
^ permalink raw reply [flat|nested] 45+ messages in thread
* Re: [PATCH 4/4] [POWERPC] Xilinx: Framebuffer: Use dcr infrastructure.
2008-05-06 10:56 ` Segher Boessenkool
@ 2008-05-08 1:57 ` David Gibson
0 siblings, 0 replies; 45+ messages in thread
From: David Gibson @ 2008-05-08 1:57 UTC (permalink / raw)
To: Segher Boessenkool; +Cc: linuxppc-dev
On Tue, May 06, 2008 at 12:56:44PM +0200, Segher Boessenkool wrote:
>> Current standard practice is not to represent the DCR bus as node with
>> subnodes for the DCR-controlled devices. That's because the DCR bus
>> tends to run in addition to other on-chip busses, and some things have
>> to go on another on-chip bus to make sense, but still have DCR control
>> registers (for example the internal bus bridges on 4xx).
>
> Yeah.
>
>> Arguably for DCR-only devices we should instead have a node
>> representing the DCR bus and just put the devices under it with the
>> DCR number encoded in reg in the normal way.
>
> Right.
>
>> But then its
>> inconsistent with the devices that need the other DCR representation.
>
> OTOH, it _is_ consistent with all other (non-DCR) devices that way.
>
> What you could do right now is to give such DCR-only devices both
> normal "reg" etc., and the "dcr-reg" etc. properties, containing
> both the same info. If you do that, your device tree will be
> correct (because you got all the standard stuff right), and the
> kernel will like it as well (because it looks for the "dcr-reg"
> stuff).
> Then maybe later, if/when the kernel supports the standard addressing
> for DCR as well, you could drop the "dcr-reg" things from your DTS.
> Or you could just keep it.
>
> David, will this work, do you think?
Um.. yeah, I guess that should work. Don't forget to slap on
"simple-bus" as appropriate.
>> Segher and I did toss around some ideas for generalizing the DCR
>> representation to a way of representing that any node has some
>> presence on a bus other than its "primary" parent (e.g. other-bus-reg
>> = <&dcr-bus 0x0d0 0x010 &strange-i2c-control-bus 0xabc>). Then
>> DCR-only devices would use normal "reg", devices that sit on another
>> bus would sit on that bus and use this representation to show their
>> DCR control registers. Maybe one day.
>
> One day perhaps, yes :-)
>
> It sounds cleaner to split such a prop into separate props per bus.
> Maybe I said the opposite before, heh :-)
No I think you said that before, too, but I'm not sure I agree.
Seperate props per bus (well, per bus type, really) is probably more
human-readable, but a single prop has the advantage that we can have a
common parser that will interpret future variations on this theme
correctly without having to add things to some list of properties to
look for.
--
David Gibson | I'll have my music baroque, and my code
david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_
| _way_ _around_!
http://www.ozlabs.org/~dgibson
^ permalink raw reply [flat|nested] 45+ messages in thread
* RE: [PATCH 4/4] [POWERPC] Xilinx: Framebuffer: Use dcrinfrastructure.
2008-05-06 6:14 ` David Gibson
2008-05-06 10:56 ` Segher Boessenkool
@ 2008-05-06 17:43 ` Stephen Neuendorffer
2008-05-08 1:59 ` David Gibson
1 sibling, 1 reply; 45+ messages in thread
From: Stephen Neuendorffer @ 2008-05-06 17:43 UTC (permalink / raw)
To: David Gibson, Grant Likely; +Cc: linuxppc-dev
> -----Original Message-----
> From: David Gibson [mailto:david@gibson.dropbear.id.au]
> Sent: Monday, May 05, 2008 11:15 PM
> To: Grant Likely
> Cc: Stephen Neuendorffer; linuxppc-dev@ozlabs.org
> Subject: Re: [PATCH 4/4] [POWERPC] Xilinx: Framebuffer: Use
dcrinfrastructure.
>=20
> On Mon, May 05, 2008 at 10:55:53PM -0600, Grant Likely wrote:
> > On Mon, May 5, 2008 at 11:56 AM, Stephen Neuendorffer
> > <stephen.neuendorffer@xilinx.com> wrote:
> > > This device contains a dcr interface. Previously, the dcr
interface
> > > was assumed to be used in mmio mode, and the register space of
the dcr
> > > interface was precomputed and stuffed in the device tree. This
patch
> > > makes use of the new dcr infrastructure to represent the dcr
interface
> > > as any other dcr interface in the device tree. This enables the
dcr
> > > interface to be connected directly to a native dcr interface in a
> > > clean way.
> > >
> > > In particular, the device tree expected looks like:
> > >
> > > dcr_v29_0: dcr@0 {
> > > #address-cells =3D <1>;
> > > #size-cells =3D <1>;
> > > compatible =3D
"xlnx,dcr-v29-1.00.a";
> > > VGA_FrameBuffer: tft@80 {
> > > compatible =3D
"xlnx,plb-tft-cntlr-ref-1.00.a";
> > > dcr-parent =3D
<&opb2dcr_bridge_0>;
> > > dcr-reg =3D < 80 2 >;
> > > xlnx,default-tft-base-addr
=3D <7f>;
> > > xlnx,dps-init =3D <1>;
> > > xlnx,on-init =3D <1>;
> > >
xlnx,pixclk-is-busclk-divby4 =3D <1>;
> > > } ;
> > > } ;
> > >
> > > opb2dcr_bridge_0: opb2dcr-bridge@40700000
{
> > > compatible =3D
"xlnx,opb2dcr-bridge-1.00.b";
> > > dcr-access-method =3D "mmio";
> > > dcr-controller ;
> > > dcr-mmio-range =3D < 40700000 1000
>;
> > > dcr-mmio-stride =3D <4>;
> > > reg =3D < 40700000 1000 >;
> > > xlnx,family =3D "virtex2p";
> > > } ;
> >
> > Hmmm, something doesn't quite feel right about this. The node
> > describing the tft device is a child of the dcr@0 node which is the
> > dcr bus. However, dcr bindings use dcr-bus and dcr-reg instead of
> > parent-child relationship to specify how to access the dcr
registers.
> > So, in this example; if the device is described by tft@80, and the
dcr
> > bus is described by opb2dcr-bridge@40700000, then what does dcr@0
> > describe? (I do understand what they really describe in EDK terms;
> > but I'm looking at it through device tree glasses).
> >
> > I don't think the presence of a dcr@0 node is a problem, but in this
> > case #size/address-cells doesn't have any meaning (the child doesn't
> > have a reg property) and it looks like it should be a child of the
> > opb2dcr-bridge node (otherwise, what is it attached to?).
>=20
> Yes, indeed. If dcr@0 is representing the DCR bus / interface it
> should really have the dcr-access-method property and have all the
> dcr-parent handles point at it.
Hmm, I tend to agree. Certainly the address-cells and size-cells can
go. Part of the nastiness is that I'm trying to maintain a modicum of
backward compatibility at the moment in the device tree generator. This
structure allow the dcr@0 node to have ranges; and the tft node to have
a properly translated reg =3D <> property for the existing driver which
only understands mmio. I don't think it really works for the opb2dcr
bridge to be a bridge and a dcr-controller at the same time. :) This
structure is also very similar to what is generated if the
dcr-controller is native from the processor (there's just no bridge).
> Current standard practice is not to represent the DCR bus as node with
> subnodes for the DCR-controlled devices. That's because the DCR bus
> tends to run in addition to other on-chip busses, and some things have
> to go on another on-chip bus to make sense, but still have DCR control
> registers (for example the internal bus bridges on 4xx).
>=20
> Arguably for DCR-only devices we should instead have a node
> representing the DCR bus and just put the devices under it with the
> DCR number encoded in reg in the normal way. But then its
> inconsistent with the devices that need the other DCR representation.
Yup, it's exactly this problem I'm trying to fix in the case of the tft
driver.
> Segher and I did toss around some ideas for generalizing the DCR
> representation to a way of representing that any node has some
> presence on a bus other than its "primary" parent (e.g. other-bus-reg
> =3D <&dcr-bus 0x0d0 0x010 &strange-i2c-control-bus 0xabc>). Then
> DCR-only devices would use normal "reg", devices that sit on another
> bus would sit on that bus and use this representation to show their
> DCR control registers. Maybe one day.
Don't know if I like this, since it obscures the types of the
interfaces.
Steve
^ permalink raw reply [flat|nested] 45+ messages in thread* Re: [PATCH 4/4] [POWERPC] Xilinx: Framebuffer: Use dcrinfrastructure.
2008-05-06 17:43 ` [PATCH 4/4] [POWERPC] Xilinx: Framebuffer: Use dcrinfrastructure Stephen Neuendorffer
@ 2008-05-08 1:59 ` David Gibson
2008-05-08 4:46 ` [PATCH 4/4] [POWERPC] Xilinx: Framebuffer: Usedcrinfrastructure Stephen Neuendorffer
0 siblings, 1 reply; 45+ messages in thread
From: David Gibson @ 2008-05-08 1:59 UTC (permalink / raw)
To: Stephen Neuendorffer; +Cc: linuxppc-dev
On Tue, May 06, 2008 at 10:43:50AM -0700, Stephen Neuendorffer wrote:
[snip]
> > > Hmmm, something doesn't quite feel right about this. The node
> > > describing the tft device is a child of the dcr@0 node which is the
> > > dcr bus. However, dcr bindings use dcr-bus and dcr-reg instead of
> > > parent-child relationship to specify how to access the dcr
> registers.
> > > So, in this example; if the device is described by tft@80, and the
> dcr
> > > bus is described by opb2dcr-bridge@40700000, then what does dcr@0
> > > describe? (I do understand what they really describe in EDK terms;
> > > but I'm looking at it through device tree glasses).
> > >
> > > I don't think the presence of a dcr@0 node is a problem, but in this
> > > case #size/address-cells doesn't have any meaning (the child doesn't
> > > have a reg property) and it looks like it should be a child of the
> > > opb2dcr-bridge node (otherwise, what is it attached to?).
> >
> > Yes, indeed. If dcr@0 is representing the DCR bus / interface it
> > should really have the dcr-access-method property and have all the
> > dcr-parent handles point at it.
>
> Hmm, I tend to agree. Certainly the address-cells and size-cells can
> go. Part of the nastiness is that I'm trying to maintain a modicum of
> backward compatibility at the moment in the device tree generator. This
> structure allow the dcr@0 node to have ranges; and the tft node to have
> a properly translated reg = <> property for the existing driver which
> only understands mmio. I don't think it really works for the opb2dcr
> bridge to be a bridge and a dcr-controller at the same time. :) This
> structure is also very similar to what is generated if the
> dcr-controller is native from the processor (there's just no bridge).
I don't really understand what you're getting at here, sorry. Perhaps
you could describe what you're doing with the tft device in more
detail?
> > Current standard practice is not to represent the DCR bus as node with
> > subnodes for the DCR-controlled devices. That's because the DCR bus
> > tends to run in addition to other on-chip busses, and some things have
> > to go on another on-chip bus to make sense, but still have DCR control
> > registers (for example the internal bus bridges on 4xx).
> >
> > Arguably for DCR-only devices we should instead have a node
> > representing the DCR bus and just put the devices under it with the
> > DCR number encoded in reg in the normal way. But then its
> > inconsistent with the devices that need the other DCR representation.
>
> Yup, it's exactly this problem I'm trying to fix in the case of the tft
> driver.
--
David Gibson | I'll have my music baroque, and my code
david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_
| _way_ _around_!
http://www.ozlabs.org/~dgibson
^ permalink raw reply [flat|nested] 45+ messages in thread
* RE: [PATCH 4/4] [POWERPC] Xilinx: Framebuffer: Usedcrinfrastructure.
2008-05-08 1:59 ` David Gibson
@ 2008-05-08 4:46 ` Stephen Neuendorffer
2008-05-08 7:01 ` David Gibson
0 siblings, 1 reply; 45+ messages in thread
From: Stephen Neuendorffer @ 2008-05-08 4:46 UTC (permalink / raw)
To: David Gibson; +Cc: linuxppc-dev
[-- Attachment #1: Type: text/plain, Size: 3882 bytes --]
The problem is that the tft driver currently in mainline assumes that the dcr control registers
are accessed through mmio, and probes for a reg=<> property. Since the device is actually a dcr device,
it can be connected via mmio through a bride, or to the native dcr bus of the processor. For the time being,
I'd like to generate device trees compatible with either mechanism.
so, the problem is that the tree-parents of the tft node all have ranges, and if the dcr-parent of the node is a bridge, then it has a reg=<> property,
But if the dcr-parent is also a tree-parent, then it has to have ranges; and a reg=<> property, which seems very strange, and not something I think is
a good thing to do.
Steve
-----Original Message-----
From: David Gibson [mailto:david@gibson.dropbear.id.au]
Sent: Wed 5/7/2008 6:59 PM
To: Stephen Neuendorffer
Cc: Grant Likely; linuxppc-dev@ozlabs.org
Subject: Re: [PATCH 4/4] [POWERPC] Xilinx: Framebuffer: Usedcrinfrastructure.
On Tue, May 06, 2008 at 10:43:50AM -0700, Stephen Neuendorffer wrote:
[snip]
> > > Hmmm, something doesn't quite feel right about this. The node
> > > describing the tft device is a child of the dcr@0 node which is the
> > > dcr bus. However, dcr bindings use dcr-bus and dcr-reg instead of
> > > parent-child relationship to specify how to access the dcr
> registers.
> > > So, in this example; if the device is described by tft@80, and the
> dcr
> > > bus is described by opb2dcr-bridge@40700000, then what does dcr@0
> > > describe? (I do understand what they really describe in EDK terms;
> > > but I'm looking at it through device tree glasses).
> > >
> > > I don't think the presence of a dcr@0 node is a problem, but in this
> > > case #size/address-cells doesn't have any meaning (the child doesn't
> > > have a reg property) and it looks like it should be a child of the
> > > opb2dcr-bridge node (otherwise, what is it attached to?).
> >
> > Yes, indeed. If dcr@0 is representing the DCR bus / interface it
> > should really have the dcr-access-method property and have all the
> > dcr-parent handles point at it.
>
> Hmm, I tend to agree. Certainly the address-cells and size-cells can
> go. Part of the nastiness is that I'm trying to maintain a modicum of
> backward compatibility at the moment in the device tree generator. This
> structure allow the dcr@0 node to have ranges; and the tft node to have
> a properly translated reg = <> property for the existing driver which
> only understands mmio. I don't think it really works for the opb2dcr
> bridge to be a bridge and a dcr-controller at the same time. :) This
> structure is also very similar to what is generated if the
> dcr-controller is native from the processor (there's just no bridge).
I don't really understand what you're getting at here, sorry. Perhaps
you could describe what you're doing with the tft device in more
detail?
> > Current standard practice is not to represent the DCR bus as node with
> > subnodes for the DCR-controlled devices. That's because the DCR bus
> > tends to run in addition to other on-chip busses, and some things have
> > to go on another on-chip bus to make sense, but still have DCR control
> > registers (for example the internal bus bridges on 4xx).
> >
> > Arguably for DCR-only devices we should instead have a node
> > representing the DCR bus and just put the devices under it with the
> > DCR number encoded in reg in the normal way. But then its
> > inconsistent with the devices that need the other DCR representation.
>
> Yup, it's exactly this problem I'm trying to fix in the case of the tft
> driver.
--
David Gibson | I'll have my music baroque, and my code
david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_
| _way_ _around_!
http://www.ozlabs.org/~dgibson
[-- Attachment #2: Type: text/html, Size: 5306 bytes --]
^ permalink raw reply [flat|nested] 45+ messages in thread
* Re: [PATCH 4/4] [POWERPC] Xilinx: Framebuffer: Usedcrinfrastructure.
2008-05-08 4:46 ` [PATCH 4/4] [POWERPC] Xilinx: Framebuffer: Usedcrinfrastructure Stephen Neuendorffer
@ 2008-05-08 7:01 ` David Gibson
0 siblings, 0 replies; 45+ messages in thread
From: David Gibson @ 2008-05-08 7:01 UTC (permalink / raw)
To: Stephen Neuendorffer; +Cc: linuxppc-dev
On Wed, May 07, 2008 at 09:46:51PM -0700, Stephen Neuendorffer wrote:
>
> The problem is that the tft driver currently in mainline assumes
> that the dcr control registers are accessed through mmio, and probes
> for a reg=<> property. Since the device is actually a dcr device,
> it can be connected via mmio through a bride, or to the native dcr
> bus of the processor. For the time being, I'd like to generate
> device trees compatible with either mechanism.
Ah, I think I see. The current driver expects the device to be
represented as if it were an mmio device, not using the standard dcr
notation. It can get away with this because the DCRs are
memory-mapped, in a way representable by 'ranges'.
> so, the problem is that the tree-parents of the tft node all have
> ranges, and if the dcr-parent of the node is a bridge, then it has a
> reg=<> property, But if the dcr-parent is also a tree-parent, then
> it has to have ranges; and a reg=<> property, which seems very
> strange, and not something I think is a good thing to do.
This I don't follow.
But I don't think it matters. The representation suggested by Segher,
which has the DCR addressing redundantly described in 'reg' and
'dcr-reg' properties should work for you.
--
David Gibson | I'll have my music baroque, and my code
david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_
| _way_ _around_!
http://www.ozlabs.org/~dgibson
^ permalink raw reply [flat|nested] 45+ messages in thread
[parent not found: <1208555704-8978-1-git-send-email-stephen.neuendorffer@xilinx.com>]
* [PATCH 2/2] [POWERPC] Xilinx: Virtex: Enable dcr for MMIO and NATIVE
[not found] ` <1208555704-8978-1-git-send-email-stephen.neuendorffer@xilinx.com>
@ 2008-04-18 21:55 ` Stephen Neuendorffer
2008-04-19 2:31 ` Grant Likely
0 siblings, 1 reply; 45+ messages in thread
From: Stephen Neuendorffer @ 2008-04-18 21:55 UTC (permalink / raw)
To: benh, linuxppc-dev
FPGA designs may have need of both MMIO-based and NATIVE-based dcr
interfaces.
Signed-off-by: Stephen Neuendorffer <stephen.neuendorffer@xilinx.com>
---
arch/powerpc/platforms/40x/Kconfig | 2 ++
1 files changed, 2 insertions(+), 0 deletions(-)
diff --git a/arch/powerpc/platforms/40x/Kconfig b/arch/powerpc/platforms/40x/Kconfig
index a9260e2..b8e06df 100644
--- a/arch/powerpc/platforms/40x/Kconfig
+++ b/arch/powerpc/platforms/40x/Kconfig
@@ -123,6 +123,8 @@ config 405GPR
config XILINX_VIRTEX
bool
+ select PPC_DCR_MMIO
+ select PPC_DCR_NATIVE
config XILINX_VIRTEX_II_PRO
bool
--
1.5.3.4-dirty
^ permalink raw reply related [flat|nested] 45+ messages in thread* Re: [PATCH 2/2] [POWERPC] Xilinx: Virtex: Enable dcr for MMIO and NATIVE
2008-04-18 21:55 ` [PATCH 2/2] [POWERPC] Xilinx: Virtex: Enable dcr for MMIO and NATIVE Stephen Neuendorffer
@ 2008-04-19 2:31 ` Grant Likely
0 siblings, 0 replies; 45+ messages in thread
From: Grant Likely @ 2008-04-19 2:31 UTC (permalink / raw)
To: Stephen Neuendorffer; +Cc: linuxppc-dev
On Fri, Apr 18, 2008 at 3:55 PM, Stephen Neuendorffer
<stephen.neuendorffer@xilinx.com> wrote:
> FPGA designs may have need of both MMIO-based and NATIVE-based dcr
> interfaces.
>
> Signed-off-by: Stephen Neuendorffer <stephen.neuendorffer@xilinx.com>
> ---
> arch/powerpc/platforms/40x/Kconfig | 2 ++
> 1 files changed, 2 insertions(+), 0 deletions(-)
>
> diff --git a/arch/powerpc/platforms/40x/Kconfig b/arch/powerpc/platforms/40x/Kconfig
> index a9260e2..b8e06df 100644
> --- a/arch/powerpc/platforms/40x/Kconfig
> +++ b/arch/powerpc/platforms/40x/Kconfig
> @@ -123,6 +123,8 @@ config 405GPR
>
> config XILINX_VIRTEX
> bool
> + select PPC_DCR_MMIO
> + select PPC_DCR_NATIVE
Yeah; we'll probably want to be more sophisticated about this in the
future; but for now...
Acked-by: Grant Likely <grant.likely@secretlab.ca>
(Paulus; but this does depend on patch 1 of this series)
>
> config XILINX_VIRTEX_II_PRO
> bool
> --
> 1.5.3.4-dirty
>
>
>
> _______________________________________________
> Linuxppc-dev mailing list
> Linuxppc-dev@ozlabs.org
> https://ozlabs.org/mailman/listinfo/linuxppc-dev
>
--
Grant Likely, B.Sc., P.Eng.
Secret Lab Technologies Ltd.
^ permalink raw reply [flat|nested] 45+ messages in thread