| Message ID | 1276156241-28804-1-git-send-email-kgene.kim@samsung.com |
|---|---|
| State | Not Applicable |
| Delegated to: | David Miller |
| Headers | show |
On 06/10/2010 03:50 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. > > Signed-off-by: Abhilash Kesavan<a.kesavan@samsung.com> > Signed-off-by: Kukjin Kim<kgene.kim@samsung.com> > --- > > Changes since v2: > - Changed the DRV_NAME to pata_samsung_cf > - Used msleep instead of mdelay > > drivers/ata/Makefile | 1 + > drivers/ata/pata_samsung_cf.c | 608 +++++++++++++++++++++++++++++++++++++++++ > 3 files changed, 618 insertions(+), 0 deletions(-) > create mode 100644 drivers/ata/pata_samsung_cf.c Acked-by: Jeff Garzik <jgarzik@redhat.com> I presume you'll send this upstream via the appropriate ARM tree. -- To unsubscribe from this list: send the line "unsubscribe linux-ide" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, Jun 10, 2010 at 04:52:07AM -0400, Jeff Garzik wrote: > On 06/10/2010 03:50 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. >> >> Signed-off-by: Abhilash Kesavan<a.kesavan@samsung.com> >> Signed-off-by: Kukjin Kim<kgene.kim@samsung.com> >> --- >> >> Changes since v2: >> - Changed the DRV_NAME to pata_samsung_cf >> - Used msleep instead of mdelay >> >> drivers/ata/Makefile | 1 + >> drivers/ata/pata_samsung_cf.c | 608 +++++++++++++++++++++++++++++++++++++++++ >> 3 files changed, 618 insertions(+), 0 deletions(-) >> create mode 100644 drivers/ata/pata_samsung_cf.c > > Acked-by: Jeff Garzik <jgarzik@redhat.com> > > > I presume you'll send this upstream via the appropriate ARM tree. It depends on whether Linus un-declares his current suspensio nof ARM based work. The machine updtaes should go via the samsung tree but whether to send the driver that was is another matter (I prefer to have drivers go via the driver's tree to avoid clashes in Makefile and Kconfig). PS, what's your policy on -rc inclusion of completely new code such as this?
On 06/10/2010 04:56 AM, Ben Dooks wrote: > On Thu, Jun 10, 2010 at 04:52:07AM -0400, Jeff Garzik wrote: >> On 06/10/2010 03:50 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. >>> >>> Signed-off-by: Abhilash Kesavan<a.kesavan@samsung.com> >>> Signed-off-by: Kukjin Kim<kgene.kim@samsung.com> >>> --- >>> >>> Changes since v2: >>> - Changed the DRV_NAME to pata_samsung_cf >>> - Used msleep instead of mdelay >>> >>> drivers/ata/Makefile | 1 + >>> drivers/ata/pata_samsung_cf.c | 608 +++++++++++++++++++++++++++++++++++++++++ >>> 3 files changed, 618 insertions(+), 0 deletions(-) >>> create mode 100644 drivers/ata/pata_samsung_cf.c >> >> Acked-by: Jeff Garzik<jgarzik@redhat.com> >> >> >> I presume you'll send this upstream via the appropriate ARM tree. > > It depends on whether Linus un-declares his current suspensio nof > ARM based work. The machine updtaes should go via the samsung tree > but whether to send the driver that was is another matter (I prefer > to have drivers go via the driver's tree to avoid clashes in > Makefile and Kconfig). I don't mind merging it, but usually platform drivers do not build and/or work without additional platform glue. Isn't that the case here? > PS, what's your policy on -rc inclusion of completely new code > such as this? New drivers are fine (which is standard policy outside libata, too), as they will not cause regressions, and will enable new users. Jeff -- To unsubscribe from this list: send the line "unsubscribe linux-ide" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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_cf.c b/drivers/ata/pata_samsung_cf.c > new file mode 100644 > index 0000000..fef5515 > --- /dev/null > +++ b/drivers/ata/pata_samsung_cf.c > @@ -0,0 +1,608 @@ > +/* linux/drivers/ata/pata_samsung_cf.c File names in the heading comment are discouraged. [...] > +static unsigned long > +pata_s3c_setup_timing(struct s3c_ide_info *info, struct ata_device *adev) > +{ > + const struct ata_timing *timing; > + int cycle_time; > + int t1; > + int t2; > + int t2i; > + ulong piotime; > + > + cycle_time = (int)(1000000000UL / clk_get_rate(info->clk)); > + > + timing = ata_timing_find_mode(adev->pio_mode); > + t1 = (timing->setup / cycle_time) & 0xf; > + t2 = (timing->act8b / cycle_time) & 0xff; > + t2i = (timing->rec8b / cycle_time) & 0xff; Why are you still not using ata_timing_compute()? > + > + piotime = (t2i << 12) | (t2 << 4) | t1; > + > + return piotime; > +} > + > +static void pata_s3c_set_piomode(struct ata_port *ap, struct ata_device *adev) > +{ > + int mode = adev->pio_mode - XFER_PIO_0; > + struct s3c_ide_info *info = ap->host->private_data; > + ulong ata_cfg = readl(info->ide_addr + S3C_ATA_CFG); > + ulong piotime; > + > + /* Calculates timing parameters for PIO mode */ > + piotime = pata_s3c_setup_timing(info, adev); In fact, for 8-bit (command) timing you should program the slowest mode of the two drives. However, with CF, you probably only have only one drive per channel... > + > + /* Enables IORDY if mode requires it */ > + if (ata_pio_need_iordy(adev)) > + ata_cfg |= S3C_ATA_CFG_IORDYEN; > + else > + ata_cfg &= ~S3C_ATA_CFG_IORDYEN; > + > + /* Host controller supports upto PIO4 only */ > + if (mode >= 0 && mode <= 4) { No need to check -- you won't be passed a mode not specified by your pio_mask. > + writel(ata_cfg, info->ide_addr + S3C_ATA_CFG); > + writel(piotime, info->ide_addr + S3C_ATA_PIO_TIME); > + } > +} [...] > +/* > + * 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 *data_ptr = (u16 *)buf; > + > + if (rw == READ) > + for (i = 0; i < words; i++, data_ptr++) { > + wait_for_host_ready(info); > + *data_ptr = readw(data_addr); Why not just ignore the result? > + wait_for_host_ready(info); > + *data_ptr = readw(info->ide_addr > + + S3C_ATA_PIO_RDATA); > + } > + else > + for (i = 0; i < words; i++, data_ptr++) { > + wait_for_host_ready(info); > + writel(*data_ptr, data_addr); > + } > + > + return words << 1; > +} [...] > +static struct ata_port_operations pata_s3c_port_ops = { > + .inherits = &ata_sff_port_ops, > + .sff_check_status = pata_s3c_check_status, > + .sff_check_altstatus = pata_s3c_check_altstatus, > + .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, > + .sff_dev_select = pata_s3c_dev_select, > + .sff_set_devctl = pata_s3c_set_devctl, I forgot: you also need to override the softreset() method as it accesses several taskfile registers and ioread8()/iowrite8() won't suffice -- Jeff was too quick to ack the patch. See ata_bus_softreset(), ata_devchk(), and ata_sff_wait_after_reset() for the accesses I'm talking about... [...] > +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 = -EINVAL; > + 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 = -EINVAL; > + goto release_device_mem; > + } > + > + if (!request_mem_region(res->start, resource_size(res), DRV_NAME)) { Probably should call devm_request_mem_region() if you're using devm_ioremap()... > + dev_err(dev, "error requesting register region\n"); > + return -EBUSY; > + } > + > + info->ide_addr = devm_ioremap(dev, res->start, resource_size(res)); > + 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; No 'goto' here? [...] > +stop_clk: > + clk_disable(info->clk); Where is clk_put() call? > +release_mem: > + release_mem_region(res->start, resource_size(res)); > +release_device_mem: > + kfree(info); Doesn't using devm_kzalloc() guarantee that the memory will be freed up automatically? > +return ret; Wrong indentation. > +static struct platform_driver pata_s3c_driver = { > + .probe = pata_s3c_probe, > > 2 empty lines -- broken patch? > + .remove = __devexit_p(pata_s3c_remove), > + .id_table = pata_s3c_driver_ids, > + .driver = { > + .name = DRV_NAME, > + .owner = THIS_MODULE, > +#ifdef CONFIG_PM > + .pm = &pata_s3c_pm_ops, > +#endif > + }, > +}; [...] MBR, Sergei -- To unsubscribe from this list: send the line "unsubscribe linux-ide" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, Jun 10, 2010 at 02:43:34PM +0400, Sergei Shtylyov wrote: > 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_cf.c b/drivers/ata/pata_samsung_cf.c >> new file mode 100644 >> index 0000000..fef5515 >> --- /dev/null >> +++ b/drivers/ata/pata_samsung_cf.c >> @@ -0,0 +1,608 @@ >> +/* linux/drivers/ata/pata_samsung_cf.c > > File names in the heading comment are discouraged. > > [...] >> +static void pata_s3c_set_piomode(struct ata_port *ap, struct ata_device *adev) >> +{ >> + int mode = adev->pio_mode - XFER_PIO_0; >> + struct s3c_ide_info *info = ap->host->private_data; >> + ulong ata_cfg = readl(info->ide_addr + S3C_ATA_CFG); >> + ulong piotime; >> + >> + /* Calculates timing parameters for PIO mode */ >> + piotime = pata_s3c_setup_timing(info, adev); > > In fact, for 8-bit (command) timing you should program the slowest > mode of the two drives. However, with CF, you probably only have only one > drive per channel... > >> + >> + /* Enables IORDY if mode requires it */ >> + if (ata_pio_need_iordy(adev)) >> + ata_cfg |= S3C_ATA_CFG_IORDYEN; >> + else >> + ata_cfg &= ~S3C_ATA_CFG_IORDYEN; >> + >> + /* Host controller supports upto PIO4 only */ >> + if (mode >= 0 && mode <= 4) { > > No need to check -- you won't be passed a mode not specified by your pio_mask. probably not even worth bthering with a WARN_ON() for this. .> + writel(ata_cfg, info->ide_addr + S3C_ATA_CFG); >> + writel(piotime, info->ide_addr + S3C_ATA_PIO_TIME); >> + } >> +} > > [...] > >> +/* >> + * 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 *data_ptr = (u16 *)buf; >> + >> + if (rw == READ) >> + for (i = 0; i < words; i++, data_ptr++) { >> + wait_for_host_ready(info); >> + *data_ptr = readw(data_addr); > > Why not just ignore the result? possibly (void)readw(data_addr); >> + wait_for_host_ready(info); >> + *data_ptr = readw(info->ide_addr >> + + S3C_ATA_PIO_RDATA); >> + } It would be nice to have a description of why we're throwing data away from the FIFO. >> + else >> + for (i = 0; i < words; i++, data_ptr++) { >> + wait_for_host_ready(info); >> + writel(*data_ptr, data_addr); >> + } For the write case, would it be better to read the FIFO free lvel and then then write that many words each time? >> + return words << 1; >> +} > > [...] > >> +static struct ata_port_operations pata_s3c_port_ops = { >> + .inherits = &ata_sff_port_ops, >> + .sff_check_status = pata_s3c_check_status, >> + .sff_check_altstatus = pata_s3c_check_altstatus, >> + .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, >> + .sff_dev_select = pata_s3c_dev_select, >> + .sff_set_devctl = pata_s3c_set_devctl, > > I forgot: you also need to override the softreset() method as it > accesses several taskfile registers and ioread8()/iowrite8() won't > suffice -- Jeff was too quick to ack the patch. See ata_bus_softreset(), > ata_devchk(), and ata_sff_wait_after_reset() for the accesses I'm talking > about... > > [...] > >> +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 = -EINVAL; >> + 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 = -EINVAL; >> + goto release_device_mem; >> + } >> + >> + if (!request_mem_region(res->start, resource_size(res), DRV_NAME)) { > > Probably should call devm_request_mem_region() if you're using > devm_ioremap()... > >> + dev_err(dev, "error requesting register region\n"); >> + return -EBUSY; >> + } >> + >> + info->ide_addr = devm_ioremap(dev, res->start, resource_size(res)); >> + 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; We really need to get away from passing a clock name here.
On Thu, Jun 10, 2010 at 04:50:41PM +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> a few more comments, you may or may not want to fix before first submission. > --- > > Changes since v2: > - Changed the DRV_NAME to pata_samsung_cf > - Used msleep instead of mdelay > > drivers/ata/Makefile | 1 + > drivers/ata/pata_samsung_cf.c | 608 +++++++++++++++++++++++++++++++++++++++++ > 3 files changed, 618 insertions(+), 0 deletions(-) > create mode 100644 drivers/ata/pata_samsung_cf.c > > diff --git a/drivers/ata/Kconfig b/drivers/ata/Kconfig > index aa85a98..1b5facf 100644 > --- a/drivers/ata/Kconfig > +++ b/drivers/ata/Kconfig > @@ -796,6 +796,15 @@ config PATA_RZ1000 > > If unsure, say N. > > +config PATA_SAMSUNG_CF > + tristate "Samsung SoC PATA support" > + depends on SAMSUNG_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_WINBOND_VLB > tristate "Winbond W83759A VLB PATA support (Experimental)" > depends on ISA && EXPERIMENTAL > diff --git a/drivers/ata/Makefile b/drivers/ata/Makefile > index 7ef89d7..9576776 100644 > --- a/drivers/ata/Makefile > +++ b/drivers/ata/Makefile > @@ -87,6 +87,7 @@ obj-$(CONFIG_PATA_OF_PLATFORM) += pata_of_platform.o > obj-$(CONFIG_PATA_QDI) += pata_qdi.o > obj-$(CONFIG_PATA_RB532) += pata_rb532_cf.o > obj-$(CONFIG_PATA_RZ1000) += pata_rz1000.o > +obj-$(CONFIG_PATA_SAMSUNG_CF) += pata_samsung_cf.o > obj-$(CONFIG_PATA_WINBOND_VLB) += pata_winbond.o > > # Should be last but two libata driver > diff --git a/drivers/ata/pata_samsung_cf.c b/drivers/ata/pata_samsung_cf.c > new file mode 100644 > index 0000000..fef5515 > --- /dev/null > +++ b/drivers/ata/pata_samsung_cf.c > @@ -0,0 +1,608 @@ > +/* linux/drivers/ata/pata_samsung_cf.c > + * > + * Copyright (c) 2010 Samsung Electronics Co., Ltd. > + * http://www.samsung.com > + * > + * PATA driver for Samsung 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_cf" > +#define DRV_VERSION "0.1" > + > +enum s3c_cpu_type { > + TYPE_S3C64XX, > + 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 void pata_s3c_set_endian(void *s3c_ide_regbase, u8 mode) > +{ should be void __iomem. > +static void pata_s3c_set_piomode(struct ata_port *ap, struct ata_device *adev) > +{ > + int mode = adev->pio_mode - XFER_PIO_0; > + struct s3c_ide_info *info = ap->host->private_data; > + ulong ata_cfg = readl(info->ide_addr + S3C_ATA_CFG); > + ulong piotime; > + > + /* Calculates timing parameters for PIO mode */ > + piotime = pata_s3c_setup_timing(info, adev); > + > + /* Enables IORDY if mode requires it */ > + if (ata_pio_need_iordy(adev)) > + ata_cfg |= S3C_ATA_CFG_IORDYEN; > + else > + ata_cfg &= ~S3C_ATA_CFG_IORDYEN; > + > + /* Host controller supports upto PIO4 only */ > + if (mode >= 0 && mode <= 4) { if this code was to stay in, it would have been better to either WARN_ON() or print some form of error messagee instead of just silently ignoring it... also, you should have probably set the minumum acceptable ATA mode to ensure any further actions would not cause problems with trying to run the b us too fast. > +/* > + * Waits until the IDE controller is able to perform next read/write > + * operation to the disk. Needed for 64XX series boards only. > + */ if it is needed only for s3c64xx series, why don't we bail if the info->type != 64xx? > +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) would be neater to have void __iomem *fifo_reg = info->ide_addr + info->fifo_status_reg ... while(...) { if (readl(fifo_reg) >> 28) == 0) ... > + return 0; > + } > + return -EBUSY; > +} [snip] > +/* > + * 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 *data_ptr = (u16 *)buf; note, is there any chance of being passed a number of bytes? if so should we at least WARN_ON(buflen & 1) and return an error? > + if (rw == READ) > + for (i = 0; i < words; i++, data_ptr++) { > + wait_for_host_ready(info); > + *data_ptr = readw(data_addr); > + wait_for_host_ready(info); > + *data_ptr = readw(info->ide_addr > + + S3C_ATA_PIO_RDATA); > + } > + else > + for (i = 0; i < words; i++, data_ptr++) { > + wait_for_host_ready(info); > + writel(*data_ptr, data_addr); > + } > + > + return words << 1; > +} > + > +/* > + * pata_s3c_dev_select - Select device on ATA bus > + */ > +static void pata_s3c_dev_select(struct ata_port *ap, unsigned int device) > +{ > + u8 tmp; > + > + if (device == 0) > + tmp = ATA_DEVICE_OBS; > + else > + tmp = ATA_DEVICE_OBS | ATA_DEV1; you could have done u8 tmp = ATA_DEVICE_OBS; if (device != 0) tmp |= ATA_DEV1 > + ata_outb(ap->host, tmp, ap->ioaddr.device_addr); > + ata_sff_pause(ap); > +} > + [snip] > +static void pata_s3c_enable(void *s3c_ide_regbase, u8 state) state would be better as a bool or a natural int. > +{ > + 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); no decoding of the interrupt cause? > + return ata_sff_interrupt(irq, dev_instance); > +} > +static int __devexit pata_s3c_remove(struct platform_device *pdev) > +{ > + struct ata_host *host = platform_get_drvdata(pdev); > + struct s3c_ide_info *info; > + struct resource *res; > + > + if (!host) > + return 0; don't think this check is really necessary. > + info = host->private_data; > + > + ata_host_detach(host); > + > + clk_disable(info->clk); > + clk_put(info->clk); > + res = platform_get_resource(pdev, IORESOURCE_MEM, 0); > + release_mem_region(res->start, resource_size(res)); > + kfree(info); > + > + return 0; > +} > + > +#ifdef CONFIG_PM > +static int pata_s3c_suspend(struct device *dev) > +{ > + struct platform_device *pdev = to_platform_device(dev); > + struct ata_host *host = platform_get_drvdata(pdev); > + pm_message_t state = PMSG_SUSPEND; > + > + if (host) > + return ata_host_suspend(host, state); again, don't think you'l lget called here if you didn't sucessfully bind. > + else > + return 0; > +} > + > +static int pata_s3c_resume(struct device *dev) > +{ > + struct platform_device *pdev = to_platform_device(dev); > + struct ata_host *host = platform_get_drvdata(pdev); > + 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; > +} > + > +static const struct dev_pm_ops pata_s3c_pm_ops = { > + .suspend = pata_s3c_suspend, > + .resume = pata_s3c_resume, > +};
Sergei Shtylyov wrote: > > 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. > Hi, Thanks for your comments. > > 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_cf.c b/drivers/ata/pata_samsung_cf.c > > new file mode 100644 > > index 0000000..fef5515 > > --- /dev/null > > +++ b/drivers/ata/pata_samsung_cf.c > > @@ -0,0 +1,608 @@ > > +/* linux/drivers/ata/pata_samsung_cf.c > > File names in the heading comment are discouraged. > Hmm. I used like that in other device drivers. Ok..will remove the file name in the heading comment. > [...] > > > +static unsigned long > > +pata_s3c_setup_timing(struct s3c_ide_info *info, struct ata_device *adev) > > +{ > > + const struct ata_timing *timing; > > + int cycle_time; > > + int t1; > > + int t2; > > + int t2i; > > + ulong piotime; > > + > > + cycle_time = (int)(1000000000UL / clk_get_rate(info->clk)); > > + > > + timing = ata_timing_find_mode(adev->pio_mode); > > + t1 = (timing->setup / cycle_time) & 0xf; > > + t2 = (timing->act8b / cycle_time) & 0xff; > > + t2i = (timing->rec8b / cycle_time) & 0xff; > > Why are you still not using ata_timing_compute()? > Ok, will remove ata_timing_find_mode and used ata_timing_compute in pata_s3c_set_piomode for timing parameters. > > + > > + piotime = (t2i << 12) | (t2 << 4) | t1; > > + > > + return piotime; > > +} > > + > > +static void pata_s3c_set_piomode(struct ata_port *ap, struct ata_device *adev) > > +{ > > + int mode = adev->pio_mode - XFER_PIO_0; > > + struct s3c_ide_info *info = ap->host->private_data; > > + ulong ata_cfg = readl(info->ide_addr + S3C_ATA_CFG); > > + ulong piotime; > > + > > + /* Calculates timing parameters for PIO mode */ > > + piotime = pata_s3c_setup_timing(info, adev); > > In fact, for 8-bit (command) timing you should program the slowest mode of > the two drives. However, with CF, you probably only have only one drive per > channel... > Below code looks OK ? if (ata_timing_compute(adev, adev->pio_mode, &timing, cycle_time, 0)) { dev_err(ap->dev, "Failed to compute ATA timing\n"); piotime = pata_s3c_setup_timing(info, &initial_timing); } else { piotime = pata_s3c_setup_timing(info, &timing); } where initial_timing is for PIO0. I have added the below struct static const struct ata_timing initial_timing = {XFER_PIO_0, 70, 290, 240, 600, 165, 150, 0, 600, 0}; > > + > > + /* Enables IORDY if mode requires it */ > > + if (ata_pio_need_iordy(adev)) > > + ata_cfg |= S3C_ATA_CFG_IORDYEN; > > + else > > + ata_cfg &= ~S3C_ATA_CFG_IORDYEN; > > + > > + /* Host controller supports upto PIO4 only */ > > + if (mode >= 0 && mode <= 4) { > > No need to check -- you won't be passed a mode not specified by your > pio_mask. > Will remove the check. > > + writel(ata_cfg, info->ide_addr + S3C_ATA_CFG); > > + writel(piotime, info->ide_addr + S3C_ATA_PIO_TIME); > > + } > > +} > > [...] > > > +/* > > + * 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 *data_ptr = (u16 *)buf; > > + > > + if (rw == READ) > > + for (i = 0; i < words; i++, data_ptr++) { > > + wait_for_host_ready(info); > > + *data_ptr = readw(data_addr); > > Why not just ignore the result? > Will change to (void)readw(data_addr) as Ben Dooks suggested. > > + wait_for_host_ready(info); > > + *data_ptr = readw(info->ide_addr > > + + S3C_ATA_PIO_RDATA); > > + } > > + else > > + for (i = 0; i < words; i++, data_ptr++) { > > + wait_for_host_ready(info); > > + writel(*data_ptr, data_addr); > > + } > > + > > + return words << 1; > > +} > > [...] > > > +static struct ata_port_operations pata_s3c_port_ops = { > > + .inherits = &ata_sff_port_ops, > > + .sff_check_status = pata_s3c_check_status, > > + .sff_check_altstatus = pata_s3c_check_altstatus, > > + .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, > > + .sff_dev_select = pata_s3c_dev_select, > > + .sff_set_devctl = pata_s3c_set_devctl, > > I forgot: you also need to override the softreset() method as it accesses > several taskfile registers and ioread8()/iowrite8() won't suffice -- Jeff was > too quick to ack the patch. See ata_bus_softreset(), ata_devchk(), and > ata_sff_wait_after_reset() for the accesses I'm talking about... > Will add softreset method and the associated functions required. > [...] > > > +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 = -EINVAL; > > + 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 = -EINVAL; > > + goto release_device_mem; > > + } > > + > > + if (!request_mem_region(res->start, resource_size(res), DRV_NAME)) { > > Probably should call devm_request_mem_region() if you're using > devm_ioremap()... > Will change to devm_request_mem_region. > > + dev_err(dev, "error requesting register region\n"); > > + return -EBUSY; > > + } > > + > > + info->ide_addr = devm_ioremap(dev, res->start, resource_size(res)); > > + 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; > > No 'goto' here? > Will add return ret instead of goto. > [...] > > > +stop_clk: > > + clk_disable(info->clk); > > Where is clk_put() call? > Will add clk_put > > +release_mem: > > + release_mem_region(res->start, resource_size(res)); > > +release_device_mem: > > + kfree(info); > > Doesn't using devm_kzalloc() guarantee that the memory will be freed up > automatically? > Will remove kfree and release_mem_region because of devm_kzalloc and devm_request_mem_region usage > > +return ret; > > Wrong indentation. > OK. > > +static struct platform_driver pata_s3c_driver = { > > + .probe = pata_s3c_probe, > > > > > > 2 empty lines -- broken patch? > Seems OK at my end. > > + .remove = __devexit_p(pata_s3c_remove), > > + .id_table = pata_s3c_driver_ids, > > + .driver = { > > + .name = DRV_NAME, > > + .owner = THIS_MODULE, > > +#ifdef CONFIG_PM > > + .pm = &pata_s3c_pm_ops, > > +#endif > > + }, > > +}; > > [...] > > MBR, Sergei Thanks. Best regards, Kgene. -- Kukjin Kim <kgene.kim@samsung.com>, Senior Engineer, SW Solution Development Team, Samsung Electronics Co., Ltd. -- To unsubscribe from this list: send the line "unsubscribe linux-ide" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Ben Dooks wrote: > > On Thu, Jun 10, 2010 at 04:50:41PM +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. > > Thanks for your review. :-) > > Signed-off-by: Abhilash Kesavan <a.kesavan@samsung.com> > > Signed-off-by: Kukjin Kim <kgene.kim@samsung.com> > > a few more comments, you may or may not want to fix before first > submission. > > > --- > > > > Changes since v2: > > - Changed the DRV_NAME to pata_samsung_cf > > - Used msleep instead of mdelay > > > > drivers/ata/Makefile | 1 + > > drivers/ata/pata_samsung_cf.c | 608 > +++++++++++++++++++++++++++++++++++++++++ > > 3 files changed, 618 insertions(+), 0 deletions(-) > > create mode 100644 drivers/ata/pata_samsung_cf.c > > > > diff --git a/drivers/ata/Kconfig b/drivers/ata/Kconfig > > index aa85a98..1b5facf 100644 > > --- a/drivers/ata/Kconfig > > +++ b/drivers/ata/Kconfig > > @@ -796,6 +796,15 @@ config PATA_RZ1000 > > > > If unsure, say N. > > > > +config PATA_SAMSUNG_CF > > + tristate "Samsung SoC PATA support" > > + depends on SAMSUNG_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_WINBOND_VLB > > tristate "Winbond W83759A VLB PATA support (Experimental)" > > depends on ISA && EXPERIMENTAL > > diff --git a/drivers/ata/Makefile b/drivers/ata/Makefile > > index 7ef89d7..9576776 100644 > > --- a/drivers/ata/Makefile > > +++ b/drivers/ata/Makefile > > @@ -87,6 +87,7 @@ obj-$(CONFIG_PATA_OF_PLATFORM) += > pata_of_platform.o > > obj-$(CONFIG_PATA_QDI) += pata_qdi.o > > obj-$(CONFIG_PATA_RB532) += pata_rb532_cf.o > > obj-$(CONFIG_PATA_RZ1000) += pata_rz1000.o > > +obj-$(CONFIG_PATA_SAMSUNG_CF) += pata_samsung_cf.o > > obj-$(CONFIG_PATA_WINBOND_VLB) += pata_winbond.o > > > > # Should be last but two libata driver > > diff --git a/drivers/ata/pata_samsung_cf.c b/drivers/ata/pata_samsung_cf.c > > new file mode 100644 > > index 0000000..fef5515 > > --- /dev/null > > +++ b/drivers/ata/pata_samsung_cf.c > > @@ -0,0 +1,608 @@ > > +/* linux/drivers/ata/pata_samsung_cf.c > > + * > > + * Copyright (c) 2010 Samsung Electronics Co., Ltd. > > + * http://www.samsung.com > > + * > > + * PATA driver for Samsung 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_cf" > > +#define DRV_VERSION "0.1" > > + > > +enum s3c_cpu_type { > > + TYPE_S3C64XX, > > + 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 void pata_s3c_set_endian(void *s3c_ide_regbase, u8 mode) > > +{ > should be void __iomem. > OK. > > +static void pata_s3c_set_piomode(struct ata_port *ap, struct ata_device *adev) > > +{ > > + int mode = adev->pio_mode - XFER_PIO_0; > > + struct s3c_ide_info *info = ap->host->private_data; > > + ulong ata_cfg = readl(info->ide_addr + S3C_ATA_CFG); > > + ulong piotime; > > + > > + /* Calculates timing parameters for PIO mode */ > > + piotime = pata_s3c_setup_timing(info, adev); > > + > > + /* Enables IORDY if mode requires it */ > > + if (ata_pio_need_iordy(adev)) > > + ata_cfg |= S3C_ATA_CFG_IORDYEN; > > + else > > + ata_cfg &= ~S3C_ATA_CFG_IORDYEN; > > + > > + /* Host controller supports upto PIO4 only */ > > + if (mode >= 0 && mode <= 4) { > > if this code was to stay in, it would have been better to either > WARN_ON() or print some form of error messagee instead of just silently > ignoring it... also, you should have probably set the minumum acceptable > ATA mode to ensure any further actions would not cause problems with > trying to run the b us too fast. > Will remove the code..will set up the timing parameters using ata_timing_compute and the initial timing will be for PIO0 in case of failure. > > +/* > > + * Waits until the IDE controller is able to perform next read/write > > + * operation to the disk. Needed for 64XX series boards only. > > + */ > > if it is needed only for s3c64xx series, why don't we bail if the > info->type != 64xx? > This is called via pata_s3c_port_ops for 64xx else goes to pata_s5p_port_ops. > > +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) > > would be neater to have > void __iomem *fifo_reg = info->ide_addr + info->fifo_status_reg > ... > > while(...) { > if (readl(fifo_reg) >> 28) == 0) > ... > OK. > > + return 0; > > + } > > + return -EBUSY; > > +} > > [snip] > > > +/* > > + * 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 *data_ptr = (u16 *)buf; > > note, is there any chance of being passed a number of bytes? if so > should we at least WARN_ON(buflen & 1) and return an error? > Will add a WARN_ON. > > + if (rw == READ) > > + for (i = 0; i < words; i++, data_ptr++) { > > + wait_for_host_ready(info); > > + *data_ptr = readw(data_addr); > > + wait_for_host_ready(info); > > + *data_ptr = readw(info->ide_addr > > + + S3C_ATA_PIO_RDATA); > > + } Will add a description for read/write accesses. > > + else > > + for (i = 0; i < words; i++, data_ptr++) { > > + wait_for_host_ready(info); > > + writel(*data_ptr, data_addr); > > + } > > + > > + return words << 1; > > +} > > + > > +/* > > + * pata_s3c_dev_select - Select device on ATA bus > > + */ > > +static void pata_s3c_dev_select(struct ata_port *ap, unsigned int device) > > +{ > > + u8 tmp; > > + > > + if (device == 0) > > + tmp = ATA_DEVICE_OBS; > > + else > > + tmp = ATA_DEVICE_OBS | ATA_DEV1; > > you could have done > u8 tmp = ATA_DEVICE_OBS; > > if (device != 0) > tmp |= ATA_DEV1 > > > + ata_outb(ap->host, tmp, ap->ioaddr.device_addr); > > + ata_sff_pause(ap); > > +} > > + > > [snip] > > > +static void pata_s3c_enable(void *s3c_ide_regbase, u8 state) > state would be better as a bool or a natural int. OK. > > +{ > > + 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); > > no decoding of the interrupt cause? > Others interrupts are for udma, mdma and EBI (we are using direct mode) and so I let them be. > > + return ata_sff_interrupt(irq, dev_instance); > > +} > > > +static int __devexit pata_s3c_remove(struct platform_device *pdev) > > +{ > > + struct ata_host *host = platform_get_drvdata(pdev); > > + struct s3c_ide_info *info; > > + struct resource *res; > > > + > > + if (!host) > > + return 0; > > don't think this check is really necessary. > OK. > > + info = host->private_data; > > + > > + ata_host_detach(host); > > + > > + clk_disable(info->clk); > > + clk_put(info->clk); > > + res = platform_get_resource(pdev, IORESOURCE_MEM, 0); > > + release_mem_region(res->start, resource_size(res)); > > + kfree(info); > > + > > + return 0; > > +} > > + > > +#ifdef CONFIG_PM > > +static int pata_s3c_suspend(struct device *dev) > > +{ > > + struct platform_device *pdev = to_platform_device(dev); > > + struct ata_host *host = platform_get_drvdata(pdev); > > + pm_message_t state = PMSG_SUSPEND; > > + > > + if (host) > > + return ata_host_suspend(host, state); > > again, don't think you'l lget called here if you didn't sucessfully bind. > OK. > > + else > > + return 0; > > +} > > + > > +static int pata_s3c_resume(struct device *dev) > > +{ > > + struct platform_device *pdev = to_platform_device(dev); > > + struct ata_host *host = platform_get_drvdata(pdev); > > + 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; > > +} > > + > > +static const struct dev_pm_ops pata_s3c_pm_ops = { > > + .suspend = pata_s3c_suspend, > > + .resume = pata_s3c_resume, > > +}; > > -- Thanks. Best regards, Kgene. -- Kukjin Kim <kgene.kim@samsung.com>, Senior Engineer, SW Solution Development Team, Samsung Electronics Co., Ltd. -- To unsubscribe from this list: send the line "unsubscribe linux-ide" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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. > Hi, > Thanks for your comments. >>> 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_cf.c b/drivers/ata/pata_samsung_cf.c >>> new file mode 100644 >>> index 0000000..fef5515 >>> --- /dev/null >>> +++ b/drivers/ata/pata_samsung_cf.c >>> @@ -0,0 +1,608 @@ >>> +/* linux/drivers/ata/pata_samsung_cf.c >> File names in the heading comment are discouraged. > Hmm. I used like that in other device drivers. Nevertheless, it's quite an old rule already. > Ok..will remove the file name in the heading comment. >> [...] >>> + >>> + piotime = (t2i << 12) | (t2 << 4) | t1; >>> + >>> + return piotime; >>> +} >>> + >>> +static void pata_s3c_set_piomode(struct ata_port *ap, struct ata_device *adev) >>> +{ >>> + int mode = adev->pio_mode - XFER_PIO_0; >>> + struct s3c_ide_info *info = ap->host->private_data; >>> + ulong ata_cfg = readl(info->ide_addr + S3C_ATA_CFG); >>> + ulong piotime; >>> + >>> + /* Calculates timing parameters for PIO mode */ >>> + piotime = pata_s3c_setup_timing(info, adev); >> In fact, for 8-bit (command) timing you should program the slowest mode of >> the two drives. However, with CF, you probably only have only one drive per >> channel... > Below code looks OK ? No, it doesn't. > if (ata_timing_compute(adev, adev->pio_mode, &timing, cycle_time, 0)) > { > dev_err(ap->dev, "Failed to compute ATA timing\n"); > piotime = pata_s3c_setup_timing(info, &initial_timing); > } else { > piotime = pata_s3c_setup_timing(info, &timing); > } > where initial_timing is for PIO0. I have added the below struct > static const struct ata_timing initial_timing = > {XFER_PIO_0, 70, 290, 240, 600, 165, 150, 0, 600, 0}; I'd call ata_timing_find_mode(XFER_PIO_0) rather than duplicating the ata_timing entry. But really, you shouldn't set any timing for an invalid mode and, as I said, you won't be passed one, so there's nor much sense in calling ata_timing_compute() and checking its result; anyway, you'd want to call ata_timing_find_mode() here instead of ata_timing_compute() because the latter returns already quantized timings, but initial_timing is not quantized, you'll have to call ata_timing_compute() in pata_s3c_setup_timing() anyway. >>> + >>> + /* Enables IORDY if mode requires it */ >>> + if (ata_pio_need_iordy(adev)) >>> + ata_cfg |= S3C_ATA_CFG_IORDYEN; >>> + else >>> + ata_cfg &= ~S3C_ATA_CFG_IORDYEN; >>> + >>> + /* Host controller supports upto PIO4 only */ >>> + if (mode >= 0 && mode <= 4) { >> No need to check -- you won't be passed a mode not specified by your >> pio_mask. > Will remove the check. >>> +static int __devinit pata_s3c_probe(struct platform_device *pdev) [...] >>> + if (!request_mem_region(res->start, resource_size(res), DRV_NAME)) { >> Probably should call devm_request_mem_region() if you're using >> devm_ioremap()... > Will change to devm_request_mem_region. This function does exist, if I don't mistake... >>> +release_mem: >>> + release_mem_region(res->start, resource_size(res)); >>> +release_device_mem: >>> + kfree(info); >> Doesn't using devm_kzalloc() guarantee that the memory will be freed up >> automatically? > Will remove kfree and release_mem_region because of devm_kzalloc and > devm_request_mem_region usage I don't know devres librarry capabilities well. Tejun, am I right? >>> +static struct platform_driver pata_s3c_driver = { >>> + .probe = pata_s3c_probe, >>> >>> >> 2 empty lines -- broken patch? > Seems OK at my end. The sent patch had them, nevertheless. MBR, Sergei -- To unsubscribe from this list: send the line "unsubscribe linux-ide" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Sergei Shtylyov wrote: > > 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. > > > Hi, > > > Thanks for your comments. > > >>> 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_cf.c b/drivers/ata/pata_samsung_cf.c > >>> new file mode 100644 > >>> index 0000000..fef5515 > >>> --- /dev/null > >>> +++ b/drivers/ata/pata_samsung_cf.c > >>> @@ -0,0 +1,608 @@ > >>> +/* linux/drivers/ata/pata_samsung_cf.c > > >> File names in the heading comment are discouraged. > > > Hmm. I used like that in other device drivers. > > Nevertheless, it's quite an old rule already. > > > Ok..will remove the file name in the heading comment. > > >> [...] > > >>> + > >>> + piotime = (t2i << 12) | (t2 << 4) | t1; > >>> + > >>> + return piotime; > >>> +} > >>> + > >>> +static void pata_s3c_set_piomode(struct ata_port *ap, struct ata_device > *adev) > >>> +{ > >>> + int mode = adev->pio_mode - XFER_PIO_0; > >>> + struct s3c_ide_info *info = ap->host->private_data; > >>> + ulong ata_cfg = readl(info->ide_addr + S3C_ATA_CFG); > >>> + ulong piotime; > >>> + > >>> + /* Calculates timing parameters for PIO mode */ > >>> + piotime = pata_s3c_setup_timing(info, adev); > > >> In fact, for 8-bit (command) timing you should program the slowest mode > of > >> the two drives. However, with CF, you probably only have only one drive per > >> channel... > > > Below code looks OK ? > > No, it doesn't. > > > if (ata_timing_compute(adev, adev->pio_mode, &timing, cycle_time, 0)) > > { > > dev_err(ap->dev, "Failed to compute ATA timing\n"); > > piotime = pata_s3c_setup_timing(info, &initial_timing); > > } else { > > piotime = pata_s3c_setup_timing(info, &timing); > > } > > where initial_timing is for PIO0. I have added the below struct > > > static const struct ata_timing initial_timing = > > {XFER_PIO_0, 70, 290, 240, 600, 165, 150, 0, 600, 0}; > > I'd call ata_timing_find_mode(XFER_PIO_0) rather than duplicating the > ata_timing entry. But really, you shouldn't set any timing for an invalid mode > and, as I said, you won't be passed one, so there's nor much sense in calling > ata_timing_compute() and checking its result; anyway, you'd want to call > ata_timing_find_mode() here instead of ata_timing_compute() because the latter > returns already quantized timings, but initial_timing is not quantized, you'll > have to call ata_timing_compute() in pata_s3c_setup_timing() anyway. > OK, no check for invalid mode then. Now using 'ata_timing_compute' to get the quantized timings t1, t2 and t2i and then writing them to PIO_TIME registers. PIO_TIME [0:3] t1 [4:11] t2 [12:19] t2i. > >>> + > >>> + /* Enables IORDY if mode requires it */ > >>> + if (ata_pio_need_iordy(adev)) > >>> + ata_cfg |= S3C_ATA_CFG_IORDYEN; > >>> + else > >>> + ata_cfg &= ~S3C_ATA_CFG_IORDYEN; > >>> + > >>> + /* Host controller supports upto PIO4 only */ > >>> + if (mode >= 0 && mode <= 4) { > > >> No need to check -- you won't be passed a mode not specified by your > >> pio_mask. > > > Will remove the check. > > >>> +static int __devinit pata_s3c_probe(struct platform_device *pdev) > [...] > >>> + if (!request_mem_region(res->start, resource_size(res), DRV_NAME)) { > > >> Probably should call devm_request_mem_region() if you're using > >> devm_ioremap()... > > > Will change to devm_request_mem_region. > > This function does exist, if I don't mistake... > > >>> +release_mem: > >>> + release_mem_region(res->start, resource_size(res)); > >>> +release_device_mem: > >>> + kfree(info); > > >> Doesn't using devm_kzalloc() guarantee that the memory will be freed up > >> automatically? > > > Will remove kfree and release_mem_region because of devm_kzalloc and > > devm_request_mem_region usage > > I don't know devres librarry capabilities well. Tejun, am I right? > > >>> +static struct platform_driver pata_s3c_driver = { > >>> + .probe = pata_s3c_probe, > >>> > >>> > > >> 2 empty lines -- broken patch? > > > Seems OK at my end. > > The sent patch had them, nevertheless. > > MBR, Sergei Thanks. Best regards, Kgene. -- Kukjin Kim <kgene.kim@samsung.com>, Senior Engineer, SW Solution Development Team, Samsung Electronics Co., Ltd. -- To unsubscribe from this list: send the line "unsubscribe linux-ide" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/ata/Kconfig b/drivers/ata/Kconfig index aa85a98..1b5facf 100644 --- a/drivers/ata/Kconfig +++ b/drivers/ata/Kconfig @@ -796,6 +796,15 @@ config PATA_RZ1000 If unsure, say N. +config PATA_SAMSUNG_CF + tristate "Samsung SoC PATA support" + depends on SAMSUNG_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_WINBOND_VLB tristate "Winbond W83759A VLB PATA support (Experimental)" depends on ISA && EXPERIMENTAL diff --git a/drivers/ata/Makefile b/drivers/ata/Makefile index 7ef89d7..9576776 100644 --- a/drivers/ata/Makefile +++ b/drivers/ata/Makefile @@ -87,6 +87,7 @@ obj-$(CONFIG_PATA_OF_PLATFORM) += pata_of_platform.o obj-$(CONFIG_PATA_QDI) += pata_qdi.o obj-$(CONFIG_PATA_RB532) += pata_rb532_cf.o obj-$(CONFIG_PATA_RZ1000) += pata_rz1000.o +obj-$(CONFIG_PATA_SAMSUNG_CF) += pata_samsung_cf.o obj-$(CONFIG_PATA_WINBOND_VLB) += pata_winbond.o # Should be last but two libata driver diff --git a/drivers/ata/pata_samsung_cf.c b/drivers/ata/pata_samsung_cf.c new file mode 100644 index 0000000..fef5515 --- /dev/null +++ b/drivers/ata/pata_samsung_cf.c @@ -0,0 +1,608 @@ +/* linux/drivers/ata/pata_samsung_cf.c + * + * Copyright (c) 2010 Samsung Electronics Co., Ltd. + * http://www.samsung.com + * + * PATA driver for Samsung 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_cf" +#define DRV_VERSION "0.1" + +enum s3c_cpu_type { + TYPE_S3C64XX, + 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 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_cfg_mode(void *s3c_ide_sfrbase) +{ + /* Select true-ide as the internal operating mode */ + writel(readl(s3c_ide_sfrbase + S3C_CFATA_MUX) | S3C_CFATA_MUX_TRUEIDE, + s3c_ide_sfrbase + S3C_CFATA_MUX); +} + +static unsigned long +pata_s3c_setup_timing(struct s3c_ide_info *info, struct ata_device *adev) +{ + const struct ata_timing *timing; + int cycle_time; + int t1; + int t2; + int t2i; + ulong piotime; + + cycle_time = (int)(1000000000UL / clk_get_rate(info->clk)); + + timing = ata_timing_find_mode(adev->pio_mode); + t1 = (timing->setup / cycle_time) & 0xf; + t2 = (timing->act8b / cycle_time) & 0xff; + t2i = (timing->rec8b / cycle_time) & 0xff; + + piotime = (t2i << 12) | (t2 << 4) | t1; + + return piotime; +} + +static void pata_s3c_set_piomode(struct ata_port *ap, struct ata_device *adev) +{ + int mode = adev->pio_mode - XFER_PIO_0; + struct s3c_ide_info *info = ap->host->private_data; + ulong ata_cfg = readl(info->ide_addr + S3C_ATA_CFG); + ulong piotime; + + /* Calculates timing parameters for PIO mode */ + piotime = pata_s3c_setup_timing(info, adev); + + /* Enables IORDY if mode requires it */ + if (ata_pio_need_iordy(adev)) + ata_cfg |= S3C_ATA_CFG_IORDYEN; + else + ata_cfg &= ~S3C_ATA_CFG_IORDYEN; + + /* Host controller supports upto PIO4 only */ + if (mode >= 0 && mode <= 4) { + writel(ata_cfg, info->ide_addr + S3C_ATA_CFG); + writel(piotime, info->ide_addr + S3C_ATA_PIO_TIME); + } +} + +/* + * 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); + 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 = readb(reg); + wait_for_host_ready(info); + temp = 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) { + 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)) { + 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->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) { + 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; + } +} + +/* + * 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_check_altstatus - Read alternate device status register + */ +static u8 pata_s3c_check_altstatus(struct ata_port *ap) +{ + return ata_inb(ap->host, ap->ioaddr.altstatus_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 *data_ptr = (u16 *)buf; + + if (rw == READ) + for (i = 0; i < words; i++, data_ptr++) { + wait_for_host_ready(info); + *data_ptr = readw(data_addr); + wait_for_host_ready(info); + *data_ptr = readw(info->ide_addr + + S3C_ATA_PIO_RDATA); + } + else + for (i = 0; i < words; i++, data_ptr++) { + wait_for_host_ready(info); + writel(*data_ptr, data_addr); + } + + return words << 1; +} + +/* + * pata_s3c_dev_select - Select device on ATA bus + */ +static void pata_s3c_dev_select(struct ata_port *ap, unsigned int device) +{ + u8 tmp; + + if (device == 0) + tmp = ATA_DEVICE_OBS; + else + tmp = ATA_DEVICE_OBS | ATA_DEV1; + + ata_outb(ap->host, tmp, ap->ioaddr.device_addr); + ata_sff_pause(ap); +} + +/* + * pata_s3c_set_devctl - Write device control register + */ +static void pata_s3c_set_devctl(struct ata_port *ap, u8 ctl) +{ + ata_outb(ap->host, ctl, ap->ioaddr.ctl_addr); +} + +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_check_altstatus = pata_s3c_check_altstatus, + .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, + .sff_dev_select = pata_s3c_dev_select, + .sff_set_devctl = pata_s3c_set_devctl, + .set_piomode = pata_s3c_set_piomode, +}; + +static struct ata_port_operations pata_s5p_port_ops = { + .inherits = &ata_sff_port_ops, + .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_hwinit(struct s3c_ide_info *info, + struct s3c_ide_platdata *pdata) +{ + switch (info->cpu_type) { + case TYPE_S3C64XX: + /* Configure as big endian */ + pata_s3c_cfg_mode(info->sfr_addr); + pata_s3c_set_endian(info->ide_addr, 1); + pata_s3c_enable(info->ide_addr, 1); + msleep(100); + + /* Remove IRQ Status */ + writel(0x1f, info->ide_addr + S3C_ATA_IRQ); + writel(0x1b, info->ide_addr + S3C_ATA_IRQ_MSK); + break; + + case TYPE_S5PC100: + pata_s3c_cfg_mode(info->sfr_addr); + + case TYPE_S5PV210: + /* Configure as little endian */ + pata_s3c_set_endian(info->ide_addr, 0); + pata_s3c_enable(info->ide_addr, 1); + msleep(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 = -EINVAL; + 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 = -EINVAL; + goto release_device_mem; + } + + if (!request_mem_region(res->start, resource_size(res), DRV_NAME)) { + dev_err(dev, "error requesting register region\n"); + return -EBUSY; + } + + info->ide_addr = devm_ioremap(dev, res->start, resource_size(res)); + 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; + } + + clk_enable(info->clk); + + /* 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_S3C64XX) { + ap->ops = &pata_s3c_port_ops; + info->sfr_addr = info->ide_addr + 0x1800; + 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 += 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; + + 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 ? pata_s3c_irq : NULL, + 0, &pata_s3c_sht); + + platform_set_drvdata(pdev, host); + +stop_clk: + clk_disable(info->clk); +release_mem: + release_mem_region(res->start, resource_size(res)); +release_device_mem: + kfree(info); +return ret; +} + +static int __devexit pata_s3c_remove(struct platform_device *pdev) +{ + struct ata_host *host = platform_get_drvdata(pdev); + struct s3c_ide_info *info; + struct resource *res; + + if (!host) + return 0; + info = host->private_data; + + ata_host_detach(host); + + clk_disable(info->clk); + clk_put(info->clk); + res = platform_get_resource(pdev, IORESOURCE_MEM, 0); + release_mem_region(res->start, resource_size(res)); + kfree(info); + + return 0; +} + +#ifdef CONFIG_PM +static int pata_s3c_suspend(struct device *dev) +{ + struct platform_device *pdev = to_platform_device(dev); + struct ata_host *host = platform_get_drvdata(pdev); + pm_message_t state = PMSG_SUSPEND; + + if (host) + return ata_host_suspend(host, state); + else + return 0; +} + +static int pata_s3c_resume(struct device *dev) +{ + struct platform_device *pdev = to_platform_device(dev); + struct ata_host *host = platform_get_drvdata(pdev); + 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; +} + +static const struct dev_pm_ops pata_s3c_pm_ops = { + .suspend = pata_s3c_suspend, + .resume = pata_s3c_resume, +}; +#endif + +/* driver device registration */ +static struct platform_device_id pata_s3c_driver_ids[] = { + { + .name = "s3c64xx-pata", + .driver_data = TYPE_S3C64XX, + }, { + .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 + .pm = &pata_s3c_pm_ops, +#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);