[PATCH] [POWERPC] 8xx: timebase frequency should not depend on bus-frequency

[PATCH] [POWERPC] 8xx: timebase frequency should not depend on bus-frequency

[Anton Vorontsov]

m8xx_setup.c says:
   /* Force all 8xx processors to use divide by 16 processor clock. */

And at the same time it is using bus-frequency for calculating
timebase. It is okay for most setups because bus-frequency is
equal to clock-frequency.

The problem emerges when cpu frequency is > 66MHz, quoting
u-boot/cpu/mpc8xx/speed.c:

        if (gd->cpu_clk <= 66000000) {
                sccr_reg |= SCCR_EBDF00;        /* bus division factor = 1 */
                gd->bus_clk = gd->cpu_clk;
        } else {
                sccr_reg |= SCCR_EBDF01;        /* bus division factor = 2 */
                gd->bus_clk = gd->cpu_clk / 2;
        }

So in case of cpu clock > 66MHz, bus_clk = cpu_clk / 2. An then, from
Linux, we calculate timebase frequency as tb_freq = bus_clk / 16,
that is cpu_clk / 2 / 16, which is wrong.

This patch fixes system time drifting problem on the EP885C board
running at 133MHz.

Signed-off-by: Anton Vorontsov <avorontsov@ru.mvista.com>
---
 arch/powerpc/platforms/8xx/m8xx_setup.c |    7 +------
 1 files changed, 1 insertions(+), 6 deletions(-)

diff --git a/arch/powerpc/platforms/8xx/m8xx_setup.c b/arch/powerpc/platforms/8xx/m8xx_setup.c
index 184f998..0d9f75c 100644
--- a/arch/powerpc/platforms/8xx/m8xx_setup.c
+++ b/arch/powerpc/platforms/8xx/m8xx_setup.c
@@ -111,17 +111,12 @@ void __init mpc8xx_calibrate_decr(void)
 
 	/* Processor frequency is MHz.
 	 */
-	ppc_tb_freq = 50000000;
-	if (!get_freq("bus-frequency", &ppc_tb_freq)) {
-		printk(KERN_ERR "WARNING: Estimating decrementer frequency "
-		                "(not found)\n");
-	}
-	ppc_tb_freq /= 16;
 	ppc_proc_freq = 50000000;
 	if (!get_freq("clock-frequency", &ppc_proc_freq))
 		printk(KERN_ERR "WARNING: Estimating processor frequency "
 		                "(not found)\n");
 
+	ppc_tb_freq = ppc_proc_freq / 16;
 	printk("Decrementer Frequency = 0x%lx\n", ppc_tb_freq);
 
 	/* Perform some more timer/timebase initialization.  This used
-- 
1.5.2.2

Re: [PATCH] [POWERPC] 8xx: timebase frequency should not depend on bus-frequency

[Scott Wood]

Anton Vorontsov wrote:
> diff --git a/arch/powerpc/platforms/8xx/m8xx_setup.c b/arch/powerpc/platforms/8xx/m8xx_setup.c
> index 184f998..0d9f75c 100644
> --- a/arch/powerpc/platforms/8xx/m8xx_setup.c
> +++ b/arch/powerpc/platforms/8xx/m8xx_setup.c
> @@ -111,17 +111,12 @@ void __init mpc8xx_calibrate_decr(void)
>  
>  	/* Processor frequency is MHz.
>  	 */
> -	ppc_tb_freq = 50000000;
> -	if (!get_freq("bus-frequency", &ppc_tb_freq)) {
> -		printk(KERN_ERR "WARNING: Estimating decrementer frequency "
> -		                "(not found)\n");
> -	}
> -	ppc_tb_freq /= 16;
>  	ppc_proc_freq = 50000000;
>  	if (!get_freq("clock-frequency", &ppc_proc_freq))
>  		printk(KERN_ERR "WARNING: Estimating processor frequency "
>  		                "(not found)\n");
>  
> +	ppc_tb_freq = ppc_proc_freq / 16;

Shouldn't we just use the timebase-frequency property?

-Scott

Re: [PATCH] [POWERPC] 8xx: timebase frequency should not depend on bus-frequency

[Anton Vorontsov]

On Thu, Feb 21, 2008 at 01:48:23PM -0600, Scott Wood wrote:
> Anton Vorontsov wrote:
> >diff --git a/arch/powerpc/platforms/8xx/m8xx_setup.c 
> >b/arch/powerpc/platforms/8xx/m8xx_setup.c
> >index 184f998..0d9f75c 100644
> >--- a/arch/powerpc/platforms/8xx/m8xx_setup.c
> >+++ b/arch/powerpc/platforms/8xx/m8xx_setup.c
> >@@ -111,17 +111,12 @@ void __init mpc8xx_calibrate_decr(void)
> > 
> > 	/* Processor frequency is MHz.
> > 	 */
> >-	ppc_tb_freq = 50000000;
> >-	if (!get_freq("bus-frequency", &ppc_tb_freq)) {
> >-		printk(KERN_ERR "WARNING: Estimating decrementer frequency "
> >-		                "(not found)\n");
> >-	}
> >-	ppc_tb_freq /= 16;
> > 	ppc_proc_freq = 50000000;
> > 	if (!get_freq("clock-frequency", &ppc_proc_freq))
> > 		printk(KERN_ERR "WARNING: Estimating processor frequency "
> > 		                "(not found)\n");
> > 
> >+	ppc_tb_freq = ppc_proc_freq / 16;
> 
> Shouldn't we just use the timebase-frequency property?

Nope. Most u-boots currently do not setup timebase-frequency, and if
they are setting it up, they're doing it wrong, in sense that Linux
overwrites timebase setup (yeah, in this regard MPC8xx is special).

So, for MPC8xx, Linux doesn't care what timebase setup was used in
the firmware.

-- 
Anton Vorontsov
email: cbou@mail.ru
backup email: ya-cbou@yandex.ru
irc://irc.freenode.net/bd2

Re: [PATCH] [POWERPC] 8xx: timebase frequency should not depend on bus-frequency

[Scott Wood]

Anton Vorontsov wrote:
> On Thu, Feb 21, 2008 at 01:48:23PM -0600, Scott Wood wrote:
>> Anton Vorontsov wrote:
>>> diff --git a/arch/powerpc/platforms/8xx/m8xx_setup.c 
>>> b/arch/powerpc/platforms/8xx/m8xx_setup.c
>>> index 184f998..0d9f75c 100644
>>> --- a/arch/powerpc/platforms/8xx/m8xx_setup.c
>>> +++ b/arch/powerpc/platforms/8xx/m8xx_setup.c
>>> @@ -111,17 +111,12 @@ void __init mpc8xx_calibrate_decr(void)
>>>
>>> 	/* Processor frequency is MHz.
>>> 	 */
>>> -	ppc_tb_freq = 50000000;
>>> -	if (!get_freq("bus-frequency", &ppc_tb_freq)) {
>>> -		printk(KERN_ERR "WARNING: Estimating decrementer frequency "
>>> -		                "(not found)\n");
>>> -	}
>>> -	ppc_tb_freq /= 16;
>>> 	ppc_proc_freq = 50000000;
>>> 	if (!get_freq("clock-frequency", &ppc_proc_freq))
>>> 		printk(KERN_ERR "WARNING: Estimating processor frequency "
>>> 		                "(not found)\n");
>>>
>>> +	ppc_tb_freq = ppc_proc_freq / 16;
>> Shouldn't we just use the timebase-frequency property?
> 
> Nope. Most u-boots currently do not setup timebase-frequency, and if
> they are setting it up, they're doing it wrong, in sense that Linux
> overwrites timebase setup (yeah, in this regard MPC8xx is special).

Current u-boots don't support device trees at all on 8xx.  The most 
recent 8xx FDT patch I saw called get_tbclk() to fill in 
timebase-frequency; does get_tbclk() not work?

Obviously, when booting with a cuImage we can't use u-boot's value, 
since it's not in the bd_t.  In that case, we should fix the wrapper's 
calculation of the timebase frequency.

-Scott

Re: [PATCH] [POWERPC] 8xx: timebase frequency should not depend on bus-frequency

[Anton Vorontsov]

On Thu, Feb 21, 2008 at 02:06:58PM -0600, Scott Wood wrote:
[...]
> >>>+++ b/arch/powerpc/platforms/8xx/m8xx_setup.c
> >>>@@ -111,17 +111,12 @@ void __init mpc8xx_calibrate_decr(void)
> >>>
> >>>	/* Processor frequency is MHz.
> >>>	 */
> >>>-	ppc_tb_freq = 50000000;
> >>>-	if (!get_freq("bus-frequency", &ppc_tb_freq)) {
> >>>-		printk(KERN_ERR "WARNING: Estimating decrementer frequency "
> >>>-		                "(not found)\n");
> >>>-	}
> >>>-	ppc_tb_freq /= 16;
> >>>	ppc_proc_freq = 50000000;
> >>>	if (!get_freq("clock-frequency", &ppc_proc_freq))
> >>>		printk(KERN_ERR "WARNING: Estimating processor frequency "
> >>>		                "(not found)\n");
> >>>
> >>>+	ppc_tb_freq = ppc_proc_freq / 16;
> >>Shouldn't we just use the timebase-frequency property?
> >
> >Nope. Most u-boots currently do not setup timebase-frequency, and if
> >they are setting it up, they're doing it wrong, in sense that Linux
> >overwrites timebase setup (yeah, in this regard MPC8xx is special).
> 
> Current u-boots don't support device trees at all on 8xx.

Yes, vanilla u-boots. I assume many of us use some u-boot hacks to
actually boot with the device tree (no, not cuboots)... ;-)

> The most 
> recent 8xx FDT patch I saw called get_tbclk() to fill in 
> timebase-frequency; does get_tbclk() not work?

I don't know. I didn't look at this, it isn't interesting because
currently getting timebase-frequency from the device tree is
meaningless on MPC8xx. Linux is doing timebase stuff on its own.

Surely, moving timebase setup code into the cuboot will solve
backwards compatibility issues, so we'll setup timebase-frequency
there, and will use generic_calibrate_decr(). And for FDT-aware
u-boots we'll simply get fixed up timebase-frequency.

Unfortunately (or fortunately ;-) I don't have this 8xx board
any longer, so I'd suggest to not expect such 8xx cleanups from
me in near future...

If you have such patches pending... surely, they would be more
correct way to solve the issue.

Thanks,

-- 
Anton Vorontsov
email: cbou@mail.ru
backup email: ya-cbou@yandex.ru
irc://irc.freenode.net/bd2

Re: [PATCH] [POWERPC] 8xx: timebase frequency should not depend on bus-frequency

[Scott Wood]

Anton Vorontsov wrote:
> On Thu, Feb 21, 2008 at 02:06:58PM -0600, Scott Wood wrote:
>> Current u-boots don't support device trees at all on 8xx.
> 
> Yes, vanilla u-boots. I assume many of us use some u-boot hacks to
> actually boot with the device tree (no, not cuboots)... ;-)

Fine, but don't expect misbehavior from out-of-tree u-boots to be used 
as justification for the kernel ignoring device tree content. :-)

-Scott

Re: [PATCH] [POWERPC] 8xx: timebase frequency should not depend on bus-frequency

[Anton Vorontsov]

On Thu, Feb 21, 2008 at 03:20:10PM -0600, Scott Wood wrote:
> Anton Vorontsov wrote:
> > On Thu, Feb 21, 2008 at 02:06:58PM -0600, Scott Wood wrote:
> >> Current u-boots don't support device trees at all on 8xx.
> > 
> > Yes, vanilla u-boots. I assume many of us use some u-boot hacks to
> > actually boot with the device tree (no, not cuboots)... ;-)
> 
> Fine, but don't expect misbehavior from out-of-tree u-boots to be used 
> as justification for the kernel ignoring device tree content. :-)

You got me wrong, maybe I wasn't clear enough: it wasn't justification of
any kind. It's was just a remark regarding u-boot still not supporting
fdt for 8xx (20 lines of code we're lazy to cleanup, write annotation
to the patch and send it ;-).

Regarding timebase issue. This issue isn't introduced by the
u-boots. It is _Linux_ code that is buggy. I'm agree that ideally we
should get timebase-frequency from the device tree, but if we'll
start getting it from inside the current code, it will look like:

...linux setups tbfreq = cpu_clk / 16...
tbfreq = of_get_property(.."timebase-frequency"..);

You see? This is illogical, error-prone, and whatnot. :-)
I agree, to fix this right way, we should:
a) fix current u-boots;
b) rework 8xx/ Linux code; and
c) rework cuboot for compatibility;

Until that happen that tiny patch is a valid bug fix, right?

-- 
Anton Vorontsov
email: cbou@mail.ru
backup email: ya-cbou@yandex.ru
irc://irc.freenode.net/bd2

Re: [PATCH] [POWERPC] 8xx: timebase frequency should not depend on bus-frequency

[Bryan O'Donoghue]

On Fri, 22 Feb 2008 02:41:36 +0300
Anton Vorontsov <cbouatmailru@gmail.com> wrote:

> On Thu, Feb 21, 2008 at 03:20:10PM -0600, Scott Wood wrote:
> > Anton Vorontsov wrote:
> > > On Thu, Feb 21, 2008 at 02:06:58PM -0600, Scott Wood wrote:
> > >> Current u-boots don't support device trees at all on 8xx.
> > > 
> > > Yes, vanilla u-boots. I assume many of us use some u-boot hacks to
> > > actually boot with the device tree (no, not cuboots)... ;-)
> > 
> > Fine, but don't expect misbehavior from out-of-tree u-boots to be used 
> > as justification for the kernel ignoring device tree content. :-)
> 
> You got me wrong, maybe I wasn't clear enough: it wasn't justification of
> any kind. It's was just a remark regarding u-boot still not supporting
> fdt for 8xx (20 lines of code we're lazy to cleanup, write annotation
> to the patch and send it ;-).

Hey Anton.

I won't make any comment on what Linux should do with an older version of
u-boot.

However, just to let you know, there are a couple of patches to do a more modern
fdt on 8xx- sent to the u-boot list in the last <= ~ 10 days.

Using those patches - which incorporate the changes suggested by Scott re:
get_tbclk()- the timebase-frequency field is indeed stuffed with u-boot's
get_tbclk(); - see below.

u-boot/cpu/mpc8xx/fdt.c

void ft_cpu_setup(void *blob, bd_t *bd)
{
        do_fixup_by_prop_u32(blob, "device_type", "cpu", 4,
                "timebase-frequency", get_tbclk(), 1);
        do_fixup_by_prop_u32(blob, "device_type", "cpu", 4,
                "bus-frequency", bd->bi_busfreq, 1);
        do_fixup_by_prop_u32(blob, "device_type", "cpu", 4,
                "clock-frequency", bd->bi_intfreq, 1);
        do_fixup_by_compat_u32(blob, "fsl,cpm-brg", "clock-frequency",
                gd->brg_clk, 1);

        /* Fixup ethernet MAC addresses */
        fdt_fixup_ethernet(blob, bd);

        fdt_fixup_memory(blob, (u64)bd->bi_memstart, (u64)bd->bi_memsize);
}

Adding fdt support to an 8xx board is then as simple as defining

/* pass open firmware flat tree */
#define CONFIG_OF_LIBFDT        1
#define CONFIG_OF_BOARD_SETUP   1


In your u-boot/include/configs/MySuperBoard.h

Best,
Bryan