Re: [PATCH v2 1/3] i3c: Add support for mastership request to I3C subsystem

[Boris Brezillon <bbrezillon@kernel.org>]

On Mon, 28 Jan 2019 10:37:15 +0000
Przemyslaw Gaj <pgaj@cadence.com> wrote:

> The 01/15/2019 22:09, Boris Brezillon wrote:
> > EXTERNAL MAIL
> > 
> >   
> > > @@ -1727,6 +1836,13 @@ static int i3c_master_bus_init(struct i3c_master_controller *master)
> > >  	}
> > >  
> > >  	/*
> > > +	 * Don't reset addresses if this is secondary master.  
> > 
> > When is the bitmap initialization happening then? I really think  
> 
> What do you mean bitmap? Are you asking about slot statuses?

Yes.

> 
> > secondary master init should happen early on and be limited to SW-side
> > object initialization. We can have a separate function to do that, and
> > maybe a separate register function too
> > (i3c_{main,secondary}_master_register()).  
> 
> Ok, I get it. Because of lack of information at secondary master init time, I
> decided to register devices with full information. PID is blocking me. I'll try
> to separate register routines.

I was talking about master registration, not i3c devices (slaves
connected to the master) registration.

> 
> >   
> > > +	 * Secondary masters can't do DAA.
> > > +	 */
> > > +	if (master->secondary)
> > > +		return 0;
> > > +
> > > +	/*
> > >  	 * Reset all dynamic address that may have been assigned before
> > >  	 * (assigned by the bootloader for example).
> > >  	 */
> > > index f13fd8b..16e7995 100644
> > > --- a/include/linux/i3c/master.h
> > > +++ b/include/linux/i3c/master.h
> > > @@ -418,6 +418,21 @@ struct i3c_bus {
> > >   *		      for a future IBI
> > >   *		      This method is mandatory only if ->request_ibi is not
> > >   *		      NULL.
> > > + * @update_devs: updates device list. Called after bus takeover. Secondary
> > > + *               master can't perform DAA procedure. This function allows to
> > > + *               update devices received from previous bus owner in DEFSLVS
> > > + *               command. Useful also when new device joins the bus controlled
> > > + *               by secondary master, main master will be able to add
> > > + *               this device after mastership takeover.  
> > 
> > Do we really need this hook? AFAIU, this is only called from the master
> > controller's work which is responsible with mastership handover, so
> > this can be done entirely from the master driver without requiring help
> > from the framework (just need to do it with the maintenance lock held).
> > Am I missing something?
> >  
> 
> Actually, I use this hook at secondary master init time (in framework) to
> register devices which could be received by DEFSLVS command.

Hm, so you're considering the case where DEFSLVS has been received
before the secondary master was registered. Can't you just add those
drivers from the ->bus_init() implementation? I mean, you know when
you're acting as a secondary master, so it shouldn't be hard to have
one ->bus_init() per master type (secondary vs main) and in the
secondary master ->bus_init() add devices reported by the previous
DEFSLVS frame.

> I can register
> those devices in the driver. Separate secondary_master_register routine should
> help in this case.

Yes, having separate register functions should make things clearer and
allow you to customize each patch (for instance, no DAA in case of a
secondary master).

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