by Krzysztof Kozlowski

[permalink] [raw]

Subject: Re: [PATCH v4 12/17] arm64: dts: freescale: apalis-imx8: analogue audio comment

On 18/01/2023 15:33, Marcel Ziswiler wrote:
> On Wed, 2023-01-18 at 14:59 +0100, Krzysztof Kozlowski wrote:
>> On 18/01/2023 08:26, Marcel Ziswiler wrote:
>>> From: Marcel Ziswiler <[email protected]>
>>>
>>> Added a comment about Apalis analogue audio being another todo.
>>>
>>> Signed-off-by: Marcel Ziswiler <[email protected]>
>>
>> You just added this file in previous patch. This should be squashed
>> instead of splitting one logical change (adding new hardware) into
>> multiple independent patches.
>
> Remember, I explicitly did that for review purpose and even mentioned this in the cover letter.

There is b4 diff for this purpose. Splitting patches makes them
difficult to apply and confuse.

Best regards,
Krzysztof

2023-01-19 01:49:17

by Liu Ying

[permalink] [raw]

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

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 <[email protected]>
> > >
> > > This patch adds pwm_lvds0/1 support together with a
> > > i.MX 8QM LVDS subsystem device tree.
> > >
> > > Signed-off-by: Liu Ying <[email protected]>
> > > Signed-off-by: Marcel Ziswiler <[email protected]>
> > >
> > > ---
> > >
> > > 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