Linux-i3c Archive on lore.kernel.org
 help / color / Atom feed
From: Vitor Soares <Vitor.Soares@synopsys.com>
To: Boris Brezillon <boris.brezillon@collabora.com>,
	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: Mon, 22 Apr 2019 19:59:46 +0000
Message-ID: <13D59CF9CEBAF94592A12E8AE55501350A61C658@DE02WEMBXB.internal.synopsys.com> (raw)
In-Reply-To: <20190422202732.0d539af3@collabora.com>

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

  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
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 [this message]
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=13D59CF9CEBAF94592A12E8AE55501350A61C658@DE02WEMBXB.internal.synopsys.com \
    --to=vitor.soares@synopsys.com \
    --cc=Joao.Pinto@synopsys.com \
    --cc=bbrezillon@kernel.org \
    --cc=boris.brezillon@collabora.com \
    --cc=linux-i3c@lists.infradead.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=stable@vger.kernel.org \
    /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