Re: [PATCH 3/4] clk: meson: axg: add Video Clocks

2020-09-11 Thread Kevin Hilman 
Neil Armstrong  writes:

> On 10/09/2020 12:13, Jerome Brunet wrote:

[...]

>> Why is NOCACHE ? Is there something poking behind CCF back ? if yes, why
>> is this required ?
>
> I'm surprised you ask this since the situation hasn't changed since I pushed 
> the
> video clock for GXBB, then G12A.

Even if the situation hasn't changed, providing context is very helpful
for new reviewers or reviewers who might need a reminder of that
situation.  I for one had already forgotten the video clock context so I
had a similiar question as Jerome when I saw the NOCACHE and UNUSED
flags.

As you know, new usage of those flags is always highly scrutinized, so
providing the right context and explanation, even if it's repeating
stuff you've stated elsewhere, can help preempt any questions.  

> The switch to CCF for the VPU is still planned, but until now I was unable to 
> allocate enough
> time for this huge rework.
>
> Since I'm the single DRM driver contributor and lacking any other reviewers, 
> I must deal
> with new features & bugfixing before moving to CCF.

Not at all a blocker for this patch, but just curious do you have a TODO
list someplace for this driver in case others might have some time to
help you with the migration?

> The clock situation is far from perfect, and I'd also like it to be solved at 
> some point.
>
> I'll add the same explanation I gave for GXBB in 
> https://lkml.kernel.org/r/1541516257-16157-5-git-send-email-narmstr...@baylibre.com

Yes, that's perfect.

Thanks,

Kevin

Re: [PATCH 3/4] clk: meson: axg: add Video Clocks

2020-09-10 Thread Neil Armstrong 
Hi,

On 10/09/2020 12:13, Jerome Brunet wrote:
> 
> On Mon 07 Sep 2020 at 11:38, Neil Armstrong  wrote:
> 
>> Add the Video Clocks present on the Amlogic AXg SoCs.
>>
>> The AXG only has a single ENCL CTS clock and even if VCLK exist along VCLK2,
>> only VCLK2 is used since it clocks the MIPI DSI IP directly.
>>
>> Signed-off-by: Neil Armstrong 
>> ---
>>  drivers/clk/meson/axg.c | 774 
>>  drivers/clk/meson/axg.h |  21 +-
>>  2 files changed, 794 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/clk/meson/axg.c b/drivers/clk/meson/axg.c
>> index 13fc0006f63d..2550616c14b0 100644
>> --- a/drivers/clk/meson/axg.c
>> +++ b/drivers/clk/meson/axg.c
>> @@ -1026,6 +1026,704 @@ static struct clk_regmap axg_sd_emmc_c_clk0 = {
>>  },
>>  };
>>  
>> +/* VPU Clock */
>> +
>> +static const struct clk_hw *axg_vpu_parent_hws[] = {
>> +_fclk_div4.hw,
>> +_fclk_div3.hw,
>> +_fclk_div5.hw,
>> +_fclk_div7.hw,
>> +};
>> +
>> +static struct clk_regmap axg_vpu_0_sel = {
>> +.data = &(struct clk_regmap_mux_data){
>> +.offset = HHI_VPU_CLK_CNTL,
>> +.mask = 0x3,
>> +.shift = 9,
>> +},
>> +.hw.init = &(struct clk_init_data){
>> +.name = "vpu_0_sel",
>> +.ops = _regmap_mux_ops,
>> +/*
>> + * bits 9:10 selects from 4 possible parents:
>> + * fclk_div4, fclk_div3, fclk_div5, fclk_div7,
>> + */
> 
> These comments (and the same bellow) are not very useful

Ok, these are copy/paste from gxbb.c and g12a.c, I wonder why these are now 
useless.

> 
>> +.parent_hws = axg_vpu_parent_hws,
>> +.num_parents = ARRAY_SIZE(axg_vpu_parent_hws),
> 
> Could you add a comment here explaining why parenting needs to be
> manually controlled ?

Ditto, this was already explained for GXBB & G12A.
The VPU clock needs a specific clock parenting defined in DT to
achieve the frequency used by the vendor.

> 
>> +.flags = CLK_SET_RATE_NO_REPARENT,
>> +},
>> +};
>> +
>> +static struct clk_regmap axg_vpu_0_div = {
>> +.data = &(struct clk_regmap_div_data){
>> +.offset = HHI_VPU_CLK_CNTL,
>> +.shift = 0,
>> +.width = 7,
>> +},
>> +.hw.init = &(struct clk_init_data){
>> +.name = "vpu_0_div",
>> +.ops = _regmap_divider_ops,
>> +.parent_hws = (const struct clk_hw *[]) { _vpu_0_sel.hw },
>> +.num_parents = 1,
>> +.flags = CLK_SET_RATE_PARENT,
>> +},
>> +};
>> +
>> +static struct clk_regmap axg_vpu_0 = {
>> +.data = &(struct clk_regmap_gate_data){
>> +.offset = HHI_VPU_CLK_CNTL,
>> +.bit_idx = 8,
>> +},
>> +.hw.init = &(struct clk_init_data) {
>> +.name = "vpu_0",
>> +.ops = _regmap_gate_ops,
>> +.parent_hws = (const struct clk_hw *[]) { _vpu_0_div.hw },
>> +.num_parents = 1,
>> +.flags = CLK_SET_RATE_PARENT | CLK_IGNORE_UNUSED,
> 
> Why is UNUSED required ?

It's UNUSED until we have a mechanism to keep a clock enabled until the proper
module is loaded, and it was already discussed for GXBB & G12A.

> 
>> +},
>> +};
>> +
>> +static struct clk_regmap axg_vpu_1_sel = {
>> +.data = &(struct clk_regmap_mux_data){
>> +.offset = HHI_VPU_CLK_CNTL,
>> +.mask = 0x3,
>> +.shift = 25,
>> +},
>> +.hw.init = &(struct clk_init_data){
>> +.name = "vpu_1_sel",
>> +.ops = _regmap_mux_ops,
>> +/*
>> + * bits 25:26 selects from 4 possible parents:
>> + * fclk_div4, fclk_div3, fclk_div5, fclk_div7,
>> + */
>> +.parent_hws = axg_vpu_parent_hws,
>> +.num_parents = ARRAY_SIZE(axg_vpu_parent_hws),
>> +.flags = CLK_SET_RATE_NO_REPARENT,
>> +},
>> +};
>> +
>> +static struct clk_regmap axg_vpu_1_div = {
>> +.data = &(struct clk_regmap_div_data){
>> +.offset = HHI_VPU_CLK_CNTL,
>> +.shift = 16,
>> +.width = 7,
>> +},
>> +.hw.init = &(struct clk_init_data){
>> +.name = "vpu_1_div",
>> +.ops = _regmap_divider_ops,
>> +.parent_hws = (const struct clk_hw *[]) { _vpu_1_sel.hw },
>> +.num_parents = 1,
>> +.flags = CLK_SET_RATE_PARENT,
>> +},
>> +};
>> +
>> +static struct clk_regmap axg_vpu_1 = {
>> +.data = &(struct clk_regmap_gate_data){
>> +.offset = HHI_VPU_CLK_CNTL,
>> +.bit_idx = 24,
>> +},
>> +.hw.init = &(struct clk_init_data) {
>> +.name = "vpu_1",
>> +.ops = _regmap_gate_ops,
>> +.parent_hws = (const struct clk_hw *[]) { _vpu_1_div.hw },
>> +.num_parents = 1,
>> +.flags = CLK_SET_RATE_PARENT | CLK_IGNORE_UNUSED,
>> +},
>> +};
>> +
>> +static struct clk_regmap axg_vpu = {
>> +.data = &(struct

Re: [PATCH 3/4] clk: meson: axg: add Video Clocks

2020-09-10 Thread Jerome Brunet 


On Mon 07 Sep 2020 at 11:38, Neil Armstrong  wrote:

> Add the Video Clocks present on the Amlogic AXg SoCs.
>
> The AXG only has a single ENCL CTS clock and even if VCLK exist along VCLK2,
> only VCLK2 is used since it clocks the MIPI DSI IP directly.
>
> Signed-off-by: Neil Armstrong 
> ---
>  drivers/clk/meson/axg.c | 774 
>  drivers/clk/meson/axg.h |  21 +-
>  2 files changed, 794 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/clk/meson/axg.c b/drivers/clk/meson/axg.c
> index 13fc0006f63d..2550616c14b0 100644
> --- a/drivers/clk/meson/axg.c
> +++ b/drivers/clk/meson/axg.c
> @@ -1026,6 +1026,704 @@ static struct clk_regmap axg_sd_emmc_c_clk0 = {
>   },
>  };
>  
> +/* VPU Clock */
> +
> +static const struct clk_hw *axg_vpu_parent_hws[] = {
> + _fclk_div4.hw,
> + _fclk_div3.hw,
> + _fclk_div5.hw,
> + _fclk_div7.hw,
> +};
> +
> +static struct clk_regmap axg_vpu_0_sel = {
> + .data = &(struct clk_regmap_mux_data){
> + .offset = HHI_VPU_CLK_CNTL,
> + .mask = 0x3,
> + .shift = 9,
> + },
> + .hw.init = &(struct clk_init_data){
> + .name = "vpu_0_sel",
> + .ops = _regmap_mux_ops,
> + /*
> +  * bits 9:10 selects from 4 possible parents:
> +  * fclk_div4, fclk_div3, fclk_div5, fclk_div7,
> +  */

These comments (and the same bellow) are not very useful

> + .parent_hws = axg_vpu_parent_hws,
> + .num_parents = ARRAY_SIZE(axg_vpu_parent_hws),

Could you add a comment here explaining why parenting needs to be
manually controlled ?

> + .flags = CLK_SET_RATE_NO_REPARENT,
> + },
> +};
> +
> +static struct clk_regmap axg_vpu_0_div = {
> + .data = &(struct clk_regmap_div_data){
> + .offset = HHI_VPU_CLK_CNTL,
> + .shift = 0,
> + .width = 7,
> + },
> + .hw.init = &(struct clk_init_data){
> + .name = "vpu_0_div",
> + .ops = _regmap_divider_ops,
> + .parent_hws = (const struct clk_hw *[]) { _vpu_0_sel.hw },
> + .num_parents = 1,
> + .flags = CLK_SET_RATE_PARENT,
> + },
> +};
> +
> +static struct clk_regmap axg_vpu_0 = {
> + .data = &(struct clk_regmap_gate_data){
> + .offset = HHI_VPU_CLK_CNTL,
> + .bit_idx = 8,
> + },
> + .hw.init = &(struct clk_init_data) {
> + .name = "vpu_0",
> + .ops = _regmap_gate_ops,
> + .parent_hws = (const struct clk_hw *[]) { _vpu_0_div.hw },
> + .num_parents = 1,
> + .flags = CLK_SET_RATE_PARENT | CLK_IGNORE_UNUSED,

Why is UNUSED required ?

> + },
> +};
> +
> +static struct clk_regmap axg_vpu_1_sel = {
> + .data = &(struct clk_regmap_mux_data){
> + .offset = HHI_VPU_CLK_CNTL,
> + .mask = 0x3,
> + .shift = 25,
> + },
> + .hw.init = &(struct clk_init_data){
> + .name = "vpu_1_sel",
> + .ops = _regmap_mux_ops,
> + /*
> +  * bits 25:26 selects from 4 possible parents:
> +  * fclk_div4, fclk_div3, fclk_div5, fclk_div7,
> +  */
> + .parent_hws = axg_vpu_parent_hws,
> + .num_parents = ARRAY_SIZE(axg_vpu_parent_hws),
> + .flags = CLK_SET_RATE_NO_REPARENT,
> + },
> +};
> +
> +static struct clk_regmap axg_vpu_1_div = {
> + .data = &(struct clk_regmap_div_data){
> + .offset = HHI_VPU_CLK_CNTL,
> + .shift = 16,
> + .width = 7,
> + },
> + .hw.init = &(struct clk_init_data){
> + .name = "vpu_1_div",
> + .ops = _regmap_divider_ops,
> + .parent_hws = (const struct clk_hw *[]) { _vpu_1_sel.hw },
> + .num_parents = 1,
> + .flags = CLK_SET_RATE_PARENT,
> + },
> +};
> +
> +static struct clk_regmap axg_vpu_1 = {
> + .data = &(struct clk_regmap_gate_data){
> + .offset = HHI_VPU_CLK_CNTL,
> + .bit_idx = 24,
> + },
> + .hw.init = &(struct clk_init_data) {
> + .name = "vpu_1",
> + .ops = _regmap_gate_ops,
> + .parent_hws = (const struct clk_hw *[]) { _vpu_1_div.hw },
> + .num_parents = 1,
> + .flags = CLK_SET_RATE_PARENT | CLK_IGNORE_UNUSED,
> + },
> +};
> +
> +static struct clk_regmap axg_vpu = {
> + .data = &(struct clk_regmap_mux_data){
> + .offset = HHI_VPU_CLK_CNTL,
> + .mask = 1,
> + .shift = 31,
> + },
> + .hw.init = &(struct clk_init_data){
> + .name = "vpu",
> + .ops = _regmap_mux_ops,
> + /*
> +  * bit 31 selects from 2 possible parents:
> +  * vpu_0 or vpu_1
> +  */
> + .parent_hws = (const struct clk_hw *[]) {
> + _vpu_0.hw,
> +

[PATCH 3/4] clk: meson: axg: add Video Clocks

2020-09-07 Thread Neil Armstrong 
Add the Video Clocks present on the Amlogic AXg SoCs.

The AXG only has a single ENCL CTS clock and even if VCLK exist along VCLK2,
only VCLK2 is used since it clocks the MIPI DSI IP directly.

Signed-off-by: Neil Armstrong 
---
 drivers/clk/meson/axg.c | 774 
 drivers/clk/meson/axg.h |  21 +-
 2 files changed, 794 insertions(+), 1 deletion(-)

diff --git a/drivers/clk/meson/axg.c b/drivers/clk/meson/axg.c
index 13fc0006f63d..2550616c14b0 100644
--- a/drivers/clk/meson/axg.c
+++ b/drivers/clk/meson/axg.c
@@ -1026,6 +1026,704 @@ static struct clk_regmap axg_sd_emmc_c_clk0 = {
},
 };
 
+/* VPU Clock */
+
+static const struct clk_hw *axg_vpu_parent_hws[] = {
+   _fclk_div4.hw,
+   _fclk_div3.hw,
+   _fclk_div5.hw,
+   _fclk_div7.hw,
+};
+
+static struct clk_regmap axg_vpu_0_sel = {
+   .data = &(struct clk_regmap_mux_data){
+   .offset = HHI_VPU_CLK_CNTL,
+   .mask = 0x3,
+   .shift = 9,
+   },
+   .hw.init = &(struct clk_init_data){
+   .name = "vpu_0_sel",
+   .ops = _regmap_mux_ops,
+   /*
+* bits 9:10 selects from 4 possible parents:
+* fclk_div4, fclk_div3, fclk_div5, fclk_div7,
+*/
+   .parent_hws = axg_vpu_parent_hws,
+   .num_parents = ARRAY_SIZE(axg_vpu_parent_hws),
+   .flags = CLK_SET_RATE_NO_REPARENT,
+   },
+};
+
+static struct clk_regmap axg_vpu_0_div = {
+   .data = &(struct clk_regmap_div_data){
+   .offset = HHI_VPU_CLK_CNTL,
+   .shift = 0,
+   .width = 7,
+   },
+   .hw.init = &(struct clk_init_data){
+   .name = "vpu_0_div",
+   .ops = _regmap_divider_ops,
+   .parent_hws = (const struct clk_hw *[]) { _vpu_0_sel.hw },
+   .num_parents = 1,
+   .flags = CLK_SET_RATE_PARENT,
+   },
+};
+
+static struct clk_regmap axg_vpu_0 = {
+   .data = &(struct clk_regmap_gate_data){
+   .offset = HHI_VPU_CLK_CNTL,
+   .bit_idx = 8,
+   },
+   .hw.init = &(struct clk_init_data) {
+   .name = "vpu_0",
+   .ops = _regmap_gate_ops,
+   .parent_hws = (const struct clk_hw *[]) { _vpu_0_div.hw },
+   .num_parents = 1,
+   .flags = CLK_SET_RATE_PARENT | CLK_IGNORE_UNUSED,
+   },
+};
+
+static struct clk_regmap axg_vpu_1_sel = {
+   .data = &(struct clk_regmap_mux_data){
+   .offset = HHI_VPU_CLK_CNTL,
+   .mask = 0x3,
+   .shift = 25,
+   },
+   .hw.init = &(struct clk_init_data){
+   .name = "vpu_1_sel",
+   .ops = _regmap_mux_ops,
+   /*
+* bits 25:26 selects from 4 possible parents:
+* fclk_div4, fclk_div3, fclk_div5, fclk_div7,
+*/
+   .parent_hws = axg_vpu_parent_hws,
+   .num_parents = ARRAY_SIZE(axg_vpu_parent_hws),
+   .flags = CLK_SET_RATE_NO_REPARENT,
+   },
+};
+
+static struct clk_regmap axg_vpu_1_div = {
+   .data = &(struct clk_regmap_div_data){
+   .offset = HHI_VPU_CLK_CNTL,
+   .shift = 16,
+   .width = 7,
+   },
+   .hw.init = &(struct clk_init_data){
+   .name = "vpu_1_div",
+   .ops = _regmap_divider_ops,
+   .parent_hws = (const struct clk_hw *[]) { _vpu_1_sel.hw },
+   .num_parents = 1,
+   .flags = CLK_SET_RATE_PARENT,
+   },
+};
+
+static struct clk_regmap axg_vpu_1 = {
+   .data = &(struct clk_regmap_gate_data){
+   .offset = HHI_VPU_CLK_CNTL,
+   .bit_idx = 24,
+   },
+   .hw.init = &(struct clk_init_data) {
+   .name = "vpu_1",
+   .ops = _regmap_gate_ops,
+   .parent_hws = (const struct clk_hw *[]) { _vpu_1_div.hw },
+   .num_parents = 1,
+   .flags = CLK_SET_RATE_PARENT | CLK_IGNORE_UNUSED,
+   },
+};
+
+static struct clk_regmap axg_vpu = {
+   .data = &(struct clk_regmap_mux_data){
+   .offset = HHI_VPU_CLK_CNTL,
+   .mask = 1,
+   .shift = 31,
+   },
+   .hw.init = &(struct clk_init_data){
+   .name = "vpu",
+   .ops = _regmap_mux_ops,
+   /*
+* bit 31 selects from 2 possible parents:
+* vpu_0 or vpu_1
+*/
+   .parent_hws = (const struct clk_hw *[]) {
+   _vpu_0.hw,
+   _vpu_1.hw
+   },
+   .num_parents = 2,
+   .flags = CLK_SET_RATE_NO_REPARENT,
+   },
+};
+
+/* VAPB Clock */
+
+static const struct clk_hw *axg_vapb_parent_hws[] = {
+   _fclk_div4.hw,
+   _fclk_div3.hw,
+   _fclk_div5.hw,
+   _fclk_div7.hw,
+};
+
+static struct