[PATCH 1/2] spi: sh-hspi: Improve performance

[PATCH 1/2] spi: sh-hspi: Improve performance

[Phil Edworthy]

The driver attempts to read the recieved data immediately after
writing to the transmit buffer. If no data is available, the driver
currently waits 20ms until trying again. Since the hardware needs
to shift out the transmitted data, the first poll always fails,
leading to 20ms delay between bytes.

This patch reduces the polling interval to 1us, and also reduces
the timeout to 10ms.

Signed-off-by: Phil Edworthy <phil.edworthy-zM6kxYcvzFBBDgjK7y7TUQ@public.gmane.org>
---
 drivers/spi/spi-sh-hspi.c |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/spi/spi-sh-hspi.c b/drivers/spi/spi-sh-hspi.c
index 934138c..0ca18c8 100644
--- a/drivers/spi/spi-sh-hspi.c
+++ b/drivers/spi/spi-sh-hspi.c
@@ -73,13 +73,13 @@ static u32 hspi_read(struct hspi_priv *hspi, int reg)
  */
 static int hspi_status_check_timeout(struct hspi_priv *hspi, u32 mask, u32 val)
 {
-	int t = 256;
+	int t = 10000; /* 10ms max timeout */
 
 	while (t--) {
 		if ((mask & hspi_read(hspi, SPSR)) == val)
 			return 0;
 
-		msleep(20);
+		udelay(1);
 	}
 
 	dev_err(hspi->dev, "timeout\n");
-- 
1.7.5.4


------------------------------------------------------------------------------
Monitor your physical, virtual and cloud infrastructure from a single
web console. Get in-depth insight into apps, servers, databases, vmware,
SAP, cloud infrastructure, etc. Download 30-day Free Trial.
Pricing starts from $795 for 25 servers or applications!
http://p.sf.net/sfu/zoho_dev2dev_nov

[PATCH 2/2] spi: sh-hspi: add CS manual control support

[Phil Edworthy]

The current HSPI driver used automatic CS control, leading to CS
active for each byte transmitted. This patch changes the driver
to manual CS control, and ensures CS is active thoughout a whole
message. Additionally, it uses the cs_change field to determine
if CS is disabled between transfers in the message.

Signed-off-by: Phil Edworthy <phil.edworthy-zM6kxYcvzFBBDgjK7y7TUQ@public.gmane.org>
---
 drivers/spi/spi-sh-hspi.c |   43 +++++++++++++++++++++++++++++++++++++++++--
 1 files changed, 41 insertions(+), 2 deletions(-)

diff --git a/drivers/spi/spi-sh-hspi.c b/drivers/spi/spi-sh-hspi.c
index 0ca18c8..592e236 100644
--- a/drivers/spi/spi-sh-hspi.c
+++ b/drivers/spi/spi-sh-hspi.c
@@ -68,6 +68,16 @@ static u32 hspi_read(struct hspi_priv *hspi, int reg)
 	return ioread32(hspi->addr + reg);
 }
 
+static void hspi_bit_set(struct hspi_priv *hspi, int reg, u32 mask, u32 set)
+{
+	u32 val = hspi_read(hspi, reg);
+
+	val &= ~mask;
+	val |= set & mask;
+
+	hspi_write(hspi, reg, val);
+}
+
 /*
  *		transfer function
  */
@@ -105,6 +115,13 @@ static int hspi_unprepare_transfer(struct spi_master *master)
 	return 0;
 }
 
+#define hspi_hw_cs_enable(hspi)		hspi_hw_cs_ctrl(hspi, 0)
+#define hspi_hw_cs_disable(hspi)	hspi_hw_cs_ctrl(hspi, 1)
+static void hspi_hw_cs_ctrl(struct hspi_priv *hspi, int hi)
+{
+	hspi_bit_set(hspi, SPSCR, (1 << 6), (hi) << 6);
+}
+
 static void hspi_hw_setup(struct hspi_priv *hspi,
 			  struct spi_message *msg,
 			  struct spi_transfer *t)
@@ -155,7 +172,7 @@ static void hspi_hw_setup(struct hspi_priv *hspi,
 
 	hspi_write(hspi, SPCR, spcr);
 	hspi_write(hspi, SPSR, 0x0);
-	hspi_write(hspi, SPSCR, 0x1);	/* master mode */
+	hspi_write(hspi, SPSCR, 0x21);	/* master mode / CS control */
 }
 
 static int hspi_transfer_one_message(struct spi_master *master,
@@ -166,12 +183,21 @@ static int hspi_transfer_one_message(struct spi_master *master,
 	u32 tx;
 	u32 rx;
 	int ret, i;
+	unsigned int cs_change;
+	const int nsecs = 50;
 
 	dev_dbg(hspi->dev, "%s\n", __func__);
 
+	cs_change = 1;
 	ret = 0;
 	list_for_each_entry(t, &msg->transfers, transfer_list) {
-		hspi_hw_setup(hspi, msg, t);
+
+		if (cs_change) {
+			hspi_hw_setup(hspi, msg, t);
+			hspi_hw_cs_enable(hspi);
+			ndelay(nsecs);
+		}
+		cs_change = t->cs_change;
 
 		for (i = 0; i < t->len; i++) {
 
@@ -198,9 +224,22 @@ static int hspi_transfer_one_message(struct spi_master *master,
 		}
 
 		msg->actual_length += t->len;
+
+		if (t->delay_usecs)
+			udelay(t->delay_usecs);
+
+		if (cs_change) {
+			ndelay(nsecs);
+			hspi_hw_cs_disable(hspi);
+			ndelay(nsecs);
+		}
 	}
 
 	msg->status = ret;
+	if (!cs_change) {
+		ndelay(nsecs);
+		hspi_hw_cs_disable(hspi);
+	}
 	spi_finalize_current_message(master);
 
 	return ret;
-- 
1.7.5.4


------------------------------------------------------------------------------
Monitor your physical, virtual and cloud infrastructure from a single
web console. Get in-depth insight into apps, servers, databases, vmware,
SAP, cloud infrastructure, etc. Download 30-day Free Trial.
Pricing starts from $795 for 25 servers or applications!
http://p.sf.net/sfu/zoho_dev2dev_nov

Re: [PATCH 1/2] spi: sh-hspi: Improve performance

[Grant Likely]

On Thu, 22 Nov 2012 14:37:26 +0000, Phil Edworthy <phil.edworthy-zM6kxYcvzFBBDgjK7y7TUQ@public.gmane.org> wrote:
> The driver attempts to read the recieved data immediately after
> writing to the transmit buffer. If no data is available, the driver
> currently waits 20ms until trying again. Since the hardware needs
> to shift out the transmitted data, the first poll always fails,
> leading to 20ms delay between bytes.
> 
> This patch reduces the polling interval to 1us, and also reduces
> the timeout to 10ms.
> 
> Signed-off-by: Phil Edworthy <phil.edworthy-zM6kxYcvzFBBDgjK7y7TUQ@public.gmane.org>
> ---
>  drivers/spi/spi-sh-hspi.c |    4 ++--
>  1 files changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/spi/spi-sh-hspi.c b/drivers/spi/spi-sh-hspi.c
> index 934138c..0ca18c8 100644
> --- a/drivers/spi/spi-sh-hspi.c
> +++ b/drivers/spi/spi-sh-hspi.c
> @@ -73,13 +73,13 @@ static u32 hspi_read(struct hspi_priv *hspi, int reg)
>   */
>  static int hspi_status_check_timeout(struct hspi_priv *hspi, u32 mask, u32 val)
>  {
> -	int t = 256;
> +	int t = 10000; /* 10ms max timeout */
>  
>  	while (t--) {
>  		if ((mask & hspi_read(hspi, SPSR)) == val)
>  			return 0;
>  
> -		msleep(20);
> +		udelay(1);
>  	}

This does have the side effect that the cpu is now spinning instead of
scheduling something else when the device is busy. Is that what you
want? (the answer depends on how big the fifo is and how fast the SPI
bus runs).

g.


------------------------------------------------------------------------------
Monitor your physical, virtual and cloud infrastructure from a single
web console. Get in-depth insight into apps, servers, databases, vmware,
SAP, cloud infrastructure, etc. Download 30-day Free Trial.
Pricing starts from $795 for 25 servers or applications!
http://p.sf.net/sfu/zoho_dev2dev_nov

Re: [PATCH 2/2] spi: sh-hspi: add CS manual control support

[Grant Likely]

On Thu, 22 Nov 2012 14:37:27 +0000, Phil Edworthy <phil.edworthy-zM6kxYcvzFBBDgjK7y7TUQ@public.gmane.org> wrote:
> The current HSPI driver used automatic CS control, leading to CS
> active for each byte transmitted. This patch changes the driver
> to manual CS control, and ensures CS is active thoughout a whole
> message. Additionally, it uses the cs_change field to determine
> if CS is disabled between transfers in the message.
> 
> Signed-off-by: Phil Edworthy <phil.edworthy-zM6kxYcvzFBBDgjK7y7TUQ@public.gmane.org>

Applied, thanks.

g.


------------------------------------------------------------------------------
Monitor your physical, virtual and cloud infrastructure from a single
web console. Get in-depth insight into apps, servers, databases, vmware,
SAP, cloud infrastructure, etc. Download 30-day Free Trial.
Pricing starts from $795 for 25 servers or applications!
http://p.sf.net/sfu/zoho_dev2dev_nov

Re: [PATCH 1/2] spi: sh-hspi: Improve performance

[phil.edworthy-zM6kxYcvzFBBDgjK7y7TUQ]

Hi Grant,

> Sent by: Grant Likely <glikely-s3s/WqlpOiPyB63q8FvJNQ@public.gmane.org>
>
> On Thu, 22 Nov 2012 14:37:26 +0000, Phil Edworthy
> <phil.edworthy-zM6kxYcvzFBBDgjK7y7TUQ@public.gmane.org> wrote:
> > The driver attempts to read the recieved data immediately after
> > writing to the transmit buffer. If no data is available, the driver
> > currently waits 20ms until trying again. Since the hardware needs
> > to shift out the transmitted data, the first poll always fails,
> > leading to 20ms delay between bytes.
> >
> > This patch reduces the polling interval to 1us, and also reduces
> > the timeout to 10ms.
> >
> > Signed-off-by: Phil Edworthy <phil.edworthy-zM6kxYcvzFBBDgjK7y7TUQ@public.gmane.org>
> > ---
> >  drivers/spi/spi-sh-hspi.c |    4 ++--
> >  1 files changed, 2 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/spi/spi-sh-hspi.c b/drivers/spi/spi-sh-hspi.c
> > index 934138c..0ca18c8 100644
> > --- a/drivers/spi/spi-sh-hspi.c
> > +++ b/drivers/spi/spi-sh-hspi.c
> > @@ -73,13 +73,13 @@ static u32 hspi_read(struct hspi_priv *hspi, int reg)
> >   */
> >  static int hspi_status_check_timeout(struct hspi_priv *hspi, u32
> mask, u32 val)
> >  {
> > -   int t = 256;
> > +   int t = 10000; /* 10ms max timeout */
> >
> >     while (t--) {
> >        if ((mask & hspi_read(hspi, SPSR)) == val)
> >           return 0;
> >
> > -      msleep(20);
> > +      udelay(1);
> >     }
>
> This does have the side effect that the cpu is now spinning instead of
> scheduling something else when the device is busy. Is that what you
> want? (the answer depends on how big the fifo is and how fast the SPI
> bus runs).
That's true... The HW has an 8 byte fifo, but at the moment this driver
doesn't use it. I suppose the sensible thing to do is calculate the time
we expect to wait for the received data to become available, and if this
time is over some threshold, use msleep instead of udelay.

Thanks
Phil
------------------------------------------------------------------------------
Monitor your physical, virtual and cloud infrastructure from a single
web console. Get in-depth insight into apps, servers, databases, vmware,
SAP, cloud infrastructure, etc. Download 30-day Free Trial.
Pricing starts from $795 for 25 servers or applications!
http://p.sf.net/sfu/zoho_dev2dev_nov

[PATCH v2] spi: sh-hspi: Improve performance

[Phil Edworthy]

The driver attempts to read the received data immediately after
writing to the transmit buffer. If no data is available, the driver
currently waits 20ms until trying again. Since the hardware needs
to shift out the transmitted data, the first poll always fails,
leading to 20ms delay between bytes.

This patch waits for the time it takes to transmit a byte before
reading the received data.

Signed-off-by: Phil Edworthy <phil.edworthy-zM6kxYcvzFBBDgjK7y7TUQ@public.gmane.org>
---
v2:
 Instead of replacing msleep(20) with udelay(1), this version now
 calculates the time it takes to shift out a byte, and uses udelay or
 ndelay for this time. After this it falls back to polling every 20ms
 using msleep. The delay has an empirical element based on testing at
 different speeds.

 drivers/spi/spi-sh-hspi.c |   25 +++++++++++++++++++++++++
 1 files changed, 25 insertions(+), 0 deletions(-)

diff --git a/drivers/spi/spi-sh-hspi.c b/drivers/spi/spi-sh-hspi.c
index bc25419..407eac0 100644
--- a/drivers/spi/spi-sh-hspi.c
+++ b/drivers/spi/spi-sh-hspi.c
@@ -53,6 +53,7 @@ struct hspi_priv {
 	struct spi_master *master;
 	struct device *dev;
 	struct clk *clk;
+	u32 actual_speed;
 };
 
 /*
@@ -168,6 +169,7 @@ static void hspi_hw_setup(struct hspi_priv *hspi,
 	if (spi->mode & SPI_CPOL)
 		spcr |= 1 << 6;
 
+	hspi->actual_speed = best_rate;
 	dev_dbg(dev, "speed %d/%d\n", target_rate, best_rate);
 
 	hspi_write(hspi, SPCR, spcr);
@@ -185,6 +187,8 @@ static int hspi_transfer_one_message(struct spi_master *master,
 	int ret, i;
 	unsigned int cs_change;
 	const int nsecs = 50;
+	u32 tx_us = 0;
+	u32 tx_ns = 0;
 
 	dev_dbg(hspi->dev, "%s\n", __func__);
 
@@ -194,6 +198,23 @@ static int hspi_transfer_one_message(struct spi_master *master,
 
 		if (cs_change) {
 			hspi_hw_setup(hspi, msg, t);
+
+			/* Calculate time to shift out 8 bits & get rx data.
+			 * The values of 8x, 9x & 11x are empirically derived,
+			 * using a scope to check that we haven't dropped in
+			 * a 20ms delay between bytes.
+			 * If delay is >1ms, poll using msleep so we don't
+			 * block.
+			 */
+			tx_us = 0;
+			tx_ns = 0;
+			if (hspi->actual_speed > 1000000)
+				tx_ns = 8*1000000/(hspi->actual_speed/1000);
+			else if (hspi->actual_speed > 100000)
+				tx_ns = 9*1000000/(hspi->actual_speed/1000);
+			else if (hspi->actual_speed > 1000)
+				tx_us = 11*1000000/hspi->actual_speed;
+
 			hspi_hw_cs_enable(hspi);
 			ndelay(nsecs);
 		}
@@ -212,6 +233,10 @@ static int hspi_transfer_one_message(struct spi_master *master,
 
 			hspi_write(hspi, SPTBR, tx);
 
+			/* Wait to get recieved data */
+			udelay(tx_us);
+			ndelay(tx_ns);
+
 			/* wait recive */
 			ret = hspi_status_check_timeout(hspi, 0x4, 0x4);
 			if (ret < 0)
-- 
1.7.5.4


------------------------------------------------------------------------------
Monitor your physical, virtual and cloud infrastructure from a single
web console. Get in-depth insight into apps, servers, databases, vmware,
SAP, cloud infrastructure, etc. Download 30-day Free Trial.
Pricing starts from $795 for 25 servers or applications!
http://p.sf.net/sfu/zoho_dev2dev_nov

Re: [PATCH v2] spi: sh-hspi: Improve performance

[Grant Likely]

On Fri, 23 Nov 2012 14:46:24 +0000, Phil Edworthy <phil.edworthy-zM6kxYcvzFBBDgjK7y7TUQ@public.gmane.org> wrote:
> The driver attempts to read the received data immediately after
> writing to the transmit buffer. If no data is available, the driver
> currently waits 20ms until trying again. Since the hardware needs
> to shift out the transmitted data, the first poll always fails,
> leading to 20ms delay between bytes.
> 
> This patch waits for the time it takes to transmit a byte before
> reading the received data.
> 
> Signed-off-by: Phil Edworthy <phil.edworthy-zM6kxYcvzFBBDgjK7y7TUQ@public.gmane.org>
> ---
> v2:
>  Instead of replacing msleep(20) with udelay(1), this version now
>  calculates the time it takes to shift out a byte, and uses udelay or
>  ndelay for this time. After this it falls back to polling every 20ms
>  using msleep. The delay has an empirical element based on testing at
>  different speeds.
> 
>  drivers/spi/spi-sh-hspi.c |   25 +++++++++++++++++++++++++
>  1 files changed, 25 insertions(+), 0 deletions(-)
> 
> diff --git a/drivers/spi/spi-sh-hspi.c b/drivers/spi/spi-sh-hspi.c
> index bc25419..407eac0 100644
> --- a/drivers/spi/spi-sh-hspi.c
> +++ b/drivers/spi/spi-sh-hspi.c
> @@ -53,6 +53,7 @@ struct hspi_priv {
>  	struct spi_master *master;
>  	struct device *dev;
>  	struct clk *clk;
> +	u32 actual_speed;
>  };
>  
>  /*
> @@ -168,6 +169,7 @@ static void hspi_hw_setup(struct hspi_priv *hspi,
>  	if (spi->mode & SPI_CPOL)
>  		spcr |= 1 << 6;
>  
> +	hspi->actual_speed = best_rate;
>  	dev_dbg(dev, "speed %d/%d\n", target_rate, best_rate);
>  
>  	hspi_write(hspi, SPCR, spcr);
> @@ -185,6 +187,8 @@ static int hspi_transfer_one_message(struct spi_master *master,
>  	int ret, i;
>  	unsigned int cs_change;
>  	const int nsecs = 50;
> +	u32 tx_us = 0;
> +	u32 tx_ns = 0;
>  
>  	dev_dbg(hspi->dev, "%s\n", __func__);
>  
> @@ -194,6 +198,23 @@ static int hspi_transfer_one_message(struct spi_master *master,
>  
>  		if (cs_change) {
>  			hspi_hw_setup(hspi, msg, t);
> +
> +			/* Calculate time to shift out 8 bits & get rx data.
> +			 * The values of 8x, 9x & 11x are empirically derived,
> +			 * using a scope to check that we haven't dropped in
> +			 * a 20ms delay between bytes.
> +			 * If delay is >1ms, poll using msleep so we don't
> +			 * block.
> +			 */
> +			tx_us = 0;
> +			tx_ns = 0;
> +			if (hspi->actual_speed > 1000000)
> +				tx_ns = 8*1000000/(hspi->actual_speed/1000);
> +			else if (hspi->actual_speed > 100000)
> +				tx_ns = 9*1000000/(hspi->actual_speed/1000);
> +			else if (hspi->actual_speed > 1000)
> +				tx_us = 11*1000000/hspi->actual_speed;
> +
>  			hspi_hw_cs_enable(hspi);
>  			ndelay(nsecs);

This is still just busywaiting. Which means just burning CPU cycles
unconditionally even if it does finish early.

Instead of doing nothing, it is actually a whole lot better to use a
small delay and check if there is stuff to do frequently than it is to
try and delay for the entire duration. Due to the way this driver is
architected, nothing else can use the CPU during the delay().

What would really be better is to load up the fifo and full as it will
go and then rely on IRQs to tell the driver when it becomes ready to
fill with more data.

g.


------------------------------------------------------------------------------
LogMeIn Rescue: Anywhere, Anytime Remote support for IT. Free Trial
Remotely access PCs and mobile devices and provide instant support
Improve your efficiency, and focus on delivering more value-add services
Discover what IT Professionals Know. Rescue delivers
http://p.sf.net/sfu/logmein_12329d2d

Re: [PATCH v2] spi: sh-hspi: Improve performance

[phil.edworthy-zM6kxYcvzFBBDgjK7y7TUQ]

Hi Grant,

> <phil.edworthy-zM6kxYcvzFBBDgjK7y7TUQ@public.gmane.org> wrote:
> > The driver attempts to read the received data immediately after
> > writing to the transmit buffer. If no data is available, the driver
> > currently waits 20ms until trying again. Since the hardware needs
> > to shift out the transmitted data, the first poll always fails,
> > leading to 20ms delay between bytes.
> >
> > This patch waits for the time it takes to transmit a byte before
> > reading the received data.
> >
> > Signed-off-by: Phil Edworthy <phil.edworthy-zM6kxYcvzFBBDgjK7y7TUQ@public.gmane.org>
> > ---
> > v2:
> >  Instead of replacing msleep(20) with udelay(1), this version now
> >  calculates the time it takes to shift out a byte, and uses udelay or
> >  ndelay for this time. After this it falls back to polling every 20ms
> >  using msleep. The delay has an empirical element based on testing at
> >  different speeds.
<snip>

> This is still just busywaiting. Which means just burning CPU cycles
> unconditionally even if it does finish early.
>
> Instead of doing nothing, it is actually a whole lot better to use a
> small delay and check if there is stuff to do frequently than it is to
> try and delay for the entire duration. Due to the way this driver is
> architected, nothing else can use the CPU during the delay().
>
> What would really be better is to load up the fifo and full as it will
> go and then rely on IRQs to tell the driver when it becomes ready to
> fill with more data.

Ok, I see now. I'll look at using dma instead though, as that will be better in the long run.

Thanks for your comments,
Phil
------------------------------------------------------------------------------
LogMeIn Rescue: Anywhere, Anytime Remote support for IT. Free Trial
Remotely access PCs and mobile devices and provide instant support
Improve your efficiency, and focus on delivering more value-add services
Discover what IT Professionals Know. Rescue delivers
http://p.sf.net/sfu/logmein_12329d2d