Hi All, I have implemented the SPI daisy chain support for kernel 4.19. I am enclosing the patch. The implementation is transparent for the SPI devices and doesn't require their modifications. It is based on a virtual SPI device (spi-daisy_chain) and defines two required device tree properties ('spi-daisy-chain-len' and 'spi-daisy-chain-noop') and one optional ('spi-daisy-chain-bits_per_word'). It has been tested on hardware with a chain of three ltc2694 devices. I am able to successfully control them. Unfortunately, I can't confirm that readout works properly If you agree with this daisy-chain concept, in order to upstream this feature, I am willing to merge it with the master (could you navigate me to branch, repository against which I should do it?). Regards, Adrian On 17.06.2020 14:33, Adrian Fiergolski wrote: > Hi All, > > A quick update: I have started implementation taking into account most > of the points from our discussion (a separate spi,daisy-chain spi > device, single device addressed at once, etc.). I have realized that the > daisy-chain device doesn't need to be a SPI controller and information > about the chain can be stored in the spi_device struct and then handled > by the SPI subsystem. Once I have a working implementation, I will share > it with you for a review. > > Regards, > > Adrian > > On 16.06.2020 12:25, Adrian Fiergolski wrote: >> 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 wrote: >>>> Hi Adrian, >>>> >>>> On Mon, Jun 15, 2020 at 6:05 PM Adrian Fiergolski >>>> wrote: >>>>> On 15.06.2020 16:40, Geert Uytterhoeven wrote: >>>>>> On Mon, Jun 15, 2020 at 4:06 PM Adrian Fiergolski >>>>>> 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 >>>>>>>>> wrote: >>>>>>>>>> On 13.06.2020 09:33, Geert Uytterhoeven wrote: >>>>>>>>>>> On Fri, Jun 12, 2020 at 6:26 PM Adrian Fiergolski >>>>>>>>>>> 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 >>