Re: [PATCH v7 3/4 - variant 1] drm: rcar-du: Fix legacy DT to create LVDS encoder nodes

[Laurent Pinchart <laurent.pinchart@ideasonboard.com>]

Hi Niklas,

On Friday, 2 March 2018 02:42:18 EET Niklas Söderlund wrote:
> Hi Laurent,
> 
> Thanks for your patch,
> 
> All comments in this mail also applies to variant 2 of this patch as the
> difference between them are so small.
> 
> First I got a question about your usage of __init  thru out this driver.
> What would happen if a system is booted without a DU node and then a
> overlay is added using the old DU bindings will things work as expected,
> the old style overlay load but DU/LVDS will be in a none functioning
> state or will it explode? I don't feel this is a concern that you need
> to address as it feels a bit extreme use-case but since this is such a
> new approach (to me at least) to solve backward compatibility problems I
> just had to ask :-)

So, in a nutshell, you want to boot a system with a recent kernel (with this 
series merged) with a device tree that doesn't describe the hardware fully, 
add display support through an overlay using old-style bindings (which are 
deprecated by this series) using an overlay API that currently doesn't exist 
upstream (and thus require out-of-tree modules compiled for the new kernel), 
and expect it to work ? :-)

> I also trust you that all of the register values in the dts files are
> correct.
> 
> On 2018-03-02 00:05:38 +0200, Laurent Pinchart wrote:
> > The internal LVDS encoders now have their own DT bindings. Before
> > switching the driver infrastructure to those new bindings, implement
> > backward-compatibility through live DT patching.
> > 
> > Patching is disabled and will be enabled along with support for the new
> > DT bindings in the DU driver.
> > 
> > Signed-off-by: Laurent Pinchart
> > <laurent.pinchart+renesas@ideasonboard.com>
> > ---
> > Changes since v6:
> > 
> > - Don't free data used by the applied overlay
> > 
> > Changes since v5:
> > 
> > - Use a private copy of rcar_du_of_changeset_add_property()
> > 
> > Changes since v3:
> > 
> > - Use the OF changeset API
> > - Use of_graph_get_endpoint_by_regs()
> > - Replace hardcoded constants by sizeof()
> > 
> > Changes since v2:
> > 
> > - Update the SPDX headers to use C-style comments in header files
> > - Removed the manually created __local_fixups__ node
> > - Perform manual fixups on live DT instead of overlay
> > 
> > Changes since v1:
> > 
> > - Select OF_FLATTREE
> > - Compile LVDS DT bindings patch code when DRM_RCAR_LVDS is selected
> > - Update the SPDX headers to use GPL-2.0 instead of GPL-2.0-only
> > - Turn __dtb_rcar_du_of_lvds_(begin|end) from u8 to char
> > - Pass void begin and end pointers to rcar_du_of_get_overlay()
> > - Use of_get_parent() instead of accessing the parent pointer directly
> > - Find the LVDS endpoints nodes based on the LVDS node instead of the
> >   root of the overlay
> > - Update to the <soc>-lvds compatible string format
> > ---
> > 
> >  drivers/gpu/drm/rcar-du/Kconfig                    |   2 +
> >  drivers/gpu/drm/rcar-du/Makefile                   |   7 +-
> >  drivers/gpu/drm/rcar-du/rcar_du_of.c               | 334 ++++++++++++++++
> >  drivers/gpu/drm/rcar-du/rcar_du_of.h               |  20 ++
> >  .../gpu/drm/rcar-du/rcar_du_of_lvds_r8a7790.dts    |  79 +++++
> >  .../gpu/drm/rcar-du/rcar_du_of_lvds_r8a7791.dts    |  53 ++++
> >  .../gpu/drm/rcar-du/rcar_du_of_lvds_r8a7793.dts    |  53 ++++
> >  .../gpu/drm/rcar-du/rcar_du_of_lvds_r8a7795.dts    |  53 ++++
> >  .../gpu/drm/rcar-du/rcar_du_of_lvds_r8a7796.dts    |  53 ++++
> >  9 files changed, 653 insertions(+), 1 deletion(-)
> >  create mode 100644 drivers/gpu/drm/rcar-du/rcar_du_of.c
> >  create mode 100644 drivers/gpu/drm/rcar-du/rcar_du_of.h
> >  create mode 100644 drivers/gpu/drm/rcar-du/rcar_du_of_lvds_r8a7790.dts
> >  create mode 100644 drivers/gpu/drm/rcar-du/rcar_du_of_lvds_r8a7791.dts
> >  create mode 100644 drivers/gpu/drm/rcar-du/rcar_du_of_lvds_r8a7793.dts
> >  create mode 100644 drivers/gpu/drm/rcar-du/rcar_du_of_lvds_r8a7795.dts
> >  create mode 100644 drivers/gpu/drm/rcar-du/rcar_du_of_lvds_r8a7796.dts

[snip]

> > diff --git a/drivers/gpu/drm/rcar-du/rcar_du_of.c
> > b/drivers/gpu/drm/rcar-du/rcar_du_of.c new file mode 100644
> > index 000000000000..067278b91caa
> > --- /dev/null
> > +++ b/drivers/gpu/drm/rcar-du/rcar_du_of.c
> > @@ -0,0 +1,334 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/*
> > + * rcar_du_of.c - Legacy DT bindings compatibility
> > + *
> > + * Copyright (C) 2018 Laurent Pinchart
> > <laurent.pinchart@ideasonboard.com>
> > + *
> > + * Based on work from Jyri Sarha <jsarha@ti.com>
> > + * Copyright (C) 2015 Texas Instruments
> > + */
> > +
> > +#include <linux/init.h>
> > +#include <linux/kernel.h>
> > +#include <linux/of.h>
> > +#include <linux/of_address.h>
> > +#include <linux/of_fdt.h>
> > +#include <linux/of_graph.h>
> > +#include <linux/slab.h>
> > +
> > +#include "rcar_du_crtc.h"
> > +#include "rcar_du_drv.h"
> > +
> > +/* ----------------------------------------------------------------------
> > + * Generic Overlay Handling
> > + */
> > +
> > +struct rcar_du_of_overlay {
> > +	const char *compatible;
> > +	void *begin;
> > +	void *end;
> > +};
> > +
> > +#define RCAR_DU_OF_DTB(type, soc)					\
> > +	extern char __dtb_rcar_du_of_##type##_##soc##_begin[];		\
> > +	extern char __dtb_rcar_du_of_##type##_##soc##_end[]
> 
> I think I understand this but just to be sure the names are derived from
> the object files of the DTBs right? Would a comment on where these
> external blobs come from make it easier to understand or is it just my
> lack of experience that is showing?

I didn't find this particularly unclear when I wrote the code so I didn't 
think a comment was needed, but I can add one.

> > +
> > +#define RCAR_DU_OF_OVERLAY(type, soc)					\
> > +	{								\
> > +		.compatible = "renesas,du-" #soc,			\
> > +		.begin = __dtb_rcar_du_of_##type##_##soc##_begin,	\
> > +		.end = __dtb_rcar_du_of_##type##_##soc##_end,		\
> > +	}
> > +
> > +static int __init rcar_du_of_apply_overlay(const struct
> > rcar_du_of_overlay *dtbs, +					   const char *compatible)
> > +{
> > +	const struct rcar_du_of_overlay *dtb = NULL;
> > +	struct device_node *node;
> > +	unsigned int i;
> > +	int ovcs_id;
> > +	void *data;
> > +	void *mem;
> > +
> > +	for (i = 0; dtbs[i].compatible; ++i) {
> > +		if (!strcmp(dtbs[i].compatible, compatible)) {
> > +			dtb = &dtbs[i];
> > +			break;
> > +		}
> > +	}
> > +
> > +	if (!dtb)
> > +		return -ENODEV;
> > +
> > +	data = kmemdup(dtb->begin, dtb->end - dtb->begin, GFP_KERNEL);
> > +	if (!data)
> > +		return -ENOMEM;
> > +
> > +	mem = of_fdt_unflatten_tree(data, NULL, &node);
> > +	if (!mem) {
> > +		kfree(data);
> > +		return -ENOMEM;
> > +	}
> > +
> > +	ovcs_id = 0;
> > +	return of_overlay_apply(node, &ovcs_id);
> > +}
> > +
> > +static int __init rcar_du_of_add_property(struct of_changeset *ocs,
> > +					  struct device_node *np,
> > +					  const char *name, const void *value,
> > +					  int length)
> > +{
> > +	struct property *prop;
> > +	int ret = -ENOMEM;
> > +
> > +	prop = kzalloc(sizeof(*prop), GFP_KERNEL);
> > +	if (!prop)
> > +		return -ENOMEM;
> > +
> > +	prop->name = kstrdup(name, GFP_KERNEL);
> > +	if (!prop->name)
> > +		goto out_err;
> > +
> > +	prop->value = kmemdup(value, length, GFP_KERNEL);
> > +	if (!prop->value)
> > +		goto out_err;
> > +
> > +	of_property_set_flag(prop, OF_DYNAMIC);
> > +
> > +	prop->length = length;
> > +
> > +	ret = of_changeset_add_property(ocs, np, prop);
> > +	if (!ret)
> > +		return 0;
> > +
> > +out_err:
> > +	kfree(prop->value);
> > +	kfree(prop->name);
> > +	kfree(prop);
> > +	return ret;
> > +}
> > +
> > +/* ----------------------------------------------------------------------
> > + * LVDS Overlays
> > + */
> > +
> > +RCAR_DU_OF_DTB(lvds, r8a7790);
> > +RCAR_DU_OF_DTB(lvds, r8a7791);
> > +RCAR_DU_OF_DTB(lvds, r8a7793);
> > +RCAR_DU_OF_DTB(lvds, r8a7795);
> > +RCAR_DU_OF_DTB(lvds, r8a7796);
> > +
> > +static const struct rcar_du_of_overlay rcar_du_lvds_overlays[]
> > __initconst = {
> > +	RCAR_DU_OF_OVERLAY(lvds, r8a7790),
> > +	RCAR_DU_OF_OVERLAY(lvds, r8a7791),
> > +	RCAR_DU_OF_OVERLAY(lvds, r8a7793),
> > +	RCAR_DU_OF_OVERLAY(lvds, r8a7795),
> > +	RCAR_DU_OF_OVERLAY(lvds, r8a7796),
> > +	{ /* Sentinel */ },
> > +};
> > +
> > +static struct of_changeset rcar_du_lvds_changeset;
> > +
> > +static void __init rcar_du_of_lvds_patch_one(struct device_node *lvds,
> > +					     const struct of_phandle_args *clk,
> > +					     struct device_node *local,
> > +					     struct device_node *remote)
> > +{
> > +	unsigned int psize;
> > +	unsigned int i;
> > +	__be32 value[4];
> > +	int ret;
> > +
> > +	/*
> > +	 * Set the LVDS clocks property. This can't be performed by the overlay
> > +	 * as the structure of the clock specifier has changed over time, and
> > we
> > +	 * don't know at compile time which binding version the system we will
> > +	 * run on uses.
> > +	 */
> > +	if (clk->args_count >= ARRAY_SIZE(value) - 1)
> > +		return;
> > +
> > +	of_changeset_init(&rcar_du_lvds_changeset);
> > +
> > +	value[0] = cpu_to_be32(clk->np->phandle);
> > +	for (i = 0; i < clk->args_count; ++i)
> > +		value[i + 1] = cpu_to_be32(clk->args[i]);
> > +
> > +	psize = (clk->args_count + 1) * 4;
> > +	ret = rcar_du_of_add_property(&rcar_du_lvds_changeset, lvds,
> > +				      "clocks", value, psize);
> > +	if (ret < 0)
> > +		goto done;
> > +
> > +	/*
> > +	 * Insert the node in the OF graph: patch the LVDS ports remote-
> > endpoint
> > +	 * properties to point to the endpoints of the sibling nodes in the
> > +	 * graph. This can't be performed by the overlay: on the input side the
> > +	 * overlay would contain a phandle for the DU LVDS output port that
> > +	 * would clash with the system DT, and on the output side the
> > connection
> > +	 * is board-specific.
> > +	 */
> > +	value[0] = cpu_to_be32(local->phandle);
> > +	value[1] = cpu_to_be32(remote->phandle);
> > +
> > +	for (i = 0; i < 2; ++i) {
> > +		struct device_node *endpoint;
> > +
> > +		endpoint = of_graph_get_endpoint_by_regs(lvds, i, 0);
> > +		if (!endpoint) {
> > +			ret = -EINVAL;
> > +			goto done;
> > +		}
> > +
> > +		ret = rcar_du_of_add_property(&rcar_du_lvds_changeset,
> > +					      endpoint, "remote-endpoint",
> > +					      &value[i], sizeof(value[i]));
> > +		of_node_put(endpoint);
> > +		if (ret < 0)
> > +			goto done;
> > +	}
> > +
> > +	ret = of_changeset_apply(&rcar_du_lvds_changeset);
> > +
> > +done:
> > +	if (ret < 0)
> > +		of_changeset_destroy(&rcar_du_lvds_changeset);
> > +}
> > +
> > +struct lvds_of_data {
> > +	struct resource res;
> > +	struct of_phandle_args clkspec;
> > +	struct device_node *local;
> > +	struct device_node *remote;
> > +};
> > +
> > +static void __init rcar_du_of_lvds_patch(const struct of_device_id
> > *of_ids)
> > +{
> > +	const struct rcar_du_device_info *info;
> > +	const struct of_device_id *match;
> > +	struct lvds_of_data lvds_data[2] = { };
> > +	struct device_node *lvds_node;
> > +	struct device_node *soc_node;
> > +	struct device_node *du_node;
> > +	char compatible[21];
> 
> Do this need to be 22 bytes long to not create problems on boards where
> the model name is one character longer then on the earlier boards in
> Gen3? For example V3M would be "renesas,r8a77970-lvds\n".

V3M will only be supported using the new-style bindings, the compat code will 
only run for older SoCs that use 4 digits. I'll however make the array 22 
characters long just in case, as the compiler will pad it to 24 anyway.

> Whit these small questions/issues addressed feel free to add to which
> ever variant of this patch gets picked:
> 
> Reviewed-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>

Thank you.

> > +	const char *soc_name;
> > +	unsigned int i;
> > +	int ret;
> > +
> > +	/* Get the DU node and exit if not present or disabled. */
> > +	du_node = of_find_matching_node_and_match(NULL, of_ids, &match);
> > +	if (!du_node || !of_device_is_available(du_node)) {
> > +		of_node_put(du_node);
> > +		return;
> > +	}
> > +
> > +	info = match->data;
> > +	soc_node = of_get_parent(du_node);
> > +
> > +	if (WARN_ON(info->num_lvds > ARRAY_SIZE(lvds_data)))
> > +		goto done;
> > +
> > +	/*
> > +	 * Skip if the LVDS nodes already exists.
> > +	 *
> > +	 * The nodes are searched based on the compatible string, which we
> > +	 * construct from the SoC name found in the DU compatible string. As a
> > +	 * match has been found we know the compatible string matches the
> > +	 * expected format and can thus skip some of the string manipulation
> > +	 * normal safety checks.
> > +	 */
> > +	soc_name = strchr(match->compatible, '-') + 1;
> > +	sprintf(compatible, "renesas,%s-lvds", soc_name);
> > +	lvds_node = of_find_compatible_node(NULL, NULL, compatible);
> > +	if (lvds_node) {
> > +		of_node_put(lvds_node);
> > +		return;
> > +	}
> > +
> > +	/*
> > +	 * Parse the DU node and store the register specifier, the clock
> > +	 * specifier and the local and remote endpoint of the LVDS link for
> > +	 * later use.
> > +	 */
> > +	for (i = 0; i < info->num_lvds; ++i) {
> > +		struct lvds_of_data *lvds = &lvds_data[i];
> > +		unsigned int port;
> > +		char name[7];
> > +		int index;
> > +
> > +		sprintf(name, "lvds.%u", i);
> > +		index = of_property_match_string(du_node, "clock-names", name);
> > +		if (index < 0)
> > +			continue;
> > +
> > +		ret = of_parse_phandle_with_args(du_node, "clocks",
> > +						 "#clock-cells", index,
> > +						 &lvds->clkspec);
> > +		if (ret < 0)
> > +			continue;
> > +
> > +		port = info->routes[RCAR_DU_OUTPUT_LVDS0 + i].port;
> > +
> > +		lvds->local = of_graph_get_endpoint_by_regs(du_node, port, 0);
> > +		if (!lvds->local)
> > +			continue;
> > +
> > +		lvds->remote = of_graph_get_remote_endpoint(lvds->local);
> > +		if (!lvds->remote)
> > +			continue;
> > +
> > +		index = of_property_match_string(du_node, "reg-names", name);
> > +		if (index < 0)
> > +			continue;
> > +
> > +		of_address_to_resource(du_node, index, &lvds->res);
> > +	}
> > +
> > +	/* Parse and apply the overlay. This will resolve phandles. */
> > +	ret = rcar_du_of_apply_overlay(rcar_du_lvds_overlays,
> > +				       match->compatible);
> > +	if (ret < 0)
> > +		goto done;
> > +
> > +	/* Patch the newly created LVDS encoder nodes. */
> > +	for_each_child_of_node(soc_node, lvds_node) {
> > +		struct resource res;
> > +
> > +		if (!of_device_is_compatible(lvds_node, compatible))
> > +			continue;
> > +
> > +		/* Locate the lvds_data entry based on the resource start. */
> > +		ret = of_address_to_resource(lvds_node, 0, &res);
> > +		if (ret < 0)
> > +			continue;
> > +
> > +		for (i = 0; i < ARRAY_SIZE(lvds_data); ++i) {
> > +			if (lvds_data[i].res.start == res.start)
> > +				break;
> > +		}
> > +
> > +		if (i == ARRAY_SIZE(lvds_data))
> > +			continue;
> > +
> > +		/* Patch the LVDS encoder. */
> > +		rcar_du_of_lvds_patch_one(lvds_node, &lvds_data[i].clkspec,
> > +					  lvds_data[i].local,
> > +					  lvds_data[i].remote);
> > +	}
> > +
> > +done:
> > +	for (i = 0; i < info->num_lvds; ++i) {
> > +		of_node_put(lvds_data[i].clkspec.np);
> > +		of_node_put(lvds_data[i].local);
> > +		of_node_put(lvds_data[i].remote);
> > +	}
> > +
> > +	of_node_put(soc_node);
> > +	of_node_put(du_node);
> > +}
> > +
> > +void __init rcar_du_of_init(const struct of_device_id *of_ids)
> > +{
> > +	rcar_du_of_lvds_patch(of_ids);
> > +}

[snip]

-- 
Regards,

Laurent Pinchart