[PATCH] spi: sun4i: allow transfers to set transmission speed

[PATCH] spi: sun4i: allow transfers to set transmission speed

[Marcus Weseloh]

Allow transfers to set the transmission speed, only fall back to
board max_speed_hz if requested speed is not set or invalid.

Signed-off-by: Marcus Weseloh <mweseloh42-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
---
 drivers/spi/spi-sun4i.c | 15 +++++++++++----
 1 file changed, 11 insertions(+), 4 deletions(-)

diff --git a/drivers/spi/spi-sun4i.c b/drivers/spi/spi-sun4i.c
index fbb0a4d..0630691 100644
--- a/drivers/spi/spi-sun4i.c
+++ b/drivers/spi/spi-sun4i.c
@@ -173,6 +173,7 @@ static int sun4i_spi_transfer_one(struct spi_master *master,
 	unsigned int tx_len = 0;
 	int ret = 0;
 	u32 reg;
+	u32 speed;
 
 	/* We don't support transfer larger than the FIFO */
 	if (tfr->len > SUN4I_FIFO_DEPTH)
@@ -227,10 +228,16 @@ static int sun4i_spi_transfer_one(struct spi_master *master,
 
 	sun4i_spi_write(sspi, SUN4I_CTL_REG, reg);
 
+	/* Transfer speed setup with fallback to board max_speed_hz */
+	if (tfr->speed_hz > 0 && tfr->speed_hz <= spi->max_speed_hz)
+		speed = tfr->speed_hz;
+	else
+		speed = spi->max_speed_hz;
+
 	/* Ensure that we have a parent clock fast enough */
 	mclk_rate = clk_get_rate(sspi->mclk);
-	if (mclk_rate < (2 * spi->max_speed_hz)) {
-		clk_set_rate(sspi->mclk, 2 * spi->max_speed_hz);
+	if (mclk_rate < (2 * speed)) {
+		clk_set_rate(sspi->mclk, 2 * speed);
 		mclk_rate = clk_get_rate(sspi->mclk);
 	}
 
@@ -248,14 +255,14 @@ 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 * spi->max_speed_hz);
+	div = mclk_rate / (2 * speed);
 	if (div <= (SUN4I_CLK_CTL_CDR2_MASK + 1)) {
 		if (div > 0)
 			div--;
 
 		reg = SUN4I_CLK_CTL_CDR2(div) | SUN4I_CLK_CTL_DRS;
 	} else {
-		div = ilog2(mclk_rate) - ilog2(spi->max_speed_hz);
+		div = ilog2(mclk_rate) - ilog2(speed);
 		reg = SUN4I_CLK_CTL_CDR1(div);
 	}
 
-- 
1.9.1

Re: [PATCH] spi: sun4i: allow transfers to set transmission speed

[Mark Brown]

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

On Sat, Nov 07, 2015 at 11:41:05PM +0100, Marcus Weseloh wrote:

> +	/* Transfer speed setup with fallback to board max_speed_hz */
> +	if (tfr->speed_hz > 0 && tfr->speed_hz <= spi->max_speed_hz)
> +		speed = tfr->speed_hz;
> +	else
> +		speed = spi->max_speed_hz;

This is not needed, the core will always ensure that the transfer speed
is set per on each transfer.

Re: [PATCH] spi: sun4i: allow transfers to set transmission speed

[Marcus Weseloh]

Hi Mark,

2015-11-08 10:35 GMT+01:00 Mark Brown <broonie-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>:
> On Sat, Nov 07, 2015 at 11:41:05PM +0100, Marcus Weseloh wrote:
>
>> +     /* Transfer speed setup with fallback to board max_speed_hz */
>> +     if (tfr->speed_hz > 0 && tfr->speed_hz <= spi->max_speed_hz)
>> +             speed = tfr->speed_hz;
>> +     else
>> +             speed = spi->max_speed_hz;
>
> This is not needed, the core will always ensure that the transfer speed
> is set per on each transfer.

Ah, so we could always use tfr->speed_hz and completely ignore the
spi->max_speed_hz for the clock calculations, correct?

I did notice however, that the SPI system doesn't validate the
speed_hz value. So if I leave out the above quoted sanity checking,
then tfr->speed_hz could also contain values like -400, which trips up
the spi system in such a way that I could only repair with a reboot. I
was testing this with spidev... so maybe spidev should be patched to
check the speed value for validity?

Cheers,
  Marcus
--
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] spi: sun4i: allow transfers to set transmission speed

[Marcus Weseloh]

2015-11-08 11:13 GMT+01:00 Marcus Weseloh <mweseloh42-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>:
> I did notice however, that the SPI system doesn't validate the
> speed_hz value. So if I leave out the above quoted sanity checking,
> then tfr->speed_hz could also contain values like -400, which trips up
> the spi system in such a way that I could only repair with a reboot. I
> was testing this with spidev... so maybe spidev should be patched to
> check the speed value for validity?

I just had a look through the spi core and noticed that it does indeed
validate the maximum and minium speed if set correctly on the spi
master. So I guess it's just a matter of a missing minimum transfer
speed in the DT. Sorry, should have done that earlier.

Ok, I will prepare a new patch version that simply uses tfr->speed_hz.
And I will also change sun6i-spi in the same patch, as sun4i and sun6i
are using the same transfer logic.

Cheers,

   Marcus

Re: [PATCH] spi: sun4i: allow transfers to set transmission speed

[Mark Brown]

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

On Sun, Nov 08, 2015 at 11:13:33AM +0100, Marcus Weseloh wrote:
> 2015-11-08 10:35 GMT+01:00 Mark Brown <broonie-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>:

> > This is not needed, the core will always ensure that the transfer speed
> > is set per on each transfer.

> Ah, so we could always use tfr->speed_hz and completely ignore the
> spi->max_speed_hz for the clock calculations, correct?

Yes.

> I did notice however, that the SPI system doesn't validate the
> speed_hz value. So if I leave out the above quoted sanity checking,
> then tfr->speed_hz could also contain values like -400, which trips up
> the spi system in such a way that I could only repair with a reboot. I
> was testing this with spidev... so maybe spidev should be patched to
> check the speed value for validity?

No, why would it make sense to have this validation in spidev - it's
just one SPI device.  It's better to add the validation to the SPI core.

[PATCH v2] spi: sun4i: allow transfers to set transmission speed

[Marcus Weseloh]

Allow transfers to set the transmission speed rather than using the
device max_speed_hz value. The SPI core makes sure that the speed_hz
value is always set on the transfer.

Signed-off-by: Marcus Weseloh <mweseloh42-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
---
Changes from v1:
  * Remove fallback to spi->max_speed_hz and sanity checks (as requested
    by Mark Brown)
  * Also patch identical code in sun6i-spi.c
---
 drivers/spi/spi-sun4i.c | 8 ++++----
 drivers/spi/spi-sun6i.c | 8 ++++----
 2 files changed, 8 insertions(+), 8 deletions(-)

diff --git a/drivers/spi/spi-sun4i.c b/drivers/spi/spi-sun4i.c
index fbb0a4d..f60a6d6 100644
--- a/drivers/spi/spi-sun4i.c
+++ b/drivers/spi/spi-sun4i.c
@@ -229,8 +229,8 @@ static int sun4i_spi_transfer_one(struct spi_master *master,
 
 	/* Ensure that we have a parent clock fast enough */
 	mclk_rate = clk_get_rate(sspi->mclk);
-	if (mclk_rate < (2 * spi->max_speed_hz)) {
-		clk_set_rate(sspi->mclk, 2 * spi->max_speed_hz);
+	if (mclk_rate < (2 * tfr->speed_hz)) {
+		clk_set_rate(sspi->mclk, 2 * tfr->speed_hz);
 		mclk_rate = clk_get_rate(sspi->mclk);
 	}
 
@@ -248,14 +248,14 @@ 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 * spi->max_speed_hz);
+	div = mclk_rate / (2 * tfr->speed_hz);
 	if (div <= (SUN4I_CLK_CTL_CDR2_MASK + 1)) {
 		if (div > 0)
 			div--;
 
 		reg = SUN4I_CLK_CTL_CDR2(div) | SUN4I_CLK_CTL_DRS;
 	} else {
-		div = ilog2(mclk_rate) - ilog2(spi->max_speed_hz);
+		div = ilog2(mclk_rate) - ilog2(tfr->speed_hz);
 		reg = SUN4I_CLK_CTL_CDR1(div);
 	}
 
diff --git a/drivers/spi/spi-sun6i.c b/drivers/spi/spi-sun6i.c
index ac48f59..42e2c4b 100644
--- a/drivers/spi/spi-sun6i.c
+++ b/drivers/spi/spi-sun6i.c
@@ -217,8 +217,8 @@ static int sun6i_spi_transfer_one(struct spi_master *master,
 
 	/* Ensure that we have a parent clock fast enough */
 	mclk_rate = clk_get_rate(sspi->mclk);
-	if (mclk_rate < (2 * spi->max_speed_hz)) {
-		clk_set_rate(sspi->mclk, 2 * spi->max_speed_hz);
+	if (mclk_rate < (2 * tfr->speed_hz)) {
+		clk_set_rate(sspi->mclk, 2 * tfr->speed_hz);
 		mclk_rate = clk_get_rate(sspi->mclk);
 	}
 
@@ -236,14 +236,14 @@ static int sun6i_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 * spi->max_speed_hz);
+	div = mclk_rate / (2 * tfr->speed_hz);
 	if (div <= (SUN6I_CLK_CTL_CDR2_MASK + 1)) {
 		if (div > 0)
 			div--;
 
 		reg = SUN6I_CLK_CTL_CDR2(div) | SUN6I_CLK_CTL_DRS;
 	} else {
-		div = ilog2(mclk_rate) - ilog2(spi->max_speed_hz);
+		div = ilog2(mclk_rate) - ilog2(tfr->speed_hz);
 		reg = SUN6I_CLK_CTL_CDR1(div);
 	}
 
-- 
1.9.1

Re: [PATCH] spi: sun4i: allow transfers to set transmission speed

[Marcus Weseloh]

Hi Mark, Maxime,

2015-11-07 23:41 GMT+01:00 Marcus Weseloh <mweseloh42-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>:
> Allow transfers to set the transmission speed, only fall back to
> board max_speed_hz if requested speed is not set or invalid.
>
> Signed-off-by: Marcus Weseloh <mweseloh42-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> ---
>  drivers/spi/spi-sun4i.c | 15 +++++++++++----
>  1 file changed, 11 insertions(+), 4 deletions(-)

Just a quick follow-up: is there anything else I need to do to push
this patch along? Or do I just need to be more patient?

Best regards,

  Marcus
--
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] spi: sun4i: allow transfers to set transmission speed

[Mark Brown]

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

On Tue, Nov 17, 2015 at 04:50:54PM +0100, Marcus Weseloh wrote:
> 2015-11-07 23:41 GMT+01:00 Marcus Weseloh <mweseloh42-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>:

> > Allow transfers to set the transmission speed, only fall back to
> > board max_speed_hz if requested speed is not set or invalid.

> Just a quick follow-up: is there anything else I need to do to push
> this patch along? Or do I just need to be more patient?

I don't have this patch, presumably there were review comments that I
was expecting to see some action on.

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

Re: [PATCH] spi: sun4i: allow transfers to set transmission speed

[Marcus Weseloh]

Hi Mark,

2015-11-17 17:57 GMT+01:00 Mark Brown <broonie-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>:
>> Just a quick follow-up: is there anything else I need to do to push
>> this patch along? Or do I just need to be more patient?
>
> I don't have this patch, presumably there were review comments that I
> was expecting to see some action on.

Sorry, the mail I was quoting was actually the first version of the
patch, which you did comment on.  I had sent a v2 patch addressing
your comments, sent on the 8th of November, 12:08 CET:
http://marc.info/?l=linux-spi&m=144698062222607&w=2

Looks like I make a mistake when git send-email asked me for the
message id that this second version refers to. The linux-spi mailing
list didn't pick up on the reference and created a new thread for
it...

Best regards,

    Marcus
--
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] spi: sun4i: allow transfers to set transmission speed

[Mark Brown]

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

On Tue, Nov 17, 2015 at 07:14:14PM +0100, Marcus Weseloh wrote:

> Sorry, the mail I was quoting was actually the first version of the
> patch, which you did comment on.  I had sent a v2 patch addressing
> your comments, sent on the 8th of November, 12:08 CET:
> http://marc.info/?l=linux-spi&m=144698062222607&w=2

I have a form letter I usually send for this:

Please don't send content free pings and please allow a reasonable time
for review.  People get busy, go on holiday, attend conferences and so 
on so unless there is some reason for urgency (like critical bug fixes)
please allow at least a couple of weeks for review.  Sending content
free pings just adds to the mail volume (if they are seen at all) and if 
something has gone wrong you'll have to resend the patches anyway.

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

Re: [PATCH] spi: sun4i: allow transfers to set transmission speed

[Marcus Weseloh]

2015-11-17 19:34 GMT+01:00 Mark Brown <broonie-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>:
> Please don't send content free pings and please allow a reasonable time
> for review.  People get busy, go on holiday, attend conferences and so
> on so unless there is some reason for urgency (like critical bug fixes)
> please allow at least a couple of weeks for review.  Sending content
> free pings just adds to the mail volume (if they are seen at all) and if
> something has gone wrong you'll have to resend the patches anyway.

OK, message received and understood. Please accept my apologies! This
is my first code-changing contribution to mainline and I really was
unsure if the second version of the patch came through alright. I
guess I was spoiled by your very quick responses and review of the
first version, so I got worried when I didn't hear anything for a few
days. I'll be more patient in the future.

Best regards,
Marcus

Re: [PATCH] spi: sun4i: allow transfers to set transmission speed

[Mark Brown]

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

On Tue, Nov 17, 2015 at 11:06:54PM +0100, Marcus Weseloh wrote:

> OK, message received and understood. Please accept my apologies! This
> is my first code-changing contribution to mainline and I really was
> unsure if the second version of the patch came through alright. I
> guess I was spoiled by your very quick responses and review of the
> first version, so I got worried when I didn't hear anything for a few
> days. I'll be more patient in the future.

I'd probably have been quicker but the merge window was open until
Sunday so I've not been applying anything.

Applied "spi: sun4i: allow transfers to set transmission speed" to the spi tree

[Mark Brown]

The patch

   spi: sun4i: allow transfers to set transmission speed

has been applied to the spi tree at

   git://git.kernel.org/pub/scm/linux/kernel/git/broonie/spi.git 

All being well this means that it will be integrated into the linux-next
tree (usually sometime in the next 24 hours) and sent to Linus during
the next merge window (or sooner if it is a bug fix), however if
problems are discovered then the patch may be dropped or reverted.  

You may get further e-mails resulting from automated or manual testing
and review of the tree, please engage with people reporting problems and
send followup patches addressing any issues that are reported if needed.

If any updates are required or you are submitting further changes they
should be sent as incremental updates against current git, existing
patches will not be replaced.

Please add any relevant lists and maintainers to the CCs when replying
to this mail.

Thanks,
Mark

>From 47284e3e0f3c427c93f8583549b6c938e8a18015 Mon Sep 17 00:00:00 2001
From: Marcus Weseloh <mweseloh42-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
Date: Sun, 8 Nov 2015 12:03:23 +0100
Subject: [PATCH] spi: sun4i: allow transfers to set transmission speed

Allow transfers to set the transmission speed rather than using the
device max_speed_hz value. The SPI core makes sure that the speed_hz
value is always set on the transfer.

Signed-off-by: Marcus Weseloh <mweseloh42-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
Signed-off-by: Mark Brown <broonie-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
---
 drivers/spi/spi-sun4i.c | 8 ++++----
 drivers/spi/spi-sun6i.c | 8 ++++----
 2 files changed, 8 insertions(+), 8 deletions(-)

diff --git a/drivers/spi/spi-sun4i.c b/drivers/spi/spi-sun4i.c
index fbb0a4d74e91..f60a6d634d61 100644
--- a/drivers/spi/spi-sun4i.c
+++ b/drivers/spi/spi-sun4i.c
@@ -229,8 +229,8 @@ static int sun4i_spi_transfer_one(struct spi_master *master,
 
 	/* Ensure that we have a parent clock fast enough */
 	mclk_rate = clk_get_rate(sspi->mclk);
-	if (mclk_rate < (2 * spi->max_speed_hz)) {
-		clk_set_rate(sspi->mclk, 2 * spi->max_speed_hz);
+	if (mclk_rate < (2 * tfr->speed_hz)) {
+		clk_set_rate(sspi->mclk, 2 * tfr->speed_hz);
 		mclk_rate = clk_get_rate(sspi->mclk);
 	}
 
@@ -248,14 +248,14 @@ 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 * spi->max_speed_hz);
+	div = mclk_rate / (2 * tfr->speed_hz);
 	if (div <= (SUN4I_CLK_CTL_CDR2_MASK + 1)) {
 		if (div > 0)
 			div--;
 
 		reg = SUN4I_CLK_CTL_CDR2(div) | SUN4I_CLK_CTL_DRS;
 	} else {
-		div = ilog2(mclk_rate) - ilog2(spi->max_speed_hz);
+		div = ilog2(mclk_rate) - ilog2(tfr->speed_hz);
 		reg = SUN4I_CLK_CTL_CDR1(div);
 	}
 
diff --git a/drivers/spi/spi-sun6i.c b/drivers/spi/spi-sun6i.c
index ac48f59705a8..42e2c4bd690a 100644
--- a/drivers/spi/spi-sun6i.c
+++ b/drivers/spi/spi-sun6i.c
@@ -217,8 +217,8 @@ static int sun6i_spi_transfer_one(struct spi_master *master,
 
 	/* Ensure that we have a parent clock fast enough */
 	mclk_rate = clk_get_rate(sspi->mclk);
-	if (mclk_rate < (2 * spi->max_speed_hz)) {
-		clk_set_rate(sspi->mclk, 2 * spi->max_speed_hz);
+	if (mclk_rate < (2 * tfr->speed_hz)) {
+		clk_set_rate(sspi->mclk, 2 * tfr->speed_hz);
 		mclk_rate = clk_get_rate(sspi->mclk);
 	}
 
@@ -236,14 +236,14 @@ static int sun6i_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 * spi->max_speed_hz);
+	div = mclk_rate / (2 * tfr->speed_hz);
 	if (div <= (SUN6I_CLK_CTL_CDR2_MASK + 1)) {
 		if (div > 0)
 			div--;
 
 		reg = SUN6I_CLK_CTL_CDR2(div) | SUN6I_CLK_CTL_DRS;
 	} else {
-		div = ilog2(mclk_rate) - ilog2(spi->max_speed_hz);
+		div = ilog2(mclk_rate) - ilog2(tfr->speed_hz);
 		reg = SUN6I_CLK_CTL_CDR1(div);
 	}
 
-- 
2.6.2

--
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