[RFC 0/9] drm: rcar-du: Add CMM support to M3-W (plumbing only)

[RFC 0/9] drm: rcar-du: Add CMM support to M3-W (plumbing only)

[Jacopo Mondi]

Hello,
   this patches add the plumbing to integrate CMM support in Renesas R-Car
Display Unit.

CMM unit performs color correction and image enhancement on the DU channels
output pixels, before they get displayed through the connector they attach to.

Support for CMM has been added in Renesas BSP v4.19 and re-sent a few weeks ago
to upstream in the series: "[PATCH 0/8] v4.19.0 Added Color Management Module"
in a form not consumable by mainline.

In order to ease support for implementing CMM support in the mainline DU driver
I took care with this series of addressing the plumbing between DU and CMM,
providing support in device tree and core DU driver.

I'm keeping the submission internal to the renesas-soc list and I'm not
including yet DT and DRM people as the scope of the series is limited and should
not be considered for inclusion, but I would like to start collecting feedbacks.

The series is in RFC as:
- The CMM driver is actually empty. Both BSP and Bosh have working
  implementations of CMM driver which actually does something (I assume so, at
  least). Once the here proposed design is accepted, we can start
  discussing how better expose and control the tables used by the CMM to
  perform color correction.

- It only supports M3-W, mostly because that's what I'm developing on.
  Once the here proposed design is accepted, a patch to add the CMM clock
  definitions will be required for each SoC with CMM support. The same applies
  to device tree updating and DU feature expansion.

I have verified the DU output is still operational with CMM enabled, but again,
this is not the real target of the series, which mostly addresses the necessary
plumbing to have CMM integrated in the DU driver.

Knowing very few things about DRM/KMS I welcome suggestions on how to expose
the CMM tables and controls with DRM/KMS properties, which in my opinion should
be done on top of this series.

Not-Yet-Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>

Thanks
   j

Jacopo Mondi (9):
  dt-bindings: display: renesas,cmm: Add R-Car CMM documentation
  dt-bindings: display, renesas,du: Document cmms property
  [TODO] drm: rcar-du: Add basic support for CMM
  drm: rcar-du: kms: Create CMM instances
  drm: rcar-du: Add CMM support for M3-W
  drm: rcar-du: crtc: Setup the CMM
  drm: rcar-du: group: Enable CMM unit
  clk: renesas: r8a7796: Add CMM clocks
  arm64: dts: renesas: r8a7796: Add CMM units

 .../bindings/display/renesas,cmm.txt          | 24 ++++++
 .../bindings/display/renesas,du.txt           |  4 +
 arch/arm64/boot/dts/renesas/r8a7796.dtsi      | 25 ++++++
 drivers/clk/renesas/r8a7796-cpg-mssr.c        |  3 +
 drivers/gpu/drm/rcar-du/Kconfig               |  7 ++
 drivers/gpu/drm/rcar-du/Makefile              |  1 +
 drivers/gpu/drm/rcar-du/rcar_du_cmm.c         | 78 +++++++++++++++++++
 drivers/gpu/drm/rcar-du/rcar_du_cmm.h         | 23 ++++++
 drivers/gpu/drm/rcar-du/rcar_du_crtc.c        | 38 ++++++++-
 drivers/gpu/drm/rcar-du/rcar_du_crtc.h        |  2 +
 drivers/gpu/drm/rcar-du/rcar_du_drv.c         |  3 +-
 drivers/gpu/drm/rcar-du/rcar_du_drv.h         |  4 +
 drivers/gpu/drm/rcar-du/rcar_du_group.c       |  8 ++
 drivers/gpu/drm/rcar-du/rcar_du_group.h       |  2 +
 drivers/gpu/drm/rcar-du/rcar_du_kms.c         | 68 ++++++++++++++++
 drivers/gpu/drm/rcar-du/rcar_du_regs.h        |  5 ++
 16 files changed, 292 insertions(+), 3 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/display/renesas,cmm.txt
 create mode 100644 drivers/gpu/drm/rcar-du/rcar_du_cmm.c
 create mode 100644 drivers/gpu/drm/rcar-du/rcar_du_cmm.h

--
2.21.0

[RFC 1/9] dt-bindings: display: renesas,cmm: Add R-Car CMM documentation

[Jacopo Mondi]

Add device tree bindings documentation for the Renesas R-Car Display
Unit Color Management Module.

CMM is the image enhancement module available on each R-Car DU video
channel.

Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
---
 .../bindings/display/renesas,cmm.txt          | 24 +++++++++++++++++++
 1 file changed, 24 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/display/renesas,cmm.txt

diff --git a/Documentation/devicetree/bindings/display/renesas,cmm.txt b/Documentation/devicetree/bindings/display/renesas,cmm.txt
new file mode 100644
index 000000000000..d7674417edb4
--- /dev/null
+++ b/Documentation/devicetree/bindings/display/renesas,cmm.txt
@@ -0,0 +1,24 @@
+* Renesas R-Car Color Management Unit (CMM)
+
+Renesas R-Car image enhancement module connected to R-Car DU video channels.
+
+Required properties:
+ - compatible: shall be:
+   - "renesas,cmm"
+
+ - reg: the address base and length of the memory area where CMM control
+   registers are mapped to.
+
+ - clocks: phandle and clock-specifier pair to the CMM functional clock
+   supplier.
+
+Example:
+--------
+
+	cmm0: cmm@fea40000 {
+		compatible = "renesas,cmm";
+		reg = <0 0xfea40000 0 0x1000>;
+		power-domains = <&sysc R8A7796_PD_ALWAYS_ON>;
+		clocks = <&cpg CPG_MOD 711>;
+		resets = <&cpg 711>;
+	};
-- 
2.21.0

[RFC 2/9] dt-bindings: display, renesas,du: Document cmms property

[Jacopo Mondi]

Document the newly added 'cmms' property which accepts a list of phandle
and channel index pairs that point to the CMM units available for each
Display Unit output video channel.

Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
---
 Documentation/devicetree/bindings/display/renesas,du.txt | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/Documentation/devicetree/bindings/display/renesas,du.txt b/Documentation/devicetree/bindings/display/renesas,du.txt
index aedb22b4d161..2ccf78bf9a18 100644
--- a/Documentation/devicetree/bindings/display/renesas,du.txt
+++ b/Documentation/devicetree/bindings/display/renesas,du.txt
@@ -44,6 +44,10 @@ Required Properties:
     instance that serves the DU channel, and the channel index identifies the
     LIF instance in that VSP.
 
+  - cmms: A list of phandle and channel index tuples to the CMM modules
+    connected to DU channels. The phandle identifies the CMM instance that
+    serves the DU channel identified by the index.
+
 Required nodes:
 
 The connections to the DU output video ports are modeled using the OF graph
-- 
2.21.0

[RFC 3/9] [TODO] drm: rcar-du: Add basic support for CMM

[Jacopo Mondi]

Add a driver skeleton for the R-Car Display Unit Color Correction
Module.

Each DU output channel is provided with a CMM unit to perform image
enhancement and color correction.

Add support for CMM through a skeleton driver to be later expanded to
support setting color correction tables through the DRM/KMS properties
framework.

As of now, the driver is only useful to demonstrate the integration with
the DU driver.

Not-Yet-Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
---
 drivers/gpu/drm/rcar-du/Kconfig       |  7 +++
 drivers/gpu/drm/rcar-du/Makefile      |  1 +
 drivers/gpu/drm/rcar-du/rcar_du_cmm.c | 78 +++++++++++++++++++++++++++
 drivers/gpu/drm/rcar-du/rcar_du_cmm.h | 23 ++++++++
 4 files changed, 109 insertions(+)
 create mode 100644 drivers/gpu/drm/rcar-du/rcar_du_cmm.c
 create mode 100644 drivers/gpu/drm/rcar-du/rcar_du_cmm.h

diff --git a/drivers/gpu/drm/rcar-du/Kconfig b/drivers/gpu/drm/rcar-du/Kconfig
index 1529849e217e..820f2b85a073 100644
--- a/drivers/gpu/drm/rcar-du/Kconfig
+++ b/drivers/gpu/drm/rcar-du/Kconfig
@@ -13,6 +13,13 @@ config DRM_RCAR_DU
 	  Choose this option if you have an R-Car chipset.
 	  If M is selected the module will be called rcar-du-drm.
 
+config DRM_RCAR_CMM
+	bool "R-Car DU Color Correction Module Support"
+	depends on DRM && OF
+	depends on DRM_RCAR_DU && ARCH_RCAR_GEN3
+	help
+	  Enable support for R-Car Gen3 Color Correction Module (CMM).
+
 config DRM_RCAR_DW_HDMI
 	tristate "R-Car DU Gen3 HDMI Encoder Support"
 	depends on DRM && OF
diff --git a/drivers/gpu/drm/rcar-du/Makefile b/drivers/gpu/drm/rcar-du/Makefile
index 6c2ed9c46467..ae26d465d421 100644
--- a/drivers/gpu/drm/rcar-du/Makefile
+++ b/drivers/gpu/drm/rcar-du/Makefile
@@ -15,6 +15,7 @@ rcar-du-drm-$(CONFIG_DRM_RCAR_LVDS)	+= rcar_du_of.o \
 rcar-du-drm-$(CONFIG_DRM_RCAR_VSP)	+= rcar_du_vsp.o
 rcar-du-drm-$(CONFIG_DRM_RCAR_WRITEBACK) += rcar_du_writeback.o
 
+obj-$(CONFIG_DRM_RCAR_CMM)		+= rcar_du_cmm.o
 obj-$(CONFIG_DRM_RCAR_DU)		+= rcar-du-drm.o
 obj-$(CONFIG_DRM_RCAR_DW_HDMI)		+= rcar_dw_hdmi.o
 obj-$(CONFIG_DRM_RCAR_LVDS)		+= rcar_lvds.o
diff --git a/drivers/gpu/drm/rcar-du/rcar_du_cmm.c b/drivers/gpu/drm/rcar-du/rcar_du_cmm.c
new file mode 100644
index 000000000000..63f545830bb9
--- /dev/null
+++ b/drivers/gpu/drm/rcar-du/rcar_du_cmm.c
@@ -0,0 +1,78 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * rcar_du_cmm.c -- R-Car Display Unit Color Management Module
+ *
+ * Copyright (C) 2019 Jacopo Mondi <jacopo+renesas@jmondi.org>
+ */
+
+#include <linux/clk.h>
+#include <linux/io.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/platform_device.h>
+
+#include "rcar_du_cmm.h"
+#include "rcar_du_crtc.h"
+
+struct rcar_cmm {
+	struct clk *clk;
+	void __iomem *base;
+};
+
+int rcar_du_cmm_setup(struct platform_device *pdev, struct rcar_du_crtc *rcrtc)
+{
+	struct rcar_cmm *rcmm = platform_get_drvdata(pdev);
+	int ret;
+
+	ret = clk_prepare_enable(rcmm->clk);
+	if (ret)
+		return ret;
+
+	return 0;
+}
+
+static int rcar_cmm_probe(struct platform_device *pdev)
+{
+	struct rcar_cmm *rcmm;
+	struct resource *res;
+
+	rcmm = devm_kzalloc(&pdev->dev, sizeof(*rcmm), GFP_KERNEL);
+	if (!rcmm)
+		return -ENOMEM;
+
+	platform_set_drvdata(pdev, rcmm);
+
+	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	rcmm->base = devm_ioremap_resource(&pdev->dev, res);
+	if (IS_ERR(rcmm->base))
+		return PTR_ERR(rcmm->base);
+
+	rcmm->clk = devm_clk_get(&pdev->dev, NULL);
+	if (IS_ERR(rcmm->clk)) {
+		dev_err(&pdev->dev, "Failed to get CMM clock");
+		return PTR_ERR(rcmm->clk);
+	}
+
+	return 0;
+}
+
+static const struct of_device_id rcar_cmm_of_table[] = {
+	{ .compatible = "renesas,cmm" },
+	{ },
+};
+
+MODULE_DEVICE_TABLE(of, rcar_cmm_of_table);
+
+static struct platform_driver rcar_cmm_platform_driver = {
+	.probe		= rcar_cmm_probe,
+	.driver		= {
+		.name	= "rcar-cmm",
+		.of_match_table = rcar_cmm_of_table,
+	},
+};
+
+module_platform_driver(rcar_cmm_platform_driver);
+
+MODULE_AUTHOR("Jacopo Mondi <jacopo+renesas@jmondi.org");
+MODULE_DESCRIPTION("Renesas R-Car CMM Driver");
+MODULE_LICENSE("GPL v2");
diff --git a/drivers/gpu/drm/rcar-du/rcar_du_cmm.h b/drivers/gpu/drm/rcar-du/rcar_du_cmm.h
new file mode 100644
index 000000000000..be9ac1a091b0
--- /dev/null
+++ b/drivers/gpu/drm/rcar-du/rcar_du_cmm.h
@@ -0,0 +1,23 @@
+/* SPDX-License-Identifier: GPL-2.0+ */
+/*
+ * rcar_du_cmm.c -- R-Car Display Unit Color Management Module
+ *
+ * Copyright (C) 2019 Jacopo Mondi <jacopo+renesas@jmondi.org>
+ */
+
+#ifndef __RCAR_DU_CMM_H_
+#define __RCAR_DU_CMM_H_
+
+#include <linux/of.h>
+#include <linux/platform_device.h>
+
+struct rcar_du_crtc;
+
+struct rcar_du_cmm {
+	struct device_node *np;
+	unsigned int crtc;
+};
+
+int rcar_du_cmm_setup(struct platform_device *, struct rcar_du_crtc *);
+
+#endif /* __RCAR_DU_CMM_H_ */
-- 
2.21.0

[RFC 4/9] drm: rcar-du: kms: Create CMM instances

[Jacopo Mondi]

Implement device tree parsing to collect the available CMM units and
store them in the group to be later enabled at start up time.

Define a new feature to let each SoC claim support for CMM.

Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
---
 drivers/gpu/drm/rcar-du/rcar_du_drv.h   |  4 ++
 drivers/gpu/drm/rcar-du/rcar_du_group.h |  2 +
 drivers/gpu/drm/rcar-du/rcar_du_kms.c   | 68 +++++++++++++++++++++++++
 3 files changed, 74 insertions(+)

diff --git a/drivers/gpu/drm/rcar-du/rcar_du_drv.h b/drivers/gpu/drm/rcar-du/rcar_du_drv.h
index 1327cd0df90a..aac916a7a46c 100644
--- a/drivers/gpu/drm/rcar-du/rcar_du_drv.h
+++ b/drivers/gpu/drm/rcar-du/rcar_du_drv.h
@@ -13,6 +13,7 @@
 #include <linux/kernel.h>
 #include <linux/wait.h>
 
+#include "rcar_du_cmm.h"
 #include "rcar_du_crtc.h"
 #include "rcar_du_group.h"
 #include "rcar_du_vsp.h"
@@ -28,6 +29,7 @@ struct rcar_du_encoder;
 #define RCAR_DU_FEATURE_VSP1_SOURCE	BIT(1)	/* Has inputs from VSP1 */
 #define RCAR_DU_FEATURE_INTERLACED	BIT(2)	/* HW supports interlaced */
 #define RCAR_DU_FEATURE_TVM_SYNC	BIT(3)	/* Has TV switch/sync modes */
+#define RCAR_DU_FEATURE_CMM		BIT(4)	/* Has CCM */
 
 #define RCAR_DU_QUIRK_ALIGN_128B	BIT(0)	/* Align pitches to 128 bytes */
 
@@ -69,6 +71,7 @@ struct rcar_du_device_info {
 
 #define RCAR_DU_MAX_CRTCS		4
 #define RCAR_DU_MAX_GROUPS		DIV_ROUND_UP(RCAR_DU_MAX_CRTCS, 2)
+#define RCAR_DU_MAX_CMMS		4
 #define RCAR_DU_MAX_VSPS		4
 
 struct rcar_du_device {
@@ -85,6 +88,7 @@ struct rcar_du_device {
 	struct rcar_du_encoder *encoders[RCAR_DU_OUTPUT_MAX];
 
 	struct rcar_du_group groups[RCAR_DU_MAX_GROUPS];
+	struct rcar_du_cmm cmms[RCAR_DU_MAX_CMMS];
 	struct rcar_du_vsp vsps[RCAR_DU_MAX_VSPS];
 
 	struct {
diff --git a/drivers/gpu/drm/rcar-du/rcar_du_group.h b/drivers/gpu/drm/rcar-du/rcar_du_group.h
index 87950c1f6a52..b0c1466593a3 100644
--- a/drivers/gpu/drm/rcar-du/rcar_du_group.h
+++ b/drivers/gpu/drm/rcar-du/rcar_du_group.h
@@ -22,6 +22,7 @@ struct rcar_du_device;
  * @mmio_offset: registers offset in the device memory map
  * @index: group index
  * @channels_mask: bitmask of populated DU channels in this group
+ * @cmms_mask: bitmask of enabled CMMs in this group
  * @num_crtcs: number of CRTCs in this group (1 or 2)
  * @use_count: number of users of the group (rcar_du_group_(get|put))
  * @used_crtcs: number of CRTCs currently in use
@@ -37,6 +38,7 @@ struct rcar_du_group {
 	unsigned int index;
 
 	unsigned int channels_mask;
+	unsigned int cmms_mask;
 	unsigned int num_crtcs;
 	unsigned int use_count;
 	unsigned int used_crtcs;
diff --git a/drivers/gpu/drm/rcar-du/rcar_du_kms.c b/drivers/gpu/drm/rcar-du/rcar_du_kms.c
index adbc4f5d8fc5..684a60feac5c 100644
--- a/drivers/gpu/drm/rcar-du/rcar_du_kms.c
+++ b/drivers/gpu/drm/rcar-du/rcar_du_kms.c
@@ -614,6 +614,64 @@ static int rcar_du_vsps_init(struct rcar_du_device *rcdu)
 	return ret;
 }
 
+static int rcar_du_cmm_init(struct rcar_du_device *rcdu)
+{
+	const struct device_node *np = rcdu->dev->of_node;
+	unsigned int cells;
+	unsigned int i;
+	int ret;
+
+	/*
+	 * Make sure the DT 'cmms' property is well formed and populate
+	 * the list of enabled CMM.
+	 */
+	cells = of_property_count_u32_elems(np, "cmms");
+	if (cells % 2 || cells > rcdu->num_crtcs * 2) {
+		dev_err(rcdu->dev, "invalid 'cmms' property format\n");
+		return -EINVAL;
+	}
+
+	for (i = 0; i < cells / 2; ++i) {
+		struct of_phandle_args args;
+		struct rcar_du_group *rgrp;
+		unsigned int crtc;
+
+		ret = of_parse_phandle_with_fixed_args(np, "cmms", 1, i,
+						       &args);
+		if (ret < 0) {
+			dev_err(rcdu->dev, "failed to parse 'cmms' property\n");
+			goto error;
+		}
+
+		crtc = args.args[0];
+		if (crtc >= rcdu->num_crtcs ||
+		    !(rcdu->info->channels_mask & BIT(crtc))) {
+			dev_err(rcdu->dev,
+				"invalid DU channel %u in 'cmms' property\n",
+				crtc);
+			goto error;
+		}
+
+		rcdu->cmms[i].np = args.np;
+		rcdu->cmms[i].crtc = crtc;
+
+		/*
+		 * CMMs are activated by the DU group: store the active CMMs
+		 * indexes in the group.
+		 */
+		rgrp = &rcdu->groups[crtc / 2];
+		rgrp->cmms_mask |= BIT(crtc % 2);
+	}
+
+	return 0;
+
+error:
+	for (; i > 0; --i)
+		of_node_put(rcdu->cmms[i - 1].np);
+
+	return ret;
+}
+
 int rcar_du_modeset_init(struct rcar_du_device *rcdu)
 {
 	static const unsigned int mmio_offsets[] = {
@@ -680,6 +738,9 @@ int rcar_du_modeset_init(struct rcar_du_device *rcdu)
 				   & GENMASK(1, 0);
 		rgrp->num_crtcs = hweight8(rgrp->channels_mask);
 
+		/* The active CMMs mask will be later populated. */
+		rgrp->cmms_mask = 0;
+
 		/*
 		 * If we have more than one CRTCs in this group pre-associate
 		 * the low-order planes with CRTC 0 and the high-order planes
@@ -704,6 +765,13 @@ int rcar_du_modeset_init(struct rcar_du_device *rcdu)
 			return ret;
 	}
 
+	/* Initialize the Color Management Modules. */
+	if (rcar_du_has(rcdu, RCAR_DU_FEATURE_CMM)) {
+		ret = rcar_du_cmm_init(rcdu);
+		if (ret < 0)
+			return ret;
+	}
+
 	/* Create the CRTCs. */
 	for (swindex = 0, hwindex = 0; swindex < rcdu->num_crtcs; ++hwindex) {
 		struct rcar_du_group *rgrp;
-- 
2.21.0

[RFC 5/9] drm: rcar-du: Add CMM support for M3-W

[Jacopo Mondi]

R-Car M3-W provides a CMM for each of it's DU output channel. Add CMM as
a supported feature for the SoC.

Temporary commit to be later expanded to all Gen3 SoCs V3H/M apart.

Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
---
 drivers/gpu/drm/rcar-du/rcar_du_drv.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/rcar-du/rcar_du_drv.c b/drivers/gpu/drm/rcar-du/rcar_du_drv.c
index 75ab17af13a9..984553342b1f 100644
--- a/drivers/gpu/drm/rcar-du/rcar_du_drv.c
+++ b/drivers/gpu/drm/rcar-du/rcar_du_drv.c
@@ -280,7 +280,8 @@ static const struct rcar_du_device_info rcar_du_r8a7796_info = {
 	.features = RCAR_DU_FEATURE_CRTC_IRQ_CLOCK
 		  | RCAR_DU_FEATURE_VSP1_SOURCE
 		  | RCAR_DU_FEATURE_INTERLACED
-		  | RCAR_DU_FEATURE_TVM_SYNC,
+		  | RCAR_DU_FEATURE_TVM_SYNC
+		  | RCAR_DU_FEATURE_CMM,
 	.channels_mask = BIT(2) | BIT(1) | BIT(0),
 	.routes = {
 		/*
-- 
2.21.0

[RFC 6/9] drm: rcar-du: crtc: Setup the CMM

[Jacopo Mondi]

Save a reference to the CMM unit associated with the CRTC and set it up
at rcar_du_crtc_setup() time, just after having enabled the compositor.

Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
---
 drivers/gpu/drm/rcar-du/rcar_du_crtc.c | 38 ++++++++++++++++++++++++--
 drivers/gpu/drm/rcar-du/rcar_du_crtc.h |  2 ++
 2 files changed, 38 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/rcar-du/rcar_du_crtc.c b/drivers/gpu/drm/rcar-du/rcar_du_crtc.c
index 2da46e3dc4ae..b5a9af8e4df0 100644
--- a/drivers/gpu/drm/rcar-du/rcar_du_crtc.c
+++ b/drivers/gpu/drm/rcar-du/rcar_du_crtc.c
@@ -9,6 +9,7 @@
 
 #include <linux/clk.h>
 #include <linux/mutex.h>
+#include <linux/of_platform.h>
 #include <linux/platform_device.h>
 #include <linux/sys_soc.h>
 
@@ -21,6 +22,7 @@
 #include <drm/drm_plane_helper.h>
 #include <drm/drm_vblank.h>
 
+#include "rcar_du_cmm.h"
 #include "rcar_du_crtc.h"
 #include "rcar_du_drv.h"
 #include "rcar_du_encoder.h"
@@ -478,7 +480,7 @@ static void rcar_du_crtc_wait_page_flip(struct rcar_du_crtc *rcrtc)
  * Start/Stop and Suspend/Resume
  */
 
-static void rcar_du_crtc_setup(struct rcar_du_crtc *rcrtc)
+static int rcar_du_crtc_setup(struct rcar_du_crtc *rcrtc)
 {
 	/* Set display off and background to black */
 	rcar_du_crtc_write(rcrtc, DOOR, DOOR_RGB(0, 0, 0));
@@ -495,8 +497,24 @@ static void rcar_du_crtc_setup(struct rcar_du_crtc *rcrtc)
 	if (rcar_du_has(rcrtc->dev, RCAR_DU_FEATURE_VSP1_SOURCE))
 		rcar_du_vsp_enable(rcrtc);
 
+	/* Enable CMM, if present. */
+	if (rcrtc->cmm) {
+		struct platform_device *pdev;
+		int ret;
+
+		pdev = of_find_device_by_node(rcrtc->cmm->np);
+		if (!pdev)
+			return PTR_ERR(pdev);
+
+		ret = rcar_du_cmm_setup(pdev, rcrtc);
+		if (ret)
+			return ret;
+	}
+
 	/* Turn vertical blanking interrupt reporting on. */
 	drm_crtc_vblank_on(&rcrtc->crtc);
+
+	return 0;
 }
 
 static int rcar_du_crtc_get(struct rcar_du_crtc *rcrtc)
@@ -522,7 +540,10 @@ static int rcar_du_crtc_get(struct rcar_du_crtc *rcrtc)
 	if (ret < 0)
 		goto error_group;
 
-	rcar_du_crtc_setup(rcrtc);
+	ret = rcar_du_crtc_setup(rcrtc);
+	if (ret < 0)
+		goto error_group;
+
 	rcrtc->initialized = true;
 
 	return 0;
@@ -1182,6 +1203,19 @@ int rcar_du_crtc_create(struct rcar_du_group *rgrp, unsigned int swindex,
 	rcrtc->index = hwindex;
 	rcrtc->dsysr = (rcrtc->index % 2 ? 0 : DSYSR_DRES) | DSYSR_TVM_TVSYNC;
 
+	if (rcar_du_has(rcdu, RCAR_DU_FEATURE_CMM)) {
+		unsigned int i;
+
+		for (i = 0; i < RCAR_DU_MAX_CMMS; ++i) {
+			struct rcar_du_cmm *cmm = &rcdu->cmms[i];
+
+			if (cmm->crtc != hwindex)
+				continue;
+
+			rcrtc->cmm = cmm;
+		}
+	}
+
 	if (rcar_du_has(rcdu, RCAR_DU_FEATURE_VSP1_SOURCE))
 		primary = &rcrtc->vsp->planes[rcrtc->vsp_pipe].plane;
 	else
diff --git a/drivers/gpu/drm/rcar-du/rcar_du_crtc.h b/drivers/gpu/drm/rcar-du/rcar_du_crtc.h
index 3b7fc668996f..8b31406bd11b 100644
--- a/drivers/gpu/drm/rcar-du/rcar_du_crtc.h
+++ b/drivers/gpu/drm/rcar-du/rcar_du_crtc.h
@@ -19,6 +19,7 @@
 
 #include <media/vsp1.h>
 
+struct rcar_du_cmm;
 struct rcar_du_group;
 struct rcar_du_vsp;
 
@@ -64,6 +65,7 @@ struct rcar_du_crtc {
 	unsigned int vblank_count;
 
 	struct rcar_du_group *group;
+	struct rcar_du_cmm *cmm;
 	struct rcar_du_vsp *vsp;
 	unsigned int vsp_pipe;
 
-- 
2.21.0

[RFC 7/9] drm: rcar-du: group: Enable CMM unit

[Jacopo Mondi]

Enable the CMM units present in the group through the display unit
extensional function control group register DEFR7.

Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
---
 drivers/gpu/drm/rcar-du/rcar_du_group.c | 8 ++++++++
 drivers/gpu/drm/rcar-du/rcar_du_regs.h  | 5 +++++
 2 files changed, 13 insertions(+)

diff --git a/drivers/gpu/drm/rcar-du/rcar_du_group.c b/drivers/gpu/drm/rcar-du/rcar_du_group.c
index 9eee47969e77..ce25e41b04bc 100644
--- a/drivers/gpu/drm/rcar-du/rcar_du_group.c
+++ b/drivers/gpu/drm/rcar-du/rcar_du_group.c
@@ -147,6 +147,14 @@ static void rcar_du_group_setup(struct rcar_du_group *rgrp)
 
 	rcar_du_group_setup_pins(rgrp);
 
+	if (rgrp->cmms_mask) {
+		u32 defr7 = DEFR7_CODE;
+		defr7 |= rgrp->cmms_mask & BIT(1) ? DEFR7_CMME1 : 0;
+		defr7 |= rgrp->cmms_mask & BIT(0) ? DEFR7_CMME0 : 0;
+
+		rcar_du_group_write(rgrp, DEFR7, defr7);
+	}
+
 	if (rcdu->info->gen >= 2) {
 		rcar_du_group_setup_defr8(rgrp);
 		rcar_du_group_setup_didsr(rgrp);
diff --git a/drivers/gpu/drm/rcar-du/rcar_du_regs.h b/drivers/gpu/drm/rcar-du/rcar_du_regs.h
index bc87f080b170..fb9964949368 100644
--- a/drivers/gpu/drm/rcar-du/rcar_du_regs.h
+++ b/drivers/gpu/drm/rcar-du/rcar_du_regs.h
@@ -197,6 +197,11 @@
 #define DEFR6_MLOS1		(1 << 2)
 #define DEFR6_DEFAULT		(DEFR6_CODE | DEFR6_TCNE1)
 
+#define DEFR7			0x000ec
+#define DEFR7_CODE		(0x7779 << 16)
+#define DEFR7_CMME1		BIT(6)
+#define DEFR7_CMME0		BIT(4)
+
 /* -----------------------------------------------------------------------------
  * R8A7790-only Control Registers
  */
-- 
2.21.0

[RFC 8/9] clk: renesas: r8a7796: Add CMM clocks

[Jacopo Mondi]

Add clock defintions for CMM units on Renesas R-Car Gen3 M3-W.

Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
---
 drivers/clk/renesas/r8a7796-cpg-mssr.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/clk/renesas/r8a7796-cpg-mssr.c b/drivers/clk/renesas/r8a7796-cpg-mssr.c
index 12c455859f2c..6044aeda0f83 100644
--- a/drivers/clk/renesas/r8a7796-cpg-mssr.c
+++ b/drivers/clk/renesas/r8a7796-cpg-mssr.c
@@ -179,6 +179,9 @@ static const struct mssr_mod_clk r8a7796_mod_clks[] __initconst = {
 	DEF_MOD("ehci1",		 702,	R8A7796_CLK_S3D4),
 	DEF_MOD("ehci0",		 703,	R8A7796_CLK_S3D4),
 	DEF_MOD("hsusb",		 704,	R8A7796_CLK_S3D4),
+	DEF_MOD("cmm2",			 709,	R8A7796_CLK_S2D1),
+	DEF_MOD("cmm1",			 710,	R8A7796_CLK_S2D1),
+	DEF_MOD("cmm0",			 711,	R8A7796_CLK_S2D1),
 	DEF_MOD("csi20",		 714,	R8A7796_CLK_CSI0),
 	DEF_MOD("csi40",		 716,	R8A7796_CLK_CSI0),
 	DEF_MOD("du2",			 722,	R8A7796_CLK_S2D1),
-- 
2.21.0

[RFC 9/9] arm64: dts: renesas: r8a7796: Add CMM units

[Jacopo Mondi]

Add CMM units to Renesas R-Car M3-W device tree and reference them from
the Display Unit they are connected to.

Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
---
 arch/arm64/boot/dts/renesas/r8a7796.dtsi | 25 ++++++++++++++++++++++++
 1 file changed, 25 insertions(+)

diff --git a/arch/arm64/boot/dts/renesas/r8a7796.dtsi b/arch/arm64/boot/dts/renesas/r8a7796.dtsi
index cdf784899cf8..b81c4711dce3 100644
--- a/arch/arm64/boot/dts/renesas/r8a7796.dtsi
+++ b/arch/arm64/boot/dts/renesas/r8a7796.dtsi
@@ -2503,6 +2503,30 @@
 			renesas,fcp = <&fcpf0>;
 		};
 
+		cmm0: cmm@fea40000 {
+			compatible = "renesas,cmm";
+			reg = <0 0xfea40000 0 0x1000>;
+			clocks = <&cpg CPG_MOD 711>;
+			power-domains = <&sysc R8A7796_PD_ALWAYS_ON>;
+			resets = <&cpg 711>;
+		};
+
+		cmm1: cmm@fea50000 {
+			compatible = "renesas,cmm";
+			reg = <0 0xfea50000 0 0x1000>;
+			clocks = <&cpg CPG_MOD 710>;
+			power-domains = <&sysc R8A7796_PD_ALWAYS_ON>;
+			resets = <&cpg 710>;
+		};
+
+		cmm2: cmm@fea60000 {
+			compatible = "renesas,cmm";
+			reg = <0 0xfea60000 0 0x1000>;
+			clocks = <&cpg CPG_MOD 709>;
+			power-domains = <&sysc R8A7796_PD_ALWAYS_ON>;
+			resets = <&cpg 709>;
+		};
+
 		fcpf0: fcp@fe950000 {
 			compatible = "renesas,fcpf";
 			reg = <0 0xfe950000 0 0x200>;
@@ -2763,6 +2787,7 @@
 			status = "disabled";
 
 			vsps = <&vspd0 &vspd1 &vspd2>;
+			cmms = <&cmm0 0 &cmm1 1 &cmm2 2>;
 
 			ports {
 				#address-cells = <1>;
-- 
2.21.0

Re: [RFC 8/9] clk: renesas: r8a7796: Add CMM clocks

[Geert Uytterhoeven]

On Wed, May 8, 2019 at 8:27 PM Jacopo Mondi <jacopo+renesas@jmondi.org> wrote:
> Add clock defintions for CMM units on Renesas R-Car Gen3 M3-W.
>
> Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>

Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [RFC 1/9] dt-bindings: display: renesas,cmm: Add R-Car CMM documentation

[Laurent Pinchart]

Hi Jacopo,

Thank you for the patch.

On Wed, May 08, 2019 at 07:34:20PM +0200, Jacopo Mondi wrote:
> Add device tree bindings documentation for the Renesas R-Car Display
> Unit Color Management Module.
> 
> CMM is the image enhancement module available on each R-Car DU video
> channel.

Not on all of them, V3M and V3H don't include a CMM module.

> Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
> ---
>  .../bindings/display/renesas,cmm.txt          | 24 +++++++++++++++++++
>  1 file changed, 24 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/display/renesas,cmm.txt
> 
> diff --git a/Documentation/devicetree/bindings/display/renesas,cmm.txt b/Documentation/devicetree/bindings/display/renesas,cmm.txt
> new file mode 100644
> index 000000000000..d7674417edb4
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/display/renesas,cmm.txt
> @@ -0,0 +1,24 @@
> +* Renesas R-Car Color Management Unit (CMM)

It's called Color Management Module in the documentation (hence the CMM
abbreviation)

> +
> +Renesas R-Car image enhancement module connected to R-Car DU video channels.
> +
> +Required properties:
> + - compatible: shall be:
> +   - "renesas,cmm"

There's a CMM in R-Car Gen2 with a different feature set, so I think you
need at least two compatible strings. As far as I can tell SoC-specific
compatible strings are required.

> +
> + - reg: the address base and length of the memory area where CMM control
> +   registers are mapped to.
> +
> + - clocks: phandle and clock-specifier pair to the CMM functional clock
> +   supplier.
> +
> +Example:
> +--------
> +
> +	cmm0: cmm@fea40000 {
> +		compatible = "renesas,cmm";
> +		reg = <0 0xfea40000 0 0x1000>;
> +		power-domains = <&sysc R8A7796_PD_ALWAYS_ON>;
> +		clocks = <&cpg CPG_MOD 711>;
> +		resets = <&cpg 711>;
> +	};

-- 
Regards,

Laurent Pinchart

Re: [RFC 8/9] clk: renesas: r8a7796: Add CMM clocks

[Laurent Pinchart]

Hi Jacopo,

Thank you for the patch.

On Wed, May 08, 2019 at 07:34:27PM +0200, Jacopo Mondi wrote:
> Add clock defintions for CMM units on Renesas R-Car Gen3 M3-W.
> 
> Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>

We have no clear confirmation that the parent clock is S2D1, but this
assumption makes sense given that the DU uses that clock.

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

> ---
>  drivers/clk/renesas/r8a7796-cpg-mssr.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/drivers/clk/renesas/r8a7796-cpg-mssr.c b/drivers/clk/renesas/r8a7796-cpg-mssr.c
> index 12c455859f2c..6044aeda0f83 100644
> --- a/drivers/clk/renesas/r8a7796-cpg-mssr.c
> +++ b/drivers/clk/renesas/r8a7796-cpg-mssr.c
> @@ -179,6 +179,9 @@ static const struct mssr_mod_clk r8a7796_mod_clks[] __initconst = {
>  	DEF_MOD("ehci1",		 702,	R8A7796_CLK_S3D4),
>  	DEF_MOD("ehci0",		 703,	R8A7796_CLK_S3D4),
>  	DEF_MOD("hsusb",		 704,	R8A7796_CLK_S3D4),
> +	DEF_MOD("cmm2",			 709,	R8A7796_CLK_S2D1),
> +	DEF_MOD("cmm1",			 710,	R8A7796_CLK_S2D1),
> +	DEF_MOD("cmm0",			 711,	R8A7796_CLK_S2D1),
>  	DEF_MOD("csi20",		 714,	R8A7796_CLK_CSI0),
>  	DEF_MOD("csi40",		 716,	R8A7796_CLK_CSI0),
>  	DEF_MOD("du2",			 722,	R8A7796_CLK_S2D1),

-- 
Regards,

Laurent Pinchart

Re: [RFC 2/9] dt-bindings: display, renesas,du: Document cmms property

[Laurent Pinchart]

Hi Jacopo,

Thank you for the patch.

On Wed, May 08, 2019 at 07:34:21PM +0200, Jacopo Mondi wrote:
> Document the newly added 'cmms' property which accepts a list of phandle
> and channel index pairs that point to the CMM units available for each
> Display Unit output video channel.
> 
> Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
> ---
>  Documentation/devicetree/bindings/display/renesas,du.txt | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/display/renesas,du.txt b/Documentation/devicetree/bindings/display/renesas,du.txt
> index aedb22b4d161..2ccf78bf9a18 100644
> --- a/Documentation/devicetree/bindings/display/renesas,du.txt
> +++ b/Documentation/devicetree/bindings/display/renesas,du.txt
> @@ -44,6 +44,10 @@ Required Properties:
>      instance that serves the DU channel, and the channel index identifies the
>      LIF instance in that VSP.
>  
> +  - cmms: A list of phandle and channel index tuples to the CMM modules
> +    connected to DU channels. The phandle identifies the CMM instance that
> +    serves the DU channel identified by the index.

Do we need the index ?

It should also be noted that the property is optional for SoCs that
don't have any CMM.

> +
>  Required nodes:
>  
>  The connections to the DU output video ports are modeled using the OF graph

Could you update the example ?

-- 
Regards,

Laurent Pinchart

Re: [RFC 9/9] arm64: dts: renesas: r8a7796: Add CMM units

[Laurent Pinchart]

Hi Jacopo,

Thank you for the patch.

On Wed, May 08, 2019 at 07:34:28PM +0200, Jacopo Mondi wrote:
> Add CMM units to Renesas R-Car M3-W device tree and reference them from
> the Display Unit they are connected to.
> 
> Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
> ---
>  arch/arm64/boot/dts/renesas/r8a7796.dtsi | 25 ++++++++++++++++++++++++
>  1 file changed, 25 insertions(+)
> 
> diff --git a/arch/arm64/boot/dts/renesas/r8a7796.dtsi b/arch/arm64/boot/dts/renesas/r8a7796.dtsi
> index cdf784899cf8..b81c4711dce3 100644
> --- a/arch/arm64/boot/dts/renesas/r8a7796.dtsi
> +++ b/arch/arm64/boot/dts/renesas/r8a7796.dtsi
> @@ -2503,6 +2503,30 @@
>  			renesas,fcp = <&fcpf0>;
>  		};
>  
> +		cmm0: cmm@fea40000 {
> +			compatible = "renesas,cmm";
> +			reg = <0 0xfea40000 0 0x1000>;
> +			clocks = <&cpg CPG_MOD 711>;
> +			power-domains = <&sysc R8A7796_PD_ALWAYS_ON>;
> +			resets = <&cpg 711>;
> +		};
> +
> +		cmm1: cmm@fea50000 {
> +			compatible = "renesas,cmm";
> +			reg = <0 0xfea50000 0 0x1000>;
> +			clocks = <&cpg CPG_MOD 710>;
> +			power-domains = <&sysc R8A7796_PD_ALWAYS_ON>;
> +			resets = <&cpg 710>;
> +		};
> +
> +		cmm2: cmm@fea60000 {
> +			compatible = "renesas,cmm";
> +			reg = <0 0xfea60000 0 0x1000>;
> +			clocks = <&cpg CPG_MOD 709>;
> +			power-domains = <&sysc R8A7796_PD_ALWAYS_ON>;
> +			resets = <&cpg 709>;
> +		};
> +

This looks good to me.

>  		fcpf0: fcp@fe950000 {
>  			compatible = "renesas,fcpf";
>  			reg = <0 0xfe950000 0 0x200>;
> @@ -2763,6 +2787,7 @@
>  			status = "disabled";
>  
>  			vsps = <&vspd0 &vspd1 &vspd2>;
> +			cmms = <&cmm0 0 &cmm1 1 &cmm2 2>;

Apart from the indices that I don't think are needed this looks good to
me too,

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

>  
>  			ports {
>  				#address-cells = <1>;

-- 
Regards,

Laurent Pinchart

Re: [RFC 3/9] [TODO] drm: rcar-du: Add basic support for CMM

[Laurent Pinchart]

Hi Jacopo,

Thank you for the patch.

On Wed, May 08, 2019 at 07:34:22PM +0200, Jacopo Mondi wrote:
> Add a driver skeleton for the R-Car Display Unit Color Correction
> Module.
> 
> Each DU output channel is provided with a CMM unit to perform image
> enhancement and color correction.
> 
> Add support for CMM through a skeleton driver to be later expanded to
> support setting color correction tables through the DRM/KMS properties
> framework.
> 
> As of now, the driver is only useful to demonstrate the integration with
> the DU driver.
> 
> Not-Yet-Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
> ---
>  drivers/gpu/drm/rcar-du/Kconfig       |  7 +++
>  drivers/gpu/drm/rcar-du/Makefile      |  1 +
>  drivers/gpu/drm/rcar-du/rcar_du_cmm.c | 78 +++++++++++++++++++++++++++
>  drivers/gpu/drm/rcar-du/rcar_du_cmm.h | 23 ++++++++

Please rename these files to rcar_cmm.c and rcar_cmm.h, as they're not
part of the DU driver. This will allow adding rcar_du_cmm.[ch] files if
needed for the CMM integration code in the DU driver.

>  4 files changed, 109 insertions(+)
>  create mode 100644 drivers/gpu/drm/rcar-du/rcar_du_cmm.c
>  create mode 100644 drivers/gpu/drm/rcar-du/rcar_du_cmm.h
> 
> diff --git a/drivers/gpu/drm/rcar-du/Kconfig b/drivers/gpu/drm/rcar-du/Kconfig
> index 1529849e217e..820f2b85a073 100644
> --- a/drivers/gpu/drm/rcar-du/Kconfig
> +++ b/drivers/gpu/drm/rcar-du/Kconfig
> @@ -13,6 +13,13 @@ config DRM_RCAR_DU
>  	  Choose this option if you have an R-Car chipset.
>  	  If M is selected the module will be called rcar-du-drm.
>  
> +config DRM_RCAR_CMM
> +	bool "R-Car DU Color Correction Module Support"
> +	depends on DRM && OF
> +	depends on DRM_RCAR_DU && ARCH_RCAR_GEN3

You can remove ARCH_RCAR_GEN3, CMM is also available on Gen2, and even
if we don't support this yet, there's no reason to deny compilation of
the module on Gen2 platforms.

> +	help
> +	  Enable support for R-Car Gen3 Color Correction Module (CMM).

Gen2 and Gen3.

> +
>  config DRM_RCAR_DW_HDMI
>  	tristate "R-Car DU Gen3 HDMI Encoder Support"
>  	depends on DRM && OF
> diff --git a/drivers/gpu/drm/rcar-du/Makefile b/drivers/gpu/drm/rcar-du/Makefile
> index 6c2ed9c46467..ae26d465d421 100644
> --- a/drivers/gpu/drm/rcar-du/Makefile
> +++ b/drivers/gpu/drm/rcar-du/Makefile
> @@ -15,6 +15,7 @@ rcar-du-drm-$(CONFIG_DRM_RCAR_LVDS)	+= rcar_du_of.o \
>  rcar-du-drm-$(CONFIG_DRM_RCAR_VSP)	+= rcar_du_vsp.o
>  rcar-du-drm-$(CONFIG_DRM_RCAR_WRITEBACK) += rcar_du_writeback.o
>  
> +obj-$(CONFIG_DRM_RCAR_CMM)		+= rcar_du_cmm.o
>  obj-$(CONFIG_DRM_RCAR_DU)		+= rcar-du-drm.o
>  obj-$(CONFIG_DRM_RCAR_DW_HDMI)		+= rcar_dw_hdmi.o
>  obj-$(CONFIG_DRM_RCAR_LVDS)		+= rcar_lvds.o
> diff --git a/drivers/gpu/drm/rcar-du/rcar_du_cmm.c b/drivers/gpu/drm/rcar-du/rcar_du_cmm.c
> new file mode 100644
> index 000000000000..63f545830bb9
> --- /dev/null
> +++ b/drivers/gpu/drm/rcar-du/rcar_du_cmm.c
> @@ -0,0 +1,78 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * rcar_du_cmm.c -- R-Car Display Unit Color Management Module
> + *
> + * Copyright (C) 2019 Jacopo Mondi <jacopo+renesas@jmondi.org>
> + */
> +
> +#include <linux/clk.h>
> +#include <linux/io.h>
> +#include <linux/module.h>
> +#include <linux/of.h>
> +#include <linux/platform_device.h>
> +
> +#include "rcar_du_cmm.h"
> +#include "rcar_du_crtc.h"
> +
> +struct rcar_cmm {
> +	struct clk *clk;
> +	void __iomem *base;
> +};
> +
> +int rcar_du_cmm_setup(struct platform_device *pdev, struct rcar_du_crtc *rcrtc)
> +{
> +	struct rcar_cmm *rcmm = platform_get_drvdata(pdev);
> +	int ret;
> +
> +	ret = clk_prepare_enable(rcmm->clk);
> +	if (ret)
> +		return ret;
> +
> +	return 0;
> +}

I'd name this function rcar_cmm_enable(), and add a rcar_cmm_disable().
There's no need to pass the CRTC pointer. You will also need an
rcar_cmm_setup() function that will take a data structure with
configuration parameters. I propose implementing LUT support already, so
the structure could look like

struct rcar_cmm_configuration {
	struct {
		bool enable;
		u32 *lut;
	} lut;
};

The enable flag shall program the LUT_EN bit in CM2_LUT_CTRL, and the
lut pointer, if present, shall point to 256 32-bit entries to be written
to the LUT through CM2_LUT_TBL (you can ignore double-buffering for
now). The LUT shall be initialised with an identity table at probe time:

	for (i = 0; i < 256; ++i)
		CM2_LUT_TBL[i] = (i << 16) | (i << 8) | i;

(using the appropriate I/O write function of course).

> +
> +static int rcar_cmm_probe(struct platform_device *pdev)
> +{
> +	struct rcar_cmm *rcmm;
> +	struct resource *res;
> +
> +	rcmm = devm_kzalloc(&pdev->dev, sizeof(*rcmm), GFP_KERNEL);
> +	if (!rcmm)
> +		return -ENOMEM;
> +
> +	platform_set_drvdata(pdev, rcmm);
> +
> +	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> +	rcmm->base = devm_ioremap_resource(&pdev->dev, res);
> +	if (IS_ERR(rcmm->base))
> +		return PTR_ERR(rcmm->base);
> +
> +	rcmm->clk = devm_clk_get(&pdev->dev, NULL);
> +	if (IS_ERR(rcmm->clk)) {
> +		dev_err(&pdev->dev, "Failed to get CMM clock");
> +		return PTR_ERR(rcmm->clk);
> +	}
> +
> +	return 0;
> +}
> +
> +static const struct of_device_id rcar_cmm_of_table[] = {
> +	{ .compatible = "renesas,cmm" },
> +	{ },
> +};
> +
> +MODULE_DEVICE_TABLE(of, rcar_cmm_of_table);
> +
> +static struct platform_driver rcar_cmm_platform_driver = {
> +	.probe		= rcar_cmm_probe,
> +	.driver		= {
> +		.name	= "rcar-cmm",
> +		.of_match_table = rcar_cmm_of_table,
> +	},
> +};
> +
> +module_platform_driver(rcar_cmm_platform_driver);
> +
> +MODULE_AUTHOR("Jacopo Mondi <jacopo+renesas@jmondi.org");
> +MODULE_DESCRIPTION("Renesas R-Car CMM Driver");
> +MODULE_LICENSE("GPL v2");
> diff --git a/drivers/gpu/drm/rcar-du/rcar_du_cmm.h b/drivers/gpu/drm/rcar-du/rcar_du_cmm.h
> new file mode 100644
> index 000000000000..be9ac1a091b0
> --- /dev/null
> +++ b/drivers/gpu/drm/rcar-du/rcar_du_cmm.h
> @@ -0,0 +1,23 @@
> +/* SPDX-License-Identifier: GPL-2.0+ */
> +/*
> + * rcar_du_cmm.c -- R-Car Display Unit Color Management Module
> + *
> + * Copyright (C) 2019 Jacopo Mondi <jacopo+renesas@jmondi.org>
> + */
> +
> +#ifndef __RCAR_DU_CMM_H_
> +#define __RCAR_DU_CMM_H_
> +
> +#include <linux/of.h>
> +#include <linux/platform_device.h>

You can instead add forward declarations of struct device_node and
struct platform_device.

> +
> +struct rcar_du_crtc;
> +
> +struct rcar_du_cmm {
> +	struct device_node *np;
> +	unsigned int crtc;
> +};

This structure is only used in the DU driver and should thus not be
declared here.

> +
> +int rcar_du_cmm_setup(struct platform_device *, struct rcar_du_crtc *);
> +
> +#endif /* __RCAR_DU_CMM_H_ */

-- 
Regards,

Laurent Pinchart

Re: [RFC 5/9] drm: rcar-du: Add CMM support for M3-W

[Laurent Pinchart]

Hi Jacopo,

Thank you for the patch.

On Wed, May 08, 2019 at 07:34:24PM +0200, Jacopo Mondi wrote:
> R-Car M3-W provides a CMM for each of it's DU output channel. Add CMM as
> a supported feature for the SoC.
> 
> Temporary commit to be later expanded to all Gen3 SoCs V3H/M apart.

Could you please do so in a single commit ? It should be pretty
straightforward. Apart from that this looks good to me,

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

> Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
> ---
>  drivers/gpu/drm/rcar-du/rcar_du_drv.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/rcar-du/rcar_du_drv.c b/drivers/gpu/drm/rcar-du/rcar_du_drv.c
> index 75ab17af13a9..984553342b1f 100644
> --- a/drivers/gpu/drm/rcar-du/rcar_du_drv.c
> +++ b/drivers/gpu/drm/rcar-du/rcar_du_drv.c
> @@ -280,7 +280,8 @@ static const struct rcar_du_device_info rcar_du_r8a7796_info = {
>  	.features = RCAR_DU_FEATURE_CRTC_IRQ_CLOCK
>  		  | RCAR_DU_FEATURE_VSP1_SOURCE
>  		  | RCAR_DU_FEATURE_INTERLACED
> -		  | RCAR_DU_FEATURE_TVM_SYNC,
> +		  | RCAR_DU_FEATURE_TVM_SYNC
> +		  | RCAR_DU_FEATURE_CMM,
>  	.channels_mask = BIT(2) | BIT(1) | BIT(0),
>  	.routes = {
>  		/*

-- 
Regards,

Laurent Pinchart

Re: [RFC 4/9] drm: rcar-du: kms: Create CMM instances

[Laurent Pinchart]

Hi Jacopo,

Thank you for the patch.

On Wed, May 08, 2019 at 07:34:23PM +0200, Jacopo Mondi wrote:
> Implement device tree parsing to collect the available CMM units and
> store them in the group to be later enabled at start up time.
> 
> Define a new feature to let each SoC claim support for CMM.
> 
> Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
> ---
>  drivers/gpu/drm/rcar-du/rcar_du_drv.h   |  4 ++
>  drivers/gpu/drm/rcar-du/rcar_du_group.h |  2 +
>  drivers/gpu/drm/rcar-du/rcar_du_kms.c   | 68 +++++++++++++++++++++++++
>  3 files changed, 74 insertions(+)
> 
> diff --git a/drivers/gpu/drm/rcar-du/rcar_du_drv.h b/drivers/gpu/drm/rcar-du/rcar_du_drv.h
> index 1327cd0df90a..aac916a7a46c 100644
> --- a/drivers/gpu/drm/rcar-du/rcar_du_drv.h
> +++ b/drivers/gpu/drm/rcar-du/rcar_du_drv.h
> @@ -13,6 +13,7 @@
>  #include <linux/kernel.h>
>  #include <linux/wait.h>
>  
> +#include "rcar_du_cmm.h"
>  #include "rcar_du_crtc.h"
>  #include "rcar_du_group.h"
>  #include "rcar_du_vsp.h"
> @@ -28,6 +29,7 @@ struct rcar_du_encoder;
>  #define RCAR_DU_FEATURE_VSP1_SOURCE	BIT(1)	/* Has inputs from VSP1 */
>  #define RCAR_DU_FEATURE_INTERLACED	BIT(2)	/* HW supports interlaced */
>  #define RCAR_DU_FEATURE_TVM_SYNC	BIT(3)	/* Has TV switch/sync modes */
> +#define RCAR_DU_FEATURE_CMM		BIT(4)	/* Has CCM */
>  
>  #define RCAR_DU_QUIRK_ALIGN_128B	BIT(0)	/* Align pitches to 128 bytes */
>  
> @@ -69,6 +71,7 @@ struct rcar_du_device_info {
>  
>  #define RCAR_DU_MAX_CRTCS		4
>  #define RCAR_DU_MAX_GROUPS		DIV_ROUND_UP(RCAR_DU_MAX_CRTCS, 2)
> +#define RCAR_DU_MAX_CMMS		4
>  #define RCAR_DU_MAX_VSPS		4
>  
>  struct rcar_du_device {
> @@ -85,6 +88,7 @@ struct rcar_du_device {
>  	struct rcar_du_encoder *encoders[RCAR_DU_OUTPUT_MAX];
>  
>  	struct rcar_du_group groups[RCAR_DU_MAX_GROUPS];
> +	struct rcar_du_cmm cmms[RCAR_DU_MAX_CMMS];
>  	struct rcar_du_vsp vsps[RCAR_DU_MAX_VSPS];
>  
>  	struct {
> diff --git a/drivers/gpu/drm/rcar-du/rcar_du_group.h b/drivers/gpu/drm/rcar-du/rcar_du_group.h
> index 87950c1f6a52..b0c1466593a3 100644
> --- a/drivers/gpu/drm/rcar-du/rcar_du_group.h
> +++ b/drivers/gpu/drm/rcar-du/rcar_du_group.h
> @@ -22,6 +22,7 @@ struct rcar_du_device;
>   * @mmio_offset: registers offset in the device memory map
>   * @index: group index
>   * @channels_mask: bitmask of populated DU channels in this group
> + * @cmms_mask: bitmask of enabled CMMs in this group
>   * @num_crtcs: number of CRTCs in this group (1 or 2)
>   * @use_count: number of users of the group (rcar_du_group_(get|put))
>   * @used_crtcs: number of CRTCs currently in use
> @@ -37,6 +38,7 @@ struct rcar_du_group {
>  	unsigned int index;
>  
>  	unsigned int channels_mask;
> +	unsigned int cmms_mask;
>  	unsigned int num_crtcs;
>  	unsigned int use_count;
>  	unsigned int used_crtcs;
> diff --git a/drivers/gpu/drm/rcar-du/rcar_du_kms.c b/drivers/gpu/drm/rcar-du/rcar_du_kms.c
> index adbc4f5d8fc5..684a60feac5c 100644
> --- a/drivers/gpu/drm/rcar-du/rcar_du_kms.c
> +++ b/drivers/gpu/drm/rcar-du/rcar_du_kms.c
> @@ -614,6 +614,64 @@ static int rcar_du_vsps_init(struct rcar_du_device *rcdu)
>  	return ret;
>  }
>  
> +static int rcar_du_cmm_init(struct rcar_du_device *rcdu)
> +{
> +	const struct device_node *np = rcdu->dev->of_node;
> +	unsigned int cells;
> +	unsigned int i;
> +	int ret;
> +
> +	/*
> +	 * Make sure the DT 'cmms' property is well formed and populate
> +	 * the list of enabled CMM.
> +	 */
> +	cells = of_property_count_u32_elems(np, "cmms");
> +	if (cells % 2 || cells > rcdu->num_crtcs * 2) {
> +		dev_err(rcdu->dev, "invalid 'cmms' property format\n");
> +		return -EINVAL;
> +	}
> +
> +	for (i = 0; i < cells / 2; ++i) {
> +		struct of_phandle_args args;
> +		struct rcar_du_group *rgrp;
> +		unsigned int crtc;
> +
> +		ret = of_parse_phandle_with_fixed_args(np, "cmms", 1, i,
> +						       &args);
> +		if (ret < 0) {
> +			dev_err(rcdu->dev, "failed to parse 'cmms' property\n");
> +			goto error;
> +		}
> +
> +		crtc = args.args[0];
> +		if (crtc >= rcdu->num_crtcs ||
> +		    !(rcdu->info->channels_mask & BIT(crtc))) {
> +			dev_err(rcdu->dev,
> +				"invalid DU channel %u in 'cmms' property\n",
> +				crtc);
> +			goto error;
> +		}
> +
> +		rcdu->cmms[i].np = args.np;

How about storing the struct device pointer instead (using
of_find_device_by_node()) ? You should add a rcar_cmm_init() function to
the CMM API, similar to vsp1_du_init().

> +		rcdu->cmms[i].crtc = crtc;
> +
> +		/*
> +		 * CMMs are activated by the DU group: store the active CMMs
> +		 * indexes in the group.
> +		 */
> +		rgrp = &rcdu->groups[crtc / 2];
> +		rgrp->cmms_mask |= BIT(crtc % 2);
> +	}

I think you can simplify this by dropping the index from renesas,cmms.
You can simply loop over the CRTCs and get the phandle corresponding to
the CRTC index.

> +
> +	return 0;
> +
> +error:
> +	for (; i > 0; --i)
> +		of_node_put(rcdu->cmms[i - 1].np);
> +
> +	return ret;
> +}
> +
>  int rcar_du_modeset_init(struct rcar_du_device *rcdu)
>  {
>  	static const unsigned int mmio_offsets[] = {
> @@ -680,6 +738,9 @@ int rcar_du_modeset_init(struct rcar_du_device *rcdu)
>  				   & GENMASK(1, 0);
>  		rgrp->num_crtcs = hweight8(rgrp->channels_mask);
>  
> +		/* The active CMMs mask will be later populated. */
> +		rgrp->cmms_mask = 0;
> +

The structures are initialised to 0 when allocated so this isn't
strictly required.

>  		/*
>  		 * If we have more than one CRTCs in this group pre-associate
>  		 * the low-order planes with CRTC 0 and the high-order planes
> @@ -704,6 +765,13 @@ int rcar_du_modeset_init(struct rcar_du_device *rcdu)
>  			return ret;
>  	}
>  
> +	/* Initialize the Color Management Modules. */
> +	if (rcar_du_has(rcdu, RCAR_DU_FEATURE_CMM)) {
> +		ret = rcar_du_cmm_init(rcdu);
> +		if (ret < 0)
> +			return ret;
> +	}
> +
>  	/* Create the CRTCs. */
>  	for (swindex = 0, hwindex = 0; swindex < rcdu->num_crtcs; ++hwindex) {
>  		struct rcar_du_group *rgrp;

-- 
Regards,

Laurent Pinchart

Re: [RFC 6/9] drm: rcar-du: crtc: Setup the CMM

[Laurent Pinchart]

Hi Jacopo,

Thank you for the patch.

On Wed, May 08, 2019 at 07:34:25PM +0200, Jacopo Mondi wrote:
> Save a reference to the CMM unit associated with the CRTC and set it up
> at rcar_du_crtc_setup() time, just after having enabled the compositor.
> 
> Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
> ---
>  drivers/gpu/drm/rcar-du/rcar_du_crtc.c | 38 ++++++++++++++++++++++++--
>  drivers/gpu/drm/rcar-du/rcar_du_crtc.h |  2 ++
>  2 files changed, 38 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/rcar-du/rcar_du_crtc.c b/drivers/gpu/drm/rcar-du/rcar_du_crtc.c
> index 2da46e3dc4ae..b5a9af8e4df0 100644
> --- a/drivers/gpu/drm/rcar-du/rcar_du_crtc.c
> +++ b/drivers/gpu/drm/rcar-du/rcar_du_crtc.c
> @@ -9,6 +9,7 @@
>  
>  #include <linux/clk.h>
>  #include <linux/mutex.h>
> +#include <linux/of_platform.h>
>  #include <linux/platform_device.h>
>  #include <linux/sys_soc.h>
>  
> @@ -21,6 +22,7 @@
>  #include <drm/drm_plane_helper.h>
>  #include <drm/drm_vblank.h>
>  
> +#include "rcar_du_cmm.h"
>  #include "rcar_du_crtc.h"
>  #include "rcar_du_drv.h"
>  #include "rcar_du_encoder.h"
> @@ -478,7 +480,7 @@ static void rcar_du_crtc_wait_page_flip(struct rcar_du_crtc *rcrtc)
>   * Start/Stop and Suspend/Resume
>   */
>  
> -static void rcar_du_crtc_setup(struct rcar_du_crtc *rcrtc)
> +static int rcar_du_crtc_setup(struct rcar_du_crtc *rcrtc)
>  {
>  	/* Set display off and background to black */
>  	rcar_du_crtc_write(rcrtc, DOOR, DOOR_RGB(0, 0, 0));
> @@ -495,8 +497,24 @@ static void rcar_du_crtc_setup(struct rcar_du_crtc *rcrtc)
>  	if (rcar_du_has(rcrtc->dev, RCAR_DU_FEATURE_VSP1_SOURCE))
>  		rcar_du_vsp_enable(rcrtc);
>  
> +	/* Enable CMM, if present. */
> +	if (rcrtc->cmm) {
> +		struct platform_device *pdev;
> +		int ret;
> +
> +		pdev = of_find_device_by_node(rcrtc->cmm->np);
> +		if (!pdev)
> +			return PTR_ERR(pdev);

This should go to init time, in order to defer probing of the DU if CMM
isn't probed yet.

> +
> +		ret = rcar_du_cmm_setup(pdev, rcrtc);
> +		if (ret)
> +			return ret;
> +	}
> +
>  	/* Turn vertical blanking interrupt reporting on. */
>  	drm_crtc_vblank_on(&rcrtc->crtc);
> +
> +	return 0;
>  }
>  
>  static int rcar_du_crtc_get(struct rcar_du_crtc *rcrtc)
> @@ -522,7 +540,10 @@ static int rcar_du_crtc_get(struct rcar_du_crtc *rcrtc)
>  	if (ret < 0)
>  		goto error_group;
>  
> -	rcar_du_crtc_setup(rcrtc);
> +	ret = rcar_du_crtc_setup(rcrtc);
> +	if (ret < 0)
> +		goto error_group;
> +
>  	rcrtc->initialized = true;
>  
>  	return 0;
> @@ -1182,6 +1203,19 @@ int rcar_du_crtc_create(struct rcar_du_group *rgrp, unsigned int swindex,
>  	rcrtc->index = hwindex;
>  	rcrtc->dsysr = (rcrtc->index % 2 ? 0 : DSYSR_DRES) | DSYSR_TVM_TVSYNC;
>  
> +	if (rcar_du_has(rcdu, RCAR_DU_FEATURE_CMM)) {
> +		unsigned int i;
> +
> +		for (i = 0; i < RCAR_DU_MAX_CMMS; ++i) {
> +			struct rcar_du_cmm *cmm = &rcdu->cmms[i];
> +
> +			if (cmm->crtc != hwindex)
> +				continue;
> +
> +			rcrtc->cmm = cmm;

Let's just store the CMM instances in the same order as the CRTCs, and
this can becomre rcrtc->cmm = &rcdu->cmms[swindex]; (and should be moved
to the previous patch I think).

> +		}
> +	}
> +
>  	if (rcar_du_has(rcdu, RCAR_DU_FEATURE_VSP1_SOURCE))
>  		primary = &rcrtc->vsp->planes[rcrtc->vsp_pipe].plane;
>  	else
> diff --git a/drivers/gpu/drm/rcar-du/rcar_du_crtc.h b/drivers/gpu/drm/rcar-du/rcar_du_crtc.h
> index 3b7fc668996f..8b31406bd11b 100644
> --- a/drivers/gpu/drm/rcar-du/rcar_du_crtc.h
> +++ b/drivers/gpu/drm/rcar-du/rcar_du_crtc.h
> @@ -19,6 +19,7 @@
>  
>  #include <media/vsp1.h>
>  
> +struct rcar_du_cmm;
>  struct rcar_du_group;
>  struct rcar_du_vsp;
>  
> @@ -64,6 +65,7 @@ struct rcar_du_crtc {
>  	unsigned int vblank_count;
>  
>  	struct rcar_du_group *group;
> +	struct rcar_du_cmm *cmm;
>  	struct rcar_du_vsp *vsp;
>  	unsigned int vsp_pipe;
>  

-- 
Regards,

Laurent Pinchart

Re: [RFC 7/9] drm: rcar-du: group: Enable CMM unit

[Laurent Pinchart]

Hi Jacopo,

Thank you for the patch.

On Wed, May 08, 2019 at 07:34:26PM +0200, Jacopo Mondi wrote:
> Enable the CMM units present in the group through the display unit
> extensional function control group register DEFR7.
> 
> Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
> ---
>  drivers/gpu/drm/rcar-du/rcar_du_group.c | 8 ++++++++
>  drivers/gpu/drm/rcar-du/rcar_du_regs.h  | 5 +++++
>  2 files changed, 13 insertions(+)
> 
> diff --git a/drivers/gpu/drm/rcar-du/rcar_du_group.c b/drivers/gpu/drm/rcar-du/rcar_du_group.c
> index 9eee47969e77..ce25e41b04bc 100644
> --- a/drivers/gpu/drm/rcar-du/rcar_du_group.c
> +++ b/drivers/gpu/drm/rcar-du/rcar_du_group.c
> @@ -147,6 +147,14 @@ static void rcar_du_group_setup(struct rcar_du_group *rgrp)
>  
>  	rcar_du_group_setup_pins(rgrp);
>  
> +	if (rgrp->cmms_mask) {
> +		u32 defr7 = DEFR7_CODE;
> +		defr7 |= rgrp->cmms_mask & BIT(1) ? DEFR7_CMME1 : 0;
> +		defr7 |= rgrp->cmms_mask & BIT(0) ? DEFR7_CMME0 : 0;
> +
> +		rcar_du_group_write(rgrp, DEFR7, defr7);
> +	}
> +

I would guard this with the CMM feature bit instead of cmms_mask, in
order to write the register with 0 if CMMs are not supported, just in
case it maye have been written to another value by the boot loader (or
before a warm reboot).

Apart from that,

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

>  	if (rcdu->info->gen >= 2) {
>  		rcar_du_group_setup_defr8(rgrp);
>  		rcar_du_group_setup_didsr(rgrp);
> diff --git a/drivers/gpu/drm/rcar-du/rcar_du_regs.h b/drivers/gpu/drm/rcar-du/rcar_du_regs.h
> index bc87f080b170..fb9964949368 100644
> --- a/drivers/gpu/drm/rcar-du/rcar_du_regs.h
> +++ b/drivers/gpu/drm/rcar-du/rcar_du_regs.h
> @@ -197,6 +197,11 @@
>  #define DEFR6_MLOS1		(1 << 2)
>  #define DEFR6_DEFAULT		(DEFR6_CODE | DEFR6_TCNE1)
>  
> +#define DEFR7			0x000ec
> +#define DEFR7_CODE		(0x7779 << 16)
> +#define DEFR7_CMME1		BIT(6)
> +#define DEFR7_CMME0		BIT(4)
> +
>  /* -----------------------------------------------------------------------------
>   * R8A7790-only Control Registers
>   */

-- 
Regards,

Laurent Pinchart

Re: [RFC 2/9] dt-bindings: display, renesas,du: Document cmms property

[Jacopo Mondi]

[-- Attachment #1: Type: text/plain, Size: 2626 bytes --]

Hi Laurent,
   thanks for the comments

On Sat, May 11, 2019 at 09:23:30PM +0300, Laurent Pinchart wrote:
> Hi Jacopo,
>
> Thank you for the patch.
>
> On Wed, May 08, 2019 at 07:34:21PM +0200, Jacopo Mondi wrote:
> > Document the newly added 'cmms' property which accepts a list of phandle
> > and channel index pairs that point to the CMM units available for each
> > Display Unit output video channel.
> >
> > Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
> > ---
> >  Documentation/devicetree/bindings/display/renesas,du.txt | 4 ++++
> >  1 file changed, 4 insertions(+)
> >
> > diff --git a/Documentation/devicetree/bindings/display/renesas,du.txt b/Documentation/devicetree/bindings/display/renesas,du.txt
> > index aedb22b4d161..2ccf78bf9a18 100644
> > --- a/Documentation/devicetree/bindings/display/renesas,du.txt
> > +++ b/Documentation/devicetree/bindings/display/renesas,du.txt
> > @@ -44,6 +44,10 @@ Required Properties:
> >      instance that serves the DU channel, and the channel index identifies the
> >      LIF instance in that VSP.
> >
> > +  - cmms: A list of phandle and channel index tuples to the CMM modules
> > +    connected to DU channels. The phandle identifies the CMM instance that
> > +    serves the DU channel identified by the index.
>
> Do we need the index ?
>

Well, I struggled a bit at deciding if this was a good idea or not.
In the end I decided to use the index, as in this version, by just
providing the cmm phandle, the CMM gets enabled for the DU channel it
is associated to. It is true I could just enumerate them, and assign
the CMM corresponding to the first phandle to channel #0, the second
to channel #1 and so on, but in the (very unlikely?) case where a
developer what to enable CMM for, say, channel #0 and #2 but not #1,
this scheme would break, as I have then decided to have a mandatory
channel index to make the association stable. True that a CMM unit is
associated to a DU channel only, and I could derive this from the base
address or a custom property (like 'renesa,du-channel) in the CMM
device node, but this seems better handled here.

Now that I wrote this, I wonder if I actually need to know which
channel a CMM is associated to, or I could just go and enable the ones
listed in the 'cmms' property and that's it. Was this the idea behind
your question?

Thanks
   j
> It should also be noted that the property is optional for SoCs that
> don't have any CMM.
>
> > +
> >  Required nodes:
> >
> >  The connections to the DU output video ports are modeled using the OF graph
>
> Could you update the example ?
>
> --
> Regards,
>
> Laurent Pinchart

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [RFC 2/9] dt-bindings: display, renesas,du: Document cmms property

[Laurent Pinchart]

Hi Jacopo,

On Wed, May 15, 2019 at 04:12:20PM +0200, Jacopo Mondi wrote:
> On Sat, May 11, 2019 at 09:23:30PM +0300, Laurent Pinchart wrote:
> > On Wed, May 08, 2019 at 07:34:21PM +0200, Jacopo Mondi wrote:
> >> Document the newly added 'cmms' property which accepts a list of phandle
> >> and channel index pairs that point to the CMM units available for each
> >> Display Unit output video channel.
> >>
> >> Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
> >> ---
> >>  Documentation/devicetree/bindings/display/renesas,du.txt | 4 ++++
> >>  1 file changed, 4 insertions(+)
> >>
> >> diff --git a/Documentation/devicetree/bindings/display/renesas,du.txt b/Documentation/devicetree/bindings/display/renesas,du.txt
> >> index aedb22b4d161..2ccf78bf9a18 100644
> >> --- a/Documentation/devicetree/bindings/display/renesas,du.txt
> >> +++ b/Documentation/devicetree/bindings/display/renesas,du.txt
> >> @@ -44,6 +44,10 @@ Required Properties:
> >>      instance that serves the DU channel, and the channel index identifies the
> >>      LIF instance in that VSP.
> >>
> >> +  - cmms: A list of phandle and channel index tuples to the CMM modules
> >> +    connected to DU channels. The phandle identifies the CMM instance that
> >> +    serves the DU channel identified by the index.
> >
> > Do we need the index ?
> 
> Well, I struggled a bit at deciding if this was a good idea or not.
> In the end I decided to use the index, as in this version, by just
> providing the cmm phandle, the CMM gets enabled for the DU channel it
> is associated to. It is true I could just enumerate them, and assign
> the CMM corresponding to the first phandle to channel #0, the second
> to channel #1 and so on, but in the (very unlikely?) case where a
> developer what to enable CMM for, say, channel #0 and #2 but not #1,
> this scheme would break, as I have then decided to have a mandatory
> channel index to make the association stable. True that a CMM unit is
> associated to a DU channel only, and I could derive this from the base
> address or a custom property (like 'renesa,du-channel) in the CMM
> device node, but this seems better handled here.

First of all, DT should describe the hardware, enabling or disabling CMM
should not be done through this property. Then, if you really wanted to
control which channels use CMM through DT, you could just insert a 0
value in the cmms array in the corresponding position instead of the
phandler of a CMM node. Finally, you can also use status = "disabled" in
the CMM nodes to disable them, and the DU driver could check that. That
seems a better mechanism if we want to disable CMM support in DT.

> Now that I wrote this, I wonder if I actually need to know which
> channel a CMM is associated to, or I could just go and enable the ones
> listed in the 'cmms' property and that's it. Was this the idea behind
> your question?

You need to know the association, as you will have to configure the
CMMs, not just enable or disable them.

> > It should also be noted that the property is optional for SoCs that
> > don't have any CMM.
> >
> >> +
> >>  Required nodes:
> >>
> >>  The connections to the DU output video ports are modeled using the OF graph
> >
> > Could you update the example ?
> >

-- 
Regards,

Laurent Pinchart

Re: [RFC 8/9] clk: renesas: r8a7796: Add CMM clocks

[Geert Uytterhoeven]

On Sat, May 11, 2019 at 8:22 PM Laurent Pinchart
<laurent.pinchart@ideasonboard.com> wrote:
> On Wed, May 08, 2019 at 07:34:27PM +0200, Jacopo Mondi wrote:
> > Add clock defintions for CMM units on Renesas R-Car Gen3 M3-W.
> >
> > Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
>
> We have no clear confirmation that the parent clock is S2D1, but this
> assumption makes sense given that the DU uses that clock.
>
> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

Applied to clk-renesas-for-v5.3, with the "defintions" fixed, and the context
updated.

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [RFC 1/9] dt-bindings: display: renesas,cmm: Add R-Car CMM documentation

[Jacopo Mondi]

[-- Attachment #1: Type: text/plain, Size: 2428 bytes --]

Hi Laurent,

On Sat, May 11, 2019 at 09:16:18PM +0300, Laurent Pinchart wrote:
> Hi Jacopo,
>
> Thank you for the patch.
>
> On Wed, May 08, 2019 at 07:34:20PM +0200, Jacopo Mondi wrote:
> > Add device tree bindings documentation for the Renesas R-Car Display
> > Unit Color Management Module.
> >
> > CMM is the image enhancement module available on each R-Car DU video
> > channel.
>
> Not on all of them, V3M and V3H don't include a CMM module.
>
> > Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
> > ---
> >  .../bindings/display/renesas,cmm.txt          | 24 +++++++++++++++++++
> >  1 file changed, 24 insertions(+)
> >  create mode 100644 Documentation/devicetree/bindings/display/renesas,cmm.txt
> >
> > diff --git a/Documentation/devicetree/bindings/display/renesas,cmm.txt b/Documentation/devicetree/bindings/display/renesas,cmm.txt
> > new file mode 100644
> > index 000000000000..d7674417edb4
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/display/renesas,cmm.txt
> > @@ -0,0 +1,24 @@
> > +* Renesas R-Car Color Management Unit (CMM)
>
> It's called Color Management Module in the documentation (hence the CMM
> abbreviation)
>
> > +
> > +Renesas R-Car image enhancement module connected to R-Car DU video channels.
> > +
> > +Required properties:
> > + - compatible: shall be:
> > +   - "renesas,cmm"
>
> There's a CMM in R-Car Gen2 with a different feature set, so I think you
> need at least two compatible strings. As far as I can tell SoC-specific
> compatible strings are required.

I assume you meant "SoC-specific compatible strings are NOT required" ?

Could you otherwise specify why do you think we need a per-SoC
compatible, since there are no platform specific data (for now, at
least, but considering the CMM seems identical in all SoCs I hardly
think we will have any in the near future).

Ack on the gen2/gen3 specific strings though.

Thanks
   j


>
> > +
> > + - reg: the address base and length of the memory area where CMM control
> > +   registers are mapped to.
> > +
> > + - clocks: phandle and clock-specifier pair to the CMM functional clock
> > +   supplier.
> > +
> > +Example:
> > +--------
> > +
> > +	cmm0: cmm@fea40000 {
> > +		compatible = "renesas,cmm";
> > +		reg = <0 0xfea40000 0 0x1000>;
> > +		power-domains = <&sysc R8A7796_PD_ALWAYS_ON>;
> > +		clocks = <&cpg CPG_MOD 711>;
> > +		resets = <&cpg 711>;
> > +	};
>
> --
> Regards,
>
> Laurent Pinchart

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [RFC 1/9] dt-bindings: display: renesas,cmm: Add R-Car CMM documentation

[Laurent Pinchart]

Hi Jacopo,

On Tue, May 28, 2019 at 02:37:25PM +0200, Jacopo Mondi wrote:
> On Sat, May 11, 2019 at 09:16:18PM +0300, Laurent Pinchart wrote:
> > On Wed, May 08, 2019 at 07:34:20PM +0200, Jacopo Mondi wrote:
> >> Add device tree bindings documentation for the Renesas R-Car Display
> >> Unit Color Management Module.
> >>
> >> CMM is the image enhancement module available on each R-Car DU video
> >> channel.
> >
> > Not on all of them, V3M and V3H don't include a CMM module.
> >
> >> Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
> >> ---
> >>  .../bindings/display/renesas,cmm.txt          | 24 +++++++++++++++++++
> >>  1 file changed, 24 insertions(+)
> >>  create mode 100644 Documentation/devicetree/bindings/display/renesas,cmm.txt
> >>
> >> diff --git a/Documentation/devicetree/bindings/display/renesas,cmm.txt b/Documentation/devicetree/bindings/display/renesas,cmm.txt
> >> new file mode 100644
> >> index 000000000000..d7674417edb4
> >> --- /dev/null
> >> +++ b/Documentation/devicetree/bindings/display/renesas,cmm.txt
> >> @@ -0,0 +1,24 @@
> >> +* Renesas R-Car Color Management Unit (CMM)
> >
> > It's called Color Management Module in the documentation (hence the CMM
> > abbreviation)
> >
> >> +
> >> +Renesas R-Car image enhancement module connected to R-Car DU video channels.
> >> +
> >> +Required properties:
> >> + - compatible: shall be:
> >> +   - "renesas,cmm"
> >
> > There's a CMM in R-Car Gen2 with a different feature set, so I think you
> > need at least two compatible strings. As far as I can tell SoC-specific
> > compatible strings are required.
> 
> I assume you meant "SoC-specific compatible strings are NOT required" ?

Correct, sorry.

> Could you otherwise specify why do you think we need a per-SoC
> compatible, since there are no platform specific data (for now, at
> least, but considering the CMM seems identical in all SoCs I hardly
> think we will have any in the near future).
> 
> Ack on the gen2/gen3 specific strings though.
> 
> >> +
> >> + - reg: the address base and length of the memory area where CMM control
> >> +   registers are mapped to.
> >> +
> >> + - clocks: phandle and clock-specifier pair to the CMM functional clock
> >> +   supplier.
> >> +
> >> +Example:
> >> +--------
> >> +
> >> +	cmm0: cmm@fea40000 {
> >> +		compatible = "renesas,cmm";
> >> +		reg = <0 0xfea40000 0 0x1000>;
> >> +		power-domains = <&sysc R8A7796_PD_ALWAYS_ON>;
> >> +		clocks = <&cpg CPG_MOD 711>;
> >> +		resets = <&cpg 711>;
> >> +	};

-- 
Regards,

Laurent Pinchart

Re: [RFC 1/9] dt-bindings: display: renesas,cmm: Add R-Car CMM documentation

[Geert Uytterhoeven]

Hi Laurent,

On Tue, May 28, 2019 at 4:31 PM Laurent Pinchart
<laurent.pinchart@ideasonboard.com> wrote:
> On Tue, May 28, 2019 at 02:37:25PM +0200, Jacopo Mondi wrote:
> > On Sat, May 11, 2019 at 09:16:18PM +0300, Laurent Pinchart wrote:
> > > On Wed, May 08, 2019 at 07:34:20PM +0200, Jacopo Mondi wrote:
> > >> Add device tree bindings documentation for the Renesas R-Car Display
> > >> Unit Color Management Module.
> > >>
> > >> CMM is the image enhancement module available on each R-Car DU video
> > >> channel.
> > >
> > > Not on all of them, V3M and V3H don't include a CMM module.
> > >
> > >> Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>

> > >> --- /dev/null
> > >> +++ b/Documentation/devicetree/bindings/display/renesas,cmm.txt
> > >> @@ -0,0 +1,24 @@
> > >> +* Renesas R-Car Color Management Unit (CMM)
> > >
> > > It's called Color Management Module in the documentation (hence the CMM
> > > abbreviation)
> > >
> > >> +
> > >> +Renesas R-Car image enhancement module connected to R-Car DU video channels.
> > >> +
> > >> +Required properties:
> > >> + - compatible: shall be:
> > >> +   - "renesas,cmm"
> > >
> > > There's a CMM in R-Car Gen2 with a different feature set, so I think you
> > > need at least two compatible strings. As far as I can tell SoC-specific
> > > compatible strings are required.
> >
> > I assume you meant "SoC-specific compatible strings are NOT required" ?
>
> Correct, sorry.
>
> > Could you otherwise specify why do you think we need a per-SoC
> > compatible, since there are no platform specific data (for now, at
> > least, but considering the CMM seems identical in all SoCs I hardly
> > think we will have any in the near future).
> >
> > Ack on the gen2/gen3 specific strings though.

Usually we do define SoC-specific compatible values in the DT bindings,
unless there is a version register, like on the VPSs.

Why would we want to deviate from that practice for the CMM?

Thanks!

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds