[PATCH] AT91: SAM9G45 - add a separate clock entry for every single TC block

[PATCH] AT91: SAM9G45 - add a separate clock entry for every single TC block

[Fabian Godehardt]

Without this patch you will not be able to register the first block
because of the second association call on at91_add_device_tc().

Signed-off-by: Fabian Godehardt <fg@emlix.com>
---
 arch/arm/mach-at91/at91sam9g45.c         |   12 +++++++++---
 arch/arm/mach-at91/at91sam9g45_devices.c |    4 ++--
 2 files changed, 11 insertions(+), 5 deletions(-)

diff --git a/arch/arm/mach-at91/at91sam9g45.c b/arch/arm/mach-at91/at91sam9g45.c
index 753c0d3..52ef2d6 100644
--- a/arch/arm/mach-at91/at91sam9g45.c
+++ b/arch/arm/mach-at91/at91sam9g45.c
@@ -121,8 +121,13 @@ static struct clk ssc1_clk = {
 	.pmc_mask	= 1 << AT91SAM9G45_ID_SSC1,
 	.type		= CLK_TYPE_PERIPHERAL,
 };
-static struct clk tcb_clk = {
-	.name		= "tcb_clk",
+static struct clk tcb0_clk = {
+	.name		= "tcb0_clk",
+	.pmc_mask	= 1 << AT91SAM9G45_ID_TCB,
+	.type		= CLK_TYPE_PERIPHERAL,
+};
+static struct clk tcb1_clk = {
+	.name		= "tcb1_clk",
 	.pmc_mask	= 1 << AT91SAM9G45_ID_TCB,
 	.type		= CLK_TYPE_PERIPHERAL,
 };
@@ -208,7 +213,8 @@ static struct clk *periph_clocks[] __initdata = {
 	&spi1_clk,
 	&ssc0_clk,
 	&ssc1_clk,
-	&tcb_clk,
+	&tcb0_clk,
+	&tcb1_clk,
 	&pwm_clk,
 	&tsc_clk,
 	&dma_clk,
diff --git a/arch/arm/mach-at91/at91sam9g45_devices.c b/arch/arm/mach-at91/at91sam9g45_devices.c
index 809114d..4822019 100644
--- a/arch/arm/mach-at91/at91sam9g45_devices.c
+++ b/arch/arm/mach-at91/at91sam9g45_devices.c
@@ -835,9 +835,9 @@ static struct platform_device at91sam9g45_tcb1_device = {
 static void __init at91_add_device_tc(void)
 {
 	/* this chip has one clock and irq for all six TC channels */
-	at91_clock_associate("tcb_clk", &at91sam9g45_tcb0_device.dev, "t0_clk");
+	at91_clock_associate("tcb0_clk", &at91sam9g45_tcb0_device.dev, "t0_clk");
 	platform_device_register(&at91sam9g45_tcb0_device);
-	at91_clock_associate("tcb_clk", &at91sam9g45_tcb1_device.dev, "t0_clk");
+	at91_clock_associate("tcb1_clk", &at91sam9g45_tcb1_device.dev, "t0_clk");
 	platform_device_register(&at91sam9g45_tcb1_device);
 }
 #else
-- 
1.6.6.1

[PATCH] AT91: SAM9G45 - add a separate clock entry for every single TC block

[Jean-Christophe PLAGNIOL-VILLARD]

On 13:31 Fri 03 Sep     , Fabian Godehardt wrote:
> Without this patch you will not be able to register the first block
> because of the second association call on at91_add_device_tc().
> 
> Signed-off-by: Fabian Godehardt <fg@emlix.com>
duplicate code is not a good idea

this is one of the reason I'm think to drop the current clock assotion code of
at91 to use the generic clkdev

here you can also do not specify the device as the clock is not device
specific

Best Regards,
J.

[PATCH] AT91: SAM9G45 - add a separate clock entry for every single TC block

[Nicolas Ferre]

Le 03/09/2010 14:31, Fabian Godehardt :
> Without this patch you will not be able to register the first block
> because of the second association call on at91_add_device_tc().

Yes, I noticed that.

> Signed-off-by: Fabian Godehardt <fg@emlix.com>

Tell me if my little modification is ok for you. I will then sign it and
send it to Russell patch tracking system: what do you think about it?

> ---
>  arch/arm/mach-at91/at91sam9g45.c         |   12 +++++++++---
>  arch/arm/mach-at91/at91sam9g45_devices.c |    4 ++--
>  2 files changed, 11 insertions(+), 5 deletions(-)
> 
> diff --git a/arch/arm/mach-at91/at91sam9g45.c b/arch/arm/mach-at91/at91sam9g45.c
> index 753c0d3..52ef2d6 100644
> --- a/arch/arm/mach-at91/at91sam9g45.c
> +++ b/arch/arm/mach-at91/at91sam9g45.c
> @@ -121,8 +121,13 @@ static struct clk ssc1_clk = {
>  	.pmc_mask	= 1 << AT91SAM9G45_ID_SSC1,
>  	.type		= CLK_TYPE_PERIPHERAL,
>  };
> -static struct clk tcb_clk = {
> -	.name		= "tcb_clk",
> +static struct clk tcb0_clk = {
> +	.name		= "tcb0_clk",
> +	.pmc_mask	= 1 << AT91SAM9G45_ID_TCB,
> +	.type		= CLK_TYPE_PERIPHERAL,
> +};
> +static struct clk tcb1_clk = {
> +	.name		= "tcb1_clk",
>  	.pmc_mask	= 1 << AT91SAM9G45_ID_TCB,
>  	.type		= CLK_TYPE_PERIPHERAL,

I prefer to add a "fake" clock that depend on the first one instead of
doubling the definition: like this:

/* One additional fake clock for second TC block */
static struct clk tcb1_clk = {
        .name           = "tcb1_clk",
        .pmc_mask       = 0,
        .type           = CLK_TYPE_PERIPHERAL,
        .parent         = &tcb0_clk,
};

This way we keep the single clock entry but we provide a fake entry for
registering the second TC block.


>  };
> @@ -208,7 +213,8 @@ static struct clk *periph_clocks[] __initdata = {
>  	&spi1_clk,
>  	&ssc0_clk,
>  	&ssc1_clk,
> -	&tcb_clk,
> +	&tcb0_clk,
> +	&tcb1_clk,
>  	&pwm_clk,
>  	&tsc_clk,
>  	&dma_clk,
> diff --git a/arch/arm/mach-at91/at91sam9g45_devices.c b/arch/arm/mach-at91/at91sam9g45_devices.c
> index 809114d..4822019 100644
> --- a/arch/arm/mach-at91/at91sam9g45_devices.c
> +++ b/arch/arm/mach-at91/at91sam9g45_devices.c
> @@ -835,9 +835,9 @@ static struct platform_device at91sam9g45_tcb1_device = {
>  static void __init at91_add_device_tc(void)
>  {
>  	/* this chip has one clock and irq for all six TC channels */
> -	at91_clock_associate("tcb_clk", &at91sam9g45_tcb0_device.dev, "t0_clk");
> +	at91_clock_associate("tcb0_clk", &at91sam9g45_tcb0_device.dev, "t0_clk");
>  	platform_device_register(&at91sam9g45_tcb0_device);
> -	at91_clock_associate("tcb_clk", &at91sam9g45_tcb1_device.dev, "t0_clk");
> +	at91_clock_associate("tcb1_clk", &at91sam9g45_tcb1_device.dev, "t0_clk");
>  	platform_device_register(&at91sam9g45_tcb1_device);
>  }
>  #else

Ok for other modifications,

Best regards,
-- 
Nicolas Ferre

[PATCH] AT91: SAM9G45 - add a separate clock entry for every single TC block

[Jean-Christophe PLAGNIOL-VILLARD]

On 15:05 Fri 03 Sep     , Nicolas Ferre wrote:
> Le 03/09/2010 14:31, Fabian Godehardt :
> > Without this patch you will not be able to register the first block
> > because of the second association call on at91_add_device_tc().
> 
> Yes, I noticed that.
> 
> > Signed-off-by: Fabian Godehardt <fg@emlix.com>
> 
> Tell me if my little modification is ok for you. I will then sign it and
> send it to Russell patch tracking system: what do you think about it?
> 
> > ---
> >  arch/arm/mach-at91/at91sam9g45.c         |   12 +++++++++---
> >  arch/arm/mach-at91/at91sam9g45_devices.c |    4 ++--
> >  2 files changed, 11 insertions(+), 5 deletions(-)
> > 
> > diff --git a/arch/arm/mach-at91/at91sam9g45.c b/arch/arm/mach-at91/at91sam9g45.c
> > index 753c0d3..52ef2d6 100644
> > --- a/arch/arm/mach-at91/at91sam9g45.c
> > +++ b/arch/arm/mach-at91/at91sam9g45.c
> > @@ -121,8 +121,13 @@ static struct clk ssc1_clk = {
> >  	.pmc_mask	= 1 << AT91SAM9G45_ID_SSC1,
> >  	.type		= CLK_TYPE_PERIPHERAL,
> >  };
> > -static struct clk tcb_clk = {
> > -	.name		= "tcb_clk",
> > +static struct clk tcb0_clk = {
> > +	.name		= "tcb0_clk",
> > +	.pmc_mask	= 1 << AT91SAM9G45_ID_TCB,
> > +	.type		= CLK_TYPE_PERIPHERAL,
> > +};
> > +static struct clk tcb1_clk = {
> > +	.name		= "tcb1_clk",
> >  	.pmc_mask	= 1 << AT91SAM9G45_ID_TCB,
> >  	.type		= CLK_TYPE_PERIPHERAL,
> 
> I prefer to add a "fake" clock that depend on the first one instead of
> doubling the definition: like this:
> 
> /* One additional fake clock for second TC block */
> static struct clk tcb1_clk = {
>         .name           = "tcb1_clk",
>         .pmc_mask       = 0,
>         .type           = CLK_TYPE_PERIPHERAL,
>         .parent         = &tcb0_clk,
> };
> 
this modification will also work when we enable and disable the clock
the fact of duplicating the clock will result that if we disable the clock
we will disable it for both device

in the Nico implementation the cloc will be disable only when no-one use it

it's not the best solution but for now on it's before switching to the clkdev

Acked-by: Jean-Christophe PLAGNIOL-VILLARD <plagnioj@jcrosoft.com>

Best Regards,
J.

[PATCH] AT91: SAM9G45 - add a separate clock entry for every single TC block

[Fabian Godehardt]

Hi,

Am Freitag 03 September 2010 14:05:43 schrieben Sie:
> Le 03/09/2010 14:31, Fabian Godehardt :
> > Without this patch you will not be able to register the first block
> > because of the second association call on at91_add_device_tc().
> 
> Yes, I noticed that.
> 
> > Signed-off-by: Fabian Godehardt <fg@emlix.com>
> 
> Tell me if my little modification is ok for you. I will then sign it and
> send it to Russell patch tracking system: what do you think about it?
> 
> > ---
> >  arch/arm/mach-at91/at91sam9g45.c         |   12 +++++++++---
> >  arch/arm/mach-at91/at91sam9g45_devices.c |    4 ++--
> >  2 files changed, 11 insertions(+), 5 deletions(-)
> >
> > diff --git a/arch/arm/mach-at91/at91sam9g45.c
> > b/arch/arm/mach-at91/at91sam9g45.c index 753c0d3..52ef2d6 100644
> > --- a/arch/arm/mach-at91/at91sam9g45.c
> > +++ b/arch/arm/mach-at91/at91sam9g45.c
> > @@ -121,8 +121,13 @@ static struct clk ssc1_clk = {
> >  	.pmc_mask	= 1 << AT91SAM9G45_ID_SSC1,
> >  	.type		= CLK_TYPE_PERIPHERAL,
> >  };
> > -static struct clk tcb_clk = {
> > -	.name		= "tcb_clk",
> > +static struct clk tcb0_clk = {
> > +	.name		= "tcb0_clk",
> > +	.pmc_mask	= 1 << AT91SAM9G45_ID_TCB,
> > +	.type		= CLK_TYPE_PERIPHERAL,
> > +};
> > +static struct clk tcb1_clk = {
> > +	.name		= "tcb1_clk",
> >  	.pmc_mask	= 1 << AT91SAM9G45_ID_TCB,
> >  	.type		= CLK_TYPE_PERIPHERAL,
> 
> I prefer to add a "fake" clock that depend on the first one instead of
> doubling the definition: like this:
> 
> /* One additional fake clock for second TC block */
> static struct clk tcb1_clk = {
>         .name           = "tcb1_clk",
>         .pmc_mask       = 0,
>         .type           = CLK_TYPE_PERIPHERAL,
>         .parent         = &tcb0_clk,
> };
> 
> This way we keep the single clock entry but we provide a fake entry for
> registering the second TC block.

It's ok and works for me - thanks.

Acked-by: Fabian Godehardt <fg@emlix.com>


Best regards,
Fabian

[PATCH] AT91: SAM9G45 - add a separate clock entry for every single TC block

[Nicolas Ferre]

From: Fabian Godehardt <fg@emlix.com>

Without this patch you will not be able to register the first block
because of the second association call on at91_add_device_tc().

Signed-off-by: Fabian Godehardt <fg@emlix.com>
[nicolas.ferre at atmel.com: change tcb1_clk to fake child clock of tcb0_clk]
Acked-by: Jean-Christophe PLAGNIOL-VILLARD <plagnioj@jcrosoft.com>
Signed-off-by: Nicolas Ferre <nicolas.ferre@atmel.com>
---
 arch/arm/mach-at91/at91sam9g45.c         |   15 ++++++++++++---
 arch/arm/mach-at91/at91sam9g45_devices.c |    4 ++--
 2 files changed, 14 insertions(+), 5 deletions(-)

diff --git a/arch/arm/mach-at91/at91sam9g45.c b/arch/arm/mach-at91/at91sam9g45.c
index 753c0d3..c67b47f 100644
--- a/arch/arm/mach-at91/at91sam9g45.c
+++ b/arch/arm/mach-at91/at91sam9g45.c
@@ -121,8 +121,8 @@ static struct clk ssc1_clk = {
 	.pmc_mask	= 1 << AT91SAM9G45_ID_SSC1,
 	.type		= CLK_TYPE_PERIPHERAL,
 };
-static struct clk tcb_clk = {
-	.name		= "tcb_clk",
+static struct clk tcb0_clk = {
+	.name		= "tcb0_clk",
 	.pmc_mask	= 1 << AT91SAM9G45_ID_TCB,
 	.type		= CLK_TYPE_PERIPHERAL,
 };
@@ -192,6 +192,14 @@ static struct clk ohci_clk = {
 	.parent		= &uhphs_clk,
 };
 
+/* One additional fake clock for second TC block */
+static struct clk tcb1_clk = {
+	.name		= "tcb1_clk",
+	.pmc_mask	= 0,
+	.type		= CLK_TYPE_PERIPHERAL,
+	.parent		= &tcb0_clk,
+};
+
 static struct clk *periph_clocks[] __initdata = {
 	&pioA_clk,
 	&pioB_clk,
@@ -208,7 +216,7 @@ static struct clk *periph_clocks[] __initdata = {
 	&spi1_clk,
 	&ssc0_clk,
 	&ssc1_clk,
-	&tcb_clk,
+	&tcb0_clk,
 	&pwm_clk,
 	&tsc_clk,
 	&dma_clk,
@@ -221,6 +229,7 @@ static struct clk *periph_clocks[] __initdata = {
 	&mmc1_clk,
 	// irq0
 	&ohci_clk,
+	&tcb1_clk,
 };
 
 /*
diff --git a/arch/arm/mach-at91/at91sam9g45_devices.c b/arch/arm/mach-at91/at91sam9g45_devices.c
index 809114d..4822019 100644
--- a/arch/arm/mach-at91/at91sam9g45_devices.c
+++ b/arch/arm/mach-at91/at91sam9g45_devices.c
@@ -835,9 +835,9 @@ static struct platform_device at91sam9g45_tcb1_device = {
 static void __init at91_add_device_tc(void)
 {
 	/* this chip has one clock and irq for all six TC channels */
-	at91_clock_associate("tcb_clk", &at91sam9g45_tcb0_device.dev, "t0_clk");
+	at91_clock_associate("tcb0_clk", &at91sam9g45_tcb0_device.dev, "t0_clk");
 	platform_device_register(&at91sam9g45_tcb0_device);
-	at91_clock_associate("tcb_clk", &at91sam9g45_tcb1_device.dev, "t0_clk");
+	at91_clock_associate("tcb1_clk", &at91sam9g45_tcb1_device.dev, "t0_clk");
 	platform_device_register(&at91sam9g45_tcb1_device);
 }
 #else
-- 
1.5.6.5

[PATCH] AT91: SAM9G45 - add a separate clock entry for every single TC block

[avictor.za at gmail.com]

> Without this patch you will not be able to register the first block
> because of the second association call on at91_add_device_tc().
>
> Signed-off-by: Fabian Godehardt <fg@emlix.com>
> [nicolas.ferre at atmel.com: change tcb1_clk to fake child clock of tcb0_clk]
> Acked-by: Jean-Christophe PLAGNIOL-VILLARD <plagnioj@jcrosoft.com>
> Signed-off-by: Nicolas Ferre

Acked-By: Andrew Victor <linux@maxim.org.za>

[PATCH] AT91: SAM9G45 - add a separate clock entry for every single TC block

[avictor.za at gmail.com]

hi,

> +/* One additional fake clock for second TC block */
> +static struct clk tcb1_clk = {
> + ? ? ? .name ? ? ? ? ? = "tcb1_clk",
> + ? ? ? .pmc_mask ? ? ? = 0,
> + ? ? ? .type ? ? ? ? ? = CLK_TYPE_PERIPHERAL,
> + ? ? ? .parent ? ? ? ? = &tcb0_clk,
> +};
> +

Looking at this again...  since type is CLK_TYPE_PERIPHERAL, when you
call clk_register() the "parent" is changed to the master clock.

Which means, then later you call clk_enable() the "pmc_mask" is still
0, so 0 gets written (in pmc_periph_mode) to AT91_PMC_PCER.  So the
TCB clock won't be enabled.

Or am I missing something?


Regards,
  Andrew Victor

[PATCH] AT91: SAM9G45 - add a separate clock entry for every single TC block

[Nicolas Ferre]

Le 07/09/2010 21:42, avictor.za at gmail.com :
> hi,
> 
>> +/* One additional fake clock for second TC block */
>> +static struct clk tcb1_clk = {
>> +       .name           = "tcb1_clk",
>> +       .pmc_mask       = 0,
>> +       .type           = CLK_TYPE_PERIPHERAL,
>> +       .parent         = &tcb0_clk,
>> +};
>> +
> 
> Looking at this again...  since type is CLK_TYPE_PERIPHERAL, when you
> call clk_register() the "parent" is changed to the master clock.
> 
> Which means, then later you call clk_enable() the "pmc_mask" is still
> 0, so 0 gets written (in pmc_periph_mode) to AT91_PMC_PCER.  So the
> TCB clock won't be enabled.
> 
> Or am I missing something?

You are absolutely right!

What do you think about this modification of clk_register() function?

--- a/arch/arm/mach-at91/clock.c
+++ b/arch/arm/mach-at91/clock.c
@@ -501,7 +501,8 @@ postcore_initcall(at91_clk_debugfs_init);
 int __init clk_register(struct clk *clk)
 {
        if (clk_is_peripheral(clk)) {
-               clk->parent = &mck;
+               if (!clk->parent)
+                       clk->parent = &mck;
                clk->mode = pmc_periph_mode;
                list_add_tail(&clk->node, &clocks);
        }

It is a very little modification which implements what I had in mind
while creating a kind of "child peripheral" clock.

Best regards,
-- 
Nicolas Ferre

[PATCH] AT91: SAM9G45 - add a separate clock entry for every single TC block

[Jean-Christophe PLAGNIOL-VILLARD]

On 11:00 Wed 08 Sep     , Nicolas Ferre wrote:
> Le 07/09/2010 21:42, avictor.za at gmail.com :
> > hi,
> > 
> >> +/* One additional fake clock for second TC block */
> >> +static struct clk tcb1_clk = {
> >> +       .name           = "tcb1_clk",
> >> +       .pmc_mask       = 0,
> >> +       .type           = CLK_TYPE_PERIPHERAL,
> >> +       .parent         = &tcb0_clk,
> >> +};
> >> +
> > 
> > Looking at this again...  since type is CLK_TYPE_PERIPHERAL, when you
> > call clk_register() the "parent" is changed to the master clock.
> > 
> > Which means, then later you call clk_enable() the "pmc_mask" is still
> > 0, so 0 gets written (in pmc_periph_mode) to AT91_PMC_PCER.  So the
> > TCB clock won't be enabled.
> > 
> > Or am I missing something?
> 
> You are absolutely right!
> 
> What do you think about this modification of clk_register() function?
> 
> --- a/arch/arm/mach-at91/clock.c
> +++ b/arch/arm/mach-at91/clock.c
> @@ -501,7 +501,8 @@ postcore_initcall(at91_clk_debugfs_init);
>  int __init clk_register(struct clk *clk)
>  {
>         if (clk_is_peripheral(clk)) {
> -               clk->parent = &mck;
> +               if (!clk->parent)
> +                       clk->parent = &mck;
>                 clk->mode = pmc_periph_mode;
>                 list_add_tail(&clk->node, &clocks);
>         }
> 
> It is a very little modification which implements what I had in mind
> while creating a kind of "child peripheral" clock.
so so

but until we switch to clkdev it will solve the issue

Acked-by: Jean-Christophe PLAGNIOL-VILLARD <plagnioj@jcrosoft.com>

Best Regards,
J.