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.
How do we know if the provided timings are for SYNC mode or ASYNC mode?
>>>
>>>>> 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>;
};
};
};
>>>
>>>>> +
>>>>> + 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
