Received-SPF: pass (google.com: domain of linux-kernel+bounces-187690-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:4601:e00::3 as permitted sender) client-ip=2604:1380:4601:e00::3;
Message-ID: <6e929426-25fa-4e91-8790-0774d59b34e0@baylibre.com>
Date: Thu, 23 May 2024 10:05:49 -0500
Precedence: bulk
MIME-Version: 1.0
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH RFC v2 1/8] spi: dt-bindings: spi-peripheral-props: add
 spi-offloads property
To: =?UTF-8?Q?Nuno_S=C3=A1?= <noname.nuno@gmail.com>,
 Conor Dooley <conor@kernel.org>
Cc: Mark Brown <broonie@kernel.org>, Jonathan Cameron <jic23@kernel.org>,
 Rob Herring <robh@kernel.org>, Krzysztof Kozlowski <krzk+dt@kernel.org>,
 Conor Dooley <conor+dt@kernel.org>, =?UTF-8?Q?Nuno_S=C3=A1?=
 <nuno.sa@analog.com>, Michael Hennerich <Michael.Hennerich@analog.com>,
 Lars-Peter Clausen <lars@metafoo.de>, David Jander <david@protonic.nl>,
 Martin Sperl <kernel@martin.sperl.org>, linux-spi@vger.kernel.org,
 devicetree@vger.kernel.org, linux-kernel@vger.kernel.org,
 linux-iio@vger.kernel.org
References: <20240510-dlech-mainline-spi-engine-offload-2-v2-0-8707a870c435@baylibre.com>
 <20240510-dlech-mainline-spi-engine-offload-2-v2-1-8707a870c435@baylibre.com>
 <20240513-headsman-hacking-d51fcc811695@spud>
 <CAMknhBE5XJzhdJ=PQUXiubw_CiCLcn1jihiscnQZUzDWMASPKw@mail.gmail.com>
 <20240514-aspire-ascension-449556da3615@spud>
 <CAMknhBFFpEGcMoLo5gsC11Syv+CwUM0mnq1yDMUzL1uutUtB+Q@mail.gmail.com>
 <20240516-rudder-reburial-dcf300504c0a@spud>
 <CAMknhBF_s0btus4yqPe-T=F3z7Asi9KkRGsGr7FHDFi=k4EQjw@mail.gmail.com>
 <20240519-abreast-haziness-096a57ef57d3@spud>
 <CAMknhBHvEse2FyDoBXR1PvymGpSGq8dotKfm+8XH+0+k+xKtQw@mail.gmail.com>
 <20240522-gullible-ibuprofen-cf9111c25f6f@spud>
 <5ad0b5782434eaf4cf565cffb0e4c14b7414ae38.camel@gmail.com>
Content-Language: en-US
From: David Lechner <dlechner@baylibre.com>
In-Reply-To: <5ad0b5782434eaf4cf565cffb0e4c14b7414ae38.camel@gmail.com>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

On 5/23/24 7:15 AM, Nuno Sá wrote:
> On Wed, 2024-05-22 at 19:24 +0100, Conor Dooley wrote:
>> On Tue, May 21, 2024 at 09:54:39AM -0500, David Lechner wrote:
>>> On Sun, May 19, 2024 at 7:53 AM Conor Dooley <conor@kernel.org> wrote:
>>>>
>>>> On Fri, May 17, 2024 at 11:51:58AM -0500, David Lechner wrote:
>>>>> On Thu, May 16, 2024 at 4:32 PM Conor Dooley <conor@kernel.org> wrote:
>>>>>> On Tue, May 14, 2024 at 05:56:47PM -0500, David Lechner wrote:
>>>>

..

>> To remind myself, "Application 2" featured an offload engine designed
>> specifically to work with a particular data format that would strip a
>> CRC byte and check the validity of the data stream.
>>
> 
> I think the data manipulation is not really a property of the engine. Typically data
> going out of the offload engine goes into another "data reorder" block that is pure
> HW.
> 
>> I think you're right something like that is a stretch to say that that
>> is a feature of the SPI controller - but I still don't believe that
>> modelling it as part of the ADC is correct. I don't fully understand the
>> io-backends and how they work yet, but the features you describe there
>> seem like something that should/could be modelled as one, with its own
>> node and compatible etc. Describing custom RTL stuff ain't always
>> strightforward, but the stuff from Analog is versioned and documented
>> etc so it shouldn't be quite that hard.
>>
> 
> Putting this in io-backends is likely a stretch but one thing to add is that the
> peripheral is always (I think) kind of the consumer of the resources. Taking the
> trigger (PWM) as an example and even when it is directly connected with the offload
> block, the peripheral still needs to know about it. Think of sampling frequency...
> The period of the trigger signal is strictly connected with the sampling frequency of
> the peripheral for example. So I see 2 things:
> 
> 1) Enabling/Disabling the trigger could be easily done from the peripheral even with
> the resource in the spi engine. I think David already has some code in the series
> that would make this trivial and so having the property in the spi controller brings
> no added complexity.
> 
> 2) Controlling things like the trigger period/sample_rate. This could be harder to do
> over SPI (or making it generic enough) so we would still need to have the same
> property on the peripheral (even if not directly connected to it). I kind of agree
> with David that having the property both in the peripheral and controller is a bit
> weird.
> 
> And the DMA block is a complete different story. Sharing that data back with the
> peripheral driver (in this case, the IIO subsystem) would be very interesting at the
> very least. Note that the DMA block is not really something that is part of the
> controller nor the offload block. It is an external block that gets the data coming
> out of the offload engine (or the data reorder block). In IIO, we already have a DMA
> buffer interface so users of the peripheral can get the data without any intervention
> of the driver (on the data). We "just" enable buffering and then everything happens
> on HW and userspace can start requesting data. If we are going to attach the DMA in
> the controller, I have no idea how we can handle it. Moreover, the offload it's
> really just a way of replaying the same spi transfer over and over and that happens
> in HW so I'm not sure how we could "integrate" that with dmaengine.
> 
> But maybe I'm overlooking things... And thinking more in how this can be done in SW
> rather than what makes sense from an HW perspective.
> 
> 
>> continuation:
>> If offload engines have their own register region in the memory map,
> 
> 
> Don't think it has it's own register region... David?

I think the question here was if the CRC checker IP block (or descrambler shown
in the link below, or whatever) had registers in the offload/SPI controller
to control that extra part or if they had their own dedicated registers. So far,
these have been fixed-purpose, so have no registers at all. But I could see
needing a register, e.g. for turning it on or off. In this case, I think it
does become something like an io-backend. Or would we add this on/off switch
to the AXI SPI Engine registers?

Also, as shown in the link below, the extra bits share a clock domain with the
AXI SPI Engine. So, yes, technically I suppose they could/should? be independent
consumers of the same clock like Conor suggests below. It does seems kind of
goofy if we have to write a driver just to turn on a clock that is already
guaranteed to be on though.

> 
>> their own resources (the RTL is gonna need at the very least a clock)
>> and potentially also provide other services (like acting as an
>> io-backend type device that pre-processes data) it doesn't sound like
>> either the controller or peripheral nodes are the right place for these
>> properties. And uh, spi-offloads gets a phandle once more...
>>
> 
> But then it would be valid for both peripheral and controller to reference that
> phandle right (and get the resources of interest)?
> 
> FWIW, maybe look at the below usecase. It has some block diagrams that may be helpful
> to you:
> 
> https://wiki.analog.com/resources/eval/user-guides/ad463x/hdl
> 
> - Nuno Sá
> 
>