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 <rabel@xxxxxxxxxxxxxxxxxxxxxxx> > --- > 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. > * @name DTS 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> /‍* 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) > + 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) \ > + 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. > #define GPMC_GET_TICKS(reg, st, end, field) \ > - get_gpmc_timing_reg(cs, (reg), (st), (end), field, GPMC_CD_FCLK, 0, 0, 0) > + get_gpmc_timing_reg(cs, (reg), (st), (end), 0, field, GPMC_CD_FCLK, 0, 0, 0) > #define GPMC_GET_TICKS_CD(reg, st, end, field, cd) \ > - get_gpmc_timing_reg(cs, (reg), (st), (end), field, (cd), 0, 0, 0) > + get_gpmc_timing_reg(cs, (reg), (st), (end), 0, field, (cd), 0, 0, 0) > +#define GPMC_GET_TICKS_CD_MAX(reg, st, end, max, field, cd) \ > + get_gpmc_timing_reg(cs, (reg), (st), (end), (max), field, (cd), 0, 0, 0) > > static void gpmc_show_regs(int cs, const char *desc) > { > @@ -475,11 +490,11 @@ static void gpmc_cs_show_timings(int cs, const char *desc) > pr_info("gpmc cs%i access configuration:\n", cs); > GPMC_GET_RAW_BOOL(GPMC_CS_CONFIG1, 4, 4, "time-para-granularity"); > GPMC_GET_RAW(GPMC_CS_CONFIG1, 8, 9, "mux-add-data"); > - GPMC_GET_RAW(GPMC_CS_CONFIG1, 12, 13, "device-width"); > + GPMC_GET_RAW_MAX(GPMC_CS_CONFIG1, 12, 13, GPMC_CONFIG1_DEVICESIZE_MAX, "device-width"); > GPMC_GET_RAW(GPMC_CS_CONFIG1, 16, 17, "wait-pin"); > GPMC_GET_RAW_BOOL(GPMC_CS_CONFIG1, 21, 21, "wait-on-write"); > GPMC_GET_RAW_BOOL(GPMC_CS_CONFIG1, 22, 22, "wait-on-read"); > - GPMC_GET_RAW_SHIFT(GPMC_CS_CONFIG1, 23, 24, 4, "burst-length"); > + GPMC_GET_RAW_SHIFT(GPMC_CS_CONFIG1, 23, 24, 4, 2, "burst-length"); want to define GPMC_CONFIG1_BURSTLENGTH_MAX? > GPMC_GET_RAW_BOOL(GPMC_CS_CONFIG1, 27, 27, "sync-write"); > GPMC_GET_RAW_BOOL(GPMC_CS_CONFIG1, 28, 28, "burst-write"); > GPMC_GET_RAW_BOOL(GPMC_CS_CONFIG1, 29, 29, "gpmc,sync-read"); > @@ -519,8 +534,8 @@ static void gpmc_cs_show_timings(int cs, const char *desc) > GPMC_GET_TICKS(GPMC_CS_CONFIG6, 0, 3, "bus-turnaround-ns"); > GPMC_GET_TICKS(GPMC_CS_CONFIG6, 8, 11, "cycle2cycle-delay-ns"); > > - 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. > + GPMC_GET_TICKS_CD_MAX(GPMC_CS_CONFIG1, 25, 26, GPMC_CONFIG1_CLKACTIVATIONTIME_MAX, "clk-activation-ns", GPMC_CD_FCLK); > > GPMC_GET_TICKS(GPMC_CS_CONFIG6, 16, 19, "wr-data-mux-bus-ns"); > GPMC_GET_TICKS(GPMC_CS_CONFIG6, 24, 28, "wr-access-ns"); > @@ -537,13 +552,15 @@ static inline void gpmc_cs_show_timings(int cs, const char *desc) > * @reg GPMC_CS_CONFIGn register offset. > * @st_bit Start Bit > * @end_bit End Bit. Must be >= @st_bit. > + * @max Maximum parameter value. > + * If 0, maximum is as high as @st_bit and @end_bit allow. > * @time Timing parameter in ns. > * @cd Timing parameter clock domain. > * @name Timing parameter name. > * @note Caller is expected to have initialized CONFIG1 GPMCFCLKDIVIDER > * prior to calling this function with @cd equal to GPMC_CD_CLK. > */ > -static int set_gpmc_timing_reg(int cs, int reg, int st_bit, int end_bit, > +static int set_gpmc_timing_reg(int cs, int reg, int st_bit, int end_bit, int max, > int time, enum gpmc_clk_domain cd, const char *name) > { > u32 l; > @@ -556,9 +573,12 @@ static int set_gpmc_timing_reg(int cs, int reg, int st_bit, int end_bit, > nr_bits = end_bit - st_bit + 1; > mask = (1 << nr_bits) - 1; > > - if (ticks > mask) { > + if (!max) > + max = mask; > + > + if (ticks > max) { > pr_err("%s: GPMC CS%d: %s %d ns, %d ticks > %d ticks\n", > - __func__, cs, name, time, ticks, mask); > + __func__, cs, name, time, ticks, max); > > return -1; > } > @@ -577,13 +597,16 @@ static int set_gpmc_timing_reg(int cs, int reg, int st_bit, int end_bit, > return 0; > } > > -#define GPMC_SET_ONE_CD(reg, st, end, field, cd) \ > - if (set_gpmc_timing_reg(cs, (reg), (st), (end), \ > - t->field, (cd), #field) < 0) \ > +#define GPMC_SET_ONE_CD_MAX(reg, st, end, max, field, cd) \ > + if (set_gpmc_timing_reg(cs, (reg), (st), (end), (max), \ > + t->field, (cd), #field) < 0) \ > return -1 > > +#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. > + > #define GPMC_SET_ONE(reg, st, end, field) \ > - GPMC_SET_ONE_CD(reg, st, end, field, GPMC_CD_FCLK) > + GPMC_SET_ONE_CD_MAX(reg, st, end, 0, field, GPMC_CD_FCLK) > > /** > * gpmc_calc_waitmonitoring_divider - calculate proper GPMCFCLKDIVIDER based on WAITMONITORINGTIME > @@ -702,8 +725,8 @@ int gpmc_cs_set_timings(int cs, const struct gpmc_timings *t, const struct gpmc_ > l |= (div - 1); > gpmc_cs_write_reg(cs, GPMC_CS_CONFIG1, l); > > - GPMC_SET_ONE_CD(GPMC_CS_CONFIG1, 18, 19, wait_monitoring, GPMC_CD_CLK); > - GPMC_SET_ONE(GPMC_CS_CONFIG1, 25, 26, clk_activation); > + GPMC_SET_ONE_CD_MAX(GPMC_CS_CONFIG1, 18, 19, GPMC_CONFIG1_WAIT_MON_TIME_MAX, wait_monitoring, GPMC_CD_CLK); > + GPMC_SET_ONE_CD_MAX(GPMC_CS_CONFIG1, 25, 26, GPMC_CONFIG1_CLKACTIVATIONTIME_MAX, clk_activation, GPMC_CD_FCLK); > > #ifdef DEBUG > printk(KERN_INFO "GPMC CS%d CLK period is %lu ns (div %d)\n", > cheers, -roger -- 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