Nouveau - Jul 2009 - [RFC] patch 0/4: DRM MMIO accessor cleanup

Hi,

this is continuation for the MMIO accessor rewrite and cleanup.
I am currently running nv28 with these patches applied, but
I cannot test on PPC.

Please, review and comment. If the direction is good, I'll do the
same to INSTANCE_{RD,WR} as I did for nv_{rd,wr}32, and change
PRAMIN from drm_local_map to simple ioremap.

Can the same be done for channel specific mappings, that is
nvchan_{rd,wr}32?

The macros nv_{in,out}32 are very close to removal now.

It also looks like nouveau_load() is in need of serious error
path review for resource leakage. And maybe refactorization,
too, into small functions.

- pq

This removes the ugliness of using drm_local_map. The use of ioremap()
is identical to what drm_addmap()/drm_rmmap() did.

Is ioremap() the right thing or should we use e.g. ioremap_uc()?
The ioremap API has been reworked in the past, and some assumptions
changed in the mapping caching type.

Signed-off-by: Pekka Paalanen <pq at iki.fi>
---
 drivers/gpu/drm/nouveau/nouveau_drv.h   |   14 +++++++-------
 drivers/gpu/drm/nouveau/nouveau_state.c |   21 +++++++++++----------
 2 files changed, 18 insertions(+), 17 deletions(-)

diff --git a/drivers/gpu/drm/nouveau/nouveau_drv.h
b/drivers/gpu/drm/nouveau/nouveau_drv.h
index cba3b3b..90802dc 100644
--- a/drivers/gpu/drm/nouveau/nouveau_drv.h
+++ b/drivers/gpu/drm/nouveau/nouveau_drv.h
@@ -447,7 +447,7 @@ struct drm_nouveau_private {
 	int chipset;
 	int flags;
 
-	struct drm_local_map *mmio;
+	void __iomem *mmio;
 	struct drm_local_map *fb;
 	struct drm_local_map *ramin;
 
@@ -975,38 +975,38 @@ extern int nouveau_gem_ioctl_info(struct drm_device *,
void *,
 static inline u32 nv_rd32(struct drm_device *dev, unsigned reg)
 {
 	struct drm_nouveau_private *dev_priv = dev->dev_private;
-	return in_be32((void __force __iomem *)dev_priv->mmio->handle + reg);
+	return in_be32(dev_priv->mmio + reg);
 }
 
 static inline void nv_wr32(struct drm_device *dev, unsigned reg, u32 val)
 {
 	struct drm_nouveau_private *dev_priv = dev->dev_private;
-	out_be32((void __force __iomem *)dev_priv->mmio->handle + reg, val);
+	out_be32(dev_priv->mmio + reg, val);
 }
 #else
 static inline u32 nv_rd32(struct drm_device *dev, unsigned reg)
 {
 	struct drm_nouveau_private *dev_priv = dev->dev_private;
-	return readl((void __force __iomem *)dev_priv->mmio->handle + reg);
+	return readl(dev_priv->mmio + reg);
 }
 
 static inline void nv_wr32(struct drm_device *dev, unsigned reg, u32 val)
 {
 	struct drm_nouveau_private *dev_priv = dev->dev_private;
-	writel(val, (void __force __iomem *)dev_priv->mmio->handle + reg);
+	writel(val, dev_priv->mmio + reg);
 }
 #endif /* not __powerpc__ */
 
 static inline u8 nv_rd08(struct drm_device *dev, unsigned reg)
 {
 	struct drm_nouveau_private *dev_priv = dev->dev_private;
-	return readb((void __force __iomem *)dev_priv->mmio->handle + reg);
+	return readb(dev_priv->mmio + reg);
 }
 
 static inline void nv_wr08(struct drm_device *dev, unsigned reg, u8 val)
 {
 	struct drm_nouveau_private *dev_priv = dev->dev_private;
-	writeb(val, (void __force __iomem *)dev_priv->mmio->handle + reg);
+	writeb(val, dev_priv->mmio + reg);
 }
 
 #define nv_wait(reg,mask,val) nouveau_wait_until(dev, 2000000000ULL, (reg),   
\
diff --git a/drivers/gpu/drm/nouveau/nouveau_state.c
b/drivers/gpu/drm/nouveau/nouveau_state.c
index ae3d2d5..911dc7e 100644
--- a/drivers/gpu/drm/nouveau/nouveau_state.c
+++ b/drivers/gpu/drm/nouveau/nouveau_state.c
@@ -454,6 +454,7 @@ int nouveau_load(struct drm_device *dev, unsigned long
flags)
 #endif
 	uint32_t reg0;
 	uint8_t architecture = 0;
+	resource_size_t mmio_start_offs;
 	int ret;
 
 	dev_priv = kzalloc(sizeof(*dev_priv), GFP_KERNEL);
@@ -474,16 +475,15 @@ int nouveau_load(struct drm_device *dev, unsigned long
flags)
 	/* resource 6 is bios */
 
 	/* map the mmio regs */
-	ret = drm_addmap(dev, drm_get_resource_start(dev, 0),
-			 0x00800000, _DRM_REGISTERS, _DRM_READ_ONLY |
-			 _DRM_DRIVER, &dev_priv->mmio);
-	if (ret) {
-		NV_ERROR(dev, "Unable to initialize the mmio mapping (%d). "
-			      "Please report your setup to " DRIVER_EMAIL "\n",
-			 ret);
+	mmio_start_offs = pci_resource_start(dev->pdev, 0);
+	dev_priv->mmio = ioremap(mmio_start_offs, 0x00800000);
+	if (!dev_priv->mmio) {
+		NV_ERROR(dev, "Unable to initialize the mmio mapping. "
+			 "Please report your setup to " DRIVER_EMAIL "\n");
 		return -EINVAL;
 	}
-	NV_DEBUG(dev, "regs mapped ok at 0x%lx\n", (unsigned
long)dev_priv->mmio->offset);
+	NV_DEBUG(dev, "regs mapped ok at 0x%llx\n",
+					(unsigned long long)mmio_start_offs);
 
 #if defined(__powerpc__)
 	/* Put the card in BE mode if it's not */
@@ -538,7 +538,8 @@ int nouveau_load(struct drm_device *dev, unsigned long
flags)
 		dev_priv->card_type = NV_UNKNOWN;
 	}
 
-	NV_INFO(dev, "Detected an NV%d generation card (0x%08x)\n",
dev_priv->card_type,reg0);
+	NV_INFO(dev, "Detected an NV%d generation card (0x%08x)\n",
+						dev_priv->card_type, reg0);
 
 	if (dev_priv->card_type == NV_UNKNOWN) {
 		return -EINVAL;
@@ -653,7 +654,7 @@ int nouveau_unload(struct drm_device *dev)
 		nouveau_close(dev);
 	}
 
-	drm_rmmap(dev, dev_priv->mmio);
+	iounmap(dev_priv->mmio);
 	drm_rmmap(dev, dev_priv->ramin);
 	drm_rmmap(dev, dev_priv->fb);
 
-- 
1.6.3.3

Use the generic io{read,write}32be() functions on big-endian instead of the
PPC-specific private {in,out}_be32(), which are not meant to be used
with PCI devices.

While we are at it, change all #ifdef __powerpc__ that are really just
for endianness into #ifdef __BIG_ENDIAN, and clean up a bit of style.

On Wed, 22 Jul 2009 00:01:59 +0200
Arnd Bergmann <arnd at arndb.de> wrote:
> The powerpc in_le32 style functions are a completely different
> story, they are basically defined to operate only on on-chip
> components, while ioread32 and readl do work on PCI devices.
On Wed, 22 Jul 2009 23:20:58 +0200
Arnd Bergmann <arnd at arndb.de> wrote:
> If it's a PCI/AGP/PCIe device, use iowrite32be(), otherwise it
> may not work correctly on a pseries, celleb or qs20 machine.
>
> If it's connected over a different bus (IOIF on PS3), out_be32
> would be more appropriate, but AFAICT, iowrite32be should work
> just as well.
Signed-off-by: Pekka Paalanen <pq at iki.fi>
---
 drivers/gpu/drm/nouveau/nouveau_drv.h   |   39 +++++++++++++++++-------------
 drivers/gpu/drm/nouveau/nouveau_state.c |    2 +-
 2 files changed, 23 insertions(+), 18 deletions(-)

diff --git a/drivers/gpu/drm/nouveau/nouveau_drv.h
b/drivers/gpu/drm/nouveau/nouveau_drv.h
index 90802dc..7a37559 100644
--- a/drivers/gpu/drm/nouveau/nouveau_drv.h
+++ b/drivers/gpu/drm/nouveau/nouveau_drv.h
@@ -948,40 +948,45 @@ extern int nouveau_gem_ioctl_info(struct drm_device *,
void *,
 				  struct drm_file *);
 
 #define NVDEV ((struct drm_nouveau_private *)dev->dev_private)
-#if defined(__powerpc__)
-#define nv_out32(map,reg,val) out_be32((void __iomem *)NVDEV->map->handle
+ (reg), (val))
-#define nv_out16(map,reg,val) out_be16((void __iomem *)NVDEV->map->handle
+ (reg), (val))
-#define nv_in32(map,reg) in_be32((void __iomem *)NVDEV->map->handle +
(reg))
-#define nv_in16(map,reg) in_be16((void __iomem *)NVDEV->map->handle +
(reg))
+#ifdef __BIG_ENDIAN
+#define nv_out32(map, reg, val) \
+	iowrite32be((val), (void __iomem *)NVDEV->map->handle + (reg))
+#define nv_out16(map, reg, val) \
+	iowrite16be((val), (void __iomem *)NVDEV->map->handle + (reg))
+#define nv_in32(map, reg) \
+	ioread32be((void __iomem *)NVDEV->map->handle + (reg))
+#define nv_in16(map, reg) \
+	ioread16be((void __iomem *)NVDEV->map->handle + (reg))
 #else
-#define nv_out32(map,reg,val) DRM_WRITE32(NVDEV->map, (reg), (val))
-#define nv_out16(map,reg,val) DRM_WRITE16(NVDEV->map, (reg), (val))
-#define nv_in32(map,reg) DRM_READ32(NVDEV->map, (reg))
-#define nv_in16(map,reg) DRM_READ16(NVDEV->map, (reg))
+#define nv_out32(map, reg, val) DRM_WRITE32(NVDEV->map, (reg), (val))
+#define nv_out16(map, reg, val) DRM_WRITE16(NVDEV->map, (reg), (val))
+#define nv_in32(map, reg) DRM_READ32(NVDEV->map, (reg))
+#define nv_in16(map, reg) DRM_READ16(NVDEV->map, (reg))
 #endif
 
 /* channel control reg access */
-#if defined(__powerpc__)
-#define nvchan_wr32(reg,val) out_be32((void __iomem *)chan->user->handle
+ (reg), (val))
-#define nvchan_rd32(reg) in_be32((void __iomem *)chan->user->handle +
(reg))
+#ifdef __BIG_ENDIAN
+#define nvchan_wr32(reg, val) \
+	iowrite32be((val), (void __iomem *)chan->user->handle + (reg))
+#define nvchan_rd32(reg) ioread32be((void __iomem *)chan->user->handle +
(reg))
 #else
-#define nvchan_wr32(reg,val) DRM_WRITE32(chan->user, (reg), (val))
+#define nvchan_wr32(reg, val) DRM_WRITE32(chan->user, (reg), (val))
 #define nvchan_rd32(reg) DRM_READ32(chan->user, (reg))
 #endif
 
 
 /* register access */
-#if defined(__powerpc__)
+#ifdef __BIG_ENDIAN
 static inline u32 nv_rd32(struct drm_device *dev, unsigned reg)
 {
 	struct drm_nouveau_private *dev_priv = dev->dev_private;
-	return in_be32(dev_priv->mmio + reg);
+	return ioread32be(dev_priv->mmio + reg);
 }
 
 static inline void nv_wr32(struct drm_device *dev, unsigned reg, u32 val)
 {
 	struct drm_nouveau_private *dev_priv = dev->dev_private;
-	out_be32(dev_priv->mmio + reg, val);
+	iowrite32be(val, dev_priv->mmio + reg);
 }
 #else
 static inline u32 nv_rd32(struct drm_device *dev, unsigned reg)
@@ -995,7 +1000,7 @@ static inline void nv_wr32(struct drm_device *dev, unsigned
reg, u32 val)
 	struct drm_nouveau_private *dev_priv = dev->dev_private;
 	writel(val, dev_priv->mmio + reg);
 }
-#endif /* not __powerpc__ */
+#endif /* not __BIG_ENDIAN */
 
 static inline u8 nv_rd08(struct drm_device *dev, unsigned reg)
 {
diff --git a/drivers/gpu/drm/nouveau/nouveau_state.c
b/drivers/gpu/drm/nouveau/nouveau_state.c
index 911dc7e..b70ec33 100644
--- a/drivers/gpu/drm/nouveau/nouveau_state.c
+++ b/drivers/gpu/drm/nouveau/nouveau_state.c
@@ -485,7 +485,7 @@ int nouveau_load(struct drm_device *dev, unsigned long
flags)
 	NV_DEBUG(dev, "regs mapped ok at 0x%llx\n",
 					(unsigned long long)mmio_start_offs);
 
-#if defined(__powerpc__)
+#ifdef __BIG_ENDIAN
 	/* Put the card in BE mode if it's not */
 	if (nv_rd32(dev, NV03_PMC_BOOT_1))
 		nv_wr32(dev, NV03_PMC_BOOT_1, 0x00000001);
-- 
1.6.3.3