diff mbox series

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

Message ID 05fdeea79db83970e9ecb0d7045b4dd98f206f06.1555350118.git.vitor.soares@synopsys.com (mailing list archive)
State Superseded
Headers show
Series Fix i2c and i3c scl rate according bus mode | expand

Commit Message

Vitor Soares April 15, 2019, 6:46 p.m. UTC
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(-)

Comments

Boris Brezillon April 16, 2019, 5:50 a.m. UTC | #1
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;
>
Vitor Soares April 16, 2019, 2:24 p.m. UTC | #2
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
Boris Brezillon April 16, 2019, 2:52 p.m. UTC | #3
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.
Vitor Soares April 22, 2019, 3:54 p.m. UTC | #4
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?
Boris Brezillon April 22, 2019, 4:07 p.m. UTC | #5
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, ...).
Vitor Soares April 22, 2019, 5:54 p.m. UTC | #6
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.
Boris Brezillon April 22, 2019, 6:27 p.m. UTC | #7
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.
Vitor Soares April 22, 2019, 7:59 p.m. UTC | #8
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
diff mbox series

Patch

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;