diff mbox series

[1/3] drm/tegra: vic: Implement explicit reset support

Message ID 20181123120639.16706-1-thierry.reding@gmail.com (mailing list archive)
State New, archived
Headers show
Series [1/3] drm/tegra: vic: Implement explicit reset support | expand

Commit Message

Thierry Reding Nov. 23, 2018, 12:06 p.m. UTC
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(-)

Comments

Jon Hunter Nov. 29, 2018, 1:40 p.m. UTC | #1
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
Thierry Reding Nov. 29, 2018, 2:51 p.m. UTC | #2
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
Jon Hunter Nov. 29, 2018, 3:11 p.m. UTC | #3
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 mbox series

Patch

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;