*Re: [PATCH v4 3/6] i3c: Add support for mastership request to I3C subsystem
2019-04-10 10:05 ` Vitor Soares@ 2019-04-10 10:36 ` Boris Brezillon
2019-04-12 14:28 ` Vitor Soares0 siblings, 1 reply; 34+ messages in thread
From: Boris Brezillon @ 2019-04-10 10:36 UTC (permalink / raw)
To: Vitor Soares; +Cc: Przemyslaw Gaj, bbrezillon, linux-i3c, rafalc, agolec
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, ...).
>
> >
> > >
> > > >
> > > > >
> > > > > > > + 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.
>
> >
> > > >
> > > > > 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 :-)).
>
> 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.
_______________________________________________
linux-i3c mailing list
linux-i3c@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-i3c^permalinkrawreply [flat|nested] 34+ messages in thread

*RE: [PATCH v4 3/6] i3c: Add support for mastership request to I3C subsystem
2019-04-10 10:36 ` Boris Brezillon@ 2019-04-12 14:28 ` Vitor Soares
2019-04-12 14:57 ` Boris Brezillon0 siblings, 1 reply; 34+ messages in thread
From: Vitor Soares @ 2019-04-12 14:28 UTC (permalink / raw)
To: Boris Brezillon, Vitor Soares
Cc: Przemyslaw Gaj, bbrezillon, linux-i3c, rafalc, agolec
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^permalinkrawreply [flat|nested] 34+ messages in thread

*Re: [PATCH v4 3/6] i3c: Add support for mastership request to I3C subsystem
2019-04-12 14:28 ` Vitor Soares@ 2019-04-12 14:57 ` Boris Brezillon
2019-04-15 12:02 ` Vitor Soares0 siblings, 1 reply; 34+ messages in thread
From: Boris Brezillon @ 2019-04-12 14:57 UTC (permalink / raw)
To: Vitor Soares; +Cc: Przemyslaw Gaj, bbrezillon, linux-i3c, rafalc, agolec
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?
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).
>
> 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.
> > > > > > > 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.
> 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. 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.
_______________________________________________
linux-i3c mailing list
linux-i3c@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-i3c^permalinkrawreply [flat|nested] 34+ messages in thread

*RE: [PATCH v4 3/6] i3c: Add support for mastership request to I3C subsystem
2019-04-12 14:57 ` Boris Brezillon@ 2019-04-15 12:02 ` Vitor Soares
2019-04-15 13:08 ` Boris Brezillon0 siblings, 1 reply; 34+ messages in thread
From: Vitor Soares @ 2019-04-15 12:02 UTC (permalink / raw)
To: Boris Brezillon, Vitor Soares
Cc: Przemyslaw Gaj, bbrezillon, linux-i3c, rafalc, agolec
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^permalinkrawreply [flat|nested] 34+ messages in thread

*Re: [PATCH v4 3/6] i3c: Add support for mastership request to I3C subsystem
2019-04-15 12:02 ` Vitor Soares@ 2019-04-15 13:08 ` Boris Brezillon0 siblings, 0 replies; 34+ messages in thread
From: Boris Brezillon @ 2019-04-15 13:08 UTC (permalink / raw)
To: Vitor Soares; +Cc: Przemyslaw Gaj, bbrezillon, linux-i3c, rafalc, agolec
Hi Vitor,
On Mon, 15 Apr 2019 12:02:02 +0000
Vitor Soares <vitor.soares@synopsys.com> wrote:
> 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.
Attached to a bus_type, maybe, under an I3C bus object, I'm not
convinced.
> 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.
Yes, AFAICT, the slave status (and potentially the I3C dynamic address)
are the only thing an I3C slave controller driver might need.
>
> The I2C subsystem implement that and it is working.
But is it needed? That's what I'm questioning here. What would you put
in the bus object if the only thing you know about it is that the slave
controller is present on the bus. Does it make sense to re-use the same
logic/struct for the slave case?
Maybe my feeling about this problem is wrong, but as usual, I don't see
a real use case expressed or a PoC that shows me why you'd need to have
an i3c_bus attached to the slave. So it's hard to say who's right/wrong.
If you come up with an RFC for the slave API we'll have something
concrete to discuss about. Until this happens, I'd like to close this
topic. And of course, I'd appreciate that we keep focusing on the
mastership handover/secondary master case without thinking too much
about how things will be impacted by the slave API.
>
> I still don't understand your position, is there any other reasons?
My position is: I don't see the point of adding complex concepts to
something that's otherwise way simpler than managing an I3C bus. You can
prove me wrong with a detailed example, or even better, a patch series
adding the slave API plus a reference driver to show how it's supposed
to work :P.
>
> > 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)
Yes, but that case falls in the I3C master API.
> - 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.
We need to share a few things (CCC defs and probably other things,
but I don't know which ones yet), yes, but I don't think the i3c_bus
struct is one of them.
I also don't know what the slave API will look like, and that's
actually something someone (you??) should work on before having
advanced discussions about what has to be shared between the
master/slave layers.
>
> >
> > > > > > > > > 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?
This is exactly what I recommend you to do: provide an helper that will
automate that, but don't force other drivers to use this helper. Since
the Cadence driver will not use this helper, it should not be
introduced in this patch series, that's all I said.
> 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?
There's a ->cur_master field in struct i3c_bus. It should be updated
when the bus owner changes. Not sure exactly how other masters can be
informed about that, but those doing the handshake know about the
change, that's for sure.
>
> >
> > > 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?
I keep explaining you that reconstructing a fake DEFSLVS frame just for
the sake of having all drivers use the helper you want to add is not a
good idea.
To sum-up one last time: adding the
i3c_master_parse_defslvs_and_add_devs() helper is fine, forcing all
drivers to use it is not. Is that clear enough?
>
> > 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.
So you're back to the master/slave dual role thing?
>
> The implementation should made with this in mind and I'm asking
> flexibility for that.
What you're asking for is the opposite of flexibility, and most
importantly it's based on assumptions about something that has not been
fully designed yet: the I3C slave API.
So let's have a plan for this discussion to actually lead to something
useful:
1/ propose an RFC for the slave API + a reference driver
2/ think about how dual role devs can implement both interfaces and the
things they need to share
See, we keep discussing #2 while #1 is not even ready yet, and it's
definitely a pre-requisite to #2. So please work #1 first.
Regards,
Boris
_______________________________________________
linux-i3c mailing list
linux-i3c@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-i3c^permalinkrawreply [flat|nested] 34+ messages in thread

*Re: [PATCH v4 4/6] i3c: master: cdns: add support for mastership request to Cadence I3C master driver.
2019-03-30 15:44 ` Boris Brezillon@ 2019-04-29 10:36 ` Przemyslaw Gaj
2019-05-18 7:34 ` Boris Brezillon0 siblings, 1 reply; 34+ messages in thread
From: Przemyslaw Gaj @ 2019-04-29 10:36 UTC (permalink / raw)
To: Boris Brezillon; +Cc: linux-i3c, vitor.soares, rafalc, agolec, bbrezillon
Hi Boris,
I'm sorry for my late response. I hope you remember this thread :-)
I'm implementing this and have some questions.
The 03/30/2019 16:44, Boris Brezillon wrote:
> >
> > @@ -1274,9 +1353,32 @@ static int cdns_i3c_master_bus_init(struct i3c_master_controller *m)
> >
> > cdns_i3c_master_enable(master);
> >
> > + if (m->secondary) {
> > + i3c_bus_maintenance_lock(&master->base.bus);
> > + cdns_i3c_master_update_devs(&master->base);
> > + i3c_bus_maintenance_unlock(&master->base.bus);
> > + }
>
> Okay, I changed my mind on this solution (it's not the first time that
> happens, and unfortunately won't be the last time either :-)). I think
> I don't like the idea of exposing the i3c_bus_maintenance_unlock/lock()
> functions in the end.
Ok :-)
>
> I'd like to reconsider what you initially proposed: having an
> ->update_devs() hook that is called by the core, except I'd call it
> ->populate_bus().
Ok, we can back to previous approach.
>
> BTW, there's still something that's unclear to me. You seem to populate
> the bus here and also when acquiring bus ownership. Is this correct?
Yes, this is correct. I'm doing this here to register all the devices received
by DEFSLVS on master initialization time. I'm also populating new devices when
acquiring the bus because some device could join the bus dynamically and we
want to register this new devices on our side also.
> I'd expect it to be 'partially' populated at bus-init+defslvs time,
> which would trigger a mastership request if I3C devices are present (as
> we need to send a GETPID to know more about I3C devs).
So, you want to allocate and attach devices and then, when possible get devices
info and register them? I mean when mastership request is possible. If not,
just leave devices allocated and register them when ENEC(MR) received, correct?
Previously, I allocated and registered all the devices after successful
mastership request. Which way is better in your opinion?
>
> Also, what happens if i3c_master_add_i3c_dev_locked() fails? You
> don't seem to handle that case at all.
For now, I just skipped it silently.
>
> > +
> > +static void cdns_i3c_master_mastership_takeover(struct cdns_i3c_master *master)
> > +{
> > + if (master->base.init_done) {
>
> Can this really happen that init_done is not set when you reach this
> point.
Yes, it was possible. Mastership was taken but master wasn't registered yet.
With new approach I think this won't happen.
>
> > + i3c_bus_maintenance_lock(&master->base.bus);
> > + cdns_i3c_master_update_devs(&master->base);
> > + i3c_bus_maintenance_unlock(&master->base.bus);
> > +
> > + i3c_master_register_new_devs(&master->base);
>
> The core will somehow be informed that this master now owns the bus, so
> it can call i3c_master_register_new_devs() for us, right?
I think it can. I'm sure it worked like that before. When HC driver changed
cur_master, new devices were populated.
>
> But as said above, I'm not even sure this is correct to do it from
> here. I'd expect this to happen at DEFSLVS or BUS init time.
>
Ok. New(Previous) approach allows that.
--
--
Przemyslaw Gaj
_______________________________________________
linux-i3c mailing list
linux-i3c@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-i3c^permalinkrawreply [flat|nested] 34+ messages in thread

*Re: [PATCH v4 4/6] i3c: master: cdns: add support for mastership request to Cadence I3C master driver.
2019-04-29 10:36 ` Przemyslaw Gaj@ 2019-05-18 7:34 ` Boris Brezillon0 siblings, 0 replies; 34+ messages in thread
From: Boris Brezillon @ 2019-05-18 7:34 UTC (permalink / raw)
To: Przemyslaw Gaj; +Cc: linux-i3c, vitor.soares, rafalc, agolec, bbrezillon
On Mon, 29 Apr 2019 11:36:42 +0100
Przemyslaw Gaj <pgaj@cadence.com> wrote:
> Hi Boris,
>
> I'm sorry for my late response. I hope you remember this thread :-)
Unfortunately not :-/, and it's now my turn to apologize for the late
reply.
> I'm implementing this and have some questions.
>
> The 03/30/2019 16:44, Boris Brezillon wrote:
> > >
> > > @@ -1274,9 +1353,32 @@ static int cdns_i3c_master_bus_init(struct i3c_master_controller *m)
> > >
> > > cdns_i3c_master_enable(master);
> > >
> > > + if (m->secondary) {
> > > + i3c_bus_maintenance_lock(&master->base.bus);
> > > + cdns_i3c_master_update_devs(&master->base);
> > > + i3c_bus_maintenance_unlock(&master->base.bus);
> > > + }
> >
> > Okay, I changed my mind on this solution (it's not the first time that
> > happens, and unfortunately won't be the last time either :-)). I think
> > I don't like the idea of exposing the i3c_bus_maintenance_unlock/lock()
> > functions in the end.
>
> Ok :-)
>
> >
> > I'd like to reconsider what you initially proposed: having an
> > ->update_devs() hook that is called by the core, except I'd call it
> > ->populate_bus().
>
> Ok, we can back to previous approach.
>
> >
> > BTW, there's still something that's unclear to me. You seem to populate
> > the bus here and also when acquiring bus ownership. Is this correct?
>
> Yes, this is correct. I'm doing this here to register all the devices received
> by DEFSLVS on master initialization time. I'm also populating new devices when
> acquiring the bus because some device could join the bus dynamically and we
> want to register this new devices on our side also.
Hm, I don't get that part. I thought we were supposed to add devices as
soon as we know about them. In case of HJ and assuming your master is
not currently owning the bus, the active master should send you a
DEFSLVS which should serve as a trigger for registering new devs on all
non active masters. Since registering new devices will trigger a
mastership handover to query devs info (PID + other stuff) we should be
all good, right?
>
> > I'd expect it to be 'partially' populated at bus-init+defslvs time,
> > which would trigger a mastership request if I3C devices are present (as
> > we need to send a GETPID to know more about I3C devs).
>
> So, you want to allocate and attach devices and then, when possible get devices
> info and register them? I mean when mastership request is possible. If not,
> just leave devices allocated and register them when ENEC(MR) received, correct?
Kind of, yes. We can probably just have a "want_to_acquire_bus" flag
set, and the partially registered/discovered devices present in the
master list would be registered automatically every time the master
acquires bus ownership. This way we can re-use this logic for any
operation that requires the master to own the bus to do something
specific.
>
> Previously, I allocated and registered all the devices after successful
> mastership request. Which way is better in your opinion?
That's a solution too, but it feels like a lot of generic code is
open-coded in the driver if we do it this way. I know I'm the one who
suggested this approach, but now that I see the code, I realize I was
wrong (sorry about that).
>
> >
> > Also, what happens if i3c_master_add_i3c_dev_locked() fails? You
> > don't seem to handle that case at all.
>
> For now, I just skipped it silently.
We should at least print an error/warning.
>
> >
> > > +
> > > +static void cdns_i3c_master_mastership_takeover(struct cdns_i3c_master *master)
> > > +{
> > > + if (master->base.init_done) {
> >
> > Can this really happen that init_done is not set when you reach this
> > point.
>
> Yes, it was possible. Mastership was taken but master wasn't registered yet.
> With new approach I think this won't happen.
One more reason to switch to the new approach.
>
> >
> > > + i3c_bus_maintenance_lock(&master->base.bus);
> > > + cdns_i3c_master_update_devs(&master->base);
> > > + i3c_bus_maintenance_unlock(&master->base.bus);
> > > +
> > > + i3c_master_register_new_devs(&master->base);
> >
> > The core will somehow be informed that this master now owns the bus, so
> > it can call i3c_master_register_new_devs() for us, right?
>
> I think it can. I'm sure it worked like that before. When HC driver changed
> cur_master, new devices were populated.
>
> >
> > But as said above, I'm not even sure this is correct to do it from
> > here. I'd expect this to happen at DEFSLVS or BUS init time.
> >
>
> Ok. New(Previous) approach allows that.
>
Ok, good.
_______________________________________________
linux-i3c mailing list
linux-i3c@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-i3c^permalinkrawreply [flat|nested] 34+ messages in thread