[PATCH 0/8] Some fixes for spi-s3c64xx

[PATCH 0/8] Some fixes for spi-s3c64xx

[Łukasz Stelmach]

This is a series of fixes created during porting a device driver (these
patches will be released soon too) for an SPI device to the current kernel.

The two most important are 

  spi: spi-s3c64xx: swap s3c64xx_spi_set_cs() and s3c64xx_enable_datapath()
  spi: spi-s3s64xx: Add S3C64XX_SPI_QUIRK_CS_AUTO for Exynos3250

Without them DMA transfers larger than 512 bytes from the SPI controller
would fail.

Łukasz Stelmach (8):
  spi: spi-s3c64xx: swap s3c64xx_spi_set_cs() and
    s3c64xx_enable_datapath()
  spi: spi-s3s64xx: Add S3C64XX_SPI_QUIRK_CS_AUTO for Exynos3250
  spi: spi-s3c64xx: Report more information when errors occur
  spi: spi-s3c64xx: Rename S3C64XX_SPI_SLAVE_* to S3C64XX_SPI_CS_*
  spi: spi-s3c64xx: Fix doc comment for struct s3c64xx_spi_driver_data
  spi: spi-s3c64xx: Check return values
  spi: spi-s3c64xx: Increase tranfer timeout
  spi: spi-s3c64xx: Turn on interrupts upon resume

 drivers/spi/spi-s3c64xx.c | 81 +++++++++++++++++++++++++++------------
 1 file changed, 56 insertions(+), 25 deletions(-)

-- 
2.26.2


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH 1/8] spi: spi-s3c64xx: swap s3c64xx_spi_set_cs() and s3c64xx_enable_datapath()

[Łukasz Stelmach]

Fix issues with DMA transfers bigger than 512 on Exynos3250.

Signed-off-by: Łukasz Stelmach <l.stelmach@samsung.com>
---
 drivers/spi/spi-s3c64xx.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/spi/spi-s3c64xx.c b/drivers/spi/spi-s3c64xx.c
index cf67ea60dc0e..fb5e2ba4b6b9 100644
--- a/drivers/spi/spi-s3c64xx.c
+++ b/drivers/spi/spi-s3c64xx.c
@@ -678,11 +678,11 @@ static int s3c64xx_spi_transfer_one(struct spi_master *master,
 		sdd->state &= ~RXBUSY;
 		sdd->state &= ~TXBUSY;
 
-		s3c64xx_enable_datapath(sdd, xfer, use_dma);
-
 		/* Start the signals */
 		s3c64xx_spi_set_cs(spi, true);
 
+		s3c64xx_enable_datapath(sdd, xfer, use_dma);
+
 		spin_unlock_irqrestore(&sdd->lock, flags);
 
 		if (use_dma)
-- 
2.26.2


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 1/8] spi: spi-s3c64xx: swap s3c64xx_spi_set_cs() and s3c64xx_enable_datapath()

[Krzysztof Kozlowski]

On Wed, Aug 19, 2020 at 02:32:01PM +0200, Łukasz Stelmach wrote:
> Fix issues with DMA transfers bigger than 512 on Exynos3250.

Fix, but how? With proper explanation this should go to CC-stable.

Best regards,
Krzysztof

> 
> Signed-off-by: Łukasz Stelmach <l.stelmach@samsung.com>
> ---
>  drivers/spi/spi-s3c64xx.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/spi/spi-s3c64xx.c b/drivers/spi/spi-s3c64xx.c
> index cf67ea60dc0e..fb5e2ba4b6b9 100644
> --- a/drivers/spi/spi-s3c64xx.c
> +++ b/drivers/spi/spi-s3c64xx.c
> @@ -678,11 +678,11 @@ static int s3c64xx_spi_transfer_one(struct spi_master *master,
>  		sdd->state &= ~RXBUSY;
>  		sdd->state &= ~TXBUSY;
>  
> -		s3c64xx_enable_datapath(sdd, xfer, use_dma);
> -
>  		/* Start the signals */
>  		s3c64xx_spi_set_cs(spi, true);
>  
> +		s3c64xx_enable_datapath(sdd, xfer, use_dma);
> +
>  		spin_unlock_irqrestore(&sdd->lock, flags);
>  
>  		if (use_dma)
> -- 
> 2.26.2
> 

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 1/8] spi: spi-s3c64xx: swap s3c64xx_spi_set_cs() and s3c64xx_enable_datapath()

[Lukasz Stelmach]

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

It was <2020-08-19 śro 14:38>, when Krzysztof Kozlowski wrote:
> On Wed, Aug 19, 2020 at 02:32:01PM +0200, Łukasz Stelmach wrote:
>> Fix issues with DMA transfers bigger than 512 on Exynos3250.
>
> Fix, but how? With proper explanation this should go to CC-stable.

Honestly, I don't know and I couldn't find out why. It makes stuff
work. There has been a commit like this before

    0f5a751ace25 spi/s3c64xx: Enable GPIO /CS prior to starting hardware

Apparently, it was lost in

    0732a9d2a155 spi/s3c64xx: Use core message handling

>> 
>> Signed-off-by: Łukasz Stelmach <l.stelmach@samsung.com>
>> ---
>>  drivers/spi/spi-s3c64xx.c | 4 ++--
>>  1 file changed, 2 insertions(+), 2 deletions(-)
>> 
>> diff --git a/drivers/spi/spi-s3c64xx.c b/drivers/spi/spi-s3c64xx.c
>> index cf67ea60dc0e..fb5e2ba4b6b9 100644
>> --- a/drivers/spi/spi-s3c64xx.c
>> +++ b/drivers/spi/spi-s3c64xx.c
>> @@ -678,11 +678,11 @@ static int s3c64xx_spi_transfer_one(struct spi_master *master,
>>  		sdd->state &= ~RXBUSY;
>>  		sdd->state &= ~TXBUSY;
>>  
>> -		s3c64xx_enable_datapath(sdd, xfer, use_dma);
>> -
>>  		/* Start the signals */
>>  		s3c64xx_spi_set_cs(spi, true);
>>  
>> +		s3c64xx_enable_datapath(sdd, xfer, use_dma);
>> +
>>  		spin_unlock_irqrestore(&sdd->lock, flags);
>>  
>>  		if (use_dma)
>> -- 
>> 2.26.2
>> 

-- 
Łukasz Stelmach
Samsung R&D Institute Poland
Samsung Electronics

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

[-- Attachment #2: Type: text/plain, Size: 176 bytes --]

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 1/8] spi: spi-s3c64xx: swap s3c64xx_spi_set_cs() and s3c64xx_enable_datapath()

[Krzysztof Kozlowski]

On Wed, Aug 19, 2020 at 02:51:27PM +0200, Lukasz Stelmach wrote:
> It was <2020-08-19 śro 14:38>, when Krzysztof Kozlowski wrote:
> > On Wed, Aug 19, 2020 at 02:32:01PM +0200, Łukasz Stelmach wrote:
> >> Fix issues with DMA transfers bigger than 512 on Exynos3250.
> >
> > Fix, but how? With proper explanation this should go to CC-stable.
> 
> Honestly, I don't know and I couldn't find out why. It makes stuff
> work. There has been a commit like this before
> 
>     0f5a751ace25 spi/s3c64xx: Enable GPIO /CS prior to starting hardware
> 
> Apparently, it was lost in
> 
>     0732a9d2a155 spi/s3c64xx: Use core message handling

Then describe at least this... maybe Mark knows why he brough back old
code after refactoring?

Best regards,
Krzysztof

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 1/8] spi: spi-s3c64xx: swap s3c64xx_spi_set_cs() and s3c64xx_enable_datapath()

[Mark Brown]

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

On Wed, Aug 19, 2020 at 02:58:22PM +0200, Krzysztof Kozlowski wrote:
> On Wed, Aug 19, 2020 at 02:51:27PM +0200, Lukasz Stelmach wrote:

> > Honestly, I don't know and I couldn't find out why. It makes stuff
> > work. There has been a commit like this before

> >     0f5a751ace25 spi/s3c64xx: Enable GPIO /CS prior to starting hardware

> > Apparently, it was lost in

> >     0732a9d2a155 spi/s3c64xx: Use core message handling

> Then describe at least this... maybe Mark knows why he brough back old
> code after refactoring?

I'm not sure what's being referred to as being lost in the second commit
TBH.  The first commit is simple code motion rather than a correctness
thing, and more related to the handling of GPIO controlled chip selects
according to the description (which people should be using with that
controller anyway where possible IIRC, the native chip select has too
many assumptions about what it's doing).  I don't know that I ever
actually used a system that used the native chip select as the actual
chip select.  Perhaps some quirk was introduced where the chip select
signal does something?

The commit is also lacking a description of what the issues that are
being fixed are.

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

[-- Attachment #2: Type: text/plain, Size: 176 bytes --]

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 1/8] spi: spi-s3c64xx: swap s3c64xx_spi_set_cs() and s3c64xx_enable_datapath()

[Lukasz Stelmach]

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

It was <2020-08-19 śro 14:16>, when Mark Brown wrote:
> On Wed, Aug 19, 2020 at 02:58:22PM +0200, Krzysztof Kozlowski wrote:
>> On Wed, Aug 19, 2020 at 02:51:27PM +0200, Lukasz Stelmach wrote:
>
>> > Honestly, I don't know and I couldn't find out why. It makes stuff
>> > work. There has been a commit like this before
>
>> >     0f5a751ace25 spi/s3c64xx: Enable GPIO /CS prior to starting hardware
>
>> > Apparently, it was lost in
>
>> >     0732a9d2a155 spi/s3c64xx: Use core message handling
>
>> Then describe at least this... maybe Mark knows why he brough back old
>> code after refactoring?
>
> I'm not sure what's being referred to as being lost in the second commit
> TBH.

Order of enable_cs() and enable_datapath(). The order 0f5a sets makes
(for a reaseon I don't know) my devices work. In the latter commit,
which rewrites "everything", enable_datapath() is called before what
later (in aa4964c4eb3e) became s3c64xx_spi_set_cs().

> The first commit is simple code motion rather than a correctness
> thing, and more related to the handling of GPIO controlled chip
> selects according to the description (which people should be using
> with that controller anyway where possible IIRC, the native chip
> select has too many assumptions about what it's doing).

Funny, but without the automatic CS control (see the next patch in this
series) my stuff does not work.

> I don't know that I ever actually used a system that used the native
> chip select as the actual chip select.  Perhaps some quirk was
> introduced where the chip select signal does something?
>
> The commit is also lacking a description of what the issues that are
> being fixed are.

On Exynos3250 DMA transfers from SPI longer than 512 fail.

-- 
Łukasz Stelmach
Samsung R&D Institute Poland
Samsung Electronics

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

[-- Attachment #2: Type: text/plain, Size: 176 bytes --]

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 1/8] spi: spi-s3c64xx: swap s3c64xx_spi_set_cs() and s3c64xx_enable_datapath()

[Mark Brown]

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

On Wed, Aug 19, 2020 at 04:01:52PM +0200, Lukasz Stelmach wrote:
> It was <2020-08-19 śro 14:16>, when Mark Brown wrote:
> > On Wed, Aug 19, 2020 at 02:58:22PM +0200, Krzysztof Kozlowski wrote:
> >> On Wed, Aug 19, 2020 at 02:51:27PM +0200, Lukasz Stelmach wrote:

> >> >     0732a9d2a155 spi/s3c64xx: Use core message handling

> >> Then describe at least this... maybe Mark knows why he brough back old
> >> code after refactoring?

> > I'm not sure what's being referred to as being lost in the second commit
> > TBH.

> Order of enable_cs() and enable_datapath(). The order 0f5a sets makes
> (for a reaseon I don't know) my devices work. In the latter commit,
> which rewrites "everything", enable_datapath() is called before what
> later (in aa4964c4eb3e) became s3c64xx_spi_set_cs().

That's doesn't look like what the changes do.  Note that the enable_cs()
function that got moved in 0f5a751ace250097 (spi/s3c64xx: Enable GPIO
/CS prior to starting hardware) does not touch the chip registers at
all, it only manipulates GPIOs, code that was subsequently factored out
into the core.  The write to the _SLAVE_SEL register has so far as I can
see always been after enable_datapath() right back to the initial
commit, it just got made more complex for the Exynos7 controller (I'm
guessing your new one might be an ancestor of that?) in bf77cba95f8c06
(spi: s3c64xx: add support for exynos7 SPI controller) and then factored
out in the commit you mention above.

Are you sure your new ordering works for all controller revisions?
According to the comment the _set_cs() is what's actually kicking off
the transfer which suggests that the data/DMA needed to be ready
beforehand to avoid an underflow or something (and nobody reported
issues before, I know people have done things like downloaded firmwares
using this controller...), this could be something that got changed
between revisions.

Please include human readable descriptions of things like commits and
issues being discussed in e-mail in your mails, this makes them much
easier for humans to read especially when they have no internet access.

> > The first commit is simple code motion rather than a correctness
> > thing, and more related to the handling of GPIO controlled chip
> > selects according to the description (which people should be using
> > with that controller anyway where possible IIRC, the native chip
> > select has too many assumptions about what it's doing).

> Funny, but without the automatic CS control (see the next patch in this
> series) my stuff does not work.

There's two things, there's changing the controller registers and there
is the use of the signal coming out of the controller as the chip select
that the device on the SPI bus sees.  Most systems have pinmuxing which
allows the internal chip select to just not be connected to anything
which is what I'm talking about in the above text, IIRC all versions of
the controller have unfortunate assumptions about how chip selects
should work which make GPIO controlled chip selects much better.

> > I don't know that I ever actually used a system that used the native
> > chip select as the actual chip select.  Perhaps some quirk was
> > introduced where the chip select signal does something?

> > The commit is also lacking a description of what the issues that are
> > being fixed are.

> On Exynos3250 DMA transfers from SPI longer than 512 fail.

Could you expand on "fail" please?

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

[-- Attachment #2: Type: text/plain, Size: 176 bytes --]

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 1/8] spi: spi-s3c64xx: swap s3c64xx_spi_set_cs() and s3c64xx_enable_datapath()

[Lukasz Stelmach]

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

It was <2020-08-19 śro 20:12>, when Mark Brown wrote:
> On Wed, Aug 19, 2020 at 04:01:52PM +0200, Lukasz Stelmach wrote:
>> It was <2020-08-19 śro 14:16>, when Mark Brown wrote:
>>> On Wed, Aug 19, 2020 at 02:58:22PM +0200, Krzysztof Kozlowski wrote:
>>>> On Wed, Aug 19, 2020 at 02:51:27PM +0200, Lukasz Stelmach wrote:
>>>>
>>>>>     0732a9d2a155 spi/s3c64xx: Use core message handling
>>>>
>>>> Then describe at least this... maybe Mark knows why he brough back old
>>>> code after refactoring?
>>>>
>>> I'm not sure what's being referred to as being lost in the second commit
>>> TBH.
>
>> Order of enable_cs() and enable_datapath(). The order 0f5a sets makes
>> (for a reaseon I don't know) my devices work. In the latter commit,
>> which rewrites "everything", enable_datapath() is called before what
>> later (in aa4964c4eb3e) became s3c64xx_spi_set_cs().
>
> That's doesn't look like what the changes do.  Note that the enable_cs()
> function that got moved in 0f5a751ace250097 (spi/s3c64xx: Enable GPIO
> /CS prior to starting hardware) does not touch the chip registers at
> all, it only manipulates GPIOs, code that was subsequently factored out
> into the core.

Indeed, you are 100% right. Anyway that commit has inspired me after
days of trying different stuff to switch enable_datapath() and set_cs()
and it worked. Even if without any technical connection with your commit.

> The write to the _SLAVE_SEL register has so far as I can see always
> been after enable_datapath() right back to the initial commit, it just
> got made more complex for the Exynos7 controller (I'm guessing your
> new one might be an ancestor of that?) in bf77cba95f8c06 (spi:
> s3c64xx: add support for exynos7 SPI controller) and then factored out
> in the commit you mention above.
>
> Are you sure your new ordering works for all controller revisions?

Not 100%, but we've tested it on several differnt SoCs, and haven't seen
any problems.

> According to the comment the _set_cs() is what's actually kicking off
> the transfer

I don't think so. Indeed, without the CS_AUTO quirk CS is pulled down
(the SPI device is selected) but for the transfer to start the SPI clock
needs to start ticking.

> Please include human readable descriptions of things like commits and
> issues being discussed in e-mail in your mails, this makes them much
> easier for humans to read especially when they have no internet access.

I will.

>>> I don't know that I ever actually used a system that used the native
>>> chip select as the actual chip select.  Perhaps some quirk was
>>> introduced where the chip select signal does something?
>>>
>>> The commit is also lacking a description of what the issues that are
>>> being fixed are.
>>
>> On Exynos3250 DMA transfers from SPI longer than 512 fail.
>
> Could you expand on "fail" please?

Stopping a transfer and hitting timeout with a few bytes (<20) left
pending in the SPI controller.

-- 
Łukasz Stelmach
Samsung R&D Institute Poland
Samsung Electronics

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

[-- Attachment #2: Type: text/plain, Size: 176 bytes --]

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH 2/8] spi: spi-s3s64xx: Add S3C64XX_SPI_QUIRK_CS_AUTO for Exynos3250

[Łukasz Stelmach]

Signed-off-by: Łukasz Stelmach <l.stelmach@samsung.com>
---
 drivers/spi/spi-s3c64xx.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/spi/spi-s3c64xx.c b/drivers/spi/spi-s3c64xx.c
index fb5e2ba4b6b9..8fe44451d303 100644
--- a/drivers/spi/spi-s3c64xx.c
+++ b/drivers/spi/spi-s3c64xx.c
@@ -1372,6 +1372,7 @@ static struct s3c64xx_spi_port_config exynos4_spi_port_config = {
 	.tx_st_done	= 25,
 	.high_speed	= true,
 	.clk_from_cmu	= true,
+	.quirks		=  S3C64XX_SPI_QUIRK_CS_AUTO,
 };
 
 static struct s3c64xx_spi_port_config exynos7_spi_port_config = {
-- 
2.26.2


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 2/8] spi: spi-s3s64xx: Add S3C64XX_SPI_QUIRK_CS_AUTO for Exynos3250

[Krzysztof Kozlowski]

On Wed, Aug 19, 2020 at 02:32:02PM +0200, Łukasz Stelmach wrote:
> Signed-off-by: Łukasz Stelmach <l.stelmach@samsung.com>

Add a quirk - why? There is here no commit msg, no explanation.

Best regards,
Krzysztof


> ---
>  drivers/spi/spi-s3c64xx.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/drivers/spi/spi-s3c64xx.c b/drivers/spi/spi-s3c64xx.c
> index fb5e2ba4b6b9..8fe44451d303 100644
> --- a/drivers/spi/spi-s3c64xx.c
> +++ b/drivers/spi/spi-s3c64xx.c
> @@ -1372,6 +1372,7 @@ static struct s3c64xx_spi_port_config exynos4_spi_port_config = {
>  	.tx_st_done	= 25,
>  	.high_speed	= true,
>  	.clk_from_cmu	= true,
> +	.quirks		=  S3C64XX_SPI_QUIRK_CS_AUTO,
>  };
>  
>  static struct s3c64xx_spi_port_config exynos7_spi_port_config = {
> -- 
> 2.26.2
> 

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 2/8] spi: spi-s3s64xx: Add S3C64XX_SPI_QUIRK_CS_AUTO for Exynos3250

[Lukasz Stelmach]

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

It was <2020-08-19 śro 14:39>, when Krzysztof Kozlowski wrote:
> On Wed, Aug 19, 2020 at 02:32:02PM +0200, Łukasz Stelmach wrote:
>> Signed-off-by: Łukasz Stelmach <l.stelmach@samsung.com>
>
> Add a quirk - why?

Because stuff does not work without it and works with it and it is
turned on for other SoCs which have simmilar SPI HW.

> There is here no commit msg, no explanation.

As I wrote in the cover letter, this and previous commits make things
work on Exynos3250 (ARTIK5 precisely). I can't explain why. I read
everything I could about this HW and there were no details about
automatic CS handling other than how to turn it on and off.

>> ---
>>  drivers/spi/spi-s3c64xx.c | 1 +
>>  1 file changed, 1 insertion(+)
>> 
>> diff --git a/drivers/spi/spi-s3c64xx.c b/drivers/spi/spi-s3c64xx.c
>> index fb5e2ba4b6b9..8fe44451d303 100644
>> --- a/drivers/spi/spi-s3c64xx.c
>> +++ b/drivers/spi/spi-s3c64xx.c
>> @@ -1372,6 +1372,7 @@ static struct s3c64xx_spi_port_config exynos4_spi_port_config = {
>>  	.tx_st_done	= 25,
>>  	.high_speed	= true,
>>  	.clk_from_cmu	= true,
>> +	.quirks		=  S3C64XX_SPI_QUIRK_CS_AUTO,
>>  };
>>  
>>  static struct s3c64xx_spi_port_config exynos7_spi_port_config = {
>> -- 
>> 2.26.2
>> 
>

-- 
Łukasz Stelmach
Samsung R&D Institute Poland
Samsung Electronics

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

[-- Attachment #2: Type: text/plain, Size: 176 bytes --]

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 2/8] spi: spi-s3s64xx: Add S3C64XX_SPI_QUIRK_CS_AUTO for Exynos3250

[Krzysztof Kozlowski]

On Wed, Aug 19, 2020 at 03:01:21PM +0200, Lukasz Stelmach wrote:
> It was <2020-08-19 śro 14:39>, when Krzysztof Kozlowski wrote:
> > On Wed, Aug 19, 2020 at 02:32:02PM +0200, Łukasz Stelmach wrote:
> >> Signed-off-by: Łukasz Stelmach <l.stelmach@samsung.com>
> >
> > Add a quirk - why?
> 
> Because stuff does not work without it and works with it and it is
> turned on for other SoCs which have simmilar SPI HW.
> 
> > There is here no commit msg, no explanation.
> 
> As I wrote in the cover letter, this and previous commits make things
> work on Exynos3250 (ARTIK5 precisely). I can't explain why. I read
> everything I could about this HW and there were no details about
> automatic CS handling other than how to turn it on and off.

At least this information would be useful. If vendor kernel also does
it, that's another argument to use, since there are no better ones...

Best regards,
Krzysztof

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 2/8] spi: spi-s3s64xx: Add S3C64XX_SPI_QUIRK_CS_AUTO for Exynos3250

[Mark Brown]

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

On Wed, Aug 19, 2020 at 03:01:21PM +0200, Lukasz Stelmach wrote:
> It was <2020-08-19 śro 14:39>, when Krzysztof Kozlowski wrote:

> > There is here no commit msg, no explanation.

> As I wrote in the cover letter, this and previous commits make things
> work on Exynos3250 (ARTIK5 precisely). I can't explain why. I read
> everything I could about this HW and there were no details about
> automatic CS handling other than how to turn it on and off.

What is similar about those other SoCs - could you be more specific
here, or what goes wrong if you don't set this?  The auto mode (or at
least the auto mode that was on the Exynos7) is not compatible with many
SPI devices if the controller chip select is actually in use, the quirk
was added for controllers that just don't have the manual mode.

See also:

  https://lore.kernel.org/linux-spi/CAAgF-BfGwcNzMx0meFVkJqNMTbQ4_PP1PZ3i6edOm6U3bc26_Q@mail.gmail.com/

for an explanation of the quirk.

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

[-- Attachment #2: Type: text/plain, Size: 176 bytes --]

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 2/8] spi: spi-s3s64xx: Add S3C64XX_SPI_QUIRK_CS_AUTO for Exynos3250

[Lukasz Stelmach]

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

It was <2020-08-19 śro 20:38>, when Mark Brown wrote:
> On Wed, Aug 19, 2020 at 03:01:21PM +0200, Lukasz Stelmach wrote:
>> It was <2020-08-19 śro 14:39>, when Krzysztof Kozlowski wrote:
>
>>> There is here no commit msg, no explanation.
>
>> As I wrote in the cover letter, this and previous commits make things
>> work on Exynos3250 (ARTIK5 precisely). I can't explain why. I read
>> everything I could about this HW and there were no details about
>> automatic CS handling other than how to turn it on and off.
>
> What is similar about those other SoCs - could you be more specific
> here, or what goes wrong if you don't set this?

Without the quirk set DMA transfers longer than 512 bytes fail. They
simply stop and hit the timeout with a few (<20) bytes pending.

As far as I can tell the SPI controller is the same in different Exynos
SoCs.

> The auto mode (or at least the auto mode that was on the Exynos7) is
> not compatible with many SPI devices if the controller chip select is
> actually in use, the quirk was added for controllers that just don't
> have the manual mode.

According to the manual the auto mode makes the controller toggle CS
between SPI packets (bytes?).

I didn't have any problem transferring data (>512 bytes) from the SPI
device in the polling mode. Only the DMA caused problems.
> See also:
>
>   https://lore.kernel.org/linux-spi/CAAgF-BfGwcNzMx0meFVkJqNMTbQ4_PP1PZ3i6edOm6U3bc26_Q@mail.gmail.com/
>
> for an explanation of the quirk.
>

>> CS can also be controlled automatically by setting AUTO_N_MANUAL to 1
>> in CS_CFG. When it is auto CS automatically toggles between packet to
>> packet. NCS_TIME_COUNT in CS_CFG controls the inactive period. The
>> driver by default uses manual mode. But on exynos7 the manual mode is
>> removed.

I *suspect* that the automatic CS toggling between packets gives better
(?) synchronisation between the SPI device and the controller's
internals and prevents some kind of a deadlock inside the
controller. These are just speculations.

-- 
Łukasz Stelmach
Samsung R&D Institute Poland
Samsung Electronics

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

[-- Attachment #2: Type: text/plain, Size: 176 bytes --]

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH 3/8] spi: spi-s3c64xx: Report more information when errors occur

[Łukasz Stelmach]

Report amount of pending data when a transfer stops due to errors.

Report if DMA was used to transfer data and print the status code.

Signed-off-by: Łukasz Stelmach <l.stelmach@samsung.com>
---
 drivers/spi/spi-s3c64xx.c | 22 +++++++++++++++++-----
 1 file changed, 17 insertions(+), 5 deletions(-)

diff --git a/drivers/spi/spi-s3c64xx.c b/drivers/spi/spi-s3c64xx.c
index 8fe44451d303..4ab68cd1b821 100644
--- a/drivers/spi/spi-s3c64xx.c
+++ b/drivers/spi/spi-s3c64xx.c
@@ -122,6 +122,7 @@
 
 struct s3c64xx_spi_dma_data {
 	struct dma_chan *ch;
+	dma_cookie_t cookie;
 	enum dma_transfer_direction direction;
 };
 
@@ -297,7 +298,7 @@ static void prepare_dma(struct s3c64xx_spi_dma_data *dma,
 	desc->callback = s3c64xx_spi_dmacb;
 	desc->callback_param = dma;
 
-	dmaengine_submit(desc);
+	dma->cookie = dmaengine_submit(desc);
 	dma_async_issue_pending(dma->ch);
 }
 
@@ -692,17 +693,28 @@ static int s3c64xx_spi_transfer_one(struct spi_master *master,
 
 		if (status) {
 			dev_err(&spi->dev,
-				"I/O Error: rx-%d tx-%d res:rx-%c tx-%c len-%d\n",
+				"I/O Error: rx-%d tx-%d rx-%c tx-%c len-%d dma-%d res-(%d)\n",
 				xfer->rx_buf ? 1 : 0, xfer->tx_buf ? 1 : 0,
 				(sdd->state & RXBUSY) ? 'f' : 'p',
 				(sdd->state & TXBUSY) ? 'f' : 'p',
-				xfer->len);
+				xfer->len, use_dma ? 1 : 0, status);
 
 			if (use_dma) {
-				if (xfer->tx_buf && (sdd->state & TXBUSY))
+				struct dma_tx_state s;
+
+				if (xfer->tx_buf && (sdd->state & TXBUSY)) {
+					dmaengine_pause(sdd->tx_dma.ch);
+					dmaengine_tx_status(sdd->tx_dma.ch, sdd->tx_dma.cookie, &s);
 					dmaengine_terminate_all(sdd->tx_dma.ch);
-				if (xfer->rx_buf && (sdd->state & RXBUSY))
+					dev_err(&spi->dev, "TX residue: %d\n", s.residue);
+
+				}
+				if (xfer->rx_buf && (sdd->state & RXBUSY)) {
+					dmaengine_pause(sdd->rx_dma.ch);
+					dmaengine_tx_status(sdd->rx_dma.ch, sdd->rx_dma.cookie, &s);
 					dmaengine_terminate_all(sdd->rx_dma.ch);
+					dev_err(&spi->dev, "RX residue: %d\n", s.residue);
+				}
 			}
 		} else {
 			s3c64xx_flush_fifo(sdd);
-- 
2.26.2


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 3/8] spi: spi-s3c64xx: Report more information when errors occur

[Krzysztof Kozlowski]

On Wed, Aug 19, 2020 at 02:32:03PM +0200, Łukasz Stelmach wrote:
> Report amount of pending data when a transfer stops due to errors.
> 
> Report if DMA was used to transfer data and print the status code.
> 
> Signed-off-by: Łukasz Stelmach <l.stelmach@samsung.com>
> ---
>  drivers/spi/spi-s3c64xx.c | 22 +++++++++++++++++-----
>  1 file changed, 17 insertions(+), 5 deletions(-)

Reviewed-by: Krzysztof Kozlowski <krzk@kernel.org>

Best regards,
Krzysztof


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH 4/8] spi: spi-s3c64xx: Rename S3C64XX_SPI_SLAVE_* to S3C64XX_SPI_CS_*

[Łukasz Stelmach]

Rename S3C64XX_SPI_SLAVE_* to S3C64XX_SPI_CS_* to match documentation.

Signed-off-by: Łukasz Stelmach <l.stelmach@samsung.com>
---
 drivers/spi/spi-s3c64xx.c | 26 +++++++++++++-------------
 1 file changed, 13 insertions(+), 13 deletions(-)

diff --git a/drivers/spi/spi-s3c64xx.c b/drivers/spi/spi-s3c64xx.c
index 4ab68cd1b821..6fe896f2be18 100644
--- a/drivers/spi/spi-s3c64xx.c
+++ b/drivers/spi/spi-s3c64xx.c
@@ -29,7 +29,7 @@
 #define S3C64XX_SPI_CH_CFG		0x00
 #define S3C64XX_SPI_CLK_CFG		0x04
 #define S3C64XX_SPI_MODE_CFG		0x08
-#define S3C64XX_SPI_SLAVE_SEL		0x0C
+#define S3C64XX_SPI_CS_REG		0x0C
 #define S3C64XX_SPI_INT_EN		0x10
 #define S3C64XX_SPI_STATUS		0x14
 #define S3C64XX_SPI_TX_DATA		0x18
@@ -64,9 +64,9 @@
 #define S3C64XX_SPI_MODE_TXDMA_ON		(1<<1)
 #define S3C64XX_SPI_MODE_4BURST			(1<<0)
 
-#define S3C64XX_SPI_SLAVE_AUTO			(1<<1)
-#define S3C64XX_SPI_SLAVE_SIG_INACT		(1<<0)
-#define S3C64XX_SPI_SLAVE_NSC_CNT_2		(2<<4)
+#define S3C64XX_SPI_CS_NSC_CNT_2		(2<<4)
+#define S3C64XX_SPI_CS_AUTO			(1<<1)
+#define S3C64XX_SPI_CS_SIG_INACT		(1<<0)
 
 #define S3C64XX_SPI_INT_TRAILING_EN		(1<<6)
 #define S3C64XX_SPI_INT_RX_OVERRUN_EN		(1<<5)
@@ -312,18 +312,18 @@ static void s3c64xx_spi_set_cs(struct spi_device *spi, bool enable)
 
 	if (enable) {
 		if (!(sdd->port_conf->quirks & S3C64XX_SPI_QUIRK_CS_AUTO)) {
-			writel(0, sdd->regs + S3C64XX_SPI_SLAVE_SEL);
+			writel(0, sdd->regs + S3C64XX_SPI_CS_REG);
 		} else {
-			u32 ssel = readl(sdd->regs + S3C64XX_SPI_SLAVE_SEL);
+			u32 ssel = readl(sdd->regs + S3C64XX_SPI_CS_REG);
 
-			ssel |= (S3C64XX_SPI_SLAVE_AUTO |
-						S3C64XX_SPI_SLAVE_NSC_CNT_2);
-			writel(ssel, sdd->regs + S3C64XX_SPI_SLAVE_SEL);
+			ssel |= (S3C64XX_SPI_CS_AUTO |
+						S3C64XX_SPI_CS_NSC_CNT_2);
+			writel(ssel, sdd->regs + S3C64XX_SPI_CS_REG);
 		}
 	} else {
 		if (!(sdd->port_conf->quirks & S3C64XX_SPI_QUIRK_CS_AUTO))
-			writel(S3C64XX_SPI_SLAVE_SIG_INACT,
-			       sdd->regs + S3C64XX_SPI_SLAVE_SEL);
+			writel(S3C64XX_SPI_CS_SIG_INACT,
+			       sdd->regs + S3C64XX_SPI_CS_REG);
 	}
 }
 
@@ -944,9 +944,9 @@ static void s3c64xx_spi_hwinit(struct s3c64xx_spi_driver_data *sdd)
 	sdd->cur_speed = 0;
 
 	if (sci->no_cs)
-		writel(0, sdd->regs + S3C64XX_SPI_SLAVE_SEL);
+		writel(0, sdd->regs + S3C64XX_SPI_CS_REG);
 	else if (!(sdd->port_conf->quirks & S3C64XX_SPI_QUIRK_CS_AUTO))
-		writel(S3C64XX_SPI_SLAVE_SIG_INACT, sdd->regs + S3C64XX_SPI_SLAVE_SEL);
+		writel(S3C64XX_SPI_CS_SIG_INACT, sdd->regs + S3C64XX_SPI_CS_REG);
 
 	/* Disable Interrupts - we use Polling if not DMA mode */
 	writel(0, regs + S3C64XX_SPI_INT_EN);
-- 
2.26.2


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 4/8] spi: spi-s3c64xx: Rename S3C64XX_SPI_SLAVE_* to S3C64XX_SPI_CS_*

[Krzysztof Kozlowski]

On Wed, Aug 19, 2020 at 02:32:04PM +0200, Łukasz Stelmach wrote:
> Rename S3C64XX_SPI_SLAVE_* to S3C64XX_SPI_CS_* to match documentation.
> 
> Signed-off-by: Łukasz Stelmach <l.stelmach@samsung.com>
> ---
>  drivers/spi/spi-s3c64xx.c | 26 +++++++++++++-------------
>  1 file changed, 13 insertions(+), 13 deletions(-)

Reviewed-by: Krzysztof Kozlowski <krzk@kernel.org>

Best regards,
Krzysztof

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH 5/8] spi: spi-s3c64xx: Fix doc comment for struct s3c64xx_spi_driver_data

[Łukasz Stelmach]

Signed-off-by: Łukasz Stelmach <l.stelmach@samsung.com>
---
 drivers/spi/spi-s3c64xx.c | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

diff --git a/drivers/spi/spi-s3c64xx.c b/drivers/spi/spi-s3c64xx.c
index 6fe896f2be18..505789f91fdf 100644
--- a/drivers/spi/spi-s3c64xx.c
+++ b/drivers/spi/spi-s3c64xx.c
@@ -155,19 +155,21 @@ struct s3c64xx_spi_port_config {
  * @clk: Pointer to the spi clock.
  * @src_clk: Pointer to the clock used to generate SPI signals.
  * @ioclk: Pointer to the i/o clock between master and slave
+ * @pdev: Pointer to the SPI controller platform device
  * @master: Pointer to the SPI Protocol master.
  * @cntrlr_info: Platform specific data for the controller this driver manages.
  * @lock: Controller specific lock.
  * @state: Set of FLAGS to indicate status.
- * @rx_dmach: Controller's DMA channel for Rx.
- * @tx_dmach: Controller's DMA channel for Tx.
+ * @rx_dma: Controller's DMA channel for Rx.
+ * @tx_dma: Controller's DMA channel for Tx.
  * @sfr_start: BUS address of SPI controller regs.
  * @regs: Pointer to ioremap'ed controller registers.
- * @irq: interrupt
  * @xfer_completion: To indicate completion of xfer task.
  * @cur_mode: Stores the active configuration of the controller.
  * @cur_bpw: Stores the active bits per word settings.
  * @cur_speed: Stores the active xfer clock speed.
+ * @port_conf: Pointer to the SPI port configuration
+ * @port_id: SPI port ID number
  */
 struct s3c64xx_spi_driver_data {
 	void __iomem                    *regs;
@@ -176,7 +178,7 @@ struct s3c64xx_spi_driver_data {
 	struct clk                      *ioclk;
 	struct platform_device          *pdev;
 	struct spi_master               *master;
-	struct s3c64xx_spi_info  *cntrlr_info;
+	struct s3c64xx_spi_info         *cntrlr_info;
 	spinlock_t                      lock;
 	unsigned long                   sfr_start;
 	struct completion               xfer_completion;
-- 
2.26.2


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 5/8] spi: spi-s3c64xx: Fix doc comment for struct s3c64xx_spi_driver_data

[Krzysztof Kozlowski]

On Wed, Aug 19, 2020 at 02:32:05PM +0200, Łukasz Stelmach wrote:
> Signed-off-by: Łukasz Stelmach <l.stelmach@samsung.com>

Hi,

Missing commit msg - fix what exactly?

You need to rebase your patch as most of these were already fixed by Lee
Jones.

However he did not remove the @rx_dmach and tx entries...

Best regards,
Krzysztof

> ---
>  drivers/spi/spi-s3c64xx.c | 10 ++++++----
>  1 file changed, 6 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/spi/spi-s3c64xx.c b/drivers/spi/spi-s3c64xx.c
> index 6fe896f2be18..505789f91fdf 100644
> --- a/drivers/spi/spi-s3c64xx.c
> +++ b/drivers/spi/spi-s3c64xx.c
> @@ -155,19 +155,21 @@ struct s3c64xx_spi_port_config {
>   * @clk: Pointer to the spi clock.
>   * @src_clk: Pointer to the clock used to generate SPI signals.
>   * @ioclk: Pointer to the i/o clock between master and slave
> + * @pdev: Pointer to the SPI controller platform device
>   * @master: Pointer to the SPI Protocol master.
>   * @cntrlr_info: Platform specific data for the controller this driver manages.
>   * @lock: Controller specific lock.
>   * @state: Set of FLAGS to indicate status.
> - * @rx_dmach: Controller's DMA channel for Rx.
> - * @tx_dmach: Controller's DMA channel for Tx.
> + * @rx_dma: Controller's DMA channel for Rx.
> + * @tx_dma: Controller's DMA channel for Tx.
>   * @sfr_start: BUS address of SPI controller regs.
>   * @regs: Pointer to ioremap'ed controller registers.
> - * @irq: interrupt
>   * @xfer_completion: To indicate completion of xfer task.
>   * @cur_mode: Stores the active configuration of the controller.
>   * @cur_bpw: Stores the active bits per word settings.
>   * @cur_speed: Stores the active xfer clock speed.
> + * @port_conf: Pointer to the SPI port configuration
> + * @port_id: SPI port ID number
>   */

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 5/8] spi: spi-s3c64xx: Fix doc comment for struct s3c64xx_spi_driver_data

[Lukasz Stelmach]

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

It was <2020-08-19 śro 14:37>, when Krzysztof Kozlowski wrote:
> On Wed, Aug 19, 2020 at 02:32:05PM +0200, Łukasz Stelmach wrote:
>> Signed-off-by: Łukasz Stelmach <l.stelmach@samsung.com>
>
> Hi,
>
> Missing commit msg - fix what exactly?
>
> You need to rebase your patch as most of these were already fixed by Lee
> Jones.
>
> However he did not remove the @rx_dmach and tx entries...

I will.

>> ---
>>  drivers/spi/spi-s3c64xx.c | 10 ++++++----
>>  1 file changed, 6 insertions(+), 4 deletions(-)
>> 
>> diff --git a/drivers/spi/spi-s3c64xx.c b/drivers/spi/spi-s3c64xx.c
>> index 6fe896f2be18..505789f91fdf 100644
>> --- a/drivers/spi/spi-s3c64xx.c
>> +++ b/drivers/spi/spi-s3c64xx.c
>> @@ -155,19 +155,21 @@ struct s3c64xx_spi_port_config {
>>   * @clk: Pointer to the spi clock.
>>   * @src_clk: Pointer to the clock used to generate SPI signals.
>>   * @ioclk: Pointer to the i/o clock between master and slave
>> + * @pdev: Pointer to the SPI controller platform device
>>   * @master: Pointer to the SPI Protocol master.
>>   * @cntrlr_info: Platform specific data for the controller this driver manages.
>>   * @lock: Controller specific lock.
>>   * @state: Set of FLAGS to indicate status.
>> - * @rx_dmach: Controller's DMA channel for Rx.
>> - * @tx_dmach: Controller's DMA channel for Tx.
>> + * @rx_dma: Controller's DMA channel for Rx.
>> + * @tx_dma: Controller's DMA channel for Tx.
>>   * @sfr_start: BUS address of SPI controller regs.
>>   * @regs: Pointer to ioremap'ed controller registers.
>> - * @irq: interrupt
>>   * @xfer_completion: To indicate completion of xfer task.
>>   * @cur_mode: Stores the active configuration of the controller.
>>   * @cur_bpw: Stores the active bits per word settings.
>>   * @cur_speed: Stores the active xfer clock speed.
>> + * @port_conf: Pointer to the SPI port configuration
>> + * @port_id: SPI port ID number
>>   */
>
>

-- 
Łukasz Stelmach
Samsung R&D Institute Poland
Samsung Electronics

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

[-- Attachment #2: Type: text/plain, Size: 176 bytes --]

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH 6/8] spi: spi-s3c64xx: Check return values

[Łukasz Stelmach]

Signed-off-by: Łukasz Stelmach <l.stelmach@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 505789f91fdf..27d77600a820 100644
--- a/drivers/spi/spi-s3c64xx.c
+++ b/drivers/spi/spi-s3c64xx.c
@@ -273,6 +273,7 @@ static void prepare_dma(struct s3c64xx_spi_dma_data *dma,
 	struct s3c64xx_spi_driver_data *sdd;
 	struct dma_slave_config config;
 	struct dma_async_tx_descriptor *desc;
+	int ret;
 
 	memset(&config, 0, sizeof(config));
 
@@ -296,11 +297,22 @@ static void prepare_dma(struct s3c64xx_spi_dma_data *dma,
 
 	desc = dmaengine_prep_slave_sg(dma->ch, sgt->sgl, sgt->nents,
 				       dma->direction, DMA_PREP_INTERRUPT);
+	if (!desc) {
+		dev_err(&sdd->pdev->dev, "unable to prepare %s scatterlist",
+			dma->direction == DMA_DEV_TO_MEM ? "rx" : "tx");
+		return;
+	}
 
 	desc->callback = s3c64xx_spi_dmacb;
 	desc->callback_param = dma;
 
 	dma->cookie = dmaengine_submit(desc);
+	ret = dma_submit_error(dma->cookie);
+	if (ret) {
+		dev_err(&sdd->pdev->dev, "dmaengine_submit() failed %d", ret);
+		return;
+	}
+
 	dma_async_issue_pending(dma->ch);
 }
 
-- 
2.26.2


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 6/8] spi: spi-s3c64xx: Check return values

[Krzysztof Kozlowski]

On Wed, Aug 19, 2020 at 02:32:06PM +0200, Łukasz Stelmach wrote:
> Signed-off-by: Łukasz Stelmach <l.stelmach@samsung.com>
> ---
>  drivers/spi/spi-s3c64xx.c | 12 ++++++++++++
>  1 file changed, 12 insertions(+)

Oh, come on, stop fixing the same local issue without fixing bigger
picture... or at least documenting why bigger picture does not have to be
fixed and simple 'return' is enough.

That's the third, same fix for the same problem.

https://lore.kernel.org/lkml/20190314064202.14864-1-kjlu@umn.edu/
https://lore.kernel.org/lkml/20170207204520.h2eo3yn5kge56lk7@kozik-lap/

Best regards,
Krzysztof

> 
> diff --git a/drivers/spi/spi-s3c64xx.c b/drivers/spi/spi-s3c64xx.c
> index 505789f91fdf..27d77600a820 100644
> --- a/drivers/spi/spi-s3c64xx.c
> +++ b/drivers/spi/spi-s3c64xx.c
> @@ -273,6 +273,7 @@ static void prepare_dma(struct s3c64xx_spi_dma_data *dma,
>  	struct s3c64xx_spi_driver_data *sdd;
>  	struct dma_slave_config config;
>  	struct dma_async_tx_descriptor *desc;
> +	int ret;
>  
>  	memset(&config, 0, sizeof(config));
>  
> @@ -296,11 +297,22 @@ static void prepare_dma(struct s3c64xx_spi_dma_data *dma,
>  
>  	desc = dmaengine_prep_slave_sg(dma->ch, sgt->sgl, sgt->nents,
>  				       dma->direction, DMA_PREP_INTERRUPT);
> +	if (!desc) {
> +		dev_err(&sdd->pdev->dev, "unable to prepare %s scatterlist",
> +			dma->direction == DMA_DEV_TO_MEM ? "rx" : "tx");
> +		return;
> +	}
>  
>  	desc->callback = s3c64xx_spi_dmacb;
>  	desc->callback_param = dma;
>  
>  	dma->cookie = dmaengine_submit(desc);
> +	ret = dma_submit_error(dma->cookie);
> +	if (ret) {
> +		dev_err(&sdd->pdev->dev, "dmaengine_submit() failed %d", ret);
> +		return;
> +	}
> +
>  	dma_async_issue_pending(dma->ch);
>  }
>  
> -- 
> 2.26.2
> 

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 6/8] spi: spi-s3c64xx: Check return values

[Lukasz Stelmach]

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

It was <2020-08-19 śro 14:48>, when Krzysztof Kozlowski wrote:
> On Wed, Aug 19, 2020 at 02:32:06PM +0200, Łukasz Stelmach wrote:
>> Signed-off-by: Łukasz Stelmach <l.stelmach@samsung.com>
>> ---
>>  drivers/spi/spi-s3c64xx.c | 12 ++++++++++++
>>  1 file changed, 12 insertions(+)
>
> Oh, come on, stop fixing the same local issue without fixing bigger
> picture... or at least documenting why bigger picture does not have to be
> fixed and simple 'return' is enough.
>
> That's the third, same fix for the same problem.
>
> https://lore.kernel.org/lkml/20190314064202.14864-1-kjlu@umn.edu/
> https://lore.kernel.org/lkml/20170207204520.h2eo3yn5kge56lk7@kozik-lap/

No wonder. There is a possible NULL dereference below. Now at least we
know something about conditions that led to this.

Should I drop the entire patch, or just the dmaengine_prep_slave_sg() part?


>> diff --git a/drivers/spi/spi-s3c64xx.c b/drivers/spi/spi-s3c64xx.c
>> index 505789f91fdf..27d77600a820 100644
>> --- a/drivers/spi/spi-s3c64xx.c
>> +++ b/drivers/spi/spi-s3c64xx.c
>> @@ -273,6 +273,7 @@ static void prepare_dma(struct s3c64xx_spi_dma_data *dma,
>>  	struct s3c64xx_spi_driver_data *sdd;
>>  	struct dma_slave_config config;
>>  	struct dma_async_tx_descriptor *desc;
>> +	int ret;
>>  
>>  	memset(&config, 0, sizeof(config));
>>  
>> @@ -296,11 +297,22 @@ static void prepare_dma(struct s3c64xx_spi_dma_data *dma,
>>  
>>  	desc = dmaengine_prep_slave_sg(dma->ch, sgt->sgl, sgt->nents,
>>  				       dma->direction, DMA_PREP_INTERRUPT);
>> +	if (!desc) {
>> +		dev_err(&sdd->pdev->dev, "unable to prepare %s scatterlist",
>> +			dma->direction == DMA_DEV_TO_MEM ? "rx" : "tx");
>> +		return;
>> +	}
>>  
>>  	desc->callback = s3c64xx_spi_dmacb;
>>  	desc->callback_param = dma;
>>  
>>  	dma->cookie = dmaengine_submit(desc);
>> +	ret = dma_submit_error(dma->cookie);
>> +	if (ret) {
>> +		dev_err(&sdd->pdev->dev, "dmaengine_submit() failed %d", ret);
>> +		return;
>> +	}
>> +
>>  	dma_async_issue_pending(dma->ch);
>>  }
>>  
>> -- 
>> 2.26.2
>> 
>
>

-- 
Łukasz Stelmach
Samsung R&D Institute Poland
Samsung Electronics

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

[-- Attachment #2: Type: text/plain, Size: 176 bytes --]

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 6/8] spi: spi-s3c64xx: Check return values

[Krzysztof Kozlowski]

On Wed, Aug 19, 2020 at 05:41:43PM +0200, Lukasz Stelmach wrote:
> It was <2020-08-19 śro 14:48>, when Krzysztof Kozlowski wrote:
> > On Wed, Aug 19, 2020 at 02:32:06PM +0200, Łukasz Stelmach wrote:
> >> Signed-off-by: Łukasz Stelmach <l.stelmach@samsung.com>
> >> ---
> >>  drivers/spi/spi-s3c64xx.c | 12 ++++++++++++
> >>  1 file changed, 12 insertions(+)
> >
> > Oh, come on, stop fixing the same local issue without fixing bigger
> > picture... or at least documenting why bigger picture does not have to be
> > fixed and simple 'return' is enough.
> >
> > That's the third, same fix for the same problem.
> >
> > https://lore.kernel.org/lkml/20190314064202.14864-1-kjlu@umn.edu/
> > https://lore.kernel.org/lkml/20170207204520.h2eo3yn5kge56lk7@kozik-lap/
> 
> No wonder. There is a possible NULL dereference below. Now at least we
> know something about conditions that led to this.
> 
> Should I drop the entire patch, or just the dmaengine_prep_slave_sg() part?

The best would be to really go through the call stack and handle the
error properly.

This means returning an error code and propagating it further. It is not
a trivial change...

Best regards,
Krzysztof

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH 7/8] spi: spi-s3c64xx: Increase transfer timeout

[Łukasz Stelmach]

Signed-off-by: Łukasz Stelmach <l.stelmach@samsung.com>
---
 drivers/spi/spi-s3c64xx.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/spi/spi-s3c64xx.c b/drivers/spi/spi-s3c64xx.c
index 27d77600a820..27db1e0f6f32 100644
--- a/drivers/spi/spi-s3c64xx.c
+++ b/drivers/spi/spi-s3c64xx.c
@@ -464,7 +464,8 @@ static int s3c64xx_wait_for_dma(struct s3c64xx_spi_driver_data *sdd,
 
 	/* millisecs to xfer 'len' bytes @ 'cur_speed' */
 	ms = xfer->len * 8 * 1000 / sdd->cur_speed;
-	ms += 10; /* some tolerance */
+	ms = (ms * 10) + 30;    /* some tolerance */
+	ms = max(ms, 100);      /* minimum timeout */
 
 	val = msecs_to_jiffies(ms) + 10;
 	val = wait_for_completion_timeout(&sdd->xfer_completion, val);
-- 
2.26.2


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 7/8] spi: spi-s3c64xx: Increase transfer timeout

[Krzysztof Kozlowski]

On Wed, Aug 19, 2020 at 02:32:07PM +0200, Łukasz Stelmach wrote:
> Signed-off-by: Łukasz Stelmach <l.stelmach@samsung.com>

Why? Everything works fine and suddenly minimum timeout is 100 ms?

Best regards,
Krzysztof

> ---
>  drivers/spi/spi-s3c64xx.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/spi/spi-s3c64xx.c b/drivers/spi/spi-s3c64xx.c
> index 27d77600a820..27db1e0f6f32 100644
> --- a/drivers/spi/spi-s3c64xx.c
> +++ b/drivers/spi/spi-s3c64xx.c
> @@ -464,7 +464,8 @@ static int s3c64xx_wait_for_dma(struct s3c64xx_spi_driver_data *sdd,
>  
>  	/* millisecs to xfer 'len' bytes @ 'cur_speed' */
>  	ms = xfer->len * 8 * 1000 / sdd->cur_speed;
> -	ms += 10; /* some tolerance */
> +	ms = (ms * 10) + 30;    /* some tolerance */
> +	ms = max(ms, 100);      /* minimum timeout */
>  
>  	val = msecs_to_jiffies(ms) + 10;
>  	val = wait_for_completion_timeout(&sdd->xfer_completion, val);
> -- 
> 2.26.2
> 

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 7/8] spi: spi-s3c64xx: Increase transfer timeout

[Lukasz Stelmach]

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

It was <2020-08-19 śro 14:49>, when Krzysztof Kozlowski wrote:
> On Wed, Aug 19, 2020 at 02:32:07PM +0200, Łukasz Stelmach wrote:
>> Signed-off-by: Łukasz Stelmach <l.stelmach@samsung.com>
>
> Why? Everything works fine and suddenly minimum timeout is 100 ms?

Actually I am not 100% sure the max() call is required, maybe +30 is
enough. Definitely some minimum value is required because for small
tranfers (100s of bytes) ms is 0 after the first assignment.

>> ---
>>  drivers/spi/spi-s3c64xx.c | 3 ++-
>>  1 file changed, 2 insertions(+), 1 deletion(-)
>> 
>> diff --git a/drivers/spi/spi-s3c64xx.c b/drivers/spi/spi-s3c64xx.c
>> index 27d77600a820..27db1e0f6f32 100644
>> --- a/drivers/spi/spi-s3c64xx.c
>> +++ b/drivers/spi/spi-s3c64xx.c
>> @@ -464,7 +464,8 @@ static int s3c64xx_wait_for_dma(struct s3c64xx_spi_driver_data *sdd,
>>  
>>  	/* millisecs to xfer 'len' bytes @ 'cur_speed' */
>>  	ms = xfer->len * 8 * 1000 / sdd->cur_speed;
>> -	ms += 10; /* some tolerance */
>> +	ms = (ms * 10) + 30;    /* some tolerance */
>> +	ms = max(ms, 100);      /* minimum timeout */
>>  
>>  	val = msecs_to_jiffies(ms) + 10;
>>  	val = wait_for_completion_timeout(&sdd->xfer_completion, val);
>> -- 
>> 2.26.2
>> 
>
>

-- 
Łukasz Stelmach
Samsung R&D Institute Poland
Samsung Electronics

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

[-- Attachment #2: Type: text/plain, Size: 176 bytes --]

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 7/8] spi: spi-s3c64xx: Increase transfer timeout

[Krzysztof Kozlowski]

On Wed, Aug 19, 2020 at 03:39:56PM +0200, Lukasz Stelmach wrote:
> It was <2020-08-19 śro 14:49>, when Krzysztof Kozlowski wrote:
> > On Wed, Aug 19, 2020 at 02:32:07PM +0200, Łukasz Stelmach wrote:
> >> Signed-off-by: Łukasz Stelmach <l.stelmach@samsung.com>
> >
> > Why? Everything works fine and suddenly minimum timeout is 100 ms?
> 
> Actually I am not 100% sure the max() call is required, maybe +30 is
> enough. Definitely some minimum value is required because for small
> tranfers (100s of bytes) ms is 0 after the first assignment.

Sure, just please describe it all in commit msg. All these questions
"why?" came because of lack of explanation in commit msg.

I guess minimum timeout 100 ms for each transfer is quite high, so maybe
just bump the tolerance to 30 which also would be a minimum timeout.

Best regards,
Krzysztof


> 
> >> ---
> >>  drivers/spi/spi-s3c64xx.c | 3 ++-
> >>  1 file changed, 2 insertions(+), 1 deletion(-)
> >> 
> >> diff --git a/drivers/spi/spi-s3c64xx.c b/drivers/spi/spi-s3c64xx.c
> >> index 27d77600a820..27db1e0f6f32 100644
> >> --- a/drivers/spi/spi-s3c64xx.c
> >> +++ b/drivers/spi/spi-s3c64xx.c
> >> @@ -464,7 +464,8 @@ static int s3c64xx_wait_for_dma(struct s3c64xx_spi_driver_data *sdd,
> >>  
> >>  	/* millisecs to xfer 'len' bytes @ 'cur_speed' */
> >>  	ms = xfer->len * 8 * 1000 / sdd->cur_speed;
> >> -	ms += 10; /* some tolerance */
> >> +	ms = (ms * 10) + 30;    /* some tolerance */
> >> +	ms = max(ms, 100);      /* minimum timeout */
> >>  
> >>  	val = msecs_to_jiffies(ms) + 10;
> >>  	val = wait_for_completion_timeout(&sdd->xfer_completion, val);
> >> -- 
> >> 2.26.2
> >> 
> >
> >
> 
> -- 
> Łukasz Stelmach
> Samsung R&D Institute Poland
> Samsung Electronics



_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH 8/8] spi: spi-s3c64xx: Turn on interrupts upon resume

[Łukasz Stelmach]

Signed-off-by: Łukasz Stelmach <l.stelmach@samsung.com>
---
 drivers/spi/spi-s3c64xx.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/spi/spi-s3c64xx.c b/drivers/spi/spi-s3c64xx.c
index 27db1e0f6f32..5dcad43fb847 100644
--- a/drivers/spi/spi-s3c64xx.c
+++ b/drivers/spi/spi-s3c64xx.c
@@ -1356,6 +1356,10 @@ static int s3c64xx_spi_runtime_resume(struct device *dev)
 
 	s3c64xx_spi_hwinit(sdd);
 
+	writel(S3C64XX_SPI_INT_RX_OVERRUN_EN | S3C64XX_SPI_INT_RX_UNDERRUN_EN |
+	       S3C64XX_SPI_INT_TX_OVERRUN_EN | S3C64XX_SPI_INT_TX_UNDERRUN_EN,
+	       sdd->regs + S3C64XX_SPI_INT_EN);
+
 	return 0;
 
 err_disable_src_clk:
-- 
2.26.2


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 8/8] spi: spi-s3c64xx: Turn on interrupts upon resume

[Krzysztof Kozlowski]

On Wed, Aug 19, 2020 at 02:32:08PM +0200, Łukasz Stelmach wrote:
> Signed-off-by: Łukasz Stelmach <l.stelmach@samsung.com>
> ---
>  drivers/spi/spi-s3c64xx.c | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/drivers/spi/spi-s3c64xx.c b/drivers/spi/spi-s3c64xx.c
> index 27db1e0f6f32..5dcad43fb847 100644
> --- a/drivers/spi/spi-s3c64xx.c
> +++ b/drivers/spi/spi-s3c64xx.c
> @@ -1356,6 +1356,10 @@ static int s3c64xx_spi_runtime_resume(struct device *dev)
>  
>  	s3c64xx_spi_hwinit(sdd);
>  
> +	writel(S3C64XX_SPI_INT_RX_OVERRUN_EN | S3C64XX_SPI_INT_RX_UNDERRUN_EN |
> +	       S3C64XX_SPI_INT_TX_OVERRUN_EN | S3C64XX_SPI_INT_TX_UNDERRUN_EN,
> +	       sdd->regs + S3C64XX_SPI_INT_EN);
> +

Makes sense but you should explain in the commit msg what happens
without this (or why they are not enabled).

Best regards,
Krzysztof

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 8/8] spi: spi-s3c64xx: Turn on interrupts upon resume

[Lukasz Stelmach]

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

It was <2020-08-19 śro 14:53>, when Krzysztof Kozlowski wrote:
> On Wed, Aug 19, 2020 at 02:32:08PM +0200, Łukasz Stelmach wrote:
>> Signed-off-by: Łukasz Stelmach <l.stelmach@samsung.com>
>> ---
>>  drivers/spi/spi-s3c64xx.c | 4 ++++
>>  1 file changed, 4 insertions(+)
>> 
>> diff --git a/drivers/spi/spi-s3c64xx.c b/drivers/spi/spi-s3c64xx.c
>> index 27db1e0f6f32..5dcad43fb847 100644
>> --- a/drivers/spi/spi-s3c64xx.c
>> +++ b/drivers/spi/spi-s3c64xx.c
>> @@ -1356,6 +1356,10 @@ static int s3c64xx_spi_runtime_resume(struct device *dev)
>>  
>>  	s3c64xx_spi_hwinit(sdd);
>>  
>> +	writel(S3C64XX_SPI_INT_RX_OVERRUN_EN | S3C64XX_SPI_INT_RX_UNDERRUN_EN |
>> +	       S3C64XX_SPI_INT_TX_OVERRUN_EN | S3C64XX_SPI_INT_TX_UNDERRUN_EN,
>> +	       sdd->regs + S3C64XX_SPI_INT_EN);
>> +
>
> Makes sense but you should explain in the commit msg what happens
> without this (or why they are not enabled).

Done, thanks.

-- 
Łukasz Stelmach
Samsung R&D Institute Poland
Samsung Electronics

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

[-- Attachment #2: Type: text/plain, Size: 176 bytes --]

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 0/8] Some fixes for spi-s3c64xx

[Krzysztof Kozlowski]

On Wed, Aug 19, 2020 at 02:32:00PM +0200, Łukasz Stelmach wrote:
> This is a series of fixes created during porting a device driver (these
> patches will be released soon too) for an SPI device to the current kernel.
> 
> The two most important are 
> 
>   spi: spi-s3c64xx: swap s3c64xx_spi_set_cs() and s3c64xx_enable_datapath()
>   spi: spi-s3s64xx: Add S3C64XX_SPI_QUIRK_CS_AUTO for Exynos3250
> 
> Without them DMA transfers larger than 512 bytes from the SPI controller
> would fail.

If these are two most important patches here, there should have a commit
message explaining their importance.

Best regards,
Krzysztof

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel