From mboxrd@z Thu Jan 1 00:00:00 1970 Message-ID: <5295D102.2010101@ti.com> Date: Wed, 27 Nov 2013 13:01:22 +0200 From: "ivan.khoronzhuk" MIME-Version: 1.0 To: Sekhar Nori Subject: Re: [PATCH] ARM: davinci: aemif: get rid of davinci-nand driver dependency on aemif References: <5291CB01.1010506@ti.com> <1385409654-19006-1-git-send-email-ivan.khoronzhuk@ti.com> <5295AECB.7040503@ti.com> In-Reply-To: <5295AECB.7040503@ti.com> Content-Type: text/plain; charset="ISO-8859-1"; format=flowed Content-Transfer-Encoding: 7bit Cc: mark.rutland@arm.com, devicetree@vger.kernel.org, grygorii.strashko@ti.com, linux@arm.linux.org.uk, pawel.moll@arm.com, swarren@wwwdotorg.org, ijc+devicetree@hellion.org.uk, galak@kernel.crashing.org, rob.herring@calxeda.com, linux-kernel@vger.kernel.org, linux-mtd@lists.infradead.org, rob@landley.net, Brian Norris , David Woodhouse , linux-arm-kernel@lists.infradead.org List-Id: Linux MTD discussion mailing list List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , 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 >> --- >> 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 >> #include >> #include >> +#include >> >> #include >> #include >> @@ -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 From mboxrd@z Thu Jan 1 00:00:00 1970 From: ivan.khoronzhuk@ti.com (ivan.khoronzhuk) Date: Wed, 27 Nov 2013 13:01:22 +0200 Subject: [PATCH] ARM: davinci: aemif: get rid of davinci-nand driver dependency on aemif In-Reply-To: <5295AECB.7040503@ti.com> References: <5291CB01.1010506@ti.com> <1385409654-19006-1-git-send-email-ivan.khoronzhuk@ti.com> <5295AECB.7040503@ti.com> Message-ID: <5295D102.2010101@ti.com> To: linux-arm-kernel@lists.infradead.org List-Id: linux-arm-kernel.lists.infradead.org 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 >> --- >> 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 >> #include >> #include >> +#include >> >> #include >> #include >> @@ -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 From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1754222Ab3K0LDF (ORCPT ); Wed, 27 Nov 2013 06:03:05 -0500 Received: from arroyo.ext.ti.com ([192.94.94.40]:39236 "EHLO arroyo.ext.ti.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753967Ab3K0LDA (ORCPT ); Wed, 27 Nov 2013 06:03:00 -0500 Message-ID: <5295D102.2010101@ti.com> Date: Wed, 27 Nov 2013 13:01:22 +0200 From: "ivan.khoronzhuk" User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:24.0) Gecko/20100101 Thunderbird/24.1.1 MIME-Version: 1.0 To: Sekhar Nori CC: , , , , , , , , , , , , , Brian Norris , David Woodhouse Subject: Re: [PATCH] ARM: davinci: aemif: get rid of davinci-nand driver dependency on aemif References: <5291CB01.1010506@ti.com> <1385409654-19006-1-git-send-email-ivan.khoronzhuk@ti.com> <5295AECB.7040503@ti.com> In-Reply-To: <5295AECB.7040503@ti.com> Content-Type: text/plain; charset="ISO-8859-1"; format=flowed Content-Transfer-Encoding: 7bit X-Originating-IP: [10.167.145.122] X-EXCLAIMER-MD-CONFIG: f9c360f5-3d1e-4c3c-8703-f45bf52eff6b Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org 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 >> --- >> 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 >> #include >> #include >> +#include >> >> #include >> #include >> @@ -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