Message-ID:
 <VI1PR04MB500569EA6CB30F6C6807FD57E8AA2@VI1PR04MB5005.eurprd04.prod.outlook.com>
Date: Wed, 24 Jul 2024 02:28:24 +0000
From: Carlos Song <carlos.song@....com>
To: Frank Li <frank.li@....com>, "alexandre.belloni@...tlin.com"
	<alexandre.belloni@...tlin.com>
CC: "miquel.raynal@...tlin.com" <miquel.raynal@...tlin.com>,
	"conor.culhane@...vaco.com" <conor.culhane@...vaco.com>,
	"linux-i3c@...ts.infradead.org" <linux-i3c@...ts.infradead.org>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
	"imx@...ts.linux.dev" <imx@...ts.linux.dev>, dl-linux-imx <linux-imx@....com>
Subject: RE: [PATCH v2] i3c: master: svc: adjust the first broadcast speed



> -----Original Message-----
> From: Frank Li <frank.li@....com>
> Sent: Monday, July 22, 2024 11:02 PM
> To: Carlos Song <carlos.song@....com>
> Cc: miquel.raynal@...tlin.com; conor.culhane@...vaco.com;
> alexandre.belloni@...tlin.com; linux-i3c@...ts.infradead.org;
> linux-kernel@...r.kernel.org; imx@...ts.linux.dev; dl-linux-imx
> <linux-imx@....com>
> Subject: Re: [PATCH v2] i3c: master: svc: adjust the first broadcast speed
> 
> On Sat, Jul 20, 2024 at 04:42:18AM +0000, Carlos Song wrote:
> >
> >
> > > -----Original Message-----
> > > From: Frank Li <frank.li@....com>
> > > Sent: Saturday, July 20, 2024 2:26 AM
> > > To: Carlos Song <carlos.song@....com>
> > > Cc: miquel.raynal@...tlin.com; conor.culhane@...vaco.com;
> > > alexandre.belloni@...tlin.com; linux-i3c@...ts.infradead.org;
> > > linux-kernel@...r.kernel.org; imx@...ts.linux.dev; dl-linux-imx
> > > <linux-imx@....com>
> > > Subject: Re: [PATCH v2] i3c: master: svc: adjust the first broadcast
> > > speed
> > >
> > > On Fri, Jul 19, 2024 at 04:37:01PM +0000, Carlos Song wrote:
> > > >
> > > >
> > > > > -----Original Message-----
> > > > > From: Frank Li <frank.li@....com>
> > > > > Sent: Friday, July 19, 2024 11:01 PM
> > > > > To: Carlos Song <carlos.song@....com>
> > > > > Cc: miquel.raynal@...tlin.com; conor.culhane@...vaco.com;
> > > > > alexandre.belloni@...tlin.com; linux-i3c@...ts.infradead.org;
> > > > > linux-kernel@...r.kernel.org; imx@...ts.linux.dev; dl-linux-imx
> > > > > <linux-imx@....com>
> > > > > Subject: Re: [PATCH v2] i3c: master: svc: adjust the first
> > > > > broadcast speed
> > > > >
> > > > > On Fri, Jul 19, 2024 at 04:03:51PM +0800, carlos.song@....com wrote:
> > > > > > From: Carlos Song <carlos.song@....com>
> > > > > >
> > > > > > According to the i3c spec 6.2 Timing Specification, the first
> > > > > > broadcast open drain timing should be adjust to High Period of
> > > > > > SCL Clock is 200ns at least. I3C device working as a i2c
> > > > > > device will see the broadcast to close its Spike Filter to change to i3c mode.
> > > > > > After that I3C open drain SCL high level should be adjust to 32ns~45ns.
> > > > > >
> > > > > > Signed-off-by: Carlos Song <carlos.song@....com>
> > > > > > ---
> > > > > > Change for V2:
> > > > > > - use slow_speed instead of first_broadcast
> > > > > > - add default_speed variable in svc_i3c_xfer to avoid set default
> > > > > >   speed every time
> > > > > > - change start_xfer if else for easy understand
> > > > > > ---
> > > > > >  drivers/i3c/master/svc-i3c-master.c | 55
> > > > > > +++++++++++++++++++++++++++++
> > > > > >  1 file changed, 55 insertions(+)
> > > > > >
> > > > > > diff --git a/drivers/i3c/master/svc-i3c-master.c
> > > > > > b/drivers/i3c/master/svc-i3c-master.c
> > > > > > index 78116530f431..7cd3a9a4d7dd 100644
> > > > > > --- a/drivers/i3c/master/svc-i3c-master.c
> > > > > > +++ b/drivers/i3c/master/svc-i3c-master.c
> > > > > > @@ -142,6 +142,7 @@ struct svc_i3c_cmd {
> > > > > >  	unsigned int actual_len;
> > > > > >  	struct i3c_priv_xfer *xfer;
> > > > > >  	bool continued;
> > > > > > +	bool slow_address;
> > > > > >  };
> > > > > >
> > > > > >  struct svc_i3c_xfer {
> > > > > > @@ -214,6 +215,11 @@ struct svc_i3c_master {
> > > > > >  	} ibi;
> > > > > >  	struct mutex lock;
> > > > > >  	int enabled_events;
> > > > > > +
> > > > > > +	unsigned long fclk_rate;
> > > > > > +	u32 mctrl_config;
> > > > > > +	bool slow_speed;
> > > > > > +	bool default_speed;
> > > > >
> > > > > I think you needn't two varible 'slow_speed' and 'default_speed'.
> > > > > 	default_speed should always !slow_speed
> > > > >
> > > > > Frank
> > > > >
> > > >
> > > > Hi, Frank
> > > >
> > > > In fact, I am struggling for using just one variable: slow speed.
> > > > Adding a
> > > variable "default_speed "was also a have-to move.
> > > >
> > > > If I use "if else" in xfer for easy understand, it means I only
> > > > can change the
> > > MCTRL register value one time in every xfer. So in the first xfer, I
> > > must change slow_speed to false, then next xfer cmd->slow_address
> > > will be false. So in next xfer, I can set initial configuration back to the controller.
> > > > But the question is I have to set it every time, so I add the
> > > > extra variable to
> > > avoid writel in every xfer.
> > > > It looks bad... Sorry for my poor coding.
> > > >
> > > > If only one variable " slow_speed " is used , I think "if else"
> > > > and "writel one
> > > time", only one can be used. Maybe there is a better code method but
> > > I don't get it?
> > >
> > > If I understand correct.
> > >
> > > svc_i3c_master_set_slow_address_speed()
> > > {
> > > 	...
> > > 	master->slow_speed = true;
> > > }
> > >
> > > svc_i3c_master_set_default_speed()
> > > {
> > > 	if (master->slow_speed) {
> > > 		writel();
> > > 		master->slow_speed false;
> > > 	}
> > > }
> > >
> > >   if (cmd->slow_address)
> > > 	svc_i3c_master_set_slow_address_speed(master);
> > >   else
> > > 	svc_i3c_master_set_default_speed(master);
> > >
> >
> > Above logic will never enter the
> > svc_i3c_master_set_default_speed(master);
> > Because this :
> > svc_i3c_master_send_bdcast_ccc_cmd(){
> >    if (master->slow_speed)
> 
> I think, it should be master->first_cmd, which reflect the real means.
> Then it means use two variables.
> 
Yes, it is. 
> If it is i3c standard, I prefer it should do by framework to avoid every driver add
> similar logic.
> 
> May Alexandre Belloni can provide comemnts about this.
> 
> static int i3c_master_bus_init(struct i3c_master_controller *master) {
> 	...
> 	master->ops->set_speed(low);
> 
> 	ret = i3c_master_rstdaa_locked(master, I3C_BROADCAST_ADDR);
> 
> 	master->ops->set_speed(normal);
> 	...
> }
> 
I agree that. If this can be handled in common code, which can indeed simplify the i3c controller driver.

Hi, Alexandre Belloni,

Now I am supporting P3T1085 sensor, I meet the issue, I ask help to the sensor engineer, he told me that:
When P3T1085 received I3C 7E header, then it will disable I2C 50 ns filter, and then I3C mode speed is supported.
It's defined in the I3C specification for all I3C target devices. It needs at least Thigh > 200 nS to get I3C header 0x7E to work.
The spec requirement in Specification for I3C(11-Jun-2021 Version 1.1.1) in page 495 Table 122 I3C Open Drain Timing Parameter.

As I understand it(if I am not wrong), all I3C target devices wit 50ns filter should be designed to keep the MIPI I3C spec. Slow speed 7e
permits any I3C Target with Spike Filter to detect that it is on an I3C Bus, and as a result disable the Spike Filter, then work at I3C timing.

Is there a mistake in my understanding? Can you help evaluate whether slow first broadcast should be supported in common code?

Carlos

> > 		cmd->slow_address = true;
> >    else
> > 		cmd->slow_address = false;
> > }
> >
> > Then svc_i3c_master_start_xfer_locked(){
> > 	if (cmd->slow_address)
> >  		svc_i3c_master_set_slow_address_speed(master);
> > 	else
> >  		svc_i3c_master_set_default_speed(master);
> > }
> >
> > In svc_i3c_master_send_bdcast_ccc_cmd, we always need slow_speed to
> > dicide cmd->slow_address is true or false. Then in
> svc_i3c_master_start_xfer_locked, We use cmd->slow_address to chose set
> slow_speed or default speed.
> > when use "if else", it means we put slow_speed true->false into next xfer.
> >
> > When first xfer finish, slow_speed is still true. So in next
> svc_i3c_master_send_bdcast_ccc_cmd:
> > then cmd->slow_address=ture, so in next xfer it will continue enter the "if"
> branch not the "else" branch:
> >
> > if (cmd->slow_address)
> >  	svc_i3c_master_set_slow_address_speed(master);
> > else
> >  	svc_i3c_master_set_default_speed(master);
> >
> > so whatever we need change master->slow_speed = false in the first xfer, don't
> put it into next xfer.
> > This is the reason I always do taht in my V1 V2 patch.
> >
> > Carlos
> >
> > > When slow_address is ture, always set slow_speed = ture when
> > > slow_address is false, call to set_sefault_speed, if previous
> > > 	slow_speed is true, then change to default speed, slow_speed will be false.
> > > 	when next time call to set_default_speed() do nothing.
> > >
> > > Frank
> > > >
> > > > Carlos
> > > > > >  };
> > > > > >
> > > > > >  /**
> > > > > > @@ -531,6 +537,43 @@ static irqreturn_t
> > > > > > svc_i3c_master_irq_handler(int
> > > > > irq, void *dev_id)
> > > > > >  	return IRQ_HANDLED;
> > > > > >  }
> > > > > >
> > > > > > +static void svc_i3c_master_set_slow_address_speed(struct
> > > > > > +svc_i3c_master *master) {
> > > > > > +	struct i3c_bus *bus = i3c_master_get_bus(&master->base);
> > > > > > +	u32 ppbaud, odbaud, odhpp, mconfig;
> > > > > > +
> > > > > > +	master->mctrl_config = readl(master->regs + SVC_I3C_MCONFIG);
> > > > > > +	mconfig = master->mctrl_config;
> > > > > > +
> > > > > > +	/*
> > > > > > +	 * Set the I3C OPEN-DRAIN mode to the FM speed of 50%
> > > > > duty-cycle(400K/2500ns),
> > > > > > +	 * so that the first broadcast is visible to all devices on the i3c bus.
> > > > > > +	 * I3C device with 50ns filter will turn off the filter.
> > > > > > +	 */
> > > > > > +
> > > > > > +	ppbaud = FIELD_GET(GENMASK(11, 8), mconfig);
> > > > > > +	odhpp = 0;
> > > > > > +	odbaud = DIV_ROUND_UP(master->fclk_rate, bus->scl_rate.i2c *
> > > > > > +(2
> > > > > > ++ 2 *
> > > > > ppbaud)) - 1;
> > > > > > +	mconfig &= ~GENMASK(24, 16);
> > > > > > +	mconfig |= SVC_I3C_MCONFIG_ODBAUD(odbaud) |
> > > > > > +SVC_I3C_MCONFIG_ODHPP(odhpp);
> > > > > > +
> > > > > > +	writel(mconfig, master->regs + SVC_I3C_MCONFIG);
> > > > > > +	master->slow_speed = false;
> > > > > > +}
> > > > > > +
> > > > > > +static void svc_i3c_master_set_default_speed(struct
> > > > > > +svc_i3c_master
> > > > > > +*master) {
> > > > > > +	/*
> > > > > > +	 * The bus mode is already determined when the bus is
> > > > > > +initialized, so
> > > > > setting initial
> > > > > > +	 * configuration back to the controller. No need to set it
> > > > > > +in every transfer,
> > > > > just
> > > > > > +	 * restore it once time.
> > > > > > +	 */
> > > > > > +	if (!master->default_speed) {
> > > > > > +		writel(master->mctrl_config, master->regs +
> > > SVC_I3C_MCONFIG);
> > > > > > +		master->default_speed = true;
> > > > > > +	}
> > > > > > +}
> > > > > > +
> > > > > >  static int svc_i3c_master_bus_init(struct
> > > > > > i3c_master_controller
> > > > > > *m) {
> > > > > >  	struct svc_i3c_master *master = to_svc_i3c_master(m); @@
> > > > > > -624,6
> > > > > > +667,8 @@ static int svc_i3c_master_bus_init(struct
> > > > > > +i3c_master_controller
> > > > > *m)
> > > > > >  	      SVC_I3C_MCONFIG_I2CBAUD(i2cbaud);
> > > > > >  	writel(reg, master->regs + SVC_I3C_MCONFIG);
> > > > > >
> > > > > > +	master->slow_speed = true;
> > > > > > +	master->fclk_rate = fclk_rate;
> > > > > >  	/* Master core's registration */
> > > > > >  	ret = i3c_master_get_free_addr(m, 0);
> > > > > >  	if (ret < 0)
> > > > > > @@ -1251,6 +1296,11 @@ static void
> > > > > svc_i3c_master_start_xfer_locked(struct svc_i3c_master *master)
> > > > > >  	for (i = 0; i < xfer->ncmds; i++) {
> > > > > >  		struct svc_i3c_cmd *cmd = &xfer->cmds[i];
> > > > > >
> > > > > > +		if (cmd->slow_address)
> > > > > > +			svc_i3c_master_set_slow_address_speed(master);
> > > > > > +		else
> > > > > > +			svc_i3c_master_set_default_speed(master);
> > > > > > +
> > > > > >  		ret = svc_i3c_master_xfer(master, cmd->rnw, xfer->type,
> > > > > >  					  cmd->addr, cmd->in, cmd->out,
> > > > > >  					  cmd->len, &cmd->actual_len, @@ -1346,6
> > > +1396,11 @@ static
> > > > > > int
> > > > > svc_i3c_master_send_bdcast_ccc_cmd(struct svc_i3c_master
> > > > > *master,
> > > > > >  	cmd->actual_len = 0;
> > > > > >  	cmd->continued = false;
> > > > > >
> > > > > > +	if (master->slow_speed)
> > > > > > +		cmd->slow_address = true;
> > > > > > +	else
> > > > > > +		cmd->slow_address = false;
> > > > > > +
> > > > > >  	mutex_lock(&master->lock);
> > > > > >  	svc_i3c_master_enqueue_xfer(master, xfer);
> > > > > >  	if (!wait_for_completion_timeout(&xfer->comp,
> > > > > > msecs_to_jiffies(1000)))
> > > > > > --
> > > > > > 2.34.1
> > > > > >

Powered by blists - more mailing lists