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