| Message ID | 47de89f2335930df0ed6903be9afe6de4f46e503.1559821228.git.vitor.soares@synopsys.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Fix i2c and i3c scl rate according bus mode | expand |
On Thu, 6 Jun 2019 16:00:01 +0200 Vitor Soares <Vitor.Soares@synopsys.com> wrote: > 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. > > 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. > > 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> > --- > Changes in v2: > Enhance commit message > Add dev_warn() in case user-defined i2c rate doesn't match LVR constraint > Add dev_warn() in case user-defined i3c rate lower than i2c rate. > > drivers/i3c/master.c | 61 +++++++++++++++++++++++++++++++++++++++++----------- > 1 file changed, 48 insertions(+), 13 deletions(-) > > diff --git a/drivers/i3c/master.c b/drivers/i3c/master.c > index 5f4bd52..8cd5824 100644 > --- a/drivers/i3c/master.c > +++ b/drivers/i3c/master.c > @@ -91,6 +91,12 @@ void i3c_bus_normaluse_unlock(struct i3c_bus *bus) > up_read(&bus->lock); > } > > +static struct i3c_master_controller * > +i3c_bus_to_i3c_master(struct i3c_bus *i3cbus) > +{ > + return container_of(i3cbus, struct i3c_master_controller, bus); > +} > + > static struct i3c_master_controller *dev_to_i3cmaster(struct device *dev) > { > return container_of(dev, struct i3c_master_controller, dev); > @@ -565,20 +571,48 @@ 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 max_i2c_scl_rate) > { > - i3cbus->mode = mode; > > - if (!i3cbus->scl_rate.i3c) > - i3cbus->scl_rate.i3c = I3C_BUS_TYP_I3C_SCL_RATE; > + struct i3c_master_controller *master = i3c_bus_to_i3c_master(i3cbus); > > - 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; > + i3cbus->mode = mode; > + > + 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 = max_i2c_scl_rate; > + break; > + case I3C_BUS_MODE_MIXED_SLOW: > + if (!i3cbus->scl_rate.i2c) > + i3cbus->scl_rate.i2c = max_i2c_scl_rate; > + if (!i3cbus->scl_rate.i3c || > + i3cbus->scl_rate.i3c > i3cbus->scl_rate.i2c) > + i3cbus->scl_rate.i3c = i3cbus->scl_rate.i2c; > + break; > + default: > + return -EINVAL; > } > > + if (i3cbus->scl_rate.i3c < i3cbus->scl_rate.i2c) > + dev_warn(&master->dev, > + "i3c-scl-hz=%ld lower than i2c-scl-hz=%ld\n", > + i3cbus->scl_rate.i3c, i3cbus->scl_rate.i2c); > + > + if (i3cbus->scl_rate.i2c != I3C_BUS_I2C_FM_SCL_RATE && > + i3cbus->scl_rate.i2c != I3C_BUS_I2C_FM_PLUS_SCL_RATE && > + i3cbus->mode != I3C_BUS_MODE_PURE) If you are so strict, there's clearly no point exposing an i2c-scl-hz property. I'm still not convinced having an i2c rate that's slower than what the I2C/I3C spec defines as the *typical* rate is a bad thing, just like I'm not convinced having an I3C rate that's slower than the I2C one is a problem (it's definitely a weird situation, but there's nothing preventing that in the spec). > + dev_warn(&master->dev, > + "i2c-scl-hz=%ld not defined according MIPI I3C spec\n" > + , i3cbus->scl_rate.i2c); The comma should be on the previous line. > + > /* > * I3C/I2C frequency may have been overridden, check that user-provided > * values are not exceeding max possible frequency. > @@ -1966,9 +2000,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); > > @@ -2417,6 +2448,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; > @@ -2466,9 +2498,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; >
From: Boris Brezillon <boris.brezillon@collabora.com> Date: Thu, Jun 06, 2019 at 15:18:44 > On Thu, 6 Jun 2019 16:00:01 +0200 > Vitor Soares <Vitor.Soares@synopsys.com> wrote: > > > 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. > > > > 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. > > > > 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> > > --- > > Changes in v2: > > Enhance commit message > > Add dev_warn() in case user-defined i2c rate doesn't match LVR constraint > > Add dev_warn() in case user-defined i3c rate lower than i2c rate. > > > > drivers/i3c/master.c | 61 +++++++++++++++++++++++++++++++++++++++++----------- > > 1 file changed, 48 insertions(+), 13 deletions(-) > > > > diff --git a/drivers/i3c/master.c b/drivers/i3c/master.c > > index 5f4bd52..8cd5824 100644 > > --- a/drivers/i3c/master.c > > +++ b/drivers/i3c/master.c > > @@ -91,6 +91,12 @@ void i3c_bus_normaluse_unlock(struct i3c_bus *bus) > > up_read(&bus->lock); > > } > > > > +static struct i3c_master_controller * > > +i3c_bus_to_i3c_master(struct i3c_bus *i3cbus) > > +{ > > + return container_of(i3cbus, struct i3c_master_controller, bus); > > +} > > + > > static struct i3c_master_controller *dev_to_i3cmaster(struct device *dev) > > { > > return container_of(dev, struct i3c_master_controller, dev); > > @@ -565,20 +571,48 @@ 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 max_i2c_scl_rate) > > { > > - i3cbus->mode = mode; > > > > - if (!i3cbus->scl_rate.i3c) > > - i3cbus->scl_rate.i3c = I3C_BUS_TYP_I3C_SCL_RATE; > > + struct i3c_master_controller *master = i3c_bus_to_i3c_master(i3cbus); > > > > - 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; > > + i3cbus->mode = mode; > > + > > + 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 = max_i2c_scl_rate; > > + break; > > + case I3C_BUS_MODE_MIXED_SLOW: > > + if (!i3cbus->scl_rate.i2c) > > + i3cbus->scl_rate.i2c = max_i2c_scl_rate; > > + if (!i3cbus->scl_rate.i3c || > > + i3cbus->scl_rate.i3c > i3cbus->scl_rate.i2c) > > + i3cbus->scl_rate.i3c = i3cbus->scl_rate.i2c; > > + break; > > + default: > > + return -EINVAL; > > } > > > > + if (i3cbus->scl_rate.i3c < i3cbus->scl_rate.i2c) > > + dev_warn(&master->dev, > > + "i3c-scl-hz=%ld lower than i2c-scl-hz=%ld\n", > > + i3cbus->scl_rate.i3c, i3cbus->scl_rate.i2c); > > + > > + if (i3cbus->scl_rate.i2c != I3C_BUS_I2C_FM_SCL_RATE && > > + i3cbus->scl_rate.i2c != I3C_BUS_I2C_FM_PLUS_SCL_RATE && > > + i3cbus->mode != I3C_BUS_MODE_PURE) > > If you are so strict, there's clearly no point exposing an i2c-scl-hz > property. I'm still not convinced having an i2c rate that's slower than > what the I2C/I3C spec defines as the *typical* rate is a bad thing, I'm not been strictive, I just inform the user about that case. > just > like I'm not convinced having an I3C rate that's slower than the I2C > one is a problem (it's definitely a weird situation, but there's nothing > preventing that in the spec). You agree that there is no point for case where i3c rate < i2c rate yet you are not convinced. Do you thing that will be users for this case? Anyway, this isn't a high requirement for me. The all point of this patch is to introduce the limited bus configuration. > > > + dev_warn(&master->dev, > > + "i2c-scl-hz=%ld not defined according MIPI I3C spec\n" > > + , i3cbus->scl_rate.i2c); > > The comma should be on the previous line. > > > + > > /* > > * I3C/I2C frequency may have been overridden, check that user-provided > > * values are not exceeding max possible frequency. > > @@ -1966,9 +2000,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); > > > > @@ -2417,6 +2448,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; > > @@ -2466,9 +2498,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 Thu, 6 Jun 2019 17:16:55 +0000 Vitor Soares <Vitor.Soares@synopsys.com> wrote: > From: Boris Brezillon <boris.brezillon@collabora.com> > Date: Thu, Jun 06, 2019 at 15:18:44 > > > On Thu, 6 Jun 2019 16:00:01 +0200 > > Vitor Soares <Vitor.Soares@synopsys.com> wrote: > > > > > 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. > > > > > > 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. > > > > > > 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> > > > --- > > > Changes in v2: > > > Enhance commit message > > > Add dev_warn() in case user-defined i2c rate doesn't match LVR constraint > > > Add dev_warn() in case user-defined i3c rate lower than i2c rate. > > > > > > drivers/i3c/master.c | 61 +++++++++++++++++++++++++++++++++++++++++----------- > > > 1 file changed, 48 insertions(+), 13 deletions(-) > > > > > > diff --git a/drivers/i3c/master.c b/drivers/i3c/master.c > > > index 5f4bd52..8cd5824 100644 > > > --- a/drivers/i3c/master.c > > > +++ b/drivers/i3c/master.c > > > @@ -91,6 +91,12 @@ void i3c_bus_normaluse_unlock(struct i3c_bus *bus) > > > up_read(&bus->lock); > > > } > > > > > > +static struct i3c_master_controller * > > > +i3c_bus_to_i3c_master(struct i3c_bus *i3cbus) > > > +{ > > > + return container_of(i3cbus, struct i3c_master_controller, bus); > > > +} > > > + > > > static struct i3c_master_controller *dev_to_i3cmaster(struct device *dev) > > > { > > > return container_of(dev, struct i3c_master_controller, dev); > > > @@ -565,20 +571,48 @@ 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 max_i2c_scl_rate) > > > { > > > - i3cbus->mode = mode; > > > > > > - if (!i3cbus->scl_rate.i3c) > > > - i3cbus->scl_rate.i3c = I3C_BUS_TYP_I3C_SCL_RATE; > > > + struct i3c_master_controller *master = i3c_bus_to_i3c_master(i3cbus); > > > > > > - 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; > > > + i3cbus->mode = mode; > > > + > > > + 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 = max_i2c_scl_rate; > > > + break; > > > + case I3C_BUS_MODE_MIXED_SLOW: > > > + if (!i3cbus->scl_rate.i2c) > > > + i3cbus->scl_rate.i2c = max_i2c_scl_rate; > > > + if (!i3cbus->scl_rate.i3c || > > > + i3cbus->scl_rate.i3c > i3cbus->scl_rate.i2c) > > > + i3cbus->scl_rate.i3c = i3cbus->scl_rate.i2c; > > > + break; > > > + default: > > > + return -EINVAL; > > > } > > > > > > + if (i3cbus->scl_rate.i3c < i3cbus->scl_rate.i2c) > > > + dev_warn(&master->dev, > > > + "i3c-scl-hz=%ld lower than i2c-scl-hz=%ld\n", > > > + i3cbus->scl_rate.i3c, i3cbus->scl_rate.i2c); > > > + > > > + if (i3cbus->scl_rate.i2c != I3C_BUS_I2C_FM_SCL_RATE && > > > + i3cbus->scl_rate.i2c != I3C_BUS_I2C_FM_PLUS_SCL_RATE && > > > + i3cbus->mode != I3C_BUS_MODE_PURE) > > > > If you are so strict, there's clearly no point exposing an i2c-scl-hz > > property. I'm still not convinced having an i2c rate that's slower than > > what the I2C/I3C spec defines as the *typical* rate is a bad thing, > > I'm not been strictive, I just inform the user about that case. Then use dev_debug() and don't make the trace conditional on i2c_rate != typical_rate. The only case where we should warn users is i2c_rate > typical_rate, because that might lead to malfunctions. > > > just > > like I'm not convinced having an I3C rate that's slower than the I2C > > one is a problem (it's definitely a weird situation, but there's nothing > > preventing that in the spec). > > You agree that there is no point for case where i3c rate < i2c rate yet > you are not convinced. I didn't say that, there might be use cases where one wants to slow down the I3C bus to be able to probe it or use a slower rate when things do not work properly. It's rather unlikely to happen, but I don't think it deserves a warning message when that's the case. > Do you thing that will be users for this case? > > Anyway, this isn't a high requirement for me. The all point of this patch > is to introduce the limited bus configuration. And yet, you keep insisting (and ignoring my feedback) on that point :P.
From: Boris Brezillon <boris.brezillon@collabora.com> Date: Thu, Jun 06, 2019 at 18:35:40 > On Thu, 6 Jun 2019 17:16:55 +0000 > Vitor Soares <Vitor.Soares@synopsys.com> wrote: > > > From: Boris Brezillon <boris.brezillon@collabora.com> > > Date: Thu, Jun 06, 2019 at 15:18:44 > > > > > On Thu, 6 Jun 2019 16:00:01 +0200 > > > Vitor Soares <Vitor.Soares@synopsys.com> wrote: > > > > > > > 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. > > > > > > > > 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. > > > > > > > > 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> > > > > --- > > > > Changes in v2: > > > > Enhance commit message > > > > Add dev_warn() in case user-defined i2c rate doesn't match LVR constraint > > > > Add dev_warn() in case user-defined i3c rate lower than i2c rate. > > > > > > > > drivers/i3c/master.c | 61 +++++++++++++++++++++++++++++++++++++++++----------- > > > > 1 file changed, 48 insertions(+), 13 deletions(-) > > > > > > > > diff --git a/drivers/i3c/master.c b/drivers/i3c/master.c > > > > index 5f4bd52..8cd5824 100644 > > > > --- a/drivers/i3c/master.c > > > > +++ b/drivers/i3c/master.c > > > > @@ -91,6 +91,12 @@ void i3c_bus_normaluse_unlock(struct i3c_bus *bus) > > > > up_read(&bus->lock); > > > > } > > > > > > > > +static struct i3c_master_controller * > > > > +i3c_bus_to_i3c_master(struct i3c_bus *i3cbus) > > > > +{ > > > > + return container_of(i3cbus, struct i3c_master_controller, bus); > > > > +} > > > > + > > > > static struct i3c_master_controller *dev_to_i3cmaster(struct device *dev) > > > > { > > > > return container_of(dev, struct i3c_master_controller, dev); > > > > @@ -565,20 +571,48 @@ 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 max_i2c_scl_rate) > > > > { > > > > - i3cbus->mode = mode; > > > > > > > > - if (!i3cbus->scl_rate.i3c) > > > > - i3cbus->scl_rate.i3c = I3C_BUS_TYP_I3C_SCL_RATE; > > > > + struct i3c_master_controller *master = i3c_bus_to_i3c_master(i3cbus); > > > > > > > > - 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; > > > > + i3cbus->mode = mode; > > > > + > > > > + 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 = max_i2c_scl_rate; > > > > + break; > > > > + case I3C_BUS_MODE_MIXED_SLOW: > > > > + if (!i3cbus->scl_rate.i2c) > > > > + i3cbus->scl_rate.i2c = max_i2c_scl_rate; > > > > + if (!i3cbus->scl_rate.i3c || > > > > + i3cbus->scl_rate.i3c > i3cbus->scl_rate.i2c) > > > > + i3cbus->scl_rate.i3c = i3cbus->scl_rate.i2c; > > > > + break; > > > > + default: > > > > + return -EINVAL; > > > > } > > > > > > > > + if (i3cbus->scl_rate.i3c < i3cbus->scl_rate.i2c) > > > > + dev_warn(&master->dev, > > > > + "i3c-scl-hz=%ld lower than i2c-scl-hz=%ld\n", > > > > + i3cbus->scl_rate.i3c, i3cbus->scl_rate.i2c); > > > > + > > > > + if (i3cbus->scl_rate.i2c != I3C_BUS_I2C_FM_SCL_RATE && > > > > + i3cbus->scl_rate.i2c != I3C_BUS_I2C_FM_PLUS_SCL_RATE && > > > > + i3cbus->mode != I3C_BUS_MODE_PURE) > > > > > > If you are so strict, there's clearly no point exposing an i2c-scl-hz > > > property. I'm still not convinced having an i2c rate that's slower than > > > what the I2C/I3C spec defines as the *typical* rate is a bad thing, > > > > I'm not been strictive, I just inform the user about that case. > > Then use dev_debug() and don't make the trace conditional on > i2c_rate != typical_rate. Ok. I will change to dev_debug(). > The only case where we should warn users > is i2c_rate > typical_rate, because that might lead to malfunctions. Can you explain why? > > > > > > just > > > like I'm not convinced having an I3C rate that's slower than the I2C > > > one is a problem (it's definitely a weird situation, but there's nothing > > > preventing that in the spec). > > > > You agree that there is no point for case where i3c rate < i2c rate yet > > you are not convinced. > > I didn't say that, there might be use cases where one wants to slow > down the I3C bus to be able to probe it or use a slower rate when > things do not work properly. It's rather unlikely to happen, but I > don't think it deserves a warning message when that's the case. > > > Do you thing that will be users for this case? > > > > Anyway, this isn't a high requirement for me. The all point of this patch > > is to introduce the limited bus configuration. > > And yet, you keep insisting (and ignoring my feedback) on that point :P. If you check the previous version you see that I'm trying to follow
On Thu, 6 Jun 2019 18:08:11 +0000 Vitor Soares <Vitor.Soares@synopsys.com> wrote: > From: Boris Brezillon <boris.brezillon@collabora.com> > Date: Thu, Jun 06, 2019 at 18:35:40 > > > On Thu, 6 Jun 2019 17:16:55 +0000 > > Vitor Soares <Vitor.Soares@synopsys.com> wrote: > > > > > From: Boris Brezillon <boris.brezillon@collabora.com> > > > Date: Thu, Jun 06, 2019 at 15:18:44 > > > > > > > On Thu, 6 Jun 2019 16:00:01 +0200 > > > > Vitor Soares <Vitor.Soares@synopsys.com> wrote: > > > > > > > > > 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. > > > > > > > > > > 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. > > > > > > > > > > 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> > > > > > --- > > > > > Changes in v2: > > > > > Enhance commit message > > > > > Add dev_warn() in case user-defined i2c rate doesn't match LVR constraint > > > > > Add dev_warn() in case user-defined i3c rate lower than i2c rate. > > > > > > > > > > drivers/i3c/master.c | 61 +++++++++++++++++++++++++++++++++++++++++----------- > > > > > 1 file changed, 48 insertions(+), 13 deletions(-) > > > > > > > > > > diff --git a/drivers/i3c/master.c b/drivers/i3c/master.c > > > > > index 5f4bd52..8cd5824 100644 > > > > > --- a/drivers/i3c/master.c > > > > > +++ b/drivers/i3c/master.c > > > > > @@ -91,6 +91,12 @@ void i3c_bus_normaluse_unlock(struct i3c_bus *bus) > > > > > up_read(&bus->lock); > > > > > } > > > > > > > > > > +static struct i3c_master_controller * > > > > > +i3c_bus_to_i3c_master(struct i3c_bus *i3cbus) > > > > > +{ > > > > > + return container_of(i3cbus, struct i3c_master_controller, bus); > > > > > +} > > > > > + > > > > > static struct i3c_master_controller *dev_to_i3cmaster(struct device *dev) > > > > > { > > > > > return container_of(dev, struct i3c_master_controller, dev); > > > > > @@ -565,20 +571,48 @@ 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 max_i2c_scl_rate) > > > > > { > > > > > - i3cbus->mode = mode; > > > > > > > > > > - if (!i3cbus->scl_rate.i3c) > > > > > - i3cbus->scl_rate.i3c = I3C_BUS_TYP_I3C_SCL_RATE; > > > > > + struct i3c_master_controller *master = i3c_bus_to_i3c_master(i3cbus); > > > > > > > > > > - 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; > > > > > + i3cbus->mode = mode; > > > > > + > > > > > + 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 = max_i2c_scl_rate; > > > > > + break; > > > > > + case I3C_BUS_MODE_MIXED_SLOW: > > > > > + if (!i3cbus->scl_rate.i2c) > > > > > + i3cbus->scl_rate.i2c = max_i2c_scl_rate; > > > > > + if (!i3cbus->scl_rate.i3c || > > > > > + i3cbus->scl_rate.i3c > i3cbus->scl_rate.i2c) > > > > > + i3cbus->scl_rate.i3c = i3cbus->scl_rate.i2c; > > > > > + break; > > > > > + default: > > > > > + return -EINVAL; > > > > > } > > > > > > > > > > + if (i3cbus->scl_rate.i3c < i3cbus->scl_rate.i2c) > > > > > + dev_warn(&master->dev, > > > > > + "i3c-scl-hz=%ld lower than i2c-scl-hz=%ld\n", > > > > > + i3cbus->scl_rate.i3c, i3cbus->scl_rate.i2c); > > > > > + > > > > > + if (i3cbus->scl_rate.i2c != I3C_BUS_I2C_FM_SCL_RATE && > > > > > + i3cbus->scl_rate.i2c != I3C_BUS_I2C_FM_PLUS_SCL_RATE && > > > > > + i3cbus->mode != I3C_BUS_MODE_PURE) > > > > > > > > If you are so strict, there's clearly no point exposing an i2c-scl-hz > > > > property. I'm still not convinced having an i2c rate that's slower than > > > > what the I2C/I3C spec defines as the *typical* rate is a bad thing, > > > > > > I'm not been strictive, I just inform the user about that case. > > > > Then use dev_debug() and don't make the trace conditional on > > i2c_rate != typical_rate. > > Ok. I will change to dev_debug(). > > > The only case where we should warn users > > is i2c_rate > typical_rate, because that might lead to malfunctions. > > Can you explain why? Because the speed is limited by the device capabilities. Using a slower freq works, driving the SLC line faster than what's supported by I2C slaves doesn't. > > > > > > > > > > just > > > > like I'm not convinced having an I3C rate that's slower than the I2C > > > > one is a problem (it's definitely a weird situation, but there's nothing > > > > preventing that in the spec). > > > > > > You agree that there is no point for case where i3c rate < i2c rate yet > > > you are not convinced. > > > > I didn't say that, there might be use cases where one wants to slow > > down the I3C bus to be able to probe it or use a slower rate when > > things do not work properly. It's rather unlikely to happen, but I > > don't think it deserves a warning message when that's the case. > > > > > Do you thing that will be users for this case? > > > > > > Anyway, this isn't a high requirement for me. The all point of this patch > > > is to introduce the limited bus configuration. > > > > And yet, you keep insisting (and ignoring my feedback) on that point :P. > > If you check the previous version you see that I'm trying to follow
diff --git a/drivers/i3c/master.c b/drivers/i3c/master.c index 5f4bd52..8cd5824 100644 --- a/drivers/i3c/master.c +++ b/drivers/i3c/master.c @@ -91,6 +91,12 @@ void i3c_bus_normaluse_unlock(struct i3c_bus *bus) up_read(&bus->lock); } +static struct i3c_master_controller * +i3c_bus_to_i3c_master(struct i3c_bus *i3cbus) +{ + return container_of(i3cbus, struct i3c_master_controller, bus); +} + static struct i3c_master_controller *dev_to_i3cmaster(struct device *dev) { return container_of(dev, struct i3c_master_controller, dev); @@ -565,20 +571,48 @@ 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 max_i2c_scl_rate) { - i3cbus->mode = mode; - if (!i3cbus->scl_rate.i3c) - i3cbus->scl_rate.i3c = I3C_BUS_TYP_I3C_SCL_RATE; + struct i3c_master_controller *master = i3c_bus_to_i3c_master(i3cbus); - 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; + i3cbus->mode = mode; + + 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 = max_i2c_scl_rate; + break; + case I3C_BUS_MODE_MIXED_SLOW: + if (!i3cbus->scl_rate.i2c) + i3cbus->scl_rate.i2c = max_i2c_scl_rate; + if (!i3cbus->scl_rate.i3c || + i3cbus->scl_rate.i3c > i3cbus->scl_rate.i2c) + i3cbus->scl_rate.i3c = i3cbus->scl_rate.i2c; + break; + default: + return -EINVAL; } + if (i3cbus->scl_rate.i3c < i3cbus->scl_rate.i2c) + dev_warn(&master->dev, + "i3c-scl-hz=%ld lower than i2c-scl-hz=%ld\n", + i3cbus->scl_rate.i3c, i3cbus->scl_rate.i2c); + + if (i3cbus->scl_rate.i2c != I3C_BUS_I2C_FM_SCL_RATE && + i3cbus->scl_rate.i2c != I3C_BUS_I2C_FM_PLUS_SCL_RATE && + i3cbus->mode != I3C_BUS_MODE_PURE) + dev_warn(&master->dev, + "i2c-scl-hz=%ld not defined according MIPI I3C spec\n" + , i3cbus->scl_rate.i2c); + /* * I3C/I2C frequency may have been overridden, check that user-provided * values are not exceeding max possible frequency. @@ -1966,9 +2000,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); @@ -2417,6 +2448,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; @@ -2466,9 +2498,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 the I3C framework limits SCL frequency to FM speed when dealing with a mixed slow bus, even if all I2C devices are FM+ capable. 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. 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> --- Changes in v2: Enhance commit message Add dev_warn() in case user-defined i2c rate doesn't match LVR constraint Add dev_warn() in case user-defined i3c rate lower than i2c rate. drivers/i3c/master.c | 61 +++++++++++++++++++++++++++++++++++++++++----------- 1 file changed, 48 insertions(+), 13 deletions(-)