[PATCH v4 05/17] arm64: dts: imx8qm: add pwm_lvds0/1 support
Marcel Ziswiler
marcel.ziswiler at toradex.com
Wed Jan 18 05:31:40 PST 2023
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 at 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 at nxp.com>
> > Signed-off-by: Marcel Ziswiler <marcel.ziswiler at 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 at 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 at 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 at 56240000 {
> compatible = "fsl,imx-irqsteer";
> ...
> };
>
> lvds0_csr: bus at 56241000 {
> compatible = "fsl,imx8qm-lvds-csr", "syscon", "simple-
> pm-bus";
> ...
> };
>
> lvds0_pwm_lpcg: clock-controller at 5624300c {
> compatible = "fsl,imx8qm-lpcg", "fsl,imx8qxp-lpcg";
> ...
> };
>
> lvds0_pwm: pwm at 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 at 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 at 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 at 57240000 {
>
> Above comments apply for 'lvds2_subsys' similarly.
>
> [...]
>
> Regards,
> Liu Ying
[1] https://lore.kernel.org/all/549bf1f26b8212de2d4890a27e396250257aa027.camel@toradex.com/
Cheers
Marcel
More information about the linux-arm-kernel
mailing list