* [PATCH 0/2] i3c: Add basic HDR mode support
@ 2025-01-29 20:05 Frank Li
2025-01-29 20:05 ` [PATCH 1/2] i3c: Add basic HDR support Frank Li
2025-01-29 20:05 ` [PATCH 2/2] i3c: master: svc: Add basic HDR mode support Frank Li
0 siblings, 2 replies; 21+ messages in thread
From: Frank Li @ 2025-01-29 20:05 UTC (permalink / raw)
To: Alexandre Belloni, Miquel Raynal, Conor Culhane
Cc: linux-i3c, linux-kernel, imx, Frank Li
Add basic HDR mode support, only support private transfer, not support
CCC command.
Update i3c framework API to allow pass down mode and extend driver callback
function.
Implement HDR transfer in svc i3c master driver.
Simplifed HDR flow is (ref i3c spec line 5514) Figure 129
<-- SDR ---> | <--- HDR
START 0x7E RnW(0) ACK CCC(ENTHDR0) T HDR-CMD(00-7f write, 80--ff read)
----> |
HDR-DATA HDR-CRC HDR-RESTART .... HDR-EXIT
Note: HDR-CMD is 16bit data, which included 7bit slave address and 8bit
read/write command.
svc hardware can auto issue SDR part.
To: Alexandre Belloni <alexandre.belloni@bootlin.com>
To: Miquel Raynal <miquel.raynal@bootlin.com>
To: Conor Culhane <conor.culhane@silvaco.com>
Cc: linux-i3c@lists.infradead.org
Cc: linux-kernel@vger.kernel.org
Cc: imx@lists.linux.dev
Signed-off-by: Frank Li <Frank.Li@nxp.com>
---
Frank Li (2):
i3c: Add basic HDR support
i3c: master: svc: Add basic HDR mode support
drivers/i3c/device.c | 19 ++++++----
drivers/i3c/internals.h | 2 +-
drivers/i3c/master.c | 8 ++++-
drivers/i3c/master/svc-i3c-master.c | 69 +++++++++++++++++++++++++++----------
include/linux/i3c/device.h | 12 ++++++-
include/linux/i3c/master.h | 3 ++
6 files changed, 86 insertions(+), 27 deletions(-)
---
base-commit: 66408a65f3db566c45fd7658fa7bc0b17aaa4807
change-id: 20250129-i3c_ddr-b15488901eb8
Best regards,
---
Frank Li <Frank.Li@nxp.com>
--
linux-i3c mailing list
linux-i3c@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-i3c
^ permalink raw reply [flat|nested] 21+ messages in thread* [PATCH 1/2] i3c: Add basic HDR support 2025-01-29 20:05 [PATCH 0/2] i3c: Add basic HDR mode support Frank Li @ 2025-01-29 20:05 ` Frank Li 2025-02-01 17:54 ` Mukesh Kumar Savaliya 2025-01-29 20:05 ` [PATCH 2/2] i3c: master: svc: Add basic HDR mode support Frank Li 1 sibling, 1 reply; 21+ messages in thread From: Frank Li @ 2025-01-29 20:05 UTC (permalink / raw) To: Alexandre Belloni, Miquel Raynal, Conor Culhane Cc: linux-i3c, linux-kernel, imx, Frank Li 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(-) 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, -- 2.34.1 -- linux-i3c mailing list linux-i3c@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-i3c ^ permalink raw reply related [flat|nested] 21+ messages in thread
* Re: [PATCH 1/2] i3c: Add basic HDR support 2025-01-29 20:05 ` [PATCH 1/2] i3c: Add basic HDR support Frank Li @ 2025-02-01 17:54 ` Mukesh Kumar Savaliya 2025-02-03 2:32 ` Joshua Yeong 0 siblings, 1 reply; 21+ messages in thread From: Mukesh Kumar Savaliya @ 2025-02-01 17:54 UTC (permalink / raw) To: Frank Li, Alexandre Belloni, Miquel Raynal, Conor Culhane Cc: linux-i3c, linux-kernel, imx 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, > -- linux-i3c mailing list linux-i3c@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-i3c ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH 1/2] i3c: Add basic HDR support 2025-02-01 17:54 ` Mukesh Kumar Savaliya @ 2025-02-03 2:32 ` Joshua Yeong 2025-02-03 5:22 ` Mukesh Kumar Savaliya 2025-02-03 15:52 ` Frank Li 0 siblings, 2 replies; 21+ messages in thread From: Joshua Yeong @ 2025-02-03 2:32 UTC (permalink / raw) To: Mukesh Kumar Savaliya, Frank Li, Alexandre Belloni, Miquel Raynal, Conor Culhane Cc: linux-i3c@lists.infradead.org, linux-kernel@vger.kernel.org, imx@lists.linux.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, >> > > -- linux-i3c mailing list linux-i3c@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-i3c ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH 1/2] i3c: Add basic HDR support 2025-02-03 2:32 ` Joshua Yeong @ 2025-02-03 5:22 ` Mukesh Kumar Savaliya 2025-02-03 15:52 ` Frank Li 1 sibling, 0 replies; 21+ messages in thread From: Mukesh Kumar Savaliya @ 2025-02-03 5:22 UTC (permalink / raw) To: Joshua Yeong, Frank Li, Alexandre Belloni, Miquel Raynal, Conor Culhane Cc: linux-i3c@lists.infradead.org, linux-kernel@vger.kernel.org, imx@lists.linux.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, >>> >> >> > -- linux-i3c mailing list linux-i3c@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-i3c ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH 1/2] i3c: Add basic HDR support 2025-02-03 2:32 ` Joshua Yeong 2025-02-03 5:22 ` Mukesh Kumar Savaliya @ 2025-02-03 15:52 ` Frank Li 2025-02-04 3:17 ` Joshua Yeong 2025-02-04 5:54 ` Mukesh Kumar Savaliya 1 sibling, 2 replies; 21+ messages in thread From: Frank Li @ 2025-02-03 15:52 UTC (permalink / raw) To: Joshua Yeong Cc: Mukesh Kumar Savaliya, Alexandre Belloni, Miquel Raynal, Conor Culhane, linux-i3c@lists.infradead.org, linux-kernel@vger.kernel.org, imx@lists.linux.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, > >> > > > > > -- linux-i3c mailing list linux-i3c@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-i3c ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH 1/2] i3c: Add basic HDR support 2025-02-03 15:52 ` Frank Li @ 2025-02-04 3:17 ` Joshua Yeong 2025-02-04 15:38 ` Frank Li 2025-02-04 5:54 ` Mukesh Kumar Savaliya 1 sibling, 1 reply; 21+ messages in thread From: Joshua Yeong @ 2025-02-04 3:17 UTC (permalink / raw) To: Frank Li Cc: Mukesh Kumar Savaliya, Alexandre Belloni, Miquel Raynal, Conor Culhane, linux-i3c@lists.infradead.org, linux-kernel@vger.kernel.org, imx@lists.linux.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, >>>> >>> >>> >> -- linux-i3c mailing list linux-i3c@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-i3c ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH 1/2] i3c: Add basic HDR support 2025-02-04 3:17 ` Joshua Yeong @ 2025-02-04 15:38 ` Frank Li 2025-02-05 6:30 ` Joshua Yeong 0 siblings, 1 reply; 21+ messages in thread From: Frank Li @ 2025-02-04 15:38 UTC (permalink / raw) To: Joshua Yeong Cc: Mukesh Kumar Savaliya, Alexandre Belloni, Miquel Raynal, Conor Culhane, linux-i3c@lists.infradead.org, linux-kernel@vger.kernel.org, imx@lists.linux.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, > >>>> > >>> > >>> > >> > -- linux-i3c mailing list linux-i3c@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-i3c ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH 1/2] i3c: Add basic HDR support 2025-02-04 15:38 ` Frank Li @ 2025-02-05 6:30 ` Joshua Yeong 2025-02-05 15:58 ` Frank Li 0 siblings, 1 reply; 21+ messages in thread From: Joshua Yeong @ 2025-02-05 6:30 UTC (permalink / raw) To: Frank Li Cc: Mukesh Kumar Savaliya, Alexandre Belloni, Miquel Raynal, Conor Culhane, linux-i3c@lists.infradead.org, linux-kernel@vger.kernel.org, imx@lists.linux.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, >>>>>> >>>>> >>>>> >>>> >> -- linux-i3c mailing list linux-i3c@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-i3c ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH 1/2] i3c: Add basic HDR support 2025-02-05 6:30 ` Joshua Yeong @ 2025-02-05 15:58 ` Frank Li 2025-02-06 7:05 ` Joshua Yeong 0 siblings, 1 reply; 21+ messages in thread From: Frank Li @ 2025-02-05 15:58 UTC (permalink / raw) To: Joshua Yeong Cc: Mukesh Kumar Savaliya, Alexandre Belloni, Miquel Raynal, Conor Culhane, linux-i3c@lists.infradead.org, linux-kernel@vger.kernel.org, imx@lists.linux.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 -- linux-i3c mailing list linux-i3c@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-i3c ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH 1/2] i3c: Add basic HDR support 2025-02-05 15:58 ` Frank Li @ 2025-02-06 7:05 ` Joshua Yeong 2025-02-06 15:55 ` Frank Li 0 siblings, 1 reply; 21+ messages in thread From: Joshua Yeong @ 2025-02-06 7:05 UTC (permalink / raw) To: Frank Li Cc: Mukesh Kumar Savaliya, Alexandre Belloni, Miquel Raynal, Conor Culhane, linux-i3c@lists.infradead.org, linux-kernel@vger.kernel.org, imx@lists.linux.dev 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 -- linux-i3c mailing list linux-i3c@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-i3c ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH 1/2] i3c: Add basic HDR support 2025-02-06 7:05 ` Joshua Yeong @ 2025-02-06 15:55 ` Frank Li 2025-02-09 22:26 ` Joshua Yeong 0 siblings, 1 reply; 21+ messages in thread From: Frank Li @ 2025-02-06 15:55 UTC (permalink / raw) To: Joshua Yeong Cc: Mukesh Kumar Savaliya, Alexandre Belloni, Miquel Raynal, Conor Culhane, linux-i3c@lists.infradead.org, linux-kernel@vger.kernel.org, imx@lists.linux.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 -- linux-i3c mailing list linux-i3c@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-i3c ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH 1/2] i3c: Add basic HDR support 2025-02-06 15:55 ` Frank Li @ 2025-02-09 22:26 ` Joshua Yeong 0 siblings, 0 replies; 21+ messages in thread From: Joshua Yeong @ 2025-02-09 22:26 UTC (permalink / raw) To: Frank Li Cc: Mukesh Kumar Savaliya, Alexandre Belloni, Miquel Raynal, Conor Culhane, linux-i3c@lists.infradead.org, linux-kernel@vger.kernel.org, imx@lists.linux.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 -- linux-i3c mailing list linux-i3c@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-i3c ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH 1/2] i3c: Add basic HDR support 2025-02-03 15:52 ` Frank Li 2025-02-04 3:17 ` Joshua Yeong @ 2025-02-04 5:54 ` Mukesh Kumar Savaliya 2025-02-04 15:43 ` Frank Li 1 sibling, 1 reply; 21+ messages in thread From: Mukesh Kumar Savaliya @ 2025-02-04 5:54 UTC (permalink / raw) To: Frank Li, Joshua Yeong Cc: Alexandre Belloni, Miquel Raynal, Conor Culhane, linux-i3c@lists.infradead.org, linux-kernel@vger.kernel.org, imx@lists.linux.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, >>>> >>> >>> >> -- linux-i3c mailing list linux-i3c@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-i3c ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH 1/2] i3c: Add basic HDR support 2025-02-04 5:54 ` Mukesh Kumar Savaliya @ 2025-02-04 15:43 ` Frank Li 2025-02-05 14:19 ` Mukesh Kumar Savaliya 0 siblings, 1 reply; 21+ messages in thread From: Frank Li @ 2025-02-04 15:43 UTC (permalink / raw) To: Mukesh Kumar Savaliya Cc: Joshua Yeong, Alexandre Belloni, Miquel Raynal, Conor Culhane, linux-i3c@lists.infradead.org, linux-kernel@vger.kernel.org, imx@lists.linux.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, > > > > > > > > > > > > > > > > > -- linux-i3c mailing list linux-i3c@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-i3c ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH 1/2] i3c: Add basic HDR support 2025-02-04 15:43 ` Frank Li @ 2025-02-05 14:19 ` Mukesh Kumar Savaliya 2025-02-05 15:44 ` Frank Li 0 siblings, 1 reply; 21+ messages in thread From: Mukesh Kumar Savaliya @ 2025-02-05 14:19 UTC (permalink / raw) To: Frank Li Cc: Joshua Yeong, Alexandre Belloni, Miquel Raynal, Conor Culhane, linux-i3c@lists.infradead.org, linux-kernel@vger.kernel.org, imx@lists.linux.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, >>>>>> >>>>> >>>>> >>>> >> -- linux-i3c mailing list linux-i3c@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-i3c ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH 1/2] i3c: Add basic HDR support 2025-02-05 14:19 ` Mukesh Kumar Savaliya @ 2025-02-05 15:44 ` Frank Li 2025-02-06 9:44 ` Mukesh Kumar Savaliya 0 siblings, 1 reply; 21+ messages in thread From: Frank Li @ 2025-02-05 15:44 UTC (permalink / raw) To: Mukesh Kumar Savaliya Cc: Joshua Yeong, Alexandre Belloni, Miquel Raynal, Conor Culhane, linux-i3c@lists.infradead.org, linux-kernel@vger.kernel.org, imx@lists.linux.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, > > > > > > > > > > > > > > > > > > > > > > > > > > > > -- linux-i3c mailing list linux-i3c@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-i3c ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH 1/2] i3c: Add basic HDR support 2025-02-05 15:44 ` Frank Li @ 2025-02-06 9:44 ` Mukesh Kumar Savaliya 2025-02-06 15:32 ` Frank Li 0 siblings, 1 reply; 21+ messages in thread From: Mukesh Kumar Savaliya @ 2025-02-06 9:44 UTC (permalink / raw) To: Frank Li Cc: Joshua Yeong, Alexandre Belloni, Miquel Raynal, Conor Culhane, linux-i3c@lists.infradead.org, linux-kernel@vger.kernel.org, imx@lists.linux.dev 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, >>>>>>>> >>>>>>> >>>>>>> >>>>>> >>>> >> -- linux-i3c mailing list linux-i3c@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-i3c ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH 1/2] i3c: Add basic HDR support 2025-02-06 9:44 ` Mukesh Kumar Savaliya @ 2025-02-06 15:32 ` Frank Li 2025-02-07 9:08 ` Mukesh Kumar Savaliya 0 siblings, 1 reply; 21+ messages in thread From: Frank Li @ 2025-02-06 15:32 UTC (permalink / raw) To: Mukesh Kumar Savaliya Cc: Joshua Yeong, Alexandre Belloni, Miquel Raynal, Conor Culhane, linux-i3c@lists.infradead.org, linux-kernel@vger.kernel.org, imx@lists.linux.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, > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > -- linux-i3c mailing list linux-i3c@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-i3c ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH 1/2] i3c: Add basic HDR support 2025-02-06 15:32 ` Frank Li @ 2025-02-07 9:08 ` Mukesh Kumar Savaliya 0 siblings, 0 replies; 21+ messages in thread From: Mukesh Kumar Savaliya @ 2025-02-07 9:08 UTC (permalink / raw) To: Frank Li Cc: Joshua Yeong, Alexandre Belloni, Miquel Raynal, Conor Culhane, linux-i3c@lists.infradead.org, linux-kernel@vger.kernel.org, imx@lists.linux.dev 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, >>>>>>>>>> >>>>>>>>> >>>>>>>>> >>>>>>>> >>>>>> >>>> >> -- linux-i3c mailing list linux-i3c@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-i3c ^ permalink raw reply [flat|nested] 21+ messages in thread
* [PATCH 2/2] i3c: master: svc: Add basic HDR mode support 2025-01-29 20:05 [PATCH 0/2] i3c: Add basic HDR mode support Frank Li 2025-01-29 20:05 ` [PATCH 1/2] i3c: Add basic HDR support Frank Li @ 2025-01-29 20:05 ` Frank Li 1 sibling, 0 replies; 21+ messages in thread From: Frank Li @ 2025-01-29 20:05 UTC (permalink / raw) To: Alexandre Belloni, Miquel Raynal, Conor Culhane Cc: linux-i3c, linux-kernel, imx, Frank Li Add basic HDR mode support for the svs I3C master driver. Supports HDR only for private transfers and does not support sending CCC commands in HDR mode. Key differences: - HDR uses commands (0x00-0x7F for write, 0x80-0xFF for read) to distinguish transfer direction. - HDR read/write commands must be written to FIFO before issuing the I3C address command. The hardware automatically sends the standard CCC command to enter HDR mode. - HDR exit pattern must be sent instead of send a stop after transfer completion. - Read/write data size must be an even number. Signed-off-by: Frank Li <Frank.Li@nxp.com> --- drivers/i3c/master/svc-i3c-master.c | 69 +++++++++++++++++++++++++++---------- 1 file changed, 51 insertions(+), 18 deletions(-) diff --git a/drivers/i3c/master/svc-i3c-master.c b/drivers/i3c/master/svc-i3c-master.c index 19a16eb44220c..28b14972ca987 100644 --- a/drivers/i3c/master/svc-i3c-master.c +++ b/drivers/i3c/master/svc-i3c-master.c @@ -39,11 +39,13 @@ #define SVC_I3C_MCTRL_REQUEST_NONE 0 #define SVC_I3C_MCTRL_REQUEST_START_ADDR 1 #define SVC_I3C_MCTRL_REQUEST_STOP 2 +#define SVC_I3C_MCTRL_REQUEST_FORCE_EXIT 6 #define SVC_I3C_MCTRL_REQUEST_IBI_ACKNACK 3 #define SVC_I3C_MCTRL_REQUEST_PROC_DAA 4 #define SVC_I3C_MCTRL_REQUEST_AUTO_IBI 7 #define SVC_I3C_MCTRL_TYPE_I3C 0 #define SVC_I3C_MCTRL_TYPE_I2C BIT(4) +#define SVC_I3C_MCTRL_TYPE_DDR BIT(5) #define SVC_I3C_MCTRL_IBIRESP_AUTO 0 #define SVC_I3C_MCTRL_IBIRESP_ACK_WITHOUT_BYTE 0 #define SVC_I3C_MCTRL_IBIRESP_ACK_WITH_BYTE BIT(7) @@ -135,12 +137,13 @@ struct svc_i3c_cmd { u8 addr; - bool rnw; + u8 rnw; u8 *in; const void *out; unsigned int len; unsigned int actual_len; struct i3c_priv_xfer *xfer; + enum i3c_hdr_mode mode; bool continued; }; @@ -337,6 +340,20 @@ svc_i3c_master_dev_from_addr(struct svc_i3c_master *master, return master->descs[i]; } +static bool svc_is_read(u8 rnw_cmd, u32 type) +{ + return (type == SVC_I3C_MCTRL_TYPE_DDR) ? !!(rnw_cmd & 0x80) : rnw_cmd; +} + +static void svc_i3c_master_emit_force_exit(struct svc_i3c_master *master) +{ + u32 reg = 0; + + writel(SVC_I3C_MCTRL_REQUEST_FORCE_EXIT, master->regs + SVC_I3C_MCTRL); + readl_poll_timeout(master->regs + SVC_I3C_MSTATUS, reg, + SVC_I3C_MSTATUS_MCTRLDONE(reg), 0, 1000); + udelay(1); +} static void svc_i3c_master_emit_stop(struct svc_i3c_master *master) { writel(SVC_I3C_MCTRL_REQUEST_STOP, master->regs + SVC_I3C_MCTRL); @@ -1205,6 +1222,7 @@ static int svc_i3c_master_xfer(struct svc_i3c_master *master, u8 *in, const u8 *out, unsigned int xfer_len, unsigned int *actual_len, bool continued) { + unsigned int rdterm_len = *actual_len; int retry = 2; u32 reg; int ret; @@ -1212,14 +1230,19 @@ static int svc_i3c_master_xfer(struct svc_i3c_master *master, /* clean SVC_I3C_MINT_IBIWON w1c bits */ writel(SVC_I3C_MINT_IBIWON, master->regs + SVC_I3C_MSTATUS); + if (xfer_type == SVC_I3C_MCTRL_TYPE_DDR) { + writel(rnw, master->regs + SVC_I3C_MWDATAB); + if (svc_is_read(rnw, xfer_type)) + rdterm_len = DIV_ROUND_UP(*actual_len, 2); + } while (retry--) { writel(SVC_I3C_MCTRL_REQUEST_START_ADDR | xfer_type | SVC_I3C_MCTRL_IBIRESP_NACK | - SVC_I3C_MCTRL_DIR(rnw) | + SVC_I3C_MCTRL_DIR(svc_is_read(rnw, xfer_type)) | SVC_I3C_MCTRL_ADDR(addr) | - SVC_I3C_MCTRL_RDTERM(*actual_len), + SVC_I3C_MCTRL_RDTERM(rdterm_len), master->regs + SVC_I3C_MCTRL); ret = readl_poll_timeout(master->regs + SVC_I3C_MSTATUS, reg, @@ -1279,15 +1302,14 @@ static int svc_i3c_master_xfer(struct svc_i3c_master *master, break; } } - - if (rnw) + if (svc_is_read(rnw, xfer_type)) ret = svc_i3c_master_read(master, in, xfer_len); else ret = svc_i3c_master_write(master, out, xfer_len); if (ret < 0) goto emit_stop; - if (rnw) + if (svc_is_read(rnw, xfer_type)) *actual_len = ret; ret = readl_poll_timeout(master->regs + SVC_I3C_MSTATUS, reg, @@ -1298,7 +1320,10 @@ static int svc_i3c_master_xfer(struct svc_i3c_master *master, writel(SVC_I3C_MINT_COMPLETE, master->regs + SVC_I3C_MSTATUS); if (!continued) { - svc_i3c_master_emit_stop(master); + if (xfer_type != SVC_I3C_MCTRL_TYPE_DDR) + svc_i3c_master_emit_stop(master); + else + svc_i3c_master_emit_force_exit(master); /* Wait idle if stop is sent. */ readl_poll_timeout(master->regs + SVC_I3C_MSTATUS, reg, @@ -1308,7 +1333,11 @@ static int svc_i3c_master_xfer(struct svc_i3c_master *master, return 0; emit_stop: - svc_i3c_master_emit_stop(master); + if (xfer_type != SVC_I3C_MCTRL_TYPE_DDR) + svc_i3c_master_emit_stop(master); + else + svc_i3c_master_emit_force_exit(master); + svc_i3c_master_clear_merrwarn(master); return ret; @@ -1354,6 +1383,11 @@ static void svc_i3c_master_dequeue_xfer(struct svc_i3c_master *master, spin_unlock_irqrestore(&master->xferqueue.lock, flags); } +static int mode_to_type(enum i3c_hdr_mode mode) +{ + return (mode == I3C_SDR) ? SVC_I3C_MCTRL_TYPE_I3C : SVC_I3C_MCTRL_TYPE_DDR; +} + static void svc_i3c_master_start_xfer_locked(struct svc_i3c_master *master) { struct svc_i3c_xfer *xfer = master->xferqueue.cur; @@ -1368,7 +1402,7 @@ static void svc_i3c_master_start_xfer_locked(struct svc_i3c_master *master) for (i = 0; i < xfer->ncmds; i++) { struct svc_i3c_cmd *cmd = &xfer->cmds[i]; - ret = svc_i3c_master_xfer(master, cmd->rnw, xfer->type, + ret = svc_i3c_master_xfer(master, cmd->rnw, mode_to_type(cmd->mode), cmd->addr, cmd->in, cmd->out, cmd->len, &cmd->actual_len, cmd->continued); @@ -1544,9 +1578,8 @@ static int svc_i3c_master_send_ccc_cmd(struct i3c_master_controller *m, return ret; } -static int svc_i3c_master_priv_xfers(struct i3c_dev_desc *dev, - struct i3c_priv_xfer *xfers, - int nxfers) +static int svc_i3c_master_priv_xfers_mode(struct i3c_dev_desc *dev, struct i3c_priv_xfer *xfers, + int nxfers, enum i3c_hdr_mode mode) { struct i3c_master_controller *m = i3c_dev_get_master(dev); struct svc_i3c_master *master = to_svc_i3c_master(m); @@ -1558,16 +1591,16 @@ static int svc_i3c_master_priv_xfers(struct i3c_dev_desc *dev, if (!xfer) return -ENOMEM; - xfer->type = SVC_I3C_MCTRL_TYPE_I3C; for (i = 0; i < nxfers; i++) { + u8 rnw_cmd = (mode == I3C_SDR) ? xfers[i].rnw : xfers[i].cmd; struct svc_i3c_cmd *cmd = &xfer->cmds[i]; - + cmd->mode = mode; cmd->xfer = &xfers[i]; cmd->addr = master->addrs[data->index]; - cmd->rnw = xfers[i].rnw; - cmd->in = xfers[i].rnw ? xfers[i].data.in : NULL; - cmd->out = xfers[i].rnw ? NULL : xfers[i].data.out; + cmd->rnw = rnw_cmd; + cmd->in = svc_is_read(rnw_cmd, mode_to_type(mode)) ? xfers[i].data.in : NULL; + cmd->out = svc_is_read(rnw_cmd, mode_to_type(mode)) ? NULL : xfers[i].data.out; cmd->len = xfers[i].len; cmd->actual_len = xfers[i].rnw ? xfers[i].len : 0; cmd->continued = (i + 1) < nxfers; @@ -1766,7 +1799,7 @@ static const struct i3c_master_controller_ops svc_i3c_master_ops = { .do_daa = svc_i3c_master_do_daa, .supports_ccc_cmd = svc_i3c_master_supports_ccc_cmd, .send_ccc_cmd = svc_i3c_master_send_ccc_cmd, - .priv_xfers = svc_i3c_master_priv_xfers, + .priv_xfers_mode = svc_i3c_master_priv_xfers_mode, .i2c_xfers = svc_i3c_master_i2c_xfers, .request_ibi = svc_i3c_master_request_ibi, .free_ibi = svc_i3c_master_free_ibi, -- 2.34.1 -- linux-i3c mailing list linux-i3c@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-i3c ^ permalink raw reply related [flat|nested] 21+ messages in thread
end of thread, other threads:[~2025-02-09 22:26 UTC | newest] Thread overview: 21+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2025-01-29 20:05 [PATCH 0/2] i3c: Add basic HDR mode support Frank Li 2025-01-29 20:05 ` [PATCH 1/2] i3c: Add basic HDR support Frank Li 2025-02-01 17:54 ` Mukesh Kumar Savaliya 2025-02-03 2:32 ` Joshua Yeong 2025-02-03 5:22 ` Mukesh Kumar Savaliya 2025-02-03 15:52 ` Frank Li 2025-02-04 3:17 ` Joshua Yeong 2025-02-04 15:38 ` Frank Li 2025-02-05 6:30 ` Joshua Yeong 2025-02-05 15:58 ` Frank Li 2025-02-06 7:05 ` Joshua Yeong 2025-02-06 15:55 ` Frank Li 2025-02-09 22:26 ` Joshua Yeong 2025-02-04 5:54 ` Mukesh Kumar Savaliya 2025-02-04 15:43 ` Frank Li 2025-02-05 14:19 ` Mukesh Kumar Savaliya 2025-02-05 15:44 ` Frank Li 2025-02-06 9:44 ` Mukesh Kumar Savaliya 2025-02-06 15:32 ` Frank Li 2025-02-07 9:08 ` Mukesh Kumar Savaliya 2025-01-29 20:05 ` [PATCH 2/2] i3c: master: svc: Add basic HDR mode support Frank Li
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).