Skip to content
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

Merged
merged 1 commit into from
Mar 26, 2024

Conversation

benpicco
Copy link
Contributor

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 with tests/periph/spi:

2024-02-21 23:17:44,351 # Sent bytes
2024-02-21 23:17:44,359 #    0    1    2    3    4    5    6    7    8    9   10   11   12   13 
2024-02-21 23:17:44,365 #   0x74 0x68 0x69 0x73 0x20 0x69 0x73 0x20 0x61 0x20 0x74 0x65 0x73 0x74
2024-02-21 23:17:44,370 #     t    h    i    s         i    s         a         t    e    s    t 
2024-02-21 23:17:44,371 # 
2024-02-21 23:17:44,373 # Received bytes
2024-02-21 23:17:44,379 #    0    1    2    3    4    5    6    7    8    9   10   11   12   13 
2024-02-21 23:17:44,384 #   0x74 0x68 0x69 0x73 0x20 0x69 0x73 0x20 0x61 0x20 0x74 0x65 0x73 0x74
2024-02-21 23:17:44,392 #     t    h    i    s         i    s         a         t    e    s    t 

100 kHz

bmp_27_001

400 kHz

bmp_27_002

1 MHz

bmp_27_003

5 MHz

bmp_27_004

10 MHz

bmp_27_005

Issues/PRs references

useful for #20218

@benpicco benpicco added Type: enhancement The issue suggests enhanceable parts / The PR enhances parts of the codebase / documentation CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR labels Feb 21, 2024
@benpicco benpicco requested a review from maribu February 21, 2024 22:28
@riot-ci
Copy link

riot-ci commented Feb 21, 2024

Murdock results

✔️ PASSED

75bf0e3 cpu/esp8266: allow arbitrary SPI clocks

Success Failures Total Runtime
9997 0 9997 08m:28s

Artifacts

@github-actions github-actions bot added Platform: ESP Platform: This PR/issue effects ESP-based platforms Area: cpu Area: CPU/MCU ports labels Feb 22, 2024
Comment on lines +268 to +280
/**
* @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;

Copy link
Contributor

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?

Copy link
Contributor Author

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.

Copy link
Contributor

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?

Copy link
Contributor Author

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.

Copy link
Contributor

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?

Copy link
Contributor Author

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.

Copy link
Contributor

@Teufelchen1 Teufelchen1 left a 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.

@Teufelchen1 Teufelchen1 added this pull request to the merge queue Mar 26, 2024
@MrKevinWeiss
Copy link
Contributor

Sorry, #20438 will never get in if every merge conflicts and kicks it out. I will requeue when ready.

@MrKevinWeiss MrKevinWeiss removed this pull request from the merge queue due to a manual request Mar 26, 2024
@MrKevinWeiss MrKevinWeiss added this pull request to the merge queue Mar 26, 2024
Merged via the queue into RIOT-OS:master with commit 6c6d2f9 Mar 26, 2024
34 checks passed
@benpicco benpicco deleted the cpu/esp8266-spi_clk branch March 26, 2024 18:20
@MrKevinWeiss MrKevinWeiss added this to the Release 2024.04 milestone Apr 30, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area: cpu Area: CPU/MCU ports CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR Platform: ESP Platform: This PR/issue effects ESP-based platforms Type: enhancement The issue suggests enhanceable parts / The PR enhances parts of the codebase / documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants