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

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

[Jan Kiszka]

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.

Changes in v2:
- factored out handle_bad_msg (Andy)
- reordered code in patch 3, avoiding unneeded pci_free_irq_vectors (Andy)

Jan

Jan Kiszka (3):
  spi: pxa2xx: Factor out handle_bad_msg
  spi: pxa2xx: Prepare for edge-triggered interrupts
  spi: pca2xx-pci: Allow MSI

 drivers/spi/spi-pxa2xx-pci.c |  8 ++++++-
 drivers/spi/spi-pxa2xx.c     | 51 +++++++++++++++++++++++++-------------------
 2 files changed, 36 insertions(+), 23 deletions(-)

-- 
2.1.4

[PATCH v2 1/3] spi: pxa2xx: Factor out handle_bad_msg

[Jan Kiszka]

As suggested by Andy Shevchenko: Decouple this corner cause from the
general handling logic in ssp_int.

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

diff --git a/drivers/spi/spi-pxa2xx.c b/drivers/spi/spi-pxa2xx.c
index dd7b5b4..0d10090 100644
--- a/drivers/spi/spi-pxa2xx.c
+++ b/drivers/spi/spi-pxa2xx.c
@@ -732,6 +732,20 @@ static irqreturn_t interrupt_transfer(struct driver_data *drv_data)
 	return IRQ_HANDLED;
 }
 
+static void handle_bad_msg(struct driver_data *drv_data)
+{
+	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);
+
+	dev_err(&drv_data->pdev->dev,
+		"bad message state in interrupt handler\n");
+}
+
 static irqreturn_t ssp_int(int irq, void *dev_id)
 {
 	struct driver_data *drv_data = dev_id;
@@ -772,20 +786,7 @@ static irqreturn_t ssp_int(int irq, void *dev_id)
 		return IRQ_NONE;
 
 	if (!drv_data->master->cur_msg) {
-
-		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);
-
-		dev_err(&drv_data->pdev->dev,
-			"bad message state in interrupt handler\n");
-
+		handle_bad_msg(drv_data);
 		/* Never fail */
 		return IRQ_HANDLED;
 	}
-- 
2.1.4

Re: [PATCH v2 1/3] spi: pxa2xx: Factor out handle_bad_msg

[Jarkko Nikula]

On 01/16/2017 08:44 PM, Jan Kiszka wrote:
> As suggested by Andy Shevchenko: Decouple this corner cause from the
> general handling logic in ssp_int.
>
> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
> ---
>  drivers/spi/spi-pxa2xx.c | 29 +++++++++++++++--------------
>  1 file changed, 15 insertions(+), 14 deletions(-)
>
Reviewed-by: Jarkko Nikula <jarkko.nikula@linux.intel.com>

Applied "spi: pxa2xx: Factor out handle_bad_msg" to the spi tree

[Mark Brown]

The patch

   spi: pxa2xx: Factor out handle_bad_msg

has been applied to the spi tree at

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

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

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

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

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

Thanks,
Mark

>From b03124825b8612bf371e5b4ccc2cd812ed3c2dbb Mon Sep 17 00:00:00 2001
From: Jan Kiszka <jan.kiszka@siemens.com>
Date: Mon, 16 Jan 2017 19:44:54 +0100
Subject: [PATCH] spi: pxa2xx: Factor out handle_bad_msg

As suggested by Andy Shevchenko: Decouple this corner cause from the
general handling logic in ssp_int.

Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
Reviewed-by: Jarkko Nikula <jarkko.nikula@linux.intel.com>
Signed-off-by: Mark Brown <broonie@kernel.org>
---
 drivers/spi/spi-pxa2xx.c | 29 +++++++++++++++--------------
 1 file changed, 15 insertions(+), 14 deletions(-)

diff --git a/drivers/spi/spi-pxa2xx.c b/drivers/spi/spi-pxa2xx.c
index d6239fa718be..8c65bc1823f3 100644
--- a/drivers/spi/spi-pxa2xx.c
+++ b/drivers/spi/spi-pxa2xx.c
@@ -732,6 +732,20 @@ static irqreturn_t interrupt_transfer(struct driver_data *drv_data)
 	return IRQ_HANDLED;
 }
 
+static void handle_bad_msg(struct driver_data *drv_data)
+{
+	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);
+
+	dev_err(&drv_data->pdev->dev,
+		"bad message state in interrupt handler\n");
+}
+
 static irqreturn_t ssp_int(int irq, void *dev_id)
 {
 	struct driver_data *drv_data = dev_id;
@@ -772,20 +786,7 @@ static irqreturn_t ssp_int(int irq, void *dev_id)
 		return IRQ_NONE;
 
 	if (!drv_data->master->cur_msg) {
-
-		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);
-
-		dev_err(&drv_data->pdev->dev,
-			"bad message state in interrupt handler\n");
-
+		handle_bad_msg(drv_data);
 		/* Never fail */
 		return IRQ_HANDLED;
 	}
-- 
2.11.0

[PATCH v2 2/3] spi: pxa2xx: Prepare for edge-triggered interrupts

[Jan Kiszka]

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 | 30 ++++++++++++++++++------------
 1 file changed, 18 insertions(+), 12 deletions(-)

diff --git a/drivers/spi/spi-pxa2xx.c b/drivers/spi/spi-pxa2xx.c
index 0d10090..ac49b80 100644
--- a/drivers/spi/spi-pxa2xx.c
+++ b/drivers/spi/spi-pxa2xx.c
@@ -751,6 +751,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;
 
 	/*
@@ -774,24 +775,29 @@ 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) {
-		handle_bad_msg(drv_data);
-		/* Never fail */
-		return IRQ_HANDLED;
-	}
+		if (!(status & mask))
+			return ret;
+
+		if (!drv_data->master->cur_msg) {
+			handle_bad_msg(drv_data);
+			/* Never fail */
+			return IRQ_HANDLED;
+		}
+
+		ret |= drv_data->transfer_handler(drv_data);
 
-	return drv_data->transfer_handler(drv_data);
+		status = pxa2xx_spi_read(drv_data, SSSR);
+		sccr1_reg = pxa2xx_spi_read(drv_data, SSCR1);
+	}
 }
 
 /*
-- 
2.1.4

Re: [PATCH v2 2/3] spi: pxa2xx: Prepare for edge-triggered interrupts

[Andy Shevchenko]

On Mon, 2017-01-16 at 19:44 +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).
> 

So, more comments/questions below.

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

Can we switch to do-while and move previous block here? Btw, can TINTE
bit be set again during a loop?

> +		/* Ignore possible writes if we don't need to write
> */
> +		if (!(sccr1_reg & SSCR1_TIE))
> +			mask &= ~SSSR_TFS;
>  
> -	if (!drv_data->master->cur_msg) {
> -		handle_bad_msg(drv_data);
> -		/* Never fail */
> -		return IRQ_HANDLED;
> -	}
> +		if (!(status & mask))
> +			return ret;
> +
> +		if (!drv_data->master->cur_msg) {
> +			handle_bad_msg(drv_data);
> +			/* Never fail */
> +			return IRQ_HANDLED;
> +		}
> +

> +		ret |= drv_data->transfer_handler(drv_data);

So, we might call handler several times. This needs to be commented in
the code why you do so.

>  
> -	return drv_data->transfer_handler(drv_data);
> +		status = pxa2xx_spi_read(drv_data, SSSR);

Would it be possible to get all 1:s from the register
(something/autosuspend just powered off it by timeout?) ?

> +		sccr1_reg = pxa2xx_spi_read(drv_data, SSCR1);
> +	}
>  }
>  
>  /*

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

Re: [PATCH v2 2/3] spi: pxa2xx: Prepare for edge-triggered interrupts

[Jan Kiszka]

On 2017-01-16 20:07, Andy Shevchenko wrote:
> On Mon, 2017-01-16 at 19:44 +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).
>>
> 
> So, more comments/questions below.
> 
>>  
>>  	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) {
> 
> Can we switch to do-while and move previous block here?

Don't see how this would help (without duplicating more code).

> Btw, can TINTE
> bit be set again during a loop?

Nope, it's statically set, at least so far.

What we could do is simply restarting ssp_int

> 
>> +		/* Ignore possible writes if we don't need to write
>> */
>> +		if (!(sccr1_reg & SSCR1_TIE))
>> +			mask &= ~SSSR_TFS;
>>  
>> -	if (!drv_data->master->cur_msg) {
>> -		handle_bad_msg(drv_data);
>> -		/* Never fail */
>> -		return IRQ_HANDLED;
>> -	}
>> +		if (!(status & mask))
>> +			return ret;
>> +
>> +		if (!drv_data->master->cur_msg) {
>> +			handle_bad_msg(drv_data);
>> +			/* Never fail */
>> +			return IRQ_HANDLED;
>> +		}
>> +
> 
>> +		ret |= drv_data->transfer_handler(drv_data);
> 
> So, we might call handler several times. This needs to be commented in
> the code why you do so.

I can move the commit log into the code.

> 
>>  
>> -	return drv_data->transfer_handler(drv_data);
>> +		status = pxa2xx_spi_read(drv_data, SSSR);
> 
> Would it be possible to get all 1:s from the register
> (something/autosuspend just powered off it by timeout?) ?
> 

Not sure if that can happen, but I guess it would be simpler and more
readable to simply do this instead:

	while (1) {
		/*
		 * If the device is not yet in RPM suspended state and we get an
		 * interrupt that is meant for another device, check if status
		 * bits are all set to one. That means that the device is
		 * already powered off.
		 */
		status = pxa2xx_spi_read(drv_data, SSSR);
		if (status == ~0)
			return ret;

		sccr1_reg = pxa2xx_spi_read(drv_data, SSCR1);

		/* Ignore RX timeout interrupt if it is disabled */
		if (!(sccr1_reg & SSCR1_TINTE))
			mask &= ~SSSR_TINT;

		/* Ignore possible writes if we don't need to write */
		if (!(sccr1_reg & SSCR1_TIE))
			mask &= ~SSSR_TFS;

		if (!(status & mask))
			return ret;

		if (!drv_data->master->cur_msg) {
			handle_bad_msg(drv_data);
			/* Never fail */
			return IRQ_HANDLED;
		}

		ret |= drv_data->transfer_handler(drv_data);
	}


i.e. preserve the current structure, just add the loop.

Jan

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

Re: [PATCH v2 2/3] spi: pxa2xx: Prepare for edge-triggered interrupts

[Jan Kiszka]

On 2017-01-16 20:46, Jan Kiszka wrote:
> On 2017-01-16 20:07, Andy Shevchenko wrote:
>> On Mon, 2017-01-16 at 19:44 +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).
>>>
>>
>> So, more comments/questions below.
>>
>>>  
>>>  	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) {
>>
>> Can we switch to do-while and move previous block here?
> 
> Don't see how this would help (without duplicating more code).
> 
>> Btw, can TINTE
>> bit be set again during a loop?
> 
> Nope, it's statically set, at least so far.
> 
> What we could do is simply restarting ssp_int
> 
>>
>>> +		/* Ignore possible writes if we don't need to write
>>> */
>>> +		if (!(sccr1_reg & SSCR1_TIE))
>>> +			mask &= ~SSSR_TFS;
>>>  
>>> -	if (!drv_data->master->cur_msg) {
>>> -		handle_bad_msg(drv_data);
>>> -		/* Never fail */
>>> -		return IRQ_HANDLED;
>>> -	}
>>> +		if (!(status & mask))
>>> +			return ret;
>>> +
>>> +		if (!drv_data->master->cur_msg) {
>>> +			handle_bad_msg(drv_data);
>>> +			/* Never fail */
>>> +			return IRQ_HANDLED;
>>> +		}
>>> +
>>
>>> +		ret |= drv_data->transfer_handler(drv_data);
>>
>> So, we might call handler several times. This needs to be commented in
>> the code why you do so.
> 
> I can move the commit log into the code.
> 
>>
>>>  
>>> -	return drv_data->transfer_handler(drv_data);
>>> +		status = pxa2xx_spi_read(drv_data, SSSR);
>>
>> Would it be possible to get all 1:s from the register
>> (something/autosuspend just powered off it by timeout?) ?
>>
> 
> Not sure if that can happen, but I guess it would be simpler and more
> readable to simply do this instead:
> 
> 	while (1) {
> 		/*
> 		 * If the device is not yet in RPM suspended state and we get an
> 		 * interrupt that is meant for another device, check if status
> 		 * bits are all set to one. That means that the device is
> 		 * already powered off.
> 		 */
> 		status = pxa2xx_spi_read(drv_data, SSSR);
> 		if (status == ~0)
> 			return ret;
> 
> 		sccr1_reg = pxa2xx_spi_read(drv_data, SSCR1);
> 
> 		/* Ignore RX timeout interrupt if it is disabled */
> 		if (!(sccr1_reg & SSCR1_TINTE))
> 			mask &= ~SSSR_TINT;
> 
> 		/* Ignore possible writes if we don't need to write */
> 		if (!(sccr1_reg & SSCR1_TIE))
> 			mask &= ~SSSR_TFS;
> 
> 		if (!(status & mask))
> 			return ret;
> 
> 		if (!drv_data->master->cur_msg) {
> 			handle_bad_msg(drv_data);
> 			/* Never fail */
> 			return IRQ_HANDLED;
> 		}
> 
> 		ret |= drv_data->transfer_handler(drv_data);
> 	}
> 
> 
> i.e. preserve the current structure, just add the loop.
> 

OK, please let me know if you want a v3 of this patch with the structure
above. If there are further concerns/questions, just let me know as
well, but I'd like to close this topic if possible.

Thanks,
Jan

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

Re: [PATCH v2 2/3] spi: pxa2xx: Prepare for edge-triggered interrupts

[Robert Jarzmik]

Jan Kiszka <jan.kiszka@siemens.com> writes:

> 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>
Hi Jan,

> +	while (1) {
This bit worries me a bit, as this can be either :
 - hogging the SoC's CPU, endlessly running
 - or even worse, blocking the CPU for ever

The question behind is, should this be done in a top-half, or moved to a irq
thread ?

> +		/* Ignore possible writes if we don't need to write */
> +		if (!(sccr1_reg & SSCR1_TIE))
> +			mask &= ~SSSR_TFS;
>  
> -	if (!drv_data->master->cur_msg) {
> -		handle_bad_msg(drv_data);
> -		/* Never fail */
> -		return IRQ_HANDLED;
> -	}
> +		if (!(status & mask))
> +			return ret;
> +
> +		if (!drv_data->master->cur_msg) {
> +			handle_bad_msg(drv_data);
> +			/* Never fail */
> +			return IRQ_HANDLED;
> +		}
> +
> +		ret |= drv_data->transfer_handler(drv_data);
Mmm that looks weird to me, oring a irqreturn.

Imagine that on first iteration the handler returns IRQ_NONE, and on second
IRQ_HANDLED. This makes ret IRQ_HANDLED. Yet after the first iteration the
handler should have exited, especially if the interrupt is shared with another
driver.

Another thing which is along what Andy already said : it would be better
practice to have this loop in the form :
do {
...
} while (exit_condition_not_met);

Just for maintainability, it's better, and it concentrates the test on the
"exit_condition_not_met" in one place, which will enable us to review better the
algorithm.

Cheers.

-- 
Robert

Re: [PATCH v2 2/3] spi: pxa2xx: Prepare for edge-triggered interrupts

[Jan Kiszka]

On 2017-01-17 08:54, Robert Jarzmik wrote:
> Jan Kiszka <jan.kiszka@siemens.com> writes:
> 
>> 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>
> Hi Jan,
> 
>> +	while (1) {
> This bit worries me a bit, as this can be either :
>  - hogging the SoC's CPU, endlessly running
>  - or even worse, blocking the CPU for ever
> 
> The question behind is, should this be done in a top-half, or moved to a irq
> thread ?

Every device with a broken interrupt source can hog CPUs, nothing
special with this one. If you don't close the loop in the handler
itself, you close it over the hardware retriggering the interrupt over
and over again.

So, I don't see a point in offloading to a thread. The normal case is
some TX done (FIFO available) event followed by an RX event, then the
transfer is complete, isn't it?

> 
>> +		/* Ignore possible writes if we don't need to write */
>> +		if (!(sccr1_reg & SSCR1_TIE))
>> +			mask &= ~SSSR_TFS;
>>  
>> -	if (!drv_data->master->cur_msg) {
>> -		handle_bad_msg(drv_data);
>> -		/* Never fail */
>> -		return IRQ_HANDLED;
>> -	}
>> +		if (!(status & mask))
>> +			return ret;
>> +
>> +		if (!drv_data->master->cur_msg) {
>> +			handle_bad_msg(drv_data);
>> +			/* Never fail */
>> +			return IRQ_HANDLED;
>> +		}
>> +
>> +		ret |= drv_data->transfer_handler(drv_data);
> Mmm that looks weird to me, oring a irqreturn.

Not really an uncommon pattern, though.

> 
> Imagine that on first iteration the handler returns IRQ_NONE, and on second
> IRQ_HANDLED. This makes ret IRQ_HANDLED. Yet after the first iteration the
> handler should have exited, especially if the interrupt is shared with another
> driver.

That would be a bug in transfer_handler, because we don't enter it
without a reason (status != 0).

> 
> Another thing which is along what Andy already said : it would be better
> practice to have this loop in the form :
> do {
> ...
> } while (exit_condition_not_met);

This implies code duplication in order to calculate the condition
(mask...). I can do this if desired, I wouldn't do this to my own code,
though.

Jan

> 
> Just for maintainability, it's better, and it concentrates the test on the
> "exit_condition_not_met" in one place, which will enable us to review better the
> algorithm.
> 
> Cheers.
> 


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

Re: [PATCH v2 2/3] spi: pxa2xx: Prepare for edge-triggered interrupts

[Robert Jarzmik]

Jan Kiszka <jan.kiszka@siemens.com> writes:

> On 2017-01-17 08:54, Robert Jarzmik wrote:
>> Jan Kiszka <jan.kiszka@siemens.com> writes:
>> 
>>> 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>
>> Hi Jan,
>> 
>>> +	while (1) {
>> This bit worries me a bit, as this can be either :
>>  - hogging the SoC's CPU, endlessly running
>>  - or even worse, blocking the CPU for ever
>> 
>> The question behind is, should this be done in a top-half, or moved to a irq
>> thread ?
>
> Every device with a broken interrupt source can hog CPUs, nothing
> special with this one. If you don't close the loop in the handler
> itself, you close it over the hardware retriggering the interrupt over
> and over again.
I'm not speaking of a broken interrupt source, I'm speaking of a broken code,
such as in the handler, or broken status readback, or lack of understanding on
the status register which may imply the while(1) to loop forever.

> So, I don't see a point in offloading to a thread. The normal case is
> some TX done (FIFO available) event followed by an RX event, then the
> transfer is complete, isn't it?
The point is if you stay forever in the while(1) loop, you can at least have a
print a backtrace (LOCKUP_DETECTOR).

>> Imagine that on first iteration the handler returns IRQ_NONE, and on second
>> IRQ_HANDLED. This makes ret IRQ_HANDLED. Yet after the first iteration the
>> handler should have exited, especially if the interrupt is shared with another
>> driver.
>
> That would be a bug in transfer_handler, because we don't enter it
> without a reason (status != 0).
Sure, but can you be sure that all the people modifying the code after you will
see that also ? The other way will _force_ them to see it.

>> Another thing which is along what Andy already said : it would be better
>> practice to have this loop in the form :
>> do {
>> ...
>> } while (exit_condition_not_met);
>
> This implies code duplication in order to calculate the condition
> (mask...). I can do this if desired, I wouldn't do this to my own code,
> though.
Okay, that's acceptable.
Why not have something like this :

sccr1_reg = pxa2xx_spi_read(drv_data, SSCR1);
if (!(sccr1_reg & SSCR1_TIE))
	mask &= ~SSSR_TFS;

/* Ignore RX timeout interrupt if it is disabled */
if (!(sccr1_reg & SSCR1_TINTE))
	mask &= ~SSSR_TINT;

status = pxa2xx_spi_read(drv_data, SSR);
while (status & mask) {
   ... handlers etc ...
   status = pxa2xx_spi_read(drv_data, SSR);
};

There is a duplication of the status read, but that looks acceptable, and the
mask calculation is moved out of the loop (this should be checked more
thoroughly as it looked to me only probe() would change these values, yet I
might be wrong).

Cheers.

-- 
Robert

Re: [PATCH v2 2/3] spi: pxa2xx: Prepare for edge-triggered interrupts

[Jan Kiszka]

On 2017-01-18 09:21, Robert Jarzmik wrote:
> Jan Kiszka <jan.kiszka@siemens.com> writes:
> 
>> On 2017-01-17 08:54, Robert Jarzmik wrote:
>>> Jan Kiszka <jan.kiszka@siemens.com> writes:
>>>
>>>> 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>
>>> Hi Jan,
>>>
>>>> +	while (1) {
>>> This bit worries me a bit, as this can be either :
>>>  - hogging the SoC's CPU, endlessly running
>>>  - or even worse, blocking the CPU for ever
>>>
>>> The question behind is, should this be done in a top-half, or moved to a irq
>>> thread ?
>>
>> Every device with a broken interrupt source can hog CPUs, nothing
>> special with this one. If you don't close the loop in the handler
>> itself, you close it over the hardware retriggering the interrupt over
>> and over again.
> I'm not speaking of a broken interrupt source, I'm speaking of a broken code,
> such as in the handler, or broken status readback, or lack of understanding on
> the status register which may imply the while(1) to loop forever.
> 
>> So, I don't see a point in offloading to a thread. The normal case is
>> some TX done (FIFO available) event followed by an RX event, then the
>> transfer is complete, isn't it?
> The point is if you stay forever in the while(1) loop, you can at least have a
> print a backtrace (LOCKUP_DETECTOR).

I won't consider "debugability" as a good reason to move interrupt
handlers into threads. There should be real workload that requires
offloading or specific prioritization.

> 
>>> Imagine that on first iteration the handler returns IRQ_NONE, and on second
>>> IRQ_HANDLED. This makes ret IRQ_HANDLED. Yet after the first iteration the
>>> handler should have exited, especially if the interrupt is shared with another
>>> driver.
>>
>> That would be a bug in transfer_handler, because we don't enter it
>> without a reason (status != 0).
> Sure, but can you be sure that all the people modifying the code after you will
> see that also ? The other way will _force_ them to see it.
> 
>>> Another thing which is along what Andy already said : it would be better
>>> practice to have this loop in the form :
>>> do {
>>> ...
>>> } while (exit_condition_not_met);
>>
>> This implies code duplication in order to calculate the condition
>> (mask...). I can do this if desired, I wouldn't do this to my own code,
>> though.
> Okay, that's acceptable.
> Why not have something like this :
> 
> sccr1_reg = pxa2xx_spi_read(drv_data, SSCR1);
> if (!(sccr1_reg & SSCR1_TIE))
> 	mask &= ~SSSR_TFS;
> 
> /* Ignore RX timeout interrupt if it is disabled */
> if (!(sccr1_reg & SSCR1_TINTE))
> 	mask &= ~SSSR_TINT;
> 
> status = pxa2xx_spi_read(drv_data, SSR);
> while (status & mask) {
>    ... handlers etc ...
>    status = pxa2xx_spi_read(drv_data, SSR);
> };
> 
> There is a duplication of the status read, but that looks acceptable, and the
> mask calculation is moved out of the loop (this should be checked more
> thoroughly as it looked to me only probe() would change these values, yet I
> might be wrong).

Unfortunately, mask can change if SSCR1_TIE is cleared. So this is not
correct.

What would be an alternative to looping is masking (would be required
for threaded irq anyway - but then we won't need to loop in the first
place): disable all irq sources, check the status bits once, re-enable
according to a potentially updated set, leave the handler and let the
hardware call us again.

Jan

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

Re: [PATCH v2 2/3] spi: pxa2xx: Prepare for edge-triggered interrupts

[Mark Brown]

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

On Wed, Jan 18, 2017 at 10:33:07AM +0100, Jan Kiszka wrote:
> On 2017-01-18 09:21, Robert Jarzmik wrote:

> >>>> +	while (1) {

> >>> This bit worries me a bit, as this can be either :
> >>>  - hogging the SoC's CPU, endlessly running
> >>>  - or even worse, blocking the CPU for ever

> >>> The question behind is, should this be done in a top-half, or moved to a irq
> >>> thread ?

> >> Every device with a broken interrupt source can hog CPUs, nothing
> >> special with this one. If you don't close the loop in the handler
> >> itself, you close it over the hardware retriggering the interrupt over
> >> and over again.

> > I'm not speaking of a broken interrupt source, I'm speaking of a broken code,
> > such as in the handler, or broken status readback, or lack of understanding on
> > the status register which may imply the while(1) to loop forever.

> >> So, I don't see a point in offloading to a thread. The normal case is
> >> some TX done (FIFO available) event followed by an RX event, then the
> >> transfer is complete, isn't it?

> > The point is if you stay forever in the while(1) loop, you can at least have a
> > print a backtrace (LOCKUP_DETECTOR).

> I won't consider "debugability" as a good reason to move interrupt
> handlers into threads. There should be real workload that requires
> offloading or specific prioritization.

It's failure mitigation - you're translating a hard lockup into
something that will potentially allow the system to soldier on which is
likely to be less severe for the user as well as making things easier to
figure out.  If we're doing something like this I'd at least have a
limit on how long we allow the interrupt to scream.

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

Re: [PATCH v2 2/3] spi: pxa2xx: Prepare for edge-triggered interrupts

[Jan Kiszka]

[-- Attachment #1.1: Type: text/plain, Size: 3180 bytes --]

On 2017-01-18 13:46, Mark Brown wrote:
> On Wed, Jan 18, 2017 at 10:33:07AM +0100, Jan Kiszka wrote:
>> On 2017-01-18 09:21, Robert Jarzmik wrote:
> 
>>>>>> +	while (1) {
> 
>>>>> This bit worries me a bit, as this can be either :
>>>>>  - hogging the SoC's CPU, endlessly running
>>>>>  - or even worse, blocking the CPU for ever
> 
>>>>> The question behind is, should this be done in a top-half, or moved to a irq
>>>>> thread ?
> 
>>>> Every device with a broken interrupt source can hog CPUs, nothing
>>>> special with this one. If you don't close the loop in the handler
>>>> itself, you close it over the hardware retriggering the interrupt over
>>>> and over again.
> 
>>> I'm not speaking of a broken interrupt source, I'm speaking of a broken code,
>>> such as in the handler, or broken status readback, or lack of understanding on
>>> the status register which may imply the while(1) to loop forever.
> 
>>>> So, I don't see a point in offloading to a thread. The normal case is
>>>> some TX done (FIFO available) event followed by an RX event, then the
>>>> transfer is complete, isn't it?
> 
>>> The point is if you stay forever in the while(1) loop, you can at least have a
>>> print a backtrace (LOCKUP_DETECTOR).
> 
>> I won't consider "debugability" as a good reason to move interrupt
>> handlers into threads. There should be real workload that requires
>> offloading or specific prioritization.
> 
> It's failure mitigation - you're translating a hard lockup into
> something that will potentially allow the system to soldier on which is
> likely to be less severe for the user as well as making things easier to
> figure out.  If we're doing something like this I'd at least have a
> limit on how long we allow the interrupt to scream.
> 

OK, OK, if that is the biggest worry, I can change the pattern from
loop-based to SCCR1-based, i.e. mask all interrupt sources once per
interrupt so that we enforce a falling edge. Fine.

But now I'm looking at the driver, wondering who all is fiddling under
which conditions with SCCR1. There are a lot of RMW patterns, but I do
not see the locking pattern behind that. Are all RMW accesses run only
in the interrupt handler context? Unlikely, at least with the dmaengine
in the loop.

Closing my eyes regarding this potential issue for now, the patch could
become as simple as

diff --git a/drivers/spi/spi-pxa2xx.c b/drivers/spi/spi-pxa2xx.c
index 0d10090..f9c2329 100644
--- a/drivers/spi/spi-pxa2xx.c
+++ b/drivers/spi/spi-pxa2xx.c
@@ -785,6 +785,9 @@ static irqreturn_t ssp_int(int irq, void *dev_id)
 	if (!(status & mask))
 		return IRQ_NONE;
 
+	pxa2xx_spi_write(drv_data, SSCR1, sccr1_reg & ~drv_data->int_cr1);
+	pxa2xx_spi_write(drv_data, SSCR1, sccr1_reg);
+
 	if (!drv_data->master->cur_msg) {
 		handle_bad_msg(drv_data);
 		/* Never fail */

Not efficient /wrt register accesses, but that's apparently not yet
a design goal anyway (I stumbled over the SSCR1 locking while
considering to introduce a cache for that reg).

Jan

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


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

[PATCH v3 2/3] spi: pxa2xx: Prepare for edge-triggered interrupts

[Jan Kiszka]

[-- Attachment #1.1: Type: text/plain, Size: 1283 bytes --]

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 disabling all interrupt sources for a moment
before processing them according to the status register. If a new
interrupt should have arrived after we read the status, it will now
re-trigger the interrupt, even in edge mode.

Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
---

Successfully tested by now.

Changes to v2:
- avoid inner looping by making the hardware retrigger on "forgotten"
  IRQ sources

 drivers/spi/spi-pxa2xx.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/spi/spi-pxa2xx.c b/drivers/spi/spi-pxa2xx.c
index 0d10090..f9c2329 100644
--- a/drivers/spi/spi-pxa2xx.c
+++ b/drivers/spi/spi-pxa2xx.c
@@ -785,6 +785,9 @@ static irqreturn_t ssp_int(int irq, void *dev_id)
 	if (!(status & mask))
 		return IRQ_NONE;
 
+	pxa2xx_spi_write(drv_data, SSCR1, sccr1_reg & ~drv_data->int_cr1);
+	pxa2xx_spi_write(drv_data, SSCR1, sccr1_reg);
+
 	if (!drv_data->master->cur_msg) {
 		handle_bad_msg(drv_data);
 		/* Never fail */
-- 
2.1.4


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

Re: [PATCH v3 2/3] spi: pxa2xx: Prepare for edge-triggered interrupts

[Mark Brown]

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

On Thu, Jan 19, 2017 at 08:37:40PM +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

I'm missing patches 1 and 3, what's going on here?

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

Re: [PATCH v3 2/3] spi: pxa2xx: Prepare for edge-triggered interrupts

[Andy Shevchenko]

On Thu, 2017-01-19 at 19:57 +0000, Mark Brown wrote:
> On Thu, Jan 19, 2017 at 08:37:40PM +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
> 
> I'm missing patches 1 and 3, what's going on here?

Patch 1 had been applied by you. I think Jan just needs to resend with
proper version and set of patches.

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

Re: [PATCH v3 2/3] spi: pxa2xx: Prepare for edge-triggered interrupts

[Mark Brown]

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

On Thu, Jan 19, 2017 at 10:04:32PM +0200, Andy Shevchenko wrote:
> On Thu, 2017-01-19 at 19:57 +0000, Mark Brown wrote:

> > I'm missing patches 1 and 3, what's going on here?

> Patch 1 had been applied by you. I think Jan just needs to resend with
> proper version and set of patches.

OK - Jan, the purpose of numbering patches in a series is so we can tell
what order they come in.  That's it.  If the set of patches gets changed
due to things being added or removed the numbers will be different but
that's totally OK as their only relevance is in ordering things within a
given posting.

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

Re: [PATCH v3 2/3] spi: pxa2xx: Prepare for edge-triggered interrupts

[Jan Kiszka]

[-- Attachment #1.1: Type: text/plain, Size: 946 bytes --]

On 2017-01-20 13:21, Mark Brown wrote:
> On Thu, Jan 19, 2017 at 10:04:32PM +0200, Andy Shevchenko wrote:
>> On Thu, 2017-01-19 at 19:57 +0000, Mark Brown wrote:
> 
>>> I'm missing patches 1 and 3, what's going on here?
> 
>> Patch 1 had been applied by you. I think Jan just needs to resend with
>> proper version and set of patches.
> 
> OK - Jan, the purpose of numbering patches in a series is so we can tell
> what order they come in.  That's it.  If the set of patches gets changed
> due to things being added or removed the numbers will be different but
> that's totally OK as their only relevance is in ordering things within a
> given posting.

Sorry if the numbering was confusing for you, it was, of course, a
selective update. Just tell me, if you need a new round with of patches
2 and 3 (as 1 and 2).

Jan

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


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

Re: [PATCH v3 2/3] spi: pxa2xx: Prepare for edge-triggered interrupts

[Mark Brown]

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

On Fri, Jan 20, 2017 at 04:29:28PM +0100, Jan Kiszka wrote:

> Sorry if the numbering was confusing for you, it was, of course, a
> selective update. Just tell me, if you need a new round with of patches
> 2 and 3 (as 1 and 2).

Please resend.

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

Re: [PATCH v3 2/3] spi: pxa2xx: Prepare for edge-triggered interrupts

[Robert Jarzmik]

Jan Kiszka <jan.kiszka@siemens.com> writes:

> 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 disabling all interrupt sources for a moment
> before processing them according to the status register. If a new
> interrupt should have arrived after we read the status, it will now
> re-trigger the interrupt, even in edge mode.
>
> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
Acked-by: Robert Jarzmik <robert.jarzmik@free.fr>

Cheers.

--
Robert
 

Applied "spi: pxa2xx: Prepare for edge-triggered interrupts" to the spi tree

[Mark Brown]

The patch

   spi: pxa2xx: Prepare for edge-triggered interrupts

has been applied to the spi tree at

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

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

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

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

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

Thanks,
Mark

>From e51e9b93049f624c179bab2c651995bca22b5bb7 Mon Sep 17 00:00:00 2001
From: Jan Kiszka <jan.kiszka@siemens.com>
Date: Sat, 21 Jan 2017 10:06:38 +0100
Subject: [PATCH] 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 disabling all interrupt sources for a moment
before processing them according to the status register. If a new
interrupt should have arrived after we read the status, it will now
re-trigger the interrupt, even in edge mode.

Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
Acked-by: Robert Jarzmik <robert.jarzmik@free.fr>
Signed-off-by: Mark Brown <broonie@kernel.org>
---
 drivers/spi/spi-pxa2xx.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/spi/spi-pxa2xx.c b/drivers/spi/spi-pxa2xx.c
index 8c65bc1823f3..06ade434c083 100644
--- a/drivers/spi/spi-pxa2xx.c
+++ b/drivers/spi/spi-pxa2xx.c
@@ -785,6 +785,9 @@ static irqreturn_t ssp_int(int irq, void *dev_id)
 	if (!(status & mask))
 		return IRQ_NONE;
 
+	pxa2xx_spi_write(drv_data, SSCR1, sccr1_reg & ~drv_data->int_cr1);
+	pxa2xx_spi_write(drv_data, SSCR1, sccr1_reg);
+
 	if (!drv_data->master->cur_msg) {
 		handle_bad_msg(drv_data);
 		/* Never fail */
-- 
2.11.0

Re: [PATCH v2 2/3] spi: pxa2xx: Prepare for edge-triggered interrupts

[Robert Jarzmik]

Jan Kiszka <jan.kiszka@siemens.com> writes:

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

I'd like moreover to add a question here.

In pxa architecture, SPI interrupts are already edge-triggered, and it's working
well. The interrupt source disabling is not disabled, but the interrupt
controller doesn't trigger an interrupt anymore (as it is masked), yet it marks
it as pending if an interrupt arrives while the interrupt handler is running.

All of this is handled by the interrupt core. My question is why for Intel MSI's
is it necessary to make a change in the driver instead or relying on the
interrupt core as for the pxa ?

Cheers.

--
Robert

Re: [PATCH v2 2/3] spi: pxa2xx: Prepare for edge-triggered interrupts

[Jan Kiszka]

On 2017-01-17 08:58, Robert Jarzmik wrote:
> Jan Kiszka <jan.kiszka@siemens.com> writes:
> 
>> 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).
> 
> I'd like moreover to add a question here.
> 
> In pxa architecture, SPI interrupts are already edge-triggered, and it's working
> well. The interrupt source disabling is not disabled, but the interrupt
> controller doesn't trigger an interrupt anymore (as it is masked), yet it marks
> it as pending if an interrupt arrives while the interrupt handler is running.
> 
> All of this is handled by the interrupt core. My question is why for Intel MSI's
> is it necessary to make a change in the driver instead or relying on the
> interrupt core as for the pxa ?

If someone was using this driver with edge-triggered interrupt sources
so far, it was probably slower hardware and some luck (I've seen this
when driving fast-clocked devices vs. slower ones - only the latter
exposed the bug). Or that hardware did some temporary masking at
interrupt controller level while the handler was running. But that is
also not by design. It's the driver's task to ensure that all interrupt
sources are addressed once when returning from an edge-triggered
handler, and that is missing in this one.

Jan

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

Re: [PATCH v2 2/3] spi: pxa2xx: Prepare for edge-triggered interrupts

[Jarkko Nikula]

On 01/17/2017 10:10 AM, Jan Kiszka wrote:
> On 2017-01-17 08:58, Robert Jarzmik wrote:
>> Jan Kiszka <jan.kiszka@siemens.com> writes:
>>
>>> 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).
>>
>> I'd like moreover to add a question here.
>>
>> In pxa architecture, SPI interrupts are already edge-triggered, and it's working
>> well. The interrupt source disabling is not disabled, but the interrupt
>> controller doesn't trigger an interrupt anymore (as it is masked), yet it marks
>> it as pending if an interrupt arrives while the interrupt handler is running.
>>
>> All of this is handled by the interrupt core. My question is why for Intel MSI's
>> is it necessary to make a change in the driver instead or relying on the
>> interrupt core as for the pxa ?
>
> If someone was using this driver with edge-triggered interrupt sources
> so far, it was probably slower hardware and some luck (I've seen this
> when driving fast-clocked devices vs. slower ones - only the latter
> exposed the bug). Or that hardware did some temporary masking at
> interrupt controller level while the handler was running. But that is
> also not by design. It's the driver's task to ensure that all interrupt
> sources are addressed once when returning from an edge-triggered
> handler, and that is missing in this one.
>
Are you seeing actual problem here or adding loop just in case? Is it 
really so that PCI bridge doesn't generate another MSI interrupt if SPI 
controller has interrupt pending when handler returns? I don't know but 
I would expect irq line between SPI controller and PCI bridge is still 
level sensitive even PCI bridge issues MSIs to the CPU.

-- 
Jarkko

Re: [PATCH v2 2/3] spi: pxa2xx: Prepare for edge-triggered interrupts

[Jan Kiszka]

On 2017-01-17 14:11, Jarkko Nikula wrote:
> On 01/17/2017 10:10 AM, Jan Kiszka wrote:
>> On 2017-01-17 08:58, Robert Jarzmik wrote:
>>> Jan Kiszka <jan.kiszka@siemens.com> writes:
>>>
>>>> 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).
>>>
>>> I'd like moreover to add a question here.
>>>
>>> In pxa architecture, SPI interrupts are already edge-triggered, and
>>> it's working
>>> well. The interrupt source disabling is not disabled, but the interrupt
>>> controller doesn't trigger an interrupt anymore (as it is masked),
>>> yet it marks
>>> it as pending if an interrupt arrives while the interrupt handler is
>>> running.
>>>
>>> All of this is handled by the interrupt core. My question is why for
>>> Intel MSI's
>>> is it necessary to make a change in the driver instead or relying on the
>>> interrupt core as for the pxa ?
>>
>> If someone was using this driver with edge-triggered interrupt sources
>> so far, it was probably slower hardware and some luck (I've seen this
>> when driving fast-clocked devices vs. slower ones - only the latter
>> exposed the bug). Or that hardware did some temporary masking at
>> interrupt controller level while the handler was running. But that is
>> also not by design. It's the driver's task to ensure that all interrupt
>> sources are addressed once when returning from an edge-triggered
>> handler, and that is missing in this one.
>>
> Are you seeing actual problem here or adding loop just in case? Is it
> really so that PCI bridge doesn't generate another MSI interrupt if SPI
> controller has interrupt pending when handler returns? I don't know but
> I would expect irq line between SPI controller and PCI bridge is still
> level sensitive even PCI bridge issues MSIs to the CPU.
> 

Yes, I'm seeing real problems, e.g. on the Galileo board when running
against a slower SPI device and using MSI: An interrupt is raised
because the TX queue was flushed towards the device (threshold
underrun). While handling that interrupt, the device starts to respond
and an RX event occurs as well. This raises the related interrupt reason
before the TX source was satisfied. Therefore, the interrupt output of
the SPI master will never go down, and there will be no additional edge
generated. Using level-interrupts, this is no problem, but with
edge-triggered we get stuck.

Jan

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

[PATCH v2 3/3] spi: pca2xx-pci: Allow MSI

[Jan Kiszka]

Now that the core is ready for edge-triggered interrupts, we can safely
allow the PCI versions that provide this to enable the feature and,
thus, have less shared interrupts.

Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
---
 drivers/spi/spi-pxa2xx-pci.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/drivers/spi/spi-pxa2xx-pci.c b/drivers/spi/spi-pxa2xx-pci.c
index 58d2d48..58dcadb 100644
--- a/drivers/spi/spi-pxa2xx-pci.c
+++ b/drivers/spi/spi-pxa2xx-pci.c
@@ -203,10 +203,16 @@ static int pxa2xx_spi_pci_probe(struct pci_dev *dev,
 	ssp = &spi_pdata.ssp;
 	ssp->phys_base = pci_resource_start(dev, 0);
 	ssp->mmio_base = pcim_iomap_table(dev)[0];
-	ssp->irq = dev->irq;
 	ssp->port_id = (c->port_id >= 0) ? c->port_id : dev->devfn;
 	ssp->type = c->type;
 
+	pci_set_master(dev);
+
+	ret = pci_alloc_irq_vectors(dev, 1, 1, PCI_IRQ_ALL_TYPES);
+	if (ret < 0)
+		return ret;
+	ssp->irq = pci_irq_vector(dev, 0);
+
 	snprintf(buf, sizeof(buf), "pxa2xx-spi.%d", ssp->port_id);
 	ssp->clk = clk_register_fixed_rate(&dev->dev, buf , NULL, 0,
 					   c->max_clk_rate);
-- 
2.1.4

Re: [PATCH v2 3/3] spi: pca2xx-pci: Allow MSI

[Andy Shevchenko]

On Mon, 2017-01-16 at 19:44 +0100, Jan Kiszka wrote:
> Now that the core is ready for edge-triggered interrupts, we can
> safely
> allow the PCI versions that provide this to enable the feature and,
> thus, have less shared interrupts.
> 

Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>



> drivers/spi/spi-pxa2xx-pci.c | 8 +++++++-
>  1 file changed, 7 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/spi/spi-pxa2xx-pci.c b/drivers/spi/spi-pxa2xx-
> pci.c
> index 58d2d48..58dcadb 100644
> --- a/drivers/spi/spi-pxa2xx-pci.c
> +++ b/drivers/spi/spi-pxa2xx-pci.c
> @@ -203,10 +203,16 @@ static int pxa2xx_spi_pci_probe(struct pci_dev
> *dev,
>  	ssp = &spi_pdata.ssp;
>  	ssp->phys_base = pci_resource_start(dev, 0);
>  	ssp->mmio_base = pcim_iomap_table(dev)[0];
> -	ssp->irq = dev->irq;
>  	ssp->port_id = (c->port_id >= 0) ? c->port_id : dev->devfn;
>  	ssp->type = c->type;
>  
> +	pci_set_master(dev);
> +
> +	ret = pci_alloc_irq_vectors(dev, 1, 1, PCI_IRQ_ALL_TYPES);
> +	if (ret < 0)
> +		return ret;

+ perhaps an empty line?

> +	ssp->irq = pci_irq_vector(dev, 0);
> +
>  	snprintf(buf, sizeof(buf), "pxa2xx-spi.%d", ssp->port_id);
>  	ssp->clk = clk_register_fixed_rate(&dev->dev, buf , NULL, 0,
 					   c->max_clk_rate);

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

Re: [PATCH v2 3/3] spi: pca2xx-pci: Allow MSI

[Jarkko Nikula]

On 01/16/2017 09:08 PM, Andy Shevchenko wrote:
> On Mon, 2017-01-16 at 19:44 +0100, Jan Kiszka wrote:
>> Now that the core is ready for edge-triggered interrupts, we can
>> safely
>> allow the PCI versions that provide this to enable the feature and,
>> thus, have less shared interrupts.
>>
>
> Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
>
I was looking at is there need to call pci_free_irq_vectors() but 
pcim_release() takes care of that since this driver uses 
pcim_enable_device().

Reviewed-by: Jarkko Nikula <jarkko.nikula@linux.intel.com>

Applied "spi: pca2xx-pci: Allow MSI" to the spi tree

[Mark Brown]

The patch

   spi: pca2xx-pci: Allow MSI

has been applied to the spi tree at

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

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

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

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

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

Thanks,
Mark

>From 64e02cb0bdfc7cef0a01e2ad4d567fdc0a74450e Mon Sep 17 00:00:00 2001
From: Jan Kiszka <jan.kiszka@siemens.com>
Date: Sat, 21 Jan 2017 10:06:39 +0100
Subject: [PATCH] spi: pca2xx-pci: Allow MSI

Now that the core is ready for edge-triggered interrupts, we can safely
allow the PCI versions that provide this to enable the feature and,
thus, have less shared interrupts.

Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Reviewed-by: Jarkko Nikula <jarkko.nikula@linux.intel.com>
Signed-off-by: Mark Brown <broonie@kernel.org>
---
 drivers/spi/spi-pxa2xx-pci.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/drivers/spi/spi-pxa2xx-pci.c b/drivers/spi/spi-pxa2xx-pci.c
index 868452d8e3e1..869f188b02eb 100644
--- a/drivers/spi/spi-pxa2xx-pci.c
+++ b/drivers/spi/spi-pxa2xx-pci.c
@@ -227,10 +227,16 @@ static int pxa2xx_spi_pci_probe(struct pci_dev *dev,
 	ssp = &spi_pdata.ssp;
 	ssp->phys_base = pci_resource_start(dev, 0);
 	ssp->mmio_base = pcim_iomap_table(dev)[0];
-	ssp->irq = dev->irq;
 	ssp->port_id = (c->port_id >= 0) ? c->port_id : dev->devfn;
 	ssp->type = c->type;
 
+	pci_set_master(dev);
+
+	ret = pci_alloc_irq_vectors(dev, 1, 1, PCI_IRQ_ALL_TYPES);
+	if (ret < 0)
+		return ret;
+	ssp->irq = pci_irq_vector(dev, 0);
+
 	snprintf(buf, sizeof(buf), "pxa2xx-spi.%d", ssp->port_id);
 	ssp->clk = clk_register_fixed_rate(&dev->dev, buf , NULL, 0,
 					   c->max_clk_rate);
-- 
2.11.0