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) + 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;
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;elsei3cbus->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;elsei3cbus->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;elsei3cbus->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;elsei3cbus->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 😉 I will change the dev_warn() to dev_dbg() due the trace is indeed too much.
Best regards, Vitor Soares
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;elsei3cbus->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 😉 I will change the dev_warn() to dev_dbg() due the trace is indeed too much.
I have the feeling that you endlessly argue on details while the vast majority of changes are okay, which means we both spend a lot of time on things that are not super important.
linux-stable-mirror@lists.linaro.org
-
Boris Brezillon -
Vitor Soares