Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1298226 > unrolled thread

[PATCH v6 2/3] spi: sun4i: Fix clock calculations to be predictable and never exceed the requested rate

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  [PATCH v6 2/3] spi: sun4i: Fix clock calculations to be predictable and never exceed the requested rate Marcus Weseloh <mweseloh42@gmail.com> - 2015-12-26 17:00 +0100
    Re: [PATCH v6 2/3] spi: sun4i: Fix clock calculations to be  predictable and never exceed the requested rate Marcus Weseloh <mweseloh42@gmail.com> - 2015-12-28 03:50 +0100
      Re: [PATCH v6 2/3] spi: sun4i: Fix clock calculations to be  predictable and never exceed the requested rate Marcus Weseloh <mweseloh42@gmail.com> - 2015-12-28 17:30 +0100
    Re: [PATCH v6 2/3] spi: sun4i: Fix clock calculations to be  predictable and never exceed the requested rate Maxime Ripard <maxime.ripard@free-electrons.com> - 2015-12-28 03:50 +0100

#1298226 — [PATCH v6 2/3] spi: sun4i: Fix clock calculations to be predictable and never exceed the requested rate

This patch fixes multiple problems with the current clock calculations:

1. The A10/A20 datasheet contains the formula AHB_CLK / (2^(n+1)) to
calculate SPI_CLK from CDR1, but this formula is wrong. The actual
formula - determined by analyzing the actual waveforms - is
AHB_CLK / (2^n).

2. The divisor calculations for CDR1 and CDR2 both rounded to the
nearest integer. This could lead to a transfer speed that is higher than
the requested speed. This patch changes both calculations to always
round down.

3. The mclk frequency was only ever increased, never decreased. This could
lead to unpredictable transfer speeds, depending on the order in which
transfers with different speeds where serviced by the SPI driver.

Signed-off-by: Marcus Weseloh <mweseloh42@gmail.com>
---
 drivers/spi/spi-sun4i.c | 26 ++++++++++++++++----------
 1 file changed, 16 insertions(+), 10 deletions(-)

diff --git a/drivers/spi/spi-sun4i.c b/drivers/spi/spi-sun4i.c
index f60a6d6..d67e142 100644
--- a/drivers/spi/spi-sun4i.c
+++ b/drivers/spi/spi-sun4i.c
@@ -79,6 +79,9 @@ struct sun4i_spi {
 	struct clk		*hclk;
 	struct clk		*mclk;
 
+	int			cur_max_speed;
+	int			cur_mclk;
+
 	struct completion	done;
 
 	const u8		*tx_buf;
@@ -227,11 +230,17 @@ static int sun4i_spi_transfer_one(struct spi_master *master,
 
 	sun4i_spi_write(sspi, SUN4I_CTL_REG, reg);
 
-	/* Ensure that we have a parent clock fast enough */
+	/*
+	 * Ensure that the parent clock is set to twice the max speed
+	 * of the spi device (possibly rounded up by the clk driver)
+	 */
 	mclk_rate = clk_get_rate(sspi->mclk);
-	if (mclk_rate < (2 * tfr->speed_hz)) {
-		clk_set_rate(sspi->mclk, 2 * tfr->speed_hz);
+	if (spi->max_speed_hz != sspi->cur_max_speed ||
+	    mclk_rate != sspi->cur_mclk) {
+		clk_set_rate(sspi->mclk, 2 * spi->max_speed_hz);
 		mclk_rate = clk_get_rate(sspi->mclk);
+		sspi->cur_mclk = mclk_rate;
+		sspi->cur_max_speed = spi->max_speed_hz;
 	}
 
 	/*
@@ -239,7 +248,7 @@ static int sun4i_spi_transfer_one(struct spi_master *master,
 	 *
 	 * We have two choices there. Either we can use the clock
 	 * divide rate 1, which is calculated thanks to this formula:
-	 * SPI_CLK = MOD_CLK / (2 ^ (cdr + 1))
+	 * SPI_CLK = MOD_CLK / (2 ^ cdr)
 	 * Or we can use CDR2, which is calculated with the formula:
 	 * SPI_CLK = MOD_CLK / (2 * (cdr + 1))
 	 * Wether we use the former or the latter is set through the
@@ -248,14 +257,11 @@ static int sun4i_spi_transfer_one(struct spi_master *master,
 	 * First try CDR2, and if we can't reach the expected
 	 * frequency, fall back to CDR1.
 	 */
-	div = mclk_rate / (2 * tfr->speed_hz);
-	if (div <= (SUN4I_CLK_CTL_CDR2_MASK + 1)) {
-		if (div > 0)
-			div--;
-
+	div = DIV_ROUND_UP(mclk_rate, 2 * tfr->speed_hz) - 1;
+	if (div <= SUN4I_CLK_CTL_CDR2_MASK) {
 		reg = SUN4I_CLK_CTL_CDR2(div) | SUN4I_CLK_CTL_DRS;
 	} else {
-		div = ilog2(mclk_rate) - ilog2(tfr->speed_hz);
+		div = ilog2(roundup_pow_of_two(mclk_rate / tfr->speed_hz));
 		reg = SUN4I_CLK_CTL_CDR1(div);
 	}
 
-- 
1.9.1

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [next] | [standalone]

#1298419 — Re: [PATCH v6 2/3] spi: sun4i: Fix clock calculations to be predictable and never exceed the requested rate

Hi,

2015-12-27 22:09 GMT+01:00 Maxime Ripard <maxime.ripard@free-electrons.com>:
> On Sat, Dec 26, 2015 at 04:53:05PM +0100, Marcus Weseloh wrote:
>> This patch fixes multiple problems with the current clock calculations:
>>
>> 1. The A10/A20 datasheet contains the formula AHB_CLK / (2^(n+1)) to
>> calculate SPI_CLK from CDR1, but this formula is wrong. The actual
>> formula - determined by analyzing the actual waveforms - is
>> AHB_CLK / (2^n).
>>
>> 2. The divisor calculations for CDR1 and CDR2 both rounded to the
>> nearest integer. This could lead to a transfer speed that is higher than
>> the requested speed. This patch changes both calculations to always
>> round down.
>>
>> 3. The mclk frequency was only ever increased, never decreased. This could
>> lead to unpredictable transfer speeds, depending on the order in which
>> transfers with different speeds where serviced by the SPI driver.
>
> These 3 should probably be separate patches (and be Cc'd to stable

Will do. But I have the feeling that at least 1. and 2. should be in
the same patch as they touch the same lines of code. Do you think that
would be ok?
And before CC'ing stable, I would love to have someone with access to
A10 hardware and a scope (or even a bus pirate) check that the A10 SPI
controller does indeed have the same "bug". I strongly think so, but
would sleep better if it could be confirmed.

[...]
>> -     /* Ensure that we have a parent clock fast enough */
>> +     /*
>> +      * Ensure that the parent clock is set to twice the max speed
>> +      * of the spi device (possibly rounded up by the clk driver)
>> +      */
>>       mclk_rate = clk_get_rate(sspi->mclk);
>> -     if (mclk_rate < (2 * tfr->speed_hz)) {
>> -             clk_set_rate(sspi->mclk, 2 * tfr->speed_hz);
>> +     if (spi->max_speed_hz != sspi->cur_max_speed ||
>> +         mclk_rate != sspi->cur_mclk) {
>
> Do you need to cache the values? As far as I'm aware, you end up doing
> the computation all the time anyway.

By caching the values we optimize the case when a single SPI slave
device (or even multiple slave devices with the same max_speed) are
used multiple times in a row. In that case, we're saving two calls:
clk_set_rate and clk_get_rate. I was unsure about how expensive the
clk_* calls were, so I thought it would be safer use caching. But
maybe it's not worth the extra code?

Oh, and I just noticed a mistake in the comment: the clock driver
rounds up _or_ down, so I should drop the "up".

[...]
>> -     div = mclk_rate / (2 * tfr->speed_hz);
>> -     if (div <= (SUN4I_CLK_CTL_CDR2_MASK + 1)) {
>> -             if (div > 0)
>> -                     div--;
>> -
>> +     div = DIV_ROUND_UP(mclk_rate, 2 * tfr->speed_hz) - 1;
>
> Isn't it exactly the same thing as mclk_rate / (2 * tfr->speed_hz) ?

It is quite often, but not in all cases. The plain division rounds to
the nearest integer, so it rounds down sometimes. Consider the
following case: we have a slow SPI device with a spi-max-frequency of
50kHz. Our clock driver can't find a clock as slow as 100kHz, so it
sets mclk to 214,285Hz.

Using the old calculation we get: 214,285 / (2 * 50,000) = 2, so div =
1 as the old code subtracts 1 two lines further down
The new calculation results in: DIV_ROUND_UP(214,285, 2 * 50,000)  =
3, so div = 2 if we add the -1
We end up with a SPI_CLK of 53,571Hz using the old calculation and
35,714Hz using the new one. The old SPI_CLK is obviously closer to the
requested speed, but nevertheless it exceeds the requested limit and
should not have been chosen.

Thanks for the review!

Cheers,

   Marcus
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1298699 — Re: [PATCH v6 2/3] spi: sun4i: Fix clock calculations to be predictable and never exceed the requested rate

Hi again,

2015-12-28 0:29 GMT+01:00 Marcus Weseloh <mweseloh42@gmail.com>:
> 2015-12-27 22:09 GMT+01:00 Maxime Ripard <maxime.ripard@free-electrons.com>:
[...]
> [...]
>>> -     /* Ensure that we have a parent clock fast enough */
>>> +     /*
>>> +      * Ensure that the parent clock is set to twice the max speed
>>> +      * of the spi device (possibly rounded up by the clk driver)
>>> +      */
>>>       mclk_rate = clk_get_rate(sspi->mclk);
>>> -     if (mclk_rate < (2 * tfr->speed_hz)) {
>>> -             clk_set_rate(sspi->mclk, 2 * tfr->speed_hz);
>>> +     if (spi->max_speed_hz != sspi->cur_max_speed ||
>>> +         mclk_rate != sspi->cur_mclk) {
>>
>> Do you need to cache the values? As far as I'm aware, you end up doing
>> the computation all the time anyway.
>
> By caching the values we optimize the case when a single SPI slave
> device (or even multiple slave devices with the same max_speed) are
> used multiple times in a row. In that case, we're saving two calls:
> clk_set_rate and clk_get_rate. I was unsure about how expensive the
> clk_* calls were, so I thought it would be safer use caching. But
> maybe it's not worth the extra code?
>
> Oh, and I just noticed a mistake in the comment: the clock driver
> rounds up _or_ down, so I should drop the "up".

As I'm looking further into this, I think the comment should read
"possibly rounded down", as the clk framework is expected to set a
frequency that is less or equal to the requested frequency. So the
effect I was seeing (that I got a frequency higher than the requested
one) is actually a bug!? Further details below.

> [...]
>>> -     div = mclk_rate / (2 * tfr->speed_hz);
>>> -     if (div <= (SUN4I_CLK_CTL_CDR2_MASK + 1)) {
>>> -             if (div > 0)
>>> -                     div--;
>>> -
>>> +     div = DIV_ROUND_UP(mclk_rate, 2 * tfr->speed_hz) - 1;
>>
>> Isn't it exactly the same thing as mclk_rate / (2 * tfr->speed_hz) ?
>
> It is quite often, but not in all cases. The plain division rounds to
> the nearest integer, so it rounds down sometimes. Consider the
> following case: we have a slow SPI device with a spi-max-frequency of
> 50kHz. Our clock driver can't find a clock as slow as 100kHz, so it
> sets mclk to 214,285Hz.

That clk_set_rate sets a higher frequency than requested seems to be a
problem in itself. I had a look at clk/sunxi/clk-mod0.c and noticed a
few small problems there. Will post an RFC patch in a couple of
minutes.

That doesn't invalidate any of the fixes proposed in this patch,
though. They are still needed, as I see it. But after fixing
clk-mod0.c, we need to make further changes to the spi-sun4i clock
selection, as clk_set_rate could now return -EINVAL. Will amend this
patch as well after receiving feedback on the (soon to come) mod0 clk
patch.

Cheers,

  Marcus
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1298456 — Re: [PATCH v6 2/3] spi: sun4i: Fix clock calculations to be predictable and never exceed the requested rate

[Multipart message — attachments visible in raw view] — view raw

On Sat, Dec 26, 2015 at 04:53:05PM +0100, Marcus Weseloh wrote:
> This patch fixes multiple problems with the current clock calculations:
> 
> 1. The A10/A20 datasheet contains the formula AHB_CLK / (2^(n+1)) to
> calculate SPI_CLK from CDR1, but this formula is wrong. The actual
> formula - determined by analyzing the actual waveforms - is
> AHB_CLK / (2^n).
> 
> 2. The divisor calculations for CDR1 and CDR2 both rounded to the
> nearest integer. This could lead to a transfer speed that is higher than
> the requested speed. This patch changes both calculations to always
> round down.
> 
> 3. The mclk frequency was only ever increased, never decreased. This could
> lead to unpredictable transfer speeds, depending on the order in which
> transfers with different speeds where serviced by the SPI driver.

These 3 should probably be separate patches (and be Cc'd to stable

> Signed-off-by: Marcus Weseloh <mweseloh42@gmail.com>
> ---
>  drivers/spi/spi-sun4i.c | 26 ++++++++++++++++----------
>  1 file changed, 16 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/spi/spi-sun4i.c b/drivers/spi/spi-sun4i.c
> index f60a6d6..d67e142 100644
> --- a/drivers/spi/spi-sun4i.c
> +++ b/drivers/spi/spi-sun4i.c
> @@ -79,6 +79,9 @@ struct sun4i_spi {
>  	struct clk		*hclk;
>  	struct clk		*mclk;
>  
> +	int			cur_max_speed;
> +	int			cur_mclk;
> +
>  	struct completion	done;
>  
>  	const u8		*tx_buf;
> @@ -227,11 +230,17 @@ static int sun4i_spi_transfer_one(struct spi_master *master,
>  
>  	sun4i_spi_write(sspi, SUN4I_CTL_REG, reg);
>  
> -	/* Ensure that we have a parent clock fast enough */
> +	/*
> +	 * Ensure that the parent clock is set to twice the max speed
> +	 * of the spi device (possibly rounded up by the clk driver)
> +	 */
>  	mclk_rate = clk_get_rate(sspi->mclk);
> -	if (mclk_rate < (2 * tfr->speed_hz)) {
> -		clk_set_rate(sspi->mclk, 2 * tfr->speed_hz);
> +	if (spi->max_speed_hz != sspi->cur_max_speed ||
> +	    mclk_rate != sspi->cur_mclk) {

Do you need to cache the values? As far as I'm aware, you end up doing
the computation all the time anyway.

> +		clk_set_rate(sspi->mclk, 2 * spi->max_speed_hz);
>  		mclk_rate = clk_get_rate(sspi->mclk);
> +		sspi->cur_mclk = mclk_rate;
> +		sspi->cur_max_speed = spi->max_speed_hz;
>  	}
>  
>  	/*
> @@ -239,7 +248,7 @@ static int sun4i_spi_transfer_one(struct spi_master *master,
>  	 *
>  	 * We have two choices there. Either we can use the clock
>  	 * divide rate 1, which is calculated thanks to this formula:
> -	 * SPI_CLK = MOD_CLK / (2 ^ (cdr + 1))
> +	 * SPI_CLK = MOD_CLK / (2 ^ cdr)
>
>  	 * Or we can use CDR2, which is calculated with the formula:
>  	 * SPI_CLK = MOD_CLK / (2 * (cdr + 1))
>  	 * Wether we use the former or the latter is set through the
> @@ -248,14 +257,11 @@ static int sun4i_spi_transfer_one(struct spi_master *master,
>  	 * First try CDR2, and if we can't reach the expected
>  	 * frequency, fall back to CDR1.
>  	 */
> -	div = mclk_rate / (2 * tfr->speed_hz);
> -	if (div <= (SUN4I_CLK_CTL_CDR2_MASK + 1)) {
> -		if (div > 0)
> -			div--;
> -
> +	div = DIV_ROUND_UP(mclk_rate, 2 * tfr->speed_hz) - 1;

Isn't it exactly the same thing as mclk_rate / (2 * tfr->speed_hz) ?

Thanks,
Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web