[PATCH 0/5] Aspeed: Enable video engine

[PATCH 0/5] Aspeed: Enable video engine

[Eddie James]

This series enables the video engine on Aspeed BMC platforms. The video engine
clocking is added to the Aspeed clock driver. The use of the video engine reset
line, originally planned to be made externally available in the clock driver,
is removed from the video engine driver. Finally a node for the video engine is
added to the AST2500 devicetree.
The series also includes a missing property for reserved memory in the
devicetree documentation, and a small change to make the video engine driver
start without reserved memory.

Eddie James (5):
  media: platform: Aspeed: Remove use of reset line
  media: platform: Aspeed: Make reserved memory optional
  media: dt-bindings: aspeed-video: Add missing memory-region property
  clk: Aspeed: Setup video engine clocking
  ARM: dts: aspeed-g5: Add video engine

 .../devicetree/bindings/media/aspeed-video.txt     |  6 ++++
 arch/arm/boot/dts/aspeed-g5.dtsi                   | 10 ++++++
 drivers/clk/clk-aspeed.c                           | 42 ++++++++++++++++++++--
 drivers/media/platform/aspeed-video.c              | 33 ++++-------------
 4 files changed, 61 insertions(+), 30 deletions(-)

-- 
1.8.3.1

[PATCH 1/5] media: platform: Aspeed: Remove use of reset line

[Eddie James]

The reset line is toggled by enabling the clocks, so it's not necessary
to manually toggle the reset as well.

Signed-off-by: Eddie James <eajames@linux.ibm.com>
---
 drivers/media/platform/aspeed-video.c | 27 +++++----------------------
 1 file changed, 5 insertions(+), 22 deletions(-)

diff --git a/drivers/media/platform/aspeed-video.c b/drivers/media/platform/aspeed-video.c
index 692e08e..55c55a6 100644
--- a/drivers/media/platform/aspeed-video.c
+++ b/drivers/media/platform/aspeed-video.c
@@ -14,7 +14,6 @@
 #include <linux/of_irq.h>
 #include <linux/of_reserved_mem.h>
 #include <linux/platform_device.h>
-#include <linux/reset.h>
 #include <linux/sched.h>
 #include <linux/spinlock.h>
 #include <linux/string.h>
@@ -208,7 +207,6 @@ struct aspeed_video {
 	void __iomem *base;
 	struct clk *eclk;
 	struct clk *vclk;
-	struct reset_control *rst;
 
 	struct device *dev;
 	struct v4l2_ctrl_handler ctrl_handler;
@@ -483,19 +481,10 @@ static void aspeed_video_enable_mode_detect(struct aspeed_video *video)
 	aspeed_video_update(video, VE_SEQ_CTRL, 0, VE_SEQ_CTRL_TRIG_MODE_DET);
 }
 
-static void aspeed_video_reset(struct aspeed_video *video)
-{
-	/* Reset the engine */
-	reset_control_assert(video->rst);
-
-	/* Don't usleep here; function may be called in interrupt context */
-	udelay(100);
-	reset_control_deassert(video->rst);
-}
-
 static void aspeed_video_off(struct aspeed_video *video)
 {
-	aspeed_video_reset(video);
+	/* Disable interrupts */
+	aspeed_video_write(video, VE_INTERRUPT_CTRL, 0);
 
 	/* Turn off the relevant clocks */
 	clk_disable_unprepare(video->vclk);
@@ -507,8 +496,6 @@ static void aspeed_video_on(struct aspeed_video *video)
 	/* Turn on the relevant clocks */
 	clk_prepare_enable(video->eclk);
 	clk_prepare_enable(video->vclk);
-
-	aspeed_video_reset(video);
 }
 
 static void aspeed_video_bufs_done(struct aspeed_video *video,
@@ -1464,7 +1451,9 @@ static void aspeed_video_stop_streaming(struct vb2_queue *q)
 		 * Need to force stop any DMA and try and get HW into a good
 		 * state for future calls to start streaming again.
 		 */
-		aspeed_video_reset(video);
+		aspeed_video_off(video);
+		aspeed_video_on(video);
+
 		aspeed_video_init_regs(video);
 
 		aspeed_video_get_resolution(video);
@@ -1619,12 +1608,6 @@ static int aspeed_video_init(struct aspeed_video *video)
 		return PTR_ERR(video->vclk);
 	}
 
-	video->rst = devm_reset_control_get_exclusive(dev, NULL);
-	if (IS_ERR(video->rst)) {
-		dev_err(dev, "Unable to get VE reset\n");
-		return PTR_ERR(video->rst);
-	}
-
 	rc = of_reserved_mem_device_init(dev);
 	if (rc) {
 		dev_err(dev, "Unable to reserve memory\n");
-- 
1.8.3.1

[PATCH 2/5] media: platform: Aspeed: Make reserved memory optional

[Eddie James]

Reserved memory doesn't need to be required; system memory would work
fine.

Signed-off-by: Eddie James <eajames@linux.ibm.com>
---
 drivers/media/platform/aspeed-video.c | 6 +-----
 1 file changed, 1 insertion(+), 5 deletions(-)

diff --git a/drivers/media/platform/aspeed-video.c b/drivers/media/platform/aspeed-video.c
index 55c55a6..8144fe3 100644
--- a/drivers/media/platform/aspeed-video.c
+++ b/drivers/media/platform/aspeed-video.c
@@ -1608,11 +1608,7 @@ static int aspeed_video_init(struct aspeed_video *video)
 		return PTR_ERR(video->vclk);
 	}
 
-	rc = of_reserved_mem_device_init(dev);
-	if (rc) {
-		dev_err(dev, "Unable to reserve memory\n");
-		return rc;
-	}
+	of_reserved_mem_device_init(dev);
 
 	rc = dma_set_mask_and_coherent(dev, DMA_BIT_MASK(32));
 	if (rc) {
-- 
1.8.3.1

Re: [PATCH 1/5] media: platform: Aspeed: Remove use of reset line

[Joel Stanley]

On Tue, 2 Apr 2019 at 18:24, Eddie James <eajames@linux.ibm.com> wrote:
>
> The reset line is toggled by enabling the clocks, so it's not necessary
> to manually toggle the reset as well.
>
> Signed-off-by: Eddie James <eajames@linux.ibm.com>

Reviewed-by: Joel Stanley <joel@jms.id.au>

Re: [PATCH 2/5] media: platform: Aspeed: Make reserved memory optional

[Andrew Jeffery]

On Thu, 4 Apr 2019, at 01:05, Eddie James wrote:
> 
> On 4/3/19 1:01 AM, Andrew Jeffery wrote:
> >
> > On Wed, 3 Apr 2019, at 04:55, Eddie James wrote:
> >> Reserved memory doesn't need to be required; system memory would work
> >> fine.
> > I had to do a bit of legwork to understand what you were doing here. My
> > understanding is that we allocate out of the default CMA region if the
> > memory-region property isn't specified. Is that what you're expecting?
> > Could be helpful to be a little less terse in the commit message.
> 
> 
> Correct.
> 
> 
> >
> >> Signed-off-by: Eddie James <eajames@linux.ibm.com>
> >> ---
> >>   drivers/media/platform/aspeed-video.c | 6 +-----
> >>   1 file changed, 1 insertion(+), 5 deletions(-)
> >>
> >> diff --git a/drivers/media/platform/aspeed-video.c
> >> b/drivers/media/platform/aspeed-video.c
> >> index 55c55a6..8144fe3 100644
> >> --- a/drivers/media/platform/aspeed-video.c
> >> +++ b/drivers/media/platform/aspeed-video.c
> >> @@ -1608,11 +1608,7 @@ static int aspeed_video_init(struct aspeed_video
> >> *video)
> >>   		return PTR_ERR(video->vclk);
> >>   	}
> >>   
> >> -	rc = of_reserved_mem_device_init(dev);
> >> -	if (rc) {
> >> -		dev_err(dev, "Unable to reserve memory\n");
> >> -		return rc;
> >> -	}
> >> +	of_reserved_mem_device_init(dev);
> > You're ignoring *all* errors here with the expectation that the cause is the
> > missing memory-region property. However, other errors can propagate
> > out of of_reserved_mem_device_init() - e.g. ENOMEM. Rather than remove
> > error checking, I think you should explicitly test for ENODEV, which is what is
> > returned if the memory-region property is absent.
> 
> 
> But it doesn't matter if it fails for any reason, any DMA allocation 
> should fall back to default CMA memory. In the case of ENOMEM or other 
> errors, then the later calls to allocate DMA may fail and we can deal 
> with it then.

Fair enough then. I think it deserves a comment, but up to you.

Andrew

Re: [PATCH 2/5] media: platform: Aspeed: Make reserved memory optional

[Eddie James]

On 4/3/19 1:01 AM, Andrew Jeffery wrote:
>
> On Wed, 3 Apr 2019, at 04:55, Eddie James wrote:
>> Reserved memory doesn't need to be required; system memory would work
>> fine.
> I had to do a bit of legwork to understand what you were doing here. My
> understanding is that we allocate out of the default CMA region if the
> memory-region property isn't specified. Is that what you're expecting?
> Could be helpful to be a little less terse in the commit message.


Correct.


>
>> Signed-off-by: Eddie James <eajames@linux.ibm.com>
>> ---
>>   drivers/media/platform/aspeed-video.c | 6 +-----
>>   1 file changed, 1 insertion(+), 5 deletions(-)
>>
>> diff --git a/drivers/media/platform/aspeed-video.c
>> b/drivers/media/platform/aspeed-video.c
>> index 55c55a6..8144fe3 100644
>> --- a/drivers/media/platform/aspeed-video.c
>> +++ b/drivers/media/platform/aspeed-video.c
>> @@ -1608,11 +1608,7 @@ static int aspeed_video_init(struct aspeed_video
>> *video)
>>   		return PTR_ERR(video->vclk);
>>   	}
>>   
>> -	rc = of_reserved_mem_device_init(dev);
>> -	if (rc) {
>> -		dev_err(dev, "Unable to reserve memory\n");
>> -		return rc;
>> -	}
>> +	of_reserved_mem_device_init(dev);
> You're ignoring *all* errors here with the expectation that the cause is the
> missing memory-region property. However, other errors can propagate
> out of of_reserved_mem_device_init() - e.g. ENOMEM. Rather than remove
> error checking, I think you should explicitly test for ENODEV, which is what is
> returned if the memory-region property is absent.


But it doesn't matter if it fails for any reason, any DMA allocation 
should fall back to default CMA memory. In the case of ENOMEM or other 
errors, then the later calls to allocate DMA may fail and we can deal 
with it then.


Thanks,

Eddie


>
> Cheers,
>
> Andrew
>
>>   
>>   	rc = dma_set_mask_and_coherent(dev, DMA_BIT_MASK(32));
>>   	if (rc) {
>> -- 
>> 1.8.3.1
>>
>>

Re: [PATCH 2/5] media: platform: Aspeed: Make reserved memory optional

[Andrew Jeffery]

On Wed, 3 Apr 2019, at 04:55, Eddie James wrote:
> Reserved memory doesn't need to be required; system memory would work
> fine.

I had to do a bit of legwork to understand what you were doing here. My
understanding is that we allocate out of the default CMA region if the
memory-region property isn't specified. Is that what you're expecting?
Could be helpful to be a little less terse in the commit message.

> 
> Signed-off-by: Eddie James <eajames@linux.ibm.com>
> ---
>  drivers/media/platform/aspeed-video.c | 6 +-----
>  1 file changed, 1 insertion(+), 5 deletions(-)
> 
> diff --git a/drivers/media/platform/aspeed-video.c 
> b/drivers/media/platform/aspeed-video.c
> index 55c55a6..8144fe3 100644
> --- a/drivers/media/platform/aspeed-video.c
> +++ b/drivers/media/platform/aspeed-video.c
> @@ -1608,11 +1608,7 @@ static int aspeed_video_init(struct aspeed_video 
> *video)
>  		return PTR_ERR(video->vclk);
>  	}
>  
> -	rc = of_reserved_mem_device_init(dev);
> -	if (rc) {
> -		dev_err(dev, "Unable to reserve memory\n");
> -		return rc;
> -	}
> +	of_reserved_mem_device_init(dev);

You're ignoring *all* errors here with the expectation that the cause is the
missing memory-region property. However, other errors can propagate
out of of_reserved_mem_device_init() - e.g. ENOMEM. Rather than remove
error checking, I think you should explicitly test for ENODEV, which is what is
returned if the memory-region property is absent.

Cheers,

Andrew

>  
>  	rc = dma_set_mask_and_coherent(dev, DMA_BIT_MASK(32));
>  	if (rc) {
> -- 
> 1.8.3.1
> 
>

[PATCH 2/5] media: platform: Aspeed: Make reserved memory optional

[Eddie James]

Reserved memory doesn't need to be required; system memory would work
fine.

Signed-off-by: Eddie James <eajames@linux.ibm.com>
---
 drivers/media/platform/aspeed-video.c | 6 +-----
 1 file changed, 1 insertion(+), 5 deletions(-)

diff --git a/drivers/media/platform/aspeed-video.c b/drivers/media/platform/aspeed-video.c
index 55c55a6..8144fe3 100644
--- a/drivers/media/platform/aspeed-video.c
+++ b/drivers/media/platform/aspeed-video.c
@@ -1608,11 +1608,7 @@ static int aspeed_video_init(struct aspeed_video *video)
 		return PTR_ERR(video->vclk);
 	}
 
-	rc = of_reserved_mem_device_init(dev);
-	if (rc) {
-		dev_err(dev, "Unable to reserve memory\n");
-		return rc;
-	}
+	of_reserved_mem_device_init(dev);
 
 	rc = dma_set_mask_and_coherent(dev, DMA_BIT_MASK(32));
 	if (rc) {
-- 
1.8.3.1