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 -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
