[PATCH v9 1/5] dt-bindings: display: Add support for Intel KeemBay Display

[Chrisanthus, Anitha]

Hi Neil,

 Thanks for your review, please see my reply inline.

> -----Original Message-----
> From: Neil Armstrong <narmstrong at baylibre.com>
> Sent: Friday, October 9, 2020 2:10 AM
> To: Chrisanthus, Anitha <anitha.chrisanthus at intel.com>; dri-
> devel at lists.freedesktop.org; devicetree at vger.kernel.org; Vetter, Daniel
> <daniel.vetter at intel.com>
> Cc: Dea, Edmund J <edmund.j.dea at intel.com>; sam at 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 at 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 at intel.com>
> > +  - Edmond J Dea <edmund.j.dea at 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 at 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