MIME-Version: 1.0
References: <20211104161804.587250-1-aford173@gmail.com> <20211104161804.587250-5-aford173@gmail.com>
 <YY7kg3GC5lnX7TgW@robh.at.kernel.org>
In-Reply-To: <YY7kg3GC5lnX7TgW@robh.at.kernel.org>
From:   Adam Ford <aford173@gmail.com>
Date:   Fri, 12 Nov 2021 18:35:20 -0500
Message-ID: <CAHCN7xJHx5eDt3iMH3vtDrL=zgvHq3LtLxe_8XUSjSJR4b1Fug@mail.gmail.com>
Subject: Re: [PATCH V3 4/9] dt-bindings: soc: add binding for i.MX8MN DISP blk-ctrl
To:     Rob Herring <robh@kernel.org>
Cc:     arm-soc <linux-arm-kernel@lists.infradead.org>,
        Adam Ford-BE <aford@beaconembedded.com>,
        ariel.dalessandro@collabora.com,
        Krzysztof Kozlowski <krzk@kernel.org>,
        Tim Harvey <tharvey@gateworks.com>,
        Lucas Stach <l.stach@pengutronix.de>,
        devicetree <devicetree@vger.kernel.org>,
        Shawn Guo <shawnguo@kernel.org>,
        Sascha Hauer <s.hauer@pengutronix.de>,
        Pengutronix Kernel Team <kernel@pengutronix.de>,
        Fabio Estevam <festevam@gmail.com>,
        NXP Linux Team <linux-imx@nxp.com>,
        Linux Kernel Mailing List <linux-kernel@vger.kernel.org>
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

On Fri, Nov 12, 2021 at 5:02 PM Rob Herring <robh@kernel.org> wrote:
>
> On Thu, Nov 04, 2021 at 11:17:59AM -0500, Adam Ford wrote:
> > Add the DT binding for the i.MX8MN DISP blk-ctrl.
> >
> > Signed-off-by: Adam Ford <aford173@gmail.com>
> >
> > diff --git a/Documentation/devicetree/bindings/soc/imx/fsl,imx8mn-disp-blk-ctrl.yaml b/Documentation/devicetree/bindings/soc/imx/fsl,imx8mn-disp-blk-ctrl.yaml
> > new file mode 100644
> > index 000000000000..fbeaac399c50
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/soc/imx/fsl,imx8mn-disp-blk-ctrl.yaml
> > @@ -0,0 +1,97 @@
> > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> > +%YAML 1.2
> > +---
> > +$id: http://devicetree.org/schemas/soc/imx/fsl,imx8mn-disp-blk-ctrl.yaml#
> > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > +
> > +title: NXP i.MX8MN DISP blk-ctrl
> > +
> > +maintainers:
> > +  - Lucas Stach <l.stach@pengutronix.de>
> > +
> > +description:
> > +  The i.MX8MN DISP blk-ctrl is a top-level peripheral providing access to
> > +  the NoC and ensuring proper power sequencing of the display and MIPI CSI
> > +  peripherals located in the DISP domain of the SoC.
> > +
> > +properties:
> > +  compatible:
> > +    items:
> > +      - const: fsl,imx8mn-disp-blk-ctrl
> > +      - const: syscon
>
> Are there other functions in this block? If so, what?

This is similar to the i.MX8M Mini.  From what I can tell, there are
some extra clock and reset registers that are not associated with
their respective blocks. The main power domain controller called GPCv2
partially enables the power domains, but there is some ping-pong
between that IP block and this one,

>
> > +
> > +  reg:
> > +    maxItems: 1
> > +
> > +  '#power-domain-cells':
> > +    const: 1
> > +
> > +  power-domains:
> > +    minItems: 5
> > +    maxItems: 5
> > +
> > +  power-domain-names:
> > +    items:
> > +      - const: bus
> > +      - const: isi
> > +      - const: lcdif
> > +      - const: mipi-dsi
> > +      - const: mipi-csi
> > +
> > +  clocks:
> > +    minItems: 11
> > +    maxItems: 11
> > +
> > +  clock-names:
> > +    items:
> > +      - const: disp_axi
> > +      - const: disp_apb
> > +      - const: disp_axi_root
> > +      - const: disp_apb_root
> > +      - const: lcdif-axi
> > +      - const: lcdif-apb
> > +      - const: lcdif-pix
> > +      - const: dsi-pclk
> > +      - const: dsi-ref
> > +      - const: csi-aclk
> > +      - const: csi-pclk
> > +
> > +required:
> > +  - compatible
> > +  - reg
> > +  - power-domains
> > +  - power-domain-names
> > +  - clocks
> > +  - clock-names
> > +
> > +additionalProperties: false
> > +
> > +examples:
> > +  - |
> > +    #include <dt-bindings/clock/imx8mn-clock.h>
> > +    #include <dt-bindings/power/imx8mn-power.h>
> > +
> > +    disp_blk_ctl: blk_ctrl@32e28000 {
> > +      compatible = "fsl,imx8mn-disp-blk-ctrl", "syscon";
> > +      reg = <0x32e28000 0x100>;
> > +      power-domains = <&pgc_dispmix>, <&pgc_dispmix>,
> > +                      <&pgc_dispmix>, <&pgc_mipi>,
> > +                      <&pgc_mipi>;
> > +      power-domain-names = "bus", "isi", "lcdif", "mipi-dsi",
> > +                           "mipi-csi";
>
> This looks odd. These are the same power domains this node provides.

It is odd, but  I'll try to explain it the best I can.

When this SoC was developed, there were a few additional registers
placed into this IP block that control other IP blocks, and ping-pong
with the main power domain controller to make the various IP blocks
work.

GPCv2, the main power domain controller attempts to enable the domain,
but it has to enable the bus clock and clear the bus reset from this
IP block because neither the reset nor the clock enable were placed
into reset or clock IP blocks.

For example,  when the mipi-csi controller needs to come up, it needs
to request the power domain from disp-blk-ctrl IP block.  The blk-ctrl
block first requests the pgc_mipi power domain.  The GPC ping-pongs
the blk-ctrl which has to set or clear registers so the GPC can finish
its job.  One the GPC has finished its part the blk-ctrl then enables
the clock and reset that controls the csi.  This same is true for
several quasi-related items like DSI, CSI, and LCDIF.

The items listed in the power-domains are from the GPC, and the items
in the power-domain-names are inside the blk-ctrl.  When an item needs
a power domain from the blk-ctrl, the corresponding GPC power domain
must also be referenced.

In order to enable some of the registers, the blk-ctrl IP block also
needs to enable some additional clocks or the system can hang.

Lucas Stach wrote the original driver and might be able to better
explain how it works on the Mini, but the Nano behaves the same way
with different bits in the regisers.


>
> > +      clocks = <&clk IMX8MN_CLK_DISP_AXI>,
> > +               <&clk IMX8MN_CLK_DISP_APB>,
> > +               <&clk IMX8MN_CLK_DISP_AXI_ROOT>,
> > +               <&clk IMX8MN_CLK_DISP_APB_ROOT>,
> > +               <&clk IMX8MN_CLK_DISP_AXI_ROOT>,
> > +               <&clk IMX8MN_CLK_DISP_APB_ROOT>,
> > +               <&clk IMX8MN_CLK_DISP_PIXEL_ROOT>,
> > +               <&clk IMX8MN_CLK_DSI_CORE>,
> > +               <&clk IMX8MN_CLK_DSI_PHY_REF>,
> > +               <&clk IMX8MN_CLK_CSI1_PHY_REF>,
> > +               <&clk IMX8MN_CLK_CAMERA_PIXEL_ROOT>;
> > +       clock-names = "disp_axi", "disp_apb", "disp_axi_root", "disp_apb_root",
> > +                     "lcdif-axi", "lcdif-apb", "lcdif-pix", "dsi-pclk",
> > +                     "dsi-ref", "csi-aclk", "csi-pclk";
> > +       #power-domain-cells = <1>;
> > +    };
> > --
> > 2.32.0
> >
> >