Introduce calibration API for the MSPI subsystem

[natto1784]

Problem Description

Some MSPI controllers may need to be calibrated with non deterministic values. This calibration is different from the configuration that happens using the existing APIs, as these may require some mechanism to determine whether the calibration was successful or not. An example of this would be verifying the JEDEC ID for verifying correct calibration.

Proposed Change (Summary)

This proposal includes adding an API to calibrate the controller by supplying a callback mechanism to check and re-calibrate as needed. If this API is not implemented for a controller, it will simply return 0 for success as calibration is not needed. The API is intended to be used by MSPI device peripherals such as flash as well as user applications.

Proposed Change (Detailed)

Introduction of the API with following new types and symbols

/**
 * @typedef mspi_calibration_callback_t
 * @brief Define the calibration callback function signature
 *
 * @param Opaque data (void pointer)
 *
 * @retval 0 if successful.
 * @retval Negative value if unsuccessful
 */
typedef void (*mspi_calibration_callback_t)(void *calibration_callback_data);

typedef int (*mspi_api_calibrate)(const struct device *controller, mspi_calibration_callback_t cb,
				  void *cb_data);

__subsystem struct mspi_driver_api {
	mspi_api_config config;
	mspi_api_dev_config dev_config;
	mspi_api_get_channel_status get_channel_status;
	mspi_api_transceive transceive;
	mspi_api_register_callback register_callback;
	mspi_api_xip_config xip_config;
	mspi_api_scramble_config scramble_config;
	mspi_api_timing_config timing_config;
        /* NEW */
	mspi_api_calibrate calibrate;
};


/**
 * @brief Calibrate controller
 *
 * This routine provides a way for the the caller to calibrate the controller based on a callback
 * function. This function can be used to determine whether the calibration is successful and
 * recalibrate as needed.
 *
 * @param controller Pointer to the device structure for the driver instance.
 * @param cb Pointer to the callback function to determine whether calibration was successful or
 * not.
 * @param cb_data Opaque pointer to the data to be passed to the callback function
 *
 * @retval 0 If successful or unneeded
 * @retval -ENOTSUP Failed to calibrate
 */
static inline int mspi_calibrate(const struct device *controller, mspi_calibration_callback_t cb,
				 void *cb_data)
{
	const struct mspi_driver_api *api = (const struct mspi_driver_api *)controller->api;

	if (!api->calibrate) {
		return 0;
	}

	return api->calibrate(controller, cb, cb_data);
}

Dependencies

No response

Concerns and Unresolved Questions

No response

Alternatives Considered

We can add MSPI controller specific calibration code in the MSPI devices (peripherals) using compile time macros but that would be undesirable in the long run.

[github-actions]

Hi @natto1784! We appreciate you submitting your first issue for our open-source project. 🌟

Even though I'm a bot, I can assure you that the whole community is genuinely grateful for your time and effort. 🤖💙

[m-braunschweig]

A few thoughts I have:

• I would assume for the same PCB layout and the timing calibration values lead to the same stability for data transfers. In that case it might still be faster/safer to provide the data via the timing API and the devicetree (safer since barely stable values can't be taken as stable by mistake and opcodes with bad timing can't be accidentally interpreted as other opcodes). However an API or/and sample for determining these values dynamically would still be pretty valuable to reduce the required effort (as opposed to determining delays manually). However I also see the benefit or being able to just call an API for determining and applying these values at runtime
• I think for devices not implementing calibration it would be good to allow returning -ENOSYS for consistency. For failed calibrations (e.g. no stable values found) something like -EIO should rather be returned since -ENOTSUP is reserved for hardware not supporting an API. Source: Return codes in the API Design Guidelines
• For lower complexity in calling the API it might be preferable to have it synchronous to avoid users having to add an extra spinlock when they want to wait until the calibration is finished after calling it. Another idea would be to specify that, if no callback function is given (NULL), the function behaves synchronous
• Instead of a void pointer as parameter it might be good to instead have a pointer to a struct that includes a transfer request, expected data result and then a void pointer for device specific data (which shouldn't be needed in most cases). I hope in most cases a transfer with expected result should be enough
• In case an implementation sends the same request repeaditly with different configurations it might be a good idea to add shared code for that in a header file so drivers can use that instead of rewriting the code over and over again

Note: I didn't do such a calibration yet so the first point mainly relies on what I think potential problems could be. Also I'm pinging @swift-tk according to the MAINTAINERS.yml file for more opinions

[natto1784]

Great comments,

* I would assume for the same PCB layout and the timing calibration values lead to the same stability for data transfers. In that case it might still be faster/safer to provide the data via the timing API and the devicetree (safer since barely stable values can't be taken as stable by mistake and opcodes with bad timing can't be accidentally interpreted as other opcodes). However an API or/and sample for determining these values dynamically would still be pretty valuable to reduce the required effort (as opposed to determining delays manually). However I also see the benefit or being able to just call an API for determining and applying these values at runtime

That is the easiest way to go about it, however determining dynamically allows more accurate calibration and as you said, saves the required effort of manual configuration. I asked for suggestions at my workplace and it seems the consensus is strongly towards doing it dynamically. However, feel free to speak your own mind on it.

* I think for devices not implementing calibration it would be good to allow returning `-ENOSYS` for consistency. For failed calibrations (e.g. no stable values found) something like `-EIO` should rather be returned since `-ENOTSUP` is reserved for hardware not supporting an API. Source: [Return codes in the API Design Guidelines](https://docs.zephyrproject.org/latest/develop/api/design_guidelines.html#return-codes)

That makes perfect sense

* For lower complexity in calling the API it might be preferable to have it synchronous to avoid users having to add an extra spinlock when they want to wait until the calibration is finished after calling it. Another idea would be to specify that, if no callback function is given (`NULL`), the function behaves synchronous

I think so too, otherwise most usecases are going to involve locks anyway.

* Instead of a void pointer as parameter it might be good to instead have a pointer to a struct that includes a transfer request, expected data result and then a void pointer for device specific data (which shouldn't be needed in most cases). I hope in most cases a transfer with expected result should be enough

I am all for it, this also solves the previous issue nicely. Using a callback is more versatile but this solves at least the problem we had. A void pointer is not going to be necessary IMO in such a case.

* In case an implementation sends the same request repeaditly with different configurations it might be a good idea to add shared code for that in a header file so drivers can use that instead of rewriting the code over and over again

I believe repeated recalibrations should be handled by the controller itself, but I can imagine a use case where the caller needs to adjust the expected data as well.

[swift-tk]

What kind of calibration code are we talking here? Why do we need a callback here? Doesn't the calibrate API return with desirable result?

Is the API intended for runtime or just during initialization? How would this API get used?

What is the difference between this API and mspi_timing_config?

Does the timing or data come up from your calibration routine affected by the device types(psram, flash) or device states(freq, io mode, sdr/ddr, dqs/nondqs)?
Does your calibration routine involves writing and reading data from externally connected slave device?

There are some discussion on how to handle vendor specific timing at #91166.

I have a calibration utility in a PR(https://github.com/zephyrproject-rtos/zephyr/pull/88858/files), the file is drivers/mspi/mspi_ambiq_timing_scan.c. Our timing is vendor specific but heavily influenced by the kind of device the controller connects to, thus they have to be on the device layer.

[natto1784]

What kind of calibration code are we talking here? Why do we need a callback here? Doesn't the calibrate API return with desirable result?

There is a configurable delay that we use in our controller that is different from txrx dummy cycles. This is something we figure out dynamically, and to verify if it works we do RDID operations.

Is the API intended for runtime or just during initialization? How would this API get used?

During initialization most likely

What is the difference between this API and mspi_timing_config?

Say, I am calibrating from a flash device then that would mean including controller specific code to make mspi_timing_config work, which we do not know or care about from the flash's perspective.

Does the timing or data come up from your calibration routine affected by the device types(psram, flash) or device states(freq, io mode, sdr/ddr, dqs/nondqs)? Does your calibration routine involves writing and reading data from externally connected slave device?

Yes, this is indeed the case.

There are some discussion on how to handle vendor specific timing at #91166.

I have a calibration utility in a PR(https://github.com/zephyrproject-rtos/zephyr/pull/88858/files), the file is drivers/mspi/mspi_ambiq_timing_scan.c. Our timing is vendor specific but heavily influenced by the kind of device the controller connects to, thus they have to be on the device layer.

Will take a look at it, thanks.
Thanks.

[swift-tk]

What kind of calibration code are we talking here? Why do we need a callback here? Doesn't the calibrate API return with desirable result?

There is a configurable delay that we use in our controller that is different from txrx dummy cycles. This is something we figure out dynamically, and to verify if it works we do RDID operations.

Read ID op is a slave device specific thing to do, how do you run it on controller? This maybe too specific to controllers as they are not expected to know its device states. Additionally, if you are just reading ID, it may be insufficient considering the fact that register access may be different from the actual memory access op in terms of dummy cycle, io mode and even frequency. From my experience(Ambiq), there might be correlation to the transfer size as larger transfer means higher stress on power supply, so the timing you find on shorter access may have a problem on longer accesses.

Is the API intended for runtime or just during initialization? How would this API get used?

During initialization most likely

Runtime calibration may be time consuming, is this expected to be called from device layer init then?
I splitted the Ambiq runtime calibration into a utiltiy so we don't need to run from init.

What is the difference between this API and mspi_timing_config?

Say, I am calibrating from a flash device then that would mean including controller specific code to make mspi_timing_config work, which we do not know or care about from the flash's perspective.

okay, you need to get it from calibration and apply it with mspi_timing_config. But your timing parameter still would correspond to a set of flash(device) mspi settings.

[natto1784]

Read ID op is a slave device specific thing to do, how do you run it on controller?

We cannot, as of now, which is why this API (or something similar) is needed.

register access may be different from the actual memory access op in terms of dummy cycle, io mode and even frequency

This "delay" is different from the dummy cycles used by the MSPI peripheral (in our case, flash), hence it should be fine for different request/packet configurations.

Runtime calibration may be time consuming, is this expected to be called from device layer init then?

Yes, that is correct.

okay, you need to get it from calibration and apply it with mspi_timing_config. But your timing parameter still would correspond to a set of flash(device) mspi settings.

No, I plan on avoiding mspi_timing_config altogether as that would require controller specific fields and code in the flash (device) driver layer. A generic callback (or a request + expected_id) mechanism will allow the flash (device) to let the controller figure it out based on a RDID verification callback (or a request + expected_id) registered from the flash (device) itself.

[swift-tk]

Read ID op is a slave device specific thing to do, how do you run it on controller?

We cannot, as of now, which is why this API (or something similar) is needed.

Then the calibration is too vendor specific to be called from device level which could end up even worse than mspi_timing_config.

register access may be different from the actual memory access op in terms of dummy cycle, io mode and even frequency

This "delay" is different from the dummy cycles used by the MSPI peripheral (in our case, flash), hence it should be fine for different request/packet configurations.

I thought you said the delay is also kind of sensitive to different device state(op modes and etc). Then do you expect this timing changes when some part of the slave device DT changes?

Runtime calibration may be time consuming, is this expected to be called from device layer init then?

Yes, that is correct.

Then my suggestion would be put down calibration as utility to save boot up time and apply the timing by mspi_timing_config.

okay, you need to get it from calibration and apply it with mspi_timing_config. But your timing parameter still would correspond to a set of flash(device) mspi settings.

No, I plan on avoiding mspi_timing_config altogether as that would require controller specific fields and code in the flash (device) driver layer. A generic callback (or a request + expected_id) mechanism will allow the flash (device) to let the controller figure it out based on a RDID verification callback (or a request + expected_id) registered from the flash (device) itself.

Where is this API getting called exactly? Do you have code to look at?
If this API is to be used on device level, it could be messier than mspi_timing_config because other controller might not even share the same process which is calibration by read device ID.

[natto1784]

My apologies for the very late reply.

Then the calibration is too vendor specific to be called from device level which could end up even worse than mspi_timing_config.

The hard part is that even mspi_timing_config is very much vendor (controller) specific.

I thought you said the delay is also kind of sensitive to different device state(op modes and etc). Then do you expect this timing changes when some part of the slave device DT changes?

Yes, that is correct, which is why there needs to be some mechanism to configure this "delay" from the slave.

Then my suggestion would be put down calibration as utility to save boot up time and apply the timing by mspi_timing_config.

Then, is this expected to be called by the application rather than the slave? That is something I want to avoid if possible.

Where is this API getting called exactly? Do you have code to look at?

It just essentially passes a function that does read_jedec_id and matches it with DT to the controller. If the function executes successfully, then that would mean calibration is successful and the controller would return success, otherwise the controller retries until it runs out of values to configure and returns an error.

If this API is to be used on device level, it could be messier than mspi_timing_config because other controller might not even share the same process which is calibration by read device ID.

That makes sense.

[swift-tk]

My apologies for the very late reply.

Then the calibration is too vendor specific to be called from device level which could end up even worse than mspi_timing_config.

The hard part is that even mspi_timing_config is very much vendor (controller) specific.

I thought you said the delay is also kind of sensitive to different device state(op modes and etc). Then do you expect this timing changes when some part of the slave device DT changes?

Yes, that is correct, which is why there needs to be some mechanism to configure this "delay" from the slave.

It was intended to be vendor specific... The plan was to have vendors write down the parameters obtained from calibration run in dts and apply to the controller after device init or device acquire during runtime switching. The parameters might be different, but it would just require a vendor specific DTMacro to obtain that and pass them by the opague type from the slave.

Then my suggestion would be put down calibration as utility to save boot up time and apply the timing by mspi_timing_config.

Then, is this expected to be called by the application rather than the slave? That is something I want to avoid if possible.

I think in general, the timing calibration is not part of the user software unless they change drastically due to temperature or some other enviromental factor. Given the design is set on the PCB, the calibration may just be performed for the whole life time of the product. So to answer your question, yes, the utility would be from application level but still not part of the user software. If calibration run gets put into init, the process becomes somewhat permanent which would not only slow down initialization but also unnecessarily occupies ROM space.

Where is this API getting called exactly? Do you have code to look at?

It just essentially passes a function that does read_jedec_id and matches it with DT to the controller. If the function executes successfully, then that would mean calibration is successful and the controller would return success, otherwise the controller retries until it runs out of values to configure and returns an error.

I don't think passing a function that should typically run device level to controller is the correct practice... If it should run on device level, then that's where it shall stay. If not, then what is the point having device APIs... You might ought to explore the option to add a generic flash API that utilizes read ID or read learning pattern for calibration provided that flash devices follow standard jedec spec.