From: Roger Quadros <rogerq@ti.com> To: Robert ABEL <rabel@cit-ec.uni-bielefeld.de>, <khilman@deeprootsystems.com>, <tony@atomide.com>, <linux@arm.linux.org.uk>, <linux-omap@vger.kernel.org> Cc: <linux-kernel@vger.kernel.org> Subject: Re: [PATCH 3/4] ARM OMAP2+ GPMC: fix WAITMONITORINGTIME divider bug Date: Tue, 17 Feb 2015 11:27:14 +0200 [thread overview] Message-ID: <54E30972.4020107@ti.com> (raw) In-Reply-To: <1424101741-24152-4-git-send-email-rabel@cit-ec.uni-bielefeld.de> 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@cit-ec.uni-bielefeld.de> > --- > 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
WARNING: multiple messages have this Message-ID (diff)
From: Roger Quadros <rogerq@ti.com> To: Robert ABEL <rabel@cit-ec.uni-bielefeld.de>, khilman@deeprootsystems.com, tony@atomide.com, linux@arm.linux.org.uk, linux-omap@vger.kernel.org Cc: linux-kernel@vger.kernel.org Subject: Re: [PATCH 3/4] ARM OMAP2+ GPMC: fix WAITMONITORINGTIME divider bug Date: Tue, 17 Feb 2015 11:27:14 +0200 [thread overview] Message-ID: <54E30972.4020107@ti.com> (raw) In-Reply-To: <1424101741-24152-4-git-send-email-rabel@cit-ec.uni-bielefeld.de> 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@cit-ec.uni-bielefeld.de> > --- > 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
next prev parent reply other threads:[~2015-02-17 9:27 UTC|newest] Thread overview: 29+ messages / expand[flat|nested] mbox.gz Atom feed top 2015-02-16 15:48 [PATCH 0/4] ARM OMAP2+ GPMC: various fixes and bus children Robert ABEL 2015-02-16 15:48 ` [PATCH 1/4] ARM OMAP2+ GPMC: fix debug output alignment Robert ABEL 2015-02-16 15:48 ` [PATCH 2/4] ARM OMAP2+ GPMC: always program GPMCFCLKDIVIDER Robert ABEL 2015-02-16 15:49 ` [PATCH 3/4] ARM OMAP2+ GPMC: fix WAITMONITORINGTIME divider bug Robert ABEL 2015-02-16 15:49 ` [PATCH 4/4] ARM OMAP2+ GPMC: add bus children Robert ABEL 2015-02-17 9:41 ` Roger Quadros 2015-02-17 9:41 ` Roger Quadros 2015-02-17 13:57 ` Robert Abel 2015-02-17 14:15 ` Roger Quadros 2015-02-17 14:15 ` Roger Quadros 2015-02-17 9:27 ` Roger Quadros [this message] 2015-02-17 9:27 ` [PATCH 3/4] ARM OMAP2+ GPMC: fix WAITMONITORINGTIME divider bug Roger Quadros 2015-02-17 13:48 ` Robert Abel 2015-02-17 13:56 ` Roger Quadros 2015-02-17 13:56 ` Roger Quadros 2015-02-16 17:10 ` [PATCH 2/4] ARM OMAP2+ GPMC: always program GPMCFCLKDIVIDER Tony Lindgren 2015-02-16 20:09 ` Robert Abel 2015-02-17 8:12 ` Roger Quadros 2015-02-17 8:12 ` Roger Quadros 2015-02-17 13:47 ` Robert Abel [not found] ` <CAMdRc4F9B0ft-ExgQ1vHqwXMiONwWKn3FPCRDyHsjgGe1Dn_1w@mail.gmail.com> 2015-02-17 13:52 ` Roger Quadros 2015-02-17 13:52 ` Roger Quadros 2015-02-17 14:06 ` Robert Abel 2015-02-17 14:25 ` Roger Quadros 2015-02-17 14:25 ` Roger Quadros 2015-02-23 21:38 ` Robert Abel 2015-02-23 22:03 ` Tony Lindgren 2015-02-24 11:53 ` Roger Quadros 2015-02-24 11:53 ` Roger Quadros
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=54E30972.4020107@ti.com \ --to=rogerq@ti.com \ --cc=khilman@deeprootsystems.com \ --cc=linux-kernel@vger.kernel.org \ --cc=linux-omap@vger.kernel.org \ --cc=linux@arm.linux.org.uk \ --cc=rabel@cit-ec.uni-bielefeld.de \ --cc=tony@atomide.com \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.