* [PATCH 4/4] pata_samsung: Add Samsung PATA controller driver
@ 2010-05-27 8:22 ` Kukjin Kim
0 siblings, 0 replies; 48+ messages in thread
From: Kukjin Kim @ 2010-05-27 8:22 UTC (permalink / raw)
To: linux-arm-kernel
From: Abhilash Kesavan <a.kesavan@samsung.com>
Adds support for the Samsung PATA controller. This driver is based on the
Libata subsystem and references the earlier patches sent for IDE subsystem.
Signed-off-by: Abhilash Kesavan <a.kesavan@samsung.com>
Signed-off-by: Kukjin Kim <kgene.kim@samsung.com>
---
drivers/ata/Kconfig | 9 +
drivers/ata/Makefile | 1 +
drivers/ata/pata_samsung.c | 591 ++++++++++++++++++++++++++++++++++++++++++++
3 files changed, 601 insertions(+), 0 deletions(-)
create mode 100644 drivers/ata/pata_samsung.c
diff --git a/drivers/ata/Kconfig b/drivers/ata/Kconfig
index e68541f..ce40f66 100644
--- a/drivers/ata/Kconfig
+++ b/drivers/ata/Kconfig
@@ -640,6 +640,15 @@ config PATA_RZ1000
If unsure, say N.
+config PATA_SAMSUNG
+ tristate "Samsung SoC PATA support"
+ depends on S3C_DEV_IDE
+ help
+ This option enables basic support for Samsung's S3C/S5P board
+ PATA controllers via the new ATA layer
+
+ If unsure, say N.
+
config PATA_SC1200
tristate "SC1200 PATA support"
depends on PCI
diff --git a/drivers/ata/Makefile b/drivers/ata/Makefile
index d0a93c4..0b6d29a 100644
--- a/drivers/ata/Makefile
+++ b/drivers/ata/Makefile
@@ -62,6 +62,7 @@ obj-$(CONFIG_PATA_RADISYS) += pata_radisys.o
obj-$(CONFIG_PATA_RB532) += pata_rb532_cf.o
obj-$(CONFIG_PATA_RDC) += pata_rdc.o
obj-$(CONFIG_PATA_RZ1000) += pata_rz1000.o
+obj-$(CONFIG_PATA_SAMSUNG) += pata_samsung.o
obj-$(CONFIG_PATA_SC1200) += pata_sc1200.o
obj-$(CONFIG_PATA_SERVERWORKS) += pata_serverworks.o
obj-$(CONFIG_PATA_SIL680) += pata_sil680.o
diff --git a/drivers/ata/pata_samsung.c b/drivers/ata/pata_samsung.c
new file mode 100644
index 0000000..14381e4
--- /dev/null
+++ b/drivers/ata/pata_samsung.c
@@ -0,0 +1,591 @@
+/* linux/drivers/ata/pata_samsung.c
+ *
+ * Copyright (c) 2010 Samsung Electronics Co., Ltd.
+ * http://www.samsung.com
+ *
+ * PATA driver for Samsung SoC's.
+ * Supports CF Interface in True IDE mode. Currently only PIO mode has been
+ * implemented; UDMA support has to be added.
+ *
+ * Based on:
+ * PATA driver for AT91SAM9260 Static Memory Controller
+ * PATA driver for Toshiba SCC controller
+ *
+ * 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/kernel.h>
+#include <linux/module.h>
+#include <linux/init.h>
+#include <linux/clk.h>
+#include <linux/libata.h>
+#include <linux/platform_device.h>
+#include <linux/slab.h>
+
+#include <plat/ata.h>
+#include <plat/regs-ata.h>
+
+#define DRV_NAME "pata_samsung"
+#define DRV_VERSION "0.1"
+
+enum s3c_cpu_type {
+ TYPE_S3C6400,
+ TYPE_S5PC100,
+ TYPE_S5PV210,
+};
+
+/*
+ * struct s3c_ide_info - S3C PATA instance.
+ * @clk: The clock resource for this controller.
+ * @ide_addr: The area mapped for the hardware registers.
+ * @sfr_addr: The area mapped for the special function registers.
+ * @irq: The IRQ number we are using.
+ * @cpu_type: The exact type of this controller.
+ * @fifo_status_reg: The ATA_FIFO_STATUS register offset.
+ */
+struct s3c_ide_info {
+ struct clk *clk;
+ void __iomem *ide_addr;
+ void __iomem *sfr_addr;
+ unsigned int irq;
+ enum s3c_cpu_type cpu_type;
+ unsigned int fifo_status_reg;
+};
+
+static unsigned long piotime[5];
+
+static void pata_s3c_set_endian(void *s3c_ide_regbase, u8 mode)
+{
+ u32 reg = readl(s3c_ide_regbase + S3C_ATA_CFG);
+ reg = mode ? (reg & ~S3C_ATA_CFG_SWAP) : (reg | S3C_ATA_CFG_SWAP);
+ writel(reg, s3c_ide_regbase + S3C_ATA_CFG);
+}
+
+static void pata_s3c_tune_chipset(struct s3c_ide_info *info, u8 ide_mode)
+{
+ ulong ata_cfg = readl(info->ide_addr + S3C_ATA_CFG);
+
+ /* IORDY is enabled for modes > PIO2 */
+ if (XFER_PIO_0 <= ide_mode && ide_mode <= XFER_PIO_4) {
+
+ switch (ide_mode) {
+ case XFER_PIO_0:
+ case XFER_PIO_1:
+ case XFER_PIO_2:
+ ata_cfg &= (~S3C_ATA_CFG_IORDYEN);
+ break;
+ case XFER_PIO_3:
+ case XFER_PIO_4:
+ ata_cfg |= S3C_ATA_CFG_IORDYEN;
+ break;
+ }
+
+ writel(ata_cfg, info->ide_addr + S3C_ATA_CFG);
+ writel(piotime[ide_mode - XFER_PIO_0],
+ info->ide_addr + S3C_ATA_PIO_TIME);
+ } else {
+ /* Default to PIO0 */
+ ata_cfg &= (~S3C_ATA_CFG_IORDYEN);
+ writel(ata_cfg, info->ide_addr + S3C_ATA_CFG);
+ writel(piotime[0], info->ide_addr + S3C_ATA_PIO_TIME);
+ }
+}
+
+static void
+pata_s3c_set_piomode(struct ata_port *ap, struct ata_device *adev)
+{
+ struct s3c_ide_info *info = ap->host->private_data;
+
+ /* Configure IORDY based on PIO mode and also set the timing value */
+ pata_s3c_tune_chipset(info, adev->pio_mode);
+}
+
+/*
+ * Waits until the IDE controller is able to perform next read/write
+ * operation to the disk. Needed for 64XX series boards only.
+ */
+static int wait_for_host_ready(struct s3c_ide_info *info)
+{
+ ulong timeout;
+
+ /* wait for maximum of 20 msec */
+ timeout = jiffies + msecs_to_jiffies(20);
+ while (time_before(jiffies, timeout)) {
+ if ((readl(info->ide_addr + info->fifo_status_reg) >> 28) == 0)
+ return 0;
+ }
+ return -EBUSY;
+}
+
+/*
+ * Writes to one of the task file registers.
+ */
+static void ata_outb(struct ata_host *host, u8 addr, void __iomem *reg)
+{
+ struct s3c_ide_info *info = host->private_data;
+
+ wait_for_host_ready(info);
+ __raw_writeb(addr, reg);
+}
+
+/*
+ * Reads from one of the task file registers.
+ */
+static u8 ata_inb(struct ata_host *host, void __iomem *reg)
+{
+ struct s3c_ide_info *info = host->private_data;
+ u8 temp;
+
+ wait_for_host_ready(info);
+ temp = __raw_readb(reg);
+ wait_for_host_ready(info);
+ temp = __raw_readb(info->ide_addr + S3C_ATA_PIO_RDATA);
+ return temp;
+}
+
+/*
+ * pata_s3c_tf_load - send taskfile registers to host controller
+ */
+static void pata_s3c_tf_load(struct ata_port *ap,
+ const struct ata_taskfile *tf)
+{
+ struct ata_ioports *ioaddr = &ap->ioaddr;
+ unsigned int is_addr = tf->flags & ATA_TFLAG_ISADDR;
+
+ if (tf->ctl != ap->last_ctl) {
+ if (ioaddr->ctl_addr)
+ ata_outb(ap->host, tf->ctl, ioaddr->ctl_addr);
+ ap->last_ctl = tf->ctl;
+ ata_wait_idle(ap);
+ }
+
+ if (is_addr && (tf->flags & ATA_TFLAG_LBA48)) {
+ WARN_ON_ONCE(!ioaddr->ctl_addr);
+ ata_outb(ap->host, tf->hob_feature, ioaddr->feature_addr);
+ ata_outb(ap->host, tf->hob_nsect, ioaddr->nsect_addr);
+ ata_outb(ap->host, tf->hob_lbal, ioaddr->lbal_addr);
+ ata_outb(ap->host, tf->hob_lbam, ioaddr->lbam_addr);
+ ata_outb(ap->host, tf->hob_lbah, ioaddr->lbah_addr);
+ }
+
+ if (is_addr) {
+ ata_outb(ap->host, tf->feature, ioaddr->feature_addr);
+ ata_outb(ap->host, tf->nsect, ioaddr->nsect_addr);
+ ata_outb(ap->host, tf->lbal, ioaddr->lbal_addr);
+ ata_outb(ap->host, tf->lbam, ioaddr->lbam_addr);
+ ata_outb(ap->host, tf->lbah, ioaddr->lbah_addr);
+ }
+
+ if (tf->flags & ATA_TFLAG_DEVICE)
+ ata_outb(ap->host, tf->device, ioaddr->device_addr);
+
+ ata_wait_idle(ap);
+}
+
+/*
+ * pata_s3c_tf_read - input device's ATA taskfile shadow registers
+ */
+static void pata_s3c_tf_read(struct ata_port *ap, struct ata_taskfile *tf)
+{
+ struct ata_ioports *ioaddr = &ap->ioaddr;
+
+ tf->command = ata_sff_check_status(ap);
+ tf->feature = ata_inb(ap->host, ioaddr->error_addr);
+ tf->nsect = ata_inb(ap->host, ioaddr->nsect_addr);
+ tf->lbal = ata_inb(ap->host, ioaddr->lbal_addr);
+ tf->lbam = ata_inb(ap->host, ioaddr->lbam_addr);
+ tf->lbah = ata_inb(ap->host, ioaddr->lbah_addr);
+ tf->device = ata_inb(ap->host, ioaddr->device_addr);
+
+ if (tf->flags & ATA_TFLAG_LBA48) {
+ if (likely(ioaddr->ctl_addr)) {
+ iowrite8(tf->ctl | ATA_HOB, ioaddr->ctl_addr);
+ tf->hob_feature = ata_inb(ap->host, ioaddr->error_addr);
+ tf->hob_nsect = ata_inb(ap->host, ioaddr->nsect_addr);
+ tf->hob_lbal = ata_inb(ap->host, ioaddr->lbal_addr);
+ tf->hob_lbam = ata_inb(ap->host, ioaddr->lbam_addr);
+ tf->hob_lbah = ata_inb(ap->host, ioaddr->lbah_addr);
+ iowrite8(tf->ctl, ioaddr->ctl_addr);
+ ap->last_ctl = tf->ctl;
+ } else
+ WARN_ON_ONCE(1);
+ }
+}
+
+/*
+ * pata_s3c_exec_command - issue ATA command to host controller
+ */
+static void pata_s3c_exec_command(struct ata_port *ap,
+ const struct ata_taskfile *tf)
+{
+ ata_outb(ap->host, tf->command, ap->ioaddr.command_addr);
+ ata_sff_pause(ap);
+}
+
+/*
+ * pata_s3c_check_status - Read device status register
+ */
+static u8 pata_s3c_check_status(struct ata_port *ap)
+{
+ return ata_inb(ap->host, ap->ioaddr.status_addr);
+}
+
+/*
+ * pata_s3c_data_xfer - Transfer data by PIO
+ */
+unsigned int pata_s3c_data_xfer(struct ata_device *dev, unsigned char *buf,
+ unsigned int buflen, int rw)
+{
+ struct ata_port *ap = dev->link->ap;
+ struct s3c_ide_info *info = ap->host->private_data;
+ void __iomem *data_addr = ap->ioaddr.data_addr;
+ unsigned int words = buflen >> 1, i;
+ u16 *temp_addr = (u16 *)buf;
+
+ if (rw == READ) {
+ for (i = 0; i < words; i++, temp_addr++) {
+ wait_for_host_ready(info);
+ *temp_addr = __raw_readw(data_addr);
+ wait_for_host_ready(info);
+ *temp_addr = __raw_readw(info->ide_addr
+ + S3C_ATA_PIO_RDATA);
+ }
+ } else {
+ for (i = 0; i < words; i++, temp_addr++) {
+ wait_for_host_ready(info);
+ writel(*temp_addr, data_addr);
+ }
+ }
+
+ return words << 1;
+}
+
+static struct scsi_host_template pata_s3c_sht = {
+ ATA_PIO_SHT(DRV_NAME),
+};
+
+static struct ata_port_operations pata_s3c_port_ops = {
+ .inherits = &ata_sff_port_ops,
+ .sff_check_status = pata_s3c_check_status,
+ .sff_tf_load = pata_s3c_tf_load,
+ .sff_tf_read = pata_s3c_tf_read,
+ .sff_data_xfer = pata_s3c_data_xfer,
+ .sff_exec_command = pata_s3c_exec_command,
+ .qc_prep = ata_noop_qc_prep,
+ .set_piomode = pata_s3c_set_piomode,
+};
+
+static struct ata_port_operations pata_s5p_port_ops = {
+ .inherits = &ata_sff_port_ops,
+ .qc_prep = ata_noop_qc_prep,
+ .set_piomode = pata_s3c_set_piomode,
+};
+
+static void pata_s3c_enable(void *s3c_ide_regbase, u8 state)
+{
+ u32 temp = readl(s3c_ide_regbase + S3C_ATA_CTRL);
+ temp = state ? temp | 1 : temp & ~1;
+ writel(temp, s3c_ide_regbase + S3C_ATA_CTRL);
+}
+
+static irqreturn_t pata_s3c_irq(int irq, void *dev_instance)
+{
+ struct ata_host *host = dev_instance;
+ struct s3c_ide_info *info = host->private_data;
+ u32 reg;
+
+ reg = readl(info->ide_addr + S3C_ATA_IRQ);
+ writel(reg, info->ide_addr + S3C_ATA_IRQ);
+
+ return ata_sff_interrupt(irq, dev_instance);
+}
+
+static void pata_s3c_setup_timing(struct s3c_ide_info *info)
+{
+ uint t1, t2, teoc, i;
+
+ uint pio_t1[5] = { 70, 50, 30, 30, 25 };
+ uint pio_t2[5] = { 290, 290, 290, 80, 70 };
+ uint pio_teoc[5] = { 240, 43, 10, 70, 25 };
+
+ ulong cycle_time = (uint) (1000000000 / clk_get_rate(info->clk));
+
+ for (i = 0; i < 5; i++) {
+ t1 = (pio_t1[i] / cycle_time) & 0x0f;
+ t2 = (pio_t2[i] / cycle_time) & 0xff;
+ teoc = (pio_teoc[i] / cycle_time) & 0xff;
+ piotime[i] = (teoc << 12) | (t2 << 4) | t1;
+ }
+}
+
+static void pata_s3c_hwinit(struct s3c_ide_info *info,
+ struct s3c_ide_platdata *pdata)
+{
+ /* Select true-ide as the internal operating mode*/
+ if (pdata && (pdata->cfg_mode))
+ pdata->cfg_mode(info->sfr_addr);
+
+ switch (info->cpu_type) {
+ case TYPE_S3C6400:
+ /* Configure as big endian and enable the i/f*/
+ pata_s3c_set_endian(info->ide_addr, 1);
+ pata_s3c_enable(info->ide_addr, 1);
+ mdelay(100);
+
+ /* Remove IRQ Status */
+ writel(0x1f, info->ide_addr + S3C_ATA_IRQ);
+ writel(0x1b, info->ide_addr + S3C_ATA_IRQ_MSK);
+ break;
+
+ case TYPE_S5PV210:
+ case TYPE_S5PC100:
+ /* Configure as little endian and enable the i/f */
+ pata_s3c_set_endian(info->ide_addr, 0);
+ pata_s3c_enable(info->ide_addr, 1);
+ mdelay(100);
+
+ /* Remove IRQ Status */
+ writel(0x3f, info->ide_addr + S3C_ATA_IRQ);
+ writel(0x3f, info->ide_addr + S3C_ATA_IRQ_MSK);
+ break;
+
+ default:
+ BUG();
+ }
+}
+
+static int __devinit pata_s3c_probe(struct platform_device *pdev)
+{
+ struct s3c_ide_platdata *pdata = pdev->dev.platform_data;
+ struct device *dev = &pdev->dev;
+ struct s3c_ide_info *info;
+ struct resource *res;
+ struct ata_port *ap;
+ struct ata_host *host;
+ enum s3c_cpu_type cpu_type;
+ int ret;
+
+ cpu_type = platform_get_device_id(pdev)->driver_data;
+
+ info = devm_kzalloc(dev, sizeof(*info), GFP_KERNEL);
+ if (!info) {
+ dev_err(dev, "failed to allocate memory for device data\n");
+ return -ENOMEM;
+ }
+
+ info->irq = platform_get_irq(pdev, 0);
+ if (info->irq < 0) {
+ dev_err(dev, "could not obtain irq number\n");
+ ret = -ENODEV;
+ goto release_device_mem;
+ }
+
+ res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+ if (res == NULL) {
+ dev_err(dev, "failed to get mem resource\n");
+ ret = -ENODEV;
+ goto release_device_mem;
+ }
+
+ info->ide_addr = devm_ioremap(dev,
+ res->start, res->end - res->start + 1);
+ if (!info->ide_addr) {
+ dev_err(dev, "failed to map IO base address\n");
+ ret = -ENOMEM;
+ goto release_mem;
+ }
+
+ info->clk = clk_get(&pdev->dev, "cfcon");
+ if (IS_ERR(info->clk)) {
+ dev_err(dev, "failed to get access to cf controller clock\n");
+ ret = PTR_ERR(info->clk);
+ info->clk = NULL;
+ goto unmap;
+ }
+
+ if (clk_enable(info->clk)) {
+ dev_err(dev, "failed to enable clock source.\n");
+ goto clkerr;
+ }
+
+ /* init ata host */
+ host = ata_host_alloc(dev, 1);
+ if (!host) {
+ dev_err(dev, "failed to allocate ide host\n");
+ ret = -ENOMEM;
+ goto stop_clk;
+ }
+
+ ap = host->ports[0];
+ ap->flags |= ATA_FLAG_MMIO;
+ ap->pio_mask = ATA_PIO4;
+
+ if (cpu_type == TYPE_S3C6400) {
+ ap->ops = &pata_s3c_port_ops;
+ info->sfr_addr = info->ide_addr + 0x1800;
+ info->ide_addr = info->ide_addr + 0x1900;
+ info->fifo_status_reg = 0x94;
+ } else if (cpu_type == TYPE_S5PC100) {
+ ap->ops = &pata_s5p_port_ops;
+ info->sfr_addr = info->ide_addr + 0x1800;
+ info->ide_addr = info->ide_addr + 0x1900;
+ info->fifo_status_reg = 0x84;
+ } else {
+ ap->ops = &pata_s5p_port_ops;
+ info->fifo_status_reg = 0x84;
+ }
+
+ info->cpu_type = cpu_type;
+
+ if (!(info->irq)) {
+ ap->flags |= ATA_FLAG_PIO_POLLING;
+ ata_port_desc(ap, "no IRQ, using PIO polling\n");
+ }
+
+ ap->ioaddr.cmd_addr = info->ide_addr + S3C_ATA_CMD;
+ ap->ioaddr.data_addr = info->ide_addr + S3C_ATA_PIO_DTR;
+ ap->ioaddr.error_addr = info->ide_addr + S3C_ATA_PIO_FED;
+ ap->ioaddr.feature_addr = info->ide_addr + S3C_ATA_PIO_FED;
+ ap->ioaddr.nsect_addr = info->ide_addr + S3C_ATA_PIO_SCR;
+ ap->ioaddr.lbal_addr = info->ide_addr + S3C_ATA_PIO_LLR;
+ ap->ioaddr.lbam_addr = info->ide_addr + S3C_ATA_PIO_LMR;
+ ap->ioaddr.lbah_addr = info->ide_addr + S3C_ATA_PIO_LHR;
+ ap->ioaddr.device_addr = info->ide_addr + S3C_ATA_PIO_DVR;
+ ap->ioaddr.status_addr = info->ide_addr + S3C_ATA_PIO_CSD;
+ ap->ioaddr.command_addr = info->ide_addr + S3C_ATA_PIO_CSD;
+ ap->ioaddr.altstatus_addr = info->ide_addr + S3C_ATA_PIO_DAD;
+ ap->ioaddr.ctl_addr = info->ide_addr + S3C_ATA_PIO_DAD;
+
+
+ ata_port_desc(ap, "mmio cmd 0x%llx ",
+ (unsigned long long)res->start);
+
+ host->private_data = info;
+
+ /* Calculates timing parameters for PIO mode */
+ pata_s3c_setup_timing(info);
+
+ if (pdata && (pdata->setup_gpio))
+ pdata->setup_gpio();
+
+ /* Set endianness and enable the interface */
+ pata_s3c_hwinit(info, pdata);
+
+ return ata_host_activate(host, info->irq ? info->irq : 0,
+ info->irq ? pata_s3c_irq : NULL,
+ IRQF_DISABLED, &pata_s3c_sht);
+
+ dev_set_drvdata(&pdev->dev, host);
+
+stop_clk:
+ clk_disable(info->clk);
+clkerr:
+ clk_put(info->clk);
+unmap:
+ devm_iounmap(dev, info->ide_addr);
+release_mem:
+ release_mem_region(res->start, res->end - res->start + 1);
+release_device_mem:
+ kfree(info);
+return ret;
+}
+
+static int __devexit pata_s3c_remove(struct platform_device *pdev)
+{
+ struct ata_host *host = dev_get_drvdata(&pdev->dev);
+ struct s3c_ide_info *info;
+ struct resource *res;
+
+ if (!host)
+ return 0;
+ info = host->private_data;
+
+ ata_host_detach(host);
+
+ devm_iounmap(&pdev->dev, info->ide_addr);
+ clk_disable(info->clk);
+ clk_put(info->clk);
+ res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+ release_mem_region(res->start, res->end - res->start + 1);
+ kfree(info);
+
+ return 0;
+}
+
+#ifdef CONFIG_PM
+static int pata_s3c_suspend(struct platform_device *pdev, pm_message_t state)
+{
+ struct ata_host *host = dev_get_drvdata(&pdev->dev);
+ if (host)
+ return ata_host_suspend(host, state);
+ else
+ return 0;
+}
+
+static int pata_s3c_resume(struct platform_device *pdev)
+{
+ struct ata_host *host = dev_get_drvdata(&pdev->dev);
+ struct s3c_ide_platdata *pdata = pdev->dev.platform_data;
+ struct s3c_ide_info *info;
+
+ info = host->private_data;
+
+ if (host) {
+ pata_s3c_hwinit(info, pdata);
+ ata_host_resume(host);
+ }
+
+ return 0;
+}
+#endif
+
+/* driver device registration */
+static struct platform_device_id pata_s3c_driver_ids[] = {
+ {
+ .name = "s3c6400-pata",
+ .driver_data = TYPE_S3C6400,
+ }, {
+ .name = "s5pc100-pata",
+ .driver_data = TYPE_S5PC100,
+ }, {
+ .name = "s5pv210-pata",
+ .driver_data = TYPE_S5PV210,
+ },
+ { }
+};
+
+MODULE_DEVICE_TABLE(platform, pata_s3c_driver_ids);
+
+static struct platform_driver pata_s3c_driver = {
+ .probe = pata_s3c_probe,
+ .remove = __devexit_p(pata_s3c_remove),
+ .id_table = pata_s3c_driver_ids,
+ .driver = {
+ .name = DRV_NAME,
+ .owner = THIS_MODULE,
+ },
+#ifdef CONFIG_PM
+ .suspend = pata_s3c_suspend,
+ .resume = pata_s3c_resume,
+#endif
+};
+
+static int __init pata_s3c_init(void)
+{
+ return platform_driver_register(&pata_s3c_driver);
+}
+
+static void __exit pata_s3c_exit(void)
+{
+ platform_driver_unregister(&pata_s3c_driver);
+}
+
+module_init(pata_s3c_init);
+module_exit(pata_s3c_exit);
+
+MODULE_AUTHOR("Abhilash Kesavan, <a.kesavan@samsung.com>");
+MODULE_DESCRIPTION("low-level driver for Samsung PATA controller");
+MODULE_LICENSE("GPL");
+MODULE_VERSION(DRV_VERSION);
--
1.6.2.5
^ permalink raw reply related [flat|nested] 48+ messages in thread* Re: [PATCH 4/4] pata_samsung: Add Samsung PATA controller driver
2010-05-27 8:22 ` Kukjin Kim
@ 2010-05-27 8:43 ` Ben Dooks
-1 siblings, 0 replies; 48+ messages in thread
From: Ben Dooks @ 2010-05-27 8:43 UTC (permalink / raw)
To: Kukjin Kim
Cc: linux-ide, linux-arm-kernel, linux-samsung-soc, ben-linux,
Abhilash Kesavan
On Thu, May 27, 2010 at 05:22:04PM +0900, Kukjin Kim wrote:
> From: Abhilash Kesavan <a.kesavan@samsung.com>
>
> Adds support for the Samsung PATA controller. This driver is based on the
> Libata subsystem and references the earlier patches sent for IDE subsystem.
>
> Signed-off-by: Abhilash Kesavan <a.kesavan@samsung.com>
> Signed-off-by: Kukjin Kim <kgene.kim@samsung.com>
> ---
> drivers/ata/Kconfig | 9 +
> drivers/ata/Makefile | 1 +
> drivers/ata/pata_samsung.c | 591 ++++++++++++++++++++++++++++++++++++++++++++
> 3 files changed, 601 insertions(+), 0 deletions(-)
> create mode 100644 drivers/ata/pata_samsung.c
>
> diff --git a/drivers/ata/Kconfig b/drivers/ata/Kconfig
> index e68541f..ce40f66 100644
> --- a/drivers/ata/Kconfig
> +++ b/drivers/ata/Kconfig
> @@ -640,6 +640,15 @@ config PATA_RZ1000
>
> If unsure, say N.
>
> +config PATA_SAMSUNG
> + tristate "Samsung SoC PATA support"
> + depends on S3C_DEV_IDE
> + help
> + This option enables basic support for Samsung's S3C/S5P board
> + PATA controllers via the new ATA layer
> +
> + If unsure, say N.
> +
> config PATA_SC1200
> tristate "SC1200 PATA support"
> depends on PCI
> diff --git a/drivers/ata/Makefile b/drivers/ata/Makefile
> index d0a93c4..0b6d29a 100644
> --- a/drivers/ata/Makefile
> +++ b/drivers/ata/Makefile
> @@ -62,6 +62,7 @@ obj-$(CONFIG_PATA_RADISYS) += pata_radisys.o
> obj-$(CONFIG_PATA_RB532) += pata_rb532_cf.o
> obj-$(CONFIG_PATA_RDC) += pata_rdc.o
> obj-$(CONFIG_PATA_RZ1000) += pata_rz1000.o
> +obj-$(CONFIG_PATA_SAMSUNG) += pata_samsung.o
do you want to call it pata_samsung_soc.o instead, in case samsung make
otehr types of pata controller?
> obj-$(CONFIG_PATA_SC1200) += pata_sc1200.o
> obj-$(CONFIG_PATA_SERVERWORKS) += pata_serverworks.o
> obj-$(CONFIG_PATA_SIL680) += pata_sil680.o
> diff --git a/drivers/ata/pata_samsung.c b/drivers/ata/pata_samsung.c
> new file mode 100644
> index 0000000..14381e4
> --- /dev/null
> +++ b/drivers/ata/pata_samsung.c
> @@ -0,0 +1,591 @@
> +/* linux/drivers/ata/pata_samsung.c
> + *
> + * Copyright (c) 2010 Samsung Electronics Co., Ltd.
> + * http://www.samsung.com
> + *
> + * PATA driver for Samsung SoC's.
SoCs.
> + * Supports CF Interface in True IDE mode. Currently only PIO mode has been
> + * implemented; UDMA support has to be added.
> + *
> + * Based on:
> + * PATA driver for AT91SAM9260 Static Memory Controller
> + * PATA driver for Toshiba SCC controller
> + *
> + * 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/kernel.h>
> +#include <linux/module.h>
> +#include <linux/init.h>
> +#include <linux/clk.h>
> +#include <linux/libata.h>
> +#include <linux/platform_device.h>
> +#include <linux/slab.h>
> +
> +#include <plat/ata.h>
> +#include <plat/regs-ata.h>
> +
> +#define DRV_NAME "pata_samsung"
> +#define DRV_VERSION "0.1"
> +
> +enum s3c_cpu_type {
> + TYPE_S3C6400,
> + TYPE_S5PC100,
> + TYPE_S5PV210,
> +};
> +
> +/*
> + * struct s3c_ide_info - S3C PATA instance.
> + * @clk: The clock resource for this controller.
> + * @ide_addr: The area mapped for the hardware registers.
> + * @sfr_addr: The area mapped for the special function registers.
> + * @irq: The IRQ number we are using.
> + * @cpu_type: The exact type of this controller.
> + * @fifo_status_reg: The ATA_FIFO_STATUS register offset.
> + */
> +struct s3c_ide_info {
> + struct clk *clk;
> + void __iomem *ide_addr;
> + void __iomem *sfr_addr;
> + unsigned int irq;
> + enum s3c_cpu_type cpu_type;
> + unsigned int fifo_status_reg;
> +};
> +
> +static unsigned long piotime[5];
> +
> +static void pata_s3c_set_endian(void *s3c_ide_regbase, u8 mode)
> +{
> + u32 reg = readl(s3c_ide_regbase + S3C_ATA_CFG);
> + reg = mode ? (reg & ~S3C_ATA_CFG_SWAP) : (reg | S3C_ATA_CFG_SWAP);
> + writel(reg, s3c_ide_regbase + S3C_ATA_CFG);
> +}
> +
> +static void pata_s3c_tune_chipset(struct s3c_ide_info *info, u8 ide_mode)
> +{
> + ulong ata_cfg = readl(info->ide_addr + S3C_ATA_CFG);
> +
> + /* IORDY is enabled for modes > PIO2 */
> + if (XFER_PIO_0 <= ide_mode && ide_mode <= XFER_PIO_4) {
> +
> + switch (ide_mode) {
> + case XFER_PIO_0:
> + case XFER_PIO_1:
> + case XFER_PIO_2:
> + ata_cfg &= (~S3C_ATA_CFG_IORDYEN);
> + break;
> + case XFER_PIO_3:
> + case XFER_PIO_4:
> + ata_cfg |= S3C_ATA_CFG_IORDYEN;
> + break;
> + }
> +
> + writel(ata_cfg, info->ide_addr + S3C_ATA_CFG);
> + writel(piotime[ide_mode - XFER_PIO_0],
> + info->ide_addr + S3C_ATA_PIO_TIME);
> + } else {
> + /* Default to PIO0 */
> + ata_cfg &= (~S3C_ATA_CFG_IORDYEN);
technically no need to ().
> + writel(ata_cfg, info->ide_addr + S3C_ATA_CFG);
> + writel(piotime[0], info->ide_addr + S3C_ATA_PIO_TIME);
> + }
> +}
> +
> +static void
> +pata_s3c_set_piomode(struct ata_port *ap, struct ata_device *adev)
> +{
> + struct s3c_ide_info *info = ap->host->private_data;
> +
> + /* Configure IORDY based on PIO mode and also set the timing value */
> + pata_s3c_tune_chipset(info, adev->pio_mode);
> +}
> +
> +/*
> + * Waits until the IDE controller is able to perform next read/write
> + * operation to the disk. Needed for 64XX series boards only.
> + */
> +static int wait_for_host_ready(struct s3c_ide_info *info)
> +{
> + ulong timeout;
> +
> + /* wait for maximum of 20 msec */
> + timeout = jiffies + msecs_to_jiffies(20);
> + while (time_before(jiffies, timeout)) {
> + if ((readl(info->ide_addr + info->fifo_status_reg) >> 28) == 0)
> + return 0;
> + }
> + return -EBUSY;
> +}
> +
> +/*
> + * Writes to one of the task file registers.
> + */
> +static void ata_outb(struct ata_host *host, u8 addr, void __iomem *reg)
> +{
> + struct s3c_ide_info *info = host->private_data;
> +
> + wait_for_host_ready(info);
> + __raw_writeb(addr, reg);
> +}
probably want writeb() here. you've used the non __raw versions elsewhere.
> +/*
> + * Reads from one of the task file registers.
> + */
> +static u8 ata_inb(struct ata_host *host, void __iomem *reg)
> +{
> + struct s3c_ide_info *info = host->private_data;
> + u8 temp;
> +
> + wait_for_host_ready(info);
> + temp = __raw_readb(reg);
> + wait_for_host_ready(info);
> + temp = __raw_readb(info->ide_addr + S3C_ATA_PIO_RDATA);
> + return temp;
> +}
> +
> +/*
> + * pata_s3c_tf_load - send taskfile registers to host controller
> + */
> +static void pata_s3c_tf_load(struct ata_port *ap,
> + const struct ata_taskfile *tf)
> +{
> + struct ata_ioports *ioaddr = &ap->ioaddr;
> + unsigned int is_addr = tf->flags & ATA_TFLAG_ISADDR;
> +
> + if (tf->ctl != ap->last_ctl) {
> + if (ioaddr->ctl_addr)
> + ata_outb(ap->host, tf->ctl, ioaddr->ctl_addr);
> + ap->last_ctl = tf->ctl;
> + ata_wait_idle(ap);
> + }
> +
> + if (is_addr && (tf->flags & ATA_TFLAG_LBA48)) {
> + WARN_ON_ONCE(!ioaddr->ctl_addr);
> + ata_outb(ap->host, tf->hob_feature, ioaddr->feature_addr);
> + ata_outb(ap->host, tf->hob_nsect, ioaddr->nsect_addr);
> + ata_outb(ap->host, tf->hob_lbal, ioaddr->lbal_addr);
> + ata_outb(ap->host, tf->hob_lbam, ioaddr->lbam_addr);
> + ata_outb(ap->host, tf->hob_lbah, ioaddr->lbah_addr);
> + }
> +
> + if (is_addr) {
> + ata_outb(ap->host, tf->feature, ioaddr->feature_addr);
> + ata_outb(ap->host, tf->nsect, ioaddr->nsect_addr);
> + ata_outb(ap->host, tf->lbal, ioaddr->lbal_addr);
> + ata_outb(ap->host, tf->lbam, ioaddr->lbam_addr);
> + ata_outb(ap->host, tf->lbah, ioaddr->lbah_addr);
> + }
> +
> + if (tf->flags & ATA_TFLAG_DEVICE)
> + ata_outb(ap->host, tf->device, ioaddr->device_addr);
> +
> + ata_wait_idle(ap);
> +}
> +
> +/*
> + * pata_s3c_tf_read - input device's ATA taskfile shadow registers
> + */
> +static void pata_s3c_tf_read(struct ata_port *ap, struct ata_taskfile *tf)
> +{
> + struct ata_ioports *ioaddr = &ap->ioaddr;
> +
> + tf->command = ata_sff_check_status(ap);
> + tf->feature = ata_inb(ap->host, ioaddr->error_addr);
> + tf->nsect = ata_inb(ap->host, ioaddr->nsect_addr);
> + tf->lbal = ata_inb(ap->host, ioaddr->lbal_addr);
> + tf->lbam = ata_inb(ap->host, ioaddr->lbam_addr);
> + tf->lbah = ata_inb(ap->host, ioaddr->lbah_addr);
> + tf->device = ata_inb(ap->host, ioaddr->device_addr);
> +
> + if (tf->flags & ATA_TFLAG_LBA48) {
> + if (likely(ioaddr->ctl_addr)) {
> + iowrite8(tf->ctl | ATA_HOB, ioaddr->ctl_addr);
> + tf->hob_feature = ata_inb(ap->host, ioaddr->error_addr);
> + tf->hob_nsect = ata_inb(ap->host, ioaddr->nsect_addr);
> + tf->hob_lbal = ata_inb(ap->host, ioaddr->lbal_addr);
> + tf->hob_lbam = ata_inb(ap->host, ioaddr->lbam_addr);
> + tf->hob_lbah = ata_inb(ap->host, ioaddr->lbah_addr);
> + iowrite8(tf->ctl, ioaddr->ctl_addr);
> + ap->last_ctl = tf->ctl;
> + } else
> + WARN_ON_ONCE(1);
> + }
> +}
> +
> +/*
> + * pata_s3c_exec_command - issue ATA command to host controller
> + */
> +static void pata_s3c_exec_command(struct ata_port *ap,
> + const struct ata_taskfile *tf)
> +{
> + ata_outb(ap->host, tf->command, ap->ioaddr.command_addr);
> + ata_sff_pause(ap);
> +}
> +
> +/*
> + * pata_s3c_check_status - Read device status register
> + */
> +static u8 pata_s3c_check_status(struct ata_port *ap)
> +{
> + return ata_inb(ap->host, ap->ioaddr.status_addr);
> +}
> +
> +/*
> + * pata_s3c_data_xfer - Transfer data by PIO
> + */
> +unsigned int pata_s3c_data_xfer(struct ata_device *dev, unsigned char *buf,
> + unsigned int buflen, int rw)
> +{
> + struct ata_port *ap = dev->link->ap;
> + struct s3c_ide_info *info = ap->host->private_data;
> + void __iomem *data_addr = ap->ioaddr.data_addr;
> + unsigned int words = buflen >> 1, i;
> + u16 *temp_addr = (u16 *)buf;
hmm, how about calling it data_ptr instead of temp_addr?
> + if (rw == READ) {
> + for (i = 0; i < words; i++, temp_addr++) {
> + wait_for_host_ready(info);
> + *temp_addr = __raw_readw(data_addr);
> + wait_for_host_ready(info);
> + *temp_addr = __raw_readw(info->ide_addr
> + + S3C_ATA_PIO_RDATA)
think you want readw() here, please ensure consitency.
this also look a bit weird, you are writing to *temp_addr twice,
could you simply dicscard the first read?
note, you say s3c64xx only needs wait_for_host_ready() but you
always call it?
> + }
> + } else {
> + for (i = 0; i < words; i++, temp_addr++) {
> + wait_for_host_ready(info);
> + writel(*temp_addr, data_addr);
> + }
> + }
> +
> + return words << 1;
> +}
> +
> +static struct scsi_host_template pata_s3c_sht = {
> + ATA_PIO_SHT(DRV_NAME),
> +};
> +
> +static struct ata_port_operations pata_s3c_port_ops = {
> + .inherits = &ata_sff_port_ops,
> + .sff_check_status = pata_s3c_check_status,
> + .sff_tf_load = pata_s3c_tf_load,
> + .sff_tf_read = pata_s3c_tf_read,
> + .sff_data_xfer = pata_s3c_data_xfer,
> + .sff_exec_command = pata_s3c_exec_command,
> + .qc_prep = ata_noop_qc_prep,
> + .set_piomode = pata_s3c_set_piomode,
> +};
> +
> +static struct ata_port_operations pata_s5p_port_ops = {
> + .inherits = &ata_sff_port_ops,
> + .qc_prep = ata_noop_qc_prep,
> + .set_piomode = pata_s3c_set_piomode,
> +};
> +
> +static void pata_s3c_enable(void *s3c_ide_regbase, u8 state)
> +{
> + u32 temp = readl(s3c_ide_regbase + S3C_ATA_CTRL);
> + temp = state ? temp | 1 : temp & ~1;
you might want ()around the temp |1 and temp & ~1
> + writel(temp, s3c_ide_regbase + S3C_ATA_CTRL);
> +}
> +
> +static irqreturn_t pata_s3c_irq(int irq, void *dev_instance)
> +{
> + struct ata_host *host = dev_instance;
> + struct s3c_ide_info *info = host->private_data;
> + u32 reg;
> +
> + reg = readl(info->ide_addr + S3C_ATA_IRQ);
> + writel(reg, info->ide_addr + S3C_ATA_IRQ);
> +
> + return ata_sff_interrupt(irq, dev_instance);
> +}
> +
> +static void pata_s3c_setup_timing(struct s3c_ide_info *info)
> +{
> + uint t1, t2, teoc, i;
> +
> + uint pio_t1[5] = { 70, 50, 30, 30, 25 };
> + uint pio_t2[5] = { 290, 290, 290, 80, 70 };
> + uint pio_teoc[5] = { 240, 43, 10, 70, 25 };
> +
> + ulong cycle_time = (uint) (1000000000 / clk_get_rate(info->clk));
ulong and uinit? surely pick one. you might want to keep UL and cajhnge
1000000000 to 1000000000UL
> + for (i = 0; i < 5; i++) {
> + t1 = (pio_t1[i] / cycle_time) & 0x0f;
> + t2 = (pio_t2[i] / cycle_time) & 0xff;
> + teoc = (pio_teoc[i] / cycle_time) & 0xff;
> + piotime[i] = (teoc << 12) | (t2 << 4) | t1;
> + }
> +}
> +
> +static void pata_s3c_hwinit(struct s3c_ide_info *info,
> + struct s3c_ide_platdata *pdata)
> +{
> + /* Select true-ide as the internal operating mode*/
> + if (pdata && (pdata->cfg_mode))
> + pdata->cfg_mode(info->sfr_addr);
> +
> + switch (info->cpu_type) {
> + case TYPE_S3C6400:
> + /* Configure as big endian and enable the i/f*/
> + pata_s3c_set_endian(info->ide_addr, 1);
> + pata_s3c_enable(info->ide_addr, 1);
> + mdelay(100);
> +
> + /* Remove IRQ Status */
> + writel(0x1f, info->ide_addr + S3C_ATA_IRQ);
> + writel(0x1b, info->ide_addr + S3C_ATA_IRQ_MSK);
> + break;
> +
> + case TYPE_S5PV210:
> + case TYPE_S5PC100:
> + /* Configure as little endian and enable the i/f */
> + pata_s3c_set_endian(info->ide_addr, 0);
> + pata_s3c_enable(info->ide_addr, 1);
> + mdelay(100);
> +
> + /* Remove IRQ Status */
> + writel(0x3f, info->ide_addr + S3C_ATA_IRQ);
> + writel(0x3f, info->ide_addr + S3C_ATA_IRQ_MSK);
> + break;
> +
> + default:
> + BUG();
> + }
> +}
> +
> +static int __devinit pata_s3c_probe(struct platform_device *pdev)
> +{
> + struct s3c_ide_platdata *pdata = pdev->dev.platform_data;
> + struct device *dev = &pdev->dev;
> + struct s3c_ide_info *info;
> + struct resource *res;
> + struct ata_port *ap;
> + struct ata_host *host;
> + enum s3c_cpu_type cpu_type;
> + int ret;
> +
> + cpu_type = platform_get_device_id(pdev)->driver_data;
> +
> + info = devm_kzalloc(dev, sizeof(*info), GFP_KERNEL);
> + if (!info) {
> + dev_err(dev, "failed to allocate memory for device data\n");
> + return -ENOMEM;
> + }
> +
> + info->irq = platform_get_irq(pdev, 0);
> + if (info->irq < 0) {
> + dev_err(dev, "could not obtain irq number\n");
> + ret = -ENODEV;
> + goto release_device_mem;
> + }
> +
> + res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> + if (res == NULL) {
> + dev_err(dev, "failed to get mem resource\n");
> + ret = -ENODEV;
if i remeber correctly, you don't want to be returning -ENODEV as
it will be ignored by the uper layers.
> + goto release_device_mem;
> + }
> +
> + info->ide_addr = devm_ioremap(dev,
> + res->start, res->end - res->start + 1);
resource_size() is your friend here.
> + if (!info->ide_addr) {
> + dev_err(dev, "failed to map IO base address\n");
> + ret = -ENOMEM;
> + goto release_mem;
> + }
> +
> + info->clk = clk_get(&pdev->dev, "cfcon");
need to do something about using explicit names here, should be clk_get(, NULL)
> + if (IS_ERR(info->clk)) {
> + dev_err(dev, "failed to get access to cf controller clock\n");
> + ret = PTR_ERR(info->clk);
> + info->clk = NULL;
> + goto unmap;
> + }
> +
> + if (clk_enable(info->clk)) {
> + dev_err(dev, "failed to enable clock source.\n");
> + goto clkerr;
> + }
> +
> + /* init ata host */
> + host = ata_host_alloc(dev, 1);
> + if (!host) {
> + dev_err(dev, "failed to allocate ide host\n");
> + ret = -ENOMEM;
> + goto stop_clk;
> + }
> +
> + ap = host->ports[0];
> + ap->flags |= ATA_FLAG_MMIO;
> + ap->pio_mask = ATA_PIO4;
> +
> + if (cpu_type == TYPE_S3C6400) {
> + ap->ops = &pata_s3c_port_ops;
> + info->sfr_addr = info->ide_addr + 0x1800;
> + info->ide_addr = info->ide_addr + 0x1900;
> + info->fifo_status_reg = 0x94;
> + } else if (cpu_type == TYPE_S5PC100) {
> + ap->ops = &pata_s5p_port_ops;
> + info->sfr_addr = info->ide_addr + 0x1800;
> + info->ide_addr = info->ide_addr + 0x1900;
> + info->fifo_status_reg = 0x84;
> + } else {
> + ap->ops = &pata_s5p_port_ops;
> + info->fifo_status_reg = 0x84;
> + }
> +
> + info->cpu_type = cpu_type;
> +
> + if (!(info->irq)) {
> + ap->flags |= ATA_FLAG_PIO_POLLING;
> + ata_port_desc(ap, "no IRQ, using PIO polling\n");
> + }
> +
> + ap->ioaddr.cmd_addr = info->ide_addr + S3C_ATA_CMD;
> + ap->ioaddr.data_addr = info->ide_addr + S3C_ATA_PIO_DTR;
> + ap->ioaddr.error_addr = info->ide_addr + S3C_ATA_PIO_FED;
> + ap->ioaddr.feature_addr = info->ide_addr + S3C_ATA_PIO_FED;
> + ap->ioaddr.nsect_addr = info->ide_addr + S3C_ATA_PIO_SCR;
> + ap->ioaddr.lbal_addr = info->ide_addr + S3C_ATA_PIO_LLR;
> + ap->ioaddr.lbam_addr = info->ide_addr + S3C_ATA_PIO_LMR;
> + ap->ioaddr.lbah_addr = info->ide_addr + S3C_ATA_PIO_LHR;
> + ap->ioaddr.device_addr = info->ide_addr + S3C_ATA_PIO_DVR;
> + ap->ioaddr.status_addr = info->ide_addr + S3C_ATA_PIO_CSD;
> + ap->ioaddr.command_addr = info->ide_addr + S3C_ATA_PIO_CSD;
> + ap->ioaddr.altstatus_addr = info->ide_addr + S3C_ATA_PIO_DAD;
> + ap->ioaddr.ctl_addr = info->ide_addr + S3C_ATA_PIO_DAD;
> +
> +
> + ata_port_desc(ap, "mmio cmd 0x%llx ",
> + (unsigned long long)res->start);
> +
> + host->private_data = info;
> +
> + /* Calculates timing parameters for PIO mode */
> + pata_s3c_setup_timing(info);
> +
> + if (pdata && (pdata->setup_gpio))
no need for () around the pdata->setup_gpio
> + pdata->setup_gpio();
> +
> + /* Set endianness and enable the interface */
> + pata_s3c_hwinit(info, pdata);
> +
> + return ata_host_activate(host, info->irq ? info->irq : 0,
> + info->irq ? pata_s3c_irq : NULL,
> + IRQF_DISABLED, &pata_s3c_sht);
do you really need to run with IRQs disabled?
> + dev_set_drvdata(&pdev->dev, host);
> +
> +stop_clk:
> + clk_disable(info->clk);
> +clkerr:
> + clk_put(info->clk);
> +unmap:
> + devm_iounmap(dev, info->ide_addr);
> +release_mem:
> + release_mem_region(res->start, res->end - res->start + 1);
> +release_device_mem:
> + kfree(info);
> +return ret;
> +}
> +
> +static int __devexit pata_s3c_remove(struct platform_device *pdev)
> +{
> + struct ata_host *host = dev_get_drvdata(&pdev->dev);
> + struct s3c_ide_info *info;
> + struct resource *res;
> +
> + if (!host)
> + return 0;
> + info = host->private_data;
> +
> + ata_host_detach(host);
> +
> + devm_iounmap(&pdev->dev, info->ide_addr);
> + clk_disable(info->clk);
> + clk_put(info->clk);
> + res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> + release_mem_region(res->start, res->end - res->start + 1);
> + kfree(info);
> +
> + return 0;
> +}
> +
> +#ifdef CONFIG_PM
> +static int pata_s3c_suspend(struct platform_device *pdev, pm_message_t state)
> +{
> + struct ata_host *host = dev_get_drvdata(&pdev->dev);
> + if (host)
> + return ata_host_suspend(host, state);
> + else
> + return 0;
> +}
> +
> +static int pata_s3c_resume(struct platform_device *pdev)
> +{
> + struct ata_host *host = dev_get_drvdata(&pdev->dev);
> + struct s3c_ide_platdata *pdata = pdev->dev.platform_data;
> + struct s3c_ide_info *info;
> +
> + info = host->private_data;
> +
> + if (host) {
> + pata_s3c_hwinit(info, pdata);
> + ata_host_resume(host);
> + }
> +
> + return 0;
> +}
> +#endif
> +
> +/* driver device registration */
> +static struct platform_device_id pata_s3c_driver_ids[] = {
> + {
> + .name = "s3c6400-pata",
> + .driver_data = TYPE_S3C6400,
> + }, {
> + .name = "s5pc100-pata",
> + .driver_data = TYPE_S5PC100,
> + }, {
> + .name = "s5pv210-pata",
> + .driver_data = TYPE_S5PV210,
> + },
> + { }
> +};
> +
> +MODULE_DEVICE_TABLE(platform, pata_s3c_driver_ids);
> +
> +static struct platform_driver pata_s3c_driver = {
> + .probe = pata_s3c_probe,
> + .remove = __devexit_p(pata_s3c_remove),
> + .id_table = pata_s3c_driver_ids,
> + .driver = {
> + .name = DRV_NAME,
> + .owner = THIS_MODULE,
> + },
> +#ifdef CONFIG_PM
> + .suspend = pata_s3c_suspend,
> + .resume = pata_s3c_resume,
> +#endif
please look at using the new pm_ops.
> +};
> +
> +static int __init pata_s3c_init(void)
> +{
> + return platform_driver_register(&pata_s3c_driver);
> +}
> +
> +static void __exit pata_s3c_exit(void)
> +{
> + platform_driver_unregister(&pata_s3c_driver);
> +}
> +
> +module_init(pata_s3c_init);
> +module_exit(pata_s3c_exit);
> +
> +MODULE_AUTHOR("Abhilash Kesavan, <a.kesavan@samsung.com>");
> +MODULE_DESCRIPTION("low-level driver for Samsung PATA controller");
> +MODULE_LICENSE("GPL");
> +MODULE_VERSION(DRV_VERSION);
> --
> 1.6.2.5
not bad, needs some cleanup.
--
Ben
Q: What's a light-year?
A: One-third less calories than a regular year.
^ permalink raw reply [flat|nested] 48+ messages in thread* [PATCH 4/4] pata_samsung: Add Samsung PATA controller driver
@ 2010-05-27 8:43 ` Ben Dooks
0 siblings, 0 replies; 48+ messages in thread
From: Ben Dooks @ 2010-05-27 8:43 UTC (permalink / raw)
To: linux-arm-kernel
On Thu, May 27, 2010 at 05:22:04PM +0900, Kukjin Kim wrote:
> From: Abhilash Kesavan <a.kesavan@samsung.com>
>
> Adds support for the Samsung PATA controller. This driver is based on the
> Libata subsystem and references the earlier patches sent for IDE subsystem.
>
> Signed-off-by: Abhilash Kesavan <a.kesavan@samsung.com>
> Signed-off-by: Kukjin Kim <kgene.kim@samsung.com>
> ---
> drivers/ata/Kconfig | 9 +
> drivers/ata/Makefile | 1 +
> drivers/ata/pata_samsung.c | 591 ++++++++++++++++++++++++++++++++++++++++++++
> 3 files changed, 601 insertions(+), 0 deletions(-)
> create mode 100644 drivers/ata/pata_samsung.c
>
> diff --git a/drivers/ata/Kconfig b/drivers/ata/Kconfig
> index e68541f..ce40f66 100644
> --- a/drivers/ata/Kconfig
> +++ b/drivers/ata/Kconfig
> @@ -640,6 +640,15 @@ config PATA_RZ1000
>
> If unsure, say N.
>
> +config PATA_SAMSUNG
> + tristate "Samsung SoC PATA support"
> + depends on S3C_DEV_IDE
> + help
> + This option enables basic support for Samsung's S3C/S5P board
> + PATA controllers via the new ATA layer
> +
> + If unsure, say N.
> +
> config PATA_SC1200
> tristate "SC1200 PATA support"
> depends on PCI
> diff --git a/drivers/ata/Makefile b/drivers/ata/Makefile
> index d0a93c4..0b6d29a 100644
> --- a/drivers/ata/Makefile
> +++ b/drivers/ata/Makefile
> @@ -62,6 +62,7 @@ obj-$(CONFIG_PATA_RADISYS) += pata_radisys.o
> obj-$(CONFIG_PATA_RB532) += pata_rb532_cf.o
> obj-$(CONFIG_PATA_RDC) += pata_rdc.o
> obj-$(CONFIG_PATA_RZ1000) += pata_rz1000.o
> +obj-$(CONFIG_PATA_SAMSUNG) += pata_samsung.o
do you want to call it pata_samsung_soc.o instead, in case samsung make
otehr types of pata controller?
> obj-$(CONFIG_PATA_SC1200) += pata_sc1200.o
> obj-$(CONFIG_PATA_SERVERWORKS) += pata_serverworks.o
> obj-$(CONFIG_PATA_SIL680) += pata_sil680.o
> diff --git a/drivers/ata/pata_samsung.c b/drivers/ata/pata_samsung.c
> new file mode 100644
> index 0000000..14381e4
> --- /dev/null
> +++ b/drivers/ata/pata_samsung.c
> @@ -0,0 +1,591 @@
> +/* linux/drivers/ata/pata_samsung.c
> + *
> + * Copyright (c) 2010 Samsung Electronics Co., Ltd.
> + * http://www.samsung.com
> + *
> + * PATA driver for Samsung SoC's.
SoCs.
> + * Supports CF Interface in True IDE mode. Currently only PIO mode has been
> + * implemented; UDMA support has to be added.
> + *
> + * Based on:
> + * PATA driver for AT91SAM9260 Static Memory Controller
> + * PATA driver for Toshiba SCC controller
> + *
> + * 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/kernel.h>
> +#include <linux/module.h>
> +#include <linux/init.h>
> +#include <linux/clk.h>
> +#include <linux/libata.h>
> +#include <linux/platform_device.h>
> +#include <linux/slab.h>
> +
> +#include <plat/ata.h>
> +#include <plat/regs-ata.h>
> +
> +#define DRV_NAME "pata_samsung"
> +#define DRV_VERSION "0.1"
> +
> +enum s3c_cpu_type {
> + TYPE_S3C6400,
> + TYPE_S5PC100,
> + TYPE_S5PV210,
> +};
> +
> +/*
> + * struct s3c_ide_info - S3C PATA instance.
> + * @clk: The clock resource for this controller.
> + * @ide_addr: The area mapped for the hardware registers.
> + * @sfr_addr: The area mapped for the special function registers.
> + * @irq: The IRQ number we are using.
> + * @cpu_type: The exact type of this controller.
> + * @fifo_status_reg: The ATA_FIFO_STATUS register offset.
> + */
> +struct s3c_ide_info {
> + struct clk *clk;
> + void __iomem *ide_addr;
> + void __iomem *sfr_addr;
> + unsigned int irq;
> + enum s3c_cpu_type cpu_type;
> + unsigned int fifo_status_reg;
> +};
> +
> +static unsigned long piotime[5];
> +
> +static void pata_s3c_set_endian(void *s3c_ide_regbase, u8 mode)
> +{
> + u32 reg = readl(s3c_ide_regbase + S3C_ATA_CFG);
> + reg = mode ? (reg & ~S3C_ATA_CFG_SWAP) : (reg | S3C_ATA_CFG_SWAP);
> + writel(reg, s3c_ide_regbase + S3C_ATA_CFG);
> +}
> +
> +static void pata_s3c_tune_chipset(struct s3c_ide_info *info, u8 ide_mode)
> +{
> + ulong ata_cfg = readl(info->ide_addr + S3C_ATA_CFG);
> +
> + /* IORDY is enabled for modes > PIO2 */
> + if (XFER_PIO_0 <= ide_mode && ide_mode <= XFER_PIO_4) {
> +
> + switch (ide_mode) {
> + case XFER_PIO_0:
> + case XFER_PIO_1:
> + case XFER_PIO_2:
> + ata_cfg &= (~S3C_ATA_CFG_IORDYEN);
> + break;
> + case XFER_PIO_3:
> + case XFER_PIO_4:
> + ata_cfg |= S3C_ATA_CFG_IORDYEN;
> + break;
> + }
> +
> + writel(ata_cfg, info->ide_addr + S3C_ATA_CFG);
> + writel(piotime[ide_mode - XFER_PIO_0],
> + info->ide_addr + S3C_ATA_PIO_TIME);
> + } else {
> + /* Default to PIO0 */
> + ata_cfg &= (~S3C_ATA_CFG_IORDYEN);
technically no need to ().
> + writel(ata_cfg, info->ide_addr + S3C_ATA_CFG);
> + writel(piotime[0], info->ide_addr + S3C_ATA_PIO_TIME);
> + }
> +}
> +
> +static void
> +pata_s3c_set_piomode(struct ata_port *ap, struct ata_device *adev)
> +{
> + struct s3c_ide_info *info = ap->host->private_data;
> +
> + /* Configure IORDY based on PIO mode and also set the timing value */
> + pata_s3c_tune_chipset(info, adev->pio_mode);
> +}
> +
> +/*
> + * Waits until the IDE controller is able to perform next read/write
> + * operation to the disk. Needed for 64XX series boards only.
> + */
> +static int wait_for_host_ready(struct s3c_ide_info *info)
> +{
> + ulong timeout;
> +
> + /* wait for maximum of 20 msec */
> + timeout = jiffies + msecs_to_jiffies(20);
> + while (time_before(jiffies, timeout)) {
> + if ((readl(info->ide_addr + info->fifo_status_reg) >> 28) == 0)
> + return 0;
> + }
> + return -EBUSY;
> +}
> +
> +/*
> + * Writes to one of the task file registers.
> + */
> +static void ata_outb(struct ata_host *host, u8 addr, void __iomem *reg)
> +{
> + struct s3c_ide_info *info = host->private_data;
> +
> + wait_for_host_ready(info);
> + __raw_writeb(addr, reg);
> +}
probably want writeb() here. you've used the non __raw versions elsewhere.
> +/*
> + * Reads from one of the task file registers.
> + */
> +static u8 ata_inb(struct ata_host *host, void __iomem *reg)
> +{
> + struct s3c_ide_info *info = host->private_data;
> + u8 temp;
> +
> + wait_for_host_ready(info);
> + temp = __raw_readb(reg);
> + wait_for_host_ready(info);
> + temp = __raw_readb(info->ide_addr + S3C_ATA_PIO_RDATA);
> + return temp;
> +}
> +
> +/*
> + * pata_s3c_tf_load - send taskfile registers to host controller
> + */
> +static void pata_s3c_tf_load(struct ata_port *ap,
> + const struct ata_taskfile *tf)
> +{
> + struct ata_ioports *ioaddr = &ap->ioaddr;
> + unsigned int is_addr = tf->flags & ATA_TFLAG_ISADDR;
> +
> + if (tf->ctl != ap->last_ctl) {
> + if (ioaddr->ctl_addr)
> + ata_outb(ap->host, tf->ctl, ioaddr->ctl_addr);
> + ap->last_ctl = tf->ctl;
> + ata_wait_idle(ap);
> + }
> +
> + if (is_addr && (tf->flags & ATA_TFLAG_LBA48)) {
> + WARN_ON_ONCE(!ioaddr->ctl_addr);
> + ata_outb(ap->host, tf->hob_feature, ioaddr->feature_addr);
> + ata_outb(ap->host, tf->hob_nsect, ioaddr->nsect_addr);
> + ata_outb(ap->host, tf->hob_lbal, ioaddr->lbal_addr);
> + ata_outb(ap->host, tf->hob_lbam, ioaddr->lbam_addr);
> + ata_outb(ap->host, tf->hob_lbah, ioaddr->lbah_addr);
> + }
> +
> + if (is_addr) {
> + ata_outb(ap->host, tf->feature, ioaddr->feature_addr);
> + ata_outb(ap->host, tf->nsect, ioaddr->nsect_addr);
> + ata_outb(ap->host, tf->lbal, ioaddr->lbal_addr);
> + ata_outb(ap->host, tf->lbam, ioaddr->lbam_addr);
> + ata_outb(ap->host, tf->lbah, ioaddr->lbah_addr);
> + }
> +
> + if (tf->flags & ATA_TFLAG_DEVICE)
> + ata_outb(ap->host, tf->device, ioaddr->device_addr);
> +
> + ata_wait_idle(ap);
> +}
> +
> +/*
> + * pata_s3c_tf_read - input device's ATA taskfile shadow registers
> + */
> +static void pata_s3c_tf_read(struct ata_port *ap, struct ata_taskfile *tf)
> +{
> + struct ata_ioports *ioaddr = &ap->ioaddr;
> +
> + tf->command = ata_sff_check_status(ap);
> + tf->feature = ata_inb(ap->host, ioaddr->error_addr);
> + tf->nsect = ata_inb(ap->host, ioaddr->nsect_addr);
> + tf->lbal = ata_inb(ap->host, ioaddr->lbal_addr);
> + tf->lbam = ata_inb(ap->host, ioaddr->lbam_addr);
> + tf->lbah = ata_inb(ap->host, ioaddr->lbah_addr);
> + tf->device = ata_inb(ap->host, ioaddr->device_addr);
> +
> + if (tf->flags & ATA_TFLAG_LBA48) {
> + if (likely(ioaddr->ctl_addr)) {
> + iowrite8(tf->ctl | ATA_HOB, ioaddr->ctl_addr);
> + tf->hob_feature = ata_inb(ap->host, ioaddr->error_addr);
> + tf->hob_nsect = ata_inb(ap->host, ioaddr->nsect_addr);
> + tf->hob_lbal = ata_inb(ap->host, ioaddr->lbal_addr);
> + tf->hob_lbam = ata_inb(ap->host, ioaddr->lbam_addr);
> + tf->hob_lbah = ata_inb(ap->host, ioaddr->lbah_addr);
> + iowrite8(tf->ctl, ioaddr->ctl_addr);
> + ap->last_ctl = tf->ctl;
> + } else
> + WARN_ON_ONCE(1);
> + }
> +}
> +
> +/*
> + * pata_s3c_exec_command - issue ATA command to host controller
> + */
> +static void pata_s3c_exec_command(struct ata_port *ap,
> + const struct ata_taskfile *tf)
> +{
> + ata_outb(ap->host, tf->command, ap->ioaddr.command_addr);
> + ata_sff_pause(ap);
> +}
> +
> +/*
> + * pata_s3c_check_status - Read device status register
> + */
> +static u8 pata_s3c_check_status(struct ata_port *ap)
> +{
> + return ata_inb(ap->host, ap->ioaddr.status_addr);
> +}
> +
> +/*
> + * pata_s3c_data_xfer - Transfer data by PIO
> + */
> +unsigned int pata_s3c_data_xfer(struct ata_device *dev, unsigned char *buf,
> + unsigned int buflen, int rw)
> +{
> + struct ata_port *ap = dev->link->ap;
> + struct s3c_ide_info *info = ap->host->private_data;
> + void __iomem *data_addr = ap->ioaddr.data_addr;
> + unsigned int words = buflen >> 1, i;
> + u16 *temp_addr = (u16 *)buf;
hmm, how about calling it data_ptr instead of temp_addr?
> + if (rw == READ) {
> + for (i = 0; i < words; i++, temp_addr++) {
> + wait_for_host_ready(info);
> + *temp_addr = __raw_readw(data_addr);
> + wait_for_host_ready(info);
> + *temp_addr = __raw_readw(info->ide_addr
> + + S3C_ATA_PIO_RDATA)
think you want readw() here, please ensure consitency.
this also look a bit weird, you are writing to *temp_addr twice,
could you simply dicscard the first read?
note, you say s3c64xx only needs wait_for_host_ready() but you
always call it?
> + }
> + } else {
> + for (i = 0; i < words; i++, temp_addr++) {
> + wait_for_host_ready(info);
> + writel(*temp_addr, data_addr);
> + }
> + }
> +
> + return words << 1;
> +}
> +
> +static struct scsi_host_template pata_s3c_sht = {
> + ATA_PIO_SHT(DRV_NAME),
> +};
> +
> +static struct ata_port_operations pata_s3c_port_ops = {
> + .inherits = &ata_sff_port_ops,
> + .sff_check_status = pata_s3c_check_status,
> + .sff_tf_load = pata_s3c_tf_load,
> + .sff_tf_read = pata_s3c_tf_read,
> + .sff_data_xfer = pata_s3c_data_xfer,
> + .sff_exec_command = pata_s3c_exec_command,
> + .qc_prep = ata_noop_qc_prep,
> + .set_piomode = pata_s3c_set_piomode,
> +};
> +
> +static struct ata_port_operations pata_s5p_port_ops = {
> + .inherits = &ata_sff_port_ops,
> + .qc_prep = ata_noop_qc_prep,
> + .set_piomode = pata_s3c_set_piomode,
> +};
> +
> +static void pata_s3c_enable(void *s3c_ide_regbase, u8 state)
> +{
> + u32 temp = readl(s3c_ide_regbase + S3C_ATA_CTRL);
> + temp = state ? temp | 1 : temp & ~1;
you might want ()around the temp |1 and temp & ~1
> + writel(temp, s3c_ide_regbase + S3C_ATA_CTRL);
> +}
> +
> +static irqreturn_t pata_s3c_irq(int irq, void *dev_instance)
> +{
> + struct ata_host *host = dev_instance;
> + struct s3c_ide_info *info = host->private_data;
> + u32 reg;
> +
> + reg = readl(info->ide_addr + S3C_ATA_IRQ);
> + writel(reg, info->ide_addr + S3C_ATA_IRQ);
> +
> + return ata_sff_interrupt(irq, dev_instance);
> +}
> +
> +static void pata_s3c_setup_timing(struct s3c_ide_info *info)
> +{
> + uint t1, t2, teoc, i;
> +
> + uint pio_t1[5] = { 70, 50, 30, 30, 25 };
> + uint pio_t2[5] = { 290, 290, 290, 80, 70 };
> + uint pio_teoc[5] = { 240, 43, 10, 70, 25 };
> +
> + ulong cycle_time = (uint) (1000000000 / clk_get_rate(info->clk));
ulong and uinit? surely pick one. you might want to keep UL and cajhnge
1000000000 to 1000000000UL
> + for (i = 0; i < 5; i++) {
> + t1 = (pio_t1[i] / cycle_time) & 0x0f;
> + t2 = (pio_t2[i] / cycle_time) & 0xff;
> + teoc = (pio_teoc[i] / cycle_time) & 0xff;
> + piotime[i] = (teoc << 12) | (t2 << 4) | t1;
> + }
> +}
> +
> +static void pata_s3c_hwinit(struct s3c_ide_info *info,
> + struct s3c_ide_platdata *pdata)
> +{
> + /* Select true-ide as the internal operating mode*/
> + if (pdata && (pdata->cfg_mode))
> + pdata->cfg_mode(info->sfr_addr);
> +
> + switch (info->cpu_type) {
> + case TYPE_S3C6400:
> + /* Configure as big endian and enable the i/f*/
> + pata_s3c_set_endian(info->ide_addr, 1);
> + pata_s3c_enable(info->ide_addr, 1);
> + mdelay(100);
> +
> + /* Remove IRQ Status */
> + writel(0x1f, info->ide_addr + S3C_ATA_IRQ);
> + writel(0x1b, info->ide_addr + S3C_ATA_IRQ_MSK);
> + break;
> +
> + case TYPE_S5PV210:
> + case TYPE_S5PC100:
> + /* Configure as little endian and enable the i/f */
> + pata_s3c_set_endian(info->ide_addr, 0);
> + pata_s3c_enable(info->ide_addr, 1);
> + mdelay(100);
> +
> + /* Remove IRQ Status */
> + writel(0x3f, info->ide_addr + S3C_ATA_IRQ);
> + writel(0x3f, info->ide_addr + S3C_ATA_IRQ_MSK);
> + break;
> +
> + default:
> + BUG();
> + }
> +}
> +
> +static int __devinit pata_s3c_probe(struct platform_device *pdev)
> +{
> + struct s3c_ide_platdata *pdata = pdev->dev.platform_data;
> + struct device *dev = &pdev->dev;
> + struct s3c_ide_info *info;
> + struct resource *res;
> + struct ata_port *ap;
> + struct ata_host *host;
> + enum s3c_cpu_type cpu_type;
> + int ret;
> +
> + cpu_type = platform_get_device_id(pdev)->driver_data;
> +
> + info = devm_kzalloc(dev, sizeof(*info), GFP_KERNEL);
> + if (!info) {
> + dev_err(dev, "failed to allocate memory for device data\n");
> + return -ENOMEM;
> + }
> +
> + info->irq = platform_get_irq(pdev, 0);
> + if (info->irq < 0) {
> + dev_err(dev, "could not obtain irq number\n");
> + ret = -ENODEV;
> + goto release_device_mem;
> + }
> +
> + res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> + if (res == NULL) {
> + dev_err(dev, "failed to get mem resource\n");
> + ret = -ENODEV;
if i remeber correctly, you don't want to be returning -ENODEV as
it will be ignored by the uper layers.
> + goto release_device_mem;
> + }
> +
> + info->ide_addr = devm_ioremap(dev,
> + res->start, res->end - res->start + 1);
resource_size() is your friend here.
> + if (!info->ide_addr) {
> + dev_err(dev, "failed to map IO base address\n");
> + ret = -ENOMEM;
> + goto release_mem;
> + }
> +
> + info->clk = clk_get(&pdev->dev, "cfcon");
need to do something about using explicit names here, should be clk_get(, NULL)
> + if (IS_ERR(info->clk)) {
> + dev_err(dev, "failed to get access to cf controller clock\n");
> + ret = PTR_ERR(info->clk);
> + info->clk = NULL;
> + goto unmap;
> + }
> +
> + if (clk_enable(info->clk)) {
> + dev_err(dev, "failed to enable clock source.\n");
> + goto clkerr;
> + }
> +
> + /* init ata host */
> + host = ata_host_alloc(dev, 1);
> + if (!host) {
> + dev_err(dev, "failed to allocate ide host\n");
> + ret = -ENOMEM;
> + goto stop_clk;
> + }
> +
> + ap = host->ports[0];
> + ap->flags |= ATA_FLAG_MMIO;
> + ap->pio_mask = ATA_PIO4;
> +
> + if (cpu_type == TYPE_S3C6400) {
> + ap->ops = &pata_s3c_port_ops;
> + info->sfr_addr = info->ide_addr + 0x1800;
> + info->ide_addr = info->ide_addr + 0x1900;
> + info->fifo_status_reg = 0x94;
> + } else if (cpu_type == TYPE_S5PC100) {
> + ap->ops = &pata_s5p_port_ops;
> + info->sfr_addr = info->ide_addr + 0x1800;
> + info->ide_addr = info->ide_addr + 0x1900;
> + info->fifo_status_reg = 0x84;
> + } else {
> + ap->ops = &pata_s5p_port_ops;
> + info->fifo_status_reg = 0x84;
> + }
> +
> + info->cpu_type = cpu_type;
> +
> + if (!(info->irq)) {
> + ap->flags |= ATA_FLAG_PIO_POLLING;
> + ata_port_desc(ap, "no IRQ, using PIO polling\n");
> + }
> +
> + ap->ioaddr.cmd_addr = info->ide_addr + S3C_ATA_CMD;
> + ap->ioaddr.data_addr = info->ide_addr + S3C_ATA_PIO_DTR;
> + ap->ioaddr.error_addr = info->ide_addr + S3C_ATA_PIO_FED;
> + ap->ioaddr.feature_addr = info->ide_addr + S3C_ATA_PIO_FED;
> + ap->ioaddr.nsect_addr = info->ide_addr + S3C_ATA_PIO_SCR;
> + ap->ioaddr.lbal_addr = info->ide_addr + S3C_ATA_PIO_LLR;
> + ap->ioaddr.lbam_addr = info->ide_addr + S3C_ATA_PIO_LMR;
> + ap->ioaddr.lbah_addr = info->ide_addr + S3C_ATA_PIO_LHR;
> + ap->ioaddr.device_addr = info->ide_addr + S3C_ATA_PIO_DVR;
> + ap->ioaddr.status_addr = info->ide_addr + S3C_ATA_PIO_CSD;
> + ap->ioaddr.command_addr = info->ide_addr + S3C_ATA_PIO_CSD;
> + ap->ioaddr.altstatus_addr = info->ide_addr + S3C_ATA_PIO_DAD;
> + ap->ioaddr.ctl_addr = info->ide_addr + S3C_ATA_PIO_DAD;
> +
> +
> + ata_port_desc(ap, "mmio cmd 0x%llx ",
> + (unsigned long long)res->start);
> +
> + host->private_data = info;
> +
> + /* Calculates timing parameters for PIO mode */
> + pata_s3c_setup_timing(info);
> +
> + if (pdata && (pdata->setup_gpio))
no need for () around the pdata->setup_gpio
> + pdata->setup_gpio();
> +
> + /* Set endianness and enable the interface */
> + pata_s3c_hwinit(info, pdata);
> +
> + return ata_host_activate(host, info->irq ? info->irq : 0,
> + info->irq ? pata_s3c_irq : NULL,
> + IRQF_DISABLED, &pata_s3c_sht);
do you really need to run with IRQs disabled?
> + dev_set_drvdata(&pdev->dev, host);
> +
> +stop_clk:
> + clk_disable(info->clk);
> +clkerr:
> + clk_put(info->clk);
> +unmap:
> + devm_iounmap(dev, info->ide_addr);
> +release_mem:
> + release_mem_region(res->start, res->end - res->start + 1);
> +release_device_mem:
> + kfree(info);
> +return ret;
> +}
> +
> +static int __devexit pata_s3c_remove(struct platform_device *pdev)
> +{
> + struct ata_host *host = dev_get_drvdata(&pdev->dev);
> + struct s3c_ide_info *info;
> + struct resource *res;
> +
> + if (!host)
> + return 0;
> + info = host->private_data;
> +
> + ata_host_detach(host);
> +
> + devm_iounmap(&pdev->dev, info->ide_addr);
> + clk_disable(info->clk);
> + clk_put(info->clk);
> + res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> + release_mem_region(res->start, res->end - res->start + 1);
> + kfree(info);
> +
> + return 0;
> +}
> +
> +#ifdef CONFIG_PM
> +static int pata_s3c_suspend(struct platform_device *pdev, pm_message_t state)
> +{
> + struct ata_host *host = dev_get_drvdata(&pdev->dev);
> + if (host)
> + return ata_host_suspend(host, state);
> + else
> + return 0;
> +}
> +
> +static int pata_s3c_resume(struct platform_device *pdev)
> +{
> + struct ata_host *host = dev_get_drvdata(&pdev->dev);
> + struct s3c_ide_platdata *pdata = pdev->dev.platform_data;
> + struct s3c_ide_info *info;
> +
> + info = host->private_data;
> +
> + if (host) {
> + pata_s3c_hwinit(info, pdata);
> + ata_host_resume(host);
> + }
> +
> + return 0;
> +}
> +#endif
> +
> +/* driver device registration */
> +static struct platform_device_id pata_s3c_driver_ids[] = {
> + {
> + .name = "s3c6400-pata",
> + .driver_data = TYPE_S3C6400,
> + }, {
> + .name = "s5pc100-pata",
> + .driver_data = TYPE_S5PC100,
> + }, {
> + .name = "s5pv210-pata",
> + .driver_data = TYPE_S5PV210,
> + },
> + { }
> +};
> +
> +MODULE_DEVICE_TABLE(platform, pata_s3c_driver_ids);
> +
> +static struct platform_driver pata_s3c_driver = {
> + .probe = pata_s3c_probe,
> + .remove = __devexit_p(pata_s3c_remove),
> + .id_table = pata_s3c_driver_ids,
> + .driver = {
> + .name = DRV_NAME,
> + .owner = THIS_MODULE,
> + },
> +#ifdef CONFIG_PM
> + .suspend = pata_s3c_suspend,
> + .resume = pata_s3c_resume,
> +#endif
please look at using the new pm_ops.
> +};
> +
> +static int __init pata_s3c_init(void)
> +{
> + return platform_driver_register(&pata_s3c_driver);
> +}
> +
> +static void __exit pata_s3c_exit(void)
> +{
> + platform_driver_unregister(&pata_s3c_driver);
> +}
> +
> +module_init(pata_s3c_init);
> +module_exit(pata_s3c_exit);
> +
> +MODULE_AUTHOR("Abhilash Kesavan, <a.kesavan@samsung.com>");
> +MODULE_DESCRIPTION("low-level driver for Samsung PATA controller");
> +MODULE_LICENSE("GPL");
> +MODULE_VERSION(DRV_VERSION);
> --
> 1.6.2.5
not bad, needs some cleanup.
--
Ben
Q: What's a light-year?
A: One-third less calories than a regular year.
^ permalink raw reply [flat|nested] 48+ messages in thread* RE: [PATCH 4/4] pata_samsung: Add Samsung PATA controller driver
2010-05-27 8:43 ` Ben Dooks
@ 2010-06-02 2:37 ` Kukjin Kim
-1 siblings, 0 replies; 48+ messages in thread
From: Kukjin Kim @ 2010-06-02 2:37 UTC (permalink / raw)
To: 'Ben Dooks'
Cc: linux-ide, linux-arm-kernel, linux-samsung-soc,
'Abhilash Kesavan'
Ben Dooks wrote:
>
> On Thu, May 27, 2010 at 05:22:04PM +0900, Kukjin Kim wrote:
> > From: Abhilash Kesavan <a.kesavan@samsung.com>
> >
> > Adds support for the Samsung PATA controller. This driver is based on the
> > Libata subsystem and references the earlier patches sent for IDE
subsystem.
> >
> > Signed-off-by: Abhilash Kesavan <a.kesavan@samsung.com>
> > Signed-off-by: Kukjin Kim <kgene.kim@samsung.com>
> > ---
> > drivers/ata/Kconfig | 9 +
> > drivers/ata/Makefile | 1 +
> > drivers/ata/pata_samsung.c | 591
> ++++++++++++++++++++++++++++++++++++++++++++
> > 3 files changed, 601 insertions(+), 0 deletions(-)
> > create mode 100644 drivers/ata/pata_samsung.c
> >
> > diff --git a/drivers/ata/Kconfig b/drivers/ata/Kconfig
> > index e68541f..ce40f66 100644
> > --- a/drivers/ata/Kconfig
> > +++ b/drivers/ata/Kconfig
> > @@ -640,6 +640,15 @@ config PATA_RZ1000
> >
> > If unsure, say N.
> >
> > +config PATA_SAMSUNG
> > + tristate "Samsung SoC PATA support"
> > + depends on S3C_DEV_IDE
> > + help
> > + This option enables basic support for Samsung's S3C/S5P board
> > + PATA controllers via the new ATA layer
> > +
> > + If unsure, say N.
> > +
> > config PATA_SC1200
> > tristate "SC1200 PATA support"
> > depends on PCI
> > diff --git a/drivers/ata/Makefile b/drivers/ata/Makefile
> > index d0a93c4..0b6d29a 100644
> > --- a/drivers/ata/Makefile
> > +++ b/drivers/ata/Makefile
> > @@ -62,6 +62,7 @@ obj-$(CONFIG_PATA_RADISYS) += pata_radisys.o
> > obj-$(CONFIG_PATA_RB532) += pata_rb532_cf.o
> > obj-$(CONFIG_PATA_RDC) += pata_rdc.o
> > obj-$(CONFIG_PATA_RZ1000) += pata_rz1000.o
> > +obj-$(CONFIG_PATA_SAMSUNG) += pata_samsung.o
> do you want to call it pata_samsung_soc.o instead, in case samsung make
> otehr types of pata controller?
Will change the name to pata_samsung_cf.
> > obj-$(CONFIG_PATA_SC1200) += pata_sc1200.o
> > obj-$(CONFIG_PATA_SERVERWORKS) += pata_serverworks.o
> > obj-$(CONFIG_PATA_SIL680) += pata_sil680.o
> > diff --git a/drivers/ata/pata_samsung.c b/drivers/ata/pata_samsung.c
> > new file mode 100644
> > index 0000000..14381e4
> > --- /dev/null
> > +++ b/drivers/ata/pata_samsung.c
> > @@ -0,0 +1,591 @@
> > +/* linux/drivers/ata/pata_samsung.c
> > + *
> > + * Copyright (c) 2010 Samsung Electronics Co., Ltd.
> > + * http://www.samsung.com
> > + *
> > + * PATA driver for Samsung SoC's.
> SoCs.
OK
> > + * Supports CF Interface in True IDE mode. Currently only PIO mode has
been
> > + * implemented; UDMA support has to be added.
> > + *
> > + * Based on:
> > + * PATA driver for AT91SAM9260 Static Memory Controller
> > + * PATA driver for Toshiba SCC controller
> > + *
> > + * 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/kernel.h>
> > +#include <linux/module.h>
> > +#include <linux/init.h>
> > +#include <linux/clk.h>
> > +#include <linux/libata.h>
> > +#include <linux/platform_device.h>
> > +#include <linux/slab.h>
> > +
> > +#include <plat/ata.h>
> > +#include <plat/regs-ata.h>
> > +
> > +#define DRV_NAME "pata_samsung"
> > +#define DRV_VERSION "0.1"
> > +
> > +enum s3c_cpu_type {
> > + TYPE_S3C6400,
> > + TYPE_S5PC100,
> > + TYPE_S5PV210,
> > +};
> > +
> > +/*
> > + * struct s3c_ide_info - S3C PATA instance.
> > + * @clk: The clock resource for this controller.
> > + * @ide_addr: The area mapped for the hardware registers.
> > + * @sfr_addr: The area mapped for the special function registers.
> > + * @irq: The IRQ number we are using.
> > + * @cpu_type: The exact type of this controller.
> > + * @fifo_status_reg: The ATA_FIFO_STATUS register offset.
> > + */
> > +struct s3c_ide_info {
> > + struct clk *clk;
> > + void __iomem *ide_addr;
> > + void __iomem *sfr_addr;
> > + unsigned int irq;
> > + enum s3c_cpu_type cpu_type;
> > + unsigned int fifo_status_reg;
> > +};
> > +
> > +static unsigned long piotime[5];
> > +
> > +static void pata_s3c_set_endian(void *s3c_ide_regbase, u8 mode)
> > +{
> > + u32 reg = readl(s3c_ide_regbase + S3C_ATA_CFG);
> > + reg = mode ? (reg & ~S3C_ATA_CFG_SWAP) : (reg |
> S3C_ATA_CFG_SWAP);
> > + writel(reg, s3c_ide_regbase + S3C_ATA_CFG);
> > +}
> > +
> > +static void pata_s3c_tune_chipset(struct s3c_ide_info *info, u8
ide_mode)
> > +{
> > + ulong ata_cfg = readl(info->ide_addr + S3C_ATA_CFG);
> > +
> > + /* IORDY is enabled for modes > PIO2 */
> > + if (XFER_PIO_0 <= ide_mode && ide_mode <= XFER_PIO_4) {
> > +
> > + switch (ide_mode) {
> > + case XFER_PIO_0:
> > + case XFER_PIO_1:
> > + case XFER_PIO_2:
> > + ata_cfg &= (~S3C_ATA_CFG_IORDYEN);
> > + break;
> > + case XFER_PIO_3:
> > + case XFER_PIO_4:
> > + ata_cfg |= S3C_ATA_CFG_IORDYEN;
> > + break;
> > + }
> > +
> > + writel(ata_cfg, info->ide_addr + S3C_ATA_CFG);
> > + writel(piotime[ide_mode - XFER_PIO_0],
> > + info->ide_addr + S3C_ATA_PIO_TIME);
> > + } else {
> > + /* Default to PIO0 */
> > + ata_cfg &= (~S3C_ATA_CFG_IORDYEN);
> technically no need to ().
OK
> > + writel(ata_cfg, info->ide_addr + S3C_ATA_CFG);
> > + writel(piotime[0], info->ide_addr + S3C_ATA_PIO_TIME);
> > + }
> > +}
> > +
> > +static void
> > +pata_s3c_set_piomode(struct ata_port *ap, struct ata_device *adev)
> > +{
> > + struct s3c_ide_info *info = ap->host->private_data;
> > +
> > + /* Configure IORDY based on PIO mode and also set the timing value */
> > + pata_s3c_tune_chipset(info, adev->pio_mode);
> > +}
> > +
> > +/*
> > + * Waits until the IDE controller is able to perform next read/write
> > + * operation to the disk. Needed for 64XX series boards only.
> > + */
> > +static int wait_for_host_ready(struct s3c_ide_info *info)
> > +{
> > + ulong timeout;
> > +
> > + /* wait for maximum of 20 msec */
> > + timeout = jiffies + msecs_to_jiffies(20);
> > + while (time_before(jiffies, timeout)) {
> > + if ((readl(info->ide_addr + info->fifo_status_reg) >> 28) ==
0)
> > + return 0;
> > + }
> > + return -EBUSY;
> > +}
> > +
> > +/*
> > + * Writes to one of the task file registers.
> > + */
> > +static void ata_outb(struct ata_host *host, u8 addr, void __iomem *reg)
> > +{
> > + struct s3c_ide_info *info = host->private_data;
> > +
> > + wait_for_host_ready(info);
> > + __raw_writeb(addr, reg);
> > +}
>
> probably want writeb() here. you've used the non __raw versions elsewhere.
>
OK
> > +/*
> > + * Reads from one of the task file registers.
> > + */
> > +static u8 ata_inb(struct ata_host *host, void __iomem *reg)
> > +{
> > + struct s3c_ide_info *info = host->private_data;
> > + u8 temp;
> > +
> > + wait_for_host_ready(info);
> > + temp = __raw_readb(reg);
> > + wait_for_host_ready(info);
> > + temp = __raw_readb(info->ide_addr + S3C_ATA_PIO_RDATA);
> > + return temp;
> > +}
> > +
> > +/*
> > + * pata_s3c_tf_load - send taskfile registers to host controller
> > + */
> > +static void pata_s3c_tf_load(struct ata_port *ap,
> > + const struct ata_taskfile *tf)
> > +{
> > + struct ata_ioports *ioaddr = &ap->ioaddr;
> > + unsigned int is_addr = tf->flags & ATA_TFLAG_ISADDR;
> > +
> > + if (tf->ctl != ap->last_ctl) {
> > + if (ioaddr->ctl_addr)
> > + ata_outb(ap->host, tf->ctl, ioaddr->ctl_addr);
> > + ap->last_ctl = tf->ctl;
> > + ata_wait_idle(ap);
> > + }
> > +
> > + if (is_addr && (tf->flags & ATA_TFLAG_LBA48)) {
> > + WARN_ON_ONCE(!ioaddr->ctl_addr);
> > + ata_outb(ap->host, tf->hob_feature, ioaddr->feature_addr);
> > + ata_outb(ap->host, tf->hob_nsect, ioaddr->nsect_addr);
> > + ata_outb(ap->host, tf->hob_lbal, ioaddr->lbal_addr);
> > + ata_outb(ap->host, tf->hob_lbam, ioaddr->lbam_addr);
> > + ata_outb(ap->host, tf->hob_lbah, ioaddr->lbah_addr);
> > + }
> > +
> > + if (is_addr) {
> > + ata_outb(ap->host, tf->feature, ioaddr->feature_addr);
> > + ata_outb(ap->host, tf->nsect, ioaddr->nsect_addr);
> > + ata_outb(ap->host, tf->lbal, ioaddr->lbal_addr);
> > + ata_outb(ap->host, tf->lbam, ioaddr->lbam_addr);
> > + ata_outb(ap->host, tf->lbah, ioaddr->lbah_addr);
> > + }
> > +
> > + if (tf->flags & ATA_TFLAG_DEVICE)
> > + ata_outb(ap->host, tf->device, ioaddr->device_addr);
> > +
> > + ata_wait_idle(ap);
> > +}
> > +
> > +/*
> > + * pata_s3c_tf_read - input device's ATA taskfile shadow registers
> > + */
> > +static void pata_s3c_tf_read(struct ata_port *ap, struct ata_taskfile
*tf)
> > +{
> > + struct ata_ioports *ioaddr = &ap->ioaddr;
> > +
> > + tf->command = ata_sff_check_status(ap);
> > + tf->feature = ata_inb(ap->host, ioaddr->error_addr);
> > + tf->nsect = ata_inb(ap->host, ioaddr->nsect_addr);
> > + tf->lbal = ata_inb(ap->host, ioaddr->lbal_addr);
> > + tf->lbam = ata_inb(ap->host, ioaddr->lbam_addr);
> > + tf->lbah = ata_inb(ap->host, ioaddr->lbah_addr);
> > + tf->device = ata_inb(ap->host, ioaddr->device_addr);
> > +
> > + if (tf->flags & ATA_TFLAG_LBA48) {
> > + if (likely(ioaddr->ctl_addr)) {
> > + iowrite8(tf->ctl | ATA_HOB, ioaddr->ctl_addr);
> > + tf->hob_feature = ata_inb(ap->host,
ioaddr->error_addr);
> > + tf->hob_nsect = ata_inb(ap->host,
ioaddr->nsect_addr);
> > + tf->hob_lbal = ata_inb(ap->host, ioaddr->lbal_addr);
> > + tf->hob_lbam = ata_inb(ap->host, ioaddr->lbam_addr);
> > + tf->hob_lbah = ata_inb(ap->host, ioaddr->lbah_addr);
> > + iowrite8(tf->ctl, ioaddr->ctl_addr);
> > + ap->last_ctl = tf->ctl;
> > + } else
> > + WARN_ON_ONCE(1);
> > + }
> > +}
> > +
> > +/*
> > + * pata_s3c_exec_command - issue ATA command to host controller
> > + */
> > +static void pata_s3c_exec_command(struct ata_port *ap,
> > + const struct ata_taskfile *tf)
> > +{
> > + ata_outb(ap->host, tf->command, ap->ioaddr.command_addr);
> > + ata_sff_pause(ap);
> > +}
> > +
> > +/*
> > + * pata_s3c_check_status - Read device status register
> > + */
> > +static u8 pata_s3c_check_status(struct ata_port *ap)
> > +{
> > + return ata_inb(ap->host, ap->ioaddr.status_addr);
> > +}
> > +
> > +/*
> > + * pata_s3c_data_xfer - Transfer data by PIO
> > + */
> > +unsigned int pata_s3c_data_xfer(struct ata_device *dev, unsigned char
*buf,
> > + unsigned int buflen, int rw)
> > +{
> > + struct ata_port *ap = dev->link->ap;
> > + struct s3c_ide_info *info = ap->host->private_data;
> > + void __iomem *data_addr = ap->ioaddr.data_addr;
> > + unsigned int words = buflen >> 1, i;
> > + u16 *temp_addr = (u16 *)buf;
>
> hmm, how about calling it data_ptr instead of temp_addr?
>
OK
> > + if (rw == READ) {
> > + for (i = 0; i < words; i++, temp_addr++) {
> > + wait_for_host_ready(info);
> > + *temp_addr = __raw_readw(data_addr);
> > + wait_for_host_ready(info);
> > + *temp_addr = __raw_readw(info->ide_addr
> > + + S3C_ATA_PIO_RDATA)
>
> think you want readw() here, please ensure consitency.
>
> this also look a bit weird, you are writing to *temp_addr twice,
> could you simply dicscard the first read?
>
> note, you say s3c64xx only needs wait_for_host_ready() but you
> always call it?
>
wait_for_host_ready is called via pata_s3c_port_ops which is only for 64xx.
> > + }
> > + } else {
> > + for (i = 0; i < words; i++, temp_addr++) {
> > + wait_for_host_ready(info);
> > + writel(*temp_addr, data_addr);
> > + }
> > + }
> > +
> > + return words << 1;
> > +}
> > +
> > +static struct scsi_host_template pata_s3c_sht = {
> > + ATA_PIO_SHT(DRV_NAME),
> > +};
> > +
> > +static struct ata_port_operations pata_s3c_port_ops = {
> > + .inherits = &ata_sff_port_ops,
> > + .sff_check_status = pata_s3c_check_status,
> > + .sff_tf_load = pata_s3c_tf_load,
> > + .sff_tf_read = pata_s3c_tf_read,
> > + .sff_data_xfer = pata_s3c_data_xfer,
> > + .sff_exec_command = pata_s3c_exec_command,
> > + .qc_prep = ata_noop_qc_prep,
> > + .set_piomode = pata_s3c_set_piomode,
> > +};
> > +
> > +static struct ata_port_operations pata_s5p_port_ops = {
> > + .inherits = &ata_sff_port_ops,
> > + .qc_prep = ata_noop_qc_prep,
> > + .set_piomode = pata_s3c_set_piomode,
> > +};
> > +
> > +static void pata_s3c_enable(void *s3c_ide_regbase, u8 state)
> > +{
> > + u32 temp = readl(s3c_ide_regbase + S3C_ATA_CTRL);
> > + temp = state ? temp | 1 : temp & ~1;
> you might want ()around the temp |1 and temp & ~1
OK
> > + writel(temp, s3c_ide_regbase + S3C_ATA_CTRL);
> > +}
> > +
> > +static irqreturn_t pata_s3c_irq(int irq, void *dev_instance)
> > +{
> > + struct ata_host *host = dev_instance;
> > + struct s3c_ide_info *info = host->private_data;
> > + u32 reg;
> > +
> > + reg = readl(info->ide_addr + S3C_ATA_IRQ);
> > + writel(reg, info->ide_addr + S3C_ATA_IRQ);
> > +
> > + return ata_sff_interrupt(irq, dev_instance);
> > +}
> > +
> > +static void pata_s3c_setup_timing(struct s3c_ide_info *info)
> > +{
> > + uint t1, t2, teoc, i;
> > +
> > + uint pio_t1[5] = { 70, 50, 30, 30, 25 };
> > + uint pio_t2[5] = { 290, 290, 290, 80, 70 };
> > + uint pio_teoc[5] = { 240, 43, 10, 70, 25 };
> > +
> > + ulong cycle_time = (uint) (1000000000 / clk_get_rate(info->clk));
>
> ulong and uinit? surely pick one. you might want to keep UL and cajhnge
> 1000000000 to 1000000000UL
>
OK
> > + for (i = 0; i < 5; i++) {
> > + t1 = (pio_t1[i] / cycle_time) & 0x0f;
> > + t2 = (pio_t2[i] / cycle_time) & 0xff;
> > + teoc = (pio_teoc[i] / cycle_time) & 0xff;
> > + piotime[i] = (teoc << 12) | (t2 << 4) | t1;
> > + }
> > +}
> > +
> > +static void pata_s3c_hwinit(struct s3c_ide_info *info,
> > + struct s3c_ide_platdata *pdata)
> > +{
> > + /* Select true-ide as the internal operating mode*/
> > + if (pdata && (pdata->cfg_mode))
> > + pdata->cfg_mode(info->sfr_addr);
> > +
> > + switch (info->cpu_type) {
> > + case TYPE_S3C6400:
> > + /* Configure as big endian and enable the i/f*/
> > + pata_s3c_set_endian(info->ide_addr, 1);
> > + pata_s3c_enable(info->ide_addr, 1);
> > + mdelay(100);
> > +
> > + /* Remove IRQ Status */
> > + writel(0x1f, info->ide_addr + S3C_ATA_IRQ);
> > + writel(0x1b, info->ide_addr + S3C_ATA_IRQ_MSK);
> > + break;
> > +
> > + case TYPE_S5PV210:
> > + case TYPE_S5PC100:
> > + /* Configure as little endian and enable the i/f */
> > + pata_s3c_set_endian(info->ide_addr, 0);
> > + pata_s3c_enable(info->ide_addr, 1);
> > + mdelay(100);
> > +
> > + /* Remove IRQ Status */
> > + writel(0x3f, info->ide_addr + S3C_ATA_IRQ);
> > + writel(0x3f, info->ide_addr + S3C_ATA_IRQ_MSK);
> > + break;
> > +
> > + default:
> > + BUG();
> > + }
> > +}
> > +
> > +static int __devinit pata_s3c_probe(struct platform_device *pdev)
> > +{
> > + struct s3c_ide_platdata *pdata = pdev->dev.platform_data;
> > + struct device *dev = &pdev->dev;
> > + struct s3c_ide_info *info;
> > + struct resource *res;
> > + struct ata_port *ap;
> > + struct ata_host *host;
> > + enum s3c_cpu_type cpu_type;
> > + int ret;
> > +
> > + cpu_type = platform_get_device_id(pdev)->driver_data;
> > +
> > + info = devm_kzalloc(dev, sizeof(*info), GFP_KERNEL);
> > + if (!info) {
> > + dev_err(dev, "failed to allocate memory for device data\n");
> > + return -ENOMEM;
> > + }
> > +
> > + info->irq = platform_get_irq(pdev, 0);
> > + if (info->irq < 0) {
> > + dev_err(dev, "could not obtain irq number\n");
> > + ret = -ENODEV;
> > + goto release_device_mem;
> > + }
> > +
> > + res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> > + if (res == NULL) {
> > + dev_err(dev, "failed to get mem resource\n");
> > + ret = -ENODEV;
>
> if i remeber correctly, you don't want to be returning -ENODEV as
> it will be ignored by the uper layers.
>
Will replace with EINVAL.
> > + goto release_device_mem;
> > + }
> > +
> > + info->ide_addr = devm_ioremap(dev,
> > + res->start, res->end - res->start + 1);
>
> resource_size() is your friend here.
>
OK
> > + if (!info->ide_addr) {
> > + dev_err(dev, "failed to map IO base address\n");
> > + ret = -ENOMEM;
> > + goto release_mem;
> > + }
> > +
> > + info->clk = clk_get(&pdev->dev, "cfcon");
>
> need to do something about using explicit names here, should be clk_get(,
NULL)
>
OK
> > + if (IS_ERR(info->clk)) {
> > + dev_err(dev, "failed to get access to cf controller
clock\n");
> > + ret = PTR_ERR(info->clk);
> > + info->clk = NULL;
> > + goto unmap;
> > + }
> > +
> > + if (clk_enable(info->clk)) {
> > + dev_err(dev, "failed to enable clock source.\n");
> > + goto clkerr;
> > + }
> > +
> > + /* init ata host */
> > + host = ata_host_alloc(dev, 1);
> > + if (!host) {
> > + dev_err(dev, "failed to allocate ide host\n");
> > + ret = -ENOMEM;
> > + goto stop_clk;
> > + }
> > +
> > + ap = host->ports[0];
> > + ap->flags |= ATA_FLAG_MMIO;
> > + ap->pio_mask = ATA_PIO4;
> > +
> > + if (cpu_type == TYPE_S3C6400) {
> > + ap->ops = &pata_s3c_port_ops;
> > + info->sfr_addr = info->ide_addr + 0x1800;
> > + info->ide_addr = info->ide_addr + 0x1900;
> > + info->fifo_status_reg = 0x94;
> > + } else if (cpu_type == TYPE_S5PC100) {
> > + ap->ops = &pata_s5p_port_ops;
> > + info->sfr_addr = info->ide_addr + 0x1800;
> > + info->ide_addr = info->ide_addr + 0x1900;
> > + info->fifo_status_reg = 0x84;
> > + } else {
> > + ap->ops = &pata_s5p_port_ops;
> > + info->fifo_status_reg = 0x84;
> > + }
> > +
> > + info->cpu_type = cpu_type;
> > +
> > + if (!(info->irq)) {
> > + ap->flags |= ATA_FLAG_PIO_POLLING;
> > + ata_port_desc(ap, "no IRQ, using PIO polling\n");
> > + }
> > +
> > + ap->ioaddr.cmd_addr = info->ide_addr + S3C_ATA_CMD;
> > + ap->ioaddr.data_addr = info->ide_addr + S3C_ATA_PIO_DTR;
> > + ap->ioaddr.error_addr = info->ide_addr + S3C_ATA_PIO_FED;
> > + ap->ioaddr.feature_addr = info->ide_addr + S3C_ATA_PIO_FED;
> > + ap->ioaddr.nsect_addr = info->ide_addr + S3C_ATA_PIO_SCR;
> > + ap->ioaddr.lbal_addr = info->ide_addr + S3C_ATA_PIO_LLR;
> > + ap->ioaddr.lbam_addr = info->ide_addr + S3C_ATA_PIO_LMR;
> > + ap->ioaddr.lbah_addr = info->ide_addr + S3C_ATA_PIO_LHR;
> > + ap->ioaddr.device_addr = info->ide_addr + S3C_ATA_PIO_DVR;
> > + ap->ioaddr.status_addr = info->ide_addr + S3C_ATA_PIO_CSD;
> > + ap->ioaddr.command_addr = info->ide_addr + S3C_ATA_PIO_CSD;
> > + ap->ioaddr.altstatus_addr = info->ide_addr + S3C_ATA_PIO_DAD;
> > + ap->ioaddr.ctl_addr = info->ide_addr + S3C_ATA_PIO_DAD;
> > +
> > +
> > + ata_port_desc(ap, "mmio cmd 0x%llx ",
> > + (unsigned long long)res->start);
> > +
> > + host->private_data = info;
> > +
> > + /* Calculates timing parameters for PIO mode */
> > + pata_s3c_setup_timing(info);
> > +
> > + if (pdata && (pdata->setup_gpio))
> no need for () around the pdata->setup_gpio
OK
> > + pdata->setup_gpio();
> > +
> > + /* Set endianness and enable the interface */
> > + pata_s3c_hwinit(info, pdata);
> > +
> > + return ata_host_activate(host, info->irq ? info->irq : 0,
> > + info->irq ? pata_s3c_irq : NULL,
> > + IRQF_DISABLED, &pata_s3c_sht);
>
> do you really need to run with IRQs disabled?
>
No.. will correct it
> > + dev_set_drvdata(&pdev->dev, host);
> > +
> > +stop_clk:
> > + clk_disable(info->clk);
> > +clkerr:
> > + clk_put(info->clk);
> > +unmap:
> > + devm_iounmap(dev, info->ide_addr);
> > +release_mem:
> > + release_mem_region(res->start, res->end - res->start + 1);
> > +release_device_mem:
> > + kfree(info);
> > +return ret;
> > +}
> > +
> > +static int __devexit pata_s3c_remove(struct platform_device *pdev)
> > +{
> > + struct ata_host *host = dev_get_drvdata(&pdev->dev);
> > + struct s3c_ide_info *info;
> > + struct resource *res;
> > +
> > + if (!host)
> > + return 0;
> > + info = host->private_data;
> > +
> > + ata_host_detach(host);
> > +
> > + devm_iounmap(&pdev->dev, info->ide_addr);
> > + clk_disable(info->clk);
> > + clk_put(info->clk);
> > + res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> > + release_mem_region(res->start, res->end - res->start + 1);
> > + kfree(info);
> > +
> > + return 0;
> > +}
> > +
> > +#ifdef CONFIG_PM
> > +static int pata_s3c_suspend(struct platform_device *pdev, pm_message_t
state)
> > +{
> > + struct ata_host *host = dev_get_drvdata(&pdev->dev);
> > + if (host)
> > + return ata_host_suspend(host, state);
> > + else
> > + return 0;
> > +}
> > +
> > +static int pata_s3c_resume(struct platform_device *pdev)
> > +{
> > + struct ata_host *host = dev_get_drvdata(&pdev->dev);
> > + struct s3c_ide_platdata *pdata = pdev->dev.platform_data;
> > + struct s3c_ide_info *info;
> > +
> > + info = host->private_data;
> > +
> > + if (host) {
> > + pata_s3c_hwinit(info, pdata);
> > + ata_host_resume(host);
> > + }
> > +
> > + return 0;
> > +}
> > +#endif
> > +
> > +/* driver device registration */
> > +static struct platform_device_id pata_s3c_driver_ids[] = {
> > + {
> > + .name = "s3c6400-pata",
> > + .driver_data = TYPE_S3C6400,
> > + }, {
> > + .name = "s5pc100-pata",
> > + .driver_data = TYPE_S5PC100,
> > + }, {
> > + .name = "s5pv210-pata",
> > + .driver_data = TYPE_S5PV210,
> > + },
> > + { }
> > +};
> > +
> > +MODULE_DEVICE_TABLE(platform, pata_s3c_driver_ids);
> > +
> > +static struct platform_driver pata_s3c_driver = {
> > + .probe = pata_s3c_probe,
> > + .remove = __devexit_p(pata_s3c_remove),
> > + .id_table = pata_s3c_driver_ids,
> > + .driver = {
> > + .name = DRV_NAME,
> > + .owner = THIS_MODULE,
> > + },
> > +#ifdef CONFIG_PM
> > + .suspend = pata_s3c_suspend,
> > + .resume = pata_s3c_resume,
> > +#endif
> please look at using the new pm_ops.
>
Will use dev_pm_ops.
> > +};
> > +
> > +static int __init pata_s3c_init(void)
> > +{
> > + return platform_driver_register(&pata_s3c_driver);
> > +}
> > +
> > +static void __exit pata_s3c_exit(void)
> > +{
> > + platform_driver_unregister(&pata_s3c_driver);
> > +}
> > +
> > +module_init(pata_s3c_init);
> > +module_exit(pata_s3c_exit);
> > +
> > +MODULE_AUTHOR("Abhilash Kesavan, <a.kesavan@samsung.com>");
> > +MODULE_DESCRIPTION("low-level driver for Samsung PATA controller");
> > +MODULE_LICENSE("GPL");
> > +MODULE_VERSION(DRV_VERSION);
> > --
> > 1.6.2.5
>
> not bad, needs some cleanup.
>
Thanks.
Best regards,
Kgene.
--
Kukjin Kim <kgene.kim@samsung.com>, Senior Engineer,
SW Solution Development Team, Samsung Electronics Co., Ltd.
^ permalink raw reply [flat|nested] 48+ messages in thread* [PATCH 4/4] pata_samsung: Add Samsung PATA controller driver
@ 2010-06-02 2:37 ` Kukjin Kim
0 siblings, 0 replies; 48+ messages in thread
From: Kukjin Kim @ 2010-06-02 2:37 UTC (permalink / raw)
To: linux-arm-kernel
Ben Dooks wrote:
>
> On Thu, May 27, 2010 at 05:22:04PM +0900, Kukjin Kim wrote:
> > From: Abhilash Kesavan <a.kesavan@samsung.com>
> >
> > Adds support for the Samsung PATA controller. This driver is based on the
> > Libata subsystem and references the earlier patches sent for IDE
subsystem.
> >
> > Signed-off-by: Abhilash Kesavan <a.kesavan@samsung.com>
> > Signed-off-by: Kukjin Kim <kgene.kim@samsung.com>
> > ---
> > drivers/ata/Kconfig | 9 +
> > drivers/ata/Makefile | 1 +
> > drivers/ata/pata_samsung.c | 591
> ++++++++++++++++++++++++++++++++++++++++++++
> > 3 files changed, 601 insertions(+), 0 deletions(-)
> > create mode 100644 drivers/ata/pata_samsung.c
> >
> > diff --git a/drivers/ata/Kconfig b/drivers/ata/Kconfig
> > index e68541f..ce40f66 100644
> > --- a/drivers/ata/Kconfig
> > +++ b/drivers/ata/Kconfig
> > @@ -640,6 +640,15 @@ config PATA_RZ1000
> >
> > If unsure, say N.
> >
> > +config PATA_SAMSUNG
> > + tristate "Samsung SoC PATA support"
> > + depends on S3C_DEV_IDE
> > + help
> > + This option enables basic support for Samsung's S3C/S5P board
> > + PATA controllers via the new ATA layer
> > +
> > + If unsure, say N.
> > +
> > config PATA_SC1200
> > tristate "SC1200 PATA support"
> > depends on PCI
> > diff --git a/drivers/ata/Makefile b/drivers/ata/Makefile
> > index d0a93c4..0b6d29a 100644
> > --- a/drivers/ata/Makefile
> > +++ b/drivers/ata/Makefile
> > @@ -62,6 +62,7 @@ obj-$(CONFIG_PATA_RADISYS) += pata_radisys.o
> > obj-$(CONFIG_PATA_RB532) += pata_rb532_cf.o
> > obj-$(CONFIG_PATA_RDC) += pata_rdc.o
> > obj-$(CONFIG_PATA_RZ1000) += pata_rz1000.o
> > +obj-$(CONFIG_PATA_SAMSUNG) += pata_samsung.o
> do you want to call it pata_samsung_soc.o instead, in case samsung make
> otehr types of pata controller?
Will change the name to pata_samsung_cf.
> > obj-$(CONFIG_PATA_SC1200) += pata_sc1200.o
> > obj-$(CONFIG_PATA_SERVERWORKS) += pata_serverworks.o
> > obj-$(CONFIG_PATA_SIL680) += pata_sil680.o
> > diff --git a/drivers/ata/pata_samsung.c b/drivers/ata/pata_samsung.c
> > new file mode 100644
> > index 0000000..14381e4
> > --- /dev/null
> > +++ b/drivers/ata/pata_samsung.c
> > @@ -0,0 +1,591 @@
> > +/* linux/drivers/ata/pata_samsung.c
> > + *
> > + * Copyright (c) 2010 Samsung Electronics Co., Ltd.
> > + * http://www.samsung.com
> > + *
> > + * PATA driver for Samsung SoC's.
> SoCs.
OK
> > + * Supports CF Interface in True IDE mode. Currently only PIO mode has
been
> > + * implemented; UDMA support has to be added.
> > + *
> > + * Based on:
> > + * PATA driver for AT91SAM9260 Static Memory Controller
> > + * PATA driver for Toshiba SCC controller
> > + *
> > + * 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/kernel.h>
> > +#include <linux/module.h>
> > +#include <linux/init.h>
> > +#include <linux/clk.h>
> > +#include <linux/libata.h>
> > +#include <linux/platform_device.h>
> > +#include <linux/slab.h>
> > +
> > +#include <plat/ata.h>
> > +#include <plat/regs-ata.h>
> > +
> > +#define DRV_NAME "pata_samsung"
> > +#define DRV_VERSION "0.1"
> > +
> > +enum s3c_cpu_type {
> > + TYPE_S3C6400,
> > + TYPE_S5PC100,
> > + TYPE_S5PV210,
> > +};
> > +
> > +/*
> > + * struct s3c_ide_info - S3C PATA instance.
> > + * @clk: The clock resource for this controller.
> > + * @ide_addr: The area mapped for the hardware registers.
> > + * @sfr_addr: The area mapped for the special function registers.
> > + * @irq: The IRQ number we are using.
> > + * @cpu_type: The exact type of this controller.
> > + * @fifo_status_reg: The ATA_FIFO_STATUS register offset.
> > + */
> > +struct s3c_ide_info {
> > + struct clk *clk;
> > + void __iomem *ide_addr;
> > + void __iomem *sfr_addr;
> > + unsigned int irq;
> > + enum s3c_cpu_type cpu_type;
> > + unsigned int fifo_status_reg;
> > +};
> > +
> > +static unsigned long piotime[5];
> > +
> > +static void pata_s3c_set_endian(void *s3c_ide_regbase, u8 mode)
> > +{
> > + u32 reg = readl(s3c_ide_regbase + S3C_ATA_CFG);
> > + reg = mode ? (reg & ~S3C_ATA_CFG_SWAP) : (reg |
> S3C_ATA_CFG_SWAP);
> > + writel(reg, s3c_ide_regbase + S3C_ATA_CFG);
> > +}
> > +
> > +static void pata_s3c_tune_chipset(struct s3c_ide_info *info, u8
ide_mode)
> > +{
> > + ulong ata_cfg = readl(info->ide_addr + S3C_ATA_CFG);
> > +
> > + /* IORDY is enabled for modes > PIO2 */
> > + if (XFER_PIO_0 <= ide_mode && ide_mode <= XFER_PIO_4) {
> > +
> > + switch (ide_mode) {
> > + case XFER_PIO_0:
> > + case XFER_PIO_1:
> > + case XFER_PIO_2:
> > + ata_cfg &= (~S3C_ATA_CFG_IORDYEN);
> > + break;
> > + case XFER_PIO_3:
> > + case XFER_PIO_4:
> > + ata_cfg |= S3C_ATA_CFG_IORDYEN;
> > + break;
> > + }
> > +
> > + writel(ata_cfg, info->ide_addr + S3C_ATA_CFG);
> > + writel(piotime[ide_mode - XFER_PIO_0],
> > + info->ide_addr + S3C_ATA_PIO_TIME);
> > + } else {
> > + /* Default to PIO0 */
> > + ata_cfg &= (~S3C_ATA_CFG_IORDYEN);
> technically no need to ().
OK
> > + writel(ata_cfg, info->ide_addr + S3C_ATA_CFG);
> > + writel(piotime[0], info->ide_addr + S3C_ATA_PIO_TIME);
> > + }
> > +}
> > +
> > +static void
> > +pata_s3c_set_piomode(struct ata_port *ap, struct ata_device *adev)
> > +{
> > + struct s3c_ide_info *info = ap->host->private_data;
> > +
> > + /* Configure IORDY based on PIO mode and also set the timing value */
> > + pata_s3c_tune_chipset(info, adev->pio_mode);
> > +}
> > +
> > +/*
> > + * Waits until the IDE controller is able to perform next read/write
> > + * operation to the disk. Needed for 64XX series boards only.
> > + */
> > +static int wait_for_host_ready(struct s3c_ide_info *info)
> > +{
> > + ulong timeout;
> > +
> > + /* wait for maximum of 20 msec */
> > + timeout = jiffies + msecs_to_jiffies(20);
> > + while (time_before(jiffies, timeout)) {
> > + if ((readl(info->ide_addr + info->fifo_status_reg) >> 28) ==
0)
> > + return 0;
> > + }
> > + return -EBUSY;
> > +}
> > +
> > +/*
> > + * Writes to one of the task file registers.
> > + */
> > +static void ata_outb(struct ata_host *host, u8 addr, void __iomem *reg)
> > +{
> > + struct s3c_ide_info *info = host->private_data;
> > +
> > + wait_for_host_ready(info);
> > + __raw_writeb(addr, reg);
> > +}
>
> probably want writeb() here. you've used the non __raw versions elsewhere.
>
OK
> > +/*
> > + * Reads from one of the task file registers.
> > + */
> > +static u8 ata_inb(struct ata_host *host, void __iomem *reg)
> > +{
> > + struct s3c_ide_info *info = host->private_data;
> > + u8 temp;
> > +
> > + wait_for_host_ready(info);
> > + temp = __raw_readb(reg);
> > + wait_for_host_ready(info);
> > + temp = __raw_readb(info->ide_addr + S3C_ATA_PIO_RDATA);
> > + return temp;
> > +}
> > +
> > +/*
> > + * pata_s3c_tf_load - send taskfile registers to host controller
> > + */
> > +static void pata_s3c_tf_load(struct ata_port *ap,
> > + const struct ata_taskfile *tf)
> > +{
> > + struct ata_ioports *ioaddr = &ap->ioaddr;
> > + unsigned int is_addr = tf->flags & ATA_TFLAG_ISADDR;
> > +
> > + if (tf->ctl != ap->last_ctl) {
> > + if (ioaddr->ctl_addr)
> > + ata_outb(ap->host, tf->ctl, ioaddr->ctl_addr);
> > + ap->last_ctl = tf->ctl;
> > + ata_wait_idle(ap);
> > + }
> > +
> > + if (is_addr && (tf->flags & ATA_TFLAG_LBA48)) {
> > + WARN_ON_ONCE(!ioaddr->ctl_addr);
> > + ata_outb(ap->host, tf->hob_feature, ioaddr->feature_addr);
> > + ata_outb(ap->host, tf->hob_nsect, ioaddr->nsect_addr);
> > + ata_outb(ap->host, tf->hob_lbal, ioaddr->lbal_addr);
> > + ata_outb(ap->host, tf->hob_lbam, ioaddr->lbam_addr);
> > + ata_outb(ap->host, tf->hob_lbah, ioaddr->lbah_addr);
> > + }
> > +
> > + if (is_addr) {
> > + ata_outb(ap->host, tf->feature, ioaddr->feature_addr);
> > + ata_outb(ap->host, tf->nsect, ioaddr->nsect_addr);
> > + ata_outb(ap->host, tf->lbal, ioaddr->lbal_addr);
> > + ata_outb(ap->host, tf->lbam, ioaddr->lbam_addr);
> > + ata_outb(ap->host, tf->lbah, ioaddr->lbah_addr);
> > + }
> > +
> > + if (tf->flags & ATA_TFLAG_DEVICE)
> > + ata_outb(ap->host, tf->device, ioaddr->device_addr);
> > +
> > + ata_wait_idle(ap);
> > +}
> > +
> > +/*
> > + * pata_s3c_tf_read - input device's ATA taskfile shadow registers
> > + */
> > +static void pata_s3c_tf_read(struct ata_port *ap, struct ata_taskfile
*tf)
> > +{
> > + struct ata_ioports *ioaddr = &ap->ioaddr;
> > +
> > + tf->command = ata_sff_check_status(ap);
> > + tf->feature = ata_inb(ap->host, ioaddr->error_addr);
> > + tf->nsect = ata_inb(ap->host, ioaddr->nsect_addr);
> > + tf->lbal = ata_inb(ap->host, ioaddr->lbal_addr);
> > + tf->lbam = ata_inb(ap->host, ioaddr->lbam_addr);
> > + tf->lbah = ata_inb(ap->host, ioaddr->lbah_addr);
> > + tf->device = ata_inb(ap->host, ioaddr->device_addr);
> > +
> > + if (tf->flags & ATA_TFLAG_LBA48) {
> > + if (likely(ioaddr->ctl_addr)) {
> > + iowrite8(tf->ctl | ATA_HOB, ioaddr->ctl_addr);
> > + tf->hob_feature = ata_inb(ap->host,
ioaddr->error_addr);
> > + tf->hob_nsect = ata_inb(ap->host,
ioaddr->nsect_addr);
> > + tf->hob_lbal = ata_inb(ap->host, ioaddr->lbal_addr);
> > + tf->hob_lbam = ata_inb(ap->host, ioaddr->lbam_addr);
> > + tf->hob_lbah = ata_inb(ap->host, ioaddr->lbah_addr);
> > + iowrite8(tf->ctl, ioaddr->ctl_addr);
> > + ap->last_ctl = tf->ctl;
> > + } else
> > + WARN_ON_ONCE(1);
> > + }
> > +}
> > +
> > +/*
> > + * pata_s3c_exec_command - issue ATA command to host controller
> > + */
> > +static void pata_s3c_exec_command(struct ata_port *ap,
> > + const struct ata_taskfile *tf)
> > +{
> > + ata_outb(ap->host, tf->command, ap->ioaddr.command_addr);
> > + ata_sff_pause(ap);
> > +}
> > +
> > +/*
> > + * pata_s3c_check_status - Read device status register
> > + */
> > +static u8 pata_s3c_check_status(struct ata_port *ap)
> > +{
> > + return ata_inb(ap->host, ap->ioaddr.status_addr);
> > +}
> > +
> > +/*
> > + * pata_s3c_data_xfer - Transfer data by PIO
> > + */
> > +unsigned int pata_s3c_data_xfer(struct ata_device *dev, unsigned char
*buf,
> > + unsigned int buflen, int rw)
> > +{
> > + struct ata_port *ap = dev->link->ap;
> > + struct s3c_ide_info *info = ap->host->private_data;
> > + void __iomem *data_addr = ap->ioaddr.data_addr;
> > + unsigned int words = buflen >> 1, i;
> > + u16 *temp_addr = (u16 *)buf;
>
> hmm, how about calling it data_ptr instead of temp_addr?
>
OK
> > + if (rw == READ) {
> > + for (i = 0; i < words; i++, temp_addr++) {
> > + wait_for_host_ready(info);
> > + *temp_addr = __raw_readw(data_addr);
> > + wait_for_host_ready(info);
> > + *temp_addr = __raw_readw(info->ide_addr
> > + + S3C_ATA_PIO_RDATA)
>
> think you want readw() here, please ensure consitency.
>
> this also look a bit weird, you are writing to *temp_addr twice,
> could you simply dicscard the first read?
>
> note, you say s3c64xx only needs wait_for_host_ready() but you
> always call it?
>
wait_for_host_ready is called via pata_s3c_port_ops which is only for 64xx.
> > + }
> > + } else {
> > + for (i = 0; i < words; i++, temp_addr++) {
> > + wait_for_host_ready(info);
> > + writel(*temp_addr, data_addr);
> > + }
> > + }
> > +
> > + return words << 1;
> > +}
> > +
> > +static struct scsi_host_template pata_s3c_sht = {
> > + ATA_PIO_SHT(DRV_NAME),
> > +};
> > +
> > +static struct ata_port_operations pata_s3c_port_ops = {
> > + .inherits = &ata_sff_port_ops,
> > + .sff_check_status = pata_s3c_check_status,
> > + .sff_tf_load = pata_s3c_tf_load,
> > + .sff_tf_read = pata_s3c_tf_read,
> > + .sff_data_xfer = pata_s3c_data_xfer,
> > + .sff_exec_command = pata_s3c_exec_command,
> > + .qc_prep = ata_noop_qc_prep,
> > + .set_piomode = pata_s3c_set_piomode,
> > +};
> > +
> > +static struct ata_port_operations pata_s5p_port_ops = {
> > + .inherits = &ata_sff_port_ops,
> > + .qc_prep = ata_noop_qc_prep,
> > + .set_piomode = pata_s3c_set_piomode,
> > +};
> > +
> > +static void pata_s3c_enable(void *s3c_ide_regbase, u8 state)
> > +{
> > + u32 temp = readl(s3c_ide_regbase + S3C_ATA_CTRL);
> > + temp = state ? temp | 1 : temp & ~1;
> you might want ()around the temp |1 and temp & ~1
OK
> > + writel(temp, s3c_ide_regbase + S3C_ATA_CTRL);
> > +}
> > +
> > +static irqreturn_t pata_s3c_irq(int irq, void *dev_instance)
> > +{
> > + struct ata_host *host = dev_instance;
> > + struct s3c_ide_info *info = host->private_data;
> > + u32 reg;
> > +
> > + reg = readl(info->ide_addr + S3C_ATA_IRQ);
> > + writel(reg, info->ide_addr + S3C_ATA_IRQ);
> > +
> > + return ata_sff_interrupt(irq, dev_instance);
> > +}
> > +
> > +static void pata_s3c_setup_timing(struct s3c_ide_info *info)
> > +{
> > + uint t1, t2, teoc, i;
> > +
> > + uint pio_t1[5] = { 70, 50, 30, 30, 25 };
> > + uint pio_t2[5] = { 290, 290, 290, 80, 70 };
> > + uint pio_teoc[5] = { 240, 43, 10, 70, 25 };
> > +
> > + ulong cycle_time = (uint) (1000000000 / clk_get_rate(info->clk));
>
> ulong and uinit? surely pick one. you might want to keep UL and cajhnge
> 1000000000 to 1000000000UL
>
OK
> > + for (i = 0; i < 5; i++) {
> > + t1 = (pio_t1[i] / cycle_time) & 0x0f;
> > + t2 = (pio_t2[i] / cycle_time) & 0xff;
> > + teoc = (pio_teoc[i] / cycle_time) & 0xff;
> > + piotime[i] = (teoc << 12) | (t2 << 4) | t1;
> > + }
> > +}
> > +
> > +static void pata_s3c_hwinit(struct s3c_ide_info *info,
> > + struct s3c_ide_platdata *pdata)
> > +{
> > + /* Select true-ide as the internal operating mode*/
> > + if (pdata && (pdata->cfg_mode))
> > + pdata->cfg_mode(info->sfr_addr);
> > +
> > + switch (info->cpu_type) {
> > + case TYPE_S3C6400:
> > + /* Configure as big endian and enable the i/f*/
> > + pata_s3c_set_endian(info->ide_addr, 1);
> > + pata_s3c_enable(info->ide_addr, 1);
> > + mdelay(100);
> > +
> > + /* Remove IRQ Status */
> > + writel(0x1f, info->ide_addr + S3C_ATA_IRQ);
> > + writel(0x1b, info->ide_addr + S3C_ATA_IRQ_MSK);
> > + break;
> > +
> > + case TYPE_S5PV210:
> > + case TYPE_S5PC100:
> > + /* Configure as little endian and enable the i/f */
> > + pata_s3c_set_endian(info->ide_addr, 0);
> > + pata_s3c_enable(info->ide_addr, 1);
> > + mdelay(100);
> > +
> > + /* Remove IRQ Status */
> > + writel(0x3f, info->ide_addr + S3C_ATA_IRQ);
> > + writel(0x3f, info->ide_addr + S3C_ATA_IRQ_MSK);
> > + break;
> > +
> > + default:
> > + BUG();
> > + }
> > +}
> > +
> > +static int __devinit pata_s3c_probe(struct platform_device *pdev)
> > +{
> > + struct s3c_ide_platdata *pdata = pdev->dev.platform_data;
> > + struct device *dev = &pdev->dev;
> > + struct s3c_ide_info *info;
> > + struct resource *res;
> > + struct ata_port *ap;
> > + struct ata_host *host;
> > + enum s3c_cpu_type cpu_type;
> > + int ret;
> > +
> > + cpu_type = platform_get_device_id(pdev)->driver_data;
> > +
> > + info = devm_kzalloc(dev, sizeof(*info), GFP_KERNEL);
> > + if (!info) {
> > + dev_err(dev, "failed to allocate memory for device data\n");
> > + return -ENOMEM;
> > + }
> > +
> > + info->irq = platform_get_irq(pdev, 0);
> > + if (info->irq < 0) {
> > + dev_err(dev, "could not obtain irq number\n");
> > + ret = -ENODEV;
> > + goto release_device_mem;
> > + }
> > +
> > + res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> > + if (res == NULL) {
> > + dev_err(dev, "failed to get mem resource\n");
> > + ret = -ENODEV;
>
> if i remeber correctly, you don't want to be returning -ENODEV as
> it will be ignored by the uper layers.
>
Will replace with EINVAL.
> > + goto release_device_mem;
> > + }
> > +
> > + info->ide_addr = devm_ioremap(dev,
> > + res->start, res->end - res->start + 1);
>
> resource_size() is your friend here.
>
OK
> > + if (!info->ide_addr) {
> > + dev_err(dev, "failed to map IO base address\n");
> > + ret = -ENOMEM;
> > + goto release_mem;
> > + }
> > +
> > + info->clk = clk_get(&pdev->dev, "cfcon");
>
> need to do something about using explicit names here, should be clk_get(,
NULL)
>
OK
> > + if (IS_ERR(info->clk)) {
> > + dev_err(dev, "failed to get access to cf controller
clock\n");
> > + ret = PTR_ERR(info->clk);
> > + info->clk = NULL;
> > + goto unmap;
> > + }
> > +
> > + if (clk_enable(info->clk)) {
> > + dev_err(dev, "failed to enable clock source.\n");
> > + goto clkerr;
> > + }
> > +
> > + /* init ata host */
> > + host = ata_host_alloc(dev, 1);
> > + if (!host) {
> > + dev_err(dev, "failed to allocate ide host\n");
> > + ret = -ENOMEM;
> > + goto stop_clk;
> > + }
> > +
> > + ap = host->ports[0];
> > + ap->flags |= ATA_FLAG_MMIO;
> > + ap->pio_mask = ATA_PIO4;
> > +
> > + if (cpu_type == TYPE_S3C6400) {
> > + ap->ops = &pata_s3c_port_ops;
> > + info->sfr_addr = info->ide_addr + 0x1800;
> > + info->ide_addr = info->ide_addr + 0x1900;
> > + info->fifo_status_reg = 0x94;
> > + } else if (cpu_type == TYPE_S5PC100) {
> > + ap->ops = &pata_s5p_port_ops;
> > + info->sfr_addr = info->ide_addr + 0x1800;
> > + info->ide_addr = info->ide_addr + 0x1900;
> > + info->fifo_status_reg = 0x84;
> > + } else {
> > + ap->ops = &pata_s5p_port_ops;
> > + info->fifo_status_reg = 0x84;
> > + }
> > +
> > + info->cpu_type = cpu_type;
> > +
> > + if (!(info->irq)) {
> > + ap->flags |= ATA_FLAG_PIO_POLLING;
> > + ata_port_desc(ap, "no IRQ, using PIO polling\n");
> > + }
> > +
> > + ap->ioaddr.cmd_addr = info->ide_addr + S3C_ATA_CMD;
> > + ap->ioaddr.data_addr = info->ide_addr + S3C_ATA_PIO_DTR;
> > + ap->ioaddr.error_addr = info->ide_addr + S3C_ATA_PIO_FED;
> > + ap->ioaddr.feature_addr = info->ide_addr + S3C_ATA_PIO_FED;
> > + ap->ioaddr.nsect_addr = info->ide_addr + S3C_ATA_PIO_SCR;
> > + ap->ioaddr.lbal_addr = info->ide_addr + S3C_ATA_PIO_LLR;
> > + ap->ioaddr.lbam_addr = info->ide_addr + S3C_ATA_PIO_LMR;
> > + ap->ioaddr.lbah_addr = info->ide_addr + S3C_ATA_PIO_LHR;
> > + ap->ioaddr.device_addr = info->ide_addr + S3C_ATA_PIO_DVR;
> > + ap->ioaddr.status_addr = info->ide_addr + S3C_ATA_PIO_CSD;
> > + ap->ioaddr.command_addr = info->ide_addr + S3C_ATA_PIO_CSD;
> > + ap->ioaddr.altstatus_addr = info->ide_addr + S3C_ATA_PIO_DAD;
> > + ap->ioaddr.ctl_addr = info->ide_addr + S3C_ATA_PIO_DAD;
> > +
> > +
> > + ata_port_desc(ap, "mmio cmd 0x%llx ",
> > + (unsigned long long)res->start);
> > +
> > + host->private_data = info;
> > +
> > + /* Calculates timing parameters for PIO mode */
> > + pata_s3c_setup_timing(info);
> > +
> > + if (pdata && (pdata->setup_gpio))
> no need for () around the pdata->setup_gpio
OK
> > + pdata->setup_gpio();
> > +
> > + /* Set endianness and enable the interface */
> > + pata_s3c_hwinit(info, pdata);
> > +
> > + return ata_host_activate(host, info->irq ? info->irq : 0,
> > + info->irq ? pata_s3c_irq : NULL,
> > + IRQF_DISABLED, &pata_s3c_sht);
>
> do you really need to run with IRQs disabled?
>
No.. will correct it
> > + dev_set_drvdata(&pdev->dev, host);
> > +
> > +stop_clk:
> > + clk_disable(info->clk);
> > +clkerr:
> > + clk_put(info->clk);
> > +unmap:
> > + devm_iounmap(dev, info->ide_addr);
> > +release_mem:
> > + release_mem_region(res->start, res->end - res->start + 1);
> > +release_device_mem:
> > + kfree(info);
> > +return ret;
> > +}
> > +
> > +static int __devexit pata_s3c_remove(struct platform_device *pdev)
> > +{
> > + struct ata_host *host = dev_get_drvdata(&pdev->dev);
> > + struct s3c_ide_info *info;
> > + struct resource *res;
> > +
> > + if (!host)
> > + return 0;
> > + info = host->private_data;
> > +
> > + ata_host_detach(host);
> > +
> > + devm_iounmap(&pdev->dev, info->ide_addr);
> > + clk_disable(info->clk);
> > + clk_put(info->clk);
> > + res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> > + release_mem_region(res->start, res->end - res->start + 1);
> > + kfree(info);
> > +
> > + return 0;
> > +}
> > +
> > +#ifdef CONFIG_PM
> > +static int pata_s3c_suspend(struct platform_device *pdev, pm_message_t
state)
> > +{
> > + struct ata_host *host = dev_get_drvdata(&pdev->dev);
> > + if (host)
> > + return ata_host_suspend(host, state);
> > + else
> > + return 0;
> > +}
> > +
> > +static int pata_s3c_resume(struct platform_device *pdev)
> > +{
> > + struct ata_host *host = dev_get_drvdata(&pdev->dev);
> > + struct s3c_ide_platdata *pdata = pdev->dev.platform_data;
> > + struct s3c_ide_info *info;
> > +
> > + info = host->private_data;
> > +
> > + if (host) {
> > + pata_s3c_hwinit(info, pdata);
> > + ata_host_resume(host);
> > + }
> > +
> > + return 0;
> > +}
> > +#endif
> > +
> > +/* driver device registration */
> > +static struct platform_device_id pata_s3c_driver_ids[] = {
> > + {
> > + .name = "s3c6400-pata",
> > + .driver_data = TYPE_S3C6400,
> > + }, {
> > + .name = "s5pc100-pata",
> > + .driver_data = TYPE_S5PC100,
> > + }, {
> > + .name = "s5pv210-pata",
> > + .driver_data = TYPE_S5PV210,
> > + },
> > + { }
> > +};
> > +
> > +MODULE_DEVICE_TABLE(platform, pata_s3c_driver_ids);
> > +
> > +static struct platform_driver pata_s3c_driver = {
> > + .probe = pata_s3c_probe,
> > + .remove = __devexit_p(pata_s3c_remove),
> > + .id_table = pata_s3c_driver_ids,
> > + .driver = {
> > + .name = DRV_NAME,
> > + .owner = THIS_MODULE,
> > + },
> > +#ifdef CONFIG_PM
> > + .suspend = pata_s3c_suspend,
> > + .resume = pata_s3c_resume,
> > +#endif
> please look at using the new pm_ops.
>
Will use dev_pm_ops.
> > +};
> > +
> > +static int __init pata_s3c_init(void)
> > +{
> > + return platform_driver_register(&pata_s3c_driver);
> > +}
> > +
> > +static void __exit pata_s3c_exit(void)
> > +{
> > + platform_driver_unregister(&pata_s3c_driver);
> > +}
> > +
> > +module_init(pata_s3c_init);
> > +module_exit(pata_s3c_exit);
> > +
> > +MODULE_AUTHOR("Abhilash Kesavan, <a.kesavan@samsung.com>");
> > +MODULE_DESCRIPTION("low-level driver for Samsung PATA controller");
> > +MODULE_LICENSE("GPL");
> > +MODULE_VERSION(DRV_VERSION);
> > --
> > 1.6.2.5
>
> not bad, needs some cleanup.
>
Thanks.
Best regards,
Kgene.
--
Kukjin Kim <kgene.kim@samsung.com>, Senior Engineer,
SW Solution Development Team, Samsung Electronics Co., Ltd.
^ permalink raw reply [flat|nested] 48+ messages in thread
* Re: [PATCH 4/4] pata_samsung: Add Samsung PATA controller driver
2010-05-27 8:22 ` Kukjin Kim
@ 2010-05-27 8:43 ` Jassi Brar
-1 siblings, 0 replies; 48+ messages in thread
From: Jassi Brar @ 2010-05-27 8:43 UTC (permalink / raw)
To: Kukjin Kim
Cc: linux-ide, linux-arm-kernel, linux-samsung-soc, ben-linux,
Abhilash Kesavan
On Thu, May 27, 2010 at 5:22 PM, Kukjin Kim <kgene.kim@samsung.com> wrote:
> From: Abhilash Kesavan <a.kesavan@samsung.com>
>
> Adds support for the Samsung PATA controller. This driver is based on the
> Libata subsystem and references the earlier patches sent for IDE subsystem.
>
> Signed-off-by: Abhilash Kesavan <a.kesavan@samsung.com>
> Signed-off-by: Kukjin Kim <kgene.kim@samsung.com>
> ---
> drivers/ata/Kconfig | 9 +
> drivers/ata/Makefile | 1 +
> drivers/ata/pata_samsung.c | 591
Fasten your seat belts before reading further....
Rather than generic 'samsung', I would suggest the driver named
after the SoC, that is supported first(chronologically) in mainline kernel.
All newer SoCs should be simply taken to contain the controller of that SoC.
Otherwise, the same naming problem comes back to haunt us should
Samsung decides to use a different IP in future SoCs. What would we
call that driver? pata_samsung_v2.c ?
my two sparks
Jassi
^ permalink raw reply [flat|nested] 48+ messages in thread
* [PATCH 4/4] pata_samsung: Add Samsung PATA controller driver
@ 2010-05-27 8:43 ` Jassi Brar
0 siblings, 0 replies; 48+ messages in thread
From: Jassi Brar @ 2010-05-27 8:43 UTC (permalink / raw)
To: linux-arm-kernel
On Thu, May 27, 2010 at 5:22 PM, Kukjin Kim <kgene.kim@samsung.com> wrote:
> From: Abhilash Kesavan <a.kesavan@samsung.com>
>
> Adds support for the Samsung PATA controller. This driver is based on the
> Libata subsystem and references the earlier patches sent for IDE subsystem.
>
> Signed-off-by: Abhilash Kesavan <a.kesavan@samsung.com>
> Signed-off-by: Kukjin Kim <kgene.kim@samsung.com>
> ---
> ?drivers/ata/Kconfig ? ? ? ?| ? ?9 +
> ?drivers/ata/Makefile ? ? ? | ? ?1 +
> ?drivers/ata/pata_samsung.c | ?591
Fasten your seat belts before reading further....
Rather than generic 'samsung', I would suggest the driver named
after the SoC, that is supported first(chronologically) in mainline kernel.
All newer SoCs should be simply taken to contain the controller of that SoC.
Otherwise, the same naming problem comes back to haunt us should
Samsung decides to use a different IP in future SoCs. What would we
call that driver? pata_samsung_v2.c ?
my two sparks
Jassi
^ permalink raw reply [flat|nested] 48+ messages in thread
* Re: [PATCH 4/4] pata_samsung: Add Samsung PATA controller driver
2010-05-27 8:43 ` Jassi Brar
@ 2010-05-27 8:57 ` Ben Dooks
-1 siblings, 0 replies; 48+ messages in thread
From: Ben Dooks @ 2010-05-27 8:57 UTC (permalink / raw)
To: Jassi Brar
Cc: Kukjin Kim, linux-ide, linux-arm-kernel, linux-samsung-soc,
ben-linux, Abhilash Kesavan
On Thu, May 27, 2010 at 05:43:47PM +0900, Jassi Brar wrote:
> On Thu, May 27, 2010 at 5:22 PM, Kukjin Kim <kgene.kim@samsung.com> wrote:
> > From: Abhilash Kesavan <a.kesavan@samsung.com>
> >
> > Adds support for the Samsung PATA controller. This driver is based on the
> > Libata subsystem and references the earlier patches sent for IDE subsystem.
> >
> > Signed-off-by: Abhilash Kesavan <a.kesavan@samsung.com>
> > Signed-off-by: Kukjin Kim <kgene.kim@samsung.com>
> > ---
> > drivers/ata/Kconfig | 9 +
> > drivers/ata/Makefile | 1 +
> > drivers/ata/pata_samsung.c | 591
>
> Fasten your seat belts before reading further....
>
> Rather than generic 'samsung', I would suggest the driver named
> after the SoC, that is supported first(chronologically) in mainline kernel.
> All newer SoCs should be simply taken to contain the controller of that SoC.
> Otherwise, the same naming problem comes back to haunt us should
> Samsung decides to use a different IP in future SoCs. What would we
> call that driver? pata_samsung_v2.c ?
I'm not so bothered, but it could be pata_samsung_cfcon or anything,
a new block could be called pata_samsung_v2 or fred for all I really
care about this.
> my two sparks
> Jassi
--
--
Ben
Q: What's a light-year?
A: One-third less calories than a regular year.
^ permalink raw reply [flat|nested] 48+ messages in thread
* [PATCH 4/4] pata_samsung: Add Samsung PATA controller driver
@ 2010-05-27 8:57 ` Ben Dooks
0 siblings, 0 replies; 48+ messages in thread
From: Ben Dooks @ 2010-05-27 8:57 UTC (permalink / raw)
To: linux-arm-kernel
On Thu, May 27, 2010 at 05:43:47PM +0900, Jassi Brar wrote:
> On Thu, May 27, 2010 at 5:22 PM, Kukjin Kim <kgene.kim@samsung.com> wrote:
> > From: Abhilash Kesavan <a.kesavan@samsung.com>
> >
> > Adds support for the Samsung PATA controller. This driver is based on the
> > Libata subsystem and references the earlier patches sent for IDE subsystem.
> >
> > Signed-off-by: Abhilash Kesavan <a.kesavan@samsung.com>
> > Signed-off-by: Kukjin Kim <kgene.kim@samsung.com>
> > ---
> > ?drivers/ata/Kconfig ? ? ? ?| ? ?9 +
> > ?drivers/ata/Makefile ? ? ? | ? ?1 +
> > ?drivers/ata/pata_samsung.c | ?591
>
> Fasten your seat belts before reading further....
>
> Rather than generic 'samsung', I would suggest the driver named
> after the SoC, that is supported first(chronologically) in mainline kernel.
> All newer SoCs should be simply taken to contain the controller of that SoC.
> Otherwise, the same naming problem comes back to haunt us should
> Samsung decides to use a different IP in future SoCs. What would we
> call that driver? pata_samsung_v2.c ?
I'm not so bothered, but it could be pata_samsung_cfcon or anything,
a new block could be called pata_samsung_v2 or fred for all I really
care about this.
> my two sparks
> Jassi
--
--
Ben
Q: What's a light-year?
A: One-third less calories than a regular year.
^ permalink raw reply [flat|nested] 48+ messages in thread
* Re: [PATCH 4/4] pata_samsung: Add Samsung PATA controller driver
2010-05-27 8:57 ` Ben Dooks
@ 2010-05-27 9:18 ` Jassi Brar
-1 siblings, 0 replies; 48+ messages in thread
From: Jassi Brar @ 2010-05-27 9:18 UTC (permalink / raw)
To: Ben Dooks
Cc: Kukjin Kim, linux-ide, linux-arm-kernel, linux-samsung-soc,
Abhilash Kesavan
On Thu, May 27, 2010 at 5:57 PM, Ben Dooks <ben-linux@fluff.org> wrote:
> On Thu, May 27, 2010 at 05:43:47PM +0900, Jassi Brar wrote:
>> On Thu, May 27, 2010 at 5:22 PM, Kukjin Kim <kgene.kim@samsung.com> wrote:
>> > From: Abhilash Kesavan <a.kesavan@samsung.com>
>> >
>> > Adds support for the Samsung PATA controller. This driver is based on the
>> > Libata subsystem and references the earlier patches sent for IDE subsystem.
>> >
>> > Signed-off-by: Abhilash Kesavan <a.kesavan@samsung.com>
>> > Signed-off-by: Kukjin Kim <kgene.kim@samsung.com>
>> > ---
>> > drivers/ata/Kconfig | 9 +
>> > drivers/ata/Makefile | 1 +
>> > drivers/ata/pata_samsung.c | 591
>>
>> Fasten your seat belts before reading further....
>>
>> Rather than generic 'samsung', I would suggest the driver named
>> after the SoC, that is supported first(chronologically) in mainline kernel.
>> All newer SoCs should be simply taken to contain the controller of that SoC.
>> Otherwise, the same naming problem comes back to haunt us should
>> Samsung decides to use a different IP in future SoCs. What would we
>> call that driver? pata_samsung_v2.c ?
>
> I'm not so bothered, but it could be pata_samsung_cfcon or anything,
> a new block could be called pata_samsung_v2 or fred
well, then pata_samsung.c is better for we know chances of this IP change in
future SoCs are quite slim.
And even if it does change we shall call it
pata_samsung_really_final_this_time.c ;)
^ permalink raw reply [flat|nested] 48+ messages in thread
* [PATCH 4/4] pata_samsung: Add Samsung PATA controller driver
@ 2010-05-27 9:18 ` Jassi Brar
0 siblings, 0 replies; 48+ messages in thread
From: Jassi Brar @ 2010-05-27 9:18 UTC (permalink / raw)
To: linux-arm-kernel
On Thu, May 27, 2010 at 5:57 PM, Ben Dooks <ben-linux@fluff.org> wrote:
> On Thu, May 27, 2010 at 05:43:47PM +0900, Jassi Brar wrote:
>> On Thu, May 27, 2010 at 5:22 PM, Kukjin Kim <kgene.kim@samsung.com> wrote:
>> > From: Abhilash Kesavan <a.kesavan@samsung.com>
>> >
>> > Adds support for the Samsung PATA controller. This driver is based on the
>> > Libata subsystem and references the earlier patches sent for IDE subsystem.
>> >
>> > Signed-off-by: Abhilash Kesavan <a.kesavan@samsung.com>
>> > Signed-off-by: Kukjin Kim <kgene.kim@samsung.com>
>> > ---
>> > ?drivers/ata/Kconfig ? ? ? ?| ? ?9 +
>> > ?drivers/ata/Makefile ? ? ? | ? ?1 +
>> > ?drivers/ata/pata_samsung.c | ?591
>>
>> Fasten your seat belts before reading further....
>>
>> Rather than generic 'samsung', I would suggest the driver named
>> after the SoC, that is supported first(chronologically) in mainline kernel.
>> All newer SoCs should be simply taken to contain the controller of that SoC.
>> Otherwise, the same naming problem comes back to haunt us should
>> Samsung decides to use a different IP in future SoCs. What would we
>> call that driver? pata_samsung_v2.c ?
>
> I'm not so bothered, but it could be pata_samsung_cfcon or anything,
> a new block could be called pata_samsung_v2 or fred
well, then pata_samsung.c is better for we know chances of this IP change in
future SoCs are quite slim.
And even if it does change we shall call it
pata_samsung_really_final_this_time.c ;)
^ permalink raw reply [flat|nested] 48+ messages in thread
* Re: [PATCH 4/4] pata_samsung: Add Samsung PATA controller driver
2010-05-27 8:22 ` Kukjin Kim
@ 2010-05-27 10:17 ` Sergei Shtylyov
-1 siblings, 0 replies; 48+ messages in thread
From: Sergei Shtylyov @ 2010-05-27 10:17 UTC (permalink / raw)
To: Kukjin Kim
Cc: linux-ide, linux-arm-kernel, linux-samsung-soc, ben-linux,
Abhilash Kesavan
Hello.
Kukjin Kim wrote:
> From: Abhilash Kesavan <a.kesavan@samsung.com>
> Adds support for the Samsung PATA controller. This driver is based on the
> Libata subsystem and references the earlier patches sent for IDE subsystem.
> Signed-off-by: Abhilash Kesavan <a.kesavan@samsung.com>
> Signed-off-by: Kukjin Kim <kgene.kim@samsung.com>
> diff --git a/drivers/ata/pata_samsung.c b/drivers/ata/pata_samsung.c
> new file mode 100644
> index 0000000..14381e4
> --- /dev/null
> +++ b/drivers/ata/pata_samsung.c
> @@ -0,0 +1,591 @@
[...]
> +static void pata_s3c_tune_chipset(struct s3c_ide_info *info, u8 ide_mode)
> +{
> + ulong ata_cfg = readl(info->ide_addr + S3C_ATA_CFG);
> +
> + /* IORDY is enabled for modes > PIO2 */
> + if (XFER_PIO_0 <= ide_mode && ide_mode <= XFER_PIO_4) {
> +
> + switch (ide_mode) {
> + case XFER_PIO_0:
> + case XFER_PIO_1:
> + case XFER_PIO_2:
> + ata_cfg &= (~S3C_ATA_CFG_IORDYEN);
Useless parens.
> + break;
> + case XFER_PIO_3:
> + case XFER_PIO_4:
> + ata_cfg |= S3C_ATA_CFG_IORDYEN;
IORDY should be controlled by ata_id_pio_need_iordy(), not by mode
number. PIO2 also can have IODRY enabled.
> + break;
> + }
> +
> + writel(ata_cfg, info->ide_addr + S3C_ATA_CFG);
> + writel(piotime[ide_mode - XFER_PIO_0],
> + info->ide_addr + S3C_ATA_PIO_TIME);
> + } else {
> + /* Default to PIO0 */
> + ata_cfg &= (~S3C_ATA_CFG_IORDYEN);
> + writel(ata_cfg, info->ide_addr + S3C_ATA_CFG);
> + writel(piotime[0], info->ide_addr + S3C_ATA_PIO_TIME);
If you don't support DMA modes or modes higher than PIO4 (PIO5 and
PIO6 would have been good for CF though), just do nothing.
> + }
> +}
> +
> +static void
> +pata_s3c_set_piomode(struct ata_port *ap, struct ata_device *adev)
> +{
> + struct s3c_ide_info *info = ap->host->private_data;
> +
> + /* Configure IORDY based on PIO mode and also set the timing value */
> + pata_s3c_tune_chipset(info, adev->pio_mode);
Don't see why you need a separate function for that. Maybe in
preparation to UDMA support?
> +/*
> + * Writes to one of the task file registers.
> + */
> +static void ata_outb(struct ata_host *host, u8 addr, void __iomem *reg)
> +{
> + struct s3c_ide_info *info = host->private_data;
> +
> + wait_for_host_ready(info);
> + __raw_writeb(addr, reg);
You should use write?() or __raw_write?() consistently...
> +/*
> + * pata_s3c_tf_load - send taskfile registers to host controller
> + */
> +static void pata_s3c_tf_load(struct ata_port *ap,
> + const struct ata_taskfile *tf)
> +{
> + struct ata_ioports *ioaddr = &ap->ioaddr;
> + unsigned int is_addr = tf->flags & ATA_TFLAG_ISADDR;
> +
> + if (tf->ctl != ap->last_ctl) {
> + if (ioaddr->ctl_addr)
This register does exist and you assign ioaddr->ctl_addr in
pata_s3c_probe(), hence the check is pointless.
> + ata_outb(ap->host, tf->ctl, ioaddr->ctl_addr);
> + ap->last_ctl = tf->ctl;
> + ata_wait_idle(ap);
> + }
> +
> + if (is_addr && (tf->flags & ATA_TFLAG_LBA48)) {
> + WARN_ON_ONCE(!ioaddr->ctl_addr);
You don't need this either.
> +/*
> + * pata_s3c_tf_read - input device's ATA taskfile shadow registers
> + */
> +static void pata_s3c_tf_read(struct ata_port *ap, struct ata_taskfile *tf)
> +{
> + struct ata_ioports *ioaddr = &ap->ioaddr;
> +
> + tf->command = ata_sff_check_status(ap);
But you have overriden this method, so ata_sff_check_status()
shouldn't work!
> + tf->feature = ata_inb(ap->host, ioaddr->error_addr);
> + tf->nsect = ata_inb(ap->host, ioaddr->nsect_addr);
> + tf->lbal = ata_inb(ap->host, ioaddr->lbal_addr);
> + tf->lbam = ata_inb(ap->host, ioaddr->lbam_addr);
> + tf->lbah = ata_inb(ap->host, ioaddr->lbah_addr);
> + tf->device = ata_inb(ap->host, ioaddr->device_addr);
> +
> + if (tf->flags & ATA_TFLAG_LBA48) {
> + if (likely(ioaddr->ctl_addr)) {
Not just likely, it's always true. Emilinate the check please.
> + iowrite8(tf->ctl | ATA_HOB, ioaddr->ctl_addr);
> + tf->hob_feature = ata_inb(ap->host, ioaddr->error_addr);
> + tf->hob_nsect = ata_inb(ap->host, ioaddr->nsect_addr);
> + tf->hob_lbal = ata_inb(ap->host, ioaddr->lbal_addr);
> + tf->hob_lbam = ata_inb(ap->host, ioaddr->lbam_addr);
> + tf->hob_lbah = ata_inb(ap->host, ioaddr->lbah_addr);
> + iowrite8(tf->ctl, ioaddr->ctl_addr);
> + ap->last_ctl = tf->ctl;
> + } else
> + WARN_ON_ONCE(1);
Not needed.
> +/*
> + * pata_s3c_data_xfer - Transfer data by PIO
> + */
> +unsigned int pata_s3c_data_xfer(struct ata_device *dev, unsigned char *buf,
> + unsigned int buflen, int rw)
> +{
> + struct ata_port *ap = dev->link->ap;
> + struct s3c_ide_info *info = ap->host->private_data;
> + void __iomem *data_addr = ap->ioaddr.data_addr;
> + unsigned int words = buflen >> 1, i;
> + u16 *temp_addr = (u16 *)buf;
> +
> + if (rw == READ) {
{} not needed.
> + for (i = 0; i < words; i++, temp_addr++) {
> + wait_for_host_ready(info);
> + *temp_addr = __raw_readw(data_addr);
> + wait_for_host_ready(info);
> + *temp_addr = __raw_readw(info->ide_addr
> + + S3C_ATA_PIO_RDATA);
> + }
> + } else {
{} not needed.
> + for (i = 0; i < words; i++, temp_addr++) {
> + wait_for_host_ready(info);
> + writel(*temp_addr, data_addr);
> + }
> + }
Well, if this is pere CF case, 'buflen' can't be odd, but otherwise
you should handle that case...
> +
> + return words << 1;
> +}
> +
> +static struct scsi_host_template pata_s3c_sht = {
> + ATA_PIO_SHT(DRV_NAME),
> +};
> +
> +static struct ata_port_operations pata_s3c_port_ops = {
> + .inherits = &ata_sff_port_ops,
> + .sff_check_status = pata_s3c_check_status,
Since simple iowrite8() isn't enough in your case, you also need to
override sff_dev_select(), sff_check_altstatus(), and sff_set_devctl()
methods...
> + .sff_tf_load = pata_s3c_tf_load,
> + .sff_tf_read = pata_s3c_tf_read,
> + .sff_data_xfer = pata_s3c_data_xfer,
> + .sff_exec_command = pata_s3c_exec_command,
> + .qc_prep = ata_noop_qc_prep,
> + .set_piomode = pata_s3c_set_piomode,
> +};
> +
> +static struct ata_port_operations pata_s5p_port_ops = {
> + .inherits = &ata_sff_port_ops,
> + .qc_prep = ata_noop_qc_prep,
> + .set_piomode = pata_s3c_set_piomode,
> +};
[...]
> +static void pata_s3c_setup_timing(struct s3c_ide_info *info)
> +{
> + uint t1, t2, teoc, i;
> +
> + uint pio_t1[5] = { 70, 50, 30, 30, 25 };
Could use ata_timing_find_mode() here to get the mode timings,
intead of duplicating them from libata-ocre.c...
> + uint pio_t2[5] = { 290, 290, 290, 80, 70 };
Note that those active times are for command cycles, not for data ones.
> + uint pio_teoc[5] = { 240, 43, 10, 70, 25 };
What timing is this? :-O
> +
> + ulong cycle_time = (uint) (1000000000 / clk_get_rate(info->clk));
> +
> + for (i = 0; i < 5; i++) {
> + t1 = (pio_t1[i] / cycle_time) & 0x0f;
> + t2 = (pio_t2[i] / cycle_time) & 0xff;
Could use ata_timing_compute() here.
> + teoc = (pio_teoc[i] / cycle_time) & 0xff;
> + piotime[i] = (teoc << 12) | (t2 << 4) | t1;
> + }
> +}
> +
> +static void pata_s3c_hwinit(struct s3c_ide_info *info,
> + struct s3c_ide_platdata *pdata)
> +{
> + /* Select true-ide as the internal operating mode*/
> + if (pdata && (pdata->cfg_mode))
> + pdata->cfg_mode(info->sfr_addr);
> +
> + switch (info->cpu_type) {
> + case TYPE_S3C6400:
> + /* Configure as big endian and enable the i/f*/
Put space before */, please.
> +static int __devinit pata_s3c_probe(struct platform_device *pdev)
> +{
> + struct s3c_ide_platdata *pdata = pdev->dev.platform_data;
> + struct device *dev = &pdev->dev;
> + struct s3c_ide_info *info;
> + struct resource *res;
> + struct ata_port *ap;
> + struct ata_host *host;
> + enum s3c_cpu_type cpu_type;
> + int ret;
> +
> + cpu_type = platform_get_device_id(pdev)->driver_data;
> +
> + info = devm_kzalloc(dev, sizeof(*info), GFP_KERNEL);
> + if (!info) {
> + dev_err(dev, "failed to allocate memory for device data\n");
> + return -ENOMEM;
> + }
> +
> + info->irq = platform_get_irq(pdev, 0);
> + if (info->irq < 0) {
> + dev_err(dev, "could not obtain irq number\n");
> + ret = -ENODEV;
> + goto release_device_mem;
> + }
> +
> + res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> + if (res == NULL) {
> + dev_err(dev, "failed to get mem resource\n");
> + ret = -ENODEV;
> + goto release_device_mem;
> + }
> +
> + info->ide_addr = devm_ioremap(dev,
> + res->start, res->end - res->start + 1);
> + if (!info->ide_addr) {
> + dev_err(dev, "failed to map IO base address\n");
> + ret = -ENOMEM;
> + goto release_mem;
> + }
> +
> + info->clk = clk_get(&pdev->dev, "cfcon");
> + if (IS_ERR(info->clk)) {
> + dev_err(dev, "failed to get access to cf controller clock\n");
> + ret = PTR_ERR(info->clk);
> + info->clk = NULL;
> + goto unmap;
> + }
> +
> + if (clk_enable(info->clk)) {
Can it really fail?
> + dev_err(dev, "failed to enable clock source.\n");
> + goto clkerr;
> + }
> +
> + /* init ata host */
> + host = ata_host_alloc(dev, 1);
> + if (!host) {
> + dev_err(dev, "failed to allocate ide host\n");
> + ret = -ENOMEM;
> + goto stop_clk;
> + }
> +
> + ap = host->ports[0];
> + ap->flags |= ATA_FLAG_MMIO;
> + ap->pio_mask = ATA_PIO4;
> +
> + if (cpu_type == TYPE_S3C6400) {
> + ap->ops = &pata_s3c_port_ops;
> + info->sfr_addr = info->ide_addr + 0x1800;
> + info->ide_addr = info->ide_addr + 0x1900;
> + info->fifo_status_reg = 0x94;
> + } else if (cpu_type == TYPE_S5PC100) {
> + ap->ops = &pata_s5p_port_ops;
> + info->sfr_addr = info->ide_addr + 0x1800;
> + info->ide_addr = info->ide_addr + 0x1900;
Does make sense to assign those before *if*.
> + info->fifo_status_reg = 0x84;
> + } else {
> + ap->ops = &pata_s5p_port_ops;
You don't assign 'info->ide_addr' here but you'll need it in
pata_s3c_tune_chipset() which will be called thru pata_s5p_port_ops!
> + info->fifo_status_reg = 0x84;
> + }
> +
> + info->cpu_type = cpu_type;
> +
> + if (!(info->irq)) {
Parens not needed around 'info->irq'.
> + ap->flags |= ATA_FLAG_PIO_POLLING;
> + ata_port_desc(ap, "no IRQ, using PIO polling\n");
> + }
> +
> + ap->ioaddr.cmd_addr = info->ide_addr + S3C_ATA_CMD;
> + ap->ioaddr.data_addr = info->ide_addr + S3C_ATA_PIO_DTR;
> + ap->ioaddr.error_addr = info->ide_addr + S3C_ATA_PIO_FED;
> + ap->ioaddr.feature_addr = info->ide_addr + S3C_ATA_PIO_FED;
> + ap->ioaddr.nsect_addr = info->ide_addr + S3C_ATA_PIO_SCR;
> + ap->ioaddr.lbal_addr = info->ide_addr + S3C_ATA_PIO_LLR;
> + ap->ioaddr.lbam_addr = info->ide_addr + S3C_ATA_PIO_LMR;
> + ap->ioaddr.lbah_addr = info->ide_addr + S3C_ATA_PIO_LHR;
> + ap->ioaddr.device_addr = info->ide_addr + S3C_ATA_PIO_DVR;
> + ap->ioaddr.status_addr = info->ide_addr + S3C_ATA_PIO_CSD;
> + ap->ioaddr.command_addr = info->ide_addr + S3C_ATA_PIO_CSD;
> + ap->ioaddr.altstatus_addr = info->ide_addr + S3C_ATA_PIO_DAD;
> + ap->ioaddr.ctl_addr = info->ide_addr + S3C_ATA_PIO_DAD;
> +
> +
Extra newline?
> + ata_port_desc(ap, "mmio cmd 0x%llx ",
> + (unsigned long long)res->start);
> +
> + host->private_data = info;
> +
> + /* Calculates timing parameters for PIO mode */
> + pata_s3c_setup_timing(info);
You could do it right in pata_s3c_tune_chipset() -- no need to
precalculate the timings.
> + if (pdata && (pdata->setup_gpio))
Prens not needed around 'pdata->setup_gpio'.
> + pdata->setup_gpio();
> +
> + /* Set endianness and enable the interface */
> + pata_s3c_hwinit(info, pdata);
> +
> + return ata_host_activate(host, info->irq ? info->irq : 0,
This ?: doesn't make sense, just pass 'info->irq'.
> + info->irq ? pata_s3c_irq : NULL,
> + IRQF_DISABLED, &pata_s3c_sht);
> +
> + dev_set_drvdata(&pdev->dev, host);
Could use platform_set_drvdata(pdev, host)...
> +
> +stop_clk:
> + clk_disable(info->clk);
> +clkerr:
> + clk_put(info->clk);
> +unmap:
> + devm_iounmap(dev, info->ide_addr);
devm_ioremap() should "look after itself", so no explicait call
should be needed, if I don't mistake...
> +release_mem:
> + release_mem_region(res->start, res->end - res->start + 1);
But you didn't call request_mem_region()!
> +release_device_mem:
> + kfree(info);
> +return ret;
> +}
> +
> +static int __devexit pata_s3c_remove(struct platform_device *pdev)
> +{
> + struct ata_host *host = dev_get_drvdata(&pdev->dev);
Could use platform_get_drvdata(pdev)...
> + struct s3c_ide_info *info;
> + struct resource *res;
> +
> + if (!host)
> + return 0;
> + info = host->private_data;
> +
> + ata_host_detach(host);
> +
> + devm_iounmap(&pdev->dev, info->ide_addr);
> + clk_disable(info->clk);
> + clk_put(info->clk);
> + res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> + release_mem_region(res->start, res->end - res->start + 1);
Use resource_size(). Also, you don't call request_mem_region().
> + kfree(info);
> +
> + return 0;
> +}
> +
> +#ifdef CONFIG_PM
> +static int pata_s3c_suspend(struct platform_device *pdev, pm_message_t state)
> +{
> + struct ata_host *host = dev_get_drvdata(&pdev->dev);
Could use platform_get_drvdata(pdev)...
> + if (host)
> + return ata_host_suspend(host, state);
> + else
> + return 0;
> +}
> +
> +static int pata_s3c_resume(struct platform_device *pdev)
> +{
> + struct ata_host *host = dev_get_drvdata(&pdev->dev);
Could use platform_get_drvdata(pdev)...
MBR, Sergei
^ permalink raw reply [flat|nested] 48+ messages in thread* [PATCH 4/4] pata_samsung: Add Samsung PATA controller driver
@ 2010-05-27 10:17 ` Sergei Shtylyov
0 siblings, 0 replies; 48+ messages in thread
From: Sergei Shtylyov @ 2010-05-27 10:17 UTC (permalink / raw)
To: linux-arm-kernel
Hello.
Kukjin Kim wrote:
> From: Abhilash Kesavan <a.kesavan@samsung.com>
> Adds support for the Samsung PATA controller. This driver is based on the
> Libata subsystem and references the earlier patches sent for IDE subsystem.
> Signed-off-by: Abhilash Kesavan <a.kesavan@samsung.com>
> Signed-off-by: Kukjin Kim <kgene.kim@samsung.com>
> diff --git a/drivers/ata/pata_samsung.c b/drivers/ata/pata_samsung.c
> new file mode 100644
> index 0000000..14381e4
> --- /dev/null
> +++ b/drivers/ata/pata_samsung.c
> @@ -0,0 +1,591 @@
[...]
> +static void pata_s3c_tune_chipset(struct s3c_ide_info *info, u8 ide_mode)
> +{
> + ulong ata_cfg = readl(info->ide_addr + S3C_ATA_CFG);
> +
> + /* IORDY is enabled for modes > PIO2 */
> + if (XFER_PIO_0 <= ide_mode && ide_mode <= XFER_PIO_4) {
> +
> + switch (ide_mode) {
> + case XFER_PIO_0:
> + case XFER_PIO_1:
> + case XFER_PIO_2:
> + ata_cfg &= (~S3C_ATA_CFG_IORDYEN);
Useless parens.
> + break;
> + case XFER_PIO_3:
> + case XFER_PIO_4:
> + ata_cfg |= S3C_ATA_CFG_IORDYEN;
IORDY should be controlled by ata_id_pio_need_iordy(), not by mode
number. PIO2 also can have IODRY enabled.
> + break;
> + }
> +
> + writel(ata_cfg, info->ide_addr + S3C_ATA_CFG);
> + writel(piotime[ide_mode - XFER_PIO_0],
> + info->ide_addr + S3C_ATA_PIO_TIME);
> + } else {
> + /* Default to PIO0 */
> + ata_cfg &= (~S3C_ATA_CFG_IORDYEN);
> + writel(ata_cfg, info->ide_addr + S3C_ATA_CFG);
> + writel(piotime[0], info->ide_addr + S3C_ATA_PIO_TIME);
If you don't support DMA modes or modes higher than PIO4 (PIO5 and
PIO6 would have been good for CF though), just do nothing.
> + }
> +}
> +
> +static void
> +pata_s3c_set_piomode(struct ata_port *ap, struct ata_device *adev)
> +{
> + struct s3c_ide_info *info = ap->host->private_data;
> +
> + /* Configure IORDY based on PIO mode and also set the timing value */
> + pata_s3c_tune_chipset(info, adev->pio_mode);
Don't see why you need a separate function for that. Maybe in
preparation to UDMA support?
> +/*
> + * Writes to one of the task file registers.
> + */
> +static void ata_outb(struct ata_host *host, u8 addr, void __iomem *reg)
> +{
> + struct s3c_ide_info *info = host->private_data;
> +
> + wait_for_host_ready(info);
> + __raw_writeb(addr, reg);
You should use write?() or __raw_write?() consistently...
> +/*
> + * pata_s3c_tf_load - send taskfile registers to host controller
> + */
> +static void pata_s3c_tf_load(struct ata_port *ap,
> + const struct ata_taskfile *tf)
> +{
> + struct ata_ioports *ioaddr = &ap->ioaddr;
> + unsigned int is_addr = tf->flags & ATA_TFLAG_ISADDR;
> +
> + if (tf->ctl != ap->last_ctl) {
> + if (ioaddr->ctl_addr)
This register does exist and you assign ioaddr->ctl_addr in
pata_s3c_probe(), hence the check is pointless.
> + ata_outb(ap->host, tf->ctl, ioaddr->ctl_addr);
> + ap->last_ctl = tf->ctl;
> + ata_wait_idle(ap);
> + }
> +
> + if (is_addr && (tf->flags & ATA_TFLAG_LBA48)) {
> + WARN_ON_ONCE(!ioaddr->ctl_addr);
You don't need this either.
> +/*
> + * pata_s3c_tf_read - input device's ATA taskfile shadow registers
> + */
> +static void pata_s3c_tf_read(struct ata_port *ap, struct ata_taskfile *tf)
> +{
> + struct ata_ioports *ioaddr = &ap->ioaddr;
> +
> + tf->command = ata_sff_check_status(ap);
But you have overriden this method, so ata_sff_check_status()
shouldn't work!
> + tf->feature = ata_inb(ap->host, ioaddr->error_addr);
> + tf->nsect = ata_inb(ap->host, ioaddr->nsect_addr);
> + tf->lbal = ata_inb(ap->host, ioaddr->lbal_addr);
> + tf->lbam = ata_inb(ap->host, ioaddr->lbam_addr);
> + tf->lbah = ata_inb(ap->host, ioaddr->lbah_addr);
> + tf->device = ata_inb(ap->host, ioaddr->device_addr);
> +
> + if (tf->flags & ATA_TFLAG_LBA48) {
> + if (likely(ioaddr->ctl_addr)) {
Not just likely, it's always true. Emilinate the check please.
> + iowrite8(tf->ctl | ATA_HOB, ioaddr->ctl_addr);
> + tf->hob_feature = ata_inb(ap->host, ioaddr->error_addr);
> + tf->hob_nsect = ata_inb(ap->host, ioaddr->nsect_addr);
> + tf->hob_lbal = ata_inb(ap->host, ioaddr->lbal_addr);
> + tf->hob_lbam = ata_inb(ap->host, ioaddr->lbam_addr);
> + tf->hob_lbah = ata_inb(ap->host, ioaddr->lbah_addr);
> + iowrite8(tf->ctl, ioaddr->ctl_addr);
> + ap->last_ctl = tf->ctl;
> + } else
> + WARN_ON_ONCE(1);
Not needed.
> +/*
> + * pata_s3c_data_xfer - Transfer data by PIO
> + */
> +unsigned int pata_s3c_data_xfer(struct ata_device *dev, unsigned char *buf,
> + unsigned int buflen, int rw)
> +{
> + struct ata_port *ap = dev->link->ap;
> + struct s3c_ide_info *info = ap->host->private_data;
> + void __iomem *data_addr = ap->ioaddr.data_addr;
> + unsigned int words = buflen >> 1, i;
> + u16 *temp_addr = (u16 *)buf;
> +
> + if (rw == READ) {
{} not needed.
> + for (i = 0; i < words; i++, temp_addr++) {
> + wait_for_host_ready(info);
> + *temp_addr = __raw_readw(data_addr);
> + wait_for_host_ready(info);
> + *temp_addr = __raw_readw(info->ide_addr
> + + S3C_ATA_PIO_RDATA);
> + }
> + } else {
{} not needed.
> + for (i = 0; i < words; i++, temp_addr++) {
> + wait_for_host_ready(info);
> + writel(*temp_addr, data_addr);
> + }
> + }
Well, if this is pere CF case, 'buflen' can't be odd, but otherwise
you should handle that case...
> +
> + return words << 1;
> +}
> +
> +static struct scsi_host_template pata_s3c_sht = {
> + ATA_PIO_SHT(DRV_NAME),
> +};
> +
> +static struct ata_port_operations pata_s3c_port_ops = {
> + .inherits = &ata_sff_port_ops,
> + .sff_check_status = pata_s3c_check_status,
Since simple iowrite8() isn't enough in your case, you also need to
override sff_dev_select(), sff_check_altstatus(), and sff_set_devctl()
methods...
> + .sff_tf_load = pata_s3c_tf_load,
> + .sff_tf_read = pata_s3c_tf_read,
> + .sff_data_xfer = pata_s3c_data_xfer,
> + .sff_exec_command = pata_s3c_exec_command,
> + .qc_prep = ata_noop_qc_prep,
> + .set_piomode = pata_s3c_set_piomode,
> +};
> +
> +static struct ata_port_operations pata_s5p_port_ops = {
> + .inherits = &ata_sff_port_ops,
> + .qc_prep = ata_noop_qc_prep,
> + .set_piomode = pata_s3c_set_piomode,
> +};
[...]
> +static void pata_s3c_setup_timing(struct s3c_ide_info *info)
> +{
> + uint t1, t2, teoc, i;
> +
> + uint pio_t1[5] = { 70, 50, 30, 30, 25 };
Could use ata_timing_find_mode() here to get the mode timings,
intead of duplicating them from libata-ocre.c...
> + uint pio_t2[5] = { 290, 290, 290, 80, 70 };
Note that those active times are for command cycles, not for data ones.
> + uint pio_teoc[5] = { 240, 43, 10, 70, 25 };
What timing is this? :-O
> +
> + ulong cycle_time = (uint) (1000000000 / clk_get_rate(info->clk));
> +
> + for (i = 0; i < 5; i++) {
> + t1 = (pio_t1[i] / cycle_time) & 0x0f;
> + t2 = (pio_t2[i] / cycle_time) & 0xff;
Could use ata_timing_compute() here.
> + teoc = (pio_teoc[i] / cycle_time) & 0xff;
> + piotime[i] = (teoc << 12) | (t2 << 4) | t1;
> + }
> +}
> +
> +static void pata_s3c_hwinit(struct s3c_ide_info *info,
> + struct s3c_ide_platdata *pdata)
> +{
> + /* Select true-ide as the internal operating mode*/
> + if (pdata && (pdata->cfg_mode))
> + pdata->cfg_mode(info->sfr_addr);
> +
> + switch (info->cpu_type) {
> + case TYPE_S3C6400:
> + /* Configure as big endian and enable the i/f*/
Put space before */, please.
> +static int __devinit pata_s3c_probe(struct platform_device *pdev)
> +{
> + struct s3c_ide_platdata *pdata = pdev->dev.platform_data;
> + struct device *dev = &pdev->dev;
> + struct s3c_ide_info *info;
> + struct resource *res;
> + struct ata_port *ap;
> + struct ata_host *host;
> + enum s3c_cpu_type cpu_type;
> + int ret;
> +
> + cpu_type = platform_get_device_id(pdev)->driver_data;
> +
> + info = devm_kzalloc(dev, sizeof(*info), GFP_KERNEL);
> + if (!info) {
> + dev_err(dev, "failed to allocate memory for device data\n");
> + return -ENOMEM;
> + }
> +
> + info->irq = platform_get_irq(pdev, 0);
> + if (info->irq < 0) {
> + dev_err(dev, "could not obtain irq number\n");
> + ret = -ENODEV;
> + goto release_device_mem;
> + }
> +
> + res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> + if (res == NULL) {
> + dev_err(dev, "failed to get mem resource\n");
> + ret = -ENODEV;
> + goto release_device_mem;
> + }
> +
> + info->ide_addr = devm_ioremap(dev,
> + res->start, res->end - res->start + 1);
> + if (!info->ide_addr) {
> + dev_err(dev, "failed to map IO base address\n");
> + ret = -ENOMEM;
> + goto release_mem;
> + }
> +
> + info->clk = clk_get(&pdev->dev, "cfcon");
> + if (IS_ERR(info->clk)) {
> + dev_err(dev, "failed to get access to cf controller clock\n");
> + ret = PTR_ERR(info->clk);
> + info->clk = NULL;
> + goto unmap;
> + }
> +
> + if (clk_enable(info->clk)) {
Can it really fail?
> + dev_err(dev, "failed to enable clock source.\n");
> + goto clkerr;
> + }
> +
> + /* init ata host */
> + host = ata_host_alloc(dev, 1);
> + if (!host) {
> + dev_err(dev, "failed to allocate ide host\n");
> + ret = -ENOMEM;
> + goto stop_clk;
> + }
> +
> + ap = host->ports[0];
> + ap->flags |= ATA_FLAG_MMIO;
> + ap->pio_mask = ATA_PIO4;
> +
> + if (cpu_type == TYPE_S3C6400) {
> + ap->ops = &pata_s3c_port_ops;
> + info->sfr_addr = info->ide_addr + 0x1800;
> + info->ide_addr = info->ide_addr + 0x1900;
> + info->fifo_status_reg = 0x94;
> + } else if (cpu_type == TYPE_S5PC100) {
> + ap->ops = &pata_s5p_port_ops;
> + info->sfr_addr = info->ide_addr + 0x1800;
> + info->ide_addr = info->ide_addr + 0x1900;
Does make sense to assign those before *if*.
> + info->fifo_status_reg = 0x84;
> + } else {
> + ap->ops = &pata_s5p_port_ops;
You don't assign 'info->ide_addr' here but you'll need it in
pata_s3c_tune_chipset() which will be called thru pata_s5p_port_ops!
> + info->fifo_status_reg = 0x84;
> + }
> +
> + info->cpu_type = cpu_type;
> +
> + if (!(info->irq)) {
Parens not needed around 'info->irq'.
> + ap->flags |= ATA_FLAG_PIO_POLLING;
> + ata_port_desc(ap, "no IRQ, using PIO polling\n");
> + }
> +
> + ap->ioaddr.cmd_addr = info->ide_addr + S3C_ATA_CMD;
> + ap->ioaddr.data_addr = info->ide_addr + S3C_ATA_PIO_DTR;
> + ap->ioaddr.error_addr = info->ide_addr + S3C_ATA_PIO_FED;
> + ap->ioaddr.feature_addr = info->ide_addr + S3C_ATA_PIO_FED;
> + ap->ioaddr.nsect_addr = info->ide_addr + S3C_ATA_PIO_SCR;
> + ap->ioaddr.lbal_addr = info->ide_addr + S3C_ATA_PIO_LLR;
> + ap->ioaddr.lbam_addr = info->ide_addr + S3C_ATA_PIO_LMR;
> + ap->ioaddr.lbah_addr = info->ide_addr + S3C_ATA_PIO_LHR;
> + ap->ioaddr.device_addr = info->ide_addr + S3C_ATA_PIO_DVR;
> + ap->ioaddr.status_addr = info->ide_addr + S3C_ATA_PIO_CSD;
> + ap->ioaddr.command_addr = info->ide_addr + S3C_ATA_PIO_CSD;
> + ap->ioaddr.altstatus_addr = info->ide_addr + S3C_ATA_PIO_DAD;
> + ap->ioaddr.ctl_addr = info->ide_addr + S3C_ATA_PIO_DAD;
> +
> +
Extra newline?
> + ata_port_desc(ap, "mmio cmd 0x%llx ",
> + (unsigned long long)res->start);
> +
> + host->private_data = info;
> +
> + /* Calculates timing parameters for PIO mode */
> + pata_s3c_setup_timing(info);
You could do it right in pata_s3c_tune_chipset() -- no need to
precalculate the timings.
> + if (pdata && (pdata->setup_gpio))
Prens not needed around 'pdata->setup_gpio'.
> + pdata->setup_gpio();
> +
> + /* Set endianness and enable the interface */
> + pata_s3c_hwinit(info, pdata);
> +
> + return ata_host_activate(host, info->irq ? info->irq : 0,
This ?: doesn't make sense, just pass 'info->irq'.
> + info->irq ? pata_s3c_irq : NULL,
> + IRQF_DISABLED, &pata_s3c_sht);
> +
> + dev_set_drvdata(&pdev->dev, host);
Could use platform_set_drvdata(pdev, host)...
> +
> +stop_clk:
> + clk_disable(info->clk);
> +clkerr:
> + clk_put(info->clk);
> +unmap:
> + devm_iounmap(dev, info->ide_addr);
devm_ioremap() should "look after itself", so no explicait call
should be needed, if I don't mistake...
> +release_mem:
> + release_mem_region(res->start, res->end - res->start + 1);
But you didn't call request_mem_region()!
> +release_device_mem:
> + kfree(info);
> +return ret;
> +}
> +
> +static int __devexit pata_s3c_remove(struct platform_device *pdev)
> +{
> + struct ata_host *host = dev_get_drvdata(&pdev->dev);
Could use platform_get_drvdata(pdev)...
> + struct s3c_ide_info *info;
> + struct resource *res;
> +
> + if (!host)
> + return 0;
> + info = host->private_data;
> +
> + ata_host_detach(host);
> +
> + devm_iounmap(&pdev->dev, info->ide_addr);
> + clk_disable(info->clk);
> + clk_put(info->clk);
> + res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> + release_mem_region(res->start, res->end - res->start + 1);
Use resource_size(). Also, you don't call request_mem_region().
> + kfree(info);
> +
> + return 0;
> +}
> +
> +#ifdef CONFIG_PM
> +static int pata_s3c_suspend(struct platform_device *pdev, pm_message_t state)
> +{
> + struct ata_host *host = dev_get_drvdata(&pdev->dev);
Could use platform_get_drvdata(pdev)...
> + if (host)
> + return ata_host_suspend(host, state);
> + else
> + return 0;
> +}
> +
> +static int pata_s3c_resume(struct platform_device *pdev)
> +{
> + struct ata_host *host = dev_get_drvdata(&pdev->dev);
Could use platform_get_drvdata(pdev)...
MBR, Sergei
^ permalink raw reply [flat|nested] 48+ messages in thread* RE: [PATCH 4/4] pata_samsung: Add Samsung PATA controller driver
2010-05-27 10:17 ` Sergei Shtylyov
@ 2010-06-02 2:42 ` Kukjin Kim
-1 siblings, 0 replies; 48+ messages in thread
From: Kukjin Kim @ 2010-06-02 2:42 UTC (permalink / raw)
To: 'Sergei Shtylyov'
Cc: linux-ide, linux-arm-kernel, linux-samsung-soc, ben-linux,
'Abhilash Kesavan'
Sergei Shtylyov wrote:
>
> Hello.
>
Hi :-)
> Kukjin Kim wrote:
>
> > From: Abhilash Kesavan <a.kesavan@samsung.com>
>
> > Adds support for the Samsung PATA controller. This driver is based on the
> > Libata subsystem and references the earlier patches sent for IDE
subsystem.
>
> > Signed-off-by: Abhilash Kesavan <a.kesavan@samsung.com>
> > Signed-off-by: Kukjin Kim <kgene.kim@samsung.com>
>
> > diff --git a/drivers/ata/pata_samsung.c b/drivers/ata/pata_samsung.c
> > new file mode 100644
> > index 0000000..14381e4
> > --- /dev/null
> > +++ b/drivers/ata/pata_samsung.c
> > @@ -0,0 +1,591 @@
> [...]
> > +static void pata_s3c_tune_chipset(struct s3c_ide_info *info, u8
ide_mode)
> > +{
> > + ulong ata_cfg = readl(info->ide_addr + S3C_ATA_CFG);
> > +
> > + /* IORDY is enabled for modes > PIO2 */
> > + if (XFER_PIO_0 <= ide_mode && ide_mode <= XFER_PIO_4) {
> > +
> > + switch (ide_mode) {
> > + case XFER_PIO_0:
> > + case XFER_PIO_1:
> > + case XFER_PIO_2:
> > + ata_cfg &= (~S3C_ATA_CFG_IORDYEN);
>
> Useless parens.
>
OK
> > + break;
> > + case XFER_PIO_3:
> > + case XFER_PIO_4:
> > + ata_cfg |= S3C_ATA_CFG_IORDYEN;
>
> IORDY should be controlled by ata_id_pio_need_iordy(), not by mode
> number. PIO2 also can have IODRY enabled.
>
Will use ata_id_pio_need_iordy().
> > + break;
> > + }
> > +
> > + writel(ata_cfg, info->ide_addr + S3C_ATA_CFG);
> > + writel(piotime[ide_mode - XFER_PIO_0],
> > + info->ide_addr + S3C_ATA_PIO_TIME);
> > + } else {
> > + /* Default to PIO0 */
> > + ata_cfg &= (~S3C_ATA_CFG_IORDYEN);
> > + writel(ata_cfg, info->ide_addr + S3C_ATA_CFG);
> > + writel(piotime[0], info->ide_addr + S3C_ATA_PIO_TIME);
>
> If you don't support DMA modes or modes higher than PIO4 (PIO5 and
> PIO6 would have been good for CF though), just do nothing.
>
OK
> > + }
> > +}
> > +
> > +static void
> > +pata_s3c_set_piomode(struct ata_port *ap, struct ata_device *adev)
> > +{
> > + struct s3c_ide_info *info = ap->host->private_data;
> > +
> > + /* Configure IORDY based on PIO mode and also set the timing value */
> > + pata_s3c_tune_chipset(info, adev->pio_mode);
>
> Don't see why you need a separate function for that. Maybe in
> preparation to UDMA support?
>
That was it..but will remove pata_s3c_set_piomode() and call
pata_s3c_tune_chipset() directly.
> > +/*
> > + * Writes to one of the task file registers.
> > + */
> > +static void ata_outb(struct ata_host *host, u8 addr, void __iomem *reg)
> > +{
> > + struct s3c_ide_info *info = host->private_data;
> > +
> > + wait_for_host_ready(info);
> > + __raw_writeb(addr, reg);
>
> You should use write?() or __raw_write?() consistently...
>
OK
> > +/*
> > + * pata_s3c_tf_load - send taskfile registers to host controller
> > + */
> > +static void pata_s3c_tf_load(struct ata_port *ap,
> > + const struct ata_taskfile *tf)
> > +{
> > + struct ata_ioports *ioaddr = &ap->ioaddr;
> > + unsigned int is_addr = tf->flags & ATA_TFLAG_ISADDR;
> > +
> > + if (tf->ctl != ap->last_ctl) {
> > + if (ioaddr->ctl_addr)
>
> This register does exist and you assign ioaddr->ctl_addr in
> pata_s3c_probe(), hence the check is pointless.
>
Will remove the check.
> > + ata_outb(ap->host, tf->ctl, ioaddr->ctl_addr);
> > + ap->last_ctl = tf->ctl;
> > + ata_wait_idle(ap);
> > + }
> > +
> > + if (is_addr && (tf->flags & ATA_TFLAG_LBA48)) {
> > + WARN_ON_ONCE(!ioaddr->ctl_addr);
>
> You don't need this either.
>
OK
> > +/*
> > + * pata_s3c_tf_read - input device's ATA taskfile shadow registers
> > + */
> > +static void pata_s3c_tf_read(struct ata_port *ap, struct ata_taskfile
*tf)
> > +{
> > + struct ata_ioports *ioaddr = &ap->ioaddr;
> > +
> > + tf->command = ata_sff_check_status(ap);
>
> But you have overriden this method, so ata_sff_check_status()
> shouldn't work!
>
OK
> > + tf->feature = ata_inb(ap->host, ioaddr->error_addr);
> > + tf->nsect = ata_inb(ap->host, ioaddr->nsect_addr);
> > + tf->lbal = ata_inb(ap->host, ioaddr->lbal_addr);
> > + tf->lbam = ata_inb(ap->host, ioaddr->lbam_addr);
> > + tf->lbah = ata_inb(ap->host, ioaddr->lbah_addr);
> > + tf->device = ata_inb(ap->host, ioaddr->device_addr);
> > +
> > + if (tf->flags & ATA_TFLAG_LBA48) {
> > + if (likely(ioaddr->ctl_addr)) {
>
> Not just likely, it's always true. Emilinate the check please.
>
OK
> > + iowrite8(tf->ctl | ATA_HOB, ioaddr->ctl_addr);
> > + tf->hob_feature = ata_inb(ap->host,
ioaddr->error_addr);
> > + tf->hob_nsect = ata_inb(ap->host,
ioaddr->nsect_addr);
> > + tf->hob_lbal = ata_inb(ap->host, ioaddr->lbal_addr);
> > + tf->hob_lbam = ata_inb(ap->host, ioaddr->lbam_addr);
> > + tf->hob_lbah = ata_inb(ap->host, ioaddr->lbah_addr);
> > + iowrite8(tf->ctl, ioaddr->ctl_addr);
> > + ap->last_ctl = tf->ctl;
> > + } else
> > + WARN_ON_ONCE(1);
>
> Not needed.
>
OK
> > +/*
> > + * pata_s3c_data_xfer - Transfer data by PIO
> > + */
> > +unsigned int pata_s3c_data_xfer(struct ata_device *dev, unsigned char
*buf,
> > + unsigned int buflen, int rw)
> > +{
> > + struct ata_port *ap = dev->link->ap;
> > + struct s3c_ide_info *info = ap->host->private_data;
> > + void __iomem *data_addr = ap->ioaddr.data_addr;
> > + unsigned int words = buflen >> 1, i;
> > + u16 *temp_addr = (u16 *)buf;
> > +
> > + if (rw == READ) {
>
> {} not needed.
>
OK
> > + for (i = 0; i < words; i++, temp_addr++) {
> > + wait_for_host_ready(info);
> > + *temp_addr = __raw_readw(data_addr);
> > + wait_for_host_ready(info);
> > + *temp_addr = __raw_readw(info->ide_addr
> > + + S3C_ATA_PIO_RDATA);
> > + }
> > + } else {
>
> {} not needed.
>
OK
> > + for (i = 0; i < words; i++, temp_addr++) {
> > + wait_for_host_ready(info);
> > + writel(*temp_addr, data_addr);
> > + }
> > + }
>
> Well, if this is pere CF case, 'buflen' can't be odd, but otherwise
> you should handle that case...
>
OK
> > +
> > + return words << 1;
> > +}
> > +
> > +static struct scsi_host_template pata_s3c_sht = {
> > + ATA_PIO_SHT(DRV_NAME),
> > +};
> > +
> > +static struct ata_port_operations pata_s3c_port_ops = {
> > + .inherits = &ata_sff_port_ops,
> > + .sff_check_status = pata_s3c_check_status,
>
> Since simple iowrite8() isn't enough in your case, you also need to
> override sff_dev_select(), sff_check_altstatus(), and sff_set_devctl()
> methods...
>
Will add these overrides.
> > + .sff_tf_load = pata_s3c_tf_load,
> > + .sff_tf_read = pata_s3c_tf_read,
> > + .sff_data_xfer = pata_s3c_data_xfer,
> > + .sff_exec_command = pata_s3c_exec_command,
> > + .qc_prep = ata_noop_qc_prep,
> > + .set_piomode = pata_s3c_set_piomode,
> > +};
> > +
> > +static struct ata_port_operations pata_s5p_port_ops = {
> > + .inherits = &ata_sff_port_ops,
> > + .qc_prep = ata_noop_qc_prep,
> > + .set_piomode = pata_s3c_set_piomode,
> > +};
>
> [...]
>
> > +static void pata_s3c_setup_timing(struct s3c_ide_info *info)
> > +{
> > + uint t1, t2, teoc, i;
> > +
> > + uint pio_t1[5] = { 70, 50, 30, 30, 25 };
>
> Could use ata_timing_find_mode() here to get the mode timings,
> intead of duplicating them from libata-ocre.c...
>
> > + uint pio_t2[5] = { 290, 290, 290, 80, 70 };
>
> Note that those active times are for command cycles, not for data ones.
>
OK
> > + uint pio_teoc[5] = { 240, 43, 10, 70, 25 };
>
> What timing is this? :-O
>
Should have been t2i rec.
> > +
> > + ulong cycle_time = (uint) (1000000000 / clk_get_rate(info->clk));
> > +
> > + for (i = 0; i < 5; i++) {
> > + t1 = (pio_t1[i] / cycle_time) & 0x0f;
> > + t2 = (pio_t2[i] / cycle_time) & 0xff;
>
> Could use ata_timing_compute() here.
>
Will re-use the timing parameters and ata_timing_compute(),
ata_timing_find_mode() from libata-core.c
> > + teoc = (pio_teoc[i] / cycle_time) & 0xff;
> > + piotime[i] = (teoc << 12) | (t2 << 4) | t1;
> > + }
> > +}
> > +
> > +static void pata_s3c_hwinit(struct s3c_ide_info *info,
> > + struct s3c_ide_platdata *pdata)
> > +{
> > + /* Select true-ide as the internal operating mode*/
> > + if (pdata && (pdata->cfg_mode))
> > + pdata->cfg_mode(info->sfr_addr);
> > +
> > + switch (info->cpu_type) {
> > + case TYPE_S3C6400:
> > + /* Configure as big endian and enable the i/f*/
>
> Put space before */, please.
>
OK
> > +static int __devinit pata_s3c_probe(struct platform_device *pdev)
> > +{
> > + struct s3c_ide_platdata *pdata = pdev->dev.platform_data;
> > + struct device *dev = &pdev->dev;
> > + struct s3c_ide_info *info;
> > + struct resource *res;
> > + struct ata_port *ap;
> > + struct ata_host *host;
> > + enum s3c_cpu_type cpu_type;
> > + int ret;
> > +
> > + cpu_type = platform_get_device_id(pdev)->driver_data;
> > +
> > + info = devm_kzalloc(dev, sizeof(*info), GFP_KERNEL);
> > + if (!info) {
> > + dev_err(dev, "failed to allocate memory for device data\n");
> > + return -ENOMEM;
> > + }
> > +
> > + info->irq = platform_get_irq(pdev, 0);
> > + if (info->irq < 0) {
> > + dev_err(dev, "could not obtain irq number\n");
> > + ret = -ENODEV;
> > + goto release_device_mem;
> > + }
> > +
> > + res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> > + if (res == NULL) {
> > + dev_err(dev, "failed to get mem resource\n");
> > + ret = -ENODEV;
> > + goto release_device_mem;
> > + }
> > +
> > + info->ide_addr = devm_ioremap(dev,
> > + res->start, res->end - res->start + 1);
> > + if (!info->ide_addr) {
> > + dev_err(dev, "failed to map IO base address\n");
> > + ret = -ENOMEM;
> > + goto release_mem;
> > + }
> > +
> > + info->clk = clk_get(&pdev->dev, "cfcon");
> > + if (IS_ERR(info->clk)) {
> > + dev_err(dev, "failed to get access to cf controller
clock\n");
> > + ret = PTR_ERR(info->clk);
> > + info->clk = NULL;
> > + goto unmap;
> > + }
> > +
> > + if (clk_enable(info->clk)) {
>
> Can it really fail?
>
Will remove it.
> > + dev_err(dev, "failed to enable clock source.\n");
> > + goto clkerr;
> > + }
> > +
> > + /* init ata host */
> > + host = ata_host_alloc(dev, 1);
> > + if (!host) {
> > + dev_err(dev, "failed to allocate ide host\n");
> > + ret = -ENOMEM;
> > + goto stop_clk;
> > + }
> > +
> > + ap = host->ports[0];
> > + ap->flags |= ATA_FLAG_MMIO;
> > + ap->pio_mask = ATA_PIO4;
> > +
> > + if (cpu_type == TYPE_S3C6400) {
> > + ap->ops = &pata_s3c_port_ops;
> > + info->sfr_addr = info->ide_addr + 0x1800;
> > + info->ide_addr = info->ide_addr + 0x1900;
> > + info->fifo_status_reg = 0x94;
> > + } else if (cpu_type == TYPE_S5PC100) {
> > + ap->ops = &pata_s5p_port_ops;
> > + info->sfr_addr = info->ide_addr + 0x1800;
> > + info->ide_addr = info->ide_addr + 0x1900;
>
> Does make sense to assign those before *if*.
>
Offsets not required for S5PV210/S5PC110.
> > + info->fifo_status_reg = 0x84;
> > + } else {
> > + ap->ops = &pata_s5p_port_ops;
>
> You don't assign 'info->ide_addr' here but you'll need it in
> pata_s3c_tune_chipset() which will be called thru pata_s5p_port_ops!
>
The address received at ioremap() is enough for S5PV210/S5PC110..no offset
needed.
> > + info->fifo_status_reg = 0x84;
> > + }
> > +
> > + info->cpu_type = cpu_type;
> > +
> > + if (!(info->irq)) {
>
> Parens not needed around 'info->irq'.
>
OK
> > + ap->flags |= ATA_FLAG_PIO_POLLING;
> > + ata_port_desc(ap, "no IRQ, using PIO polling\n");
> > + }
> > +
> > + ap->ioaddr.cmd_addr = info->ide_addr + S3C_ATA_CMD;
> > + ap->ioaddr.data_addr = info->ide_addr + S3C_ATA_PIO_DTR;
> > + ap->ioaddr.error_addr = info->ide_addr + S3C_ATA_PIO_FED;
> > + ap->ioaddr.feature_addr = info->ide_addr + S3C_ATA_PIO_FED;
> > + ap->ioaddr.nsect_addr = info->ide_addr + S3C_ATA_PIO_SCR;
> > + ap->ioaddr.lbal_addr = info->ide_addr + S3C_ATA_PIO_LLR;
> > + ap->ioaddr.lbam_addr = info->ide_addr + S3C_ATA_PIO_LMR;
> > + ap->ioaddr.lbah_addr = info->ide_addr + S3C_ATA_PIO_LHR;
> > + ap->ioaddr.device_addr = info->ide_addr + S3C_ATA_PIO_DVR;
> > + ap->ioaddr.status_addr = info->ide_addr + S3C_ATA_PIO_CSD;
> > + ap->ioaddr.command_addr = info->ide_addr + S3C_ATA_PIO_CSD;
> > + ap->ioaddr.altstatus_addr = info->ide_addr + S3C_ATA_PIO_DAD;
> > + ap->ioaddr.ctl_addr = info->ide_addr + S3C_ATA_PIO_DAD;
> > +
> > +
>
> Extra newline?
>
Yes..will remove it.
> > + ata_port_desc(ap, "mmio cmd 0x%llx ",
> > + (unsigned long long)res->start);
> > +
> > + host->private_data = info;
> > +
> > + /* Calculates timing parameters for PIO mode */
> > + pata_s3c_setup_timing(info);
>
> You could do it right in pata_s3c_tune_chipset() -- no need to
> precalculate the timings.
>
Will move it.
> > + if (pdata && (pdata->setup_gpio))
>
> Prens not needed around 'pdata->setup_gpio'.
>
OK
> > + pdata->setup_gpio();
> > +
> > + /* Set endianness and enable the interface */
> > + pata_s3c_hwinit(info, pdata);
> > +
> > + return ata_host_activate(host, info->irq ? info->irq : 0,
>
> This ?: doesn't make sense, just pass 'info->irq'.
>
OK
> > + info->irq ? pata_s3c_irq : NULL,
> > + IRQF_DISABLED, &pata_s3c_sht);
> > +
> > + dev_set_drvdata(&pdev->dev, host);
>
> Could use platform_set_drvdata(pdev, host)...
>
OK
> > +
> > +stop_clk:
> > + clk_disable(info->clk);
> > +clkerr:
> > + clk_put(info->clk);
> > +unmap:
> > + devm_iounmap(dev, info->ide_addr);
>
> devm_ioremap() should "look after itself", so no explicait call
> should be needed, if I don't mistake...
>
OK
> > +release_mem:
> > + release_mem_region(res->start, res->end - res->start + 1);
>
> But you didn't call request_mem_region()!
>
I didn't..will remove.
> > +release_device_mem:
> > + kfree(info);
> > +return ret;
> > +}
> > +
> > +static int __devexit pata_s3c_remove(struct platform_device *pdev)
> > +{
> > + struct ata_host *host = dev_get_drvdata(&pdev->dev);
>
> Could use platform_get_drvdata(pdev)...
>
OK
> > + struct s3c_ide_info *info;
> > + struct resource *res;
> > +
> > + if (!host)
> > + return 0;
> > + info = host->private_data;
> > +
> > + ata_host_detach(host);
> > +
> > + devm_iounmap(&pdev->dev, info->ide_addr);
> > + clk_disable(info->clk);
> > + clk_put(info->clk);
> > + res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> > + release_mem_region(res->start, res->end - res->start + 1);
>
> Use resource_size(). Also, you don't call request_mem_region().
>
OK
> > + kfree(info);
> > +
> > + return 0;
> > +}
> > +
> > +#ifdef CONFIG_PM
> > +static int pata_s3c_suspend(struct platform_device *pdev, pm_message_t
state)
> > +{
> > + struct ata_host *host = dev_get_drvdata(&pdev->dev);
>
> Could use platform_get_drvdata(pdev)...
>
OK
> > + if (host)
> > + return ata_host_suspend(host, state);
> > + else
> > + return 0;
> > +}
> > +
> > +static int pata_s3c_resume(struct platform_device *pdev)
> > +{
> > + struct ata_host *host = dev_get_drvdata(&pdev->dev);
>
> Could use platform_get_drvdata(pdev)...
>
OK
Thanks.
Best regards,
Kgene.
--
Kukjin Kim <kgene.kim@samsung.com>, Senior Engineer,
SW Solution Development Team, Samsung Electronics Co., Ltd.
^ permalink raw reply [flat|nested] 48+ messages in thread* [PATCH 4/4] pata_samsung: Add Samsung PATA controller driver
@ 2010-06-02 2:42 ` Kukjin Kim
0 siblings, 0 replies; 48+ messages in thread
From: Kukjin Kim @ 2010-06-02 2:42 UTC (permalink / raw)
To: linux-arm-kernel
Sergei Shtylyov wrote:
>
> Hello.
>
Hi :-)
> Kukjin Kim wrote:
>
> > From: Abhilash Kesavan <a.kesavan@samsung.com>
>
> > Adds support for the Samsung PATA controller. This driver is based on the
> > Libata subsystem and references the earlier patches sent for IDE
subsystem.
>
> > Signed-off-by: Abhilash Kesavan <a.kesavan@samsung.com>
> > Signed-off-by: Kukjin Kim <kgene.kim@samsung.com>
>
> > diff --git a/drivers/ata/pata_samsung.c b/drivers/ata/pata_samsung.c
> > new file mode 100644
> > index 0000000..14381e4
> > --- /dev/null
> > +++ b/drivers/ata/pata_samsung.c
> > @@ -0,0 +1,591 @@
> [...]
> > +static void pata_s3c_tune_chipset(struct s3c_ide_info *info, u8
ide_mode)
> > +{
> > + ulong ata_cfg = readl(info->ide_addr + S3C_ATA_CFG);
> > +
> > + /* IORDY is enabled for modes > PIO2 */
> > + if (XFER_PIO_0 <= ide_mode && ide_mode <= XFER_PIO_4) {
> > +
> > + switch (ide_mode) {
> > + case XFER_PIO_0:
> > + case XFER_PIO_1:
> > + case XFER_PIO_2:
> > + ata_cfg &= (~S3C_ATA_CFG_IORDYEN);
>
> Useless parens.
>
OK
> > + break;
> > + case XFER_PIO_3:
> > + case XFER_PIO_4:
> > + ata_cfg |= S3C_ATA_CFG_IORDYEN;
>
> IORDY should be controlled by ata_id_pio_need_iordy(), not by mode
> number. PIO2 also can have IODRY enabled.
>
Will use ata_id_pio_need_iordy().
> > + break;
> > + }
> > +
> > + writel(ata_cfg, info->ide_addr + S3C_ATA_CFG);
> > + writel(piotime[ide_mode - XFER_PIO_0],
> > + info->ide_addr + S3C_ATA_PIO_TIME);
> > + } else {
> > + /* Default to PIO0 */
> > + ata_cfg &= (~S3C_ATA_CFG_IORDYEN);
> > + writel(ata_cfg, info->ide_addr + S3C_ATA_CFG);
> > + writel(piotime[0], info->ide_addr + S3C_ATA_PIO_TIME);
>
> If you don't support DMA modes or modes higher than PIO4 (PIO5 and
> PIO6 would have been good for CF though), just do nothing.
>
OK
> > + }
> > +}
> > +
> > +static void
> > +pata_s3c_set_piomode(struct ata_port *ap, struct ata_device *adev)
> > +{
> > + struct s3c_ide_info *info = ap->host->private_data;
> > +
> > + /* Configure IORDY based on PIO mode and also set the timing value */
> > + pata_s3c_tune_chipset(info, adev->pio_mode);
>
> Don't see why you need a separate function for that. Maybe in
> preparation to UDMA support?
>
That was it..but will remove pata_s3c_set_piomode() and call
pata_s3c_tune_chipset() directly.
> > +/*
> > + * Writes to one of the task file registers.
> > + */
> > +static void ata_outb(struct ata_host *host, u8 addr, void __iomem *reg)
> > +{
> > + struct s3c_ide_info *info = host->private_data;
> > +
> > + wait_for_host_ready(info);
> > + __raw_writeb(addr, reg);
>
> You should use write?() or __raw_write?() consistently...
>
OK
> > +/*
> > + * pata_s3c_tf_load - send taskfile registers to host controller
> > + */
> > +static void pata_s3c_tf_load(struct ata_port *ap,
> > + const struct ata_taskfile *tf)
> > +{
> > + struct ata_ioports *ioaddr = &ap->ioaddr;
> > + unsigned int is_addr = tf->flags & ATA_TFLAG_ISADDR;
> > +
> > + if (tf->ctl != ap->last_ctl) {
> > + if (ioaddr->ctl_addr)
>
> This register does exist and you assign ioaddr->ctl_addr in
> pata_s3c_probe(), hence the check is pointless.
>
Will remove the check.
> > + ata_outb(ap->host, tf->ctl, ioaddr->ctl_addr);
> > + ap->last_ctl = tf->ctl;
> > + ata_wait_idle(ap);
> > + }
> > +
> > + if (is_addr && (tf->flags & ATA_TFLAG_LBA48)) {
> > + WARN_ON_ONCE(!ioaddr->ctl_addr);
>
> You don't need this either.
>
OK
> > +/*
> > + * pata_s3c_tf_read - input device's ATA taskfile shadow registers
> > + */
> > +static void pata_s3c_tf_read(struct ata_port *ap, struct ata_taskfile
*tf)
> > +{
> > + struct ata_ioports *ioaddr = &ap->ioaddr;
> > +
> > + tf->command = ata_sff_check_status(ap);
>
> But you have overriden this method, so ata_sff_check_status()
> shouldn't work!
>
OK
> > + tf->feature = ata_inb(ap->host, ioaddr->error_addr);
> > + tf->nsect = ata_inb(ap->host, ioaddr->nsect_addr);
> > + tf->lbal = ata_inb(ap->host, ioaddr->lbal_addr);
> > + tf->lbam = ata_inb(ap->host, ioaddr->lbam_addr);
> > + tf->lbah = ata_inb(ap->host, ioaddr->lbah_addr);
> > + tf->device = ata_inb(ap->host, ioaddr->device_addr);
> > +
> > + if (tf->flags & ATA_TFLAG_LBA48) {
> > + if (likely(ioaddr->ctl_addr)) {
>
> Not just likely, it's always true. Emilinate the check please.
>
OK
> > + iowrite8(tf->ctl | ATA_HOB, ioaddr->ctl_addr);
> > + tf->hob_feature = ata_inb(ap->host,
ioaddr->error_addr);
> > + tf->hob_nsect = ata_inb(ap->host,
ioaddr->nsect_addr);
> > + tf->hob_lbal = ata_inb(ap->host, ioaddr->lbal_addr);
> > + tf->hob_lbam = ata_inb(ap->host, ioaddr->lbam_addr);
> > + tf->hob_lbah = ata_inb(ap->host, ioaddr->lbah_addr);
> > + iowrite8(tf->ctl, ioaddr->ctl_addr);
> > + ap->last_ctl = tf->ctl;
> > + } else
> > + WARN_ON_ONCE(1);
>
> Not needed.
>
OK
> > +/*
> > + * pata_s3c_data_xfer - Transfer data by PIO
> > + */
> > +unsigned int pata_s3c_data_xfer(struct ata_device *dev, unsigned char
*buf,
> > + unsigned int buflen, int rw)
> > +{
> > + struct ata_port *ap = dev->link->ap;
> > + struct s3c_ide_info *info = ap->host->private_data;
> > + void __iomem *data_addr = ap->ioaddr.data_addr;
> > + unsigned int words = buflen >> 1, i;
> > + u16 *temp_addr = (u16 *)buf;
> > +
> > + if (rw == READ) {
>
> {} not needed.
>
OK
> > + for (i = 0; i < words; i++, temp_addr++) {
> > + wait_for_host_ready(info);
> > + *temp_addr = __raw_readw(data_addr);
> > + wait_for_host_ready(info);
> > + *temp_addr = __raw_readw(info->ide_addr
> > + + S3C_ATA_PIO_RDATA);
> > + }
> > + } else {
>
> {} not needed.
>
OK
> > + for (i = 0; i < words; i++, temp_addr++) {
> > + wait_for_host_ready(info);
> > + writel(*temp_addr, data_addr);
> > + }
> > + }
>
> Well, if this is pere CF case, 'buflen' can't be odd, but otherwise
> you should handle that case...
>
OK
> > +
> > + return words << 1;
> > +}
> > +
> > +static struct scsi_host_template pata_s3c_sht = {
> > + ATA_PIO_SHT(DRV_NAME),
> > +};
> > +
> > +static struct ata_port_operations pata_s3c_port_ops = {
> > + .inherits = &ata_sff_port_ops,
> > + .sff_check_status = pata_s3c_check_status,
>
> Since simple iowrite8() isn't enough in your case, you also need to
> override sff_dev_select(), sff_check_altstatus(), and sff_set_devctl()
> methods...
>
Will add these overrides.
> > + .sff_tf_load = pata_s3c_tf_load,
> > + .sff_tf_read = pata_s3c_tf_read,
> > + .sff_data_xfer = pata_s3c_data_xfer,
> > + .sff_exec_command = pata_s3c_exec_command,
> > + .qc_prep = ata_noop_qc_prep,
> > + .set_piomode = pata_s3c_set_piomode,
> > +};
> > +
> > +static struct ata_port_operations pata_s5p_port_ops = {
> > + .inherits = &ata_sff_port_ops,
> > + .qc_prep = ata_noop_qc_prep,
> > + .set_piomode = pata_s3c_set_piomode,
> > +};
>
> [...]
>
> > +static void pata_s3c_setup_timing(struct s3c_ide_info *info)
> > +{
> > + uint t1, t2, teoc, i;
> > +
> > + uint pio_t1[5] = { 70, 50, 30, 30, 25 };
>
> Could use ata_timing_find_mode() here to get the mode timings,
> intead of duplicating them from libata-ocre.c...
>
> > + uint pio_t2[5] = { 290, 290, 290, 80, 70 };
>
> Note that those active times are for command cycles, not for data ones.
>
OK
> > + uint pio_teoc[5] = { 240, 43, 10, 70, 25 };
>
> What timing is this? :-O
>
Should have been t2i rec.
> > +
> > + ulong cycle_time = (uint) (1000000000 / clk_get_rate(info->clk));
> > +
> > + for (i = 0; i < 5; i++) {
> > + t1 = (pio_t1[i] / cycle_time) & 0x0f;
> > + t2 = (pio_t2[i] / cycle_time) & 0xff;
>
> Could use ata_timing_compute() here.
>
Will re-use the timing parameters and ata_timing_compute(),
ata_timing_find_mode() from libata-core.c
> > + teoc = (pio_teoc[i] / cycle_time) & 0xff;
> > + piotime[i] = (teoc << 12) | (t2 << 4) | t1;
> > + }
> > +}
> > +
> > +static void pata_s3c_hwinit(struct s3c_ide_info *info,
> > + struct s3c_ide_platdata *pdata)
> > +{
> > + /* Select true-ide as the internal operating mode*/
> > + if (pdata && (pdata->cfg_mode))
> > + pdata->cfg_mode(info->sfr_addr);
> > +
> > + switch (info->cpu_type) {
> > + case TYPE_S3C6400:
> > + /* Configure as big endian and enable the i/f*/
>
> Put space before */, please.
>
OK
> > +static int __devinit pata_s3c_probe(struct platform_device *pdev)
> > +{
> > + struct s3c_ide_platdata *pdata = pdev->dev.platform_data;
> > + struct device *dev = &pdev->dev;
> > + struct s3c_ide_info *info;
> > + struct resource *res;
> > + struct ata_port *ap;
> > + struct ata_host *host;
> > + enum s3c_cpu_type cpu_type;
> > + int ret;
> > +
> > + cpu_type = platform_get_device_id(pdev)->driver_data;
> > +
> > + info = devm_kzalloc(dev, sizeof(*info), GFP_KERNEL);
> > + if (!info) {
> > + dev_err(dev, "failed to allocate memory for device data\n");
> > + return -ENOMEM;
> > + }
> > +
> > + info->irq = platform_get_irq(pdev, 0);
> > + if (info->irq < 0) {
> > + dev_err(dev, "could not obtain irq number\n");
> > + ret = -ENODEV;
> > + goto release_device_mem;
> > + }
> > +
> > + res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> > + if (res == NULL) {
> > + dev_err(dev, "failed to get mem resource\n");
> > + ret = -ENODEV;
> > + goto release_device_mem;
> > + }
> > +
> > + info->ide_addr = devm_ioremap(dev,
> > + res->start, res->end - res->start + 1);
> > + if (!info->ide_addr) {
> > + dev_err(dev, "failed to map IO base address\n");
> > + ret = -ENOMEM;
> > + goto release_mem;
> > + }
> > +
> > + info->clk = clk_get(&pdev->dev, "cfcon");
> > + if (IS_ERR(info->clk)) {
> > + dev_err(dev, "failed to get access to cf controller
clock\n");
> > + ret = PTR_ERR(info->clk);
> > + info->clk = NULL;
> > + goto unmap;
> > + }
> > +
> > + if (clk_enable(info->clk)) {
>
> Can it really fail?
>
Will remove it.
> > + dev_err(dev, "failed to enable clock source.\n");
> > + goto clkerr;
> > + }
> > +
> > + /* init ata host */
> > + host = ata_host_alloc(dev, 1);
> > + if (!host) {
> > + dev_err(dev, "failed to allocate ide host\n");
> > + ret = -ENOMEM;
> > + goto stop_clk;
> > + }
> > +
> > + ap = host->ports[0];
> > + ap->flags |= ATA_FLAG_MMIO;
> > + ap->pio_mask = ATA_PIO4;
> > +
> > + if (cpu_type == TYPE_S3C6400) {
> > + ap->ops = &pata_s3c_port_ops;
> > + info->sfr_addr = info->ide_addr + 0x1800;
> > + info->ide_addr = info->ide_addr + 0x1900;
> > + info->fifo_status_reg = 0x94;
> > + } else if (cpu_type == TYPE_S5PC100) {
> > + ap->ops = &pata_s5p_port_ops;
> > + info->sfr_addr = info->ide_addr + 0x1800;
> > + info->ide_addr = info->ide_addr + 0x1900;
>
> Does make sense to assign those before *if*.
>
Offsets not required for S5PV210/S5PC110.
> > + info->fifo_status_reg = 0x84;
> > + } else {
> > + ap->ops = &pata_s5p_port_ops;
>
> You don't assign 'info->ide_addr' here but you'll need it in
> pata_s3c_tune_chipset() which will be called thru pata_s5p_port_ops!
>
The address received at ioremap() is enough for S5PV210/S5PC110..no offset
needed.
> > + info->fifo_status_reg = 0x84;
> > + }
> > +
> > + info->cpu_type = cpu_type;
> > +
> > + if (!(info->irq)) {
>
> Parens not needed around 'info->irq'.
>
OK
> > + ap->flags |= ATA_FLAG_PIO_POLLING;
> > + ata_port_desc(ap, "no IRQ, using PIO polling\n");
> > + }
> > +
> > + ap->ioaddr.cmd_addr = info->ide_addr + S3C_ATA_CMD;
> > + ap->ioaddr.data_addr = info->ide_addr + S3C_ATA_PIO_DTR;
> > + ap->ioaddr.error_addr = info->ide_addr + S3C_ATA_PIO_FED;
> > + ap->ioaddr.feature_addr = info->ide_addr + S3C_ATA_PIO_FED;
> > + ap->ioaddr.nsect_addr = info->ide_addr + S3C_ATA_PIO_SCR;
> > + ap->ioaddr.lbal_addr = info->ide_addr + S3C_ATA_PIO_LLR;
> > + ap->ioaddr.lbam_addr = info->ide_addr + S3C_ATA_PIO_LMR;
> > + ap->ioaddr.lbah_addr = info->ide_addr + S3C_ATA_PIO_LHR;
> > + ap->ioaddr.device_addr = info->ide_addr + S3C_ATA_PIO_DVR;
> > + ap->ioaddr.status_addr = info->ide_addr + S3C_ATA_PIO_CSD;
> > + ap->ioaddr.command_addr = info->ide_addr + S3C_ATA_PIO_CSD;
> > + ap->ioaddr.altstatus_addr = info->ide_addr + S3C_ATA_PIO_DAD;
> > + ap->ioaddr.ctl_addr = info->ide_addr + S3C_ATA_PIO_DAD;
> > +
> > +
>
> Extra newline?
>
Yes..will remove it.
> > + ata_port_desc(ap, "mmio cmd 0x%llx ",
> > + (unsigned long long)res->start);
> > +
> > + host->private_data = info;
> > +
> > + /* Calculates timing parameters for PIO mode */
> > + pata_s3c_setup_timing(info);
>
> You could do it right in pata_s3c_tune_chipset() -- no need to
> precalculate the timings.
>
Will move it.
> > + if (pdata && (pdata->setup_gpio))
>
> Prens not needed around 'pdata->setup_gpio'.
>
OK
> > + pdata->setup_gpio();
> > +
> > + /* Set endianness and enable the interface */
> > + pata_s3c_hwinit(info, pdata);
> > +
> > + return ata_host_activate(host, info->irq ? info->irq : 0,
>
> This ?: doesn't make sense, just pass 'info->irq'.
>
OK
> > + info->irq ? pata_s3c_irq : NULL,
> > + IRQF_DISABLED, &pata_s3c_sht);
> > +
> > + dev_set_drvdata(&pdev->dev, host);
>
> Could use platform_set_drvdata(pdev, host)...
>
OK
> > +
> > +stop_clk:
> > + clk_disable(info->clk);
> > +clkerr:
> > + clk_put(info->clk);
> > +unmap:
> > + devm_iounmap(dev, info->ide_addr);
>
> devm_ioremap() should "look after itself", so no explicait call
> should be needed, if I don't mistake...
>
OK
> > +release_mem:
> > + release_mem_region(res->start, res->end - res->start + 1);
>
> But you didn't call request_mem_region()!
>
I didn't..will remove.
> > +release_device_mem:
> > + kfree(info);
> > +return ret;
> > +}
> > +
> > +static int __devexit pata_s3c_remove(struct platform_device *pdev)
> > +{
> > + struct ata_host *host = dev_get_drvdata(&pdev->dev);
>
> Could use platform_get_drvdata(pdev)...
>
OK
> > + struct s3c_ide_info *info;
> > + struct resource *res;
> > +
> > + if (!host)
> > + return 0;
> > + info = host->private_data;
> > +
> > + ata_host_detach(host);
> > +
> > + devm_iounmap(&pdev->dev, info->ide_addr);
> > + clk_disable(info->clk);
> > + clk_put(info->clk);
> > + res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> > + release_mem_region(res->start, res->end - res->start + 1);
>
> Use resource_size(). Also, you don't call request_mem_region().
>
OK
> > + kfree(info);
> > +
> > + return 0;
> > +}
> > +
> > +#ifdef CONFIG_PM
> > +static int pata_s3c_suspend(struct platform_device *pdev, pm_message_t
state)
> > +{
> > + struct ata_host *host = dev_get_drvdata(&pdev->dev);
>
> Could use platform_get_drvdata(pdev)...
>
OK
> > + if (host)
> > + return ata_host_suspend(host, state);
> > + else
> > + return 0;
> > +}
> > +
> > +static int pata_s3c_resume(struct platform_device *pdev)
> > +{
> > + struct ata_host *host = dev_get_drvdata(&pdev->dev);
>
> Could use platform_get_drvdata(pdev)...
>
OK
Thanks.
Best regards,
Kgene.
--
Kukjin Kim <kgene.kim@samsung.com>, Senior Engineer,
SW Solution Development Team, Samsung Electronics Co., Ltd.
^ permalink raw reply [flat|nested] 48+ messages in thread* Re: [PATCH 4/4] pata_samsung: Add Samsung PATA controller driver
2010-06-02 2:42 ` Kukjin Kim
@ 2010-06-02 9:51 ` Sergei Shtylyov
-1 siblings, 0 replies; 48+ messages in thread
From: Sergei Shtylyov @ 2010-06-02 9:51 UTC (permalink / raw)
To: Kukjin Kim
Cc: 'Sergei Shtylyov', linux-ide, linux-arm-kernel,
linux-samsung-soc, ben-linux, 'Abhilash Kesavan'
Hello.
Kukjin Kim wrote:
>>> From: Abhilash Kesavan <a.kesavan@samsung.com>
>>> Adds support for the Samsung PATA controller. This driver is based on the
>>> Libata subsystem and references the earlier patches sent for IDE subsystem.
Where I those I wonder? :-)
It's a pity they didn't get accepted.
>>> Signed-off-by: Abhilash Kesavan <a.kesavan@samsung.com>
>>> Signed-off-by: Kukjin Kim <kgene.kim@samsung.com>
[...]
>>> + for (i = 0; i < words; i++, temp_addr++) {
>>> + wait_for_host_ready(info);
>>> + writel(*temp_addr, data_addr);
>>> + }
>>> + }
>> Well, if this is pere CF case, 'buflen' can't be odd, but otherwise
I meant to type "pure". :-)
>> you should handle that case...
>>> + uint pio_teoc[5] = { 240, 43, 10, 70, 25 };
>> What timing is this? :-O
> Should have been t2i rec.
If it's t2i timing, it is incorrect for the first 3 PIO modes.
>>> + if (cpu_type == TYPE_S3C6400) {
>>> + ap->ops = &pata_s3c_port_ops;
>>> + info->sfr_addr = info->ide_addr + 0x1800;
>>> + info->ide_addr = info->ide_addr + 0x1900;
>>> + info->fifo_status_reg = 0x94;
>>> + } else if (cpu_type == TYPE_S5PC100) {
>>> + ap->ops = &pata_s5p_port_ops;
>>> + info->sfr_addr = info->ide_addr + 0x1800;
>>> + info->ide_addr = info->ide_addr + 0x1900;
>> Does make sense to assign those before *if*.
> Offsets not required for S5PV210/S5PC110.
pata_s3c_tune_chipset() certainly requires them.
>>> + info->fifo_status_reg = 0x84;
>>> + } else {
>>> + ap->ops = &pata_s5p_port_ops;
>> You don't assign 'info->ide_addr' here but you'll need it in
>> pata_s3c_tune_chipset() which will be called thru pata_s5p_port_ops!
> The address received at ioremap() is enough for S5PV210/S5PC110..no offset
> needed.
I only have to restate that pata_s3c_tune_chipset() does use
'info->ide_addr'. Maybe you shouldn't install this method for S5PV210/S5PC110?
Or do you mean thgat offset of 0 will work?
>>> +release_mem:
>>> + release_mem_region(res->start, res->end - res->start + 1);
>> But you didn't call request_mem_region()!
> I didn't..will remove.
But you should call request_mem_region() in one or another form...
MBR, Sergei
^ permalink raw reply [flat|nested] 48+ messages in thread* [PATCH 4/4] pata_samsung: Add Samsung PATA controller driver
@ 2010-06-02 9:51 ` Sergei Shtylyov
0 siblings, 0 replies; 48+ messages in thread
From: Sergei Shtylyov @ 2010-06-02 9:51 UTC (permalink / raw)
To: linux-arm-kernel
Hello.
Kukjin Kim wrote:
>>> From: Abhilash Kesavan <a.kesavan@samsung.com>
>>> Adds support for the Samsung PATA controller. This driver is based on the
>>> Libata subsystem and references the earlier patches sent for IDE subsystem.
Where I those I wonder? :-)
It's a pity they didn't get accepted.
>>> Signed-off-by: Abhilash Kesavan <a.kesavan@samsung.com>
>>> Signed-off-by: Kukjin Kim <kgene.kim@samsung.com>
[...]
>>> + for (i = 0; i < words; i++, temp_addr++) {
>>> + wait_for_host_ready(info);
>>> + writel(*temp_addr, data_addr);
>>> + }
>>> + }
>> Well, if this is pere CF case, 'buflen' can't be odd, but otherwise
I meant to type "pure". :-)
>> you should handle that case...
>>> + uint pio_teoc[5] = { 240, 43, 10, 70, 25 };
>> What timing is this? :-O
> Should have been t2i rec.
If it's t2i timing, it is incorrect for the first 3 PIO modes.
>>> + if (cpu_type == TYPE_S3C6400) {
>>> + ap->ops = &pata_s3c_port_ops;
>>> + info->sfr_addr = info->ide_addr + 0x1800;
>>> + info->ide_addr = info->ide_addr + 0x1900;
>>> + info->fifo_status_reg = 0x94;
>>> + } else if (cpu_type == TYPE_S5PC100) {
>>> + ap->ops = &pata_s5p_port_ops;
>>> + info->sfr_addr = info->ide_addr + 0x1800;
>>> + info->ide_addr = info->ide_addr + 0x1900;
>> Does make sense to assign those before *if*.
> Offsets not required for S5PV210/S5PC110.
pata_s3c_tune_chipset() certainly requires them.
>>> + info->fifo_status_reg = 0x84;
>>> + } else {
>>> + ap->ops = &pata_s5p_port_ops;
>> You don't assign 'info->ide_addr' here but you'll need it in
>> pata_s3c_tune_chipset() which will be called thru pata_s5p_port_ops!
> The address received at ioremap() is enough for S5PV210/S5PC110..no offset
> needed.
I only have to restate that pata_s3c_tune_chipset() does use
'info->ide_addr'. Maybe you shouldn't install this method for S5PV210/S5PC110?
Or do you mean thgat offset of 0 will work?
>>> +release_mem:
>>> + release_mem_region(res->start, res->end - res->start + 1);
>> But you didn't call request_mem_region()!
> I didn't..will remove.
But you should call request_mem_region() in one or another form...
MBR, Sergei
^ permalink raw reply [flat|nested] 48+ messages in thread* RE: [PATCH 4/4] pata_samsung: Add Samsung PATA controller driver
2010-06-02 9:51 ` Sergei Shtylyov
@ 2010-06-03 0:25 ` Kukjin Kim
-1 siblings, 0 replies; 48+ messages in thread
From: Kukjin Kim @ 2010-06-03 0:25 UTC (permalink / raw)
To: 'Sergei Shtylyov'
Cc: linux-ide, linux-arm-kernel, linux-samsung-soc, ben-linux,
'Abhilash Kesavan'
Sergei Shtylyov wrote:
>
> Hello.
Hi ;-)
> Kukjin Kim wrote:
>
> >>> From: Abhilash Kesavan <a.kesavan@samsung.com>
> >>> Adds support for the Samsung PATA controller. This driver is based on
the
> >>> Libata subsystem and references the earlier patches sent for IDE
subsystem.
>
> Where I those I wonder? :-)
> It's a pity they didn't get accepted.
>
Was told that that only new libata based drivers would be accepted.
> >>> Signed-off-by: Abhilash Kesavan <a.kesavan@samsung.com>
> >>> Signed-off-by: Kukjin Kim <kgene.kim@samsung.com>
>
> [...]
>
> >>> + for (i = 0; i < words; i++, temp_addr++) {
> >>> + wait_for_host_ready(info);
> >>> + writel(*temp_addr, data_addr);
> >>> + }
> >>> + }
>
> >> Well, if this is pere CF case, 'buflen' can't be odd, but otherwise
>
> I meant to type "pure". :-)
>
considering CF in true-ide mode..should I still add the check?
> >> you should handle that case...
>
> >>> + uint pio_teoc[5] = { 240, 43, 10, 70, 25 };
>
> >> What timing is this? :-O
>
> > Should have been t2i rec.
>
> If it's t2i timing, it is incorrect for the first 3 PIO modes.
>
Should have been (240, 93, 40, 70, 25)
> >>> + if (cpu_type == TYPE_S3C6400) {
> >>> + ap->ops = &pata_s3c_port_ops;
> >>> + info->sfr_addr = info->ide_addr + 0x1800;
> >>> + info->ide_addr = info->ide_addr + 0x1900;
> >>> + info->fifo_status_reg = 0x94;
> >>> + } else if (cpu_type == TYPE_S5PC100) {
> >>> + ap->ops = &pata_s5p_port_ops;
> >>> + info->sfr_addr = info->ide_addr + 0x1800;
> >>> + info->ide_addr = info->ide_addr + 0x1900;
>
> >> Does make sense to assign those before *if*.
>
> > Offsets not required for S5PV210/S5PC110.
>
> pata_s3c_tune_chipset() certainly requires them.
>
yes..but for V210/C110 just the ioremapped info->ide_addr with 0 offset is
enough.
> >>> + info->fifo_status_reg = 0x84;
> >>> + } else {
> >>> + ap->ops = &pata_s5p_port_ops;
>
> >> You don't assign 'info->ide_addr' here but you'll need it in
> >> pata_s3c_tune_chipset() which will be called thru pata_s5p_port_ops!
>
> > The address received at ioremap() is enough for S5PV210/S5PC110..no
offset
> > needed.
>
> I only have to restate that pata_s3c_tune_chipset() does use
> 'info->ide_addr'. Maybe you shouldn't install this method for
S5PV210/S5PC110?
> Or do you mean thgat offset of 0 will work?
>
V210/C110 requires 0 offset as compared to 6410 and C100 which require
0x1800.
> >>> +release_mem:
> >>> + release_mem_region(res->start, res->end - res->start + 1);
>
> >> But you didn't call request_mem_region()!
>
> > I didn't..will remove.
>
> But you should call request_mem_region() in one or another form...
>
OK..will request mem_region() before ioremap()..and then the release_mem's
should be fine.
Thanks.
Best regards,
Kgene.
--
Kukjin Kim <kgene.kim@samsung.com>, Senior Engineer,
SW Solution Development Team, Samsung Electronics Co., Ltd.
^ permalink raw reply [flat|nested] 48+ messages in thread* [PATCH 4/4] pata_samsung: Add Samsung PATA controller driver
@ 2010-06-03 0:25 ` Kukjin Kim
0 siblings, 0 replies; 48+ messages in thread
From: Kukjin Kim @ 2010-06-03 0:25 UTC (permalink / raw)
To: linux-arm-kernel
Sergei Shtylyov wrote:
>
> Hello.
Hi ;-)
> Kukjin Kim wrote:
>
> >>> From: Abhilash Kesavan <a.kesavan@samsung.com>
> >>> Adds support for the Samsung PATA controller. This driver is based on
the
> >>> Libata subsystem and references the earlier patches sent for IDE
subsystem.
>
> Where I those I wonder? :-)
> It's a pity they didn't get accepted.
>
Was told that that only new libata based drivers would be accepted.
> >>> Signed-off-by: Abhilash Kesavan <a.kesavan@samsung.com>
> >>> Signed-off-by: Kukjin Kim <kgene.kim@samsung.com>
>
> [...]
>
> >>> + for (i = 0; i < words; i++, temp_addr++) {
> >>> + wait_for_host_ready(info);
> >>> + writel(*temp_addr, data_addr);
> >>> + }
> >>> + }
>
> >> Well, if this is pere CF case, 'buflen' can't be odd, but otherwise
>
> I meant to type "pure". :-)
>
considering CF in true-ide mode..should I still add the check?
> >> you should handle that case...
>
> >>> + uint pio_teoc[5] = { 240, 43, 10, 70, 25 };
>
> >> What timing is this? :-O
>
> > Should have been t2i rec.
>
> If it's t2i timing, it is incorrect for the first 3 PIO modes.
>
Should have been (240, 93, 40, 70, 25)
> >>> + if (cpu_type == TYPE_S3C6400) {
> >>> + ap->ops = &pata_s3c_port_ops;
> >>> + info->sfr_addr = info->ide_addr + 0x1800;
> >>> + info->ide_addr = info->ide_addr + 0x1900;
> >>> + info->fifo_status_reg = 0x94;
> >>> + } else if (cpu_type == TYPE_S5PC100) {
> >>> + ap->ops = &pata_s5p_port_ops;
> >>> + info->sfr_addr = info->ide_addr + 0x1800;
> >>> + info->ide_addr = info->ide_addr + 0x1900;
>
> >> Does make sense to assign those before *if*.
>
> > Offsets not required for S5PV210/S5PC110.
>
> pata_s3c_tune_chipset() certainly requires them.
>
yes..but for V210/C110 just the ioremapped info->ide_addr with 0 offset is
enough.
> >>> + info->fifo_status_reg = 0x84;
> >>> + } else {
> >>> + ap->ops = &pata_s5p_port_ops;
>
> >> You don't assign 'info->ide_addr' here but you'll need it in
> >> pata_s3c_tune_chipset() which will be called thru pata_s5p_port_ops!
>
> > The address received at ioremap() is enough for S5PV210/S5PC110..no
offset
> > needed.
>
> I only have to restate that pata_s3c_tune_chipset() does use
> 'info->ide_addr'. Maybe you shouldn't install this method for
S5PV210/S5PC110?
> Or do you mean thgat offset of 0 will work?
>
V210/C110 requires 0 offset as compared to 6410 and C100 which require
0x1800.
> >>> +release_mem:
> >>> + release_mem_region(res->start, res->end - res->start + 1);
>
> >> But you didn't call request_mem_region()!
>
> > I didn't..will remove.
>
> But you should call request_mem_region() in one or another form...
>
OK..will request mem_region() before ioremap()..and then the release_mem's
should be fine.
Thanks.
Best regards,
Kgene.
--
Kukjin Kim <kgene.kim@samsung.com>, Senior Engineer,
SW Solution Development Team, Samsung Electronics Co., Ltd.
^ permalink raw reply [flat|nested] 48+ messages in thread
* Re: [PATCH 4/4] pata_samsung: Add Samsung PATA controller driver
2010-05-27 8:22 ` Kukjin Kim
@ 2010-05-27 21:52 ` Tejun Heo
-1 siblings, 0 replies; 48+ messages in thread
From: Tejun Heo @ 2010-05-27 21:52 UTC (permalink / raw)
To: Kukjin Kim
Cc: linux-ide, linux-arm-kernel, linux-samsung-soc, ben-linux,
Abhilash Kesavan
Hello,
On 05/27/2010 10:22 AM, Kukjin Kim wrote:
> From: Abhilash Kesavan <a.kesavan@samsung.com>
>
> Adds support for the Samsung PATA controller. This driver is based on the
> Libata subsystem and references the earlier patches sent for IDE subsystem.
Just one small thing.
> +static struct ata_port_operations pata_s3c_port_ops = {
> + .inherits = &ata_sff_port_ops,
> + .sff_check_status = pata_s3c_check_status,
> + .sff_tf_load = pata_s3c_tf_load,
> + .sff_tf_read = pata_s3c_tf_read,
> + .sff_data_xfer = pata_s3c_data_xfer,
> + .sff_exec_command = pata_s3c_exec_command,
> + .qc_prep = ata_noop_qc_prep,
> + .set_piomode = pata_s3c_set_piomode,
> +};
> +
> +static struct ata_port_operations pata_s5p_port_ops = {
> + .inherits = &ata_sff_port_ops,
> + .qc_prep = ata_noop_qc_prep,
> + .set_piomode = pata_s3c_set_piomode,
> +};
You don't need to override .qc_prep to ata_noop_qc_prep() and can you
please base the patch against the current libata-dev#upstream?
Thanks.
--
tejun
^ permalink raw reply [flat|nested] 48+ messages in thread* [PATCH 4/4] pata_samsung: Add Samsung PATA controller driver
@ 2010-05-27 21:52 ` Tejun Heo
0 siblings, 0 replies; 48+ messages in thread
From: Tejun Heo @ 2010-05-27 21:52 UTC (permalink / raw)
To: linux-arm-kernel
Hello,
On 05/27/2010 10:22 AM, Kukjin Kim wrote:
> From: Abhilash Kesavan <a.kesavan@samsung.com>
>
> Adds support for the Samsung PATA controller. This driver is based on the
> Libata subsystem and references the earlier patches sent for IDE subsystem.
Just one small thing.
> +static struct ata_port_operations pata_s3c_port_ops = {
> + .inherits = &ata_sff_port_ops,
> + .sff_check_status = pata_s3c_check_status,
> + .sff_tf_load = pata_s3c_tf_load,
> + .sff_tf_read = pata_s3c_tf_read,
> + .sff_data_xfer = pata_s3c_data_xfer,
> + .sff_exec_command = pata_s3c_exec_command,
> + .qc_prep = ata_noop_qc_prep,
> + .set_piomode = pata_s3c_set_piomode,
> +};
> +
> +static struct ata_port_operations pata_s5p_port_ops = {
> + .inherits = &ata_sff_port_ops,
> + .qc_prep = ata_noop_qc_prep,
> + .set_piomode = pata_s3c_set_piomode,
> +};
You don't need to override .qc_prep to ata_noop_qc_prep() and can you
please base the patch against the current libata-dev#upstream?
Thanks.
--
tejun
^ permalink raw reply [flat|nested] 48+ messages in thread* RE: [PATCH 4/4] pata_samsung: Add Samsung PATA controller driver
2010-05-27 21:52 ` Tejun Heo
@ 2010-06-02 2:46 ` Kukjin Kim
-1 siblings, 0 replies; 48+ messages in thread
From: Kukjin Kim @ 2010-06-02 2:46 UTC (permalink / raw)
To: 'Tejun Heo'
Cc: linux-ide, linux-arm-kernel, linux-samsung-soc, ben-linux,
'Abhilash Kesavan'
Tejun Heo wrote:
>
> Hello,
>
> On 05/27/2010 10:22 AM, Kukjin Kim wrote:
> > From: Abhilash Kesavan <a.kesavan@samsung.com>
> >
> > Adds support for the Samsung PATA controller. This driver is based on the
> > Libata subsystem and references the earlier patches sent for IDE
subsystem.
>
> Just one small thing.
>
> > +static struct ata_port_operations pata_s3c_port_ops = {
> > + .inherits = &ata_sff_port_ops,
> > + .sff_check_status = pata_s3c_check_status,
> > + .sff_tf_load = pata_s3c_tf_load,
> > + .sff_tf_read = pata_s3c_tf_read,
> > + .sff_data_xfer = pata_s3c_data_xfer,
> > + .sff_exec_command = pata_s3c_exec_command,
> > + .qc_prep = ata_noop_qc_prep,
> > + .set_piomode = pata_s3c_set_piomode,
> > +};
> > +
> > +static struct ata_port_operations pata_s5p_port_ops = {
> > + .inherits = &ata_sff_port_ops,
> > + .qc_prep = ata_noop_qc_prep,
> > + .set_piomode = pata_s3c_set_piomode,
> > +};
>
> You don't need to override .qc_prep to ata_noop_qc_prep() and can you
> please base the patch against the current libata-dev#upstream?
>
Will remove the override and rebase the new patches against upstream branch.
Thanks.
Best regards,
Kgene.
--
Kukjin Kim <kgene.kim@samsung.com>, Senior Engineer,
SW Solution Development Team, Samsung Electronics Co., Ltd.
^ permalink raw reply [flat|nested] 48+ messages in thread* [PATCH 4/4] pata_samsung: Add Samsung PATA controller driver
@ 2010-06-02 2:46 ` Kukjin Kim
0 siblings, 0 replies; 48+ messages in thread
From: Kukjin Kim @ 2010-06-02 2:46 UTC (permalink / raw)
To: linux-arm-kernel
Tejun Heo wrote:
>
> Hello,
>
> On 05/27/2010 10:22 AM, Kukjin Kim wrote:
> > From: Abhilash Kesavan <a.kesavan@samsung.com>
> >
> > Adds support for the Samsung PATA controller. This driver is based on the
> > Libata subsystem and references the earlier patches sent for IDE
subsystem.
>
> Just one small thing.
>
> > +static struct ata_port_operations pata_s3c_port_ops = {
> > + .inherits = &ata_sff_port_ops,
> > + .sff_check_status = pata_s3c_check_status,
> > + .sff_tf_load = pata_s3c_tf_load,
> > + .sff_tf_read = pata_s3c_tf_read,
> > + .sff_data_xfer = pata_s3c_data_xfer,
> > + .sff_exec_command = pata_s3c_exec_command,
> > + .qc_prep = ata_noop_qc_prep,
> > + .set_piomode = pata_s3c_set_piomode,
> > +};
> > +
> > +static struct ata_port_operations pata_s5p_port_ops = {
> > + .inherits = &ata_sff_port_ops,
> > + .qc_prep = ata_noop_qc_prep,
> > + .set_piomode = pata_s3c_set_piomode,
> > +};
>
> You don't need to override .qc_prep to ata_noop_qc_prep() and can you
> please base the patch against the current libata-dev#upstream?
>
Will remove the override and rebase the new patches against upstream branch.
Thanks.
Best regards,
Kgene.
--
Kukjin Kim <kgene.kim@samsung.com>, Senior Engineer,
SW Solution Development Team, Samsung Electronics Co., Ltd.
^ permalink raw reply [flat|nested] 48+ messages in thread