| Message ID | 05fdeea79db83970e9ecb0d7045b4dd98f206f06.1555350118.git.vitor.soares@synopsys.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Fix i2c and i3c scl rate according bus mode | expand |
On Mon, 15 Apr 2019 20:46:41 +0200 Vitor Soares <vitor.soares@synopsys.com> wrote: > Currently in case of mixed slow bus topologie and all i2c devices > support FM+ speed, the i3c subsystem limite the SCL to FM speed. " Currently the I3C framework limits SCL frequency to FM speed when dealing with a mixed slow bus, even if all I2C devices are FM+ capable. " > Also in case on mixed slow bus mode the max speed for both > i2c or i3c transfers is FM or FM+. Looks like you're basically repeating what you said above. > > This patch fix the definition of i2c and i3c scl rate based on bus ^fixes on the bus > topologie and LVR[4] if no user input. ^topology ^if the rate is not already specified by the user. > In case of mixed slow mode the i3c scl rate is overridden. ^ with the max I2C rate. > > Fixes: 3a379bbcea0a ("i3c: Add core I3C infrastructure") > Signed-off-by: Vitor Soares <vitor.soares@synopsys.com> > Cc: Boris Brezillon <bbrezillon@kernel.org> > Cc: <stable@vger.kernel.org> > Cc: <linux-kernel@vger.kernel.org> > --- > drivers/i3c/master.c | 39 +++++++++++++++++++++++++-------------- > 1 file changed, 25 insertions(+), 14 deletions(-) > > diff --git a/drivers/i3c/master.c b/drivers/i3c/master.c > index 909c2ad..1c4a86a 100644 > --- a/drivers/i3c/master.c > +++ b/drivers/i3c/master.c > @@ -564,20 +564,30 @@ static const struct device_type i3c_masterdev_type = { > .groups = i3c_masterdev_groups, > }; > > -int i3c_bus_set_mode(struct i3c_bus *i3cbus, enum i3c_bus_mode mode) > +int i3c_bus_set_mode(struct i3c_bus *i3cbus, enum i3c_bus_mode mode, > + unsigned long i2c_scl_rate) Can we rename the last arg into max_i2c_scl_rate? > { > i3cbus->mode = mode; > > - if (!i3cbus->scl_rate.i3c) > - i3cbus->scl_rate.i3c = I3C_BUS_TYP_I3C_SCL_RATE; > - > - if (!i3cbus->scl_rate.i2c) { > - if (i3cbus->mode == I3C_BUS_MODE_MIXED_SLOW) > - i3cbus->scl_rate.i2c = I3C_BUS_I2C_FM_SCL_RATE; > - else > - i3cbus->scl_rate.i2c = I3C_BUS_I2C_FM_PLUS_SCL_RATE; > + switch (i3cbus->mode) { > + case I3C_BUS_MODE_PURE: > + if (!i3cbus->scl_rate.i3c) > + i3cbus->scl_rate.i3c = I3C_BUS_TYP_I3C_SCL_RATE; > + break; > + case I3C_BUS_MODE_MIXED_FAST: > + if (!i3cbus->scl_rate.i3c) > + i3cbus->scl_rate.i3c = I3C_BUS_TYP_I3C_SCL_RATE; > + if (!i3cbus->scl_rate.i2c) > + i3cbus->scl_rate.i2c = i2c_scl_rate; > + break; > + case I3C_BUS_MODE_MIXED_SLOW: > + if (!i3cbus->scl_rate.i2c) > + i3cbus->scl_rate.i2c = i2c_scl_rate; > + i3cbus->scl_rate.i3c = i3cbus->scl_rate.i2c; Maybe we should do if (!i3cbus->scl_rate.i3c || i3cbus->scl_rate.i3c > i3cbus->scl_rate.i2c) i3cbus->scl_rate.i3c = i3cbus->scl_rate.i2c; Just in case the I3C rate forced by the user is lower than the max I2C rate. The patch looks good otherwise. > + break; > + default: > + return -EINVAL; > } > - > /* > * I3C/I2C frequency may have been overridden, check that user-provided > * values are not exceeding max possible frequency. > @@ -1980,9 +1990,6 @@ of_i3c_master_add_i2c_boardinfo(struct i3c_master_controller *master, > /* LVR is encoded in reg[2]. */ > boardinfo->lvr = reg[2]; > > - if (boardinfo->lvr & I3C_LVR_I2C_FM_MODE) > - master->bus.scl_rate.i2c = I3C_BUS_I2C_FM_SCL_RATE; > - > list_add_tail(&boardinfo->node, &master->boardinfo.i2c); > of_node_get(node); > > @@ -2432,6 +2439,7 @@ int i3c_master_register(struct i3c_master_controller *master, > const struct i3c_master_controller_ops *ops, > bool secondary) > { > + unsigned long i2c_scl_rate = I3C_BUS_I2C_FM_PLUS_SCL_RATE; > struct i3c_bus *i3cbus = i3c_master_get_bus(master); > enum i3c_bus_mode mode = I3C_BUS_MODE_PURE; > struct i2c_dev_boardinfo *i2cbi; > @@ -2481,9 +2489,12 @@ int i3c_master_register(struct i3c_master_controller *master, > ret = -EINVAL; > goto err_put_dev; > } > + > + if (i2cbi->lvr & I3C_LVR_I2C_FM_MODE) > + i2c_scl_rate = I3C_BUS_I2C_FM_SCL_RATE; > } > > - ret = i3c_bus_set_mode(i3cbus, mode); > + ret = i3c_bus_set_mode(i3cbus, mode, i2c_scl_rate); > if (ret) > goto err_put_dev; >
Hi Boris, From: Boris Brezillon <boris.brezillon@collabora.com> Date: Tue, Apr 16, 2019 at 06:50:41 > On Mon, 15 Apr 2019 20:46:41 +0200 > Vitor Soares <vitor.soares@synopsys.com> wrote: > > > Currently in case of mixed slow bus topologie and all i2c devices > > support FM+ speed, the i3c subsystem limite the SCL to FM speed. > I will it replace with your message below. > " > Currently the I3C framework limits SCL frequency to FM speed when > dealing with a mixed slow bus, even if all I2C devices are FM+ capable. > " > > > Also in case on mixed slow bus mode the max speed for both > > i2c or i3c transfers is FM or FM+. > > Looks like you're basically repeating what you said above. What I meant was that I3C framework isn't limiting the I3C speed in case of mixed slow bus. > > > > > This patch fix the definition of i2c and i3c scl rate based on bus > > ^fixes on the bus > > > topologie and LVR[4] if no user input. > > ^topology ^if the rate is not already specified by the user. > > > In case of mixed slow mode the i3c scl rate is overridden. > > ^ with the max > I2C rate. > > > > > Fixes: 3a379bbcea0a ("i3c: Add core I3C infrastructure") > > Signed-off-by: Vitor Soares <vitor.soares@synopsys.com> > > Cc: Boris Brezillon <bbrezillon@kernel.org> > > Cc: <stable@vger.kernel.org> > > Cc: <linux-kernel@vger.kernel.org> > > --- > > drivers/i3c/master.c | 39 +++++++++++++++++++++++++-------------- > > 1 file changed, 25 insertions(+), 14 deletions(-) > > > > diff --git a/drivers/i3c/master.c b/drivers/i3c/master.c > > index 909c2ad..1c4a86a 100644 > > --- a/drivers/i3c/master.c > > +++ b/drivers/i3c/master.c > > @@ -564,20 +564,30 @@ static const struct device_type i3c_masterdev_type = { > > .groups = i3c_masterdev_groups, > > }; > > > > -int i3c_bus_set_mode(struct i3c_bus *i3cbus, enum i3c_bus_mode mode) > > +int i3c_bus_set_mode(struct i3c_bus *i3cbus, enum i3c_bus_mode mode, > > + unsigned long i2c_scl_rate) > > > Can we rename the last arg into max_i2c_scl_rate? The i2c_scl_rate is base on LVR[4] bit and the user can set a higher scl rate, this is reason I didn't named it to max_i2c_scl_rate. But if you think that make more sense I'm ok with that. > > > { > > i3cbus->mode = mode; > > > > - if (!i3cbus->scl_rate.i3c) > > - i3cbus->scl_rate.i3c = I3C_BUS_TYP_I3C_SCL_RATE; > > - > > - if (!i3cbus->scl_rate.i2c) { > > - if (i3cbus->mode == I3C_BUS_MODE_MIXED_SLOW) > > - i3cbus->scl_rate.i2c = I3C_BUS_I2C_FM_SCL_RATE; > > - else > > - i3cbus->scl_rate.i2c = I3C_BUS_I2C_FM_PLUS_SCL_RATE; > > + switch (i3cbus->mode) { > > + case I3C_BUS_MODE_PURE: > > + if (!i3cbus->scl_rate.i3c) > > + i3cbus->scl_rate.i3c = I3C_BUS_TYP_I3C_SCL_RATE; > > + break; > > + case I3C_BUS_MODE_MIXED_FAST: > > + if (!i3cbus->scl_rate.i3c) > > + i3cbus->scl_rate.i3c = I3C_BUS_TYP_I3C_SCL_RATE; > > + if (!i3cbus->scl_rate.i2c) > > + i3cbus->scl_rate.i2c = i2c_scl_rate; > > + break; > > + case I3C_BUS_MODE_MIXED_SLOW: > > + if (!i3cbus->scl_rate.i2c) > > + i3cbus->scl_rate.i2c = i2c_scl_rate; > > + i3cbus->scl_rate.i3c = i3cbus->scl_rate.i2c; > > Maybe we should do > > if (!i3cbus->scl_rate.i3c || > i3cbus->scl_rate.i3c > i3cbus->scl_rate.i2c) > i3cbus->scl_rate.i3c = i3cbus->scl_rate.i2c; > > Just in case the I3C rate forced by the user is lower than the max I2C > rate. That was something that I considered but TBH it isn't a real use case. > > The patch looks good otherwise. Thanks. > > > + break; > > + default: > > + return -EINVAL; > > } > > - > > /* > > * I3C/I2C frequency may have been overridden, check that user-provided > > * values are not exceeding max possible frequency. > > @@ -1980,9 +1990,6 @@ of_i3c_master_add_i2c_boardinfo(struct i3c_master_controller *master, > > /* LVR is encoded in reg[2]. */ > > boardinfo->lvr = reg[2]; > > > > - if (boardinfo->lvr & I3C_LVR_I2C_FM_MODE) > > - master->bus.scl_rate.i2c = I3C_BUS_I2C_FM_SCL_RATE; > > - > > list_add_tail(&boardinfo->node, &master->boardinfo.i2c); > > of_node_get(node); > > > > @@ -2432,6 +2439,7 @@ int i3c_master_register(struct i3c_master_controller *master, > > const struct i3c_master_controller_ops *ops, > > bool secondary) > > { > > + unsigned long i2c_scl_rate = I3C_BUS_I2C_FM_PLUS_SCL_RATE; > > struct i3c_bus *i3cbus = i3c_master_get_bus(master); > > enum i3c_bus_mode mode = I3C_BUS_MODE_PURE; > > struct i2c_dev_boardinfo *i2cbi; > > @@ -2481,9 +2489,12 @@ int i3c_master_register(struct i3c_master_controller *master, > > ret = -EINVAL; > > goto err_put_dev; > > } > > + > > + if (i2cbi->lvr & I3C_LVR_I2C_FM_MODE) > > + i2c_scl_rate = I3C_BUS_I2C_FM_SCL_RATE; > > } > > > > - ret = i3c_bus_set_mode(i3cbus, mode); > > + ret = i3c_bus_set_mode(i3cbus, mode, i2c_scl_rate); > > if (ret) > > goto err_put_dev; > > Best regards, Vitor Soares
On Tue, 16 Apr 2019 14:24:57 +0000 Vitor Soares <vitor.soares@synopsys.com> wrote: > Hi Boris, > > From: Boris Brezillon <boris.brezillon@collabora.com> > Date: Tue, Apr 16, 2019 at 06:50:41 > > > On Mon, 15 Apr 2019 20:46:41 +0200 > > Vitor Soares <vitor.soares@synopsys.com> wrote: > > > > > Currently in case of mixed slow bus topologie and all i2c devices > > > support FM+ speed, the i3c subsystem limite the SCL to FM speed. > > > > I will it replace with your message below. > > > " > > Currently the I3C framework limits SCL frequency to FM speed when > > dealing with a mixed slow bus, even if all I2C devices are FM+ capable. > > " > > > > > Also in case on mixed slow bus mode the max speed for both > > > i2c or i3c transfers is FM or FM+. > > > > Looks like you're basically repeating what you said above. > > What I meant was that I3C framework isn't limiting the I3C speed in case > of mixed slow bus. Oh, okay, then maybe something like " The core was also not accounting for I3C speed limitations when operating in mixed slow mode and was erroneously using FM+ speed as the max I2C speed when operating in mixed fast mode. " > > > > > > > > > This patch fix the definition of i2c and i3c scl rate based on bus > > > > ^fixes on the bus > > > > > topologie and LVR[4] if no user input. > > > > ^topology ^if the rate is not already specified by the user. > > > > > In case of mixed slow mode the i3c scl rate is overridden. > > > > ^ with the max > > I2C rate. > > > > > > > > Fixes: 3a379bbcea0a ("i3c: Add core I3C infrastructure") > > > Signed-off-by: Vitor Soares <vitor.soares@synopsys.com> > > > Cc: Boris Brezillon <bbrezillon@kernel.org> > > > Cc: <stable@vger.kernel.org> > > > Cc: <linux-kernel@vger.kernel.org> > > > --- > > > drivers/i3c/master.c | 39 +++++++++++++++++++++++++-------------- > > > 1 file changed, 25 insertions(+), 14 deletions(-) > > > > > > diff --git a/drivers/i3c/master.c b/drivers/i3c/master.c > > > index 909c2ad..1c4a86a 100644 > > > --- a/drivers/i3c/master.c > > > +++ b/drivers/i3c/master.c > > > @@ -564,20 +564,30 @@ static const struct device_type i3c_masterdev_type = { > > > .groups = i3c_masterdev_groups, > > > }; > > > > > > -int i3c_bus_set_mode(struct i3c_bus *i3cbus, enum i3c_bus_mode mode) > > > +int i3c_bus_set_mode(struct i3c_bus *i3cbus, enum i3c_bus_mode mode, > > > + unsigned long i2c_scl_rate) > > > > > > Can we rename the last arg into max_i2c_scl_rate? > > The i2c_scl_rate is base on LVR[4] bit and the user can set a higher scl > rate, this is reason I didn't named it to max_i2c_scl_rate. > But if you think that make more sense I'm ok with that. In this context it does encode the maximum rate allowed by the spec (based on LVR parsing), so max_i2c_rate sounds like a correct name to me. > > > > > > { > > > i3cbus->mode = mode; > > > > > > - if (!i3cbus->scl_rate.i3c) > > > - i3cbus->scl_rate.i3c = I3C_BUS_TYP_I3C_SCL_RATE; > > > - > > > - if (!i3cbus->scl_rate.i2c) { > > > - if (i3cbus->mode == I3C_BUS_MODE_MIXED_SLOW) > > > - i3cbus->scl_rate.i2c = I3C_BUS_I2C_FM_SCL_RATE; > > > - else > > > - i3cbus->scl_rate.i2c = I3C_BUS_I2C_FM_PLUS_SCL_RATE; > > > + switch (i3cbus->mode) { > > > + case I3C_BUS_MODE_PURE: > > > + if (!i3cbus->scl_rate.i3c) > > > + i3cbus->scl_rate.i3c = I3C_BUS_TYP_I3C_SCL_RATE; > > > + break; > > > + case I3C_BUS_MODE_MIXED_FAST: > > > + if (!i3cbus->scl_rate.i3c) > > > + i3cbus->scl_rate.i3c = I3C_BUS_TYP_I3C_SCL_RATE; > > > + if (!i3cbus->scl_rate.i2c) > > > + i3cbus->scl_rate.i2c = i2c_scl_rate; > > > + break; > > > + case I3C_BUS_MODE_MIXED_SLOW: > > > + if (!i3cbus->scl_rate.i2c) > > > + i3cbus->scl_rate.i2c = i2c_scl_rate; > > > + i3cbus->scl_rate.i3c = i3cbus->scl_rate.i2c; > > > > Maybe we should do > > > > if (!i3cbus->scl_rate.i3c || > > i3cbus->scl_rate.i3c > i3cbus->scl_rate.i2c) > > i3cbus->scl_rate.i3c = i3cbus->scl_rate.i2c; > > > > Just in case the I3C rate forced by the user is lower than the max I2C > > rate. > > That was something that I considered but TBH it isn't a real use case. Add a WARN_ON() to at least catch such inconsistencies. And maybe we should add a dev_warn() when the user-defined rates do not match the mode/LVR constraints. It's easy to do a mistake when writing a dts.
Hi Boris, From: Boris Brezillon <boris.brezillon@collabora.com> Date: Tue, Apr 16, 2019 at 15:52:50 > On Tue, 16 Apr 2019 14:24:57 +0000 > Vitor Soares <vitor.soares@synopsys.com> wrote: > > > Hi Boris, > > > > From: Boris Brezillon <boris.brezillon@collabora.com> > > Date: Tue, Apr 16, 2019 at 06:50:41 > > > > > On Mon, 15 Apr 2019 20:46:41 +0200 > > > Vitor Soares <vitor.soares@synopsys.com> wrote: > > > > > > > Currently in case of mixed slow bus topologie and all i2c devices > > > > support FM+ speed, the i3c subsystem limite the SCL to FM speed. > > > > > > > I will it replace with your message below. > > > > > " > > > Currently the I3C framework limits SCL frequency to FM speed when > > > dealing with a mixed slow bus, even if all I2C devices are FM+ capable. > > > " > > > > > > > Also in case on mixed slow bus mode the max speed for both > > > > i2c or i3c transfers is FM or FM+. > > > > > > Looks like you're basically repeating what you said above. > > > > What I meant was that I3C framework isn't limiting the I3C speed in case > > of mixed slow bus. > > Oh, okay, then maybe something like > > " > The core was also not accounting for I3C speed limitations when > operating in mixed slow mode and was erroneously using FM+ speed as the > max I2C speed when operating in mixed fast mode. > " Sounds good to me. Thanks for the advise. > > > > > > > > > > > > > > This patch fix the definition of i2c and i3c scl rate based on bus > > > > > > ^fixes on the bus > > > > > > > topologie and LVR[4] if no user input. > > > > > > ^topology ^if the rate is not already specified by the user. > > > > > > > In case of mixed slow mode the i3c scl rate is overridden. > > > > > > ^ with the max > > > I2C rate. > > > > > > > > > > > Fixes: 3a379bbcea0a ("i3c: Add core I3C infrastructure") > > > > Signed-off-by: Vitor Soares <vitor.soares@synopsys.com> > > > > Cc: Boris Brezillon <bbrezillon@kernel.org> > > > > Cc: <stable@vger.kernel.org> > > > > Cc: <linux-kernel@vger.kernel.org> > > > > --- > > > > drivers/i3c/master.c | 39 +++++++++++++++++++++++++-------------- > > > > 1 file changed, 25 insertions(+), 14 deletions(-) > > > > > > > > diff --git a/drivers/i3c/master.c b/drivers/i3c/master.c > > > > index 909c2ad..1c4a86a 100644 > > > > --- a/drivers/i3c/master.c > > > > +++ b/drivers/i3c/master.c > > > > @@ -564,20 +564,30 @@ static const struct device_type i3c_masterdev_type = { > > > > .groups = i3c_masterdev_groups, > > > > }; > > > > > > > > -int i3c_bus_set_mode(struct i3c_bus *i3cbus, enum i3c_bus_mode mode) > > > > +int i3c_bus_set_mode(struct i3c_bus *i3cbus, enum i3c_bus_mode mode, > > > > + unsigned long i2c_scl_rate) > > > > > > > > > Can we rename the last arg into max_i2c_scl_rate? > > > > The i2c_scl_rate is base on LVR[4] bit and the user can set a higher scl > > rate, this is reason I didn't named it to max_i2c_scl_rate. > > But if you think that make more sense I'm ok with that. > > In this context it does encode the maximum rate allowed by the spec > (based on LVR parsing), so max_i2c_rate sounds like a correct name to > me. > > > > > > > > > > { > > > > i3cbus->mode = mode; > > > > > > > > - if (!i3cbus->scl_rate.i3c) > > > > - i3cbus->scl_rate.i3c = I3C_BUS_TYP_I3C_SCL_RATE; > > > > - > > > > - if (!i3cbus->scl_rate.i2c) { > > > > - if (i3cbus->mode == I3C_BUS_MODE_MIXED_SLOW) > > > > - i3cbus->scl_rate.i2c = I3C_BUS_I2C_FM_SCL_RATE; > > > > - else > > > > - i3cbus->scl_rate.i2c = I3C_BUS_I2C_FM_PLUS_SCL_RATE; > > > > + switch (i3cbus->mode) { > > > > + case I3C_BUS_MODE_PURE: > > > > + if (!i3cbus->scl_rate.i3c) > > > > + i3cbus->scl_rate.i3c = I3C_BUS_TYP_I3C_SCL_RATE; > > > > + break; > > > > + case I3C_BUS_MODE_MIXED_FAST: > > > > + if (!i3cbus->scl_rate.i3c) > > > > + i3cbus->scl_rate.i3c = I3C_BUS_TYP_I3C_SCL_RATE; > > > > + if (!i3cbus->scl_rate.i2c) > > > > + i3cbus->scl_rate.i2c = i2c_scl_rate; > > > > + break; > > > > + case I3C_BUS_MODE_MIXED_SLOW: > > > > + if (!i3cbus->scl_rate.i2c) > > > > + i3cbus->scl_rate.i2c = i2c_scl_rate; > > > > + i3cbus->scl_rate.i3c = i3cbus->scl_rate.i2c; > > > > > > Maybe we should do > > > > > > if (!i3cbus->scl_rate.i3c || > > > i3cbus->scl_rate.i3c > i3cbus->scl_rate.i2c) > > > i3cbus->scl_rate.i3c = i3cbus->scl_rate.i2c; > > > > > > Just in case the I3C rate forced by the user is lower than the max I2C > > > rate. > > > > That was something that I considered but TBH it isn't a real use case. > > Add a WARN_ON() to at least catch such inconsistencies. And maybe we > should add a dev_warn() when the user-defined rates do not match > the mode/LVR constraints. It's easy to do a mistake when writing a dts. I think the WARN_ON() is too evasive on the screen and won't provide the information we want. The dev_warn() should work perfectly here. if (i3cbus->scl_rate.i3c < i3cbus->scl_rate.i2c) dev_warn(&i3cbus->cur_master->dev->dev, "%s: i3c-scl-hz lower then i2c-scl-hz\n", __func__); if (i3cbus->scl_rate.i2c != I3C_BUS_I2C_FM_SCL_RATE || i3cbus->scl_rate.i2c != I3C_BUS_I2C_FM_PLUS_SCL_RATE) dev_warn(&i3cbus->cur_master->dev->dev, "%s: i2c-scl-hz not defined according MIPI I3C spec\n", __func__); Maybe it make more sense to do this check on of_populate_i3c_bus(), what do you think?
On Mon, 22 Apr 2019 15:54:33 +0000 Vitor Soares <Vitor.Soares@synopsys.com> wrote: > > > > > > > > > { > > > > > i3cbus->mode = mode; > > > > > > > > > > - if (!i3cbus->scl_rate.i3c) > > > > > - i3cbus->scl_rate.i3c = I3C_BUS_TYP_I3C_SCL_RATE; > > > > > - > > > > > - if (!i3cbus->scl_rate.i2c) { > > > > > - if (i3cbus->mode == I3C_BUS_MODE_MIXED_SLOW) > > > > > - i3cbus->scl_rate.i2c = I3C_BUS_I2C_FM_SCL_RATE; > > > > > - else > > > > > - i3cbus->scl_rate.i2c = I3C_BUS_I2C_FM_PLUS_SCL_RATE; > > > > > + switch (i3cbus->mode) { > > > > > + case I3C_BUS_MODE_PURE: > > > > > + if (!i3cbus->scl_rate.i3c) > > > > > + i3cbus->scl_rate.i3c = I3C_BUS_TYP_I3C_SCL_RATE; > > > > > + break; > > > > > + case I3C_BUS_MODE_MIXED_FAST: > > > > > + if (!i3cbus->scl_rate.i3c) > > > > > + i3cbus->scl_rate.i3c = I3C_BUS_TYP_I3C_SCL_RATE; > > > > > + if (!i3cbus->scl_rate.i2c) > > > > > + i3cbus->scl_rate.i2c = i2c_scl_rate; > > > > > + break; > > > > > + case I3C_BUS_MODE_MIXED_SLOW: > > > > > + if (!i3cbus->scl_rate.i2c) > > > > > + i3cbus->scl_rate.i2c = i2c_scl_rate; > > > > > + i3cbus->scl_rate.i3c = i3cbus->scl_rate.i2c; > > > > > > > > Maybe we should do > > > > > > > > if (!i3cbus->scl_rate.i3c || > > > > i3cbus->scl_rate.i3c > i3cbus->scl_rate.i2c) > > > > i3cbus->scl_rate.i3c = i3cbus->scl_rate.i2c; > > > > > > > > Just in case the I3C rate forced by the user is lower than the max I2C > > > > rate. > > > > > > That was something that I considered but TBH it isn't a real use case. > > > > Add a WARN_ON() to at least catch such inconsistencies. And maybe we > > should add a dev_warn() when the user-defined rates do not match > > the mode/LVR constraints. It's easy to do a mistake when writing a dts. > > I think the WARN_ON() is too evasive on the screen and won't provide the > information we want. > The dev_warn() should work perfectly here. > > if (i3cbus->scl_rate.i3c < i3cbus->scl_rate.i2c) > dev_warn(&i3cbus->cur_master->dev->dev, > "%s: i3c-scl-hz lower then i2c-scl-hz\n", __func__); Using dev_warn() sounds good, though I don't think you need the __func__ here. Also, please print the i2c/i3c rates in the message, and align the second line on the open parens. > if (i3cbus->scl_rate.i2c != I3C_BUS_I2C_FM_SCL_RATE || > i3cbus->scl_rate.i2c != I3C_BUS_I2C_FM_PLUS_SCL_RATE) > dev_warn(&i3cbus->cur_master->dev->dev, > "%s: i2c-scl-hz not defined according MIPI I3C spec\n", > __func__); Is that really a problem? Having an i2c rate that is less than FM speed sounds like a valid case to me. > > Maybe it make more sense to do this check on of_populate_i3c_bus(), what > do you think? > No, we really want to have this check here, because we might support other HW description formats at some point (board-files, ACPI, ...).
From: Boris Brezillon <boris.brezillon@collabora.com> Date: Mon, Apr 22, 2019 at 17:07:15 > On Mon, 22 Apr 2019 15:54:33 +0000 > Vitor Soares <Vitor.Soares@synopsys.com> wrote: > > > > > > > > > > > > > { > > > > > > i3cbus->mode = mode; > > > > > > > > > > > > - if (!i3cbus->scl_rate.i3c) > > > > > > - i3cbus->scl_rate.i3c = I3C_BUS_TYP_I3C_SCL_RATE; > > > > > > - > > > > > > - if (!i3cbus->scl_rate.i2c) { > > > > > > - if (i3cbus->mode == I3C_BUS_MODE_MIXED_SLOW) > > > > > > - i3cbus->scl_rate.i2c = I3C_BUS_I2C_FM_SCL_RATE; > > > > > > - else > > > > > > - i3cbus->scl_rate.i2c = I3C_BUS_I2C_FM_PLUS_SCL_RATE; > > > > > > + switch (i3cbus->mode) { > > > > > > + case I3C_BUS_MODE_PURE: > > > > > > + if (!i3cbus->scl_rate.i3c) > > > > > > + i3cbus->scl_rate.i3c = I3C_BUS_TYP_I3C_SCL_RATE; > > > > > > + break; > > > > > > + case I3C_BUS_MODE_MIXED_FAST: > > > > > > + if (!i3cbus->scl_rate.i3c) > > > > > > + i3cbus->scl_rate.i3c = I3C_BUS_TYP_I3C_SCL_RATE; > > > > > > + if (!i3cbus->scl_rate.i2c) > > > > > > + i3cbus->scl_rate.i2c = i2c_scl_rate; > > > > > > + break; > > > > > > + case I3C_BUS_MODE_MIXED_SLOW: > > > > > > + if (!i3cbus->scl_rate.i2c) > > > > > > + i3cbus->scl_rate.i2c = i2c_scl_rate; > > > > > > + i3cbus->scl_rate.i3c = i3cbus->scl_rate.i2c; > > > > > > > > > > Maybe we should do > > > > > > > > > > if (!i3cbus->scl_rate.i3c || > > > > > i3cbus->scl_rate.i3c > i3cbus->scl_rate.i2c) > > > > > i3cbus->scl_rate.i3c = i3cbus->scl_rate.i2c; > > > > > > > > > > Just in case the I3C rate forced by the user is lower than the max I2C > > > > > rate. > > > > > > > > That was something that I considered but TBH it isn't a real use case. > > > > > > Add a WARN_ON() to at least catch such inconsistencies. And maybe we > > > should add a dev_warn() when the user-defined rates do not match > > > the mode/LVR constraints. It's easy to do a mistake when writing a dts. > > > > I think the WARN_ON() is too evasive on the screen and won't provide the > > information we want. > > The dev_warn() should work perfectly here. > > > > if (i3cbus->scl_rate.i3c < i3cbus->scl_rate.i2c) > > dev_warn(&i3cbus->cur_master->dev->dev, > > "%s: i3c-scl-hz lower then i2c-scl-hz\n", __func__); > > Using dev_warn() sounds good, though I don't think you need the > __func__ here. Also, please print the i2c/i3c rates in the message, and > align the second line on the open parens. > > > if (i3cbus->scl_rate.i2c != I3C_BUS_I2C_FM_SCL_RATE || > > i3cbus->scl_rate.i2c != I3C_BUS_I2C_FM_PLUS_SCL_RATE) > > dev_warn(&i3cbus->cur_master->dev->dev, > > "%s: i2c-scl-hz not defined according MIPI I3C spec\n", > > __func__); > > Is that really a problem? Having an i2c rate that is less than FM speed > sounds like a valid case to me. I'm addressing the spec constrains. In the practice it can be SM or even HS, it depends on the interface. > > > > > Maybe it make more sense to do this check on of_populate_i3c_bus(), what > > do you think? > > > > No, we really want to have this check here, because we might support > other HW description formats at some point (board-files, ACPI, ...). Yes, you are right. I forgot that point.
On Mon, 22 Apr 2019 17:54:29 +0000 Vitor Soares <Vitor.Soares@synopsys.com> wrote: > > > > > > > > > > > > > { > > > > > > > i3cbus->mode = mode; > > > > > > > > > > > > > > - if (!i3cbus->scl_rate.i3c) > > > > > > > - i3cbus->scl_rate.i3c = I3C_BUS_TYP_I3C_SCL_RATE; > > > > > > > - > > > > > > > - if (!i3cbus->scl_rate.i2c) { > > > > > > > - if (i3cbus->mode == I3C_BUS_MODE_MIXED_SLOW) > > > > > > > - i3cbus->scl_rate.i2c = I3C_BUS_I2C_FM_SCL_RATE; > > > > > > > - else > > > > > > > - i3cbus->scl_rate.i2c = I3C_BUS_I2C_FM_PLUS_SCL_RATE; > > > > > > > + switch (i3cbus->mode) { > > > > > > > + case I3C_BUS_MODE_PURE: > > > > > > > + if (!i3cbus->scl_rate.i3c) > > > > > > > + i3cbus->scl_rate.i3c = I3C_BUS_TYP_I3C_SCL_RATE; > > > > > > > + break; > > > > > > > + case I3C_BUS_MODE_MIXED_FAST: > > > > > > > + if (!i3cbus->scl_rate.i3c) > > > > > > > + i3cbus->scl_rate.i3c = I3C_BUS_TYP_I3C_SCL_RATE; > > > > > > > + if (!i3cbus->scl_rate.i2c) > > > > > > > + i3cbus->scl_rate.i2c = i2c_scl_rate; > > > > > > > + break; > > > > > > > + case I3C_BUS_MODE_MIXED_SLOW: > > > > > > > + if (!i3cbus->scl_rate.i2c) > > > > > > > + i3cbus->scl_rate.i2c = i2c_scl_rate; > > > > > > > + i3cbus->scl_rate.i3c = i3cbus->scl_rate.i2c; > > > > > > > > > > > > Maybe we should do > > > > > > > > > > > > if (!i3cbus->scl_rate.i3c || > > > > > > i3cbus->scl_rate.i3c > i3cbus->scl_rate.i2c) > > > > > > i3cbus->scl_rate.i3c = i3cbus->scl_rate.i2c; > > > > > > > > > > > > Just in case the I3C rate forced by the user is lower than the max I2C > > > > > > rate. > > > > > > > > > > That was something that I considered but TBH it isn't a real use case. > > > > > > > > Add a WARN_ON() to at least catch such inconsistencies. And maybe we > > > > should add a dev_warn() when the user-defined rates do not match > > > > the mode/LVR constraints. It's easy to do a mistake when writing a dts. > > > > > > I think the WARN_ON() is too evasive on the screen and won't provide the > > > information we want. > > > The dev_warn() should work perfectly here. > > > > > > if (i3cbus->scl_rate.i3c < i3cbus->scl_rate.i2c) > > > dev_warn(&i3cbus->cur_master->dev->dev, > > > "%s: i3c-scl-hz lower then i2c-scl-hz\n", __func__); > > > > Using dev_warn() sounds good, though I don't think you need the > > __func__ here. Also, please print the i2c/i3c rates in the message, and > > align the second line on the open parens. > > > > > if (i3cbus->scl_rate.i2c != I3C_BUS_I2C_FM_SCL_RATE || > > > i3cbus->scl_rate.i2c != I3C_BUS_I2C_FM_PLUS_SCL_RATE) > > > dev_warn(&i3cbus->cur_master->dev->dev, > > > "%s: i2c-scl-hz not defined according MIPI I3C spec\n", > > > __func__); > > > > Is that really a problem? Having an i2c rate that is less than FM speed > > sounds like a valid case to me. > > I'm addressing the spec constrains. "Table 57 I3C Timing Requirements When Communicating With I2C Legacy Devices" says that freq can be between 0 and 400KHz when operating in slow(FM) mode. Yes, maximum rate when not specified otherwise is 400Khz, but the point of overloading the max I2C/I3C spec is to allow custom rates when the default/spec ones are not achievable, so I'm not sure complaining in that case is legitimate. We should definitely complain when one tries to set a maximum rate that is higher than what devices can do (i3cbus->scl_rate.i2c > max_i2c_scl_rate). Same goes for I3C communications, we shouldn't care when the forced rate is lower than what the bus is capable of, what's important is to complain when it's higher than what's supported.
From: Boris Brezillon <boris.brezillon@collabora.com> Date: Mon, Apr 22, 2019 at 19:27:32 > On Mon, 22 Apr 2019 17:54:29 +0000 > Vitor Soares <Vitor.Soares@synopsys.com> wrote: > > > > > > > > > > > > > > > > { > > > > > > > > i3cbus->mode = mode; > > > > > > > > > > > > > > > > - if (!i3cbus->scl_rate.i3c) > > > > > > > > - i3cbus->scl_rate.i3c = I3C_BUS_TYP_I3C_SCL_RATE; > > > > > > > > - > > > > > > > > - if (!i3cbus->scl_rate.i2c) { > > > > > > > > - if (i3cbus->mode == I3C_BUS_MODE_MIXED_SLOW) > > > > > > > > - i3cbus->scl_rate.i2c = I3C_BUS_I2C_FM_SCL_RATE; > > > > > > > > - else > > > > > > > > - i3cbus->scl_rate.i2c = I3C_BUS_I2C_FM_PLUS_SCL_RATE; > > > > > > > > + switch (i3cbus->mode) { > > > > > > > > + case I3C_BUS_MODE_PURE: > > > > > > > > + if (!i3cbus->scl_rate.i3c) > > > > > > > > + i3cbus->scl_rate.i3c = I3C_BUS_TYP_I3C_SCL_RATE; > > > > > > > > + break; > > > > > > > > + case I3C_BUS_MODE_MIXED_FAST: > > > > > > > > + if (!i3cbus->scl_rate.i3c) > > > > > > > > + i3cbus->scl_rate.i3c = I3C_BUS_TYP_I3C_SCL_RATE; > > > > > > > > + if (!i3cbus->scl_rate.i2c) > > > > > > > > + i3cbus->scl_rate.i2c = i2c_scl_rate; > > > > > > > > + break; > > > > > > > > + case I3C_BUS_MODE_MIXED_SLOW: > > > > > > > > + if (!i3cbus->scl_rate.i2c) > > > > > > > > + i3cbus->scl_rate.i2c = i2c_scl_rate; > > > > > > > > + i3cbus->scl_rate.i3c = i3cbus->scl_rate.i2c; > > > > > > > > > > > > > > Maybe we should do > > > > > > > > > > > > > > if (!i3cbus->scl_rate.i3c || > > > > > > > i3cbus->scl_rate.i3c > i3cbus->scl_rate.i2c) > > > > > > > i3cbus->scl_rate.i3c = i3cbus->scl_rate.i2c; > > > > > > > > > > > > > > Just in case the I3C rate forced by the user is lower than the max I2C > > > > > > > rate. > > > > > > > > > > > > That was something that I considered but TBH it isn't a real use case. > > > > > > > > > > Add a WARN_ON() to at least catch such inconsistencies. And maybe we > > > > > should add a dev_warn() when the user-defined rates do not match > > > > > the mode/LVR constraints. It's easy to do a mistake when writing a dts. > > > > > > > > I think the WARN_ON() is too evasive on the screen and won't provide the > > > > information we want. > > > > The dev_warn() should work perfectly here. > > > > > > > > if (i3cbus->scl_rate.i3c < i3cbus->scl_rate.i2c) > > > > dev_warn(&i3cbus->cur_master->dev->dev, > > > > "%s: i3c-scl-hz lower then i2c-scl-hz\n", __func__); > > > > > > Using dev_warn() sounds good, though I don't think you need the > > > __func__ here. Also, please print the i2c/i3c rates in the message, and > > > align the second line on the open parens. > > > > > > > if (i3cbus->scl_rate.i2c != I3C_BUS_I2C_FM_SCL_RATE || > > > > i3cbus->scl_rate.i2c != I3C_BUS_I2C_FM_PLUS_SCL_RATE) > > > > dev_warn(&i3cbus->cur_master->dev->dev, > > > > "%s: i2c-scl-hz not defined according MIPI I3C spec\n", > > > > __func__); > > > > > > Is that really a problem? Having an i2c rate that is less than FM speed > > > sounds like a valid case to me. > > > > I'm addressing the spec constrains. > > "Table 57 I3C Timing Requirements When Communicating With I2C Legacy > Devices" says that freq can be between 0 and 400KHz when operating in > slow(FM) mode. That is the definition of Fast-mode: "Fast-mode devices can receive and transmit at up to 400kbit/s" as per i2c spec. So this way the slaves can be connected to Standard Mode bus... you already know that. For the FM requirements in I3C bus, please refer to I3C FAQ Q2.2 and table 56 of I3C v1.0 basic spec. > Yes, maximum rate when not specified otherwise is > 400Khz, By default you will have always FM or FM+ due the LVR register be mandatory. > but the point of overloading the max I2C/I3C spec is to allow > custom rates when the default/spec ones are not achievable, so I'm not > sure complaining in that case is legitimate. > Well, if the users aren't able to achieve at least FM is because something is not correct. > We should definitely complain when one tries to set a maximum rate that > is higher than what devices can do (i3cbus->scl_rate.i2c > > max_i2c_scl_rate). > I can put another if() for such cases. > Same goes for I3C communications, we shouldn't care when the forced rate > is lower than what the bus is capable of, what's important is to > complain when it's higher than what's supported. The point is complain when scl_rate.i3c < scl_rate.i2c because it can be error from user. Best regards, Vitor Soares
diff --git a/drivers/i3c/master.c b/drivers/i3c/master.c index 909c2ad..1c4a86a 100644 --- a/drivers/i3c/master.c +++ b/drivers/i3c/master.c @@ -564,20 +564,30 @@ static const struct device_type i3c_masterdev_type = { .groups = i3c_masterdev_groups, }; -int i3c_bus_set_mode(struct i3c_bus *i3cbus, enum i3c_bus_mode mode) +int i3c_bus_set_mode(struct i3c_bus *i3cbus, enum i3c_bus_mode mode, + unsigned long i2c_scl_rate) { i3cbus->mode = mode; - if (!i3cbus->scl_rate.i3c) - i3cbus->scl_rate.i3c = I3C_BUS_TYP_I3C_SCL_RATE; - - if (!i3cbus->scl_rate.i2c) { - if (i3cbus->mode == I3C_BUS_MODE_MIXED_SLOW) - i3cbus->scl_rate.i2c = I3C_BUS_I2C_FM_SCL_RATE; - else - i3cbus->scl_rate.i2c = I3C_BUS_I2C_FM_PLUS_SCL_RATE; + switch (i3cbus->mode) { + case I3C_BUS_MODE_PURE: + if (!i3cbus->scl_rate.i3c) + i3cbus->scl_rate.i3c = I3C_BUS_TYP_I3C_SCL_RATE; + break; + case I3C_BUS_MODE_MIXED_FAST: + if (!i3cbus->scl_rate.i3c) + i3cbus->scl_rate.i3c = I3C_BUS_TYP_I3C_SCL_RATE; + if (!i3cbus->scl_rate.i2c) + i3cbus->scl_rate.i2c = i2c_scl_rate; + break; + case I3C_BUS_MODE_MIXED_SLOW: + if (!i3cbus->scl_rate.i2c) + i3cbus->scl_rate.i2c = i2c_scl_rate; + i3cbus->scl_rate.i3c = i3cbus->scl_rate.i2c; + break; + default: + return -EINVAL; } - /* * I3C/I2C frequency may have been overridden, check that user-provided * values are not exceeding max possible frequency. @@ -1980,9 +1990,6 @@ of_i3c_master_add_i2c_boardinfo(struct i3c_master_controller *master, /* LVR is encoded in reg[2]. */ boardinfo->lvr = reg[2]; - if (boardinfo->lvr & I3C_LVR_I2C_FM_MODE) - master->bus.scl_rate.i2c = I3C_BUS_I2C_FM_SCL_RATE; - list_add_tail(&boardinfo->node, &master->boardinfo.i2c); of_node_get(node); @@ -2432,6 +2439,7 @@ int i3c_master_register(struct i3c_master_controller *master, const struct i3c_master_controller_ops *ops, bool secondary) { + unsigned long i2c_scl_rate = I3C_BUS_I2C_FM_PLUS_SCL_RATE; struct i3c_bus *i3cbus = i3c_master_get_bus(master); enum i3c_bus_mode mode = I3C_BUS_MODE_PURE; struct i2c_dev_boardinfo *i2cbi; @@ -2481,9 +2489,12 @@ int i3c_master_register(struct i3c_master_controller *master, ret = -EINVAL; goto err_put_dev; } + + if (i2cbi->lvr & I3C_LVR_I2C_FM_MODE) + i2c_scl_rate = I3C_BUS_I2C_FM_SCL_RATE; } - ret = i3c_bus_set_mode(i3cbus, mode); + ret = i3c_bus_set_mode(i3cbus, mode, i2c_scl_rate); if (ret) goto err_put_dev;
Currently in case of mixed slow bus topologie and all i2c devices support FM+ speed, the i3c subsystem limite the SCL to FM speed. Also in case on mixed slow bus mode the max speed for both i2c or i3c transfers is FM or FM+. This patch fix the definition of i2c and i3c scl rate based on bus topologie and LVR[4] if no user input. In case of mixed slow mode the i3c scl rate is overridden. Fixes: 3a379bbcea0a ("i3c: Add core I3C infrastructure") Signed-off-by: Vitor Soares <vitor.soares@synopsys.com> Cc: Boris Brezillon <bbrezillon@kernel.org> Cc: <stable@vger.kernel.org> Cc: <linux-kernel@vger.kernel.org> --- drivers/i3c/master.c | 39 +++++++++++++++++++++++++-------------- 1 file changed, 25 insertions(+), 14 deletions(-)