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 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
Re: [PATCH 1/2] spi: pxa2xx: Prepare for edge-triggered interrupts
On Mon, Jan 16, 2017 at 1:18 PM, Jan Kiszka 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
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
>> ---
>> 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
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
> ---
> 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
Intel Finland Oy
