Message-ID: <Z6OKvl7DOFrXcB1A@lizhi-Precision-Tower-5810>
Date: Wed, 5 Feb 2025 10:58:54 -0500
From: Frank Li <Frank.li@....com>
To: Joshua Yeong <joshua.yeong@...rfivetech.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 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'?

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

Powered by blists - more mailing lists