Re: [PATCH RFC v2 1/8] spi: dt-bindings: spi-peripheral-props: add spi-offloads property

[David Lechner <dlechner@baylibre.com>]

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á
> 
>