Linux-i3c Archive on lore.kernel.org
 help / color / Atom feed
From: Vitor Soares <vitor.soares@synopsys.com>
To: Boris Brezillon <boris.brezillon@collabora.com>,
	Vitor Soares <vitor.soares@synopsys.com>
Cc: Przemyslaw Gaj <pgaj@cadence.com>,
	"bbrezillon@kernel.org" <bbrezillon@kernel.org>,
	"linux-i3c@lists.infradead.org" <linux-i3c@lists.infradead.org>,
	"rafalc@cadence.com" <rafalc@cadence.com>,
	"agolec@cadence.com" <agolec@cadence.com>
Subject: RE: [PATCH v4 3/6] i3c: Add support for mastership request to I3C subsystem
Date: Fri, 12 Apr 2019 14:28:26 +0000
Message-ID: <13D59CF9CEBAF94592A12E8AE55501350A614581@DE02WEMBXB.internal.synopsys.com> (raw)
In-Reply-To: <20190410123603.0ef10c19@collabora.com>

From: Boris Brezillon <boris.brezillon@collabora.com>
Date: Wed, Apr 10, 2019 at 11:36:03

> On Wed, 10 Apr 2019 10:05:33 +0000
> Vitor Soares <vitor.soares@synopsys.com> wrote:
> 
> 
> > > > > >   
> > > > > > > > @@ -2504,9 +2745,11 @@ int i3c_master_register(struct   
> > > > > i3c_master_controller   
> > > > > > > *master,  
> > > > > > > >  	 * register I3C devices dicovered during the initial DAA.
> > > > > > > >  	 */
> > > > > > > >  	master->init_done = true;
> > > > > > > > -	i3c_bus_normaluse_lock(&master->bus);
> > > > > > > > -	i3c_master_register_new_i3c_devs(master);
> > > > > > > > -	i3c_bus_normaluse_unlock(&master->bus);
> > > > > > > > +	i3c_master_register_new_devs(master);
> > > > > > > > +
> > > > > > > > +	i3c_bus_maintenance_lock(&master->bus);
> > > > > > > > +	i3c_master_enable_mr_events(master);  
> > > > > > 
> > > > > > Why are you enabling MR events here? As a standard function this might case   
> > > > > issues  
> > > > > > because the master can support ENEC/DISEC commands but not multi-master   
> > > > > typologies.
> > > > > 
> > > > > I enable it at the end of master registration to be sure that current master 
> > > > > is
> > > > > ready for bus mastership relinquish. If controller does not support secondary
> > > > > master role it can just do nothing with it.  
> > > > 
> > > > I think it isn't good idea to have this here because you are forcing HC driver to
> > > > verify it. If it is to be done in subsystem side probably it is better to have a
> > > > master/slave functionalities structure.  
> > > 
> > > If multi-master topology is not supported, request_mastership() hook won't be
> > > implemented and also you will not implement enable/disable_mr_events() hooks.
> > > You don't have to verify it in the driver.  
> > 
> > In that case we will need a driver for each role/set of available 
> > features and duplicate the code otherwise this need to be checked in HC 
> > side.
> 
> Not really, just 2 set of ops, one with the hooks set and another one
> with the hooks left unassigned (NULL). But I guess we can also add
> a caps field where we'd list i3c master caps (secondary master,
> supports MR, ...).

Yes we can do this, but what I see is to have several ops structures 
based on HC capabilities.

> 
> > 
> > >   
> > > >   
> > > > >   
> > > > > >   
> > > > > > > > +	i3c_bus_maintenance_unlock(&master->bus);
> > > > > > > >  
> > > > > > > >  	return 0;
> > > > > > > >  
> > > > > > > > @@ -2524,6 +2767,30 @@ int i3c_master_register(struct   
> > > > > i3c_master_controller   
> > > > > > > *master,  
> > > > > > > >  EXPORT_SYMBOL_GPL(i3c_master_register);
> > > > > > > >    
> > > > > > 
> > > > > > I'm thinking if isn't better to instantiate the bus apart and them register   
> > > > > the secondary master.
> > > > > 
> > > > > It looked like that before but we decided to register everything or nothing.  
> > > > 
> > > > I see your point, but for the future, slave implementation, we can have a
> > > > function to instantiate just the bus and another for master or slave.
> > > >   
> > > 
> > > We can go the same way with slave, and register bus and slave at once. If this
> > > is the case.  
> 
> The slave framework isn't there yet, and I don't think we should expose
> the bus concept to slave drivers anyway. If a master can act both as a
> slave (I mean a slave that can do more than just send MR requests)  and
> a master (secondary), the driver should register to both framework.
> 
> > 
> > What if the slave is a secondary master? In your opinion what should be 
> > the flow?
> 
> i3c_slave_regiter(&ctrl->slave);
> i3c_master_regiter(&ctrl->master);
> 
> The order is arbitrary, and those might actually be called from
> different path if it makes more sense. I'm just pointing out that
> registering a slave and registering a master are 2 different things,
> and the master side of the framework should probably not automate slave
> registration, at least not until we have a better idea of what the
> slave API will look like.

We need to understand better your point.

The secondary master can have both roles and even not implementing all 
slave function it is a slave when is not a master.
For me still keep the idea:

        bus
         / \
       /     \
slave    master

and just one role is active at time.

I didn't get why the bus shouldn't be instantiated for slave. Can you 
explain?

In any case we will need a common point to switch the roles.

> 
> > 
> > >   
> > > > >   
> > > > > > In this way you are able to add DEFSLVS even before the HC has enable MR   
> > > > > events like it is done  
> > > > > > with dt devices.  
> > > > > 
> > > > > I don't get it. What do you mean "add DEFSLVS"? DEFSLVS should be received 
> > > > > from
> > > > > current master right after ENTDAA. Could you please explain?  
> > > > 
> > > > So the current master receive an hot join and send the DEFSLVS, when do you
> > > > add them to the bus? Will you go for the all process to get all dev info and so on?
> > > >   
> > > 
> > > When current master receives an hot-join, do ENTDAA to enumerate device which
> > > joined the bus and then sends DEFSLVS. This data is stored in secondary master
> > > controller and if secondary master can request mastership, collects all
> > > required informations and adds the devices. It has to collect PID, DEFSLVS does
> > > not provide this information.  
> > 
> > I see now. You need to take care here because when the secondary master 
> > add the i3c_dev it might change the address.
> > This is one of the reasons I would prefer a dedicated function to add the 
> > DEFSLVS.
> 
> IIRC, Cadence IP is parsing DEFSLVS in HW, and the device table is
> automatically filled based on that. We should only add the DEFSLVS
> parsing helper once we start seeing a need for it (probably when adding
> secondary slave support since you seem to insist on this aspect :-)).

For me the subsystem should hold and handle all this information. Anyway, 
this information is received and needed before the mastership takeover.
Basically I want to avoid to put this type of logistics in HC driver 
layer and call the maintenance_lock.

I'm insisting because it seems that I have a different use case to 
address and I don't see how this fit on it.

> 
> > 
> > Another point is what happen if the current master received MR request 
> > during this registration?
> 
> You mean when registering the primary I3C master? The driver should
> take care of disabling MR interrupts and if possible reject all
> incoming MR. MR should only be re-enabled when ->enable_mr_events() is
> called by the core.

I'm aware of that I just didn't see any disable events.


Best regards,
Vitor Soares

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

  reply index

Thread overview: 34+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-03-10 13:58 [PATCH v4 0/6] Add the I3C mastership request Przemyslaw Gaj
2019-03-10 13:58 ` [PATCH v4 1/6] i3c: add addr and lvr to i2c_dev_desc structure Przemyslaw Gaj
2019-03-30 14:36   ` Boris Brezillon
2019-04-01 18:17     ` vitor
2019-04-01 18:31       ` Boris Brezillon
2019-04-01 18:48         ` vitor
2019-04-01 19:10           ` Przemyslaw Gaj
2019-04-01 19:24           ` Boris Brezillon
2019-04-02 11:10             ` vitor
2019-04-02 11:22               ` Przemyslaw Gaj
2019-04-02 11:32                 ` vitor
2019-04-02 11:53                   ` Boris Brezillon
2019-04-02 11:48     ` Boris Brezillon
2019-03-10 13:58 ` [PATCH v4 2/6] i3c: export bus maintenance lock and unlock functions Przemyslaw Gaj
2019-03-10 13:58 ` [PATCH v4 3/6] i3c: Add support for mastership request to I3C subsystem Przemyslaw Gaj
2019-03-30 15:06   ` Boris Brezillon
2019-04-01 18:41   ` vitor
2019-04-01 19:18     ` Boris Brezillon
2019-04-09 14:31     ` Vitor Soares
2019-04-09 15:20       ` Przemyslaw Gaj
2019-04-09 15:46         ` Vitor Soares
2019-04-10  6:53           ` Przemyslaw Gaj
2019-04-10 10:05             ` Vitor Soares
2019-04-10 10:36               ` Boris Brezillon
2019-04-12 14:28                 ` Vitor Soares [this message]
2019-04-12 14:57                   ` Boris Brezillon
2019-04-15 12:02                     ` Vitor Soares
2019-04-15 13:08                       ` Boris Brezillon
2019-03-10 13:58 ` [PATCH v4 4/6] i3c: master: cdns: add support for mastership request to Cadence I3C master driver Przemyslaw Gaj
2019-03-30 15:44   ` Boris Brezillon
2019-04-29 10:36     ` Przemyslaw Gaj
2019-05-18  7:34       ` Boris Brezillon
2019-03-10 13:58 ` [PATCH v4 5/6] i3c: master: Add module author Przemyslaw Gaj
2019-03-10 13:58 ` [PATCH v4 6/6] MAINTAINERS: add myself as co-maintainer of i3c subsystem Przemyslaw Gaj

Reply instructions:

You may reply publically to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=13D59CF9CEBAF94592A12E8AE55501350A614581@DE02WEMBXB.internal.synopsys.com \
    --to=vitor.soares@synopsys.com \
    --cc=agolec@cadence.com \
    --cc=bbrezillon@kernel.org \
    --cc=boris.brezillon@collabora.com \
    --cc=linux-i3c@lists.infradead.org \
    --cc=pgaj@cadence.com \
    --cc=rafalc@cadence.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

Linux-i3c Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-i3c/0 linux-i3c/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-i3c linux-i3c/ https://lore.kernel.org/linux-i3c \
		linux-i3c@lists.infradead.org
	public-inbox-index linux-i3c

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.infradead.lists.linux-i3c


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git