| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Date: Tue, 21 May 2024 09:30:22 +0200
From: Krzysztof Kozlowski <krzk@...nel.org>
To: keith <keith.zhao@...rfivetech.com>, andrzej.hajda@...el.com,
neil.armstrong@...aro.org, rfoss@...nel.org,
Laurent.pinchart@...asonboard.com, jonas@...boo.se,
jernej.skrabec@...il.com, maarten.lankhorst@...ux.intel.com,
mripard@...nel.org, tzimmermann@...e.de, airlied@...il.com, daniel@...ll.ch,
robh@...nel.org, krzk+dt@...nel.org, conor+dt@...nel.org,
hjc@...k-chips.com, heiko@...ech.de, andy.yan@...k-chips.com,
xingyu.wu@...rfivetech.com, p.zabel@...gutronix.de,
jack.zhu@...rfivetech.com, shengyang.chen@...rfivetech.com
Cc: dri-devel@...ts.freedesktop.org, devicetree@...r.kernel.org,
linux-kernel@...r.kernel.org, linux-arm-kernel@...ts.infradead.org
Subject: Re: [PATCH v4 01/10] dt-bindings: display: Add YAML schema for JH7110
display pipeline
On 21/05/2024 12:58, keith wrote:
> JH7110 SoC display pipeline includes a display controller and hdmi.
> Dc controller IP : Vivante DC8200 Dual Display
> HDMI IP : INNOSILICON HDMI2.0
>
> As the INNO hdmi ip is also used by rockchip SoC in the driver code,
> the innosilicon,inno-hdmi.yaml schema containing the common properties
> for the INNO DesignWare HDMI TX controller isn't a full device
> tree binding specification, but is meant to be referenced by
> platform-specific bindings for the IP core.
>
> Signed-off-by: keith <keith.zhao@...rfivetech.com>
> ---
> .../display/bridge/innosilicon,inno-hdmi.yaml | 49 +++++
> .../display/rockchip/rockchip,inno-hdmi.yaml | 27 +--
> .../starfive/starfive,dsi-encoder.yaml | 92 ++++++++++
> .../starfive/starfive,jh7110-dc8200.yaml | 169 ++++++++++++++++++
> .../starfive/starfive,jh7110-inno-hdmi.yaml | 75 ++++++++
> .../soc/starfive/starfive,jh7110-syscon.yaml | 1 +
> MAINTAINERS | 8 +
> 7 files changed, 396 insertions(+), 25 deletions(-)
> create mode 100644 Documentation/devicetree/bindings/display/bridge/innosilicon,inno-hdmi.yaml
> create mode 100644 Documentation/devicetree/bindings/display/starfive/starfive,dsi-encoder.yaml
> create mode 100644 Documentation/devicetree/bindings/display/starfive/starfive,jh7110-dc8200.yaml
> create mode 100644 Documentation/devicetree/bindings/display/starfive/starfive,jh7110-inno-hdmi.yaml
>
> diff --git a/Documentation/devicetree/bindings/display/bridge/innosilicon,inno-hdmi.yaml b/Documentation/devicetree/bindings/display/bridge/innosilicon,inno-hdmi.yaml
> new file mode 100644
> index 000000000000..8540174dcaeb
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/display/bridge/innosilicon,inno-hdmi.yaml
> @@ -0,0 +1,49 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/display/bridge/innosilicon,inno-hdmi.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Common Properties for Innosilicon HDMI TX IP
Your patch is difficult to review. Split changing existing bindings (and
defining common part) to a separate patch.
> +
> +maintainers:
> + - Keith Zhao <keith.zhao@...rfivetech.com>
> +
> +description: |
> + This document defines device tree properties for the Innosilicon HDMI TX
Nothing improved here. Don't say obvious that this documents says
something. It cannot do anything else.
"Innosilicon HDMI TX is a foo bar device present on zap SoC ...."
> + controller (INNO HDMI) IP core. It doesn't constitute a full device tree
> + binding specification by itself but is meant to be referenced by device tree
> + bindings for the platform-specific integrations of the INNO HDMI.
I don't understand this at all. I don't know what is "full device tree
binding specification".
> +
> + When referenced from platform device tree bindings the properties defined in
> + this document are defined as follows. The platform device tree bindings are
> + responsible for defining whether each property is required or optional.
Nothing improved - drop paragraph.
> +
> +properties:
> + reg:
> + maxItems: 1
> +
> + interrupts:
> + maxItems: 1
> +
> + ports:
> + $ref: /schemas/graph.yaml#/properties/ports
> +
> + properties:
> + port@0:
> + $ref: /schemas/graph.yaml#/properties/port
> + description:
> + Port node with one endpoint connected to a display controller node.
> +
> + port@1:
> + $ref: /schemas/graph.yaml#/properties/port
> + description:
> + Port node with one endpoint connected to a hdmi-connector node.
> +
> + required:
> + - port@0
> + - port@1
..
> diff --git a/Documentation/devicetree/bindings/display/starfive/starfive,dsi-encoder.yaml b/Documentation/devicetree/bindings/display/starfive/starfive,dsi-encoder.yaml
> new file mode 100644
> index 000000000000..07aa147a9db1
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/display/starfive/starfive,dsi-encoder.yaml
> @@ -0,0 +1,92 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/display/starfive/starfive,dsi-encoder.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: starfive jh7110 SoC Encoder
> +
> +maintainers:
> + - Keith Zhao <keith.zhao@...rfivetech.com>
> +
> +description:
> + Device-Tree bindings for simple encoder.
Again you ignored the comments.
When you receive a comment, apply it everywhere, not in only one part of
patch while keeping wrong code everywhere else.
> + Simple encoder driver only has basic functionality.
Not related to bindings, drop. This is about hardware, not your driver.
> + the hardware of dc8200 has 2 output interface, and uses
> + syscon to select which one to be used.
Nothing improved.
Read my previous comments:
1. Please make it a proper sentences, with proper wrapping.
> +
> +properties:
> + compatible:
> + const: starfive,dsi-encoder
> +
> + starfive,syscon:
> + $ref: /schemas/types.yaml#/definitions/phandle-array
> + items:
> + - items:
> + - description: phandle to syscon that select crtc output.
> + - description: Offset of crtc selection
> + - description: Shift of crtc selection
> + description:
> + A phandle to syscon with two arguments that configure select output.
> + The argument one is the offset of crtc selection, the
> + argument two is the shift of crtc selection.
Don't repeat constraints in free form text. Say something useful
instead, like what is its purpose.
No reg? Then why this is not just part of syscon? That's not a separate
device, no.
You received a feedback already about this: do not create fake bindings
just to instantiate your drivers.
> +
> + ports:
> + $ref: /schemas/graph.yaml#/properties/ports
> +
> + properties:
> + port@0:
> + $ref: /schemas/graph.yaml#/properties/port
> + description:
> + The first port should be the input coming from the associated dc channel.
> +
> + port@1:
> + $ref: /schemas/graph.yaml#/properties/port
> + description:
> + The second port should be the output coming from the associated bridge.
> +
> + required:
> + - port@0
> + - port@1
> +
> +required:
> + - compatible
> + - ports
> +
> +additionalProperties: false
> +
> +examples:
> + - |
> + dssctrl: dssctrl@...b0000 {
Drop node, not erlated.
> + compatible = "starfive,jh7110-vout-syscon", "syscon";
> + reg = <0x295b0000 0x90>;
> + };
> +
> + dsi_encoder: dsi_encoder {
Totally messed indentation. Use 4 spaces for example indentation.
Also, drop label.
Also, underscores are not allowed in node names.
Also, Node names should be generic. See also an explanation and list of
examples (not exhaustive) in DT specification:
https://devicetree-specification.readthedocs.io/en/latest/chapter2-devicetree-basics.html#generic-names-recommendation
This is terrible code.
> + compatible = "starfive,dsi-encoder";
> + starfive,syscon = <&dssctrl 0x8 0x12>;
> +
> + ports {
> + #address-cells = <1>;
> + #size-cells = <0>;
> + /* input */
> + port@0 {
> + #address-cells = <1>;
> + #size-cells = <0>;
> + reg = <0>;
> + dsi_input0:endpoint@0 {
> + reg = <0>;
> + remote-endpoint = <&dc_out_dpi1>;
> + };
> + };
> + /* output */
> + port@1 {
> + reg = <1>;
> + #address-cells = <1>;
> + #size-cells = <0>;
> + dsi_out:endpoint {
> + remote-endpoint = <&mipi_in>;
> + };
> + };
> + };
> + };
> diff --git a/Documentation/devicetree/bindings/display/starfive/starfive,jh7110-dc8200.yaml b/Documentation/devicetree/bindings/display/starfive/starfive,jh7110-dc8200.yaml
> new file mode 100644
> index 000000000000..a28dfdd471b6
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/display/starfive/starfive,jh7110-dc8200.yaml
> @@ -0,0 +1,169 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/display/starfive/starfive,jh7110-dc8200.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: STARFIVE JH7110 SoC display controller
> +
> +description:
> + The STARFIVE JH7110 SoC uses the display controller based on Verisilicon IP(DC8200)
> + to transfer the image data from a video memory buffer to an external LCD interface.
> +
> + pipe0 binds HDMI for primary display,
> + pipe1 binds DSI for external display.
> +
> + +------------------------------+
> + | |
> + | |
> + +----+ | +-------------------+ | +-------+ +------+ +------+
> + | +----->+ dc controller 0 +--->----->+HDMICtl| ->+ PHY +-->+PANEL0+
> + |AXI | | +-------------------+ | +-------+ +------+ +------+
> + | | | |
> + | | | |
> + | | | |
> + | | | |
> + |APB | | +-------------------+ +---------+ +------+ +-------+
> + | +----->+ dc controller 1 +--->---->+ dsiTx +--->+DPHY +->+ PANEL1+
> + | | | +-------------------+ +---------+ +------+ +-------+
> + +----+ | |
> + +------------------------------+
> +
> +maintainers:
> + - Keith Zhao <keith.zhao@...rfivetech.com>
> +
> +properties:
> + compatible:
> + const: starfive,jh7110-dc8200
> +
> + reg:
> + items:
> + - description: host interface address and length
> + - description: display physical base address and length
> +
> + reg-names:
> + items:
> + - const: host
> + - const: base
> +
> + clocks:
> + items:
> + - description: Clock for display system noc bus.
> + - description: Core clock for display controller.
> + - description: Clock for axi bus to access ddr.
> + - description: Clock for ahb bus to R/W the phy regs.
> + - description: Pixel clock for display channel 0.
> + - description: Pixel clock for display channel 1.
> + - description: Pixel clock from hdmi.
> + - description: Pixel clock for soc .
> +
> + clock-names:
> + items:
> + - const: noc_bus
> + - const: dc_core
> + - const: axi_core
> + - const: ahb
> + - const: channel0
> + - const: channel1
> + - const: hdmi_tx
> + - const: dc_parent
> +
> + resets:
> + items:
> + - description: Reset for axi bus.
> + - description: Reset for ahb bus.
> + - description: Core reset of display controller.
> +
> + reset-names:
> + items:
> + - const: axi
> + - const: ahb
> + - const: core
> +
> + interrupts:
> + items:
> + - description: The interrupt will be generated when DC finish one frame
> +
> + ports:
> + $ref: /schemas/graph.yaml#/properties/ports
> +
> + properties:
> + port@0:
> + $ref: /schemas/graph.yaml#/properties/port
> + description:
> + channel 0 output
> +
> + port@1:
> + $ref: /schemas/graph.yaml#/properties/port
> + description:
> + channel 1 output
> +
> + required:
> + - port@0
> + - port@1
> +
> +required:
> + - compatible
> + - reg
> + - interrupts
> + - clocks
> + - clock-names
> + - ports
> +
> +additionalProperties: false
> +
> +examples:
> + - |
> + #include <dt-bindings/clock/starfive,jh7110-crg.h>
> + #include <dt-bindings/reset/starfive,jh7110-crg.h>
> + dc8200: lcd-controller@...00000 {
> + compatible = "starfive,jh7110-dc8200";
> + reg = <0x29400000 0x100>,
> + <0x29400800 0x2000>;
> + reg-names = "host", "base";
> +
> + interrupts = <95>;
> + clocks = <&syscrg JH7110_SYSCLK_NOC_BUS_DISP_AXI>,
> + <&voutcrg JH7110_VOUTCLK_DC8200_CORE>,
Align the lines. In other places as well.
> + <&voutcrg JH7110_VOUTCLK_DC8200_AXI>,
> + <&voutcrg JH7110_VOUTCLK_DC8200_AHB>,
> + <&voutcrg JH7110_VOUTCLK_DC8200_PIX0>,
> + <&voutcrg JH7110_VOUTCLK_DC8200_PIX1>,
> + <&hdmitx0_pixelclk>,
> + <&voutcrg JH7110_VOUTCLK_DC8200_PIX>;
> + clock-names = "noc_bus", "dc_core", "axi_core", "ahb",
> + "channel0", "channel1", "hdmi_tx", "dc_parent";
> + resets = <&voutcrg JH7110_VOUTRST_DC8200_AXI>,
> + <&voutcrg JH7110_VOUTRST_DC8200_AHB>,
> + <&voutcrg JH7110_VOUTRST_DC8200_CORE>;
> + reset-names = "axi","ahb", "core";
> +
> + crtc_out: ports {
Totally messed indentation.
> diff --git a/Documentation/devicetree/bindings/display/starfive/starfive,jh7110-inno-hdmi.yaml b/Documentation/devicetree/bindings/display/starfive/starfive,jh7110-inno-hdmi.yaml
> new file mode 100644
> index 000000000000..bfd7dc41fc14
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/display/starfive/starfive,jh7110-inno-hdmi.yaml
> @@ -0,0 +1,75 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/display/starfive/starfive,jh7110-inno-hdmi.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Starfive JH7110 Innosilicon HDMI controller
> +
> +maintainers:
> + - Keith Zhao <keith.zhao@...rfivetech.com>
> +
> +allOf:
> + - $ref: ../bridge/innosilicon,inno-hdmi.yaml#
> +
> +properties:
> + compatible:
> + const: "starfive,jh7110-inno-hdmi"
You did not test it. Drop quotes.
Untested bindings, so I will skip review for now.
Best regards,
Krzysztof
Powered by blists - more mailing lists