* [PATCH 0/2] Add the I3C HDR modes @ 2018-12-13 12:18 Przemyslaw Gaj 2018-12-13 12:18 ` [PATCH 1/2] i3c: Add support for " Przemyslaw Gaj 2018-12-13 12:18 ` [PATCH 2/2] i3c: master: cdns: Add support for HDR-DDR mode Przemyslaw Gaj 0 siblings, 2 replies; 14+ messages in thread From: Przemyslaw Gaj @ 2018-12-13 12:18 UTC (permalink / raw) To: bbrezillon, linux-i3c; +Cc: Przemyslaw Gaj, psroka, rafalc, vitor.soares This patch series adds support for HDR modes to the I3C subsystem and Cadence I3C master driver. Przemyslaw Gaj (2): i3c: Add support for HDR modes. i3c: master: cdns: Add support for HDR-DDR mode drivers/i3c/device.c | 37 +++++++ drivers/i3c/internals.h | 3 + drivers/i3c/master.c | 31 ++++++ drivers/i3c/master/i3c-master-cdns.c | 195 ++++++++++++++++++++++++++++++++++- include/linux/i3c/device.h | 29 ++++++ include/linux/i3c/master.h | 7 ++ 6 files changed, 300 insertions(+), 2 deletions(-) -- 2.4.5 _______________________________________________ linux-i3c mailing list linux-i3c@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-i3c ^ permalink raw reply [flat|nested] 14+ messages in thread
* [PATCH 1/2] i3c: Add support for HDR modes. 2018-12-13 12:18 [PATCH 0/2] Add the I3C HDR modes Przemyslaw Gaj @ 2018-12-13 12:18 ` Przemyslaw Gaj 2018-12-13 12:44 ` Boris Brezillon 2019-02-21 15:15 ` vitor 2018-12-13 12:18 ` [PATCH 2/2] i3c: master: cdns: Add support for HDR-DDR mode Przemyslaw Gaj 1 sibling, 2 replies; 14+ messages in thread From: Przemyslaw Gaj @ 2018-12-13 12:18 UTC (permalink / raw) To: bbrezillon, linux-i3c; +Cc: Przemyslaw Gaj, psroka, rafalc, vitor.soares 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; + } + + 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, -- 2.4.5 _______________________________________________ linux-i3c mailing list linux-i3c@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-i3c ^ permalink raw reply related [flat|nested] 14+ messages in thread
* Re: [PATCH 1/2] i3c: Add support for HDR modes. 2018-12-13 12:18 ` [PATCH 1/2] i3c: Add support for " Przemyslaw Gaj @ 2018-12-13 12:44 ` Boris Brezillon 2019-02-21 15:15 ` vitor 1 sibling, 0 replies; 14+ messages in thread From: Boris Brezillon @ 2018-12-13 12:44 UTC (permalink / raw) To: Przemyslaw Gaj; +Cc: linux-i3c, psroka, rafalc, vitor.soares 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, _______________________________________________ linux-i3c mailing list linux-i3c@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-i3c ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 1/2] i3c: Add support for HDR modes. 2018-12-13 12:18 ` [PATCH 1/2] i3c: Add support for " Przemyslaw Gaj 2018-12-13 12:44 ` Boris Brezillon @ 2019-02-21 15:15 ` vitor 2019-02-22 14:52 ` Boris Brezillon 1 sibling, 1 reply; 14+ messages in thread From: vitor @ 2019-02-21 15:15 UTC (permalink / raw) To: Przemyslaw Gaj, bbrezillon, linux-i3c; +Cc: psroka, rafalc, vitor.soares 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 _______________________________________________ linux-i3c mailing list linux-i3c@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-i3c ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 1/2] i3c: Add support for HDR modes. 2019-02-21 15:15 ` vitor @ 2019-02-22 14:52 ` Boris Brezillon 2019-02-22 15:02 ` Przemyslaw Gaj 0 siblings, 1 reply; 14+ messages in thread From: Boris Brezillon @ 2019-02-22 14:52 UTC (permalink / raw) To: vitor; +Cc: Przemyslaw Gaj, psroka, linux-i3c, rafalc 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); ... }; _______________________________________________ linux-i3c mailing list linux-i3c@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-i3c ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 1/2] i3c: Add support for HDR modes. 2019-02-22 14:52 ` Boris Brezillon @ 2019-02-22 15:02 ` Przemyslaw Gaj 2019-02-22 17:41 ` vitor 0 siblings, 1 reply; 14+ messages in thread From: Przemyslaw Gaj @ 2019-02-22 15:02 UTC (permalink / raw) To: Boris Brezillon; +Cc: linux-i3c, psroka, rafalc, vitor 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. -- -- Przemyslaw Gaj _______________________________________________ linux-i3c mailing list linux-i3c@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-i3c ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 1/2] i3c: Add support for HDR modes. 2019-02-22 15:02 ` Przemyslaw Gaj @ 2019-02-22 17:41 ` vitor 2019-02-25 8:56 ` Boris Brezillon 0 siblings, 1 reply; 14+ messages in thread From: vitor @ 2019-02-22 17:41 UTC (permalink / raw) To: Przemyslaw Gaj, Boris Brezillon; +Cc: linux-i3c, psroka, rafalc, vitor 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 _______________________________________________ linux-i3c mailing list linux-i3c@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-i3c ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 1/2] i3c: Add support for HDR modes. 2019-02-22 17:41 ` vitor @ 2019-02-25 8:56 ` Boris Brezillon 2019-02-25 12:56 ` vitor 0 siblings, 1 reply; 14+ messages in thread From: Boris Brezillon @ 2019-02-25 8:56 UTC (permalink / raw) To: vitor; +Cc: Przemyslaw Gaj, psroka, linux-i3c, rafalc 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 _______________________________________________ linux-i3c mailing list linux-i3c@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-i3c ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 1/2] i3c: Add support for HDR modes. 2019-02-25 8:56 ` Boris Brezillon @ 2019-02-25 12:56 ` vitor 2019-02-25 13:10 ` Boris Brezillon 0 siblings, 1 reply; 14+ messages in thread From: vitor @ 2019-02-25 12:56 UTC (permalink / raw) To: Boris Brezillon, vitor; +Cc: Przemyslaw Gaj, psroka, linux-i3c, rafalc 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 _______________________________________________ linux-i3c mailing list linux-i3c@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-i3c ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 1/2] i3c: Add support for HDR modes. 2019-02-25 12:56 ` vitor @ 2019-02-25 13:10 ` Boris Brezillon 2019-02-25 16:45 ` vitor 0 siblings, 1 reply; 14+ messages in thread From: Boris Brezillon @ 2019-02-25 13:10 UTC (permalink / raw) To: vitor; +Cc: Przemyslaw Gaj, psroka, linux-i3c, rafalc 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. _______________________________________________ linux-i3c mailing list linux-i3c@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-i3c ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 1/2] i3c: Add support for HDR modes. 2019-02-25 13:10 ` Boris Brezillon @ 2019-02-25 16:45 ` vitor 2019-02-25 17:03 ` Boris Brezillon 0 siblings, 1 reply; 14+ messages in thread From: vitor @ 2019-02-25 16:45 UTC (permalink / raw) To: Boris Brezillon, vitor; +Cc: Przemyslaw Gaj, psroka, linux-i3c, rafalc 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 _______________________________________________ linux-i3c mailing list linux-i3c@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-i3c ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 1/2] i3c: Add support for HDR modes. 2019-02-25 16:45 ` vitor @ 2019-02-25 17:03 ` Boris Brezillon 0 siblings, 0 replies; 14+ messages in thread From: Boris Brezillon @ 2019-02-25 17:03 UTC (permalink / raw) To: vitor; +Cc: Przemyslaw Gaj, psroka, linux-i3c, rafalc 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 _______________________________________________ linux-i3c mailing list linux-i3c@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-i3c ^ permalink raw reply [flat|nested] 14+ messages in thread
* [PATCH 2/2] i3c: master: cdns: Add support for HDR-DDR mode 2018-12-13 12:18 [PATCH 0/2] Add the I3C HDR modes Przemyslaw Gaj 2018-12-13 12:18 ` [PATCH 1/2] i3c: Add support for " Przemyslaw Gaj @ 2018-12-13 12:18 ` Przemyslaw Gaj 2018-12-13 12:45 ` Boris Brezillon 1 sibling, 1 reply; 14+ messages in thread From: Przemyslaw Gaj @ 2018-12-13 12:18 UTC (permalink / raw) To: bbrezillon, linux-i3c; +Cc: Przemyslaw Gaj, psroka, rafalc, vitor.soares Cadence I3C master controller HDR-DDR mode support. This feature was originally created by Boris Brezillon <boris.brezillon@bootlin.com>. I made some changes/fixes. Signed-off-by: Przemyslaw Gaj <pgaj@cadence.com> --- drivers/i3c/master/i3c-master-cdns.c | 195 ++++++++++++++++++++++++++++++++++- 1 file changed, 193 insertions(+), 2 deletions(-) diff --git a/drivers/i3c/master/i3c-master-cdns.c b/drivers/i3c/master/i3c-master-cdns.c index a33f3a6..b1a97be 100644 --- a/drivers/i3c/master/i3c-master-cdns.c +++ b/drivers/i3c/master/i3c-master-cdns.c @@ -571,7 +571,7 @@ static void cdns_i3c_master_end_xfer_locked(struct cdns_i3c_master *master, !(status0 & MST_STATUS0_CMDR_EMP); status0 = readl(master->regs + MST_STATUS0)) { struct cdns_i3c_cmd *cmd; - u32 cmdr, rx_len, id; + u32 cmdr, rx_len, id, xfer_bytes; cmdr = readl(master->regs + CMDR); id = CMDR_CMDID(cmdr); @@ -581,7 +581,11 @@ static void cdns_i3c_master_end_xfer_locked(struct cdns_i3c_master *master, continue; cmd = &xfer->cmds[CMDR_CMDID(cmdr)]; - rx_len = min_t(u32, CMDR_XFER_BYTES(cmdr), cmd->rx_len); + xfer_bytes = CMDR_XFER_BYTES(cmdr); + if(cmd->cmd0 & CMD0_FIFO_IS_DDR) + xfer_bytes = xfer_bytes * 4; + rx_len = min_t(u32, xfer_bytes, cmd->rx_len); + cdns_i3c_master_rd_from_rx_fifo(master, cmd->rx_buf, rx_len); cmd->error = CMDR_ERROR(cmdr); } @@ -893,6 +897,192 @@ static int cdns_i3c_master_priv_xfers(struct i3c_dev_desc *dev, return ret; } +#define I3C_DDR_FIRST_DATA_WORD_PREAMBLE 0x2 +#define I3C_DDR_DATA_WORD_PREAMBLE 0x3 + +#define I3C_DDR_PREAMBLE(p) ((p) << 18) + +static u32 prepare_ddr_word(u16 payload) +{ + u32 ret; + u16 pb; + + ret = (u32)payload << 2; + + /* Calculate parity. */ + pb = (payload >> 15) ^ (payload >> 13) ^ (payload >> 11) ^ + (payload >> 9) ^ (payload >> 7) ^ (payload >> 5) ^ + (payload >> 3) ^ (payload >> 1); + ret |= (pb & 1) << 1; + pb = (payload >> 14) ^ (payload >> 12) ^ (payload >> 10) ^ + (payload >> 8) ^ (payload >> 6) ^ (payload >> 4) ^ + (payload >> 2) ^ payload ^ 1; + ret |= (pb & 1); + + return ret; +} + +static u32 prepare_ddr_data_word(u16 data, bool first) +{ + return prepare_ddr_word(data) | + I3C_DDR_PREAMBLE(first ? + I3C_DDR_FIRST_DATA_WORD_PREAMBLE : + I3C_DDR_DATA_WORD_PREAMBLE); +} + +#define I3C_DDR_READ_CMD BIT(15) + +static u32 prepare_ddr_cmd_word(u16 cmd) +{ + return prepare_ddr_word(cmd) | I3C_DDR_PREAMBLE(1); +} + +static u32 prepare_ddr_crc_word(u8 crc5) +{ + return (((u32)crc5 & 0x1f) << 9) | (0xc << 14) | + I3C_DDR_PREAMBLE(1); +} + +static u32 prepare_ddr_parity_bit(u32 cmdword) +{ + u16 pb; + + pb = (cmdword >> 14) ^ (cmdword >> 12) ^ (cmdword >> 10) ^ + (cmdword >> 8) ^ (cmdword >> 6) ^ (cmdword >> 4) ^ + (cmdword >> 2); + + if (pb & 1) + cmdword |= BIT(0); + + return cmdword; +} + +static u8 update_crc5(u8 crc5, u16 word) +{ + u8 crc0; + int i; + + /* + * crc0 = next_data_bit ^ crc[4] + * 1 2 3 4 + * crc[4:0] = { crc[3:2], crc[1]^crc0, crc[0], crc0 } + */ + for (i = 15; i >= 0; --i) { + crc0 = ((word >> i) ^ (crc5 >> 4)) & 0x1; + crc5 = ((crc5 << 1) & 0x1a) | + (((crc5 >> 1) ^ crc0) << 2) | + crc0; + } + + return crc5 & 0x1f; +} + +static int cdns_i3c_master_send_hdr_cmd(struct i3c_dev_desc *dev, + const struct i3c_hdr_cmd *cmds, + int ncmds) +{ + struct i3c_master_controller *m = i3c_dev_get_master(dev); + struct cdns_i3c_master *master = to_cdns_i3c_master(m); + int ret, i, ntxwords = 1, nrxwords = 0; + struct cdns_i3c_xfer *xfer; + struct cdns_i3c_cmd *ccmd; + u16 cmdword, datain; + u32 checkword, word; + u32 *buf = NULL; + u8 crc5; + + if (ncmds < 1) + return 0; + + if (ncmds > 1 || cmds[0].ndatawords > CMD0_FIFO_PL_LEN_MAX) + return -ENOTSUPP; + + if (cmds[0].mode != I3C_HDR_DDR) + return -ENOTSUPP; + + cmdword = ((u16)cmds[0].code << 8) | (dev->info.dyn_addr << 1); + if (cmdword & I3C_DDR_READ_CMD) + nrxwords += cmds[0].ndatawords + 1; + else + ntxwords += cmds[0].ndatawords + 1; + + if (ntxwords > master->caps.txfifodepth || + nrxwords > master->caps.rxfifodepth) + return -ENOTSUPP; + + buf = kzalloc((nrxwords + ntxwords) * sizeof(*buf), GFP_KERNEL); + if (!buf) + return -ENOMEM; + + xfer = cdns_i3c_master_alloc_xfer(master, 2); + if (!xfer) { + ret = -ENOMEM; + goto out_free_buf; + } + + ccmd = &xfer->cmds[0]; + ccmd->cmd1 = CMD1_FIFO_CCC(I3C_CCC_ENTHDR(0)); + ccmd->cmd0 = CMD0_FIFO_IS_CCC; + + ccmd = &xfer->cmds[1]; + + if (cmdword & I3C_DDR_READ_CMD) + cmdword = prepare_ddr_parity_bit(cmdword); + + ccmd->tx_len = ntxwords * sizeof(u32); + ccmd->tx_buf = buf; + ccmd->rx_len = nrxwords * sizeof(u32); + ccmd->rx_buf = buf + ntxwords; + + buf[0] = prepare_ddr_cmd_word(cmdword); + crc5 = update_crc5(0x1f, cmdword); + for (i = 0; i < ntxwords - 2; i++) { + crc5 = update_crc5(crc5, cmds[0].data.out[i]); + buf[i + 1] = prepare_ddr_data_word(cmds[0].data.out[i], !i); + } + + if(!(cmdword & I3C_DDR_READ_CMD)) + buf[ntxwords-1] = prepare_ddr_crc_word(crc5); + + ccmd->cmd0 = CMD0_FIFO_IS_DDR | CMD0_FIFO_PL_LEN(ntxwords); + + cdns_i3c_master_queue_xfer(master, xfer); + if (!wait_for_completion_timeout(&xfer->comp, msecs_to_jiffies(1000))) + cdns_i3c_master_unqueue_xfer(master, xfer); + + ret = xfer->ret; + + if (!xfer->ret && nrxwords) { + for (i = 0; i < nrxwords - 1; i++) { + word = (((u32 *)ccmd->rx_buf)[i] & GENMASK(19, 0)); + datain = (word >> 2) & GENMASK(15, 0); + checkword = prepare_ddr_data_word(datain, !i); + + if (checkword != word) { + ret = -EIO; + break; + } + + crc5 = update_crc5(crc5, datain); + cmds[0].data.in[i] = datain; + } + + word = (((u32 *)ccmd->rx_buf)[i] & GENMASK(19, 9)); + datain = (word >> 2) & GENMASK(15, 0); + checkword = prepare_ddr_crc_word(crc5); + + if (checkword != word) + ret = -EIO; + } + + cdns_i3c_master_free_xfer(xfer); + +out_free_buf: + kfree(buf); + + return ret; +} + static int cdns_i3c_master_i2c_xfers(struct i2c_dev_desc *dev, const struct i2c_msg *xfers, int nxfers) { @@ -1714,6 +1904,7 @@ static const struct i3c_master_controller_ops cdns_i3c_master_ops = { .supports_ccc_cmd = cdns_i3c_master_supports_ccc_cmd, .send_ccc_cmd = cdns_i3c_master_send_ccc_cmd, .priv_xfers = cdns_i3c_master_priv_xfers, + .send_hdr_cmds = cdns_i3c_master_send_hdr_cmd, .i2c_xfers = cdns_i3c_master_i2c_xfers, .i2c_funcs = cdns_i3c_master_i2c_funcs, .enable_ibi = cdns_i3c_master_enable_ibi, -- 2.4.5 _______________________________________________ linux-i3c mailing list linux-i3c@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-i3c ^ permalink raw reply related [flat|nested] 14+ messages in thread
* Re: [PATCH 2/2] i3c: master: cdns: Add support for HDR-DDR mode 2018-12-13 12:18 ` [PATCH 2/2] i3c: master: cdns: Add support for HDR-DDR mode Przemyslaw Gaj @ 2018-12-13 12:45 ` Boris Brezillon 0 siblings, 0 replies; 14+ messages in thread From: Boris Brezillon @ 2018-12-13 12:45 UTC (permalink / raw) To: Przemyslaw Gaj; +Cc: linux-i3c, psroka, rafalc, vitor.soares On Thu, 13 Dec 2018 12:18:32 +0000 Przemyslaw Gaj <pgaj@cadence.com> wrote: > Cadence I3C master controller HDR-DDR mode support. > > This feature was originally created by Boris Brezillon > <boris.brezillon@bootlin.com>. I made some changes/fixes. Same here. > > Signed-off-by: Przemyslaw Gaj <pgaj@cadence.com> > --- > drivers/i3c/master/i3c-master-cdns.c | 195 ++++++++++++++++++++++++++++++++++- > 1 file changed, 193 insertions(+), 2 deletions(-) > > diff --git a/drivers/i3c/master/i3c-master-cdns.c b/drivers/i3c/master/i3c-master-cdns.c > index a33f3a6..b1a97be 100644 > --- a/drivers/i3c/master/i3c-master-cdns.c > +++ b/drivers/i3c/master/i3c-master-cdns.c > @@ -571,7 +571,7 @@ static void cdns_i3c_master_end_xfer_locked(struct cdns_i3c_master *master, > !(status0 & MST_STATUS0_CMDR_EMP); > status0 = readl(master->regs + MST_STATUS0)) { > struct cdns_i3c_cmd *cmd; > - u32 cmdr, rx_len, id; > + u32 cmdr, rx_len, id, xfer_bytes; > > cmdr = readl(master->regs + CMDR); > id = CMDR_CMDID(cmdr); > @@ -581,7 +581,11 @@ static void cdns_i3c_master_end_xfer_locked(struct cdns_i3c_master *master, > continue; > > cmd = &xfer->cmds[CMDR_CMDID(cmdr)]; > - rx_len = min_t(u32, CMDR_XFER_BYTES(cmdr), cmd->rx_len); > + xfer_bytes = CMDR_XFER_BYTES(cmdr); > + if(cmd->cmd0 & CMD0_FIFO_IS_DDR) > + xfer_bytes = xfer_bytes * 4; > + rx_len = min_t(u32, xfer_bytes, cmd->rx_len); > + > cdns_i3c_master_rd_from_rx_fifo(master, cmd->rx_buf, rx_len); > cmd->error = CMDR_ERROR(cmdr); > } > @@ -893,6 +897,192 @@ static int cdns_i3c_master_priv_xfers(struct i3c_dev_desc *dev, > return ret; > } > > +#define I3C_DDR_FIRST_DATA_WORD_PREAMBLE 0x2 > +#define I3C_DDR_DATA_WORD_PREAMBLE 0x3 > + > +#define I3C_DDR_PREAMBLE(p) ((p) << 18) > + > +static u32 prepare_ddr_word(u16 payload) > +{ > + u32 ret; > + u16 pb; > + > + ret = (u32)payload << 2; > + > + /* Calculate parity. */ > + pb = (payload >> 15) ^ (payload >> 13) ^ (payload >> 11) ^ > + (payload >> 9) ^ (payload >> 7) ^ (payload >> 5) ^ > + (payload >> 3) ^ (payload >> 1); > + ret |= (pb & 1) << 1; > + pb = (payload >> 14) ^ (payload >> 12) ^ (payload >> 10) ^ > + (payload >> 8) ^ (payload >> 6) ^ (payload >> 4) ^ > + (payload >> 2) ^ payload ^ 1; > + ret |= (pb & 1); > + > + return ret; > +} > + > +static u32 prepare_ddr_data_word(u16 data, bool first) > +{ > + return prepare_ddr_word(data) | > + I3C_DDR_PREAMBLE(first ? > + I3C_DDR_FIRST_DATA_WORD_PREAMBLE : > + I3C_DDR_DATA_WORD_PREAMBLE); > +} > + > +#define I3C_DDR_READ_CMD BIT(15) > + > +static u32 prepare_ddr_cmd_word(u16 cmd) > +{ > + return prepare_ddr_word(cmd) | I3C_DDR_PREAMBLE(1); > +} > + > +static u32 prepare_ddr_crc_word(u8 crc5) > +{ > + return (((u32)crc5 & 0x1f) << 9) | (0xc << 14) | > + I3C_DDR_PREAMBLE(1); > +} > + > +static u32 prepare_ddr_parity_bit(u32 cmdword) > +{ > + u16 pb; > + > + pb = (cmdword >> 14) ^ (cmdword >> 12) ^ (cmdword >> 10) ^ > + (cmdword >> 8) ^ (cmdword >> 6) ^ (cmdword >> 4) ^ > + (cmdword >> 2); > + > + if (pb & 1) > + cmdword |= BIT(0); > + > + return cmdword; > +} > + > +static u8 update_crc5(u8 crc5, u16 word) > +{ > + u8 crc0; > + int i; > + > + /* > + * crc0 = next_data_bit ^ crc[4] > + * 1 2 3 4 > + * crc[4:0] = { crc[3:2], crc[1]^crc0, crc[0], crc0 } > + */ > + for (i = 15; i >= 0; --i) { > + crc0 = ((word >> i) ^ (crc5 >> 4)) & 0x1; > + crc5 = ((crc5 << 1) & 0x1a) | > + (((crc5 >> 1) ^ crc0) << 2) | > + crc0; > + } > + > + return crc5 & 0x1f; > +} > + > +static int cdns_i3c_master_send_hdr_cmd(struct i3c_dev_desc *dev, > + const struct i3c_hdr_cmd *cmds, > + int ncmds) > +{ > + struct i3c_master_controller *m = i3c_dev_get_master(dev); > + struct cdns_i3c_master *master = to_cdns_i3c_master(m); > + int ret, i, ntxwords = 1, nrxwords = 0; > + struct cdns_i3c_xfer *xfer; > + struct cdns_i3c_cmd *ccmd; > + u16 cmdword, datain; > + u32 checkword, word; > + u32 *buf = NULL; > + u8 crc5; > + > + if (ncmds < 1) > + return 0; > + > + if (ncmds > 1 || cmds[0].ndatawords > CMD0_FIFO_PL_LEN_MAX) > + return -ENOTSUPP; > + > + if (cmds[0].mode != I3C_HDR_DDR) > + return -ENOTSUPP; > + > + cmdword = ((u16)cmds[0].code << 8) | (dev->info.dyn_addr << 1); > + if (cmdword & I3C_DDR_READ_CMD) > + nrxwords += cmds[0].ndatawords + 1; > + else > + ntxwords += cmds[0].ndatawords + 1; > + > + if (ntxwords > master->caps.txfifodepth || > + nrxwords > master->caps.rxfifodepth) > + return -ENOTSUPP; > + > + buf = kzalloc((nrxwords + ntxwords) * sizeof(*buf), GFP_KERNEL); > + if (!buf) > + return -ENOMEM; > + > + xfer = cdns_i3c_master_alloc_xfer(master, 2); > + if (!xfer) { > + ret = -ENOMEM; > + goto out_free_buf; > + } > + > + ccmd = &xfer->cmds[0]; > + ccmd->cmd1 = CMD1_FIFO_CCC(I3C_CCC_ENTHDR(0)); > + ccmd->cmd0 = CMD0_FIFO_IS_CCC; > + > + ccmd = &xfer->cmds[1]; > + > + if (cmdword & I3C_DDR_READ_CMD) > + cmdword = prepare_ddr_parity_bit(cmdword); > + > + ccmd->tx_len = ntxwords * sizeof(u32); > + ccmd->tx_buf = buf; > + ccmd->rx_len = nrxwords * sizeof(u32); > + ccmd->rx_buf = buf + ntxwords; > + > + buf[0] = prepare_ddr_cmd_word(cmdword); > + crc5 = update_crc5(0x1f, cmdword); > + for (i = 0; i < ntxwords - 2; i++) { > + crc5 = update_crc5(crc5, cmds[0].data.out[i]); > + buf[i + 1] = prepare_ddr_data_word(cmds[0].data.out[i], !i); > + } > + > + if(!(cmdword & I3C_DDR_READ_CMD)) > + buf[ntxwords-1] = prepare_ddr_crc_word(crc5); > + > + ccmd->cmd0 = CMD0_FIFO_IS_DDR | CMD0_FIFO_PL_LEN(ntxwords); > + > + cdns_i3c_master_queue_xfer(master, xfer); > + if (!wait_for_completion_timeout(&xfer->comp, msecs_to_jiffies(1000))) > + cdns_i3c_master_unqueue_xfer(master, xfer); > + > + ret = xfer->ret; > + > + if (!xfer->ret && nrxwords) { > + for (i = 0; i < nrxwords - 1; i++) { > + word = (((u32 *)ccmd->rx_buf)[i] & GENMASK(19, 0)); > + datain = (word >> 2) & GENMASK(15, 0); > + checkword = prepare_ddr_data_word(datain, !i); > + > + if (checkword != word) { > + ret = -EIO; > + break; > + } > + > + crc5 = update_crc5(crc5, datain); > + cmds[0].data.in[i] = datain; > + } > + > + word = (((u32 *)ccmd->rx_buf)[i] & GENMASK(19, 9)); > + datain = (word >> 2) & GENMASK(15, 0); > + checkword = prepare_ddr_crc_word(crc5); > + > + if (checkword != word) > + ret = -EIO; > + } > + > + cdns_i3c_master_free_xfer(xfer); > + > +out_free_buf: > + kfree(buf); > + > + return ret; > +} > + > static int cdns_i3c_master_i2c_xfers(struct i2c_dev_desc *dev, > const struct i2c_msg *xfers, int nxfers) > { > @@ -1714,6 +1904,7 @@ static const struct i3c_master_controller_ops cdns_i3c_master_ops = { > .supports_ccc_cmd = cdns_i3c_master_supports_ccc_cmd, > .send_ccc_cmd = cdns_i3c_master_send_ccc_cmd, > .priv_xfers = cdns_i3c_master_priv_xfers, > + .send_hdr_cmds = cdns_i3c_master_send_hdr_cmd, > .i2c_xfers = cdns_i3c_master_i2c_xfers, > .i2c_funcs = cdns_i3c_master_i2c_funcs, > .enable_ibi = cdns_i3c_master_enable_ibi, _______________________________________________ linux-i3c mailing list linux-i3c@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-i3c ^ permalink raw reply [flat|nested] 14+ messages in thread
end of thread, other threads:[~2019-02-25 17:03 UTC | newest] Thread overview: 14+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2018-12-13 12:18 [PATCH 0/2] Add the I3C HDR modes Przemyslaw Gaj 2018-12-13 12:18 ` [PATCH 1/2] i3c: Add support for " Przemyslaw Gaj 2018-12-13 12:44 ` Boris Brezillon 2019-02-21 15:15 ` vitor 2019-02-22 14:52 ` Boris Brezillon 2019-02-22 15:02 ` Przemyslaw Gaj 2019-02-22 17:41 ` vitor 2019-02-25 8:56 ` Boris Brezillon 2019-02-25 12:56 ` vitor 2019-02-25 13:10 ` Boris Brezillon 2019-02-25 16:45 ` vitor 2019-02-25 17:03 ` Boris Brezillon 2018-12-13 12:18 ` [PATCH 2/2] i3c: master: cdns: Add support for HDR-DDR mode Przemyslaw Gaj 2018-12-13 12:45 ` Boris Brezillon
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).