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

[Conor Dooley]

On Thu, May 23, 2024 at 10:05:49AM -0500, David Lechner wrote:
> 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@xxxxxxxxxx> 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@xxxxxxxxxx> 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.

I don't think there was a question here at all. I was simply stating
that if the offload engine was not just a subordinate feature of the SPI
controller, but also provided additional data processing features then
treating the offload engine as a component of the SPI controller would
not be accurate.

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

Seems to be that the CRC checking is a separate piece of IP though, and
so is not part of the offload engine at all, so my concern was
misplaced. I think whether or not you have registers to control it, it
should be represented in DT. How do you know it is there otherwise?

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

You wouldn't necessarily need to write a driver for it, you could reach
out and turn it on from the backend consumer for example. And, obviously
there may be other ways that the FPGA design is configured where the
clock is not shared with the SPI controller or the offload engine.

Attachment: signature.asc
Description: PGP signature