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

["ivan.khoronzhuk" <ivan.khoronzhuk@ti.com>]

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