[PATCH v1 0/2] spi: pxa2xx: allow to set mostly any baudrate for Quark case

[PATCH v1 0/2] spi: pxa2xx: allow to set mostly any baudrate for Quark case

[Andy Shevchenko]

Currently driver relies on a table that is really unflexible and contains only
small subset of all possible variabts.

The patch series replaces the table by an approximation.

The code has been tested on Galileo Gen 1 board with loopback mode enabled.

Mark, it would be nice to have this in 4.0-rcX since it will be the first
kernel with Quark support. And I applied necessary patch to allow user to
select SPI driver for it, though it's up to you.

P.S. It was my bad I had to insist to use the approximation instead of some
unflexible tables in the first place.

Andy Shevchenko (2):
  spi: pxa2xx: shift clk_div in one place
  spi: pxa2xx: replace ugly table by approximation

 drivers/spi/spi-pxa2xx.c | 161 +++++++++++++++++++++++++++--------------------
 1 file changed, 94 insertions(+), 67 deletions(-)

-- 
2.1.4

--
To unsubscribe from this list: send the line "unsubscribe linux-spi" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v1 1/2] spi: pxa2xx: shift clk_div in one place

[Andy Shevchenko]

This patch refactors ssp_get_clk_div() and pxa2xx_ssp_get_clk_div() to align
clk_div calculations, i.e. ssp_get_clk_div() and quark_x1000_set_clk_regvals()
will return plain clk_div and it will be shifted to proper position in
pxa2xx_ssp_get_clk_div().

Signed-off-by: Andy Shevchenko <andriy.shevchenko-VuQAYsv1563Yd54FQh9/CA@public.gmane.org>
---
 drivers/spi/spi-pxa2xx.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/drivers/spi/spi-pxa2xx.c b/drivers/spi/spi-pxa2xx.c
index 6f72ad0..dd56bf5 100644
--- a/drivers/spi/spi-pxa2xx.c
+++ b/drivers/spi/spi-pxa2xx.c
@@ -730,23 +730,23 @@ static unsigned int ssp_get_clk_div(struct driver_data *drv_data, int rate)
 	rate = min_t(int, ssp_clk, rate);
 
 	if (ssp->type == PXA25x_SSP || ssp->type == CE4100_SSP)
-		return ((ssp_clk / (2 * rate) - 1) & 0xff) << 8;
+		return (ssp_clk / (2 * rate) - 1) & 0xff;
 	else
-		return ((ssp_clk / rate - 1) & 0xfff) << 8;
+		return (ssp_clk / rate - 1) & 0xfff;
 }
 
 static unsigned int pxa2xx_ssp_get_clk_div(struct driver_data *drv_data,
 					   struct chip_data *chip, int rate)
 {
-	u32 clk_div;
+	unsigned int clk_div;
 
 	switch (drv_data->ssp_type) {
 	case QUARK_X1000_SSP:
 		quark_x1000_set_clk_regvals(rate, &chip->dds_rate, &clk_div);
-		return clk_div << 8;
 	default:
-		return ssp_get_clk_div(drv_data, rate);
+		clk_div = ssp_get_clk_div(drv_data, rate);
 	}
+	return clk_div << 8;
 }
 
 static void pump_transfers(unsigned long data)
-- 
2.1.4

--
To unsubscribe from this list: send the line "unsubscribe linux-spi" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v1 2/2] spi: pxa2xx: replace ugly table by approximation

[Andy Shevchenko]

The Quark SoC data sheet describes the baud rate setting using fractional
divider. The subset of possible values represented by a table suggests that the
divisor has one block that could divide by 5. This explains a satan number in
some cases in the table Thus, in this particular case the divisor can be
evaluated as

	5^i * 2^j * 2 * k,

where

	i = [0, 1]
	j = [0, 23]
	k = [1, 256]

There are few special cases as mentioned in the data sheet, i.e. better form of
the clock signal will be in case if DDS_CLK_RATE either 2^n or 2/5. It's also
possible to use any value that is less or equal to 0x33333 (1/5/16 = 1/80).

All three cases are compared to each other and the one that suits better is
chosen by approximation algorithm. Anyone can play with the script [1] that
represents the algorithm.

[1] https://gist.github.com/06b084488b3629898121

Signed-off-by: Andy Shevchenko <andriy.shevchenko-VuQAYsv1563Yd54FQh9/CA@public.gmane.org>
---
 drivers/spi/spi-pxa2xx.c | 151 ++++++++++++++++++++++++++++-------------------
 1 file changed, 89 insertions(+), 62 deletions(-)

diff --git a/drivers/spi/spi-pxa2xx.c b/drivers/spi/spi-pxa2xx.c
index dd56bf5..6212802 100644
--- a/drivers/spi/spi-pxa2xx.c
+++ b/drivers/spi/spi-pxa2xx.c
@@ -20,6 +20,7 @@
 #include <linux/errno.h>
 #include <linux/err.h>
 #include <linux/interrupt.h>
+#include <linux/kernel.h>
 #include <linux/platform_device.h>
 #include <linux/spi/pxa2xx_spi.h>
 #include <linux/spi/spi.h>
@@ -67,54 +68,6 @@ MODULE_ALIAS("platform:pxa2xx-spi");
 #define LPSS_TX_LOTHRESH_DFLT	160
 #define LPSS_TX_HITHRESH_DFLT	224
 
-struct quark_spi_rate {
-	u32 bitrate;
-	u32 dds_clk_rate;
-	u32 clk_div;
-};
-
-/*
- * 'rate', 'dds', 'clk_div' lookup table, which is defined in
- * the Quark SPI datasheet.
- */
-static const struct quark_spi_rate quark_spi_rate_table[] = {
-/*	bitrate,	dds_clk_rate,	clk_div */
-	{50000000,	0x800000,	0},
-	{40000000,	0x666666,	0},
-	{25000000,	0x400000,	0},
-	{20000000,	0x666666,	1},
-	{16667000,	0x800000,	2},
-	{13333000,	0x666666,	2},
-	{12500000,	0x200000,	0},
-	{10000000,	0x800000,	4},
-	{8000000,	0x666666,	4},
-	{6250000,	0x400000,	3},
-	{5000000,	0x400000,	4},
-	{4000000,	0x666666,	9},
-	{3125000,	0x80000,	0},
-	{2500000,	0x400000,	9},
-	{2000000,	0x666666,	19},
-	{1563000,	0x40000,	0},
-	{1250000,	0x200000,	9},
-	{1000000,	0x400000,	24},
-	{800000,	0x666666,	49},
-	{781250,	0x20000,	0},
-	{625000,	0x200000,	19},
-	{500000,	0x400000,	49},
-	{400000,	0x666666,	99},
-	{390625,	0x10000,	0},
-	{250000,	0x400000,	99},
-	{200000,	0x666666,	199},
-	{195313,	0x8000,		0},
-	{125000,	0x100000,	49},
-	{100000,	0x200000,	124},
-	{50000,		0x100000,	124},
-	{25000,		0x80000,	124},
-	{10016,		0x20000,	77},
-	{5040,		0x20000,	154},
-	{1002,		0x8000,		194},
-};
-
 /* Offset from drv_data->lpss_base */
 #define GENERAL_REG		0x08
 #define GENERAL_REG_RXTO_HOLDOFF_DISABLE BIT(24)
@@ -701,25 +654,99 @@ static irqreturn_t ssp_int(int irq, void *dev_id)
 }
 
 /*
- * The Quark SPI data sheet gives a table, and for the given 'rate',
- * the 'dds' and 'clk_div' can be found in the table.
+ * The Quark SPI has an additional 24 bit register to multiply input frequency
+ * by fractions of 2^24. It also has a divider by 5.
+ *
+ * Fssp = Fsys * DDS_CLK_RATE / 2**24
+ * Baud rate = Fsclk = Fssp / (2 * (SCR + 1))
+ *
+ * DDS_CLK_RATE either 2^n or 2^n / 5.
+ * SCR is in range 0 .. 255
+ *
+ * Divisor = 5^i * 2^j * 2 * k
+ *       i = [0, 1]      i = 1 iff j = 0 or j > 3
+ *       j = [0, 23]     j = 0 iff i = 1
+ *       k = [1, 256]
+ * Special case: j = 0, i = 1: Divisor = 2 / 5
+ *
+ * We can also specify any value of DDS_CLK_RATE that is less or equal 1 / 80,
+ * i.e. 0x33333.
  */
-static u32 quark_x1000_set_clk_regvals(u32 rate, u32 *dds, u32 *clk_div)
+static unsigned int quark_x1000_get_clk_div(int rate, u32 *dds)
 {
-	unsigned int i;
-
-	for (i = 0; i < ARRAY_SIZE(quark_spi_rate_table); i++) {
-		if (rate >= quark_spi_rate_table[i].bitrate) {
-			*dds = quark_spi_rate_table[i].dds_clk_rate;
-			*clk_div = quark_spi_rate_table[i].clk_div;
-			return quark_spi_rate_table[i].bitrate;
+	/* Quark SoC has 200MHz xtal */
+	unsigned long xtal = 200000000, fref = xtal / 2;
+	/* Define two reference frequencies */
+	unsigned long fref1 = fref / 2, fref2 = fref * 2 / 5;
+	/* Keep quots and remainders for different cases here */
+	unsigned long q, q1, q2;
+	unsigned long scale;
+	long r, r1, r2;
+	u32 mul, mul1;
+
+	/* Set initial value for DDS_CLK_RATE */
+	mul1 = (1 << 24) >> 1;
+
+	/* Calculate initial quot */
+	q1 = DIV_ROUND_CLOSEST(fref1, rate);
+
+	/* Scale q1 if it's too big */
+	if (q1 > 256) {
+		/* Scale q1 to range [1, 512] */
+		scale = fls_long(q1 - 1);
+		if (scale > 9) {
+			q1 >>= scale - 9;
+			mul1 >>= scale - 9;
 		}
+
+		/* Round the result if we have a remainder */
+		q1 += q1 & 1;
 	}
 
-	*dds = quark_spi_rate_table[i-1].dds_clk_rate;
-	*clk_div = quark_spi_rate_table[i-1].clk_div;
+	/* Decrease DDS_CLK_RATE as much as we can without loss in precision */
+	scale = __ffs(q1);
+	q1 >>= scale;
+	mul1 >>= scale;
+
+	/* Get the remainder */
+	r1 = abs(fref1 / (1 << (24 - fls_long(mul1))) / q1 - rate);
+
+	q2 = DIV_ROUND_CLOSEST(fref2, rate);
+	r2 = abs(fref2 / q2 - rate);
+
+	/* Choose the best between two */
+	if (r2 >= r1 || q2 > 256) {
+		r = r1;
+		q = q1;
+		mul = mul1;
+	} else {
+		r = r2;
+		q = q2;
+		mul = (1 << 24) * 2 / 5;		/* 0x666666 */
+	}
+
+	/* In case the divisor is big enough */
+	if (fref / rate >= 80) {
+		u64 fssp;
+
+		/* Calculate initial quot */
+		q1 = DIV_ROUND_CLOSEST(fref, rate);
+		mul1 = (1 << 24) / q1;
+
+		/* Get the remainder */
+		fssp = (u64)fref * mul1;
+		do_div(fssp, 1 << 24);
+		r1 = abs(fssp - rate);
+
+		/* Choose this one if it suits better */
+		if (r1 < r) {
+			q = 1;
+			mul = mul1;
+		}
+	}
 
-	return quark_spi_rate_table[i-1].bitrate;
+	*dds = mul;
+	return q - 1;
 }
 
 static unsigned int ssp_get_clk_div(struct driver_data *drv_data, int rate)
@@ -742,7 +769,7 @@ static unsigned int pxa2xx_ssp_get_clk_div(struct driver_data *drv_data,
 
 	switch (drv_data->ssp_type) {
 	case QUARK_X1000_SSP:
-		quark_x1000_set_clk_regvals(rate, &chip->dds_rate, &clk_div);
+		clk_div = quark_x1000_get_clk_div(rate, &chip->dds_rate);
 	default:
 		clk_div = ssp_get_clk_div(drv_data, rate);
 	}
-- 
2.1.4

--
To unsubscribe from this list: send the line "unsubscribe linux-spi" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v1 0/2] spi: pxa2xx: allow to set mostly any baudrate for Quark case

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 359 bytes --]

On Tue, Mar 24, 2015 at 05:43:20PM +0200, Andy Shevchenko wrote:

> Mark, it would be nice to have this in 4.0-rcX since it will be the first
> kernel with Quark support. And I applied necessary patch to allow user to
> select SPI driver for it, though it's up to you.

We're at -rc5 now, that seems too late to try to get a new feature into
v4.0 I'm afraid.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

Re: [PATCH v1 0/2] spi: pxa2xx: allow to set mostly any baudrate for Quark case

[Andy Shevchenko]

On Tue, 2015-03-24 at 09:23 -0700, Mark Brown wrote:
> On Tue, Mar 24, 2015 at 05:43:20PM +0200, Andy Shevchenko wrote:
> 
> > Mark, it would be nice to have this in 4.0-rcX since it will be the first
> > kernel with Quark support. And I applied necessary patch to allow user to
> > select SPI driver for it, though it's up to you.
> 
> We're at -rc5 now, that seems too late to try to get a new feature into
> v4.0 I'm afraid.

I'm fine with it, we may push this to stable release then.

-- 
Andy Shevchenko <andriy.shevchenko-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
Intel Finland Oy

--
To unsubscribe from this list: send the line "unsubscribe linux-spi" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v1 0/2] spi: pxa2xx: allow to set mostly any baudrate for Quark case

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 360 bytes --]

On Tue, Mar 24, 2015 at 06:27:32PM +0200, Andy Shevchenko wrote:
> On Tue, 2015-03-24 at 09:23 -0700, Mark Brown wrote:

> > We're at -rc5 now, that seems too late to try to get a new feature into
> > v4.0 I'm afraid.

> I'm fine with it, we may push this to stable release then.

No, this doesn't seem like -stable material either - LTSI would be a
good fit.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

Re: [PATCH v1 2/2] spi: pxa2xx: replace ugly table by approximation

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 1767 bytes --]

On Tue, Mar 24, 2015 at 05:43:22PM +0200, Andy Shevchenko wrote:
> The Quark SoC data sheet describes the baud rate setting using fractional
> divider. The subset of possible values represented by a table suggests that the
> divisor has one block that could divide by 5. This explains a satan number in
> some cases in the table Thus, in this particular case the divisor can be
> evaluated as

What is a "satan number"?

> +	/* Quark SoC has 200MHz xtal */
> +	unsigned long xtal = 200000000, fref = xtal / 2;
> +	/* Define two reference frequencies */
> +	unsigned long fref1 = fref / 2, fref2 = fref * 2 / 5;

The Linux coding style generally frowns on multiple initializaitons on a
single line for legibility reasons.

> +	/* Choose the best between two */
> +	if (r2 >= r1 || q2 > 256) {
> +		r = r1;
> +		q = q1;
> +		mul = mul1;
> +	} else {
> +		r = r2;
> +		q = q2;
> +		mul = (1 << 24) * 2 / 5;		/* 0x666666 */
> +	}

Making the comments on this a bit more explicit might be helpful - a
mention of why it's better to pick pick one of the settings or the other
would help.

> +	/* In case the divisor is big enough */
> +	if (fref / rate >= 80) {
> +		u64 fssp;
> +
> +		/* Calculate initial quot */
> +		q1 = DIV_ROUND_CLOSEST(fref, rate);
> +		mul1 = (1 << 24) / q1;
> +
> +		/* Get the remainder */
> +		fssp = (u64)fref * mul1;
> +		do_div(fssp, 1 << 24);
> +		r1 = abs(fssp - rate);
> +
> +		/* Choose this one if it suits better */
> +		if (r1 < r) {
> +			q = 1;
> +			mul = mul1;
> +		}
> +	}

So what do we do if the divisor is not big enough?  I'm not sure that
one could reasonably expect someone to follow this code without doing
detective work.  I think the biggest thing is that the comments aren't
saying why the code is doing what it's doing.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

Re: [PATCH v1 1/2] spi: pxa2xx: shift clk_div in one place

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 344 bytes --]

On Tue, Mar 24, 2015 at 05:43:21PM +0200, Andy Shevchenko wrote:
> This patch refactors ssp_get_clk_div() and pxa2xx_ssp_get_clk_div() to align
> clk_div calculations, i.e. ssp_get_clk_div() and quark_x1000_set_clk_regvals()
> will return plain clk_div and it will be shifted to proper position in
> pxa2xx_ssp_get_clk_div().

Applied, thanks.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

Re: [PATCH v1 2/2] spi: pxa2xx: replace ugly table by approximation

[Andy Shevchenko]

On Tue, 2015-03-24 at 09:59 -0700, Mark Brown wrote:
> On Tue, Mar 24, 2015 at 05:43:22PM +0200, Andy Shevchenko wrote:
> > The Quark SoC data sheet describes the baud rate setting using fractional
> > divider. The subset of possible values represented by a table suggests that the
> > divisor has one block that could divide by 5. This explains a satan number in
> > some cases in the table Thus, in this particular case the divisor can be
> > evaluated as
> 
> What is a "satan number"?

Number of the beast 666.

> 
> > +	/* Quark SoC has 200MHz xtal */
> > +	unsigned long xtal = 200000000, fref = xtal / 2;
> > +	/* Define two reference frequencies */
> > +	unsigned long fref1 = fref / 2, fref2 = fref * 2 / 5;
> 
> The Linux coding style generally frowns on multiple initializaitons on a
> single line for legibility reasons.

Not a problem, however they are tighten to each other.

> 
> > +	/* Choose the best between two */
> > +	if (r2 >= r1 || q2 > 256) {
> > +		r = r1;
> > +		q = q1;
> > +		mul = mul1;
> > +	} else {
> > +		r = r2;
> > +		q = q2;
> > +		mul = (1 << 24) * 2 / 5;		/* 0x666666 */
> > +	}
> 
> Making the comments on this a bit more explicit might be helpful - a
> mention of why it's better to pick pick one of the settings or the other
> would help.

Okay, I will make it more verbose.

> 
> > +	/* In case the divisor is big enough */
> > +	if (fref / rate >= 80) {
> > +		u64 fssp;
> > +
> > +		/* Calculate initial quot */
> > +		q1 = DIV_ROUND_CLOSEST(fref, rate);
> > +		mul1 = (1 << 24) / q1;
> > +
> > +		/* Get the remainder */
> > +		fssp = (u64)fref * mul1;
> > +		do_div(fssp, 1 << 24);
> > +		r1 = abs(fssp - rate);
> > +
> > +		/* Choose this one if it suits better */
> > +		if (r1 < r) {
> > +			q = 1;
> > +			mul = mul1;
> > +		}
> > +	}
> 
> So what do we do if the divisor is not big enough?  

Then we choose one of the first two variants.

> I'm not sure that
> one could reasonably expect someone to follow this code without doing
> detective work.  I think the biggest thing is that the comments aren't
> saying why the code is doing what it's doing.

Should I cite data sheet in case to explain some branches?

-- 
Andy Shevchenko <andriy.shevchenko-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
Intel Finland Oy

--
To unsubscribe from this list: send the line "unsubscribe linux-spi" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v1 2/2] spi: pxa2xx: replace ugly table by approximation

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 434 bytes --]

On Wed, Mar 25, 2015 at 12:37:45PM +0200, Andy Shevchenko wrote:
> On Tue, 2015-03-24 at 09:59 -0700, Mark Brown wrote:

> > I'm not sure that
> > one could reasonably expect someone to follow this code without doing
> > detective work.  I think the biggest thing is that the comments aren't
> > saying why the code is doing what it's doing.

> Should I cite data sheet in case to explain some branches?

Yes, that should be helpful.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

Re: [PATCH v1 2/2] spi: pxa2xx: replace ugly table by approximation

[Andy Shevchenko]

On Wed, 2015-03-25 at 08:17 -0700, Mark Brown wrote:
> On Wed, Mar 25, 2015 at 12:37:45PM +0200, Andy Shevchenko wrote:
> > On Tue, 2015-03-24 at 09:59 -0700, Mark Brown wrote:
> 
> > > I'm not sure that
> > > one could reasonably expect someone to follow this code without doing
> > > detective work.  I think the biggest thing is that the comments aren't
> > > saying why the code is doing what it's doing.
> 
> > Should I cite data sheet in case to explain some branches?
> 
> Yes, that should be helpful.

I early today sent v2, I hope it becomes more clear.

-- 
Andy Shevchenko <andriy.shevchenko-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
Intel Finland Oy

--
To unsubscribe from this list: send the line "unsubscribe linux-spi" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html