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

[Liu Ying]

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@xxxxxxx>
> > > 
> > > This patch adds pwm_lvds0/1 support together with a
> > > i.MX 8QM LVDS subsystem device tree.
> > > 
> > > Signed-off-by: Liu Ying <victor.liu@xxxxxxx>
> > > Signed-off-by: Marcel Ziswiler <marcel.ziswiler@xxxxxxxxxxx>
> > > 
> > > ---
> > > 
> > > 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