diff mbox series

[v4,05/17] arm64: dts: imx8qm: add pwm_lvds0/1 support

Message ID 20230118072656.18845-6-marcel@ziswiler.com (mailing list archive)
State New, archived
Headers show
Series arm64: dts: freescale: prepare and add apalis imx8 support | expand

Commit Message

Marcel Ziswiler Jan. 18, 2023, 7:26 a.m. UTC
From: Liu Ying <victor.liu@nxp.com>

This patch adds pwm_lvds0/1 support together with a
i.MX 8QM LVDS subsystem device tree.

Signed-off-by: Liu Ying <victor.liu@nxp.com>
Signed-off-by: Marcel Ziswiler <marcel.ziswiler@toradex.com>

---

Changes in v4:
- New patch combining the following downstream patches limitted to LVDS
  PWM functionality for now:
  commit 036c6b28a186 ("arm64: imx8qm.dtsi: Add LVDS0/1 subsystems support")
  commit c3d29611d9d4 ("arm64: imx8qm-ss-lvds.dtsi: Add pwm_lvds0/1 support")
  commit baf1b0f22f8a ("LF-882-1 arm64: imx8qm-ss-lvds.dtsi: Separate ipg clock for lvds0/1 subsystems")

 .../boot/dts/freescale/imx8qm-ss-lvds.dtsi    | 83 +++++++++++++++++++
 arch/arm64/boot/dts/freescale/imx8qm.dtsi     |  1 +
 2 files changed, 84 insertions(+)
 create mode 100644 arch/arm64/boot/dts/freescale/imx8qm-ss-lvds.dtsi

Comments

Liu Ying Jan. 18, 2023, 8:47 a.m. UTC | #1
Hi Marcel,

On Wed, 2023-01-18 at 08:26 +0100, Marcel Ziswiler wrote:
> From: Liu Ying <victor.liu@nxp.com>
> 
> This patch adds pwm_lvds0/1 support together with a
> i.MX 8QM LVDS subsystem device tree.
> 
> Signed-off-by: Liu Ying <victor.liu@nxp.com>
> Signed-off-by: Marcel Ziswiler <marcel.ziswiler@toradex.com>
> 
> ---
> 
> Changes in v4:
> - New patch combining the following downstream patches limitted to
> LVDS
>   PWM functionality for now:
>   commit 036c6b28a186 ("arm64: imx8qm.dtsi: Add LVDS0/1 subsystems
> support")
>   commit c3d29611d9d4 ("arm64: imx8qm-ss-lvds.dtsi: Add pwm_lvds0/1
> support")
>   commit baf1b0f22f8a ("LF-882-1 arm64: imx8qm-ss-lvds.dtsi: Separate
> ipg clock for lvds0/1 subsystems")

Sorry, I don't think the downstream patches are doing things correct.
The biggest problem is that the lvds related devices should be child
devices of display subsystem pixel link MSI bus device(See below
comments).

I have local patches to add some lvds related devices which haven't
been submitted.

> 
>  .../boot/dts/freescale/imx8qm-ss-lvds.dtsi    | 83
> +++++++++++++++++++
>  arch/arm64/boot/dts/freescale/imx8qm.dtsi     |  1 +
>  2 files changed, 84 insertions(+)
>  create mode 100644 arch/arm64/boot/dts/freescale/imx8qm-ss-lvds.dtsi
> 
> diff --git a/arch/arm64/boot/dts/freescale/imx8qm-ss-lvds.dtsi
> b/arch/arm64/boot/dts/freescale/imx8qm-ss-lvds.dtsi
> new file mode 100644
> index 000000000000..4b940fc3c890
> --- /dev/null
> +++ b/arch/arm64/boot/dts/freescale/imx8qm-ss-lvds.dtsi
> @@ -0,0 +1,83 @@
> +// SPDX-License-Identifier: GPL-2.0+
> +/*
> + * Copyright 2023 NXP
> + */
> +
> +/ {
> +	lvds1_subsys: bus@56240000 {
> +		compatible = "simple-bus";

In my local patches, there is no 'lvds{0,1}_subsys'. Instead, lvds
related devices are child devices of 'dc{0,1}_pl_msi_bus' buses,
something like this:

In imx8qm-ss-dc.dtsi:
&dc0_subsys {
        dc0_pl_msi_bus: bus@56200000 {
                compatible = "fsl,imx8qm-display-pixel-link-msi-bus",
"simple-pm-bus";
                #address-cells = <1>;
                #size-cells = <1>;
                reg = <0x56200000 0x20000>;
                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>;
                status = "disabled";
        };
};

In imx8qm-ss-lvds.dtsi:
&dc0_pl_msi_bus {
        lvds0_irqsteer: interrupt-controller@56240000 {
                compatible = "fsl,imx-irqsteer";
                ...
        };

        lvds0_csr: bus@56241000 {
                compatible = "fsl,imx8qm-lvds-csr", "syscon", "simple-
pm-bus";
		...
	};

	lvds0_pwm_lpcg: clock-controller@5624300c {
                compatible = "fsl,imx8qm-lpcg", "fsl,imx8qxp-lpcg";
		...
	};

	lvds0_pwm: pwm@56244000 {
                compatible = "fsl,imx8qm-pwm", "fsl,imx27-pwm";
		...
	};
};

The below patch is needed to use clocks for pixel link MSI bus as a
simple-pm-bus.


https://lore.kernel.org/lkml/20221226031417.1056745-1-victor.liu@nxp.com/t/

"fsl,imx8qm-lvds-csr" needs to be added into simple_pm_bus_of_match[]
in simple-pm-bus.c.

> +		#address-cells = <1>;
> +		#size-cells = <1>;
> +		ranges = <0x56240000 0x0 0x56240000 0x10000>;
> +
> +		lvds0_ipg_clk: clock-lvds-ipg {
> +			compatible = "fixed-clock";
> +			#clock-cells = <0>;
> +			clock-frequency = <24000000>;
> +			clock-output-names = "lvds0_ipg_clk";
> +		};
> +
> +		lvds0_pwm_lpcg: clock-controller@5624300c {
> +			compatible = "fsl,imx8qm-lpcg";

Should list "fsl,imx8qxp-lpcg" as one item as well, since imx8qxp-
lpcg.yaml mentions it.

> +			reg = <0x5624300c 0x4>;
> +			#clock-cells = <1>;
> +			clocks = <&clk IMX_SC_R_LVDS_0_PWM_0
> IMX_SC_PM_CLK_PER>,
> +				 <&lvds0_ipg_clk>;
> +			clock-indices = <IMX_LPCG_CLK_0>,
> <IMX_LPCG_CLK_4>;
> +			clock-output-names = "lvds0_pwm_lpcg_clk",
> +					     "lvds0_pwm_lpcg_ipg_clk";
> +			power-domains = <&pd IMX_SC_R_LVDS_0_PWM_0>;
> +		};
> +
> +		pwm_lvds0: pwm@56244000 {
> +			compatible = "fsl,imx27-pwm";

Need to document "fsl,imx8qm-pwm" in imx-pwm.yaml and list it in the
compatible sting here.

> +			reg = <0x56244000 0x1000>;
> +			clocks = <&lvds0_pwm_lpcg 0>,
> +				 <&lvds0_pwm_lpcg 1>;

In my local patches, I set the clocks property as:
                clocks = <&lvds0_pwm_lpcg IMX_LPCG_CLK_0>,
                         <&lvds0_pwm_lpcg IMX_LPCG_CLK_4>;

I'm not sure if it is correct now.

> +			clock-names = "per", "ipg";
> +			assigned-clocks = <&clk IMX_SC_R_LVDS_0_PWM_0
> IMX_SC_PM_CLK_PER>;
> +			assigned-clock-rates = <24000000>;
> +			#pwm-cells = <2>;
> +			power-domains = <&pd IMX_SC_R_LVDS_0_PWM_0>;
> +			status = "disabled";

In my local patches, this node has the below interrupt related
properties:
                interrupt-parent = <&lvds0_irqsteer>;
                interrupts = <12>;

> +		};
> +	};
> +
> +	lvds2_subsys: bus@57240000 {

Above comments apply for 'lvds2_subsys' similarly.

[...]

Regards,
Liu Ying
Marcel Ziswiler Jan. 18, 2023, 1:31 p.m. UTC | #2
Hi Liu

Thank you very much for your valuable feedback.

On Wed, 2023-01-18 at 16:47 +0800, Liu Ying wrote:
> Hi Marcel,
> 
> On Wed, 2023-01-18 at 08:26 +0100, Marcel Ziswiler wrote:
> > From: Liu Ying <victor.liu@nxp.com>
> > 
> > This patch adds pwm_lvds0/1 support together with a
> > i.MX 8QM LVDS subsystem device tree.
> > 
> > Signed-off-by: Liu Ying <victor.liu@nxp.com>
> > Signed-off-by: Marcel Ziswiler <marcel.ziswiler@toradex.com>
> > 
> > ---
> > 
> > Changes in v4:
> > - New patch combining the following downstream patches limitted to
> > LVDS
> >   PWM functionality for now:
> >   commit 036c6b28a186 ("arm64: imx8qm.dtsi: Add LVDS0/1 subsystems
> > support")
> >   commit c3d29611d9d4 ("arm64: imx8qm-ss-lvds.dtsi: Add pwm_lvds0/1
> > support")
> >   commit baf1b0f22f8a ("LF-882-1 arm64: imx8qm-ss-lvds.dtsi: Separate
> > ipg clock for lvds0/1 subsystems")
> 
> Sorry, I don't think the downstream patches are doing things correct.
> The biggest problem is that the lvds related devices should be child
> devices of display subsystem pixel link MSI bus device(See below
> comments).

Remember, I even inquired about this [1] but did not get any feedback (yet).

> I have local patches to add some lvds related devices which haven't
> been submitted.

As mentioned before, I would be very interested in actually testing such and giving hopefully valuable
feedback.

> >  .../boot/dts/freescale/imx8qm-ss-lvds.dtsi    | 83
> > +++++++++++++++++++
> >  arch/arm64/boot/dts/freescale/imx8qm.dtsi     |  1 +
> >  2 files changed, 84 insertions(+)
> >  create mode 100644 arch/arm64/boot/dts/freescale/imx8qm-ss-lvds.dtsi
> > 
> > diff --git a/arch/arm64/boot/dts/freescale/imx8qm-ss-lvds.dtsi
> > b/arch/arm64/boot/dts/freescale/imx8qm-ss-lvds.dtsi
> > new file mode 100644
> > index 000000000000..4b940fc3c890
> > --- /dev/null
> > +++ b/arch/arm64/boot/dts/freescale/imx8qm-ss-lvds.dtsi
> > @@ -0,0 +1,83 @@
> > +// SPDX-License-Identifier: GPL-2.0+
> > +/*
> > + * Copyright 2023 NXP
> > + */
> > +
> > +/ {
> > +       lvds1_subsys: bus@56240000 {
> > +               compatible = "simple-bus";
> 
> In my local patches, there is no 'lvds{0,1}_subsys'. Instead, lvds
> related devices are child devices of 'dc{0,1}_pl_msi_bus' buses,
> something like this:
> 
> In imx8qm-ss-dc.dtsi:
> &dc0_subsys {
>         dc0_pl_msi_bus: bus@56200000 {
>                 compatible = "fsl,imx8qm-display-pixel-link-msi-bus",
> "simple-pm-bus";
>                 #address-cells = <1>;
>                 #size-cells = <1>;
>                 reg = <0x56200000 0x20000>;
>                 interrupt-parent = <&dc0_irqsteer>;

Concerning irqsteer I was unsure about whether or not all this is already upstream. At least the device tree
parts seem missing.

>                 interrupts = <320>;
>                 ranges;
>                 clocks = <&dc0_disp_ctrl_link_mst0_lpcg
> IMX_LPCG_CLK_4>,

I believe those IMX_LPCG_CLK_ are indices only. But more on that further below.

>                          <&dc0_disp_ctrl_link_mst0_lpcg
> IMX_LPCG_CLK_4>;
>                 clock-names = "msi", "ahb";
>                 power-domains = <&pd IMX_SC_R_DC_0>;
>                 status = "disabled";
>         };
> };
> 
> In imx8qm-ss-lvds.dtsi:
> &dc0_pl_msi_bus {
>         lvds0_irqsteer: interrupt-controller@56240000 {
>                 compatible = "fsl,imx-irqsteer";
>                 ...
>         };
> 
>         lvds0_csr: bus@56241000 {
>                 compatible = "fsl,imx8qm-lvds-csr", "syscon", "simple-
> pm-bus";
>                 ...
>         };
> 
>         lvds0_pwm_lpcg: clock-controller@5624300c {
>                 compatible = "fsl,imx8qm-lpcg", "fsl,imx8qxp-lpcg";
>                 ...
>         };
> 
>         lvds0_pwm: pwm@56244000 {
>                 compatible = "fsl,imx8qm-pwm", "fsl,imx27-pwm";
>                 ...
>         };
> };
> 
> The below patch is needed to use clocks for pixel link MSI bus as a
> simple-pm-bus.
> 
> 
> https://lore.kernel.org/lkml/20221226031417.1056745-1-victor.liu@nxp.com/t/
> 
> "fsl,imx8qm-lvds-csr" needs to be added into simple_pm_bus_of_match[]
> in simple-pm-bus.c.

Okay, I was not aware of that.

> > +               #address-cells = <1>;
> > +               #size-cells = <1>;
> > +               ranges = <0x56240000 0x0 0x56240000 0x10000>;
> > +
> > +               lvds0_ipg_clk: clock-lvds-ipg {
> > +                       compatible = "fixed-clock";
> > +                       #clock-cells = <0>;
> > +                       clock-frequency = <24000000>;
> > +                       clock-output-names = "lvds0_ipg_clk";
> > +               };
> > +
> > +               lvds0_pwm_lpcg: clock-controller@5624300c {
> > +                       compatible = "fsl,imx8qm-lpcg";
> 
> Should list "fsl,imx8qxp-lpcg" as one item as well, since imx8qxp-
> lpcg.yaml mentions it.

Agreed.

> > +                       reg = <0x5624300c 0x4>;
> > +                       #clock-cells = <1>;
> > +                       clocks = <&clk IMX_SC_R_LVDS_0_PWM_0
> > IMX_SC_PM_CLK_PER>,
> > +                                <&lvds0_ipg_clk>;
> > +                       clock-indices = <IMX_LPCG_CLK_0>,
> > <IMX_LPCG_CLK_4>;
> > +                       clock-output-names = "lvds0_pwm_lpcg_clk",
> > +                                            "lvds0_pwm_lpcg_ipg_clk";
> > +                       power-domains = <&pd IMX_SC_R_LVDS_0_PWM_0>;
> > +               };
> > +
> > +               pwm_lvds0: pwm@56244000 {
> > +                       compatible = "fsl,imx27-pwm";
> 
> Need to document "fsl,imx8qm-pwm" in imx-pwm.yaml and list it in the
> compatible sting here.

But so far no such "fsl,imx8qm-pwm" exists anywhere. Is it really required?

> > +                       reg = <0x56244000 0x1000>;
> > +                       clocks = <&lvds0_pwm_lpcg 0>,
> > +                                <&lvds0_pwm_lpcg 1>;
> 
> In my local patches, I set the clocks property as:
>                 clocks = <&lvds0_pwm_lpcg IMX_LPCG_CLK_0>,
>                          <&lvds0_pwm_lpcg IMX_LPCG_CLK_4>;
> 
> I'm not sure if it is correct now.

I doubt as those IMX_LPCG_CLK_ are defines for the indices e.g. IMX_LPCG_CLK_4 actually is 16 and not 1 (or 4)
(;-p).

> > +                       clock-names = "per", "ipg";
> > +                       assigned-clocks = <&clk IMX_SC_R_LVDS_0_PWM_0
> > IMX_SC_PM_CLK_PER>;
> > +                       assigned-clock-rates = <24000000>;
> > +                       #pwm-cells = <2>;
> > +                       power-domains = <&pd IMX_SC_R_LVDS_0_PWM_0>;
> > +                       status = "disabled";
> 
> In my local patches, this node has the below interrupt related
> properties:
>                 interrupt-parent = <&lvds0_irqsteer>;
>                 interrupts = <12>;

As mentioned above my familiarity with irqsteer is far from complete. Plus interestingly for me this LVDS PWM
actually really works without interrupts. Not sure whether or not or what exactly might not "fully" work
without.

> > +               };
> > +       };
> > +
> > +       lvds2_subsys: bus@57240000 {
> 
> Above comments apply for 'lvds2_subsys' similarly.
> 
> [...]
> 
> Regards,
> Liu Ying

[1] https://lore.kernel.org/all/549bf1f26b8212de2d4890a27e396250257aa027.camel@toradex.com/

Cheers

Marcel
Liu Ying Jan. 19, 2023, 1:23 a.m. UTC | #3
Hi Marcel,

On Wed, 2023-01-18 at 13:31 +0000, Marcel Ziswiler wrote:
> Hi Liu
> 
> Thank you very much for your valuable feedback.
> 
> On Wed, 2023-01-18 at 16:47 +0800, Liu Ying wrote:
> > Hi Marcel,
> > 
> > On Wed, 2023-01-18 at 08:26 +0100, Marcel Ziswiler wrote:
> > > From: Liu Ying <victor.liu@nxp.com>
> > > 
> > > This patch adds pwm_lvds0/1 support together with a
> > > i.MX 8QM LVDS subsystem device tree.
> > > 
> > > Signed-off-by: Liu Ying <victor.liu@nxp.com>
> > > Signed-off-by: Marcel Ziswiler <marcel.ziswiler@toradex.com>
> > > 
> > > ---
> > > 
> > > Changes in v4:
> > > - New patch combining the following downstream patches limitted
> > > to
> > > LVDS
> > >   PWM functionality for now:
> > >   commit 036c6b28a186 ("arm64: imx8qm.dtsi: Add LVDS0/1
> > > subsystems
> > > support")
> > >   commit c3d29611d9d4 ("arm64: imx8qm-ss-lvds.dtsi: Add
> > > pwm_lvds0/1
> > > support")
> > >   commit baf1b0f22f8a ("LF-882-1 arm64: imx8qm-ss-lvds.dtsi:
> > > Separate
> > > ipg clock for lvds0/1 subsystems")
> > 
> > Sorry, I don't think the downstream patches are doing things
> > correct.
> > The biggest problem is that the lvds related devices should be
> > child
> > devices of display subsystem pixel link MSI bus device(See below
> > comments).
> 
> Remember, I even inquired about this [1] but did not get any feedback
> (yet).
> 
> > I have local patches to add some lvds related devices which haven't
> > been submitted.
> 
> As mentioned before, I would be very interested in actually testing
> such and giving hopefully valuable
> feedback.

We don't have any official public git for sharing local patches like I
mentioned earlier.  That's not convenient.  I'll see if I can share my
local patches/changes to you in some way, or please wait until they are
submitted for review.

> 
> > >  .../boot/dts/freescale/imx8qm-ss-lvds.dtsi    | 83
> > > +++++++++++++++++++
> > >  arch/arm64/boot/dts/freescale/imx8qm.dtsi     |  1 +
> > >  2 files changed, 84 insertions(+)
> > >  create mode 100644 arch/arm64/boot/dts/freescale/imx8qm-ss-
> > > lvds.dtsi
> > > 
> > > diff --git a/arch/arm64/boot/dts/freescale/imx8qm-ss-lvds.dtsi
> > > b/arch/arm64/boot/dts/freescale/imx8qm-ss-lvds.dtsi
> > > new file mode 100644
> > > index 000000000000..4b940fc3c890
> > > --- /dev/null
> > > +++ b/arch/arm64/boot/dts/freescale/imx8qm-ss-lvds.dtsi
> > > @@ -0,0 +1,83 @@
> > > +// SPDX-License-Identifier: GPL-2.0+
> > > +/*
> > > + * Copyright 2023 NXP
> > > + */
> > > +
> > > +/ {
> > > +       lvds1_subsys: bus@56240000 {
> > > +               compatible = "simple-bus";
> > 
> > In my local patches, there is no 'lvds{0,1}_subsys'. Instead, lvds
> > related devices are child devices of 'dc{0,1}_pl_msi_bus' buses,
> > something like this:
> > 
> > In imx8qm-ss-dc.dtsi:
> > &dc0_subsys {
> >         dc0_pl_msi_bus: bus@56200000 {
> >                 compatible = "fsl,imx8qm-display-pixel-link-msi-
> > bus",
> > "simple-pm-bus";
> >                 #address-cells = <1>;
> >                 #size-cells = <1>;
> >                 reg = <0x56200000 0x20000>;
> >                 interrupt-parent = <&dc0_irqsteer>;
> 
> Concerning irqsteer I was unsure about whether or not all this is
> already upstream. At least the device tree
> parts seem missing.

Dt-binding documentation and driver were upstreamed:
Documentation/devicetree/bindings/interrupt-
controller/fsl,irqsteer.yaml
drivers/irqchip/irq-imx-irqsteer.c

'dc{0,1}_irqsteer' is not yet upstreamed in device tree.

> 
> >                 interrupts = <320>;
> >                 ranges;
> >                 clocks = <&dc0_disp_ctrl_link_mst0_lpcg
> > IMX_LPCG_CLK_4>,
> 
> I believe those IMX_LPCG_CLK_ are indices only. But more on that
> further below.

Yes, they should be indices and an IMX_LPCG_CLK_ index should be used
here to specify the consumed clock according to imx8qxp-lpcg.yaml.

> 
> >                          <&dc0_disp_ctrl_link_mst0_lpcg
> > IMX_LPCG_CLK_4>;
> >                 clock-names = "msi", "ahb";
> >                 power-domains = <&pd IMX_SC_R_DC_0>;
> >                 status = "disabled";
> >         };
> > };
> > 
> > In imx8qm-ss-lvds.dtsi:
> > &dc0_pl_msi_bus {
> >         lvds0_irqsteer: interrupt-controller@56240000 {
> >                 compatible = "fsl,imx-irqsteer";
> >                 ...
> >         };
> > 
> >         lvds0_csr: bus@56241000 {
> >                 compatible = "fsl,imx8qm-lvds-csr", "syscon",
> > "simple-
> > pm-bus";
> >                 ...
> >         };
> > 
> >         lvds0_pwm_lpcg: clock-controller@5624300c {
> >                 compatible = "fsl,imx8qm-lpcg", "fsl,imx8qxp-lpcg";
> >                 ...
> >         };
> > 
> >         lvds0_pwm: pwm@56244000 {
> >                 compatible = "fsl,imx8qm-pwm", "fsl,imx27-pwm";
> >                 ...
> >         };
> > };
> > 
> > The below patch is needed to use clocks for pixel link MSI bus as a
> > simple-pm-bus.
> > 
> > 
> > 
https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Flore.kernel.org%2Flkml%2F20221226031417.1056745-1-victor.liu%40nxp.com%2Ft%2F&data=05%7C01%7Cvictor.liu%40nxp.com%7Cc85f20a3212c4da6178f08daf9585749%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0%7C638096455062491606%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&sdata=SR5g4yuJ14y3tqRNM3QdlF7gmWeup74D6Q69F8gJhlU%3D&reserved=0
> > 
> > "fsl,imx8qm-lvds-csr" needs to be added into
> > simple_pm_bus_of_match[]
> > in simple-pm-bus.c.
> 
> Okay, I was not aware of that.
> 
> > > +               #address-cells = <1>;
> > > +               #size-cells = <1>;
> > > +               ranges = <0x56240000 0x0 0x56240000 0x10000>;
> > > +
> > > +               lvds0_ipg_clk: clock-lvds-ipg {
> > > +                       compatible = "fixed-clock";
> > > +                       #clock-cells = <0>;
> > > +                       clock-frequency = <24000000>;
> > > +                       clock-output-names = "lvds0_ipg_clk";
> > > +               };
> > > +
> > > +               lvds0_pwm_lpcg: clock-controller@5624300c {
> > > +                       compatible = "fsl,imx8qm-lpcg";
> > 
> > Should list "fsl,imx8qxp-lpcg" as one item as well, since imx8qxp-
> > lpcg.yaml mentions it.
> 
> Agreed.
> 
> > > +                       reg = <0x5624300c 0x4>;
> > > +                       #clock-cells = <1>;
> > > +                       clocks = <&clk IMX_SC_R_LVDS_0_PWM_0
> > > IMX_SC_PM_CLK_PER>,
> > > +                                <&lvds0_ipg_clk>;
> > > +                       clock-indices = <IMX_LPCG_CLK_0>,
> > > <IMX_LPCG_CLK_4>;
> > > +                       clock-output-names =
> > > "lvds0_pwm_lpcg_clk",
> > > +                                           
> > > "lvds0_pwm_lpcg_ipg_clk";
> > > +                       power-domains = <&pd
> > > IMX_SC_R_LVDS_0_PWM_0>;
> > > +               };
> > > +
> > > +               pwm_lvds0: pwm@56244000 {
> > > +                       compatible = "fsl,imx27-pwm";
> > 
> > Need to document "fsl,imx8qm-pwm" in imx-pwm.yaml and list it in
> > the
> > compatible sting here.
> 
> But so far no such "fsl,imx8qm-pwm" exists anywhere. Is it really
> required?

Yes, I think it is required. See imx-pwm.yaml, there are quite a few
"fsl,soc-pwm" compatible strings listed as one item together with
"fsl,imx27-pwm".

> 
> > > +                       reg = <0x56244000 0x1000>;
> > > +                       clocks = <&lvds0_pwm_lpcg 0>,
> > > +                                <&lvds0_pwm_lpcg 1>;
> > 
> > In my local patches, I set the clocks property as:
> >                 clocks = <&lvds0_pwm_lpcg IMX_LPCG_CLK_0>,
> >                          <&lvds0_pwm_lpcg IMX_LPCG_CLK_4>;
> > 
> > I'm not sure if it is correct now.
> 
> I doubt as those IMX_LPCG_CLK_ are defines for the indices e.g.
> IMX_LPCG_CLK_4 actually is 16 and not 1 (or 4)
> (;-p).

IMX_LPCG_CLK_ should be used here.

Like I mentioned above, imx8qxp-lpcg.yaml explicitly said "The clock
consumer should specify the desired clock by having the clock ID in its
"clocks" phandle cell. See the full list of clock IDs from:
include/dt-bindings/clock/imx8-lpcg.h".

> 
> > > +                       clock-names = "per", "ipg";
> > > +                       assigned-clocks = <&clk
> > > IMX_SC_R_LVDS_0_PWM_0
> > > IMX_SC_PM_CLK_PER>;
> > > +                       assigned-clock-rates = <24000000>;
> > > +                       #pwm-cells = <2>;
> > > +                       power-domains = <&pd
> > > IMX_SC_R_LVDS_0_PWM_0>;
> > > +                       status = "disabled";
> > 
> > In my local patches, this node has the below interrupt related
> > properties:
> >                 interrupt-parent = <&lvds0_irqsteer>;
> >                 interrupts = <12>;
> 
> As mentioned above my familiarity with irqsteer is far from complete.
> Plus interestingly for me this LVDS PWM
> actually really works without interrupts. Not sure whether or not or
> what exactly might not "fully" work
> without.

Device tree describes hardware, which doesn't bind with
software/operating systems.  If an IP generates interrupts to an
interrupt controller, then the interrupts property should be documented
in dt-binding documentation and device tree should list the property.

imx-pwm.yaml actually requires the interrupts property.

Regards,
Liu Ying

> 
> > > +               };
> > > +       };
> > > +
> > > +       lvds2_subsys: bus@57240000 {
> > 
> > Above comments apply for 'lvds2_subsys' similarly.
> > 
> > [...]
> > 
> > Regards,
> > Liu Ying
> 
> [1] 
> https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Flore.kernel.org%2Fall%2F549bf1f26b8212de2d4890a27e396250257aa027.camel%40toradex.com%2F&data=05%7C01%7Cvictor.liu%40nxp.com%7Cc85f20a3212c4da6178f08daf9585749%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0%7C638096455062491606%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&sdata=9VyU3y7%2BCbmNNvtfWIXK4p1xaDpCEIh9q2crZNmzgO0%3D&reserved=0
> 
> Cheers
> 
> Marcel
diff mbox series

Patch

diff --git a/arch/arm64/boot/dts/freescale/imx8qm-ss-lvds.dtsi b/arch/arm64/boot/dts/freescale/imx8qm-ss-lvds.dtsi
new file mode 100644
index 000000000000..4b940fc3c890
--- /dev/null
+++ b/arch/arm64/boot/dts/freescale/imx8qm-ss-lvds.dtsi
@@ -0,0 +1,83 @@ 
+// SPDX-License-Identifier: GPL-2.0+
+/*
+ * Copyright 2023 NXP
+ */
+
+/ {
+	lvds1_subsys: bus@56240000 {
+		compatible = "simple-bus";
+		#address-cells = <1>;
+		#size-cells = <1>;
+		ranges = <0x56240000 0x0 0x56240000 0x10000>;
+
+		lvds0_ipg_clk: clock-lvds-ipg {
+			compatible = "fixed-clock";
+			#clock-cells = <0>;
+			clock-frequency = <24000000>;
+			clock-output-names = "lvds0_ipg_clk";
+		};
+
+		lvds0_pwm_lpcg: clock-controller@5624300c {
+			compatible = "fsl,imx8qm-lpcg";
+			reg = <0x5624300c 0x4>;
+			#clock-cells = <1>;
+			clocks = <&clk IMX_SC_R_LVDS_0_PWM_0 IMX_SC_PM_CLK_PER>,
+				 <&lvds0_ipg_clk>;
+			clock-indices = <IMX_LPCG_CLK_0>, <IMX_LPCG_CLK_4>;
+			clock-output-names = "lvds0_pwm_lpcg_clk",
+					     "lvds0_pwm_lpcg_ipg_clk";
+			power-domains = <&pd IMX_SC_R_LVDS_0_PWM_0>;
+		};
+
+		pwm_lvds0: pwm@56244000 {
+			compatible = "fsl,imx27-pwm";
+			reg = <0x56244000 0x1000>;
+			clocks = <&lvds0_pwm_lpcg 0>,
+				 <&lvds0_pwm_lpcg 1>;
+			clock-names = "per", "ipg";
+			assigned-clocks = <&clk IMX_SC_R_LVDS_0_PWM_0 IMX_SC_PM_CLK_PER>;
+			assigned-clock-rates = <24000000>;
+			#pwm-cells = <2>;
+			power-domains = <&pd IMX_SC_R_LVDS_0_PWM_0>;
+			status = "disabled";
+		};
+	};
+
+	lvds2_subsys: bus@57240000 {
+		compatible = "simple-bus";
+		#address-cells = <1>;
+		#size-cells = <1>;
+		ranges = <0x57240000 0x0 0x57240000 0x10000>;
+
+		lvds1_ipg_clk: clock-lvds-ipg {
+			compatible = "fixed-clock";
+			#clock-cells = <0>;
+			clock-frequency = <24000000>;
+			clock-output-names = "lvds1_ipg_clk";
+		};
+
+		lvds1_pwm_lpcg: clock-controller@5724300c {
+			compatible = "fsl,imx8qm-lpcg";
+			reg = <0x5724300c 0x4>;
+			#clock-cells = <1>;
+			clocks = <&clk IMX_SC_R_LVDS_1_PWM_0 IMX_SC_PM_CLK_PER>,
+				 <&lvds1_ipg_clk>;
+			clock-indices = <IMX_LPCG_CLK_0>, <IMX_LPCG_CLK_4>;
+			clock-output-names = "lvds1_pwm_lpcg_clk",
+					     "lvds1_pwm_lpcg_ipg_clk";
+			power-domains = <&pd IMX_SC_R_LVDS_1_PWM_0>;
+		};
+
+		pwm_lvds1: pwm@57244000 {
+			compatible = "fsl,imx27-pwm";
+			reg = <0x57244000 0x1000>;
+			clock-names = "ipg", "per";
+			clocks = <&lvds1_pwm_lpcg 4>,
+				 <&lvds1_pwm_lpcg 1>;
+			assigned-clocks = <&clk IMX_SC_R_LVDS_1_PWM_0 IMX_SC_PM_CLK_PER>;
+			assigned-clock-rates = <24000000>;
+			#pwm-cells = <2>;
+			status = "disabled";
+		};
+	};
+};
diff --git a/arch/arm64/boot/dts/freescale/imx8qm.dtsi b/arch/arm64/boot/dts/freescale/imx8qm.dtsi
index 41ce8336f29e..422edd2f20fa 100644
--- a/arch/arm64/boot/dts/freescale/imx8qm.dtsi
+++ b/arch/arm64/boot/dts/freescale/imx8qm.dtsi
@@ -222,3 +222,4 @@  rtc: rtc {
 #include "imx8qm-ss-dma.dtsi"
 #include "imx8qm-ss-conn.dtsi"
 #include "imx8qm-ss-lsio.dtsi"
+#include "imx8qm-ss-lvds.dtsi"