diff mbox series

[dev-4.19,2/6] clk: aspeed: Setup video engine clocking

Message ID 1546552448-22621-3-git-send-email-eajames@linux.ibm.com
State Changes Requested, archived
Headers show
Series Aspeed Video Engine | expand

Commit Message

Eddie James Jan. 3, 2019, 9:54 p.m. UTC 
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(-)

Comments

Joel Stanley Jan. 8, 2019, 2:14 a.m. UTC | #1 
On Fri, 4 Jan 2019 at 08:54, Eddie James <eajames@linux.ibm.com> wrote:

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

> @@ -317,6 +338,7 @@ struct aspeed_reset {
>         [ASPEED_RESET_PECI]     = 10,
>         [ASPEED_RESET_I2C]      =  2,
>         [ASPEED_RESET_AHB]      =  1,
> +       [ASPEED_RESET_VIDEO]    =  6,

This is incorrect. You've already added the video reset to the video
clock, so the reset will be released by enabling the clock.

If this is the patch that went upstream then upstream needs to be fixed too.
Eddie James Jan. 8, 2019, 3:12 p.m. UTC | #2 
On 01/07/2019 08:14 PM, Joel Stanley wrote:
> On Fri, 4 Jan 2019 at 08:54, Eddie James <eajames@linux.ibm.com> wrote:
>
>>   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 */
>> @@ -317,6 +338,7 @@ struct aspeed_reset {
>>          [ASPEED_RESET_PECI]     = 10,
>>          [ASPEED_RESET_I2C]      =  2,
>>          [ASPEED_RESET_AHB]      =  1,
>> +       [ASPEED_RESET_VIDEO]    =  6,
> This is incorrect. You've already added the video reset to the video
> clock, so the reset will be released by enabling the clock.

I don't quite understand what's wrong about this. Why can't the clock 
control and reset control both be done independently? Of course the 
clock control overrides the reset control, but that's fine, presumably?

I'd rather have the reset available separately, so if this must be 
changed, I'd prefer to drop the reset from the clock description. That 
way the video driver doesn't need changes.

Thanks,
Eddie

>
> If this is the patch that went upstream then upstream needs to be fixed too.
>
Eddie James Jan. 8, 2019, 4:14 p.m. UTC | #3 
On 01/08/2019 09:12 AM, Eddie James wrote:
>
>
> On 01/07/2019 08:14 PM, Joel Stanley wrote:
>> On Fri, 4 Jan 2019 at 08:54, Eddie James <eajames@linux.ibm.com> wrote:
>>
>>>   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 */
>>> @@ -317,6 +338,7 @@ struct aspeed_reset {
>>>          [ASPEED_RESET_PECI]     = 10,
>>>          [ASPEED_RESET_I2C]      =  2,
>>>          [ASPEED_RESET_AHB]      =  1,
>>> +       [ASPEED_RESET_VIDEO]    =  6,
>> This is incorrect. You've already added the video reset to the video
>> clock, so the reset will be released by enabling the clock.
>
> I don't quite understand what's wrong about this. Why can't the clock 
> control and reset control both be done independently? Of course the 
> clock control overrides the reset control, but that's fine, presumably?
>
> I'd rather have the reset available separately, so if this must be 
> changed, I'd prefer to drop the reset from the clock description. That 
> way the video driver doesn't need changes.

Nevermind, clock start requires the reset toggle according to the spec, 
so can't drop that...

>
> Thanks,
> Eddie
>
>>
>> If this is the patch that went upstream then upstream needs to be 
>> fixed too.
>>
>
Eddie James Jan. 28, 2019, 8:41 p.m. UTC | #4 
On 01/08/2019 09:12 AM, Eddie James wrote:
>
>
> On 01/07/2019 08:14 PM, Joel Stanley wrote:
>> On Fri, 4 Jan 2019 at 08:54, Eddie James <eajames@linux.ibm.com> wrote:
>>
>>>   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 */
>>> @@ -317,6 +338,7 @@ struct aspeed_reset {
>>>          [ASPEED_RESET_PECI]     = 10,
>>>          [ASPEED_RESET_I2C]      =  2,
>>>          [ASPEED_RESET_AHB]      =  1,
>>> +       [ASPEED_RESET_VIDEO]    =  6,
>> This is incorrect. You've already added the video reset to the video
>> clock, so the reset will be released by enabling the clock.
>
> I don't quite understand what's wrong about this. Why can't the clock 
> control and reset control both be done independently? Of course the 
> clock control overrides the reset control, but that's fine, presumably?

Hey Joel,

After some more testing, the driver NEEDS to have the reset available 
separately. The problem is that the clock driver doesn't enable reset 
before the clocks turn off, resulting in odd behavior. For example, I 
get endless IRQs and can't disable them by writing the engine registers, 
since the clocks are off.

What do you suggest here?

Thanks,
Eddie

>
> I'd rather have the reset available separately, so if this must be 
> changed, I'd prefer to drop the reset from the clock description. That 
> way the video driver doesn't need changes.
>
> Thanks,
> Eddie
>
>>
>> If this is the patch that went upstream then upstream needs to be 
>> fixed too.
>>
>
diff mbox series

Patch

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++) {