| Message ID | 20180112005858.26472-1-laurent.pinchart+renesas@ideasonboard.com |
|---|---|
| Headers | show |
| Series | R-Car DU: Convert LVDS code to bridge driver | expand |
Hi Laurent, On Fri, Jan 12, 2018 at 1:58 AM, Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com> wrote: > This patch series addresses a design mistake that dates back from the initial > DU support. Support for the LVDS encoders, which are IP cores separate from > the DU, was bundled in the DU driver. Worse, both the DU and LVDS were > described through a single DT node. > > To fix the, patches 01/10 and 02/10 define new DT bindings for the LVDS > encoders, and deprecate their description inside the DU bindings. To retain > backward compatibility with existing DT, patch 03/10 then patches the device > tree at runtime to convert the legacy bindings to the new ones. Looks like we will have to postpone the R-Car Gen2 Modern DT flag day again by a few kernel releases? 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 -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Laurent, On Fri, Jan 12, 2018 at 1:58 AM, Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com> 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> Thanks for your patch! > --- a/drivers/gpu/drm/rcar-du/Kconfig > +++ b/drivers/gpu/drm/rcar-du/Kconfig > @@ -22,6 +22,7 @@ config DRM_RCAR_LVDS > bool "R-Car DU LVDS Encoder Support" > depends on DRM_RCAR_DU > select DRM_PANEL > + select OF_OVERLAY select OF_FLATTREE for of_fdt_unflatten_tree() (you can probably check with sparc all*config) > --- /dev/null > +++ b/drivers/gpu/drm/rcar-du/rcar_du_of.c > @@ -0,0 +1,440 @@ > +// SPDX-License-Identifier: GPL-2.0-only -ENOENT in Documentation/process/license-rules.rst ;-) > +extern u8 __dtb_rcar_du_of_lvds_begin[]; > +extern u8 __dtb_rcar_du_of_lvds_end[]; Typically sections are declared using char, not u8. > +static int __init rcar_du_of_get_overlay(struct rcar_du_of_overlay *overlay, > + u8 *begin, u8 *end) "void *begin, void *end" sounds more natural to me. > +static void __init rcar_du_of_lvds_patch_one(struct device_node *du, > + unsigned int port_id, > + const struct resource *res, > + const __be32 *reg, > + const struct of_phandle_args *clkspec, > + struct device_node *local, > + struct device_node *remote) > +{ > + > + /* Skip if the LVDS node already exists. */ > + sprintf(name, "lvds@%llx", (u64)res->start); I guess you cannot use %pa because you don't want a 0x prefix? > + /* > + * Patch the LVDS and DU port nodes names and the associated fixup > + * entries. > + */ > + lvds = rcar_du_of_find_node_by_path(overlay.np, > + "/fragment@0/__overlay__/lvds"); > + lvds_endpoints[0] = rcar_du_of_find_node_by_path(overlay.np, > + "/fragment@0/__overlay__/lvds/ports/port@0/endpoint"); > + lvds_endpoints[1] = rcar_du_of_find_node_by_path(overlay.np, > + "/fragment@0/__overlay__/lvds/ports/port@1/endpoint"); > + du_port = rcar_du_of_find_node_by_path(overlay.np, > + "/fragment@1/__overlay__/ports/port@0"); > + du_port_fixup = rcar_du_of_find_node_by_path(overlay.np, > + "/__local_fixups__/fragment@1/__overlay__/ports/port@0"); Many strings with similar prefixes or substrings? Would it make sense to e.g. locate "/fragment@0/__overlay__/lvds/ports" first, and continue from there? 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 -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Geert, On Friday, 12 January 2018 11:47:03 EET Geert Uytterhoeven wrote: > On Fri, Jan 12, 2018 at 1:58 AM, Laurent Pinchart wrote: > > This patch series addresses a design mistake that dates back from the > > initial DU support. Support for the LVDS encoders, which are IP cores > > separate from the DU, was bundled in the DU driver. Worse, both the DU > > and LVDS were described through a single DT node. > > > > To fix the, patches 01/10 and 02/10 define new DT bindings for the LVDS > > encoders, and deprecate their description inside the DU bindings. To > > retain backward compatibility with existing DT, patch 03/10 then patches > > the device tree at runtime to convert the legacy bindings to the new ones. > > Looks like we will have to postpone the R-Car Gen2 Modern DT flag day > again by a few kernel releases? Why so ? We don't have to drop support for all legacy DT bindings at the same time, do we ? We can switch to the new-style clock bindings on Gen2 already, and drop the legacy LVDS bindings later.
Hi Geert, On Friday, 12 January 2018 12:09:45 EET Geert Uytterhoeven wrote: > On Fri, Jan 12, 2018 at 1:58 AM, 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> > > Thanks for your patch! > > > --- a/drivers/gpu/drm/rcar-du/Kconfig > > +++ b/drivers/gpu/drm/rcar-du/Kconfig > > @@ -22,6 +22,7 @@ config DRM_RCAR_LVDS > > bool "R-Car DU LVDS Encoder Support" > > depends on DRM_RCAR_DU > > select DRM_PANEL > > + select OF_OVERLAY > > select OF_FLATTREE for of_fdt_unflatten_tree() I'll fix that. > (you can probably check with sparc all*config) I'd have to install a sparc cross-compiler :-) > > --- /dev/null > > +++ b/drivers/gpu/drm/rcar-du/rcar_du_of.c > > @@ -0,0 +1,440 @@ [snip] > > +static void __init rcar_du_of_lvds_patch_one(struct device_node *du, > > + unsigned int port_id, > > + const struct resource *res, > > + const __be32 *reg, > > + const struct of_phandle_args > > *clkspec, > > + struct device_node *local, > > + struct device_node *remote) > > +{ > > > > + > > + /* Skip if the LVDS node already exists. */ > > + sprintf(name, "lvds@%llx", (u64)res->start); > > I guess you cannot use %pa because you don't want a 0x prefix? Correct. > > + /* > > + * Patch the LVDS and DU port nodes names and the associated fixup > > + * entries. > > + */ > > + lvds = rcar_du_of_find_node_by_path(overlay.np, > > + "/fragment@0/__overlay__/lvds"); > > + lvds_endpoints[0] = rcar_du_of_find_node_by_path(overlay.np, > > + "/fragment@0/__overlay__/lvds/ports/port@0/endpoint"); > > + lvds_endpoints[1] = rcar_du_of_find_node_by_path(overlay.np, > > + "/fragment@0/__overlay__/lvds/ports/port@1/endpoint"); > > + du_port = rcar_du_of_find_node_by_path(overlay.np, > > + "/fragment@1/__overlay__/ports/port@0"); > > + du_port_fixup = rcar_du_of_find_node_by_path(overlay.np, > > + "/__local_fixups__/fragment@1/__overlay__/ports/port@0"); > > Many strings with similar prefixes or substrings? > Would it make sense to e.g. locate "/fragment@0/__overlay__/lvds/ports" > first, and continue from there? I can do that for the first three, yes.
On Fri, Jan 12, 2018 at 03:48:25PM +0200, Laurent Pinchart wrote: > Hi Geert, > > On Friday, 12 January 2018 11:47:03 EET Geert Uytterhoeven wrote: > > On Fri, Jan 12, 2018 at 1:58 AM, Laurent Pinchart wrote: > > > This patch series addresses a design mistake that dates back from the > > > initial DU support. Support for the LVDS encoders, which are IP cores > > > separate from the DU, was bundled in the DU driver. Worse, both the DU > > > and LVDS were described through a single DT node. > > > > > > To fix the, patches 01/10 and 02/10 define new DT bindings for the LVDS > > > encoders, and deprecate their description inside the DU bindings. To > > > retain backward compatibility with existing DT, patch 03/10 then patches > > > the device tree at runtime to convert the legacy bindings to the new ones. > > > > Looks like we will have to postpone the R-Car Gen2 Modern DT flag day > > again by a few kernel releases? > > Why so ? We don't have to drop support for all legacy DT bindings at the same > time, do we ? We can switch to the new-style clock bindings on Gen2 already, > and drop the legacy LVDS bindings later. To clarify, after this patchset. * Old DTs work with old and new kernels. * New DTs require new kernels. -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, Jan 12, 2018 at 02:58:48AM +0200, Laurent Pinchart wrote: > Hello, > > This patch series addresses a design mistake that dates back from the initial > DU support. Support for the LVDS encoders, which are IP cores separate from > the DU, was bundled in the DU driver. Worse, both the DU and LVDS were > described through a single DT node. > > To fix the, patches 01/10 and 02/10 define new DT bindings for the LVDS > encoders, and deprecate their description inside the DU bindings. To retain > backward compatibility with existing DT, patch 03/10 then patches the device > tree at runtime to convert the legacy bindings to the new ones. > > With the DT side addressed, patch 04/10 then converts the LVDS support code to > a separate bridge driver. After a small fix to the Porter board device tree in > patch 05/10, patches 06/10 to 10/10 then update all the device tree sources to > the new LVDS encoders bindings. > > I decided to go for live DT patching in patch 03/10 because implementing > support for both the legacy and new bindings in the driver would have been > very intrusive, and prevented further cleanups. I'm in a way both proud and > ashamed of that patch that I would call a neat and dirty hack. It was fun to > write perhaps in a similar way that one would enjoy researching and developing > proof-of-concepts for security holes: they're good and bad at the same time. > > Anyway, with this philosophical considerations aside, there were a few > shortcomings in the OF API that I worked around with local code in the driver. > If anyone is interested in performing similar live DT patching I think we > could move some of the code to the OF core. For instance I was surprised > to not find a device node lookup by path function that would start at a > given node instead of the root of the live device tree, and had to write > rcar_du_of_find_node_by_path(). Utility functions to add or modify properties > or to rename nodes could similarly be added. > > Laurent Pinchart (10): > dt-bindings: display: renesas: Add R-Car LVDS encoder DT bindings > dt-bindings: display: renesas: Deprecate LVDS support in the DU > bindings > drm: rcar-du: Fix legacy DT to create LVDS encoder nodes > drm: rcar-du: Convert LVDS encoder code to bridge driver > ARM: dts: porter: Fix HDMI output routing > ARM: dts: r8a7790: Convert to new LVDS DT bindings > ARM: dts: r8a7791: Convert to new LVDS DT bindings > ARM: dts: r8a7793: Convert to new LVDS DT bindings > arm64: dts: renesas: r8a7795: Convert to new LVDS DT bindings > arm64: dts: renesas: r8a7796: Convert to new LVDS DT bindings I understand that there will be a v2 to address review of the non DTS patches. I am marking the DTS patches as "Changes Requested" as they are dependent on the bindings patches from my PoV. -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Simon, On Monday, 15 January 2018 08:57:48 EET Simon Horman wrote: > On Fri, Jan 12, 2018 at 03:48:25PM +0200, Laurent Pinchart wrote: > > On Friday, 12 January 2018 11:47:03 EET Geert Uytterhoeven wrote: > >> On Fri, Jan 12, 2018 at 1:58 AM, Laurent Pinchart wrote: > >>> This patch series addresses a design mistake that dates back from the > >>> initial DU support. Support for the LVDS encoders, which are IP cores > >>> separate from the DU, was bundled in the DU driver. Worse, both the DU > >>> and LVDS were described through a single DT node. > >>> > >>> To fix the, patches 01/10 and 02/10 define new DT bindings for the > >>> LVDS encoders, and deprecate their description inside the DU bindings. > >>> To retain backward compatibility with existing DT, patch 03/10 then > >>> patches the device tree at runtime to convert the legacy bindings to the > >>> new ones. > >> > >> Looks like we will have to postpone the R-Car Gen2 Modern DT flag day > >> again by a few kernel releases? > > > > Why so ? We don't have to drop support for all legacy DT bindings at the > > same time, do we ? We can switch to the new-style clock bindings on Gen2 > > already, and drop the legacy LVDS bindings later. > > To clarify, after this patchset. > * Old DTs work with old and new kernels. > * New DTs require new kernels. That's correct. I've tested old DTs with new kernels successfully on Lager, Salvator-X (both H3 ES1.x and M3-W) and Salvator-XS.
On Mon, Jan 15, 2018 at 09:00:59AM +0200, Laurent Pinchart wrote: > Hi Simon, > > On Monday, 15 January 2018 08:57:48 EET Simon Horman wrote: > > On Fri, Jan 12, 2018 at 03:48:25PM +0200, Laurent Pinchart wrote: > > > On Friday, 12 January 2018 11:47:03 EET Geert Uytterhoeven wrote: > > >> On Fri, Jan 12, 2018 at 1:58 AM, Laurent Pinchart wrote: > > >>> This patch series addresses a design mistake that dates back from the > > >>> initial DU support. Support for the LVDS encoders, which are IP cores > > >>> separate from the DU, was bundled in the DU driver. Worse, both the DU > > >>> and LVDS were described through a single DT node. > > >>> > > >>> To fix the, patches 01/10 and 02/10 define new DT bindings for the > > >>> LVDS encoders, and deprecate their description inside the DU bindings. > > >>> To retain backward compatibility with existing DT, patch 03/10 then > > >>> patches the device tree at runtime to convert the legacy bindings to the > > >>> new ones. > > >> > > >> Looks like we will have to postpone the R-Car Gen2 Modern DT flag day > > >> again by a few kernel releases? > > > > > > Why so ? We don't have to drop support for all legacy DT bindings at the > > > same time, do we ? We can switch to the new-style clock bindings on Gen2 > > > already, and drop the legacy LVDS bindings later. > > > > To clarify, after this patchset. > > * Old DTs work with old and new kernels. > > * New DTs require new kernels. > > That's correct. I've tested old DTs with new kernels successfully on Lager, > Salvator-X (both H3 ES1.x and M3-W) and Salvator-XS. Thanks, no objections to this approach from my side. -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hello, This patch series addresses a design mistake that dates back from the initial DU support. Support for the LVDS encoders, which are IP cores separate from the DU, was bundled in the DU driver. Worse, both the DU and LVDS were described through a single DT node. To fix the, patches 01/10 and 02/10 define new DT bindings for the LVDS encoders, and deprecate their description inside the DU bindings. To retain backward compatibility with existing DT, patch 03/10 then patches the device tree at runtime to convert the legacy bindings to the new ones. With the DT side addressed, patch 04/10 then converts the LVDS support code to a separate bridge driver. After a small fix to the Porter board device tree in patch 05/10, patches 06/10 to 10/10 then update all the device tree sources to the new LVDS encoders bindings. I decided to go for live DT patching in patch 03/10 because implementing support for both the legacy and new bindings in the driver would have been very intrusive, and prevented further cleanups. I'm in a way both proud and ashamed of that patch that I would call a neat and dirty hack. It was fun to write perhaps in a similar way that one would enjoy researching and developing proof-of-concepts for security holes: they're good and bad at the same time. Anyway, with this philosophical considerations aside, there were a few shortcomings in the OF API that I worked around with local code in the driver. If anyone is interested in performing similar live DT patching I think we could move some of the code to the OF core. For instance I was surprised to not find a device node lookup by path function that would start at a given node instead of the root of the live device tree, and had to write rcar_du_of_find_node_by_path(). Utility functions to add or modify properties or to rename nodes could similarly be added. Laurent Pinchart (10): dt-bindings: display: renesas: Add R-Car LVDS encoder DT bindings dt-bindings: display: renesas: Deprecate LVDS support in the DU bindings drm: rcar-du: Fix legacy DT to create LVDS encoder nodes drm: rcar-du: Convert LVDS encoder code to bridge driver ARM: dts: porter: Fix HDMI output routing ARM: dts: r8a7790: Convert to new LVDS DT bindings ARM: dts: r8a7791: Convert to new LVDS DT bindings ARM: dts: r8a7793: Convert to new LVDS DT bindings arm64: dts: renesas: r8a7795: Convert to new LVDS DT bindings arm64: dts: renesas: r8a7796: Convert to new LVDS DT bindings .../bindings/display/bridge/renesas,lvds.txt | 54 ++ .../devicetree/bindings/display/renesas,du.txt | 26 +- MAINTAINERS | 1 + arch/arm/boot/dts/r8a7790-lager.dts | 22 +- arch/arm/boot/dts/r8a7790.dtsi | 61 ++- arch/arm/boot/dts/r8a7791-koelsch.dts | 10 +- arch/arm/boot/dts/r8a7791-porter.dts | 18 +- arch/arm/boot/dts/r8a7791.dtsi | 35 +- arch/arm/boot/dts/r8a7793-gose.dts | 10 +- arch/arm/boot/dts/r8a7793.dtsi | 35 +- .../boot/dts/renesas/r8a7795-es1-salvator-x.dts | 3 +- arch/arm64/boot/dts/renesas/r8a7795-h3ulcb.dts | 3 +- arch/arm64/boot/dts/renesas/r8a7795-salvator-x.dts | 3 +- .../arm64/boot/dts/renesas/r8a7795-salvator-xs.dts | 3 +- arch/arm64/boot/dts/renesas/r8a7795.dtsi | 35 +- arch/arm64/boot/dts/renesas/r8a7796-m3ulcb.dts | 3 +- arch/arm64/boot/dts/renesas/r8a7796-salvator-x.dts | 3 +- arch/arm64/boot/dts/renesas/r8a7796.dtsi | 35 +- drivers/gpu/drm/rcar-du/Kconfig | 5 +- drivers/gpu/drm/rcar-du/Makefile | 6 +- drivers/gpu/drm/rcar-du/rcar_du_drv.c | 21 +- drivers/gpu/drm/rcar-du/rcar_du_drv.h | 5 - drivers/gpu/drm/rcar-du/rcar_du_encoder.c | 175 +------ drivers/gpu/drm/rcar-du/rcar_du_encoder.h | 12 - drivers/gpu/drm/rcar-du/rcar_du_kms.c | 14 +- drivers/gpu/drm/rcar-du/rcar_du_lvdscon.c | 93 ---- drivers/gpu/drm/rcar-du/rcar_du_lvdscon.h | 24 - drivers/gpu/drm/rcar-du/rcar_du_lvdsenc.c | 270 ---------- drivers/gpu/drm/rcar-du/rcar_du_lvdsenc.h | 64 --- drivers/gpu/drm/rcar-du/rcar_du_of.c | 440 ++++++++++++++++ drivers/gpu/drm/rcar-du/rcar_du_of.h | 16 + drivers/gpu/drm/rcar-du/rcar_du_of_lvds.dts | 82 +++ drivers/gpu/drm/rcar-du/rcar_lvds.c | 554 +++++++++++++++++++++ 33 files changed, 1420 insertions(+), 721 deletions(-) create mode 100644 Documentation/devicetree/bindings/display/bridge/renesas,lvds.txt delete mode 100644 drivers/gpu/drm/rcar-du/rcar_du_lvdscon.c delete mode 100644 drivers/gpu/drm/rcar-du/rcar_du_lvdscon.h delete mode 100644 drivers/gpu/drm/rcar-du/rcar_du_lvdsenc.c delete mode 100644 drivers/gpu/drm/rcar-du/rcar_du_lvdsenc.h 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.dts create mode 100644 drivers/gpu/drm/rcar-du/rcar_lvds.c