Message-ID: <20240826184924.53b48861@xps-13>
Date: Mon, 26 Aug 2024 18:49:24 +0200
From: Miquel Raynal <miquel.raynal@...tlin.com>
To: Frank Li <Frank.li@....com>
Cc: Alexandre Belloni <alexandre.belloni@...tlin.com>, Boris Brezillon
 <boris.brezillon@...labora.com>, Parshuram Thombare <pthombar@...ence.com>,
 Greg Kroah-Hartman <gregkh@...uxfoundation.org>, Boris Brezillon
 <bbrezillon@...nel.org>, Arnd Bergmann <arnd@...db.de>, Conor Culhane
 <conor.culhane@...vaco.com>, linux-i3c@...ts.infradead.org,
 linux-kernel@...r.kernel.org, imx@...ts.linux.dev
Subject: Re: [PATCH v3 03/11] i3c: master: Extend address status bit to 4
 and add I3C_ADDR_SLOT_EXT_INIT

Hi Frank,

Frank.li@....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@....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

Powered by blists - more mailing lists