diff mbox

spi: omap2-mcspi: Fix native cs with new set_cs

Message ID 1431452337-19280-1-git-send-email-mwelling@ieee.org (mailing list archive)
State New, archived
Headers show

Commit Message

Michael Welling May 12, 2015, 5:38 p.m. UTC
GPIO chip select patch series appears to have broken the native chip select
support. This patch pulls the manual native chip select toggling out of
the transfer_one routine and adds a set_cs routine.

Tested natively on AM3354 with SPI serial flash on spi0cs0.

Signed-off-by: Michael Welling <mwelling@ieee.org>
---
 drivers/spi/spi-omap2-mcspi.c |   33 +++++++++++----------------------
 1 file changed, 11 insertions(+), 22 deletions(-)

Comments

Nishanth Menon May 12, 2015, 6:57 p.m. UTC | #1
On 05/12/2015 12:38 PM, Michael Welling wrote:
> GPIO chip select patch series appears to have broken the native chip select
> support. This patch pulls the manual native chip select toggling out of
> the transfer_one routine and adds a set_cs routine.
> 
> Tested natively on AM3354 with SPI serial flash on spi0cs0.
> 
> Signed-off-by: Michael Welling <mwelling@ieee.org>
> ---
>  drivers/spi/spi-omap2-mcspi.c |   33 +++++++++++----------------------
>  1 file changed, 11 insertions(+), 22 deletions(-)
> 
> diff --git a/drivers/spi/spi-omap2-mcspi.c b/drivers/spi/spi-omap2-mcspi.c
> index 90cf7e7..a7d85c5 100644
> --- a/drivers/spi/spi-omap2-mcspi.c
> +++ b/drivers/spi/spi-omap2-mcspi.c
> @@ -243,17 +243,20 @@ static void omap2_mcspi_set_enable(const struct spi_device *spi, int enable)
>  	mcspi_read_cs_reg(spi, OMAP2_MCSPI_CHCTRL0);
>  }
>  
> -static void omap2_mcspi_force_cs(struct spi_device *spi, int cs_active)
> +static void omap2_mcspi_set_cs(struct spi_device *spi, bool enable)
>  {
>  	u32 l;
>  
> -	l = mcspi_cached_chconf0(spi);
> -	if (cs_active)
> -		l |= OMAP2_MCSPI_CHCONF_FORCE;
> -	else
> -		l &= ~OMAP2_MCSPI_CHCONF_FORCE;
> +	if (spi->controller_state) {
> +		l = mcspi_cached_chconf0(spi);
>  
> -	mcspi_write_chconf0(spi, l);
> +		if (enable)
> +			l &= ~OMAP2_MCSPI_CHCONF_FORCE;
> +		else
> +			l |= OMAP2_MCSPI_CHCONF_FORCE;
> +
> +		mcspi_write_chconf0(spi, l);
> +	}
>  }
>  
>  static void omap2_mcspi_set_master_mode(struct spi_master *master)
> @@ -1075,7 +1078,6 @@ static int omap2_mcspi_work_one(struct omap2_mcspi *mcspi,
>  
>  	struct spi_master		*master;
>  	struct omap2_mcspi_dma		*mcspi_dma;
> -	int				cs_active = 0;
>  	struct omap2_mcspi_cs		*cs;
>  	struct omap2_mcspi_device_config *cd;
>  	int				par_override = 0;
> @@ -1118,11 +1120,6 @@ static int omap2_mcspi_work_one(struct omap2_mcspi *mcspi,
>  			mcspi_read_cs_reg(spi, OMAP2_MCSPI_MODULCTRL);
>  	}
>  
> -	if (!cs_active) {
> -		omap2_mcspi_force_cs(spi, 1);
> -		cs_active = 1;
> -	}
> -
>  	chconf = mcspi_cached_chconf0(spi);
>  	chconf &= ~OMAP2_MCSPI_CHCONF_TRM_MASK;
>  	chconf &= ~OMAP2_MCSPI_CHCONF_TURBO;
> @@ -1169,12 +1166,6 @@ static int omap2_mcspi_work_one(struct omap2_mcspi *mcspi,
>  	if (t->delay_usecs)
>  		udelay(t->delay_usecs);
>  
> -	/* ignore the "leave it on after last xfer" hint */
> -	if (t->cs_change) {
> -		omap2_mcspi_force_cs(spi, 0);
> -		cs_active = 0;
> -	}
> -
>  	omap2_mcspi_set_enable(spi, 0);
>  
>  	if (mcspi->fifo_depth > 0)
> @@ -1187,9 +1178,6 @@ out:
>  		status = omap2_mcspi_setup_transfer(spi, NULL);
>  	}
>  
> -	if (cs_active)
> -		omap2_mcspi_force_cs(spi, 0);
> -
>  	if (cd && cd->cs_per_word) {
>  		chconf = mcspi->ctx.modulctrl;
>  		chconf |= OMAP2_MCSPI_MODULCTRL_SINGLE;
> @@ -1334,6 +1322,7 @@ static int omap2_mcspi_probe(struct platform_device *pdev)
>  	master->setup = omap2_mcspi_setup;
>  	master->auto_runtime_pm = true;
>  	master->transfer_one = omap2_mcspi_transfer_one;
> +	master->set_cs = omap2_mcspi_set_cs;
>  	master->cleanup = omap2_mcspi_cleanup;
>  	master->dev.of_node = node;
>  	master->max_speed_hz = OMAP2_MCSPI_MAX_FREQ;
> 

Tested on next-20150512
http://paste.ubuntu.org.cn/2600748
Since the original issue was reported by me in
http://marc.info/?t=143136312900001&r=1&w=2
Reported-by: Nishanth Menon <nm@ti.com>

Tested-by: Nishanth Menon <nm@ti.com>
Mark Brown May 12, 2015, 7:17 p.m. UTC | #2
On Tue, May 12, 2015 at 12:38:57PM -0500, Michael Welling wrote:
> GPIO chip select patch series appears to have broken the native chip select
> support. This patch pulls the manual native chip select toggling out of
> the transfer_one routine and adds a set_cs routine.

Applied, thanks
Mark Brown May 12, 2015, 7:19 p.m. UTC | #3
On Tue, May 12, 2015 at 01:57:36PM -0500, Nishanth Menon wrote:
> On 05/12/2015 12:38 PM, Michael Welling wrote:
> > GPIO chip select patch series appears to have broken the native chip select
> > support. This patch pulls the manual native chip select toggling out of
> > the transfer_one routine and adds a set_cs routine.

Please remember to delete unneeded context from your replies, the reader
shouldn't need to page through the entire patch to find out you reviewed
it.
Nishanth Menon May 12, 2015, 7:21 p.m. UTC | #4
On 05/12/2015 02:19 PM, Mark Brown wrote:
> On Tue, May 12, 2015 at 01:57:36PM -0500, Nishanth Menon wrote:
>> On 05/12/2015 12:38 PM, Michael Welling wrote:
>>> GPIO chip select patch series appears to have broken the native chip select
>>> support. This patch pulls the manual native chip select toggling out of
>>> the transfer_one routine and adds a set_cs routine.
> 
> Please remember to delete unneeded context from your replies, the reader
> shouldn't need to page through the entire patch to find out you reviewed
> it.

Will do so. Anyways, I did test it to be accurate - have'nt reviewed it.
Michael Welling May 21, 2015, 2:07 a.m. UTC | #5
On Tue, May 12, 2015 at 08:17:58PM +0100, Mark Brown wrote:
> On Tue, May 12, 2015 at 12:38:57PM -0500, Michael Welling wrote:
> > GPIO chip select patch series appears to have broken the native chip select
> > support. This patch pulls the manual native chip select toggling out of
> > the transfer_one routine and adds a set_cs routine.
> 
> Applied, thanks

It appears that in haste, this fix for the native cs support broke
the GPIO chip select support that I was originally shooting for.

[    2.653658] mcp23s08 spi1.1: TXS timed out
[    2.657961] mcp23s08 spi1.1: SPI transfer failed: -5
[    2.663305] spi_master spi1: failed to transfer one message from queue
[    2.670172] mcp23s08 spi1.1: can't setup chip 64, --> -5
[    2.675784] GPIO chip mcp23s08: gpiochip_unexport: status -19

My guess is that the set_cs needs to be called even when toggling as GPIO.

How should I handle this?
Mark Brown May 21, 2015, 10:18 a.m. UTC | #6
On Wed, May 20, 2015 at 09:07:09PM -0500, Michael Welling wrote:

> My guess is that the set_cs needs to be called even when toggling as GPIO.

> How should I handle this?

It shouldn't be part of a set_cs() operation but rather part of the main
transfer operation.
Michael Welling May 21, 2015, 9:04 p.m. UTC | #7
On Thu, May 21, 2015 at 11:18:57AM +0100, Mark Brown wrote:
> On Wed, May 20, 2015 at 09:07:09PM -0500, Michael Welling wrote:
> 
> > My guess is that the set_cs needs to be called even when toggling as GPIO.
> 
> > How should I handle this?
> 
> It shouldn't be part of a set_cs() operation but rather part of the main
> transfer operation.


Okay then this patch should be reverted.

Do you want to revert the patch and apply a new one or should I provide a
patch that reverts the changes and fixes it all in one?

Sorry for this mess.
Mark Brown May 21, 2015, 9:16 p.m. UTC | #8
On Thu, May 21, 2015 at 04:04:11PM -0500, Michael Welling wrote:

> Do you want to revert the patch and apply a new one or should I provide a
> patch that reverts the changes and fixes it all in one?

Can you please send me separate revert and re-add patches, that's
probably going to be easier to review.
Michael Welling May 21, 2015, 11:48 p.m. UTC | #9
On Thu, May 21, 2015 at 10:16:38PM +0100, Mark Brown wrote:
> On Thu, May 21, 2015 at 04:04:11PM -0500, Michael Welling wrote:
> 
> > Do you want to revert the patch and apply a new one or should I provide a
> > patch that reverts the changes and fixes it all in one?
> 
> Can you please send me separate revert and re-add patches, that's
> probably going to be easier to review.

So after reverting this patch I found there is a issue in that it is difficult
to determine when a transfer is complete to properly drive the chipselect from
within the transfer_one function.

Then I figured that we could handle the case when GPIOs are being used with
some conditional calls to omap2_mcspi_set_cs in the omap2_mcspi_work_one
function.

Near the beginning of the function I added:
	if (gpio_is_valid(spi->cs_gpio))
		omap2_mcspi_set_cs(spi, 0);

Near the end of the function I added:
	if (gpio_is_valid(spi->cs_gpio))
		omap2_mcspi_set_cs(spi, 1);

This makes GPIO chip select support work while leaving the native working
as previous.

Is this solution acceptible?

In the process of reviewing the changes I found a few other things that
should be changed as well.

Here you will see a delay that is already handled by the core spi driver:
https://git.kernel.org/cgit/linux/kernel/git/next/linux-next.git/tree/drivers/spi/spi-omap2-mcspi.c#n1166

In the set_cs function the inversion of the enable that occurs in the
core spi_set_cs function based on SPI_CS_HIGH needs to revert as the
controller handles the inversion with OMAP2_MCSPI_CHCONF_EPOL bit in the
CHCONF register.

If you approve of the fix for the GPIO support, I will provide a patch
series with all of these above mentioned fixes.
Mark Brown May 22, 2015, 12:25 p.m. UTC | #10
On Thu, May 21, 2015 at 06:48:33PM -0500, Michael Welling wrote:

> So after reverting this patch I found there is a issue in that it is difficult
> to determine when a transfer is complete to properly drive the chipselect from
> within the transfer_one function.

Is unprepare_message() a suitable place here?  I've got a feeling the
answer is no...

> Then I figured that we could handle the case when GPIOs are being used with
> some conditional calls to omap2_mcspi_set_cs in the omap2_mcspi_work_one
> function.
> 
> Near the beginning of the function I added:
> 	if (gpio_is_valid(spi->cs_gpio))
> 		omap2_mcspi_set_cs(spi, 0);
> 
> Near the end of the function I added:
> 	if (gpio_is_valid(spi->cs_gpio))
> 		omap2_mcspi_set_cs(spi, 1);
> 
> This makes GPIO chip select support work while leaving the native working
> as previous.
> 
> Is this solution acceptible?

I think that's probably OK as well, it's not ideal though (and risky if
the chip select is routed somewhere...).  

> In the process of reviewing the changes I found a few other things that
> should be changed as well.

Please send fixes for these as separate patches (ideally without any
dependency on your new work so we can send them to Linus as fixes).

> Here you will see a delay that is already handled by the core spi driver:
> https://git.kernel.org/cgit/linux/kernel/git/next/linux-next.git/tree/drivers/spi/spi-omap2-mcspi.c#n1166

I can't actually see that since I have no internet access right now!
Michael Welling May 22, 2015, 3:31 p.m. UTC | #11
On Fri, May 22, 2015 at 01:25:44PM +0100, Mark Brown wrote:
> On Thu, May 21, 2015 at 06:48:33PM -0500, Michael Welling wrote:
> 
> > So after reverting this patch I found there is a issue in that it is difficult
> > to determine when a transfer is complete to properly drive the chipselect from
> > within the transfer_one function.
> 
> Is unprepare_message() a suitable place here?  I've got a feeling the
> answer is no...
> 
> > Then I figured that we could handle the case when GPIOs are being used with
> > some conditional calls to omap2_mcspi_set_cs in the omap2_mcspi_work_one
> > function.
> > 
> > Near the beginning of the function I added:
> > 	if (gpio_is_valid(spi->cs_gpio))
> > 		omap2_mcspi_set_cs(spi, 0);
> > 
> > Near the end of the function I added:
> > 	if (gpio_is_valid(spi->cs_gpio))
> > 		omap2_mcspi_set_cs(spi, 1);
> > 
> > This makes GPIO chip select support work while leaving the native working
> > as previous.
> > 
> > Is this solution acceptible?
> 
> I think that's probably OK as well, it's not ideal though (and risky if
> the chip select is routed somewhere...).  
> 
> > In the process of reviewing the changes I found a few other things that
> > should be changed as well.
> 
> Please send fixes for these as separate patches (ideally without any
> dependency on your new work so we can send them to Linus as fixes).
>

Well actually these fixes are needed as the results of the first three patches
pushed into next.

When switching from transfer_one_message to tranfer_one I did not realize that
the delay was handled in the core.

When adding the set_cs function I did not notice that the enable was
complemented by the core driver.

> > Here you will see a delay that is already handled by the core spi driver:
> > https://git.kernel.org/cgit/linux/kernel/git/next/linux-next.git/tree/drivers/spi/spi-omap2-mcspi.c#n1166
> 
> I can't actually see that since I have no internet access right now!

That's like a fish out of water.
Michael Welling May 24, 2015, 2:13 a.m. UTC | #12
The recent update of the OMAP2 McSPI driver left some unresolved issues.
These patches should take care them and again allow for GPIO chip selects
and native chip selects.

Michael Welling (4):
  spi: omap2-mcspi: Remove unnecessary delay
  spi: omap2-mcspi: Fix set_cs function for active high
  spi: omap2-mcspi: Fix GPIO chip select support
  spi: omap2-mcspi: Handle error on gpio_request

 drivers/spi/spi-omap2-mcspi.c |   25 +++++++++++++++++++------
 1 file changed, 19 insertions(+), 6 deletions(-)
Mark Brown May 25, 2015, noon UTC | #13
On Sat, May 23, 2015 at 09:13:41PM -0500, Michael Welling wrote:
> The recent update of the OMAP2 McSPI driver left some unresolved issues.
> These patches should take care them and again allow for GPIO chip selects
> and native chip selects.

Applied all, thanks.
diff mbox

Patch

diff --git a/drivers/spi/spi-omap2-mcspi.c b/drivers/spi/spi-omap2-mcspi.c
index 90cf7e7..a7d85c5 100644
--- a/drivers/spi/spi-omap2-mcspi.c
+++ b/drivers/spi/spi-omap2-mcspi.c
@@ -243,17 +243,20 @@  static void omap2_mcspi_set_enable(const struct spi_device *spi, int enable)
 	mcspi_read_cs_reg(spi, OMAP2_MCSPI_CHCTRL0);
 }
 
-static void omap2_mcspi_force_cs(struct spi_device *spi, int cs_active)
+static void omap2_mcspi_set_cs(struct spi_device *spi, bool enable)
 {
 	u32 l;
 
-	l = mcspi_cached_chconf0(spi);
-	if (cs_active)
-		l |= OMAP2_MCSPI_CHCONF_FORCE;
-	else
-		l &= ~OMAP2_MCSPI_CHCONF_FORCE;
+	if (spi->controller_state) {
+		l = mcspi_cached_chconf0(spi);
 
-	mcspi_write_chconf0(spi, l);
+		if (enable)
+			l &= ~OMAP2_MCSPI_CHCONF_FORCE;
+		else
+			l |= OMAP2_MCSPI_CHCONF_FORCE;
+
+		mcspi_write_chconf0(spi, l);
+	}
 }
 
 static void omap2_mcspi_set_master_mode(struct spi_master *master)
@@ -1075,7 +1078,6 @@  static int omap2_mcspi_work_one(struct omap2_mcspi *mcspi,
 
 	struct spi_master		*master;
 	struct omap2_mcspi_dma		*mcspi_dma;
-	int				cs_active = 0;
 	struct omap2_mcspi_cs		*cs;
 	struct omap2_mcspi_device_config *cd;
 	int				par_override = 0;
@@ -1118,11 +1120,6 @@  static int omap2_mcspi_work_one(struct omap2_mcspi *mcspi,
 			mcspi_read_cs_reg(spi, OMAP2_MCSPI_MODULCTRL);
 	}
 
-	if (!cs_active) {
-		omap2_mcspi_force_cs(spi, 1);
-		cs_active = 1;
-	}
-
 	chconf = mcspi_cached_chconf0(spi);
 	chconf &= ~OMAP2_MCSPI_CHCONF_TRM_MASK;
 	chconf &= ~OMAP2_MCSPI_CHCONF_TURBO;
@@ -1169,12 +1166,6 @@  static int omap2_mcspi_work_one(struct omap2_mcspi *mcspi,
 	if (t->delay_usecs)
 		udelay(t->delay_usecs);
 
-	/* ignore the "leave it on after last xfer" hint */
-	if (t->cs_change) {
-		omap2_mcspi_force_cs(spi, 0);
-		cs_active = 0;
-	}
-
 	omap2_mcspi_set_enable(spi, 0);
 
 	if (mcspi->fifo_depth > 0)
@@ -1187,9 +1178,6 @@  out:
 		status = omap2_mcspi_setup_transfer(spi, NULL);
 	}
 
-	if (cs_active)
-		omap2_mcspi_force_cs(spi, 0);
-
 	if (cd && cd->cs_per_word) {
 		chconf = mcspi->ctx.modulctrl;
 		chconf |= OMAP2_MCSPI_MODULCTRL_SINGLE;
@@ -1334,6 +1322,7 @@  static int omap2_mcspi_probe(struct platform_device *pdev)
 	master->setup = omap2_mcspi_setup;
 	master->auto_runtime_pm = true;
 	master->transfer_one = omap2_mcspi_transfer_one;
+	master->set_cs = omap2_mcspi_set_cs;
 	master->cleanup = omap2_mcspi_cleanup;
 	master->dev.of_node = node;
 	master->max_speed_hz = OMAP2_MCSPI_MAX_FREQ;