linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH 0/5] Aspeed: Enable video engine
@ 2019-04-02 18:24 Eddie James
  2019-04-02 18:25 ` [PATCH 1/5] media: platform: Aspeed: Remove use of reset line Eddie James
                   ` (5 more replies)
  0 siblings, 6 replies; 16+ messages in thread
From: Eddie James @ 2019-04-02 18:24 UTC (permalink / raw)
  To: linux-media
  Cc: linux-kernel, linux-aspeed, joel, andrew, mchehab, linux-clk,
	sboyd, mturquette, devicetree, mark.rutland, robh+dt

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


^ permalink raw reply	[flat|nested] 16+ messages in thread

* [PATCH 1/5] media: platform: Aspeed: Remove use of reset line
  2019-04-02 18:24 [PATCH 0/5] Aspeed: Enable video engine Eddie James
@ 2019-04-02 18:25 ` Eddie James
  2019-04-11  2:49   ` Joel Stanley
  2019-04-02 18:25 ` [PATCH 2/5] media: platform: Aspeed: Make reserved memory optional Eddie James
                   ` (4 subsequent siblings)
  5 siblings, 1 reply; 16+ messages in thread
From: Eddie James @ 2019-04-02 18:25 UTC (permalink / raw)
  To: linux-media
  Cc: linux-kernel, linux-aspeed, joel, andrew, mchehab, linux-clk,
	sboyd, mturquette, devicetree, mark.rutland, robh+dt

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


^ permalink raw reply related	[flat|nested] 16+ messages in thread

* [PATCH 2/5] media: platform: Aspeed: Make reserved memory optional
  2019-04-02 18:24 [PATCH 0/5] Aspeed: Enable video engine Eddie James
  2019-04-02 18:25 ` [PATCH 1/5] media: platform: Aspeed: Remove use of reset line Eddie James
@ 2019-04-02 18:25 ` Eddie James
  2019-04-03  6:01   ` Andrew Jeffery
  2019-04-02 18:25 ` [PATCH 3/5] media: dt-bindings: aspeed-video: Add missing memory-region property Eddie James
                   ` (3 subsequent siblings)
  5 siblings, 1 reply; 16+ messages in thread
From: Eddie James @ 2019-04-02 18:25 UTC (permalink / raw)
  To: linux-media
  Cc: linux-kernel, linux-aspeed, joel, andrew, mchehab, linux-clk,
	sboyd, mturquette, devicetree, mark.rutland, robh+dt

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


^ permalink raw reply related	[flat|nested] 16+ messages in thread

* [PATCH 3/5] media: dt-bindings: aspeed-video: Add missing memory-region property
  2019-04-02 18:24 [PATCH 0/5] Aspeed: Enable video engine Eddie James
  2019-04-02 18:25 ` [PATCH 1/5] media: platform: Aspeed: Remove use of reset line Eddie James
  2019-04-02 18:25 ` [PATCH 2/5] media: platform: Aspeed: Make reserved memory optional Eddie James
@ 2019-04-02 18:25 ` Eddie James
  2019-04-02 21:05   ` Jae Hyun Yoo
  2019-04-06  6:06   ` Rob Herring
  2019-04-02 18:25 ` [PATCH 4/5] clk: Aspeed: Setup video engine clocking Eddie James
                   ` (2 subsequent siblings)
  5 siblings, 2 replies; 16+ messages in thread
From: Eddie James @ 2019-04-02 18:25 UTC (permalink / raw)
  To: linux-media
  Cc: linux-kernel, linux-aspeed, joel, andrew, mchehab, linux-clk,
	sboyd, mturquette, devicetree, mark.rutland, robh+dt

Missed documenting this property in the initial commit.

Signed-off-by: Eddie James <eajames@linux.ibm.com>
---
 Documentation/devicetree/bindings/media/aspeed-video.txt | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/Documentation/devicetree/bindings/media/aspeed-video.txt b/Documentation/devicetree/bindings/media/aspeed-video.txt
index 78b464a..346c2d3 100644
--- a/Documentation/devicetree/bindings/media/aspeed-video.txt
+++ b/Documentation/devicetree/bindings/media/aspeed-video.txt
@@ -14,6 +14,11 @@ Required properties:
 			the VE
  - interrupts:		the interrupt associated with the VE on this platform
 
+Optional properties:
+ - memory-region:
+	phandle to a memory region to allocate from, as defined in
+	Documentation/devicetree/bindings/reserved-memory/reserved-memory.txt
+
 Example:
 
 video-engine@1e700000 {
@@ -23,4 +28,5 @@ video-engine@1e700000 {
     clock-names = "vclk", "eclk";
     resets = <&syscon ASPEED_RESET_VIDEO>;
     interrupts = <7>;
+    memory-region = <&video_engine_memory>
 };
-- 
1.8.3.1


^ permalink raw reply related	[flat|nested] 16+ messages in thread

* [PATCH 4/5] clk: Aspeed: Setup video engine clocking
  2019-04-02 18:24 [PATCH 0/5] Aspeed: Enable video engine Eddie James
                   ` (2 preceding siblings ...)
  2019-04-02 18:25 ` [PATCH 3/5] media: dt-bindings: aspeed-video: Add missing memory-region property Eddie James
@ 2019-04-02 18:25 ` Eddie James
  2019-04-11  2:50   ` Joel Stanley
  2019-04-02 18:25 ` [PATCH 5/5] ARM: dts: aspeed-g5: Add video engine Eddie James
  2019-04-24 10:50 ` [PATCH 0/5] Aspeed: Enable " Hans Verkuil
  5 siblings, 1 reply; 16+ messages in thread
From: Eddie James @ 2019-04-02 18:25 UTC (permalink / raw)
  To: linux-media
  Cc: linux-kernel, linux-aspeed, joel, andrew, mchehab, linux-clk,
	sboyd, mturquette, devicetree, mark.rutland, robh+dt

Add eclk mux and clock divider table. Also change the video engine reset
to the correct clock; it was previously on the video capture but needs
to be on the video engine clock.

Signed-off-by: Eddie James <eajames@linux.ibm.com>
---
 drivers/clk/clk-aspeed.c | 42 +++++++++++++++++++++++++++++++++++++++---
 1 file changed, 39 insertions(+), 3 deletions(-)

diff --git a/drivers/clk/clk-aspeed.c b/drivers/clk/clk-aspeed.c
index 5961367..42b4df6 100644
--- a/drivers/clk/clk-aspeed.c
+++ b/drivers/clk/clk-aspeed.c
@@ -87,10 +87,10 @@ struct aspeed_clk_gate {
 /* TODO: ask Aspeed about the actual parent data */
 static const struct aspeed_gate_data aspeed_gates[] = {
 	/*				 clk rst   name			parent	flags */
-	[ASPEED_CLK_GATE_ECLK] =	{  0, -1, "eclk-gate",		"eclk",	0 }, /* Video Engine */
+	[ASPEED_CLK_GATE_ECLK] =	{  0,  6, "eclk-gate",		"eclk",	0 }, /* Video Engine */
 	[ASPEED_CLK_GATE_GCLK] =	{  1,  7, "gclk-gate",		NULL,	0 }, /* 2D engine */
 	[ASPEED_CLK_GATE_MCLK] =	{  2, -1, "mclk-gate",		"mpll",	CLK_IS_CRITICAL }, /* SDRAM */
-	[ASPEED_CLK_GATE_VCLK] =	{  3,  6, "vclk-gate",		NULL,	0 }, /* Video Capture */
+	[ASPEED_CLK_GATE_VCLK] =	{  3, -1, "vclk-gate",		NULL,	0 }, /* Video Capture */
 	[ASPEED_CLK_GATE_BCLK] =	{  4,  8, "bclk-gate",		"bclk",	CLK_IS_CRITICAL }, /* PCIe/PCI */
 	[ASPEED_CLK_GATE_DCLK] =	{  5, -1, "dclk-gate",		NULL,	CLK_IS_CRITICAL }, /* DAC */
 	[ASPEED_CLK_GATE_REFCLK] =	{  6, -1, "refclk-gate",	"clkin", CLK_IS_CRITICAL },
@@ -113,6 +113,24 @@ struct aspeed_clk_gate {
 	[ASPEED_CLK_GATE_LHCCLK] =	{ 28, -1, "lhclk-gate",		"lhclk", 0 }, /* LPC master/LPC+ */
 };
 
+static const char * const eclk_parent_names[] = {
+	"mpll",
+	"hpll",
+	"dpll",
+};
+
+static const struct clk_div_table ast2500_eclk_div_table[] = {
+	{ 0x0, 2 },
+	{ 0x1, 2 },
+	{ 0x2, 3 },
+	{ 0x3, 4 },
+	{ 0x4, 5 },
+	{ 0x5, 6 },
+	{ 0x6, 7 },
+	{ 0x7, 8 },
+	{ 0 }
+};
+
 static const struct clk_div_table ast2500_mac_div_table[] = {
 	{ 0x0, 4 }, /* Yep, really. Aspeed confirmed this is correct */
 	{ 0x1, 4 },
@@ -192,18 +210,21 @@ static struct clk_hw *aspeed_ast2500_calc_pll(const char *name, u32 val)
 
 struct aspeed_clk_soc_data {
 	const struct clk_div_table *div_table;
+	const struct clk_div_table *eclk_div_table;
 	const struct clk_div_table *mac_div_table;
 	struct clk_hw *(*calc_pll)(const char *name, u32 val);
 };
 
 static const struct aspeed_clk_soc_data ast2500_data = {
 	.div_table = ast2500_div_table,
+	.eclk_div_table = ast2500_eclk_div_table,
 	.mac_div_table = ast2500_mac_div_table,
 	.calc_pll = aspeed_ast2500_calc_pll,
 };
 
 static const struct aspeed_clk_soc_data ast2400_data = {
 	.div_table = ast2400_div_table,
+	.eclk_div_table = ast2400_div_table,
 	.mac_div_table = ast2400_div_table,
 	.calc_pll = aspeed_ast2400_calc_pll,
 };
@@ -522,6 +543,22 @@ static int aspeed_clk_probe(struct platform_device *pdev)
 		return PTR_ERR(hw);
 	aspeed_clk_data->hws[ASPEED_CLK_24M] = hw;
 
+	hw = clk_hw_register_mux(dev, "eclk-mux", eclk_parent_names,
+				 ARRAY_SIZE(eclk_parent_names), 0,
+				 scu_base + ASPEED_CLK_SELECTION, 2, 0x3, 0,
+				 &aspeed_clk_lock);
+	if (IS_ERR(hw))
+		return PTR_ERR(hw);
+	aspeed_clk_data->hws[ASPEED_CLK_ECLK_MUX] = hw;
+
+	hw = clk_hw_register_divider_table(dev, "eclk", "eclk-mux", 0,
+					   scu_base + ASPEED_CLK_SELECTION, 28,
+					   3, 0, soc_data->eclk_div_table,
+					   &aspeed_clk_lock);
+	if (IS_ERR(hw))
+		return PTR_ERR(hw);
+	aspeed_clk_data->hws[ASPEED_CLK_ECLK] = hw;
+
 	/*
 	 * TODO: There are a number of clocks that not included in this driver
 	 * as more information is required:
@@ -531,7 +568,6 @@ static int aspeed_clk_probe(struct platform_device *pdev)
 	 *   RGMII
 	 *   RMII
 	 *   UART[1..5] clock source mux
-	 *   Video Engine (ECLK) mux and clock divider
 	 */
 
 	for (i = 0; i < ARRAY_SIZE(aspeed_gates); i++) {
-- 
1.8.3.1


^ permalink raw reply related	[flat|nested] 16+ messages in thread

* [PATCH 5/5] ARM: dts: aspeed-g5: Add video engine
  2019-04-02 18:24 [PATCH 0/5] Aspeed: Enable video engine Eddie James
                   ` (3 preceding siblings ...)
  2019-04-02 18:25 ` [PATCH 4/5] clk: Aspeed: Setup video engine clocking Eddie James
@ 2019-04-02 18:25 ` Eddie James
  2019-04-24 10:50 ` [PATCH 0/5] Aspeed: Enable " Hans Verkuil
  5 siblings, 0 replies; 16+ messages in thread
From: Eddie James @ 2019-04-02 18:25 UTC (permalink / raw)
  To: linux-media
  Cc: linux-kernel, linux-aspeed, joel, andrew, mchehab, linux-clk,
	sboyd, mturquette, devicetree, mark.rutland, robh+dt

Add a node to describe the video engine on the AST2500.

Signed-off-by: Eddie James <eajames@linux.ibm.com>
---
 arch/arm/boot/dts/aspeed-g5.dtsi | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/arch/arm/boot/dts/aspeed-g5.dtsi b/arch/arm/boot/dts/aspeed-g5.dtsi
index 85ed9db..c6d5edc 100644
--- a/arch/arm/boot/dts/aspeed-g5.dtsi
+++ b/arch/arm/boot/dts/aspeed-g5.dtsi
@@ -243,6 +243,16 @@
 				status = "disabled";
 			};
 
+			video: video@1e700000 {
+				compatible = "aspeed,ast2500-video-engine";
+				reg = <0x1e700000 0x1000>;
+				clocks = <&syscon ASPEED_CLK_GATE_VCLK>,
+					 <&syscon ASPEED_CLK_GATE_ECLK>;
+				clock-names = "vclk", "eclk";
+				interrupts = <7>;
+				status = "disabled";
+			};
+
 			sram: sram@1e720000 {
 				compatible = "mmio-sram";
 				reg = <0x1e720000 0x9000>;	// 36K
-- 
1.8.3.1


^ permalink raw reply related	[flat|nested] 16+ messages in thread

* Re: [PATCH 3/5] media: dt-bindings: aspeed-video: Add missing memory-region property
  2019-04-02 18:25 ` [PATCH 3/5] media: dt-bindings: aspeed-video: Add missing memory-region property Eddie James
@ 2019-04-02 21:05   ` Jae Hyun Yoo
  2019-04-06  6:06   ` Rob Herring
  1 sibling, 0 replies; 16+ messages in thread
From: Jae Hyun Yoo @ 2019-04-02 21:05 UTC (permalink / raw)
  To: Eddie James, linux-media
  Cc: mark.rutland, devicetree, linux-aspeed, mturquette, linux-kernel,
	sboyd, robh+dt, mchehab, linux-clk

Hi Eddie,

On 4/2/2019 11:25 AM, Eddie James wrote:
> Missed documenting this property in the initial commit.
> 
> Signed-off-by: Eddie James <eajames@linux.ibm.com>
> ---
>   Documentation/devicetree/bindings/media/aspeed-video.txt | 6 ++++++
>   1 file changed, 6 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/media/aspeed-video.txt b/Documentation/devicetree/bindings/media/aspeed-video.txt
> index 78b464a..346c2d3 100644
> --- a/Documentation/devicetree/bindings/media/aspeed-video.txt
> +++ b/Documentation/devicetree/bindings/media/aspeed-video.txt
> @@ -14,6 +14,11 @@ Required properties:
>   			the VE
>    - interrupts:		the interrupt associated with the VE on this platform
>   
> +Optional properties:
> + - memory-region:
> +	phandle to a memory region to allocate from, as defined in
> +	Documentation/devicetree/bindings/reserved-memory/reserved-memory.txt
> +
>   Example:
>   
>   video-engine@1e700000 {
> @@ -23,4 +28,5 @@ video-engine@1e700000 {
>       clock-names = "vclk", "eclk";
>       resets = <&syscon ASPEED_RESET_VIDEO>;
>       interrupts = <7>;
> +    memory-region = <&video_engine_memory>

nit: a semicolon at the end of the line.

>   };
> 

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH 2/5] media: platform: Aspeed: Make reserved memory optional
  2019-04-02 18:25 ` [PATCH 2/5] media: platform: Aspeed: Make reserved memory optional Eddie James
@ 2019-04-03  6:01   ` Andrew Jeffery
  2019-04-03 14:35     ` Eddie James
  0 siblings, 1 reply; 16+ messages in thread
From: Andrew Jeffery @ 2019-04-03  6:01 UTC (permalink / raw)
  To: Eddie James, linux-media
  Cc: linux-kernel, linux-aspeed, Joel Stanley, Mauro Carvalho Chehab,
	linux-clk, Stephen Boyd, Michael Turquette, devicetree,
	mark.rutland, Rob Herring



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
> 
>

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH 2/5] media: platform: Aspeed: Make reserved memory optional
  2019-04-03  6:01   ` Andrew Jeffery
@ 2019-04-03 14:35     ` Eddie James
  2019-04-04  2:16       ` Andrew Jeffery
  0 siblings, 1 reply; 16+ messages in thread
From: Eddie James @ 2019-04-03 14:35 UTC (permalink / raw)
  To: Andrew Jeffery, Eddie James, linux-media
  Cc: linux-kernel, linux-aspeed, Joel Stanley, Mauro Carvalho Chehab,
	linux-clk, Stephen Boyd, Michael Turquette, devicetree,
	mark.rutland, Rob Herring


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
>>
>>


^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH 2/5] media: platform: Aspeed: Make reserved memory optional
  2019-04-03 14:35     ` Eddie James
@ 2019-04-04  2:16       ` Andrew Jeffery
  0 siblings, 0 replies; 16+ messages in thread
From: Andrew Jeffery @ 2019-04-04  2:16 UTC (permalink / raw)
  To: Eddie James, Eddie James, linux-media
  Cc: linux-kernel, linux-aspeed, Joel Stanley, Mauro Carvalho Chehab,
	linux-clk, Stephen Boyd, Michael Turquette, devicetree,
	mark.rutland, Rob Herring



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

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH 3/5] media: dt-bindings: aspeed-video: Add missing memory-region property
  2019-04-02 18:25 ` [PATCH 3/5] media: dt-bindings: aspeed-video: Add missing memory-region property Eddie James
  2019-04-02 21:05   ` Jae Hyun Yoo
@ 2019-04-06  6:06   ` Rob Herring
  1 sibling, 0 replies; 16+ messages in thread
From: Rob Herring @ 2019-04-06  6:06 UTC (permalink / raw)
  To: Eddie James
  Cc: linux-media, linux-kernel, linux-aspeed, joel, andrew, mchehab,
	linux-clk, sboyd, mturquette, devicetree, mark.rutland, robh+dt

On Tue,  2 Apr 2019 18:25:02 +0000, Eddie James wrote:
> Missed documenting this property in the initial commit.
> 
> Signed-off-by: Eddie James <eajames@linux.ibm.com>
> ---
>  Documentation/devicetree/bindings/media/aspeed-video.txt | 6 ++++++
>  1 file changed, 6 insertions(+)
> 

Reviewed-by: Rob Herring <robh@kernel.org>


^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH 1/5] media: platform: Aspeed: Remove use of reset line
  2019-04-02 18:25 ` [PATCH 1/5] media: platform: Aspeed: Remove use of reset line Eddie James
@ 2019-04-11  2:49   ` Joel Stanley
  0 siblings, 0 replies; 16+ messages in thread
From: Joel Stanley @ 2019-04-11  2:49 UTC (permalink / raw)
  To: Eddie James
  Cc: linux-media, Linux Kernel Mailing List, linux-aspeed,
	Joel Stanley, Andrew Jeffery, mchehab, linux-clk, Stephen Boyd,
	Michael Turquette, devicetree, Mark Rutland, Rob Herring

On Tue, 2 Apr 2019 at 18:25, 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>

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH 4/5] clk: Aspeed: Setup video engine clocking
  2019-04-02 18:25 ` [PATCH 4/5] clk: Aspeed: Setup video engine clocking Eddie James
@ 2019-04-11  2:50   ` Joel Stanley
  2019-04-18 21:56     ` Stephen Boyd
  0 siblings, 1 reply; 16+ messages in thread
From: Joel Stanley @ 2019-04-11  2:50 UTC (permalink / raw)
  To: Eddie James
  Cc: linux-media, Linux Kernel Mailing List, linux-aspeed,
	Joel Stanley, Andrew Jeffery, mchehab, linux-clk, Stephen Boyd,
	Michael Turquette, devicetree, Mark Rutland, Rob Herring

On Tue, 2 Apr 2019 at 18:25, Eddie James <eajames@linux.ibm.com> wrote:
>
> Add eclk mux and clock divider table. Also change the video engine reset
> to the correct clock; it was previously on the video capture but needs
> to be on the video engine clock.
>
> Signed-off-by: Eddie James <eajames@linux.ibm.com>

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

Stephen, Eddie and I have worked together on this change and I am
happy with it now.

Cheers,

Joel

> ---
>  drivers/clk/clk-aspeed.c | 42 +++++++++++++++++++++++++++++++++++++++---
>  1 file changed, 39 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/clk/clk-aspeed.c b/drivers/clk/clk-aspeed.c
> index 5961367..42b4df6 100644
> --- a/drivers/clk/clk-aspeed.c
> +++ b/drivers/clk/clk-aspeed.c
> @@ -87,10 +87,10 @@ struct aspeed_clk_gate {
>  /* TODO: ask Aspeed about the actual parent data */
>  static const struct aspeed_gate_data aspeed_gates[] = {
>         /*                               clk rst   name                 parent  flags */
> -       [ASPEED_CLK_GATE_ECLK] =        {  0, -1, "eclk-gate",          "eclk", 0 }, /* Video Engine */
> +       [ASPEED_CLK_GATE_ECLK] =        {  0,  6, "eclk-gate",          "eclk", 0 }, /* Video Engine */
>         [ASPEED_CLK_GATE_GCLK] =        {  1,  7, "gclk-gate",          NULL,   0 }, /* 2D engine */
>         [ASPEED_CLK_GATE_MCLK] =        {  2, -1, "mclk-gate",          "mpll", CLK_IS_CRITICAL }, /* SDRAM */
> -       [ASPEED_CLK_GATE_VCLK] =        {  3,  6, "vclk-gate",          NULL,   0 }, /* Video Capture */
> +       [ASPEED_CLK_GATE_VCLK] =        {  3, -1, "vclk-gate",          NULL,   0 }, /* Video Capture */
>         [ASPEED_CLK_GATE_BCLK] =        {  4,  8, "bclk-gate",          "bclk", CLK_IS_CRITICAL }, /* PCIe/PCI */
>         [ASPEED_CLK_GATE_DCLK] =        {  5, -1, "dclk-gate",          NULL,   CLK_IS_CRITICAL }, /* DAC */
>         [ASPEED_CLK_GATE_REFCLK] =      {  6, -1, "refclk-gate",        "clkin", CLK_IS_CRITICAL },
> @@ -113,6 +113,24 @@ struct aspeed_clk_gate {
>         [ASPEED_CLK_GATE_LHCCLK] =      { 28, -1, "lhclk-gate",         "lhclk", 0 }, /* LPC master/LPC+ */
>  };
>
> +static const char * const eclk_parent_names[] = {
> +       "mpll",
> +       "hpll",
> +       "dpll",
> +};
> +
> +static const struct clk_div_table ast2500_eclk_div_table[] = {
> +       { 0x0, 2 },
> +       { 0x1, 2 },
> +       { 0x2, 3 },
> +       { 0x3, 4 },
> +       { 0x4, 5 },
> +       { 0x5, 6 },
> +       { 0x6, 7 },
> +       { 0x7, 8 },
> +       { 0 }
> +};
> +
>  static const struct clk_div_table ast2500_mac_div_table[] = {
>         { 0x0, 4 }, /* Yep, really. Aspeed confirmed this is correct */
>         { 0x1, 4 },
> @@ -192,18 +210,21 @@ static struct clk_hw *aspeed_ast2500_calc_pll(const char *name, u32 val)
>
>  struct aspeed_clk_soc_data {
>         const struct clk_div_table *div_table;
> +       const struct clk_div_table *eclk_div_table;
>         const struct clk_div_table *mac_div_table;
>         struct clk_hw *(*calc_pll)(const char *name, u32 val);
>  };
>
>  static const struct aspeed_clk_soc_data ast2500_data = {
>         .div_table = ast2500_div_table,
> +       .eclk_div_table = ast2500_eclk_div_table,
>         .mac_div_table = ast2500_mac_div_table,
>         .calc_pll = aspeed_ast2500_calc_pll,
>  };
>
>  static const struct aspeed_clk_soc_data ast2400_data = {
>         .div_table = ast2400_div_table,
> +       .eclk_div_table = ast2400_div_table,
>         .mac_div_table = ast2400_div_table,
>         .calc_pll = aspeed_ast2400_calc_pll,
>  };
> @@ -522,6 +543,22 @@ static int aspeed_clk_probe(struct platform_device *pdev)
>                 return PTR_ERR(hw);
>         aspeed_clk_data->hws[ASPEED_CLK_24M] = hw;
>
> +       hw = clk_hw_register_mux(dev, "eclk-mux", eclk_parent_names,
> +                                ARRAY_SIZE(eclk_parent_names), 0,
> +                                scu_base + ASPEED_CLK_SELECTION, 2, 0x3, 0,
> +                                &aspeed_clk_lock);
> +       if (IS_ERR(hw))
> +               return PTR_ERR(hw);
> +       aspeed_clk_data->hws[ASPEED_CLK_ECLK_MUX] = hw;
> +
> +       hw = clk_hw_register_divider_table(dev, "eclk", "eclk-mux", 0,
> +                                          scu_base + ASPEED_CLK_SELECTION, 28,
> +                                          3, 0, soc_data->eclk_div_table,
> +                                          &aspeed_clk_lock);
> +       if (IS_ERR(hw))
> +               return PTR_ERR(hw);
> +       aspeed_clk_data->hws[ASPEED_CLK_ECLK] = hw;
> +
>         /*
>          * TODO: There are a number of clocks that not included in this driver
>          * as more information is required:
> @@ -531,7 +568,6 @@ static int aspeed_clk_probe(struct platform_device *pdev)
>          *   RGMII
>          *   RMII
>          *   UART[1..5] clock source mux
> -        *   Video Engine (ECLK) mux and clock divider
>          */
>
>         for (i = 0; i < ARRAY_SIZE(aspeed_gates); i++) {
> --
> 1.8.3.1
>

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH 4/5] clk: Aspeed: Setup video engine clocking
  2019-04-11  2:50   ` Joel Stanley
@ 2019-04-18 21:56     ` Stephen Boyd
  0 siblings, 0 replies; 16+ messages in thread
From: Stephen Boyd @ 2019-04-18 21:56 UTC (permalink / raw)
  To: Eddie James, Joel Stanley
  Cc: linux-media, Linux Kernel Mailing List, linux-aspeed,
	Joel Stanley, Andrew Jeffery, mchehab, linux-clk,
	Michael Turquette, devicetree, Mark Rutland, Rob Herring

Quoting Joel Stanley (2019-04-10 19:50:16)
> On Tue, 2 Apr 2019 at 18:25, Eddie James <eajames@linux.ibm.com> wrote:
> >
> > Add eclk mux and clock divider table. Also change the video engine reset
> > to the correct clock; it was previously on the video capture but needs
> > to be on the video engine clock.
> >
> > Signed-off-by: Eddie James <eajames@linux.ibm.com>
> 
> Reviewed-by: Joel Stanley <joel@jms.id.au>
> 
> Stephen, Eddie and I have worked together on this change and I am
> happy with it now.
> 

Thanks! Applied to clk-next.


^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH 0/5] Aspeed: Enable video engine
  2019-04-02 18:24 [PATCH 0/5] Aspeed: Enable video engine Eddie James
                   ` (4 preceding siblings ...)
  2019-04-02 18:25 ` [PATCH 5/5] ARM: dts: aspeed-g5: Add video engine Eddie James
@ 2019-04-24 10:50 ` Hans Verkuil
  2019-04-24 15:00   ` Eddie James
  5 siblings, 1 reply; 16+ messages in thread
From: Hans Verkuil @ 2019-04-24 10:50 UTC (permalink / raw)
  To: Eddie James, linux-media
  Cc: linux-kernel, linux-aspeed, joel, andrew, mchehab, linux-clk,
	sboyd, mturquette, devicetree, mark.rutland, robh+dt

Hi Eddie,

On 4/2/19 8:24 PM, Eddie James wrote:
> 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.

I assume you'll post a v3 since there were some comments?

If you want to get it in for 5.2, then be aware that you should post it tomorrow
at the latest.

Regards,

	Hans

> 
> 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(-)
> 


^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH 0/5] Aspeed: Enable video engine
  2019-04-24 10:50 ` [PATCH 0/5] Aspeed: Enable " Hans Verkuil
@ 2019-04-24 15:00   ` Eddie James
  0 siblings, 0 replies; 16+ messages in thread
From: Eddie James @ 2019-04-24 15:00 UTC (permalink / raw)
  To: Hans Verkuil, Eddie James, linux-media
  Cc: linux-kernel, linux-aspeed, joel, andrew, mchehab, linux-clk,
	sboyd, mturquette, devicetree, mark.rutland, robh+dt


On 4/24/19 5:50 AM, Hans Verkuil wrote:
> Hi Eddie,
>
> On 4/2/19 8:24 PM, Eddie James wrote:
>> 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.
> I assume you'll post a v3 since there were some comments?


Sure... The only change would be a missing semi-colon in the devicetree 
doc. And we're only on v2 :)


>
> If you want to get it in for 5.2, then be aware that you should post it tomorrow
> at the latest.
>
> Regards,
>
> 	Hans
>
>> 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(-)
>>


^ permalink raw reply	[flat|nested] 16+ messages in thread

end of thread, other threads:[~2019-04-24 15:00 UTC | newest]

Thread overview: 16+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-04-02 18:24 [PATCH 0/5] Aspeed: Enable video engine Eddie James
2019-04-02 18:25 ` [PATCH 1/5] media: platform: Aspeed: Remove use of reset line Eddie James
2019-04-11  2:49   ` Joel Stanley
2019-04-02 18:25 ` [PATCH 2/5] media: platform: Aspeed: Make reserved memory optional Eddie James
2019-04-03  6:01   ` Andrew Jeffery
2019-04-03 14:35     ` Eddie James
2019-04-04  2:16       ` Andrew Jeffery
2019-04-02 18:25 ` [PATCH 3/5] media: dt-bindings: aspeed-video: Add missing memory-region property Eddie James
2019-04-02 21:05   ` Jae Hyun Yoo
2019-04-06  6:06   ` Rob Herring
2019-04-02 18:25 ` [PATCH 4/5] clk: Aspeed: Setup video engine clocking Eddie James
2019-04-11  2:50   ` Joel Stanley
2019-04-18 21:56     ` Stephen Boyd
2019-04-02 18:25 ` [PATCH 5/5] ARM: dts: aspeed-g5: Add video engine Eddie James
2019-04-24 10:50 ` [PATCH 0/5] Aspeed: Enable " Hans Verkuil
2019-04-24 15:00   ` Eddie James

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).