RE: [PATCH 4/4] pata_samsung: Add Samsung PATA controller driver
Sergei Shtylyov wrote:
>
> Hello.
Hi ;-)
> Kukjin Kim wrote:
>
> >>> From: Abhilash Kesavan
> >>> Adds support for the Samsung PATA controller. This driver is based on
the
> >>> Libata subsystem and references the earlier patches sent for IDE
subsystem.
>
> Where I those I wonder? :-)
> It's a pity they didn't get accepted.
>
Was told that that only new libata based drivers would be accepted.
> >>> Signed-off-by: Abhilash Kesavan
> >>> Signed-off-by: Kukjin Kim
>
> [...]
>
> >>> + for (i = 0; i < words; i++, temp_addr++) {
> >>> + wait_for_host_ready(info);
> >>> + writel(*temp_addr, data_addr);
> >>> + }
> >>> + }
>
> >> Well, if this is pere CF case, 'buflen' can't be odd, but otherwise
>
> I meant to type "pure". :-)
>
considering CF in true-ide mode..should I still add the check?
> >> you should handle that case...
>
> >>> + uint pio_teoc[5] = { 240, 43, 10, 70, 25 };
>
> >> What timing is this? :-O
>
> > Should have been t2i rec.
>
> If it's t2i timing, it is incorrect for the first 3 PIO modes.
>
Should have been (240, 93, 40, 70, 25)
> >>> + if (cpu_type == TYPE_S3C6400) {
> >>> + ap->ops = &pata_s3c_port_ops;
> >>> + info->sfr_addr = info->ide_addr + 0x1800;
> >>> + info->ide_addr = info->ide_addr + 0x1900;
> >>> + info->fifo_status_reg = 0x94;
> >>> + } else if (cpu_type == TYPE_S5PC100) {
> >>> + ap->ops = &pata_s5p_port_ops;
> >>> + info->sfr_addr = info->ide_addr + 0x1800;
> >>> + info->ide_addr = info->ide_addr + 0x1900;
>
> >> Does make sense to assign those before *if*.
>
> > Offsets not required for S5PV210/S5PC110.
>
> pata_s3c_tune_chipset() certainly requires them.
>
yes..but for V210/C110 just the ioremapped info->ide_addr with 0 offset is
enough.
> >>> + info->fifo_status_reg = 0x84;
> >>> + } else {
> >>> + ap->ops = &pata_s5p_port_ops;
>
> >> You don't assign 'info->ide_addr' here but you'll need it in
> >> pata_s3c_tune_chipset() which will be called thru pata_s5p_port_ops!
>
> > The address received at ioremap() is enough for S5PV210/S5PC110..no
offset
> > needed.
>
> I only have to restate that pata_s3c_tune_chipset() does use
> 'info->ide_addr'. Maybe you shouldn't install this method for
S5PV210/S5PC110?
> Or do you mean thgat offset of 0 will work?
>
V210/C110 requires 0 offset as compared to 6410 and C100 which require
0x1800.
> >>> +release_mem:
> >>> + release_mem_region(res->start, res->end - res->start + 1);
>
> >> But you didn't call request_mem_region()!
>
> > I didn't..will remove.
>
> But you should call request_mem_region() in one or another form...
>
OK..will request mem_region() before ioremap()..and then the release_mem's
should be fine.
Thanks.
Best regards,
Kgene.
--
Kukjin Kim , Senior Engineer,
SW Solution Development Team, Samsung Electronics Co., Ltd.
--
To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 4/4] pata_samsung: Add Samsung PATA controller driver
Hello.
Kukjin Kim wrote:
From: Abhilash Kesavan
Adds support for the Samsung PATA controller. This driver is based on the
Libata subsystem and references the earlier patches sent for IDE subsystem.
Where I those I wonder? :-)
It's a pity they didn't get accepted.
Signed-off-by: Abhilash Kesavan
Signed-off-by: Kukjin Kim
[...]
+ for (i = 0; i < words; i++, temp_addr++) {
+ wait_for_host_ready(info);
+ writel(*temp_addr, data_addr);
+ }
+ }
Well, if this is pere CF case, 'buflen' can't be odd, but otherwise
I meant to type "pure". :-)
you should handle that case...
+ uint pio_teoc[5] = { 240, 43, 10, 70, 25 };
What timing is this? :-O
Should have been t2i rec.
If it's t2i timing, it is incorrect for the first 3 PIO modes.
+ if (cpu_type == TYPE_S3C6400) {
+ ap->ops = &pata_s3c_port_ops;
+ info->sfr_addr = info->ide_addr + 0x1800;
+ info->ide_addr = info->ide_addr + 0x1900;
+ info->fifo_status_reg = 0x94;
+ } else if (cpu_type == TYPE_S5PC100) {
+ ap->ops = &pata_s5p_port_ops;
+ info->sfr_addr = info->ide_addr + 0x1800;
+ info->ide_addr = info->ide_addr + 0x1900;
Does make sense to assign those before *if*.
Offsets not required for S5PV210/S5PC110.
pata_s3c_tune_chipset() certainly requires them.
+ info->fifo_status_reg = 0x84;
+ } else {
+ ap->ops = &pata_s5p_port_ops;
You don't assign 'info->ide_addr' here but you'll need it in
pata_s3c_tune_chipset() which will be called thru pata_s5p_port_ops!
The address received at ioremap() is enough for S5PV210/S5PC110..no offset
needed.
I only have to restate that pata_s3c_tune_chipset() does use
'info->ide_addr'. Maybe you shouldn't install this method for S5PV210/S5PC110?
Or do you mean thgat offset of 0 will work?
+release_mem:
+ release_mem_region(res->start, res->end - res->start + 1);
But you didn't call request_mem_region()!
I didn't..will remove.
But you should call request_mem_region() in one or another form...
MBR, Sergei
--
To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [PATCH 4/4] pata_samsung: Add Samsung PATA controller driver
Tejun Heo wrote:
>
> Hello,
>
> On 05/27/2010 10:22 AM, Kukjin Kim wrote:
> > From: Abhilash Kesavan
> >
> > Adds support for the Samsung PATA controller. This driver is based on the
> > Libata subsystem and references the earlier patches sent for IDE
subsystem.
>
> Just one small thing.
>
> > +static struct ata_port_operations pata_s3c_port_ops = {
> > + .inherits = &ata_sff_port_ops,
> > + .sff_check_status = pata_s3c_check_status,
> > + .sff_tf_load= pata_s3c_tf_load,
> > + .sff_tf_read= pata_s3c_tf_read,
> > + .sff_data_xfer = pata_s3c_data_xfer,
> > + .sff_exec_command = pata_s3c_exec_command,
> > + .qc_prep= ata_noop_qc_prep,
> > + .set_piomode= pata_s3c_set_piomode,
> > +};
> > +
> > +static struct ata_port_operations pata_s5p_port_ops = {
> > + .inherits = &ata_sff_port_ops,
> > + .qc_prep= ata_noop_qc_prep,
> > + .set_piomode= pata_s3c_set_piomode,
> > +};
>
> You don't need to override .qc_prep to ata_noop_qc_prep() and can you
> please base the patch against the current libata-dev#upstream?
>
Will remove the override and rebase the new patches against upstream branch.
Thanks.
Best regards,
Kgene.
--
Kukjin Kim , Senior Engineer,
SW Solution Development Team, Samsung Electronics Co., Ltd.
--
To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [PATCH 4/4] pata_samsung: Add Samsung PATA controller driver
Sergei Shtylyov wrote:
>
> Hello.
>
Hi :-)
> Kukjin Kim wrote:
>
> > From: Abhilash Kesavan
>
> > 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
> > Signed-off-by: Kukjin Kim
>
> > diff --git a/drivers/ata/pata_samsung.c b/drivers/ata/pata_samsung.c
> > new file mode 100644
> > index 000..14381e4
> > --- /dev/null
> > +++ b/drivers/ata/pata_samsung.c
> > @@ -0,0 +1,591 @@
> [...]
> > +static void pata_s3c_tune_chipset(struct s3c_ide_info *info, u8
ide_mode)
> > +{
> > + ulong ata_cfg = readl(info->ide_addr + S3C_ATA_CFG);
> > +
> > + /* IORDY is enabled for modes > PIO2 */
> > + if (XFER_PIO_0 <= ide_mode && ide_mode <= XFER_PIO_4) {
> > +
> > + switch (ide_mode) {
> > + case XFER_PIO_0:
> > + case XFER_PIO_1:
> > + case XFER_PIO_2:
> > + ata_cfg &= (~S3C_ATA_CFG_IORDYEN);
>
> Useless parens.
>
OK
> > + break;
> > + case XFER_PIO_3:
> > + case XFER_PIO_4:
> > + ata_cfg |= S3C_ATA_CFG_IORDYEN;
>
> IORDY should be controlled by ata_id_pio_need_iordy(), not by mode
> number. PIO2 also can have IODRY enabled.
>
Will use ata_id_pio_need_iordy().
> > + break;
> > + }
> > +
> > + writel(ata_cfg, info->ide_addr + S3C_ATA_CFG);
> > + writel(piotime[ide_mode - XFER_PIO_0],
> > + info->ide_addr + S3C_ATA_PIO_TIME);
> > + } else {
> > + /* Default to PIO0 */
> > + ata_cfg &= (~S3C_ATA_CFG_IORDYEN);
> > + writel(ata_cfg, info->ide_addr + S3C_ATA_CFG);
> > + writel(piotime[0], info->ide_addr + S3C_ATA_PIO_TIME);
>
> If you don't support DMA modes or modes higher than PIO4 (PIO5 and
> PIO6 would have been good for CF though), just do nothing.
>
OK
> > + }
> > +}
> > +
> > +static void
> > +pata_s3c_set_piomode(struct ata_port *ap, struct ata_device *adev)
> > +{
> > + struct s3c_ide_info *info = ap->host->private_data;
> > +
> > + /* Configure IORDY based on PIO mode and also set the timing value */
> > + pata_s3c_tune_chipset(info, adev->pio_mode);
>
> Don't see why you need a separate function for that. Maybe in
> preparation to UDMA support?
>
That was it..but will remove pata_s3c_set_piomode() and call
pata_s3c_tune_chipset() directly.
> > +/*
> > + * Writes to one of the task file registers.
> > + */
> > +static void ata_outb(struct ata_host *host, u8 addr, void __iomem *reg)
> > +{
> > + struct s3c_ide_info *info = host->private_data;
> > +
> > + wait_for_host_ready(info);
> > + __raw_writeb(addr, reg);
>
> You should use write?() or __raw_write?() consistently...
>
OK
> > +/*
> > + * pata_s3c_tf_load - send taskfile registers to host controller
> > + */
> > +static void pata_s3c_tf_load(struct ata_port *ap,
> > + const struct ata_taskfile *tf)
> > +{
> > + struct ata_ioports *ioaddr = &ap->ioaddr;
> > + unsigned int is_addr = tf->flags & ATA_TFLAG_ISADDR;
> > +
> > + if (tf->ctl != ap->last_ctl) {
> > + if (ioaddr->ctl_addr)
>
> This register does exist and you assign ioaddr->ctl_addr in
> pata_s3c_probe(), hence the check is pointless.
>
Will remove the check.
> > + ata_outb(ap->host, tf->ctl, ioaddr->ctl_addr);
> > + ap->last_ctl = tf->ctl;
> > + ata_wait_idle(ap);
> > + }
> > +
> > + if (is_addr && (tf->flags & ATA_TFLAG_LBA48)) {
> > + WARN_ON_ONCE(!ioaddr->ctl_addr);
>
> You don't need this either.
>
OK
> > +/*
> > + * pata_s3c_tf_read - input device's ATA taskfile shadow registers
> > + */
> > +static void pata_s3c_tf_read(struct ata_port *ap, struct ata_taskfile
*tf)
> > +{
> > + struct ata_ioports *ioaddr = &ap->ioaddr;
> > +
> > + tf->command = ata_sff_check_status(ap);
>
> But you have overriden this method, so ata_sff_check_status()
> shouldn't work!
>
OK
> > + tf->feature = ata_inb(ap->host, ioaddr->error_addr);
> > + tf->nsect = ata_inb(ap->host, ioaddr->nsect_addr);
> > + tf->lbal = ata_inb(ap->host, ioaddr->lbal_addr);
> > + tf->lbam = ata_inb(ap->host, ioaddr->lbam_addr);
> > + tf->lbah = ata_inb(ap->host, ioaddr->lbah_addr);
> > + tf->device = ata_inb(ap->host, ioaddr->device_addr);
> > +
> > + if (tf->flags & ATA_TFLAG_LBA48) {
> > + if (likely(ioaddr->ctl_addr)) {
>
> Not just likely, it's always true. Emilinate the check please.
>
OK
> > + iowrite8(tf->ctl | ATA_HOB, ioaddr->ctl_addr);
> > + tf->hob_feature = ata_inb(ap->host,
ioaddr->error_addr);
> > + tf->hob_nsect = ata_inb(ap->host,
ioaddr->nsect_addr);
> > + tf->hob_lbal = ata_inb(ap->host, ioaddr->lbal_addr);
> > + tf->hob_lbam = ata_inb(ap->host, ioaddr->lbam_addr);
> > +
RE: [PATCH 4/4] pata_samsung: Add Samsung PATA controller driver
Ben Dooks wrote:
>
> On Thu, May 27, 2010 at 05:22:04PM +0900, Kukjin Kim wrote:
> > From: Abhilash Kesavan
> >
> > 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
> > Signed-off-by: Kukjin Kim
> > ---
> > drivers/ata/Kconfig|9 +
> > drivers/ata/Makefile |1 +
> > drivers/ata/pata_samsung.c | 591
>
> > 3 files changed, 601 insertions(+), 0 deletions(-)
> > create mode 100644 drivers/ata/pata_samsung.c
> >
> > diff --git a/drivers/ata/Kconfig b/drivers/ata/Kconfig
> > index e68541f..ce40f66 100644
> > --- a/drivers/ata/Kconfig
> > +++ b/drivers/ata/Kconfig
> > @@ -640,6 +640,15 @@ config PATA_RZ1000
> >
> > If unsure, say N.
> >
> > +config PATA_SAMSUNG
> > + tristate "Samsung SoC PATA support"
> > + depends on S3C_DEV_IDE
> > + help
> > + This option enables basic support for Samsung's S3C/S5P board
> > + PATA controllers via the new ATA layer
> > +
> > + If unsure, say N.
> > +
> > config PATA_SC1200
> > tristate "SC1200 PATA support"
> > depends on PCI
> > diff --git a/drivers/ata/Makefile b/drivers/ata/Makefile
> > index d0a93c4..0b6d29a 100644
> > --- a/drivers/ata/Makefile
> > +++ b/drivers/ata/Makefile
> > @@ -62,6 +62,7 @@ obj-$(CONFIG_PATA_RADISYS)+= pata_radisys.o
> > obj-$(CONFIG_PATA_RB532) += pata_rb532_cf.o
> > obj-$(CONFIG_PATA_RDC) += pata_rdc.o
> > obj-$(CONFIG_PATA_RZ1000) += pata_rz1000.o
> > +obj-$(CONFIG_PATA_SAMSUNG) += pata_samsung.o
> do you want to call it pata_samsung_soc.o instead, in case samsung make
> otehr types of pata controller?
Will change the name to pata_samsung_cf.
> > obj-$(CONFIG_PATA_SC1200) += pata_sc1200.o
> > obj-$(CONFIG_PATA_SERVERWORKS) += pata_serverworks.o
> > obj-$(CONFIG_PATA_SIL680) += pata_sil680.o
> > diff --git a/drivers/ata/pata_samsung.c b/drivers/ata/pata_samsung.c
> > new file mode 100644
> > index 000..14381e4
> > --- /dev/null
> > +++ b/drivers/ata/pata_samsung.c
> > @@ -0,0 +1,591 @@
> > +/* linux/drivers/ata/pata_samsung.c
> > + *
> > + * Copyright (c) 2010 Samsung Electronics Co., Ltd.
> > + * http://www.samsung.com
> > + *
> > + * PATA driver for Samsung SoC's.
> SoCs.
OK
> > + * Supports CF Interface in True IDE mode. Currently only PIO mode has
been
> > + * implemented; UDMA support has to be added.
> > + *
> > + * Based on:
> > + * PATA driver for AT91SAM9260 Static Memory Controller
> > + * PATA driver for Toshiba SCC controller
> > + *
> > + * This program is free software; you can redistribute it and/or modify
it
> > + * under the terms of the GNU General Public License version 2
> > + * as published by the Free Software Foundation.
> > +*/
> > +
> > +#include
> > +#include
> > +#include
> > +#include
> > +#include
> > +#include
> > +#include
> > +
> > +#include
> > +#include
> > +
> > +#define DRV_NAME "pata_samsung"
> > +#define DRV_VERSION "0.1"
> > +
> > +enum s3c_cpu_type {
> > + TYPE_S3C6400,
> > + TYPE_S5PC100,
> > + TYPE_S5PV210,
> > +};
> > +
> > +/*
> > + * struct s3c_ide_info - S3C PATA instance.
> > + * @clk: The clock resource for this controller.
> > + * @ide_addr: The area mapped for the hardware registers.
> > + * @sfr_addr: The area mapped for the special function registers.
> > + * @irq: The IRQ number we are using.
> > + * @cpu_type: The exact type of this controller.
> > + * @fifo_status_reg: The ATA_FIFO_STATUS register offset.
> > + */
> > +struct s3c_ide_info {
> > + struct clk *clk;
> > + void __iomem *ide_addr;
> > + void __iomem *sfr_addr;
> > + unsigned int irq;
> > + enum s3c_cpu_type cpu_type;
> > + unsigned int fifo_status_reg;
> > +};
> > +
> > +static unsigned long piotime[5];
> > +
> > +static void pata_s3c_set_endian(void *s3c_ide_regbase, u8 mode)
> > +{
> > + u32 reg = readl(s3c_ide_regbase + S3C_ATA_CFG);
> > + reg = mode ? (reg & ~S3C_ATA_CFG_SWAP) : (reg |
> S3C_ATA_CFG_SWAP);
> > + writel(reg, s3c_ide_regbase + S3C_ATA_CFG);
> > +}
> > +
> > +static void pata_s3c_tune_chipset(struct s3c_ide_info *info, u8
ide_mode)
> > +{
> > + ulong ata_cfg = readl(info->ide_addr + S3C_ATA_CFG);
> > +
> > + /* IORDY is enabled for modes > PIO2 */
> > + if (XFER_PIO_0 <= ide_mode && ide_mode <= XFER_PIO_4) {
> > +
> > + switch (ide_mode) {
> > + case XFER_PIO_0:
> > + case XFER_PIO_1:
> > + case XFER_PIO_2:
> > + ata_cfg &= (~S3C_ATA_CFG_IORDYEN);
> > + break;
> > + case XFER_PIO_3:
> > + case XFER_PIO_4:
> > + ata_cfg |= S3C_ATA_CFG_IORDYEN;
> > + break;
> > + }
> > +
> > + writel(ata_cfg, info->ide_addr + S3C_ATA_CFG);
> > + writel(piotime[ide_mode - XFER_PIO_0],
> > + info->ide_addr + S3C_ATA_PIO_
Re: [PATCH 4/4] pata_samsung: Add Samsung PATA controller driver
Hello,
On 05/27/2010 10:22 AM, Kukjin Kim wrote:
> From: Abhilash Kesavan
>
> Adds support for the Samsung PATA controller. This driver is based on the
> Libata subsystem and references the earlier patches sent for IDE subsystem.
Just one small thing.
> +static struct ata_port_operations pata_s3c_port_ops = {
> + .inherits = &ata_sff_port_ops,
> + .sff_check_status = pata_s3c_check_status,
> + .sff_tf_load= pata_s3c_tf_load,
> + .sff_tf_read= pata_s3c_tf_read,
> + .sff_data_xfer = pata_s3c_data_xfer,
> + .sff_exec_command = pata_s3c_exec_command,
> + .qc_prep= ata_noop_qc_prep,
> + .set_piomode= pata_s3c_set_piomode,
> +};
> +
> +static struct ata_port_operations pata_s5p_port_ops = {
> + .inherits = &ata_sff_port_ops,
> + .qc_prep= ata_noop_qc_prep,
> + .set_piomode= pata_s3c_set_piomode,
> +};
You don't need to override .qc_prep to ata_noop_qc_prep() and can you
please base the patch against the current libata-dev#upstream?
Thanks.
--
tejun
--
To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 4/4] pata_samsung: Add Samsung PATA controller driver
Hello.
Kukjin Kim wrote:
From: Abhilash Kesavan
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
Signed-off-by: Kukjin Kim
diff --git a/drivers/ata/pata_samsung.c b/drivers/ata/pata_samsung.c
new file mode 100644
index 000..14381e4
--- /dev/null
+++ b/drivers/ata/pata_samsung.c
@@ -0,0 +1,591 @@
[...]
+static void pata_s3c_tune_chipset(struct s3c_ide_info *info, u8 ide_mode)
+{
+ ulong ata_cfg = readl(info->ide_addr + S3C_ATA_CFG);
+
+ /* IORDY is enabled for modes > PIO2 */
+ if (XFER_PIO_0 <= ide_mode && ide_mode <= XFER_PIO_4) {
+
+ switch (ide_mode) {
+ case XFER_PIO_0:
+ case XFER_PIO_1:
+ case XFER_PIO_2:
+ ata_cfg &= (~S3C_ATA_CFG_IORDYEN);
Useless parens.
+ break;
+ case XFER_PIO_3:
+ case XFER_PIO_4:
+ ata_cfg |= S3C_ATA_CFG_IORDYEN;
IORDY should be controlled by ata_id_pio_need_iordy(), not by mode
number. PIO2 also can have IODRY enabled.
+ break;
+ }
+
+ writel(ata_cfg, info->ide_addr + S3C_ATA_CFG);
+ writel(piotime[ide_mode - XFER_PIO_0],
+ info->ide_addr + S3C_ATA_PIO_TIME);
+ } else {
+ /* Default to PIO0 */
+ ata_cfg &= (~S3C_ATA_CFG_IORDYEN);
+ writel(ata_cfg, info->ide_addr + S3C_ATA_CFG);
+ writel(piotime[0], info->ide_addr + S3C_ATA_PIO_TIME);
If you don't support DMA modes or modes higher than PIO4 (PIO5 and
PIO6 would have been good for CF though), just do nothing.
+ }
+}
+
+static void
+pata_s3c_set_piomode(struct ata_port *ap, struct ata_device *adev)
+{
+ struct s3c_ide_info *info = ap->host->private_data;
+
+ /* Configure IORDY based on PIO mode and also set the timing value */
+ pata_s3c_tune_chipset(info, adev->pio_mode);
Don't see why you need a separate function for that. Maybe in
preparation to UDMA support?
+/*
+ * Writes to one of the task file registers.
+ */
+static void ata_outb(struct ata_host *host, u8 addr, void __iomem *reg)
+{
+ struct s3c_ide_info *info = host->private_data;
+
+ wait_for_host_ready(info);
+ __raw_writeb(addr, reg);
You should use write?() or __raw_write?() consistently...
+/*
+ * pata_s3c_tf_load - send taskfile registers to host controller
+ */
+static void pata_s3c_tf_load(struct ata_port *ap,
+ const struct ata_taskfile *tf)
+{
+ struct ata_ioports *ioaddr = &ap->ioaddr;
+ unsigned int is_addr = tf->flags & ATA_TFLAG_ISADDR;
+
+ if (tf->ctl != ap->last_ctl) {
+ if (ioaddr->ctl_addr)
This register does exist and you assign ioaddr->ctl_addr in
pata_s3c_probe(), hence the check is pointless.
+ ata_outb(ap->host, tf->ctl, ioaddr->ctl_addr);
+ ap->last_ctl = tf->ctl;
+ ata_wait_idle(ap);
+ }
+
+ if (is_addr && (tf->flags & ATA_TFLAG_LBA48)) {
+ WARN_ON_ONCE(!ioaddr->ctl_addr);
You don't need this either.
+/*
+ * pata_s3c_tf_read - input device's ATA taskfile shadow registers
+ */
+static void pata_s3c_tf_read(struct ata_port *ap, struct ata_taskfile *tf)
+{
+ struct ata_ioports *ioaddr = &ap->ioaddr;
+
+ tf->command = ata_sff_check_status(ap);
But you have overriden this method, so ata_sff_check_status()
shouldn't work!
+ tf->feature = ata_inb(ap->host, ioaddr->error_addr);
+ tf->nsect = ata_inb(ap->host, ioaddr->nsect_addr);
+ tf->lbal = ata_inb(ap->host, ioaddr->lbal_addr);
+ tf->lbam = ata_inb(ap->host, ioaddr->lbam_addr);
+ tf->lbah = ata_inb(ap->host, ioaddr->lbah_addr);
+ tf->device = ata_inb(ap->host, ioaddr->device_addr);
+
+ if (tf->flags & ATA_TFLAG_LBA48) {
+ if (likely(ioaddr->ctl_addr)) {
Not just likely, it's always true. Emilinate the check please.
+ iowrite8(tf->ctl | ATA_HOB, ioaddr->ctl_addr);
+ tf->hob_feature = ata_inb(ap->host, ioaddr->error_addr);
+ tf->hob_nsect = ata_inb(ap->host, ioaddr->nsect_addr);
+ tf->hob_lbal = ata_inb(ap->host, ioaddr->lbal_addr);
+ tf->hob_lbam = ata_inb(ap->host, ioaddr->lbam_addr);
+ tf->hob_lbah = ata_inb(ap->host, ioaddr->lbah_addr);
+ iowrite8(tf->ctl, ioaddr->ctl_addr);
+ ap->last_ctl = tf->ctl;
+ } else
+ WARN_ON_ONCE(1);
Not needed.
+/*
+ * pata_s3c_data_xfer - Transfer data by PIO
+ */
+unsigned int pata_s3c_data_xfer(struct ata_device *dev, unsigned char *buf,
+ unsign
Re: [PATCH 4/4] pata_samsung: Add Samsung PATA controller driver
On Thu, May 27, 2010 at 5:57 PM, Ben Dooks wrote: > On Thu, May 27, 2010 at 05:43:47PM +0900, Jassi Brar wrote: >> On Thu, May 27, 2010 at 5:22 PM, Kukjin Kim wrote: >> > From: Abhilash Kesavan >> > >> > 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 >> > Signed-off-by: Kukjin Kim >> > --- >> > drivers/ata/Kconfig | 9 + >> > drivers/ata/Makefile | 1 + >> > drivers/ata/pata_samsung.c | 591 >> >> Fasten your seat belts before reading further >> >> Rather than generic 'samsung', I would suggest the driver named >> after the SoC, that is supported first(chronologically) in mainline kernel. >> All newer SoCs should be simply taken to contain the controller of that SoC. >> Otherwise, the same naming problem comes back to haunt us should >> Samsung decides to use a different IP in future SoCs. What would we >> call that driver? pata_samsung_v2.c ? > > I'm not so bothered, but it could be pata_samsung_cfcon or anything, > a new block could be called pata_samsung_v2 or fred well, then pata_samsung.c is better for we know chances of this IP change in future SoCs are quite slim. And even if it does change we shall call it pata_samsung_really_final_this_time.c ;) -- To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 4/4] pata_samsung: Add Samsung PATA controller driver
On Thu, May 27, 2010 at 05:43:47PM +0900, Jassi Brar wrote: > On Thu, May 27, 2010 at 5:22 PM, Kukjin Kim wrote: > > From: Abhilash Kesavan > > > > 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 > > Signed-off-by: Kukjin Kim > > --- > > drivers/ata/Kconfig | 9 + > > drivers/ata/Makefile | 1 + > > drivers/ata/pata_samsung.c | 591 > > Fasten your seat belts before reading further > > Rather than generic 'samsung', I would suggest the driver named > after the SoC, that is supported first(chronologically) in mainline kernel. > All newer SoCs should be simply taken to contain the controller of that SoC. > Otherwise, the same naming problem comes back to haunt us should > Samsung decides to use a different IP in future SoCs. What would we > call that driver? pata_samsung_v2.c ? I'm not so bothered, but it could be pata_samsung_cfcon or anything, a new block could be called pata_samsung_v2 or fred for all I really care about this. > my two sparks > Jassi -- -- Ben Q: What's a light-year? A: One-third less calories than a regular year. -- To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 4/4] pata_samsung: Add Samsung PATA controller driver
On Thu, May 27, 2010 at 5:22 PM, Kukjin Kim wrote: > From: Abhilash Kesavan > > 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 > Signed-off-by: Kukjin Kim > --- > drivers/ata/Kconfig | 9 + > drivers/ata/Makefile | 1 + > drivers/ata/pata_samsung.c | 591 Fasten your seat belts before reading further Rather than generic 'samsung', I would suggest the driver named after the SoC, that is supported first(chronologically) in mainline kernel. All newer SoCs should be simply taken to contain the controller of that SoC. Otherwise, the same naming problem comes back to haunt us should Samsung decides to use a different IP in future SoCs. What would we call that driver? pata_samsung_v2.c ? my two sparks Jassi -- To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 4/4] pata_samsung: Add Samsung PATA controller driver
On Thu, May 27, 2010 at 05:22:04PM +0900, Kukjin Kim wrote:
> From: Abhilash Kesavan
>
> 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
> Signed-off-by: Kukjin Kim
> ---
> drivers/ata/Kconfig|9 +
> drivers/ata/Makefile |1 +
> drivers/ata/pata_samsung.c | 591
>
> 3 files changed, 601 insertions(+), 0 deletions(-)
> create mode 100644 drivers/ata/pata_samsung.c
>
> diff --git a/drivers/ata/Kconfig b/drivers/ata/Kconfig
> index e68541f..ce40f66 100644
> --- a/drivers/ata/Kconfig
> +++ b/drivers/ata/Kconfig
> @@ -640,6 +640,15 @@ config PATA_RZ1000
>
> If unsure, say N.
>
> +config PATA_SAMSUNG
> + tristate "Samsung SoC PATA support"
> + depends on S3C_DEV_IDE
> + help
> + This option enables basic support for Samsung's S3C/S5P board
> + PATA controllers via the new ATA layer
> +
> + If unsure, say N.
> +
> config PATA_SC1200
> tristate "SC1200 PATA support"
> depends on PCI
> diff --git a/drivers/ata/Makefile b/drivers/ata/Makefile
> index d0a93c4..0b6d29a 100644
> --- a/drivers/ata/Makefile
> +++ b/drivers/ata/Makefile
> @@ -62,6 +62,7 @@ obj-$(CONFIG_PATA_RADISYS) += pata_radisys.o
> obj-$(CONFIG_PATA_RB532) += pata_rb532_cf.o
> obj-$(CONFIG_PATA_RDC) += pata_rdc.o
> obj-$(CONFIG_PATA_RZ1000)+= pata_rz1000.o
> +obj-$(CONFIG_PATA_SAMSUNG) += pata_samsung.o
do you want to call it pata_samsung_soc.o instead, in case samsung make
otehr types of pata controller?
> obj-$(CONFIG_PATA_SC1200)+= pata_sc1200.o
> obj-$(CONFIG_PATA_SERVERWORKS) += pata_serverworks.o
> obj-$(CONFIG_PATA_SIL680)+= pata_sil680.o
> diff --git a/drivers/ata/pata_samsung.c b/drivers/ata/pata_samsung.c
> new file mode 100644
> index 000..14381e4
> --- /dev/null
> +++ b/drivers/ata/pata_samsung.c
> @@ -0,0 +1,591 @@
> +/* linux/drivers/ata/pata_samsung.c
> + *
> + * Copyright (c) 2010 Samsung Electronics Co., Ltd.
> + * http://www.samsung.com
> + *
> + * PATA driver for Samsung SoC's.
SoCs.
> + * Supports CF Interface in True IDE mode. Currently only PIO mode has been
> + * implemented; UDMA support has to be added.
> + *
> + * Based on:
> + * PATA driver for AT91SAM9260 Static Memory Controller
> + * PATA driver for Toshiba SCC controller
> + *
> + * This program is free software; you can redistribute it and/or modify it
> + * under the terms of the GNU General Public License version 2
> + * as published by the Free Software Foundation.
> +*/
> +
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +
> +#include
> +#include
> +
> +#define DRV_NAME "pata_samsung"
> +#define DRV_VERSION "0.1"
> +
> +enum s3c_cpu_type {
> + TYPE_S3C6400,
> + TYPE_S5PC100,
> + TYPE_S5PV210,
> +};
> +
> +/*
> + * struct s3c_ide_info - S3C PATA instance.
> + * @clk: The clock resource for this controller.
> + * @ide_addr: The area mapped for the hardware registers.
> + * @sfr_addr: The area mapped for the special function registers.
> + * @irq: The IRQ number we are using.
> + * @cpu_type: The exact type of this controller.
> + * @fifo_status_reg: The ATA_FIFO_STATUS register offset.
> + */
> +struct s3c_ide_info {
> + struct clk *clk;
> + void __iomem *ide_addr;
> + void __iomem *sfr_addr;
> + unsigned int irq;
> + enum s3c_cpu_type cpu_type;
> + unsigned int fifo_status_reg;
> +};
> +
> +static unsigned long piotime[5];
> +
> +static void pata_s3c_set_endian(void *s3c_ide_regbase, u8 mode)
> +{
> + u32 reg = readl(s3c_ide_regbase + S3C_ATA_CFG);
> + reg = mode ? (reg & ~S3C_ATA_CFG_SWAP) : (reg | S3C_ATA_CFG_SWAP);
> + writel(reg, s3c_ide_regbase + S3C_ATA_CFG);
> +}
> +
> +static void pata_s3c_tune_chipset(struct s3c_ide_info *info, u8 ide_mode)
> +{
> + ulong ata_cfg = readl(info->ide_addr + S3C_ATA_CFG);
> +
> + /* IORDY is enabled for modes > PIO2 */
> + if (XFER_PIO_0 <= ide_mode && ide_mode <= XFER_PIO_4) {
> +
> + switch (ide_mode) {
> + case XFER_PIO_0:
> + case XFER_PIO_1:
> + case XFER_PIO_2:
> + ata_cfg &= (~S3C_ATA_CFG_IORDYEN);
> + break;
> + case XFER_PIO_3:
> + case XFER_PIO_4:
> + ata_cfg |= S3C_ATA_CFG_IORDYEN;
> + break;
> + }
> +
> + writel(ata_cfg, info->ide_addr + S3C_ATA_CFG);
> + writel(piotime[ide_mode - XFER_PIO_0],
> + info->ide_addr + S3C_ATA_PIO_TIME);
> + } else {
> + /* Default to PIO0 */
> + ata_cfg &= (~S3C_ATA_CFG_IORDYEN);
technically no need to ().
> + writel(ata_cfg, info->ide_addr + S3C_ATA_CFG);
> + writel(piotime[0], info->ide_addr + S3
[PATCH 4/4] pata_samsung: Add Samsung PATA controller driver
From: Abhilash Kesavan
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
Signed-off-by: Kukjin Kim
---
drivers/ata/Kconfig|9 +
drivers/ata/Makefile |1 +
drivers/ata/pata_samsung.c | 591
3 files changed, 601 insertions(+), 0 deletions(-)
create mode 100644 drivers/ata/pata_samsung.c
diff --git a/drivers/ata/Kconfig b/drivers/ata/Kconfig
index e68541f..ce40f66 100644
--- a/drivers/ata/Kconfig
+++ b/drivers/ata/Kconfig
@@ -640,6 +640,15 @@ config PATA_RZ1000
If unsure, say N.
+config PATA_SAMSUNG
+ tristate "Samsung SoC PATA support"
+ depends on S3C_DEV_IDE
+ help
+ This option enables basic support for Samsung's S3C/S5P board
+ PATA controllers via the new ATA layer
+
+ If unsure, say N.
+
config PATA_SC1200
tristate "SC1200 PATA support"
depends on PCI
diff --git a/drivers/ata/Makefile b/drivers/ata/Makefile
index d0a93c4..0b6d29a 100644
--- a/drivers/ata/Makefile
+++ b/drivers/ata/Makefile
@@ -62,6 +62,7 @@ obj-$(CONFIG_PATA_RADISYS)+= pata_radisys.o
obj-$(CONFIG_PATA_RB532) += pata_rb532_cf.o
obj-$(CONFIG_PATA_RDC) += pata_rdc.o
obj-$(CONFIG_PATA_RZ1000) += pata_rz1000.o
+obj-$(CONFIG_PATA_SAMSUNG) += pata_samsung.o
obj-$(CONFIG_PATA_SC1200) += pata_sc1200.o
obj-$(CONFIG_PATA_SERVERWORKS) += pata_serverworks.o
obj-$(CONFIG_PATA_SIL680) += pata_sil680.o
diff --git a/drivers/ata/pata_samsung.c b/drivers/ata/pata_samsung.c
new file mode 100644
index 000..14381e4
--- /dev/null
+++ b/drivers/ata/pata_samsung.c
@@ -0,0 +1,591 @@
+/* linux/drivers/ata/pata_samsung.c
+ *
+ * Copyright (c) 2010 Samsung Electronics Co., Ltd.
+ * http://www.samsung.com
+ *
+ * PATA driver for Samsung SoC's.
+ * Supports CF Interface in True IDE mode. Currently only PIO mode has been
+ * implemented; UDMA support has to be added.
+ *
+ * Based on:
+ * PATA driver for AT91SAM9260 Static Memory Controller
+ * PATA driver for Toshiba SCC controller
+ *
+ * This program is free software; you can redistribute it and/or modify it
+ * under the terms of the GNU General Public License version 2
+ * as published by the Free Software Foundation.
+*/
+
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+
+#include
+#include
+
+#define DRV_NAME "pata_samsung"
+#define DRV_VERSION "0.1"
+
+enum s3c_cpu_type {
+ TYPE_S3C6400,
+ TYPE_S5PC100,
+ TYPE_S5PV210,
+};
+
+/*
+ * struct s3c_ide_info - S3C PATA instance.
+ * @clk: The clock resource for this controller.
+ * @ide_addr: The area mapped for the hardware registers.
+ * @sfr_addr: The area mapped for the special function registers.
+ * @irq: The IRQ number we are using.
+ * @cpu_type: The exact type of this controller.
+ * @fifo_status_reg: The ATA_FIFO_STATUS register offset.
+ */
+struct s3c_ide_info {
+ struct clk *clk;
+ void __iomem *ide_addr;
+ void __iomem *sfr_addr;
+ unsigned int irq;
+ enum s3c_cpu_type cpu_type;
+ unsigned int fifo_status_reg;
+};
+
+static unsigned long piotime[5];
+
+static void pata_s3c_set_endian(void *s3c_ide_regbase, u8 mode)
+{
+ u32 reg = readl(s3c_ide_regbase + S3C_ATA_CFG);
+ reg = mode ? (reg & ~S3C_ATA_CFG_SWAP) : (reg | S3C_ATA_CFG_SWAP);
+ writel(reg, s3c_ide_regbase + S3C_ATA_CFG);
+}
+
+static void pata_s3c_tune_chipset(struct s3c_ide_info *info, u8 ide_mode)
+{
+ ulong ata_cfg = readl(info->ide_addr + S3C_ATA_CFG);
+
+ /* IORDY is enabled for modes > PIO2 */
+ if (XFER_PIO_0 <= ide_mode && ide_mode <= XFER_PIO_4) {
+
+ switch (ide_mode) {
+ case XFER_PIO_0:
+ case XFER_PIO_1:
+ case XFER_PIO_2:
+ ata_cfg &= (~S3C_ATA_CFG_IORDYEN);
+ break;
+ case XFER_PIO_3:
+ case XFER_PIO_4:
+ ata_cfg |= S3C_ATA_CFG_IORDYEN;
+ break;
+ }
+
+ writel(ata_cfg, info->ide_addr + S3C_ATA_CFG);
+ writel(piotime[ide_mode - XFER_PIO_0],
+ info->ide_addr + S3C_ATA_PIO_TIME);
+ } else {
+ /* Default to PIO0 */
+ ata_cfg &= (~S3C_ATA_CFG_IORDYEN);
+ writel(ata_cfg, info->ide_addr + S3C_ATA_CFG);
+ writel(piotime[0], info->ide_addr + S3C_ATA_PIO_TIME);
+ }
+}
+
+static void
+pata_s3c_set_piomode(struct ata_port *ap, struct ata_device *adev)
+{
+ struct s3c_ide_info *info = ap->host->private_data;
+
+ /* Configure IORDY based on PIO mode and also set the timing value */
+ pata_s3c_tune_chipset(info, adev->pio_mode);
+}
+
+/*
+ * Waits until the IDE controller is able to perform next read/write
+ * opera
