-
Notifications
You must be signed in to change notification settings - Fork 2k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
cpu/esp8266: allow arbitrary SPI clocks #20410
Conversation
/** | ||
* @brief Override SPI clock speed values | ||
* @{ | ||
*/ | ||
#define HAVE_SPI_CLK_T | ||
typedef enum { | ||
SPI_CLK_100KHZ = KHZ(100), /**< drive the SPI bus with 100KHz */ | ||
SPI_CLK_400KHZ = KHZ(400), /**< drive the SPI bus with 400KHz */ | ||
SPI_CLK_1MHZ = MHZ(1), /**< drive the SPI bus with 1MHz */ | ||
SPI_CLK_5MHZ = MHZ(5), /**< drive the SPI bus with 5MHz */ | ||
SPI_CLK_10MHZ = MHZ(10), /**< drive the SPI bus with 10MHz */ | ||
} spi_clk_t; | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why was this change necessary if the enum values are (no longer?) used?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The enum values only go from 0 to 4, with this you can just write the desired SPI frequency to the clk
parameter instead.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Where is the original enum definition? I somehow doubt it is a good idea to override those only for a single platform?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The previous definition was here - you will find that overwriting this is done by most platforms already since the original interface is pretty bad/limiting.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hum, maybe worth an issue to clean up the original interface then in a subsequent PR?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We might introduce a new feature for platforms where you can select arbitrary SPI clocks, so drivers can rely on this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I did not test this physically.
Sorry, #20438 will never get in if every merge conflicts and kicks it out. I will requeue when ready. |
Contribution description
The formula for the ESP8266 SPI clock is
clk_spi = clk_src / ((prediv + 1) * (N + 1))
where N has to be at least 1.Since
prediv
is 13 bit, this gets us down to 4882 Hz without further touching N, so just use that.Testing procedure
I connected an oscilloscope to the clock pin of
esp8266-esp-12x
and tried out all pre-defined SPI clocks withtests/periph/spi
:100 kHz
400 kHz
1 MHz
5 MHz
10 MHz
Issues/PRs references
useful for #20218