Re: [PATCH 8/8 v2] ARM OMAP2+ GPMC: fix WAITMONITORINGTIME divider bug
On 25/02/15 19:22, Robert Abel wrote: > Hi Roger, > > On 25 Feb 2015 18:17, Roger Quadros wrote: >> How will the user know by looking at the kernel log that it was really an >> error? >> We don't fail probe if set_gpmc_timing_reg() fails so I feel it is necessary >> to >> clearly show an Error message. >> >> You can probably reword it like "%s: Error!! GPMC CS %d..." > > I'll put "error" in there. But just for the record, it's this messaged > followed by a WARN that explains "failed to add child %s". > So I'd expect the user to put A and B together ;) Sorry, my bad. We are in fact returning -1 in GPMC_SET_ONE(). So no need to add the "Error" string. cheers, -roger -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 8/8 v2] ARM OMAP2+ GPMC: fix WAITMONITORINGTIME divider bug
Hi Roger, On 25 Feb 2015 18:17, Roger Quadros wrote: How will the user know by looking at the kernel log that it was really an error? We don't fail probe if set_gpmc_timing_reg() fails so I feel it is necessary to clearly show an Error message. You can probably reword it like "%s: Error!! GPMC CS %d..." I'll put "error" in there. But just for the record, it's this messaged followed by a WARN that explains "failed to add child %s". So I'd expect the user to put A and B together ;) Robert -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 8/8 v2] ARM OMAP2+ GPMC: fix WAITMONITORINGTIME divider bug
Robert,
On 25/02/15 19:07, Robert Abel wrote:
> Hi Roger,
>
> On 25 Feb 2015 17:58, Roger Quadros wrote:
>>> static unsigned int gpmc_ticks_to_ps(unsigned int ticks)
>>> @@ -346,16 +395,22 @@ static void gpmc_cs_bool_timings(int cs, const struct
>>> gpmc_bool_timings *p)
>>>* @st_bit Start Bit
>>>* @end_bit End Bit. Must be >= @st_bit.
>>>* @nameDTS node name, w/o "gpmc,"
>>> + * @cd Clock Domain of timing parameter.
>>> + * @shift Parameter value left shifts @shift, which is then printed
>>> instead of value.
>>>* @raw Raw Format Option.
>>>* raw format: gpmc,name =
>>>* tick format: gpmc,name = /* x ticks */
>>>* @noval Parameter values equal to 0 are not printed.
>>> - * @shift Parameter value left shifts @shift, which is then printed
>>> instead of value.
>>>*
>>>*/
>>> -static int get_gpmc_timing_reg(int cs, int reg, int st_bit, int end_bit,
>>> - bool raw, bool noval, int shift,
>>> - const char *name)
>>> +static int get_gpmc_timing_reg(
>>> +/* timing specifiers */
>>> +int cs, int reg, int st_bit, int end_bit,
>>> +const char *name, const enum gpmc_clk_domain cd,
>>> +/* value transform */
>>> +int shift,
>>> +/* format specifiers */
>>> +bool raw, bool noval)
>> now that you are rearranging the parameters, "name" parameter should
>> probably be
>> at the same position (or last) in get_gpmc_timing_reg() and
>> set_gpmc_timing_reg()?
>> Also clock domain (cd) position could be matched if possible.
> I rearranged them primarily, because I wanted to group the specifiers
> according to function, because I found it unnatural to add clock domain to
> the end, when it's "more important" than the format specifiers.
> set_gpmc_timing_reg are fine in that regard as it doesn't have format
> specifiers.
OK.
>>> +/**
>>> + * set_gpmc_timing_reg - set a single timing parameter for Chip Select
>>> Region.
>>> + * @cs Chip Select Region.
>>> + * @reg GPMC_CS_CONFIGn register offset.
>>> + * @st_bit Start Bit
>>> + * @end_bit End Bit. Must be >= @st_bit.
>>> + * @timeTiming parameter in ns.
>>> + * @cd Timing parameter clock domain.
>>> + * @nameTiming parameter name.
>>> + * @noteCaller is expected to have initialized CONFIG1 GPMCFCLKDIVIDER
>> @note is not a parameter.
> Well no, note's a note. This is a doxygen-style comment, so tools should put
> a note in the created documentation. Doxygen will put a box with yellow
> background, for instance.
Oh ok.
>>> -pr_err("%s: GPMC error! CS%d: %s: %d ns, %d ticks > %d\n",
>>> +pr_err("%s: GPMC CS%d: %s %d ns, %d ticks > %d ticks\n",
>> any reason for removing the "error!" string?
> It's already pr_err, the "error!" in-between "GPMC CS%d" made it hard to read
> and there's a WARN after that statement in all cases, because a child _must_
> fail if a timing parameter constraint is broken.
How will the user know by looking at the kernel log that it was really an error?
We don't fail probe if set_gpmc_timing_reg() fails so I feel it is necessary to
clearly show an Error message.
You can probably reword it like "%s: Error!! GPMC CS %d..."
cheers,
-roger
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 8/8 v2] ARM OMAP2+ GPMC: fix WAITMONITORINGTIME divider bug
Hi Roger,
On 25 Feb 2015 17:58, Roger Quadros wrote:
static unsigned int gpmc_ticks_to_ps(unsigned int ticks)
@@ -346,16 +395,22 @@ static void gpmc_cs_bool_timings(int cs, const struct
gpmc_bool_timings *p)
* @st_bit Start Bit
* @end_bit End Bit. Must be >= @st_bit.
* @nameDTS node name, w/o "gpmc,"
+ * @cd Clock Domain of timing parameter.
+ * @shift Parameter value left shifts @shift, which is then printed instead
of value.
* @raw Raw Format Option.
* raw format: gpmc,name =
* tick format: gpmc,name = /* x ticks */
* @noval Parameter values equal to 0 are not printed.
- * @shift Parameter value left shifts @shift, which is then printed instead
of value.
*
*/
-static int get_gpmc_timing_reg(int cs, int reg, int st_bit, int end_bit,
- bool raw, bool noval, int shift,
- const char *name)
+static int get_gpmc_timing_reg(
+ /* timing specifiers */
+ int cs, int reg, int st_bit, int end_bit,
+ const char *name, const enum gpmc_clk_domain cd,
+ /* value transform */
+ int shift,
+ /* format specifiers */
+ bool raw, bool noval)
now that you are rearranging the parameters, "name" parameter should probably be
at the same position (or last) in get_gpmc_timing_reg() and
set_gpmc_timing_reg()?
Also clock domain (cd) position could be matched if possible.
I rearranged them primarily, because I wanted to group the specifiers
according to function, because I found it unnatural to add clock domain
to the end, when it's "more important" than the format specifiers.
set_gpmc_timing_reg are fine in that regard as it doesn't have format
specifiers.
+/**
+ * set_gpmc_timing_reg - set a single timing parameter for Chip Select Region.
+ * @cs Chip Select Region.
+ * @reg GPMC_CS_CONFIGn register offset.
+ * @st_bit Start Bit
+ * @end_bit End Bit. Must be >= @st_bit.
+ * @timeTiming parameter in ns.
+ * @cd Timing parameter clock domain.
+ * @nameTiming parameter name.
+ * @noteCaller is expected to have initialized CONFIG1 GPMCFCLKDIVIDER
@note is not a parameter.
Well no, note's a note. This is a doxygen-style comment, so tools should
put a note in the created documentation. Doxygen will put a box with
yellow background, for instance.
- pr_err("%s: GPMC error! CS%d: %s: %d ns, %d ticks > %d\n",
+ pr_err("%s: GPMC CS%d: %s %d ns, %d ticks > %d ticks\n",
any reason for removing the "error!" string?
It's already pr_err, the "error!" in-between "GPMC CS%d" made it hard to
read and there's a WARN after that statement in all cases, because a
child _must_ fail if a timing parameter constraint is broken.
Regards,
Robert
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 8/8 v2] ARM OMAP2+ GPMC: fix WAITMONITORINGTIME divider bug
Robert,
On 24/02/15 22:05, Robert ABEL wrote:
> The WAITMONITORINGTIME is expressed as a number of GPMC_CLK clock cycles,
> even though the access is defined as asynchronous, and no GPMC_CLK clock
> is provided to the external device. Still, GPMCFCLKDIVIDER is used as a
> divider
> for the GPMC clock, so it must be programmed to define the
> correct WAITMONITORINGTIME delay.
>
> This patch correctly computes WAITMONITORINGTIME in GPMC_CLK cycles instead
> of GPMC_FCLK cycles,
> both during programming (gpmc_cs_set_timings) and during retrieval
> (gpmc_cs_show_timings).
>
> Signed-off-by: Robert ABEL
> ---
> drivers/memory/omap-gpmc.c | 125
> +++--
> 1 file changed, 99 insertions(+), 26 deletions(-)
>
> diff --git a/drivers/memory/omap-gpmc.c b/drivers/memory/omap-gpmc.c
> index 9cf8d21..6591991 100644
> --- a/drivers/memory/omap-gpmc.c
> +++ b/drivers/memory/omap-gpmc.c
> @@ -170,6 +170,11 @@
> */
> #define GPMC_NR_IRQ 2
>
> +enum gpmc_clk_domain {
> + GPMC_CD_FCLK,
> + GPMC_CD_CLK
> +};
> +
> struct gpmc_cs_data {
> const char *name;
>
> @@ -268,16 +273,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 clock domain in ps
> + * @cs Chip Select Region.
> + * @cd Clock Domain.
> + *
> + * GPMC_CS_CONFIG1 GPMCFCLKDIVIDER for cs has to be setup
> + * prior to calling this function with GPMC_CD_CLK.
> + *
> + */
> +static unsigned long gpmc_get_clk_period(int cs, enum gpmc_clk_domain cd)
> +{
> +
> + unsigned long tick_ps = gpmc_get_fclk_period();
> + u32 l;
> + int div;
> +
> + switch (cd) {
> + case GPMC_CD_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_CD_FCLK:
> + /* FALL-THROUGH */
> + default:
> + break;
> + }
> +
> + return tick_ps;
> +
> +}
> +
> +static unsigned int gpmc_ns_to_clk_ticks(unsigned int time_ns, int cs, enum
> gpmc_clk_domain cd)
> {
> 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, cd);
>
> 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, /* any CS */ 0, GPMC_CD_FCLK);
> +}
> +
> static unsigned int gpmc_ps_to_ticks(unsigned int time_ps)
> {
> unsigned long tick_ps;
> @@ -288,9 +332,14 @@ static unsigned int gpmc_ps_to_ticks(unsigned int
> time_ps)
> return (time_ps + tick_ps - 1) / tick_ps;
> }
>
> +unsigned int gpmc_clk_ticks_to_ns(unsigned ticks, int cs, enum
> gpmc_clk_domain cd)
> +{
> + return ticks * gpmc_get_clk_period(cs, cd) / 1000;
> +}
> +
> unsigned int gpmc_ticks_to_ns(unsigned int ticks)
> {
> - return ticks * gpmc_get_fclk_period() / 1000;
> + return gpmc_clk_ticks_to_ns(ticks, /* any CS */ 0, GPMC_CD_FCLK);
> }
>
> static unsigned int gpmc_ticks_to_ps(unsigned int ticks)
> @@ -346,16 +395,22 @@ static void gpmc_cs_bool_timings(int cs, const struct
> gpmc_bool_timings *p)
> * @st_bit Start Bit
> * @end_bit End Bit. Must be >= @st_bit.
> * @nameDTS node name, w/o "gpmc,"
> + * @cd Clock Domain of timing parameter.
> + * @shift Parameter value left shifts @shift, which is then printed
> instead of value.
> * @raw Raw Format Option.
> * raw format: gpmc,name =
> * tick format: gpmc,name = /* x ticks */
> * @noval Parameter values equal to 0 are not printed.
> - * @shift Parameter value left shifts @shift, which is then printed
> instead of value.
> *
> */
> -static int get_gpmc_timing_reg(int cs, int reg, int st_bit, int end_bit,
> -bool raw, bool noval, int shift,
> -const char *name)
> +static int get_gpmc_timing_reg(
> + /* timing specifiers */
> + int cs, int reg, int st_bit, int end_bit,
> + const char *name, const enum gpmc_clk_domain cd,
> + /* value transform */
> + int shift,
> + /* format specifiers */
> + bool raw, bool noval)
now that you are rearranging the parameters, "name" parameter should probably be
at the same position (or last) in get_gpmc_timing_reg() and
set_gpmc_timing_reg()?
Also clock domain (cd) position could be matched if possible.
> {
> u32 l;
> int nr_bits;
> @@ -373,7 +428,7 @@ static int get_gpmc_timing_reg(int cs, int reg, int
> st_bit, int end_bit,
> /* DTS tick format for timings in ns */
> unsigned int time_ns;
>
> - time_ns = gpmc_tic
[PATCH 8/8 v2] ARM OMAP2+ GPMC: fix WAITMONITORINGTIME divider bug
The WAITMONITORINGTIME is expressed as a number of GPMC_CLK clock cycles,
even though the access is defined as asynchronous, and no GPMC_CLK clock
is provided to the external device. Still, GPMCFCLKDIVIDER is used as a divider
for the GPMC clock, so it must be programmed to define the
correct WAITMONITORINGTIME delay.
This patch correctly computes WAITMONITORINGTIME in GPMC_CLK cycles instead of
GPMC_FCLK cycles,
both during programming (gpmc_cs_set_timings) and during retrieval
(gpmc_cs_show_timings).
Signed-off-by: Robert ABEL
---
drivers/memory/omap-gpmc.c | 125 +++--
1 file changed, 99 insertions(+), 26 deletions(-)
diff --git a/drivers/memory/omap-gpmc.c b/drivers/memory/omap-gpmc.c
index 9cf8d21..6591991 100644
--- a/drivers/memory/omap-gpmc.c
+++ b/drivers/memory/omap-gpmc.c
@@ -170,6 +170,11 @@
*/
#defineGPMC_NR_IRQ 2
+enum gpmc_clk_domain {
+ GPMC_CD_FCLK,
+ GPMC_CD_CLK
+};
+
struct gpmc_cs_data {
const char *name;
@@ -268,16 +273,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 clock domain in ps
+ * @cs Chip Select Region.
+ * @cd Clock Domain.
+ *
+ * GPMC_CS_CONFIG1 GPMCFCLKDIVIDER for cs has to be setup
+ * prior to calling this function with GPMC_CD_CLK.
+ *
+ */
+static unsigned long gpmc_get_clk_period(int cs, enum gpmc_clk_domain cd)
+{
+
+ unsigned long tick_ps = gpmc_get_fclk_period();
+ u32 l;
+ int div;
+
+ switch (cd) {
+ case GPMC_CD_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_CD_FCLK:
+ /* FALL-THROUGH */
+ default:
+ break;
+ }
+
+ return tick_ps;
+
+}
+
+static unsigned int gpmc_ns_to_clk_ticks(unsigned int time_ns, int cs, enum
gpmc_clk_domain cd)
{
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, cd);
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, /* any CS */ 0, GPMC_CD_FCLK);
+}
+
static unsigned int gpmc_ps_to_ticks(unsigned int time_ps)
{
unsigned long tick_ps;
@@ -288,9 +332,14 @@ static unsigned int gpmc_ps_to_ticks(unsigned int time_ps)
return (time_ps + tick_ps - 1) / tick_ps;
}
+unsigned int gpmc_clk_ticks_to_ns(unsigned ticks, int cs, enum gpmc_clk_domain
cd)
+{
+ return ticks * gpmc_get_clk_period(cs, cd) / 1000;
+}
+
unsigned int gpmc_ticks_to_ns(unsigned int ticks)
{
- return ticks * gpmc_get_fclk_period() / 1000;
+ return gpmc_clk_ticks_to_ns(ticks, /* any CS */ 0, GPMC_CD_FCLK);
}
static unsigned int gpmc_ticks_to_ps(unsigned int ticks)
@@ -346,16 +395,22 @@ static void gpmc_cs_bool_timings(int cs, const struct
gpmc_bool_timings *p)
* @st_bit Start Bit
* @end_bit End Bit. Must be >= @st_bit.
* @nameDTS node name, w/o "gpmc,"
+ * @cd Clock Domain of timing parameter.
+ * @shift Parameter value left shifts @shift, which is then printed instead
of value.
* @raw Raw Format Option.
* raw format: gpmc,name =
* tick format: gpmc,name = /* x ticks */
* @noval Parameter values equal to 0 are not printed.
- * @shift Parameter value left shifts @shift, which is then printed instead
of value.
*
*/
-static int get_gpmc_timing_reg(int cs, int reg, int st_bit, int end_bit,
- bool raw, bool noval, int shift,
- const char *name)
+static int get_gpmc_timing_reg(
+ /* timing specifiers */
+ int cs, int reg, int st_bit, int end_bit,
+ const char *name, const enum gpmc_clk_domain cd,
+ /* value transform */
+ int shift,
+ /* format specifiers */
+ bool raw, bool noval)
{
u32 l;
int nr_bits;
@@ -373,7 +428,7 @@ static int get_gpmc_timing_reg(int cs, int reg, int st_bit,
int end_bit,
/* DTS tick format for timings in ns */
unsigned int time_ns;
- time_ns = gpmc_ticks_to_ns(l);
+ time_ns = gpmc_clk_ticks_to_ns(l, cs, cd);
pr_info("gpmc,%s = <%u> /* %i ticks */\n",
name, time_ns, l);
} else {
@@ -388,13 +443,15 @@ static int get_gpmc_timing_reg(int cs, int reg, int
st_bit, int end_bit,
pr_info("cs%i %s: 0x%08x\n", cs, #config, \
gpmc_cs_read_reg(cs, config))
#define GPMC_GET_RAW(reg, st, end, field) \
- get_gpmc_timing_reg(cs, (reg), (st), (end
