* [patch v3 1/1] ata: Add iMX pata support
[not found] <20110724183805.992906251@rtp-net.org>
@ 2011-07-24 18:38 ` Arnaud Patard
2011-07-26 7:39 ` Sascha Hauer
0 siblings, 1 reply; 4+ messages in thread
From: Arnaud Patard @ 2011-07-24 18:38 UTC (permalink / raw)
To: linux-ide; +Cc: s.hauer, jgarzik
[-- Attachment #1: efika_pata_driver.patch --]
[-- Type: text/plain, Size: 9042 bytes --]
Add basic support for pata on iMX. It has been tested only on imx51.
SDMA support will probably be added later so this version supports only
PIO.
v2:
- enable only when needed IORDY
- use dev_get_drvdata
v3:
- add missing clk_put() calls
- use platform_get_irq()
- fix resume code to avoid disabling IORDY on resume
Signed-off-by: Arnaud Patard <arnaud.patard@rtp-net.org>
Index: linux-2.6-submit/drivers/ata/Kconfig
===================================================================
--- linux-2.6-submit.orig/drivers/ata/Kconfig 2011-07-22 23:29:06.000000000 +0200
+++ linux-2.6-submit/drivers/ata/Kconfig 2011-07-22 23:30:58.000000000 +0200
@@ -467,6 +467,15 @@ config PATA_ICSIDE
interface card. This is not required for ICS partition support.
If you are unsure, say N to this.
+config PATA_IMX
+ tristate "PATA support for Freescale iMX (Experimental)"
+ depends on ARCH_MX5 && EXPERIMENTAL
+ help
+ This option enables support for the PATA host available on Freescale
+ iMX SoCs.
+
+ If unsure, say N.
+
config PATA_IT8213
tristate "IT8213 PATA support (Experimental)"
depends on PCI && EXPERIMENTAL
Index: linux-2.6-submit/drivers/ata/Makefile
===================================================================
--- linux-2.6-submit.orig/drivers/ata/Makefile 2011-07-22 23:29:06.000000000 +0200
+++ linux-2.6-submit/drivers/ata/Makefile 2011-07-22 23:30:58.000000000 +0200
@@ -48,6 +48,7 @@ obj-$(CONFIG_PATA_HPT37X) += pata_hpt37x
obj-$(CONFIG_PATA_HPT3X2N) += pata_hpt3x2n.o
obj-$(CONFIG_PATA_HPT3X3) += pata_hpt3x3.o
obj-$(CONFIG_PATA_ICSIDE) += pata_icside.o
+obj-$(CONFIG_PATA_IMX) += pata_imx.o
obj-$(CONFIG_PATA_IT8213) += pata_it8213.o
obj-$(CONFIG_PATA_IT821X) += pata_it821x.o
obj-$(CONFIG_PATA_JMICRON) += pata_jmicron.o
Index: linux-2.6-submit/drivers/ata/pata_imx.c
===================================================================
--- /dev/null 1970-01-01 00:00:00.000000000 +0000
+++ linux-2.6-submit/drivers/ata/pata_imx.c 2011-07-24 20:35:24.000000000 +0200
@@ -0,0 +1,263 @@
+/*
+ * Freescale iMX PATA driver
+ *
+ * Copyright (C) 2011 Arnaud Patard <arnaud.patard@rtp-net.org>
+ *
+ * Based on pata_platform - Copyright (C) 2006 - 2007 Paul Mundt
+ *
+ * This file is subject to the terms and conditions of the GNU General Public
+ * License. See the file "COPYING" in the main directory of this archive
+ * for more details.
+ *
+ * TODO:
+ * - dmaengine support
+ * - check if timing stuff needed
+ */
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/init.h>
+#include <linux/blkdev.h>
+#include <scsi/scsi_host.h>
+#include <linux/ata.h>
+#include <linux/libata.h>
+#include <linux/platform_device.h>
+#include <linux/clk.h>
+
+#define DRV_NAME "pata_imx"
+
+#define PATA_IMX_ATA_CONTROL 0x24
+#define PATA_IMX_ATA_CTRL_FIFO_RST_B (1<<7)
+#define PATA_IMX_ATA_CTRL_ATA_RST_B (1<<6)
+#define PATA_IMX_ATA_CTRL_IORDY_EN (1<<0)
+#define PATA_IMX_ATA_INT_EN 0x2C
+#define PATA_IMX_ATA_INTR_ATA_INTRQ2 (1<<3)
+#define PATA_IMX_DRIVE_DATA 0xA0
+#define PATA_IMX_DRIVE_CONTROL 0xD8
+
+struct pata_imx_priv {
+ struct clk *clk;
+ /* timings/interrupt/control regs */
+ u8 *host_regs;
+ u32 ata_ctl;
+};
+
+static int pata_imx_set_mode(struct ata_link *link, struct ata_device **unused)
+{
+ struct ata_device *dev;
+ struct ata_port *ap = link->ap;
+ struct pata_imx_priv *priv = ap->host->private_data;
+ u32 val;
+
+ ata_for_each_dev(dev, link, ENABLED) {
+ dev->pio_mode = dev->xfer_mode = XFER_PIO_0;
+ dev->xfer_shift = ATA_SHIFT_PIO;
+ dev->flags |= ATA_DFLAG_PIO;
+
+ val = __raw_readl(priv->host_regs + PATA_IMX_ATA_CONTROL);
+ if (ata_pio_need_iordy(dev))
+ val |= PATA_IMX_ATA_CTRL_IORDY_EN;
+ else
+ val &= ~PATA_IMX_ATA_CTRL_IORDY_EN;
+ __raw_writel(val, priv->host_regs + PATA_IMX_ATA_CONTROL);
+
+ ata_dev_printk(dev, KERN_INFO, "configured for PIO\n");
+ }
+ return 0;
+}
+
+static struct scsi_host_template pata_imx_sht = {
+ ATA_PIO_SHT(DRV_NAME),
+};
+
+static struct ata_port_operations pata_imx_port_ops = {
+ .inherits = &ata_sff_port_ops,
+ .sff_data_xfer = ata_sff_data_xfer_noirq,
+ .cable_detect = ata_cable_unknown,
+ .set_mode = pata_imx_set_mode,
+};
+
+static void pata_imx_setup_port(struct ata_ioports *ioaddr)
+{
+ /* Fixup the port shift for platforms that need it */
+ ioaddr->data_addr = ioaddr->cmd_addr + (ATA_REG_DATA << 2);
+ ioaddr->error_addr = ioaddr->cmd_addr + (ATA_REG_ERR << 2);
+ ioaddr->feature_addr = ioaddr->cmd_addr + (ATA_REG_FEATURE << 2);
+ ioaddr->nsect_addr = ioaddr->cmd_addr + (ATA_REG_NSECT << 2);
+ ioaddr->lbal_addr = ioaddr->cmd_addr + (ATA_REG_LBAL << 2);
+ ioaddr->lbam_addr = ioaddr->cmd_addr + (ATA_REG_LBAM << 2);
+ ioaddr->lbah_addr = ioaddr->cmd_addr + (ATA_REG_LBAH << 2);
+ ioaddr->device_addr = ioaddr->cmd_addr + (ATA_REG_DEVICE << 2);
+ ioaddr->status_addr = ioaddr->cmd_addr + (ATA_REG_STATUS << 2);
+ ioaddr->command_addr = ioaddr->cmd_addr + (ATA_REG_CMD << 2);
+}
+
+static int __devinit pata_imx_probe(struct platform_device *pdev)
+{
+ struct ata_host *host;
+ struct ata_port *ap;
+ struct pata_imx_priv *priv;
+ int irq = 0;
+ struct resource *io_res;
+
+ io_res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+ if (io_res == NULL)
+ return -EINVAL;
+
+ irq = platform_get_irq(pdev, 0);
+ if (irq <= 0)
+ return -EINVAL;
+
+ priv = kzalloc(sizeof(struct pata_imx_priv), GFP_KERNEL);
+ if (!priv)
+ return -ENOMEM;
+
+ priv->clk = clk_get(&pdev->dev, "imx-pata");
+ if (!priv->clk)
+ dev_warn(&pdev->dev, "Failed to get clock\n");
+ else
+ clk_enable(priv->clk);
+
+
+ host = ata_host_alloc(&pdev->dev, 1);
+ if (!host)
+ goto free_priv;
+
+ host->private_data = priv;
+ ap = host->ports[0];
+
+ ap->ops = &pata_imx_port_ops;
+ ap->pio_mask = ATA_PIO0;
+ ap->flags |= ATA_FLAG_SLAVE_POSS;
+
+ priv->host_regs = devm_ioremap(&pdev->dev, io_res->start, 0x40);
+ ap->ioaddr.cmd_addr = devm_ioremap(&pdev->dev,
+ io_res->start + PATA_IMX_DRIVE_DATA,
+ 0x20);
+ ap->ioaddr.ctl_addr = devm_ioremap(&pdev->dev,
+ io_res->start + PATA_IMX_DRIVE_CONTROL,
+ 0x04);
+ if (!priv->host_regs || !ap->ioaddr.cmd_addr || !ap->ioaddr.ctl_addr) {
+ dev_err(&pdev->dev, "failed to map IO/CTL base\n");
+ goto free_priv;
+ }
+
+ ap->ioaddr.altstatus_addr = ap->ioaddr.ctl_addr;
+
+
+ pata_imx_setup_port(&ap->ioaddr);
+
+ ata_port_desc(ap, "cmd 0x%llx ctl 0x%llx",
+ (unsigned long long)io_res->start + PATA_IMX_DRIVE_DATA,
+ (unsigned long long)io_res->start + PATA_IMX_DRIVE_CONTROL);
+
+ /* deassert resets */
+ __raw_writel(PATA_IMX_ATA_CTRL_FIFO_RST_B |
+ PATA_IMX_ATA_CTRL_ATA_RST_B,
+ priv->host_regs + PATA_IMX_ATA_CONTROL);
+ /* enable interrupts */
+ __raw_writel(PATA_IMX_ATA_INTR_ATA_INTRQ2,
+ priv->host_regs + PATA_IMX_ATA_INT_EN);
+
+ /* activate */
+ return ata_host_activate(host, irq, ata_sff_interrupt, 0,
+ &pata_imx_sht);
+
+free_priv:
+ if (priv->clk) {
+ clk_disable(priv->clk);
+ clk_put(priv->clk);
+ }
+ kfree(priv);
+ return -ENOMEM;
+}
+
+static int __devexit pata_imx_remove(struct platform_device *pdev)
+{
+ struct ata_host *host = dev_get_drvdata(&pdev->dev);
+ struct pata_imx_priv *priv = host->private_data;
+
+ ata_host_detach(host);
+
+ __raw_writel(0, priv->host_regs + PATA_IMX_ATA_INT_EN);
+
+ if (priv->clk) {
+ clk_disable(priv->clk);
+ clk_put(priv->clk);
+ }
+
+ kfree(priv);
+
+ return 0;
+}
+
+#ifdef CONFIG_PM
+static int pata_imx_suspend(struct device *dev)
+{
+ struct ata_host *host = dev_get_drvdata(dev);
+ struct pata_imx_priv *priv = host->private_data;
+ int ret;
+
+ ret = ata_host_suspend(host, PMSG_SUSPEND);
+ if (!ret) {
+ __raw_writel(0, priv->host_regs + PATA_IMX_ATA_INT_EN);
+ priv->ata_ctl =
+ __raw_readl(priv->host_regs + PATA_IMX_ATA_CONTROL);
+ if (priv->clk)
+ clk_disable(priv->clk);
+ }
+
+ return ret;
+}
+
+static int pata_imx_resume(struct device *dev)
+{
+ struct ata_host *host = dev_get_drvdata(dev);
+ struct pata_imx_priv *priv = host->private_data;
+
+ if (priv->clk)
+ clk_enable(priv->clk);
+
+ __raw_writel(priv->ata_ctl, priv->host_regs + PATA_IMX_ATA_CONTROL);
+
+ __raw_writel(PATA_IMX_ATA_INTR_ATA_INTRQ2,
+ priv->host_regs + PATA_IMX_ATA_INT_EN);
+
+ ata_host_resume(host);
+
+ return 0;
+}
+
+static const struct dev_pm_ops pata_imx_pm_ops = {
+ .suspend = pata_imx_suspend,
+ .resume = pata_imx_resume,
+};
+#endif
+
+static struct platform_driver pata_imx_driver = {
+ .probe = pata_imx_probe,
+ .remove = __devexit_p(pata_imx_remove),
+ .driver = {
+ .name = DRV_NAME,
+ .owner = THIS_MODULE,
+#ifdef CONFIG_PM
+ .pm = &pata_imx_pm_ops,
+#endif
+ },
+};
+
+static int __init pata_imx_init(void)
+{
+ return platform_driver_register(&pata_imx_driver);
+}
+
+static void __exit pata_imx_exit(void)
+{
+ platform_driver_unregister(&pata_imx_driver);
+}
+module_init(pata_imx_init);
+module_exit(pata_imx_exit);
+
+MODULE_AUTHOR("Arnaud Patard <arnaud.patard@rtp-net.org>");
+MODULE_DESCRIPTION("low-level driver for iMX PATA");
+MODULE_LICENSE("GPL");
+MODULE_ALIAS("platform:" DRV_NAME);
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [patch v3 1/1] ata: Add iMX pata support
2011-07-24 18:38 ` [patch v3 1/1] ata: Add iMX pata support Arnaud Patard
@ 2011-07-26 7:39 ` Sascha Hauer
2011-07-26 10:04 ` Arnaud Patard
0 siblings, 1 reply; 4+ messages in thread
From: Sascha Hauer @ 2011-07-26 7:39 UTC (permalink / raw)
To: Arnaud Patard; +Cc: linux-ide, jgarzik
Hi Arnaud,
On Sun, Jul 24, 2011 at 08:38:07PM +0200, Arnaud Patard wrote:
> Add basic support for pata on iMX. It has been tested only on imx51.
> SDMA support will probably be added later so this version supports only
> PIO.
>
> v2:
> - enable only when needed IORDY
> - use dev_get_drvdata
> v3:
> - add missing clk_put() calls
> - use platform_get_irq()
> - fix resume code to avoid disabling IORDY on resume
>
> Signed-off-by: Arnaud Patard <arnaud.patard@rtp-net.org>
> Index: linux-2.6-submit/drivers/ata/Kconfig
> ===================================================================
> --- linux-2.6-submit.orig/drivers/ata/Kconfig 2011-07-22 23:29:06.000000000 +0200
> +++ linux-2.6-submit/drivers/ata/Kconfig 2011-07-22 23:30:58.000000000 +0200
> @@ -467,6 +467,15 @@ config PATA_ICSIDE
> interface card. This is not required for ICS partition support.
> If you are unsure, say N to this.
>
> +config PATA_IMX
> + tristate "PATA support for Freescale iMX (Experimental)"
> + depends on ARCH_MX5 && EXPERIMENTAL
Should this really depend on EXPERIMENTAL? This driver looks pretty
straightforward.
Also, please depend on ARCH_MXC instead. This may make this driver
selectable on some i.MX SoCs which do not support it, but we don't
have to add new ARCH_MX* to this list once a new SoC is supported.
(And no, IMX_HAVE_PLATFORM_BLA_BLA is not a good idea either as we are
converting to the device tree and the Kconfig entries will probably go
in the longer run)
> +
> +static int __devinit pata_imx_probe(struct platform_device *pdev)
> +{
> + struct ata_host *host;
> + struct ata_port *ap;
> + struct pata_imx_priv *priv;
> + int irq = 0;
> + struct resource *io_res;
> +
> + io_res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> + if (io_res == NULL)
> + return -EINVAL;
> +
> + irq = platform_get_irq(pdev, 0);
> + if (irq <= 0)
> + return -EINVAL;
> +
> + priv = kzalloc(sizeof(struct pata_imx_priv), GFP_KERNEL);
> + if (!priv)
> + return -ENOMEM;
Given that you already use devm_ioremap, you can also use devm_kzalloc
here.
> +
> + priv->clk = clk_get(&pdev->dev, "imx-pata");
> + if (!priv->clk)
IS_ERR(priv->clk)
(also for the checks below)
Does it really make sense to continue when the clock failed?
> + dev_warn(&pdev->dev, "Failed to get clock\n");
> + else
> + clk_enable(priv->clk);
> +
> +
> + host = ata_host_alloc(&pdev->dev, 1);
> + if (!host)
> + goto free_priv;
> +
> + host->private_data = priv;
> + ap = host->ports[0];
> +
> + ap->ops = &pata_imx_port_ops;
> + ap->pio_mask = ATA_PIO0;
> + ap->flags |= ATA_FLAG_SLAVE_POSS;
> +
> + priv->host_regs = devm_ioremap(&pdev->dev, io_res->start, 0x40);
resource_size() instead of hardcoded 0x40?
> + ap->ioaddr.cmd_addr = devm_ioremap(&pdev->dev,
> + io_res->start + PATA_IMX_DRIVE_DATA,
> + 0x20);
> + ap->ioaddr.ctl_addr = devm_ioremap(&pdev->dev,
> + io_res->start + PATA_IMX_DRIVE_CONTROL,
> + 0x04);
It looks strange to ioremap the register offsets again. Why not just a
ap->ioaddr.cmd_addr = priv->host_regs + PATA_IMX_DRIVE_DATA
ap->ioaddr.ctl_addr = priv->host_regs + PATA_IMX_DRIVE_CONTROL
Hm, probably copied from the pxa driver, but this uses different
resources which makes different ioremaps necessary.
> + if (!priv->host_regs || !ap->ioaddr.cmd_addr || !ap->ioaddr.ctl_addr) {
> + dev_err(&pdev->dev, "failed to map IO/CTL base\n");
> + goto free_priv;
> + }
> +
> + ap->ioaddr.altstatus_addr = ap->ioaddr.ctl_addr;
> +
> +
> + pata_imx_setup_port(&ap->ioaddr);
> +
> + ata_port_desc(ap, "cmd 0x%llx ctl 0x%llx",
> + (unsigned long long)io_res->start + PATA_IMX_DRIVE_DATA,
> + (unsigned long long)io_res->start + PATA_IMX_DRIVE_CONTROL);
> +
> + /* deassert resets */
> + __raw_writel(PATA_IMX_ATA_CTRL_FIFO_RST_B |
> + PATA_IMX_ATA_CTRL_ATA_RST_B,
> + priv->host_regs + PATA_IMX_ATA_CONTROL);
> + /* enable interrupts */
> + __raw_writel(PATA_IMX_ATA_INTR_ATA_INTRQ2,
> + priv->host_regs + PATA_IMX_ATA_INT_EN);
> +
> + /* activate */
> + return ata_host_activate(host, irq, ata_sff_interrupt, 0,
> + &pata_imx_sht);
> +
> +free_priv:
> + if (priv->clk) {
> + clk_disable(priv->clk);
> + clk_put(priv->clk);
> + }
> + kfree(priv);
> + return -ENOMEM;
> +}
> +
--
Pengutronix e.K. | |
Industrial Linux Solutions | http://www.pengutronix.de/ |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0 |
Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 |
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [patch v3 1/1] ata: Add iMX pata support
2011-07-26 7:39 ` Sascha Hauer
@ 2011-07-26 10:04 ` Arnaud Patard
2011-07-26 12:15 ` Sascha Hauer
0 siblings, 1 reply; 4+ messages in thread
From: Arnaud Patard @ 2011-07-26 10:04 UTC (permalink / raw)
To: Sascha Hauer; +Cc: linux-ide, jgarzik
Sascha Hauer <s.hauer@pengutronix.de> writes:
> Hi Arnaud,
Hi,
>
> On Sun, Jul 24, 2011 at 08:38:07PM +0200, Arnaud Patard wrote:
>> Add basic support for pata on iMX. It has been tested only on imx51.
>> SDMA support will probably be added later so this version supports only
>> PIO.
>>
>> v2:
>> - enable only when needed IORDY
>> - use dev_get_drvdata
>> v3:
>> - add missing clk_put() calls
>> - use platform_get_irq()
>> - fix resume code to avoid disabling IORDY on resume
>>
>> Signed-off-by: Arnaud Patard <arnaud.patard@rtp-net.org>
>> Index: linux-2.6-submit/drivers/ata/Kconfig
>> ===================================================================
>> --- linux-2.6-submit.orig/drivers/ata/Kconfig 2011-07-22 23:29:06.000000000 +0200
>> +++ linux-2.6-submit/drivers/ata/Kconfig 2011-07-22 23:30:58.000000000 +0200
>> @@ -467,6 +467,15 @@ config PATA_ICSIDE
>> interface card. This is not required for ICS partition support.
>> If you are unsure, say N to this.
>>
>> +config PATA_IMX
>> + tristate "PATA support for Freescale iMX (Experimental)"
>> + depends on ARCH_MX5 && EXPERIMENTAL
>
> Should this really depend on EXPERIMENTAL? This driver looks pretty
> straightforward.
I've been wondering about it but given that it has been tested only on
efika platforms, I prefered to keep it. If you think it's better to
remove it, fine. I'll do it.
>
> Also, please depend on ARCH_MXC instead. This may make this driver
> selectable on some i.MX SoCs which do not support it, but we don't
> have to add new ARCH_MX* to this list once a new SoC is supported.
Well, it's for the same reason as for the EXPERIMENTAL. It's working on
my mx51 platforms, but I've not tested on plateforms like iMX3 (as I
don't have them). Will change.
>
> (And no, IMX_HAVE_PLATFORM_BLA_BLA is not a good idea either as we are
> converting to the device tree and the Kconfig entries will probably go
> in the longer run)
>
>
>> +
>> +static int __devinit pata_imx_probe(struct platform_device *pdev)
>> +{
>> + struct ata_host *host;
>> + struct ata_port *ap;
>> + struct pata_imx_priv *priv;
>> + int irq = 0;
>> + struct resource *io_res;
>> +
>> + io_res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>> + if (io_res == NULL)
>> + return -EINVAL;
>> +
>> + irq = platform_get_irq(pdev, 0);
>> + if (irq <= 0)
>> + return -EINVAL;
>> +
>> + priv = kzalloc(sizeof(struct pata_imx_priv), GFP_KERNEL);
>> + if (!priv)
>> + return -ENOMEM;
>
> Given that you already use devm_ioremap, you can also use devm_kzalloc
> here.
thanks, will do. I didn't know there was such a function.
>
>> +
>> + priv->clk = clk_get(&pdev->dev, "imx-pata");
>> + if (!priv->clk)
>
> IS_ERR(priv->clk)
>
> (also for the checks below)
>
> Does it really make sense to continue when the clock failed?
if you have uboot version like the one I have, the clock will be already
enabled before booting linux so the driver will work. Maybe too fragile ?
I've changed the checks on my local version and about to test it. Let me
know if you think it should really be a failure, in order to avoid
having to send a new version only to make it a failure.
>
>> + dev_warn(&pdev->dev, "Failed to get clock\n");
>> + else
>> + clk_enable(priv->clk);
>> +
>> +
>> + host = ata_host_alloc(&pdev->dev, 1);
>> + if (!host)
>> + goto free_priv;
>> +
>> + host->private_data = priv;
>> + ap = host->ports[0];
>> +
>> + ap->ops = &pata_imx_port_ops;
>> + ap->pio_mask = ATA_PIO0;
>> + ap->flags |= ATA_FLAG_SLAVE_POSS;
>> +
>> + priv->host_regs = devm_ioremap(&pdev->dev, io_res->start, 0x40);
>
> resource_size() instead of hardcoded 0x40?
>
>> + ap->ioaddr.cmd_addr = devm_ioremap(&pdev->dev,
>> + io_res->start + PATA_IMX_DRIVE_DATA,
>> + 0x20);
>> + ap->ioaddr.ctl_addr = devm_ioremap(&pdev->dev,
>> + io_res->start + PATA_IMX_DRIVE_CONTROL,
>> + 0x04);
>
> It looks strange to ioremap the register offsets again. Why not just a
>
> ap->ioaddr.cmd_addr = priv->host_regs + PATA_IMX_DRIVE_DATA
> ap->ioaddr.ctl_addr = priv->host_regs + PATA_IMX_DRIVE_CONTROL
>
> Hm, probably copied from the pxa driver, but this uses different
> resources which makes different ioremaps necessary.
There are holes in the register map in the manual, that's why I used
several ioremap. I prefer staying near to what the manual says and avoid
remapping unknown registers.
Thanks,
Arnaud
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [patch v3 1/1] ata: Add iMX pata support
2011-07-26 10:04 ` Arnaud Patard
@ 2011-07-26 12:15 ` Sascha Hauer
0 siblings, 0 replies; 4+ messages in thread
From: Sascha Hauer @ 2011-07-26 12:15 UTC (permalink / raw)
To: Arnaud Patard; +Cc: linux-ide, jgarzik
On Tue, Jul 26, 2011 at 12:04:27PM +0200, Arnaud Patard wrote:
> Sascha Hauer <s.hauer@pengutronix.de> writes:
>
> > Hi Arnaud,
>
> Hi,
> >
> > On Sun, Jul 24, 2011 at 08:38:07PM +0200, Arnaud Patard wrote:
> >> Add basic support for pata on iMX. It has been tested only on imx51.
> >> SDMA support will probably be added later so this version supports only
> >> PIO.
> >>
> >> v2:
> >> - enable only when needed IORDY
> >> - use dev_get_drvdata
> >> v3:
> >> - add missing clk_put() calls
> >> - use platform_get_irq()
> >> - fix resume code to avoid disabling IORDY on resume
> >>
> >> Signed-off-by: Arnaud Patard <arnaud.patard@rtp-net.org>
> >> Index: linux-2.6-submit/drivers/ata/Kconfig
> >> ===================================================================
> >> --- linux-2.6-submit.orig/drivers/ata/Kconfig 2011-07-22 23:29:06.000000000 +0200
> >> +++ linux-2.6-submit/drivers/ata/Kconfig 2011-07-22 23:30:58.000000000 +0200
> >> @@ -467,6 +467,15 @@ config PATA_ICSIDE
> >> interface card. This is not required for ICS partition support.
> >> If you are unsure, say N to this.
> >>
> >> +config PATA_IMX
> >> + tristate "PATA support for Freescale iMX (Experimental)"
> >> + depends on ARCH_MX5 && EXPERIMENTAL
> >
> > Should this really depend on EXPERIMENTAL? This driver looks pretty
> > straightforward.
>
> I've been wondering about it but given that it has been tested only on
> efika platforms, I prefered to keep it. If you think it's better to
> remove it, fine. I'll do it.
Only the efika platform registers the device, so there's no risk to
harm other boards.
> >
> > IS_ERR(priv->clk)
> >
> > (also for the checks below)
> >
> > Does it really make sense to continue when the clock failed?
>
> if you have uboot version like the one I have, the clock will be already
> enabled before booting linux so the driver will work. Maybe too fragile ?
> I've changed the checks on my local version and about to test it. Let me
> know if you think it should really be a failure, in order to avoid
> having to send a new version only to make it a failure.
Yes, it should be a failure.
> >> + ap->ioaddr.cmd_addr = devm_ioremap(&pdev->dev,
> >> + io_res->start + PATA_IMX_DRIVE_DATA,
> >> + 0x20);
> >> + ap->ioaddr.ctl_addr = devm_ioremap(&pdev->dev,
> >> + io_res->start + PATA_IMX_DRIVE_CONTROL,
> >> + 0x04);
> >
> > It looks strange to ioremap the register offsets again. Why not just a
> >
> > ap->ioaddr.cmd_addr = priv->host_regs + PATA_IMX_DRIVE_DATA
> > ap->ioaddr.ctl_addr = priv->host_regs + PATA_IMX_DRIVE_CONTROL
> >
> > Hm, probably copied from the pxa driver, but this uses different
> > resources which makes different ioremaps necessary.
>
> There are holes in the register map in the manual, that's why I used
> several ioremap. I prefer staying near to what the manual says and avoid
> remapping unknown registers.
Having holes in the register space is a very common case, no need to do
several ioremaps. Besides, ioremap will give you a whole page anyway.
Sascha
--
Pengutronix e.K. | |
Industrial Linux Solutions | http://www.pengutronix.de/ |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0 |
Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 |
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2011-07-26 12:15 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
[not found] <20110724183805.992906251@rtp-net.org>
2011-07-24 18:38 ` [patch v3 1/1] ata: Add iMX pata support Arnaud Patard
2011-07-26 7:39 ` Sascha Hauer
2011-07-26 10:04 ` Arnaud Patard
2011-07-26 12:15 ` Sascha Hauer
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.