Re: [PATCH 1/2] drm/tegra: hub: Enable all required clocks

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

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

On Thu, Nov 29, 2018 at 07:01:52PM +0300, Dmitry Osipenko wrote:
> On 28.11.2018 19:44, Thierry Reding wrote:
> > From: Thierry Reding <treding@nvidia.com>
> > 
> > The display architecture on Tegra186 and Tegra194 requires that there be
> > some valid clock on all domains before accessing any display register. A
> > further requirement is that in addition to the host1x, hub, disp and dsc
> > clocks, all the head clocks (pclk0-2 on Tegra186 or pclk0-3 on Tegra194)
> > must also be enabled.
> > 
> > Implement this logic within the display hub driver to ensure the clocks
> > are always enabled at the right time.
> > 
> > Signed-off-by: Thierry Reding <treding@nvidia.com>
> > ---
> >  drivers/gpu/drm/tegra/hub.c | 47 +++++++++++++++++++++++++++++++++++--
> >  drivers/gpu/drm/tegra/hub.h |  3 +++
> >  2 files changed, 48 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/tegra/hub.c b/drivers/gpu/drm/tegra/hub.c
> > index 6112d9042979..7e7742877b90 100644
> > --- a/drivers/gpu/drm/tegra/hub.c
> > +++ b/drivers/gpu/drm/tegra/hub.c
> > @@ -742,7 +742,9 @@ static const struct host1x_client_ops tegra_display_hub_ops = {
> >  
> >  static int tegra_display_hub_probe(struct platform_device *pdev)
> >  {
> > +	struct device_node *child = NULL;
> >  	struct tegra_display_hub *hub;
> > +	struct clk *clk;
> >  	unsigned int i;
> >  	int err;
> >  
> > @@ -801,6 +803,33 @@ static int tegra_display_hub_probe(struct platform_device *pdev)
> >  			return err;
> >  	}
> >  
> > +	hub->num_heads = of_get_child_count(pdev->dev.of_node);
> > +
> > +	hub->clk_heads = devm_kcalloc(&pdev->dev, hub->num_heads, sizeof(clk),
> > +				      GFP_KERNEL);
> > +	if (!hub->clk_heads)
> > +		return -ENOMEM;
> > +
> > +	for (i = 0; i < hub->num_heads; i++) {
> > +		child = of_get_next_child(pdev->dev.of_node, child);
> > +		if (!child) {
> > +			dev_err(&pdev->dev, "failed to find node for head %u\n",
> > +				i);
> > +			return -ENODEV;
> > +		}
> > +
> > +		clk = devm_get_clk_from_child(&pdev->dev, child, "dc");
> > +		if (IS_ERR(clk)) {
> > +			dev_err(&pdev->dev, "failed to get clock for head %u\n",
> > +				i);
> 
> Looks like "of_node_put(child);" missed here.

Indeed, good catch!

> > +			return PTR_ERR(clk);
> > +		}
> > +
> > +		hub->clk_heads[i] = clk;
> > +	}
> > +
> > +	of_node_put(child);
> > +
> >  	/* XXX: enable clock across reset? */
> >  	err = reset_control_assert(hub->rst);
> >  	if (err < 0)
> > @@ -840,12 +869,16 @@ static int tegra_display_hub_remove(struct platform_device *pdev)
> >  static int __maybe_unused tegra_display_hub_suspend(struct device *dev)
> >  {
> >  	struct tegra_display_hub *hub = dev_get_drvdata(dev);
> > +	unsigned int i;
> >  	int err;
> >  
> >  	err = reset_control_assert(hub->rst);
> >  	if (err < 0)
> >  		return err;
> >  
> > +	for (i = 0; i < hub->num_heads; i++)
> > +		clk_disable_unprepare(hub->clk_heads[i]);
> > +
> >  	clk_disable_unprepare(hub->clk_hub);
> >  	clk_disable_unprepare(hub->clk_dsc);
> >  	clk_disable_unprepare(hub->clk_disp);
> > @@ -856,6 +889,7 @@ static int __maybe_unused tegra_display_hub_suspend(struct device *dev)
> >  static int __maybe_unused tegra_display_hub_resume(struct device *dev)
> >  {
> >  	struct tegra_display_hub *hub = dev_get_drvdata(dev);
> > +	unsigned int i;
> >  	int err;
> >  
> >  	err = clk_prepare_enable(hub->clk_disp);
> > @@ -870,13 +904,22 @@ static int __maybe_unused tegra_display_hub_resume(struct device *dev)
> >  	if (err < 0)
> >  		goto disable_dsc;
> >  
> > +	for (i = 0; i < hub->num_heads; i++) {
> > +		err = clk_prepare_enable(hub->clk_heads[i]);
> > +		if (err < 0)
> > +			goto disable_heads;
> > +	}
> 
> What about to use clk_bulk_* API?

Can't do that because we get these clocks from the child nodes, which is
something that we can't do with clk_bulk_*.

What I could do, though, is to call clk_disable_unprepare() in reverse
order to more closely mirror what clk_bulk_* does, which is obviously
the right thing in terms of symmetry.

Thierry

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

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

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