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