[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 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");

hub->clk_heads[i].clk.. of course