| Message ID | 20241120061848.196754-2-keith.zhao@starfivetech.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | drm/verisilicon : support DC8200 and inno hdmi | expand |
On Wed, Nov 20, 2024 at 02:18:40PM +0800, keith zhao wrote: > - Added bindings to support the display subsystem on the JH7110 SoC. Please do not use "This commit/patch/change" (implied) and past tense, but imperative mood. See longer explanation here: https://elixir.bootlin.com/linux/v5.17.1/source/Documentation/process/submitting-patches.rst#L95 > > - Included the DC8200 display controller and Inno HDMI controller. > > - Created innosilicon,inno-hdmi.yaml schema containing common properties > for the Inno DesignWare HDMI TX controller. > This isn't a full device tree binding specification, > but is intended to be referenced by platform-specific bindings > for the IP core. > > Signed-off-by: keith zhao <keith.zhao@starfivetech.com> > --- > .../display/bridge/innosilicon,inno-hdmi.yaml | 45 +++++ > .../display/rockchip/rockchip,inno-hdmi.yaml | 27 +-- > .../starfive/starfive,jh7110-dc8200.yaml | 176 ++++++++++++++++++ > .../starfive/starfive,jh7110-inno-hdmi.yaml | 91 +++++++++ > .../soc/starfive/starfive,jh7110-syscon.yaml | 1 + I do not see how you addressed my feedback. I asked you to split the patch. Where is the split? Your answer to my request to split was "Background is ...", yeah, so what? You are not going to split? > MAINTAINERS | 8 + > 6 files changed, 323 insertions(+), 25 deletions(-) > create mode 100644 Documentation/devicetree/bindings/display/bridge/innosilicon,inno-hdmi.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..f2543aebc312 > --- /dev/null > +++ b/Documentation/devicetree/bindings/display/bridge/innosilicon,inno-hdmi.yaml > @@ -0,0 +1,45 @@ > +# 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 > + > +maintainers: > + - keith zhao <keith.zhao@starfivetech.com> > + > +description: | Do not need '|' unless you need to preserve formatting. > + Innosilicon HDMI TX is an HDMI transmission device integrated into the zap SoC. Keep and extend. > + This document specifies the device tree properties for the INNO HDMI IP core. Keep... but what is INNO HDMI IP core? Another name? Different block? > + It is intended to be referenced by platform-specific device tree bindings, > + which will determine the necessity of each property. Not much improved here. Last two sentences are almost useless. Again - document the hardware. Drop this sentence. I already complained about this. I already complained that you keep ignoring my comments and this does not improve much. Since you did not implement my main feedback I am going to NAK the rest except one more thing: ... > + > +properties: > + compatible: > + const: "starfive,jh7110-inno-hdmi" I could not be more specific in my previous feedback. Improvements? No. Stop wasting our time. NAK. Best regards, Krzysztof
> -----Original Message----- > From: Krzysztof Kozlowski <krzk@kernel.org> > Sent: 2024年11月21日 16:23 > To: Keith Zhao <keith.zhao@starfivetech.com> > Cc: devicetree@vger.kernel.org; dri-devel@lists.freedesktop.org; > andrzej.hajda@intel.com; neil.armstrong@linaro.org; rfoss@kernel.org; > Laurent.pinchart@ideasonboard.com; jonas@kwiboo.se; > jernej.skrabec@gmail.com; maarten.lankhorst@linux.intel.com; > mripard@kernel.org; tzimmermann@suse.de; airlied@gmail.com; > simona@ffwll.ch; robh@kernel.org; krzk+dt@kernel.org; conor+dt@kernel.org; > hjc@rock-chips.com; heiko@sntech.de; andy.yan@rock-chips.com; William Qiu > <william.qiu@starfivetech.com>; Xingyu Wu <xingyu.wu@starfivetech.com>; > kernel@esmil.dk; paul.walmsley@sifive.com; palmer@dabbelt.com; > aou@eecs.berkeley.edu; p.zabel@pengutronix.de; Changhuang Liang > <changhuang.liang@starfivetech.com>; Jack Zhu <jack.zhu@starfivetech.com>; > linux-kernel@vger.kernel.org > Subject: Re: [PATCH v5 1/9] dt-bindings: display: bindings for starfive,JH7110 > display pipeline > > On Wed, Nov 20, 2024 at 02:18:40PM +0800, keith zhao wrote: > > - Added bindings to support the display subsystem on the JH7110 SoC. > > Please do not use "This commit/patch/change" (implied) and past tense, but > imperative mood. See longer explanation here: > https://elixir.bootlin.com/linux/v5.17.1/source/Documentation/process/submitti > ng-patches.rst#L95 > > > > > - Included the DC8200 display controller and Inno HDMI controller. > > > > - Created innosilicon,inno-hdmi.yaml schema containing common properties > > for the Inno DesignWare HDMI TX controller. > > This isn't a full device tree binding specification, > > but is intended to be referenced by platform-specific bindings > > for the IP core. > > > > Signed-off-by: keith zhao <keith.zhao@starfivetech.com> > > --- > > .../display/bridge/innosilicon,inno-hdmi.yaml | 45 +++++ > > .../display/rockchip/rockchip,inno-hdmi.yaml | 27 +-- > > .../starfive/starfive,jh7110-dc8200.yaml | 176 ++++++++++++++++++ > > .../starfive/starfive,jh7110-inno-hdmi.yaml | 91 +++++++++ > > .../soc/starfive/starfive,jh7110-syscon.yaml | 1 + > > I do not see how you addressed my feedback. I asked you to split the patch. > Where is the split? Oh sorry, I had a bit of a misunderstanding here before. "Your patch is difficult to review. Split changing existing bindings (and defining common part) to a separate patch." ...... Split changing existing bindings (and defining common part) to a separate "bindings". ....... > > Your answer to my request to split was "Background is ...", yeah, so what? You > are not going to split? > Based on the misunderstanding of this idea, it led to not splitting the patch this time. Now I understand. Will split it . > > > MAINTAINERS | 8 + > > 6 files changed, 323 insertions(+), 25 deletions(-) create mode > > 100644 > > Documentation/devicetree/bindings/display/bridge/innosilicon,inno-hdmi > > .yaml create mode 100644 > > Documentation/devicetree/bindings/display/starfive/starfive,jh7110-dc8 > > 200.yaml create mode 100644 > > Documentation/devicetree/bindings/display/starfive/starfive,jh7110-inn > > o-hdmi.yaml > > > > diff --git > > a/Documentation/devicetree/bindings/display/bridge/innosilicon,inno-hd > > mi.yaml > > b/Documentation/devicetree/bindings/display/bridge/innosilicon,inno-hd > > mi.yaml > > new file mode 100644 > > index 000000000000..f2543aebc312 > > --- /dev/null > > +++ b/Documentation/devicetree/bindings/display/bridge/innosilicon,inn > > +++ o-hdmi.yaml > > @@ -0,0 +1,45 @@ > > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) %YAML 1.2 > > +--- > > +$id: > > +http://devicetree.org/schemas/display/bridge/innosilicon,inno-hdmi.ya > > +ml# > > +$schema: http://devicetree.org/meta-schemas/core.yaml# > > + > > +title: Common Properties for Innosilicon HDMI TX IP > > + > > +maintainers: > > + - keith zhao <keith.zhao@starfivetech.com> > > + > > +description: | > > Do not need '|' unless you need to preserve formatting. > I will remove the '|' since it's not necessary for preserving formatting. And here is the reason why add '|' before. https://elixir.bootlin.com/linux/v6.12/source/Documentation/devicetree/bindings/display/bridge/synopsys,dw-hdmi.yaml#L12 > > > > + Innosilicon HDMI TX is an HDMI transmission device integrated into the > zap SoC. > > Keep and extend. Ok > > > + This document specifies the device tree properties for the INNO HDMI IP > core. > > Keep... but what is INNO HDMI IP core? Another name? Different block? > > > > + It is intended to be referenced by platform-specific device tree > > + bindings, which will determine the necessity of each property. > > Not much improved here. Last two sentences are almost useless. Again - > document the hardware. Drop this sentence. > > I already complained about this. I already complained that you keep ignoring my > comments and this does not improve much. > > Since you did not implement my main feedback I am going to NAK the rest > except one more thing: I appreciate your patience, I will not refer to the existing YAML files in the main branch for writing anymore; I will only follow your suggestions https://elixir.bootlin.com/linux/v6.12/source/Documentation/devicetree/bindings/display/bridge/synopsys,dw-hdmi.yaml#L18 > > ... > > > + > > +properties: > > + compatible: > > + const: "starfive,jh7110-inno-hdmi" > > I could not be more specific in my previous feedback. Improvements? No. > Stop wasting our time. > This modification was overlooked when merging, sorry again. Thank you for your feedback. > NAK. > > Best regards, > Krzysztof
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..f2543aebc312 --- /dev/null +++ b/Documentation/devicetree/bindings/display/bridge/innosilicon,inno-hdmi.yaml @@ -0,0 +1,45 @@ +# 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 + +maintainers: + - keith zhao <keith.zhao@starfivetech.com> + +description: | + Innosilicon HDMI TX is an HDMI transmission device integrated into the zap SoC. + This document specifies the device tree properties for the INNO HDMI IP core. + It is intended to be referenced by platform-specific device tree bindings, + which will determine the necessity of each property. + +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 + +additionalProperties: true + +... diff --git a/Documentation/devicetree/bindings/display/rockchip/rockchip,inno-hdmi.yaml b/Documentation/devicetree/bindings/display/rockchip/rockchip,inno-hdmi.yaml index 5b87b0f1963e..589dedfcbee0 100644 --- a/Documentation/devicetree/bindings/display/rockchip/rockchip,inno-hdmi.yaml +++ b/Documentation/devicetree/bindings/display/rockchip/rockchip,inno-hdmi.yaml @@ -16,12 +16,6 @@ properties: - rockchip,rk3036-inno-hdmi - rockchip,rk3128-inno-hdmi - reg: - maxItems: 1 - - interrupts: - maxItems: 1 - clocks: minItems: 1 items: @@ -40,24 +34,6 @@ properties: "#sound-dai-cells": const: 0 - 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 vop 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 - required: - compatible - reg @@ -69,6 +45,7 @@ required: - ports allOf: + - $ref: ../bridge/innosilicon,inno-hdmi.yaml# - $ref: /schemas/sound/dai-common.yaml# - if: properties: @@ -95,7 +72,7 @@ allOf: required: - power-domains -additionalProperties: false +unevaluatedProperties: false examples: - | 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..5f57c405e857 --- /dev/null +++ b/Documentation/devicetree/bindings/display/starfive/starfive,jh7110-dc8200.yaml @@ -0,0 +1,176 @@ +# 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 (DC8200) + +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@starfivetech.com> + +properties: + compatible: + const: starfive,jh7110-dc8200 + + reg: + items: + - description: host interface + - description: display physical base + + 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 + + starfive,syscon: + $ref: /schemas/types.yaml#/definitions/phandle + description: Phandle to the general register files syscon + + 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@29400000 { + 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>, + <&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"; + + starfive,syscon = <&vout_syscon>; + + ports { + #address-cells = <1>; + #size-cells = <0>; + + dc_out0: port@0 { + reg = <0>; + #address-cells = <1>; + #size-cells = <0>; + + dc_out_dpi0: endpoint@0 { + reg = <0>; + remote-endpoint = <&hdmi_enc>; + }; + + }; + + dc_out1: port@1 { + reg = <1>; + #address-cells = <1>; + #size-cells = <0>; + + dc_out_dpi1: endpoint@1 { + reg = <1>; + remote-endpoint = <&dsi_enc>; + }; + + }; + }; + }; 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..62cb62885d71 --- /dev/null +++ b/Documentation/devicetree/bindings/display/starfive/starfive,jh7110-inno-hdmi.yaml @@ -0,0 +1,91 @@ +# 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@starfivetech.com> + +description: | + Innosilicon HDMI TX IP is designed for transmitting video and audio data + from a video source device to a display device. + It contains a digital controller and a physical layer. + +allOf: + - $ref: ../bridge/innosilicon,inno-hdmi.yaml# + +properties: + compatible: + const: "starfive,jh7110-inno-hdmi" + + clocks: + items: + - description: System clock of HDMI module. + - description: Mclk clock of HDMI audio. + - description: Bclk clock of HDMI audio. + + clock-names: + items: + - const: sysclk + - const: mclk + - const: bclk + + resets: + maxItems: 1 + +required: + - compatible + - reg + - interrupts + - clocks + - clock-names + - resets + - ports + +unevaluatedProperties: false + +examples: + - | + hdmi: hdmi@29590000 { + compatible = "starfive,jh7110-inno-hdmi"; + reg = <0x29590000 0x4000>; + interrupts = <99>; + clocks = <&voutcrg 17>, <&voutcrg 15>, <&voutcrg 16>; + clock-names = "sysclk", "mclk","bclk"; + resets = <&voutcrg 9>; + + ports { + #address-cells = <1>; + #size-cells = <0>; + + hdmi_in: port@0 { + reg = <0>; + hdmi_in_vop: endpoint { + remote-endpoint = <&dc_out_dpi0>; + }; + }; + + hdmi_out: port@1 { + reg = <1>; + hdmi_out_con: endpoint { + remote-endpoint = <&hdmi_con_in>; + }; + }; + }; + }; + + hdmi-out { + compatible = "hdmi-connector"; + type = "a"; + + port { + hdmi_con_in: endpoint { + remote-endpoint = <&hdmi_out_con>; + }; + }; + }; + +... diff --git a/Documentation/devicetree/bindings/soc/starfive/starfive,jh7110-syscon.yaml b/Documentation/devicetree/bindings/soc/starfive/starfive,jh7110-syscon.yaml index 0039319e91fe..cf9b657d0e8a 100644 --- a/Documentation/devicetree/bindings/soc/starfive/starfive,jh7110-syscon.yaml +++ b/Documentation/devicetree/bindings/soc/starfive/starfive,jh7110-syscon.yaml @@ -24,6 +24,7 @@ properties: - enum: - starfive,jh7110-aon-syscon - starfive,jh7110-stg-syscon + - starfive,jh7110-vout-syscon - const: syscon reg: diff --git a/MAINTAINERS b/MAINTAINERS index 84086d47db69..f787dd625497 100644 --- a/MAINTAINERS +++ b/MAINTAINERS @@ -7432,6 +7432,14 @@ T: git https://gitlab.freedesktop.org/drm/misc/kernel.git F: Documentation/devicetree/bindings/display/ste,mcde.yaml F: drivers/gpu/drm/mcde/ +DRM DRIVERS FOR STARFIVE +M: Keith Zhao <keith.zhao@starfivetech.com> +L: dri-devel@lists.freedesktop.org +S: Maintained +T: git https://gitlab.freedesktop.org/drm/misc/kernel.git +F: Documentation/devicetree/bindings/display/bridge/innosilicon,inno-hdmi.yaml +F: Documentation/devicetree/bindings/display/starfive/ + DRM DRIVER FOR SYNAPTICS R63353 PANELS M: Michael Trimarchi <michael@amarulasolutions.com> S: Maintained
- Added bindings to support the display subsystem on the JH7110 SoC. - Included the DC8200 display controller and Inno HDMI controller. - Created innosilicon,inno-hdmi.yaml schema containing common properties for the Inno DesignWare HDMI TX controller. This isn't a full device tree binding specification, but is intended to be referenced by platform-specific bindings for the IP core. Signed-off-by: keith zhao <keith.zhao@starfivetech.com> --- .../display/bridge/innosilicon,inno-hdmi.yaml | 45 +++++ .../display/rockchip/rockchip,inno-hdmi.yaml | 27 +-- .../starfive/starfive,jh7110-dc8200.yaml | 176 ++++++++++++++++++ .../starfive/starfive,jh7110-inno-hdmi.yaml | 91 +++++++++ .../soc/starfive/starfive,jh7110-syscon.yaml | 1 + MAINTAINERS | 8 + 6 files changed, 323 insertions(+), 25 deletions(-) create mode 100644 Documentation/devicetree/bindings/display/bridge/innosilicon,inno-hdmi.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