Re: [PATCH v6 00/10] Add the I3C subsystem

[Boris Brezillon]

Hi Arnd,

On Tue, 24 Jul 2018 16:03:38 +0200
Arnd Bergmann <arnd@xxxxxxxx> wrote:

> On Fri, Jul 20, 2018 at 3:17 PM, Boris Brezillon
> <boris.brezillon@xxxxxxxxxxx> wrote:
> > On Fri, 20 Jul 2018 13:28:10 +0200 Arnd Bergmann <arnd@xxxxxxxx> wrote:
> >  
> >> On Fri, Jul 20, 2018 at 1:13 PM, Peter Rosin <peda@xxxxxxxxxx> wrote:  
> >> > On 2018-07-20 12:57, Arnd Bergmann wrote:  
> >> >> * What I understand from reading i2c-demux-pinctrl.c, a slave device
> >> >>   will only ever be observable from one master at a time, when you
> >> >>   switch over, all children get removed on one master and added to
> >> >>   the other one, to be probed again by their respective drivers.
> >> >>   I can see this as a useful feature on i3c as well, in particular to
> >> >>   deal with the situation where we have i2c slaves connected to a
> >> >>   pinmux that can switch them between an i3c master and an
> >> >>   i2c-only master (possibly a gpio based one). That particular use
> >> >>   case however doesn't seem to fix well in the current code, which
> >> >>   is structure around i3c buses.  
> >> >
> >> > It's pretty easy to come up with examples where this reprobing is
> >> > not desirable at all. E.g. if one of the involved I2C devices is
> >> > a HDMI encoder (I have a TDA19988 here) sitting in the middle of the
> >> > graphics pipeline. Blink-blink on the screen because some *other*
> >> > unrelated device needed to be accessed by an alternative master. Not
> >> > pretty.  
> >>
> >> Agreed, we definitely don't want to reprobe all devices during normal
> >> operation for i3c master handover.
> >>  
> >
> > Re-probing would not happen, no matter the solution we choose. It's
> > that, in one case, you would have X virtual/linux devices representing
> > the same physical device and in the other case, you would just have
> > one, and everytime a transfer is requested by the driver, the core
> > would pick the appropriate master to do it (most likely the one in
> > control of the bus at that time)  
> 
> I think this is one of the cases I'd want to avoid: controlling multiple
> masters that are active at the same time without going through
> the handover.

That's simply not possible, the I3C protocol forbids it. There can only
be one active master on the bus at any point in time.

> 
> If we have an actual pinmux between two masters and only one
> of them can even see the bus, I think we should go through a
> complete remove/probe cycle the way that the i2c-demux-pinctrl
> does today. If OTOH we a primary/secondary master pair with
> handover capability, I would prefer to not see one slave on
> both devices at the same time, or (ideally) only use one of the
> two masters and disable the other one completely.

Again, you don't have a choice because it's part of the protocol. At
any time, you only have one active master on the bus, and other masters
are acting as slaves until they gain bus ownership (if they ever do).
Say that device A wants to do an HDR transfer on the bus, and HDR is
only supported by master X, but master Y is currently owning the bus.
Master X will first have to request bus ownership before doing the
transfer requested by device A.

Now, imagine that device A wants to do an SDR transfer which is
supported by both master X and master Y, and master Y is in control.
Instead of requesting a bus handover, the framework would just
automatically decide to do the transfer through master Y. That's the
sort of things this separate bus/master representation allows.

> 
> >> If we find a case in which it is needed, we could still deal with it
> >> like this:
> >> - enumerate all slaves connected to the bus for each of the
> >>   two masters  
> >
> > That's what will happen if you don't share the same bus representation.  
> 
> To clarify: I meant list them in the DT representation, not enumerate
> them in Linux during boot. Sorry for using a misleading description here.

Okay.

> 
> >> - mark each slave as status="enabled" in at most one of the
> >>   buses, and as disabled everywhere else  
> >
> > We shouldn't need to do that. We can just let the driver check whether
> > the master provides the necessary capabilities to efficiently
> > communicate with the device, and if it does not just return -ENOTSUPP
> > in the ->probe() function. This way you'll have a device, but not
> > driver controlling it on one bus, and on the other bus, you'll have
> > another device (which points to the same physical device) this time
> > with a driver attached to it.  
> 
> I'd still hope that we can completely avoid that case and never
> have the case where one physical device has two live
> representations in the kernel. It /could/ still be done of course,
> but would not always do the right thing, depending on the
> type of device (a temperature sensor could just be probed
> twice without problems, a network device probably cannot)

Not really feasible if we don't share the same bus representation. So,
that means you hope we'll never have a real case where 2 masters are
connected to the same physical bus and both exposed to the same Linux
instance.

I'm still unsure what you think adds complexity in the current
approach. When I implemented it, it looked like is was almost the same
(in term of complexity) to have a bus object separated from the master,
but I'm probably missing something.

Anyway, here's what I propose. I'll work on a v7 where the bus object
is tied to the master (and not exposed in sysfs or the DT
representation) and the master itself is not represented as a device on
the bus. This way you'll have both solutions to compare them and take a
decision.

Regards,

Boris