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