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

From: Frank Li
Date: Tue Feb 04 2025 - 10:40:25 EST

Next message: Lucas De Marchi: "Re: [PATCH-resent-to-correct-ml 5/8] drm/xe/coredump: Use guard helpers for xe_force_wake."
Previous message: Dmitry Baryshkov: "Re: [PATCH v2] drm/bridge: it6505: support hdmi_codec_ops for audio stream setup"
In reply to: Joshua Yeong: "Re: [PATCH 1/2] i3c: Add basic HDR support"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

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

Next message: Lucas De Marchi: "Re: [PATCH-resent-to-correct-ml 5/8] drm/xe/coredump: Use guard helpers for xe_force_wake."
Previous message: Dmitry Baryshkov: "Re: [PATCH v2] drm/bridge: it6505: support hdmi_codec_ops for audio stream setup"
In reply to: Joshua Yeong: "Re: [PATCH 1/2] i3c: Add basic HDR support"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]