| Message ID | 20181123120639.16706-1-thierry.reding@gmail.com |
|---|---|
| State | Accepted |
| Headers | show |
| Series | [1/3] drm/tegra: vic: Implement explicit reset support | expand |
On 23/11/2018 12:06, Thierry Reding wrote: > From: Thierry Reding <treding@nvidia.com> > > Tegra supports generic PM domains on 64-bit ARM, and if that is enabled, > the power domain code will make sure that resets are asserted and > deasserted at appropriate points in time. > > If generic PM domains are not implemented, such as on 32-bit Tegra, the > resets need to be asserted and deasserted explicitly by the driver. > > Signed-off-by: Thierry Reding <treding@nvidia.com> > --- > drivers/gpu/drm/tegra/vic.c | 35 ++++++++++++++++++++++++++++++++++- > 1 file changed, 34 insertions(+), 1 deletion(-) > > diff --git a/drivers/gpu/drm/tegra/vic.c b/drivers/gpu/drm/tegra/vic.c > index 9fa77405db01..23f530db45ad 100644 > --- a/drivers/gpu/drm/tegra/vic.c > +++ b/drivers/gpu/drm/tegra/vic.c > @@ -38,6 +38,7 @@ struct vic { > struct iommu_domain *domain; > struct device *dev; > struct clk *clk; > + struct reset_control *rst; > > /* Platform configuration */ > const struct vic_config *config; > @@ -56,13 +57,37 @@ static void vic_writel(struct vic *vic, u32 value, unsigned int offset) > static int vic_runtime_resume(struct device *dev) > { > struct vic *vic = dev_get_drvdata(dev); > + int err; > + > + err = clk_prepare_enable(vic->clk); > + if (err < 0) > + return err; > + > + usleep_range(2000, 4000); The Tegra genpd code has a usleep_range(10, 20), is that not sufficient here? If it is, it would be good to be consistent. > + > + err = reset_control_deassert(vic->rst); > + if (err < 0) > + goto disable; > + > + usleep_range(2000, 4000); > + > + return 0; > > - return clk_prepare_enable(vic->clk); > +disable: > + clk_disable_unprepare(vic->clk); > + return err; > } > > static int vic_runtime_suspend(struct device *dev) > { > struct vic *vic = dev_get_drvdata(dev); > + int err; > + > + err = reset_control_assert(vic->rst); > + if (err < 0) > + return err; > + > + usleep_range(2000, 4000); > > clk_disable_unprepare(vic->clk); > > @@ -324,6 +349,14 @@ static int vic_probe(struct platform_device *pdev) > return PTR_ERR(vic->clk); > } > > + if (!dev->pm_domain) { > + vic->rst = devm_reset_control_get(dev, "vic"); > + if (IS_ERR(vic->rst)) { > + dev_err(&pdev->dev, "failed to get reset\n"); > + return PTR_ERR(vic->rst); > + } > + } > + > vic->falcon.dev = dev; > vic->falcon.regs = vic->regs; > vic->falcon.ops = &vic_falcon_ops; > Cheers Jon
On Thu, Nov 29, 2018 at 01:40:32PM +0000, Jon Hunter wrote: > > On 23/11/2018 12:06, Thierry Reding wrote: > > From: Thierry Reding <treding@nvidia.com> > > > > Tegra supports generic PM domains on 64-bit ARM, and if that is enabled, > > the power domain code will make sure that resets are asserted and > > deasserted at appropriate points in time. > > > > If generic PM domains are not implemented, such as on 32-bit Tegra, the > > resets need to be asserted and deasserted explicitly by the driver. > > > > Signed-off-by: Thierry Reding <treding@nvidia.com> > > --- > > drivers/gpu/drm/tegra/vic.c | 35 ++++++++++++++++++++++++++++++++++- > > 1 file changed, 34 insertions(+), 1 deletion(-) > > > > diff --git a/drivers/gpu/drm/tegra/vic.c b/drivers/gpu/drm/tegra/vic.c > > index 9fa77405db01..23f530db45ad 100644 > > --- a/drivers/gpu/drm/tegra/vic.c > > +++ b/drivers/gpu/drm/tegra/vic.c > > @@ -38,6 +38,7 @@ struct vic { > > struct iommu_domain *domain; > > struct device *dev; > > struct clk *clk; > > + struct reset_control *rst; > > > > /* Platform configuration */ > > const struct vic_config *config; > > @@ -56,13 +57,37 @@ static void vic_writel(struct vic *vic, u32 value, unsigned int offset) > > static int vic_runtime_resume(struct device *dev) > > { > > struct vic *vic = dev_get_drvdata(dev); > > + int err; > > + > > + err = clk_prepare_enable(vic->clk); > > + if (err < 0) > > + return err; > > + > > + usleep_range(2000, 4000); > > The Tegra genpd code has a usleep_range(10, 20), is that not sufficient > here? If it is, it would be good to be consistent. Yeah, I think that's enough. The Tegra DRM driver uses these ranges in many places, so that's where I copied them from. None of these are part of a hot path or anything, so whether this sleeps for 10 ns or 4 ms is not going to matter much. With that changed, can I consider this R-b you? Thierry
On 29/11/2018 14:51, Thierry Reding wrote: > On Thu, Nov 29, 2018 at 01:40:32PM +0000, Jon Hunter wrote: >> >> On 23/11/2018 12:06, Thierry Reding wrote: >>> From: Thierry Reding <treding@nvidia.com> >>> >>> Tegra supports generic PM domains on 64-bit ARM, and if that is enabled, >>> the power domain code will make sure that resets are asserted and >>> deasserted at appropriate points in time. >>> >>> If generic PM domains are not implemented, such as on 32-bit Tegra, the >>> resets need to be asserted and deasserted explicitly by the driver. >>> >>> Signed-off-by: Thierry Reding <treding@nvidia.com> >>> --- >>> drivers/gpu/drm/tegra/vic.c | 35 ++++++++++++++++++++++++++++++++++- >>> 1 file changed, 34 insertions(+), 1 deletion(-) >>> >>> diff --git a/drivers/gpu/drm/tegra/vic.c b/drivers/gpu/drm/tegra/vic.c >>> index 9fa77405db01..23f530db45ad 100644 >>> --- a/drivers/gpu/drm/tegra/vic.c >>> +++ b/drivers/gpu/drm/tegra/vic.c >>> @@ -38,6 +38,7 @@ struct vic { >>> struct iommu_domain *domain; >>> struct device *dev; >>> struct clk *clk; >>> + struct reset_control *rst; >>> >>> /* Platform configuration */ >>> const struct vic_config *config; >>> @@ -56,13 +57,37 @@ static void vic_writel(struct vic *vic, u32 value, unsigned int offset) >>> static int vic_runtime_resume(struct device *dev) >>> { >>> struct vic *vic = dev_get_drvdata(dev); >>> + int err; >>> + >>> + err = clk_prepare_enable(vic->clk); >>> + if (err < 0) >>> + return err; >>> + >>> + usleep_range(2000, 4000); >> >> The Tegra genpd code has a usleep_range(10, 20), is that not sufficient >> here? If it is, it would be good to be consistent. > > Yeah, I think that's enough. The Tegra DRM driver uses these ranges in > many places, so that's where I copied them from. None of these are part > of a hot path or anything, so whether this sleeps for 10 ns or 4 ms is > not going to matter much. > > With that changed, can I consider this R-b you? Yes please add my r-b. Thanks Jon
diff --git a/drivers/gpu/drm/tegra/vic.c b/drivers/gpu/drm/tegra/vic.c index 9fa77405db01..23f530db45ad 100644 --- a/drivers/gpu/drm/tegra/vic.c +++ b/drivers/gpu/drm/tegra/vic.c @@ -38,6 +38,7 @@ struct vic { struct iommu_domain *domain; struct device *dev; struct clk *clk; + struct reset_control *rst; /* Platform configuration */ const struct vic_config *config; @@ -56,13 +57,37 @@ static void vic_writel(struct vic *vic, u32 value, unsigned int offset) static int vic_runtime_resume(struct device *dev) { struct vic *vic = dev_get_drvdata(dev); + int err; + + err = clk_prepare_enable(vic->clk); + if (err < 0) + return err; + + usleep_range(2000, 4000); + + err = reset_control_deassert(vic->rst); + if (err < 0) + goto disable; + + usleep_range(2000, 4000); + + return 0; - return clk_prepare_enable(vic->clk); +disable: + clk_disable_unprepare(vic->clk); + return err; } static int vic_runtime_suspend(struct device *dev) { struct vic *vic = dev_get_drvdata(dev); + int err; + + err = reset_control_assert(vic->rst); + if (err < 0) + return err; + + usleep_range(2000, 4000); clk_disable_unprepare(vic->clk); @@ -324,6 +349,14 @@ static int vic_probe(struct platform_device *pdev) return PTR_ERR(vic->clk); } + if (!dev->pm_domain) { + vic->rst = devm_reset_control_get(dev, "vic"); + if (IS_ERR(vic->rst)) { + dev_err(&pdev->dev, "failed to get reset\n"); + return PTR_ERR(vic->rst); + } + } + vic->falcon.dev = dev; vic->falcon.regs = vic->regs; vic->falcon.ops = &vic_falcon_ops;