[PATCH 1/2 v2] drm/pl111: Support the Versatile Express

[PATCH 1/2 v2] drm/pl111: Support the Versatile Express

[Linus Walleij]

The Versatile Express uses a special configuration controller
deeply embedded in the system motherboard FPGA to multiplex the
two to three (!) CLCD instances out to the single SiI9022
bridge.

Set up an extra file with the logic to probe to the FPGA mux
register on the system controller bus, then parse the memory
range argument to figure out what path the CLCD signal is
actually taking, and set up the mux accordingly.

If there is a CLCD instance on the core tile (the daughterboard
with the CPUs fitted) then that CLCD instance will take
precedence since it can address all memory.

Scale down the Versatile Express to 16BPP so we can support a
1024x768 display despite the bus bandwidth restrictions on this
platform.

Cc: Liviu Dudau <liviu.dudau@arm.com>
Cc: Pawel Moll <pawel.moll@arm.com>
Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
---
ChangeLog v1->v2:
- No changes just reposting rebased on mainline changes.
---
 drivers/gpu/drm/pl111/Makefile          |   1 +
 drivers/gpu/drm/pl111/pl111_drm.h       |   3 +-
 drivers/gpu/drm/pl111/pl111_versatile.c |  48 ++++++++++++++-
 drivers/gpu/drm/pl111/pl111_vexpress.c  | 106 ++++++++++++++++++++++++++++++++
 drivers/gpu/drm/pl111/pl111_vexpress.h  |  22 +++++++
 5 files changed, 178 insertions(+), 2 deletions(-)
 create mode 100644 drivers/gpu/drm/pl111/pl111_vexpress.c
 create mode 100644 drivers/gpu/drm/pl111/pl111_vexpress.h

diff --git a/drivers/gpu/drm/pl111/Makefile b/drivers/gpu/drm/pl111/Makefile
index 9c5e8dba8ac6..19a8189dc54f 100644
--- a/drivers/gpu/drm/pl111/Makefile
+++ b/drivers/gpu/drm/pl111/Makefile
@@ -3,6 +3,7 @@ pl111_drm-y +=	pl111_display.o \
 		pl111_versatile.o \
 		pl111_drv.o
 
+pl111_drm-$(CONFIG_ARCH_VEXPRESS) += pl111_vexpress.o
 pl111_drm-$(CONFIG_DEBUG_FS) += pl111_debugfs.o
 
 obj-$(CONFIG_DRM_PL111) += pl111_drm.o
diff --git a/drivers/gpu/drm/pl111/pl111_drm.h b/drivers/gpu/drm/pl111/pl111_drm.h
index 8639b2d4ddf7..916154ac733a 100644
--- a/drivers/gpu/drm/pl111/pl111_drm.h
+++ b/drivers/gpu/drm/pl111/pl111_drm.h
@@ -64,7 +64,8 @@ struct pl111_drm_dev_private {
 	struct drm_bridge *bridge;
 	struct drm_simple_display_pipe pipe;
 
-	void *regs;
+	void __iomem *clcd_memory;
+	void __iomem *regs;
 	u32 memory_bw;
 	u32 ienb;
 	u32 ctrl;
diff --git a/drivers/gpu/drm/pl111/pl111_versatile.c b/drivers/gpu/drm/pl111/pl111_versatile.c
index 9302f516045e..569edf02a36a 100644
--- a/drivers/gpu/drm/pl111/pl111_versatile.c
+++ b/drivers/gpu/drm/pl111/pl111_versatile.c
@@ -1,12 +1,14 @@
 #include <linux/amba/clcd-regs.h>
 #include <linux/device.h>
 #include <linux/of.h>
+#include <linux/of_platform.h>
 #include <linux/regmap.h>
 #include <linux/mfd/syscon.h>
 #include <linux/bitops.h>
 #include <linux/module.h>
 #include <drm/drmP.h>
 #include "pl111_versatile.h"
+#include "pl111_vexpress.h"
 #include "pl111_drm.h"
 
 static struct regmap *versatile_syscon_map;
@@ -22,6 +24,7 @@ enum versatile_clcd {
 	REALVIEW_CLCD_PB11MP,
 	REALVIEW_CLCD_PBA8,
 	REALVIEW_CLCD_PBX,
+	VEXPRESS_CLCD_V2M,
 };
 
 static const struct of_device_id versatile_clcd_of_match[] = {
@@ -53,6 +56,10 @@ static const struct of_device_id versatile_clcd_of_match[] = {
 		.compatible = "arm,realview-pbx-syscon",
 		.data = (void *)REALVIEW_CLCD_PBX,
 	},
+	{
+		.compatible = "arm,vexpress-muxfpga",
+		.data = (void *)VEXPRESS_CLCD_V2M,
+	},
 	{},
 };
 
@@ -286,12 +293,26 @@ static const struct pl111_variant_data pl111_realview = {
 	.fb_bpp = 16,
 };
 
+/*
+ * Versatile Express PL111 variant, again we just push the maximum
+ * BPP to 16 to be able to get 1024x768 without saturating the memory
+ * bus. The clockdivider also seems broken on the Versatile Express.
+ */
+static const struct pl111_variant_data pl111_vexpress = {
+	.name = "PL111 Versatile Express",
+	.formats = pl111_realview_pixel_formats,
+	.nformats = ARRAY_SIZE(pl111_realview_pixel_formats),
+	.fb_bpp = 16,
+	.broken_clockdivider = true,
+};
+
 int pl111_versatile_init(struct device *dev, struct pl111_drm_dev_private *priv)
 {
 	const struct of_device_id *clcd_id;
 	enum versatile_clcd versatile_clcd_type;
 	struct device_node *np;
 	struct regmap *map;
+	int ret;
 
 	np = of_find_matching_node_and_match(NULL, versatile_clcd_of_match,
 					     &clcd_id);
@@ -301,7 +322,25 @@ int pl111_versatile_init(struct device *dev, struct pl111_drm_dev_private *priv)
 	}
 	versatile_clcd_type = (enum versatile_clcd)clcd_id->data;
 
-	map = syscon_node_to_regmap(np);
+	/* Versatile Express special handling */
+	if (versatile_clcd_type == VEXPRESS_CLCD_V2M) {
+		struct platform_device *pdev;
+
+		/* Call into deep Vexpress configuration API */
+		pdev = of_find_device_by_node(np);
+		if (!pdev) {
+			dev_err(dev, "can't find the sysreg device, deferring\n");
+			return -EPROBE_DEFER;
+		}
+		map = dev_get_drvdata(&pdev->dev);
+		if (!map) {
+			dev_err(dev, "sysreg has not yet probed\n");
+			return -EPROBE_DEFER;
+		}
+	} else {
+		map = syscon_node_to_regmap(np);
+	}
+
 	if (IS_ERR(map)) {
 		dev_err(dev, "no Versatile syscon regmap\n");
 		return PTR_ERR(map);
@@ -340,6 +379,13 @@ int pl111_versatile_init(struct device *dev, struct pl111_drm_dev_private *priv)
 		priv->variant_display_disable = pl111_realview_clcd_disable;
 		dev_info(dev, "set up callbacks for RealView PL111\n");
 		break;
+	case VEXPRESS_CLCD_V2M:
+		priv->variant = &pl111_vexpress;
+		dev_info(dev, "initializing Versatile Express PL111\n");
+		ret = pl111_vexpress_clcd_init(dev, priv, map);
+		if (ret)
+			return ret;
+		break;
 	default:
 		dev_info(dev, "unknown Versatile system controller\n");
 		break;
diff --git a/drivers/gpu/drm/pl111/pl111_vexpress.c b/drivers/gpu/drm/pl111/pl111_vexpress.c
new file mode 100644
index 000000000000..720244f497fe
--- /dev/null
+++ b/drivers/gpu/drm/pl111/pl111_vexpress.c
@@ -0,0 +1,106 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Versatile Express PL111 handling
+ * Copyright (C) 2018 Linus Walleij
+ *
+ * This module binds to the "arm,vexpress-muxfpga" device on the
+ * Versatile Express configuration bus and sets up which CLCD instance
+ * gets muxed out on the DVI bridge.
+ */
+#include <linux/device.h>
+#include <linux/module.h>
+#include <linux/regmap.h>
+#include <linux/vexpress.h>
+#include <linux/platform_device.h>
+#include <linux/of.h>
+#include <linux/of_address.h>
+#include <linux/of_platform.h>
+#include "pl111_drm.h"
+#include "pl111_vexpress.h"
+
+#define VEXPRESS_FPGAMUX_MOTHERBOARD		0x00
+#define VEXPRESS_FPGAMUX_DAUGHTERBOARD_1	0x01
+#define VEXPRESS_FPGAMUX_DAUGHTERBOARD_2	0x02
+
+static bool daughterboard_muxed = false;
+static bool motherboard_muxed = false;
+
+int pl111_vexpress_clcd_init(struct device *dev,
+			     struct pl111_drm_dev_private *priv,
+			     struct regmap *map)
+{
+	struct device_node *memory;
+	u32 val;
+	int ret;
+
+	/*
+	 * The CLCD on the motherboard has a special memory region and
+	 * does not make use of CMA. We differentiate between the different
+	 * CLCD controllers using this memory region phandle.
+	 */
+	memory = of_parse_phandle(dev->of_node, "memory-region", 0);
+	if (!memory) {
+		if (motherboard_muxed) {
+			dev_info(dev, "motherboard CLCD muxed in\n");
+			dev_info(dev, "daughterboard takes precedence\n");
+			dev_info(dev, "motherboard CLCD will not be muxed out\n");
+		}
+		dev_info(dev,
+			 "DVI muxed to daughterboard 1 (core tile) CLCD\n");
+		val = VEXPRESS_FPGAMUX_DAUGHTERBOARD_1;
+		daughterboard_muxed = true;
+	} else {
+		priv->clcd_memory = of_iomap(memory, 0);
+		if (!priv->clcd_memory)
+			dev_err(dev, "could not remap CLCD memory\n");
+		/* Fall through and try to use CMA */
+		if (daughterboard_muxed) {
+			dev_info(dev, "daughterboard takes precedence\n");
+			dev_info(dev, "motherboard CLCD will not be muxed out\n");
+			return -ENODEV;
+		}
+		dev_info(dev, "DVI muxed to motherboard CLCD\n");
+		val = VEXPRESS_FPGAMUX_MOTHERBOARD;
+		motherboard_muxed = true;
+	}
+
+	ret = regmap_write(map, 0, val);
+	if (ret) {
+		dev_err(dev, "error setting DVI muxmode\n");
+		return -ENODEV;
+	}
+
+	return 0;
+}
+
+/*
+ * This sets up the regmap pointer that will then be retrieved by
+ * the detection code in pl111_versatile.c and passed in to the
+ * pl111_vexpress_clcd_init() function above.
+ */
+static int vexpress_muxfpga_probe(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	struct regmap *map;
+
+	map = devm_regmap_init_vexpress_config(&pdev->dev);
+	if (IS_ERR(map))
+		return PTR_ERR(map);
+	dev_set_drvdata(dev, map);
+
+	return 0;
+}
+
+static const struct of_device_id vexpress_muxfpga_match[] = {
+	{ .compatible = "arm,vexpress-muxfpga", }
+};
+
+static struct platform_driver vexpress_muxfpga_driver = {
+	.driver = {
+		.name = "vexpress-muxfpga",
+		.of_match_table = of_match_ptr(vexpress_muxfpga_match),
+	},
+	.probe = vexpress_muxfpga_probe,
+};
+
+module_platform_driver(vexpress_muxfpga_driver);
diff --git a/drivers/gpu/drm/pl111/pl111_vexpress.h b/drivers/gpu/drm/pl111/pl111_vexpress.h
new file mode 100644
index 000000000000..49876417f7b6
--- /dev/null
+++ b/drivers/gpu/drm/pl111/pl111_vexpress.h
@@ -0,0 +1,22 @@
+// SPDX-License-Identifier: GPL-2.0
+
+struct device;
+struct pl111_drm_dev_private;
+struct regmap;
+
+#ifdef CONFIG_ARCH_VEXPRESS
+
+int pl111_vexpress_clcd_init(struct device *dev,
+			     struct pl111_drm_dev_private *priv,
+			     struct regmap *map);
+
+#else
+
+static int inline pl111_vexpress_clcd_init(struct device *dev,
+					   struct pl111_drm_dev_private *priv,
+					   struct regmap *map)
+{
+	return -ENODEV;
+}
+
+#endif
-- 
2.14.3

[PATCH 2/2 v2] drm/pl111: Enable device-specific assigned memory

[Linus Walleij]

The Versatile Express has 8 MB of dedicated video RAM (VRAM)
on the motherboard, which is what we should be using for the
PL111 if available. On this platform, the memory backplane
is constructed so that only this memory will work properly
with the CLCD on the motherboard, using any other memory
region just gives random snow on the display.

The CA9 Versatile Express also has a PL111 instance on its
core tile. This is OK, it has been tested with the motherboard
VRAM and that works just as fine as regular CMA memory.

The memory is assigned to the device using the memory-region
device tree property and a "shared-dma-pool" reserved
memory pool like this:

reserved-memory {
        #address-cells = <1>;
        #size-cells = <1>;
        ranges;

        vram: vram at 48000000 {
                compatible = "shared-dma-pool";
                reg = <0x48000000 0x00800000>;
                no-map;
        };
};

clcd at 1f000 {
        compatible = "arm,pl111", "arm,primecell";
	(...)
        memory-region = <&vram>;
}?;

Cc: Liviu Dudau <liviu.dudau@arm.com>
Cc: Mali DP Maintainers <malidp@foss.arm.com>
Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
---
ChangeLog v1->v2:
- Make sure to also call of_reserved_mem_device_release() at
  remove() and errorpath.
---
 drivers/gpu/drm/pl111/pl111_drv.c | 13 ++++++++++++-
 1 file changed, 12 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/pl111/pl111_drv.c b/drivers/gpu/drm/pl111/pl111_drv.c
index 4621259d5387..bc57c15d9fe4 100644
--- a/drivers/gpu/drm/pl111/pl111_drv.c
+++ b/drivers/gpu/drm/pl111/pl111_drv.c
@@ -60,6 +60,7 @@
 #include <linux/slab.h>
 #include <linux/of.h>
 #include <linux/of_graph.h>
+#include <linux/of_reserved_mem.h>
 
 #include <drm/drmP.h>
 #include <drm/drm_atomic_helper.h>
@@ -257,6 +258,10 @@ static int pl111_amba_probe(struct amba_device *amba_dev,
 	drm->dev_private = priv;
 	priv->variant = variant;
 
+	ret = of_reserved_mem_device_init(dev);
+	if (!ret)
+		dev_info(dev, "using device-specific reserved memory\n");
+
 	if (of_property_read_u32(dev->of_node, "max-memory-bandwidth",
 				 &priv->memory_bw)) {
 		dev_info(dev, "no max memory bandwidth specified, assume unlimited\n");
@@ -275,7 +280,8 @@ static int pl111_amba_probe(struct amba_device *amba_dev,
 	priv->regs = devm_ioremap_resource(dev, &amba_dev->res);
 	if (IS_ERR(priv->regs)) {
 		dev_err(dev, "%s failed mmio\n", __func__);
-		return PTR_ERR(priv->regs);
+		ret = PTR_ERR(priv->regs);
+		goto mem_rel;
 	}
 
 	/* This may override some variant settings */
@@ -305,11 +311,15 @@ static int pl111_amba_probe(struct amba_device *amba_dev,
 
 dev_unref:
 	drm_dev_unref(drm);
+mem_rel:
+	of_reserved_mem_device_release(dev);
+
 	return ret;
 }
 
 static int pl111_amba_remove(struct amba_device *amba_dev)
 {
+	struct device *dev = &amba_dev->dev;
 	struct drm_device *drm = amba_get_drvdata(amba_dev);
 	struct pl111_drm_dev_private *priv = drm->dev_private;
 
@@ -319,6 +329,7 @@ static int pl111_amba_remove(struct amba_device *amba_dev)
 		drm_panel_bridge_remove(priv->bridge);
 	drm_mode_config_cleanup(drm);
 	drm_dev_unref(drm);
+	of_reserved_mem_device_release(dev);
 
 	return 0;
 }
-- 
2.14.3

[PATCH 2/2 v2] drm/pl111: Enable device-specific assigned memory

[Eric Anholt]

Linus Walleij <linus.walleij@linaro.org> writes:

> The Versatile Express has 8 MB of dedicated video RAM (VRAM)
> on the motherboard, which is what we should be using for the
> PL111 if available. On this platform, the memory backplane
> is constructed so that only this memory will work properly
> with the CLCD on the motherboard, using any other memory
> region just gives random snow on the display.
>
> The CA9 Versatile Express also has a PL111 instance on its
> core tile. This is OK, it has been tested with the motherboard
> VRAM and that works just as fine as regular CMA memory.
>
> The memory is assigned to the device using the memory-region
> device tree property and a "shared-dma-pool" reserved
> memory pool like this:
>
> reserved-memory {
>         #address-cells = <1>;
>         #size-cells = <1>;
>         ranges;
>
>         vram: vram at 48000000 {
>                 compatible = "shared-dma-pool";
>                 reg = <0x48000000 0x00800000>;
>                 no-map;
>         };
> };
>
> clcd at 1f000 {
>         compatible = "arm,pl111", "arm,primecell";
> 	(...)
>         memory-region = <&vram>;
> }?;
>
> Cc: Liviu Dudau <liviu.dudau@arm.com>
> Cc: Mali DP Maintainers <malidp@foss.arm.com>
> Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
> ---
> ChangeLog v1->v2:
> - Make sure to also call of_reserved_mem_device_release() at
>   remove() and errorpath.
> ---
>  drivers/gpu/drm/pl111/pl111_drv.c | 13 ++++++++++++-
>  1 file changed, 12 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/pl111/pl111_drv.c b/drivers/gpu/drm/pl111/pl111_drv.c
> index 4621259d5387..bc57c15d9fe4 100644
> --- a/drivers/gpu/drm/pl111/pl111_drv.c
> +++ b/drivers/gpu/drm/pl111/pl111_drv.c
> @@ -60,6 +60,7 @@
>  #include <linux/slab.h>
>  #include <linux/of.h>
>  #include <linux/of_graph.h>
> +#include <linux/of_reserved_mem.h>
>  
>  #include <drm/drmP.h>
>  #include <drm/drm_atomic_helper.h>
> @@ -257,6 +258,10 @@ static int pl111_amba_probe(struct amba_device *amba_dev,
>  	drm->dev_private = priv;
>  	priv->variant = variant;
>  
> +	ret = of_reserved_mem_device_init(dev);
> +	if (!ret)
> +		dev_info(dev, "using device-specific reserved memory\n");
> +
>  	if (of_property_read_u32(dev->of_node, "max-memory-bandwidth",
>  				 &priv->memory_bw)) {
>  		dev_info(dev, "no max memory bandwidth specified, assume unlimited\n");
> @@ -275,7 +280,8 @@ static int pl111_amba_probe(struct amba_device *amba_dev,
>  	priv->regs = devm_ioremap_resource(dev, &amba_dev->res);
>  	if (IS_ERR(priv->regs)) {
>  		dev_err(dev, "%s failed mmio\n", __func__);
> -		return PTR_ERR(priv->regs);
> +		ret = PTR_ERR(priv->regs);
> +		goto mem_rel;
>  	}

Shouldn't this error path be jumping to dev_unref, as well?  We're
trying to match drm_dev_alloc(), right?

I'm still a little bothered that we're allowing imports to pl111 of CMA
buffers that we can't scan out.  Could we just refuse all
.gem_prime_import*() on this platform?
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 832 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20180407/792f9cc3/attachment.sig>

[PATCH 2/2 v2] drm/pl111: Enable device-specific assigned memory

[Linus Walleij]

On Sun, Apr 8, 2018 at 3:08 AM, Eric Anholt <eric@anholt.net> wrote:

>>       if (of_property_read_u32(dev->of_node, "max-memory-bandwidth",
>>                                &priv->memory_bw)) {
>>               dev_info(dev, "no max memory bandwidth specified, assume unlimited\n");
>> @@ -275,7 +280,8 @@ static int pl111_amba_probe(struct amba_device *amba_dev,
>>       priv->regs = devm_ioremap_resource(dev, &amba_dev->res);
>>       if (IS_ERR(priv->regs)) {
>>               dev_err(dev, "%s failed mmio\n", __func__);
>> -             return PTR_ERR(priv->regs);
>> +             ret = PTR_ERR(priv->regs);
>> +             goto mem_rel;
>>       }
>
> Shouldn't this error path be jumping to dev_unref, as well?  We're
> trying to match drm_dev_alloc(), right?

OK I fixed.

> I'm still a little bothered that we're allowing imports to pl111 of CMA
> buffers that we can't scan out.  Could we just refuse all
> .gem_prime_import*() on this platform?

I am sorry but I do not understand how CMA, dmabuf and GEM play
together to decode this and figure out what to do.

Do you mean that if we find device assigned memory, i.e. that there
is this special memory which is all the controller can scan out,
I should just implement .gem_prime_impport() instead of the
currently assigned drm_gem_prime_import() to something just
returning return ERR_PTR(-EINVAL); so it always fails?

What about .gem_prime_import_sg_table()? Exporting should
work fine since the memory is always readable by CPU.

Yours,
Linus Walleij

[PATCH 2/2 v2] drm/pl111: Enable device-specific assigned memory

[Eric Anholt]

Linus Walleij <linus.walleij@linaro.org> writes:

> On Sun, Apr 8, 2018 at 3:08 AM, Eric Anholt <eric@anholt.net> wrote:
>
>>>       if (of_property_read_u32(dev->of_node, "max-memory-bandwidth",
>>>                                &priv->memory_bw)) {
>>>               dev_info(dev, "no max memory bandwidth specified, assume unlimited\n");
>>> @@ -275,7 +280,8 @@ static int pl111_amba_probe(struct amba_device *amba_dev,
>>>       priv->regs = devm_ioremap_resource(dev, &amba_dev->res);
>>>       if (IS_ERR(priv->regs)) {
>>>               dev_err(dev, "%s failed mmio\n", __func__);
>>> -             return PTR_ERR(priv->regs);
>>> +             ret = PTR_ERR(priv->regs);
>>> +             goto mem_rel;
>>>       }
>>
>> Shouldn't this error path be jumping to dev_unref, as well?  We're
>> trying to match drm_dev_alloc(), right?
>
> OK I fixed.
>
>> I'm still a little bothered that we're allowing imports to pl111 of CMA
>> buffers that we can't scan out.  Could we just refuse all
>> .gem_prime_import*() on this platform?
>
> I am sorry but I do not understand how CMA, dmabuf and GEM play
> together to decode this and figure out what to do.
>
> Do you mean that if we find device assigned memory, i.e. that there
> is this special memory which is all the controller can scan out,
> I should just implement .gem_prime_impport() instead of the
> currently assigned drm_gem_prime_import() to something just
> returning return ERR_PTR(-EINVAL); so it always fails?
>
> What about .gem_prime_import_sg_table()? Exporting should
> work fine since the memory is always readable by CPU.

Oh, I think you still want drm_gem_prime_import()'s path for "I'm
importing an fd from this same driver to into another process".  So,
yeah, have our .import_sg_table hook throw -EINVAL if called on the
device memory platform, and otherwise call down to
drm_gem_cma_prime_import_sg_table().
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 832 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20180417/ae011011/attachment.sig>

[PATCH 1/2 v2] drm/pl111: Support the Versatile Express

[Eric Anholt]

Linus Walleij <linus.walleij@linaro.org> writes:

> The Versatile Express uses a special configuration controller
> deeply embedded in the system motherboard FPGA to multiplex the
> two to three (!) CLCD instances out to the single SiI9022
> bridge.
>
> Set up an extra file with the logic to probe to the FPGA mux
> register on the system controller bus, then parse the memory
> range argument to figure out what path the CLCD signal is
> actually taking, and set up the mux accordingly.
>
> If there is a CLCD instance on the core tile (the daughterboard
> with the CPUs fitted) then that CLCD instance will take
> precedence since it can address all memory.
>
> Scale down the Versatile Express to 16BPP so we can support a
> 1024x768 display despite the bus bandwidth restrictions on this
> platform.
>
> Cc: Liviu Dudau <liviu.dudau@arm.com>
> Cc: Pawel Moll <pawel.moll@arm.com>
> Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
> ---
> ChangeLog v1->v2:
> - No changes just reposting rebased on mainline changes.
> ---
>  drivers/gpu/drm/pl111/Makefile          |   1 +
>  drivers/gpu/drm/pl111/pl111_drm.h       |   3 +-
>  drivers/gpu/drm/pl111/pl111_versatile.c |  48 ++++++++++++++-
>  drivers/gpu/drm/pl111/pl111_vexpress.c  | 106 ++++++++++++++++++++++++++++++++
>  drivers/gpu/drm/pl111/pl111_vexpress.h  |  22 +++++++
>  5 files changed, 178 insertions(+), 2 deletions(-)
>  create mode 100644 drivers/gpu/drm/pl111/pl111_vexpress.c
>  create mode 100644 drivers/gpu/drm/pl111/pl111_vexpress.h
>
> diff --git a/drivers/gpu/drm/pl111/Makefile b/drivers/gpu/drm/pl111/Makefile
> index 9c5e8dba8ac6..19a8189dc54f 100644
> --- a/drivers/gpu/drm/pl111/Makefile
> +++ b/drivers/gpu/drm/pl111/Makefile
> @@ -3,6 +3,7 @@ pl111_drm-y +=	pl111_display.o \
>  		pl111_versatile.o \
>  		pl111_drv.o
>  
> +pl111_drm-$(CONFIG_ARCH_VEXPRESS) += pl111_vexpress.o
>  pl111_drm-$(CONFIG_DEBUG_FS) += pl111_debugfs.o
>  
>  obj-$(CONFIG_DRM_PL111) += pl111_drm.o
> diff --git a/drivers/gpu/drm/pl111/pl111_drm.h b/drivers/gpu/drm/pl111/pl111_drm.h
> index 8639b2d4ddf7..916154ac733a 100644
> --- a/drivers/gpu/drm/pl111/pl111_drm.h
> +++ b/drivers/gpu/drm/pl111/pl111_drm.h
> @@ -64,7 +64,8 @@ struct pl111_drm_dev_private {
>  	struct drm_bridge *bridge;
>  	struct drm_simple_display_pipe pipe;
>  
> -	void *regs;
> +	void __iomem *clcd_memory;

This doesn't seem to be used anywhere.

> diff --git a/drivers/gpu/drm/pl111/pl111_versatile.c b/drivers/gpu/drm/pl111/pl111_versatile.c
> index 9302f516045e..569edf02a36a 100644
> --- a/drivers/gpu/drm/pl111/pl111_versatile.c
> +++ b/drivers/gpu/drm/pl111/pl111_versatile.c
> @@ -1,12 +1,14 @@
>  #include <linux/amba/clcd-regs.h>
>  #include <linux/device.h>
>  #include <linux/of.h>
> +#include <linux/of_platform.h>
>  #include <linux/regmap.h>
>  #include <linux/mfd/syscon.h>
>  #include <linux/bitops.h>
>  #include <linux/module.h>
>  #include <drm/drmP.h>
>  #include "pl111_versatile.h"
> +#include "pl111_vexpress.h"
>  #include "pl111_drm.h"
>  
>  static struct regmap *versatile_syscon_map;
> @@ -22,6 +24,7 @@ enum versatile_clcd {
>  	REALVIEW_CLCD_PB11MP,
>  	REALVIEW_CLCD_PBA8,
>  	REALVIEW_CLCD_PBX,
> +	VEXPRESS_CLCD_V2M,
>  };
>  
>  static const struct of_device_id versatile_clcd_of_match[] = {
> @@ -53,6 +56,10 @@ static const struct of_device_id versatile_clcd_of_match[] = {
>  		.compatible = "arm,realview-pbx-syscon",
>  		.data = (void *)REALVIEW_CLCD_PBX,
>  	},
> +	{
> +		.compatible = "arm,vexpress-muxfpga",
> +		.data = (void *)VEXPRESS_CLCD_V2M,
> +	},
>  	{},
>  };
>  
> @@ -286,12 +293,26 @@ static const struct pl111_variant_data pl111_realview = {
>  	.fb_bpp = 16,
>  };
>  
> +/*
> + * Versatile Express PL111 variant, again we just push the maximum
> + * BPP to 16 to be able to get 1024x768 without saturating the memory
> + * bus. The clockdivider also seems broken on the Versatile Express.
> + */
> +static const struct pl111_variant_data pl111_vexpress = {
> +	.name = "PL111 Versatile Express",
> +	.formats = pl111_realview_pixel_formats,
> +	.nformats = ARRAY_SIZE(pl111_realview_pixel_formats),
> +	.fb_bpp = 16,
> +	.broken_clockdivider = true,
> +};
> +
>  int pl111_versatile_init(struct device *dev, struct pl111_drm_dev_private *priv)
>  {
>  	const struct of_device_id *clcd_id;
>  	enum versatile_clcd versatile_clcd_type;
>  	struct device_node *np;
>  	struct regmap *map;
> +	int ret;
>  
>  	np = of_find_matching_node_and_match(NULL, versatile_clcd_of_match,
>  					     &clcd_id);
> @@ -301,7 +322,25 @@ int pl111_versatile_init(struct device *dev, struct pl111_drm_dev_private *priv)
>  	}
>  	versatile_clcd_type = (enum versatile_clcd)clcd_id->data;
>  
> -	map = syscon_node_to_regmap(np);
> +	/* Versatile Express special handling */
> +	if (versatile_clcd_type == VEXPRESS_CLCD_V2M) {
> +		struct platform_device *pdev;
> +
> +		/* Call into deep Vexpress configuration API */
> +		pdev = of_find_device_by_node(np);
> +		if (!pdev) {
> +			dev_err(dev, "can't find the sysreg device, deferring\n");
> +			return -EPROBE_DEFER;
> +		}
> +		map = dev_get_drvdata(&pdev->dev);
> +		if (!map) {
> +			dev_err(dev, "sysreg has not yet probed\n");
> +			return -EPROBE_DEFER;
> +		}
> +	} else {
> +		map = syscon_node_to_regmap(np);
> +	}
> +
>  	if (IS_ERR(map)) {
>  		dev_err(dev, "no Versatile syscon regmap\n");
>  		return PTR_ERR(map);
> @@ -340,6 +379,13 @@ int pl111_versatile_init(struct device *dev, struct pl111_drm_dev_private *priv)
>  		priv->variant_display_disable = pl111_realview_clcd_disable;
>  		dev_info(dev, "set up callbacks for RealView PL111\n");
>  		break;
> +	case VEXPRESS_CLCD_V2M:
> +		priv->variant = &pl111_vexpress;
> +		dev_info(dev, "initializing Versatile Express PL111\n");
> +		ret = pl111_vexpress_clcd_init(dev, priv, map);
> +		if (ret)
> +			return ret;
> +		break;
>  	default:
>  		dev_info(dev, "unknown Versatile system controller\n");
>  		break;
> diff --git a/drivers/gpu/drm/pl111/pl111_vexpress.c b/drivers/gpu/drm/pl111/pl111_vexpress.c
> new file mode 100644
> index 000000000000..720244f497fe
> --- /dev/null
> +++ b/drivers/gpu/drm/pl111/pl111_vexpress.c
> @@ -0,0 +1,106 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Versatile Express PL111 handling
> + * Copyright (C) 2018 Linus Walleij
> + *
> + * This module binds to the "arm,vexpress-muxfpga" device on the
> + * Versatile Express configuration bus and sets up which CLCD instance
> + * gets muxed out on the DVI bridge.
> + */
> +#include <linux/device.h>
> +#include <linux/module.h>
> +#include <linux/regmap.h>
> +#include <linux/vexpress.h>
> +#include <linux/platform_device.h>
> +#include <linux/of.h>
> +#include <linux/of_address.h>
> +#include <linux/of_platform.h>
> +#include "pl111_drm.h"
> +#include "pl111_vexpress.h"
> +
> +#define VEXPRESS_FPGAMUX_MOTHERBOARD		0x00
> +#define VEXPRESS_FPGAMUX_DAUGHTERBOARD_1	0x01
> +#define VEXPRESS_FPGAMUX_DAUGHTERBOARD_2	0x02
> +
> +static bool daughterboard_muxed = false;
> +static bool motherboard_muxed = false;
> +
> +int pl111_vexpress_clcd_init(struct device *dev,
> +			     struct pl111_drm_dev_private *priv,
> +			     struct regmap *map)
> +{
> +	struct device_node *memory;
> +	u32 val;
> +	int ret;
> +
> +	/*
> +	 * The CLCD on the motherboard has a special memory region and
> +	 * does not make use of CMA. We differentiate between the different
> +	 * CLCD controllers using this memory region phandle.
> +	 */
> +	memory = of_parse_phandle(dev->of_node, "memory-region", 0);
> +	if (!memory) {
> +		if (motherboard_muxed) {
> +			dev_info(dev, "motherboard CLCD muxed in\n");
> +			dev_info(dev, "daughterboard takes precedence\n");
> +			dev_info(dev, "motherboard CLCD will not be muxed out\n");
> +		}
> +		dev_info(dev,
> +			 "DVI muxed to daughterboard 1 (core tile) CLCD\n");
> +		val = VEXPRESS_FPGAMUX_DAUGHTERBOARD_1;
> +		daughterboard_muxed = true;
> +	} else {
> +		priv->clcd_memory = of_iomap(memory, 0);
> +		if (!priv->clcd_memory)
> +			dev_err(dev, "could not remap CLCD memory\n");
> +		/* Fall through and try to use CMA */
> +		if (daughterboard_muxed) {
> +			dev_info(dev, "daughterboard takes precedence\n");
> +			dev_info(dev, "motherboard CLCD will not be muxed out\n");
> +			return -ENODEV;
> +		}
> +		dev_info(dev, "DVI muxed to motherboard CLCD\n");
> +		val = VEXPRESS_FPGAMUX_MOTHERBOARD;
> +		motherboard_muxed = true;
> +	}

I'm confused by global bools here.  It seems like we're trying to
coordinate between the instantiation of multiple CLCDs here, to set up
the mux appropriately.  Does it make sense to do things that way, if
probe order is unreliable?
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 832 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20180407/c021d538/attachment-0001.sig>

[PATCH 1/2 v2] drm/pl111: Support the Versatile Express

[Linus Walleij]

On Sun, Apr 8, 2018 at 3:16 AM, Eric Anholt <eric@anholt.net> wrote:
> Linus Walleij <linus.walleij@linaro.org> writes:

>> -     void *regs;
>> +     void __iomem *clcd_memory;
>
> This doesn't seem to be used anywhere.

Ah development artifact, I'll get rid of it.

>> +int pl111_vexpress_clcd_init(struct device *dev,
>> +                          struct pl111_drm_dev_private *priv,
>> +                          struct regmap *map)
>> +{
>> +     struct device_node *memory;
>> +     u32 val;
>> +     int ret;
>> +
>> +     /*
>> +      * The CLCD on the motherboard has a special memory region and
>> +      * does not make use of CMA. We differentiate between the different
>> +      * CLCD controllers using this memory region phandle.
>> +      */
>> +     memory = of_parse_phandle(dev->of_node, "memory-region", 0);
>> +     if (!memory) {
>> +             if (motherboard_muxed) {
>> +                     dev_info(dev, "motherboard CLCD muxed in\n");
>> +                     dev_info(dev, "daughterboard takes precedence\n");
>> +                     dev_info(dev, "motherboard CLCD will not be muxed out\n");
>> +             }
>> +             dev_info(dev,
>> +                      "DVI muxed to daughterboard 1 (core tile) CLCD\n");
>> +             val = VEXPRESS_FPGAMUX_DAUGHTERBOARD_1;
>> +             daughterboard_muxed = true;
>> +     } else {
>> +             priv->clcd_memory = of_iomap(memory, 0);
>> +             if (!priv->clcd_memory)
>> +                     dev_err(dev, "could not remap CLCD memory\n");
>> +             /* Fall through and try to use CMA */
>> +             if (daughterboard_muxed) {
>> +                     dev_info(dev, "daughterboard takes precedence\n");
>> +                     dev_info(dev, "motherboard CLCD will not be muxed out\n");
>> +                     return -ENODEV;
>> +             }
>> +             dev_info(dev, "DVI muxed to motherboard CLCD\n");
>> +             val = VEXPRESS_FPGAMUX_MOTHERBOARD;
>> +             motherboard_muxed = true;
>> +     }
>
> I'm confused by global bools here.  It seems like we're trying to
> coordinate between the instantiation of multiple CLCDs here, to set up
> the mux appropriately.  Does it make sense to do things that way, if
> probe order is unreliable?

Hm I guess I should parse the graph and see what is connected instead,
and bail out if we have two CLCDs and are not the desired one.

I'll go and poke at it.

Yours,
Linus Walleij

[PATCH 1/2 v2] drm/pl111: Support the Versatile Express

[Liviu Dudau]

Hi Linus,

I need to do a bit of archeology to get my Versatile Express back
to working order in order to test your patches, which might not
happen this week or so, but I have some comments on your approach
which I would like clarification on.

On Fri, Apr 06, 2018 at 04:19:34PM +0200, Linus Walleij wrote:
> The Versatile Express uses a special configuration controller
> deeply embedded in the system motherboard FPGA to multiplex the
> two to three (!) CLCD instances out to the single SiI9022
> bridge.

That configuration controller is already supported by the
drivers/misc/vexpress-syscfg.c driver and the muxfpga node is present
in arch/arm/boot/dts/vexpress-v2m-rs1.dtsi (and vexpress-v2m.dtsi for
older CoreTiles that don't use the RS1 memory map).

> 
> Set up an extra file with the logic to probe to the FPGA mux
> register on the system controller bus, then parse the memory
> range argument to figure out what path the CLCD signal is
> actually taking, and set up the mux accordingly.

I'm not sure you need all this extra code, I would have thought that
all you want is to use the regmap that you already have in order to
switch the MUX to the correct input. The MUXFPGA in the DT is already
setup to use function 7 (which is the function you need for FPGA), you
just need a property in the PL111 node to tell you which "site" you are
on (Motherboard, Daughterboard 1 or 2) and use the relevant value when
setting the regmap.

Info on what the correct values are are here:
http://infocenter.arm.com/help/topic/com.arm.doc.dui0447j/CACDEFGH.html#CACCGJFF

> 
> If there is a CLCD instance on the core tile (the daughterboard
> with the CPUs fitted) then that CLCD instance will take
> precedence since it can address all memory.

What does "take precedence" mean here? You mean prefer to switch the MUX
to that source instead of using the MB? That should always be the
default choice, as the MB CLCD is mostly for boot up info being
displayed and it is known to have horrendous performance.

Best regards,
Liviu

> 
> Scale down the Versatile Express to 16BPP so we can support a
> 1024x768 display despite the bus bandwidth restrictions on this
> platform.
> 
> Cc: Liviu Dudau <liviu.dudau@arm.com>
> Cc: Pawel Moll <pawel.moll@arm.com>
> Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
> ---
> ChangeLog v1->v2:
> - No changes just reposting rebased on mainline changes.
> ---
>  drivers/gpu/drm/pl111/Makefile          |   1 +
>  drivers/gpu/drm/pl111/pl111_drm.h       |   3 +-
>  drivers/gpu/drm/pl111/pl111_versatile.c |  48 ++++++++++++++-
>  drivers/gpu/drm/pl111/pl111_vexpress.c  | 106 ++++++++++++++++++++++++++++++++
>  drivers/gpu/drm/pl111/pl111_vexpress.h  |  22 +++++++
>  5 files changed, 178 insertions(+), 2 deletions(-)
>  create mode 100644 drivers/gpu/drm/pl111/pl111_vexpress.c
>  create mode 100644 drivers/gpu/drm/pl111/pl111_vexpress.h
> 
> diff --git a/drivers/gpu/drm/pl111/Makefile b/drivers/gpu/drm/pl111/Makefile
> index 9c5e8dba8ac6..19a8189dc54f 100644
> --- a/drivers/gpu/drm/pl111/Makefile
> +++ b/drivers/gpu/drm/pl111/Makefile
> @@ -3,6 +3,7 @@ pl111_drm-y +=	pl111_display.o \
>  		pl111_versatile.o \
>  		pl111_drv.o
>  
> +pl111_drm-$(CONFIG_ARCH_VEXPRESS) += pl111_vexpress.o
>  pl111_drm-$(CONFIG_DEBUG_FS) += pl111_debugfs.o
>  
>  obj-$(CONFIG_DRM_PL111) += pl111_drm.o
> diff --git a/drivers/gpu/drm/pl111/pl111_drm.h b/drivers/gpu/drm/pl111/pl111_drm.h
> index 8639b2d4ddf7..916154ac733a 100644
> --- a/drivers/gpu/drm/pl111/pl111_drm.h
> +++ b/drivers/gpu/drm/pl111/pl111_drm.h
> @@ -64,7 +64,8 @@ struct pl111_drm_dev_private {
>  	struct drm_bridge *bridge;
>  	struct drm_simple_display_pipe pipe;
>  
> -	void *regs;
> +	void __iomem *clcd_memory;
> +	void __iomem *regs;
>  	u32 memory_bw;
>  	u32 ienb;
>  	u32 ctrl;
> diff --git a/drivers/gpu/drm/pl111/pl111_versatile.c b/drivers/gpu/drm/pl111/pl111_versatile.c
> index 9302f516045e..569edf02a36a 100644
> --- a/drivers/gpu/drm/pl111/pl111_versatile.c
> +++ b/drivers/gpu/drm/pl111/pl111_versatile.c
> @@ -1,12 +1,14 @@
>  #include <linux/amba/clcd-regs.h>
>  #include <linux/device.h>
>  #include <linux/of.h>
> +#include <linux/of_platform.h>
>  #include <linux/regmap.h>
>  #include <linux/mfd/syscon.h>
>  #include <linux/bitops.h>
>  #include <linux/module.h>
>  #include <drm/drmP.h>
>  #include "pl111_versatile.h"
> +#include "pl111_vexpress.h"
>  #include "pl111_drm.h"
>  
>  static struct regmap *versatile_syscon_map;
> @@ -22,6 +24,7 @@ enum versatile_clcd {
>  	REALVIEW_CLCD_PB11MP,
>  	REALVIEW_CLCD_PBA8,
>  	REALVIEW_CLCD_PBX,
> +	VEXPRESS_CLCD_V2M,
>  };
>  
>  static const struct of_device_id versatile_clcd_of_match[] = {
> @@ -53,6 +56,10 @@ static const struct of_device_id versatile_clcd_of_match[] = {
>  		.compatible = "arm,realview-pbx-syscon",
>  		.data = (void *)REALVIEW_CLCD_PBX,
>  	},
> +	{
> +		.compatible = "arm,vexpress-muxfpga",
> +		.data = (void *)VEXPRESS_CLCD_V2M,
> +	},
>  	{},
>  };
>  
> @@ -286,12 +293,26 @@ static const struct pl111_variant_data pl111_realview = {
>  	.fb_bpp = 16,
>  };
>  
> +/*
> + * Versatile Express PL111 variant, again we just push the maximum
> + * BPP to 16 to be able to get 1024x768 without saturating the memory
> + * bus. The clockdivider also seems broken on the Versatile Express.
> + */
> +static const struct pl111_variant_data pl111_vexpress = {
> +	.name = "PL111 Versatile Express",
> +	.formats = pl111_realview_pixel_formats,
> +	.nformats = ARRAY_SIZE(pl111_realview_pixel_formats),
> +	.fb_bpp = 16,
> +	.broken_clockdivider = true,
> +};
> +
>  int pl111_versatile_init(struct device *dev, struct pl111_drm_dev_private *priv)
>  {
>  	const struct of_device_id *clcd_id;
>  	enum versatile_clcd versatile_clcd_type;
>  	struct device_node *np;
>  	struct regmap *map;
> +	int ret;
>  
>  	np = of_find_matching_node_and_match(NULL, versatile_clcd_of_match,
>  					     &clcd_id);
> @@ -301,7 +322,25 @@ int pl111_versatile_init(struct device *dev, struct pl111_drm_dev_private *priv)
>  	}
>  	versatile_clcd_type = (enum versatile_clcd)clcd_id->data;
>  
> -	map = syscon_node_to_regmap(np);
> +	/* Versatile Express special handling */
> +	if (versatile_clcd_type == VEXPRESS_CLCD_V2M) {
> +		struct platform_device *pdev;
> +
> +		/* Call into deep Vexpress configuration API */
> +		pdev = of_find_device_by_node(np);
> +		if (!pdev) {
> +			dev_err(dev, "can't find the sysreg device, deferring\n");
> +			return -EPROBE_DEFER;
> +		}
> +		map = dev_get_drvdata(&pdev->dev);
> +		if (!map) {
> +			dev_err(dev, "sysreg has not yet probed\n");
> +			return -EPROBE_DEFER;
> +		}
> +	} else {
> +		map = syscon_node_to_regmap(np);
> +	}
> +
>  	if (IS_ERR(map)) {
>  		dev_err(dev, "no Versatile syscon regmap\n");
>  		return PTR_ERR(map);
> @@ -340,6 +379,13 @@ int pl111_versatile_init(struct device *dev, struct pl111_drm_dev_private *priv)
>  		priv->variant_display_disable = pl111_realview_clcd_disable;
>  		dev_info(dev, "set up callbacks for RealView PL111\n");
>  		break;
> +	case VEXPRESS_CLCD_V2M:
> +		priv->variant = &pl111_vexpress;
> +		dev_info(dev, "initializing Versatile Express PL111\n");
> +		ret = pl111_vexpress_clcd_init(dev, priv, map);
> +		if (ret)
> +			return ret;
> +		break;
>  	default:
>  		dev_info(dev, "unknown Versatile system controller\n");
>  		break;
> diff --git a/drivers/gpu/drm/pl111/pl111_vexpress.c b/drivers/gpu/drm/pl111/pl111_vexpress.c
> new file mode 100644
> index 000000000000..720244f497fe
> --- /dev/null
> +++ b/drivers/gpu/drm/pl111/pl111_vexpress.c
> @@ -0,0 +1,106 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Versatile Express PL111 handling
> + * Copyright (C) 2018 Linus Walleij
> + *
> + * This module binds to the "arm,vexpress-muxfpga" device on the
> + * Versatile Express configuration bus and sets up which CLCD instance
> + * gets muxed out on the DVI bridge.
> + */
> +#include <linux/device.h>
> +#include <linux/module.h>
> +#include <linux/regmap.h>
> +#include <linux/vexpress.h>
> +#include <linux/platform_device.h>
> +#include <linux/of.h>
> +#include <linux/of_address.h>
> +#include <linux/of_platform.h>
> +#include "pl111_drm.h"
> +#include "pl111_vexpress.h"
> +
> +#define VEXPRESS_FPGAMUX_MOTHERBOARD		0x00
> +#define VEXPRESS_FPGAMUX_DAUGHTERBOARD_1	0x01
> +#define VEXPRESS_FPGAMUX_DAUGHTERBOARD_2	0x02
> +
> +static bool daughterboard_muxed = false;
> +static bool motherboard_muxed = false;
> +
> +int pl111_vexpress_clcd_init(struct device *dev,
> +			     struct pl111_drm_dev_private *priv,
> +			     struct regmap *map)
> +{
> +	struct device_node *memory;
> +	u32 val;
> +	int ret;
> +
> +	/*
> +	 * The CLCD on the motherboard has a special memory region and
> +	 * does not make use of CMA. We differentiate between the different
> +	 * CLCD controllers using this memory region phandle.
> +	 */
> +	memory = of_parse_phandle(dev->of_node, "memory-region", 0);
> +	if (!memory) {
> +		if (motherboard_muxed) {
> +			dev_info(dev, "motherboard CLCD muxed in\n");
> +			dev_info(dev, "daughterboard takes precedence\n");
> +			dev_info(dev, "motherboard CLCD will not be muxed out\n");
> +		}
> +		dev_info(dev,
> +			 "DVI muxed to daughterboard 1 (core tile) CLCD\n");
> +		val = VEXPRESS_FPGAMUX_DAUGHTERBOARD_1;
> +		daughterboard_muxed = true;
> +	} else {
> +		priv->clcd_memory = of_iomap(memory, 0);
> +		if (!priv->clcd_memory)
> +			dev_err(dev, "could not remap CLCD memory\n");
> +		/* Fall through and try to use CMA */
> +		if (daughterboard_muxed) {
> +			dev_info(dev, "daughterboard takes precedence\n");
> +			dev_info(dev, "motherboard CLCD will not be muxed out\n");
> +			return -ENODEV;
> +		}
> +		dev_info(dev, "DVI muxed to motherboard CLCD\n");
> +		val = VEXPRESS_FPGAMUX_MOTHERBOARD;
> +		motherboard_muxed = true;
> +	}
> +
> +	ret = regmap_write(map, 0, val);
> +	if (ret) {
> +		dev_err(dev, "error setting DVI muxmode\n");
> +		return -ENODEV;
> +	}
> +
> +	return 0;
> +}
> +
> +/*
> + * This sets up the regmap pointer that will then be retrieved by
> + * the detection code in pl111_versatile.c and passed in to the
> + * pl111_vexpress_clcd_init() function above.
> + */
> +static int vexpress_muxfpga_probe(struct platform_device *pdev)
> +{
> +	struct device *dev = &pdev->dev;
> +	struct regmap *map;
> +
> +	map = devm_regmap_init_vexpress_config(&pdev->dev);
> +	if (IS_ERR(map))
> +		return PTR_ERR(map);
> +	dev_set_drvdata(dev, map);
> +
> +	return 0;
> +}
> +
> +static const struct of_device_id vexpress_muxfpga_match[] = {
> +	{ .compatible = "arm,vexpress-muxfpga", }
> +};
> +
> +static struct platform_driver vexpress_muxfpga_driver = {
> +	.driver = {
> +		.name = "vexpress-muxfpga",
> +		.of_match_table = of_match_ptr(vexpress_muxfpga_match),
> +	},
> +	.probe = vexpress_muxfpga_probe,
> +};
> +
> +module_platform_driver(vexpress_muxfpga_driver);
> diff --git a/drivers/gpu/drm/pl111/pl111_vexpress.h b/drivers/gpu/drm/pl111/pl111_vexpress.h
> new file mode 100644
> index 000000000000..49876417f7b6
> --- /dev/null
> +++ b/drivers/gpu/drm/pl111/pl111_vexpress.h
> @@ -0,0 +1,22 @@
> +// SPDX-License-Identifier: GPL-2.0
> +
> +struct device;
> +struct pl111_drm_dev_private;
> +struct regmap;
> +
> +#ifdef CONFIG_ARCH_VEXPRESS
> +
> +int pl111_vexpress_clcd_init(struct device *dev,
> +			     struct pl111_drm_dev_private *priv,
> +			     struct regmap *map);
> +
> +#else
> +
> +static int inline pl111_vexpress_clcd_init(struct device *dev,
> +					   struct pl111_drm_dev_private *priv,
> +					   struct regmap *map)
> +{
> +	return -ENODEV;
> +}
> +
> +#endif
> -- 
> 2.14.3
> 

-- 
====================
| I would like to |
| fix the world,  |
| but they're not |
| giving me the   |
 \ source code!  /
  ---------------
    ?\_(?)_/?

[PATCH 1/2 v2] drm/pl111: Support the Versatile Express

[Linus Walleij]

On Tue, Apr 10, 2018 at 11:57 AM, Liviu Dudau <liviu.dudau@arm.com> wrote:

> I need to do a bit of archeology to get my Versatile Express back
> to working order in order to test your patches, which might not
> happen this week or so, but I have some comments on your approach
> which I would like clarification on.

OK no hurries :)

> On Fri, Apr 06, 2018 at 04:19:34PM +0200, Linus Walleij wrote:
>>
>> The Versatile Express uses a special configuration controller
>> deeply embedded in the system motherboard FPGA to multiplex the
>> two to three (!) CLCD instances out to the single SiI9022
>> bridge.
>
> That configuration controller is already supported by the
> drivers/misc/vexpress-syscfg.c driver and the muxfpga node is present
> in arch/arm/boot/dts/vexpress-v2m-rs1.dtsi (and vexpress-v2m.dtsi for
> older CoreTiles that don't use the RS1 memory map).

Yes the code is using that driver through the public interface in
<linux/vexpress.h>, so it should be fine.

>> Set up an extra file with the logic to probe to the FPGA mux
>> register on the system controller bus, then parse the memory
>> range argument to figure out what path the CLCD signal is
>> actually taking, and set up the mux accordingly.
>
> I'm not sure you need all this extra code,

I think the commit message may be confusing.... I will try
to reword it.

> I would have thought that
> all you want is to use the regmap that you already have in order to
> switch the MUX to the correct input. The MUXFPGA in the DT is already
> setup to use function 7 (which is the function you need for FPGA), you
> just need a property in the PL111 node to tell you which "site" you are
> on (Motherboard, Daughterboard 1 or 2) and use the relevant value when
> setting the regmap.
>
> Info on what the correct values are are here:
> http://infocenter.arm.com/help/topic/com.arm.doc.dui0447j/CACDEFGH.html#CACCGJFF

Yes this is what I'm using, I am manipulating SYS_CFG_MUXFPGA
through the interface exposed in <linux/vexpress.h>.

>> If there is a CLCD instance on the core tile (the daughterboard
>> with the CPUs fitted) then that CLCD instance will take
>> precedence since it can address all memory.
>
> What does "take precedence" mean here? You mean prefer to switch the MUX
> to that source instead of using the MB? That should always be the
> default choice, as the MB CLCD is mostly for boot up info being
> displayed and it is known to have horrendous performance.

Yes, so that is what I am trying to do. If there is a CLCD on the
core tile (daughterboard as it its called in the manual), that one
should take precedence.

Unfortunately there is just one single vexpress core tile in the
upstream kernel that define a CLCD controller, the CA9 (4xA9)
that I am using. All the others just use the MB CLCD.

I am thinking there is some never finished DTS upstreaming
here that ought to happen so we use the core tile CLCD on some
other boards as well.

Yours,
Linus Walleij

[PATCH 1/2 v2] drm/pl111: Support the Versatile Express

[Robin Murphy]

On 17/04/18 13:32, Linus Walleij wrote:
[...]
> Unfortunately there is just one single vexpress core tile in the
> upstream kernel that define a CLCD controller, the CA9 (4xA9)
> that I am using. All the others just use the MB CLCD.
> 
> I am thinking there is some never finished DTS upstreaming
> here that ought to happen so we use the core tile CLCD on some
> other boards as well.

Barring custom FPGA images, I think V2P-CA9 *is* the only VExpress tile 
implementing PL11x; all the more recent test chips had HDLCD instead.

Robin.

[PATCH 1/2 v2] drm/pl111: Support the Versatile Express

[Linus Walleij]

On Tue, Apr 17, 2018 at 3:12 PM, Robin Murphy <robin.murphy@arm.com> wrote:
> On 17/04/18 13:32, Linus Walleij wrote:
> [...]
>>
>> Unfortunately there is just one single vexpress core tile in the
>> upstream kernel that define a CLCD controller, the CA9 (4xA9)
>> that I am using. All the others just use the MB CLCD.
>>
>> I am thinking there is some never finished DTS upstreaming
>> here that ought to happen so we use the core tile CLCD on some
>> other boards as well.
>
>
> Barring custom FPGA images, I think V2P-CA9 *is* the only VExpress tile
> implementing PL11x; all the more recent test chips had HDLCD instead.

Yup it looks like that.

I am restructuring the code to look for any graphics on the
core tile and only turn on the motherboard CLCD if there is
no CLCD or HDLCD on the core tile, e.g. if that device node has
been set to "disabled" or the HDLCD driver is not compiled in
but the CLCD driver is.

I found some vague mentions of a LCD display that can be
connected to the motherboard, so that would then be sitting
on the CLCD, is this something you ARM guys have seen
around? In that case I guess the VExpress (etc) would actually
be able to have two screens, one LCD panel on the motherboard
CLCD and a DVI from the core tile.

Yours,
Linus Walleij

[PATCH 1/2 v2] drm/pl111: Support the Versatile Express

[Robin Murphy]

On 17/04/18 20:31, Linus Walleij wrote:
> On Tue, Apr 17, 2018 at 3:12 PM, Robin Murphy <robin.murphy@arm.com> wrote:
>> On 17/04/18 13:32, Linus Walleij wrote:
>> [...]
>>>
>>> Unfortunately there is just one single vexpress core tile in the
>>> upstream kernel that define a CLCD controller, the CA9 (4xA9)
>>> that I am using. All the others just use the MB CLCD.
>>>
>>> I am thinking there is some never finished DTS upstreaming
>>> here that ought to happen so we use the core tile CLCD on some
>>> other boards as well.
>>
>>
>> Barring custom FPGA images, I think V2P-CA9 *is* the only VExpress tile
>> implementing PL11x; all the more recent test chips had HDLCD instead.
> 
> Yup it looks like that.
> 
> I am restructuring the code to look for any graphics on the
> core tile and only turn on the motherboard CLCD if there is
> no CLCD or HDLCD on the core tile, e.g. if that device node has
> been set to "disabled" or the HDLCD driver is not compiled in
> but the CLCD driver is.
> 
> I found some vague mentions of a LCD display that can be
> connected to the motherboard, so that would then be sitting
> on the CLCD, is this something you ARM guys have seen
> around? In that case I guess the VExpress (etc) would actually
> be able to have two screens, one LCD panel on the motherboard
> CLCD and a DVI from the core tile.

I don't see anything in the V2M motherboard manual to suggest the IOFPGA 
CLCD output is routed anywhere other than the DVI mux, and there are 
certainly no unaccounted-for connectors on the VExpress version 
currently in front of me (HBI-0190D). The only things I'm immediately 
aware of having a panel connector directly on the board are 
Integrator/CP and RealView EB.

Robin.

[PATCH 1/2 v2] drm/pl111: Support the Versatile Express

[Liviu Dudau]

On Wed, Apr 18, 2018 at 12:50:10PM +0100, Robin Murphy wrote:
> On 17/04/18 20:31, Linus Walleij wrote:
> > On Tue, Apr 17, 2018 at 3:12 PM, Robin Murphy <robin.murphy@arm.com> wrote:
> > > On 17/04/18 13:32, Linus Walleij wrote:
> > > [...]
> > > > 
> > > > Unfortunately there is just one single vexpress core tile in the
> > > > upstream kernel that define a CLCD controller, the CA9 (4xA9)
> > > > that I am using. All the others just use the MB CLCD.
> > > > 
> > > > I am thinking there is some never finished DTS upstreaming
> > > > here that ought to happen so we use the core tile CLCD on some
> > > > other boards as well.
> > > 
> > > 
> > > Barring custom FPGA images, I think V2P-CA9 *is* the only VExpress tile
> > > implementing PL11x; all the more recent test chips had HDLCD instead.
> > 
> > Yup it looks like that.
> > 
> > I am restructuring the code to look for any graphics on the
> > core tile and only turn on the motherboard CLCD if there is
> > no CLCD or HDLCD on the core tile, e.g. if that device node has
> > been set to "disabled" or the HDLCD driver is not compiled in
> > but the CLCD driver is.
> > 
> > I found some vague mentions of a LCD display that can be
> > connected to the motherboard, so that would then be sitting
> > on the CLCD, is this something you ARM guys have seen
> > around? In that case I guess the VExpress (etc) would actually
> > be able to have two screens, one LCD panel on the motherboard
> > CLCD and a DVI from the core tile.
> 
> I don't see anything in the V2M motherboard manual to suggest the IOFPGA
> CLCD output is routed anywhere other than the DVI mux, and there are
> certainly no unaccounted-for connectors on the VExpress version currently in
> front of me (HBI-0190D). The only things I'm immediately aware of having a
> panel connector directly on the board are Integrator/CP and RealView EB.

Yes, I think the last platform that could have had an LCD display
attached directly was the Versatile platform, not the VExpress one.

Regarding the mux and trying to figure out a clever way of deciding
which one should have priority: I would not bother adding that to the
CLCD driver, instead I would expect the bootloader to set the MUX
correctly. Long time ago, before Juno was a thing and the only platform
HDLCD was running on was VExpress, I did have a patch to use
vexpress_config_write() (API before vexpress-config got converted to
regmap) to set the MUX. Then Juno happened and I've realised it was not
worth baking into the driver the existence of the MUX, as it was a quirk
of the board.

Best regards,
Liviu

> 
> Robin.

-- 
====================
| I would like to |
| fix the world,  |
| but they're not |
| giving me the   |
 \ source code!  /
  ---------------
    ?\_(?)_/?