bcm2835_spi.c mod to support mode and clock in ivars from spibus

[bobf_mrp3.com]

Summary

spibus now correctly handles mode and clock ivar settings via ioctl, in addition to the CS polarity bit SPIBUS_CS_HIGH.

The current implementation of bcm2835_spi.c masks the SPIBUS_CS_HIGH bit and does not use it. The code modifications below make use of it by setting the SPI_CS_CSPOL bit in the device's CS control register.

Additionally, the mode (SPIBUS_IVAR_MODE) and clock (SPIBUS_IVAR_CLOCK) values are correctly set for the driver on a per-transaction basis.

Since the definition of the 'clock' value is the MAXIMUM clock speed, the driver modifications will lower the SPI device's clock speed to match the setting of 'clock' in the ivars, if the currently assigned bus speed is higher than that for 'clock' in the ivars. it also respects a 'clock' value of '0' (i.e. 'use default').

This may be the only driver making use of 'clock' and so it's worth reviewing how it's implemented here, to make sure it's "the right way".

Test Plan

use new 'spi' utility and various hardware, with a Raspberry Pi, to verify mode/speed changes.

results:
with test device, speeds > 100khz weren't working properly (due to the device). slower speeds ok

modes 0, 1, and 3 on the test device only worked with modes 0, 1, and 3 from the bcm2835 driver
mode 2 on the device worked with both mode 2 and mode 3 (for whatever reason). It's worth noting since it may say something about my test device.

Conclusion: mode settings are working as expected with a 'cursory' test. no data loss nor bit shifting observed.

NOTE: tested by sending a non-zero command byte followed by 11 bytes of data read from the test device, using the mode programmed into the test device and every other mode to verify

[bobf_mrp3.com]

verified mode and speed appear to be working (as assigned via spigen) although some weirdness in mode 3 causes the first received bit to be dropped. This could be my hardware doing it, and not the broadcom. but it's worth noting. it might be clock glitches or timing of the first clock pulse following cs, which can't be delayed with the existing spibus and driver. Applying a delay might help this, or not. Otherwise, it appears to be working properly unless someone can demonstrate a problem that i wasn't able to detect. Worst case I could set up a 2-channel o-scope to trigger off of the first clock pulse and display the bits and their timing, if any of this becomes suspect.

[bobf_mrp3.com]

sys/arm/broadcom/bcm2835/bcm2835_spi.c

479 ↗

(On Diff #41339)

this call needs to happen after modifying the clock speed, phase, and polarity. Apparently the interrupt bits are what start the transaction, and if the clock phase and polarity and speed are modified AFTER that, it might be causing a glitch that I observed when doing a mode 3 transfer.

519 ↗

(On Diff #41339)

I have to wonder whether it would be better to leave the clock phase, polarity, and speed the way it is, rather than restoring it.

[bobf_mrp3.com]

sys/arm/broadcom/bcm2835/bcm2835_spi.c

483 ↗

(On Diff #41339)

if all 3 CS pins have the same polarity this will work. however, there's only a single polarity bit for all 3 CS lines. Given that fact, it may be better to NOT set the SPI_CS_CSPOL bit, but have a sysctl var for it (or device parameter) instead? For now it may simply need to be documented that having different polarities for the 3 CS pins is going to cause problems if you're using multiple SPI devices on the same bus.

once the SPI_CS_TA bit is set, the CS lines flip to whatever state is described. This also suggests that the SPI_CS_CSPOL bit should be 'sticky' across transactions, or you could have a real _mess_, depending on the state of the clock line following the transaction. It should also be verified as being properly set when the 'spibus' devices are first created from FDT info, though setting it correctly on the very first transaction will probably work in most cases.

This is all because clock polarities, depending on the mode, may attempt to shift in/out a bit if the CS line has the wrong polarity while the clock mode is being changed. Further, if the default clock polarity is opposite, it's possible that the mode switch itself might trigger something if CS polarity is wrong at the moment this all happens. It really complicates things and suggests that CS polarity should probably happen first, then the mode and clock speed, and finally the SPI_CS_TA bit [with int flags on as well, like before].

If all of this can't be accomplished 'safely', the SPI_CS_CSPOL bit should probably be left as 0, with 'TODO' comments in the code to implement this when it can be done.

519 ↗

(On Diff #41339)

after working on this a bit, it seems that the clock speed should be restored, and the mode CAN be restored (but does not necessarily have to be restored).

what is accidentally correct here is that the 'SPI_CS_CSPOL' is _not_ being restored, which is a good thing. Restoring the CS polarity, i.e. 'flipping' it, would be nothing less than a major problem on the SPI bus. So it has to remain 'sticky' with the previous transaction, and be (hopefully) restricted to 'same polarity' for all of the cs definitions in the FDT info.

[bobf_mrp3.com]

sys/arm/broadcom/bcm2835/bcm2835_spi.c

483 ↗

(On Diff #41339)

there may be a better way: assign the 3 polarity bits SPI_CS_CSPOL0, through SPI_CS_CSPOL2 and leave SPI_CS_CSPOL at zero. Unfortunately this would have to be done during device initialization to prevent any problems.

however, there's an inherent problem: _some_how, the clock polarity _must_ be assigned correctly before _any_ device on the SPI bus sees the clock wiggling. Because, clock pulses with an 'active high' device are going to cause that device to start accepting input and sending output, along with the device you intend to communicate with, which of course is _bad_.

On the other hand, there does not seem to be a mechanism for this, although I'll be looking to see what happens during the initialization phase with things like 'ofw_bus_gen_setup_devinfo()' to see if that helps in any way. A short reference to ofw_spibus_attach near line 120 in ofw_spibus.c where the child devices are being set up from FDT info.

[bobf_mrp3.com]

re-arranged the order in which the mode and clock speed are set up. changed the method of setting clock polarity to use the individual 'CSPOL' bits instead of the 'global' one that applies to all CS lines (that would only have worked if all 3 lines have the same polarity). This should correct for the mode 3 glitch (still need to test it). however, it does not fully handle the potential problems with CS polarity; in short, unless you set up all of the CS polarity bits before any transaction occurs, any 'active high' CS device could act as if it were selected while the CS is in its default 'high' state, likely causing contention between slave devices. For now, the actual code is in the proposed patch as-is, and I'll research this further.

[bobf_mrp3.com]

sys/arm/broadcom/bcm2835/bcm2835_spi.c

491 ↗

(On Diff #41388)

I'm inclined to just comment out this section, and not support the SPIBUS_CS_HIGH bit, mostly because it's rarely used, and because its implementation may require additional support from spibus that affects every single SPI driver.

[• ian]

sys/arm/broadcom/bcm2835/bcm2835_spi.c

466 ↗

(On Diff #41388)

Don't do these reads until after waiting for BCM_SPI_BUSY below.

526 ↗

(On Diff #41388)

The interpretation of clock is that the device wants the bus to go as fast as possible, but no faster than clock. The clock should be reset for each transfer using that logic. That is, if clock is 1mhz for cs 0 and 50mhz for cs 1, it's not okay to do 1mhz transfers on cs 1 just because that's the value already in the register and it's lower than the max.

This concept of "0 means default" is insane. What does that even mean? I can't remember a single spi controller I've ever seen that has the idea of a default speed. About the closest we might come to that is to capture whatever is in the clock divider register at attach() time and call that the default (i.e., default means "whatever uboot set up"). But if that value is zero or an uninitialized invalid value, who knows what happens.

573 ↗

(On Diff #41388)

I don't think there's any need to restore anything. Each new transfer should set up mode and clock before starting the transfer. If we're going to have the idea of a default clock, we should capture its value once at attach time.

[• ian]

sys/arm/broadcom/bcm2835/bcm2835_spi.c

241 ↗

(On Diff #41388)

I think all these sysctls should be removed. CS polarity is an attribute of the slave device, not the controller.

[bobf_mrp3.com]

sys/arm/broadcom/bcm2835/bcm2835_spi.c

241 ↗

(On Diff #41388)

I am not opposed to making that change. If there's a consensus, I'll do it. I only added 'cspol2' because the cs==2 case wasn't being handled the same way.

466 ↗

(On Diff #41388)

I also moved the 2 calls that get the transaction's mode and clock to be after doing BCM_SPI_LOCK [same kind of thinking]. Good point. Although they probably will not change, in theory they _might_ change, and it's a simple thing to place the calls to be _after_ locking the bus.

526 ↗

(On Diff #41388)

I'm good with what you say. Right now the ivars definition (used to?) includes 'clock == 0' as "default" . I checked and it looks like your patch adds an additional comment about this in ofw_spibus.c (which wasn't there before).

So, do you have a suggestion for how the logic _should_ work? Should I simply check for 'maximum of' but NOT handle a value of zero as 'use default' ? Or, just assign it 'as-is' every time? [I actually like this better]

This logic that assumes that the overlay correctly assigns the maximum frequency would require changing the overlay appropriately.

The driver default is currently 500khz [which becomes the maximum]. It can be changed by tweeking the sysctl var for it. The current behavior (as posted here) would use the 500khz value if the clock were assigned to '0' in spibus (assuming it hadn't been changed by tweeking the sysctl var for it).

The data sheet says that the SPI speed can be up to 1/2 of the APB clock, which is defined as SPI_CORE_CLK in the driver code. "bcm2835_spireg.h" defines this value as 250Mhz (although in theory it should be read from fdt data) so, if I interpret this correctly, the max clock is 125Mhz. This also means that the minimum SPI speed for this device is around 3.8khz.

573 ↗

(On Diff #41388)

I'll leave this as-is for now, until I get the necessary clarity on how to handle clock speed assignment.

[bobf_mrp3.com]

making some of the requested changes, waiting on others.

[bobf_mrp3.com]

1. making the sysctl variables all read-only so you can still view them. changing the values happens with each transaction via spibus ivars. This could be made into a 'debug build only' feature.
2. placed '#ifdef' block around assignment of CS_CSPOLn bits. This feature is effectively broken since there's no apparent method to set them all before any transaction happens. Once there is such a method, they should probably be set just one time and not per transaction. I will investigate doing this on load, and see if it is possible that the cs information is made available while creating the spigen devices.
3. removed save/restore of clock freq and mode (it's not necessary)
4. check for clock == 0 and fail with EINVAL

[bobf_mrp3.com]

sys/arm/broadcom/bcm2835/bcm2835_spi.c

60 ↗

(On Diff #41875)

'magic number' within the code now a macro value

149 ↗

(On Diff #41875)

this code was only relevant when the clock value was assigned via sysctl. It was commented out for reference since this is the official way to set the clock freq. It can be removed once the 'per transaction' clock assignment is verified correct.

175 ↗

(On Diff #41875)

this code is only called for sysctl vars implemented within this source file. Since they're all read-only now, the write code has been effectively removed. It has been commented out and left for reference.

333 ↗

(On Diff #41875)

I'm not sure how to get the correct maximum frequency from the fdt data. I'll research it.

Additionally, I changed the magic number '500000' into a macro BCM2835_SPI_DEFAULT_CLOCK (defined near the top of the file).

241 ↗

(On Diff #41388)

I have made them all read-only. Let me know if you'd rather have them removed.

[bobf_mrp3.com]

sys/arm/broadcom/bcm2835/bcm2835_spi.c

466 ↗

(On Diff #41875)

new behavior does not allow clock == 0 and assumes that this is the speed you want the bus to run at, with a maximum being the max capabilities of the device. It is, however, a clock speed that's assigned on a per-cs basis in spibus. If this is a problem the overlays could address it, setting the maximum frequency for every device on the spibus to something that doesn't cause problems. But if anything more is required let's put it into spibus...

526 ↗

(On Diff #41388)

0 means default has been removed - see commend near line 465

[bobf_mrp3.com]

updated a few comments. RPi 1,2 testing has been pretty good on my end. still needs RPi3 testing though.

[bobf_mrp3.com]

removed some commented-out code, minor edits

[• ian]

sys/arm/broadcom/bcm2835/bcm2835_spi.c

241 ↗

(On Diff #41388)

If they're only valid during a transfer they don't have any real use at all, we should just remove them.

573 ↗

(On Diff #41388)

In general, we're making this driver work properly with fdt data, and the device bindings require a spi-max-frequency property in each child, so we can require that a speed be present. The bindings say nothing about 0, it just says the value is frequency in Hz. 0Hz is insane, so let's just call that an error (1 would also be insane, but at least the transfer would finish some day).

[bobf_mrp3.com]

sys/arm/broadcom/bcm2835/bcm2835_spi.c

136 ↗

(On Diff #43638)

this next section removed since it's read-only now, and the code below handles writes.

156 ↗

(On Diff #43638)

this next section removed since it's read-only now, and the code below handles writes.