On Wed, May 29, 2024 at 10:07:37AM +0200, Nuno Sá wrote: > On Sun, 2024-05-26 at 18:35 +0100, Conor Dooley wrote: > > On Thu, May 23, 2024 at 02:15:35PM +0200, Nuno Sá wrote: > > > On Wed, 2024-05-22 at 19:24 +0100, Conor Dooley wrote: > > > 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. > > > > Can you explain what you mean by "same property on the peripheral"? I > > would expect a peripheral to state its trigger period (just like how it > > states the max frequency) and for the trigger period not to appear in > > the controller. > > > > Just have the same 'pwms' property on both the controller and peripheral... Yeah, no... Opinion unchanged since my last message. > > I think a way that this could be modelled to reduce some software > > complexity is considering that the periodic trigger is a clock, not > > a PWM, provided you are only interested in the period. That'd give you > > an interface that was less concerned about what the provider of the > > periodic trigger is. After all, I doubt the ADC cares how you decide to > > generate the trigger, as long as the periodicity is correct. > > With the examples provided, you'd get something like: > > > > Unfortunately that's not the case. Ahh, that sucks. Oh well. > For instance, in the design on the link I gave you > on the last reply we do have an averaging mode where we actually need an offset > (effort for supporting that in PWM is already ongoing) between the offload trigger > and the peripheral conversion signal (so assuming we only care about period will fail > pretty soon :)). > > > 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. > > > > I don't think you're overlooking things at all, I'm intentionally being > > a bit difficult and ignoring what may be convenient for the adc driver. > > This is being presented as a solution to a generic problem (and I think > > you're right to do that), but it feels as if the one implementation is > > all that's being considered here and I'm trying to ensure that what we > > end up with doesn't make limiting assumptions. > > Yeah, I know and I do agree we need something generic enough and not something that > only fits a couple usecases. If only we had another user... I suppose you lads are the market leader in these kinds of devices. If I did happen to know if Microchip was working on anything similar (which I don't, I work on FPGAs not these kinds of devices) I couldn't even tell you. I suppose I could ask around and see. Do you know if TI is doing anything along these lines? > > Part of me says "sure, hook the DMAs up to the devices, as that's what > > happens for other IIO devices" but at the same time I recognise that the > > DMA isn't actually hooked up like that and the other IIO devices I see > > like that are all actually on the SoC, rather than connected over SPI. > > Yeah, I know... But note (but again, only for ADI designs) that the DMA role is > solely for carrying the peripheral data. It is done like this so everything works in > HW and there's no need for SW to deal with the samples at all. I mean, only the > userspace app touches the samples. > > TBH, the DMA is the bit that worries me the most as it may be overly complex to share > buffers (using dma-buf or something else) from the spi controller back to consumers > of it (IIO in this case). And I mean sharing in a way that there's no need to touch > the buffers. <snip> > Maybe having an offload dedicated API (through spi) to get/share a DMA handle would > be acceptable. Then we could add support to "import" it in the IIO core. Then it > would be up to the controller to accept or not to share the handle (in some cases the > controller could really want to have the control of the DMA transfers). Yeah, that is about what I was thinking. I wasn't expecting the spi code to grow handing for dmabuf or anything like that, just a way for the offload consumer to say "yo, can you tell me what dma buffer I can use?". Unless (until?) there's some controller that wants to manage it, I think that'd be sufficient?
Attachment:
signature.asc
Description: PGP signature
