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

[David Lechner <dlechner@xxxxxxxxxxxx>]

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@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:
>>>

...

>> This time, the periodic trigger (PWM) is connected to the pin on the
>> ADC that triggers a sample conversion (CNV). The ADC has a BUSY output
>> that will go high at the start of the conversion and go low at the end
>> of the conversion. The BUSY output of the ADC is wired as the hardware
>> trigger input of the offload.
>>
>> In this case would we still consider the PWM as part of the SPI
>> controller/offload since it can only be used in conjunction with the
>> SPI offload? It isn't connected to it at all though.
> 
> No, in this case the ADC is a PWM consumer and the offload engine is
> not. The ADC is a "trigger" provider and the SPI offload engine is a
> trigger consumer.

Makes sense.

...


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

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



> 
> 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.
> 
> continuation:
> If offload engines have their own register region in the memory map,
> 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...
>