Re: [PATCH 9/8 v2] ARM OMAP2+ GPMC: fix programming/showing reserved timing parameters
Robert, On 25/02/15 17:17, Robert Abel wrote: > Hi Roger, > > On 25 Feb 2015 11:44, Roger Quadros wrote: >> typo ATTCHEDDEVICEPAGELENGTH->ATTACHEDDEVICEPAGELENGTH > Yep. >>> +/** DEVICESIZE Max Value */ >>> +#define GPMC_CONFIG1_DEVICESIZE_MAX GPMC_CONFIG1_DEVICESIZE_16 >> Shouldn't this be 1 instead? I'm hoping max value is without the shift >> based on GPMC_CONFIG1_CLKACTIVATIONTIME_MAX. > Yes, it should. It's a remnant from the change below... >>> + * @max Maximum parameter value (after optional @shift). >> max should be absolute value, without the shift. > Yes, forgot to change that. >>> +#define GPMC_GET_RAW_SHIFT(reg, st, end, shift, max, field) \ >>> +get_gpmc_timing_reg(cs, (reg), (st), (end), (max), field, >>> GPMC_CD_FCLK, (shift), 1, 1) >> Is it better to rename this to GPMC_GET_RAW_SHIFT_MAX() as it takes the max >> parameter. > Ok. >>> +GPMC_GET_RAW_SHIFT(GPMC_CS_CONFIG1, 23, 24, 4, 2, "burst-length"); >> want to define GPMC_CONFIG1_BURSTLENGTH_MAX? > Yes, for consistency. Originally, i was not going to create defines for > limits used only in one place, as opposed to WAITMONITORINGTIME and > CLKACTIVATIONTIME. >>> -GPMC_GET_TICKS_CD(GPMC_CS_CONFIG1, 18, 19, "wait-monitoring-ns", >>> GPMC_CD_CLK); >>> -GPMC_GET_TICKS(GPMC_CS_CONFIG1, 25, 26, "clk-activation-ns"); >>> +GPMC_GET_TICKS_CD_MAX(GPMC_CS_CONFIG1, 18, 19, >>> GPMC_CONFIG1_WAIT_MON_TIME_MAX, "wait-monitoring-ns", GPMC_CD_CLK); >> use GPMC_CONFIG1_WAITMONTIME_MAX to have consistent naming. > I used the naming the other define had. Saw no point in changing them. They > go mostly unused anyway. > Might as well change it. Not much of an issue. I leave it upto you. cheers, -roger > >>> +#define GPMC_SET_ONE_CD(reg, st, end, field, cd) \ >>> +GPMC_SET_ONE_CD_MAX(reg, st, end, 0, field, GPMC_CD_FCLK) >> last parameter should be (cd) instead of GPMC_CD_FCLK. > Ah, copy-paste, there you are again. Luckily this define goes unused, because > all GPMC_SET_ONE_CD became GPMC_SET_ONE_CD_MAX anyway. > So I'll delete that. Quite a keen eye, though ;) > > 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 9/8 v2] ARM OMAP2+ GPMC: fix programming/showing reserved timing parameters
Hi Roger, On 25 Feb 2015 11:44, Roger Quadros wrote: typo ATTCHEDDEVICEPAGELENGTH->ATTACHEDDEVICEPAGELENGTH Yep. +/** DEVICESIZE Max Value */ +#define GPMC_CONFIG1_DEVICESIZE_MAX GPMC_CONFIG1_DEVICESIZE_16 Shouldn't this be 1 instead? I'm hoping max value is without the shift based on GPMC_CONFIG1_CLKACTIVATIONTIME_MAX. Yes, it should. It's a remnant from the change below... + * @max Maximum parameter value (after optional @shift). max should be absolute value, without the shift. Yes, forgot to change that. +#define GPMC_GET_RAW_SHIFT(reg, st, end, shift, max, field) \ + get_gpmc_timing_reg(cs, (reg), (st), (end), (max), field, GPMC_CD_FCLK, (shift), 1, 1) Is it better to rename this to GPMC_GET_RAW_SHIFT_MAX() as it takes the max parameter. Ok. + GPMC_GET_RAW_SHIFT(GPMC_CS_CONFIG1, 23, 24, 4, 2, "burst-length"); want to define GPMC_CONFIG1_BURSTLENGTH_MAX? Yes, for consistency. Originally, i was not going to create defines for limits used only in one place, as opposed to WAITMONITORINGTIME and CLKACTIVATIONTIME. - GPMC_GET_TICKS_CD(GPMC_CS_CONFIG1, 18, 19, "wait-monitoring-ns", GPMC_CD_CLK); - GPMC_GET_TICKS(GPMC_CS_CONFIG1, 25, 26, "clk-activation-ns"); + GPMC_GET_TICKS_CD_MAX(GPMC_CS_CONFIG1, 18, 19, GPMC_CONFIG1_WAIT_MON_TIME_MAX, "wait-monitoring-ns", GPMC_CD_CLK); use GPMC_CONFIG1_WAITMONTIME_MAX to have consistent naming. I used the naming the other define had. Saw no point in changing them. They go mostly unused anyway. Might as well change it. +#define GPMC_SET_ONE_CD(reg, st, end, field, cd) \ + GPMC_SET_ONE_CD_MAX(reg, st, end, 0, field, GPMC_CD_FCLK) last parameter should be (cd) instead of GPMC_CD_FCLK. Ah, copy-paste, there you are again. Luckily this define goes unused, because all GPMC_SET_ONE_CD became GPMC_SET_ONE_CD_MAX anyway. So I'll delete that. Quite a keen eye, though ;) 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 9/8 v2] ARM OMAP2+ GPMC: fix programming/showing reserved timing parameters
Robert,
On 24/02/15 22:05, Robert ABEL wrote:
> GPMC_CONFIG1_i parameters CLKACTIVATIONTIME and WAITMONITORINGTIME
> have reserved values.
> Raise an error if calculated timings try to program reserved values.
>
> GPMC_CONFIG1_i ATTCHEDDEVICEPAGELENGTH and DEVICESIZE were already checked
typo ATTCHEDDEVICEPAGELENGTH->ATTACHEDDEVICEPAGELENGTH
> when parsing the DT.
>
> Explicitly comment invalid values on gpmc_cs_show_timings for
> -CLKACTIVATIONTIME
> -WAITMONITORINGTIME
> -DEVICESIZE
> -ATTACHEDDEVICEPAGELENGTH
>
> Signed-off-by: Robert ABEL
> ---
> drivers/memory/omap-gpmc.c | 69
> ++
> 1 file changed, 46 insertions(+), 23 deletions(-)
>
> diff --git a/drivers/memory/omap-gpmc.c b/drivers/memory/omap-gpmc.c
> index 6591991..cc2e6d0 100644
> --- a/drivers/memory/omap-gpmc.c
> +++ b/drivers/memory/omap-gpmc.c
> @@ -135,6 +135,8 @@
> #define GPMC_CONFIG1_WRITETYPE_ASYNC(0 << 27)
> #define GPMC_CONFIG1_WRITETYPE_SYNC (1 << 27)
> #define GPMC_CONFIG1_CLKACTIVATIONTIME(val) ((val & 3) << 25)
> +/** CLKACTIVATIONTIME Max Ticks */
> +#define GPMC_CONFIG1_CLKACTIVATIONTIME_MAX 2
> #define GPMC_CONFIG1_PAGE_LEN(val) ((val & 3) << 23)
> #define GPMC_CONFIG1_WAIT_READ_MON (1 << 22)
> #define GPMC_CONFIG1_WAIT_WRITE_MON (1 << 21)
> @@ -144,6 +146,8 @@
> #define GPMC_CONFIG1_WAIT_PIN_SEL(val) ((val & 3) << 16)
> #define GPMC_CONFIG1_DEVICESIZE(val)((val & 3) << 12)
> #define GPMC_CONFIG1_DEVICESIZE_16 GPMC_CONFIG1_DEVICESIZE(1)
> +/** DEVICESIZE Max Value */
> +#define GPMC_CONFIG1_DEVICESIZE_MAX GPMC_CONFIG1_DEVICESIZE_16
Shouldn't this be 1 instead? I'm hoping max value is without the shift
based on GPMC_CONFIG1_CLKACTIVATIONTIME_MAX.
> #define GPMC_CONFIG1_DEVICETYPE(val)((val & 3) << 10)
> #define GPMC_CONFIG1_DEVICETYPE_NOR GPMC_CONFIG1_DEVICETYPE(0)
> #define GPMC_CONFIG1_MUXTYPE(val) ((val & 3) << 8)
> @@ -394,18 +398,21 @@ static void gpmc_cs_bool_timings(int cs, const struct
> gpmc_bool_timings *p)
> * @reg GPMC_CS_CONFIGn register offset.
> * @st_bit Start Bit
> * @end_bit End Bit. Must be >= @st_bit.
> + * @max Maximum parameter value (after optional @shift).
max should be absolute value, without the shift.
> + * If 0, maximum is as high as @st_bit and @end_bit allow.
> * @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 */
> + * When @max is exceeded, "invalid" is printed inside comment.
> * @noval Parameter values equal to 0 are not printed.
> *
> */
> static int get_gpmc_timing_reg(
> /* timing specifiers */
> - int cs, int reg, int st_bit, int end_bit,
> + int cs, int reg, int st_bit, int end_bit, int max,
> const char *name, const enum gpmc_clk_domain cd,
> /* value transform */
> int shift,
> @@ -415,11 +422,15 @@ static int get_gpmc_timing_reg(
> u32 l;
> int nr_bits;
> int mask;
> + bool invalid;
>
> l = gpmc_cs_read_reg(cs, reg);
> nr_bits = end_bit - st_bit + 1;
> mask = (1 << nr_bits) - 1;;
> l = (l >> st_bit) & mask;
> + if (!max)
> + max = mask;
> + invalid = l > max;
> if (shift)
> l = (shift << l);
> if (noval && (l == 0))
> @@ -429,11 +440,11 @@ static int get_gpmc_timing_reg(
> unsigned int time_ns;
>
> time_ns = gpmc_clk_ticks_to_ns(l, cs, cd);
> - pr_info("gpmc,%s = <%u> /* %i ticks */\n",
> - name, time_ns, l);
> + pr_info("gpmc,%s = <%u> /* %i ticks %s*/\n",
> + name, time_ns, l, invalid ? "; invalid " : "");
> } else {
> /* raw format */
> - pr_info("gpmc,%s = <%u>\n", name, l);
> + pr_info("gpmc,%s = <%u>%s\n", name, l, invalid ? " /* invalid
> */" : "");
> }
>
> return l;
> @@ -443,15 +454,19 @@ static int get_gpmc_timing_reg(
> 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), field, GPMC_CD_FCLK, 0, 1,
> 0)
> + get_gpmc_timing_reg(cs, (reg), (st), (end), 0, field, GPMC_CD_FCLK, 0,
> 1, 0)
> +#define GPMC_GET_RAW_MAX(reg, st, end, max, field) \
> + get_gpmc_timing_reg(cs, (reg), (st), (end), (max), field, GPMC_CD_FCLK,
> 0, 1, 0)
> #define GPMC_GET_RAW_BOOL(reg, st, end, field) \
> - get_gpmc_timing_reg(cs, (reg), (st), (end), field, GPMC_CD_FCLK, 0, 1,
> 1)
> -#define GPMC_GET_RAW_SHIFT(reg, st, end, shift, field) \
> - get_gpmc_timing_reg(cs, (reg), (st), (end), field, GPMC_CD_FCLK,
> (shift), 1, 1)
> +
Re: [PATCH 9/8 v2] ARM OMAP2+ GPMC: fix programming/showing reserved timing parameters
Robert, On 25/02/15 17:17, Robert Abel wrote: Hi Roger, On 25 Feb 2015 11:44, Roger Quadros wrote: typo ATTCHEDDEVICEPAGELENGTH-ATTACHEDDEVICEPAGELENGTH Yep. +/** DEVICESIZE Max Value */ +#define GPMC_CONFIG1_DEVICESIZE_MAX GPMC_CONFIG1_DEVICESIZE_16 Shouldn't this be 1 instead? I'm hoping max value is without the shift based on GPMC_CONFIG1_CLKACTIVATIONTIME_MAX. Yes, it should. It's a remnant from the change below... + * @max Maximum parameter value (after optional @shift). max should be absolute value, without the shift. Yes, forgot to change that. +#define GPMC_GET_RAW_SHIFT(reg, st, end, shift, max, field) \ +get_gpmc_timing_reg(cs, (reg), (st), (end), (max), field, GPMC_CD_FCLK, (shift), 1, 1) Is it better to rename this to GPMC_GET_RAW_SHIFT_MAX() as it takes the max parameter. Ok. +GPMC_GET_RAW_SHIFT(GPMC_CS_CONFIG1, 23, 24, 4, 2, burst-length); want to define GPMC_CONFIG1_BURSTLENGTH_MAX? Yes, for consistency. Originally, i was not going to create defines for limits used only in one place, as opposed to WAITMONITORINGTIME and CLKACTIVATIONTIME. -GPMC_GET_TICKS_CD(GPMC_CS_CONFIG1, 18, 19, wait-monitoring-ns, GPMC_CD_CLK); -GPMC_GET_TICKS(GPMC_CS_CONFIG1, 25, 26, clk-activation-ns); +GPMC_GET_TICKS_CD_MAX(GPMC_CS_CONFIG1, 18, 19, GPMC_CONFIG1_WAIT_MON_TIME_MAX, wait-monitoring-ns, GPMC_CD_CLK); use GPMC_CONFIG1_WAITMONTIME_MAX to have consistent naming. I used the naming the other define had. Saw no point in changing them. They go mostly unused anyway. Might as well change it. Not much of an issue. I leave it upto you. cheers, -roger +#define GPMC_SET_ONE_CD(reg, st, end, field, cd) \ +GPMC_SET_ONE_CD_MAX(reg, st, end, 0, field, GPMC_CD_FCLK) last parameter should be (cd) instead of GPMC_CD_FCLK. Ah, copy-paste, there you are again. Luckily this define goes unused, because all GPMC_SET_ONE_CD became GPMC_SET_ONE_CD_MAX anyway. So I'll delete that. Quite a keen eye, though ;) 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 9/8 v2] ARM OMAP2+ GPMC: fix programming/showing reserved timing parameters
Hi Roger, On 25 Feb 2015 11:44, Roger Quadros wrote: typo ATTCHEDDEVICEPAGELENGTH-ATTACHEDDEVICEPAGELENGTH Yep. +/** DEVICESIZE Max Value */ +#define GPMC_CONFIG1_DEVICESIZE_MAX GPMC_CONFIG1_DEVICESIZE_16 Shouldn't this be 1 instead? I'm hoping max value is without the shift based on GPMC_CONFIG1_CLKACTIVATIONTIME_MAX. Yes, it should. It's a remnant from the change below... + * @max Maximum parameter value (after optional @shift). max should be absolute value, without the shift. Yes, forgot to change that. +#define GPMC_GET_RAW_SHIFT(reg, st, end, shift, max, field) \ + get_gpmc_timing_reg(cs, (reg), (st), (end), (max), field, GPMC_CD_FCLK, (shift), 1, 1) Is it better to rename this to GPMC_GET_RAW_SHIFT_MAX() as it takes the max parameter. Ok. + GPMC_GET_RAW_SHIFT(GPMC_CS_CONFIG1, 23, 24, 4, 2, burst-length); want to define GPMC_CONFIG1_BURSTLENGTH_MAX? Yes, for consistency. Originally, i was not going to create defines for limits used only in one place, as opposed to WAITMONITORINGTIME and CLKACTIVATIONTIME. - GPMC_GET_TICKS_CD(GPMC_CS_CONFIG1, 18, 19, wait-monitoring-ns, GPMC_CD_CLK); - GPMC_GET_TICKS(GPMC_CS_CONFIG1, 25, 26, clk-activation-ns); + GPMC_GET_TICKS_CD_MAX(GPMC_CS_CONFIG1, 18, 19, GPMC_CONFIG1_WAIT_MON_TIME_MAX, wait-monitoring-ns, GPMC_CD_CLK); use GPMC_CONFIG1_WAITMONTIME_MAX to have consistent naming. I used the naming the other define had. Saw no point in changing them. They go mostly unused anyway. Might as well change it. +#define GPMC_SET_ONE_CD(reg, st, end, field, cd) \ + GPMC_SET_ONE_CD_MAX(reg, st, end, 0, field, GPMC_CD_FCLK) last parameter should be (cd) instead of GPMC_CD_FCLK. Ah, copy-paste, there you are again. Luckily this define goes unused, because all GPMC_SET_ONE_CD became GPMC_SET_ONE_CD_MAX anyway. So I'll delete that. Quite a keen eye, though ;) 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 9/8 v2] ARM OMAP2+ GPMC: fix programming/showing reserved timing parameters
Robert,
On 24/02/15 22:05, Robert ABEL wrote:
GPMC_CONFIG1_i parameters CLKACTIVATIONTIME and WAITMONITORINGTIME
have reserved values.
Raise an error if calculated timings try to program reserved values.
GPMC_CONFIG1_i ATTCHEDDEVICEPAGELENGTH and DEVICESIZE were already checked
typo ATTCHEDDEVICEPAGELENGTH-ATTACHEDDEVICEPAGELENGTH
when parsing the DT.
Explicitly comment invalid values on gpmc_cs_show_timings for
-CLKACTIVATIONTIME
-WAITMONITORINGTIME
-DEVICESIZE
-ATTACHEDDEVICEPAGELENGTH
Signed-off-by: Robert ABEL ra...@cit-ec.uni-bielefeld.de
---
drivers/memory/omap-gpmc.c | 69
++
1 file changed, 46 insertions(+), 23 deletions(-)
diff --git a/drivers/memory/omap-gpmc.c b/drivers/memory/omap-gpmc.c
index 6591991..cc2e6d0 100644
--- a/drivers/memory/omap-gpmc.c
+++ b/drivers/memory/omap-gpmc.c
@@ -135,6 +135,8 @@
#define GPMC_CONFIG1_WRITETYPE_ASYNC(0 27)
#define GPMC_CONFIG1_WRITETYPE_SYNC (1 27)
#define GPMC_CONFIG1_CLKACTIVATIONTIME(val) ((val 3) 25)
+/** CLKACTIVATIONTIME Max Ticks */
+#define GPMC_CONFIG1_CLKACTIVATIONTIME_MAX 2
#define GPMC_CONFIG1_PAGE_LEN(val) ((val 3) 23)
#define GPMC_CONFIG1_WAIT_READ_MON (1 22)
#define GPMC_CONFIG1_WAIT_WRITE_MON (1 21)
@@ -144,6 +146,8 @@
#define GPMC_CONFIG1_WAIT_PIN_SEL(val) ((val 3) 16)
#define GPMC_CONFIG1_DEVICESIZE(val)((val 3) 12)
#define GPMC_CONFIG1_DEVICESIZE_16 GPMC_CONFIG1_DEVICESIZE(1)
+/** DEVICESIZE Max Value */
+#define GPMC_CONFIG1_DEVICESIZE_MAX GPMC_CONFIG1_DEVICESIZE_16
Shouldn't this be 1 instead? I'm hoping max value is without the shift
based on GPMC_CONFIG1_CLKACTIVATIONTIME_MAX.
#define GPMC_CONFIG1_DEVICETYPE(val)((val 3) 10)
#define GPMC_CONFIG1_DEVICETYPE_NOR GPMC_CONFIG1_DEVICETYPE(0)
#define GPMC_CONFIG1_MUXTYPE(val) ((val 3) 8)
@@ -394,18 +398,21 @@ static void gpmc_cs_bool_timings(int cs, const struct
gpmc_bool_timings *p)
* @reg GPMC_CS_CONFIGn register offset.
* @st_bit Start Bit
* @end_bit End Bit. Must be = @st_bit.
+ * @max Maximum parameter value (after optional @shift).
max should be absolute value, without the shift.
+ * If 0, maximum is as high as @st_bit and @end_bit allow.
* @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 = value
* tick format: gpmc,name = value /zwj;* x ticks *zwj;/
+ * When @max is exceeded, invalid is printed inside comment.
* @noval Parameter values equal to 0 are not printed.
*
*/
static int get_gpmc_timing_reg(
/* timing specifiers */
- int cs, int reg, int st_bit, int end_bit,
+ int cs, int reg, int st_bit, int end_bit, int max,
const char *name, const enum gpmc_clk_domain cd,
/* value transform */
int shift,
@@ -415,11 +422,15 @@ static int get_gpmc_timing_reg(
u32 l;
int nr_bits;
int mask;
+ bool invalid;
l = gpmc_cs_read_reg(cs, reg);
nr_bits = end_bit - st_bit + 1;
mask = (1 nr_bits) - 1;;
l = (l st_bit) mask;
+ if (!max)
+ max = mask;
+ invalid = l max;
if (shift)
l = (shift l);
if (noval (l == 0))
@@ -429,11 +440,11 @@ static int get_gpmc_timing_reg(
unsigned int time_ns;
time_ns = gpmc_clk_ticks_to_ns(l, cs, cd);
- pr_info(gpmc,%s = %u /* %i ticks */\n,
- name, time_ns, l);
+ pr_info(gpmc,%s = %u /* %i ticks %s*/\n,
+ name, time_ns, l, invalid ? ; invalid : );
} else {
/* raw format */
- pr_info(gpmc,%s = %u\n, name, l);
+ pr_info(gpmc,%s = %u%s\n, name, l, invalid ? /* invalid
*/ : );
}
return l;
@@ -443,15 +454,19 @@ static int get_gpmc_timing_reg(
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), field, GPMC_CD_FCLK, 0, 1,
0)
+ get_gpmc_timing_reg(cs, (reg), (st), (end), 0, field, GPMC_CD_FCLK, 0,
1, 0)
+#define GPMC_GET_RAW_MAX(reg, st, end, max, field) \
+ get_gpmc_timing_reg(cs, (reg), (st), (end), (max), field, GPMC_CD_FCLK,
0, 1, 0)
#define GPMC_GET_RAW_BOOL(reg, st, end, field) \
- get_gpmc_timing_reg(cs, (reg), (st), (end), field, GPMC_CD_FCLK, 0, 1,
1)
-#define GPMC_GET_RAW_SHIFT(reg, st, end, shift, field) \
- get_gpmc_timing_reg(cs, (reg), (st), (end), field, GPMC_CD_FCLK,
(shift), 1, 1)
+ get_gpmc_timing_reg(cs, (reg), (st), (end), 0, field, GPMC_CD_FCLK, 0,
1, 1)
+#define GPMC_GET_RAW_SHIFT(reg, st, end, shift, max, field) \
+
