2020-12-18 17:54:34

by Guido Günther

Subject: [PATCH] arm64: dts: imx8mq: Add clock parents for mipi dphy

This makes sure the clock tree setup for the dphy is not dependent on
other components.

Without this change bringing up the display can fail like

kernel: phy phy-30a00300.dphy.2: Invalid CM/CN/CO values: 165/217/1
kernel: phy phy-30a00300.dphy.2: for hs_clk/ref_clk=451656000/593999998 ~ 165/217

if LCDIF doesn't set up that part of the clock tree first. This was
noticed when testing the Librem 5 devkit with defconfig. It doesn't
happen when modules are built in.

Signed-off-by: Guido Günther <[email protected]>
---
arch/arm64/boot/dts/freescale/imx8mq.dtsi | 11 ++++++++---
1 file changed, 8 insertions(+), 3 deletions(-)

diff --git a/arch/arm64/boot/dts/freescale/imx8mq.dtsi b/arch/arm64/boot/dts/freescale/imx8mq.dtsi
index a841a023e8e0..ca0847e8f13c 100644
--- a/arch/arm64/boot/dts/freescale/imx8mq.dtsi
+++ b/arch/arm64/boot/dts/freescale/imx8mq.dtsi
@@ -1016,9 +1016,14 @@ dphy: dphy@30a00300 {
reg = <0x30a00300 0x100>;
clocks = <&clk IMX8MQ_CLK_DSI_PHY_REF>;
clock-names = "phy_ref";
- assigned-clocks = <&clk IMX8MQ_CLK_DSI_PHY_REF>;
- assigned-clock-parents = <&clk IMX8MQ_VIDEO_PLL1_OUT>;
- assigned-clock-rates = <24000000>;
+ assigned-clocks = <&clk IMX8MQ_VIDEO_PLL1_REF_SEL>,
+ <&clk IMX8MQ_VIDEO_PLL1_BYPASS>,
+ <&clk IMX8MQ_CLK_DSI_PHY_REF>,
+ <&clk IMX8MQ_VIDEO_PLL1>;
+ assigned-clock-parents = <&clk IMX8MQ_CLK_25M>,
+ <&clk IMX8MQ_VIDEO_PLL1>,
+ <&clk IMX8MQ_VIDEO_PLL1_OUT>;
+ assigned-clock-rates = <0>, <0>, <24000000>;
#phy-cells = <0>;
power-domains = <&pgc_mipi>;
status = "disabled";
--
2.29.2

2021-01-10 12:48:49

by Shawn Guo

Subject: Re: [PATCH] arm64: dts: imx8mq: Add clock parents for mipi dphy

On Fri, Dec 18, 2020 at 06:50:05PM +0100, Guido G?nther wrote:
> This makes sure the clock tree setup for the dphy is not dependent on
> other components.
>
> Without this change bringing up the display can fail like
>
> kernel: phy phy-30a00300.dphy.2: Invalid CM/CN/CO values: 165/217/1
> kernel: phy phy-30a00300.dphy.2: for hs_clk/ref_clk=451656000/593999998 ~ 165/217
>
> if LCDIF doesn't set up that part of the clock tree first. This was
> noticed when testing the Librem 5 devkit with defconfig. It doesn't
> happen when modules are built in.
>
> Signed-off-by: Guido G?nther <[email protected]>
> ---
> arch/arm64/boot/dts/freescale/imx8mq.dtsi | 11 ++++++++---
> 1 file changed, 8 insertions(+), 3 deletions(-)
>
> diff --git a/arch/arm64/boot/dts/freescale/imx8mq.dtsi b/arch/arm64/boot/dts/freescale/imx8mq.dtsi
> index a841a023e8e0..ca0847e8f13c 100644
> --- a/arch/arm64/boot/dts/freescale/imx8mq.dtsi
> +++ b/arch/arm64/boot/dts/freescale/imx8mq.dtsi
> @@ -1016,9 +1016,14 @@ dphy: dphy@30a00300 {
> reg = <0x30a00300 0x100>;
> clocks = <&clk IMX8MQ_CLK_DSI_PHY_REF>;
> clock-names = "phy_ref";
> - assigned-clocks = <&clk IMX8MQ_CLK_DSI_PHY_REF>;
> - assigned-clock-parents = <&clk IMX8MQ_VIDEO_PLL1_OUT>;
> - assigned-clock-rates = <24000000>;
> + assigned-clocks = <&clk IMX8MQ_VIDEO_PLL1_REF_SEL>,
> + <&clk IMX8MQ_VIDEO_PLL1_BYPASS>,
> + <&clk IMX8MQ_CLK_DSI_PHY_REF>,
> + <&clk IMX8MQ_VIDEO_PLL1>;

You do not seem to set parent or rate on IMX8MQ_VIDEO_PLL1. Why do you
have it here?

Shawn

> + assigned-clock-parents = <&clk IMX8MQ_CLK_25M>,
> + <&clk IMX8MQ_VIDEO_PLL1>,
> + <&clk IMX8MQ_VIDEO_PLL1_OUT>;
> + assigned-clock-rates = <0>, <0>, <24000000>;
> #phy-cells = <0>;
> power-domains = <&pgc_mipi>;
> status = "disabled";
> --
> 2.29.2
>

2021-01-10 17:05:24

by Guido Günther

Subject: Re: [PATCH] arm64: dts: imx8mq: Add clock parents for mipi dphy

Hi,
On Sun, Jan 10, 2021 at 08:46:29PM +0800, Shawn Guo wrote:
> On Fri, Dec 18, 2020 at 06:50:05PM +0100, Guido G?nther wrote:
> > This makes sure the clock tree setup for the dphy is not dependent on
> > other components.
> >
> > Without this change bringing up the display can fail like
> >
> > kernel: phy phy-30a00300.dphy.2: Invalid CM/CN/CO values: 165/217/1
> > kernel: phy phy-30a00300.dphy.2: for hs_clk/ref_clk=451656000/593999998 ~ 165/217
> >
> > if LCDIF doesn't set up that part of the clock tree first. This was
> > noticed when testing the Librem 5 devkit with defconfig. It doesn't
> > happen when modules are built in.
> >
> > Signed-off-by: Guido G?nther <[email protected]>
> > ---
> > arch/arm64/boot/dts/freescale/imx8mq.dtsi | 11 ++++++++---
> > 1 file changed, 8 insertions(+), 3 deletions(-)
> >
> > diff --git a/arch/arm64/boot/dts/freescale/imx8mq.dtsi b/arch/arm64/boot/dts/freescale/imx8mq.dtsi
> > index a841a023e8e0..ca0847e8f13c 100644
> > --- a/arch/arm64/boot/dts/freescale/imx8mq.dtsi
> > +++ b/arch/arm64/boot/dts/freescale/imx8mq.dtsi
> > @@ -1016,9 +1016,14 @@ dphy: dphy@30a00300 {
> > reg = <0x30a00300 0x100>;
> > clocks = <&clk IMX8MQ_CLK_DSI_PHY_REF>;
> > clock-names = "phy_ref";
> > - assigned-clocks = <&clk IMX8MQ_CLK_DSI_PHY_REF>;
> > - assigned-clock-parents = <&clk IMX8MQ_VIDEO_PLL1_OUT>;
> > - assigned-clock-rates = <24000000>;
> > + assigned-clocks = <&clk IMX8MQ_VIDEO_PLL1_REF_SEL>,
> > + <&clk IMX8MQ_VIDEO_PLL1_BYPASS>,
> > + <&clk IMX8MQ_CLK_DSI_PHY_REF>,
> > + <&clk IMX8MQ_VIDEO_PLL1>;
>
> You do not seem to set parent or rate on IMX8MQ_VIDEO_PLL1. Why do you
> have it here?

Good point. I've added a clock rate for IMX8MQ_VIDEO_PLL1 since
then the clock tree reproduces exactly with and with the DSI host
controller disabled in DT (otherwise we end up with a rate of 22MHz
instead of 24Mhz).

Cheers,
-- Guido

>
> Shawn
>
> > + assigned-clock-parents = <&clk IMX8MQ_CLK_25M>,
> > + <&clk IMX8MQ_VIDEO_PLL1>,
> > + <&clk IMX8MQ_VIDEO_PLL1_OUT>;
> > + assigned-clock-rates = <0>, <0>, <24000000>;
> > #phy-cells = <0>;
> > power-domains = <&pgc_mipi>;
> > status = "disabled";
> > --
> > 2.29.2
> >
>