Skip to content

include: spi.h: Add native HW timing parameters to config API struct #87427

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.

Sign up for GitHub

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

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
+53 −17
Open

include: spi.h: Add native HW timing parameters to config API struct #87427

Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
38c69dc
include: spi.h: Add HW timing parameters
decsny Mar 20, 2025
8b0a93f
spi_nxp_lpspi: Support API timing parameters
decsny Mar 20, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
43 changes: 35 additions & 8 deletions include/zephyr/drivers/spi.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -152,24 +152,49 @@ extern "C" {
/**
* @brief SPI Chip Select control structure
*
* This can be used to control a CS line via a GPIO line, instead of
* using the controller inner CS logic.
* This describes how the SPI CS line should be controlled, including whether
* to use GPIO or native hardware CS, and timing parameters.
*
*/
struct spi_cs_control {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Perhaps it's time to rename this to spi_cs_and_delay_control

Copy link
Member Author

Choose a reason for hiding this comment

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

renaming would be an API change instead of an extension though, are you sure it's worth doing that, as opposed to just having a couple extra bytes in the overall spi_config struct (whether the way I have in this PR or introduce a new spi_timing struct)

Copy link
Collaborator

Choose a reason for hiding this comment

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

If we need an API change (whatever that could be), so be it. Better having a sane API rather than a half-hacked one.

/**
* GPIO devicetree specification of CS GPIO.
* The device pointer can be set to NULL to fully inhibit CS control if
* necessary. The GPIO flags GPIO_ACTIVE_LOW/GPIO_ACTIVE_HIGH should be
*
* If set to NULL, hardware driver can attempt to natively control CS
* automatically if it has that capability and the correct pinmux has
* been applied. Otherwise, NULL will inhibit CS control.
*
* The GPIO flags GPIO_ACTIVE_LOW/GPIO_ACTIVE_HIGH should be
* equivalent to SPI_CS_ACTIVE_HIGH/SPI_CS_ACTIVE_LOW options in struct
* spi_config.
*/
struct gpio_dt_spec gpio;

/**
* Delay in microseconds to wait before starting the
* transmission and before releasing the CS line.
* Timing configuration.
* For GPIO CS, delay is microseconds to wait before starting
Copy link
Collaborator

Choose a reason for hiding this comment

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

this was not only meant for gpio cs actually. thus why the structure was called spi_cs_control, and not spi_gpio_cs_control (for instance)

Copy link
Member Author

Choose a reason for hiding this comment

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

yeah, that was convenient that it was already named generically, I noticed

* transmission and releasing CS.
* For hardware CS, pcs_to_sck_delay_ns and sck_to_pcs_delay_ns
* control assertion/deassertion timing.
*/
uint32_t delay;
union {
/**
* In the case of GPIO CS, this is delay in microseconds to wait
* before starting the transmission and before releasing the CS line.
*/
uint32_t delay;

struct {
/** @brief Delay from PCS assertion to first SCK edge in nanoseconds.
* If set to 0, hardware default will be used.
*/
uint8_t pcs_to_sck_delay_ns;
Copy link
Contributor

Choose a reason for hiding this comment

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

Just curious what the standard is here, was looking at this relative to dw_spi and the delay is clock speed relative, not time relative. Might be worth also adding some helpers.

Copy link
Member Author

Choose a reason for hiding this comment

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

For NXP, the register programming model is also relative to an equation relating some clock speed and transfer prescalers, and not nanoseconds. But since this is generic API it seems better to use a standard unit which would mean the same no matter the platform, and driver can calculate how to program hardware.

Copy link
Contributor

Choose a reason for hiding this comment

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

Then that follows up that we should create some generic helpers if common platforms are going to be doing the same math

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't think so, because the math is completely platform specific, that's the point of why I want to specify in ns in the first place

/** @brief Delay from last SCK edge to PCS deassertion in nanoseconds.
* If set to 0, hardware default will be used.
*/
uint8_t sck_to_pcs_delay_ns;
};
};
};

/**
Expand Down Expand Up @@ -329,8 +354,10 @@ struct spi_config {
* if not used).
*/
struct spi_cs_control cs;
};

/** @brief Delay between data frames in nanoseconds (0 means 50% of frequency) */
uint16_t dfs_delay_ns;
Copy link
Collaborator

Choose a reason for hiding this comment

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

and this could go into the anonymous struct you added, in the union.

That way: to change in structure size.

That said, this is very much spi controller specific. Some controller do not offer anything to configure this.
And 0 as 50% of the frequency, why? why not just "hardware default"?

Copy link
Member Author

@decsny decsny Mar 21, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

I thought about this too but since the struct was called spi_cs_control and this was not related I thought it would be confusing

the reason for 50% of period is because of the same reason that is motivating this PR, which is that the hardware default for the NXP spi is the shortest possible, which causes problems and looks strange, and people often report it as a bug because the users actually expect that the behavior will be 50% of period by default, and don't even think about configuring it.

And this expectation is I think , not specific to people using the NXP part. A 50% delay of clk period between words means that the clock signal does not have any blips between words. Also this makes perhaps applications more portable because the hardware default can be totally different between parts but like I said maybe they just always expect 50% of period like has been reported to me

};
/**
* @brief Structure initializer for spi_config from devicetree
*
Expand Down