i3c secondary controller driver

[jvasanth1]

We are developing zephyr i3c driver for our device which has a i3c secondary controller. The secondary controller will come up as target on the bus initially. We don't see enough support in current Zephyr i3c for implementing the secondary controller as target.

1. Zephyr supports two APIs- i3c_target_register and i3c_target_unregister to register the target to the controller. The registration uses the i3c_target_config to configure the target. But we really need fields from i3c_config_target like PID, HDR modes, MRL, MWL as well to configure the target.

Both structures are defined in target_device.h

Feature Request 1: i3c_target_register and i3c_target_unregister APIs need updates to incorporate the i3c_config_target fields

2. The target callbacks structure i3c_target_callbacks has read and write function which are byte oriented. I think we need to expand this to be buffer pointer and data length. We can't be processing byte by byte for I3C.

Feature request 2: Request to update i3c target callback functions to operate using buffer instead of byte by byte

[dcpleung]

@jvasanth1 Since you are actually writing drivers, could you also do the necessary changes to the target device API/interface? I do not have such a setup so I cannot test any changes.

[jvasanth1]

Feature request 2 would break the cdns secondary controller implementation since it implements the target using byte by byte callbacks.

[dcpleung]

@XenuIsWatching

[XenuIsWatching]

1. I do not know much about your controller but why you need to send over the PID, HDR, MRL, MWL to configured target from within the target itself. PID is something that makes sense though, as applications may want control over their MIPI Vendor ID as well as it's instance ID. but for HDR, MRL, and MWL are something that I'd expected to be controlled by the 'active controller' using the CCC commands for SETMRL and SETMWL to the i3c target as well as the controller is the one that will initiate the HDR transfer.
   Does your I3C peripheral have control over those such as are those written to in the beginning?
   I'm also not sure why'd you need to use the i3c_target_register and i3c_target_unregister for those. From that struct i3c_config_target. It makes sense to use the enum i3c_config_type with the enum type as I3C_CONFIG_TARGET and pass in i3c_config_target as the config parameter to the already existing i3c api i3c_configure

2. changing it to be a buffer might be okay for the cdns as that uses a hw fifo and how I utilize it is with a callback in a loop getting byte by byte the buffer to write in to the fifo... but I'm not sure what you gain with a buffer plus it may not be known how much data it can retrieve out of it as the buffer may end or the hw fifo can only take so much so I'm not quite sure how a buffer callback would be beneficial. With a buffer you'd need to know the exact number of bytes that can be retrieved from software and written in to the device hardware.

[jvasanth1]

My understanding is that the target needs to tell the controller if it supports a variable limit on the maximum number of data bytes per message. This is done through return values for GETMRL and GETMWL. Based on the read values, the controller may choose to modify them using SETMRL/SETMWL. In the i3c_device_basic_info_get() in i3c_common.c the controller reads the target's MRL and MWL and stores them which I believe the application code needs to refer before talking to the target. So we need ability to configure MRL and MWL during target initialization. Based on these values that the controller reads (through GETMRL, GETMWL) it may choose to modify them (using SETMRL, SETMWL).
Similar for HDR capability which needs to be configured for the target (based on target's capability) and which the controller discovers through GETCAPS command.

I missed the i3c_configure API; so we can use that for additional configurations (after i3c_target_register). But then it has to be initiated by the application. Do we need to consider moving some of these target configurations (PID, MRL, MWL, HDR modes) for the secondary controller to the device tree?

Regarding the read/write callbacks - byte by byte vs buffer pointer and datalen -->

The buffer pointer model is followed in smbus driver for smbus BLOCK READ and BLOCK WRITE functions - smbus_block_write, smbus_block_read. I am thinking similar model is required for i3c. In i3c there is no clock stretching on the bus. At high speeds, we don't want to ACK/NACK the next byte based on application's return value in ISR. We don't want to keep using the i2c type target callback functions for i3c.
For I3C write, my thinking is that the hardware should be always ready to receive all the bytes based on MWL. Similarly for I3C read, it should be ready to transmit the data expected by the controller. This is especially valid in MCTP over I3C where the target (MCTP slave) prepares MCTP packet in the transmit buffer before initiating IBI - see MCTP over I3C binding spec. This means that we need a target API which application can use to prepare for transmission before it calls the raise IBI API.

[XenuIsWatching]

My understanding is that the target needs to tell the controller if it supports a variable limit on the maximum number of data bytes per message. This is done through return values for GETMRL and GETMWL. Based on the read values, the controller may choose to modify them using SETMRL/SETMWL. In the i3c_device_basic_info_get() in i3c_common.c the controller reads the target's MRL and MWL and stores them which I believe the application code needs to refer before talking to the target. So we need ability to configure MRL and MWL during target initialization. Based on these values that the controller reads (through GETMRL, GETMWL) it may choose to modify them (using SETMRL, SETMWL). Similar for HDR capability which needs to be configured for the target (based on target's capability) and which the controller discovers through GETCAPS command.

I missed the i3c_configure API; so we can use that for additional configurations (after i3c_target_register). But then it has to be initiated by the application. Do we need to consider moving some of these target configurations (PID, MRL, MWL, HDR modes) for the secondary controller to the device tree?

No, those should be defined with software at the application level with it's call to i3c_configure..... but I do not know much about your core you are working with (is it possible for to share unless if it's under NDA?). You could have some of the more 'hardware' capabilities (such as if an HDR mode is supported) in the DTS and then written in to it's configuration at the init function if it can not be determined at runtime.

Regarding the read/write callbacks - byte by byte vs buffer pointer and datalen -->

The buffer pointer model is followed in smbus driver for smbus BLOCK READ and BLOCK WRITE functions - smbus_block_write, smbus_block_read. I am thinking similar model is required for i3c. In i3c there is no clock stretching on the bus. At high speeds, we don't want to ACK/NACK the next byte based on application's return value in ISR. We don't want to keep using the i2c type target callback functions for i3c. For I3C write, my thinking is that the hardware should be always ready to receive all the bytes based on MWL. Similarly for I3C read, it should be ready to transmit the data expected by the controller. This is especially valid in MCTP over I3C where the target (MCTP slave) prepares MCTP packet in the transmit buffer before initiating IBI - see MCTP over I3C binding spec. This means that we need a target API which application can use to prepare for transmission before it calls the raise IBI API.

There is no ACK/NACK in i3c when data is being read. It's nEOD/EOD. Is the MWL and and MRL not handled in hardware for you, and you have to receive those values at the software level after the SETMRL/SETMWL is received? The target will send an EOD as soon as it stops writing to the TX_DATA register and there is more data left to transmit, or when it hits the counter for the MRL value. Are you concerned at high speeds that the ISR will not get around quick enough to write more data into it's tx_data register? You should be able to just write in a loop the data byte by byte that needs to go to be transmitted.

Those APIs for block-read and write is still while it is in controller mode as an smbus. That would be no different than the already existing i3c_transfer, but that function uses a more complicated buffer.... but still that is sending or reading data api as those smbus apis.

[jvasanth1]

Yes, the core we are working with supports configurations for PID, MRL, MWL and Max Read Speed, Max Write Speed, maximum read turnaround time as well. So I think these fields need to be added to the i3c_config_target structure. One main issue with the configure API is that I guess it is intended to be called by the application after the init API is executed. But we really want to configure these parameters before the secondary controller is initialized and ready to go.

For the target transmit, I see that Zephyr has the target_tx_write API which can be used to pass a buffer with intended bytes to transmit. So the target application can call this API to queue the transmit before raising IBI and this can work with MCTP type applications. If there is not enough space in the hardware FIFO, then the application can continue transmitting through the callback byte by byte. Can't we have similar model for the receive as well, where the target application passes the buffer which will be filled first before notifying the application of additional data through byte by byte?

The byte by byte model is workable. It puts the responsibility of buffer management to the application. But if we want to use DMA to do i3c transfers which is desirable at I3C speeds then this model will fall short.

[dcpleung]

FYI, the I3C target API comes from I2C target API, and I am sure there are lots of holes to be filled. When it was introduced, there were no driver. @jvasanth1, since you are writing the driver and actually have some use cases, could you amend the API so we can all benefit from it? :)

[XenuIsWatching]

Yes, the core we are working with supports configurations for PID, MRL, MWL and Max Read Speed, Max Write Speed, maximum read turnaround time as well. So I think these fields need to be added to the i3c_config_target structure. One main issue with the configure API is that I guess it is intended to be called by the application after the init API is executed. But we really want to configure these parameters before the secondary controller is initialized and ready to go.

I think that's find to add those to i3c_config_target but most of those are already there. ( it looks like the only one missing is max read turnaround).
If you really need it configured by the init. Then maybe devicetree props would be best??? (I may be wrong, but it's up to you on how to solve that)

For the target transmit, I see that Zephyr has the target_tx_write API which can be used to pass a buffer with intended bytes to transmit. So the target application can call this API to queue the transmit before raising IBI and this can work with MCTP type applications. If there is not enough space in the hardware FIFO, then the application can continue transmitting through the callback byte by byte. Can't we have similar model for the receive as well, where the target application passes the buffer which will be filled first before notifying the application of additional data through byte by byte?

As @dcpleung said, this was modified after the existing I2C target API (and even then.... that API is rather experimental and may have holes). It's just that I3C is a little different from I2C where the target can control when to stop sending data with "Abort" packets and some IPs will not even ACK the transmision so the byte by byte call backs can start so a little 'kickoff' api was created.

The byte by byte model is workable. It puts the responsibility of buffer management to the application. But if we want to use DMA to do i3c transfers which is desirable at I3C speeds then this model will fall short.

I understand your use case there. If you can come up with an API for that then that may work.

[jvasanth1]

Okay, we will review and propose incremental changes.