diff mbox series

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

Message ID 973b08fe414321ba4ade096a4917cadc2013426e.1615221459.git.cristian.ciocaltea@gmail.com (mailing list archive)
State New, archived
Headers show
Series Improve clock support for Actions S500 SoC | expand

Commit Message

Cristian Ciocaltea March 8, 2021, 5:18 p.m. UTC 
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(-)

Comments

'Manivannan Sadhasivam' March 16, 2021, 3:58 a.m. UTC | #1 
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
>
Cristian Ciocaltea March 16, 2021, 6:14 p.m. UTC | #2 
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
> >
'Manivannan Sadhasivam' May 26, 2021, 10:07 a.m. UTC | #3 
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
> > >
Cristian Ciocaltea May 27, 2021, 1:27 p.m. UTC | #4 
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
> > > >
diff mbox series

Patch

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 },
 };