| Message ID | 1411156429-19797-1-git-send-email-seanpaul@chromium.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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@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. 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 >
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. 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
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
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
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? > > - 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@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html >
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
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. See, Documentation/devicetree/bindings/clock/clock-bindings.txt drivers/clk/clk-conf.c Regards, Mike > > Thierry
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
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@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. This seems the better approach indeed. Unless the rate would differ based on board or other external factors. Cheers, Peter.
On Wed, Oct 8, 2014 at 8:11 AM, Peter De Schrijver <pdeschrijver@nvidia.com> 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@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. > > 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.
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
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(-)