[PATCH 0/3] Optimize R-Car SATA driver

[PATCH 0/3] Optimize R-Car SATA driver

[Sergei Shtylyov]

Hello.

   Here's the set of 3 patches against the Tejun Heo's 'libata.git' repo's
'for-3.11' branch.
   They deal with some shortcomings in the R-Car SATA driver and generally
optimize it. I've come up with them while trying to add NCQ support to this
driver.

[1/3] sata_rcar: kill superfluous code in sata_rcar_bmdma_fill_sg()
[2/3] sata_rcar: correct 'sata_rcar_sht'
[3/3] sata_rcar: add 'base' local variable to some functions

MBR, Sergei

[PATCH 1/3] sata_rcar: kill superfluous code in sata_rcar_bmdma_fill_sg()

[Sergei Shtylyov]

I've modified sata_rcar_bmdma_fill_sg() to take care of splitting long scatter/
gather segments due to the descriptor table transfer counter being only 28 bits
wide (bit 1 to bit 28) but that was in vain as even if 'sata_rcar_sht' specified
a correct 'dma_boundary' field, the DMA and block layers would have split the
S/G segments on the necassary boundaries. Since the driver uses ATA_BMDMA_SHT()
to initilaize 'sata_rcar_sht', the boundary is much smaller, only 0xFFFF, so the
code I've added is even more useless, and it's better to just remove it.

Signed-off-by: Sergei Shtylyov <sergei.shtylyov@cogentembedded.com>

---
 drivers/ata/sata_rcar.c |   24 ++++++------------------
 1 file changed, 6 insertions(+), 18 deletions(-)

Index: libata/drivers/ata/sata_rcar.c
=================================--- libata.orig/drivers/ata/sata_rcar.c
+++ libata/drivers/ata/sata_rcar.c
@@ -474,11 +474,10 @@ static void sata_rcar_bmdma_fill_sg(stru
 	struct ata_port *ap = qc->ap;
 	struct ata_bmdma_prd *prd = ap->bmdma_prd;
 	struct scatterlist *sg;
-	unsigned int si, pi;
+	unsigned int si;
 
-	pi = 0;
 	for_each_sg(qc->sg, sg, qc->n_elem, si) {
-		u32 addr, sg_len, len;
+		u32 addr, sg_len;
 
 		/*
 		 * Note: h/w doesn't support 64-bit, so we unconditionally
@@ -487,24 +486,13 @@ static void sata_rcar_bmdma_fill_sg(stru
 		addr = (u32)sg_dma_address(sg);
 		sg_len = sg_dma_len(sg);
 
-		/* H/w transfer count is only 29 bits long, let's be careful */
-		while (sg_len) {
-			len = sg_len;
-			if (len > 0x1ffffffe)
-				len = 0x1ffffffe;
-
-			prd[pi].addr = cpu_to_le32(addr);
-			prd[pi].flags_len = cpu_to_le32(len);
-			VPRINTK("PRD[%u] = (0x%X, 0x%X)\n", pi, addr, len);
-
-			pi++;
-			sg_len -= len;
-			addr += len;
-		}
+		prd[si].addr = cpu_to_le32(addr);
+		prd[si].flags_len = cpu_to_le32(sg_len);
+		VPRINTK("PRD[%u] = (0x%X, 0x%X)\n", si, addr, sg_len);
 	}
 
 	/* end-of-table flag */
-	prd[pi - 1].addr |= cpu_to_le32(SATA_RCAR_DTEND);
+	prd[si - 1].addr |= cpu_to_le32(SATA_RCAR_DTEND);
 }
 
 static void sata_rcar_qc_prep(struct ata_queued_cmd *qc)

[PATCH 2/3] sata_rcar: correct 'sata_rcar_sht'

[Sergei Shtylyov]

Using ATA_BMDMA_SHT() to intialize 'sata_rcar_sht' was suboptimal as the R-Car
descriptor table transfer counter is 28 bits wide (bit 1 to bit 28), so that the
'dma_boundary' field of 0xFFFF is just too small, as well as the 'sg_tablesize'
field of 128.  Use ATA_BASE_SHT() to initialize 'sata_rcar_sht' instead and give
proper values to the 'dma_boundary' and 'sg_tablesize' fields explicitly. 

Signed-off-by: Sergei Shtylyov <sergei.shtylyov@cogentembedded.com>

---
 drivers/ata/sata_rcar.c |   11 ++++++++++-
 1 file changed, 10 insertions(+), 1 deletion(-)

Index: libata/drivers/ata/sata_rcar.c
=================================--- libata.orig/drivers/ata/sata_rcar.c
+++ libata/drivers/ata/sata_rcar.c
@@ -121,6 +121,8 @@
 /* Descriptor table word 0 bit (when DTA32M = 1) */
 #define SATA_RCAR_DTEND			BIT(0)
 
+#define SATA_RCAR_DMA_BOUNDARY		0x1FFFFFFEUL
+
 struct sata_rcar_priv {
 	void __iomem *base;
 	struct clk *clk;
@@ -575,7 +577,14 @@ static u8 sata_rcar_bmdma_status(struct 
 }
 
 static struct scsi_host_template sata_rcar_sht = {
-	ATA_BMDMA_SHT(DRV_NAME),
+	ATA_BASE_SHT(DRV_NAME),
+	/*
+	 * This controller allows transfer chunks up to 512MB which cross 64KB
+	 * boundaries, therefore the DMA limits are more relaxed than standard
+	 * ATA SFF.
+	 */
+	.sg_tablesize		= ATA_MAX_PRD,
+	.dma_boundary		= SATA_RCAR_DMA_BOUNDARY,
 };
 
 static struct ata_port_operations sata_rcar_port_ops = {

[PATCH 3/3] sata_rcar: add 'base' local variable to some functions

[Sergei Shtylyov]

The 'base' field of 'struct sata_rcar_priv' is used very often throughout the
driver, so it seems worth loading it into a local variable  if it's used more
than once in a function.

While at it, put some unitialized variables after intialized ones for aesthetic
reasons. :-)

Signed-off-by: Sergei Shtylyov <sergei.shtylyov@cogentembedded.com>

---
 drivers/ata/sata_rcar.c |  102 ++++++++++++++++++++++++++----------------------
 1 file changed, 57 insertions(+), 45 deletions(-)

Index: libata/drivers/ata/sata_rcar.c
=================================--- libata.orig/drivers/ata/sata_rcar.c
+++ libata/drivers/ata/sata_rcar.c
@@ -130,41 +130,44 @@ struct sata_rcar_priv {
 
 static void sata_rcar_phy_initialize(struct sata_rcar_priv *priv)
 {
+	void __iomem *base = priv->base;
+
 	/* idle state */
-	iowrite32(0, priv->base + SATAPHYADDR_REG);
+	iowrite32(0, base + SATAPHYADDR_REG);
 	/* reset */
-	iowrite32(SATAPHYRESET_PHYRST, priv->base + SATAPHYRESET_REG);
+	iowrite32(SATAPHYRESET_PHYRST, base + SATAPHYRESET_REG);
 	udelay(10);
 	/* deassert reset */
-	iowrite32(0, priv->base + SATAPHYRESET_REG);
+	iowrite32(0, base + SATAPHYRESET_REG);
 }
 
 static void sata_rcar_phy_write(struct sata_rcar_priv *priv, u16 reg, u32 val,
 				int group)
 {
+	void __iomem *base = priv->base;
 	int timeout;
 
 	/* deassert reset */
-	iowrite32(0, priv->base + SATAPHYRESET_REG);
+	iowrite32(0, base + SATAPHYRESET_REG);
 	/* lane 1 */
-	iowrite32(SATAPHYACCEN_PHYLANE, priv->base + SATAPHYACCEN_REG);
+	iowrite32(SATAPHYACCEN_PHYLANE, base + SATAPHYACCEN_REG);
 	/* write phy register value */
-	iowrite32(val, priv->base + SATAPHYWDATA_REG);
+	iowrite32(val, base + SATAPHYWDATA_REG);
 	/* set register group */
 	if (group)
 		reg |= SATAPHYADDR_PHYRATEMODE;
 	/* write command */
-	iowrite32(SATAPHYADDR_PHYCMD_WRITE | reg, priv->base + SATAPHYADDR_REG);
+	iowrite32(SATAPHYADDR_PHYCMD_WRITE | reg, base + SATAPHYADDR_REG);
 	/* wait for ack */
 	for (timeout = 0; timeout < 100; timeout++) {
-		val = ioread32(priv->base + SATAPHYACK_REG);
+		val = ioread32(base + SATAPHYACK_REG);
 		if (val & SATAPHYACK_PHYACK)
 			break;
 	}
 	if (timeout >= 100)
 		pr_err("%s timeout\n", __func__);
 	/* idle state */
-	iowrite32(0, priv->base + SATAPHYADDR_REG);
+	iowrite32(0, base + SATAPHYADDR_REG);
 }
 
 static void sata_rcar_freeze(struct ata_port *ap)
@@ -180,14 +183,15 @@ static void sata_rcar_freeze(struct ata_
 static void sata_rcar_thaw(struct ata_port *ap)
 {
 	struct sata_rcar_priv *priv = ap->host->private_data;
+	void __iomem *base = priv->base;
 
 	/* ack */
-	iowrite32(~SATA_RCAR_INT_MASK, priv->base + SATAINTSTAT_REG);
+	iowrite32(~SATA_RCAR_INT_MASK, base + SATAINTSTAT_REG);
 
 	ata_sff_thaw(ap);
 
 	/* unmask */
-	iowrite32(0x7ff & ~SATA_RCAR_INT_MASK, priv->base + SATAINTMASK_REG);
+	iowrite32(0x7ff & ~SATA_RCAR_INT_MASK, base + SATAINTMASK_REG);
 }
 
 static void sata_rcar_ioread16_rep(void __iomem *reg, void *buffer, int count)
@@ -509,15 +513,16 @@ static void sata_rcar_bmdma_setup(struct
 {
 	struct ata_port *ap = qc->ap;
 	unsigned int rw = qc->tf.flags & ATA_TFLAG_WRITE;
-	u32 dmactl;
 	struct sata_rcar_priv *priv = ap->host->private_data;
+	void __iomem *base = priv->base;
+	u32 dmactl;
 
 	/* load PRD table addr. */
 	mb();   /* make sure PRD table writes are visible to controller */
-	iowrite32(ap->bmdma_prd_dma, priv->base + ATAPI_DTB_ADR_REG);
+	iowrite32(ap->bmdma_prd_dma, base + ATAPI_DTB_ADR_REG);
 
 	/* specify data direction, triple-check start bit is clear */
-	dmactl = ioread32(priv->base + ATAPI_CONTROL1_REG);
+	dmactl = ioread32(base + ATAPI_CONTROL1_REG);
 	dmactl &= ~(ATAPI_CONTROL1_RW | ATAPI_CONTROL1_STOP);
 	if (dmactl & ATAPI_CONTROL1_START) {
 		dmactl &= ~ATAPI_CONTROL1_START;
@@ -525,7 +530,7 @@ static void sata_rcar_bmdma_setup(struct
 	}
 	if (!rw)
 		dmactl |= ATAPI_CONTROL1_RW;
-	iowrite32(dmactl, priv->base + ATAPI_CONTROL1_REG);
+	iowrite32(dmactl, base + ATAPI_CONTROL1_REG);
 
 	/* issue r/w command */
 	ap->ops->sff_exec_command(ap, &qc->tf);
@@ -534,27 +539,29 @@ static void sata_rcar_bmdma_setup(struct
 static void sata_rcar_bmdma_start(struct ata_queued_cmd *qc)
 {
 	struct ata_port *ap = qc->ap;
-	u32 dmactl;
 	struct sata_rcar_priv *priv = ap->host->private_data;
+	void __iomem *base = priv->base;
+	u32 dmactl;
 
 	/* start host DMA transaction */
-	dmactl = ioread32(priv->base + ATAPI_CONTROL1_REG);
+	dmactl = ioread32(base + ATAPI_CONTROL1_REG);
 	dmactl |= ATAPI_CONTROL1_START;
-	iowrite32(dmactl, priv->base + ATAPI_CONTROL1_REG);
+	iowrite32(dmactl, base + ATAPI_CONTROL1_REG);
 }
 
 static void sata_rcar_bmdma_stop(struct ata_queued_cmd *qc)
 {
 	struct ata_port *ap = qc->ap;
 	struct sata_rcar_priv *priv = ap->host->private_data;
+	void __iomem *base = priv->base;
 	u32 dmactl;
 
 	/* force termination of DMA transfer if active */
-	dmactl = ioread32(priv->base + ATAPI_CONTROL1_REG);
+	dmactl = ioread32(base + ATAPI_CONTROL1_REG);
 	if (dmactl & ATAPI_CONTROL1_START) {
 		dmactl &= ~ATAPI_CONTROL1_START;
 		dmactl |= ATAPI_CONTROL1_STOP;
-		iowrite32(dmactl, priv->base + ATAPI_CONTROL1_REG);
+		iowrite32(dmactl, base + ATAPI_CONTROL1_REG);
 	}
 
 	/* one-PIO-cycle guaranteed wait, per spec, for HDMA1:0 transition */
@@ -564,8 +571,8 @@ static void sata_rcar_bmdma_stop(struct 
 static u8 sata_rcar_bmdma_status(struct ata_port *ap)
 {
 	struct sata_rcar_priv *priv = ap->host->private_data;
-	u32 status;
 	u8 host_stat = 0;
+	u32 status;
 
 	status = ioread32(priv->base + ATAPI_STATUS_REG);
 	if (status & ATAPI_STATUS_DEVINT)
@@ -666,19 +673,19 @@ static irqreturn_t sata_rcar_interrupt(i
 {
 	struct ata_host *host = dev_instance;
 	struct sata_rcar_priv *priv = host->private_data;
-	struct ata_port *ap;
+	void __iomem *base = priv->base;
 	unsigned int handled = 0;
+	struct ata_port *ap;
 	u32 sataintstat;
 	unsigned long flags;
 
 	spin_lock_irqsave(&host->lock, flags);
 
-	sataintstat = ioread32(priv->base + SATAINTSTAT_REG);
+	sataintstat = ioread32(base + SATAINTSTAT_REG);
 	if (!sataintstat)
 		goto done;
 	/* ack */
-	iowrite32(sataintstat & ~SATA_RCAR_INT_MASK,
-		 priv->base + SATAINTSTAT_REG);
+	iowrite32(sataintstat & ~SATA_RCAR_INT_MASK, base + SATAINTSTAT_REG);
 
 	ap = host->ports[0];
 
@@ -699,15 +706,16 @@ static void sata_rcar_setup_port(struct 
 	struct ata_port *ap = host->ports[0];
 	struct ata_ioports *ioaddr = &ap->ioaddr;
 	struct sata_rcar_priv *priv = host->private_data;
+	void __iomem *base = priv->base;
 
 	ap->ops		= &sata_rcar_port_ops;
 	ap->pio_mask	= ATA_PIO4;
 	ap->udma_mask	= ATA_UDMA6;
 	ap->flags	|= ATA_FLAG_SATA;
 
-	ioaddr->cmd_addr = priv->base + SDATA_REG;
-	ioaddr->ctl_addr = priv->base + SSDEVCON_REG;
-	ioaddr->scr_addr = priv->base + SCRSSTS_REG;
+	ioaddr->cmd_addr = base + SDATA_REG;
+	ioaddr->ctl_addr = base + SSDEVCON_REG;
+	ioaddr->scr_addr = base + SCRSSTS_REG;
 	ioaddr->altstatus_addr = ioaddr->ctl_addr;
 
 	ioaddr->data_addr	= ioaddr->cmd_addr + (ATA_REG_DATA << 2);
@@ -725,6 +733,7 @@ static void sata_rcar_setup_port(struct 
 static void sata_rcar_init_controller(struct ata_host *host)
 {
 	struct sata_rcar_priv *priv = host->private_data;
+	void __iomem *base = priv->base;
 	u32 val;
 
 	/* reset and setup phy */
@@ -737,27 +746,27 @@ static void sata_rcar_init_controller(st
 	sata_rcar_phy_write(priv, SATAPCTLR4_REG, 0x28E80000, 0);
 
 	/* SATA-IP reset state */
-	val = ioread32(priv->base + ATAPI_CONTROL1_REG);
+	val = ioread32(base + ATAPI_CONTROL1_REG);
 	val |= ATAPI_CONTROL1_RESET;
-	iowrite32(val, priv->base + ATAPI_CONTROL1_REG);
+	iowrite32(val, base + ATAPI_CONTROL1_REG);
 
 	/* ISM mode, PRD mode, DTEND flag at bit 0 */
-	val = ioread32(priv->base + ATAPI_CONTROL1_REG);
+	val = ioread32(base + ATAPI_CONTROL1_REG);
 	val |= ATAPI_CONTROL1_ISM;
 	val |= ATAPI_CONTROL1_DESE;
 	val |= ATAPI_CONTROL1_DTA32M;
-	iowrite32(val, priv->base + ATAPI_CONTROL1_REG);
+	iowrite32(val, base + ATAPI_CONTROL1_REG);
 
 	/* Release the SATA-IP from the reset state */
-	val = ioread32(priv->base + ATAPI_CONTROL1_REG);
+	val = ioread32(base + ATAPI_CONTROL1_REG);
 	val &= ~ATAPI_CONTROL1_RESET;
-	iowrite32(val, priv->base + ATAPI_CONTROL1_REG);
+	iowrite32(val, base + ATAPI_CONTROL1_REG);
 
 	/* ack and mask */
-	iowrite32(0, priv->base + SATAINTSTAT_REG);
-	iowrite32(0x7ff, priv->base + SATAINTMASK_REG);
+	iowrite32(0, base + SATAINTSTAT_REG);
+	iowrite32(0x7ff, base + SATAINTMASK_REG);
 	/* enable interrupts */
-	iowrite32(ATAPI_INT_ENABLE_SATAINT, priv->base + ATAPI_INT_ENABLE_REG);
+	iowrite32(ATAPI_INT_ENABLE_SATAINT, base + ATAPI_INT_ENABLE_REG);
 }
 
 static int sata_rcar_probe(struct platform_device *pdev)
@@ -824,14 +833,15 @@ static int sata_rcar_remove(struct platf
 {
 	struct ata_host *host = platform_get_drvdata(pdev);
 	struct sata_rcar_priv *priv = host->private_data;
+	void __iomem *base = priv->base;
 
 	ata_host_detach(host);
 
 	/* disable interrupts */
-	iowrite32(0, priv->base + ATAPI_INT_ENABLE_REG);
+	iowrite32(0, base + ATAPI_INT_ENABLE_REG);
 	/* ack and mask */
-	iowrite32(0, priv->base + SATAINTSTAT_REG);
-	iowrite32(0x7ff, priv->base + SATAINTMASK_REG);
+	iowrite32(0, base + SATAINTSTAT_REG);
+	iowrite32(0x7ff, base + SATAINTMASK_REG);
 
 	clk_disable(priv->clk);
 
@@ -843,14 +853,15 @@ static int sata_rcar_suspend(struct devi
 {
 	struct ata_host *host = dev_get_drvdata(dev);
 	struct sata_rcar_priv *priv = host->private_data;
+	void __iomem *base = priv->base;
 	int ret;
 
 	ret = ata_host_suspend(host, PMSG_SUSPEND);
 	if (!ret) {
 		/* disable interrupts */
-		iowrite32(0, priv->base + ATAPI_INT_ENABLE_REG);
+		iowrite32(0, base + ATAPI_INT_ENABLE_REG);
 		/* mask */
-		iowrite32(0x7ff, priv->base + SATAINTMASK_REG);
+		iowrite32(0x7ff, base + SATAINTMASK_REG);
 
 		clk_disable(priv->clk);
 	}
@@ -862,14 +873,15 @@ static int sata_rcar_resume(struct devic
 {
 	struct ata_host *host = dev_get_drvdata(dev);
 	struct sata_rcar_priv *priv = host->private_data;
+	void __iomem *base = priv->base;
 
 	clk_enable(priv->clk);
 
 	/* ack and mask */
-	iowrite32(0, priv->base + SATAINTSTAT_REG);
-	iowrite32(0x7ff, priv->base + SATAINTMASK_REG);
+	iowrite32(0, base + SATAINTSTAT_REG);
+	iowrite32(0x7ff, base + SATAINTMASK_REG);
 	/* enable interrupts */
-	iowrite32(ATAPI_INT_ENABLE_SATAINT, priv->base + ATAPI_INT_ENABLE_REG);
+	iowrite32(ATAPI_INT_ENABLE_SATAINT, base + ATAPI_INT_ENABLE_REG);
 
 	ata_host_resume(host);
 

Re: [PATCH 0/3] Optimize R-Car SATA driver

[Tejun Heo]

On Tue, May 28, 2013 at 02:38:17AM +0400, Sergei Shtylyov wrote:
> Hello.
> 
>    Here's the set of 3 patches against the Tejun Heo's 'libata.git' repo's
> 'for-3.11' branch.
>    They deal with some shortcomings in the R-Car SATA driver and generally
> optimize it. I've come up with them while trying to add NCQ support to this
> driver.
> 
> [1/3] sata_rcar: kill superfluous code in sata_rcar_bmdma_fill_sg()
> [2/3] sata_rcar: correct 'sata_rcar_sht'
> [3/3] sata_rcar: add 'base' local variable to some functions

Applied to libata/for-3.11.

Thanks.

-- 
tejun

Re: [PATCH 0/3] Optimize R-Car SATA driver

[Sergei Shtylyov]

Hello.

On 28-05-2013 4:20, Tejun Heo wrote:

>>     Here's the set of 3 patches against the Tejun Heo's 'libata.git' repo's
>> 'for-3.11' branch.
>>     They deal with some shortcomings in the R-Car SATA driver and generally
>> optimize it. I've come up with them while trying to add NCQ support to this
>> driver.

>> [1/3] sata_rcar: kill superfluous code in sata_rcar_bmdma_fill_sg()
>> [2/3] sata_rcar: correct 'sata_rcar_sht'
>> [3/3] sata_rcar: add 'base' local variable to some functions

> Applied to libata/for-3.11.

    Again, I don't see these commits on git.kernel.org thru cgit...

> Thanks.

WBR, Sergei

Re: [PATCH 0/3] Optimize R-Car SATA driver

[Tejun Heo]

On Tue, May 28, 2013 at 03:53:13PM +0400, Sergei Shtylyov wrote:
> Hello.
> 
> On 28-05-2013 4:20, Tejun Heo wrote:
> 
> >>    Here's the set of 3 patches against the Tejun Heo's 'libata.git' repo's
> >>'for-3.11' branch.
> >>    They deal with some shortcomings in the R-Car SATA driver and generally
> >>optimize it. I've come up with them while trying to add NCQ support to this
> >>driver.
> 
> >>[1/3] sata_rcar: kill superfluous code in sata_rcar_bmdma_fill_sg()
> >>[2/3] sata_rcar: correct 'sata_rcar_sht'
> >>[3/3] sata_rcar: add 'base' local variable to some functions
> 
> >Applied to libata/for-3.11.
> 
>    Again, I don't see these commits on git.kernel.org thru cgit...

Still traveling.  Just pushed out.  :)

-- 
tejun