[PATCH] [V3] Xilinx : Framebuffer Driver: Add PLB support and cleanup DCR

[PATCH] [V3] Xilinx : Framebuffer Driver: Add PLB support and cleanup DCR

[John Linn]

Added support for the new xps tft controller. The new core
has PLB interface support in addition to existing DCR interface.

Removed platform device support as both MicroBlaze and PowerPC
use device tree.

Previously, the dcr interface was assumed to be used in mmio mode,
and the register space of the dcr interface was precomputed and stuffed
into the device tree. This driver now makes use of the new dcr
infrastructure to represent the dcr interface. This enables the dcr
interface to be connected directly to a native dcr interface in a clean
way.

Added compatibility for ml507 dvi core.

Signed-off-by: Suneel <suneelg@xilinx.com>
Signed-off-by: Stephen Neuendorffer <stephen.neuendorffer@xilinx.com>
Signed-off-by: John Linn <john.linn@xilinx.com>
---

V2 - cleanup based on review
V3 - update to be based on top of tree rather than Xilinx tree, sorry for the
 confusion with this, update the name of the patch slightly to be more accurate

 drivers/video/xilinxfb.c |  295 ++++++++++++++++++++++++----------------------
 1 files changed, 154 insertions(+), 141 deletions(-)

diff --git a/drivers/video/xilinxfb.c b/drivers/video/xilinxfb.c
index 40a3a2a..d151237 100644
--- a/drivers/video/xilinxfb.c
+++ b/drivers/video/xilinxfb.c
@@ -1,13 +1,13 @@
 /*
- * xilinxfb.c
  *
- * Xilinx TFT LCD frame buffer driver
+ * Xilinx TFT frame buffer driver
  *
  * Author: MontaVista Software, Inc.
  *         source@mvista.com
  *
  * 2002-2007 (c) MontaVista Software, Inc.
  * 2007 (c) Secret Lab Technologies, Ltd.
+ * 2009 (c) Xilinx Inc.
  *
  * This file is licensed under the terms of the GNU General Public License
  * version 2.  This program is licensed "as is" without any warranty of any
@@ -24,33 +24,38 @@
 #include <linux/device.h>
 #include <linux/module.h>
 #include <linux/kernel.h>
+#include <linux/version.h>
 #include <linux/errno.h>
 #include <linux/string.h>
 #include <linux/mm.h>
 #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>
-#endif
-#include <asm/io.h>
+#include <linux/io.h>
 #include <linux/xilinxfb.h>
+#include <asm/dcr.h>
 
 #define DRIVER_NAME		"xilinxfb"
-#define DRIVER_DESCRIPTION	"Xilinx TFT LCD frame buffer driver"
+
 
 /*
  * Xilinx calls it "PLB TFT LCD Controller" though it can also be used for
- * the VGA port on the Xilinx ML40x board. This is a hardware display controller
- * for a 640x480 resolution TFT or VGA screen.
+ * the VGA port on the Xilinx ML40x board. This is a hardware display
+ * controller for a 640x480 resolution TFT or VGA screen.
  *
  * The interface to the framebuffer is nice and simple.  There are two
  * control registers.  The first tells the LCD interface where in memory
  * the frame buffer is (only the 11 most significant bits are used, so
  * don't start thinking about scrolling).  The second allows the LCD to
  * be turned on or off as well as rotated 180 degrees.
+ *
+ * In case of direct PLB access the second control register will be at
+ * an offset of 4 as compared to the DCR access where the offset is 1
+ * i.e. REG_CTRL. So this is taken care in the function
+ * xilinx_fb_out_be32 where it left shifts the offset 2 times in case of
+ * direct PLB access.
  */
 #define NUM_REGS	2
 #define REG_FB_ADDR	0
@@ -107,17 +112,28 @@ static struct fb_var_screeninfo xilinx_fb_var = {
 	.activate =	FB_ACTIVATE_NOW
 };
 
+
+#define PLB_ACCESS_FLAG	0x1		/* 1 = PLB, 0 = DCR */
+
 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 */
+	phys_addr_t	regs_phys;	/* phys. address of the control
+						registers */
+	void __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 */
 	int		fb_alloced;	/* Flag, was the fb memory alloced? */
 
+	u8 		flags;		/* features of the driver */
+
 	u32		reg_ctrl_default;
 
 	u32		pseudo_palette[PALETTE_ENTRIES_NO];
@@ -128,14 +144,19 @@ struct xilinxfb_drvdata {
 	container_of(_info, struct xilinxfb_drvdata, info)
 
 /*
- * The LCD controller has DCR interface to its registers, but all
- * the boards and configurations the driver has been tested with
- * use opb2dcr bridge. So the registers are seen as memory mapped.
- * This macro is to make it simple to add the direct DCR access
- * when it's needed.
+ * The XPS TFT Controller can be accessed through PLB or DCR interface.
+ * To perform the read/write on the registers we need to check on
+ * which bus its connected and call the appropriate write API.
  */
-#define xilinx_fb_out_be32(driverdata, offset, val) \
-	out_be32(driverdata->regs + offset, val)
+static void xilinx_fb_out_be32(struct xilinxfb_drvdata *drvdata, u32 offset,
+				u32 val)
+{
+	if (drvdata->flags & PLB_ACCESS_FLAG)
+		out_be32(drvdata->regs + (offset << 2), val);
+	else
+		dcr_write(drvdata->dcr_host, offset, val);
+
+}
 
 static int
 xilinx_fb_setcolreg(unsigned regno, unsigned red, unsigned green, unsigned blue,
@@ -173,7 +194,8 @@ xilinx_fb_blank(int blank_mode, struct fb_info *fbi)
 	switch (blank_mode) {
 	case FB_BLANK_UNBLANK:
 		/* turn on panel */
-		xilinx_fb_out_be32(drvdata, REG_CTRL, drvdata->reg_ctrl_default);
+		xilinx_fb_out_be32(drvdata, REG_CTRL,
+					drvdata->reg_ctrl_default);
 		break;
 
 	case FB_BLANK_NORMAL:
@@ -189,8 +211,7 @@ xilinx_fb_blank(int blank_mode, struct fb_info *fbi)
 	return 0; /* success */
 }
 
-static struct fb_ops xilinxfb_ops =
-{
+static struct fb_ops xilinxfb_ops = {
 	.owner			= THIS_MODULE,
 	.fb_setcolreg		= xilinx_fb_setcolreg,
 	.fb_blank		= xilinx_fb_blank,
@@ -203,35 +224,34 @@ 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,
+			   struct xilinxfb_drvdata *drvdata,
+			   unsigned long physaddr,
 			   struct xilinxfb_platform_data *pdata)
 {
-	struct xilinxfb_drvdata *drvdata;
 	int rc;
 	int fbsize = pdata->xvirt * pdata->yvirt * BYTES_PER_PIXEL;
 
-	/* Allocate the driver data region */
-	drvdata = kzalloc(sizeof(*drvdata), GFP_KERNEL);
-	if (!drvdata) {
-		dev_err(dev, "Couldn't allocate device private record\n");
-		return -ENOMEM;
-	}
-	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;
+	if (drvdata->flags & PLB_ACCESS_FLAG) {
+		/*
+		 * Map the control registers in if the controller
+		 * is on direct PLB interface.
+		 */
+		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;
+		}
 	}
 
 	/* Allocate the framebuffer memory */
@@ -247,7 +267,10 @@ 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;
+		if (drvdata->flags & PLB_ACCESS_FLAG)
+			goto err_fbmem;
+		else
+			goto err_region;
 	}
 
 	/* Clear (turn to black) the framebuffer */
@@ -260,7 +283,8 @@ static int xilinxfb_assign(struct device *dev, unsigned long physaddr,
 	drvdata->reg_ctrl_default = REG_CTRL_ENABLE;
 	if (pdata->rotate_screen)
 		drvdata->reg_ctrl_default |= REG_CTRL_ROTATE;
-	xilinx_fb_out_be32(drvdata, REG_CTRL, drvdata->reg_ctrl_default);
+	xilinx_fb_out_be32(drvdata, REG_CTRL,
+					drvdata->reg_ctrl_default);
 
 	/* Fill struct fb_info */
 	drvdata->info.device = dev;
@@ -296,11 +320,14 @@ static int xilinxfb_assign(struct device *dev, unsigned long physaddr,
 		goto err_regfb;
 	}
 
+	if (drvdata->flags & PLB_ACCESS_FLAG) {
+		/* Put a banner in the log (for DEBUG) */
+		dev_dbg(dev, "regs: phys=%lx, virt=%p\n", physaddr,
+					drvdata->regs);
+	}
 	/* 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=%llx, virt=%p, size=%x\n",
-		(unsigned long long) drvdata->fb_phys, drvdata->fb_virt,
-		fbsize);
+	dev_dbg(dev, "fb: phys=%p, virt=%p, size=%x\n",
+		(void *)drvdata->fb_phys, drvdata->fb_virt, fbsize);
 
 	return 0;	/* success */
 
@@ -311,14 +338,19 @@ err_cmap:
 	if (drvdata->fb_alloced)
 		dma_free_coherent(dev, PAGE_ALIGN(fbsize), drvdata->fb_virt,
 			drvdata->fb_phys);
+	else
+		iounmap(drvdata->fb_virt);
+
 	/* Turn off the display */
 	xilinx_fb_out_be32(drvdata, REG_CTRL, 0);
 
 err_fbmem:
-	iounmap(drvdata->regs);
+	if (drvdata->flags & PLB_ACCESS_FLAG)
+		iounmap(drvdata->regs);
 
 err_map:
-	release_mem_region(physaddr, 8);
+	if (drvdata->flags & PLB_ACCESS_FLAG)
+		release_mem_region(physaddr, 8);
 
 err_region:
 	kfree(drvdata);
@@ -342,12 +374,18 @@ static int xilinxfb_release(struct device *dev)
 	if (drvdata->fb_alloced)
 		dma_free_coherent(dev, PAGE_ALIGN(drvdata->info.fix.smem_len),
 				  drvdata->fb_virt, drvdata->fb_phys);
+	else
+		iounmap(drvdata->fb_virt);
 
 	/* Turn off the display */
 	xilinx_fb_out_be32(drvdata, REG_CTRL, 0);
-	iounmap(drvdata->regs);
 
-	release_mem_region(drvdata->regs_phys, 8);
+	/* Release the resources, as allocated based on interface */
+	if (drvdata->flags & PLB_ACCESS_FLAG) {
+		iounmap(drvdata->regs);
+		release_mem_region(drvdata->regs_phys, 8);
+	} else
+		dcr_unmap(drvdata->dcr_host, drvdata->dcr_len);
 
 	kfree(drvdata);
 	dev_set_drvdata(dev, NULL);
@@ -356,77 +394,57 @@ 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
  */
 
-#if defined(CONFIG_OF)
 static int __devinit
 xilinxfb_of_probe(struct of_device *op, const struct of_device_id *match)
 {
-	struct resource res;
 	const u32 *prop;
+	u32 *p;
+	u32 tft_access;
 	struct xilinxfb_platform_data pdata;
+	struct resource res;
 	int size, rc;
+	int start = 0, len = 0;
+	dcr_host_t dcr_host;
+	struct xilinxfb_drvdata *drvdata;
 
 	/* 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) {
-		dev_err(&op->dev, "invalid address\n");
-		return rc;
+	/*
+	 * To check whether the core is connected directly to DCR or PLB
+	 * interface and initialize the tft_access accordingly.
+	 */
+	p = (u32 *)of_get_property(op->node, "xlnx,dcr-splb-slave-if", NULL);
+
+	if (p)
+		tft_access = *p;
+	else
+		tft_access = 0;		/* For backward compatibility */
+
+	/*
+	 * Fill the resource structure if its direct PLB interface
+	 * otherwise fill the dcr_host structure.
+	 */
+	if (tft_access) {
+		rc = of_address_to_resource(op->node, 0, &res);
+		if (rc) {
+			dev_err(&op->dev, "invalid address\n");
+			return -ENODEV;
+		}
+
+	} else {
+		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 -ENODEV;
+		}
 	}
 
 	prop = of_get_property(op->node, "phys-size", &size);
@@ -450,7 +468,26 @@ 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);
+	/* Allocate the driver data region */
+	drvdata = kzalloc(sizeof(*drvdata), GFP_KERNEL);
+	if (!drvdata) {
+		dev_err(&op->dev, "Couldn't allocate device private record\n");
+		return -ENOMEM;
+	}
+	dev_set_drvdata(&op->dev, drvdata);
+
+	if (tft_access)
+		drvdata->flags |= PLB_ACCESS_FLAG;
+
+	/* Arguments are passed based on the interface */
+	if (drvdata->flags & PLB_ACCESS_FLAG) {
+		return xilinxfb_assign(&op->dev, drvdata, res.start, &pdata);
+	} else {
+		drvdata->dcr_start = start;
+		drvdata->dcr_len = len;
+		drvdata->dcr_host = dcr_host;
+		return xilinxfb_assign(&op->dev, drvdata, 0, &pdata);
+	}
 }
 
 static int __devexit xilinxfb_of_remove(struct of_device *op)
@@ -460,7 +497,9 @@ static int __devexit xilinxfb_of_remove(struct of_device *op)
 
 /* Match table for of_platform binding */
 static struct of_device_id xilinxfb_of_match[] __devinitdata = {
+	{ .compatible = "xlnx,xps-tft-1.00.a", },
 	{ .compatible = "xlnx,plb-tft-cntlr-ref-1.00.a", },
+	{ .compatible = "xlnx,plb-dvi-cntlr-ref-1.00.c", },
 	{},
 };
 MODULE_DEVICE_TABLE(of, xilinxfb_of_match);
@@ -476,22 +515,6 @@ static struct of_platform_driver xilinxfb_of_driver = {
 	},
 };
 
-/* Registration helpers to keep the number of #ifdefs to a minimum */
-static inline int __init xilinxfb_of_register(void)
-{
-	pr_debug("xilinxfb: calling of_register_platform_driver()\n");
-	return of_register_platform_driver(&xilinxfb_of_driver);
-}
-
-static inline void __exit xilinxfb_of_unregister(void)
-{
-	of_unregister_platform_driver(&xilinxfb_of_driver);
-}
-#else /* CONFIG_OF */
-/* CONFIG_OF not enabled; do nothing helpers */
-static inline int __init xilinxfb_of_register(void) { return 0; }
-static inline void __exit xilinxfb_of_unregister(void) { }
-#endif /* CONFIG_OF */
 
 /* ---------------------------------------------------------------------
  * Module setup and teardown
@@ -500,28 +523,18 @@ 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 of_register_platform_driver(&xilinxfb_of_driver);
 }
 
 static void __exit
 xilinxfb_cleanup(void)
 {
-	platform_driver_unregister(&xilinxfb_platform_driver);
-	xilinxfb_of_unregister();
+	of_unregister_platform_driver(&xilinxfb_of_driver);
 }
 
 module_init(xilinxfb_init);
 module_exit(xilinxfb_cleanup);
 
 MODULE_AUTHOR("MontaVista Software, Inc. <source@mvista.com>");
-MODULE_DESCRIPTION(DRIVER_DESCRIPTION);
+MODULE_DESCRIPTION("Xilinx TFT frame buffer driver");
 MODULE_LICENSE("GPL");
-- 
1.6.2.1



This email and any attachments are intended for the sole use of the named recipient(s) and contain(s) confidential information that may be proprietary, privileged or copyrighted under applicable law. If you are not the intended recipient, do not read, copy, or forward this email message or any attachments. Delete this email message and any attachments immediately.

Re: [PATCH] [V3] Xilinx : Framebuffer Driver: Add PLB support and cleanup DCR

[Grant Likely]

On Tue, Apr 14, 2009 at 2:08 PM, John Linn <john.linn@xilinx.com> wrote:
> Added support for the new xps tft controller. The new core
> has PLB interface support in addition to existing DCR interface.

Good looking patch.  A few more comments below.

g.

> =A0/*
> =A0* Xilinx calls it "PLB TFT LCD Controller" though it can also be used =
for
> - * the VGA port on the Xilinx ML40x board. This is a hardware display co=
ntroller
> - * for a 640x480 resolution TFT or VGA screen.
> + * the VGA port on the Xilinx ML40x board. This is a hardware display
> + * controller for a 640x480 resolution TFT or VGA screen.

This isn't necessarily true.  This driver will handle controllers
using different resolutions (I know, I know, this is just cleaning up
an existing comment, but no need to leave inaccuracies in there)  :-)

> -#define xilinx_fb_out_be32(driverdata, offset, val) \
> - =A0 =A0 =A0 out_be32(driverdata->regs + offset, val)
> +static void xilinx_fb_out_be32(struct xilinxfb_drvdata *drvdata, u32 off=
set,
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 u32 val)
> +{
> + =A0 =A0 =A0 if (drvdata->flags & PLB_ACCESS_FLAG)
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 out_be32(drvdata->regs + (offset << 2), val=
);
> + =A0 =A0 =A0 else
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 dcr_write(drvdata->dcr_host, offset, val);
> +

This could more simply be:

        if (drvdata->regs)
                out_be32(drvdata->regs + (offset << 2), val);
        else
                dcr_write(drvdata->dcr_host, offset, val);

regs will be NULL if DCR is active.

> -static int xilinxfb_assign(struct device *dev, unsigned long physaddr,
> +static int xilinxfb_assign(struct device *dev,
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0struct xilinxfb_drvd=
ata *drvdata,
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0unsigned long physad=
dr,
> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 struct xilinxfb_platf=
orm_data *pdata)
> =A0{
> - =A0 =A0 =A0 struct xilinxfb_drvdata *drvdata;
> =A0 =A0 =A0 =A0int rc;
> =A0 =A0 =A0 =A0int fbsize =3D pdata->xvirt * pdata->yvirt * BYTES_PER_PIX=
EL;
>
> - =A0 =A0 =A0 /* Allocate the driver data region */
> - =A0 =A0 =A0 drvdata =3D kzalloc(sizeof(*drvdata), GFP_KERNEL);
> - =A0 =A0 =A0 if (!drvdata) {
> - =A0 =A0 =A0 =A0 =A0 =A0 =A0 dev_err(dev, "Couldn't allocate device priv=
ate record\n");
> - =A0 =A0 =A0 =A0 =A0 =A0 =A0 return -ENOMEM;
> - =A0 =A0 =A0 }
> - =A0 =A0 =A0 dev_set_drvdata(dev, drvdata);
> -
> - =A0 =A0 =A0 /* Map the control registers in */
> - =A0 =A0 =A0 if (!request_mem_region(physaddr, 8, DRIVER_NAME)) {
> - =A0 =A0 =A0 =A0 =A0 =A0 =A0 dev_err(dev, "Couldn't lock memory region a=
t 0x%08lX\n",
> - =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 physaddr);
> - =A0 =A0 =A0 =A0 =A0 =A0 =A0 rc =3D -ENODEV;
> - =A0 =A0 =A0 =A0 =A0 =A0 =A0 goto err_region;
> - =A0 =A0 =A0 }
> - =A0 =A0 =A0 drvdata->regs_phys =3D physaddr;
> - =A0 =A0 =A0 drvdata->regs =3D ioremap(physaddr, 8);
> - =A0 =A0 =A0 if (!drvdata->regs) {
> - =A0 =A0 =A0 =A0 =A0 =A0 =A0 dev_err(dev, "Couldn't lock memory region a=
t 0x%08lX\n",
> - =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 physaddr);
> - =A0 =A0 =A0 =A0 =A0 =A0 =A0 rc =3D -ENODEV;
> - =A0 =A0 =A0 =A0 =A0 =A0 =A0 goto err_map;
> + =A0 =A0 =A0 if (drvdata->flags & PLB_ACCESS_FLAG) {

Again, I think using "if (physaddr) {" would be simpler and make more
sense and would eliminate even needing a flags data member.

> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 /*
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0* Map the control registers in if the co=
ntroller
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0* is on direct PLB interface.
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0*/
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 if (!request_mem_region(physaddr, 8, DRIVER=
_NAME)) {
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 dev_err(dev, "Couldn't lock=
 memory region at 0x%08lX\n",
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 physaddr);
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 rc =3D -ENODEV;
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 goto err_region;
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 }
> +
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 drvdata->regs_phys =3D physaddr;
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 drvdata->regs =3D ioremap(physaddr, 8);
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 if (!drvdata->regs) {
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 dev_err(dev, "Couldn't lock=
 memory region at 0x%08lX\n",
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 physaddr);
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 rc =3D -ENODEV;
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 goto err_map;
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 }
> =A0 =A0 =A0 =A0}
>
> =A0 =A0 =A0 =A0/* Allocate the framebuffer memory */
> @@ -247,7 +267,10 @@ static int xilinxfb_assign(struct device *dev, unsig=
ned long physaddr,
> =A0 =A0 =A0 =A0if (!drvdata->fb_virt) {
> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0dev_err(dev, "Could not allocate frame buf=
fer memory\n");
> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0rc =3D -ENOMEM;
> - =A0 =A0 =A0 =A0 =A0 =A0 =A0 goto err_fbmem;
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 if (drvdata->flags & PLB_ACCESS_FLAG)
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 goto err_fbmem;
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 else
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 goto err_region;

This change doesn't make much sense because the same condition is
tested for again at the err_fbmem: label.  To avoid confusion later
on, keep the unwind path simple (don't jump to different labels).

> =A0 =A0 =A0 =A0}
>
> =A0 =A0 =A0 =A0/* Clear (turn to black) the framebuffer */
> @@ -260,7 +283,8 @@ static int xilinxfb_assign(struct device *dev, unsign=
ed long physaddr,
> =A0 =A0 =A0 =A0drvdata->reg_ctrl_default =3D REG_CTRL_ENABLE;
> =A0 =A0 =A0 =A0if (pdata->rotate_screen)
> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0drvdata->reg_ctrl_default |=3D REG_CTRL_RO=
TATE;
> - =A0 =A0 =A0 xilinx_fb_out_be32(drvdata, REG_CTRL, drvdata->reg_ctrl_def=
ault);
> + =A0 =A0 =A0 xilinx_fb_out_be32(drvdata, REG_CTRL,
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0=
 =A0 drvdata->reg_ctrl_default);
>
> =A0 =A0 =A0 =A0/* Fill struct fb_info */
> =A0 =A0 =A0 =A0drvdata->info.device =3D dev;
> @@ -296,11 +320,14 @@ static int xilinxfb_assign(struct device *dev, unsi=
gned long physaddr,
> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0goto err_regfb;
> =A0 =A0 =A0 =A0}
>
> + =A0 =A0 =A0 if (drvdata->flags & PLB_ACCESS_FLAG) {

if (drvdata->regs) {

> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 /* Put a banner in the log (for DEBUG) */
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 dev_dbg(dev, "regs: phys=3D%lx, virt=3D%p\n=
", physaddr,
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0=
 =A0 drvdata->regs);
> + =A0 =A0 =A0 }
> =A0 =A0 =A0 =A0/* Put a banner in the log (for DEBUG) */
> - =A0 =A0 =A0 dev_dbg(dev, "regs: phys=3D%lx, virt=3D%p\n", physaddr, drv=
data->regs);
> - =A0 =A0 =A0 dev_dbg(dev, "fb: phys=3D%llx, virt=3D%p, size=3D%x\n",
> - =A0 =A0 =A0 =A0 =A0 =A0 =A0 (unsigned long long) drvdata->fb_phys, drvd=
ata->fb_virt,
> - =A0 =A0 =A0 =A0 =A0 =A0 =A0 fbsize);
> + =A0 =A0 =A0 dev_dbg(dev, "fb: phys=3D%p, virt=3D%p, size=3D%x\n",
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 (void *)drvdata->fb_phys, drvdata->fb_virt,=
 fbsize);
>
> =A0 =A0 =A0 =A0return 0; =A0 =A0 =A0 /* success */
>
> @@ -311,14 +338,19 @@ err_cmap:
> =A0 =A0 =A0 =A0if (drvdata->fb_alloced)
> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0dma_free_coherent(dev, PAGE_ALIGN(fbsize),=
 drvdata->fb_virt,
> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0drvdata->fb_phys);
> + =A0 =A0 =A0 else
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 iounmap(drvdata->fb_virt);
> +
> =A0 =A0 =A0 =A0/* Turn off the display */
> =A0 =A0 =A0 =A0xilinx_fb_out_be32(drvdata, REG_CTRL, 0);
>
> =A0err_fbmem:
> - =A0 =A0 =A0 iounmap(drvdata->regs);
> + =A0 =A0 =A0 if (drvdata->flags & PLB_ACCESS_FLAG)
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 iounmap(drvdata->regs);

if (drvdata->regs)

>
> =A0err_map:
> - =A0 =A0 =A0 release_mem_region(physaddr, 8);
> + =A0 =A0 =A0 if (drvdata->flags & PLB_ACCESS_FLAG)
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 release_mem_region(physaddr, 8);

if (physaddr)

>
> =A0err_region:
> =A0 =A0 =A0 =A0kfree(drvdata);
> @@ -342,12 +374,18 @@ static int xilinxfb_release(struct device *dev)
> =A0 =A0 =A0 =A0if (drvdata->fb_alloced)
> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0dma_free_coherent(dev, PAGE_ALIGN(drvdata-=
>info.fix.smem_len),
> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0drvdat=
a->fb_virt, drvdata->fb_phys);
> + =A0 =A0 =A0 else
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 iounmap(drvdata->fb_virt);
>
> =A0 =A0 =A0 =A0/* Turn off the display */
> =A0 =A0 =A0 =A0xilinx_fb_out_be32(drvdata, REG_CTRL, 0);
> - =A0 =A0 =A0 iounmap(drvdata->regs);
>
> - =A0 =A0 =A0 release_mem_region(drvdata->regs_phys, 8);
> + =A0 =A0 =A0 /* Release the resources, as allocated based on interface *=
/
> + =A0 =A0 =A0 if (drvdata->flags & PLB_ACCESS_FLAG) {
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 iounmap(drvdata->regs);
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 release_mem_region(drvdata->regs_phys, 8);
> + =A0 =A0 =A0 } else
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 dcr_unmap(drvdata->dcr_host, drvdata->dcr_l=
en);

et cetera

> -#if defined(CONFIG_OF)
> =A0static int __devinit
> =A0xilinxfb_of_probe(struct of_device *op, const struct of_device_id *mat=
ch)
> =A0{
> - =A0 =A0 =A0 struct resource res;
> =A0 =A0 =A0 =A0const u32 *prop;
> + =A0 =A0 =A0 u32 *p;
> + =A0 =A0 =A0 u32 tft_access;
> =A0 =A0 =A0 =A0struct xilinxfb_platform_data pdata;
> + =A0 =A0 =A0 struct resource res;
> =A0 =A0 =A0 =A0int size, rc;
> + =A0 =A0 =A0 int start =3D 0, len =3D 0;
> + =A0 =A0 =A0 dcr_host_t dcr_host;
> + =A0 =A0 =A0 struct xilinxfb_drvdata *drvdata;
>
> =A0 =A0 =A0 =A0/* Copy with the default pdata (not a ptr reference!) */
> =A0 =A0 =A0 =A0pdata =3D xilinx_fb_default_pdata;
>
> =A0 =A0 =A0 =A0dev_dbg(&op->dev, "xilinxfb_of_probe(%p, %p)\n", op, match=
);
>
> - =A0 =A0 =A0 rc =3D of_address_to_resource(op->node, 0, &res);
> - =A0 =A0 =A0 if (rc) {
> - =A0 =A0 =A0 =A0 =A0 =A0 =A0 dev_err(&op->dev, "invalid address\n");
> - =A0 =A0 =A0 =A0 =A0 =A0 =A0 return rc;
> + =A0 =A0 =A0 /*
> + =A0 =A0 =A0 =A0* To check whether the core is connected directly to DCR=
 or PLB
> + =A0 =A0 =A0 =A0* interface and initialize the tft_access accordingly.
> + =A0 =A0 =A0 =A0*/
> + =A0 =A0 =A0 p =3D (u32 *)of_get_property(op->node, "xlnx,dcr-splb-slave=
-if", NULL);

Hmmm.  This binding is undocumented.  It would be better to make the
decision on the presence/absence of the dcr-reg and/or reg properties.

> +
> + =A0 =A0 =A0 if (p)
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 tft_access =3D *p;
> + =A0 =A0 =A0 else
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 tft_access =3D 0; =A0 =A0 =A0 =A0 /* For ba=
ckward compatibility */

Common pattern: tft_access =3D p ? *p : 0;

> +
> + =A0 =A0 =A0 /*
> + =A0 =A0 =A0 =A0* Fill the resource structure if its direct PLB interfac=
e
> + =A0 =A0 =A0 =A0* otherwise fill the dcr_host structure.
> + =A0 =A0 =A0 =A0*/
> + =A0 =A0 =A0 if (tft_access) {
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 rc =3D of_address_to_resource(op->node, 0, =
&res);
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 if (rc) {
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 dev_err(&op->dev, "invalid =
address\n");
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 return -ENODEV;
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 }
> +
> + =A0 =A0 =A0 } else {
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 start =3D dcr_resource_start(op->node, 0);
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 len =3D dcr_resource_len(op->node, 0);
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 dcr_host =3D dcr_map(op->node, start, len);
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 if (!DCR_MAP_OK(dcr_host)) {
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 dev_err(&op->dev, "invalid =
address\n");
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 return -ENODEV;
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 }
> =A0 =A0 =A0 =A0}
>
> =A0 =A0 =A0 =A0prop =3D of_get_property(op->node, "phys-size", &size);
> @@ -450,7 +468,26 @@ xilinxfb_of_probe(struct of_device *op, const struct=
 of_device_id *match)
> =A0 =A0 =A0 =A0if (of_find_property(op->node, "rotate-display", NULL))
> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0pdata.rotate_screen =3D 1;
>
> - =A0 =A0 =A0 return xilinxfb_assign(&op->dev, res.start, &pdata);
> + =A0 =A0 =A0 /* Allocate the driver data region */
> + =A0 =A0 =A0 drvdata =3D kzalloc(sizeof(*drvdata), GFP_KERNEL);
> + =A0 =A0 =A0 if (!drvdata) {
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 dev_err(&op->dev, "Couldn't allocate device=
 private record\n");
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 return -ENOMEM;
> + =A0 =A0 =A0 }
> + =A0 =A0 =A0 dev_set_drvdata(&op->dev, drvdata);
> +
> + =A0 =A0 =A0 if (tft_access)
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 drvdata->flags |=3D PLB_ACCESS_FLAG;
> +
> + =A0 =A0 =A0 /* Arguments are passed based on the interface */
> + =A0 =A0 =A0 if (drvdata->flags & PLB_ACCESS_FLAG) {
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 return xilinxfb_assign(&op->dev, drvdata, r=
es.start, &pdata);
> + =A0 =A0 =A0 } else {
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 drvdata->dcr_start =3D start;
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 drvdata->dcr_len =3D len;
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 drvdata->dcr_host =3D dcr_host;
> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 return xilinxfb_assign(&op->dev, drvdata, 0=
, &pdata);
> + =A0 =A0 =A0 }
> =A0}
>
> =A0static int __devexit xilinxfb_of_remove(struct of_device *op)
> @@ -460,7 +497,9 @@ static int __devexit xilinxfb_of_remove(struct of_dev=
ice *op)
>
> =A0/* Match table for of_platform binding */
> =A0static struct of_device_id xilinxfb_of_match[] __devinitdata =3D {
> + =A0 =A0 =A0 { .compatible =3D "xlnx,xps-tft-1.00.a", },
> =A0 =A0 =A0 =A0{ .compatible =3D "xlnx,plb-tft-cntlr-ref-1.00.a", },
> + =A0 =A0 =A0 { .compatible =3D "xlnx,plb-dvi-cntlr-ref-1.00.c", },
> =A0 =A0 =A0 =A0{},
> =A0};
> =A0MODULE_DEVICE_TABLE(of, xilinxfb_of_match);
> @@ -476,22 +515,6 @@ static struct of_platform_driver xilinxfb_of_driver =
=3D {
> =A0 =A0 =A0 =A0},
> =A0};
>
> -/* Registration helpers to keep the number of #ifdefs to a minimum */
> -static inline int __init xilinxfb_of_register(void)
> -{
> - =A0 =A0 =A0 pr_debug("xilinxfb: calling of_register_platform_driver()\n=
");
> - =A0 =A0 =A0 return of_register_platform_driver(&xilinxfb_of_driver);
> -}
> -
> -static inline void __exit xilinxfb_of_unregister(void)
> -{
> - =A0 =A0 =A0 of_unregister_platform_driver(&xilinxfb_of_driver);
> -}
> -#else /* CONFIG_OF */
> -/* CONFIG_OF not enabled; do nothing helpers */
> -static inline int __init xilinxfb_of_register(void) { return 0; }
> -static inline void __exit xilinxfb_of_unregister(void) { }
> -#endif /* CONFIG_OF */
>
> =A0/* -------------------------------------------------------------------=
--
> =A0* Module setup and teardown
> @@ -500,28 +523,18 @@ static inline void __exit xilinxfb_of_unregister(vo=
id) { }
> =A0static int __init
> =A0xilinxfb_init(void)
> =A0{
> - =A0 =A0 =A0 int rc;
> - =A0 =A0 =A0 rc =3D xilinxfb_of_register();
> - =A0 =A0 =A0 if (rc)
> - =A0 =A0 =A0 =A0 =A0 =A0 =A0 return rc;
> -
> - =A0 =A0 =A0 rc =3D platform_driver_register(&xilinxfb_platform_driver);
> - =A0 =A0 =A0 if (rc)
> - =A0 =A0 =A0 =A0 =A0 =A0 =A0 xilinxfb_of_unregister();
> -
> - =A0 =A0 =A0 return rc;
> + =A0 =A0 =A0 return of_register_platform_driver(&xilinxfb_of_driver);
> =A0}
>
> =A0static void __exit
> =A0xilinxfb_cleanup(void)
> =A0{
> - =A0 =A0 =A0 platform_driver_unregister(&xilinxfb_platform_driver);
> - =A0 =A0 =A0 xilinxfb_of_unregister();
> + =A0 =A0 =A0 of_unregister_platform_driver(&xilinxfb_of_driver);
> =A0}
>
> =A0module_init(xilinxfb_init);
> =A0module_exit(xilinxfb_cleanup);
>
> =A0MODULE_AUTHOR("MontaVista Software, Inc. <source@mvista.com>");
> -MODULE_DESCRIPTION(DRIVER_DESCRIPTION);
> +MODULE_DESCRIPTION("Xilinx TFT frame buffer driver");
> =A0MODULE_LICENSE("GPL");
> --
> 1.6.2.1
>
>
>
> This email and any attachments are intended for the sole use of the named=
 recipient(s) and contain(s) confidential information that may be proprieta=
ry, privileged or copyrighted under applicable law. If you are not the inte=
nded recipient, do not read, copy, or forward this email message or any att=
achments. Delete this email message and any attachments immediately.
>
>
>



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

RE: [PATCH] [V3] Xilinx : Framebuffer Driver: Add PLB support and cleanup DCR

[Stephen Neuendorffer]

> > - =A0 =A0 =A0 rc =3D of_address_to_resource(op->node, 0, &res);
> > - =A0 =A0 =A0 if (rc) {
> > - =A0 =A0 =A0 =A0 =A0 =A0 =A0 dev_err(&op->dev, "invalid address\n");
> > - =A0 =A0 =A0 =A0 =A0 =A0 =A0 return rc;
> > + =A0 =A0 =A0 /*
> > + =A0 =A0 =A0 =A0* To check whether the core is connected directly to D=
CR or PLB
> > + =A0 =A0 =A0 =A0* interface and initialize the tft_access accordingly.=

> > + =A0 =A0 =A0 =A0*/
> > + =A0 =A0 =A0 p =3D (u32 *)of_get_property(op->node, "xlnx,dcr-splb-sla=
ve-if", NULL);
> =

> Hmmm.  This binding is undocumented.  It would be better to make the
> decision on the presence/absence of the dcr-reg and/or reg properties.

For backward compatibility with the 'old' way, the device tree generator fo=
r this core has both dcr-reg and reg properties (where the reg has been tra=
nslated back through the bridge).   =


You're probably right that it should just be one or the other, but the devi=
ce tree generator change needs to be synchronized, and it will break backwa=
rd compatibility with the older driver.

Steve =


This email and any attachments are intended for the sole use of the named r=
ecipient(s) and contain(s) confidential information that may be proprietary=
, privileged or copyrighted under applicable law. If you are not the intend=
ed recipient, do not read, copy, or forward this email message or any attac=
hments. Delete this email message and any attachments immediately.

Re: [PATCH] [V3] Xilinx : Framebuffer Driver: Add PLB support and cleanup DCR

[Grant Likely]

On Wed, Apr 15, 2009 at 9:58 AM, Stephen Neuendorffer
<stephen.neuendorffer@xilinx.com> wrote:
>> > - =A0 =A0 =A0 rc =3D of_address_to_resource(op->node, 0, &res);
>> > - =A0 =A0 =A0 if (rc) {
>> > - =A0 =A0 =A0 =A0 =A0 =A0 =A0 dev_err(&op->dev, "invalid address\n");
>> > - =A0 =A0 =A0 =A0 =A0 =A0 =A0 return rc;
>> > + =A0 =A0 =A0 /*
>> > + =A0 =A0 =A0 =A0* To check whether the core is connected directly to =
DCR or PLB
>> > + =A0 =A0 =A0 =A0* interface and initialize the tft_access accordingly=
.
>> > + =A0 =A0 =A0 =A0*/
>> > + =A0 =A0 =A0 p =3D (u32 *)of_get_property(op->node, "xlnx,dcr-splb-sl=
ave-if", NULL);
>>
>> Hmmm. =A0This binding is undocumented. =A0It would be better to make the
>> decision on the presence/absence of the dcr-reg and/or reg properties.
>
> For backward compatibility with the 'old' way, the device tree generator =
for this core has both dcr-reg and reg properties (where the reg has been t=
ranslated back through the bridge).

So, what is in the regs and dcr-regs properties when DCR is used?

How about when MMIO is used?

g.


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

RE: [PATCH] [V3] Xilinx : Framebuffer Driver: Add PLB support and cleanup DCR

[Stephen Neuendorffer]

> -----Original Message-----
> From: Grant Likely [mailto:grant.likely@secretlab.ca]
> Sent: Wednesday, April 15, 2009 9:03 AM
> To: Stephen Neuendorffer
> Cc: John Linn; jwboyer@linux.vnet.ibm.com; linux-fbdev-devel@lists.source=
forge.net; linuxppc-
> dev@ozlabs.org; akonovalov@ru.mvista.com; adaplas@gmail.com; Suneel Garap=
ati
> Subject: Re: [PATCH] [V3] Xilinx : Framebuffer Driver: Add PLB support an=
d cleanup DCR
> =

> On Wed, Apr 15, 2009 at 9:58 AM, Stephen Neuendorffer
> <stephen.neuendorffer@xilinx.com> wrote:
> >> > - =A0 =A0 =A0 rc =3D of_address_to_resource(op->node, 0, &res);
> >> > - =A0 =A0 =A0 if (rc) {
> >> > - =A0 =A0 =A0 =A0 =A0 =A0 =A0 dev_err(&op->dev, "invalid address\n")=
;
> >> > - =A0 =A0 =A0 =A0 =A0 =A0 =A0 return rc;
> >> > + =A0 =A0 =A0 /*
> >> > + =A0 =A0 =A0 =A0* To check whether the core is connected directly t=
o DCR or PLB
> >> > + =A0 =A0 =A0 =A0* interface and initialize the tft_access according=
ly.
> >> > + =A0 =A0 =A0 =A0*/
> >> > + =A0 =A0 =A0 p =3D (u32 *)of_get_property(op->node, "xlnx,dcr-splb-=
slave-if", NULL);
> >>
> >> Hmmm. =A0This binding is undocumented. =A0It would be better to make t=
he
> >> decision on the presence/absence of the dcr-reg and/or reg properties.=

> >
> > For backward compatibility with the 'old' way, the device tree generato=
r for this core has both
> dcr-reg and reg properties (where the reg has been translated back throug=
h the bridge).
> =

> So, what is in the regs and dcr-regs properties when DCR is used?
> =

> How about when MMIO is used?
> =


Currently:
Core has DCR access, connected DCR bus:
	device tree contains dcr-reg property.
Core has DCR access, accessed through plb->dcr bridge:
	device tree contains dcr-reg property, AND for backward compatibility with=
 the =

	old driver, a reg property which contains the apparent registers on the bu=
s.
Core has PLB access: =

	device tree contains reg property only.

So, I guess it really doesn't matter... The only interesting case is the se=
cond one where (because the way the reg property is published), either meth=
od will work...  So, nevermind, the device tree question is completely inde=
pendent and I agree with your comment that there's probably not a need for =
a separate binding.

What I do think would be nice (at some point in the future maybe) is perhap=
s a bit of DCR_or_PLB abstraction for this core and the ll_temac driver to =
share which would avoid duplicating the code in each driver as to whether i=
t uses DCR or PLB access...

Steve


This email and any attachments are intended for the sole use of the named r=
ecipient(s) and contain(s) confidential information that may be proprietary=
, privileged or copyrighted under applicable law. If you are not the intend=
ed recipient, do not read, copy, or forward this email message or any attac=
hments. Delete this email message and any attachments immediately.

Re: [PATCH] [V3] Xilinx : Framebuffer Driver: Add PLB support and cleanup DCR

[Grant Likely]

On Wed, Apr 15, 2009 at 10:44 AM, Stephen Neuendorffer
<stephen.neuendorffer@xilinx.com> wrote:
>
>
>> -----Original Message-----
>> From: Grant Likely [mailto:grant.likely@secretlab.ca]
>> Sent: Wednesday, April 15, 2009 9:03 AM
>> To: Stephen Neuendorffer
>> Cc: John Linn; jwboyer@linux.vnet.ibm.com; linux-fbdev-devel@lists.sourc=
eforge.net; linuxppc-
>> dev@ozlabs.org; akonovalov@ru.mvista.com; adaplas@gmail.com; Suneel Gara=
pati
>> Subject: Re: [PATCH] [V3] Xilinx : Framebuffer Driver: Add PLB support a=
nd cleanup DCR
>>
>> On Wed, Apr 15, 2009 at 9:58 AM, Stephen Neuendorffer
>> <stephen.neuendorffer@xilinx.com> wrote:
>> >> > - =A0 =A0 =A0 rc =3D of_address_to_resource(op->node, 0, &res);
>> >> > - =A0 =A0 =A0 if (rc) {
>> >> > - =A0 =A0 =A0 =A0 =A0 =A0 =A0 dev_err(&op->dev, "invalid address\n"=
);
>> >> > - =A0 =A0 =A0 =A0 =A0 =A0 =A0 return rc;
>> >> > + =A0 =A0 =A0 /*
>> >> > + =A0 =A0 =A0 =A0* To check whether the core is connected directly =
to DCR or PLB
>> >> > + =A0 =A0 =A0 =A0* interface and initialize the tft_access accordin=
gly.
>> >> > + =A0 =A0 =A0 =A0*/
>> >> > + =A0 =A0 =A0 p =3D (u32 *)of_get_property(op->node, "xlnx,dcr-splb=
-slave-if", NULL);
>> >>
>> >> Hmmm. =A0This binding is undocumented. =A0It would be better to make =
the
>> >> decision on the presence/absence of the dcr-reg and/or reg properties=
.
>> >
>> > For backward compatibility with the 'old' way, the device tree generat=
or for this core has both
>> dcr-reg and reg properties (where the reg has been translated back throu=
gh the bridge).
>>
>> So, what is in the regs and dcr-regs properties when DCR is used?
>>
>> How about when MMIO is used?
>>
>
> Currently:
> Core has DCR access, connected DCR bus:
> =A0 =A0 =A0 =A0device tree contains dcr-reg property.
> Core has DCR access, accessed through plb->dcr bridge:
> =A0 =A0 =A0 =A0device tree contains dcr-reg property, AND for backward co=
mpatibility with the
> =A0 =A0 =A0 =A0old driver, a reg property which contains the apparent reg=
isters on the bus.
> Core has PLB access:
> =A0 =A0 =A0 =A0device tree contains reg property only.
>
> So, I guess it really doesn't matter... The only interesting case is the =
second one where (because the way the reg property is published), either me=
thod will work... =A0So, nevermind, the device tree question is completely =
independent and I agree with your comment that there's probably not a need =
for a separate binding.
>
> What I do think would be nice (at some point in the future maybe) is perh=
aps a bit of DCR_or_PLB abstraction for this core and the ll_temac driver t=
o share which would avoid duplicating the code in each driver as to whether=
 it uses DCR or PLB access...

Good idea.

g.

>
> Steve
>
>
> This email and any attachments are intended for the sole use of the named=
 recipient(s) and contain(s) confidential information that may be proprieta=
ry, privileged or copyrighted under applicable law. If you are not the inte=
nded recipient, do not read, copy, or forward this email message or any att=
achments. Delete this email message and any attachments immediately.
>
>
>



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