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.
> >
> 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.
> 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.
> 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.
> >
> >> 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.
>
> So, in case of incompatible mode, let's throw an error and remove
> devices of the whole daisy chain, maybe?
Yep.
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
