[PATCH 00/12] Introduce davinci AEMIF driver

[PATCH 00/12] Introduce davinci AEMIF driver

[Khoronzhuk, Ivan]

These patches introduce AEMIF driver for davinci/keystone archs
and defines AEMIF and NAND devices in keystone DT.
Also some changes added to davinci NAND driver in order to reuse it
for Keystone arch.

The patches can be spitted to several series if needed, like

12      - add AEMIF/NAND device entry in DT
arm: dts: keystone: add AEMIF/NAND device entry

9-11    - reuse davinci-nand driver for keystone arch
mtd: nand: davinci: don't request AEMIF address range
mtd: nand: davinci: don't set timings if AEMIF is used
mtd: nand: davinci: reuse driver for Keystone arch

7-8     - introduce AEMIF driver
memory: davinci-aemif: add bindings for AEMIF driver
memory: davinci-aemif: introduce AEMIF driver

4-6     - move davinci-nand bindings under mtd
mtd: nand: davinci: adjust DT properties to MTD generic
mtd: nand: davinci: extend description of bindings
mtd: nand: davinci: move bindings under mtd

1-3     - separate nand fixes
mtd: nand: davinci: simplify error handling
mtd: nand: davinci: check required ti,davinci-chipselect property
mtd: nand: davinci: fix driver registration

See Documentation:
Davinci DM646x - http://www.ti.com/lit/ug/sprueq7c/sprueq7c.pdf
OMAP-L138 - http://www.ti.com/lit/ug/spruh77a/spruh77a.pdf
Kestone - http://www.ti.com/lit/ug/sprugz3a/sprugz3a.pdf

Test:
# flash_erase /dev/mtd2 0 0

Erasing 128 Kibyte @ 7de0000 -- 99 % complete flash_erase: Skipping bad block at
07e00000
flash_erase: Skipping bad block at 07e20000
flash_erase: Skipping bad block at 07e40000
flash_erase: Skipping bad block at 07e60000
Erasing 128 Kibyte @ 7e60000 -- 100 % complete

# nandtest -k -p 3 /dev/mtd2
ECC corrections: 0
ECC failures   : 0
Bad blocks     : 4
BBT blocks     : 0
Bad block at 0x07e00000
Bad block at 0x07e20000
Bad block at 0x07e40000
Bad block at 0x07e60000

Finished pass 1 successfully
Bad block at 0x07e00000F44864
Bad block at 0x07e20000
Bad block at 0x07e40000
Bad block at 0x07e60000

Finished pass 2 successfully
Bad block at 0x07e00000
Bad block at 0x07e20000
Bad block at 0x07e40000
Bad block at 0x07e60000

Finished pass 3 successfully

Based on
git://git.kernel.org/pub/scm/linux/kernel/git/ssantosh/linux-keystone.git
keystone/master

Ivan Khoronzhuk (12):
  mtd: nand: davinci: fix driver registration
  mtd: nand: davinci: check required ti,davinci-chipselect property
  mtd: nand: davinci: simplify error handling
  mtd: nand: davinci: move bindings under mtd
  mtd: nand: davinci: extend description of bindings
  mtd: nand: davinci: adjust DT properties to MTD generic
  memory: davinci-aemif: introduce AEMIF driver
  memory: davinci-aemif: add bindings for AEMIF driver
  mtd: nand: davinci: reuse driver for Keystone arch
  mtd: nand: davinci: don't set timings if AEMIF is used
  mtd: nand: davinci: don't request AEMIF address range
  arm: dts: keystone: add AEMIF/NAND device entry

 .../devicetree/bindings/arm/davinci/nand.txt       |   46 ---
 .../bindings/memory-controllers/davinci-aemif.txt  |  198 ++++++++++
 .../devicetree/bindings/mtd/davinci-nand.txt       |   94 +++++
 arch/arm/boot/dts/keystone.dts                     |   63 +++
 drivers/memory/Kconfig                             |   11 +
 drivers/memory/Makefile                            |    1 +
 drivers/memory/davinci-aemif.c                     |  415 ++++++++++++++++++++
 drivers/mtd/nand/Kconfig                           |    6 +-
 drivers/mtd/nand/davinci_nand.c                    |   95 +++--
 9 files changed, 839 insertions(+), 90 deletions(-)
 delete mode 100644 Documentation/devicetree/bindings/arm/davinci/nand.txt
 create mode 100644 Documentation/devicetree/bindings/memory-controllers/davinci-aemif.txt
 create mode 100644 Documentation/devicetree/bindings/mtd/davinci-nand.txt
 create mode 100644 drivers/memory/davinci-aemif.c

--
1.7.9.5

[PATCH 01/12] mtd: nand: davinci: fix driver registration

[Khoronzhuk, Ivan]

When kernel is booted using DT, there is no guarantee that Davinci
NAND device has been created already at the time when driver init
function is executed. Therefore, platform_driver_probe() can't be used
because this may result the Davinci NAND driver will never be probed.
The driver probing has to be made with core mechanism.

Signed-off-by: Ivan Khoronzhuk <ivan.khoronzhuk@ti.com>
---
 drivers/mtd/nand/davinci_nand.c |    3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/mtd/nand/davinci_nand.c b/drivers/mtd/nand/davinci_nand.c
index b77a01e..d87213f 100644
--- a/drivers/mtd/nand/davinci_nand.c
+++ b/drivers/mtd/nand/davinci_nand.c
@@ -877,6 +877,7 @@ static int __exit nand_davinci_remove(struct platform_device *pdev)
 }

 static struct platform_driver nand_davinci_driver = {
+       .probe          = nand_davinci_probe,
        .remove         = __exit_p(nand_davinci_remove),
        .driver         = {
                .name   = "davinci_nand",
@@ -886,7 +887,7 @@ static struct platform_driver nand_davinci_driver = {
 };
 MODULE_ALIAS("platform:davinci_nand");

-module_platform_driver_probe(nand_davinci_driver, nand_davinci_probe);
+module_platform_driver(nand_davinci_driver);

 MODULE_LICENSE("GPL");
 MODULE_AUTHOR("Texas Instruments");
--
1.7.9.5

Re: [PATCH 01/12] mtd: nand: davinci: fix driver registration

[Santosh Shilimkar]

On Monday 11 November 2013 11:52 AM, Khoronzhuk, Ivan wrote:
> 
> When kernel is booted using DT, there is no guarantee that Davinci
> NAND device has been created already at the time when driver init
> function is executed. Therefore, platform_driver_probe() can't be used
> because this may result the Davinci NAND driver will never be probed.
> The driver probing has to be made with core mechanism.
> 
> Signed-off-by: Ivan Khoronzhuk <ivan.khoronzhuk@ti.com>
> ---
Acked-by: Santosh Shilimkar <santosh.shilimkar@ti.com>

[PATCH 02/12] mtd: nand: davinci: check required ti,davinci-chipselect property

[Khoronzhuk, Ivan]

The property "ti,davinci-chipselect" is required. So we have to check
if it is set.

Signed-off-by: Ivan Khoronzhuk <ivan.khoronzhuk@ti.com>
---
 drivers/mtd/nand/davinci_nand.c |   10 +++++++++-
 1 file changed, 9 insertions(+), 1 deletion(-)

diff --git a/drivers/mtd/nand/davinci_nand.c b/drivers/mtd/nand/davinci_nand.c
index d87213f..8e1c88e 100644
--- a/drivers/mtd/nand/davinci_nand.c
+++ b/drivers/mtd/nand/davinci_nand.c
@@ -541,10 +541,14 @@ static struct davinci_nand_pdata
                                GFP_KERNEL);
                pdev->dev.platform_data = pdata;
                if (!pdata)
-                       return NULL;
+                       return ERR_PTR(-ENOMEM);
+
                if (!of_property_read_u32(pdev->dev.of_node,
                        "ti,davinci-chipselect", &prop))
                        pdev->id = prop;
+               else
+                       return ERR_PTR(-EINVAL);
+
                if (!of_property_read_u32(pdev->dev.of_node,
                        "ti,davinci-mask-ale", &prop))
                        pdata->mask_ale = prop;
@@ -598,6 +602,10 @@ static int __init nand_davinci_probe(struct platform_device *pdev)
        nand_ecc_modes_t                ecc_mode;

        pdata = nand_davinci_get_pdata(pdev);
+       if (IS_ERR(pdata)) {
+               return PTR_ERR(pdata);
+       }
+
        /* insist on board-specific configuration */
        if (!pdata)
                return -ENODEV;
--
1.7.9.5

Re: [PATCH 02/12] mtd: nand: davinci: check required ti, davinci-chipselect property

[Santosh Shilimkar]

On Monday 11 November 2013 11:53 AM, Khoronzhuk, Ivan wrote:
> The property "ti,davinci-chipselect" is required. So we have to check
> if it is set.
> 
> Signed-off-by: Ivan Khoronzhuk <ivan.khoronzhuk@ti.com>
> ---
>  drivers/mtd/nand/davinci_nand.c |   10 +++++++++-
>  1 file changed, 9 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/mtd/nand/davinci_nand.c b/drivers/mtd/nand/davinci_nand.c
> index d87213f..8e1c88e 100644
> --- a/drivers/mtd/nand/davinci_nand.c
> +++ b/drivers/mtd/nand/davinci_nand.c
> @@ -541,10 +541,14 @@ static struct davinci_nand_pdata
>                                 GFP_KERNEL);
>                 pdev->dev.platform_data = pdata;
>                 if (!pdata)
> -                       return NULL;
> +                       return ERR_PTR(-ENOMEM);
> +
This change don't follow commit message.

>                 if (!of_property_read_u32(pdev->dev.of_node,
>                         "ti,davinci-chipselect", &prop))
>                         pdev->id = prop;
> +               else
> +                       return ERR_PTR(-EINVAL);
> +
So the check already exist but the error case wasn't handled.
This should be reflected in change log.

>                 if (!of_property_read_u32(pdev->dev.of_node,
>                         "ti,davinci-mask-ale", &prop))
>                         pdata->mask_ale = prop;
> @@ -598,6 +602,10 @@ static int __init nand_davinci_probe(struct platform_device *pdev)
>         nand_ecc_modes_t                ecc_mode;
> 
>         pdata = nand_davinci_get_pdata(pdev);
> +       if (IS_ERR(pdata)) {
> +               return PTR_ERR(pdata);
> +       }
> +
Again not related to commit log. You might want to split this patch
then.

Regards,
Santosh

Re: [PATCH 02/12] mtd: nand: davinci: check required ti, davinci-chipselect property

[ivan.khoronzhuk]

On 11/12/2013 05:50 PM, Santosh Shilimkar wrote:
> On Monday 11 November 2013 11:53 AM, Khoronzhuk, Ivan wrote:
>> The property "ti,davinci-chipselect" is required. So we have to check
>> if it is set.
>>
>> Signed-off-by: Ivan Khoronzhuk <ivan.khoronzhuk@ti.com>
>> ---
>>   drivers/mtd/nand/davinci_nand.c |   10 +++++++++-
>>   1 file changed, 9 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/mtd/nand/davinci_nand.c b/drivers/mtd/nand/davinci_nand.c
>> index d87213f..8e1c88e 100644
>> --- a/drivers/mtd/nand/davinci_nand.c
>> +++ b/drivers/mtd/nand/davinci_nand.c
>> @@ -541,10 +541,14 @@ static struct davinci_nand_pdata
>>                                  GFP_KERNEL);
>>                  pdev->dev.platform_data = pdata;
>>                  if (!pdata)
>> -                       return NULL;
>> +                       return ERR_PTR(-ENOMEM);
>> +
> This change don't follow commit message.
>

I'll move it to separate patch, that will be first patch

>>                  if (!of_property_read_u32(pdev->dev.of_node,
>>                          "ti,davinci-chipselect", &prop))
>>                          pdev->id = prop;
>> +               else
>> +                       return ERR_PTR(-EINVAL);
>> +
> So the check already exist but the error case wasn't handled.
> This should be reflected in change log.
>

The error will be handled at return by:
	pdata = nand_davinci_get_pdata(pdev);
  	if (IS_ERR(pdata))
		return PTR_ERR(pdata);

or did you mean I should add dev_err().

>>                  if (!of_property_read_u32(pdev->dev.of_node,
>>                          "ti,davinci-mask-ale", &prop))
>>                          pdata->mask_ale = prop;
>> @@ -598,6 +602,10 @@ static int __init nand_davinci_probe(struct platform_device *pdev)
>>          nand_ecc_modes_t                ecc_mode;
>>
>>          pdata = nand_davinci_get_pdata(pdev);
>> +       if (IS_ERR(pdata)) {
>> +               return PTR_ERR(pdata);
>> +       }
>> +
> Again not related to commit log. You might want to split this patch
> then.
>

I'll move it in the same separate patch.

> Regards,
> Santosh
>


-- 
Regards,
Ivan Khoronzhuk

[PATCH 03/12] mtd: nand: davinci: simplify error handling

[Khoronzhuk, Ivan]

There is not needed to use a lot of names for err handling.
It complicates code support and reading.

Signed-off-by: Ivan Khoronzhuk <ivan.khoronzhuk@ti.com>
---
 drivers/mtd/nand/davinci_nand.c |   46 +++++++++++++++------------------------
 1 file changed, 17 insertions(+), 29 deletions(-)

diff --git a/drivers/mtd/nand/davinci_nand.c b/drivers/mtd/nand/davinci_nand.c
index 8e1c88e..9bcbaa9 100644
--- a/drivers/mtd/nand/davinci_nand.c
+++ b/drivers/mtd/nand/davinci_nand.c
@@ -617,8 +617,7 @@ static int __init nand_davinci_probe(struct platform_device *pdev)
        info = devm_kzalloc(&pdev->dev, sizeof(*info), GFP_KERNEL);
        if (!info) {
                dev_err(&pdev->dev, "unable to allocate memory\n");
-               ret = -ENOMEM;
-               goto err_nomem;
+               return -ENOMEM;
        }

        platform_set_drvdata(pdev, info);
@@ -627,20 +626,16 @@ static int __init nand_davinci_probe(struct platform_device *pdev)
        res2 = platform_get_resource(pdev, IORESOURCE_MEM, 1);
        if (!res1 || !res2) {
                dev_err(&pdev->dev, "resource missing\n");
-               ret = -EINVAL;
-               goto err_nomem;
+               return -EINVAL;
        }

        vaddr = devm_ioremap_resource(&pdev->dev, res1);
-       if (IS_ERR(vaddr)) {
-               ret = PTR_ERR(vaddr);
-               goto err_ioremap;
-       }
+       if (IS_ERR(vaddr))
+               return PTR_ERR(vaddr);
+
        base = devm_ioremap_resource(&pdev->dev, res2);
-       if (IS_ERR(base)) {
-               ret = PTR_ERR(base);
-               goto err_ioremap;
-       }
+       if (IS_ERR(base))
+               return PTR_ERR(base);

        info->dev               = &pdev->dev;
        info->base              = base;
@@ -707,7 +702,7 @@ static int __init nand_davinci_probe(struct platform_device *pdev)
                        spin_unlock_irq(&davinci_nand_lock);

                        if (ret == -EBUSY)
-                               goto err_ecc;
+                               return ret;

                        info->chip.ecc.calculate = nand_davinci_calculate_4bit;
                        info->chip.ecc.correct = nand_davinci_correct_4bit;
@@ -723,8 +718,7 @@ static int __init nand_davinci_probe(struct platform_device *pdev)
                info->chip.ecc.strength = pdata->ecc_bits;
                break;
        default:
-               ret = -EINVAL;
-               goto err_ecc;
+               return -EINVAL;
        }
        info->chip.ecc.mode = ecc_mode;

@@ -732,7 +726,7 @@ static int __init nand_davinci_probe(struct platform_device *pdev)
        if (IS_ERR(info->clk)) {
                ret = PTR_ERR(info->clk);
                dev_dbg(&pdev->dev, "unable to get AEMIF clock, err %d\n", ret);
-               goto err_clk;
+               return ret;
        }

        ret = clk_prepare_enable(info->clk);
@@ -761,7 +755,7 @@ static int __init nand_davinci_probe(struct platform_device *pdev)
                                                        info->core_chipsel);
        if (ret < 0) {
                dev_dbg(&pdev->dev, "NAND timing values setup fail\n");
-               goto err_timing;
+               goto err;
        }

        spin_lock_irq(&davinci_nand_lock);
@@ -777,7 +771,7 @@ static int __init nand_davinci_probe(struct platform_device *pdev)
        ret = nand_scan_ident(&info->mtd, pdata->mask_chipsel ? 2 : 1, NULL);
        if (ret < 0) {
                dev_dbg(&pdev->dev, "no NAND chip(s) found\n");
-               goto err_scan;
+               goto err;
        }

        /* Update ECC layout if needed ... for 1-bit HW ECC, the default
@@ -791,7 +785,7 @@ static int __init nand_davinci_probe(struct platform_device *pdev)
                if (!chunks || info->mtd.oobsize < 16) {
                        dev_dbg(&pdev->dev, "too small\n");
                        ret = -EINVAL;
-                       goto err_scan;
+                       goto err;
                }

                /* For small page chips, preserve the manufacturer's
@@ -822,7 +816,7 @@ static int __init nand_davinci_probe(struct platform_device *pdev)
                dev_warn(&pdev->dev, "no 4-bit ECC support yet "
                                "for 4KiB-page NAND\n");
                ret = -EIO;
-               goto err_scan;
+               goto err;

 syndrome_done:
                info->chip.ecc.layout = &info->ecclayout;
@@ -830,7 +824,7 @@ syndrome_done:

        ret = nand_scan_tail(&info->mtd);
        if (ret < 0)
-               goto err_scan;
+               goto err;

        if (pdata->parts)
                ret = mtd_device_parse_register(&info->mtd, NULL, NULL,
@@ -843,7 +837,7 @@ syndrome_done:
                                                NULL, 0);
        }
        if (ret < 0)
-               goto err_scan;
+               goto err;

        val = davinci_nand_readl(info, NRCSR_OFFSET);
        dev_info(&pdev->dev, "controller rev. %d.%d\n",
@@ -851,8 +845,7 @@ syndrome_done:

        return 0;

-err_scan:
-err_timing:
+err:
        clk_disable_unprepare(info->clk);

 err_clk_enable:
@@ -860,11 +853,6 @@ err_clk_enable:
        if (ecc_mode == NAND_ECC_HW_SYNDROME)
                ecc4_busy = false;
        spin_unlock_irq(&davinci_nand_lock);
-
-err_ecc:
-err_clk:
-err_ioremap:
-err_nomem:
        return ret;
 }

--
1.7.9.5

Re: [PATCH 03/12] mtd: nand: davinci: simplify error handling

[Santosh Shilimkar]

On Monday 11 November 2013 11:54 AM, Khoronzhuk, Ivan wrote:
> There is not needed to use a lot of names for err handling.
> It complicates code support and reading.
> 
This is not always true but looking at the patch, the labels
are just useless since no special handling per label.

> Signed-off-by: Ivan Khoronzhuk <ivan.khoronzhuk@ti.com>
> ---
Acked-by: Santosh Shilimkar <santosh.shilimkar@ti.com>

[PATCH 04/12] mtd: nand: davinci: move bindings under mtd

[Khoronzhuk, Ivan]

Move bindings under mtd. Do this in order to make davinci-nand
driver usable by keystone architecture.

Signed-off-by: Ivan Khoronzhuk <ivan.khoronzhuk@ti.com>
---
 .../{arm/davinci/nand.txt => mtd/davinci-nand.txt} |    0
 1 file changed, 0 insertions(+), 0 deletions(-)
 rename Documentation/devicetree/bindings/{arm/davinci/nand.txt => mtd/davinci-nand.txt} (100%)

diff --git a/Documentation/devicetree/bindings/arm/davinci/nand.txt b/Documentation/devicetree/bindings/mtd/davinci-nand.txt
similarity index 100%
rename from Documentation/devicetree/bindings/arm/davinci/nand.txt
rename to Documentation/devicetree/bindings/mtd/davinci-nand.txt
--
1.7.9.5

Re: [PATCH 04/12] mtd: nand: davinci: move bindings under mtd

[Santosh Shilimkar]

On Monday 11 November 2013 11:55 AM, Khoronzhuk, Ivan wrote:
> Move bindings under mtd. Do this in order to make davinci-nand
> driver usable by keystone architecture.
> 
> Signed-off-by: Ivan Khoronzhuk <ivan.khoronzhuk@ti.com>
> ---
Acked-by: Santosh Shilimkar <santosh.shilimkar@ti.com>

[PATCH 05/12] mtd: nand: davinci: extend description of bindings

[Khoronzhuk, Ivan]

Extend bindings for davinci_nand driver to be more clear.

Signed-off-by: Ivan Khoronzhuk <ivan.khoronzhuk@ti.com>
---
 .../devicetree/bindings/mtd/davinci-nand.txt       |   77 ++++++++++++++------
 1 file changed, 54 insertions(+), 23 deletions(-)

diff --git a/Documentation/devicetree/bindings/mtd/davinci-nand.txt b/Documentation/devicetree/bindings/mtd/davinci-nand.txt
index 3545ea7..d2a3fc0 100644
--- a/Documentation/devicetree/bindings/mtd/davinci-nand.txt
+++ b/Documentation/devicetree/bindings/mtd/davinci-nand.txt
@@ -1,36 +1,67 @@
-* Texas Instruments Davinci NAND
+Device tree bindings for Texas instruments Davinci NAND controller

-This file provides information, what the device node for the
-davinci nand interface contain.
+This file provides information, what the device node for the davinci NAND
+interface contains.
+
+Documentation:
+Davinci DM646x - http://www.ti.com/lit/ug/sprueq7c/sprueq7c.pdf

 Required properties:
-- compatible: "ti,davinci-nand";
-- reg : contain 2 offset/length values:
-        - offset and length for the access window
-        - offset and length for accessing the aemif control registers
-- ti,davinci-chipselect: Indicates on the davinci_nand driver which
-                         chipselect is used for accessing the nand.
+
+- compatible:                  "ti,davinci-nand"
+
+- reg:                         Contains 2 offset/length values:
+                               - offset and length for the access window.
+                               - offset and length for accessing the AEMIF
+                               control registers.
+
+- ti,davinci-chipselect:       number of chipselect. Indicates on the
+                               davinci_nand driver which chipselect is used
+                               for accessing the nand.
+                               Can be in the range [0-3].

 Recommended properties :
-- ti,davinci-mask-ale: mask for ale
-- ti,davinci-mask-cle: mask for cle
-- ti,davinci-mask-chipsel: mask for chipselect
-- ti,davinci-ecc-mode: ECC mode valid values for davinci driver:
-               - "none"
-               - "soft"
-               - "hw"
-- ti,davinci-ecc-bits: used ECC bits, currently supported 1 or 4.
-- ti,davinci-nand-buswidth: buswidth 8 or 16
-- ti,davinci-nand-use-bbt: use flash based bad block table support.
-
-nand device bindings may contain additional sub-nodes describing
-partitions of the address space. See partition.txt for more detail.
+
+- ti,davinci-mask-ale:         mask for ALE. Needed for executing address
+                               phase. These offset will be added to the base
+                               address for the chip select space the NAND Flash
+                               device is connected to.
+                               If not set equal to 0x08.
+
+- ti,davinci-mask-cle:         mask for CLE. Needed for executing command
+                               phase. These offset will be added to the base
+                               address for the chip select space the NAND Flash
+                               device is connected to.
+                               If not set equal to 0x10.
+
+- ti,davinci-mask-chipsel:     mask for chipselect address. Needed to mask
+                               addresses for given chipselect.
+
+- ti,davinci-ecc-mode:         operation mode of the NAND ecc mode. ECC mode
+                               valid values for davinci driver:
+                               - "none"
+                               - "soft"
+                               - "hw"
+
+- ti,davinci-ecc-bits:         used ECC bits, currently supported 1 or 4.
+
+- ti,davinci-nand-buswidth:    buswidth 8 or 16.
+
+- ti,davinci-nand-use-bbt:     use flash based bad block table support. OOB
+                               identifier is saved in OOB area.
+
+Nand device bindings may contain additional sub-nodes describing partitions of
+the address space. See partition.txt for more detail. The NAND Flash timing
+values must be programmed in the chip select’s node of AEMIF
+memory-controller (see Documentation/devicetree/bindings/memory-controllers/
+davinci-aemif.txt).

 Example(da850 EVM ):
+
 nand_cs3@62000000 {
        compatible = "ti,davinci-nand";
        reg = <0x62000000 0x807ff
-               0x68000000 0x8000>;
+              0x68000000 0x8000>;
        ti,davinci-chipselect = <1>;
        ti,davinci-mask-ale = <0>;
        ti,davinci-mask-cle = <0>;
--
1.7.9.5

Re: [PATCH 05/12] mtd: nand: davinci: extend description of bindings

[Santosh Shilimkar]

On Monday 11 November 2013 11:58 AM, Khoronzhuk, Ivan wrote:
> Extend bindings for davinci_nand driver to be more clear.
> 
> Signed-off-by: Ivan Khoronzhuk <ivan.khoronzhuk@ti.com>
> ---
Looks fine to me but it needs blessing from DT guys. 

[PATCH 06/12] mtd: nand: davinci: adjust DT properties to MTD generic

[Khoronzhuk, Ivan]

The properties davinci-ecc-mode, davinci-nand-use-bbt, davinci-nand-buswidth
are MTD generic. Correct names for them are: nand-ecc-mode, nand-on-flash-bbt,
nand-bus-width accordingly. So rename them in dts and documentation.

Signed-off-by: Ivan Khoronzhuk <ivan.khoronzhuk@ti.com>
---
 .../devicetree/bindings/mtd/davinci-nand.txt       |   25 ++++++++++++++++----
 drivers/mtd/nand/davinci_nand.c                    |   11 ++++++---
 2 files changed, 28 insertions(+), 8 deletions(-)

diff --git a/Documentation/devicetree/bindings/mtd/davinci-nand.txt b/Documentation/devicetree/bindings/mtd/davinci-nand.txt
index d2a3fc0..befaa5b 100644
--- a/Documentation/devicetree/bindings/mtd/davinci-nand.txt
+++ b/Documentation/devicetree/bindings/mtd/davinci-nand.txt
@@ -37,7 +37,7 @@ Recommended properties :
 - ti,davinci-mask-chipsel:     mask for chipselect address. Needed to mask
                                addresses for given chipselect.

-- ti,davinci-ecc-mode:         operation mode of the NAND ecc mode. ECC mode
+- nand-ecc-mode:               operation mode of the NAND ecc mode. ECC mode
                                valid values for davinci driver:
                                - "none"
                                - "soft"
@@ -45,10 +45,25 @@ Recommended properties :

 - ti,davinci-ecc-bits:         used ECC bits, currently supported 1 or 4.

-- ti,davinci-nand-buswidth:    buswidth 8 or 16.
+- nand-bus-width:              buswidth 8 or 16. If not present 8.
+
+- nand-on-flash-bbt:           use flash based bad block table support. OOB
+                               identifier is saved in OOB area. If not present
+                               false.
+
+Deprecated properties:
+
+- ti,davinci-ecc-mode:         operation mode of the NAND ecc mode. ECC mode
+                               valid values for davinci driver:
+                               - "none"
+                               - "soft"
+                               - "hw"
+
+- ti,davinci-nand-buswidth:    buswidth 8 or 16. If not present 8.

 - ti,davinci-nand-use-bbt:     use flash based bad block table support. OOB
-                               identifier is saved in OOB area.
+                               identifier is saved in OOB area. If not present
+                               false.

 Nand device bindings may contain additional sub-nodes describing partitions of
 the address space. See partition.txt for more detail. The NAND Flash timing
@@ -66,9 +81,9 @@ nand_cs3@62000000 {
        ti,davinci-mask-ale = <0>;
        ti,davinci-mask-cle = <0>;
        ti,davinci-mask-chipsel = <0>;
-       ti,davinci-ecc-mode = "hw";
+       nand-ecc-mode = "hw";
        ti,davinci-ecc-bits = <4>;
-       ti,davinci-nand-use-bbt;
+       nand-on-flash-bbt;

        partition@180000 {
                label = "ubifs";
diff --git a/drivers/mtd/nand/davinci_nand.c b/drivers/mtd/nand/davinci_nand.c
index 9bcbaa9..78b8d66 100644
--- a/drivers/mtd/nand/davinci_nand.c
+++ b/drivers/mtd/nand/davinci_nand.c
@@ -534,7 +534,6 @@ static struct davinci_nand_pdata
                struct davinci_nand_pdata *pdata;
                const char *mode;
                u32 prop;
-               int len;

                pdata =  devm_kzalloc(&pdev->dev,
                                sizeof(struct davinci_nand_pdata),
@@ -559,6 +558,8 @@ static struct davinci_nand_pdata
                        "ti,davinci-mask-chipsel", &prop))
                        pdata->mask_chipsel = prop;
                if (!of_property_read_string(pdev->dev.of_node,
+                       "nand-ecc-mode", &mode) ||
+                   !of_property_read_string(pdev->dev.of_node,
                        "ti,davinci-ecc-mode", &mode)) {
                        if (!strncmp("none", mode, 4))
                                pdata->ecc_mode = NAND_ECC_NONE;
@@ -571,11 +572,15 @@ static struct davinci_nand_pdata
                        "ti,davinci-ecc-bits", &prop))
                        pdata->ecc_bits = prop;
                if (!of_property_read_u32(pdev->dev.of_node,
+                       "nand-bus-width", &prop) ||
+                   !of_property_read_u32(pdev->dev.of_node,
                        "ti,davinci-nand-buswidth", &prop))
                        if (prop == 16)
                                pdata->options |= NAND_BUSWIDTH_16;
-               if (of_find_property(pdev->dev.of_node,
-                       "ti,davinci-nand-use-bbt", &len))
+               if (of_property_read_bool(pdev->dev.of_node,
+                       "nand-on-flash-bbt") ||
+                   of_property_read_bool(pdev->dev.of_node,
+                       "ti,davinci-nand-use-bbt"))
                        pdata->bbt_options = NAND_BBT_USE_FLASH;
        }

--
1.7.9.5

Re: [PATCH 06/12] mtd: nand: davinci: adjust DT properties to MTD generic

[Sekhar Nori]

On Monday 11 November 2013 10:31 PM, Khoronzhuk, Ivan wrote:
> The properties davinci-ecc-mode, davinci-nand-use-bbt, davinci-nand-buswidth
> are MTD generic. Correct names for them are: nand-ecc-mode, nand-on-flash-bbt,
> nand-bus-width accordingly. So rename them in dts and documentation.
> 
> Signed-off-by: Ivan Khoronzhuk <ivan.khoronzhuk@ti.com>
> ---
>  .../devicetree/bindings/mtd/davinci-nand.txt       |   25 ++++++++++++++++----
>  drivers/mtd/nand/davinci_nand.c                    |   11 ++++++---
>  2 files changed, 28 insertions(+), 8 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/mtd/davinci-nand.txt b/Documentation/devicetree/bindings/mtd/davinci-nand.txt
> index d2a3fc0..befaa5b 100644
> --- a/Documentation/devicetree/bindings/mtd/davinci-nand.txt
> +++ b/Documentation/devicetree/bindings/mtd/davinci-nand.txt
> @@ -37,7 +37,7 @@ Recommended properties :
>  - ti,davinci-mask-chipsel:     mask for chipselect address. Needed to mask
>                                 addresses for given chipselect.
> 
> -- ti,davinci-ecc-mode:         operation mode of the NAND ecc mode. ECC mode
> +- nand-ecc-mode:               operation mode of the NAND ecc mode. ECC mode
>                                 valid values for davinci driver:
>                                 - "none"
>                                 - "soft"
> @@ -45,10 +45,25 @@ Recommended properties :
> 
>  - ti,davinci-ecc-bits:         used ECC bits, currently supported 1 or 4.
> 
> -- ti,davinci-nand-buswidth:    buswidth 8 or 16.

Shouldn't we keep the documentation for existing bindings while just
marking them as deprecated?

Thanks,
Sekhar

Re: [PATCH 06/12] mtd: nand: davinci: adjust DT properties to MTD generic

[Grygorii Strashko]

On 11/26/2013 09:03 AM, Sekhar Nori wrote:
> On Monday 11 November 2013 10:31 PM, Khoronzhuk, Ivan wrote:
>> The properties davinci-ecc-mode, davinci-nand-use-bbt, davinci-nand-buswidth
>> are MTD generic. Correct names for them are: nand-ecc-mode, nand-on-flash-bbt,
>> nand-bus-width accordingly. So rename them in dts and documentation.
>>
>> Signed-off-by: Ivan Khoronzhuk <ivan.khoronzhuk@ti.com>
>> ---
>>   .../devicetree/bindings/mtd/davinci-nand.txt       |   25 ++++++++++++++++----
>>   drivers/mtd/nand/davinci_nand.c                    |   11 ++++++---
>>   2 files changed, 28 insertions(+), 8 deletions(-)
>>
>> diff --git a/Documentation/devicetree/bindings/mtd/davinci-nand.txt b/Documentation/devicetree/bindings/mtd/davinci-nand.txt
>> index d2a3fc0..befaa5b 100644
>> --- a/Documentation/devicetree/bindings/mtd/davinci-nand.txt
>> +++ b/Documentation/devicetree/bindings/mtd/davinci-nand.txt
>> @@ -37,7 +37,7 @@ Recommended properties :
>>   - ti,davinci-mask-chipsel:     mask for chipselect address. Needed to mask
>>                                  addresses for given chipselect.
>>
>> -- ti,davinci-ecc-mode:         operation mode of the NAND ecc mode. ECC mode
>> +- nand-ecc-mode:               operation mode of the NAND ecc mode. ECC mode
>>                                  valid values for davinci driver:
>>                                  - "none"
>>                                  - "soft"
>> @@ -45,10 +45,25 @@ Recommended properties :
>>
>>   - ti,davinci-ecc-bits:         used ECC bits, currently supported 1 or 4.
>>
>> -- ti,davinci-nand-buswidth:    buswidth 8 or 16.
> 
> Shouldn't we keep the documentation for existing bindings while just
> marking them as deprecated?

I haven't found any requirements about this in doc. 
It's done in both way in other drivers - some keep properties in bindings
documentation and some are not.

Regards,
-grygorii

Re: [PATCH 06/12] mtd: nand: davinci: adjust DT properties to MTD generic

[ivan.khoronzhuk]

On 11/26/2013 12:30 PM, Grygorii Strashko wrote:
> On 11/26/2013 09:03 AM, Sekhar Nori wrote:
>> On Monday 11 November 2013 10:31 PM, Khoronzhuk, Ivan wrote:
>>> The properties davinci-ecc-mode, davinci-nand-use-bbt, davinci-nand-buswidth
>>> are MTD generic. Correct names for them are: nand-ecc-mode, nand-on-flash-bbt,
>>> nand-bus-width accordingly. So rename them in dts and documentation.
>>>
>>> Signed-off-by: Ivan Khoronzhuk <ivan.khoronzhuk@ti.com>
>>> ---
>>>    .../devicetree/bindings/mtd/davinci-nand.txt       |   25 ++++++++++++++++----
>>>    drivers/mtd/nand/davinci_nand.c                    |   11 ++++++---
>>>    2 files changed, 28 insertions(+), 8 deletions(-)
>>>
>>> diff --git a/Documentation/devicetree/bindings/mtd/davinci-nand.txt b/Documentation/devicetree/bindings/mtd/davinci-nand.txt
>>> index d2a3fc0..befaa5b 100644
>>> --- a/Documentation/devicetree/bindings/mtd/davinci-nand.txt
>>> +++ b/Documentation/devicetree/bindings/mtd/davinci-nand.txt
>>> @@ -37,7 +37,7 @@ Recommended properties :
>>>    - ti,davinci-mask-chipsel:     mask for chipselect address. Needed to mask
>>>                                   addresses for given chipselect.
>>>
>>> -- ti,davinci-ecc-mode:         operation mode of the NAND ecc mode. ECC mode
>>> +- nand-ecc-mode:               operation mode of the NAND ecc mode. ECC mode
>>>                                   valid values for davinci driver:
>>>                                   - "none"
>>>                                   - "soft"
>>> @@ -45,10 +45,25 @@ Recommended properties :
>>>
>>>    - ti,davinci-ecc-bits:         used ECC bits, currently supported 1 or 4.
>>>
>>> -- ti,davinci-nand-buswidth:    buswidth 8 or 16.
>>
>> Shouldn't we keep the documentation for existing bindings while just
>> marking them as deprecated?
>
> I haven't found any requirements about this in doc.
> It's done in both way in other drivers - some keep properties in bindings
> documentation and some are not.
>
> Regards,
> -grygorii
>

For me this is not needed, but for example from mtd:
Documentation/devicetree/bindings/mtd/gpmc-nand.txt
,it contains deprecated properties, like a lot of others.

And this sad me I should remain deprecated properties.
Anyway if we have these properties in driver we should describe them.

-- 
Regards,
Ivan Khoronzhuk

[PATCH 07/12] memory: davinci-aemif: introduce AEMIF driver

[Khoronzhuk, Ivan]

Add new AEMIF driver for EMIF16 davinci controller. The EMIF16 module
is intended to provide a glue-less interface to a variety of
asynchronous memory devices like ASRA M, NOR and NAND memory. A total
of 256M bytes of any of these memories can be accessed at any given
time via four chip selects with 64M byte access per chip select.

Synchronous memories such as DDR1 SD RAM, SDR SDRAM and Mobile SDR
are not supported.

See http://www.ti.com/lit/ug/sprugz3a/sprugz3a.pdf

Signed-off-by: Ivan Khoronzhuk <ivan.khoronzhuk@ti.com>
---
 drivers/memory/Kconfig         |   11 ++
 drivers/memory/Makefile        |    1 +
 drivers/memory/davinci-aemif.c |  415 ++++++++++++++++++++++++++++++++++++++++
 3 files changed, 427 insertions(+)
 create mode 100644 drivers/memory/davinci-aemif.c

diff --git a/drivers/memory/Kconfig b/drivers/memory/Kconfig
index 29a11db..010e75e 100644
--- a/drivers/memory/Kconfig
+++ b/drivers/memory/Kconfig
@@ -7,6 +7,17 @@ menuconfig MEMORY

 if MEMORY

+config TI_DAVINCI_AEMIF
+       bool "Texas Instruments DaVinci AEMIF driver"
+       depends on (ARCH_DAVINCI || ARCH_KEYSTONE) && OF
+       help
+         This driver is for the AEMIF module available in Texas Instruments
+         SoCs. AEMIF stands for Asynchronous External Memory Interface and
+         is intended to provide a glue-less interface to a variety of
+         asynchronuous memory devices like ASRAM, NOR and NAND memory. A total
+         of 256M bytes of any of these memories can be accessed at a given
+         time via four chip selects with 64M byte access per chip select.
+
 config TI_EMIF
        tristate "Texas Instruments EMIF driver"
        depends on ARCH_OMAP2PLUS
diff --git a/drivers/memory/Makefile b/drivers/memory/Makefile
index 969d923..af14126 100644
--- a/drivers/memory/Makefile
+++ b/drivers/memory/Makefile
@@ -5,6 +5,7 @@
 ifeq ($(CONFIG_DDR),y)
 obj-$(CONFIG_OF)               += of_memory.o
 endif
+obj-$(CONFIG_TI_DAVINCI_AEMIF) += davinci-aemif.o
 obj-$(CONFIG_TI_EMIF)          += emif.o
 obj-$(CONFIG_MVEBU_DEVBUS)     += mvebu-devbus.o
 obj-$(CONFIG_TEGRA20_MC)       += tegra20-mc.o
diff --git a/drivers/memory/davinci-aemif.c b/drivers/memory/davinci-aemif.c
new file mode 100644
index 0000000..e36b74b
--- /dev/null
+++ b/drivers/memory/davinci-aemif.c
@@ -0,0 +1,415 @@
+/*
+ * DaVinci/Keystone AEMIF driver
+ *
+ * Copyright (C) 2010 - 2013 Texas Instruments Incorporated. http://www.ti.com/
+ * Copyright (C) Heiko Schocher <hs@denx.de>
+ * Copyright (C) Ivan Khoronzhuk <ivan.khoronzhuk@ti.com>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ */
+
+#include <linux/clk.h>
+#include <linux/err.h>
+#include <linux/io.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/of_platform.h>
+#include <linux/platform_device.h>
+
+#define TA_SHIFT       2
+#define RHOLD_SHIFT    4
+#define RSTROBE_SHIFT  7
+#define RSETUP_SHIFT   13
+#define WHOLD_SHIFT    17
+#define WSTROBE_SHIFT  20
+#define WSETUP_SHIFT   26
+#define EW_SHIFT       30
+#define SS_SHIFT       31
+
+#define TA(x)          ((x) << TA_SHIFT)
+#define RHOLD(x)       ((x) << RHOLD_SHIFT)
+#define RSTROBE(x)     ((x) << RSTROBE_SHIFT)
+#define RSETUP(x)      ((x) << RSETUP_SHIFT)
+#define WHOLD(x)       ((x) << WHOLD_SHIFT)
+#define WSTROBE(x)     ((x) << WSTROBE_SHIFT)
+#define WSETUP(x)      ((x) << WSETUP_SHIFT)
+#define EW(x)          ((x) << EW_SHIFT)
+#define SS(x)          ((x) << SS_SHIFT)
+
+#define ASIZE_MAX      0x1
+#define TA_MAX         0x3
+#define RHOLD_MAX      0x7
+#define RSTROBE_MAX    0x3f
+#define RSETUP_MAX     0xf
+#define WHOLD_MAX      0x7
+#define WSTROBE_MAX    0x3f
+#define WSETUP_MAX     0xf
+#define EW_MAX         0x1
+#define SS_MAX         0x1
+#define NUM_CS         4
+
+#define TA_VAL(x)      (((x) & TA(TA_MAX)) >> TA_SHIFT)
+#define RHOLD_VAL(x)   (((x) & RHOLD(RHOLD_MAX)) >> RHOLD_SHIFT)
+#define RSTROBE_VAL(x) (((x) & RSTROBE(RSTROBE_MAX)) >> RSTROBE_SHIFT)
+#define RSETUP_VAL(x)  (((x) & RSETUP(RSETUP_MAX)) >> RSETUP_SHIFT)
+#define WHOLD_VAL(x)   (((x) & WHOLD(WHOLD_MAX)) >> WHOLD_SHIFT)
+#define WSTROBE_VAL(x) (((x) & WSTROBE(WSTROBE_MAX)) >> WSTROBE_SHIFT)
+#define WSETUP_VAL(x)  (((x) & WSETUP(WSETUP_MAX)) >> WSETUP_SHIFT)
+#define EW_VAL(x)      (((x) & EW(EW_MAX)) >> EW_SHIFT)
+#define SS_VAL(x)      (((x) & SS(SS_MAX)) >> SS_SHIFT)
+
+#define NRCSR_OFFSET   0x00
+#define AWCCR_OFFSET   0x04
+#define A1CR_OFFSET    0x10
+
+#define ACR_ASIZE_MASK 0x3
+#define ACR_EW_MASK    BIT(30)
+#define ACR_SS_MASK    BIT(31)
+#define ASIZE_16BIT    1
+
+#define CONFIG_MASK    (TA(TA_MAX) | \
+                               RHOLD(RHOLD_MAX) | \
+                               RSTROBE(RSTROBE_MAX) |  \
+                               RSETUP(RSETUP_MAX) | \
+                               WHOLD(WHOLD_MAX) | \
+                               WSTROBE(WSTROBE_MAX) | \
+                               WSETUP(WSETUP_MAX) | \
+                               EW(EW_MAX) | SS(SS_MAX) | \
+                               ASIZE_MAX)
+
+#define DRV_NAME       "davinci-aemif"
+
+/**
+ * structure to hold cs parameters
+ * @cs: chip-select number
+ * @wstrobe: write strobe width, ns
+ * @rstrobe: read strobe width, ns
+ * @wsetup: write setup width, ns
+ * @whold: write hold width, ns
+ * @rsetup: read setup width, ns
+ * @rhold: read hold width, ns
+ * @ta: minimum turn around time, ns
+ * @enable_ss: enable/disable select strobe mode
+ * @enable_ew: enable/disable extended wait mode
+ * @asize: width of the asynchronous device's data bus
+ */
+struct davinci_aemif_cs_data {
+       u8      cs;
+       u16     wstrobe;
+       u16     rstrobe;
+       u8      wsetup;
+       u8      whold;
+       u8      rsetup;
+       u8      rhold;
+       u8      ta;
+       u8      enable_ss;
+       u8      enable_ew;
+       u8      asize;
+};
+
+/**
+ * structure to hold device data
+ * @base: base address of AEMIF registers
+ * @clk: source clock
+ * @clk_rate: clock's rate in kHz
+ * @num_cs: number of assigned chip-selects
+ * @cs_data: array of chip-select settings
+ */
+struct davinci_aemif_device {
+       struct device *dev;
+       void __iomem *base;
+       struct clk *clk;
+       unsigned long clk_rate;
+       u8 num_cs;
+       int cs_offset;
+       struct davinci_aemif_cs_data cs_data[NUM_CS];
+};
+
+static struct davinci_aemif_device *aemif;
+/**
+ * davinci_aemif_calc_rate - calculate timing data.
+ * @wanted: The cycle time needed in nanoseconds.
+ * @clk: The input clock rate in kHz.
+ * @max: The maximum divider value that can be programmed.
+ *
+ * On success, returns the calculated timing value minus 1 for easy
+ * programming into AEMIF timing registers, else negative errno.
+ */
+static int davinci_aemif_calc_rate(int wanted, unsigned long clk, int max)
+{
+       int result;
+
+       result = DIV_ROUND_UP((wanted * clk), NSEC_PER_MSEC) - 1;
+
+       dev_dbg(aemif->dev, "%s: result %d from %ld, %d\n", __func__, result,
+               clk, wanted);
+
+       /* It is generally OK to have a more relaxed timing than requested... */
+       if (result < 0)
+               result = 0;
+
+       /* ... But configuring tighter timings is not an option. */
+       else if (result > max)
+               result = -EINVAL;
+
+       return result;
+}
+
+/**
+ * davinci_aemif_config_abus - configure async bus parameters
+ * @data: aemif chip select configuration
+ *
+ * This function programs the given timing values (in real clock) into the
+ * AEMIF registers taking the AEMIF clock into account.
+ *
+ * This function does not use any locking while programming the AEMIF
+ * because it is expected that there is only one user of a given
+ * chip-select.
+ *
+ * Returns 0 on success, else negative errno.
+ */
+static int davinci_aemif_config_abus(struct davinci_aemif_cs_data *data)
+{
+       int ta, rhold, rstrobe, rsetup, whold, wstrobe, wsetup;
+       unsigned offset;
+       u32 set, val;
+
+       offset = A1CR_OFFSET + (data->cs - aemif->cs_offset) * 4;
+
+       ta      = davinci_aemif_calc_rate(data->ta, aemif->clk_rate, TA_MAX);
+       rhold   = davinci_aemif_calc_rate(data->rhold, aemif->clk_rate,
+                                         RHOLD_MAX);
+       rstrobe = davinci_aemif_calc_rate(data->rstrobe, aemif->clk_rate,
+                                         RSTROBE_MAX);
+       rsetup  = davinci_aemif_calc_rate(data->rsetup, aemif->clk_rate,
+                                         RSETUP_MAX);
+       whold   = davinci_aemif_calc_rate(data->whold, aemif->clk_rate,
+                                         WHOLD_MAX);
+       wstrobe = davinci_aemif_calc_rate(data->wstrobe, aemif->clk_rate,
+                                         WSTROBE_MAX);
+       wsetup  = davinci_aemif_calc_rate(data->wsetup, aemif->clk_rate,
+                                         WSETUP_MAX);
+
+       if (ta < 0 || rhold < 0 || rstrobe < 0 || rsetup < 0 ||
+                       whold < 0 || wstrobe < 0 || wsetup < 0) {
+               dev_err(aemif->dev, "%s: cannot get suitable timings\n",
+                       __func__);
+               return -EINVAL;
+       }
+
+       set = TA(ta) | RHOLD(rhold) | RSTROBE(rstrobe) | RSETUP(rsetup) |
+               WHOLD(whold) | WSTROBE(wstrobe) | WSETUP(wsetup);
+
+       set |= (data->asize & ACR_ASIZE_MASK);
+       if (data->enable_ew)
+               set |= ACR_EW_MASK;
+       if (data->enable_ss)
+               set |= ACR_SS_MASK;
+
+       val = readl(aemif->base + offset);
+       val &= ~CONFIG_MASK;
+       val |= set;
+       writel(val, aemif->base + offset);
+
+       return 0;
+}
+
+inline int davinci_aemif_cycles_to_nsec(int val)
+{
+       return ((val + 1) * NSEC_PER_MSEC) / aemif->clk_rate;
+}
+
+/**
+ * davinci_aemif_get_hw_params - function to read hw register values
+ * @data: ptr to cs data
+ *
+ * This function reads the defaults from the registers and update
+ * the timing values. Required for get/set commands and also for
+ * the case when driver needs to use defaults in hardware.
+ */
+static void davinci_aemif_get_hw_params(struct davinci_aemif_cs_data *data)
+{
+       u32 val, offset;
+       offset = A1CR_OFFSET + (data->cs - aemif->cs_offset) * 4;
+
+       val = readl(aemif->base + offset);
+       data->ta = davinci_aemif_cycles_to_nsec(TA_VAL(val));
+       data->rhold = davinci_aemif_cycles_to_nsec(RHOLD_VAL(val));
+       data->rstrobe = davinci_aemif_cycles_to_nsec(RSTROBE_VAL(val));
+       data->rsetup = davinci_aemif_cycles_to_nsec(RSETUP_VAL(val));
+       data->whold = davinci_aemif_cycles_to_nsec(WHOLD_VAL(val));
+       data->wstrobe = davinci_aemif_cycles_to_nsec(WSTROBE_VAL(val));
+       data->wsetup = davinci_aemif_cycles_to_nsec(WSETUP_VAL(val));
+       data->enable_ew = EW_VAL(val);
+       data->enable_ss = SS_VAL(val);
+       data->asize = val & ASIZE_MAX;
+}
+
+/**
+ * of_davinci_aemif_parse_abus_config - parse CS configuration from DT
+ * @np: device node ptr
+ *
+ * This function update the emif async bus configuration based on the values
+ * configured in a cs device binding node.
+ */
+static int of_davinci_aemif_parse_abus_config(struct device_node *np)
+{
+       struct davinci_aemif_cs_data *data;
+       unsigned long cs;
+       u32 val;
+
+       if (kstrtoul(np->name + 2, 10, &cs) < 0) {
+               dev_dbg(aemif->dev, "cs name is incorrect");
+               return -EINVAL;
+       }
+
+       if (cs - aemif->cs_offset >= NUM_CS || cs < aemif->cs_offset) {
+               dev_dbg(aemif->dev, "cs number is incorrect %lu", cs);
+               return -EINVAL;
+       }
+
+       if (aemif->num_cs >= NUM_CS) {
+               dev_dbg(aemif->dev, "cs count is more than %d", NUM_CS);
+               return -EINVAL;
+       }
+
+       data = &aemif->cs_data[aemif->num_cs++];
+       data->cs = cs;
+
+       /* read the current value in the hw register */
+       davinci_aemif_get_hw_params(data);
+
+       /* override the values from device node */
+       of_property_read_u8(np, "ti,davinci-cs-ta", &data->ta);
+       of_property_read_u8(np, "ti,davinci-cs-rhold", &data->rhold);
+       of_property_read_u16(np, "ti,davinci-cs-rstrobe", &data->rstrobe);
+       of_property_read_u8(np, "ti,davinci-cs-rsetup", &data->rsetup);
+       of_property_read_u8(np, "ti,davinci-cs-whold", &data->whold);
+       of_property_read_u16(np, "ti,davinci-cs-wstrobe", &data->wstrobe);
+       of_property_read_u8(np, "ti,davinci-cs-wsetup", &data->wsetup);
+       if (!of_property_read_u32(np, "ti,bus-width", &val))
+               if (val == 16)
+                       data->asize = 1;
+       data->enable_ew = of_property_read_bool(np, "ti,davinci-cs-ew");
+       data->enable_ss = of_property_read_bool(np, "ti,davinci-cs-ss");
+       return 0;
+}
+
+static const struct of_device_id davinci_aemif_of_match[] = {
+       { .compatible = "ti,davinci-aemif", },
+       { .compatible = "ti,keystone-aemif", },
+       { .compatible = "ti,omap-L138-aemif", },
+       {},
+};
+
+static const struct of_device_id davinci_cs_of_match[] = {
+       { .compatible = "ti,davinci-cs", },
+       {},
+};
+
+static int davinci_aemif_probe(struct platform_device *pdev)
+{
+       int ret  = -ENODEV, i;
+       struct resource *res;
+       struct device *dev = &pdev->dev;
+       struct device_node *np = dev->of_node;
+
+       if (np == NULL)
+               return 0;
+
+       if (aemif) {
+               dev_err(dev, "davinci_aemif driver is in use currently\n");
+               return -EBUSY;
+       }
+
+       aemif = devm_kzalloc(dev, sizeof(*aemif), GFP_KERNEL);
+
+       if (!aemif) {
+               dev_err(dev, "cannot allocate memory for aemif\n");
+               return -ENOMEM;
+       }
+
+       aemif->clk = devm_clk_get(dev, "aemif");
+       if (IS_ERR(aemif->clk)) {
+               dev_err(dev, "cannot get clock 'aemif'\n");
+               return PTR_ERR(aemif->clk);
+       }
+
+       clk_prepare_enable(aemif->clk);
+       aemif->clk_rate = clk_get_rate(aemif->clk) / 1000;
+
+       aemif->dev = dev;
+
+       if (of_device_is_compatible(np, "ti,omap-L138-aemif"))
+               aemif->cs_offset = 2;
+
+       res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+       aemif->base = devm_ioremap_resource(dev, res);
+       if (IS_ERR(aemif->base)) {
+               ret = PTR_ERR(aemif->base);
+               goto error;
+       }
+
+       /*
+        * For every controller device node, there is a cs device node that
+        * describe the bus configuration parameters. This functions iterate
+        * over these nodes and update the cs data array.
+        */
+       for_each_matching_node(np, of_match_ptr(davinci_cs_of_match)) {
+               ret = of_davinci_aemif_parse_abus_config(np);
+               if (ret < 0)
+                       goto error;
+       }
+
+       for (i = 0; i < aemif->num_cs; i++) {
+               ret = davinci_aemif_config_abus(&aemif->cs_data[i]);
+               if (ret < 0) {
+                       dev_err(dev, "Error configuring chip select %d\n",
+                               aemif->cs_data[i].cs);
+                       goto error;
+               }
+       }
+
+       /*
+        * Create a child devices explicitly from here to
+        * guarantee that the child will be probed after the AEMIF timing
+        * parameters are set.
+        */
+       for_each_matching_node(np, of_match_ptr(davinci_cs_of_match)) {
+               ret = of_platform_populate(np, NULL, NULL, dev);
+               if (ret < 0)
+                       goto error;
+       }
+
+       return 0;
+error:
+       clk_disable_unprepare(aemif->clk);
+       return ret;
+}
+
+static int davinci_aemif_remove(struct platform_device *pdev)
+{
+       clk_disable_unprepare(aemif->clk);
+       return 0;
+}
+
+static struct platform_driver davinci_aemif_driver = {
+       .probe = davinci_aemif_probe,
+       .remove = davinci_aemif_remove,
+       .driver = {
+               .name = DRV_NAME,
+               .owner = THIS_MODULE,
+               .of_match_table = of_match_ptr(davinci_aemif_of_match),
+       },
+};
+
+module_platform_driver(davinci_aemif_driver);
+
+MODULE_AUTHOR("Murali Karicheri <m-karicheri2@ti.com>");
+MODULE_AUTHOR("Ivan Khoronzhuk <ivan.khoronzhuk@ti.com>");
+MODULE_DESCRIPTION("Texas Instruments AEMIF driver");
+MODULE_LICENSE("GPL v2");
+MODULE_ALIAS("platform:" DRV_NAME);
--
1.7.9.5

Re: [PATCH 07/12] memory: davinci-aemif: introduce AEMIF driver

[Santosh Shilimkar]

+ Greg KH (drivers/memory/* patches goes through his queue)

On Monday 11 November 2013 12:06 PM, Khoronzhuk, Ivan wrote:
> Add new AEMIF driver for EMIF16 davinci controller. The EMIF16 module
> is intended to provide a glue-less interface to a variety of
> asynchronous memory devices like ASRA M, NOR and NAND memory. A total
> of 256M bytes of any of these memories can be accessed at any given
> time via four chip selects with 64M byte access per chip select.
> 
> Synchronous memories such as DDR1 SD RAM, SDR SDRAM and Mobile SDR
> are not supported.
> 
> See http://www.ti.com/lit/ug/sprugz3a/sprugz3a.pdf
> 
> Signed-off-by: Ivan Khoronzhuk <ivan.khoronzhuk@ti.com>
> ---
>  drivers/memory/Kconfig         |   11 ++
>  drivers/memory/Makefile        |    1 +
>  drivers/memory/davinci-aemif.c |  415 ++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 427 insertions(+)
>  create mode 100644 drivers/memory/davinci-aemif.c
> 
> diff --git a/drivers/memory/Kconfig b/drivers/memory/Kconfig
> index 29a11db..010e75e 100644
> --- a/drivers/memory/Kconfig
> +++ b/drivers/memory/Kconfig
> @@ -7,6 +7,17 @@ menuconfig MEMORY
> 
>  if MEMORY
> 
> +config TI_DAVINCI_AEMIF
s/TI_DAVINCI_AEMIF/TI_AEMIF

> +       bool "Texas Instruments DaVinci AEMIF driver"
Drop DaVinci above since its used on more SOCs.
> +       depends on (ARCH_DAVINCI || ARCH_KEYSTONE) && OF
> +       help
> +         This driver is for the AEMIF module available in Texas Instruments
> +         SoCs. AEMIF stands for Asynchronous External Memory Interface and
> +         is intended to provide a glue-less interface to a variety of
> +         asynchronuous memory devices like ASRAM, NOR and NAND memory. A total
> +         of 256M bytes of any of these memories can be accessed at a given
> +         time via four chip selects with 64M byte access per chip select.
> +
>  config TI_EMIF
>         tristate "Texas Instruments EMIF driver"
>         depends on ARCH_OMAP2PLUS
> diff --git a/drivers/memory/Makefile b/drivers/memory/Makefile
> index 969d923..af14126 100644
> --- a/drivers/memory/Makefile
> +++ b/drivers/memory/Makefile
> @@ -5,6 +5,7 @@
>  ifeq ($(CONFIG_DDR),y)
>  obj-$(CONFIG_OF)               += of_memory.o
>  endif
> +obj-$(CONFIG_TI_DAVINCI_AEMIF) += davinci-aemif.o

Change this accordingly once the config is renamed.

>  obj-$(CONFIG_TI_EMIF)          += emif.o
>  obj-$(CONFIG_MVEBU_DEVBUS)     += mvebu-devbus.o
>  obj-$(CONFIG_TEGRA20_MC)       += tegra20-mc.o
> diff --git a/drivers/memory/davinci-aemif.c b/drivers/memory/davinci-aemif.c
> new file mode 100644
> index 0000000..e36b74b
> --- /dev/null
> +++ b/drivers/memory/davinci-aemif.c
> @@ -0,0 +1,415 @@
> +/*
> + * DaVinci/Keystone AEMIF driver
s/{DaVinci/Keystone}/TI
> + *
> + * Copyright (C) 2010 - 2013 Texas Instruments Incorporated. http://www.ti.com/
> + * Copyright (C) Heiko Schocher <hs@denx.de>
> + * Copyright (C) Ivan Khoronzhuk <ivan.khoronzhuk@ti.com>
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 as
> + * published by the Free Software Foundation.
> + */
> +
> +#include <linux/clk.h>
> +#include <linux/err.h>
> +#include <linux/io.h>
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/of.h>
> +#include <linux/of_platform.h>
> +#include <linux/platform_device.h>
> +
> +#define TA_SHIFT       2
> +#define RHOLD_SHIFT    4
> +#define RSTROBE_SHIFT  7
> +#define RSETUP_SHIFT   13
> +#define WHOLD_SHIFT    17
> +#define WSTROBE_SHIFT  20
> +#define WSETUP_SHIFT   26
> +#define EW_SHIFT       30
> +#define SS_SHIFT       31
> +
> +#define TA(x)          ((x) << TA_SHIFT)
> +#define RHOLD(x)       ((x) << RHOLD_SHIFT)
> +#define RSTROBE(x)     ((x) << RSTROBE_SHIFT)
> +#define RSETUP(x)      ((x) << RSETUP_SHIFT)
> +#define WHOLD(x)       ((x) << WHOLD_SHIFT)
> +#define WSTROBE(x)     ((x) << WSTROBE_SHIFT)
> +#define WSETUP(x)      ((x) << WSETUP_SHIFT)
> +#define EW(x)          ((x) << EW_SHIFT)
> +#define SS(x)          ((x) << SS_SHIFT)
> +
> +#define ASIZE_MAX      0x1
> +#define TA_MAX         0x3
> +#define RHOLD_MAX      0x7
> +#define RSTROBE_MAX    0x3f
> +#define RSETUP_MAX     0xf
> +#define WHOLD_MAX      0x7
> +#define WSTROBE_MAX    0x3f
> +#define WSETUP_MAX     0xf
> +#define EW_MAX         0x1
> +#define SS_MAX         0x1
> +#define NUM_CS         4
> +
> +#define TA_VAL(x)      (((x) & TA(TA_MAX)) >> TA_SHIFT)
> +#define RHOLD_VAL(x)   (((x) & RHOLD(RHOLD_MAX)) >> RHOLD_SHIFT)
> +#define RSTROBE_VAL(x) (((x) & RSTROBE(RSTROBE_MAX)) >> RSTROBE_SHIFT)
> +#define RSETUP_VAL(x)  (((x) & RSETUP(RSETUP_MAX)) >> RSETUP_SHIFT)
> +#define WHOLD_VAL(x)   (((x) & WHOLD(WHOLD_MAX)) >> WHOLD_SHIFT)
> +#define WSTROBE_VAL(x) (((x) & WSTROBE(WSTROBE_MAX)) >> WSTROBE_SHIFT)
> +#define WSETUP_VAL(x)  (((x) & WSETUP(WSETUP_MAX)) >> WSETUP_SHIFT)
> +#define EW_VAL(x)      (((x) & EW(EW_MAX)) >> EW_SHIFT)
> +#define SS_VAL(x)      (((x) & SS(SS_MAX)) >> SS_SHIFT)
> +
> +#define NRCSR_OFFSET   0x00
> +#define AWCCR_OFFSET   0x04
> +#define A1CR_OFFSET    0x10
> +
> +#define ACR_ASIZE_MASK 0x3
> +#define ACR_EW_MASK    BIT(30)
> +#define ACR_SS_MASK    BIT(31)
> +#define ASIZE_16BIT    1
> +
> +#define CONFIG_MASK    (TA(TA_MAX) | \
> +                               RHOLD(RHOLD_MAX) | \
> +                               RSTROBE(RSTROBE_MAX) |  \
> +                               RSETUP(RSETUP_MAX) | \
> +                               WHOLD(WHOLD_MAX) | \
> +                               WSTROBE(WSTROBE_MAX) | \
> +                               WSETUP(WSETUP_MAX) | \
> +                               EW(EW_MAX) | SS(SS_MAX) | \
> +                               ASIZE_MAX)
> +
> +#define DRV_NAME       "davinci-aemif"
s/davinci-aemif/ti-aemif
> +
> +/**
> + * structure to hold cs parameters
> + * @cs: chip-select number
> + * @wstrobe: write strobe width, ns
> + * @rstrobe: read strobe width, ns
> + * @wsetup: write setup width, ns
> + * @whold: write hold width, ns
> + * @rsetup: read setup width, ns
> + * @rhold: read hold width, ns
> + * @ta: minimum turn around time, ns
> + * @enable_ss: enable/disable select strobe mode
> + * @enable_ew: enable/disable extended wait mode
> + * @asize: width of the asynchronous device's data bus
> + */
> +struct davinci_aemif_cs_data {
> +       u8      cs;
> +       u16     wstrobe;
> +       u16     rstrobe;
> +       u8      wsetup;
> +       u8      whold;
> +       u8      rsetup;
> +       u8      rhold;
> +       u8      ta;
> +       u8      enable_ss;
> +       u8      enable_ew;
> +       u8      asize;
> +};
> +
I suggest you drop davinci prefix throughout the
driver.

[..]

> +static const struct of_device_id davinci_aemif_of_match[] = {
> +       { .compatible = "ti,davinci-aemif", },
> +       { .compatible = "ti,keystone-aemif", },
> +       { .compatible = "ti,omap-L138-aemif", },
> +       {},
> +};
Do we need three seperate compatible or "ti,aemif" should
be enough ? Are there differences which we need to handled
based on compatibles ?

> +
> +static const struct of_device_id davinci_cs_of_match[] = {
> +       { .compatible = "ti,davinci-cs", },
> +       {},
> +};
> +
> +static int davinci_aemif_probe(struct platform_device *pdev)
> +{
> +       int ret  = -ENODEV, i;
> +       struct resource *res;
> +       struct device *dev = &pdev->dev;
> +       struct device_node *np = dev->of_node;
> +
> +       if (np == NULL)
> +               return 0;
> +
> +       if (aemif) {
> +               dev_err(dev, "davinci_aemif driver is in use currently\n");
> +               return -EBUSY;
> +       }
> +
> +       aemif = devm_kzalloc(dev, sizeof(*aemif), GFP_KERNEL);
> +
Drop this extra line.

> +       if (!aemif) {
> +               dev_err(dev, "cannot allocate memory for aemif\n");
> +               return -ENOMEM;
> +       }
> +
> +       aemif->clk = devm_clk_get(dev, "aemif");
> +       if (IS_ERR(aemif->clk)) {
> +               dev_err(dev, "cannot get clock 'aemif'\n");
> +               return PTR_ERR(aemif->clk);
> +       }
> +
> +       clk_prepare_enable(aemif->clk);
> +       aemif->clk_rate = clk_get_rate(aemif->clk) / 1000;
> +
1000 divider ? Define a marco and use it to be clear.

Other than above comments patch looks fine to me.

Regards,
Santosh

Re: [PATCH 07/12] memory: davinci-aemif: introduce AEMIF driver

[ivan.khoronzhuk]

On 11/12/2013 06:08 PM, Santosh Shilimkar wrote:
> + Greg KH (drivers/memory/* patches goes through his queue)
> 
> On Monday 11 November 2013 12:06 PM, Khoronzhuk, Ivan wrote:
>> Add new AEMIF driver for EMIF16 davinci controller. The EMIF16 module
>> is intended to provide a glue-less interface to a variety of
>> asynchronous memory devices like ASRA M, NOR and NAND memory. A total
>> of 256M bytes of any of these memories can be accessed at any given
>> time via four chip selects with 64M byte access per chip select.
>>
>> Synchronous memories such as DDR1 SD RAM, SDR SDRAM and Mobile SDR
>> are not supported.
>>
>> See http://www.ti.com/lit/ug/sprugz3a/sprugz3a.pdf
>>
>> Signed-off-by: Ivan Khoronzhuk <ivan.khoronzhuk@ti.com>
>> ---
>>   drivers/memory/Kconfig         |   11 ++
>>   drivers/memory/Makefile        |    1 +
>>   drivers/memory/davinci-aemif.c |  415 ++++++++++++++++++++++++++++++++++++++++
>>   3 files changed, 427 insertions(+)
>>   create mode 100644 drivers/memory/davinci-aemif.c
>>
>> diff --git a/drivers/memory/Kconfig b/drivers/memory/Kconfig
>> index 29a11db..010e75e 100644
>> --- a/drivers/memory/Kconfig
>> +++ b/drivers/memory/Kconfig
>> @@ -7,6 +7,17 @@ menuconfig MEMORY
>>
>>   if MEMORY
>>
>> +config TI_DAVINCI_AEMIF
> s/TI_DAVINCI_AEMIF/TI_AEMIF
> 

Ok

>> +       bool "Texas Instruments DaVinci AEMIF driver"
> Drop DaVinci above since its used on more SOCs.
>> +       depends on (ARCH_DAVINCI || ARCH_KEYSTONE) && OF
>> +       help
>> +         This driver is for the AEMIF module available in Texas Instruments
>> +         SoCs. AEMIF stands for Asynchronous External Memory Interface and
>> +         is intended to provide a glue-less interface to a variety of
>> +         asynchronuous memory devices like ASRAM, NOR and NAND memory. A total
>> +         of 256M bytes of any of these memories can be accessed at a given
>> +         time via four chip selects with 64M byte access per chip select.
>> +
>>   config TI_EMIF
>>          tristate "Texas Instruments EMIF driver"
>>          depends on ARCH_OMAP2PLUS
>> diff --git a/drivers/memory/Makefile b/drivers/memory/Makefile
>> index 969d923..af14126 100644
>> --- a/drivers/memory/Makefile
>> +++ b/drivers/memory/Makefile
>> @@ -5,6 +5,7 @@
>>   ifeq ($(CONFIG_DDR),y)
>>   obj-$(CONFIG_OF)               += of_memory.o
>>   endif
>> +obj-$(CONFIG_TI_DAVINCI_AEMIF) += davinci-aemif.o
> 
> Change this accordingly once the config is renamed.
> 

Ok

>>   obj-$(CONFIG_TI_EMIF)          += emif.o
>>   obj-$(CONFIG_MVEBU_DEVBUS)     += mvebu-devbus.o
>>   obj-$(CONFIG_TEGRA20_MC)       += tegra20-mc.o
>> diff --git a/drivers/memory/davinci-aemif.c b/drivers/memory/davinci-aemif.c
>> new file mode 100644
>> index 0000000..e36b74b
>> --- /dev/null
>> +++ b/drivers/memory/davinci-aemif.c
>> @@ -0,0 +1,415 @@
>> +/*
>> + * DaVinci/Keystone AEMIF driver
> s/{DaVinci/Keystone}/TI
>> + *
>> + * Copyright (C) 2010 - 2013 Texas Instruments Incorporated. http://www.ti.com/
>> + * Copyright (C) Heiko Schocher <hs@denx.de>
>> + * Copyright (C) Ivan Khoronzhuk <ivan.khoronzhuk@ti.com>
>> + *
>> + * This program is free software; you can redistribute it and/or modify
>> + * it under the terms of the GNU General Public License version 2 as
>> + * published by the Free Software Foundation.
>> + */
>> +
>> +#include <linux/clk.h>
>> +#include <linux/err.h>
>> +#include <linux/io.h>
>> +#include <linux/kernel.h>
>> +#include <linux/module.h>
>> +#include <linux/of.h>
>> +#include <linux/of_platform.h>
>> +#include <linux/platform_device.h>
>> +
>> +#define TA_SHIFT       2
>> +#define RHOLD_SHIFT    4
>> +#define RSTROBE_SHIFT  7
>> +#define RSETUP_SHIFT   13
>> +#define WHOLD_SHIFT    17
>> +#define WSTROBE_SHIFT  20
>> +#define WSETUP_SHIFT   26
>> +#define EW_SHIFT       30
>> +#define SS_SHIFT       31
>> +
>> +#define TA(x)          ((x) << TA_SHIFT)
>> +#define RHOLD(x)       ((x) << RHOLD_SHIFT)
>> +#define RSTROBE(x)     ((x) << RSTROBE_SHIFT)
>> +#define RSETUP(x)      ((x) << RSETUP_SHIFT)
>> +#define WHOLD(x)       ((x) << WHOLD_SHIFT)
>> +#define WSTROBE(x)     ((x) << WSTROBE_SHIFT)
>> +#define WSETUP(x)      ((x) << WSETUP_SHIFT)
>> +#define EW(x)          ((x) << EW_SHIFT)
>> +#define SS(x)          ((x) << SS_SHIFT)
>> +
>> +#define ASIZE_MAX      0x1
>> +#define TA_MAX         0x3
>> +#define RHOLD_MAX      0x7
>> +#define RSTROBE_MAX    0x3f
>> +#define RSETUP_MAX     0xf
>> +#define WHOLD_MAX      0x7
>> +#define WSTROBE_MAX    0x3f
>> +#define WSETUP_MAX     0xf
>> +#define EW_MAX         0x1
>> +#define SS_MAX         0x1
>> +#define NUM_CS         4
>> +
>> +#define TA_VAL(x)      (((x) & TA(TA_MAX)) >> TA_SHIFT)
>> +#define RHOLD_VAL(x)   (((x) & RHOLD(RHOLD_MAX)) >> RHOLD_SHIFT)
>> +#define RSTROBE_VAL(x) (((x) & RSTROBE(RSTROBE_MAX)) >> RSTROBE_SHIFT)
>> +#define RSETUP_VAL(x)  (((x) & RSETUP(RSETUP_MAX)) >> RSETUP_SHIFT)
>> +#define WHOLD_VAL(x)   (((x) & WHOLD(WHOLD_MAX)) >> WHOLD_SHIFT)
>> +#define WSTROBE_VAL(x) (((x) & WSTROBE(WSTROBE_MAX)) >> WSTROBE_SHIFT)
>> +#define WSETUP_VAL(x)  (((x) & WSETUP(WSETUP_MAX)) >> WSETUP_SHIFT)
>> +#define EW_VAL(x)      (((x) & EW(EW_MAX)) >> EW_SHIFT)
>> +#define SS_VAL(x)      (((x) & SS(SS_MAX)) >> SS_SHIFT)
>> +
>> +#define NRCSR_OFFSET   0x00
>> +#define AWCCR_OFFSET   0x04
>> +#define A1CR_OFFSET    0x10
>> +
>> +#define ACR_ASIZE_MASK 0x3
>> +#define ACR_EW_MASK    BIT(30)
>> +#define ACR_SS_MASK    BIT(31)
>> +#define ASIZE_16BIT    1
>> +
>> +#define CONFIG_MASK    (TA(TA_MAX) | \
>> +                               RHOLD(RHOLD_MAX) | \
>> +                               RSTROBE(RSTROBE_MAX) |  \
>> +                               RSETUP(RSETUP_MAX) | \
>> +                               WHOLD(WHOLD_MAX) | \
>> +                               WSTROBE(WSTROBE_MAX) | \
>> +                               WSETUP(WSETUP_MAX) | \
>> +                               EW(EW_MAX) | SS(SS_MAX) | \
>> +                               ASIZE_MAX)
>> +
>> +#define DRV_NAME       "davinci-aemif"
> s/davinci-aemif/ti-aemif

Ok

>> +
>> +/**
>> + * structure to hold cs parameters
>> + * @cs: chip-select number
>> + * @wstrobe: write strobe width, ns
>> + * @rstrobe: read strobe width, ns
>> + * @wsetup: write setup width, ns
>> + * @whold: write hold width, ns
>> + * @rsetup: read setup width, ns
>> + * @rhold: read hold width, ns
>> + * @ta: minimum turn around time, ns
>> + * @enable_ss: enable/disable select strobe mode
>> + * @enable_ew: enable/disable extended wait mode
>> + * @asize: width of the asynchronous device's data bus
>> + */
>> +struct davinci_aemif_cs_data {
>> +       u8      cs;
>> +       u16     wstrobe;
>> +       u16     rstrobe;
>> +       u8      wsetup;
>> +       u8      whold;
>> +       u8      rsetup;
>> +       u8      rhold;
>> +       u8      ta;
>> +       u8      enable_ss;
>> +       u8      enable_ew;
>> +       u8      asize;
>> +};
>> +
> I suggest you drop davinci prefix throughout the
> driver.
> 
> [..]
> 

Seems there is nothing to prevent
Ok

>> +static const struct of_device_id davinci_aemif_of_match[] = {
>> +       { .compatible = "ti,davinci-aemif", },
>> +       { .compatible = "ti,keystone-aemif", },
>> +       { .compatible = "ti,omap-L138-aemif", },
>> +       {},
>> +};
> Do we need three seperate compatible or "ti,aemif" should
> be enough ? Are there differences which we need to handled
> based on compatibles ?
> 

We need separate compatibles. Depending on this cs start number is evaluated.
For omap-L138-aemif it is 2 for keystone-aemif it is 0.

>> +
>> +static const struct of_device_id davinci_cs_of_match[] = {
>> +       { .compatible = "ti,davinci-cs", },
>> +       {},
>> +};
>> +
>> +static int davinci_aemif_probe(struct platform_device *pdev)
>> +{
>> +       int ret  = -ENODEV, i;
>> +       struct resource *res;
>> +       struct device *dev = &pdev->dev;
>> +       struct device_node *np = dev->of_node;
>> +
>> +       if (np == NULL)
>> +               return 0;
>> +
>> +       if (aemif) {
>> +               dev_err(dev, "davinci_aemif driver is in use currently\n");
>> +               return -EBUSY;
>> +       }
>> +
>> +       aemif = devm_kzalloc(dev, sizeof(*aemif), GFP_KERNEL);
>> +
> Drop this extra line.
> 

Ok, I will

>> +       if (!aemif) {
>> +               dev_err(dev, "cannot allocate memory for aemif\n");
>> +               return -ENOMEM;
>> +       }
>> +
>> +       aemif->clk = devm_clk_get(dev, "aemif");
>> +       if (IS_ERR(aemif->clk)) {
>> +               dev_err(dev, "cannot get clock 'aemif'\n");
>> +               return PTR_ERR(aemif->clk);
>> +       }
>> +
>> +       clk_prepare_enable(aemif->clk);
>> +       aemif->clk_rate = clk_get_rate(aemif->clk) / 1000;
>> +
> 1000 divider ? Define a marco and use it to be clear.

Is it OK if I use MSEC_PER_SEC (linux/time.h).

> 
> Other than above comments patch looks fine to me.
> 
> Regards,
> Santosh
> 


-- 
Regards,
Ivan Khoronzhuk

Re: [PATCH 07/12] memory: davinci-aemif: introduce AEMIF driver

[Sekhar Nori]

On Monday 11 November 2013 10:36 PM, Khoronzhuk, Ivan wrote:
> Add new AEMIF driver for EMIF16 davinci controller. The EMIF16 module
> is intended to provide a glue-less interface to a variety of
> asynchronous memory devices like ASRA M, NOR and NAND memory. A total
> of 256M bytes of any of these memories can be accessed at any given
> time via four chip selects with 64M byte access per chip select.
> 
> Synchronous memories such as DDR1 SD RAM, SDR SDRAM and Mobile SDR
> are not supported.
> 
> See http://www.ti.com/lit/ug/sprugz3a/sprugz3a.pdf
> 
> Signed-off-by: Ivan Khoronzhuk <ivan.khoronzhuk@ti.com>
> ---
>  drivers/memory/Kconfig         |   11 ++
>  drivers/memory/Makefile        |    1 +
>  drivers/memory/davinci-aemif.c |  415 ++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 427 insertions(+)
>  create mode 100644 drivers/memory/davinci-aemif.c
> 
> diff --git a/drivers/memory/Kconfig b/drivers/memory/Kconfig
> index 29a11db..010e75e 100644
> --- a/drivers/memory/Kconfig
> +++ b/drivers/memory/Kconfig
> @@ -7,6 +7,17 @@ menuconfig MEMORY
> 
>  if MEMORY
> 
> +config TI_DAVINCI_AEMIF
> +       bool "Texas Instruments DaVinci AEMIF driver"

This driver cannot be a module? If not, why not?

> +       depends on (ARCH_DAVINCI || ARCH_KEYSTONE) && OF
> +       help
> +         This driver is for the AEMIF module available in Texas Instruments
> +         SoCs. AEMIF stands for Asynchronous External Memory Interface and
> +         is intended to provide a glue-less interface to a variety of
> +         asynchronuous memory devices like ASRAM, NOR and NAND memory. A total
> +         of 256M bytes of any of these memories can be accessed at a given
> +         time via four chip selects with 64M byte access per chip select.
> +
>  config TI_EMIF
>         tristate "Texas Instruments EMIF driver"
>         depends on ARCH_OMAP2PLUS
> diff --git a/drivers/memory/Makefile b/drivers/memory/Makefile
> index 969d923..af14126 100644
> --- a/drivers/memory/Makefile
> +++ b/drivers/memory/Makefile
> @@ -5,6 +5,7 @@
>  ifeq ($(CONFIG_DDR),y)
>  obj-$(CONFIG_OF)               += of_memory.o
>  endif
> +obj-$(CONFIG_TI_DAVINCI_AEMIF) += davinci-aemif.o
>  obj-$(CONFIG_TI_EMIF)          += emif.o
>  obj-$(CONFIG_MVEBU_DEVBUS)     += mvebu-devbus.o
>  obj-$(CONFIG_TEGRA20_MC)       += tegra20-mc.o
> diff --git a/drivers/memory/davinci-aemif.c b/drivers/memory/davinci-aemif.c
> new file mode 100644
> index 0000000..e36b74b
> --- /dev/null
> +++ b/drivers/memory/davinci-aemif.c
> @@ -0,0 +1,415 @@
> +/*
> + * DaVinci/Keystone AEMIF driver
> + *
> + * Copyright (C) 2010 - 2013 Texas Instruments Incorporated. http://www.ti.com/
> + * Copyright (C) Heiko Schocher <hs@denx.de>
> + * Copyright (C) Ivan Khoronzhuk <ivan.khoronzhuk@ti.com>

Just checking: You meant Author: ? Code is already copyrighted to TI by
line above.

> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 as
> + * published by the Free Software Foundation.
> + */
> +
> +#include <linux/clk.h>
> +#include <linux/err.h>
> +#include <linux/io.h>
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/of.h>
> +#include <linux/of_platform.h>
> +#include <linux/platform_device.h>
> +
> +#define TA_SHIFT       2
> +#define RHOLD_SHIFT    4
> +#define RSTROBE_SHIFT  7
> +#define RSETUP_SHIFT   13
> +#define WHOLD_SHIFT    17
> +#define WSTROBE_SHIFT  20
> +#define WSETUP_SHIFT   26
> +#define EW_SHIFT       30
> +#define SS_SHIFT       31
> +
> +#define TA(x)          ((x) << TA_SHIFT)
> +#define RHOLD(x)       ((x) << RHOLD_SHIFT)
> +#define RSTROBE(x)     ((x) << RSTROBE_SHIFT)
> +#define RSETUP(x)      ((x) << RSETUP_SHIFT)
> +#define WHOLD(x)       ((x) << WHOLD_SHIFT)
> +#define WSTROBE(x)     ((x) << WSTROBE_SHIFT)
> +#define WSETUP(x)      ((x) << WSETUP_SHIFT)
> +#define EW(x)          ((x) << EW_SHIFT)
> +#define SS(x)          ((x) << SS_SHIFT)
> +
> +#define ASIZE_MAX      0x1
> +#define TA_MAX         0x3
> +#define RHOLD_MAX      0x7
> +#define RSTROBE_MAX    0x3f
> +#define RSETUP_MAX     0xf
> +#define WHOLD_MAX      0x7
> +#define WSTROBE_MAX    0x3f
> +#define WSETUP_MAX     0xf
> +#define EW_MAX         0x1
> +#define SS_MAX         0x1
> +#define NUM_CS         4
> +
> +#define TA_VAL(x)      (((x) & TA(TA_MAX)) >> TA_SHIFT)
> +#define RHOLD_VAL(x)   (((x) & RHOLD(RHOLD_MAX)) >> RHOLD_SHIFT)
> +#define RSTROBE_VAL(x) (((x) & RSTROBE(RSTROBE_MAX)) >> RSTROBE_SHIFT)
> +#define RSETUP_VAL(x)  (((x) & RSETUP(RSETUP_MAX)) >> RSETUP_SHIFT)
> +#define WHOLD_VAL(x)   (((x) & WHOLD(WHOLD_MAX)) >> WHOLD_SHIFT)
> +#define WSTROBE_VAL(x) (((x) & WSTROBE(WSTROBE_MAX)) >> WSTROBE_SHIFT)
> +#define WSETUP_VAL(x)  (((x) & WSETUP(WSETUP_MAX)) >> WSETUP_SHIFT)
> +#define EW_VAL(x)      (((x) & EW(EW_MAX)) >> EW_SHIFT)
> +#define SS_VAL(x)      (((x) & SS(SS_MAX)) >> SS_SHIFT)
> +
> +#define NRCSR_OFFSET   0x00
> +#define AWCCR_OFFSET   0x04
> +#define A1CR_OFFSET    0x10
> +
> +#define ACR_ASIZE_MASK 0x3
> +#define ACR_EW_MASK    BIT(30)
> +#define ACR_SS_MASK    BIT(31)
> +#define ASIZE_16BIT    1
> +
> +#define CONFIG_MASK    (TA(TA_MAX) | \
> +                               RHOLD(RHOLD_MAX) | \
> +                               RSTROBE(RSTROBE_MAX) |  \
> +                               RSETUP(RSETUP_MAX) | \
> +                               WHOLD(WHOLD_MAX) | \
> +                               WSTROBE(WSTROBE_MAX) | \
> +                               WSETUP(WSETUP_MAX) | \
> +                               EW(EW_MAX) | SS(SS_MAX) | \
> +                               ASIZE_MAX)
> +
> +#define DRV_NAME       "davinci-aemif"
> +
> +/**
> + * structure to hold cs parameters
> + * @cs: chip-select number
> + * @wstrobe: write strobe width, ns
> + * @rstrobe: read strobe width, ns
> + * @wsetup: write setup width, ns
> + * @whold: write hold width, ns
> + * @rsetup: read setup width, ns
> + * @rhold: read hold width, ns
> + * @ta: minimum turn around time, ns
> + * @enable_ss: enable/disable select strobe mode
> + * @enable_ew: enable/disable extended wait mode
> + * @asize: width of the asynchronous device's data bus
> + */
> +struct davinci_aemif_cs_data {
> +       u8      cs;
> +       u16     wstrobe;
> +       u16     rstrobe;
> +       u8      wsetup;
> +       u8      whold;
> +       u8      rsetup;
> +       u8      rhold;
> +       u8      ta;
> +       u8      enable_ss;
> +       u8      enable_ew;
> +       u8      asize;
> +};
> +
> +/**
> + * structure to hold device data
> + * @base: base address of AEMIF registers
> + * @clk: source clock
> + * @clk_rate: clock's rate in kHz
> + * @num_cs: number of assigned chip-selects
> + * @cs_data: array of chip-select settings
> + */
> +struct davinci_aemif_device {
> +       struct device *dev;
> +       void __iomem *base;
> +       struct clk *clk;
> +       unsigned long clk_rate;
> +       u8 num_cs;
> +       int cs_offset;
> +       struct davinci_aemif_cs_data cs_data[NUM_CS];
> +};
> +
> +static struct davinci_aemif_device *aemif;
> +/**
> + * davinci_aemif_calc_rate - calculate timing data.
> + * @wanted: The cycle time needed in nanoseconds.
> + * @clk: The input clock rate in kHz.
> + * @max: The maximum divider value that can be programmed.
> + *
> + * On success, returns the calculated timing value minus 1 for easy
> + * programming into AEMIF timing registers, else negative errno.
> + */
> +static int davinci_aemif_calc_rate(int wanted, unsigned long clk, int max)
> +{
> +       int result;
> +
> +       result = DIV_ROUND_UP((wanted * clk), NSEC_PER_MSEC) - 1;
> +
> +       dev_dbg(aemif->dev, "%s: result %d from %ld, %d\n", __func__, result,
> +               clk, wanted);
> +
> +       /* It is generally OK to have a more relaxed timing than requested... */
> +       if (result < 0)
> +               result = 0;
> +
> +       /* ... But configuring tighter timings is not an option. */
> +       else if (result > max)
> +               result = -EINVAL;
> +
> +       return result;
> +}
> +
> +/**
> + * davinci_aemif_config_abus - configure async bus parameters
> + * @data: aemif chip select configuration
> + *
> + * This function programs the given timing values (in real clock) into the
> + * AEMIF registers taking the AEMIF clock into account.
> + *
> + * This function does not use any locking while programming the AEMIF
> + * because it is expected that there is only one user of a given
> + * chip-select.
> + *
> + * Returns 0 on success, else negative errno.
> + */
> +static int davinci_aemif_config_abus(struct davinci_aemif_cs_data *data)
> +{
> +       int ta, rhold, rstrobe, rsetup, whold, wstrobe, wsetup;
> +       unsigned offset;
> +       u32 set, val;
> +
> +       offset = A1CR_OFFSET + (data->cs - aemif->cs_offset) * 4;
> +
> +       ta      = davinci_aemif_calc_rate(data->ta, aemif->clk_rate, TA_MAX);
> +       rhold   = davinci_aemif_calc_rate(data->rhold, aemif->clk_rate,
> +                                         RHOLD_MAX);
> +       rstrobe = davinci_aemif_calc_rate(data->rstrobe, aemif->clk_rate,
> +                                         RSTROBE_MAX);
> +       rsetup  = davinci_aemif_calc_rate(data->rsetup, aemif->clk_rate,
> +                                         RSETUP_MAX);
> +       whold   = davinci_aemif_calc_rate(data->whold, aemif->clk_rate,
> +                                         WHOLD_MAX);
> +       wstrobe = davinci_aemif_calc_rate(data->wstrobe, aemif->clk_rate,
> +                                         WSTROBE_MAX);
> +       wsetup  = davinci_aemif_calc_rate(data->wsetup, aemif->clk_rate,
> +                                         WSETUP_MAX);
> +
> +       if (ta < 0 || rhold < 0 || rstrobe < 0 || rsetup < 0 ||
> +                       whold < 0 || wstrobe < 0 || wsetup < 0) {
> +               dev_err(aemif->dev, "%s: cannot get suitable timings\n",
> +                       __func__);
> +               return -EINVAL;
> +       }
> +
> +       set = TA(ta) | RHOLD(rhold) | RSTROBE(rstrobe) | RSETUP(rsetup) |
> +               WHOLD(whold) | WSTROBE(wstrobe) | WSETUP(wsetup);
> +
> +       set |= (data->asize & ACR_ASIZE_MASK);
> +       if (data->enable_ew)
> +               set |= ACR_EW_MASK;
> +       if (data->enable_ss)
> +               set |= ACR_SS_MASK;
> +
> +       val = readl(aemif->base + offset);
> +       val &= ~CONFIG_MASK;
> +       val |= set;
> +       writel(val, aemif->base + offset);
> +
> +       return 0;
> +}
> +
> +inline int davinci_aemif_cycles_to_nsec(int val)
> +{
> +       return ((val + 1) * NSEC_PER_MSEC) / aemif->clk_rate;
> +}
> +
> +/**
> + * davinci_aemif_get_hw_params - function to read hw register values
> + * @data: ptr to cs data
> + *
> + * This function reads the defaults from the registers and update
> + * the timing values. Required for get/set commands and also for
> + * the case when driver needs to use defaults in hardware.
> + */
> +static void davinci_aemif_get_hw_params(struct davinci_aemif_cs_data *data)
> +{
> +       u32 val, offset;
> +       offset = A1CR_OFFSET + (data->cs - aemif->cs_offset) * 4;
> +
> +       val = readl(aemif->base + offset);
> +       data->ta = davinci_aemif_cycles_to_nsec(TA_VAL(val));
> +       data->rhold = davinci_aemif_cycles_to_nsec(RHOLD_VAL(val));
> +       data->rstrobe = davinci_aemif_cycles_to_nsec(RSTROBE_VAL(val));
> +       data->rsetup = davinci_aemif_cycles_to_nsec(RSETUP_VAL(val));
> +       data->whold = davinci_aemif_cycles_to_nsec(WHOLD_VAL(val));
> +       data->wstrobe = davinci_aemif_cycles_to_nsec(WSTROBE_VAL(val));
> +       data->wsetup = davinci_aemif_cycles_to_nsec(WSETUP_VAL(val));
> +       data->enable_ew = EW_VAL(val);
> +       data->enable_ss = SS_VAL(val);
> +       data->asize = val & ASIZE_MAX;
> +}
> +
> +/**
> + * of_davinci_aemif_parse_abus_config - parse CS configuration from DT
> + * @np: device node ptr
> + *
> + * This function update the emif async bus configuration based on the values
> + * configured in a cs device binding node.
> + */
> +static int of_davinci_aemif_parse_abus_config(struct device_node *np)
> +{
> +       struct davinci_aemif_cs_data *data;
> +       unsigned long cs;
> +       u32 val;
> +
> +       if (kstrtoul(np->name + 2, 10, &cs) < 0) {
> +               dev_dbg(aemif->dev, "cs name is incorrect");
> +               return -EINVAL;
> +       }
> +
> +       if (cs - aemif->cs_offset >= NUM_CS || cs < aemif->cs_offset) {
> +               dev_dbg(aemif->dev, "cs number is incorrect %lu", cs);
> +               return -EINVAL;
> +       }
> +
> +       if (aemif->num_cs >= NUM_CS) {
> +               dev_dbg(aemif->dev, "cs count is more than %d", NUM_CS);
> +               return -EINVAL;
> +       }
> +
> +       data = &aemif->cs_data[aemif->num_cs++];
> +       data->cs = cs;
> +
> +       /* read the current value in the hw register */
> +       davinci_aemif_get_hw_params(data);
> +
> +       /* override the values from device node */
> +       of_property_read_u8(np, "ti,davinci-cs-ta", &data->ta);
> +       of_property_read_u8(np, "ti,davinci-cs-rhold", &data->rhold);
> +       of_property_read_u16(np, "ti,davinci-cs-rstrobe", &data->rstrobe);
> +       of_property_read_u8(np, "ti,davinci-cs-rsetup", &data->rsetup);
> +       of_property_read_u8(np, "ti,davinci-cs-whold", &data->whold);
> +       of_property_read_u16(np, "ti,davinci-cs-wstrobe", &data->wstrobe);
> +       of_property_read_u8(np, "ti,davinci-cs-wsetup", &data->wsetup);
> +       if (!of_property_read_u32(np, "ti,bus-width", &val))
> +               if (val == 16)
> +                       data->asize = 1;
> +       data->enable_ew = of_property_read_bool(np, "ti,davinci-cs-ew");
> +       data->enable_ss = of_property_read_bool(np, "ti,davinci-cs-ss");
> +       return 0;
> +}
> +
> +static const struct of_device_id davinci_aemif_of_match[] = {
> +       { .compatible = "ti,davinci-aemif", },
> +       { .compatible = "ti,keystone-aemif", },
> +       { .compatible = "ti,omap-L138-aemif", },

Please use ti,da850-aemif instead. We dont use the omap-like paper part
number for these devices in kernel to avoid confusion with real OMAP
devices.

> +       {},
> +};
> +
> +static const struct of_device_id davinci_cs_of_match[] = {
> +       { .compatible = "ti,davinci-cs", },
> +       {},
> +};
> +
> +static int davinci_aemif_probe(struct platform_device *pdev)
> +{
> +       int ret  = -ENODEV, i;
> +       struct resource *res;
> +       struct device *dev = &pdev->dev;
> +       struct device_node *np = dev->of_node;
> +
> +       if (np == NULL)
> +               return 0;
> +
> +       if (aemif) {
> +               dev_err(dev, "davinci_aemif driver is in use currently\n");
> +               return -EBUSY;
> +       }

Why expressly prevent multiple AEMIF devices? Its entirely conceivable
to have two memories like NAND and NOR flash connect to two different
AEMIF interfaces.

> +
> +       aemif = devm_kzalloc(dev, sizeof(*aemif), GFP_KERNEL);
> +
> +       if (!aemif) {
> +               dev_err(dev, "cannot allocate memory for aemif\n");
> +               return -ENOMEM;
> +       }
> +
> +       aemif->clk = devm_clk_get(dev, "aemif");

If this is the only clock you care about, you can use NULL for
connection id.

> +       if (IS_ERR(aemif->clk)) {
> +               dev_err(dev, "cannot get clock 'aemif'\n");
> +               return PTR_ERR(aemif->clk);
> +       }
> +
> +       clk_prepare_enable(aemif->clk);
> +       aemif->clk_rate = clk_get_rate(aemif->clk) / 1000;
> +
> +       aemif->dev = dev;
> +
> +       if (of_device_is_compatible(np, "ti,omap-L138-aemif"))
> +               aemif->cs_offset = 2;
> +
> +       res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> +       aemif->base = devm_ioremap_resource(dev, res);
> +       if (IS_ERR(aemif->base)) {
> +               ret = PTR_ERR(aemif->base);
> +               goto error;
> +       }
> +
> +       /*
> +        * For every controller device node, there is a cs device node that
> +        * describe the bus configuration parameters. This functions iterate
> +        * over these nodes and update the cs data array.
> +        */
> +       for_each_matching_node(np, of_match_ptr(davinci_cs_of_match)) {
> +               ret = of_davinci_aemif_parse_abus_config(np);
> +               if (ret < 0)
> +                       goto error;
> +       }
> +
> +       for (i = 0; i < aemif->num_cs; i++) {
> +               ret = davinci_aemif_config_abus(&aemif->cs_data[i]);
> +               if (ret < 0) {
> +                       dev_err(dev, "Error configuring chip select %d\n",
> +                               aemif->cs_data[i].cs);
> +                       goto error;
> +               }
> +       }
> +
> +       /*
> +        * Create a child devices explicitly from here to
> +        * guarantee that the child will be probed after the AEMIF timing
> +        * parameters are set.
> +        */
> +       for_each_matching_node(np, of_match_ptr(davinci_cs_of_match)) {
> +               ret = of_platform_populate(np, NULL, NULL, dev);
> +               if (ret < 0)
> +                       goto error;
> +       }
> +
> +       return 0;
> +error:
> +       clk_disable_unprepare(aemif->clk);
> +       return ret;
> +}
> +
> +static int davinci_aemif_remove(struct platform_device *pdev)
> +{
> +       clk_disable_unprepare(aemif->clk);
> +       return 0;
> +}
> +
> +static struct platform_driver davinci_aemif_driver = {
> +       .probe = davinci_aemif_probe,
> +       .remove = davinci_aemif_remove,
> +       .driver = {
> +               .name = DRV_NAME,
> +               .owner = THIS_MODULE,
> +               .of_match_table = of_match_ptr(davinci_aemif_of_match),
> +       },
> +};
> +
> +module_platform_driver(davinci_aemif_driver);
> +
> +MODULE_AUTHOR("Murali Karicheri <m-karicheri2@ti.com>");

Is this correct? If yes, I would have expected him to sign-off on the
patch too. If not, please drop this.

Thanks,
Sekhar

Re: [PATCH 07/12] memory: davinci-aemif: introduce AEMIF driver

[Santosh Shilimkar]

On Tuesday 26 November 2013 02:20 AM, Sekhar Nori wrote:
> On Monday 11 November 2013 10:36 PM, Khoronzhuk, Ivan wrote:
>> Add new AEMIF driver for EMIF16 davinci controller. The EMIF16 module
>> is intended to provide a glue-less interface to a variety of
>> asynchronous memory devices like ASRA M, NOR and NAND memory. A total
>> of 256M bytes of any of these memories can be accessed at any given
>> time via four chip selects with 64M byte access per chip select.
>>
>> Synchronous memories such as DDR1 SD RAM, SDR SDRAM and Mobile SDR
>> are not supported.
>>
>> See http://www.ti.com/lit/ug/sprugz3a/sprugz3a.pdf
>>
>> Signed-off-by: Ivan Khoronzhuk <ivan.khoronzhuk@ti.com>
>> ---

[...]

>> +static int davinci_aemif_probe(struct platform_device *pdev)
>> +{
>> +       int ret  = -ENODEV, i;
>> +       struct resource *res;
>> +       struct device *dev = &pdev->dev;
>> +       struct device_node *np = dev->of_node;
>> +
>> +       if (np == NULL)
>> +               return 0;
>> +
>> +       if (aemif) {
>> +               dev_err(dev, "davinci_aemif driver is in use currently\n");
>> +               return -EBUSY;
>> +       }
> 
> Why expressly prevent multiple AEMIF devices? Its entirely conceivable
> to have two memories like NAND and NOR flash connect to two different
> AEMIF interfaces.
> 
Ivan wanted me to clarify the Keystone hardware which supports
1 instance of controller with 4 CS. That allows already four
devices to be connected. Currently NAND and NOR are connected on it
and two more slots are free.

Since driver support what hardware is, lets not build a driver for
hardware which don't exist. And if at all such a support would be
needed in future, we can always add it.

Regards,
Santosh

Re: [PATCH 07/12] memory: davinci-aemif: introduce AEMIF driver

[Sekhar Nori]

On 11/26/2013 8:35 PM, Santosh Shilimkar wrote:
> On Tuesday 26 November 2013 02:20 AM, Sekhar Nori wrote:
>> On Monday 11 November 2013 10:36 PM, Khoronzhuk, Ivan wrote:
>>> Add new AEMIF driver for EMIF16 davinci controller. The EMIF16 module
>>> is intended to provide a glue-less interface to a variety of
>>> asynchronous memory devices like ASRA M, NOR and NAND memory. A total
>>> of 256M bytes of any of these memories can be accessed at any given
>>> time via four chip selects with 64M byte access per chip select.
>>>
>>> Synchronous memories such as DDR1 SD RAM, SDR SDRAM and Mobile SDR
>>> are not supported.
>>>
>>> See http://www.ti.com/lit/ug/sprugz3a/sprugz3a.pdf
>>>
>>> Signed-off-by: Ivan Khoronzhuk <ivan.khoronzhuk@ti.com>
>>> ---
> 
> [...]
> 
>>> +static int davinci_aemif_probe(struct platform_device *pdev)
>>> +{
>>> +       int ret  = -ENODEV, i;
>>> +       struct resource *res;
>>> +       struct device *dev = &pdev->dev;
>>> +       struct device_node *np = dev->of_node;
>>> +
>>> +       if (np == NULL)
>>> +               return 0;
>>> +
>>> +       if (aemif) {
>>> +               dev_err(dev, "davinci_aemif driver is in use currently\n");
>>> +               return -EBUSY;
>>> +       }
>>
>> Why expressly prevent multiple AEMIF devices? Its entirely conceivable
>> to have two memories like NAND and NOR flash connect to two different
>> AEMIF interfaces.
>>
> Ivan wanted me to clarify the Keystone hardware which supports
> 1 instance of controller with 4 CS. That allows already four
> devices to be connected. Currently NAND and NOR are connected on it
> and two more slots are free.
> 
> Since driver support what hardware is, lets not build a driver for
> hardware which don't exist. And if at all such a support would be
> needed in future, we can always add it.

I understand the lack of hardware part, but its common to write the
driver such that it can handle multiple instances. Is there any gain on
current hardware because of this check? From what I am hearing, the code
in question wont be exercised at all. So why go all the way and add it
in first place?

Thanks,
Sekhar

Re: [PATCH 07/12] memory: davinci-aemif: introduce AEMIF driver

[Santosh Shilimkar]

On Tuesday 26 November 2013 12:21 PM, Sekhar Nori wrote:
> On 11/26/2013 8:35 PM, Santosh Shilimkar wrote:
>> On Tuesday 26 November 2013 02:20 AM, Sekhar Nori wrote:
>>> On Monday 11 November 2013 10:36 PM, Khoronzhuk, Ivan wrote:
>>>> Add new AEMIF driver for EMIF16 davinci controller. The EMIF16 module
>>>> is intended to provide a glue-less interface to a variety of
>>>> asynchronous memory devices like ASRA M, NOR and NAND memory. A total
>>>> of 256M bytes of any of these memories can be accessed at any given
>>>> time via four chip selects with 64M byte access per chip select.
>>>>
>>>> Synchronous memories such as DDR1 SD RAM, SDR SDRAM and Mobile SDR
>>>> are not supported.
>>>>
>>>> See http://www.ti.com/lit/ug/sprugz3a/sprugz3a.pdf
>>>>
>>>> Signed-off-by: Ivan Khoronzhuk <ivan.khoronzhuk@ti.com>
>>>> ---
>>
>> [...]
>>
>>>> +static int davinci_aemif_probe(struct platform_device *pdev)
>>>> +{
>>>> +       int ret  = -ENODEV, i;
>>>> +       struct resource *res;
>>>> +       struct device *dev = &pdev->dev;
>>>> +       struct device_node *np = dev->of_node;
>>>> +
>>>> +       if (np == NULL)
>>>> +               return 0;
>>>> +
>>>> +       if (aemif) {
>>>> +               dev_err(dev, "davinci_aemif driver is in use currently\n");
>>>> +               return -EBUSY;
>>>> +       }
>>>
>>> Why expressly prevent multiple AEMIF devices? Its entirely conceivable
>>> to have two memories like NAND and NOR flash connect to two different
>>> AEMIF interfaces.
>>>
>> Ivan wanted me to clarify the Keystone hardware which supports
>> 1 instance of controller with 4 CS. That allows already four
>> devices to be connected. Currently NAND and NOR are connected on it
>> and two more slots are free.
>>
>> Since driver support what hardware is, lets not build a driver for
>> hardware which don't exist. And if at all such a support would be
>> needed in future, we can always add it.
> 
> I understand the lack of hardware part, but its common to write the
> driver such that it can handle multiple instances. Is there any gain on
> current hardware because of this check? From what I am hearing, the code
> in question wont be exercised at all. So why go all the way and add it
> in first place?
> 
Fair enough. The check can be dropped.

Regards,
Santosh

Re: [PATCH 07/12] memory: davinci-aemif: introduce AEMIF driver

[ivan.khoronzhuk]

On 11/26/2013 08:26 PM, Santosh Shilimkar wrote:
> Fair enough

Ok, I will do

-- 
Regards,
Ivan Khoronzhuk

Re: [PATCH 07/12] memory: davinci-aemif: introduce AEMIF driver

[Brian Norris]

On Tue, Nov 26, 2013 at 01:26:44PM -0500, Santosh Shilimkar wrote:
> On Tuesday 26 November 2013 12:21 PM, Sekhar Nori wrote:
> > On 11/26/2013 8:35 PM, Santosh Shilimkar wrote:
> >> On Tuesday 26 November 2013 02:20 AM, Sekhar Nori wrote:
> >>> On Monday 11 November 2013 10:36 PM, Khoronzhuk, Ivan wrote:
> >>>> +static int davinci_aemif_probe(struct platform_device *pdev)
> >>>> +{
> >>>> +       int ret  = -ENODEV, i;
> >>>> +       struct resource *res;
> >>>> +       struct device *dev = &pdev->dev;
> >>>> +       struct device_node *np = dev->of_node;
> >>>> +
> >>>> +       if (np == NULL)
> >>>> +               return 0;
> >>>> +
> >>>> +       if (aemif) {
> >>>> +               dev_err(dev, "davinci_aemif driver is in use currently\n");
> >>>> +               return -EBUSY;
> >>>> +       }
> >>>
> >>> Why expressly prevent multiple AEMIF devices? Its entirely conceivable
> >>> to have two memories like NAND and NOR flash connect to two different
> >>> AEMIF interfaces.
> >>>
> >> Ivan wanted me to clarify the Keystone hardware which supports
> >> 1 instance of controller with 4 CS. That allows already four
> >> devices to be connected. Currently NAND and NOR are connected on it
> >> and two more slots are free.
> >>
> >> Since driver support what hardware is, lets not build a driver for
> >> hardware which don't exist. And if at all such a support would be
> >> needed in future, we can always add it.
> > 
> > I understand the lack of hardware part, but its common to write the
> > driver such that it can handle multiple instances. Is there any gain on
> > current hardware because of this check? From what I am hearing, the code
> > in question wont be exercised at all. So why go all the way and add it
> > in first place?
> > 
> Fair enough. The check can be dropped.

Hmm, while the sentiment expressed by Sekhar is noble (to avoid
unnecessarily constraining the driver), removing the check is not
enough. You're still using a global static variable 'aemif', and it is
important not to accidentally re-use this struct if a second device ever
becomes available. So for the current implementation, the check is
necessary IMO. If instead, you were to make 'aemif' a per-device struct
(like with platform_set_drvdata()), then you would not have this issue.

Brian

Re: [PATCH 07/12] memory: davinci-aemif: introduce AEMIF driver

[ivan.khoronzhuk]

On 11/27/2013 02:37 AM, Brian Norris wrote:
> On Tue, Nov 26, 2013 at 01:26:44PM -0500, Santosh Shilimkar wrote:
>> On Tuesday 26 November 2013 12:21 PM, Sekhar Nori wrote:
>>> On 11/26/2013 8:35 PM, Santosh Shilimkar wrote:
>>>> On Tuesday 26 November 2013 02:20 AM, Sekhar Nori wrote:
>>>>> On Monday 11 November 2013 10:36 PM, Khoronzhuk, Ivan wrote:
>>>>>> +static int davinci_aemif_probe(struct platform_device *pdev)
>>>>>> +{
>>>>>> +       int ret  = -ENODEV, i;
>>>>>> +       struct resource *res;
>>>>>> +       struct device *dev = &pdev->dev;
>>>>>> +       struct device_node *np = dev->of_node;
>>>>>> +
>>>>>> +       if (np == NULL)
>>>>>> +               return 0;
>>>>>> +
>>>>>> +       if (aemif) {
>>>>>> +               dev_err(dev, "davinci_aemif driver is in use currently\n");
>>>>>> +               return -EBUSY;
>>>>>> +       }
>>>>>
>>>>> Why expressly prevent multiple AEMIF devices? Its entirely conceivable
>>>>> to have two memories like NAND and NOR flash connect to two different
>>>>> AEMIF interfaces.
>>>>>
>>>> Ivan wanted me to clarify the Keystone hardware which supports
>>>> 1 instance of controller with 4 CS. That allows already four
>>>> devices to be connected. Currently NAND and NOR are connected on it
>>>> and two more slots are free.
>>>>
>>>> Since driver support what hardware is, lets not build a driver for
>>>> hardware which don't exist. And if at all such a support would be
>>>> needed in future, we can always add it.
>>>
>>> I understand the lack of hardware part, but its common to write the
>>> driver such that it can handle multiple instances. Is there any gain on
>>> current hardware because of this check? From what I am hearing, the code
>>> in question wont be exercised at all. So why go all the way and add it
>>> in first place?
>>>
>> Fair enough. The check can be dropped.
>
> Hmm, while the sentiment expressed by Sekhar is noble (to avoid
> unnecessarily constraining the driver), removing the check is not
> enough. You're still using a global static variable 'aemif', and it is
> important not to accidentally re-use this struct if a second device ever
> becomes available. So for the current implementation, the check is
> necessary IMO. If instead, you were to make 'aemif' a per-device struct
> (like with platform_set_drvdata()), then you would not have this issue.
>
> Brian
>

Yes, that is the plan to make it a per-device.

-- 
Regards,
Ivan Khoronzhuk

Re: [PATCH 07/12] memory: davinci-aemif: introduce AEMIF driver

[ivan.khoronzhuk]

On 11/26/2013 09:20 AM, Sekhar Nori wrote:
> On Monday 11 November 2013 10:36 PM, Khoronzhuk, Ivan wrote:
>> Add new AEMIF driver for EMIF16 davinci controller. The EMIF16 module
>> is intended to provide a glue-less interface to a variety of
>> asynchronous memory devices like ASRA M, NOR and NAND memory. A total
>> of 256M bytes of any of these memories can be accessed at any given
>> time via four chip selects with 64M byte access per chip select.
>>
>> Synchronous memories such as DDR1 SD RAM, SDR SDRAM and Mobile SDR
>> are not supported.
>>
>> See http://www.ti.com/lit/ug/sprugz3a/sprugz3a.pdf
>>
>> Signed-off-by: Ivan Khoronzhuk <ivan.khoronzhuk@ti.com>
>> ---
>>   drivers/memory/Kconfig         |   11 ++
>>   drivers/memory/Makefile        |    1 +
>>   drivers/memory/davinci-aemif.c |  415 ++++++++++++++++++++++++++++++++++++++++
>>   3 files changed, 427 insertions(+)
>>   create mode 100644 drivers/memory/davinci-aemif.c
>>
>> diff --git a/drivers/memory/Kconfig b/drivers/memory/Kconfig
>> index 29a11db..010e75e 100644
>> --- a/drivers/memory/Kconfig
>> +++ b/drivers/memory/Kconfig
>> @@ -7,6 +7,17 @@ menuconfig MEMORY
>>
>>   if MEMORY
>>
>> +config TI_DAVINCI_AEMIF
>> +       bool "Texas Instruments DaVinci AEMIF driver"
> 
> This driver cannot be a module? If not, why not?
> 

Yes it can be a module, I will correct it.

>> +       depends on (ARCH_DAVINCI || ARCH_KEYSTONE) && OF
>> +       help
>> +         This driver is for the AEMIF module available in Texas Instruments
>> +         SoCs. AEMIF stands for Asynchronous External Memory Interface and
>> +         is intended to provide a glue-less interface to a variety of
>> +         asynchronuous memory devices like ASRAM, NOR and NAND memory. A total
>> +         of 256M bytes of any of these memories can be accessed at a given
>> +         time via four chip selects with 64M byte access per chip select.
>> +
>>   config TI_EMIF
>>          tristate "Texas Instruments EMIF driver"
>>          depends on ARCH_OMAP2PLUS
>> diff --git a/drivers/memory/Makefile b/drivers/memory/Makefile
>> index 969d923..af14126 100644
>> --- a/drivers/memory/Makefile
>> +++ b/drivers/memory/Makefile
>> @@ -5,6 +5,7 @@
>>   ifeq ($(CONFIG_DDR),y)
>>   obj-$(CONFIG_OF)               += of_memory.o
>>   endif
>> +obj-$(CONFIG_TI_DAVINCI_AEMIF) += davinci-aemif.o
>>   obj-$(CONFIG_TI_EMIF)          += emif.o
>>   obj-$(CONFIG_MVEBU_DEVBUS)     += mvebu-devbus.o
>>   obj-$(CONFIG_TEGRA20_MC)       += tegra20-mc.o
>> diff --git a/drivers/memory/davinci-aemif.c b/drivers/memory/davinci-aemif.c
>> new file mode 100644
>> index 0000000..e36b74b
>> --- /dev/null
>> +++ b/drivers/memory/davinci-aemif.c
>> @@ -0,0 +1,415 @@
>> +/*
>> + * DaVinci/Keystone AEMIF driver
>> + *
>> + * Copyright (C) 2010 - 2013 Texas Instruments Incorporated. http://www.ti.com/
>> + * Copyright (C) Heiko Schocher <hs@denx.de>
>> + * Copyright (C) Ivan Khoronzhuk <ivan.khoronzhuk@ti.com>
> 
> Just checking: You meant Author: ? Code is already copyrighted to TI by
> line above.
> 

It will be

Author:
Heiko Schocher <hs@denx.de>
Ivan Khoronzhuk <ivan.khoronzhuk@ti.com>

>> + *
>> + * This program is free software; you can redistribute it and/or modify
>> + * it under the terms of the GNU General Public License version 2 as
>> + * published by the Free Software Foundation.
>> + */
>> +
>> +#include <linux/clk.h>
>> +#include <linux/err.h>
>> +#include <linux/io.h>
>> +#include <linux/kernel.h>
>> +#include <linux/module.h>
>> +#include <linux/of.h>
>> +#include <linux/of_platform.h>
>> +#include <linux/platform_device.h>
>> +
>> +#define TA_SHIFT       2
>> +#define RHOLD_SHIFT    4
>> +#define RSTROBE_SHIFT  7
>> +#define RSETUP_SHIFT   13
>> +#define WHOLD_SHIFT    17
>> +#define WSTROBE_SHIFT  20
>> +#define WSETUP_SHIFT   26
>> +#define EW_SHIFT       30
>> +#define SS_SHIFT       31
>> +
>> +#define TA(x)          ((x) << TA_SHIFT)
>> +#define RHOLD(x)       ((x) << RHOLD_SHIFT)
>> +#define RSTROBE(x)     ((x) << RSTROBE_SHIFT)
>> +#define RSETUP(x)      ((x) << RSETUP_SHIFT)
>> +#define WHOLD(x)       ((x) << WHOLD_SHIFT)
>> +#define WSTROBE(x)     ((x) << WSTROBE_SHIFT)
>> +#define WSETUP(x)      ((x) << WSETUP_SHIFT)
>> +#define EW(x)          ((x) << EW_SHIFT)
>> +#define SS(x)          ((x) << SS_SHIFT)
>> +
>> +#define ASIZE_MAX      0x1
>> +#define TA_MAX         0x3
>> +#define RHOLD_MAX      0x7
>> +#define RSTROBE_MAX    0x3f
>> +#define RSETUP_MAX     0xf
>> +#define WHOLD_MAX      0x7
>> +#define WSTROBE_MAX    0x3f
>> +#define WSETUP_MAX     0xf
>> +#define EW_MAX         0x1
>> +#define SS_MAX         0x1
>> +#define NUM_CS         4
>> +
>> +#define TA_VAL(x)      (((x) & TA(TA_MAX)) >> TA_SHIFT)
>> +#define RHOLD_VAL(x)   (((x) & RHOLD(RHOLD_MAX)) >> RHOLD_SHIFT)
>> +#define RSTROBE_VAL(x) (((x) & RSTROBE(RSTROBE_MAX)) >> RSTROBE_SHIFT)
>> +#define RSETUP_VAL(x)  (((x) & RSETUP(RSETUP_MAX)) >> RSETUP_SHIFT)
>> +#define WHOLD_VAL(x)   (((x) & WHOLD(WHOLD_MAX)) >> WHOLD_SHIFT)
>> +#define WSTROBE_VAL(x) (((x) & WSTROBE(WSTROBE_MAX)) >> WSTROBE_SHIFT)
>> +#define WSETUP_VAL(x)  (((x) & WSETUP(WSETUP_MAX)) >> WSETUP_SHIFT)
>> +#define EW_VAL(x)      (((x) & EW(EW_MAX)) >> EW_SHIFT)
>> +#define SS_VAL(x)      (((x) & SS(SS_MAX)) >> SS_SHIFT)
>> +
>> +#define NRCSR_OFFSET   0x00
>> +#define AWCCR_OFFSET   0x04
>> +#define A1CR_OFFSET    0x10
>> +
>> +#define ACR_ASIZE_MASK 0x3
>> +#define ACR_EW_MASK    BIT(30)
>> +#define ACR_SS_MASK    BIT(31)
>> +#define ASIZE_16BIT    1
>> +
>> +#define CONFIG_MASK    (TA(TA_MAX) | \
>> +                               RHOLD(RHOLD_MAX) | \
>> +                               RSTROBE(RSTROBE_MAX) |  \
>> +                               RSETUP(RSETUP_MAX) | \
>> +                               WHOLD(WHOLD_MAX) | \
>> +                               WSTROBE(WSTROBE_MAX) | \
>> +                               WSETUP(WSETUP_MAX) | \
>> +                               EW(EW_MAX) | SS(SS_MAX) | \
>> +                               ASIZE_MAX)
>> +
>> +#define DRV_NAME       "davinci-aemif"
>> +
>> +/**
>> + * structure to hold cs parameters
>> + * @cs: chip-select number
>> + * @wstrobe: write strobe width, ns
>> + * @rstrobe: read strobe width, ns
>> + * @wsetup: write setup width, ns
>> + * @whold: write hold width, ns
>> + * @rsetup: read setup width, ns
>> + * @rhold: read hold width, ns
>> + * @ta: minimum turn around time, ns
>> + * @enable_ss: enable/disable select strobe mode
>> + * @enable_ew: enable/disable extended wait mode
>> + * @asize: width of the asynchronous device's data bus
>> + */
>> +struct davinci_aemif_cs_data {
>> +       u8      cs;
>> +       u16     wstrobe;
>> +       u16     rstrobe;
>> +       u8      wsetup;
>> +       u8      whold;
>> +       u8      rsetup;
>> +       u8      rhold;
>> +       u8      ta;
>> +       u8      enable_ss;
>> +       u8      enable_ew;
>> +       u8      asize;
>> +};
>> +
>> +/**
>> + * structure to hold device data
>> + * @base: base address of AEMIF registers
>> + * @clk: source clock
>> + * @clk_rate: clock's rate in kHz
>> + * @num_cs: number of assigned chip-selects
>> + * @cs_data: array of chip-select settings
>> + */
>> +struct davinci_aemif_device {
>> +       struct device *dev;
>> +       void __iomem *base;
>> +       struct clk *clk;
>> +       unsigned long clk_rate;
>> +       u8 num_cs;
>> +       int cs_offset;
>> +       struct davinci_aemif_cs_data cs_data[NUM_CS];
>> +};
>> +
>> +static struct davinci_aemif_device *aemif;
>> +/**
>> + * davinci_aemif_calc_rate - calculate timing data.
>> + * @wanted: The cycle time needed in nanoseconds.
>> + * @clk: The input clock rate in kHz.
>> + * @max: The maximum divider value that can be programmed.
>> + *
>> + * On success, returns the calculated timing value minus 1 for easy
>> + * programming into AEMIF timing registers, else negative errno.
>> + */
>> +static int davinci_aemif_calc_rate(int wanted, unsigned long clk, int max)
>> +{
>> +       int result;
>> +
>> +       result = DIV_ROUND_UP((wanted * clk), NSEC_PER_MSEC) - 1;
>> +
>> +       dev_dbg(aemif->dev, "%s: result %d from %ld, %d\n", __func__, result,
>> +               clk, wanted);
>> +
>> +       /* It is generally OK to have a more relaxed timing than requested... */
>> +       if (result < 0)
>> +               result = 0;
>> +
>> +       /* ... But configuring tighter timings is not an option. */
>> +       else if (result > max)
>> +               result = -EINVAL;
>> +
>> +       return result;
>> +}
>> +
>> +/**
>> + * davinci_aemif_config_abus - configure async bus parameters
>> + * @data: aemif chip select configuration
>> + *
>> + * This function programs the given timing values (in real clock) into the
>> + * AEMIF registers taking the AEMIF clock into account.
>> + *
>> + * This function does not use any locking while programming the AEMIF
>> + * because it is expected that there is only one user of a given
>> + * chip-select.
>> + *
>> + * Returns 0 on success, else negative errno.
>> + */
>> +static int davinci_aemif_config_abus(struct davinci_aemif_cs_data *data)
>> +{
>> +       int ta, rhold, rstrobe, rsetup, whold, wstrobe, wsetup;
>> +       unsigned offset;
>> +       u32 set, val;
>> +
>> +       offset = A1CR_OFFSET + (data->cs - aemif->cs_offset) * 4;
>> +
>> +       ta      = davinci_aemif_calc_rate(data->ta, aemif->clk_rate, TA_MAX);
>> +       rhold   = davinci_aemif_calc_rate(data->rhold, aemif->clk_rate,
>> +                                         RHOLD_MAX);
>> +       rstrobe = davinci_aemif_calc_rate(data->rstrobe, aemif->clk_rate,
>> +                                         RSTROBE_MAX);
>> +       rsetup  = davinci_aemif_calc_rate(data->rsetup, aemif->clk_rate,
>> +                                         RSETUP_MAX);
>> +       whold   = davinci_aemif_calc_rate(data->whold, aemif->clk_rate,
>> +                                         WHOLD_MAX);
>> +       wstrobe = davinci_aemif_calc_rate(data->wstrobe, aemif->clk_rate,
>> +                                         WSTROBE_MAX);
>> +       wsetup  = davinci_aemif_calc_rate(data->wsetup, aemif->clk_rate,
>> +                                         WSETUP_MAX);
>> +
>> +       if (ta < 0 || rhold < 0 || rstrobe < 0 || rsetup < 0 ||
>> +                       whold < 0 || wstrobe < 0 || wsetup < 0) {
>> +               dev_err(aemif->dev, "%s: cannot get suitable timings\n",
>> +                       __func__);
>> +               return -EINVAL;
>> +       }
>> +
>> +       set = TA(ta) | RHOLD(rhold) | RSTROBE(rstrobe) | RSETUP(rsetup) |
>> +               WHOLD(whold) | WSTROBE(wstrobe) | WSETUP(wsetup);
>> +
>> +       set |= (data->asize & ACR_ASIZE_MASK);
>> +       if (data->enable_ew)
>> +               set |= ACR_EW_MASK;
>> +       if (data->enable_ss)
>> +               set |= ACR_SS_MASK;
>> +
>> +       val = readl(aemif->base + offset);
>> +       val &= ~CONFIG_MASK;
>> +       val |= set;
>> +       writel(val, aemif->base + offset);
>> +
>> +       return 0;
>> +}
>> +
>> +inline int davinci_aemif_cycles_to_nsec(int val)
>> +{
>> +       return ((val + 1) * NSEC_PER_MSEC) / aemif->clk_rate;
>> +}
>> +
>> +/**
>> + * davinci_aemif_get_hw_params - function to read hw register values
>> + * @data: ptr to cs data
>> + *
>> + * This function reads the defaults from the registers and update
>> + * the timing values. Required for get/set commands and also for
>> + * the case when driver needs to use defaults in hardware.
>> + */
>> +static void davinci_aemif_get_hw_params(struct davinci_aemif_cs_data *data)
>> +{
>> +       u32 val, offset;
>> +       offset = A1CR_OFFSET + (data->cs - aemif->cs_offset) * 4;
>> +
>> +       val = readl(aemif->base + offset);
>> +       data->ta = davinci_aemif_cycles_to_nsec(TA_VAL(val));
>> +       data->rhold = davinci_aemif_cycles_to_nsec(RHOLD_VAL(val));
>> +       data->rstrobe = davinci_aemif_cycles_to_nsec(RSTROBE_VAL(val));
>> +       data->rsetup = davinci_aemif_cycles_to_nsec(RSETUP_VAL(val));
>> +       data->whold = davinci_aemif_cycles_to_nsec(WHOLD_VAL(val));
>> +       data->wstrobe = davinci_aemif_cycles_to_nsec(WSTROBE_VAL(val));
>> +       data->wsetup = davinci_aemif_cycles_to_nsec(WSETUP_VAL(val));
>> +       data->enable_ew = EW_VAL(val);
>> +       data->enable_ss = SS_VAL(val);
>> +       data->asize = val & ASIZE_MAX;
>> +}
>> +
>> +/**
>> + * of_davinci_aemif_parse_abus_config - parse CS configuration from DT
>> + * @np: device node ptr
>> + *
>> + * This function update the emif async bus configuration based on the values
>> + * configured in a cs device binding node.
>> + */
>> +static int of_davinci_aemif_parse_abus_config(struct device_node *np)
>> +{
>> +       struct davinci_aemif_cs_data *data;
>> +       unsigned long cs;
>> +       u32 val;
>> +
>> +       if (kstrtoul(np->name + 2, 10, &cs) < 0) {
>> +               dev_dbg(aemif->dev, "cs name is incorrect");
>> +               return -EINVAL;
>> +       }
>> +
>> +       if (cs - aemif->cs_offset >= NUM_CS || cs < aemif->cs_offset) {
>> +               dev_dbg(aemif->dev, "cs number is incorrect %lu", cs);
>> +               return -EINVAL;
>> +       }
>> +
>> +       if (aemif->num_cs >= NUM_CS) {
>> +               dev_dbg(aemif->dev, "cs count is more than %d", NUM_CS);
>> +               return -EINVAL;
>> +       }
>> +
>> +       data = &aemif->cs_data[aemif->num_cs++];
>> +       data->cs = cs;
>> +
>> +       /* read the current value in the hw register */
>> +       davinci_aemif_get_hw_params(data);
>> +
>> +       /* override the values from device node */
>> +       of_property_read_u8(np, "ti,davinci-cs-ta", &data->ta);
>> +       of_property_read_u8(np, "ti,davinci-cs-rhold", &data->rhold);
>> +       of_property_read_u16(np, "ti,davinci-cs-rstrobe", &data->rstrobe);
>> +       of_property_read_u8(np, "ti,davinci-cs-rsetup", &data->rsetup);
>> +       of_property_read_u8(np, "ti,davinci-cs-whold", &data->whold);
>> +       of_property_read_u16(np, "ti,davinci-cs-wstrobe", &data->wstrobe);
>> +       of_property_read_u8(np, "ti,davinci-cs-wsetup", &data->wsetup);
>> +       if (!of_property_read_u32(np, "ti,bus-width", &val))
>> +               if (val == 16)
>> +                       data->asize = 1;
>> +       data->enable_ew = of_property_read_bool(np, "ti,davinci-cs-ew");
>> +       data->enable_ss = of_property_read_bool(np, "ti,davinci-cs-ss");
>> +       return 0;
>> +}
>> +
>> +static const struct of_device_id davinci_aemif_of_match[] = {
>> +       { .compatible = "ti,davinci-aemif", },
>> +       { .compatible = "ti,keystone-aemif", },
>> +       { .compatible = "ti,omap-L138-aemif", },
> 
> Please use ti,da850-aemif instead. We dont use the omap-like paper part
> number for these devices in kernel to avoid confusion with real OMAP
> devices.
> 


Ok, I'll replace ti,omap-L138-aemif -> ti,da850-aemif

>> +       {},
>> +};
>> +
>> +static const struct of_device_id davinci_cs_of_match[] = {
>> +       { .compatible = "ti,davinci-cs", },
>> +       {},
>> +};
>> +
>> +static int davinci_aemif_probe(struct platform_device *pdev)
>> +{
>> +       int ret  = -ENODEV, i;
>> +       struct resource *res;
>> +       struct device *dev = &pdev->dev;
>> +       struct device_node *np = dev->of_node;
>> +
>> +       if (np == NULL)
>> +               return 0;
>> +
>> +       if (aemif) {
>> +               dev_err(dev, "davinci_aemif driver is in use currently\n");
>> +               return -EBUSY;
>> +       }
> 
> Why expressly prevent multiple AEMIF devices? Its entirely conceivable
> to have two memories like NAND and NOR flash connect to two different
> AEMIF interfaces.
> 

It can be, but I'm not sure if it is needed. Currently I've not seen case where
more than 2 cses were used, I mean we have 2 cs free, why do we need the second AEMIF
controller?

>> +
>> +       aemif = devm_kzalloc(dev, sizeof(*aemif), GFP_KERNEL);
>> +
>> +       if (!aemif) {
>> +               dev_err(dev, "cannot allocate memory for aemif\n");
>> +               return -ENOMEM;
>> +       }
>> +
>> +       aemif->clk = devm_clk_get(dev, "aemif");
> 
> If this is the only clock you care about, you can use NULL for
> connection id.
> 

Yes I can, I'll replace aemif name on NULL.

>> +       if (IS_ERR(aemif->clk)) {
>> +               dev_err(dev, "cannot get clock 'aemif'\n");
>> +               return PTR_ERR(aemif->clk);
>> +       }
>> +
>> +       clk_prepare_enable(aemif->clk);
>> +       aemif->clk_rate = clk_get_rate(aemif->clk) / 1000;
>> +
>> +       aemif->dev = dev;
>> +
>> +       if (of_device_is_compatible(np, "ti,omap-L138-aemif"))
>> +               aemif->cs_offset = 2;
>> +
>> +       res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>> +       aemif->base = devm_ioremap_resource(dev, res);
>> +       if (IS_ERR(aemif->base)) {
>> +               ret = PTR_ERR(aemif->base);
>> +               goto error;
>> +       }
>> +
>> +       /*
>> +        * For every controller device node, there is a cs device node that
>> +        * describe the bus configuration parameters. This functions iterate
>> +        * over these nodes and update the cs data array.
>> +        */
>> +       for_each_matching_node(np, of_match_ptr(davinci_cs_of_match)) {
>> +               ret = of_davinci_aemif_parse_abus_config(np);
>> +               if (ret < 0)
>> +                       goto error;
>> +       }
>> +
>> +       for (i = 0; i < aemif->num_cs; i++) {
>> +               ret = davinci_aemif_config_abus(&aemif->cs_data[i]);
>> +               if (ret < 0) {
>> +                       dev_err(dev, "Error configuring chip select %d\n",
>> +                               aemif->cs_data[i].cs);
>> +                       goto error;
>> +               }
>> +       }
>> +
>> +       /*
>> +        * Create a child devices explicitly from here to
>> +        * guarantee that the child will be probed after the AEMIF timing
>> +        * parameters are set.
>> +        */
>> +       for_each_matching_node(np, of_match_ptr(davinci_cs_of_match)) {
>> +               ret = of_platform_populate(np, NULL, NULL, dev);
>> +               if (ret < 0)
>> +                       goto error;
>> +       }
>> +
>> +       return 0;
>> +error:
>> +       clk_disable_unprepare(aemif->clk);
>> +       return ret;
>> +}
>> +
>> +static int davinci_aemif_remove(struct platform_device *pdev)
>> +{
>> +       clk_disable_unprepare(aemif->clk);
>> +       return 0;
>> +}
>> +
>> +static struct platform_driver davinci_aemif_driver = {
>> +       .probe = davinci_aemif_probe,
>> +       .remove = davinci_aemif_remove,
>> +       .driver = {
>> +               .name = DRV_NAME,
>> +               .owner = THIS_MODULE,
>> +               .of_match_table = of_match_ptr(davinci_aemif_of_match),
>> +       },
>> +};
>> +
>> +module_platform_driver(davinci_aemif_driver);
>> +
>> +MODULE_AUTHOR("Murali Karicheri <m-karicheri2@ti.com>");
> 
> Is this correct? If yes, I would have expected him to sign-off on the
> patch too. If not, please drop this.

I will add sign-off

> 
> Thanks,
> Sekhar
> 


-- 
Regards,
Ivan Khoronzhuk

Re: [PATCH 07/12] memory: davinci-aemif: introduce AEMIF driver

[Santosh Shilimkar]

Ivan,

On Tuesday 26 November 2013 12:44 PM, ivan.khoronzhuk wrote:
> On 11/26/2013 09:20 AM, Sekhar Nori wrote:
>> On Monday 11 November 2013 10:36 PM, Khoronzhuk, Ivan wrote:

>>> +static struct platform_driver davinci_aemif_driver = {
>>> +       .probe = davinci_aemif_probe,
>>> +       .remove = davinci_aemif_remove,
>>> +       .driver = {
>>> +               .name = DRV_NAME,
>>> +               .owner = THIS_MODULE,
>>> +               .of_match_table = of_match_ptr(davinci_aemif_of_match),
>>> +       },
>>> +};
>>> +
>>> +module_platform_driver(davinci_aemif_driver);
>>> +
>>> +MODULE_AUTHOR("Murali Karicheri <m-karicheri2@ti.com>");
>>
>> Is this correct? If yes, I would have expected him to sign-off on the
>> patch too. If not, please drop this.
> 
> I will add sign-off
> 
Please add you as a MODULE_AUTHOR as well since you re-wrote the
downstream driver. Ok to have both of you sign of on it.

Regards,
Santosh

Re: [PATCH 07/12] memory: davinci-aemif: introduce AEMIF driver

[ivan.khoronzhuk]

On 11/26/2013 08:30 PM, Santosh Shilimkar wrote:
> Ivan,
>
> On Tuesday 26 November 2013 12:44 PM, ivan.khoronzhuk wrote:
>> On 11/26/2013 09:20 AM, Sekhar Nori wrote:
>>> On Monday 11 November 2013 10:36 PM, Khoronzhuk, Ivan wrote:
>
>>>> +static struct platform_driver davinci_aemif_driver = {
>>>> +       .probe = davinci_aemif_probe,
>>>> +       .remove = davinci_aemif_remove,
>>>> +       .driver = {
>>>> +               .name = DRV_NAME,
>>>> +               .owner = THIS_MODULE,
>>>> +               .of_match_table = of_match_ptr(davinci_aemif_of_match),
>>>> +       },
>>>> +};
>>>> +
>>>> +module_platform_driver(davinci_aemif_driver);
>>>> +
>>>> +MODULE_AUTHOR("Murali Karicheri <m-karicheri2@ti.com>");
>>>
>>> Is this correct? If yes, I would have expected him to sign-off on the
>>> patch too. If not, please drop this.
>>
>> I will add sign-off
>>
> Please add you as a MODULE_AUTHOR as well since you re-wrote the
> downstream driver. Ok to have both of you sign of on it.
>
> Regards,
> Santosh
>

Ok

-- 
Regards,
Ivan Khoronzhuk

Re: [PATCH 07/12] memory: davinci-aemif: introduce AEMIF driver

[Sekhar Nori]

On Tuesday 26 November 2013 11:14 PM, ivan.khoronzhuk wrote:

>>> +static int davinci_aemif_probe(struct platform_device *pdev)
>>> +{
>>> +       int ret  = -ENODEV, i;
>>> +       struct resource *res;
>>> +       struct device *dev = &pdev->dev;
>>> +       struct device_node *np = dev->of_node;
>>> +
>>> +       if (np == NULL)
>>> +               return 0;
>>> +
>>> +       if (aemif) {
>>> +               dev_err(dev, "davinci_aemif driver is in use currently\n");
>>> +               return -EBUSY;
>>> +       }
>>
>> Why expressly prevent multiple AEMIF devices? Its entirely conceivable
>> to have two memories like NAND and NOR flash connect to two different
>> AEMIF interfaces.
>>
> 
> It can be, but I'm not sure if it is needed. Currently I've not seen case where
> more than 2 cses were used, I mean we have 2 cs free, why do we need the second AEMIF
> controller?

One usual reason is pinmux constraints. Its probably not a concern on
the device you are working with right now but as devices get smaller,
functionality on pins is multiplexed to handle multiple different use cases.

Thanks,
Sekhar

[PATCH 08/12] memory: davinci-aemif: add bindings for AEMIF driver

[Khoronzhuk, Ivan]

Add bindings for AEMIF controller drivers/memory/davinici-aemif.c

Signed-off-by: Ivan Khoronzhuk <ivan.khoronzhuk@ti.com>
---
 .../bindings/memory-controllers/davinci-aemif.txt  |  198 ++++++++++++++++++++
 1 file changed, 198 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/memory-controllers/davinci-aemif.txt

diff --git a/Documentation/devicetree/bindings/memory-controllers/davinci-aemif.txt b/Documentation/devicetree/bindings/memory-controllers/davinci-aemif.txt
new file mode 100644
index 0000000..5e5319a
--- /dev/null
+++ b/Documentation/devicetree/bindings/memory-controllers/davinci-aemif.txt
@@ -0,0 +1,198 @@
+* Device tree bindings for Texas instruments Davinci/Keystone AEMIF controller
+
+Th Async External Memory Interface (EMIF16/AEMIF) controller is intended to
+provide a glue-less interface to a variety of asynchronous memory devices like
+ASRA M, NOR and NAND memory. A total of 256M bytes of any of these memories
+can be accessed at any given time via four chip selects with 64M byte access
+per chip select. Synchronous memories such as DDR1 SD RAM, SDR SDRAM
+and Mobile SDR are not supported.
+
+Documentation:
+Davinci DM646x - http://www.ti.com/lit/ug/sprueq7c/sprueq7c.pdf
+OMAP-L138 - http://www.ti.com/lit/ug/spruh77a/spruh77a.pdf
+Kestone - http://www.ti.com/lit/ug/sprugz3a/sprugz3a.pdf
+
+Required properties:
+
+- compatible:          "ti,davinci-aemif"
+                       "ti,keystone-aemif"
+                       "ti,omap-L138-aemif"
+
+- #address-cells:      Must be 2. The first cell is the memory partition
+                       number. The 0 partition is for chip selects. And the
+                       second cell is the offset into the partition, for the 0
+                       partition it corresponds to chip select offset.
+
+- #size-cells:         Must be set to 1.
+
+- reg:                 contains offset/length value for AEMIF control registers
+                       space.
+
+- ranges:              Must be set up to reflect the memory layout for 4
+                       chipselects and for AEMIF control range.
+
+- clocks:              phandle reference to the controller clock. Required only
+                       if clock tree data present in device tree.
+                       See clock-bindings.txt
+
+- clock-names:         clock name. It has to be "aemif". Required only if clock
+                       tree data present in device tree, in another case don't
+                       use it.
+                       See clock-bindings.txt
+
+- clock-ranges:                Empty property indicating that child nodes can inherit
+                       named clocks. Required only if clock tree data present
+                       in device tree.
+                       See clock-bindings.txt
+
+
+Child chip-select (cs) nodes contain the memory devices nodes connected to
+such as NOR (e.g. cfi-flash) and NAND (ti,davinci-nand, see davinci-nand.txt).
+There might be board specific devices like FPGAs.
+
+Required child cs node properties:
+
+- compatible:          "ti,davinci-cs"
+
+- #address-cells:      Must be 2. The first cell is the memory partition
+                       number. The 0 partition is for chip selects. And the
+                       second cell is the offset into the partition, for the 0
+                       partition it corresponds to chip select offset.
+
+- #size-cells:         Must be 1.
+
+- ranges:              Empty property indicating that child nodes can inherit
+                       memory layout.
+
+- clock-ranges:                Empty property indicating that child nodes can inherit
+                       named clocks. Required only if clock tree data present
+                       in device tree.
+
+Optional child cs node properties:
+
+
+- ti,bus-width:                        width of the asynchronous device's data bus
+                               8 or 16 if not preset 8
+
+- ti,davinci-cs-ss:            enable/disable select strobe mode
+                               In select strobe mode chip select behaves as
+                               the strobe and is active only during the strobe
+                               period. If present then enable.
+
+- ti,davinci-cs-ew:            enable/disable extended wait mode
+                               if set, the controller monitors the EMIFWAIT pin
+                               mapped to that chip select to determine if the
+                               device wants to extend the strobe period. If
+                               present then enable.
+
+- ti,davinci-cs-ta:            minimum turn around time, ns
+                               Time between the end of one asynchronous memory
+                               access and the start of another asynchronous
+                               memory access. This delay is not incurred
+                               between a read followed by read or a write
+                               followed by a write to same chip select.
+
+- ti,davinci-cs-rsetup:                read setup width, ns
+                               Time between the beginning of a memory cycle
+                               and the activation of read strobe.
+                               Minimum value is 1 (0 treated as 1).
+
+- ti,davinci-cs-rstobe:                read strobe width, ns
+                               Time between the activation and deactivation of
+                               the read strobe.
+                               Minimum value is 1 (0 treated as 1).
+
+- ti,davinci-cs-rhold:         read hold width, ns
+                               Time between the deactivation of the read
+                               strobe and the end of the cycle (which may be
+                               either an address change or the deactivation of
+                               the chip select signal.
+                               Minimum value is 1 (0 treated as 1).
+
+- ti,davinci-cs-wsetup:                write setup width, ns
+                               Time between the beginning of a memory cycle
+                               and the activation of write strobe.
+                               Minimum value is 1 (0 treated as 1).
+
+- ti,davinci-cs-wstrobe:       write strobe width, ns
+                               Time between the activation and deactivation of
+                               the write strobe.
+                               Minimum value is 1 (0 treated as 1).
+
+- ti,davinci-cs-whold:         write hold width, ns
+                               Time between the deactivation of the write
+                               strobe and the end of the cycle (which may be
+                               either an address change or the deactivation of
+                               the chip select signal.
+                               Minimum value is 1 (0 treated as 1).
+
+If any of the above parameters are absent, current parameter value will be taken
+from the corresponding HW reg.
+
+The name for cs node must be in format csN, where N is the cs number.
+For compatibles "ti,davinci-aemif" and "ti,keystone-aemif" N = [0-3].
+For compatible "ti,omap-L138-aemif" N = [2-5].
+
+Example for aemif, davinci nand and nor flash chip select shown below.
+
+memory-controller@21000A00 {
+       compatible = "ti,keystone-aemif";
+       #address-cells = <2>;
+       #size-cells = <1>;
+       clocks = <&clkaemif 0>;
+       clock-names = "aemif";
+       clock-ranges;
+       reg = <0x2100A00 0x00000100>;
+       ranges = <0 0 0x70000000 0x10000000
+                 1 0 0x21000A00 0x0000100>;
+
+       nand:cs2 {
+               compatible = "ti,davinci-cs";
+               #address-cells = <2>;
+               #size-cells = <1>;
+               clock-ranges;
+               ranges;
+
+               /* all timings in nanoseconds */
+               ti,davinci-cs-ta = <0>;
+               ti,davinci-cs-rhold = <7>;
+               ti,davinci-cs-rstrobe = <42>;
+               ti,davinci-cs-rsetup = <14>;
+               ti,davinci-cs-whold = <7>;
+               ti,davinci-cs-wstrobe = <42>;
+               ti,davinci-cs-wsetup = <14>;
+
+               nand@0,0x8000000 {
+                       compatible = "ti,davinci-nand";
+                       reg = <0 0x8000000 0x4000000
+                              1 0x0000000 0x0000100>;
+
+                       .. see davinci-nand.txt
+               };
+       };
+
+       nor:cs0 {
+               compatible = "ti,davinci-cs";
+               #address-cells = <2>;
+               #size-cells = <1>;
+               clock-ranges;
+               ranges;
+
+               /* all timings in nanoseconds */
+               ti,davinci-cs-ta = <0>;
+               ti,davinci-cs-rhold = <8>;
+               ti,davinci-cs-rstrobe = <40>;
+               ti,davinci-cs-rsetup = <14>;
+               ti,davinci-cs-whold = <7>;
+               ti,davinci-cs-wstrobe = <40>;
+               ti,davinci-cs-wsetup = <14>;
+               ti,davinci-cs-asize = <1>;
+
+               flash@0,0x0000000 {
+                       compatible = "cfi-flash";
+                       reg = <0 0x0000000 0x4000000>;
+
+                       ...
+               };
+       };
+};
--
1.7.9.5

[PATCH 09/12] mtd: nand: davinci: reuse driver for Keystone arch

[Khoronzhuk, Ivan]

The Keystone arch has compatible nand device, so reuse it.
In case with Keystone it depends on TI_DAVINCI_AEMIF because AEMIF
driver is responsible to set timings.

See http://www.ti.com/lit/ug/sprugz3a/sprugz3a.pdf

Signed-off-by: Ivan Khoronzhuk <ivan.khoronzhuk@ti.com>
---
 .../devicetree/bindings/mtd/davinci-nand.txt       |    8 +++++---
 drivers/mtd/nand/Kconfig                           |    6 +++---
 drivers/mtd/nand/davinci_nand.c                    |    1 +
 3 files changed, 9 insertions(+), 6 deletions(-)

diff --git a/Documentation/devicetree/bindings/mtd/davinci-nand.txt b/Documentation/devicetree/bindings/mtd/davinci-nand.txt
index befaa5b..cfb18ab 100644
--- a/Documentation/devicetree/bindings/mtd/davinci-nand.txt
+++ b/Documentation/devicetree/bindings/mtd/davinci-nand.txt
@@ -1,14 +1,16 @@
-Device tree bindings for Texas instruments Davinci NAND controller
+Device tree bindings for Texas instruments Davinci/Keystone NAND controller

-This file provides information, what the device node for the davinci NAND
-interface contains.
+This file provides information, what the device node for the davinci/keystone
+NAND interface contains.

 Documentation:
 Davinci DM646x - http://www.ti.com/lit/ug/sprueq7c/sprueq7c.pdf
+Kestone - http://www.ti.com/lit/ug/sprugz3a/sprugz3a.pdf

 Required properties:

 - compatible:                  "ti,davinci-nand"
+                               "ti,keystone-nand"

 - reg:                         Contains 2 offset/length values:
                                - offset and length for the access window.
diff --git a/drivers/mtd/nand/Kconfig b/drivers/mtd/nand/Kconfig
index d885298..62dbcd5 100644
--- a/drivers/mtd/nand/Kconfig
+++ b/drivers/mtd/nand/Kconfig
@@ -492,11 +492,11 @@ config MTD_NAND_SH_FLCTL
          for NAND Flash using FLCTL.

 config MTD_NAND_DAVINCI
-        tristate "Support NAND on DaVinci SoC"
-        depends on ARCH_DAVINCI
+        tristate "Support NAND on DaVinci/Keystone SoC"
+        depends on ARCH_DAVINCI || (ARCH_KEYSTONE && TI_DAVINCI_AEMIF)
         help
          Enable the driver for NAND flash chips on Texas Instruments
-         DaVinci processors.
+         DaVinci/Keystone processors.

 config MTD_NAND_TXX9NDFMC
        tristate "NAND Flash support for TXx9 SoC"
diff --git a/drivers/mtd/nand/davinci_nand.c b/drivers/mtd/nand/davinci_nand.c
index 78b8d66..4705214 100644
--- a/drivers/mtd/nand/davinci_nand.c
+++ b/drivers/mtd/nand/davinci_nand.c
@@ -523,6 +523,7 @@ static struct nand_ecclayout hwecc4_2048 __initconst = {
 #if defined(CONFIG_OF)
 static const struct of_device_id davinci_nand_of_match[] = {
        {.compatible = "ti,davinci-nand", },
+       {.compatible = "ti,keystone-nand", },
        {},
 };
 MODULE_DEVICE_TABLE(of, davinci_nand_of_match);
--
1.7.9.5

Re: [PATCH 09/12] mtd: nand: davinci: reuse driver for Keystone arch

[Santosh Shilimkar]

On Monday 11 November 2013 12:09 PM, Khoronzhuk, Ivan wrote:
> The Keystone arch has compatible nand device, so reuse it.
> In case with Keystone it depends on TI_DAVINCI_AEMIF because AEMIF
> driver is responsible to set timings.
> 
> See http://www.ti.com/lit/ug/sprugz3a/sprugz3a.pdf
> 
> Signed-off-by: Ivan Khoronzhuk <ivan.khoronzhuk@ti.com>
> ---
Acked-by: Santosh Shilimkar <santosh.shilimkar@ti.com>

[PATCH 10/12] mtd: nand: davinci: don't set timings if AEMIF is used

[Khoronzhuk, Ivan]

If Davinci AEMIF is used we don't need to set timings and bus width.
It is done by AEMIF driver (drivers/memory/davinci-aemfi.c).

Signed-off-by: Ivan Khoronzhuk <ivan.khoronzhuk@ti.com>
---
 drivers/mtd/nand/davinci_nand.c |   22 +++++++++++++++-------
 1 file changed, 15 insertions(+), 7 deletions(-)

diff --git a/drivers/mtd/nand/davinci_nand.c b/drivers/mtd/nand/davinci_nand.c
index 4705214..879e915 100644
--- a/drivers/mtd/nand/davinci_nand.c
+++ b/drivers/mtd/nand/davinci_nand.c
@@ -742,27 +742,35 @@ static int __init nand_davinci_probe(struct platform_device *pdev)
                goto err_clk_enable;
        }

+#if !IS_ENABLED(CONFIG_TI_DAVINCI_AEMIF)
        /*
-        * Setup Async configuration register in case we did not boot from
-        * NAND and so bootloader did not bother to set it up.
+        * Setup Async configuration register in case we did not boot
+        * from NAND and so bootloader did not bother to set it up.
         */
-       val = davinci_nand_readl(info, A1CR_OFFSET + info->core_chipsel * 4);
+       val = davinci_nand_readl(info, A1CR_OFFSET +
+                                info->core_chipsel * 4);

-       /* Extended Wait is not valid and Select Strobe mode is not used */
+       /*
+        * Extended Wait is not valid and Select Strobe mode is not
+        * used
+        */
        val &= ~(ACR_ASIZE_MASK | ACR_EW_MASK | ACR_SS_MASK);
        if (info->chip.options & NAND_BUSWIDTH_16)
                val |= 0x1;

-       davinci_nand_writel(info, A1CR_OFFSET + info->core_chipsel * 4, val);
+       davinci_nand_writel(info, A1CR_OFFSET +
+                           info->core_chipsel * 4, val);

        ret = 0;
        if (info->timing)
-               ret = davinci_aemif_setup_timing(info->timing, info->base,
-                                                       info->core_chipsel);
+               ret = davinci_aemif_setup_timing(info->timing,
+                                                info->base,
+                                                info->core_chipsel);
        if (ret < 0) {
                dev_dbg(&pdev->dev, "NAND timing values setup fail\n");
                goto err;
        }
+#endif

        spin_lock_irq(&davinci_nand_lock);

--
1.7.9.5

Re: [PATCH 10/12] mtd: nand: davinci: don't set timings if AEMIF is used

[Santosh Shilimkar]

On Monday 11 November 2013 12:10 PM, Khoronzhuk, Ivan wrote:
> If Davinci AEMIF is used we don't need to set timings and bus width.
> It is done by AEMIF driver (drivers/memory/davinci-aemfi.c).
> 
> Signed-off-by: Ivan Khoronzhuk <ivan.khoronzhuk@ti.com>
> ---
>  drivers/mtd/nand/davinci_nand.c |   22 +++++++++++++++-------
>  1 file changed, 15 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/mtd/nand/davinci_nand.c b/drivers/mtd/nand/davinci_nand.c
> index 4705214..879e915 100644
> --- a/drivers/mtd/nand/davinci_nand.c
> +++ b/drivers/mtd/nand/davinci_nand.c
> @@ -742,27 +742,35 @@ static int __init nand_davinci_probe(struct platform_device *pdev)
>                 goto err_clk_enable;
>         }
> 
> +#if !IS_ENABLED(CONFIG_TI_DAVINCI_AEMIF)
>
Instead above #if, just use a variable.
	bool aemif = IS_ENABLED(CONFIG_TI_DAVINCI_AEMIF) and then skip
the below code. #if block in the middle of the code looks ugly.

Other than that patch looks fine to me.

Regards,
Santosh

Re: [PATCH 10/12] mtd: nand: davinci: don't set timings if AEMIF is used

[Grygorii Strashko]

On 11/12/2013 06:13 PM, Santosh Shilimkar wrote:
> On Monday 11 November 2013 12:10 PM, Khoronzhuk, Ivan wrote:
>> If Davinci AEMIF is used we don't need to set timings and bus width.
>> It is done by AEMIF driver (drivers/memory/davinci-aemfi.c).
>>
>> Signed-off-by: Ivan Khoronzhuk <ivan.khoronzhuk@ti.com>
>> ---
>>   drivers/mtd/nand/davinci_nand.c |   22 +++++++++++++++-------
>>   1 file changed, 15 insertions(+), 7 deletions(-)
>>
>> diff --git a/drivers/mtd/nand/davinci_nand.c b/drivers/mtd/nand/davinci_nand.c
>> index 4705214..879e915 100644
>> --- a/drivers/mtd/nand/davinci_nand.c
>> +++ b/drivers/mtd/nand/davinci_nand.c
>> @@ -742,27 +742,35 @@ static int __init nand_davinci_probe(struct platform_device *pdev)
>>                  goto err_clk_enable;
>>          }
>>
>> +#if !IS_ENABLED(CONFIG_TI_DAVINCI_AEMIF)
>>
> Instead above #if, just use a variable.
> 	bool aemif = IS_ENABLED(CONFIG_TI_DAVINCI_AEMIF) and then skip
> the below code. #if block in the middle of the code looks ugly.

Yes, this is the hack.
The problem is that this part of code contains the call of Davinci 
platform function davinci_aemif_setup_timing() which is not accessible 
if Kernel is built for Keystone only.
That's why "#if" has been used.

This part of code has to be removed together with Davinci aemif platform 
code (aemif.c), once Davinci will be converted to DT and use
new driver.

The corresponding comment can be added here. Is it ok?

>
> Other than that patch looks fine to me.
>
> Regards,
> Santosh
>

Re: [PATCH 10/12] mtd: nand: davinci: don't set timings if AEMIF is used

[Santosh Shilimkar]

On Monday 18 November 2013 06:35 AM, Grygorii Strashko wrote:
> On 11/12/2013 06:13 PM, Santosh Shilimkar wrote:
>> On Monday 11 November 2013 12:10 PM, Khoronzhuk, Ivan wrote:
>>> If Davinci AEMIF is used we don't need to set timings and bus width.
>>> It is done by AEMIF driver (drivers/memory/davinci-aemfi.c).
>>>
>>> Signed-off-by: Ivan Khoronzhuk <ivan.khoronzhuk@ti.com>
>>> ---
>>>   drivers/mtd/nand/davinci_nand.c |   22 +++++++++++++++-------
>>>   1 file changed, 15 insertions(+), 7 deletions(-)
>>>
>>> diff --git a/drivers/mtd/nand/davinci_nand.c b/drivers/mtd/nand/davinci_nand.c
>>> index 4705214..879e915 100644
>>> --- a/drivers/mtd/nand/davinci_nand.c
>>> +++ b/drivers/mtd/nand/davinci_nand.c
>>> @@ -742,27 +742,35 @@ static int __init nand_davinci_probe(struct platform_device *pdev)
>>>                  goto err_clk_enable;
>>>          }
>>>
>>> +#if !IS_ENABLED(CONFIG_TI_DAVINCI_AEMIF)
>>>
>> Instead above #if, just use a variable.
>>     bool aemif = IS_ENABLED(CONFIG_TI_DAVINCI_AEMIF) and then skip
>> the below code. #if block in the middle of the code looks ugly.
> 
> Yes, this is the hack.
> The problem is that this part of code contains the call of Davinci platform function davinci_aemif_setup_timing() which is not accessible if Kernel is built for Keystone only.
> That's why "#if" has been used.
> 
> This part of code has to be removed together with Davinci aemif platform code (aemif.c), once Davinci will be converted to DT and use
> new driver.
> 
> The corresponding comment can be added here. Is it ok?
> 
Thats more or less what I proposed in another reply. Lets clearly
document it on why such a check was added.

Regards,
Santosh

Re: [PATCH 10/12] mtd: nand: davinci: don't set timings if AEMIF is used

[ivan.khoronzhuk]

On 11/18/2013 04:08 PM, Santosh Shilimkar wrote:
> On Monday 18 November 2013 06:35 AM, Grygorii Strashko wrote:
>> On 11/12/2013 06:13 PM, Santosh Shilimkar wrote:
>>> On Monday 11 November 2013 12:10 PM, Khoronzhuk, Ivan wrote:
>>>> If Davinci AEMIF is used we don't need to set timings and bus width.
>>>> It is done by AEMIF driver (drivers/memory/davinci-aemfi.c).
>>>>
>>>> Signed-off-by: Ivan Khoronzhuk <ivan.khoronzhuk@ti.com>
>>>> ---
>>>>    drivers/mtd/nand/davinci_nand.c |   22 +++++++++++++++-------
>>>>    1 file changed, 15 insertions(+), 7 deletions(-)
>>>>
>>>> diff --git a/drivers/mtd/nand/davinci_nand.c b/drivers/mtd/nand/davinci_nand.c
>>>> index 4705214..879e915 100644
>>>> --- a/drivers/mtd/nand/davinci_nand.c
>>>> +++ b/drivers/mtd/nand/davinci_nand.c
>>>> @@ -742,27 +742,35 @@ static int __init nand_davinci_probe(struct platform_device *pdev)
>>>>                   goto err_clk_enable;
>>>>           }
>>>>
>>>> +#if !IS_ENABLED(CONFIG_TI_DAVINCI_AEMIF)
>>>>
>>> Instead above #if, just use a variable.
>>>      bool aemif = IS_ENABLED(CONFIG_TI_DAVINCI_AEMIF) and then skip
>>> the below code. #if block in the middle of the code looks ugly.
>>
>> Yes, this is the hack.
>> The problem is that this part of code contains the call of Davinci platform function davinci_aemif_setup_timing() which is not accessible if Kernel is built for Keystone only.
>> That's why "#if" has been used.
>>
>> This part of code has to be removed together with Davinci aemif platform code (aemif.c), once Davinci will be converted to DT and use
>> new driver.
>>
>> The corresponding comment can be added here. Is it ok?
>>
> Thats more or less what I proposed in another reply. Lets clearly
> document it on why such a check was added.
>
> Regards,
> Santosh
>

Ok, I'll add more explanation on it.

-- 
Regards,
Ivan Khoronzhuk

Re: [PATCH 10/12] mtd: nand: davinci: don't set timings if AEMIF is used

[Sekhar Nori]

On Monday 11 November 2013 10:40 PM, Khoronzhuk, Ivan wrote:
> If Davinci AEMIF is used we don't need to set timings and bus width.
> It is done by AEMIF driver (drivers/memory/davinci-aemfi.c).
> 
> Signed-off-by: Ivan Khoronzhuk <ivan.khoronzhuk@ti.com>
> ---
>  drivers/mtd/nand/davinci_nand.c |   22 +++++++++++++++-------
>  1 file changed, 15 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/mtd/nand/davinci_nand.c b/drivers/mtd/nand/davinci_nand.c
> index 4705214..879e915 100644
> --- a/drivers/mtd/nand/davinci_nand.c
> +++ b/drivers/mtd/nand/davinci_nand.c
> @@ -742,27 +742,35 @@ static int __init nand_davinci_probe(struct platform_device *pdev)
>                 goto err_clk_enable;
>         }
> 
> +#if !IS_ENABLED(CONFIG_TI_DAVINCI_AEMIF)

This is a hack! Just because AEMIF driver is enabled, it does not
guarantee that the timings have been setup by it. Instead of configuring
timings in two drivers, why not just convert everyone over to use the
new driver. Dont worry about breaking old platforms, I will help test
and ack them.

Thanks,
Sekhar

Re: [PATCH 10/12] mtd: nand: davinci: don't set timings if AEMIF is used

[Santosh Shilimkar]

On Wednesday 13 November 2013 12:02 AM, Sekhar Nori wrote:
> On Monday 11 November 2013 10:40 PM, Khoronzhuk, Ivan wrote:
>> If Davinci AEMIF is used we don't need to set timings and bus width.
>> It is done by AEMIF driver (drivers/memory/davinci-aemfi.c).
>>
>> Signed-off-by: Ivan Khoronzhuk <ivan.khoronzhuk@ti.com>
>> ---
>>  drivers/mtd/nand/davinci_nand.c |   22 +++++++++++++++-------
>>  1 file changed, 15 insertions(+), 7 deletions(-)
>>
>> diff --git a/drivers/mtd/nand/davinci_nand.c b/drivers/mtd/nand/davinci_nand.c
>> index 4705214..879e915 100644
>> --- a/drivers/mtd/nand/davinci_nand.c
>> +++ b/drivers/mtd/nand/davinci_nand.c
>> @@ -742,27 +742,35 @@ static int __init nand_davinci_probe(struct platform_device *pdev)
>>                 goto err_clk_enable;
>>         }
>>
>> +#if !IS_ENABLED(CONFIG_TI_DAVINCI_AEMIF)
> 
> This is a hack! Just because AEMIF driver is enabled, it does not
> guarantee that the timings have been setup by it. Instead of configuring
> timings in two drivers, why not just convert everyone over to use the
> new driver. Dont worry about breaking old platforms, I will help test
> and ack them.
> 
How about you take a stab at and convert the DaVinci code over to make
use of new driver. We are trying to re-use as much as possible from the
common drivers and also making an option so that the DaVinci arch can
move over to these drivers if they want to.

Thanks.

Regards,
Santosh

Re: [PATCH 10/12] mtd: nand: davinci: don't set timings if AEMIF is used

[Sekhar Nori]

On Wednesday 13 November 2013 07:44 PM, Santosh Shilimkar wrote:
> On Wednesday 13 November 2013 12:02 AM, Sekhar Nori wrote:
>> On Monday 11 November 2013 10:40 PM, Khoronzhuk, Ivan wrote:
>>> If Davinci AEMIF is used we don't need to set timings and bus width.
>>> It is done by AEMIF driver (drivers/memory/davinci-aemfi.c).
>>>
>>> Signed-off-by: Ivan Khoronzhuk <ivan.khoronzhuk@ti.com>
>>> ---
>>>  drivers/mtd/nand/davinci_nand.c |   22 +++++++++++++++-------
>>>  1 file changed, 15 insertions(+), 7 deletions(-)
>>>
>>> diff --git a/drivers/mtd/nand/davinci_nand.c b/drivers/mtd/nand/davinci_nand.c
>>> index 4705214..879e915 100644
>>> --- a/drivers/mtd/nand/davinci_nand.c
>>> +++ b/drivers/mtd/nand/davinci_nand.c
>>> @@ -742,27 +742,35 @@ static int __init nand_davinci_probe(struct platform_device *pdev)
>>>                 goto err_clk_enable;
>>>         }
>>>
>>> +#if !IS_ENABLED(CONFIG_TI_DAVINCI_AEMIF)
>>
>> This is a hack! Just because AEMIF driver is enabled, it does not
>> guarantee that the timings have been setup by it. Instead of configuring
>> timings in two drivers, why not just convert everyone over to use the
>> new driver. Dont worry about breaking old platforms, I will help test
>> and ack them.
>>
> How about you take a stab at and convert the DaVinci code over to make
> use of new driver. We are trying to re-use as much as possible from the
> common drivers and also making an option so that the DaVinci arch can
> move over to these drivers if they want to.

Sure I could.

Ivan,

The AEMIF driver does not use platfrom_data currently. Is that something
you can add or you want me to take a stab at that as well?

Thanks,
Sekhar

Re: [PATCH 10/12] mtd: nand: davinci: don't set timings if AEMIF is used

[Santosh Shilimkar]

On Thursday 14 November 2013 05:53 AM, Nori, Sekhar wrote:
> On Wednesday 13 November 2013 07:44 PM, Santosh Shilimkar wrote:
>> On Wednesday 13 November 2013 12:02 AM, Sekhar Nori wrote:
>>> On Monday 11 November 2013 10:40 PM, Khoronzhuk, Ivan wrote:
>>>> If Davinci AEMIF is used we don't need to set timings and bus width.
>>>> It is done by AEMIF driver (drivers/memory/davinci-aemfi.c).
>>>>
>>>> Signed-off-by: Ivan Khoronzhuk <ivan.khoronzhuk@ti.com>
>>>> ---
>>>>  drivers/mtd/nand/davinci_nand.c |   22 +++++++++++++++-------
>>>>  1 file changed, 15 insertions(+), 7 deletions(-)
>>>>
>>>> diff --git a/drivers/mtd/nand/davinci_nand.c b/drivers/mtd/nand/davinci_nand.c
>>>> index 4705214..879e915 100644
>>>> --- a/drivers/mtd/nand/davinci_nand.c
>>>> +++ b/drivers/mtd/nand/davinci_nand.c
>>>> @@ -742,27 +742,35 @@ static int __init nand_davinci_probe(struct platform_device *pdev)
>>>>                 goto err_clk_enable;
>>>>         }
>>>>
>>>> +#if !IS_ENABLED(CONFIG_TI_DAVINCI_AEMIF)
>>>
>>> This is a hack! Just because AEMIF driver is enabled, it does not
>>> guarantee that the timings have been setup by it. Instead of configuring
>>> timings in two drivers, why not just convert everyone over to use the
>>> new driver. Dont worry about breaking old platforms, I will help test
>>> and ack them.
>>>
>> How about you take a stab at and convert the DaVinci code over to make
>> use of new driver. We are trying to re-use as much as possible from the
>> common drivers and also making an option so that the DaVinci arch can
>> move over to these drivers if they want to.
> 
> Sure I could.
> 
Thanks

> Ivan,
> 
> The AEMIF driver does not use platfrom_data currently. Is that something
> you can add or you want me to take a stab at that as well?
> 
The AEMIF new driver is device tree only and thats the direction. So the
better thing would be to convert Davinci to DT and then make use of this
new driver. Thats how most of the new drivers used on arm socs are
moving. Adding platform data to new driver is a step in backward
direction so I would want to avoid that.

Once you start using new driver the $subject patch code skipping
is trivial. Till then I would like to proceed with current approach
which allows Nand support for Keystone.

Regards,
Santosh

Re: [PATCH 10/12] mtd: nand: davinci: don't set timings if AEMIF is used

[ivan.khoronzhuk]

On 11/14/2013 04:36 PM, Santosh Shilimkar wrote:
> On Thursday 14 November 2013 05:53 AM, Nori, Sekhar wrote:
>> On Wednesday 13 November 2013 07:44 PM, Santosh Shilimkar wrote:
>>> On Wednesday 13 November 2013 12:02 AM, Sekhar Nori wrote:
>>>> On Monday 11 November 2013 10:40 PM, Khoronzhuk, Ivan wrote:
>>>>> If Davinci AEMIF is used we don't need to set timings and bus width.
>>>>> It is done by AEMIF driver (drivers/memory/davinci-aemfi.c).
>>>>>
>>>>> Signed-off-by: Ivan Khoronzhuk <ivan.khoronzhuk@ti.com>
>>>>> ---
>>>>>   drivers/mtd/nand/davinci_nand.c |   22 +++++++++++++++-------
>>>>>   1 file changed, 15 insertions(+), 7 deletions(-)
>>>>>
>>>>> diff --git a/drivers/mtd/nand/davinci_nand.c b/drivers/mtd/nand/davinci_nand.c
>>>>> index 4705214..879e915 100644
>>>>> --- a/drivers/mtd/nand/davinci_nand.c
>>>>> +++ b/drivers/mtd/nand/davinci_nand.c
>>>>> @@ -742,27 +742,35 @@ static int __init nand_davinci_probe(struct platform_device *pdev)
>>>>>                  goto err_clk_enable;
>>>>>          }
>>>>>
>>>>> +#if !IS_ENABLED(CONFIG_TI_DAVINCI_AEMIF)
>>>>
>>>> This is a hack! Just because AEMIF driver is enabled, it does not
>>>> guarantee that the timings have been setup by it. Instead of configuring
>>>> timings in two drivers, why not just convert everyone over to use the
>>>> new driver. Dont worry about breaking old platforms, I will help test
>>>> and ack them.
>>>>
>>> How about you take a stab at and convert the DaVinci code over to make
>>> use of new driver. We are trying to re-use as much as possible from the
>>> common drivers and also making an option so that the DaVinci arch can
>>> move over to these drivers if they want to.
>>
>> Sure I could.
>>
> Thanks
>
>> Ivan,
>>
>> The AEMIF driver does not use platfrom_data currently. Is that something
>> you can add or you want me to take a stab at that as well?
>>
> The AEMIF new driver is device tree only and thats the direction. So the
> better thing would be to convert Davinci to DT and then make use of this
> new driver. Thats how most of the new drivers used on arm socs are
> moving. Adding platform data to new driver is a step in backward
> direction so I would want to avoid that.
>
> Once you start using new driver the $subject patch code skipping
> is trivial. Till then I would like to proceed with current approach
> which allows Nand support for Keystone.
>
> Regards,
> Santosh
>

Agree with Santosh

-- 
Regards,
Ivan Khoronzhuk

Re: [PATCH 10/12] mtd: nand: davinci: don't set timings if AEMIF is used

[Sekhar Nori]

On 11/14/2013 8:06 PM, Santosh Shilimkar wrote:
> On Thursday 14 November 2013 05:53 AM, Nori, Sekhar wrote:
>> On Wednesday 13 November 2013 07:44 PM, Santosh Shilimkar wrote:
>>> On Wednesday 13 November 2013 12:02 AM, Sekhar Nori wrote:
>>>> On Monday 11 November 2013 10:40 PM, Khoronzhuk, Ivan wrote:
>>>>> If Davinci AEMIF is used we don't need to set timings and bus width.
>>>>> It is done by AEMIF driver (drivers/memory/davinci-aemfi.c).
>>>>>
>>>>> Signed-off-by: Ivan Khoronzhuk <ivan.khoronzhuk@ti.com>
>>>>> ---
>>>>>  drivers/mtd/nand/davinci_nand.c |   22 +++++++++++++++-------
>>>>>  1 file changed, 15 insertions(+), 7 deletions(-)
>>>>>
>>>>> diff --git a/drivers/mtd/nand/davinci_nand.c b/drivers/mtd/nand/davinci_nand.c
>>>>> index 4705214..879e915 100644
>>>>> --- a/drivers/mtd/nand/davinci_nand.c
>>>>> +++ b/drivers/mtd/nand/davinci_nand.c
>>>>> @@ -742,27 +742,35 @@ static int __init nand_davinci_probe(struct platform_device *pdev)
>>>>>                 goto err_clk_enable;
>>>>>         }
>>>>>
>>>>> +#if !IS_ENABLED(CONFIG_TI_DAVINCI_AEMIF)
>>>>
>>>> This is a hack! Just because AEMIF driver is enabled, it does not
>>>> guarantee that the timings have been setup by it. Instead of configuring
>>>> timings in two drivers, why not just convert everyone over to use the
>>>> new driver. Dont worry about breaking old platforms, I will help test
>>>> and ack them.
>>>>
>>> How about you take a stab at and convert the DaVinci code over to make
>>> use of new driver. We are trying to re-use as much as possible from the
>>> common drivers and also making an option so that the DaVinci arch can
>>> move over to these drivers if they want to.
>>
>> Sure I could.
>>
> Thanks
> 
>> Ivan,
>>
>> The AEMIF driver does not use platfrom_data currently. Is that something
>> you can add or you want me to take a stab at that as well?
>>
> The AEMIF new driver is device tree only and thats the direction. So the
> better thing would be to convert Davinci to DT and then make use of this
> new driver. Thats how most of the new drivers used on arm socs are
> moving. Adding platform data to new driver is a step in backward
> direction so I would want to avoid that.

Yes, that would be the ideal way. But the reality is that there is close
to zero chance of all the DaVinci platforms using AEMIF ever getting
converted to DT. This means that we will never be able to get rid of
this piece of code.

I do agree platform data is not the direction you want to take on ARM
but at the same time its not really a deprecated mechanism as far as the
broader kernel is concerned.

So, can we consider adding platform data mechanism to the AEMIF driver?

Thanks,
Sekhar

Re: [PATCH 10/12] mtd: nand: davinci: don't set timings if AEMIF is used

[Santosh Shilimkar]

On Thursday 21 November 2013 12:07 PM, Nori, Sekhar wrote:
> On 11/14/2013 8:06 PM, Santosh Shilimkar wrote:
>> On Thursday 14 November 2013 05:53 AM, Nori, Sekhar wrote:
>>> On Wednesday 13 November 2013 07:44 PM, Santosh Shilimkar wrote:
>>>> On Wednesday 13 November 2013 12:02 AM, Sekhar Nori wrote:
>>>>> On Monday 11 November 2013 10:40 PM, Khoronzhuk, Ivan wrote:
>>>>>> If Davinci AEMIF is used we don't need to set timings and bus width.
>>>>>> It is done by AEMIF driver (drivers/memory/davinci-aemfi.c).
>>>>>>
>>>>>> Signed-off-by: Ivan Khoronzhuk <ivan.khoronzhuk@ti.com>
>>>>>> ---
>>>>>>  drivers/mtd/nand/davinci_nand.c |   22 +++++++++++++++-------
>>>>>>  1 file changed, 15 insertions(+), 7 deletions(-)
>>>>>>
>>>>>> diff --git a/drivers/mtd/nand/davinci_nand.c b/drivers/mtd/nand/davinci_nand.c
>>>>>> index 4705214..879e915 100644
>>>>>> --- a/drivers/mtd/nand/davinci_nand.c
>>>>>> +++ b/drivers/mtd/nand/davinci_nand.c
>>>>>> @@ -742,27 +742,35 @@ static int __init nand_davinci_probe(struct platform_device *pdev)
>>>>>>                 goto err_clk_enable;
>>>>>>         }
>>>>>>
>>>>>> +#if !IS_ENABLED(CONFIG_TI_DAVINCI_AEMIF)
>>>>>
>>>>> This is a hack! Just because AEMIF driver is enabled, it does not
>>>>> guarantee that the timings have been setup by it. Instead of configuring
>>>>> timings in two drivers, why not just convert everyone over to use the
>>>>> new driver. Dont worry about breaking old platforms, I will help test
>>>>> and ack them.
>>>>>
>>>> How about you take a stab at and convert the DaVinci code over to make
>>>> use of new driver. We are trying to re-use as much as possible from the
>>>> common drivers and also making an option so that the DaVinci arch can
>>>> move over to these drivers if they want to.
>>>
>>> Sure I could.
>>>
>> Thanks
>>
>>> Ivan,
>>>
>>> The AEMIF driver does not use platfrom_data currently. Is that something
>>> you can add or you want me to take a stab at that as well?
>>>
>> The AEMIF new driver is device tree only and thats the direction. So the
>> better thing would be to convert Davinci to DT and then make use of this
>> new driver. Thats how most of the new drivers used on arm socs are
>> moving. Adding platform data to new driver is a step in backward
>> direction so I would want to avoid that.
> 
> Yes, that would be the ideal way. But the reality is that there is close
> to zero chance of all the DaVinci platforms using AEMIF ever getting
> converted to DT. This means that we will never be able to get rid of
> this piece of code.
>
Well that means some of those platforms support will get deprecated
soon considering other basic frameworks like irq, timer wheel etc
is assuming the DT direction. But its your call because if you
want those platforms to live, you need to convert them to DT.
 
> I do agree platform data is not the direction you want to take on ARM
> but at the same time its not really a deprecated mechanism as far as the
> broader kernel is concerned.
> 
> So, can we consider adding platform data mechanism to the AEMIF driver?
> 
Sorry but answer is "NO". If the above hack is really bothering you,
we can actually rip out the above code in question and move it
to mach-davinci. Infact thats the best direction to make the
mtd nandn driver independent of any platform quirks(dt, pdata etc)

Ivan already has a patch to do that and he is going to post
that patch. With that patch, we won't need $subject patch.

Regards,
Santosh

Re: [PATCH 10/12] mtd: nand: davinci: don't set timings if AEMIF is used

[Sekhar Nori]

On Friday 22 November 2013 11:08 PM, Santosh Shilimkar wrote:
> On Thursday 21 November 2013 12:07 PM, Nori, Sekhar wrote:
>> On 11/14/2013 8:06 PM, Santosh Shilimkar wrote:
>>> On Thursday 14 November 2013 05:53 AM, Nori, Sekhar wrote:
>>>> On Wednesday 13 November 2013 07:44 PM, Santosh Shilimkar wrote:
>>>>> On Wednesday 13 November 2013 12:02 AM, Sekhar Nori wrote:
>>>>>> On Monday 11 November 2013 10:40 PM, Khoronzhuk, Ivan wrote:
>>>>>>> If Davinci AEMIF is used we don't need to set timings and bus width.
>>>>>>> It is done by AEMIF driver (drivers/memory/davinci-aemfi.c).
>>>>>>>
>>>>>>> Signed-off-by: Ivan Khoronzhuk <ivan.khoronzhuk@ti.com>
>>>>>>> ---
>>>>>>>  drivers/mtd/nand/davinci_nand.c |   22 +++++++++++++++-------
>>>>>>>  1 file changed, 15 insertions(+), 7 deletions(-)
>>>>>>>
>>>>>>> diff --git a/drivers/mtd/nand/davinci_nand.c b/drivers/mtd/nand/davinci_nand.c
>>>>>>> index 4705214..879e915 100644
>>>>>>> --- a/drivers/mtd/nand/davinci_nand.c
>>>>>>> +++ b/drivers/mtd/nand/davinci_nand.c
>>>>>>> @@ -742,27 +742,35 @@ static int __init nand_davinci_probe(struct platform_device *pdev)
>>>>>>>                 goto err_clk_enable;
>>>>>>>         }
>>>>>>>
>>>>>>> +#if !IS_ENABLED(CONFIG_TI_DAVINCI_AEMIF)
>>>>>>
>>>>>> This is a hack! Just because AEMIF driver is enabled, it does not
>>>>>> guarantee that the timings have been setup by it. Instead of configuring
>>>>>> timings in two drivers, why not just convert everyone over to use the
>>>>>> new driver. Dont worry about breaking old platforms, I will help test
>>>>>> and ack them.
>>>>>>
>>>>> How about you take a stab at and convert the DaVinci code over to make
>>>>> use of new driver. We are trying to re-use as much as possible from the
>>>>> common drivers and also making an option so that the DaVinci arch can
>>>>> move over to these drivers if they want to.
>>>>
>>>> Sure I could.
>>>>
>>> Thanks
>>>
>>>> Ivan,
>>>>
>>>> The AEMIF driver does not use platfrom_data currently. Is that something
>>>> you can add or you want me to take a stab at that as well?
>>>>
>>> The AEMIF new driver is device tree only and thats the direction. So the
>>> better thing would be to convert Davinci to DT and then make use of this
>>> new driver. Thats how most of the new drivers used on arm socs are
>>> moving. Adding platform data to new driver is a step in backward
>>> direction so I would want to avoid that.
>>
>> Yes, that would be the ideal way. But the reality is that there is close
>> to zero chance of all the DaVinci platforms using AEMIF ever getting
>> converted to DT. This means that we will never be able to get rid of
>> this piece of code.
>>
> Well that means some of those platforms support will get deprecated
> soon considering other basic frameworks like irq, timer wheel etc
> is assuming the DT direction. But its your call because if you
> want those platforms to live, you need to convert them to DT.
>  
>> I do agree platform data is not the direction you want to take on ARM
>> but at the same time its not really a deprecated mechanism as far as the
>> broader kernel is concerned.
>>
>> So, can we consider adding platform data mechanism to the AEMIF driver?
>>
> Sorry but answer is "NO". If the above hack is really bothering you,
> we can actually rip out the above code in question and move it
> to mach-davinci. Infact thats the best direction to make the
> mtd nandn driver independent of any platform quirks(dt, pdata etc)
> 
> Ivan already has a patch to do that and he is going to post
> that patch. With that patch, we won't need $subject patch.
> 

Okay I will wait for the patch.

Thanks,
Sekhar

[PATCH] ARM: davinci: aemif: get rid of davinci-nand driver dependency on aemif

[Ivan Khoronzhuk]

The problem that the set timings code contains the call of Davinci
platform function davinci_aemif_setup_timing() which is not
accessible if kernel is built for another platform like Keystone.

The Keysone platform is going to use TI AEMIF driver.
If TI AEMIF is used we don't need to set timings and bus width.
It is done by AEMIF driver.

To get rid of davinci-nand driver dependency on aemif platform code
we moved aemif code to davinci platform.

The platform AEMIF code (aemif.c) has to be removed once Davinci
will be converted to DT and use ti-aemif.c driver.

Signed-off-by: Ivan Khoronzhuk <ivan.khoronzhuk@ti.com>
---
 arch/arm/mach-davinci/aemif.c                   |   73 ++++++++++++++++++++++-
 arch/arm/mach-davinci/board-da830-evm.c         |    3 +
 arch/arm/mach-davinci/board-da850-evm.c         |    3 +
 arch/arm/mach-davinci/board-dm355-evm.c         |    5 ++
 arch/arm/mach-davinci/board-dm355-leopard.c     |    5 ++
 arch/arm/mach-davinci/board-dm365-evm.c         |    4 ++
 arch/arm/mach-davinci/board-dm644x-evm.c        |    5 ++
 arch/arm/mach-davinci/board-dm646x-evm.c        |    3 +
 arch/arm/mach-davinci/board-mityomapl138.c      |    3 +
 arch/arm/mach-davinci/board-neuros-osd2.c       |    9 ++-
 arch/arm/mach-davinci/devices-tnetv107x.c       |    3 +
 drivers/mtd/nand/davinci_nand.c                 |   23 -------
 include/linux/platform_data/mtd-davinci-aemif.h |    5 +-
 13 files changed, 116 insertions(+), 28 deletions(-)

diff --git a/arch/arm/mach-davinci/aemif.c b/arch/arm/mach-davinci/aemif.c
index f091a90..51b14c6 100644
--- a/arch/arm/mach-davinci/aemif.c
+++ b/arch/arm/mach-davinci/aemif.c
@@ -16,6 +16,7 @@
 #include <linux/time.h>
 
 #include <linux/platform_data/mtd-davinci-aemif.h>
+#include <linux/platform_data/mtd-davinci.h>
 
 /* Timing value configuration */
 
@@ -43,6 +44,17 @@
 				WSTROBE(WSTROBE_MAX) | \
 				WSETUP(WSETUP_MAX))
 
+static inline unsigned int davinci_aemif_readl(void __iomem *base, int offset)
+{
+	return readl_relaxed(base + offset);
+}
+
+static inline void davinci_aemif_writel(void __iomem *base,
+					int offset, unsigned long value)
+{
+	writel_relaxed(value, base + offset);
+}
+
 /*
  * aemif_calc_rate - calculate timing data.
  * @wanted: The cycle time needed in nanoseconds.
@@ -86,7 +98,7 @@ static int aemif_calc_rate(int wanted, unsigned long clk, int max)
  *
  * Returns 0 on success, else negative errno.
  */
-int davinci_aemif_setup_timing(struct davinci_aemif_timing *t,
+static int davinci_aemif_setup_timing(struct davinci_aemif_timing *t,
 					void __iomem *base, unsigned cs)
 {
 	unsigned set, val;
@@ -130,4 +142,61 @@ int davinci_aemif_setup_timing(struct davinci_aemif_timing *t,
 
 	return 0;
 }
-EXPORT_SYMBOL(davinci_aemif_setup_timing);
+
+/**
+ * davinci_aemif_setup - setup AEMIF interface by davinci_nand_pdata
+ * @pdev - link to platform device to setup settings for
+ *
+ * This function does not use any locking while programming the AEMIF
+ * because it is expected that there is only one user of a given
+ * chip-select.
+ *
+ * Returns 0 on success, else negative errno.
+ */
+int davinci_aemif_setup(struct platform_device *pdev)
+{
+	struct davinci_nand_pdata *pdata = dev_get_platdata(&pdev->dev);
+	uint32_t val;
+	struct resource	*res;
+	void __iomem *base;
+	int ret = 0;
+
+	res = platform_get_resource(pdev, IORESOURCE_MEM, 1);
+	if (!res) {
+		dev_err(&pdev->dev, "cannot get IORESOURCE_MEM\n");
+		return -ENOMEM;
+	}
+
+	base = ioremap(res->start, resource_size(res));
+	if (!base) {
+		dev_err(&pdev->dev, "ioremap failed for resource %pR\n", res);
+		ret = -ENOMEM;
+		goto err;
+	}
+
+	/*
+	 * Setup Async configuration register in case we did not boot
+	 * from NAND and so bootloader did not bother to set it up.
+	 */
+	val = davinci_aemif_readl(base, A1CR_OFFSET + pdev->id * 4);
+	/*
+	 * Extended Wait is not valid and Select Strobe mode is not
+	 * used
+	 */
+	val &= ~(ACR_ASIZE_MASK | ACR_EW_MASK | ACR_SS_MASK);
+	if (pdata->options & NAND_BUSWIDTH_16)
+		val |= 0x1;
+
+	davinci_aemif_writel(base, A1CR_OFFSET + pdev->id * 4, val);
+
+	if (pdata->timing)
+		ret = davinci_aemif_setup_timing(pdata->timing, base, pdev->id);
+
+	if (ret < 0)
+		dev_dbg(&pdev->dev, "NAND timing values setup fail\n");
+
+err:
+	iounmap(base);
+	return ret;
+}
+EXPORT_SYMBOL(davinci_aemif_setup);
diff --git a/arch/arm/mach-davinci/board-da830-evm.c b/arch/arm/mach-davinci/board-da830-evm.c
index d1f45af..5623131 100644
--- a/arch/arm/mach-davinci/board-da830-evm.c
+++ b/arch/arm/mach-davinci/board-da830-evm.c
@@ -419,6 +419,9 @@ static inline void da830_evm_init_nand(int mux_mode)
 	if (ret)
 		pr_warning("da830_evm_init: NAND device not registered.\n");
 
+	if (davinci_aemif_setup(&da830_evm_nand_device))
+		pr_warn("%s: Cannot configure AEMIF.\n", __func__);
+
 	gpio_direction_output(mux_mode, 1);
 }
 #else
diff --git a/arch/arm/mach-davinci/board-da850-evm.c b/arch/arm/mach-davinci/board-da850-evm.c
index e0af0ec..234c5bb 100644
--- a/arch/arm/mach-davinci/board-da850-evm.c
+++ b/arch/arm/mach-davinci/board-da850-evm.c
@@ -358,6 +358,9 @@ static inline void da850_evm_setup_nor_nand(void)
 
 		platform_add_devices(da850_evm_devices,
 					ARRAY_SIZE(da850_evm_devices));
+
+		if (davinci_aemif_setup(&da850_evm_nandflash_device))
+			pr_warn("%s: Cannot configure AEMIF.\n", __func__);
 	}
 }
 
diff --git a/arch/arm/mach-davinci/board-dm355-evm.c b/arch/arm/mach-davinci/board-dm355-evm.c
index ecdc7d4..f35095f 100644
--- a/arch/arm/mach-davinci/board-dm355-evm.c
+++ b/arch/arm/mach-davinci/board-dm355-evm.c
@@ -27,6 +27,7 @@
 #include <linux/platform_data/mtd-davinci.h>
 #include <linux/platform_data/mmc-davinci.h>
 #include <linux/platform_data/usb-davinci.h>
+#include <linux/platform_data/mtd-davinci-aemif.h>
 
 #include <asm/mach-types.h>
 #include <asm/mach/arch.h>
@@ -395,6 +396,10 @@ static __init void dm355_evm_init(void)
 
 	platform_add_devices(davinci_evm_devices,
 			     ARRAY_SIZE(davinci_evm_devices));
+
+	if (davinci_aemif_setup(&davinci_nand_device))
+		pr_warn("%s: Cannot configure AEMIF.\n", __func__);
+
 	evm_init_i2c();
 	davinci_serial_init(dm355_serial_device);
 
diff --git a/arch/arm/mach-davinci/board-dm355-leopard.c b/arch/arm/mach-davinci/board-dm355-leopard.c
index 43bacbf..7f45c5a 100644
--- a/arch/arm/mach-davinci/board-dm355-leopard.c
+++ b/arch/arm/mach-davinci/board-dm355-leopard.c
@@ -23,6 +23,7 @@
 #include <linux/platform_data/mmc-davinci.h>
 #include <linux/platform_data/mtd-davinci.h>
 #include <linux/platform_data/usb-davinci.h>
+#include <linux/platform_data/mtd-davinci-aemif.h>
 
 #include <asm/mach-types.h>
 #include <asm/mach/arch.h>
@@ -253,6 +254,10 @@ static __init void dm355_leopard_init(void)
 
 	platform_add_devices(davinci_leopard_devices,
 			     ARRAY_SIZE(davinci_leopard_devices));
+
+	if (davinci_aemif_setup(&davinci_nand_device))
+		pr_warn("%s: Cannot configure AEMIF.\n", __func__);
+
 	leopard_init_i2c();
 	davinci_serial_init(dm355_serial_device);
 
diff --git a/arch/arm/mach-davinci/board-dm365-evm.c b/arch/arm/mach-davinci/board-dm365-evm.c
index e08a868..88c01c3 100644
--- a/arch/arm/mach-davinci/board-dm365-evm.c
+++ b/arch/arm/mach-davinci/board-dm365-evm.c
@@ -39,6 +39,7 @@
 #include <linux/platform_data/mmc-davinci.h>
 #include <linux/platform_data/mtd-davinci.h>
 #include <linux/platform_data/keyscan-davinci.h>
+#include <linux/platform_data/mtd-davinci-aemif.h>
 
 #include <media/ths7303.h>
 #include <media/tvp514x.h>
@@ -665,6 +666,9 @@ fail:
 
 		platform_add_devices(dm365_evm_nand_devices,
 				ARRAY_SIZE(dm365_evm_nand_devices));
+
+		if (davinci_aemif_setup(&davinci_nand_device))
+			pr_warn("%s: Cannot configure AEMIF.\n", __func__);
 	} else {
 		/* no OneNAND support yet */
 	}
diff --git a/arch/arm/mach-davinci/board-dm644x-evm.c b/arch/arm/mach-davinci/board-dm644x-evm.c
index 987605b7..5602957 100644
--- a/arch/arm/mach-davinci/board-dm644x-evm.c
+++ b/arch/arm/mach-davinci/board-dm644x-evm.c
@@ -778,6 +778,11 @@ static __init void davinci_evm_init(void)
 		/* only one device will be jumpered and detected */
 		if (HAS_NAND) {
 			platform_device_register(&davinci_evm_nandflash_device);
+
+			if (davinci_aemif_setup(&davinci_evm_nandflash_device))
+				pr_warn("%s: Cannot configure AEMIF.\n",
+					__func__);
+
 			evm_leds[7].default_trigger = "nand-disk";
 			if (HAS_NOR)
 				pr_warning("WARNING: both NAND and NOR flash "
diff --git a/arch/arm/mach-davinci/board-dm646x-evm.c b/arch/arm/mach-davinci/board-dm646x-evm.c
index 13d0801..ae129bc 100644
--- a/arch/arm/mach-davinci/board-dm646x-evm.c
+++ b/arch/arm/mach-davinci/board-dm646x-evm.c
@@ -805,6 +805,9 @@ static __init void evm_init(void)
 
 	platform_device_register(&davinci_nand_device);
 
+	if (davinci_aemif_setup(&davinci_nand_device))
+		pr_warn("%s: Cannot configure AEMIF.\n", __func__);
+
 	dm646x_init_edma(dm646x_edma_rsv);
 
 	if (HAS_ATA)
diff --git a/arch/arm/mach-davinci/board-mityomapl138.c b/arch/arm/mach-davinci/board-mityomapl138.c
index 7aa105b..98a66ff 100644
--- a/arch/arm/mach-davinci/board-mityomapl138.c
+++ b/arch/arm/mach-davinci/board-mityomapl138.c
@@ -432,6 +432,9 @@ static void __init mityomapl138_setup_nand(void)
 {
 	platform_add_devices(mityomapl138_devices,
 				 ARRAY_SIZE(mityomapl138_devices));
+
+	if (davinci_aemif_setup(&mityomapl138_nandflash_device))
+		pr_warn("%s: Cannot configure AEMIF.\n", __func__);
 }
 
 static const short mityomap_mii_pins[] = {
diff --git a/arch/arm/mach-davinci/board-neuros-osd2.c b/arch/arm/mach-davinci/board-neuros-osd2.c
index bb680af..a7d6668 100644
--- a/arch/arm/mach-davinci/board-neuros-osd2.c
+++ b/arch/arm/mach-davinci/board-neuros-osd2.c
@@ -31,6 +31,7 @@
 #include <linux/platform_data/mmc-davinci.h>
 #include <linux/platform_data/mtd-davinci.h>
 #include <linux/platform_data/usb-davinci.h>
+#include <linux/platform_data/mtd-davinci-aemif.h>
 
 #include <asm/mach-types.h>
 #include <asm/mach/arch.h>
@@ -192,9 +193,15 @@ static __init void davinci_ntosd2_init(void)
 		davinci_cfg_reg(DM644X_ATAEN_DISABLE);
 
 		/* only one device will be jumpered and detected */
-		if (HAS_NAND)
+		if (HAS_NAND) {
 			platform_device_register(
 					&davinci_ntosd2_nandflash_device);
+
+			if (davinci_aemif_setup(
+				&davinci_ntosd2_nandflash_device))
+				pr_warn("%s: Cannot configure AEMIF.\n",
+					__func__);
+		}
 	}
 
 	platform_add_devices(davinci_ntosd2_devices,
diff --git a/arch/arm/mach-davinci/devices-tnetv107x.c b/arch/arm/mach-davinci/devices-tnetv107x.c
index 01d8686..de4862e 100644
--- a/arch/arm/mach-davinci/devices-tnetv107x.c
+++ b/arch/arm/mach-davinci/devices-tnetv107x.c
@@ -324,6 +324,9 @@ static int __init nand_init(int chipsel, struct davinci_nand_pdata *data)
 		return ret;
 	}
 
+	if (davinci_aemif_setup(pdev))
+		pr_warn("%s: Cannot configure AEMIF.\n", __func__);
+
 	return platform_device_register(pdev);
 }
 
diff --git a/drivers/mtd/nand/davinci_nand.c b/drivers/mtd/nand/davinci_nand.c
index b77a01e..6cc506c 100644
--- a/drivers/mtd/nand/davinci_nand.c
+++ b/drivers/mtd/nand/davinci_nand.c
@@ -734,28 +734,6 @@ static int __init nand_davinci_probe(struct platform_device *pdev)
 		goto err_clk_enable;
 	}
 
-	/*
-	 * Setup Async configuration register in case we did not boot from
-	 * NAND and so bootloader did not bother to set it up.
-	 */
-	val = davinci_nand_readl(info, A1CR_OFFSET + info->core_chipsel * 4);
-
-	/* Extended Wait is not valid and Select Strobe mode is not used */
-	val &= ~(ACR_ASIZE_MASK | ACR_EW_MASK | ACR_SS_MASK);
-	if (info->chip.options & NAND_BUSWIDTH_16)
-		val |= 0x1;
-
-	davinci_nand_writel(info, A1CR_OFFSET + info->core_chipsel * 4, val);
-
-	ret = 0;
-	if (info->timing)
-		ret = davinci_aemif_setup_timing(info->timing, info->base,
-							info->core_chipsel);
-	if (ret < 0) {
-		dev_dbg(&pdev->dev, "NAND timing values setup fail\n");
-		goto err_timing;
-	}
-
 	spin_lock_irq(&davinci_nand_lock);
 
 	/* put CSxNAND into NAND mode */
@@ -844,7 +822,6 @@ syndrome_done:
 	return 0;
 
 err_scan:
-err_timing:
 	clk_disable_unprepare(info->clk);
 
 err_clk_enable:
diff --git a/include/linux/platform_data/mtd-davinci-aemif.h b/include/linux/platform_data/mtd-davinci-aemif.h
index 05b2934..97948ac 100644
--- a/include/linux/platform_data/mtd-davinci-aemif.h
+++ b/include/linux/platform_data/mtd-davinci-aemif.h
@@ -10,6 +10,8 @@
 #ifndef _MACH_DAVINCI_AEMIF_H
 #define _MACH_DAVINCI_AEMIF_H
 
+#include <linux/platform_device.h>
+
 #define NRCSR_OFFSET		0x00
 #define AWCCR_OFFSET		0x04
 #define A1CR_OFFSET		0x10
@@ -31,6 +33,5 @@ struct davinci_aemif_timing {
 	u8	ta;
 };
 
-int davinci_aemif_setup_timing(struct davinci_aemif_timing *t,
-					void __iomem *base, unsigned cs);
+int davinci_aemif_setup(struct platform_device *pdev);
 #endif
-- 
1.7.9.5

Re: [PATCH] ARM: davinci: aemif: get rid of davinci-nand driver dependency on aemif

[Sekhar Nori]

+ MTD maintainers

On Tuesday 26 November 2013 01:30 AM, Ivan Khoronzhuk wrote:
> The problem that the set timings code contains the call of Davinci
> platform function davinci_aemif_setup_timing() which is not
> accessible if kernel is built for another platform like Keystone.
> 
> The Keysone platform is going to use TI AEMIF driver.
> If TI AEMIF is used we don't need to set timings and bus width.
> It is done by AEMIF driver.
> 
> To get rid of davinci-nand driver dependency on aemif platform code
> we moved aemif code to davinci platform.
> 
> The platform AEMIF code (aemif.c) has to be removed once Davinci
> will be converted to DT and use ti-aemif.c driver.
> 
> Signed-off-by: Ivan Khoronzhuk <ivan.khoronzhuk@ti.com>
> ---
>  arch/arm/mach-davinci/aemif.c                   |   73 ++++++++++++++++++++++-
>  arch/arm/mach-davinci/board-da830-evm.c         |    3 +
>  arch/arm/mach-davinci/board-da850-evm.c         |    3 +
>  arch/arm/mach-davinci/board-dm355-evm.c         |    5 ++
>  arch/arm/mach-davinci/board-dm355-leopard.c     |    5 ++
>  arch/arm/mach-davinci/board-dm365-evm.c         |    4 ++
>  arch/arm/mach-davinci/board-dm644x-evm.c        |    5 ++
>  arch/arm/mach-davinci/board-dm646x-evm.c        |    3 +
>  arch/arm/mach-davinci/board-mityomapl138.c      |    3 +
>  arch/arm/mach-davinci/board-neuros-osd2.c       |    9 ++-
>  arch/arm/mach-davinci/devices-tnetv107x.c       |    3 +
>  drivers/mtd/nand/davinci_nand.c                 |   23 -------
>  include/linux/platform_data/mtd-davinci-aemif.h |    5 +-
>  13 files changed, 116 insertions(+), 28 deletions(-)

[...]

> +
> +/**
> + * davinci_aemif_setup - setup AEMIF interface by davinci_nand_pdata
> + * @pdev - link to platform device to setup settings for
> + *
> + * This function does not use any locking while programming the AEMIF
> + * because it is expected that there is only one user of a given
> + * chip-select.
> + *
> + * Returns 0 on success, else negative errno.
> + */
> +int davinci_aemif_setup(struct platform_device *pdev)
> +{
> +	struct davinci_nand_pdata *pdata = dev_get_platdata(&pdev->dev);
> +	uint32_t val;
> +	struct resource	*res;
> +	void __iomem *base;
> +	int ret = 0;
> +
> +	res = platform_get_resource(pdev, IORESOURCE_MEM, 1);
> +	if (!res) {
> +		dev_err(&pdev->dev, "cannot get IORESOURCE_MEM\n");
> +		return -ENOMEM;
> +	}
> +
> +	base = ioremap(res->start, resource_size(res));
> +	if (!base) {
> +		dev_err(&pdev->dev, "ioremap failed for resource %pR\n", res);
> +		ret = -ENOMEM;
> +		goto err;
> +	}
> +
> +	/*
> +	 * Setup Async configuration register in case we did not boot
> +	 * from NAND and so bootloader did not bother to set it up.
> +	 */
> +	val = davinci_aemif_readl(base, A1CR_OFFSET + pdev->id * 4);

The AEMIF clock has to be enabled before this. The NAND driver might
load much later.

> +	/*
> +	 * Extended Wait is not valid and Select Strobe mode is not
> +	 * used
> +	 */
> +	val &= ~(ACR_ASIZE_MASK | ACR_EW_MASK | ACR_SS_MASK);
> +	if (pdata->options & NAND_BUSWIDTH_16)
> +		val |= 0x1;
> +
> +	davinci_aemif_writel(base, A1CR_OFFSET + pdev->id * 4, val);
> +
> +	if (pdata->timing)
> +		ret = davinci_aemif_setup_timing(pdata->timing, base, pdev->id);
> +
> +	if (ret < 0)
> +		dev_dbg(&pdev->dev, "NAND timing values setup fail\n");
> +
> +err:
> +	iounmap(base);
> +	return ret;
> +}
> +EXPORT_SYMBOL(davinci_aemif_setup);

No need to export this symbol as nothing apart from platform code uses it.

>  static const short mityomap_mii_pins[] = {
> diff --git a/arch/arm/mach-davinci/board-neuros-osd2.c b/arch/arm/mach-davinci/board-neuros-osd2.c
> index bb680af..a7d6668 100644
> --- a/arch/arm/mach-davinci/board-neuros-osd2.c
> +++ b/arch/arm/mach-davinci/board-neuros-osd2.c
> @@ -31,6 +31,7 @@
>  #include <linux/platform_data/mmc-davinci.h>
>  #include <linux/platform_data/mtd-davinci.h>
>  #include <linux/platform_data/usb-davinci.h>
> +#include <linux/platform_data/mtd-davinci-aemif.h>
>  
>  #include <asm/mach-types.h>
>  #include <asm/mach/arch.h>
> @@ -192,9 +193,15 @@ static __init void davinci_ntosd2_init(void)
>  		davinci_cfg_reg(DM644X_ATAEN_DISABLE);
>  
>  		/* only one device will be jumpered and detected */
> -		if (HAS_NAND)
> +		if (HAS_NAND) {
>  			platform_device_register(
>  					&davinci_ntosd2_nandflash_device);
> +
> +			if (davinci_aemif_setup(
> +				&davinci_ntosd2_nandflash_device))
> +				pr_warn("%s: Cannot configure AEMIF.\n",
> +					__func__);

This is looking really ugly. Can you shorten
davinci_ntosd2_nandflash_device to just "ntosd2_nandflash" or similar?

I am yet to test this. Will test once the next version is posted.

Overall, I cannot say I am fan of this approach (mostly because we are
ending up having two drivers for the same hardware in kernel). But given
that the other option of adding platform device support to AEMIF driver
is not acceptable to you, I guess I will live with this.

Thanks,
Sekhar

Re: [PATCH] ARM: davinci: aemif: get rid of davinci-nand driver dependency on aemif

[ivan.khoronzhuk]

On 11/27/2013 10:35 AM, Sekhar Nori wrote:
> + MTD maintainers
>
> On Tuesday 26 November 2013 01:30 AM, Ivan Khoronzhuk wrote:
>> The problem that the set timings code contains the call of Davinci
>> platform function davinci_aemif_setup_timing() which is not
>> accessible if kernel is built for another platform like Keystone.
>>
>> The Keysone platform is going to use TI AEMIF driver.
>> If TI AEMIF is used we don't need to set timings and bus width.
>> It is done by AEMIF driver.
>>
>> To get rid of davinci-nand driver dependency on aemif platform code
>> we moved aemif code to davinci platform.
>>
>> The platform AEMIF code (aemif.c) has to be removed once Davinci
>> will be converted to DT and use ti-aemif.c driver.
>>
>> Signed-off-by: Ivan Khoronzhuk <ivan.khoronzhuk@ti.com>
>> ---
>>   arch/arm/mach-davinci/aemif.c                   |   73 ++++++++++++++++++++++-
>>   arch/arm/mach-davinci/board-da830-evm.c         |    3 +
>>   arch/arm/mach-davinci/board-da850-evm.c         |    3 +
>>   arch/arm/mach-davinci/board-dm355-evm.c         |    5 ++
>>   arch/arm/mach-davinci/board-dm355-leopard.c     |    5 ++
>>   arch/arm/mach-davinci/board-dm365-evm.c         |    4 ++
>>   arch/arm/mach-davinci/board-dm644x-evm.c        |    5 ++
>>   arch/arm/mach-davinci/board-dm646x-evm.c        |    3 +
>>   arch/arm/mach-davinci/board-mityomapl138.c      |    3 +
>>   arch/arm/mach-davinci/board-neuros-osd2.c       |    9 ++-
>>   arch/arm/mach-davinci/devices-tnetv107x.c       |    3 +
>>   drivers/mtd/nand/davinci_nand.c                 |   23 -------
>>   include/linux/platform_data/mtd-davinci-aemif.h |    5 +-
>>   13 files changed, 116 insertions(+), 28 deletions(-)
>
> [...]
>
>> +
>> +/**
>> + * davinci_aemif_setup - setup AEMIF interface by davinci_nand_pdata
>> + * @pdev - link to platform device to setup settings for
>> + *
>> + * This function does not use any locking while programming the AEMIF
>> + * because it is expected that there is only one user of a given
>> + * chip-select.
>> + *
>> + * Returns 0 on success, else negative errno.
>> + */
>> +int davinci_aemif_setup(struct platform_device *pdev)
>> +{
>> +	struct davinci_nand_pdata *pdata = dev_get_platdata(&pdev->dev);
>> +	uint32_t val;
>> +	struct resource	*res;
>> +	void __iomem *base;
>> +	int ret = 0;
>> +
>> +	res = platform_get_resource(pdev, IORESOURCE_MEM, 1);
>> +	if (!res) {
>> +		dev_err(&pdev->dev, "cannot get IORESOURCE_MEM\n");
>> +		return -ENOMEM;
>> +	}
>> +
>> +	base = ioremap(res->start, resource_size(res));
>> +	if (!base) {
>> +		dev_err(&pdev->dev, "ioremap failed for resource %pR\n", res);
>> +		ret = -ENOMEM;
>> +		goto err;
>> +	}
>> +
>> +	/*
>> +	 * Setup Async configuration register in case we did not boot
>> +	 * from NAND and so bootloader did not bother to set it up.
>> +	 */
>> +	val = davinci_aemif_readl(base, A1CR_OFFSET + pdev->id * 4);
>
> The AEMIF clock has to be enabled before this. The NAND driver might
> load much later.
>

Ok

>> +	/*
>> +	 * Extended Wait is not valid and Select Strobe mode is not
>> +	 * used
>> +	 */
>> +	val &= ~(ACR_ASIZE_MASK | ACR_EW_MASK | ACR_SS_MASK);
>> +	if (pdata->options & NAND_BUSWIDTH_16)
>> +		val |= 0x1;
>> +
>> +	davinci_aemif_writel(base, A1CR_OFFSET + pdev->id * 4, val);
>> +
>> +	if (pdata->timing)
>> +		ret = davinci_aemif_setup_timing(pdata->timing, base, pdev->id);
>> +
>> +	if (ret < 0)
>> +		dev_dbg(&pdev->dev, "NAND timing values setup fail\n");
>> +
>> +err:
>> +	iounmap(base);
>> +	return ret;
>> +}
>> +EXPORT_SYMBOL(davinci_aemif_setup);
>
> No need to export this symbol as nothing apart from platform code uses it.
>

Ok

>>   static const short mityomap_mii_pins[] = {
>> diff --git a/arch/arm/mach-davinci/board-neuros-osd2.c b/arch/arm/mach-davinci/board-neuros-osd2.c
>> index bb680af..a7d6668 100644
>> --- a/arch/arm/mach-davinci/board-neuros-osd2.c
>> +++ b/arch/arm/mach-davinci/board-neuros-osd2.c
>> @@ -31,6 +31,7 @@
>>   #include <linux/platform_data/mmc-davinci.h>
>>   #include <linux/platform_data/mtd-davinci.h>
>>   #include <linux/platform_data/usb-davinci.h>
>> +#include <linux/platform_data/mtd-davinci-aemif.h>
>>
>>   #include <asm/mach-types.h>
>>   #include <asm/mach/arch.h>
>> @@ -192,9 +193,15 @@ static __init void davinci_ntosd2_init(void)
>>   		davinci_cfg_reg(DM644X_ATAEN_DISABLE);
>>
>>   		/* only one device will be jumpered and detected */
>> -		if (HAS_NAND)
>> +		if (HAS_NAND) {
>>   			platform_device_register(
>>   					&davinci_ntosd2_nandflash_device);
>> +
>> +			if (davinci_aemif_setup(
>> +				&davinci_ntosd2_nandflash_device))
>> +				pr_warn("%s: Cannot configure AEMIF.\n",
>> +					__func__);
>
> This is looking really ugly. Can you shorten
> davinci_ntosd2_nandflash_device to just "ntosd2_nandflash" or similar?

The rename is not related to the patch, so I won't do this.

>
> I am yet to test this. Will test once the next version is posted.
>
> Overall, I cannot say I am fan of this approach (mostly because we are
> ending up having two drivers for the same hardware in kernel). But given
> that the other option of adding platform device support to AEMIF driver
> is not acceptable to you, I guess I will live with this.
>
> Thanks,
> Sekhar
>

-- 
Regards,
Ivan Khoronzhuk

Re: [PATCH] ARM: davinci: aemif: get rid of davinci-nand driver dependency on aemif

[Sekhar Nori]

On Wednesday 27 November 2013 04:31 PM, ivan.khoronzhuk wrote:
>>> @@ -192,9 +193,15 @@ static __init void davinci_ntosd2_init(void)
>>>           davinci_cfg_reg(DM644X_ATAEN_DISABLE);
>>>
>>>           /* only one device will be jumpered and detected */
>>> -        if (HAS_NAND)
>>> +        if (HAS_NAND) {
>>>               platform_device_register(
>>>                       &davinci_ntosd2_nandflash_device);
>>> +
>>> +            if (davinci_aemif_setup(
>>> +                &davinci_ntosd2_nandflash_device))
>>> +                pr_warn("%s: Cannot configure AEMIF.\n",
>>> +                    __func__);
>>
>> This is looking really ugly. Can you shorten
>> davinci_ntosd2_nandflash_device to just "ntosd2_nandflash" or similar?
> 
> The rename is not related to the patch, so I won't do this.

The reason I asked you to rename is not because I want to push some
random clean-up into this patch. You had to introduce a line so broken
that its almost unreadable. I offered the suggestion as a way to improve
the readability of the code you are introducing in *this* patch.

Thanks,
Sekhar

Re: [PATCH] ARM: davinci: aemif: get rid of davinci-nand driver dependency on aemif

[ivan.khoronzhuk]

On 11/27/2013 03:07 PM, Sekhar Nori wrote:
> On Wednesday 27 November 2013 04:31 PM, ivan.khoronzhuk wrote:
>>>> @@ -192,9 +193,15 @@ static __init void davinci_ntosd2_init(void)
>>>>            davinci_cfg_reg(DM644X_ATAEN_DISABLE);
>>>>
>>>>            /* only one device will be jumpered and detected */
>>>> -        if (HAS_NAND)
>>>> +        if (HAS_NAND) {
>>>>                platform_device_register(
>>>>                        &davinci_ntosd2_nandflash_device);
>>>> +
>>>> +            if (davinci_aemif_setup(
>>>> +                &davinci_ntosd2_nandflash_device))
>>>> +                pr_warn("%s: Cannot configure AEMIF.\n",
>>>> +                    __func__);
>>>
>>> This is looking really ugly. Can you shorten
>>> davinci_ntosd2_nandflash_device to just "ntosd2_nandflash" or similar?
>>
>> The rename is not related to the patch, so I won't do this.
>
> The reason I asked you to rename is not because I want to push some
> random clean-up into this patch. You had to introduce a line so broken
> that its almost unreadable. I offered the suggestion as a way to improve
> the readability of the code you are introducing in *this* patch.
>
> Thanks,
> Sekhar
>

Ok, I will rename

-- 
Regards,
Ivan Khoronzhuk

Re: [PATCH 10/12] mtd: nand: davinci: don't set timings if AEMIF is used

[Santosh Shilimkar]

On Sunday 24 November 2013 04:46 AM, Sekhar Nori wrote:
> On Friday 22 November 2013 11:08 PM, Santosh Shilimkar wrote:
>> On Thursday 21 November 2013 12:07 PM, Nori, Sekhar wrote:
>>> On 11/14/2013 8:06 PM, Santosh Shilimkar wrote:

[..]

>>> Yes, that would be the ideal way. But the reality is that there is close
>>> to zero chance of all the DaVinci platforms using AEMIF ever getting
>>> converted to DT. This means that we will never be able to get rid of
>>> this piece of code.
>>>
>> Well that means some of those platforms support will get deprecated
>> soon considering other basic frameworks like irq, timer wheel etc
>> is assuming the DT direction. But its your call because if you
>> want those platforms to live, you need to convert them to DT.
>>  
>>> I do agree platform data is not the direction you want to take on ARM
>>> but at the same time its not really a deprecated mechanism as far as the
>>> broader kernel is concerned.
>>>
>>> So, can we consider adding platform data mechanism to the AEMIF driver?
>>>
>> Sorry but answer is "NO". If the above hack is really bothering you,
>> we can actually rip out the above code in question and move it
>> to mach-davinci. Infact thats the best direction to make the
>> mtd nandn driver independent of any platform quirks(dt, pdata etc)
>>
>> Ivan already has a patch to do that and he is going to post
>> that patch. With that patch, we won't need $subject patch.
>>
> 
> Okay I will wait for the patch.
> 
Ivan has posted a patch already and you are copied on it.

Regards,
Santosh

[PATCH 11/12] mtd: nand: davinci: don't request AEMIF address range

[Khoronzhuk, Ivan]

The AEMIF driver registers are used to setup timings for each chip
select. The same registers range is used to setup NAND settings.
The AEMIF and NAND drivers not use the same registers in this range.

In case with AEMIF driver, the memory address range is requested
already by AEMIF, so we cannot request it twice, just ioremap.

Signed-off-by: Ivan Khoronzhuk <ivan.khoronzhuk@ti.com>
---
 drivers/mtd/nand/davinci_nand.c |    8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/drivers/mtd/nand/davinci_nand.c b/drivers/mtd/nand/davinci_nand.c
index 879e915..bb6b7e5 100644
--- a/drivers/mtd/nand/davinci_nand.c
+++ b/drivers/mtd/nand/davinci_nand.c
@@ -639,9 +639,11 @@ static int __init nand_davinci_probe(struct platform_device *pdev)
        if (IS_ERR(vaddr))
                return PTR_ERR(vaddr);

-       base = devm_ioremap_resource(&pdev->dev, res2);
-       if (IS_ERR(base))
-               return PTR_ERR(base);
+       base = devm_ioremap(&pdev->dev, res2->start, resource_size(res2));
+       if (!base) {
+               dev_err(&pdev->dev, "ioremap failed for resource %pR\n", res2);
+               return -EADDRNOTAVAIL;
+       }

        info->dev               = &pdev->dev;
        info->base              = base;
--
1.7.9.5

Re: [PATCH 11/12] mtd: nand: davinci: don't request AEMIF address range

[Santosh Shilimkar]

On Monday 11 November 2013 12:12 PM, Khoronzhuk, Ivan wrote:
> The AEMIF driver registers are used to setup timings for each chip
> select. The same registers range is used to setup NAND settings.
> The AEMIF and NAND drivers not use the same registers in this range.
> 
> In case with AEMIF driver, the memory address range is requested
> already by AEMIF, so we cannot request it twice, just ioremap.
> 
> Signed-off-by: Ivan Khoronzhuk <ivan.khoronzhuk@ti.com>
> ---
Acked-by: Santosh Shilimkar <santosh.shilimkar@ti.com>

[PATCH 12/12] arm: dts: keystone: add AEMIF/NAND device entry

[Khoronzhuk, Ivan]

Add AEMIF/NAND device entry.

Signed-off-by: Ivan Khoronzhuk <ivan.khoronzhuk@ti.com>
---
 arch/arm/boot/dts/keystone.dts |   63 ++++++++++++++++++++++++++++++++++++++++
 1 file changed, 63 insertions(+)

diff --git a/arch/arm/boot/dts/keystone.dts b/arch/arm/boot/dts/keystone.dts
index 100bdf5..998da98 100644
--- a/arch/arm/boot/dts/keystone.dts
+++ b/arch/arm/boot/dts/keystone.dts
@@ -179,5 +179,68 @@
                        interrupts = <GIC_SPI 300 IRQ_TYPE_EDGE_RISING>;
                        clocks = <&clkspi>;
                };
+
+               aemif@021000A00 {
+                       compatible = "ti,keystone-aemif";
+                       #address-cells = <2>;
+                       #size-cells = <1>;
+                       clocks = <&clkaemif>;
+                       clock-names = "aemif";
+                       clock-ranges;
+
+                       reg = <0x2100A00 0x00000100>;
+                       ranges = <0 0 0x30000000 0x10000000
+                                 1 0 0x21000A00 0x00000100>;
+
+                       nand:cs0 {
+                               compatible = "ti,davinci-cs";
+                               #address-cells = <2>;
+                               #size-cells = <1>;
+                               clock-ranges;
+                               ranges;
+
+                               /* all timings in nanoseconds */
+                               ti,davinci-cs-ta = <12>;
+                               ti,davinci-cs-rhold = <6>;
+                               ti,davinci-cs-rstrobe = <23>;
+                               ti,davinci-cs-rsetup = <9>;
+                               ti,davinci-cs-whold = <8>;
+                               ti,davinci-cs-wstrobe = <23>;
+                               ti,davinci-cs-wsetup = <8>;
+
+                               nand@0,0 {
+                                       compatible = "ti,keystone-nand";
+                                       #address-cells = <1>;
+                                       #size-cells = <1>;
+                                       reg = <0 0 0x4000000
+                                              1 0 0x0000100>;
+
+                                       ti,davinci-chipselect = <0>;
+                                       ti,davinci-mask-ale = <0x2000>;
+                                       ti,davinci-mask-cle = <0x4000>;
+                                       ti,davinci-mask-chipsel = <0>;
+                                       nand-ecc-mode = "hw";
+                                       ti,davinci-ecc-bits = <4>;
+                                       nand-on-flash-bbt;
+
+                                       partition@0 {
+                                               label = "u-boot";
+                                               reg = <0x0 0x100000>;
+                                               read-only;
+                                       };
+
+                                       partition@100000 {
+                                               label = "params";
+                                               reg = <0x100000 0x80000>;
+                                               read-only;
+                                       };
+
+                                       partition@180000 {
+                                               label = "ubifs";
+                                               reg = <0x180000 0x7E80000>;
+                                       };
+                               };
+                       };
+               };
        };
 };
--
1.7.9.5

Re: [PATCH 12/12] arm: dts: keystone: add AEMIF/NAND device entry

[Santosh Shilimkar]

On Monday 11 November 2013 12:13 PM, Khoronzhuk, Ivan wrote:
> Add AEMIF/NAND device entry.
> 
> Signed-off-by: Ivan Khoronzhuk <ivan.khoronzhuk@ti.com>
> ---
>  arch/arm/boot/dts/keystone.dts |   63 ++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 63 insertions(+)
> 
> diff --git a/arch/arm/boot/dts/keystone.dts b/arch/arm/boot/dts/keystone.dts
> index 100bdf5..998da98 100644
> --- a/arch/arm/boot/dts/keystone.dts
> +++ b/arch/arm/boot/dts/keystone.dts
> @@ -179,5 +179,68 @@
>                         interrupts = <GIC_SPI 300 IRQ_TYPE_EDGE_RISING>;
>                         clocks = <&clkspi>;
>                 };
> +
> +               aemif@021000A00 {
> +                       compatible = "ti,keystone-aemif";
> +                       #address-cells = <2>;
> +                       #size-cells = <1>;
> +                       clocks = <&clkaemif>;
> +                       clock-names = "aemif";
> +                       clock-ranges;
> +
> +                       reg = <0x2100A00 0x00000100>;
> +                       ranges = <0 0 0x30000000 0x10000000
> +                                 1 0 0x21000A00 0x00000100>;
> +
> +                       nand:cs0 {
> +                               compatible = "ti,davinci-cs";
> +                               #address-cells = <2>;
> +                               #size-cells = <1>;
> +                               clock-ranges;
> +                               ranges;
> +
> +                               /* all timings in nanoseconds */
> +                               ti,davinci-cs-ta = <12>;
> +                               ti,davinci-cs-rhold = <6>;
> +                               ti,davinci-cs-rstrobe = <23>;
> +                               ti,davinci-cs-rsetup = <9>;
> +                               ti,davinci-cs-whold = <8>;
> +                               ti,davinci-cs-wstrobe = <23>;
> +                               ti,davinci-cs-wsetup = <8>;
> +
> +                               nand@0,0 {
> +                                       compatible = "ti,keystone-nand";
> +                                       #address-cells = <1>;
> +                                       #size-cells = <1>;
> +                                       reg = <0 0 0x4000000
> +                                              1 0 0x0000100>;
> +
> +                                       ti,davinci-chipselect = <0>;
> +                                       ti,davinci-mask-ale = <0x2000>;
> +                                       ti,davinci-mask-cle = <0x4000>;
> +                                       ti,davinci-mask-chipsel = <0>;
> +                                       nand-ecc-mode = "hw";
> +                                       ti,davinci-ecc-bits = <4>;
> +                                       nand-on-flash-bbt;
> +
> +                                       partition@0 {
> +                                               label = "u-boot";
> +                                               reg = <0x0 0x100000>;
> +                                               read-only;
> +                                       };
> +
> +                                       partition@100000 {
> +                                               label = "params";
> +                                               reg = <0x100000 0x80000>;
> +                                               read-only;
> +                                       };
> +
> +                                       partition@180000 {
> +                                               label = "ubifs";
> +                                               reg = <0x180000 0x7E80000>;
> +                                       };
Lets create now board specific k2hk-evm.dts and rename keystone.dts
to keystone.dtsi and include that in boards. That way you can add the
board specific stuff in board files and common soc stuff in keystone.dtsi.

You can update that once the bindings are blessed by DT folks.

Regards,
Santosh