RE: [PATCH v4 3/6] i3c: Add support for mastership request to I3C subsystem

Vitor Soares
2019-04-10T10:05:42Z

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