[PATCH 1/2] drm/tegra: Set the dsi lp clk parent and rate

[PATCH 1/2] drm/tegra: Set the dsi lp clk parent and rate

[Sean Paul]

Per NVidia, this clock rate should be around 70MHz in
order to properly sample reads on data lane 0. In order
to achieve this rate, we need to reparent the clock from
clk_m which can only achieve 12MHz. Add parent_lp to the
dts bindings and set the parent & rate on init.

Signed-off-by: Sean Paul <seanpaul@chromium.org>
---
 .../devicetree/bindings/gpu/nvidia,tegra20-host1x.txt  | 10 ++++++++--
 drivers/gpu/drm/tegra/dsi.c                            | 18 ++++++++++++++++++
 drivers/gpu/drm/tegra/dsi.h                            |  3 +++
 3 files changed, 29 insertions(+), 2 deletions(-)

diff --git a/Documentation/devicetree/bindings/gpu/nvidia,tegra20-host1x.txt b/Documentation/devicetree/bindings/gpu/nvidia,tegra20-host1x.txt
index b48f4ef..fef2918 100644
--- a/Documentation/devicetree/bindings/gpu/nvidia,tegra20-host1x.txt
+++ b/Documentation/devicetree/bindings/gpu/nvidia,tegra20-host1x.txt
@@ -191,6 +191,10 @@ of the following host1x client modules:
   - nvidia,hpd-gpio: specifies a GPIO used for hotplug detection
   - nvidia,edid: supplies a binary EDID blob
   - nvidia,panel: phandle of a display panel
+  - clock-names: Can include the following entries:
+    - lp_parent: The parent clock for lp
+  - clocks: Must contain an entry for each optional entry in clock-names.
+    See ../clocks/clock-bindings.txt for details.
 
 - sor: serial output resource
 
@@ -360,8 +364,10 @@ Example:
 			compatible = "nvidia,tegra20-dsi";
 			reg = <0x54300000 0x00040000>;
 			clocks = <&tegra_car TEGRA20_CLK_DSI>,
-				 <&tegra_car TEGRA20_CLK_PLL_D_OUT0>;
-			clock-names = "dsi", "parent";
+				 <&tegra_car TEGRA124_CLK_DSIALP>,
+				 <&tegra_car TEGRA20_CLK_PLL_D_OUT0>,
+				 <&tegra_car TEGRA124_CLK_PLL_P>;
+			clock-names = "dsi", "lp", "parent", "lp_parent";
 			resets = <&tegra_car 48>;
 			reset-names = "dsi";
 			status = "disabled";
diff --git a/drivers/gpu/drm/tegra/dsi.c b/drivers/gpu/drm/tegra/dsi.c
index f787445..c0258ae 100644
--- a/drivers/gpu/drm/tegra/dsi.c
+++ b/drivers/gpu/drm/tegra/dsi.c
@@ -837,6 +837,7 @@ static int tegra_dsi_probe(struct platform_device *pdev)
 	struct tegra_dsi *dsi;
 	struct resource *regs;
 	int err;
+	struct clk *lp_parent;
 
 	dsi = devm_kzalloc(&pdev->dev, sizeof(*dsi), GFP_KERNEL);
 	if (!dsi)
@@ -879,6 +880,23 @@ static int tegra_dsi_probe(struct platform_device *pdev)
 		return PTR_ERR(dsi->clk_lp);
 	}
 
+	lp_parent = devm_clk_get(&pdev->dev, "lp_parent");
+	if (!IS_ERR(lp_parent)) {
+		err = clk_set_parent(dsi->clk_lp, lp_parent);
+		if (err < 0) {
+			dev_err(&pdev->dev, "cannot set lp clock parent\n");
+			return err;
+		}
+	} else {
+		dev_info(&pdev->dev, "no lp clock parent, using hw default\n");
+	}
+
+	err = clk_set_rate(dsi->clk_lp, DSI_LP_CLK_RATE);
+	if (err < 0) {
+		dev_err(&pdev->dev, "cannot set low-power clock rate\n");
+		return err;
+	}
+
 	err = clk_prepare_enable(dsi->clk_lp);
 	if (err < 0) {
 		dev_err(&pdev->dev, "cannot enable low-power clock\n");
diff --git a/drivers/gpu/drm/tegra/dsi.h b/drivers/gpu/drm/tegra/dsi.h
index 5ce610d..a332caf 100644
--- a/drivers/gpu/drm/tegra/dsi.h
+++ b/drivers/gpu/drm/tegra/dsi.h
@@ -127,4 +127,7 @@ enum tegra_dsi_format {
 	TEGRA_DSI_FORMAT_24P,
 };
 
+/* default lp clock rate */
+#define DSI_LP_CLK_RATE			(70 * 1000 * 1000)
+
 #endif
-- 
2.0.0

[PATCH 2/2] ARM: tegra: Add lp_parent clock to dsi

[Sean Paul]

This patch adds the lp_parent clk to the dsi node for
tegra114. The TRM states that PLLP should be used
upstream of the low power dsi clock.

Signed-off-by: Sean Paul <seanpaul@chromium.org>
---
 arch/arm/boot/dts/tegra114.dtsi | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/arch/arm/boot/dts/tegra114.dtsi b/arch/arm/boot/dts/tegra114.dtsi
index 80b8edd..20f78e7 100644
--- a/arch/arm/boot/dts/tegra114.dtsi
+++ b/arch/arm/boot/dts/tegra114.dtsi
@@ -99,7 +99,8 @@
 			clocks = <&tegra_car TEGRA114_CLK_DSIA>,
 				 <&tegra_car TEGRA114_CLK_DSIALP>,
 				 <&tegra_car TEGRA114_CLK_PLL_D_OUT0>;
-			clock-names = "dsi", "lp", "parent";
+				 <&tegra_car TEGRA114_CLK_PLL_P>;
+			clock-names = "dsi", "lp", "parent", "lp_parent";
 			resets = <&tegra_car 48>;
 			reset-names = "dsi";
 			nvidia,mipi-calibrate = <&mipi 0x060>; /* DSIA & DSIB pads */
-- 
2.0.0

Re: [PATCH 1/2] drm/tegra: Set the dsi lp clk parent and rate

[Andrzej Hajda]

Hi Sean,

On 09/19/2014 09:53 PM, Sean Paul wrote:
> Per NVidia, this clock rate should be around 70MHz in
> order to properly sample reads on data lane 0. In order
> to achieve this rate, we need to reparent the clock from
> clk_m which can only achieve 12MHz. Add parent_lp to the
> dts bindings and set the parent & rate on init.
> 
> Signed-off-by: Sean Paul <seanpaul-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org>
> ---
>  .../devicetree/bindings/gpu/nvidia,tegra20-host1x.txt  | 10 ++++++++--
>  drivers/gpu/drm/tegra/dsi.c                            | 18 ++++++++++++++++++
>  drivers/gpu/drm/tegra/dsi.h                            |  3 +++
>  3 files changed, 29 insertions(+), 2 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/gpu/nvidia,tegra20-host1x.txt b/Documentation/devicetree/bindings/gpu/nvidia,tegra20-host1x.txt
> index b48f4ef..fef2918 100644
> --- a/Documentation/devicetree/bindings/gpu/nvidia,tegra20-host1x.txt
> +++ b/Documentation/devicetree/bindings/gpu/nvidia,tegra20-host1x.txt
> @@ -191,6 +191,10 @@ of the following host1x client modules:
>    - nvidia,hpd-gpio: specifies a GPIO used for hotplug detection
>    - nvidia,edid: supplies a binary EDID blob
>    - nvidia,panel: phandle of a display panel
> +  - clock-names: Can include the following entries:
> +    - lp_parent: The parent clock for lp
> +  - clocks: Must contain an entry for each optional entry in clock-names.

Have you looked at assigned-clocks, assigned-clock-parents,
assigned-clock-rates common clock properties, they seems to fit to this
scenario.

Regards
Andrzej


> +    See ../clocks/clock-bindings.txt for details.
>  
>  - sor: serial output resource
>  
> @@ -360,8 +364,10 @@ Example:
>  			compatible = "nvidia,tegra20-dsi";
>  			reg = <0x54300000 0x00040000>;
>  			clocks = <&tegra_car TEGRA20_CLK_DSI>,
> -				 <&tegra_car TEGRA20_CLK_PLL_D_OUT0>;
> -			clock-names = "dsi", "parent";
> +				 <&tegra_car TEGRA124_CLK_DSIALP>,
> +				 <&tegra_car TEGRA20_CLK_PLL_D_OUT0>,
> +				 <&tegra_car TEGRA124_CLK_PLL_P>;
> +			clock-names = "dsi", "lp", "parent", "lp_parent";
>  			resets = <&tegra_car 48>;
>  			reset-names = "dsi";
>  			status = "disabled";
> diff --git a/drivers/gpu/drm/tegra/dsi.c b/drivers/gpu/drm/tegra/dsi.c
> index f787445..c0258ae 100644
> --- a/drivers/gpu/drm/tegra/dsi.c
> +++ b/drivers/gpu/drm/tegra/dsi.c
> @@ -837,6 +837,7 @@ static int tegra_dsi_probe(struct platform_device *pdev)
>  	struct tegra_dsi *dsi;
>  	struct resource *regs;
>  	int err;
> +	struct clk *lp_parent;
>  
>  	dsi = devm_kzalloc(&pdev->dev, sizeof(*dsi), GFP_KERNEL);
>  	if (!dsi)
> @@ -879,6 +880,23 @@ static int tegra_dsi_probe(struct platform_device *pdev)
>  		return PTR_ERR(dsi->clk_lp);
>  	}
>  
> +	lp_parent = devm_clk_get(&pdev->dev, "lp_parent");
> +	if (!IS_ERR(lp_parent)) {
> +		err = clk_set_parent(dsi->clk_lp, lp_parent);
> +		if (err < 0) {
> +			dev_err(&pdev->dev, "cannot set lp clock parent\n");
> +			return err;
> +		}
> +	} else {
> +		dev_info(&pdev->dev, "no lp clock parent, using hw default\n");
> +	}
> +
> +	err = clk_set_rate(dsi->clk_lp, DSI_LP_CLK_RATE);
> +	if (err < 0) {
> +		dev_err(&pdev->dev, "cannot set low-power clock rate\n");
> +		return err;
> +	}
> +
>  	err = clk_prepare_enable(dsi->clk_lp);
>  	if (err < 0) {
>  		dev_err(&pdev->dev, "cannot enable low-power clock\n");
> diff --git a/drivers/gpu/drm/tegra/dsi.h b/drivers/gpu/drm/tegra/dsi.h
> index 5ce610d..a332caf 100644
> --- a/drivers/gpu/drm/tegra/dsi.h
> +++ b/drivers/gpu/drm/tegra/dsi.h
> @@ -127,4 +127,7 @@ enum tegra_dsi_format {
>  	TEGRA_DSI_FORMAT_24P,
>  };
>  
> +/* default lp clock rate */
> +#define DSI_LP_CLK_RATE			(70 * 1000 * 1000)
> +
>  #endif
> 

Re: [PATCH 1/2] drm/tegra: Set the dsi lp clk parent and rate

[Lucas Stach]

Am Freitag, den 19.09.2014, 15:53 -0400 schrieb Sean Paul:
> Per NVidia, this clock rate should be around 70MHz in
> order to properly sample reads on data lane 0. In order
> to achieve this rate, we need to reparent the clock from
> clk_m which can only achieve 12MHz. Add parent_lp to the
> dts bindings and set the parent & rate on init.
> 
> Signed-off-by: Sean Paul <seanpaul-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org>

NACK

You are pushing SoC integration details into the binding of the device.

You have two reasonable routes to go here: either the clock driver needs
to be made smarter to reparent the clock in case the required clock rate
could not be achieved with the current parent or you go the easy route
and reparent the clock as part of the initial configuration.

Regards,
Lucas

> ---
>  .../devicetree/bindings/gpu/nvidia,tegra20-host1x.txt  | 10 ++++++++--
>  drivers/gpu/drm/tegra/dsi.c                            | 18 ++++++++++++++++++
>  drivers/gpu/drm/tegra/dsi.h                            |  3 +++
>  3 files changed, 29 insertions(+), 2 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/gpu/nvidia,tegra20-host1x.txt b/Documentation/devicetree/bindings/gpu/nvidia,tegra20-host1x.txt
> index b48f4ef..fef2918 100644
> --- a/Documentation/devicetree/bindings/gpu/nvidia,tegra20-host1x.txt
> +++ b/Documentation/devicetree/bindings/gpu/nvidia,tegra20-host1x.txt
> @@ -191,6 +191,10 @@ of the following host1x client modules:
>    - nvidia,hpd-gpio: specifies a GPIO used for hotplug detection
>    - nvidia,edid: supplies a binary EDID blob
>    - nvidia,panel: phandle of a display panel
> +  - clock-names: Can include the following entries:
> +    - lp_parent: The parent clock for lp
> +  - clocks: Must contain an entry for each optional entry in clock-names.
> +    See ../clocks/clock-bindings.txt for details.
>  
>  - sor: serial output resource
>  
> @@ -360,8 +364,10 @@ Example:
>  			compatible = "nvidia,tegra20-dsi";
>  			reg = <0x54300000 0x00040000>;
>  			clocks = <&tegra_car TEGRA20_CLK_DSI>,
> -				 <&tegra_car TEGRA20_CLK_PLL_D_OUT0>;
> -			clock-names = "dsi", "parent";
> +				 <&tegra_car TEGRA124_CLK_DSIALP>,
> +				 <&tegra_car TEGRA20_CLK_PLL_D_OUT0>,
> +				 <&tegra_car TEGRA124_CLK_PLL_P>;
> +			clock-names = "dsi", "lp", "parent", "lp_parent";
>  			resets = <&tegra_car 48>;
>  			reset-names = "dsi";
>  			status = "disabled";
> diff --git a/drivers/gpu/drm/tegra/dsi.c b/drivers/gpu/drm/tegra/dsi.c
> index f787445..c0258ae 100644
> --- a/drivers/gpu/drm/tegra/dsi.c
> +++ b/drivers/gpu/drm/tegra/dsi.c
> @@ -837,6 +837,7 @@ static int tegra_dsi_probe(struct platform_device *pdev)
>  	struct tegra_dsi *dsi;
>  	struct resource *regs;
>  	int err;
> +	struct clk *lp_parent;
>  
>  	dsi = devm_kzalloc(&pdev->dev, sizeof(*dsi), GFP_KERNEL);
>  	if (!dsi)
> @@ -879,6 +880,23 @@ static int tegra_dsi_probe(struct platform_device *pdev)
>  		return PTR_ERR(dsi->clk_lp);
>  	}
>  
> +	lp_parent = devm_clk_get(&pdev->dev, "lp_parent");
> +	if (!IS_ERR(lp_parent)) {
> +		err = clk_set_parent(dsi->clk_lp, lp_parent);
> +		if (err < 0) {
> +			dev_err(&pdev->dev, "cannot set lp clock parent\n");
> +			return err;
> +		}
> +	} else {
> +		dev_info(&pdev->dev, "no lp clock parent, using hw default\n");
> +	}
> +
> +	err = clk_set_rate(dsi->clk_lp, DSI_LP_CLK_RATE);
> +	if (err < 0) {
> +		dev_err(&pdev->dev, "cannot set low-power clock rate\n");
> +		return err;
> +	}
> +
>  	err = clk_prepare_enable(dsi->clk_lp);
>  	if (err < 0) {
>  		dev_err(&pdev->dev, "cannot enable low-power clock\n");
> diff --git a/drivers/gpu/drm/tegra/dsi.h b/drivers/gpu/drm/tegra/dsi.h
> index 5ce610d..a332caf 100644
> --- a/drivers/gpu/drm/tegra/dsi.h
> +++ b/drivers/gpu/drm/tegra/dsi.h
> @@ -127,4 +127,7 @@ enum tegra_dsi_format {
>  	TEGRA_DSI_FORMAT_24P,
>  };
>  
> +/* default lp clock rate */
> +#define DSI_LP_CLK_RATE			(70 * 1000 * 1000)
> +
>  #endif

-- 
Pengutronix e.K.             | Lucas Stach                 |
Industrial Linux Solutions   | http://www.pengutronix.de/  |

Re: [PATCH 1/2] drm/tegra: Set the dsi lp clk parent and rate

[Thierry Reding]

[-- Attachment #1.1: Type: text/plain, Size: 367 bytes --]

On Fri, Sep 19, 2014 at 03:53:48PM -0400, Sean Paul wrote:
> Per NVidia, this clock rate should be around 70MHz in
> order to properly sample reads on data lane 0.

Can you point out where you get 70 MHz from? I only see the TRM mention
72 MHz that are needed for calibration.

Also, what's the effect of doing this? Does it fix an issue that you're
seeing?

Thierry

[-- Attachment #1.2: Type: application/pgp-signature, Size: 819 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 1/2] drm/tegra: Set the dsi lp clk parent and rate

[Thierry Reding]

[-- Attachment #1.1: Type: text/plain, Size: 1370 bytes --]

On Mon, Sep 22, 2014 at 11:00:56AM +0200, Lucas Stach wrote:
> Am Freitag, den 19.09.2014, 15:53 -0400 schrieb Sean Paul:
> > Per NVidia, this clock rate should be around 70MHz in
> > order to properly sample reads on data lane 0. In order
> > to achieve this rate, we need to reparent the clock from
> > clk_m which can only achieve 12MHz. Add parent_lp to the
> > dts bindings and set the parent & rate on init.
> > 
> > Signed-off-by: Sean Paul <seanpaul@chromium.org>
> 
> NACK
> 
> You are pushing SoC integration details into the binding of the device.
> 
> You have two reasonable routes to go here: either the clock driver needs
> to be made smarter to reparent the clock in case the required clock rate
> could not be achieved with the current parent or you go the easy route
> and reparent the clock as part of the initial configuration.

Agreed. There doesn't seem to be a case where it would make sense to
have this configurable per-board. Can you achieve the same effect by
adding this to the clock initialization table?

Oh, I just see that we have this in the Tegra124 clock initialization
table:

	{TEGRA114_CLK_DSIALP, TEGRA114_CLK_PLL_P, 68000000, 0},
	{TEGRA114_CLK_DSIBLP, TEGRA114_CLK_PLL_P, 68000000, 0},

Doesn't that work for you already? If not that'd be a bug that should be
fixed in the clock driver.

Thierry

[-- Attachment #1.2: Type: application/pgp-signature, Size: 819 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 1/2] drm/tegra: Set the dsi lp clk parent and rate

[Mark Rutland]

On Fri, Sep 19, 2014 at 08:53:48PM +0100, Sean Paul wrote:
> Per NVidia, this clock rate should be around 70MHz in
> order to properly sample reads on data lane 0. In order
> to achieve this rate, we need to reparent the clock from
> clk_m which can only achieve 12MHz. Add parent_lp to the
> dts bindings and set the parent & rate on init.
> 
> Signed-off-by: Sean Paul <seanpaul-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org>
> ---
>  .../devicetree/bindings/gpu/nvidia,tegra20-host1x.txt  | 10 ++++++++--
>  drivers/gpu/drm/tegra/dsi.c                            | 18 ++++++++++++++++++
>  drivers/gpu/drm/tegra/dsi.h                            |  3 +++
>  3 files changed, 29 insertions(+), 2 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/gpu/nvidia,tegra20-host1x.txt b/Documentation/devicetree/bindings/gpu/nvidia,tegra20-host1x.txt
> index b48f4ef..fef2918 100644
> --- a/Documentation/devicetree/bindings/gpu/nvidia,tegra20-host1x.txt
> +++ b/Documentation/devicetree/bindings/gpu/nvidia,tegra20-host1x.txt
> @@ -191,6 +191,10 @@ of the following host1x client modules:
>    - nvidia,hpd-gpio: specifies a GPIO used for hotplug detection
>    - nvidia,edid: supplies a binary EDID blob
>    - nvidia,panel: phandle of a display panel
> +  - clock-names: Can include the following entries:
> +    - lp_parent: The parent clock for lp
> +  - clocks: Must contain an entry for each optional entry in clock-names.
> +    See ../clocks/clock-bindings.txt for details.

Did this driver previously acquire clocks?

What order or names did it expect if so?

>  
>  - sor: serial output resource
>  
> @@ -360,8 +364,10 @@ Example:
>  			compatible = "nvidia,tegra20-dsi";
>  			reg = <0x54300000 0x00040000>;
>  			clocks = <&tegra_car TEGRA20_CLK_DSI>,
> -				 <&tegra_car TEGRA20_CLK_PLL_D_OUT0>;
> -			clock-names = "dsi", "parent";
> +				 <&tegra_car TEGRA124_CLK_DSIALP>,
> +				 <&tegra_car TEGRA20_CLK_PLL_D_OUT0>,
> +				 <&tegra_car TEGRA124_CLK_PLL_P>;
> +			clock-names = "dsi", "lp", "parent", "lp_parent";

Please document _all_ the names you expect.

What exactly are these two new clocks?

Is this all the clocks that feed into the DSI block? Are any of these
not directly wired to the DSI block?

Why exactly do you need to reparent it to this particular clock, and why
do you need a reference here in order to do so, given it presumably
doesn't feed directly into the DSI block?

How are these clocks described w.r.t. each other at the moment?

>  			resets = <&tegra_car 48>;
>  			reset-names = "dsi";
>  			status = "disabled";
> diff --git a/drivers/gpu/drm/tegra/dsi.c b/drivers/gpu/drm/tegra/dsi.c
> index f787445..c0258ae 100644
> --- a/drivers/gpu/drm/tegra/dsi.c
> +++ b/drivers/gpu/drm/tegra/dsi.c
> @@ -837,6 +837,7 @@ static int tegra_dsi_probe(struct platform_device *pdev)
>  	struct tegra_dsi *dsi;
>  	struct resource *regs;
>  	int err;
> +	struct clk *lp_parent;
>  
>  	dsi = devm_kzalloc(&pdev->dev, sizeof(*dsi), GFP_KERNEL);
>  	if (!dsi)
> @@ -879,6 +880,23 @@ static int tegra_dsi_probe(struct platform_device *pdev)
>  		return PTR_ERR(dsi->clk_lp);
>  	}
>  
> +	lp_parent = devm_clk_get(&pdev->dev, "lp_parent");
> +	if (!IS_ERR(lp_parent)) {
> +		err = clk_set_parent(dsi->clk_lp, lp_parent);
> +		if (err < 0) {
> +			dev_err(&pdev->dev, "cannot set lp clock parent\n");
> +			return err;
> +		}
> +	} else {
> +		dev_info(&pdev->dev, "no lp clock parent, using hw default\n");
> +	}
> +
> +	err = clk_set_rate(dsi->clk_lp, DSI_LP_CLK_RATE);
> +	if (err < 0) {
> +		dev_err(&pdev->dev, "cannot set low-power clock rate\n");
> +		return err;
> +	}

This looks like a change of behaviour given the "lp" clock wasn't
required originally.

Mark.

> +
>  	err = clk_prepare_enable(dsi->clk_lp);
>  	if (err < 0) {
>  		dev_err(&pdev->dev, "cannot enable low-power clock\n");
> diff --git a/drivers/gpu/drm/tegra/dsi.h b/drivers/gpu/drm/tegra/dsi.h
> index 5ce610d..a332caf 100644
> --- a/drivers/gpu/drm/tegra/dsi.h
> +++ b/drivers/gpu/drm/tegra/dsi.h
> @@ -127,4 +127,7 @@ enum tegra_dsi_format {
>  	TEGRA_DSI_FORMAT_24P,
>  };
>  
> +/* default lp clock rate */
> +#define DSI_LP_CLK_RATE			(70 * 1000 * 1000)
> +
>  #endif
> -- 
> 2.0.0
> 
> --
> To unsubscribe from this list: send the line "unsubscribe devicetree" in
> the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

Re: [PATCH 1/2] drm/tegra: Set the dsi lp clk parent and rate

[Thierry Reding]

[-- Attachment #1.1: Type: text/plain, Size: 3180 bytes --]

On Mon, Sep 22, 2014 at 06:46:52PM +0100, Mark Rutland wrote:
> On Fri, Sep 19, 2014 at 08:53:48PM +0100, Sean Paul wrote:
> > Per NVidia, this clock rate should be around 70MHz in
> > order to properly sample reads on data lane 0. In order
> > to achieve this rate, we need to reparent the clock from
> > clk_m which can only achieve 12MHz. Add parent_lp to the
> > dts bindings and set the parent & rate on init.
> > 
> > Signed-off-by: Sean Paul <seanpaul@chromium.org>
> > ---
> >  .../devicetree/bindings/gpu/nvidia,tegra20-host1x.txt  | 10 ++++++++--
> >  drivers/gpu/drm/tegra/dsi.c                            | 18 ++++++++++++++++++
> >  drivers/gpu/drm/tegra/dsi.h                            |  3 +++
> >  3 files changed, 29 insertions(+), 2 deletions(-)
> > 
> > diff --git a/Documentation/devicetree/bindings/gpu/nvidia,tegra20-host1x.txt b/Documentation/devicetree/bindings/gpu/nvidia,tegra20-host1x.txt
> > index b48f4ef..fef2918 100644
> > --- a/Documentation/devicetree/bindings/gpu/nvidia,tegra20-host1x.txt
> > +++ b/Documentation/devicetree/bindings/gpu/nvidia,tegra20-host1x.txt
> > @@ -191,6 +191,10 @@ of the following host1x client modules:
> >    - nvidia,hpd-gpio: specifies a GPIO used for hotplug detection
> >    - nvidia,edid: supplies a binary EDID blob
> >    - nvidia,panel: phandle of a display panel
> > +  - clock-names: Can include the following entries:
> > +    - lp_parent: The parent clock for lp
> > +  - clocks: Must contain an entry for each optional entry in clock-names.
> > +    See ../clocks/clock-bindings.txt for details.
> 
> Did this driver previously acquire clocks?
> 
> What order or names did it expect if so?

This is badly placed. There are clocks and clock-names properties in a
"Required properties" section above this hunk which lists all the clocks
that this module uses. Presumably this was added to the optional section
because it isn't always needed.

> >  - sor: serial output resource
> >  
> > @@ -360,8 +364,10 @@ Example:
> >  			compatible = "nvidia,tegra20-dsi";
> >  			reg = <0x54300000 0x00040000>;
> >  			clocks = <&tegra_car TEGRA20_CLK_DSI>,
> > -				 <&tegra_car TEGRA20_CLK_PLL_D_OUT0>;
> > -			clock-names = "dsi", "parent";
> > +				 <&tegra_car TEGRA124_CLK_DSIALP>,
> > +				 <&tegra_car TEGRA20_CLK_PLL_D_OUT0>,
> > +				 <&tegra_car TEGRA124_CLK_PLL_P>;
> > +			clock-names = "dsi", "lp", "parent", "lp_parent";
> 
> Please document _all_ the names you expect.
> 
> What exactly are these two new clocks?

"lp" isn't actually new, it's just missing from the example.

> Is this all the clocks that feed into the DSI block? Are any of these
> not directly wired to the DSI block?
> 
> Why exactly do you need to reparent it to this particular clock, and why
> do you need a reference here in order to do so, given it presumably
> doesn't feed directly into the DSI block?

It seems like the hardware default is to use a parent clock unsuitable
for the DSI low-power mode, but as discussed elsewhere this should be
fixed in the clock driver where we already have a way to statically set
the parent clock at boot time.

Thierry

[-- Attachment #1.2: Type: application/pgp-signature, Size: 819 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 1/2] drm/tegra: Set the dsi lp clk parent and rate

[Mike Turquette]

Quoting Thierry Reding (2014-09-23 00:22:05)
> On Mon, Sep 22, 2014 at 06:46:52PM +0100, Mark Rutland wrote:
> > On Fri, Sep 19, 2014 at 08:53:48PM +0100, Sean Paul wrote:
> > > Per NVidia, this clock rate should be around 70MHz in
> > > order to properly sample reads on data lane 0. In order
> > > to achieve this rate, we need to reparent the clock from
> > > clk_m which can only achieve 12MHz. Add parent_lp to the
> > > dts bindings and set the parent & rate on init.
> > > 
> > > Signed-off-by: Sean Paul <seanpaul-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org>
> > > ---
> > >  .../devicetree/bindings/gpu/nvidia,tegra20-host1x.txt  | 10 ++++++++--
> > >  drivers/gpu/drm/tegra/dsi.c                            | 18 ++++++++++++++++++
> > >  drivers/gpu/drm/tegra/dsi.h                            |  3 +++
> > >  3 files changed, 29 insertions(+), 2 deletions(-)
> > > 
> > > diff --git a/Documentation/devicetree/bindings/gpu/nvidia,tegra20-host1x.txt b/Documentation/devicetree/bindings/gpu/nvidia,tegra20-host1x.txt
> > > index b48f4ef..fef2918 100644
> > > --- a/Documentation/devicetree/bindings/gpu/nvidia,tegra20-host1x.txt
> > > +++ b/Documentation/devicetree/bindings/gpu/nvidia,tegra20-host1x.txt
> > > @@ -191,6 +191,10 @@ of the following host1x client modules:
> > >    - nvidia,hpd-gpio: specifies a GPIO used for hotplug detection
> > >    - nvidia,edid: supplies a binary EDID blob
> > >    - nvidia,panel: phandle of a display panel
> > > +  - clock-names: Can include the following entries:
> > > +    - lp_parent: The parent clock for lp
> > > +  - clocks: Must contain an entry for each optional entry in clock-names.
> > > +    See ../clocks/clock-bindings.txt for details.
> > 
> > Did this driver previously acquire clocks?
> > 
> > What order or names did it expect if so?
> 
> This is badly placed. There are clocks and clock-names properties in a
> "Required properties" section above this hunk which lists all the clocks
> that this module uses. Presumably this was added to the optional section
> because it isn't always needed.
> 
> > >  - sor: serial output resource
> > >  
> > > @@ -360,8 +364,10 @@ Example:
> > >                     compatible = "nvidia,tegra20-dsi";
> > >                     reg = <0x54300000 0x00040000>;
> > >                     clocks = <&tegra_car TEGRA20_CLK_DSI>,
> > > -                            <&tegra_car TEGRA20_CLK_PLL_D_OUT0>;
> > > -                   clock-names = "dsi", "parent";
> > > +                            <&tegra_car TEGRA124_CLK_DSIALP>,
> > > +                            <&tegra_car TEGRA20_CLK_PLL_D_OUT0>,
> > > +                            <&tegra_car TEGRA124_CLK_PLL_P>;
> > > +                   clock-names = "dsi", "lp", "parent", "lp_parent";
> > 
> > Please document _all_ the names you expect.
> > 
> > What exactly are these two new clocks?
> 
> "lp" isn't actually new, it's just missing from the example.
> 
> > Is this all the clocks that feed into the DSI block? Are any of these
> > not directly wired to the DSI block?
> > 
> > Why exactly do you need to reparent it to this particular clock, and why
> > do you need a reference here in order to do so, given it presumably
> > doesn't feed directly into the DSI block?
> 
> It seems like the hardware default is to use a parent clock unsuitable
> for the DSI low-power mode, but as discussed elsewhere this should be
> fixed in the clock driver where we already have a way to statically set
> the parent clock at boot time.

The driver fix is perfectly fine, but we also have the
assigned-clock-parent stuff now if you prefer to manage it from DT.

See,

Documentation/devicetree/bindings/clock/clock-bindings.txt
drivers/clk/clk-conf.c

Regards,
Mike

> 
> Thierry

Re: [PATCH 1/2] drm/tegra: Set the dsi lp clk parent and rate

[Thierry Reding]

[-- Attachment #1.1: Type: text/plain, Size: 4500 bytes --]

On Sat, Sep 27, 2014 at 01:05:32PM -0700, Mike Turquette wrote:
> Quoting Thierry Reding (2014-09-23 00:22:05)
> > On Mon, Sep 22, 2014 at 06:46:52PM +0100, Mark Rutland wrote:
> > > On Fri, Sep 19, 2014 at 08:53:48PM +0100, Sean Paul wrote:
> > > > Per NVidia, this clock rate should be around 70MHz in
> > > > order to properly sample reads on data lane 0. In order
> > > > to achieve this rate, we need to reparent the clock from
> > > > clk_m which can only achieve 12MHz. Add parent_lp to the
> > > > dts bindings and set the parent & rate on init.
> > > > 
> > > > Signed-off-by: Sean Paul <seanpaul@chromium.org>
> > > > ---
> > > >  .../devicetree/bindings/gpu/nvidia,tegra20-host1x.txt  | 10 ++++++++--
> > > >  drivers/gpu/drm/tegra/dsi.c                            | 18 ++++++++++++++++++
> > > >  drivers/gpu/drm/tegra/dsi.h                            |  3 +++
> > > >  3 files changed, 29 insertions(+), 2 deletions(-)
> > > > 
> > > > diff --git a/Documentation/devicetree/bindings/gpu/nvidia,tegra20-host1x.txt b/Documentation/devicetree/bindings/gpu/nvidia,tegra20-host1x.txt
> > > > index b48f4ef..fef2918 100644
> > > > --- a/Documentation/devicetree/bindings/gpu/nvidia,tegra20-host1x.txt
> > > > +++ b/Documentation/devicetree/bindings/gpu/nvidia,tegra20-host1x.txt
> > > > @@ -191,6 +191,10 @@ of the following host1x client modules:
> > > >    - nvidia,hpd-gpio: specifies a GPIO used for hotplug detection
> > > >    - nvidia,edid: supplies a binary EDID blob
> > > >    - nvidia,panel: phandle of a display panel
> > > > +  - clock-names: Can include the following entries:
> > > > +    - lp_parent: The parent clock for lp
> > > > +  - clocks: Must contain an entry for each optional entry in clock-names.
> > > > +    See ../clocks/clock-bindings.txt for details.
> > > 
> > > Did this driver previously acquire clocks?
> > > 
> > > What order or names did it expect if so?
> > 
> > This is badly placed. There are clocks and clock-names properties in a
> > "Required properties" section above this hunk which lists all the clocks
> > that this module uses. Presumably this was added to the optional section
> > because it isn't always needed.
> > 
> > > >  - sor: serial output resource
> > > >  
> > > > @@ -360,8 +364,10 @@ Example:
> > > >                     compatible = "nvidia,tegra20-dsi";
> > > >                     reg = <0x54300000 0x00040000>;
> > > >                     clocks = <&tegra_car TEGRA20_CLK_DSI>,
> > > > -                            <&tegra_car TEGRA20_CLK_PLL_D_OUT0>;
> > > > -                   clock-names = "dsi", "parent";
> > > > +                            <&tegra_car TEGRA124_CLK_DSIALP>,
> > > > +                            <&tegra_car TEGRA20_CLK_PLL_D_OUT0>,
> > > > +                            <&tegra_car TEGRA124_CLK_PLL_P>;
> > > > +                   clock-names = "dsi", "lp", "parent", "lp_parent";
> > > 
> > > Please document _all_ the names you expect.
> > > 
> > > What exactly are these two new clocks?
> > 
> > "lp" isn't actually new, it's just missing from the example.
> > 
> > > Is this all the clocks that feed into the DSI block? Are any of these
> > > not directly wired to the DSI block?
> > > 
> > > Why exactly do you need to reparent it to this particular clock, and why
> > > do you need a reference here in order to do so, given it presumably
> > > doesn't feed directly into the DSI block?
> > 
> > It seems like the hardware default is to use a parent clock unsuitable
> > for the DSI low-power mode, but as discussed elsewhere this should be
> > fixed in the clock driver where we already have a way to statically set
> > the parent clock at boot time.
> 
> The driver fix is perfectly fine, but we also have the
> assigned-clock-parent stuff now if you prefer to manage it from DT.

I don't think this should be done in the driver. It should never have to
bother with what the parent of the LP clock is. The only reason why this
is necessary is because the hardware default isn't appropriate. For the
same reason I don't think it should be encoded in DT. It's an SoC-level
detail and likely will never need to change per board, so it'll just be
redundant information in DT.

We already have code in the Tegra clock driver to set up this parent and
rate properly, so I'm not even sure why this patch is necessary or where
the disconnect comes from. Perhaps this wasn't tested against the
upstream kernel?

Thierry

[-- Attachment #1.2: Type: application/pgp-signature, Size: 819 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 1/2] drm/tegra: Set the dsi lp clk parent and rate

[Peter De Schrijver]

On Mon, Sep 22, 2014 at 12:11:54PM +0200, Thierry Reding wrote:
> * PGP Signed by an unknown key
> 
> On Mon, Sep 22, 2014 at 11:00:56AM +0200, Lucas Stach wrote:
> > Am Freitag, den 19.09.2014, 15:53 -0400 schrieb Sean Paul:
> > > Per NVidia, this clock rate should be around 70MHz in
> > > order to properly sample reads on data lane 0. In order
> > > to achieve this rate, we need to reparent the clock from
> > > clk_m which can only achieve 12MHz. Add parent_lp to the
> > > dts bindings and set the parent & rate on init.
> > > 
> > > Signed-off-by: Sean Paul <seanpaul-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org>
> > 
> > NACK
> > 
> > You are pushing SoC integration details into the binding of the device.
> > 
> > You have two reasonable routes to go here: either the clock driver needs
> > to be made smarter to reparent the clock in case the required clock rate
> > could not be achieved with the current parent or you go the easy route
> > and reparent the clock as part of the initial configuration.
> 
> Agreed. There doesn't seem to be a case where it would make sense to
> have this configurable per-board. Can you achieve the same effect by
> adding this to the clock initialization table?
> 
> Oh, I just see that we have this in the Tegra124 clock initialization
> table:
> 
> 	{TEGRA114_CLK_DSIALP, TEGRA114_CLK_PLL_P, 68000000, 0},
> 	{TEGRA114_CLK_DSIBLP, TEGRA114_CLK_PLL_P, 68000000, 0},
> 
> Doesn't that work for you already? If not that'd be a bug that should be
> fixed in the clock driver.

This seems the better approach indeed. Unless the rate would differ based
on board or other external factors.

Cheers,

Peter.

Re: [PATCH 1/2] drm/tegra: Set the dsi lp clk parent and rate

[Sean Paul]

On Wed, Oct 8, 2014 at 8:11 AM, Peter De Schrijver
<pdeschrijver-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org> wrote:
> On Mon, Sep 22, 2014 at 12:11:54PM +0200, Thierry Reding wrote:
>> * PGP Signed by an unknown key
>>
>> On Mon, Sep 22, 2014 at 11:00:56AM +0200, Lucas Stach wrote:
>> > Am Freitag, den 19.09.2014, 15:53 -0400 schrieb Sean Paul:
>> > > Per NVidia, this clock rate should be around 70MHz in
>> > > order to properly sample reads on data lane 0. In order
>> > > to achieve this rate, we need to reparent the clock from
>> > > clk_m which can only achieve 12MHz. Add parent_lp to the
>> > > dts bindings and set the parent & rate on init.
>> > >
>> > > Signed-off-by: Sean Paul <seanpaul-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org>
>> >
>> > NACK
>> >
>> > You are pushing SoC integration details into the binding of the device.
>> >
>> > You have two reasonable routes to go here: either the clock driver needs
>> > to be made smarter to reparent the clock in case the required clock rate
>> > could not be achieved with the current parent or you go the easy route
>> > and reparent the clock as part of the initial configuration.
>>
>> Agreed. There doesn't seem to be a case where it would make sense to
>> have this configurable per-board. Can you achieve the same effect by
>> adding this to the clock initialization table?
>>
>> Oh, I just see that we have this in the Tegra124 clock initialization
>> table:
>>
>>       {TEGRA114_CLK_DSIALP, TEGRA114_CLK_PLL_P, 68000000, 0},
>>       {TEGRA114_CLK_DSIBLP, TEGRA114_CLK_PLL_P, 68000000, 0},
>>
>> Doesn't that work for you already? If not that'd be a bug that should be
>> fixed in the clock driver.
>
> This seems the better approach indeed. Unless the rate would differ based
> on board or other external factors.
>

Thanks, Peter.

Just to close the loop, the patch: "clk: tegra124: Add init data for
dsi lp clocks" I sent up last week replaces this one.

Sean


> Cheers,
>
> Peter.