Re: [PATCH v2 10/11] staging: typec: fusb302: Hook up mux support using tcpc_gen_mux support

[Hans de Goede]

Hi,

On 13-09-17 15:38, Rob Herring wrote:
> On Wed, Sep 13, 2017 at 3:56 AM, Hans de Goede <hdegoede@xxxxxxxxxx> wrote:
> > Hi,
> >
> >
> > On 13-09-17 00:20, Rob Herring wrote:
> > > On Tue, Sep 05, 2017 at 06:42:20PM +0200, Hans de Goede wrote:
> > > > Add mux support to the fusb302 driver, call devm_tcpc_gen_mux_create()
> > > > to let the generic tcpc_mux_dev code create a tcpc_mux_dev for us.
> > > >
> > > > Also document the mux-names used by the generic tcpc_mux_dev code in
> > > > our devicetree bindings.
> > > >
> > > > Cc: Rob Herring <robh+dt@xxxxxxxxxx>
> > > > Cc: Mark Rutland <mark.rutland@xxxxxxx>
> > > > Cc: devicetree@xxxxxxxxxxxxxxx
> > > > Signed-off-by: Hans de Goede <hdegoede@xxxxxxxxxx>
> > > > ---
> > > >    Documentation/devicetree/bindings/usb/fcs,fusb302.txt |  3 +++
> > > >    drivers/staging/typec/fusb302/fusb302.c               | 11 ++++++++++-
> > > >    2 files changed, 13 insertions(+), 1 deletion(-)
> > > >
> > > > diff --git a/Documentation/devicetree/bindings/usb/fcs,fusb302.txt
> > > > b/Documentation/devicetree/bindings/usb/fcs,fusb302.txt
> > > > index 472facfa5a71..63d639eadacd 100644
> > > > --- a/Documentation/devicetree/bindings/usb/fcs,fusb302.txt
> > > > +++ b/Documentation/devicetree/bindings/usb/fcs,fusb302.txt
> > > > @@ -6,6 +6,9 @@ Required properties :
> > > >    - interrupts             : Interrupt specifier
> > > >      Optional properties :
> > > > +- mux-controls           : List of mux-ctrl-specifiers containing 1 or 2
> > > > muxes
> > > > +- mux-names              : "type-c-mode-mux" when using 1 mux, or
> > > > +                           "type-c-mode-mux", "usb-role-mux" when using
> > > > 2 muxes
> > >
> > >
> > > I'm not sure this is the right place for this. The mux is outside the
> > > FUSB302. In a type-C connector node or USB phy node would make more
> > > sense to me.
> >
> >
> > The mux certainly does not belong in the USB phy node, it sits between the
> > USB PHY
> > and the Type-C connector and can for example also mux the Type-C pins to a
> > Display
> > Port PHY.
>
> Thinking about this some more, the mux(es) should be its own node(s).
> Then the question is where to put the nodes.

Right, the mux will be its own node per Documentation/devicetree/bindings/mux/mux-controller.txt
the bindings bit this patch is adding is only adding phandles pointing
to that mux-node as the fusb302 "consumes" the mux functionality.

So as such (the fusb302 is the component which should control the mux)
I do believe that the bindings this patch adds are correct.

> > As for putting it in a type-C connector node, currently we do not have such
> > a node,
>
> Well, you should. Type-C connectors are certainly complicated enough
> that we'll need one. Plus we already require connector nodes for
> display outputs, so what do we do once you add display muxing?

An interesting question, I'm working on this for x86 + ACPI boards actually,
not a board using DT I've been adding DT bindings docs for device-properties
I use because that seems like the right thing to do where the binding is obvious
(which I believe it is in this case as the fusb302 is the mux consumer) and
because the device-property code should work the same on x86 + ACPI
(where some platform-specific drivers attach the device properties) and
on e.g. ARM + DT.

The rest should probably be left to be figured out when an actual DT
using device using the fusb302 or another Type-C controller shows up.

> > the closest thing we do have to a node describing it is actually the fusb302
> > node
> > itself. E.g. it may also contain a regulator to turn Vbus on / off (already
> > there
> > in the code, but I forgot to document this when I added the missing DT
> > bindings
> > doc for the fusb302 with a previous patch).
>
> Either you can have a vbus-supply property in connector node or it can
> be implied that the controller chip provides that. For example, HDMI
> connectors have a hpd-gpios property if HPD is connected to GPIO or
> they have nothing and it's implicit that the HDMI encoder handles HPD.
>
>
> > Also these properties:
> >
> > > >    - fcs,max-sink-microvolt : Maximum voltage to negotiate when configured
> > > > as sink
> > > >    - fcs,max-sink-microamp  : Maximum current to negotiate when configured
> > > > as sink
> > > >    - fcs,max-sink-microwatt : Maximum power to negotiate when configured
> > > > as sink
> >
> >
> > Have more to do with the charger-IC used (which determines the limits) then
> > with
> > the fusb302 itself, but the fusb302 needs to know these as it negotiates the
> > limits.
>
> Those should probably be elsewhere and not be fusb302 specific. I did
> ack that, but it was a single node for a single component which is
> fine. But once we start adding more external pieces we need to pay
> more attention to the overall structure.
>
> > Likewise the fusb302 negotiates how the data pins will be used and thus to
> > which pins
> > on the SoC the mux should mux the data pins.
> >
> > TL;DR: The fusb302 does all the negotiation and ties all the Type-C
> > connected
> > ICs together, so this seems like the right place for it (it certainly is the
> > natural place to put these from a driver code pov).
>
> Things in DT should follow what the h/w design looks like which is not
> necessarily aligned with the driver structure. If the USB PD chip
> needs information from the charger, then we need a kernel interface
> for that.

Well this really is board specific data, the charger IC may be handle
for example up to 17V, but if the board is only designed for 12V then
we should only negotiate up to 12V because the board may have voltage
overprotection circuitry which trips at say 14V. This is actually the
case with the 2 x86 boards with a Type-C connector and fusb302 Type-C
controller I have, the charger on there can handle upto 17V according
to its datasheet but Windows never negotiates more then 12V, even when
attaching a Type-C charger which can do 5V, 9V or 14V, Windows choses 9V.

So it is the Type-C controller which does the negotiating and
the limits may be stricter then the maximum ratings of the
charger IC, so I guess my earlier remark of this coming from the
charger IC was not accurate and having this info in the Type-C controller
node is the right thing to do, sorry about that.

> My concern here is not so much this binding in particular, but rather
> that we handle Type-C connectors in a common way and not adhoc with
> each platform doing things their own way. Otherwise, we end up with a
> mess of platform specific bindings like charger/battery bindings
> (though there's some work improving those).

I understand, but see my remark about me working on this on
X86 / ACPI boards. One advantage of this, is that the device-properties
are being set by platform drivers living under drivers/platform/x86,
so if the need arises they can be changed without breaking any ABI as
in my use-case they are 100% kernel internal stuff.

Regards,

Hans