libata: pata_samsung_cf: Add Samsung PATA controller driver
Subsystems (2)
PATCH v3 1/1 libata: pata_samsung_cf: Add Samsung PATA controller driver
From: Abhilash Kesavan <[email protected]>
Adds support for the Samsung PATA controller. This driver is based on the Libata subsystem and references the earlier patches sent for IDE subsystem.
---
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
drivers/ata/Kconfig +9 -0
@@ -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
drivers/ata/Makefile +1 -0
@@ -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
drivers/ata/pata_samsung_cf.c new file +608 -0
608 lines not on this page · raw message
Signature · 1 line
1.6.2.5
PATCH v3 1/1 libata: pata_samsung_cf: Add Samsung PATA controller driver
Quoted 9 lines · Kukjin Kim
From: Abhilash Kesavan<[email protected]>
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<[email protected]>Signed-off-by Kukjin Kim<[email protected]>---
Quoted 8 lines
Changes since v2:
- Changed theDRV_NAMEtopata_samsung_cf
- Used msleep instead of mdelaydrivers/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
I presume you'll send this upstream via the appropriate ARM tree.
PATCH v3 1/1 libata: pata_samsung_cf: Add Samsung PATA controller driver
Quoted 19 lines · Jeff Garzik
Quoted 9 lines · Kukjin Kim
From: Abhilash Kesavan<[email protected]>
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<[email protected]>Signed-off-by Kukjin Kim<[email protected]>---
Quoted 8 lines
Changes since v2:
- Changed theDRV_NAMEtopata_samsung_cf
- Used msleep instead of mdelaydrivers/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
Quoted 4 lines
Acked-by Jeff Garzik <[email protected]>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?
Signature · 4 lines
Ben Q: What's a light-year? A: One-third less calories than a regular year.
PATCH v3 1/1 libata: pata_samsung_cf: Add Samsung PATA controller driver
Quoted 23 lines · Ben Dooks
Quoted 22 lines · Jeff Garzik
Quoted 9 lines · Kukjin Kim
From: Abhilash Kesavan<[email protected]>
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<[email protected]>Signed-off-by Kukjin Kim<[email protected]>---
Quoted 8 lines
Changes since v2:
- Changed theDRV_NAMEtopata_samsung_cf
- Used msleep instead of mdelaydrivers/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.cAcked-by Jeff Garzik<[email protected]>
Quoted 7 lines
Quoted 1 line
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
MakefileandKconfig).
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?
Quoted 2 lines
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
PATCH v3 1/1 libata: pata_samsung_cf: Add Samsung PATA controller driver
In reply to Kukjin KimQuoted 7 lines · Kukjin Kim
From: Abhilash Kesavan <[email protected]>
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 <[email protected]>Signed-off-by Kukjin Kim <[email protected]>
[...]
drivers/ata/pata_samsung_cf.c line 1
Quoted 7 lines
drivers/ata/pata_samsung_cf.c new file +1
-0@@ -0,0 +1,608 @@ +/* linux/drivers/ata/pata_samsung_cf.c
File names in the heading comment are discouraged.
[...]
Quoted 6 lines
drivers/ata/pata_samsung_cf.c new file +6
-0+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;
drivers/ata/pata_samsung_cf.c line 86
Quoted 10 lines
drivers/ata/pata_samsung_cf.c new file +10
-0+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()?
Quoted 5 lines
drivers/ata/pata_samsung_cf.c new file +5
-0+ + piotime = (t2i <<12 ) | (t2 <<4 ) | t1; + +return piotime; +}
drivers/ata/pata_samsung_cf.c line 101
Quoted 10 lines
drivers/ata/pata_samsung_cf.c new file +10
-0+ +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...
drivers/ata/pata_samsung_cf.c line 110
Quoted 9 lines
drivers/ata/pata_samsung_cf.c new file +9
-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.
Quoted 4 lines
drivers/ata/pata_samsung_cf.c new file +4
-0+writel (ata_cfg, info->ide_addr +S3C_ATA_CFG ); +writel (piotime, info->ide_addr +S3C_ATA_PIO_TIME ); + } +}
[...]
Quoted 6 lines
drivers/ata/pata_samsung_cf.c new file +6
-0+/* +* 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) +{
drivers/ata/pata_samsung_cf.c line 263
Quoted 10 lines
drivers/ata/pata_samsung_cf.c new file +10
-0+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?
Quoted 12 lines
drivers/ata/pata_samsung_cf.c new file +12
-0+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 ; +}
[...]
drivers/ata/pata_samsung_cf.c line 314
Quoted 10 lines
drivers/ata/pata_samsung_cf.c new file +10
-0+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...
[...]
Quoted 24 lines
drivers/ata/pata_samsung_cf.c new file +24
-0+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;
drivers/ata/pata_samsung_cf.c line 410
Quoted 10 lines
drivers/ata/pata_samsung_cf.c new file +10
-0+ } + + 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 usingdevm_ioremap()...
Quoted 6 lines
drivers/ata/pata_samsung_cf.c new file +6
-0+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) {
drivers/ata/pata_samsung_cf.c line 426
Quoted 10 lines
drivers/ata/pata_samsung_cf.c new file +10
-0+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?
[...]
drivers/ata/pata_samsung_cf.c
Quoted 2 lines
drivers/ata/pata_samsung_cf.c new file +2
-0+stop_clk: +clk_disable (info->clk);
Where is clk_put() call?
drivers/ata/pata_samsung_cf.c line 501
Quoted 4 lines
drivers/ata/pata_samsung_cf.c new file +4
-0+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?
Quoted 1 line
+return ret;
Wrong indentation.
drivers/ata/pata_samsung_cf.c line 580
Quoted 4 lines
drivers/ata/pata_samsung_cf.c new file +2
-0+static struct platform_driver pata_s3c_driver = { + .probe = pata_s3c_probe,
2 empty lines -- broken patch?
drivers/ata/pata_samsung_cf.c line 590
Quoted 10 lines
drivers/ata/pata_samsung_cf.c new file +10
-0+ .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
PATCH v3 1/1 libata: pata_samsung_cf: Add Samsung PATA controller driver
Quoted 38 lines · Sergei Shtylyov
Quoted 7 lines · Kukjin Kim
From: Abhilash Kesavan <[email protected]>
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 <[email protected]>Signed-off-by Kukjin Kim <[email protected]>[...]
drivers/ata/pata_samsung_cf.c line 1
Quoted 7 lines
drivers/ata/pata_samsung_cf.c new file +1
-0@@ -0,0 +1,608 @@ +/* linux/drivers/ata/pata_samsung_cf.c File names in the heading comment are discouraged.
[...]
drivers/ata/pata_samsung_cf.c line 101
Quoted 9 lines
drivers/ata/pata_samsung_cf.c new file +9
-0+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...
Quoted 11 lines
drivers/ata/pata_samsung_cf.c line 110
Quoted 9 lines
drivers/ata/pata_samsung_cf.c new file +9
-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.
probably not even worth bthering with a WARN_ON() for this.
.> + writel(ata_cfg, info->ide_addr + S3C_ATA_CFG);
Quoted 16 lines
Quoted 3 lines
drivers/ata/pata_samsung_cf.c new file +3
-0+writel (piotime, info->ide_addr +S3C_ATA_PIO_TIME ); + } +}[...]
Quoted 10 lines
drivers/ata/pata_samsung_cf.c new file +10
-0+/* +* 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;
Quoted 8 lines
drivers/ata/pata_samsung_cf.c line 263
Quoted 6 lines
drivers/ata/pata_samsung_cf.c new file +6
-0+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);
Quoted 4 lines
drivers/ata/pata_samsung_cf.c line 267
Quoted 4 lines
drivers/ata/pata_samsung_cf.c new file +4
-0+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.
Quoted 5 lines
drivers/ata/pata_samsung_cf.c line 272
Quoted 5 lines
drivers/ata/pata_samsung_cf.c new file +5
-0+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?
Quoted 62 lines
Quoted 2 lines
drivers/ata/pata_samsung_cf.c new file +2
-0+return words <<1 ; +}[...]
drivers/ata/pata_samsung_cf.c line 314
Quoted 10 lines
drivers/ata/pata_samsung_cf.c new file +10
-0+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. Seeata_bus_softreset(),ata_devchk(), andata_sff_wait_after_reset()for the accesses I'm talking about...[...]
Quoted 24 lines
drivers/ata/pata_samsung_cf.c new file +24
-0+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;drivers/ata/pata_samsung_cf.c line 410
Quoted 10 lines
drivers/ata/pata_samsung_cf.c new file +10
-0+ } + + 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 usingdevm_ioremap()...
Quoted 16 lines
Quoted 6 lines
drivers/ata/pata_samsung_cf.c new file +6
-0+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) {drivers/ata/pata_samsung_cf.c line 426
Quoted 10 lines
drivers/ata/pata_samsung_cf.c new file +10
-0+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.
Signature · 4 lines
Ben Q: What's a light-year? A: One-third less calories than a regular year.
PATCH v3 1/1 libata: pata_samsung_cf: Add Samsung PATA controller driver
In reply to Sergei ShtylyovQuoted 7 lines · Sergei Shtylyov
Quoted 4 lines · Kukjin Kim
From: Abhilash Kesavan <[email protected]>
Adds support for the Samsung PATA controller. This driver is based on the Libata subsystem and references the earlier patches sent for IDE
subsystem.
Quoted 1 line
Hi,
Thanks for your comments.
Quoted 6 lines
Quoted 2 lines
Signed-off-by Abhilash Kesavan <[email protected]>Signed-off-by Kukjin Kim <[email protected]>[...]
Review
Quoted 1 line
(quoted diff) +0
-0
b/drivers/ata/pata_samsung_cf.c
Quoted 9 lines
drivers/ata/pata_samsung_cf.c line 1
Quoted 6 lines
new file mode 100644
index 0000000..fef5515drivers/ata/pata_samsung_cf.c new file +1
-0@@ -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.
Quoted 4 lines
[...]
drivers/ata/pata_samsung_cf.c
Quoted 2 lines
drivers/ata/pata_samsung_cf.c new file +2
-0+static unsigned long +pata_s3c_setup_timing(struct s3c_ide_info *info, struct ata_device
*adev)
Quoted 8 lines
Quoted 8 lines
drivers/ata/pata_samsung_cf.c new file +8
-0+{ +const struct ata_timing *timing; +int cycle_time; +int t1; +int t2; +int t2i; +ulong piotime; +
Quoted 9 lines
drivers/ata/pata_samsung_cf.c line 86
Quoted 6 lines
drivers/ata/pata_samsung_cf.c new file +6
-0+ 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.
Quoted 7 lines
drivers/ata/pata_samsung_cf.c
Quoted 7 lines
drivers/ata/pata_samsung_cf.c new file +7
-0+ + piotime = (t2i <<12 ) | (t2 <<4 ) | t1; + +return piotime; +} + +static void pata_s3c_set_piomode(struct ata_port *ap, struct ata_device
*adev)
Quoted 10 lines
drivers/ata/pata_samsung_cf.c line 101
Quoted 8 lines
drivers/ata/pata_samsung_cf.c new file +8
-0+{ +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
Quoted 3 lines
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};Quoted 4 lines
Quoted 4 lines
drivers/ata/pata_samsung_cf.c new file +4
-0+ +/* Enables IORDY if mode requires it */ +if (ata_pio_need_iordy (adev)) + ata_cfg |=S3C_ATA_CFG_IORDYEN ;
Quoted 9 lines
drivers/ata/pata_samsung_cf.c line 110
Quoted 5 lines
drivers/ata/pata_samsung_cf.c new file +5
-0+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.
Quoted 11 lines
Quoted 4 lines
drivers/ata/pata_samsung_cf.c new file +4
-0+writel (ata_cfg, info->ide_addr +S3C_ATA_CFG ); +writel (piotime, info->ide_addr +S3C_ATA_PIO_TIME ); + } +}[...]
drivers/ata/pata_samsung_cf.c
Quoted 4 lines
drivers/ata/pata_samsung_cf.c new file +4
-0+/* +* pata_s3c_data_xfer - Transfer data by PIO +*/ +unsigned int pata_s3c_data_xfer(struct ata_device *dev, unsigned char
*buf,
Quoted 6 lines
Quoted 6 lines
drivers/ata/pata_samsung_cf.c new file +6
-0+ 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;
Quoted 9 lines
drivers/ata/pata_samsung_cf.c line 263
Quoted 6 lines
drivers/ata/pata_samsung_cf.c new file +6
-0+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.
Quoted 15 lines
Quoted 12 lines
drivers/ata/pata_samsung_cf.c new file +12
-0+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 ; +}[...]
Quoted 12 lines
drivers/ata/pata_samsung_cf.c line 314
Quoted 10 lines
drivers/ata/pata_samsung_cf.c new file +10
-0+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
Quoted 1 line
several taskfile registers and ioread8()/iowrite8() won't suffice -- Jeff
was
Quoted 3 lines
too quick to ack the patch. See
ata_bus_softreset(),ata_devchk(), andata_sff_wait_after_reset()for the accesses I'm talking about...
Will add softreset method and the associated functions required.
Quoted 31 lines
[...]
Quoted 29 lines
drivers/ata/pata_samsung_cf.c new file +29
-0+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" );
Quoted 9 lines
drivers/ata/pata_samsung_cf.c line 410
Quoted 5 lines
drivers/ata/pata_samsung_cf.c new file +5
-0+ 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 usingdevm_ioremap()...
Will change to devm_request_mem_region.
Quoted 6 lines
Quoted 6 lines
drivers/ata/pata_samsung_cf.c new file +6
-0+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) {
Quoted 8 lines
drivers/ata/pata_samsung_cf.c
Quoted 8 lines
drivers/ata/pata_samsung_cf.c new file +8
-0+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");
Quoted 5 lines
drivers/ata/pata_samsung_cf.c line 426
Quoted 2 lines
drivers/ata/pata_samsung_cf.c new file +2
-0+ ret =PTR_ERR (info->clk); + info->clk =NULL ;No 'goto' here?
Will add return ret instead of goto.
Quoted 7 lines
[...]
drivers/ata/pata_samsung_cf.c
Quoted 2 lines
drivers/ata/pata_samsung_cf.c new file +2
-0+stop_clk: +clk_disable (info->clk);Where is
clk_put()call?
Will add clk_put
Quoted 8 lines
drivers/ata/pata_samsung_cf.c line 501
Quoted 4 lines
drivers/ata/pata_samsung_cf.c new file +4
-0+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
Quoted 4 lines
Quoted 1 line
+return ret;
Wrong indentation.
OK.
Quoted 7 lines
drivers/ata/pata_samsung_cf.c line 580
Quoted 4 lines
drivers/ata/pata_samsung_cf.c new file +2
-0+static struct platform_driver pata_s3c_driver = { + .probe = pata_s3c_probe,2 empty lines -- broken patch?
Seems OK at my end.
Quoted 13 lines
Quoted 10 lines
drivers/ata/pata_samsung_cf.c new file +10
-0+ .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 + }, +};[...]
Quoted 1 line
MBR, Sergei
Thanks.
Best regards,
Kgene.
--
Kukjin Kim <[email protected]>, Senior Engineer, SW Solution Development Team, Samsung Electronics Co., Ltd.
PATCH v3 1/1 libata: pata_samsung_cf: Add Samsung PATA controller driver
Hello.
Quoted 22 lines · Kukjin Kim
Quoted 3 lines
Quoted 3 lines
From: Abhilash Kesavan <[email protected]>
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.
Quoted 4 lines
Quoted 2 lines
Signed-off-by Abhilash Kesavan <[email protected]>Signed-off-by Kukjin Kim <[email protected]>[...]
Quoted 9 lines
drivers/ata/pata_samsung_cf.c line 1
Quoted 7 lines
drivers/ata/pata_samsung_cf.c new file +1
-0@@ -0,0 +1,608 @@ +/* linux/drivers/ata/pata_samsung_cf.c File names in the heading comment are discouraged.
Quoted 1 line
Hmm. I used like that in other device drivers.
Nevertheless, it's quite an old rule already.
Quoted 24 lines
Ok..will remove the file name in the heading comment.
Quoted 13 lines
[...]
Quoted 11 lines
drivers/ata/pata_samsung_cf.c new file +11
-0+ + 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 );Quoted 8 lines
drivers/ata/pata_samsung_cf.c line 101
Quoted 4 lines
drivers/ata/pata_samsung_cf.c new file +4
-0+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...
Quoted 1 line
Below code looks OK ?
No, it doesn't.
Quoted 11 lines
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_timingis for PIO0. I have added the below structstatic const struct
ata_timinginitial_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 theata_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.
Quoted 16 lines
Quoted 4 lines
Quoted 4 lines
drivers/ata/pata_samsung_cf.c new file +4
-0+ +/* Enables IORDY if mode requires it */ +if (ata_pio_need_iordy (adev)) + ata_cfg |=S3C_ATA_CFG_IORDYEN ;Quoted 8 lines
drivers/ata/pata_samsung_cf.c line 110
Quoted 5 lines
drivers/ata/pata_samsung_cf.c new file +5
-0+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.
Quoted 1 line
Quoted 1 line
+static int
__devinitpata_s3c_probe(struct platform_device *pdev)
[...]
Quoted 6 lines
Quoted 4 lines
drivers/ata/pata_samsung_cf.c line 410
Quoted 1 line
drivers/ata/pata_samsung_cf.c new file +1
-0+if (!request_mem_region (res->start,resource_size (res),DRV_NAME )) {Probably should call
devm_request_mem_region()if you're usingdevm_ioremap()...Will change to
devm_request_mem_region.
This function does exist, if I don't mistake...
Quoted 10 lines
Quoted 7 lines
drivers/ata/pata_samsung_cf.c line 501
Quoted 4 lines
drivers/ata/pata_samsung_cf.c new file +4
-0+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_regionbecause ofdevm_kzallocanddevm_request_mem_regionusage
I don't know devres librarry capabilities well. Tejun, am I right?
Quoted 8 lines
Quoted 6 lines
drivers/ata/pata_samsung_cf.c line 580
Quoted 4 lines
drivers/ata/pata_samsung_cf.c new file +2
-0+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
PATCH v3 1/1 libata: pata_samsung_cf: Add Samsung PATA controller driver
Quoted 7 lines · Sergei Shtylyov
Hello.
Quoted 2 lines · Kukjin Kim
Quoted 2 lines
Quoted 2 lines
From: Abhilash Kesavan <[email protected]>
Adds support for the Samsung PATA controller. This driver is based on
the
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
Libata subsystem and references the earlier patches sent for IDE
subsystem.
Quoted 10 lines
Quoted 10 lines
Hi,
Thanks for your comments.
Quoted 5 lines
Quoted 2 lines
Signed-off-by Abhilash Kesavan <[email protected]>Signed-off-by Kukjin Kim <[email protected]>[...]
Review
Quoted 1 line
(quoted diff) +0
-0
b/drivers/ata/pata_samsung_cf.c
Quoted 17 lines
Quoted 10 lines
Quoted 8 lines
drivers/ata/pata_samsung_cf.c line 1
Quoted 6 lines
new file mode 100644
index 0000000..fef5515drivers/ata/pata_samsung_cf.c new file +1
-0@@ -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.
Quoted 3 lines
Ok..will remove the file name in the heading comment.
Quoted 1 line
[...]
Quoted 7 lines
Quoted 7 lines
Quoted 7 lines
drivers/ata/pata_samsung_cf.c
Quoted 7 lines
drivers/ata/pata_samsung_cf.c new file +7
-0+ + piotime = (t2i <<12 ) | (t2 <<4 ) | t1; + +return piotime; +} + +static void pata_s3c_set_piomode(struct ata_port *ap, struct
ata_device
Quoted 11 lines
*adev)
Quoted 10 lines
Quoted 10 lines
drivers/ata/pata_samsung_cf.c line 101
Quoted 8 lines
drivers/ata/pata_samsung_cf.c new file +8
-0+{ +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
Quoted 2 lines
of
Quoted 1 line
Quoted 1 line
the two drives. However, with CF, you probably only have only one drive
per
Quoted 15 lines
Quoted 3 lines
Quoted 1 line
channel...
Below code looks OK ?
No, it doesn't.
Quoted 8 lines
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_timingis for PIO0. I have added the below struct
Quoted 5 lines
Quoted 2 lines
static const struct
ata_timinginitial_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 theata_timingentry. But really, you shouldn't set any timing for an invalid
mode
Quoted 1 line
and, as I said, you won't be passed one, so there's nor much sense in
calling
Quoted 2 lines
ata_timing_compute()and checking its result; anyway, you'd want to callata_timing_find_mode()here instead ofata_timing_compute()because the
latter
Quoted 1 line
returns already quantized timings, but
initial_timingis not quantized,
you'll
Quoted 2 lines
have to call
ata_timing_compute()inpata_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.
Quoted 17 lines
Quoted 16 lines
Quoted 4 lines
Quoted 4 lines
drivers/ata/pata_samsung_cf.c new file +4
-0+ +/* Enables IORDY if mode requires it */ +if (ata_pio_need_iordy (adev)) + ata_cfg |=S3C_ATA_CFG_IORDYEN ;Quoted 8 lines
drivers/ata/pata_samsung_cf.c line 110
Quoted 5 lines
drivers/ata/pata_samsung_cf.c new file +5
-0+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.
Quoted 1 line
Quoted 1 line
+static int
__devinitpata_s3c_probe(struct platform_device *pdev)[...]
Quoted 15 lines
Quoted 6 lines
Quoted 4 lines
drivers/ata/pata_samsung_cf.c line 410
Quoted 1 line
drivers/ata/pata_samsung_cf.c new file +1
-0+if (!request_mem_region (res->start,resource_size (res),DRV_NAME )) {Probably should call
devm_request_mem_region()if you're usingdevm_ioremap()...Will change to
devm_request_mem_region.This function does exist, if I don't mistake...
Quoted 6 lines
Quoted 6 lines
drivers/ata/pata_samsung_cf.c line 501
Quoted 4 lines
drivers/ata/pata_samsung_cf.c new file +4
-0+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
Quoted 14 lines
Quoted 4 lines
Quoted 1 line
automatically?
Will remove kfree and
release_mem_regionbecause ofdevm_kzallocanddevm_request_mem_regionusageI don't know devres librarry capabilities well. Tejun, am I right?
Quoted 6 lines
Quoted 6 lines
drivers/ata/pata_samsung_cf.c line 580
Quoted 4 lines
drivers/ata/pata_samsung_cf.c new file +2
-0+static struct platform_driver pata_s3c_driver = { + .probe = pata_s3c_probe,2 empty lines -- broken patch?
Quoted 5 lines
Quoted 1 line
Seems OK at my end.
The sent patch had them, nevertheless.
MBR, Sergei
Thanks.
Best regards,
Kgene.
--
Kukjin Kim <[email protected]>, Senior Engineer, SW Solution Development Team, Samsung Electronics Co., Ltd.
PATCH v3 1/1 libata: pata_samsung_cf: Add Samsung PATA controller driver
In reply to Kukjin KimQuoted 7 lines · Kukjin Kim
From: Abhilash Kesavan <[email protected]>
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 <[email protected]>Signed-off-by Kukjin Kim <[email protected]>
a few more comments, you may or may not want to fix before first submission.
Quoted 97 lines
---
Changes since v2:
- Changed theDRV_NAMEtopata_samsung_cf
- Used msleep instead of mdelaydrivers/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.cdrivers/ata/Kconfig +9
-0@@ -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 && EXPERIMENTALdrivers/ata/Makefile +1
-0@@ -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 driverdrivers/ata/pata_samsung_cf.c new file +48
-0@@ -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 {
drivers/ata/pata_samsung_cf.c line 58
Quoted 10 lines
drivers/ata/pata_samsung_cf.c new file +10
-0+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.
Quoted 8 lines
drivers/ata/pata_samsung_cf.c new file +8
-0+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 */
drivers/ata/pata_samsung_cf.c line 110
Quoted 10 lines
drivers/ata/pata_samsung_cf.c new file +10
-0+ 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.
drivers/ata/pata_samsung_cf.c line 119
Quoted 4 lines
drivers/ata/pata_samsung_cf.c new file +4
-0+/* +* 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?
drivers/ata/pata_samsung_cf.c line 127
Quoted 8 lines
drivers/ata/pata_samsung_cf.c new file +8
-0+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)
...Quoted 4 lines
drivers/ata/pata_samsung_cf.c new file +4
-0+return 0 ; + } +return -EBUSY ; +}
[snip]
drivers/ata/pata_samsung_cf.c line 258
Quoted 11 lines
drivers/ata/pata_samsung_cf.c new file +11
-0+/* +* 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?
Quoted 18 lines
drivers/ata/pata_samsung_cf.c new file +18
-0+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 ; +} + +/*
drivers/ata/pata_samsung_cf.c line 287
Quoted 10 lines
drivers/ata/pata_samsung_cf.c new file +10
-0+* 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_DEV1Quoted 4 lines
drivers/ata/pata_samsung_cf.c new file +4
-0+ata_outb (ap->host, tmp, ap->ioaddr.device_addr); +ata_sff_pause (ap); +} +
[snip]
Quoted 1 line
+static void
pata_s3c_enable(void *s3c_ide_regbase, u8 state)
state would be better as a bool or a natural int.
Quoted 4 lines
drivers/ata/pata_samsung_cf.c new file +4
-0+{ +u32 temp =readl (s3c_ide_regbase +S3C_ATA_CTRL ); + temp = state ? (temp |1 ) : (temp & ~1 ); +writel (temp, s3c_ide_regbase +S3C_ATA_CTRL );
drivers/ata/pata_samsung_cf.c line 337
Quoted 10 lines
drivers/ata/pata_samsung_cf.c new file +10
-0+} + +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?
drivers/ata/pata_samsung_cf.c line 512
Quoted 12 lines
drivers/ata/pata_samsung_cf.c new file +10
-0+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.
Quoted 12 lines
drivers/ata/pata_samsung_cf.c new file +12
-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 ; +}
drivers/ata/pata_samsung_cf.c line 534
Quoted 10 lines
drivers/ata/pata_samsung_cf.c new file +10
-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.
Quoted 25 lines
drivers/ata/pata_samsung_cf.c new file +25
-0+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, +};
Signature · 4 lines
Ben Q: What's a light-year? A: One-third less calories than a regular year.
PATCH v3 1/1 libata: pata_samsung_cf: Add Samsung PATA controller driver
Quoted 6 lines · Ben Dooks
Quoted 4 lines · Kukjin Kim
From: Abhilash Kesavan <[email protected]>
Adds support for the Samsung PATA controller. This driver is based on the Libata subsystem and references the earlier patches sent for IDE
subsystem.
Quoted 1 line
Quoted 1 line
Thanks for your review. :-)
Quoted 44 lines
Quoted 2 lines
Signed-off-by Abhilash Kesavan <[email protected]>Signed-off-by Kukjin Kim <[email protected]>a few more comments, you may or may not want to fix before first submission.
Quoted 8 lines
---
Changes since v2:
- Changed theDRV_NAMEtopata_samsung_cf
- Used msleep instead of mdelaydrivers/ata/Makefile | 1 + drivers/ata/pata_samsung_cf.c | 608+++++++++++++++++++++++++++++++++++++++++
drivers/ata/Makefile obj-$(CONFIG_PATA_OF_PLATFORM) +=
Quoted 28 lines
3 files changed, 618 insertions(+), 0 deletions(-) create mode 100644 drivers/ata/pata_samsung_cf.cdrivers/ata/Kconfig +9
-0@@ -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 && EXPERIMENTALdrivers/ata/Makefile +0
-0@@ -87,6 +87,7 @@ obj-$(CONFIG_PATA_OF_PLATFORM) +=pata_of_platform.o
Quoted 8 lines
Review
Quoted 8 lines
drivers/ata/Makefile +1
-0obj-$(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(quoted diff) +0
-0
b/drivers/ata/pata_samsung_cf.c
Quoted 4 lines
Quoted 4 lines
new file mode 100644
index 0000000..fef5515drivers/ata/pata_samsung_cf.c new file +0
-0
Quoted 8 lines
drivers/ata/pata_samsung_cf.c line 7
Quoted 8 lines
drivers/ata/pata_samsung_cf.c new file +7
-0@@ -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
Quoted 7 lines
drivers/ata/pata_samsung_cf.c
Quoted 7 lines
drivers/ata/pata_samsung_cf.c new file +7
-0+* 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
Quoted 37 lines
Quoted 37 lines
drivers/ata/pata_samsung_cf.c new file +37
-0+* 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;
Quoted 9 lines
drivers/ata/pata_samsung_cf.c line 58
Quoted 7 lines
drivers/ata/pata_samsung_cf.c new file +7
-0+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.
Quoted 1 line
drivers/ata/pata_samsung_cf.c line 59
Quoted 1 line
drivers/ata/pata_samsung_cf.c new file +1
-0+static void pata_s3c_set_piomode(struct ata_port *ap, struct ata_device
*adev)
Quoted 15 lines
Quoted 15 lines
drivers/ata/pata_samsung_cf.c new file +15
-0+{ +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 ; +
Quoted 9 lines
drivers/ata/pata_samsung_cf.c line 110
Quoted 2 lines
drivers/ata/pata_samsung_cf.c new file +2
-0+/* 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.
Quoted 8 lines
drivers/ata/pata_samsung_cf.c line 119
Quoted 4 lines
drivers/ata/pata_samsung_cf.c new file +4
-0+/* +* 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.
Quoted 8 lines
drivers/ata/pata_samsung_cf.c
Quoted 8 lines
drivers/ata/pata_samsung_cf.c new file +8
-0+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)
Quoted 9 lines
would be neater to have
void __iomem *fifo_reg = info->ide_addr + info->fifo_status_reg ... while(...) { if (readl(fifo_reg) >> 28) == 0) ...
OK.
Quoted 11 lines
Quoted 4 lines
drivers/ata/pata_samsung_cf.c new file +4
-0+return 0 ; + } +return -EBUSY ; +}[snip]
drivers/ata/pata_samsung_cf.c
Quoted 4 lines
drivers/ata/pata_samsung_cf.c new file +4
-0+/* +* pata_s3c_data_xfer - Transfer data by PIO +*/ +unsigned int pata_s3c_data_xfer(struct ata_device *dev, unsigned char
*buf,
Quoted 11 lines
drivers/ata/pata_samsung_cf.c line 258
Quoted 7 lines
drivers/ata/pata_samsung_cf.c new file +7
-0+ 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.
Quoted 8 lines
drivers/ata/pata_samsung_cf.c line 267
Quoted 8 lines
drivers/ata/pata_samsung_cf.c new file +8
-0+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.
Quoted 5 lines
Quoted 5 lines
drivers/ata/pata_samsung_cf.c new file +5
-0+else +for (i =0 ; i < words; i++, data_ptr++) { +wait_for_host_ready (info); +writel (*data_ptr, data_addr); + }
Quoted 8 lines
drivers/ata/pata_samsung_cf.c
Quoted 8 lines
drivers/ata/pata_samsung_cf.c new file +8
-0+ +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)
Quoted 21 lines
drivers/ata/pata_samsung_cf.c line 287
Quoted 7 lines
drivers/ata/pata_samsung_cf.c new file +7
-0+{ +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_DEV1Quoted 4 lines
drivers/ata/pata_samsung_cf.c new file +4
-0+ata_outb (ap->host, tmp, ap->ioaddr.device_addr); +ata_sff_pause (ap); +} +[snip]
Quoted 2 lines
Quoted 1 line
+static void
pata_s3c_enable(void *s3c_ide_regbase, u8 state)state would be better as a bool or a natural int.
OK.
Quoted 8 lines
Quoted 8 lines
drivers/ata/pata_samsung_cf.c new file +8
-0+{ +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) +{
Quoted 9 lines
drivers/ata/pata_samsung_cf.c line 337
Quoted 6 lines
drivers/ata/pata_samsung_cf.c new file +6
-0+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.
Quoted 9 lines
Quoted 8 lines
drivers/ata/pata_samsung_cf.c new file +7
-0+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;
Quoted 6 lines
drivers/ata/pata_samsung_cf.c line 512
Quoted 3 lines
drivers/ata/pata_samsung_cf.c new file +3
-0+ +if (!host) +return 0 ;don't think this check is really necessary.
OK.
Quoted 16 lines
Quoted 16 lines
drivers/ata/pata_samsung_cf.c new file +16
-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) +{
Quoted 9 lines
drivers/ata/pata_samsung_cf.c line 534
Quoted 6 lines
drivers/ata/pata_samsung_cf.c new file +6
-0+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.
Quoted 19 lines
Quoted 19 lines
drivers/ata/pata_samsung_cf.c new file +19
-0+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 ;
Quoted 8 lines
drivers/ata/pata_samsung_cf.c line 559
Quoted 6 lines
drivers/ata/pata_samsung_cf.c new file +6
-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 <[email protected]>, Senior Engineer, SW Solution Development Team, Samsung Electronics Co., Ltd.