[PATCH 0/6] Improve clock support for Actions S500 SoC

[PATCH 0/6] Improve clock support for Actions S500 SoC

[Cristian Ciocaltea]

While working on a driver to support the Actions Semi Owl Ethernet MAC,
I found and fixed some issues on the existing implementation of the S500
SoC clock subsystem and, additionally, I added two missing clocks.

Thanks,
Cristi

Cristian Ciocaltea (6):
  clk: actions: Fix UART clock dividers on Owl S500 SoC
  clk: actions: Fix SD clocks factor table on Owl S500 SoC
  clk: actions: Fix bisp_factor_table based clocks on Owl S500 SoC
  clk: actions: Fix AHPPREDIV-H-AHB clock chain on Owl S500 SoC
  dt-bindings: clock: Add NIC and ETHERNET bindings for Actions S500 SoC
  clk: actions: Add NIC and ETHERNET clock support for Actions S500 SoC

 drivers/clk/actions/owl-s500.c               | 99 +++++++++++++-------
 include/dt-bindings/clock/actions,s500-cmu.h |  6 +-
 2 files changed, 70 insertions(+), 35 deletions(-)

-- 
2.30.1

[PATCH 1/6] clk: actions: Fix UART clock dividers on Owl S500 SoC

[Cristian Ciocaltea]

Use correct divider registers for the Actions Semi Owl S500 SoC's UART
clocks.

Fixes: ed6b4795ece4 ("clk: actions: Add clock driver for S500 SoC")
Signed-off-by: Cristian Ciocaltea <cristian.ciocaltea@gmail.com>
---
 drivers/clk/actions/owl-s500.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/drivers/clk/actions/owl-s500.c b/drivers/clk/actions/owl-s500.c
index 61bb224f6330..75b7186185b0 100644
--- a/drivers/clk/actions/owl-s500.c
+++ b/drivers/clk/actions/owl-s500.c
@@ -305,7 +305,7 @@ static OWL_COMP_FIXED_FACTOR(i2c3_clk, "i2c3_clk", "ethernet_pll_clk",
 static OWL_COMP_DIV(uart0_clk, "uart0_clk", uart_clk_mux_p,
 			OWL_MUX_HW(CMU_UART0CLK, 16, 1),
 			OWL_GATE_HW(CMU_DEVCLKEN1, 6, 0),
-			OWL_DIVIDER_HW(CMU_UART1CLK, 0, 8, CLK_DIVIDER_ROUND_CLOSEST, NULL),
+			OWL_DIVIDER_HW(CMU_UART0CLK, 0, 8, CLK_DIVIDER_ROUND_CLOSEST, NULL),
 			CLK_IGNORE_UNUSED);
 
 static OWL_COMP_DIV(uart1_clk, "uart1_clk", uart_clk_mux_p,
@@ -317,31 +317,31 @@ static OWL_COMP_DIV(uart1_clk, "uart1_clk", uart_clk_mux_p,
 static OWL_COMP_DIV(uart2_clk, "uart2_clk", uart_clk_mux_p,
 			OWL_MUX_HW(CMU_UART2CLK, 16, 1),
 			OWL_GATE_HW(CMU_DEVCLKEN1, 8, 0),
-			OWL_DIVIDER_HW(CMU_UART1CLK, 0, 8, CLK_DIVIDER_ROUND_CLOSEST, NULL),
+			OWL_DIVIDER_HW(CMU_UART2CLK, 0, 8, CLK_DIVIDER_ROUND_CLOSEST, NULL),
 			CLK_IGNORE_UNUSED);
 
 static OWL_COMP_DIV(uart3_clk, "uart3_clk", uart_clk_mux_p,
 			OWL_MUX_HW(CMU_UART3CLK, 16, 1),
 			OWL_GATE_HW(CMU_DEVCLKEN1, 19, 0),
-			OWL_DIVIDER_HW(CMU_UART1CLK, 0, 8, CLK_DIVIDER_ROUND_CLOSEST, NULL),
+			OWL_DIVIDER_HW(CMU_UART3CLK, 0, 8, CLK_DIVIDER_ROUND_CLOSEST, NULL),
 			CLK_IGNORE_UNUSED);
 
 static OWL_COMP_DIV(uart4_clk, "uart4_clk", uart_clk_mux_p,
 			OWL_MUX_HW(CMU_UART4CLK, 16, 1),
 			OWL_GATE_HW(CMU_DEVCLKEN1, 20, 0),
-			OWL_DIVIDER_HW(CMU_UART1CLK, 0, 8, CLK_DIVIDER_ROUND_CLOSEST, NULL),
+			OWL_DIVIDER_HW(CMU_UART4CLK, 0, 8, CLK_DIVIDER_ROUND_CLOSEST, NULL),
 			CLK_IGNORE_UNUSED);
 
 static OWL_COMP_DIV(uart5_clk, "uart5_clk", uart_clk_mux_p,
 			OWL_MUX_HW(CMU_UART5CLK, 16, 1),
 			OWL_GATE_HW(CMU_DEVCLKEN1, 21, 0),
-			OWL_DIVIDER_HW(CMU_UART1CLK, 0, 8, CLK_DIVIDER_ROUND_CLOSEST, NULL),
+			OWL_DIVIDER_HW(CMU_UART5CLK, 0, 8, CLK_DIVIDER_ROUND_CLOSEST, NULL),
 			CLK_IGNORE_UNUSED);
 
 static OWL_COMP_DIV(uart6_clk, "uart6_clk", uart_clk_mux_p,
 			OWL_MUX_HW(CMU_UART6CLK, 16, 1),
 			OWL_GATE_HW(CMU_DEVCLKEN1, 18, 0),
-			OWL_DIVIDER_HW(CMU_UART1CLK, 0, 8, CLK_DIVIDER_ROUND_CLOSEST, NULL),
+			OWL_DIVIDER_HW(CMU_UART6CLK, 0, 8, CLK_DIVIDER_ROUND_CLOSEST, NULL),
 			CLK_IGNORE_UNUSED);
 
 static OWL_COMP_DIV(i2srx_clk, "i2srx_clk", i2s_clk_mux_p,
-- 
2.30.1

Re: [PATCH 1/6] clk: actions: Fix UART clock dividers on Owl S500 SoC

[Manivannan Sadhasivam]

On Mon, Mar 08, 2021 at 07:18:26PM +0200, Cristian Ciocaltea wrote:
> Use correct divider registers for the Actions Semi Owl S500 SoC's UART
> clocks.
> 
> Fixes: ed6b4795ece4 ("clk: actions: Add clock driver for S500 SoC")
> Signed-off-by: Cristian Ciocaltea <cristian.ciocaltea@gmail.com>

Reviewed-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>

Thanks,
Mani

> ---
>  drivers/clk/actions/owl-s500.c | 12 ++++++------
>  1 file changed, 6 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/clk/actions/owl-s500.c b/drivers/clk/actions/owl-s500.c
> index 61bb224f6330..75b7186185b0 100644
> --- a/drivers/clk/actions/owl-s500.c
> +++ b/drivers/clk/actions/owl-s500.c
> @@ -305,7 +305,7 @@ static OWL_COMP_FIXED_FACTOR(i2c3_clk, "i2c3_clk", "ethernet_pll_clk",
>  static OWL_COMP_DIV(uart0_clk, "uart0_clk", uart_clk_mux_p,
>  			OWL_MUX_HW(CMU_UART0CLK, 16, 1),
>  			OWL_GATE_HW(CMU_DEVCLKEN1, 6, 0),
> -			OWL_DIVIDER_HW(CMU_UART1CLK, 0, 8, CLK_DIVIDER_ROUND_CLOSEST, NULL),
> +			OWL_DIVIDER_HW(CMU_UART0CLK, 0, 8, CLK_DIVIDER_ROUND_CLOSEST, NULL),
>  			CLK_IGNORE_UNUSED);
>  
>  static OWL_COMP_DIV(uart1_clk, "uart1_clk", uart_clk_mux_p,
> @@ -317,31 +317,31 @@ static OWL_COMP_DIV(uart1_clk, "uart1_clk", uart_clk_mux_p,
>  static OWL_COMP_DIV(uart2_clk, "uart2_clk", uart_clk_mux_p,
>  			OWL_MUX_HW(CMU_UART2CLK, 16, 1),
>  			OWL_GATE_HW(CMU_DEVCLKEN1, 8, 0),
> -			OWL_DIVIDER_HW(CMU_UART1CLK, 0, 8, CLK_DIVIDER_ROUND_CLOSEST, NULL),
> +			OWL_DIVIDER_HW(CMU_UART2CLK, 0, 8, CLK_DIVIDER_ROUND_CLOSEST, NULL),
>  			CLK_IGNORE_UNUSED);
>  
>  static OWL_COMP_DIV(uart3_clk, "uart3_clk", uart_clk_mux_p,
>  			OWL_MUX_HW(CMU_UART3CLK, 16, 1),
>  			OWL_GATE_HW(CMU_DEVCLKEN1, 19, 0),
> -			OWL_DIVIDER_HW(CMU_UART1CLK, 0, 8, CLK_DIVIDER_ROUND_CLOSEST, NULL),
> +			OWL_DIVIDER_HW(CMU_UART3CLK, 0, 8, CLK_DIVIDER_ROUND_CLOSEST, NULL),
>  			CLK_IGNORE_UNUSED);
>  
>  static OWL_COMP_DIV(uart4_clk, "uart4_clk", uart_clk_mux_p,
>  			OWL_MUX_HW(CMU_UART4CLK, 16, 1),
>  			OWL_GATE_HW(CMU_DEVCLKEN1, 20, 0),
> -			OWL_DIVIDER_HW(CMU_UART1CLK, 0, 8, CLK_DIVIDER_ROUND_CLOSEST, NULL),
> +			OWL_DIVIDER_HW(CMU_UART4CLK, 0, 8, CLK_DIVIDER_ROUND_CLOSEST, NULL),
>  			CLK_IGNORE_UNUSED);
>  
>  static OWL_COMP_DIV(uart5_clk, "uart5_clk", uart_clk_mux_p,
>  			OWL_MUX_HW(CMU_UART5CLK, 16, 1),
>  			OWL_GATE_HW(CMU_DEVCLKEN1, 21, 0),
> -			OWL_DIVIDER_HW(CMU_UART1CLK, 0, 8, CLK_DIVIDER_ROUND_CLOSEST, NULL),
> +			OWL_DIVIDER_HW(CMU_UART5CLK, 0, 8, CLK_DIVIDER_ROUND_CLOSEST, NULL),
>  			CLK_IGNORE_UNUSED);
>  
>  static OWL_COMP_DIV(uart6_clk, "uart6_clk", uart_clk_mux_p,
>  			OWL_MUX_HW(CMU_UART6CLK, 16, 1),
>  			OWL_GATE_HW(CMU_DEVCLKEN1, 18, 0),
> -			OWL_DIVIDER_HW(CMU_UART1CLK, 0, 8, CLK_DIVIDER_ROUND_CLOSEST, NULL),
> +			OWL_DIVIDER_HW(CMU_UART6CLK, 0, 8, CLK_DIVIDER_ROUND_CLOSEST, NULL),
>  			CLK_IGNORE_UNUSED);
>  
>  static OWL_COMP_DIV(i2srx_clk, "i2srx_clk", i2s_clk_mux_p,
> -- 
> 2.30.1
> 

[PATCH 2/6] clk: actions: Fix SD clocks factor table on Owl S500 SoC

[Cristian Ciocaltea]

Drop the unsupported entries in the factor table used for the SD[0-2]
clocks definitions on the Actions Semi Owl S500 SoC.

Fixes: ed6b4795ece4 ("clk: actions: Add clock driver for S500 SoC")
Signed-off-by: Cristian Ciocaltea <cristian.ciocaltea@gmail.com>
---
 drivers/clk/actions/owl-s500.c | 4 ----
 1 file changed, 4 deletions(-)

diff --git a/drivers/clk/actions/owl-s500.c b/drivers/clk/actions/owl-s500.c
index 75b7186185b0..69cd959205f5 100644
--- a/drivers/clk/actions/owl-s500.c
+++ b/drivers/clk/actions/owl-s500.c
@@ -127,8 +127,6 @@ static struct clk_factor_table sd_factor_table[] = {
 	{ 12, 1, 13 }, { 13, 1, 14 }, { 14, 1, 15 }, { 15, 1, 16 },
 	{ 16, 1, 17 }, { 17, 1, 18 }, { 18, 1, 19 }, { 19, 1, 20 },
 	{ 20, 1, 21 }, { 21, 1, 22 }, { 22, 1, 23 }, { 23, 1, 24 },
-	{ 24, 1, 25 }, { 25, 1, 26 }, { 26, 1, 27 }, { 27, 1, 28 },
-	{ 28, 1, 29 }, { 29, 1, 30 }, { 30, 1, 31 }, { 31, 1, 32 },
 
 	/* bit8: /128 */
 	{ 256, 1, 1 * 128 }, { 257, 1, 2 * 128 }, { 258, 1, 3 * 128 }, { 259, 1, 4 * 128 },
@@ -137,8 +135,6 @@ static struct clk_factor_table sd_factor_table[] = {
 	{ 268, 1, 13 * 128 }, { 269, 1, 14 * 128 }, { 270, 1, 15 * 128 }, { 271, 1, 16 * 128 },
 	{ 272, 1, 17 * 128 }, { 273, 1, 18 * 128 }, { 274, 1, 19 * 128 }, { 275, 1, 20 * 128 },
 	{ 276, 1, 21 * 128 }, { 277, 1, 22 * 128 }, { 278, 1, 23 * 128 }, { 279, 1, 24 * 128 },
-	{ 280, 1, 25 * 128 }, { 281, 1, 26 * 128 }, { 282, 1, 27 * 128 }, { 283, 1, 28 * 128 },
-	{ 284, 1, 29 * 128 }, { 285, 1, 30 * 128 }, { 286, 1, 31 * 128 }, { 287, 1, 32 * 128 },
 	{ 0, 0, 0 },
 };
 
-- 
2.30.1

Re: [PATCH 2/6] clk: actions: Fix SD clocks factor table on Owl S500 SoC

[Manivannan Sadhasivam]

On Mon, Mar 08, 2021 at 07:18:27PM +0200, Cristian Ciocaltea wrote:
> Drop the unsupported entries in the factor table used for the SD[0-2]
> clocks definitions on the Actions Semi Owl S500 SoC.
> 
> Fixes: ed6b4795ece4 ("clk: actions: Add clock driver for S500 SoC")
> Signed-off-by: Cristian Ciocaltea <cristian.ciocaltea@gmail.com>
> ---
>  drivers/clk/actions/owl-s500.c | 4 ----
>  1 file changed, 4 deletions(-)
> 
> diff --git a/drivers/clk/actions/owl-s500.c b/drivers/clk/actions/owl-s500.c
> index 75b7186185b0..69cd959205f5 100644
> --- a/drivers/clk/actions/owl-s500.c
> +++ b/drivers/clk/actions/owl-s500.c
> @@ -127,8 +127,6 @@ static struct clk_factor_table sd_factor_table[] = {
>  	{ 12, 1, 13 }, { 13, 1, 14 }, { 14, 1, 15 }, { 15, 1, 16 },
>  	{ 16, 1, 17 }, { 17, 1, 18 }, { 18, 1, 19 }, { 19, 1, 20 },
>  	{ 20, 1, 21 }, { 21, 1, 22 }, { 22, 1, 23 }, { 23, 1, 24 },
> -	{ 24, 1, 25 }, { 25, 1, 26 }, { 26, 1, 27 }, { 27, 1, 28 },
> -	{ 28, 1, 29 }, { 29, 1, 30 }, { 30, 1, 31 }, { 31, 1, 32 },

How did you determine that these values are not supported?

I've seen cases where the datasheet has the incomplete information about the
supported ranges but the downstream driver has everything.

Thanks,
Mani

>  
>  	/* bit8: /128 */
>  	{ 256, 1, 1 * 128 }, { 257, 1, 2 * 128 }, { 258, 1, 3 * 128 }, { 259, 1, 4 * 128 },
> @@ -137,8 +135,6 @@ static struct clk_factor_table sd_factor_table[] = {
>  	{ 268, 1, 13 * 128 }, { 269, 1, 14 * 128 }, { 270, 1, 15 * 128 }, { 271, 1, 16 * 128 },
>  	{ 272, 1, 17 * 128 }, { 273, 1, 18 * 128 }, { 274, 1, 19 * 128 }, { 275, 1, 20 * 128 },
>  	{ 276, 1, 21 * 128 }, { 277, 1, 22 * 128 }, { 278, 1, 23 * 128 }, { 279, 1, 24 * 128 },
> -	{ 280, 1, 25 * 128 }, { 281, 1, 26 * 128 }, { 282, 1, 27 * 128 }, { 283, 1, 28 * 128 },
> -	{ 284, 1, 29 * 128 }, { 285, 1, 30 * 128 }, { 286, 1, 31 * 128 }, { 287, 1, 32 * 128 },
>  	{ 0, 0, 0 },
>  };
>  
> -- 
> 2.30.1
> 

Re: [PATCH 2/6] clk: actions: Fix SD clocks factor table on Owl S500 SoC

[Cristian Ciocaltea]

Hi Mani,

Thanks for reviewing this patch series!

On Tue, Mar 16, 2021 at 09:28:45AM +0530, Manivannan Sadhasivam wrote:
> On Mon, Mar 08, 2021 at 07:18:27PM +0200, Cristian Ciocaltea wrote:
> > Drop the unsupported entries in the factor table used for the SD[0-2]
> > clocks definitions on the Actions Semi Owl S500 SoC.
> > 
> > Fixes: ed6b4795ece4 ("clk: actions: Add clock driver for S500 SoC")
> > Signed-off-by: Cristian Ciocaltea <cristian.ciocaltea@gmail.com>
> > ---
> >  drivers/clk/actions/owl-s500.c | 4 ----
> >  1 file changed, 4 deletions(-)
> > 
> > diff --git a/drivers/clk/actions/owl-s500.c b/drivers/clk/actions/owl-s500.c
> > index 75b7186185b0..69cd959205f5 100644
> > --- a/drivers/clk/actions/owl-s500.c
> > +++ b/drivers/clk/actions/owl-s500.c
> > @@ -127,8 +127,6 @@ static struct clk_factor_table sd_factor_table[] = {
> >  	{ 12, 1, 13 }, { 13, 1, 14 }, { 14, 1, 15 }, { 15, 1, 16 },
> >  	{ 16, 1, 17 }, { 17, 1, 18 }, { 18, 1, 19 }, { 19, 1, 20 },
> >  	{ 20, 1, 21 }, { 21, 1, 22 }, { 22, 1, 23 }, { 23, 1, 24 },
> > -	{ 24, 1, 25 }, { 25, 1, 26 }, { 26, 1, 27 }, { 27, 1, 28 },
> > -	{ 28, 1, 29 }, { 29, 1, 30 }, { 30, 1, 31 }, { 31, 1, 32 },
> 
> How did you determine that these values are not supported?
> 
> I've seen cases where the datasheet has the incomplete information about the
> supported ranges but the downstream driver has everything.

My primary source of information is the xapp-le kernel source code:
https://github.com/xapp-le/kernel

I always try to double check the implementation with the information
in the datasheet, but sometimes, as you already pointed out, it is
incomplete.

For the SD clocks, it is even worse: there is absolutely no information
related to the CMU_SD[0-2]CLK registers. Therefore I had to rely
exclusively on the downstream driver code.

Hence, for the SD1 clock, I identified the following code snippets:

static struct owl_clkreq divbit_PRESD0_CLK          = BITMAP(CMU_SD0CLK,                0x0000001f, 0);
static struct owl_clkreq divbit_SD0_CLK_2X          = BITMAP(CMU_SD0CLK,                0x00000100, 8);
static struct owl_refertab T_sdx2   = {{1, 128, -1}, 0};

static struct owl_div divider_PRESD0_CLK = {
    .type = DIV_T_NATURE,
    .range_from = 0,
    .range_to = 24,
    .reg = &divbit_PRESD0_CLK,
};

static struct owl_div divider_SD0_CLK_2X = {
    .type = DIV_T_TABLE,
    .range_from = 0,
    .range_to = 1,
    .ext = {.tab = &T_sdx2,},
    .reg = &divbit_SD0_CLK_2X,
};

This is basically what gets translated to sd_factor_table and I removed
the extra entries 25..31. Actually I also dropped the 24th one, since
that would give us an odd number of items, although I'm not quite sure
this is a bug in the xapp-le code or the HW is really supposed to work
like that.

Kind regards,
Cristi

> Thanks,
> Mani
> 
> >  
> >  	/* bit8: /128 */
> >  	{ 256, 1, 1 * 128 }, { 257, 1, 2 * 128 }, { 258, 1, 3 * 128 }, { 259, 1, 4 * 128 },
> > @@ -137,8 +135,6 @@ static struct clk_factor_table sd_factor_table[] = {
> >  	{ 268, 1, 13 * 128 }, { 269, 1, 14 * 128 }, { 270, 1, 15 * 128 }, { 271, 1, 16 * 128 },
> >  	{ 272, 1, 17 * 128 }, { 273, 1, 18 * 128 }, { 274, 1, 19 * 128 }, { 275, 1, 20 * 128 },
> >  	{ 276, 1, 21 * 128 }, { 277, 1, 22 * 128 }, { 278, 1, 23 * 128 }, { 279, 1, 24 * 128 },
> > -	{ 280, 1, 25 * 128 }, { 281, 1, 26 * 128 }, { 282, 1, 27 * 128 }, { 283, 1, 28 * 128 },
> > -	{ 284, 1, 29 * 128 }, { 285, 1, 30 * 128 }, { 286, 1, 31 * 128 }, { 287, 1, 32 * 128 },
> >  	{ 0, 0, 0 },
> >  };
> >  
> > -- 
> > 2.30.1
> > 

Re: [PATCH 2/6] clk: actions: Fix SD clocks factor table on Owl S500 SoC

[Manivannan Sadhasivam]

On Tue, Mar 16, 2021 at 08:14:37PM +0200, Cristian Ciocaltea wrote:
> Hi Mani,
> 
> Thanks for reviewing this patch series!
> 
> On Tue, Mar 16, 2021 at 09:28:45AM +0530, Manivannan Sadhasivam wrote:
> > On Mon, Mar 08, 2021 at 07:18:27PM +0200, Cristian Ciocaltea wrote:
> > > Drop the unsupported entries in the factor table used for the SD[0-2]
> > > clocks definitions on the Actions Semi Owl S500 SoC.
> > > 
> > > Fixes: ed6b4795ece4 ("clk: actions: Add clock driver for S500 SoC")
> > > Signed-off-by: Cristian Ciocaltea <cristian.ciocaltea@gmail.com>
> > > ---
> > >  drivers/clk/actions/owl-s500.c | 4 ----
> > >  1 file changed, 4 deletions(-)
> > > 
> > > diff --git a/drivers/clk/actions/owl-s500.c b/drivers/clk/actions/owl-s500.c
> > > index 75b7186185b0..69cd959205f5 100644
> > > --- a/drivers/clk/actions/owl-s500.c
> > > +++ b/drivers/clk/actions/owl-s500.c
> > > @@ -127,8 +127,6 @@ static struct clk_factor_table sd_factor_table[] = {
> > >  	{ 12, 1, 13 }, { 13, 1, 14 }, { 14, 1, 15 }, { 15, 1, 16 },
> > >  	{ 16, 1, 17 }, { 17, 1, 18 }, { 18, 1, 19 }, { 19, 1, 20 },
> > >  	{ 20, 1, 21 }, { 21, 1, 22 }, { 22, 1, 23 }, { 23, 1, 24 },
> > > -	{ 24, 1, 25 }, { 25, 1, 26 }, { 26, 1, 27 }, { 27, 1, 28 },
> > > -	{ 28, 1, 29 }, { 29, 1, 30 }, { 30, 1, 31 }, { 31, 1, 32 },
> > 

[...]

> This is basically what gets translated to sd_factor_table and I removed
> the extra entries 25..31. Actually I also dropped the 24th one, since
> that would give us an odd number of items, although I'm not quite sure
> this is a bug in the xapp-le code or the HW is really supposed to work
> like that.
> 

In my datasheet I can see the factor values till 24. So let's remove the
entries from 25-31.

Thanks,
Mani

> Kind regards,
> Cristi
> 
> > Thanks,
> > Mani
> > 
> > >  
> > >  	/* bit8: /128 */
> > >  	{ 256, 1, 1 * 128 }, { 257, 1, 2 * 128 }, { 258, 1, 3 * 128 }, { 259, 1, 4 * 128 },
> > > @@ -137,8 +135,6 @@ static struct clk_factor_table sd_factor_table[] = {
> > >  	{ 268, 1, 13 * 128 }, { 269, 1, 14 * 128 }, { 270, 1, 15 * 128 }, { 271, 1, 16 * 128 },
> > >  	{ 272, 1, 17 * 128 }, { 273, 1, 18 * 128 }, { 274, 1, 19 * 128 }, { 275, 1, 20 * 128 },
> > >  	{ 276, 1, 21 * 128 }, { 277, 1, 22 * 128 }, { 278, 1, 23 * 128 }, { 279, 1, 24 * 128 },
> > > -	{ 280, 1, 25 * 128 }, { 281, 1, 26 * 128 }, { 282, 1, 27 * 128 }, { 283, 1, 28 * 128 },
> > > -	{ 284, 1, 29 * 128 }, { 285, 1, 30 * 128 }, { 286, 1, 31 * 128 }, { 287, 1, 32 * 128 },
> > >  	{ 0, 0, 0 },
> > >  };
> > >  
> > > -- 
> > > 2.30.1
> > > 

Re: [PATCH 2/6] clk: actions: Fix SD clocks factor table on Owl S500 SoC

[Cristian Ciocaltea]

On Wed, May 26, 2021 at 03:37:43PM +0530, Manivannan Sadhasivam wrote:
> On Tue, Mar 16, 2021 at 08:14:37PM +0200, Cristian Ciocaltea wrote:
> > Hi Mani,
> > 
> > Thanks for reviewing this patch series!
> > 
> > On Tue, Mar 16, 2021 at 09:28:45AM +0530, Manivannan Sadhasivam wrote:
> > > On Mon, Mar 08, 2021 at 07:18:27PM +0200, Cristian Ciocaltea wrote:
> > > > Drop the unsupported entries in the factor table used for the SD[0-2]
> > > > clocks definitions on the Actions Semi Owl S500 SoC.
> > > > 
> > > > Fixes: ed6b4795ece4 ("clk: actions: Add clock driver for S500 SoC")
> > > > Signed-off-by: Cristian Ciocaltea <cristian.ciocaltea@gmail.com>
> > > > ---
> > > >  drivers/clk/actions/owl-s500.c | 4 ----
> > > >  1 file changed, 4 deletions(-)
> > > > 
> > > > diff --git a/drivers/clk/actions/owl-s500.c b/drivers/clk/actions/owl-s500.c
> > > > index 75b7186185b0..69cd959205f5 100644
> > > > --- a/drivers/clk/actions/owl-s500.c
> > > > +++ b/drivers/clk/actions/owl-s500.c
> > > > @@ -127,8 +127,6 @@ static struct clk_factor_table sd_factor_table[] = {
> > > >  	{ 12, 1, 13 }, { 13, 1, 14 }, { 14, 1, 15 }, { 15, 1, 16 },
> > > >  	{ 16, 1, 17 }, { 17, 1, 18 }, { 18, 1, 19 }, { 19, 1, 20 },
> > > >  	{ 20, 1, 21 }, { 21, 1, 22 }, { 22, 1, 23 }, { 23, 1, 24 },
> > > > -	{ 24, 1, 25 }, { 25, 1, 26 }, { 26, 1, 27 }, { 27, 1, 28 },
> > > > -	{ 28, 1, 29 }, { 29, 1, 30 }, { 30, 1, 31 }, { 31, 1, 32 },
> > > 
> 
> [...]
> 
> > This is basically what gets translated to sd_factor_table and I removed
> > the extra entries 25..31. Actually I also dropped the 24th one, since
> > that would give us an odd number of items, although I'm not quite sure
> > this is a bug in the xapp-le code or the HW is really supposed to work
> > like that.
> > 
> 
> In my datasheet I can see the factor values till 24. So let's remove the
> entries from 25-31.

I got an updated datasheet and I confirm 24 is a valid selector. Applied
the correction in v2:

https://lore.kernel.org/lkml/cover.1622119892.git.cristian.ciocaltea@gmail.com/

Thanks,
Cristi

> Thanks,
> Mani
> 
> > Kind regards,
> > Cristi
> > 
> > > Thanks,
> > > Mani
> > > 
> > > >  
> > > >  	/* bit8: /128 */
> > > >  	{ 256, 1, 1 * 128 }, { 257, 1, 2 * 128 }, { 258, 1, 3 * 128 }, { 259, 1, 4 * 128 },
> > > > @@ -137,8 +135,6 @@ static struct clk_factor_table sd_factor_table[] = {
> > > >  	{ 268, 1, 13 * 128 }, { 269, 1, 14 * 128 }, { 270, 1, 15 * 128 }, { 271, 1, 16 * 128 },
> > > >  	{ 272, 1, 17 * 128 }, { 273, 1, 18 * 128 }, { 274, 1, 19 * 128 }, { 275, 1, 20 * 128 },
> > > >  	{ 276, 1, 21 * 128 }, { 277, 1, 22 * 128 }, { 278, 1, 23 * 128 }, { 279, 1, 24 * 128 },
> > > > -	{ 280, 1, 25 * 128 }, { 281, 1, 26 * 128 }, { 282, 1, 27 * 128 }, { 283, 1, 28 * 128 },
> > > > -	{ 284, 1, 29 * 128 }, { 285, 1, 30 * 128 }, { 286, 1, 31 * 128 }, { 287, 1, 32 * 128 },
> > > >  	{ 0, 0, 0 },
> > > >  };
> > > >  
> > > > -- 
> > > > 2.30.1
> > > > 

[PATCH 3/6] clk: actions: Fix bisp_factor_table based clocks on Owl S500 SoC

[Cristian Ciocaltea]

The following clocks of the Actions Semi Owl S500 SoC have been defined
to use a shared clock factor table 'bisp_factor_table[]': DE[1-2], VCE,
VDE, BISP, SENSOR[0-1]

There are several issues involved in this approach:

* 'bisp_factor_table[]' describes the configuration of a regular 8-rates
  divider, so its usage is redundant. Additionally, judging by the BISP
  clock context, it is incomplete since it maps only 8 out of 12
  possible entries.

* The clocks mentioned above are not identical in terms of the available
  rates, therefore cannot rely on the same factor table. Specifically,
  BISP and SENSOR* are standard 12-rate dividers so their configuration
  should rely on a proper clock div table, while VCE and VDE require a
  factor table that is a actually a subset of the one needed for DE[1-2]
  clocks.

Let's fix this by implementing the following:

* Add new factor tables 'de_factor_table' and 'hde_factor_table' to
  properly handle DE[1-2], VCE and VDE clocks.

* Add a common div table 'std12rate_div_table' for BISP and SENSOR[0-1]
  clocks converted to OWL_COMP_DIV.

* Drop the now unused 'bisp_factor_table[]'.

Additionally, since SENSOR[0-1] are not gated, unset the OWL_GATE_HW
configuration and drop the CLK_IGNORE_UNUSED flag in their definitions.

Fixes: ed6b4795ece4 ("clk: actions: Add clock driver for S500 SoC")
Signed-off-by: Cristian Ciocaltea <cristian.ciocaltea@gmail.com>
---
 drivers/clk/actions/owl-s500.c | 48 ++++++++++++++++++++++------------
 1 file changed, 31 insertions(+), 17 deletions(-)

diff --git a/drivers/clk/actions/owl-s500.c b/drivers/clk/actions/owl-s500.c
index 69cd959205f5..abe8874353de 100644
--- a/drivers/clk/actions/owl-s500.c
+++ b/drivers/clk/actions/owl-s500.c
@@ -138,9 +138,16 @@ static struct clk_factor_table sd_factor_table[] = {
 	{ 0, 0, 0 },
 };
 
-static struct clk_factor_table bisp_factor_table[] = {
-	{ 0, 1, 1 }, { 1, 1, 2 }, { 2, 1, 3 }, { 3, 1, 4 },
-	{ 4, 1, 5 }, { 5, 1, 6 }, { 6, 1, 7 }, { 7, 1, 8 },
+static struct clk_factor_table de_factor_table[] = {
+	{ 0, 1, 1 }, { 1, 2, 3 }, { 2, 1, 2 }, { 3, 2, 5 },
+	{ 4, 1, 3 }, { 5, 1, 4 }, { 6, 1, 6 }, { 7, 1, 8 },
+	{ 8, 1, 12 },
+	{ 0, 0, 0 },
+};
+
+static struct clk_factor_table hde_factor_table[] = {
+	{ 0, 1, 1 }, { 1, 2, 3 }, { 2, 1, 2 }, { 3, 2, 5 },
+	{ 4, 1, 3 }, { 5, 1, 4 }, { 6, 1, 6 }, { 7, 1, 8 },
 	{ 0, 0, 0 },
 };
 
@@ -154,6 +161,13 @@ static struct clk_div_table rmii_ref_div_table[] = {
 	{ 0, 0 },
 };
 
+static struct clk_div_table std12rate_div_table[] = {
+	{ 0, 1 }, { 1, 2 }, { 2, 3 }, { 3, 4 },
+	{ 4, 5 }, { 5, 6 }, { 6, 7 }, { 7, 8 },
+	{ 8, 9 }, { 9, 10 }, { 10, 11 }, { 11, 12 },
+	{ 0, 0 },
+};
+
 static struct clk_div_table i2s_div_table[] = {
 	{ 0, 1 }, { 1, 2 }, { 2, 3 }, { 3, 4 },
 	{ 4, 6 }, { 5, 8 }, { 6, 12 }, { 7, 16 },
@@ -189,39 +203,39 @@ static OWL_DIVIDER(rmii_ref_clk, "rmii_ref_clk", "ethernet_pll_clk", CMU_ETHERNE
 
 /* factor clocks */
 static OWL_FACTOR(ahb_clk, "ahb_clk", "h_clk", CMU_BUSCLK1, 2, 2, ahb_factor_table, 0, 0);
-static OWL_FACTOR(de1_clk, "de_clk1", "de_clk", CMU_DECLK, 0, 3, bisp_factor_table, 0, 0);
-static OWL_FACTOR(de2_clk, "de_clk2", "de_clk", CMU_DECLK, 4, 3, bisp_factor_table, 0, 0);
+static OWL_FACTOR(de1_clk, "de_clk1", "de_clk", CMU_DECLK, 0, 4, de_factor_table, 0, 0);
+static OWL_FACTOR(de2_clk, "de_clk2", "de_clk", CMU_DECLK, 4, 4, de_factor_table, 0, 0);
 
 /* composite clocks */
 static OWL_COMP_FACTOR(vce_clk, "vce_clk", hde_clk_mux_p,
 			OWL_MUX_HW(CMU_VCECLK, 4, 2),
 			OWL_GATE_HW(CMU_DEVCLKEN0, 26, 0),
-			OWL_FACTOR_HW(CMU_VCECLK, 0, 3, 0, bisp_factor_table),
+			OWL_FACTOR_HW(CMU_VCECLK, 0, 3, 0, hde_factor_table),
 			0);
 
 static OWL_COMP_FACTOR(vde_clk, "vde_clk", hde_clk_mux_p,
 			OWL_MUX_HW(CMU_VDECLK, 4, 2),
 			OWL_GATE_HW(CMU_DEVCLKEN0, 25, 0),
-			OWL_FACTOR_HW(CMU_VDECLK, 0, 3, 0, bisp_factor_table),
+			OWL_FACTOR_HW(CMU_VDECLK, 0, 3, 0, hde_factor_table),
 			0);
 
-static OWL_COMP_FACTOR(bisp_clk, "bisp_clk", bisp_clk_mux_p,
+static OWL_COMP_DIV(bisp_clk, "bisp_clk", bisp_clk_mux_p,
 			OWL_MUX_HW(CMU_BISPCLK, 4, 1),
 			OWL_GATE_HW(CMU_DEVCLKEN0, 14, 0),
-			OWL_FACTOR_HW(CMU_BISPCLK, 0, 3, 0, bisp_factor_table),
+			OWL_DIVIDER_HW(CMU_BISPCLK, 0, 4, 0, std12rate_div_table),
 			0);
 
-static OWL_COMP_FACTOR(sensor0_clk, "sensor0_clk", sensor_clk_mux_p,
+static OWL_COMP_DIV(sensor0_clk, "sensor0_clk", sensor_clk_mux_p,
 			OWL_MUX_HW(CMU_SENSORCLK, 4, 1),
-			OWL_GATE_HW(CMU_DEVCLKEN0, 14, 0),
-			OWL_FACTOR_HW(CMU_SENSORCLK, 0, 3, 0, bisp_factor_table),
-			CLK_IGNORE_UNUSED);
+			{ 0 },
+			OWL_DIVIDER_HW(CMU_SENSORCLK, 0, 4, 0, std12rate_div_table),
+			0);
 
-static OWL_COMP_FACTOR(sensor1_clk, "sensor1_clk", sensor_clk_mux_p,
+static OWL_COMP_DIV(sensor1_clk, "sensor1_clk", sensor_clk_mux_p,
 			OWL_MUX_HW(CMU_SENSORCLK, 4, 1),
-			OWL_GATE_HW(CMU_DEVCLKEN0, 14, 0),
-			OWL_FACTOR_HW(CMU_SENSORCLK, 8, 3, 0, bisp_factor_table),
-			CLK_IGNORE_UNUSED);
+			{ 0 },
+			OWL_DIVIDER_HW(CMU_SENSORCLK, 8, 4, 0, std12rate_div_table),
+			0);
 
 static OWL_COMP_FACTOR(sd0_clk, "sd0_clk", sd_clk_mux_p,
 			OWL_MUX_HW(CMU_SD0CLK, 9, 1),
-- 
2.30.1

Re: [PATCH 3/6] clk: actions: Fix bisp_factor_table based clocks on Owl S500 SoC

[Manivannan Sadhasivam]

On Mon, Mar 08, 2021 at 07:18:28PM +0200, Cristian Ciocaltea wrote:
> The following clocks of the Actions Semi Owl S500 SoC have been defined
> to use a shared clock factor table 'bisp_factor_table[]': DE[1-2], VCE,
> VDE, BISP, SENSOR[0-1]
> 
> There are several issues involved in this approach:
> 
> * 'bisp_factor_table[]' describes the configuration of a regular 8-rates
>   divider, so its usage is redundant. Additionally, judging by the BISP
>   clock context, it is incomplete since it maps only 8 out of 12
>   possible entries.
> 
> * The clocks mentioned above are not identical in terms of the available
>   rates, therefore cannot rely on the same factor table. Specifically,
>   BISP and SENSOR* are standard 12-rate dividers so their configuration
>   should rely on a proper clock div table, while VCE and VDE require a
>   factor table that is a actually a subset of the one needed for DE[1-2]
>   clocks.
> 
> Let's fix this by implementing the following:
> 
> * Add new factor tables 'de_factor_table' and 'hde_factor_table' to
>   properly handle DE[1-2], VCE and VDE clocks.
> 
> * Add a common div table 'std12rate_div_table' for BISP and SENSOR[0-1]
>   clocks converted to OWL_COMP_DIV.
> 
> * Drop the now unused 'bisp_factor_table[]'.
> 

Nice!

> Additionally, since SENSOR[0-1] are not gated, unset the OWL_GATE_HW
> configuration and drop the CLK_IGNORE_UNUSED flag in their definitions.
> 

No. You should not screen the functionality exposed by the hw, that's what the
purpose of these CLK_ flags.

Other than that, this patch looks good to me.

> Fixes: ed6b4795ece4 ("clk: actions: Add clock driver for S500 SoC")
> Signed-off-by: Cristian Ciocaltea <cristian.ciocaltea@gmail.com>
> ---
>  drivers/clk/actions/owl-s500.c | 48 ++++++++++++++++++++++------------
>  1 file changed, 31 insertions(+), 17 deletions(-)
> 
> diff --git a/drivers/clk/actions/owl-s500.c b/drivers/clk/actions/owl-s500.c
> index 69cd959205f5..abe8874353de 100644
> --- a/drivers/clk/actions/owl-s500.c
> +++ b/drivers/clk/actions/owl-s500.c
> @@ -138,9 +138,16 @@ static struct clk_factor_table sd_factor_table[] = {
>  	{ 0, 0, 0 },
>  };
>  
> -static struct clk_factor_table bisp_factor_table[] = {
> -	{ 0, 1, 1 }, { 1, 1, 2 }, { 2, 1, 3 }, { 3, 1, 4 },
> -	{ 4, 1, 5 }, { 5, 1, 6 }, { 6, 1, 7 }, { 7, 1, 8 },
> +static struct clk_factor_table de_factor_table[] = {
> +	{ 0, 1, 1 }, { 1, 2, 3 }, { 2, 1, 2 }, { 3, 2, 5 },
> +	{ 4, 1, 3 }, { 5, 1, 4 }, { 6, 1, 6 }, { 7, 1, 8 },
> +	{ 8, 1, 12 },
> +	{ 0, 0, 0 },
> +};
> +
> +static struct clk_factor_table hde_factor_table[] = {
> +	{ 0, 1, 1 }, { 1, 2, 3 }, { 2, 1, 2 }, { 3, 2, 5 },
> +	{ 4, 1, 3 }, { 5, 1, 4 }, { 6, 1, 6 }, { 7, 1, 8 },
>  	{ 0, 0, 0 },
>  };
>  
> @@ -154,6 +161,13 @@ static struct clk_div_table rmii_ref_div_table[] = {
>  	{ 0, 0 },
>  };
>  
> +static struct clk_div_table std12rate_div_table[] = {
> +	{ 0, 1 }, { 1, 2 }, { 2, 3 }, { 3, 4 },
> +	{ 4, 5 }, { 5, 6 }, { 6, 7 }, { 7, 8 },
> +	{ 8, 9 }, { 9, 10 }, { 10, 11 }, { 11, 12 },
> +	{ 0, 0 },
> +};
> +
>  static struct clk_div_table i2s_div_table[] = {
>  	{ 0, 1 }, { 1, 2 }, { 2, 3 }, { 3, 4 },
>  	{ 4, 6 }, { 5, 8 }, { 6, 12 }, { 7, 16 },
> @@ -189,39 +203,39 @@ static OWL_DIVIDER(rmii_ref_clk, "rmii_ref_clk", "ethernet_pll_clk", CMU_ETHERNE
>  
>  /* factor clocks */
>  static OWL_FACTOR(ahb_clk, "ahb_clk", "h_clk", CMU_BUSCLK1, 2, 2, ahb_factor_table, 0, 0);
> -static OWL_FACTOR(de1_clk, "de_clk1", "de_clk", CMU_DECLK, 0, 3, bisp_factor_table, 0, 0);
> -static OWL_FACTOR(de2_clk, "de_clk2", "de_clk", CMU_DECLK, 4, 3, bisp_factor_table, 0, 0);
> +static OWL_FACTOR(de1_clk, "de_clk1", "de_clk", CMU_DECLK, 0, 4, de_factor_table, 0, 0);
> +static OWL_FACTOR(de2_clk, "de_clk2", "de_clk", CMU_DECLK, 4, 4, de_factor_table, 0, 0);
>  
>  /* composite clocks */
>  static OWL_COMP_FACTOR(vce_clk, "vce_clk", hde_clk_mux_p,
>  			OWL_MUX_HW(CMU_VCECLK, 4, 2),
>  			OWL_GATE_HW(CMU_DEVCLKEN0, 26, 0),
> -			OWL_FACTOR_HW(CMU_VCECLK, 0, 3, 0, bisp_factor_table),
> +			OWL_FACTOR_HW(CMU_VCECLK, 0, 3, 0, hde_factor_table),
>  			0);
>  
>  static OWL_COMP_FACTOR(vde_clk, "vde_clk", hde_clk_mux_p,
>  			OWL_MUX_HW(CMU_VDECLK, 4, 2),
>  			OWL_GATE_HW(CMU_DEVCLKEN0, 25, 0),
> -			OWL_FACTOR_HW(CMU_VDECLK, 0, 3, 0, bisp_factor_table),
> +			OWL_FACTOR_HW(CMU_VDECLK, 0, 3, 0, hde_factor_table),
>  			0);
>  
> -static OWL_COMP_FACTOR(bisp_clk, "bisp_clk", bisp_clk_mux_p,
> +static OWL_COMP_DIV(bisp_clk, "bisp_clk", bisp_clk_mux_p,
>  			OWL_MUX_HW(CMU_BISPCLK, 4, 1),
>  			OWL_GATE_HW(CMU_DEVCLKEN0, 14, 0),
> -			OWL_FACTOR_HW(CMU_BISPCLK, 0, 3, 0, bisp_factor_table),
> +			OWL_DIVIDER_HW(CMU_BISPCLK, 0, 4, 0, std12rate_div_table),
>  			0);
>  
> -static OWL_COMP_FACTOR(sensor0_clk, "sensor0_clk", sensor_clk_mux_p,
> +static OWL_COMP_DIV(sensor0_clk, "sensor0_clk", sensor_clk_mux_p,
>  			OWL_MUX_HW(CMU_SENSORCLK, 4, 1),
> -			OWL_GATE_HW(CMU_DEVCLKEN0, 14, 0),
> -			OWL_FACTOR_HW(CMU_SENSORCLK, 0, 3, 0, bisp_factor_table),
> -			CLK_IGNORE_UNUSED);
> +			{ 0 },
> +			OWL_DIVIDER_HW(CMU_SENSORCLK, 0, 4, 0, std12rate_div_table),
> +			0);
>  
> -static OWL_COMP_FACTOR(sensor1_clk, "sensor1_clk", sensor_clk_mux_p,
> +static OWL_COMP_DIV(sensor1_clk, "sensor1_clk", sensor_clk_mux_p,
>  			OWL_MUX_HW(CMU_SENSORCLK, 4, 1),
> -			OWL_GATE_HW(CMU_DEVCLKEN0, 14, 0),
> -			OWL_FACTOR_HW(CMU_SENSORCLK, 8, 3, 0, bisp_factor_table),
> -			CLK_IGNORE_UNUSED);
> +			{ 0 },
> +			OWL_DIVIDER_HW(CMU_SENSORCLK, 8, 4, 0, std12rate_div_table),
> +			0);
>  
>  static OWL_COMP_FACTOR(sd0_clk, "sd0_clk", sd_clk_mux_p,
>  			OWL_MUX_HW(CMU_SD0CLK, 9, 1),
> -- 
> 2.30.1
> 

Re: [PATCH 3/6] clk: actions: Fix bisp_factor_table based clocks on Owl S500 SoC

[Cristian Ciocaltea]

On Tue, Mar 16, 2021 at 09:47:39AM +0530, Manivannan Sadhasivam wrote:
> On Mon, Mar 08, 2021 at 07:18:28PM +0200, Cristian Ciocaltea wrote:
> > The following clocks of the Actions Semi Owl S500 SoC have been defined
> > to use a shared clock factor table 'bisp_factor_table[]': DE[1-2], VCE,
> > VDE, BISP, SENSOR[0-1]
> > 
> > There are several issues involved in this approach:
> > 
> > * 'bisp_factor_table[]' describes the configuration of a regular 8-rates
> >   divider, so its usage is redundant. Additionally, judging by the BISP
> >   clock context, it is incomplete since it maps only 8 out of 12
> >   possible entries.
> > 
> > * The clocks mentioned above are not identical in terms of the available
> >   rates, therefore cannot rely on the same factor table. Specifically,
> >   BISP and SENSOR* are standard 12-rate dividers so their configuration
> >   should rely on a proper clock div table, while VCE and VDE require a
> >   factor table that is a actually a subset of the one needed for DE[1-2]
> >   clocks.
> > 
> > Let's fix this by implementing the following:
> > 
> > * Add new factor tables 'de_factor_table' and 'hde_factor_table' to
> >   properly handle DE[1-2], VCE and VDE clocks.
> > 
> > * Add a common div table 'std12rate_div_table' for BISP and SENSOR[0-1]
> >   clocks converted to OWL_COMP_DIV.
> > 
> > * Drop the now unused 'bisp_factor_table[]'.
> > 
> 
> Nice!
> 
> > Additionally, since SENSOR[0-1] are not gated, unset the OWL_GATE_HW
> > configuration and drop the CLK_IGNORE_UNUSED flag in their definitions.
> > 
> 
> No. You should not screen the functionality exposed by the hw, that's what the
> purpose of these CLK_ flags.

I'm not sure I get this, or maybe I wasn't clear enough with my
explanation regarding the changes to SENSOR clocks: they are not gated
in hardware, hence the statement 'OWL_GATE_HW(CMU_DEVCLKEN0, 14, 0)' 
was invalid and I replaced it with '{ 0 }'.

Additionally, I assumed the 'CLK_IGNORE_UNUSED' flag makes sense only
for the gated clocks. Do I miss something?

> Other than that, this patch looks good to me.

Thanks,
Cristi

[...]

Re: [PATCH 3/6] clk: actions: Fix bisp_factor_table based clocks on Owl S500 SoC

[Manivannan Sadhasivam]

On Tue, Mar 16, 2021 at 08:37:53PM +0200, Cristian Ciocaltea wrote:
> On Tue, Mar 16, 2021 at 09:47:39AM +0530, Manivannan Sadhasivam wrote:
> > On Mon, Mar 08, 2021 at 07:18:28PM +0200, Cristian Ciocaltea wrote:
> > > The following clocks of the Actions Semi Owl S500 SoC have been defined
> > > to use a shared clock factor table 'bisp_factor_table[]': DE[1-2], VCE,
> > > VDE, BISP, SENSOR[0-1]
> > > 
> > > There are several issues involved in this approach:
> > > 
> > > * 'bisp_factor_table[]' describes the configuration of a regular 8-rates
> > >   divider, so its usage is redundant. Additionally, judging by the BISP
> > >   clock context, it is incomplete since it maps only 8 out of 12
> > >   possible entries.
> > > 
> > > * The clocks mentioned above are not identical in terms of the available
> > >   rates, therefore cannot rely on the same factor table. Specifically,
> > >   BISP and SENSOR* are standard 12-rate dividers so their configuration
> > >   should rely on a proper clock div table, while VCE and VDE require a
> > >   factor table that is a actually a subset of the one needed for DE[1-2]
> > >   clocks.
> > > 
> > > Let's fix this by implementing the following:
> > > 
> > > * Add new factor tables 'de_factor_table' and 'hde_factor_table' to
> > >   properly handle DE[1-2], VCE and VDE clocks.
> > > 
> > > * Add a common div table 'std12rate_div_table' for BISP and SENSOR[0-1]
> > >   clocks converted to OWL_COMP_DIV.
> > > 
> > > * Drop the now unused 'bisp_factor_table[]'.
> > > 
> > 
> > Nice!
> > 
> > > Additionally, since SENSOR[0-1] are not gated, unset the OWL_GATE_HW
> > > configuration and drop the CLK_IGNORE_UNUSED flag in their definitions.
> > > 
> > 
> > No. You should not screen the functionality exposed by the hw, that's what the
> > purpose of these CLK_ flags.
> 
> I'm not sure I get this, or maybe I wasn't clear enough with my
> explanation regarding the changes to SENSOR clocks: they are not gated
> in hardware, hence the statement 'OWL_GATE_HW(CMU_DEVCLKEN0, 14, 0)' 
> was invalid and I replaced it with '{ 0 }'.
> 

This clock is gated in hw as per the datasheet. Again, please don't make
judgements based on the vendor code as it is not upto date with HW. I
know it is silly but that's how things are...

> Additionally, I assumed the 'CLK_IGNORE_UNUSED' flag makes sense only
> for the gated clocks. Do I miss something?
> 

CLK_IGNORE_UNUSED is used by the clk framework to essentially skip
gating the clocks which are turned ON by the bootloader and there is no
other driver using it. But I think you can remove this flag because
there is no reason to leave this specific clock to be ON always.

Thanks,
Mani

> > Other than that, this patch looks good to me.
> 
> Thanks,
> Cristi
> 
> [...]

Re: [PATCH 3/6] clk: actions: Fix bisp_factor_table based clocks on Owl S500 SoC

[Cristian Ciocaltea]

On Wed, May 26, 2021 at 03:48:30PM +0530, Manivannan Sadhasivam wrote:
> On Tue, Mar 16, 2021 at 08:37:53PM +0200, Cristian Ciocaltea wrote:
> > On Tue, Mar 16, 2021 at 09:47:39AM +0530, Manivannan Sadhasivam wrote:
> > > On Mon, Mar 08, 2021 at 07:18:28PM +0200, Cristian Ciocaltea wrote:
> > > > The following clocks of the Actions Semi Owl S500 SoC have been defined
> > > > to use a shared clock factor table 'bisp_factor_table[]': DE[1-2], VCE,
> > > > VDE, BISP, SENSOR[0-1]
> > > > 
> > > > There are several issues involved in this approach:
> > > > 
> > > > * 'bisp_factor_table[]' describes the configuration of a regular 8-rates
> > > >   divider, so its usage is redundant. Additionally, judging by the BISP
> > > >   clock context, it is incomplete since it maps only 8 out of 12
> > > >   possible entries.
> > > > 
> > > > * The clocks mentioned above are not identical in terms of the available
> > > >   rates, therefore cannot rely on the same factor table. Specifically,
> > > >   BISP and SENSOR* are standard 12-rate dividers so their configuration
> > > >   should rely on a proper clock div table, while VCE and VDE require a
> > > >   factor table that is a actually a subset of the one needed for DE[1-2]
> > > >   clocks.
> > > > 
> > > > Let's fix this by implementing the following:
> > > > 
> > > > * Add new factor tables 'de_factor_table' and 'hde_factor_table' to
> > > >   properly handle DE[1-2], VCE and VDE clocks.
> > > > 
> > > > * Add a common div table 'std12rate_div_table' for BISP and SENSOR[0-1]
> > > >   clocks converted to OWL_COMP_DIV.
> > > > 
> > > > * Drop the now unused 'bisp_factor_table[]'.
> > > > 
> > > 
> > > Nice!
> > > 
> > > > Additionally, since SENSOR[0-1] are not gated, unset the OWL_GATE_HW
> > > > configuration and drop the CLK_IGNORE_UNUSED flag in their definitions.
> > > > 
> > > 
> > > No. You should not screen the functionality exposed by the hw, that's what the
> > > purpose of these CLK_ flags.
> > 
> > I'm not sure I get this, or maybe I wasn't clear enough with my
> > explanation regarding the changes to SENSOR clocks: they are not gated
> > in hardware, hence the statement 'OWL_GATE_HW(CMU_DEVCLKEN0, 14, 0)' 
> > was invalid and I replaced it with '{ 0 }'.
> > 
> 
> This clock is gated in hw as per the datasheet. Again, please don't make
> judgements based on the vendor code as it is not upto date with HW. I
> know it is silly but that's how things are...

Indeed, a newer datasheet states the clock is gated. I fixed the patch
accordingly in v2.

> > Additionally, I assumed the 'CLK_IGNORE_UNUSED' flag makes sense only
> > for the gated clocks. Do I miss something?
> > 
> 
> CLK_IGNORE_UNUSED is used by the clk framework to essentially skip
> gating the clocks which are turned ON by the bootloader and there is no
> other driver using it. But I think you can remove this flag because
> there is no reason to leave this specific clock to be ON always.

Thanks for the explanation, I kept the flag removed in v2.

Regards,
Cristi

> Thanks,
> Mani
> 
> > > Other than that, this patch looks good to me.
> > 
> > Thanks,
> > Cristi
> > 
> > [...]

[PATCH 4/6] clk: actions: Fix AHPPREDIV-H-AHB clock chain on Owl S500 SoC

[Cristian Ciocaltea]

There are a few issues with the setup of the Actions Semi Owl S500 SoC's
clock chain involving AHPPREDIV, H and AHB clocks:

* AHBPREDIV clock is defined as a muxer only, although it also acts as
  a divider.
* H clock is defined as a standard divider, although the raw value zero
  is not supported.
* AHB is defined as a multi-rate factor clock, but it is actually just
  a fixed pass clock.

Let's provide the following fixes:

* Change AHBPREDIV clock to an ungated OWL_COMP_DIV definition.
* Add a clock div table 'h_div_table' for the H clock to drop the
  unsupported 0 rate and use the correct register shift value in the
  OWL_DIVIDER definition.
* Drop the unneeded 'ahb_factor_table[]' and change AHB clock to an
  ungated OWL_COMP_FIXED_FACTOR definition.

Fixes: ed6b4795ece4 ("clk: actions: Add clock driver for S500 SoC")
Signed-off-by: Cristian Ciocaltea <cristian.ciocaltea@gmail.com>
---
 drivers/clk/actions/owl-s500.c | 20 ++++++++++++++------
 1 file changed, 14 insertions(+), 6 deletions(-)

diff --git a/drivers/clk/actions/owl-s500.c b/drivers/clk/actions/owl-s500.c
index abe8874353de..b9e434173b4f 100644
--- a/drivers/clk/actions/owl-s500.c
+++ b/drivers/clk/actions/owl-s500.c
@@ -151,9 +151,9 @@ static struct clk_factor_table hde_factor_table[] = {
 	{ 0, 0, 0 },
 };
 
-static struct clk_factor_table ahb_factor_table[] = {
-	{ 1, 1, 2 }, { 2, 1, 3 },
-	{ 0, 0, 0 },
+static struct clk_div_table h_div_table[] = {
+	{ 1, 2 }, { 2, 3 }, { 3, 4 },
+	{ 0, 0 },
 };
 
 static struct clk_div_table rmii_ref_div_table[] = {
@@ -184,7 +184,6 @@ static struct clk_div_table nand_div_table[] = {
 
 /* mux clock */
 static OWL_MUX(dev_clk, "dev_clk", dev_clk_mux_p, CMU_DEVPLL, 12, 1, CLK_SET_RATE_PARENT);
-static OWL_MUX(ahbprediv_clk, "ahbprediv_clk", ahbprediv_clk_mux_p, CMU_BUSCLK1, 8, 3, CLK_SET_RATE_PARENT);
 
 /* gate clocks */
 static OWL_GATE(gpio_clk, "gpio_clk", "apb_clk", CMU_DEVCLKEN0, 18, 0, 0);
@@ -197,16 +196,25 @@ static OWL_GATE(timer_clk, "timer_clk", "hosc", CMU_DEVCLKEN1, 27, 0, 0);
 static OWL_GATE(hdmi_clk, "hdmi_clk", "hosc", CMU_DEVCLKEN1, 3, 0, 0);
 
 /* divider clocks */
-static OWL_DIVIDER(h_clk, "h_clk", "ahbprediv_clk", CMU_BUSCLK1, 12, 2, NULL, 0, 0);
+static OWL_DIVIDER(h_clk, "h_clk", "ahbprediv_clk", CMU_BUSCLK1, 2, 2, h_div_table, 0, 0);
 static OWL_DIVIDER(apb_clk, "apb_clk", "ahb_clk", CMU_BUSCLK1, 14, 2, NULL, 0, 0);
 static OWL_DIVIDER(rmii_ref_clk, "rmii_ref_clk", "ethernet_pll_clk", CMU_ETHERNETPLL, 1, 1, rmii_ref_div_table, 0, 0);
 
 /* factor clocks */
-static OWL_FACTOR(ahb_clk, "ahb_clk", "h_clk", CMU_BUSCLK1, 2, 2, ahb_factor_table, 0, 0);
 static OWL_FACTOR(de1_clk, "de_clk1", "de_clk", CMU_DECLK, 0, 4, de_factor_table, 0, 0);
 static OWL_FACTOR(de2_clk, "de_clk2", "de_clk", CMU_DECLK, 4, 4, de_factor_table, 0, 0);
 
 /* composite clocks */
+static OWL_COMP_DIV(ahbprediv_clk, "ahbprediv_clk", ahbprediv_clk_mux_p,
+			OWL_MUX_HW(CMU_BUSCLK1, 8, 3),
+			{ 0 },
+			OWL_DIVIDER_HW(CMU_BUSCLK1, 12, 2, 0, NULL),
+			0);
+
+static OWL_COMP_FIXED_FACTOR(ahb_clk, "ahb_clk", "h_clk",
+			{ 0 },
+			1, 1, CLK_SET_RATE_PARENT);
+
 static OWL_COMP_FACTOR(vce_clk, "vce_clk", hde_clk_mux_p,
 			OWL_MUX_HW(CMU_VCECLK, 4, 2),
 			OWL_GATE_HW(CMU_DEVCLKEN0, 26, 0),
-- 
2.30.1

Re: [PATCH 4/6] clk: actions: Fix AHPPREDIV-H-AHB clock chain on Owl S500 SoC

[Manivannan Sadhasivam]

On Mon, Mar 08, 2021 at 07:18:29PM +0200, Cristian Ciocaltea wrote:
> There are a few issues with the setup of the Actions Semi Owl S500 SoC's
> clock chain involving AHPPREDIV, H and AHB clocks:
> 
> * AHBPREDIV clock is defined as a muxer only, although it also acts as
>   a divider.
> * H clock is defined as a standard divider, although the raw value zero
>   is not supported.

What do you mean by not supported? The datasheet lists "0" as the valid divisor
value for divide by 1.

Rest looks good to me.

Thanks,
Mani

> * AHB is defined as a multi-rate factor clock, but it is actually just
>   a fixed pass clock.
> 
> Let's provide the following fixes:
> 
> * Change AHBPREDIV clock to an ungated OWL_COMP_DIV definition.
> * Add a clock div table 'h_div_table' for the H clock to drop the
>   unsupported 0 rate and use the correct register shift value in the
>   OWL_DIVIDER definition.
> * Drop the unneeded 'ahb_factor_table[]' and change AHB clock to an
>   ungated OWL_COMP_FIXED_FACTOR definition.
> 
> Fixes: ed6b4795ece4 ("clk: actions: Add clock driver for S500 SoC")
> Signed-off-by: Cristian Ciocaltea <cristian.ciocaltea@gmail.com>
> ---
>  drivers/clk/actions/owl-s500.c | 20 ++++++++++++++------
>  1 file changed, 14 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/clk/actions/owl-s500.c b/drivers/clk/actions/owl-s500.c
> index abe8874353de..b9e434173b4f 100644
> --- a/drivers/clk/actions/owl-s500.c
> +++ b/drivers/clk/actions/owl-s500.c
> @@ -151,9 +151,9 @@ static struct clk_factor_table hde_factor_table[] = {
>  	{ 0, 0, 0 },
>  };
>  
> -static struct clk_factor_table ahb_factor_table[] = {
> -	{ 1, 1, 2 }, { 2, 1, 3 },
> -	{ 0, 0, 0 },
> +static struct clk_div_table h_div_table[] = {
> +	{ 1, 2 }, { 2, 3 }, { 3, 4 },
> +	{ 0, 0 },
>  };
>  
>  static struct clk_div_table rmii_ref_div_table[] = {
> @@ -184,7 +184,6 @@ static struct clk_div_table nand_div_table[] = {
>  
>  /* mux clock */
>  static OWL_MUX(dev_clk, "dev_clk", dev_clk_mux_p, CMU_DEVPLL, 12, 1, CLK_SET_RATE_PARENT);
> -static OWL_MUX(ahbprediv_clk, "ahbprediv_clk", ahbprediv_clk_mux_p, CMU_BUSCLK1, 8, 3, CLK_SET_RATE_PARENT);
>  
>  /* gate clocks */
>  static OWL_GATE(gpio_clk, "gpio_clk", "apb_clk", CMU_DEVCLKEN0, 18, 0, 0);
> @@ -197,16 +196,25 @@ static OWL_GATE(timer_clk, "timer_clk", "hosc", CMU_DEVCLKEN1, 27, 0, 0);
>  static OWL_GATE(hdmi_clk, "hdmi_clk", "hosc", CMU_DEVCLKEN1, 3, 0, 0);
>  
>  /* divider clocks */
> -static OWL_DIVIDER(h_clk, "h_clk", "ahbprediv_clk", CMU_BUSCLK1, 12, 2, NULL, 0, 0);
> +static OWL_DIVIDER(h_clk, "h_clk", "ahbprediv_clk", CMU_BUSCLK1, 2, 2, h_div_table, 0, 0);
>  static OWL_DIVIDER(apb_clk, "apb_clk", "ahb_clk", CMU_BUSCLK1, 14, 2, NULL, 0, 0);
>  static OWL_DIVIDER(rmii_ref_clk, "rmii_ref_clk", "ethernet_pll_clk", CMU_ETHERNETPLL, 1, 1, rmii_ref_div_table, 0, 0);
>  
>  /* factor clocks */
> -static OWL_FACTOR(ahb_clk, "ahb_clk", "h_clk", CMU_BUSCLK1, 2, 2, ahb_factor_table, 0, 0);
>  static OWL_FACTOR(de1_clk, "de_clk1", "de_clk", CMU_DECLK, 0, 4, de_factor_table, 0, 0);
>  static OWL_FACTOR(de2_clk, "de_clk2", "de_clk", CMU_DECLK, 4, 4, de_factor_table, 0, 0);
>  
>  /* composite clocks */
> +static OWL_COMP_DIV(ahbprediv_clk, "ahbprediv_clk", ahbprediv_clk_mux_p,
> +			OWL_MUX_HW(CMU_BUSCLK1, 8, 3),
> +			{ 0 },
> +			OWL_DIVIDER_HW(CMU_BUSCLK1, 12, 2, 0, NULL),
> +			0);
> +
> +static OWL_COMP_FIXED_FACTOR(ahb_clk, "ahb_clk", "h_clk",
> +			{ 0 },
> +			1, 1, CLK_SET_RATE_PARENT);
> +
>  static OWL_COMP_FACTOR(vce_clk, "vce_clk", hde_clk_mux_p,
>  			OWL_MUX_HW(CMU_VCECLK, 4, 2),
>  			OWL_GATE_HW(CMU_DEVCLKEN0, 26, 0),
> -- 
> 2.30.1
> 

Re: [PATCH 4/6] clk: actions: Fix AHPPREDIV-H-AHB clock chain on Owl S500 SoC

[Cristian Ciocaltea]

On Tue, Mar 16, 2021 at 11:15:47AM +0530, Manivannan Sadhasivam wrote:
> On Mon, Mar 08, 2021 at 07:18:29PM +0200, Cristian Ciocaltea wrote:
> > There are a few issues with the setup of the Actions Semi Owl S500 SoC's
> > clock chain involving AHPPREDIV, H and AHB clocks:
> > 
> > * AHBPREDIV clock is defined as a muxer only, although it also acts as
> >   a divider.
> > * H clock is defined as a standard divider, although the raw value zero
> >   is not supported.
> 
> What do you mean by not supported? The datasheet lists "0" as the valid divisor
> value for divide by 1.

Unfortunately CMU_BUSCLK1 is not documented in my S500 Datasheet
(Version: 1.6, 2016-03-07). Do you have a newer (or a more official)
one?

The reference xapp-le code snipped is:

static struct owl_div divider_H_CLK = {
    .type = DIV_T_NATURE,
    .range_from = 1,  /* reserve H_CLK divsor 1 */
    .range_to = 3,
    .reg = &divbit_H_CLK,
};

Not sure why divisor 1 has been reserved..

Thanks,
Cristi

> Rest looks good to me.
> 
> Thanks,
> Mani
> 
[...]

Re: [PATCH 4/6] clk: actions: Fix AHPPREDIV-H-AHB clock chain on Owl S500 SoC

[Manivannan Sadhasivam]

On Tue, Mar 16, 2021 at 08:50:14PM +0200, Cristian Ciocaltea wrote:
> On Tue, Mar 16, 2021 at 11:15:47AM +0530, Manivannan Sadhasivam wrote:
> > On Mon, Mar 08, 2021 at 07:18:29PM +0200, Cristian Ciocaltea wrote:
> > > There are a few issues with the setup of the Actions Semi Owl S500 SoC's
> > > clock chain involving AHPPREDIV, H and AHB clocks:
> > > 
> > > * AHBPREDIV clock is defined as a muxer only, although it also acts as
> > >   a divider.
> > > * H clock is defined as a standard divider, although the raw value zero
> > >   is not supported.
> > 
> > What do you mean by not supported? The datasheet lists "0" as the valid divisor
> > value for divide by 1.
> 
> Unfortunately CMU_BUSCLK1 is not documented in my S500 Datasheet
> (Version: 1.6, 2016-03-07). Do you have a newer (or a more official)
> one?
> 

Yes I do have a newer version of the datasheet (v1.8) and there I can
see the divisor 0.

> The reference xapp-le code snipped is:
> 
> static struct owl_div divider_H_CLK = {
>     .type = DIV_T_NATURE,
>     .range_from = 1,  /* reserve H_CLK divsor 1 */
>     .range_to = 3,
>     .reg = &divbit_H_CLK,
> };
> 
> Not sure why divisor 1 has been reserved..
> 

It is not as per the datasheet. Did you run into any issues with this?
Else I'd suggest to keep it as it is.

Thanks,
Mani

> Thanks,
> Cristi
> 
> > Rest looks good to me.
> > 
> > Thanks,
> > Mani
> > 
> [...]

Re: [PATCH 4/6] clk: actions: Fix AHPPREDIV-H-AHB clock chain on Owl S500 SoC

[Cristian Ciocaltea]

On Wed, May 26, 2021 at 03:42:30PM +0530, Manivannan Sadhasivam wrote:
> On Tue, Mar 16, 2021 at 08:50:14PM +0200, Cristian Ciocaltea wrote:
> > On Tue, Mar 16, 2021 at 11:15:47AM +0530, Manivannan Sadhasivam wrote:
> > > On Mon, Mar 08, 2021 at 07:18:29PM +0200, Cristian Ciocaltea wrote:
> > > > There are a few issues with the setup of the Actions Semi Owl S500 SoC's
> > > > clock chain involving AHPPREDIV, H and AHB clocks:
> > > > 
> > > > * AHBPREDIV clock is defined as a muxer only, although it also acts as
> > > >   a divider.
> > > > * H clock is defined as a standard divider, although the raw value zero
> > > >   is not supported.
> > > 
> > > What do you mean by not supported? The datasheet lists "0" as the valid divisor
> > > value for divide by 1.
> > 
> > Unfortunately CMU_BUSCLK1 is not documented in my S500 Datasheet
> > (Version: 1.6, 2016-03-07). Do you have a newer (or a more official)
> > one?
> > 
> 
> Yes I do have a newer version of the datasheet (v1.8) and there I can
> see the divisor 0.

I got an updated datasheet (v1.9) and I confirm dividing by 1 is valid.

> > The reference xapp-le code snipped is:
> > 
> > static struct owl_div divider_H_CLK = {
> >     .type = DIV_T_NATURE,
> >     .range_from = 1,  /* reserve H_CLK divsor 1 */
> >     .range_to = 3,
> >     .reg = &divbit_H_CLK,
> > };
> > 
> > Not sure why divisor 1 has been reserved..
> > 
> 
> It is not as per the datasheet. Did you run into any issues with this?
> Else I'd suggest to keep it as it is.

I reverted the changes (please see v2) and did not encounter any issues
so far, so let's ignore the vendor driver implementation.

Thanks for the review,
Cristi

> Thanks,
> Mani
> 
> > Thanks,
> > Cristi
> > 
> > > Rest looks good to me.
> > > 
> > > Thanks,
> > > Mani
> > > 
> > [...]

[PATCH 5/6] dt-bindings: clock: Add NIC and ETHERNET bindings for Actions S500 SoC

[Cristian Ciocaltea]

Add the missing NIC and ETHERNET clock bindings constants for Actions
Semi Owl S500 SoC.

Signed-off-by: Cristian Ciocaltea <cristian.ciocaltea@gmail.com>
---
 include/dt-bindings/clock/actions,s500-cmu.h | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/include/dt-bindings/clock/actions,s500-cmu.h b/include/dt-bindings/clock/actions,s500-cmu.h
index a250a52a6192..a237eb26accb 100644
--- a/include/dt-bindings/clock/actions,s500-cmu.h
+++ b/include/dt-bindings/clock/actions,s500-cmu.h
@@ -74,10 +74,12 @@
 #define CLK_RMII_REF		54
 #define CLK_GPIO		55
 
-/* system clock (part 2) */
+/* additional clocks */
 #define CLK_APB			56
 #define CLK_DMAC		57
+#define CLK_NIC			58
+#define CLK_ETHERNET		59
 
-#define CLK_NR_CLKS		(CLK_DMAC + 1)
+#define CLK_NR_CLKS		(CLK_ETHERNET + 1)
 
 #endif /* __DT_BINDINGS_CLOCK_S500_CMU_H */
-- 
2.30.1

Re: [PATCH 5/6] dt-bindings: clock: Add NIC and ETHERNET bindings for Actions S500 SoC

[Rob Herring]

On Mon, 08 Mar 2021 19:18:30 +0200, Cristian Ciocaltea wrote:
> Add the missing NIC and ETHERNET clock bindings constants for Actions
> Semi Owl S500 SoC.
> 
> Signed-off-by: Cristian Ciocaltea <cristian.ciocaltea@gmail.com>
> ---
>  include/dt-bindings/clock/actions,s500-cmu.h | 6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
> 

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

[PATCH 6/6] clk: actions: Add NIC and ETHERNET clock support for Actions S500 SoC

[Cristian Ciocaltea]

Add support for the missing NIC and ETHERNET clocks in the Actions Semi
Owl S500 SoC clock driver.

Additionally, change APB clock parent from AHB to the newly added NIC.

Signed-off-by: Cristian Ciocaltea <cristian.ciocaltea@gmail.com>
---
 drivers/clk/actions/owl-s500.c | 17 ++++++++++++++++-
 1 file changed, 16 insertions(+), 1 deletion(-)

diff --git a/drivers/clk/actions/owl-s500.c b/drivers/clk/actions/owl-s500.c
index b9e434173b4f..0ccc9619b302 100644
--- a/drivers/clk/actions/owl-s500.c
+++ b/drivers/clk/actions/owl-s500.c
@@ -112,6 +112,7 @@ static const char * const bisp_clk_mux_p[] = { "display_pll_clk", "dev_clk" };
 static const char * const sensor_clk_mux_p[] = { "hosc", "bisp_clk" };
 static const char * const sd_clk_mux_p[] = { "dev_clk", "nand_pll_clk" };
 static const char * const pwm_clk_mux_p[] = { "losc", "hosc" };
+static const char * const nic_clk_mux_p[] = { "dev_clk", "display_pll_clk", "nand_pll_clk", "ddr_pll_clk" };
 static const char * const ahbprediv_clk_mux_p[] = { "dev_clk", "display_pll_clk", "nand_pll_clk", "ddr_pll_clk" };
 static const char * const uart_clk_mux_p[] = { "hosc", "dev_pll_clk" };
 static const char * const de_clk_mux_p[] = { "display_pll_clk", "dev_clk" };
@@ -197,7 +198,7 @@ static OWL_GATE(hdmi_clk, "hdmi_clk", "hosc", CMU_DEVCLKEN1, 3, 0, 0);
 
 /* divider clocks */
 static OWL_DIVIDER(h_clk, "h_clk", "ahbprediv_clk", CMU_BUSCLK1, 2, 2, h_div_table, 0, 0);
-static OWL_DIVIDER(apb_clk, "apb_clk", "ahb_clk", CMU_BUSCLK1, 14, 2, NULL, 0, 0);
+static OWL_DIVIDER(apb_clk, "apb_clk", "nic_clk", CMU_BUSCLK1, 14, 2, NULL, 0, 0);
 static OWL_DIVIDER(rmii_ref_clk, "rmii_ref_clk", "ethernet_pll_clk", CMU_ETHERNETPLL, 1, 1, rmii_ref_div_table, 0, 0);
 
 /* factor clocks */
@@ -205,6 +206,12 @@ static OWL_FACTOR(de1_clk, "de_clk1", "de_clk", CMU_DECLK, 0, 4, de_factor_table
 static OWL_FACTOR(de2_clk, "de_clk2", "de_clk", CMU_DECLK, 4, 4, de_factor_table, 0, 0);
 
 /* composite clocks */
+static OWL_COMP_DIV(nic_clk, "nic_clk", nic_clk_mux_p,
+			OWL_MUX_HW(CMU_BUSCLK1, 4, 3),
+			{ 0 },
+			OWL_DIVIDER_HW(CMU_BUSCLK1, 16, 2, 0, NULL),
+			0);
+
 static OWL_COMP_DIV(ahbprediv_clk, "ahbprediv_clk", ahbprediv_clk_mux_p,
 			OWL_MUX_HW(CMU_BUSCLK1, 8, 3),
 			{ 0 },
@@ -320,6 +327,10 @@ static OWL_COMP_FIXED_FACTOR(i2c3_clk, "i2c3_clk", "ethernet_pll_clk",
 			OWL_GATE_HW(CMU_DEVCLKEN1, 31, 0),
 			1, 5, 0);
 
+static OWL_COMP_FIXED_FACTOR(ethernet_clk, "ethernet_clk", "ethernet_pll_clk",
+			OWL_GATE_HW(CMU_DEVCLKEN1, 22, 0),
+			1, 20, 0);
+
 static OWL_COMP_DIV(uart0_clk, "uart0_clk", uart_clk_mux_p,
 			OWL_MUX_HW(CMU_UART0CLK, 16, 1),
 			OWL_GATE_HW(CMU_DEVCLKEN1, 6, 0),
@@ -454,6 +465,8 @@ static struct owl_clk_common *s500_clks[] = {
 	&apb_clk.common,
 	&dmac_clk.common,
 	&gpio_clk.common,
+	&nic_clk.common,
+	&ethernet_clk.common,
 };
 
 static struct clk_hw_onecell_data s500_hw_clks = {
@@ -513,6 +526,8 @@ static struct clk_hw_onecell_data s500_hw_clks = {
 		[CLK_APB]		= &apb_clk.common.hw,
 		[CLK_DMAC]		= &dmac_clk.common.hw,
 		[CLK_GPIO]		= &gpio_clk.common.hw,
+		[CLK_NIC]		= &nic_clk.common.hw,
+		[CLK_ETHERNET]		= &ethernet_clk.common.hw,
 	},
 	.num = CLK_NR_CLKS,
 };
-- 
2.30.1

Re: [PATCH 6/6] clk: actions: Add NIC and ETHERNET clock support for Actions S500 SoC

[Manivannan Sadhasivam]

On Mon, Mar 08, 2021 at 07:18:31PM +0200, Cristian Ciocaltea wrote:
> Add support for the missing NIC and ETHERNET clocks in the Actions Semi
> Owl S500 SoC clock driver.
> 
> Additionally, change APB clock parent from AHB to the newly added NIC.
> 
> Signed-off-by: Cristian Ciocaltea <cristian.ciocaltea@gmail.com>
> ---
>  drivers/clk/actions/owl-s500.c | 17 ++++++++++++++++-
>  1 file changed, 16 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/clk/actions/owl-s500.c b/drivers/clk/actions/owl-s500.c
> index b9e434173b4f..0ccc9619b302 100644
> --- a/drivers/clk/actions/owl-s500.c
> +++ b/drivers/clk/actions/owl-s500.c
> @@ -112,6 +112,7 @@ static const char * const bisp_clk_mux_p[] = { "display_pll_clk", "dev_clk" };
>  static const char * const sensor_clk_mux_p[] = { "hosc", "bisp_clk" };
>  static const char * const sd_clk_mux_p[] = { "dev_clk", "nand_pll_clk" };
>  static const char * const pwm_clk_mux_p[] = { "losc", "hosc" };
> +static const char * const nic_clk_mux_p[] = { "dev_clk", "display_pll_clk", "nand_pll_clk", "ddr_pll_clk" };

As per the reg field order, this should come after "ahbprediv_clk_mux_p"

Rest looks good.

Thanks,
Mani

>  static const char * const ahbprediv_clk_mux_p[] = { "dev_clk", "display_pll_clk", "nand_pll_clk", "ddr_pll_clk" };
>  static const char * const uart_clk_mux_p[] = { "hosc", "dev_pll_clk" };
>  static const char * const de_clk_mux_p[] = { "display_pll_clk", "dev_clk" };
> @@ -197,7 +198,7 @@ static OWL_GATE(hdmi_clk, "hdmi_clk", "hosc", CMU_DEVCLKEN1, 3, 0, 0);
>  
>  /* divider clocks */
>  static OWL_DIVIDER(h_clk, "h_clk", "ahbprediv_clk", CMU_BUSCLK1, 2, 2, h_div_table, 0, 0);
> -static OWL_DIVIDER(apb_clk, "apb_clk", "ahb_clk", CMU_BUSCLK1, 14, 2, NULL, 0, 0);
> +static OWL_DIVIDER(apb_clk, "apb_clk", "nic_clk", CMU_BUSCLK1, 14, 2, NULL, 0, 0);
>  static OWL_DIVIDER(rmii_ref_clk, "rmii_ref_clk", "ethernet_pll_clk", CMU_ETHERNETPLL, 1, 1, rmii_ref_div_table, 0, 0);
>  
>  /* factor clocks */
> @@ -205,6 +206,12 @@ static OWL_FACTOR(de1_clk, "de_clk1", "de_clk", CMU_DECLK, 0, 4, de_factor_table
>  static OWL_FACTOR(de2_clk, "de_clk2", "de_clk", CMU_DECLK, 4, 4, de_factor_table, 0, 0);
>  
>  /* composite clocks */
> +static OWL_COMP_DIV(nic_clk, "nic_clk", nic_clk_mux_p,
> +			OWL_MUX_HW(CMU_BUSCLK1, 4, 3),
> +			{ 0 },
> +			OWL_DIVIDER_HW(CMU_BUSCLK1, 16, 2, 0, NULL),
> +			0);
> +
>  static OWL_COMP_DIV(ahbprediv_clk, "ahbprediv_clk", ahbprediv_clk_mux_p,
>  			OWL_MUX_HW(CMU_BUSCLK1, 8, 3),
>  			{ 0 },
> @@ -320,6 +327,10 @@ static OWL_COMP_FIXED_FACTOR(i2c3_clk, "i2c3_clk", "ethernet_pll_clk",
>  			OWL_GATE_HW(CMU_DEVCLKEN1, 31, 0),
>  			1, 5, 0);
>  
> +static OWL_COMP_FIXED_FACTOR(ethernet_clk, "ethernet_clk", "ethernet_pll_clk",
> +			OWL_GATE_HW(CMU_DEVCLKEN1, 22, 0),
> +			1, 20, 0);
> +
>  static OWL_COMP_DIV(uart0_clk, "uart0_clk", uart_clk_mux_p,
>  			OWL_MUX_HW(CMU_UART0CLK, 16, 1),
>  			OWL_GATE_HW(CMU_DEVCLKEN1, 6, 0),
> @@ -454,6 +465,8 @@ static struct owl_clk_common *s500_clks[] = {
>  	&apb_clk.common,
>  	&dmac_clk.common,
>  	&gpio_clk.common,
> +	&nic_clk.common,
> +	&ethernet_clk.common,
>  };
>  
>  static struct clk_hw_onecell_data s500_hw_clks = {
> @@ -513,6 +526,8 @@ static struct clk_hw_onecell_data s500_hw_clks = {
>  		[CLK_APB]		= &apb_clk.common.hw,
>  		[CLK_DMAC]		= &dmac_clk.common.hw,
>  		[CLK_GPIO]		= &gpio_clk.common.hw,
> +		[CLK_NIC]		= &nic_clk.common.hw,
> +		[CLK_ETHERNET]		= &ethernet_clk.common.hw,
>  	},
>  	.num = CLK_NR_CLKS,
>  };
> -- 
> 2.30.1
> 

Re: [PATCH 6/6] clk: actions: Add NIC and ETHERNET clock support for Actions S500 SoC

[Cristian Ciocaltea]

On Tue, Mar 16, 2021 at 11:22:40AM +0530, Manivannan Sadhasivam wrote:
> On Mon, Mar 08, 2021 at 07:18:31PM +0200, Cristian Ciocaltea wrote:
> > Add support for the missing NIC and ETHERNET clocks in the Actions Semi
> > Owl S500 SoC clock driver.
> > 
> > Additionally, change APB clock parent from AHB to the newly added NIC.
> > 
> > Signed-off-by: Cristian Ciocaltea <cristian.ciocaltea@gmail.com>
> > ---
> >  drivers/clk/actions/owl-s500.c | 17 ++++++++++++++++-
> >  1 file changed, 16 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/clk/actions/owl-s500.c b/drivers/clk/actions/owl-s500.c
> > index b9e434173b4f..0ccc9619b302 100644
> > --- a/drivers/clk/actions/owl-s500.c
> > +++ b/drivers/clk/actions/owl-s500.c
> > @@ -112,6 +112,7 @@ static const char * const bisp_clk_mux_p[] = { "display_pll_clk", "dev_clk" };
> >  static const char * const sensor_clk_mux_p[] = { "hosc", "bisp_clk" };
> >  static const char * const sd_clk_mux_p[] = { "dev_clk", "nand_pll_clk" };
> >  static const char * const pwm_clk_mux_p[] = { "losc", "hosc" };
> > +static const char * const nic_clk_mux_p[] = { "dev_clk", "display_pll_clk", "nand_pll_clk", "ddr_pll_clk" };
> 
> As per the reg field order, this should come after "ahbprediv_clk_mux_p"

Fixed, thanks!

> Rest looks good.
> 
> Thanks,
> Mani
> 
[...]