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@xxxxxxxxxx>
>>>>
>>>> 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@xxxxxxxxxx>
>>>> ---
>>>> 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
