[PATCH v2 1/3] i3c: fix i2c and i3c scl rate by bus mode

[PATCH v2 1/3] i3c: fix i2c and i3c scl rate by bus mode

[Vitor Soares]

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;
 
-- 
2.7.4

[PATCH v2 2/3] i3c: add mixed limited bus mode

[Vitor Soares]

The i3c bus spec defines a bus configuration where i2c devices don't
have a 50ns filter but support SCL running at SDR max rate (12.5MHz).

This patch introduces the limited bus mode so that users can use
a higher speed in presence of i2c devices index 1.

Signed-off-by: Vitor Soares <vitor.soares@synopsys.com>
Cc: Boris Brezillon <bbrezillon@kernel.org>
Cc: <linux-kernel@vger.kernel.org>
---
Changes in v2:
  Enhance commit message

 drivers/i3c/master.c       | 5 +++++
 include/linux/i3c/master.h | 5 +++++
 2 files changed, 10 insertions(+)

diff --git a/drivers/i3c/master.c b/drivers/i3c/master.c
index 8cd5824..f446c4d 100644
--- a/drivers/i3c/master.c
+++ b/drivers/i3c/master.c
@@ -470,6 +470,7 @@ static int i3c_bus_init(struct i3c_bus *i3cbus)
 static const char * const i3c_bus_mode_strings[] = {
 	[I3C_BUS_MODE_PURE] = "pure",
 	[I3C_BUS_MODE_MIXED_FAST] = "mixed-fast",
+	[I3C_BUS_MODE_MIXED_LIMITED] = "mixed-limited",
 	[I3C_BUS_MODE_MIXED_SLOW] = "mixed-slow",
 };
 
@@ -585,6 +586,7 @@ int i3c_bus_set_mode(struct i3c_bus *i3cbus, enum i3c_bus_mode mode,
 			i3cbus->scl_rate.i3c = I3C_BUS_TYP_I3C_SCL_RATE;
 		break;
 	case I3C_BUS_MODE_MIXED_FAST:
+	case I3C_BUS_MODE_MIXED_LIMITED:
 		if (!i3cbus->scl_rate.i3c)
 			i3cbus->scl_rate.i3c = I3C_BUS_TYP_I3C_SCL_RATE;
 		if (!i3cbus->scl_rate.i2c)
@@ -2490,6 +2492,9 @@ int i3c_master_register(struct i3c_master_controller *master,
 				mode = I3C_BUS_MODE_MIXED_FAST;
 			break;
 		case I3C_LVR_I2C_INDEX(1):
+			if (mode < I3C_BUS_MODE_MIXED_LIMITED)
+				mode = I3C_BUS_MODE_MIXED_LIMITED;
+			break;
 		case I3C_LVR_I2C_INDEX(2):
 			if (mode < I3C_BUS_MODE_MIXED_SLOW)
 				mode = I3C_BUS_MODE_MIXED_SLOW;
diff --git a/include/linux/i3c/master.h b/include/linux/i3c/master.h
index f13fd8b..89ea461 100644
--- a/include/linux/i3c/master.h
+++ b/include/linux/i3c/master.h
@@ -250,12 +250,17 @@ struct i3c_device {
  *			     the bus. The only impact in this mode is that the
  *			     high SCL pulse has to stay below 50ns to trick I2C
  *			     devices when transmitting I3C frames
+ * @I3C_BUS_MODE_MIXED_LIMITED: I2C devices without 50ns spike filter are
+ *				present on the bus. However they allows
+ *				compliance up to the maximum SDR SCL clock
+ *				frequency.
  * @I3C_BUS_MODE_MIXED_SLOW: I2C devices without 50ns spike filter are present
  *			     on the bus
  */
 enum i3c_bus_mode {
 	I3C_BUS_MODE_PURE,
 	I3C_BUS_MODE_MIXED_FAST,
+	I3C_BUS_MODE_MIXED_LIMITED,
 	I3C_BUS_MODE_MIXED_SLOW,
 };
 
-- 
2.7.4

[PATCH v2 3/3] i3c: dw: add limited bus mode support

[Vitor Soares]

This patch add limited bus mode support for DesignWare i3c master

Signed-off-by: Vitor Soares <vitor.soares@synopsys.com>
Cc: Boris Brezillon <bbrezillon@kernel.org>
Cc: <linux-kernel@vger.kernel.org>
---
Changes in v2:
  None

 drivers/i3c/master/dw-i3c-master.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/i3c/master/dw-i3c-master.c b/drivers/i3c/master/dw-i3c-master.c
index 1d83c97..9612d93 100644
--- a/drivers/i3c/master/dw-i3c-master.c
+++ b/drivers/i3c/master/dw-i3c-master.c
@@ -599,6 +599,7 @@ static int dw_i3c_master_bus_init(struct i3c_master_controller *m)
 
 	switch (bus->mode) {
 	case I3C_BUS_MODE_MIXED_FAST:
+	case I3C_BUS_MODE_MIXED_LIMITED:
 		ret = dw_i2c_clk_cfg(master);
 		if (ret)
 			return ret;
-- 
2.7.4

Re: [PATCH v2 1/3] i3c: fix i2c and i3c scl rate by bus mode

[Boris Brezillon]

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;
>  

RE: [PATCH v2 1/3] i3c: fix i2c and i3c scl rate by bus mode

[Vitor Soares]

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

Re: [PATCH v2 1/3] i3c: fix i2c and i3c scl rate by bus mode

[Boris Brezillon]

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.

RE: [PATCH v2 1/3] i3c: fix i2c and i3c scl rate by bus mode

[Vitor Soares]

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 😉
I will change the dev_warn() to dev_dbg() due the trace is indeed too 
much.

Best regards,
Vitor Soares

Re: [PATCH v2 1/3] i3c: fix i2c and i3c scl rate by bus mode

[Boris Brezillon]

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 😉
> 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.