Thread (1 message) 1 message, 1 author, 2020-08-20

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

From: Lukasz Stelmach <l.stelmach@samsung.com>
Date: 2020-08-20 11:05:11
Also in: linux-samsung-soc, linux-spi, lkml

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:
quoted
It was <2020-08-19 śro 14:16>, when Mark Brown wrote:
quoted
On Wed, Aug 19, 2020 at 02:58:22PM +0200, Krzysztof Kozlowski wrote:
quoted
On Wed, Aug 19, 2020 at 02:51:27PM +0200, Lukasz Stelmach wrote:
quoted
    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.
quoted
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.
quoted
quoted
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
Keyboard shortcuts
hback out one level
jnext message in thread
kprevious message in thread
ldrill in
Escclose help / fold thread tree
?toggle this help