Re: [PATCH v1 1/1] mux: mux-intel-usb: Add Intel USB Multiplexer driver

[sathyanarayanan kuppuswamy]

Hi Hans,


On 05/31/2017 05:21 AM, Hans de Goede wrote:
> Hi,
>
> On 30-05-17 20:50, Andy Shevchenko wrote:
> > On Tue, May 30, 2017 at 9:21 PM, sathyanarayanan kuppuswamy
> > <sathyanarayanan.kuppuswamy@xxxxxxxxxxxxxxx> wrote:
> > > > On Tue, May 30, 2017 at 3:47 AM,
> > > > <sathyanarayanan.kuppuswamy@xxxxxxxxxxxxxxx> wrote:
> >
> > > > > +       tristate "Intel USB Mux"
> > > >
> > > > It's indeed Intel's IP?
> > >
> > > Register map to control this MUX comes from Intel vendor defined XHCI
> > > extended cap region of SOC.
> > > > I would rather believe that it is some 3rd
> > > > party known IP block with platform specific soldering.
> > >
> > > I don't think its platform specific support. I believe its a SOC 
> > > specific
> > > thing( mainly for CHT and APL SoCs).
> >
> > Okay, the best people to give a feedback here are Heikki and Hans.
>
> Interesting, I have been working on this myself too (and posted
> a version of my driver for the mux block a while back), see:
>
> https://www.spinics.net/lists/linux-usb/msg151032.html
> https://www.spinics.net/lists/linux-usb/msg151033.html
> https://www.spinics.net/lists/linux-usb/msg151034.html
>
> I was actually planning on posting a new version of this set
> soon. I still need to to modify the first patch to trigger
> bu PCI-ids to avoid registering non existing mux platform
> device on non CHT / APL Intel platforms.
>
> I've since changed the second patch into a platform driver,
> see:
>
> https://github.com/jwrdegoede/linux-sunxi/commit/e51057609102c35dd35b4a5965139aff81fbefb8 
>
>
> Note that this patch has 2 differences compared to
> sathyanarayanan's patch:
>
> 1) It ties directly into the extcon framework and responds
> to extcon events instead of using the mux framework,
What about devices with no ID events ? At least the APL device that I am
using does not have external IRQ for VBUS and ID.
> actually this is the first time I hear about a mux framework
> at all. Is there a git tree with the patches for this somewhere ?
>
> 2) It controls the ID and VBUS bits separately, this is
> important because on some Cherry Trail devices the ID bit
> is automatically set be ACPI code (through a gpio irq
> event handler), but the ACPI code does not update the
> VBUS bit causing the otg port on these devices to not
> work in device mode.
Even if you listen to ID and VBUS events separately, I think the end
result is selecting either SW host or SW device mode right ? Then
why not abstract MUX functionality outside the your driver and
control the MUX from your driver appropriately ? or do we get
any advantage is modifying just VBUS_VALID or SW_IDPIN bits
separately ?

> 2. also means that this no longer really is a mux driver ...
> I've run a whole lot of tests with these patches on
> various Cherry Trail devices using either ACPI code
> to set the ID pin or the already merged
> drivers/extcon/extcon-intel-int3496.c
> driver for the tablets where the ACPI code defines
> a INT3496 ACPI device instead of handling the id
> pin / bit itself.
>
> And this driver works well in both scenarios. Recently
> I've become aware of one potential problem, which is
> integrating this with devices which have a USB-C
> connector. I've one CHT device with a USB-C connector
> and a FUSB302 USB controller, in that case we need
> the mux driver to react to data / changes from the
> FUSB302 driver. One way to do this would be for the
> FUSB302 driver to also register an extcon driver.
>
> But there still is a lot of work TBD wrt getting USB-C
> to work on these devices (where it is not all handled
> in firmware like it is on regular x86 laptops). So it
> might be best to just move forward with my version of
> this driver for now, which at least makes the micro-usb
> connector found on many of these devices work properly
> in both host and device mode all the time.
>
> Regards,
>
> Hans

--
Sathyanarayanan Kuppuswamy
Linux kernel developer