[PATCH v3] libata: pata_samsung_cf: Add Samsung PATA controller driver

[Kukjin Kim]

Ben Dooks wrote:
> 
> On Thu, Jun 10, 2010 at 04:50:41PM +0900, Kukjin Kim wrote:
> > From: Abhilash Kesavan <a.kesavan at 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 at samsung.com>
> > Signed-off-by: Kukjin Kim <kgene.kim at 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 at samsung.com>, Senior Engineer,
SW Solution Development Team, Samsung Electronics Co., Ltd.