RE: [EXT] Re: [PATCH v6 2/8] dt-bindings: display: bridge: Add Cadence MHDP8501 HDMI and DP

[Sandor Yu <sandor.yu@nxp.com>]

Hi Rob,

Thanks for your comments,

> -----Original Message-----
> From: Rob Herring <robh@kernel.org>
> Sent: 2023年6月20日 23:49
> To: Sandor Yu <sandor.yu@nxp.com>
> Cc: andrzej.hajda@intel.com; neil.armstrong@linaro.org;
> robert.foss@linaro.org; Laurent.pinchart@ideasonboard.com;
> jonas@kwiboo.se; jernej.skrabec@gmail.com; airlied@gmail.com;
> daniel@ffwll.ch; krzysztof.kozlowski+dt@linaro.org; shawnguo@kernel.org;
> s.hauer@pengutronix.de; festevam@gmail.com; vkoul@kernel.org;
> dri-devel@lists.freedesktop.org; devicetree@vger.kernel.org;
> linux-arm-kernel@lists.infradead.org; linux-kernel@vger.kernel.org;
> linux-phy@lists.infradead.org; kernel@pengutronix.de; dl-linux-imx
> <linux-imx@nxp.com>; Oliver Brown <oliver.brown@nxp.com>
> Subject: [EXT] Re: [PATCH v6 2/8] dt-bindings: display: bridge: Add Cadence
> MHDP8501 HDMI and DP
> 
> Caution: This is an external email. Please take care when clicking links or
> opening attachments. When in doubt, report the message using the 'Report
> this email' button
> 
> 
> On Thu, Jun 15, 2023 at 09:38:12AM +0800, Sandor Yu wrote:
> > Add bindings for Cadence MHDP8501 DisplayPort and HDMI driver.
> 
> Bindings are for h/w, not a driver.
OK, I will change it in the next version.
> 
> >
> > Signed-off-by: Sandor Yu <Sandor.yu@nxp.com>
> > ---
> >  .../display/bridge/cdns,mhdp8501.yaml         | 105
> ++++++++++++++++++
> >  1 file changed, 105 insertions(+)
> >  create mode 100644
> > Documentation/devicetree/bindings/display/bridge/cdns,mhdp8501.yaml
> >
> > diff --git
> > a/Documentation/devicetree/bindings/display/bridge/cdns,mhdp8501.yaml
> > b/Documentation/devicetree/bindings/display/bridge/cdns,mhdp8501.yaml
> > new file mode 100644
> > index 000000000000..a54756815e6f
> > --- /dev/null
> > +++
> b/Documentation/devicetree/bindings/display/bridge/cdns,mhdp8501.y
> > +++ aml
> > @@ -0,0 +1,105 @@
> > +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause) %YAML 1.2
> > +---
> > +$id:
> > +https://eur01.safelinks.protection.outlook.com/?url=http%3A%2F%2Fdevi
> >
> +cetree.org%2Fschemas%2Fdisplay%2Fbridge%2Fcdns%2Cmhdp8501.yaml%
> 23&dat
> >
> +a=05%7C01%7CSandor.yu%40nxp.com%7C4d4e118d60d744b5dba708db71
> a5de79%7C
> >
> +686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0%7C63822872943965530
> 2%7CUnkno
> >
> +wn%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1
> haWwi
> >
> +LCJXVCI6Mn0%3D%7C3000%7C%7C%7C&sdata=UEsMdkZBmfD7tM1wzJ71
> DHQoi4zVOkpT
> > +A9TNE7Rxn%2B8%3D&reserved=0
> > +$schema:
> > +https://eur01.safelinks.protection.outlook.com/?url=http%3A%2F%2Fdevi
> >
> +cetree.org%2Fmeta-schemas%2Fcore.yaml%23&data=05%7C01%7CSandor.
> yu%40n
> >
> +xp.com%7C4d4e118d60d744b5dba708db71a5de79%7C686ea1d3bc2b4c6fa
> 92cd99c5
> >
> +c301635%7C0%7C0%7C638228729439655302%7CUnknown%7CTWFpbGZs
> b3d8eyJWIjoi
> >
> +MC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3
> 000%7C%
> >
> +7C%7C&sdata=Zu3v0yG2BXWXvTWV5oLiGvdu3O3PhK%2FrYNJIS2zHwpI%3
> D&reserved
> > +=0
> > +
> > +title: Cadence MHDP8501 Displayport bridge
> > +
> > +maintainers:
> > +  - Sandor Yu <Sandor.yu@nxp.com>
> > +
> > +description:
> > +  The Cadence MHDP8501 Displayport/HDMI TX interface.
> > +
> > +properties:
> > +  compatible:
> > +    enum:
> > +      - cdns,mhdp8501-dp
> > +      - cdns,mhdp8501-hdmi
> > +      - fsl,imx8mq-mhdp8501-dp
> > +      - fsl,imx8mq-mhdp8501-hdmi
> 
> Is DP vs. HDMI fixed for a particular SoC implementation or it's a board level
> decision. In the latter case, the type of connector should determine the mode,
> not compatible.
DP or HDMI is bord level decision. 
Because DP and HDMI have different initialize process and less functions could be reuse, so they have different drivers.
Please check it in patch
[PATCH v6 3/8] drm: bridge: Cadence: Add MHDP8501 DP driver
[PATCH v6 5/8] drm: bridge: Cadence: Add MHDP8501 HDMI driver

If use the type of connector to determine the mode, hdmi and DP driver have to combine into one driver.
So the compatible may the better choice.
> 
> > +
> > +  reg:
> > +    maxItems: 1
> > +
> > +  clocks:
> > +    maxItems: 1
> > +    description: MHDP8501 DP/HDMI APB clock.
> 
> Seems odd there's no clock tied to the pixel/serdes clock.
MHDP8501 for i.MX8MQ use the pixel clock from PHY PLL not from external CCM.
The pixel clock will be set in function phy_configure

B.R
Sandor
> 
> > +
> > +  phys:
> > +    maxItems: 1
> > +
> > +  interrupts:
> > +    items:
> > +      - description: Hotplug cable plugin.
> > +      - description: Hotplug cable plugout.
> > +
> > +  interrupt-names:
> > +    items:
> > +      - const: plug_in
> > +      - const: plug_out
> > +
> > +  ports:
> > +    $ref: /schemas/graph.yaml#/properties/ports
> > +
> > +    properties:
> > +      port@0:
> > +        $ref: /schemas/graph.yaml#/properties/port
> > +        description:
> > +          Input port from display controller output.
> > +      port@1:
> > +        $ref: /schemas/graph.yaml#/properties/port
> > +        description:
> > +          Output port to DP/HDMI connector.
> > +
> > +    required:
> > +      - port@0
> > +      - port@1
> > +
> > +required:
> > +  - compatible
> > +  - reg
> > +  - clocks
> > +  - interrupts
> > +  - interrupt-names
> > +  - phys
> > +  - ports
> > +
> > +additionalProperties: false
> > +
> > +examples:
> > +  - |
> > +    #include <dt-bindings/clock/imx8mq-clock.h>
> > +    #include <dt-bindings/interrupt-controller/arm-gic.h>
> > +
> > +    mhdp_dp: dp-bridge@32c00000 {
> > +        compatible = "fsl,imx8mq-mhdp8501-dp";
> > +        reg = <0x32c00000 0x100000>;
> > +        interrupts = <GIC_SPI 16 IRQ_TYPE_LEVEL_HIGH>,
> > +                     <GIC_SPI 25 IRQ_TYPE_LEVEL_HIGH>;
> > +        interrupt-names = "plug_in", "plug_out";
> > +        clocks = <&clk IMX8MQ_CLK_DISP_APB_ROOT>;
> > +        phys = <&dp_phy>;
> > +
> > +        ports {
> > +            #address-cells = <1>;
> > +            #size-cells = <0>;
> > +
> > +            port@0 {
> > +                reg = <0>;
> > +
> > +                mhdp_in: endpoint {
> > +                    remote-endpoint = <&dcss_out>;
> > +                };
> > +            };
> > +
> > +            port@1 {
> > +                reg = <1>;
> > +
> > +                mhdp_out: endpoint {
> > +                    remote-endpoint = <&dp_con>;
> > +                };
> > +            };
> > +        };
> > +    };
> > --
> > 2.34.1
> >