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: Mon, 15 Apr 2019 12:02:02 +0000
Message-ID: <13D59CF9CEBAF94592A12E8AE55501350A614688@DE02WEMBXB.internal.synopsys.com> (raw)
In-Reply-To: <20190412165719.0cd91421@collabora.com>


From: Boris Brezillon <boris.brezillon@collabora.com>
Date: Fri, Apr 12, 
2019 at 15:57:19

> On Fri, 12 Apr 2019 14:28:26 +0000
> Vitor Soares <vitor.soares@synopsys.com> wrote:
> 
> > > > 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?
> 
> Because, from a SW PoV, pure slave devices don't care about the bus
> concept. Do you have use cases where you'd need to know what bus the
> slave is connected to?
> 

In device model I expect that the slave is under a bus. You may not need 
to know the bus topologies and the devices connect but at least we might 
need to know the bus is alive.

The I2C subsystem implement that and it is working.

I still don't understand your position, is there any other reasons?

> AFAICT, all you can do is reply to master requests (probably with some
> predefined messages, like values stored in a regmap or data queued in
> a FIFO).

We can list it:
  - RX/TX SDR data
  - RX/TX HDR data
  - SIR
  - HJ
  - MR (case of sec master)
  - CCC 
  - and perhaps timing control.

> 
> > 
> > In any case we will need a common point to switch the roles.
> 
> We'd need a way to relinquish bus ownership, that's all. When the
> master is not the current master, it automatically becomes a slave, and
> if it has any "I3C slave" profile registered, it can reply to requests
> coming from the current master.

This is too abstract to me. Can you point a current example? Because for 
me we need a common layer for both master slave structures.

> 
> > > > > > > > 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.
> 
> It is, and we already have a bunch of helpers to add new devices. Maybe
> we need a few more, I'm just saying that forging a DEFSLVS frame to then
> pass it to the core is not the right solution IMO. 
> If you need an helper
> that automatically parses a DEFSLVS frame and add the new devices, then
> fine, add this helper to the framework and use it in your driver, but
> don't force other drivers to use this method.

This is not based in a need. I can also add the devices one by one.
If I have everything available why not send it straight away to the 
subsystem and let it do the management?
But ok, I understand your point and I can do the helper to parse the 
DEFSLVS.

Another question:
  How do we know what is the main master in case we want to send back the 
bus mastership?

> 
> > Basically I want to avoid to put this type of logistics in HC driver 
> > layer and call the maintenance_lock.
> 
> And yet, I don't think forging a DEFSLVS frame is the right way to
> provide the kind of abstraction you're talking about. 

Can you explain?

> Note that I said
> a few things should be provided as helpers in my review.
> 
> > 
> > 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.
> 
> Can you be more specific, because we don't know about your use cases.

According to the spec the sec master may have all function of a Main 
master and may have all function of a slave.

The implementation should made with this in mind and I'm asking 
flexibility for that. 

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
2019-04-12 14:57                   ` Boris Brezillon
2019-04-15 12:02                     ` Vitor Soares [this message]
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=13D59CF9CEBAF94592A12E8AE55501350A614688@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