| Message ID | 1329284944-17943-1-git-send-email-qiang.liu@freescale.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
On Wed, Feb 15, 2012 at 1:49 PM, Qiang Liu <qiang.liu@freescale.com> wrote: Hi Liu Qiang, The patch is fine except for the title and comment. It's too vague to say I/O load balancing. You need explicit description like "add interrupt coalescing support" as title. > Reduce interrupt signals through reset Interrupt Coalescing Control Reg. > Provide dynamic method to adjust interrupt signals and timer ticks by sysfs. > It is a tradeoff for different applications. How about: Adds support for interrupt coalescing feature to reduce interrupt events. Provides mechanism of adjusting coalescing count and timeout tick by sysfs on runtime, so that tradeoff of latency and CPU load can be made depending on applications. - Leo
> -----Original Message----- > From: linux-ide-owner@vger.kernel.org [mailto:linux-ide- > owner@vger.kernel.org] On Behalf Of Li Yang > Sent: Wednesday, February 15, 2012 3:22 PM > To: Liu Qiang-B32616 > Cc: jgarzik@pobox.com; linux-ide@vger.kernel.org; linuxppc- > dev@lists.ozlabs.org; linux-kernel@vger.kernel.org > Subject: Re: [PATCH V2 RESEND] fsl-sata: I/O load balancing > > On Wed, Feb 15, 2012 at 1:49 PM, Qiang Liu <qiang.liu@freescale.com> > wrote: > > Hi Liu Qiang, > > The patch is fine except for the title and comment. It's too vague to > say I/O load balancing. You need explicit description like "add > interrupt coalescing support" as title. > > > Reduce interrupt signals through reset Interrupt Coalescing Control Reg. > > Provide dynamic method to adjust interrupt signals and timer ticks by > sysfs. > > It is a tradeoff for different applications. > > How about: > > Adds support for interrupt coalescing feature to reduce interrupt events. > Provides mechanism of adjusting coalescing count and timeout tick by > sysfs on runtime, so that tradeoff of latency and CPU load can be made > depending on applications. > Ok, I will change the title and comments more specifically. > - Leo > -- > To unsubscribe from this list: send the line "unsubscribe linux-ide" in > the body of a message to majordomo@vger.kernel.org More majordomo info at > http://vger.kernel.org/majordomo-info.html
On Wed, 2012-02-15 at 13:49 +0800, Qiang Liu wrote: ] > diff --git a/drivers/ata/sata_fsl.c b/drivers/ata/sata_fsl.c > index 0120b0d..d6577b9 100644 > --- a/drivers/ata/sata_fsl.c > +++ b/drivers/ata/sata_fsl.c > @@ -6,7 +6,7 @@ > * Author: Ashish Kalra <ashish.kalra@freescale.com> > * Li Yang <leoli@freescale.com> > * > - * Copyright (c) 2006-2007, 2011 Freescale Semiconductor, Inc. > + * Copyright (c) 2006-2007, 2011-2012 Freescale Semiconductor, Inc. > * > * This program is free software; you can redistribute it and/or modify it > * under the terms of the GNU General Public License as published by the > @@ -26,6 +26,15 @@ > #include <asm/io.h> > #include <linux/of_platform.h> > > +static unsigned int intr_coalescing_count; > +module_param(intr_coalescing_count, int, S_IRUGO); > +MODULE_PARM_DESC(intr_coalescing_count, > + "INT coalescing count threshold (1..31)"); > + > +static unsigned int intr_coalescing_ticks; > +module_param(intr_coalescing_ticks, int, S_IRUGO); > +MODULE_PARM_DESC(intr_coalescing_ticks, > + "INT coalescing timer threshold in AHB ticks"); You don't seem to provide very useful defaults... for example, intr_coalescing_count starts at 0 which isn't even in range (the code fixes that up but still...). I tried a debian install on the p5020ds here and I find SATA to be extremely slow, generating millions of interrupts. I think the defaults should be a lot more aggressive at coalescing. Cheers, Ben. > /* Controller information */ > enum { > SATA_FSL_QUEUE_DEPTH = 16, > @@ -83,6 +92,16 @@ enum { > }; > > /* > + * Interrupt Coalescing Control Register bitdefs */ > +enum { > + ICC_MIN_INT_COUNT_THRESHOLD = 1, > + ICC_MAX_INT_COUNT_THRESHOLD = ((1 << 5) - 1), > + ICC_MIN_INT_TICKS_THRESHOLD = 0, > + ICC_MAX_INT_TICKS_THRESHOLD = ((1 << 19) - 1), > + ICC_SAFE_INT_TICKS = 1, > +}; > + > +/* > * Host Controller command register set - per port > */ > enum { > @@ -263,8 +282,65 @@ struct sata_fsl_host_priv { > void __iomem *csr_base; > int irq; > int data_snoop; > + struct device_attribute intr_coalescing; > }; > > +static void fsl_sata_set_irq_coalescing(struct ata_host *host, > + unsigned int count, unsigned int ticks) > +{ > + struct sata_fsl_host_priv *host_priv = host->private_data; > + void __iomem *hcr_base = host_priv->hcr_base; > + > + if (count > ICC_MAX_INT_COUNT_THRESHOLD) > + count = ICC_MAX_INT_COUNT_THRESHOLD; > + else if (count < ICC_MIN_INT_COUNT_THRESHOLD) > + count = ICC_MIN_INT_COUNT_THRESHOLD; > + > + if (ticks > ICC_MAX_INT_TICKS_THRESHOLD) > + ticks = ICC_MAX_INT_TICKS_THRESHOLD; > + else if ((ICC_MIN_INT_TICKS_THRESHOLD == ticks) && > + (count > ICC_MIN_INT_COUNT_THRESHOLD)) > + ticks = ICC_SAFE_INT_TICKS; > + > + spin_lock(&host->lock); > + iowrite32((count << 24 | ticks), hcr_base + ICC); > + > + intr_coalescing_count = count; > + intr_coalescing_ticks = ticks; > + spin_unlock(&host->lock); > + > + DPRINTK("intrrupt coalescing, count = 0x%x, ticks = %x\n", > + intr_coalescing_count, intr_coalescing_ticks); > + DPRINTK("ICC register status: (hcr base: 0x%x) = 0x%x\n", > + hcr_base, ioread32(hcr_base + ICC)); > +} > + > +static ssize_t fsl_sata_intr_coalescing_show(struct device *dev, > + struct device_attribute *attr, char *buf) > +{ > + return sprintf(buf, "%d %d\n", > + intr_coalescing_count, intr_coalescing_ticks); > +} > + > +static ssize_t fsl_sata_intr_coalescing_store(struct device *dev, > + struct device_attribute *attr, > + const char *buf, size_t count) > +{ > + unsigned int coalescing_count, coalescing_ticks; > + > + if (sscanf(buf, "%d%d", > + &coalescing_count, > + &coalescing_ticks) != 2) { > + printk(KERN_ERR "fsl-sata: wrong parameter format.\n"); > + return -EINVAL; > + } > + > + fsl_sata_set_irq_coalescing(dev_get_drvdata(dev), > + coalescing_count, coalescing_ticks); > + > + return strlen(buf); > +} > + > static inline unsigned int sata_fsl_tag(unsigned int tag, > void __iomem *hcr_base) > { > @@ -346,10 +422,10 @@ static unsigned int sata_fsl_fill_sg(struct ata_queued_cmd *qc, void *cmd_desc, > (unsigned long long)sg_addr, sg_len); > > /* warn if each s/g element is not dword aligned */ > - if (sg_addr & 0x03) > + if (unlikely(sg_addr & 0x03)) > ata_port_err(qc->ap, "s/g addr unaligned : 0x%llx\n", > (unsigned long long)sg_addr); > - if (sg_len & 0x03) > + if (unlikely(sg_len & 0x03)) > ata_port_err(qc->ap, "s/g len unaligned : 0x%x\n", > sg_len); > > @@ -1245,6 +1321,13 @@ static int sata_fsl_init_controller(struct ata_host *host) > iowrite32(0x00000FFFF, hcr_base + CE); > iowrite32(0x00000FFFF, hcr_base + DE); > > + /* > + * reset the number of command complete bits which will cause the > + * interrupt to be signaled > + */ > + fsl_sata_set_irq_coalescing(host, intr_coalescing_count, > + intr_coalescing_ticks); > + > /* > * host controller will be brought on-line, during xx_port_start() > * callback, that should also initiate the OOB, COMINIT sequence > @@ -1309,7 +1392,7 @@ static int sata_fsl_probe(struct platform_device *ofdev) > void __iomem *csr_base = NULL; > struct sata_fsl_host_priv *host_priv = NULL; > int irq; > - struct ata_host *host; > + struct ata_host *host = NULL; > u32 temp; > > struct ata_port_info pi = sata_fsl_port_info[0]; > @@ -1356,6 +1439,10 @@ static int sata_fsl_probe(struct platform_device *ofdev) > > /* allocate host structure */ > host = ata_host_alloc_pinfo(&ofdev->dev, ppi, SATA_FSL_MAX_PORTS); > + if (!host) { > + retval = -ENOMEM; > + goto error_exit_with_cleanup; > + } > > /* host->iomap is not used currently */ > host->private_data = host_priv; > @@ -1373,10 +1460,24 @@ static int sata_fsl_probe(struct platform_device *ofdev) > > dev_set_drvdata(&ofdev->dev, host); > > + host_priv->intr_coalescing.show = fsl_sata_intr_coalescing_show; > + host_priv->intr_coalescing.store = fsl_sata_intr_coalescing_store; > + sysfs_attr_init(&host_priv->intr_coalescing.attr); > + host_priv->intr_coalescing.attr.name = "intr_coalescing"; > + host_priv->intr_coalescing.attr.mode = S_IRUGO | S_IWUSR; > + retval = device_create_file(host->dev, &host_priv->intr_coalescing); > + if (retval) > + goto error_exit_with_cleanup; > + > return 0; > > error_exit_with_cleanup: > > + if (host) { > + dev_set_drvdata(&ofdev->dev, NULL); > + ata_host_detach(host); > + } > + > if (hcr_base) > iounmap(hcr_base); > if (host_priv) > @@ -1390,6 +1491,8 @@ static int sata_fsl_remove(struct platform_device *ofdev) > struct ata_host *host = dev_get_drvdata(&ofdev->dev); > struct sata_fsl_host_priv *host_priv = host->private_data; > > + device_remove_file(&ofdev->dev, &host_priv->intr_coalescing); > + > ata_host_detach(host); > > dev_set_drvdata(&ofdev->dev, NULL); > -- > 1.6.4 > > > _______________________________________________ > Linuxppc-dev mailing list > Linuxppc-dev@lists.ozlabs.org > https://lists.ozlabs.org/listinfo/linuxppc-dev
> -----Original Message----- > From: Benjamin Herrenschmidt [mailto:benh@kernel.crashing.org] > Sent: Friday, February 17, 2012 8:40 AM > To: Liu Qiang-B32616 > Cc: jgarzik@pobox.com; linux-ide@vger.kernel.org; linuxppc- > dev@lists.ozlabs.org; linux-kernel@vger.kernel.org > Subject: Re: [PATCH V2 RESEND] fsl-sata: I/O load balancing > > On Wed, 2012-02-15 at 13:49 +0800, Qiang Liu wrote: > ] > > diff --git a/drivers/ata/sata_fsl.c b/drivers/ata/sata_fsl.c index > > 0120b0d..d6577b9 100644 > > --- a/drivers/ata/sata_fsl.c > > +++ b/drivers/ata/sata_fsl.c > > @@ -6,7 +6,7 @@ > > * Author: Ashish Kalra <ashish.kalra@freescale.com> > > * Li Yang <leoli@freescale.com> > > * > > - * Copyright (c) 2006-2007, 2011 Freescale Semiconductor, Inc. > > + * Copyright (c) 2006-2007, 2011-2012 Freescale Semiconductor, Inc. > > * > > * This program is free software; you can redistribute it and/or > modify it > > * under the terms of the GNU General Public License as published > > by the @@ -26,6 +26,15 @@ #include <asm/io.h> #include > > <linux/of_platform.h> > > > > +static unsigned int intr_coalescing_count; > > +module_param(intr_coalescing_count, int, S_IRUGO); > > +MODULE_PARM_DESC(intr_coalescing_count, > > + "INT coalescing count threshold (1..31)"); > > + > > +static unsigned int intr_coalescing_ticks; > > +module_param(intr_coalescing_ticks, int, S_IRUGO); > > +MODULE_PARM_DESC(intr_coalescing_ticks, > > + "INT coalescing timer threshold in AHB ticks"); > > You don't seem to provide very useful defaults... for example, > intr_coalescing_count starts at 0 which isn't even in range (the code > fixes that up but still...). > > I tried a debian install on the p5020ds here and I find SATA to be > extremely slow, generating millions of interrupts. I think the defaults > should be a lot more aggressive at coalescing. Hello Ben, The default will be set in a common interface fsl_sata_set_irq_coalescing when initialize the controller. This interface will check the range of intr count and ticks and make sure the values are reasonably. It's hard to find a aggressive value to adapt all scenarios, so I use echo to adjust the value. I remember P5020 have some performance issue, I will check it. BTW, which filesystem do you use? Ext2 is lower than ext4 because metadata is continuously wrote to disk. You can try ext4 or xfs. Thanks. > > Cheers, > Ben. > > > /* Controller information */ > > enum { > > SATA_FSL_QUEUE_DEPTH = 16, > > @@ -83,6 +92,16 @@ enum { > > }; > > > > /* > > + * Interrupt Coalescing Control Register bitdefs */ enum { > > + ICC_MIN_INT_COUNT_THRESHOLD = 1, > > + ICC_MAX_INT_COUNT_THRESHOLD = ((1 << 5) - 1), > > + ICC_MIN_INT_TICKS_THRESHOLD = 0, > > + ICC_MAX_INT_TICKS_THRESHOLD = ((1 << 19) - 1), > > + ICC_SAFE_INT_TICKS = 1, > > +}; > > + > > +/* > > * Host Controller command register set - per port */ enum { @@ > > -263,8 +282,65 @@ struct sata_fsl_host_priv { > > void __iomem *csr_base; > > int irq; > > int data_snoop; > > + struct device_attribute intr_coalescing; > > }; > > > > +static void fsl_sata_set_irq_coalescing(struct ata_host *host, > > + unsigned int count, unsigned int ticks) { > > + struct sata_fsl_host_priv *host_priv = host->private_data; > > + void __iomem *hcr_base = host_priv->hcr_base; > > + > > + if (count > ICC_MAX_INT_COUNT_THRESHOLD) > > + count = ICC_MAX_INT_COUNT_THRESHOLD; > > + else if (count < ICC_MIN_INT_COUNT_THRESHOLD) > > + count = ICC_MIN_INT_COUNT_THRESHOLD; > > + > > + if (ticks > ICC_MAX_INT_TICKS_THRESHOLD) > > + ticks = ICC_MAX_INT_TICKS_THRESHOLD; > > + else if ((ICC_MIN_INT_TICKS_THRESHOLD == ticks) && > > + (count > ICC_MIN_INT_COUNT_THRESHOLD)) > > + ticks = ICC_SAFE_INT_TICKS; > > + > > + spin_lock(&host->lock); > > + iowrite32((count << 24 | ticks), hcr_base + ICC); > > + > > + intr_coalescing_count = count; > > + intr_coalescing_ticks = ticks; > > + spin_unlock(&host->lock); > > + > > + DPRINTK("intrrupt coalescing, count = 0x%x, ticks = %x\n", > > + intr_coalescing_count, intr_coalescing_ticks); > > + DPRINTK("ICC register status: (hcr base: 0x%x) = 0x%x\n", > > + hcr_base, ioread32(hcr_base + ICC)); } > > + > > +static ssize_t fsl_sata_intr_coalescing_show(struct device *dev, > > + struct device_attribute *attr, char *buf) { > > + return sprintf(buf, "%d %d\n", > > + intr_coalescing_count, intr_coalescing_ticks); } > > + > > +static ssize_t fsl_sata_intr_coalescing_store(struct device *dev, > > + struct device_attribute *attr, > > + const char *buf, size_t count) > > +{ > > + unsigned int coalescing_count, coalescing_ticks; > > + > > + if (sscanf(buf, "%d%d", > > + &coalescing_count, > > + &coalescing_ticks) != 2) { > > + printk(KERN_ERR "fsl-sata: wrong parameter format.\n"); > > + return -EINVAL; > > + } > > + > > + fsl_sata_set_irq_coalescing(dev_get_drvdata(dev), > > + coalescing_count, coalescing_ticks); > > + > > + return strlen(buf); > > +} > > + > > static inline unsigned int sata_fsl_tag(unsigned int tag, > > void __iomem *hcr_base) > > { > > @@ -346,10 +422,10 @@ static unsigned int sata_fsl_fill_sg(struct > ata_queued_cmd *qc, void *cmd_desc, > > (unsigned long long)sg_addr, sg_len); > > > > /* warn if each s/g element is not dword aligned */ > > - if (sg_addr & 0x03) > > + if (unlikely(sg_addr & 0x03)) > > ata_port_err(qc->ap, "s/g addr unaligned : 0x%llx\n", > > (unsigned long long)sg_addr); > > - if (sg_len & 0x03) > > + if (unlikely(sg_len & 0x03)) > > ata_port_err(qc->ap, "s/g len unaligned : 0x%x\n", > > sg_len); > > > > @@ -1245,6 +1321,13 @@ static int sata_fsl_init_controller(struct > ata_host *host) > > iowrite32(0x00000FFFF, hcr_base + CE); > > iowrite32(0x00000FFFF, hcr_base + DE); > > > > + /* > > + * reset the number of command complete bits which will cause the > > + * interrupt to be signaled > > + */ > > + fsl_sata_set_irq_coalescing(host, intr_coalescing_count, > > + intr_coalescing_ticks); > > + > > /* > > * host controller will be brought on-line, during xx_port_start() > > * callback, that should also initiate the OOB, COMINIT sequence @@ > > -1309,7 +1392,7 @@ static int sata_fsl_probe(struct platform_device > *ofdev) > > void __iomem *csr_base = NULL; > > struct sata_fsl_host_priv *host_priv = NULL; > > int irq; > > - struct ata_host *host; > > + struct ata_host *host = NULL; > > u32 temp; > > > > struct ata_port_info pi = sata_fsl_port_info[0]; @@ -1356,6 > +1439,10 > > @@ static int sata_fsl_probe(struct platform_device *ofdev) > > > > /* allocate host structure */ > > host = ata_host_alloc_pinfo(&ofdev->dev, ppi, SATA_FSL_MAX_PORTS); > > + if (!host) { > > + retval = -ENOMEM; > > + goto error_exit_with_cleanup; > > + } > > > > /* host->iomap is not used currently */ > > host->private_data = host_priv; > > @@ -1373,10 +1460,24 @@ static int sata_fsl_probe(struct > > platform_device *ofdev) > > > > dev_set_drvdata(&ofdev->dev, host); > > > > + host_priv->intr_coalescing.show = fsl_sata_intr_coalescing_show; > > + host_priv->intr_coalescing.store = fsl_sata_intr_coalescing_store; > > + sysfs_attr_init(&host_priv->intr_coalescing.attr); > > + host_priv->intr_coalescing.attr.name = "intr_coalescing"; > > + host_priv->intr_coalescing.attr.mode = S_IRUGO | S_IWUSR; > > + retval = device_create_file(host->dev, &host_priv->intr_coalescing); > > + if (retval) > > + goto error_exit_with_cleanup; > > + > > return 0; > > > > error_exit_with_cleanup: > > > > + if (host) { > > + dev_set_drvdata(&ofdev->dev, NULL); > > + ata_host_detach(host); > > + } > > + > > if (hcr_base) > > iounmap(hcr_base); > > if (host_priv) > > @@ -1390,6 +1491,8 @@ static int sata_fsl_remove(struct platform_device > *ofdev) > > struct ata_host *host = dev_get_drvdata(&ofdev->dev); > > struct sata_fsl_host_priv *host_priv = host->private_data; > > > > + device_remove_file(&ofdev->dev, &host_priv->intr_coalescing); > > + > > ata_host_detach(host); > > > > dev_set_drvdata(&ofdev->dev, NULL); > > -- > > 1.6.4 > > > > > > _______________________________________________ > > Linuxppc-dev mailing list > > Linuxppc-dev@lists.ozlabs.org > > https://lists.ozlabs.org/listinfo/linuxppc-dev > >
>I tried a debian install on the p5020ds here and I find SATA to be >extremely slow, generating millions of interrupts. I think the defaults >should be a lot more aggressive at coalescing. The P5020 has a hardware problem with SATA, making it generate more interrupts than it should. A new revision of the silicon will fix it. - Leo
On Fri, 2012-02-17 at 01:54 +0000, Liu Qiang-B32616 wrote: > The default will be set in a common interface fsl_sata_set_irq_coalescing when > initialize the controller. This interface will check the range of intr count > and ticks and make sure the values are reasonably. Allright, but the current defaults are basically no coalescing right ? > It's hard to find a aggressive value to adapt all scenarios, so I use echo to adjust > the value. I remember P5020 have some performance issue, I will check it. > BTW, which filesystem do you use? Ext2 is lower than ext4 because metadata is > continuously wrote to disk. You can try ext4 or xfs. ext3 at the moment, I plan to switch to ext4 when I finish that fsck pass which is taking hours... I am not aware of the 5020 performance issues, is this something documented and/or fixable ? Cheers, Ben.
On Fri, 2012-02-17 at 03:09 +0000, Li Yang-R58472 wrote: > >I tried a debian install on the p5020ds here and I find SATA to be > >extremely slow, generating millions of interrupts. I think the > defaults > >should be a lot more aggressive at coalescing. > > The P5020 has a hardware problem with SATA, making it generate more > interrupts than it should. A new revision of the silicon will fix it. Ok cool. With a bit of luck you guys can send me a new silicon when it's available then :-) (The one in the board you got me seems to be in a socket). Cheers, Ben.
> -----Original Message----- > From: Benjamin Herrenschmidt [mailto:benh@kernel.crashing.org] > Sent: Friday, February 17, 2012 11:18 AM > To: Liu Qiang-B32616 > Cc: jgarzik@pobox.com; linux-ide@vger.kernel.org; linuxppc- > dev@lists.ozlabs.org; linux-kernel@vger.kernel.org > Subject: RE: [PATCH V2 RESEND] fsl-sata: I/O load balancing > > On Fri, 2012-02-17 at 01:54 +0000, Liu Qiang-B32616 wrote: > > The default will be set in a common interface > > fsl_sata_set_irq_coalescing when initialize the controller. This > > interface will check the range of intr count and ticks and make sure > the values are reasonably. > > Allright, but the current defaults are basically no coalescing right ? > > > It's hard to find a aggressive value to adapt all scenarios, so I use > > echo to adjust the value. I remember P5020 have some performance issue, > I will check it. > > BTW, which filesystem do you use? Ext2 is lower than ext4 because > > metadata is continuously wrote to disk. You can try ext4 or xfs. > > ext3 at the moment, I plan to switch to ext4 when I finish that fsck pass > which is taking hours... I am not aware of the 5020 performance issues, > is this something documented and/or fixable ? > My patch may not meet your requirement:( For your performance requirement, I suggest use ext4 filesystem and SSD. Thanks. > Cheers, > Ben. > >
> -----Original Message----- > From: linuxppc-dev-bounces+tie-fei.zang=freescale.com@lists.ozlabs.org > [mailto:linuxppc-dev-bounces+tie-fei.zang=freescale.com@lists.ozlabs.org] > On Behalf Of Benjamin Herrenschmidt > Sent: Friday, February 17, 2012 11:19 AM > To: Li Yang-R58472 > Cc: Liu Qiang-B32616; jgarzik@pobox.com; linuxppc-dev@lists.ozlabs.org; > linux-kernel@vger.kernel.org; linux-ide@vger.kernel.org > Subject: RE: [PATCH V2 RESEND] fsl-sata: I/O load balancing > > On Fri, 2012-02-17 at 03:09 +0000, Li Yang-R58472 wrote: > > >I tried a debian install on the p5020ds here and I find SATA to be > > >extremely slow, generating millions of interrupts. I think the > > defaults > > >should be a lot more aggressive at coalescing. > > > > The P5020 has a hardware problem with SATA, making it generate more > > interrupts than it should. A new revision of the silicon will fix it. > > Ok cool. With a bit of luck you guys can send me a new silicon when it's > available then :-) (The one in the board you got me seems to be in a > socket). Ben, We still do not have the a P5020 sample to fix the problem. One workaround is to use the patch ( I think you have noticed the patch): http://git.freescale.com/git/cgit.cgi/ppc/sdk/linux.git/commit/?id=92c0ce1e599e788ffc0908739db9bd5e0dbdad69 and use a SSD hard drive. It will have a usable performance. Thanks. Roy
diff --git a/drivers/ata/sata_fsl.c b/drivers/ata/sata_fsl.c index 0120b0d..d6577b9 100644 --- a/drivers/ata/sata_fsl.c +++ b/drivers/ata/sata_fsl.c @@ -6,7 +6,7 @@ * Author: Ashish Kalra <ashish.kalra@freescale.com> * Li Yang <leoli@freescale.com> * - * Copyright (c) 2006-2007, 2011 Freescale Semiconductor, Inc. + * Copyright (c) 2006-2007, 2011-2012 Freescale Semiconductor, Inc. * * This program is free software; you can redistribute it and/or modify it * under the terms of the GNU General Public License as published by the @@ -26,6 +26,15 @@ #include <asm/io.h> #include <linux/of_platform.h> +static unsigned int intr_coalescing_count; +module_param(intr_coalescing_count, int, S_IRUGO); +MODULE_PARM_DESC(intr_coalescing_count, + "INT coalescing count threshold (1..31)"); + +static unsigned int intr_coalescing_ticks; +module_param(intr_coalescing_ticks, int, S_IRUGO); +MODULE_PARM_DESC(intr_coalescing_ticks, + "INT coalescing timer threshold in AHB ticks"); /* Controller information */ enum { SATA_FSL_QUEUE_DEPTH = 16, @@ -83,6 +92,16 @@ enum { }; /* + * Interrupt Coalescing Control Register bitdefs */ +enum { + ICC_MIN_INT_COUNT_THRESHOLD = 1, + ICC_MAX_INT_COUNT_THRESHOLD = ((1 << 5) - 1), + ICC_MIN_INT_TICKS_THRESHOLD = 0, + ICC_MAX_INT_TICKS_THRESHOLD = ((1 << 19) - 1), + ICC_SAFE_INT_TICKS = 1, +}; + +/* * Host Controller command register set - per port */ enum { @@ -263,8 +282,65 @@ struct sata_fsl_host_priv { void __iomem *csr_base; int irq; int data_snoop; + struct device_attribute intr_coalescing; }; +static void fsl_sata_set_irq_coalescing(struct ata_host *host, + unsigned int count, unsigned int ticks) +{ + struct sata_fsl_host_priv *host_priv = host->private_data; + void __iomem *hcr_base = host_priv->hcr_base; + + if (count > ICC_MAX_INT_COUNT_THRESHOLD) + count = ICC_MAX_INT_COUNT_THRESHOLD; + else if (count < ICC_MIN_INT_COUNT_THRESHOLD) + count = ICC_MIN_INT_COUNT_THRESHOLD; + + if (ticks > ICC_MAX_INT_TICKS_THRESHOLD) + ticks = ICC_MAX_INT_TICKS_THRESHOLD; + else if ((ICC_MIN_INT_TICKS_THRESHOLD == ticks) && + (count > ICC_MIN_INT_COUNT_THRESHOLD)) + ticks = ICC_SAFE_INT_TICKS; + + spin_lock(&host->lock); + iowrite32((count << 24 | ticks), hcr_base + ICC); + + intr_coalescing_count = count; + intr_coalescing_ticks = ticks; + spin_unlock(&host->lock); + + DPRINTK("intrrupt coalescing, count = 0x%x, ticks = %x\n", + intr_coalescing_count, intr_coalescing_ticks); + DPRINTK("ICC register status: (hcr base: 0x%x) = 0x%x\n", + hcr_base, ioread32(hcr_base + ICC)); +} + +static ssize_t fsl_sata_intr_coalescing_show(struct device *dev, + struct device_attribute *attr, char *buf) +{ + return sprintf(buf, "%d %d\n", + intr_coalescing_count, intr_coalescing_ticks); +} + +static ssize_t fsl_sata_intr_coalescing_store(struct device *dev, + struct device_attribute *attr, + const char *buf, size_t count) +{ + unsigned int coalescing_count, coalescing_ticks; + + if (sscanf(buf, "%d%d", + &coalescing_count, + &coalescing_ticks) != 2) { + printk(KERN_ERR "fsl-sata: wrong parameter format.\n"); + return -EINVAL; + } + + fsl_sata_set_irq_coalescing(dev_get_drvdata(dev), + coalescing_count, coalescing_ticks); + + return strlen(buf); +} + static inline unsigned int sata_fsl_tag(unsigned int tag, void __iomem *hcr_base) { @@ -346,10 +422,10 @@ static unsigned int sata_fsl_fill_sg(struct ata_queued_cmd *qc, void *cmd_desc, (unsigned long long)sg_addr, sg_len); /* warn if each s/g element is not dword aligned */ - if (sg_addr & 0x03) + if (unlikely(sg_addr & 0x03)) ata_port_err(qc->ap, "s/g addr unaligned : 0x%llx\n", (unsigned long long)sg_addr); - if (sg_len & 0x03) + if (unlikely(sg_len & 0x03)) ata_port_err(qc->ap, "s/g len unaligned : 0x%x\n", sg_len); @@ -1245,6 +1321,13 @@ static int sata_fsl_init_controller(struct ata_host *host) iowrite32(0x00000FFFF, hcr_base + CE); iowrite32(0x00000FFFF, hcr_base + DE); + /* + * reset the number of command complete bits which will cause the + * interrupt to be signaled + */ + fsl_sata_set_irq_coalescing(host, intr_coalescing_count, + intr_coalescing_ticks); + /* * host controller will be brought on-line, during xx_port_start() * callback, that should also initiate the OOB, COMINIT sequence @@ -1309,7 +1392,7 @@ static int sata_fsl_probe(struct platform_device *ofdev) void __iomem *csr_base = NULL; struct sata_fsl_host_priv *host_priv = NULL; int irq; - struct ata_host *host; + struct ata_host *host = NULL; u32 temp; struct ata_port_info pi = sata_fsl_port_info[0]; @@ -1356,6 +1439,10 @@ static int sata_fsl_probe(struct platform_device *ofdev) /* allocate host structure */ host = ata_host_alloc_pinfo(&ofdev->dev, ppi, SATA_FSL_MAX_PORTS); + if (!host) { + retval = -ENOMEM; + goto error_exit_with_cleanup; + } /* host->iomap is not used currently */ host->private_data = host_priv; @@ -1373,10 +1460,24 @@ static int sata_fsl_probe(struct platform_device *ofdev) dev_set_drvdata(&ofdev->dev, host); + host_priv->intr_coalescing.show = fsl_sata_intr_coalescing_show; + host_priv->intr_coalescing.store = fsl_sata_intr_coalescing_store; + sysfs_attr_init(&host_priv->intr_coalescing.attr); + host_priv->intr_coalescing.attr.name = "intr_coalescing"; + host_priv->intr_coalescing.attr.mode = S_IRUGO | S_IWUSR; + retval = device_create_file(host->dev, &host_priv->intr_coalescing); + if (retval) + goto error_exit_with_cleanup; + return 0; error_exit_with_cleanup: + if (host) { + dev_set_drvdata(&ofdev->dev, NULL); + ata_host_detach(host); + } + if (hcr_base) iounmap(hcr_base); if (host_priv) @@ -1390,6 +1491,8 @@ static int sata_fsl_remove(struct platform_device *ofdev) struct ata_host *host = dev_get_drvdata(&ofdev->dev); struct sata_fsl_host_priv *host_priv = host->private_data; + device_remove_file(&ofdev->dev, &host_priv->intr_coalescing); + ata_host_detach(host); dev_set_drvdata(&ofdev->dev, NULL);
Reduce interrupt signals through reset Interrupt Coalescing Control Reg. Provide dynamic method to adjust interrupt signals and timer ticks by sysfs. It is a tradeoff for different applications. Signed-off-by: Qiang Liu <qiang.liu@freescale.com> --- change for V2 support dynamic config interrupt coalescing register by /sysfs test random small file with iometer adjust kernel source baseline for apply upstream Description: 1. fsl-sata interrupt will be raised 130 thousand times when write 8G file (dd if=/dev/zero of=/dev/sda2 bs=128K count=65536); 2. most of interrupts raised because of only 1-4 commands completed; 3. only 30 thousand times will be raised after set max interrupt threshold, more interrupts are coalesced as the description of ICC; Test methods and results: 1. test sequential large file performance, [root@p2020ds root]# echo 31 524287 > \ /sys/devices/soc.0/ffe18000.sata/intr_coalescing [root@p2020ds root]# dd if=/dev/zero of=/dev/sda2 bs=128K count=65536 & [root@p2020ds root]# top CPU % | dd | flush-8:0 | softirq --------------------------------------- before | 20-22 | 17-19 | 7 --------------------------------------- after | 18-21 | 15-16 | 5 --------------------------------------- 2. test random small file with iometer, iometer paramters: 4 I/Os burst length, 1MB transfer request size, 100% write, 2MB file size as default configuration of interrupt coalescing register, 1 interrupts and no timeout config, total write performance is 119MB per second, after config with the maximum value, write performance is 110MB per second. After compare the test results, a configuable interrupt coalescing should be better when cope with flexible context. drivers/ata/sata_fsl.c | 111 ++++++++++++++++++++++++++++++++++++++++++++++-- 1 files changed, 107 insertions(+), 4 deletions(-) -- 1.6.4