| Message ID | 20200819123208.12337-2-l.stelmach@samsung.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [1/8] spi: spi-s3c64xx: swap s3c64xx_spi_set_cs() and s3c64xx_enable_datapath() | expand |
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 >
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 >>
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
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.
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.
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?
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.
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)
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(-)