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

[Frank Li <Frank.li@nxp.com>]

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