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

[PATCH 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.

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               | 44 ++++++++++++++++++++++++++------------
 drivers/i3c/master/dw-i3c-master.c |  1 +
 include/linux/i3c/master.h         |  5 +++++
 3 files changed, 36 insertions(+), 14 deletions(-)

-- 
2.7.4


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

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

[Vitor Soares]

Currently in case of mixed slow bus topologie and all i2c devices
support FM+ speed, the i3c subsystem limite the SCL to FM speed.
Also in case on mixed slow bus mode the max speed for both
i2c or i3c transfers is FM or FM+.

This patch fix the definition of i2c and i3c scl rate based on bus
topologie and LVR[4] if no user input.
In case of mixed slow mode the i3c scl rate is overridden.

Fixes: 3a379bbcea0a ("i3c: Add core I3C infrastructure")
Signed-off-by: Vitor Soares <vitor.soares@synopsys.com>
Cc: Boris Brezillon <bbrezillon@kernel.org>
Cc: <stable@vger.kernel.org>
Cc: <linux-kernel@vger.kernel.org>
---
 drivers/i3c/master.c | 39 +++++++++++++++++++++++++--------------
 1 file changed, 25 insertions(+), 14 deletions(-)

diff --git a/drivers/i3c/master.c b/drivers/i3c/master.c
index 909c2ad..1c4a86a 100644
--- a/drivers/i3c/master.c
+++ b/drivers/i3c/master.c
@@ -564,20 +564,30 @@ static const struct device_type i3c_masterdev_type = {
 	.groups	= i3c_masterdev_groups,
 };
 
-int i3c_bus_set_mode(struct i3c_bus *i3cbus, enum i3c_bus_mode mode)
+int i3c_bus_set_mode(struct i3c_bus *i3cbus, enum i3c_bus_mode mode,
+		     unsigned long i2c_scl_rate)
 {
 	i3cbus->mode = mode;
 
-	if (!i3cbus->scl_rate.i3c)
-		i3cbus->scl_rate.i3c = I3C_BUS_TYP_I3C_SCL_RATE;
-
-	if (!i3cbus->scl_rate.i2c) {
-		if (i3cbus->mode == I3C_BUS_MODE_MIXED_SLOW)
-			i3cbus->scl_rate.i2c = I3C_BUS_I2C_FM_SCL_RATE;
-		else
-			i3cbus->scl_rate.i2c = I3C_BUS_I2C_FM_PLUS_SCL_RATE;
+	switch (i3cbus->mode) {
+	case I3C_BUS_MODE_PURE:
+		if (!i3cbus->scl_rate.i3c)
+			i3cbus->scl_rate.i3c = I3C_BUS_TYP_I3C_SCL_RATE;
+		break;
+	case I3C_BUS_MODE_MIXED_FAST:
+		if (!i3cbus->scl_rate.i3c)
+			i3cbus->scl_rate.i3c = I3C_BUS_TYP_I3C_SCL_RATE;
+		if (!i3cbus->scl_rate.i2c)
+			i3cbus->scl_rate.i2c = i2c_scl_rate;
+		break;
+	case I3C_BUS_MODE_MIXED_SLOW:
+		if (!i3cbus->scl_rate.i2c)
+			i3cbus->scl_rate.i2c = i2c_scl_rate;
+		i3cbus->scl_rate.i3c = i3cbus->scl_rate.i2c;
+		break;
+	default:
+		return -EINVAL;
 	}
-
 	/*
 	 * I3C/I2C frequency may have been overridden, check that user-provided
 	 * values are not exceeding max possible frequency.
@@ -1980,9 +1990,6 @@ of_i3c_master_add_i2c_boardinfo(struct i3c_master_controller *master,
 	/* LVR is encoded in reg[2]. */
 	boardinfo->lvr = reg[2];
 
-	if (boardinfo->lvr & I3C_LVR_I2C_FM_MODE)
-		master->bus.scl_rate.i2c = I3C_BUS_I2C_FM_SCL_RATE;
-
 	list_add_tail(&boardinfo->node, &master->boardinfo.i2c);
 	of_node_get(node);
 
@@ -2432,6 +2439,7 @@ int i3c_master_register(struct i3c_master_controller *master,
 			const struct i3c_master_controller_ops *ops,
 			bool secondary)
 {
+	unsigned long i2c_scl_rate = I3C_BUS_I2C_FM_PLUS_SCL_RATE;
 	struct i3c_bus *i3cbus = i3c_master_get_bus(master);
 	enum i3c_bus_mode mode = I3C_BUS_MODE_PURE;
 	struct i2c_dev_boardinfo *i2cbi;
@@ -2481,9 +2489,12 @@ int i3c_master_register(struct i3c_master_controller *master,
 			ret = -EINVAL;
 			goto err_put_dev;
 		}
+
+		if (i2cbi->lvr & I3C_LVR_I2C_FM_MODE)
+			i2c_scl_rate = I3C_BUS_I2C_FM_SCL_RATE;
 	}
 
-	ret = i3c_bus_set_mode(i3cbus, mode);
+	ret = i3c_bus_set_mode(i3cbus, mode, i2c_scl_rate);
 	if (ret)
 		goto err_put_dev;
 
-- 
2.7.4


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

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

[Vitor Soares]

The i3c bus spec define a bus configuration where the i2c devices
doesn't have the 50ns filter yet they allow the SDR max speed.

This patch introduce the limited bus mode so the users can use
a higher speed on 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>
---
 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 1c4a86a..46d3774 100644
--- a/drivers/i3c/master.c
+++ b/drivers/i3c/master.c
@@ -463,6 +463,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",
 };
 
@@ -575,6 +576,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)
@@ -2481,6 +2483,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 44fb3cf..740235e 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 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>
---
 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 0b01607..4005508 100644
--- a/drivers/i3c/master/dw-i3c-master.c
+++ b/drivers/i3c/master/dw-i3c-master.c
@@ -650,6 +650,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 1/3] i3c: fix i2c and i3c scl rate by bus mode

[Boris Brezillon]

On Mon, 15 Apr 2019 20:46:41 +0200
Vitor Soares <vitor.soares@synopsys.com> wrote:

> Currently in case of mixed slow bus topologie and all i2c devices
> support FM+ speed, the i3c subsystem limite the SCL to FM speed.

"
Currently the I3C framework limits SCL frequency to FM speed when
dealing with a mixed slow bus, even if all I2C devices are FM+ capable.
"

> Also in case on mixed slow bus mode the max speed for both
> i2c or i3c transfers is FM or FM+.

Looks like you're basically repeating what you said above.

> 
> This patch fix the definition of i2c and i3c scl rate based on bus

	     ^fixes					      on the bus

> topologie and LVR[4] if no user input.

  ^topology		^if the rate is not already specified by the user.

> In case of mixed slow mode the i3c scl rate is overridden.

							   ^ with the max
I2C rate.

> 
> Fixes: 3a379bbcea0a ("i3c: Add core I3C infrastructure")
> Signed-off-by: Vitor Soares <vitor.soares@synopsys.com>
> Cc: Boris Brezillon <bbrezillon@kernel.org>
> Cc: <stable@vger.kernel.org>
> Cc: <linux-kernel@vger.kernel.org>
> ---
>  drivers/i3c/master.c | 39 +++++++++++++++++++++++++--------------
>  1 file changed, 25 insertions(+), 14 deletions(-)
> 
> diff --git a/drivers/i3c/master.c b/drivers/i3c/master.c
> index 909c2ad..1c4a86a 100644
> --- a/drivers/i3c/master.c
> +++ b/drivers/i3c/master.c
> @@ -564,20 +564,30 @@ static const struct device_type i3c_masterdev_type = {
>  	.groups	= i3c_masterdev_groups,
>  };
>  
> -int i3c_bus_set_mode(struct i3c_bus *i3cbus, enum i3c_bus_mode mode)
> +int i3c_bus_set_mode(struct i3c_bus *i3cbus, enum i3c_bus_mode mode,
> +		     unsigned long i2c_scl_rate)


Can we rename the last arg into max_i2c_scl_rate?

>  {
>  	i3cbus->mode = mode;
>  
> -	if (!i3cbus->scl_rate.i3c)
> -		i3cbus->scl_rate.i3c = I3C_BUS_TYP_I3C_SCL_RATE;
> -
> -	if (!i3cbus->scl_rate.i2c) {
> -		if (i3cbus->mode == I3C_BUS_MODE_MIXED_SLOW)
> -			i3cbus->scl_rate.i2c = I3C_BUS_I2C_FM_SCL_RATE;
> -		else
> -			i3cbus->scl_rate.i2c = I3C_BUS_I2C_FM_PLUS_SCL_RATE;
> +	switch (i3cbus->mode) {
> +	case I3C_BUS_MODE_PURE:
> +		if (!i3cbus->scl_rate.i3c)
> +			i3cbus->scl_rate.i3c = I3C_BUS_TYP_I3C_SCL_RATE;
> +		break;
> +	case I3C_BUS_MODE_MIXED_FAST:
> +		if (!i3cbus->scl_rate.i3c)
> +			i3cbus->scl_rate.i3c = I3C_BUS_TYP_I3C_SCL_RATE;
> +		if (!i3cbus->scl_rate.i2c)
> +			i3cbus->scl_rate.i2c = i2c_scl_rate;
> +		break;
> +	case I3C_BUS_MODE_MIXED_SLOW:
> +		if (!i3cbus->scl_rate.i2c)
> +			i3cbus->scl_rate.i2c = i2c_scl_rate;
> +		i3cbus->scl_rate.i3c = i3cbus->scl_rate.i2c;

Maybe we should do

		if (!i3cbus->scl_rate.i3c ||
		    i3cbus->scl_rate.i3c > i3cbus->scl_rate.i2c)
			i3cbus->scl_rate.i3c = i3cbus->scl_rate.i2c;
					   
Just in case the I3C rate forced by the user is lower than the max I2C
rate.

The patch looks good otherwise.

> +		break;
> +	default:
> +		return -EINVAL;
>  	}
> -
>  	/*
>  	 * I3C/I2C frequency may have been overridden, check that user-provided
>  	 * values are not exceeding max possible frequency.
> @@ -1980,9 +1990,6 @@ of_i3c_master_add_i2c_boardinfo(struct i3c_master_controller *master,
>  	/* LVR is encoded in reg[2]. */
>  	boardinfo->lvr = reg[2];
>  
> -	if (boardinfo->lvr & I3C_LVR_I2C_FM_MODE)
> -		master->bus.scl_rate.i2c = I3C_BUS_I2C_FM_SCL_RATE;
> -
>  	list_add_tail(&boardinfo->node, &master->boardinfo.i2c);
>  	of_node_get(node);
>  
> @@ -2432,6 +2439,7 @@ int i3c_master_register(struct i3c_master_controller *master,
>  			const struct i3c_master_controller_ops *ops,
>  			bool secondary)
>  {
> +	unsigned long i2c_scl_rate = I3C_BUS_I2C_FM_PLUS_SCL_RATE;
>  	struct i3c_bus *i3cbus = i3c_master_get_bus(master);
>  	enum i3c_bus_mode mode = I3C_BUS_MODE_PURE;
>  	struct i2c_dev_boardinfo *i2cbi;
> @@ -2481,9 +2489,12 @@ int i3c_master_register(struct i3c_master_controller *master,
>  			ret = -EINVAL;
>  			goto err_put_dev;
>  		}
> +
> +		if (i2cbi->lvr & I3C_LVR_I2C_FM_MODE)
> +			i2c_scl_rate = I3C_BUS_I2C_FM_SCL_RATE;
>  	}
>  
> -	ret = i3c_bus_set_mode(i3cbus, mode);
> +	ret = i3c_bus_set_mode(i3cbus, mode, i2c_scl_rate);
>  	if (ret)
>  		goto err_put_dev;
>  


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

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

[Boris Brezillon]

Hi Vitor,

On Mon, 15 Apr 2019 20:46:42 +0200
Vitor Soares <vitor.soares@synopsys.com> wrote:

> The i3c bus spec define a bus configuration where the i2c devices

		   ^defines			    I2C devices...

> doesn't have the 50ns filter yet they allow the SDR max speed.

  ^don't	^ a 50ns filter but support SCL running at SDR max
rate (12MHz).

> 
> This patch introduce the limited bus mode so the users can use

	     ^introduces		    ^ so that users

> a higher speed on presence of i2c devices index 1.

		 ^in

> 
> Signed-off-by: Vitor Soares <vitor.soares@synopsys.com>
> Cc: Boris Brezillon <bbrezillon@kernel.org>
> Cc: <linux-kernel@vger.kernel.org>
> ---
>  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 1c4a86a..46d3774 100644
> --- a/drivers/i3c/master.c
> +++ b/drivers/i3c/master.c
> @@ -463,6 +463,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",
>  };
>  
> @@ -575,6 +576,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)
> @@ -2481,6 +2483,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 44fb3cf..740235e 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.

						    However they support
				SCL clock running at maximum SDR rate
				(12.5MHz).

>   * @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,
>  };
>  

The code itself looks good.

Thanks,

Boris

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

Re: [PATCH 3/3] i3c: dw: Add limited bus mode support

[Boris Brezillon]

+Przemek

On Mon, 15 Apr 2019 20:46:43 +0200
Vitor Soares <vitor.soares@synopsys.com> wrote:

> 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>
> ---
>  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 0b01607..4005508 100644
> --- a/drivers/i3c/master/dw-i3c-master.c
> +++ b/drivers/i3c/master/dw-i3c-master.c
> @@ -650,6 +650,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;

Przemek, you might want to do the same thing in the Cadence driver.

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

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

[Vitor Soares]

Hi Boris,

From: Boris Brezillon <boris.brezillon@collabora.com>
Date: Tue, Apr 16, 2019 at 06:50:41

> On Mon, 15 Apr 2019 20:46:41 +0200
> Vitor Soares <vitor.soares@synopsys.com> wrote:
> 
> > Currently in case of mixed slow bus topologie and all i2c devices
> > support FM+ speed, the i3c subsystem limite the SCL to FM speed.
> 

I will it replace with your message below.

> "
> Currently the I3C framework limits SCL frequency to FM speed when
> dealing with a mixed slow bus, even if all I2C devices are FM+ capable.
> "
> 
> > Also in case on mixed slow bus mode the max speed for both
> > i2c or i3c transfers is FM or FM+.
> 
> Looks like you're basically repeating what you said above.

What I meant was that I3C framework isn't limiting the I3C speed in case 
of mixed slow bus.

> 
> > 
> > This patch fix the definition of i2c and i3c scl rate based on bus
> 
> 	     ^fixes					      on the bus
> 
> > topologie and LVR[4] if no user input.
> 
>   ^topology		^if the rate is not already specified by the user.
> 
> > In case of mixed slow mode the i3c scl rate is overridden.
> 
> 							   ^ with the max
> I2C rate.
> 
> > 
> > Fixes: 3a379bbcea0a ("i3c: Add core I3C infrastructure")
> > Signed-off-by: Vitor Soares <vitor.soares@synopsys.com>
> > Cc: Boris Brezillon <bbrezillon@kernel.org>
> > Cc: <stable@vger.kernel.org>
> > Cc: <linux-kernel@vger.kernel.org>
> > ---
> >  drivers/i3c/master.c | 39 +++++++++++++++++++++++++--------------
> >  1 file changed, 25 insertions(+), 14 deletions(-)
> > 
> > diff --git a/drivers/i3c/master.c b/drivers/i3c/master.c
> > index 909c2ad..1c4a86a 100644
> > --- a/drivers/i3c/master.c
> > +++ b/drivers/i3c/master.c
> > @@ -564,20 +564,30 @@ static const struct device_type i3c_masterdev_type = {
> >  	.groups	= i3c_masterdev_groups,
> >  };
> >  
> > -int i3c_bus_set_mode(struct i3c_bus *i3cbus, enum i3c_bus_mode mode)
> > +int i3c_bus_set_mode(struct i3c_bus *i3cbus, enum i3c_bus_mode mode,
> > +		     unsigned long i2c_scl_rate)
> 
> 
> Can we rename the last arg into max_i2c_scl_rate?

The i2c_scl_rate is base on LVR[4] bit and the user can set a higher scl 
rate, this is reason I didn't named it to max_i2c_scl_rate.
But if you think that make more sense I'm ok with that.

> 
> >  {
> >  	i3cbus->mode = mode;
> >  
> > -	if (!i3cbus->scl_rate.i3c)
> > -		i3cbus->scl_rate.i3c = I3C_BUS_TYP_I3C_SCL_RATE;
> > -
> > -	if (!i3cbus->scl_rate.i2c) {
> > -		if (i3cbus->mode == I3C_BUS_MODE_MIXED_SLOW)
> > -			i3cbus->scl_rate.i2c = I3C_BUS_I2C_FM_SCL_RATE;
> > -		else
> > -			i3cbus->scl_rate.i2c = I3C_BUS_I2C_FM_PLUS_SCL_RATE;
> > +	switch (i3cbus->mode) {
> > +	case I3C_BUS_MODE_PURE:
> > +		if (!i3cbus->scl_rate.i3c)
> > +			i3cbus->scl_rate.i3c = I3C_BUS_TYP_I3C_SCL_RATE;
> > +		break;
> > +	case I3C_BUS_MODE_MIXED_FAST:
> > +		if (!i3cbus->scl_rate.i3c)
> > +			i3cbus->scl_rate.i3c = I3C_BUS_TYP_I3C_SCL_RATE;
> > +		if (!i3cbus->scl_rate.i2c)
> > +			i3cbus->scl_rate.i2c = i2c_scl_rate;
> > +		break;
> > +	case I3C_BUS_MODE_MIXED_SLOW:
> > +		if (!i3cbus->scl_rate.i2c)
> > +			i3cbus->scl_rate.i2c = i2c_scl_rate;
> > +		i3cbus->scl_rate.i3c = i3cbus->scl_rate.i2c;
> 
> Maybe we should do
> 
> 		if (!i3cbus->scl_rate.i3c ||
> 		    i3cbus->scl_rate.i3c > i3cbus->scl_rate.i2c)
> 			i3cbus->scl_rate.i3c = i3cbus->scl_rate.i2c;
> 					   
> Just in case the I3C rate forced by the user is lower than the max I2C
> rate.

That was something that I considered but TBH it isn't a real use case.

> 
> The patch looks good otherwise.

Thanks.

> 
> > +		break;
> > +	default:
> > +		return -EINVAL;
> >  	}
> > -
> >  	/*
> >  	 * I3C/I2C frequency may have been overridden, check that user-provided
> >  	 * values are not exceeding max possible frequency.
> > @@ -1980,9 +1990,6 @@ of_i3c_master_add_i2c_boardinfo(struct i3c_master_controller *master,
> >  	/* LVR is encoded in reg[2]. */
> >  	boardinfo->lvr = reg[2];
> >  
> > -	if (boardinfo->lvr & I3C_LVR_I2C_FM_MODE)
> > -		master->bus.scl_rate.i2c = I3C_BUS_I2C_FM_SCL_RATE;
> > -
> >  	list_add_tail(&boardinfo->node, &master->boardinfo.i2c);
> >  	of_node_get(node);
> >  
> > @@ -2432,6 +2439,7 @@ int i3c_master_register(struct i3c_master_controller *master,
> >  			const struct i3c_master_controller_ops *ops,
> >  			bool secondary)
> >  {
> > +	unsigned long i2c_scl_rate = I3C_BUS_I2C_FM_PLUS_SCL_RATE;
> >  	struct i3c_bus *i3cbus = i3c_master_get_bus(master);
> >  	enum i3c_bus_mode mode = I3C_BUS_MODE_PURE;
> >  	struct i2c_dev_boardinfo *i2cbi;
> > @@ -2481,9 +2489,12 @@ int i3c_master_register(struct i3c_master_controller *master,
> >  			ret = -EINVAL;
> >  			goto err_put_dev;
> >  		}
> > +
> > +		if (i2cbi->lvr & I3C_LVR_I2C_FM_MODE)
> > +			i2c_scl_rate = I3C_BUS_I2C_FM_SCL_RATE;
> >  	}
> >  
> > -	ret = i3c_bus_set_mode(i3cbus, mode);
> > +	ret = i3c_bus_set_mode(i3cbus, mode, i2c_scl_rate);
> >  	if (ret)
> >  		goto err_put_dev;
> >  

Best regards,
Vitor Soares

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

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

[Vitor Soares]

Hi Boris,

From: Boris Brezillon <boris.brezillon@collabora.com>
Date: Tue, Apr 16, 2019 at 07:00:49

> Hi Vitor,
> 
> On Mon, 15 Apr 2019 20:46:42 +0200
> Vitor Soares <vitor.soares@synopsys.com> wrote:
> 
> > The i3c bus spec define a bus configuration where the i2c devices
> 
> 		   ^defines			    I2C devices...
> 
> > doesn't have the 50ns filter yet they allow the SDR max speed.
> 
>   ^don't	^ a 50ns filter but support SCL running at SDR max
> rate (12MHz).
> 
> > 
> > This patch introduce the limited bus mode so the users can use
> 
> 	     ^introduces		    ^ so that users
> 
> > a higher speed on presence of i2c devices index 1.
> 
> 		 ^in
> 
> > 
> > Signed-off-by: Vitor Soares <vitor.soares@synopsys.com>
> > Cc: Boris Brezillon <bbrezillon@kernel.org>
> > Cc: <linux-kernel@vger.kernel.org>
> > ---
> >  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 1c4a86a..46d3774 100644
> > --- a/drivers/i3c/master.c
> > +++ b/drivers/i3c/master.c
> > @@ -463,6 +463,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",
> >  };
> >  
> > @@ -575,6 +576,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)
> > @@ -2481,6 +2483,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 44fb3cf..740235e 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.
> 
> 						    However they support
> 				SCL clock running at maximum SDR rate
> 				(12.5MHz).
> 
> >   * @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,
> >  };
> >  
> 
> The code itself looks good.
> 
> Thanks,
> 
> Boris

Thanks for your feedback I will address the fixes next version.


Best regards,
Vitor Soares



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

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

[Boris Brezillon]

On Tue, 16 Apr 2019 14:24:57 +0000
Vitor Soares <vitor.soares@synopsys.com> wrote:

> Hi Boris,
> 
> From: Boris Brezillon <boris.brezillon@collabora.com>
> Date: Tue, Apr 16, 2019 at 06:50:41
> 
> > On Mon, 15 Apr 2019 20:46:41 +0200
> > Vitor Soares <vitor.soares@synopsys.com> wrote:
> >   
> > > Currently in case of mixed slow bus topologie and all i2c devices
> > > support FM+ speed, the i3c subsystem limite the SCL to FM speed.  
> >   
> 
> I will it replace with your message below.
> 
> > "
> > Currently the I3C framework limits SCL frequency to FM speed when
> > dealing with a mixed slow bus, even if all I2C devices are FM+ capable.
> > "
> >   
> > > Also in case on mixed slow bus mode the max speed for both
> > > i2c or i3c transfers is FM or FM+.  
> > 
> > Looks like you're basically repeating what you said above.  
> 
> What I meant was that I3C framework isn't limiting the I3C speed in case 
> of mixed slow bus.

Oh, okay, then maybe something like

"
The core was also not accounting for I3C speed limitations when
operating in mixed slow mode and was erroneously using FM+ speed as the
max I2C speed when operating in mixed fast mode.
"

> 
> >   
> > > 
> > > This patch fix the definition of i2c and i3c scl rate based on bus  
> > 
> > 	     ^fixes					      on the bus
> >   
> > > topologie and LVR[4] if no user input.  
> > 
> >   ^topology		^if the rate is not already specified by the user.
> >   
> > > In case of mixed slow mode the i3c scl rate is overridden.  
> > 
> > 							   ^ with the max
> > I2C rate.
> >   
> > > 
> > > Fixes: 3a379bbcea0a ("i3c: Add core I3C infrastructure")
> > > Signed-off-by: Vitor Soares <vitor.soares@synopsys.com>
> > > Cc: Boris Brezillon <bbrezillon@kernel.org>
> > > Cc: <stable@vger.kernel.org>
> > > Cc: <linux-kernel@vger.kernel.org>
> > > ---
> > >  drivers/i3c/master.c | 39 +++++++++++++++++++++++++--------------
> > >  1 file changed, 25 insertions(+), 14 deletions(-)
> > > 
> > > diff --git a/drivers/i3c/master.c b/drivers/i3c/master.c
> > > index 909c2ad..1c4a86a 100644
> > > --- a/drivers/i3c/master.c
> > > +++ b/drivers/i3c/master.c
> > > @@ -564,20 +564,30 @@ static const struct device_type i3c_masterdev_type = {
> > >  	.groups	= i3c_masterdev_groups,
> > >  };
> > >  
> > > -int i3c_bus_set_mode(struct i3c_bus *i3cbus, enum i3c_bus_mode mode)
> > > +int i3c_bus_set_mode(struct i3c_bus *i3cbus, enum i3c_bus_mode mode,
> > > +		     unsigned long i2c_scl_rate)  
> > 
> > 
> > Can we rename the last arg into max_i2c_scl_rate?  
> 
> The i2c_scl_rate is base on LVR[4] bit and the user can set a higher scl 
> rate, this is reason I didn't named it to max_i2c_scl_rate.
> But if you think that make more sense I'm ok with that.

In this context it does encode the maximum rate allowed by the spec
(based on LVR parsing), so max_i2c_rate sounds like a correct name to
me.

> 
> >   
> > >  {
> > >  	i3cbus->mode = mode;
> > >  
> > > -	if (!i3cbus->scl_rate.i3c)
> > > -		i3cbus->scl_rate.i3c = I3C_BUS_TYP_I3C_SCL_RATE;
> > > -
> > > -	if (!i3cbus->scl_rate.i2c) {
> > > -		if (i3cbus->mode == I3C_BUS_MODE_MIXED_SLOW)
> > > -			i3cbus->scl_rate.i2c = I3C_BUS_I2C_FM_SCL_RATE;
> > > -		else
> > > -			i3cbus->scl_rate.i2c = I3C_BUS_I2C_FM_PLUS_SCL_RATE;
> > > +	switch (i3cbus->mode) {
> > > +	case I3C_BUS_MODE_PURE:
> > > +		if (!i3cbus->scl_rate.i3c)
> > > +			i3cbus->scl_rate.i3c = I3C_BUS_TYP_I3C_SCL_RATE;
> > > +		break;
> > > +	case I3C_BUS_MODE_MIXED_FAST:
> > > +		if (!i3cbus->scl_rate.i3c)
> > > +			i3cbus->scl_rate.i3c = I3C_BUS_TYP_I3C_SCL_RATE;
> > > +		if (!i3cbus->scl_rate.i2c)
> > > +			i3cbus->scl_rate.i2c = i2c_scl_rate;
> > > +		break;
> > > +	case I3C_BUS_MODE_MIXED_SLOW:
> > > +		if (!i3cbus->scl_rate.i2c)
> > > +			i3cbus->scl_rate.i2c = i2c_scl_rate;
> > > +		i3cbus->scl_rate.i3c = i3cbus->scl_rate.i2c;  
> > 
> > Maybe we should do
> > 
> > 		if (!i3cbus->scl_rate.i3c ||
> > 		    i3cbus->scl_rate.i3c > i3cbus->scl_rate.i2c)
> > 			i3cbus->scl_rate.i3c = i3cbus->scl_rate.i2c;
> > 					   
> > Just in case the I3C rate forced by the user is lower than the max I2C
> > rate.  
> 
> That was something that I considered but TBH it isn't a real use case.

Add a WARN_ON() to at least catch such inconsistencies. And maybe we
should add a dev_warn() when the user-defined rates do not match
the mode/LVR constraints. It's easy to do a mistake when writing a dts.

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

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

[Vitor Soares]

Hi Boris,

From: Boris Brezillon <boris.brezillon@collabora.com>
Date: Tue, Apr 16, 2019 at 15:52:50

> On Tue, 16 Apr 2019 14:24:57 +0000
> Vitor Soares <vitor.soares@synopsys.com> wrote:
> 
> > Hi Boris,
> > 
> > From: Boris Brezillon <boris.brezillon@collabora.com>
> > Date: Tue, Apr 16, 2019 at 06:50:41
> > 
> > > On Mon, 15 Apr 2019 20:46:41 +0200
> > > Vitor Soares <vitor.soares@synopsys.com> wrote:
> > >   
> > > > Currently in case of mixed slow bus topologie and all i2c devices
> > > > support FM+ speed, the i3c subsystem limite the SCL to FM speed.  
> > >   
> > 
> > I will it replace with your message below.
> > 
> > > "
> > > Currently the I3C framework limits SCL frequency to FM speed when
> > > dealing with a mixed slow bus, even if all I2C devices are FM+ capable.
> > > "
> > >   
> > > > Also in case on mixed slow bus mode the max speed for both
> > > > i2c or i3c transfers is FM or FM+.  
> > > 
> > > Looks like you're basically repeating what you said above.  
> > 
> > What I meant was that I3C framework isn't limiting the I3C speed in case 
> > of mixed slow bus.
> 
> Oh, okay, then maybe something like
> 
> "
> The core was also not accounting for I3C speed limitations when
> operating in mixed slow mode and was erroneously using FM+ speed as the
> max I2C speed when operating in mixed fast mode.
> "

Sounds good to me. Thanks for the advise.

> 
> > 
> > >   
> > > > 
> > > > This patch fix the definition of i2c and i3c scl rate based on bus  
> > > 
> > > 	     ^fixes					      on the bus
> > >   
> > > > topologie and LVR[4] if no user input.  
> > > 
> > >   ^topology		^if the rate is not already specified by the user.
> > >   
> > > > In case of mixed slow mode the i3c scl rate is overridden.  
> > > 
> > > 							   ^ with the max
> > > I2C rate.
> > >   
> > > > 
> > > > Fixes: 3a379bbcea0a ("i3c: Add core I3C infrastructure")
> > > > Signed-off-by: Vitor Soares <vitor.soares@synopsys.com>
> > > > Cc: Boris Brezillon <bbrezillon@kernel.org>
> > > > Cc: <stable@vger.kernel.org>
> > > > Cc: <linux-kernel@vger.kernel.org>
> > > > ---
> > > >  drivers/i3c/master.c | 39 +++++++++++++++++++++++++--------------
> > > >  1 file changed, 25 insertions(+), 14 deletions(-)
> > > > 
> > > > diff --git a/drivers/i3c/master.c b/drivers/i3c/master.c
> > > > index 909c2ad..1c4a86a 100644
> > > > --- a/drivers/i3c/master.c
> > > > +++ b/drivers/i3c/master.c
> > > > @@ -564,20 +564,30 @@ static const struct device_type i3c_masterdev_type = {
> > > >  	.groups	= i3c_masterdev_groups,
> > > >  };
> > > >  
> > > > -int i3c_bus_set_mode(struct i3c_bus *i3cbus, enum i3c_bus_mode mode)
> > > > +int i3c_bus_set_mode(struct i3c_bus *i3cbus, enum i3c_bus_mode mode,
> > > > +		     unsigned long i2c_scl_rate)  
> > > 
> > > 
> > > Can we rename the last arg into max_i2c_scl_rate?  
> > 
> > The i2c_scl_rate is base on LVR[4] bit and the user can set a higher scl 
> > rate, this is reason I didn't named it to max_i2c_scl_rate.
> > But if you think that make more sense I'm ok with that.
> 
> In this context it does encode the maximum rate allowed by the spec
> (based on LVR parsing), so max_i2c_rate sounds like a correct name to
> me.
> 
> > 
> > >   
> > > >  {
> > > >  	i3cbus->mode = mode;
> > > >  
> > > > -	if (!i3cbus->scl_rate.i3c)
> > > > -		i3cbus->scl_rate.i3c = I3C_BUS_TYP_I3C_SCL_RATE;
> > > > -
> > > > -	if (!i3cbus->scl_rate.i2c) {
> > > > -		if (i3cbus->mode == I3C_BUS_MODE_MIXED_SLOW)
> > > > -			i3cbus->scl_rate.i2c = I3C_BUS_I2C_FM_SCL_RATE;
> > > > -		else
> > > > -			i3cbus->scl_rate.i2c = I3C_BUS_I2C_FM_PLUS_SCL_RATE;
> > > > +	switch (i3cbus->mode) {
> > > > +	case I3C_BUS_MODE_PURE:
> > > > +		if (!i3cbus->scl_rate.i3c)
> > > > +			i3cbus->scl_rate.i3c = I3C_BUS_TYP_I3C_SCL_RATE;
> > > > +		break;
> > > > +	case I3C_BUS_MODE_MIXED_FAST:
> > > > +		if (!i3cbus->scl_rate.i3c)
> > > > +			i3cbus->scl_rate.i3c = I3C_BUS_TYP_I3C_SCL_RATE;
> > > > +		if (!i3cbus->scl_rate.i2c)
> > > > +			i3cbus->scl_rate.i2c = i2c_scl_rate;
> > > > +		break;
> > > > +	case I3C_BUS_MODE_MIXED_SLOW:
> > > > +		if (!i3cbus->scl_rate.i2c)
> > > > +			i3cbus->scl_rate.i2c = i2c_scl_rate;
> > > > +		i3cbus->scl_rate.i3c = i3cbus->scl_rate.i2c;  
> > > 
> > > Maybe we should do
> > > 
> > > 		if (!i3cbus->scl_rate.i3c ||
> > > 		    i3cbus->scl_rate.i3c > i3cbus->scl_rate.i2c)
> > > 			i3cbus->scl_rate.i3c = i3cbus->scl_rate.i2c;
> > > 					   
> > > Just in case the I3C rate forced by the user is lower than the max I2C
> > > rate.  
> > 
> > That was something that I considered but TBH it isn't a real use case.
> 
> Add a WARN_ON() to at least catch such inconsistencies. And maybe we
> should add a dev_warn() when the user-defined rates do not match
> the mode/LVR constraints. It's easy to do a mistake when writing a dts.

I think the WARN_ON() is too evasive on the screen and won't provide the 
information we want.
The dev_warn() should work perfectly here.

		if (i3cbus->scl_rate.i3c < i3cbus->scl_rate.i2c)
			dev_warn(&i3cbus->cur_master->dev->dev,
				     "%s: i3c-scl-hz lower then i2c-scl-hz\n",  __func__);
		if (i3cbus->scl_rate.i2c != I3C_BUS_I2C_FM_SCL_RATE ||
		    i3cbus->scl_rate.i2c != I3C_BUS_I2C_FM_PLUS_SCL_RATE)
			dev_warn(&i3cbus->cur_master->dev->dev,
				     "%s: i2c-scl-hz not defined according MIPI I3C spec\n", 
				     __func__);

Maybe it make more sense to do this check on of_populate_i3c_bus(), what 
do you think?





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

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

[Boris Brezillon]

On Mon, 22 Apr 2019 15:54:33 +0000
Vitor Soares <Vitor.Soares@synopsys.com> wrote:


> > > >     
> > > > >  {
> > > > >  	i3cbus->mode = mode;
> > > > >  
> > > > > -	if (!i3cbus->scl_rate.i3c)
> > > > > -		i3cbus->scl_rate.i3c = I3C_BUS_TYP_I3C_SCL_RATE;
> > > > > -
> > > > > -	if (!i3cbus->scl_rate.i2c) {
> > > > > -		if (i3cbus->mode == I3C_BUS_MODE_MIXED_SLOW)
> > > > > -			i3cbus->scl_rate.i2c = I3C_BUS_I2C_FM_SCL_RATE;
> > > > > -		else
> > > > > -			i3cbus->scl_rate.i2c = I3C_BUS_I2C_FM_PLUS_SCL_RATE;
> > > > > +	switch (i3cbus->mode) {
> > > > > +	case I3C_BUS_MODE_PURE:
> > > > > +		if (!i3cbus->scl_rate.i3c)
> > > > > +			i3cbus->scl_rate.i3c = I3C_BUS_TYP_I3C_SCL_RATE;
> > > > > +		break;
> > > > > +	case I3C_BUS_MODE_MIXED_FAST:
> > > > > +		if (!i3cbus->scl_rate.i3c)
> > > > > +			i3cbus->scl_rate.i3c = I3C_BUS_TYP_I3C_SCL_RATE;
> > > > > +		if (!i3cbus->scl_rate.i2c)
> > > > > +			i3cbus->scl_rate.i2c = i2c_scl_rate;
> > > > > +		break;
> > > > > +	case I3C_BUS_MODE_MIXED_SLOW:
> > > > > +		if (!i3cbus->scl_rate.i2c)
> > > > > +			i3cbus->scl_rate.i2c = i2c_scl_rate;
> > > > > +		i3cbus->scl_rate.i3c = i3cbus->scl_rate.i2c;    
> > > > 
> > > > Maybe we should do
> > > > 
> > > > 		if (!i3cbus->scl_rate.i3c ||
> > > > 		    i3cbus->scl_rate.i3c > i3cbus->scl_rate.i2c)
> > > > 			i3cbus->scl_rate.i3c = i3cbus->scl_rate.i2c;
> > > > 					   
> > > > Just in case the I3C rate forced by the user is lower than the max I2C
> > > > rate.    
> > > 
> > > That was something that I considered but TBH it isn't a real use case.  
> > 
> > Add a WARN_ON() to at least catch such inconsistencies. And maybe we
> > should add a dev_warn() when the user-defined rates do not match
> > the mode/LVR constraints. It's easy to do a mistake when writing a dts.  
> 
> I think the WARN_ON() is too evasive on the screen and won't provide the 
> information we want.
> The dev_warn() should work perfectly here.
> 
> 		if (i3cbus->scl_rate.i3c < i3cbus->scl_rate.i2c)
> 			dev_warn(&i3cbus->cur_master->dev->dev,
> 				     "%s: i3c-scl-hz lower then i2c-scl-hz\n",  __func__);

Using dev_warn() sounds good, though I don't think you need the
__func__ here. Also, please print the i2c/i3c rates in the message, and
align the second line on the open parens.

> 		if (i3cbus->scl_rate.i2c != I3C_BUS_I2C_FM_SCL_RATE ||
> 		    i3cbus->scl_rate.i2c != I3C_BUS_I2C_FM_PLUS_SCL_RATE)
> 			dev_warn(&i3cbus->cur_master->dev->dev,
> 				     "%s: i2c-scl-hz not defined according MIPI I3C spec\n", 
> 				     __func__);

Is that really a problem? Having an i2c rate that is less than FM speed
sounds like a valid case to me.

> 
> Maybe it make more sense to do this check on of_populate_i3c_bus(), what 
> do you think?
> 

No, we really want to have this check here, because we might support
other HW description formats at some point (board-files, ACPI, ...).

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

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

[Vitor Soares]

From: Boris Brezillon <boris.brezillon@collabora.com>
Date: Mon, Apr 22, 2019 at 17:07:15

> On Mon, 22 Apr 2019 15:54:33 +0000
> Vitor Soares <Vitor.Soares@synopsys.com> wrote:
> 
> 
> > > > >     
> > > > > >  {
> > > > > >  	i3cbus->mode = mode;
> > > > > >  
> > > > > > -	if (!i3cbus->scl_rate.i3c)
> > > > > > -		i3cbus->scl_rate.i3c = I3C_BUS_TYP_I3C_SCL_RATE;
> > > > > > -
> > > > > > -	if (!i3cbus->scl_rate.i2c) {
> > > > > > -		if (i3cbus->mode == I3C_BUS_MODE_MIXED_SLOW)
> > > > > > -			i3cbus->scl_rate.i2c = I3C_BUS_I2C_FM_SCL_RATE;
> > > > > > -		else
> > > > > > -			i3cbus->scl_rate.i2c = I3C_BUS_I2C_FM_PLUS_SCL_RATE;
> > > > > > +	switch (i3cbus->mode) {
> > > > > > +	case I3C_BUS_MODE_PURE:
> > > > > > +		if (!i3cbus->scl_rate.i3c)
> > > > > > +			i3cbus->scl_rate.i3c = I3C_BUS_TYP_I3C_SCL_RATE;
> > > > > > +		break;
> > > > > > +	case I3C_BUS_MODE_MIXED_FAST:
> > > > > > +		if (!i3cbus->scl_rate.i3c)
> > > > > > +			i3cbus->scl_rate.i3c = I3C_BUS_TYP_I3C_SCL_RATE;
> > > > > > +		if (!i3cbus->scl_rate.i2c)
> > > > > > +			i3cbus->scl_rate.i2c = i2c_scl_rate;
> > > > > > +		break;
> > > > > > +	case I3C_BUS_MODE_MIXED_SLOW:
> > > > > > +		if (!i3cbus->scl_rate.i2c)
> > > > > > +			i3cbus->scl_rate.i2c = i2c_scl_rate;
> > > > > > +		i3cbus->scl_rate.i3c = i3cbus->scl_rate.i2c;    
> > > > > 
> > > > > Maybe we should do
> > > > > 
> > > > > 		if (!i3cbus->scl_rate.i3c ||
> > > > > 		    i3cbus->scl_rate.i3c > i3cbus->scl_rate.i2c)
> > > > > 			i3cbus->scl_rate.i3c = i3cbus->scl_rate.i2c;
> > > > > 					   
> > > > > Just in case the I3C rate forced by the user is lower than the max I2C
> > > > > rate.    
> > > > 
> > > > That was something that I considered but TBH it isn't a real use case.  
> > > 
> > > Add a WARN_ON() to at least catch such inconsistencies. And maybe we
> > > should add a dev_warn() when the user-defined rates do not match
> > > the mode/LVR constraints. It's easy to do a mistake when writing a dts.  
> > 
> > I think the WARN_ON() is too evasive on the screen and won't provide the 
> > information we want.
> > The dev_warn() should work perfectly here.
> > 
> > 		if (i3cbus->scl_rate.i3c < i3cbus->scl_rate.i2c)
> > 			dev_warn(&i3cbus->cur_master->dev->dev,
> > 				     "%s: i3c-scl-hz lower then i2c-scl-hz\n",  __func__);
> 
> Using dev_warn() sounds good, though I don't think you need the
> __func__ here. Also, please print the i2c/i3c rates in the message, and
> align the second line on the open parens.
> 
> > 		if (i3cbus->scl_rate.i2c != I3C_BUS_I2C_FM_SCL_RATE ||
> > 		    i3cbus->scl_rate.i2c != I3C_BUS_I2C_FM_PLUS_SCL_RATE)
> > 			dev_warn(&i3cbus->cur_master->dev->dev,
> > 				     "%s: i2c-scl-hz not defined according MIPI I3C spec\n", 
> > 				     __func__);
> 
> Is that really a problem? Having an i2c rate that is less than FM speed
> sounds like a valid case to me.

I'm addressing the spec constrains.

In the practice it can be SM or even HS, it depends on the interface.

> 
> > 
> > Maybe it make more sense to do this check on of_populate_i3c_bus(), what 
> > do you think?
> > 
> 
> No, we really want to have this check here, because we might support
> other HW description formats at some point (board-files, ACPI, ...).

Yes, you are right. I forgot that point. 


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

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

[Boris Brezillon]

On Mon, 22 Apr 2019 17:54:29 +0000
Vitor Soares <Vitor.Soares@synopsys.com> wrote:

> > > > > >       
> > > > > > >  {
> > > > > > >  	i3cbus->mode = mode;
> > > > > > >  
> > > > > > > -	if (!i3cbus->scl_rate.i3c)
> > > > > > > -		i3cbus->scl_rate.i3c = I3C_BUS_TYP_I3C_SCL_RATE;
> > > > > > > -
> > > > > > > -	if (!i3cbus->scl_rate.i2c) {
> > > > > > > -		if (i3cbus->mode == I3C_BUS_MODE_MIXED_SLOW)
> > > > > > > -			i3cbus->scl_rate.i2c = I3C_BUS_I2C_FM_SCL_RATE;
> > > > > > > -		else
> > > > > > > -			i3cbus->scl_rate.i2c = I3C_BUS_I2C_FM_PLUS_SCL_RATE;
> > > > > > > +	switch (i3cbus->mode) {
> > > > > > > +	case I3C_BUS_MODE_PURE:
> > > > > > > +		if (!i3cbus->scl_rate.i3c)
> > > > > > > +			i3cbus->scl_rate.i3c = I3C_BUS_TYP_I3C_SCL_RATE;
> > > > > > > +		break;
> > > > > > > +	case I3C_BUS_MODE_MIXED_FAST:
> > > > > > > +		if (!i3cbus->scl_rate.i3c)
> > > > > > > +			i3cbus->scl_rate.i3c = I3C_BUS_TYP_I3C_SCL_RATE;
> > > > > > > +		if (!i3cbus->scl_rate.i2c)
> > > > > > > +			i3cbus->scl_rate.i2c = i2c_scl_rate;
> > > > > > > +		break;
> > > > > > > +	case I3C_BUS_MODE_MIXED_SLOW:
> > > > > > > +		if (!i3cbus->scl_rate.i2c)
> > > > > > > +			i3cbus->scl_rate.i2c = i2c_scl_rate;
> > > > > > > +		i3cbus->scl_rate.i3c = i3cbus->scl_rate.i2c;      
> > > > > > 
> > > > > > Maybe we should do
> > > > > > 
> > > > > > 		if (!i3cbus->scl_rate.i3c ||
> > > > > > 		    i3cbus->scl_rate.i3c > i3cbus->scl_rate.i2c)
> > > > > > 			i3cbus->scl_rate.i3c = i3cbus->scl_rate.i2c;
> > > > > > 					   
> > > > > > Just in case the I3C rate forced by the user is lower than the max I2C
> > > > > > rate.      
> > > > > 
> > > > > That was something that I considered but TBH it isn't a real use case.    
> > > > 
> > > > Add a WARN_ON() to at least catch such inconsistencies. And maybe we
> > > > should add a dev_warn() when the user-defined rates do not match
> > > > the mode/LVR constraints. It's easy to do a mistake when writing a dts.    
> > > 
> > > I think the WARN_ON() is too evasive on the screen and won't provide the 
> > > information we want.
> > > The dev_warn() should work perfectly here.
> > > 
> > > 		if (i3cbus->scl_rate.i3c < i3cbus->scl_rate.i2c)
> > > 			dev_warn(&i3cbus->cur_master->dev->dev,
> > > 				     "%s: i3c-scl-hz lower then i2c-scl-hz\n",  __func__);  
> > 
> > Using dev_warn() sounds good, though I don't think you need the
> > __func__ here. Also, please print the i2c/i3c rates in the message, and
> > align the second line on the open parens.
> >   
> > > 		if (i3cbus->scl_rate.i2c != I3C_BUS_I2C_FM_SCL_RATE ||
> > > 		    i3cbus->scl_rate.i2c != I3C_BUS_I2C_FM_PLUS_SCL_RATE)
> > > 			dev_warn(&i3cbus->cur_master->dev->dev,
> > > 				     "%s: i2c-scl-hz not defined according MIPI I3C spec\n", 
> > > 				     __func__);  
> > 
> > Is that really a problem? Having an i2c rate that is less than FM speed
> > sounds like a valid case to me.  
> 
> I'm addressing the spec constrains.

"Table 57 I3C Timing Requirements When Communicating With I2C Legacy
Devices" says that freq can be between 0 and 400KHz when operating in
slow(FM) mode. Yes, maximum rate when not specified otherwise is
400Khz, but the point of overloading the max I2C/I3C spec is to allow
custom rates when the default/spec ones are not achievable, so I'm not
sure complaining in that case is legitimate.

We should definitely complain when one tries to set a maximum rate that
is higher than what devices can do (i3cbus->scl_rate.i2c >
max_i2c_scl_rate).

Same goes for I3C communications, we shouldn't care when the forced rate
is lower than what the bus is capable of, what's important is to
complain when it's higher than what's supported.

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

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

[Vitor Soares]

From: Boris Brezillon <boris.brezillon@collabora.com>
Date: Mon, Apr 22, 2019 at 19:27:32

> On Mon, 22 Apr 2019 17:54:29 +0000
> Vitor Soares <Vitor.Soares@synopsys.com> wrote:
> 
> > > > > > >       
> > > > > > > >  {
> > > > > > > >  	i3cbus->mode = mode;
> > > > > > > >  
> > > > > > > > -	if (!i3cbus->scl_rate.i3c)
> > > > > > > > -		i3cbus->scl_rate.i3c = I3C_BUS_TYP_I3C_SCL_RATE;
> > > > > > > > -
> > > > > > > > -	if (!i3cbus->scl_rate.i2c) {
> > > > > > > > -		if (i3cbus->mode == I3C_BUS_MODE_MIXED_SLOW)
> > > > > > > > -			i3cbus->scl_rate.i2c = I3C_BUS_I2C_FM_SCL_RATE;
> > > > > > > > -		else
> > > > > > > > -			i3cbus->scl_rate.i2c = I3C_BUS_I2C_FM_PLUS_SCL_RATE;
> > > > > > > > +	switch (i3cbus->mode) {
> > > > > > > > +	case I3C_BUS_MODE_PURE:
> > > > > > > > +		if (!i3cbus->scl_rate.i3c)
> > > > > > > > +			i3cbus->scl_rate.i3c = I3C_BUS_TYP_I3C_SCL_RATE;
> > > > > > > > +		break;
> > > > > > > > +	case I3C_BUS_MODE_MIXED_FAST:
> > > > > > > > +		if (!i3cbus->scl_rate.i3c)
> > > > > > > > +			i3cbus->scl_rate.i3c = I3C_BUS_TYP_I3C_SCL_RATE;
> > > > > > > > +		if (!i3cbus->scl_rate.i2c)
> > > > > > > > +			i3cbus->scl_rate.i2c = i2c_scl_rate;
> > > > > > > > +		break;
> > > > > > > > +	case I3C_BUS_MODE_MIXED_SLOW:
> > > > > > > > +		if (!i3cbus->scl_rate.i2c)
> > > > > > > > +			i3cbus->scl_rate.i2c = i2c_scl_rate;
> > > > > > > > +		i3cbus->scl_rate.i3c = i3cbus->scl_rate.i2c;      
> > > > > > > 
> > > > > > > Maybe we should do
> > > > > > > 
> > > > > > > 		if (!i3cbus->scl_rate.i3c ||
> > > > > > > 		    i3cbus->scl_rate.i3c > i3cbus->scl_rate.i2c)
> > > > > > > 			i3cbus->scl_rate.i3c = i3cbus->scl_rate.i2c;
> > > > > > > 					   
> > > > > > > Just in case the I3C rate forced by the user is lower than the max I2C
> > > > > > > rate.      
> > > > > > 
> > > > > > That was something that I considered but TBH it isn't a real use case.    
> > > > > 
> > > > > Add a WARN_ON() to at least catch such inconsistencies. And maybe we
> > > > > should add a dev_warn() when the user-defined rates do not match
> > > > > the mode/LVR constraints. It's easy to do a mistake when writing a dts.    
> > > > 
> > > > I think the WARN_ON() is too evasive on the screen and won't provide the 
> > > > information we want.
> > > > The dev_warn() should work perfectly here.
> > > > 
> > > > 		if (i3cbus->scl_rate.i3c < i3cbus->scl_rate.i2c)
> > > > 			dev_warn(&i3cbus->cur_master->dev->dev,
> > > > 				     "%s: i3c-scl-hz lower then i2c-scl-hz\n",  __func__);  
> > > 
> > > Using dev_warn() sounds good, though I don't think you need the
> > > __func__ here. Also, please print the i2c/i3c rates in the message, and
> > > align the second line on the open parens.
> > >   
> > > > 		if (i3cbus->scl_rate.i2c != I3C_BUS_I2C_FM_SCL_RATE ||
> > > > 		    i3cbus->scl_rate.i2c != I3C_BUS_I2C_FM_PLUS_SCL_RATE)
> > > > 			dev_warn(&i3cbus->cur_master->dev->dev,
> > > > 				     "%s: i2c-scl-hz not defined according MIPI I3C spec\n", 
> > > > 				     __func__);  
> > > 
> > > Is that really a problem? Having an i2c rate that is less than FM speed
> > > sounds like a valid case to me.  
> > 
> > I'm addressing the spec constrains.
> 
> "Table 57 I3C Timing Requirements When Communicating With I2C Legacy
> Devices" says that freq can be between 0 and 400KHz when operating in
> slow(FM) mode.

That is the definition of Fast-mode:
"Fast-mode devices can receive and transmit at up to 400kbit/s" as per 
i2c spec.
So this way the slaves can be connected to Standard Mode bus... you 
already know that.

For the FM requirements in I3C bus, please refer to I3C FAQ Q2.2 and 
table 56 of I3C v1.0 basic spec.

> Yes, maximum rate when not specified otherwise is
> 400Khz, 

By default you will have always FM or FM+ due the LVR register be 
mandatory.

> but the point of overloading the max I2C/I3C spec is to allow
> custom rates when the default/spec ones are not achievable, so I'm not
> sure complaining in that case is legitimate.
> 

Well, if the users aren't able to achieve at least FM is because 
something is not correct.

> We should definitely complain when one tries to set a maximum rate that
> is higher than what devices can do (i3cbus->scl_rate.i2c >
> max_i2c_scl_rate).
> 

I can put another if() for such cases.

> Same goes for I3C communications, we shouldn't care when the forced rate
> is lower than what the bus is capable of, what's important is to
> complain when it's higher than what's supported.

The point is complain when scl_rate.i3c < scl_rate.i2c because it can be 
error from user.

Best regards,
Vitor Soares

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