Skip to content

I3C Controller API #46087

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

Merged
merged 16 commits into from
Sep 9, 2022
Merged

I3C Controller API #46087

merged 16 commits into from
Sep 9, 2022

Conversation

dcpleung
Copy link
Member

@dcpleung dcpleung commented May 27, 2022

This introduces a basic API for I3C controller, and sample usage. This is developed using i.MX RT685 Evaluation Board (part number MIMXRT685-EVK) and the X-NUCLEO-IKS01A3 add-on board (especially the LSM6DSO sensor onboard).

Requires zephyrproject-rtos/hal_nxp#171

Here are a few things that have been working with LSM6DSO:

  • Broadcast RSTDAA works.
  • SETDASA works for LSM6DSO with address 0x6B.
  • GETBCR/GETDCR works.
  • Able to get chip ID (0x6C) from the sensor.

There are still a few things to be done:

  • Make LPH22HH sample app work via I3C
    • lps22hh_init_chip finishes without error
    • Polling mode working
    • Trigger mode working
  • DAA works (with LSM6DSO0)
  • IBI for the MCUX driver.
  • IBI for LPS22HH
  • #define for register value (shift, mask, etc.)
  • More helpers functions for CCC.
  • Reconsider the I2C portion of the API => Using a virtual controller as a bridge.
  • Provide I2C native API in I3C controller so that I2C devices can function without modifications (and without the virtual controller).

@dcpleung dcpleung force-pushed the i3c_api branch 3 times, most recently from e6ed176 to b925827 Compare May 27, 2022 22:52
int (*i3c_device_register)(const struct device *dev,
struct i3c_target_desc *desc);

int (*i3c_i2c_xfers)(const struct device *dev,
Copy link
Contributor

@teburd teburd May 28, 2022

Choose a reason for hiding this comment

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

Is it actually required by the driver to provide this transactional scatter/gather like functionality or can it provide a simpler subset with a helper that does the scatter/gather loop?

Copy link
Member Author

Choose a reason for hiding this comment

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

Not necessarily... this is simply following the I2C API to make it a bit more familiar to developers.

Copy link
Contributor

@teburd teburd Jun 1, 2022

Choose a reason for hiding this comment

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

This API leaves it up to the driver to do polling/isr looping over each message transfer, while also ensuring the device is exclusively held from others.

The result is, at least in i2c, each driver basically repeats this same bit of looping logic and mutex/semaphore locking logic.

It means writing an async API each driver now has to reimplement the loop in an async way effectively replicating the logic again.

I'd at least strongly consider if it makes more sense to have the driver API provide a primitive for sending a single message with a callback rather than the entire message list. It would be relatively simple to build a i3c_transfer and i3c_transfer_async on top of that primitive. Leaving the exclusive access and looping logic in common code.

I get that might require some additional signaling/flags sent to the driver, especially around a RESTART.

Copy link
Member Author

Choose a reason for hiding this comment

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

Could you make a mock up of what you have in mind on the I2C side? I am not imaginative enough to picture what the whole solution would be.

Copy link
Contributor

@teburd teburd Jun 3, 2022

Choose a reason for hiding this comment

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

I can, it won't be until after next week though probably. It's on my TODO list of potential improvements to i2c anyways so makes sense!

Copy link
Member

Choose a reason for hiding this comment

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

I can't quite picture what @teburd is saying either... but if the i3c_driver_api nests the i2c_driver_api it should be able to reuse all those i2c_burst_write, i2c_write_read, etc... This would also leave it open in the future for when/if an i2c async api is added to the i2c_driver_api it would just be a matter of implementing that api within the nested i2c_driver_api within the i3c controller
discussion here: #46087 (comment)

@XenuIsWatching
Copy link
Member

I think this is awesome that i3c could come to Zephyr!!! 🥳 I've been working a lot with i3c myself, and have maintained zephyr drivers for i3c devices internally (within our company) and controllers, but as none of these are "off-the-shelf" IPs for microcontrollers like that NXP you've got there, this has made upstreaming our work somewhat useless for the meantime...

Overall, I think this is a really great start and I have a lot of comments about this work.

/*
* Set static addresses as dynamic addresses.
*/
ret = mcux_i3c_do_setdasa(dev, &need_daa);
Copy link
Member

Choose a reason for hiding this comment

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

Shouldn't this section be broken out into the i3c_common for setting dynamic addresses? I would imaging following the FSM for initialization should be common between all i3c controllers

Copy link
Member Author

Choose a reason for hiding this comment

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

This will get factored out when I start writing more helper functions for CCC.

continue;
}

ret = i3c_ccc_do_getbcr(desc, &bcr);
Copy link
Member

Choose a reason for hiding this comment

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

getting bcr and dcr could be within a "common" "getting device info" function rather than within the driver. There also could be a lot more added for "getting device info" that would be common for all device controllers based on bits within the bcr such as speed limits and hdr modes supported.

Note that the DT node of a I3C target device should be in format
"<node name>@<static address>", e.g. "sensor@42".

For I2C devices, the 3 fields are static address, 0x00, and
Copy link
Member

@XenuIsWatching XenuIsWatching May 30, 2022

Choose a reason for hiding this comment

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

Would this even work for including i2c devices on the bus? The way the python dts scripts work with existing i2c drivers is that they would match the bus: parameter with the on-bus: parameter. All those existing i2c devices are defined with their parameter as on-bus: i2c and will not match with an i3c device. This would require a lot more under the hood work to get working... I tried in the past with #32730 ... but there wasn't a lot of interest in i3c at that time. Probably what ill need to happen for i3c devices to support all the existing work done on i2c devices would some under the hood work done to support i2c devices binding to i3c devices as I attempted in that pull request. It's also probably best if the lvr field is added to the i2c-device.yaml as a field rather than within the reg as a required: false and default to 0.

Perhaps @mbolivar could comment more about this on-bus: and bus: compatibility issue with how an i3c controller can include i2c devices on its bus.

Copy link
Member Author

Choose a reason for hiding this comment

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

If you look under dts/bindings/sensor, there are st,lsm6dso-i2c.yaml and st,lsm6dso-spi.yaml where they include another YAML file with corresponding on-bus property. To support it on I3C bus, another one st,lsm6dso-i3c.yaml is being added. In the current scheme, this will require some driver work to enable them to work in a I3C bus, as DT_ON_BUS() and DT_INST_ON_BUS() only work on one type of bus.

Copy link
Member

Choose a reason for hiding this comment

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

yes, but for an existing driver such as atmel,at24.yaml. This would only exist for the i2c protocol and includes the i2c-device.yaml. If this is defined within the devicetree under an i3c bus... it will not bind and pull in it's definitions. That is what I was trying to fix with that pull request.
I don't think it's feasible to go into every i2c device driver and add the DT_ON_BUS() for i3c and have it load in the i2c api calls. It should just work with the existing i2c definition...

struct i3c_target_desc;
struct i3c_i2c_device_desc;

__subsystem struct i3c_driver_api {
Copy link
Member

Choose a reason for hiding this comment

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

The I2C api can be nested within the i3c_driver_api. That way all the existing i2c api call the i3c dev for i2c transactions.

For example, a driver could use the existing API for i2c transfers and for configure

i3c_i2c_xfers removed from the struct. It should be expected that the hardware driver will implement the i2c api function calls within them to point to the i2c_api

__subsystem struct i3c_driver_api {
	struct i2c_driver_api i2c_api;
	int (*do_daa)(const struct device *dev);
	int (*do_daa)(const struct device *dev);
	int (*do_ccc)(const struct device *dev,
		      struct i3c_ccc_payload *payload);

	int (*i3c_xfers)(const struct device *dev,
			 struct i3c_target_desc *target,
			 struct i3c_msg *msgs,
			 uint8_t num_msgs);
	int (*i3c_device_register)(const struct device *dev,
				   struct i3c_target_desc *desc);
	int (*i3c_i2c_device_register)(const struct device *dev,
				       struct i3c_i2c_device_desc *desc);

	int (*i3c_ibi_slot_request)(const struct device *dev,
				    struct i3c_target_desc *target);
	int (*i3c_ibi_slot_free)(const struct device *dev,
				 struct i3c_target_desc *target);
	int (*i3c_ibi_enable)(const struct device *dev,
			      struct i3c_target_desc *target);
	int (*i3c_ibi_disable)(const struct device *dev,
			       struct i3c_target_desc *target);
};

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 am aware of a I3C controller where devices are not addressed using target address directly, and there is a redirection table involved. Without using the struct i3c_i2c_device_desc for the controller driver to store addition data to locate the entries in the redirection table, the controller driver will need to iterate the entries in the redirection table for the entry with the desired target address. This results in additional processing for each transfer that can be avoided by shifting the burden to when the driver is being written (by having struct i3c_i2c_device_desc).

Copy link
Member

Choose a reason for hiding this comment

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

I'm not quite sure what you're on about here.... but I was referring to maintain compatibility with existing i2c device drivers that use the i2c_driver_api.
So that when a device driver is on that i3c bus... it can use the same i2c calls that have always existed such as the i2c_write() function here for example

err = i2c_write(config->bus.i2c.bus, block, sizeof(block),

When it defined in the i3c controller driver... it'll just look like this for that nxp..

static const struct i3c_driver_api mcux_i3c_driver_api = {
	.i2c_api = {
		.transfer = mcux_i3c_i2c_transfer,
		.configure = mcux_i3c_i2c_configure,
	},
	.do_daa = mcux_i3c_do_daa,
	.do_ccc = mcux_i3c_do_ccc,

	.i3c_device_register = mcux_i3c_dev_register,
	.i3c_i2c_device_register = mcux_i3c_i2c_dev_register,

	.i3c_xfers = mcux_i3c_transfer,
};

Yes, I actually use that I3C controller that uses a retaining register with the target's dyanmic address... This would actually require some additional optional apis that aren't in this struct. Similar to the ones in Linux which are attach_i3c, reattach_i3c, and the detach_i3c apis (and same for i2c). If those controllers where to be implemented, then I would expect it to add those devices into it's retaining registers with i3c_device_register and i3c_i2c_device_register. I don't this this NXP controller would need some of those...

That controller does not need to iterate through those retaining registers

Copy link
Member Author

Choose a reason for hiding this comment

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

That particular controller is not using simple registers but actually have an array of records on the memory of controller hardware. The idea of using the struct is simply to store some data in the controller_priv field for quick lookup. Note that the size of this memory (and thus number of addressable devices) is not fixed and can vary between actual implementation on hardware. The number of entries can be queried from hardware. If the driver of such controller is implemented as you say, the driver will need to have some way of pre-allocate a lookup table at build time. This will require some configuration be specified for each board or platform, and possibly resulting in some confusion during board bring-up. If we amend the drivers to use the I3C specific API, this can be done on the main Zephyr repo. This is kind of shifting the burden from downstream users to upstream driver writers. The intent is to make it easier for downstream to consume Zephyr. That's the thought process. Either way, the actual user of the devices (e.g. sensors) would be shielded from this low level details.

TBH, either way is fine to me. I don't have much attachment to the current proposed implementation. One way or the other, someone has the burden to make everything work on the final product (whether upstream or downstream, and also the driver writers). Though, one thing to consider is that, if reusing the I2C API, the device would not know whether it is connected to a I2C or I3C controller (... or that if this information is even important).

Note that if we go the reusing route, that will not be simple drop-in either. It will require some work on the devicetree side. One is the bus property. Another one is iterating children nodes, as we need to distinguish between I2C and I3C devices.

Copy link
Member Author

Choose a reason for hiding this comment

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

Though, let me get the part with only I3C devices working correctly first.

Copy link
Member

Choose a reason for hiding this comment

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

That particular controller is not using simple registers but actually have an array of records on the memory of controller hardware. The idea of using the struct is simply to store some data in the controller_priv field for quick lookup. Note that the size of this memory (and thus number of addressable devices) is not fixed and can vary between actual implementation on hardware. The number of entries can be queried from hardware. If the driver of such controller is implemented as you say, the driver will need to have some way of pre-allocate a lookup table at build time. This will require some configuration be specified for each board or platform, and possibly resulting in some confusion during board bring-up. If we amend the drivers to use the I3C specific API, this can be done on the main Zephyr repo. This is kind of shifting the burden from downstream users to upstream driver writers. The intent is to make it easier for downstream to consume Zephyr. That's the thought process. Either way, the actual user of the devices (e.g. sensors) would be shielded from this low level details.

Ah, I understand what you're saying now. Yeah, for that controller, there would have to be pre-allocated lookup table and that's how we're doing it for that controller within our Zephyr fork with a controller_priv.

TBH, either way is fine to me. I don't have much attachment to the current proposed implementation. One way or the other, someone has the burden to make everything work on the final product (whether upstream or downstream, and also the driver writers). Though, one thing to consider is that, if reusing the I2C API, the device would not know whether it is connected to a I2C or I3C controller (... or that if this information is even important).

Note that if we go the reusing route, that will not be simple drop-in either. It will require some work on the devicetree side. One is the bus property. Another one is iterating children nodes, as we need to distinguish between I2C and I3C devices.

I believe that we should maintain portability with drivers existing on i2c buses to stick with that i2c api and expect all i3c controllers to implement the i2c api as well. That way any existing code that does the i2c api will 'just work' on an i3c controller. I would rather see this shifted to "upstream"... because I would see that as requiring less work and would be easier to maintain IMO but I'm also curious to hear other's opinions. It would only need to be implemented within the controller code (or a common i3c header file) rather than going back and editing all existing i2c drivers and expecting all future i2c drivers to implement i3c api calls.

You're right if we go the reusing route, that would require some 'one-time' work on the bus property (like what I tried with #32730)... but once that work is completed we should be able to distinguish between I2C and I3C devices through the DT_INST_FOREACH_CHILD macro and the DT_INST_ON_BUS macro (I think?) that could build a struct i3c_i2c_device_desc to be passed in to the i3c_i2c_device_register function...

Copy link
Member

@XenuIsWatching XenuIsWatching Jun 3, 2022

Choose a reason for hiding this comment

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

Though, let me get the part with only I3C devices working correctly first.

yes, for sure. I'm sure there's plenty of I3C work to be done.
Meanwhile in parallel, I'll see if can get some discussion and input from other people about how I2C devices could be implemented. There would also need to be some work within the zephyr build system to support a bus that as compatibility with another bus.

Copy link
Member

Choose a reason for hiding this comment

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

I'm beginning to think another option for i2c on an i3c bus would be to use a child binding on the DTS for the i2c devices to be nested within the i3c node in the device tree. Kinda like the tca954x driver..

struct i3c_i2c_device_desc;

__subsystem struct i3c_driver_api {
int (*do_daa)(const struct device *dev);
Copy link
Member

Choose a reason for hiding this comment

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

use typedefs for these function pointers

Copy link
Member Author

Choose a reason for hiding this comment

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

There is no requirement saying that these must use typedef. My reason for not using typedef is simply that, under IDE, you can see all the function signatures inside the struct in code reference pop-ups, instead of needing to do one redirect to see the signature.

struct i3c_target_desc;
struct i3c_i2c_device_desc;

__subsystem struct i3c_driver_api {
Copy link
Member

Choose a reason for hiding this comment

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

Would a "i3c_set_speed" api be a need here?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, it is one of the task to write more CCC helper functions.

@hongshui3000
Copy link
Contributor

@dcpleung
Copy link
Member Author

dcpleung commented Jun 1, 2022

AspeedTech zephyr I3C: https://github.com/AspeedTech-BMC/zephyr/tree/aspeed-main-v2.6.0/drivers/i3c

Do you happen to have a link to the publicly available datasheet?

@dcpleung dcpleung force-pushed the i3c_api branch 3 times, most recently from cb1ef05 to d2e03b2 Compare June 2, 2022 01:12
@zephyrbot zephyrbot added manifest west manifest-hal_nxp DNM This PR should not be merged (Do Not Merge) labels Jun 2, 2022
@zephyrbot
Copy link

zephyrbot commented Jun 2, 2022

The following west manifest projects have been modified in this Pull Request:

Name Old Revision New Revision Diff
hal_nxp zephyrproject-rtos/hal_nxp@00acb97 zephyrproject-rtos/hal_nxp@64d813a zephyrproject-rtos/[email protected]

Note: This message is automatically posted and updated by the Manifest GitHub Action.

@hongshui3000
Copy link
Contributor

I only know that the controller ip they use isDesignWare MIPI I3C Controller IP

@dcpleung dcpleung force-pushed the i3c_api branch 2 times, most recently from 0b47cb6 to cd32eee Compare June 3, 2022 21:40
Copy link
Member

@gmarull gmarull left a comment

Choose a reason for hiding this comment

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

just some comments about generic stuff, not familiar with i3c

config I3C_MCUX
bool "MCUX I3C driver"
depends on HAS_MCUX
select PINCTRL
Copy link
Member

Choose a reason for hiding this comment

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

why select pinctrl here? better depends?

Copy link
Member Author

Choose a reason for hiding this comment

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

CONFIG_I2C_MCUX also selects PINCTRL and I simply copied it from there.

int bitpos;
int idx;

__ASSERT_NO_MSG(slots != NULL);
Copy link
Member

Choose a reason for hiding this comment

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

note here, afaik we tipically not assert for programming errors like this one.

Copy link
Member Author

Choose a reason for hiding this comment

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

This is a new API, so I thought it could help the early adopters. :)

*
* @return String associated with the @p status.
*/
static const char *mcux_i3c_cb_status_str_get(status_t status)
Copy link
Member

Choose a reason for hiding this comment

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

shouldn't this be optional? are all these strings useful in production/release builds? maybe condition to CONFIG_LOG=y?)

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 changed it so that it only returns error strings if LOG is at debug level. Otherwise it simply returns an empty string so it prints nothing.

* struct i3c_device_desc to create an initializer for.
* @param ibi_cb Pointer to IBI callback.
*/
#define I3C_DEVICE_DESC_DT_WITH_IBI_CB(node_id, ibi_cb) \
Copy link
Member

Choose a reason for hiding this comment

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

we've been using the dt-spec pattern, so would be nice to use the same terminology here for consistency, comment applies to other initializers in this file, would probably be nice to have _dt APIs as well (if they apply)

Copy link
Member Author

Choose a reason for hiding this comment

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

The dt-spec pattern in I2C kind of implies that you can simply create a struct at runtime and pass it to the API. However, it cannot be done the same way here. The device descriptor struct is mutable at runtime, that it must be created at build time and its pointer must be passed to APIs. Therefore I am trying not to use the same terminology to avoid confusion. Though _DT_ is still there to indicate it has some properties are coming from devicetree. I am fine either way, so please let me know your thought on this.

Comment on lines 680 to 1023
#define I3C_DEVICE_DT_DEFINE(node_id, init_fn, pm_device, \
data_ptr, cfg_ptr, level, prio, \
api_ptr, ...) \
DEVICE_DT_DEFINE(node_id, init_fn, pm_device, \
data_ptr, cfg_ptr, level, prio, \
api_ptr, __VA_ARGS__)
Copy link
Member

Choose a reason for hiding this comment

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

what's the reason for this wrapper?

Copy link
Member Author

Choose a reason for hiding this comment

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

It originally came from I2C. I wish to keep the wrapper here so that it can be extended to incorporate statistics tracking later.

Copy link
Contributor

@mbolivar-nordic mbolivar-nordic left a comment

Choose a reason for hiding this comment

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

@dcpleung If you agree with the below requested changes, I'm fine with merging this now to hit RC1 and then treating them as fixes in the stabilization period, if you want to handle them in follow ups.

@@ -370,15 +373,15 @@ def write_ranges(node):
for i,range in enumerate(node.ranges):
idx_vals.append((f"{path_id}_RANGES_IDX_{i}_EXISTS", 1))

if node.bus == "pcie":
if node.buses is not None and "pcie" in node.buses:
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you use an empty list when there are no buses? That way you won't need the null check. The cases are:

  • no bus: node.buses is []
  • one bus: node.buses has one element, e.g. ["pcie"]
  • multiple buses: node.buses has multiple elements, e.g. ["i2c", "i3c"]

Copy link
Member Author

Choose a reason for hiding this comment

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

Changed to do empty list instead of None.

@@ -868,7 +871,7 @@ def write_global_macros(edt):
compat2buses = defaultdict(list) # just for "okay" nodes
for compat, okay_nodes in edt.compat2okay.items():
for node in okay_nodes:
bus = node.on_bus
bus = node.on_buses
Copy link
Contributor

Choose a reason for hiding this comment

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

But this is a list now; don't you need to use .extend below instead of .append, or something like that, along with any other needed changes? This doesn't seem right; what am I missing?

Copy link
Member Author

Choose a reason for hiding this comment

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

Fixed.


for one_bus in bus_list:
out_define(
f"DT_COMPAT_{str2ident(compat)}_BUS_{str2ident(one_bus)}", 1)
Copy link
Contributor

Choose a reason for hiding this comment

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

Updated the Node class to have 'buses' and 'on-buses' so they are both lists.

So the above if/else block should be simplified now that the type is known, right?

bus node, then this attribute is None.
attribute holds the list of supported bus types, e.g. ["i2c"], ["spi"]
or ["i3c", "i2c"] if multiple protocols are supported via the same bus.
If the node is not a bus node, then this attribute is None.
Copy link
Contributor

Choose a reason for hiding this comment

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

Making it an empty list in this case would be simpler for the user as shown above.

For a single bus that supports multiple protocols, e.g. I3C and I2C,
the single value "bus:" setting is no longer sufficient, as a I3C bus
cannot be matched to a device having "on-bus: I2C". This commit
extends the "bus:" setting so that it can accept a list of values.
This change allows corresponding devicetree macros to be generated
so that DT_ON_BUS() can work properly in this scenario.

Signed-off-by: Daniel Leung <[email protected]>
This adds a few bits to the devicetree API tests for multi-bus
nodes where a bus can support multiple protocols. This uses
I3C as basis as I3C controller can support both I2C and I3C on
the same bus, while I2C controller cannot support both. So
this needs to make sure the correct bus macros are generated
if appropriate (and not generated if not needed).

Signed-off-by: Daniel Leung <[email protected]>
This adds a base binding for I3C controllers.

Note that this follows the Linux kernel 5.17 bindings under
Documentation/devicetree/bindings/i3c/i3c.yaml.

Signed-off-by: Daniel Leung <[email protected]>
This introduces the I3C API for I3C controllers. Currently,
this supports one controller per bus under Zephyr.

Signed-off-by: Daniel Leung <[email protected]>
This adds the bits to allow an I3C controller to act as target
device. This is modelled after the I2C target model.

Signed-off-by: Daniel Leung <[email protected]>
Adds support for a global workqueue so drivers can defer
IBI callbacks instead of doing it in interrupt context.

Signed-off-by: Daniel Leung <[email protected]>
This adds a fews bits about the newly introduced I3C API
for I3C controller.

Signed-off-by: Daniel Leung <[email protected]>
This adds a binding for NXP MCUX I3C controller, with compatible
string "nxp,mcux-i3c".

Signed-off-by: Daniel Leung <[email protected]>
This adds a very basic driver to utilize the I3C IP block
on MCUX (e.g. RT685). Note that, for now, this only supports
being the active controller on the bus.

Origin: NXP MCUXpresso SDK
License: BSD 3-Clause
URL: https://github.com/zephyrproject-rtos/hal_nxp
Commit: 2302a1e
Purpose: Enabling the I3C controller on RT685.

Signed-off-by: Daniel Leung <[email protected]>
This adds a few bits to the RT6xx SoC code to support the I3C
bus interface on RT600 series.

Signed-off-by: Daniel Leung <[email protected]>
This adds the pinctrl/pinmux device tree node for the I3C
block on RT685 SoC.

Note the the drive strengths and slew rates for SCL and SDA
are recommended by the NXP application note AN12796: RT600 I3C
Simple Master, which can downloaded from the NXP website.

Signed-off-by: Daniel Leung <[email protected]>
This adds I3C access functions so that STM sensors connected
on I3C bus can utilize I3C.

Signed-off-by: Daniel Leung <[email protected]>
This adds a new YAML file to allow LPS22HH to work on I3C bus.

Signed-off-by: Daniel Leung <[email protected]>
This extends the lps22hh driver to support being on I3C bus.

Signed-off-by: Daniel Leung <[email protected]>
This adds some skeleton files to enable using LPS22HH on I3C bus.
This still uses the C source file for I2C. The new files here
are simply to provide a way to have overlay for each board that
would not conflict with the I2C one. Also this is set to build
only because it may require external hardware that is not
necessary on the board being tested.

Signed-off-by: Daniel Leung <[email protected]>
This adds some skeleton files to enable using LSM6DSO on I3C bus
while still acting like a I2C device. The new files here are
simply to provide a way to have overlay for each board that
would not conflict with the pure I2C one. Also this is set to
build only because it require external hardware that is not
necessary on the board being tested.

Signed-off-by: Daniel Leung <[email protected]>
Copy link
Contributor

@mbolivar-nordic mbolivar-nordic left a comment

Choose a reason for hiding this comment

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

Good enough for RC1. We'll fix anything else we need to during stabilization period. This may include further edtlib API changes, but I think that's unlikely. Thank you @dcpleung

@nashif nashif merged commit 47bf9ee into zephyrproject-rtos:main Sep 9, 2022
@dcpleung dcpleung deleted the i3c_api branch September 9, 2022 21:43
@XenuIsWatching
Copy link
Member

XenuIsWatching commented Sep 24, 2022

@galak @dcpleung Trying to catch up with my time here... but it looks like the conversation lead to somewhere I don't think i3c is built for. The i3c and i2c device list is now fixed with no run-time way of having this now.

The issue this can lead to is that i3c is hot-pluggable (unlike i2c). While Hotplug support is not in the zephyr i3c... yet. This will become an issue in the future. Where devices will need to be registered (and unregistered) at run time. I also see the slist nodes are left over in the i3c_device_desc and i3c_i2c_device_desc struct.
While having i2c devices listed at compile time makes 'some' sense with how i2c devices are defined to be fixed.... this can not work with i3c. For i2c, this could hamper an application if it needs to write to a 'specific' address that's not defined in the device tree, and there's no way to register the i2c address.... so i3c_i2c_device_find will always return an error for the i2c transaction.

This also would bring an issue where other i3c controllers (as mentioned earlier) have a hw lookup table that needs to be written to.

I see a reference to i3c_device_register in the comments, but that api got removed. It would be better if those were brought back.

I'll try to get an issue open this week describing my concerns.

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

Successfully merging this pull request may close these issues.