mbox series

[00/39] drm: renesas: shmobile: Atomic conversion + DT support

Message ID cover.1687423204.git.geert+renesas@glider.be
Headers show
Series drm: renesas: shmobile: Atomic conversion + DT support | expand

Message

Geert Uytterhoeven June 22, 2023, 9:21 a.m. UTC
Hi all,

It has been 3 years since the last conversion of a DRM driver to atomic
modesetting, so I guess it's time for another one? ;-)

Currently, there are two drivers for the LCD controller on Renesas
SuperH-based and ARM-based SH-Mobile and R-Mobile SoCs:
  1. sh_mobile_lcdcfb, using the fbdev framework,
  2. shmob_drm, using the DRM framework.
However, only the former driver is used, as all platform support
integrates the former.  None of these drivers support DT-based systems.

This patch series is a follow-up to [1] (which is already included in
drm-next).  It converts the SH-Mobile DRM driver to atomic modesetting,
and adds DT support, complemented by the customary set of fixes and
improvements.

Overview:
  - Patch 1 adds DT bindings for the SH-Mobile LCD controller,
  - Patch 2 adds definitions for RGB666 9:9 media bus formats,
  - Patches 3-33 contains miscellaneous fixes, improvements, and
    cleanups for the SH-Mobile DRM driver,
  - Patches 34-38 convert the SH-Mobile DRM driver to atomic
    modesetting,
  - Patch 39 adds DT support to the SH-Mobile DRM driver.

To reduce strain on the audience, I have CCed the DT and media people
only on the cover letter and the DT resp. media patches.  If interested,
the full series should be available through lore.kernel.org.

Some comments and questioned can be found in the individual patches.

This has been tested on the R-Mobile A1-based Atmark Techno
Armadillo-800-EVA development board, using both legacy[2] and
DT-based[3] instantiation, with the fbdev-emulated text console and
modetest, a.o.

    modetest -M shmob-drm -s 43:800x480@RG16 -P 33@41:640x320+80+80@RG16
    modetest -M shmob-drm -s 43:800x480@RG16

The output of "modetest -M shmob-drm" can be found below[4].

Thanks for your comments!

[1] "[PATCH v3 0/5] drm: shmobile: Fixes and enhancements"
    https://lore.kernel.org/r/cover.1684854992.git.geert+renesas@glider.be

[2] "[PATCH/RFC] staging: board: armadillo800eva: Add DRM support"
    https://lore.kernel.org/r/f7874a9da4bcb20fbc9cd133147b67862ebcf0b9.1687418281.git.geert+renesas@glider.be

[3] "[PATCH 0/2] ARM: dts: r8a7740/armadillo800eva: Add LCD support"
    https://lore.kernel.org/r/cover.1687417585.git.geert+renesas@glider.be

[4] Encoders:
    id	crtc	type	possible crtcs	possible clones	
    42	41	DPI	0x00000001	0x00000001

    Connectors:
    id	encoder	status		name		size (mm)	modes	encoders
    43	42	connected	DPI-1          	111x67		1	42
      modes:
	    index name refresh (Hz) hdisp hss hse htot vdisp vss vse vtot
      #0 800x480 59.99 800 840 968 1056 480 515 517 525 33260 flags: nhsync, nvsync; type: preferred, driver
      props:
	    1 EDID:
		    flags: immutable blob
		    blobs:

		    value:
	    2 DPMS:
		    flags: enum
		    enums: On=0 Standby=1 Suspend=2 Off=3
		    value: 0
	    5 link-status:
		    flags: enum
		    enums: Good=0 Bad=1
		    value: 0
	    6 non-desktop:
		    flags: immutable range
		    values: 0 1
		    value: 0
	    4 TILE:
		    flags: immutable blob
		    blobs:

		    value:

    CRTCs:
    id	fb	pos	size
    41	44	(0,0)	(800x480)
      #0 800x480 59.99 800 840 968 1056 480 515 517 525 33260 flags: nhsync, nvsync; type: preferred, driver
      props:

    Planes:
    id	crtc	fb	CRTC x,y	x,y	gamma size	possible crtcs
    31	41	44	0,0		0,0	0       	0x00000001
      formats: RG16 RG24 AR24 XR24 NV12 NV21 NV16 NV61 NV24 NV42
      props:
	    8 type:
		    flags: immutable enum
		    enums: Overlay=0 Primary=1 Cursor=2
		    value: 1
	    30 IN_FORMATS:
		    flags: immutable blob
		    blobs:

		    value:
			    01000000000000000a00000018000000
			    01000000400000005247313652473234
			    41523234585232344e5631324e563231
			    4e5631364e5636314e5632344e563432
			    ff030000000000000000000000000000
			    0000000000000000
		    in_formats blob decoded:
			     RG16:  LINEAR
			     RG24:  LINEAR
			     AR24:  LINEAR
			     XR24:  LINEAR
			     NV12:  LINEAR
			     NV21:  LINEAR
			     NV16:  LINEAR
			     NV61:  LINEAR
			     NV24:  LINEAR
			     NV42:  LINEAR
    33	0	0	0,0		0,0	0       	0x00000001
      formats: RG16 RG24 AR24 XR24 NV12 NV21 NV16 NV61 NV24 NV42
      props:
	    8 type:
		    flags: immutable enum
		    enums: Overlay=0 Primary=1 Cursor=2
		    value: 0
	    30 IN_FORMATS:
		    flags: immutable blob
		    blobs:

		    value:
			    01000000000000000a00000018000000
			    01000000400000005247313652473234
			    41523234585232344e5631324e563231
			    4e5631364e5636314e5632344e563432
			    ff030000000000000000000000000000
			    0000000000000000
		    in_formats blob decoded:
			     RG16:  LINEAR
			     RG24:  LINEAR
			     AR24:  LINEAR
			     XR24:  LINEAR
			     NV12:  LINEAR
			     NV21:  LINEAR
			     NV16:  LINEAR
			     NV61:  LINEAR
			     NV24:  LINEAR
			     NV42:  LINEAR
    35	0	0	0,0		0,0	0       	0x00000001
      formats: RG16 RG24 AR24 XR24 NV12 NV21 NV16 NV61 NV24 NV42
      props:
	    8 type:
		    flags: immutable enum
		    enums: Overlay=0 Primary=1 Cursor=2
		    value: 0
	    30 IN_FORMATS:
		    flags: immutable blob
		    blobs:

		    value:
			    01000000000000000a00000018000000
			    01000000400000005247313652473234
			    41523234585232344e5631324e563231
			    4e5631364e5636314e5632344e563432
			    ff030000000000000000000000000000
			    0000000000000000
		    in_formats blob decoded:
			     RG16:  LINEAR
			     RG24:  LINEAR
			     AR24:  LINEAR
			     XR24:  LINEAR
			     NV12:  LINEAR
			     NV21:  LINEAR
			     NV16:  LINEAR
			     NV61:  LINEAR
			     NV24:  LINEAR
			     NV42:  LINEAR
    37	0	0	0,0		0,0	0       	0x00000001
      formats: RG16 RG24 AR24 XR24 NV12 NV21 NV16 NV61 NV24 NV42
      props:
	    8 type:
		    flags: immutable enum
		    enums: Overlay=0 Primary=1 Cursor=2
		    value: 0
	    30 IN_FORMATS:
		    flags: immutable blob
		    blobs:

		    value:
			    01000000000000000a00000018000000
			    01000000400000005247313652473234
			    41523234585232344e5631324e563231
			    4e5631364e5636314e5632344e563432
			    ff030000000000000000000000000000
			    0000000000000000
		    in_formats blob decoded:
			     RG16:  LINEAR
			     RG24:  LINEAR
			     AR24:  LINEAR
			     XR24:  LINEAR
			     NV12:  LINEAR
			     NV21:  LINEAR
			     NV16:  LINEAR
			     NV61:  LINEAR
			     NV24:  LINEAR
			     NV42:  LINEAR
    39	0	0	0,0		0,0	0       	0x00000001
      formats: RG16 RG24 AR24 XR24 NV12 NV21 NV16 NV61 NV24 NV42
      props:
	    8 type:
		    flags: immutable enum
		    enums: Overlay=0 Primary=1 Cursor=2
		    value: 0
	    30 IN_FORMATS:
		    flags: immutable blob
		    blobs:

		    value:
			    01000000000000000a00000018000000
			    01000000400000005247313652473234
			    41523234585232344e5631324e563231
			    4e5631364e5636314e5632344e563432
			    ff030000000000000000000000000000
			    0000000000000000
		    in_formats blob decoded:
			     RG16:  LINEAR
			     RG24:  LINEAR
			     AR24:  LINEAR
			     XR24:  LINEAR
			     NV12:  LINEAR
			     NV21:  LINEAR
			     NV16:  LINEAR
			     NV61:  LINEAR
			     NV24:  LINEAR
			     NV42:  LINEAR

    Frame buffers:
    id	size	pitch

Geert Uytterhoeven (34):
  dt-bindings: display: Add Renesas SH-Mobile LCDC bindings
  media: uapi: Add MEDIA_BUS_FMT_RGB666_2X9 variants
  drm: renesas: shmobile: Fix overlay plane disable
  drm: renesas: shmobile: Fix ARGB32 overlay format typo
  drm: renesas: shmobile: Correct encoder/connector types
  drm: renesas: shmobile: Add support for Runtime PM
  drm: renesas: shmobile: Restore indentation of
    shmob_drm_setup_clocks()
  drm: renesas: shmobile: Use %p4cc to print fourcc code
  drm: renesas: shmobile: Add missing YCbCr formats
  drm: renesas: shmobile: Improve shmob_drm_format_info table
  drm: renesas: shmobile: Improve error handling
  drm: renesas: shmobile: Convert to use devm_request_irq()
  drm: renesas: shmobile: Use drmm_universal_plane_alloc()
  drm: renesas: shmobile: Embed drm_device in shmob_drm_device
  drm: renesas: shmobile: Convert container helpers to static inline
    functions
  drm: renesas: shmobile: Replace .dev_private with container_of()
  drm: renesas: shmobile: Use media bus formats in platform data
  drm: renesas: shmobile: Move interface handling to connector setup
  drm: renesas: shmobile: Unify plane allocation
  drm: renesas: shmobile: Rename shmob_drm_crtc.crtc
  drm: renesas: shmobile: Rename shmob_drm_connector.connector
  drm: renesas: shmobile: Rename shmob_drm_plane.plane
  drm: renesas: shmobile: Use drm_crtc_handle_vblank()
  drm: renesas: shmobile: Move shmob_drm_crtc_finish_page_flip()
  drm: renesas: shmobile: Wait for page flip when turning CRTC off
  drm: renesas: shmobile: Turn vblank on/off when enabling/disabling
    CRTC
  drm: renesas: shmobile: Shutdown the display on remove
  drm: renesas: shmobile: Cleanup encoder
  drm: renesas: shmobile: Atomic conversion part 1
  drm: renesas: shmobile: Atomic conversion part 2
  drm: renesas: shmobile: Use suspend/resume helpers
  drm: renesas: shmobile: Remove internal CRTC state tracking
  drm: renesas: shmobile: Atomic conversion part 3
  drm: renesas: shmobile: Add DT support

Laurent Pinchart (5):
  drm: renesas: shmobile: Remove backlight support
  drm: renesas: shmobile: Don't set display info width and height twice
  drm: renesas: shmobile: Rename input clocks
  drm: renesas: shmobile: Remove support for SYS panels
  drm: renesas: shmobile: Use struct videomode in platform data

 .../display/renesas,shmobile-lcdc.yaml        | 108 +++
 .../media/v4l/subdev-formats.rst              | 144 ++++
 MAINTAINERS                                   |   1 +
 drivers/gpu/drm/renesas/shmobile/Makefile     |   3 +-
 .../renesas/shmobile/shmob_drm_backlight.c    |  82 ---
 .../renesas/shmobile/shmob_drm_backlight.h    |  19 -
 .../gpu/drm/renesas/shmobile/shmob_drm_crtc.c | 666 +++++++++---------
 .../gpu/drm/renesas/shmobile/shmob_drm_crtc.h |  27 +-
 .../gpu/drm/renesas/shmobile/shmob_drm_drv.c  | 153 ++--
 .../gpu/drm/renesas/shmobile/shmob_drm_drv.h  |  18 +-
 .../gpu/drm/renesas/shmobile/shmob_drm_kms.c  |  77 +-
 .../gpu/drm/renesas/shmobile/shmob_drm_kms.h  |   9 +-
 .../drm/renesas/shmobile/shmob_drm_plane.c    | 386 +++++-----
 .../drm/renesas/shmobile/shmob_drm_plane.h    |   4 +-
 include/linux/platform_data/shmob_drm.h       |  57 +-
 include/uapi/linux/media-bus-format.h         |   4 +-
 16 files changed, 965 insertions(+), 793 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/display/renesas,shmobile-lcdc.yaml
 delete mode 100644 drivers/gpu/drm/renesas/shmobile/shmob_drm_backlight.c
 delete mode 100644 drivers/gpu/drm/renesas/shmobile/shmob_drm_backlight.h

Cc: Rob Herring <robh+dt@kernel.org>"
Cc: Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org>"
Cc: Conor Dooley <conor+dt@kernel.org>"
Cc: Mauro Carvalho Chehab <mchehab@kernel.org>"
Cc: devicetree@vger.kernel.org"
Cc: linux-media@vger.kernel.org"

Comments

Rob Herring (Arm) June 22, 2023, 2:52 p.m. UTC | #1
On Thu, Jun 22, 2023 at 11:21:13AM +0200, Geert Uytterhoeven wrote:
> Add device tree bindings for the LCD Controller (LCDC) found in Renesas
> SuperH SH-Mobile and ARM SH/R-Mobile SOCs.
> 
> Based on a plain text prototype by Laurent Pinchart.
> 
> Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
> Cc: Rob Herring <robh+dt@kernel.org>
> Cc: Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org>
> Cc: Conor Dooley <conor+dt@kernel.org>
> Cc: devicetree@vger.kernel.org
> ---
> Changes compared to Laurent's original:
>   - Convert to json-schema,
>   - Rename compatible values from "renesas,lcdc-<SoC>" to
>     "renesas,<SoC>-lcdc",
>   - Add power-domains property,
>   - Add MIPI-DSI port on SH-Mobile AG5,
>   - Update example to reflect reality,
>   - Add to MAINTAINERS.
> ---
>  .../display/renesas,shmobile-lcdc.yaml        | 108 ++++++++++++++++++
>  MAINTAINERS                                   |   1 +
>  2 files changed, 109 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/display/renesas,shmobile-lcdc.yaml
> 
> diff --git a/Documentation/devicetree/bindings/display/renesas,shmobile-lcdc.yaml b/Documentation/devicetree/bindings/display/renesas,shmobile-lcdc.yaml
> new file mode 100644
> index 0000000000000000..72a39fce7294d56d
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/display/renesas,shmobile-lcdc.yaml
> @@ -0,0 +1,108 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/display/renesas,shmobile-lcdc.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Renesas SH-Mobile LCD Controller (LCDC)
> +
> +maintainers:
> +  - Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
> +
> +properties:
> +  compatible:
> +    enum:
> +      - renesas,r8a7740-lcdc # R-Mobile A1
> +      - renesas,sh73a0-lcdc  # SH-Mobile AG5
> +
> +  reg:
> +    maxItems: 1
> +
> +  interrupts:
> +    maxItems: 1
> +
> +  clocks:
> +    minItems: 1
> +    maxItems: 5
> +    description:
> +      Only the functional clock is mandatory.
> +      Some of the optional clocks are model-dependent (e.g. "video" (a.k.a.
> +      "vou" or "dv_clk") is available on R-Mobile A1 only).
> +
> +  clock-names:
> +    minItems: 1
> +    maxItems: 5
> +    items:
> +      enum: [ fck, media, lclk, hdmi, video ]
> +
> +  power-domains:
> +    maxItems: 1
> +
> +  ports:
> +    $ref: /schemas/graph.yaml#/properties/ports
> +    description: |
> +      The connections to the output video ports are modeled using the OF graph
> +      bindings specified in Documentation/devicetree/bindings/graph.txt.

Please read this file.

> +      The number of ports and their assignment are model-dependent.
> +      Each port shall have a single endpoint.

I'd just drop the whole description.

> +
> +    properties:
> +      port@0:
> +        $ref: /schemas/graph.yaml#/properties/port
> +        description: LCD port (R-Mobile A1 and SH-Mobile AG5)
> +        unevaluatedProperties: false

Don't need this.

> +
> +      port@1:
> +        $ref: /schemas/graph.yaml#/properties/port
> +        description: HDMI port (R-Mobile A1 LCDC1 and SH-Mobile AG5)
> +        unevaluatedProperties: false
> +
> +      port@2:
> +        $ref: /schemas/graph.yaml#/properties/port
> +        description: MIPI-DSI port (SH-Mobile AG5)
> +        unevaluatedProperties: false
> +
> +    required:
> +      - port@0
> +
> +    unevaluatedProperties: false
> +
> +required:
> +  - compatible
> +  - reg
> +  - interrupts
> +  - clocks
> +  - clock-names
> +  - power-domains
> +  - ports
> +
> +additionalProperties: false
> +
> +examples:
> +  - |
> +    #include <dt-bindings/clock/r8a7740-clock.h>
> +    #include <dt-bindings/interrupt-controller/arm-gic.h>
> +
> +    lcdc0: lcd-controller@fe940000 {

Drop label.

> +        compatible = "renesas,r8a7740-lcdc";
> +        reg = <0xfe940000 0x4000>;
> +        interrupts = <GIC_SPI 177 IRQ_TYPE_LEVEL_HIGH>;
> +        clocks = <&mstp1_clks R8A7740_CLK_LCDC0>,
> +                 <&cpg_clocks R8A7740_CLK_M3>, <&lcdlclk0_clk>,
> +                 <&vou_clk>;
> +        clock-names = "fck", "media", "lclk", "video";
> +        power-domains = <&pd_a4lc>;
> +        status = "disabled";

Drop.

> +
> +        ports {
> +            #address-cells = <1>;
> +            #size-cells = <0>;
> +
> +            port@0 {
> +                reg = <0>;
> +
> +                lcdc0_rgb: endpoint {
> +                };
> +            };
> +        };
> +    };
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 83e9f4ac6bedaa9f..dc1935c196cb0e0b 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -7023,6 +7023,7 @@ F:	Documentation/devicetree/bindings/display/bridge/renesas,dsi-csi2-tx.yaml
>  F:	Documentation/devicetree/bindings/display/bridge/renesas,dw-hdmi.yaml
>  F:	Documentation/devicetree/bindings/display/bridge/renesas,lvds.yaml
>  F:	Documentation/devicetree/bindings/display/renesas,du.yaml
> +F:	Documentation/devicetree/bindings/display/renesas,shmobile-lcdc.yaml
>  F:	drivers/gpu/drm/renesas/
>  F:	include/linux/platform_data/shmob_drm.h
>  
> -- 
> 2.34.1
>
Laurent Pinchart June 23, 2023, 2:43 p.m. UTC | #2
Hi Geert,

Thank you for the patch.

On Thu, Jun 22, 2023 at 11:21:13AM +0200, Geert Uytterhoeven wrote:
> Add device tree bindings for the LCD Controller (LCDC) found in Renesas
> SuperH SH-Mobile and ARM SH/R-Mobile SOCs.
> 
> Based on a plain text prototype by Laurent Pinchart.
> 
> Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
> Cc: Rob Herring <robh+dt@kernel.org>
> Cc: Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org>
> Cc: Conor Dooley <conor+dt@kernel.org>
> Cc: devicetree@vger.kernel.org
> ---
> Changes compared to Laurent's original:
>   - Convert to json-schema,
>   - Rename compatible values from "renesas,lcdc-<SoC>" to
>     "renesas,<SoC>-lcdc",
>   - Add power-domains property,
>   - Add MIPI-DSI port on SH-Mobile AG5,
>   - Update example to reflect reality,
>   - Add to MAINTAINERS.
> ---
>  .../display/renesas,shmobile-lcdc.yaml        | 108 ++++++++++++++++++
>  MAINTAINERS                                   |   1 +
>  2 files changed, 109 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/display/renesas,shmobile-lcdc.yaml
> 
> diff --git a/Documentation/devicetree/bindings/display/renesas,shmobile-lcdc.yaml b/Documentation/devicetree/bindings/display/renesas,shmobile-lcdc.yaml
> new file mode 100644
> index 0000000000000000..72a39fce7294d56d
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/display/renesas,shmobile-lcdc.yaml
> @@ -0,0 +1,108 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/display/renesas,shmobile-lcdc.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Renesas SH-Mobile LCD Controller (LCDC)
> +
> +maintainers:
> +  - Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>

I'd be happy if you co-maintained this with me :-) Or even took
ownership completely.

> +
> +properties:
> +  compatible:
> +    enum:
> +      - renesas,r8a7740-lcdc # R-Mobile A1
> +      - renesas,sh73a0-lcdc  # SH-Mobile AG5
> +
> +  reg:
> +    maxItems: 1
> +
> +  interrupts:
> +    maxItems: 1
> +
> +  clocks:
> +    minItems: 1
> +    maxItems: 5
> +    description:
> +      Only the functional clock is mandatory.
> +      Some of the optional clocks are model-dependent (e.g. "video" (a.k.a.
> +      "vou" or "dv_clk") is available on R-Mobile A1 only).
> +
> +  clock-names:
> +    minItems: 1
> +    maxItems: 5
> +    items:
> +      enum: [ fck, media, lclk, hdmi, video ]

Switching to per-item descriptions would allow documenting which clock
applies to which SoC.

Are enum items unique by default ?

This would allow a combination of clocks that doesn't include the fck
clock, that's not right.

> +
> +  power-domains:
> +    maxItems: 1
> +
> +  ports:
> +    $ref: /schemas/graph.yaml#/properties/ports
> +    description: |
> +      The connections to the output video ports are modeled using the OF graph
> +      bindings specified in Documentation/devicetree/bindings/graph.txt.

it's available in YAML form now. I'd just drop the "specified in ...".

> +      The number of ports and their assignment are model-dependent.
> +      Each port shall have a single endpoint.
> +
> +    properties:
> +      port@0:
> +        $ref: /schemas/graph.yaml#/properties/port
> +        description: LCD port (R-Mobile A1 and SH-Mobile AG5)
> +        unevaluatedProperties: false
> +
> +      port@1:
> +        $ref: /schemas/graph.yaml#/properties/port
> +        description: HDMI port (R-Mobile A1 LCDC1 and SH-Mobile AG5)
> +        unevaluatedProperties: false
> +
> +      port@2:
> +        $ref: /schemas/graph.yaml#/properties/port
> +        description: MIPI-DSI port (SH-Mobile AG5)
> +        unevaluatedProperties: false

Let's condition the ports on the compatible value to enable automatic
validation.

> +
> +    required:
> +      - port@0

Based on the above, port@1 is required too as it's present on all
supported SoCs. Let's condition this on the compatible value too.

> +
> +    unevaluatedProperties: false
> +
> +required:
> +  - compatible
> +  - reg
> +  - interrupts
> +  - clocks
> +  - clock-names
> +  - power-domains
> +  - ports
> +
> +additionalProperties: false
> +
> +examples:
> +  - |
> +    #include <dt-bindings/clock/r8a7740-clock.h>
> +    #include <dt-bindings/interrupt-controller/arm-gic.h>
> +
> +    lcdc0: lcd-controller@fe940000 {
> +        compatible = "renesas,r8a7740-lcdc";
> +        reg = <0xfe940000 0x4000>;
> +        interrupts = <GIC_SPI 177 IRQ_TYPE_LEVEL_HIGH>;
> +        clocks = <&mstp1_clks R8A7740_CLK_LCDC0>,
> +                 <&cpg_clocks R8A7740_CLK_M3>, <&lcdlclk0_clk>,
> +                 <&vou_clk>;
> +        clock-names = "fck", "media", "lclk", "video";
> +        power-domains = <&pd_a4lc>;
> +        status = "disabled";
> +
> +        ports {
> +            #address-cells = <1>;
> +            #size-cells = <0>;
> +
> +            port@0 {
> +                reg = <0>;
> +
> +                lcdc0_rgb: endpoint {
> +                };
> +            };
> +        };
> +    };
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 83e9f4ac6bedaa9f..dc1935c196cb0e0b 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -7023,6 +7023,7 @@ F:	Documentation/devicetree/bindings/display/bridge/renesas,dsi-csi2-tx.yaml
>  F:	Documentation/devicetree/bindings/display/bridge/renesas,dw-hdmi.yaml
>  F:	Documentation/devicetree/bindings/display/bridge/renesas,lvds.yaml
>  F:	Documentation/devicetree/bindings/display/renesas,du.yaml
> +F:	Documentation/devicetree/bindings/display/renesas,shmobile-lcdc.yaml
>  F:	drivers/gpu/drm/renesas/
>  F:	include/linux/platform_data/shmob_drm.h
>
Geert Uytterhoeven June 23, 2023, 3:19 p.m. UTC | #3
Hi Laurent,

On Fri, Jun 23, 2023 at 4:43 PM Laurent Pinchart
<laurent.pinchart@ideasonboard.com> wrote:
> On Thu, Jun 22, 2023 at 11:21:13AM +0200, Geert Uytterhoeven wrote:
> > Add device tree bindings for the LCD Controller (LCDC) found in Renesas
> > SuperH SH-Mobile and ARM SH/R-Mobile SOCs.
> >
> > Based on a plain text prototype by Laurent Pinchart.
> >
> > Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>

> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/display/renesas,shmobile-lcdc.yaml
> > @@ -0,0 +1,108 @@
> > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> > +%YAML 1.2
> > +---
> > +$id: http://devicetree.org/schemas/display/renesas,shmobile-lcdc.yaml#
> > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > +
> > +title: Renesas SH-Mobile LCD Controller (LCDC)
> > +
> > +maintainers:
> > +  - Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
>
> I'd be happy if you co-maintained this with me :-) Or even took
> ownership completely.

OK. Thinking about it ;-)

> > +
> > +properties:
> > +  compatible:
> > +    enum:
> > +      - renesas,r8a7740-lcdc # R-Mobile A1
> > +      - renesas,sh73a0-lcdc  # SH-Mobile AG5
> > +
> > +  reg:
> > +    maxItems: 1
> > +
> > +  interrupts:
> > +    maxItems: 1
> > +
> > +  clocks:
> > +    minItems: 1
> > +    maxItems: 5
> > +    description:
> > +      Only the functional clock is mandatory.
> > +      Some of the optional clocks are model-dependent (e.g. "video" (a.k.a.
> > +      "vou" or "dv_clk") is available on R-Mobile A1 only).
> > +
> > +  clock-names:
> > +    minItems: 1
> > +    maxItems: 5
> > +    items:
> > +      enum: [ fck, media, lclk, hdmi, video ]
>
> Switching to per-item descriptions would allow documenting which clock
> applies to which SoC.
>
> Are enum items unique by default ?

Given how about all clocks but fck are optional, it's a bit hard
to handle this in a perfect way.
Note that "pattern: '^dclkin\.[0123]$'" in renesas,du.yaml has the same issue.

> This would allow a combination of clocks that doesn't include the fck
> clock, that's not right.

Right. But when fixing the first to "fck", you have to duplicate all others.
So it should become something like:

  - const: fck
  - enum: [ media, lclk, hdmi, video ]
  - enum: [ media, lclk, hdmi, video ]
  - enum: [ media, lclk, hdmi, video ]
  - enum: [ media, lclk, hdmi, video ]

>
> > +
> > +  power-domains:
> > +    maxItems: 1
> > +
> > +  ports:
> > +    $ref: /schemas/graph.yaml#/properties/ports
> > +    description: |
> > +      The connections to the output video ports are modeled using the OF graph
> > +      bindings specified in Documentation/devicetree/bindings/graph.txt.
>
> it's available in YAML form now. I'd just drop the "specified in ...".

OK.

> > +      The number of ports and their assignment are model-dependent.
> > +      Each port shall have a single endpoint.
> > +
> > +    properties:
> > +      port@0:
> > +        $ref: /schemas/graph.yaml#/properties/port
> > +        description: LCD port (R-Mobile A1 and SH-Mobile AG5)
> > +        unevaluatedProperties: false
> > +
> > +      port@1:
> > +        $ref: /schemas/graph.yaml#/properties/port
> > +        description: HDMI port (R-Mobile A1 LCDC1 and SH-Mobile AG5)
> > +        unevaluatedProperties: false
> > +
> > +      port@2:
> > +        $ref: /schemas/graph.yaml#/properties/port
> > +        description: MIPI-DSI port (SH-Mobile AG5)
> > +        unevaluatedProperties: false
>
> Let's condition the ports on the compatible value to enable automatic
> validation.
>
> > +
> > +    required:
> > +      - port@0
>
> Based on the above, port@1 is required too as it's present on all
> supported SoCs. Let's condition this on the compatible value too.

It does not depend solely on the SoC, but also on the LCDC instance.
port@1 is not available on R-Mobile A1 LCDC0, only on LCDC1.

Gr{oetje,eeting}s,

                        Geert
Laurent Pinchart June 23, 2023, 3:33 p.m. UTC | #4
On Fri, Jun 23, 2023 at 05:19:45PM +0200, Geert Uytterhoeven wrote:
> On Fri, Jun 23, 2023 at 4:43 PM Laurent Pinchart wrote:
> > On Thu, Jun 22, 2023 at 11:21:13AM +0200, Geert Uytterhoeven wrote:
> > > Add device tree bindings for the LCD Controller (LCDC) found in Renesas
> > > SuperH SH-Mobile and ARM SH/R-Mobile SOCs.
> > >
> > > Based on a plain text prototype by Laurent Pinchart.
> > >
> > > Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
> 
> > > --- /dev/null
> > > +++ b/Documentation/devicetree/bindings/display/renesas,shmobile-lcdc.yaml
> > > @@ -0,0 +1,108 @@
> > > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> > > +%YAML 1.2
> > > +---
> > > +$id: http://devicetree.org/schemas/display/renesas,shmobile-lcdc.yaml#
> > > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > > +
> > > +title: Renesas SH-Mobile LCD Controller (LCDC)
> > > +
> > > +maintainers:
> > > +  - Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
> >
> > I'd be happy if you co-maintained this with me :-) Or even took
> > ownership completely.
> 
> OK. Thinking about it ;-)
> 
> > > +
> > > +properties:
> > > +  compatible:
> > > +    enum:
> > > +      - renesas,r8a7740-lcdc # R-Mobile A1
> > > +      - renesas,sh73a0-lcdc  # SH-Mobile AG5
> > > +
> > > +  reg:
> > > +    maxItems: 1
> > > +
> > > +  interrupts:
> > > +    maxItems: 1
> > > +
> > > +  clocks:
> > > +    minItems: 1
> > > +    maxItems: 5
> > > +    description:
> > > +      Only the functional clock is mandatory.
> > > +      Some of the optional clocks are model-dependent (e.g. "video" (a.k.a.
> > > +      "vou" or "dv_clk") is available on R-Mobile A1 only).
> > > +
> > > +  clock-names:
> > > +    minItems: 1
> > > +    maxItems: 5
> > > +    items:
> > > +      enum: [ fck, media, lclk, hdmi, video ]
> >
> > Switching to per-item descriptions would allow documenting which clock
> > applies to which SoC.
> >
> > Are enum items unique by default ?
> 
> Given how about all clocks but fck are optional, it's a bit hard
> to handle this in a perfect way.
> Note that "pattern: '^dclkin\.[0123]$'" in renesas,du.yaml has the same issue.
> 
> > This would allow a combination of clocks that doesn't include the fck
> > clock, that's not right.
> 
> Right. But when fixing the first to "fck", you have to duplicate all others.
> So it should become something like:
> 
>   - const: fck
>   - enum: [ media, lclk, hdmi, video ]
>   - enum: [ media, lclk, hdmi, video ]
>   - enum: [ media, lclk, hdmi, video ]
>   - enum: [ media, lclk, hdmi, video ]

It's not great. Any input from the DT maintainers ?

> > > +
> > > +  power-domains:
> > > +    maxItems: 1
> > > +
> > > +  ports:
> > > +    $ref: /schemas/graph.yaml#/properties/ports
> > > +    description: |
> > > +      The connections to the output video ports are modeled using the OF graph
> > > +      bindings specified in Documentation/devicetree/bindings/graph.txt.
> >
> > it's available in YAML form now. I'd just drop the "specified in ...".
> 
> OK.
> 
> > > +      The number of ports and their assignment are model-dependent.
> > > +      Each port shall have a single endpoint.
> > > +
> > > +    properties:
> > > +      port@0:
> > > +        $ref: /schemas/graph.yaml#/properties/port
> > > +        description: LCD port (R-Mobile A1 and SH-Mobile AG5)
> > > +        unevaluatedProperties: false
> > > +
> > > +      port@1:
> > > +        $ref: /schemas/graph.yaml#/properties/port
> > > +        description: HDMI port (R-Mobile A1 LCDC1 and SH-Mobile AG5)
> > > +        unevaluatedProperties: false
> > > +
> > > +      port@2:
> > > +        $ref: /schemas/graph.yaml#/properties/port
> > > +        description: MIPI-DSI port (SH-Mobile AG5)
> > > +        unevaluatedProperties: false
> >
> > Let's condition the ports on the compatible value to enable automatic
> > validation.
> >
> > > +
> > > +    required:
> > > +      - port@0
> >
> > Based on the above, port@1 is required too as it's present on all
> > supported SoCs. Let's condition this on the compatible value too.
> 
> It does not depend solely on the SoC, but also on the LCDC instance.
> port@1 is not available on R-Mobile A1 LCDC0, only on LCDC1.

Ah, my bad. It can't be mandatory indeed. I'd still prefer conditioning
ports to the compatible string for proper validation.
Laurent Pinchart June 23, 2023, 5:50 p.m. UTC | #5
Hi Geert,

Thank you for the patch.

On Thu, Jun 22, 2023 at 11:21:51AM +0200, Geert Uytterhoeven wrote:
> Add DT support, by:
>   1. Creating a panel bridge from DT, and attaching it to the encoder,
>   2. Replacing the custom connector with a bridge connector,
>   3. Obtaining clock configuration based on the compatible value.
> 
> Note that for now the driver uses a fixed clock configuration selecting
> the bus clock, as the current code to select other clock inputs needs
> changes to support any other SoCs than SH7724.
> 
> Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
> Cc: Rob Herring <robh+dt@kernel.org>
> Cc: Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org>
> Cc: Conor Dooley <conor+dt@kernel.org>
> Cc: devicetree@vger.kernel.org
> ---
> SH-Mobile AG5 (SH73A0) support is untested.
> 
> Unbind crashes when drm_encoder_cleanup() calls drm_bridge_detach(), as
> the bridge (allocated by devm_drm_panel_bridge_add()) has already been
> freed by that time.
> Should I allocate my encoder with devm_kzalloc(), instead of embedding
> it inside struct shmob_drm_device?

That shouldn't be needed, if you manage the memory for shmob_drm_device
with the DRM managed helpers.

Lifetime management of bridges is currently completely broken, there's
nothing that prevents bridges from being freed while still in use.
That's an issue in DRM, not in your driver.

> ---
>  .../gpu/drm/renesas/shmobile/shmob_drm_crtc.c | 101 +++++++++++++++---
>  .../gpu/drm/renesas/shmobile/shmob_drm_crtc.h |   1 +
>  .../gpu/drm/renesas/shmobile/shmob_drm_drv.c  |  27 ++++-
>  .../gpu/drm/renesas/shmobile/shmob_drm_drv.h  |   6 ++
>  4 files changed, 118 insertions(+), 17 deletions(-)
> 
> diff --git a/drivers/gpu/drm/renesas/shmobile/shmob_drm_crtc.c b/drivers/gpu/drm/renesas/shmobile/shmob_drm_crtc.c
> index 17456dde57637ab8..1ec87841658de4f0 100644
> --- a/drivers/gpu/drm/renesas/shmobile/shmob_drm_crtc.c
> +++ b/drivers/gpu/drm/renesas/shmobile/shmob_drm_crtc.c
> @@ -9,12 +9,16 @@
>  
>  #include <linux/clk.h>
>  #include <linux/media-bus-format.h>
> +#include <linux/of.h>
> +#include <linux/of_graph.h>
>  #include <linux/pm_runtime.h>
>  
>  #include <drm/drm_atomic.h>
>  #include <drm/drm_atomic_helper.h>
>  #include <drm/drm_atomic_state_helper.h>
>  #include <drm/drm_atomic_uapi.h>
> +#include <drm/drm_bridge.h>
> +#include <drm/drm_bridge_connector.h>
>  #include <drm/drm_crtc.h>
>  #include <drm/drm_crtc_helper.h>
>  #include <drm/drm_fb_dma_helper.h>
> @@ -23,6 +27,7 @@
>  #include <drm/drm_gem_dma_helper.h>
>  #include <drm/drm_modeset_helper.h>
>  #include <drm/drm_modeset_helper_vtables.h>
> +#include <drm/drm_panel.h>
>  #include <drm/drm_probe_helper.h>
>  #include <drm/drm_simple_kms_helper.h>
>  #include <drm/drm_vblank.h>
> @@ -35,10 +40,6 @@
>  #include "shmob_drm_plane.h"
>  #include "shmob_drm_regs.h"
>  
> -/*
> - * TODO: panel support
> - */
> -
>  /* -----------------------------------------------------------------------------
>   * Clock management
>   */
> @@ -129,7 +130,6 @@ static void shmob_drm_crtc_setup_geometry(struct shmob_drm_crtc *scrtc)
>  		value |= LDMT1R_VPOL;
>  	if (mode->flags & DRM_MODE_FLAG_NHSYNC)
>  		value |= LDMT1R_HPOL;
> -

This could be moved to one of the patches in the series that touch this
code.

>  	lcdc_write(sdev, LDMT1R, value);
>  
>  	value = ((mode->hdisplay / 8) << 16)			/* HDCN */
> @@ -191,7 +191,7 @@ static void shmob_drm_crtc_start(struct shmob_drm_crtc *scrtc)
>  {
>  	struct drm_crtc *crtc = &scrtc->base;
>  	struct shmob_drm_device *sdev = to_shmob_device(crtc->dev);
> -	const struct shmob_drm_interface_data *idata = &sdev->pdata->iface;
> +	unsigned int clk_div = sdev->config.clk_div;
>  	struct device *dev = sdev->dev;
>  	u32 value;
>  	int ret;
> @@ -220,17 +220,17 @@ static void shmob_drm_crtc_start(struct shmob_drm_crtc *scrtc)
>  	lcdc_write(sdev, LDPMR, 0);
>  
>  	value = sdev->lddckr;
> -	if (idata->clk_div) {
> +	if (clk_div) {
>  		/* FIXME: sh7724 can only use 42, 48, 54 and 60 for the divider
>  		 * denominator.
>  		 */
>  		lcdc_write(sdev, LDDCKPAT1R, 0);
> -		lcdc_write(sdev, LDDCKPAT2R, (1 << (idata->clk_div / 2)) - 1);
> +		lcdc_write(sdev, LDDCKPAT2R, (1 << (clk_div / 2)) - 1);
>  
> -		if (idata->clk_div == 1)
> +		if (clk_div == 1)
>  			value |= LDDCKR_MOSEL;
>  		else
> -			value |= idata->clk_div;
> +			value |= clk_div;
>  	}
>  
>  	lcdc_write(sdev, LDDCKR, value);
> @@ -479,7 +479,7 @@ int shmob_drm_crtc_create(struct shmob_drm_device *sdev)
>  }
>  
>  /* -----------------------------------------------------------------------------
> - * Encoder
> + * Legacy Encoder
>   */
>  
>  static bool shmob_drm_encoder_mode_fixup(struct drm_encoder *encoder,
> @@ -508,9 +508,43 @@ static const struct drm_encoder_helper_funcs encoder_helper_funcs = {
>  	.mode_fixup = shmob_drm_encoder_mode_fixup,
>  };
>  
> +/* -----------------------------------------------------------------------------
> + * Encoder
> + */
> +
> +static int shmob_drm_encoder_init(struct shmob_drm_device *sdev,
> +				  struct device_node *enc_node)
> +{
> +	struct drm_bridge *bridge;
> +	struct drm_panel *panel;
> +	int ret;
> +
> +	/* Create a panel bridge */
> +	panel = of_drm_find_panel(enc_node);

Using drm_of_find_panel_or_bridge() would allow supporting platforms
that connect a non-panel device to the SoC, in additional to the already
supported panels.

> +	if (IS_ERR(panel))
> +		return PTR_ERR(panel);
> +
> +	bridge = devm_drm_panel_bridge_add(sdev->dev, panel);
> +	if (IS_ERR(bridge))
> +		return PTR_ERR(bridge);
> +
> +	/* Attach the bridge to the encoder */
> +	ret = drm_bridge_attach(&sdev->encoder, bridge, NULL,
> +				DRM_BRIDGE_ATTACH_NO_CONNECTOR);
> +	if (ret) {
> +		dev_err(sdev->dev, "failed to attach bridge %pOF: %pe\n",
> +			bridge->of_node, ERR_PTR(ret));
> +		return ret;
> +	}
> +
> +	return 0;
> +}
> +
>  int shmob_drm_encoder_create(struct shmob_drm_device *sdev)
>  {
>  	struct drm_encoder *encoder = &sdev->encoder;
> +	struct device_node *np = sdev->dev->of_node;
> +	struct device_node *ep_node, *entity;
>  	int ret;
>  
>  	encoder->possible_crtcs = 1;
> @@ -520,13 +554,45 @@ int shmob_drm_encoder_create(struct shmob_drm_device *sdev)
>  	if (ret < 0)
>  		return ret;
>  
> -	drm_encoder_helper_add(encoder, &encoder_helper_funcs);
> +	if (sdev->pdata) {
> +		drm_encoder_helper_add(encoder, &encoder_helper_funcs);
> +		return 0;
> +	}
> +
> +	for_each_endpoint_of_node(np, ep_node) {
> +		struct of_endpoint ep;
> +
> +		ret = of_graph_parse_endpoint(ep_node, &ep);
> +		if (ret < 0) {
> +			of_node_put(ep_node);
> +			return ret;
> +		}
> +		/* Ignore all but the LCD port */
> +		if (ep.port || ep.id)
> +			continue;
> +
> +		entity = of_graph_get_remote_port_parent(ep.local_node);
> +		if (!entity)
> +			continue;
> +
> +		if (!of_device_is_available(entity)) {
> +			of_node_put(entity);
> +			continue;
> +		}
> +
> +		ret = shmob_drm_encoder_init(sdev, entity);
> +		if (ret < 0) {
> +			of_node_put(entity);
> +			of_node_put(ep_node);
> +			return ret;
> +		}
> +	}
>  
>  	return 0;
>  }
>  
>  /* -----------------------------------------------------------------------------
> - * Connector
> + * Legacy Connector
>   */
>  
>  static inline struct shmob_drm_connector *to_shmob_connector(struct drm_connector *connector)
> @@ -626,13 +692,20 @@ shmob_drm_connector_init(struct shmob_drm_device *sdev,
>  	return connector;
>  }
>  
> +/* -----------------------------------------------------------------------------
> + * Connector
> + */
> +
>  int shmob_drm_connector_create(struct shmob_drm_device *sdev,
>  			       struct drm_encoder *encoder)
>  {
>  	struct drm_connector *connector;
>  	int ret;
>  
> -	connector = shmob_drm_connector_init(sdev, encoder);
> +	if (sdev->pdata)
> +		connector = shmob_drm_connector_init(sdev, encoder);
> +	else
> +		connector = drm_bridge_connector_init(&sdev->ddev, encoder);
>  	if (IS_ERR(connector)) {
>  		dev_err(sdev->dev, "failed to created connector: %pe\n",
>  			connector);
> diff --git a/drivers/gpu/drm/renesas/shmobile/shmob_drm_crtc.h b/drivers/gpu/drm/renesas/shmobile/shmob_drm_crtc.h
> index 89a0746f9a35807d..16e1712dd04e0f2b 100644
> --- a/drivers/gpu/drm/renesas/shmobile/shmob_drm_crtc.h
> +++ b/drivers/gpu/drm/renesas/shmobile/shmob_drm_crtc.h
> @@ -29,6 +29,7 @@ struct shmob_drm_crtc {
>  	wait_queue_head_t flip_wait;
>  };
>  
> +/* Legacy connector */
>  struct shmob_drm_connector {
>  	struct drm_connector base;
>  	struct drm_encoder *encoder;
> diff --git a/drivers/gpu/drm/renesas/shmobile/shmob_drm_drv.c b/drivers/gpu/drm/renesas/shmobile/shmob_drm_drv.c
> index 576869164479ec6b..db72ca1c8b2f44c9 100644
> --- a/drivers/gpu/drm/renesas/shmobile/shmob_drm_drv.c
> +++ b/drivers/gpu/drm/renesas/shmobile/shmob_drm_drv.c
> @@ -11,6 +11,7 @@
>  #include <linux/io.h>
>  #include <linux/mm.h>
>  #include <linux/module.h>
> +#include <linux/of.h>
>  #include <linux/platform_device.h>
>  #include <linux/pm.h>
>  #include <linux/pm_runtime.h>
> @@ -147,11 +148,13 @@ static int shmob_drm_remove(struct platform_device *pdev)
>  static int shmob_drm_probe(struct platform_device *pdev)
>  {
>  	struct shmob_drm_platform_data *pdata = pdev->dev.platform_data;

How about dropping non-DT support ? That would simplify the driver.

> +	const struct shmob_drm_config *config;
>  	struct shmob_drm_device *sdev;
>  	struct drm_device *ddev;
>  	int ret;
>  
> -	if (pdata == NULL) {
> +	config = of_device_get_match_data(&pdev->dev);
> +	if (!config && !pdata) {
>  		dev_err(&pdev->dev, "no platform data\n");
>  		return -EINVAL;
>  	}
> @@ -167,7 +170,13 @@ static int shmob_drm_probe(struct platform_device *pdev)
>  
>  	ddev = &sdev->ddev;
>  	sdev->dev = &pdev->dev;
> -	sdev->pdata = pdata;
> +	if (config) {
> +		sdev->config = *config;
> +	} else {
> +		sdev->pdata = pdata;
> +		sdev->config.clk_source = pdata->clk_source;
> +		sdev->config.clk_div = pdata->iface.clk_div;
> +	}
>  	spin_lock_init(&sdev->irq_lock);
>  
>  	platform_set_drvdata(pdev, sdev);
> @@ -180,7 +189,7 @@ static int shmob_drm_probe(struct platform_device *pdev)
>  	if (ret)
>  		return ret;
>  
> -	ret = shmob_drm_setup_clocks(sdev, pdata->clk_source);
> +	ret = shmob_drm_setup_clocks(sdev, sdev->config.clk_source);
>  	if (ret < 0)
>  		return ret;
>  
> @@ -224,11 +233,23 @@ static int shmob_drm_probe(struct platform_device *pdev)
>  	return ret;
>  }
>  
> +static const struct shmob_drm_config shmob_arm_config = {
> +	.clk_source = SHMOB_DRM_CLK_BUS,
> +	.clk_div = 5,
> +};
> +
> +static const struct of_device_id shmob_drm_of_table[] __maybe_unused = {
> +	{ .compatible = "renesas,r8a7740-lcdc",	.data = &shmob_arm_config, },
> +	{ .compatible = "renesas,sh73a0-lcdc",	.data = &shmob_arm_config, },
> +	{ /* sentinel */ }
> +};
> +
>  static struct platform_driver shmob_drm_platform_driver = {
>  	.probe		= shmob_drm_probe,
>  	.remove		= shmob_drm_remove,
>  	.driver		= {
>  		.name	= "shmob-drm",
> +		.of_match_table = of_match_ptr(shmob_drm_of_table),
>  		.pm	= pm_sleep_ptr(&shmob_drm_pm_ops),
>  	},
>  };
> diff --git a/drivers/gpu/drm/renesas/shmobile/shmob_drm_drv.h b/drivers/gpu/drm/renesas/shmobile/shmob_drm_drv.h
> index 18907e5ace51c681..088ac5381e91e61a 100644
> --- a/drivers/gpu/drm/renesas/shmobile/shmob_drm_drv.h
> +++ b/drivers/gpu/drm/renesas/shmobile/shmob_drm_drv.h
> @@ -20,9 +20,15 @@ struct clk;
>  struct device;
>  struct drm_device;
>  
> +struct shmob_drm_config {
> +	enum shmob_drm_clk_source clk_source;
> +	unsigned int clk_div;
> +};
> +
>  struct shmob_drm_device {
>  	struct device *dev;
>  	const struct shmob_drm_platform_data *pdata;
> +	struct shmob_drm_config config;
>  
>  	void __iomem *mmio;
>  	struct clk *clock;
Sam Ravnborg June 23, 2023, 5:54 p.m. UTC | #6
On Fri, Jun 23, 2023 at 08:50:19PM +0300, Laurent Pinchart wrote:
> Hi Geert,
> 
> Thank you for the patch.
> 
> On Thu, Jun 22, 2023 at 11:21:51AM +0200, Geert Uytterhoeven wrote:
> > Add DT support, by:
> >   1. Creating a panel bridge from DT, and attaching it to the encoder,
> >   2. Replacing the custom connector with a bridge connector,
> >   3. Obtaining clock configuration based on the compatible value.
> > 
> > Note that for now the driver uses a fixed clock configuration selecting
> > the bus clock, as the current code to select other clock inputs needs
> > changes to support any other SoCs than SH7724.
> > 
> > Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
> > Cc: Rob Herring <robh+dt@kernel.org>
> > Cc: Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org>
> > Cc: Conor Dooley <conor+dt@kernel.org>
> > Cc: devicetree@vger.kernel.org
> > ---
> > SH-Mobile AG5 (SH73A0) support is untested.
> > 
> > Unbind crashes when drm_encoder_cleanup() calls drm_bridge_detach(), as
> > the bridge (allocated by devm_drm_panel_bridge_add()) has already been
> > freed by that time.
> > Should I allocate my encoder with devm_kzalloc(), instead of embedding
> > it inside struct shmob_drm_device?
> 
> That shouldn't be needed, if you manage the memory for shmob_drm_device
> with the DRM managed helpers.
> 
> Lifetime management of bridges is currently completely broken, there's
> nothing that prevents bridges from being freed while still in use.
> That's an issue in DRM, not in your driver.
> 
> > ---
> >  .../gpu/drm/renesas/shmobile/shmob_drm_crtc.c | 101 +++++++++++++++---
> >  .../gpu/drm/renesas/shmobile/shmob_drm_crtc.h |   1 +
> >  .../gpu/drm/renesas/shmobile/shmob_drm_drv.c  |  27 ++++-
> >  .../gpu/drm/renesas/shmobile/shmob_drm_drv.h  |   6 ++
> >  4 files changed, 118 insertions(+), 17 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/renesas/shmobile/shmob_drm_crtc.c b/drivers/gpu/drm/renesas/shmobile/shmob_drm_crtc.c
> > index 17456dde57637ab8..1ec87841658de4f0 100644
> > --- a/drivers/gpu/drm/renesas/shmobile/shmob_drm_crtc.c
> > +++ b/drivers/gpu/drm/renesas/shmobile/shmob_drm_crtc.c
> > @@ -9,12 +9,16 @@
> >  
> >  #include <linux/clk.h>
> >  #include <linux/media-bus-format.h>
> > +#include <linux/of.h>
> > +#include <linux/of_graph.h>
> >  #include <linux/pm_runtime.h>
> >  
> >  #include <drm/drm_atomic.h>
> >  #include <drm/drm_atomic_helper.h>
> >  #include <drm/drm_atomic_state_helper.h>
> >  #include <drm/drm_atomic_uapi.h>
> > +#include <drm/drm_bridge.h>
> > +#include <drm/drm_bridge_connector.h>
> >  #include <drm/drm_crtc.h>
> >  #include <drm/drm_crtc_helper.h>
> >  #include <drm/drm_fb_dma_helper.h>
> > @@ -23,6 +27,7 @@
> >  #include <drm/drm_gem_dma_helper.h>
> >  #include <drm/drm_modeset_helper.h>
> >  #include <drm/drm_modeset_helper_vtables.h>
> > +#include <drm/drm_panel.h>
> >  #include <drm/drm_probe_helper.h>
> >  #include <drm/drm_simple_kms_helper.h>
> >  #include <drm/drm_vblank.h>
> > @@ -35,10 +40,6 @@
> >  #include "shmob_drm_plane.h"
> >  #include "shmob_drm_regs.h"
> >  
> > -/*
> > - * TODO: panel support
> > - */
> > -
> >  /* -----------------------------------------------------------------------------
> >   * Clock management
> >   */
> > @@ -129,7 +130,6 @@ static void shmob_drm_crtc_setup_geometry(struct shmob_drm_crtc *scrtc)
> >  		value |= LDMT1R_VPOL;
> >  	if (mode->flags & DRM_MODE_FLAG_NHSYNC)
> >  		value |= LDMT1R_HPOL;
> > -
> 
> This could be moved to one of the patches in the series that touch this
> code.
> 
> >  	lcdc_write(sdev, LDMT1R, value);
> >  
> >  	value = ((mode->hdisplay / 8) << 16)			/* HDCN */
> > @@ -191,7 +191,7 @@ static void shmob_drm_crtc_start(struct shmob_drm_crtc *scrtc)
> >  {
> >  	struct drm_crtc *crtc = &scrtc->base;
> >  	struct shmob_drm_device *sdev = to_shmob_device(crtc->dev);
> > -	const struct shmob_drm_interface_data *idata = &sdev->pdata->iface;
> > +	unsigned int clk_div = sdev->config.clk_div;
> >  	struct device *dev = sdev->dev;
> >  	u32 value;
> >  	int ret;
> > @@ -220,17 +220,17 @@ static void shmob_drm_crtc_start(struct shmob_drm_crtc *scrtc)
> >  	lcdc_write(sdev, LDPMR, 0);
> >  
> >  	value = sdev->lddckr;
> > -	if (idata->clk_div) {
> > +	if (clk_div) {
> >  		/* FIXME: sh7724 can only use 42, 48, 54 and 60 for the divider
> >  		 * denominator.
> >  		 */
> >  		lcdc_write(sdev, LDDCKPAT1R, 0);
> > -		lcdc_write(sdev, LDDCKPAT2R, (1 << (idata->clk_div / 2)) - 1);
> > +		lcdc_write(sdev, LDDCKPAT2R, (1 << (clk_div / 2)) - 1);
> >  
> > -		if (idata->clk_div == 1)
> > +		if (clk_div == 1)
> >  			value |= LDDCKR_MOSEL;
> >  		else
> > -			value |= idata->clk_div;
> > +			value |= clk_div;
> >  	}
> >  
> >  	lcdc_write(sdev, LDDCKR, value);
> > @@ -479,7 +479,7 @@ int shmob_drm_crtc_create(struct shmob_drm_device *sdev)
> >  }
> >  
> >  /* -----------------------------------------------------------------------------
> > - * Encoder
> > + * Legacy Encoder
> >   */
> >  
> >  static bool shmob_drm_encoder_mode_fixup(struct drm_encoder *encoder,
> > @@ -508,9 +508,43 @@ static const struct drm_encoder_helper_funcs encoder_helper_funcs = {
> >  	.mode_fixup = shmob_drm_encoder_mode_fixup,
> >  };
> >  
> > +/* -----------------------------------------------------------------------------
> > + * Encoder
> > + */
> > +
> > +static int shmob_drm_encoder_init(struct shmob_drm_device *sdev,
> > +				  struct device_node *enc_node)
> > +{
> > +	struct drm_bridge *bridge;
> > +	struct drm_panel *panel;
> > +	int ret;
> > +
> > +	/* Create a panel bridge */
> > +	panel = of_drm_find_panel(enc_node);
> 
> Using drm_of_find_panel_or_bridge() would allow supporting platforms
> that connect a non-panel device to the SoC, in additional to the already
> supported panels.
Geert Uytterhoeven June 23, 2023, 6:09 p.m. UTC | #7
Hi Sam, Laurent,

On Fri, Jun 23, 2023 at 7:54 PM Sam Ravnborg <sam@ravnborg.org> wrote:
> On Fri, Jun 23, 2023 at 08:50:19PM +0300, Laurent Pinchart wrote:
> > On Thu, Jun 22, 2023 at 11:21:51AM +0200, Geert Uytterhoeven wrote:
> > > Add DT support, by:
> > >   1. Creating a panel bridge from DT, and attaching it to the encoder,
> > >   2. Replacing the custom connector with a bridge connector,
> > >   3. Obtaining clock configuration based on the compatible value.
> > >
> > > Note that for now the driver uses a fixed clock configuration selecting
> > > the bus clock, as the current code to select other clock inputs needs
> > > changes to support any other SoCs than SH7724.
> > >
> > > Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
> > > Cc: Rob Herring <robh+dt@kernel.org>
> > > Cc: Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org>
> > > Cc: Conor Dooley <conor+dt@kernel.org>
> > > Cc: devicetree@vger.kernel.org
> > > ---
> > > SH-Mobile AG5 (SH73A0) support is untested.
> > >
> > > Unbind crashes when drm_encoder_cleanup() calls drm_bridge_detach(), as
> > > the bridge (allocated by devm_drm_panel_bridge_add()) has already been
> > > freed by that time.
> > > Should I allocate my encoder with devm_kzalloc(), instead of embedding
> > > it inside struct shmob_drm_device?
> >
> > That shouldn't be needed, if you manage the memory for shmob_drm_device
> > with the DRM managed helpers.

Well, Marek said unbind works fine in drivers/gpu/drm/mxsfb/lcdif_drv.c,
where the order is:

    bridge = devm_drm_of_get_bridge(...)
    encoder = devm_kzalloc(...)
    drm_encoder_init(...)

> > Lifetime management of bridges is currently completely broken, there's
> > nothing that prevents bridges from being freed while still in use.
> > That's an issue in DRM, not in your driver.

OK ;-) (or :-(

> > > --- a/drivers/gpu/drm/renesas/shmobile/shmob_drm_crtc.c
> > > +++ b/drivers/gpu/drm/renesas/shmobile/shmob_drm_crtc.c
> > > @@ -508,9 +508,43 @@ static const struct drm_encoder_helper_funcs encoder_helper_funcs = {
> > >     .mode_fixup = shmob_drm_encoder_mode_fixup,
> > >  };
> > >
> > > +/* -----------------------------------------------------------------------------
> > > + * Encoder
> > > + */
> > > +
> > > +static int shmob_drm_encoder_init(struct shmob_drm_device *sdev,
> > > +                             struct device_node *enc_node)
> > > +{
> > > +   struct drm_bridge *bridge;
> > > +   struct drm_panel *panel;
> > > +   int ret;
> > > +
> > > +   /* Create a panel bridge */
> > > +   panel = of_drm_find_panel(enc_node);
> >
> > Using drm_of_find_panel_or_bridge() would allow supporting platforms
> > that connect a non-panel device to the SoC, in additional to the already
> > supported panels.
>
> From the documentation of drm_of_find_panel_or_bridge():
>
>  * This function is deprecated and should not be used in new drivers. Use
>  * devm_drm_of_get_bridge() instead.
>
> I suggest to go that route.

OK (do I have the feeling that these helpers are sometimes deprecated
faster than they are written? ;-)

> > > @@ -147,11 +148,13 @@ static int shmob_drm_remove(struct platform_device *pdev)
> > >  static int shmob_drm_probe(struct platform_device *pdev)
> > >  {
> > >     struct shmob_drm_platform_data *pdata = pdev->dev.platform_data;
> >
> > How about dropping non-DT support ? That would simplify the driver.
>
> +1 for that, without knowing the implications.

That depends on your priorities: do you want to migrate all users of
sh_mobile_lcdc_fb to shmob_drm, or do you want the SuperH users to
stick with sh_mobile_lcdc_fb until they have migrated to DT? ;-)

Regardless of the above, I do not have (visible) access to any of the
affected SH772[234] platforms...

Gr{oetje,eeting}s,

                        Geert
Laurent Pinchart June 23, 2023, 6:52 p.m. UTC | #8
On Fri, Jun 23, 2023 at 08:09:53PM +0200, Geert Uytterhoeven wrote:
> On Fri, Jun 23, 2023 at 7:54 PM Sam Ravnborg <sam@ravnborg.org> wrote:
> > On Fri, Jun 23, 2023 at 08:50:19PM +0300, Laurent Pinchart wrote:
> > > On Thu, Jun 22, 2023 at 11:21:51AM +0200, Geert Uytterhoeven wrote:
> > > > Add DT support, by:
> > > >   1. Creating a panel bridge from DT, and attaching it to the encoder,
> > > >   2. Replacing the custom connector with a bridge connector,
> > > >   3. Obtaining clock configuration based on the compatible value.
> > > >
> > > > Note that for now the driver uses a fixed clock configuration selecting
> > > > the bus clock, as the current code to select other clock inputs needs
> > > > changes to support any other SoCs than SH7724.
> > > >
> > > > Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
> > > > Cc: Rob Herring <robh+dt@kernel.org>
> > > > Cc: Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org>
> > > > Cc: Conor Dooley <conor+dt@kernel.org>
> > > > Cc: devicetree@vger.kernel.org
> > > > ---
> > > > SH-Mobile AG5 (SH73A0) support is untested.
> > > >
> > > > Unbind crashes when drm_encoder_cleanup() calls drm_bridge_detach(), as
> > > > the bridge (allocated by devm_drm_panel_bridge_add()) has already been
> > > > freed by that time.
> > > > Should I allocate my encoder with devm_kzalloc(), instead of embedding
> > > > it inside struct shmob_drm_device?
> > >
> > > That shouldn't be needed, if you manage the memory for shmob_drm_device
> > > with the DRM managed helpers.
> 
> Well, Marek said unbind works fine in drivers/gpu/drm/mxsfb/lcdif_drv.c,
> where the order is:
> 
>     bridge = devm_drm_of_get_bridge(...)
>     encoder = devm_kzalloc(...)
>     drm_encoder_init(...)
> 
> > > Lifetime management of bridges is currently completely broken, there's
> > > nothing that prevents bridges from being freed while still in use.
> > > That's an issue in DRM, not in your driver.
> 
> OK ;-) (or :-(
> 
> > > > --- a/drivers/gpu/drm/renesas/shmobile/shmob_drm_crtc.c
> > > > +++ b/drivers/gpu/drm/renesas/shmobile/shmob_drm_crtc.c
> > > > @@ -508,9 +508,43 @@ static const struct drm_encoder_helper_funcs encoder_helper_funcs = {
> > > >     .mode_fixup = shmob_drm_encoder_mode_fixup,
> > > >  };
> > > >
> > > > +/* -----------------------------------------------------------------------------
> > > > + * Encoder
> > > > + */
> > > > +
> > > > +static int shmob_drm_encoder_init(struct shmob_drm_device *sdev,
> > > > +                             struct device_node *enc_node)
> > > > +{
> > > > +   struct drm_bridge *bridge;
> > > > +   struct drm_panel *panel;
> > > > +   int ret;
> > > > +
> > > > +   /* Create a panel bridge */
> > > > +   panel = of_drm_find_panel(enc_node);
> > >
> > > Using drm_of_find_panel_or_bridge() would allow supporting platforms
> > > that connect a non-panel device to the SoC, in additional to the already
> > > supported panels.
> >
> > From the documentation of drm_of_find_panel_or_bridge():
> >
> >  * This function is deprecated and should not be used in new drivers. Use
> >  * devm_drm_of_get_bridge() instead.

Indeed, my bad/ devm_drm_of_get_bridge() is better.

> > I suggest to go that route.
> 
> OK (do I have the feeling that these helpers are sometimes deprecated
> faster than they are written? ;-)
> 
> > > > @@ -147,11 +148,13 @@ static int shmob_drm_remove(struct platform_device *pdev)
> > > >  static int shmob_drm_probe(struct platform_device *pdev)
> > > >  {
> > > >     struct shmob_drm_platform_data *pdata = pdev->dev.platform_data;
> > >
> > > How about dropping non-DT support ? That would simplify the driver.
> >
> > +1 for that, without knowing the implications.
> 
> That depends on your priorities: do you want to migrate all users of
> sh_mobile_lcdc_fb to shmob_drm, or do you want the SuperH users to
> stick with sh_mobile_lcdc_fb until they have migrated to DT? ;-)

I'd vote for dropping LCDC support from the SH users. Does anyone
*really* need it ? If they do, they should convert the board to DT.

> Regardless of the above, I do not have (visible) access to any of the
> affected SH772[234] platforms...
Geert Uytterhoeven July 17, 2023, 1:57 p.m. UTC | #9
Hi Rob,

Thanks for your review!

On Thu, Jun 22, 2023 at 4:52 PM Rob Herring <robh@kernel.org> wrote:
> On Thu, Jun 22, 2023 at 11:21:13AM +0200, Geert Uytterhoeven wrote:
> > Add device tree bindings for the LCD Controller (LCDC) found in Renesas
> > SuperH SH-Mobile and ARM SH/R-Mobile SOCs.
> >
> > Based on a plain text prototype by Laurent Pinchart.
> >
> > Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>

> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/display/renesas,shmobile-lcdc.yaml
> > @@ -0,0 +1,108 @@
> > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> > +%YAML 1.2
> > +---
> > +$id: http://devicetree.org/schemas/display/renesas,shmobile-lcdc.yaml#
> > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > +
> > +title: Renesas SH-Mobile LCD Controller (LCDC)
> > +
> > +maintainers:
> > +  - Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
> > +
> > +properties:
> > +  compatible:
> > +    enum:
> > +      - renesas,r8a7740-lcdc # R-Mobile A1
> > +      - renesas,sh73a0-lcdc  # SH-Mobile AG5
> > +
> > +  reg:
> > +    maxItems: 1
> > +
> > +  interrupts:
> > +    maxItems: 1
> > +
> > +  clocks:
> > +    minItems: 1
> > +    maxItems: 5
> > +    description:
> > +      Only the functional clock is mandatory.
> > +      Some of the optional clocks are model-dependent (e.g. "video" (a.k.a.
> > +      "vou" or "dv_clk") is available on R-Mobile A1 only).
> > +
> > +  clock-names:
> > +    minItems: 1
> > +    maxItems: 5
> > +    items:
> > +      enum: [ fck, media, lclk, hdmi, video ]
> > +
> > +  power-domains:
> > +    maxItems: 1
> > +
> > +  ports:
> > +    $ref: /schemas/graph.yaml#/properties/ports
> > +    description: |
> > +      The connections to the output video ports are modeled using the OF graph
> > +      bindings specified in Documentation/devicetree/bindings/graph.txt.
>
> Please read this file.
>
> > +      The number of ports and their assignment are model-dependent.
> > +      Each port shall have a single endpoint.
>
> I'd just drop the whole description.
>
> > +
> > +    properties:
> > +      port@0:
> > +        $ref: /schemas/graph.yaml#/properties/port
> > +        description: LCD port (R-Mobile A1 and SH-Mobile AG5)
> > +        unevaluatedProperties: false
>
> Don't need this.

You mean the "unevaluatedProperties: false"?
Or more?

Thanks again!

Gr{oetje,eeting}s,

                        Geert