"Reddy, Teerth" <teerth@xxxxxx> writes:
> From e5e82efbf6f736984668cc5e94d62635ed4de05e Mon Sep 17 00:00:00 2001
> From: Teerth Reddy <teerth@xxxxxx>
> Date: Wed, 23 Dec 2009 18:40:34 +0530
> Subject: [PATCH] Dynamic Calculation of SDRC clock stabilization delay
Subject prefix should probably be 'OMAP3: SDRC:'.
> The patch has the changes to calculate the dpll3 clock
> stabilization delay dynamically. The SRAM delay is
> calibrated during bootup using the gptimers and used while
> calculating the stabilization delay.
Thanks for the update using the GPtimer. I think this simplifies
things quite a bit in terms of upstream code, however there are a
couple of new snags. More below...
> By using the dynamic
> method the dependency on the type of cache being used is
> removed. Hence there is no need of loop based calculation.
>
> TO DO: However the value of 6us for SDRC to stabilize has
> been hardcoded currently. The formula used to calculate
> this value gives a very small number thus causing
> instability. The right formula will be used which will
> work across all boards.
Can you explain which platforms the hard-coded value should work for
and how that value was determined?
Also, should we be expecting another patch for this 'TO DO' part.
To me this is a problem that prevents merging this patch.
> Signed-off-by: Teerth Reddy <teerth@xxxxxx>
> Signed-off-by: Romit Dasgupta <romit@xxxxxx>
> ---
> arch/arm/mach-omap2/clock34xx.c | 21 +++++++++------
> arch/arm/mach-omap2/clock34xx.h | 2 +
> arch/arm/mach-omap2/clock34xx_data.c | 4 +++
> arch/arm/mach-omap2/sram34xx.S | 25 ++++++++++++++++++
> arch/arm/plat-omap/include/plat/sram.h | 5 +++
> arch/arm/plat-omap/sram.c | 43 ++++++++++++++++++++++++++++++++
> 6 files changed, 91 insertions(+), 9 deletions(-)
>
> diff --git a/arch/arm/mach-omap2/clock34xx.c b/arch/arm/mach-omap2/clock34xx.c
> index d9329e2..6d87419 100644
> --- a/arch/arm/mach-omap2/clock34xx.c
> +++ b/arch/arm/mach-omap2/clock34xx.c
> @@ -56,9 +56,18 @@
> */
> #define DPLL5_FREQ_FOR_USBHOST 120000000
>
> +/*
> + * SDRC_TIME_STABILIZE: Time for SDRC to stabilize in us
> + * TO DO: Use formula to calculate across all platforms
> + */
> +#define SDRC_TIME_STABILIZE 6
> +
> /* needed by omap3_core_dpll_m2_set_rate() */
> struct clk *sdrc_ick_p, *arm_fck_p;
>
> +/* SRAM delay required for sdrc clk stab delay calculation */
> +unsigned int delay_sram;
> +
Please get rid of the global variable and get the delay value
by calling a function that returns the value. Also, please
be sure to do this with SMP in mind.
> /**
> * omap3430es2_clk_ssi_find_idlest - return CM_IDLEST info for SSI
> * @clk: struct clk * being enabled
> @@ -215,16 +224,10 @@ int omap3_core_dpll_m2_set_rate(struct clk *clk, unsigned long rate)
> unlock_dll = 1;
> }
>
> - /*
> - * XXX This only needs to be done when the CPU frequency changes
> - */
> + /* Calculate the number of MPU cycles to wait for SDRC to stabilize */
> +
> _mpurate = arm_fck_p->rate / CYCLES_PER_MHZ;
> - c = (_mpurate << SDRC_MPURATE_SCALE) >> SDRC_MPURATE_BASE_SHIFT;
> - c += 1; /* for safety */
> - c *= SDRC_MPURATE_LOOPS;
> - c >>= SDRC_MPURATE_SCALE;
> - if (c == 0)
> - c = 1;
> + c = ((SDRC_TIME_STABILIZE * _mpurate) / (delay_sram * 2));
>
> pr_debug("clock: changing CORE DPLL rate from %lu to %lu\n", clk->rate,
> validrate);
> diff --git a/arch/arm/mach-omap2/clock34xx.h b/arch/arm/mach-omap2/clock34xx.h
> index 9a2c07e..1bc5044 100644
> --- a/arch/arm/mach-omap2/clock34xx.h
> +++ b/arch/arm/mach-omap2/clock34xx.h
> @@ -21,4 +21,6 @@ extern const struct clkops clkops_omap3430es2_hsotgusb_wait;
> extern const struct clkops clkops_omap3430es2_dss_usbhost_wait;
> extern const struct clkops clkops_noncore_dpll_ops;
>
> +extern unsigned int delay_sram;
> +
> #endif
> diff --git a/arch/arm/mach-omap2/clock34xx_data.c b/arch/arm/mach-omap2/clock34xx_data.c
> index 8bdcc9c..e5d0941 100644
> --- a/arch/arm/mach-omap2/clock34xx_data.c
> +++ b/arch/arm/mach-omap2/clock34xx_data.c
> @@ -22,6 +22,7 @@
>
> #include <plat/control.h>
> #include <plat/clkdev_omap.h>
> +#include <plat/sram.h>
>
> #include "clock.h"
> #include "clock34xx.h"
> @@ -3285,5 +3286,8 @@ int __init omap2_clk_init(void)
> sdrc_ick_p = clk_get(NULL, "sdrc_ick");
> arm_fck_p = clk_get(NULL, "arm_fck");
>
> + /* Measure sram delay */
> + delay_sram = measure_sram_delay(10000);
> + pr_debug("SRAM delay: %d\n", delay_sram);
> return 0;
> }
> diff --git a/arch/arm/mach-omap2/sram34xx.S b/arch/arm/mach-omap2/sram34xx.S
> index de99ba2..b4fba50 100644
> --- a/arch/arm/mach-omap2/sram34xx.S
> +++ b/arch/arm/mach-omap2/sram34xx.S
> @@ -68,6 +68,10 @@
> /* CM_CLKSEL1_PLL bit settings */
> #define CORE_DPLL_CLKOUT_DIV_SHIFT 0x1b
>
> +#define GPTIMER10_TCRR_PHY 0x48086028
> +
> +#define GPTIMER10_TCRR OMAP2_L4_IO_ADDRESS(GPTIMER10_TCRR_PHY)
> +
> /*
> * omap3_sram_configure_core_dpll - change DPLL3 M2 divider
> *
> @@ -313,3 +317,24 @@ core_m2_mask_val:
> ENTRY(omap3_sram_configure_core_dpll_sz)
> .word . - omap3_sram_configure_core_dpll
>
> +ENTRY(__sram_wait_delay)
> + stmfd sp!, {r1-r12, lr} @ store regs to stack
> + ldr r2, gptimer10_counter
> + ldr r3, [r2]
> +
> +loop1:
> + subs r0, r0, #1
> + bne loop1
> +
> + isb
> + ldr r4, [r2]
> + subs r5, r4, r3
> +
> + mov r0, r5 @ return value
> + ldmfd sp!, {r1-r12, pc} @ restore regs and return
> +
> +gptimer10_counter:
> + .word GPTIMER10_TCRR
> +
> +ENTRY(__sram_wait_delay_sz)
> + .word . - __sram_wait_delay
> diff --git a/arch/arm/plat-omap/include/plat/sram.h b/arch/arm/plat-omap/include/plat/sram.h
> index 16a1b45..7d0be2a 100644
> --- a/arch/arm/plat-omap/include/plat/sram.h
> +++ b/arch/arm/plat-omap/include/plat/sram.h
> @@ -69,6 +69,11 @@ extern u32 omap3_sram_configure_core_dpll(
> u32 sdrc_actim_ctrl_b_1, u32 sdrc_mr_1);
> extern unsigned long omap3_sram_configure_core_dpll_sz;
>
> +extern unsigned int measure_sram_delay(unsigned int);
> +
> +extern u32 __sram_wait_delay(unsigned int);
> +extern unsigned long __sram_wait_delay_sz;
> +
> #ifdef CONFIG_PM
> extern void omap_push_sram_idle(void);
> #else
> diff --git a/arch/arm/plat-omap/sram.c b/arch/arm/plat-omap/sram.c
> index d8d5094..ddbdaaf 100644
> --- a/arch/arm/plat-omap/sram.c
> +++ b/arch/arm/plat-omap/sram.c
> @@ -30,6 +30,9 @@
> #include <plat/cpu.h>
> #include <plat/vram.h>
>
> +#include <linux/clk.h>
> +#include <plat/dmtimer.h>
> +
> #include <plat/control.h>
>
> #if defined(CONFIG_ARCH_OMAP2) || defined(CONFIG_ARCH_OMAP3)
> @@ -437,11 +440,51 @@ static inline int omap34xx_sram_init(void)
> }
> #endif
>
> +
> +#ifdef CONFIG_ARCH_OMAP3
> +unsigned long (*_omap3_sram_delay)(unsigned long);
> +unsigned int measure_sram_delay(unsigned int loop)
> +{
> + static struct omap_dm_timer *gpt;
> + unsigned long flags, diff = 0, gt_rate, mpurate;
> + unsigned int delay_sram, error_gain;
> +
> + omap_dm_timer_init();
> + gpt = omap_dm_timer_request_specific(10);
While I understand the reasons you used a specific timer, I'm not
crazy about it. Among other things, it has also forced you to
hard-code OMAP3 specific register values where this should be done
more generically.
The sram_delay function should take a struct dm_timer as an argument,
but there's currently no way to get the base address. So, I see a
couple options.
Add a omap_dm_timer_get_phys_base() or similar to the DM timer API and
then pass that value into the sram delay function and use register
offsets in the asm function instead of fixed addresses.
That being said however, by default, the GPtimers are in posted mode,
so your reads are not taking that into account.
Ideally, what's needed is extending the DMtimer API to have a delay
function. Something like:
/* add kerneldoc description here */
unsigned int omap_dm_timer_delay_loops(struct omap_dm_timer *timer, u32 loops)
{
unsigned int start, end;
start = omap_dm_timer_read_reg(timer, OMAP_TIMER_COUNTER_REG);
while(loops--)
;
end = omap_dm_timer_read_reg(timer, OMAP_TIMER_COUNTER_REG);
return start - end;
}
which is then copied to SRAM to be executed. This would possibly lead
to a few extra cycles counted because the stack variables and the
calls to the DMtimer API would in SDRAM, but the tightloop would be
running from SRAM.
Of course, I've been on vacation for two weeks, so I could be
forgetting some other basic reason this wouldn't work.
> + if (!gpt)
> + pr_err("Could not get the gptimer\n");
> + omap_dm_timer_set_source(gpt, OMAP_TIMER_SRC_SYS_CLK);
> +
> + gt_rate = clk_get_rate(omap_dm_timer_get_fclk(gpt));
> + omap_dm_timer_set_load_start(gpt, 0, 0);
> +
> + local_irq_save(flags);
> + diff = _omap3_sram_delay(loop);
> + local_irq_restore(flags);
> +
> + omap_dm_timer_stop(gpt);
> + omap_dm_timer_free(gpt);
> +
> + mpurate = clk_get_rate(clk_get(NULL, "arm_fck"));
Please some error checking when using clock API.
> + /* calculate the sram delay */
> + delay_sram = (((mpurate / gt_rate) * diff) / 20000);
> +
> + error_gain = mpurate / gt_rate;
> + delay_sram = delay_sram + error_gain;
> +
> + return delay_sram;
> +}
> +#endif
> +
> int __init omap_sram_init(void)
> {
> omap_detect_sram();
> omap_map_sram();
>
> + _omap3_sram_delay = omap_sram_push(__sram_wait_delay,
> + __sram_wait_delay_sz);
> +
> if (!(cpu_class_is_omap2()))
> omap1_sram_init();
> else if (cpu_is_omap242x())
Kevin
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
