[PATCH] SPI: BCM2835: fix all checkpath --strict messages

[PATCH] SPI: BCM2835: fix all checkpath --strict messages

[kernel-TqfNSX0MhmxHKSADF0wUEw]

From: Martin Sperl <kernel-TqfNSX0MhmxHKSADF0wUEw@public.gmane.org>

Signed-off-by: Martin Sperl <kernel-TqfNSX0MhmxHKSADF0wUEw@public.gmane.org>
---
 drivers/spi/spi-bcm2835.c |   28 +++++++++++++++++-----------
 1 file changed, 17 insertions(+), 11 deletions(-)

diff --git a/drivers/spi/spi-bcm2835.c b/drivers/spi/spi-bcm2835.c
index 419a782..e1dea40 100644
--- a/drivers/spi/spi-bcm2835.c
+++ b/drivers/spi/spi-bcm2835.c
@@ -178,8 +178,9 @@ static irqreturn_t bcm2835_spi_interrupt(int irq, void *dev_id)
 	return IRQ_NONE;
 }
 
-static int bcm2835_spi_start_transfer(struct spi_device *spi,
-		struct spi_transfer *tfr)
+static int bcm2835_spi_start_transfer(
+	struct spi_device *spi,
+	struct spi_transfer *tfr)
 {
 	struct bcm2835_spi *bs = spi_master_get_devdata(spi->master);
 	unsigned long spi_hz, clk_hz, cdiv;
@@ -196,8 +197,9 @@ static int bcm2835_spi_start_transfer(struct spi_device *spi,
 
 		if (cdiv >= 65536)
 			cdiv = 0; /* 0 is the slowest we can go */
-	} else
+	} else {
 		cdiv = 0; /* 0 is the slowest we can go */
+	}
 
 	if (spi->mode & SPI_CPOL)
 		cs |= BCM2835_SPI_CS_CPOL;
@@ -230,8 +232,9 @@ static int bcm2835_spi_start_transfer(struct spi_device *spi,
 	return 0;
 }
 
-static int bcm2835_spi_finish_transfer(struct spi_device *spi,
-		struct spi_transfer *tfr, bool cs_change)
+static int bcm2835_spi_finish_transfer(
+	struct spi_device *spi,
+	struct spi_transfer *tfr, bool cs_change)
 {
 	struct bcm2835_spi *bs = spi_master_get_devdata(spi->master);
 	u32 cs = bcm2835_rd(bs, BCM2835_SPI_CS);
@@ -252,8 +255,9 @@ static int bcm2835_spi_finish_transfer(struct spi_device *spi,
 	return 0;
 }
 
-static int bcm2835_spi_transfer_one(struct spi_master *master,
-		struct spi_message *mesg)
+static int bcm2835_spi_transfer_one(
+	struct spi_master *master,
+	struct spi_message *mesg)
 {
 	struct bcm2835_spi *bs = spi_master_get_devdata(master);
 	struct spi_transfer *tfr;
@@ -267,8 +271,9 @@ static int bcm2835_spi_transfer_one(struct spi_master *master,
 		if (err)
 			goto out;
 
-		timeout = wait_for_completion_timeout(&bs->done,
-				msecs_to_jiffies(BCM2835_SPI_TIMEOUT_MS));
+		timeout = wait_for_completion_timeout(
+			&bs->done,
+			msecs_to_jiffies(BCM2835_SPI_TIMEOUT_MS));
 		if (!timeout) {
 			err = -ETIMEDOUT;
 			goto out;
@@ -342,8 +347,9 @@ static int bcm2835_spi_probe(struct platform_device *pdev)
 
 	clk_prepare_enable(bs->clk);
 
-	err = devm_request_irq(&pdev->dev, bs->irq, bcm2835_spi_interrupt, 0,
-				dev_name(&pdev->dev), master);
+	err = devm_request_irq(
+		&pdev->dev, bs->irq, bcm2835_spi_interrupt, 0,
+		dev_name(&pdev->dev), master);
 	if (err) {
 		dev_err(&pdev->dev, "could not request IRQ: %d\n", err);
 		goto out_clk_disable;
-- 
1.7.10.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 V2] SPI: BCM2835: allow arbitrary GPIO to act as SPI-chip_select

[kernel-TqfNSX0MhmxHKSADF0wUEw]

From: Martin Sperl <kernel-TqfNSX0MhmxHKSADF0wUEw@public.gmane.org>

use cs-gpios for the spi bus master and standard gpio operation
instead of relying on the HW with just 2 chip_selects.

Signed-off-by: Martin Sperl <kernel-TqfNSX0MhmxHKSADF0wUEw@public.gmane.org>
---
 drivers/spi/spi-bcm2835.c |   65 ++++++++++++++++++++++++++++++++++++++++-----
 1 file changed, 58 insertions(+), 7 deletions(-)

diff --git a/drivers/spi/spi-bcm2835.c b/drivers/spi/spi-bcm2835.c
index e1dea40..5fbad64 100644
--- a/drivers/spi/spi-bcm2835.c
+++ b/drivers/spi/spi-bcm2835.c
@@ -30,6 +30,7 @@
 #include <linux/of.h>
 #include <linux/of_irq.h>
 #include <linux/of_device.h>
+#include <linux/of_gpio.h>
 #include <linux/spi/spi.h>

 /* SPI register offsets */
@@ -116,6 +117,16 @@ static inline void bcm2835_wr_fifo(struct bcm2835_spi *bs, int len)
 	}
 }

+/* ideally spi_set_cs would be exported by spi-core */
+static inline void bcm2835_set_cs(struct spi_device *spi, bool enable)
+{
+	if (spi->mode & SPI_CS_HIGH)
+		enable = !enable;
+
+	if (gpio_is_valid(spi->cs_gpio))
+		gpio_set_value(spi->cs_gpio, !enable);
+}
+
 static irqreturn_t bcm2835_spi_interrupt(int irq, void *dev_id)
 {
 	struct spi_master *master = dev_id;
@@ -207,12 +218,24 @@ static int bcm2835_spi_start_transfer(
 		cs |= BCM2835_SPI_CS_CPHA;

 	if (!(spi->mode & SPI_NO_CS)) {
-		if (spi->mode & SPI_CS_HIGH) {
-			cs |= BCM2835_SPI_CS_CSPOL;
-			cs |= BCM2835_SPI_CS_CSPOL0 << spi->chip_select;
-		}
+		if (gpio_is_valid(spi->cs_gpio)) {
+			bcm2835_set_cs(spi, 1);
+		} else {
+			/* Legacy mode using HW chip selects
+			 * note that there is a bug in this when there are
+			 * multiple devices on the bus with at least one
+			 * having SPI_CS_HIGH set - the other CS_CSPOLX get
+			 * reset to 0 when any other device starts a transfer
+			 * this is due to a shared register for the polarity
+			 */
+			if (spi->mode & SPI_CS_HIGH) {
+				cs |= BCM2835_SPI_CS_CSPOL;
+				cs |= BCM2835_SPI_CS_CSPOL0
+					<< spi->chip_select;
+			}

-		cs |= spi->chip_select;
+			cs |= spi->chip_select;
+		}
 	}

 	reinit_completion(&bs->done);
@@ -248,9 +271,12 @@ static int bcm2835_spi_finish_transfer(
 	if (tfr->delay_usecs)
 		udelay(tfr->delay_usecs);

-	if (cs_change)
+	if (cs_change) {
+		/* reset CS */
+		bcm2835_set_cs(spi, 0);
 		/* Clear TA flag */
 		bcm2835_wr(bs, BCM2835_SPI_CS, cs & ~BCM2835_SPI_CS_TA);
+	}

 	return 0;
 }
@@ -290,15 +316,39 @@ static int bcm2835_spi_transfer_one(
 	}

 out:
-	/* Clear FIFOs, and disable the HW block */
+	/* Clear FIFOs, reset CS and disable the HW block */
 	bcm2835_wr(bs, BCM2835_SPI_CS,
 		   BCM2835_SPI_CS_CLEAR_RX | BCM2835_SPI_CS_CLEAR_TX);
+	bcm2835_set_cs(spi, 0);
 	mesg->status = err;
 	spi_finalize_current_message(master);

 	return 0;
 }

+static int bcm2835_spi_setup(struct spi_device *spi)
+{
+	/* do not care when in SPI_NO_CS mode */
+	if (spi->mode & SPI_NO_CS)
+		return 0;
+
+	/* setting up CS right from the start */
+	if (gpio_is_valid(spi->cs_gpio)) {
+		bcm2835_set_cs(spi, 0);
+		return 0;
+	}
+
+	/* Legacy mode using HW chipselects */
+	if (spi->chip_select > 2) {
+		dev_err(&spi->dev,
+			"setup: only three native chip-selects are supported\n"
+			);
+		return -EINVAL;
+	}
+
+	return 0;
+}
+
 static int bcm2835_spi_probe(struct platform_device *pdev)
 {
 	struct spi_master *master;
@@ -318,6 +368,7 @@ static int bcm2835_spi_probe(struct platform_device *pdev)
 	master->bits_per_word_mask = SPI_BPW_MASK(8);
 	master->num_chipselect = 3;
 	master->transfer_one_message = bcm2835_spi_transfer_one;
+	master->setup = bcm2835_spi_setup;
 	master->dev.of_node = pdev->dev.of_node;

 	bs = spi_master_get_devdata(master);
--
1.7.10.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 V2] SPI: BCM2835: allow arbitrary GPIO to act as SPI-chip_select

[Stephen Warren]

On 03/19/2015 03:01 AM, kernel-TqfNSX0MhmxHKSADF0wUEw@public.gmane.org wrote:
> From: Martin Sperl <kernel-TqfNSX0MhmxHKSADF0wUEw@public.gmane.org>
> 
> use cs-gpios for the spi bus master and standard gpio operation
> instead of relying on the HW with just 2 chip_selects.

A changelog between v1 and v2 would be useful. I think this looks OK
though, so,
Acked-by: Stephen Warren <swarren-3lzwWm7+Weoh9ZMKESR00Q@public.gmane.org>
--
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 V2] SPI: BCM2835: allow arbitrary GPIO to act as SPI-chip_select

[Mark Brown]

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

On Thu, Mar 19, 2015 at 11:05:13PM -0600, Stephen Warren wrote:
> On 03/19/2015 03:01 AM, kernel-TqfNSX0MhmxHKSADF0wUEw@public.gmane.org wrote:
> > From: Martin Sperl <kernel-TqfNSX0MhmxHKSADF0wUEw@public.gmane.org>
> > 
> > use cs-gpios for the spi bus master and standard gpio operation
> > instead of relying on the HW with just 2 chip_selects.
> 
> A changelog between v1 and v2 would be useful. I think this looks OK
> though, so,
> Acked-by: Stephen Warren <swarren-3lzwWm7+Weoh9ZMKESR00Q@public.gmane.org>

Note that none of these patches appear to have been sent to me, Stephen
has CCed me in as far as I can tell...

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

Re: [PATCH V2 - RESEND] SPI: BCM2835: allow arbitrary GPIO to act as SPI-chip_select

[Martin Sperl]

use cs-gpios for the spi bus master and standard gpio operation
instead of relying on the HW with just 2 chip_selects.

Tested with 2x mcp251x (can bus), fb_st7735 (TFT framebuffer device) 
and enc28j60 (ethernet) drivers.

Signed-off-by: Martin Sperl <kernel-TqfNSX0MhmxHKSADF0wUEw@public.gmane.org>

---
 drivers/spi/spi-bcm2835.c |   65 ++++++++++++++++++++++++++++++++++++++++-----
 1 file changed, 58 insertions(+), 7 deletions(-)


> On 20.03.2015, at 10:52, Mark Brown <broonie-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org> wrote:
> 
> On Thu, Mar 19, 2015 at 11:05:13PM -0600, Stephen Warren wrote:
>> 
>> 
>> A changelog between v1 and v2 would be useful. I think this looks OK
>> though, so,
>> Acked-by: Stephen Warren <swarren-3lzwWm7+Weoh9ZMKESR00Q@public.gmane.org>
> 

Changelog:
  V1->V2: use gpio_is_valid
          added error for null GPIO in DT for any CS except 0 or 1.
          no patch for bcm2835 device tree to use new system

> Note that none of these patches appear to have been sent to me, Stephen
> has CCed me in as far as I can tell...
Resent patch below.


diff --git a/drivers/spi/spi-bcm2835.c b/drivers/spi/spi-bcm2835.c
index e1dea40..5fbad64 100644
--- a/drivers/spi/spi-bcm2835.c
+++ b/drivers/spi/spi-bcm2835.c
@@ -30,6 +30,7 @@
 #include <linux/of.h>
 #include <linux/of_irq.h>
 #include <linux/of_device.h>
+#include <linux/of_gpio.h>
 #include <linux/spi/spi.h>

 /* SPI register offsets */
@@ -116,6 +117,16 @@ static inline void bcm2835_wr_fifo(struct bcm2835_spi *bs, int len)
 	}
 }

+/* ideally spi_set_cs would be exported by spi-core */
+static inline void bcm2835_set_cs(struct spi_device *spi, bool enable)
+{
+	if (spi->mode & SPI_CS_HIGH)
+		enable = !enable;
+
+	if (gpio_is_valid(spi->cs_gpio))
+		gpio_set_value(spi->cs_gpio, !enable);
+}
+
 static irqreturn_t bcm2835_spi_interrupt(int irq, void *dev_id)
 {
 	struct spi_master *master = dev_id;
@@ -207,12 +218,24 @@ static int bcm2835_spi_start_transfer(
 		cs |= BCM2835_SPI_CS_CPHA;

 	if (!(spi->mode & SPI_NO_CS)) {
-		if (spi->mode & SPI_CS_HIGH) {
-			cs |= BCM2835_SPI_CS_CSPOL;
-			cs |= BCM2835_SPI_CS_CSPOL0 << spi->chip_select;
-		}
+		if (gpio_is_valid(spi->cs_gpio)) {
+			bcm2835_set_cs(spi, 1);
+		} else {
+			/* Legacy mode using HW chip selects
+			 * note that there is a bug in this when there are
+			 * multiple devices on the bus with at least one
+			 * having SPI_CS_HIGH set - the other CS_CSPOLX get
+			 * reset to 0 when any other device starts a transfer
+			 * this is due to a shared register for the polarity
+			 */
+			if (spi->mode & SPI_CS_HIGH) {
+				cs |= BCM2835_SPI_CS_CSPOL;
+				cs |= BCM2835_SPI_CS_CSPOL0
+					<< spi->chip_select;
+			}

-		cs |= spi->chip_select;
+			cs |= spi->chip_select;
+		}
 	}

 	reinit_completion(&bs->done);
@@ -248,9 +271,12 @@ static int bcm2835_spi_finish_transfer(
 	if (tfr->delay_usecs)
 		udelay(tfr->delay_usecs);

-	if (cs_change)
+	if (cs_change) {
+		/* reset CS */
+		bcm2835_set_cs(spi, 0);
 		/* Clear TA flag */
 		bcm2835_wr(bs, BCM2835_SPI_CS, cs & ~BCM2835_SPI_CS_TA);
+	}

 	return 0;
 }
@@ -290,15 +316,39 @@ static int bcm2835_spi_transfer_one(
 	}

 out:
-	/* Clear FIFOs, and disable the HW block */
+	/* Clear FIFOs, reset CS and disable the HW block */
 	bcm2835_wr(bs, BCM2835_SPI_CS,
 		   BCM2835_SPI_CS_CLEAR_RX | BCM2835_SPI_CS_CLEAR_TX);
+	bcm2835_set_cs(spi, 0);
 	mesg->status = err;
 	spi_finalize_current_message(master);

 	return 0;
 }

+static int bcm2835_spi_setup(struct spi_device *spi)
+{
+	/* do not care when in SPI_NO_CS mode */
+	if (spi->mode & SPI_NO_CS)
+		return 0;
+
+	/* setting up CS right from the start */
+	if (gpio_is_valid(spi->cs_gpio)) {
+		bcm2835_set_cs(spi, 0);
+		return 0;
+	}
+
+	/* Legacy mode using HW chipselects */
+	if (spi->chip_select > 2) {
+		dev_err(&spi->dev,
+			"setup: only three native chip-selects are supported\n"
+			);
+		return -EINVAL;
+	}
+
+	return 0;
+}
+
 static int bcm2835_spi_probe(struct platform_device *pdev)
 {
 	struct spi_master *master;
@@ -318,6 +368,7 @@ static int bcm2835_spi_probe(struct platform_device *pdev)
 	master->bits_per_word_mask = SPI_BPW_MASK(8);
 	master->num_chipselect = 3;
 	master->transfer_one_message = bcm2835_spi_transfer_one;
+	master->setup = bcm2835_spi_setup;
 	master->dev.of_node = pdev->dev.of_node;

 	bs = spi_master_get_devdata(master);
--
1.7.10.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 V2 - RESEND] SPI: BCM2835: allow arbitrary GPIO to act as SPI-chip_select

[Mark Brown]

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

On Wed, Mar 25, 2015 at 08:10:57AM +0100, Martin Sperl wrote:

> +static inline void bcm2835_set_cs(struct spi_device *spi, bool enable)
> +{
> +	if (spi->mode & SPI_CS_HIGH)
> +		enable = !enable;
> +
> +	if (gpio_is_valid(spi->cs_gpio))
> +		gpio_set_value(spi->cs_gpio, !enable);
> +}

This appears to be open coding the core spi_set_cs() support for GPIOs,
why are we not just setting cs_gpio and letting the core take care of
things?

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

Re: [PATCH V2 - RESEND] SPI: BCM2835: allow arbitrary GPIO to act as SPI-chip_select

[Martin Sperl]

> On 25.03.2015, at 16:16, Mark Brown <broonie-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org> wrote:
> 
> This appears to be open coding the core spi_set_cs() support for GPIOs,
> why are we not just setting cs_gpio and letting the core take care of
> things?
In principle yes - the problem is that the core method is not exported.
If you agree to export it then I will create a patch using that.
Maybe even exporting it in that patch).

The problem with the core framework as is is that it currently does
not support handling multiple spi_transfers in the irq-handler itself.
Instead we have to wake up the worker-thread to handle the next
spi_transfer which will - at some point - will trigger another irq,
where the overhead is again 5-6us until the spi-irq-handler runs.
This obviously adds unnecessary overhead.

Note that it takes about 5us after the IRQ-handler itself exits for
the irq framework to release the CPU and then some more overhead (2-3us)
to schedule the worker-thread. This is obviously an unnecessary waste of
CPU and adds unnecessary waits on the SPI bus - especially when running
spi_write_then_read style requests.

So I would like to test that portion out with something stable and as I am
testing against 4 devices right now, so I need some kernel that supports
4 devices...

If we come up with some improvements which we might be able to generalize,
then we can move these things into core and convert the whole driver to 
the new model.

Keeping it as is means we can easily backport(=copy) it to the kernel
that the RPI foundation maintains and that gets lots of exposure and
testing.

Martin

--
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 V2 - RESEND] SPI: BCM2835: allow arbitrary GPIO to act as SPI-chip_select

[Mark Brown]

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

On Wed, Mar 25, 2015 at 05:13:13PM +0100, Martin Sperl wrote:
> > On 25.03.2015, at 16:16, Mark Brown <broonie-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org> wrote:

> > This appears to be open coding the core spi_set_cs() support for GPIOs,
> > why are we not just setting cs_gpio and letting the core take care of
> > things?

> In principle yes - the problem is that the core method is not exported.
> If you agree to export it then I will create a patch using that.
> Maybe even exporting it in that patch).

There is no need for it to be exported, as I said you simply need to set
cs_gpio and it will be used.

> The problem with the core framework as is is that it currently does
> not support handling multiple spi_transfers in the irq-handler itself.
> Instead we have to wake up the worker-thread to handle the next
> spi_transfer which will - at some point - will trigger another irq,
> where the overhead is again 5-6us until the spi-irq-handler runs.
> This obviously adds unnecessary overhead.

We've been round this loop repeatedly, as we've discussed improving the
framework is a good idea.

> Keeping it as is means we can easily backport(=copy) it to the kernel
> that the RPI foundation maintains and that gets lots of exposure and
> testing.

The thing to do here is to get this code upstream, supporting out of
tree code isn't particularly persuasive here - people commonly try to
use their out of tree requirements as a reason to merge code that's
got problems upstream and I don't want people to get the idea that this
is an argument that's going to work.

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

Re: [PATCH V2 - RESEND] SPI: BCM2835: allow arbitrary GPIO to act as SPI-chip_select

[Martin Sperl]

> On 25.03.2015, at 17:54, Mark Brown <broonie-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org> wrote:
> There is no need for it to be exported, as I said you simply need to set
> cs_gpio and it will be used.

That is not true, because for that to work you need to move the driver
away from using transfer_one_message to use transfer_one.

But then, as explained I have to revert back to transfer_one_message
to implement the "aggregating/handling" of transfers inside the interrupt
handler itself without having to call complete(master->xfer_completion)
on every spi_transfer. If I wrap it arround transfer_one the code will
look ugly, as it needs to work arround limitations of this api.
> 
> We've been round this loop repeatedly, as we've discussed improving the
> framework is a good idea.

That is what I try to do provide something we can take as a template
and then come up with approaches how we can generalize it to other
use-cases.

>From my perspective I want to minimize the dependencies while I create
this proof of concept.
And for my testing this with several different devices I need support
for multiple devices and for that I need this patch go in first.
Taking baby steps here.

Also there is another point here, that Stephen pointed out:
Would not a switch from the use of the "native-cs" to "gpio-only" mean
a required change in Device-tree? And isn't DT assumed to be a stabe API?

>> Keeping it as is means we can easily backport(=copy) it to the kernel
>> that the RPI foundation maintains and that gets lots of exposure and
>> testing.
> The thing to do here is to get this code upstream, supporting out of
> tree code isn't particularly persuasive here - people commonly try to
> use their out of tree requirements as a reason to merge code that's
> got problems upstream and I don't want people to get the idea that this
> is an argument that's going to work.

That is what I am doing: I am developing on spi/for-next and if we get
something of interest we will merge into the 3.18 kernel that the 
foundation is using - not the other way around!

Note that in principle you can take: bcm2835_spi_start_transfer
and use that as transfer_one (it needs master as an extra argument)
also there is the bcm2835_spi_finish_transfer that we would need to
move to some other place...

So to summarize: do you want me to move to transfer_one instead and
then revert back for the optimizations?

Thanks,
	Martin


--
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 V2 - RESEND] SPI: BCM2835: allow arbitrary GPIO to act as SPI-chip_select

[Mark Brown]

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

On Wed, Mar 25, 2015 at 06:59:22PM +0100, Martin Sperl wrote:
> > On 25.03.2015, at 17:54, Mark Brown <broonie-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org> wrote:

> > There is no need for it to be exported, as I said you simply need to set
> > cs_gpio and it will be used.

> That is not true, because for that to work you need to move the driver
> away from using transfer_one_message to use transfer_one.

I'm confused.  You're writing a set_cs() operation but you can't use it?

> But then, as explained I have to revert back to transfer_one_message
> to implement the "aggregating/handling" of transfers inside the interrupt
> handler itself without having to call complete(master->xfer_completion)
> on every spi_transfer. If I wrap it arround transfer_one the code will
> look ugly, as it needs to work arround limitations of this api.

This sounds like you're trying to work aroud the core rather than work
with the core - enhance the core so that it can express what you need.

> From my perspective I want to minimize the dependencies while I create
> this proof of concept.
> And for my testing this with several different devices I need support
> for multiple devices and for that I need this patch go in first.
> Taking baby steps here.

I had been under the impression that you had already done a lot of proof
of concept work with this stuff?

> Also there is another point here, that Stephen pointed out:
> Would not a switch from the use of the "native-cs" to "gpio-only" mean
> a required change in Device-tree? And isn't DT assumed to be a stabe API?

Can you be more specific about what change you think might be required?
Currently the driver doesn't support GPIOs as chip selects at all so
anything enabling them is going to be new.

> So to summarize: do you want me to move to transfer_one instead and
> then revert back for the optimizations?

Drivers should in general be moving to use modern APIs.  If current APIs
need enhancing then look into doing that.

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

Re: [PATCH V2 - RESEND] SPI: BCM2835: allow arbitrary GPIO to act as SPI-chip_select

[Martin Sperl]

> On 25.03.2015, at 19:50, Mark Brown <broonie-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org> wrote:
> 
> This sounds like you're trying to work aroud the core rather than work
> with the core - enhance the core so that it can express what you need.
> 

Working in a single driver makes things more local without having
a "moving" target with lots of other patches. If done right most of it
then can get moved to the core - if needed...

>> From my perspective I want to minimize the dependencies while I create
>> this proof of concept.
>> And for my testing this with several different devices I need support
>> for multiple devices and for that I need this patch go in first.
>> Taking baby steps here.
> 
> I had been under the impression that you had already done a lot of proof
> of concept work with this stuff?

I had an Idea - that specific one has not been tested yet, but from the
measurements on interrupt and scheduling latencies these seem low hanging
fruit.

> 
>> Also there is another point here, that Stephen pointed out:
>> Would not a switch from the use of the "native-cs" to "gpio-only" mean
>> a required change in Device-tree? And isn't DT assumed to be a stabe API?
> 
> Can you be more specific about what change you think might be required?
> Currently the driver doesn't support GPIOs as chip selects at all so
> anything enabling them is going to be new.
Well, if we say we "need" gpio_cs to be set in DT of the master, then
that might be assumed an API change that might break some custom 
device-trees - I was mostly referring to that.

So how would that situation be with the requirement to use the GPIO_CS 
explicitly?

>> So to summarize: do you want me to move to transfer_one instead and
>> then revert back for the optimizations?
> 
> Drivers should in general be moving to use modern APIs.  If current APIs
> need enhancing then look into doing that.
OK, so I will try to take the "modernization" approach, but a quick look
shows some headaches, that the current driver is hiding behind scheduling
latencies...

Martin

--
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 V2 - RESEND] SPI: BCM2835: allow arbitrary GPIO to act as SPI-chip_select

[Mark Brown]

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

On Wed, Mar 25, 2015 at 08:43:36PM +0100, Martin Sperl wrote:
> > On 25.03.2015, at 19:50, Mark Brown <broonie-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org> wrote:

> > This sounds like you're trying to work aroud the core rather than work
> > with the core - enhance the core so that it can express what you need.

> Working in a single driver makes things more local without having
> a "moving" target with lots of other patches. If done right most of it
> then can get moved to the core - if needed...

That's fine for your out of tree stuff but not for mainline.

> >> Also there is another point here, that Stephen pointed out:
> >> Would not a switch from the use of the "native-cs" to "gpio-only" mean
> >> a required change in Device-tree? And isn't DT assumed to be a stabe API?

> > Can you be more specific about what change you think might be required?
> > Currently the driver doesn't support GPIOs as chip selects at all so
> > anything enabling them is going to be new.

> Well, if we say we "need" gpio_cs to be set in DT of the master, then
> that might be assumed an API change that might break some custom 
> device-trees - I was mostly referring to that.

> So how would that situation be with the requirement to use the GPIO_CS 
> explicitly?

I'm sorry but I still don't understand what you're saying here.
Assigning a value to a variable in the kernel has no obvious impact on
the DT, and anything that adds a DT binding to the driver that doesn't
exist already is obviously a change.

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

[PATCH] SPI: bcm2835: move to the transfer_one driver model

[Martin Sperl]

This also allows for GPIO-CS to get used removing the limitation of
2/3 SPI devises on the SPI bus.

Fixes: spi-cs-high with native CS with multiple devices on the spi-bus
resetting the chip selects to "normal" polarity after a finished
transfer.

No other functionality/improvements added.

Tested with the following 4 devices on the spi-bus:
* mcp2515 with native CS
* mcp2515 with gpio CS
* fb_st7735r with native CS
    (plus spi-cs-high via transistor inverting polarity)
* enc28j60 with gpio-CS
Tested-by: Martin Sperl <kernel-TqfNSX0MhmxHKSADF0wUEw@public.gmane.org>

Signed-off-by: Martin Sperl <kernel-TqfNSX0MhmxHKSADF0wUEw@public.gmane.org>

---

Note that there is quite a bit of complexity involved to make the native 
CS work correctly.
Also a few future optimizations in the pipeline will only work reliably 
with gpio CS. 

So the question is if we should depreciate native chip-selects for this
driver with one of those future improvements listed below.

Here a list of planned future improvements:
* initially fill the FIFO without triggering an interrupt to do that work
* use polling for "short" transfers (<20us)
* implement DMA for longer transfers (>48 bytes) if tx_dma/rx_dma are set.
* implement can_dma et.al. for dma_mapping of transfers in the framework
* multiple spi_transfers handled in interrupt alone without waking up the
  worker-thread (for some transfers) to reduce context switching
  overheads and the corresponding latencies.

As for testing: I have also tried to test with mmc_spi, but I have not
been able to make that driver work reliably in any recent kernel
versions.
Most of the time I see timeouts - and with lots of different SD-cards...

IIRC the last time I tested it successfully was with 3.12.

So if someone has experience how to make it work on the RPI with a 
recent kernel, please let me know, so that I can extend my setup to add
this device/driver also to my test-scenario to increase diversity of the
test.

Martin

 drivers/spi/spi-bcm2835.c |  212 ++++++++++++++++++++++++++-------------------
 1 file changed, 124 insertions(+), 88 deletions(-)

diff --git a/drivers/spi/spi-bcm2835.c b/drivers/spi/spi-bcm2835.c
index 3f93718..05be082 100644
--- a/drivers/spi/spi-bcm2835.c
+++ b/drivers/spi/spi-bcm2835.c
@@ -3,6 +3,7 @@
  *
  * Copyright (C) 2012 Chris Boot
  * Copyright (C) 2013 Stephen Warren
+ * Copyright (C) 2015 Martin Sperl
  *
  * This driver is inspired by:
  * spi-ath79.c, Copyright (C) 2009-2011 Gabor Juhos <juhosg-p3rKhJxN3npAfugRpC6u6w@public.gmane.org>
@@ -29,6 +30,7 @@
 #include <linux/module.h>
 #include <linux/of.h>
 #include <linux/of_irq.h>
+#include <linux/of_gpio.h>
 #include <linux/of_device.h>
 #include <linux/spi/spi.h>
 
@@ -76,10 +78,10 @@ struct bcm2835_spi {
 	void __iomem *regs;
 	struct clk *clk;
 	int irq;
-	struct completion done;
 	const u8 *tx_buf;
 	u8 *rx_buf;
-	int len;
+	int tx_len;
+	int rx_len;
 };
 
 static inline u32 bcm2835_rd(struct bcm2835_spi *bs, unsigned reg)
@@ -96,10 +98,12 @@ static inline void bcm2835_rd_fifo(struct bcm2835_spi *bs)
 {
 	u8 byte;
 
-	while (bcm2835_rd(bs, BCM2835_SPI_CS) & BCM2835_SPI_CS_RXD) {
+	while ((bs->rx_len) &&
+	       (bcm2835_rd(bs, BCM2835_SPI_CS) & BCM2835_SPI_CS_RXD)) {
 		byte = bcm2835_rd(bs, BCM2835_SPI_FIFO);
 		if (bs->rx_buf)
 			*bs->rx_buf++ = byte;
+		bs->rx_len--;
 	}
 }
 
@@ -107,47 +111,60 @@ static inline void bcm2835_wr_fifo(struct bcm2835_spi *bs)
 {
 	u8 byte;
 
-	while ((bs->len) &&
+	while ((bs->tx_len) &&
 	       (bcm2835_rd(bs, BCM2835_SPI_CS) & BCM2835_SPI_CS_TXD)) {
 		byte = bs->tx_buf ? *bs->tx_buf++ : 0;
 		bcm2835_wr(bs, BCM2835_SPI_FIFO, byte);
-		bs->len--;
+		bs->tx_len--;
 	}
 }
 
+static void bcm2835_spi_reset_hw(struct spi_master *master)
+{
+	struct bcm2835_spi *bs = spi_master_get_devdata(master);
+	u32 cs = bcm2835_rd(bs, BCM2835_SPI_CS);
+
+	/* Disable SPI interrupts and transfer */
+	cs &= ~(BCM2835_SPI_CS_INTR |
+		BCM2835_SPI_CS_INTD |
+		BCM2835_SPI_CS_TA);
+	/* and reset RX/TX FIFOS */
+	cs |= BCM2835_SPI_CS_CLEAR_RX | BCM2835_SPI_CS_CLEAR_TX;
+
+	/* and reset the SPI_HW */
+	bcm2835_wr(bs, BCM2835_SPI_CS, cs);
+}
+
 static irqreturn_t bcm2835_spi_interrupt(int irq, void *dev_id)
 {
 	struct spi_master *master = dev_id;
 	struct bcm2835_spi *bs = spi_master_get_devdata(master);
-	u32 cs = bcm2835_rd(bs, BCM2835_SPI_CS);
 
 	/* Read as many bytes as possible from FIFO */
 	bcm2835_rd_fifo(bs);
-
-	if (bs->len) { /* there is more data to transmit */
-		bcm2835_wr_fifo(bs);
-	} else { /* Transfer complete */
-		/* Disable SPI interrupts */
-		cs &= ~(BCM2835_SPI_CS_INTR | BCM2835_SPI_CS_INTD);
-		bcm2835_wr(bs, BCM2835_SPI_CS, cs);
-
-		/*
-		 * Wake up bcm2835_spi_transfer_one(), which will call
-		 * bcm2835_spi_finish_transfer(), to drain the RX FIFO.
-		 */
-		complete(&bs->done);
+	/* Write as many bytes as possible to FIFO */
+	bcm2835_wr_fifo(bs);
+
+	/* based on flags decide if we can finish the transfer */
+	if (bcm2835_rd(bs, BCM2835_SPI_CS) & BCM2835_SPI_CS_DONE) {
+		/* Transfer complete - reset SPI HW */
+		bcm2835_spi_reset_hw(master);
+		/* wake up the framework */
+		complete(&master->xfer_completion);
 	}
 
 	return IRQ_HANDLED;
 }
 
-static int bcm2835_spi_start_transfer(struct spi_device *spi,
-				      struct spi_transfer *tfr)
+static int bcm2835_spi_transfer_one(struct spi_master *master,
+				    struct spi_device *spi,
+				    struct spi_transfer *tfr)
 {
-	struct bcm2835_spi *bs = spi_master_get_devdata(spi->master);
+	struct bcm2835_spi *bs = spi_master_get_devdata(master);
 	unsigned long spi_hz, clk_hz, cdiv;
-	u32 cs = BCM2835_SPI_CS_INTR | BCM2835_SPI_CS_INTD | BCM2835_SPI_CS_TA;
+	u32 cs = bcm2835_rd(bs, BCM2835_SPI_CS);
 
+	/* set clock */
 	spi_hz = tfr->speed_hz;
 	clk_hz = clk_get_rate(bs->clk);
 
@@ -163,100 +180,118 @@ static int bcm2835_spi_start_transfer(struct spi_device *spi,
 	} else {
 		cdiv = 0; /* 0 is the slowest we can go */
 	}
+	bcm2835_wr(bs, BCM2835_SPI_CLK, cdiv);
 
+	/* handle all the modes */
 	if ((spi->mode & SPI_3WIRE) && (tfr->rx_buf))
 		cs |= BCM2835_SPI_CS_REN;
-
 	if (spi->mode & SPI_CPOL)
 		cs |= BCM2835_SPI_CS_CPOL;
 	if (spi->mode & SPI_CPHA)
 		cs |= BCM2835_SPI_CS_CPHA;
 
-	if (!(spi->mode & SPI_NO_CS)) {
-		if (spi->mode & SPI_CS_HIGH) {
-			cs |= BCM2835_SPI_CS_CSPOL;
-			cs |= BCM2835_SPI_CS_CSPOL0 << spi->chip_select;
-		}
-
-		cs |= spi->chip_select;
-	}
+	/* for gpio_cs set dummy CS so that no HW-CS get changed
+	 * we can not run this in bcm2835_spi_set_cs, as it does
+	 * not get called for cs_gpio cases, so we need to do it here
+	 */
+	if (gpio_is_valid(spi->cs_gpio) || (spi->mode & SPI_NO_CS))
+		cs |= BCM2835_SPI_CS_CS_10 | BCM2835_SPI_CS_CS_01;
 
-	reinit_completion(&bs->done);
+	/* set transmit buffers and length */
 	bs->tx_buf = tfr->tx_buf;
 	bs->rx_buf = tfr->rx_buf;
-	bs->len = tfr->len;
+	bs->tx_len = tfr->len;
+	bs->rx_len = tfr->len;
 
-	bcm2835_wr(bs, BCM2835_SPI_CLK, cdiv);
 	/*
 	 * Enable the HW block. This will immediately trigger a DONE (TX
 	 * empty) interrupt, upon which we will fill the TX FIFO with the
 	 * first TX bytes. Pre-filling the TX FIFO here to avoid the
 	 * interrupt doesn't work:-(
 	 */
+	cs |= BCM2835_SPI_CS_INTR | BCM2835_SPI_CS_INTD | BCM2835_SPI_CS_TA;
 	bcm2835_wr(bs, BCM2835_SPI_CS, cs);
 
-	return 0;
+	/* signal that we need to wait for completion */
+	return 1;
 }
 
-static int bcm2835_spi_finish_transfer(struct spi_device *spi,
-				       struct spi_transfer *tfr,
-				       bool cs_change)
+static void bcm2835_spi_handle_err(struct spi_master *master,
+				   struct spi_message *msg)
 {
-	struct bcm2835_spi *bs = spi_master_get_devdata(spi->master);
-	u32 cs = bcm2835_rd(bs, BCM2835_SPI_CS);
-
-	if (tfr->delay_usecs)
-		udelay(tfr->delay_usecs);
-
-	if (cs_change)
-		/* Clear TA flag */
-		bcm2835_wr(bs, BCM2835_SPI_CS, cs & ~BCM2835_SPI_CS_TA);
-
-	return 0;
+	bcm2835_spi_reset_hw(master);
 }
 
-static int bcm2835_spi_transfer_one(struct spi_master *master,
-				    struct spi_message *mesg)
+static void bcm2835_spi_set_cs(struct spi_device *spi, bool gpio_level)
 {
+	/*
+	 * we can assume that we are "native" as per spi_set_cs
+	 *   calling us ONLY when cs_gpio is not set
+	 * we can also assume that we are CS < 3 as per bcm2835_spi_setup
+	 *   we would not get called because of error handling there.
+	 * the level passed is the electrical level not enabled/disabled
+	 *   so it has to get translated back to enable/disable
+	 *   see spi_set_cs in spi.c for the implementation
+	 */
+
+	struct spi_master *master = spi->master;
 	struct bcm2835_spi *bs = spi_master_get_devdata(master);
-	struct spi_transfer *tfr;
-	struct spi_device *spi = mesg->spi;
-	int err = 0;
-	unsigned int timeout;
-	bool cs_change;
-
-	list_for_each_entry(tfr, &mesg->transfers, transfer_list) {
-		err = bcm2835_spi_start_transfer(spi, tfr);
-		if (err)
-			goto out;
-
-		timeout = wait_for_completion_timeout(
-			&bs->done,
-			msecs_to_jiffies(BCM2835_SPI_TIMEOUT_MS)
-			);
-		if (!timeout) {
-			err = -ETIMEDOUT;
-			goto out;
-		}
+	u32 cs = bcm2835_rd(bs, BCM2835_SPI_CS);
+	bool enable;
 
-		cs_change = tfr->cs_change ||
-			list_is_last(&tfr->transfer_list, &mesg->transfers);
+	/* calculate the enable flag from the passed gpio_level */
+	enable = (spi->mode & SPI_CS_HIGH) ? gpio_level : !gpio_level;
 
-		err = bcm2835_spi_finish_transfer(spi, tfr, cs_change);
-		if (err)
-			goto out;
+	/* set flags for "reverse" polarity in the registers */
+	if (spi->mode & SPI_CS_HIGH) {
+		/* set the correct CS-bits */
+		cs |= BCM2835_SPI_CS_CSPOL;
+		cs |= BCM2835_SPI_CS_CSPOL0 << spi->chip_select;
+	} else {
+		/* clean the CS-bits */
+		cs &= ~BCM2835_SPI_CS_CSPOL;
+		cs &= ~(BCM2835_SPI_CS_CSPOL0 << spi->chip_select);
+	}
 
-		mesg->actual_length += (tfr->len - bs->len);
+	/* select the correct chip_select depending on disabled/enabled */
+	if (enable) {
+		/* set cs correctly */
+		if (spi->mode & SPI_NO_CS) {
+			/* use the "undefined" chip-select */
+			cs |= BCM2835_SPI_CS_CS_10 | BCM2835_SPI_CS_CS_01;
+		} else {
+			/* set the chip select */
+			cs &= ~(BCM2835_SPI_CS_CS_10 | BCM2835_SPI_CS_CS_01);
+			cs |= spi->chip_select;
+		}
+	} else {
+		/* disable CSPOL which puts HW-CS into deselected state */
+		cs &= ~BCM2835_SPI_CS_CSPOL;
+		/* use the "undefined" chip-select as precaution */
+		cs |= BCM2835_SPI_CS_CS_10 | BCM2835_SPI_CS_CS_01;
 	}
 
-out:
-	/* Clear FIFOs, and disable the HW block */
-	bcm2835_wr(bs, BCM2835_SPI_CS,
-		   BCM2835_SPI_CS_CLEAR_RX | BCM2835_SPI_CS_CLEAR_TX);
-	mesg->status = err;
-	spi_finalize_current_message(master);
+	/* finally set the calculated flags in SPI_CS */
+	bcm2835_wr(bs, BCM2835_SPI_CS, cs);
+}
 
-	return 0;
+static int bcm2835_spi_setup(struct spi_device *spi)
+{
+	/*
+	 * sanity checking the native-chipselects
+	 */
+	if (spi->mode & SPI_NO_CS)
+		return 0;
+	if (gpio_is_valid(spi->cs_gpio))
+		return 0;
+	if (spi->chip_select < 3)
+		return 0;
+
+	/* error in the case of native CS requested with CS-id > 2 */
+	dev_err(&spi->dev,
+		"setup: only three native chip-selects are supported\n"
+		);
+	return -EINVAL;
 }
 
 static int bcm2835_spi_probe(struct platform_device *pdev)
@@ -277,13 +312,14 @@ static int bcm2835_spi_probe(struct platform_device *pdev)
 	master->mode_bits = BCM2835_SPI_MODE_BITS;
 	master->bits_per_word_mask = SPI_BPW_MASK(8);
 	master->num_chipselect = 3;
-	master->transfer_one_message = bcm2835_spi_transfer_one;
+	master->setup = bcm2835_spi_setup;
+	master->set_cs = bcm2835_spi_set_cs;
+	master->transfer_one = bcm2835_spi_transfer_one;
+	master->handle_err = bcm2835_spi_handle_err;
 	master->dev.of_node = pdev->dev.of_node;
 
 	bs = spi_master_get_devdata(master);
 
-	init_completion(&bs->done);
-
 	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
 	bs->regs = devm_ioremap_resource(&pdev->dev, res);
 	if (IS_ERR(bs->regs)) {
@@ -314,7 +350,7 @@ static int bcm2835_spi_probe(struct platform_device *pdev)
 		goto out_clk_disable;
 	}
 
-	/* initialise the hardware */
+	/* initialise the hardware with the default polarities */
 	bcm2835_wr(bs, BCM2835_SPI_CS,
 		   BCM2835_SPI_CS_CLEAR_RX | BCM2835_SPI_CS_CLEAR_TX);
 
-- 
1.7.10.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] SPI: bcm2835: move to the transfer_one driver model

[Mark Brown]

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

On Thu, Mar 26, 2015 at 11:08:36AM +0100, Martin Sperl wrote:
> This also allows for GPIO-CS to get used removing the limitation of
> 2/3 SPI devises on the SPI bus.

This doesn't apply against current code, please check and resend.  As
previously mentioned please use subject lines matching the style for the
subsystem.  A few other things below but it's basically all good.

> So the question is if we should depreciate native chip-selects for this
> driver with one of those future improvements listed below.

Given that this driver is only going to be used with a single SoC (or
perhaps a very limited set of SoCs, I don't know if it's the same driver
in the Pi 2) even if the DT specifies a hardware chip select we should
be able to look up which pin that's brought out to and put it into GPIO
mode if that's the most sensible thing for the driver.

> * multiple spi_transfers handled in interrupt alone without waking up the
>   worker-thread (for some transfers) to reduce context switching
>   overheads and the corresponding latencies.

This in particular is something the framework should have: it's
generally useful and we should be able to do it with either a new
transfer_irq_safe() (or something) operation or a refactoring of the
existing one.

> +	/* error in the case of native CS requested with CS-id > 2 */
> +	dev_err(&spi->dev,
> +		"setup: only three native chip-selects are supported\n"
> +		);

The indentation here is weird - at least the last ); should be with the
string.

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

Re: [PATCH] SPI: bcm2835: move to the transfer_one driver model

[Stephen Warren]

On 03/26/2015 11:35 AM, Mark Brown wrote:
> On Thu, Mar 26, 2015 at 11:08:36AM +0100, Martin Sperl wrote:
>> This also allows for GPIO-CS to get used removing the limitation of
>> 2/3 SPI devises on the SPI bus.
>
> This doesn't apply against current code, please check and resend.  As
> previously mentioned please use subject lines matching the style for the
> subsystem.  A few other things below but it's basically all good.
>
>> So the question is if we should depreciate native chip-selects for this
>> driver with one of those future improvements listed below.
>
> Given that this driver is only going to be used with a single SoC (or
> perhaps a very limited set of SoCs, I don't know if it's the same driver
> in the Pi 2) even if the DT specifies a hardware chip select we should
> be able to look up which pin that's brought out to and put it into GPIO
> mode if that's the most sensible thing for the driver.

The Pi and Pi2 are essentially identical with the exception of the CPU, 
so yes this driver should run there too (and the GPIO and pinmux are 
identical too AFAIK, so any interaction there ought to work the same).
--
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: bcm2835: move to the transfer_one driver model

[Martin Sperl]

> On 26.03.2015, at 18:35, Mark Brown <broonie-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org> wrote:
> 
> On Thu, Mar 26, 2015 at 11:08:36AM +0100, Martin Sperl wrote:
>> This also allows for GPIO-CS to get used removing the limitation of
>> 2/3 SPI devises on the SPI bus.
> 
> This doesn't apply against current code, please check and resend.  As
> previously mentioned please use subject lines matching the style for the
> subsystem.  A few other things below but it's basically all good.

I have applied it against "for-next" (8befd715f1c1684b3).

The reason I chose for-next is because there all the other patches
I have submitted so far have been added there already...

Against which one do you want to apply it instead?

You want me to merge all those patches into a single patch instead?

>> So the question is if we should depreciate native chip-selects for this
>> driver with one of those future improvements listed below.
> 
> Given that this driver is only going to be used with a single SoC (or
> perhaps a very limited set of SoCs, I don't know if it's the same driver
> in the Pi 2) even if the DT specifies a hardware chip select we should
> be able to look up which pin that's brought out to and put it into GPIO
> mode if that's the most sensible thing for the driver.
The point here is that we need to change the alternate function for that
pin to output make it work.

That is why I was recommending the "simple" approach to "depreciate" 
that function if acceptable - less code to maintain...

Any yes: RPI2 uses the same SPI block and the same driver - the main 
difference is the arms

> 
>> * multiple spi_transfers handled in interrupt alone without waking up the
>>  worker-thread (for some transfers) to reduce context switching
>>  overheads and the corresponding latencies.
> 
> This in particular is something the framework should have: it's
> generally useful and we should be able to do it with either a new
> transfer_irq_safe() (or something) operation or a refactoring of the
> existing one.

Let me see what I can do with the current setup and then we can generalize
from that.

For most parts I see that I would use transfer_one and return 0 immediately
if we can "chain" it in interrupts - only the last transfer would trigger
a complete in the interrupt handler and return. We could even run the 
callback in the irq, if it is permitted...

But as I see there would be a few cases that would also need a complete.
These would be "delays" and maybe a CS-change - there mostly because of
the hard-coded "udelay(10)", but then it might be acceptable to sleep
10us in the irq, because the overhead of the irq handler releasing the CPU
is arround 5us and then you got another 3us inside the scheduler before
the worker-thread wakes up. If you take all that then you may as well sleep
in the interrupt handler - those at least are my measurements for
a RPI.

> 
>> +	/* error in the case of native CS requested with CS-id > 2 */
>> +	dev_err(&spi->dev,
>> +		"setup: only three native chip-selects are supported\n"
>> +		);
> 
> The indentation here is weird - at least the last ); should be with the
> string.

Will fix that when you tell me which branch you want it to get patch to 
apply against.

Martin

--
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: bcm2835: move to the transfer_one driver model

[Mark Brown]

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

On Thu, Mar 26, 2015 at 08:15:26PM +0100, Martin Sperl wrote:
> > On 26.03.2015, at 18:35, Mark Brown <broonie-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org> wrote:

> > On Thu, Mar 26, 2015 at 11:08:36AM +0100, Martin Sperl wrote:
> >> This also allows for GPIO-CS to get used removing the limitation of
> >> 2/3 SPI devises on the SPI bus.

> > This doesn't apply against current code, please check and resend.  As
> > previously mentioned please use subject lines matching the style for the
> > subsystem.  A few other things below but it's basically all good.

> I have applied it against "for-next" (8befd715f1c1684b3).

> The reason I chose for-next is because there all the other patches
> I have submitted so far have been added there already...

> Against which one do you want to apply it instead?

I would expect to apply patches against the relevant topic branch if one
exists, if they don't apply there then mention the dependencies.  I've
managed to figure that out given the above and applied the patch, though
in addition to the issue with the subject line that I mentioned earlier
please (as I think also mentioned several times now) do not send patches
in reply to existing threads.

> You want me to merge all those patches into a single patch instead?

Please never do this, neither resend already applied patches nor combine
unrelated patches into a single patch.  Resending already applied
patches wastes people's time and combining unrelated patches makes
review harder and goes against the good practice covered in
SubmittingPatches.

> >> So the question is if we should depreciate native chip-selects for this
> >> driver with one of those future improvements listed below.

> > Given that this driver is only going to be used with a single SoC (or
> > perhaps a very limited set of SoCs, I don't know if it's the same driver
> > in the Pi 2) even if the DT specifies a hardware chip select we should
> > be able to look up which pin that's brought out to and put it into GPIO
> > mode if that's the most sensible thing for the driver.

> The point here is that we need to change the alternate function for that
> pin to output make it work.

Yes, I understand that.  To repeat we should (given that this only needs
to cope with a very limited range of systems) be able to figure this out
without DT.

> > This in particular is something the framework should have: it's
> > generally useful and we should be able to do it with either a new
> > transfer_irq_safe() (or something) operation or a refactoring of the
> > existing one.

> Let me see what I can do with the current setup and then we can generalize
> from that.

Pushing the next transfer immediately from the completion seems pretty
general?

> For most parts I see that I would use transfer_one and return 0 immediately
> if we can "chain" it in interrupts - only the last transfer would trigger
> a complete in the interrupt handler and return. We could even run the 
> callback in the irq, if it is permitted...

You can't rely on an existing transfer_one() being interrupt safe.

> But as I see there would be a few cases that would also need a complete.
> These would be "delays" and maybe a CS-change - there mostly because of
> the hard-coded "udelay(10)", but then it might be acceptable to sleep
> 10us in the irq, because the overhead of the irq handler releasing the CPU
> is arround 5us and then you got another 3us inside the scheduler before
> the worker-thread wakes up. If you take all that then you may as well sleep
> in the interrupt handler - those at least are my measurements for
> a RPI.

Yes, delays and /CS bouncing need to be punted to threads.

> >> +	/* error in the case of native CS requested with CS-id > 2 */
> >> +	dev_err(&spi->dev,
> >> +		"setup: only three native chip-selects are supported\n"
> >> +		);

> > The indentation here is weird - at least the last ); should be with the
> > string.

> Will fix that when you tell me which branch you want it to get patch to 
> apply against.

Please send an incremental patch.

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

Re: [PATCH] SPI: bcm2835: move to the transfer_one driver model

[Stephen Warren]

On 03/26/2015 04:08 AM, Martin Sperl wrote:
...
> ---
> 
> Note that there is quite a bit of complexity involved to make the native 
> CS work correctly.
> Also a few future optimizations in the pipeline will only work reliably 
> with gpio CS.

Can you expand on that a bit more?

Are you planning on implementing code in the driver so it always uses
GPIO CS even when GPIOs aren't specified in the DT, or disabling those
optimizations when native CS is in use?

> So the question is if we should depreciate native chip-selects for this
> driver with one of those future improvements listed below.

Only if you can make the driver transparently use GPIO CS mode even when
no GPIOs are specified in the DT. DT is an ABI, and old DTs need to
continue to work on newer kernels.

I haven't had a chance to look at the code in this patch yet.

> As for testing: I have also tried to test with mmc_spi, but I have not
> been able to make that driver work reliably in any recent kernel
> versions.
> Most of the time I see timeouts - and with lots of different SD-cards...
> 
> IIRC the last time I tested it successfully was with 3.12.

It'd be great if you could use "git bisect" to track down the change
that broke this.
--
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: bcm2835: move to the transfer_one driver model

[Martin Sperl]

> On 28.03.2015, at 05:09, Stephen Warren <swarren-3lzwWm7+Weoh9ZMKESR00Q@public.gmane.org> wrote:
> Can you expand on that a bit more?
> 
> Are you planning on implementing code in the driver so it always uses
> GPIO CS even when GPIOs aren't specified in the DT, or disabling those
> optimizations when native CS is in use?

>> So the question is if we should depreciate native chip-selects for this
>> driver with one of those future improvements listed below.
> 
> Only if you can make the driver transparently use GPIO CS mode even when
> no GPIOs are specified in the DT. DT is an ABI, and old DTs need to
> continue to work on newer kernels.
No I think I will just have a case where some optimization only get used
when using gpio - even though it adds a bit of complexity/code to maintain.

The code to alter the pin-mode from ALT1 to OUT is probably more complex
than just adding a "dev_warn" during probe to indicate that it is 
depreciated/no longer recommended and an if statement to discriminate the
situation.

If you know how to change the Mode of a pin on the fly, then we can add
that at a some point.

> 
>> As for testing: I have also tried to test with mmc_spi, but I have not
>> been able to make that driver work reliably in any recent kernel
>> versions.
>> Most of the time I see timeouts - and with lots of different SD-cards...
>> 
>> IIRC the last time I tested it successfully was with 3.12.
> 
> It'd be great if you could use "git bisect" to track down the change
> that broke this.

The problem is that it is a lot of kernel-versions I would have to test
to figure out why and with the rpi used for compiling the kernel it 
becomes a prohibitive long exercise...

Also I may have a slightly different setup now compared to when I did
the test initially, so this may be the trigger as well. 
Hence I was asking if someone had similar issues/has a working setup 
with an up to date kernel. (Also I think the last time I tried it was
still based on the foundation kernels before an upstream kernel existed)

I will give it a try running an old kernel, but it may take some time...



--
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: bcm2835: move to the transfer_one driver model

[Stephen Warren]

On 03/28/2015 12:11 PM, Martin Sperl wrote:
>> On 28.03.2015, at 05:09, Stephen Warren <swarren-3lzwWm7+Weoh9ZMKESR00Q@public.gmane.org> wrote:
...
>>> As for testing: I have also tried to test with mmc_spi, but I have not
>>> been able to make that driver work reliably in any recent kernel
>>> versions.
>>> Most of the time I see timeouts - and with lots of different SD-cards...
>>>
>>> IIRC the last time I tested it successfully was with 3.12.
>>
>> It'd be great if you could use "git bisect" to track down the change
>> that broke this.
> 
> The problem is that it is a lot of kernel-versions I would have to test
> to figure out why and with the rpi used for compiling the kernel it 
> becomes a prohibitive long exercise...

Why not just cross-compile from a faster machine? It probably only takes
a few minutes per commit to test (and git bisect is pretty optimal
w.r.t. the number of commits you need to test).

> Also I may have a slightly different setup now compared to when I did
> the test initially, so this may be the trigger as well. 

That's also pretty easy to test; just go back and retest the old kernel
version that you believe was working. If it's still working but the
latest kernel is broken, there's been a regression. If the old kernel
also doesn't work, then something has changed in your setup.
--
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: bcm2835: move to the transfer_one driver model

[Martin Sperl]

> On 29.03.2015, at 05:24, Stephen Warren <swarren-3lzwWm7+Weoh9ZMKESR00Q@public.gmane.org> wrote:
> Why not just cross-compile from a faster machine? It probably only takes
> a few minutes per commit to test (and git bisect is pretty optimal
> w.r.t. the number of commits you need to test).
This never was high on my priority and I never felt the need...

> That's also pretty easy to test; just go back and retest the old kernel
> version that you believe was working. If it's still working but the
> latest kernel is broken, there's been a regression. If the old kernel
> also doesn't work, then something has changed in your setup.
all obviously an option but time consuming and an effort...
Let us see if I find some time to test...

Martin

--
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: bcm2835: move to the transfer_one driver model

[Martin Sperl]

Hi Stephen!

> all obviously an option but time consuming and an effort...
> Let us see if I find some time to test...

I started with the foundation kernels (as I know I was working
with those at the time):
3.8.13  - d996a1b91b2bf3dc06f4f4f822a56f4496457aa1 - WORKS
3.9.11  - d5572370289f698b101f3d0198b1c99f17f0d278 - WORKS
3.10.38 - 1b49b450222df26e4abf7abb6d9302f72b2ed386 - FAIL
3.12.36 - ee9b8c7d46f2b1787b1e64604acafc70f70191cf - FAIL

I will now try the same with the mainline kernel comparing 3.9
and 3.10 and maybe I can come up with something...
(unfortunately this is still quite time-consuming - 
even cross-compiling)

Note that some comments in commit messages of rpi-linux talk 
about some backport of code from 3.13, so this may be the real
reason...

Here the commit inside the foundation kernel that I refer to:

commit cff74f34502d46a6160dba6572db5f936cfa80ae
Author: ghollingworth <gordon-FnsA7b+Nu9XbIbC87yuRow@public.gmane.org>
Date:   Fri Mar 21 12:14:41 2014 +0000

    Support eMMC 5.1 cards

    Already upstream and in 3.13


>From the diff of drivers/mmc it also looks as if there is more 
error-handling code in the generic mmc code which may trigger
now...

One other issue with the upstream kernel is that for 3.9 there is
no spi-bcm2835 driver, so testing against does not work - it only
got included with 3.10, so testing is futile...

I will try to see if the commit 0385850e67359838f7d63 which is
right before cff74f34502d46a6160db is working or not and then
check if that commit introduced the issue.

Martin



--
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: bcm2835: move to the transfer_one driver model

[Martin Sperl]

> On 28.03.2015, at 05:09, Stephen Warren <swarren-3lzwWm7+Weoh9ZMKESR00Q@public.gmane.org> wrote:
>> As for testing: I have also tried to test with mmc_spi, but I have not
>> been able to make that driver work reliably in any recent kernel
>> versions.
>> Most of the time I see timeouts - and with lots of different SD-cards...
>> 
>> IIRC the last time I tested it successfully was with 3.12.
> 
> It'd be great if you could use "git bisect" to track down the change
> that broke this.

Here some info on my testing with the upstream kernels:
3.12.0 - 5e01dc7b26d9f24f39abace5da98ccbd6a5ceb52 - WORKS
3.13.0 - d8ec26d7f8287f5788a494f56e8814210f0e64be - FAILS
which is what I had suspected.

Bisection between these shows the responsible commit is:

[sperl@build linux-upstream]$ git bisect bad
0589342c27944e50ebd7a54f5215002b6598b748 is the first bad commit
commit 0589342c27944e50ebd7a54f5215002b6598b748
Author: Rob Herring <rob.herring-bsGFqQB8/DxBDgjK7y7TUQ@public.gmane.org>
Date:   Tue Oct 29 23:36:46 2013 -0500

    of: set dma_mask to point to coherent_dma_mask

    Platform devices created by DT code don't initialize dma_mask pointer to
    anything. Set it to coherent_dma_mask by default if the architecture
    code has not set it.

    Signed-off-by: Rob Herring <rob.herring-bsGFqQB8/DxBDgjK7y7TUQ@public.gmane.org>

:040000 040000 805e59ffdb181a71299f354ee7cca727dad4778a 5445e765b6be497d45af3f108c3f38ef6aabb44a M	drivers

It is a surprising result and does not really explain the situation.

I did check the last working (0589342c2794) and non 
working (13ccacd5945a) commits again after cleaning the builds
and rebuilding from scratch.

I did not believe this so I reran the bisect just to make sure - 
this time also rebuilding the dtb every time.

Now I went back to 4.0.rc5+ and patched it as follows:
--- a/drivers/of/platform.c
+++ b/drivers/of/platform.c
@@ -179,9 +179,10 @@ static void of_dma_configure(struct device *dev)
         * Set it to coherent_dma_mask by default if the architecture
         * code has not set it.
         */
+/*
        if (!dev->dma_mask)
                dev->dma_mask = &dev->coherent_dma_mask;
-
+*/
        ret = of_dma_get_range(dev->of_node, &dma_addr, &paddr, &size);
        if (ret < 0) {
                dma_addr = offset = 0;

And now it works - the SD card gets detected immediately,
I can mount it and everything...

Is this something we need to fix in bcm2835 by using the "correct"
coherent_dma_mask?

Still I do not understand why we need dma_mask not set for the
mmc_spi driver and how it influences the behaviour...

Martin

P.s: Note that I have built like this:
make bcm2835_defconfig
echo "CONFIG_MMC_SPI=y" >> .config

using the following diff to enable mmc_spi in the device-tree:
--- a/arch/arm/boot/dts/bcm2835-rpi-b.dts
+++ b/arch/arm/boot/dts/bcm2835-rpi-b.dts
@@ -50,3 +50,14 @@
 	status = "okay";
 	bus-width = <4>;
 };
+
+&spi {
+	status = "okay";
+	sd1: sd@1 {
+		reg = <1>;
+		status = "okay";
+		compatible = "spi,mmc_spi";
+		voltage-ranges = <3200 3500>;
+		spi-max-frequency = <8000000>;
+	};
+};


Note if you want to repeat this, then you have to boot with the
SD card removed, because otherwise mmc0 will be the sd card on
the SPI bus and not the "native" SD card...

I have now also patched out the changes introduces with the patch
in a 4.0rc and the result is 

here the bisect log:
git bisect start
# good: [5e01dc7b26d9f24f39abace5da98ccbd6a5ceb52] Linux 3.12
git bisect good 5e01dc7b26d9f24f39abace5da98ccbd6a5ceb52
# bad: [d8ec26d7f8287f5788a494f56e8814210f0e64be] Linux 3.13
git bisect bad d8ec26d7f8287f5788a494f56e8814210f0e64be
# bad: [42a2d923cc349583ebf6fdd52a7d35e1c2f7e6bd] Merge git://git.kernel.org/pub/scm/linux/kernel/git/davem/net-next
git bisect bad 42a2d923cc349583ebf6fdd52a7d35e1c2f7e6bd
# good: [4b4d2b463461f1b86fd89353184e6f2938e7566b] Merge tag 'h8300-for-linus' of git://git.kernel.org/pub/scm/linux/kernel/git/groeck/linux-staging
git bisect good 4b4d2b463461f1b86fd89353184e6f2938e7566b
# bad: [5cbb3d216e2041700231bcfc383ee5f8b7fc8b74] Merge branch 'akpm' (patches from Andrew Morton)
git bisect bad 5cbb3d216e2041700231bcfc383ee5f8b7fc8b74
# good: [eeab517b68beb9e044e869bee18e3bdfa60e5aca] Merge tag 'sound-3.13-rc1' of git://git.kernel.org/pub/scm/linux/kernel/git/tiwai/sound
git bisect good eeab517b68beb9e044e869bee18e3bdfa60e5aca
# bad: [07eb6597c059d9276c8dc7cd0401083ed66ad714] MAINTAINERS: remove Richard Purdie as backlight maintainer
git bisect bad 07eb6597c059d9276c8dc7cd0401083ed66ad714
# good: [85b656cf1560e27a89354a23f2c10ba229d2f173] Merge branch 'for-next' of git://git.kernel.org/pub/scm/linux/kernel/git/cooloney/linux-leds
git bisect good 85b656cf1560e27a89354a23f2c10ba229d2f173
# bad: [a8f70de37bbb991033208edff7758d997d68db37] ocfs2: use bitmap_weight()
git bisect bad a8f70de37bbb991033208edff7758d997d68db37
# good: [b5b4bb3f6a11f9c37b6d53138244f2ffe5bacd12] of: only include prom.h on sparc
git bisect good b5b4bb3f6a11f9c37b6d53138244f2ffe5bacd12
# good: [355e62f5ad12b005c862838156262eb2df2f8dff] of/irq: Fix potential buffer overflow
git bisect good 355e62f5ad12b005c862838156262eb2df2f8dff
# bad: [74dac2ed699cbe1dee0e4e7891619d53a5f2632f] of: irq: Fix interrupt-map entry matching
git bisect bad 74dac2ed699cbe1dee0e4e7891619d53a5f2632f
# bad: [f0e1bfbd2d1b0900a56c25cea35be2467a5cfc32] of: Add AU Optronics Corporation vendor prefix
git bisect bad f0e1bfbd2d1b0900a56c25cea35be2467a5cfc32
# good: [67aeb39a173b5c172c75c5b9f1844853aec16eb4] of: Add vendor prefix for Cadence
git bisect good 67aeb39a173b5c172c75c5b9f1844853aec16eb4
# good: [13ccacd5945aa5ce81d8566f57587e95fbd90742] of: add vendor prefix for PHYTEC Messtechnik GmbH
git bisect good 13ccacd5945aa5ce81d8566f57587e95fbd90742
# bad: [0589342c27944e50ebd7a54f5215002b6598b748] of: set dma_mask to point to coherent_dma_mask
git bisect bad 0589342c27944e50ebd7a54f5215002b6598b748
# first bad commit: [0589342c27944e50ebd7a54f5215002b6598b748] of: set dma_mask to point to coherent_dma_mask

--
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: bcm2835: move to the transfer_one driver model

[Stephen Warren]

On 03/31/2015 08:49 AM, Martin Sperl wrote:
>
>> On 28.03.2015, at 05:09, Stephen Warren <swarren-3lzwWm7+Weoh9ZMKESR00Q@public.gmane.org> wrote:
>>> As for testing: I have also tried to test with mmc_spi, but I have not
>>> been able to make that driver work reliably in any recent kernel
>>> versions.
>>> Most of the time I see timeouts - and with lots of different SD-cards...
>>>
>>> IIRC the last time I tested it successfully was with 3.12.
>>
>> It'd be great if you could use "git bisect" to track down the change
>> that broke this.
...
> Bisection between these shows the responsible commit is:
...
> commit 0589342c27944e50ebd7a54f5215002b6598b748
> Author: Rob Herring <rob.herring-bsGFqQB8/DxBDgjK7y7TUQ@public.gmane.org>
> Date:   Tue Oct 29 23:36:46 2013 -0500
>
>      of: set dma_mask to point to coherent_dma_mask
>
>      Platform devices created by DT code don't initialize dma_mask pointer to
>      anything. Set it to coherent_dma_mask by default if the architecture
>      code has not set it.
>
>      Signed-off-by: Rob Herring <rob.herring-bsGFqQB8/DxBDgjK7y7TUQ@public.gmane.org>
...
> Now I went back to 4.0.rc5+ and patched it as follows:
> --- a/drivers/of/platform.c
> +++ b/drivers/of/platform.c
> @@ -179,9 +179,10 @@ static void of_dma_configure(struct device *dev)
>           * Set it to coherent_dma_mask by default if the architecture
>           * code has not set it.
>           */
> +/*
>          if (!dev->dma_mask)
>                  dev->dma_mask = &dev->coherent_dma_mask;
> -
> +*/
>          ret = of_dma_get_range(dev->of_node, &dma_addr, &paddr, &size);
>          if (ret < 0) {
>                  dma_addr = offset = 0;
>
> And now it works - the SD card gets detected immediately,
> I can mount it and everything...

Does that change cause the SDHCI core to use vs. not-use any of the 
SDHCI DMA modes? If so, I wonder if this is because the upstream kernel 
doesn't deal with the bcm2835's ARM physical address <-> SoC bus address 
conversions, which in turn can cause DMA and CPU access to not use the 
same caching settings and become incoherent, which in turn can cause DMA 
data corruption. See the following link for some background:

https://www.mail-archive.com/u-boot-0aAXYlwwYIKGBzrmiIFOJg@public.gmane.org/msg167376.html
[PATCH 2/3] ARM: bcm2835: implement phys_to_bus/bus_to_phys

(you'll want to read all of patches 1-3 for the complete background I 
think, but patch 2 describes the HW issue)
--
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: bcm2835: move to the transfer_one driver model

[Martin Sperl]

> On 01.04.2015, at 17:20, Stephen Warren <swarren-3lzwWm7+Weoh9ZMKESR00Q@public.gmane.org> wrote:
> 
> Does that change cause the SDHCI core to use vs. not-use any of the SDHCI DMA modes? If so, I wonder if this is because the upstream kernel doesn't deal with the bcm2835's ARM physical address <-> SoC bus address conversions, which in turn can cause DMA and CPU access to not use the same caching settings and become incoherent, which in turn can cause DMA data corruption. See the following link for some background:
> 
> https://www.mail-archive.com/u-boot-0aAXYlwwYIKGBzrmiIFOJg@public.gmane.org/msg167376.html
> [PATCH 2/3] ARM: bcm2835: implement phys_to_bus/bus_to_phys
> 
> (you'll want to read all of patches 1-3 for the complete background I think, but patch 2 describes the HW issue)

I can also just patch out the assignement to dev_dma here and it works as well:
	if (spi->master->dev.parent->dma_mask) {
		struct device   *dev = spi->master->dev.parent;

		host->dma_dev = dev;
		host->ones_dma = dma_map_single(dev, ones,
				MMC_SPI_BLOCKSIZE, DMA_TO_DEVICE);
		host->data_dma = dma_map_single(dev, host->data,
				sizeof(*host->data), DMA_BIDIRECTIONAL);
So DMA there is not an issue with SDHCI - it is just when using the
"normal" spi-bcm2835 that I can reproduce it.

If you read the mmc_spi driver there is a lot of places where we have 
constructs like this:
        if (host->dma_dev) {
                host->m.is_dma_mapped = 1;
                dma_sync_single_for_device(host->dma_dev,
                                host->data_dma, sizeof(*host->data),
                                DMA_BIDIRECTIONAL);
        }
        status = spi_sync_locked(host->spi, &host->m);

        if (host->dma_dev)
                dma_sync_single_for_cpu(host->dma_dev,
                                host->data_dma, sizeof(*host->data),
                                DMA_BIDIRECTIONAL);

Similar constructs exist for: dma_map_page and dma_unmap_page.

As far as I can tell these are the "typical" dma-related calls.

But the point is that the spi-bcm2835 driver does not implement DMA (yet),
so some things may not be fully set up for mapping inside 
spi->master->dev.parent and some mapping options may fail/not work as
expected. This may results in possibly "bad" behavior, just because dma_mask
 is set.

So i am not sure if this "mapping" issue you are mentioning is really the
root-cause - also I test on a RPI1, but have also tried on a RPI2 with the 
foundation kernel.

On top my reading/experience with DMA on the bcm2835 is that I was always
using 0xC0000000 as the bus-address, as the L1/L2 addresses in the
documentation are only related to VideoCore caches - and that is I guess
why the "L2 preload" is explicitly mentioned in the documentation - probably
to speed up the GPU by  prefetching the necessary data via DMA and thus 
avoiding to wait for data to get read from SRAM.

Unfortunately I can not test if the the mmc_spi issue exists on a
beaglebone, as the mmc_spi driver can not even get compiled there because
of CONFIG_HIGHMEM being set which conflicts with KConfig portion for
mmc_spi...


Ciao,
	Martin


--
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: bcm2835: move to the transfer_one driver model

[Mark Brown]

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

On Fri, Apr 03, 2015 at 12:17:31PM +0200, Martin Sperl wrote:
> > On 01.04.2015, at 17:20, Stephen Warren <swarren-3lzwWm7+Weoh9ZMKESR00Q@public.gmane.org> wrote:

It looks like something rewrote Stephen's message to have no word
wrapping...

> If you read the mmc_spi driver there is a lot of places where we have 
> constructs like this:
>         if (host->dma_dev) {
>                 host->m.is_dma_mapped = 1;
>                 dma_sync_single_for_device(host->dma_dev,
>                                 host->data_dma, sizeof(*host->data),
>                                 DMA_BIDIRECTIONAL);
>         }
>         status = spi_sync_locked(host->spi, &host->m);

We probably need to have a good, hard look at this stuff - in general we
shouldn't be using this sort of mapping in clients, and for some systems
this will actually end up broken.

> Unfortunately I can not test if the the mmc_spi issue exists on a
> beaglebone, as the mmc_spi driver can not even get compiled there because
> of CONFIG_HIGHMEM being set which conflicts with KConfig portion for
> mmc_spi...

This is a bit alarming and probably also indicates that the code needs
looking at.

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

[PATCH] SPI: BCM2835: clock divider can be a multiple of 2

[kernel-TqfNSX0MhmxHKSADF0wUEw]

From: Martin Sperl <kernel-TqfNSX0MhmxHKSADF0wUEw@public.gmane.org>

The official documentation is wrong in this respect.
Has been tested empirically for dividers 2-1024

Signed-off-by: Martin Sperl <kernel-TqfNSX0MhmxHKSADF0wUEw@public.gmane.org>
---
 drivers/spi/spi-bcm2835.c |    5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/spi/spi-bcm2835.c b/drivers/spi/spi-bcm2835.c
index e1dea40..779e3a8 100644
--- a/drivers/spi/spi-bcm2835.c
+++ b/drivers/spi/spi-bcm2835.c
@@ -192,8 +192,9 @@ static int bcm2835_spi_start_transfer(
 	if (spi_hz >= clk_hz / 2) {
 		cdiv = 2; /* clk_hz/2 is the fastest we can go */
 	} else if (spi_hz) {
-		/* CDIV must be a power of two */
-		cdiv = roundup_pow_of_two(DIV_ROUND_UP(clk_hz, spi_hz));
+		/* CDIV must be a multiple of two */
+		cdiv = DIV_ROUND_UP(clk_hz, spi_hz);
+		cdiv += (cdiv % 2);
 
 		if (cdiv >= 65536)
 			cdiv = 0; /* 0 is the slowest we can go */
-- 
1.7.10.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] SPI: BCM2835: clock divider can be a multiple of 2

[Stephen Warren]

On 03/19/2015 03:01 AM, kernel-TqfNSX0MhmxHKSADF0wUEw@public.gmane.org wrote:
> From: Martin Sperl <kernel-TqfNSX0MhmxHKSADF0wUEw@public.gmane.org>
> 
> The official documentation is wrong in this respect.
> Has been tested empirically for dividers 2-1024

Do you have a link to a confirmation of this from the RPi Foundation or
Broadcom, even a forum post or something? How does the RPI Foundation's
downstream kernel restrict the divider?
--
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: BCM2835: clock divider can be a multiple of 2

[Martin Sperl]

> On 20.03.2015, at 06:06, Stephen Warren <swarren-3lzwWm7+Weoh9ZMKESR00Q@public.gmane.org> wrote:
> 
> On 03/19/2015 03:01 AM, kernel-TqfNSX0MhmxHKSADF0wUEw@public.gmane.org wrote:
>> From: Martin Sperl <kernel-TqfNSX0MhmxHKSADF0wUEw@public.gmane.org>
>> 
>> The official documentation is wrong in this respect.
>> Has been tested empirically for dividers 2-1024
> 
> Do you have a link to a confirmation of this from the RPi Foundation or
> Broadcom, even a forum post or something? How does the RPI Foundation's
> downstream kernel restrict the divider?

Unfortunately there are no official errata by the foundation.

Here the argument by a foundation official (Gordon Hollingworth)
why they do not (want to) provide updates:
http://www.raspberrypi.org/forums/viewtopic.php?p=447724#p447724

That is in a slightly different context, but still it also applies to
errata in general.

The foundation spi-bcm2708 driver also relies on the power-of 2 rule
following to the letter the documentation they provide.

For some more infos of experience please look here:
https://github.com/notro/spi-bcm2708/wiki
http://www.raspberrypi.org/forums/viewtopic.php?f=44&t=43442
http://elinux.org/BCM2835_datasheet_errata#p156

Notro and I have been running an out of tree driver for a long time
and that never showed any issues with this multiple of 2.

Some of these kernels/drivers have been shared with lots of people
for the use with TFT displays or CAN controllers.

I have measured this empirically for every divider between 2 to 1024
by doing some transfers, and measuring them with a SALEAE logic analyzer.

See here: for the analysis:
https://github.com/msperl/spi-bcm2835/issues/8

You can also fetch the raw data and csv exports here:
https://github.com/msperl/spi-bcm2835/blob/patch_the_upstream/images/

Note that the descriptions of how to do polled, interrupt driven and 
DMA driven SPI transfers (page 158) is also not completely accurate.
It seems to show some "procedures", but these are not optimized and
some are inconsistent: e.g: this limitation of 16/12 byte transfers
while the register documentation says 16 words not bytes - which means 
64 bytes can enter the FIFO buffer (which is also the limit when
the HW signals that it is full via BCM2835_SPI_CS_TXD)

This will be one of the next patches for improvement of the driver.

Martin

--
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: BCM2835: clock divider can be a multiple of 2

[Noralf Trønnes]

Den 20.03.2015 08:04, skrev Martin Sperl:
>> On 20.03.2015, at 06:06, Stephen Warren <swarren-3lzwWm7+Weoh9ZMKESR00Q@public.gmane.org> wrote:
>>
>> On 03/19/2015 03:01 AM, kernel-TqfNSX0MhmxHKSADF0wUEw@public.gmane.org wrote:
>>> From: Martin Sperl <kernel-TqfNSX0MhmxHKSADF0wUEw@public.gmane.org>
>>>
>>> The official documentation is wrong in this respect.
>>> Has been tested empirically for dividers 2-1024
>> Do you have a link to a confirmation of this from the RPi Foundation or
>> Broadcom, even a forum post or something? How does the RPI Foundation's
>> downstream kernel restrict the divider?
> Unfortunately there are no official errata by the foundation.
>
> Here the argument by a foundation official (Gordon Hollingworth)
> why they do not (want to) provide updates:
> http://www.raspberrypi.org/forums/viewtopic.php?p=447724#p447724
>
> That is in a slightly different context, but still it also applies to
> errata in general.
>
> The foundation spi-bcm2708 driver also relies on the power-of 2 rule
> following to the letter the documentation they provide.
>
> For some more infos of experience please look here:
> https://github.com/notro/spi-bcm2708/wiki
> http://www.raspberrypi.org/forums/viewtopic.php?f=44&t=43442
> http://elinux.org/BCM2835_datasheet_errata#p156
>
> Notro and I have been running an out of tree driver for a long time
> and that never showed any issues with this multiple of 2.
>
> Some of these kernels/drivers have been shared with lots of people
> for the use with TFT displays or CAN controllers.
>
> I have measured this empirically for every divider between 2 to 1024
> by doing some transfers, and measuring them with a SALEAE logic analyzer.
>
> See here: for the analysis:
> https://github.com/msperl/spi-bcm2835/issues/8
>
> You can also fetch the raw data and csv exports here:
> https://github.com/msperl/spi-bcm2835/blob/patch_the_upstream/images/
>
> Note that the descriptions of how to do polled, interrupt driven and
> DMA driven SPI transfers (page 158) is also not completely accurate.
> It seems to show some "procedures", but these are not optimized and
> some are inconsistent: e.g: this limitation of 16/12 byte transfers
> while the register documentation says 16 words not bytes - which means
> 64 bytes can enter the FIFO buffer (which is also the limit when
> the HW signals that it is full via BCM2835_SPI_CS_TXD)
>
> This will be one of the next patches for improvement of the driver.

The out-of-tree github.com/notro/spi-bcm2708.c is used in numerous
installations since may 2013. I just heard of a 50,000 order for a
particular SPI TFT display.
This driver has no restriction on CDIV except for boundary checking:
         cdiv = DIV_ROUND_UP(bus_hz, hz);

Many of these displays also have a touch controller on the same bus.
AFAIK the most common default speeds used with displays are: 
16,24,32,48,64,80,128MHz
Default transmit buffer size is 4k. The touch controller is set to 2MHz.
I haven't heard any reports of this driver being used for reading large 
buffers.

To my knowledge there hasn't been any issues with lifting this CDIV 
restriction.


Regards,
Noralf Trønnes

Re: [PATCH] SPI: BCM2835: clock divider can be a multiple of 2

[Mark Brown]

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

On Thu, Mar 19, 2015 at 09:01:52AM +0000, kernel-TqfNSX0MhmxHKSADF0wUEw@public.gmane.org wrote:
> From: Martin Sperl <kernel-TqfNSX0MhmxHKSADF0wUEw@public.gmane.org>
> 
> The official documentation is wrong in this respect.
> Has been tested empirically for dividers 2-1024

Applied, thanks.

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

[PATCH] SPI: BCM2835: enable support of 3-wire mode

[kernel-TqfNSX0MhmxHKSADF0wUEw]

From: Martin Sperl <kernel-TqfNSX0MhmxHKSADF0wUEw@public.gmane.org>

Signed-off-by: Martin Sperl <kernel-TqfNSX0MhmxHKSADF0wUEw@public.gmane.org>
---
 drivers/spi/spi-bcm2835.c |    6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/drivers/spi/spi-bcm2835.c b/drivers/spi/spi-bcm2835.c
index e1dea40..51883f8 100644
--- a/drivers/spi/spi-bcm2835.c
+++ b/drivers/spi/spi-bcm2835.c
@@ -67,7 +67,8 @@
 #define BCM2835_SPI_CS_CS_01		0x00000001
 
 #define BCM2835_SPI_TIMEOUT_MS	30000
-#define BCM2835_SPI_MODE_BITS	(SPI_CPOL | SPI_CPHA | SPI_CS_HIGH | SPI_NO_CS)
+#define BCM2835_SPI_MODE_BITS	(SPI_CPOL | SPI_CPHA | SPI_CS_HIGH \
+				| SPI_NO_CS | SPI_3WIRE)
 
 #define DRV_NAME	"spi-bcm2835"
 
@@ -201,6 +202,9 @@ static int bcm2835_spi_start_transfer(
 		cdiv = 0; /* 0 is the slowest we can go */
 	}
 
+	if ((spi->mode & SPI_3WIRE) && (tfr->rx_buf))
+		cs |= BCM2835_SPI_CS_REN;
+
 	if (spi->mode & SPI_CPOL)
 		cs |= BCM2835_SPI_CS_CPOL;
 	if (spi->mode & SPI_CPHA)
-- 
1.7.10.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] SPI: BCM2835: enable support of 3-wire mode

[Stephen Warren]

On 03/19/2015 03:01 AM, kernel-TqfNSX0MhmxHKSADF0wUEw@public.gmane.org wrote:

> diff --git a/drivers/spi/spi-bcm2835.c b/drivers/spi/spi-bcm2835.c

> @@ -201,6 +202,9 @@ static int bcm2835_spi_start_transfer(
>  		cdiv = 0; /* 0 is the slowest we can go */
>  	}
>  
> +	if ((spi->mode & SPI_3WIRE) && (tfr->rx_buf))
> +		cs |= BCM2835_SPI_CS_REN;
> +

I can see how that tells the HW which direction to transfer data in
3WIRE mode, but how does the HW know whether it's in 3WIRE mode or has
separate RX/TX lines?
--
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: BCM2835: enable support of 3-wire mode

[Martin Sperl]

> On 20.03.2015, at 06:12, Stephen Warren <swarren-3lzwWm7+Weoh9ZMKESR00Q@public.gmane.org> wrote:
> I can see how that tells the HW which direction to transfer data in
> 3WIRE mode, but how does the HW know whether it's in 3WIRE mode or has
> separate RX/TX lines?

The HW assumes with this flag that MISO (=RX) sits on the same physical
line as MOSI (=TX). This flag essentially just changes directions of
the TX GPIO pin from out to in and samples the level on the TX pin.

Without this patch what you would need to do to make such a device
work is connecting RX with TX and that with the MIMO pin of the final
device (maybe add a pullup). This flag is essentially just a shortcut.

Note that the SPI-framework does the testing so that either RX or TX
can be set in SPI-3WIRE mode (not both).

Ciao,
	Martin



--
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: BCM2835: enable support of 3-wire mode

[Mark Brown]

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

On Thu, Mar 19, 2015 at 09:01:53AM +0000, kernel-TqfNSX0MhmxHKSADF0wUEw@public.gmane.org wrote:
> From: Martin Sperl <kernel-TqfNSX0MhmxHKSADF0wUEw@public.gmane.org>
> 
> Signed-off-by: Martin Sperl <kernel-TqfNSX0MhmxHKSADF0wUEw@public.gmane.org>

Applied, thanks.

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

Re: [PATCH] SPI: BCM2835: fix all checkpath --strict messages

[Stephen Warren]

On 03/19/2015 03:01 AM, kernel-TqfNSX0MhmxHKSADF0wUEw@public.gmane.org wrote:
> From: Martin Sperl <kernel-TqfNSX0MhmxHKSADF0wUEw@public.gmane.org>

Do you have a list of the warnings this fixes and why? The extra
wrapping here looks a bit odd, since the lines before your patch are
well under 80 columns.
--
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: BCM2835: fix all checkpath --strict messages

[Martin Sperl]

> On 20.03.2015, at 05:58, Stephen Warren <swarren-3lzwWm7+Weoh9ZMKESR00Q@public.gmane.org> wrote:
> 
> Do you have a list of the warnings this fixes and why? The extra
> wrapping here looks a bit odd, since the lines before your patch are
> well under 80 columns.

scripts/checkpatch.pl --file --terse --strict drivers/spi/spi-bcm2835.c
drivers/spi/spi-bcm2835.c:182: CHECK: Alignment should match open parenthesis
drivers/spi/spi-bcm2835.c:191: CHECK: braces {} should be used on all arms of this statement
drivers/spi/spi-bcm2835.c:234: CHECK: Alignment should match open parenthesis
drivers/spi/spi-bcm2835.c:256: CHECK: Alignment should match open parenthesis
drivers/spi/spi-bcm2835.c:271: CHECK: Alignment should match open parenthesis
drivers/spi/spi-bcm2835.c:346: CHECK: Alignment should match open parenthesis
total: 0 errors, 0 warnings, 6 checks, 403 lines checked

Martin--
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: BCM2835: fix all checkpath --strict messages

[Mark Brown]

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

On Fri, Mar 20, 2015 at 07:26:29AM +0100, Martin Sperl wrote:
> > On 20.03.2015, at 05:58, Stephen Warren <swarren-3lzwWm7+Weoh9ZMKESR00Q@public.gmane.org> wrote:

> > Do you have a list of the warnings this fixes and why? The extra
> > wrapping here looks a bit odd, since the lines before your patch are
> > well under 80 columns.

> scripts/checkpatch.pl --file --terse --strict drivers/spi/spi-bcm2835.c
> drivers/spi/spi-bcm2835.c:182: CHECK: Alignment should match open parenthesis

This is telling you to add more indentation to the second argument not
wrap the first.  Though frankly I don't think it matters - it's
certainly not a routinely followed thing and it frequently makes things
worse.

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

[PATCH V2] SPI: BCM2835: fix all checkpath --strict messages

[Martin Sperl]

The following errors/warnings issued by checkpatch.pl --strict have been fixed:
drivers/spi/spi-bcm2835.c:182: CHECK: Alignment should match open parenthesis
drivers/spi/spi-bcm2835.c:191: CHECK: braces {} should be used on all arms of this statement
drivers/spi/spi-bcm2835.c:234: CHECK: Alignment should match open parenthesis
drivers/spi/spi-bcm2835.c:256: CHECK: Alignment should match open parenthesis
drivers/spi/spi-bcm2835.c:271: CHECK: Alignment should match open parenthesis
drivers/spi/spi-bcm2835.c:346: CHECK: Alignment should match open parenthesis
total: 0 errors, 0 warnings, 6 checks, 403 lines checked

In 2 locations the arguments had to get split/moved to the next line so that the
line width stays below 80 chars.

Signed-off-by: Martin Sperl <kernel-TqfNSX0MhmxHKSADF0wUEw@public.gmane.org>
---
 drivers/spi/spi-bcm2835.c |   18 +++++++++++-------
 1 file changed, 11 insertions(+), 7 deletions(-)

Changelog:
  v2: minimized changes where possible

diff --git a/drivers/spi/spi-bcm2835.c b/drivers/spi/spi-bcm2835.c
index 419a782..779d3a8 100644
--- a/drivers/spi/spi-bcm2835.c
+++ b/drivers/spi/spi-bcm2835.c
@@ -179,7 +179,7 @@ static irqreturn_t bcm2835_spi_interrupt(int irq, void *dev_id)
 }
 
 static int bcm2835_spi_start_transfer(struct spi_device *spi,
-		struct spi_transfer *tfr)
+				      struct spi_transfer *tfr)
 {
 	struct bcm2835_spi *bs = spi_master_get_devdata(spi->master);
 	unsigned long spi_hz, clk_hz, cdiv;
@@ -196,8 +196,9 @@ static int bcm2835_spi_start_transfer(struct spi_device *spi,
 
 		if (cdiv >= 65536)
 			cdiv = 0; /* 0 is the slowest we can go */
-	} else
+	} else {
 		cdiv = 0; /* 0 is the slowest we can go */
+	}
 
 	if (spi->mode & SPI_CPOL)
 		cs |= BCM2835_SPI_CS_CPOL;
@@ -231,7 +232,8 @@ static int bcm2835_spi_start_transfer(struct spi_device *spi,
 }
 
 static int bcm2835_spi_finish_transfer(struct spi_device *spi,
-		struct spi_transfer *tfr, bool cs_change)
+				       struct spi_transfer *tfr,
+				       bool cs_change)
 {
 	struct bcm2835_spi *bs = spi_master_get_devdata(spi->master);
 	u32 cs = bcm2835_rd(bs, BCM2835_SPI_CS);
@@ -253,7 +255,7 @@ static int bcm2835_spi_finish_transfer(struct spi_device *spi,
 }
 
 static int bcm2835_spi_transfer_one(struct spi_master *master,
-		struct spi_message *mesg)
+				    struct spi_message *mesg)
 {
 	struct bcm2835_spi *bs = spi_master_get_devdata(master);
 	struct spi_transfer *tfr;
@@ -267,8 +269,10 @@ static int bcm2835_spi_transfer_one(struct spi_master *master,
 		if (err)
 			goto out;
 
-		timeout = wait_for_completion_timeout(&bs->done,
-				msecs_to_jiffies(BCM2835_SPI_TIMEOUT_MS));
+		timeout = wait_for_completion_timeout(
+			&bs->done,
+			msecs_to_jiffies(BCM2835_SPI_TIMEOUT_MS)
+			);
 		if (!timeout) {
 			err = -ETIMEDOUT;
 			goto out;
@@ -343,7 +347,7 @@ static int bcm2835_spi_probe(struct platform_device *pdev)
 	clk_prepare_enable(bs->clk);
 
 	err = devm_request_irq(&pdev->dev, bs->irq, bcm2835_spi_interrupt, 0,
-				dev_name(&pdev->dev), master);
+			       dev_name(&pdev->dev), master);
 	if (err) {
 		dev_err(&pdev->dev, "could not request IRQ: %d\n", err);
 		goto out_clk_disable;
-- 
1.7.10.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 V2] SPI: BCM2835: fix all checkpath --strict messages

[Mark Brown]

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

On Fri, Mar 20, 2015 at 03:26:11PM +0100, Martin Sperl wrote:
> The following errors/warnings issued by checkpatch.pl --strict have been fixed:
> drivers/spi/spi-bcm2835.c:182: CHECK: Alignment should match open parenthesis

Applied, but please use subject lines matching the style for the
subsystem and don't bury new patches in the middle of existing threads
- both make it hard to find and follow things.

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

Re: [PATCH] SPI: BCM2835: fix all checkpath --strict messages

[Mark Brown]

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

On Thu, Mar 19, 2015 at 09:01:50AM +0000, kernel-TqfNSX0MhmxHKSADF0wUEw@public.gmane.org wrote:

Looks like I did get these but they took a while to come through...
odd.

> -static int bcm2835_spi_start_transfer(struct spi_device *spi,
> -		struct spi_transfer *tfr)
> +static int bcm2835_spi_start_transfer(
> +	struct spi_device *spi,
> +	struct spi_transfer *tfr)

Like Stephen says I'm not 100% sure what issues this is fixing but I
have to say I find the overall result worse.  It's possible the
warning is something reasonable but without knowing what it is it's hard
to say, I do like to see at least the first argument on the same line as
the function name though.

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