[PATCH 0/4] Add polling support for 64xx spi controller

[PATCH 0/4] Add polling support for 64xx spi controller

[Girish K S]

[PATCH 1/4]: fixes the error handling in the interrupt handler
[PATCH 2/4]: The existing driver support partial polling mode.
	     This patch modifies the current driver to support
	     only polling mode.
[PATCH 3/4]: Adds quirk to support SoC's with dedicated i/o pins
[PATCH 4/4]: Adds the support for exynos5440 SoC, this SoC has
	     no support for dma transfer and consists of dedicated
	     i/o pins.

Girish K S (4):
  spi: s3c64xx: modified error interrupt handling and init
  spi: s3c64xx: added support for polling mode
  spi: s3c64xx: add gpio quirk for controller
  spi: s3c64xx: add support for exynos5440 spi

 drivers/spi/spi-s3c64xx.c |  157 ++++++++++++++++++++++++++++-----------------
 1 file changed, 98 insertions(+), 59 deletions(-)

-- 
1.7.10.4

[PATCH 1/4] spi: s3c64xx: modified error interrupt handling and init

[Girish K S]

The status of the interrupt is available in the status register,
so reading the clear pending register and writing back the same
value will not actually clear the pending interrupts. This patch
modifies the interrupt handler to read the status register and
clear the corresponding pending bit in the clear pending register.

Modified the hwInit function to clear all the pending interrupts.

Signed-off-by: Girish K S <ks.giri@samsung.com>
---
 drivers/spi/spi-s3c64xx.c |   41 +++++++++++++++++++++++++----------------
 1 file changed, 25 insertions(+), 16 deletions(-)

diff --git a/drivers/spi/spi-s3c64xx.c b/drivers/spi/spi-s3c64xx.c
index ad93231..b770f88 100644
--- a/drivers/spi/spi-s3c64xx.c
+++ b/drivers/spi/spi-s3c64xx.c
@@ -997,25 +997,30 @@ static irqreturn_t s3c64xx_spi_irq(int irq, void *data)
 {
 	struct s3c64xx_spi_driver_data *sdd = data;
 	struct spi_master *spi = sdd->master;
-	unsigned int val;
+	unsigned int val, clr = 0;
 
-	val = readl(sdd->regs + S3C64XX_SPI_PENDING_CLR);
+	val = readl(sdd->regs + S3C64XX_SPI_STATUS);
 
-	val &= S3C64XX_SPI_PND_RX_OVERRUN_CLR |
-		S3C64XX_SPI_PND_RX_UNDERRUN_CLR |
-		S3C64XX_SPI_PND_TX_OVERRUN_CLR |
-		S3C64XX_SPI_PND_TX_UNDERRUN_CLR;
-
-	writel(val, sdd->regs + S3C64XX_SPI_PENDING_CLR);
-
-	if (val & S3C64XX_SPI_PND_RX_OVERRUN_CLR)
+	if (val & S3C64XX_SPI_ST_RX_OVERRUN_ERR) {
+		clr = S3C64XX_SPI_PND_RX_OVERRUN_CLR;
 		dev_err(&spi->dev, "RX overrun\n");
-	if (val & S3C64XX_SPI_PND_RX_UNDERRUN_CLR)
+	}
+	if (val & S3C64XX_SPI_ST_RX_UNDERRUN_ERR) {
+		clr |= S3C64XX_SPI_PND_RX_UNDERRUN_CLR;
 		dev_err(&spi->dev, "RX underrun\n");
-	if (val & S3C64XX_SPI_PND_TX_OVERRUN_CLR)
+	}
+	if (val & S3C64XX_SPI_ST_TX_OVERRUN_ERR) {
+		clr |= S3C64XX_SPI_PND_TX_OVERRUN_CLR;
 		dev_err(&spi->dev, "TX overrun\n");
-	if (val & S3C64XX_SPI_PND_TX_UNDERRUN_CLR)
+	}
+	if (val & S3C64XX_SPI_ST_TX_UNDERRUN_ERR) {
+		clr |= S3C64XX_SPI_PND_TX_UNDERRUN_CLR;
 		dev_err(&spi->dev, "TX underrun\n");
+	}
+
+	/* Clear the pending irq by setting and then clearing it */
+	writel(clr, sdd->regs + S3C64XX_SPI_PENDING_CLR);
+	writel(clr & ~clr, sdd->regs + S3C64XX_SPI_PENDING_CLR);
 
 	return IRQ_HANDLED;
 }
@@ -1039,9 +1044,13 @@ static void s3c64xx_spi_hwinit(struct s3c64xx_spi_driver_data *sdd, int channel)
 	writel(0, regs + S3C64XX_SPI_MODE_CFG);
 	writel(0, regs + S3C64XX_SPI_PACKET_CNT);
 
-	/* Clear any irq pending bits */
-	writel(readl(regs + S3C64XX_SPI_PENDING_CLR),
-				regs + S3C64XX_SPI_PENDING_CLR);
+	/* Clear any irq pending bits, should set and clear the bits */
+	val = S3C64XX_SPI_PND_RX_OVERRUN_CLR |
+		S3C64XX_SPI_PND_RX_UNDERRUN_CLR |
+		S3C64XX_SPI_PND_TX_OVERRUN_CLR |
+		S3C64XX_SPI_PND_TX_UNDERRUN_CLR;
+	writel(val, regs + S3C64XX_SPI_PENDING_CLR);
+	writel(val & ~val, regs + S3C64XX_SPI_PENDING_CLR);
 
 	writel(0, regs + S3C64XX_SPI_SWAP_CFG);
 
-- 
1.7.10.4

Re: [PATCH 1/4] spi: s3c64xx: modified error interrupt handling and init

[Grant Likely]

On Tue,  5 Feb 2013 15:09:41 -0800, Girish K S <girishks2000@gmail.com> wrote:
> The status of the interrupt is available in the status register,
> so reading the clear pending register and writing back the same
> value will not actually clear the pending interrupts. This patch
> modifies the interrupt handler to read the status register and
> clear the corresponding pending bit in the clear pending register.
> 
> Modified the hwInit function to clear all the pending interrupts.
> 
> Signed-off-by: Girish K S <ks.giri@samsung.com>
> ---
>  drivers/spi/spi-s3c64xx.c |   41 +++++++++++++++++++++++++----------------
>  1 file changed, 25 insertions(+), 16 deletions(-)
> 
> diff --git a/drivers/spi/spi-s3c64xx.c b/drivers/spi/spi-s3c64xx.c
> index ad93231..b770f88 100644
> --- a/drivers/spi/spi-s3c64xx.c
> +++ b/drivers/spi/spi-s3c64xx.c
> @@ -997,25 +997,30 @@ static irqreturn_t s3c64xx_spi_irq(int irq, void *data)
>  {
>  	struct s3c64xx_spi_driver_data *sdd = data;
>  	struct spi_master *spi = sdd->master;
> -	unsigned int val;
> +	unsigned int val, clr = 0;
>  
> -	val = readl(sdd->regs + S3C64XX_SPI_PENDING_CLR);
> +	val = readl(sdd->regs + S3C64XX_SPI_STATUS);
>  
> -	val &= S3C64XX_SPI_PND_RX_OVERRUN_CLR |
> -		S3C64XX_SPI_PND_RX_UNDERRUN_CLR |
> -		S3C64XX_SPI_PND_TX_OVERRUN_CLR |
> -		S3C64XX_SPI_PND_TX_UNDERRUN_CLR;
> -
> -	writel(val, sdd->regs + S3C64XX_SPI_PENDING_CLR);
> -
> -	if (val & S3C64XX_SPI_PND_RX_OVERRUN_CLR)
> +	if (val & S3C64XX_SPI_ST_RX_OVERRUN_ERR) {
> +		clr = S3C64XX_SPI_PND_RX_OVERRUN_CLR;
>  		dev_err(&spi->dev, "RX overrun\n");
> -	if (val & S3C64XX_SPI_PND_RX_UNDERRUN_CLR)
> +	}
> +	if (val & S3C64XX_SPI_ST_RX_UNDERRUN_ERR) {
> +		clr |= S3C64XX_SPI_PND_RX_UNDERRUN_CLR;
>  		dev_err(&spi->dev, "RX underrun\n");
> -	if (val & S3C64XX_SPI_PND_TX_OVERRUN_CLR)
> +	}
> +	if (val & S3C64XX_SPI_ST_TX_OVERRUN_ERR) {
> +		clr |= S3C64XX_SPI_PND_TX_OVERRUN_CLR;
>  		dev_err(&spi->dev, "TX overrun\n");
> -	if (val & S3C64XX_SPI_PND_TX_UNDERRUN_CLR)
> +	}
> +	if (val & S3C64XX_SPI_ST_TX_UNDERRUN_ERR) {
> +		clr |= S3C64XX_SPI_PND_TX_UNDERRUN_CLR;
>  		dev_err(&spi->dev, "TX underrun\n");
> +	}
> +
> +	/* Clear the pending irq by setting and then clearing it */
> +	writel(clr, sdd->regs + S3C64XX_SPI_PENDING_CLR);
> +	writel(clr & ~clr, sdd->regs + S3C64XX_SPI_PENDING_CLR);

Wait, what?  clr & ~clr == 0   Always.  What are you actually trying to do here?

>  
>  	return IRQ_HANDLED;
>  }
> @@ -1039,9 +1044,13 @@ static void s3c64xx_spi_hwinit(struct s3c64xx_spi_driver_data *sdd, int channel)
>  	writel(0, regs + S3C64XX_SPI_MODE_CFG);
>  	writel(0, regs + S3C64XX_SPI_PACKET_CNT);
>  
> -	/* Clear any irq pending bits */
> -	writel(readl(regs + S3C64XX_SPI_PENDING_CLR),
> -				regs + S3C64XX_SPI_PENDING_CLR);
> +	/* Clear any irq pending bits, should set and clear the bits */
> +	val = S3C64XX_SPI_PND_RX_OVERRUN_CLR |
> +		S3C64XX_SPI_PND_RX_UNDERRUN_CLR |
> +		S3C64XX_SPI_PND_TX_OVERRUN_CLR |
> +		S3C64XX_SPI_PND_TX_UNDERRUN_CLR;
> +	writel(val, regs + S3C64XX_SPI_PENDING_CLR);
> +	writel(val & ~val, regs + S3C64XX_SPI_PENDING_CLR);

Ditto.

g.

Re: [PATCH 1/4] spi: s3c64xx: modified error interrupt handling and init

[Girish KS]

On Wed, Feb 6, 2013 at 2:26 AM, Grant Likely <grant.likely@secretlab.ca> wrote:
> On Tue,  5 Feb 2013 15:09:41 -0800, Girish K S <girishks2000@gmail.com> wrote:
>> The status of the interrupt is available in the status register,
>> so reading the clear pending register and writing back the same
>> value will not actually clear the pending interrupts. This patch
>> modifies the interrupt handler to read the status register and
>> clear the corresponding pending bit in the clear pending register.
>>
>> Modified the hwInit function to clear all the pending interrupts.
>>
>> Signed-off-by: Girish K S <ks.giri@samsung.com>
>> ---
>>  drivers/spi/spi-s3c64xx.c |   41 +++++++++++++++++++++++++----------------
>>  1 file changed, 25 insertions(+), 16 deletions(-)
>>
>> diff --git a/drivers/spi/spi-s3c64xx.c b/drivers/spi/spi-s3c64xx.c
>> index ad93231..b770f88 100644
>> --- a/drivers/spi/spi-s3c64xx.c
>> +++ b/drivers/spi/spi-s3c64xx.c
>> @@ -997,25 +997,30 @@ static irqreturn_t s3c64xx_spi_irq(int irq, void *data)
>>  {
>>       struct s3c64xx_spi_driver_data *sdd = data;
>>       struct spi_master *spi = sdd->master;
>> -     unsigned int val;
>> +     unsigned int val, clr = 0;
>>
>> -     val = readl(sdd->regs + S3C64XX_SPI_PENDING_CLR);
>> +     val = readl(sdd->regs + S3C64XX_SPI_STATUS);
>>
>> -     val &= S3C64XX_SPI_PND_RX_OVERRUN_CLR |
>> -             S3C64XX_SPI_PND_RX_UNDERRUN_CLR |
>> -             S3C64XX_SPI_PND_TX_OVERRUN_CLR |
>> -             S3C64XX_SPI_PND_TX_UNDERRUN_CLR;
>> -
>> -     writel(val, sdd->regs + S3C64XX_SPI_PENDING_CLR);
>> -
>> -     if (val & S3C64XX_SPI_PND_RX_OVERRUN_CLR)
>> +     if (val & S3C64XX_SPI_ST_RX_OVERRUN_ERR) {
>> +             clr = S3C64XX_SPI_PND_RX_OVERRUN_CLR;
>>               dev_err(&spi->dev, "RX overrun\n");
>> -     if (val & S3C64XX_SPI_PND_RX_UNDERRUN_CLR)
>> +     }
>> +     if (val & S3C64XX_SPI_ST_RX_UNDERRUN_ERR) {
>> +             clr |= S3C64XX_SPI_PND_RX_UNDERRUN_CLR;
>>               dev_err(&spi->dev, "RX underrun\n");
>> -     if (val & S3C64XX_SPI_PND_TX_OVERRUN_CLR)
>> +     }
>> +     if (val & S3C64XX_SPI_ST_TX_OVERRUN_ERR) {
>> +             clr |= S3C64XX_SPI_PND_TX_OVERRUN_CLR;
>>               dev_err(&spi->dev, "TX overrun\n");
>> -     if (val & S3C64XX_SPI_PND_TX_UNDERRUN_CLR)
>> +     }
>> +     if (val & S3C64XX_SPI_ST_TX_UNDERRUN_ERR) {
>> +             clr |= S3C64XX_SPI_PND_TX_UNDERRUN_CLR;
>>               dev_err(&spi->dev, "TX underrun\n");
>> +     }
>> +
>> +     /* Clear the pending irq by setting and then clearing it */
>> +     writel(clr, sdd->regs + S3C64XX_SPI_PENDING_CLR);
>> +     writel(clr & ~clr, sdd->regs + S3C64XX_SPI_PENDING_CLR);
>
> Wait, what?  clr & ~clr == 0   Always.  What are you actually trying to do here?
The user manual says, wirting 1 to the pending clear register clears
the interrupt (its not auto clear to 0). so i need to explicitly reset
those bits thats what the 2nd write does
>
>>
>>       return IRQ_HANDLED;
>>  }
>> @@ -1039,9 +1044,13 @@ static void s3c64xx_spi_hwinit(struct s3c64xx_spi_driver_data *sdd, int channel)
>>       writel(0, regs + S3C64XX_SPI_MODE_CFG);
>>       writel(0, regs + S3C64XX_SPI_PACKET_CNT);
>>
>> -     /* Clear any irq pending bits */
>> -     writel(readl(regs + S3C64XX_SPI_PENDING_CLR),
>> -                             regs + S3C64XX_SPI_PENDING_CLR);
>> +     /* Clear any irq pending bits, should set and clear the bits */
>> +     val = S3C64XX_SPI_PND_RX_OVERRUN_CLR |
>> +             S3C64XX_SPI_PND_RX_UNDERRUN_CLR |
>> +             S3C64XX_SPI_PND_TX_OVERRUN_CLR |
>> +             S3C64XX_SPI_PND_TX_UNDERRUN_CLR;
>> +     writel(val, regs + S3C64XX_SPI_PENDING_CLR);
>> +     writel(val & ~val, regs + S3C64XX_SPI_PENDING_CLR);
>
> Ditto.
same as above
>
> g.

Re: [PATCH 1/4] spi: s3c64xx: modified error interrupt handling and init

[Grant Likely]

On Wed, Feb 6, 2013 at 8:12 PM, Girish KS <girishks2000@gmail.com> wrote:
> On Wed, Feb 6, 2013 at 2:26 AM, Grant Likely <grant.likely@secretlab.ca> wrote:
>> On Tue,  5 Feb 2013 15:09:41 -0800, Girish K S <girishks2000@gmail.com> wrote:
>>> The status of the interrupt is available in the status register,
>>> so reading the clear pending register and writing back the same
>>> value will not actually clear the pending interrupts. This patch
>>> modifies the interrupt handler to read the status register and
>>> clear the corresponding pending bit in the clear pending register.
>>>
>>> Modified the hwInit function to clear all the pending interrupts.
>>>
>>> Signed-off-by: Girish K S <ks.giri@samsung.com>
>>> ---
>>>  drivers/spi/spi-s3c64xx.c |   41 +++++++++++++++++++++++++----------------
>>>  1 file changed, 25 insertions(+), 16 deletions(-)
>>>
>>> diff --git a/drivers/spi/spi-s3c64xx.c b/drivers/spi/spi-s3c64xx.c
>>> index ad93231..b770f88 100644
>>> --- a/drivers/spi/spi-s3c64xx.c
>>> +++ b/drivers/spi/spi-s3c64xx.c
>>> @@ -997,25 +997,30 @@ static irqreturn_t s3c64xx_spi_irq(int irq, void *data)
>>>  {
>>>       struct s3c64xx_spi_driver_data *sdd = data;
>>>       struct spi_master *spi = sdd->master;
>>> -     unsigned int val;
>>> +     unsigned int val, clr = 0;
>>>
>>> -     val = readl(sdd->regs + S3C64XX_SPI_PENDING_CLR);
>>> +     val = readl(sdd->regs + S3C64XX_SPI_STATUS);
>>>
>>> -     val &= S3C64XX_SPI_PND_RX_OVERRUN_CLR |
>>> -             S3C64XX_SPI_PND_RX_UNDERRUN_CLR |
>>> -             S3C64XX_SPI_PND_TX_OVERRUN_CLR |
>>> -             S3C64XX_SPI_PND_TX_UNDERRUN_CLR;
>>> -
>>> -     writel(val, sdd->regs + S3C64XX_SPI_PENDING_CLR);
>>> -
>>> -     if (val & S3C64XX_SPI_PND_RX_OVERRUN_CLR)
>>> +     if (val & S3C64XX_SPI_ST_RX_OVERRUN_ERR) {
>>> +             clr = S3C64XX_SPI_PND_RX_OVERRUN_CLR;
>>>               dev_err(&spi->dev, "RX overrun\n");
>>> -     if (val & S3C64XX_SPI_PND_RX_UNDERRUN_CLR)
>>> +     }
>>> +     if (val & S3C64XX_SPI_ST_RX_UNDERRUN_ERR) {
>>> +             clr |= S3C64XX_SPI_PND_RX_UNDERRUN_CLR;
>>>               dev_err(&spi->dev, "RX underrun\n");
>>> -     if (val & S3C64XX_SPI_PND_TX_OVERRUN_CLR)
>>> +     }
>>> +     if (val & S3C64XX_SPI_ST_TX_OVERRUN_ERR) {
>>> +             clr |= S3C64XX_SPI_PND_TX_OVERRUN_CLR;
>>>               dev_err(&spi->dev, "TX overrun\n");
>>> -     if (val & S3C64XX_SPI_PND_TX_UNDERRUN_CLR)
>>> +     }
>>> +     if (val & S3C64XX_SPI_ST_TX_UNDERRUN_ERR) {
>>> +             clr |= S3C64XX_SPI_PND_TX_UNDERRUN_CLR;
>>>               dev_err(&spi->dev, "TX underrun\n");
>>> +     }
>>> +
>>> +     /* Clear the pending irq by setting and then clearing it */
>>> +     writel(clr, sdd->regs + S3C64XX_SPI_PENDING_CLR);
>>> +     writel(clr & ~clr, sdd->regs + S3C64XX_SPI_PENDING_CLR);
>>
>> Wait, what?  clr & ~clr == 0   Always.  What are you actually trying to do here?
> The user manual says, wirting 1 to the pending clear register clears
> the interrupt (its not auto clear to 0). so i need to explicitly reset
> those bits thats what the 2nd write does

Then write 0. That's the result of what the code does anyway, but the
code as-written is nonsensical.

g.

Re: [PATCH 1/4] spi: s3c64xx: modified error interrupt handling and init

[Girish KS]

On Wed, Feb 6, 2013 at 3:48 PM, Grant Likely <grant.likely@secretlab.ca> wrote:
> On Wed, Feb 6, 2013 at 8:12 PM, Girish KS <girishks2000@gmail.com> wrote:
>> On Wed, Feb 6, 2013 at 2:26 AM, Grant Likely <grant.likely@secretlab.ca> wrote:
>>> On Tue,  5 Feb 2013 15:09:41 -0800, Girish K S <girishks2000@gmail.com> wrote:
>>>> The status of the interrupt is available in the status register,
>>>> so reading the clear pending register and writing back the same
>>>> value will not actually clear the pending interrupts. This patch
>>>> modifies the interrupt handler to read the status register and
>>>> clear the corresponding pending bit in the clear pending register.
>>>>
>>>> Modified the hwInit function to clear all the pending interrupts.
>>>>
>>>> Signed-off-by: Girish K S <ks.giri@samsung.com>
>>>> ---
>>>>  drivers/spi/spi-s3c64xx.c |   41 +++++++++++++++++++++++++----------------
>>>>  1 file changed, 25 insertions(+), 16 deletions(-)
>>>>
>>>> diff --git a/drivers/spi/spi-s3c64xx.c b/drivers/spi/spi-s3c64xx.c
>>>> index ad93231..b770f88 100644
>>>> --- a/drivers/spi/spi-s3c64xx.c
>>>> +++ b/drivers/spi/spi-s3c64xx.c
>>>> @@ -997,25 +997,30 @@ static irqreturn_t s3c64xx_spi_irq(int irq, void *data)
>>>>  {
>>>>       struct s3c64xx_spi_driver_data *sdd = data;
>>>>       struct spi_master *spi = sdd->master;
>>>> -     unsigned int val;
>>>> +     unsigned int val, clr = 0;
>>>>
>>>> -     val = readl(sdd->regs + S3C64XX_SPI_PENDING_CLR);
>>>> +     val = readl(sdd->regs + S3C64XX_SPI_STATUS);
>>>>
>>>> -     val &= S3C64XX_SPI_PND_RX_OVERRUN_CLR |
>>>> -             S3C64XX_SPI_PND_RX_UNDERRUN_CLR |
>>>> -             S3C64XX_SPI_PND_TX_OVERRUN_CLR |
>>>> -             S3C64XX_SPI_PND_TX_UNDERRUN_CLR;
>>>> -
>>>> -     writel(val, sdd->regs + S3C64XX_SPI_PENDING_CLR);
>>>> -
>>>> -     if (val & S3C64XX_SPI_PND_RX_OVERRUN_CLR)
>>>> +     if (val & S3C64XX_SPI_ST_RX_OVERRUN_ERR) {
>>>> +             clr = S3C64XX_SPI_PND_RX_OVERRUN_CLR;
>>>>               dev_err(&spi->dev, "RX overrun\n");
>>>> -     if (val & S3C64XX_SPI_PND_RX_UNDERRUN_CLR)
>>>> +     }
>>>> +     if (val & S3C64XX_SPI_ST_RX_UNDERRUN_ERR) {
>>>> +             clr |= S3C64XX_SPI_PND_RX_UNDERRUN_CLR;
>>>>               dev_err(&spi->dev, "RX underrun\n");
>>>> -     if (val & S3C64XX_SPI_PND_TX_OVERRUN_CLR)
>>>> +     }
>>>> +     if (val & S3C64XX_SPI_ST_TX_OVERRUN_ERR) {
>>>> +             clr |= S3C64XX_SPI_PND_TX_OVERRUN_CLR;
>>>>               dev_err(&spi->dev, "TX overrun\n");
>>>> -     if (val & S3C64XX_SPI_PND_TX_UNDERRUN_CLR)
>>>> +     }
>>>> +     if (val & S3C64XX_SPI_ST_TX_UNDERRUN_ERR) {
>>>> +             clr |= S3C64XX_SPI_PND_TX_UNDERRUN_CLR;
>>>>               dev_err(&spi->dev, "TX underrun\n");
>>>> +     }
>>>> +
>>>> +     /* Clear the pending irq by setting and then clearing it */
>>>> +     writel(clr, sdd->regs + S3C64XX_SPI_PENDING_CLR);
>>>> +     writel(clr & ~clr, sdd->regs + S3C64XX_SPI_PENDING_CLR);
>>>
>>> Wait, what?  clr & ~clr == 0   Always.  What are you actually trying to do here?
>> The user manual says, wirting 1 to the pending clear register clears
>> the interrupt (its not auto clear to 0). so i need to explicitly reset
>> those bits thats what the 2nd write does
>
> Then write 0. That's the result of what the code does anyway, but the
> code as-written is nonsensical.
Just writing 0 doest clear the status bit. The status bit in the
status register is cleared only when the corresponding bit in clear
pending register is set.
If not cleared after setting. On the next Interrupt, writing 1 to a
previously set bit will not clear the status bit.
Hope its clear
>
> g.

Re: [PATCH 1/4] spi: s3c64xx: modified error interrupt handling and init

[Girish KS]

On Wed, Feb 6, 2013 at 3:48 PM, Grant Likely <grant.likely@secretlab.ca> wrote:
> On Wed, Feb 6, 2013 at 8:12 PM, Girish KS <girishks2000@gmail.com> wrote:
>> On Wed, Feb 6, 2013 at 2:26 AM, Grant Likely <grant.likely@secretlab.ca> wrote:
>>> On Tue,  5 Feb 2013 15:09:41 -0800, Girish K S <girishks2000@gmail.com> wrote:
>>>> The status of the interrupt is available in the status register,
>>>> so reading the clear pending register and writing back the same
>>>> value will not actually clear the pending interrupts. This patch
>>>> modifies the interrupt handler to read the status register and
>>>> clear the corresponding pending bit in the clear pending register.
>>>>
>>>> Modified the hwInit function to clear all the pending interrupts.
>>>>
>>>> Signed-off-by: Girish K S <ks.giri@samsung.com>
>>>> ---
>>>>  drivers/spi/spi-s3c64xx.c |   41 +++++++++++++++++++++++++----------------
>>>>  1 file changed, 25 insertions(+), 16 deletions(-)
>>>>
>>>> diff --git a/drivers/spi/spi-s3c64xx.c b/drivers/spi/spi-s3c64xx.c
>>>> index ad93231..b770f88 100644
>>>> --- a/drivers/spi/spi-s3c64xx.c
>>>> +++ b/drivers/spi/spi-s3c64xx.c
>>>> @@ -997,25 +997,30 @@ static irqreturn_t s3c64xx_spi_irq(int irq, void *data)
>>>>  {
>>>>       struct s3c64xx_spi_driver_data *sdd = data;
>>>>       struct spi_master *spi = sdd->master;
>>>> -     unsigned int val;
>>>> +     unsigned int val, clr = 0;
>>>>
>>>> -     val = readl(sdd->regs + S3C64XX_SPI_PENDING_CLR);
>>>> +     val = readl(sdd->regs + S3C64XX_SPI_STATUS);
>>>>
>>>> -     val &= S3C64XX_SPI_PND_RX_OVERRUN_CLR |
>>>> -             S3C64XX_SPI_PND_RX_UNDERRUN_CLR |
>>>> -             S3C64XX_SPI_PND_TX_OVERRUN_CLR |
>>>> -             S3C64XX_SPI_PND_TX_UNDERRUN_CLR;
>>>> -
>>>> -     writel(val, sdd->regs + S3C64XX_SPI_PENDING_CLR);
>>>> -
>>>> -     if (val & S3C64XX_SPI_PND_RX_OVERRUN_CLR)
>>>> +     if (val & S3C64XX_SPI_ST_RX_OVERRUN_ERR) {
>>>> +             clr = S3C64XX_SPI_PND_RX_OVERRUN_CLR;
>>>>               dev_err(&spi->dev, "RX overrun\n");
>>>> -     if (val & S3C64XX_SPI_PND_RX_UNDERRUN_CLR)
>>>> +     }
>>>> +     if (val & S3C64XX_SPI_ST_RX_UNDERRUN_ERR) {
>>>> +             clr |= S3C64XX_SPI_PND_RX_UNDERRUN_CLR;
>>>>               dev_err(&spi->dev, "RX underrun\n");
>>>> -     if (val & S3C64XX_SPI_PND_TX_OVERRUN_CLR)
>>>> +     }
>>>> +     if (val & S3C64XX_SPI_ST_TX_OVERRUN_ERR) {
>>>> +             clr |= S3C64XX_SPI_PND_TX_OVERRUN_CLR;
>>>>               dev_err(&spi->dev, "TX overrun\n");
>>>> -     if (val & S3C64XX_SPI_PND_TX_UNDERRUN_CLR)
>>>> +     }
>>>> +     if (val & S3C64XX_SPI_ST_TX_UNDERRUN_ERR) {
>>>> +             clr |= S3C64XX_SPI_PND_TX_UNDERRUN_CLR;
>>>>               dev_err(&spi->dev, "TX underrun\n");
>>>> +     }
>>>> +
>>>> +     /* Clear the pending irq by setting and then clearing it */
>>>> +     writel(clr, sdd->regs + S3C64XX_SPI_PENDING_CLR);
>>>> +     writel(clr & ~clr, sdd->regs + S3C64XX_SPI_PENDING_CLR);
>>>
>>> Wait, what?  clr & ~clr == 0   Always.  What are you actually trying to do here?
>> The user manual says, wirting 1 to the pending clear register clears
>> the interrupt (its not auto clear to 0). so i need to explicitly reset
>> those bits thats what the 2nd write does
>
> Then write 0. That's the result of what the code does anyway, but the
> code as-written is nonsensical.
i cannot write 0. because the 0th bit is trailing byte interrupt clear
pending bit, which is not being handled in the handler.
>
> g.

Re: [PATCH 1/4] spi: s3c64xx: modified error interrupt handling and init

[Girish KS]

On Thu, Feb 7, 2013 at 5:04 PM, Girish KS <girishks2000@gmail.com> wrote:
> On Wed, Feb 6, 2013 at 3:48 PM, Grant Likely <grant.likely@secretlab.ca> wrote:
>> On Wed, Feb 6, 2013 at 8:12 PM, Girish KS <girishks2000@gmail.com> wrote:
>>> On Wed, Feb 6, 2013 at 2:26 AM, Grant Likely <grant.likely@secretlab.ca> wrote:
>>>> On Tue,  5 Feb 2013 15:09:41 -0800, Girish K S <girishks2000@gmail.com> wrote:
>>>>> The status of the interrupt is available in the status register,
>>>>> so reading the clear pending register and writing back the same
>>>>> value will not actually clear the pending interrupts. This patch
>>>>> modifies the interrupt handler to read the status register and
>>>>> clear the corresponding pending bit in the clear pending register.
>>>>>
>>>>> Modified the hwInit function to clear all the pending interrupts.
>>>>>
>>>>> Signed-off-by: Girish K S <ks.giri@samsung.com>
>>>>> ---
>>>>>  drivers/spi/spi-s3c64xx.c |   41 +++++++++++++++++++++++++----------------
>>>>>  1 file changed, 25 insertions(+), 16 deletions(-)
>>>>>
>>>>> diff --git a/drivers/spi/spi-s3c64xx.c b/drivers/spi/spi-s3c64xx.c
>>>>> index ad93231..b770f88 100644
>>>>> --- a/drivers/spi/spi-s3c64xx.c
>>>>> +++ b/drivers/spi/spi-s3c64xx.c
>>>>> @@ -997,25 +997,30 @@ static irqreturn_t s3c64xx_spi_irq(int irq, void *data)
>>>>>  {
>>>>>       struct s3c64xx_spi_driver_data *sdd = data;
>>>>>       struct spi_master *spi = sdd->master;
>>>>> -     unsigned int val;
>>>>> +     unsigned int val, clr = 0;
>>>>>
>>>>> -     val = readl(sdd->regs + S3C64XX_SPI_PENDING_CLR);
>>>>> +     val = readl(sdd->regs + S3C64XX_SPI_STATUS);
>>>>>
>>>>> -     val &= S3C64XX_SPI_PND_RX_OVERRUN_CLR |
>>>>> -             S3C64XX_SPI_PND_RX_UNDERRUN_CLR |
>>>>> -             S3C64XX_SPI_PND_TX_OVERRUN_CLR |
>>>>> -             S3C64XX_SPI_PND_TX_UNDERRUN_CLR;
>>>>> -
>>>>> -     writel(val, sdd->regs + S3C64XX_SPI_PENDING_CLR);
>>>>> -
>>>>> -     if (val & S3C64XX_SPI_PND_RX_OVERRUN_CLR)
>>>>> +     if (val & S3C64XX_SPI_ST_RX_OVERRUN_ERR) {
>>>>> +             clr = S3C64XX_SPI_PND_RX_OVERRUN_CLR;
>>>>>               dev_err(&spi->dev, "RX overrun\n");
>>>>> -     if (val & S3C64XX_SPI_PND_RX_UNDERRUN_CLR)
>>>>> +     }
>>>>> +     if (val & S3C64XX_SPI_ST_RX_UNDERRUN_ERR) {
>>>>> +             clr |= S3C64XX_SPI_PND_RX_UNDERRUN_CLR;
>>>>>               dev_err(&spi->dev, "RX underrun\n");
>>>>> -     if (val & S3C64XX_SPI_PND_TX_OVERRUN_CLR)
>>>>> +     }
>>>>> +     if (val & S3C64XX_SPI_ST_TX_OVERRUN_ERR) {
>>>>> +             clr |= S3C64XX_SPI_PND_TX_OVERRUN_CLR;
>>>>>               dev_err(&spi->dev, "TX overrun\n");
>>>>> -     if (val & S3C64XX_SPI_PND_TX_UNDERRUN_CLR)
>>>>> +     }
>>>>> +     if (val & S3C64XX_SPI_ST_TX_UNDERRUN_ERR) {
>>>>> +             clr |= S3C64XX_SPI_PND_TX_UNDERRUN_CLR;
>>>>>               dev_err(&spi->dev, "TX underrun\n");
>>>>> +     }
>>>>> +
>>>>> +     /* Clear the pending irq by setting and then clearing it */
>>>>> +     writel(clr, sdd->regs + S3C64XX_SPI_PENDING_CLR);
>>>>> +     writel(clr & ~clr, sdd->regs + S3C64XX_SPI_PENDING_CLR);
>>>>
>>>> Wait, what?  clr & ~clr == 0   Always.  What are you actually trying to do here?
>>> The user manual says, wirting 1 to the pending clear register clears
>>> the interrupt (its not auto clear to 0). so i need to explicitly reset
>>> those bits thats what the 2nd write does
>>
>> Then write 0. That's the result of what the code does anyway, but the
>> code as-written is nonsensical.
> i cannot write 0. because the 0th bit is trailing byte interrupt clear
> pending bit, which is not being handled in the handler.

Sorry, Writing 0  will still be valid after code for trainling byte is added..
will change and resubmit

>>
>> g.

Re: [PATCH 1/4] spi: s3c64xx: modified error interrupt handling and init

[Tomasz Figa]

Hi Girish,

On Wednesday 06 of February 2013 12:12:29 Girish KS wrote:
> On Wed, Feb 6, 2013 at 2:26 AM, Grant Likely <grant.likely@secretlab.ca> 
wrote:
> > On Tue,  5 Feb 2013 15:09:41 -0800, Girish K S 
<girishks2000@gmail.com> wrote:
> >> The status of the interrupt is available in the status register,
> >> so reading the clear pending register and writing back the same
> >> value will not actually clear the pending interrupts. This patch
> >> modifies the interrupt handler to read the status register and
> >> clear the corresponding pending bit in the clear pending register.
> >> 
> >> Modified the hwInit function to clear all the pending interrupts.
> >> 
> >> Signed-off-by: Girish K S <ks.giri@samsung.com>
> >> ---
> >> 
> >>  drivers/spi/spi-s3c64xx.c |   41
> >>  +++++++++++++++++++++++++---------------- 1 file changed, 25
> >>  insertions(+), 16 deletions(-)
> >> 
> >> diff --git a/drivers/spi/spi-s3c64xx.c b/drivers/spi/spi-s3c64xx.c
> >> index ad93231..b770f88 100644
> >> --- a/drivers/spi/spi-s3c64xx.c
> >> +++ b/drivers/spi/spi-s3c64xx.c
> >> @@ -997,25 +997,30 @@ static irqreturn_t s3c64xx_spi_irq(int irq,
> >> void *data)>> 
> >>  {
> >>  
> >>       struct s3c64xx_spi_driver_data *sdd = data;
> >>       struct spi_master *spi = sdd->master;
> >> 
> >> -     unsigned int val;
> >> +     unsigned int val, clr = 0;
> >> 
> >> -     val = readl(sdd->regs + S3C64XX_SPI_PENDING_CLR);
> >> +     val = readl(sdd->regs + S3C64XX_SPI_STATUS);
> >> 
> >> -     val &= S3C64XX_SPI_PND_RX_OVERRUN_CLR |
> >> -             S3C64XX_SPI_PND_RX_UNDERRUN_CLR |
> >> -             S3C64XX_SPI_PND_TX_OVERRUN_CLR |
> >> -             S3C64XX_SPI_PND_TX_UNDERRUN_CLR;
> >> -
> >> -     writel(val, sdd->regs + S3C64XX_SPI_PENDING_CLR);
> >> -
> >> -     if (val & S3C64XX_SPI_PND_RX_OVERRUN_CLR)
> >> +     if (val & S3C64XX_SPI_ST_RX_OVERRUN_ERR) {
> >> +             clr = S3C64XX_SPI_PND_RX_OVERRUN_CLR;
> >> 
> >>               dev_err(&spi->dev, "RX overrun\n");
> >> 
> >> -     if (val & S3C64XX_SPI_PND_RX_UNDERRUN_CLR)
> >> +     }
> >> +     if (val & S3C64XX_SPI_ST_RX_UNDERRUN_ERR) {
> >> +             clr |= S3C64XX_SPI_PND_RX_UNDERRUN_CLR;
> >> 
> >>               dev_err(&spi->dev, "RX underrun\n");
> >> 
> >> -     if (val & S3C64XX_SPI_PND_TX_OVERRUN_CLR)
> >> +     }
> >> +     if (val & S3C64XX_SPI_ST_TX_OVERRUN_ERR) {
> >> +             clr |= S3C64XX_SPI_PND_TX_OVERRUN_CLR;
> >> 
> >>               dev_err(&spi->dev, "TX overrun\n");
> >> 
> >> -     if (val & S3C64XX_SPI_PND_TX_UNDERRUN_CLR)
> >> +     }
> >> +     if (val & S3C64XX_SPI_ST_TX_UNDERRUN_ERR) {
> >> +             clr |= S3C64XX_SPI_PND_TX_UNDERRUN_CLR;
> >> 
> >>               dev_err(&spi->dev, "TX underrun\n");
> >> 
> >> +     }
> >> +
> >> +     /* Clear the pending irq by setting and then clearing it */
> >> +     writel(clr, sdd->regs + S3C64XX_SPI_PENDING_CLR);
> >> +     writel(clr & ~clr, sdd->regs + S3C64XX_SPI_PENDING_CLR);
> > 
> > Wait, what?  clr & ~clr == 0   Always.  What are you actually trying
> > to do here?
> The user manual says, wirting 1 to the pending clear register clears
> the interrupt (its not auto clear to 0). so i need to explicitly reset
> those bits thats what the 2nd write does

I have looked through user's manuals of different Samsung SoCs. All of 
them said that writing 1 to a bit clears the corresponding interrupt, but 
none of them contain any note that it must be manually cleared to 0.

In addition the expression

clr & ~clr

makes no sense, because it is equal to 0.

If you really need to clear those bits manually (and I don't think so), 
you should replace this expression with 0.

Best regards,
-- 
Tomasz Figa
Samsung Poland R&D Center
SW Solution Development, Linux Platform

Re: [PATCH 1/4] spi: s3c64xx: modified error interrupt handling and init

[Girish KS]

On Thu, Feb 7, 2013 at 3:09 AM, Tomasz Figa <t.figa@samsung.com> wrote:
> Hi Girish,
>
> On Wednesday 06 of February 2013 12:12:29 Girish KS wrote:
>> On Wed, Feb 6, 2013 at 2:26 AM, Grant Likely <grant.likely@secretlab.ca>
> wrote:
>> > On Tue,  5 Feb 2013 15:09:41 -0800, Girish K S
> <girishks2000@gmail.com> wrote:
>> >> The status of the interrupt is available in the status register,
>> >> so reading the clear pending register and writing back the same
>> >> value will not actually clear the pending interrupts. This patch
>> >> modifies the interrupt handler to read the status register and
>> >> clear the corresponding pending bit in the clear pending register.
>> >>
>> >> Modified the hwInit function to clear all the pending interrupts.
>> >>
>> >> Signed-off-by: Girish K S <ks.giri@samsung.com>
>> >> ---
>> >>
>> >>  drivers/spi/spi-s3c64xx.c |   41
>> >>  +++++++++++++++++++++++++---------------- 1 file changed, 25
>> >>  insertions(+), 16 deletions(-)
>> >>
>> >> diff --git a/drivers/spi/spi-s3c64xx.c b/drivers/spi/spi-s3c64xx.c
>> >> index ad93231..b770f88 100644
>> >> --- a/drivers/spi/spi-s3c64xx.c
>> >> +++ b/drivers/spi/spi-s3c64xx.c
>> >> @@ -997,25 +997,30 @@ static irqreturn_t s3c64xx_spi_irq(int irq,
>> >> void *data)>>
>> >>  {
>> >>
>> >>       struct s3c64xx_spi_driver_data *sdd = data;
>> >>       struct spi_master *spi = sdd->master;
>> >>
>> >> -     unsigned int val;
>> >> +     unsigned int val, clr = 0;
>> >>
>> >> -     val = readl(sdd->regs + S3C64XX_SPI_PENDING_CLR);
>> >> +     val = readl(sdd->regs + S3C64XX_SPI_STATUS);
>> >>
>> >> -     val &= S3C64XX_SPI_PND_RX_OVERRUN_CLR |
>> >> -             S3C64XX_SPI_PND_RX_UNDERRUN_CLR |
>> >> -             S3C64XX_SPI_PND_TX_OVERRUN_CLR |
>> >> -             S3C64XX_SPI_PND_TX_UNDERRUN_CLR;
>> >> -
>> >> -     writel(val, sdd->regs + S3C64XX_SPI_PENDING_CLR);
>> >> -
>> >> -     if (val & S3C64XX_SPI_PND_RX_OVERRUN_CLR)
>> >> +     if (val & S3C64XX_SPI_ST_RX_OVERRUN_ERR) {
>> >> +             clr = S3C64XX_SPI_PND_RX_OVERRUN_CLR;
>> >>
>> >>               dev_err(&spi->dev, "RX overrun\n");
>> >>
>> >> -     if (val & S3C64XX_SPI_PND_RX_UNDERRUN_CLR)
>> >> +     }
>> >> +     if (val & S3C64XX_SPI_ST_RX_UNDERRUN_ERR) {
>> >> +             clr |= S3C64XX_SPI_PND_RX_UNDERRUN_CLR;
>> >>
>> >>               dev_err(&spi->dev, "RX underrun\n");
>> >>
>> >> -     if (val & S3C64XX_SPI_PND_TX_OVERRUN_CLR)
>> >> +     }
>> >> +     if (val & S3C64XX_SPI_ST_TX_OVERRUN_ERR) {
>> >> +             clr |= S3C64XX_SPI_PND_TX_OVERRUN_CLR;
>> >>
>> >>               dev_err(&spi->dev, "TX overrun\n");
>> >>
>> >> -     if (val & S3C64XX_SPI_PND_TX_UNDERRUN_CLR)
>> >> +     }
>> >> +     if (val & S3C64XX_SPI_ST_TX_UNDERRUN_ERR) {
>> >> +             clr |= S3C64XX_SPI_PND_TX_UNDERRUN_CLR;
>> >>
>> >>               dev_err(&spi->dev, "TX underrun\n");
>> >>
>> >> +     }
>> >> +
>> >> +     /* Clear the pending irq by setting and then clearing it */
>> >> +     writel(clr, sdd->regs + S3C64XX_SPI_PENDING_CLR);
>> >> +     writel(clr & ~clr, sdd->regs + S3C64XX_SPI_PENDING_CLR);
>> >
>> > Wait, what?  clr & ~clr == 0   Always.  What are you actually trying
>> > to do here?
>> The user manual says, wirting 1 to the pending clear register clears
>> the interrupt (its not auto clear to 0). so i need to explicitly reset
>> those bits thats what the 2nd write does
>
> I have looked through user's manuals of different Samsung SoCs. All of
> them said that writing 1 to a bit clears the corresponding interrupt, but
> none of them contain any note that it must be manually cleared to 0.
What i meant was the clear pending bit will not clear automatically.
When I set the
clear pending bit, it remains set. This is a problem for the next
interrupt cycle.
> In addition the expression
>
> clr & ~clr
>
> makes no sense, because it is equal to 0.
It makes sense, because we are not disturbing the interrupt pending
bit at position 0, which is a trailing clr bit.
>
> If you really need to clear those bits manually (and I don't think so),
> you should replace this expression with 0.
since we are not enabling (was not enabled in previous implementation)
trailing byte interrupt, and not handling the same in handler
we cannot write 0.

>
> Best regards,
> --
> Tomasz Figa
> Samsung Poland R&D Center
> SW Solution Development, Linux Platform
>

Re: [PATCH 1/4] spi: s3c64xx: modified error interrupt handling and init

[Tomasz Figa]

On Thursday 07 of February 2013 09:46:58 Girish KS wrote:
> On Thu, Feb 7, 2013 at 3:09 AM, Tomasz Figa <t.figa@samsung.com> wrote:
> > Hi Girish,
> > 
> > On Wednesday 06 of February 2013 12:12:29 Girish KS wrote:
> >> On Wed, Feb 6, 2013 at 2:26 AM, Grant Likely
> >> <grant.likely@secretlab.ca>> 
> > wrote:
> >> > On Tue,  5 Feb 2013 15:09:41 -0800, Girish K S
> > 
> > <girishks2000@gmail.com> wrote:
> >> >> The status of the interrupt is available in the status register,
> >> >> so reading the clear pending register and writing back the same
> >> >> value will not actually clear the pending interrupts. This patch
> >> >> modifies the interrupt handler to read the status register and
> >> >> clear the corresponding pending bit in the clear pending register.
> >> >> 
> >> >> Modified the hwInit function to clear all the pending interrupts.
> >> >> 
> >> >> Signed-off-by: Girish K S <ks.giri@samsung.com>
> >> >> ---
> >> >> 
> >> >>  drivers/spi/spi-s3c64xx.c |   41
> >> >>  +++++++++++++++++++++++++---------------- 1 file changed, 25
> >> >>  insertions(+), 16 deletions(-)
> >> >> 
> >> >> diff --git a/drivers/spi/spi-s3c64xx.c b/drivers/spi/spi-s3c64xx.c
> >> >> index ad93231..b770f88 100644
> >> >> --- a/drivers/spi/spi-s3c64xx.c
> >> >> +++ b/drivers/spi/spi-s3c64xx.c
> >> >> @@ -997,25 +997,30 @@ static irqreturn_t s3c64xx_spi_irq(int irq,
> >> >> void *data)>>
> >> >> 
> >> >>  {
> >> >>  
> >> >>       struct s3c64xx_spi_driver_data *sdd = data;
> >> >>       struct spi_master *spi = sdd->master;
> >> >> 
> >> >> -     unsigned int val;
> >> >> +     unsigned int val, clr = 0;
> >> >> 
> >> >> -     val = readl(sdd->regs + S3C64XX_SPI_PENDING_CLR);
> >> >> +     val = readl(sdd->regs + S3C64XX_SPI_STATUS);
> >> >> 
> >> >> -     val &= S3C64XX_SPI_PND_RX_OVERRUN_CLR |
> >> >> -             S3C64XX_SPI_PND_RX_UNDERRUN_CLR |
> >> >> -             S3C64XX_SPI_PND_TX_OVERRUN_CLR |
> >> >> -             S3C64XX_SPI_PND_TX_UNDERRUN_CLR;
> >> >> -
> >> >> -     writel(val, sdd->regs + S3C64XX_SPI_PENDING_CLR);
> >> >> -
> >> >> -     if (val & S3C64XX_SPI_PND_RX_OVERRUN_CLR)
> >> >> +     if (val & S3C64XX_SPI_ST_RX_OVERRUN_ERR) {
> >> >> +             clr = S3C64XX_SPI_PND_RX_OVERRUN_CLR;
> >> >> 
> >> >>               dev_err(&spi->dev, "RX overrun\n");
> >> >> 
> >> >> -     if (val & S3C64XX_SPI_PND_RX_UNDERRUN_CLR)
> >> >> +     }
> >> >> +     if (val & S3C64XX_SPI_ST_RX_UNDERRUN_ERR) {
> >> >> +             clr |= S3C64XX_SPI_PND_RX_UNDERRUN_CLR;
> >> >> 
> >> >>               dev_err(&spi->dev, "RX underrun\n");
> >> >> 
> >> >> -     if (val & S3C64XX_SPI_PND_TX_OVERRUN_CLR)
> >> >> +     }
> >> >> +     if (val & S3C64XX_SPI_ST_TX_OVERRUN_ERR) {
> >> >> +             clr |= S3C64XX_SPI_PND_TX_OVERRUN_CLR;
> >> >> 
> >> >>               dev_err(&spi->dev, "TX overrun\n");
> >> >> 
> >> >> -     if (val & S3C64XX_SPI_PND_TX_UNDERRUN_CLR)
> >> >> +     }
> >> >> +     if (val & S3C64XX_SPI_ST_TX_UNDERRUN_ERR) {
> >> >> +             clr |= S3C64XX_SPI_PND_TX_UNDERRUN_CLR;
> >> >> 
> >> >>               dev_err(&spi->dev, "TX underrun\n");
> >> >> 
> >> >> +     }
> >> >> +
> >> >> +     /* Clear the pending irq by setting and then clearing it */
> >> >> +     writel(clr, sdd->regs + S3C64XX_SPI_PENDING_CLR);
> >> >> +     writel(clr & ~clr, sdd->regs + S3C64XX_SPI_PENDING_CLR);
> >> > 
> >> > Wait, what?  clr & ~clr == 0   Always.  What are you actually
> >> > trying
> >> > to do here?
> >> 
> >> The user manual says, wirting 1 to the pending clear register clears
> >> the interrupt (its not auto clear to 0). so i need to explicitly
> >> reset
> >> those bits thats what the 2nd write does
> > 
> > I have looked through user's manuals of different Samsung SoCs. All of
> > them said that writing 1 to a bit clears the corresponding interrupt,
> > but none of them contain any note that it must be manually cleared to
> > 0.
> What i meant was the clear pending bit will not clear automatically.
> When I set the
> clear pending bit, it remains set. This is a problem for the next
> interrupt cycle.

How did you check that it does not clear automatically?

> > In addition the expression
> > 
> > clr & ~clr
> > 
> > makes no sense, because it is equal to 0.
> 
> It makes sense, because we are not disturbing the interrupt pending
> bit at position 0, which is a trailing clr bit.

You either seem to misunderstand the problem I'm mentioning or not 
understanding it at all.

If you take a variable named clr, no matter what value it is set to, and 
you AND it with bitwise negation of the same variable, you will get 0.

See on this example:

Bits:     7 | 6 | 5 | 4 | 3 | 2 | 1 | 0
         -------------------------------
Values:   1 | 1 | 0 | 0 | 1 | 0 | 0 | 1
         -------------------------------
Negation: 0 | 0 | 1 | 1 | 0 | 1 | 1 | 0
         -------------------------------
AND:      0 | 0 | 0 | 0 | 0 | 0 | 0 | 0

Now, can you see that (clr & ~clr) is the same as (0)?

Best regards,
Tomasz

Re: [PATCH 1/4] spi: s3c64xx: modified error interrupt handling and init

[Girish KS]

On Fri, Feb 8, 2013 at 12:33 AM, Tomasz Figa <tomasz.figa@gmail.com> wrote:
> On Thursday 07 of February 2013 09:46:58 Girish KS wrote:
>> On Thu, Feb 7, 2013 at 3:09 AM, Tomasz Figa <t.figa@samsung.com> wrote:
>> > Hi Girish,
>> >
>> > On Wednesday 06 of February 2013 12:12:29 Girish KS wrote:
>> >> On Wed, Feb 6, 2013 at 2:26 AM, Grant Likely
>> >> <grant.likely@secretlab.ca>>
>> > wrote:
>> >> > On Tue,  5 Feb 2013 15:09:41 -0800, Girish K S
>> >
>> > <girishks2000@gmail.com> wrote:
>> >> >> The status of the interrupt is available in the status register,
>> >> >> so reading the clear pending register and writing back the same
>> >> >> value will not actually clear the pending interrupts. This patch
>> >> >> modifies the interrupt handler to read the status register and
>> >> >> clear the corresponding pending bit in the clear pending register.
>> >> >>
>> >> >> Modified the hwInit function to clear all the pending interrupts.
>> >> >>
>> >> >> Signed-off-by: Girish K S <ks.giri@samsung.com>
>> >> >> ---
>> >> >>
>> >> >>  drivers/spi/spi-s3c64xx.c |   41
>> >> >>  +++++++++++++++++++++++++---------------- 1 file changed, 25
>> >> >>  insertions(+), 16 deletions(-)
>> >> >>
>> >> >> diff --git a/drivers/spi/spi-s3c64xx.c b/drivers/spi/spi-s3c64xx.c
>> >> >> index ad93231..b770f88 100644
>> >> >> --- a/drivers/spi/spi-s3c64xx.c
>> >> >> +++ b/drivers/spi/spi-s3c64xx.c
>> >> >> @@ -997,25 +997,30 @@ static irqreturn_t s3c64xx_spi_irq(int irq,
>> >> >> void *data)>>
>> >> >>
>> >> >>  {
>> >> >>
>> >> >>       struct s3c64xx_spi_driver_data *sdd = data;
>> >> >>       struct spi_master *spi = sdd->master;
>> >> >>
>> >> >> -     unsigned int val;
>> >> >> +     unsigned int val, clr = 0;
>> >> >>
>> >> >> -     val = readl(sdd->regs + S3C64XX_SPI_PENDING_CLR);
>> >> >> +     val = readl(sdd->regs + S3C64XX_SPI_STATUS);
>> >> >>
>> >> >> -     val &= S3C64XX_SPI_PND_RX_OVERRUN_CLR |
>> >> >> -             S3C64XX_SPI_PND_RX_UNDERRUN_CLR |
>> >> >> -             S3C64XX_SPI_PND_TX_OVERRUN_CLR |
>> >> >> -             S3C64XX_SPI_PND_TX_UNDERRUN_CLR;
>> >> >> -
>> >> >> -     writel(val, sdd->regs + S3C64XX_SPI_PENDING_CLR);
>> >> >> -
>> >> >> -     if (val & S3C64XX_SPI_PND_RX_OVERRUN_CLR)
>> >> >> +     if (val & S3C64XX_SPI_ST_RX_OVERRUN_ERR) {
>> >> >> +             clr = S3C64XX_SPI_PND_RX_OVERRUN_CLR;
>> >> >>
>> >> >>               dev_err(&spi->dev, "RX overrun\n");
>> >> >>
>> >> >> -     if (val & S3C64XX_SPI_PND_RX_UNDERRUN_CLR)
>> >> >> +     }
>> >> >> +     if (val & S3C64XX_SPI_ST_RX_UNDERRUN_ERR) {
>> >> >> +             clr |= S3C64XX_SPI_PND_RX_UNDERRUN_CLR;
>> >> >>
>> >> >>               dev_err(&spi->dev, "RX underrun\n");
>> >> >>
>> >> >> -     if (val & S3C64XX_SPI_PND_TX_OVERRUN_CLR)
>> >> >> +     }
>> >> >> +     if (val & S3C64XX_SPI_ST_TX_OVERRUN_ERR) {
>> >> >> +             clr |= S3C64XX_SPI_PND_TX_OVERRUN_CLR;
>> >> >>
>> >> >>               dev_err(&spi->dev, "TX overrun\n");
>> >> >>
>> >> >> -     if (val & S3C64XX_SPI_PND_TX_UNDERRUN_CLR)
>> >> >> +     }
>> >> >> +     if (val & S3C64XX_SPI_ST_TX_UNDERRUN_ERR) {
>> >> >> +             clr |= S3C64XX_SPI_PND_TX_UNDERRUN_CLR;
>> >> >>
>> >> >>               dev_err(&spi->dev, "TX underrun\n");
>> >> >>
>> >> >> +     }
>> >> >> +
>> >> >> +     /* Clear the pending irq by setting and then clearing it */
>> >> >> +     writel(clr, sdd->regs + S3C64XX_SPI_PENDING_CLR);
>> >> >> +     writel(clr & ~clr, sdd->regs + S3C64XX_SPI_PENDING_CLR);
>> >> >
>> >> > Wait, what?  clr & ~clr == 0   Always.  What are you actually
>> >> > trying
>> >> > to do here?
>> >>
>> >> The user manual says, wirting 1 to the pending clear register clears
>> >> the interrupt (its not auto clear to 0). so i need to explicitly
>> >> reset
>> >> those bits thats what the 2nd write does
>> >
>> > I have looked through user's manuals of different Samsung SoCs. All of
>> > them said that writing 1 to a bit clears the corresponding interrupt,
>> > but none of them contain any note that it must be manually cleared to
>> > 0.
>> What i meant was the clear pending bit will not clear automatically.
>> When I set the
>> clear pending bit, it remains set. This is a problem for the next
>> interrupt cycle.
>
> How did you check that it does not clear automatically?
I checked it with trace32 debugger. Also  confirmed with the IP
validation engineer.
>
>> > In addition the expression
>> >
>> > clr & ~clr
>> >
>> > makes no sense, because it is equal to 0.
>>
>> It makes sense, because we are not disturbing the interrupt pending
>> bit at position 0, which is a trailing clr bit.
>
> You either seem to misunderstand the problem I'm mentioning or not
> understanding it at all.
>
> If you take a variable named clr, no matter what value it is set to, and
> you AND it with bitwise negation of the same variable, you will get 0.
>
> See on this example:
>
> Bits:     7 | 6 | 5 | 4 | 3 | 2 | 1 | 0
>          -------------------------------
> Values:   1 | 1 | 0 | 0 | 1 | 0 | 0 | 1
>          -------------------------------
> Negation: 0 | 0 | 1 | 1 | 0 | 1 | 1 | 0
>          -------------------------------
> AND:      0 | 0 | 0 | 0 | 0 | 0 | 0 | 0
>
> Now, can you see that (clr & ~clr) is the same as (0)?

Already apolozised for the same: will resubmit.

>
> Best regards,
> Tomasz
>

Re: [PATCH 1/4] spi: s3c64xx: modified error interrupt handling and init

[Girish KS]

On Fri, Feb 8, 2013 at 12:58 AM, Girish KS <girishks2000@gmail.com> wrote:
> On Fri, Feb 8, 2013 at 12:33 AM, Tomasz Figa <tomasz.figa@gmail.com> wrote:
>> On Thursday 07 of February 2013 09:46:58 Girish KS wrote:
>>> On Thu, Feb 7, 2013 at 3:09 AM, Tomasz Figa <t.figa@samsung.com> wrote:
>>> > Hi Girish,
>>> >
>>> > On Wednesday 06 of February 2013 12:12:29 Girish KS wrote:
>>> >> On Wed, Feb 6, 2013 at 2:26 AM, Grant Likely
>>> >> <grant.likely@secretlab.ca>>
>>> > wrote:
>>> >> > On Tue,  5 Feb 2013 15:09:41 -0800, Girish K S
>>> >
>>> > <girishks2000@gmail.com> wrote:
>>> >> >> The status of the interrupt is available in the status register,
>>> >> >> so reading the clear pending register and writing back the same
>>> >> >> value will not actually clear the pending interrupts. This patch
>>> >> >> modifies the interrupt handler to read the status register and
>>> >> >> clear the corresponding pending bit in the clear pending register.
>>> >> >>
>>> >> >> Modified the hwInit function to clear all the pending interrupts.
>>> >> >>
>>> >> >> Signed-off-by: Girish K S <ks.giri@samsung.com>
>>> >> >> ---
>>> >> >>
>>> >> >>  drivers/spi/spi-s3c64xx.c |   41
>>> >> >>  +++++++++++++++++++++++++---------------- 1 file changed, 25
>>> >> >>  insertions(+), 16 deletions(-)
>>> >> >>
>>> >> >> diff --git a/drivers/spi/spi-s3c64xx.c b/drivers/spi/spi-s3c64xx.c
>>> >> >> index ad93231..b770f88 100644
>>> >> >> --- a/drivers/spi/spi-s3c64xx.c
>>> >> >> +++ b/drivers/spi/spi-s3c64xx.c
>>> >> >> @@ -997,25 +997,30 @@ static irqreturn_t s3c64xx_spi_irq(int irq,
>>> >> >> void *data)>>
>>> >> >>
>>> >> >>  {
>>> >> >>
>>> >> >>       struct s3c64xx_spi_driver_data *sdd = data;
>>> >> >>       struct spi_master *spi = sdd->master;
>>> >> >>
>>> >> >> -     unsigned int val;
>>> >> >> +     unsigned int val, clr = 0;
>>> >> >>
>>> >> >> -     val = readl(sdd->regs + S3C64XX_SPI_PENDING_CLR);
>>> >> >> +     val = readl(sdd->regs + S3C64XX_SPI_STATUS);
>>> >> >>
>>> >> >> -     val &= S3C64XX_SPI_PND_RX_OVERRUN_CLR |
>>> >> >> -             S3C64XX_SPI_PND_RX_UNDERRUN_CLR |
>>> >> >> -             S3C64XX_SPI_PND_TX_OVERRUN_CLR |
>>> >> >> -             S3C64XX_SPI_PND_TX_UNDERRUN_CLR;
>>> >> >> -
>>> >> >> -     writel(val, sdd->regs + S3C64XX_SPI_PENDING_CLR);
>>> >> >> -
>>> >> >> -     if (val & S3C64XX_SPI_PND_RX_OVERRUN_CLR)
>>> >> >> +     if (val & S3C64XX_SPI_ST_RX_OVERRUN_ERR) {
>>> >> >> +             clr = S3C64XX_SPI_PND_RX_OVERRUN_CLR;
>>> >> >>
>>> >> >>               dev_err(&spi->dev, "RX overrun\n");
>>> >> >>
>>> >> >> -     if (val & S3C64XX_SPI_PND_RX_UNDERRUN_CLR)
>>> >> >> +     }
>>> >> >> +     if (val & S3C64XX_SPI_ST_RX_UNDERRUN_ERR) {
>>> >> >> +             clr |= S3C64XX_SPI_PND_RX_UNDERRUN_CLR;
>>> >> >>
>>> >> >>               dev_err(&spi->dev, "RX underrun\n");
>>> >> >>
>>> >> >> -     if (val & S3C64XX_SPI_PND_TX_OVERRUN_CLR)
>>> >> >> +     }
>>> >> >> +     if (val & S3C64XX_SPI_ST_TX_OVERRUN_ERR) {
>>> >> >> +             clr |= S3C64XX_SPI_PND_TX_OVERRUN_CLR;
>>> >> >>
>>> >> >>               dev_err(&spi->dev, "TX overrun\n");
>>> >> >>
>>> >> >> -     if (val & S3C64XX_SPI_PND_TX_UNDERRUN_CLR)
>>> >> >> +     }
>>> >> >> +     if (val & S3C64XX_SPI_ST_TX_UNDERRUN_ERR) {
>>> >> >> +             clr |= S3C64XX_SPI_PND_TX_UNDERRUN_CLR;
>>> >> >>
>>> >> >>               dev_err(&spi->dev, "TX underrun\n");
>>> >> >>
>>> >> >> +     }
>>> >> >> +
>>> >> >> +     /* Clear the pending irq by setting and then clearing it */
>>> >> >> +     writel(clr, sdd->regs + S3C64XX_SPI_PENDING_CLR);
>>> >> >> +     writel(clr & ~clr, sdd->regs + S3C64XX_SPI_PENDING_CLR);
>>> >> >
>>> >> > Wait, what?  clr & ~clr == 0   Always.  What are you actually
>>> >> > trying
>>> >> > to do here?
>>> >>
>>> >> The user manual says, wirting 1 to the pending clear register clears
>>> >> the interrupt (its not auto clear to 0). so i need to explicitly
>>> >> reset
>>> >> those bits thats what the 2nd write does
>>> >
>>> > I have looked through user's manuals of different Samsung SoCs. All of
>>> > them said that writing 1 to a bit clears the corresponding interrupt,
>>> > but none of them contain any note that it must be manually cleared to
>>> > 0.
>>> What i meant was the clear pending bit will not clear automatically.
>>> When I set the
>>> clear pending bit, it remains set. This is a problem for the next
>>> interrupt cycle.
>>
>> How did you check that it does not clear automatically?
> I checked it with trace32 debugger. Also  confirmed with the IP
> validation engineer.

To be more clear. When i open the trace32 memory window (start with
SPI register's base address), the RX under run bit is set in the
status register. coz the debugger tries to read from the RX data
register. At that time i forcibly set the clear pending register. Once
i set it. It just remains set until i reset it. I consulted the
validation engineer after i saw this behaviour.

Thank you Thomas, Grant and Mark for you time.

>>
>>> > In addition the expression
>>> >
>>> > clr & ~clr
>>> >
>>> > makes no sense, because it is equal to 0.
>>>
>>> It makes sense, because we are not disturbing the interrupt pending
>>> bit at position 0, which is a trailing clr bit.
>>
>> You either seem to misunderstand the problem I'm mentioning or not
>> understanding it at all.
>>
>> If you take a variable named clr, no matter what value it is set to, and
>> you AND it with bitwise negation of the same variable, you will get 0.
>>
>> See on this example:
>>
>> Bits:     7 | 6 | 5 | 4 | 3 | 2 | 1 | 0
>>          -------------------------------
>> Values:   1 | 1 | 0 | 0 | 1 | 0 | 0 | 1
>>          -------------------------------
>> Negation: 0 | 0 | 1 | 1 | 0 | 1 | 1 | 0
>>          -------------------------------
>> AND:      0 | 0 | 0 | 0 | 0 | 0 | 0 | 0
>>
>> Now, can you see that (clr & ~clr) is the same as (0)?
>
> Already apolozised for the same: will resubmit.
>
>>
>> Best regards,
>> Tomasz
>>

[PATCH 2/4] spi: s3c64xx: added support for polling mode

[Girish K S]

The 64xx spi driver supports partial polling mode.
Only the last chunk of the transfer length is transferred
or recieved in polling mode.

Some SoC's that adopt this controller might not have have dma
interface. This patch adds support for complete polling mode
and gives flexibity for the user to select poll/dma mode.

Signed-off-by: Girish K S <ks.giri@samsung.com>
---
 drivers/spi/spi-s3c64xx.c |   65 +++++++++++++++++++++------------------------
 1 file changed, 30 insertions(+), 35 deletions(-)

diff --git a/drivers/spi/spi-s3c64xx.c b/drivers/spi/spi-s3c64xx.c
index b770f88..90770bd 100644
--- a/drivers/spi/spi-s3c64xx.c
+++ b/drivers/spi/spi-s3c64xx.c
@@ -35,6 +35,7 @@
 #include <linux/platform_data/spi-s3c64xx.h>
 
 #define MAX_SPI_PORTS		3
+#define S3C64XX_SPI_QUIRK_POLL		(1 << 0)
 
 /* Registers and bit-fields */
 
@@ -126,6 +127,7 @@
 #define S3C64XX_SPI_TRAILCNT		S3C64XX_SPI_MAX_TRAILCNT
 
 #define msecs_to_loops(t) (loops_per_jiffy / 1000 * HZ * t)
+#define is_polling(x)	(x->port_conf->quirks & S3C64XX_SPI_QUIRK_POLL)
 
 #define RXBUSY    (1<<2)
 #define TXBUSY    (1<<3)
@@ -155,6 +157,7 @@ struct s3c64xx_spi_port_config {
 	int	fifo_lvl_mask[MAX_SPI_PORTS];
 	int	rx_lvl_offset;
 	int	tx_st_done;
+	int	quirks;
 	bool	high_speed;
 	bool	clk_from_cmu;
 };
@@ -345,19 +348,7 @@ static void enable_datapath(struct s3c64xx_spi_driver_data *sdd,
 
 	chcfg = readl(regs + S3C64XX_SPI_CH_CFG);
 	chcfg &= ~S3C64XX_SPI_CH_TXCH_ON;
-
-	if (dma_mode) {
 		chcfg &= ~S3C64XX_SPI_CH_RXCH_ON;
-	} else {
-		/* Always shift in data in FIFO, even if xfer is Tx only,
-		 * this helps setting PCKT_CNT value for generating clocks
-		 * as exactly needed.
-		 */
-		chcfg |= S3C64XX_SPI_CH_RXCH_ON;
-		writel(((xfer->len * 8 / sdd->cur_bpw) & 0xffff)
-					| S3C64XX_SPI_PACKET_CNT_EN,
-					regs + S3C64XX_SPI_PACKET_CNT);
-	}
 
 	if (xfer->tx_buf != NULL) {
 		sdd->state |= TXBUSY;
@@ -385,6 +376,10 @@ static void enable_datapath(struct s3c64xx_spi_driver_data *sdd,
 
 	if (xfer->rx_buf != NULL) {
 		sdd->state |= RXBUSY;
+		chcfg |= S3C64XX_SPI_CH_RXCH_ON;
+		writel(((xfer->len * 8 / sdd->cur_bpw) & 0xffff)
+				| S3C64XX_SPI_PACKET_CNT_EN,
+				regs + S3C64XX_SPI_PACKET_CNT);
 
 		if (sdd->port_conf->high_speed && sdd->cur_speed >= 30000000UL
 					&& !(sdd->cur_mode & SPI_CPHA))
@@ -392,10 +387,6 @@ static void enable_datapath(struct s3c64xx_spi_driver_data *sdd,
 
 		if (dma_mode) {
 			modecfg |= S3C64XX_SPI_MODE_RXDMA_ON;
-			chcfg |= S3C64XX_SPI_CH_RXCH_ON;
-			writel(((xfer->len * 8 / sdd->cur_bpw) & 0xffff)
-					| S3C64XX_SPI_PACKET_CNT_EN,
-					regs + S3C64XX_SPI_PACKET_CNT);
 			prepare_dma(&sdd->rx_dma, xfer->len, xfer->rx_dma);
 		}
 	}
@@ -421,6 +412,9 @@ static inline void enable_cs(struct s3c64xx_spi_driver_data *sdd,
 
 	cs = spi->controller_data;
 	gpio_set_value(cs->line, spi->mode & SPI_CS_HIGH ? 1 : 0);
+
+	/* Start the signals */
+	writel(0, sdd->regs + S3C64XX_SPI_SLAVE_SEL);
 }
 
 static int wait_for_xfer(struct s3c64xx_spi_driver_data *sdd,
@@ -445,12 +439,12 @@ static int wait_for_xfer(struct s3c64xx_spi_driver_data *sdd,
 		} while (RX_FIFO_LVL(status, sdd) < xfer->len && --val);
 	}
 
-	if (!val)
-		return -EIO;
-
 	if (dma_mode) {
 		u32 status;
 
+		if (!val)
+			return -EIO;
+
 		/*
 		 * DmaTx returns after simply writing data in the FIFO,
 		 * w/o waiting for real transmission on the bus to finish.
@@ -480,16 +474,19 @@ static int wait_for_xfer(struct s3c64xx_spi_driver_data *sdd,
 
 		switch (sdd->cur_bpw) {
 		case 32:
-			ioread32_rep(regs + S3C64XX_SPI_RX_DATA,
-				xfer->rx_buf, xfer->len / 4);
+			for (val = 0; val < (xfer->len / 4); val++)
+				*((u32 *)xfer->rx_buf + val) =
+					ioread32(regs + S3C64XX_SPI_RX_DATA);
 			break;
 		case 16:
-			ioread16_rep(regs + S3C64XX_SPI_RX_DATA,
-				xfer->rx_buf, xfer->len / 2);
+			for (val = 0; val < (xfer->len / 2); val++)
+				*((u16 *)xfer->rx_buf + val) =
+					ioread16(regs + S3C64XX_SPI_RX_DATA);
 			break;
 		default:
-			ioread8_rep(regs + S3C64XX_SPI_RX_DATA,
-				xfer->rx_buf, xfer->len);
+			for (val = 0; val < xfer->len; val++)
+				*((u8 *)xfer->rx_buf + val) =
+					ioread8(regs + S3C64XX_SPI_RX_DATA);
 			break;
 		}
 		sdd->state &= ~RXBUSY;
@@ -507,6 +504,10 @@ static inline void disable_cs(struct s3c64xx_spi_driver_data *sdd,
 		sdd->tgl_spi = NULL;
 
 	gpio_set_value(cs->line, spi->mode & SPI_CS_HIGH ? 0 : 1);
+
+	/* Quiese the signals */
+	writel(S3C64XX_SPI_SLAVE_SIG_INACT,
+	sdd->regs + S3C64XX_SPI_SLAVE_SEL);
 }
 
 static void s3c64xx_spi_config(struct s3c64xx_spi_driver_data *sdd)
@@ -588,7 +589,7 @@ static int s3c64xx_spi_map_mssg(struct s3c64xx_spi_driver_data *sdd,
 	struct device *dev = &sdd->pdev->dev;
 	struct spi_transfer *xfer;
 
-	if (msg->is_dma_mapped)
+	if (is_polling(sdd) || msg->is_dma_mapped)
 		return 0;
 
 	/* First mark all xfer unmapped */
@@ -637,7 +638,7 @@ static void s3c64xx_spi_unmap_mssg(struct s3c64xx_spi_driver_data *sdd,
 	struct device *dev = &sdd->pdev->dev;
 	struct spi_transfer *xfer;
 
-	if (msg->is_dma_mapped)
+	if (is_polling(sdd) || msg->is_dma_mapped)
 		return;
 
 	list_for_each_entry(xfer, &msg->transfers, transfer_list) {
@@ -715,7 +716,8 @@ static int s3c64xx_spi_transfer_one_message(struct spi_master *master,
 		}
 
 		/* Polling method for xfers not bigger than FIFO capacity */
-		if (xfer->len <= ((FIFO_LVL_MASK(sdd) >> 1) + 1))
+		if (is_polling(sdd) ||
+			xfer->len <= ((FIFO_LVL_MASK(sdd) >> 1) + 1))
 			use_dma = 0;
 		else
 			use_dma = 1;
@@ -731,17 +733,10 @@ static int s3c64xx_spi_transfer_one_message(struct spi_master *master,
 		/* Slave Select */
 		enable_cs(sdd, spi);
 
-		/* Start the signals */
-		writel(0, sdd->regs + S3C64XX_SPI_SLAVE_SEL);
-
 		spin_unlock_irqrestore(&sdd->lock, flags);
 
 		status = wait_for_xfer(sdd, xfer, use_dma);
 
-		/* Quiese the signals */
-		writel(S3C64XX_SPI_SLAVE_SIG_INACT,
-		       sdd->regs + S3C64XX_SPI_SLAVE_SEL);
-
 		if (status) {
 			dev_err(&spi->dev, "I/O Error: "
 				"rx-%d tx-%d res:rx-%c tx-%c len-%d\n",
-- 
1.7.10.4

Re: [PATCH 2/4] spi: s3c64xx: added support for polling mode

[Grant Likely]

On Tue,  5 Feb 2013 15:09:42 -0800, Girish K S <girishks2000@gmail.com> wrote:
> The 64xx spi driver supports partial polling mode.
> Only the last chunk of the transfer length is transferred
> or recieved in polling mode.
> 
> Some SoC's that adopt this controller might not have have dma
> interface. This patch adds support for complete polling mode
> and gives flexibity for the user to select poll/dma mode.
> 
> Signed-off-by: Girish K S <ks.giri@samsung.com>
> ---
>  drivers/spi/spi-s3c64xx.c |   65 +++++++++++++++++++++------------------------
>  1 file changed, 30 insertions(+), 35 deletions(-)
> 
> diff --git a/drivers/spi/spi-s3c64xx.c b/drivers/spi/spi-s3c64xx.c
> index b770f88..90770bd 100644
> --- a/drivers/spi/spi-s3c64xx.c
> +++ b/drivers/spi/spi-s3c64xx.c
> @@ -345,19 +348,7 @@ static void enable_datapath(struct s3c64xx_spi_driver_data *sdd,
>  
>  	chcfg = readl(regs + S3C64XX_SPI_CH_CFG);
>  	chcfg &= ~S3C64XX_SPI_CH_TXCH_ON;
> -
> -	if (dma_mode) {
>  		chcfg &= ~S3C64XX_SPI_CH_RXCH_ON;
> -	} else {
> -		/* Always shift in data in FIFO, even if xfer is Tx only,
> -		 * this helps setting PCKT_CNT value for generating clocks
> -		 * as exactly needed.
> -		 */
> -		chcfg |= S3C64XX_SPI_CH_RXCH_ON;
> -		writel(((xfer->len * 8 / sdd->cur_bpw) & 0xffff)
> -					| S3C64XX_SPI_PACKET_CNT_EN,
> -					regs + S3C64XX_SPI_PACKET_CNT);
> -	}

The removes a block of code, but leaves the modification of chcfg where
it is without fixing the indentation. I could also use some help
understanding why this particular else block is moved down in the
function.

>  	if (xfer->tx_buf != NULL) {
>  		sdd->state |= TXBUSY;
> @@ -385,6 +376,10 @@ static void enable_datapath(struct s3c64xx_spi_driver_data *sdd,
>  
>  	if (xfer->rx_buf != NULL) {
>  		sdd->state |= RXBUSY;
> +		chcfg |= S3C64XX_SPI_CH_RXCH_ON;
> +		writel(((xfer->len * 8 / sdd->cur_bpw) & 0xffff)
> +				| S3C64XX_SPI_PACKET_CNT_EN,
> +				regs + S3C64XX_SPI_PACKET_CNT);
>  
>  		if (sdd->port_conf->high_speed && sdd->cur_speed >= 30000000UL
>  					&& !(sdd->cur_mode & SPI_CPHA))
> @@ -392,10 +387,6 @@ static void enable_datapath(struct s3c64xx_spi_driver_data *sdd,
>  
>  		if (dma_mode) {
>  			modecfg |= S3C64XX_SPI_MODE_RXDMA_ON;
> -			chcfg |= S3C64XX_SPI_CH_RXCH_ON;
> -			writel(((xfer->len * 8 / sdd->cur_bpw) & 0xffff)
> -					| S3C64XX_SPI_PACKET_CNT_EN,
> -					regs + S3C64XX_SPI_PACKET_CNT);
>  			prepare_dma(&sdd->rx_dma, xfer->len, xfer->rx_dma);
>  		}
>  	}
> @@ -421,6 +412,9 @@ static inline void enable_cs(struct s3c64xx_spi_driver_data *sdd,
>  
>  	cs = spi->controller_data;
>  	gpio_set_value(cs->line, spi->mode & SPI_CS_HIGH ? 1 : 0);
> +
> +	/* Start the signals */
> +	writel(0, sdd->regs + S3C64XX_SPI_SLAVE_SEL);
>  }
>  
>  static int wait_for_xfer(struct s3c64xx_spi_driver_data *sdd,
> @@ -480,16 +474,19 @@ static int wait_for_xfer(struct s3c64xx_spi_driver_data *sdd,
>  
>  		switch (sdd->cur_bpw) {
>  		case 32:
> -			ioread32_rep(regs + S3C64XX_SPI_RX_DATA,
> -				xfer->rx_buf, xfer->len / 4);
> +			for (val = 0; val < (xfer->len / 4); val++)
> +				*((u32 *)xfer->rx_buf + val) =
> +					ioread32(regs + S3C64XX_SPI_RX_DATA);
>  			break;
>  		case 16:
> -			ioread16_rep(regs + S3C64XX_SPI_RX_DATA,
> -				xfer->rx_buf, xfer->len / 2);
> +			for (val = 0; val < (xfer->len / 2); val++)
> +				*((u16 *)xfer->rx_buf + val) =
> +					ioread16(regs + S3C64XX_SPI_RX_DATA);
>  			break;
>  		default:
> -			ioread8_rep(regs + S3C64XX_SPI_RX_DATA,
> -				xfer->rx_buf, xfer->len);
> +			for (val = 0; val < xfer->len; val++)
> +				*((u8 *)xfer->rx_buf + val) =
> +					ioread8(regs + S3C64XX_SPI_RX_DATA);

What are all of the above lines changed? That seems to have nothing to
do with making the driver be albe to do polling transfers. Nor is it
described in the patch description.

I think this patch needs some more work. It certainly need to be
described better, and it appears that some of the changes really should be
in a separate patch.

Re: [PATCH 2/4] spi: s3c64xx: added support for polling mode

[Girish KS]

On Wed, Feb 6, 2013 at 2:35 AM, Grant Likely <grant.likely@secretlab.ca> wrote:
> On Tue,  5 Feb 2013 15:09:42 -0800, Girish K S <girishks2000@gmail.com> wrote:
>> The 64xx spi driver supports partial polling mode.
>> Only the last chunk of the transfer length is transferred
>> or recieved in polling mode.
>>
>> Some SoC's that adopt this controller might not have have dma
>> interface. This patch adds support for complete polling mode
>> and gives flexibity for the user to select poll/dma mode.
>>
>> Signed-off-by: Girish K S <ks.giri@samsung.com>
>> ---
>>  drivers/spi/spi-s3c64xx.c |   65 +++++++++++++++++++++------------------------
>>  1 file changed, 30 insertions(+), 35 deletions(-)
>>
>> diff --git a/drivers/spi/spi-s3c64xx.c b/drivers/spi/spi-s3c64xx.c
>> index b770f88..90770bd 100644
>> --- a/drivers/spi/spi-s3c64xx.c
>> +++ b/drivers/spi/spi-s3c64xx.c
>> @@ -345,19 +348,7 @@ static void enable_datapath(struct s3c64xx_spi_driver_data *sdd,
>>
>>       chcfg = readl(regs + S3C64XX_SPI_CH_CFG);
>>       chcfg &= ~S3C64XX_SPI_CH_TXCH_ON;
>> -
>> -     if (dma_mode) {
>>               chcfg &= ~S3C64XX_SPI_CH_RXCH_ON;
>> -     } else {
>> -             /* Always shift in data in FIFO, even if xfer is Tx only,
>> -              * this helps setting PCKT_CNT value for generating clocks
>> -              * as exactly needed.
>> -              */
>> -             chcfg |= S3C64XX_SPI_CH_RXCH_ON;
>> -             writel(((xfer->len * 8 / sdd->cur_bpw) & 0xffff)
>> -                                     | S3C64XX_SPI_PACKET_CNT_EN,
>> -                                     regs + S3C64XX_SPI_PACKET_CNT);
>> -     }
>
> The removes a block of code, but leaves the modification of chcfg where
> it is without fixing the indentation. I could also use some help
Yes sorry missed that indentation.
> understanding why this particular else block is moved down in the
> function.
The else block is related only to polling mode, so moved it inside the
rx block.
Clock generation and RX is enabled only when there is a valid Rx
buffer to receive the data from FIFO.

>
>>       if (xfer->tx_buf != NULL) {
>>               sdd->state |= TXBUSY;
>> @@ -385,6 +376,10 @@ static void enable_datapath(struct s3c64xx_spi_driver_data *sdd,
>>
>>       if (xfer->rx_buf != NULL) {
>>               sdd->state |= RXBUSY;
>> +             chcfg |= S3C64XX_SPI_CH_RXCH_ON;
>> +             writel(((xfer->len * 8 / sdd->cur_bpw) & 0xffff)
>> +                             | S3C64XX_SPI_PACKET_CNT_EN,
>> +                             regs + S3C64XX_SPI_PACKET_CNT);
>>
>>               if (sdd->port_conf->high_speed && sdd->cur_speed >= 30000000UL
>>                                       && !(sdd->cur_mode & SPI_CPHA))
>> @@ -392,10 +387,6 @@ static void enable_datapath(struct s3c64xx_spi_driver_data *sdd,
>>
>>               if (dma_mode) {
>>                       modecfg |= S3C64XX_SPI_MODE_RXDMA_ON;
>> -                     chcfg |= S3C64XX_SPI_CH_RXCH_ON;
>> -                     writel(((xfer->len * 8 / sdd->cur_bpw) & 0xffff)
>> -                                     | S3C64XX_SPI_PACKET_CNT_EN,
>> -                                     regs + S3C64XX_SPI_PACKET_CNT);
>>                       prepare_dma(&sdd->rx_dma, xfer->len, xfer->rx_dma);
>>               }
>>       }
>> @@ -421,6 +412,9 @@ static inline void enable_cs(struct s3c64xx_spi_driver_data *sdd,
>>
>>       cs = spi->controller_data;
>>       gpio_set_value(cs->line, spi->mode & SPI_CS_HIGH ? 1 : 0);
>> +
>> +     /* Start the signals */
>> +     writel(0, sdd->regs + S3C64XX_SPI_SLAVE_SEL);
>>  }
>>
>>  static int wait_for_xfer(struct s3c64xx_spi_driver_data *sdd,
>> @@ -480,16 +474,19 @@ static int wait_for_xfer(struct s3c64xx_spi_driver_data *sdd,
>>
>>               switch (sdd->cur_bpw) {
>>               case 32:
>> -                     ioread32_rep(regs + S3C64XX_SPI_RX_DATA,
>> -                             xfer->rx_buf, xfer->len / 4);
>> +                     for (val = 0; val < (xfer->len / 4); val++)
>> +                             *((u32 *)xfer->rx_buf + val) =
>> +                                     ioread32(regs + S3C64XX_SPI_RX_DATA);
>>                       break;
>>               case 16:
>> -                     ioread16_rep(regs + S3C64XX_SPI_RX_DATA,
>> -                             xfer->rx_buf, xfer->len / 2);
>> +                     for (val = 0; val < (xfer->len / 2); val++)
>> +                             *((u16 *)xfer->rx_buf + val) =
>> +                                     ioread16(regs + S3C64XX_SPI_RX_DATA);
>>                       break;
>>               default:
>> -                     ioread8_rep(regs + S3C64XX_SPI_RX_DATA,
>> -                             xfer->rx_buf, xfer->len);
>> +                     for (val = 0; val < xfer->len; val++)
>> +                             *((u8 *)xfer->rx_buf + val) =
>> +                                     ioread8(regs + S3C64XX_SPI_RX_DATA);
>
> What are all of the above lines changed? That seems to have nothing to
> do with making the driver be albe to do polling transfers. Nor is it
> described in the patch description.
As per the original implementation, the polling path is taken only for
the last chunk of tarnsfer size. The last chunk is always
<=  all other other previous transfers. so readx_rep can read the FIFO
data successfully for the given transfer size.
But in polling mode, readx_rep would read junk if the transfer size is
more than the threshold limit.
e.g
dd if=/dev/mtd0  of=temp.bin bs=512 count=1
In this case transfer size to receive is 512. The threshold for FIFO
is 128 bytes (half of FIFO depth, i didnt modify this as it is in DMA
transfer also)
Now in the above receive case with repx_read, i always received first
128 valid bytes and all the rest as junk. With the above modification
I just make sure i read out all the data whatever is received 1 by 1.
>
> I think this patch needs some more work. It certainly need to be
> described better, and it appears that some of the changes really should be
> in a separate patch.
sure I will give a clear description of what is done.

[PATCH 3/4] spi: s3c64xx: add gpio quirk for controller

[Girish K S]

This patch adds support for spi controllers with
dedicated clk/miso/mosi/cs pins. It skips the gpio
parsing and initialization for controllers that
have dedicated pins.

Signed-off-by: Girish K S <ks.giri@samsung.com>
---
 drivers/spi/spi-s3c64xx.c |   39 +++++++++++++++++++++++++++++++--------
 1 file changed, 31 insertions(+), 8 deletions(-)

diff --git a/drivers/spi/spi-s3c64xx.c b/drivers/spi/spi-s3c64xx.c
index 90770bd..f06bbee 100644
--- a/drivers/spi/spi-s3c64xx.c
+++ b/drivers/spi/spi-s3c64xx.c
@@ -36,6 +36,7 @@
 
 #define MAX_SPI_PORTS		3
 #define S3C64XX_SPI_QUIRK_POLL		(1 << 0)
+#define S3C64XX_SPI_QUIRK_GPIO		(1 << 1)
 
 /* Registers and bit-fields */
 
@@ -404,14 +405,16 @@ static inline void enable_cs(struct s3c64xx_spi_driver_data *sdd,
 		if (sdd->tgl_spi != spi) { /* if last mssg on diff device */
 			/* Deselect the last toggled device */
 			cs = sdd->tgl_spi->controller_data;
-			gpio_set_value(cs->line,
-				spi->mode & SPI_CS_HIGH ? 0 : 1);
+			if (!(sdd->port_conf->quirks & S3C64XX_SPI_QUIRK_GPIO))
+				gpio_set_value(cs->line,
+					spi->mode & SPI_CS_HIGH ? 0 : 1);
 		}
 		sdd->tgl_spi = NULL;
 	}
 
 	cs = spi->controller_data;
-	gpio_set_value(cs->line, spi->mode & SPI_CS_HIGH ? 1 : 0);
+	if (!(sdd->port_conf->quirks & S3C64XX_SPI_QUIRK_GPIO))
+		gpio_set_value(cs->line, spi->mode & SPI_CS_HIGH ? 1 : 0);
 
 	/* Start the signals */
 	writel(0, sdd->regs + S3C64XX_SPI_SLAVE_SEL);
@@ -503,7 +506,8 @@ static inline void disable_cs(struct s3c64xx_spi_driver_data *sdd,
 	if (sdd->tgl_spi == spi)
 		sdd->tgl_spi = NULL;
 
-	gpio_set_value(cs->line, spi->mode & SPI_CS_HIGH ? 0 : 1);
+	if (!(sdd->port_conf->quirks & S3C64XX_SPI_QUIRK_GPIO))
+		gpio_set_value(cs->line, spi->mode & SPI_CS_HIGH ? 0 : 1);
 
 	/* Quiese the signals */
 	writel(S3C64XX_SPI_SLAVE_SIG_INACT,
@@ -842,7 +846,10 @@ static struct s3c64xx_spi_csinfo *s3c64xx_get_slave_ctrldata(
 		return ERR_PTR(-ENOMEM);
 	}
 
-	cs->line = of_get_named_gpio(data_np, "cs-gpio", 0);
+	/* In case of dedicated cs pin skip the gpio initialization */
+	if (!(sdd->port_conf->quirks & S3C64XX_SPI_QUIRK_GPIO))
+		cs->line = of_get_named_gpio(data_np, "cs-gpio", 0);
+
 	if (!gpio_is_valid(cs->line)) {
 		dev_err(&spi->dev, "chip select gpio is not specified or "
 					"invalid\n");
@@ -883,7 +890,7 @@ static int s3c64xx_spi_setup(struct spi_device *spi)
 		return -ENODEV;
 	}
 
-	if (!spi_get_ctldata(spi)) {
+	if (!(sdd->port_conf->quirks & S3C64XX_SPI_QUIRK_GPIO)) {
 		err = gpio_request_one(cs->line, GPIOF_OUT_INIT_HIGH,
 				       dev_name(&spi->dev));
 		if (err) {
@@ -892,9 +899,11 @@ static int s3c64xx_spi_setup(struct spi_device *spi)
 				cs->line, err);
 			goto err_gpio_req;
 		}
-		spi_set_ctldata(spi, cs);
 	}
 
+	if (!spi_get_ctldata(spi))
+		spi_set_ctldata(spi, cs);
+
 	sci = sdd->cntrlr_info;
 
 	spin_lock_irqsave(&sdd->lock, flags);
@@ -979,8 +988,11 @@ err_gpio_req:
 static void s3c64xx_spi_cleanup(struct spi_device *spi)
 {
 	struct s3c64xx_spi_csinfo *cs = spi_get_ctldata(spi);
+	struct s3c64xx_spi_driver_data *sdd;
+
+	sdd = spi_master_get_devdata(spi->master);
 
-	if (cs) {
+	if (!(sdd->port_conf->quirks & S3C64XX_SPI_QUIRK_GPIO) && cs) {
 		gpio_free(cs->line);
 		if (spi->dev.of_node)
 			kfree(cs);
@@ -1107,6 +1119,13 @@ static int s3c64xx_spi_parse_dt_gpio(struct s3c64xx_spi_driver_data *sdd)
 	struct device *dev = &sdd->pdev->dev;
 	int idx, gpio, ret;
 
+	/*
+	 * If cs is not controlled by gpio, and
+	 * the SoC uses internal dedicated pins
+	 */
+	if (sdd->port_conf->quirks & S3C64XX_SPI_QUIRK_GPIO)
+		return 0;
+
 	/* find gpios for mosi, miso and clock lines */
 	for (idx = 0; idx < 3; idx++) {
 		gpio = of_get_gpio(dev->of_node, idx);
@@ -1133,6 +1152,10 @@ free_gpio:
 static void s3c64xx_spi_dt_gpio_free(struct s3c64xx_spi_driver_data *sdd)
 {
 	unsigned int idx;
+
+	if (sdd->port_conf->quirks & S3C64XX_SPI_QUIRK_GPIO)
+		return;
+
 	for (idx = 0; idx < 3; idx++)
 		gpio_free(sdd->gpios[idx]);
 }
-- 
1.7.10.4

Re: [PATCH 3/4] spi: s3c64xx: add gpio quirk for controller

[Grant Likely]

On Tue,  5 Feb 2013 15:09:43 -0800, Girish K S <girishks2000@gmail.com> wrote:
> This patch adds support for spi controllers with
> dedicated clk/miso/mosi/cs pins. It skips the gpio
> parsing and initialization for controllers that
> have dedicated pins.
> 
> Signed-off-by: Girish K S <ks.giri@samsung.com>

Instead of making this a quirk, you should use the presence/absense of a
cs-gpios property to determin whether or not a gpio is used to
manipulate the CS signal.

Also, are you certain that you want to make this change? Most SPI
controllers are actually unfriendly in that they insist on deasserting the
CS line if the FIFO runs empty. Does this driver correctly keep the CS
signal asserted when there are multiple transfers in a message?

g.

> ---
>  drivers/spi/spi-s3c64xx.c |   39 +++++++++++++++++++++++++++++++--------
>  1 file changed, 31 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/spi/spi-s3c64xx.c b/drivers/spi/spi-s3c64xx.c
> index 90770bd..f06bbee 100644
> --- a/drivers/spi/spi-s3c64xx.c
> +++ b/drivers/spi/spi-s3c64xx.c
> @@ -36,6 +36,7 @@
>  
>  #define MAX_SPI_PORTS		3
>  #define S3C64XX_SPI_QUIRK_POLL		(1 << 0)
> +#define S3C64XX_SPI_QUIRK_GPIO		(1 << 1)
>  
>  /* Registers and bit-fields */
>  
> @@ -404,14 +405,16 @@ static inline void enable_cs(struct s3c64xx_spi_driver_data *sdd,
>  		if (sdd->tgl_spi != spi) { /* if last mssg on diff device */
>  			/* Deselect the last toggled device */
>  			cs = sdd->tgl_spi->controller_data;
> -			gpio_set_value(cs->line,
> -				spi->mode & SPI_CS_HIGH ? 0 : 1);
> +			if (!(sdd->port_conf->quirks & S3C64XX_SPI_QUIRK_GPIO))
> +				gpio_set_value(cs->line,
> +					spi->mode & SPI_CS_HIGH ? 0 : 1);
>  		}
>  		sdd->tgl_spi = NULL;
>  	}
>  
>  	cs = spi->controller_data;
> -	gpio_set_value(cs->line, spi->mode & SPI_CS_HIGH ? 1 : 0);
> +	if (!(sdd->port_conf->quirks & S3C64XX_SPI_QUIRK_GPIO))
> +		gpio_set_value(cs->line, spi->mode & SPI_CS_HIGH ? 1 : 0);
>  
>  	/* Start the signals */
>  	writel(0, sdd->regs + S3C64XX_SPI_SLAVE_SEL);
> @@ -503,7 +506,8 @@ static inline void disable_cs(struct s3c64xx_spi_driver_data *sdd,
>  	if (sdd->tgl_spi == spi)
>  		sdd->tgl_spi = NULL;
>  
> -	gpio_set_value(cs->line, spi->mode & SPI_CS_HIGH ? 0 : 1);
> +	if (!(sdd->port_conf->quirks & S3C64XX_SPI_QUIRK_GPIO))
> +		gpio_set_value(cs->line, spi->mode & SPI_CS_HIGH ? 0 : 1);
>  
>  	/* Quiese the signals */
>  	writel(S3C64XX_SPI_SLAVE_SIG_INACT,
> @@ -842,7 +846,10 @@ static struct s3c64xx_spi_csinfo *s3c64xx_get_slave_ctrldata(
>  		return ERR_PTR(-ENOMEM);
>  	}
>  
> -	cs->line = of_get_named_gpio(data_np, "cs-gpio", 0);
> +	/* In case of dedicated cs pin skip the gpio initialization */
> +	if (!(sdd->port_conf->quirks & S3C64XX_SPI_QUIRK_GPIO))
> +		cs->line = of_get_named_gpio(data_np, "cs-gpio", 0);
> +
>  	if (!gpio_is_valid(cs->line)) {
>  		dev_err(&spi->dev, "chip select gpio is not specified or "
>  					"invalid\n");
> @@ -883,7 +890,7 @@ static int s3c64xx_spi_setup(struct spi_device *spi)
>  		return -ENODEV;
>  	}
>  
> -	if (!spi_get_ctldata(spi)) {
> +	if (!(sdd->port_conf->quirks & S3C64XX_SPI_QUIRK_GPIO)) {
>  		err = gpio_request_one(cs->line, GPIOF_OUT_INIT_HIGH,
>  				       dev_name(&spi->dev));
>  		if (err) {
> @@ -892,9 +899,11 @@ static int s3c64xx_spi_setup(struct spi_device *spi)
>  				cs->line, err);
>  			goto err_gpio_req;
>  		}
> -		spi_set_ctldata(spi, cs);
>  	}
>  
> +	if (!spi_get_ctldata(spi))
> +		spi_set_ctldata(spi, cs);
> +
>  	sci = sdd->cntrlr_info;
>  
>  	spin_lock_irqsave(&sdd->lock, flags);
> @@ -979,8 +988,11 @@ err_gpio_req:
>  static void s3c64xx_spi_cleanup(struct spi_device *spi)
>  {
>  	struct s3c64xx_spi_csinfo *cs = spi_get_ctldata(spi);
> +	struct s3c64xx_spi_driver_data *sdd;
> +
> +	sdd = spi_master_get_devdata(spi->master);
>  
> -	if (cs) {
> +	if (!(sdd->port_conf->quirks & S3C64XX_SPI_QUIRK_GPIO) && cs) {
>  		gpio_free(cs->line);
>  		if (spi->dev.of_node)
>  			kfree(cs);
> @@ -1107,6 +1119,13 @@ static int s3c64xx_spi_parse_dt_gpio(struct s3c64xx_spi_driver_data *sdd)
>  	struct device *dev = &sdd->pdev->dev;
>  	int idx, gpio, ret;
>  
> +	/*
> +	 * If cs is not controlled by gpio, and
> +	 * the SoC uses internal dedicated pins
> +	 */
> +	if (sdd->port_conf->quirks & S3C64XX_SPI_QUIRK_GPIO)
> +		return 0;
> +
>  	/* find gpios for mosi, miso and clock lines */
>  	for (idx = 0; idx < 3; idx++) {
>  		gpio = of_get_gpio(dev->of_node, idx);
> @@ -1133,6 +1152,10 @@ free_gpio:
>  static void s3c64xx_spi_dt_gpio_free(struct s3c64xx_spi_driver_data *sdd)
>  {
>  	unsigned int idx;
> +
> +	if (sdd->port_conf->quirks & S3C64XX_SPI_QUIRK_GPIO)
> +		return;
> +
>  	for (idx = 0; idx < 3; idx++)
>  		gpio_free(sdd->gpios[idx]);
>  }
> -- 
> 1.7.10.4
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/

-- 
Grant Likely, B.Sc, P.Eng.
Secret Lab Technologies, Ltd.

Re: [PATCH 3/4] spi: s3c64xx: add gpio quirk for controller

[Girish KS]

On Wed, Feb 6, 2013 at 2:40 AM, Grant Likely <grant.likely@secretlab.ca> wrote:
> On Tue,  5 Feb 2013 15:09:43 -0800, Girish K S <girishks2000@gmail.com> wrote:
>> This patch adds support for spi controllers with
>> dedicated clk/miso/mosi/cs pins. It skips the gpio
>> parsing and initialization for controllers that
>> have dedicated pins.
>>
>> Signed-off-by: Girish K S <ks.giri@samsung.com>
>
> Instead of making this a quirk, you should use the presence/absense of a
> cs-gpios property to determin whether or not a gpio is used to
> manipulate the CS signal.
I can do this for CS signal. Other signals are also dedicated pins.
can i check the "gpios" property for them?
>
> Also, are you certain that you want to make this change? Most SPI
> controllers are actually unfriendly in that they insist on deasserting the
> CS line if the FIFO runs empty. Does this driver correctly keep the CS
> signal asserted when there are multiple transfers in a message?
The CS signal is asserted when the slave select bit is cleared, this
is done in the enable_cs function.
The CS signal is deasserted when the slave select bit is set, this is
done in the disable_cs function.
enable_cs is called at the begining of the message transfer and
disable is called at the end of the transfer.
>
> g.
>
>> ---
>>  drivers/spi/spi-s3c64xx.c |   39 +++++++++++++++++++++++++++++++--------
>>  1 file changed, 31 insertions(+), 8 deletions(-)
>>
>> diff --git a/drivers/spi/spi-s3c64xx.c b/drivers/spi/spi-s3c64xx.c
>> index 90770bd..f06bbee 100644
>> --- a/drivers/spi/spi-s3c64xx.c
>> +++ b/drivers/spi/spi-s3c64xx.c
>> @@ -36,6 +36,7 @@
>>
>>  #define MAX_SPI_PORTS                3
>>  #define S3C64XX_SPI_QUIRK_POLL               (1 << 0)
>> +#define S3C64XX_SPI_QUIRK_GPIO               (1 << 1)
>>
>>  /* Registers and bit-fields */
>>
>> @@ -404,14 +405,16 @@ static inline void enable_cs(struct s3c64xx_spi_driver_data *sdd,
>>               if (sdd->tgl_spi != spi) { /* if last mssg on diff device */
>>                       /* Deselect the last toggled device */
>>                       cs = sdd->tgl_spi->controller_data;
>> -                     gpio_set_value(cs->line,
>> -                             spi->mode & SPI_CS_HIGH ? 0 : 1);
>> +                     if (!(sdd->port_conf->quirks & S3C64XX_SPI_QUIRK_GPIO))
>> +                             gpio_set_value(cs->line,
>> +                                     spi->mode & SPI_CS_HIGH ? 0 : 1);
>>               }
>>               sdd->tgl_spi = NULL;
>>       }
>>
>>       cs = spi->controller_data;
>> -     gpio_set_value(cs->line, spi->mode & SPI_CS_HIGH ? 1 : 0);
>> +     if (!(sdd->port_conf->quirks & S3C64XX_SPI_QUIRK_GPIO))
>> +             gpio_set_value(cs->line, spi->mode & SPI_CS_HIGH ? 1 : 0);
>>
>>       /* Start the signals */
>>       writel(0, sdd->regs + S3C64XX_SPI_SLAVE_SEL);
>> @@ -503,7 +506,8 @@ static inline void disable_cs(struct s3c64xx_spi_driver_data *sdd,
>>       if (sdd->tgl_spi == spi)
>>               sdd->tgl_spi = NULL;
>>
>> -     gpio_set_value(cs->line, spi->mode & SPI_CS_HIGH ? 0 : 1);
>> +     if (!(sdd->port_conf->quirks & S3C64XX_SPI_QUIRK_GPIO))
>> +             gpio_set_value(cs->line, spi->mode & SPI_CS_HIGH ? 0 : 1);
>>
>>       /* Quiese the signals */
>>       writel(S3C64XX_SPI_SLAVE_SIG_INACT,
>> @@ -842,7 +846,10 @@ static struct s3c64xx_spi_csinfo *s3c64xx_get_slave_ctrldata(
>>               return ERR_PTR(-ENOMEM);
>>       }
>>
>> -     cs->line = of_get_named_gpio(data_np, "cs-gpio", 0);
>> +     /* In case of dedicated cs pin skip the gpio initialization */
>> +     if (!(sdd->port_conf->quirks & S3C64XX_SPI_QUIRK_GPIO))
>> +             cs->line = of_get_named_gpio(data_np, "cs-gpio", 0);
>> +
>>       if (!gpio_is_valid(cs->line)) {
>>               dev_err(&spi->dev, "chip select gpio is not specified or "
>>                                       "invalid\n");
>> @@ -883,7 +890,7 @@ static int s3c64xx_spi_setup(struct spi_device *spi)
>>               return -ENODEV;
>>       }
>>
>> -     if (!spi_get_ctldata(spi)) {
>> +     if (!(sdd->port_conf->quirks & S3C64XX_SPI_QUIRK_GPIO)) {
>>               err = gpio_request_one(cs->line, GPIOF_OUT_INIT_HIGH,
>>                                      dev_name(&spi->dev));
>>               if (err) {
>> @@ -892,9 +899,11 @@ static int s3c64xx_spi_setup(struct spi_device *spi)
>>                               cs->line, err);
>>                       goto err_gpio_req;
>>               }
>> -             spi_set_ctldata(spi, cs);
>>       }
>>
>> +     if (!spi_get_ctldata(spi))
>> +             spi_set_ctldata(spi, cs);
>> +
>>       sci = sdd->cntrlr_info;
>>
>>       spin_lock_irqsave(&sdd->lock, flags);
>> @@ -979,8 +988,11 @@ err_gpio_req:
>>  static void s3c64xx_spi_cleanup(struct spi_device *spi)
>>  {
>>       struct s3c64xx_spi_csinfo *cs = spi_get_ctldata(spi);
>> +     struct s3c64xx_spi_driver_data *sdd;
>> +
>> +     sdd = spi_master_get_devdata(spi->master);
>>
>> -     if (cs) {
>> +     if (!(sdd->port_conf->quirks & S3C64XX_SPI_QUIRK_GPIO) && cs) {
>>               gpio_free(cs->line);
>>               if (spi->dev.of_node)
>>                       kfree(cs);
>> @@ -1107,6 +1119,13 @@ static int s3c64xx_spi_parse_dt_gpio(struct s3c64xx_spi_driver_data *sdd)
>>       struct device *dev = &sdd->pdev->dev;
>>       int idx, gpio, ret;
>>
>> +     /*
>> +      * If cs is not controlled by gpio, and
>> +      * the SoC uses internal dedicated pins
>> +      */
>> +     if (sdd->port_conf->quirks & S3C64XX_SPI_QUIRK_GPIO)
>> +             return 0;
>> +
>>       /* find gpios for mosi, miso and clock lines */
>>       for (idx = 0; idx < 3; idx++) {
>>               gpio = of_get_gpio(dev->of_node, idx);
>> @@ -1133,6 +1152,10 @@ free_gpio:
>>  static void s3c64xx_spi_dt_gpio_free(struct s3c64xx_spi_driver_data *sdd)
>>  {
>>       unsigned int idx;
>> +
>> +     if (sdd->port_conf->quirks & S3C64XX_SPI_QUIRK_GPIO)
>> +             return;
>> +
>>       for (idx = 0; idx < 3; idx++)
>>               gpio_free(sdd->gpios[idx]);
>>  }
>> --
>> 1.7.10.4
>>
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>> Please read the FAQ at  http://www.tux.org/lkml/
>
> --
> Grant Likely, B.Sc, P.Eng.
> Secret Lab Technologies, Ltd.

Re: [PATCH 3/4] spi: s3c64xx: add gpio quirk for controller

[Mark Brown]

On Tue, Feb 05, 2013 at 03:09:43PM -0800, Girish K S wrote:
> This patch adds support for spi controllers with
> dedicated clk/miso/mosi/cs pins. It skips the gpio
> parsing and initialization for controllers that
> have dedicated pins.

>  		if (sdd->tgl_spi != spi) { /* if last mssg on diff device */
>  			/* Deselect the last toggled device */
>  			cs = sdd->tgl_spi->controller_data;
> -			gpio_set_value(cs->line,
> -				spi->mode & SPI_CS_HIGH ? 0 : 1);
> +			if (!(sdd->port_conf->quirks & S3C64XX_SPI_QUIRK_GPIO))
> +				gpio_set_value(cs->line,
> +					spi->mode & SPI_CS_HIGH ? 0 : 1);
>  		}

This isn't going to work with system designs which ignore the /CS line
the controller has and just use a GPIO instead.  This is very common,
for example when connecting multiple devices to the same SPI bus.

It seems like there's really two changes here.  One change is making the
provision of pinmux information optional, the other is allowing the user
to use the controller /CS management rather than using GPIO.

Re: [PATCH 3/4] spi: s3c64xx: add gpio quirk for controller

[Girish KS]

On Thu, Feb 7, 2013 at 3:55 AM, Mark Brown
<broonie@opensource.wolfsonmicro.com> wrote:
> On Tue, Feb 05, 2013 at 03:09:43PM -0800, Girish K S wrote:
>> This patch adds support for spi controllers with
>> dedicated clk/miso/mosi/cs pins. It skips the gpio
>> parsing and initialization for controllers that
>> have dedicated pins.
>
>>               if (sdd->tgl_spi != spi) { /* if last mssg on diff device */
>>                       /* Deselect the last toggled device */
>>                       cs = sdd->tgl_spi->controller_data;
>> -                     gpio_set_value(cs->line,
>> -                             spi->mode & SPI_CS_HIGH ? 0 : 1);
>> +                     if (!(sdd->port_conf->quirks & S3C64XX_SPI_QUIRK_GPIO))
>> +                             gpio_set_value(cs->line,
>> +                                     spi->mode & SPI_CS_HIGH ? 0 : 1);
>>               }
>
> This isn't going to work with system designs which ignore the /CS line
> the controller has and just use a GPIO instead.  This is very common,
> for example when connecting multiple devices to the same SPI bus.
As per grant's comment i would remove the quirk option and check for
"cs-gpio" property
to handle /CS. When multiple devices are connected to the same spi
bus, "cs-gpio" entry would exist in the dts
file, and works with /CS gpio lines.
>
> It seems like there's really two changes here.  One change is making the
> provision of pinmux information optional, the other is allowing the user
> to use the controller /CS management rather than using GPIO.
As suggested i would make two changes.
1. would parse the "gpios"  property and make decision whether to use
gpios/dedicated pins for miso/miso/clk.
2.would parse "cs-gpio"  property from dts node to handle the chip select
correct me if my understanding is wrong

Re: [PATCH 3/4] spi: s3c64xx: add gpio quirk for controller

[Mark Brown]

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

On Thu, Feb 07, 2013 at 10:54:01AM -0800, Girish KS wrote:

> As suggested i would make two changes.
> 1. would parse the "gpios"  property and make decision whether to use
> gpios/dedicated pins for miso/miso/clk.
> 2.would parse "cs-gpio"  property from dts node to handle the chip select
> correct me if my understanding is wrong

I think that's probably OK.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

[PATCH 4/4] spi: s3c64xx: add support for exynos5440 spi

[Girish K S]

This patch adds support for the exynos5440 spi controller.
The integration of the spi IP in exynos5440 is different from
other SoC's. The I/O pins are no more configured via gpio, they
have dedicated pins.

Signed-off-by: Girish K S <ks.giri@samsung.com>
---
 drivers/spi/spi-s3c64xx.c |   12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/drivers/spi/spi-s3c64xx.c b/drivers/spi/spi-s3c64xx.c
index f06bbee..bf26abc 100644
--- a/drivers/spi/spi-s3c64xx.c
+++ b/drivers/spi/spi-s3c64xx.c
@@ -1547,6 +1547,15 @@ static struct s3c64xx_spi_port_config exynos4_spi_port_config = {
 	.clk_from_cmu	= true,
 };
 
+static struct s3c64xx_spi_port_config exynos5440_spi_port_config = {
+	.fifo_lvl_mask	= { 0xff },
+	.rx_lvl_offset	= 15,
+	.tx_st_done	= 25,
+	.high_speed	= true,
+	.clk_from_cmu	= true,
+	.quirks		= S3C64XX_SPI_QUIRK_GPIO | S3C64XX_SPI_QUIRK_POLL,
+};
+
 static struct platform_device_id s3c64xx_spi_driver_ids[] = {
 	{
 		.name		= "s3c2443-spi",
@@ -1575,6 +1584,9 @@ static const struct of_device_id s3c64xx_spi_dt_match[] = {
 	{ .compatible = "samsung,exynos4210-spi",
 			.data = (void *)&exynos4_spi_port_config,
 	},
+	{ .compatible = "samsung,exynos5440-spi",
+			.data = (void *)&exynos5440_spi_port_config,
+	},
 	{ },
 };
 MODULE_DEVICE_TABLE(of, s3c64xx_spi_dt_match);
-- 
1.7.10.4