Skip to contentSkip to conversation

drm/tegra: hub: Enable all required clocks

PATCH In review Thierry Reding 2 participants 4 messages Nov 29, 2018

Subsystems (3)
lore
Share & tools
lore
#0 lore raw

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

From: Thierry Reding <[email protected]>

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.

---
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(-)
drivers/gpu/drm/tegra/hub.c +46 -2
index 6112d9042979..922a48d5a483 100644
@@ -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;
+		}
+
+		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);
drivers/gpu/drm/tegra/hub.h +3 -0
index 6696a85fc1f2..479087c0705a 100644
@@ -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;
 };
Signature · 6 lines
2.19.1

_______________________________________________
dri-devel mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/dri-devel
#1 lore raw

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

From: Thierry Reding <[email protected]>

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.

---
 drivers/gpu/drm/tegra/dc.c | 38 ++++++++++++++++++++++----------------
 1 file changed, 22 insertions(+), 16 deletions(-)
drivers/gpu/drm/tegra/dc.c +22 -16
index f80e82e16475..607a6ea17ecc 100644
@@ -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);
Signature · 6 lines
2.19.1

_______________________________________________
dri-devel mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/dri-devel
#2 lore raw
In reply to Thierry Reding
Quoted 43 lines · Thierry Reding

From: Thierry Reding <[email protected]>

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.

---
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(-)
drivers/gpu/drm/tegra/hub.c +6 -0
index 6112d9042979..922a48d5a483 100644
@@ -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);

drivers/gpu/drm/tegra/hub.c static int tegra_display_hub_probe(struct platform_device *pdev) · line 819

Quoted 10 lines
drivers/gpu/drm/tegra/hub.c +10 -0
+	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.

Quoted 73 lines
drivers/gpu/drm/tegra/hub.c +30 -2
+
+		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);
drivers/gpu/drm/tegra/hub.h +0 -0
index 6696a85fc1f2..479087c0705a 100644

drivers/gpu/drm/tegra/hub.h struct tegra_display_hub { · line 57

Quoted 9 lines
drivers/gpu/drm/tegra/hub.h +3 -0
@@ -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
[email protected]
https://lists.freedesktop.org/mailman/listinfo/dri-devel

#3 lore raw
In reply to Thierry Reding
Quoted 8 lines · Thierry Reding

From: Thierry Reding <[email protected]>

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.

Quoted 9 lines

Implement this logic within the display hub driver to ensure the clocks are always enabled at the right time.

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

drivers/gpu/drm/tegra/hub.c +32 -1
index 6112d9042979..d597de7d0de4 100644
@@ -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:
drivers/gpu/drm/tegra/hub.h +3 -0
index 6696a85fc1f2..40866765ef0d 100644
@@ -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;
 };

_______________________________________________
dri-devel mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/dri-devel