Re: [PATCH 1/2] i3c: Add basic HDR support
From: Mukesh Kumar Savaliya
Date: Fri Feb 07 2025 - 04:08:52 EST
Thanks Frank for reverting with clarity and details.
On 2/6/2025 9:02 PM, Frank Li wrote:
On Thu, Feb 06, 2025 at 03:14:20PM +0530, Mukesh Kumar Savaliya wrote:
Hi Frank,
On 2/5/2025 9:14 PM, Frank Li wrote:
On Wed, Feb 05, 2025 at 07:49:20PM +0530, Mukesh Kumar Savaliya wrote:
Thanks Frank ! please review below.
On 2/4/2025 9:13 PM, Frank Li wrote:
On Tue, Feb 04, 2025 at 11:24:03AM +0530, Mukesh Kumar Savaliya wrote:
On 2/3/2025 9:22 PM, Frank Li wrote:
On Mon, Feb 03, 2025 at 02:32:51AM +0000, Joshua Yeong wrote:
Hi Mukesh and Frank,
On 02-Feb-2025 1:54 AM, Mukesh Kumar Savaliya wrote:
Hi Frank,
On 1/30/2025 1:35 AM, Frank Li wrote:
I3C HDR requires enter/exit patterns during each I3C transfer. Add a new
API i3c_device_do_priv_xfers_mode(). The existing
i3c_device_do_priv_xfers() now calls i3c_device_do_priv_xfers_mode(I3C_SDR)
to maintain backward compatibility.
Introduce a 'cmd' field in 'struct i3c_priv_xfer', using an anonymous union
with 'rnw' since HDR mode relies on read/write commands instead of the
'rnw' bit in the address as in SDR mode.
Add a priv_xfers_mode callback for I3C master drivers. If priv_xfers_mode
is not implemented, fallback to SDR mode using the existing priv_xfers
callback.
Signed-off-by: Frank Li <Frank.Li@xxxxxxx>
---
Why not add hdr mode in struct i3c_priv_xfer because mode can't be mixed in
one i3c transfer. for example, can't send a HDR follow one SDR between
START and STOP.
Is it wise to add two function inside main funcion and separate SDR vs HDR ? so we don't need to change current function arguments.
I am not sure if understand your means. HDR have difference mode, anyway
need add new argument.
let me clarify, We can have main entry/hookup function as it is.
From priv_xfer , we can call sdr_priv_xfer and hdr_priv_xfer ? based on the
structure information if its enum SDR or enum HDR.
I think the 'private transfers' are limited to SDR communications,
would it be better if we have a different interface/naming for hdr transfers?
The key is what's difference between hdr and private transfers. So far only
command vs rnw, any other differences I missed?
yes, but can we make it priv_xfer() = sdr_priv_xfer() + hdr_priv_xfer() ?
struct i3c_priv_xfer is array, each i3c_priv_xfer item is indepent. Can you
write a simple code to demostrate your idea.
Frank
======
Please see below. My only intention is to add hdr/sdr decision inside
i3c_priv_xfer and rest of the functions should remain as it is.
If you still need, may be we can add local function but no need to change
prototype of function being explosed to many vendors.
I hope i am not missing any major thing.
struct i3c_priv_xfer {
u8 rnw;
u16 len;
union {
void *in;
const void *out;
} data;
enum i3c_error_code err;
// Just Add below two members and rest can be passed till end point/function
and can process as required.
enum i3c_hdr_mode mode;
union {
u8 rnw;
u8 cmd;
};
};
device.c :
i3c_device_do_priv_xfers => i3c_dev_do_priv_xfers_locked
int i3c_device_do_priv_xfers(struct i3c_device *dev, struct i3c_priv_xfer
*xfers, int nxfers) {
ret = i3c_dev_do_priv_xfers_locked(dev->desc, xfers, xfers);
}
master.c :
int i3c_dev_do_priv_xfers_locked(struct i3c_dev_desc *dev, struct
i3c_priv_xfer *xfers, int nxfers) {
if (master->ops->priv_xfers_mode)
return master->ops->priv_xfers_mode(dev, xfers, nxfers);
if (xfers->mode != I3C_SDR)
return -EINVAL;
}
The problem is
struct i3c_priv_xfer xfer[3] = {
{ .mode = SDR ...},
{ .mode = HDR ...},
{ .mode = SDR ...},
}
i3c_device_do_priv_xfers(dev, xfer, 3);
How to handle this case?
I think i3c_device_do_priv_xfers() means one whole i3c transcation:
START, xfer[0], xfer[1], xfer[2], STOP.
Frank
I think they are separate transfers, it means one call but multiple buffer
to send with separate start/stop.
Two options :
1. When multiple transfers in a single call, driver internally itrerates and
should take care of start-xfer-stop. I think HW should be taking care unless
using the restart intentionally.
2. if any xfer[i] has HDR mode, internally it can call local function OR
should diverge based on mode wherever it suits better to do.
We should define such behavior at API to avoid cleint driver guess master
controller driver's implemtation.
What's the criteria to be used to decide at mater controller driver ?
I think target device and it's client should also be supporting and
hence should instruct master driver.
May be i can keep this open for suggestions from other experts.
It think xfer[n] by one API call, should be one whole transfer.
I agree. with this if no other option, then do we still provide
different mode of transfers in a single function call ?
if not then can think of separate transfer passing the mode, the way you
do right now.
START xfer[0], REPEAT START, xfer[1], REPEAT START xfer[2], ... STOP.
If xfer[0]'s buffer is uncontinue, it should be use sg list. But i3c data
len is related small now, so one buf point should be enough now.
If client want to multi START...STOP, START...STOP, it should be call API
mulit times. One API call only support one START...STOP.
REPEAT START != START.
About HDR, according to spec line 5522
START SDR_CMD_ENTER_DDR xfer[0], DDR_RESTART xfer[1], ..., DDR_EXIT.
So seperate/extended API is needed to keep API behavior simple and
consistent.
sure. if we can't enforce multimode transfers in a single call.
Frank
svc-i3c-master.c
//process everything using xfers.mode and xfer.cmd.
I hope you can add all rest of the changes inside this function itself.
static int svc_i3c_master_priv_xfers(struct i3c_dev_desc *dev, struct
i3c_priv_xfer *xfers, int nxfers)
==========
i3c_priv_xfer should be treat as whole i3c transactions. If user want send
HDR follow SDR, should be call i3c_device_do_priv_xfers_mode() twice,
instead put into a big i3c_priv_xfer[n].
---
drivers/i3c/device.c | 19 +++++++++++++------
drivers/i3c/internals.h | 2 +-
drivers/i3c/master.c | 8 +++++++-
include/linux/i3c/device.h | 12 +++++++++++-
include/linux/i3c/master.h | 3 +++
5 files changed, 35 insertions(+), 9 deletions(-)
diff --git a/drivers/i3c/device.c b/drivers/i3c/device.c
index e80e487569146..e3db3a6a9e4f6 100644
--- a/drivers/i3c/device.c
+++ b/drivers/i3c/device.c
@@ -15,12 +15,13 @@
#include "internals.h"
/**
- * i3c_device_do_priv_xfers() - do I3C SDR private transfers directed to a
- * specific device
+ * i3c_device_do_priv_xfers_mode() - do I3C SDR private transfers directed to a
+ * specific device
*
* @dev: device with which the transfers should be done
* @xfers: array of transfers
* @nxfers: number of transfers
+ * @mode: transfer mode
*
* Initiate one or several private SDR transfers with @dev.
*
@@ -32,9 +33,9 @@
* driver needs to resend the 'xfers' some time later.
* See I3C spec ver 1.1.1 09-Jun-2021. Section: 5.1.2.2.3.
*/
-int i3c_device_do_priv_xfers(struct i3c_device *dev,
- struct i3c_priv_xfer *xfers,
- int nxfers)
+int i3c_device_do_priv_xfers_mode(struct i3c_device *dev,
+ struct i3c_priv_xfer *xfers,
+ int nxfers, enum i3c_hdr_mode mode)
why can't we pass mode as another structure member inside struct i3c_priv_xfer ? Adding function agrument parameter impacts existing drivers.
If that, it will be possible, xfers[0] is sdr, xfers[1] is hdr. Actually,
I3C can't do that. we assume finish whole xfers between one whole traction
(between START and STOP).
Frank
{
int ret, i;
@@ -47,11 +48,17 @@ int i3c_device_do_priv_xfers(struct i3c_device *dev,
}
i3c_bus_normaluse_lock(dev->bus);
- ret = i3c_dev_do_priv_xfers_locked(dev->desc, xfers, nxfers);
+ ret = i3c_dev_do_priv_xfers_locked(dev->desc, xfers, nxfers, mode);
i3c_bus_normaluse_unlock(dev->bus);
return ret;
}
+EXPORT_SYMBOL_GPL(i3c_device_do_priv_xfers_mode);
+
+int i3c_device_do_priv_xfers(struct i3c_device *dev, struct i3c_priv_xfer *xfers, int nxfers)
+{
+ return i3c_device_do_priv_xfers_mode(dev, xfers, nxfers, I3C_SDR);
+}
EXPORT_SYMBOL_GPL(i3c_device_do_priv_xfers);
/**
diff --git a/drivers/i3c/internals.h b/drivers/i3c/internals.h
index 433f6088b7cec..553edc9846ac0 100644
--- a/drivers/i3c/internals.h
+++ b/drivers/i3c/internals.h
@@ -16,7 +16,7 @@ void i3c_bus_normaluse_unlock(struct i3c_bus *bus);
int i3c_dev_setdasa_locked(struct i3c_dev_desc *dev);
int i3c_dev_do_priv_xfers_locked(struct i3c_dev_desc *dev,
struct i3c_priv_xfer *xfers,
- int nxfers);
+ int nxfers, enum i3c_hdr_mode mode);
int i3c_dev_disable_ibi_locked(struct i3c_dev_desc *dev);
int i3c_dev_enable_ibi_locked(struct i3c_dev_desc *dev);
int i3c_dev_request_ibi_locked(struct i3c_dev_desc *dev,
diff --git a/drivers/i3c/master.c b/drivers/i3c/master.c
index d5dc4180afbcf..67aaba0a38db2 100644
--- a/drivers/i3c/master.c
+++ b/drivers/i3c/master.c
@@ -2945,7 +2945,7 @@ int i3c_dev_setdasa_locked(struct i3c_dev_desc *dev)
int i3c_dev_do_priv_xfers_locked(struct i3c_dev_desc *dev,
struct i3c_priv_xfer *xfers,
- int nxfers)
+ int nxfers, enum i3c_hdr_mode mode)
{
struct i3c_master_controller *master;
@@ -2956,9 +2956,15 @@ int i3c_dev_do_priv_xfers_locked(struct i3c_dev_desc *dev,
if (!master || !xfers)
return -EINVAL;
+ if (master->ops->priv_xfers_mode)
+ return master->ops->priv_xfers_mode(dev, xfers, nxfers, mode);
+
if (!master->ops->priv_xfers)
return -ENOTSUPP;
+ if (mode != I3C_SDR)
+ return -EINVAL;
+
return master->ops->priv_xfers(dev, xfers, nxfers);
}
diff --git a/include/linux/i3c/device.h b/include/linux/i3c/device.h
index b674f64d0822e..7ce70d0967e27 100644
--- a/include/linux/i3c/device.h
+++ b/include/linux/i3c/device.h
@@ -40,11 +40,13 @@ enum i3c_error_code {
/**
* enum i3c_hdr_mode - HDR mode ids
+ * @I3C_SDR: SDR mode (NOT HDR mode)
* @I3C_HDR_DDR: DDR mode
* @I3C_HDR_TSP: TSP mode
* @I3C_HDR_TSL: TSL mode
*/
enum i3c_hdr_mode {
+ I3C_SDR,
I3C_HDR_DDR,
I3C_HDR_TSP,
I3C_HDR_TSL,
@@ -53,6 +55,7 @@ enum i3c_hdr_mode {
/**
* struct i3c_priv_xfer - I3C SDR private transfer
* @rnw: encodes the transfer direction. true for a read, false for a write
+ * @cmd: Read/Write command in HDR mode, read: 0x80 - 0xff, write: 0x00 - 0x7f
* @len: transfer length in bytes of the transfer
* @actual_len: actual length in bytes are transferred by the controller
* @data: input/output buffer
@@ -61,7 +64,10 @@ enum i3c_hdr_mode {
* @err: I3C error code
*/
struct i3c_priv_xfer {
- u8 rnw;
+ union {
+ u8 rnw;
+ u8 cmd;
+ };
u16 len;
u16 actual_len;
union {
@@ -301,6 +307,10 @@ int i3c_device_do_priv_xfers(struct i3c_device *dev,
struct i3c_priv_xfer *xfers,
int nxfers);
+int i3c_device_do_priv_xfers_mode(struct i3c_device *dev,
+ struct i3c_priv_xfer *xfers,
+ int nxfers, enum i3c_hdr_mode mode);
+
int i3c_device_do_setdasa(struct i3c_device *dev);
void i3c_device_get_info(const struct i3c_device *dev, struct i3c_device_info *info);
diff --git a/include/linux/i3c/master.h b/include/linux/i3c/master.h
index 12d532b012c5a..352bd41139569 100644
--- a/include/linux/i3c/master.h
+++ b/include/linux/i3c/master.h
@@ -472,6 +472,9 @@ struct i3c_master_controller_ops {
int (*priv_xfers)(struct i3c_dev_desc *dev,
struct i3c_priv_xfer *xfers,
int nxfers);
+ int (*priv_xfers_mode)(struct i3c_dev_desc *dev,
+ struct i3c_priv_xfer *xfers,
+ int nxfers, enum i3c_hdr_mode mode);
int (*attach_i2c_dev)(struct i2c_dev_desc *dev);
void (*detach_i2c_dev)(struct i2c_dev_desc *dev);
int (*i2c_xfers)(struct i2c_dev_desc *dev,