| Message ID | 20250129-i3c_ddr-v1-1-028a7a5d4324@nxp.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | i3c: Add basic HDR mode support | expand |
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@nxp.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. > 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. > { > 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, >
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@nxp.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 think the 'private transfers' are limited to SDR communications, would it be better if we have a different interface/naming for hdr transfers? >> 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. >> { >> 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, >> > >
Hi Joshua, On 2/3/2025 8:02 AM, 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@nxp.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 think the 'private transfers' are limited to SDR communications, > would it be better if we have a different interface/naming for hdr transfers? > >>> 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. Yes, that would be fine too as we can clearly differentiate by inteface/hook up name. >>> { >>> 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, >>> >> >> >
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@nxp.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? > > >> 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 > >> { > >> 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, > >> > > > > >
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@nxp.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. 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, >>>> >>> >>> >>
On 2/3/2025 9:22 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@nxp.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. > let me clarify, We can have main entry/hookup function as it is. From priv_xfer , we can call sdr_priv_xfer and hdr_priv_xfer ? based on the structure information if its enum SDR or enum HDR. >> >> 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? > yes, but can we make it priv_xfer() = sdr_priv_xfer() + hdr_priv_xfer() ? >> >>>> 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 >>>> { >>>> 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, >>>> >>> >>> >>
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@nxp.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 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, > >>>> > >>> > >>> > >> >
On Tue, Feb 04, 2025 at 11:24:03AM +0530, Mukesh Kumar Savaliya wrote: > > > On 2/3/2025 9:22 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@nxp.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. > > > let me clarify, We can have main entry/hookup function as it is. > From priv_xfer , we can call sdr_priv_xfer and hdr_priv_xfer ? based on the > structure information if its enum SDR or enum HDR. > > > > > > > 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? > > > yes, but can we make it priv_xfer() = sdr_priv_xfer() + hdr_priv_xfer() ? struct i3c_priv_xfer is array, each i3c_priv_xfer item is indepent. Can you write a simple code to demostrate your idea. 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 > > > > > { > > > > > 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, > > > > > > > > > > > > > > > > >
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@nxp.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 we dont have to lump the function into `priv_xfer`, and we can go `ml_xfer` instead. 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. 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, >>>>>> >>>>> >>>>> >>>> >>
Thanks Frank ! please review below. On 2/4/2025 9:13 PM, Frank Li wrote: > On Tue, Feb 04, 2025 at 11:24:03AM +0530, Mukesh Kumar Savaliya wrote: >> >> >> On 2/3/2025 9:22 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@nxp.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. >>> >> let me clarify, We can have main entry/hookup function as it is. >> From priv_xfer , we can call sdr_priv_xfer and hdr_priv_xfer ? based on the >> structure information if its enum SDR or enum HDR. >> >>>> >>>> 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? >>> >> yes, but can we make it priv_xfer() = sdr_priv_xfer() + hdr_priv_xfer() ? > > struct i3c_priv_xfer is array, each i3c_priv_xfer item is indepent. Can you > write a simple code to demostrate your idea. > > Frank ====== Please see below. My only intention is to add hdr/sdr decision inside i3c_priv_xfer and rest of the functions should remain as it is. If you still need, may be we can add local function but no need to change prototype of function being explosed to many vendors. I hope i am not missing any major thing. struct i3c_priv_xfer { u8 rnw; u16 len; union { void *in; const void *out; } data; enum i3c_error_code err; // Just Add below two members and rest can be passed till end point/function and can process as required. enum i3c_hdr_mode mode; union { u8 rnw; u8 cmd; }; }; device.c : i3c_device_do_priv_xfers => i3c_dev_do_priv_xfers_locked int i3c_device_do_priv_xfers(struct i3c_device *dev, struct i3c_priv_xfer *xfers, int nxfers) { ret = i3c_dev_do_priv_xfers_locked(dev->desc, xfers, xfers); } master.c : int i3c_dev_do_priv_xfers_locked(struct i3c_dev_desc *dev, struct i3c_priv_xfer *xfers, int nxfers) { if (master->ops->priv_xfers_mode) return master->ops->priv_xfers_mode(dev, xfers, nxfers); if (xfers->mode != I3C_SDR) return -EINVAL; } svc-i3c-master.c //process everything using xfers.mode and xfer.cmd. I hope you can add all rest of the changes inside this function itself. static int svc_i3c_master_priv_xfers(struct i3c_dev_desc *dev, struct i3c_priv_xfer *xfers, int nxfers) ========== > >>>> >>>>>> 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 >>>>>> { >>>>>> 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, >>>>>> >>>>> >>>>> >>>> >>
On Wed, Feb 05, 2025 at 07:49:20PM +0530, Mukesh Kumar Savaliya wrote: > Thanks Frank ! please review below. > > On 2/4/2025 9:13 PM, Frank Li wrote: > > On Tue, Feb 04, 2025 at 11:24:03AM +0530, Mukesh Kumar Savaliya wrote: > > > > > > > > > On 2/3/2025 9:22 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@nxp.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. > > > > > > > let me clarify, We can have main entry/hookup function as it is. > > > From priv_xfer , we can call sdr_priv_xfer and hdr_priv_xfer ? based on the > > > structure information if its enum SDR or enum HDR. > > > > > > > > > > > > > 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? > > > > > > > yes, but can we make it priv_xfer() = sdr_priv_xfer() + hdr_priv_xfer() ? > > > > struct i3c_priv_xfer is array, each i3c_priv_xfer item is indepent. Can you > > write a simple code to demostrate your idea. > > > > Frank > > ====== > Please see below. My only intention is to add hdr/sdr decision inside > i3c_priv_xfer and rest of the functions should remain as it is. > > If you still need, may be we can add local function but no need to change > prototype of function being explosed to many vendors. > > I hope i am not missing any major thing. > > struct i3c_priv_xfer { > u8 rnw; > u16 len; > union { > void *in; > const void *out; > } data; > enum i3c_error_code err; > // Just Add below two members and rest can be passed till end point/function > and can process as required. > enum i3c_hdr_mode mode; > union { > u8 rnw; > u8 cmd; > }; > }; > > > device.c : > i3c_device_do_priv_xfers => i3c_dev_do_priv_xfers_locked > > int i3c_device_do_priv_xfers(struct i3c_device *dev, struct i3c_priv_xfer > *xfers, int nxfers) { > > ret = i3c_dev_do_priv_xfers_locked(dev->desc, xfers, xfers); > > } > > > master.c : > int i3c_dev_do_priv_xfers_locked(struct i3c_dev_desc *dev, struct > i3c_priv_xfer *xfers, int nxfers) { > > if (master->ops->priv_xfers_mode) > return master->ops->priv_xfers_mode(dev, xfers, nxfers); > > if (xfers->mode != I3C_SDR) > return -EINVAL; > } > The problem is struct i3c_priv_xfer xfer[3] = { { .mode = SDR ...}, { .mode = HDR ...}, { .mode = SDR ...}, } i3c_device_do_priv_xfers(dev, xfer, 3); How to handle this case? I think i3c_device_do_priv_xfers() means one whole i3c transcation: START, xfer[0], xfer[1], xfer[2], STOP. Frank > > svc-i3c-master.c > //process everything using xfers.mode and xfer.cmd. > > I hope you can add all rest of the changes inside this function itself. > static int svc_i3c_master_priv_xfers(struct i3c_dev_desc *dev, struct > i3c_priv_xfer *xfers, int nxfers) > ========== > > > > > > > > > > > > > > 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 > > > > > > > { > > > > > > > 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, > > > > > > > > > > > > > > > > > > > > > > > > > > > >
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@nxp.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@lists.infradead.org > http://lists.infradead.org/mailman/listinfo/linux-i3c
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@nxp.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` 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@lists.infradead.org >> http://lists.infradead.org/mailman/listinfo/linux-i3c
Hi Frank, On 2/5/2025 9:14 PM, Frank Li wrote: > On Wed, Feb 05, 2025 at 07:49:20PM +0530, Mukesh Kumar Savaliya wrote: >> Thanks Frank ! please review below. >> >> On 2/4/2025 9:13 PM, Frank Li wrote: >>> On Tue, Feb 04, 2025 at 11:24:03AM +0530, Mukesh Kumar Savaliya wrote: >>>> >>>> >>>> On 2/3/2025 9:22 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@nxp.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. >>>>> >>>> let me clarify, We can have main entry/hookup function as it is. >>>> From priv_xfer , we can call sdr_priv_xfer and hdr_priv_xfer ? based on the >>>> structure information if its enum SDR or enum HDR. >>>> >>>>>> >>>>>> 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? >>>>> >>>> yes, but can we make it priv_xfer() = sdr_priv_xfer() + hdr_priv_xfer() ? >>> >>> struct i3c_priv_xfer is array, each i3c_priv_xfer item is indepent. Can you >>> write a simple code to demostrate your idea. >>> >>> Frank >> >> ====== >> Please see below. My only intention is to add hdr/sdr decision inside >> i3c_priv_xfer and rest of the functions should remain as it is. >> >> If you still need, may be we can add local function but no need to change >> prototype of function being explosed to many vendors. >> >> I hope i am not missing any major thing. >> >> struct i3c_priv_xfer { >> u8 rnw; >> u16 len; >> union { >> void *in; >> const void *out; >> } data; >> enum i3c_error_code err; >> // Just Add below two members and rest can be passed till end point/function >> and can process as required. >> enum i3c_hdr_mode mode; >> union { >> u8 rnw; >> u8 cmd; >> }; >> }; >> >> >> device.c : >> i3c_device_do_priv_xfers => i3c_dev_do_priv_xfers_locked >> >> int i3c_device_do_priv_xfers(struct i3c_device *dev, struct i3c_priv_xfer >> *xfers, int nxfers) { >> >> ret = i3c_dev_do_priv_xfers_locked(dev->desc, xfers, xfers); >> >> } >> >> >> master.c : >> int i3c_dev_do_priv_xfers_locked(struct i3c_dev_desc *dev, struct >> i3c_priv_xfer *xfers, int nxfers) { >> >> if (master->ops->priv_xfers_mode) >> return master->ops->priv_xfers_mode(dev, xfers, nxfers); >> >> if (xfers->mode != I3C_SDR) >> return -EINVAL; >> } >> > > The problem is > > struct i3c_priv_xfer xfer[3] = { > { .mode = SDR ...}, > { .mode = HDR ...}, > { .mode = SDR ...}, > } > > i3c_device_do_priv_xfers(dev, xfer, 3); > > How to handle this case? > > I think i3c_device_do_priv_xfers() means one whole i3c transcation: > START, xfer[0], xfer[1], xfer[2], STOP. > > Frank > I think they are separate transfers, it means one call but multiple buffer to send with separate start/stop. Two options : 1. When multiple transfers in a single call, driver internally itrerates and should take care of start-xfer-stop. I think HW should be taking care unless using the restart intentionally. 2. if any xfer[i] has HDR mode, internally it can call local function OR should diverge based on mode wherever it suits better to do. >> >> svc-i3c-master.c >> //process everything using xfers.mode and xfer.cmd. >> >> I hope you can add all rest of the changes inside this function itself. >> static int svc_i3c_master_priv_xfers(struct i3c_dev_desc *dev, struct >> i3c_priv_xfer *xfers, int nxfers) >> ========== >>> >>>>>> >>>>>>>> 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 >>>>>>>> { >>>>>>>> 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, >>>>>>>> >>>>>>> >>>>>>> >>>>>> >>>> >>
On Thu, Feb 06, 2025 at 03:14:20PM +0530, Mukesh Kumar Savaliya wrote: > Hi Frank, > > On 2/5/2025 9:14 PM, Frank Li wrote: > > On Wed, Feb 05, 2025 at 07:49:20PM +0530, Mukesh Kumar Savaliya wrote: > > > Thanks Frank ! please review below. > > > > > > On 2/4/2025 9:13 PM, Frank Li wrote: > > > > On Tue, Feb 04, 2025 at 11:24:03AM +0530, Mukesh Kumar Savaliya wrote: > > > > > > > > > > > > > > > On 2/3/2025 9:22 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@nxp.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. > > > > > > > > > > > let me clarify, We can have main entry/hookup function as it is. > > > > > From priv_xfer , we can call sdr_priv_xfer and hdr_priv_xfer ? based on the > > > > > structure information if its enum SDR or enum HDR. > > > > > > > > > > > > > > > > > > > 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? > > > > > > > > > > > yes, but can we make it priv_xfer() = sdr_priv_xfer() + hdr_priv_xfer() ? > > > > > > > > struct i3c_priv_xfer is array, each i3c_priv_xfer item is indepent. Can you > > > > write a simple code to demostrate your idea. > > > > > > > > Frank > > > > > > ====== > > > Please see below. My only intention is to add hdr/sdr decision inside > > > i3c_priv_xfer and rest of the functions should remain as it is. > > > > > > If you still need, may be we can add local function but no need to change > > > prototype of function being explosed to many vendors. > > > > > > I hope i am not missing any major thing. > > > > > > struct i3c_priv_xfer { > > > u8 rnw; > > > u16 len; > > > union { > > > void *in; > > > const void *out; > > > } data; > > > enum i3c_error_code err; > > > // Just Add below two members and rest can be passed till end point/function > > > and can process as required. > > > enum i3c_hdr_mode mode; > > > union { > > > u8 rnw; > > > u8 cmd; > > > }; > > > }; > > > > > > > > > device.c : > > > i3c_device_do_priv_xfers => i3c_dev_do_priv_xfers_locked > > > > > > int i3c_device_do_priv_xfers(struct i3c_device *dev, struct i3c_priv_xfer > > > *xfers, int nxfers) { > > > > > > ret = i3c_dev_do_priv_xfers_locked(dev->desc, xfers, xfers); > > > > > > } > > > > > > > > > master.c : > > > int i3c_dev_do_priv_xfers_locked(struct i3c_dev_desc *dev, struct > > > i3c_priv_xfer *xfers, int nxfers) { > > > > > > if (master->ops->priv_xfers_mode) > > > return master->ops->priv_xfers_mode(dev, xfers, nxfers); > > > > > > if (xfers->mode != I3C_SDR) > > > return -EINVAL; > > > } > > > > > > > The problem is > > > > struct i3c_priv_xfer xfer[3] = { > > { .mode = SDR ...}, > > { .mode = HDR ...}, > > { .mode = SDR ...}, > > } > > > > i3c_device_do_priv_xfers(dev, xfer, 3); > > > > How to handle this case? > > > > I think i3c_device_do_priv_xfers() means one whole i3c transcation: > > START, xfer[0], xfer[1], xfer[2], STOP. > > > > Frank > > > I think they are separate transfers, it means one call but multiple buffer > to send with separate start/stop. > > Two options : > 1. When multiple transfers in a single call, driver internally itrerates and > should take care of start-xfer-stop. I think HW should be taking care unless > using the restart intentionally. > > 2. if any xfer[i] has HDR mode, internally it can call local function OR > should diverge based on mode wherever it suits better to do. We should define such behavior at API to avoid cleint driver guess master controller driver's implemtation. It think xfer[n] by one API call, should be one whole transfer. START xfer[0], REPEAT START, xfer[1], REPEAT START xfer[2], ... STOP. If xfer[0]'s buffer is uncontinue, it should be use sg list. But i3c data len is related small now, so one buf point should be enough now. If client want to multi START...STOP, START...STOP, it should be call API mulit times. One API call only support one START...STOP. REPEAT START != START. About HDR, according to spec line 5522 START SDR_CMD_ENTER_DDR xfer[0], DDR_RESTART xfer[1], ..., DDR_EXIT. So seperate/extended API is needed to keep API behavior simple and consistent. Frank > > > > > > > svc-i3c-master.c > > > //process everything using xfers.mode and xfer.cmd. > > > > > > I hope you can add all rest of the changes inside this function itself. > > > static int svc_i3c_master_priv_xfers(struct i3c_dev_desc *dev, struct > > > i3c_priv_xfer *xfers, int nxfers) > > > ========== > > > > > > > > > > > > > > > > > > > > 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 > > > > > > > > > { > > > > > > > > > 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, > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > >
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@nxp.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 > > 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@lists.infradead.org > >> http://lists.infradead.org/mailman/listinfo/linux-i3c > > -- > linux-i3c mailing list > linux-i3c@lists.infradead.org > http://lists.infradead.org/mailman/listinfo/linux-i3c
Thanks Frank for reverting with clarity and details. On 2/6/2025 9:02 PM, Frank Li wrote: > On Thu, Feb 06, 2025 at 03:14:20PM +0530, Mukesh Kumar Savaliya wrote: >> Hi Frank, >> >> On 2/5/2025 9:14 PM, Frank Li wrote: >>> On Wed, Feb 05, 2025 at 07:49:20PM +0530, Mukesh Kumar Savaliya wrote: >>>> Thanks Frank ! please review below. >>>> >>>> On 2/4/2025 9:13 PM, Frank Li wrote: >>>>> On Tue, Feb 04, 2025 at 11:24:03AM +0530, Mukesh Kumar Savaliya wrote: >>>>>> >>>>>> >>>>>> On 2/3/2025 9:22 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@nxp.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. >>>>>>> >>>>>> let me clarify, We can have main entry/hookup function as it is. >>>>>> From priv_xfer , we can call sdr_priv_xfer and hdr_priv_xfer ? based on the >>>>>> structure information if its enum SDR or enum HDR. >>>>>> >>>>>>>> >>>>>>>> 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? >>>>>>> >>>>>> yes, but can we make it priv_xfer() = sdr_priv_xfer() + hdr_priv_xfer() ? >>>>> >>>>> struct i3c_priv_xfer is array, each i3c_priv_xfer item is indepent. Can you >>>>> write a simple code to demostrate your idea. >>>>> >>>>> Frank >>>> >>>> ====== >>>> Please see below. My only intention is to add hdr/sdr decision inside >>>> i3c_priv_xfer and rest of the functions should remain as it is. >>>> >>>> If you still need, may be we can add local function but no need to change >>>> prototype of function being explosed to many vendors. >>>> >>>> I hope i am not missing any major thing. >>>> >>>> struct i3c_priv_xfer { >>>> u8 rnw; >>>> u16 len; >>>> union { >>>> void *in; >>>> const void *out; >>>> } data; >>>> enum i3c_error_code err; >>>> // Just Add below two members and rest can be passed till end point/function >>>> and can process as required. >>>> enum i3c_hdr_mode mode; >>>> union { >>>> u8 rnw; >>>> u8 cmd; >>>> }; >>>> }; >>>> >>>> >>>> device.c : >>>> i3c_device_do_priv_xfers => i3c_dev_do_priv_xfers_locked >>>> >>>> int i3c_device_do_priv_xfers(struct i3c_device *dev, struct i3c_priv_xfer >>>> *xfers, int nxfers) { >>>> >>>> ret = i3c_dev_do_priv_xfers_locked(dev->desc, xfers, xfers); >>>> >>>> } >>>> >>>> >>>> master.c : >>>> int i3c_dev_do_priv_xfers_locked(struct i3c_dev_desc *dev, struct >>>> i3c_priv_xfer *xfers, int nxfers) { >>>> >>>> if (master->ops->priv_xfers_mode) >>>> return master->ops->priv_xfers_mode(dev, xfers, nxfers); >>>> >>>> if (xfers->mode != I3C_SDR) >>>> return -EINVAL; >>>> } >>>> >>> >>> The problem is >>> >>> struct i3c_priv_xfer xfer[3] = { >>> { .mode = SDR ...}, >>> { .mode = HDR ...}, >>> { .mode = SDR ...}, >>> } >>> >>> i3c_device_do_priv_xfers(dev, xfer, 3); >>> >>> How to handle this case? >>> >>> I think i3c_device_do_priv_xfers() means one whole i3c transcation: >>> START, xfer[0], xfer[1], xfer[2], STOP. >>> >>> Frank >>> >> I think they are separate transfers, it means one call but multiple buffer >> to send with separate start/stop. >> >> Two options : >> 1. When multiple transfers in a single call, driver internally itrerates and >> should take care of start-xfer-stop. I think HW should be taking care unless >> using the restart intentionally. >> >> 2. if any xfer[i] has HDR mode, internally it can call local function OR >> should diverge based on mode wherever it suits better to do. > > We should define such behavior at API to avoid cleint driver guess master > controller driver's implemtation. > What's the criteria to be used to decide at mater controller driver ? I think target device and it's client should also be supporting and hence should instruct master driver. May be i can keep this open for suggestions from other experts. > It think xfer[n] by one API call, should be one whole transfer. > I agree. with this if no other option, then do we still provide different mode of transfers in a single function call ? if not then can think of separate transfer passing the mode, the way you do right now. > START xfer[0], REPEAT START, xfer[1], REPEAT START xfer[2], ... STOP. > > If xfer[0]'s buffer is uncontinue, it should be use sg list. But i3c data > len is related small now, so one buf point should be enough now. > > If client want to multi START...STOP, START...STOP, it should be call API > mulit times. One API call only support one START...STOP. > > REPEAT START != START. > > About HDR, according to spec line 5522 > > START SDR_CMD_ENTER_DDR xfer[0], DDR_RESTART xfer[1], ..., DDR_EXIT. > > So seperate/extended API is needed to keep API behavior simple and > consistent. > sure. if we can't enforce multimode transfers in a single call. > Frank > >> >>>> >>>> svc-i3c-master.c >>>> //process everything using xfers.mode and xfer.cmd. >>>> >>>> I hope you can add all rest of the changes inside this function itself. >>>> static int svc_i3c_master_priv_xfers(struct i3c_dev_desc *dev, struct >>>> i3c_priv_xfer *xfers, int nxfers) >>>> ========== >>>>> >>>>>>>> >>>>>>>>>> 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 >>>>>>>>>> { >>>>>>>>>> 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, >>>>>>>>>> >>>>>>>>> >>>>>>>>> >>>>>>>> >>>>>> >>>> >>
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@nxp.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@lists.infradead.org >>>> http://lists.infradead.org/mailman/listinfo/linux-i3c >> >> -- >> linux-i3c mailing list >> linux-i3c@lists.infradead.org >> http://lists.infradead.org/mailman/listinfo/linux-i3c
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) { 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,
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@nxp.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. 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(-)