Re: [PATCH 3/3] dt-bindings: bus: Add Freescale i.MX8qxp pixel link MSI bus binding

[Liu Ying <victor.liu@nxp.com>]

On Tue, 2022-08-02 at 13:04 +0200, Krzysztof Kozlowski wrote:
> On 02/08/2022 09:13, Liu Ying wrote:
> > Freescale i.MX8qxp pixel link MSI bus is a simple memory-mapped
> > bus.
> > It is used to access peripherals in i.MX8qm/qxp imaging, LVDS, MIPI
> > DSI and HDMI TX subsystems, like I2C controller, PWM controller,
> > MIPI DSI controller and Control and Status Registers (CSR) module.
> > 
> > Reference simple-pm-bus bindings and add Freescale i.MX8qxp pixel
> > link MSI bus specific bindings.
> > 
> > Signed-off-by: Liu Ying <victor.liu@nxp.com>
> > ---
> >  .../bus/fsl,imx8qxp-pixel-link-msi-bus.yaml   | 84
> > +++++++++++++++++++
> >  1 file changed, 84 insertions(+)
> >  create mode 100644
> > Documentation/devicetree/bindings/bus/fsl,imx8qxp-pixel-link-msi-
> > bus.yaml
> > 
> > diff --git a/Documentation/devicetree/bindings/bus/fsl,imx8qxp-
> > pixel-link-msi-bus.yaml
> > b/Documentation/devicetree/bindings/bus/fsl,imx8qxp-pixel-link-msi-
> > bus.yaml
> > new file mode 100644
> > index 000000000000..24f50535f5c2
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/bus/fsl,imx8qxp-pixel-link-
> > msi-bus.yaml
> > @@ -0,0 +1,84 @@
> > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> > +%YAML 1.2
> > +---
> > +$id: 
> > https://eur01.safelinks.protection.outlook.com/?url=http%3A%2F%2Fdevicetree.org%2Fschemas%2Fbus%2Ffsl%2Cimx8qxp-pixel-link-msi-bus.yaml%23&amp;data=05%7C01%7Cvictor.liu%40nxp.com%7C7ee06e5e179b4cb8529308da7476be70%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0%7C637950350594434521%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&amp;sdata=GsZgqUmEsVm2If6bvx%2FrVOFWnaiJp2zFVERvzWP%2BecM%3D&amp;reserved=0
> > +$schema: 
> > https://eur01.safelinks.protection.outlook.com/?url=http%3A%2F%2Fdevicetree.org%2Fmeta-schemas%2Fcore.yaml%23&amp;data=05%7C01%7Cvictor.liu%40nxp.com%7C7ee06e5e179b4cb8529308da7476be70%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0%7C637950350594434521%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&amp;sdata=v6x0rQDqg%2FrLshXZvKdeM97IdOWtnu9O5o0Dz3%2FQID8%3D&amp;reserved=0
> > +
> > +title: Freescale i.MX8qxp Pixel Link Medium Speed Interconnect
> > (MSI) Bus
> 
> Shouldn't this be interconnect, not a bus? Not only located in
> interconnect directory but actually being proper interconnect?
> Although
> you mentioned that the firmware controls it, so maybe that would
> explain
> this being only a resource provider.

Linux kernel just enables the power domain and the two input clocks
for the MSI bus, then the MSI bus starts to work. All other stuff is
totally out of Linux kernel's control, which means SCFW doesn't expose
any direct or indirect MSI bus control interfaces to it's user. So, it
looks like the MSI bus fits into the simple power-managed bus category.

> 
> You should be sure of it, because later if you want to add proper
> interconnect properties (e.g. bandwidth voting, paths) *you will not
> be
> able*. Ever.

There is really nothing more than the power domain and the two input
clocks that can be controlled for the MSI bus by Linux driver. I don't
expect to add any other properties.

> 
> > +
> > +maintainers:
> > +  - Liu Ying <victor.liu@nxp.com>
> > +
> > +description: |
> > +  i.MX8qxp pixel link MSI bus is used to control settings of PHYs,
> > I/Os
> > +  sitting together with the PHYs.  It is not the same as the MSI
> > bus coming
> > +  from i.MX8 System Controller Unit (SCU) which is used to control
> > power,
> > +  clock and reset through the i.MX8 Distributed Slave System
> > Controller (DSC).
> > +
> > +  i.MX8qxp pixel link MSI bus is a simple memory-mapped bus. Two
> > input clocks,
> > +  that is, MSI clock and AHB clock, need to be enabled so that
> > peripherals
> > +  connected to the bus can be accessed. Also, the bus is part of a
> > power
> > +  domain. The power domain needs to be enabled before the
> > peripherals can
> > +  be accessed.
> > +
> > +  Peripherals in i.MX8qm/qxp imaging, LVDS, MIPI DSI and HDMI TX
> > subsystems,
> > +  like I2C controller, PWM controller, MIPI DSI controller and
> > Control and
> > +  Status Registers (CSR) module, are accessed through the bus.
> > +
> > +  The i.MX System Controller Firmware (SCFW) owns and uses the
> > i.MX8qm/qxp
> > +  pixel link MSI bus controller and does not allow SCFW user to
> > control it.
> > +  So, the controller's registers cannot be accessed by SCFW user.
> > Hence,
> > +  the interrupts generated by the controller don't make any sense
> > from SCFW
> > +  user's point of view.
> > +
> > +allOf:
> > +  - $ref: simple-pm-bus.yaml#
> > +
> > +properties:
> > +  compatible:
> > +    items:
> > +      - enum:
> > +          - fsl,imx8qxp-display-pixel-link-msi-bus
> > +          - fsl,imx8qm-display-pixel-link-msi-bus
> > +      - {} # simple-pm-bus, but not listed here to avoid false
> > select
> 
> simple-pm-bus must be here. You need to sort out the select instead,
> just like we do it for other devices (e.g. primecell).

Will do.

> 
> > +
> > +  reg:
> > +    maxItems: 1
> > +
> > +  interrupts:
> > +    maxItems: 1
> > +
> > +  clocks:
> > +    items:
> > +      - description: master gated clock from system
> > +      - description: AHB clock
> > +
> > +  clock-names:
> > +    items:
> > +      - const: msi
> > +      - const: ahb
> > +
> > +required:
> 
> compatible and reg as well.

Will do.

> 
> > +  - clocks
> > +  - clock-names
> > +  - power-domains
> > +
> > +unevaluatedProperties: false
> > +
> > +examples:
> > +  - |
> > +    #include <dt-bindings/clock/imx8-lpcg.h>
> > +    #include <dt-bindings/firmware/imx/rsrc.h>
> > +    bus@56200000 {
> > +        compatible = "fsl,imx8qxp-display-pixel-link-msi-bus",
> > "simple-pm-bus";
> > +        #address-cells = <1>;
> > +        #size-cells = <1>;
> > +        reg = <0x56200000 0x20000>;
> 
> put reg just after compatible.

Will do.

Thanks for your review.

Liu Ying

> 
> > +        interrupt-parent = <&dc0_irqsteer>;
> > +        interrupts = <320>;
> > +        ranges;
> > +        clocks = <&dc0_disp_ctrl_link_mst0_lpcg IMX_LPCG_CLK_4>,
> > +                 <&dc0_disp_ctrl_link_mst0_lpcg IMX_LPCG_CLK_4>;
> > +        clock-names = "msi", "ahb";
> > +        power-domains = <&pd IMX_SC_R_DC_0>;
> > +    };
> 
> 
> Best regards,
> Krzysztof