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

[PATCH 0/2] 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.

Jan

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

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

-- 
2.1.4

[PATCH 1/2] 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 | 50 +++++++++++++++++++++++++++---------------------
 1 file changed, 28 insertions(+), 22 deletions(-)

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

[PATCH 2/2] 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 | 14 ++++++++++++--
 1 file changed, 12 insertions(+), 2 deletions(-)

diff --git a/drivers/spi/spi-pxa2xx-pci.c b/drivers/spi/spi-pxa2xx-pci.c
index 58d2d48..e7e5b08 100644
--- a/drivers/spi/spi-pxa2xx-pci.c
+++ b/drivers/spi/spi-pxa2xx-pci.c
@@ -203,16 +203,24 @@ 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;
 
 	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);
-	 if (IS_ERR(ssp->clk))
+	if (IS_ERR(ssp->clk))
 		return PTR_ERR(ssp->clk);
 
+	pci_set_master(dev);
+
+	ret = pci_alloc_irq_vectors(dev, 1, 1, PCI_IRQ_ALL_TYPES);
+	if (ret < 0) {
+		clk_unregister(ssp->clk);
+		return ret;
+	}
+	ssp->irq = pci_irq_vector(dev, 0);
+
 	memset(&pi, 0, sizeof(pi));
 	pi.fwnode = dev->dev.fwnode;
 	pi.parent = &dev->dev;
@@ -224,6 +232,7 @@ static int pxa2xx_spi_pci_probe(struct pci_dev *dev,
 	pdev = platform_device_register_full(&pi);
 	if (IS_ERR(pdev)) {
 		clk_unregister(ssp->clk);
+		pci_free_irq_vectors(dev);
 		return PTR_ERR(pdev);
 	}
 
@@ -241,6 +250,7 @@ static void pxa2xx_spi_pci_remove(struct pci_dev *dev)
 
 	platform_device_unregister(pdev);
 	clk_unregister(spi_pdata->ssp.clk);
+	pci_free_irq_vectors(dev);
 }
 
 static const struct pci_device_id pxa2xx_spi_pci_devices[] = {
-- 
2.1.4

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

[Andy Shevchenko]

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

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

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

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

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

[Andy Shevchenko]

On Mon, 2017-01-16 at 10:06 +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.
> 

My comments below.

> -	 if (IS_ERR(ssp->clk))
> +	if (IS_ERR(ssp->clk))
>  		return PTR_ERR(ssp->clk);

This doesn't belong to the patch.
 
> +	pci_set_master(dev);
> +
> +	ret = pci_alloc_irq_vectors(dev, 1, 1, PCI_IRQ_ALL_TYPES);
> +	if (ret < 0) {
> +		clk_unregister(ssp->clk);
> +		return ret;
> +	}
> +	ssp->irq = pci_irq_vector(dev, 0);
> +

This looks good, though I would put it closer to the initial place of
ssp->irq assignment, i.e. before clock registering.

> +		pci_free_irq_vectors(dev);
> +	pci_free_irq_vectors(dev);

You know my answer, right? So, please be sure that we are using
pcim_alloc_irq_vectors().

Yes, I know there is (was?) no such API, needs to be created. Currently
this might make a mess on ->remove().

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

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

[Jan Kiszka]

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

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

Thanks,
Jan

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

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

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

[Jan Kiszka]

On 2017-01-16 10:29, Andy Shevchenko wrote:
> On Mon, 2017-01-16 at 10:06 +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.
>>
> 
> My comments below.
> 
>> -	 if (IS_ERR(ssp->clk))
>> +	if (IS_ERR(ssp->clk))
>>  		return PTR_ERR(ssp->clk);
> 
> This doesn't belong to the patch.
>  
>> +	pci_set_master(dev);
>> +
>> +	ret = pci_alloc_irq_vectors(dev, 1, 1, PCI_IRQ_ALL_TYPES);
>> +	if (ret < 0) {
>> +		clk_unregister(ssp->clk);
>> +		return ret;
>> +	}
>> +	ssp->irq = pci_irq_vector(dev, 0);
>> +
> 
> This looks good, though I would put it closer to the initial place of
> ssp->irq assignment, i.e. before clock registering.
> 
>> +		pci_free_irq_vectors(dev);
>> +	pci_free_irq_vectors(dev);
> 
> You know my answer, right? So, please be sure that we are using
> pcim_alloc_irq_vectors().
> 
> Yes, I know there is (was?) no such API, needs to be created. Currently
> this might make a mess on ->remove().
> 

FWIW, I've an updated version of this patch already, addressing the
remarks. Just waiting for a reply on the other patch now.

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

[Andy Shevchenko]

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

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

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

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

Let's start from above.

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

-- 
With Best Regards,
Andy Shevchenko

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

[Jan Kiszka]

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

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

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

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

Jan

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