On Tue, Oct 17, 2017 at 03:20:02PM +0300, Roger Quadros wrote:
> Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki. Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki
>
> On 17/10/17 14:55, Ladislav Michl wrote:
> > On Tue, Oct 17, 2017 at 02:35:46PM +0300, Roger Quadros wrote:
> >> Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki. Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki
> >>
> >> On 17/10/17 12:16, Ladislav Michl wrote:
> >>> On Tue, Oct 17, 2017 at 11:34:30AM +0300, Roger Quadros wrote:
> >>>> Hi,
> >>>>
> >>>>
> >>>> Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki. Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki
> >>>>
> >>>> On 16/10/17 02:21, Ladislav Michl wrote:
> >>>>> OneNAND sets up timings on its own.
> >>>>>
> >>>>> Signed-off-by: Ladislav Michl <ladis@xxxxxxxxxxxxxx>
> >>>>> ---
> >>>>> Note: see cover letter.
> >>>>>
> >>>>> drivers/memory/omap-gpmc.c | 167 ++++++++++++++++++++++++++++++++++++++++++++-
> >>>>> include/linux/omap-gpmc.h | 7 ++
> >>>>> 2 files changed, 171 insertions(+), 3 deletions(-)
> >>>>
> >>>> We could squash this into patch 7.
> >>>
> >>> Yes, but only after someone (myself after some coffee?) comes with an idea
> >>> how to return sync read/write flags into MTD driver properly.
> >>
> >> Currently gpmc-onenand.c doesn't make a clear distinction as to what it expects
> >> from DT. We need to make it clear so that only ASYNC timings (which is default mode of
> >> OneNAND) are provided at the least in DT.
> >>
> >> As SYNC timings depend on the OneNAND chip only that part should be dynamically
> >> set at runtime. for sync read/write flags during SYNC mode we probably need new
> >> DT properties.
> >>
> >> ti,onenand-sync-read; /* enable synchronous read */
> >> ti,onenand-sync-write; /* enable synchronous write */
> >>
> >> Only if one of them is set we program SYNC mode, else do nothing.
> >
> > See arch/arm/boot/dts/omap3-igep.dtsi (I'm okay with fixing that DT)
> > Really we need new properties? It seems those already provided
> > are are holding required info:
> > gpmc,sync-read;
> > gpmc,sync-write;
> > We just need to pass them to the MTD driver.
> >
> However sync-read/sync-write would be invalid settings for ASYNC mode right?
> Also omap3430-sdp.dts doesn't provide them.
As we agreed nodes without compatible property needs to be fixed anyway,
this probably does not matter.
> How do we know if the provided timings are for SYNC mode or ASYNC mode?
See bellow.
> >>>
> >>>>> diff --git a/drivers/memory/omap-gpmc.c b/drivers/memory/omap-gpmc.c
> >>>>> index fc7c3c2a0b04..b2fe3be4f914 100644
> >>>>> --- a/drivers/memory/omap-gpmc.c
> >>>>> +++ b/drivers/memory/omap-gpmc.c
> >>>>> @@ -1143,6 +1143,169 @@ struct gpmc_nand_ops *gpmc_omap_get_nand_ops(struct gpmc_nand_regs *reg, int cs)
> >>>>> }
> >>>>> EXPORT_SYMBOL_GPL(gpmc_omap_get_nand_ops);
> >>>>>
> >>>>> +static void gpmc_omap_onenand_calc_async_timings(struct gpmc_timings *t,
> >>>>> + struct gpmc_settings *s)
> >>>>> +{
> >>>>> + struct gpmc_device_timings dev_t;
> >>>>> + const int t_cer = 15;
> >>>>> + const int t_avdp = 12;
> >>>>> + const int t_aavdh = 7;
> >>>>> + const int t_ce = 76;
> >>>>> + const int t_aa = 76;
> >>>>> + const int t_oe = 20;
> >>>>> + const int t_cez = 20; /* max of t_cez, t_oez */
> >>>>> + const int t_wpl = 40;
> >>>>> + const int t_wph = 30;
> >>>>> +
> >>>>> + /*
> >>>>> + * Note that we need to keep sync_write set for the call to
> >>>>> + * omap2_onenand_set_async_mode() to work to detect the onenand
> >>>>> + * supported clock rate for the sync timings.
> >>>>> + */
> >>>>> + s->sync_read = false;
> >>>>> + s->sync_write = true;
> >>>>> +
> >>>>> + memset(&dev_t, 0, sizeof(dev_t));
> >>>>> +
> >>>>> + dev_t.t_avdp_r = max_t(int, t_avdp, t_cer) * 1000;
> >>>>> + dev_t.t_avdp_w = dev_t.t_avdp_r;
> >>>>> + dev_t.t_aavdh = t_aavdh * 1000;
> >>>>> + dev_t.t_aa = t_aa * 1000;
> >>>>> + dev_t.t_ce = t_ce * 1000;
> >>>>> + dev_t.t_oe = t_oe * 1000;
> >>>>> + dev_t.t_cez_r = t_cez * 1000;
> >>>>> + dev_t.t_cez_w = dev_t.t_cez_r;
> >>>>> + dev_t.t_wpl = t_wpl * 1000;
> >>>>> + dev_t.t_wph = t_wph * 1000;
> >>>>> +
> >>>>> + gpmc_calc_timings(t, s, &dev_t);
> >>>>> +}
> >>>>
> >>>> Let's get rid of manually setting async timings. The async timings should come from the
> >>>> device tree like it is done now for NAND or any other generic GPMC child.
> >>>
> >>> I'd go for smaller incremental steps :)
> >>>
> >>>> We can populate the same values into all the NAND DT nodes present as of now.
> >>>
> >>> And those values are?
> >>
> >> whatever that will lead you to the same timings & settings that you are setting in the above function.
> >> You could use gpmc_cs_show_timings() to help you with that with CONFIG_OMAP_GPMC_DEBUG enabled.
> >>
> >>>
> >>>>> +static int gpmc_omap_onenand_calc_sync_timings(struct gpmc_timings *t,
> >>>>> + struct gpmc_settings *s,
> >>>>> + int freq)
> >>>>> +{
> >>>>> + struct gpmc_device_timings dev_t;
> >>>>> + const int t_cer = 15;
> >>>>> + const int t_avdp = 12;
> >>>>> + const int t_cez = 20; /* max of t_cez, t_oez */
> >>>>> + const int t_wpl = 40;
> >>>>> + const int t_wph = 30;
> >>>>> + int min_gpmc_clk_period, t_ces, t_avds, t_avdh, t_ach, t_aavdh, t_rdyo;
> >>>>> + int div, gpmc_clk_ns, latency;
> >>>>> +
> >>>>> + if (!s->sync_read && !s->sync_write)
> >>>>> + return 0;
> >>>>> +
> >>>>> + switch (freq) {
> >>>>> + case 104:
> >>>>> + min_gpmc_clk_period = 9600; /* 104 MHz */
> >>>>> + t_ces = 3;
> >>>>> + t_avds = 4;
> >>>>> + t_avdh = 2;
> >>>>> + t_ach = 3;
> >>>>> + t_aavdh = 6;
> >>>>> + t_rdyo = 6;
> >>>>> + break;
> >>>>> + case 83:
> >>>>> + min_gpmc_clk_period = 12000; /* 83 MHz */
> >>>>> + t_ces = 5;
> >>>>> + t_avds = 4;
> >>>>> + t_avdh = 2;
> >>>>> + t_ach = 6;
> >>>>> + t_aavdh = 6;
> >>>>> + t_rdyo = 9;
> >>>>> + break;
> >>>>> + case 66:
> >>>>> + min_gpmc_clk_period = 15000; /* 66 MHz */
> >>>>> + t_ces = 6;
> >>>>> + t_avds = 5;
> >>>>> + t_avdh = 2;
> >>>>> + t_ach = 6;
> >>>>> + t_aavdh = 6;
> >>>>> + t_rdyo = 11;
> >>>>> + break;
> >>>>> + default:
> >>>>> + min_gpmc_clk_period = 18500; /* 54 MHz */
> >>>>> + t_ces = 7;
> >>>>> + t_avds = 7;
> >>>>> + t_avdh = 7;
> >>>>> + t_ach = 9;
> >>>>> + t_aavdh = 7;
> >>>>> + t_rdyo = 15;
> >>>>> + s->sync_write = false;
> >>>>> + s->burst_write = false;
> >>>>> + break;
> >>>>> + }
> >>>>> +
> >>>>> + div = gpmc_calc_divider(min_gpmc_clk_period);
> >>>>> + gpmc_clk_ns = gpmc_ticks_to_ns(div);
> >>>>> + if (gpmc_clk_ns < 12) /* >83 MHz */
> >>>>> + latency = 8;
> >>>>> + else if (gpmc_clk_ns < 15) /* >66 MHz */
> >>>>> + latency = 6;
> >>>>> + else if (gpmc_clk_ns >= 25) /* 40 MHz */
> >>>>> + latency = 3;
> >>>>> + else
> >>>>> + latency = 4;
> >>>>> +
> >>>>> + /* Set synchronous read timings */
> >>>>> + memset(&dev_t, 0, sizeof(dev_t));
> >>>>> +
> >>>>> + if (!s->sync_write) {
> >>>>> + dev_t.t_avdp_w = max(t_avdp, t_cer) * 1000;
> >>>>> + dev_t.t_wpl = t_wpl * 1000;
> >>>>> + dev_t.t_wph = t_wph * 1000;
> >>>>> + dev_t.t_aavdh = t_aavdh * 1000;
> >>>>> + }
> >>>>> + dev_t.ce_xdelay = true;
> >>>>> + dev_t.avd_xdelay = true;
> >>>>> + dev_t.oe_xdelay = true;
> >>>>> + dev_t.we_xdelay = true;
> >>>>> + dev_t.clk = min_gpmc_clk_period;
> >>>>> + dev_t.t_bacc = dev_t.clk;
> >>>>> + dev_t.t_ces = t_ces * 1000;
> >>>>> + dev_t.t_avds = t_avds * 1000;
> >>>>> + dev_t.t_avdh = t_avdh * 1000;
> >>>>> + dev_t.t_ach = t_ach * 1000;
> >>>>> + dev_t.cyc_iaa = (latency + 1);
> >>>>> + dev_t.t_cez_r = t_cez * 1000;
> >>>>> + dev_t.t_cez_w = dev_t.t_cez_r;
> >>>>> + dev_t.cyc_aavdh_oe = 1;
> >>>>> + dev_t.t_rdyo = t_rdyo * 1000 + min_gpmc_clk_period;
> >>>>> +
> >>>>> + gpmc_calc_timings(t, s, &dev_t);
> >>>>> +
> >>>>> + return latency;
> >>>>> +}
> >>>>> +
> >>>>> +int gpmc_omap_onenand_set_sync_timings(struct device *dev, int cs, int freq)
> >>>>> +{
> >>>>> + int latency, ret;
> >>>>> + struct gpmc_timings gpmc_t;
> >>>>> + struct gpmc_settings gpmc_s;
> >>>>> +
> >>>>> + gpmc_read_settings_dt(dev->of_node, &gpmc_s);
> >>>>> +
> >>>>> + latency = gpmc_omap_onenand_calc_sync_timings(&gpmc_t, &gpmc_s, freq);
> >>>>> + if (latency > 0) {
> >>>>> + ret = gpmc_cs_program_settings(cs, &gpmc_s);
> >>>>> + if (ret < 0)
> >>>>> + return ret;
> >>>>> +
> >>>>> + ret = gpmc_cs_set_timings(cs, &gpmc_t, &gpmc_s);
> >>>>> + if (ret < 0)
> >>>>> + return ret;
> >>>>> + }
> >>>>> +
> >>>>> +// if (gpmc_s.sync_read)
> >>>>> +// if (gpmc_s.sync_write)
> >>>>
> >>>> ??
> >>>
> >>> See first paragraph answer and cover letter
> >> so these hints should come from onenand Driver. It is supposed to decide whether
> >> sync mode is required or not and for which direction.
> >
> > Do you mean driver itself gets this knowledge from ti,onenand-sync-read and
> > ti,onenand-sync-write properties above? And what about gpmc properties
>
> yes.
>
> > foir sync access?
> >
>
> I'm not sure. Are the GPMC setting very different in the 2 modes?
> Can they be built at run time using the hints freq, sync-read, sync-write and
> the async timings/settings?
>
> If not then they will have to be provided in the DT
>
> e.g.
> gpmc {
> onenand {
> <async timings>;
> <async settings>;
>
> {
> <sync settings>;
> <sync timings>;
> };
> };
> };
Please note that we do not need sync and async settings at the same time.
(I swear, from now on, I'll stop pointing at patches 1-5, which were created
only to show what is currently happening at timings setup time).
In case sync timings is requested, async timings is programmed only to read
device frequency needed to setup sync timings. So we probably need either
sync or async timings, not both.
Having datasheet would help a bit here, as it is hard to argue what is really
needed based only on informations from current code :-/
> >>>
> >>>>> +
> >>>>> + return latency;
> >>>>> +}
> >>>>> +
> >>>>> +EXPORT_SYMBOL_GPL(gpmc_omap_onenand_set_sync_timings);
> >>>>> +
> >>>>> int gpmc_get_client_irq(unsigned irq_config)
> >>>>> {
> >>>>> if (!gpmc_irq_domain) {
> >>>>> @@ -2097,9 +2260,7 @@ static int gpmc_probe_child(struct platform_device *pdev,
> >>>>> if (ret)
> >>>>> goto err;
> >>>>>
> >>>>> - /* OneNAND driver handles timings on its own */
> >>>>> - gpmc_cs_enable_mem(cs);
> >>>>> - goto no_timings;
> >>>>> + gpmc_omap_onenand_calc_async_timings(&gpmc_t, &gpmc_s);
> >>>>
> >>>> this should be undone.
> >>>
> >>> Yes, after we fix timings in DT. Seems you are considering it safe and easy,
> >>> okay to sent separate patches this late in release cycle? ;-)
> >>> We could start merging with clean table then.
> >>>
> >>>>> } else {
> >>>>> ret = of_property_read_u32(child, "bank-width",
> >>>>> &gpmc_s.device_width);
> >>>>> diff --git a/include/linux/omap-gpmc.h b/include/linux/omap-gpmc.h
> >>>>> index fd0de00c0d77..22a2e6b43669 100644
> >>>>> --- a/include/linux/omap-gpmc.h
> >>>>> +++ b/include/linux/omap-gpmc.h
> >>>>> @@ -28,12 +28,19 @@ struct gpmc_nand_regs;
> >>>>> #if IS_ENABLED(CONFIG_OMAP_GPMC)
> >>>>> struct gpmc_nand_ops *gpmc_omap_get_nand_ops(struct gpmc_nand_regs *regs,
> >>>>> int cs);
> >>>>> +int gpmc_omap_onenand_set_sync_timings(struct device *dev, int cs, int freq);
> >>>>> #else
> >>>>> static inline struct gpmc_nand_ops *gpmc_omap_get_nand_ops(struct gpmc_nand_regs *regs,
> >>>>> int cs)
> >>>>> {
> >>>>> return NULL;
> >>>>> }
> >>>>> +
> >>>>> +static inline int gpmc_omap_onenand_set_sync_timings(struct device *dev,
> >>>>> + int cs, int freq)
> >>>>> +{
> >>>>> + return 0;
> >>>>> +}
> >>>>> #endif /* CONFIG_OMAP_GPMC */
> >>>>>
> >>>>> /*--------------------------------*/
> >>>>>
> >>>>
> >>
>
> --
> 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
