Re: [cadence-spi] daisy chain

[Adrian Fiergolski <adrian.fiergolski@xxxxxxxxxxxxx>]

Sorry for the typo in the example device tree:

On 15.06.2020 15:57, Adrian Fiergolski wrote:
> Hi Geert,
>
> Thank you for the quick reply.
>
> On 15.06.2020 15:07, Geert Uytterhoeven wrote:
>> Hi Adrian,
>>
>> CC devicetree
>>
>> 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>;

               reg = <0>;
               
               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>;
               };
           };
        };

>
> Once a node has daisy-chanied-devices property defined,
> of_register_spi_device (spi.c) will interpret it as a daisy chain. I
> will assume, that for the given chain the lowest frequency of the whole
> chain should be used. When it comes to the mode, as in case of
> incompatibility no much can be done anyway, the mode of the addressed
> spi device will be used.
>
>>> If you agree with the above description, I could try to implement it. Of
>>> course any suggestion are welcome, however, I would like to have a
>>> working solution until end of this week, so I would appreciate an active
>>> feedback. As my SoC works with kernel v4.19, I would implement it for
>>> it, test it, and move it afterwards to the master version (I hope, there
>>> were no big changes in the SPI subsystem, right?).
>> Having something that works by the end of the week sounds doable to.
>> Getting it in shape for upstreaming is a different thing...
> Let's try then. As I wrote earlier, I will try to implement it and test
> it with 4.19. Afterwards, I will share it with you for a general concept
> review. Once no comments, I will try to move it to the master and we can
> start from there the upstreaming process.
>
> Regards,
>
> Adrian
>
>