Linux-i3c Archive on lore.kernel.org
 help / color / Atom feed
From: Vitor Soares <vitor.soares@synopsys.com>
To: Przemyslaw Gaj <pgaj@cadence.com>,
	Vitor Soares <vitor.soares@synopsys.com>
Cc: "linux-i3c@lists.infradead.org" <linux-i3c@lists.infradead.org>,
	"agolec@cadence.com" <agolec@cadence.com>,
	"rafalc@cadence.com" <rafalc@cadence.com>,
	"bbrezillon@kernel.org" <bbrezillon@kernel.org>
Subject: RE: [PATCH v4 3/6] i3c: Add support for mastership request to I3C subsystem
Date: Wed, 10 Apr 2019 10:05:33 +0000
Message-ID: <13D59CF9CEBAF94592A12E8AE55501350A6131C9@DE02WEMBXB.internal.synopsys.com> (raw)
In-Reply-To: <20190410065339.GB8934@global.cadence.com>

Hi Przemek,

From: Przemyslaw Gaj <pgaj@cadence.com>
Date: Wed, Apr 10, 2019 at 07:53:40

> Hi Vitor,
> 
> The 04/09/2019 15:46, Vitor Soares wrote:
> > 
> > Hi Przemek,
> > 
> > From: Przemyslaw Gaj <pgaj@cadence.com>
> > Date: Tue, Apr 09, 2019 at 16:20:30
> > 
> > > Hi Vitor,
> > > 
> > > The 04/09/2019 14:31, Vitor Soares wrote:
> > > > 
> > > > Hi Przemek,
> > > > 
> > > > From my analyses you do i3c_master_register for secondary master in follow 
> > > cases:
> > > > - HC has already an dynamic address and it is the owner of the bus.
> > > 
> > > Correct. It requests bus ownership before registration, if DA is assigned.
> > > 
> > > > Or 
> > > > - when it receive MR event enabled
> > > 
> > > Actually, when MR event is enabled and secondary master is not initialized 
> > > yet.
> > 
> > I didn't find where you do this verification, can you point me?
> 
> In the driver, cdns_i3c_sec_master_event_up(). I'm requesting mastership and
> registering the master when event is enabled and master is not registered yet.
> 

Sorry, I really don't see that. I assume it is some logic in the 
controller.

> > 
> > > 
> > > > 
> > > > Is this correct?
> > > > 
> > > > From: vitor <soares@synopsys.com>
> > > > Date: Mon, Apr 01, 2019 at 19:41:39
> > > > 
> > > > > Hi Przemek,
> > > > > 
> > > > > > +/**
> > > > > >   * i3c_master_register() - register an I3C master
> > > > > >   * @master: master used to send frames on the bus
> > > > > >   * @parent: the parent device (the one that provides this I3C master
> > > > > >   *	    controller)
> > > > > >   * @ops: the master controller operations
> > > > > > - * @secondary: true if you are registering a secondary master. Will 
> > > return
> > > > > > - *	       -ENOTSUPP if set to true since secondary masters are not yet
> > > > > > - *	       supported
> > > > > > + * @secondary: true if you are registering a secondary master.
> > > > > >   *
> > > > > >   * This function takes care of everything for you:
> > > > > >   *
> > > > > > @@ -2427,10 +2672,6 @@ int i3c_master_register(struct 
> > > i3c_master_controller 
> > > > > *master,
> > > > > >  	struct i2c_dev_boardinfo *i2cbi;
> > > > > >  	int ret;
> > > > > >  
> > > > > > -	/* We do not support secondary masters yet. */
> > > > > > -	if (secondary)
> > > > > > -		return -ENOTSUPP;
> > > > > > -
> > > > > >  	ret = i3c_master_check_ops(ops);
> > > > > >  	if (ret)
> > > > > >  		return ret;
> > > > 
> > > > The bus mode is set before the sec master does 
> > > i3c_master_add_i3c_dev_locked() and
> > > > i3c_master_add_i2c_dev() and after that it isn't updated. This can cause 
> > > troubles for
> > > > HDR-TS modes and when you have i2c devices INDEX(2) on the bus.
> > > 
> > > Yes, I see this now. It may happen before HC driver updates device list. I
> > > should update device list right before i3c_master_register().
> > > 
> > > > 
> > > > > > @@ -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.

> 
> > 
> > > 
> > > > 
> > > > > > +	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.

What if the slave is a secondary master? In your opinion what should be 
the flow?

> 
> > > 
> > > > 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.

Another point is what happen if the current master received MR request 
during this registration?

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 [this message]
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
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=13D59CF9CEBAF94592A12E8AE55501350A6131C9@DE02WEMBXB.internal.synopsys.com \
    --to=vitor.soares@synopsys.com \
    --cc=agolec@cadence.com \
    --cc=bbrezillon@kernel.org \
    --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 linux-i3c@archiver.kernel.org
	public-inbox-index linux-i3c


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