[PATCH v2 0/4] soc: amlogic: add meson-canvas

[PATCH v2 0/4] soc: amlogic: add meson-canvas

[Maxime Jourdan]

Amlogic SoCs feature a set of 256 canvas that act as pixel buffer
descriptors. Some IPs like the display and video decoders access those
pixels by using canvas IDs rather than direct phy addresses.

As such, allocating/manipulating canvases can be done concurrently and
there is a need for a standalone, lock-aware canvas provider module.

Currently, canvas code lies in the drm/meson module as it is the sole
user.

This patchset adds such canvas provider module and converts drm/meson to
using it, stripping/moving the current canvas code.

Changes since v1: [0]
 - Convert ops struct to a public API
 - Added comments
 - Hid the of-node probe code behind meson_canvas_get
 - Changed device lock to a spinlock with irqsave

[0]: https://lkml.org/lkml/2018/8/1/1512

Maxime Jourdan (4):
  soc: amlogic: add meson-canvas driver
  dt-bindings: soc: amlogic: add meson-canvas documentation
  ARM64: dts: meson-gx: add dmcbus and canvas nodes.
  drm/meson: convert to the new canvas module

 .../bindings/display/amlogic,meson-vpu.txt    |   9 +-
 .../soc/amlogic/amlogic,meson-canvas.txt      |  36 ++++
 arch/arm64/boot/dts/amlogic/meson-gx.dtsi     |  24 ++-
 drivers/gpu/drm/meson/Kconfig                 |   1 +
 drivers/gpu/drm/meson/Makefile                |   2 +-
 drivers/gpu/drm/meson/meson_canvas.c          |  70 -------
 drivers/gpu/drm/meson/meson_canvas.h          |  42 ----
 drivers/gpu/drm/meson/meson_crtc.c            |   9 +-
 drivers/gpu/drm/meson/meson_drv.c             |  22 +--
 drivers/gpu/drm/meson/meson_drv.h             |   5 +-
 drivers/gpu/drm/meson/meson_plane.c           |   3 +-
 drivers/gpu/drm/meson/meson_viu.c             |   1 -
 drivers/soc/amlogic/Kconfig                   |   7 +
 drivers/soc/amlogic/Makefile                  |   1 +
 drivers/soc/amlogic/meson-canvas.c            | 185 ++++++++++++++++++
 include/linux/soc/amlogic/meson-canvas.h      |  65 ++++++
 16 files changed, 337 insertions(+), 145 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/soc/amlogic/amlogic,meson-canvas.txt
 delete mode 100644 drivers/gpu/drm/meson/meson_canvas.c
 delete mode 100644 drivers/gpu/drm/meson/meson_canvas.h
 create mode 100644 drivers/soc/amlogic/meson-canvas.c
 create mode 100644 include/linux/soc/amlogic/meson-canvas.h

-- 
2.18.0

[PATCH v2 1/4] soc: amlogic: add meson-canvas driver

[Maxime Jourdan]

Amlogic SoCs have a repository of 256 canvas which they use to
describe pixel buffers.

They contain metadata like width, height, block mode, endianness [..]

Many IPs within those SoCs like vdec/vpu rely on those canvas to read/write
pixels.

Signed-off-by: Maxime Jourdan <maxi.jourdan@wanadoo.fr>
Tested-by: Neil Armstrong <narmstrong@baylibre.com>
---
 drivers/soc/amlogic/Kconfig              |   7 +
 drivers/soc/amlogic/Makefile             |   1 +
 drivers/soc/amlogic/meson-canvas.c       | 185 +++++++++++++++++++++++
 include/linux/soc/amlogic/meson-canvas.h |  65 ++++++++
 4 files changed, 258 insertions(+)
 create mode 100644 drivers/soc/amlogic/meson-canvas.c
 create mode 100644 include/linux/soc/amlogic/meson-canvas.h

diff --git a/drivers/soc/amlogic/Kconfig b/drivers/soc/amlogic/Kconfig
index b04f6e4aedbc..2f282b472912 100644
--- a/drivers/soc/amlogic/Kconfig
+++ b/drivers/soc/amlogic/Kconfig
@@ -1,5 +1,12 @@
 menu "Amlogic SoC drivers"
 
+config MESON_CANVAS
+	tristate "Amlogic Meson Canvas driver"
+	depends on ARCH_MESON || COMPILE_TEST
+	default n
+	help
+	  Say yes to support the canvas IP for Amlogic SoCs.
+
 config MESON_GX_SOCINFO
 	bool "Amlogic Meson GX SoC Information driver"
 	depends on ARCH_MESON || COMPILE_TEST
diff --git a/drivers/soc/amlogic/Makefile b/drivers/soc/amlogic/Makefile
index 8fa321893928..0ab16d35ac36 100644
--- a/drivers/soc/amlogic/Makefile
+++ b/drivers/soc/amlogic/Makefile
@@ -1,3 +1,4 @@
+obj-$(CONFIG_MESON_CANVAS) += meson-canvas.o
 obj-$(CONFIG_MESON_GX_SOCINFO) += meson-gx-socinfo.o
 obj-$(CONFIG_MESON_GX_PM_DOMAINS) += meson-gx-pwrc-vpu.o
 obj-$(CONFIG_MESON_MX_SOCINFO) += meson-mx-socinfo.o
diff --git a/drivers/soc/amlogic/meson-canvas.c b/drivers/soc/amlogic/meson-canvas.c
new file mode 100644
index 000000000000..c461334b36d4
--- /dev/null
+++ b/drivers/soc/amlogic/meson-canvas.c
@@ -0,0 +1,185 @@
+// SPDX-License-Identifier: GPL-2.0+
+/*
+ * Copyright (C) 2018 Maxime Jourdan <maxi.jourdan@wanadoo.fr>
+ * Copyright (C) 2016 BayLibre, SAS
+ * Author: Neil Armstrong <narmstrong@baylibre.com>
+ * Copyright (C) 2015 Amlogic, Inc. All rights reserved.
+ * Copyright (C) 2014 Endless Mobile
+ */
+
+#include <linux/kernel.h>
+#include <linux/mfd/syscon.h>
+#include <linux/module.h>
+#include <linux/regmap.h>
+#include <linux/soc/amlogic/meson-canvas.h>
+#include <linux/of_address.h>
+#include <linux/of_platform.h>
+#include <linux/io.h>
+
+#define NUM_CANVAS 256
+
+/* DMC Registers */
+#define DMC_CAV_LUT_DATAL	0x48 /* 0x12 offset in data sheet */
+	#define CANVAS_WIDTH_LBIT	29
+	#define CANVAS_WIDTH_LWID       3
+#define DMC_CAV_LUT_DATAH	0x4c /* 0x13 offset in data sheet */
+	#define CANVAS_WIDTH_HBIT       0
+	#define CANVAS_HEIGHT_BIT       9
+	#define CANVAS_BLKMODE_BIT      24
+#define DMC_CAV_LUT_ADDR	0x50 /* 0x14 offset in data sheet */
+	#define CANVAS_LUT_WR_EN        (0x2 << 8)
+	#define CANVAS_LUT_RD_EN        (0x1 << 8)
+
+struct meson_canvas {
+	struct device *dev;
+	struct regmap *regmap_dmc;
+	spinlock_t lock; /* canvas device lock */
+	u8 used[NUM_CANVAS];
+};
+
+struct meson_canvas *meson_canvas_get(struct device *dev)
+{
+	struct device_node *canvas_node;
+	struct platform_device *canvas_pdev;
+	struct meson_canvas *canvas;
+
+	canvas_node = of_parse_phandle(dev->of_node, "amlogic,canvas", 0);
+	if (!canvas_node)
+		return ERR_PTR(-ENODEV);
+
+	canvas_pdev = of_find_device_by_node(canvas_node);
+	if (!canvas_pdev) {
+		dev_err(dev, "Unable to find canvas pdev\n");
+		return ERR_PTR(-ENODEV);
+	}
+
+	canvas = dev_get_drvdata(&canvas_pdev->dev);
+	if (!canvas)
+		return ERR_PTR(-ENODEV);
+
+	return canvas;
+}
+EXPORT_SYMBOL_GPL(meson_canvas_get);
+
+int meson_canvas_setup(struct meson_canvas *canvas, u8 canvas_index,
+		       u32 addr, u32 stride, u32 height,
+		       unsigned int wrap,
+		       unsigned int blkmode,
+		       unsigned int endian)
+{
+	struct regmap *regmap = canvas->regmap_dmc;
+	unsigned long flags;
+	u32 val;
+
+	spin_lock_irqsave(&canvas->lock, flags);
+	if (!canvas->used[canvas_index]) {
+		dev_err(canvas->dev,
+			"Trying to setup non allocated canvas %u\n",
+			canvas_index);
+		spin_unlock_irqrestore(&canvas->lock, flags);
+		return -EINVAL;
+	}
+
+	regmap_write(regmap, DMC_CAV_LUT_DATAL,
+		     ((addr + 7) >> 3) |
+		     (((stride + 7) >> 3) << CANVAS_WIDTH_LBIT));
+
+	regmap_write(regmap, DMC_CAV_LUT_DATAH,
+		     ((((stride + 7) >> 3) >> CANVAS_WIDTH_LWID) <<
+						CANVAS_WIDTH_HBIT) |
+		     (height << CANVAS_HEIGHT_BIT) |
+		     (wrap << 22) |
+		     (blkmode << CANVAS_BLKMODE_BIT) |
+		     (endian << 26));
+
+	regmap_write(regmap, DMC_CAV_LUT_ADDR,
+		     CANVAS_LUT_WR_EN | canvas_index);
+
+	/* Force a read-back to make sure everything is flushed. */
+	regmap_read(regmap, DMC_CAV_LUT_DATAH, &val);
+	spin_unlock_irqrestore(&canvas->lock, flags);
+
+	return 0;
+}
+EXPORT_SYMBOL_GPL(meson_canvas_setup);
+
+int meson_canvas_alloc(struct meson_canvas *canvas, u8 *canvas_index)
+{
+	int i;
+	unsigned long flags;
+
+	spin_lock_irqsave(&canvas->lock, flags);
+	for (i = 0; i < NUM_CANVAS; ++i) {
+		if (!canvas->used[i]) {
+			canvas->used[i] = 1;
+			spin_unlock_irqrestore(&canvas->lock, flags);
+			*canvas_index = i;
+			return 0;
+		}
+	}
+	spin_unlock_irqrestore(&canvas->lock, flags);
+
+	dev_err(canvas->dev, "No more canvas available\n");
+	return -ENODEV;
+}
+EXPORT_SYMBOL_GPL(meson_canvas_alloc);
+
+int meson_canvas_free(struct meson_canvas *canvas, u8 canvas_index)
+{
+	unsigned long flags;
+
+	spin_lock_irqsave(&canvas->lock, flags);
+	if (!canvas->used[canvas_index]) {
+		dev_err(canvas->dev,
+			"Trying to free unused canvas %u\n", canvas_index);
+		spin_unlock_irqrestore(&canvas->lock, flags);
+		return -EINVAL;
+	}
+	canvas->used[canvas_index] = 0;
+	spin_unlock_irqrestore(&canvas->lock, flags);
+
+	return 0;
+}
+EXPORT_SYMBOL_GPL(meson_canvas_free);
+
+static int meson_canvas_probe(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	struct meson_canvas *canvas;
+
+	canvas = devm_kzalloc(dev, sizeof(*canvas), GFP_KERNEL);
+	if (!canvas)
+		return -ENOMEM;
+
+	canvas->regmap_dmc =
+		syscon_node_to_regmap(of_get_parent(dev->of_node));
+	if (IS_ERR(canvas->regmap_dmc)) {
+		dev_err(dev, "failed to get DMC regmap\n");
+		return PTR_ERR(canvas->regmap_dmc);
+	}
+
+	canvas->dev = dev;
+	spin_lock_init(&canvas->lock);
+	dev_set_drvdata(dev, canvas);
+
+	return 0;
+}
+
+static const struct of_device_id canvas_dt_match[] = {
+	{ .compatible = "amlogic,canvas" },
+	{}
+};
+MODULE_DEVICE_TABLE(of, canvas_dt_match);
+
+static struct platform_driver meson_canvas_driver = {
+	.probe = meson_canvas_probe,
+	.driver = {
+		.name = "amlogic-canvas",
+		.of_match_table = canvas_dt_match,
+	},
+};
+module_platform_driver(meson_canvas_driver);
+
+MODULE_DESCRIPTION("Amlogic Canvas driver");
+MODULE_AUTHOR("Maxime Jourdan <maxi.jourdan@wanadoo.fr>");
+MODULE_LICENSE("GPL");
diff --git a/include/linux/soc/amlogic/meson-canvas.h b/include/linux/soc/amlogic/meson-canvas.h
new file mode 100644
index 000000000000..c164202d8e39
--- /dev/null
+++ b/include/linux/soc/amlogic/meson-canvas.h
@@ -0,0 +1,65 @@
+/* SPDX-License-Identifier: GPL-2.0+ */
+/*
+ * Copyright (C) 2018 Maxime Jourdan <maxi.jourdan@wanadoo.fr>
+ */
+#ifndef MESON_CANVAS_H
+#define MESON_CANVAS_H
+
+#include <linux/kernel.h>
+
+#define MESON_CANVAS_WRAP_NONE	0x00
+#define MESON_CANVAS_WRAP_X	0x01
+#define MESON_CANVAS_WRAP_Y	0x02
+
+#define MESON_CANVAS_BLKMODE_LINEAR	0x00
+#define MESON_CANVAS_BLKMODE_32x32	0x01
+#define MESON_CANVAS_BLKMODE_64x64	0x02
+
+#define MESON_CANVAS_ENDIAN_SWAP16	0x1
+#define MESON_CANVAS_ENDIAN_SWAP32	0x3
+#define MESON_CANVAS_ENDIAN_SWAP64	0x7
+#define MESON_CANVAS_ENDIAN_SWAP128	0xf
+
+struct meson_canvas;
+
+/**
+ * meson_canvas_get() - get a canvas provider instance
+ *
+ * @dev: your device
+ */
+struct meson_canvas *meson_canvas_get(struct device *dev);
+
+/**
+ * meson_canvas_alloc() - take ownership of a canvas
+ *
+ * @canvas: canvas provider instance retrieved from meson_canvas_get()
+ * @canvas_index: will be filled with the canvas ID
+ */
+int meson_canvas_alloc(struct meson_canvas *canvas, u8 *canvas_index);
+
+/**
+ * meson_canvas_free() - remove ownership from a canvas
+ *
+ * @canvas: canvas provider instance retrieved from meson_canvas_get()
+ * @canvas_index: canvas ID that was obtained via meson_canvas_alloc()
+ */
+int meson_canvas_free(struct meson_canvas *canvas, u8 canvas_index);
+
+/**
+ * meson_canvas_setup() - configure a canvas
+ *
+ * @canvas: canvas provider instance retrieved from meson_canvas_get()
+ * @canvas_index: canvas ID that was obtained via meson_canvas_alloc()
+ * @addr: physical address to the pixel buffer
+ * @stride: width of the buffer
+ * @height: height of the buffer
+ * @wrap: undocumented
+ * @blkmode: block mode (linear, 32x32, 64x64)
+ * @endian: byte swapping (swap16, swap32, swap64, swap128)
+ */
+int meson_canvas_setup(struct meson_canvas *canvas, u8 canvas_index,
+		       u32 addr, u32 stride, u32 height,
+		       unsigned int wrap, unsigned int blkmode,
+		       unsigned int endian);
+
+#endif
-- 
2.18.0

[PATCH v2 2/4] dt-bindings: soc: amlogic: add meson-canvas documentation

[Maxime Jourdan]

DT bindings doc for amlogic,meson-canvas

Signed-off-by: Maxime Jourdan <maxi.jourdan@wanadoo.fr>
---
 .../soc/amlogic/amlogic,meson-canvas.txt      | 36 +++++++++++++++++++
 1 file changed, 36 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/soc/amlogic/amlogic,meson-canvas.txt

diff --git a/Documentation/devicetree/bindings/soc/amlogic/amlogic,meson-canvas.txt b/Documentation/devicetree/bindings/soc/amlogic/amlogic,meson-canvas.txt
new file mode 100644
index 000000000000..5f0351717bee
--- /dev/null
+++ b/Documentation/devicetree/bindings/soc/amlogic/amlogic,meson-canvas.txt
@@ -0,0 +1,36 @@
+Amlogic Canvas
+================================
+
+A canvas is a collection of metadata that describes a pixel buffer.
+Those metadata include: width, height, phyaddr, wrapping, block mode
+and endianness.
+
+Many IPs within Amlogic SoCs rely on canvas indexes to read/write pixel data
+rather than use the phy addresses directly. For instance, this is the case for
+the video decoders and the display.
+
+Amlogic SoCs have 256 canvas.
+
+Device Tree Bindings:
+---------------------
+
+Canvas Provider
+--------------------------
+
+Required properties:
+- compatible: "amlogic,canvas"
+
+Parent node should have the following properties :
+- compatible: "amlogic,gx-dmc-sysctrl", "syscon", "simple-mfd"
+- reg: base address and size of the DMC system control register space.
+
+Example:
+
+sysctrl_DMC: system-controller@0 {
+	compatible = "amlogic,gx-dmc-sysctrl", "syscon", "simple-mfd";
+	reg = <0x0 0x0 0x0 0x1000>;
+
+	canvas: canvas-provider@0 {
+		compatible = "amlogic,canvas";
+	};
+};
-- 
2.18.0

[PATCH v2 3/4] ARM64: dts: meson-gx: add dmcbus and canvas nodes.

[Maxime Jourdan]

Wrap the canvas node in a syscon node.

Signed-off-by: Maxime Jourdan <maxi.jourdan@wanadoo.fr>
---
 arch/arm64/boot/dts/amlogic/meson-gx.dtsi | 17 +++++++++++++++++
 1 file changed, 17 insertions(+)

diff --git a/arch/arm64/boot/dts/amlogic/meson-gx.dtsi b/arch/arm64/boot/dts/amlogic/meson-gx.dtsi
index b8dc4dbb391b..c98198662ae2 100644
--- a/arch/arm64/boot/dts/amlogic/meson-gx.dtsi
+++ b/arch/arm64/boot/dts/amlogic/meson-gx.dtsi
@@ -423,6 +423,23 @@
 			};
 		};
 
+		dmcbus: bus@c8838000 {
+			compatible = "simple-bus";
+			reg = <0x0 0xc8838000 0x0 0x1000>;
+			#address-cells = <2>;
+			#size-cells = <2>;
+			ranges = <0x0 0x0 0x0 0xc8838000 0x0 0x1000>;
+
+			sysctrl_DMC: system-controller@0 {
+				compatible = "amlogic,gx-dmc-sysctrl", "syscon", "simple-mfd";
+				reg = <0x0 0x0 0x0 0x1000>;
+
+				canvas: canvas-provider@0 {
+					compatible = "amlogic,canvas";
+				};
+			};
+		};
+
 		hiubus: bus@c883c000 {
 			compatible = "simple-bus";
 			reg = <0x0 0xc883c000 0x0 0x2000>;
-- 
2.18.0

[PATCH v2 4/4] drm/meson: convert to the new canvas module

[Maxime Jourdan]

This removes the meson_canvas files within the meson/drm layer
and makes use of the new canvas module that is referenced in the dts.

Canvases can be used by different IPs and modules, and it is as such
preferable to rely on a module that can safely dispatch canvases on
demand.

Signed-off-by: Maxime Jourdan <maxi.jourdan@wanadoo.fr>
Tested-by: Neil Armstrong <narmstrong@baylibre.com>
---
 .../bindings/display/amlogic,meson-vpu.txt    |  9 +--
 arch/arm64/boot/dts/amlogic/meson-gx.dtsi     |  7 +-
 drivers/gpu/drm/meson/Kconfig                 |  1 +
 drivers/gpu/drm/meson/Makefile                |  2 +-
 drivers/gpu/drm/meson/meson_canvas.c          | 70 -------------------
 drivers/gpu/drm/meson/meson_canvas.h          | 42 -----------
 drivers/gpu/drm/meson/meson_crtc.c            |  9 ++-
 drivers/gpu/drm/meson/meson_drv.c             | 22 ++----
 drivers/gpu/drm/meson/meson_drv.h             |  5 +-
 drivers/gpu/drm/meson/meson_plane.c           |  3 +-
 drivers/gpu/drm/meson/meson_viu.c             |  1 -
 11 files changed, 26 insertions(+), 145 deletions(-)
 delete mode 100644 drivers/gpu/drm/meson/meson_canvas.c
 delete mode 100644 drivers/gpu/drm/meson/meson_canvas.h

diff --git a/Documentation/devicetree/bindings/display/amlogic,meson-vpu.txt b/Documentation/devicetree/bindings/display/amlogic,meson-vpu.txt
index 057b81335775..60b6e1398636 100644
--- a/Documentation/devicetree/bindings/display/amlogic,meson-vpu.txt
+++ b/Documentation/devicetree/bindings/display/amlogic,meson-vpu.txt
@@ -60,9 +60,9 @@ Required properties:
 - reg: base address and size of he following memory-mapped regions :
 	- vpu
 	- hhi
-	- dmc
 - reg-names: should contain the names of the previous memory regions
 - interrupts: should contain the VENC Vsync interrupt number
+- amlogic,canvas: should point to a meson canvas provider node
 
 Optional properties:
 - power-domains: Optional phandle to associated power domain as described in
@@ -98,13 +98,14 @@ tv-connector {
 vpu: vpu@d0100000 {
 	compatible = "amlogic,meson-gxbb-vpu";
 	reg = <0x0 0xd0100000 0x0 0x100000>,
-	      <0x0 0xc883c000 0x0 0x1000>,
-	      <0x0 0xc8838000 0x0 0x1000>;
-	reg-names = "vpu", "hhi", "dmc";
+	      <0x0 0xc883c000 0x0 0x1000>;
+	reg-names = "vpu", "hhi";
 	interrupts = <GIC_SPI 3 IRQ_TYPE_EDGE_RISING>;
 	#address-cells = <1>;
 	#size-cells = <0>;
 
+	amlogic,canvas = <&canvas>;
+
 	/* CVBS VDAC output port */
 	port@0 {
 		reg = <0>;
diff --git a/arch/arm64/boot/dts/amlogic/meson-gx.dtsi b/arch/arm64/boot/dts/amlogic/meson-gx.dtsi
index c98198662ae2..8b4dfe7dd4bb 100644
--- a/arch/arm64/boot/dts/amlogic/meson-gx.dtsi
+++ b/arch/arm64/boot/dts/amlogic/meson-gx.dtsi
@@ -503,13 +503,14 @@
 		vpu: vpu@d0100000 {
 			compatible = "amlogic,meson-gx-vpu";
 			reg = <0x0 0xd0100000 0x0 0x100000>,
-			      <0x0 0xc883c000 0x0 0x1000>,
-			      <0x0 0xc8838000 0x0 0x1000>;
-			reg-names = "vpu", "hhi", "dmc";
+			      <0x0 0xc883c000 0x0 0x1000>;
+			reg-names = "vpu", "hhi";
 			interrupts = <GIC_SPI 3 IRQ_TYPE_EDGE_RISING>;
 			#address-cells = <1>;
 			#size-cells = <0>;
 
+			amlogic,canvas = <&canvas>;
+
 			/* CVBS VDAC output port */
 			cvbs_vdac_port: port@0 {
 				reg = <0>;
diff --git a/drivers/gpu/drm/meson/Kconfig b/drivers/gpu/drm/meson/Kconfig
index 3ce51d8dfe1c..c28b69f48555 100644
--- a/drivers/gpu/drm/meson/Kconfig
+++ b/drivers/gpu/drm/meson/Kconfig
@@ -7,6 +7,7 @@ config DRM_MESON
 	select DRM_GEM_CMA_HELPER
 	select VIDEOMODE_HELPERS
 	select REGMAP_MMIO
+	select MESON_CANVAS
 
 config DRM_MESON_DW_HDMI
 	tristate "HDMI Synopsys Controller support for Amlogic Meson Display"
diff --git a/drivers/gpu/drm/meson/Makefile b/drivers/gpu/drm/meson/Makefile
index c5c4cc362f02..bd67429185ff 100644
--- a/drivers/gpu/drm/meson/Makefile
+++ b/drivers/gpu/drm/meson/Makefile
@@ -1,5 +1,5 @@
 meson-drm-y := meson_drv.o meson_plane.o meson_crtc.o meson_venc_cvbs.o
-meson-drm-y += meson_viu.o meson_vpp.o meson_venc.o meson_vclk.o meson_canvas.o
+meson-drm-y += meson_viu.o meson_vpp.o meson_venc.o meson_vclk.o
 
 obj-$(CONFIG_DRM_MESON) += meson-drm.o
 obj-$(CONFIG_DRM_MESON_DW_HDMI) += meson_dw_hdmi.o
diff --git a/drivers/gpu/drm/meson/meson_canvas.c b/drivers/gpu/drm/meson/meson_canvas.c
deleted file mode 100644
index 08f6073d967e..000000000000
--- a/drivers/gpu/drm/meson/meson_canvas.c
+++ /dev/null
@@ -1,70 +0,0 @@
-/*
- * Copyright (C) 2016 BayLibre, SAS
- * Author: Neil Armstrong <narmstrong@baylibre.com>
- * Copyright (C) 2015 Amlogic, Inc. All rights reserved.
- * Copyright (C) 2014 Endless Mobile
- *
- * 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, see <http://www.gnu.org/licenses/>.
- */
-
-#include <linux/kernel.h>
-#include <linux/module.h>
-#include "meson_drv.h"
-#include "meson_canvas.h"
-#include "meson_registers.h"
-
-/**
- * DOC: Canvas
- *
- * CANVAS is a memory zone where physical memory frames information
- * are stored for the VIU to scanout.
- */
-
-/* DMC Registers */
-#define DMC_CAV_LUT_DATAL	0x48 /* 0x12 offset in data sheet */
-#define CANVAS_WIDTH_LBIT	29
-#define CANVAS_WIDTH_LWID       3
-#define DMC_CAV_LUT_DATAH	0x4c /* 0x13 offset in data sheet */
-#define CANVAS_WIDTH_HBIT       0
-#define CANVAS_HEIGHT_BIT       9
-#define CANVAS_BLKMODE_BIT      24
-#define DMC_CAV_LUT_ADDR	0x50 /* 0x14 offset in data sheet */
-#define CANVAS_LUT_WR_EN        (0x2 << 8)
-#define CANVAS_LUT_RD_EN        (0x1 << 8)
-
-void meson_canvas_setup(struct meson_drm *priv,
-			uint32_t canvas_index, uint32_t addr,
-			uint32_t stride, uint32_t height,
-			unsigned int wrap,
-			unsigned int blkmode)
-{
-	unsigned int val;
-
-	regmap_write(priv->dmc, DMC_CAV_LUT_DATAL,
-		(((addr + 7) >> 3)) |
-		(((stride + 7) >> 3) << CANVAS_WIDTH_LBIT));
-
-	regmap_write(priv->dmc, DMC_CAV_LUT_DATAH,
-		((((stride + 7) >> 3) >> CANVAS_WIDTH_LWID) <<
-						CANVAS_WIDTH_HBIT) |
-		(height << CANVAS_HEIGHT_BIT) |
-		(wrap << 22) |
-		(blkmode << CANVAS_BLKMODE_BIT));
-
-	regmap_write(priv->dmc, DMC_CAV_LUT_ADDR,
-			CANVAS_LUT_WR_EN | canvas_index);
-
-	/* Force a read-back to make sure everything is flushed. */
-	regmap_read(priv->dmc, DMC_CAV_LUT_DATAH, &val);
-}
diff --git a/drivers/gpu/drm/meson/meson_canvas.h b/drivers/gpu/drm/meson/meson_canvas.h
deleted file mode 100644
index af1759da4b27..000000000000
--- a/drivers/gpu/drm/meson/meson_canvas.h
+++ /dev/null
@@ -1,42 +0,0 @@
-/*
- * Copyright (C) 2016 BayLibre, SAS
- * Author: Neil Armstrong <narmstrong@baylibre.com>
- * Copyright (C) 2014 Endless Mobile
- *
- * 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, see <http://www.gnu.org/licenses/>.
- */
-
-/* Canvas LUT Memory */
-
-#ifndef __MESON_CANVAS_H
-#define __MESON_CANVAS_H
-
-#define MESON_CANVAS_ID_OSD1	0x4e
-
-/* Canvas configuration. */
-#define MESON_CANVAS_WRAP_NONE	0x00
-#define	MESON_CANVAS_WRAP_X	0x01
-#define	MESON_CANVAS_WRAP_Y	0x02
-
-#define	MESON_CANVAS_BLKMODE_LINEAR	0x00
-#define	MESON_CANVAS_BLKMODE_32x32	0x01
-#define	MESON_CANVAS_BLKMODE_64x64	0x02
-
-void meson_canvas_setup(struct meson_drm *priv,
-			uint32_t canvas_index, uint32_t addr,
-			uint32_t stride, uint32_t height,
-			unsigned int wrap,
-			unsigned int blkmode);
-
-#endif /* __MESON_CANVAS_H */
diff --git a/drivers/gpu/drm/meson/meson_crtc.c b/drivers/gpu/drm/meson/meson_crtc.c
index 05520202c967..9580b85de679 100644
--- a/drivers/gpu/drm/meson/meson_crtc.c
+++ b/drivers/gpu/drm/meson/meson_crtc.c
@@ -36,7 +36,6 @@
 #include "meson_venc.h"
 #include "meson_vpp.h"
 #include "meson_viu.h"
-#include "meson_canvas.h"
 #include "meson_registers.h"
 
 /* CRTC definition */
@@ -193,10 +192,10 @@ void meson_crtc_irq(struct meson_drm *priv)
 		} else
 			meson_vpp_disable_interlace_vscaler_osd1(priv);
 
-		meson_canvas_setup(priv, MESON_CANVAS_ID_OSD1,
-			   priv->viu.osd1_addr, priv->viu.osd1_stride,
-			   priv->viu.osd1_height, MESON_CANVAS_WRAP_NONE,
-			   MESON_CANVAS_BLKMODE_LINEAR);
+		meson_canvas_setup(priv->canvas, priv->canvas_id_osd1,
+				priv->viu.osd1_addr, priv->viu.osd1_stride,
+				priv->viu.osd1_height, MESON_CANVAS_WRAP_NONE,
+				MESON_CANVAS_BLKMODE_LINEAR, 0);
 
 		/* Enable OSD1 */
 		writel_bits_relaxed(VPP_OSD1_POSTBLEND, VPP_OSD1_POSTBLEND,
diff --git a/drivers/gpu/drm/meson/meson_drv.c b/drivers/gpu/drm/meson/meson_drv.c
index d3443125e661..fb5b0e3c5efc 100644
--- a/drivers/gpu/drm/meson/meson_drv.c
+++ b/drivers/gpu/drm/meson/meson_drv.c
@@ -47,7 +47,6 @@
 #include "meson_vpp.h"
 #include "meson_viu.h"
 #include "meson_venc.h"
-#include "meson_canvas.h"
 #include "meson_registers.h"
 
 #define DRIVER_NAME "meson"
@@ -216,25 +215,15 @@ static int meson_drv_bind_master(struct device *dev, bool has_components)
 		goto free_drm;
 	}
 
-	res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "dmc");
-	if (!res) {
-		ret = -EINVAL;
-		goto free_drm;
-	}
-	/* Simply ioremap since it may be a shared register zone */
-	regs = devm_ioremap(dev, res->start, resource_size(res));
-	if (!regs) {
-		ret = -EADDRNOTAVAIL;
+	priv->canvas = meson_canvas_get(dev);
+	if (IS_ERR(priv->canvas)) {
+		ret = PTR_ERR(priv->canvas);
 		goto free_drm;
 	}
 
-	priv->dmc = devm_regmap_init_mmio(dev, regs,
-					  &meson_regmap_config);
-	if (IS_ERR(priv->dmc)) {
-		dev_err(&pdev->dev, "Couldn't create the DMC regmap\n");
-		ret = PTR_ERR(priv->dmc);
+	ret = meson_canvas_alloc(priv->canvas, &priv->canvas_id_osd1);
+	if (ret)
 		goto free_drm;
-	}
 
 	priv->vsync_irq = platform_get_irq(pdev, 0);
 
@@ -315,6 +304,7 @@ static void meson_drv_unbind(struct device *dev)
 	struct drm_device *drm = dev_get_drvdata(dev);
 	struct meson_drm *priv = drm->dev_private;
 
+	meson_canvas_free(priv->canvas, priv->canvas_id_osd1);
 	drm_dev_unregister(drm);
 	drm_kms_helper_poll_fini(drm);
 	drm_fbdev_cma_fini(priv->fbdev);
diff --git a/drivers/gpu/drm/meson/meson_drv.h b/drivers/gpu/drm/meson/meson_drv.h
index 8450d6ac8c9b..9e902a6df784 100644
--- a/drivers/gpu/drm/meson/meson_drv.h
+++ b/drivers/gpu/drm/meson/meson_drv.h
@@ -22,15 +22,18 @@
 #include <linux/platform_device.h>
 #include <linux/regmap.h>
 #include <linux/of.h>
+#include <linux/soc/amlogic/meson-canvas.h>
 #include <drm/drmP.h>
 
 struct meson_drm {
 	struct device *dev;
 	void __iomem *io_base;
 	struct regmap *hhi;
-	struct regmap *dmc;
 	int vsync_irq;
 
+	struct meson_canvas *canvas;
+	u8 canvas_id_osd1;
+
 	struct drm_device *drm;
 	struct drm_crtc *crtc;
 	struct drm_fbdev_cma *fbdev;
diff --git a/drivers/gpu/drm/meson/meson_plane.c b/drivers/gpu/drm/meson/meson_plane.c
index 12c80dfcff59..8745f9209625 100644
--- a/drivers/gpu/drm/meson/meson_plane.c
+++ b/drivers/gpu/drm/meson/meson_plane.c
@@ -36,7 +36,6 @@
 #include "meson_plane.h"
 #include "meson_vpp.h"
 #include "meson_viu.h"
-#include "meson_canvas.h"
 #include "meson_registers.h"
 
 struct meson_plane {
@@ -105,7 +104,7 @@ static void meson_plane_atomic_update(struct drm_plane *plane,
 				   OSD_BLK0_ENABLE;
 
 	/* Set up BLK0 to point to the right canvas */
-	priv->viu.osd1_blk0_cfg[0] = ((MESON_CANVAS_ID_OSD1 << OSD_CANVAS_SEL) |
+	priv->viu.osd1_blk0_cfg[0] = ((priv->canvas_id_osd1 << OSD_CANVAS_SEL) |
 				      OSD_ENDIANNESS_LE);
 
 	/* On GXBB, Use the old non-HDR RGB2YUV converter */
diff --git a/drivers/gpu/drm/meson/meson_viu.c b/drivers/gpu/drm/meson/meson_viu.c
index 6bcfa527c180..5b48c4c0985b 100644
--- a/drivers/gpu/drm/meson/meson_viu.c
+++ b/drivers/gpu/drm/meson/meson_viu.c
@@ -25,7 +25,6 @@
 #include "meson_viu.h"
 #include "meson_vpp.h"
 #include "meson_venc.h"
-#include "meson_canvas.h"
 #include "meson_registers.h"
 
 /**
-- 
2.18.0

Re: [PATCH v2 1/4] soc: amlogic: add meson-canvas driver

[Jerome Brunet]

On Wed, 2018-08-08 at 00:00 +0200, Maxime Jourdan wrote:
> Amlogic SoCs have a repository of 256 canvas which they use to
> describe pixel buffers.
> 
> They contain metadata like width, height, block mode, endianness [..]
> 
> Many IPs within those SoCs like vdec/vpu rely on those canvas to read/write
> pixels.
> 
> Signed-off-by: Maxime Jourdan <maxi.jourdan@wanadoo.fr>
> Tested-by: Neil Armstrong <narmstrong@baylibre.com>

Thanks for making the changes Martin, I do prefer this version. You did not have
to drop the ops, until it is really needed because of some SoC quirks, I guess
it is simpler this way

With some answers to the nitpicks below, feel free to add :

Reviewed-by: Jerome Brunet <jbrunet@baylibre.com>

> ---
>  drivers/soc/amlogic/Kconfig              |   7 +
>  drivers/soc/amlogic/Makefile             |   1 +
>  drivers/soc/amlogic/meson-canvas.c       | 185 +++++++++++++++++++++++
>  include/linux/soc/amlogic/meson-canvas.h |  65 ++++++++
>  4 files changed, 258 insertions(+)
>  create mode 100644 drivers/soc/amlogic/meson-canvas.c
>  create mode 100644 include/linux/soc/amlogic/meson-canvas.h
> 
> diff --git a/drivers/soc/amlogic/Kconfig b/drivers/soc/amlogic/Kconfig
> index b04f6e4aedbc..2f282b472912 100644
> --- a/drivers/soc/amlogic/Kconfig
> +++ b/drivers/soc/amlogic/Kconfig
> @@ -1,5 +1,12 @@
>  menu "Amlogic SoC drivers"
>  
> +config MESON_CANVAS
> +	tristate "Amlogic Meson Canvas driver"
> +	depends on ARCH_MESON || COMPILE_TEST
> +	default n
> +	help
> +	  Say yes to support the canvas IP for Amlogic SoCs.
> +
>  config MESON_GX_SOCINFO
>  	bool "Amlogic Meson GX SoC Information driver"
>  	depends on ARCH_MESON || COMPILE_TEST
> diff --git a/drivers/soc/amlogic/Makefile b/drivers/soc/amlogic/Makefile
> index 8fa321893928..0ab16d35ac36 100644
> --- a/drivers/soc/amlogic/Makefile
> +++ b/drivers/soc/amlogic/Makefile
> @@ -1,3 +1,4 @@
> +obj-$(CONFIG_MESON_CANVAS) += meson-canvas.o
>  obj-$(CONFIG_MESON_GX_SOCINFO) += meson-gx-socinfo.o
>  obj-$(CONFIG_MESON_GX_PM_DOMAINS) += meson-gx-pwrc-vpu.o
>  obj-$(CONFIG_MESON_MX_SOCINFO) += meson-mx-socinfo.o
> diff --git a/drivers/soc/amlogic/meson-canvas.c b/drivers/soc/amlogic/meson-canvas.c
> new file mode 100644
> index 000000000000..c461334b36d4
> --- /dev/null
> +++ b/drivers/soc/amlogic/meson-canvas.c
> @@ -0,0 +1,185 @@
> +// SPDX-License-Identifier: GPL-2.0+
> +/*
> + * Copyright (C) 2018 Maxime Jourdan <maxi.jourdan@wanadoo.fr>
> + * Copyright (C) 2016 BayLibre, SAS
> + * Author: Neil Armstrong <narmstrong@baylibre.com>
> + * Copyright (C) 2015 Amlogic, Inc. All rights reserved.
> + * Copyright (C) 2014 Endless Mobile
> + */
> +
> +#include <linux/kernel.h>
> +#include <linux/mfd/syscon.h>
> +#include <linux/module.h>
> +#include <linux/regmap.h>
> +#include <linux/soc/amlogic/meson-canvas.h>
> +#include <linux/of_address.h>
> +#include <linux/of_platform.h>
> +#include <linux/io.h>
> +
> +#define NUM_CANVAS 256
> +
> +/* DMC Registers */
> +#define DMC_CAV_LUT_DATAL	0x48 /* 0x12 offset in data sheet */
> +	#define CANVAS_WIDTH_LBIT	29
> +	#define CANVAS_WIDTH_LWID       3
> +#define DMC_CAV_LUT_DATAH	0x4c /* 0x13 offset in data sheet */
> +	#define CANVAS_WIDTH_HBIT       0
> +	#define CANVAS_HEIGHT_BIT       9
> +	#define CANVAS_BLKMODE_BIT      24
> +#define DMC_CAV_LUT_ADDR	0x50 /* 0x14 offset in data sheet */
> +	#define CANVAS_LUT_WR_EN        (0x2 << 8)
> +	#define CANVAS_LUT_RD_EN        (0x1 << 8)
> +
> +struct meson_canvas {
> +	struct device *dev;
> +	struct regmap *regmap_dmc;
> +	spinlock_t lock; /* canvas device lock */
> +	u8 used[NUM_CANVAS];
> +};
> +
> +struct meson_canvas *meson_canvas_get(struct device *dev)
> +{
> +	struct device_node *canvas_node;
> +	struct platform_device *canvas_pdev;
> +	struct meson_canvas *canvas;
> +
> +	canvas_node = of_parse_phandle(dev->of_node, "amlogic,canvas", 0);
> +	if (!canvas_node)
> +		return ERR_PTR(-ENODEV);
> +
> +	canvas_pdev = of_find_device_by_node(canvas_node);
> +	if (!canvas_pdev) {
> +		dev_err(dev, "Unable to find canvas pdev\n");
> +		return ERR_PTR(-ENODEV);
> +	}
> +
> +	canvas = dev_get_drvdata(&canvas_pdev->dev);
> +	if (!canvas)
> +		return ERR_PTR(-ENODEV);

You've got your device at this point, maybe EINVAL instead ?

> +
> +	return canvas;
> +}
> +EXPORT_SYMBOL_GPL(meson_canvas_get);
> +
> +int meson_canvas_setup(struct meson_canvas *canvas, u8 canvas_index,
> +		       u32 addr, u32 stride, u32 height,
> +		       unsigned int wrap,
> +		       unsigned int blkmode,
> +		       unsigned int endian)
> +{
> +	struct regmap *regmap = canvas->regmap_dmc;
> +	unsigned long flags;
> +	u32 val;
> +
> +	spin_lock_irqsave(&canvas->lock, flags);
> +	if (!canvas->used[canvas_index]) {
> +		dev_err(canvas->dev,
> +			"Trying to setup non allocated canvas %u\n",
> +			canvas_index);
> +		spin_unlock_irqrestore(&canvas->lock, flags);
> +		return -EINVAL;
> +	}
> +
> +	regmap_write(regmap, DMC_CAV_LUT_DATAL,
> +		     ((addr + 7) >> 3) |
> +		     (((stride + 7) >> 3) << CANVAS_WIDTH_LBIT));
> +
> +	regmap_write(regmap, DMC_CAV_LUT_DATAH,
> +		     ((((stride + 7) >> 3) >> CANVAS_WIDTH_LWID) <<
> +						CANVAS_WIDTH_HBIT) |
> +		     (height << CANVAS_HEIGHT_BIT) |
> +		     (wrap << 22) |
> +		     (blkmode << CANVAS_BLKMODE_BIT) |
> +		     (endian << 26));
> +
> +	regmap_write(regmap, DMC_CAV_LUT_ADDR,
> +		     CANVAS_LUT_WR_EN | canvas_index);
> +
> +	/* Force a read-back to make sure everything is flushed. */
> +	regmap_read(regmap, DMC_CAV_LUT_DATAH, &val);

I suppose this something you got from the vendor kernel ? Out of curiosity, have
you tried w/o it ? I am wondering if this is really necessary ? 


> +	spin_unlock_irqrestore(&canvas->lock, flags);
> +
> +	return 0;
> +}
> +EXPORT_SYMBOL_GPL(meson_canvas_setup);
> +
> +int meson_canvas_alloc(struct meson_canvas *canvas, u8 *canvas_index)
> +{
> +	int i;
> +	unsigned long flags;
> +
> +	spin_lock_irqsave(&canvas->lock, flags);
> +	for (i = 0; i < NUM_CANVAS; ++i) {
> +		if (!canvas->used[i]) {
> +			canvas->used[i] = 1;
> +			spin_unlock_irqrestore(&canvas->lock, flags);
> +			*canvas_index = i;
> +			return 0;
> +		}
> +	}
> +	spin_unlock_irqrestore(&canvas->lock, flags);
> +
> +	dev_err(canvas->dev, "No more canvas available\n");
> +	return -ENODEV;
> +}
> +EXPORT_SYMBOL_GPL(meson_canvas_alloc);
> +
> +int meson_canvas_free(struct meson_canvas *canvas, u8 canvas_index)
> +{
> +	unsigned long flags;
> +
> +	spin_lock_irqsave(&canvas->lock, flags);
> +	if (!canvas->used[canvas_index]) {
> +		dev_err(canvas->dev,
> +			"Trying to free unused canvas %u\n", canvas_index);
> +		spin_unlock_irqrestore(&canvas->lock, flags);
> +		return -EINVAL;
> +	}
> +	canvas->used[canvas_index] = 0;
> +	spin_unlock_irqrestore(&canvas->lock, flags);
> +
> +	return 0;
> +}
> +EXPORT_SYMBOL_GPL(meson_canvas_free);
> +
> +static int meson_canvas_probe(struct platform_device *pdev)
> +{
> +	struct device *dev = &pdev->dev;
> +	struct meson_canvas *canvas;
> +
> +	canvas = devm_kzalloc(dev, sizeof(*canvas), GFP_KERNEL);
> +	if (!canvas)
> +		return -ENOMEM;
> +
> +	canvas->regmap_dmc =
> +		syscon_node_to_regmap(of_get_parent(dev->of_node));
> +	if (IS_ERR(canvas->regmap_dmc)) {
> +		dev_err(dev, "failed to get DMC regmap\n");
> +		return PTR_ERR(canvas->regmap_dmc);
> +	}
> +
> +	canvas->dev = dev;
> +	spin_lock_init(&canvas->lock);
> +	dev_set_drvdata(dev, canvas);
> +
> +	return 0;
> +}
> +
> +static const struct of_device_id canvas_dt_match[] = {
> +	{ .compatible = "amlogic,canvas" },
> +	{}
> +};
> +MODULE_DEVICE_TABLE(of, canvas_dt_match);
> +
> +static struct platform_driver meson_canvas_driver = {
> +	.probe = meson_canvas_probe,
> +	.driver = {
> +		.name = "amlogic-canvas",
> +		.of_match_table = canvas_dt_match,
> +	},
> +};
> +module_platform_driver(meson_canvas_driver);
> +
> +MODULE_DESCRIPTION("Amlogic Canvas driver");
> +MODULE_AUTHOR("Maxime Jourdan <maxi.jourdan@wanadoo.fr>");
> +MODULE_LICENSE("GPL");
> diff --git a/include/linux/soc/amlogic/meson-canvas.h b/include/linux/soc/amlogic/meson-canvas.h
> new file mode 100644
> index 000000000000..c164202d8e39
> --- /dev/null
> +++ b/include/linux/soc/amlogic/meson-canvas.h
> @@ -0,0 +1,65 @@
> +/* SPDX-License-Identifier: GPL-2.0+ */
> +/*
> + * Copyright (C) 2018 Maxime Jourdan <maxi.jourdan@wanadoo.fr>
> + */
> +#ifndef MESON_CANVAS_H
> +#define MESON_CANVAS_H
> +
> +#include <linux/kernel.h>
> +
> +#define MESON_CANVAS_WRAP_NONE	0x00
> +#define MESON_CANVAS_WRAP_X	0x01
> +#define MESON_CANVAS_WRAP_Y	0x02
> +
> +#define MESON_CANVAS_BLKMODE_LINEAR	0x00
> +#define MESON_CANVAS_BLKMODE_32x32	0x01
> +#define MESON_CANVAS_BLKMODE_64x64	0x02
> +
> +#define MESON_CANVAS_ENDIAN_SWAP16	0x1
> +#define MESON_CANVAS_ENDIAN_SWAP32	0x3
> +#define MESON_CANVAS_ENDIAN_SWAP64	0x7
> +#define MESON_CANVAS_ENDIAN_SWAP128	0xf
> +
> +struct meson_canvas;
> +
> +/**
> + * meson_canvas_get() - get a canvas provider instance
> + *
> + * @dev: your device

s/your device/consumer device pointer ?

> + */
> +struct meson_canvas *meson_canvas_get(struct device *dev);
> +
> +/**
> + * meson_canvas_alloc() - take ownership of a canvas
> + *
> + * @canvas: canvas provider instance retrieved from meson_canvas_get()
> + * @canvas_index: will be filled with the canvas ID
> + */
> +int meson_canvas_alloc(struct meson_canvas *canvas, u8 *canvas_index);
> +
> +/**
> + * meson_canvas_free() - remove ownership from a canvas
> + *
> + * @canvas: canvas provider instance retrieved from meson_canvas_get()
> + * @canvas_index: canvas ID that was obtained via meson_canvas_alloc()
> + */
> +int meson_canvas_free(struct meson_canvas *canvas, u8 canvas_index);
> +
> +/**
> + * meson_canvas_setup() - configure a canvas
> + *
> + * @canvas: canvas provider instance retrieved from meson_canvas_get()
> + * @canvas_index: canvas ID that was obtained via meson_canvas_alloc()
> + * @addr: physical address to the pixel buffer
> + * @stride: width of the buffer
> + * @height: height of the buffer
> + * @wrap: undocumented
> + * @blkmode: block mode (linear, 32x32, 64x64)
> + * @endian: byte swapping (swap16, swap32, swap64, swap128)
> + */
> +int meson_canvas_setup(struct meson_canvas *canvas, u8 canvas_index,
> +		       u32 addr, u32 stride, u32 height,
> +		       unsigned int wrap, unsigned int blkmode,
> +		       unsigned int endian);
> +
> +#endif

Re: [PATCH v2 3/4] ARM64: dts: meson-gx: add dmcbus and canvas nodes.

[Jerome Brunet]

On Wed, 2018-08-08 at 00:00 +0200, Maxime Jourdan wrote:
> Wrap the canvas node in a syscon node.
> 
> Signed-off-by: Maxime Jourdan <maxi.jourdan@wanadoo.fr>
> ---
>  arch/arm64/boot/dts/amlogic/meson-gx.dtsi | 17 +++++++++++++++++
>  1 file changed, 17 insertions(+)
> 
> diff --git a/arch/arm64/boot/dts/amlogic/meson-gx.dtsi b/arch/arm64/boot/dts/amlogic/meson-gx.dtsi
> index b8dc4dbb391b..c98198662ae2 100644
> --- a/arch/arm64/boot/dts/amlogic/meson-gx.dtsi
> +++ b/arch/arm64/boot/dts/amlogic/meson-gx.dtsi
> @@ -423,6 +423,23 @@
>  			};
>  		};
>  
> +		dmcbus: bus@c8838000 {
> +			compatible = "simple-bus";
> +			reg = <0x0 0xc8838000 0x0 0x1000>;
> +			#address-cells = <2>;
> +			#size-cells = <2>;
> +			ranges = <0x0 0x0 0x0 0xc8838000 0x0 0x1000>;
> +
> +			sysctrl_DMC: system-controller@0 {
> +				compatible = "amlogic,gx-dmc-sysctrl", "syscon", "simple-mfd";
> +				reg = <0x0 0x0 0x0 0x1000>;
> +
> +				canvas: canvas-provider@0 {
> +					compatible = "amlogic,canvas";

If there is only one canvas provider under "sysctrl_DMC" and it has no reg
property , you should not put a unit-address (@0) here. (same for the
documentation patch)

You may have seen unit-address without a reg property used elsewhere (ASoC
simple-card, my recent axg-sound-card), when there is multiple node with the
same node-name (ex: dai-link).

As Martin pointed out, the DT spec says we should not use unit-address unless
there is a reg property. We did not get Rob's view on this and we might have to
update this later on. In your case, unless I missed something, you should
definitely not have it

nitpick regarding the node-name (canvas-provider). If appropriate, we should try
to stick to one of the generic names proposed in the spec. I wonder if the
canvas provider could be viewed as a "memory" or "memory-controller"

So, what about this ? Just a proposition, feel free to comment ;)

	sysctrl_DMC: system-controller@0 {							compatib
le = "amlogic,gx-dmc-sysctrl", "syscon", "simple-mfd";> 		reg =
<0x0 0x0 0x0 0x1000>;
	
		canvas: memory-controller {
			compatible = "amlogic,canvas";
		}

[...]


> +				};
> +			};
> +		};
> +
>  		hiubus: bus@c883c000 {
>  			compatible = "simple-bus";
>  			reg = <0x0 0xc883c000 0x0 0x2000>;

Re: [PATCH v2 1/4] soc: amlogic: add meson-canvas driver

[Neil Armstrong]

On 08/08/2018 09:45, Jerome Brunet wrote:
> On Wed, 2018-08-08 at 00:00 +0200, Maxime Jourdan wrote:
>> Amlogic SoCs have a repository of 256 canvas which they use to
>> describe pixel buffers.
>>
>> They contain metadata like width, height, block mode, endianness [..]
>>
>> Many IPs within those SoCs like vdec/vpu rely on those canvas to read/write
>> pixels.
>>
>> Signed-off-by: Maxime Jourdan <maxi.jourdan@wanadoo.fr>
>> Tested-by: Neil Armstrong <narmstrong@baylibre.com>
> 
> Thanks for making the changes Martin, I do prefer this version. You did not have
> to drop the ops, until it is really needed because of some SoC quirks, I guess
> it is simpler this way
> 
> With some answers to the nitpicks below, feel free to add :
> 
> Reviewed-by: Jerome Brunet <jbrunet@baylibre.com>
> 
>> ---
>>  drivers/soc/amlogic/Kconfig              |   7 +
>>  drivers/soc/amlogic/Makefile             |   1 +
>>  drivers/soc/amlogic/meson-canvas.c       | 185 +++++++++++++++++++++++
>>  include/linux/soc/amlogic/meson-canvas.h |  65 ++++++++
>>  4 files changed, 258 insertions(+)
>>  create mode 100644 drivers/soc/amlogic/meson-canvas.c
>>  create mode 100644 include/linux/soc/amlogic/meson-canvas.h
>>
>> diff --git a/drivers/soc/amlogic/Kconfig b/drivers/soc/amlogic/Kconfig
>> index b04f6e4aedbc..2f282b472912 100644
>> --- a/drivers/soc/amlogic/Kconfig
>> +++ b/drivers/soc/amlogic/Kconfig
>> @@ -1,5 +1,12 @@
>>  menu "Amlogic SoC drivers"
>>  
>> +config MESON_CANVAS
>> +	tristate "Amlogic Meson Canvas driver"
>> +	depends on ARCH_MESON || COMPILE_TEST
>> +	default n
>> +	help
>> +	  Say yes to support the canvas IP for Amlogic SoCs.
>> +
>>  config MESON_GX_SOCINFO
>>  	bool "Amlogic Meson GX SoC Information driver"
>>  	depends on ARCH_MESON || COMPILE_TEST
>> diff --git a/drivers/soc/amlogic/Makefile b/drivers/soc/amlogic/Makefile
>> index 8fa321893928..0ab16d35ac36 100644
>> --- a/drivers/soc/amlogic/Makefile
>> +++ b/drivers/soc/amlogic/Makefile
>> @@ -1,3 +1,4 @@
>> +obj-$(CONFIG_MESON_CANVAS) += meson-canvas.o
>>  obj-$(CONFIG_MESON_GX_SOCINFO) += meson-gx-socinfo.o
>>  obj-$(CONFIG_MESON_GX_PM_DOMAINS) += meson-gx-pwrc-vpu.o
>>  obj-$(CONFIG_MESON_MX_SOCINFO) += meson-mx-socinfo.o
>> diff --git a/drivers/soc/amlogic/meson-canvas.c b/drivers/soc/amlogic/meson-canvas.c
>> new file mode 100644
>> index 000000000000..c461334b36d4
>> --- /dev/null
>> +++ b/drivers/soc/amlogic/meson-canvas.c
>> @@ -0,0 +1,185 @@
>> +// SPDX-License-Identifier: GPL-2.0+
>> +/*
>> + * Copyright (C) 2018 Maxime Jourdan <maxi.jourdan@wanadoo.fr>
>> + * Copyright (C) 2016 BayLibre, SAS
>> + * Author: Neil Armstrong <narmstrong@baylibre.com>
>> + * Copyright (C) 2015 Amlogic, Inc. All rights reserved.
>> + * Copyright (C) 2014 Endless Mobile
>> + */
>> +
>> +#include <linux/kernel.h>
>> +#include <linux/mfd/syscon.h>
>> +#include <linux/module.h>
>> +#include <linux/regmap.h>
>> +#include <linux/soc/amlogic/meson-canvas.h>
>> +#include <linux/of_address.h>
>> +#include <linux/of_platform.h>
>> +#include <linux/io.h>
>> +
>> +#define NUM_CANVAS 256
>> +
>> +/* DMC Registers */
>> +#define DMC_CAV_LUT_DATAL	0x48 /* 0x12 offset in data sheet */
>> +	#define CANVAS_WIDTH_LBIT	29
>> +	#define CANVAS_WIDTH_LWID       3
>> +#define DMC_CAV_LUT_DATAH	0x4c /* 0x13 offset in data sheet */
>> +	#define CANVAS_WIDTH_HBIT       0
>> +	#define CANVAS_HEIGHT_BIT       9
>> +	#define CANVAS_BLKMODE_BIT      24
>> +#define DMC_CAV_LUT_ADDR	0x50 /* 0x14 offset in data sheet */
>> +	#define CANVAS_LUT_WR_EN        (0x2 << 8)
>> +	#define CANVAS_LUT_RD_EN        (0x1 << 8)
>> +
>> +struct meson_canvas {
>> +	struct device *dev;
>> +	struct regmap *regmap_dmc;
>> +	spinlock_t lock; /* canvas device lock */
>> +	u8 used[NUM_CANVAS];
>> +};
>> +
>> +struct meson_canvas *meson_canvas_get(struct device *dev)
>> +{
>> +	struct device_node *canvas_node;
>> +	struct platform_device *canvas_pdev;
>> +	struct meson_canvas *canvas;
>> +
>> +	canvas_node = of_parse_phandle(dev->of_node, "amlogic,canvas", 0);
>> +	if (!canvas_node)
>> +		return ERR_PTR(-ENODEV);
>> +
>> +	canvas_pdev = of_find_device_by_node(canvas_node);
>> +	if (!canvas_pdev) {
>> +		dev_err(dev, "Unable to find canvas pdev\n");
>> +		return ERR_PTR(-ENODEV);
>> +	}

I should be -EPROBE_DEFER here since the canvas driver maybe hasn't probed yet.

>> +
>> +	canvas = dev_get_drvdata(&canvas_pdev->dev);
>> +	if (!canvas)
>> +		return ERR_PTR(-ENODEV);
> 
> You've got your device at this point, maybe EINVAL instead ?

This shouldn't fail here... I'm not sure what should be the error.

> 
>> +
>> +	return canvas;
>> +}
>> +EXPORT_SYMBOL_GPL(meson_canvas_get);
>> +
>> +int meson_canvas_setup(struct meson_canvas *canvas, u8 canvas_index,
>> +		       u32 addr, u32 stride, u32 height,
>> +		       unsigned int wrap,
>> +		       unsigned int blkmode,
>> +		       unsigned int endian)
>> +{
>> +	struct regmap *regmap = canvas->regmap_dmc;
>> +	unsigned long flags;
>> +	u32 val;
>> +
>> +	spin_lock_irqsave(&canvas->lock, flags);
>> +	if (!canvas->used[canvas_index]) {
>> +		dev_err(canvas->dev,
>> +			"Trying to setup non allocated canvas %u\n",
>> +			canvas_index);
>> +		spin_unlock_irqrestore(&canvas->lock, flags);
>> +		return -EINVAL;
>> +	}
>> +
>> +	regmap_write(regmap, DMC_CAV_LUT_DATAL,
>> +		     ((addr + 7) >> 3) |
>> +		     (((stride + 7) >> 3) << CANVAS_WIDTH_LBIT));
>> +
>> +	regmap_write(regmap, DMC_CAV_LUT_DATAH,
>> +		     ((((stride + 7) >> 3) >> CANVAS_WIDTH_LWID) <<
>> +						CANVAS_WIDTH_HBIT) |
>> +		     (height << CANVAS_HEIGHT_BIT) |
>> +		     (wrap << 22) |
>> +		     (blkmode << CANVAS_BLKMODE_BIT) |
>> +		     (endian << 26));
>> +
>> +	regmap_write(regmap, DMC_CAV_LUT_ADDR,
>> +		     CANVAS_LUT_WR_EN | canvas_index);
>> +
>> +	/* Force a read-back to make sure everything is flushed. */
>> +	regmap_read(regmap, DMC_CAV_LUT_DATAH, &val);
> 
> I suppose this something you got from the vendor kernel ? Out of curiosity, have
> you tried w/o it ? I am wondering if this is really necessary ? 


It's explicit in the Amlogic source, you need a readback to make sure it's taken in account.

> 
> 
>> +	spin_unlock_irqrestore(&canvas->lock, flags);
>> +
>> +	return 0;
>> +}
>> +EXPORT_SYMBOL_GPL(meson_canvas_setup);
>> +
>> +int meson_canvas_alloc(struct meson_canvas *canvas, u8 *canvas_index)
>> +{
>> +	int i;
>> +	unsigned long flags;
>> +
>> +	spin_lock_irqsave(&canvas->lock, flags);
>> +	for (i = 0; i < NUM_CANVAS; ++i) {
>> +		if (!canvas->used[i]) {
>> +			canvas->used[i] = 1;
>> +			spin_unlock_irqrestore(&canvas->lock, flags);
>> +			*canvas_index = i;
>> +			return 0;
>> +		}
>> +	}
>> +	spin_unlock_irqrestore(&canvas->lock, flags);
>> +
>> +	dev_err(canvas->dev, "No more canvas available\n");
>> +	return -ENODEV;
>> +}
>> +EXPORT_SYMBOL_GPL(meson_canvas_alloc);
>> +
>> +int meson_canvas_free(struct meson_canvas *canvas, u8 canvas_index)
>> +{
>> +	unsigned long flags;
>> +
>> +	spin_lock_irqsave(&canvas->lock, flags);
>> +	if (!canvas->used[canvas_index]) {
>> +		dev_err(canvas->dev,
>> +			"Trying to free unused canvas %u\n", canvas_index);
>> +		spin_unlock_irqrestore(&canvas->lock, flags);
>> +		return -EINVAL;
>> +	}
>> +	canvas->used[canvas_index] = 0;
>> +	spin_unlock_irqrestore(&canvas->lock, flags);
>> +
>> +	return 0;
>> +}
>> +EXPORT_SYMBOL_GPL(meson_canvas_free);
>> +
>> +static int meson_canvas_probe(struct platform_device *pdev)
>> +{
>> +	struct device *dev = &pdev->dev;
>> +	struct meson_canvas *canvas;
>> +
>> +	canvas = devm_kzalloc(dev, sizeof(*canvas), GFP_KERNEL);
>> +	if (!canvas)
>> +		return -ENOMEM;
>> +
>> +	canvas->regmap_dmc =
>> +		syscon_node_to_regmap(of_get_parent(dev->of_node));
>> +	if (IS_ERR(canvas->regmap_dmc)) {
>> +		dev_err(dev, "failed to get DMC regmap\n");
>> +		return PTR_ERR(canvas->regmap_dmc);
>> +	}
>> +
>> +	canvas->dev = dev;
>> +	spin_lock_init(&canvas->lock);
>> +	dev_set_drvdata(dev, canvas);
>> +
>> +	return 0;
>> +}
>> +
>> +static const struct of_device_id canvas_dt_match[] = {
>> +	{ .compatible = "amlogic,canvas" },
>> +	{}
>> +};
>> +MODULE_DEVICE_TABLE(of, canvas_dt_match);
>> +
>> +static struct platform_driver meson_canvas_driver = {
>> +	.probe = meson_canvas_probe,
>> +	.driver = {
>> +		.name = "amlogic-canvas",
>> +		.of_match_table = canvas_dt_match,
>> +	},
>> +};
>> +module_platform_driver(meson_canvas_driver);
>> +
>> +MODULE_DESCRIPTION("Amlogic Canvas driver");
>> +MODULE_AUTHOR("Maxime Jourdan <maxi.jourdan@wanadoo.fr>");
>> +MODULE_LICENSE("GPL");
>> diff --git a/include/linux/soc/amlogic/meson-canvas.h b/include/linux/soc/amlogic/meson-canvas.h
>> new file mode 100644
>> index 000000000000..c164202d8e39
>> --- /dev/null
>> +++ b/include/linux/soc/amlogic/meson-canvas.h
>> @@ -0,0 +1,65 @@
>> +/* SPDX-License-Identifier: GPL-2.0+ */
>> +/*
>> + * Copyright (C) 2018 Maxime Jourdan <maxi.jourdan@wanadoo.fr>
>> + */
>> +#ifndef MESON_CANVAS_H
>> +#define MESON_CANVAS_H
>> +
>> +#include <linux/kernel.h>
>> +
>> +#define MESON_CANVAS_WRAP_NONE	0x00
>> +#define MESON_CANVAS_WRAP_X	0x01
>> +#define MESON_CANVAS_WRAP_Y	0x02
>> +
>> +#define MESON_CANVAS_BLKMODE_LINEAR	0x00
>> +#define MESON_CANVAS_BLKMODE_32x32	0x01
>> +#define MESON_CANVAS_BLKMODE_64x64	0x02
>> +
>> +#define MESON_CANVAS_ENDIAN_SWAP16	0x1
>> +#define MESON_CANVAS_ENDIAN_SWAP32	0x3
>> +#define MESON_CANVAS_ENDIAN_SWAP64	0x7
>> +#define MESON_CANVAS_ENDIAN_SWAP128	0xf
>> +
>> +struct meson_canvas;
>> +
>> +/**
>> + * meson_canvas_get() - get a canvas provider instance
>> + *
>> + * @dev: your device
> 
> s/your device/consumer device pointer ?
> 
>> + */
>> +struct meson_canvas *meson_canvas_get(struct device *dev);
>> +
>> +/**
>> + * meson_canvas_alloc() - take ownership of a canvas
>> + *
>> + * @canvas: canvas provider instance retrieved from meson_canvas_get()
>> + * @canvas_index: will be filled with the canvas ID
>> + */
>> +int meson_canvas_alloc(struct meson_canvas *canvas, u8 *canvas_index);
>> +
>> +/**
>> + * meson_canvas_free() - remove ownership from a canvas
>> + *
>> + * @canvas: canvas provider instance retrieved from meson_canvas_get()
>> + * @canvas_index: canvas ID that was obtained via meson_canvas_alloc()
>> + */
>> +int meson_canvas_free(struct meson_canvas *canvas, u8 canvas_index);
>> +
>> +/**
>> + * meson_canvas_setup() - configure a canvas
>> + *
>> + * @canvas: canvas provider instance retrieved from meson_canvas_get()
>> + * @canvas_index: canvas ID that was obtained via meson_canvas_alloc()
>> + * @addr: physical address to the pixel buffer
>> + * @stride: width of the buffer
>> + * @height: height of the buffer
>> + * @wrap: undocumented
>> + * @blkmode: block mode (linear, 32x32, 64x64)
>> + * @endian: byte swapping (swap16, swap32, swap64, swap128)
>> + */
>> +int meson_canvas_setup(struct meson_canvas *canvas, u8 canvas_index,
>> +		       u32 addr, u32 stride, u32 height,
>> +		       unsigned int wrap, unsigned int blkmode,
>> +		       unsigned int endian);
>> +
>> +#endif
> 
> 

Re: [PATCH v2 3/4] ARM64: dts: meson-gx: add dmcbus and canvas nodes.

[Neil Armstrong]

On 08/08/2018 10:41, Jerome Brunet wrote:
> On Wed, 2018-08-08 at 00:00 +0200, Maxime Jourdan wrote:
>> Wrap the canvas node in a syscon node.
>>
>> Signed-off-by: Maxime Jourdan <maxi.jourdan@wanadoo.fr>
>> ---
>>  arch/arm64/boot/dts/amlogic/meson-gx.dtsi | 17 +++++++++++++++++
>>  1 file changed, 17 insertions(+)
>>
>> diff --git a/arch/arm64/boot/dts/amlogic/meson-gx.dtsi b/arch/arm64/boot/dts/amlogic/meson-gx.dtsi
>> index b8dc4dbb391b..c98198662ae2 100644
>> --- a/arch/arm64/boot/dts/amlogic/meson-gx.dtsi
>> +++ b/arch/arm64/boot/dts/amlogic/meson-gx.dtsi
>> @@ -423,6 +423,23 @@
>>  			};
>>  		};
>>  
>> +		dmcbus: bus@c8838000 {
>> +			compatible = "simple-bus";
>> +			reg = <0x0 0xc8838000 0x0 0x1000>;
>> +			#address-cells = <2>;
>> +			#size-cells = <2>;
>> +			ranges = <0x0 0x0 0x0 0xc8838000 0x0 0x1000>;
>> +
>> +			sysctrl_DMC: system-controller@0 {
>> +				compatible = "amlogic,gx-dmc-sysctrl", "syscon", "simple-mfd";
>> +				reg = <0x0 0x0 0x0 0x1000>;
>> +
>> +				canvas: canvas-provider@0 {
>> +					compatible = "amlogic,canvas";
> 
> If there is only one canvas provider under "sysctrl_DMC" and it has no reg
> property , you should not put a unit-address (@0) here. (same for the
> documentation patch)
> 
> You may have seen unit-address without a reg property used elsewhere (ASoC
> simple-card, my recent axg-sound-card), when there is multiple node with the
> same node-name (ex: dai-link).
> 
> As Martin pointed out, the DT spec says we should not use unit-address unless
> there is a reg property. We did not get Rob's view on this and we might have to
> update this later on. In your case, unless I missed something, you should
> definitely not have it
> 
> nitpick regarding the node-name (canvas-provider). If appropriate, we should try
> to stick to one of the generic names proposed in the spec. I wonder if the
> canvas provider could be viewed as a "memory" or "memory-controller"

It's not really a memory or memory-controller, it's a Lookup Table on top of the memory controller,
nothing really matches here...

> 
> So, what about this ? Just a proposition, feel free to comment ;)
> 
> 	sysctrl_DMC: system-controller@0 {							compatib
> le = "amlogic,gx-dmc-sysctrl", "syscon", "simple-mfd";> 		reg =
> <0x0 0x0 0x0 0x1000>;
> 	
> 		canvas: memory-controller {
> 			compatible = "amlogic,canvas";
> 		}
> 
> [...]
> 
> 
>> +				};
>> +			};
>> +		};
>> +
>>  		hiubus: bus@c883c000 {
>>  			compatible = "simple-bus";
>>  			reg = <0x0 0xc883c000 0x0 0x2000>;
> 
> 

Re: [PATCH v2 1/4] soc: amlogic: add meson-canvas driver

[Maxime Jourdan]

2018-08-08 17:10 GMT+02:00 Neil Armstrong <narmstrong@baylibre.com>:
> On 08/08/2018 09:45, Jerome Brunet wrote:
>> On Wed, 2018-08-08 at 00:00 +0200, Maxime Jourdan wrote:
>>> Amlogic SoCs have a repository of 256 canvas which they use to
>>> describe pixel buffers.
>>>
>>> They contain metadata like width, height, block mode, endianness [..]
>>>
>>> Many IPs within those SoCs like vdec/vpu rely on those canvas to read/write
>>> pixels.
>>>
>>> Signed-off-by: Maxime Jourdan <maxi.jourdan@wanadoo.fr>
>>> Tested-by: Neil Armstrong <narmstrong@baylibre.com>
>>
>> Thanks for making the changes Martin, I do prefer this version. You did not have
>> to drop the ops, until it is really needed because of some SoC quirks, I guess
>> it is simpler this way
>>
>> With some answers to the nitpicks below, feel free to add :
>>
>> Reviewed-by: Jerome Brunet <jbrunet@baylibre.com>
>>
>>> ---
>>>  drivers/soc/amlogic/Kconfig              |   7 +
>>>  drivers/soc/amlogic/Makefile             |   1 +
>>>  drivers/soc/amlogic/meson-canvas.c       | 185 +++++++++++++++++++++++
>>>  include/linux/soc/amlogic/meson-canvas.h |  65 ++++++++
>>>  4 files changed, 258 insertions(+)
>>>  create mode 100644 drivers/soc/amlogic/meson-canvas.c
>>>  create mode 100644 include/linux/soc/amlogic/meson-canvas.h
>>>
>>> diff --git a/drivers/soc/amlogic/Kconfig b/drivers/soc/amlogic/Kconfig
>>> index b04f6e4aedbc..2f282b472912 100644
>>> --- a/drivers/soc/amlogic/Kconfig
>>> +++ b/drivers/soc/amlogic/Kconfig
>>> @@ -1,5 +1,12 @@
>>>  menu "Amlogic SoC drivers"
>>>
>>> +config MESON_CANVAS
>>> +    tristate "Amlogic Meson Canvas driver"
>>> +    depends on ARCH_MESON || COMPILE_TEST
>>> +    default n
>>> +    help
>>> +      Say yes to support the canvas IP for Amlogic SoCs.
>>> +
>>>  config MESON_GX_SOCINFO
>>>      bool "Amlogic Meson GX SoC Information driver"
>>>      depends on ARCH_MESON || COMPILE_TEST
>>> diff --git a/drivers/soc/amlogic/Makefile b/drivers/soc/amlogic/Makefile
>>> index 8fa321893928..0ab16d35ac36 100644
>>> --- a/drivers/soc/amlogic/Makefile
>>> +++ b/drivers/soc/amlogic/Makefile
>>> @@ -1,3 +1,4 @@
>>> +obj-$(CONFIG_MESON_CANVAS) += meson-canvas.o
>>>  obj-$(CONFIG_MESON_GX_SOCINFO) += meson-gx-socinfo.o
>>>  obj-$(CONFIG_MESON_GX_PM_DOMAINS) += meson-gx-pwrc-vpu.o
>>>  obj-$(CONFIG_MESON_MX_SOCINFO) += meson-mx-socinfo.o
>>> diff --git a/drivers/soc/amlogic/meson-canvas.c b/drivers/soc/amlogic/meson-canvas.c
>>> new file mode 100644
>>> index 000000000000..c461334b36d4
>>> --- /dev/null
>>> +++ b/drivers/soc/amlogic/meson-canvas.c
>>> @@ -0,0 +1,185 @@
>>> +// SPDX-License-Identifier: GPL-2.0+
>>> +/*
>>> + * Copyright (C) 2018 Maxime Jourdan <maxi.jourdan@wanadoo.fr>
>>> + * Copyright (C) 2016 BayLibre, SAS
>>> + * Author: Neil Armstrong <narmstrong@baylibre.com>
>>> + * Copyright (C) 2015 Amlogic, Inc. All rights reserved.
>>> + * Copyright (C) 2014 Endless Mobile
>>> + */
>>> +
>>> +#include <linux/kernel.h>
>>> +#include <linux/mfd/syscon.h>
>>> +#include <linux/module.h>
>>> +#include <linux/regmap.h>
>>> +#include <linux/soc/amlogic/meson-canvas.h>
>>> +#include <linux/of_address.h>
>>> +#include <linux/of_platform.h>
>>> +#include <linux/io.h>
>>> +
>>> +#define NUM_CANVAS 256
>>> +
>>> +/* DMC Registers */
>>> +#define DMC_CAV_LUT_DATAL   0x48 /* 0x12 offset in data sheet */
>>> +    #define CANVAS_WIDTH_LBIT       29
>>> +    #define CANVAS_WIDTH_LWID       3
>>> +#define DMC_CAV_LUT_DATAH   0x4c /* 0x13 offset in data sheet */
>>> +    #define CANVAS_WIDTH_HBIT       0
>>> +    #define CANVAS_HEIGHT_BIT       9
>>> +    #define CANVAS_BLKMODE_BIT      24
>>> +#define DMC_CAV_LUT_ADDR    0x50 /* 0x14 offset in data sheet */
>>> +    #define CANVAS_LUT_WR_EN        (0x2 << 8)
>>> +    #define CANVAS_LUT_RD_EN        (0x1 << 8)
>>> +
>>> +struct meson_canvas {
>>> +    struct device *dev;
>>> +    struct regmap *regmap_dmc;
>>> +    spinlock_t lock; /* canvas device lock */
>>> +    u8 used[NUM_CANVAS];
>>> +};
>>> +
>>> +struct meson_canvas *meson_canvas_get(struct device *dev)
>>> +{
>>> +    struct device_node *canvas_node;
>>> +    struct platform_device *canvas_pdev;
>>> +    struct meson_canvas *canvas;
>>> +
>>> +    canvas_node = of_parse_phandle(dev->of_node, "amlogic,canvas", 0);
>>> +    if (!canvas_node)
>>> +            return ERR_PTR(-ENODEV);
>>> +
>>> +    canvas_pdev = of_find_device_by_node(canvas_node);
>>> +    if (!canvas_pdev) {
>>> +            dev_err(dev, "Unable to find canvas pdev\n");
>>> +            return ERR_PTR(-ENODEV);
>>> +    }
>
> I should be -EPROBE_DEFER here since the canvas driver maybe hasn't probed yet.

Ack.

>>> +
>>> +    canvas = dev_get_drvdata(&canvas_pdev->dev);
>>> +    if (!canvas)
>>> +            return ERR_PTR(-ENODEV);
>>
>> You've got your device at this point, maybe EINVAL instead ?
>
> This shouldn't fail here... I'm not sure what should be the error.

I wasn't sure if it was possible to retrieve the pdev with a failed probe.

Should I just assume canvas will never be NULL here then ?

>>
>>> +
>>> +    return canvas;
>>> +}
>>> +EXPORT_SYMBOL_GPL(meson_canvas_get);
>>> +
>>> +int meson_canvas_setup(struct meson_canvas *canvas, u8 canvas_index,
>>> +                   u32 addr, u32 stride, u32 height,
>>> +                   unsigned int wrap,
>>> +                   unsigned int blkmode,
>>> +                   unsigned int endian)
>>> +{
>>> +    struct regmap *regmap = canvas->regmap_dmc;
>>> +    unsigned long flags;
>>> +    u32 val;
>>> +
>>> +    spin_lock_irqsave(&canvas->lock, flags);
>>> +    if (!canvas->used[canvas_index]) {
>>> +            dev_err(canvas->dev,
>>> +                    "Trying to setup non allocated canvas %u\n",
>>> +                    canvas_index);
>>> +            spin_unlock_irqrestore(&canvas->lock, flags);
>>> +            return -EINVAL;
>>> +    }
>>> +
>>> +    regmap_write(regmap, DMC_CAV_LUT_DATAL,
>>> +                 ((addr + 7) >> 3) |
>>> +                 (((stride + 7) >> 3) << CANVAS_WIDTH_LBIT));
>>> +
>>> +    regmap_write(regmap, DMC_CAV_LUT_DATAH,
>>> +                 ((((stride + 7) >> 3) >> CANVAS_WIDTH_LWID) <<
>>> +                                            CANVAS_WIDTH_HBIT) |
>>> +                 (height << CANVAS_HEIGHT_BIT) |
>>> +                 (wrap << 22) |
>>> +                 (blkmode << CANVAS_BLKMODE_BIT) |
>>> +                 (endian << 26));
>>> +
>>> +    regmap_write(regmap, DMC_CAV_LUT_ADDR,
>>> +                 CANVAS_LUT_WR_EN | canvas_index);
>>> +
>>> +    /* Force a read-back to make sure everything is flushed. */
>>> +    regmap_read(regmap, DMC_CAV_LUT_DATAH, &val);
>>
>> I suppose this something you got from the vendor kernel ? Out of curiosity, have
>> you tried w/o it ? I am wondering if this is really necessary ?
>
>
> It's explicit in the Amlogic source, you need a readback to make sure it's taken in account.
>
>>
>>
>>> +    spin_unlock_irqrestore(&canvas->lock, flags);
>>> +
>>> +    return 0;
>>> +}
>>> +EXPORT_SYMBOL_GPL(meson_canvas_setup);
>>> +
>>> +int meson_canvas_alloc(struct meson_canvas *canvas, u8 *canvas_index)
>>> +{
>>> +    int i;
>>> +    unsigned long flags;
>>> +
>>> +    spin_lock_irqsave(&canvas->lock, flags);
>>> +    for (i = 0; i < NUM_CANVAS; ++i) {
>>> +            if (!canvas->used[i]) {
>>> +                    canvas->used[i] = 1;
>>> +                    spin_unlock_irqrestore(&canvas->lock, flags);
>>> +                    *canvas_index = i;
>>> +                    return 0;
>>> +            }
>>> +    }
>>> +    spin_unlock_irqrestore(&canvas->lock, flags);
>>> +
>>> +    dev_err(canvas->dev, "No more canvas available\n");
>>> +    return -ENODEV;
>>> +}
>>> +EXPORT_SYMBOL_GPL(meson_canvas_alloc);
>>> +
>>> +int meson_canvas_free(struct meson_canvas *canvas, u8 canvas_index)
>>> +{
>>> +    unsigned long flags;
>>> +
>>> +    spin_lock_irqsave(&canvas->lock, flags);
>>> +    if (!canvas->used[canvas_index]) {
>>> +            dev_err(canvas->dev,
>>> +                    "Trying to free unused canvas %u\n", canvas_index);
>>> +            spin_unlock_irqrestore(&canvas->lock, flags);
>>> +            return -EINVAL;
>>> +    }
>>> +    canvas->used[canvas_index] = 0;
>>> +    spin_unlock_irqrestore(&canvas->lock, flags);
>>> +
>>> +    return 0;
>>> +}
>>> +EXPORT_SYMBOL_GPL(meson_canvas_free);
>>> +
>>> +static int meson_canvas_probe(struct platform_device *pdev)
>>> +{
>>> +    struct device *dev = &pdev->dev;
>>> +    struct meson_canvas *canvas;
>>> +
>>> +    canvas = devm_kzalloc(dev, sizeof(*canvas), GFP_KERNEL);
>>> +    if (!canvas)
>>> +            return -ENOMEM;
>>> +
>>> +    canvas->regmap_dmc =
>>> +            syscon_node_to_regmap(of_get_parent(dev->of_node));
>>> +    if (IS_ERR(canvas->regmap_dmc)) {
>>> +            dev_err(dev, "failed to get DMC regmap\n");
>>> +            return PTR_ERR(canvas->regmap_dmc);
>>> +    }
>>> +
>>> +    canvas->dev = dev;
>>> +    spin_lock_init(&canvas->lock);
>>> +    dev_set_drvdata(dev, canvas);
>>> +
>>> +    return 0;
>>> +}
>>> +
>>> +static const struct of_device_id canvas_dt_match[] = {
>>> +    { .compatible = "amlogic,canvas" },
>>> +    {}
>>> +};
>>> +MODULE_DEVICE_TABLE(of, canvas_dt_match);
>>> +
>>> +static struct platform_driver meson_canvas_driver = {
>>> +    .probe = meson_canvas_probe,
>>> +    .driver = {
>>> +            .name = "amlogic-canvas",
>>> +            .of_match_table = canvas_dt_match,
>>> +    },
>>> +};
>>> +module_platform_driver(meson_canvas_driver);
>>> +
>>> +MODULE_DESCRIPTION("Amlogic Canvas driver");
>>> +MODULE_AUTHOR("Maxime Jourdan <maxi.jourdan@wanadoo.fr>");
>>> +MODULE_LICENSE("GPL");
>>> diff --git a/include/linux/soc/amlogic/meson-canvas.h b/include/linux/soc/amlogic/meson-canvas.h
>>> new file mode 100644
>>> index 000000000000..c164202d8e39
>>> --- /dev/null
>>> +++ b/include/linux/soc/amlogic/meson-canvas.h
>>> @@ -0,0 +1,65 @@
>>> +/* SPDX-License-Identifier: GPL-2.0+ */
>>> +/*
>>> + * Copyright (C) 2018 Maxime Jourdan <maxi.jourdan@wanadoo.fr>
>>> + */
>>> +#ifndef MESON_CANVAS_H
>>> +#define MESON_CANVAS_H
>>> +
>>> +#include <linux/kernel.h>
>>> +
>>> +#define MESON_CANVAS_WRAP_NONE      0x00
>>> +#define MESON_CANVAS_WRAP_X 0x01
>>> +#define MESON_CANVAS_WRAP_Y 0x02
>>> +
>>> +#define MESON_CANVAS_BLKMODE_LINEAR 0x00
>>> +#define MESON_CANVAS_BLKMODE_32x32  0x01
>>> +#define MESON_CANVAS_BLKMODE_64x64  0x02
>>> +
>>> +#define MESON_CANVAS_ENDIAN_SWAP16  0x1
>>> +#define MESON_CANVAS_ENDIAN_SWAP32  0x3
>>> +#define MESON_CANVAS_ENDIAN_SWAP64  0x7
>>> +#define MESON_CANVAS_ENDIAN_SWAP128 0xf
>>> +
>>> +struct meson_canvas;
>>> +
>>> +/**
>>> + * meson_canvas_get() - get a canvas provider instance
>>> + *
>>> + * @dev: your device
>>
>> s/your device/consumer device pointer ?
>>

Ack.

>>> + */
>>> +struct meson_canvas *meson_canvas_get(struct device *dev);
>>> +
>>> +/**
>>> + * meson_canvas_alloc() - take ownership of a canvas
>>> + *
>>> + * @canvas: canvas provider instance retrieved from meson_canvas_get()
>>> + * @canvas_index: will be filled with the canvas ID
>>> + */
>>> +int meson_canvas_alloc(struct meson_canvas *canvas, u8 *canvas_index);
>>> +
>>> +/**
>>> + * meson_canvas_free() - remove ownership from a canvas
>>> + *
>>> + * @canvas: canvas provider instance retrieved from meson_canvas_get()
>>> + * @canvas_index: canvas ID that was obtained via meson_canvas_alloc()
>>> + */
>>> +int meson_canvas_free(struct meson_canvas *canvas, u8 canvas_index);
>>> +
>>> +/**
>>> + * meson_canvas_setup() - configure a canvas
>>> + *
>>> + * @canvas: canvas provider instance retrieved from meson_canvas_get()
>>> + * @canvas_index: canvas ID that was obtained via meson_canvas_alloc()
>>> + * @addr: physical address to the pixel buffer
>>> + * @stride: width of the buffer
>>> + * @height: height of the buffer
>>> + * @wrap: undocumented
>>> + * @blkmode: block mode (linear, 32x32, 64x64)
>>> + * @endian: byte swapping (swap16, swap32, swap64, swap128)
>>> + */
>>> +int meson_canvas_setup(struct meson_canvas *canvas, u8 canvas_index,
>>> +                   u32 addr, u32 stride, u32 height,
>>> +                   unsigned int wrap, unsigned int blkmode,
>>> +                   unsigned int endian);
>>> +
>>> +#endif
>>
>>
>
>
> _______________________________________________
> linux-amlogic mailing list
> linux-amlogic@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-amlogic

Maxime

Re: [PATCH v2 3/4] ARM64: dts: meson-gx: add dmcbus and canvas nodes.

[Maxime Jourdan]

2018-08-09 9:53 GMT+02:00 Neil Armstrong <narmstrong@baylibre.com>:
> On 08/08/2018 10:41, Jerome Brunet wrote:
>> On Wed, 2018-08-08 at 00:00 +0200, Maxime Jourdan wrote:
>>> Wrap the canvas node in a syscon node.
>>>
>>> Signed-off-by: Maxime Jourdan <maxi.jourdan@wanadoo.fr>
>>> ---
>>>  arch/arm64/boot/dts/amlogic/meson-gx.dtsi | 17 +++++++++++++++++
>>>  1 file changed, 17 insertions(+)
>>>
>>> diff --git a/arch/arm64/boot/dts/amlogic/meson-gx.dtsi b/arch/arm64/boot/dts/amlogic/meson-gx.dtsi
>>> index b8dc4dbb391b..c98198662ae2 100644
>>> --- a/arch/arm64/boot/dts/amlogic/meson-gx.dtsi
>>> +++ b/arch/arm64/boot/dts/amlogic/meson-gx.dtsi
>>> @@ -423,6 +423,23 @@
>>>                      };
>>>              };
>>>
>>> +            dmcbus: bus@c8838000 {
>>> +                    compatible = "simple-bus";
>>> +                    reg = <0x0 0xc8838000 0x0 0x1000>;
>>> +                    #address-cells = <2>;
>>> +                    #size-cells = <2>;
>>> +                    ranges = <0x0 0x0 0x0 0xc8838000 0x0 0x1000>;
>>> +
>>> +                    sysctrl_DMC: system-controller@0 {
>>> +                            compatible = "amlogic,gx-dmc-sysctrl", "syscon", "simple-mfd";
>>> +                            reg = <0x0 0x0 0x0 0x1000>;
>>> +
>>> +                            canvas: canvas-provider@0 {
>>> +                                    compatible = "amlogic,canvas";
>>
>> If there is only one canvas provider under "sysctrl_DMC" and it has no reg
>> property , you should not put a unit-address (@0) here. (same for the
>> documentation patch)
>>
>> You may have seen unit-address without a reg property used elsewhere (ASoC
>> simple-card, my recent axg-sound-card), when there is multiple node with the
>> same node-name (ex: dai-link).

Ack.

>> As Martin pointed out, the DT spec says we should not use unit-address unless
>> there is a reg property. We did not get Rob's view on this and we might have to
>> update this later on. In your case, unless I missed something, you should
>> definitely not have it
>>
>> nitpick regarding the node-name (canvas-provider). If appropriate, we should try
>> to stick to one of the generic names proposed in the spec. I wonder if the
>> canvas provider could be viewed as a "memory" or "memory-controller"
>
> It's not really a memory or memory-controller, it's a Lookup Table on top of the memory controller,
> nothing really matches here...

Thanks for the comments. It will be "video-lut" as suggested.

>>
>> So, what about this ? Just a proposition, feel free to comment ;)
>>
>>       sysctrl_DMC: system-controller@0 {                                                      compatib
>> le = "amlogic,gx-dmc-sysctrl", "syscon", "simple-mfd";>               reg =
>> <0x0 0x0 0x0 0x1000>;
>>
>>               canvas: memory-controller {
>>                       compatible = "amlogic,canvas";
>>               }
>>
>> [...]
>>
>>
>>> +                            };
>>> +                    };
>>> +            };
>>> +
>>>              hiubus: bus@c883c000 {
>>>                      compatible = "simple-bus";
>>>                      reg = <0x0 0xc883c000 0x0 0x2000>;
>>
>>
>

Re: [PATCH v2 2/4] dt-bindings: soc: amlogic: add meson-canvas documentation

[Rob Herring]

On Wed, Aug 08, 2018 at 12:00:09AM +0200, Maxime Jourdan wrote:
> DT bindings doc for amlogic,meson-canvas
> 
> Signed-off-by: Maxime Jourdan <maxi.jourdan@wanadoo.fr>
> ---
>  .../soc/amlogic/amlogic,meson-canvas.txt      | 36 +++++++++++++++++++
>  1 file changed, 36 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/soc/amlogic/amlogic,meson-canvas.txt
> 
> diff --git a/Documentation/devicetree/bindings/soc/amlogic/amlogic,meson-canvas.txt b/Documentation/devicetree/bindings/soc/amlogic/amlogic,meson-canvas.txt
> new file mode 100644
> index 000000000000..5f0351717bee
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/soc/amlogic/amlogic,meson-canvas.txt
> @@ -0,0 +1,36 @@
> +Amlogic Canvas
> +================================
> +
> +A canvas is a collection of metadata that describes a pixel buffer.
> +Those metadata include: width, height, phyaddr, wrapping, block mode
> +and endianness.
> +
> +Many IPs within Amlogic SoCs rely on canvas indexes to read/write pixel data
> +rather than use the phy addresses directly. For instance, this is the case for
> +the video decoders and the display.
> +
> +Amlogic SoCs have 256 canvas.
> +
> +Device Tree Bindings:
> +---------------------
> +
> +Canvas Provider
> +--------------------------
> +
> +Required properties:
> +- compatible: "amlogic,canvas"
> +
> +Parent node should have the following properties :
> +- compatible: "amlogic,gx-dmc-sysctrl", "syscon", "simple-mfd"

Is this documented somewhere? One child function is not a reason for an 
MFD and child nodes. And child nodes like this with no resources are 
unnecessary.

> +- reg: base address and size of the DMC system control register space.
> +
> +Example:
> +
> +sysctrl_DMC: system-controller@0 {
> +	compatible = "amlogic,gx-dmc-sysctrl", "syscon", "simple-mfd";
> +	reg = <0x0 0x0 0x0 0x1000>;
> +
> +	canvas: canvas-provider@0 {
> +		compatible = "amlogic,canvas";
> +	};
> +};
> -- 
> 2.18.0
> 
> --
> To unsubscribe from this list: send the line "unsubscribe devicetree" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 2/4] dt-bindings: soc: amlogic: add meson-canvas documentation

[Maxime Jourdan]

2018-08-13 21:07 GMT+02:00 Rob Herring <robh@kernel.org>:
> On Wed, Aug 08, 2018 at 12:00:09AM +0200, Maxime Jourdan wrote:
>> DT bindings doc for amlogic,meson-canvas
>>
>> Signed-off-by: Maxime Jourdan <maxi.jourdan@wanadoo.fr>
>> ---
>>  .../soc/amlogic/amlogic,meson-canvas.txt      | 36 +++++++++++++++++++
>>  1 file changed, 36 insertions(+)
>>  create mode 100644 Documentation/devicetree/bindings/soc/amlogic/amlogic,meson-canvas.txt
>>
>> diff --git a/Documentation/devicetree/bindings/soc/amlogic/amlogic,meson-canvas.txt b/Documentation/devicetree/bindings/soc/amlogic/amlogic,meson-canvas.txt
>> new file mode 100644
>> index 000000000000..5f0351717bee
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/soc/amlogic/amlogic,meson-canvas.txt
>> @@ -0,0 +1,36 @@
>> +Amlogic Canvas
>> +================================
>> +
>> +A canvas is a collection of metadata that describes a pixel buffer.
>> +Those metadata include: width, height, phyaddr, wrapping, block mode
>> +and endianness.
>> +
>> +Many IPs within Amlogic SoCs rely on canvas indexes to read/write pixel data
>> +rather than use the phy addresses directly. For instance, this is the case for
>> +the video decoders and the display.
>> +
>> +Amlogic SoCs have 256 canvas.
>> +
>> +Device Tree Bindings:
>> +---------------------
>> +
>> +Canvas Provider
>> +--------------------------
>> +
>> +Required properties:
>> +- compatible: "amlogic,canvas"
>> +
>> +Parent node should have the following properties :
>> +- compatible: "amlogic,gx-dmc-sysctrl", "syscon", "simple-mfd"
>
> Is this documented somewhere? One child function is not a reason for an
> MFD and child nodes. And child nodes like this with no resources are
> unnecessary.
>

Hi Rob, this was done to follow the same path as other buses on the
platform that have sysctrls in order to provide regmaps to the
devices.

I can see how it's not really necessary here though, would it be okay
with you if I turned "canvas" into a simple bus subnode, using __iomem
in the device ?

>> +- reg: base address and size of the DMC system control register space.
>> +
>> +Example:
>> +
>> +sysctrl_DMC: system-controller@0 {
>> +     compatible = "amlogic,gx-dmc-sysctrl", "syscon", "simple-mfd";
>> +     reg = <0x0 0x0 0x0 0x1000>;
>> +
>> +     canvas: canvas-provider@0 {
>> +             compatible = "amlogic,canvas";
>> +     };
>> +};
>> --
>> 2.18.0
>>
>> --
>> To unsubscribe from this list: send the line "unsubscribe devicetree" in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 2/4] dt-bindings: soc: amlogic: add meson-canvas documentation

[Rob Herring]

On Mon, Aug 13, 2018 at 8:42 PM Maxime Jourdan <maxi.jourdan@wanadoo.fr> wrote:
>
> 2018-08-13 21:07 GMT+02:00 Rob Herring <robh@kernel.org>:
> > On Wed, Aug 08, 2018 at 12:00:09AM +0200, Maxime Jourdan wrote:
> >> DT bindings doc for amlogic,meson-canvas
> >>
> >> Signed-off-by: Maxime Jourdan <maxi.jourdan@wanadoo.fr>
> >> ---
> >>  .../soc/amlogic/amlogic,meson-canvas.txt      | 36 +++++++++++++++++++
> >>  1 file changed, 36 insertions(+)
> >>  create mode 100644 Documentation/devicetree/bindings/soc/amlogic/amlogic,meson-canvas.txt
> >>
> >> diff --git a/Documentation/devicetree/bindings/soc/amlogic/amlogic,meson-canvas.txt b/Documentation/devicetree/bindings/soc/amlogic/amlogic,meson-canvas.txt
> >> new file mode 100644
> >> index 000000000000..5f0351717bee
> >> --- /dev/null
> >> +++ b/Documentation/devicetree/bindings/soc/amlogic/amlogic,meson-canvas.txt
> >> @@ -0,0 +1,36 @@
> >> +Amlogic Canvas
> >> +================================
> >> +
> >> +A canvas is a collection of metadata that describes a pixel buffer.
> >> +Those metadata include: width, height, phyaddr, wrapping, block mode
> >> +and endianness.
> >> +
> >> +Many IPs within Amlogic SoCs rely on canvas indexes to read/write pixel data
> >> +rather than use the phy addresses directly. For instance, this is the case for
> >> +the video decoders and the display.
> >> +
> >> +Amlogic SoCs have 256 canvas.
> >> +
> >> +Device Tree Bindings:
> >> +---------------------
> >> +
> >> +Canvas Provider
> >> +--------------------------
> >> +
> >> +Required properties:
> >> +- compatible: "amlogic,canvas"
> >> +
> >> +Parent node should have the following properties :
> >> +- compatible: "amlogic,gx-dmc-sysctrl", "syscon", "simple-mfd"
> >
> > Is this documented somewhere? One child function is not a reason for an
> > MFD and child nodes. And child nodes like this with no resources are
> > unnecessary.
> >
>
> Hi Rob, this was done to follow the same path as other buses on the
> platform that have sysctrls in order to provide regmaps to the
> devices.
>
> I can see how it's not really necessary here though, would it be okay
> with you if I turned "canvas" into a simple bus subnode, using __iomem
> in the device ?

That's a driver issue that has little to do with the binding. You can
create a regmap for any node, "syscon" just does that automagically
for you.

Rob