Skip to content

Conversation

@dhalbert
Copy link
Collaborator

@dhalbert dhalbert commented Jan 30, 2018

Fixes #464. Corrects baud rate computation to match formula. Allow baud register value to be 0, which allows maximum rate. On both M0 and M4, with SPI SERCOM clocked at 48 MHz, I can get 24Mhz SPI clock pulses now, as verified by my Saleae.

Previous error checking used to error out above a maximum SPI frequency. That was not intentional, but was due to checking for the baud register value being 0. That is no longer the case: 0 is a legitimate value. The frequency can be arbitrarily high, and you'll just get the highest available frequency from the clocked SERCOM.

I didn't add anything that returns you the actual frequency, but I could.

@dhalbert dhalbert requested review from deshipu and tannewt January 30, 2018 04:01
deshipu
deshipu previously approved these changes Jan 30, 2018
Copy link

@deshipu deshipu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you very much for this!

@dhalbert
Copy link
Collaborator Author

I thought of something better overnight, which is to accept any frequency and just give the nearest one. So it would never throw an exception for a bad frequency. Then I will also add an attribute to return the actual frequency. If you like that, hold off on the merge and I'll push something new.

@deshipu
Copy link

deshipu commented Jan 30, 2018

Yes, I think that would be the best approach — I think many APIs behave that way. However, I don't have the rights to merge anything here. I'm still more than happy to review and test it, of course.

@dhalbert
Copy link
Collaborator Author

@deshipu @tannewt updated since last night:

atmel-samd: Correct computation of SPI baud rate.
all: Add .frequency read-only property for busio.SPI to return actual frequency.

Fix esp8266/posix_helpers.c, which was not up to date for the new long-lived/short-lived heap allocation scheme.

@dhalbert
Copy link
Collaborator Author

git breakage: hold on until I fix the commits.

@dhalbert dhalbert force-pushed the 3.0_issue_464_spi_baud_rate branch from 080c9ff to 69924ac Compare January 30, 2018 17:06
all: Add .frequency read-only property for busio.SPI to return actual frequency.

Fix esp8266/posix_helpers.c, which was not up to date for the new
long-lived/short-lived heap allocation scheme.
@dhalbert dhalbert force-pushed the 3.0_issue_464_spi_baud_rate branch from 69924ac to e550b02 Compare January 30, 2018 17:11
@ladyada
Copy link
Member

ladyada commented Jan 30, 2018

please note, 24 mhz is outside the spec for the SERCOM max clock speed
so you can do it but its 'unofficial overclocking' :)

arduino/ArduinoCore-samd#147
http://community.atmel.com/forum/samd21-sd-card-spi-max-speed
http://atmel.force.com/support/articles/en_US/FAQ/SPI-max-clock-frequency-in-SAMD-SAMR-devices (which doesnt exist anymore, thanks Microchip!)

@deshipu
Copy link

deshipu commented Jan 30, 2018

We can keep the default in all the libraries at 12MHz, but a 24MHz SPI is really a huge improvement with displays (and only slightly out of the 20MHz spec for them), and I think it's worth it to be able to set it when one knows what they are doing.

@dhalbert
Copy link
Collaborator Author

arduino/ArduinoCore-samd#147 (comment) mentions that the datasheet gives a Typical value of 12MHz, and doesn't specify a Max. The SAMD51 datasheet doesn't specify a Max either. I can allow it but add a warning to the API documentation. Would that be OK?

deshipu
deshipu previously approved these changes Jan 30, 2018
@ladyada
Copy link
Member

ladyada commented Jan 30, 2018

yep that sounds good to me!

Copy link
Member

@tannewt tannewt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great! Its backwards compatible too.

@joseangeljimenez
Copy link

joseangeljimenez commented Jan 31, 2018

Additional clarification:

  • 24 MHz can work absolutely fine (at least inside the MCU) and should not be considered overclocking. The SAMD21 datasheet specifies a typical SPI SCK period (tSCK) of 42 ns, which translates into a typical SPI clock of 23.8 MHz. Hence, a 24 MHz clock is within a 1 % tolerance with respect to the typical value given in the datasheet.

  • Despite the previous, in my pull request Fix calculation of SPI_MIN_CLOCK_DIVIDER in SPI.h arduino/ArduinoCore-samd#292, I conservatively kept 12 MHz as the maximum clock for the SPI as this was the historical behavior of this code in the ArduinoCore repo.

  • I guess the historical 12 MHz top is due to the potential issue that enthusiasts prototyping with "air wires" and breadboards, without a proper PCB and layout, will get a malfunctioning SPI bus at higher frequencies. But that is due to quick & dirty prototyping techniques not due to an issue with the SAMD chips.

  • My main goal for the above pull request is that the SPI divider is calculated in the same way no matter which exact SAMD variant is used.

@ladyada
Copy link
Member

ladyada commented Jan 31, 2018

@joseangeljimenez hiya can you show us where you see tSCK of 42 ns? The one I just downloaded from microchip shows 84:

image

12 MHz was not chosen due to air-wiring/breadboard constraints. It was definitely the 'official' max frequency for proper SPI operation from Atmel :) I don't mind allowing 24 MHz but its not officially supported :)

@joseangeljimenez
Copy link

@ladyada I am really sorry for the confusion! I checked this using my old locally stored datasheet rev 42181D (dated 09/2014), where it states tSCK (typ.) = 42 ns. You are absolutely right that the newest datasheet shows 84 ns.

I have been working with the SAMD21 for several years now and designed several boards using SPI clocks up to 24 MHz, with a proper layout (5-6 mils narrow, short tracks PCB). I guess that Atmel revised (conservatively) the typical value of tSCK.

image

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants