Message ID | 1553196610-29595-4-git-send-email-eajames@linux.ibm.com |
---|---|
State | Changes Requested, archived |
Headers | show |
Series | Enable video engine | expand |
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 >
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 >>
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++) {