Re: [cadence-spi] daisy chain

[Adrian Fiergolski <adrian.fiergolski@xxxxxxxxxxxxx>]

Hi Geert and Rob,

Thank you for your comments.

On 16.06.2020 00:22, Rob Herring wrote:
> On Mon, Jun 15, 2020 at 3:30 PM Geert Uytterhoeven <geert@xxxxxxxxxxxxxx> wrote:
>> Hi Adrian,
>>
>> On Mon, Jun 15, 2020 at 6:05 PM Adrian Fiergolski
>> <adrian.fiergolski@xxxxxxxxxxxxx> wrote:
>>> On 15.06.2020 16:40, Geert Uytterhoeven wrote:
>>>> On Mon, Jun 15, 2020 at 4:06 PM Adrian Fiergolski
>>>> <adrian.fiergolski@xxxxxxxxxxxxx> wrote:
>>>>> Sorry for the typo in the example device tree:
>>>>>
>>>>> On 15.06.2020 15:57, Adrian Fiergolski wrote:
>>>>>> On 15.06.2020 15:07, Geert Uytterhoeven wrote:
>>>>>>> On Mon, Jun 15, 2020 at 3:01 PM Adrian Fiergolski
>>>>>>> <adrian.fiergolski@xxxxxxxxxxxxx> wrote:
>>>>>>>> On 13.06.2020 09:33, Geert Uytterhoeven wrote:
>>>>>>>>> On Fri, Jun 12, 2020 at 6:26 PM Adrian Fiergolski
>>>>>>>>> <adrian.fiergolski@xxxxxxxxxxxxx> wrote:
>>>>>>>>> I have a daisy chain of three ltc2634 slaves (iio/dac/ltc2632.c)
>>>>>>>>> connected to a single chip select of the cadence-spi master. I have the
>>>>>>>>> impression such a configuration is supported by none of those two
>>>>>>>>> drivers. I could try to extend both, however, I haven't found any other
>>>>>>>>> SPI driver, where I could find implementation inspiration. Is it
>>>>>>>>> supported by kernel?
>>>>>>>>>
>>>>>>>>> drivers/gpio/gpio-max3191x.c supports "#daisy-chained-devices".
>>>>>>>>> drivers/gpio/gpio-74x164.c supports multiple shift registers through the
>>>>>>>>> "registers-number" DT property.
>>>>>>>>>
>>>>>>>>> So both drivers handle this in their SPI slave drivers.
>>>>>>>>>
>>>>>>>>> Of course this does not handle the mixed case, i.e. daisy-chaining
>>>>>>>>> different types of devices.
>>>>>>>>>
>>>>>>>>> The documentation mentions only about the common 'daisy-chained-devices'
>>>>>>>>> property (devicetree/bindings/common-properties.txt). However, in order
>>>>>>>>> to try to implement it in the master driver, IMHO, the spi subsystem
>>>>>>>>> would need to have a call 'no-operation' to other nodes on the
>>>>>>>>> daisy-chain, which are not addressed by the given SPI access. Is there
>>>>>>>>> any recommended approach to address this case?
>>>>>>>>>
>>>>>>>>> Supporting this in a generic way would indeed be nice, as it would mean
>>>>>>>>> individual SPI slave drivers no longer have to care about it.
>>>>>>>>> However, that may be difficult, as the master needs to known which
>>>>>>>>> dummy (no-op) data is safe to shift through the non-addresses SPI slaves.
>>>>>>>> In fact, the ultimate solution would be to have it solved at the level
>>>>>>>> of the spi subsystem:
>>>>>>>>
>>>>>>>>   * /spi_device struct/ would contain extra callback which returns msg
>>>>>>>>     to be sent for no operation.
>>>>>>>>   * spi_device struct would contain a pointer to the list describing the
>>>>>>>>     given daisy chain (list of spi_devices on the chain)
>>>>>>>>   * /spi_device struct /would contain extra u8 daisy_chain_msg_length
>>>>>>>>     indicating length of a command of the addressed device if it's on
>>>>>>>>     the daisy chain
>>>>>>>>     For example, in case of the ltc2634 device, the regular message
>>>>>>>>     consists of 24 bits, but when device is a part of a daisy chain, the
>>>>>>>>     messages are 32 bits. This 32 would be stored in
>>>>>>>>     /daisy_chain_msg_length./
>>>>>>>>   * When /spi_write/ was called (include/linux/spi/spi.h), the
>>>>>>>>     /spi_message_init_with_transfer/ would create a msg of length equal
>>>>>>>>     to a sum of /daisy_chain_msg_length/ of all devices on the chain.
>>>>>>>>     Afterwards, in /spi_message_init_with_transfers/, the actual message
>>>>>>>>     would be filled with the command of the addressed device on the
>>>>>>>>     chain and no_operation content for all other devices on the chain
>>>>>>>>     not being addressed
>>>>>>> Sounds good to me.
>>>>>>>
>>>>>>>>   * I think in such a case, the /daisy-chained-devices /property would
>>>>>>>>     be not used, as chains would be build basing on the assigned
>>>>>>>>     chipselect (reg property).
>>>>>>> So you still have to describe the chain in DT in some way.
>>>>>>> As there can be only a single sub node with the same unit address
>>>>>>> (= chip select), you probably need a container with that address, which
>>>>>>> would contain all devices in the chain, in order (unit addresses 0, 1, ...).
>>>>>> Good point. So maybe at the level of the device tree, it could be
>>>>>> described like that (based on the spi-cadence example):
>>>>>>
>>>>>>         spi0: spi@ff040000 {
>>>>>>             compatible = "cdns,spi-r1p6";
>>>>>>             status = "disabled";
>>>>>>             interrupt-parent = <&gic>;
>>>>>>             interrupts = <0 19 4>;
>>>>>>             reg = <0x0 0xff040000 0x0 0x1000>;
>>>>>>             clock-names = "ref_clk", "pclk";
>>>>>>             #address-cells = <1>;
>>>>>>             #size-cells = <0>;
>>>>>>             power-domains = <&zynqmp_firmware PD_SPI_0>;
>>>>>>             daisy-chain0 : daisy_chain@0 {
>>>>>>                #address-cells = <1>;
>>>>>>                #size-cells = <0>;
>>>>>>                reg = <0>;
>>>>>>                daisy-chained-devices = 2;
>>>>>>
>>>>>>                dac0: ltc2632@0 {
>>>>>>                    compatible = "lltc,ltc2634-l12";
>>>>>>                    reg = <0>;
>>>>>>                    spi-max-frequency = <1000000>;
>>>>>>                };
>>>>>>                dac1: ltc2632@1 {
>>>>>>                    compatible = "lltc,ltc2634-l12";
>>>>>>                    reg = <1>;
>>>>>>                    spi-max-frequency = <2000000>;
>>>>>>                };
>>>>>>            };
>>>>>>         };
>>>>>         spi0: spi@ff040000 {
>>>>>             compatible = "cdns,spi-r1p6";
>>>>>             status = "disabled";
>>>>>             interrupt-parent = <&gic>;
>>>>>             interrupts = <0 19 4>;
>>>>>             reg = <0x0 0xff040000 0x0 0x1000>;
>>>>>             clock-names = "ref_clk", "pclk";
>>>>>             #address-cells = <1>;
>>>>>             #size-cells = <0>;
>>>>>             power-domains = <&zynqmp_firmware PD_SPI_0>;
>>>>>             daisy-chain0 : daisy_chain@0 {
>>>>>                #address-cells = <1>;
>>>>>                #size-cells = <0>;
>>>>>                #daisy-chained-devices = <2>;
>>>> You probably want a proper "compatible" value here instead.
>>>> I don't think "#daisy-chained-devices" is needed, it can be derived
>>>> from the number of subnodes.
> This is not how '#daisy-chained-devices' works either. The chained
> devices are represented as a single node and a single driver instance
> handles multiple physical devices.
>
> The above looks like mux'ed access rather than creating a 'super'
> device. Would you want to program all N chips in one SPI transfer or
> it's one device at a time? (We should be careful with the bindings
> that we're not encoding that into the binding as that could evolve
> with the OS.)

Well, I think the described concept of the daisy chain doesn't apply
here then. In principle, we can have different SPI devices, where each
is served by a different driver. Together, they create a long register
through which data is shifted. IMHO, the fact that the given device is
on the daisy chain with other devices should be transparent for its SPI
device driver and handled at the level of the SPI subsystem: at SPI
write/read stage, the SPI subsystem would combine message of a addressed
device with the specific no-operation pattern of all other devices being
present on the chain (each device may have its specific no-operation
pattern).

At the moment, I wouldn't try to address a few SPI devices in a single
access.

As you can see, it's no really a mux access like in case of e.g. I2C:
there, we have a physical device performing multiplexing. Once the I2C
bus is switched, no extra effort is required in communication between
the I2C master and I2C device (slave).

>>> compatible = "daisy_chain" or compatible ="simple-bus" would be better?
>> Not "simple-bus", as this is not a simple memory-mapped bus.
>> I'd use something like "spi,daisy-chain", to be validated by RobH.
> Okay, but what makes it generic?
>
> What happens with power control of the child devices? While I'd think
> the only sane design is shared supplies, resets, clocks, etc. (and
> daisy-chained-devices certainly assumes that), it's certainly possible
> some cross device control is needed.
>
>>> Both could be caught by of_register_spi_devices to populate the daisy
>>> chain. Do you agree that at that level the chip select could be defined?
>> Or by a separate SPI device driver that matches against "spi,daisy-chain",
>> and parse the subnodes.

I have been thinking about it and came to a similar idea. However, I
didn't find a straightforward way to associate the SPI devices with the
abstract daisy-chain device which would then eventually be linked with
the SPI controller. It would a simple and clean implementation then: SPI
devices at write/read would call daisy-chain device which would create a
full message and pass it to SPI controller. The issue is that the SPI
device requires for all calls SPI controller. It implies that SPI
daisy-chain would need to be SPI controller as well. However, I think
currently the SPI concept in the kernel is fixed: SPI controller <-> SPI
device and there is no place to implement SPI controller <-> SPI
controller (daisy-chain device) <-> SPI device. Any suggestions on how
to address it?

>>> The reg properties from the sub-nodes (defining actual spi devices)
>>> would be ignored, thus not even needed to be defined.
>> They are needed to determine the order in the chain.
> Agreed, you need something to address a device. A better address might
> be the bit position of the device in the chain. Then this could work
> for any mixture of devices that support chaining (though you'd need to
> know what's a nop for each device).

Well, the idea was that the daisy chain will be populated basing on all
sub-nodes and their order in the device tree. If we go for a reg
numbering the device in the chain, I think we need then
'#daisy-chained-devices' as well, such kernel knows a priori the length
of the chain and can issue an error in case some reg values are missing
on a given chain.

Regards,

Adrian