spi: pxa2xx: allow to set mostly any baudrate for Quark case
Subsystems (3)
cover spi: pxa2xx: allow to set mostly any baudrate for Quark case
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(-)
Signature · 6 lines
2.1.4 -- To unsubscribe from this list: send the line "unsubscribe linux-spi" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html
A message that is not in the archive (sent off-list or not yet received): 1427211802-217454-1-git-send-email-andriy.shevchenko-VuQAYsv1563Yd54FQh9/[email protected]
PATCH 1/2 spi: pxa2xx: shift clk_div in one place
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().
--- drivers/spi/spi-pxa2xx.c | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-)
drivers/spi/spi-pxa2xx.c +5 -5
@@ -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)
Signature · 6 lines
2.1.4 -- To unsubscribe from this list: send the line "unsubscribe linux-spi" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html
A message that is not in the archive (sent off-list or not yet received): 1427211802-217454-2-git-send-email-andriy.shevchenko-VuQAYsv1563Yd54FQh9/[email protected]
Mark Brown
AppliedRe: [PATCH v1 1/2] spi: pxa2xx: shift clk_div in one place
Quoted 4 lines · Andy Shevchenko
This patch refactors
ssp_get_clk_div()andpxa2xx_ssp_get_clk_div()to alignclk_divcalculations, i.e.ssp_get_clk_div()andquark_x1000_set_clk_regvals()will return plainclk_divand it will be shifted to proper position inpxa2xx_ssp_get_clk_div().
Applied, thanks.
PATCH 2/2 spi: pxa2xx: replace ugly table by approximation
In reply to a message not in the archiveThe 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
--- drivers/spi/spi-pxa2xx.c | 151 ++++++++++++++++++++++++++++------------------- 1 file changed, 89 insertions(+), 62 deletions(-)
drivers/spi/spi-pxa2xx.c +89 -62
@@ -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); }
Signature · 6 lines
2.1.4 -- To unsubscribe from this list: send the line "unsubscribe linux-spi" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html
A message that is not in the archive (sent off-list or not yet received): 1427211802-217454-3-git-send-email-andriy.shevchenko-VuQAYsv1563Yd54FQh9/[email protected]
Re: [PATCH v1 2/2] spi: pxa2xx: replace ugly table by approximation
Quoted 5 lines · 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
What is a "satan number"?
drivers/spi/spi-pxa2xx.c static irqreturn_t ssp_int(int irq, void *dev_id) · line 680
Quoted 4 lines
drivers/spi/spi-pxa2xx.c +4
-0+/* 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.
drivers/spi/spi-pxa2xx.c static irqreturn_t ssp_int(int irq, void *dev_id) · line 726
Quoted 10 lines
drivers/spi/spi-pxa2xx.c +10
-0+/* 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.
Quoted 9 lines
drivers/spi/spi-pxa2xx.c +9
-0+/* 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 */
drivers/spi/spi-pxa2xx.c static irqreturn_t ssp_int(int irq, void *dev_id) · line 746
Quoted 10 lines
drivers/spi/spi-pxa2xx.c +10
-0+ 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.
A message that is not in the archive (sent off-list or not yet received): [email protected]
Re: [PATCH v1 2/2] spi: pxa2xx: replace ugly table by approximation
Quoted 8 lines · Mark Brown
Quoted 5 lines · 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
What is a "satan number"?
Number of the beast 666.
Quoted 8 lines
drivers/spi/spi-pxa2xx.c static irqreturn_t ssp_int(int irq, void *dev_id) · line 680
Quoted 5 lines
drivers/spi/spi-pxa2xx.c +4
-0+/* 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.
Quoted 15 lines
drivers/spi/spi-pxa2xx.c static irqreturn_t ssp_int(int irq, void *dev_id) · line 726
Quoted 11 lines
drivers/spi/spi-pxa2xx.c +10
-0+/* 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.
Quoted 14 lines
Quoted 14 lines
drivers/spi/spi-pxa2xx.c +13
-0+/* 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); +
Quoted 8 lines
drivers/spi/spi-pxa2xx.c static irqreturn_t ssp_int(int irq, void *dev_id) · line 746
Quoted 6 lines
drivers/spi/spi-pxa2xx.c +6
-0+/* 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.
Quoted 4 lines
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?
Signature · 7 lines
Andy Shevchenko <[email protected]> Intel Finland Oy -- To unsubscribe from this list: send the line "unsubscribe linux-spi" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html
A message that is not in the archive (sent off-list or not yet received): 1427279865.14897.405.camel-VuQAYsv1563Yd54FQh9/[email protected]
Re: [PATCH v1 2/2] spi: pxa2xx: replace ugly table by approximation
Quoted 8 lines · Andy Shevchenko
Quoted 4 lines · Mark Brown
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.
A message that is not in the archive (sent off-list or not yet received): [email protected]
Re: [PATCH v1 2/2] spi: pxa2xx: replace ugly table by approximation
Quoted 11 lines · Mark Brown
Quoted 8 lines · Andy Shevchenko
Quoted 4 lines · Mark Brown
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.
Signature · 7 lines
Andy Shevchenko <[email protected]> Intel Finland Oy -- To unsubscribe from this list: send the line "unsubscribe linux-spi" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html
Quoted 3 lines · Andy Shevchenko
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.
A message that is not in the archive (sent off-list or not yet received): [email protected]
Quoted 8 lines · Mark Brown
Quoted 3 lines · Andy Shevchenko
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.
Signature · 7 lines
Andy Shevchenko <[email protected]> Intel Finland Oy -- To unsubscribe from this list: send the line "unsubscribe linux-spi" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html
A message that is not in the archive (sent off-list or not yet received): 1427214452.14897.401.camel-VuQAYsv1563Yd54FQh9/[email protected]
Quoted 6 lines · Andy Shevchenko
Quoted 2 lines · Mark Brown
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.