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.
> > .../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@xxxxxxx/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@xxxxxxxxxxx/
Cheers
Marcel
