[PATCH v2] iio: imu: st_lsm6dsx: disable I3C support during device reset

[PATCH v2] iio: imu: st_lsm6dsx: disable I3C support during device reset

[Lorenzo Bianconi]

Disable MIPI I3C during device reset in order to avoid
possible races on interrupt line 1. If the first interrupt
line is asserted during hw reset the device will work in
I3C-only mode

Reported-by: Mario Tesi <mario.tesi@st.com>
Fixes: 2660b0080bb2 ("iio: imu: st_lsm6dsx: add i3c basic support for LSM6DSO and LSM6DSR")
Signed-off-by: Lorenzo Bianconi <lorenzo@kernel.org>
---
Changes since v1:
- fix comment syntax
---
 drivers/iio/imu/st_lsm6dsx/st_lsm6dsx.h      |  2 ++
 drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c | 31 ++++++++++++++++++++
 2 files changed, 33 insertions(+)

diff --git a/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx.h b/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx.h
index f2113a63721a..dfcbe7c42493 100644
--- a/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx.h
+++ b/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx.h
@@ -266,6 +266,7 @@ struct st_lsm6dsx_ext_dev_settings {
  * @wai: Sensor WhoAmI default value.
  * @reset: register address for reset.
  * @boot: register address for boot.
+ * @i3c_disable:  register address for enabling/disabling I3C (addr + mask).
  * @bdu: register address for Block Data Update.
  * @max_fifo_size: Sensor max fifo length in FIFO words.
  * @id: List of hw id/device name supported by the driver configuration.
@@ -284,6 +285,7 @@ struct st_lsm6dsx_settings {
 	u8 wai;
 	struct st_lsm6dsx_reg reset;
 	struct st_lsm6dsx_reg boot;
+	struct st_lsm6dsx_reg i3c_disable;
 	struct st_lsm6dsx_reg bdu;
 	u16 max_fifo_size;
 	struct {
diff --git a/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c b/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c
index 84d219ae6aee..a2e775d6eaa0 100644
--- a/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c
+++ b/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c
@@ -751,6 +751,10 @@ static const struct st_lsm6dsx_settings st_lsm6dsx_sensor_settings[] = {
 			.addr = 0x12,
 			.mask = BIT(7),
 		},
+		.i3c_disable = {
+			.addr = 0x18,
+			.mask = BIT(1),
+		},
 		.bdu = {
 			.addr = 0x12,
 			.mask = BIT(6),
@@ -1128,6 +1132,10 @@ static const struct st_lsm6dsx_settings st_lsm6dsx_sensor_settings[] = {
 			.addr = 0x12,
 			.mask = BIT(7),
 		},
+		.i3c_disable = {
+			.addr = 0x18,
+			.mask = BIT(1),
+		},
 		.bdu = {
 			.addr = 0x12,
 			.mask = BIT(6),
@@ -2041,6 +2049,20 @@ static int st_lsm6dsx_init_device(struct st_lsm6dsx_hw *hw)
 	const struct st_lsm6dsx_reg *reg;
 	int err;
 
+	/*
+	 * disable MIPI I3C during device reset in order to avoid
+	 * possible races on interrupt line 1. If the first interrupt
+	 * line is asserted during hw reset the device will work in
+	 * I3C-only mode
+	 */
+	if (hw->settings->i3c_disable.addr) {
+		reg = &hw->settings->i3c_disable;
+		err = regmap_update_bits(hw->regmap, reg->addr, reg->mask,
+					 ST_LSM6DSX_SHIFT_VAL(1, reg->mask));
+		if (err < 0)
+			return err;
+	}
+
 	/* device sw reset */
 	reg = &hw->settings->reset;
 	err = regmap_update_bits(hw->regmap, reg->addr, reg->mask,
@@ -2059,6 +2081,15 @@ static int st_lsm6dsx_init_device(struct st_lsm6dsx_hw *hw)
 
 	msleep(50);
 
+	/* enable MIPI I3C */
+	if (hw->settings->i3c_disable.addr) {
+		reg = &hw->settings->i3c_disable;
+		err = regmap_update_bits(hw->regmap, reg->addr, reg->mask,
+					 ST_LSM6DSX_SHIFT_VAL(0, reg->mask));
+		if (err < 0)
+			return err;
+	}
+
 	/* enable Block Data Update */
 	reg = &hw->settings->bdu;
 	err = regmap_update_bits(hw->regmap, reg->addr, reg->mask,
-- 
2.24.1

RE: [PATCH v2] iio: imu: st_lsm6dsx: disable I3C support during device reset

[Vitor Soares]

Hi Lorenzo,

From: Lorenzo Bianconi <lorenzo@kernel.org>
Date: Sun, Mar 08, 2020 at 00:06:03

> Disable MIPI I3C during device reset in order to avoid
> possible races on interrupt line 1. If the first interrupt
> line is asserted during hw reset the device will work in
> I3C-only mode
> 
> Reported-by: Mario Tesi <mario.tesi@st.com>
> Fixes: 2660b0080bb2 ("iio: imu: st_lsm6dsx: add i3c basic support for LSM6DSO and LSM6DSR")
> Signed-off-by: Lorenzo Bianconi <lorenzo@kernel.org>
> ---
> Changes since v1:
> - fix comment syntax
> ---
>  drivers/iio/imu/st_lsm6dsx/st_lsm6dsx.h      |  2 ++
>  drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c | 31 ++++++++++++++++++++
>  2 files changed, 33 insertions(+)
> 
> diff --git a/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx.h b/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx.h
> index f2113a63721a..dfcbe7c42493 100644
> --- a/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx.h
> +++ b/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx.h
> @@ -266,6 +266,7 @@ struct st_lsm6dsx_ext_dev_settings {
>   * @wai: Sensor WhoAmI default value.
>   * @reset: register address for reset.
>   * @boot: register address for boot.
> + * @i3c_disable:  register address for enabling/disabling I3C (addr + mask).
>   * @bdu: register address for Block Data Update.
>   * @max_fifo_size: Sensor max fifo length in FIFO words.
>   * @id: List of hw id/device name supported by the driver configuration.
> @@ -284,6 +285,7 @@ struct st_lsm6dsx_settings {
>  	u8 wai;
>  	struct st_lsm6dsx_reg reset;
>  	struct st_lsm6dsx_reg boot;
> +	struct st_lsm6dsx_reg i3c_disable;
>  	struct st_lsm6dsx_reg bdu;
>  	u16 max_fifo_size;
>  	struct {
> diff --git a/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c b/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c
> index 84d219ae6aee..a2e775d6eaa0 100644
> --- a/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c
> +++ b/drivers/iio/imu/st_lsm6dsx/st_lsm6dsx_core.c
> @@ -751,6 +751,10 @@ static const struct st_lsm6dsx_settings st_lsm6dsx_sensor_settings[] = {
>  			.addr = 0x12,
>  			.mask = BIT(7),
>  		},
> +		.i3c_disable = {
> +			.addr = 0x18,
> +			.mask = BIT(1),
> +		},
>  		.bdu = {
>  			.addr = 0x12,
>  			.mask = BIT(6),
> @@ -1128,6 +1132,10 @@ static const struct st_lsm6dsx_settings st_lsm6dsx_sensor_settings[] = {
>  			.addr = 0x12,
>  			.mask = BIT(7),
>  		},
> +		.i3c_disable = {
> +			.addr = 0x18,
> +			.mask = BIT(1),
> +		},
>  		.bdu = {
>  			.addr = 0x12,
>  			.mask = BIT(6),
> @@ -2041,6 +2049,20 @@ static int st_lsm6dsx_init_device(struct st_lsm6dsx_hw *hw)
>  	const struct st_lsm6dsx_reg *reg;
>  	int err;
>  
> +	/*
> +	 * disable MIPI I3C during device reset in order to avoid
> +	 * possible races on interrupt line 1. If the first interrupt
> +	 * line is asserted during hw reset the device will work in
> +	 * I3C-only mode
> +	 */
> +	if (hw->settings->i3c_disable.addr) {
> +		reg = &hw->settings->i3c_disable;
> +		err = regmap_update_bits(hw->regmap, reg->addr, reg->mask,
> +					 ST_LSM6DSX_SHIFT_VAL(1, reg->mask));
> +		if (err < 0)
> +			return err;
> +	}
> +

After disable the i3c interface the dynamic address is no more accessible 
and fails the initialization.

Best regards,
Vitor Soares

>  	/* device sw reset */
>  	reg = &hw->settings->reset;
>  	err = regmap_update_bits(hw->regmap, reg->addr, reg->mask,
> @@ -2059,6 +2081,15 @@ static int st_lsm6dsx_init_device(struct st_lsm6dsx_hw *hw)
>  
>  	msleep(50);
>  
> +	/* enable MIPI I3C */
> +	if (hw->settings->i3c_disable.addr) {
> +		reg = &hw->settings->i3c_disable;
> +		err = regmap_update_bits(hw->regmap, reg->addr, reg->mask,
> +					 ST_LSM6DSX_SHIFT_VAL(0, reg->mask));
> +		if (err < 0)
> +			return err;
> +	}
> +
>  	/* enable Block Data Update */
>  	reg = &hw->settings->bdu;
>  	err = regmap_update_bits(hw->regmap, reg->addr, reg->mask,
> -- 
> 2.24.1

Re: [PATCH v2] iio: imu: st_lsm6dsx: disable I3C support during device reset

[Lorenzo Bianconi]

[-- Attachment #1: Type: text/plain, Size: 1317 bytes --]

> Hi Lorenzo,
> 
> From: Lorenzo Bianconi <lorenzo@kernel.org>
> Date: Sun, Mar 08, 2020 at 00:06:03
> 
> > Disable MIPI I3C during device reset in order to avoid
> > possible races on interrupt line 1. If the first interrupt
> > line is asserted during hw reset the device will work in
> > I3C-only mode
> > 

[...]

> > +
> 
> After disable the i3c interface the dynamic address is no more accessible 
> and fails the initialization.
> 

Hi Vitor,

thx for testing it. What do you mean here?
Is int1 set to vdd in your test?

Regards,
Lorenzo

> Best regards,
> Vitor Soares
> 
> >  	/* device sw reset */
> >  	reg = &hw->settings->reset;
> >  	err = regmap_update_bits(hw->regmap, reg->addr, reg->mask,
> > @@ -2059,6 +2081,15 @@ static int st_lsm6dsx_init_device(struct st_lsm6dsx_hw *hw)
> >  
> >  	msleep(50);
> >  
> > +	/* enable MIPI I3C */
> > +	if (hw->settings->i3c_disable.addr) {
> > +		reg = &hw->settings->i3c_disable;
> > +		err = regmap_update_bits(hw->regmap, reg->addr, reg->mask,
> > +					 ST_LSM6DSX_SHIFT_VAL(0, reg->mask));
> > +		if (err < 0)
> > +			return err;
> > +	}
> > +
> >  	/* enable Block Data Update */
> >  	reg = &hw->settings->bdu;
> >  	err = regmap_update_bits(hw->regmap, reg->addr, reg->mask,
> > -- 
> > 2.24.1
> 
> 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

RE: [PATCH v2] iio: imu: st_lsm6dsx: disable I3C support during device reset

[Vitor Soares]

From: Lorenzo Bianconi <lorenzo@kernel.org>
Date: Mon, Mar 09, 2020 at 15:01:01

> > Hi Lorenzo,
> > 
> > From: Lorenzo Bianconi <lorenzo@kernel.org>
> > Date: Sun, Mar 08, 2020 at 00:06:03
> > 
> > > Disable MIPI I3C during device reset in order to avoid
> > > possible races on interrupt line 1. If the first interrupt
> > > line is asserted during hw reset the device will work in
> > > I3C-only mode
> > > 
> 
> [...]
> 
> > > +
> > 
> > After disable the i3c interface the dynamic address is no more accessible 
> > and fails the initialization.
> > 
> 
> Hi Vitor,
> 
> thx for testing it. What do you mean here?
> Is int1 set to vdd in your test?
> 
> Regards,
> Lorenzo

Yes, according with figure 14 of lsm6dso datasheet.

Is there any way to clear the INT1 before the hw reset?

Best regards,
Vitor Soares

> 
> > Best regards,
> > Vitor Soares
> > 
> > >  	/* device sw reset */
> > >  	reg = &hw->settings->reset;
> > >  	err = regmap_update_bits(hw->regmap, reg->addr, reg->mask,
> > > @@ -2059,6 +2081,15 @@ static int st_lsm6dsx_init_device(struct st_lsm6dsx_hw *hw)
> > >  
> > >  	msleep(50);
> > >  
> > > +	/* enable MIPI I3C */
> > > +	if (hw->settings->i3c_disable.addr) {
> > > +		reg = &hw->settings->i3c_disable;
> > > +		err = regmap_update_bits(hw->regmap, reg->addr, reg->mask,
> > > +					 ST_LSM6DSX_SHIFT_VAL(0, reg->mask));
> > > +		if (err < 0)
> > > +			return err;
> > > +	}
> > > +
> > >  	/* enable Block Data Update */
> > >  	reg = &hw->settings->bdu;
> > >  	err = regmap_update_bits(hw->regmap, reg->addr, reg->mask,
> > > -- 
> > > 2.24.1
> > 
> > 

Re: [PATCH v2] iio: imu: st_lsm6dsx: disable I3C support during device reset

[Lorenzo Bianconi]

[-- Attachment #1: Type: text/plain, Size: 2020 bytes --]

> From: Lorenzo Bianconi <lorenzo@kernel.org>
> Date: Mon, Mar 09, 2020 at 15:01:01
> 
> > > Hi Lorenzo,
> > > 
> > > From: Lorenzo Bianconi <lorenzo@kernel.org>
> > > Date: Sun, Mar 08, 2020 at 00:06:03
> > > 
> > > > Disable MIPI I3C during device reset in order to avoid
> > > > possible races on interrupt line 1. If the first interrupt
> > > > line is asserted during hw reset the device will work in
> > > > I3C-only mode
> > > > 
> > 
> > [...]
> > 
> > > > +
> > > 
> > > After disable the i3c interface the dynamic address is no more accessible 
> > > and fails the initialization.
> > > 
> > 
> > Hi Vitor,
> > 
> > thx for testing it. What do you mean here?
> > Is int1 set to vdd in your test?
> > 
> > Regards,
> > Lorenzo
> 
> Yes, according with figure 14 of lsm6dso datasheet.

uhm..probably we should do this configuration if the device is not in I3C-only
mode. Are you able to test it without setting the int1 to vdd?
Unfortunately I have no devices with an I3C controller yet.

Regards,
Lorenzo

> 
> Is there any way to clear the INT1 before the hw reset?
> 
> Best regards,
> Vitor Soares
> 
> > 
> > > Best regards,
> > > Vitor Soares
> > > 
> > > >  	/* device sw reset */
> > > >  	reg = &hw->settings->reset;
> > > >  	err = regmap_update_bits(hw->regmap, reg->addr, reg->mask,
> > > > @@ -2059,6 +2081,15 @@ static int st_lsm6dsx_init_device(struct st_lsm6dsx_hw *hw)
> > > >  
> > > >  	msleep(50);
> > > >  
> > > > +	/* enable MIPI I3C */
> > > > +	if (hw->settings->i3c_disable.addr) {
> > > > +		reg = &hw->settings->i3c_disable;
> > > > +		err = regmap_update_bits(hw->regmap, reg->addr, reg->mask,
> > > > +					 ST_LSM6DSX_SHIFT_VAL(0, reg->mask));
> > > > +		if (err < 0)
> > > > +			return err;
> > > > +	}
> > > > +
> > > >  	/* enable Block Data Update */
> > > >  	reg = &hw->settings->bdu;
> > > >  	err = regmap_update_bits(hw->regmap, reg->addr, reg->mask,
> > > > -- 
> > > > 2.24.1
> > > 
> > > 
> 
> 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

RE: [PATCH v2] iio: imu: st_lsm6dsx: disable I3C support during device reset

[Vitor Soares]

From: Lorenzo Bianconi <lorenzo@kernel.org>
Date: Mon, Mar 09, 2020 at 15:10:35

> > From: Lorenzo Bianconi <lorenzo@kernel.org>
> > Date: Mon, Mar 09, 2020 at 15:01:01
> > 
> > > > Hi Lorenzo,
> > > > 
> > > > From: Lorenzo Bianconi <lorenzo@kernel.org>
> > > > Date: Sun, Mar 08, 2020 at 00:06:03
> > > > 
> > > > > Disable MIPI I3C during device reset in order to avoid
> > > > > possible races on interrupt line 1. If the first interrupt
> > > > > line is asserted during hw reset the device will work in
> > > > > I3C-only mode
> > > > > 
> > > 
> > > [...]
> > > 
> > > > > +
> > > > 
> > > > After disable the i3c interface the dynamic address is no more accessible 
> > > > and fails the initialization.
> > > > 
> > > 
> > > Hi Vitor,
> > > 
> > > thx for testing it. What do you mean here?
> > > Is int1 set to vdd in your test?
> > > 
> > > Regards,
> > > Lorenzo
> > 
> > Yes, according with figure 14 of lsm6dso datasheet.
> 
> uhm..probably we should do this configuration if the device is not in I3C-only
> mode. Are you able to test it without setting the int1 to vdd?
> Unfortunately I have no devices with an I3C controller yet.
> 
> Regards,
> Lorenzo
> 

Yes, I can test but I suspect we will have the same issue because it lost 
the dynamic address. I would say to add a flag during the probe to 
indicate the interface and bypass this if in I3C mode.

This may be useful when address the In-band interrupts.

Best regards,
Vitor Soares

> > 
> > Is there any way to clear the INT1 before the hw reset?
> > 
> > Best regards,
> > Vitor Soares
> > 
> > > 
> > > > Best regards,
> > > > Vitor Soares
> > > > 
> > > > >  	/* device sw reset */
> > > > >  	reg = &hw->settings->reset;
> > > > >  	err = regmap_update_bits(hw->regmap, reg->addr, reg->mask,
> > > > > @@ -2059,6 +2081,15 @@ static int st_lsm6dsx_init_device(struct st_lsm6dsx_hw *hw)
> > > > >  
> > > > >  	msleep(50);
> > > > >  
> > > > > +	/* enable MIPI I3C */
> > > > > +	if (hw->settings->i3c_disable.addr) {
> > > > > +		reg = &hw->settings->i3c_disable;
> > > > > +		err = regmap_update_bits(hw->regmap, reg->addr, reg->mask,
> > > > > +					 ST_LSM6DSX_SHIFT_VAL(0, reg->mask));
> > > > > +		if (err < 0)
> > > > > +			return err;
> > > > > +	}
> > > > > +
> > > > >  	/* enable Block Data Update */
> > > > >  	reg = &hw->settings->bdu;
> > > > >  	err = regmap_update_bits(hw->regmap, reg->addr, reg->mask,
> > > > > -- 
> > > > > 2.24.1
> > > > 
> > > > 
> > 
> > 

Re: [PATCH v2] iio: imu: st_lsm6dsx: disable I3C support during device reset

[Lorenzo Bianconi]

[-- Attachment #1: Type: text/plain, Size: 3124 bytes --]

> From: Lorenzo Bianconi <lorenzo@kernel.org>
> Date: Mon, Mar 09, 2020 at 15:10:35
> 
> > > From: Lorenzo Bianconi <lorenzo@kernel.org>
> > > Date: Mon, Mar 09, 2020 at 15:01:01
> > > 
> > > > > Hi Lorenzo,
> > > > > 
> > > > > From: Lorenzo Bianconi <lorenzo@kernel.org>
> > > > > Date: Sun, Mar 08, 2020 at 00:06:03
> > > > > 
> > > > > > Disable MIPI I3C during device reset in order to avoid
> > > > > > possible races on interrupt line 1. If the first interrupt
> > > > > > line is asserted during hw reset the device will work in
> > > > > > I3C-only mode
> > > > > > 
> > > > 
> > > > [...]
> > > > 
> > > > > > +
> > > > > 
> > > > > After disable the i3c interface the dynamic address is no more accessible 
> > > > > and fails the initialization.
> > > > > 
> > > > 
> > > > Hi Vitor,
> > > > 
> > > > thx for testing it. What do you mean here?
> > > > Is int1 set to vdd in your test?
> > > > 
> > > > Regards,
> > > > Lorenzo
> > > 
> > > Yes, according with figure 14 of lsm6dso datasheet.
> > 
> > uhm..probably we should do this configuration if the device is not in I3C-only
> > mode. Are you able to test it without setting the int1 to vdd?
> > Unfortunately I have no devices with an I3C controller yet.
> > 
> > Regards,
> > Lorenzo
> > 
> 
> Yes, I can test but I suspect we will have the same issue because it lost 
> the dynamic address. I would say to add a flag during the probe to 
> indicate the interface and bypass this if in I3C mode.

I am not an i3c expert but I think the dynamic address is reset during the boot
procedure of the sensor (this is done even if you do not disable i3c).
Re-thinking about it, we should avoid it if the device if working in i3c-only
(int1 set to vdd) but I think it would be necessary in i3c-mixed (int1 set 0
gnd). Could you please test it in the latter case? Thanks.

Regards,
Lorenzo

> 
> This may be useful when address the In-band interrupts.
> 
> Best regards,
> Vitor Soares
> 
> > > 
> > > Is there any way to clear the INT1 before the hw reset?
> > > 
> > > Best regards,
> > > Vitor Soares
> > > 
> > > > 
> > > > > Best regards,
> > > > > Vitor Soares
> > > > > 
> > > > > >  	/* device sw reset */
> > > > > >  	reg = &hw->settings->reset;
> > > > > >  	err = regmap_update_bits(hw->regmap, reg->addr, reg->mask,
> > > > > > @@ -2059,6 +2081,15 @@ static int st_lsm6dsx_init_device(struct st_lsm6dsx_hw *hw)
> > > > > >  
> > > > > >  	msleep(50);
> > > > > >  
> > > > > > +	/* enable MIPI I3C */
> > > > > > +	if (hw->settings->i3c_disable.addr) {
> > > > > > +		reg = &hw->settings->i3c_disable;
> > > > > > +		err = regmap_update_bits(hw->regmap, reg->addr, reg->mask,
> > > > > > +					 ST_LSM6DSX_SHIFT_VAL(0, reg->mask));
> > > > > > +		if (err < 0)
> > > > > > +			return err;
> > > > > > +	}
> > > > > > +
> > > > > >  	/* enable Block Data Update */
> > > > > >  	reg = &hw->settings->bdu;
> > > > > >  	err = regmap_update_bits(hw->regmap, reg->addr, reg->mask,
> > > > > > -- 
> > > > > > 2.24.1
> > > > > 
> > > > > 
> > > 
> > > 
> 
> 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

RE: [PATCH v2] iio: imu: st_lsm6dsx: disable I3C support during device reset

[Vitor Soares]

From: Lorenzo Bianconi <lorenzo@kernel.org>
Date: Mon, Mar 09, 2020 at 16:07:14

> > From: Lorenzo Bianconi <lorenzo@kernel.org>
> > Date: Mon, Mar 09, 2020 at 15:10:35
> > 
> > > > From: Lorenzo Bianconi <lorenzo@kernel.org>
> > > > Date: Mon, Mar 09, 2020 at 15:01:01
> > > > 
> > > > > > Hi Lorenzo,
> > > > > > 
> > > > > > From: Lorenzo Bianconi <lorenzo@kernel.org>
> > > > > > Date: Sun, Mar 08, 2020 at 00:06:03
> > > > > > 
> > > > > > > Disable MIPI I3C during device reset in order to avoid
> > > > > > > possible races on interrupt line 1. If the first interrupt
> > > > > > > line is asserted during hw reset the device will work in
> > > > > > > I3C-only mode
> > > > > > > 
> > > > > 
> > > > > [...]
> > > > > 
> > > > > > > +
> > > > > > 
> > > > > > After disable the i3c interface the dynamic address is no more accessible 
> > > > > > and fails the initialization.
> > > > > > 
> > > > > 
> > > > > Hi Vitor,
> > > > > 
> > > > > thx for testing it. What do you mean here?
> > > > > Is int1 set to vdd in your test?
> > > > > 
> > > > > Regards,
> > > > > Lorenzo
> > > > 
> > > > Yes, according with figure 14 of lsm6dso datasheet.
> > > 
> > > uhm..probably we should do this configuration if the device is not in I3C-only
> > > mode. Are you able to test it without setting the int1 to vdd?
> > > Unfortunately I have no devices with an I3C controller yet.
> > > 
> > > Regards,
> > > Lorenzo
> > > 
> > 
> > Yes, I can test but I suspect we will have the same issue because it lost 
> > the dynamic address. I would say to add a flag during the probe to 
> > indicate the interface and bypass this if in I3C mode.
> 
> I am not an i3c expert but I think the dynamic address is reset during the boot
> procedure of the sensor (this is done even if you do not disable i3c).
> Re-thinking about it, we should avoid it if the device if working in i3c-only
> (int1 set to vdd) but I think it would be necessary in i3c-mixed (int1 set 0
> gnd). Could you please test it in the latter case? Thanks.
> 
> Regards,
> Lorenzo

Sure, I will test it and let you know. 

Best regards,
Vitor Soares

> 
> > 
> > This may be useful when address the In-band interrupts.
> > 
> > Best regards,
> > Vitor Soares
> > 
> > > > 
> > > > Is there any way to clear the INT1 before the hw reset?
> > > > 
> > > > Best regards,
> > > > Vitor Soares
> > > > 
> > > > > 
> > > > > > Best regards,
> > > > > > Vitor Soares
> > > > > > 
> > > > > > >  	/* device sw reset */
> > > > > > >  	reg = &hw->settings->reset;
> > > > > > >  	err = regmap_update_bits(hw->regmap, reg->addr, reg->mask,
> > > > > > > @@ -2059,6 +2081,15 @@ static int st_lsm6dsx_init_device(struct st_lsm6dsx_hw *hw)
> > > > > > >  
> > > > > > >  	msleep(50);
> > > > > > >  
> > > > > > > +	/* enable MIPI I3C */
> > > > > > > +	if (hw->settings->i3c_disable.addr) {
> > > > > > > +		reg = &hw->settings->i3c_disable;
> > > > > > > +		err = regmap_update_bits(hw->regmap, reg->addr, reg->mask,
> > > > > > > +					 ST_LSM6DSX_SHIFT_VAL(0, reg->mask));
> > > > > > > +		if (err < 0)
> > > > > > > +			return err;
> > > > > > > +	}
> > > > > > > +
> > > > > > >  	/* enable Block Data Update */
> > > > > > >  	reg = &hw->settings->bdu;
> > > > > > >  	err = regmap_update_bits(hw->regmap, reg->addr, reg->mask,
> > > > > > > -- 
> > > > > > > 2.24.1
> > > > > > 
> > > > > > 
> > > > 
> > > > 
> > 
> > 

RE: [PATCH v2] iio: imu: st_lsm6dsx: disable I3C support during device reset

[Vitor Soares]

From: Lorenzo Bianconi <lorenzo@kernel.org>
Date: Mon, Mar 09, 2020 at 16:07:14

> > From: Lorenzo Bianconi <lorenzo@kernel.org>
> > Date: Mon, Mar 09, 2020 at 15:10:35
> > 
> > > > From: Lorenzo Bianconi <lorenzo@kernel.org>
> > > > Date: Mon, Mar 09, 2020 at 15:01:01
> > > > 
> > > > > > Hi Lorenzo,
> > > > > > 
> > > > > > From: Lorenzo Bianconi <lorenzo@kernel.org>
> > > > > > Date: Sun, Mar 08, 2020 at 00:06:03
> > > > > > 
> > > > > > > Disable MIPI I3C during device reset in order to avoid
> > > > > > > possible races on interrupt line 1. If the first interrupt
> > > > > > > line is asserted during hw reset the device will work in
> > > > > > > I3C-only mode
> > > > > > > 
> > > > > 
> > > > > [...]
> > > > > 
> > > > > > > +
> > > > > > 
> > > > > > After disable the i3c interface the dynamic address is no more accessible 
> > > > > > and fails the initialization.
> > > > > > 
> > > > > 
> > > > > Hi Vitor,
> > > > > 
> > > > > thx for testing it. What do you mean here?
> > > > > Is int1 set to vdd in your test?
> > > > > 
> > > > > Regards,
> > > > > Lorenzo
> > > > 
> > > > Yes, according with figure 14 of lsm6dso datasheet.
> > > 
> > > uhm..probably we should do this configuration if the device is not in I3C-only
> > > mode. Are you able to test it without setting the int1 to vdd?
> > > Unfortunately I have no devices with an I3C controller yet.
> > > 
> > > Regards,
> > > Lorenzo
> > > 
> > 
> > Yes, I can test but I suspect we will have the same issue because it lost 
> > the dynamic address. I would say to add a flag during the probe to 
> > indicate the interface and bypass this if in I3C mode.
> 
> I am not an i3c expert but I think the dynamic address is reset during the boot
> procedure of the sensor (this is done even if you do not disable i3c).

It can't because the dynamic address assignment (1) and the sensor boot 
(2) are made in different steps.
  1. probe of i3c master driver;
  2. probe of sensor driver;

> Re-thinking about it, we should avoid it if the device if working in i3c-only
> (int1 set to vdd) but I think it would be necessary in i3c-mixed (int1 set 0
> gnd). Could you please test it in the latter case? Thanks.
> 
> Regards,
> Lorenzo

I test and get the same behavior. I thought it goes back to I2C mode but 
not and neither the I3C DA is addressable.

Best regards,
Vitor Soares

> 
> > 
> > This may be useful when address the In-band interrupts.
> > 
> > Best regards,
> > Vitor Soares
> > 
> > > > 
> > > > Is there any way to clear the INT1 before the hw reset?
> > > > 
> > > > Best regards,
> > > > Vitor Soares
> > > > 
> > > > > 
> > > > > > Best regards,
> > > > > > Vitor Soares
> > > > > > 
> > > > > > >  	/* device sw reset */
> > > > > > >  	reg = &hw->settings->reset;
> > > > > > >  	err = regmap_update_bits(hw->regmap, reg->addr, reg->mask,
> > > > > > > @@ -2059,6 +2081,15 @@ static int st_lsm6dsx_init_device(struct st_lsm6dsx_hw *hw)
> > > > > > >  
> > > > > > >  	msleep(50);
> > > > > > >  
> > > > > > > +	/* enable MIPI I3C */
> > > > > > > +	if (hw->settings->i3c_disable.addr) {
> > > > > > > +		reg = &hw->settings->i3c_disable;
> > > > > > > +		err = regmap_update_bits(hw->regmap, reg->addr, reg->mask,
> > > > > > > +					 ST_LSM6DSX_SHIFT_VAL(0, reg->mask));
> > > > > > > +		if (err < 0)
> > > > > > > +			return err;
> > > > > > > +	}
> > > > > > > +
> > > > > > >  	/* enable Block Data Update */
> > > > > > >  	reg = &hw->settings->bdu;
> > > > > > >  	err = regmap_update_bits(hw->regmap, reg->addr, reg->mask,
> > > > > > > -- 
> > > > > > > 2.24.1
> > > > > > 
> > > > > > 
> > > > 
> > > > 
> > 
> > 

Re: [PATCH v2] iio: imu: st_lsm6dsx: disable I3C support during device reset

[lorenzo.bianconi]

[-- Attachment #1: Type: text/plain, Size: 4155 bytes --]

> From: Lorenzo Bianconi <lorenzo@kernel.org>
> Date: Mon, Mar 09, 2020 at 16:07:14
> 
> > > From: Lorenzo Bianconi <lorenzo@kernel.org>
> > > Date: Mon, Mar 09, 2020 at 15:10:35
> > > 
> > > > > From: Lorenzo Bianconi <lorenzo@kernel.org>
> > > > > Date: Mon, Mar 09, 2020 at 15:01:01
> > > > > 
> > > > > > > Hi Lorenzo,
> > > > > > > 
> > > > > > > From: Lorenzo Bianconi <lorenzo@kernel.org>
> > > > > > > Date: Sun, Mar 08, 2020 at 00:06:03
> > > > > > > 
> > > > > > > > Disable MIPI I3C during device reset in order to avoid
> > > > > > > > possible races on interrupt line 1. If the first interrupt
> > > > > > > > line is asserted during hw reset the device will work in
> > > > > > > > I3C-only mode
> > > > > > > > 
> > > > > > 
> > > > > > [...]
> > > > > > 
> > > > > > > > +
> > > > > > > 
> > > > > > > After disable the i3c interface the dynamic address is no more accessible 
> > > > > > > and fails the initialization.
> > > > > > > 
> > > > > > 
> > > > > > Hi Vitor,
> > > > > > 
> > > > > > thx for testing it. What do you mean here?
> > > > > > Is int1 set to vdd in your test?
> > > > > > 
> > > > > > Regards,
> > > > > > Lorenzo
> > > > > 
> > > > > Yes, according with figure 14 of lsm6dso datasheet.
> > > > 
> > > > uhm..probably we should do this configuration if the device is not in I3C-only
> > > > mode. Are you able to test it without setting the int1 to vdd?
> > > > Unfortunately I have no devices with an I3C controller yet.
> > > > 
> > > > Regards,
> > > > Lorenzo
> > > > 
> > > 
> > > Yes, I can test but I suspect we will have the same issue because it lost 
> > > the dynamic address. I would say to add a flag during the probe to 
> > > indicate the interface and bypass this if in I3C mode.
> > 
> > I am not an i3c expert but I think the dynamic address is reset during the boot
> > procedure of the sensor (this is done even if you do not disable i3c).
> 
> It can't because the dynamic address assignment (1) and the sensor boot 
> (2) are made in different steps.
>   1. probe of i3c master driver;
>   2. probe of sensor driver;
> 
> > Re-thinking about it, we should avoid it if the device if working in i3c-only
> > (int1 set to vdd) but I think it would be necessary in i3c-mixed (int1 set 0
> > gnd). Could you please test it in the latter case? Thanks.
> > 
> > Regards,
> > Lorenzo
> 
> I test and get the same behavior. I thought it goes back to I2C mode but 
> not and neither the I3C DA is addressable.

Hi Vitor,

thx a lot for testing :) Maybe this would be important just when DSO/DSR are
working in i2c. We need some info from st I guess

+ st folks

Regards,
Lorenzo

> 
> Best regards,
> Vitor Soares
> 
> > 
> > > 
> > > This may be useful when address the In-band interrupts.
> > > 
> > > Best regards,
> > > Vitor Soares
> > > 
> > > > > 
> > > > > Is there any way to clear the INT1 before the hw reset?
> > > > > 
> > > > > Best regards,
> > > > > Vitor Soares
> > > > > 
> > > > > > 
> > > > > > > Best regards,
> > > > > > > Vitor Soares
> > > > > > > 
> > > > > > > >  	/* device sw reset */
> > > > > > > >  	reg = &hw->settings->reset;
> > > > > > > >  	err = regmap_update_bits(hw->regmap, reg->addr, reg->mask,
> > > > > > > > @@ -2059,6 +2081,15 @@ static int st_lsm6dsx_init_device(struct st_lsm6dsx_hw *hw)
> > > > > > > >  
> > > > > > > >  	msleep(50);
> > > > > > > >  
> > > > > > > > +	/* enable MIPI I3C */
> > > > > > > > +	if (hw->settings->i3c_disable.addr) {
> > > > > > > > +		reg = &hw->settings->i3c_disable;
> > > > > > > > +		err = regmap_update_bits(hw->regmap, reg->addr, reg->mask,
> > > > > > > > +					 ST_LSM6DSX_SHIFT_VAL(0, reg->mask));
> > > > > > > > +		if (err < 0)
> > > > > > > > +			return err;
> > > > > > > > +	}
> > > > > > > > +
> > > > > > > >  	/* enable Block Data Update */
> > > > > > > >  	reg = &hw->settings->bdu;
> > > > > > > >  	err = regmap_update_bits(hw->regmap, reg->addr, reg->mask,
> > > > > > > > -- 
> > > > > > > > 2.24.1
> > > > > > > 
> > > > > > > 
> > > > > 
> > > > > 
> > > 
> > > 
> 
> 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [PATCH v2] iio: imu: st_lsm6dsx: disable I3C support during device reset

[Lorenzo Bianconi]

[-- Attachment #1: Type: text/plain, Size: 4561 bytes --]

> > From: Lorenzo Bianconi <lorenzo@kernel.org>
> > Date: Mon, Mar 09, 2020 at 16:07:14
> > 
> > > > From: Lorenzo Bianconi <lorenzo@kernel.org>
> > > > Date: Mon, Mar 09, 2020 at 15:10:35
> > > > 
> > > > > > From: Lorenzo Bianconi <lorenzo@kernel.org>
> > > > > > Date: Mon, Mar 09, 2020 at 15:01:01
> > > > > > 
> > > > > > > > Hi Lorenzo,
> > > > > > > > 
> > > > > > > > From: Lorenzo Bianconi <lorenzo@kernel.org>
> > > > > > > > Date: Sun, Mar 08, 2020 at 00:06:03
> > > > > > > > 
> > > > > > > > > Disable MIPI I3C during device reset in order to avoid
> > > > > > > > > possible races on interrupt line 1. If the first interrupt
> > > > > > > > > line is asserted during hw reset the device will work in
> > > > > > > > > I3C-only mode
> > > > > > > > > 
> > > > > > > 
> > > > > > > [...]
> > > > > > > 
> > > > > > > > > +
> > > > > > > > 
> > > > > > > > After disable the i3c interface the dynamic address is no more accessible 
> > > > > > > > and fails the initialization.
> > > > > > > > 
> > > > > > > 
> > > > > > > Hi Vitor,
> > > > > > > 
> > > > > > > thx for testing it. What do you mean here?
> > > > > > > Is int1 set to vdd in your test?
> > > > > > > 
> > > > > > > Regards,
> > > > > > > Lorenzo
> > > > > > 
> > > > > > Yes, according with figure 14 of lsm6dso datasheet.
> > > > > 
> > > > > uhm..probably we should do this configuration if the device is not in I3C-only
> > > > > mode. Are you able to test it without setting the int1 to vdd?
> > > > > Unfortunately I have no devices with an I3C controller yet.
> > > > > 
> > > > > Regards,
> > > > > Lorenzo
> > > > > 
> > > > 
> > > > Yes, I can test but I suspect we will have the same issue because it lost 
> > > > the dynamic address. I would say to add a flag during the probe to 
> > > > indicate the interface and bypass this if in I3C mode.
> > > 
> > > I am not an i3c expert but I think the dynamic address is reset during the boot
> > > procedure of the sensor (this is done even if you do not disable i3c).
> > 
> > It can't because the dynamic address assignment (1) and the sensor boot 
> > (2) are made in different steps.
> >   1. probe of i3c master driver;
> >   2. probe of sensor driver;
> > 
> > > Re-thinking about it, we should avoid it if the device if working in i3c-only
> > > (int1 set to vdd) but I think it would be necessary in i3c-mixed (int1 set 0
> > > gnd). Could you please test it in the latter case? Thanks.
> > > 
> > > Regards,
> > > Lorenzo
> > 
> > I test and get the same behavior. I thought it goes back to I2C mode but 
> > not and neither the I3C DA is addressable.
> 
> Hi Vitor,
> 
> thx a lot for testing :) Maybe this would be important just when DSO/DSR are
> working in i2c. We need some info from st I guess
> 
> + st folks

Mario suggested me we can fix the issue even flushing the FIFO before the reset.
I will post a v3 using this approach.

Regards,
Lorenzo

> 
> Regards,
> Lorenzo
> 
> > 
> > Best regards,
> > Vitor Soares
> > 
> > > 
> > > > 
> > > > This may be useful when address the In-band interrupts.
> > > > 
> > > > Best regards,
> > > > Vitor Soares
> > > > 
> > > > > > 
> > > > > > Is there any way to clear the INT1 before the hw reset?
> > > > > > 
> > > > > > Best regards,
> > > > > > Vitor Soares
> > > > > > 
> > > > > > > 
> > > > > > > > Best regards,
> > > > > > > > Vitor Soares
> > > > > > > > 
> > > > > > > > >  	/* device sw reset */
> > > > > > > > >  	reg = &hw->settings->reset;
> > > > > > > > >  	err = regmap_update_bits(hw->regmap, reg->addr, reg->mask,
> > > > > > > > > @@ -2059,6 +2081,15 @@ static int st_lsm6dsx_init_device(struct st_lsm6dsx_hw *hw)
> > > > > > > > >  
> > > > > > > > >  	msleep(50);
> > > > > > > > >  
> > > > > > > > > +	/* enable MIPI I3C */
> > > > > > > > > +	if (hw->settings->i3c_disable.addr) {
> > > > > > > > > +		reg = &hw->settings->i3c_disable;
> > > > > > > > > +		err = regmap_update_bits(hw->regmap, reg->addr, reg->mask,
> > > > > > > > > +					 ST_LSM6DSX_SHIFT_VAL(0, reg->mask));
> > > > > > > > > +		if (err < 0)
> > > > > > > > > +			return err;
> > > > > > > > > +	}
> > > > > > > > > +
> > > > > > > > >  	/* enable Block Data Update */
> > > > > > > > >  	reg = &hw->settings->bdu;
> > > > > > > > >  	err = regmap_update_bits(hw->regmap, reg->addr, reg->mask,
> > > > > > > > > -- 
> > > > > > > > > 2.24.1
> > > > > > > > 
> > > > > > > > 
> > > > > > 
> > > > > > 
> > > > 
> > > > 
> > 
> > 



[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]