Message ID | 20181128164403.27648-1-thierry.reding@gmail.com (mailing list archive) |
---|---|
State | New, archived |
Headers | show |
Series | [1/2] drm/tegra: hub: Enable all required clocks | expand |
On 28.11.2018 19:44, 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> > --- > 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. > + 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? > + > 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 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@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> > > --- > > 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. Thierry
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@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> >>> --- >>> 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; ...
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@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> >>>> --- >>>> 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
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); + 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; + } + 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; };