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

[Thierry Reding <thierry.reding@gmail.com>]

[-- 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