[01/14] staging: media: tegra-vde: Support BSEV clock and reset
diff mbox series

Message ID 20180813145027.16346-2-thierry.reding@gmail.com
State New
Headers show
Series
  • staging: media: tegra-vdea: Add Tegra124 support
Related show

Commit Message

Thierry Reding Aug. 13, 2018, 2:50 p.m. UTC
From: Thierry Reding <treding@nvidia.com>

The BSEV clock has a separate gate bit and can not be assumed to be
always enabled. Add explicit handling for the BSEV clock and reset.

This fixes an issue on Tegra124 where the BSEV clock is not enabled
by default and therefore accessing the BSEV registers will hang the
CPU if the BSEV clock is not enabled and the reset not deasserted.

Signed-off-by: Thierry Reding <treding@nvidia.com>
---
 drivers/staging/media/tegra-vde/tegra-vde.c | 35 +++++++++++++++++++--
 1 file changed, 33 insertions(+), 2 deletions(-)

Comments

Dmitry Osipenko Aug. 13, 2018, 3:09 p.m. UTC | #1
On Monday, 13 August 2018 17:50:14 MSK Thierry Reding wrote:
> From: Thierry Reding <treding@nvidia.com>
> 
> The BSEV clock has a separate gate bit and can not be assumed to be
> always enabled. Add explicit handling for the BSEV clock and reset.
> 
> This fixes an issue on Tegra124 where the BSEV clock is not enabled
> by default and therefore accessing the BSEV registers will hang the
> CPU if the BSEV clock is not enabled and the reset not deasserted.
> 
> Signed-off-by: Thierry Reding <treding@nvidia.com>
> ---

Are you sure that BSEV clock is really needed for T20/30? I've tried already 
to disable the clock explicitly and everything kept working, though I'll try 
again.

The device-tree changes should be reflected in the binding documentation.
Thierry Reding Aug. 14, 2018, 2:21 p.m. UTC | #2
On Mon, Aug 13, 2018 at 06:09:46PM +0300, Dmitry Osipenko wrote:
> On Monday, 13 August 2018 17:50:14 MSK Thierry Reding wrote:
> > From: Thierry Reding <treding@nvidia.com>
> > 
> > The BSEV clock has a separate gate bit and can not be assumed to be
> > always enabled. Add explicit handling for the BSEV clock and reset.
> > 
> > This fixes an issue on Tegra124 where the BSEV clock is not enabled
> > by default and therefore accessing the BSEV registers will hang the
> > CPU if the BSEV clock is not enabled and the reset not deasserted.
> > 
> > Signed-off-by: Thierry Reding <treding@nvidia.com>
> > ---
> 
> Are you sure that BSEV clock is really needed for T20/30? I've tried already 
> to disable the clock explicitly and everything kept working, though I'll try 
> again.

I think you're right that these aren't strictly required for VDE to work
on Tegra20 and Tegra30. However, the BSEV clock and reset do exist on
those platforms, so I didn't see a reason why they shouldn't be handled
uniformly across all generations.

> The device-tree changes should be reflected in the binding documentation.

Indeed, I forgot to update that.

Thierry
Dmitry Osipenko Aug. 14, 2018, 3:05 p.m. UTC | #3
On Tuesday, 14 August 2018 17:21:24 MSK Thierry Reding wrote:
> On Mon, Aug 13, 2018 at 06:09:46PM +0300, Dmitry Osipenko wrote:
> > On Monday, 13 August 2018 17:50:14 MSK Thierry Reding wrote:
> > > From: Thierry Reding <treding@nvidia.com>
> > > 
> > > The BSEV clock has a separate gate bit and can not be assumed to be
> > > always enabled. Add explicit handling for the BSEV clock and reset.
> > > 
> > > This fixes an issue on Tegra124 where the BSEV clock is not enabled
> > > by default and therefore accessing the BSEV registers will hang the
> > > CPU if the BSEV clock is not enabled and the reset not deasserted.
> > > 
> > > Signed-off-by: Thierry Reding <treding@nvidia.com>
> > > ---
> > 
> > Are you sure that BSEV clock is really needed for T20/30? I've tried
> > already to disable the clock explicitly and everything kept working,
> > though I'll try again.
> 
> I think you're right that these aren't strictly required for VDE to work
> on Tegra20 and Tegra30. However, the BSEV clock and reset do exist on
> those platforms, so I didn't see a reason why they shouldn't be handled
> uniformly across all generations.

It's a bit messy to have unsed clock being enabled.

I guess BSEV clock on T20/30 only enables the AES engine. If the decryption 
engine is integrated with the video decoder, then the clock and reset should 
be requested by the driver, but BSEV should be kept disabled if it's not used.

If BSEV clock isn't powering anything related to VDE on T20/30, then let's 
make BSEV clock and reset control optional. For the clock we could check 
whether err = -ENOENT and continue, later we may switch to 
devm_clk_get_optional() of the upcoming [0]. For the reset there is 
devm_reset_control_get_optional(). 

Please try to verify by all means that we can omit BSEV on T20/30. If you are 
not sure, then let's make them optional as we can always make them required 
later.

P.S. I'll test and review all the patches during the next days. 

[0] https://lkml.org/lkml/2018/7/18/460
Dmitry Osipenko Aug. 14, 2018, 3:16 p.m. UTC | #4
On Tuesday, 14 August 2018 18:05:51 MSK Dmitry Osipenko wrote:
> On Tuesday, 14 August 2018 17:21:24 MSK Thierry Reding wrote:
> > On Mon, Aug 13, 2018 at 06:09:46PM +0300, Dmitry Osipenko wrote:
> > > On Monday, 13 August 2018 17:50:14 MSK Thierry Reding wrote:
> > > > From: Thierry Reding <treding@nvidia.com>
> > > > 
> > > > The BSEV clock has a separate gate bit and can not be assumed to be
> > > > always enabled. Add explicit handling for the BSEV clock and reset.
> > > > 
> > > > This fixes an issue on Tegra124 where the BSEV clock is not enabled
> > > > by default and therefore accessing the BSEV registers will hang the
> > > > CPU if the BSEV clock is not enabled and the reset not deasserted.
> > > > 
> > > > Signed-off-by: Thierry Reding <treding@nvidia.com>
> > > > ---
> > > 
> > > Are you sure that BSEV clock is really needed for T20/30? I've tried
> > > already to disable the clock explicitly and everything kept working,
> > > though I'll try again.
> > 
> > I think you're right that these aren't strictly required for VDE to work
> > on Tegra20 and Tegra30. However, the BSEV clock and reset do exist on
> > those platforms, so I didn't see a reason why they shouldn't be handled
> > uniformly across all generations.
> 
> It's a bit messy to have unsed clock being enabled.
> 
> I guess BSEV clock on T20/30 only enables the AES engine. If the decryption
> engine is integrated with the video decoder, then the clock and reset should
> be requested by the driver, but BSEV should be kept disabled if it's not
> used.

Though even if encryption is not directly integrated with the video decoding, 
then it still makes sense to define the clock and reset in DT without using 
them by the VDE driver since the HW registers space is shared. If somebody 
would like to implement the AES driver, it could be made as a sub-device of 
VDE.

Patch
diff mbox series

diff --git a/drivers/staging/media/tegra-vde/tegra-vde.c b/drivers/staging/media/tegra-vde/tegra-vde.c
index 6f06061a40d9..9d8f833744db 100644
--- a/drivers/staging/media/tegra-vde/tegra-vde.c
+++ b/drivers/staging/media/tegra-vde/tegra-vde.c
@@ -74,9 +74,11 @@  struct tegra_vde {
 	struct miscdevice miscdev;
 	struct reset_control *rst;
 	struct reset_control *rst_mc;
+	struct reset_control *rst_bsev;
 	struct gen_pool *iram_pool;
 	struct completion decode_completion;
 	struct clk *clk;
+	struct clk *clk_bsev;
 	dma_addr_t iram_lists_addr;
 	u32 *iram;
 };
@@ -979,6 +981,11 @@  static int tegra_vde_runtime_suspend(struct device *dev)
 		return err;
 	}
 
+	reset_control_assert(vde->rst_bsev);
+
+	usleep_range(2000, 4000);
+
+	clk_disable_unprepare(vde->clk_bsev);
 	clk_disable_unprepare(vde->clk);
 
 	return 0;
@@ -996,6 +1003,16 @@  static int tegra_vde_runtime_resume(struct device *dev)
 		return err;
 	}
 
+	err = clk_prepare_enable(vde->clk_bsev);
+	if (err < 0)
+		return err;
+
+	err = reset_control_deassert(vde->rst_bsev);
+	if (err < 0)
+		return err;
+
+	usleep_range(2000, 4000);
+
 	return 0;
 }
 
@@ -1084,14 +1101,21 @@  static int tegra_vde_probe(struct platform_device *pdev)
 	if (IS_ERR(vde->frameid))
 		return PTR_ERR(vde->frameid);
 
-	vde->clk = devm_clk_get(dev, NULL);
+	vde->clk = devm_clk_get(dev, "vde");
 	if (IS_ERR(vde->clk)) {
 		err = PTR_ERR(vde->clk);
 		dev_err(dev, "Could not get VDE clk %d\n", err);
 		return err;
 	}
 
-	vde->rst = devm_reset_control_get(dev, NULL);
+	vde->clk_bsev = devm_clk_get(dev, "bsev");
+	if (IS_ERR(vde->clk_bsev)) {
+		err = PTR_ERR(vde->clk_bsev);
+		dev_err(dev, "failed to get BSEV clock: %d\n", err);
+		return err;
+	}
+
+	vde->rst = devm_reset_control_get(dev, "vde");
 	if (IS_ERR(vde->rst)) {
 		err = PTR_ERR(vde->rst);
 		dev_err(dev, "Could not get VDE reset %d\n", err);
@@ -1105,6 +1129,13 @@  static int tegra_vde_probe(struct platform_device *pdev)
 		return err;
 	}
 
+	vde->rst_bsev = devm_reset_control_get(dev, "bsev");
+	if (IS_ERR(vde->rst_bsev)) {
+		err = PTR_ERR(vde->rst_bsev);
+		dev_err(dev, "failed to get BSEV reset: %d\n", err);
+		return err;
+	}
+
 	irq = platform_get_irq_byname(pdev, "sync-token");
 	if (irq < 0)
 		return irq;