Linux-i3c Archive on lore.kernel.org
 help / color / Atom feed
From: Boris Brezillon <boris.brezillon@collabora.com>
To: Vitor Soares <vitor.soares@synopsys.com>
Cc: "linux-i3c@lists.infradead.org" <linux-i3c@lists.infradead.org>,
	"joao.pinto@synopsys.com" <joao.pinto@synopsys.com>,
	"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
	"stable@vger.kernel.org" <stable@vger.kernel.org>,
	Boris Brezillon <bbrezillon@kernel.org>
Subject: Re: [PATCH 1/3] i3c: fix i2c and i3c scl rate by bus mode
Date: Tue, 16 Apr 2019 16:52:50 +0200
Message-ID: <20190416165250.0606e5a2@collabora.com> (raw)
In-Reply-To: <13D59CF9CEBAF94592A12E8AE55501350A61596D@DE02WEMBXB.internal.synopsys.com>

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

  reply index

Thread overview: 15+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-04-15 18:46 [PATCH 0/3] Fix i2c and i3c scl rate according " Vitor Soares
2019-04-15 18:46 ` [PATCH 1/3] i3c: fix i2c and i3c scl rate by " Vitor Soares
2019-04-16  5:50   ` Boris Brezillon
2019-04-16 14:24     ` Vitor Soares
2019-04-16 14:52       ` Boris Brezillon [this message]
2019-04-22 15:54         ` Vitor Soares
2019-04-22 16:07           ` Boris Brezillon
2019-04-22 17:54             ` Vitor Soares
2019-04-22 18:27               ` Boris Brezillon
2019-04-22 19:59                 ` Vitor Soares
2019-04-15 18:46 ` [PATCH 2/3] i3c: add mixed limited " Vitor Soares
2019-04-16  6:00   ` Boris Brezillon
2019-04-16 14:27     ` Vitor Soares
2019-04-15 18:46 ` [PATCH 3/3] i3c: dw: Add limited bus mode support Vitor Soares
2019-04-16  6:09   ` Boris Brezillon

Reply instructions:

You may reply publically to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20190416165250.0606e5a2@collabora.com \
    --to=boris.brezillon@collabora.com \
    --cc=bbrezillon@kernel.org \
    --cc=joao.pinto@synopsys.com \
    --cc=linux-i3c@lists.infradead.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=stable@vger.kernel.org \
    --cc=vitor.soares@synopsys.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

Linux-i3c Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-i3c/0 linux-i3c/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-i3c linux-i3c/ https://lore.kernel.org/linux-i3c \
		linux-i3c@lists.infradead.org linux-i3c@archiver.kernel.org
	public-inbox-index linux-i3c


Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.infradead.lists.linux-i3c


AGPL code for this site: git clone https://public-inbox.org/ public-inbox