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

From: Joshua Yeong
Date: Thu Feb 06 2025 - 03:38:56 EST

Next message: Rob Herring (Arm): "Re: [PATCH v4 10/21] dt-bindings: riscv: add Counter delegation ISA extensions description"
Previous message: WangYuli: "Re: Re: [PATCH 6.1&6.6 0/3] kbuild: Avoid weak external linkage where possible"
In reply to: Frank Li: "Re: [PATCH 1/2] i3c: Add basic HDR support"
Next in thread: Frank Li: "Re: [PATCH 1/2] i3c: Add basic HDR support"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

On 05-Feb-2025 11:58 PM, Frank Li wrote:
> On Wed, Feb 05, 2025 at 06:30:49AM +0000, Joshua Yeong wrote:
>> On 04-Feb-2025 11:38 PM, Frank Li wrote:
>>> On Tue, Feb 04, 2025 at 03:17:36AM +0000, Joshua Yeong wrote:
>>>> 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.
>>>
>>> It should be equivalence with check length is even.
>>>
>>> The key is how client driver will easy to use HDR if they already have SDR
>>> support.
>>>
>>> suppose client:
>>>
>>> struct i3c_priv_xfer xfer[2] = {{ .rnw = 0, buf=p1, size = 4, addr=0xb },
>>> { .rnw = 1, buf=p2, size = 4, addr=0xb }}
>>>
>>> priv_xfer(..., xfer, 2};
>>>
>>> if support HDR
>>>
>>> if (hdr) {
>>> xfer[0].cmd = 0x80;
>>> xfer[1].cmd = 0x;
>>> priv_xfer(..., xfer, 2, HDR);
>>> } else {
>>> priv_xfer(..., xfer, 2, SDR);
>>> }
>>>
>>> client driver needn't distingiush if buff is u8* or u16*.
>>>
>>
>> I fine with the overall structure `.rnw`, `buf`, `size` and `addr`.
>>
>> I prefer if we could use `hdr_xfer` rather than `priv_xfer` for HDR transfer and we can have
>> distinguish mode for DDR/TSP/TSL/BT. In case we are supporting for ML in the future
>
> what's means of "ML"?
>
>> we dont have to lump the function into `priv_xfer`, and we can go `ml_xfer` instead.
>
> Actually, you suggest, rename 'i3c_device_do_priv_xfers_mode' to
> 'i3c_device_ml_xfer'?
>

So there are 3 types of I3C Protocol as defined in I3C spec v1.1.1.
Single Data Rate (SDR), High Data Rate (HDR) and Multi-lane (ML).
I suggest to kept the existing priv_xfers for SDR transfer only.
The MIPI I3C specification only uses private transfer for SDR.

I assume not all I3C IP vendor support HDR mode, but they should at least support SDR mode.
We should have another set of API that uses `i3c_device_do_hdr_xfers`.
`priv_xfer` should represent private transfer and the keyword private keyword
here are unique keyword itself should not mix up with HDR.
Unless the I3C specs stated otherwise I dont think we should mixed up here.

Also interface for sdr/hdr should resolve the other ongoing discussion thread
where mixing both sdr and hdr in `priv_xfer`

Joshua

>>
>> The issue with length indicating both u8 and u16 here is that we would get confuse how the
>> buffer length is interpreted, when length is 1 does it mean 2 bytes or 1 byte? Splitting HDR
>> interface will ensure that length always indicate u16.
>
> All data transfers should be defined in terms of bytes or block units. Note
> that QSPI DDR mode faces a similar constraint, supporting only even-length
> transfers. In most systems, alignment requirements are used to indicate
> size constraints:
>
> SDR mode: Alignment is 1 byte.
> HDR mode: Alignment is 2 bytes.
>
> When using DMA without a bounce buffer, the alignment must match the cache
> line size. Changing to a u16 type does not provide enough flexibility.
> Instead, I propose using a void* (or u8*) for the buffer and a byte count
> for the size. This approach allows for more extensible alignment
> requirements and better compatibility with other transfer systems, such as
> SPI and PCIe.
>
> Frank
>>
>> Joshua
>>
>>
>>>> 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.
>>>
>>> Yes, I can add such check. The key point is API design.
>>>
>>> Frank
>>>>
>>>>>
>>>>>>
>>>>>>>> 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,
>>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>
>>>>
>>
>> --
>> linux-i3c mailing list
>> linux-i3c@xxxxxxxxxxxxxxxxxxx
>> http://lists.infradead.org/mailman/listinfo/linux-i3c

Next message: Rob Herring (Arm): "Re: [PATCH v4 10/21] dt-bindings: riscv: add Counter delegation ISA extensions description"
Previous message: WangYuli: "Re: Re: [PATCH 6.1&6.6 0/3] kbuild: Avoid weak external linkage where possible"
In reply to: Frank Li: "Re: [PATCH 1/2] i3c: Add basic HDR support"
Next in thread: Frank Li: "Re: [PATCH 1/2] i3c: Add basic HDR support"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]