[PATCH v3 0/3] Fix i2c and i3c scl rate according bus mode

[PATCH v3 0/3] Fix i2c and i3c scl rate according bus mode

[Vitor Soares]

This patch series fix the i2c and i3c scl rate according the bus mode
and LVR register. It also introduce the mixed limited bus for the
cases where i2c devices doesn't have 50 ns filter but allow higher
clock rate for i3c transfers.
Please refer table 5 and 10 of i3c bus spec v1.0 for more detail.

Please follow each patch commit message for more details and changes
made in this version.

Vitor Soares (3):
  i3c: fix i2c and i3c scl rate by bus mode
  i3c: add mixed limited bus mode
  i3c: dw: add limited bus mode support

 drivers/i3c/master.c               | 66 ++++++++++++++++++++++++++++++--------
 drivers/i3c/master/dw-i3c-master.c |  1 +
 include/linux/i3c/master.h         |  5 +++
 3 files changed, 59 insertions(+), 13 deletions(-)

-- 
2.7.4


_______________________________________________
linux-i3c mailing list
linux-i3c@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-i3c

[PATCH v3 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 v3:
  Change dev_warn() to dev_dbg()

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..f8e580e 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_dbg(&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_dbg(&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


_______________________________________________
linux-i3c mailing list
linux-i3c@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-i3c

[PATCH v3 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 v3:
  None

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 f8e580e..025925c 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


_______________________________________________
linux-i3c mailing list
linux-i3c@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-i3c

[PATCH v3 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 v3:
  None

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


_______________________________________________
linux-i3c mailing list
linux-i3c@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-i3c

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

[Boris Brezillon]

On Tue, 11 Jun 2019 16:06:43 +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 v3:
>   Change dev_warn() to dev_dbg()
> 
> 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..f8e580e 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_dbg(&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_dbg(&master->dev,
> +			"i2c-scl-hz=%ld not defined according MIPI I3C spec\n",
> +			i3cbus->scl_rate.i2c);
> +

Again, that's not what I suggested, so I'll write it down:

	dev_dbg(&master->dev, "i2c-scl = %ld Hz i3c-scl = %ld Hz\n",
		i3cbus->scl_rate.i2c, i3cbus->scl_rate.i3c);

dev_dbg() is not printed by default, so it's just fine to have a trace
that prints the I3C and I2C rate unconditionally.

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


_______________________________________________
linux-i3c mailing list
linux-i3c@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-i3c

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

[Vitor Soares]

From: Boris Brezillon <boris.brezillon@collabora.com>
Date: Wed, Jun 12, 2019 at 07:15:33

> On Tue, 11 Jun 2019 16:06:43 +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 v3:
> >   Change dev_warn() to dev_dbg()
> > 
> > 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..f8e580e 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_dbg(&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_dbg(&master->dev,
> > +			"i2c-scl-hz=%ld not defined according MIPI I3C spec\n",
> > +			i3cbus->scl_rate.i2c);
> > +
> 
> Again, that's not what I suggested, so I'll write it down:
> 
> 	dev_dbg(&master->dev, "i2c-scl = %ld Hz i3c-scl = %ld Hz\n",
> 		i3cbus->scl_rate.i2c, i3cbus->scl_rate.i3c);
> 

I'm not ok with that change. The reasons are:
  i3cbus->scl_rate.i3c < i3cbus->scl_rate.i2c is an abnormal use case. As 
discuss early it can be cause by a wrong DT definition or just for 
testing purposes.

  i3cbus->scl_rate.i2c != I3C_BUS_I2C_FM_SCL_RATE && i3cbus->scl_rate.i2c 
!= I3C_BUS_I2C_FM_PLUS_SCL_RATE, the MIPI I3C Spec v1.0 clearly says that 
all I2C devices on the bus shall have a LVR register and thus support FM 
or FM+ modes.
By  definition a FM bus works at 400kHz and a FM+ bus 1MHz.
And for slaves, a FM device works up to 400kHz and a FM+ device works up 
to 1MHz respectively.

Apart of that, if the I2C device support you can use a custom higher or 
lower rate, yet not defined according MIPI I3C spec.

> dev_dbg() is not printed by default, so it's just fine to have a trace
> that prints the I3C and I2C rate unconditionally.

I'm ok to change the way that user is notified and I think that is here 
the problem.
Maybe the best is to change the first dev_dbg() to dev_warn() and the 
second dev_info().

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



_______________________________________________
linux-i3c mailing list
linux-i3c@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-i3c

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

[Boris Brezillon]

On Wed, 12 Jun 2019 11:16:34 +0000
Vitor Soares <Vitor.Soares@synopsys.com> wrote:

> From: Boris Brezillon <boris.brezillon@collabora.com>
> Date: Wed, Jun 12, 2019 at 07:15:33
> 
> > On Tue, 11 Jun 2019 16:06:43 +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 v3:
> > >   Change dev_warn() to dev_dbg()
> > > 
> > > 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..f8e580e 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_dbg(&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_dbg(&master->dev,
> > > +			"i2c-scl-hz=%ld not defined according MIPI I3C spec\n",
> > > +			i3cbus->scl_rate.i2c);
> > > +  
> > 
> > Again, that's not what I suggested, so I'll write it down:
> > 
> > 	dev_dbg(&master->dev, "i2c-scl = %ld Hz i3c-scl = %ld Hz\n",
> > 		i3cbus->scl_rate.i2c, i3cbus->scl_rate.i3c);
> >   
> 
> I'm not ok with that change. The reasons are:
>   i3cbus->scl_rate.i3c < i3cbus->scl_rate.i2c is an abnormal use case. As 
> discuss early it can be cause by a wrong DT definition or just for 
> testing purposes.

Is it buggy, and if it is, what are the symptoms? And I'm not talking
about slow transfers here. Also, note that forcing the I2C/I3C rate
through the DT already means you want to tweak the bus speed (either
for debugging purposes or because slowing things down is needed to fix
a HW bug).

> 
>   i3cbus->scl_rate.i2c != I3C_BUS_I2C_FM_SCL_RATE && i3cbus->scl_rate.i2c 
> != I3C_BUS_I2C_FM_PLUS_SCL_RATE, the MIPI I3C Spec v1.0 clearly says that 
> all I2C devices on the bus shall have a LVR register and thus support FM 
> or FM+ modes.

Yet, you might want to apply a lower I2C freq, and this sounds like a
valid case that doesn't deserve a dev_warn().

> By  definition a FM bus works at 400kHz and a FM+ bus 1MHz.
> And for slaves, a FM device works up to 400kHz and a FM+ device works up 
> to 1MHz respectively.

*up to*, that's the important thing to keep in mind. There's no problem
driving the SCL signal at a lower freq.

> 
> Apart of that, if the I2C device support you can use a custom higher or 
> lower rate, yet not defined according MIPI I3C spec.

I'm not going to have this discussion again, sorry. I think I gave
enough arguments to explain why having an I2C SLC rate that's slower
than what I2C devices support is fine.

> 
> > dev_dbg() is not printed by default, so it's just fine to have a trace
> > that prints the I3C and I2C rate unconditionally.  
> 
> I'm ok to change the way that user is notified and I think that is here 
> the problem.
> Maybe the best is to change the first dev_dbg() to dev_warn() and the 
> second dev_info().

Same here. I'm fine having a dev_warn() when the rate is higher than
what's supported by devices present on the bus (because that case is
buggy), but not when it's lower and still in the valid range.

_______________________________________________
linux-i3c mailing list
linux-i3c@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-i3c

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

[Vitor Soares]

From: Boris Brezillon <boris.brezillon@collabora.com>
Date: Wed, Jun 12, 2019 at 12:37:27

> On Wed, 12 Jun 2019 11:16:34 +0000
> Vitor Soares <Vitor.Soares@synopsys.com> wrote:
> 
> > From: Boris Brezillon <boris.brezillon@collabora.com>
> > Date: Wed, Jun 12, 2019 at 07:15:33
> > 
> > > On Tue, 11 Jun 2019 16:06:43 +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 v3:
> > > >   Change dev_warn() to dev_dbg()
> > > > 
> > > > 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..f8e580e 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_dbg(&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_dbg(&master->dev,
> > > > +			"i2c-scl-hz=%ld not defined according MIPI I3C spec\n",
> > > > +			i3cbus->scl_rate.i2c);
> > > > +  
> > > 
> > > Again, that's not what I suggested, so I'll write it down:
> > > 
> > > 	dev_dbg(&master->dev, "i2c-scl = %ld Hz i3c-scl = %ld Hz\n",
> > > 		i3cbus->scl_rate.i2c, i3cbus->scl_rate.i3c);
> > >   
> > 
> > I'm not ok with that change. The reasons are:
> >   i3cbus->scl_rate.i3c < i3cbus->scl_rate.i2c is an abnormal use case. As 
> > discuss early it can be cause by a wrong DT definition or just for 
> > testing purposes.
> 
> Is it buggy, and if it is, what are the symptoms? And I'm not talking
> about slow transfers here. Also, note that forcing the I2C/I3C rate
> through the DT already means you want to tweak the bus speed (either
> for debugging purposes or because slowing things down is needed to fix
> a HW bug).

Does it need to be buggy to inform the user of such inconsistence?

> 
> > 
> >   i3cbus->scl_rate.i2c != I3C_BUS_I2C_FM_SCL_RATE && i3cbus->scl_rate.i2c 
> > != I3C_BUS_I2C_FM_PLUS_SCL_RATE, the MIPI I3C Spec v1.0 clearly says that 
> > all I2C devices on the bus shall have a LVR register and thus support FM 
> > or FM+ modes.
> 
> Yet, you might want to apply a lower I2C freq, and this sounds like a
> valid case that doesn't deserve a dev_warn().

I already said that I'm ok to change the dev_warn(), you just have to 
tell me what is the best message level to use.

> 
> > By  definition a FM bus works at 400kHz and a FM+ bus 1MHz.
> > And for slaves, a FM device works up to 400kHz and a FM+ device works up 
> > to 1MHz respectively.
> 
> *up to*, that's the important thing to keep in mind. There's no problem
> driving the SCL signal at a lower freq.

We already know that a FM or FM+ supports lower frequencies due backyard 
capabilities.

> 
> > 
> > Apart of that, if the I2C device support you can use a custom higher or 
> > lower rate, yet not defined according MIPI I3C spec.
> 
> I'm not going to have this discussion again, sorry. I think I gave
> enough arguments to explain why having an I2C SLC rate that's slower
> than what I2C devices support is fine.

It is clear to me that the I2C devices on I3C bus shall support FM or 
FM+.
If not they don't follow the MIPI I3C spec and for that reason I prefer 
to inform the user.

> 
> > 
> > > dev_dbg() is not printed by default, so it's just fine to have a trace
> > > that prints the I3C and I2C rate unconditionally.  
> > 
> > I'm ok to change the way that user is notified and I think that is here 
> > the problem.
> > Maybe the best is to change the first dev_dbg() to dev_warn() and the 
> > second dev_info().
> 
> Same here. I'm fine having a dev_warn() when the rate is higher than
> what's supported by devices present on the bus (because that case is
> buggy), but not when it's lower and still in the valid range.

Please take some time to analyze it again, my concern is only to inform 
the user about inconsistencies (forced or not) with the I3C specification 
and I already agreed to change the message levels.

Best regards,
Vitor Soares

_______________________________________________
linux-i3c mailing list
linux-i3c@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-i3c

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

[Boris Brezillon]

On Wed, 12 Jun 2019 13:37:53 +0000
Vitor Soares <Vitor.Soares@synopsys.com> wrote:

> From: Boris Brezillon <boris.brezillon@collabora.com>
> Date: Wed, Jun 12, 2019 at 12:37:27
> 
> > On Wed, 12 Jun 2019 11:16:34 +0000
> > Vitor Soares <Vitor.Soares@synopsys.com> wrote:
> >   
> > > From: Boris Brezillon <boris.brezillon@collabora.com>
> > > Date: Wed, Jun 12, 2019 at 07:15:33
> > >   
> > > > On Tue, 11 Jun 2019 16:06:43 +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 v3:
> > > > >   Change dev_warn() to dev_dbg()
> > > > > 
> > > > > 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..f8e580e 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_dbg(&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_dbg(&master->dev,
> > > > > +			"i2c-scl-hz=%ld not defined according MIPI I3C spec\n",
> > > > > +			i3cbus->scl_rate.i2c);
> > > > > +    
> > > > 
> > > > Again, that's not what I suggested, so I'll write it down:
> > > > 
> > > > 	dev_dbg(&master->dev, "i2c-scl = %ld Hz i3c-scl = %ld Hz\n",
> > > > 		i3cbus->scl_rate.i2c, i3cbus->scl_rate.i3c);
> > > >     
> > > 
> > > I'm not ok with that change. The reasons are:
> > >   i3cbus->scl_rate.i3c < i3cbus->scl_rate.i2c is an abnormal use case. As 
> > > discuss early it can be cause by a wrong DT definition or just for 
> > > testing purposes.  
> > 
> > Is it buggy, and if it is, what are the symptoms? And I'm not talking
> > about slow transfers here. Also, note that forcing the I2C/I3C rate
> > through the DT already means you want to tweak the bus speed (either
> > for debugging purposes or because slowing things down is needed to fix
> > a HW bug).  
> 
> Does it need to be buggy to inform the user of such inconsistence?

It's something forced by the user through a DT property, why do you
think he should be warned about something he decided to explicitly set
to a lower value than what's supposed to be supported. Doesn't sound
like an inconsistency to me.

> 
> >   
> > > 
> > >   i3cbus->scl_rate.i2c != I3C_BUS_I2C_FM_SCL_RATE && i3cbus->scl_rate.i2c 
> > > != I3C_BUS_I2C_FM_PLUS_SCL_RATE, the MIPI I3C Spec v1.0 clearly says that 
> > > all I2C devices on the bus shall have a LVR register and thus support FM 
> > > or FM+ modes.  
> > 
> > Yet, you might want to apply a lower I2C freq, and this sounds like a
> > valid case that doesn't deserve a dev_warn().  
> 
> I already said that I'm ok to change the dev_warn(), you just have to 
> tell me what is the best message level to use.

It's simple: warn about things that can lead to undefined/weird
behavior (rate higher than what the spec allows), do nothing otherwise.
You can add a dev_dbg() message that prints both I3C and I2C rates
unconditionally so that users can still see it in the kernel logs if
they really want (that requires enabling dynamic-printk or compiling
the core with -DDEBUG).

> 
> >   
> > > By  definition a FM bus works at 400kHz and a FM+ bus 1MHz.
> > > And for slaves, a FM device works up to 400kHz and a FM+ device works up 
> > > to 1MHz respectively.  
> > 
> > *up to*, that's the important thing to keep in mind. There's no problem
> > driving the SCL signal at a lower freq.  
> 
> We already know that a FM or FM+ supports lower frequencies due backyard 
> capabilities.

So, you agree that running at a lower freq is a valid use case.

> 
> >   
> > > 
> > > Apart of that, if the I2C device support you can use a custom higher or 
> > > lower rate, yet not defined according MIPI I3C spec.  
> > 
> > I'm not going to have this discussion again, sorry. I think I gave
> > enough arguments to explain why having an I2C SLC rate that's slower
> > than what I2C devices support is fine.  
> 
> It is clear to me that the I2C devices on I3C bus shall support FM or 
> FM+.
> If not they don't follow the MIPI I3C spec and for that reason I prefer 
> to inform the user.

I'll try to explain it again. It's not about what devices support but
what the user decides to force. You can have super fast devs on a
crappy bus that can't run at full speed because of X/Y reasons (can be
some problems at the board level preventing it from running at full
speed in a reliable way). In that case, the user forces a rate for
I3C/I2C and there's nothing to complain about *unless* this new rate is
above the max limit (or below the min limit, but I don't think there's
a min in our case).

If things are well designed and/or you're not trying to artificially
lower the bus speed (to probe it?), you shouldn't have the
i3c/i2c-scl-hz props defined in the first place and all the things
we're talking about are irrelevant.

> 
> >   
> > >   
> > > > dev_dbg() is not printed by default, so it's just fine to have a trace
> > > > that prints the I3C and I2C rate unconditionally.    
> > > 
> > > I'm ok to change the way that user is notified and I think that is here 
> > > the problem.
> > > Maybe the best is to change the first dev_dbg() to dev_warn() and the 
> > > second dev_info().  
> > 
> > Same here. I'm fine having a dev_warn() when the rate is higher than
> > what's supported by devices present on the bus (because that case is
> > buggy), but not when it's lower and still in the valid range.  
> 
> Please take some time to analyze it again, my concern is only to inform 
> the user about inconsistencies (forced or not) with the I3C specification 
> and I already agreed to change the message levels.

If you only want to inform the user about the selected rates, you
should just print them unconditionally, and given that messages in the
probe path are normally reserved for errors (see [1]), you should use
dev_dbg() for that.

[1]https://lkml.org/lkml/2019/6/4/1053

_______________________________________________
linux-i3c mailing list
linux-i3c@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-i3c