Hi Stephen,
> > > 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.
>
Shawn is okay with the whole point of MX8 Arch improvement[1].
(e.g. moving clocks into DT)
But we still need the your agreement on the clock part first.
Could you help review if this is okay to you?
[1] https://patchwork.kernel.org/cover/10824537/
Regards
Dong Aisheng
> 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",
