Add sub address support to SPI driver enabling multiple devices to be be accessed using the minimum of GPIO

[aelmicro]

This is a modification I have used to access multiple SPI devices on the same SPI bus by using the minimum of SPI chip selects.
I have seen requests for this type of access and use this to access my range of addon boards, but every time a new release comes out I have to cut and paste in the code and rebuild the kernel.
It would be nice if the was included in the master source code and I'm sure others would benifit from this change.

An example is to use one chip select as default GPIO 8 and then add additional sub address lines using other nominated GPIO's.
For example adding 2 additional GPIO's as spi addresses this give the possible selection of 4 SPI devices using just 3 GPIO lines.
The target additional hardware would decode the 2 address lines along with the chip select to qualify device access, an example is a TTL 2 to 4 decoder.
Currently it is possible to create up to 32 devices, this is set by the N_SPI_MINORS value in spidev.c, this can be increased if required.
With the example above this will create spidev0.0 to spidev0.3.
If 4 address lines and 1 chip select is used this changes to spidev0.0 to spidev0.15, uses only 5 GPIO's
If 4 address lines and 2 chip selects are used this changes to spidev0.0 to spidev0.31 uses only 6 GPIO's

I have added an example DTS file showing the configuration using 2 chip selects and 4 address lines.
Similar to the use of multiple chip selects, there is no limit to the number of address lines used.

spi0-hw-sa32-overlay.zip

Thanks Mark

[pelwell]

I can see what you've done here and why, but you have a long way to go before it would be accepted into our kernel. Putting aside all of the kernel coding guidelines that you haven't followed, the main hurdle is one of support. Our kernels have to be supported, and for any patch that isn't part of a standard upstream kernel the burden of that support falls on us. There are some cases where our downstream usage goes against the philosophy of the upstream devs - declaring spidev nodes in Device Tree is an obvious example - where the utility outweighs the cost, but for something like this that touches a number of files in fairly fundamental ways we'd prefer the patches are sent upstream.

The guidelines for submitting patches into the kernel are here: https://www.kernel.org/doc/html/v4.17/process/submitting-patches.html
Note in particular the requirement that your patches pass the checkpatch testing. To show you how much work is involved I've enabled the build check tests on this PR. By the time I've finished typing this the "Advisory checkpatch review" will have flagged up (I'm guessing) tens of issues.

Edit: checkpatch has found 45 errors and many things to warn about.

[pelwell]

Having said that, there is a way to add this functionality without building your own kernel, and that's as a Device Tree overlay. The gpio-fsm driver manipulates GPIOs under control of a finite state machine (FSM). It can monitor a number of physical input GPIOs, drive other physical output GPIOs, and also create virtual (soft) GPIOs to act as inputs to and outputs from the FSM.

Here's an FSM that uses 1 CE line and 3 SA lines to drive 8 SPI devices (with appropriate external address decoding). I understand that this could also have been achieved using 2 CE lines and 2 SA lines, the difference being just how much external decoding logic is needed:

/dts-v1/;
/plugin/;

#include <dt-bindings/gpio/gpio-fsm.h>

#define VCE0  GF_SW(0)
#define VCE1  GF_SW(1)
#define VCE2  GF_SW(2)
#define VCE3  GF_SW(3)
#define VCE4  GF_SW(4)
#define VCE5  GF_SW(5)
#define VCE6  GF_SW(6)
#define VCE7  GF_SW(7)
#define CE0   GF_OP(0)
#define SA0   GF_OP(1)
#define SA1   GF_OP(2)
#define SA2   GF_OP(3)

/{
	compatible = "brcm,bcm2835";

	fragment@0 {
		target = <&spi0_cs_pins>;
		__overlay__ {
			brcm,pins = <7 8 23 24>;
			brcm,function = <1>; /* output */
		};
	};

	fragment@1 {
		target-path = "/";
		__overlay__ {
			fsm: spi-sa-2-8 {
				compatible = "rpi,gpio-fsm";

				debug = <0>;
				gpio-controller;
				#gpio-cells = <2>;
				num-swgpios = <8>;
				gpio-line-names = "vce0", "vce1", "vce2", "vce3", "vce4", "vce5", "vce6", "vce7";
				output-gpios = <&gpio 8 1>,  // CE0
					       <&gpio 7 0>,  // SA0
					       <&gpio 23 0>, // SA1
					       <&gpio 24 0>; // SA2
				shutdown-timeout-ms = <0>;

				start {
					start_state;
					set = <CE0 0>, <VCE0 1>, <VCE1 1>, <VCE2 1>, <VCE3 1>, <VCE4 1>, <VCE5 1>, <VCE6 1>, <VCE7 1>;
					idle = <GF_DELAY 0>;
				};

				idle {
					shutdown_state;
					set = <CE0 0>;
					vce0 = <VCE0 0>;
					vce1 = <VCE1 0>;
					vce2 = <VCE2 0>;
					vce3 = <VCE3 0>;
					vce4 = <VCE4 0>;
					vce5 = <VCE5 0>;
					vce6 = <VCE6 0>;
					vce7 = <VCE7 0>;
				};

				vce0 {
					set = <SA0 0>, <SA1 0>, <SA2 0>, <CE0 1>;
					idle = <VCE0 1>;
				};

				vce1 {
					set = <SA0 1>, <SA1 0>, <SA2 0>, <CE0 1>;
					idle = <VCE1 1>;
				};

				vce2 {
					set = <SA0 0>, <SA1 1>, <SA2 0>, <CE0 1>;
					idle = <VCE2 1>;
				};

				vce3 {
					set = <SA0 1>, <SA1 1>, <SA2 0>, <CE0 1>;
					idle = <VCE3 1>;
				};

				vce4 {
					set = <SA0 0>, <SA1 0>, <SA2 1>, <CE0 1>;
					idle = <VCE4 1>;
				};

				vce5 {
					set = <SA0 1>, <SA1 0>, <SA2 1>, <CE0 1>;
					idle = <VCE5 1>;
				};

				vce6 {
					set = <SA0 0>, <SA1 1>, <SA2 1>, <CE0 1>;
					idle = <VCE6 1>;
				};

				vce7 {
					set = <SA0 1>, <SA1 1>, <SA2 1>, <CE0 1>;
					idle = <VCE7 1>;
				};
			};
	       };
        };

	fragment@2 {
		target = <&spi0>;
		__overlay__ {
			/* needed to avoid dtc warning */
			#address-cells = <1>;
			#size-cells = <0>;
			pinctrl-names = "default";
			pinctrl-0 = <&spi0_pins &spi0_cs_pins>;
			cs-gpios = <&fsm 0 1>, <&fsm 1 1>, <&fsm 2 1>, <&fsm 3 1>, <&fsm 4 1>, <&fsm 5 1>, <&fsm 6 1>, <&fsm 7 1>;
			status = "okay";

			spidev0_0: spidev@0 {
				compatible = "spidev";
				reg = <0>;	/* CE0, SA=0 */
				spi-max-frequency = <125000000>;
			};

			spidev0_1: spidev@1 {
				compatible = "spidev";
				reg = <1>;	/* CE0, SA=1 */
				spi-max-frequency = <125000000>;
			};

			spidev0_2: spidev@2 {
				compatible = "spidev";
				reg = <2>;	/* CE0, SA=2 */
				spi-max-frequency = <125000000>;
			};

			spidev0_3: spidev@3 {
				compatible = "spidev";
				reg = <3>;	/* CE0, SA=3 */
				spi-max-frequency = <125000000>;
			};

			spidev0_4: spidev@4 {
				compatible = "spidev";
				reg = <4>;	/* CE0, SA=4 */
				spi-max-frequency = <125000000>;
			};

			spidev0_5: spidev@5 {
				compatible = "spidev";
				reg = <5>;	/* CE0, SA=5 */
				spi-max-frequency = <125000000>;
			};

			spidev0_6: spidev@6 {
				compatible = "spidev";
				reg = <6>;	/* CE0, SA=6 */
				spi-max-frequency = <125000000>;
			};

			spidev0_7: spidev@7 {
				compatible = "spidev";
				reg = <7>;	/* CE0, SA=7 */
				spi-max-frequency = <125000000>;
			};
		};

	};

	__overrides__ {
		fsm_debug = <&fsm>,"debug:0";
	};
};

When this is compiled and loaded it claims GPIOs 8, 7, 23 and 24 together with the usual SPI signals. It declares 8 soft GPIOs to use as virtual CE lines, and creates /dev/spidev0.0 - /dev/spidev0.7. On startup the FSM sets the CE lines (real and virtual) high, then goes to the idle state. When one of the virtual CE lines is pulled low it transitions to a state that sets the SA lines appropriately before pulling the physical CE line low, then remains in this state until the soft CE line goes high again, at which point it returns to the idle state, deasserting the physical CE line.

I documented this gpio-fsm driver and its configuration in this Forum post: https://forums.raspberrypi.com/viewtopic.php?t=304278
Feel free to ask questions - if you request a particular combination of CE lines and SA lines I can generate you a pre-built overlay to try.

[aelmicro]

Hi Phil, Thanks for you comments and the solution shown below. I have tested it and it manipulates the CE and address pins as required but for some reason the SPI clock has disappeared and I assume the MOSI signal as well. Is there something else that need to be added to this solution? Thanks Mark

[pelwell]

Is there something else that need to be added to this solution?

There shouldn't be - that works as-is for me, with just dtoverlay-spi-sa-1-8,fsm_debug in config.txt (spi-sa-1-8 is the name I chose, and fsm_debug enables diagnostics to the kernel log.

1. What's non-standard in your config.txt?
2. What does raspi-gpio get 7-11 report? I get:

   $ raspi-gpio get 7-11
   GPIO 7: level=0 fsel=1 func=OUTPUT pull=UP
   GPIO 8: level=1 fsel=1 func=OUTPUT pull=UP
   GPIO 9: level=0 fsel=4 alt=0 func=SPI0_MISO pull=DOWN
   GPIO 10: level=0 fsel=4 alt=0 func=SPI0_MOSI pull=DOWN
   GPIO 11: level=0 fsel=4 alt=0 func=SPI0_SCLK pull=DOWN
   

[aelmicro]

Hi Phil, I have attached a couple of pdf’s containing oscilloscope traces, “good Access.pdf” uses the standard overlay and GPIO 8 as the chip enable, and “bad access.pdf” using the overlay you produced. The top trace is the spi clock, the bottom the chip select. The chip enable is not in sync with the spi clock as it should and there appears to be only one chip select as opposed to two. I guess the FSM is not acting in sync with the spi driver? Thanks Mark

[pelwell]

Attachments in replies to GitHub are dropped, so there's nothing to look at. The FSM activity should all complete before the SPI driver has finished driving CE low, so it is completely synchronised.

[aelmicro]

Pitty about the attachments. So in the spi.c driver when the call is made to gpiod_set_value_cansleep() along with the descriptor and level to set, its at this point that the fsm does its magic? Thus it detects the state of the virtual pin which is what is being set/reset, sets up the other pins and then returns allowing the spi driver to then output the bytes into the hardware spi controller, for transmission. There is definitely no valid chip select pulse whenever the spi clock is running, it always appears after the clock has run and the data output is output hence it is not accessing the device. Thanks Mark

[pelwell]

Here's a capture of sending a single byte to spidev0.0, having previously written to spidev0.7 (hence all the high->low transitions on the SA signals):

As I mentioned earlier this overlay uses once CE GPIO and three SA GPIOs, but that can easily be changed to 2 CEs and 2 SAs.

[aelmicro]

Hi Phil, I don’t dispute your snapshot that is exactly what I would expect to see, unfortunately I don’t get the same result. So if I change the .dts line “cs_gpiods = <&fsm 0 1>” etc in the devce file you sent me to “cs_gpiods = <&gpio 8 0 1>”, and forget about address decoding, I get a chip select pulse in sync with the clock and data and it all works. Top clock, bottom chip select GPIO 8 ***@***.*** If I use the “cs_gpiods = <&fsm 0 1>” etc. version the chip select is out of sync, however very occasionally it is in sync and the access to the device works. The chip select is not always in the same place and there is only ever one enable not two as shown above. Top clock, bottom chip select GPIO 8 ***@***.*** I have looked into how the fsm is implemented but can’t see why it should fail at this time, I will continued to study how the fsm works. Using kernel version 6.1.21-v8+, also tried the latest 6.1.51-v8+ but no difference. Mystified Mark

[pelwell]

I'm also baffled. In case it has an influence, which hardware are you testing on? And is it with or without your SPI modifications?

[aelmicro]

I did a complete update of the PI, to remove any of my changes. I’m testing it on a pi4b with 4Gb RAM, using the I2C and SPI but never at the same time. I like a challenge so I will endeavour to find out what is going on and report my findings. Mark From: Phil Elwell ***@***.***> Sent: Friday, September 8, 2023 4:50 PM To: raspberrypi/linux ***@***.***> Cc: Mark Humphreys ***@***.***>; Author ***@***.***> Subject: Re: [raspberrypi/linux] Add sub address support to SPI driver enabling multiple devices to be be accessed using the minimum of GPIO (PR #5595) I'm also baffled. In case it has an influence, which hardware are you testing on? And is it with or without your SPI modifications? — Reply to this email directly, view it on GitHub<#5595 (comment)>, or unsubscribe<https://github.com/notifications/unsubscribe-auth/AA6QI2GRQ3QICRGNNW7XTK3XZM5CVANCNFSM6AAAAAA4PFNZ3I>. You are receiving this because you authored the thread.Message ID: ***@***.******@***.***>> Disclaimer: This e-mail contains proprietary information which may be legally privileged and is for the intended recipient only. If an addressing or transmission error misdirects this e-mail, please notify us by replying to this e-mail and then delete it. If you are not the intended recipient you cannot use, disclose, distribute, copy, print, or rely on this e-mail. We have taken reasonable precautions to ensure that any attachment to this e-mail does not contain a virus. We cannot, however, accept liability for any damage caused by software viruses. AEL Microsystems Limited Mobile:- +44 7774 773951 12 Pixiefield, Cradley, Malvern WR13 5ND AEL Microsystems Limited is registered in England, Company No. 04639641. Registered Office is at, 12 Pixiefield, Cradley, Malvern, Worcestershire. WR13 5ND VAT Registration No. GB 705 2669 38 http://www.aelmicro.com<http://www.aelmicro.com/>

[aelmicro]

Hi Phil, So I think I have made some progress now I understand the workings of the fsm code and this if I’m correct would explain what I’m seeing. Sometimes if I send just one SPI transfer it works and sometimes not, but I send multiple transfers back to back and that’s where it all falls off a cliff. Hard pin version. Fundamentally when the spi driver calls spi_set_cs() to set the chip select low, this calls gpio_set_value_cansleep() using the hard pin descriptor, which as it’s a real GPIO pin sets/resets the level of the pin as requested before it returns to carry on setting the remainder of the SPI hardware to transfer the data bytes. All great so far, works as expected. Now for the FSM version. SPI driver calls spi_set_cs() then gpio_set_value_cansleep() using the soft pin descriptor, this finds its way to gpio_fsm_set_soft() which spins through the list of soft pin events and looks for a change. If one is found then gpio_fsm_go_to_state() is called otherwise a return to the caller tree is made back to the SPI driver. If gpio_fsm_go_to_state() is called it notes the required state then schedules a worker calling schedule_work() to action the new state then immediately returns to the SPI driver, it does not do anything to the GPIO pins at this time. It is now up to the worker which is a thread on its own, to process the hardware GPIO pins, which may or may not happen immediately which is what I’m seeing, so the whole of the setting of the hardware pins is asynchronous to call to gpio_set_value_cansleep() and that is why the SPI driver merrily carries on in priority over the fsm worker thread and starts sending data before the hardware is ready. For the whole thing to be truly synchronous the setting of the hardware needs to be done in place of the scheduling of the worker so that all actions have been satisfied before returning up the caller tree back to the caller of gpio_set_value_cansleep(). The other problem I also noticed is that I issue in one instance 12 calls to transfer values to the SPI port as separate transactions back to back, in this instance some of the actions would be completely lost as there is no sort of stage transaction queue associated between the gpio_fsm_go_to_state() function and the worker thread. So to summarise I can see the fsm useful as it has been designed providing the caller changing a soft pin level does not need to be fully synchronized with the hardware, SPI transfers do!, also as there is no transaction queue between gpio_fsm_go_to_state() then calling for another state change before the worker has run the current one is going to lose state changes and possibly get confused. You may turn round and say BUT it doesn’t quite work like that, and that’s OK, but as it stands this solution is not going to work. Regards Mark

[pelwell]

I think you haven't been looking at the latest version of the gpo-fsm code, which was updated last week. In that, gpio_fsm_set_soft calls gpio_fsm_go_to_state, which calls gpio_fsm_enter_state, which (when appropriate) calls gpio_set_value_cansleep.

sudo rpi-update will install the latest version (it's not in apt update yet).

[pelwell]

Hold off on that rpi-update - there appears to be some kernel<->module version confusion.

[aelmicro]

Ok, i was looking at 6.1.50 on github last week, may be that's why yours worked and mine didn't, Source code has changed since. Will hold off until you have finished your tweaking Mark Sent from Mark's Jelly Bone

…

________________________________ From: Phil Elwell ***@***.***> Sent: Monday, September 11, 2023 10:23:09 AM To: raspberrypi/linux ***@***.***> Cc: Mark Humphreys ***@***.***>; Author ***@***.***> Subject: Re: [raspberrypi/linux] Add sub address support to SPI driver enabling multiple devices to be be accessed using the minimum of GPIO (PR #5595) Hold off on that rpi-update - there appears to be some kernel<->module version confusion. — Reply to this email directly, view it on GitHub<#5595 (comment)>, or unsubscribe<https://github.com/notifications/unsubscribe-auth/AA6QI2AHF474PSAXC4CFS3DXZ3J73ANCNFSM6AAAAAA4PFNZ3I>. You are receiving this because you authored the thread.Message ID: ***@***.***> Disclaimer: This e-mail contains proprietary information which may be legally privileged and is for the intended recipient only. If an addressing or transmission error misdirects this e-mail, please notify us by replying to this e-mail and then delete it. If you are not the intended recipient you cannot use, disclose, distribute, copy, print, or rely on this e-mail. We have taken reasonable precautions to ensure that any attachment to this e-mail does not contain a virus. We cannot, however, accept liability for any damage caused by software viruses. AEL Microsystems Limited Mobile:- +44 7774 773951 12 Pixiefield, Cradley, Malvern WR13 5ND AEL Microsystems Limited is registered in England, Company No. 04639641. Registered Office is at, 12 Pixiefield, Cradley, Malvern, Worcestershire. WR13 5ND VAT Registration No. GB 705 2669 38 http://www.aelmicro.com<http://www.aelmicro.com/>

[pelwell]

I think sudo rpi-update should be safe - the necessary 6.1.52 modules are present in the rpi-firmware repo, just not in the firmware repo.

[aelmicro]

Hi Phil, All working as I would expect it to. Thanks for implementing these changes and for your support. Regards Mark

[pelwell]

Great. I'm going to close this PR if that's OK with you, and in return my offer of a custom overlay or two still stands (unless you feel confident in doing that yourself).

[aelmicro]

That’s fine by me for closing the PR, I’m ok with building my overlays thanks for your offer. Mark