RE: [PATCH 2/4] dt-bindings: clock: imx-lpcg: add support to parse clocks from device tree

[Aisheng Dong <aisheng.dong@nxp.com>]

[...]

> > diff --git a/Documentation/devicetree/bindings/clock/imx8qxp-lpcg.txt
> > b/Documentation/devicetree/bindings/clock/imx8qxp-lpcg.txt
> > index 965cfa4..a317844 100644
> > --- a/Documentation/devicetree/bindings/clock/imx8qxp-lpcg.txt
> > +++ b/Documentation/devicetree/bindings/clock/imx8qxp-lpcg.txt
> > @@ -11,6 +11,20 @@ enabled by these control bits, it might still not
> > be running based  on the base resource.
> >
> >  Required properties:
> > +- compatible:          Should be one of:
> > +                         "fsl,imx8qxp-lpcg"
> > +                         "fsl,imx8qm-lpcg" followed by
> "fsl,imx8qxp-lpcg".
> > +- reg:                 Address and length of the register set.
> > +- #clock-cells:                Should be 1. One LPCG supports multiple
> clocks.
> > +- clocks:              Input parent clocks phandle array for each clock.
> > +- bit-offset:          An integer array indicating the bit offset for each
> clock.
> > +- hw-autogate:         Boolean array indicating whether supports HW
> autogate for
> > +                       each clock.
> 
> This looks like one clk per node style of bindings which is a direction we don't
> want DT bindings to go in. It leads to a bunch of time parsing DT to generate
> clks and in general doesn't represent the clock controller hardware that is
> there. Basically, anything with 'bit-offset'
> in the binding is not going to be acceptable.
> 

This is not one clk per node but one clock controller per node which strictly
describes the HW.

On MX8, each LPCG is a separate clock controller which can control a couple of
clock output gates for one specific device to use. Each device has a corresponding
LPCG clock controller and those LPCGs are independent with each other with
separate IO space.

Those LPCGs are distributed in various SS (subsystems) along with device resources.
Describing them in device tree SS dtsi doesn't seem to be an issue as it is representing
the real hardware.

For SCU clocks, they're similar case that each SS having separate clock controllers
In HW which are managed by SCU firmware. So it seems also okay to put them
in device tree SS dtsi file, right?

If you're concerning each node having one compatible string, how about doing like
many power domain does as below?

Having only one compatible string in clock parent nodes.

//LSIO SS
lsio_scu_clk: lsio-scu-clock-controller {
        compatible = "fsl,imx8qxp-clock", "fsl,scu-clk";

        fspi0_clk: clock-fspi0{
                #clock-cells = <0>;
                rsrc-id = <IMX_SC_R_FSPI_0>;
                clk-type = <IMX_SC_PM_CLK_PER>;
                power-domains = <&pd IMX_SC_R_FSPI_0>;
        };
		
		fspi1_clk: clock-fspi1{
			...
		};
        ...
};    

/* LPCG clocks */
lsio_lpcg_clk: lsio-lpcg-clock-controller {
        compatible = "fsl,imx8qxp-lpcg";

        pwm0_lpcg: clock-controller@5d400000 {
                reg = <0x5d400000 0x10000>;
                #clock-cells = <1>;
                clocks = <&pwm0_clk>, <&pwm0_clk>, <&pwm0_clk>,
                         <&lsio_bus_clk>, <&pwm0_clk>;
                bit-offset = <0 4 16 20 24>;
                clock-output-names = "pwm0_lpcg_ipg_clk",
                                     "pwm0_lpcg_ipg_hf_clk",
                                     "pwm0_lpcg_ipg_s_clk",
                                     "pwm0_lpcg_ipg_slv_clk",
                                     "pwm0_lpcg_ipg_mstr_clk";
                power-domains = <&pd IMX_SC_R_PWM_0>;
                status = "disabled";
        };
        ...
};

I also have spent a lot time to investigate how TI and Samsung does. However, 
finally i.MX is still different and I still believe current way is better for i.MX,
mainly due to below reasons:

1) IMX having separate clock controllers in HW, not shared one like others
2) IMX SoC is comprised of various HW SS (Subsystem) while others don't have.
  Describing clocks in DT can help a better SW architecture to describe HW.
3) Each clock is associated with a power domain. DT is the best place to indicate it.
4) Clock availability (Both SCU and LPCG) are configurable according to
  different HW partition configuration by SCU firmware.
  Defining them all in driver will cause annoying and continued churn in driver
  all the time when adding new SoC support.
  e.g. 
  Handling availability for different SS in different SoC.
  Defining Clock IDs for diferent SS in different SoC for same clocks.

By putting clocks in DT, we can make the clock driver completely generic and
no more churn in the driver anymore in the future for adding new SoC support.
It can significantly save the driver maintain effort.

Last, this won't break compatibility. It's just introduce a new binding.

Regards
Dong Aisheng

> > +- clock-output-names:  Shall be the corresponding names of the outputs.
> > +                       NOTE this property must be specified in the
> same order
> > +                       as the clock bit-offset and hw-autogate property.
> > +
> > +Legacy binding (DEPRECATED):
> >  - compatible:  Should be one of:
> >                   "fsl,imx8qxp-lpcg-adma",