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

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

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