[PATCH dev-5.0 0/7] Enable video engine

[PATCH dev-5.0 0/7] Enable video engine

[Eddie James]

This series adds the necessary clocks for the video engine to the Aspeed clock
driver. It also adds the video engine node to the AST2500 dts and enables that
node in the witherspoon and romulus dts.

Eddie James (6):
  clk: aspeed: Add video engine reset index definition
  clk: aspeed: Setup video engine clocking
  ARM: dts: aspeed-g5: Add video engine
  ARM: dts: witherspoon: Enable video engine
  ARM: dts: witherspoon: Enable vhub
  ARM: dts: romulus: Enable video engine

Wei Yongjun (1):
  media: platform: Fix missing spin_lock_init()

 arch/arm/boot/dts/aspeed-bmc-opp-romulus.dts     |  7 +++-
 arch/arm/boot/dts/aspeed-bmc-opp-witherspoon.dts | 11 ++++++-
 arch/arm/boot/dts/aspeed-g5.dtsi                 | 11 +++++++
 drivers/clk/clk-aspeed.c                         | 41 ++++++++++++++++++++++--
 drivers/media/platform/aspeed-video.c            |  1 +
 include/dt-bindings/clock/aspeed-clock.h         |  1 +
 6 files changed, 68 insertions(+), 4 deletions(-)

-- 
1.8.3.1

[PATCH dev-5.0 1/7] media: platform: Fix missing spin_lock_init()

[Eddie James]

From: Wei Yongjun <weiyongjun1@huawei.com>

The driver allocates the spinlock but not initialize it.
Use spin_lock_init() on it to initialize it correctly.

This is detected by Coccinelle semantic patch.

Fixes: d2b4387f3bdf ("media: platform: Add Aspeed Video Engine driver")

Signed-off-by: Wei Yongjun <weiyongjun1@huawei.com>
Signed-off-by: Mauro Carvalho Chehab <mchehab+samsung@kernel.org>
---
 drivers/media/platform/aspeed-video.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/media/platform/aspeed-video.c b/drivers/media/platform/aspeed-video.c
index dfec813..692e08e 100644
--- a/drivers/media/platform/aspeed-video.c
+++ b/drivers/media/platform/aspeed-video.c
@@ -1661,6 +1661,7 @@ static int aspeed_video_probe(struct platform_device *pdev)
 
 	video->frame_rate = 30;
 	video->dev = &pdev->dev;
+	spin_lock_init(&video->lock);
 	mutex_init(&video->video_lock);
 	init_waitqueue_head(&video->wait);
 	INIT_DELAYED_WORK(&video->res_work, aspeed_video_resolution_work);
-- 
1.8.3.1

[PATCH dev-5.0 2/7] clk: aspeed: Add video engine reset index definition

[Eddie James]

Add an index into the array of resets kept in the Aspeed clock driver
for the video engine. This isn't a HW bit definition.

Signed-off-by: Eddie James <eajames@linux.ibm.com>
Acked-by: Stephen Boyd <sboyd@kernel.org>
Reviewed-by: Rob Herring <robh@kernel.org>
---
 include/dt-bindings/clock/aspeed-clock.h | 1 +
 1 file changed, 1 insertion(+)

diff --git a/include/dt-bindings/clock/aspeed-clock.h b/include/dt-bindings/clock/aspeed-clock.h
index f437386..15a9059 100644
--- a/include/dt-bindings/clock/aspeed-clock.h
+++ b/include/dt-bindings/clock/aspeed-clock.h
@@ -50,5 +50,6 @@
 #define ASPEED_RESET_I2C		7
 #define ASPEED_RESET_AHB		8
 #define ASPEED_RESET_CRT1		9
+#define ASPEED_RESET_VIDEO		10
 
 #endif
-- 
1.8.3.1

[PATCH dev-5.0 3/7] clk: aspeed: Setup video engine clocking

[Eddie James]

Add the video engine reset bit. Add eclk mux and clock divider table.

Signed-off-by: Eddie James <eajames@linux.ibm.com>
Acked-by: Stephen Boyd <sboyd@kernel.org>
---
 drivers/clk/clk-aspeed.c | 41 +++++++++++++++++++++++++++++++++++++++--
 1 file changed, 39 insertions(+), 2 deletions(-)

diff --git a/drivers/clk/clk-aspeed.c b/drivers/clk/clk-aspeed.c
index 5961367..f16ce7d 100644
--- a/drivers/clk/clk-aspeed.c
+++ b/drivers/clk/clk-aspeed.c
@@ -87,7 +87,7 @@ 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 */
@@ -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,
 };
@@ -317,6 +338,7 @@ struct aspeed_reset {
 	[ASPEED_RESET_PECI]	= 10,
 	[ASPEED_RESET_I2C]	=  2,
 	[ASPEED_RESET_AHB]	=  1,
+	[ASPEED_RESET_VIDEO]	=  6,
 
 	/*
 	 * SCUD4 resets start at an offset to separate them from
@@ -522,6 +544,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 +569,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

[PATCH dev-5.0 4/7] ARM: dts: aspeed-g5: Add video engine

[Eddie James]

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 | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/arch/arm/boot/dts/aspeed-g5.dtsi b/arch/arm/boot/dts/aspeed-g5.dtsi
index a79e01f..0cb143f 100644
--- a/arch/arm/boot/dts/aspeed-g5.dtsi
+++ b/arch/arm/boot/dts/aspeed-g5.dtsi
@@ -252,6 +252,17 @@
 				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";
+				resets = <&syscon ASPEED_RESET_VIDEO>;
+				interrupts = <7>;
+				status = "disabled";
+			};
+
 			sram: sram@1e720000 {
 				compatible = "mmio-sram";
 				reg = <0x1e720000 0x9000>;	// 36K
-- 
1.8.3.1

[PATCH dev-5.0 5/7] ARM: dts: witherspoon: Enable video engine

[Eddie James]

Enable the video engine and set it's required memory-region property.

Signed-off-by: Eddie James <eajames@linux.ibm.com>
---
 arch/arm/boot/dts/aspeed-bmc-opp-witherspoon.dts | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/arch/arm/boot/dts/aspeed-bmc-opp-witherspoon.dts b/arch/arm/boot/dts/aspeed-bmc-opp-witherspoon.dts
index b933ced..9da8ffa 100644
--- a/arch/arm/boot/dts/aspeed-bmc-opp-witherspoon.dts
+++ b/arch/arm/boot/dts/aspeed-bmc-opp-witherspoon.dts
@@ -28,7 +28,7 @@
 		};
 
 		gfx_memory: framebuffer {
-			size = <0x01000000>;
+			size = <0x04000000>;
 			alignment = <0x01000000>;
 			compatible = "shared-dma-pool";
 			reusable;
@@ -660,4 +660,9 @@
 	status = "okay";
 };
 
+&video {
+	status = "okay";
+	memory-region = <&gfx_memory>;
+};
+
 #include "ibm-power9-dual.dtsi"
-- 
1.8.3.1

[PATCH dev-5.0 6/7] ARM: dts: witherspoon: Enable vhub

[Eddie James]

Enable the virtual USB hub.

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

diff --git a/arch/arm/boot/dts/aspeed-bmc-opp-witherspoon.dts b/arch/arm/boot/dts/aspeed-bmc-opp-witherspoon.dts
index 9da8ffa..93e4842 100644
--- a/arch/arm/boot/dts/aspeed-bmc-opp-witherspoon.dts
+++ b/arch/arm/boot/dts/aspeed-bmc-opp-witherspoon.dts
@@ -660,6 +660,10 @@
 	status = "okay";
 };
 
+&vhub {
+	status = "okay";
+};
+
 &video {
 	status = "okay";
 	memory-region = <&gfx_memory>;
-- 
1.8.3.1

[PATCH dev-5.0 7/7] ARM: dts: romulus: Enable video engine

[Eddie James]

Enable the video engine and set it's required memory-region property.

Signed-off-by: Eddie James <eajames@linux.ibm.com>
---
 arch/arm/boot/dts/aspeed-bmc-opp-romulus.dts | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/arch/arm/boot/dts/aspeed-bmc-opp-romulus.dts b/arch/arm/boot/dts/aspeed-bmc-opp-romulus.dts
index ee1a460..6b24608 100644
--- a/arch/arm/boot/dts/aspeed-bmc-opp-romulus.dts
+++ b/arch/arm/boot/dts/aspeed-bmc-opp-romulus.dts
@@ -37,7 +37,7 @@
 		};
 
 		gfx_memory: framebuffer {
-			size = <0x01000000>;
+			size = <0x04000000>;
 			alignment = <0x01000000>;
 			compatible = "shared-dma-pool";
 			reusable;
@@ -315,4 +315,9 @@
      memory-region = <&gfx_memory>;
 };
 
+&video {
+	status = "okay";
+	memory-region = <&gfx_memory>;
+};
+
 #include "ibm-power9-dual.dtsi"
-- 
1.8.3.1

Re: [PATCH dev-5.0 1/7] media: platform: Fix missing spin_lock_init()

[Joel Stanley]

On Thu, 21 Mar 2019 at 19:30, Eddie James <eajames@linux.ibm.com> wrote:
>
> From: Wei Yongjun <weiyongjun1@huawei.com>
>
> The driver allocates the spinlock but not initialize it.
> Use spin_lock_init() on it to initialize it correctly.
>
> This is detected by Coccinelle semantic patch.
>
> Fixes: d2b4387f3bdf ("media: platform: Add Aspeed Video Engine driver")
>
> Signed-off-by: Wei Yongjun <weiyongjun1@huawei.com>
> Signed-off-by: Mauro Carvalho Chehab <mchehab+samsung@kernel.org>

This lacks your sign off.

I have found the upstream commit and cherry picked that instead.

Cheers,

Joel

Re: [PATCH dev-5.0 3/7] clk: aspeed: Setup video engine clocking

[Joel Stanley]

On Thu, 21 Mar 2019 at 19:30, Eddie James <eajames@linux.ibm.com> wrote:
>
> Add the video engine reset bit. Add eclk mux and clock divider table.
>
> Signed-off-by: Eddie James <eajames@linux.ibm.com>
> Acked-by: Stephen Boyd <sboyd@kernel.org>
> ---
>  drivers/clk/clk-aspeed.c | 41 +++++++++++++++++++++++++++++++++++++++--
>  1 file changed, 39 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/clk/clk-aspeed.c b/drivers/clk/clk-aspeed.c
> index 5961367..f16ce7d 100644
> --- a/drivers/clk/clk-aspeed.c
> +++ b/drivers/clk/clk-aspeed.c
> @@ -87,7 +87,7 @@ 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 */

As I've said on previous reviews, it's incorrect to add the reset line
to the clock, and have it be a separate reset device.

When you add the reset bit to the clk device, we control both the
clock gating (bit 0, SCU0C) and the reset line (bit 6, SCU04) with the
clk device. You can see this happen in aspeed_clk_enable if
gate->reset_idx >= 0.

Assuming you are doing this on purpose, can you explain why you want
to have the reset device and the clk device both control the one bit?

Cheers,

Joel


>         [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 */
> @@ -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,
>  };
> @@ -317,6 +338,7 @@ struct aspeed_reset {
>         [ASPEED_RESET_PECI]     = 10,
>         [ASPEED_RESET_I2C]      =  2,
>         [ASPEED_RESET_AHB]      =  1,
> +       [ASPEED_RESET_VIDEO]    =  6,
>
>         /*
>          * SCUD4 resets start at an offset to separate them from
> @@ -522,6 +544,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 +569,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
>

Re: [PATCH dev-5.0 7/7] ARM: dts: romulus: Enable video engine

[Joel Stanley]

On Thu, 21 Mar 2019 at 19:30, Eddie James <eajames@linux.ibm.com> wrote:
>
> Enable the video engine and set it's required memory-region property.
>
> Signed-off-by: Eddie James <eajames@linux.ibm.com>
> ---
>  arch/arm/boot/dts/aspeed-bmc-opp-romulus.dts | 7 ++++++-
>  1 file changed, 6 insertions(+), 1 deletion(-)
>
> diff --git a/arch/arm/boot/dts/aspeed-bmc-opp-romulus.dts b/arch/arm/boot/dts/aspeed-bmc-opp-romulus.dts
> index ee1a460..6b24608 100644
> --- a/arch/arm/boot/dts/aspeed-bmc-opp-romulus.dts
> +++ b/arch/arm/boot/dts/aspeed-bmc-opp-romulus.dts
> @@ -37,7 +37,7 @@
>                 };
>
>                 gfx_memory: framebuffer {
> -                       size = <0x01000000>;
> +                       size = <0x04000000>;

You're using the memory region we have set aside for the gfx device -
the thing that you plug a monitor into the back of a machine with. Are
you sure that is what we want to do?

>                         alignment = <0x01000000>;
>                         compatible = "shared-dma-pool";
>                         reusable;
> @@ -315,4 +315,9 @@
>       memory-region = <&gfx_memory>;
>  };
>
> +&video {
> +       status = "okay";
> +       memory-region = <&gfx_memory>;
> +};
> +
>  #include "ibm-power9-dual.dtsi"
> --
> 1.8.3.1
>

Re: [PATCH dev-5.0 6/7] ARM: dts: witherspoon: Enable vhub

[Joel Stanley]

On Thu, 21 Mar 2019 at 19:30, Eddie James <eajames@linux.ibm.com> wrote:
>
> Enable the virtual USB hub.
>
> Signed-off-by: Eddie James <eajames@linux.ibm.com>

This seems unrelated to the video engine series. I've applied it to
dev-5.0 as the commit message is good.

Cheers,

Joel

Re: [PATCH dev-5.0 3/7] clk: aspeed: Setup video engine clocking

[Eddie James]

On 3/27/19 1:09 AM, Joel Stanley wrote:
> On Thu, 21 Mar 2019 at 19:30, Eddie James <eajames@linux.ibm.com> wrote:
>> Add the video engine reset bit. Add eclk mux and clock divider table.
>>
>> Signed-off-by: Eddie James <eajames@linux.ibm.com>
>> Acked-by: Stephen Boyd <sboyd@kernel.org>
>> ---
>>   drivers/clk/clk-aspeed.c | 41 +++++++++++++++++++++++++++++++++++++++--
>>   1 file changed, 39 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/clk/clk-aspeed.c b/drivers/clk/clk-aspeed.c
>> index 5961367..f16ce7d 100644
>> --- a/drivers/clk/clk-aspeed.c
>> +++ b/drivers/clk/clk-aspeed.c
>> @@ -87,7 +87,7 @@ 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 */
> As I've said on previous reviews, it's incorrect to add the reset line
> to the clock, and have it be a separate reset device.
>
> When you add the reset bit to the clk device, we control both the
> clock gating (bit 0, SCU0C) and the reset line (bit 6, SCU04) with the
> clk device. You can see this happen in aspeed_clk_enable if
> gate->reset_idx >= 0.
>
> Assuming you are doing this on purpose, can you explain why you want
> to have the reset device and the clk device both control the one bit?

Yes. The reset line must be added to the clock device because it's an 
important part of the clock enable/disable operation, as required in the 
AST2500 spec.

The video driver has been written around the idea that the reset device 
is available by itself as well. I ran into a number of issues trying to 
remove that dependency. Firstly its much slower to toggle the clocks 
than to simply toggle the reset, and there are cases where we need to 
quickly reset the engine and reacquire the video signal. But mainly I 
saw issues with interrupts being unstoppable, since the clock driver 
doesn't enable the reset before the clocks turn off, and then the video 
engine registers can no longer be changed once the clocks are off.

I don't really understand what's so incorrect about having both things 
control the reset. Maybe we can chat tonight to resolve this.

Thanks,

Eddie

>
> Cheers,
>
> Joel
>
>
>>          [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 */
>> @@ -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,
>>   };
>> @@ -317,6 +338,7 @@ struct aspeed_reset {
>>          [ASPEED_RESET_PECI]     = 10,
>>          [ASPEED_RESET_I2C]      =  2,
>>          [ASPEED_RESET_AHB]      =  1,
>> +       [ASPEED_RESET_VIDEO]    =  6,
>>
>>          /*
>>           * SCUD4 resets start at an offset to separate them from
>> @@ -522,6 +544,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 +569,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
>>

Re: [PATCH dev-5.0 7/7] ARM: dts: romulus: Enable video engine

[Eddie James]

On 3/27/19 1:11 AM, Joel Stanley wrote:
> On Thu, 21 Mar 2019 at 19:30, Eddie James <eajames@linux.ibm.com> wrote:
>> Enable the video engine and set it's required memory-region property.
>>
>> Signed-off-by: Eddie James <eajames@linux.ibm.com>
>> ---
>>   arch/arm/boot/dts/aspeed-bmc-opp-romulus.dts | 7 ++++++-
>>   1 file changed, 6 insertions(+), 1 deletion(-)
>>
>> diff --git a/arch/arm/boot/dts/aspeed-bmc-opp-romulus.dts b/arch/arm/boot/dts/aspeed-bmc-opp-romulus.dts
>> index ee1a460..6b24608 100644
>> --- a/arch/arm/boot/dts/aspeed-bmc-opp-romulus.dts
>> +++ b/arch/arm/boot/dts/aspeed-bmc-opp-romulus.dts
>> @@ -37,7 +37,7 @@
>>                  };
>>
>>                  gfx_memory: framebuffer {
>> -                       size = <0x01000000>;
>> +                       size = <0x04000000>;
> You're using the memory region we have set aside for the gfx device -
> the thing that you plug a monitor into the back of a machine with. Are
> you sure that is what we want to do?

It still has to be allocated in the kernel, right? I'm assuming that the 
kernel won't give memory areas we've grabbed with the gfx driver to the 
video driver, and same in reverse. I could add a separate memory region 
too, this just seemed convenient.

>
>>                          alignment = <0x01000000>;
>>                          compatible = "shared-dma-pool";
>>                          reusable;
>> @@ -315,4 +315,9 @@
>>        memory-region = <&gfx_memory>;
>>   };
>>
>> +&video {
>> +       status = "okay";
>> +       memory-region = <&gfx_memory>;
>> +};
>> +
>>   #include "ibm-power9-dual.dtsi"
>> --
>> 1.8.3.1
>>

Re: [PATCH dev-5.0 7/7] ARM: dts: romulus: Enable video engine

[Andrew Jeffery]

On Thu, 28 Mar 2019, at 01:37, Eddie James wrote:
> 
> On 3/27/19 1:11 AM, Joel Stanley wrote:
> > On Thu, 21 Mar 2019 at 19:30, Eddie James <eajames@linux.ibm.com> wrote:
> >> Enable the video engine and set it's required memory-region property.
> >>
> >> Signed-off-by: Eddie James <eajames@linux.ibm.com>
> >> ---
> >>   arch/arm/boot/dts/aspeed-bmc-opp-romulus.dts | 7 ++++++-
> >>   1 file changed, 6 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/arch/arm/boot/dts/aspeed-bmc-opp-romulus.dts b/arch/arm/boot/dts/aspeed-bmc-opp-romulus.dts
> >> index ee1a460..6b24608 100644
> >> --- a/arch/arm/boot/dts/aspeed-bmc-opp-romulus.dts
> >> +++ b/arch/arm/boot/dts/aspeed-bmc-opp-romulus.dts
> >> @@ -37,7 +37,7 @@
> >>                  };
> >>
> >>                  gfx_memory: framebuffer {
> >> -                       size = <0x01000000>;
> >> +                       size = <0x04000000>;
> > You're using the memory region we have set aside for the gfx device -
> > the thing that you plug a monitor into the back of a machine with. Are
> > you sure that is what we want to do?
> 
> It still has to be allocated in the kernel, right? I'm assuming that the 
> kernel won't give memory areas we've grabbed with the gfx driver to the 
> video driver, and same in reverse. I could add a separate memory region 
> too, this just seemed convenient.

Lets go with a new node.

> 
> >
> >>                          alignment = <0x01000000>;
> >>                          compatible = "shared-dma-pool";
> >>                          reusable;
> >> @@ -315,4 +315,9 @@
> >>        memory-region = <&gfx_memory>;
> >>   };
> >>
> >> +&video {
> >> +       status = "okay";
> >> +       memory-region = <&gfx_memory>;
> >> +};
> >> +
> >>   #include "ibm-power9-dual.dtsi"
> >> --
> >> 1.8.3.1
> >>
> 
>