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

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>
---
Changes in v2:
- reverse order in which clocks are disabled for symmetry
- don't leak child device tree node

 drivers/gpu/drm/tegra/hub.c | 48 +++++++++++++++++++++++++++++++++++--
 drivers/gpu/drm/tegra/hub.h |  3 +++
 2 files changed, 49 insertions(+), 2 deletions(-)

On 29.11.2018 19:24, 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>
> ---
> Changes in v2:
> - reverse order in which clocks are disabled for symmetry
> - don't leak child device tree node
> 
>  drivers/gpu/drm/tegra/hub.c | 48 +++++++++++++++++++++++++++++++++++--
>  drivers/gpu/drm/tegra/hub.h |  3 +++
>  2 files changed, 49 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/tegra/hub.c b/drivers/gpu/drm/tegra/hub.c
> index 6112d9042979..922a48d5a483 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,34 @@ 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;
> +		}

I'm not sure that it's really worth to check that child is NULL here, we already know the exact number of children nodes. It could be omitted safely.

> +
> +		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);
> +			of_node_put(child);
> +			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 +870,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 = hub->num_heads;
>  	int err;
>  
>  	err = reset_control_assert(hub->rst);
>  	if (err < 0)
>  		return err;
>  
> +	while (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 +890,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 +905,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;
>  };
>

On 29.11.2018 19:24, 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>
> ---
> Changes in v2:
> - reverse order in which clocks are disabled for symmetry
> - don't leak child device tree node
> 

And here is a variant that uses bulk-API, it looks nicer to me. Don't you think so?

diff --git a/drivers/gpu/drm/tegra/hub.c b/drivers/gpu/drm/tegra/hub.c
index 6112d9042979..d597de7d0de4 100644
--- a/drivers/gpu/drm/tegra/hub.c
+++ b/drivers/gpu/drm/tegra/hub.c
@@ -742,6 +742,7 @@ 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;
 	unsigned int i;
 	int err;
@@ -801,6 +802,29 @@ 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(*hub->clk_heads), 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);
+
+		hub->clk_heads[i].id = "dc";
+		hub->clk_heads[i].clk = devm_get_clk_from_child(&pdev->dev,
+								child, "dc");
+		if (IS_ERR(hub->clk_heads[i].clk)) {
+			dev_err(&pdev->dev, "failed to get clock for head %u\n",
+				i);
+			of_node_put(child);
+			return PTR_ERR(hub->clk_heads[i].clk);
+		}
+	}
+
+	of_node_put(child);
+
 	/* XXX: enable clock across reset? */
 	err = reset_control_assert(hub->rst);
 	if (err < 0)
@@ -846,6 +870,7 @@ static int __maybe_unused tegra_display_hub_suspend(struct device *dev)
 	if (err < 0)
 		return err;
 
+	clk_bulk_disable_unprepare(hub->num_heads, hub->clk_heads);
 	clk_disable_unprepare(hub->clk_hub);
 	clk_disable_unprepare(hub->clk_dsc);
 	clk_disable_unprepare(hub->clk_disp);
@@ -870,12 +895,18 @@ static int __maybe_unused tegra_display_hub_resume(struct device *dev)
 	if (err < 0)
 		goto disable_dsc;
 
-	err = reset_control_deassert(hub->rst);
+	err = clk_bulk_prepare_enable(hub->num_heads, hub->clk_heads);
 	if (err < 0)
 		goto disable_hub;
 
+	err = reset_control_deassert(hub->rst);
+	if (err < 0)
+		goto disable_heads;
+
 	return 0;
 
+disable_heads:
+	clk_bulk_disable_unprepare(hub->num_heads, hub->clk_heads);
 disable_hub:
 	clk_disable_unprepare(hub->clk_hub);
 disable_dsc:
diff --git a/drivers/gpu/drm/tegra/hub.h b/drivers/gpu/drm/tegra/hub.h
index 6696a85fc1f2..40866765ef0d 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_bulk_data *clk_heads;
+
 	const struct tegra_display_hub_soc *soc;
 	struct tegra_windowgroup *wgrps;
 };