Re: [PATCH 7/8 v2] ARM OMAP2+ GPMC: calculate GPMCFCLKDIVIDER based on WAITMONITORINGTIME

[Robert Abel]

Hi Roger,

On 25 Feb 2015 17:33, Roger Quadros wrote:
> ./scripts/checkpatch.pl detects some styling errors.
Well, there's like a million lines over 80 characters. I'm also a 
heathen and don't use an 80 character terminal either.
I'll fix the more serious issues checkpatch finds, but some styling 
errors are even from code already in the driver.

> >   #define GPMC_CONFIG1_WAIT_MON_IIME(val) ((val & 3) << 18)
> Not caused by your patch but we can fix the typo
>
> GPMC_CONFIG1_WAIT_MON_IIME -> GPMC_CONFIG1_WAIT_MON_TIME
I'd rather remove than fix. None of these defines is in active use anymore.
> > +/**
> > + * gpmc_calc_waitmonitoring_divider - calculate proper GPMCFCLKDIVIDER based on WAITMONITORINGTIME
> > + * @wait_monitoring WAITMONITORINGTIME in ns.
> > + * @return          -1 on failure to scale, else proper divider > 0.
> > + * @note            WAITMONITORINGTIME will be _at least_ as long as desired.
> > + *                  i.e. read timings should be kept            -> don't sample bus too early
> > + *                  while write timings should work out as well -> data is longer on bus
> > + */
> > +static int gpmc_calc_waitmonitoring_divider(unsigned int wait_monitoring)
> > +{
> > +
> > +	int div = gpmc_ns_to_ticks(wait_monitoring);
> > +
> > +	div += GPMC_CONFIG1_WAIT_MON_TIME_MAX - 1;
> > +	div /= GPMC_CONFIG1_WAIT_MON_TIME_MAX;
> Sorry I didn't understand how this works. :P
>  From the TRM,
>
> waitmonitoringtime_ns = waitmonitoring_ticks x (gpmc_clk_div + 1) x gpmc_fclk_ns
Using that formula:
gpmc_clk_div + 1 = ceil(ceil(waitmonitoringtime_ns / gpmc_fclk_ns) / 
waitmonitoring_ticks).

The reason I use GPMC_CONFIG1_WAIT_MON_TIME_MAX directly, is because 
every other pair is identical:

We have the following cases:
a) WAITMONITORINGTIME = 0, so div doesn't matter (we set div to 1) or
b) WAITMONITORINGTIME = 1, with div = 1 or
c) WAITMONITORINGTIME = 2, with div = n

WAITMONITORINGTIME is never 1 with div = n and div /= 1, because that's 
the same as WAITMONITORINGTIME = 2 with div = n - 1.
Cases a) and b) are caught using the fact that div = 0 after the division.
Case c) is handled by assuming that WAITMONITORINGTIME will always be 2 
(i.e. GPMC_CONFIG1_WAIT_MON_TIME_MAX) for all divs /= 0 at that point.

Took me a while to realize that this works, too.

> > +	/*
> > +	 * See if we need to change the divider for waitmonitoringtime.
> > +	 *
> > +	 * If DT contains sync-clk-ps for truly asynchronous accesses,
> > +	 * ignore it as long as waitmonitoringtime is used.
> > +	 *
> The comment in the $subject was more easier to understand for me than the above
>
> "Calculate GPMCFCLKDIVIDER independent of gpmc,sync-clk-ps in DT for
> truly asynchronous accesses, i.e. both read and write asynchronous."
Well, the comment in $subject was a general description. Here, we don't 
touch div when
a) any access is synchronous
b) all accesses are asynchronous, but WAITMONITORINGTIME is not used, 
i.e. WAITREADMONITORING and WAITWRITEMONITORING are both not set.

So, if WAITMONITORINGTIME is not used, or any access is synchronous, we 
program the sync-clk-ps anyways, because the GPMC won't rely on it for 
any timing.
So the comment is OK, IMHO.

Regards,

Robert
--
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/