Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki. Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki
On 17/10/17 12:24, Ladislav Michl wrote:
> On Tue, Oct 17, 2017 at 11:33:56AM +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:22, Ladislav Michl wrote:
>>
>> missing commit log
>
> There will be enough future versions to add some :)
>
>>> Signed-off-by: Ladislav Michl <ladis@xxxxxxxxxxxxxx>
>>> ---
>>> drivers/mtd/onenand/omap2.c | 145 +++++++++++++++++++++++++++++++++-----------
>>> 1 file changed, 110 insertions(+), 35 deletions(-)
>>>
>>> diff --git a/drivers/mtd/onenand/omap2.c b/drivers/mtd/onenand/omap2.c
>>> index 24a1388d3031..4811897a4e9f 100644
>>> --- a/drivers/mtd/onenand/omap2.c
>>> +++ b/drivers/mtd/onenand/omap2.c
>>> @@ -28,6 +28,8 @@
>>> #include <linux/mtd/mtd.h>
>>> #include <linux/mtd/onenand.h>
>>> #include <linux/mtd/partitions.h>
>>> +#include <linux/of_device.h>
>>> +#include <linux/omap-gpmc.h>
>>> #include <linux/platform_device.h>
>>> #include <linux/interrupt.h>
>>> #include <linux/delay.h>
>>> @@ -38,7 +40,6 @@
>>> #include <linux/gpio.h>
>>>
>>> #include <asm/mach/flash.h>
>>> -#include <linux/platform_data/mtd-onenand-omap2.h>
>>>
>>> #include <linux/omap-dma.h>
>>>
>>> @@ -57,10 +58,8 @@ struct omap2_onenand {
>>> struct completion irq_done;
>>> struct completion dma_done;
>>> int dma_channel;
>>> - int freq;
>>> - int (*setup)(void __iomem *base, int *freq_ptr);
>>> struct regulator *regulator;
>>> - u8 flags;
>>> + bool is_omap2;
>>> };
>>>
>>> static void omap2_onenand_dma_cb(int lch, u16 ch_status, void *data)
>>> @@ -90,6 +89,52 @@ static inline void write_reg(struct omap2_onenand *c, unsigned short value,
>>> writew(value, c->onenand.base + reg);
>>> }
>>>
>>> +/* Ensure sync read and sync write are disabled */
>>> +static void set_async_mode(struct omap2_onenand *c)
>>
>> omap2_onenand_set_async_mode()
>>
>>> +{
>>> + unsigned short reg = read_reg(c, ONENAND_REG_SYS_CFG1);
>> need blank line
>>
>>> + reg &= ~ONENAND_SYS_CFG1_SYNC_READ & ~ONENAND_SYS_CFG1_SYNC_WRITE;
>>> + write_reg(c, reg, ONENAND_REG_SYS_CFG1);
>>> +}
>>> +
>>> +static void set_cfg(struct omap2_onenand *c, bool sr, bool sw, int latency)
>>
>> omap2_onenand_set_cfg()
>>
>>> +{
>>> + unsigned short reg = ONENAND_SYS_CFG1_RDY | ONENAND_SYS_CFG1_INT;
>>> +
>>> + reg |= (latency << ONENAND_SYS_CFG1_BRL_SHIFT) |
>>> + ONENAND_SYS_CFG1_BL_16;
>>> + if (latency > 5)
>>> + reg |= ONENAND_SYS_CFG1_HF;
>>> + if (latency > 7)
>>> + reg |= ONENAND_SYS_CFG1_VHF;
>>
>> This will set both HF and VHF flags if latency is greater than 7. Is this what we want?
>
> That's why patches 1-5 are there, so you can verify...
original code is
if (gpmc_clk_ns < 15) /* >66MHz */
onenand_flags |= ONENAND_FLAG_HF;
else
onenand_flags &= ~ONENAND_FLAG_HF;
if (gpmc_clk_ns < 12) /* >83MHz */
onenand_flags |= ONENAND_FLAG_VHF;
else
onenand_flags &= ~ONENAND_FLAG_VHF;
if (onenand_flags & ONENAND_FLAG_VHF)
latency = 8;
else if (onenand_flags & ONENAND_FLAG_HF)
latency = 6;
else if (gpmc_clk_ns >= 25) /* 40 MHz*/
latency = 3;
else
latency = 4;
which looks like both HF and VHF flags are set for >=83MHz.
>
>>> + if (sr)
>>> + reg |= ONENAND_SYS_CFG1_SYNC_READ;
>>> + if (sw)
>>> + reg |= ONENAND_SYS_CFG1_SYNC_WRITE;
>>> +
>>> + write_reg(c, reg, ONENAND_REG_SYS_CFG1);
>>> +}
>>> +
>>> +static int get_freq(struct omap2_onenand *c)
>>
>> omap2_onenand_get_freq()
>
> All other helper functions are unprefixed, but will change that.
>
>>> +{
>>> + unsigned short ver = read_reg(c, ONENAND_REG_VERSION_ID);
>>> +
>>> + switch ((ver >> 4) & 0xf) {
>>> + case 0:
>>> + return 40;
>>> + case 1:
>>> + return 54;
>>> + case 2:
>>> + return 66;
>>> + case 3:
>>> + return 83;
>>> + case 4:
>>> + return 104;
>>
>> default:
>> return -EINVAL;
>> ?
>
> As we were returning -ENODEV in this case I'd use it.
>
>>> + }
>>> +
>>> + return 0;
>>> +}
>>> +
>>> static void wait_err(char *msg, int state, unsigned int ctrl, unsigned int intr)
>>> {
>>> printk(KERN_ERR "onenand_wait: %s! state %d ctrl 0x%04x intr 0x%04x\n",
>>> @@ -153,7 +198,7 @@ static int omap2_onenand_wait(struct mtd_info *mtd, int state)
>>> if (!(syscfg & ONENAND_SYS_CFG1_IOBE)) {
>>> syscfg |= ONENAND_SYS_CFG1_IOBE;
>>> write_reg(c, syscfg, ONENAND_REG_SYS_CFG1);
>>> - if (c->flags & ONENAND_IN_OMAP34XX)
>>> + if (!c->is_omap2)
>>
>> seems like you should have a is_omap3 flag as this quirk is for omap3 only.
>
> I was not sure about logic. Quirk is certainly not needed for omap2 and I'm
> not sure if OneNAND can be connected to omap4 and above.
>
all OMAPs can connect to OneNAND. Just that nobody used them beyond omap3.
>>> /* Add a delay to let GPIO settle */
>>> syscfg = read_reg(c, ONENAND_REG_SYS_CFG1);
>>> }
>>> @@ -607,30 +652,46 @@ static int omap2_onenand_disable(struct mtd_info *mtd)
>>> return ret;
>>> }
>>>
>>> +static int omap2_onenand_get_dt(struct device *dev, struct omap2_onenand *c)
>>> +{
>>> + struct device_node *child = dev->of_node;
>>> + u32 cs, val;
>>> +
>>> + if (of_property_read_u32(child, "reg", &cs) < 0) {
>>> + dev_err(dev, "reg not found in DT\n");
>>> + return -EINVAL;
>>> + }
>>> + c->gpmc_cs = cs;
>>> +
>>> + if (!of_property_read_u32(child, "dma-channel", &val))
>>> + c->dma_channel = val;
>>> + else
>>> + c->dma_channel = -1;
>>> +
>>> + c->is_omap2 = (bool)of_device_get_match_data(dev);
>>> +
>>> + /* TODO: gpio, regulator, unlocking */
>>> +
>>> + return 0;
>>> +}
>>> +
>>> static int omap2_onenand_probe(struct platform_device *pdev)
>>> {
>>> - struct omap_onenand_platform_data *pdata;
>>> struct omap2_onenand *c;
>>> struct onenand_chip *this;
>>> - int r;
>>> + int freq, r;
>>> struct resource *res;
>>>
>>> - pdata = dev_get_platdata(&pdev->dev);
>>> - if (pdata == NULL) {
>>> - dev_err(&pdev->dev, "platform data missing\n");
>>> - return -ENODEV;
>>> - }
>>> -
>>> c = kzalloc(sizeof(struct omap2_onenand), GFP_KERNEL);
>>> if (!c)
>>> return -ENOMEM;
>>>
>>> + r = omap2_onenand_get_dt(&pdev->dev, c);
>>> + if (r)
>>> + goto err_kfree;
>>> +
>>> init_completion(&c->irq_done);
>>> init_completion(&c->dma_done);
>>> - c->flags = pdata->flags;
>>> - c->gpmc_cs = pdata->cs;
>>> - c->gpio_irq = pdata->gpio_irq;
>>> - c->dma_channel = pdata->dma_channel;
>>> if (c->dma_channel < 0) {
>>> /* if -1, don't use DMA */
>>> c->gpio_irq = 0;
>>> @@ -659,16 +720,23 @@ static int omap2_onenand_probe(struct platform_device *pdev)
>>> goto err_release_mem_region;
>>> }
>>>
>>> - if (pdata->onenand_setup != NULL) {
>>> - r = pdata->onenand_setup(c->onenand.base, &c->freq);
>>> - if (r < 0) {
>>> - dev_err(&pdev->dev, "Onenand platform setup failed: "
>>> - "%d\n", r);
>>> - goto err_iounmap;
>>> - }
>>> - c->setup = pdata->onenand_setup;
>>> + set_async_mode(c);
>>> + freq = get_freq(c);
>>> + if (!freq) {
>>> + dev_err(&pdev->dev,
>>> + "Rate not detected, bad GPMC async timings?\n");
>>> + r = -ENODEV;
>>> + goto err_iounmap;
>>> }
>>>
>>> + r = gpmc_omap_onenand_set_sync_timings(&pdev->dev, c->gpmc_cs, freq);
>>> + if (r < 0) {
>>> + dev_err(&pdev->dev, "Cannot set sync timings: %d\n", r);
>>> + goto err_iounmap;
>>> + }
>>> + if (r > 0)
>>> + set_cfg(c, true, true, r);
>>> +
>>> if (c->gpio_irq) {
>>> if ((r = gpio_request(c->gpio_irq, "OneNAND irq")) < 0) {
>>> dev_err(&pdev->dev, "Failed to request GPIO%d for "
>>> @@ -706,27 +774,27 @@ static int omap2_onenand_probe(struct platform_device *pdev)
>>>
>>> dev_info(&pdev->dev, "initializing on CS%d, phys base 0x%08lx, virtual "
>>> "base %p, freq %d MHz\n", c->gpmc_cs, c->phys_base,
>>> - c->onenand.base, c->freq);
>>> + c->onenand.base, freq);
>>>
>>> c->pdev = pdev;
>>> c->mtd.priv = &c->onenand;
>>>
>>> c->mtd.dev.parent = &pdev->dev;
>>> - mtd_set_of_node(&c->mtd, pdata->of_node);
>>> + mtd_set_of_node(&c->mtd, pdev->dev.of_node);
>>>
>>> this = &c->onenand;
>>> if (c->dma_channel >= 0) {
>>> this->wait = omap2_onenand_wait;
>>> - if (c->flags & ONENAND_IN_OMAP34XX) {
>>> - this->read_bufferram = omap3_onenand_read_bufferram;
>>> - this->write_bufferram = omap3_onenand_write_bufferram;
>>> - } else {
>>> + if (c->is_omap2) {
>>> this->read_bufferram = omap2_onenand_read_bufferram;
>>> this->write_bufferram = omap2_onenand_write_bufferram;
>>> + } else {
>>> + this->read_bufferram = omap3_onenand_read_bufferram;
>>> + this->write_bufferram = omap3_onenand_write_bufferram;
>>> }
>>> }
>>>
>>> - if (pdata->regulator_can_sleep) {
>>> + if (0 /* TODO: pdata->regulator_can_sleep */) {
>>> c->regulator = regulator_get(&pdev->dev, "vonenand");
>>> if (IS_ERR(c->regulator)) {
>>> dev_err(&pdev->dev, "Failed to get regulator\n");
>>> @@ -737,14 +805,13 @@ static int omap2_onenand_probe(struct platform_device *pdev)
>>> c->onenand.disable = omap2_onenand_disable;
>>> }
>>>
>>> - if (pdata->skip_initial_unlocking)
>>> + if (0 /* TODO: pdata->skip_initial_unlocking */)
>>> this->options |= ONENAND_SKIP_INITIAL_UNLOCKING;
>>>
>>> if ((r = onenand_scan(&c->mtd, 1)) < 0)
>>> goto err_release_regulator;
>>>
>>> - r = mtd_device_register(&c->mtd, pdata ? pdata->parts : NULL,
>>> - pdata ? pdata->nr_parts : 0);
>>> + r = mtd_device_register(&c->mtd, NULL, 0);
>>> if (r)
>>> goto err_release_onenand;
>>>
>>> @@ -794,12 +861,20 @@ static int omap2_onenand_remove(struct platform_device *pdev)
>>> return 0;
>>> }
>>>
>>> +static const struct of_device_id omap2_onenand_ids[] = {
>>> + { .compatible = "ti,omap2-onenand", .data = (void *)true, },
>>> + { .compatible = "ti,omap3-onenand", .data = (void *)false, },
>>
>> This doesn't clearly show what we're doing.
>>
>> Can we instead have a proper data structure for .data?
>> If that is an overkill maybe we should avoid the of_data approach and instead do
>
> I'll go for overkill in this case.
>
>> if (of_device_is_compatible(node, "ti,omap3-nand")
>> c->need_wait_delay = true;
>>
>>> + { },
>>> +};
>>> +MODULE_DEVICE_TABLE(of, omap2_onenand_ids);
>>> +
>>> static struct platform_driver omap2_onenand_driver = {
>>> .probe = omap2_onenand_probe,
>>> .remove = omap2_onenand_remove,
>>> .shutdown = omap2_onenand_shutdown,
>>> .driver = {
>>> .name = DRIVER_NAME,
>>> + .of_match_table = of_match_ptr(omap2_onenand_ids),
>>> },
>>> };
>>>
>>>
>>
>> We need to update the DT binding doc as well
>> devicetree/bindings/mtd$ vi gpmc-onenand.txt
>>
>> This should be done in a separate patch.
>>
--
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
