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
