[PATCH v3 00/11] i3c: master: some fix and improvemnt for hotjoin

[PATCH v3 00/11] i3c: master: some fix and improvemnt for hotjoin

[Frank Li]

Signed-off-by: Frank Li <Frank.Li@nxp.com>
---
Changes in v3:
- Fix build warning
kernel test robot noticed the following build warnings:

url:    https://github.com/intel-lab-lkp/linux/commits/Frank-Li/i3c-master-Remove-i3c_dev_disable_ibi_locked-olddev-on-device-hotjoin/20240814-234209
base:   41c196e567fb1ea97f68a2ffb7faab451cd90854
patch link:    https://lore.kernel.org/r/20240813-i3c_fix-v2-10-68fe4a050188%40nxp.com
patch subject: [PATCH v2 10/11] i3c: master: svc: wait for Manual ACK/NACK Done before next step
config: x86_64-randconfig-161-20240817 (https://download.01.org/0day-ci/archive/20240818/202408180012.ifcIOjgX-lkp@intel.com/config)
compiler: clang version 18.1.5 (https://github.com/llvm/llvm-project 617a15a9eac96088ae5e9134248d8236e34b91b1)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Reported-by: Dan Carpenter <dan.carpenter@linaro.org>
| Closes: https://lore.kernel.org/r/202408180012.ifcIOjgX-lkp@intel.com/

- Link to v2: https://lore.kernel.org/r/20240813-i3c_fix-v2-0-68fe4a050188@nxp.com

Changes in v2:
- add help function at i3c: master: svc: manually emit NACK/ACK for hotjoin F
Add below new fix patch
i3c: master: svc: fix possible assignment of the same address to two devices
i3c: master: svc: wait for Manual ACK/NACK Done before next step
i3c: master: svc: use spinlock_saveirq at svc_i3c_master_ibi_work()
i3c: master: svc: need check IBIWON for dynamtica address assign

- Link to v1: https://lore.kernel.org/r/20240724-i3c_fix-v1-0-bfa500b023d6@nxp.com

---
Frank Li (11):
      i3c: master: Remove i3c_dev_disable_ibi_locked(olddev) on device hotjoin
      i3c: master: Replace hard code 2 with macro I3C_ADDR_SLOT_BITS
      i3c: master: Extend address status bit to 4 and add I3C_ADDR_SLOT_EXT_INIT
      i3c: master: Fix dynamic address leak when 'assigned-address' is present
      i3c: master: Fix miss free init_dyn_addr at i3c_master_put_i3c_addrs()
      i3c: master: svc: use repeat start when IBI WIN happens
      i3c: master: svc: manually emit NACK/ACK for hotjoin
      i3c: master: svc: need check IBIWON for dynamtica address assign
      i3c: master: svc: use spinlock_saveirq at svc_i3c_master_ibi_work()
      i3c: master: svc: wait for Manual ACK/NACK Done before next step
      i3c: master: svc: fix possible assignment of the same address to two devices

 drivers/i3c/master.c                |  66 ++++++++++++++---------
 drivers/i3c/master/svc-i3c-master.c | 104 ++++++++++++++++++++++++++----------
 include/linux/i3c/master.h          |   8 ++-
 3 files changed, 125 insertions(+), 53 deletions(-)
---
base-commit: 41c196e567fb1ea97f68a2ffb7faab451cd90854
change-id: 20240724-i3c_fix-371bf8fa9e00

Best regards,
---
Frank Li <Frank.Li@nxp.com>

[PATCH v3 01/11] i3c: master: Remove i3c_dev_disable_ibi_locked(olddev) on device hotjoin

[Frank Li]

When a new device hotjoins, a new dynamic address is assigned.
i3c_master_add_i3c_dev_locked() identifies that the device was previously
attached to the bus and locates the olddev.

i3c_master_add_i3c_dev_locked()
{
    ...
    olddev = i3c_master_search_i3c_dev_duplicate(newdev);
    ...
    if (olddev) {
        ...
        i3c_dev_disable_ibi_locked(olddev);
        ^^^^^^
        The olddev should not receive any commands on the i3c bus as it
        does not exist and has been assigned a new address. This will
        result in NACK or timeout. So remove it.
    }
}

Fixes: 317bacf960a4 ("i3c: master: add enable(disable) hot join in sys entry")
Signed-off-by: Frank Li <Frank.Li@nxp.com>
---
 drivers/i3c/master.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/drivers/i3c/master.c b/drivers/i3c/master.c
index 7028f03c2c42e..852b32178b722 100644
--- a/drivers/i3c/master.c
+++ b/drivers/i3c/master.c
@@ -2039,10 +2039,8 @@ int i3c_master_add_i3c_dev_locked(struct i3c_master_controller *master,
 			ibireq.max_payload_len = olddev->ibi->max_payload_len;
 			ibireq.num_slots = olddev->ibi->num_slots;
 
-			if (olddev->ibi->enabled) {
+			if (olddev->ibi->enabled)
 				enable_ibi = true;
-				i3c_dev_disable_ibi_locked(olddev);
-			}
 
 			i3c_dev_free_ibi_locked(olddev);
 		}

-- 
2.34.1

Re: [PATCH v3 01/11] i3c: master: Remove i3c_dev_disable_ibi_locked(olddev) on device hotjoin

[Stanley Chu]

On Tue, Aug 20, 2024 at 12:02 AM Frank Li <Frank.Li@nxp.com> wrote:
>
> When a new device hotjoins, a new dynamic address is assigned.
> i3c_master_add_i3c_dev_locked() identifies that the device was previously
> attached to the bus and locates the olddev.
>
> i3c_master_add_i3c_dev_locked()
> {
>     ...
>     olddev = i3c_master_search_i3c_dev_duplicate(newdev);
>     ...
>     if (olddev) {
>         ...
>         i3c_dev_disable_ibi_locked(olddev);
>         ^^^^^^
>         The olddev should not receive any commands on the i3c bus as it
>         does not exist and has been assigned a new address. This will
>         result in NACK or timeout. So remove it.
>     }
> }
>
> Fixes: 317bacf960a4 ("i3c: master: add enable(disable) hot join in sys entry")
> Signed-off-by: Frank Li <Frank.Li@nxp.com>
> ---
>  drivers/i3c/master.c | 4 +---
>  1 file changed, 1 insertion(+), 3 deletions(-)
>
> diff --git a/drivers/i3c/master.c b/drivers/i3c/master.c
> index 7028f03c2c42e..852b32178b722 100644
> --- a/drivers/i3c/master.c
> +++ b/drivers/i3c/master.c
> @@ -2039,10 +2039,8 @@ int i3c_master_add_i3c_dev_locked(struct i3c_master_controller *master,
>                         ibireq.max_payload_len = olddev->ibi->max_payload_len;
>                         ibireq.num_slots = olddev->ibi->num_slots;
>
> -                       if (olddev->ibi->enabled) {
> +                       if (olddev->ibi->enabled)
>                                 enable_ibi = true;
> -                               i3c_dev_disable_ibi_locked(olddev);
> -                       }
>
>                         i3c_dev_free_ibi_locked(olddev);

i3c_dev_free_ibi_locked will still encounter WARN_ON(dev->ibi->enabled).

Thanks,
Stanley

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

Re: [PATCH v3 01/11] i3c: master: Remove i3c_dev_disable_ibi_locked(olddev) on device hotjoin

[Frank Li]

On Tue, Aug 20, 2024 at 09:34:11AM +0800, Stanley Chu wrote:
> On Tue, Aug 20, 2024 at 12:02 AM Frank Li <Frank.Li@nxp.com> wrote:
> >
> > When a new device hotjoins, a new dynamic address is assigned.
> > i3c_master_add_i3c_dev_locked() identifies that the device was previously
> > attached to the bus and locates the olddev.
> >
> > i3c_master_add_i3c_dev_locked()
> > {
> >     ...
> >     olddev = i3c_master_search_i3c_dev_duplicate(newdev);
> >     ...
> >     if (olddev) {
> >         ...
> >         i3c_dev_disable_ibi_locked(olddev);
> >         ^^^^^^
> >         The olddev should not receive any commands on the i3c bus as it
> >         does not exist and has been assigned a new address. This will
> >         result in NACK or timeout. So remove it.
> >     }
> > }
> >
> > Fixes: 317bacf960a4 ("i3c: master: add enable(disable) hot join in sys entry")
> > Signed-off-by: Frank Li <Frank.Li@nxp.com>
> > ---
> >  drivers/i3c/master.c | 4 +---
> >  1 file changed, 1 insertion(+), 3 deletions(-)
> >
> > diff --git a/drivers/i3c/master.c b/drivers/i3c/master.c
> > index 7028f03c2c42e..852b32178b722 100644
> > --- a/drivers/i3c/master.c
> > +++ b/drivers/i3c/master.c
> > @@ -2039,10 +2039,8 @@ int i3c_master_add_i3c_dev_locked(struct i3c_master_controller *master,
> >                         ibireq.max_payload_len = olddev->ibi->max_payload_len;
> >                         ibireq.num_slots = olddev->ibi->num_slots;
> >
> > -                       if (olddev->ibi->enabled) {
> > +                       if (olddev->ibi->enabled)
> >                                 enable_ibi = true;
> > -                               i3c_dev_disable_ibi_locked(olddev);
> > -                       }
> >
> >                         i3c_dev_free_ibi_locked(olddev);
>
> i3c_dev_free_ibi_locked will still encounter WARN_ON(dev->ibi->enabled).

Thank you test it. The below patch should fix this problem.

https://lore.kernel.org/imx/20240820043818.3352614-1-ravindra.yashvant.shinde@nxp.com/T/#u

>
> Thanks,
> Stanley
>
> >                 }
> >
> > --
> > 2.34.1
> >
> >
> > --
> > linux-i3c mailing list
> > linux-i3c@lists.infradead.org
> > http://lists.infradead.org/mailman/listinfo/linux-i3c

Re: [PATCH v3 01/11] i3c: master: Remove i3c_dev_disable_ibi_locked(olddev) on device hotjoin

[Miquel Raynal]

Hi Frank,

Frank.li@nxp.com wrote on Tue, 20 Aug 2024 10:45:14 -0400:

> On Tue, Aug 20, 2024 at 09:34:11AM +0800, Stanley Chu wrote:
> > On Tue, Aug 20, 2024 at 12:02 AM Frank Li <Frank.Li@nxp.com> wrote:  
> > >
> > > When a new device hotjoins, a new dynamic address is assigned.
> > > i3c_master_add_i3c_dev_locked() identifies that the device was previously
> > > attached to the bus and locates the olddev.
> > >
> > > i3c_master_add_i3c_dev_locked()
> > > {
> > >     ...
> > >     olddev = i3c_master_search_i3c_dev_duplicate(newdev);
> > >     ...
> > >     if (olddev) {
> > >         ...
> > >         i3c_dev_disable_ibi_locked(olddev);
> > >         ^^^^^^
> > >         The olddev should not receive any commands on the i3c bus as it
> > >         does not exist and has been assigned a new address. This will
> > >         result in NACK or timeout. So remove it.
> > >     }
> > > }
> > >
> > > Fixes: 317bacf960a4 ("i3c: master: add enable(disable) hot join in sys entry")
> > > Signed-off-by: Frank Li <Frank.Li@nxp.com>
> > > ---
> > >  drivers/i3c/master.c | 4 +---
> > >  1 file changed, 1 insertion(+), 3 deletions(-)
> > >
> > > diff --git a/drivers/i3c/master.c b/drivers/i3c/master.c
> > > index 7028f03c2c42e..852b32178b722 100644
> > > --- a/drivers/i3c/master.c
> > > +++ b/drivers/i3c/master.c
> > > @@ -2039,10 +2039,8 @@ int i3c_master_add_i3c_dev_locked(struct i3c_master_controller *master,
> > >                         ibireq.max_payload_len = olddev->ibi->max_payload_len;
> > >                         ibireq.num_slots = olddev->ibi->num_slots;
> > >
> > > -                       if (olddev->ibi->enabled) {
> > > +                       if (olddev->ibi->enabled)
> > >                                 enable_ibi = true;
> > > -                               i3c_dev_disable_ibi_locked(olddev);
> > > -                       }
> > >
> > >                         i3c_dev_free_ibi_locked(olddev);  
> >
> > i3c_dev_free_ibi_locked will still encounter WARN_ON(dev->ibi->enabled).  
> 
> Thank you test it. The below patch should fix this problem.
> 
> https://lore.kernel.org/imx/20240820043818.3352614-1-ravindra.yashvant.shinde@nxp.com/T/#u

It should indeed. With this change added:

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

Thanks,
Miquèl

[PATCH v3 02/11] i3c: master: Replace hard code 2 with macro I3C_ADDR_SLOT_BITS

[Frank Li]

Replace the hardcoded value 2, which indicates 2 bits for I3C address
status, with the predefined macro I3C_ADDR_SLOT_BITS.

Improve maintainability and extensibility of the code.

Signed-off-by: Frank Li <Frank.Li@nxp.com>
---
 drivers/i3c/master.c       | 4 ++--
 include/linux/i3c/master.h | 4 +++-
 2 files changed, 5 insertions(+), 3 deletions(-)

diff --git a/drivers/i3c/master.c b/drivers/i3c/master.c
index 852b32178b722..85c737554c940 100644
--- a/drivers/i3c/master.c
+++ b/drivers/i3c/master.c
@@ -348,7 +348,7 @@ static enum i3c_addr_slot_status
 i3c_bus_get_addr_slot_status(struct i3c_bus *bus, u16 addr)
 {
 	unsigned long status;
-	int bitpos = addr * 2;
+	int bitpos = addr * I3C_ADDR_SLOT_BITS;
 
 	if (addr > I2C_MAX_ADDR)
 		return I3C_ADDR_SLOT_RSVD;
@@ -362,7 +362,7 @@ i3c_bus_get_addr_slot_status(struct i3c_bus *bus, u16 addr)
 static void i3c_bus_set_addr_slot_status(struct i3c_bus *bus, u16 addr,
 					 enum i3c_addr_slot_status status)
 {
-	int bitpos = addr * 2;
+	int bitpos = addr * I3C_ADDR_SLOT_BITS;
 	unsigned long *ptr;
 
 	if (addr > I2C_MAX_ADDR)
diff --git a/include/linux/i3c/master.h b/include/linux/i3c/master.h
index 074f632868d98..4601b6957f799 100644
--- a/include/linux/i3c/master.h
+++ b/include/linux/i3c/master.h
@@ -299,6 +299,8 @@ enum i3c_addr_slot_status {
 	I3C_ADDR_SLOT_STATUS_MASK = 3,
 };
 
+#define I3C_ADDR_SLOT_BITS 2
+
 /**
  * struct i3c_bus - I3C bus object
  * @cur_master: I3C master currently driving the bus. Since I3C is multi-master
@@ -340,7 +342,7 @@ enum i3c_addr_slot_status {
 struct i3c_bus {
 	struct i3c_dev_desc *cur_master;
 	int id;
-	unsigned long addrslots[((I2C_MAX_ADDR + 1) * 2) / BITS_PER_LONG];
+	unsigned long addrslots[((I2C_MAX_ADDR + 1) * I3C_ADDR_SLOT_BITS) / BITS_PER_LONG];
 	enum i3c_bus_mode mode;
 	struct {
 		unsigned long i3c;

-- 
2.34.1

Re: [PATCH v3 02/11] i3c: master: Replace hard code 2 with macro I3C_ADDR_SLOT_BITS

[Miquel Raynal]

Hi Frank,

Frank.Li@nxp.com wrote on Mon, 19 Aug 2024 12:01:56 -0400:

> Replace the hardcoded value 2, which indicates 2 bits for I3C address
> status, with the predefined macro I3C_ADDR_SLOT_BITS.

I'm fine with the idea but I don't understand the macro name. You're
talking about status bits and yet the macro is named addr_slot?

> Improve maintainability and extensibility of the code.

Thanks,
Miquèl

Re: [PATCH v3 02/11] i3c: master: Replace hard code 2 with macro I3C_ADDR_SLOT_BITS

[Frank Li]

On Fri, Aug 23, 2024 at 05:55:02PM +0200, Miquel Raynal wrote:
> Hi Frank,
>
> Frank.Li@nxp.com wrote on Mon, 19 Aug 2024 12:01:56 -0400:
>
> > Replace the hardcoded value 2, which indicates 2 bits for I3C address
> > status, with the predefined macro I3C_ADDR_SLOT_BITS.
>
> I'm fine with the idea but I don't understand the macro name. You're
> talking about status bits and yet the macro is named addr_slot?
>

How about I3C_ADDR_SLOT_STATUS_BITS ?

> > Improve maintainability and extensibility of the code.
>
> Thanks,
> Miquèl

Re: [PATCH v3 02/11] i3c: master: Replace hard code 2 with macro I3C_ADDR_SLOT_BITS

[Miquel Raynal]

Hi Frank,

Frank.li@nxp.com wrote on Fri, 23 Aug 2024 13:57:31 -0400:

> On Fri, Aug 23, 2024 at 05:55:02PM +0200, Miquel Raynal wrote:
> > Hi Frank,
> >
> > Frank.Li@nxp.com wrote on Mon, 19 Aug 2024 12:01:56 -0400:
> >  
> > > Replace the hardcoded value 2, which indicates 2 bits for I3C address
> > > status, with the predefined macro I3C_ADDR_SLOT_BITS.  
> >
> > I'm fine with the idea but I don't understand the macro name. You're
> > talking about status bits and yet the macro is named addr_slot?
> >  
> 
> How about I3C_ADDR_SLOT_STATUS_BITS ?
> 
> > > Improve maintainability and extensibility of the code.  

I'm a bit flaky on this concept, but let's try that. Perhaps with all
the changes requested (esp. rephrasing) it might become clearer in next
version.

Thanks,
Miquèl

[PATCH v3 03/11] i3c: master: Extend address status bit to 4 and add I3C_ADDR_SLOT_EXT_INIT

[Frank Li]

Extend the address status bit to 4 and introduce the I3C_ADDR_SLOT_EXT_INIT
macro to indicate that a device prefers a specific address. This is
generally set by the 'assigned-address' in the device tree source (dts)
file.

When an i3c device is removed from the bus, the old address status is set
to FREE, allowing other devices to use the address during hotjoin. The
I3C_ADDR_SLOT_EXT_INIT status indicates that an address is preferred by
some devices. The function i3c_bus_get_free_addr() will first attempt to
use unassigned addresses before searching for assigned addresses of devices
that have been removed from the bus, trying to best match the
'assigned-address'.

Signed-off-by: Frank Li <Frank.Li@nxp.com>
---
 drivers/i3c/master.c       | 43 ++++++++++++++++++++++++++++++++++---------
 include/linux/i3c/master.h |  6 +++++-
 2 files changed, 39 insertions(+), 10 deletions(-)

diff --git a/drivers/i3c/master.c b/drivers/i3c/master.c
index 85c737554c940..4281f673e08d8 100644
--- a/drivers/i3c/master.c
+++ b/drivers/i3c/master.c
@@ -345,7 +345,7 @@ const struct bus_type i3c_bus_type = {
 EXPORT_SYMBOL_GPL(i3c_bus_type);
 
 static enum i3c_addr_slot_status
-i3c_bus_get_addr_slot_status(struct i3c_bus *bus, u16 addr)
+i3c_bus_get_addr_slot_status_ext(struct i3c_bus *bus, u16 addr)
 {
 	unsigned long status;
 	int bitpos = addr * I3C_ADDR_SLOT_BITS;
@@ -356,11 +356,17 @@ i3c_bus_get_addr_slot_status(struct i3c_bus *bus, u16 addr)
 	status = bus->addrslots[bitpos / BITS_PER_LONG];
 	status >>= bitpos % BITS_PER_LONG;
 
-	return status & I3C_ADDR_SLOT_STATUS_MASK;
+	return status & I3C_ADDR_SLOT_EXT_STATUS_MASK;
 }
 
-static void i3c_bus_set_addr_slot_status(struct i3c_bus *bus, u16 addr,
-					 enum i3c_addr_slot_status status)
+static enum i3c_addr_slot_status
+i3c_bus_get_addr_slot_status(struct i3c_bus *bus, u16 addr)
+{
+	return i3c_bus_get_addr_slot_status_ext(bus, addr) & I3C_ADDR_SLOT_STATUS_MASK;
+}
+
+static void i3c_bus_set_addr_slot_status_mask(struct i3c_bus *bus, u16 addr,
+					      enum i3c_addr_slot_status status, int mask)
 {
 	int bitpos = addr * I3C_ADDR_SLOT_BITS;
 	unsigned long *ptr;
@@ -369,11 +375,22 @@ static void i3c_bus_set_addr_slot_status(struct i3c_bus *bus, u16 addr,
 		return;
 
 	ptr = bus->addrslots + (bitpos / BITS_PER_LONG);
-	*ptr &= ~((unsigned long)I3C_ADDR_SLOT_STATUS_MASK <<
-						(bitpos % BITS_PER_LONG));
+	*ptr &= ~((unsigned long)mask << (bitpos % BITS_PER_LONG));
 	*ptr |= (unsigned long)status << (bitpos % BITS_PER_LONG);
 }
 
+static void i3c_bus_set_addr_slot_status(struct i3c_bus *bus, u16 addr,
+					 enum i3c_addr_slot_status status)
+{
+	i3c_bus_set_addr_slot_status_mask(bus, addr, status, I3C_ADDR_SLOT_STATUS_MASK);
+}
+
+static void i3c_bus_set_addr_slot_status_ext(struct i3c_bus *bus, u16 addr,
+					     enum i3c_addr_slot_status status)
+{
+	i3c_bus_set_addr_slot_status_mask(bus, addr, status, I3C_ADDR_SLOT_EXT_STATUS_MASK);
+}
+
 static bool i3c_bus_dev_addr_is_avail(struct i3c_bus *bus, u8 addr)
 {
 	enum i3c_addr_slot_status status;
@@ -388,6 +405,14 @@ static int i3c_bus_get_free_addr(struct i3c_bus *bus, u8 start_addr)
 	enum i3c_addr_slot_status status;
 	u8 addr;
 
+	/* try find an address, which have not pre-allocated by assigned-address */
+	for (addr = start_addr; addr < I3C_MAX_ADDR; addr++) {
+		status = i3c_bus_get_addr_slot_status_ext(bus, addr);
+		if (status == I3C_ADDR_SLOT_FREE)
+			return addr;
+	}
+
+	/* use pre-allocoated by assigned-address because such device was removed at bus*/
 	for (addr = start_addr; addr < I3C_MAX_ADDR; addr++) {
 		status = i3c_bus_get_addr_slot_status(bus, addr);
 		if (status == I3C_ADDR_SLOT_FREE)
@@ -1906,9 +1931,9 @@ static int i3c_master_bus_init(struct i3c_master_controller *master)
 			goto err_rstdaa;
 		}
 
-		i3c_bus_set_addr_slot_status(&master->bus,
-					     i3cboardinfo->init_dyn_addr,
-					     I3C_ADDR_SLOT_I3C_DEV);
+		i3c_bus_set_addr_slot_status_ext(&master->bus,
+						 i3cboardinfo->init_dyn_addr,
+						 I3C_ADDR_SLOT_I3C_DEV | I3C_ADDR_SLOT_EXT_INIT);
 
 		/*
 		 * Only try to create/attach devices that have a static
diff --git a/include/linux/i3c/master.h b/include/linux/i3c/master.h
index 4601b6957f799..c923b76bbc321 100644
--- a/include/linux/i3c/master.h
+++ b/include/linux/i3c/master.h
@@ -284,6 +284,8 @@ enum i3c_bus_mode {
  * @I3C_ADDR_SLOT_I2C_DEV: address is assigned to an I2C device
  * @I3C_ADDR_SLOT_I3C_DEV: address is assigned to an I3C device
  * @I3C_ADDR_SLOT_STATUS_MASK: address slot mask
+ * @I3C_ADDR_SLOT_EXT_INIT: the bit mask display of addresses is preferred by some devices,
+ *			    such as the "assigned-address" in device tree source (dts).
  *
  * On an I3C bus, addresses are assigned dynamically, and we need to know which
  * addresses are free to use and which ones are already assigned.
@@ -297,9 +299,11 @@ enum i3c_addr_slot_status {
 	I3C_ADDR_SLOT_I2C_DEV,
 	I3C_ADDR_SLOT_I3C_DEV,
 	I3C_ADDR_SLOT_STATUS_MASK = 3,
+	I3C_ADDR_SLOT_EXT_STATUS_MASK = 7,
+	I3C_ADDR_SLOT_EXT_INIT = BIT(2),
 };
 
-#define I3C_ADDR_SLOT_BITS 2
+#define I3C_ADDR_SLOT_BITS 4
 
 /**
  * struct i3c_bus - I3C bus object

-- 
2.34.1

Re: [PATCH v3 03/11] i3c: master: Extend address status bit to 4 and add I3C_ADDR_SLOT_EXT_INIT

[Miquel Raynal]

Hi Frank,


>  static bool i3c_bus_dev_addr_is_avail(struct i3c_bus *bus, u8 addr)
>  {
>  	enum i3c_addr_slot_status status;
> @@ -388,6 +405,14 @@ static int i3c_bus_get_free_addr(struct i3c_bus *bus, u8 start_addr)
>  	enum i3c_addr_slot_status status;
>  	u8 addr;
>  
> +	/* try find an address, which have not pre-allocated by assigned-address */

	Try	to find			has   been

pre-allocated?

> +	for (addr = start_addr; addr < I3C_MAX_ADDR; addr++) {
> +		status = i3c_bus_get_addr_slot_status_ext(bus, addr);
> +		if (status == I3C_ADDR_SLOT_FREE)
> +			return addr;
> +	}
> +
> +	/* use pre-allocoated by assigned-address because such device was removed at bus*/

	  Use      allocated 

pre-allocated or assigned?

I guess the logic should be:
- try the assigned-address
- look for a free slot
- look for an already in use slot that must concern a disconnected
  device

But the comments are not precise enough IMHO. Can you rephrase them?

>  	for (addr = start_addr; addr < I3C_MAX_ADDR; addr++) {
>  		status = i3c_bus_get_addr_slot_status(bus, addr);
>  		if (status == I3C_ADDR_SLOT_FREE)
> @@ -1906,9 +1931,9 @@ static int i3c_master_bus_init(struct i3c_master_controller *master)
>  			goto err_rstdaa;
>  		}
>  
> -		i3c_bus_set_addr_slot_status(&master->bus,
> -					     i3cboardinfo->init_dyn_addr,
> -					     I3C_ADDR_SLOT_I3C_DEV);
> +		i3c_bus_set_addr_slot_status_ext(&master->bus,
> +						 i3cboardinfo->init_dyn_addr,
> +						 I3C_ADDR_SLOT_I3C_DEV | I3C_ADDR_SLOT_EXT_INIT);
>  
>  		/*
>  		 * Only try to create/attach devices that have a static
> diff --git a/include/linux/i3c/master.h b/include/linux/i3c/master.h
> index 4601b6957f799..c923b76bbc321 100644
> --- a/include/linux/i3c/master.h
> +++ b/include/linux/i3c/master.h
> @@ -284,6 +284,8 @@ enum i3c_bus_mode {
>   * @I3C_ADDR_SLOT_I2C_DEV: address is assigned to an I2C device
>   * @I3C_ADDR_SLOT_I3C_DEV: address is assigned to an I3C device
>   * @I3C_ADDR_SLOT_STATUS_MASK: address slot mask
> + * @I3C_ADDR_SLOT_EXT_INIT: the bit mask display of addresses is preferred by some devices,

I'm sorry, but I don't understand what "bit mask display of addresses"
means.

> + *			    such as the "assigned-address" in device tree source (dts).
>   *
>   * On an I3C bus, addresses are assigned dynamically, and we need to know which
>   * addresses are free to use and which ones are already assigned.
> @@ -297,9 +299,11 @@ enum i3c_addr_slot_status {
>  	I3C_ADDR_SLOT_I2C_DEV,
>  	I3C_ADDR_SLOT_I3C_DEV,
>  	I3C_ADDR_SLOT_STATUS_MASK = 3,
> +	I3C_ADDR_SLOT_EXT_STATUS_MASK = 7,
> +	I3C_ADDR_SLOT_EXT_INIT = BIT(2),
>  };
>  
> -#define I3C_ADDR_SLOT_BITS 2
> +#define I3C_ADDR_SLOT_BITS 4
>  
>  /**
>   * struct i3c_bus - I3C bus object
> 


Thanks,
Miquèl

Re: [PATCH v3 03/11] i3c: master: Extend address status bit to 4 and add I3C_ADDR_SLOT_EXT_INIT

[Frank Li]

On Fri, Aug 23, 2024 at 06:04:26PM +0200, Miquel Raynal wrote:
> Hi Frank,
>
>
> >  static bool i3c_bus_dev_addr_is_avail(struct i3c_bus *bus, u8 addr)
> >  {
> >  	enum i3c_addr_slot_status status;
> > @@ -388,6 +405,14 @@ static int i3c_bus_get_free_addr(struct i3c_bus *bus, u8 start_addr)
> >  	enum i3c_addr_slot_status status;
> >  	u8 addr;
> >
> > +	/* try find an address, which have not pre-allocated by assigned-address */
>
> 	Try	to find			has   been
>
> pre-allocated?
>
> > +	for (addr = start_addr; addr < I3C_MAX_ADDR; addr++) {
> > +		status = i3c_bus_get_addr_slot_status_ext(bus, addr);
> > +		if (status == I3C_ADDR_SLOT_FREE)
> > +			return addr;
> > +	}
> > +
> > +	/* use pre-allocoated by assigned-address because such device was removed at bus*/
>
> 	  Use      allocated
>
> pre-allocated or assigned?
>
> I guess the logic should be:
> - try the assigned-address
> - look for a free slot
> - look for an already in use slot that must concern a disconnected
>   device
>
> But the comments are not precise enough IMHO. Can you rephrase them?

How about:

Follow the steps below to obtain the I3C dynamic address:

1. Retrieve the assigned-address from the device tree (DT).
2. Look for an available slot address.
3. Look for an address that is pre-reserved by another device with
assigned-address in DT, but where the device is currently offline.

>
> >  	for (addr = start_addr; addr < I3C_MAX_ADDR; addr++) {
> >  		status = i3c_bus_get_addr_slot_status(bus, addr);
> >  		if (status == I3C_ADDR_SLOT_FREE)
> > @@ -1906,9 +1931,9 @@ static int i3c_master_bus_init(struct i3c_master_controller *master)
> >  			goto err_rstdaa;
> >  		}
> >
> > -		i3c_bus_set_addr_slot_status(&master->bus,
> > -					     i3cboardinfo->init_dyn_addr,
> > -					     I3C_ADDR_SLOT_I3C_DEV);
> > +		i3c_bus_set_addr_slot_status_ext(&master->bus,
> > +						 i3cboardinfo->init_dyn_addr,
> > +						 I3C_ADDR_SLOT_I3C_DEV | I3C_ADDR_SLOT_EXT_INIT);
> >
> >  		/*
> >  		 * Only try to create/attach devices that have a static
> > diff --git a/include/linux/i3c/master.h b/include/linux/i3c/master.h
> > index 4601b6957f799..c923b76bbc321 100644
> > --- a/include/linux/i3c/master.h
> > +++ b/include/linux/i3c/master.h
> > @@ -284,6 +284,8 @@ enum i3c_bus_mode {
> >   * @I3C_ADDR_SLOT_I2C_DEV: address is assigned to an I2C device
> >   * @I3C_ADDR_SLOT_I3C_DEV: address is assigned to an I3C device
> >   * @I3C_ADDR_SLOT_STATUS_MASK: address slot mask
> > + * @I3C_ADDR_SLOT_EXT_INIT: the bit mask display of addresses is preferred by some devices,
>
> I'm sorry, but I don't understand what "bit mask display of addresses"
> means.
>
> > + *			    such as the "assigned-address" in device tree source (dts).
> >   *
> >   * On an I3C bus, addresses are assigned dynamically, and we need to know which
> >   * addresses are free to use and which ones are already assigned.
> > @@ -297,9 +299,11 @@ enum i3c_addr_slot_status {
> >  	I3C_ADDR_SLOT_I2C_DEV,
> >  	I3C_ADDR_SLOT_I3C_DEV,
> >  	I3C_ADDR_SLOT_STATUS_MASK = 3,
> > +	I3C_ADDR_SLOT_EXT_STATUS_MASK = 7,
> > +	I3C_ADDR_SLOT_EXT_INIT = BIT(2),
> >  };
> >
> > -#define I3C_ADDR_SLOT_BITS 2
> > +#define I3C_ADDR_SLOT_BITS 4
> >
> >  /**
> >   * struct i3c_bus - I3C bus object
> >
>
>
> Thanks,
> Miquèl

Re: [PATCH v3 03/11] i3c: master: Extend address status bit to 4 and add I3C_ADDR_SLOT_EXT_INIT

[Miquel Raynal]

Hi Frank,

Frank.li@nxp.com wrote on Fri, 23 Aug 2024 13:55:15 -0400:

> On Fri, Aug 23, 2024 at 06:04:26PM +0200, Miquel Raynal wrote:
> > Hi Frank,
> >
> >  
> > >  static bool i3c_bus_dev_addr_is_avail(struct i3c_bus *bus, u8 addr)
> > >  {
> > >  	enum i3c_addr_slot_status status;
> > > @@ -388,6 +405,14 @@ static int i3c_bus_get_free_addr(struct i3c_bus *bus, u8 start_addr)
> > >  	enum i3c_addr_slot_status status;
> > >  	u8 addr;
> > >
> > > +	/* try find an address, which have not pre-allocated by assigned-address */  
> >
> > 	Try	to find			has   been
> >
> > pre-allocated?
> >  
> > > +	for (addr = start_addr; addr < I3C_MAX_ADDR; addr++) {
> > > +		status = i3c_bus_get_addr_slot_status_ext(bus, addr);
> > > +		if (status == I3C_ADDR_SLOT_FREE)
> > > +			return addr;
> > > +	}
> > > +
> > > +	/* use pre-allocoated by assigned-address because such device was removed at bus*/  
> >
> > 	  Use      allocated
> >
> > pre-allocated or assigned?
> >
> > I guess the logic should be:
> > - try the assigned-address
> > - look for a free slot
> > - look for an already in use slot that must concern a disconnected
> >   device
> >
> > But the comments are not precise enough IMHO. Can you rephrase them?  
> 
> How about:
> 
> Follow the steps below to obtain the I3C dynamic address:
> 
> 1. Retrieve the assigned-address from the device tree (DT).

I guess here you mean that you try to pick that address, unless if
already in use on the bus, right?

> 2. Look for an available slot address.

"available address slot"?

> 3. Look for an address that is pre-reserved by another device with
> assigned-address in DT, but where the device is currently offline.

I don't think this part is precise enough. You don't look for addresses
"pre-reserved" but rather more for busy address slots, which might no
longer be in-use because the device is currently offline. The fact that
the slot might have been pre-reserved in the DT is a detail that may,
in some cases, not be true. And as far as I understand your changes,
you're not checking the DT addresses but rather more the addresses that
have been allocated live (which is anyway better, because i3c might
very well be used on a !OF platform).

Once we settle on a description, maybe this can be part of the kdoc for
the main function searching for the best dynamic address?

Thanks,
Miquèl

Re: [PATCH v3 03/11] i3c: master: Extend address status bit to 4 and add I3C_ADDR_SLOT_EXT_INIT

[Frank Li]

On Mon, Aug 26, 2024 at 10:04:30AM +0200, Miquel Raynal wrote:
> Hi Frank,
>
> Frank.li@nxp.com wrote on Fri, 23 Aug 2024 13:55:15 -0400:
>
> > On Fri, Aug 23, 2024 at 06:04:26PM +0200, Miquel Raynal wrote:
> > > Hi Frank,
> > >
> > >
> > > >  static bool i3c_bus_dev_addr_is_avail(struct i3c_bus *bus, u8 addr)
> > > >  {
> > > >  	enum i3c_addr_slot_status status;
> > > > @@ -388,6 +405,14 @@ static int i3c_bus_get_free_addr(struct i3c_bus *bus, u8 start_addr)
> > > >  	enum i3c_addr_slot_status status;
> > > >  	u8 addr;
> > > >
> > > > +	/* try find an address, which have not pre-allocated by assigned-address */
> > >
> > > 	Try	to find			has   been
> > >
> > > pre-allocated?
> > >
> > > > +	for (addr = start_addr; addr < I3C_MAX_ADDR; addr++) {
> > > > +		status = i3c_bus_get_addr_slot_status_ext(bus, addr);
> > > > +		if (status == I3C_ADDR_SLOT_FREE)
> > > > +			return addr;
> > > > +	}
> > > > +
> > > > +	/* use pre-allocoated by assigned-address because such device was removed at bus*/
> > >
> > > 	  Use      allocated
> > >
> > > pre-allocated or assigned?
> > >
> > > I guess the logic should be:
> > > - try the assigned-address
> > > - look for a free slot
> > > - look for an already in use slot that must concern a disconnected
> > >   device
> > >
> > > But the comments are not precise enough IMHO. Can you rephrase them?
> >
> > How about:
> >
> > Follow the steps below to obtain the I3C dynamic address:
> >
> > 1. Retrieve the assigned-address from the device tree (DT).
>
> I guess here you mean that you try to pick that address, unless if
> already in use on the bus, right?
>

Sorry, It should be typo. See below


> > 2. Look for an available slot address.
>
> "available address slot"?
>
> > 3. Look for an address that is pre-reserved by another device with
> > assigned-address in DT, but where the device is currently offline.
>
> I don't think this part is precise enough. You don't look for addresses
> "pre-reserved" but rather more for busy address slots, which might no
> longer be in-use because the device is currently offline. The fact that
> the slot might have been pre-reserved in the DT is a detail that may,
> in some cases, not be true. And as far as I understand your changes,
> you're not checking the DT addresses but rather more the addresses that
> have been allocated live (which is anyway better, because i3c might
> very well be used on a !OF platform).
>
> Once we settle on a description, maybe this can be part of the kdoc for
> the main function searching for the best dynamic address?

I am not sure I understand what's your means here. The current framework
is

1. Get a free address first, the get more infromation from devices, like
BCR, DCR ...
2. Check if it is old device, or dt node have assigned-address property.
3. if it is old device, switch to use old address (if old address is free)
according to i3c spec. If dt pre-reserved address is free, switch to use
dt pre-reserved address.

To match 3's requirement as much as possible, when 1 return address, which
should avoid return dt's assigned-address.


/*
 * I3C Framework Address Assignment Flow:
 * 1 Initial Address Assignment: Attempt to obtain a free address first,
then gather additional information such as PID, BCR, and DRCR
 * 2 Address switch:
       - 2a: Check if this target device is appeared before. Switch
to use prevous address of this device.
 *     - 2b: Check if this target device have assigned-address property in dt,
switch to this address if it is free.
 *
In step 1, i3c_bus_get_free_addr() is called. To optimize for step 2b, this
function should return an address that is not pre-reserved by any target
device with an assigned address in the device tree (DT). If no such address
is available, it can return an address that was pre-reserved by a target
device that is currently offline.

 */

>
> Thanks,
> Miquèl

Re: [PATCH v3 03/11] i3c: master: Extend address status bit to 4 and add I3C_ADDR_SLOT_EXT_INIT

[Miquel Raynal]

Hi Frank,

Frank.li@nxp.com wrote on Mon, 26 Aug 2024 11:56:57 -0400:

> On Mon, Aug 26, 2024 at 10:04:30AM +0200, Miquel Raynal wrote:
> > Hi Frank,
> >
> > Frank.li@nxp.com wrote on Fri, 23 Aug 2024 13:55:15 -0400:
> >  
> > > On Fri, Aug 23, 2024 at 06:04:26PM +0200, Miquel Raynal wrote:  
> > > > Hi Frank,
> > > >
> > > >  
> > > > >  static bool i3c_bus_dev_addr_is_avail(struct i3c_bus *bus, u8 addr)
> > > > >  {
> > > > >  	enum i3c_addr_slot_status status;
> > > > > @@ -388,6 +405,14 @@ static int i3c_bus_get_free_addr(struct i3c_bus *bus, u8 start_addr)
> > > > >  	enum i3c_addr_slot_status status;
> > > > >  	u8 addr;
> > > > >
> > > > > +	/* try find an address, which have not pre-allocated by assigned-address */  
> > > >
> > > > 	Try	to find			has   been
> > > >
> > > > pre-allocated?
> > > >  
> > > > > +	for (addr = start_addr; addr < I3C_MAX_ADDR; addr++) {
> > > > > +		status = i3c_bus_get_addr_slot_status_ext(bus, addr);
> > > > > +		if (status == I3C_ADDR_SLOT_FREE)
> > > > > +			return addr;
> > > > > +	}
> > > > > +
> > > > > +	/* use pre-allocoated by assigned-address because such device was removed at bus*/  
> > > >
> > > > 	  Use      allocated
> > > >
> > > > pre-allocated or assigned?
> > > >
> > > > I guess the logic should be:
> > > > - try the assigned-address
> > > > - look for a free slot
> > > > - look for an already in use slot that must concern a disconnected
> > > >   device
> > > >
> > > > But the comments are not precise enough IMHO. Can you rephrase them?  
> > >
> > > How about:
> > >
> > > Follow the steps below to obtain the I3C dynamic address:
> > >
> > > 1. Retrieve the assigned-address from the device tree (DT).  
> >
> > I guess here you mean that you try to pick that address, unless if
> > already in use on the bus, right?
> >  
> 
> Sorry, It should be typo. See below
> 
> 
> > > 2. Look for an available slot address.  
> >
> > "available address slot"?
> >  
> > > 3. Look for an address that is pre-reserved by another device with
> > > assigned-address in DT, but where the device is currently offline.  
> >
> > I don't think this part is precise enough. You don't look for addresses
> > "pre-reserved" but rather more for busy address slots, which might no
> > longer be in-use because the device is currently offline. The fact that
> > the slot might have been pre-reserved in the DT is a detail that may,
> > in some cases, not be true. And as far as I understand your changes,
> > you're not checking the DT addresses but rather more the addresses that
> > have been allocated live (which is anyway better, because i3c might
> > very well be used on a !OF platform).
> >
> > Once we settle on a description, maybe this can be part of the kdoc for
> > the main function searching for the best dynamic address?  
> 
> I am not sure I understand what's your means here. The current framework
> is
> 
> 1. Get a free address first, the get more infromation from devices, like
> BCR, DCR ...
> 2. Check if it is old device, or dt node have assigned-address property.
> 3. if it is old device, switch to use old address (if old address is free)
> according to i3c spec. If dt pre-reserved address is free, switch to use
> dt pre-reserved address.
> 
> To match 3's requirement as much as possible, when 1 return address, which
> should avoid return dt's assigned-address.
> 
> 
> /*
>  * I3C Framework Address Assignment Flow:

	 f	   a	   a	      f

>  * 1 Initial Address Assignment: Attempt to obtain a free address first,

     1.	       a       a

> then gather additional information such as PID, BCR, and DRCR
>  * 2 Address switch:

     2.

>        - 2a: Check if this target device is appeared before. Switch
> to use prevous address of this device.

	 the previous	for

>  *     - 2b: Check if this target device have assigned-address property in dt,

				    has a preferred address based on
				    firmware data (DT). Switch to it if
				    it is the case and the address is
				    free.

Today it's the DT, maybe not tomorrow. You take these values from the
firmware on the board, that feels more generic than talking about a DT
property name.

> switch to this address if it is free.
>  *
> In step 1, i3c_bus_get_free_addr() is called. To optimize for step 2b, this
> function should return an address that is not pre-reserved by any target
> device with an assigned address in the device tree (DT).

This does not make sense, if you want to optimize for 2b, why not
selecting the assigned-address property in the first place if it's
available? Also, I don't understand why you would care to specifically
*not* return an address that might be the default one for another
device in the first place. Changing to a free slot (if possible) not
reserved by another device might be done in 2b, which makes operation 1
much more simple, so you put all the complexity at the same time. Even
if you are proceeding with two devices asking the DAA at the same time,
the procedure should work in a way which does not impact the fact that
the second device will get its desired address if the first can take
another one.

> If no such address
> is available, it can return an address that was pre-reserved by a target
> device that is currently offline.

Thanks,
Miquèl

Re: [PATCH v3 03/11] i3c: master: Extend address status bit to 4 and add I3C_ADDR_SLOT_EXT_INIT

[Frank Li]

On Mon, Aug 26, 2024 at 06:49:24PM +0200, Miquel Raynal wrote:
> Hi Frank,
>
> Frank.li@nxp.com wrote on Mon, 26 Aug 2024 11:56:57 -0400:
>
> > On Mon, Aug 26, 2024 at 10:04:30AM +0200, Miquel Raynal wrote:
> > > Hi Frank,
> > >
> > > Frank.li@nxp.com wrote on Fri, 23 Aug 2024 13:55:15 -0400:
> > >
> > > > On Fri, Aug 23, 2024 at 06:04:26PM +0200, Miquel Raynal wrote:
> > > > > Hi Frank,
> > > > >
> > > > >
> > > > > >  static bool i3c_bus_dev_addr_is_avail(struct i3c_bus *bus, u8 addr)
> > > > > >  {
> > > > > >  	enum i3c_addr_slot_status status;
> > > > > > @@ -388,6 +405,14 @@ static int i3c_bus_get_free_addr(struct i3c_bus *bus, u8 start_addr)
> > > > > >  	enum i3c_addr_slot_status status;
> > > > > >  	u8 addr;
> > > > > >
> > > > > > +	/* try find an address, which have not pre-allocated by assigned-address */
> > > > >
> > > > > 	Try	to find			has   been
> > > > >
> > > > > pre-allocated?
> > > > >
> > > > > > +	for (addr = start_addr; addr < I3C_MAX_ADDR; addr++) {
> > > > > > +		status = i3c_bus_get_addr_slot_status_ext(bus, addr);
> > > > > > +		if (status == I3C_ADDR_SLOT_FREE)
> > > > > > +			return addr;
> > > > > > +	}
> > > > > > +
> > > > > > +	/* use pre-allocoated by assigned-address because such device was removed at bus*/
> > > > >
> > > > > 	  Use      allocated
> > > > >
> > > > > pre-allocated or assigned?
> > > > >
> > > > > I guess the logic should be:
> > > > > - try the assigned-address
> > > > > - look for a free slot
> > > > > - look for an already in use slot that must concern a disconnected
> > > > >   device
> > > > >
> > > > > But the comments are not precise enough IMHO. Can you rephrase them?
> > > >
> > > > How about:
> > > >
> > > > Follow the steps below to obtain the I3C dynamic address:
> > > >
> > > > 1. Retrieve the assigned-address from the device tree (DT).
> > >
> > > I guess here you mean that you try to pick that address, unless if
> > > already in use on the bus, right?
> > >
> >
> > Sorry, It should be typo. See below
> >
> >
> > > > 2. Look for an available slot address.
> > >
> > > "available address slot"?
> > >
> > > > 3. Look for an address that is pre-reserved by another device with
> > > > assigned-address in DT, but where the device is currently offline.
> > >
> > > I don't think this part is precise enough. You don't look for addresses
> > > "pre-reserved" but rather more for busy address slots, which might no
> > > longer be in-use because the device is currently offline. The fact that
> > > the slot might have been pre-reserved in the DT is a detail that may,
> > > in some cases, not be true. And as far as I understand your changes,
> > > you're not checking the DT addresses but rather more the addresses that
> > > have been allocated live (which is anyway better, because i3c might
> > > very well be used on a !OF platform).
> > >
> > > Once we settle on a description, maybe this can be part of the kdoc for
> > > the main function searching for the best dynamic address?
> >
> > I am not sure I understand what's your means here. The current framework
> > is
> >
> > 1. Get a free address first, the get more infromation from devices, like
> > BCR, DCR ...
> > 2. Check if it is old device, or dt node have assigned-address property.
> > 3. if it is old device, switch to use old address (if old address is free)
> > according to i3c spec. If dt pre-reserved address is free, switch to use
> > dt pre-reserved address.
> >
> > To match 3's requirement as much as possible, when 1 return address, which
> > should avoid return dt's assigned-address.
> >
> >
> > /*
> >  * I3C Framework Address Assignment Flow:
>
> 	 f	   a	   a	      f
>
> >  * 1 Initial Address Assignment: Attempt to obtain a free address first,
>
>      1.	       a       a
>
> > then gather additional information such as PID, BCR, and DRCR
> >  * 2 Address switch:
>
>      2.
>
> >        - 2a: Check if this target device is appeared before. Switch
> > to use prevous address of this device.
>
> 	 the previous	for
>
> >  *     - 2b: Check if this target device have assigned-address property in dt,
>
> 				    has a preferred address based on
> 				    firmware data (DT). Switch to it if
> 				    it is the case and the address is
> 				    free.
>
> Today it's the DT, maybe not tomorrow. You take these values from the
> firmware on the board, that feels more generic than talking about a DT
> property name.
>
> > switch to this address if it is free.
> >  *
> > In step 1, i3c_bus_get_free_addr() is called. To optimize for step 2b, this
> > function should return an address that is not pre-reserved by any target
> > device with an assigned address in the device tree (DT).
>
> This does not make sense, if you want to optimize for 2b, why not
> selecting the assigned-address property in the first place if it's
> available?

This is my first idea. But I gived up this way.

Select an assigned-address here will involve a big change in i3c framework.
There are no PID information in i3c_master_get_free_addr().

In DAA flow:
- SVC is get PID first, the get_free_addr(). This case, we can use PID to
get dt assigned address.(if change/add API)
- But HCI, it is difference, hci_cmd_v2_daa(), get_free_addr() firstly then
send out DAA command. So no PID information when call get_free_addr().

To cover both case, return a *real* free address here is simplest solution.


>  Also, I don't understand why you would care to specifically
> *not* return an address that might be the default one for another
> device in the first place.

If devices A (want adddress 0xA), device B (want address 0xB), if both
device send hot join at the same time. device B's PID less than device A,

Device B will be found firstly, call get_free_addr(), 0xA will be return
if no this patch.

Device A, call try_get_freeaddr() to get 0xB.

So Devcie B will be assign to 0xA, and Device A will be assign to address 0xB.

After do_daa command, framework will add device B and device A into i3c bus.

When framework try to add device B to i3c bus, framework will try switch
device B's address to 0xB from 0xA, but it will be fail because 0xB already
assigned to device A.


> Changing to a free slot (if possible) not
> reserved by another device might be done in 2b, which makes operation 1
> much more simple, so you put all the complexity at the same time.

I am sure if I understand your means here.

I put all address (not reserved by another device) to free slot.

reserved addr by another devices but offline, should be used only when not
free slot avaible.

> Even
> if you are proceeding with two devices asking the DAA at the same time,
> the procedure should work in a way which does not impact the fact that
> the second device will get its desired address if the first can take
> another one.

I am sure if this is still validate if you understand HCI case.

Frank
>
> > If no such address
> > is available, it can return an address that was pre-reserved by a target
> > device that is currently offline.
>
> Thanks,
> Miquèl

Re: [PATCH v3 03/11] i3c: master: Extend address status bit to 4 and add I3C_ADDR_SLOT_EXT_INIT

[Miquel Raynal]

Hi Frank,
  
> > > switch to this address if it is free.
> > >  *
> > > In step 1, i3c_bus_get_free_addr() is called. To optimize for step 2b, this
> > > function should return an address that is not pre-reserved by any target
> > > device with an assigned address in the device tree (DT).  
> >
> > This does not make sense, if you want to optimize for 2b, why not
> > selecting the assigned-address property in the first place if it's
> > available?  
> 
> This is my first idea. But I gived up this way.
> 
> Select an assigned-address here will involve a big change in i3c framework.
> There are no PID information in i3c_master_get_free_addr().
> 
> In DAA flow:
> - SVC is get PID first, the get_free_addr(). This case, we can use PID to
> get dt assigned address.(if change/add API)
> - But HCI, it is difference, hci_cmd_v2_daa(), get_free_addr() firstly then
> send out DAA command. So no PID information when call get_free_addr().
> 
> To cover both case, return a *real* free address here is simplest solution.

But this is a limitation of the HCI driver? So why not addressing this
in the HCI driver instead? It would greatly simplify the core logic
which becomes complex for wrong reasons.

> >  Also, I don't understand why you would care to specifically
> > *not* return an address that might be the default one for another
> > device in the first place.  
> 
> If devices A (want adddress 0xA), device B (want address 0xB), if both
> device send hot join at the same time. device B's PID less than device A,
> 
> Device B will be found firstly, call get_free_addr(), 0xA will be return
> if no this patch.
> 
> Device A, call try_get_freeaddr() to get 0xB.
> 
> So Devcie B will be assign to 0xA, and Device A will be assign to address 0xB.
> 
> After do_daa command, framework will add device B and device A into i3c bus.
> 
> When framework try to add device B to i3c bus, framework will try switch
> device B's address to 0xB from 0xA, but it will be fail because 0xB already
> assigned to device A.

Well, okay, but that's exactly the situation that will happen if these
devices are not described in your DT. I guess it's expected that a
device not described in your DT can be connected, thanks to the
hot-join feature. In this case you cannot know what is this device
preferred address and you might end-up in the exact same situation.

May I question the need for preferred addresses at all? Is this even
part of the spec? What is the use-case?

Thanks,
Miquèl

Re: [PATCH v3 03/11] i3c: master: Extend address status bit to 4 and add I3C_ADDR_SLOT_EXT_INIT

[Frank Li]

On Mon, Sep 02, 2024 at 04:12:50PM +0200, Miquel Raynal wrote:
> Hi Frank,
>
> > > > switch to this address if it is free.
> > > >  *
> > > > In step 1, i3c_bus_get_free_addr() is called. To optimize for step 2b, this
> > > > function should return an address that is not pre-reserved by any target
> > > > device with an assigned address in the device tree (DT).
> > >
> > > This does not make sense, if you want to optimize for 2b, why not
> > > selecting the assigned-address property in the first place if it's
> > > available?
> >
> > This is my first idea. But I gived up this way.
> >
> > Select an assigned-address here will involve a big change in i3c framework.
> > There are no PID information in i3c_master_get_free_addr().
> >
> > In DAA flow:
> > - SVC is get PID first, the get_free_addr(). This case, we can use PID to
> > get dt assigned address.(if change/add API)
> > - But HCI, it is difference, hci_cmd_v2_daa(), get_free_addr() firstly then
> > send out DAA command. So no PID information when call get_free_addr().
> >
> > To cover both case, return a *real* free address here is simplest solution.
>
> But this is a limitation of the HCI driver? So why not addressing this
> in the HCI driver instead? It would greatly simplify the core logic
> which becomes complex for wrong reasons.

It is reasonable requirement to reduce stall SCL time. After get PID, SCL
have to stall low to wait for software get dynamtic address, I3C spec allow
relative long time for this, but still better if hardware can send out PID
and dynamatic address together withoug stall SCL low. Pre-alloc adddress is
good method if consider this.

>
> > >  Also, I don't understand why you would care to specifically
> > > *not* return an address that might be the default one for another
> > > device in the first place.
> >
> > If devices A (want adddress 0xA), device B (want address 0xB), if both
> > device send hot join at the same time. device B's PID less than device A,
> >
> > Device B will be found firstly, call get_free_addr(), 0xA will be return
> > if no this patch.
> >
> > Device A, call try_get_freeaddr() to get 0xB.
> >
> > So Devcie B will be assign to 0xA, and Device A will be assign to address 0xB.
> >
> > After do_daa command, framework will add device B and device A into i3c bus.
> >
> > When framework try to add device B to i3c bus, framework will try switch
> > device B's address to 0xB from 0xA, but it will be fail because 0xB already
> > assigned to device A.
>
> Well, okay, but that's exactly the situation that will happen if these
> devices are not described in your DT. I guess it's expected that a
> device not described in your DT can be connected, thanks to the
> hot-join feature. In this case you cannot know what is this device
> preferred address and you might end-up in the exact same situation.

If not descript in DT, it means that any dynmatic address can be assigned.

>
> May I question the need for preferred addresses at all? Is this even
> part of the spec? What is the use-case?

It is implements detail. I3C spec said lower dynamtic address have high IBI
priority. Spec just said assign lower dynamtic address if want to higher
IBI prioerity. Using DT assign-address just is one implement method.

>
> Thanks,
> Miquèl

Re: [PATCH v3 03/11] i3c: master: Extend address status bit to 4 and add I3C_ADDR_SLOT_EXT_INIT

[Miquel Raynal]

Hi Frank,

Frank.li@nxp.com wrote on Mon, 2 Sep 2024 14:20:51 -0400:

> On Mon, Sep 02, 2024 at 04:12:50PM +0200, Miquel Raynal wrote:
> > Hi Frank,
> >  
> > > > > switch to this address if it is free.
> > > > >  *
> > > > > In step 1, i3c_bus_get_free_addr() is called. To optimize for step 2b, this
> > > > > function should return an address that is not pre-reserved by any target
> > > > > device with an assigned address in the device tree (DT).  
> > > >
> > > > This does not make sense, if you want to optimize for 2b, why not
> > > > selecting the assigned-address property in the first place if it's
> > > > available?  
> > >
> > > This is my first idea. But I gived up this way.
> > >
> > > Select an assigned-address here will involve a big change in i3c framework.
> > > There are no PID information in i3c_master_get_free_addr().
> > >
> > > In DAA flow:
> > > - SVC is get PID first, the get_free_addr(). This case, we can use PID to
> > > get dt assigned address.(if change/add API)
> > > - But HCI, it is difference, hci_cmd_v2_daa(), get_free_addr() firstly then
> > > send out DAA command. So no PID information when call get_free_addr().
> > >
> > > To cover both case, return a *real* free address here is simplest solution.  
> >
> > But this is a limitation of the HCI driver? So why not addressing this
> > in the HCI driver instead? It would greatly simplify the core logic
> > which becomes complex for wrong reasons.  
> 
> It is reasonable requirement to reduce stall SCL time. After get PID, SCL
> have to stall low to wait for software get dynamtic address, I3C spec allow
> relative long time for this, but still better if hardware can send out PID
> and dynamatic address together withoug stall SCL low. Pre-alloc adddress is
> good method if consider this.

I don't think it is worth the trouble, given the complexity of all
the changes. I prefer to simplify a bit the software and keep it
readable than gaining few us with SCL low. In this case you also spend
more time on the configuration I guess, so is it better than keeping
SCL low (it will be low for some time anyway).

> > > >  Also, I don't understand why you would care to specifically
> > > > *not* return an address that might be the default one for another
> > > > device in the first place.  
> > >
> > > If devices A (want adddress 0xA), device B (want address 0xB), if both
> > > device send hot join at the same time. device B's PID less than device A,
> > >
> > > Device B will be found firstly, call get_free_addr(), 0xA will be return
> > > if no this patch.
> > >
> > > Device A, call try_get_freeaddr() to get 0xB.
> > >
> > > So Devcie B will be assign to 0xA, and Device A will be assign to address 0xB.
> > >
> > > After do_daa command, framework will add device B and device A into i3c bus.
> > >
> > > When framework try to add device B to i3c bus, framework will try switch
> > > device B's address to 0xB from 0xA, but it will be fail because 0xB already
> > > assigned to device A.  
> >
> > Well, okay, but that's exactly the situation that will happen if these
> > devices are not described in your DT. I guess it's expected that a
> > device not described in your DT can be connected, thanks to the
> > hot-join feature. In this case you cannot know what is this device
> > preferred address and you might end-up in the exact same situation.  
> 
> If not descript in DT, it means that any dynmatic address can be assigned.

That's the point of view of the host. But a device might be "critical"
and expect a low address, but the host not being aware. This is the
same situation as your A and B conflict example.

> > May I question the need for preferred addresses at all? Is this even
> > part of the spec? What is the use-case?  
> 
> It is implements detail. I3C spec said lower dynamtic address have high IBI
> priority. Spec just said assign lower dynamtic address if want to higher
> IBI prioerity. Using DT assign-address just is one implement method.

Thanks for all the information, for me the HCI driver must be modified
to retrieve the PID before assigning the dynamic address.

Thanks,
Miquèl

Re: [PATCH v3 03/11] i3c: master: Extend address status bit to 4 and add I3C_ADDR_SLOT_EXT_INIT

[Frank Li]

On Tue, Sep 03, 2024 at 03:00:38PM +0200, Miquel Raynal wrote:
> Hi Frank,
>
> Frank.li@nxp.com wrote on Mon, 2 Sep 2024 14:20:51 -0400:
>
> > On Mon, Sep 02, 2024 at 04:12:50PM +0200, Miquel Raynal wrote:
> > > Hi Frank,
> > >
> > > > > > switch to this address if it is free.
> > > > > >  *
> > > > > > In step 1, i3c_bus_get_free_addr() is called. To optimize for step 2b, this
> > > > > > function should return an address that is not pre-reserved by any target
> > > > > > device with an assigned address in the device tree (DT).
> > > > >
> > > > > This does not make sense, if you want to optimize for 2b, why not
> > > > > selecting the assigned-address property in the first place if it's
> > > > > available?
> > > >
> > > > This is my first idea. But I gived up this way.
> > > >
> > > > Select an assigned-address here will involve a big change in i3c framework.
> > > > There are no PID information in i3c_master_get_free_addr().
> > > >
> > > > In DAA flow:
> > > > - SVC is get PID first, the get_free_addr(). This case, we can use PID to
> > > > get dt assigned address.(if change/add API)
> > > > - But HCI, it is difference, hci_cmd_v2_daa(), get_free_addr() firstly then
> > > > send out DAA command. So no PID information when call get_free_addr().
> > > >
> > > > To cover both case, return a *real* free address here is simplest solution.
> > >
> > > But this is a limitation of the HCI driver? So why not addressing this
> > > in the HCI driver instead? It would greatly simplify the core logic
> > > which becomes complex for wrong reasons.
> >
> > It is reasonable requirement to reduce stall SCL time. After get PID, SCL
> > have to stall low to wait for software get dynamtic address, I3C spec allow
> > relative long time for this, but still better if hardware can send out PID
> > and dynamatic address together withoug stall SCL low. Pre-alloc adddress is
> > good method if consider this.
>
> I don't think it is worth the trouble, given the complexity of all
> the changes. I prefer to simplify a bit the software and keep it
> readable than gaining few us with SCL low. In this case you also spend
> more time on the configuration I guess, so is it better than keeping
> SCL low (it will be low for some time anyway).

Yes, but see below about HCI. But two solutions will be worse.

>
> > > > >  Also, I don't understand why you would care to specifically
> > > > > *not* return an address that might be the default one for another
> > > > > device in the first place.
> > > >
> > > > If devices A (want adddress 0xA), device B (want address 0xB), if both
> > > > device send hot join at the same time. device B's PID less than device A,
> > > >
> > > > Device B will be found firstly, call get_free_addr(), 0xA will be return
> > > > if no this patch.
> > > >
> > > > Device A, call try_get_freeaddr() to get 0xB.
> > > >
> > > > So Devcie B will be assign to 0xA, and Device A will be assign to address 0xB.
> > > >
> > > > After do_daa command, framework will add device B and device A into i3c bus.
> > > >
> > > > When framework try to add device B to i3c bus, framework will try switch
> > > > device B's address to 0xB from 0xA, but it will be fail because 0xB already
> > > > assigned to device A.
> > >
> > > Well, okay, but that's exactly the situation that will happen if these
> > > devices are not described in your DT. I guess it's expected that a
> > > device not described in your DT can be connected, thanks to the
> > > hot-join feature. In this case you cannot know what is this device
> > > preferred address and you might end-up in the exact same situation.
> >
> > If not descript in DT, it means that any dynmatic address can be assigned.
>
> That's the point of view of the host. But a device might be "critical"
> and expect a low address, but the host not being aware. This is the
> same situation as your A and B conflict example.

DT provided addtional information to let host aware it.

>
> > > May I question the need for preferred addresses at all? Is this even
> > > part of the spec? What is the use-case?
> >
> > It is implements detail. I3C spec said lower dynamtic address have high IBI
> > priority. Spec just said assign lower dynamtic address if want to higher
> > IBI prioerity. Using DT assign-address just is one implement method.
>
> Thanks for all the information, for me the HCI driver must be modified
> to retrieve the PID before assigning the dynamic address.

I am not famillar with HCI, but according to I3C HCI Spec 1.2, sec 6.4.1
Dynamic Address Assignment with ENTDAA:

I think it is impossible to do that. A dynatimic address must be provided
before issue ENTDAA command. HCI is MIPI I3C standard defined Host
interface. we have to consider this.

Frank
>
> Thanks,
> Miquèl

Re: [PATCH v3 03/11] i3c: master: Extend address status bit to 4 and add I3C_ADDR_SLOT_EXT_INIT

[Frank Li]

On Tue, Sep 03, 2024 at 11:06:30AM -0400, Frank Li wrote:
> On Tue, Sep 03, 2024 at 03:00:38PM +0200, Miquel Raynal wrote:
> > Hi Frank,
> >
> > Frank.li@nxp.com wrote on Mon, 2 Sep 2024 14:20:51 -0400:
> >
> > > On Mon, Sep 02, 2024 at 04:12:50PM +0200, Miquel Raynal wrote:
> > > > Hi Frank,
> > > >
> > > > > > > switch to this address if it is free.
> > > > > > >  *
> > > > > > > In step 1, i3c_bus_get_free_addr() is called. To optimize for step 2b, this
> > > > > > > function should return an address that is not pre-reserved by any target
> > > > > > > device with an assigned address in the device tree (DT).
> > > > > >
> > > > > > This does not make sense, if you want to optimize for 2b, why not
> > > > > > selecting the assigned-address property in the first place if it's
> > > > > > available?
> > > > >
> > > > > This is my first idea. But I gived up this way.
> > > > >
> > > > > Select an assigned-address here will involve a big change in i3c framework.
> > > > > There are no PID information in i3c_master_get_free_addr().
> > > > >
> > > > > In DAA flow:
> > > > > - SVC is get PID first, the get_free_addr(). This case, we can use PID to
> > > > > get dt assigned address.(if change/add API)
> > > > > - But HCI, it is difference, hci_cmd_v2_daa(), get_free_addr() firstly then
> > > > > send out DAA command. So no PID information when call get_free_addr().
> > > > >
> > > > > To cover both case, return a *real* free address here is simplest solution.
> > > >
> > > > But this is a limitation of the HCI driver? So why not addressing this
> > > > in the HCI driver instead? It would greatly simplify the core logic
> > > > which becomes complex for wrong reasons.
> > >
> > > It is reasonable requirement to reduce stall SCL time. After get PID, SCL
> > > have to stall low to wait for software get dynamtic address, I3C spec allow
> > > relative long time for this, but still better if hardware can send out PID
> > > and dynamatic address together withoug stall SCL low. Pre-alloc adddress is
> > > good method if consider this.
> >
> > I don't think it is worth the trouble, given the complexity of all
> > the changes. I prefer to simplify a bit the software and keep it
> > readable than gaining few us with SCL low. In this case you also spend
> > more time on the configuration I guess, so is it better than keeping
> > SCL low (it will be low for some time anyway).
>
> Yes, but see below about HCI. But two solutions will be worse.
>
> >
> > > > > >  Also, I don't understand why you would care to specifically
> > > > > > *not* return an address that might be the default one for another
> > > > > > device in the first place.
> > > > >
> > > > > If devices A (want adddress 0xA), device B (want address 0xB), if both
> > > > > device send hot join at the same time. device B's PID less than device A,
> > > > >
> > > > > Device B will be found firstly, call get_free_addr(), 0xA will be return
> > > > > if no this patch.
> > > > >
> > > > > Device A, call try_get_freeaddr() to get 0xB.
> > > > >
> > > > > So Devcie B will be assign to 0xA, and Device A will be assign to address 0xB.
> > > > >
> > > > > After do_daa command, framework will add device B and device A into i3c bus.
> > > > >
> > > > > When framework try to add device B to i3c bus, framework will try switch
> > > > > device B's address to 0xB from 0xA, but it will be fail because 0xB already
> > > > > assigned to device A.
> > > >
> > > > Well, okay, but that's exactly the situation that will happen if these
> > > > devices are not described in your DT. I guess it's expected that a
> > > > device not described in your DT can be connected, thanks to the
> > > > hot-join feature. In this case you cannot know what is this device
> > > > preferred address and you might end-up in the exact same situation.
> > >
> > > If not descript in DT, it means that any dynmatic address can be assigned.
> >
> > That's the point of view of the host. But a device might be "critical"
> > and expect a low address, but the host not being aware. This is the
> > same situation as your A and B conflict example.
>
> DT provided addtional information to let host aware it.
>
> >
> > > > May I question the need for preferred addresses at all? Is this even
> > > > part of the spec? What is the use-case?
> > >
> > > It is implements detail. I3C spec said lower dynamtic address have high IBI
> > > priority. Spec just said assign lower dynamtic address if want to higher
> > > IBI prioerity. Using DT assign-address just is one implement method.
> >
> > Thanks for all the information, for me the HCI driver must be modified
> > to retrieve the PID before assigning the dynamic address.
>
> I am not famillar with HCI, but according to I3C HCI Spec 1.2, sec 6.4.1
> Dynamic Address Assignment with ENTDAA:
>
> I think it is impossible to do that. A dynatimic address must be provided
> before issue ENTDAA command. HCI is MIPI I3C standard defined Host
> interface. we have to consider this.

Miquèl:
	Do you have any additional comments for this?

Frank

>
> Frank
> >
> > Thanks,
> > Miquèl

Re: [PATCH v3 03/11] i3c: master: Extend address status bit to 4 and add I3C_ADDR_SLOT_EXT_INIT

[Frank Li]

On Mon, Sep 09, 2024 at 04:01:32PM -0400, Frank Li wrote:
> On Tue, Sep 03, 2024 at 11:06:30AM -0400, Frank Li wrote:
> > On Tue, Sep 03, 2024 at 03:00:38PM +0200, Miquel Raynal wrote:
> > > Hi Frank,
> > >
> > > Frank.li@nxp.com wrote on Mon, 2 Sep 2024 14:20:51 -0400:
> > >
> > > > On Mon, Sep 02, 2024 at 04:12:50PM +0200, Miquel Raynal wrote:
> > > > > Hi Frank,
> > > > >
> > > > > > > > switch to this address if it is free.
> > > > > > > >  *
> > > > > > > > In step 1, i3c_bus_get_free_addr() is called. To optimize for step 2b, this
> > > > > > > > function should return an address that is not pre-reserved by any target
> > > > > > > > device with an assigned address in the device tree (DT).
> > > > > > >
> > > > > > > This does not make sense, if you want to optimize for 2b, why not
> > > > > > > selecting the assigned-address property in the first place if it's
> > > > > > > available?
> > > > > >
> > > > > > This is my first idea. But I gived up this way.
> > > > > >
> > > > > > Select an assigned-address here will involve a big change in i3c framework.
> > > > > > There are no PID information in i3c_master_get_free_addr().
> > > > > >
> > > > > > In DAA flow:
> > > > > > - SVC is get PID first, the get_free_addr(). This case, we can use PID to
> > > > > > get dt assigned address.(if change/add API)
> > > > > > - But HCI, it is difference, hci_cmd_v2_daa(), get_free_addr() firstly then
> > > > > > send out DAA command. So no PID information when call get_free_addr().
> > > > > >
> > > > > > To cover both case, return a *real* free address here is simplest solution.
> > > > >
> > > > > But this is a limitation of the HCI driver? So why not addressing this
> > > > > in the HCI driver instead? It would greatly simplify the core logic
> > > > > which becomes complex for wrong reasons.
> > > >
> > > > It is reasonable requirement to reduce stall SCL time. After get PID, SCL
> > > > have to stall low to wait for software get dynamtic address, I3C spec allow
> > > > relative long time for this, but still better if hardware can send out PID
> > > > and dynamatic address together withoug stall SCL low. Pre-alloc adddress is
> > > > good method if consider this.
> > >
> > > I don't think it is worth the trouble, given the complexity of all
> > > the changes. I prefer to simplify a bit the software and keep it
> > > readable than gaining few us with SCL low. In this case you also spend
> > > more time on the configuration I guess, so is it better than keeping
> > > SCL low (it will be low for some time anyway).
> >
> > Yes, but see below about HCI. But two solutions will be worse.
> >
> > >
> > > > > > >  Also, I don't understand why you would care to specifically
> > > > > > > *not* return an address that might be the default one for another
> > > > > > > device in the first place.
> > > > > >
> > > > > > If devices A (want adddress 0xA), device B (want address 0xB), if both
> > > > > > device send hot join at the same time. device B's PID less than device A,
> > > > > >
> > > > > > Device B will be found firstly, call get_free_addr(), 0xA will be return
> > > > > > if no this patch.
> > > > > >
> > > > > > Device A, call try_get_freeaddr() to get 0xB.
> > > > > >
> > > > > > So Devcie B will be assign to 0xA, and Device A will be assign to address 0xB.
> > > > > >
> > > > > > After do_daa command, framework will add device B and device A into i3c bus.
> > > > > >
> > > > > > When framework try to add device B to i3c bus, framework will try switch
> > > > > > device B's address to 0xB from 0xA, but it will be fail because 0xB already
> > > > > > assigned to device A.
> > > > >
> > > > > Well, okay, but that's exactly the situation that will happen if these
> > > > > devices are not described in your DT. I guess it's expected that a
> > > > > device not described in your DT can be connected, thanks to the
> > > > > hot-join feature. In this case you cannot know what is this device
> > > > > preferred address and you might end-up in the exact same situation.
> > > >
> > > > If not descript in DT, it means that any dynmatic address can be assigned.
> > >
> > > That's the point of view of the host. But a device might be "critical"
> > > and expect a low address, but the host not being aware. This is the
> > > same situation as your A and B conflict example.
> >
> > DT provided addtional information to let host aware it.
> >
> > >
> > > > > May I question the need for preferred addresses at all? Is this even
> > > > > part of the spec? What is the use-case?
> > > >
> > > > It is implements detail. I3C spec said lower dynamtic address have high IBI
> > > > priority. Spec just said assign lower dynamtic address if want to higher
> > > > IBI prioerity. Using DT assign-address just is one implement method.
> > >
> > > Thanks for all the information, for me the HCI driver must be modified
> > > to retrieve the PID before assigning the dynamic address.
> >
> > I am not famillar with HCI, but according to I3C HCI Spec 1.2, sec 6.4.1
> > Dynamic Address Assignment with ENTDAA:
> >
> > I think it is impossible to do that. A dynatimic address must be provided
> > before issue ENTDAA command. HCI is MIPI I3C standard defined Host
> > interface. we have to consider this.
>
> Miquèl:
> 	Do you have any additional comments for this?
>
> Frank

Do you have any additional comments? HCI is MIPI I3C standard. we have to
consider this.

Frank

>
> >
> > Frank
> > >
> > > Thanks,
> > > Miquèl
>
> --
> linux-i3c mailing list
> linux-i3c@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-i3c

[PATCH v3 04/11] i3c: master: Fix dynamic address leak when 'assigned-address' is present

[Frank Li]

If the DTS contains 'assigned-address', a dynamic address leak occurs
during hotjoin events.

Assume a device have assigned-address 0xb.
  - Device issue Hotjoin
  - Call i3c_master_do_daa()
  - Call driver xxx_do_daa()
  - Call i3c_master_get_free_addr() to get dynamic address 0x9
  - i3c_master_add_i3c_dev_locked(0x9)
  -     expected_dyn_addr  = newdev->boardinfo->init_dyn_addr (0xb);
  -     i3c_master_reattach_i3c_dev(newdev(0xb), old_dyn_addr(0x9));
  -         if (dev->info.dyn_addr != old_dyn_addr &&
                ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ 0xb != 0x9 -> TRUE
                (!dev->boardinfo ||
                 ^^^^^^^^^^^^^^^ ->  FALSE
                 dev->info.dyn_addr != dev->boardinfo->init_dyn_addr)) {
                 ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
                 0xb != 0xb      ->  FALSE
                 ...
                 i3c_bus_set_addr_slot_status(&master->bus, old_dyn_addr,
                                                     I3C_ADDR_SLOT_FREE);
		 ^^^
                 This will be skipped. So old_dyn_addr never free
            }

  - i3c_master_get_free_addr() will return increased sequence number.

Remove dev->info.dyn_addr != dev->boardinfo->init_dyn_addr condition check.
dev->info.dyn_addr should be checked before calling this function because
i3c_master_setnewda_locked() has already been called and the target device
has already accepted dyn_addr. It is too late to check if dyn_addr is free
in i3c_master_reattach_i3c_dev().

Add check to ensure expected_dyn_addr is free before
i3c_master_setnewda_locked().

Fixes: cc3a392d69b6 ("i3c: master: fix for SETDASA and DAA process")
Cc: stable@kernel.org
Signed-off-by: Frank Li <Frank.Li@nxp.com>
---
 drivers/i3c/master.c | 15 +++++----------
 1 file changed, 5 insertions(+), 10 deletions(-)

diff --git a/drivers/i3c/master.c b/drivers/i3c/master.c
index 4281f673e08d8..c8eaeada54781 100644
--- a/drivers/i3c/master.c
+++ b/drivers/i3c/master.c
@@ -1531,16 +1531,9 @@ static int i3c_master_reattach_i3c_dev(struct i3c_dev_desc *dev,
 				       u8 old_dyn_addr)
 {
 	struct i3c_master_controller *master = i3c_dev_get_master(dev);
-	enum i3c_addr_slot_status status;
 	int ret;
 
-	if (dev->info.dyn_addr != old_dyn_addr &&
-	    (!dev->boardinfo ||
-	     dev->info.dyn_addr != dev->boardinfo->init_dyn_addr)) {
-		status = i3c_bus_get_addr_slot_status(&master->bus,
-						      dev->info.dyn_addr);
-		if (status != I3C_ADDR_SLOT_FREE)
-			return -EBUSY;
+	if (dev->info.dyn_addr != old_dyn_addr) {
 		i3c_bus_set_addr_slot_status(&master->bus,
 					     dev->info.dyn_addr,
 					     I3C_ADDR_SLOT_I3C_DEV);
@@ -1931,9 +1924,10 @@ static int i3c_master_bus_init(struct i3c_master_controller *master)
 			goto err_rstdaa;
 		}
 
+		/* Not mark as occupied until real device exist in bus */
 		i3c_bus_set_addr_slot_status_ext(&master->bus,
 						 i3cboardinfo->init_dyn_addr,
-						 I3C_ADDR_SLOT_I3C_DEV | I3C_ADDR_SLOT_EXT_INIT);
+						 I3C_ADDR_SLOT_EXT_INIT);
 
 		/*
 		 * Only try to create/attach devices that have a static
@@ -2094,7 +2088,8 @@ int i3c_master_add_i3c_dev_locked(struct i3c_master_controller *master,
 	else
 		expected_dyn_addr = newdev->info.dyn_addr;
 
-	if (newdev->info.dyn_addr != expected_dyn_addr) {
+	if (newdev->info.dyn_addr != expected_dyn_addr &&
+	    i3c_bus_get_addr_slot_status(&master->bus, expected_dyn_addr) == I3C_ADDR_SLOT_FREE) {
 		/*
 		 * Try to apply the expected dynamic address. If it fails, keep
 		 * the address assigned by the master.

-- 
2.34.1

[PATCH v3 05/11] i3c: master: Fix miss free init_dyn_addr at i3c_master_put_i3c_addrs()

[Frank Li]

if (dev->boardinfo && dev->boardinfo->init_dyn_addr)
                                      ^^^ here check "init_dyn_addr"
	i3c_bus_set_addr_slot_status(&master->bus, dev->info.dyn_addr, ...)
						             ^^^^
							free "dyn_addr"
Fix typo "dyn_addr" by replacing it with "init_dyn_addr".

Cc: stable@kernel.org
Fixes: 3a379bbcea0a ("i3c: Add core I3C infrastructure")
Signed-off-by: Frank Li <Frank.Li@nxp.com>
---
 drivers/i3c/master.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/i3c/master.c b/drivers/i3c/master.c
index c8eaeada54781..65642e5afdcfb 100644
--- a/drivers/i3c/master.c
+++ b/drivers/i3c/master.c
@@ -1442,7 +1442,7 @@ static void i3c_master_put_i3c_addrs(struct i3c_dev_desc *dev)
 					     I3C_ADDR_SLOT_FREE);
 
 	if (dev->boardinfo && dev->boardinfo->init_dyn_addr)
-		i3c_bus_set_addr_slot_status(&master->bus, dev->info.dyn_addr,
+		i3c_bus_set_addr_slot_status(&master->bus, dev->boardinfo->init_dyn_addr,
 					     I3C_ADDR_SLOT_FREE);
 }
 

-- 
2.34.1

Re: [PATCH v3 05/11] i3c: master: Fix miss free init_dyn_addr at i3c_master_put_i3c_addrs()

[Miquel Raynal]

Hi Frank,

Frank.Li@nxp.com wrote on Mon, 19 Aug 2024 12:01:59 -0400:

> if (dev->boardinfo && dev->boardinfo->init_dyn_addr)
>                                       ^^^ here check "init_dyn_addr"
> 	i3c_bus_set_addr_slot_status(&master->bus, dev->info.dyn_addr, ...)
> 						             ^^^^
> 							free "dyn_addr"
> Fix typo "dyn_addr" by replacing it with "init_dyn_addr".

Maybe you can mention it's a copy/paste error, which makes the diff
-when looking at the previous lines- obvious.

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

Thanks,
Miquèl

[PATCH v3 06/11] i3c: master: svc: use repeat start when IBI WIN happens

[Frank Li]

There is a possibility of an IBI WIN occurring when addressing issues, even
when sending CCC commands. Most of the time, returning -EAGAIN is
acceptable, but the case below becomes highly complex.

When a Hotjoin event occurs:
- i3c_master_do_daa()
  - i3c_master_add_i3c_dev_locked()
    - A dynamic address (e.g., 0x9) is already set during DAA.
    - i3c_master_getpid_locked()
      - Another device issues HJ or IBI here. Returning -EAGAIN causes
        failure in adding the new device. However, the dynamic address(0x9)
        has already been assigned to this device. If another device issues
        HJ, it will get this address 0x9 again, causing two devices on the
        bus to use the same dynamic address 0x9.
      - Attempting to send RSTDAA when the first device fails at
        i3c_master_getpid_locked() could also fail when sending RSTDAA for
        the same reason.

According to the I3C spec, address arbitration only happens at START, never
at REPEAT start. Using repeat start when an IBI WIN occurs simplifies this
case, as i3c_master_getpid_locked() will not return an error when another
device tries to send HJ or IBI.

Signed-off-by: Frank Li <Frank.Li@nxp.com>
---
 drivers/i3c/master/svc-i3c-master.c | 36 ++++++++++++++++++------------------
 1 file changed, 18 insertions(+), 18 deletions(-)

diff --git a/drivers/i3c/master/svc-i3c-master.c b/drivers/i3c/master/svc-i3c-master.c
index e80c002991f75..5d19251238ff8 100644
--- a/drivers/i3c/master/svc-i3c-master.c
+++ b/drivers/i3c/master/svc-i3c-master.c
@@ -1099,6 +1099,24 @@ static int svc_i3c_master_xfer(struct svc_i3c_master *master,
 		if (ret)
 			goto emit_stop;
 
+		/*
+		 * According to I3C spec ver 1.1.1, 5.1.2.2.3 Consequence of Controller Starting a
+		 * Frame with I3C Target Address.
+		 *
+		 * The I3C Controller normally should start a Frame, the Address may be arbitrated,
+		 * and so the Controller shall monitor to see whether an In-Band Interrupt request,
+		 * a Controller Role Request (i.e., Secondary Controller requests to become the
+		 * Active Controller), or a Hot-Join Request has been made.
+		 *
+		 * If missed IBIWON check, the wrong data will be return. When IBIWON happen, issue
+		 * repeat start. Address arbitrate only happen at START, never happen at REPEAT
+		 * start.
+		 */
+		if (SVC_I3C_MSTATUS_IBIWON(reg)) {
+			writel(SVC_I3C_MINT_IBIWON, master->regs + SVC_I3C_MSTATUS);
+			continue;
+		}
+
 		if (readl(master->regs + SVC_I3C_MERRWARN) & SVC_I3C_MERRWARN_NACK) {
 			/*
 			 * According to I3C Spec 1.1.1, 11-Jun-2021, section: 5.1.2.2.3.
@@ -1132,24 +1150,6 @@ static int svc_i3c_master_xfer(struct svc_i3c_master *master,
 		}
 	}
 
-	/*
-	 * According to I3C spec ver 1.1.1, 5.1.2.2.3 Consequence of Controller Starting a Frame
-	 * with I3C Target Address.
-	 *
-	 * The I3C Controller normally should start a Frame, the Address may be arbitrated, and so
-	 * the Controller shall monitor to see whether an In-Band Interrupt request, a Controller
-	 * Role Request (i.e., Secondary Controller requests to become the Active Controller), or
-	 * a Hot-Join Request has been made.
-	 *
-	 * If missed IBIWON check, the wrong data will be return. When IBIWON happen, return failure
-	 * and yield the above events handler.
-	 */
-	if (SVC_I3C_MSTATUS_IBIWON(reg)) {
-		ret = -EAGAIN;
-		*actual_len = 0;
-		goto emit_stop;
-	}
-
 	if (rnw)
 		ret = svc_i3c_master_read(master, in, xfer_len);
 	else

-- 
2.34.1

Re: [PATCH v3 06/11] i3c: master: svc: use repeat start when IBI WIN happens

[Miquel Raynal]

Hi Frank,

Frank.Li@nxp.com wrote on Mon, 19 Aug 2024 12:02:00 -0400:

> There is a possibility of an IBI WIN occurring when addressing issues, even
> when sending CCC commands. Most of the time, returning -EAGAIN is
> acceptable, but the case below becomes highly complex.
> 
> When a Hotjoin event occurs:
> - i3c_master_do_daa()
>   - i3c_master_add_i3c_dev_locked()
>     - A dynamic address (e.g., 0x9) is already set during DAA.
>     - i3c_master_getpid_locked()
>       - Another device issues HJ or IBI here. Returning -EAGAIN causes
>         failure in adding the new device. However, the dynamic address(0x9)
>         has already been assigned to this device. If another device issues
>         HJ, it will get this address 0x9 again, causing two devices on the
>         bus to use the same dynamic address 0x9.
>       - Attempting to send RSTDAA when the first device fails at
>         i3c_master_getpid_locked() could also fail when sending RSTDAA for
>         the same reason.
> 
> According to the I3C spec, address arbitration only happens at START, never
> at REPEAT start. Using repeat start when an IBI WIN occurs simplifies this
> case, as i3c_master_getpid_locked() will not return an error when another
> device tries to send HJ or IBI.
> 
> Signed-off-by: Frank Li <Frank.Li@nxp.com>

Feels sensible.

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

Thanks,
Miquèl

[PATCH v3 07/11] i3c: master: svc: manually emit NACK/ACK for hotjoin

[Frank Li]

When the address is arbitrated at send address, the hardware can auto-send
NACK if it is an IBI. However, manual emission of NACK/ACK is needed for
hot join or controller request events.

Add help function svc_i3c_master_handle_ibi_won() to check event type and
send out NACK if the event is not an IBI.

Signed-off-by: Frank Li <Frank.Li@nxp.com>
---
 drivers/i3c/master/svc-i3c-master.c | 20 +++++++++++++++++++-
 1 file changed, 19 insertions(+), 1 deletion(-)

diff --git a/drivers/i3c/master/svc-i3c-master.c b/drivers/i3c/master/svc-i3c-master.c
index 5d19251238ff8..d665639523e3c 100644
--- a/drivers/i3c/master/svc-i3c-master.c
+++ b/drivers/i3c/master/svc-i3c-master.c
@@ -405,6 +405,24 @@ static void svc_i3c_master_nack_ibi(struct svc_i3c_master *master)
 	       master->regs + SVC_I3C_MCTRL);
 }
 
+static int svc_i3c_master_handle_ibi_won(struct svc_i3c_master *master, u32 mstatus)
+{
+	u32 ibitype;
+
+	ibitype = SVC_I3C_MSTATUS_IBITYPE(mstatus);
+
+	writel(SVC_I3C_MINT_IBIWON, master->regs + SVC_I3C_MSTATUS);
+
+	/* Hardware can't auto emit NACK for hot join and master request */
+	switch (ibitype) {
+	case SVC_I3C_MSTATUS_IBITYPE_HOT_JOIN:
+	case SVC_I3C_MSTATUS_IBITYPE_MASTER_REQUEST:
+		svc_i3c_master_nack_ibi(master);
+	}
+
+	return 0;
+}
+
 static void svc_i3c_master_ibi_work(struct work_struct *work)
 {
 	struct svc_i3c_master *master = container_of(work, struct svc_i3c_master, ibi_work);
@@ -1113,7 +1131,7 @@ static int svc_i3c_master_xfer(struct svc_i3c_master *master,
 		 * start.
 		 */
 		if (SVC_I3C_MSTATUS_IBIWON(reg)) {
-			writel(SVC_I3C_MINT_IBIWON, master->regs + SVC_I3C_MSTATUS);
+			svc_i3c_master_handle_ibi_won(master, reg);
 			continue;
 		}
 

-- 
2.34.1

Re: [PATCH v3 07/11] i3c: master: svc: manually emit NACK/ACK for hotjoin

[Miquel Raynal]

Hi Frank,

Frank.Li@nxp.com wrote on Mon, 19 Aug 2024 12:02:01 -0400:

> When the address is arbitrated at send address, the hardware can auto-send
> NACK if it is an IBI. However, manual emission of NACK/ACK is needed for
> hot join or controller request events.
> 
> Add help function svc_i3c_master_handle_ibi_won() to check event type and
> send out NACK if the event is not an IBI.
> 
> Signed-off-by: Frank Li <Frank.Li@nxp.com>

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

Thanks,
Miquèl

[PATCH v3 08/11] i3c: master: svc: need check IBIWON for dynamtica address assign

[Frank Li]

When sending REQUEST_PROC_DAA, emit START and address 7E. Address
arbitration may occur at this time if other devices trigger HJ, IBI, or
CR events.

When IBIWON happen at REQUEST_PROC_DAA, nack IBI request then use repeat
start to continue current dynamtica address assign.

Signed-off-by: Frank Li <Frank.Li@nxp.com>
---
 drivers/i3c/master/svc-i3c-master.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/drivers/i3c/master/svc-i3c-master.c b/drivers/i3c/master/svc-i3c-master.c
index d665639523e3c..161ccd824443b 100644
--- a/drivers/i3c/master/svc-i3c-master.c
+++ b/drivers/i3c/master/svc-i3c-master.c
@@ -808,6 +808,9 @@ static int svc_i3c_master_do_daa_locked(struct svc_i3c_master *master,
 	int ret, i;
 
 	while (true) {
+		/* clean SVC_I3C_MINT_IBIWON w1c bits */
+		writel(SVC_I3C_MINT_IBIWON, master->regs + SVC_I3C_MSTATUS);
+
 		/* SVC_I3C_MCTRL_REQUEST_PROC_DAA have two mode, ENTER DAA or PROCESS DAA.
 		 *
 		 * ENTER DAA:
@@ -859,6 +862,9 @@ static int svc_i3c_master_do_daa_locked(struct svc_i3c_master *master,
 			ret = svc_i3c_master_readb(master, data, 2);
 			if (ret)
 				break;
+		} else if (SVC_I3C_MSTATUS_IBIWON(reg)) {
+			svc_i3c_master_handle_ibi_won(master, reg);
+			continue;
 		} else if (SVC_I3C_MSTATUS_MCTRLDONE(reg)) {
 			if (SVC_I3C_MSTATUS_STATE_IDLE(reg) &&
 			    SVC_I3C_MSTATUS_COMPLETE(reg)) {

-- 
2.34.1

Re: [PATCH v3 08/11] i3c: master: svc: need check IBIWON for dynamtica address assign

[Miquel Raynal]

Hi Frank,

Frank.Li@nxp.com wrote on Mon, 19 Aug 2024 12:02:02 -0400:

> When sending REQUEST_PROC_DAA, emit START and address 7E. Address
> arbitration may occur at this time if other devices trigger HJ, IBI, or
> CR events.
> 
> When IBIWON happen at REQUEST_PROC_DAA, nack IBI request then use repeat

		   during a		NACK the	     send a

repeated start

> start to continue current dynamtica address assign.

                             ?
> 
> Signed-off-by: Frank Li <Frank.Li@nxp.com>

Thanks,
Miquèl

[PATCH v3 09/11] i3c: master: svc: use spinlock_saveirq at svc_i3c_master_ibi_work()

[Frank Li]

According to I3C spec ver 1.1, 09-Jun-2021, section 5.1.2.5:

The I3C Controller shall hold SCL Low while the Bus is in I3C/I2C Transfer,
ACK/NACK Phase. But maximum stall time is 100us. We have to disable irq and
schedule during whole I3C transaction, otherwise, I3C bus timeout will
happen if any irq or schedule happen during transaction.

Replace mutex with spinlock_saveirq() to make sure finish whole i3c
transaction without stall SCL more than 100us.

Signed-off-by: Frank Li <Frank.Li@nxp.com>
---
 drivers/i3c/master/svc-i3c-master.c | 14 +++++++++++---
 1 file changed, 11 insertions(+), 3 deletions(-)

diff --git a/drivers/i3c/master/svc-i3c-master.c b/drivers/i3c/master/svc-i3c-master.c
index 161ccd824443b..fbb6cef405577 100644
--- a/drivers/i3c/master/svc-i3c-master.c
+++ b/drivers/i3c/master/svc-i3c-master.c
@@ -432,7 +432,16 @@ static void svc_i3c_master_ibi_work(struct work_struct *work)
 	u32 status, val;
 	int ret;
 
-	mutex_lock(&master->lock);
+	/*
+	 * According to I3C spec ver 1.1, 09-Jun-2021, section 5.1.2.5:
+	 *
+	 * The I3C Controller shall hold SCL Low while the Bus is in I3C/I2C Transfer, ACK/NACK
+	 * Phase. But maximum stall time is 100us. We have to disable irq and schedule during whole
+	 * I3C transaction, otherwise, I3C bus timeout will happen if any irq or schedule happen
+	 * between transaction..
+	 */
+	guard(spinlock_irqsave)(&master->xferqueue.lock);
+
 	/*
 	 * IBIWON may be set before SVC_I3C_MCTRL_REQUEST_AUTO_IBI, causing
 	 * readl_relaxed_poll_timeout() to return immediately. Consequently,
@@ -452,7 +461,7 @@ static void svc_i3c_master_ibi_work(struct work_struct *work)
 	       master->regs + SVC_I3C_MCTRL);
 
 	/* Wait for IBIWON, should take approximately 100us */
-	ret = readl_relaxed_poll_timeout(master->regs + SVC_I3C_MSTATUS, val,
+	ret = readl_relaxed_poll_timeout_atomic(master->regs + SVC_I3C_MSTATUS, val,
 					 SVC_I3C_MSTATUS_IBIWON(val), 0, 1000);
 	if (ret) {
 		dev_err(master->dev, "Timeout when polling for IBIWON\n");
@@ -525,7 +534,6 @@ static void svc_i3c_master_ibi_work(struct work_struct *work)
 
 reenable_ibis:
 	svc_i3c_master_enable_interrupts(master, SVC_I3C_MINT_SLVSTART);
-	mutex_unlock(&master->lock);
 }
 
 static irqreturn_t svc_i3c_master_irq_handler(int irq, void *dev_id)

-- 
2.34.1

Re: [PATCH v3 09/11] i3c: master: svc: use spinlock_saveirq at svc_i3c_master_ibi_work()

[Miquel Raynal]

Hi Frank,

Frank.Li@nxp.com wrote on Mon, 19 Aug 2024 12:02:03 -0400:

> According to I3C spec ver 1.1, 09-Jun-2021, section 5.1.2.5:
> 
> The I3C Controller shall hold SCL Low while the Bus is in I3C/I2C Transfer,

				    low				    transfer

> ACK/NACK Phase. But maximum stall time is 100us. We have to disable irq and

and/or (I'm not sure)				the IRQs

> schedule during whole I3C transaction, otherwise, I3C bus timeout will

prevnet scheduling during the whole 		the      may
timeout.

> happen if any irq or schedule happen during transaction.
> 
> Replace mutex with spinlock_saveirq() to make sure finish whole i3c

			wrong name	to avoid stalling SCL...

> transaction without stall SCL more than 100us.
> 
> Signed-off-by: Frank Li <Frank.Li@nxp.com>

Yes, 100us is low, and that's why I initially did my best to enforce
auto ack/nack. We cannot make sure this limit will not be crossed, and
when it's the case, we need to handle the consequences.

> ---
>  drivers/i3c/master/svc-i3c-master.c | 14 +++++++++++---
>  1 file changed, 11 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/i3c/master/svc-i3c-master.c b/drivers/i3c/master/svc-i3c-master.c
> index 161ccd824443b..fbb6cef405577 100644
> --- a/drivers/i3c/master/svc-i3c-master.c
> +++ b/drivers/i3c/master/svc-i3c-master.c
> @@ -432,7 +432,16 @@ static void svc_i3c_master_ibi_work(struct work_struct *work)
>  	u32 status, val;
>  	int ret;
>  
> -	mutex_lock(&master->lock);

Don't you still need this lock for other concurrency reasons?

> +	/*
> +	 * According to I3C spec ver 1.1, 09-Jun-2021, section 5.1.2.5:
> +	 *
> +	 * The I3C Controller shall hold SCL Low while the Bus is in I3C/I2C Transfer, ACK/NACK
> +	 * Phase. But maximum stall time is 100us. We have to disable irq and schedule during whole
> +	 * I3C transaction, otherwise, I3C bus timeout will happen if any irq or schedule happen
> +	 * between transaction..
> +	 */
> +	guard(spinlock_irqsave)(&master->xferqueue.lock);
> +
>  	/*
>  	 * IBIWON may be set before SVC_I3C_MCTRL_REQUEST_AUTO_IBI, causing
>  	 * readl_relaxed_poll_timeout() to return immediately. Consequently,
> @@ -452,7 +461,7 @@ static void svc_i3c_master_ibi_work(struct work_struct *work)
>  	       master->regs + SVC_I3C_MCTRL);
>  
>  	/* Wait for IBIWON, should take approximately 100us */
> -	ret = readl_relaxed_poll_timeout(master->regs + SVC_I3C_MSTATUS, val,
> +	ret = readl_relaxed_poll_timeout_atomic(master->regs + SVC_I3C_MSTATUS, val,
>  					 SVC_I3C_MSTATUS_IBIWON(val), 0, 1000);

This means you lock one CPU for 100us doing nothing every time you send
a frame, that's not possible. Actually the delay was already very small
(could have been set to ~10 maybe) but this is not possible.

>  	if (ret) {
>  		dev_err(master->dev, "Timeout when polling for IBIWON\n");
> @@ -525,7 +534,6 @@ static void svc_i3c_master_ibi_work(struct work_struct *work)
>  
>  reenable_ibis:
>  	svc_i3c_master_enable_interrupts(master, SVC_I3C_MINT_SLVSTART);
> -	mutex_unlock(&master->lock);
>  }
>  
>  static irqreturn_t svc_i3c_master_irq_handler(int irq, void *dev_id)
> 


Thanks,
Miquèl

Re: [PATCH v3 09/11] i3c: master: svc: use spinlock_saveirq at svc_i3c_master_ibi_work()

[Frank Li]

On Fri, Aug 23, 2024 at 06:19:27PM +0200, Miquel Raynal wrote:
> Hi Frank,
>
> Frank.Li@nxp.com wrote on Mon, 19 Aug 2024 12:02:03 -0400:
>
> > According to I3C spec ver 1.1, 09-Jun-2021, section 5.1.2.5:
> >
> > The I3C Controller shall hold SCL Low while the Bus is in I3C/I2C Transfer,
>
> 				    low				    transfer
>
> > ACK/NACK Phase. But maximum stall time is 100us. We have to disable irq and
>
> and/or (I'm not sure)				the IRQs
>
> > schedule during whole I3C transaction, otherwise, I3C bus timeout will
>
> prevnet scheduling during the whole 		the      may
> timeout.
>
> > happen if any irq or schedule happen during transaction.
> >
> > Replace mutex with spinlock_saveirq() to make sure finish whole i3c
>
> 			wrong name	to avoid stalling SCL...
>
> > transaction without stall SCL more than 100us.
> >
> > Signed-off-by: Frank Li <Frank.Li@nxp.com>
>
> Yes, 100us is low, and that's why I initially did my best to enforce
> auto ack/nack. We cannot make sure this limit will not be crossed, and
> when it's the case, we need to handle the consequences.

Only IBI use auto ack/nack. hardware can't handle it for HJ and CR, which
have to manually send out.


>
> > ---
> >  drivers/i3c/master/svc-i3c-master.c | 14 +++++++++++---
> >  1 file changed, 11 insertions(+), 3 deletions(-)
> >
> > diff --git a/drivers/i3c/master/svc-i3c-master.c b/drivers/i3c/master/svc-i3c-master.c
> > index 161ccd824443b..fbb6cef405577 100644
> > --- a/drivers/i3c/master/svc-i3c-master.c
> > +++ b/drivers/i3c/master/svc-i3c-master.c
> > @@ -432,7 +432,16 @@ static void svc_i3c_master_ibi_work(struct work_struct *work)
> >  	u32 status, val;
> >  	int ret;
> >
> > -	mutex_lock(&master->lock);
>
> Don't you still need this lock for other concurrency reasons?
>
> > +	/*
> > +	 * According to I3C spec ver 1.1, 09-Jun-2021, section 5.1.2.5:
> > +	 *
> > +	 * The I3C Controller shall hold SCL Low while the Bus is in I3C/I2C Transfer, ACK/NACK
> > +	 * Phase. But maximum stall time is 100us. We have to disable irq and schedule during whole
> > +	 * I3C transaction, otherwise, I3C bus timeout will happen if any irq or schedule happen
> > +	 * between transaction..
> > +	 */
> > +	guard(spinlock_irqsave)(&master->xferqueue.lock);
> > +
> >  	/*
> >  	 * IBIWON may be set before SVC_I3C_MCTRL_REQUEST_AUTO_IBI, causing
> >  	 * readl_relaxed_poll_timeout() to return immediately. Consequently,
> > @@ -452,7 +461,7 @@ static void svc_i3c_master_ibi_work(struct work_struct *work)
> >  	       master->regs + SVC_I3C_MCTRL);
> >
> >  	/* Wait for IBIWON, should take approximately 100us */
> > -	ret = readl_relaxed_poll_timeout(master->regs + SVC_I3C_MSTATUS, val,
> > +	ret = readl_relaxed_poll_timeout_atomic(master->regs + SVC_I3C_MSTATUS, val,
> >  					 SVC_I3C_MSTATUS_IBIWON(val), 0, 1000);
>
> This means you lock one CPU for 100us doing nothing every time you send
> a frame, that's not possible. Actually the delay was already very small
> (could have been set to ~10 maybe) but this is not possible.

It happen only at error case. Most time should wait for only 9th SCL.
I think original comment is wrong.

Hardware set IBIWON at 8th SCL.

Frank

>
> >  	if (ret) {
> >  		dev_err(master->dev, "Timeout when polling for IBIWON\n");
> > @@ -525,7 +534,6 @@ static void svc_i3c_master_ibi_work(struct work_struct *work)
> >
> >  reenable_ibis:
> >  	svc_i3c_master_enable_interrupts(master, SVC_I3C_MINT_SLVSTART);
> > -	mutex_unlock(&master->lock);
> >  }
> >
> >  static irqreturn_t svc_i3c_master_irq_handler(int irq, void *dev_id)
> >
>
>
> Thanks,
> Miquèl

[PATCH v3 10/11] i3c: master: svc: wait for Manual ACK/NACK Done before next step

[Frank Li]

Wait for the controller to complete emitting ACK/NACK, otherwise the next
command may be omitted by the hardware.

Add command done check at svc_i3c_master_nack(ack)_ibi() and change return
type to int to indicate wait done timeout.

Signed-off-by: Frank Li <Frank.Li@nxp.com>
---
 drivers/i3c/master/svc-i3c-master.c | 31 +++++++++++++++++++++++++------
 1 file changed, 25 insertions(+), 6 deletions(-)

diff --git a/drivers/i3c/master/svc-i3c-master.c b/drivers/i3c/master/svc-i3c-master.c
index fbb6cef405577..2010495906eb3 100644
--- a/drivers/i3c/master/svc-i3c-master.c
+++ b/drivers/i3c/master/svc-i3c-master.c
@@ -384,10 +384,12 @@ static int svc_i3c_master_handle_ibi(struct svc_i3c_master *master,
 	return 0;
 }
 
-static void svc_i3c_master_ack_ibi(struct svc_i3c_master *master,
+static int svc_i3c_master_ack_ibi(struct svc_i3c_master *master,
 				   bool mandatory_byte)
 {
 	unsigned int ibi_ack_nack;
+	int ret;
+	u32 reg;
 
 	ibi_ack_nack = SVC_I3C_MCTRL_REQUEST_IBI_ACKNACK;
 	if (mandatory_byte)
@@ -396,18 +398,31 @@ static void svc_i3c_master_ack_ibi(struct svc_i3c_master *master,
 		ibi_ack_nack |= SVC_I3C_MCTRL_IBIRESP_ACK_WITHOUT_BYTE;
 
 	writel(ibi_ack_nack, master->regs + SVC_I3C_MCTRL);
+
+	ret = readl_poll_timeout_atomic(master->regs + SVC_I3C_MSTATUS, reg,
+					SVC_I3C_MSTATUS_MCTRLDONE(reg), 1, 1000);
+	return ret;
+
 }
 
-static void svc_i3c_master_nack_ibi(struct svc_i3c_master *master)
+static int svc_i3c_master_nack_ibi(struct svc_i3c_master *master)
 {
+	int ret;
+	u32 reg;
+
 	writel(SVC_I3C_MCTRL_REQUEST_IBI_ACKNACK |
 	       SVC_I3C_MCTRL_IBIRESP_NACK,
 	       master->regs + SVC_I3C_MCTRL);
+
+	ret = readl_poll_timeout_atomic(master->regs + SVC_I3C_MSTATUS, reg,
+					SVC_I3C_MSTATUS_MCTRLDONE(reg), 1, 1000);
+	return ret;
 }
 
 static int svc_i3c_master_handle_ibi_won(struct svc_i3c_master *master, u32 mstatus)
 {
 	u32 ibitype;
+	int ret = 0;
 
 	ibitype = SVC_I3C_MSTATUS_IBITYPE(mstatus);
 
@@ -417,10 +432,10 @@ static int svc_i3c_master_handle_ibi_won(struct svc_i3c_master *master, u32 msta
 	switch (ibitype) {
 	case SVC_I3C_MSTATUS_IBITYPE_HOT_JOIN:
 	case SVC_I3C_MSTATUS_IBITYPE_MASTER_REQUEST:
-		svc_i3c_master_nack_ibi(master);
+		ret = svc_i3c_master_nack_ibi(master);
 	}
 
-	return 0;
+	return ret;
 }
 
 static void svc_i3c_master_ibi_work(struct work_struct *work)
@@ -871,7 +886,9 @@ static int svc_i3c_master_do_daa_locked(struct svc_i3c_master *master,
 			if (ret)
 				break;
 		} else if (SVC_I3C_MSTATUS_IBIWON(reg)) {
-			svc_i3c_master_handle_ibi_won(master, reg);
+			ret = svc_i3c_master_handle_ibi_won(master, reg);
+			if (ret)
+				break;
 			continue;
 		} else if (SVC_I3C_MSTATUS_MCTRLDONE(reg)) {
 			if (SVC_I3C_MSTATUS_STATE_IDLE(reg) &&
@@ -1145,7 +1162,9 @@ static int svc_i3c_master_xfer(struct svc_i3c_master *master,
 		 * start.
 		 */
 		if (SVC_I3C_MSTATUS_IBIWON(reg)) {
-			svc_i3c_master_handle_ibi_won(master, reg);
+			ret = svc_i3c_master_handle_ibi_won(master, reg);
+			if (ret)
+				goto emit_stop;
 			continue;
 		}
 

-- 
2.34.1

Re: [PATCH v3 10/11] i3c: master: svc: wait for Manual ACK/NACK Done before next step

[Miquel Raynal]

Hi Frank,

Frank.Li@nxp.com wrote on Mon, 19 Aug 2024 12:02:04 -0400:

> Wait for the controller to complete emitting ACK/NACK, otherwise the next
> command may be omitted by the hardware.
> 
> Add command done check at svc_i3c_master_nack(ack)_ibi() and change return

	a "command done" check in

the reutnr type

> type to int to indicate wait done timeout.

		flag possible timeouts.

> 
> Signed-off-by: Frank Li <Frank.Li@nxp.com>
> ---
>  drivers/i3c/master/svc-i3c-master.c | 31 +++++++++++++++++++++++++------
>  1 file changed, 25 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/i3c/master/svc-i3c-master.c b/drivers/i3c/master/svc-i3c-master.c
> index fbb6cef405577..2010495906eb3 100644
> --- a/drivers/i3c/master/svc-i3c-master.c
> +++ b/drivers/i3c/master/svc-i3c-master.c
> @@ -384,10 +384,12 @@ static int svc_i3c_master_handle_ibi(struct svc_i3c_master *master,
>  	return 0;
>  }
>  
> -static void svc_i3c_master_ack_ibi(struct svc_i3c_master *master,
> +static int svc_i3c_master_ack_ibi(struct svc_i3c_master *master,
>  				   bool mandatory_byte)
>  {
>  	unsigned int ibi_ack_nack;
> +	int ret;
> +	u32 reg;
>  
>  	ibi_ack_nack = SVC_I3C_MCTRL_REQUEST_IBI_ACKNACK;
>  	if (mandatory_byte)
> @@ -396,18 +398,31 @@ static void svc_i3c_master_ack_ibi(struct svc_i3c_master *master,
>  		ibi_ack_nack |= SVC_I3C_MCTRL_IBIRESP_ACK_WITHOUT_BYTE;
>  
>  	writel(ibi_ack_nack, master->regs + SVC_I3C_MCTRL);
> +
> +	ret = readl_poll_timeout_atomic(master->regs + SVC_I3C_MSTATUS, reg,
> +					SVC_I3C_MSTATUS_MCTRLDONE(reg), 1, 1000);

Still concerned about the _atomic.

> +	return ret;

	return readl...

> +
>  }

Otherwise LGTM

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

Thanks,
Miquèl

Re: [PATCH v3 10/11] i3c: master: svc: wait for Manual ACK/NACK Done before next step

[Frank Li]

On Fri, Aug 23, 2024 at 06:22:40PM +0200, Miquel Raynal wrote:
> Hi Frank,
>
> Frank.Li@nxp.com wrote on Mon, 19 Aug 2024 12:02:04 -0400:
>
> > Wait for the controller to complete emitting ACK/NACK, otherwise the next
> > command may be omitted by the hardware.
> >
> > Add command done check at svc_i3c_master_nack(ack)_ibi() and change return
>
> 	a "command done" check in
>
> the reutnr type
>
> > type to int to indicate wait done timeout.
>
> 		flag possible timeouts.
>
> >
> > Signed-off-by: Frank Li <Frank.Li@nxp.com>
> > ---
> >  drivers/i3c/master/svc-i3c-master.c | 31 +++++++++++++++++++++++++------
> >  1 file changed, 25 insertions(+), 6 deletions(-)
> >
> > diff --git a/drivers/i3c/master/svc-i3c-master.c b/drivers/i3c/master/svc-i3c-master.c
> > index fbb6cef405577..2010495906eb3 100644
> > --- a/drivers/i3c/master/svc-i3c-master.c
> > +++ b/drivers/i3c/master/svc-i3c-master.c
> > @@ -384,10 +384,12 @@ static int svc_i3c_master_handle_ibi(struct svc_i3c_master *master,
> >  	return 0;
> >  }
> >
> > -static void svc_i3c_master_ack_ibi(struct svc_i3c_master *master,
> > +static int svc_i3c_master_ack_ibi(struct svc_i3c_master *master,
> >  				   bool mandatory_byte)
> >  {
> >  	unsigned int ibi_ack_nack;
> > +	int ret;
> > +	u32 reg;
> >
> >  	ibi_ack_nack = SVC_I3C_MCTRL_REQUEST_IBI_ACKNACK;
> >  	if (mandatory_byte)
> > @@ -396,18 +398,31 @@ static void svc_i3c_master_ack_ibi(struct svc_i3c_master *master,
> >  		ibi_ack_nack |= SVC_I3C_MCTRL_IBIRESP_ACK_WITHOUT_BYTE;
> >
> >  	writel(ibi_ack_nack, master->regs + SVC_I3C_MCTRL);
> > +
> > +	ret = readl_poll_timeout_atomic(master->regs + SVC_I3C_MSTATUS, reg,
> > +					SVC_I3C_MSTATUS_MCTRLDONE(reg), 1, 1000);
>
> Still concerned about the _atomic.

It may be call in atomic context. Because hardware design limition, i3c
transact have been in irq disable context to avoid 100us timeout issue.

Frank

>
> > +	return ret;
>
> 	return readl...
>
> > +
> >  }
>
> Otherwise LGTM
>
> Reviewed-by: Miquel Raynal <miquel.raynal@bootlin.com>
>
> Thanks,
> Miquèl

[PATCH v3 11/11] i3c: master: svc: fix possible assignment of the same address to two devices

[Frank Li]

svc_i3c_master_do_daa() {
    ...
    for (i = 0; i < dev_nb; i++) {
        ret = i3c_master_add_i3c_dev_locked(m, addrs[i]);
        if (ret)
            goto rpm_out;
    }
}

If two devices (A and B) are detected in DAA and address 0xa is assigned to
device A and 0xb to device B, a failure in i3c_master_add_i3c_dev_locked()
for device A (addr: 0xa) could prevent device B (addr: 0xb) from being
registered on the bus. The I3C stack might still consider 0xb a free
address. If a subsequent Hotjoin occurs, 0xb might be assigned to Device A,
causing both devices A and B to use the same address 0xb, violating the I3C
specification.

The return value for i3c_master_add_i3c_dev_locked() should not be checked
because subsequent steps will scan the entire I3C bus, independent of
whether i3c_master_add_i3c_dev_locked() returns success.

If device A registration fails, there is still a chance to register device
B. i3c_master_add_i3c_dev_locked() can reset DAA if a failure occurs while
retrieving device information.

Cc: stable@kernel.org
Fixes: 317bacf960a4 ("i3c: master: add enable(disable) hot join in sys entry")
Signed-off-by: Frank Li <Frank.Li@nxp.com>
---
 drivers/i3c/master/svc-i3c-master.c | 7 ++-----
 1 file changed, 2 insertions(+), 5 deletions(-)

diff --git a/drivers/i3c/master/svc-i3c-master.c b/drivers/i3c/master/svc-i3c-master.c
index 2010495906eb3..003565fddc261 100644
--- a/drivers/i3c/master/svc-i3c-master.c
+++ b/drivers/i3c/master/svc-i3c-master.c
@@ -1042,11 +1042,8 @@ static int svc_i3c_master_do_daa(struct i3c_master_controller *m)
 		goto rpm_out;
 
 	/* Register all devices who participated to the core */
-	for (i = 0; i < dev_nb; i++) {
-		ret = i3c_master_add_i3c_dev_locked(m, addrs[i]);
-		if (ret)
-			goto rpm_out;
-	}
+	for (i = 0; i < dev_nb; i++)
+		i3c_master_add_i3c_dev_locked(m, addrs[i]);
 
 	/* Configure IBI auto-rules */
 	ret = svc_i3c_update_ibirules(master);

-- 
2.34.1

Re: [PATCH v3 11/11] i3c: master: svc: fix possible assignment of the same address to two devices

[Miquel Raynal]

Hi Frank,

Frank.Li@nxp.com wrote on Mon, 19 Aug 2024 12:02:05 -0400:

> svc_i3c_master_do_daa() {
>     ...
>     for (i = 0; i < dev_nb; i++) {
>         ret = i3c_master_add_i3c_dev_locked(m, addrs[i]);
>         if (ret)
>             goto rpm_out;
>     }
> }
> 
> If two devices (A and B) are detected in DAA and address 0xa is assigned to
> device A and 0xb to device B, a failure in i3c_master_add_i3c_dev_locked()
> for device A (addr: 0xa) could prevent device B (addr: 0xb) from being
> registered on the bus. The I3C stack might still consider 0xb a free
> address. If a subsequent Hotjoin occurs, 0xb might be assigned to Device A,
> causing both devices A and B to use the same address 0xb, violating the I3C
> specification.
> 
> The return value for i3c_master_add_i3c_dev_locked() should not be checked
> because subsequent steps will scan the entire I3C bus, independent of
> whether i3c_master_add_i3c_dev_locked() returns success.
> 
> If device A registration fails, there is still a chance to register device
> B. i3c_master_add_i3c_dev_locked() can reset DAA if a failure occurs while
> retrieving device information.
> 
> Cc: stable@kernel.org
> Fixes: 317bacf960a4 ("i3c: master: add enable(disable) hot join in sys entry")
> Signed-off-by: Frank Li <Frank.Li@nxp.com>
> ---
>  drivers/i3c/master/svc-i3c-master.c | 7 ++-----
>  1 file changed, 2 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/i3c/master/svc-i3c-master.c b/drivers/i3c/master/svc-i3c-master.c
> index 2010495906eb3..003565fddc261 100644
> --- a/drivers/i3c/master/svc-i3c-master.c
> +++ b/drivers/i3c/master/svc-i3c-master.c
> @@ -1042,11 +1042,8 @@ static int svc_i3c_master_do_daa(struct i3c_master_controller *m)
>  		goto rpm_out;
>  
>  	/* Register all devices who participated to the core */
> -	for (i = 0; i < dev_nb; i++) {
> -		ret = i3c_master_add_i3c_dev_locked(m, addrs[i]);
> -		if (ret)
> -			goto rpm_out;
> -	}
> +	for (i = 0; i < dev_nb; i++)
> +		i3c_master_add_i3c_dev_locked(m, addrs[i]);

Makes sense, but please explain why don't check the return value in a
comment (your commit log is good).

>  
>  	/* Configure IBI auto-rules */
>  	ret = svc_i3c_update_ibirules(master);
> 

With the comment added,

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

Thanks,
Miquèl