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

Groups > linux.kernel > #1560317 > unrolled thread

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

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH v2 2/3] spi: pxa2xx: Prepare for edge-triggered interrupts Robert Jarzmik <robert.jarzmik@free.fr> - 2017-01-17 09:00 +0100
    Re: [PATCH v2 2/3] spi: pxa2xx: Prepare for edge-triggered interrupts Jan Kiszka <jan.kiszka@siemens.com> - 2017-01-17 09:10 +0100
      Re: [PATCH v2 2/3] spi: pxa2xx: Prepare for edge-triggered interrupts Robert Jarzmik <robert.jarzmik@free.fr> - 2017-01-18 09:30 +0100
        Re: [PATCH v2 2/3] spi: pxa2xx: Prepare for edge-triggered interrupts Jan Kiszka <jan.kiszka@siemens.com> - 2017-01-18 10:40 +0100
          Re: [PATCH v2 2/3] spi: pxa2xx: Prepare for edge-triggered interrupts Mark Brown <broonie@kernel.org> - 2017-01-18 13:50 +0100
            Re: [PATCH v2 2/3] spi: pxa2xx: Prepare for edge-triggered interrupts Jan Kiszka <jan.kiszka@siemens.com> - 2017-01-19 17:10 +0100
              [PATCH v3 2/3] spi: pxa2xx: Prepare for edge-triggered interrupts Jan Kiszka <jan.kiszka@siemens.com> - 2017-01-19 20:40 +0100
                Re: [PATCH v3 2/3] spi: pxa2xx: Prepare for edge-triggered  interrupts Andy Shevchenko <andriy.shevchenko@linux.intel.com> - 2017-01-19 21:10 +0100
                  Re: [PATCH v3 2/3] spi: pxa2xx: Prepare for edge-triggered interrupts Mark Brown <broonie@kernel.org> - 2017-01-20 13:30 +0100
                    Re: [PATCH v3 2/3] spi: pxa2xx: Prepare for edge-triggered interrupts Jan Kiszka <jan.kiszka@siemens.com> - 2017-01-20 16:40 +0100
                      Re: [PATCH v3 2/3] spi: pxa2xx: Prepare for edge-triggered interrupts Mark Brown <broonie@kernel.org> - 2017-01-20 18:40 +0100
                Re: [PATCH v3 2/3] spi: pxa2xx: Prepare for edge-triggered interrupts Mark Brown <broonie@kernel.org> - 2017-01-19 21:30 +0100
                Re: [PATCH v3 2/3] spi: pxa2xx: Prepare for edge-triggered interrupts Robert Jarzmik <robert.jarzmik@free.fr> - 2017-01-20 09:00 +0100
              Applied "spi: pxa2xx: Prepare for edge-triggered interrupts" to the spi tree Mark Brown <broonie@kernel.org> - 2017-01-24 19:50 +0100

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

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

[toc] | [next] | [standalone]

#1560324

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

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

#1561352

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

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

#1561395

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

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

#1561726

[Multipart message — attachments visible in raw view] — view raw

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.

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

#1562862

[Multipart message — attachments visible in raw view] — view raw

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

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

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

[Multipart message — attachments visible in raw view] — view raw

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

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

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

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

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

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

[Multipart message — attachments visible in raw view] — view raw

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.

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

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

[Multipart message — attachments visible in raw view] — view raw

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

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

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

[Multipart message — attachments visible in raw view] — view raw

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.

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

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

[Multipart message — attachments visible in raw view] — view raw

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?

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

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

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
 

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

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

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

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web