Re: [PATCH 00/11] mux/typec: Add USB / TypeC mux drivers and hook them up on some x86 systems

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Hi Peter,

On 04-09-17 13:18, Peter Rosin wrote:
On 2017-09-01 23:48, Hans de Goede wrote:
Hi All,

This series consists of 4 parts:

1) Core mux changes to add support for getting mux-controllers on
    non DT platforms and to add some standardised state values for USB

2) Add Intel CHT USB mux and Pericom-PI3USB30532 Type-C mux drivers

3) Hookup the Intel CHT USB mux driver for CHT devices without Type-C

4) Hookup both the Intel CHT USB mux and Pericom-PI3USB30532 Type-C mux
    drivers to the fusb302 Type-C port controller found on some CHT x86 devs

Please review, or in the case of the drivers/mux changes (which are a dep
for some of the other patches) merge if the patches are ready.

Hi Hans,

I see commonalities with this and the below patch seriess from Stephen
Boyd [added to Cc].

https://lkml.org/lkml/2017/7/11/696 [PATCH 0/3] USB Mux support for Chipidea
https://lkml.org/lkml/2017/7/14/654 [PATCH v2 0/3] USB Mux support for Chipidea

Interesting, it seems that Stephen and I are trying to use the mux-framework
for identical (device/host selection) purposes, except that in some cases
I also gave a Type-C cross-switch to deal with.

I noticed this discussion in the thread:

"""
> +		usb-switch-states = <0>, <1>;

I don't see the need for usb-switch-states? Just assume states 0/1 and
if someone later needs some other states, make them add a property that
overrides the defaults. Just document that 0 is host and 1 is device.
"""

I think it makes sense to just agree on fixed "state" values for
certain use-cases, as done in my
"mux: consumer.h: Add MUX_USB_* state constant defines"
patch. This means that the "state" register in the hardware and the
state as passed to mux_control_select() may not map 1:1, but that can
easily be mapped in the driver and it allows inter-changeble (compatible)
mux drivers for some common mux use-cases such as an USB device/host
mux.

Edit: Ah I see you already suggest this below, good :)

Stephen had a patch that added mux_control_get_optional() that I think
could be of use for this series?

Ack, I think that would be useful. If you plan to merge Stephen's
patch for this, please give me a link to a git repo with that merged
then I will rebase my series on top.

Anyway, there seems to be some interest in using the mux subsystem for
handling the USB host/device role. I think the role-switch interface
between the USB and mux subsystems should be done similarly, regardless
of what particular USB driver needs to switch role using whatever
particular mux driver. If at all possible. >
The way I read it, the pi3usb30532 driver is the one adding the need to
involve the DP states and the inverted bit.

Correct, the pi3usb30532 mux is a mux designed for Type-C ports, it
switches to 4 high-speed differential data-pairs the Type-C connector
has to one on: USB-controller(s), DisplayPort outputs on the GPU, or
a combination of both (2 data-pairs to each). It also takes into account
if the Type-C connector has been inserted normally or "upside-down",
which is where the inverted bits comes into play.

Those things are not used by
anything else, and I think it clutters up things for everybody when the
weird needs of one driver "invades" the mux states needed to control the
USB role. So, I would like USB role switching to have 2 states, device
and host (0 and 1 is used by the above chipidea code). And then the USB
switch can of course be idle, which is best represented with one of
MUX_IDLE_DISCONNECT, MUX_IDLE_AS_IS or one of the modes depending on if
the USB switch can or can't disconnect (or other considerations). Now,
you can't explicitly set the idle state using mux_control_select(), it
can only be set implicitly using mux_control_deselect(), so that will
require some changes in the consumer logic.

Regarding the pi3usb30532 driver, I think the above is in fact also
better since the "swap bit" means something totally different when the
switch is "open" (if I read the datasheet correctly >
I.e. PI3USB30532_CONF_OPEN | PI3USB30532_CONF_SWAP is not sane,

The only "datasheet" I could find is "PI3USB30532-PI3USB31532_App_Type-C application Note_Rev.A.pdf"
and that says the swap bits does not do anything when the switch is "open"
but that does not matter for the rest of the discussion, I do agree
that mapping deselected to open makes sense.

and that
would just go away completely if the driver implemented MUX_IDLE_DISCONNECT
as PI3USB30532_CONF_OPEN and removed the possibility to explicitly set the
"open" state with mux_control_select().

So, I think the generic USB role switch interface should be something like:

#define MUX_USB_DEVICE		(0) /* USB device mode */
#define MUX_USB_HOST		(1) /* USB host mode */

Those 2 work for me.

And then the pi3usb30532 driver can extend that:

#define MUX_PI3USB30352_HOST_AND_DP_SRC	(2)    /* USB host + 2 lanes Display Port */
#define MUX_PI3USB30352_DP_SRC		(3)    /* 4 lanes Display Port source */
#define MUX_PI3USB30352_POLARITY_INV	BIT(2) /* Polarity inverted bit */
#define MUX_PI3USB30352_STATES		(8)

Erm, I would like to keep mux-driver specific knowledge out of the
tcpm code and there might be more Type-C mux drivers in the future,
how about:

#define MUX_TYPEC_POLARITY_INV    BIT(0)   /* Polarity inverted bit */
#define MUX_TYPEC_DEVICE          (0 << 1) /* USB device mode */
#define MUX_TYPEC_HOST            (1 << 1) /* USB host mode */
#define MUX_TYPEC_HOST_AND_DP_SRC (2 << 1) /* USB host + 2 lanes Display Port */
#define MUX_TYPEC_DP_SRC          (3 << 1) /* 4 lanes Display Port source */
#define MUX_TYPEC_STATES          (4 << 1)

?

Another idea is to expose the inverted bit as a separate mux controller,
but I suspect that you're not too keen on operating three muxes in the
tcpm driver...

That won't be pretty I think, so thanks but no thanks :)

BTW, I don't think these USB defines belong in mux/consumer.h, please add
them in a new include/linux/mux/usb.h file or something. And I'd like some
MAINTAINER entry to cover the new mux drivers...

Ok, I will fix both for the next version

I'll respond to the individual patches with nits etc, but it generally looks
tidy, thank you for that!

Thank you for all the feedback and reviews.

Regards,

Hans
_______________________________________________
devel mailing list
devel@xxxxxxxxxxxxxxxxxxxxxx
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel



[Index of Archives]     [Linux Driver Backports]     [DMA Engine]     [Linux GPIO]     [Linux SPI]     [Video for Linux]     [Linux USB Devel]     [Linux Coverity]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [Yosemite Backpacking]
  Powered by Linux