[PATCH v7 08/11] ARM: OMAP2+: gpmc: generic timing calculation

[Mohammed, Afzal]

Hi Jon,

On Thu, Sep 27, 2012 at 08:54:22, Hunter, Jon wrote:
> On 09/19/2012 08:23 AM, Afzal Mohammed wrote:

> > +Dependency of peripheral timings on gpmc timings:
> > +
> > +cs_on: t_ceasu
> 
> Thanks for adding these details. Could be good to clarify that the
> left-hand side parameters are the gpmc register fields and the
> right-hand side are the timing parameters these are calculated using.

Ok

> 
> Also, given that this is in documentation for completeness it could be
> good to somewhere state what "t_ceasu" means. For the gpmc register
> field description may be we could give a reference to an OMAP document.

Yes, it has been mentioned, as quoted below, reason it has not been
mentioned here is to avoid duplication. I will add TRM reference.

"
> > +Note: Many of gpmc timings are dependent on other gpmc timings (a few

> > +mentioned above, refer timing routine for more details. To know what
> > +these peripheral timings correspond to, please see explanations in
> > +struct gpmc_device_timings definition.

> > +struct gpmc_device_timings {
> > +	u32 t_ceasu;	/* address setup to CS valid */
"

> > +adv_rd_off: t_avdp_r, t_avdh(s*)
> > +oe_on: t_oeasu, t_aavdh(a**), t_ach(s), cyc_aavdh_oe(s)
> 
> Would it be better to have ...
> 
> oe_on (sync):	t_oeasu, t_ach(*), cyc_aavdh_oe
> oe_on (async):	t_oeasu, t_aavdh(*)

Ok

> * - optional
> 
> I assume that the hold times from the clock (t_ach and t_aavdh) are used
> for fine tuning if the peripheral requires this, but a user adding a new
> device would not be required to specify these, where as t_oeasu is
> mandatory.

It depends on the peripheral, t_oeasu in not used for OneNAND, tusb sync,
so I prefer not mentioning any timing as optional or mandatory.

> Or maybe should the timings be grouped as ...
> 
> General
> Read Async
> Read Async Address/Data Multiplexed
> Read Sync
> Read Sync Address/Data Multiplexed
> Write Async
> Write Async Address/Data Multiplexed
> Write Sync
> Write Sync Address/Data Multiplexed
> 
> There may be some duplication but it will be clear where things like ADV
> timing applies.

I would prefer to keep it concise, but no strong opinion on it, if you
prefer as above, I will change it.

> > +/* can the cycles be avoided ? */
> 
> Nit should this be marked with a TODO?

> > +	/* mux check required ? */
> 
> Nit should this be marked with a TODO?

Marking XXX should Ok, right ?, reason is that they are not
kept as TODO, but rather as pointers to may be possible
improvements

> > +	gpmc_t->adv_rd_off = gpmc_round_ps_to_ticks(temp);

> Any reason why we can't return ns in the above function? Or make this
> function gpmc_round_ps_to_ns()? Then we could avoid having
> gpmc_convert_ps_to_ns() below.

Calculation in ps is required to get more accurate results.

Calculating in ns was the reason for issue faced on N800 for Tony
with previous version.

I will explain what would have happened with v6 on N800,
i.e. using ns values,
Based on logs from Tony, gpmc clk was 9ns, actually it would
have been somewhere around 9115ps.

Take below timings of previous version in tusb async case

        gpmc_t->oe_on = gpmc_round_ps_to_ticks(temp) / 1000;

        /* access */
        temp = max_t(u32, dev_t->t_iaa, /* remove t_iaa in async ? */
                                gpmc_t->oe_on * 1000 + dev_t->t_oe);
        temp = max_t(u32, temp,
                                gpmc_t->cs_on * 1000 + dev_t->t_ce);
        temp = max_t(u32, temp,
                                gpmc_t->adv_on * 1000 + dev_t->t_aa);
        gpmc_t->access = gpmc_round_ps_to_ticks(temp) / 1000;

Upon calculating we get,

oe_on = 63805 / 1000 = 63

and for access (t_oe = 300, t_ce = t_aa = t_iaa = 0),

temp = 63 * 1000 + 300 = 63300
access = 63300 / 1000 = 63

Here we get oe_on as well as access as 7 ticks, but access should
have been 8 ticks, which is what we will get by using ps values,
i.e. as in this version, as below,

        gpmc_t->oe_on = gpmc_round_ps_to_ticks(temp);

        /* access */
        temp = max_t(u32, dev_t->t_iaa, /* remove t_iaa in async ? */
                                gpmc_t->oe_on + dev_t->t_oe);
        temp = max_t(u32, temp,
                                gpmc_t->cs_on + dev_t->t_ce);
        temp = max_t(u32, temp,
                                gpmc_t->adv_on + dev_t->t_aa);
        gpmc_t->access = gpmc_round_ps_to_ticks(temp);

I believe it is always better to go with higher resolution.

> > +	gpmc_convert_ps_to_ns(gpmc_t);

> I am wondering if we could avoid this above function and then ...

This will be removed once it is confirmed that all the peripherals
using custom runtime calculation can work with this generic
routine. Then all calculation would be purely in ps.

Right now converting ps to ns has been kept only to be compatible
with custom routines and so that we can easily go back to custom
routines in case of any issues, quoting relevant commit message below,

"
    Timings are calculated in ps to prevent rounding errors and
    converted to ns at final stage so that these values can be
    directly fed to gpmc_cs_set_timings(). gpmc_cs_set_timings()
    would be modified to take ps once all custom timing routines
    are replaced by the generic routine, at the same time
    generic timing routine would be modified to provide timings
    in ps. struct gpmc_timings field types are upgraded from
    u16 => u32 so that it can hold ps values.
"

> > @@ -116,42 +116,103 @@ struct gpmc_timings {

> > -	u16 wr_access;		/* WRACCESSTIME */
> > -	u16 wr_data_mux_bus;	/* WRDATAONADMUXBUS */
> > +	u32 wr_access;		/* WRACCESSTIME */
> > +	u32 wr_data_mux_bus;	/* WRDATAONADMUXBUS */
> 
>  ... we could keep the above u16.

Due to the reasons mentioned above u32 is required.

> > +	u32 t_aavdh;	/* address hold time */

> > +	u32 t_iaa;	/* initial access time */

> > +	u32 t_oe;	/* access time from OE assertion */

> > +	u32 t_wpl;	/* write assertion time */

> > +	u8 cyc_aavdh_oe;
> > +	u8 cyc_aavdh_we;
> > +	u8 cyc_oe;
> > +	u8 cyc_wpl;
> > +	u32 cyc_iaa;
> 
> May be I should look at an example, but it would be good to document
> what these cyc_xxx parameters are/represent.

These are cycles counterpart of that of time, I will update it too.

Regards
Afzal