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

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

[Hans de Goede]

Hi All,

Here is v2 of my USB / Type-C mux series, addressing various comments
from the reviews of v1. See the per patch changelogs for details.

For reference here the cover letter of v1:

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.

Regards,

Hans

[PATCH v2 01/11] mux: core: Add of_mux_control_get helper function

[Hans de Goede]

Currently the mux_control_get implementation only deals with getting
mux controllers on DT platforms. This commit renames the current
implementation to of_mux_control_get to reflect this and makes
mux_control_get a wrapper around of_mux_control_get.

This is a preparation patch for adding support for getting muxes on
non DT platforms.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/mux/core.c | 26 ++++++++++++++++++--------
 1 file changed, 18 insertions(+), 8 deletions(-)

diff --git a/drivers/mux/core.c b/drivers/mux/core.c
index 6e5cf9d9cd99..99d29f982c0e 100644
--- a/drivers/mux/core.c
+++ b/drivers/mux/core.c
@@ -423,14 +423,8 @@ static struct mux_chip *of_find_mux_chip_by_node(struct device_node *np)
 	return dev ? to_mux_chip(dev) : NULL;
 }
 
-/**
- * mux_control_get() - Get the mux-control for a device.
- * @dev: The device that needs a mux-control.
- * @mux_name: The name identifying the mux-control.
- *
- * Return: A pointer to the mux-control, or an ERR_PTR with a negative errno.
- */
-struct mux_control *mux_control_get(struct device *dev, const char *mux_name)
+static struct mux_control *of_mux_control_get(struct device *dev,
+					      const char *mux_name)
 {
 	struct device_node *np = dev->of_node;
 	struct of_phandle_args args;
@@ -484,6 +478,22 @@ struct mux_control *mux_control_get(struct device *dev, const char *mux_name)
 
 	return &mux_chip->mux[controller];
 }
+
+/**
+ * mux_control_get() - Get the mux-control for a device.
+ * @dev: The device that needs a mux-control.
+ * @mux_name: The name identifying the mux-control.
+ *
+ * Return: A pointer to the mux-control, or an ERR_PTR with a negative errno.
+ */
+struct mux_control *mux_control_get(struct device *dev, const char *mux_name)
+{
+	/* look up via DT first */
+	if (IS_ENABLED(CONFIG_OF) && dev->of_node)
+		return of_mux_control_get(dev, mux_name);
+
+	return ERR_PTR(-ENODEV);
+}
 EXPORT_SYMBOL_GPL(mux_control_get);
 
 /**
-- 
2.13.5

[PATCH v2 02/11] mux: core: Add support for getting a mux controller on a non DT platform

[Hans de Goede]

On non DT platforms we cannot get the mux_chip by pnode. Other subsystems
(regulator, clock, pwm) have the same problem and solve this by allowing
platform / board-setup code to add entries to a lookup table and then use
this table to look things up.

This commit adds support for getting a mux controller on a non DT platform
following this pattern. It is based on a simplified version of the pwm
subsys lookup code, the dev_id and mux_name parts of a lookup table entry
are mandatory in the mux-core implementation.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
Changes in v2:
-Fixup some kerneldoc comments, add kerneldoc comment to structs
-Minor code-style tweaks
---
 drivers/mux/core.c           | 93 +++++++++++++++++++++++++++++++++++++++++++-
 include/linux/mux/consumer.h | 19 +++++++++
 2 files changed, 111 insertions(+), 1 deletion(-)

diff --git a/drivers/mux/core.c b/drivers/mux/core.c
index 99d29f982c0e..e25bb7ab03ce 100644
--- a/drivers/mux/core.c
+++ b/drivers/mux/core.c
@@ -24,6 +24,9 @@
 #include <linux/of_platform.h>
 #include <linux/slab.h>
 
+static DEFINE_MUTEX(mux_lookup_lock);
+static LIST_HEAD(mux_lookup_list);
+
 /*
  * The idle-as-is "state" is not an actual state that may be selected, it
  * only implies that the state should not be changed. So, use that state
@@ -408,6 +411,23 @@ int mux_control_deselect(struct mux_control *mux)
 }
 EXPORT_SYMBOL_GPL(mux_control_deselect);
 
+static int parent_name_match(struct device *dev, const void *data)
+{
+	const char *parent_name = dev_name(dev->parent);
+	const char *name = data;
+
+	return strcmp(parent_name, name) == 0;
+}
+
+static struct mux_chip *mux_chip_get_by_name(const char *name)
+{
+	struct device *dev;
+
+	dev = class_find_device(&mux_class, NULL, name, parent_name_match);
+
+	return dev ? to_mux_chip(dev) : NULL;
+}
+
 static int of_dev_node_match(struct device *dev, const void *data)
 {
 	return dev->of_node == data;
@@ -480,6 +500,38 @@ static struct mux_control *of_mux_control_get(struct device *dev,
 }
 
 /**
+ * mux_add_table() - Register consumer to mux-controller mappings
+ * @table: array of mappings to register
+ * @num: number of mappings in table
+ */
+void mux_add_table(struct mux_lookup *table, size_t num)
+{
+	mutex_lock(&mux_lookup_lock);
+
+	for (; num--; table++)
+		list_add_tail(&table->list, &mux_lookup_list);
+
+	mutex_unlock(&mux_lookup_lock);
+}
+EXPORT_SYMBOL_GPL(mux_add_table);
+
+/**
+ * mux_remove_table() - Unregister consumer to mux-controller mappings
+ * @table: array of mappings to unregister
+ * @num: number of mappings in table
+ */
+void mux_remove_table(struct mux_lookup *table, size_t num)
+{
+	mutex_lock(&mux_lookup_lock);
+
+	for (; num--; table++)
+		list_del(&table->list);
+
+	mutex_unlock(&mux_lookup_lock);
+}
+EXPORT_SYMBOL_GPL(mux_remove_table);
+
+/**
  * mux_control_get() - Get the mux-control for a device.
  * @dev: The device that needs a mux-control.
  * @mux_name: The name identifying the mux-control.
@@ -488,11 +540,50 @@ static struct mux_control *of_mux_control_get(struct device *dev,
  */
 struct mux_control *mux_control_get(struct device *dev, const char *mux_name)
 {
+	struct mux_lookup *m, *chosen = NULL;
+	const char *dev_id = dev_name(dev);
+	struct mux_chip *mux_chip;
+
 	/* look up via DT first */
 	if (IS_ENABLED(CONFIG_OF) && dev->of_node)
 		return of_mux_control_get(dev, mux_name);
 
-	return ERR_PTR(-ENODEV);
+	/*
+	 * For non DT we look up the provider in the static table typically
+	 * provided by board setup code.
+	 *
+	 * If a match is found, the provider mux chip is looked up by name
+	 * and a mux-control is requested using the table provided index.
+	 */
+	mutex_lock(&mux_lookup_lock);
+	list_for_each_entry(m, &mux_lookup_list, list) {
+		if (WARN_ON(!m->dev_id || !m->mux_name || !m->provider))
+			continue;
+
+		if (!strcmp(m->dev_id, dev_id) &&
+		    !strcmp(m->mux_name, mux_name))
+		{
+			chosen = m;
+			break;
+		}
+	}
+	mutex_unlock(&mux_lookup_lock);
+
+	if (!chosen)
+		return ERR_PTR(-ENODEV);
+
+	mux_chip = mux_chip_get_by_name(chosen->provider);
+	if (!mux_chip)
+		return ERR_PTR(-EPROBE_DEFER);
+
+	if (chosen->index >= mux_chip->controllers) {
+		dev_err(dev, "Mux lookup table index out of bounds %u >= %u\n",
+			chosen->index, mux_chip->controllers);
+		put_device(&mux_chip->dev);
+		return ERR_PTR(-EINVAL);
+	}
+
+	return &mux_chip->mux[chosen->index];
 }
 EXPORT_SYMBOL_GPL(mux_control_get);
 
diff --git a/include/linux/mux/consumer.h b/include/linux/mux/consumer.h
index ea96d4c82be7..62a8ec6171c9 100644
--- a/include/linux/mux/consumer.h
+++ b/include/linux/mux/consumer.h
@@ -18,6 +18,25 @@
 struct device;
 struct mux_control;
 
+/**
+ * struct mux_lookup -	Mux consumer to mux-controller lookup table entry
+ * @list:		List head, internal use only.
+ * @provider:		dev_name() of the mux-chip's parent-dev.
+ * @index:		mux-controller's index in the mux-chip's mux array
+ * @dev_id:		dev_name() of the consumer to map to this controller
+ * @mux_name		name the consumer passes to mux_control_get
+ */
+struct mux_lookup {
+	struct list_head list;
+	const char *provider;
+	unsigned int index;
+	const char *dev_id;
+	const char *mux_name;
+};
+
+void mux_add_table(struct mux_lookup *table, size_t num);
+void mux_remove_table(struct mux_lookup *table, size_t num);
+
 unsigned int mux_control_states(struct mux_control *mux);
 int __must_check mux_control_select(struct mux_control *mux,
 				    unsigned int state);
-- 
2.13.5

[PATCH v2 03/11] mux: core: Add usb.h header with MUX_USB_* and and MUX_TYPEC_* state constants

[Hans de Goede]

Add MUX_USB_* and MUX_TYPEC_* state constant defines, which can be used by
USB device/host, resp. Type-C polarity/role/altmode mux drivers and
consumers to ensure that they agree on the meaning of the
mux_control_select() state argument.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
Changes in v2:
-Start numbering of defines at 0 not 1
-Use a new usb.h header, rather then adding these to consumer.h
-Add separate MUX_USB_* and MUX_TYPEC_* defines
---
 include/linux/mux/usb.h | 32 ++++++++++++++++++++++++++++++++
 1 file changed, 32 insertions(+)
 create mode 100644 include/linux/mux/usb.h

diff --git a/include/linux/mux/usb.h b/include/linux/mux/usb.h
new file mode 100644
index 000000000000..44df5eca5256
--- /dev/null
+++ b/include/linux/mux/usb.h
@@ -0,0 +1,32 @@
+/*
+ * mux/usb.h - definitions for USB multiplexers
+ *
+ * Copyright (C) 2017 Hans de Goede <hdegoede@redhat.com>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ */
+#ifndef _LINUX_MUX_USB_H
+#define _LINUX_MUX_USB_H
+
+/* Mux state values for USB device/host role muxes */
+#define MUX_USB_DEVICE		(0) /* USB device mode */
+#define MUX_USB_HOST		(1) /* USB host mode */
+#define MUX_USB_STATES		(2)
+
+/*
+ * Mux state values for Type-C polarity/role/altmode muxes.
+ *
+ * MUX_TYPEC_POLARITY_INV may be or-ed together with any other mux-state as
+ * inverted-polarity (Type-C plugged in upside down) can happen with any
+ * other mux-state.
+ */
+#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 DP src */
+#define MUX_TYPEC_DP_SRC		(3 << 1) /* 4 lanes Display Port src */
+#define MUX_TYPEC_STATES		(4 << 1)
+
+#endif /* _LINUX_MUX_TYPEC_H */
-- 
2.13.5

Re: [PATCH v2 03/11] mux: core: Add usb.h header with MUX_USB_* and and MUX_TYPEC_* state constants

[Peter Rosin]

On 2017-09-05 18:42, Hans de Goede wrote:
> Add MUX_USB_* and MUX_TYPEC_* state constant defines, which can be used by
> USB device/host, resp. Type-C polarity/role/altmode mux drivers and
> consumers to ensure that they agree on the meaning of the
> mux_control_select() state argument.
> 
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> ---
> Changes in v2:
> -Start numbering of defines at 0 not 1
> -Use a new usb.h header, rather then adding these to consumer.h
> -Add separate MUX_USB_* and MUX_TYPEC_* defines
> ---
>  include/linux/mux/usb.h | 32 ++++++++++++++++++++++++++++++++
>  1 file changed, 32 insertions(+)
>  create mode 100644 include/linux/mux/usb.h
> 
> diff --git a/include/linux/mux/usb.h b/include/linux/mux/usb.h
> new file mode 100644
> index 000000000000..44df5eca5256
> --- /dev/null
> +++ b/include/linux/mux/usb.h
> @@ -0,0 +1,32 @@
> +/*
> + * mux/usb.h - definitions for USB multiplexers
> + *
> + * Copyright (C) 2017 Hans de Goede <hdegoede@redhat.com>
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 as
> + * published by the Free Software Foundation.
> + */
> +#ifndef _LINUX_MUX_USB_H
> +#define _LINUX_MUX_USB_H
> +
> +/* Mux state values for USB device/host role muxes */
> +#define MUX_USB_DEVICE		(0) /* USB device mode */
> +#define MUX_USB_HOST		(1) /* USB host mode */
> +#define MUX_USB_STATES		(2)
> +
> +/*
> + * Mux state values for Type-C polarity/role/altmode muxes.
> + *
> + * MUX_TYPEC_POLARITY_INV may be or-ed together with any other mux-state as
> + * inverted-polarity (Type-C plugged in upside down) can happen with any
> + * other mux-state.
> + */
> +#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 DP src */
> +#define MUX_TYPEC_DP_SRC		(3 << 1) /* 4 lanes Display Port src */
> +#define MUX_TYPEC_STATES		(4 << 1)

But USB Type-C muxes need not support just these states If I read it right?
USB Type-C seems to be usable for a variety of protocols and the above list
seems pretty much like a special case for this mux (and perhaps a set of
other similar muxes). But when someone with a USB Type-C mux for different
protocols shows up, that person will probably be frustrated by these
defines, no? Or is there something I don't see that limits USB-C to DP?

Cheers,
Peter

> +
> +#endif /* _LINUX_MUX_TYPEC_H */
> 

Re: [PATCH v2 03/11] mux: core: Add usb.h header with MUX_USB_* and and MUX_TYPEC_* state constants

[Hans de Goede]

Hi,

On 08-09-17 17:47, Peter Rosin wrote:
> On 2017-09-05 18:42, Hans de Goede wrote:
>> Add MUX_USB_* and MUX_TYPEC_* state constant defines, which can be used by
>> USB device/host, resp. Type-C polarity/role/altmode mux drivers and
>> consumers to ensure that they agree on the meaning of the
>> mux_control_select() state argument.
>>
>> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
>> ---
>> Changes in v2:
>> -Start numbering of defines at 0 not 1
>> -Use a new usb.h header, rather then adding these to consumer.h
>> -Add separate MUX_USB_* and MUX_TYPEC_* defines
>> ---
>>   include/linux/mux/usb.h | 32 ++++++++++++++++++++++++++++++++
>>   1 file changed, 32 insertions(+)
>>   create mode 100644 include/linux/mux/usb.h
>>
>> diff --git a/include/linux/mux/usb.h b/include/linux/mux/usb.h
>> new file mode 100644
>> index 000000000000..44df5eca5256
>> --- /dev/null
>> +++ b/include/linux/mux/usb.h
>> @@ -0,0 +1,32 @@
>> +/*
>> + * mux/usb.h - definitions for USB multiplexers
>> + *
>> + * Copyright (C) 2017 Hans de Goede <hdegoede@redhat.com>
>> + *
>> + * This program is free software; you can redistribute it and/or modify
>> + * it under the terms of the GNU General Public License version 2 as
>> + * published by the Free Software Foundation.
>> + */
>> +#ifndef _LINUX_MUX_USB_H
>> +#define _LINUX_MUX_USB_H
>> +
>> +/* Mux state values for USB device/host role muxes */
>> +#define MUX_USB_DEVICE		(0) /* USB device mode */
>> +#define MUX_USB_HOST		(1) /* USB host mode */
>> +#define MUX_USB_STATES		(2)
>> +
>> +/*
>> + * Mux state values for Type-C polarity/role/altmode muxes.
>> + *
>> + * MUX_TYPEC_POLARITY_INV may be or-ed together with any other mux-state as
>> + * inverted-polarity (Type-C plugged in upside down) can happen with any
>> + * other mux-state.
>> + */
>> +#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 DP src */
>> +#define MUX_TYPEC_DP_SRC		(3 << 1) /* 4 lanes Display Port src */
>> +#define MUX_TYPEC_STATES		(4 << 1)
> 
> But USB Type-C muxes need not support just these states If I read it right?
> USB Type-C seems to be usable for a variety of protocols and the above list
> seems pretty much like a special case for this mux (and perhaps a set of
> other similar muxes). But when someone with a USB Type-C mux for different
> protocols shows up, that person will probably be frustrated by these
> defines, no? Or is there something I don't see that limits USB-C to DP?

In general almost all hardware is limited to the above (+ analog audio over
the 2 Sideband use pins, but I expect that to have a separate mux).

You're right, theoretically there might be other cases, e.g. there is a spec
for HDMI over Type-C (wishful thinking from the HDMI group, no one uses this),
but:

1) I expect most muxes to implement the above set, that is what all
hardware out there supports (well that or less).

2) We can always add extra defines here, that means that a Type-C mux may
not implement all states and return -EINVAL when asked for something it
does not implement, which I understand is a bit weird from a mux subsys
pov. But that can be the case anyways because even though the mux supports
these options, the board it is used on does no necessarily have to support
these options, e.g. there may be only 2 lanes of DP hooked up to the mux
(or no DP at all, but then I would them to expect a different mux).

So the Type-C Port Manager already needs to be passed some platform
data describing which features the board has and keep that in mind
when negotiation with the dongle attached to the Type-C port, so if
we do get boards which do HDMI and no DP, then the TCPM would simply
never use the MUX_TYPEC_HOST_AND_DP_SRC and MUX_TYPEC_DP_SRC states.

Regards,

Hans

Re: [PATCH v2 03/11] mux: core: Add usb.h header with MUX_USB_* and and MUX_TYPEC_* state constants

[Peter Rosin]

On 2017-09-08 19:07, Hans de Goede wrote:
> Hi,
> 
> On 08-09-17 17:47, Peter Rosin wrote:
>> On 2017-09-05 18:42, Hans de Goede wrote:
>>> Add MUX_USB_* and MUX_TYPEC_* state constant defines, which can be used by
>>> USB device/host, resp. Type-C polarity/role/altmode mux drivers and
>>> consumers to ensure that they agree on the meaning of the
>>> mux_control_select() state argument.
>>>
>>> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
>>> ---

*snip*

>>> +/*
>>> + * Mux state values for Type-C polarity/role/altmode muxes.
>>> + *
>>> + * MUX_TYPEC_POLARITY_INV may be or-ed together with any other mux-state as
>>> + * inverted-polarity (Type-C plugged in upside down) can happen with any
>>> + * other mux-state.
>>> + */
>>> +#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 DP src */
>>> +#define MUX_TYPEC_DP_SRC		(3 << 1) /* 4 lanes Display Port src */
>>> +#define MUX_TYPEC_STATES		(4 << 1)
>>
>> But USB Type-C muxes need not support just these states If I read it right?
>> USB Type-C seems to be usable for a variety of protocols and the above list
>> seems pretty much like a special case for this mux (and perhaps a set of
>> other similar muxes). But when someone with a USB Type-C mux for different
>> protocols shows up, that person will probably be frustrated by these
>> defines, no? Or is there something I don't see that limits USB-C to DP?
> 
> In general almost all hardware is limited to the above (+ analog audio over
> the 2 Sideband use pins, but I expect that to have a separate mux).
> 
> You're right, theoretically there might be other cases, e.g. there is a spec
> for HDMI over Type-C (wishful thinking from the HDMI group, no one uses this),
> but:
> 
> 1) I expect most muxes to implement the above set, that is what all
> hardware out there supports (well that or less).
> 
> 2) We can always add extra defines here, that means that a Type-C mux may
> not implement all states and return -EINVAL when asked for something it
> does not implement, which I understand is a bit weird from a mux subsys
> pov. But that can be the case anyways because even though the mux supports
> these options, the board it is used on does no necessarily have to support
> these options, e.g. there may be only 2 lanes of DP hooked up to the mux
> (or no DP at all, but then I would them to expect a different mux).
> 
> So the Type-C Port Manager already needs to be passed some platform
> data describing which features the board has and keep that in mind
> when negotiation with the dongle attached to the Type-C port, so if
> we do get boards which do HDMI and no DP, then the TCPM would simply
> never use the MUX_TYPEC_HOST_AND_DP_SRC and MUX_TYPEC_DP_SRC states.

Ok, I googled "usb type c mux" and came up with HD3SS460 from Texas as
the first hit.

http://www.ti.com/lit/ds/symlink/hd3ss460.pdf

That one has three control pins, but two of them (AMSEL and EN) are
tri-state. So 18 states in theory. However, if EN is low everything is
HighZ, so that collapses 6 states into 1, and 2 other states are reserved.
Still 11 states, which is two more than what you have implemented for
PI3USB30532. If we ignore polarity switching, it's only a one state diff.
However, when I try to make sense of the states for the HD3SS460, I don't
see anything that selects USB device or host. And I don't really see why
a Type C mux has to know that; in my head the mux should just route the
signals. And then when I look in your PI3USB30532 driver I don't seen any
such difference either. Along the same lines, the Type C mux does not
know/care if DP is source or sink. Or?

How about:

#define MUX_TYPEC_POLARITY_INV		BIT(0)   /* Polarity inverted bit */
#define MUX_TYPEC_USB			(0 << 1) /* USB only mode */
#define MUX_TYPEC_USB_AND_DP		(1 << 1) /* USB host + 2 lanes DP */
#define MUX_TYPEC_DP			(2 << 1) /* 4 lanes Display Port */
#define MUX_TYPEC_STATES		(3 << 1)

I'm not sure what 2 states the HS3SS460 have in addition to the above, but
the way I read the spec those to are variations on the MUX_TYPEC_USB_AND_DP
state, but routing the DP signals to alternate pins. Which suggests that
more documentation is needed to describe exactly what is meant when someone
selects MUX_TYPEC_USB_AND_DP?

Cheers,
Peter

Re: [PATCH v2 03/11] mux: core: Add usb.h header with MUX_USB_* and and MUX_TYPEC_* state constants

[Hans de Goede]

Hi,

On 10-09-17 23:36, Peter Rosin wrote:
> On 2017-09-08 19:07, Hans de Goede wrote:
>> Hi,
>>
>> On 08-09-17 17:47, Peter Rosin wrote:
>>> On 2017-09-05 18:42, Hans de Goede wrote:
>>>> Add MUX_USB_* and MUX_TYPEC_* state constant defines, which can be used by
>>>> USB device/host, resp. Type-C polarity/role/altmode mux drivers and
>>>> consumers to ensure that they agree on the meaning of the
>>>> mux_control_select() state argument.
>>>>
>>>> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
>>>> ---
> 
> *snip*
> 
>>>> +/*
>>>> + * Mux state values for Type-C polarity/role/altmode muxes.
>>>> + *
>>>> + * MUX_TYPEC_POLARITY_INV may be or-ed together with any other mux-state as
>>>> + * inverted-polarity (Type-C plugged in upside down) can happen with any
>>>> + * other mux-state.
>>>> + */
>>>> +#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 DP src */
>>>> +#define MUX_TYPEC_DP_SRC		(3 << 1) /* 4 lanes Display Port src */
>>>> +#define MUX_TYPEC_STATES		(4 << 1)
>>>
>>> But USB Type-C muxes need not support just these states If I read it right?
>>> USB Type-C seems to be usable for a variety of protocols and the above list
>>> seems pretty much like a special case for this mux (and perhaps a set of
>>> other similar muxes). But when someone with a USB Type-C mux for different
>>> protocols shows up, that person will probably be frustrated by these
>>> defines, no? Or is there something I don't see that limits USB-C to DP?
>>
>> In general almost all hardware is limited to the above (+ analog audio over
>> the 2 Sideband use pins, but I expect that to have a separate mux).
>>
>> You're right, theoretically there might be other cases, e.g. there is a spec
>> for HDMI over Type-C (wishful thinking from the HDMI group, no one uses this),
>> but:
>>
>> 1) I expect most muxes to implement the above set, that is what all
>> hardware out there supports (well that or less).
>>
>> 2) We can always add extra defines here, that means that a Type-C mux may
>> not implement all states and return -EINVAL when asked for something it
>> does not implement, which I understand is a bit weird from a mux subsys
>> pov. But that can be the case anyways because even though the mux supports
>> these options, the board it is used on does no necessarily have to support
>> these options, e.g. there may be only 2 lanes of DP hooked up to the mux
>> (or no DP at all, but then I would them to expect a different mux).
>>
>> So the Type-C Port Manager already needs to be passed some platform
>> data describing which features the board has and keep that in mind
>> when negotiation with the dongle attached to the Type-C port, so if
>> we do get boards which do HDMI and no DP, then the TCPM would simply
>> never use the MUX_TYPEC_HOST_AND_DP_SRC and MUX_TYPEC_DP_SRC states.
> 
> Ok, I googled "usb type c mux" and came up with HD3SS460 from Texas as
> the first hit.
> 
> http://www.ti.com/lit/ds/symlink/hd3ss460.pdf
> 
> That one has three control pins, but two of them (AMSEL and EN) are
> tri-state. So 18 states in theory. However, if EN is low everything is
> HighZ, so that collapses 6 states into 1, and 2 other states are reserved.
> Still 11 states, which is two more than what you have implemented for
> PI3USB30532. If we ignore polarity switching, it's only a one state diff.
> However, when I try to make sense of the states for the HD3SS460, I don't
> see anything that selects USB device or host. And I don't really see why
> a Type C mux has to know that; in my head the mux should just route the
> signals. And then when I look in your PI3USB30532 driver I don't seen any
> such difference either. Along the same lines, the Type C mux does not
> know/care if DP is source or sink. Or?
> 
> How about:
> 
> #define MUX_TYPEC_POLARITY_INV		BIT(0)   /* Polarity inverted bit */
> #define MUX_TYPEC_USB			(0 << 1) /* USB only mode */
> #define MUX_TYPEC_USB_AND_DP		(1 << 1) /* USB host + 2 lanes DP */
> #define MUX_TYPEC_DP			(2 << 1) /* 4 lanes Display Port */
> #define MUX_TYPEC_STATES		(3 << 1)
> 

Sure that works for me, I will switch over to this for v3 of the patch-set.

One note though, compared to my list, this changes DEVICE / HOST to just a single
_USB entry. That works fine for my purpose, but typically USB host and device
controllers are 2 separate blocks with a mux in between them. Now most
current hardware have that mux in the SoC and then an external mux to
mux the USB3 lines and optionally also DP lines to the Type-C connector
and the above table does is correct (as the Type-C mux only has 1 USB
state not separate host / device states as my proposal was). But my
reason for having separate DEVICE / HOST states (and treating those
identical in the pi3usb30532 driver) was to future proof things a bit.

> I'm not sure what 2 states the HS3SS460 have in addition to the above, but
> the way I read the spec those to are variations on the MUX_TYPEC_USB_AND_DP
> state, but routing the DP signals to alternate pins. Which suggests that
> more documentation is needed to describe exactly what is meant when someone
> selects MUX_TYPEC_USB_AND_DP?

The DP over Type-C spec unfortunately is not open, but the slva844a.pdf
TI appnote has a table listing the possible pin permutations which can
be used with DP over Type-C (Table 1. page 5) which always has all the
superspeed USB pins in the same place.

Regards,

Hans

[PATCH v2 04/11] usb: xhci: Add Intel cherrytrail extended cap / otg phy mux handling

[Hans de Goede]

The Intel cherrytrail xhci controller has an extended cap mmio-range
which contains registers to control the muxing to the xhci (host mode)
or the dwc3 (device mode) and vbus-detection for the otg usb-phy.

Having a mux driver included in the xhci code (or under drivers/usb/host)
is not desirable. So this commit adds a simple handler for this extended
capability, which creates a platform device with the caps mmio region as
resource, this allows us to write a separate platform mux driver for the
mux.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
Changes in v2:
-Check xHCI controller PCI device-id instead of only checking for the
 Intel Extended capability ID, as the Extended capability ID is used on
 other model Intel xHCI controllers too
---
 drivers/usb/host/Makefile            |  2 +-
 drivers/usb/host/xhci-intel-quirks.c | 85 ++++++++++++++++++++++++++++++++++++
 drivers/usb/host/xhci-pci.c          |  4 ++
 drivers/usb/host/xhci.h              |  2 +
 4 files changed, 92 insertions(+), 1 deletion(-)
 create mode 100644 drivers/usb/host/xhci-intel-quirks.c

diff --git a/drivers/usb/host/Makefile b/drivers/usb/host/Makefile
index cf2691fffcc0..441edf82eb1c 100644
--- a/drivers/usb/host/Makefile
+++ b/drivers/usb/host/Makefile
@@ -63,7 +63,7 @@ obj-$(CONFIG_USB_OHCI_HCD_DAVINCI)	+= ohci-da8xx.o
 obj-$(CONFIG_USB_UHCI_HCD)	+= uhci-hcd.o
 obj-$(CONFIG_USB_FHCI_HCD)	+= fhci.o
 obj-$(CONFIG_USB_XHCI_HCD)	+= xhci-hcd.o
-obj-$(CONFIG_USB_XHCI_PCI)	+= xhci-pci.o
+obj-$(CONFIG_USB_XHCI_PCI)	+= xhci-pci.o xhci-intel-quirks.o
 obj-$(CONFIG_USB_XHCI_PLATFORM) += xhci-plat-hcd.o
 obj-$(CONFIG_USB_XHCI_MTK)	+= xhci-mtk.o
 obj-$(CONFIG_USB_XHCI_TEGRA)	+= xhci-tegra.o
diff --git a/drivers/usb/host/xhci-intel-quirks.c b/drivers/usb/host/xhci-intel-quirks.c
new file mode 100644
index 000000000000..819f5f9da9ee
--- /dev/null
+++ b/drivers/usb/host/xhci-intel-quirks.c
@@ -0,0 +1,85 @@
+/*
+ * Intel Vendor Defined XHCI extended capability handling
+ *
+ * Copyright (c) 2016) Hans de Goede <hdegoede@redhat.com>
+ *
+ * Loosely based on android x86 kernel code which is:
+ *
+ * Copyright (C) 2014 Intel Corp.
+ *
+ * Author: Wu, Hao
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope that it will be useful, but
+ * WITHOUT ANY WARRANTY; without even the implied warranty of MERCHANTABILITY
+ * or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License
+ * for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program;
+ */
+
+#include <linux/platform_device.h>
+#include "xhci.h"
+
+/* Extended capability IDs for Intel Vendor Defined */
+#define XHCI_EXT_CAPS_INTEL_HOST_CAP	192
+
+static void xhci_intel_unregister_pdev(void *arg)
+{
+	platform_device_unregister(arg);
+}
+
+int xhci_create_intel_cht_mux_pdev(struct xhci_hcd *xhci)
+{
+	struct usb_hcd *hcd = xhci_to_hcd(xhci);
+	struct device *dev = hcd->self.controller;
+	struct platform_device *pdev;
+	struct resource	res = { 0, };
+	int ret, ext_offset;
+
+	ext_offset = xhci_find_next_ext_cap(&xhci->cap_regs->hc_capbase, 0,
+					    XHCI_EXT_CAPS_INTEL_HOST_CAP);
+	if (!ext_offset) {
+		xhci_err(xhci, "couldn't find Intel ext caps\n");
+		return -ENODEV;
+	}
+
+	pdev = platform_device_alloc("intel_cht_usb_mux", PLATFORM_DEVID_NONE);
+	if (!pdev) {
+		xhci_err(xhci, "couldn't allocate intel_cht_usb_mux pdev\n");
+		return -ENOMEM;
+	}
+
+	res.start = hcd->rsrc_start + ext_offset;
+	res.end	  = res.start + 0x3ff;
+	res.name  = "intel_cht_usb_mux";
+	res.flags = IORESOURCE_MEM;
+
+	ret = platform_device_add_resources(pdev, &res, 1);
+	if (ret) {
+		dev_err(dev, "couldn't add resources to intel_cht_usb_mux pdev\n");
+		platform_device_put(pdev);
+		return ret;
+	}
+
+	pdev->dev.parent = dev;
+
+	ret = platform_device_add(pdev);
+	if (ret) {
+		dev_err(dev, "couldn't register intel_cht_usb_mux pdev\n");
+		platform_device_put(pdev);
+		return ret;
+	}
+
+	ret = devm_add_action_or_reset(dev, xhci_intel_unregister_pdev, pdev);
+	if (ret) {
+		dev_err(dev, "couldn't add unregister action for intel_cht_usb_mux pdev\n");
+		return ret;
+	}
+
+	return 0;
+}
diff --git a/drivers/usb/host/xhci-pci.c b/drivers/usb/host/xhci-pci.c
index 8071c8fdd15e..b55c1e96abf0 100644
--- a/drivers/usb/host/xhci-pci.c
+++ b/drivers/usb/host/xhci-pci.c
@@ -188,6 +188,7 @@ static void xhci_pci_quirks(struct device *dev, struct xhci_hcd *xhci)
 	if (pdev->vendor == PCI_VENDOR_ID_INTEL &&
 		 pdev->device == PCI_DEVICE_ID_INTEL_CHERRYVIEW_XHCI) {
 		xhci->quirks |= XHCI_SSIC_PORT_UNUSED;
+		xhci->quirks |= XHCI_INTEL_CHT_USB_MUX;
 	}
 	if (pdev->vendor == PCI_VENDOR_ID_INTEL &&
 	    (pdev->device == PCI_DEVICE_ID_INTEL_CHERRYVIEW_XHCI ||
@@ -328,6 +329,9 @@ static int xhci_pci_probe(struct pci_dev *dev, const struct pci_device_id *id)
 	if (xhci->quirks & XHCI_PME_STUCK_QUIRK)
 		xhci_pme_acpi_rtd3_enable(dev);
 
+	if (xhci->quirks & XHCI_INTEL_CHT_USB_MUX)
+		xhci_create_intel_cht_mux_pdev(xhci);
+
 	/* USB-2 and USB-3 roothubs initialized, allow runtime pm suspend */
 	pm_runtime_put_noidle(&dev->dev);
 
diff --git a/drivers/usb/host/xhci.h b/drivers/usb/host/xhci.h
index e3e935291ed6..f722ee31e50d 100644
--- a/drivers/usb/host/xhci.h
+++ b/drivers/usb/host/xhci.h
@@ -1821,6 +1821,7 @@ struct xhci_hcd {
 #define XHCI_LIMIT_ENDPOINT_INTERVAL_7	(1 << 26)
 #define XHCI_U2_DISABLE_WAKE	(1 << 27)
 #define XHCI_ASMEDIA_MODIFY_FLOWCONTROL	(1 << 28)
+#define XHCI_INTEL_CHT_USB_MUX	(1 << 29)
 
 	unsigned int		num_active_eps;
 	unsigned int		limit_active_eps;
@@ -2005,6 +2006,7 @@ void xhci_init_driver(struct hc_driver *drv,
 		      const struct xhci_driver_overrides *over);
 int xhci_disable_slot(struct xhci_hcd *xhci,
 			struct xhci_command *command, u32 slot_id);
+int xhci_create_intel_cht_mux_pdev(struct xhci_hcd *xhci);
 
 int xhci_suspend(struct xhci_hcd *xhci, bool do_wakeup);
 int xhci_resume(struct xhci_hcd *xhci, bool hibernated);
-- 
2.13.5

Re: [PATCH v2 04/11] usb: xhci: Add Intel cherrytrail extended cap / otg phy mux handling

[Mathias Nyman]

On 05.09.2017 19:42, Hans de Goede wrote:
> The Intel cherrytrail xhci controller has an extended cap mmio-range
> which contains registers to control the muxing to the xhci (host mode)
> or the dwc3 (device mode) and vbus-detection for the otg usb-phy.
>
> Having a mux driver included in the xhci code (or under drivers/usb/host)
> is not desirable. So this commit adds a simple handler for this extended
> capability, which creates a platform device with the caps mmio region as
> resource, this allows us to write a separate platform mux driver for the
> mux.
>
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> ---
> Changes in v2:
> -Check xHCI controller PCI device-id instead of only checking for the
>   Intel Extended capability ID, as the Extended capability ID is used on
>   other model Intel xHCI controllers too
> ---
>   drivers/usb/host/Makefile            |  2 +-
>   drivers/usb/host/xhci-intel-quirks.c | 85 ++++++++++++++++++++++++++++++++++++
>   drivers/usb/host/xhci-pci.c          |  4 ++
>   drivers/usb/host/xhci.h              |  2 +
>   4 files changed, 92 insertions(+), 1 deletion(-)
>   create mode 100644 drivers/usb/host/xhci-intel-quirks.c
>
> diff --git a/drivers/usb/host/Makefile b/drivers/usb/host/Makefile
> index cf2691fffcc0..441edf82eb1c 100644
> --- a/drivers/usb/host/Makefile
> +++ b/drivers/usb/host/Makefile
> @@ -63,7 +63,7 @@ obj-$(CONFIG_USB_OHCI_HCD_DAVINCI)	+= ohci-da8xx.o
>   obj-$(CONFIG_USB_UHCI_HCD)	+= uhci-hcd.o
>   obj-$(CONFIG_USB_FHCI_HCD)	+= fhci.o
>   obj-$(CONFIG_USB_XHCI_HCD)	+= xhci-hcd.o
> -obj-$(CONFIG_USB_XHCI_PCI)	+= xhci-pci.o
> +obj-$(CONFIG_USB_XHCI_PCI)	+= xhci-pci.o xhci-intel-quirks.o
>   obj-$(CONFIG_USB_XHCI_PLATFORM) += xhci-plat-hcd.o
>   obj-$(CONFIG_USB_XHCI_MTK)	+= xhci-mtk.o
>   obj-$(CONFIG_USB_XHCI_TEGRA)	+= xhci-tegra.o
> diff --git a/drivers/usb/host/xhci-intel-quirks.c b/drivers/usb/host/xhci-intel-quirks.c
> new file mode 100644

I think it would be better to have one place where we add handlers for
vendor specific extended capabilities.

Something like xhci-vendor-ext-caps.c, or just xhci-ext-caps.c as
there's a xhci-ext-caps.h header already

We could walk through the capability list once and add the needed handlers.
Something like:

+int xhci_ext_cap_init(void __iomem *base)
+{
+        u32 val;
+       u32 cap_offset;
+
+        cap_offset = xhci_next_ext_cap(base, 0);
+
+        while (cap_offset) {
+                val = readl(base + cap_offset);
+
+                switch (XHCI_EXT_CAPS_ID(val)) {
+                case XHCI_EXT_CAPS_VENDOR_INTEL:
+                        /* check hw/id/something, and call what's needed */
+                        break;
+                case XHCI_EXT_CAPS_VENDOR_XYZ:
+                        /* do something */
+                        break;
+                default:
+                        break;
+                }
+
+               printk(KERN_ERR "MATTU EXT_CAP id %d\n", XHCI_EXT_CAPS_ID(val));
+
+               cap_offset = xhci_next_ext_cap(base, cap_offset);
+        }
+
+        return 0;
+}

xhci_next_ext_cap() doesn't exist anywhere else than my local sandbox branch yet.

> +
> +/* Extended capability IDs for Intel Vendor Defined */
> +#define XHCI_EXT_CAPS_INTEL_HOST_CAP	192

XHCI_EXT_CAPS_VENDOR_INTEL
and should be in xhci-ext-caps.h

> +
> +static void xhci_intel_unregister_pdev(void *arg)
> +{
> +	platform_device_unregister(arg);
> +}
> +
> +int xhci_create_intel_cht_mux_pdev(struct xhci_hcd *xhci)
> +{
> +	struct usb_hcd *hcd = xhci_to_hcd(xhci);
> +	struct device *dev = hcd->self.controller;
> +	struct platform_device *pdev;
> +	struct resource	res = { 0, };
> +	int ret, ext_offset;
> +
> +	ext_offset = xhci_find_next_ext_cap(&xhci->cap_regs->hc_capbase, 0,
> +					    XHCI_EXT_CAPS_INTEL_HOST_CAP);
> +	if (!ext_offset) {
> +		xhci_err(xhci, "couldn't find Intel ext caps\n");
> +		return -ENODEV;
> +	}
> +
> +	pdev = platform_device_alloc("intel_cht_usb_mux", PLATFORM_DEVID_NONE);
> +	if (!pdev) {
> +		xhci_err(xhci, "couldn't allocate intel_cht_usb_mux pdev\n");
> +		return -ENOMEM;
> +	}
> +
> +	res.start = hcd->rsrc_start + ext_offset;
> +	res.end	  = res.start + 0x3ff;
> +	res.name  = "intel_cht_usb_mux";
> +	res.flags = IORESOURCE_MEM;
> +
> +	ret = platform_device_add_resources(pdev, &res, 1);
> +	if (ret) {
> +		dev_err(dev, "couldn't add resources to intel_cht_usb_mux pdev\n");
> +		platform_device_put(pdev);
> +		return ret;
> +	}
> +
> +	pdev->dev.parent = dev;
> +
> +	ret = platform_device_add(pdev);
> +	if (ret) {
> +		dev_err(dev, "couldn't register intel_cht_usb_mux pdev\n");
> +		platform_device_put(pdev);
> +		return ret;
> +	}
> +
> +	ret = devm_add_action_or_reset(dev, xhci_intel_unregister_pdev, pdev);
> +	if (ret) {
> +		dev_err(dev, "couldn't add unregister action for intel_cht_usb_mux pdev\n");
> +		return ret;
> +	}
> +
> +	return 0;
> +}
> diff --git a/drivers/usb/host/xhci-pci.c b/drivers/usb/host/xhci-pci.c
> index 8071c8fdd15e..b55c1e96abf0 100644
> --- a/drivers/usb/host/xhci-pci.c
> +++ b/drivers/usb/host/xhci-pci.c
> @@ -188,6 +188,7 @@ static void xhci_pci_quirks(struct device *dev, struct xhci_hcd *xhci)
>   	if (pdev->vendor == PCI_VENDOR_ID_INTEL &&
>   		 pdev->device == PCI_DEVICE_ID_INTEL_CHERRYVIEW_XHCI) {
>   		xhci->quirks |= XHCI_SSIC_PORT_UNUSED;
> +		xhci->quirks |= XHCI_INTEL_CHT_USB_MUX;
>   	}
>   	if (pdev->vendor == PCI_VENDOR_ID_INTEL &&
>   	    (pdev->device == PCI_DEVICE_ID_INTEL_CHERRYVIEW_XHCI ||
> @@ -328,6 +329,9 @@ static int xhci_pci_probe(struct pci_dev *dev, const struct pci_device_id *id)
>   	if (xhci->quirks & XHCI_PME_STUCK_QUIRK)
>   		xhci_pme_acpi_rtd3_enable(dev);
>
> +	if (xhci->quirks & XHCI_INTEL_CHT_USB_MUX)
> +		xhci_create_intel_cht_mux_pdev(xhci);
> +
>   	/* USB-2 and USB-3 roothubs initialized, allow runtime pm suspend */
>   	pm_runtime_put_noidle(&dev->dev);
>
> diff --git a/drivers/usb/host/xhci.h b/drivers/usb/host/xhci.h
> index e3e935291ed6..f722ee31e50d 100644
> --- a/drivers/usb/host/xhci.h
> +++ b/drivers/usb/host/xhci.h
> @@ -1821,6 +1821,7 @@ struct xhci_hcd {
>   #define XHCI_LIMIT_ENDPOINT_INTERVAL_7	(1 << 26)
>   #define XHCI_U2_DISABLE_WAKE	(1 << 27)
>   #define XHCI_ASMEDIA_MODIFY_FLOWCONTROL	(1 << 28)
> +#define XHCI_INTEL_CHT_USB_MUX	(1 << 29)

Everything you said about the quirk flags in v1 is true, and I hate to
make the bulk of xhci even more PCI dependent, but I'm running out of flag bits and
would like a better solution than the current quirk flags we have.

This isn't really your concern, this patch is just doing it the same way it has
always been done, but if you have a better idea I'm all ears.

-Mathias

Re: [PATCH v2 04/11] usb: xhci: Add Intel cherrytrail extended cap / otg phy mux handling

[Hans de Goede]

Hi,

On 07-09-17 15:14, Mathias Nyman wrote:
> On 05.09.2017 19:42, Hans de Goede wrote:
>> The Intel cherrytrail xhci controller has an extended cap mmio-range
>> which contains registers to control the muxing to the xhci (host mode)
>> or the dwc3 (device mode) and vbus-detection for the otg usb-phy.
>>
>> Having a mux driver included in the xhci code (or under drivers/usb/host)
>> is not desirable. So this commit adds a simple handler for this extended
>> capability, which creates a platform device with the caps mmio region as
>> resource, this allows us to write a separate platform mux driver for the
>> mux.
>>
>> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
>> ---
>> Changes in v2:
>> -Check xHCI controller PCI device-id instead of only checking for the
>>   Intel Extended capability ID, as the Extended capability ID is used on
>>   other model Intel xHCI controllers too
>> ---
>>   drivers/usb/host/Makefile            |  2 +-
>>   drivers/usb/host/xhci-intel-quirks.c | 85 ++++++++++++++++++++++++++++++++++++
>>   drivers/usb/host/xhci-pci.c          |  4 ++
>>   drivers/usb/host/xhci.h              |  2 +
>>   4 files changed, 92 insertions(+), 1 deletion(-)
>>   create mode 100644 drivers/usb/host/xhci-intel-quirks.c
>>
>> diff --git a/drivers/usb/host/Makefile b/drivers/usb/host/Makefile
>> index cf2691fffcc0..441edf82eb1c 100644
>> --- a/drivers/usb/host/Makefile
>> +++ b/drivers/usb/host/Makefile
>> @@ -63,7 +63,7 @@ obj-$(CONFIG_USB_OHCI_HCD_DAVINCI)    += ohci-da8xx.o
>>   obj-$(CONFIG_USB_UHCI_HCD)    += uhci-hcd.o
>>   obj-$(CONFIG_USB_FHCI_HCD)    += fhci.o
>>   obj-$(CONFIG_USB_XHCI_HCD)    += xhci-hcd.o
>> -obj-$(CONFIG_USB_XHCI_PCI)    += xhci-pci.o
>> +obj-$(CONFIG_USB_XHCI_PCI)    += xhci-pci.o xhci-intel-quirks.o
>>   obj-$(CONFIG_USB_XHCI_PLATFORM) += xhci-plat-hcd.o
>>   obj-$(CONFIG_USB_XHCI_MTK)    += xhci-mtk.o
>>   obj-$(CONFIG_USB_XHCI_TEGRA)    += xhci-tegra.o
>> diff --git a/drivers/usb/host/xhci-intel-quirks.c b/drivers/usb/host/xhci-intel-quirks.c
>> new file mode 100644
> 
> I think it would be better to have one place where we add handlers for
> vendor specific extended capabilities.
> 
> Something like xhci-vendor-ext-caps.c, or just xhci-ext-caps.c as
> there's a xhci-ext-caps.h header already
> 
> We could walk through the capability list once and add the needed handlers.
> Something like:
> 
> +int xhci_ext_cap_init(void __iomem *base)

This will need to take a struct xhci_hcd *xhci param instead
as some of the ext_cap handling (including the cht mux code)
will need access to this.

> +{
> +        u32 val;
> +       u32 cap_offset;
> +
> +        cap_offset = xhci_next_ext_cap(base, 0);
> +
> +        while (cap_offset) {
> +                val = readl(base + cap_offset);
> +
> +                switch (XHCI_EXT_CAPS_ID(val)) {
> +                case XHCI_EXT_CAPS_VENDOR_INTEL:
> +                        /* check hw/id/something, and call what's needed */

So I see 2 options here (without making this function PCI specific)
1) Add an u32 product_id field to struct xhci_hcd; or
2) Use a quirk flag as my current code is doing.

I'm fine with doing this either way, please let me know your preference.

> +                        break;
> +                case XHCI_EXT_CAPS_VENDOR_XYZ:
> +                        /* do something */
> +                        break;
> +                default:
> +                        break;
> +                }
> +
> +               printk(KERN_ERR "MATTU EXT_CAP id %d\n", XHCI_EXT_CAPS_ID(val));
> +
> +               cap_offset = xhci_next_ext_cap(base, cap_offset);
> +        }
> +
> +        return 0;
> +}
> 
> xhci_next_ext_cap() doesn't exist anywhere else than my local sandbox branch yet.

Can you do a "git format-patch" of that and send it to me? If you
can give me that + your preference for how to check if we're
dealing with a cht xhci hcd in xhci_ext_cap_init I can do a v3
with your suggestions applied.

> 
>> +
>> +/* Extended capability IDs for Intel Vendor Defined */
>> +#define XHCI_EXT_CAPS_INTEL_HOST_CAP    192
> 
> XHCI_EXT_CAPS_VENDOR_INTEL
> and should be in xhci-ext-caps.h

Ok.

Regards,

Hans


> 
>> +
>> +static void xhci_intel_unregister_pdev(void *arg)
>> +{
>> +    platform_device_unregister(arg);
>> +}
>> +
>> +int xhci_create_intel_cht_mux_pdev(struct xhci_hcd *xhci)
>> +{
>> +    struct usb_hcd *hcd = xhci_to_hcd(xhci);
>> +    struct device *dev = hcd->self.controller;
>> +    struct platform_device *pdev;
>> +    struct resource    res = { 0, };
>> +    int ret, ext_offset;
>> +
>> +    ext_offset = xhci_find_next_ext_cap(&xhci->cap_regs->hc_capbase, 0,
>> +                        XHCI_EXT_CAPS_INTEL_HOST_CAP);
>> +    if (!ext_offset) {
>> +        xhci_err(xhci, "couldn't find Intel ext caps\n");
>> +        return -ENODEV;
>> +    }
>> +
>> +    pdev = platform_device_alloc("intel_cht_usb_mux", PLATFORM_DEVID_NONE);
>> +    if (!pdev) {
>> +        xhci_err(xhci, "couldn't allocate intel_cht_usb_mux pdev\n");
>> +        return -ENOMEM;
>> +    }
>> +
>> +    res.start = hcd->rsrc_start + ext_offset;
>> +    res.end      = res.start + 0x3ff;
>> +    res.name  = "intel_cht_usb_mux";
>> +    res.flags = IORESOURCE_MEM;
>> +
>> +    ret = platform_device_add_resources(pdev, &res, 1);
>> +    if (ret) {
>> +        dev_err(dev, "couldn't add resources to intel_cht_usb_mux pdev\n");
>> +        platform_device_put(pdev);
>> +        return ret;
>> +    }
>> +
>> +    pdev->dev.parent = dev;
>> +
>> +    ret = platform_device_add(pdev);
>> +    if (ret) {
>> +        dev_err(dev, "couldn't register intel_cht_usb_mux pdev\n");
>> +        platform_device_put(pdev);
>> +        return ret;
>> +    }
>> +
>> +    ret = devm_add_action_or_reset(dev, xhci_intel_unregister_pdev, pdev);
>> +    if (ret) {
>> +        dev_err(dev, "couldn't add unregister action for intel_cht_usb_mux pdev\n");
>> +        return ret;
>> +    }
>> +
>> +    return 0;
>> +}
>> diff --git a/drivers/usb/host/xhci-pci.c b/drivers/usb/host/xhci-pci.c
>> index 8071c8fdd15e..b55c1e96abf0 100644
>> --- a/drivers/usb/host/xhci-pci.c
>> +++ b/drivers/usb/host/xhci-pci.c
>> @@ -188,6 +188,7 @@ static void xhci_pci_quirks(struct device *dev, struct xhci_hcd *xhci)
>>       if (pdev->vendor == PCI_VENDOR_ID_INTEL &&
>>            pdev->device == PCI_DEVICE_ID_INTEL_CHERRYVIEW_XHCI) {
>>           xhci->quirks |= XHCI_SSIC_PORT_UNUSED;
>> +        xhci->quirks |= XHCI_INTEL_CHT_USB_MUX;
>>       }
>>       if (pdev->vendor == PCI_VENDOR_ID_INTEL &&
>>           (pdev->device == PCI_DEVICE_ID_INTEL_CHERRYVIEW_XHCI ||
>> @@ -328,6 +329,9 @@ static int xhci_pci_probe(struct pci_dev *dev, const struct pci_device_id *id)
>>       if (xhci->quirks & XHCI_PME_STUCK_QUIRK)
>>           xhci_pme_acpi_rtd3_enable(dev);
>>
>> +    if (xhci->quirks & XHCI_INTEL_CHT_USB_MUX)
>> +        xhci_create_intel_cht_mux_pdev(xhci);
>> +
>>       /* USB-2 and USB-3 roothubs initialized, allow runtime pm suspend */
>>       pm_runtime_put_noidle(&dev->dev);
>>
>> diff --git a/drivers/usb/host/xhci.h b/drivers/usb/host/xhci.h
>> index e3e935291ed6..f722ee31e50d 100644
>> --- a/drivers/usb/host/xhci.h
>> +++ b/drivers/usb/host/xhci.h
>> @@ -1821,6 +1821,7 @@ struct xhci_hcd {
>>   #define XHCI_LIMIT_ENDPOINT_INTERVAL_7    (1 << 26)
>>   #define XHCI_U2_DISABLE_WAKE    (1 << 27)
>>   #define XHCI_ASMEDIA_MODIFY_FLOWCONTROL    (1 << 28)
>> +#define XHCI_INTEL_CHT_USB_MUX    (1 << 29)
> 
> Everything you said about the quirk flags in v1 is true, and I hate to
> make the bulk of xhci even more PCI dependent, but I'm running out of flag bits and
> would like a better solution than the current quirk flags we have.
> 
> This isn't really your concern, this patch is just doing it the same way it has
> always been done, but if you have a better idea I'm all ears.
> 
> -Mathias

Re: [PATCH v2 04/11] usb: xhci: Add Intel cherrytrail extended cap / otg phy mux handling

[Mathias Nyman]

[-- Attachment #1: Type: text/plain, Size: 2041 bytes --]

Hi,

sorry about the long delay

On 07.09.2017 18:49, Hans de Goede wrote:
> Hi,
>
> On 07-09-17 15:14, Mathias Nyman wrote:
>> On 05.09.2017 19:42, Hans de Goede wrote:
>>> The Intel cherrytrail xhci controller has an extended cap mmio-range
>>> which contains registers to control the muxing to the xhci (host mode)
>>> or the dwc3 (device mode) and vbus-detection for the otg usb-phy.
>>>
>>> Having a mux driver included in the xhci code (or under drivers/usb/host)
>>> is not desirable. So this commit adds a simple handler for this extended
>>> capability, which creates a platform device with the caps mmio region as
>>> resource, this allows us to write a separate platform mux driver for the
>>> mux.
>>>
>> I think it would be better to have one place where we add handlers for
>> vendor specific extended capabilities.
>>
>> Something like xhci-vendor-ext-caps.c, or just xhci-ext-caps.c as
>> there's a xhci-ext-caps.h header already
>>
>> We could walk through the capability list once and add the needed handlers.
>> Something like:
>>
>> +int xhci_ext_cap_init(void __iomem *base)
>
> This will need to take a struct xhci_hcd *xhci param instead
> as some of the ext_cap handling (including the cht mux code)
> will need access to this.
>

yes, sample code added in second patch for reference/testing.

>
> So I see 2 options here (without making this function PCI specific)
> 1) Add an u32 product_id field to struct xhci_hcd; or
> 2) Use a quirk flag as my current code is doing.
>
> I'm fine with doing this either way, please let me know your preference.

Lets go with the quirk for now, I'll sort that out later

>
> Can you do a "git format-patch" of that and send it to me? If you
> can give me that + your preference for how to check if we're
> dealing with a cht xhci hcd in xhci_ext_cap_init I can do a v3
> with your suggestions applied.

Ended up modifying xhci_find_next_ext_cap() using id = 0 for
the next capability in list. Patch attached,

Second patch is just for reference how to use it.

Thanks
-Mathias



[-- Attachment #2: 0001-xhci-Add-option-to-get-next-extended-capability-in-l.patch --]
[-- Type: text/x-patch, Size: 1730 bytes --]

>From d5f26c1595f211ea7d46fca91551f70d1207330d Mon Sep 17 00:00:00 2001
From: Mathias Nyman <mathias.nyman@linux.intel.com>
Date: Tue, 19 Sep 2017 14:54:58 +0300
Subject: [PATCH 1/2] xhci: Add option to get next extended capability in list
 by passing id = 0

Modify xhci_find_next_ext_cap(base, offset, id) to return the next
capability offset if 0 is passed for id. Otherwise it will behave as
previously and return the offset of the next capability with matching id

capability id 0 is not used by xhci (reserved)

This is useful when we want to loop through all capabilities.

Signed-off-by: Mathias Nyman <mathias.nyman@linux.intel.com>
---
 drivers/usb/host/xhci-ext-caps.h | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/usb/host/xhci-ext-caps.h b/drivers/usb/host/xhci-ext-caps.h
index 28deea5..c1b4042 100644
--- a/drivers/usb/host/xhci-ext-caps.h
+++ b/drivers/usb/host/xhci-ext-caps.h
@@ -96,7 +96,8 @@
  * @base	PCI MMIO registers base address.
  * @start	address at which to start looking, (0 or HCC_PARAMS to start at
  *		beginning of list)
- * @id		Extended capability ID to search for.
+ * @id		Extended capability ID to search for, or 0 for the next
+ *		capability
  *
  * Returns the offset of the next matching extended capability structure.
  * Some capabilities can occur several times, e.g., the XHCI_EXT_CAPS_PROTOCOL,
@@ -122,7 +123,7 @@ static inline int xhci_find_next_ext_cap(void __iomem *base, u32 start, int id)
 		val = readl(base + offset);
 		if (val == ~0)
 			return 0;
-		if (XHCI_EXT_CAPS_ID(val) == id && offset != start)
+		if (offset != start && (id == 0 || XHCI_EXT_CAPS_ID(val) == id))
 			return offset;
 
 		next = XHCI_EXT_CAPS_NEXT(val);
-- 
1.9.1


[-- Attachment #3: 0002-xhci-test-xhci_find_next_ext_cap-with-0-id.patch --]
[-- Type: text/x-patch, Size: 2274 bytes --]

>From da44e961605d382829f90fdcfb90b61fa5ca9590 Mon Sep 17 00:00:00 2001
From: Mathias Nyman <mathias.nyman@linux.intel.com>
Date: Tue, 19 Sep 2017 14:58:40 +0300
Subject: [PATCH 2/2] xhci: test xhci_find_next_ext_cap with 0 id

NOT for UPSTREAM, just testing/reference code

Signed-off-by: Mathias Nyman <mathias.nyman@linux.intel.com>
---
 drivers/usb/host/xhci-ext-caps.h |  3 +++
 drivers/usb/host/xhci.c          | 30 ++++++++++++++++++++++++++++++
 2 files changed, 33 insertions(+)

diff --git a/drivers/usb/host/xhci-ext-caps.h b/drivers/usb/host/xhci-ext-caps.h
index c1b4042..7deb9e7 100644
--- a/drivers/usb/host/xhci-ext-caps.h
+++ b/drivers/usb/host/xhci-ext-caps.h
@@ -51,6 +51,9 @@
 #define XHCI_EXT_CAPS_ROUTE	5
 /* IDs 6-9 reserved */
 #define XHCI_EXT_CAPS_DEBUG	10
+/* IDs 192 - 255 vendor specific extensions */
+#define XHCI_EXT_CAPS_VENDOR_INTEL 192
+
 /* USB Legacy Support Capability - section 7.1.1 */
 #define XHCI_HC_BIOS_OWNED	(1 << 16)
 #define XHCI_HC_OS_OWNED	(1 << 24)
diff --git a/drivers/usb/host/xhci.c b/drivers/usb/host/xhci.c
index 870093a..99c804a 100644
--- a/drivers/usb/host/xhci.c
+++ b/drivers/usb/host/xhci.c
@@ -4772,6 +4772,34 @@ static int xhci_get_frame(struct usb_hcd *hcd)
 	return readl(&xhci->run_regs->microframe_index) >> 3;
 }
 
+int xhci_ext_cap_init(struct xhci_hcd *xhci)
+{
+	void __iomem *base;
+	u32 cap_offset;
+	u32 val;
+
+	base = &xhci->cap_regs->hc_capbase;
+	cap_offset = xhci_find_next_ext_cap(base, 0, 0);
+
+	do {
+		val = readl(base + cap_offset);
+
+		switch (XHCI_EXT_CAPS_ID(val)) {
+		case XHCI_EXT_CAPS_VENDOR_INTEL:
+			printk(KERN_ERR "TEST EXT_CAPS_VENDOR_INTEL\n");
+			break;
+		default:
+			break;
+		}
+
+		printk(KERN_ERR "TEST EXT_CAP id %d\n", XHCI_EXT_CAPS_ID(val));
+
+		cap_offset = xhci_find_next_ext_cap(base, cap_offset, 0);
+	} while (cap_offset);
+
+	return 0;
+}
+
 int xhci_gen_setup(struct usb_hcd *hcd, xhci_get_quirks_t get_quirks)
 {
 	struct xhci_hcd		*xhci;
@@ -4886,6 +4914,8 @@ int xhci_gen_setup(struct usb_hcd *hcd, xhci_get_quirks_t get_quirks)
 		dma_set_coherent_mask(dev, DMA_BIT_MASK(32));
 	}
 
+	xhci_ext_cap_init(xhci);
+
 	xhci_dbg(xhci, "Calling HCD init\n");
 	/* Initialize HCD and host controller data structures. */
 	retval = xhci_init(hcd);
-- 
1.9.1

Re: [PATCH v2 04/11] usb: xhci: Add Intel cherrytrail extended cap / otg phy mux handling

[Hans de Goede]

Hi,

On 19-09-17 14:40, Mathias Nyman wrote:
> Hi,
> 
> sorry about the long delay
> 
> On 07.09.2017 18:49, Hans de Goede wrote:
>> Hi,
>>
>> On 07-09-17 15:14, Mathias Nyman wrote:
>>> On 05.09.2017 19:42, Hans de Goede wrote:
>>>> The Intel cherrytrail xhci controller has an extended cap mmio-range
>>>> which contains registers to control the muxing to the xhci (host mode)
>>>> or the dwc3 (device mode) and vbus-detection for the otg usb-phy.
>>>>
>>>> Having a mux driver included in the xhci code (or under drivers/usb/host)
>>>> is not desirable. So this commit adds a simple handler for this extended
>>>> capability, which creates a platform device with the caps mmio region as
>>>> resource, this allows us to write a separate platform mux driver for the
>>>> mux.
>>>>
>>> I think it would be better to have one place where we add handlers for
>>> vendor specific extended capabilities.
>>>
>>> Something like xhci-vendor-ext-caps.c, or just xhci-ext-caps.c as
>>> there's a xhci-ext-caps.h header already
>>>
>>> We could walk through the capability list once and add the needed handlers.
>>> Something like:
>>>
>>> +int xhci_ext_cap_init(void __iomem *base)
>>
>> This will need to take a struct xhci_hcd *xhci param instead
>> as some of the ext_cap handling (including the cht mux code)
>> will need access to this.
>>
> 
> yes, sample code added in second patch for reference/testing.
> 
>>
>> So I see 2 options here (without making this function PCI specific)
>> 1) Add an u32 product_id field to struct xhci_hcd; or
>> 2) Use a quirk flag as my current code is doing.
>>
>> I'm fine with doing this either way, please let me know your preference.
> 
> Lets go with the quirk for now, I'll sort that out later
> 
>>
>> Can you do a "git format-patch" of that and send it to me? If you
>> can give me that + your preference for how to check if we're
>> dealing with a cht xhci hcd in xhci_ext_cap_init I can do a v3
>> with your suggestions applied.
> 
> Ended up modifying xhci_find_next_ext_cap() using id = 0 for
> the next capability in list. Patch attached,
> 
> Second patch is just for reference how to use it.

Thank you for the patches, I'm working on prepping a v3 of
this series which includes and uses the first patch.

Regards,

Hans

Re: [PATCH v2 04/11] usb: xhci: Add Intel cherrytrail extended cap / otg phy mux handling

[Peter Rosin]

On 2017-09-05 18:42, Hans de Goede wrote:
> The Intel cherrytrail xhci controller has an extended cap mmio-range
> which contains registers to control the muxing to the xhci (host mode)
> or the dwc3 (device mode) and vbus-detection for the otg usb-phy.
> 
> Having a mux driver included in the xhci code (or under drivers/usb/host)
> is not desirable. So this commit adds a simple handler for this extended
> capability, which creates a platform device with the caps mmio region as
> resource, this allows us to write a separate platform mux driver for the
> mux.
> 
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> ---
> Changes in v2:
> -Check xHCI controller PCI device-id instead of only checking for the
>  Intel Extended capability ID, as the Extended capability ID is used on
>  other model Intel xHCI controllers too
> ---
>  drivers/usb/host/Makefile            |  2 +-
>  drivers/usb/host/xhci-intel-quirks.c | 85 ++++++++++++++++++++++++++++++++++++
>  drivers/usb/host/xhci-pci.c          |  4 ++
>  drivers/usb/host/xhci.h              |  2 +
>  4 files changed, 92 insertions(+), 1 deletion(-)
>  create mode 100644 drivers/usb/host/xhci-intel-quirks.c
> 
> diff --git a/drivers/usb/host/Makefile b/drivers/usb/host/Makefile
> index cf2691fffcc0..441edf82eb1c 100644
> --- a/drivers/usb/host/Makefile
> +++ b/drivers/usb/host/Makefile
> @@ -63,7 +63,7 @@ obj-$(CONFIG_USB_OHCI_HCD_DAVINCI)	+= ohci-da8xx.o
>  obj-$(CONFIG_USB_UHCI_HCD)	+= uhci-hcd.o
>  obj-$(CONFIG_USB_FHCI_HCD)	+= fhci.o
>  obj-$(CONFIG_USB_XHCI_HCD)	+= xhci-hcd.o
> -obj-$(CONFIG_USB_XHCI_PCI)	+= xhci-pci.o
> +obj-$(CONFIG_USB_XHCI_PCI)	+= xhci-pci.o xhci-intel-quirks.o
>  obj-$(CONFIG_USB_XHCI_PLATFORM) += xhci-plat-hcd.o
>  obj-$(CONFIG_USB_XHCI_MTK)	+= xhci-mtk.o
>  obj-$(CONFIG_USB_XHCI_TEGRA)	+= xhci-tegra.o
> diff --git a/drivers/usb/host/xhci-intel-quirks.c b/drivers/usb/host/xhci-intel-quirks.c
> new file mode 100644
> index 000000000000..819f5f9da9ee
> --- /dev/null
> +++ b/drivers/usb/host/xhci-intel-quirks.c
> @@ -0,0 +1,85 @@
> +/*
> + * Intel Vendor Defined XHCI extended capability handling
> + *
> + * Copyright (c) 2016) Hans de Goede <hdegoede@redhat.com>

2017? And drop the stray bracket.

Cheers,
Peter

[PATCH v2 05/11] mux: Add Intel Cherrytrail USB mux driver

[Hans de Goede]

Intel Cherrytrail SoCs have an internal USB mux for muxing the otg-port
USB data lines between the xHCI host controller and the dwc3 gadget
controller. On some Cherrytrail systems this mux is controlled through
AML code reacting on a GPIO IRQ connected to the USB OTG id pin (through
an _AIE ACPI method) so things just work.

But on other Cherrytrail systems we need to control the mux ourselves
this driver exports the mux through the mux subsys, so that other drivers
can control it if necessary.

This driver also updates the vbus-valid reporting to the dwc3 gadget
controller, as this uses the same registers as the mux. This is needed
for gadget/device mode to work properly (even on systems which control
the mux from their AML code).

Note this depends on the xhci driver registering a platform device
named "intel_cht_usb_mux", which has an IOMEM resource 0 which points
to the Intel Vendor Defined XHCI extended capabilities region.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
Changes in v2:
-Complete rewrite as a stand-alone platform-driver rather then as a phy
 driver, since this is just a mux, not a phy

Changes in v3:
-Make this a mux subsys driver instead of listening to USB_HOST extcon
 cable events and responding to those

Changes in v4 (of patch, v2 of new mux based series):
-Rename C-file to use - in name
-Add MAINTAINERS entry
-Various code-style fixes
---
 MAINTAINERS                     |   5 +
 drivers/mux/Kconfig             |  11 ++
 drivers/mux/Makefile            |  10 +-
 drivers/mux/intel-cht-usb-mux.c | 257 ++++++++++++++++++++++++++++++++++++++++
 4 files changed, 279 insertions(+), 4 deletions(-)
 create mode 100644 drivers/mux/intel-cht-usb-mux.c

diff --git a/MAINTAINERS b/MAINTAINERS
index 14a2fd905217..dfaed958db85 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -8991,6 +8991,11 @@ F:	include/linux/dt-bindings/mux/
 F:	include/linux/mux/
 F:	drivers/mux/
 
+MULTIPLEXER SUBSYSTEM INTEL CHT USB MUX DRIVER
+M:	Hans de Goede <hdegoede@redhat.com>
+S:	Maintained
+F:	drivers/mix/intel-cht-usb-mux.c
+
 MULTISOUND SOUND DRIVER
 M:	Andrew Veliath <andrewtv@usa.net>
 S:	Maintained
diff --git a/drivers/mux/Kconfig b/drivers/mux/Kconfig
index 19e4e904c9bf..947cfd7af02a 100644
--- a/drivers/mux/Kconfig
+++ b/drivers/mux/Kconfig
@@ -34,6 +34,17 @@ config MUX_GPIO
 	  To compile the driver as a module, choose M here: the module will
 	  be called mux-gpio.
 
+config MUX_INTEL_CHT_USB_MUX
+	tristate "Intel Cherrytrail USB Multiplexer"
+	depends on ACPI && X86 && EXTCON
+	help
+	  This driver adds support for the internal USB mux for muxing the OTG
+	  USB data lines between the xHCI host controller and the dwc3 gadget
+	  controller found on Intel Cherrytrail SoCs.
+
+	  To compile the driver as a module, choose M here: the module will
+	  be called mux-intel_cht_usb_mux.
+
 config MUX_MMIO
 	tristate "MMIO register bitfield-controlled Multiplexer"
 	depends on (OF && MFD_SYSCON) || COMPILE_TEST
diff --git a/drivers/mux/Makefile b/drivers/mux/Makefile
index 0e1e59760e3f..6cf41be2754f 100644
--- a/drivers/mux/Makefile
+++ b/drivers/mux/Makefile
@@ -5,9 +5,11 @@
 mux-core-objs			:= core.o
 mux-adg792a-objs		:= adg792a.o
 mux-gpio-objs			:= gpio.o
+mux-intel-cht-usb-mux-objs	:= intel-cht-usb-mux.o
 mux-mmio-objs			:= mmio.o
 
-obj-$(CONFIG_MULTIPLEXER)	+= mux-core.o
-obj-$(CONFIG_MUX_ADG792A)	+= mux-adg792a.o
-obj-$(CONFIG_MUX_GPIO)		+= mux-gpio.o
-obj-$(CONFIG_MUX_MMIO)		+= mux-mmio.o
+obj-$(CONFIG_MULTIPLEXER)		+= mux-core.o
+obj-$(CONFIG_MUX_ADG792A)		+= mux-adg792a.o
+obj-$(CONFIG_MUX_GPIO)			+= mux-gpio.o
+obj-$(CONFIG_MUX_INTEL_CHT_USB_MUX)	+= mux-intel-cht-usb-mux.o
+obj-$(CONFIG_MUX_MMIO)			+= mux-mmio.o
diff --git a/drivers/mux/intel-cht-usb-mux.c b/drivers/mux/intel-cht-usb-mux.c
new file mode 100644
index 000000000000..9cd1a1f5027f
--- /dev/null
+++ b/drivers/mux/intel-cht-usb-mux.c
@@ -0,0 +1,257 @@
+/*
+ * Intel Cherrytrail USB OTG MUX driver
+ *
+ * Copyright (c) 2016 Hans de Goede <hdegoede@redhat.com>
+ *
+ * Loosely based on android x86 kernel code which is:
+ *
+ * Copyright (C) 2014 Intel Corp.
+ *
+ * Author: Wu, Hao
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation, or (at your option)
+ * any later version.
+ */
+
+#include <linux/acpi.h>
+#include <linux/delay.h>
+#include <linux/err.h>
+#include <linux/extcon.h>
+#include <linux/io.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/mux/driver.h>
+#include <linux/mux/usb.h>
+#include <linux/platform_device.h>
+#include <linux/workqueue.h>
+
+/* register definition */
+#define DUAL_ROLE_CFG0			0x68
+#define SW_VBUS_VALID			(1 << 24)
+#define SW_IDPIN_EN			(1 << 21)
+#define SW_IDPIN			(1 << 20)
+
+#define DUAL_ROLE_CFG1			0x6c
+#define HOST_MODE			(1 << 29)
+
+#define DUAL_ROLE_CFG1_POLL_TIMEOUT	1000
+
+#define DRV_NAME			"intel_cht_usb_mux"
+
+struct intel_cht_usb_data {
+	struct mutex cfg0_lock;
+	void __iomem *base;
+	struct extcon_dev *vbus_extcon;
+	struct notifier_block vbus_nb;
+	struct work_struct vbus_work;
+};
+
+struct intel_cht_extcon_info {
+	const char *hid;
+	int hrv;
+	const char *extcon;
+};
+
+static const struct intel_cht_extcon_info vbus_providers[] = {
+	{ .hid = "INT33F4", .hrv = -1, .extcon = "axp288_extcon" },
+	{ .hid = "INT34D3", .hrv =  3, .extcon = "cht_wcove_pwrsrc" },
+};
+
+static const unsigned int vbus_cable_ids[] = {
+	EXTCON_CHG_USB_SDP, EXTCON_CHG_USB_CDP, EXTCON_CHG_USB_DCP,
+	EXTCON_CHG_USB_ACA, EXTCON_CHG_USB_FAST,
+};
+
+static int intel_cht_usb_set_mux(struct mux_control *mux, int state)
+{
+	struct intel_cht_usb_data *data = mux_chip_priv(mux->chip);
+	unsigned long timeout;
+	bool host_mode;
+	u32 val;
+
+	mutex_lock(&data->cfg0_lock);
+
+	/* Set idpin value as requested */
+	val = readl(data->base + DUAL_ROLE_CFG0);
+	if (state == MUX_USB_DEVICE) {
+		val |= SW_IDPIN;
+		host_mode = false;
+	} else {
+		val &= ~SW_IDPIN;
+		host_mode = true;
+	}
+	val |= SW_IDPIN_EN;
+	writel(val, data->base + DUAL_ROLE_CFG0);
+
+	mutex_unlock(&data->cfg0_lock);
+
+	/* In most case it takes about 600ms to finish mode switching */
+	timeout = jiffies + msecs_to_jiffies(DUAL_ROLE_CFG1_POLL_TIMEOUT);
+
+	/* Polling on CFG1 register to confirm mode switch.*/
+	while (1) {
+		val = readl(data->base + DUAL_ROLE_CFG1);
+		if (!!(val & HOST_MODE) == host_mode)
+			break;
+
+		/* Interval for polling is set to about 5 - 10 ms */
+		usleep_range(5000, 10000);
+
+		if (time_after(jiffies, timeout)) {
+			dev_warn(&mux->chip->dev,
+				 "Timeout waiting for mux to switch\n");
+			break;
+		}
+	}
+
+	return 0;
+}
+
+static void intel_cht_usb_set_vbus_valid(struct intel_cht_usb_data *data,
+					     bool valid)
+{
+	u32 val;
+
+	mutex_lock(&data->cfg0_lock);
+
+	val = readl(data->base + DUAL_ROLE_CFG0);
+	if (valid)
+		val |= SW_VBUS_VALID;
+	else
+		val &= ~SW_VBUS_VALID;
+
+	val |= SW_IDPIN_EN;
+	writel(val, data->base + DUAL_ROLE_CFG0);
+
+	mutex_unlock(&data->cfg0_lock);
+}
+
+static void intel_cht_usb_vbus_work(struct work_struct *work)
+{
+	struct intel_cht_usb_data *data =
+		container_of(work, struct intel_cht_usb_data, vbus_work);
+	bool vbus_present = false;
+	int i;
+
+	for (i = 0; i < ARRAY_SIZE(vbus_cable_ids); i++) {
+		if (extcon_get_state(data->vbus_extcon, vbus_cable_ids[i]) > 0) {
+			vbus_present = true;
+			break;
+		}
+	}
+
+	intel_cht_usb_set_vbus_valid(data, vbus_present);
+}
+
+static int intel_cht_usb_vbus_extcon_evt(struct notifier_block *nb,
+					     unsigned long event, void *param)
+{
+	struct intel_cht_usb_data *data =
+		container_of(nb, struct intel_cht_usb_data, vbus_nb);
+
+	schedule_work(&data->vbus_work);
+
+	return NOTIFY_OK;
+}
+
+static const struct mux_control_ops intel_cht_usb_ops = {
+	.set = intel_cht_usb_set_mux,
+};
+
+static int intel_cht_usb_probe(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	struct intel_cht_usb_data *data;
+	struct mux_chip *mux_chip;
+	struct resource *res;
+	resource_size_t size;
+	int i, ret;
+
+	mux_chip = devm_mux_chip_alloc(dev, 1, sizeof(*data));
+	if (IS_ERR(mux_chip))
+		return PTR_ERR(mux_chip);
+
+	mux_chip->ops = &intel_cht_usb_ops;
+	mux_chip->mux[0].idle_state = MUX_USB_DEVICE;
+	/* Keep initial state as is, for e.g. booting from an USB disk */
+	mux_chip->mux[0].cached_state = MUX_USB_DEVICE;
+	mux_chip->mux[0].states = MUX_USB_STATES;
+	data = mux_chip_priv(mux_chip);
+	mutex_init(&data->cfg0_lock);
+
+	/*
+	 * Besides controlling the mux we also need to control the vbus_valid
+	 * flag for device/gadget mode to work properly. To do this we monitor
+	 * the extcon interface exported by the PMIC drivers for the PMICs used
+	 * with the Cherry Trail SoC.
+	 *
+	 * We try to get the extcon_dev before registering the mux as this
+	 * may lead to us exiting with -EPROBE_DEFER.
+	 */
+	for (i = 0 ; i < ARRAY_SIZE(vbus_providers); i++) {
+		if (!acpi_dev_present(vbus_providers[i].hid, NULL,
+				      vbus_providers[i].hrv))
+			continue;
+
+		data->vbus_extcon = extcon_get_extcon_dev(
+						vbus_providers[i].extcon);
+		if (data->vbus_extcon == NULL)
+			return -EPROBE_DEFER;
+
+		dev_info(dev, "using extcon '%s' for vbus-valid\n",
+			 vbus_providers[i].extcon);
+		break;
+	}
+
+	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	size = (res->end + 1) - res->start;
+	data->base = devm_ioremap_nocache(dev, res->start, size);
+	if (IS_ERR(data->base)) {
+		ret = PTR_ERR(data->base);
+		dev_err(dev, "can't iomap registers: %d\n", ret);
+		return ret;
+	}
+
+	ret = devm_mux_chip_register(dev, mux_chip);
+	if (ret < 0)
+		return ret;
+
+	if (data->vbus_extcon) {
+		INIT_WORK(&data->vbus_work, intel_cht_usb_vbus_work);
+		data->vbus_nb.notifier_call = intel_cht_usb_vbus_extcon_evt;
+		ret = devm_extcon_register_notifier_all(dev, data->vbus_extcon,
+							&data->vbus_nb);
+		if (ret) {
+			dev_err(dev, "can't register vbus extcon notifier: %d\n",
+				ret);
+			return ret;
+		}
+
+		/* Sync initial mode */
+		schedule_work(&data->vbus_work);
+	}
+
+	return 0;
+}
+
+static const struct platform_device_id intel_cht_usb_table[] = {
+	{ .name = DRV_NAME },
+	{},
+};
+MODULE_DEVICE_TABLE(platform, intel_cht_usb_table);
+
+static struct platform_driver intel_cht_usb_driver = {
+	.driver = {
+		.name = DRV_NAME,
+	},
+	.id_table = intel_cht_usb_table,
+	.probe = intel_cht_usb_probe,
+};
+
+module_platform_driver(intel_cht_usb_driver);
+
+MODULE_AUTHOR("Hans de Goede <hdegoede@redhat.com>");
+MODULE_DESCRIPTION("Intel Cherrytrail USB mux driver");
+MODULE_LICENSE("GPL");
-- 
2.13.5

Re: [PATCH v2 05/11] mux: Add Intel Cherrytrail USB mux driver

[Peter Rosin]

On 2017-09-05 18:42, Hans de Goede wrote:
> Intel Cherrytrail SoCs have an internal USB mux for muxing the otg-port
> USB data lines between the xHCI host controller and the dwc3 gadget
> controller. On some Cherrytrail systems this mux is controlled through
> AML code reacting on a GPIO IRQ connected to the USB OTG id pin (through
> an _AIE ACPI method) so things just work.
> 
> But on other Cherrytrail systems we need to control the mux ourselves
> this driver exports the mux through the mux subsys, so that other drivers
> can control it if necessary.
> 
> This driver also updates the vbus-valid reporting to the dwc3 gadget
> controller, as this uses the same registers as the mux. This is needed
> for gadget/device mode to work properly (even on systems which control
> the mux from their AML code).
> 
> Note this depends on the xhci driver registering a platform device
> named "intel_cht_usb_mux", which has an IOMEM resource 0 which points
> to the Intel Vendor Defined XHCI extended capabilities region.
> 
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> ---
> Changes in v2:
> -Complete rewrite as a stand-alone platform-driver rather then as a phy
>  driver, since this is just a mux, not a phy
> 
> Changes in v3:
> -Make this a mux subsys driver instead of listening to USB_HOST extcon
>  cable events and responding to those
> 
> Changes in v4 (of patch, v2 of new mux based series):
> -Rename C-file to use - in name
> -Add MAINTAINERS entry
> -Various code-style fixes
> ---
>  MAINTAINERS                     |   5 +
>  drivers/mux/Kconfig             |  11 ++
>  drivers/mux/Makefile            |  10 +-
>  drivers/mux/intel-cht-usb-mux.c | 257 ++++++++++++++++++++++++++++++++++++++++
>  4 files changed, 279 insertions(+), 4 deletions(-)
>  create mode 100644 drivers/mux/intel-cht-usb-mux.c
> 
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 14a2fd905217..dfaed958db85 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -8991,6 +8991,11 @@ F:	include/linux/dt-bindings/mux/
>  F:	include/linux/mux/
>  F:	drivers/mux/
>  
> +MULTIPLEXER SUBSYSTEM INTEL CHT USB MUX DRIVER
> +M:	Hans de Goede <hdegoede@redhat.com>
> +S:	Maintained
> +F:	drivers/mix/intel-cht-usb-mux.c

s/mix/mux/

(also in patch 06/11)

> +
>  MULTISOUND SOUND DRIVER
>  M:	Andrew Veliath <andrewtv@usa.net>
>  S:	Maintained
> diff --git a/drivers/mux/Kconfig b/drivers/mux/Kconfig
> index 19e4e904c9bf..947cfd7af02a 100644
> --- a/drivers/mux/Kconfig
> +++ b/drivers/mux/Kconfig
> @@ -34,6 +34,17 @@ config MUX_GPIO
>  	  To compile the driver as a module, choose M here: the module will
>  	  be called mux-gpio.
>  
> +config MUX_INTEL_CHT_USB_MUX
> +	tristate "Intel Cherrytrail USB Multiplexer"
> +	depends on ACPI && X86 && EXTCON
> +	help
> +	  This driver adds support for the internal USB mux for muxing the OTG
> +	  USB data lines between the xHCI host controller and the dwc3 gadget
> +	  controller found on Intel Cherrytrail SoCs.
> +
> +	  To compile the driver as a module, choose M here: the module will
> +	  be called mux-intel_cht_usb_mux.

s/_/-/g

> +
>  config MUX_MMIO
>  	tristate "MMIO register bitfield-controlled Multiplexer"
>  	depends on (OF && MFD_SYSCON) || COMPILE_TEST
> diff --git a/drivers/mux/Makefile b/drivers/mux/Makefile
> index 0e1e59760e3f..6cf41be2754f 100644
> --- a/drivers/mux/Makefile
> +++ b/drivers/mux/Makefile
> @@ -5,9 +5,11 @@
>  mux-core-objs			:= core.o
>  mux-adg792a-objs		:= adg792a.o
>  mux-gpio-objs			:= gpio.o
> +mux-intel-cht-usb-mux-objs	:= intel-cht-usb-mux.o
>  mux-mmio-objs			:= mmio.o
>  
> -obj-$(CONFIG_MULTIPLEXER)	+= mux-core.o
> -obj-$(CONFIG_MUX_ADG792A)	+= mux-adg792a.o
> -obj-$(CONFIG_MUX_GPIO)		+= mux-gpio.o
> -obj-$(CONFIG_MUX_MMIO)		+= mux-mmio.o
> +obj-$(CONFIG_MULTIPLEXER)		+= mux-core.o
> +obj-$(CONFIG_MUX_ADG792A)		+= mux-adg792a.o
> +obj-$(CONFIG_MUX_GPIO)			+= mux-gpio.o
> +obj-$(CONFIG_MUX_INTEL_CHT_USB_MUX)	+= mux-intel-cht-usb-mux.o
> +obj-$(CONFIG_MUX_MMIO)			+= mux-mmio.o
> diff --git a/drivers/mux/intel-cht-usb-mux.c b/drivers/mux/intel-cht-usb-mux.c
> new file mode 100644
> index 000000000000..9cd1a1f5027f
> --- /dev/null
> +++ b/drivers/mux/intel-cht-usb-mux.c
> @@ -0,0 +1,257 @@
> +/*
> + * Intel Cherrytrail USB OTG MUX driver
> + *
> + * Copyright (c) 2016 Hans de Goede <hdegoede@redhat.com>

2017?

> + *
> + * Loosely based on android x86 kernel code which is:
> + *
> + * Copyright (C) 2014 Intel Corp.
> + *
> + * Author: Wu, Hao
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 as
> + * published by the Free Software Foundation, or (at your option)
> + * any later version.
> + */
> +
> +#include <linux/acpi.h>
> +#include <linux/delay.h>
> +#include <linux/err.h>
> +#include <linux/extcon.h>
> +#include <linux/io.h>
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/mux/driver.h>
> +#include <linux/mux/usb.h>
> +#include <linux/platform_device.h>
> +#include <linux/workqueue.h>
> +
> +/* register definition */
> +#define DUAL_ROLE_CFG0			0x68
> +#define SW_VBUS_VALID			(1 << 24)
> +#define SW_IDPIN_EN			(1 << 21)
> +#define SW_IDPIN			(1 << 20)
> +
> +#define DUAL_ROLE_CFG1			0x6c
> +#define HOST_MODE			(1 << 29)
> +
> +#define DUAL_ROLE_CFG1_POLL_TIMEOUT	1000
> +
> +#define DRV_NAME			"intel_cht_usb_mux"
> +
> +struct intel_cht_usb_data {
> +	struct mutex cfg0_lock;
> +	void __iomem *base;
> +	struct extcon_dev *vbus_extcon;
> +	struct notifier_block vbus_nb;
> +	struct work_struct vbus_work;
> +};
> +
> +struct intel_cht_extcon_info {
> +	const char *hid;
> +	int hrv;
> +	const char *extcon;
> +};
> +
> +static const struct intel_cht_extcon_info vbus_providers[] = {
> +	{ .hid = "INT33F4", .hrv = -1, .extcon = "axp288_extcon" },
> +	{ .hid = "INT34D3", .hrv =  3, .extcon = "cht_wcove_pwrsrc" },
> +};
> +
> +static const unsigned int vbus_cable_ids[] = {
> +	EXTCON_CHG_USB_SDP, EXTCON_CHG_USB_CDP, EXTCON_CHG_USB_DCP,
> +	EXTCON_CHG_USB_ACA, EXTCON_CHG_USB_FAST,
> +};
> +
> +static int intel_cht_usb_set_mux(struct mux_control *mux, int state)
> +{
> +	struct intel_cht_usb_data *data = mux_chip_priv(mux->chip);
> +	unsigned long timeout;
> +	bool host_mode;
> +	u32 val;
> +
> +	mutex_lock(&data->cfg0_lock);
> +
> +	/* Set idpin value as requested */
> +	val = readl(data->base + DUAL_ROLE_CFG0);
> +	if (state == MUX_USB_DEVICE) {
> +		val |= SW_IDPIN;
> +		host_mode = false;
> +	} else {
> +		val &= ~SW_IDPIN;
> +		host_mode = true;
> +	}
> +	val |= SW_IDPIN_EN;
> +	writel(val, data->base + DUAL_ROLE_CFG0);
> +
> +	mutex_unlock(&data->cfg0_lock);
> +
> +	/* In most case it takes about 600ms to finish mode switching */
> +	timeout = jiffies + msecs_to_jiffies(DUAL_ROLE_CFG1_POLL_TIMEOUT);
> +
> +	/* Polling on CFG1 register to confirm mode switch.*/
> +	while (1) {
> +		val = readl(data->base + DUAL_ROLE_CFG1);
> +		if (!!(val & HOST_MODE) == host_mode)
> +			break;
> +
> +		/* Interval for polling is set to about 5 - 10 ms */
> +		usleep_range(5000, 10000);
> +
> +		if (time_after(jiffies, timeout)) {
> +			dev_warn(&mux->chip->dev,
> +				 "Timeout waiting for mux to switch\n");
> +			break;
> +		}
> +	}
> +
> +	return 0;
> +}

I think what Andy was after was something like this (untested):

	do {
		val = readl(data->base + DUAL_ROLE_CFG1);
		if (!!(val & HOST_MODE) == host_mode)
			return 0;

		/* Interval for polling is set to about 5 - 10 ms */
		usleep_range(5000, 10000);
	} while (time_before(jiffies, timeout));

	dev_warn(&mux->chip->dev, "Timeout waiting for mux to switch\n");
	return -ETIMEDOUT;
}

Note that I changed the return value to fail on timeout. I don't know
why you didn't pass on the failure before?

> +
> +static void intel_cht_usb_set_vbus_valid(struct intel_cht_usb_data *data,
> +					     bool valid)
> +{
> +	u32 val;
> +
> +	mutex_lock(&data->cfg0_lock);
> +
> +	val = readl(data->base + DUAL_ROLE_CFG0);
> +	if (valid)
> +		val |= SW_VBUS_VALID;
> +	else
> +		val &= ~SW_VBUS_VALID;
> +
> +	val |= SW_IDPIN_EN;
> +	writel(val, data->base + DUAL_ROLE_CFG0);
> +
> +	mutex_unlock(&data->cfg0_lock);
> +}
> +
> +static void intel_cht_usb_vbus_work(struct work_struct *work)
> +{
> +	struct intel_cht_usb_data *data =
> +		container_of(work, struct intel_cht_usb_data, vbus_work);
> +	bool vbus_present = false;
> +	int i;
> +
> +	for (i = 0; i < ARRAY_SIZE(vbus_cable_ids); i++) {
> +		if (extcon_get_state(data->vbus_extcon, vbus_cable_ids[i]) > 0) {
> +			vbus_present = true;
> +			break;
> +		}
> +	}
> +
> +	intel_cht_usb_set_vbus_valid(data, vbus_present);
> +}
> +
> +static int intel_cht_usb_vbus_extcon_evt(struct notifier_block *nb,
> +					     unsigned long event, void *param)
> +{
> +	struct intel_cht_usb_data *data =
> +		container_of(nb, struct intel_cht_usb_data, vbus_nb);
> +
> +	schedule_work(&data->vbus_work);
> +
> +	return NOTIFY_OK;
> +}
> +
> +static const struct mux_control_ops intel_cht_usb_ops = {
> +	.set = intel_cht_usb_set_mux,
> +};
> +
> +static int intel_cht_usb_probe(struct platform_device *pdev)
> +{
> +	struct device *dev = &pdev->dev;
> +	struct intel_cht_usb_data *data;
> +	struct mux_chip *mux_chip;
> +	struct resource *res;
> +	resource_size_t size;
> +	int i, ret;
> +
> +	mux_chip = devm_mux_chip_alloc(dev, 1, sizeof(*data));
> +	if (IS_ERR(mux_chip))
> +		return PTR_ERR(mux_chip);
> +
> +	mux_chip->ops = &intel_cht_usb_ops;
> +	mux_chip->mux[0].idle_state = MUX_USB_DEVICE;
> +	/* Keep initial state as is, for e.g. booting from an USB disk */
> +	mux_chip->mux[0].cached_state = MUX_USB_DEVICE;

This I don't like. The cached_state is owned by the core. But I get that the
initial idle state is special and might also be special for other muxes. So,
I think some sanctioned and explicit way to special case the initial idle
state is the way to go. Honestly, I can't think of more than two sane modes
to initialize a mux though. Either you initialize it to whatever the idle
state is (like what is happening today), or you keep it in it's current
state. So, I'm thinking something like this in the driver should be
sufficient:

	mux_chip->mux[0].init_as_is = true;

I'll reply with a suggested patch implementing that...

[time passes]

When I tried to describe that commit, I realized that I had a very hard
time coming up with something sensible. The problem I had was that if it
is crucial that the mux is left alone, then registering a mux controller
for it may not be the wisest thing to do. Because mux consumers are then
provided with a way to trample on whatever depends on the mux to be in
its specific pre-registration position. The correct thing to do is to wait
with the mux controller registration until it is ok to set it to its idle
state. But maybe that kind of thinking is just utopia?

Another way would be lock the mux controller in a specific state during
registration it. But if there are competing mux consumers it gets hairy
to decide which consumer should be responsible for unselecting it. Seems
like a non-starter to me...

(I obviously have the above comment also for patch 06/11)

Cheers,
Peter

> +	mux_chip->mux[0].states = MUX_USB_STATES;
> +	data = mux_chip_priv(mux_chip);
> +	mutex_init(&data->cfg0_lock);
> +
> +	/*
> +	 * Besides controlling the mux we also need to control the vbus_valid
> +	 * flag for device/gadget mode to work properly. To do this we monitor
> +	 * the extcon interface exported by the PMIC drivers for the PMICs used
> +	 * with the Cherry Trail SoC.
> +	 *
> +	 * We try to get the extcon_dev before registering the mux as this
> +	 * may lead to us exiting with -EPROBE_DEFER.
> +	 */
> +	for (i = 0 ; i < ARRAY_SIZE(vbus_providers); i++) {
> +		if (!acpi_dev_present(vbus_providers[i].hid, NULL,
> +				      vbus_providers[i].hrv))
> +			continue;
> +
> +		data->vbus_extcon = extcon_get_extcon_dev(
> +						vbus_providers[i].extcon);
> +		if (data->vbus_extcon == NULL)
> +			return -EPROBE_DEFER;
> +
> +		dev_info(dev, "using extcon '%s' for vbus-valid\n",
> +			 vbus_providers[i].extcon);
> +		break;
> +	}
> +
> +	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> +	size = (res->end + 1) - res->start;
> +	data->base = devm_ioremap_nocache(dev, res->start, size);
> +	if (IS_ERR(data->base)) {
> +		ret = PTR_ERR(data->base);
> +		dev_err(dev, "can't iomap registers: %d\n", ret);
> +		return ret;
> +	}
> +
> +	ret = devm_mux_chip_register(dev, mux_chip);
> +	if (ret < 0)
> +		return ret;
> +
> +	if (data->vbus_extcon) {
> +		INIT_WORK(&data->vbus_work, intel_cht_usb_vbus_work);
> +		data->vbus_nb.notifier_call = intel_cht_usb_vbus_extcon_evt;
> +		ret = devm_extcon_register_notifier_all(dev, data->vbus_extcon,
> +							&data->vbus_nb);
> +		if (ret) {
> +			dev_err(dev, "can't register vbus extcon notifier: %d\n",
> +				ret);
> +			return ret;
> +		}
> +
> +		/* Sync initial mode */
> +		schedule_work(&data->vbus_work);
> +	}
> +
> +	return 0;
> +}
> +
> +static const struct platform_device_id intel_cht_usb_table[] = {
> +	{ .name = DRV_NAME },
> +	{},
> +};
> +MODULE_DEVICE_TABLE(platform, intel_cht_usb_table);
> +
> +static struct platform_driver intel_cht_usb_driver = {
> +	.driver = {
> +		.name = DRV_NAME,
> +	},
> +	.id_table = intel_cht_usb_table,
> +	.probe = intel_cht_usb_probe,
> +};
> +
> +module_platform_driver(intel_cht_usb_driver);
> +
> +MODULE_AUTHOR("Hans de Goede <hdegoede@redhat.com>");
> +MODULE_DESCRIPTION("Intel Cherrytrail USB mux driver");
> +MODULE_LICENSE("GPL");
> 

[PATCH 1/2] mux: add mux_control_get_optional() API

[Peter Rosin]

From: Stephen Boyd <stephen.boyd@linaro.org>

Sometimes drivers only use muxes under certain scenarios. For
example, the chipidea usb controller may be connected to a usb
switch on some platforms, and connected directly to a usb port on
others. The driver won't know one way or the other though, so add
a mux_control_get_optional() API that allows the driver to
differentiate errors getting the mux from there not being a mux
for the driver to use at all.
---
 Documentation/driver-model/devres.txt |   1 +
 drivers/mux/core.c                    | 104 +++++++++++++++++++++++++++-------
 include/linux/mux/consumer.h          |   4 ++
 3 files changed, 89 insertions(+), 20 deletions(-)

I haven't tested this patch, and hence I have not signed it and I also
removed the sign-off from Stephen...

Cheers,
peda

diff --git a/Documentation/driver-model/devres.txt b/Documentation/driver-model/devres.txt
index 30e04f7a690d..4fdd3e63ff8b 100644
--- a/Documentation/driver-model/devres.txt
+++ b/Documentation/driver-model/devres.txt
@@ -342,6 +342,7 @@ MUX
   devm_mux_chip_alloc()
   devm_mux_chip_register()
   devm_mux_control_get()
+  devm_mux_control_get_optional()
 
 PER-CPU MEM
   devm_alloc_percpu()
diff --git a/drivers/mux/core.c b/drivers/mux/core.c
index 2260063b0ea8..2eb234300669 100644
--- a/drivers/mux/core.c
+++ b/drivers/mux/core.c
@@ -289,6 +289,9 @@ EXPORT_SYMBOL_GPL(devm_mux_chip_register);
  */
 unsigned int mux_control_states(struct mux_control *mux)
 {
+	if (!mux)
+		return 0;
+
 	return mux->states;
 }
 EXPORT_SYMBOL_GPL(mux_control_states);
@@ -338,6 +341,9 @@ int mux_control_select(struct mux_control *mux, unsigned int state)
 {
 	int ret;
 
+	if (!mux)
+		return 0;
+
 	ret = down_killable(&mux->lock);
 	if (ret < 0)
 		return ret;
@@ -370,6 +376,9 @@ int mux_control_try_select(struct mux_control *mux, unsigned int state)
 {
 	int ret;
 
+	if (!mux)
+		return 0;
+
 	if (down_trylock(&mux->lock))
 		return -EBUSY;
 
@@ -398,6 +407,9 @@ int mux_control_deselect(struct mux_control *mux)
 {
 	int ret = 0;
 
+	if (!mux)
+		return 0;
+
 	if (mux->idle_state != MUX_IDLE_AS_IS &&
 	    mux->idle_state != mux->cached_state)
 		ret = mux_control_set(mux, mux->idle_state);
@@ -422,14 +434,8 @@ static struct mux_chip *of_find_mux_chip_by_node(struct device_node *np)
 	return dev ? to_mux_chip(dev) : NULL;
 }
 
-/**
- * mux_control_get() - Get the mux-control for a device.
- * @dev: The device that needs a mux-control.
- * @mux_name: The name identifying the mux-control.
- *
- * Return: A pointer to the mux-control, or an ERR_PTR with a negative errno.
- */
-struct mux_control *mux_control_get(struct device *dev, const char *mux_name)
+static struct mux_control *
+__mux_control_get(struct device *dev, const char *mux_name, bool optional)
 {
 	struct device_node *np = dev->of_node;
 	struct of_phandle_args args;
@@ -441,16 +447,22 @@ struct mux_control *mux_control_get(struct device *dev, const char *mux_name)
 	if (mux_name) {
 		index = of_property_match_string(np, "mux-control-names",
 						 mux_name);
+		if ((index == -EINVAL || index == -ENODATA) && optional)
+			return NULL;
 		if (index < 0) {
 			dev_err(dev, "mux controller '%s' not found\n",
 				mux_name);
 			return ERR_PTR(index);
 		}
+		/* OF does point to a mux, so it's no longer optional */
+		optional = false;
 	}
 
 	ret = of_parse_phandle_with_args(np,
 					 "mux-controls", "#mux-control-cells",
 					 index, &args);
+	if (ret == -ENOENT && optional)
+		return NULL;
 	if (ret) {
 		dev_err(dev, "%pOF: failed to get mux-control %s(%i)\n",
 			np, mux_name ?: "", index);
@@ -482,9 +494,36 @@ struct mux_control *mux_control_get(struct device *dev, const char *mux_name)
 	get_device(&mux_chip->dev);
 	return &mux_chip->mux[controller];
 }
+
+/**
+ * mux_control_get() - Get the mux-control for a device.
+ * @dev: The device that needs a mux-control.
+ * @mux_name: The name identifying the mux-control.
+ *
+ * Return: A pointer to the mux-control, or an ERR_PTR with a negative errno.
+ */
+struct mux_control *mux_control_get(struct device *dev, const char *mux_name)
+{
+	return __mux_control_get(dev, mux_name, false);
+}
 EXPORT_SYMBOL_GPL(mux_control_get);
 
 /**
+ * mux_control_get_optional() - Get the optional mux-control for a device.
+ * @dev: The device that needs a mux-control.
+ * @mux_name: The name identifying the mux-control.
+ *
+ * Return: NULL if no mux with the provided name is found, a pointer to
+ * the named mux-control or an ERR_PTR with a negative errno.
+ */
+struct mux_control *
+mux_control_get_optional(struct device *dev, const char *mux_name)
+{
+	return __mux_control_get(dev, mux_name, true);
+}
+EXPORT_SYMBOL_GPL(mux_control_get_optional);
+
+/**
  * mux_control_put() - Put away the mux-control for good.
  * @mux: The mux-control to put away.
  *
@@ -492,6 +531,9 @@ EXPORT_SYMBOL_GPL(mux_control_get);
  */
 void mux_control_put(struct mux_control *mux)
 {
+	if (!mux)
+		return;
+
 	put_device(&mux->chip->dev);
 }
 EXPORT_SYMBOL_GPL(mux_control_put);
@@ -503,16 +545,8 @@ static void devm_mux_control_release(struct device *dev, void *res)
 	mux_control_put(mux);
 }
 
-/**
- * devm_mux_control_get() - Get the mux-control for a device, with resource
- *			    management.
- * @dev: The device that needs a mux-control.
- * @mux_name: The name identifying the mux-control.
- *
- * Return: Pointer to the mux-control, or an ERR_PTR with a negative errno.
- */
-struct mux_control *devm_mux_control_get(struct device *dev,
-					 const char *mux_name)
+static struct mux_control *
+__devm_mux_control_get(struct device *dev, const char *mux_name, bool optional)
 {
 	struct mux_control **ptr, *mux;
 
@@ -520,8 +554,8 @@ struct mux_control *devm_mux_control_get(struct device *dev,
 	if (!ptr)
 		return ERR_PTR(-ENOMEM);
 
-	mux = mux_control_get(dev, mux_name);
-	if (IS_ERR(mux)) {
+	mux = __mux_control_get(dev, mux_name, optional);
+	if (IS_ERR_OR_NULL(mux)) {
 		devres_free(ptr);
 		return mux;
 	}
@@ -531,8 +565,38 @@ struct mux_control *devm_mux_control_get(struct device *dev,
 
 	return mux;
 }
+
+/**
+ * devm_mux_control_get() - Get the mux-control for a device, with resource
+ *			    management.
+ * @dev: The device that needs a mux-control.
+ * @mux_name: The name identifying the mux-control.
+ *
+ * Return: Pointer to the mux-control, or an ERR_PTR with a negative errno.
+ */
+struct mux_control *
+devm_mux_control_get(struct device *dev, const char *mux_name)
+{
+	return __devm_mux_control_get(dev, mux_name, false);
+}
 EXPORT_SYMBOL_GPL(devm_mux_control_get);
 
+/**
+ * devm_mux_control_get_optional() - Get the optional mux-control for a device,
+ *				     with resource management.
+ * @dev: The device that needs a mux-control.
+ * @mux_name: The name identifying the mux-control.
+ *
+ * Return: NULL if no mux with the provided name is found, a pointer to
+ * the named mux-control or an ERR_PTR with a negative errno.
+ */
+struct mux_control *
+devm_mux_control_get_optional(struct device *dev, const char *mux_name)
+{
+	return __devm_mux_control_get(dev, mux_name, true);
+}
+EXPORT_SYMBOL_GPL(devm_mux_control_get_optional);
+
 /*
  * Using subsys_initcall instead of module_init here to try to ensure - for
  * the non-modular case - that the subsystem is initialized when mux consumers
diff --git a/include/linux/mux/consumer.h b/include/linux/mux/consumer.h
index ea96d4c82be7..fc98547bf494 100644
--- a/include/linux/mux/consumer.h
+++ b/include/linux/mux/consumer.h
@@ -26,9 +26,13 @@ int __must_check mux_control_try_select(struct mux_control *mux,
 int mux_control_deselect(struct mux_control *mux);
 
 struct mux_control *mux_control_get(struct device *dev, const char *mux_name);
+struct mux_control *mux_control_get_optional(struct device *dev,
+					     const char *mux_name);
 void mux_control_put(struct mux_control *mux);
 
 struct mux_control *devm_mux_control_get(struct device *dev,
 					 const char *mux_name);
+struct mux_control *devm_mux_control_get_optional(struct device *dev,
+						  const char *mux_name);
 
 #endif /* _LINUX_MUX_CONSUMER_H */
-- 
2.11.0

Re: [PATCH 1/2] mux: add mux_control_get_optional() API

[Peter Rosin]

On 2017-09-08 17:45, Peter Rosin wrote:
> From: Stephen Boyd <stephen.boyd@linaro.org>
> 
> Sometimes drivers only use muxes under certain scenarios. For
> example, the chipidea usb controller may be connected to a usb
> switch on some platforms, and connected directly to a usb port on
> others. The driver won't know one way or the other though, so add
> a mux_control_get_optional() API that allows the driver to
> differentiate errors getting the mux from there not being a mux
> for the driver to use at all.
> ---
>  Documentation/driver-model/devres.txt |   1 +
>  drivers/mux/core.c                    | 104 +++++++++++++++++++++++++++-------
>  include/linux/mux/consumer.h          |   4 ++
>  3 files changed, 89 insertions(+), 20 deletions(-)
> 
> I haven't tested this patch, and hence I have not signed it and I also
> removed the sign-off from Stephen...

Huh, I definitely intended to mention that this patch is based on [1]
from Stephen, but that I've made changes according to the comments in
that thread (and more). And those changes are what made me remove the
sign-off from Stephen...

Sorry for the noise,
Peter

[1] https://lkml.org/lkml/2017/7/14/655

Re: [PATCH 1/2] mux: add mux_control_get_optional() API

[Hans de Goede]

Hi,

On 09/08/2017 05:54 PM, Peter Rosin wrote:
> On 2017-09-08 17:45, Peter Rosin wrote:
>> From: Stephen Boyd <stephen.boyd@linaro.org>
>>
>> Sometimes drivers only use muxes under certain scenarios. For
>> example, the chipidea usb controller may be connected to a usb
>> switch on some platforms, and connected directly to a usb port on
>> others. The driver won't know one way or the other though, so add
>> a mux_control_get_optional() API that allows the driver to
>> differentiate errors getting the mux from there not being a mux
>> for the driver to use at all.
>> ---
>>   Documentation/driver-model/devres.txt |   1 +
>>   drivers/mux/core.c                    | 104 +++++++++++++++++++++++++++-------
>>   include/linux/mux/consumer.h          |   4 ++
>>   3 files changed, 89 insertions(+), 20 deletions(-)
>>
>> I haven't tested this patch, and hence I have not signed it and I also
>> removed the sign-off from Stephen...
> 
> Huh, I definitely intended to mention that this patch is based on [1]
> from Stephen, but that I've made changes according to the comments in
> that thread (and more). And those changes are what made me remove the
> sign-off from Stephen...

AFAIK normally a Signed-off-by is kept if some (small-ish) changes
are made. The S-o-b is mostly an indication that the author is
ok with adding the code to the kernel under the GPL.

Anyways, Stephen are you ok with re-adding your S-o-b to
the version modified by Peter?

Regards,

Hans

Re: [PATCH 1/2] mux: add mux_control_get_optional() API

[Stephen Boyd]

Quoting Hans de Goede (2017-09-19 11:35:50)
> Hi,
> 
> On 09/08/2017 05:54 PM, Peter Rosin wrote:
> > On 2017-09-08 17:45, Peter Rosin wrote:
> >> From: Stephen Boyd <stephen.boyd@linaro.org>
> >>
> >> Sometimes drivers only use muxes under certain scenarios. For
> >> example, the chipidea usb controller may be connected to a usb
> >> switch on some platforms, and connected directly to a usb port on
> >> others. The driver won't know one way or the other though, so add
> >> a mux_control_get_optional() API that allows the driver to
> >> differentiate errors getting the mux from there not being a mux
> >> for the driver to use at all.
> >> ---
> >>   Documentation/driver-model/devres.txt |   1 +
> >>   drivers/mux/core.c                    | 104 +++++++++++++++++++++++++++-------
> >>   include/linux/mux/consumer.h          |   4 ++
> >>   3 files changed, 89 insertions(+), 20 deletions(-)
> >>
> >> I haven't tested this patch, and hence I have not signed it and I also
> >> removed the sign-off from Stephen...
> > 
> > Huh, I definitely intended to mention that this patch is based on [1]
> > from Stephen, but that I've made changes according to the comments in
> > that thread (and more). And those changes are what made me remove the
> > sign-off from Stephen...
> 
> AFAIK normally a Signed-off-by is kept if some (small-ish) changes
> are made. The S-o-b is mostly an indication that the author is
> ok with adding the code to the kernel under the GPL.
> 
> Anyways, Stephen are you ok with re-adding your S-o-b to
> the version modified by Peter?
> 

Please add my sign off

Signed-off-by: Stephen Boyd <stephen.boyd@linaro.org>

and I would expect Peter to have one as well with a maintainer tag
indicating what changed from my patch.

[PATCH 2/2] mux: add explicit hook to leave the mux as-is on init/registration

[Peter Rosin]

A board may need a mux controller to stay as-is for a while longer, e.g.
if setting the normally preferred idle state destroys booting.

The mechanism provided here is not perfect in two ways.
1. As soon as the mux controller is registered, some mux consumer can
   access it and set a state that destroys booting all the same.
2. The mux controller might linger in a state that is not the
   preferred idle state indefinitely, if no mux consumer ever selects
   and then deselects the mux.
---
 drivers/mux/core.c         | 3 +++
 include/linux/mux/driver.h | 4 ++++
 2 files changed, 7 insertions(+)

Untested -> no sign-off, and I'm also really unsure about it because of
the imperfections mentioned in the commit message.

Cheers,
peda

diff --git a/drivers/mux/core.c b/drivers/mux/core.c
index 2eb234300669..c81db7d4f9c8 100644
--- a/drivers/mux/core.c
+++ b/drivers/mux/core.c
@@ -155,6 +155,9 @@ int mux_chip_register(struct mux_chip *mux_chip)
 	for (i = 0; i < mux_chip->controllers; ++i) {
 		struct mux_control *mux = &mux_chip->mux[i];
 
+		if (mux->init_as_is)
+			continue;
+
 		if (mux->idle_state == mux->cached_state)
 			continue;
 
diff --git a/include/linux/mux/driver.h b/include/linux/mux/driver.h
index 35c3579c3304..21cf6041a962 100644
--- a/include/linux/mux/driver.h
+++ b/include/linux/mux/driver.h
@@ -36,6 +36,9 @@ struct mux_control_ops {
  * @states:		The number of mux controller states.
  * @idle_state:		The mux controller state to use when inactive, or one
  *			of MUX_IDLE_AS_IS and MUX_IDLE_DISCONNECT.
+ * @init_as_is:		Set to true to have the core leave the mux controller
+ *			state as-is until first selection. If @idle_state is
+ *			MUX_IDLE_AS_IS, @init_as_is is irrelevant.
  *
  * Mux drivers may only change @states and @idle_state, and may only do so
  * between allocation and registration of the mux controller. Specifically,
@@ -50,6 +53,7 @@ struct mux_control {
 
 	unsigned int states;
 	int idle_state;
+	bool init_as_is;
 };
 
 /**
-- 
2.11.0

Re: [PATCH v2 05/11] mux: Add Intel Cherrytrail USB mux driver

[Hans de Goede]

Hi,

Thank you for the reviews and patches!

On 09/08/2017 05:45 PM, Peter Rosin wrote:
> On 2017-09-05 18:42, Hans de Goede wrote:
>> Intel Cherrytrail SoCs have an internal USB mux for muxing the otg-port
>> USB data lines between the xHCI host controller and the dwc3 gadget
>> controller. On some Cherrytrail systems this mux is controlled through
>> AML code reacting on a GPIO IRQ connected to the USB OTG id pin (through
>> an _AIE ACPI method) so things just work.
>>
>> But on other Cherrytrail systems we need to control the mux ourselves
>> this driver exports the mux through the mux subsys, so that other drivers
>> can control it if necessary.
>>
>> This driver also updates the vbus-valid reporting to the dwc3 gadget
>> controller, as this uses the same registers as the mux. This is needed
>> for gadget/device mode to work properly (even on systems which control
>> the mux from their AML code).
>>
>> Note this depends on the xhci driver registering a platform device
>> named "intel_cht_usb_mux", which has an IOMEM resource 0 which points
>> to the Intel Vendor Defined XHCI extended capabilities region.
>>
>> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
>> ---
>> Changes in v2:
>> -Complete rewrite as a stand-alone platform-driver rather then as a phy
>>   driver, since this is just a mux, not a phy
>>
>> Changes in v3:
>> -Make this a mux subsys driver instead of listening to USB_HOST extcon
>>   cable events and responding to those
>>
>> Changes in v4 (of patch, v2 of new mux based series):
>> -Rename C-file to use - in name
>> -Add MAINTAINERS entry
>> -Various code-style fixes
>> ---
>>   MAINTAINERS                     |   5 +
>>   drivers/mux/Kconfig             |  11 ++
>>   drivers/mux/Makefile            |  10 +-
>>   drivers/mux/intel-cht-usb-mux.c | 257 ++++++++++++++++++++++++++++++++++++++++
>>   4 files changed, 279 insertions(+), 4 deletions(-)
>>   create mode 100644 drivers/mux/intel-cht-usb-mux.c
>>
>> diff --git a/MAINTAINERS b/MAINTAINERS
>> index 14a2fd905217..dfaed958db85 100644
>> --- a/MAINTAINERS
>> +++ b/MAINTAINERS
>> @@ -8991,6 +8991,11 @@ F:	include/linux/dt-bindings/mux/
>>   F:	include/linux/mux/
>>   F:	drivers/mux/
>>   
>> +MULTIPLEXER SUBSYSTEM INTEL CHT USB MUX DRIVER
>> +M:	Hans de Goede <hdegoede@redhat.com>
>> +S:	Maintained
>> +F:	drivers/mix/intel-cht-usb-mux.c
> 
> s/mix/mux/
> 
> (also in patch 06/11)
> 
>> +
>>   MULTISOUND SOUND DRIVER
>>   M:	Andrew Veliath <andrewtv@usa.net>
>>   S:	Maintained
>> diff --git a/drivers/mux/Kconfig b/drivers/mux/Kconfig
>> index 19e4e904c9bf..947cfd7af02a 100644
>> --- a/drivers/mux/Kconfig
>> +++ b/drivers/mux/Kconfig
>> @@ -34,6 +34,17 @@ config MUX_GPIO
>>   	  To compile the driver as a module, choose M here: the module will
>>   	  be called mux-gpio.
>>   
>> +config MUX_INTEL_CHT_USB_MUX
>> +	tristate "Intel Cherrytrail USB Multiplexer"
>> +	depends on ACPI && X86 && EXTCON
>> +	help
>> +	  This driver adds support for the internal USB mux for muxing the OTG
>> +	  USB data lines between the xHCI host controller and the dwc3 gadget
>> +	  controller found on Intel Cherrytrail SoCs.
>> +
>> +	  To compile the driver as a module, choose M here: the module will
>> +	  be called mux-intel_cht_usb_mux.
> 
> s/_/-/g
> 
>> +
>>   config MUX_MMIO
>>   	tristate "MMIO register bitfield-controlled Multiplexer"
>>   	depends on (OF && MFD_SYSCON) || COMPILE_TEST
>> diff --git a/drivers/mux/Makefile b/drivers/mux/Makefile
>> index 0e1e59760e3f..6cf41be2754f 100644
>> --- a/drivers/mux/Makefile
>> +++ b/drivers/mux/Makefile
>> @@ -5,9 +5,11 @@
>>   mux-core-objs			:= core.o
>>   mux-adg792a-objs		:= adg792a.o
>>   mux-gpio-objs			:= gpio.o
>> +mux-intel-cht-usb-mux-objs	:= intel-cht-usb-mux.o
>>   mux-mmio-objs			:= mmio.o
>>   
>> -obj-$(CONFIG_MULTIPLEXER)	+= mux-core.o
>> -obj-$(CONFIG_MUX_ADG792A)	+= mux-adg792a.o
>> -obj-$(CONFIG_MUX_GPIO)		+= mux-gpio.o
>> -obj-$(CONFIG_MUX_MMIO)		+= mux-mmio.o
>> +obj-$(CONFIG_MULTIPLEXER)		+= mux-core.o
>> +obj-$(CONFIG_MUX_ADG792A)		+= mux-adg792a.o
>> +obj-$(CONFIG_MUX_GPIO)			+= mux-gpio.o
>> +obj-$(CONFIG_MUX_INTEL_CHT_USB_MUX)	+= mux-intel-cht-usb-mux.o
>> +obj-$(CONFIG_MUX_MMIO)			+= mux-mmio.o
>> diff --git a/drivers/mux/intel-cht-usb-mux.c b/drivers/mux/intel-cht-usb-mux.c
>> new file mode 100644
>> index 000000000000..9cd1a1f5027f
>> --- /dev/null
>> +++ b/drivers/mux/intel-cht-usb-mux.c
>> @@ -0,0 +1,257 @@
>> +/*
>> + * Intel Cherrytrail USB OTG MUX driver
>> + *
>> + * Copyright (c) 2016 Hans de Goede <hdegoede@redhat.com>
> 
> 2017?

Actually I stated working on this (in a different form before the mux framework was
merge) quite a while back I will make this 2016-2017.

> 
>> + *
>> + * Loosely based on android x86 kernel code which is:
>> + *
>> + * Copyright (C) 2014 Intel Corp.
>> + *
>> + * Author: Wu, Hao
>> + *
>> + * This program is free software; you can redistribute it and/or modify
>> + * it under the terms of the GNU General Public License version 2 as
>> + * published by the Free Software Foundation, or (at your option)
>> + * any later version.
>> + */
>> +
>> +#include <linux/acpi.h>
>> +#include <linux/delay.h>
>> +#include <linux/err.h>
>> +#include <linux/extcon.h>
>> +#include <linux/io.h>
>> +#include <linux/kernel.h>
>> +#include <linux/module.h>
>> +#include <linux/mux/driver.h>
>> +#include <linux/mux/usb.h>
>> +#include <linux/platform_device.h>
>> +#include <linux/workqueue.h>
>> +
>> +/* register definition */
>> +#define DUAL_ROLE_CFG0			0x68
>> +#define SW_VBUS_VALID			(1 << 24)
>> +#define SW_IDPIN_EN			(1 << 21)
>> +#define SW_IDPIN			(1 << 20)
>> +
>> +#define DUAL_ROLE_CFG1			0x6c
>> +#define HOST_MODE			(1 << 29)
>> +
>> +#define DUAL_ROLE_CFG1_POLL_TIMEOUT	1000
>> +
>> +#define DRV_NAME			"intel_cht_usb_mux"
>> +
>> +struct intel_cht_usb_data {
>> +	struct mutex cfg0_lock;
>> +	void __iomem *base;
>> +	struct extcon_dev *vbus_extcon;
>> +	struct notifier_block vbus_nb;
>> +	struct work_struct vbus_work;
>> +};
>> +
>> +struct intel_cht_extcon_info {
>> +	const char *hid;
>> +	int hrv;
>> +	const char *extcon;
>> +};
>> +
>> +static const struct intel_cht_extcon_info vbus_providers[] = {
>> +	{ .hid = "INT33F4", .hrv = -1, .extcon = "axp288_extcon" },
>> +	{ .hid = "INT34D3", .hrv =  3, .extcon = "cht_wcove_pwrsrc" },
>> +};
>> +
>> +static const unsigned int vbus_cable_ids[] = {
>> +	EXTCON_CHG_USB_SDP, EXTCON_CHG_USB_CDP, EXTCON_CHG_USB_DCP,
>> +	EXTCON_CHG_USB_ACA, EXTCON_CHG_USB_FAST,
>> +};
>> +
>> +static int intel_cht_usb_set_mux(struct mux_control *mux, int state)
>> +{
>> +	struct intel_cht_usb_data *data = mux_chip_priv(mux->chip);
>> +	unsigned long timeout;
>> +	bool host_mode;
>> +	u32 val;
>> +
>> +	mutex_lock(&data->cfg0_lock);
>> +
>> +	/* Set idpin value as requested */
>> +	val = readl(data->base + DUAL_ROLE_CFG0);
>> +	if (state == MUX_USB_DEVICE) {
>> +		val |= SW_IDPIN;
>> +		host_mode = false;
>> +	} else {
>> +		val &= ~SW_IDPIN;
>> +		host_mode = true;
>> +	}
>> +	val |= SW_IDPIN_EN;
>> +	writel(val, data->base + DUAL_ROLE_CFG0);
>> +
>> +	mutex_unlock(&data->cfg0_lock);
>> +
>> +	/* In most case it takes about 600ms to finish mode switching */
>> +	timeout = jiffies + msecs_to_jiffies(DUAL_ROLE_CFG1_POLL_TIMEOUT);
>> +
>> +	/* Polling on CFG1 register to confirm mode switch.*/
>> +	while (1) {
>> +		val = readl(data->base + DUAL_ROLE_CFG1);
>> +		if (!!(val & HOST_MODE) == host_mode)
>> +			break;
>> +
>> +		/* Interval for polling is set to about 5 - 10 ms */
>> +		usleep_range(5000, 10000);
>> +
>> +		if (time_after(jiffies, timeout)) {
>> +			dev_warn(&mux->chip->dev,
>> +				 "Timeout waiting for mux to switch\n");
>> +			break;
>> +		}
>> +	}
>> +
>> +	return 0;
>> +}
> 
> I think what Andy was after was something like this (untested):
> 
> 	do {
> 		val = readl(data->base + DUAL_ROLE_CFG1);
> 		if (!!(val & HOST_MODE) == host_mode)
> 			return 0;
> 
> 		/* Interval for polling is set to about 5 - 10 ms */
> 		usleep_range(5000, 10000);
> 	} while (time_before(jiffies, timeout));
> 
> 	dev_warn(&mux->chip->dev, "Timeout waiting for mux to switch\n");
> 	return -ETIMEDOUT;
> }
> 
> Note that I changed the return value to fail on timeout. I don't know
> why you didn't pass on the failure before?

Because in the pre mux-framework version this was void and then when
I changed it I did not think things true.

>> +
>> +static void intel_cht_usb_set_vbus_valid(struct intel_cht_usb_data *data,
>> +					     bool valid)
>> +{
>> +	u32 val;
>> +
>> +	mutex_lock(&data->cfg0_lock);
>> +
>> +	val = readl(data->base + DUAL_ROLE_CFG0);
>> +	if (valid)
>> +		val |= SW_VBUS_VALID;
>> +	else
>> +		val &= ~SW_VBUS_VALID;
>> +
>> +	val |= SW_IDPIN_EN;
>> +	writel(val, data->base + DUAL_ROLE_CFG0);
>> +
>> +	mutex_unlock(&data->cfg0_lock);
>> +}
>> +
>> +static void intel_cht_usb_vbus_work(struct work_struct *work)
>> +{
>> +	struct intel_cht_usb_data *data =
>> +		container_of(work, struct intel_cht_usb_data, vbus_work);
>> +	bool vbus_present = false;
>> +	int i;
>> +
>> +	for (i = 0; i < ARRAY_SIZE(vbus_cable_ids); i++) {
>> +		if (extcon_get_state(data->vbus_extcon, vbus_cable_ids[i]) > 0) {
>> +			vbus_present = true;
>> +			break;
>> +		}
>> +	}
>> +
>> +	intel_cht_usb_set_vbus_valid(data, vbus_present);
>> +}
>> +
>> +static int intel_cht_usb_vbus_extcon_evt(struct notifier_block *nb,
>> +					     unsigned long event, void *param)
>> +{
>> +	struct intel_cht_usb_data *data =
>> +		container_of(nb, struct intel_cht_usb_data, vbus_nb);
>> +
>> +	schedule_work(&data->vbus_work);
>> +
>> +	return NOTIFY_OK;
>> +}
>> +
>> +static const struct mux_control_ops intel_cht_usb_ops = {
>> +	.set = intel_cht_usb_set_mux,
>> +};
>> +
>> +static int intel_cht_usb_probe(struct platform_device *pdev)
>> +{
>> +	struct device *dev = &pdev->dev;
>> +	struct intel_cht_usb_data *data;
>> +	struct mux_chip *mux_chip;
>> +	struct resource *res;
>> +	resource_size_t size;
>> +	int i, ret;
>> +
>> +	mux_chip = devm_mux_chip_alloc(dev, 1, sizeof(*data));
>> +	if (IS_ERR(mux_chip))
>> +		return PTR_ERR(mux_chip);
>> +
>> +	mux_chip->ops = &intel_cht_usb_ops;
>> +	mux_chip->mux[0].idle_state = MUX_USB_DEVICE;
>> +	/* Keep initial state as is, for e.g. booting from an USB disk */
>> +	mux_chip->mux[0].cached_state = MUX_USB_DEVICE;
> 
> This I don't like. The cached_state is owned by the core. But I get that the
> initial idle state is special and might also be special for other muxes. So,
> I think some sanctioned and explicit way to special case the initial idle
> state is the way to go. Honestly, I can't think of more than two sane modes
> to initialize a mux though. Either you initialize it to whatever the idle
> state is (like what is happening today), or you keep it in it's current
> state. So, I'm thinking something like this in the driver should be
> sufficient:
> 
> 	mux_chip->mux[0].init_as_is = true;
> 
> I'll reply with a suggested patch implementing that...
> 
> [time passes]
> 
> When I tried to describe that commit, I realized that I had a very hard
> time coming up with something sensible. The problem I had was that if it
> is crucial that the mux is left alone, then registering a mux controller
> for it may not be the wisest thing to do. Because mux consumers are then
> provided with a way to trample on whatever depends on the mux to be in
> its specific pre-registration position. The correct thing to do is to wait
> with the mux controller registration until it is ok to set it to its idle
> state. But maybe that kind of thinking is just utopia?

Yeah that is not going to fly, about your worries:

1. As soon as the mux controller is registered, some mux consumer can
    access it and set a state that destroys booting all the same.

Right, so consumers will need to take care of not messing things up
in cases where the initial mux state needs to be preserved the burden
for this is on the consumers and I don't see this as a problem for
the mux-core.

2. The mux controller might linger in a state that is not the
    preferred idle state indefinitely, if no mux consumer ever selects
    and then deselects the mux.

The same will happen when Linux does not have a driver for the mux,
so if the firmware boots Linux with a mux in a certain state then it
should be safe to keep the mux in that state.

So I think your patch for this is fine.

> Another way would be lock the mux controller in a specific state during
> registration it. But if there are competing mux consumers it gets hairy
> to decide which consumer should be responsible for unselecting it. Seems
> like a non-starter to me...

Ack, I think that the init_as_is flag is a nice KISS solution for this.

Regards,

Hans



> 
> (I obviously have the above comment also for patch 06/11)
> 
> Cheers,
> Peter
> 
>> +	mux_chip->mux[0].states = MUX_USB_STATES;
>> +	data = mux_chip_priv(mux_chip);
>> +	mutex_init(&data->cfg0_lock);
>> +
>> +	/*
>> +	 * Besides controlling the mux we also need to control the vbus_valid
>> +	 * flag for device/gadget mode to work properly. To do this we monitor
>> +	 * the extcon interface exported by the PMIC drivers for the PMICs used
>> +	 * with the Cherry Trail SoC.
>> +	 *
>> +	 * We try to get the extcon_dev before registering the mux as this
>> +	 * may lead to us exiting with -EPROBE_DEFER.
>> +	 */
>> +	for (i = 0 ; i < ARRAY_SIZE(vbus_providers); i++) {
>> +		if (!acpi_dev_present(vbus_providers[i].hid, NULL,
>> +				      vbus_providers[i].hrv))
>> +			continue;
>> +
>> +		data->vbus_extcon = extcon_get_extcon_dev(
>> +						vbus_providers[i].extcon);
>> +		if (data->vbus_extcon == NULL)
>> +			return -EPROBE_DEFER;
>> +
>> +		dev_info(dev, "using extcon '%s' for vbus-valid\n",
>> +			 vbus_providers[i].extcon);
>> +		break;
>> +	}
>> +
>> +	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>> +	size = (res->end + 1) - res->start;
>> +	data->base = devm_ioremap_nocache(dev, res->start, size);
>> +	if (IS_ERR(data->base)) {
>> +		ret = PTR_ERR(data->base);
>> +		dev_err(dev, "can't iomap registers: %d\n", ret);
>> +		return ret;
>> +	}
>> +
>> +	ret = devm_mux_chip_register(dev, mux_chip);
>> +	if (ret < 0)
>> +		return ret;
>> +
>> +	if (data->vbus_extcon) {
>> +		INIT_WORK(&data->vbus_work, intel_cht_usb_vbus_work);
>> +		data->vbus_nb.notifier_call = intel_cht_usb_vbus_extcon_evt;
>> +		ret = devm_extcon_register_notifier_all(dev, data->vbus_extcon,
>> +							&data->vbus_nb);
>> +		if (ret) {
>> +			dev_err(dev, "can't register vbus extcon notifier: %d\n",
>> +				ret);
>> +			return ret;
>> +		}
>> +
>> +		/* Sync initial mode */
>> +		schedule_work(&data->vbus_work);
>> +	}
>> +
>> +	return 0;
>> +}
>> +
>> +static const struct platform_device_id intel_cht_usb_table[] = {
>> +	{ .name = DRV_NAME },
>> +	{},
>> +};
>> +MODULE_DEVICE_TABLE(platform, intel_cht_usb_table);
>> +
>> +static struct platform_driver intel_cht_usb_driver = {
>> +	.driver = {
>> +		.name = DRV_NAME,
>> +	},
>> +	.id_table = intel_cht_usb_table,
>> +	.probe = intel_cht_usb_probe,
>> +};
>> +
>> +module_platform_driver(intel_cht_usb_driver);
>> +
>> +MODULE_AUTHOR("Hans de Goede <hdegoede@redhat.com>");
>> +MODULE_DESCRIPTION("Intel Cherrytrail USB mux driver");
>> +MODULE_LICENSE("GPL");
>>

[PATCH v2 06/11] mux: Add Pericom PI3USB30532 Type-C mux driver

[Hans de Goede]

Add a driver for the Pericom PI3USB30532 Type-C cross switch /
mux chip found on some devices with a Type-C port.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
Changes in v2:
-Adjust for new MUX_TYPEC_foo state defines
-Add MAINTAINERS entry
-Various code-style fixes
---
 MAINTAINERS               |  5 +++
 drivers/mux/Kconfig       | 10 +++++
 drivers/mux/Makefile      |  2 +
 drivers/mux/pi3usb30532.c | 93 +++++++++++++++++++++++++++++++++++++++++++++++
 4 files changed, 110 insertions(+)
 create mode 100644 drivers/mux/pi3usb30532.c

diff --git a/MAINTAINERS b/MAINTAINERS
index dfaed958db85..5527b0efcef3 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -8996,6 +8996,11 @@ M:	Hans de Goede <hdegoede@redhat.com>
 S:	Maintained
 F:	drivers/mix/intel-cht-usb-mux.c
 
+MULTIPLEXER SUBSYSTEM PI3USB30532 DRIVER
+M:	Hans de Goede <hdegoede@redhat.com>
+S:	Maintained
+F:	drivers/mix/pi3usb30532.c
+
 MULTISOUND SOUND DRIVER
 M:	Andrew Veliath <andrewtv@usa.net>
 S:	Maintained
diff --git a/drivers/mux/Kconfig b/drivers/mux/Kconfig
index 947cfd7af02a..1f6f017ee920 100644
--- a/drivers/mux/Kconfig
+++ b/drivers/mux/Kconfig
@@ -58,4 +58,14 @@ config MUX_MMIO
 	  To compile the driver as a module, choose M here: the module will
 	  be called mux-mmio.
 
+config MUX_PI3USB30532
+	tristate "Pericom PI3USB30532 Type-C cross switch driver"
+	depends on I2C
+	help
+	  This driver adds support for the Pericom PI3USB30532 Type-C cross
+	  switch / mux chip found on some devices with a Type-C port.
+
+	  To compile the driver as a module, choose M here: the module will
+	  be called mux-pi3usb30532.
+
 endmenu
diff --git a/drivers/mux/Makefile b/drivers/mux/Makefile
index 6cf41be2754f..df381c219708 100644
--- a/drivers/mux/Makefile
+++ b/drivers/mux/Makefile
@@ -7,9 +7,11 @@ mux-adg792a-objs		:= adg792a.o
 mux-gpio-objs			:= gpio.o
 mux-intel-cht-usb-mux-objs	:= intel-cht-usb-mux.o
 mux-mmio-objs			:= mmio.o
+mux-pi3usb30532-objs		:= pi3usb30532.o
 
 obj-$(CONFIG_MULTIPLEXER)		+= mux-core.o
 obj-$(CONFIG_MUX_ADG792A)		+= mux-adg792a.o
 obj-$(CONFIG_MUX_GPIO)			+= mux-gpio.o
 obj-$(CONFIG_MUX_INTEL_CHT_USB_MUX)	+= mux-intel-cht-usb-mux.o
 obj-$(CONFIG_MUX_MMIO)			+= mux-mmio.o
+obj-$(CONFIG_MUX_PI3USB30532)		+= mux-pi3usb30532.o
diff --git a/drivers/mux/pi3usb30532.c b/drivers/mux/pi3usb30532.c
new file mode 100644
index 000000000000..3d959e3ee8c6
--- /dev/null
+++ b/drivers/mux/pi3usb30532.c
@@ -0,0 +1,93 @@
+/*
+ * Pericom PI3USB30532 Type-C cross switch / mux driver
+ *
+ * Copyright (c) 2017 Hans de Goede <hdegoede@redhat.com>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation, or (at your option)
+ * any later version.
+ */
+
+#include <linux/i2c.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/mux/driver.h>
+#include <linux/mux/usb.h>
+
+#define PI3USB30532_CONF			0x00
+
+#define PI3USB30532_CONF_OPEN			0x00
+#define PI3USB30532_CONF_SWAP			0x01
+#define PI3USB30532_CONF_4LANE_DP		0x02
+#define PI3USB30532_CONF_USB3			0x04
+#define PI3USB30532_CONF_USB3_AND_2LANE_DP	0x06
+
+static int pi3usb30532_set_mux(struct mux_control *mux, int state)
+{
+	struct i2c_client *i2c = to_i2c_client(mux->chip->dev.parent);
+	u8 conf = PI3USB30532_CONF_OPEN;
+
+	if (state == MUX_IDLE_DISCONNECT)
+		return i2c_smbus_write_byte_data(i2c, PI3USB30532_CONF, conf);
+
+	switch (state & ~MUX_TYPEC_POLARITY_INV) {
+	case MUX_TYPEC_DEVICE:
+	case MUX_TYPEC_HOST:
+		conf = PI3USB30532_CONF_USB3;
+		break;
+	case MUX_TYPEC_HOST_AND_DP_SRC:
+		conf = PI3USB30532_CONF_USB3_AND_2LANE_DP;
+		break;
+	case MUX_TYPEC_DP_SRC:
+		conf = PI3USB30532_CONF_4LANE_DP;
+		break;
+	}
+
+	if (state & MUX_TYPEC_POLARITY_INV)
+		conf |= PI3USB30532_CONF_SWAP;
+
+	return i2c_smbus_write_byte_data(i2c, PI3USB30532_CONF, conf);
+}
+
+static const struct mux_control_ops pi3usb30532_ops = {
+	.set = pi3usb30532_set_mux,
+};
+
+static int pi3usb30532_probe(struct i2c_client *client)
+{
+	struct device *dev = &client->dev;
+	struct mux_chip *mux_chip;
+
+	mux_chip = devm_mux_chip_alloc(dev, 1, 0);
+	if (IS_ERR(mux_chip))
+		return PTR_ERR(mux_chip);
+
+	mux_chip->ops = &pi3usb30532_ops;
+	mux_chip->mux[0].idle_state = MUX_IDLE_DISCONNECT;
+	/* Keep initial state as is, for e.g. booting from an USB disk */
+	mux_chip->mux[0].cached_state = MUX_IDLE_DISCONNECT;
+	mux_chip->mux[0].states = MUX_TYPEC_STATES;
+
+	return devm_mux_chip_register(dev, mux_chip);
+}
+
+static const struct i2c_device_id pi3usb30532_table[] = {
+	{ "pi3usb30532" },
+	{ }
+};
+MODULE_DEVICE_TABLE(i2c, pi3usb30532_table);
+
+static struct i2c_driver pi3usb30532_driver = {
+	.driver = {
+		.name = "pi3usb30532",
+	},
+	.probe_new = pi3usb30532_probe,
+	.id_table = pi3usb30532_table,
+};
+
+module_i2c_driver(pi3usb30532_driver);
+
+MODULE_AUTHOR("Hans de Goede <hdegoede@redhat.com>");
+MODULE_DESCRIPTION("Pericom PI3USB30532 Type-C mux driver");
+MODULE_LICENSE("GPL");
-- 
2.13.5

[PATCH v2 07/11] extcon: intel-int3496: Add support for controlling the USB-role mux

[Hans de Goede]

Cherry Trail SoCs have a built-in USB-role mux for switching between
the host and device controllers, rather then using an external mux
controller by a GPIO.

There is a driver using the mux-subsys to control this mux, this
commit adds support to the intel-int3496 driver to get a mux_controller
handle for the mux and set the mux through the mux-subsys rather then
through a GPIO.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
Changes in v2:
-Drop || COMPILE_TEST from Kconfig deepnds on, as we will now fail to
 compile on !X86
-Minor code style tweaks
---
 drivers/extcon/Kconfig                |  3 +-
 drivers/extcon/extcon-intel-int3496.c | 59 +++++++++++++++++++++++++++++++++++
 2 files changed, 61 insertions(+), 1 deletion(-)

diff --git a/drivers/extcon/Kconfig b/drivers/extcon/Kconfig
index a7bca4207f44..168f9d710ea0 100644
--- a/drivers/extcon/Kconfig
+++ b/drivers/extcon/Kconfig
@@ -44,7 +44,8 @@ config EXTCON_GPIO
 
 config EXTCON_INTEL_INT3496
 	tristate "Intel INT3496 ACPI device extcon driver"
-	depends on GPIOLIB && ACPI && (X86 || COMPILE_TEST)
+	depends on GPIOLIB && ACPI && X86
+	select MULTIPLEXER
 	help
 	  Say Y here to enable extcon support for USB OTG ports controlled by
 	  an Intel INT3496 ACPI device.
diff --git a/drivers/extcon/extcon-intel-int3496.c b/drivers/extcon/extcon-intel-int3496.c
index 1a45e745717d..3c8e17449c12 100644
--- a/drivers/extcon/extcon-intel-int3496.c
+++ b/drivers/extcon/extcon-intel-int3496.c
@@ -23,8 +23,13 @@
 #include <linux/gpio.h>
 #include <linux/interrupt.h>
 #include <linux/module.h>
+#include <linux/mux/consumer.h>
+#include <linux/mux/usb.h>
 #include <linux/platform_device.h>
 
+#include <asm/cpu_device_id.h>
+#include <asm/intel-family.h>
+
 #define INT3496_GPIO_USB_ID	0
 #define INT3496_GPIO_VBUS_EN	1
 #define INT3496_GPIO_USB_MUX	2
@@ -37,6 +42,8 @@ struct int3496_data {
 	struct gpio_desc *gpio_usb_id;
 	struct gpio_desc *gpio_vbus_en;
 	struct gpio_desc *gpio_usb_mux;
+	struct mux_control *usb_mux;
+	bool usb_mux_set;
 	int usb_id_irq;
 };
 
@@ -56,11 +63,32 @@ static const struct acpi_gpio_mapping acpi_int3496_default_gpios[] = {
 	{ },
 };
 
+static struct mux_lookup acpi_int3496_cht_mux_lookup[] = {
+	{
+		.provider = "intel_cht_usb_mux",
+		.dev_id   = "INT3496:00",
+		.mux_name = "usb-role-mux",
+	},
+};
+
+#define ICPU(model)	{ X86_VENDOR_INTEL, 6, model, X86_FEATURE_ANY, }
+
+static const struct x86_cpu_id cht_cpu_ids[] = {
+	ICPU(INTEL_FAM6_ATOM_AIRMONT),		/* Braswell, Cherry Trail */
+	{}
+};
+
+static bool int3496_soc_has_mux(void)
+{
+	return x86_match_cpu(cht_cpu_ids);
+}
+
 static void int3496_do_usb_id(struct work_struct *work)
 {
 	struct int3496_data *data =
 		container_of(work, struct int3496_data, work.work);
 	int id = gpiod_get_value_cansleep(data->gpio_usb_id);
+	int ret;
 
 	/* id == 1: PERIPHERAL, id == 0: HOST */
 	dev_dbg(data->dev, "Connected %s cable\n", id ? "PERIPHERAL" : "HOST");
@@ -72,6 +100,22 @@ static void int3496_do_usb_id(struct work_struct *work)
 	if (!IS_ERR(data->gpio_usb_mux))
 		gpiod_direction_output(data->gpio_usb_mux, id);
 
+	if (data->usb_mux) {
+		/*
+		 * The mux framework expects multiple competing users, we must
+		 * release our previous setting before applying the new one.
+		 */
+		if (data->usb_mux_set)
+			mux_control_deselect(data->usb_mux);
+
+		ret = mux_control_select(data->usb_mux,
+					 id ? MUX_USB_DEVICE : MUX_USB_HOST);
+		if (ret)
+			dev_err(data->dev, "Error setting mux: %d\n", ret);
+
+		data->usb_mux_set = ret == 0;
+	}
+
 	if (!IS_ERR(data->gpio_vbus_en))
 		gpiod_direction_output(data->gpio_vbus_en, !id);
 
@@ -107,6 +151,21 @@ static int int3496_probe(struct platform_device *pdev)
 	data->dev = dev;
 	INIT_DELAYED_WORK(&data->work, int3496_do_usb_id);
 
+	if (int3496_soc_has_mux()) {
+		mux_add_table(acpi_int3496_cht_mux_lookup,
+			      ARRAY_SIZE(acpi_int3496_cht_mux_lookup));
+		data->usb_mux = devm_mux_control_get(dev, "usb-role-mux");
+		/* Doing this here keeps our error handling clean. */
+		mux_remove_table(acpi_int3496_cht_mux_lookup,
+				 ARRAY_SIZE(acpi_int3496_cht_mux_lookup));
+		if (IS_ERR(data->usb_mux)) {
+			ret = PTR_ERR(data->usb_mux);
+			if (ret != -EPROBE_DEFER)
+				dev_err(dev, "can't get mux: %d\n", ret);
+			return ret;
+		}
+	}
+
 	data->gpio_usb_id = devm_gpiod_get(dev, "id", GPIOD_IN);
 	if (IS_ERR(data->gpio_usb_id)) {
 		ret = PTR_ERR(data->gpio_usb_id);
-- 
2.13.5

[PATCH v2 08/11] staging: typec: tcpm: Set mux to device mode when configured as such

[Hans de Goede]

Setting the mux to TYPEC_MUX_NONE, TCPC_USB_SWITCH_DISCONNECT when the
data-role is device is not correct. Plenty of devices support operating
as USB device through a (separate) USB device controller.

So this commit instead splits out TYPEC_MUX_USB into TYPEC_MUX_USB_HOST
and TYPEC_MUX_USB_DEVICE and makes tcpm_set_roles() set the mux
accordingly.

Likewise TCPC_MUX_DP gets renamed to TCPC_MUX_DP_SRC to make clear that
this is for configuring the Type-C port as a Display Port source, not a
sink.

Last this commit makes tcpm_reset_port() to set the mux to
TYPEC_MUX_NONE, TCPC_USB_SWITCH_DISCONNECT so that it does not and up
staying in host (and with this commit also device) mode after a detach.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/staging/typec/tcpm.c |  7 ++++---
 drivers/staging/typec/tcpm.h | 22 ++++++++++++++--------
 2 files changed, 18 insertions(+), 11 deletions(-)

diff --git a/drivers/staging/typec/tcpm.c b/drivers/staging/typec/tcpm.c
index 8af62e74d54c..ffe7e26d4ed3 100644
--- a/drivers/staging/typec/tcpm.c
+++ b/drivers/staging/typec/tcpm.c
@@ -752,11 +752,11 @@ static int tcpm_set_roles(struct tcpm_port *port, bool attached,
 	int ret;
 
 	if (data == TYPEC_HOST)
-		ret = tcpm_mux_set(port, TYPEC_MUX_USB,
+		ret = tcpm_mux_set(port, TYPEC_MUX_USB_HOST,
 				   TCPC_USB_SWITCH_CONNECT);
 	else
-		ret = tcpm_mux_set(port, TYPEC_MUX_NONE,
-				   TCPC_USB_SWITCH_DISCONNECT);
+		ret = tcpm_mux_set(port, TYPEC_MUX_USB_DEVICE,
+				   TCPC_USB_SWITCH_CONNECT);
 	if (ret < 0)
 		return ret;
 
@@ -2025,6 +2025,7 @@ static void tcpm_reset_port(struct tcpm_port *port)
 	tcpm_init_vconn(port);
 	tcpm_set_current_limit(port, 0, 0);
 	tcpm_set_polarity(port, TYPEC_POLARITY_CC1);
+	tcpm_mux_set(port, TYPEC_MUX_NONE, TCPC_USB_SWITCH_DISCONNECT);
 	tcpm_set_attached_state(port, false);
 	port->try_src_count = 0;
 	port->try_snk_count = 0;
diff --git a/drivers/staging/typec/tcpm.h b/drivers/staging/typec/tcpm.h
index 7e9a6b7b5cd6..f662eed48c86 100644
--- a/drivers/staging/typec/tcpm.h
+++ b/drivers/staging/typec/tcpm.h
@@ -83,17 +83,23 @@ enum tcpc_usb_switch {
 };
 
 /* Mux state attributes */
-#define TCPC_MUX_USB_ENABLED		BIT(0)	/* USB enabled */
-#define TCPC_MUX_DP_ENABLED		BIT(1)	/* DP enabled */
-#define TCPC_MUX_POLARITY_INVERTED	BIT(2)	/* Polarity inverted */
+#define TCPC_MUX_USB_DEVICE_ENABLED		BIT(0)	/* USB device enabled */
+#define TCPC_MUX_USB_HOST_ENABLED		BIT(1)	/* USB host enabled */
+#define TCPC_MUX_DP_SRC_ENABLED			BIT(2)	/* DP enabled */
+#define TCPC_MUX_POLARITY_INVERTED		BIT(3)	/* Polarity inverted */
 
 /* Mux modes, decoded to attributes */
 enum tcpc_mux_mode {
-	TYPEC_MUX_NONE	= 0,				/* Open switch */
-	TYPEC_MUX_USB	= TCPC_MUX_USB_ENABLED,		/* USB only */
-	TYPEC_MUX_DP	= TCPC_MUX_DP_ENABLED,		/* DP only */
-	TYPEC_MUX_DOCK	= TCPC_MUX_USB_ENABLED |	/* Both USB and DP */
-			  TCPC_MUX_DP_ENABLED,
+	/* Open switch */
+	TYPEC_MUX_NONE = 0,
+	/* USB device only */
+	TYPEC_MUX_USB_DEVICE = TCPC_MUX_USB_DEVICE_ENABLED,
+	/* USB host only */
+	TYPEC_MUX_USB_HOST = TCPC_MUX_USB_HOST_ENABLED,
+	/* DP source only */
+	TYPEC_MUX_DP = TCPC_MUX_DP_SRC_ENABLED,
+	/* Both USB host and DP source */
+	TYPEC_MUX_DOCK = TCPC_MUX_USB_HOST_ENABLED | TCPC_MUX_DP_SRC_ENABLED,
 };
 
 struct tcpc_mux_dev {
-- 
2.13.5

Re: [PATCH v2 08/11] staging: typec: tcpm: Set mux to device mode when configured as such

[Guenter Roeck]

On 09/05/2017 09:42 AM, Hans de Goede wrote:
> Setting the mux to TYPEC_MUX_NONE, TCPC_USB_SWITCH_DISCONNECT when the
> data-role is device is not correct. Plenty of devices support operating
> as USB device through a (separate) USB device controller.
> 
> So this commit instead splits out TYPEC_MUX_USB into TYPEC_MUX_USB_HOST
> and TYPEC_MUX_USB_DEVICE and makes tcpm_set_roles() set the mux
> accordingly.
> 
> Likewise TCPC_MUX_DP gets renamed to TCPC_MUX_DP_SRC to make clear that
> this is for configuring the Type-C port as a Display Port source, not a
> sink.
> 
> Last this commit makes tcpm_reset_port() to set the mux to
> TYPEC_MUX_NONE, TCPC_USB_SWITCH_DISCONNECT so that it does not and up
> staying in host (and with this commit also device) mode after a detach.
> 
This sentence is hard to understand.

> Signed-off-by: Hans de Goede <hdegoede@redhat.com>

Otherwise

Reviewed-by: Guenter Roeck <linux@roeck-us.net>

> ---
>   drivers/staging/typec/tcpm.c |  7 ++++---
>   drivers/staging/typec/tcpm.h | 22 ++++++++++++++--------
>   2 files changed, 18 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/staging/typec/tcpm.c b/drivers/staging/typec/tcpm.c
> index 8af62e74d54c..ffe7e26d4ed3 100644
> --- a/drivers/staging/typec/tcpm.c
> +++ b/drivers/staging/typec/tcpm.c
> @@ -752,11 +752,11 @@ static int tcpm_set_roles(struct tcpm_port *port, bool attached,
>   	int ret;
>   
>   	if (data == TYPEC_HOST)
> -		ret = tcpm_mux_set(port, TYPEC_MUX_USB,
> +		ret = tcpm_mux_set(port, TYPEC_MUX_USB_HOST,
>   				   TCPC_USB_SWITCH_CONNECT);
>   	else
> -		ret = tcpm_mux_set(port, TYPEC_MUX_NONE,
> -				   TCPC_USB_SWITCH_DISCONNECT);
> +		ret = tcpm_mux_set(port, TYPEC_MUX_USB_DEVICE,
> +				   TCPC_USB_SWITCH_CONNECT);
>   	if (ret < 0)
>   		return ret;
>   
> @@ -2025,6 +2025,7 @@ static void tcpm_reset_port(struct tcpm_port *port)
>   	tcpm_init_vconn(port);
>   	tcpm_set_current_limit(port, 0, 0);
>   	tcpm_set_polarity(port, TYPEC_POLARITY_CC1);
> +	tcpm_mux_set(port, TYPEC_MUX_NONE, TCPC_USB_SWITCH_DISCONNECT);
>   	tcpm_set_attached_state(port, false);
>   	port->try_src_count = 0;
>   	port->try_snk_count = 0;
> diff --git a/drivers/staging/typec/tcpm.h b/drivers/staging/typec/tcpm.h
> index 7e9a6b7b5cd6..f662eed48c86 100644
> --- a/drivers/staging/typec/tcpm.h
> +++ b/drivers/staging/typec/tcpm.h
> @@ -83,17 +83,23 @@ enum tcpc_usb_switch {
>   };
>   
>   /* Mux state attributes */
> -#define TCPC_MUX_USB_ENABLED		BIT(0)	/* USB enabled */
> -#define TCPC_MUX_DP_ENABLED		BIT(1)	/* DP enabled */
> -#define TCPC_MUX_POLARITY_INVERTED	BIT(2)	/* Polarity inverted */
> +#define TCPC_MUX_USB_DEVICE_ENABLED		BIT(0)	/* USB device enabled */
> +#define TCPC_MUX_USB_HOST_ENABLED		BIT(1)	/* USB host enabled */
> +#define TCPC_MUX_DP_SRC_ENABLED			BIT(2)	/* DP enabled */
> +#define TCPC_MUX_POLARITY_INVERTED		BIT(3)	/* Polarity inverted */
>   
>   /* Mux modes, decoded to attributes */
>   enum tcpc_mux_mode {
> -	TYPEC_MUX_NONE	= 0,				/* Open switch */
> -	TYPEC_MUX_USB	= TCPC_MUX_USB_ENABLED,		/* USB only */
> -	TYPEC_MUX_DP	= TCPC_MUX_DP_ENABLED,		/* DP only */
> -	TYPEC_MUX_DOCK	= TCPC_MUX_USB_ENABLED |	/* Both USB and DP */
> -			  TCPC_MUX_DP_ENABLED,
> +	/* Open switch */
> +	TYPEC_MUX_NONE = 0,
> +	/* USB device only */
> +	TYPEC_MUX_USB_DEVICE = TCPC_MUX_USB_DEVICE_ENABLED,
> +	/* USB host only */
> +	TYPEC_MUX_USB_HOST = TCPC_MUX_USB_HOST_ENABLED,
> +	/* DP source only */
> +	TYPEC_MUX_DP = TCPC_MUX_DP_SRC_ENABLED,
> +	/* Both USB host and DP source */
> +	TYPEC_MUX_DOCK = TCPC_MUX_USB_HOST_ENABLED | TCPC_MUX_DP_SRC_ENABLED,
>   };
>   
>   struct tcpc_mux_dev {
> 

Re: [PATCH v2 08/11] staging: typec: tcpm: Set mux to device mode when configured as such

[Hans de Goede]

Hi,

On 09/11/2017 12:56 AM, Guenter Roeck wrote:
> On 09/05/2017 09:42 AM, Hans de Goede wrote:
>> Setting the mux to TYPEC_MUX_NONE, TCPC_USB_SWITCH_DISCONNECT when the
>> data-role is device is not correct. Plenty of devices support operating
>> as USB device through a (separate) USB device controller.
>>
>> So this commit instead splits out TYPEC_MUX_USB into TYPEC_MUX_USB_HOST
>> and TYPEC_MUX_USB_DEVICE and makes tcpm_set_roles() set the mux
>> accordingly.
>>
>> Likewise TCPC_MUX_DP gets renamed to TCPC_MUX_DP_SRC to make clear that
>> this is for configuring the Type-C port as a Display Port source, not a
>> sink.
>>
>> Last this commit makes tcpm_reset_port() to set the mux to
>> TYPEC_MUX_NONE, TCPC_USB_SWITCH_DISCONNECT so that it does not and up
>> staying in host (and with this commit also device) mode after a detach.
>>
> This sentence is hard to understand.

Ok, changed to:

"Last this commit makes tcpm_reset_port() call tcpm_mux_set(port,
TYPEC_MUX_NONE, TCPC_USB_SWITCH_DISCONNECT) so that the mux does _not_
stay in its last mode after a detach."

For v3 of this patchset.

> 
>> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> 
> Otherwise
> 
> Reviewed-by: Guenter Roeck <linux@roeck-us.net>

And added your Reviewed-by.

Thanks & Regards,

Hans




>
> 
>> ---
>>   drivers/staging/typec/tcpm.c |  7 ++++---
>>   drivers/staging/typec/tcpm.h | 22 ++++++++++++++--------
>>   2 files changed, 18 insertions(+), 11 deletions(-)
>>
>> diff --git a/drivers/staging/typec/tcpm.c b/drivers/staging/typec/tcpm.c
>> index 8af62e74d54c..ffe7e26d4ed3 100644
>> --- a/drivers/staging/typec/tcpm.c
>> +++ b/drivers/staging/typec/tcpm.c
>> @@ -752,11 +752,11 @@ static int tcpm_set_roles(struct tcpm_port *port, bool attached,
>>       int ret;
>>       if (data == TYPEC_HOST)
>> -        ret = tcpm_mux_set(port, TYPEC_MUX_USB,
>> +        ret = tcpm_mux_set(port, TYPEC_MUX_USB_HOST,
>>                      TCPC_USB_SWITCH_CONNECT);
>>       else
>> -        ret = tcpm_mux_set(port, TYPEC_MUX_NONE,
>> -                   TCPC_USB_SWITCH_DISCONNECT);
>> +        ret = tcpm_mux_set(port, TYPEC_MUX_USB_DEVICE,
>> +                   TCPC_USB_SWITCH_CONNECT);
>>       if (ret < 0)
>>           return ret;
>> @@ -2025,6 +2025,7 @@ static void tcpm_reset_port(struct tcpm_port *port)
>>       tcpm_init_vconn(port);
>>       tcpm_set_current_limit(port, 0, 0);
>>       tcpm_set_polarity(port, TYPEC_POLARITY_CC1);
>> +    tcpm_mux_set(port, TYPEC_MUX_NONE, TCPC_USB_SWITCH_DISCONNECT);
>>       tcpm_set_attached_state(port, false);
>>       port->try_src_count = 0;
>>       port->try_snk_count = 0;
>> diff --git a/drivers/staging/typec/tcpm.h b/drivers/staging/typec/tcpm.h
>> index 7e9a6b7b5cd6..f662eed48c86 100644
>> --- a/drivers/staging/typec/tcpm.h
>> +++ b/drivers/staging/typec/tcpm.h
>> @@ -83,17 +83,23 @@ enum tcpc_usb_switch {
>>   };
>>   /* Mux state attributes */
>> -#define TCPC_MUX_USB_ENABLED        BIT(0)    /* USB enabled */
>> -#define TCPC_MUX_DP_ENABLED        BIT(1)    /* DP enabled */
>> -#define TCPC_MUX_POLARITY_INVERTED    BIT(2)    /* Polarity inverted */
>> +#define TCPC_MUX_USB_DEVICE_ENABLED        BIT(0)    /* USB device enabled */
>> +#define TCPC_MUX_USB_HOST_ENABLED        BIT(1)    /* USB host enabled */
>> +#define TCPC_MUX_DP_SRC_ENABLED            BIT(2)    /* DP enabled */
>> +#define TCPC_MUX_POLARITY_INVERTED        BIT(3)    /* Polarity inverted */
>>   /* Mux modes, decoded to attributes */
>>   enum tcpc_mux_mode {
>> -    TYPEC_MUX_NONE    = 0,                /* Open switch */
>> -    TYPEC_MUX_USB    = TCPC_MUX_USB_ENABLED,        /* USB only */
>> -    TYPEC_MUX_DP    = TCPC_MUX_DP_ENABLED,        /* DP only */
>> -    TYPEC_MUX_DOCK    = TCPC_MUX_USB_ENABLED |    /* Both USB and DP */
>> -              TCPC_MUX_DP_ENABLED,
>> +    /* Open switch */
>> +    TYPEC_MUX_NONE = 0,
>> +    /* USB device only */
>> +    TYPEC_MUX_USB_DEVICE = TCPC_MUX_USB_DEVICE_ENABLED,
>> +    /* USB host only */
>> +    TYPEC_MUX_USB_HOST = TCPC_MUX_USB_HOST_ENABLED,
>> +    /* DP source only */
>> +    TYPEC_MUX_DP = TCPC_MUX_DP_SRC_ENABLED,
>> +    /* Both USB host and DP source */
>> +    TYPEC_MUX_DOCK = TCPC_MUX_USB_HOST_ENABLED | TCPC_MUX_DP_SRC_ENABLED,
>>   };
>>   struct tcpc_mux_dev {
>>
> 

[PATCH v2 09/11] staging: typec: Add Generic TCPC mux driver using the mux subsys

[Hans de Goede]

So far the mux functionality of the tcpm code has not been hooked up
in any tcpc drivers. This commit adds a generic TCPC mux driver using
the mux subsys, which tcpc drivers can use to provide mux functionality
in cases where an external my is used.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/staging/typec/Makefile       |   2 +-
 drivers/staging/typec/tcpc_gen_mux.c | 131 +++++++++++++++++++++++++++++++++++
 drivers/staging/typec/tcpm.h         |   2 +
 3 files changed, 134 insertions(+), 1 deletion(-)
 create mode 100644 drivers/staging/typec/tcpc_gen_mux.c

diff --git a/drivers/staging/typec/Makefile b/drivers/staging/typec/Makefile
index 30a7e29cbc9e..623d78114ee3 100644
--- a/drivers/staging/typec/Makefile
+++ b/drivers/staging/typec/Makefile
@@ -1,3 +1,3 @@
-obj-$(CONFIG_TYPEC_TCPM)	+= tcpm.o
+obj-$(CONFIG_TYPEC_TCPM)	+= tcpm.o tcpc_gen_mux.o
 obj-$(CONFIG_TYPEC_TCPCI)	+= tcpci.o
 obj-y				+= fusb302/
diff --git a/drivers/staging/typec/tcpc_gen_mux.c b/drivers/staging/typec/tcpc_gen_mux.c
new file mode 100644
index 000000000000..d40f9eaaddf5
--- /dev/null
+++ b/drivers/staging/typec/tcpc_gen_mux.c
@@ -0,0 +1,131 @@
+/*
+ * Generic TCPC mux driver using the mux subsys
+ *
+ * Copyright (c) 2017 Hans de Goede <hdegoede@redhat.com>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation, or (at your option)
+ * any later version.
+ */
+
+#include <linux/i2c.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/mux/consumer.h>
+#include <linux/mux/usb.h>
+#include "tcpm.h"
+
+struct tcpc_gen_mux_data {
+	struct tcpc_mux_dev mux;
+	struct device *dev;
+	struct mux_control *type_c_mode_mux; /* Type-C cross switch / mux */
+	struct mux_control *usb_role_mux;    /* USB Device / Host mode mux */
+	bool muxes_set;
+};
+
+static int tcpc_gen_mux_set(struct tcpc_mux_dev *mux_dev,
+			    enum tcpc_mux_mode mux_mode,
+			    enum tcpc_usb_switch usb_config,
+			    enum typec_cc_polarity polarity)
+{
+	struct tcpc_gen_mux_data *data =
+		container_of(mux_dev, struct tcpc_gen_mux_data, mux);
+	unsigned int typec_state = MUX_TYPEC_DEVICE;
+	unsigned int usb_state = MUX_USB_DEVICE;
+	int ret;
+
+	/* Put the muxes back in their open (idle) state */
+	if (data->muxes_set) {
+		mux_control_deselect(data->type_c_mode_mux);
+		if (data->usb_role_mux)
+			mux_control_deselect(data->usb_role_mux);
+		data->muxes_set = false;
+	}
+
+	switch (mux_mode) {
+	case TYPEC_MUX_NONE:
+		/* Muxes are in their open state, done. */
+		return 0;
+	case TYPEC_MUX_USB_DEVICE:
+		typec_state = MUX_TYPEC_DEVICE;
+		usb_state = MUX_USB_DEVICE;
+		break;
+	case TYPEC_MUX_USB_HOST:
+		typec_state = MUX_TYPEC_HOST;
+		usb_state = MUX_USB_HOST;
+		break;
+	case TYPEC_MUX_DP:
+		typec_state = MUX_TYPEC_DP_SRC;
+		break;
+	case TYPEC_MUX_DOCK:
+		typec_state = MUX_TYPEC_HOST_AND_DP_SRC;
+		usb_state = MUX_USB_HOST;
+		break;
+	}
+
+	if (polarity)
+		typec_state |= MUX_TYPEC_POLARITY_INV;
+
+	ret = mux_control_select(data->type_c_mode_mux, typec_state);
+	if (ret) {
+		dev_err(data->dev, "Error setting Type-C mode mux: %d\n", ret);
+		return ret;
+	}
+
+	if (!data->usb_role_mux)
+		goto out;
+
+	ret = mux_control_select(data->usb_role_mux, usb_state);
+	if (ret) {
+		dev_err(data->dev, "Error setting USB role mux: %d\n", ret);
+		mux_control_deselect(data->type_c_mode_mux);
+		return ret;
+	}
+
+out:
+	data->muxes_set = true;
+	return 0;
+}
+
+struct tcpc_mux_dev *devm_tcpc_gen_mux_create(struct device *dev)
+{
+	struct tcpc_gen_mux_data *data;
+	int ret;
+
+	data = devm_kzalloc(dev, sizeof(*data), GFP_KERNEL);
+	if (!data)
+		return ERR_PTR(-ENOMEM);
+
+	data->type_c_mode_mux = devm_mux_control_get(dev, "type-c-mode-mux");
+	if (IS_ERR(data->type_c_mode_mux)) {
+		ret = PTR_ERR(data->type_c_mode_mux);
+		if (ret != -EPROBE_DEFER)
+			dev_err(dev, "Error getting Type-C mux: %d\n", ret);
+		return ERR_PTR(-ret);
+	}
+
+	data->usb_role_mux = devm_mux_control_get(dev, "usb-role-mux");
+	if (IS_ERR(data->usb_role_mux)) {
+		ret = PTR_ERR(data->usb_role_mux);
+		/* The USB role mux is optional */
+		if (ret == -ENODEV) {
+			data->usb_role_mux = NULL;
+		} else {
+			if (ret != -EPROBE_DEFER)
+				dev_err(dev, "Error getting USB role mux: %d\n",
+					ret);
+			return ERR_PTR(-ret);
+		}
+	}
+
+	data->dev = dev;
+	data->mux.set = tcpc_gen_mux_set;
+
+	return &data->mux;
+}
+EXPORT_SYMBOL_GPL(devm_tcpc_gen_mux_create);
+
+MODULE_AUTHOR("Hans de Goede <hdegoede@redhat.com>");
+MODULE_DESCRIPTION("Generic Type-C mux driver using the mux subsys");
+MODULE_LICENSE("GPL");
diff --git a/drivers/staging/typec/tcpm.h b/drivers/staging/typec/tcpm.h
index f662eed48c86..36789005a2b0 100644
--- a/drivers/staging/typec/tcpm.h
+++ b/drivers/staging/typec/tcpm.h
@@ -164,4 +164,6 @@ void tcpm_pd_transmit_complete(struct tcpm_port *port,
 void tcpm_pd_hard_reset(struct tcpm_port *port);
 void tcpm_tcpc_reset(struct tcpm_port *port);
 
+struct tcpc_mux_dev *devm_tcpc_gen_mux_create(struct device *dev);
+
 #endif /* __LINUX_USB_TCPM_H */
-- 
2.13.5

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

[Hans de Goede]

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@kernel.org>
Cc: Mark Rutland <mark.rutland@arm.com>
Cc: devicetree@vger.kernel.org
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 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
 - 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
diff --git a/drivers/staging/typec/fusb302/fusb302.c b/drivers/staging/typec/fusb302/fusb302.c
index cf6355f59cd9..00d045d0246b 100644
--- a/drivers/staging/typec/fusb302/fusb302.c
+++ b/drivers/staging/typec/fusb302/fusb302.c
@@ -1259,7 +1259,6 @@ static void init_tcpc_dev(struct tcpc_dev *fusb302_tcpc_dev)
 	fusb302_tcpc_dev->set_roles = tcpm_set_roles;
 	fusb302_tcpc_dev->start_drp_toggling = tcpm_start_drp_toggling;
 	fusb302_tcpc_dev->pd_transmit = tcpm_pd_transmit;
-	fusb302_tcpc_dev->mux = NULL;
 }
 
 static const char * const cc_polarity_name[] = {
@@ -1817,6 +1816,16 @@ static int fusb302_probe(struct i2c_client *client,
 			return -EPROBE_DEFER;
 	}
 
+	chip->tcpc_dev.mux = devm_tcpc_gen_mux_create(dev);
+	if (IS_ERR(chip->tcpc_dev.mux)) {
+		ret = PTR_ERR(chip->tcpc_dev.mux);
+		/* Use of a mux is optional (for now?), ignore -ENODEV errors */
+		if (ret == -ENODEV)
+			chip->tcpc_dev.mux = NULL;
+		else
+			return ret;
+	}
+
 	cfg.drv_data = chip;
 	chip->psy = devm_power_supply_register(dev, &fusb302_psy_desc, &cfg);
 	if (IS_ERR(chip->psy)) {
-- 
2.13.5

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

[Rob Herring]

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@kernel.org>
> Cc: Mark Rutland <mark.rutland@arm.com>
> Cc: devicetree@vger.kernel.org
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> ---
>  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.

>  - 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

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 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@kernel.org>
>> Cc: Mark Rutland <mark.rutland@arm.com>
>> Cc: devicetree@vger.kernel.org
>> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
>> ---
>>   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.

As for putting it in a type-C connector node, currently we do not have such a node,
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).

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.

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).

Regards,

Hans

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

[Rob Herring]

On Wed, Sep 13, 2017 at 3:56 AM, Hans de Goede <hdegoede@redhat.com> 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@kernel.org>
>>> Cc: Mark Rutland <mark.rutland@arm.com>
>>> Cc: devicetree@vger.kernel.org
>>> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
>>> ---
>>>   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.

> 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?

> 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.

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).

Rob

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@redhat.com> 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@kernel.org>
>>>> Cc: Mark Rutland <mark.rutland@arm.com>
>>>> Cc: devicetree@vger.kernel.org
>>>> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
>>>> ---
>>>>    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

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

[Rob Herring]

On Wed, Sep 13, 2017 at 9:06 AM, Hans de Goede <hdegoede@redhat.com> wrote:
> Hi,
>
>
> On 13-09-17 15:38, Rob Herring wrote:
>>
>> On Wed, Sep 13, 2017 at 3:56 AM, Hans de Goede <hdegoede@redhat.com>
>> 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@kernel.org>
>>>>> Cc: Mark Rutland <mark.rutland@arm.com>
>>>>> Cc: devicetree@vger.kernel.org
>>>>> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
>>>>> ---
>>>>>    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.

Humm, that's not how the mux binding works. The mux controller is what
drives the mux select lines and is the provider. The consumer is the
mux device itself. What decides the mux state is determined by what
you are muxing, not which node has mux-controls property.

By putting mux-controls in fusb302 node, you are saying fusb302 is a
mux (or contains a mux).


>>> 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.

Well this is a new one (maybe, I suppose others have sneaked by). If
ACPI folks want to use DT bindings, then what do I care. But I have no
interest in reviewing ACPI properties. The whole notion of sharing
bindings between DT and ACPI beyond anything trivial is flawed IMO.
The ptifalls have been discussed multiple times before, so I'm not
going to repeat them here.


>>> 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.

Just as the fusb302 can have board specific properties, so can a charger IC.

> 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.

Then don't document this stuff as DT bindings when it is not really.

Rob

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 17:07, Rob Herring wrote:
> On Wed, Sep 13, 2017 at 9:06 AM, Hans de Goede <hdegoede@redhat.com> wrote:
>> Hi,
>>
>>
>> On 13-09-17 15:38, Rob Herring wrote:
>>>
>>> On Wed, Sep 13, 2017 at 3:56 AM, Hans de Goede <hdegoede@redhat.com>
>>> 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@kernel.org>
>>>>>> Cc: Mark Rutland <mark.rutland@arm.com>
>>>>>> Cc: devicetree@vger.kernel.org
>>>>>> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
>>>>>> ---
>>>>>>     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.
> 
> Humm, that's not how the mux binding works. The mux controller is what
> drives the mux select lines and is the provider. The consumer is the
> mux device itself. What decides the mux state is determined by what
> you are muxing, not which node has mux-controls property.
> 
> By putting mux-controls in fusb302 node, you are saying fusb302 is a
> mux (or contains a mux).
> 
> 
>>>> 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.
> 
> Well this is a new one (maybe, I suppose others have sneaked by). If
> ACPI folks want to use DT bindings, then what do I care. But I have no
> interest in reviewing ACPI properties. The whole notion of sharing
> bindings between DT and ACPI beyond anything trivial is flawed IMO.
> The ptifalls have been discussed multiple times before, so I'm not
> going to repeat them here.

Ok, so shall I just drop the Documentation/devicetree/bindings/usb/fcs,fusb302.txt
part of this patch then ?

Regards,

Hans

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

[Guenter Roeck]

On Wed, Sep 13, 2017 at 05:48:25PM +0200, Hans de Goede wrote:
> Hi,
> 
> On 13-09-17 17:07, Rob Herring wrote:
> >On Wed, Sep 13, 2017 at 9:06 AM, Hans de Goede <hdegoede@redhat.com> wrote:
> >>Hi,
> >>
> >>
> >>On 13-09-17 15:38, Rob Herring wrote:
> >>>
> >>>On Wed, Sep 13, 2017 at 3:56 AM, Hans de Goede <hdegoede@redhat.com>
> >>>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@kernel.org>
> >>>>>>Cc: Mark Rutland <mark.rutland@arm.com>
> >>>>>>Cc: devicetree@vger.kernel.org
> >>>>>>Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> >>>>>>---
> >>>>>>    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.
> >
> >Humm, that's not how the mux binding works. The mux controller is what
> >drives the mux select lines and is the provider. The consumer is the
> >mux device itself. What decides the mux state is determined by what
> >you are muxing, not which node has mux-controls property.
> >
> >By putting mux-controls in fusb302 node, you are saying fusb302 is a
> >mux (or contains a mux).
> >
> >
> >>>>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.
> >
> >Well this is a new one (maybe, I suppose others have sneaked by). If
> >ACPI folks want to use DT bindings, then what do I care. But I have no
> >interest in reviewing ACPI properties. The whole notion of sharing
> >bindings between DT and ACPI beyond anything trivial is flawed IMO.
> >The ptifalls have been discussed multiple times before, so I'm not
> >going to repeat them here.
> 
> Ok, so shall I just drop the Documentation/devicetree/bindings/usb/fcs,fusb302.txt
> part of this patch then ?
> 
FWIW, Android (where the fusb302 driver is coming from) does use dt.
On the other side I assume they won't jump on the new mux handling
immediately. I won't have time to look into it myself, so whatever
is done here may not match the "real" dt use case. Given that, maybe
it does make sense to drop the bindings part and revisit once it
becomes relevant.

Guenter

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

[Peter Rosin]

On 2017-09-13 17:48, Hans de Goede wrote:
> Hi,
> 
> On 13-09-17 17:07, Rob Herring wrote:
>> On Wed, Sep 13, 2017 at 9:06 AM, Hans de Goede <hdegoede@redhat.com> wrote:
>>> Hi,
>>>
>>>
>>> On 13-09-17 15:38, Rob Herring wrote:
>>>>
>>>> On Wed, Sep 13, 2017 at 3:56 AM, Hans de Goede <hdegoede@redhat.com>
>>>> 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@kernel.org>
>>>>>>> Cc: Mark Rutland <mark.rutland@arm.com>
>>>>>>> Cc: devicetree@vger.kernel.org
>>>>>>> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
>>>>>>> ---
>>>>>>>     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.
>>
>> Humm, that's not how the mux binding works. The mux controller is what
>> drives the mux select lines and is the provider. The consumer is the
>> mux device itself. What decides the mux state is determined by what
>> you are muxing, not which node has mux-controls property.
>>
>> By putting mux-controls in fusb302 node, you are saying fusb302 is a
>> mux (or contains a mux).
>>
>>
>>>>> 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.
>>
>> Well this is a new one (maybe, I suppose others have sneaked by). If
>> ACPI folks want to use DT bindings, then what do I care. But I have no
>> interest in reviewing ACPI properties. The whole notion of sharing
>> bindings between DT and ACPI beyond anything trivial is flawed IMO.
>> The ptifalls have been discussed multiple times before, so I'm not
>> going to repeat them here.
> 
> Ok, so shall I just drop the Documentation/devicetree/bindings/usb/fcs,fusb302.txt
> part of this patch then ?

I totally agree with the concern that Rob expressed about handling USB C
muxes in a non-adhoc way. And this makes this series scary. I don't know
enough details about USB C muxes and PD (I just have a very superficial
mental model) to tell if this series is going down the right path. Or
how terrible it will be to fix things up if not?

The series extends the mux subsystem with muxes that pins semantics
to specific states. That is new, and shows up exactly here when Rob is
not happy about the binding. And if Rob does not want this in the
DT bindings then I'm not so sure it is wise to do it at all? This
problem doesn't go away just because you remove the binding. I think
I would feel much better if there was a path forward on how to
represent USB C muxes in DT and how that would fit with the driver
structure.

If you compare to the i2c-muxes, there is a relatively new i2c-mux-gpmux
driver that uses some general purpose mux from the mux subsystem to
implement an i2c-mux. If USB C muxes where to be done similarly, I'd
imagine there should be a general abstraction of what USB C muxes provide
somewhere outside of the mux subsystem, and a bunch of implementations
of that abstraction. One of those implementations could be to use "raw"
muxes from the mux subsystem. Of course, this is not what this series is
doing.

Also, muxes that are not general purpose such as the ones added to the
mux subsystem by this series could perhaps be repurposed for some other
application, but since the interface implemented does not really obey
the rules (the provided mux controller interacts with different sets of
signals depending on the state) this will not be possible.

These issues are what has caused me to do a lot of thinking and to sit
silent, sorry about that, but I would like input from someone more
experienced. If possible. But I'm not sure where to turn?

As a crazy example, why is it not possible to hook up one signal pair
from the USB C mux, not to DP, but instead to some I2C controller? Then,
if done right, i2c-mux-gpmux could be hooked up with the relevant mux
controller and use the signal pair for I2C, with the mux controller
acting as a gate. So, maybe a bit crazy, but something like that is how
I think it should work from the mux subsystem point of view. And while
maybe crazy and while it might not be technically possible to do I2C
over a USB C connector for some reason, I do think that whatever
abstraction you come up with for USB C muxes, it has to deal with and
broker requests from both the USB subsystem and whatever other
subsystems deals with the alt pairs. Be it graphics for DP signals, or
whatever. IIUC, the alt signals need not be graphics, and it would be
sad to implement the USB C mux it in a way that makes it hard to use
the alt pairs for something else.

[maybe my understanding of USB C is just wrong]

Cheers,
Peter

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

[Hans de Goede]

Hi,

On 25-09-17 12:34, Peter Rosin wrote:
> On 2017-09-13 17:48, Hans de Goede wrote:
>> Hi,
>>
>> On 13-09-17 17:07, Rob Herring wrote:
>>> On Wed, Sep 13, 2017 at 9:06 AM, Hans de Goede <hdegoede@redhat.com> wrote:
>>>> Hi,
>>>>
>>>>
>>>> On 13-09-17 15:38, Rob Herring wrote:
>>>>>
>>>>> On Wed, Sep 13, 2017 at 3:56 AM, Hans de Goede <hdegoede@redhat.com>
>>>>> 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@kernel.org>
>>>>>>>> Cc: Mark Rutland <mark.rutland@arm.com>
>>>>>>>> Cc: devicetree@vger.kernel.org
>>>>>>>> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
>>>>>>>> ---
>>>>>>>>      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.
>>>
>>> Humm, that's not how the mux binding works. The mux controller is what
>>> drives the mux select lines and is the provider. The consumer is the
>>> mux device itself. What decides the mux state is determined by what
>>> you are muxing, not which node has mux-controls property.
>>>
>>> By putting mux-controls in fusb302 node, you are saying fusb302 is a
>>> mux (or contains a mux).
>>>
>>>
>>>>>> 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.
>>>
>>> Well this is a new one (maybe, I suppose others have sneaked by). If
>>> ACPI folks want to use DT bindings, then what do I care. But I have no
>>> interest in reviewing ACPI properties. The whole notion of sharing
>>> bindings between DT and ACPI beyond anything trivial is flawed IMO.
>>> The ptifalls have been discussed multiple times before, so I'm not
>>> going to repeat them here.
>>
>> Ok, so shall I just drop the Documentation/devicetree/bindings/usb/fcs,fusb302.txt
>> part of this patch then ?
> 
> I totally agree with the concern that Rob expressed about handling USB C
> muxes in a non-adhoc way. And this makes this series scary. I don't know
> enough details about USB C muxes and PD (I just have a very superficial
> mental model) to tell if this series is going down the right path. Or
> how terrible it will be to fix things up if not?
> 
> The series extends the mux subsystem with muxes that pins semantics
> to specific states. That is new, and shows up exactly here when Rob is
> not happy about the binding. And if Rob does not want this in the
> DT bindings then I'm not so sure it is wise to do it at all? This
> problem doesn't go away just because you remove the binding. I think
> I would feel much better if there was a path forward on how to
> represent USB C muxes in DT and how that would fit with the driver
> structure.
> 
> If you compare to the i2c-muxes, there is a relatively new i2c-mux-gpmux
> driver that uses some general purpose mux from the mux subsystem to
> implement an i2c-mux. If USB C muxes where to be done similarly, I'd
> imagine there should be a general abstraction of what USB C muxes provide
> somewhere outside of the mux subsystem, and a bunch of implementations
> of that abstraction. One of those implementations could be to use "raw"
> muxes from the mux subsystem. Of course, this is not what this series is
> doing.
> 
> Also, muxes that are not general purpose such as the ones added to the
> mux subsystem by this series could perhaps be repurposed for some other
> application, but since the interface implemented does not really obey
> the rules (the provided mux controller interacts with different sets of
> signals depending on the state) this will not be possible.
> 
> These issues are what has caused me to do a lot of thinking and to sit
> silent, sorry about that, but I would like input from someone more
> experienced. If possible. But I'm not sure where to turn?
> 
> As a crazy example, why is it not possible to hook up one signal pair
> from the USB C mux, not to DP, but instead to some I2C controller? Then,
> if done right, i2c-mux-gpmux could be hooked up with the relevant mux
> controller and use the signal pair for I2C, with the mux controller
> acting as a gate. So, maybe a bit crazy, but something like that is how
> I think it should work from the mux subsystem point of view. And while
> maybe crazy and while it might not be technically possible to do I2C
> over a USB C connector for some reason, I do think that whatever
> abstraction you come up with for USB C muxes, it has to deal with and
> broker requests from both the USB subsystem and whatever other
> subsystems deals with the alt pairs. Be it graphics for DP signals, or
> whatever. IIUC, the alt signals need not be graphics, and it would be
> sad to implement the USB C mux it in a way that makes it hard to use
> the alt pairs for something else.
> 
> [maybe my understanding of USB C is just wrong]

So 2 things:

1) The Type-C subsys does actually abstract the mux outside of the
Type-C port-manager (TCPM) core in the form of a tcpc_mux_dev, so for
DT based platforms we could instantiate a tcpc_mux_dev from a node
which then represents the mux as usual.

2) What is very different from how the mux subsys currently is used,
is who is in control of the mux. Currently a subsys which wants to
route data through the mux selects the mux in the right mode, and
then deselects it when done. This assumes that the mux can mux
the data paths to the requested destination at all times, just not
to multiple destinations at once. With Type-C and any moment in
time, there really is only one correct setting as the mux is
connect to a Type-C device on the other side which typically
only has one config. So what happens with Type-C is that the TCPM
negotiates with the externally connected Type-C device, and then
sets the mux to the one and only correct setting for that device
to be fully functional.

So your i2c example, for example, will not work with the normal
way where the i2c subsys asks the mux to mux a data-pair to the i2c
controller, as that only makes sense when your theoretical Type-C
device which talks i2c over a pair is connected externally, where
as when something else is connected then trying to talk i2c might
even damage the connected device. So with Type-C it is the TCPM
and only the TCPM which controls the mux.

Regards,

Hans

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

[Peter Rosin]

On 2017-09-25 13:35, Hans de Goede wrote:
> Hi,
> 
> On 25-09-17 12:34, Peter Rosin wrote:
>> On 2017-09-13 17:48, Hans de Goede wrote:
>>> Hi,
>>>
>>> On 13-09-17 17:07, Rob Herring wrote:
>>>> On Wed, Sep 13, 2017 at 9:06 AM, Hans de Goede <hdegoede@redhat.com> wrote:
>>>>> Hi,
>>>>>
>>>>>
>>>>> On 13-09-17 15:38, Rob Herring wrote:
>>>>>>
>>>>>> On Wed, Sep 13, 2017 at 3:56 AM, Hans de Goede <hdegoede@redhat.com>
>>>>>> 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@kernel.org>
>>>>>>>>> Cc: Mark Rutland <mark.rutland@arm.com>
>>>>>>>>> Cc: devicetree@vger.kernel.org
>>>>>>>>> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
>>>>>>>>> ---
>>>>>>>>>      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.
>>>>
>>>> Humm, that's not how the mux binding works. The mux controller is what
>>>> drives the mux select lines and is the provider. The consumer is the
>>>> mux device itself. What decides the mux state is determined by what
>>>> you are muxing, not which node has mux-controls property.
>>>>
>>>> By putting mux-controls in fusb302 node, you are saying fusb302 is a
>>>> mux (or contains a mux).
>>>>
>>>>
>>>>>>> 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.
>>>>
>>>> Well this is a new one (maybe, I suppose others have sneaked by). If
>>>> ACPI folks want to use DT bindings, then what do I care. But I have no
>>>> interest in reviewing ACPI properties. The whole notion of sharing
>>>> bindings between DT and ACPI beyond anything trivial is flawed IMO.
>>>> The ptifalls have been discussed multiple times before, so I'm not
>>>> going to repeat them here.
>>>
>>> Ok, so shall I just drop the Documentation/devicetree/bindings/usb/fcs,fusb302.txt
>>> part of this patch then ?
>>
>> I totally agree with the concern that Rob expressed about handling USB C
>> muxes in a non-adhoc way. And this makes this series scary. I don't know
>> enough details about USB C muxes and PD (I just have a very superficial
>> mental model) to tell if this series is going down the right path. Or
>> how terrible it will be to fix things up if not?
>>
>> The series extends the mux subsystem with muxes that pins semantics
>> to specific states. That is new, and shows up exactly here when Rob is
>> not happy about the binding. And if Rob does not want this in the
>> DT bindings then I'm not so sure it is wise to do it at all? This
>> problem doesn't go away just because you remove the binding. I think
>> I would feel much better if there was a path forward on how to
>> represent USB C muxes in DT and how that would fit with the driver
>> structure.
>>
>> If you compare to the i2c-muxes, there is a relatively new i2c-mux-gpmux
>> driver that uses some general purpose mux from the mux subsystem to
>> implement an i2c-mux. If USB C muxes where to be done similarly, I'd
>> imagine there should be a general abstraction of what USB C muxes provide
>> somewhere outside of the mux subsystem, and a bunch of implementations
>> of that abstraction. One of those implementations could be to use "raw"
>> muxes from the mux subsystem. Of course, this is not what this series is
>> doing.
>>
>> Also, muxes that are not general purpose such as the ones added to the
>> mux subsystem by this series could perhaps be repurposed for some other
>> application, but since the interface implemented does not really obey
>> the rules (the provided mux controller interacts with different sets of
>> signals depending on the state) this will not be possible.
>>
>> These issues are what has caused me to do a lot of thinking and to sit
>> silent, sorry about that, but I would like input from someone more
>> experienced. If possible. But I'm not sure where to turn?
>>
>> As a crazy example, why is it not possible to hook up one signal pair
>> from the USB C mux, not to DP, but instead to some I2C controller? Then,
>> if done right, i2c-mux-gpmux could be hooked up with the relevant mux
>> controller and use the signal pair for I2C, with the mux controller
>> acting as a gate. So, maybe a bit crazy, but something like that is how
>> I think it should work from the mux subsystem point of view. And while
>> maybe crazy and while it might not be technically possible to do I2C
>> over a USB C connector for some reason, I do think that whatever
>> abstraction you come up with for USB C muxes, it has to deal with and
>> broker requests from both the USB subsystem and whatever other
>> subsystems deals with the alt pairs. Be it graphics for DP signals, or
>> whatever. IIUC, the alt signals need not be graphics, and it would be
>> sad to implement the USB C mux it in a way that makes it hard to use
>> the alt pairs for something else.
>>
>> [maybe my understanding of USB C is just wrong]
> 
> So 2 things:
> 
> 1) The Type-C subsys does actually abstract the mux outside of the
> Type-C port-manager (TCPM) core in the form of a tcpc_mux_dev, so for
> DT based platforms we could instantiate a tcpc_mux_dev from a node
> which then represents the mux as usual.
> 
> 2) What is very different from how the mux subsys currently is used,
> is who is in control of the mux. Currently a subsys which wants to
> route data through the mux selects the mux in the right mode, and
> then deselects it when done. This assumes that the mux can mux
> the data paths to the requested destination at all times, just not
> to multiple destinations at once. With Type-C and any moment in
> time, there really is only one correct setting as the mux is
> connect to a Type-C device on the other side which typically
> only has one config. So what happens with Type-C is that the TCPM
> negotiates with the externally connected Type-C device, and then
> sets the mux to the one and only correct setting for that device
> to be fully functional.
> 
> So your i2c example, for example, will not work with the normal
> way where the i2c subsys asks the mux to mux a data-pair to the i2c
> controller, as that only makes sense when your theoretical Type-C
> device which talks i2c over a pair is connected externally, where
> as when something else is connected then trying to talk i2c might
> even damage the connected device. So with Type-C it is the TCPM
> and only the TCPM which controls the mux.

If I get this correctly, some code (the TCPM?) negotiates with the
other side over PD, and after that the way to set up the USB C mux
is known and will not change until the cable is unplugged (unless
we go wild and renegotiate, but let's assume that will not happen). 
What I don't get is why do you want to squeeze what the TCPM(?) is
doing with its specialized mux into the existing mux subsystem
interface?

I mean, the mux interface isn't really a perfect fit with its
locking and support for multiple consumers etc that really just gets
in the way of setting a register in some chip to some value. Which
is all that is really needed when you know that the TCPM is the only
one accessing the chip.

And from the point of view of the mux subsystem, the USB C muxes
like the Pericom driver will be in a class of their own. Muxes of
that class can really only be used by one thing -- the TCPM code.

So, I don't see the benefit of doing this through the mux subsystem.
My hope is that the TCPM code is generic, and that by putting the
USB C mux inside the mux subsystem, you can keep the TCPM code
generic? That might be good for the TCPM code, but it does create a
new class of devices in the mux subsystem, and opens the door for
other classes later on.

Why not just add a function pointer that the generic TCPM code calls
if it is set, and then add some code somewhere more local to the TCPM
code to back that call, and completely avoid the mux subsystem?

Cheers,
Peter

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

[Hans de Goede]

Hi,

On 25-09-17 15:45, Peter Rosin wrote:
> On 2017-09-25 13:35, Hans de Goede wrote:
>> Hi,
>>
>> On 25-09-17 12:34, Peter Rosin wrote:
>>> On 2017-09-13 17:48, Hans de Goede wrote:
>>>> Hi,
>>>>
>>>> On 13-09-17 17:07, Rob Herring wrote:
>>>>> On Wed, Sep 13, 2017 at 9:06 AM, Hans de Goede <hdegoede@redhat.com> wrote:
>>>>>> Hi,
>>>>>>
>>>>>>
>>>>>> On 13-09-17 15:38, Rob Herring wrote:
>>>>>>>
>>>>>>> On Wed, Sep 13, 2017 at 3:56 AM, Hans de Goede <hdegoede@redhat.com>
>>>>>>> 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@kernel.org>
>>>>>>>>>> Cc: Mark Rutland <mark.rutland@arm.com>
>>>>>>>>>> Cc: devicetree@vger.kernel.org
>>>>>>>>>> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
>>>>>>>>>> ---
>>>>>>>>>>       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.
>>>>>
>>>>> Humm, that's not how the mux binding works. The mux controller is what
>>>>> drives the mux select lines and is the provider. The consumer is the
>>>>> mux device itself. What decides the mux state is determined by what
>>>>> you are muxing, not which node has mux-controls property.
>>>>>
>>>>> By putting mux-controls in fusb302 node, you are saying fusb302 is a
>>>>> mux (or contains a mux).
>>>>>
>>>>>
>>>>>>>> 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.
>>>>>
>>>>> Well this is a new one (maybe, I suppose others have sneaked by). If
>>>>> ACPI folks want to use DT bindings, then what do I care. But I have no
>>>>> interest in reviewing ACPI properties. The whole notion of sharing
>>>>> bindings between DT and ACPI beyond anything trivial is flawed IMO.
>>>>> The ptifalls have been discussed multiple times before, so I'm not
>>>>> going to repeat them here.
>>>>
>>>> Ok, so shall I just drop the Documentation/devicetree/bindings/usb/fcs,fusb302.txt
>>>> part of this patch then ?
>>>
>>> I totally agree with the concern that Rob expressed about handling USB C
>>> muxes in a non-adhoc way. And this makes this series scary. I don't know
>>> enough details about USB C muxes and PD (I just have a very superficial
>>> mental model) to tell if this series is going down the right path. Or
>>> how terrible it will be to fix things up if not?
>>>
>>> The series extends the mux subsystem with muxes that pins semantics
>>> to specific states. That is new, and shows up exactly here when Rob is
>>> not happy about the binding. And if Rob does not want this in the
>>> DT bindings then I'm not so sure it is wise to do it at all? This
>>> problem doesn't go away just because you remove the binding. I think
>>> I would feel much better if there was a path forward on how to
>>> represent USB C muxes in DT and how that would fit with the driver
>>> structure.
>>>
>>> If you compare to the i2c-muxes, there is a relatively new i2c-mux-gpmux
>>> driver that uses some general purpose mux from the mux subsystem to
>>> implement an i2c-mux. If USB C muxes where to be done similarly, I'd
>>> imagine there should be a general abstraction of what USB C muxes provide
>>> somewhere outside of the mux subsystem, and a bunch of implementations
>>> of that abstraction. One of those implementations could be to use "raw"
>>> muxes from the mux subsystem. Of course, this is not what this series is
>>> doing.
>>>
>>> Also, muxes that are not general purpose such as the ones added to the
>>> mux subsystem by this series could perhaps be repurposed for some other
>>> application, but since the interface implemented does not really obey
>>> the rules (the provided mux controller interacts with different sets of
>>> signals depending on the state) this will not be possible.
>>>
>>> These issues are what has caused me to do a lot of thinking and to sit
>>> silent, sorry about that, but I would like input from someone more
>>> experienced. If possible. But I'm not sure where to turn?
>>>
>>> As a crazy example, why is it not possible to hook up one signal pair
>>> from the USB C mux, not to DP, but instead to some I2C controller? Then,
>>> if done right, i2c-mux-gpmux could be hooked up with the relevant mux
>>> controller and use the signal pair for I2C, with the mux controller
>>> acting as a gate. So, maybe a bit crazy, but something like that is how
>>> I think it should work from the mux subsystem point of view. And while
>>> maybe crazy and while it might not be technically possible to do I2C
>>> over a USB C connector for some reason, I do think that whatever
>>> abstraction you come up with for USB C muxes, it has to deal with and
>>> broker requests from both the USB subsystem and whatever other
>>> subsystems deals with the alt pairs. Be it graphics for DP signals, or
>>> whatever. IIUC, the alt signals need not be graphics, and it would be
>>> sad to implement the USB C mux it in a way that makes it hard to use
>>> the alt pairs for something else.
>>>
>>> [maybe my understanding of USB C is just wrong]
>>
>> So 2 things:
>>
>> 1) The Type-C subsys does actually abstract the mux outside of the
>> Type-C port-manager (TCPM) core in the form of a tcpc_mux_dev, so for
>> DT based platforms we could instantiate a tcpc_mux_dev from a node
>> which then represents the mux as usual.
>>
>> 2) What is very different from how the mux subsys currently is used,
>> is who is in control of the mux. Currently a subsys which wants to
>> route data through the mux selects the mux in the right mode, and
>> then deselects it when done. This assumes that the mux can mux
>> the data paths to the requested destination at all times, just not
>> to multiple destinations at once. With Type-C and any moment in
>> time, there really is only one correct setting as the mux is
>> connect to a Type-C device on the other side which typically
>> only has one config. So what happens with Type-C is that the TCPM
>> negotiates with the externally connected Type-C device, and then
>> sets the mux to the one and only correct setting for that device
>> to be fully functional.
>>
>> So your i2c example, for example, will not work with the normal
>> way where the i2c subsys asks the mux to mux a data-pair to the i2c
>> controller, as that only makes sense when your theoretical Type-C
>> device which talks i2c over a pair is connected externally, where
>> as when something else is connected then trying to talk i2c might
>> even damage the connected device. So with Type-C it is the TCPM
>> and only the TCPM which controls the mux.
> 
> If I get this correctly, some code (the TCPM?) negotiates with the
> other side over PD, and after that the way to set up the USB C mux
> is known and will not change until the cable is unplugged (unless
> we go wild and renegotiate, but let's assume that will not happen).

Correct.

> What I don't get is why do you want to squeeze what the TCPM(?) is
> doing with its specialized mux into the existing mux subsystem
> interface?

That is a good question, this really started with the micro-usb
otg stuff, where we have more or less the same problem in a much
simpler way:

1) We have a cable plugged in which determined if we should mux the
usb2 data lines on a micro-usb to an usb-host or an usb-device/gadget
controller.

2) Some way to detect the type of cable plugged in

3) A hardware block or dedicated chip which is independent from 2,
which needs to be controlled by the code implementing 2.

So we need some glue layer with an abstract (device independent)
interface between 2 and 3 so that we can plug random combinations
of 2 and 3 together. In the beginning I tried using the extcon
framework for that, but that is not really a good fit.

Then someone else did some patches for the otg-mux found on the
Intel Cherry Trail block where using the mux subsys for this and
that seemed like a good idea to me.

And sofar I must say that from my pov the mux subsys does work
quite well for this, but now that I better understand the initial
design of the mux subsys I can understand that you are reluctant
about my use case.


> I mean, the mux interface isn't really a perfect fit with its
> locking and support for multiple consumers etc that really just gets
> in the way of setting a register in some chip to some value. Which
> is all that is really needed when you know that the TCPM is the only
> one accessing the chip.
> 
> And from the point of view of the mux subsystem, the USB C muxes
> like the Pericom driver will be in a class of their own. Muxes of
> that class can really only be used by one thing -- the TCPM code.
> 
> So, I don't see the benefit of doing this through the mux subsystem.
> My hope is that the TCPM code is generic, and that by putting the
> USB C mux inside the mux subsystem, you can keep the TCPM code
> generic?

Correct, the TCPM code is generic and for example the Cherry Trail
SoC OTG mux needs to be controller by either:

A) Something which is detecting the type of cable connected to a
micro-usb (either a PMIC or a gpio reading the ID pin) on devices
with a micro-usb connecter; or

B) The TCPM code on devices with a Type-C connector.

So this is another example where the mux-subsys more or less is
a pretty good fit.

As for the locking, you're right that the locking is not necessary,
but the Type-C connector and its muxes have a clearly defined idle
state, so the deselect functionality also is a decent fit there.

> That might be good for the TCPM code, but it does create a
> new class of devices in the mux subsystem, and opens the door for
> other classes later on.
> 
> Why not just add a function pointer that the generic TCPM code calls
> if it is set, and then add some code somewhere more local to the TCPM
> code to back that call, and completely avoid the mux subsystem?

See the above example about 2 completely different ways how we
need to control the Cherry Trail SoC OTG mux. We really need some
sort of abstraction layer here, and then some way to hook the TCPM
and the Type-C and OTG-mux together, either through pnode handles
in DT on DT platforms or through something like the lookup table
code one of my patches add on x86.

For me using the mux subsys for this works well. But as said I
can understand you being reluctant. Alternatively we could add a
new usb_mux subsys for this I guess.

Regards,

Hans

[PATCH v2 11/11] platform/x86: intel_cht_int33fe: Add mux mappings for the Type-C port

[Hans de Goede]

We need to add mappings for the mux subsys to be able to find the
muxes for the fusb302 driver to be able to control the PI3USB30532
Type-C mux and the device/host mux integrated in the CHT SoC.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
Acked-by: Andy Shevchenko <andy.shevchenko@gmail.com>
---
 drivers/platform/x86/Kconfig             |  1 +
 drivers/platform/x86/intel_cht_int33fe.c | 23 +++++++++++++++++++++++
 2 files changed, 24 insertions(+)

diff --git a/drivers/platform/x86/Kconfig b/drivers/platform/x86/Kconfig
index 312d2472b8b3..830ff8d0a1eb 100644
--- a/drivers/platform/x86/Kconfig
+++ b/drivers/platform/x86/Kconfig
@@ -794,6 +794,7 @@ config ACPI_CMPC
 config INTEL_CHT_INT33FE
 	tristate "Intel Cherry Trail ACPI INT33FE Driver"
 	depends on X86 && ACPI && I2C && REGULATOR
+	select MULTIPLEXER
 	---help---
 	  This driver add support for the INT33FE ACPI device found on
 	  some Intel Cherry Trail devices.
diff --git a/drivers/platform/x86/intel_cht_int33fe.c b/drivers/platform/x86/intel_cht_int33fe.c
index b2925d996613..89ba510dac78 100644
--- a/drivers/platform/x86/intel_cht_int33fe.c
+++ b/drivers/platform/x86/intel_cht_int33fe.c
@@ -24,6 +24,7 @@
 #include <linux/i2c.h>
 #include <linux/interrupt.h>
 #include <linux/module.h>
+#include <linux/mux/consumer.h>
 #include <linux/regulator/consumer.h>
 #include <linux/slab.h>
 
@@ -35,6 +36,19 @@ struct cht_int33fe_data {
 	struct i2c_client *pi3usb30532;
 };
 
+static struct mux_lookup cht_int33fe_mux_lookup[] = {
+	{
+		.provider = "i2c-pi3usb30532",
+		.dev_id   = "i2c-fusb302",
+		.mux_name = "type-c-mode-mux",
+	},
+	{
+		.provider = "intel_cht_usb_mux",
+		.dev_id   = "i2c-fusb302",
+		.mux_name = "usb-role-mux",
+	},
+};
+
 /*
  * Grrr I severly dislike buggy BIOS-es. At least one BIOS enumerates
  * the max17047 both through the INT33FE ACPI device (it is right there
@@ -177,6 +191,9 @@ static int cht_int33fe_probe(struct i2c_client *client)
 	board_info.properties = fusb302_props;
 	board_info.irq = fusb302_irq;
 
+	mux_add_table(cht_int33fe_mux_lookup,
+		      ARRAY_SIZE(cht_int33fe_mux_lookup));
+
 	data->fusb302 = i2c_acpi_new_device(dev, 2, &board_info);
 	if (!data->fusb302)
 		goto out_unregister_max17047;
@@ -200,6 +217,9 @@ static int cht_int33fe_probe(struct i2c_client *client)
 	if (data->max17047)
 		i2c_unregister_device(data->max17047);
 
+	mux_remove_table(cht_int33fe_mux_lookup,
+		      ARRAY_SIZE(cht_int33fe_mux_lookup));
+
 	return -EPROBE_DEFER; /* Wait for the i2c-adapter to load */
 }
 
@@ -212,6 +232,9 @@ static int cht_int33fe_remove(struct i2c_client *i2c)
 	if (data->max17047)
 		i2c_unregister_device(data->max17047);
 
+	mux_remove_table(cht_int33fe_mux_lookup,
+		      ARRAY_SIZE(cht_int33fe_mux_lookup));
+
 	return 0;
 }
 
-- 
2.13.5