Re: [PATCH 3/4] ARM OMAP2+ GPMC: fix WAITMONITORINGTIME divider bug
From: Roger Quadros
Date: Tue Feb 17 2015 - 04:27:36 EST
Robert,
On 16/02/15 17:49, Robert ABEL wrote:
> WAITMONITORINGTIME is expressed in GPMC_CLK cycles (even for asynchronous accesses).
> However the driver currently converts them to GPMC_FCLK cycles, thus waitmonitoringtime in dt
> had to be predivided by divider as a workaround. This patch fixes the issue by reading the
Can you please point out which DT had used this pre-divided workaround? We will have to
fix those then.
> current GPMCFCLKDIVIDER and adjusting the divisor for WAITMONITORINGTIME accordingly.
>
> Signed-off-by: Robert ABEL <rabel@xxxxxxxxxxxxxxxxxxxxxxx>
> ---
> arch/arm/mach-omap2/gpmc.c | 78 +++++++++++++++++++++++++++++++++++++---------
> 1 file changed, 64 insertions(+), 14 deletions(-)
>
> diff --git a/arch/arm/mach-omap2/gpmc.c b/arch/arm/mach-omap2/gpmc.c
Can you please rebase your patches on top of v3.19?
gpmc.c has been moved to drivers/memory/omap-gpmc.c
> index bae4a20..4fa5ff1 100644
> --- a/arch/arm/mach-omap2/gpmc.c
> +++ b/arch/arm/mach-omap2/gpmc.c
> @@ -113,6 +113,9 @@
> */
> #define GPMC_NR_IRQ 2
>
> +#define GPMC_FCLK 0
> +#define GPMC_CLK 1
> +
enum gpmc_clksel {
GPMC_CLKSEL_FCLK,
GPMC_CLKSEL_CLK,
};
So all the below occurrences of "unsigned int clk_sel" become "enum gpmc_clksel".
> struct gpmc_client_irq {
> unsigned irq;
> u32 bitmask;
> @@ -208,16 +211,55 @@ static unsigned long gpmc_get_fclk_period(void)
> return rate;
> }
>
> -static unsigned int gpmc_ns_to_ticks(unsigned int time_ns)
> +/**
> + * gpmc_get_clk_period - get period of selected clk in ps
> + * @cs : affected chip select
> + * @clk_sel: either one of GPMC_CLK or GPMC_FCLK
> + *
> + * GPMC_CS_CONFIG1 GPMCFCLKDIVIDER for cs has to be setup
> + * prior to calling this function with GPMC_CLK.
> + *
> + */
> +static unsigned long gpmc_get_clk_period(int cs, unsigned int clk_sel)
> +{
> +
> + unsigned long tick_ps = gpmc_get_fclk_period();
> + u32 l;
> + int div;
> +
> + switch (clk_sel) {
> + case GPMC_CLK:
> + /* get current clk divider */
> + l = gpmc_cs_read_reg(cs, GPMC_CS_CONFIG1);
> + div = (l & 0x03) + 1;
> + /* get GPMC_CLK period */
> + tick_ps *= div;
> + break;
> + case GPMC_FCLK:
> + /* FALL-THROUGH */
> + default:
> + break;
> + }
> +
> + return tick_ps;
> +
> +}
> +
> +static unsigned int gpmc_ns_to_clk_ticks(unsigned int time_ns, int cs, unsigned int clk_sel)
> {
> unsigned long tick_ps;
>
> /* Calculate in picosecs to yield more exact results */
> - tick_ps = gpmc_get_fclk_period();
> + tick_ps = gpmc_get_clk_period(cs, clk_sel);
>
> return (time_ns * 1000 + tick_ps - 1) / tick_ps;
> }
>
> +static unsigned int gpmc_ns_to_ticks(unsigned int time_ns)
> +{
> + return gpmc_ns_to_clk_ticks(time_ns, 0, GPMC_FCLK);
This function should have been unchanged since we're dealing with GPMC_FCLK
and it was using gpmc_get_fclk_period().
> +}
> +
> static unsigned int gpmc_ps_to_ticks(unsigned int time_ps)
> {
> unsigned long tick_ps;
> @@ -280,10 +322,10 @@ static void gpmc_cs_bool_timings(int cs, const struct gpmc_bool_timings *p)
>
> #ifdef DEBUG
> static int set_gpmc_timing_reg(int cs, int reg, int st_bit, int end_bit,
> - int time, const char *name)
> + int time, unsigned int clk_sel, const char *name)
> #else
> static int set_gpmc_timing_reg(int cs, int reg, int st_bit, int end_bit,
> - int time)
> + int time, unsigned int clk_sel)
> #endif
> {
> u32 l;
> @@ -292,7 +334,7 @@ static int set_gpmc_timing_reg(int cs, int reg, int st_bit, int end_bit,
> if (time == 0)
> ticks = 0;
> else
> - ticks = gpmc_ns_to_ticks(time);
> + ticks = gpmc_ns_to_clk_ticks(time, cs, clk_sel);
> nr_bits = end_bit - st_bit + 1;
> if (ticks >= 1 << nr_bits) {
> #ifdef DEBUG
> @@ -307,7 +349,7 @@ static int set_gpmc_timing_reg(int cs, int reg, int st_bit, int end_bit,
> #ifdef DEBUG
> printk(KERN_INFO
> "GPMC CS%d: %-17s: %3d ticks, %3lu ns (was %3i ticks) %3d ns\n",
> - cs, name, ticks, gpmc_get_fclk_period() * ticks / 1000,
> + cs, name, ticks, gpmc_get_clk_period(cs, clk_sel) * ticks / 1000,
> (l >> st_bit) & mask, time);
> #endif
> l &= ~(mask << st_bit);
> @@ -320,12 +362,19 @@ static int set_gpmc_timing_reg(int cs, int reg, int st_bit, int end_bit,
> #ifdef DEBUG
> #define GPMC_SET_ONE(reg, st, end, field) \
> if (set_gpmc_timing_reg(cs, (reg), (st), (end), \
> - t->field, #field) < 0) \
> + t->field, GPMC_FCLK, #field) < 0) \
> return -1
> +#define GPMC_SET_ONE_C(reg, st, end, field, clk_sel) \
Let's call this GPMC_SET_ONE_CLKSEL()
> + if (set_gpmc_timing_reg(cs, (reg), (st), (end), \
> + t->field, clk_sel, #field) < 0) \
> + return -1
> #else
> #define GPMC_SET_ONE(reg, st, end, field) \
> - if (set_gpmc_timing_reg(cs, (reg), (st), (end), t->field) < 0) \
> + if (set_gpmc_timing_reg(cs, (reg), (st), (end), t->field, GPMC_FCLK) < 0) \
> return -1
> +#define GPMC_SET_ONE_C(reg, st, end, field, clk_sel) \
> + if (set_gpmc_timing_reg(cs, (reg), (st), (end), t->field, clk_sel) < 0) \
> + return -1
> #endif
>
> int gpmc_calc_divider(unsigned int sync_clk)
> @@ -374,22 +423,23 @@ int gpmc_cs_set_timings(int cs, const struct gpmc_timings *t)
> GPMC_SET_ONE(GPMC_CS_CONFIG6, 0, 3, bus_turnaround);
> GPMC_SET_ONE(GPMC_CS_CONFIG6, 8, 11, cycle2cycle_delay);
>
> - GPMC_SET_ONE(GPMC_CS_CONFIG1, 18, 19, wait_monitoring);
> - GPMC_SET_ONE(GPMC_CS_CONFIG1, 25, 26, clk_activation);
> -
> if (gpmc_capability & GPMC_HAS_WR_DATA_MUX_BUS)
> GPMC_SET_ONE(GPMC_CS_CONFIG6, 16, 19, wr_data_mux_bus);
> if (gpmc_capability & GPMC_HAS_WR_ACCESS)
> GPMC_SET_ONE(GPMC_CS_CONFIG6, 24, 28, wr_access);
>
> l = gpmc_cs_read_reg(cs, GPMC_CS_CONFIG1);
> + l &= ~0x03;
> + l |= (div - 1);
> + gpmc_cs_write_reg(cs, GPMC_CS_CONFIG1, l);
> +
> + GPMC_SET_ONE_C(GPMC_CS_CONFIG1, 18, 19, wait_monitoring, GPMC_CLK);
> + GPMC_SET_ONE(GPMC_CS_CONFIG1, 25, 26, clk_activation);
> +
> #ifdef DEBUG
> printk(KERN_INFO "GPMC CS%d CLK period is %lu ns (div %d)\n",
> cs, (div * gpmc_get_fclk_period()) / 1000, div);
> #endif
> - l &= ~0x03;
> - l |= (div - 1);
> - gpmc_cs_write_reg(cs, GPMC_CS_CONFIG1, l);
>
> gpmc_cs_bool_timings(cs, &t->bool_timings);
>
>
You will also need to correct gpmc_cs_show_timings() to show the
correct timing for "wait-monitoring-ns" based on GPMC_CLK.
cheers,
-roger
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/