Re: [PATCH 1/2] i3c: Add basic HDR support

From: Joshua Yeong
Date: Tue Feb 04 2025 - 04:51:12 EST

Next message: Maciej Fijalkowski: "Re: [PATCH bpf-next v7 4/5] igc: Refactor empty packet insertion into a reusable function"
Previous message: Thomas Weißschuh: "Re: [PATCH v2 1/2] i3c: Remove the const qualifier from i2c_msg pointer in i2c_xfers API"
In reply to: Mukesh Kumar Savaliya: "Re: [PATCH 1/2] i3c: Add basic HDR support"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

Hi Frank,

On 03-Feb-2025 11:52 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.
>
>>
>> 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?

I guess mainly the terminology. The specification differentiate HDR from private transfer.
We can have the hdr interface to have length indicating for u16 rather than u8 in private trasnfer.
I think the framework should check if device whether it supports HDR command before sending it.
I have code here that do some handling on that.

>
>>
>>>> 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

Yes, I think it is better if we split xfer transfer interface for SDR and HDR.
The interface should assume respective driver to decide how to handle ENTHDR CCC + Exit Pattern.
Else we will need additional 2 interface for start and stop for HDR.

Joshua

>>>> {
>>>>       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,
>>>>
>>>
>>>
>>

Next message: Maciej Fijalkowski: "Re: [PATCH bpf-next v7 4/5] igc: Refactor empty packet insertion into a reusable function"
Previous message: Thomas Weißschuh: "Re: [PATCH v2 1/2] i3c: Remove the const qualifier from i2c_msg pointer in i2c_xfers API"
In reply to: Mukesh Kumar Savaliya: "Re: [PATCH 1/2] i3c: Add basic HDR support"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]