[PATCH 0/4] i3c: master: svc: some bug fixes

[PATCH 0/4] i3c: master: svc: some bug fixes

[Clark Wang]

Hi,

I am using SVC I3C module recently. I fix some problems and also have a
question.

My question is:
Can I3C bus support pure I2C mode in kernel?
Or in other words, in mixed mode, must there be at least one I3C device on
the I3C bus?

The pure I3C mode works fine. But when only have one I2C device on the
I3C bus, the probe in function i3c_master_bus_init() will go error. Because
there is no one on I3C bus can ACK the I3C message with I3C message speed. Then
it will return error at function i3c_master_rstdaa_locked() because of no ACK
for 0x7e start byte.
When I use the following dtb configuration, the above problem occurs.
&i3c2 {
	#address-cells = <3>;
	#size-cells = <0>;
	pinctrl-names = "default";
	pinctrl-0 = <&pinctrl_i3c2>;
	i2c-scl-hz = <400000>;
	status = "okay";

	lsm6dso_i2c: imu@6a {
		compatible = "st,lsm6dso";
		reg = <0x6a 0x0 0x50>;
	};
};

But I saw a similar configuration example in
/home/nxf47749/work/kernel/i3c/Documentation/devicetree/bindings/i3c/snps,dw-i3c-master.txt.
I wonder if that can work normally?

I know the definition in the specification is:
Mixed xxx Bus: I3C Bus topology with both I2C and I3C Devices present
on the I3C Bus...
But I think it is feasible to use pure I2C mode with I3C module.
I am not sure why the use of pure I2C mode is restricted in the software.

If there are errors in my ideas, please correct me in time. Thank you all.

Here are the fixes.

Clark Wang (4):
  i3c: master: svc: move module reset behind clk enable
  i3c: master: svc: fix atomic issue
  i3c: master: svc: add support for slave to stop returning data
  i3c: master: svc: set ODSTOP to let I2C device see the STOP signal

 drivers/i3c/master/svc-i3c-master.c | 45 +++++++++++++++++++----------
 1 file changed, 30 insertions(+), 15 deletions(-)

-- 
2.25.1


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

[PATCH 1/4] i3c: master: svc: move module reset behind clk enable

[Clark Wang]

Reset I3C module will R/W its regs, so enable its clocks first.

Signed-off-by: Clark Wang <xiaoning.wang@nxp.com>
---
 drivers/i3c/master/svc-i3c-master.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/i3c/master/svc-i3c-master.c b/drivers/i3c/master/svc-i3c-master.c
index 879e5a64acaf..c25a372f6820 100644
--- a/drivers/i3c/master/svc-i3c-master.c
+++ b/drivers/i3c/master/svc-i3c-master.c
@@ -1381,8 +1381,6 @@ static int svc_i3c_master_probe(struct platform_device *pdev)
 
 	master->dev = dev;
 
-	svc_i3c_master_reset(master);
-
 	ret = clk_prepare_enable(master->pclk);
 	if (ret)
 		return ret;
@@ -1419,6 +1417,8 @@ static int svc_i3c_master_probe(struct platform_device *pdev)
 
 	platform_set_drvdata(pdev, master);
 
+	svc_i3c_master_reset(master);
+
 	/* Register the master */
 	ret = i3c_master_register(&master->base, &pdev->dev,
 				  &svc_i3c_master_ops, false);
-- 
2.25.1


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

Re: [PATCH 1/4] i3c: master: svc: move module reset behind clk enable

[Miquel Raynal]

Hi Clark,

Clark Wang <xiaoning.wang@nxp.com> wrote on Thu, 15 Jul 2021 16:24:10
+0800:

> Reset I3C module will R/W its regs, so enable its clocks first.
> 
> Signed-off-by: Clark Wang <xiaoning.wang@nxp.com>

Reviewed-by: Miquel Raynal <miquel.raynal@bootlin.com>

Thanks,
Miquèl

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

[PATCH 2/4] i3c: master: svc: fix atomic issue

[Clark Wang]

do_daa_locked() function is in a spin lock environment, use
readl_poll_timeout_atomic() to replace the origin
readl_poll_timeout().

Signed-off-by: Clark Wang <xiaoning.wang@nxp.com>
---
 drivers/i3c/master/svc-i3c-master.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/i3c/master/svc-i3c-master.c b/drivers/i3c/master/svc-i3c-master.c
index c25a372f6820..9d80435638ea 100644
--- a/drivers/i3c/master/svc-i3c-master.c
+++ b/drivers/i3c/master/svc-i3c-master.c
@@ -656,7 +656,7 @@ static int svc_i3c_master_readb(struct svc_i3c_master *master, u8 *dst,
 	u32 reg;
 
 	for (i = 0; i < len; i++) {
-		ret = readl_poll_timeout(master->regs + SVC_I3C_MSTATUS, reg,
+		ret = readl_poll_timeout_atomic(master->regs + SVC_I3C_MSTATUS, reg,
 					 SVC_I3C_MSTATUS_RXPEND(reg), 0, 1000);
 		if (ret)
 			return ret;
@@ -687,7 +687,7 @@ static int svc_i3c_master_do_daa_locked(struct svc_i3c_master *master,
 		 * Either one slave will send its ID, or the assignment process
 		 * is done.
 		 */
-		ret = readl_poll_timeout(master->regs + SVC_I3C_MSTATUS, reg,
+		ret = readl_poll_timeout_atomic(master->regs + SVC_I3C_MSTATUS, reg,
 					 SVC_I3C_MSTATUS_RXPEND(reg) |
 					 SVC_I3C_MSTATUS_MCTRLDONE(reg),
 					 1, 1000);
@@ -744,7 +744,7 @@ static int svc_i3c_master_do_daa_locked(struct svc_i3c_master *master,
 		}
 
 		/* Wait for the slave to be ready to receive its address */
-		ret = readl_poll_timeout(master->regs + SVC_I3C_MSTATUS, reg,
+		ret = readl_poll_timeout_atomic(master->regs + SVC_I3C_MSTATUS, reg,
 					 SVC_I3C_MSTATUS_MCTRLDONE(reg) &&
 					 SVC_I3C_MSTATUS_STATE_DAA(reg) &&
 					 SVC_I3C_MSTATUS_BETWEEN(reg),
-- 
2.25.1


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

Re: [PATCH 2/4] i3c: master: svc: fix atomic issue

[Miquel Raynal]

Hi Clark,

Clark Wang <xiaoning.wang@nxp.com> wrote on Thu, 15 Jul 2021 16:24:11
+0800:

> do_daa_locked() function is in a spin lock environment, use
> readl_poll_timeout_atomic() to replace the origin
> readl_poll_timeout().
> 
> Signed-off-by: Clark Wang <xiaoning.wang@nxp.com>
> ---
>  drivers/i3c/master/svc-i3c-master.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/i3c/master/svc-i3c-master.c b/drivers/i3c/master/svc-i3c-master.c
> index c25a372f6820..9d80435638ea 100644
> --- a/drivers/i3c/master/svc-i3c-master.c
> +++ b/drivers/i3c/master/svc-i3c-master.c
> @@ -656,7 +656,7 @@ static int svc_i3c_master_readb(struct svc_i3c_master *master, u8 *dst,
>  	u32 reg;
>  
>  	for (i = 0; i < len; i++) {
> -		ret = readl_poll_timeout(master->regs + SVC_I3C_MSTATUS, reg,
> +		ret = readl_poll_timeout_atomic(master->regs + SVC_I3C_MSTATUS, reg,
>  					 SVC_I3C_MSTATUS_RXPEND(reg), 0, 1000);

You forgot to align the parameters of the function here and below.

Otherwise,

Reviewed-by: Miquel Raynal <miquel.raynal@bootlin.com>

>  		if (ret)
>  			return ret;
> @@ -687,7 +687,7 @@ static int svc_i3c_master_do_daa_locked(struct svc_i3c_master *master,
>  		 * Either one slave will send its ID, or the assignment process
>  		 * is done.
>  		 */
> -		ret = readl_poll_timeout(master->regs + SVC_I3C_MSTATUS, reg,
> +		ret = readl_poll_timeout_atomic(master->regs + SVC_I3C_MSTATUS, reg,
>  					 SVC_I3C_MSTATUS_RXPEND(reg) |
>  					 SVC_I3C_MSTATUS_MCTRLDONE(reg),
>  					 1, 1000);
> @@ -744,7 +744,7 @@ static int svc_i3c_master_do_daa_locked(struct svc_i3c_master *master,
>  		}
>  
>  		/* Wait for the slave to be ready to receive its address */
> -		ret = readl_poll_timeout(master->regs + SVC_I3C_MSTATUS, reg,
> +		ret = readl_poll_timeout_atomic(master->regs + SVC_I3C_MSTATUS, reg,
>  					 SVC_I3C_MSTATUS_MCTRLDONE(reg) &&
>  					 SVC_I3C_MSTATUS_STATE_DAA(reg) &&
>  					 SVC_I3C_MSTATUS_BETWEEN(reg),




Thanks,
Miquèl

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

RE: [PATCH 2/4] i3c: master: svc: fix atomic issue

[Clark Wang]

[-- Attachment #1.1: Type: text/plain, Size: 2822 bytes --]


> -----Original Message-----
> From: Miquel Raynal <miquel.raynal@bootlin.com>
> Sent: Friday, July 16, 2021 6:52
> To: Clark Wang <xiaoning.wang@nxp.com>
> Cc: conor.culhane@silvaco.com; alexandre.belloni@bootlin.com;
> vitor.soares@synopsys.com; boris.brezillon@bootlin.com; linux-
> i3c@lists.infradead.org; linux-kernel@vger.kernel.org
> Subject: Re: [PATCH 2/4] i3c: master: svc: fix atomic issue
> 
> Hi Clark,
> 
> Clark Wang <xiaoning.wang@nxp.com> wrote on Thu, 15 Jul 2021 16:24:11
> +0800:
> 
> > do_daa_locked() function is in a spin lock environment, use
> > readl_poll_timeout_atomic() to replace the origin
> > readl_poll_timeout().
> >
> > Signed-off-by: Clark Wang <xiaoning.wang@nxp.com>
> > ---
> >  drivers/i3c/master/svc-i3c-master.c | 6 +++---
> >  1 file changed, 3 insertions(+), 3 deletions(-)
> >
> > diff --git a/drivers/i3c/master/svc-i3c-master.c
> > b/drivers/i3c/master/svc-i3c-master.c
> > index c25a372f6820..9d80435638ea 100644
> > --- a/drivers/i3c/master/svc-i3c-master.c
> > +++ b/drivers/i3c/master/svc-i3c-master.c
> > @@ -656,7 +656,7 @@ static int svc_i3c_master_readb(struct
> svc_i3c_master *master, u8 *dst,
> >  	u32 reg;
> >
> >  	for (i = 0; i < len; i++) {
> > -		ret = readl_poll_timeout(master->regs + SVC_I3C_MSTATUS,
> reg,
> > +		ret = readl_poll_timeout_atomic(master->regs +
> SVC_I3C_MSTATUS,
> > +reg,
> >  					 SVC_I3C_MSTATUS_RXPEND(reg), 0,
> 1000);
> 
> You forgot to align the parameters of the function here and below.

Oh, sorry about this. I will send V2 to correct this.

By the way, can you help to check the question I mentioned in the cover letter?

Thank you very much!

Best Regards,
Clark Wang

> 
> Otherwise,
> 
> Reviewed-by: Miquel Raynal <miquel.raynal@bootlin.com>
> 
> >  		if (ret)
> >  			return ret;
> > @@ -687,7 +687,7 @@ static int svc_i3c_master_do_daa_locked(struct
> svc_i3c_master *master,
> >  		 * Either one slave will send its ID, or the assignment process
> >  		 * is done.
> >  		 */
> > -		ret = readl_poll_timeout(master->regs + SVC_I3C_MSTATUS,
> reg,
> > +		ret = readl_poll_timeout_atomic(master->regs +
> SVC_I3C_MSTATUS,
> > +reg,
> >  					 SVC_I3C_MSTATUS_RXPEND(reg) |
> >
> SVC_I3C_MSTATUS_MCTRLDONE(reg),
> >  					 1, 1000);
> > @@ -744,7 +744,7 @@ static int svc_i3c_master_do_daa_locked(struct
> svc_i3c_master *master,
> >  		}
> >
> >  		/* Wait for the slave to be ready to receive its address */
> > -		ret = readl_poll_timeout(master->regs + SVC_I3C_MSTATUS,
> reg,
> > +		ret = readl_poll_timeout_atomic(master->regs +
> SVC_I3C_MSTATUS,
> > +reg,
> >
> SVC_I3C_MSTATUS_MCTRLDONE(reg) &&
> >  					 SVC_I3C_MSTATUS_STATE_DAA(reg)
> &&
> >  					 SVC_I3C_MSTATUS_BETWEEN(reg),
> 
> 
> 
> 
> Thanks,
> Miquèl

[-- Attachment #1.2: smime.p7s --]
[-- Type: application/pkcs7-signature, Size: 9583 bytes --]

[-- Attachment #2: Type: text/plain, Size: 111 bytes --]

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

[PATCH 3/4] i3c: master: svc: add support for slave to stop returning data

[Clark Wang]

When i3c controller reads data from slave device, slave device can stop
returning data with an ACK after any byte.
Add this support for svc i3c controller. Otherwise, it will go TIMEOUT
error path when the slave device ends the read operation early.

Signed-off-by: Clark Wang <xiaoning.wang@nxp.com>
---
 drivers/i3c/master/svc-i3c-master.c | 28 ++++++++++++++++++++--------
 1 file changed, 20 insertions(+), 8 deletions(-)

diff --git a/drivers/i3c/master/svc-i3c-master.c b/drivers/i3c/master/svc-i3c-master.c
index 9d80435638ea..892e57fec4b0 100644
--- a/drivers/i3c/master/svc-i3c-master.c
+++ b/drivers/i3c/master/svc-i3c-master.c
@@ -865,7 +865,7 @@ static int svc_i3c_master_read(struct svc_i3c_master *master,
 			       u8 *in, unsigned int len)
 {
 	int offset = 0, i, ret;
-	u32 mdctrl;
+	u32 mdctrl, mstatus;
 
 	while (offset < len) {
 		unsigned int count;
@@ -874,8 +874,15 @@ static int svc_i3c_master_read(struct svc_i3c_master *master,
 					 mdctrl,
 					 !(mdctrl & SVC_I3C_MDATACTRL_RXEMPTY),
 					 0, 1000);
-		if (ret)
-			return ret;
+		if (ret) {
+			ret = readl_poll_timeout(master->regs + SVC_I3C_MSTATUS,
+				 mstatus, SVC_I3C_MSTATUS_COMPLETE(mstatus),
+				 0, 1000);
+			if (ret)
+				return ret;
+			else
+				return offset;
+		}
 
 		count = SVC_I3C_MDATACTRL_RXCOUNT(mdctrl);
 		for (i = 0; i < count; i++)
@@ -884,7 +891,7 @@ static int svc_i3c_master_read(struct svc_i3c_master *master,
 		offset += count;
 	}
 
-	return 0;
+	return offset;
 }
 
 static int svc_i3c_master_write(struct svc_i3c_master *master,
@@ -917,7 +924,7 @@ static int svc_i3c_master_write(struct svc_i3c_master *master,
 static int svc_i3c_master_xfer(struct svc_i3c_master *master,
 			       bool rnw, unsigned int xfer_type, u8 addr,
 			       u8 *in, const u8 *out, unsigned int xfer_len,
-			       unsigned int read_len, bool continued)
+			       unsigned int *read_len, bool continued)
 {
 	u32 reg;
 	int ret;
@@ -927,7 +934,7 @@ static int svc_i3c_master_xfer(struct svc_i3c_master *master,
 	       SVC_I3C_MCTRL_IBIRESP_NACK |
 	       SVC_I3C_MCTRL_DIR(rnw) |
 	       SVC_I3C_MCTRL_ADDR(addr) |
-	       SVC_I3C_MCTRL_RDTERM(read_len),
+	       SVC_I3C_MCTRL_RDTERM(*read_len),
 	       master->regs + SVC_I3C_MCTRL);
 
 	ret = readl_poll_timeout(master->regs + SVC_I3C_MSTATUS, reg,
@@ -939,8 +946,10 @@ static int svc_i3c_master_xfer(struct svc_i3c_master *master,
 		ret = svc_i3c_master_read(master, in, xfer_len);
 	else
 		ret = svc_i3c_master_write(master, out, xfer_len);
-	if (ret)
+	if (ret < 0)
 		goto emit_stop;
+	if (rnw)
+		*read_len = ret;
 
 	ret = readl_poll_timeout(master->regs + SVC_I3C_MSTATUS, reg,
 				 SVC_I3C_MSTATUS_COMPLETE(reg), 0, 1000);
@@ -1012,7 +1021,7 @@ static void svc_i3c_master_start_xfer_locked(struct svc_i3c_master *master)
 
 		ret = svc_i3c_master_xfer(master, cmd->rnw, xfer->type,
 					  cmd->addr, cmd->in, cmd->out,
-					  cmd->len, cmd->read_len,
+					  cmd->len, &cmd->read_len,
 					  cmd->continued);
 		if (ret)
 			break;
@@ -1141,6 +1150,9 @@ static int svc_i3c_master_send_direct_ccc_cmd(struct svc_i3c_master *master,
 	if (!wait_for_completion_timeout(&xfer->comp, msecs_to_jiffies(1000)))
 		svc_i3c_master_dequeue_xfer(master, xfer);
 
+	if (cmd->read_len != xfer_len)
+		ccc->dests[0].payload.len = cmd->read_len;
+
 	ret = xfer->ret;
 	svc_i3c_master_free_xfer(xfer);
 
-- 
2.25.1


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

Re: [PATCH 3/4] i3c: master: svc: add support for slave to stop returning data

[Miquel Raynal]

Hi Clark,

Clark Wang <xiaoning.wang@nxp.com> wrote on Thu, 15 Jul 2021 16:24:12
+0800:

> When i3c controller reads data from slave device, slave device can stop
> returning data with an ACK after any byte.
> Add this support for svc i3c controller. Otherwise, it will go TIMEOUT
> error path when the slave device ends the read operation early.

Is this part of the I3C specification? I am not aware about it.

> Signed-off-by: Clark Wang <xiaoning.wang@nxp.com>
> ---
>  drivers/i3c/master/svc-i3c-master.c | 28 ++++++++++++++++++++--------
>  1 file changed, 20 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/i3c/master/svc-i3c-master.c b/drivers/i3c/master/svc-i3c-master.c
> index 9d80435638ea..892e57fec4b0 100644
> --- a/drivers/i3c/master/svc-i3c-master.c
> +++ b/drivers/i3c/master/svc-i3c-master.c
> @@ -865,7 +865,7 @@ static int svc_i3c_master_read(struct svc_i3c_master *master,
>  			       u8 *in, unsigned int len)
>  {
>  	int offset = 0, i, ret;
> -	u32 mdctrl;
> +	u32 mdctrl, mstatus;
>  
>  	while (offset < len) {
>  		unsigned int count;
> @@ -874,8 +874,15 @@ static int svc_i3c_master_read(struct svc_i3c_master *master,
>  					 mdctrl,
>  					 !(mdctrl & SVC_I3C_MDATACTRL_RXEMPTY),
>  					 0, 1000);
> -		if (ret)
> -			return ret;
> +		if (ret) {
> +			ret = readl_poll_timeout(master->regs + SVC_I3C_MSTATUS,
> +				 mstatus, SVC_I3C_MSTATUS_COMPLETE(mstatus),
> +				 0, 1000);
> +			if (ret)
> +				return ret;
> +			else
> +				return offset;
> +		}
>  
>  		count = SVC_I3C_MDATACTRL_RXCOUNT(mdctrl);
>  		for (i = 0; i < count; i++)
> @@ -884,7 +891,7 @@ static int svc_i3c_master_read(struct svc_i3c_master *master,
>  		offset += count;
>  	}
>  
> -	return 0;
> +	return offset;
>  }
>  
>  static int svc_i3c_master_write(struct svc_i3c_master *master,
> @@ -917,7 +924,7 @@ static int svc_i3c_master_write(struct svc_i3c_master *master,
>  static int svc_i3c_master_xfer(struct svc_i3c_master *master,
>  			       bool rnw, unsigned int xfer_type, u8 addr,
>  			       u8 *in, const u8 *out, unsigned int xfer_len,
> -			       unsigned int read_len, bool continued)
> +			       unsigned int *read_len, bool continued)
>  {
>  	u32 reg;
>  	int ret;
> @@ -927,7 +934,7 @@ static int svc_i3c_master_xfer(struct svc_i3c_master *master,
>  	       SVC_I3C_MCTRL_IBIRESP_NACK |
>  	       SVC_I3C_MCTRL_DIR(rnw) |
>  	       SVC_I3C_MCTRL_ADDR(addr) |
> -	       SVC_I3C_MCTRL_RDTERM(read_len),
> +	       SVC_I3C_MCTRL_RDTERM(*read_len),
>  	       master->regs + SVC_I3C_MCTRL);
>  
>  	ret = readl_poll_timeout(master->regs + SVC_I3C_MSTATUS, reg,
> @@ -939,8 +946,10 @@ static int svc_i3c_master_xfer(struct svc_i3c_master *master,
>  		ret = svc_i3c_master_read(master, in, xfer_len);
>  	else
>  		ret = svc_i3c_master_write(master, out, xfer_len);
> -	if (ret)
> +	if (ret < 0)
>  		goto emit_stop;
> +	if (rnw)
> +		*read_len = ret;
>  
>  	ret = readl_poll_timeout(master->regs + SVC_I3C_MSTATUS, reg,
>  				 SVC_I3C_MSTATUS_COMPLETE(reg), 0, 1000);
> @@ -1012,7 +1021,7 @@ static void svc_i3c_master_start_xfer_locked(struct svc_i3c_master *master)
>  
>  		ret = svc_i3c_master_xfer(master, cmd->rnw, xfer->type,
>  					  cmd->addr, cmd->in, cmd->out,
> -					  cmd->len, cmd->read_len,
> +					  cmd->len, &cmd->read_len,
>  					  cmd->continued);
>  		if (ret)
>  			break;
> @@ -1141,6 +1150,9 @@ static int svc_i3c_master_send_direct_ccc_cmd(struct svc_i3c_master *master,
>  	if (!wait_for_completion_timeout(&xfer->comp, msecs_to_jiffies(1000)))
>  		svc_i3c_master_dequeue_xfer(master, xfer);
>  
> +	if (cmd->read_len != xfer_len)
> +		ccc->dests[0].payload.len = cmd->read_len;
> +
>  	ret = xfer->ret;
>  	svc_i3c_master_free_xfer(xfer);
>  

Thanks,
Miquèl

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

RE: [PATCH 3/4] i3c: master: svc: add support for slave to stop returning data

[Clark Wang]

[-- Attachment #1.1: Type: text/plain, Size: 5096 bytes --]

Hi Miquel,

Thanks for your review!

> -----Original Message-----
> From: Miquel Raynal <miquel.raynal@bootlin.com>
> Sent: Friday, July 16, 2021 6:56
> To: Clark Wang <xiaoning.wang@nxp.com>
> Cc: conor.culhane@silvaco.com; alexandre.belloni@bootlin.com;
> vitor.soares@synopsys.com; boris.brezillon@bootlin.com; linux-
> i3c@lists.infradead.org; linux-kernel@vger.kernel.org
> Subject: Re: [PATCH 3/4] i3c: master: svc: add support for slave to stop
> returning data
> 
> Hi Clark,
> 
> Clark Wang <xiaoning.wang@nxp.com> wrote on Thu, 15 Jul 2021 16:24:12
> +0800:
> 
> > When i3c controller reads data from slave device, slave device can
> > stop returning data with an ACK after any byte.
> > Add this support for svc i3c controller. Otherwise, it will go TIMEOUT
> > error path when the slave device ends the read operation early.
> 
> Is this part of the I3C specification? I am not aware about it.

Yes. I think the spec mentions about this. You can check section "5.1.2.3 I3C SDR Data Words" of <mipi_i3c_basic_specification>.
[3. Ninth Bit of SDR Slave Returned (Read) Data as End-of-Data: In I2C, the ninth Data bit from
Slave to Master is an ACK by the Master. By contrast, in I3C this bit allows the Slave to end a
Read, and allows the Master to Abort a Read. In SDR terms, the ninth bit of Read data is referred
to as the T-Bit (for ‘Transition’) (see Section 5.1.2.3.4).]

Best Regards,
Clark Wang

> 
> > Signed-off-by: Clark Wang <xiaoning.wang@nxp.com>
> > ---
> >  drivers/i3c/master/svc-i3c-master.c | 28 ++++++++++++++++++++--------
> >  1 file changed, 20 insertions(+), 8 deletions(-)
> >
> > diff --git a/drivers/i3c/master/svc-i3c-master.c
> > b/drivers/i3c/master/svc-i3c-master.c
> > index 9d80435638ea..892e57fec4b0 100644
> > --- a/drivers/i3c/master/svc-i3c-master.c
> > +++ b/drivers/i3c/master/svc-i3c-master.c
> > @@ -865,7 +865,7 @@ static int svc_i3c_master_read(struct
> svc_i3c_master *master,
> >  			       u8 *in, unsigned int len)
> >  {
> >  	int offset = 0, i, ret;
> > -	u32 mdctrl;
> > +	u32 mdctrl, mstatus;
> >
> >  	while (offset < len) {
> >  		unsigned int count;
> > @@ -874,8 +874,15 @@ static int svc_i3c_master_read(struct
> svc_i3c_master *master,
> >  					 mdctrl,
> >  					 !(mdctrl &
> SVC_I3C_MDATACTRL_RXEMPTY),
> >  					 0, 1000);
> > -		if (ret)
> > -			return ret;
> > +		if (ret) {
> > +			ret = readl_poll_timeout(master->regs +
> SVC_I3C_MSTATUS,
> > +				 mstatus,
> SVC_I3C_MSTATUS_COMPLETE(mstatus),
> > +				 0, 1000);
> > +			if (ret)
> > +				return ret;
> > +			else
> > +				return offset;
> > +		}
> >
> >  		count = SVC_I3C_MDATACTRL_RXCOUNT(mdctrl);
> >  		for (i = 0; i < count; i++)
> > @@ -884,7 +891,7 @@ static int svc_i3c_master_read(struct
> svc_i3c_master *master,
> >  		offset += count;
> >  	}
> >
> > -	return 0;
> > +	return offset;
> >  }
> >
> >  static int svc_i3c_master_write(struct svc_i3c_master *master, @@
> > -917,7 +924,7 @@ static int svc_i3c_master_write(struct svc_i3c_master
> > *master,  static int svc_i3c_master_xfer(struct svc_i3c_master *master,
> >  			       bool rnw, unsigned int xfer_type, u8 addr,
> >  			       u8 *in, const u8 *out, unsigned int xfer_len,
> > -			       unsigned int read_len, bool continued)
> > +			       unsigned int *read_len, bool continued)
> >  {
> >  	u32 reg;
> >  	int ret;
> > @@ -927,7 +934,7 @@ static int svc_i3c_master_xfer(struct svc_i3c_master
> *master,
> >  	       SVC_I3C_MCTRL_IBIRESP_NACK |
> >  	       SVC_I3C_MCTRL_DIR(rnw) |
> >  	       SVC_I3C_MCTRL_ADDR(addr) |
> > -	       SVC_I3C_MCTRL_RDTERM(read_len),
> > +	       SVC_I3C_MCTRL_RDTERM(*read_len),
> >  	       master->regs + SVC_I3C_MCTRL);
> >
> >  	ret = readl_poll_timeout(master->regs + SVC_I3C_MSTATUS, reg,
> @@
> > -939,8 +946,10 @@ static int svc_i3c_master_xfer(struct svc_i3c_master
> *master,
> >  		ret = svc_i3c_master_read(master, in, xfer_len);
> >  	else
> >  		ret = svc_i3c_master_write(master, out, xfer_len);
> > -	if (ret)
> > +	if (ret < 0)
> >  		goto emit_stop;
> > +	if (rnw)
> > +		*read_len = ret;
> >
> >  	ret = readl_poll_timeout(master->regs + SVC_I3C_MSTATUS, reg,
> >  				 SVC_I3C_MSTATUS_COMPLETE(reg), 0,
> 1000); @@ -1012,7 +1021,7 @@
> > static void svc_i3c_master_start_xfer_locked(struct svc_i3c_master
> > *master)
> >
> >  		ret = svc_i3c_master_xfer(master, cmd->rnw, xfer->type,
> >  					  cmd->addr, cmd->in, cmd->out,
> > -					  cmd->len, cmd->read_len,
> > +					  cmd->len, &cmd->read_len,
> >  					  cmd->continued);
> >  		if (ret)
> >  			break;
> > @@ -1141,6 +1150,9 @@ static int
> svc_i3c_master_send_direct_ccc_cmd(struct svc_i3c_master *master,
> >  	if (!wait_for_completion_timeout(&xfer->comp,
> msecs_to_jiffies(1000)))
> >  		svc_i3c_master_dequeue_xfer(master, xfer);
> >
> > +	if (cmd->read_len != xfer_len)
> > +		ccc->dests[0].payload.len = cmd->read_len;
> > +
> >  	ret = xfer->ret;
> >  	svc_i3c_master_free_xfer(xfer);
> >
> 
> Thanks,
> Miquèl

[-- Attachment #1.2: smime.p7s --]
[-- Type: application/pkcs7-signature, Size: 9583 bytes --]

[-- Attachment #2: Type: text/plain, Size: 111 bytes --]

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

[PATCH 4/4] i3c: master: svc: set ODSTOP to let I2C device see the STOP signal

[Clark Wang]

If using I2C/I3C mixed mode, need to set ODSTOP. Otherwise, the I2C
devices cannot see the stop signal. It may cause message sending errors.

Signed-off-by: Clark Wang <xiaoning.wang@nxp.com>
---
 drivers/i3c/master/svc-i3c-master.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/drivers/i3c/master/svc-i3c-master.c b/drivers/i3c/master/svc-i3c-master.c
index 892e57fec4b0..b05cc7f521e6 100644
--- a/drivers/i3c/master/svc-i3c-master.c
+++ b/drivers/i3c/master/svc-i3c-master.c
@@ -449,7 +449,7 @@ static int svc_i3c_master_bus_init(struct i3c_master_controller *m)
 	struct i3c_device_info info = {};
 	unsigned long fclk_rate, fclk_period_ns;
 	unsigned int high_period_ns, od_low_period_ns;
-	u32 ppbaud, pplow, odhpp, odbaud, i2cbaud, reg;
+	u32 ppbaud, pplow, odhpp, odbaud, odstop, i2cbaud, reg;
 	int ret;
 
 	/* Timings derivation */
@@ -479,6 +479,7 @@ static int svc_i3c_master_bus_init(struct i3c_master_controller *m)
 	switch (bus->mode) {
 	case I3C_BUS_MODE_PURE:
 		i2cbaud = 0;
+		odstop = 0;
 		break;
 	case I3C_BUS_MODE_MIXED_FAST:
 	case I3C_BUS_MODE_MIXED_LIMITED:
@@ -487,6 +488,7 @@ static int svc_i3c_master_bus_init(struct i3c_master_controller *m)
 		 * between the high and low period does not really matter.
 		 */
 		i2cbaud = DIV_ROUND_UP(1000, od_low_period_ns) - 2;
+		odstop = 1;
 		break;
 	case I3C_BUS_MODE_MIXED_SLOW:
 		/*
@@ -494,6 +496,7 @@ static int svc_i3c_master_bus_init(struct i3c_master_controller *m)
 		 * constraints as the FM+ mode.
 		 */
 		i2cbaud = DIV_ROUND_UP(2500, od_low_period_ns) - 2;
+		odstop = 1;
 		break;
 	default:
 		return -EINVAL;
@@ -502,7 +505,7 @@ static int svc_i3c_master_bus_init(struct i3c_master_controller *m)
 	reg = SVC_I3C_MCONFIG_MASTER_EN |
 	      SVC_I3C_MCONFIG_DISTO(0) |
 	      SVC_I3C_MCONFIG_HKEEP(0) |
-	      SVC_I3C_MCONFIG_ODSTOP(0) |
+	      SVC_I3C_MCONFIG_ODSTOP(odstop) |
 	      SVC_I3C_MCONFIG_PPBAUD(ppbaud) |
 	      SVC_I3C_MCONFIG_PPLOW(pplow) |
 	      SVC_I3C_MCONFIG_ODBAUD(odbaud) |
-- 
2.25.1


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

Re: [PATCH 4/4] i3c: master: svc: set ODSTOP to let I2C device see the STOP signal

[Miquel Raynal]

Hi Clark,

Clark Wang <xiaoning.wang@nxp.com> wrote on Thu, 15 Jul 2021 16:24:13
+0800:

> If using I2C/I3C mixed mode, need to set ODSTOP. Otherwise, the I2C
> devices cannot see the stop signal. It may cause message sending errors.

Reviewed-by: Miquel Raynal <miquel.raynal@bootlin.com>

> 
> Signed-off-by: Clark Wang <xiaoning.wang@nxp.com>
> ---
>  drivers/i3c/master/svc-i3c-master.c | 7 +++++--
>  1 file changed, 5 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/i3c/master/svc-i3c-master.c b/drivers/i3c/master/svc-i3c-master.c
> index 892e57fec4b0..b05cc7f521e6 100644
> --- a/drivers/i3c/master/svc-i3c-master.c
> +++ b/drivers/i3c/master/svc-i3c-master.c
> @@ -449,7 +449,7 @@ static int svc_i3c_master_bus_init(struct i3c_master_controller *m)
>  	struct i3c_device_info info = {};
>  	unsigned long fclk_rate, fclk_period_ns;
>  	unsigned int high_period_ns, od_low_period_ns;
> -	u32 ppbaud, pplow, odhpp, odbaud, i2cbaud, reg;
> +	u32 ppbaud, pplow, odhpp, odbaud, odstop, i2cbaud, reg;
>  	int ret;
>  
>  	/* Timings derivation */
> @@ -479,6 +479,7 @@ static int svc_i3c_master_bus_init(struct i3c_master_controller *m)
>  	switch (bus->mode) {
>  	case I3C_BUS_MODE_PURE:
>  		i2cbaud = 0;
> +		odstop = 0;
>  		break;
>  	case I3C_BUS_MODE_MIXED_FAST:
>  	case I3C_BUS_MODE_MIXED_LIMITED:
> @@ -487,6 +488,7 @@ static int svc_i3c_master_bus_init(struct i3c_master_controller *m)
>  		 * between the high and low period does not really matter.
>  		 */
>  		i2cbaud = DIV_ROUND_UP(1000, od_low_period_ns) - 2;
> +		odstop = 1;
>  		break;
>  	case I3C_BUS_MODE_MIXED_SLOW:
>  		/*
> @@ -494,6 +496,7 @@ static int svc_i3c_master_bus_init(struct i3c_master_controller *m)
>  		 * constraints as the FM+ mode.
>  		 */
>  		i2cbaud = DIV_ROUND_UP(2500, od_low_period_ns) - 2;
> +		odstop = 1;
>  		break;
>  	default:
>  		return -EINVAL;
> @@ -502,7 +505,7 @@ static int svc_i3c_master_bus_init(struct i3c_master_controller *m)
>  	reg = SVC_I3C_MCONFIG_MASTER_EN |
>  	      SVC_I3C_MCONFIG_DISTO(0) |
>  	      SVC_I3C_MCONFIG_HKEEP(0) |
> -	      SVC_I3C_MCONFIG_ODSTOP(0) |
> +	      SVC_I3C_MCONFIG_ODSTOP(odstop) |
>  	      SVC_I3C_MCONFIG_PPBAUD(ppbaud) |
>  	      SVC_I3C_MCONFIG_PPLOW(pplow) |
>  	      SVC_I3C_MCONFIG_ODBAUD(odbaud) |




Thanks,
Miquèl

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

Re: [PATCH 0/4] i3c: master: svc: some bug fixes

[Miquel Raynal]

Hi Clark,

Clark Wang <xiaoning.wang@nxp.com> wrote on Thu, 15 Jul 2021 16:24:09
+0800:

> Hi,
> 
> I am using SVC I3C module recently. I fix some problems and also have a
> question.
> 
> My question is:
> Can I3C bus support pure I2C mode in kernel?
> Or in other words, in mixed mode, must there be at least one I3C device on
> the I3C bus?
> 
> The pure I3C mode works fine. But when only have one I2C device on the
> I3C bus, the probe in function i3c_master_bus_init() will go error. Because
> there is no one on I3C bus can ACK the I3C message with I3C message speed. Then
> it will return error at function i3c_master_rstdaa_locked() because of no ACK
> for 0x7e start byte.
> When I use the following dtb configuration, the above problem occurs.
> &i3c2 {
> 	#address-cells = <3>;
> 	#size-cells = <0>;
> 	pinctrl-names = "default";
> 	pinctrl-0 = <&pinctrl_i3c2>;
> 	i2c-scl-hz = <400000>;
> 	status = "okay";
> 
> 	lsm6dso_i2c: imu@6a {
> 		compatible = "st,lsm6dso";
> 		reg = <0x6a 0x0 0x50>;
> 	};
> };
> 
> But I saw a similar configuration example in
> /home/nxf47749/work/kernel/i3c/Documentation/devicetree/bindings/i3c/snps,dw-i3c-master.txt.
> I wonder if that can work normally?
> 
> I know the definition in the specification is:
> Mixed xxx Bus: I3C Bus topology with both I2C and I3C Devices present
> on the I3C Bus...
> But I think it is feasible to use pure I2C mode with I3C module.
> I am not sure why the use of pure I2C mode is restricted in the software.
> 
> If there are errors in my ideas, please correct me in time. Thank you all.

As you pointed out, I am not aware of a specific I2C only bus setting
but if you find a way to workaround the issue raised above by software
in a rather clean way, then... why not?

> Here are the fixes.
> 
> Clark Wang (4):
>   i3c: master: svc: move module reset behind clk enable
>   i3c: master: svc: fix atomic issue
>   i3c: master: svc: add support for slave to stop returning data
>   i3c: master: svc: set ODSTOP to let I2C device see the STOP signal
> 
>  drivers/i3c/master/svc-i3c-master.c | 45 +++++++++++++++++++----------
>  1 file changed, 30 insertions(+), 15 deletions(-)
> 

Thanks,
Miquèl

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

RE: [PATCH 0/4] i3c: master: svc: some bug fixes

[Clark Wang]

[-- Attachment #1.1: Type: text/plain, Size: 3026 bytes --]


> -----Original Message-----
> From: Miquel Raynal <miquel.raynal@bootlin.com>
> Sent: Friday, July 16, 2021 15:52
> To: Clark Wang <xiaoning.wang@nxp.com>
> Cc: conor.culhane@silvaco.com; alexandre.belloni@bootlin.com;
> vitor.soares@synopsys.com; boris.brezillon@bootlin.com; linux-
> i3c@lists.infradead.org; linux-kernel@vger.kernel.org
> Subject: Re: [PATCH 0/4] i3c: master: svc: some bug fixes
> 
> Hi Clark,
> 
> Clark Wang <xiaoning.wang@nxp.com> wrote on Thu, 15 Jul 2021 16:24:09
> +0800:
> 
> > Hi,
> >
> > I am using SVC I3C module recently. I fix some problems and also have a
> > question.
> >
> > My question is:
> > Can I3C bus support pure I2C mode in kernel?
> > Or in other words, in mixed mode, must there be at least one I3C device on
> > the I3C bus?
> >
> > The pure I3C mode works fine. But when only have one I2C device on the
> > I3C bus, the probe in function i3c_master_bus_init() will go error. Because
> > there is no one on I3C bus can ACK the I3C message with I3C message
> speed. Then
> > it will return error at function i3c_master_rstdaa_locked() because of no
> ACK
> > for 0x7e start byte.
> > When I use the following dtb configuration, the above problem occurs.
> > &i3c2 {
> > 	#address-cells = <3>;
> > 	#size-cells = <0>;
> > 	pinctrl-names = "default";
> > 	pinctrl-0 = <&pinctrl_i3c2>;
> > 	i2c-scl-hz = <400000>;
> > 	status = "okay";
> >
> > 	lsm6dso_i2c: imu@6a {
> > 		compatible = "st,lsm6dso";
> > 		reg = <0x6a 0x0 0x50>;
> > 	};
> > };
> >
> > But I saw a similar configuration example in
> >
> /home/nxf47749/work/kernel/i3c/Documentation/devicetree/bindings/i3c/
> snps,dw-i3c-master.txt.
> > I wonder if that can work normally?
> >
> > I know the definition in the specification is:
> > Mixed xxx Bus: I3C Bus topology with both I2C and I3C Devices present
> > on the I3C Bus...
> > But I think it is feasible to use pure I2C mode with I3C module.
> > I am not sure why the use of pure I2C mode is restricted in the software.
> >
> > If there are errors in my ideas, please correct me in time. Thank you all.
> 
> As you pointed out, I am not aware of a specific I2C only bus setting
> but if you find a way to workaround the issue raised above by software
> in a rather clean way, then... why not?

Hi Miquel,

Ok. At present, I just commented part of the code for registering i3c for pure I2C mode testing. I will try to make a clean revision afterwards.
Thank you very much for your suggestion!

Best Regards,
Clark Wang

> 
> > Here are the fixes.
> >
> > Clark Wang (4):
> >   i3c: master: svc: move module reset behind clk enable
> >   i3c: master: svc: fix atomic issue
> >   i3c: master: svc: add support for slave to stop returning data
> >   i3c: master: svc: set ODSTOP to let I2C device see the STOP signal
> >
> >  drivers/i3c/master/svc-i3c-master.c | 45 +++++++++++++++++++----------
> >  1 file changed, 30 insertions(+), 15 deletions(-)
> >
> 
> Thanks,
> Miquèl

[-- Attachment #1.2: smime.p7s --]
[-- Type: application/pkcs7-signature, Size: 9583 bytes --]

[-- Attachment #2: Type: text/plain, Size: 111 bytes --]

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