| Message ID | 1602205443-9036-2-git-send-email-anitha.chrisanthus@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Add support for KeemBay DRM driver | expand |
Hi, On 09/10/2020 03:03, Anitha Chrisanthus wrote: > This patch adds bindings for Intel KeemBay Display > > v2: review changes from Rob Herring > > Signed-off-by: Anitha Chrisanthus <anitha.chrisanthus@intel.com> > --- > .../bindings/display/intel,keembay-display.yaml | 99 ++++++++++++++++++++++ > 1 file changed, 99 insertions(+) > create mode 100644 Documentation/devicetree/bindings/display/intel,keembay-display.yaml > > diff --git a/Documentation/devicetree/bindings/display/intel,keembay-display.yaml b/Documentation/devicetree/bindings/display/intel,keembay-display.yaml > new file mode 100644 > index 0000000..a38493d > --- /dev/null > +++ b/Documentation/devicetree/bindings/display/intel,keembay-display.yaml > @@ -0,0 +1,99 @@ > +# SPDX-License-Identifier: GPL-2.0-only > +%YAML 1.2 > +--- > +$id: http://devicetree.org/schemas/display/intel,keembay-display.yaml# > +$schema: http://devicetree.org/meta-schemas/core.yaml# > + > +title: Devicetree bindings for Intel Keem Bay display controller > + > +maintainers: > + - Anitha Chrisanthus <anitha.chrisanthus@intel.com> > + - Edmond J Dea <edmund.j.dea@intel.com> > + > +properties: > + compatible: > + const: intel,kmb_display > + > + reg: > + items: > + - description: Lcd registers range > + - description: Mipi registers range Looking at the registers, the MIPI transceiver seems to be a separate IP, same for D-PHY which should have a proper PHY driver instead of beeing handled here. > + - description: Msscam registers range MSScam here seems to be a clock and reset controller for the LCD and MIPI IPs, thus should be handler out of DRM. > + > + reg-names: > + items: > + - const: lcd > + - const: mipi > + - const: msscam > + > + clocks: > + items: > + - description: LCD controller clock > + - description: Mipi DSI clock > + - description: Mipi DSI econfig clock > + - description: Mipi DSI config clock > + - description: System clock or pll0 clock > + > + clock-names: > + items: > + - const: clk_lcd > + - const: clk_mipi > + - const: clk_mipi_ecfg > + - const: clk_mipi_cfg > + - const: clk_pll0 > + > + interrupts: > + maxItems: 1 > + > + encoder-slave: > + description: bridge node entry for mipi to hdmi converter > + > + port: > + type: object > + description: > > + Port node with one endpoint connected to mipi to hdmi converter node. > + > +required: > + - compatible > + - reg > + - reg-names > + - clocks > + - clock-names > + - interrupts > + - encoder-slave > + - port > + > +additionalProperties: false > + > +examples: > + - | > + #include <dt-bindings/interrupt-controller/irq.h> > + #include <dt-bindings/interrupt-controller/arm-gic.h> > + #define MOVISOC_KMB_MSS_AUX_LCD > + #define MOVISOC_KMB_MSS_AUX_MIPI_TX0 > + #define MOVISOC_KMB_MSS_AUX_MIPI_ECFG > + #define MOVISOC_KMB_MSS_AUX_MIPI_CFG > + #define MOVISOC_KMB_A53_PLL_0_OUT_0 > + display@20900000 { > + compatible = "intel,keembay-display"; > + reg = <0x20930000 0x3000>, > + <0x20900000 0x4000>, > + <0x20910000 0x30>; > + reg-names = "lcd", "mipi", "msscam"; > + interrupts = <GIC_SPI 33 IRQ_TYPE_LEVEL_HIGH>; > + clocks = <&scmi_clk MOVISOC_KMB_MSS_AUX_LCD>, > + <&scmi_clk MOVISOC_KMB_MSS_AUX_MIPI_TX0>, > + <&scmi_clk MOVISOC_KMB_MSS_AUX_MIPI_ECFG>, > + <&scmi_clk MOVISOC_KMB_MSS_AUX_MIPI_CFG>, > + <&scmi_clk MOVISOC_KMB_A53_PLL_0_OUT_0>; > + clock-names = "clk_lcd", "clk_mipi", "clk_mipi_ecfg", > + "clk_mipi_cfg", "clk_pll0"; > + > + encoder-slave = <&adv7535>; > + > + port { > + dsi_output: endpoint { > + remote-endpoint = <&adv7535_input>; > + }; > + }; > + }; > Anitha, Daniel, this keembay driver should be architectured like other ARM-like display controllers, with separate drivers for MIPI DSI bridge and msscam clock & reset controller. Neil
> -----Original Message----- > From: Neil Armstrong <narmstrong@baylibre.com> > Sent: Friday, October 9, 2020 2:10 AM > To: Chrisanthus, Anitha <anitha.chrisanthus@intel.com>; dri- > devel@lists.freedesktop.org; devicetree@vger.kernel.org; Vetter, Daniel > <daniel.vetter@intel.com> > Cc: Dea, Edmund J <edmund.j.dea@intel.com>; sam@ravnborg.org > Subject: Re: [PATCH v9 1/5] dt-bindings: display: Add support for Intel > KeemBay Display > > Hi, > > On 09/10/2020 03:03, Anitha Chrisanthus wrote: > > This patch adds bindings for Intel KeemBay Display > > > > v2: review changes from Rob Herring > > > > Signed-off-by: Anitha Chrisanthus <anitha.chrisanthus@intel.com> > > --- > > .../bindings/display/intel,keembay-display.yaml | 99 > ++++++++++++++++++++++ > > 1 file changed, 99 insertions(+) > > create mode 100644 > Documentation/devicetree/bindings/display/intel,keembay-display.yaml > > > > diff --git a/Documentation/devicetree/bindings/display/intel,keembay- > display.yaml b/Documentation/devicetree/bindings/display/intel,keembay- > display.yaml > > new file mode 100644 > > index 0000000..a38493d > > --- /dev/null > > +++ b/Documentation/devicetree/bindings/display/intel,keembay- > display.yaml > > @@ -0,0 +1,99 @@ > > +# SPDX-License-Identifier: GPL-2.0-only > > +%YAML 1.2 > > +--- > > +$id: http://devicetree.org/schemas/display/intel,keembay- > display.yaml# > > +$schema: http://devicetree.org/meta-schemas/core.yaml# > > + > > +title: Devicetree bindings for Intel Keem Bay display controller > > + > > +maintainers: > > + - Anitha Chrisanthus <anitha.chrisanthus@intel.com> > > + - Edmond J Dea <edmund.j.dea@intel.com> > > + > > +properties: > > + compatible: > > + const: intel,kmb_display > > + > > + reg: > > + items: > > + - description: Lcd registers range > > + - description: Mipi registers range > > Looking at the registers, the MIPI transceiver seems to be a separate IP, > same for D-PHY which should have a proper PHY driver instead of beeing > handled > here. Mipi is not a separate IP, it is all part of one sub system in the Intel Movidius Soc. > > > + - description: Msscam registers range > > MSScam here seems to be a clock and reset controller for the LCD and MIPI > IPs, > thus should be handler out of DRM. > > > + > > + reg-names: > > + items: > > + - const: lcd > > + - const: mipi > > + - const: msscam > > + > > + clocks: > > + items: > > + - description: LCD controller clock > > + - description: Mipi DSI clock > > + - description: Mipi DSI econfig clock > > + - description: Mipi DSI config clock > > + - description: System clock or pll0 clock > > + > > + clock-names: > > + items: > > + - const: clk_lcd > > + - const: clk_mipi > > + - const: clk_mipi_ecfg > > + - const: clk_mipi_cfg > > + - const: clk_pll0 > > + > > + interrupts: > > + maxItems: 1 > > + > > + encoder-slave: > > + description: bridge node entry for mipi to hdmi converter > > + > > + port: > > + type: object > > + description: > > > + Port node with one endpoint connected to mipi to hdmi converter > node. > > + > > +required: > > + - compatible > > + - reg > > + - reg-names > > + - clocks > > + - clock-names > > + - interrupts > > + - encoder-slave > > + - port > > + > > +additionalProperties: false > > + > > +examples: > > + - | > > + #include <dt-bindings/interrupt-controller/irq.h> > > + #include <dt-bindings/interrupt-controller/arm-gic.h> > > + #define MOVISOC_KMB_MSS_AUX_LCD > > + #define MOVISOC_KMB_MSS_AUX_MIPI_TX0 > > + #define MOVISOC_KMB_MSS_AUX_MIPI_ECFG > > + #define MOVISOC_KMB_MSS_AUX_MIPI_CFG > > + #define MOVISOC_KMB_A53_PLL_0_OUT_0 > > + display@20900000 { > > + compatible = "intel,keembay-display"; > > + reg = <0x20930000 0x3000>, > > + <0x20900000 0x4000>, > > + <0x20910000 0x30>; > > + reg-names = "lcd", "mipi", "msscam"; > > + interrupts = <GIC_SPI 33 IRQ_TYPE_LEVEL_HIGH>; > > + clocks = <&scmi_clk MOVISOC_KMB_MSS_AUX_LCD>, > > + <&scmi_clk MOVISOC_KMB_MSS_AUX_MIPI_TX0>, > > + <&scmi_clk MOVISOC_KMB_MSS_AUX_MIPI_ECFG>, > > + <&scmi_clk MOVISOC_KMB_MSS_AUX_MIPI_CFG>, > > + <&scmi_clk MOVISOC_KMB_A53_PLL_0_OUT_0>; > > + clock-names = "clk_lcd", "clk_mipi", "clk_mipi_ecfg", > > + "clk_mipi_cfg", "clk_pll0"; > > + > > + encoder-slave = <&adv7535>; > > + > > + port { > > + dsi_output: endpoint { > > + remote-endpoint = <&adv7535_input>; > > + }; > > + }; > > + }; > > > > Anitha, Daniel, this keembay driver should be architectured like other ARM- > like display > controllers, with separate drivers for MIPI DSI bridge and msscam clock & > reset controller. > Neil
On Fri, Oct 9, 2020 at 11:09 AM Neil Armstrong <narmstrong@baylibre.com> wrote: > > Hi, > > On 09/10/2020 03:03, Anitha Chrisanthus wrote: > > This patch adds bindings for Intel KeemBay Display > > > > v2: review changes from Rob Herring > > > > Signed-off-by: Anitha Chrisanthus <anitha.chrisanthus@intel.com> > > --- > > .../bindings/display/intel,keembay-display.yaml | 99 ++++++++++++++++++++++ > > 1 file changed, 99 insertions(+) > > create mode 100644 Documentation/devicetree/bindings/display/intel,keembay-display.yaml > > > > diff --git a/Documentation/devicetree/bindings/display/intel,keembay-display.yaml b/Documentation/devicetree/bindings/display/intel,keembay-display.yaml > > new file mode 100644 > > index 0000000..a38493d > > --- /dev/null > > +++ b/Documentation/devicetree/bindings/display/intel,keembay-display.yaml > > @@ -0,0 +1,99 @@ > > +# SPDX-License-Identifier: GPL-2.0-only > > +%YAML 1.2 > > +--- > > +$id: http://devicetree.org/schemas/display/intel,keembay-display.yaml# > > +$schema: http://devicetree.org/meta-schemas/core.yaml# > > + > > +title: Devicetree bindings for Intel Keem Bay display controller > > + > > +maintainers: > > + - Anitha Chrisanthus <anitha.chrisanthus@intel.com> > > + - Edmond J Dea <edmund.j.dea@intel.com> > > + > > +properties: > > + compatible: > > + const: intel,kmb_display > > + > > + reg: > > + items: > > + - description: Lcd registers range > > + - description: Mipi registers range > > Looking at the registers, the MIPI transceiver seems to be a separate IP, > same for D-PHY which should have a proper PHY driver instead of beeing handled > here. > > > + - description: Msscam registers range > > MSScam here seems to be a clock and reset controller for the LCD and MIPI IPs, > thus should be handler out of DRM. > > > + > > + reg-names: > > + items: > > + - const: lcd > > + - const: mipi > > + - const: msscam > > + > > + clocks: > > + items: > > + - description: LCD controller clock > > + - description: Mipi DSI clock > > + - description: Mipi DSI econfig clock > > + - description: Mipi DSI config clock > > + - description: System clock or pll0 clock > > + > > + clock-names: > > + items: > > + - const: clk_lcd > > + - const: clk_mipi > > + - const: clk_mipi_ecfg > > + - const: clk_mipi_cfg > > + - const: clk_pll0 > > + > > + interrupts: > > + maxItems: 1 > > + > > + encoder-slave: > > + description: bridge node entry for mipi to hdmi converter > > + > > + port: > > + type: object > > + description: > > > + Port node with one endpoint connected to mipi to hdmi converter node. > > + > > +required: > > + - compatible > > + - reg > > + - reg-names > > + - clocks > > + - clock-names > > + - interrupts > > + - encoder-slave > > + - port > > + > > +additionalProperties: false > > + > > +examples: > > + - | > > + #include <dt-bindings/interrupt-controller/irq.h> > > + #include <dt-bindings/interrupt-controller/arm-gic.h> > > + #define MOVISOC_KMB_MSS_AUX_LCD > > + #define MOVISOC_KMB_MSS_AUX_MIPI_TX0 > > + #define MOVISOC_KMB_MSS_AUX_MIPI_ECFG > > + #define MOVISOC_KMB_MSS_AUX_MIPI_CFG > > + #define MOVISOC_KMB_A53_PLL_0_OUT_0 > > + display@20900000 { > > + compatible = "intel,keembay-display"; > > + reg = <0x20930000 0x3000>, > > + <0x20900000 0x4000>, > > + <0x20910000 0x30>; > > + reg-names = "lcd", "mipi", "msscam"; > > + interrupts = <GIC_SPI 33 IRQ_TYPE_LEVEL_HIGH>; > > + clocks = <&scmi_clk MOVISOC_KMB_MSS_AUX_LCD>, > > + <&scmi_clk MOVISOC_KMB_MSS_AUX_MIPI_TX0>, > > + <&scmi_clk MOVISOC_KMB_MSS_AUX_MIPI_ECFG>, > > + <&scmi_clk MOVISOC_KMB_MSS_AUX_MIPI_CFG>, > > + <&scmi_clk MOVISOC_KMB_A53_PLL_0_OUT_0>; > > + clock-names = "clk_lcd", "clk_mipi", "clk_mipi_ecfg", > > + "clk_mipi_cfg", "clk_pll0"; > > + > > + encoder-slave = <&adv7535>; > > + > > + port { > > + dsi_output: endpoint { > > + remote-endpoint = <&adv7535_input>; > > + }; > > + }; > > + }; > > > > Anitha, Daniel, this keembay driver should be architectured like other ARM-like display > controllers, with separate drivers for MIPI DSI bridge and msscam clock & reset controller. tbh I have no clue about dt, and very little clue about how to do armsoc drivers properly. My only take in this entire area is that drm/bridges looks confusing with too many different approaches, and people should stuck more together and figure out what to do (the entire component.c vs not so much component.c discussion, and another one is about creating drm-connector or not doing that). But how this all should be done I really have not much useful clue, so please keep me out of that. -Daniel
Hi Neil/Anitha. On Fri, Oct 09, 2020 at 11:09:45AM +0200, Neil Armstrong wrote: > Hi, > > On 09/10/2020 03:03, Anitha Chrisanthus wrote: > > This patch adds bindings for Intel KeemBay Display > > > > v2: review changes from Rob Herring > > > > Signed-off-by: Anitha Chrisanthus <anitha.chrisanthus@intel.com> > > --- > > .../bindings/display/intel,keembay-display.yaml | 99 ++++++++++++++++++++++ > > 1 file changed, 99 insertions(+) > > create mode 100644 Documentation/devicetree/bindings/display/intel,keembay-display.yaml > > > > diff --git a/Documentation/devicetree/bindings/display/intel,keembay-display.yaml b/Documentation/devicetree/bindings/display/intel,keembay-display.yaml > > new file mode 100644 > > index 0000000..a38493d > > --- /dev/null > > +++ b/Documentation/devicetree/bindings/display/intel,keembay-display.yaml > > @@ -0,0 +1,99 @@ > > +# SPDX-License-Identifier: GPL-2.0-only (GPL-2.0-only OR BSD-2-Clause) for new bindings please. > > +%YAML 1.2 > > +--- > > +$id: http://devicetree.org/schemas/display/intel,keembay-display.yaml# > > +$schema: http://devicetree.org/meta-schemas/core.yaml# > > + > > +title: Devicetree bindings for Intel Keem Bay display controller > > + > > +maintainers: > > + - Anitha Chrisanthus <anitha.chrisanthus@intel.com> > > + - Edmond J Dea <edmund.j.dea@intel.com> > > + > > +properties: > > + compatible: > > + const: intel,kmb_display > > + > > + reg: > > + items: > > + - description: Lcd registers range > > + - description: Mipi registers range > > Looking at the registers, the MIPI transceiver seems to be a separate IP, > same for D-PHY which should have a proper PHY driver instead of beeing handled > here. Looking at the register definitiosn and the code the split in a display block and a bridge block looks reasonable. The bridge block would include the MIPI<->DSI part which includes the PHY and the Msscam parts too. The PHY is an integrated part of the MIPI<->DSI IP so really not eligeble for a dedicated node in the DT. Likewise the Msscam, whatevet that is, is integrated with the MIPI<->DSI. So all in all: - One display DT Schema - One bridge DT Schema The bridge DT Schema will then have an input port and an output port. > > > + - description: Msscam registers range > > MSScam here seems to be a clock and reset controller for the LCD and MIPI IPs, > thus should be handler out of DRM. Reading the register definitions and the code it looks very integrated so as I wrote above, no dedicated DT Schema should be needed here. > > > + > > + reg-names: > > + items: > > + - const: lcd > > + - const: mipi > > + - const: msscam > > + > > + clocks: > > + items: > > + - description: LCD controller clock > > + - description: Mipi DSI clock > > + - description: Mipi DSI econfig clock > > + - description: Mipi DSI config clock > > + - description: System clock or pll0 clock > > + > > + clock-names: > > + items: > > + - const: clk_lcd > > + - const: clk_mipi > > + - const: clk_mipi_ecfg > > + - const: clk_mipi_cfg > > + - const: clk_pll0 > > + > > + interrupts: > > + maxItems: 1 > > + > > + encoder-slave: > > + description: bridge node entry for mipi to hdmi converter This node should go, as we shall find the bridge using the ports. > > + > > + port: > > + type: object > > + description: > > > + Port node with one endpoint connected to mipi to hdmi converter node. > > + > > +required: > > + - compatible > > + - reg > > + - reg-names > > + - clocks > > + - clock-names > > + - interrupts > > + - encoder-slave > > + - port > > + > > +additionalProperties: false > > + > > +examples: > > + - | > > + #include <dt-bindings/interrupt-controller/irq.h> > > + #include <dt-bindings/interrupt-controller/arm-gic.h> > > + #define MOVISOC_KMB_MSS_AUX_LCD > > + #define MOVISOC_KMB_MSS_AUX_MIPI_TX0 > > + #define MOVISOC_KMB_MSS_AUX_MIPI_ECFG > > + #define MOVISOC_KMB_MSS_AUX_MIPI_CFG > > + #define MOVISOC_KMB_A53_PLL_0_OUT_0 > > + display@20900000 { > > + compatible = "intel,keembay-display"; > > + reg = <0x20930000 0x3000>, > > + <0x20900000 0x4000>, > > + <0x20910000 0x30>; > > + reg-names = "lcd", "mipi", "msscam"; > > + interrupts = <GIC_SPI 33 IRQ_TYPE_LEVEL_HIGH>; > > + clocks = <&scmi_clk MOVISOC_KMB_MSS_AUX_LCD>, > > + <&scmi_clk MOVISOC_KMB_MSS_AUX_MIPI_TX0>, > > + <&scmi_clk MOVISOC_KMB_MSS_AUX_MIPI_ECFG>, > > + <&scmi_clk MOVISOC_KMB_MSS_AUX_MIPI_CFG>, > > + <&scmi_clk MOVISOC_KMB_A53_PLL_0_OUT_0>; > > + clock-names = "clk_lcd", "clk_mipi", "clk_mipi_ecfg", > > + "clk_mipi_cfg", "clk_pll0"; > > + > > + encoder-slave = <&adv7535>; > > + > > + port { > > + dsi_output: endpoint { > > + remote-endpoint = <&adv7535_input>; > > + }; > > + }; > > + }; > > > > Anitha, Daniel, this keembay driver should be architectured like other ARM-like display > controllers, with separate drivers for MIPI DSI bridge and msscam clock & reset controller. OK, lets decide on the binding first, and then discuss the driver afterwards. Sam
Hi Neil, Thanks for your review, please see my reply inline. > -----Original Message----- > From: Neil Armstrong <narmstrong@baylibre.com> > Sent: Friday, October 9, 2020 2:10 AM > To: Chrisanthus, Anitha <anitha.chrisanthus@intel.com>; dri- > devel@lists.freedesktop.org; devicetree@vger.kernel.org; Vetter, Daniel > <daniel.vetter@intel.com> > Cc: Dea, Edmund J <edmund.j.dea@intel.com>; sam@ravnborg.org > Subject: Re: [PATCH v9 1/5] dt-bindings: display: Add support for Intel > KeemBay Display > > Hi, > > On 09/10/2020 03:03, Anitha Chrisanthus wrote: > > This patch adds bindings for Intel KeemBay Display > > > > v2: review changes from Rob Herring > > > > Signed-off-by: Anitha Chrisanthus <anitha.chrisanthus@intel.com> > > --- > > .../bindings/display/intel,keembay-display.yaml | 99 > ++++++++++++++++++++++ > > 1 file changed, 99 insertions(+) > > create mode 100644 > Documentation/devicetree/bindings/display/intel,keembay-display.yaml > > > > diff --git a/Documentation/devicetree/bindings/display/intel,keembay- > display.yaml b/Documentation/devicetree/bindings/display/intel,keembay- > display.yaml > > new file mode 100644 > > index 0000000..a38493d > > --- /dev/null > > +++ b/Documentation/devicetree/bindings/display/intel,keembay- > display.yaml > > @@ -0,0 +1,99 @@ > > +# SPDX-License-Identifier: GPL-2.0-only > > +%YAML 1.2 > > +--- > > +$id: http://devicetree.org/schemas/display/intel,keembay- > display.yaml# > > +$schema: http://devicetree.org/meta-schemas/core.yaml# > > + > > +title: Devicetree bindings for Intel Keem Bay display controller > > + > > +maintainers: > > + - Anitha Chrisanthus <anitha.chrisanthus@intel.com> > > + - Edmond J Dea <edmund.j.dea@intel.com> > > + > > +properties: > > + compatible: > > + const: intel,kmb_display > > + > > + reg: > > + items: > > + - description: Lcd registers range > > + - description: Mipi registers range > > Looking at the registers, the MIPI transceiver seems to be a separate IP, > same for D-PHY which should have a proper PHY driver instead of beeing > handled > here. > The LCD, MIPI DSI, DPHY and MSSCAM as a group, are considered the display subsystem for Keem Bay. As such, there are several interdependencies that make splitting them up next to impossible and currently we do not have the resources available for that effort. > > + - description: Msscam registers range > > MSScam here seems to be a clock and reset controller for the LCD and MIPI > IPs, > thus should be handler out of DRM. > > > + > > + reg-names: > > + items: > > + - const: lcd > > + - const: mipi > > + - const: msscam > > + > > + clocks: > > + items: > > + - description: LCD controller clock > > + - description: Mipi DSI clock > > + - description: Mipi DSI econfig clock > > + - description: Mipi DSI config clock > > + - description: System clock or pll0 clock > > + > > + clock-names: > > + items: > > + - const: clk_lcd > > + - const: clk_mipi > > + - const: clk_mipi_ecfg > > + - const: clk_mipi_cfg > > + - const: clk_pll0 > > + > > + interrupts: > > + maxItems: 1 > > + > > + encoder-slave: > > + description: bridge node entry for mipi to hdmi converter > > + > > + port: > > + type: object > > + description: > > > + Port node with one endpoint connected to mipi to hdmi converter > node. > > + > > +required: > > + - compatible > > + - reg > > + - reg-names > > + - clocks > > + - clock-names > > + - interrupts > > + - encoder-slave > > + - port > > + > > +additionalProperties: false > > + > > +examples: > > + - | > > + #include <dt-bindings/interrupt-controller/irq.h> > > + #include <dt-bindings/interrupt-controller/arm-gic.h> > > + #define MOVISOC_KMB_MSS_AUX_LCD > > + #define MOVISOC_KMB_MSS_AUX_MIPI_TX0 > > + #define MOVISOC_KMB_MSS_AUX_MIPI_ECFG > > + #define MOVISOC_KMB_MSS_AUX_MIPI_CFG > > + #define MOVISOC_KMB_A53_PLL_0_OUT_0 > > + display@20900000 { > > + compatible = "intel,keembay-display"; > > + reg = <0x20930000 0x3000>, > > + <0x20900000 0x4000>, > > + <0x20910000 0x30>; > > + reg-names = "lcd", "mipi", "msscam"; > > + interrupts = <GIC_SPI 33 IRQ_TYPE_LEVEL_HIGH>; > > + clocks = <&scmi_clk MOVISOC_KMB_MSS_AUX_LCD>, > > + <&scmi_clk MOVISOC_KMB_MSS_AUX_MIPI_TX0>, > > + <&scmi_clk MOVISOC_KMB_MSS_AUX_MIPI_ECFG>, > > + <&scmi_clk MOVISOC_KMB_MSS_AUX_MIPI_CFG>, > > + <&scmi_clk MOVISOC_KMB_A53_PLL_0_OUT_0>; > > + clock-names = "clk_lcd", "clk_mipi", "clk_mipi_ecfg", > > + "clk_mipi_cfg", "clk_pll0"; > > + > > + encoder-slave = <&adv7535>; > > + > > + port { > > + dsi_output: endpoint { > > + remote-endpoint = <&adv7535_input>; > > + }; > > + }; > > + }; > > > > Anitha, Daniel, this keembay driver should be architectured like other ARM- > like display > controllers, with separate drivers for MIPI DSI bridge and msscam clock & > reset controller. > This driver was developed as part of the Keem Bay BSP targeting the LTS 5.4 Yocto kernel. It is part of our current production BSP release after extensive testing. At this time there are no plans to incorporate the display IP used in Keem Bay in any future products. Our goal is to get this driver into the mainline kernel so that we don't have to continuously rebase it as newer kernels are released. As mentioned above, we don't have the resources to re-architect and then re-develop a display driver for this product and see very little benefit in doing so. If we were expecting these IP blocks to be re-used in the future, creating individual drivers for each would make sense. Thanks again for taking the time to review the driver. Anitha > Neil
On Tue, Oct 13, 2020 at 12:24:38AM +0000, Chrisanthus, Anitha wrote: > Hi Neil, > > Thanks for your review, please see my reply inline. > > > -----Original Message----- > > From: Neil Armstrong <narmstrong@baylibre.com> > > Sent: Friday, October 9, 2020 2:10 AM > > To: Chrisanthus, Anitha <anitha.chrisanthus@intel.com>; dri- > > devel@lists.freedesktop.org; devicetree@vger.kernel.org; Vetter, Daniel > > <daniel.vetter@intel.com> > > Cc: Dea, Edmund J <edmund.j.dea@intel.com>; sam@ravnborg.org > > Subject: Re: [PATCH v9 1/5] dt-bindings: display: Add support for Intel > > KeemBay Display > > > > Hi, > > > > On 09/10/2020 03:03, Anitha Chrisanthus wrote: > > > This patch adds bindings for Intel KeemBay Display > > > > > > v2: review changes from Rob Herring > > > > > > Signed-off-by: Anitha Chrisanthus <anitha.chrisanthus@intel.com> > > > --- > > > .../bindings/display/intel,keembay-display.yaml | 99 > > ++++++++++++++++++++++ > > > 1 file changed, 99 insertions(+) > > > create mode 100644 > > Documentation/devicetree/bindings/display/intel,keembay-display.yaml > > > > > > diff --git a/Documentation/devicetree/bindings/display/intel,keembay- > > display.yaml b/Documentation/devicetree/bindings/display/intel,keembay- > > display.yaml > > > new file mode 100644 > > > index 0000000..a38493d > > > --- /dev/null > > > +++ b/Documentation/devicetree/bindings/display/intel,keembay- > > display.yaml > > > @@ -0,0 +1,99 @@ > > > +# SPDX-License-Identifier: GPL-2.0-only > > > +%YAML 1.2 > > > +--- > > > +$id: http://devicetree.org/schemas/display/intel,keembay- > > display.yaml# > > > +$schema: http://devicetree.org/meta-schemas/core.yaml# > > > + > > > +title: Devicetree bindings for Intel Keem Bay display controller > > > + > > > +maintainers: > > > + - Anitha Chrisanthus <anitha.chrisanthus@intel.com> > > > + - Edmond J Dea <edmund.j.dea@intel.com> > > > + > > > +properties: > > > + compatible: > > > + const: intel,kmb_display > > > + > > > + reg: > > > + items: > > > + - description: Lcd registers range > > > + - description: Mipi registers range > > > > Looking at the registers, the MIPI transceiver seems to be a separate IP, > > same for D-PHY which should have a proper PHY driver instead of beeing > > handled > > here. > > > The LCD, MIPI DSI, DPHY and MSSCAM as a group, are considered the > display subsystem for Keem Bay. As such, there are several > interdependencies that make splitting them up next to impossible and Please detail what those inter-dependencies are. It's doubtful that you have anything we have not had to deal with in other SoCs. > currently we do not have the resources available for that effort. That is certainly not justification for accepting this. Rob
> -----Original Message----- > From: dri-devel <dri-devel-bounces@lists.freedesktop.org> On Behalf Of Rob > Herring > Sent: Tuesday, October 13, 2020 8:43 AM > To: Chrisanthus, Anitha <anitha.chrisanthus@intel.com> > Cc: devicetree@vger.kernel.org; Neil Armstrong <narmstrong@baylibre.com>; > Dea, Edmund J <edmund.j.dea@intel.com>; dri-devel@lists.freedesktop.org; > Vetter, Daniel <daniel.vetter@intel.com>; sam@ravnborg.org > Subject: Re: [PATCH v9 1/5] dt-bindings: display: Add support for Intel KeemBay > Display > > On Tue, Oct 13, 2020 at 12:24:38AM +0000, Chrisanthus, Anitha wrote: > > Hi Neil, > > > > Thanks for your review, please see my reply inline. > > > > > -----Original Message----- > > > From: Neil Armstrong <narmstrong@baylibre.com> > > > Sent: Friday, October 9, 2020 2:10 AM > > > To: Chrisanthus, Anitha <anitha.chrisanthus@intel.com>; dri- > > > devel@lists.freedesktop.org; devicetree@vger.kernel.org; Vetter, Daniel > > > <daniel.vetter@intel.com> > > > Cc: Dea, Edmund J <edmund.j.dea@intel.com>; sam@ravnborg.org > > > Subject: Re: [PATCH v9 1/5] dt-bindings: display: Add support for Intel > > > KeemBay Display > > > > > > Hi, > > > > > > On 09/10/2020 03:03, Anitha Chrisanthus wrote: > > > > This patch adds bindings for Intel KeemBay Display > > > > > > > > v2: review changes from Rob Herring > > > > > > > > Signed-off-by: Anitha Chrisanthus <anitha.chrisanthus@intel.com> > > > > --- > > > > .../bindings/display/intel,keembay-display.yaml | 99 > > > ++++++++++++++++++++++ > > > > 1 file changed, 99 insertions(+) > > > > create mode 100644 > > > Documentation/devicetree/bindings/display/intel,keembay-display.yaml > > > > > > > > diff --git a/Documentation/devicetree/bindings/display/intel,keembay- > > > display.yaml b/Documentation/devicetree/bindings/display/intel,keembay- > > > display.yaml > > > > new file mode 100644 > > > > index 0000000..a38493d > > > > --- /dev/null > > > > +++ b/Documentation/devicetree/bindings/display/intel,keembay- > > > display.yaml > > > > @@ -0,0 +1,99 @@ > > > > +# SPDX-License-Identifier: GPL-2.0-only > > > > +%YAML 1.2 > > > > +--- > > > > +$id: http://devicetree.org/schemas/display/intel,keembay- > > > display.yaml# > > > > +$schema: http://devicetree.org/meta-schemas/core.yaml# > > > > + > > > > +title: Devicetree bindings for Intel Keem Bay display controller > > > > + > > > > +maintainers: > > > > + - Anitha Chrisanthus <anitha.chrisanthus@intel.com> > > > > + - Edmond J Dea <edmund.j.dea@intel.com> > > > > + > > > > +properties: > > > > + compatible: > > > > + const: intel,kmb_display > > > > + > > > > + reg: > > > > + items: > > > > + - description: Lcd registers range > > > > + - description: Mipi registers range > > > > > > Looking at the registers, the MIPI transceiver seems to be a separate IP, > > > same for D-PHY which should have a proper PHY driver instead of beeing > > > handled > > > here. > > > > > The LCD, MIPI DSI, DPHY and MSSCAM as a group, are considered the > > display subsystem for Keem Bay. As such, there are several > > interdependencies that make splitting them up next to impossible and > > Please detail what those inter-dependencies are. It's doubtful that you > have anything we have not had to deal with in other SoCs. > > > currently we do not have the resources available for that effort. > > That is certainly not justification for accepting this. > > Rob Hi Rob, the wording was probably a bit exaggerated and you're right in that there it's not unique from a hardware perspective. The problem we have (and I know, it's our problem, not yours) is that our program required us to develop this internally first and then try to upstream it. So now that we've put a large effort into developing and testing the driver, it's very difficult for us to justify the resources to re-design it to better match the design of other SOC display drivers. We did review other SOC display drivers before creating this and thought that we were following the best practices for the design. I fully agree that lack of resources is not justification for not fixing something broken. But on the flip side, neither is changing the design because it could be "better" justification for not accepting it. If there is something wrong with the driver and it will cause problems in the future, then please, let us know. That would provide the data needed to justify additional effort. Bob > _______________________________________________ > dri-devel mailing list > dri-devel@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/dri-devel
diff --git a/Documentation/devicetree/bindings/display/intel,keembay-display.yaml b/Documentation/devicetree/bindings/display/intel,keembay-display.yaml new file mode 100644 index 0000000..a38493d --- /dev/null +++ b/Documentation/devicetree/bindings/display/intel,keembay-display.yaml @@ -0,0 +1,99 @@ +# SPDX-License-Identifier: GPL-2.0-only +%YAML 1.2 +--- +$id: http://devicetree.org/schemas/display/intel,keembay-display.yaml# +$schema: http://devicetree.org/meta-schemas/core.yaml# + +title: Devicetree bindings for Intel Keem Bay display controller + +maintainers: + - Anitha Chrisanthus <anitha.chrisanthus@intel.com> + - Edmond J Dea <edmund.j.dea@intel.com> + +properties: + compatible: + const: intel,kmb_display + + reg: + items: + - description: Lcd registers range + - description: Mipi registers range + - description: Msscam registers range + + reg-names: + items: + - const: lcd + - const: mipi + - const: msscam + + clocks: + items: + - description: LCD controller clock + - description: Mipi DSI clock + - description: Mipi DSI econfig clock + - description: Mipi DSI config clock + - description: System clock or pll0 clock + + clock-names: + items: + - const: clk_lcd + - const: clk_mipi + - const: clk_mipi_ecfg + - const: clk_mipi_cfg + - const: clk_pll0 + + interrupts: + maxItems: 1 + + encoder-slave: + description: bridge node entry for mipi to hdmi converter + + port: + type: object + description: > + Port node with one endpoint connected to mipi to hdmi converter node. + +required: + - compatible + - reg + - reg-names + - clocks + - clock-names + - interrupts + - encoder-slave + - port + +additionalProperties: false + +examples: + - | + #include <dt-bindings/interrupt-controller/irq.h> + #include <dt-bindings/interrupt-controller/arm-gic.h> + #define MOVISOC_KMB_MSS_AUX_LCD + #define MOVISOC_KMB_MSS_AUX_MIPI_TX0 + #define MOVISOC_KMB_MSS_AUX_MIPI_ECFG + #define MOVISOC_KMB_MSS_AUX_MIPI_CFG + #define MOVISOC_KMB_A53_PLL_0_OUT_0 + display@20900000 { + compatible = "intel,keembay-display"; + reg = <0x20930000 0x3000>, + <0x20900000 0x4000>, + <0x20910000 0x30>; + reg-names = "lcd", "mipi", "msscam"; + interrupts = <GIC_SPI 33 IRQ_TYPE_LEVEL_HIGH>; + clocks = <&scmi_clk MOVISOC_KMB_MSS_AUX_LCD>, + <&scmi_clk MOVISOC_KMB_MSS_AUX_MIPI_TX0>, + <&scmi_clk MOVISOC_KMB_MSS_AUX_MIPI_ECFG>, + <&scmi_clk MOVISOC_KMB_MSS_AUX_MIPI_CFG>, + <&scmi_clk MOVISOC_KMB_A53_PLL_0_OUT_0>; + clock-names = "clk_lcd", "clk_mipi", "clk_mipi_ecfg", + "clk_mipi_cfg", "clk_pll0"; + + encoder-slave = <&adv7535>; + + port { + dsi_output: endpoint { + remote-endpoint = <&adv7535_input>; + }; + }; + };
This patch adds bindings for Intel KeemBay Display v2: review changes from Rob Herring Signed-off-by: Anitha Chrisanthus <anitha.chrisanthus@intel.com> --- .../bindings/display/intel,keembay-display.yaml | 99 ++++++++++++++++++++++ 1 file changed, 99 insertions(+) create mode 100644 Documentation/devicetree/bindings/display/intel,keembay-display.yaml