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

Hi Afzal,

On 09/27/2012 05:07 AM, Mohammed, Afzal wrote:

Hi Jon,

On Thu, Sep 27, 2012 at 08:54:22, Hunter, Jon wrote:

quoted

On 09/19/2012 08:23 AM, Afzal Mohammed wrote:

quoted

quoted

+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

quoted

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.

"

quoted

quoted

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

quoted

quoted

+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.

quoted

quoted

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

"

quoted

quoted

+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

quoted

* - 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.

Ok.

quoted

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.

I think that if these represent the main use-case configurations this
could add some value.

quoted

quoted

+/* can the cycles be avoided ? */

Nit should this be marked with a TODO?

quoted

quoted

+	/* 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

Sure, I don't have strong feelings about it but I would hope that at
some point this comment be removed.

quoted

quoted

+	gpmc_t->adv_rd_off = gpmc_round_ps_to_ticks(temp);

quoted

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.

Ok, thanks for clarifying.

quoted

quoted

+	gpmc_convert_ps_to_ns(gpmc_t);

quoted

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.

Ok, great. May be add a TODO here to make this clear that this is temporary.

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.
"

Ok.

quoted

quoted

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

quoted

quoted

-	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.

quoted

quoted

+	u32 t_aavdh;	/* address hold time */

quoted

quoted

+	u32 t_iaa;	/* initial access time */

quoted

quoted

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

quoted

quoted

+	u32 t_wpl;	/* write assertion time */

quoted

quoted

+	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.

Thanks!
Jon