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

Groups > linux.kernel > #1559535 > unrolled thread

[PATCH 0/2] spi: pca2xx: Prepare for and enable MSI support

Back to article view | Back to linux.kernel

Contents

  [PATCH 0/2] spi: pca2xx: Prepare for and enable MSI support Jan Kiszka <jan.kiszka@siemens.com> - 2017-01-16 10:10 +0100
    [PATCH 1/2] spi: pxa2xx: Prepare for edge-triggered interrupts Jan Kiszka <jan.kiszka@siemens.com> - 2017-01-16 10:10 +0100
      Re: [PATCH 1/2] spi: pxa2xx: Prepare for edge-triggered interrupts Andy Shevchenko <andriy.shevchenko@linux.intel.com> - 2017-01-16 10:30 +0100
        Re: [PATCH 1/2] spi: pxa2xx: Prepare for edge-triggered interrupts Jan Kiszka <jan.kiszka@siemens.com> - 2017-01-16 12:20 +0100
          Re: [PATCH 1/2] spi: pxa2xx: Prepare for edge-triggered interrupts Andy Shevchenko <andy.shevchenko@gmail.com> - 2017-01-16 19:00 +0100
            Re: [PATCH 1/2] spi: pxa2xx: Prepare for edge-triggered interrupts Jan Kiszka <jan.kiszka@siemens.com> - 2017-01-16 19:30 +0100

#1559535 — [PATCH 0/2] spi: pca2xx: Prepare for and enable MSI support

This enhances the pca2xx driver's interrupt handler with support for
edge-triggered interrupts and makes use of that for PCI-hosted variants,
like the Intel Quark SoC.

Jan

Jan Kiszka (2):
  spi: pxa2xx: Prepare for edge-triggered interrupts
  spi: pca2xx-pci: Allow MSI

 drivers/spi/spi-pxa2xx-pci.c | 14 +++++++++++--
 drivers/spi/spi-pxa2xx.c     | 50 +++++++++++++++++++++++++-------------------
 2 files changed, 40 insertions(+), 24 deletions(-)

-- 
2.1.4

[toc] | [next] | [standalone]

#1559536 — [PATCH 1/2] spi: pxa2xx: Prepare for edge-triggered interrupts

When using the a device with edge-triggered interrupts, such as MSIs,
the interrupt handler has to ensure that there is a point in time during
its execution where all interrupts sources are silent so that a new
event can trigger a new interrupt again.

This is achieved here by looping over SSSR evaluation. We need to take
into account that SSCR1 may be changed by the transfer handler, thus we
need to redo the mask calculation, at least regarding the volatile
interrupt enable bit (TIE).

Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
---
 drivers/spi/spi-pxa2xx.c | 50 +++++++++++++++++++++++++++---------------------
 1 file changed, 28 insertions(+), 22 deletions(-)

diff --git a/drivers/spi/spi-pxa2xx.c b/drivers/spi/spi-pxa2xx.c
index dd7b5b4..24bf549 100644
--- a/drivers/spi/spi-pxa2xx.c
+++ b/drivers/spi/spi-pxa2xx.c
@@ -737,6 +737,7 @@ static irqreturn_t ssp_int(int irq, void *dev_id)
 	struct driver_data *drv_data = dev_id;
 	u32 sccr1_reg;
 	u32 mask = drv_data->mask_sr;
+	irqreturn_t ret = IRQ_NONE;
 	u32 status;
 
 	/*
@@ -760,37 +761,42 @@ static irqreturn_t ssp_int(int irq, void *dev_id)
 
 	sccr1_reg = pxa2xx_spi_read(drv_data, SSCR1);
 
-	/* Ignore possible writes if we don't need to write */
-	if (!(sccr1_reg & SSCR1_TIE))
-		mask &= ~SSSR_TFS;
-
 	/* Ignore RX timeout interrupt if it is disabled */
 	if (!(sccr1_reg & SSCR1_TINTE))
 		mask &= ~SSSR_TINT;
 
-	if (!(status & mask))
-		return IRQ_NONE;
+	while (1) {
+		/* Ignore possible writes if we don't need to write */
+		if (!(sccr1_reg & SSCR1_TIE))
+			mask &= ~SSSR_TFS;
 
-	if (!drv_data->master->cur_msg) {
+		if (!(status & mask))
+			return ret;
 
-		pxa2xx_spi_write(drv_data, SSCR0,
-				 pxa2xx_spi_read(drv_data, SSCR0)
-				 & ~SSCR0_SSE);
-		pxa2xx_spi_write(drv_data, SSCR1,
-				 pxa2xx_spi_read(drv_data, SSCR1)
-				 & ~drv_data->int_cr1);
-		if (!pxa25x_ssp_comp(drv_data))
-			pxa2xx_spi_write(drv_data, SSTO, 0);
-		write_SSSR_CS(drv_data, drv_data->clear_sr);
+		if (!drv_data->master->cur_msg) {
 
-		dev_err(&drv_data->pdev->dev,
-			"bad message state in interrupt handler\n");
+			pxa2xx_spi_write(drv_data, SSCR0,
+					 pxa2xx_spi_read(drv_data, SSCR0)
+					 & ~SSCR0_SSE);
+			pxa2xx_spi_write(drv_data, SSCR1,
+					 pxa2xx_spi_read(drv_data, SSCR1)
+					 & ~drv_data->int_cr1);
+			if (!pxa25x_ssp_comp(drv_data))
+				pxa2xx_spi_write(drv_data, SSTO, 0);
+			write_SSSR_CS(drv_data, drv_data->clear_sr);
 
-		/* Never fail */
-		return IRQ_HANDLED;
-	}
+			dev_err(&drv_data->pdev->dev,
+				"bad message state in interrupt handler\n");
 
-	return drv_data->transfer_handler(drv_data);
+			/* Never fail */
+			return IRQ_HANDLED;
+		}
+
+		ret |= drv_data->transfer_handler(drv_data);
+
+		status = pxa2xx_spi_read(drv_data, SSSR);
+		sccr1_reg = pxa2xx_spi_read(drv_data, SSCR1);
+	}
 }
 
 /*
-- 
2.1.4

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

#1559545 — Re: [PATCH 1/2] spi: pxa2xx: Prepare for edge-triggered interrupts

On Mon, 2017-01-16 at 10:05 +0100, Jan Kiszka wrote:
> When using the a device with edge-triggered interrupts, such as MSIs,
> the interrupt handler has to ensure that there is a point in time
> during
> its execution where all interrupts sources are silent so that a new
> event can trigger a new interrupt again.
> 
> This is achieved here by looping over SSSR evaluation. We need to take
> into account that SSCR1 may be changed by the transfer handler, thus
> we
> need to redo the mask calculation, at least regarding the volatile
> interrupt enable bit (TIE).

Could you split this to two patches, one just move the code under
question to a helper function (no functional change), the other does
what you state in commit message here?

> 
> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
> ---
>  drivers/spi/spi-pxa2xx.c | 50 +++++++++++++++++++++++++++----------
> -----------
>  1 file changed, 28 insertions(+), 22 deletions(-)
> 
> diff --git a/drivers/spi/spi-pxa2xx.c b/drivers/spi/spi-pxa2xx.c
> index dd7b5b4..24bf549 100644
> --- a/drivers/spi/spi-pxa2xx.c
> +++ b/drivers/spi/spi-pxa2xx.c
> @@ -737,6 +737,7 @@ static irqreturn_t ssp_int(int irq, void *dev_id)
>  	struct driver_data *drv_data = dev_id;
>  	u32 sccr1_reg;
>  	u32 mask = drv_data->mask_sr;
> +	irqreturn_t ret = IRQ_NONE;
>  	u32 status;
>  
>  	/*
> @@ -760,37 +761,42 @@ static irqreturn_t ssp_int(int irq, void
> *dev_id)
>  
>  	sccr1_reg = pxa2xx_spi_read(drv_data, SSCR1);
>  
> -	/* Ignore possible writes if we don't need to write */
> -	if (!(sccr1_reg & SSCR1_TIE))
> -		mask &= ~SSSR_TFS;
> -
>  	/* Ignore RX timeout interrupt if it is disabled */
>  	if (!(sccr1_reg & SSCR1_TINTE))
>  		mask &= ~SSSR_TINT;
>  
> -	if (!(status & mask))
> -		return IRQ_NONE;
> +	while (1) {
> +		/* Ignore possible writes if we don't need to write
> */
> +		if (!(sccr1_reg & SSCR1_TIE))
> +			mask &= ~SSSR_TFS;
>  
> -	if (!drv_data->master->cur_msg) {
> +		if (!(status & mask))
> +			return ret;
>  
> -		pxa2xx_spi_write(drv_data, SSCR0,
> -				 pxa2xx_spi_read(drv_data, SSCR0)
> -				 & ~SSCR0_SSE);
> -		pxa2xx_spi_write(drv_data, SSCR1,
> -				 pxa2xx_spi_read(drv_data, SSCR1)
> -				 & ~drv_data->int_cr1);
> -		if (!pxa25x_ssp_comp(drv_data))
> -			pxa2xx_spi_write(drv_data, SSTO, 0);
> -		write_SSSR_CS(drv_data, drv_data->clear_sr);
> +		if (!drv_data->master->cur_msg) {
>  
> -		dev_err(&drv_data->pdev->dev,
> -			"bad message state in interrupt handler\n");
> +			pxa2xx_spi_write(drv_data, SSCR0,
> +					 pxa2xx_spi_read(drv_data,
> SSCR0)
> +					 & ~SSCR0_SSE);
> +			pxa2xx_spi_write(drv_data, SSCR1,
> +					 pxa2xx_spi_read(drv_data,
> SSCR1)
> +					 & ~drv_data->int_cr1);
> +			if (!pxa25x_ssp_comp(drv_data))
> +				pxa2xx_spi_write(drv_data, SSTO, 0);
> +			write_SSSR_CS(drv_data, drv_data->clear_sr);
>  
> -		/* Never fail */
> -		return IRQ_HANDLED;
> -	}
> +			dev_err(&drv_data->pdev->dev,
> +				"bad message state in interrupt
> handler\n");
>  
> -	return drv_data->transfer_handler(drv_data);
> +			/* Never fail */
> +			return IRQ_HANDLED;
> +		}
> +
> +		ret |= drv_data->transfer_handler(drv_data);
> +
> +		status = pxa2xx_spi_read(drv_data, SSSR);
> +		sccr1_reg = pxa2xx_spi_read(drv_data, SSCR1);
> +	}
>  }
>  
>  /*

-- 
Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Intel Finland Oy

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

#1559648 — Re: [PATCH 1/2] spi: pxa2xx: Prepare for edge-triggered interrupts

On 2017-01-16 10:24, Andy Shevchenko wrote:
> On Mon, 2017-01-16 at 10:05 +0100, Jan Kiszka wrote:
>> When using the a device with edge-triggered interrupts, such as MSIs,
>> the interrupt handler has to ensure that there is a point in time
>> during
>> its execution where all interrupts sources are silent so that a new
>> event can trigger a new interrupt again.
>>
>> This is achieved here by looping over SSSR evaluation. We need to take
>> into account that SSCR1 may be changed by the transfer handler, thus
>> we
>> need to redo the mask calculation, at least regarding the volatile
>> interrupt enable bit (TIE).
> 
> Could you split this to two patches, one just move the code under
> question to a helper function (no functional change), the other does
> what you state in commit message here?

IMHO, factoring out some helper called from the loop in ssp_int won't be
a natural split due to the large number of local variables being shared
here. But maybe I'm not seeing the design you have in mind, so please
propose a useful helper function signature.

Thanks,
Jan

> 
>>
>> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
>> ---
>>  drivers/spi/spi-pxa2xx.c | 50 +++++++++++++++++++++++++++----------
>> -----------
>>  1 file changed, 28 insertions(+), 22 deletions(-)
>>
>> diff --git a/drivers/spi/spi-pxa2xx.c b/drivers/spi/spi-pxa2xx.c
>> index dd7b5b4..24bf549 100644
>> --- a/drivers/spi/spi-pxa2xx.c
>> +++ b/drivers/spi/spi-pxa2xx.c
>> @@ -737,6 +737,7 @@ static irqreturn_t ssp_int(int irq, void *dev_id)
>>  	struct driver_data *drv_data = dev_id;
>>  	u32 sccr1_reg;
>>  	u32 mask = drv_data->mask_sr;
>> +	irqreturn_t ret = IRQ_NONE;
>>  	u32 status;
>>  
>>  	/*
>> @@ -760,37 +761,42 @@ static irqreturn_t ssp_int(int irq, void
>> *dev_id)
>>  
>>  	sccr1_reg = pxa2xx_spi_read(drv_data, SSCR1);
>>  
>> -	/* Ignore possible writes if we don't need to write */
>> -	if (!(sccr1_reg & SSCR1_TIE))
>> -		mask &= ~SSSR_TFS;
>> -
>>  	/* Ignore RX timeout interrupt if it is disabled */
>>  	if (!(sccr1_reg & SSCR1_TINTE))
>>  		mask &= ~SSSR_TINT;
>>  
>> -	if (!(status & mask))
>> -		return IRQ_NONE;
>> +	while (1) {
>> +		/* Ignore possible writes if we don't need to write
>> */
>> +		if (!(sccr1_reg & SSCR1_TIE))
>> +			mask &= ~SSSR_TFS;
>>  
>> -	if (!drv_data->master->cur_msg) {
>> +		if (!(status & mask))
>> +			return ret;
>>  
>> -		pxa2xx_spi_write(drv_data, SSCR0,
>> -				 pxa2xx_spi_read(drv_data, SSCR0)
>> -				 & ~SSCR0_SSE);
>> -		pxa2xx_spi_write(drv_data, SSCR1,
>> -				 pxa2xx_spi_read(drv_data, SSCR1)
>> -				 & ~drv_data->int_cr1);
>> -		if (!pxa25x_ssp_comp(drv_data))
>> -			pxa2xx_spi_write(drv_data, SSTO, 0);
>> -		write_SSSR_CS(drv_data, drv_data->clear_sr);
>> +		if (!drv_data->master->cur_msg) {
>>  
>> -		dev_err(&drv_data->pdev->dev,
>> -			"bad message state in interrupt handler\n");
>> +			pxa2xx_spi_write(drv_data, SSCR0,
>> +					 pxa2xx_spi_read(drv_data,
>> SSCR0)
>> +					 & ~SSCR0_SSE);
>> +			pxa2xx_spi_write(drv_data, SSCR1,
>> +					 pxa2xx_spi_read(drv_data,
>> SSCR1)
>> +					 & ~drv_data->int_cr1);
>> +			if (!pxa25x_ssp_comp(drv_data))
>> +				pxa2xx_spi_write(drv_data, SSTO, 0);
>> +			write_SSSR_CS(drv_data, drv_data->clear_sr);
>>  
>> -		/* Never fail */
>> -		return IRQ_HANDLED;
>> -	}
>> +			dev_err(&drv_data->pdev->dev,
>> +				"bad message state in interrupt
>> handler\n");
>>  
>> -	return drv_data->transfer_handler(drv_data);
>> +			/* Never fail */
>> +			return IRQ_HANDLED;
>> +		}
>> +
>> +		ret |= drv_data->transfer_handler(drv_data);
>> +
>> +		status = pxa2xx_spi_read(drv_data, SSSR);
>> +		sccr1_reg = pxa2xx_spi_read(drv_data, SSCR1);
>> +	}
>>  }
>>  
>>  /*
> 

-- 
Siemens AG, Corporate Technology, CT RDA ITP SES-DE
Corporate Competence Center Embedded Linux

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

#1559952 — Re: [PATCH 1/2] spi: pxa2xx: Prepare for edge-triggered interrupts

On Mon, Jan 16, 2017 at 1:18 PM, Jan Kiszka <jan.kiszka@siemens.com> wrote:
> On 2017-01-16 10:24, Andy Shevchenko wrote:
>> On Mon, 2017-01-16 at 10:05 +0100, Jan Kiszka wrote:
>>> When using the a device with edge-triggered interrupts, such as MSIs,
>>> the interrupt handler has to ensure that there is a point in time
>>> during
>>> its execution where all interrupts sources are silent so that a new
>>> event can trigger a new interrupt again.
>>>
>>> This is achieved here by looping over SSSR evaluation. We need to take
>>> into account that SSCR1 may be changed by the transfer handler, thus
>>> we
>>> need to redo the mask calculation, at least regarding the volatile
>>> interrupt enable bit (TIE).
>>
>> Could you split this to two patches, one just move the code under
>> question to a helper function (no functional change), the other does
>> what you state in commit message here?
>
> IMHO, factoring out some helper called from the loop in ssp_int won't be
> a natural split due to the large number of local variables being shared
> here. But maybe I'm not seeing the design you have in mind, so please
> propose a useful helper function signature.

At least everything starting from if (!...) {} can be a helper with
only one parameter. Something like:

static int handle_bad_msg(struct driver_data *drv_data)
{
  if (...)
    return 0;

  ...handle it...
  return 1;
}

Let's start from above.

P.S. Btw, you totally missed SPI list/maintainers. And you are using
wrong Jarkko's address.

-- 
With Best Regards,
Andy Shevchenko

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

#1559963 — Re: [PATCH 1/2] spi: pxa2xx: Prepare for edge-triggered interrupts

On 2017-01-16 18:53, Andy Shevchenko wrote:
> On Mon, Jan 16, 2017 at 1:18 PM, Jan Kiszka <jan.kiszka@siemens.com> wrote:
>> On 2017-01-16 10:24, Andy Shevchenko wrote:
>>> On Mon, 2017-01-16 at 10:05 +0100, Jan Kiszka wrote:
>>>> When using the a device with edge-triggered interrupts, such as MSIs,
>>>> the interrupt handler has to ensure that there is a point in time
>>>> during
>>>> its execution where all interrupts sources are silent so that a new
>>>> event can trigger a new interrupt again.
>>>>
>>>> This is achieved here by looping over SSSR evaluation. We need to take
>>>> into account that SSCR1 may be changed by the transfer handler, thus
>>>> we
>>>> need to redo the mask calculation, at least regarding the volatile
>>>> interrupt enable bit (TIE).
>>>
>>> Could you split this to two patches, one just move the code under
>>> question to a helper function (no functional change), the other does
>>> what you state in commit message here?
>>
>> IMHO, factoring out some helper called from the loop in ssp_int won't be
>> a natural split due to the large number of local variables being shared
>> here. But maybe I'm not seeing the design you have in mind, so please
>> propose a useful helper function signature.
> 
> At least everything starting from if (!...) {} can be a helper with
> only one parameter. Something like:
> 
> static int handle_bad_msg(struct driver_data *drv_data)
> {
>   if (...)
>     return 0;
> 
>   ...handle it...
>   return 1;
> }
> 
> Let's start from above.

OK, but I'll factor out only the handling block, ie. after the if.
That's more consistent.

> 
> P.S. Btw, you totally missed SPI list/maintainers. And you are using
> wrong Jarkko's address.

Data-mined this from the list, both typical target group as well as
Jarkko's address that he tends to use. Will adjust.

Jan

-- 
Siemens AG, Corporate Technology, CT RDA ITP SES-DE
Corporate Competence Center Embedded Linux

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web