Multiple SPI MCP23S17s sharing a CS line

Multiple SPI MCP23S17s sharing a CS line

[Phil Elwell]

Hi,

We have a user asking for help to construct a Device Tree overlay to use
the GPIOs exported by multiple MCP23S17s sharing an SPI Chip Select 
line. This is a special feature of the MCP devices whereby the SPI bus
is treated rather like an I2C bus with each device assigned a unique
address. His problem is in constructing gpiospecs to refer to all of the
GPIOs.

The mcp23s08 driver claims to support this feature, and there is a DT
property ("microchip,spi-present-mask") to declare which addresses are
used buy the devices. I've spent an hour or so looking at the driver 
code and crawling through the kernel GPIO infrastructure, and I don't 
think it's possible. Here's my logic:

1. Although all devices that are found are presented as a single SPI 
device, they are each registered as independent gpio_chips.

2. A gpio_chip has (amongst other things) a pointer to is Device Tree 
node, a number of GPIOs, and a function that translates from the 
"gpiospec" descriptors found in the device tree to an index into the 
GPIOs exposed by the device. Note that it doesn't include the base of a 
range - all gpio_chips have GPIOs indexed from 0.

3. The mcp23s08 doesn't specify a translation function, so it inherits 
the default of_gpio_simple_xlate that checks that the first parameter is 
less than the number of GPIOs the device supports, and returns it.

4. When another DT node refers to a GPIO, it does so using a gpiospec. 
This is passed to gpiochip_find, which uses 
of_gpiochip_match_node_and_xlate to filter through all the registered 
GPIO controllers to find one which:

i) has a DT node which matches the first cell (e.g. "&gpio"),
ii) has a translation function, and
iii) the translation function succeeds.

5. Continuing point 2, all the MCP devices that share an SPI CS/CE 
inherit the same DT node, which means they will all pass test i). Test 
ii) is trivially passed by all of the devices. Since they all have the 
same translation function (of_gpiochip_match_node_and_xlate), and they 
are all given the same gpiospec to translate, and you aren't meant to 
mix different size variants, the sharing devices will also either all 
pass or all fail the test iii).

Therefore I think it's impossible to construct gpiospecs to refer to all 
of the GPIOs exported by MCP23S17s in this configuration. Please explain
where I've gone wrong.

Thanks,

Phil Elwell

Re: Multiple SPI MCP23S17s sharing a CS line

[Linus Walleij]

Hi Phil,

I think the people I added to the To: line are better suited to answer
this question, they have used the MCP23* chips in practice more
than me and know how they work.

Some minor comments inline.

On Wed, Nov 27, 2019 at 12:20 PM Phil Elwell <phil@raspberrypi.org> wrote:

> We have a user asking for help to construct a Device Tree overlay to use
> the GPIOs exported by multiple MCP23S17s sharing an SPI Chip Select
> line. This is a special feature of the MCP devices whereby the SPI bus
> is treated rather like an I2C bus with each device assigned a unique
> address. His problem is in constructing gpiospecs to refer to all of the
> GPIOs.
>
> The mcp23s08 driver claims to support this feature, and there is a DT
> property ("microchip,spi-present-mask") to declare which addresses are
> used buy the devices.

It's an interesting hack and I kind of see why they are doing it.

>  I've spent an hour or so looking at the driver
> code and crawling through the kernel GPIO infrastructure, and I don't
> think it's possible. Here's my logic:
>
> 1. Although all devices that are found are presented as a single SPI
> device, they are each registered as independent gpio_chips.

So they are presented as a single SPI device, but they are
different physical packages (right?) so it is actually correct to
have several gpio_chips but incorrect that they are all
represented in a single device tree node.

Interestingly there is not a single device tree in the entire
kernel that uses the "*,spi-present-mask" attribute.

Could you provide an example?

I *THINK* the idea behind this attribute is just plain wrong
and cannot be made to work.

Instead the device should be represented as one SPI node
with subnodes for each separate physical device when this
attribute would be used.

mcp {
    compatible = "microchip,mcp23s08";
    microchip,spi-present-mask = <0x03>;
    mcp0: chip0 {
        reg = <0>;
      ....
    }
    mcp1: chip1 {
        reg = <1>;
      ....
    }
};

By introducing such child nodes it gets possible to reference
these chips by phandle <&mcp1 ...>;

Notice use of reg attribute to address subchip.

IIUC this needs to be figured out and both the DT bindings
and the driver need to be fixed to support this peculiar addressing
scheme.

Yours,
Linus Walleij

Re: Multiple SPI MCP23S17s sharing a CS line

[Phil Reid]

On 27/11/2019 20:36, Linus Walleij wrote:
> Hi Phil,
> 
> I think the people I added to the To: line are better suited to answer
> this question, they have used the MCP23* chips in practice more
> than me and know how they work.
> 
> Some minor comments inline.
> 
> On Wed, Nov 27, 2019 at 12:20 PM Phil Elwell <phil@raspberrypi.org> wrote:
> 
>> We have a user asking for help to construct a Device Tree overlay to use
>> the GPIOs exported by multiple MCP23S17s sharing an SPI Chip Select
>> line. This is a special feature of the MCP devices whereby the SPI bus
>> is treated rather like an I2C bus with each device assigned a unique
>> address. His problem is in constructing gpiospecs to refer to all of the
>> GPIOs.
>>
>> The mcp23s08 driver claims to support this feature, and there is a DT
>> property ("microchip,spi-present-mask") to declare which addresses are
>> used buy the devices.
> 
> It's an interesting hack and I kind of see why they are doing it.
> 
>>   I've spent an hour or so looking at the driver
>> code and crawling through the kernel GPIO infrastructure, and I don't
>> think it's possible. Here's my logic:
>>
>> 1. Although all devices that are found are presented as a single SPI
>> device, they are each registered as independent gpio_chips.
> 
> So they are presented as a single SPI device, but they are
> different physical packages (right?) so it is actually correct to
> have several gpio_chips but incorrect that they are all
> represented in a single device tree node.
> 
> Interestingly there is not a single device tree in the entire
> kernel that uses the "*,spi-present-mask" attribute.
> 
> Could you provide an example?
> 
> I *THINK* the idea behind this attribute is just plain wrong
> and cannot be made to work.
> 
> Instead the device should be represented as one SPI node
> with subnodes for each separate physical device when this
> attribute would be used.
> 
> mcp {
>      compatible = "microchip,mcp23s08";
>      microchip,spi-present-mask = <0x03>;
>      mcp0: chip0 {
>          reg = <0>;
>        ....
>      }
>      mcp1: chip1 {
>          reg = <1>;
>        ....
>      }
> };
> 
> By introducing such child nodes it gets possible to reference
> these chips by phandle <&mcp1 ...>;
> 
> Notice use of reg attribute to address subchip.
> 
> IIUC this needs to be figured out and both the DT bindings
> and the driver need to be fixed to support this peculiar addressing
> scheme.
> 

Similar problem has come up before with gpio line props from Jan.
The above was similar to what was discussed previously a couple of times.
eg: https://patchwork.ozlabs.org/patch/1052925/

I'd say it's the way to solve the problem.

I don't have any shared cs spi devices so it hasn't been relevant to my systems.

-- 
Regards
Phil Reid

Re: Multiple SPI MCP23S17s sharing a CS line

[Linus Walleij]

On Thu, Nov 28, 2019 at 5:11 AM Phil Reid <preid@electromag.com.au> wrote:
> On 27/11/2019 20:36, Linus Walleij wrote:

> > IIUC this needs to be figured out and both the DT bindings
> > and the driver need to be fixed to support this peculiar addressing
> > scheme.
> >
>
> Similar problem has come up before with gpio line props from Jan.
> The above was similar to what was discussed previously a couple of times.
> eg: https://patchwork.ozlabs.org/patch/1052925/
>
> I'd say it's the way to solve the problem.

OK I see.

Actually this comment from Jan:

"Linux  does not support multiple child devices "within" one SPI slave.
If there  was proper support for this, this patch would be superfluous."

This is not a real limitation, we can have children inside SPI devices,
no problem at all. The driver can just parse them with something like
for_each_available_child_of_node(parent, child) from
<linux/of.h>.

It is possible for the driver to create proper subdevices for each
node or just create (I think) the different gpio_chip:s with
a reference to the unique DT node for each chip and then the
.xlate function will figure out the GPIO translation.

Yours,
Linus Walleij

Re: Multiple SPI MCP23S17s sharing a CS line

[Phil Elwell]

Hi Linus,

Thank you for such a speedy reply.

On 27/11/2019 12:36, Linus Walleij wrote:
> Hi Phil,
> 
> I think the people I added to the To: line are better suited to answer
> this question, they have used the MCP23* chips in practice more
> than me and know how they work.
> 
> Some minor comments inline.
> 
> On Wed, Nov 27, 2019 at 12:20 PM Phil Elwell <phil@raspberrypi.org> wrote:
> 
>> We have a user asking for help to construct a Device Tree overlay to use
>> the GPIOs exported by multiple MCP23S17s sharing an SPI Chip Select
>> line. This is a special feature of the MCP devices whereby the SPI bus
>> is treated rather like an I2C bus with each device assigned a unique
>> address. His problem is in constructing gpiospecs to refer to all of the
>> GPIOs.
>>
>> The mcp23s08 driver claims to support this feature, and there is a DT
>> property ("microchip,spi-present-mask") to declare which addresses are
>> used buy the devices.
> 
> It's an interesting hack and I kind of see why they are doing it.
> 
>>   I've spent an hour or so looking at the driver
>> code and crawling through the kernel GPIO infrastructure, and I don't
>> think it's possible. Here's my logic:
>>
>> 1. Although all devices that are found are presented as a single SPI
>> device, they are each registered as independent gpio_chips.
> 
> So they are presented as a single SPI device, but they are
> different physical packages (right?) so it is actually correct to
> have several gpio_chips but incorrect that they are all
> represented in a single device tree node.
> 
> Interestingly there is not a single device tree in the entire
> kernel that uses the "*,spi-present-mask" attribute.
> 
> Could you provide an example?

I've asked the user if I may see his overlay, but they've not responded 
yet and I don't think you'll learn very much from it.

> I *THINK* the idea behind this attribute is just plain wrong
> and cannot be made to work.
> 
> Instead the device should be represented as one SPI node
> with subnodes for each separate physical device when this
> attribute would be used.
> 
> mcp {
>      compatible = "microchip,mcp23s08";
>      microchip,spi-present-mask = <0x03>;
>      mcp0: chip0 {
>          reg = <0>;
>        ....
>      }
>      mcp1: chip1 {
>          reg = <1>;
>        ....
>      }
> };
> 
> By introducing such child nodes it gets possible to reference
> these chips by phandle <&mcp1 ...>;
> 
> Notice use of reg attribute to address subchip.

I've not spent enough time in this area to know all of the rules and 
conventions, but it strikes me that if you treat the parent as a kind of 
bus and the "reg" properties as being the addresses (rather than 
indexes) then you no longer need the present mask.

Phil

> IIUC this needs to be figured out and both the DT bindings
> and the driver need to be fixed to support this peculiar addressing
> scheme.
> 
> Yours,
> Linus Walleij
> 

Re: Multiple SPI MCP23S17s sharing a CS line

[Linus Walleij]

On Thu, Nov 28, 2019 at 10:25 AM Phil Elwell <phil@raspberrypi.org> wrote:
> [Me]
> > By introducing such child nodes it gets possible to reference
> > these chips by phandle <&mcp1 ...>;
> >
> > Notice use of reg attribute to address subchip.
>
> I've not spent enough time in this area to know all of the rules and
> conventions, but it strikes me that if you treat the parent as a kind of
> bus and the "reg" properties as being the addresses (rather than
> indexes) then you no longer need the present mask.

Indeed, that would involve deprecating the present mask and
use the reg =<> property to figure out which chips are
connected instead.

Yours,
Linus Walleij

Re: Multiple SPI MCP23S17s sharing a CS line

[Jan Kundrát]

> "Linux  does not support multiple child devices "within" one SPI slave.
> If there  was proper support for this, this patch would be superfluous."
>
> This is not a real limitation, we can have children inside SPI devices,
> no problem at all. The driver can just parse them with something like
> for_each_available_child_of_node(parent, child) from
> <linux/of.h>.
>
> It is possible for the driver to create proper subdevices for each
> node or just create (I think) the different gpio_chip:s with
> a reference to the unique DT node for each chip and then the
> .xlate function will figure out the GPIO translation.

Hi,
I agree that an approach with "subnodes" is probably the best solution; I 
like that. 

When you have a patch, I'll be happy to test it :). I had no idea that you 
can put multiple "client" devices such as gpiochips behind a single parent 
device (itself a SPI client). I suspect that various fixes for making sure 
that no duplicate data are present in, say, debugfs for pintrl and regmap 
and gpio /sys/kernel/debug subtrees, will still be needed, but I think that 
most of them are in place already.

For reference, this is how I've been using these chips:

        gpio_spi_chips: gpio@1 {
                compatible = "microchip,mcp23s17";
                reg = <1>;
                interrupt-parent = <&gpio1>;
                interrupts = <22 IRQ_TYPE_LEVEL_LOW>;
                interrupt-controller;
                #interrupt-cells = <2>;
                gpio-controller;
                #gpio-cells = <2>;
                microchip,spi-present-mask = <0x06>; /* extra addresses 1 
and 2 */
                microchip,irq-mirror;
                drive-open-drain;
                spi-max-frequency = <10000000>;

                gpio-bank@1 {
                        address = <1>;
                        gpio-line-names =
                                "EDFA1_RESET",
                                "EDFA2_RESET",
                                "PMB_ALERT",
                                "EXP_GPIO2",
                                "WSS_SC",
                                "WSS_RST",
                                "I2C_XOR_RDY",
                                "OCM_HS_OUT",

                                "EDFA1_OUT_REFLECT_A",
                                "EDFA1_PUMP_CURRENT_A",
                                "EDFA1_ST1_IN_LOS_A",
                                "EDFA1_ST2_IN_LOS_A",
                                "EDFA1_ST2_OUT_GAIN_A",
                                "EDFA1_CASE_TEMP_A",
                                "EDFA1_ST1_OUT_GAIN_A",
                                "EDFA1_PUMP_TEMP_A";
                };

                gpio-bank@2 {
                        address = <2>;
                        gpio-line-names =
                                /* these are all grounded */
                                "GND",
                                "GND",
                                "GND",
                                "GND",
                                "GND",
                                "GND",
                                "GND",
                                "GND",

                                "EDFA2_OUT_REFLECT_A",
                                "EDFA2_PUMP_CURRENT_A",
                                "EDFA2_ST1_IN_LOS_A",
                                "EDFA2_ST2_IN_LOS_A",
                                "EDFA2_ST2_OUT_GAIN_A",
                                "EDFA2_CASE_TEMP_A",
                                "EDFA2_ST1_OUT_GAIN_A",
                                "EDFA2_PUMP_TEMP_A";
                };

                // FIXME: this hogs both .1 and .2 chips' #6 pin...
                i2c_xor_ready {
                        gpio-hog;
                        gpios = <6 GPIO_ACTIVE_HIGH>;
                        input;
                        line-name = "I2C XOR ready";
                };
        };

Note that FIXME, though. I think that requesting GPIOs by name from 
userspace works correctly (or perhaps only by chance? Cannot check atm, I 
don't have the board + scope on this desk), but a gpio-hog from kernel 
(which uses a GPIO offset within a chip as far as I can tell) appears to be 
slightly wonky as wittnessed by /sys/kernel/debug/gpio on this system:

gpiochip4: GPIOs 464-479, parent: spi/spi1.1, mcp23s17.2, can sleep:
 gpio-464 (GND                 )
 gpio-465 (GND                 )
 gpio-466 (GND                 )
 gpio-467 (GND                 )
 gpio-468 (GND                 )
 gpio-469 (GND                 )
 gpio-470 (GND                 |I2C XOR ready       ) in  lo 
 gpio-471 (GND                 )
 gpio-472 (EDFA2_OUT_REFLECT_A )
 gpio-473 (EDFA2_PUMP_CURRENT_A)
 gpio-474 (EDFA2_ST1_IN_LOS_A  )
 gpio-475 (EDFA2_ST2_IN_LOS_A  )
 gpio-476 (EDFA2_ST2_OUT_GAIN_A)
 gpio-477 (EDFA2_CASE_TEMP_A   )
 gpio-478 (EDFA2_ST1_OUT_GAIN_A)
 gpio-479 (EDFA2_PUMP_TEMP_A   )

gpiochip3: GPIOs 480-495, parent: spi/spi1.1, mcp23s17.1, can sleep:
 gpio-480 (EDFA1_RESET         |EDFA reset          ) out hi 
 gpio-481 (EDFA2_RESET         )
 gpio-482 (PMB_ALERT           )
 gpio-483 (EXP_GPIO2           )
 gpio-484 (WSS_SC              )
 gpio-485 (WSS_RST             )
 gpio-486 (I2C_XOR_RDY         |I2C XOR ready       ) in  lo 
 gpio-487 (OCM_HS_OUT          |OCM HS_OUT          ) in  hi IRQ 
 gpio-488 (EDFA1_OUT_REFLECT_A )
 gpio-489 (EDFA1_PUMP_CURRENT_A)
 gpio-490 (EDFA1_ST1_IN_LOS_A  )
 gpio-491 (EDFA1_ST2_IN_LOS_A  )
 gpio-492 (EDFA1_ST2_OUT_GAIN_A)
 gpio-493 (EDFA1_CASE_TEMP_A   )
 gpio-494 (EDFA1_ST1_OUT_GAIN_A)
 gpio-495 (EDFA1_PUMP_TEMP_A   )

I think this is more or less the same thing as Phil Elwell wrote in his 
initial message. But I think that 
https://patchwork.ozlabs.org/patch/1052925/ at least makes it possible to 
request the GPIOs from userspace.

Hope this helps,
Jan