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

[Lukasz Stelmach]

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: signature.asc
Description: PGP signature