| Message ID | efb3b8692d3579ab3369cc0f80e237550f3e8209.1544702829.git.pgaj@cadence.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | Add the I3C HDR modes | expand |
On Thu, 13 Dec 2018 12:18:31 +0000 Przemyslaw Gaj <pgaj@cadence.com> wrote: > HDR (High Data Rate) modes is an important feature of the I3C protocol > as it allows to get higher throughput than with the SDR (Single Data > Rate) mode. > > Add new controller hooks and extend the I3C device API to expose this > new feature. > > This feature was originally created by Boris Brezillon > <boris.brezillon@bootlin.com>. You should keep the orginal author and it's SoB and then add your own SoB. > > Signed-off-by: Przemyslaw Gaj <pgaj@cadence.com> > --- > drivers/i3c/device.c | 37 +++++++++++++++++++++++++++++++++++++ > drivers/i3c/internals.h | 3 +++ > drivers/i3c/master.c | 31 +++++++++++++++++++++++++++++++ > include/linux/i3c/device.h | 29 +++++++++++++++++++++++++++++ > include/linux/i3c/master.h | 7 +++++++ > 5 files changed, 107 insertions(+) > > diff --git a/drivers/i3c/device.c b/drivers/i3c/device.c > index 69cc040..97910aa 100644 > --- a/drivers/i3c/device.c > +++ b/drivers/i3c/device.c > @@ -51,6 +51,43 @@ int i3c_device_do_priv_xfers(struct i3c_device *dev, > EXPORT_SYMBOL_GPL(i3c_device_do_priv_xfers); > > /** > + * i3c_device_send_hdr_cmds() - send HDR commands to a specific device > + * > + * @dev: device to which these commands should be sent > + * @xfers: array of commands > + * @nxfers: number of commands > + * > + * Send one or several HDR commands to @dev. > + * > + * This function can sleep and thus cannot be called in atomic context. > + * > + * Return: 0 in case of success, a negative error core otherwise. > + */ > +int i3c_device_send_hdr_cmds(struct i3c_device *dev, > + struct i3c_hdr_cmd *cmds, > + int ncmds) > +{ > + int ret, i; > + enum i3c_hdr_mode mode; > + > + if (ncmds < 1) > + return 0; > + > + mode = cmds[0].mode; > + for (i = 1; i < ncmds; i++) { > + if (mode != cmds[i].mode) > + return -EINVAL; > + } > + > + i3c_bus_normaluse_lock(dev->bus); > + ret = i3c_dev_send_hdr_cmds_locked(dev->desc, cmds, ncmds); > + i3c_bus_normaluse_unlock(dev->bus); > + > + return ret; > +} > +EXPORT_SYMBOL_GPL(i3c_device_send_hdr_cmds); > + > +/** > * i3c_device_get_info() - get I3C device information > * > * @dev: device we want information on > diff --git a/drivers/i3c/internals.h b/drivers/i3c/internals.h > index 86b7b44..46c4de7 100644 > --- a/drivers/i3c/internals.h > +++ b/drivers/i3c/internals.h > @@ -18,6 +18,9 @@ void i3c_bus_normaluse_unlock(struct i3c_bus *bus); > int i3c_dev_do_priv_xfers_locked(struct i3c_dev_desc *dev, > struct i3c_priv_xfer *xfers, > int nxfers); > +int i3c_dev_send_hdr_cmds_locked(struct i3c_dev_desc *dev, > + struct i3c_hdr_cmd *cmds, > + int ncmds); > 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 e98b600..16d6dd5 100644 > --- a/drivers/i3c/master.c > +++ b/drivers/i3c/master.c > @@ -2791,6 +2791,37 @@ int i3c_dev_do_priv_xfers_locked(struct i3c_dev_desc *dev, > return master->ops->priv_xfers(dev, xfers, nxfers); > } > > +int i3c_dev_send_hdr_cmds_locked(struct i3c_dev_desc *dev, > + struct i3c_hdr_cmd *cmds, > + int ncmds) > +{ > + struct i3c_master_controller *master; > + int i; > + > + if (!dev) > + return -ENOENT; > + > + master = i3c_dev_get_master(dev); > + if (!master || !cmds) > + return -EINVAL; > + > + if (master->op_mode == I3C_SLAVE_MODE) { > + if (i3c_master_request_mastership(master)) > + return -EIO; > + } > + > + if (!master->ops->send_hdr_cmds) > + return -ENOTSUPP; > + > + for (i = 0; i < ncmds; i++) { > + if (!(master->this->info.hdr_cap & BIT(cmds->mode))) > + return -ENOTSUPP; > + } > + > + return master->ops->send_hdr_cmds(dev, cmds, ncmds); > +} > + > + > int i3c_dev_disable_ibi_locked(struct i3c_dev_desc *dev) > { > struct i3c_master_controller *master; > diff --git a/include/linux/i3c/device.h b/include/linux/i3c/device.h > index 5ecb055..75a947f 100644 > --- a/include/linux/i3c/device.h > +++ b/include/linux/i3c/device.h > @@ -49,6 +49,31 @@ enum i3c_hdr_mode { > I3C_HDR_TSL, > }; > > +#define I3C_HDR_GEN_WRITE_CMD(id) (id) > +#define I3C_HDR_VENDOR_WRITE_CMD(id) (0x20 + (id)) > +#define I3C_HDR_IS_READ_CMD BIT(7) > +#define I3C_HDR_GEN_READ_CMD(id) (0x80 + (id)) > +#define I3C_HDR_VENDOR_READ_CMD(id) (0xa0 + (id)) > + > +/** > + * struct i3c_hdr_cmd - I3C HDR command > + * @mode: HDR mode selected for this command > + * @code: command opcode. Bit 7 encodes the direction of the data transfer, if > + * set this is a read, otherwise this is a write > + * @ndatawords: number of data words (a word is 16bits wide) to transfer > + * @data: input/output buffer > + */ > +struct i3c_hdr_cmd { > + enum i3c_hdr_mode mode; > + u8 code; > + int ndatawords; > + union { > + u16 *in; > + const u16 *out; > + } data; > +}; > + > + > /** > * struct i3c_priv_xfer - I3C SDR private transfer > * @rnw: encodes the transfer direction. true for a read, false for a write > @@ -289,6 +314,10 @@ int i3c_device_do_priv_xfers(struct i3c_device *dev, > struct i3c_priv_xfer *xfers, > int nxfers); > > +int i3c_device_send_hdr_cmds(struct i3c_device *dev, > + struct i3c_hdr_cmd *cmds, > + int ncmds); > + > void i3c_device_get_info(struct i3c_device *dev, struct i3c_device_info *info); > > struct i3c_ibi_payload { > diff --git a/include/linux/i3c/master.h b/include/linux/i3c/master.h > index ada956a..fd50473 100644 > --- a/include/linux/i3c/master.h > +++ b/include/linux/i3c/master.h > @@ -386,6 +386,10 @@ struct i3c_bus { > * This method is mandatory. > * @priv_xfers: do one or several private I3C SDR transfers > * This method is mandatory. > + * @send_hdr_cmds: send one or several HDR commands. If there is more than one > + * command, they should ideally be sent in the same HDR > + * transaction. > + * This method is optional. > * @attach_i2c_dev: called every time an I2C device is attached to the bus. > * This is a good place to attach master controller specific > * data to I2C devices. > @@ -457,6 +461,9 @@ struct i3c_master_controller_ops { > int (*priv_xfers)(struct i3c_dev_desc *dev, > struct i3c_priv_xfer *xfers, > int nxfers); > + int (*send_hdr_cmds)(struct i3c_dev_desc *dev, > + const struct i3c_hdr_cmd *cmds, > + int ncmds); > int (*attach_i2c_dev)(struct i2c_dev_desc *dev); > void (*detach_i2c_dev)(struct i2c_dev_desc *dev); > int (*i2c_xfers)(struct i2c_dev_desc *dev,
Hi Przemek, Sorry for the late response. On 13/12/18 12:18, Przemyslaw Gaj wrote: > HDR (High Data Rate) modes is an important feature of the I3C protocol > as it allows to get higher throughput than with the SDR (Single Data > Rate) mode. > > Add new controller hooks and extend the I3C device API to expose this > new feature. > > This feature was originally created by Boris Brezillon > <boris.brezillon@bootlin.com>. > > Signed-off-by: Przemyslaw Gaj <pgaj@cadence.com> > --- > drivers/i3c/device.c | 37 +++++++++++++++++++++++++++++++++++++ > drivers/i3c/internals.h | 3 +++ > drivers/i3c/master.c | 31 +++++++++++++++++++++++++++++++ > include/linux/i3c/device.h | 29 +++++++++++++++++++++++++++++ > include/linux/i3c/master.h | 7 +++++++ > 5 files changed, 107 insertions(+) > > diff --git a/drivers/i3c/device.c b/drivers/i3c/device.c > index 69cc040..97910aa 100644 > --- a/drivers/i3c/device.c > +++ b/drivers/i3c/device.c > @@ -51,6 +51,43 @@ int i3c_device_do_priv_xfers(struct i3c_device *dev, > EXPORT_SYMBOL_GPL(i3c_device_do_priv_xfers); > > /** > + * i3c_device_send_hdr_cmds() - send HDR commands to a specific device > + * > + * @dev: device to which these commands should be sent > + * @xfers: array of commands > + * @nxfers: number of commands > + * > + * Send one or several HDR commands to @dev. > + * > + * This function can sleep and thus cannot be called in atomic context. > + * > + * Return: 0 in case of success, a negative error core otherwise. > + */ > +int i3c_device_send_hdr_cmds(struct i3c_device *dev, > + struct i3c_hdr_cmd *cmds, > + int ncmds) > +{ > + int ret, i; > + enum i3c_hdr_mode mode; > + > + if (ncmds < 1) > + return 0; > + > + mode = cmds[0].mode; > + for (i = 1; i < ncmds; i++) { > + if (mode != cmds[i].mode) > + return -EINVAL; > + } > + > + i3c_bus_normaluse_lock(dev->bus); > + ret = i3c_dev_send_hdr_cmds_locked(dev->desc, cmds, ncmds); > + i3c_bus_normaluse_unlock(dev->bus); > + > + return ret; > +} > +EXPORT_SYMBOL_GPL(i3c_device_send_hdr_cmds); > + > +/** > * i3c_device_get_info() - get I3C device information > * > * @dev: device we want information on > diff --git a/drivers/i3c/internals.h b/drivers/i3c/internals.h > index 86b7b44..46c4de7 100644 > --- a/drivers/i3c/internals.h > +++ b/drivers/i3c/internals.h > @@ -18,6 +18,9 @@ void i3c_bus_normaluse_unlock(struct i3c_bus *bus); > int i3c_dev_do_priv_xfers_locked(struct i3c_dev_desc *dev, > struct i3c_priv_xfer *xfers, > int nxfers); > +int i3c_dev_send_hdr_cmds_locked(struct i3c_dev_desc *dev, > + struct i3c_hdr_cmd *cmds, > + int ncmds); > 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 e98b600..16d6dd5 100644 > --- a/drivers/i3c/master.c > +++ b/drivers/i3c/master.c > @@ -2791,6 +2791,37 @@ int i3c_dev_do_priv_xfers_locked(struct i3c_dev_desc *dev, > return master->ops->priv_xfers(dev, xfers, nxfers); > } > > +int i3c_dev_send_hdr_cmds_locked(struct i3c_dev_desc *dev, > + struct i3c_hdr_cmd *cmds, > + int ncmds) > +{ > + struct i3c_master_controller *master; > + int i; > + > + if (!dev) > + return -ENOENT; > + > + master = i3c_dev_get_master(dev); > + if (!master || !cmds) > + return -EINVAL; > + > + if (master->op_mode == I3C_SLAVE_MODE) { > + if (i3c_master_request_mastership(master)) > + return -EIO; > + } This patch seems to be applied on top of secondary master patch proposal. I think it is better to remove the secondary master stuffs from here. > + > + if (!master->ops->send_hdr_cmds) > + return -ENOTSUPP; > + > + for (i = 0; i < ncmds; i++) { > + if (!(master->this->info.hdr_cap & BIT(cmds->mode))) > + return -ENOTSUPP; > + } > + > + return master->ops->send_hdr_cmds(dev, cmds, ncmds); > +} > + > + > int i3c_dev_disable_ibi_locked(struct i3c_dev_desc *dev) > { > struct i3c_master_controller *master; > diff --git a/include/linux/i3c/device.h b/include/linux/i3c/device.h > index 5ecb055..75a947f 100644 > --- a/include/linux/i3c/device.h > +++ b/include/linux/i3c/device.h > @@ -49,6 +49,31 @@ enum i3c_hdr_mode { > I3C_HDR_TSL, > }; > > +#define I3C_HDR_GEN_WRITE_CMD(id) (id) > +#define I3C_HDR_VENDOR_WRITE_CMD(id) (0x20 + (id)) > +#define I3C_HDR_IS_READ_CMD BIT(7) > +#define I3C_HDR_GEN_READ_CMD(id) (0x80 + (id)) > +#define I3C_HDR_VENDOR_READ_CMD(id) (0xa0 + (id)) > + > +/** > + * struct i3c_hdr_cmd - I3C HDR command > + * @mode: HDR mode selected for this command > + * @code: command opcode. Bit 7 encodes the direction of the data transfer, if > + * set this is a read, otherwise this is a write > + * @ndatawords: number of data words (a word is 16bits wide) to transfer > + * @data: input/output buffer > + */ > +struct i3c_hdr_cmd { > + enum i3c_hdr_mode mode; > + u8 code; > + int ndatawords; > + union { > + u16 *in; > + const u16 *out; > + } data; > +}; > + > + > /** > * struct i3c_priv_xfer - I3C SDR private transfer > * @rnw: encodes the transfer direction. true for a read, false for a write > @@ -289,6 +314,10 @@ int i3c_device_do_priv_xfers(struct i3c_device *dev, > struct i3c_priv_xfer *xfers, > int nxfers); > > +int i3c_device_send_hdr_cmds(struct i3c_device *dev, > + struct i3c_hdr_cmd *cmds, > + int ncmds); > + > void i3c_device_get_info(struct i3c_device *dev, struct i3c_device_info *info); > > struct i3c_ibi_payload { > diff --git a/include/linux/i3c/master.h b/include/linux/i3c/master.h > index ada956a..fd50473 100644 > --- a/include/linux/i3c/master.h > +++ b/include/linux/i3c/master.h > @@ -386,6 +386,10 @@ struct i3c_bus { > * This method is mandatory. > * @priv_xfers: do one or several private I3C SDR transfers > * This method is mandatory. > + * @send_hdr_cmds: send one or several HDR commands. If there is more than one > + * command, they should ideally be sent in the same HDR > + * transaction. > + * This method is optional. > * @attach_i2c_dev: called every time an I2C device is attached to the bus. > * This is a good place to attach master controller specific > * data to I2C devices. > @@ -457,6 +461,9 @@ struct i3c_master_controller_ops { > int (*priv_xfers)(struct i3c_dev_desc *dev, > struct i3c_priv_xfer *xfers, > int nxfers); > + int (*send_hdr_cmds)(struct i3c_dev_desc *dev, > + const struct i3c_hdr_cmd *cmds, > + int ncmds); > 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, With this approach the controller between a start and stop can only transmit in SDR or HDR. This is limited for devices that need the following frame: <Start><SDR xfer><Repeated Start><HDR command><Stop> Best regards, Vitor Soares
On Thu, 21 Feb 2019 15:15:57 +0000 vitor <vitor.soares@synopsys.com> wrote: > Hi Przemek, > > Sorry for the late response. > > On 13/12/18 12:18, Przemyslaw Gaj wrote: > > HDR (High Data Rate) modes is an important feature of the I3C protocol > > as it allows to get higher throughput than with the SDR (Single Data > > Rate) mode. > > > > Add new controller hooks and extend the I3C device API to expose this > > new feature. > > > > This feature was originally created by Boris Brezillon > > <boris.brezillon@bootlin.com>. > > > > Signed-off-by: Przemyslaw Gaj <pgaj@cadence.com> > > --- > > drivers/i3c/device.c | 37 +++++++++++++++++++++++++++++++++++++ > > drivers/i3c/internals.h | 3 +++ > > drivers/i3c/master.c | 31 +++++++++++++++++++++++++++++++ > > include/linux/i3c/device.h | 29 +++++++++++++++++++++++++++++ > > include/linux/i3c/master.h | 7 +++++++ > > 5 files changed, 107 insertions(+) > > > > diff --git a/drivers/i3c/device.c b/drivers/i3c/device.c > > index 69cc040..97910aa 100644 > > --- a/drivers/i3c/device.c > > +++ b/drivers/i3c/device.c > > @@ -51,6 +51,43 @@ int i3c_device_do_priv_xfers(struct i3c_device *dev, > > EXPORT_SYMBOL_GPL(i3c_device_do_priv_xfers); > > > > /** > > + * i3c_device_send_hdr_cmds() - send HDR commands to a specific device > > + * > > + * @dev: device to which these commands should be sent > > + * @xfers: array of commands > > + * @nxfers: number of commands > > + * > > + * Send one or several HDR commands to @dev. > > + * > > + * This function can sleep and thus cannot be called in atomic context. > > + * > > + * Return: 0 in case of success, a negative error core otherwise. > > + */ > > +int i3c_device_send_hdr_cmds(struct i3c_device *dev, > > + struct i3c_hdr_cmd *cmds, > > + int ncmds) > > +{ > > + int ret, i; > > + enum i3c_hdr_mode mode; > > + > > + if (ncmds < 1) > > + return 0; > > + > > + mode = cmds[0].mode; > > + for (i = 1; i < ncmds; i++) { > > + if (mode != cmds[i].mode) > > + return -EINVAL; > > + } > > + > > + i3c_bus_normaluse_lock(dev->bus); > > + ret = i3c_dev_send_hdr_cmds_locked(dev->desc, cmds, ncmds); > > + i3c_bus_normaluse_unlock(dev->bus); > > + > > + return ret; > > +} > > +EXPORT_SYMBOL_GPL(i3c_device_send_hdr_cmds); > > + > > +/** > > * i3c_device_get_info() - get I3C device information > > * > > * @dev: device we want information on > > diff --git a/drivers/i3c/internals.h b/drivers/i3c/internals.h > > index 86b7b44..46c4de7 100644 > > --- a/drivers/i3c/internals.h > > +++ b/drivers/i3c/internals.h > > @@ -18,6 +18,9 @@ void i3c_bus_normaluse_unlock(struct i3c_bus *bus); > > int i3c_dev_do_priv_xfers_locked(struct i3c_dev_desc *dev, > > struct i3c_priv_xfer *xfers, > > int nxfers); > > +int i3c_dev_send_hdr_cmds_locked(struct i3c_dev_desc *dev, > > + struct i3c_hdr_cmd *cmds, > > + int ncmds); > > 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 e98b600..16d6dd5 100644 > > --- a/drivers/i3c/master.c > > +++ b/drivers/i3c/master.c > > @@ -2791,6 +2791,37 @@ int i3c_dev_do_priv_xfers_locked(struct i3c_dev_desc *dev, > > return master->ops->priv_xfers(dev, xfers, nxfers); > > } > > > > +int i3c_dev_send_hdr_cmds_locked(struct i3c_dev_desc *dev, > > + struct i3c_hdr_cmd *cmds, > > + int ncmds) > > +{ > > + struct i3c_master_controller *master; > > + int i; > > + > > + if (!dev) > > + return -ENOENT; > > + > > + master = i3c_dev_get_master(dev); > > + if (!master || !cmds) > > + return -EINVAL; > > + > > + if (master->op_mode == I3C_SLAVE_MODE) { > > + if (i3c_master_request_mastership(master)) > > + return -EIO; > > + } > > This patch seems to be applied on top of secondary master patch proposal. > > I think it is better to remove the secondary master stuffs from here. > > > + > > + if (!master->ops->send_hdr_cmds) > > + return -ENOTSUPP; > > + > > + for (i = 0; i < ncmds; i++) { > > + if (!(master->this->info.hdr_cap & BIT(cmds->mode))) > > + return -ENOTSUPP; > > + } > > + > > + return master->ops->send_hdr_cmds(dev, cmds, ncmds); > > +} > > + > > + > > int i3c_dev_disable_ibi_locked(struct i3c_dev_desc *dev) > > { > > struct i3c_master_controller *master; > > diff --git a/include/linux/i3c/device.h b/include/linux/i3c/device.h > > index 5ecb055..75a947f 100644 > > --- a/include/linux/i3c/device.h > > +++ b/include/linux/i3c/device.h > > @@ -49,6 +49,31 @@ enum i3c_hdr_mode { > > I3C_HDR_TSL, > > }; > > > > +#define I3C_HDR_GEN_WRITE_CMD(id) (id) > > +#define I3C_HDR_VENDOR_WRITE_CMD(id) (0x20 + (id)) > > +#define I3C_HDR_IS_READ_CMD BIT(7) > > +#define I3C_HDR_GEN_READ_CMD(id) (0x80 + (id)) > > +#define I3C_HDR_VENDOR_READ_CMD(id) (0xa0 + (id)) > > + > > +/** > > + * struct i3c_hdr_cmd - I3C HDR command > > + * @mode: HDR mode selected for this command > > + * @code: command opcode. Bit 7 encodes the direction of the data transfer, if > > + * set this is a read, otherwise this is a write > > + * @ndatawords: number of data words (a word is 16bits wide) to transfer > > + * @data: input/output buffer > > + */ > > +struct i3c_hdr_cmd { > > + enum i3c_hdr_mode mode; > > + u8 code; > > + int ndatawords; > > + union { > > + u16 *in; > > + const u16 *out; > > + } data; > > +}; > > + > > + > > /** > > * struct i3c_priv_xfer - I3C SDR private transfer > > * @rnw: encodes the transfer direction. true for a read, false for a write > > @@ -289,6 +314,10 @@ int i3c_device_do_priv_xfers(struct i3c_device *dev, > > struct i3c_priv_xfer *xfers, > > int nxfers); > > > > +int i3c_device_send_hdr_cmds(struct i3c_device *dev, > > + struct i3c_hdr_cmd *cmds, > > + int ncmds); > > + > > void i3c_device_get_info(struct i3c_device *dev, struct i3c_device_info *info); > > > > struct i3c_ibi_payload { > > diff --git a/include/linux/i3c/master.h b/include/linux/i3c/master.h > > index ada956a..fd50473 100644 > > --- a/include/linux/i3c/master.h > > +++ b/include/linux/i3c/master.h > > @@ -386,6 +386,10 @@ struct i3c_bus { > > * This method is mandatory. > > * @priv_xfers: do one or several private I3C SDR transfers > > * This method is mandatory. > > + * @send_hdr_cmds: send one or several HDR commands. If there is more than one > > + * command, they should ideally be sent in the same HDR > > + * transaction. > > + * This method is optional. > > * @attach_i2c_dev: called every time an I2C device is attached to the bus. > > * This is a good place to attach master controller specific > > * data to I2C devices. > > @@ -457,6 +461,9 @@ struct i3c_master_controller_ops { > > int (*priv_xfers)(struct i3c_dev_desc *dev, > > struct i3c_priv_xfer *xfers, > > int nxfers); > > + int (*send_hdr_cmds)(struct i3c_dev_desc *dev, > > + const struct i3c_hdr_cmd *cmds, > > + int ncmds); > > 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, > > With this approach the controller between a start and stop can only transmit in SDR or HDR. > > This is limited for devices that need the following frame: > <Start><SDR xfer><Repeated Start><HDR command><Stop> If this is a use case we want to support, then we should probably have something more generic than what we currently have. Something like enum i3c_xfer_type { I3C_CCC_XFER, I3C_SDR_XFER, I3C_HDR_XFER, } struct i3c_xfer { enum i3c_xfer_type type; union { struct i3c_ccc_cmd ccc; struct i3c_priv_xfer sdr; struct i3c_hdr_cmd hdr; }; } struct i3c_master_controller_ops { ... int (*i3c_xfers)(struct i3c_dev_desc *dev, struct i3c_xfer *xfers, int nxfers); ... };
Sorry for the delay. It was on my todo list. The 02/22/2019 15:52, Boris Brezillon wrote: > EXTERNAL MAIL > > > On Thu, 21 Feb 2019 15:15:57 +0000 > vitor <vitor.soares@synopsys.com> wrote: > > > Hi Przemek, > > > > Sorry for the late response. > > > > On 13/12/18 12:18, Przemyslaw Gaj wrote: > > > HDR (High Data Rate) modes is an important feature of the I3C protocol > > > as it allows to get higher throughput than with the SDR (Single Data > > > Rate) mode. > > > > > > Add new controller hooks and extend the I3C device API to expose this > > > new feature. > > > > > > This feature was originally created by Boris Brezillon > > > <boris.brezillon@bootlin.com>. > > > > > > Signed-off-by: Przemyslaw Gaj <pgaj@cadence.com> > > > --- > > > drivers/i3c/device.c | 37 +++++++++++++++++++++++++++++++++++++ > > > drivers/i3c/internals.h | 3 +++ > > > drivers/i3c/master.c | 31 +++++++++++++++++++++++++++++++ > > > include/linux/i3c/device.h | 29 +++++++++++++++++++++++++++++ > > > include/linux/i3c/master.h | 7 +++++++ > > > 5 files changed, 107 insertions(+) > > > > > > diff --git a/drivers/i3c/device.c b/drivers/i3c/device.c > > > index 69cc040..97910aa 100644 > > > --- a/drivers/i3c/device.c > > > +++ b/drivers/i3c/device.c > > > @@ -51,6 +51,43 @@ int i3c_device_do_priv_xfers(struct i3c_device *dev, > > > EXPORT_SYMBOL_GPL(i3c_device_do_priv_xfers); > > > > > > /** > > > + * i3c_device_send_hdr_cmds() - send HDR commands to a specific device > > > + * > > > + * @dev: device to which these commands should be sent > > > + * @xfers: array of commands > > > + * @nxfers: number of commands > > > + * > > > + * Send one or several HDR commands to @dev. > > > + * > > > + * This function can sleep and thus cannot be called in atomic context. > > > + * > > > + * Return: 0 in case of success, a negative error core otherwise. > > > + */ > > > +int i3c_device_send_hdr_cmds(struct i3c_device *dev, > > > + struct i3c_hdr_cmd *cmds, > > > + int ncmds) > > > +{ > > > + int ret, i; > > > + enum i3c_hdr_mode mode; > > > + > > > + if (ncmds < 1) > > > + return 0; > > > + > > > + mode = cmds[0].mode; > > > + for (i = 1; i < ncmds; i++) { > > > + if (mode != cmds[i].mode) > > > + return -EINVAL; > > > + } > > > + > > > + i3c_bus_normaluse_lock(dev->bus); > > > + ret = i3c_dev_send_hdr_cmds_locked(dev->desc, cmds, ncmds); > > > + i3c_bus_normaluse_unlock(dev->bus); > > > + > > > + return ret; > > > +} > > > +EXPORT_SYMBOL_GPL(i3c_device_send_hdr_cmds); > > > + > > > +/** > > > * i3c_device_get_info() - get I3C device information > > > * > > > * @dev: device we want information on > > > diff --git a/drivers/i3c/internals.h b/drivers/i3c/internals.h > > > index 86b7b44..46c4de7 100644 > > > --- a/drivers/i3c/internals.h > > > +++ b/drivers/i3c/internals.h > > > @@ -18,6 +18,9 @@ void i3c_bus_normaluse_unlock(struct i3c_bus *bus); > > > int i3c_dev_do_priv_xfers_locked(struct i3c_dev_desc *dev, > > > struct i3c_priv_xfer *xfers, > > > int nxfers); > > > +int i3c_dev_send_hdr_cmds_locked(struct i3c_dev_desc *dev, > > > + struct i3c_hdr_cmd *cmds, > > > + int ncmds); > > > 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 e98b600..16d6dd5 100644 > > > --- a/drivers/i3c/master.c > > > +++ b/drivers/i3c/master.c > > > @@ -2791,6 +2791,37 @@ int i3c_dev_do_priv_xfers_locked(struct i3c_dev_desc *dev, > > > return master->ops->priv_xfers(dev, xfers, nxfers); > > > } > > > > > > +int i3c_dev_send_hdr_cmds_locked(struct i3c_dev_desc *dev, > > > + struct i3c_hdr_cmd *cmds, > > > + int ncmds) > > > +{ > > > + struct i3c_master_controller *master; > > > + int i; > > > + > > > + if (!dev) > > > + return -ENOENT; > > > + > > > + master = i3c_dev_get_master(dev); > > > + if (!master || !cmds) > > > + return -EINVAL; > > > + > > > + if (master->op_mode == I3C_SLAVE_MODE) { > > > + if (i3c_master_request_mastership(master)) > > > + return -EIO; > > > + } > > > > This patch seems to be applied on top of secondary master patch proposal. > > > > I think it is better to remove the secondary master stuffs from here. > > Yes, I'll do that. > > > + > > > + if (!master->ops->send_hdr_cmds) > > > + return -ENOTSUPP; > > > + > > > + for (i = 0; i < ncmds; i++) { > > > + if (!(master->this->info.hdr_cap & BIT(cmds->mode))) > > > + return -ENOTSUPP; > > > + } > > > + > > > + return master->ops->send_hdr_cmds(dev, cmds, ncmds); > > > +} > > > + > > > + > > > int i3c_dev_disable_ibi_locked(struct i3c_dev_desc *dev) > > > { > > > struct i3c_master_controller *master; > > > diff --git a/include/linux/i3c/device.h b/include/linux/i3c/device.h > > > index 5ecb055..75a947f 100644 > > > --- a/include/linux/i3c/device.h > > > +++ b/include/linux/i3c/device.h > > > @@ -49,6 +49,31 @@ enum i3c_hdr_mode { > > > I3C_HDR_TSL, > > > }; > > > > > > +#define I3C_HDR_GEN_WRITE_CMD(id) (id) > > > +#define I3C_HDR_VENDOR_WRITE_CMD(id) (0x20 + (id)) > > > +#define I3C_HDR_IS_READ_CMD BIT(7) > > > +#define I3C_HDR_GEN_READ_CMD(id) (0x80 + (id)) > > > +#define I3C_HDR_VENDOR_READ_CMD(id) (0xa0 + (id)) > > > + > > > +/** > > > + * struct i3c_hdr_cmd - I3C HDR command > > > + * @mode: HDR mode selected for this command > > > + * @code: command opcode. Bit 7 encodes the direction of the data transfer, if > > > + * set this is a read, otherwise this is a write > > > + * @ndatawords: number of data words (a word is 16bits wide) to transfer > > > + * @data: input/output buffer > > > + */ > > > +struct i3c_hdr_cmd { > > > + enum i3c_hdr_mode mode; > > > + u8 code; > > > + int ndatawords; > > > + union { > > > + u16 *in; > > > + const u16 *out; > > > + } data; > > > +}; > > > + > > > + > > > /** > > > * struct i3c_priv_xfer - I3C SDR private transfer > > > * @rnw: encodes the transfer direction. true for a read, false for a write > > > @@ -289,6 +314,10 @@ int i3c_device_do_priv_xfers(struct i3c_device *dev, > > > struct i3c_priv_xfer *xfers, > > > int nxfers); > > > > > > +int i3c_device_send_hdr_cmds(struct i3c_device *dev, > > > + struct i3c_hdr_cmd *cmds, > > > + int ncmds); > > > + > > > void i3c_device_get_info(struct i3c_device *dev, struct i3c_device_info *info); > > > > > > struct i3c_ibi_payload { > > > diff --git a/include/linux/i3c/master.h b/include/linux/i3c/master.h > > > index ada956a..fd50473 100644 > > > --- a/include/linux/i3c/master.h > > > +++ b/include/linux/i3c/master.h > > > @@ -386,6 +386,10 @@ struct i3c_bus { > > > * This method is mandatory. > > > * @priv_xfers: do one or several private I3C SDR transfers > > > * This method is mandatory. > > > + * @send_hdr_cmds: send one or several HDR commands. If there is more than one > > > + * command, they should ideally be sent in the same HDR > > > + * transaction. > > > + * This method is optional. > > > * @attach_i2c_dev: called every time an I2C device is attached to the bus. > > > * This is a good place to attach master controller specific > > > * data to I2C devices. > > > @@ -457,6 +461,9 @@ struct i3c_master_controller_ops { > > > int (*priv_xfers)(struct i3c_dev_desc *dev, > > > struct i3c_priv_xfer *xfers, > > > int nxfers); > > > + int (*send_hdr_cmds)(struct i3c_dev_desc *dev, > > > + const struct i3c_hdr_cmd *cmds, > > > + int ncmds); > > > 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, > > > > With this approach the controller between a start and stop can only transmit in SDR or HDR. > > > > This is limited for devices that need the following frame: > > <Start><SDR xfer><Repeated Start><HDR command><Stop> Actually, there is repeated start between SDR and ENTHDR. There is no repeated start between ENTHDR and HDR commands, HDR command has to start immedetly after ENTHDR. Of course, hdr restart may occur between HDR commands. > > If this is a use case we want to support, then we should probably have > something more generic than what we currently have. > > Something like > > enum i3c_xfer_type { > I3C_CCC_XFER, > I3C_SDR_XFER, > I3C_HDR_XFER, > } > > struct i3c_xfer { > enum i3c_xfer_type type; > union { > struct i3c_ccc_cmd ccc; > struct i3c_priv_xfer sdr; > struct i3c_hdr_cmd hdr; > }; > } > > struct i3c_master_controller_ops { > ... > int (*i3c_xfers)(struct i3c_dev_desc *dev, > struct i3c_xfer *xfers, > int nxfers); > ... > }; I agree. This is something we should think of.
On 22/02/19 15:02, Przemyslaw Gaj wrote: >>>> struct i3c_ibi_payload { >>>> diff --git a/include/linux/i3c/master.h b/include/linux/i3c/master.h >>>> index ada956a..fd50473 100644 >>>> --- a/include/linux/i3c/master.h >>>> +++ b/include/linux/i3c/master.h >>>> @@ -386,6 +386,10 @@ struct i3c_bus { >>>> * This method is mandatory. >>>> * @priv_xfers: do one or several private I3C SDR transfers >>>> * This method is mandatory. >>>> + * @send_hdr_cmds: send one or several HDR commands. If there is more than one >>>> + * command, they should ideally be sent in the same HDR >>>> + * transaction. >>>> + * This method is optional. >>>> * @attach_i2c_dev: called every time an I2C device is attached to the bus. >>>> * This is a good place to attach master controller specific >>>> * data to I2C devices. >>>> @@ -457,6 +461,9 @@ struct i3c_master_controller_ops { >>>> int (*priv_xfers)(struct i3c_dev_desc *dev, >>>> struct i3c_priv_xfer *xfers, >>>> int nxfers); >>>> + int (*send_hdr_cmds)(struct i3c_dev_desc *dev, >>>> + const struct i3c_hdr_cmd *cmds, >>>> + int ncmds); >>>> 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, >>> With this approach the controller between a start and stop can only transmit in SDR or HDR. >>> >>> This is limited for devices that need the following frame: >>> <Start><SDR xfer><Repeated Start><HDR command><Stop> > Actually, there is repeated start between SDR and ENTHDR. There is no repeated > start between ENTHDR and HDR commands, HDR command has to start immedetly after > ENTHDR. Of course, hdr restart may occur between HDR commands. Sorry, I meant all HDR frame. >> If this is a use case we want to support, then we should probably have >> something more generic than what we currently have. >> >> Something like >> >> enum i3c_xfer_type { >> I3C_CCC_XFER, >> I3C_SDR_XFER, >> I3C_HDR_XFER, >> } >> >> struct i3c_xfer { >> enum i3c_xfer_type type; >> union { >> struct i3c_ccc_cmd ccc; >> struct i3c_priv_xfer sdr; >> struct i3c_hdr_cmd hdr; >> }; >> } @Boris: I remember you don't want to expose CCC commands, I think at least for now we shouldn't expose too. Do you agree? >> struct i3c_master_controller_ops { >> ... >> int (*i3c_xfers)(struct i3c_dev_desc *dev, >> struct i3c_xfer *xfers, >> int nxfers); >> ... >> }; > I agree. This is something we should think of. Best regards, Vitor Soares
Hi Vitor On Fri, 22 Feb 2019 17:41:34 +0000 vitor <vitor.soares@synopsys.com> wrote: > On 22/02/19 15:02, Przemyslaw Gaj wrote: > >>>> struct i3c_ibi_payload { > >>>> diff --git a/include/linux/i3c/master.h b/include/linux/i3c/master.h > >>>> index ada956a..fd50473 100644 > >>>> --- a/include/linux/i3c/master.h > >>>> +++ b/include/linux/i3c/master.h > >>>> @@ -386,6 +386,10 @@ struct i3c_bus { > >>>> * This method is mandatory. > >>>> * @priv_xfers: do one or several private I3C SDR transfers > >>>> * This method is mandatory. > >>>> + * @send_hdr_cmds: send one or several HDR commands. If there is more than one > >>>> + * command, they should ideally be sent in the same HDR > >>>> + * transaction. > >>>> + * This method is optional. > >>>> * @attach_i2c_dev: called every time an I2C device is attached to the bus. > >>>> * This is a good place to attach master controller specific > >>>> * data to I2C devices. > >>>> @@ -457,6 +461,9 @@ struct i3c_master_controller_ops { > >>>> int (*priv_xfers)(struct i3c_dev_desc *dev, > >>>> struct i3c_priv_xfer *xfers, > >>>> int nxfers); > >>>> + int (*send_hdr_cmds)(struct i3c_dev_desc *dev, > >>>> + const struct i3c_hdr_cmd *cmds, > >>>> + int ncmds); > >>>> 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, > >>> With this approach the controller between a start and stop can only transmit in SDR or HDR. > >>> > >>> This is limited for devices that need the following frame: > >>> <Start><SDR xfer><Repeated Start><HDR command><Stop> > > Actually, there is repeated start between SDR and ENTHDR. There is no repeated > > start between ENTHDR and HDR commands, HDR command has to start immedetly after > > ENTHDR. Of course, hdr restart may occur between HDR commands. > > Sorry, I meant all HDR frame. Then the proposed interface should work just fine, since we pass an array of HDR commands to execute. > > > >> If this is a use case we want to support, then we should probably have > >> something more generic than what we currently have. > >> > >> Something like > >> > >> enum i3c_xfer_type { > >> I3C_CCC_XFER, > >> I3C_SDR_XFER, > >> I3C_HDR_XFER, > >> } > >> > >> struct i3c_xfer { > >> enum i3c_xfer_type type; > >> union { > >> struct i3c_ccc_cmd ccc; > >> struct i3c_priv_xfer sdr; > >> struct i3c_hdr_cmd hdr; > >> }; > >> } > > @Boris: I remember you don't want to expose CCC commands, I think at least for now we shouldn't expose too. > > Do you agree? Well, if we want to have a generic interface and not force all drivers to implement one hook per transfer type, we should also have CCC commands in this enum. We can still make sure the user does not pass CCC commands before forwarding the request to the controller. Anyway, as for all the other changes proposed so far, I'd like to see a real use case for this SDR/HDR[/CCC] mix. Do you have a device spec describing such a sequence? Regards, Boris
On 25/02/19 08:56, Boris Brezillon wrote: > Hi Vitor > > On Fri, 22 Feb 2019 17:41:34 +0000 > vitor <vitor.soares@synopsys.com> wrote: > >> On 22/02/19 15:02, Przemyslaw Gaj wrote: >>>>>> struct i3c_ibi_payload { >>>>>> diff --git a/include/linux/i3c/master.h b/include/linux/i3c/master.h >>>>>> index ada956a..fd50473 100644 >>>>>> --- a/include/linux/i3c/master.h >>>>>> +++ b/include/linux/i3c/master.h >>>>>> @@ -386,6 +386,10 @@ struct i3c_bus { >>>>>> * This method is mandatory. >>>>>> * @priv_xfers: do one or several private I3C SDR transfers >>>>>> * This method is mandatory. >>>>>> + * @send_hdr_cmds: send one or several HDR commands. If there is more than one >>>>>> + * command, they should ideally be sent in the same HDR >>>>>> + * transaction. >>>>>> + * This method is optional. >>>>>> * @attach_i2c_dev: called every time an I2C device is attached to the bus. >>>>>> * This is a good place to attach master controller specific >>>>>> * data to I2C devices. >>>>>> @@ -457,6 +461,9 @@ struct i3c_master_controller_ops { >>>>>> int (*priv_xfers)(struct i3c_dev_desc *dev, >>>>>> struct i3c_priv_xfer *xfers, >>>>>> int nxfers); >>>>>> + int (*send_hdr_cmds)(struct i3c_dev_desc *dev, >>>>>> + const struct i3c_hdr_cmd *cmds, >>>>>> + int ncmds); >>>>>> 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, >>>>> With this approach the controller between a start and stop can only transmit in SDR or HDR. >>>>> >>>>> This is limited for devices that need the following frame: >>>>> <Start><SDR xfer><Repeated Start><HDR command><Stop> >>> Actually, there is repeated start between SDR and ENTHDR. There is no repeated >>> start between ENTHDR and HDR commands, HDR command has to start immedetly after >>> ENTHDR. Of course, hdr restart may occur between HDR commands. >> Sorry, I meant all HDR frame. > Then the proposed interface should work just fine, since we pass an > array of HDR commands to execute. I think it is not clear yet. When I referred <HDR command> I meant all HDR frame (ENTHDRx, HDR command, HDR CRC). >>>> If this is a use case we want to support, then we should probably have >>>> something more generic than what we currently have. >>>> >>>> Something like >>>> >>>> enum i3c_xfer_type { >>>> I3C_CCC_XFER, >>>> I3C_SDR_XFER, >>>> I3C_HDR_XFER, >>>> } >>>> >>>> struct i3c_xfer { >>>> enum i3c_xfer_type type; >>>> union { >>>> struct i3c_ccc_cmd ccc; >>>> struct i3c_priv_xfer sdr; >>>> struct i3c_hdr_cmd hdr; >>>> }; >>>> } >> @Boris: I remember you don't want to expose CCC commands, I think at least for now we shouldn't expose too. >> >> Do you agree? > Well, if we want to have a generic interface and not force all drivers > to implement one hook per transfer type, we should also have CCC > commands in this enum. We can still make sure the user does not pass > CCC commands before forwarding the request to the controller. I'm ok with this. Still there is some CCC commands that might be expose for devices and we can decide it later. > Anyway, as for all the other changes proposed so far, I'd like to see a > real use case for this SDR/HDR[/CCC] mix. Do you have a device spec > describing such a sequence? From my knowledge there is no devices HDR capable on the market thus I cannot argue with real use case. But per i3c spec v1.0 there is no mention that HDR frame shall begin with a START condition (not REPEATED START). So, the question is why do not support it instead of limiting it? We don't know the implementation of the device and some of them maybe need the previous transfer (in SDR) to do the next one (in HDR). and supporting it we are cover more use cases. > Regards, > > Boris Best regards, Vitor Soares
On Mon, 25 Feb 2019 12:56:35 +0000 vitor <vitor.soares@synopsys.com> wrote: > On 25/02/19 08:56, Boris Brezillon wrote: > > Hi Vitor > > > > On Fri, 22 Feb 2019 17:41:34 +0000 > > vitor <vitor.soares@synopsys.com> wrote: > > > >> On 22/02/19 15:02, Przemyslaw Gaj wrote: > >>>>>> struct i3c_ibi_payload { > >>>>>> diff --git a/include/linux/i3c/master.h b/include/linux/i3c/master.h > >>>>>> index ada956a..fd50473 100644 > >>>>>> --- a/include/linux/i3c/master.h > >>>>>> +++ b/include/linux/i3c/master.h > >>>>>> @@ -386,6 +386,10 @@ struct i3c_bus { > >>>>>> * This method is mandatory. > >>>>>> * @priv_xfers: do one or several private I3C SDR transfers > >>>>>> * This method is mandatory. > >>>>>> + * @send_hdr_cmds: send one or several HDR commands. If there is more than one > >>>>>> + * command, they should ideally be sent in the same HDR > >>>>>> + * transaction. > >>>>>> + * This method is optional. > >>>>>> * @attach_i2c_dev: called every time an I2C device is attached to the bus. > >>>>>> * This is a good place to attach master controller specific > >>>>>> * data to I2C devices. > >>>>>> @@ -457,6 +461,9 @@ struct i3c_master_controller_ops { > >>>>>> int (*priv_xfers)(struct i3c_dev_desc *dev, > >>>>>> struct i3c_priv_xfer *xfers, > >>>>>> int nxfers); > >>>>>> + int (*send_hdr_cmds)(struct i3c_dev_desc *dev, > >>>>>> + const struct i3c_hdr_cmd *cmds, > >>>>>> + int ncmds); > >>>>>> 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, > >>>>> With this approach the controller between a start and stop can only transmit in SDR or HDR. > >>>>> > >>>>> This is limited for devices that need the following frame: > >>>>> <Start><SDR xfer><Repeated Start><HDR command><Stop> > >>> Actually, there is repeated start between SDR and ENTHDR. There is no repeated > >>> start between ENTHDR and HDR commands, HDR command has to start immedetly after > >>> ENTHDR. Of course, hdr restart may occur between HDR commands. > >> Sorry, I meant all HDR frame. > > Then the proposed interface should work just fine, since we pass an > > array of HDR commands to execute. > > I think it is not clear yet. When I referred <HDR command> I meant all HDR frame (ENTHDRx, HDR command, HDR CRC). ENTHDRX and CRC is abstracted away. It's the master controller driver responsibility to enter HDR mode (using a CCC command), generate CRCs (either in HW or in SW) and add a HDRREPEAT after each HDR frame if needed (if we have more than one HDR command to send). > > >>>> If this is a use case we want to support, then we should probably have > >>>> something more generic than what we currently have. > >>>> > >>>> Something like > >>>> > >>>> enum i3c_xfer_type { > >>>> I3C_CCC_XFER, > >>>> I3C_SDR_XFER, > >>>> I3C_HDR_XFER, > >>>> } > >>>> > >>>> struct i3c_xfer { > >>>> enum i3c_xfer_type type; > >>>> union { > >>>> struct i3c_ccc_cmd ccc; > >>>> struct i3c_priv_xfer sdr; > >>>> struct i3c_hdr_cmd hdr; > >>>> }; > >>>> } > >> @Boris: I remember you don't want to expose CCC commands, I think at least for now we shouldn't expose too. > >> > >> Do you agree? > > Well, if we want to have a generic interface and not force all drivers > > to implement one hook per transfer type, we should also have CCC > > commands in this enum. We can still make sure the user does not pass > > CCC commands before forwarding the request to the controller. > > I'm ok with this. Still there is some CCC commands that might be expose for devices and we can decide it later. > > > Anyway, as for all the other changes proposed so far, I'd like to see a > > real use case for this SDR/HDR[/CCC] mix. Do you have a device spec > > describing such a sequence? > > From my knowledge there is no devices HDR capable on the market thus I cannot argue with real use case. > > But per i3c spec v1.0 there is no mention that HDR frame shall begin with a START condition (not REPEATED START). > So, the question is why do not support it instead of limiting it? Well, all depends on the extra complexity to support this case. If it's simple enough, I won't complain. > > We don't know the implementation of the device and some of them maybe need the previous transfer (in SDR) to do the next one (in HDR). Exactly, we don't know, so maybe we should just wait before introducing HDR mode... > > and supporting it we are cover more use cases. Are we even sure all controllers will be able to chain things like that? For instance, in it's non-DMA mode, the Cadence controller is limited by the FIFO size, which means you anyway won't be able to chain too much frames using a REPEATED START. So, any device relying on REPEATED START for atomic sequences is unlikely to work with all master controllers.
On 25/02/19 13:10, Boris Brezillon wrote: >>>>>> something more generic than what we currently have. >>>>>> >>>>>> Something like >>>>>> >>>>>> enum i3c_xfer_type { >>>>>> I3C_CCC_XFER, >>>>>> I3C_SDR_XFER, >>>>>> I3C_HDR_XFER, >>>>>> } >>>>>> >>>>>> struct i3c_xfer { >>>>>> enum i3c_xfer_type type; >>>>>> union { >>>>>> struct i3c_ccc_cmd ccc; >>>>>> struct i3c_priv_xfer sdr; >>>>>> struct i3c_hdr_cmd hdr; >>>>>> }; >>>>>> } >>>> @Boris: I remember you don't want to expose CCC commands, I think at least for now we shouldn't expose too. >>>> >>>> Do you agree? >>> Well, if we want to have a generic interface and not force all drivers >>> to implement one hook per transfer type, we should also have CCC >>> commands in this enum. We can still make sure the user does not pass >>> CCC commands before forwarding the request to the controller. >> I'm ok with this. Still there is some CCC commands that might be expose for devices and we can decide it later. >> >>> Anyway, as for all the other changes proposed so far, I'd like to see a >>> real use case for this SDR/HDR[/CCC] mix. Do you have a device spec >>> describing such a sequence? >> From my knowledge there is no devices HDR capable on the market thus I cannot argue with real use case. >> >> But per i3c spec v1.0 there is no mention that HDR frame shall begin with a START condition (not REPEATED START). >> So, the question is why do not support it instead of limiting it? > Well, all depends on the extra complexity to support this case. If it's > simple enough, I won't complain. > >> We don't know the implementation of the device and some of them maybe need the previous transfer (in SDR) to do the next one (in HDR). > Exactly, we don't know, so maybe we should just wait before introducing > HDR mode... > >> and supporting it we are cover more use cases. > Are we even sure all controllers will be able to chain things like > that? Why not? for the controllers shouldn't be a problem. The problem can be if it doesn't support. > For instance, in it's non-DMA mode, the Cadence controller is > limited by the FIFO size, which means you anyway won't be able to > chain too much frames using a REPEATED START. So, any device relying on > REPEATED START for atomic sequences is unlikely to work with all master > controllers. IMO this isn't up to us to decide, we just need to provide support and try to cover the most possible use cases. Right now we only have 2 masters to support this kind of change and I just see some effort to support the CCC commands. I would like to test this scenario, what do you think that can add extra complexity? Best regards, Vitor Soares
Hi Vitor, On Mon, 25 Feb 2019 16:45:21 +0000 vitor <vitor.soares@synopsys.com> wrote: > On 25/02/19 13:10, Boris Brezillon wrote: > >>>>>> something more generic than what we currently have. > >>>>>> > >>>>>> Something like > >>>>>> > >>>>>> enum i3c_xfer_type { > >>>>>> I3C_CCC_XFER, > >>>>>> I3C_SDR_XFER, > >>>>>> I3C_HDR_XFER, Forgot I3C_I2C_XFER, > >>>>>> } > >>>>>> > >>>>>> struct i3c_xfer { > >>>>>> enum i3c_xfer_type type; > >>>>>> union { > >>>>>> struct i3c_ccc_cmd ccc; > >>>>>> struct i3c_priv_xfer sdr; > >>>>>> struct i3c_hdr_cmd hdr; and struct i2c_msg i2c; Since you want to make that fully generic. > >>>>>> }; > >>>>>> } > >>>> @Boris: I remember you don't want to expose CCC commands, I think at least for now we shouldn't expose too. > >>>> > >>>> Do you agree? > >>> Well, if we want to have a generic interface and not force all drivers > >>> to implement one hook per transfer type, we should also have CCC > >>> commands in this enum. We can still make sure the user does not pass > >>> CCC commands before forwarding the request to the controller. > >> I'm ok with this. Still there is some CCC commands that might be expose for devices and we can decide it later. > >> > >>> Anyway, as for all the other changes proposed so far, I'd like to see a > >>> real use case for this SDR/HDR[/CCC] mix. Do you have a device spec > >>> describing such a sequence? > >> From my knowledge there is no devices HDR capable on the market thus I cannot argue with real use case. > >> > >> But per i3c spec v1.0 there is no mention that HDR frame shall begin with a START condition (not REPEATED START). > >> So, the question is why do not support it instead of limiting it? > > Well, all depends on the extra complexity to support this case. If it's > > simple enough, I won't complain. > > > >> We don't know the implementation of the device and some of them maybe need the previous transfer (in SDR) to do the next one (in HDR). > > Exactly, we don't know, so maybe we should just wait before introducing > > HDR mode... > > > >> and supporting it we are cover more use cases. > > Are we even sure all controllers will be able to chain things like > > that? > > Why not? for the controllers shouldn't be a problem. The problem can be if it doesn't support. I don't know, maybe because some HW will be too limited to support chained requests. I'm speculating, just like you are :-P. > > > For instance, in it's non-DMA mode, the Cadence controller is > > limited by the FIFO size, which means you anyway won't be able to > > chain too much frames using a REPEATED START. So, any device relying on > > REPEATED START for atomic sequences is unlikely to work with all master > > controllers. > > IMO this isn't up to us to decide, we just need to provide support and try to cover the most possible use cases. And yet, you take a decision based on your own speculations. You might possibly be right, but until we start seeing devices that support HDR modes that's not something we can be sure of. > Right now we only have 2 masters to support this kind of change and I just see some effort to support the CCC commands. Support CCC commands? We already support CCC commands. What do you mean? > > I would like to test this scenario, what do you think that can add extra complexity? Go ahead and post an implementation, we'll see how it looks like. BTW, you have quite a few things on your TODO list already, but none of these have led to patches being posted. Maybe you should focus on one topic and finish it instead of constantly bringing new things you say you plan to work on. Regards, Boris
diff --git a/drivers/i3c/device.c b/drivers/i3c/device.c index 69cc040..97910aa 100644 --- a/drivers/i3c/device.c +++ b/drivers/i3c/device.c @@ -51,6 +51,43 @@ int i3c_device_do_priv_xfers(struct i3c_device *dev, EXPORT_SYMBOL_GPL(i3c_device_do_priv_xfers); /** + * i3c_device_send_hdr_cmds() - send HDR commands to a specific device + * + * @dev: device to which these commands should be sent + * @xfers: array of commands + * @nxfers: number of commands + * + * Send one or several HDR commands to @dev. + * + * This function can sleep and thus cannot be called in atomic context. + * + * Return: 0 in case of success, a negative error core otherwise. + */ +int i3c_device_send_hdr_cmds(struct i3c_device *dev, + struct i3c_hdr_cmd *cmds, + int ncmds) +{ + int ret, i; + enum i3c_hdr_mode mode; + + if (ncmds < 1) + return 0; + + mode = cmds[0].mode; + for (i = 1; i < ncmds; i++) { + if (mode != cmds[i].mode) + return -EINVAL; + } + + i3c_bus_normaluse_lock(dev->bus); + ret = i3c_dev_send_hdr_cmds_locked(dev->desc, cmds, ncmds); + i3c_bus_normaluse_unlock(dev->bus); + + return ret; +} +EXPORT_SYMBOL_GPL(i3c_device_send_hdr_cmds); + +/** * i3c_device_get_info() - get I3C device information * * @dev: device we want information on diff --git a/drivers/i3c/internals.h b/drivers/i3c/internals.h index 86b7b44..46c4de7 100644 --- a/drivers/i3c/internals.h +++ b/drivers/i3c/internals.h @@ -18,6 +18,9 @@ void i3c_bus_normaluse_unlock(struct i3c_bus *bus); int i3c_dev_do_priv_xfers_locked(struct i3c_dev_desc *dev, struct i3c_priv_xfer *xfers, int nxfers); +int i3c_dev_send_hdr_cmds_locked(struct i3c_dev_desc *dev, + struct i3c_hdr_cmd *cmds, + int ncmds); 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 e98b600..16d6dd5 100644 --- a/drivers/i3c/master.c +++ b/drivers/i3c/master.c @@ -2791,6 +2791,37 @@ int i3c_dev_do_priv_xfers_locked(struct i3c_dev_desc *dev, return master->ops->priv_xfers(dev, xfers, nxfers); } +int i3c_dev_send_hdr_cmds_locked(struct i3c_dev_desc *dev, + struct i3c_hdr_cmd *cmds, + int ncmds) +{ + struct i3c_master_controller *master; + int i; + + if (!dev) + return -ENOENT; + + master = i3c_dev_get_master(dev); + if (!master || !cmds) + return -EINVAL; + + if (master->op_mode == I3C_SLAVE_MODE) { + if (i3c_master_request_mastership(master)) + return -EIO; + } + + if (!master->ops->send_hdr_cmds) + return -ENOTSUPP; + + for (i = 0; i < ncmds; i++) { + if (!(master->this->info.hdr_cap & BIT(cmds->mode))) + return -ENOTSUPP; + } + + return master->ops->send_hdr_cmds(dev, cmds, ncmds); +} + + int i3c_dev_disable_ibi_locked(struct i3c_dev_desc *dev) { struct i3c_master_controller *master; diff --git a/include/linux/i3c/device.h b/include/linux/i3c/device.h index 5ecb055..75a947f 100644 --- a/include/linux/i3c/device.h +++ b/include/linux/i3c/device.h @@ -49,6 +49,31 @@ enum i3c_hdr_mode { I3C_HDR_TSL, }; +#define I3C_HDR_GEN_WRITE_CMD(id) (id) +#define I3C_HDR_VENDOR_WRITE_CMD(id) (0x20 + (id)) +#define I3C_HDR_IS_READ_CMD BIT(7) +#define I3C_HDR_GEN_READ_CMD(id) (0x80 + (id)) +#define I3C_HDR_VENDOR_READ_CMD(id) (0xa0 + (id)) + +/** + * struct i3c_hdr_cmd - I3C HDR command + * @mode: HDR mode selected for this command + * @code: command opcode. Bit 7 encodes the direction of the data transfer, if + * set this is a read, otherwise this is a write + * @ndatawords: number of data words (a word is 16bits wide) to transfer + * @data: input/output buffer + */ +struct i3c_hdr_cmd { + enum i3c_hdr_mode mode; + u8 code; + int ndatawords; + union { + u16 *in; + const u16 *out; + } data; +}; + + /** * struct i3c_priv_xfer - I3C SDR private transfer * @rnw: encodes the transfer direction. true for a read, false for a write @@ -289,6 +314,10 @@ int i3c_device_do_priv_xfers(struct i3c_device *dev, struct i3c_priv_xfer *xfers, int nxfers); +int i3c_device_send_hdr_cmds(struct i3c_device *dev, + struct i3c_hdr_cmd *cmds, + int ncmds); + void i3c_device_get_info(struct i3c_device *dev, struct i3c_device_info *info); struct i3c_ibi_payload { diff --git a/include/linux/i3c/master.h b/include/linux/i3c/master.h index ada956a..fd50473 100644 --- a/include/linux/i3c/master.h +++ b/include/linux/i3c/master.h @@ -386,6 +386,10 @@ struct i3c_bus { * This method is mandatory. * @priv_xfers: do one or several private I3C SDR transfers * This method is mandatory. + * @send_hdr_cmds: send one or several HDR commands. If there is more than one + * command, they should ideally be sent in the same HDR + * transaction. + * This method is optional. * @attach_i2c_dev: called every time an I2C device is attached to the bus. * This is a good place to attach master controller specific * data to I2C devices. @@ -457,6 +461,9 @@ struct i3c_master_controller_ops { int (*priv_xfers)(struct i3c_dev_desc *dev, struct i3c_priv_xfer *xfers, int nxfers); + int (*send_hdr_cmds)(struct i3c_dev_desc *dev, + const struct i3c_hdr_cmd *cmds, + int ncmds); 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,
HDR (High Data Rate) modes is an important feature of the I3C protocol as it allows to get higher throughput than with the SDR (Single Data Rate) mode. Add new controller hooks and extend the I3C device API to expose this new feature. This feature was originally created by Boris Brezillon <boris.brezillon@bootlin.com>. Signed-off-by: Przemyslaw Gaj <pgaj@cadence.com> --- drivers/i3c/device.c | 37 +++++++++++++++++++++++++++++++++++++ drivers/i3c/internals.h | 3 +++ drivers/i3c/master.c | 31 +++++++++++++++++++++++++++++++ include/linux/i3c/device.h | 29 +++++++++++++++++++++++++++++ include/linux/i3c/master.h | 7 +++++++ 5 files changed, 107 insertions(+)