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

...