Date: Thu, 30 May 2024 19:42:54 +0100
From: Conor Dooley <conor@...nel.org>
To: David Lechner <dlechner@...libre.com>
Cc: Mark Brown <broonie@...nel.org>, Jonathan Cameron <jic23@...nel.org>,
	Rob Herring <robh@...nel.org>,
	Krzysztof Kozlowski <krzk+dt@...nel.org>,
	Conor Dooley <conor+dt@...nel.org>,
	Nuno Sá <nuno.sa@...log.com>,
	Michael Hennerich <Michael.Hennerich@...log.com>,
	Lars-Peter Clausen <lars@...afoo.de>,
	David Jander <david@...tonic.nl>,
	Martin Sperl <kernel@...tin.sperl.org>, linux-spi@...r.kernel.org,
	devicetree@...r.kernel.org, linux-kernel@...r.kernel.org,
	linux-iio@...r.kernel.org
Subject: Re: [PATCH RFC v2 1/8] spi: dt-bindings: spi-peripheral-props: add
 spi-offloads property

On Thu, May 23, 2024 at 09:28:54AM -0500, David Lechner wrote:
> On 5/22/24 1:24 PM, 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@...nel.org> wrote:

> >>> In fact, looking at the documentation for the "spi engine" some more, I
> >>> am even less convinced that the right place for describing the offload is
> >>> the peripheral as I (finally?) noticed that the registers for the offload
> >>> engine are mapped directly into the "spi engine"'s memory map, rather than
> >>> it being a entirely separate IP in the FPGA fabric.
> >>
> >> True, but we don't use these registers, e.g., to configure the
> >> sampling frequency of a trigger (if it can even be done). That is done
> >> in a completely separate IP block with it's own registers.
> > 
> > Where is the binding for that IP block? I think describing that is
> > pretty key. goto continuation;
> 
> For the real-world case I used to test this series, it is an AXI PWMGEN
> [1] that is providing the trigger event source. It has a typical PWM
> provider binding with #pwm-cells [2].
> 
> Calling this a "trigger" provider to the SPI offload instance just like the
> case above where the ADC is directly connected as the offload trigger makes
> sense to me.
> 
> What I was going for in this patch (slightly revised to use #cells) is that
> this trigger provider, whatever it is, is selected by one of the cells of
> #spi-offload-cells. It doesn't seem like there should be a special case for
> if the trigger provider is a clock or PWM where the SPI controller node
> becomes a consumer of the clock or PWM provider rather than just describing
> the trigger relationship.
> 
> For example, supposing we had an FPGA/HDL that could handle all 3 wiring
> configurations we have discussed so far. A) PWM connected directly to SPI
> offload as trigger, B) PWM connected to CNV of ADC and BSY of ADC connected
> to SPI offload as trigger, C) self clocked ADC with RDY of ADC connected
> to SPI offload as trigger. So the DT would have:
> 
> controller:
> #spi-offload-cells = <2>: /* 1st cell = offload instance
>                            * 2nd cell = trigger provider */
> 
> peripheral (choose one based on actual wiring):
> spi-offloads = <0 0>; /* case A */
> spi-offloads = <0 1>; /* case B */
> spi-offloads = <0 2>; /* case C */
> 
> 
> As to what is the actual consumer of the PWM provider in each of these
> cases...
> 
> * C is easy. There isn't a PWM provider since the ADC is self-clocked.
> * B, as discussed elsewhere is fairly straight forward. The ADC node is
>   the consumer since the PWM is connected directly to the ADC.
> * A is the one we need to figure out. I'm proposing that the PWM consumer
>   should be whatever kind of composite device node we come up with that
>   also solves the issue described below about where does the CRC checker
>   (or whatever) go. I think we are in agreement here at least on the point
>   that it doesn't belong in the SPI controller node?

I think C and B are correct here. The term "composite device" scares me,
but yeah the PWM gets connected to the device that encompasses the
offload engine. In case B and C, the trigger signals are connected to
the offload engine - what property are we using to describe that?
If this were not a "passive" process and the OS was actually involved
in kicking off an action when the ADC asserted the !BSY signal,
interrupts would make sense, but I'm not sure here.

Maybe interrupts is the right thing to do, because that would also make
sense in the case that we used the busy signal without an offload and
plumbed it into a regular interrupt controller?

gpio0: gpio@...20000 {
	compatible = "microchip,mpfs-gpio";
	reg = <0x0 0x20120000 0x0 0x1000>;
	interrupt-parent = <&plic>;
	interrupt-controller;
	#interrupt-cells = <1>;
	clocks = <&clkcfg CLK_GPIO0>;
	gpio-controller;
	#gpio-cells = <2>;
};

adc {
	interrupt-parent = <&gpio0>;
	interrupts = <0 HIGH>;
};

I suppose then the offload needs to be an interrupt provider too, even
if no irqchip is ever actually created for it. I dunno how tf that's
gonna work out on the software side though? If you have spi-offloads you
interpret the interrupts property differently (IOW you ignore it)?

Or were you thinking of not really hooking that up at all, but instead
just saying checking for whatever x is in spi-offloads = <0 x> and
defining 2 to mean "active low" and 1 to mean "active high" etc?

Once again, had to take a break to make food in the middle of responding
here, so I hope I didn't write something incomprehensible.

Thanks,
Conor.

> 
> [1]: http://analogdevicesinc.github.io/hdl/library/axi_pwm_gen/index.html
> [2]: https://lore.kernel.org/linux-pwm/20240424125850.4189116-2-tgamblin@baylibre.com/


Download attachment "signature.asc" of type "application/pgp-signature" (229 bytes)

Powered by blists - more mailing lists