Message-ID: <5db8b40b-b658-4963-9f7f-bfddeb406355@starfivetech.com>
Date: Sun, 9 Feb 2025 22:26:20 +0000
From: Joshua Yeong <joshua.yeong@...rfivetech.com>
To: Frank Li <Frank.li@....com>
CC: Mukesh Kumar Savaliya <quic_msavaliy@...cinc.com>, Alexandre Belloni
	<alexandre.belloni@...tlin.com>, Miquel Raynal <miquel.raynal@...tlin.com>,
	Conor Culhane <conor.culhane@...vaco.com>, "linux-i3c@...ts.infradead.org"
	<linux-i3c@...ts.infradead.org>, "linux-kernel@...r.kernel.org"
	<linux-kernel@...r.kernel.org>, "imx@...ts.linux.dev" <imx@...ts.linux.dev>
Subject: Re: [PATCH 1/2] i3c: Add basic HDR support

On 06-Feb-2025 11:55 PM, Frank Li wrote:
> On Thu, Feb 06, 2025 at 07:05:03AM +0000, Joshua Yeong wrote:
>> 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@....com>
>>>>>>>>>> ---
>>>>>>>>>> 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`
> 
> What's you perfer 'struct hdr_xfer'? I read spec, according to line 8703,
> Just reduce scl clock number when transfer data, which should be transparent
> to software.
> 
> Frank
> 

Hi Frank,

We can have `hdr_xfer` with u8 of 'command code' instead of 'rnw' and additional of
HDR type. The ML handling can handle in the future instead. 

Thanks
Joshua
	

>>
>> 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@...ts.infradead.org
>>>> http://lists.infradead.org/mailman/listinfo/linux-i3c
>>
>> --
>> linux-i3c mailing list
>> linux-i3c@...ts.infradead.org
>> http://lists.infradead.org/mailman/listinfo/linux-i3c

Powered by blists - more mailing lists