Re: [cadence-spi] daisy chain

[Geert Uytterhoeven <geert@xxxxxxxxxxxxxx>]

Hi Adrian,

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.

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

Don't the modes have to agree, too?
Else the dummy command may be interpreted differently than intended.

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@xxxxxxxxxxxxxx

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds