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

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

[Thierry Reding]

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);
+			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;
+	}
+
 	err = reset_control_deassert(hub->rst);
 	if (err < 0)
-		goto disable_hub;
+		goto disable_heads;
 
 	return 0;
 
-disable_hub:
+disable_heads:
+	while (i--)
+		clk_disable_unprepare(hub->clk_heads[i]);
+
 	clk_disable_unprepare(hub->clk_hub);
 disable_dsc:
 	clk_disable_unprepare(hub->clk_dsc);
diff --git a/drivers/gpu/drm/tegra/hub.h b/drivers/gpu/drm/tegra/hub.h
index 6696a85fc1f2..479087c0705a 100644
--- a/drivers/gpu/drm/tegra/hub.h
+++ b/drivers/gpu/drm/tegra/hub.h
@@ -49,6 +49,9 @@ struct tegra_display_hub {
 	struct clk *clk_hub;
 	struct reset_control *rst;
 
+	unsigned int num_heads;
+	struct clk **clk_heads;
+
 	const struct tegra_display_hub_soc *soc;
 	struct tegra_windowgroup *wgrps;
 };
-- 
2.19.1

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

[PATCH 2/2] drm/tegra: dc: Don't clean up unused display controller

[Thierry Reding]

From: Thierry Reding <treding@nvidia.com>

Display controllers that don't own any windows and which therefore
aren't currently being assigned a primary plane are not registered
as a CRTC with the DRM framework. They request neither a syncpoint
nor an interrupt because they don't use them, so avoid cleaning up
those resources.

Signed-off-by: Thierry Reding <treding@nvidia.com>
---
 drivers/gpu/drm/tegra/dc.c | 38 ++++++++++++++++++++++----------------
 1 file changed, 22 insertions(+), 16 deletions(-)

diff --git a/drivers/gpu/drm/tegra/dc.c b/drivers/gpu/drm/tegra/dc.c
index f80e82e16475..607a6ea17ecc 100644
--- a/drivers/gpu/drm/tegra/dc.c
+++ b/drivers/gpu/drm/tegra/dc.c
@@ -1978,6 +1978,23 @@ static irqreturn_t tegra_dc_irq(int irq, void *data)
 	return IRQ_HANDLED;
 }
 
+static bool tegra_dc_has_window_groups(struct tegra_dc *dc)
+{
+	unsigned int i;
+
+	if (!dc->soc->wgrps)
+		return true;
+
+	for (i = 0; i < dc->soc->num_wgrps; i++) {
+		const struct tegra_windowgroup_soc *wgrp = &dc->soc->wgrps[i];
+
+		if (wgrp->dc == dc->pipe && wgrp->num_windows > 0)
+			return true;
+	}
+
+	return false;
+}
+
 static int tegra_dc_init(struct host1x_client *client)
 {
 	struct drm_device *drm = dev_get_drvdata(client->parent);
@@ -1993,22 +2010,8 @@ static int tegra_dc_init(struct host1x_client *client)
 	 * assign a primary plane to them, which in turn will cause KMS to
 	 * crash.
 	 */
-	if (dc->soc->wgrps) {
-		bool has_wgrps = false;
-		unsigned int i;
-
-		for (i = 0; i < dc->soc->num_wgrps; i++) {
-			const struct tegra_windowgroup_soc *wgrp = &dc->soc->wgrps[i];
-
-			if (wgrp->dc == dc->pipe && wgrp->num_windows > 0) {
-				has_wgrps = true;
-				break;
-			}
-		}
-
-		if (!has_wgrps)
-			return 0;
-	}
+	if (!tegra_dc_has_window_groups(dc))
+		return 0;
 
 	dc->syncpt = host1x_syncpt_request(client, flags);
 	if (!dc->syncpt)
@@ -2094,6 +2097,9 @@ static int tegra_dc_exit(struct host1x_client *client)
 	struct tegra_dc *dc = host1x_client_to_dc(client);
 	int err;
 
+	if (!tegra_dc_has_window_groups(dc))
+		return 0;
+
 	devm_free_irq(dc->dev, dc->irq, dc);
 
 	err = tegra_dc_rgb_exit(dc);
-- 
2.19.1

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

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

[Dmitry Osipenko]

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.

> +			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?

> +
>  	err = reset_control_deassert(hub->rst);
>  	if (err < 0)
> -		goto disable_hub;
> +		goto disable_heads;
>  
>  	return 0;
>  
> -disable_hub:
> +disable_heads:
> +	while (i--)
> +		clk_disable_unprepare(hub->clk_heads[i]);
> +
>  	clk_disable_unprepare(hub->clk_hub);
>  disable_dsc:
>  	clk_disable_unprepare(hub->clk_dsc);
> diff --git a/drivers/gpu/drm/tegra/hub.h b/drivers/gpu/drm/tegra/hub.h
> index 6696a85fc1f2..479087c0705a 100644
> --- a/drivers/gpu/drm/tegra/hub.h
> +++ b/drivers/gpu/drm/tegra/hub.h
> @@ -49,6 +49,9 @@ struct tegra_display_hub {
>  	struct clk *clk_hub;
>  	struct reset_control *rst;
>  
> +	unsigned int num_heads;
> +	struct clk **clk_heads;
> +
>  	const struct tegra_display_hub_soc *soc;
>  	struct tegra_windowgroup *wgrps;
>  };
> 

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

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

[Thierry Reding]

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

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

[Dmitry Osipenko]

On 29.11.2018 19:17, Thierry Reding wrote:
> 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.

Seems there is no need to use clk_bulk_* for getting the clocks, you could build up the bulk-array manually.. somewhat like this:

struct tegra_display_hub {
	struct clk *clk_hub;
	struct reset_control *rst;

	unsigned int num_heads;
	struct clk_bulk_data *clk_heads;

	...
};

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;
	}

	hub->clk_heads[i] = 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);
		return PTR_ERR(clk);
	}
}

...

err = clk_bulk_prepare_enable(hub->num_heads, hub->clk_heads);
if (err < 0)
	goto disable_dsc;

...



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

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

[Dmitry Osipenko]

On 29.11.2018 19:28, Dmitry Osipenko wrote:
> On 29.11.2018 19:17, Thierry Reding wrote:
>> 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.
> 
> Seems there is no need to use clk_bulk_* for getting the clocks, you could build up the bulk-array manually.. somewhat like this:
> 
> struct tegra_display_hub {
> 	struct clk *clk_hub;
> 	struct reset_control *rst;
> 
> 	unsigned int num_heads;
> 	struct clk_bulk_data *clk_heads;
> 
> 	...
> };
> 
> 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;
> 	}
> 
> 	hub->clk_heads[i] = devm_get_clk_from_child(&pdev->dev, child, "dc");

hub->clk_heads[i].clk.. of course
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel