[RFC,2/3] usb: roles: Add usb role switch notifier.

Message ID	20191002231617.3670-3-john.stultz@linaro.org (mailing list archive)
State	New, archived
Headers	show Return-Path: <SRS0=o8ur=X3=vger.kernel.org=linux-usb-owner@kernel.org> From: John Stultz <john.stultz@linaro.org> To: lkml <linux-kernel@vger.kernel.org> Cc: Yu Chen <chenyu56@huawei.com>, Greg Kroah-Hartman <gregkh@linuxfoundation.org>, Rob Herring <robh+dt@kernel.org>, Mark Rutland <mark.rutland@arm.com>, Heikki Krogerus <heikki.krogerus@linux.intel.com>, Suzuki K Poulose <suzuki.poulose@arm.com>, Chunfeng Yun <chunfeng.yun@mediatek.com>, Felipe Balbi <balbi@kernel.org>, Hans de Goede <hdegoede@redhat.com>, Andy Shevchenko <andy.shevchenko@gmail.com>, Jun Li <lijun.kernel@gmail.com>, Valentin Schneider <valentin.schneider@arm.com>, linux-usb@vger.kernel.org, devicetree@vger.kernel.org, John Stultz <john.stultz@linaro.org> Subject: [RFC][PATCH 2/3] usb: roles: Add usb role switch notifier. Date: Wed, 2 Oct 2019 23:16:16 +0000 Message-Id: <20191002231617.3670-3-john.stultz@linaro.org> In-Reply-To: <20191002231617.3670-1-john.stultz@linaro.org> References: <20191002231617.3670-1-john.stultz@linaro.org> Sender: linux-usb-owner@vger.kernel.org Precedence: bulk
Series	dwc3 role-switch handling for HiKey960 \| expand [RFC,0/3] dwc3 role-switch handling for HiKey960 [RFC,1/3] dt-bindings: usb: generic: Add role-switch-default-host binding [RFC,2/3] usb: roles: Add usb role switch notifier. [RFC,3/3] usb: dwc3: Registering a role switch in the DRD code.

John Stultz Oct. 2, 2019, 11:16 p.m. UTC

From: Yu Chen <chenyu56@huawei.com>

This patch adds notifier for drivers want to be informed of the usb role
switch.

Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Cc: Rob Herring <robh+dt@kernel.org>
Cc: Mark Rutland <mark.rutland@arm.com>
Cc: Heikki Krogerus <heikki.krogerus@linux.intel.com>
Cc: Suzuki K Poulose <suzuki.poulose@arm.com>
Cc: Chunfeng Yun <chunfeng.yun@mediatek.com>
Cc: Yu Chen <chenyu56@huawei.com>
Cc: Felipe Balbi <balbi@kernel.org>
Cc: Hans de Goede <hdegoede@redhat.com>
Cc: Andy Shevchenko <andy.shevchenko@gmail.com>
Cc: Jun Li <lijun.kernel@gmail.com>
Cc: Valentin Schneider <valentin.schneider@arm.com>
Cc: linux-usb@vger.kernel.org
Cc: devicetree@vger.kernel.org
Suggested-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>
Signed-off-by: Yu Chen <chenyu56@huawei.com>
Signed-off-by: John Stultz <john.stultz@linaro.org>
---
 drivers/usb/roles/class.c | 35 ++++++++++++++++++++++++++++++++++-
 include/linux/usb/role.h  | 16 ++++++++++++++++
 2 files changed, 50 insertions(+), 1 deletion(-)

Hans de Goede Oct. 3, 2019, 9:25 a.m. UTC | #1

Hi John,

On 03-10-2019 01:16, John Stultz wrote:
> From: Yu Chen <chenyu56@huawei.com>
> 
> This patch adds notifier for drivers want to be informed of the usb role
> switch.

I do not see any patches in this series actually using this new
notifier.

Maybe it is best to drop this patch until we actually have in-kernel
users of this new API show up ?

Regards,

Hans


> 
> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> Cc: Rob Herring <robh+dt@kernel.org>
> Cc: Mark Rutland <mark.rutland@arm.com>
> Cc: Heikki Krogerus <heikki.krogerus@linux.intel.com>
> Cc: Suzuki K Poulose <suzuki.poulose@arm.com>
> Cc: Chunfeng Yun <chunfeng.yun@mediatek.com>
> Cc: Yu Chen <chenyu56@huawei.com>
> Cc: Felipe Balbi <balbi@kernel.org>
> Cc: Hans de Goede <hdegoede@redhat.com>
> Cc: Andy Shevchenko <andy.shevchenko@gmail.com>
> Cc: Jun Li <lijun.kernel@gmail.com>
> Cc: Valentin Schneider <valentin.schneider@arm.com>
> Cc: linux-usb@vger.kernel.org
> Cc: devicetree@vger.kernel.org
> Suggested-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>
> Signed-off-by: Yu Chen <chenyu56@huawei.com>
> Signed-off-by: John Stultz <john.stultz@linaro.org>
> ---
>   drivers/usb/roles/class.c | 35 ++++++++++++++++++++++++++++++++++-
>   include/linux/usb/role.h  | 16 ++++++++++++++++
>   2 files changed, 50 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/usb/roles/class.c b/drivers/usb/roles/class.c
> index 94b4e7db2b94..418e762d5d72 100644
> --- a/drivers/usb/roles/class.c
> +++ b/drivers/usb/roles/class.c
> @@ -20,6 +20,7 @@ struct usb_role_switch {
>   	struct device dev;
>   	struct mutex lock; /* device lock*/
>   	enum usb_role role;
> +	struct blocking_notifier_head nh;
>   
>   	/* From descriptor */
>   	struct device *usb2_port;
> @@ -49,8 +50,10 @@ int usb_role_switch_set_role(struct usb_role_switch *sw, enum usb_role role)
>   	mutex_lock(&sw->lock);
>   
>   	ret = sw->set(sw->dev.parent, role);
> -	if (!ret)
> +	if (!ret) {
>   		sw->role = role;
> +		blocking_notifier_call_chain(&sw->nh, role, NULL);
> +	}
>   
>   	mutex_unlock(&sw->lock);
>   
> @@ -58,6 +61,35 @@ int usb_role_switch_set_role(struct usb_role_switch *sw, enum usb_role role)
>   }
>   EXPORT_SYMBOL_GPL(usb_role_switch_set_role);
>   
> +int usb_role_switch_register_notifier(struct usb_role_switch *sw,
> +				      struct notifier_block *nb)
> +{
> +	int ret = blocking_notifier_chain_register(&sw->nh, nb);
> +	enum usb_role role;
> +
> +	if (ret)
> +		return ret;
> +
> +	/* Initialize the notifier that was just registered */
> +	mutex_lock(&sw->lock);
> +	if (sw->get)
> +		role = sw->get(sw->dev.parent);
> +	else
> +		role = sw->role;
> +	blocking_notifier_call_chain(&sw->nh, role, NULL);
> +	mutex_unlock(&sw->lock);
> +
> +	return 0;
> +}
> +EXPORT_SYMBOL_GPL(usb_role_switch_register_notifier);
> +
> +int usb_role_switch_unregister_notifier(struct usb_role_switch *sw,
> +					struct notifier_block *nb)
> +{
> +	return blocking_notifier_chain_unregister(&sw->nh, nb);
> +}
> +EXPORT_SYMBOL_GPL(usb_role_switch_unregister_notifier);
> +
>   /**
>    * usb_role_switch_get_role - Get the USB role for a switch
>    * @sw: USB role switch
> @@ -296,6 +328,7 @@ usb_role_switch_register(struct device *parent,
>   		return ERR_PTR(-ENOMEM);
>   
>   	mutex_init(&sw->lock);
> +	BLOCKING_INIT_NOTIFIER_HEAD(&sw->nh);
>   
>   	sw->allow_userspace_control = desc->allow_userspace_control;
>   	sw->usb2_port = desc->usb2_port;
> diff --git a/include/linux/usb/role.h b/include/linux/usb/role.h
> index 2d77f97df72d..8dbf7940b7da 100644
> --- a/include/linux/usb/role.h
> +++ b/include/linux/usb/role.h
> @@ -54,6 +54,10 @@ struct usb_role_switch *
>   usb_role_switch_register(struct device *parent,
>   			 const struct usb_role_switch_desc *desc);
>   void usb_role_switch_unregister(struct usb_role_switch *sw);
> +int usb_role_switch_register_notifier(struct usb_role_switch *sw,
> +				      struct notifier_block *nb);
> +int usb_role_switch_unregister_notifier(struct usb_role_switch *sw,
> +					struct notifier_block *nb);
>   #else
>   static inline int usb_role_switch_set_role(struct usb_role_switch *sw,
>   		enum usb_role role)
> @@ -87,6 +91,18 @@ usb_role_switch_register(struct device *parent,
>   }
>   
>   static inline void usb_role_switch_unregister(struct usb_role_switch *sw) { }
> +
> +static int usb_role_switch_register_notifier(struct usb_role_switch *sw,
> +					     struct notifier_block *nb)
> +{
> +	return -ENODEV;
> +}
> +
> +static int usb_role_switch_unregister_notifier(struct usb_role_switch *sw,
> +					       struct notifier_block *nb)
> +{
> +	return -ENODEV;
> +}
>   #endif
>   
>   #endif /* __LINUX_USB_ROLE_H */
>

Greg KH Oct. 3, 2019, 11:26 a.m. UTC | #2

On Wed, Oct 02, 2019 at 11:16:16PM +0000, John Stultz wrote:
> From: Yu Chen <chenyu56@huawei.com>
> 
> This patch adds notifier for drivers want to be informed of the usb role
> switch.

Ick, I hate notifiers, they always come back to cause problems.

What's just wrong with a "real" call to who ever needs to know this?
And who does need to know this anyway?  Like Hans said, if we don't have
a user for it, we should not add it.

thanks,

greg k-h

John Stultz Oct. 3, 2019, 8:37 p.m. UTC | #3

On Thu, Oct 3, 2019 at 2:25 AM Hans de Goede <hdegoede@redhat.com> wrote:
> On 03-10-2019 01:16, John Stultz wrote:
> > From: Yu Chen <chenyu56@huawei.com>
> >
> > This patch adds notifier for drivers want to be informed of the usb role
> > switch.
>
> I do not see any patches in this series actually using this new
> notifier.
>
> Maybe it is best to drop this patch until we actually have in-kernel
> users of this new API show up ?

Fair point. I'm sort of taking a larger patchset and trying to break
it up into more easily reviewable chunks, but I guess here I mis-cut.

The user is the hikey960 gpio hub driver here:
  https://git.linaro.org/people/john.stultz/android-dev.git/commit/?id=b06158a2d3eb00c914f12c76c93695e92d9af00f

I'll resubmit again later with that patch included.

thanks
-john

John Stultz Oct. 3, 2019, 8:45 p.m. UTC | #4

On Thu, Oct 3, 2019 at 4:26 AM Greg Kroah-Hartman
<gregkh@linuxfoundation.org> wrote:
>
> On Wed, Oct 02, 2019 at 11:16:16PM +0000, John Stultz wrote:
> > From: Yu Chen <chenyu56@huawei.com>
> >
> > This patch adds notifier for drivers want to be informed of the usb role
> > switch.
>
> Ick, I hate notifiers, they always come back to cause problems.
>
> What's just wrong with a "real" call to who ever needs to know this?
> And who does need to know this anyway?  Like Hans said, if we don't have
> a user for it, we should not add it.

So in this case, its used for interactions between the dwc3 driver and
the hikey960 integrated USB hub, which is controlled via gpio (which I
didn't submit here as I was trying to keep things short and
reviewable, but likely misjudged).

The HiKey960 has only one USB controller, but in order to support both
USB-C gadget/OTG and USB-A (host only) ports. When the USB-C
connection is attached, it powers down and disconnects the hub. When
the USB-C connection is detached, it powers the hub on and connects
the controller to the hub.

This is pretty HiKey960 specific, so I think the notifier is useful to
let the gpio hub logic tie into the role switching events.

Suggestions for alternative approaches?

thanks
-john

Hans de Goede Oct. 3, 2019, 8:51 p.m. UTC | #5

Hi,

On 03-10-2019 22:37, John Stultz wrote:
> On Thu, Oct 3, 2019 at 2:25 AM Hans de Goede <hdegoede@redhat.com> wrote:
>> On 03-10-2019 01:16, John Stultz wrote:
>>> From: Yu Chen <chenyu56@huawei.com>
>>>
>>> This patch adds notifier for drivers want to be informed of the usb role
>>> switch.
>>
>> I do not see any patches in this series actually using this new
>> notifier.
>>
>> Maybe it is best to drop this patch until we actually have in-kernel
>> users of this new API show up ?
> 
> Fair point. I'm sort of taking a larger patchset and trying to break
> it up into more easily reviewable chunks, but I guess here I mis-cut.
> 
> The user is the hikey960 gpio hub driver here:
>    https://git.linaro.org/people/john.stultz/android-dev.git/commit/?id=b06158a2d3eb00c914f12c76c93695e92d9af00f

Hmm, that seems to tie the TypeC data-role to the power-role, which
is not going to work with role swapping.

What is controlling the usb-role-switch, and thus ultimately
causing the notifier you are suggesting to get called ?

Things like TYPEC_VBUS_POWER_OFF and TYPEC_VBUS_POWER_ON
really beg to be modeled as a regulator and then the
Type-C controller (using e.g. the drivers/usb/typec/tcpm/tcpm.c
framework) can use that regulator to control things.
in case of the tcpm.c framework it can then use that
regulator to implement the set_vbus callback.

You really do not want to tie this do the usb_switch, both
because doing so ties the data and power-roles together
which is not supposed to happen and because role-swapping
requires careful timing of the VBUS on / off at different
moments then the moments when you actually set the mux/switch
for connecting the Dp/Dn lines to the host or gadget
controller.

The usb role switch abstraction is really only intended
for the data-lines switch and should not be tied together
with other stuff.

Regards,

Hans

Hans de Goede Oct. 3, 2019, 8:56 p.m. UTC | #6

Hi,

On 03-10-2019 22:45, John Stultz wrote:
> On Thu, Oct 3, 2019 at 4:26 AM Greg Kroah-Hartman
> <gregkh@linuxfoundation.org> wrote:
>>
>> On Wed, Oct 02, 2019 at 11:16:16PM +0000, John Stultz wrote:
>>> From: Yu Chen <chenyu56@huawei.com>
>>>
>>> This patch adds notifier for drivers want to be informed of the usb role
>>> switch.
>>
>> Ick, I hate notifiers, they always come back to cause problems.
>>
>> What's just wrong with a "real" call to who ever needs to know this?
>> And who does need to know this anyway?  Like Hans said, if we don't have
>> a user for it, we should not add it.
> 
> So in this case, its used for interactions between the dwc3 driver and
> the hikey960 integrated USB hub, which is controlled via gpio (which I
> didn't submit here as I was trying to keep things short and
> reviewable, but likely misjudged).
> 
> The HiKey960 has only one USB controller, but in order to support both
> USB-C gadget/OTG and USB-A (host only) ports. When the USB-C
> connection is attached, it powers down and disconnects the hub. When
> the USB-C connection is detached, it powers the hub on and connects
> the controller to the hub.

When you say one controller, do you mean 1 host and 1 gadget controller,
or is this one of these lovely devices where a gadget controller gets
abused as / confused with a proper host controller?

And since you are doing a usb-role-switch driver, I guess that the
role-switch is integrated inside the SoC, so you only get one pair
of USB datalines to the outside ?

This does seem rather special, it might help if you can provide a diagram
with both the relevant bits inside the SoC as well as what lives outside
the Soc. even if it is in ASCII art...

Regards,

Hans

John Stultz Oct. 3, 2019, 9:33 p.m. UTC | #7

On Thu, Oct 3, 2019 at 1:56 PM Hans de Goede <hdegoede@redhat.com> wrote:
> On 03-10-2019 22:45, John Stultz wrote:
> > The HiKey960 has only one USB controller, but in order to support both
> > USB-C gadget/OTG and USB-A (host only) ports. When the USB-C
> > connection is attached, it powers down and disconnects the hub. When
> > the USB-C connection is detached, it powers the hub on and connects
> > the controller to the hub.
>
> When you say one controller, do you mean 1 host and 1 gadget controller,
> or is this one of these lovely devices where a gadget controller gets
> abused as / confused with a proper host controller?

I'm not totally sure myself, but I believe it's the latter, as the
host ports have to be disabled in order for the gadet/otg port to
function.

There was a similar situation w/ the original HiKey board (dwc2
controller) as well, though the switching was done fully in hardware
and we only needed some minor tweaks to the driver to keep the state
transitions straight.

> And since you are doing a usb-role-switch driver, I guess that the
> role-switch is integrated inside the SoC, so you only get one pair
> of USB datalines to the outside ?

I believe so, but again, I don't have a ton of knowledge about the SoC
details, Chen Yu would probably be the right person to answer, but I
don't know if he's doing upstreaming anymore.

> This does seem rather special, it might help if you can provide a diagram
> with both the relevant bits inside the SoC as well as what lives outside
> the Soc. even if it is in ASCII art...

There is a schematic pdf here:
https://github.com/96boards/documentation/raw/master/consumer/hikey/hikey960/hardware-docs/HiKey960_Schematics.pdf

The larger block diagram on page 3 might be helpful, but you can find
more details on the usb hub bits on page 17 and 18.

thanks
-john

Heikki Krogerus Oct. 4, 2019, 8 a.m. UTC | #8

On Thu, Oct 03, 2019 at 10:56:24PM +0200, Hans de Goede wrote:
> Hi,
> 
> On 03-10-2019 22:45, John Stultz wrote:
> > On Thu, Oct 3, 2019 at 4:26 AM Greg Kroah-Hartman
> > <gregkh@linuxfoundation.org> wrote:
> > > 
> > > On Wed, Oct 02, 2019 at 11:16:16PM +0000, John Stultz wrote:
> > > > From: Yu Chen <chenyu56@huawei.com>
> > > > 
> > > > This patch adds notifier for drivers want to be informed of the usb role
> > > > switch.
> > > 
> > > Ick, I hate notifiers, they always come back to cause problems.
> > > 
> > > What's just wrong with a "real" call to who ever needs to know this?
> > > And who does need to know this anyway?  Like Hans said, if we don't have
> > > a user for it, we should not add it.
> > 
> > So in this case, its used for interactions between the dwc3 driver and
> > the hikey960 integrated USB hub, which is controlled via gpio (which I
> > didn't submit here as I was trying to keep things short and
> > reviewable, but likely misjudged).
> > 
> > The HiKey960 has only one USB controller, but in order to support both
> > USB-C gadget/OTG and USB-A (host only) ports. When the USB-C
> > connection is attached, it powers down and disconnects the hub. When
> > the USB-C connection is detached, it powers the hub on and connects
> > the controller to the hub.
> 
> When you say one controller, do you mean 1 host and 1 gadget controller,
> or is this one of these lovely devices where a gadget controller gets
> abused as / confused with a proper host controller?
> 
> And since you are doing a usb-role-switch driver, I guess that the
> role-switch is integrated inside the SoC, so you only get one pair
> of USB datalines to the outside ?

Unless I'm mistaken, the dwc3 driver in this case is the
usb-role-switch. The DWC3 IP includes both USB dost and device blocks,
i.e. it's a dual role controller. Drivers like tcpm.c that negotiate
the actual role need to tell the outcome of the negotiation to the
dwc3 driver. So I think this part is OK.

The platform has also some kind of discrete switch for routing the
signals to either Standard-A (the hub) or Type-C connector, so it does
not represent the usb-role-switch. It should however affect the USB role,
as if that switch routes the data signals to the Standard-A port (to
the hub) instead of USB Type-C, the USB role needs to be fixed to host
mode.

I guess this series does not include the driver for that discrete
switch/mux. I don't remember/know how that switch was planned to be
handled.

> This does seem rather special, it might help if you can provide a diagram
> with both the relevant bits inside the SoC as well as what lives outside
> the Soc. even if it is in ASCII art...

thanks,

Heikki Krogerus Oct. 4, 2019, 8:12 a.m. UTC | #9

On Thu, Oct 03, 2019 at 10:51:38PM +0200, Hans de Goede wrote:
> Hi,
> 
> On 03-10-2019 22:37, John Stultz wrote:
> > On Thu, Oct 3, 2019 at 2:25 AM Hans de Goede <hdegoede@redhat.com> wrote:
> > > On 03-10-2019 01:16, John Stultz wrote:
> > > > From: Yu Chen <chenyu56@huawei.com>
> > > > 
> > > > This patch adds notifier for drivers want to be informed of the usb role
> > > > switch.
> > > 
> > > I do not see any patches in this series actually using this new
> > > notifier.
> > > 
> > > Maybe it is best to drop this patch until we actually have in-kernel
> > > users of this new API show up ?
> > 
> > Fair point. I'm sort of taking a larger patchset and trying to break
> > it up into more easily reviewable chunks, but I guess here I mis-cut.
> > 
> > The user is the hikey960 gpio hub driver here:
> >    https://git.linaro.org/people/john.stultz/android-dev.git/commit/?id=b06158a2d3eb00c914f12c76c93695e92d9af00f
> 
> Hmm, that seems to tie the TypeC data-role to the power-role, which
> is not going to work with role swapping.
> 
> What is controlling the usb-role-switch, and thus ultimately
> causing the notifier you are suggesting to get called ?
> 
> Things like TYPEC_VBUS_POWER_OFF and TYPEC_VBUS_POWER_ON
> really beg to be modeled as a regulator and then the
> Type-C controller (using e.g. the drivers/usb/typec/tcpm/tcpm.c
> framework) can use that regulator to control things.
> in case of the tcpm.c framework it can then use that
> regulator to implement the set_vbus callback.
> 
> You really do not want to tie this do the usb_switch, both
> because doing so ties the data and power-roles together
> which is not supposed to happen and because role-swapping
> requires careful timing of the VBUS on / off at different
> moments then the moments when you actually set the mux/switch
> for connecting the Dp/Dn lines to the host or gadget
> controller.
> 
> The usb role switch abstraction is really only intended
> for the data-lines switch and should not be tied together
> with other stuff.

Hear, hear.

Hans de Goede Oct. 6, 2019, 3:22 p.m. UTC | #10

Hi,

On 10/3/19 11:33 PM, John Stultz wrote:
> On Thu, Oct 3, 2019 at 1:56 PM Hans de Goede <hdegoede@redhat.com> wrote:
>> On 03-10-2019 22:45, John Stultz wrote:
>>> The HiKey960 has only one USB controller, but in order to support both
>>> USB-C gadget/OTG and USB-A (host only) ports. When the USB-C
>>> connection is attached, it powers down and disconnects the hub. When
>>> the USB-C connection is detached, it powers the hub on and connects
>>> the controller to the hub.
>>
>> When you say one controller, do you mean 1 host and 1 gadget controller,
>> or is this one of these lovely devices where a gadget controller gets
>> abused as / confused with a proper host controller?
> 
> I'm not totally sure myself, but I believe it's the latter, as the
> host ports have to be disabled in order for the gadet/otg port to
> function.
> 
> There was a similar situation w/ the original HiKey board (dwc2
> controller) as well, though the switching was done fully in hardware
> and we only needed some minor tweaks to the driver to keep the state
> transitions straight.
> 
>> And since you are doing a usb-role-switch driver, I guess that the
>> role-switch is integrated inside the SoC, so you only get one pair
>> of USB datalines to the outside ?
> 
> I believe so, but again, I don't have a ton of knowledge about the SoC
> details, Chen Yu would probably be the right person to answer, but I
> don't know if he's doing upstreaming anymore.
> 
>> This does seem rather special, it might help if you can provide a diagram
>> with both the relevant bits inside the SoC as well as what lives outside
>> the Soc. even if it is in ASCII art...
> 
> There is a schematic pdf here:
> https://github.com/96boards/documentation/raw/master/consumer/hikey/hikey960/hardware-docs/HiKey960_Schematics.pdf
> 
> The larger block diagram on page 3 might be helpful, but you can find
> more details on the usb hub bits on page 17 and 18.

Ok, so I took a quick look at the schematic and it is really funky.

The USB3 superspeed data pairs are only going to the USB-3 hub and
only the USB-2 lines are muxed between the TypeC and the HUB, so
in theory superspeed devices could keep working while the TypeC is
in device mode, since their data lines will still be connected,
but I guess the controller in the SoC is switched to device mode
then so this does not work. Likewise Vbus is an all or
nothing thing, either both the TypeC connector + the 2 Type-A
reeptacles get Vusb or none of them get Vusb. Also it is seems to use
the TypeC connector in host-mode together with the A receptacles.
I must say this is a weird design...

Anyways back the code to add a usb role switch notifier. I do
not think that this is a good idea, this is making "core" changes
to deal with a special case. If you are going to use a notfier for
this then IMHO the notifier should be part of the hikey960 usb role
swtich driver and not be in the usb-role-switch class code, since
this is very much a device specific hack.

Regards,

Hans

John Stultz Oct. 15, 2019, 5:39 a.m. UTC | #11

On Thu, Oct 3, 2019 at 1:51 PM Hans de Goede <hdegoede@redhat.com> wrote:
> On 03-10-2019 22:37, John Stultz wrote:
> > Fair point. I'm sort of taking a larger patchset and trying to break
> > it up into more easily reviewable chunks, but I guess here I mis-cut.
> >
> > The user is the hikey960 gpio hub driver here:
> >    https://git.linaro.org/people/john.stultz/android-dev.git/commit/?id=b06158a2d3eb00c914f12c76c93695e92d9af00f
>
> Hmm, that seems to tie the TypeC data-role to the power-role, which
> is not going to work with role swapping.

Thanks again for the feedback here. Sorry for the slow response. Been
reworking some of the easier changes but am starting to look at how to
address your feedback here.

> What is controlling the usb-role-switch, and thus ultimately
> causing the notifier you are suggesting to get called ?

The tcpm_mux_set() call via tcpm_state_machine_work()

> Things like TYPEC_VBUS_POWER_OFF and TYPEC_VBUS_POWER_ON
> really beg to be modeled as a regulator and then the
> Type-C controller (using e.g. the drivers/usb/typec/tcpm/tcpm.c
> framework) can use that regulator to control things.
> in case of the tcpm.c framework it can then use that
> regulator to implement the set_vbus callback.

So I'm looking at the bindings and I'm not sure exactly how to tie a
regulator style driver into the tcpm for this?
Looking at the driver I just see this commented out bit:
   https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/usb/typec/tcpm/tcpm.c#n3075

Do you happen to have a pointer to something closer to what you are describing?

thanks
-john

John Stultz Oct. 15, 2019, 7:03 a.m. UTC | #12

On Sun, Oct 6, 2019 at 8:22 AM Hans de Goede <hdegoede@redhat.com> wrote:
>
> Anyways back the code to add a usb role switch notifier. I do
> not think that this is a good idea, this is making "core" changes
> to deal with a special case. If you are going to use a notfier for
> this then IMHO the notifier should be part of the hikey960 usb role
> swtich driver and not be in the usb-role-switch class code, since
> this is very much a device specific hack.

Ok, that sounds fair.  I still need to find some way to hook into the
role-switch path between the tcpm and the dwc3 in order to switch to
the hub, but I guess I can try to add a hook somewhere in the dwc3
code itself. I'll dig on this a bit.

thanks
-john

Hans de Goede Oct. 16, 2019, 7:27 a.m. UTC | #13

Hi,

On 10/15/19 7:39 AM, John Stultz wrote:
> On Thu, Oct 3, 2019 at 1:51 PM Hans de Goede <hdegoede@redhat.com> wrote:
>> On 03-10-2019 22:37, John Stultz wrote:
>>> Fair point. I'm sort of taking a larger patchset and trying to break
>>> it up into more easily reviewable chunks, but I guess here I mis-cut.
>>>
>>> The user is the hikey960 gpio hub driver here:
>>>     https://git.linaro.org/people/john.stultz/android-dev.git/commit/?id=b06158a2d3eb00c914f12c76c93695e92d9af00f
>>
>> Hmm, that seems to tie the TypeC data-role to the power-role, which
>> is not going to work with role swapping.
> 
> Thanks again for the feedback here. Sorry for the slow response. Been
> reworking some of the easier changes but am starting to look at how to
> address your feedback here.
> 
>> What is controlling the usb-role-switch, and thus ultimately
>> causing the notifier you are suggesting to get called ?
> 
> The tcpm_mux_set() call via tcpm_state_machine_work()
> 
>> Things like TYPEC_VBUS_POWER_OFF and TYPEC_VBUS_POWER_ON
>> really beg to be modeled as a regulator and then the
>> Type-C controller (using e.g. the drivers/usb/typec/tcpm/tcpm.c
>> framework) can use that regulator to control things.
>> in case of the tcpm.c framework it can then use that
>> regulator to implement the set_vbus callback.
> 
> So I'm looking at the bindings and I'm not sure exactly how to tie a
> regulator style driver into the tcpm for this?
> Looking at the driver I just see this commented out bit:
>     https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/usb/typec/tcpm/tcpm.c#n3075
> 
> Do you happen to have a pointer to something closer to what you are describing?

Look at the tcpm_set_vbus implementation in drivers/usb/typec/tcpm/fusb302.c
you need to do something similar in your Type-C controller driver and
export the GPIO as as a gpio-controlled regulator and tie the regulator to
the connector.

Regards,

Hans

Hans de Goede Oct. 16, 2019, 9:10 a.m. UTC | #14

Hi,

On 10/15/19 7:39 AM, John Stultz wrote:
> On Thu, Oct 3, 2019 at 1:51 PM Hans de Goede <hdegoede@redhat.com> wrote:
>> On 03-10-2019 22:37, John Stultz wrote:
>>> Fair point. I'm sort of taking a larger patchset and trying to break
>>> it up into more easily reviewable chunks, but I guess here I mis-cut.
>>>
>>> The user is the hikey960 gpio hub driver here:
>>>     https://git.linaro.org/people/john.stultz/android-dev.git/commit/?id=b06158a2d3eb00c914f12c76c93695e92d9af00f
>>
>> Hmm, that seems to tie the TypeC data-role to the power-role, which
>> is not going to work with role swapping.
> 
> Thanks again for the feedback here. Sorry for the slow response. Been
> reworking some of the easier changes but am starting to look at how to
> address your feedback here.
> 
>> What is controlling the usb-role-switch, and thus ultimately
>> causing the notifier you are suggesting to get called ?
> 
> The tcpm_mux_set() call via tcpm_state_machine_work()
> 
>> Things like TYPEC_VBUS_POWER_OFF and TYPEC_VBUS_POWER_ON
>> really beg to be modeled as a regulator and then the
>> Type-C controller (using e.g. the drivers/usb/typec/tcpm/tcpm.c
>> framework) can use that regulator to control things.
>> in case of the tcpm.c framework it can then use that
>> regulator to implement the set_vbus callback.
> 
> So I'm looking at the bindings and I'm not sure exactly how to tie a
> regulator style driver into the tcpm for this?
> Looking at the driver I just see this commented out bit:
>     https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/usb/typec/tcpm/tcpm.c#n3075
> 
> Do you happen to have a pointer to something closer to what you are describing?

Look at the tcpm_set_vbus implementation in drivers/usb/typec/tcpm/fusb302.c
you need to do something similar in your Type-C controller driver and
export the GPIO as as a gpio-controlled regulator and tie the regulator to
the connector.

Regards,

Han

John Stultz Oct. 18, 2019, 5:55 a.m. UTC | #15

On Wed, Oct 16, 2019 at 12:27 AM Hans de Goede <hdegoede@redhat.com> wrote:
> On 10/15/19 7:39 AM, John Stultz wrote:
> > On Thu, Oct 3, 2019 at 1:51 PM Hans de Goede <hdegoede@redhat.com> wrote:
> >> On 03-10-2019 22:37, John Stultz wrote:
> >>> Fair point. I'm sort of taking a larger patchset and trying to break
> >>> it up into more easily reviewable chunks, but I guess here I mis-cut.
> >>>
> >>> The user is the hikey960 gpio hub driver here:
> >>>     https://git.linaro.org/people/john.stultz/android-dev.git/commit/?id=b06158a2d3eb00c914f12c76c93695e92d9af00f
> >>
> >> Hmm, that seems to tie the TypeC data-role to the power-role, which
> >> is not going to work with role swapping.
> >
> > Thanks again for the feedback here. Sorry for the slow response. Been
> > reworking some of the easier changes but am starting to look at how to
> > address your feedback here.
> >
> >> What is controlling the usb-role-switch, and thus ultimately
> >> causing the notifier you are suggesting to get called ?
> >
> > The tcpm_mux_set() call via tcpm_state_machine_work()
> >
> >> Things like TYPEC_VBUS_POWER_OFF and TYPEC_VBUS_POWER_ON
> >> really beg to be modeled as a regulator and then the
> >> Type-C controller (using e.g. the drivers/usb/typec/tcpm/tcpm.c
> >> framework) can use that regulator to control things.
> >> in case of the tcpm.c framework it can then use that
> >> regulator to implement the set_vbus callback.
> >
> > So I'm looking at the bindings and I'm not sure exactly how to tie a
> > regulator style driver into the tcpm for this?
> > Looking at the driver I just see this commented out bit:
> >     https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/usb/typec/tcpm/tcpm.c#n3075
> >
> > Do you happen to have a pointer to something closer to what you are describing?
>
> Look at the tcpm_set_vbus implementation in drivers/usb/typec/tcpm/fusb302.c
> you need to do something similar in your Type-C controller driver and
> export the GPIO as as a gpio-controlled regulator and tie the regulator to
> the connector.

Thanks for the suggestion, I really appreciate it! One more question
though, since I'm using the tcpci_rt1711h driver, which re-uses the
somewhat sparse tcpci.c implementation, would you recommend trying to
add generic regulator support to the tcpci code or trying to extend
the implementation somehow allow the tcpci_rt1711h driver replace just
the set_vbus function?

thanks
-john

Hans de Goede Oct. 18, 2019, 8:06 a.m. UTC | #16

Hi,

On 18-10-2019 07:55, John Stultz wrote:
> On Wed, Oct 16, 2019 at 12:27 AM Hans de Goede <hdegoede@redhat.com> wrote:
>> On 10/15/19 7:39 AM, John Stultz wrote:
>>> On Thu, Oct 3, 2019 at 1:51 PM Hans de Goede <hdegoede@redhat.com> wrote:
>>>> On 03-10-2019 22:37, John Stultz wrote:
>>>>> Fair point. I'm sort of taking a larger patchset and trying to break
>>>>> it up into more easily reviewable chunks, but I guess here I mis-cut.
>>>>>
>>>>> The user is the hikey960 gpio hub driver here:
>>>>>      https://git.linaro.org/people/john.stultz/android-dev.git/commit/?id=b06158a2d3eb00c914f12c76c93695e92d9af00f
>>>>
>>>> Hmm, that seems to tie the TypeC data-role to the power-role, which
>>>> is not going to work with role swapping.
>>>
>>> Thanks again for the feedback here. Sorry for the slow response. Been
>>> reworking some of the easier changes but am starting to look at how to
>>> address your feedback here.
>>>
>>>> What is controlling the usb-role-switch, and thus ultimately
>>>> causing the notifier you are suggesting to get called ?
>>>
>>> The tcpm_mux_set() call via tcpm_state_machine_work()
>>>
>>>> Things like TYPEC_VBUS_POWER_OFF and TYPEC_VBUS_POWER_ON
>>>> really beg to be modeled as a regulator and then the
>>>> Type-C controller (using e.g. the drivers/usb/typec/tcpm/tcpm.c
>>>> framework) can use that regulator to control things.
>>>> in case of the tcpm.c framework it can then use that
>>>> regulator to implement the set_vbus callback.
>>>
>>> So I'm looking at the bindings and I'm not sure exactly how to tie a
>>> regulator style driver into the tcpm for this?
>>> Looking at the driver I just see this commented out bit:
>>>      https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/usb/typec/tcpm/tcpm.c#n3075
>>>
>>> Do you happen to have a pointer to something closer to what you are describing?
>>
>> Look at the tcpm_set_vbus implementation in drivers/usb/typec/tcpm/fusb302.c
>> you need to do something similar in your Type-C controller driver and
>> export the GPIO as as a gpio-controlled regulator and tie the regulator to
>> the connector.
> 
> Thanks for the suggestion, I really appreciate it! One more question
> though, since I'm using the tcpci_rt1711h driver, which re-uses the
> somewhat sparse tcpci.c implementation, would you recommend trying to
> add generic regulator support to the tcpci code or trying to extend
> the implementation somehow allow the tcpci_rt1711h driver replace just
> the set_vbus function?

I have the feeling that this is more of a question for Heikki.

My first instinct is: if you are using tcpci can't you put all
the hacks you need for the usb connection shared between hub
and type-c in your firmware ?

Regards,

Hans

John Stultz Oct. 18, 2019, 6:39 p.m. UTC | #17

On Fri, Oct 18, 2019 at 1:06 AM Hans de Goede <hdegoede@redhat.com> wrote:
> On 18-10-2019 07:55, John Stultz wrote:
> > On Wed, Oct 16, 2019 at 12:27 AM Hans de Goede <hdegoede@redhat.com> wrote:
> >> Look at the tcpm_set_vbus implementation in drivers/usb/typec/tcpm/fusb302.c
> >> you need to do something similar in your Type-C controller driver and
> >> export the GPIO as as a gpio-controlled regulator and tie the regulator to
> >> the connector.
> >
> > Thanks for the suggestion, I really appreciate it! One more question
> > though, since I'm using the tcpci_rt1711h driver, which re-uses the
> > somewhat sparse tcpci.c implementation, would you recommend trying to
> > add generic regulator support to the tcpci code or trying to extend
> > the implementation somehow allow the tcpci_rt1711h driver replace just
> > the set_vbus function?
>
> I have the feeling that this is more of a question for Heikki.
>
> My first instinct is: if you are using tcpci can't you put all
> the hacks you need for the usb connection shared between hub
> and type-c in your firmware ?

I appreciate the suggestion, but I'm not aware of any USB firmware for
the board, nor do I think I have any such source.  :(

thanks
-john

Hans de Goede Oct. 18, 2019, 7:30 p.m. UTC | #18

Hi,

On 18-10-2019 20:39, John Stultz wrote:
> On Fri, Oct 18, 2019 at 1:06 AM Hans de Goede <hdegoede@redhat.com> wrote:
>> On 18-10-2019 07:55, John Stultz wrote:
>>> On Wed, Oct 16, 2019 at 12:27 AM Hans de Goede <hdegoede@redhat.com> wrote:
>>>> Look at the tcpm_set_vbus implementation in drivers/usb/typec/tcpm/fusb302.c
>>>> you need to do something similar in your Type-C controller driver and
>>>> export the GPIO as as a gpio-controlled regulator and tie the regulator to
>>>> the connector.
>>>
>>> Thanks for the suggestion, I really appreciate it! One more question
>>> though, since I'm using the tcpci_rt1711h driver, which re-uses the
>>> somewhat sparse tcpci.c implementation, would you recommend trying to
>>> add generic regulator support to the tcpci code or trying to extend
>>> the implementation somehow allow the tcpci_rt1711h driver replace just
>>> the set_vbus function?
>>
>> I have the feeling that this is more of a question for Heikki.
>>
>> My first instinct is: if you are using tcpci can't you put all
>> the hacks you need for the usb connection shared between hub
>> and type-c in your firmware ?
> 
> I appreciate the suggestion, but I'm not aware of any USB firmware for
> the board, nor do I think I have any such source.  :(

My bad, I was under the impression that tcpci was a firmware interface,
but it is not (I was confusing it with ucsi).

Looking at drivers/usb/typec/tcpm/tcpci.c: tcpci_set_vconn I see that
there is a data struct with vendor specific callbacks and that the
drivers/usb/typec/tcpm/tcpci_rt1711h.c implements that.

So you may want something similar here. But things are tricky here,
because when nothing is connected you want to provide Vbus for
the USB-A ports, which means that if someone then connects a
USB-A to C cable to connect the board to a PC (switching the port
to device mode) there will be a time when both sides are supplying
5V if I remember the schedule correctly.

I think that the original hack might not be that bad, the whole hw
design seems so, erm, broken, that you probably cannot do proper
roleswapping anyways.  So just tying Vbus to host mode might be
fine, the question then becomes again how can some other piece
of code listen to the role-switch events...

Regards,

Hans

John Stultz Oct. 18, 2019, 7:53 p.m. UTC | #19

On Fri, Oct 18, 2019 at 12:30 PM Hans de Goede <hdegoede@redhat.com> wrote:
> Looking at drivers/usb/typec/tcpm/tcpci.c: tcpci_set_vconn I see that
> there is a data struct with vendor specific callbacks and that the
> drivers/usb/typec/tcpm/tcpci_rt1711h.c implements that.
>
> So you may want something similar here. But things are tricky here,
> because when nothing is connected you want to provide Vbus for
> the USB-A ports, which means that if someone then connects a
> USB-A to C cable to connect the board to a PC (switching the port
> to device mode) there will be a time when both sides are supplying
> 5V if I remember the schedule correctly.

Ok. Thanks for the pointer, I'll take a look at that to see if I can
get it to work.

> I think that the original hack might not be that bad, the whole hw
> design seems so, erm, broken, that you probably cannot do proper
> roleswapping anyways.  So just tying Vbus to host mode might be
> fine, the question then becomes again how can some other piece
> of code listen to the role-switch events...

So, at least in the current approach (see the v3 series), I've
basically set the hub driver as an role-switch intermediary, sitting
between the calls from the tcpm to the dwc3 driver. It actually works
better then the earlier notifier method (which had some issues with
reliably establishing the initial state on boot).  Does that approach
work for you?

thanks
-john

Hans de Goede Oct. 18, 2019, 7:59 p.m. UTC | #20

Hi,

On 18-10-2019 21:53, John Stultz wrote:
> On Fri, Oct 18, 2019 at 12:30 PM Hans de Goede <hdegoede@redhat.com> wrote:
>> Looking at drivers/usb/typec/tcpm/tcpci.c: tcpci_set_vconn I see that
>> there is a data struct with vendor specific callbacks and that the
>> drivers/usb/typec/tcpm/tcpci_rt1711h.c implements that.
>>
>> So you may want something similar here. But things are tricky here,
>> because when nothing is connected you want to provide Vbus for
>> the USB-A ports, which means that if someone then connects a
>> USB-A to C cable to connect the board to a PC (switching the port
>> to device mode) there will be a time when both sides are supplying
>> 5V if I remember the schedule correctly.
> 
> Ok. Thanks for the pointer, I'll take a look at that to see if I can
> get it to work.
> 
>> I think that the original hack might not be that bad, the whole hw
>> design seems so, erm, broken, that you probably cannot do proper
>> roleswapping anyways.  So just tying Vbus to host mode might be
>> fine, the question then becomes again how can some other piece
>> of code listen to the role-switch events...
> 
> So, at least in the current approach (see the v3 series), I've
> basically set the hub driver as an role-switch intermediary, sitting
> between the calls from the tcpm to the dwc3 driver. It actually works
> better then the earlier notifier method (which had some issues with
> reliably establishing the initial state on boot).  Does that approach
> work for you?

That sounds like it might be a nice solution. But I have not seen the
code, I think I was not Cc-ed on v3. Do you have a patchwork or
lore.kernel.org link for me?

Regards,

Hans

John Stultz Oct. 18, 2019, 8:12 p.m. UTC | #21

On Fri, Oct 18, 2019 at 12:59 PM Hans de Goede <hdegoede@redhat.com> wrote:
> On 18-10-2019 21:53, John Stultz wrote:
> > On Fri, Oct 18, 2019 at 12:30 PM Hans de Goede <hdegoede@redhat.com> wrote:
> >> Looking at drivers/usb/typec/tcpm/tcpci.c: tcpci_set_vconn I see that
> >> there is a data struct with vendor specific callbacks and that the
> >> drivers/usb/typec/tcpm/tcpci_rt1711h.c implements that.
> >>
> >> So you may want something similar here. But things are tricky here,
> >> because when nothing is connected you want to provide Vbus for
> >> the USB-A ports, which means that if someone then connects a
> >> USB-A to C cable to connect the board to a PC (switching the port
> >> to device mode) there will be a time when both sides are supplying
> >> 5V if I remember the schedule correctly.
> >
> > Ok. Thanks for the pointer, I'll take a look at that to see if I can
> > get it to work.
> >
> >> I think that the original hack might not be that bad, the whole hw
> >> design seems so, erm, broken, that you probably cannot do proper
> >> roleswapping anyways.  So just tying Vbus to host mode might be
> >> fine, the question then becomes again how can some other piece
> >> of code listen to the role-switch events...
> >
> > So, at least in the current approach (see the v3 series), I've
> > basically set the hub driver as an role-switch intermediary, sitting
> > between the calls from the tcpm to the dwc3 driver. It actually works
> > better then the earlier notifier method (which had some issues with
> > reliably establishing the initial state on boot).  Does that approach
> > work for you?
>
> That sounds like it might be a nice solution. But I have not seen the
> code, I think I was not Cc-ed on v3. Do you have a patchwork or
> lore.kernel.org link for me?

Oh! I think I had you on CC, maybe it got caught in your spam folder?
My apologies either way! The thread is here:
  https://lore.kernel.org/lkml/20191016033340.1288-1-john.stultz@linaro.org/

And the hub/role-switch-intermediary driver is here:
  https://lore.kernel.org/lkml/20191016033340.1288-12-john.stultz@linaro.org/

thanks
-john

Hans de Goede Oct. 18, 2019, 8:21 p.m. UTC | #22

Hi,

On 18-10-2019 22:12, John Stultz wrote:
> On Fri, Oct 18, 2019 at 12:59 PM Hans de Goede <hdegoede@redhat.com> wrote:
>> On 18-10-2019 21:53, John Stultz wrote:
>>> On Fri, Oct 18, 2019 at 12:30 PM Hans de Goede <hdegoede@redhat.com> wrote:
>>>> Looking at drivers/usb/typec/tcpm/tcpci.c: tcpci_set_vconn I see that
>>>> there is a data struct with vendor specific callbacks and that the
>>>> drivers/usb/typec/tcpm/tcpci_rt1711h.c implements that.
>>>>
>>>> So you may want something similar here. But things are tricky here,
>>>> because when nothing is connected you want to provide Vbus for
>>>> the USB-A ports, which means that if someone then connects a
>>>> USB-A to C cable to connect the board to a PC (switching the port
>>>> to device mode) there will be a time when both sides are supplying
>>>> 5V if I remember the schedule correctly.
>>>
>>> Ok. Thanks for the pointer, I'll take a look at that to see if I can
>>> get it to work.
>>>
>>>> I think that the original hack might not be that bad, the whole hw
>>>> design seems so, erm, broken, that you probably cannot do proper
>>>> roleswapping anyways.  So just tying Vbus to host mode might be
>>>> fine, the question then becomes again how can some other piece
>>>> of code listen to the role-switch events...
>>>
>>> So, at least in the current approach (see the v3 series), I've
>>> basically set the hub driver as an role-switch intermediary, sitting
>>> between the calls from the tcpm to the dwc3 driver. It actually works
>>> better then the earlier notifier method (which had some issues with
>>> reliably establishing the initial state on boot).  Does that approach
>>> work for you?
>>
>> That sounds like it might be a nice solution. But I have not seen the
>> code, I think I was not Cc-ed on v3. Do you have a patchwork or
>> lore.kernel.org link for me?
> 
> Oh! I think I had you on CC, maybe it got caught in your spam folder?

More likely I just deleted mail to aggressively, sorry.

> My apologies either way! The thread is here:
>    https://lore.kernel.org/lkml/20191016033340.1288-1-john.stultz@linaro.org/
> 
> And the hub/role-switch-intermediary driver is here:
>    https://lore.kernel.org/lkml/20191016033340.1288-12-john.stultz@linaro.org/

Hm, this looks very nice actually, much much better then the notifier stuff!

As for your:

"NOTE: It was noted that controlling the TYPEC_VBUS_POWER_OFF and
TYPEC_VBUS_POWER_ON values here is not reccomended. I'm looking
for a way to remove that bit from the logic here, but wanted to
still get feedback on this approach."

Comment in the commit message, normally a type-c port would turn external Vbus
off until a sink is connected, IIRC the same Vbus is also used for the TupeA ports
on the hub, so that would mean those are unusable when nothing is connected to
the TypeC port, which is not what you want.

So I think that given the special case / hack-ish hw you have, that just setting
Vbus based on the role is ok(ish).

Regards,

Hans

John Stultz Oct. 18, 2019, 8:37 p.m. UTC | #23

On Fri, Oct 18, 2019 at 1:21 PM Hans de Goede <hdegoede@redhat.com> wrote:
> On 18-10-2019 22:12, John Stultz wrote:
> > On Fri, Oct 18, 2019 at 12:59 PM Hans de Goede <hdegoede@redhat.com> wrote:
> >> On 18-10-2019 21:53, John Stultz wrote:
> >>> On Fri, Oct 18, 2019 at 12:30 PM Hans de Goede <hdegoede@redhat.com> wrote:
> >>>> Looking at drivers/usb/typec/tcpm/tcpci.c: tcpci_set_vconn I see that
> >>>> there is a data struct with vendor specific callbacks and that the
> >>>> drivers/usb/typec/tcpm/tcpci_rt1711h.c implements that.
> >>>>
> >>>> So you may want something similar here. But things are tricky here,
> >>>> because when nothing is connected you want to provide Vbus for
> >>>> the USB-A ports, which means that if someone then connects a
> >>>> USB-A to C cable to connect the board to a PC (switching the port
> >>>> to device mode) there will be a time when both sides are supplying
> >>>> 5V if I remember the schedule correctly.
> >>>
> >>> Ok. Thanks for the pointer, I'll take a look at that to see if I can
> >>> get it to work.
> >>>
> >>>> I think that the original hack might not be that bad, the whole hw
> >>>> design seems so, erm, broken, that you probably cannot do proper
> >>>> roleswapping anyways.  So just tying Vbus to host mode might be
> >>>> fine, the question then becomes again how can some other piece
> >>>> of code listen to the role-switch events...
> >>>
> >>> So, at least in the current approach (see the v3 series), I've
> >>> basically set the hub driver as an role-switch intermediary, sitting
> >>> between the calls from the tcpm to the dwc3 driver. It actually works
> >>> better then the earlier notifier method (which had some issues with
> >>> reliably establishing the initial state on boot).  Does that approach
> >>> work for you?
> >>
> >> That sounds like it might be a nice solution. But I have not seen the
> >> code, I think I was not Cc-ed on v3. Do you have a patchwork or
> >> lore.kernel.org link for me?
> >
> > Oh! I think I had you on CC, maybe it got caught in your spam folder?
>
> More likely I just deleted mail to aggressively, sorry.
>
> > My apologies either way! The thread is here:
> >    https://lore.kernel.org/lkml/20191016033340.1288-1-john.stultz@linaro.org/
> >
> > And the hub/role-switch-intermediary driver is here:
> >    https://lore.kernel.org/lkml/20191016033340.1288-12-john.stultz@linaro.org/
>
> Hm, this looks very nice actually, much much better then the notifier stuff!
>
> As for your:
>
> "NOTE: It was noted that controlling the TYPEC_VBUS_POWER_OFF and
> TYPEC_VBUS_POWER_ON values here is not reccomended. I'm looking
> for a way to remove that bit from the logic here, but wanted to
> still get feedback on this approach."
>
> Comment in the commit message, normally a type-c port would turn external Vbus
> off until a sink is connected, IIRC the same Vbus is also used for the TupeA ports
> on the hub, so that would mean those are unusable when nothing is connected to
> the TypeC port, which is not what you want.

Uh, so I think for the HiKey960, the type-A ports on the hub are
separately powered via the hub_power_ctrl(hisi_hikey_usb,
HUB_VBUS_POWER_OFF/ON) call.

At least, with the current driver, the functionality is working as
expected: remove the USB-C cable, and devices connected to the hub
power on, plug something into the USB-C port and devices plugged into
the hub shutdown.

But maybe I'm missing what you mean?

> So I think that given the special case / hack-ish hw you have, that just setting
> Vbus based on the role is ok(ish).

Ok. I'm happy to stick with what works here, since it is at least the
oddness is isolated to the device specific hub driver.

thanks
-john

Hans de Goede Oct. 18, 2019, 9:05 p.m. UTC | #24

Hi,

On 18-10-2019 22:37, John Stultz wrote:
> On Fri, Oct 18, 2019 at 1:21 PM Hans de Goede <hdegoede@redhat.com> wrote:
>> On 18-10-2019 22:12, John Stultz wrote:
>>> On Fri, Oct 18, 2019 at 12:59 PM Hans de Goede <hdegoede@redhat.com> wrote:
>>>> On 18-10-2019 21:53, John Stultz wrote:
>>>>> On Fri, Oct 18, 2019 at 12:30 PM Hans de Goede <hdegoede@redhat.com> wrote:
>>>>>> Looking at drivers/usb/typec/tcpm/tcpci.c: tcpci_set_vconn I see that
>>>>>> there is a data struct with vendor specific callbacks and that the
>>>>>> drivers/usb/typec/tcpm/tcpci_rt1711h.c implements that.
>>>>>>
>>>>>> So you may want something similar here. But things are tricky here,
>>>>>> because when nothing is connected you want to provide Vbus for
>>>>>> the USB-A ports, which means that if someone then connects a
>>>>>> USB-A to C cable to connect the board to a PC (switching the port
>>>>>> to device mode) there will be a time when both sides are supplying
>>>>>> 5V if I remember the schedule correctly.
>>>>>
>>>>> Ok. Thanks for the pointer, I'll take a look at that to see if I can
>>>>> get it to work.
>>>>>
>>>>>> I think that the original hack might not be that bad, the whole hw
>>>>>> design seems so, erm, broken, that you probably cannot do proper
>>>>>> roleswapping anyways.  So just tying Vbus to host mode might be
>>>>>> fine, the question then becomes again how can some other piece
>>>>>> of code listen to the role-switch events...
>>>>>
>>>>> So, at least in the current approach (see the v3 series), I've
>>>>> basically set the hub driver as an role-switch intermediary, sitting
>>>>> between the calls from the tcpm to the dwc3 driver. It actually works
>>>>> better then the earlier notifier method (which had some issues with
>>>>> reliably establishing the initial state on boot).  Does that approach
>>>>> work for you?
>>>>
>>>> That sounds like it might be a nice solution. But I have not seen the
>>>> code, I think I was not Cc-ed on v3. Do you have a patchwork or
>>>> lore.kernel.org link for me?
>>>
>>> Oh! I think I had you on CC, maybe it got caught in your spam folder?
>>
>> More likely I just deleted mail to aggressively, sorry.
>>
>>> My apologies either way! The thread is here:
>>>     https://lore.kernel.org/lkml/20191016033340.1288-1-john.stultz@linaro.org/
>>>
>>> And the hub/role-switch-intermediary driver is here:
>>>     https://lore.kernel.org/lkml/20191016033340.1288-12-john.stultz@linaro.org/
>>
>> Hm, this looks very nice actually, much much better then the notifier stuff!
>>
>> As for your:
>>
>> "NOTE: It was noted that controlling the TYPEC_VBUS_POWER_OFF and
>> TYPEC_VBUS_POWER_ON values here is not reccomended. I'm looking
>> for a way to remove that bit from the logic here, but wanted to
>> still get feedback on this approach."
>>
>> Comment in the commit message, normally a type-c port would turn external Vbus
>> off until a sink is connected, IIRC the same Vbus is also used for the TupeA ports
>> on the hub, so that would mean those are unusable when nothing is connected to
>> the TypeC port, which is not what you want.
> 
> Uh, so I think for the HiKey960, the type-A ports on the hub are
> separately powered via the hub_power_ctrl(hisi_hikey_usb,
> HUB_VBUS_POWER_OFF/ON) call.
> 
> At least, with the current driver, the functionality is working as
> expected: remove the USB-C cable, and devices connected to the hub
> power on, plug something into the USB-C port and devices plugged into
> the hub shutdown.
> 
> But maybe I'm missing what you mean?

Ok, so double checking the schematic I do see separate Vbus-es for the
TypeC port and the TypeA ports, with the TypeC port one being controlled
by GPIO_202_VBUS_TYPEC. So ideally that gpio would be  controlled to
enable/disable vbus by the tcpm framework.

>> So I think that given the special case / hack-ish hw you have, that just setting
>> Vbus based on the role is ok(ish).
> 
> Ok. I'm happy to stick with what works here, since it is at least the
> oddness is isolated to the device specific hub driver.

Right, so for the Type-A ports Vbus controlled by PRT_CTL1 enabling it depending
on host vs devices mode makes sense. But the Type-C one really should be
controlled by the tcpm framework.

Regards,

Hans

p.s.

Sorry for the confusion I was under the impression that there was only 1
Vbus enable for both Type-A and Type-C ports.

John Stultz Oct. 22, 2019, 5:58 a.m. UTC | #25

On Fri, Oct 18, 2019 at 2:05 PM Hans de Goede <hdegoede@redhat.com> wrote:
> On 18-10-2019 22:37, John Stultz wrote:
> > At least, with the current driver, the functionality is working as
> > expected: remove the USB-C cable, and devices connected to the hub
> > power on, plug something into the USB-C port and devices plugged into
> > the hub shutdown.
> >
> > But maybe I'm missing what you mean?
>
> Ok, so double checking the schematic I do see separate Vbus-es for the
> TypeC port and the TypeA ports, with the TypeC port one being controlled
> by GPIO_202_VBUS_TYPEC. So ideally that gpio would be  controlled to
> enable/disable vbus by the tcpm framework.

So I've given this a shot, adding a gpio regulator for the type-c
vbus, and added a set_vbus hook to the tcpci_rt1711 with logic to
enable and disable the regulator depending on the source state.  I've
also added some debug logic to check the regulator disabling/enabling
is working properly. However, doing the type-c vbus control via the
tcpm logic doesn't seem to be working properly.

The issue seems to be when the USB-C cable is unplugged the device
goes into ROLE_NONE, we switch to the on-board hub. Then when we
connect a USB-C hub to the type-c port, we switch to ROLE_HOST, and
power on the regulator, and that starts to power on the USB-C hub
devices. However, since this disconnects/powers down the on-board hub,
we see the on-board hub device disconnect. I'm guessing the hub
disconnection causes some confusion in the state machine, as then I
see the state change from state change SRC_ATTACHED -> SRC_UNATTACHED,
and set_vbus is immediately called with source=0 and the regulator is
disabled, and we switch back to ROLE_NONE (which powers on the onboard
hub).  The system then seems to quickly oscillate between the
ROLE_HOST and ROLE_NONE switching the regulator on and off fairly
quickly (see log below for more details) and never really settling for
one state or the other.

Any off-hand thoughts on what might be going wrong here? I'm fine to
continue digging and working on this approach, but I also don't want
to have to pollute the core code too much for this oddball hardware
(esp since doing the vbus control in the role-switch intermediary does
work ok - or at least better then this approach so far).

thanks
-john


Starts in ROLE_NONE with nothing connected to type-c port, with the
on-board hub powered on, then we connect a type-c usb hub.

[   57.828323] JDB: tcpm_state_machine_work state chage:
SRC_ATTACH_WAIT->SRC_ATTACH_WAIT
[   58.031325] JDB: state change SRC_ATTACH_WAIT -> SNK_TRY [delayed 200 ms]
[   58.031525] JDB: tcpm_state_machine_work state chage: SNK_TRY->SNK_TRY
[   58.135273] JDB: state change SNK_TRY -> SNK_TRY_WAIT [delayed 100 ms]
[   58.135296] JDB: tcpm_state_machine_work state chage:
SNK_TRY_WAIT->SRC_TRYWAIT
[   58.149344] JDB: tcpm_state_machine_work state chage:
SRC_TRYWAIT->SRC_TRYWAIT
[   58.251273] JDB: state change SRC_TRYWAIT -> SRC_TRYWAIT_UNATTACHED
[delayed 100 ms]
[   58.251297] JDB: tcpm_state_machine_work state chage:
SRC_TRYWAIT_UNATTACHED->SNK_UNATTACHED
[   58.269076] JDB: tcpm_state_machine_work state chage:
SNK_UNATTACHED->TOGGLING
[   58.276789] JDB: tcpm_state_machine_work state chage: TOGGLING->TOGGLING
[   58.323506] JDB: tcpm_state_machine_work state chage:
SRC_ATTACH_WAIT->SRC_ATTACH_WAIT
[   58.527310] JDB: state change SRC_ATTACH_WAIT -> SRC_ATTACHED
[delayed 200 ms]
[   58.527788] JDB: hub_usb_role_switch_set switching to ROLE_HOST!
[   58.541555] usb usb1-port1: disabled by hub (EMI?), re-enabling...
[   58.548654] usb 1-1: USB disconnect, device number 2
[   58.554077] usb 1-1.5: USB disconnect, device number 3
[   58.560133] JDB: rt1711h_set_vbus  source: 1 sink: 0
[   58.565377] JDB: rt1711h_set_vbus enabling regulator!
[   58.570495] type-c-vbus-current-regulator:  being enabled! JDB!
[   58.586202] type-c-vbus-current-regulator:  enabled successfully?! JDB!
[   58.602350] rt1711h_set_vbus: vbus := On
[   58.602354] rt1711h_set_vbus: charge is already Off
[   58.747321] usb 2-1: USB disconnect, device number 2
[   58.819706] JDB: tcpm_state_machine_work state chage:
SRC_ATTACHED->SRC_ATTACHED
[   58.871270] usb 1-1: new high-speed USB device number 4 using xhci-hcd
[   59.030402] usb 1-1: New USB device found, idVendor=2109,
idProduct=2813, bcdDevice=90.11
[   59.038677] usb 1-1: New USB device strings: Mfr=1, Product=2, SerialNumber=0
[   59.045881] usb 1-1: Product: USB2.0 Hub
[   59.049838] usb 1-1: Manufacturer: VIA Labs, Inc.
[   59.104926] hub 1-1:1.0: USB hub found
[   59.109112] hub 1-1:1.0: 4 ports detected
[   59.327259] JDB: state change SRC_ATTACHED -> SRC_UNATTACHED [delayed 480 ms]
[   59.327710] JDB: rt1711h_set_vbus  source: 0 sink: 0
[   59.340022] JDB: rt1711h_set_vbus disabling regulator!
[   59.345296] type-c-vbus-current-regulator:  disabled successfully?!
JDB! (ret=0)
[   59.353458] rt1711h_set_vbus: vbus := Off
[   59.353465] rt1711h_set_vbus: charge is already Off
[   59.483278] usb 1-1.1: new low-speed USB device number 5 using xhci-hcd
[   59.571494] JDB: hub_usb_role_switch_set switching to ROLE_NONE!
[   59.577810] usb 1-1: USB disconnect, device number 4
[   59.586675] JDB: tcpm_state_machine_work state chage:
SRC_UNATTACHED->TOGGLING
[   59.593896] JDB: tcpm_state_machine_work state chage: TOGGLING->TOGGLING
[   59.600757] JDB: tcpm_state_machine_work state chage:
SRC_ATTACH_WAIT->SRC_ATTACH_WAIT
[   59.627362] usb 1-1.1: Device not responding to setup address.
[   59.661413] JDB: tcpm_state_machine_work state chage:
SRC_ATTACH_WAIT->SRC_ATTACH_WAIT
[   59.839438] usb 1-1.1: Device not responding to setup address.
[   59.863252] JDB: state change SRC_ATTACH_WAIT -> SNK_TRY [delayed 200 ms]
[   59.863428] JDB: tcpm_state_machine_work state chage: SNK_TRY->SNK_TRY
[   59.967359] JDB: state change SNK_TRY -> SNK_TRY_WAIT [delayed 100 ms]
[   59.967383] JDB: tcpm_state_machine_work state chage:
SNK_TRY_WAIT->SRC_TRYWAIT
[   59.981452] JDB: tcpm_state_machine_work state chage:
SRC_TRYWAIT->SRC_TRYWAIT
[   60.051272] usb 1-1.1: device not accepting address 5, error -71
[   60.083337] JDB: state change SRC_TRYWAIT -> SRC_TRYWAIT_UNATTACHED
[delayed 100 ms]
[   60.083365] JDB: tcpm_state_machine_work state chage:
SRC_TRYWAIT_UNATTACHED->SNK_UNATTACHED
[   60.101151] JDB: tcpm_state_machine_work state chage:
SNK_UNATTACHED->TOGGLING
[   60.108462] JDB: tcpm_state_machine_work state chage: TOGGLING->TOGGLING
[   60.155642] JDB: tcpm_state_machine_work state chage:
SRC_ATTACH_WAIT->SRC_ATTACH_WAIT
[   60.183338] usb 2-1: new SuperSpeed Gen 1 USB device number 3 using xhci-hcd
[   60.207782] usb 1-1-port1: attempt power cycle
[   60.212603] usb 2-1: New USB device found, idVendor=0424,
idProduct=5734, bcdDevice= 2.02
[   60.220923] usb 2-1: New USB device strings: Mfr=2, Product=3, SerialNumber=0
[   60.228147] usb 2-1: Product: USB5734
[   60.231883] usb 2-1: Manufacturer: Microchip Tech
[   60.256450] hub 2-1:1.0: USB hub found
[   60.260360] hub 2-1:1.0: 5 ports detected
[   60.359385] JDB: state change SRC_ATTACH_WAIT -> SRC_ATTACHED
[delayed 200 ms]
[   60.359853] JDB: hub_usb_role_switch_set switching to ROLE_HOST!
[   60.374310] JDB: rt1711h_set_vbus  source: 1 sink: 0
[   60.379386] JDB: rt1711h_set_vbus enabling regulator!
[   60.384485] type-c-vbus-current-regulator:  being enabled! JDB!
[   60.390552] hub 2-1:1.0: hub_ext_port_status failed (err = -71)
[   60.396544] type-c-vbus-current-regulator:  enabled successfully?! JDB!
[   60.403538] usb 2-1: Failed to suspend device, error -71
[   60.403694] usb 2-1: USB disconnect, device number 3
[   60.413841] rt1711h_set_vbus: vbus := On
[   60.413844] rt1711h_set_vbus: charge is already Off
[   60.631357] JDB: tcpm_state_machine_work state chage:
SRC_ATTACHED->SRC_ATTACHED
[   60.815285] usb 1-1: new high-speed USB device number 9 using xhci-hcd
[   60.969662] usb 1-1: New USB device found, idVendor=2109,
idProduct=2813, bcdDevice=90.11
[   60.977964] usb 1-1: New USB device strings: Mfr=1, Product=2, SerialNumber=0
[   60.985156] usb 1-1: Product: USB2.0 Hub
[   60.989419] usb 1-1: Manufacturer: VIA Labs, Inc.
[   61.056894] hub 1-1:1.0: USB hub found
[   61.061194] hub 1-1:1.0: 4 ports detected
[   61.119310] JDB: state change SRC_ATTACHED -> SRC_UNATTACHED [delayed 480 ms]
[   61.119759] JDB: rt1711h_set_vbus  source: 0 sink: 0
[   61.131951] JDB: rt1711h_set_vbus disabling regulator!
[   61.137141] type-c-vbus-current-regulator:  disabled successfully?!
JDB! (ret=0)
[   61.145007] rt1711h_set_vbus: vbus := Off
[   61.145010] rt1711h_set_vbus: charge is already Off
[   61.362956] JDB: hub_usb_role_switch_set switching to ROLE_NONE!
[   61.374082] usb 1-1: USB disconnect, device number 9
[   61.380600] JDB: tcpm_state_machine_work state chage:
SRC_UNATTACHED->TOGGLING
[   61.390394] JDB: tcpm_state_machine_work state chage: TOGGLING->TOGGLING
[   61.397257] JDB: tcpm_state_machine_work state chage:
SRC_ATTACH_WAIT->SRC_ATTACH_WAIT
[   61.419292] usb 1-1.1: new low-speed USB device number 10 using xhci-hcd
[   61.427378] usb 1-1-port1: attempt power cycle
[   61.452874] JDB: tcpm_state_machine_work state chage:
SRC_ATTACH_WAIT->SRC_ATTACH_WAIT
[   61.655250] JDB: state change SRC_ATTACH_WAIT -> SNK_TRY [delayed 200 ms]
[   61.655398] JDB: tcpm_state_machine_work state chage: SNK_TRY->SNK_TRY
[   61.723383] usb 2-1: new SuperSpeed Gen 1 USB device number 4 using xhci-hcd
[   61.748163] usb 2-1: New USB device found, idVendor=0424,
idProduct=5734, bcdDevice= 2.02
[   61.757846] usb 2-1: New USB device strings: Mfr=2, Product=3, SerialNumber=0
[   61.763291] JDB: state change SNK_TRY -> SNK_TRY_WAIT [delayed 100 ms]
[   61.763317] JDB: tcpm_state_machine_work state chage:
SNK_TRY_WAIT->SRC_TRYWAIT
[   61.766781] usb 2-1: Product: USB5734
[   61.782560] JDB: tcpm_state_machine_work state chage:
SRC_TRYWAIT->SRC_TRYWAIT
[   61.790221] usb 2-1: Manufacturer: Microchip Tech
[   61.824476] hub 2-1:1.0: USB hub found
[   61.828701] hub 2-1:1.0: 5 ports detected
[   61.883350] JDB: state change SRC_TRYWAIT -> SRC_TRYWAIT_UNATTACHED
[delayed 100 ms]
[   61.883378] JDB: tcpm_state_machine_work state chage:
SRC_TRYWAIT_UNATTACHED->SNK_UNATTACHED
[   61.901040] JDB: tcpm_state_machine_work state chage:
SNK_UNATTACHED->TOGGLING
[   61.909513] JDB: tcpm_state_machine_work state chage: TOGGLING->TOGGLING
[   61.955483] JDB: tcpm_state_machine_work state chage:
SRC_ATTACH_WAIT->SRC_ATTACH_WAIT
[   62.035296] usb 1-1: new high-speed USB device number 14 using xhci-hcd
[   62.159263] JDB: state change SRC_ATTACH_WAIT -> SRC_ATTACHED
[delayed 200 ms]
[   62.159750] JDB: hub_usb_role_switch_set switching to ROLE_HOST!
[   62.174502] JDB: rt1711h_set_vbus  source: 1 sink: 0
[   62.179534] JDB: rt1711h_set_vbus enabling regulator!
[   62.185067] type-c-vbus-current-regulator:  being enabled! JDB!
[   62.191039] type-c-vbus-current-regulator:  enabled successfully?! JDB!
[   62.198180] usb 1-1: device descriptor read/all, error -71
[   62.203769] rt1711h_set_vbus: vbus := On
[   62.203775] rt1711h_set_vbus: charge is already Off
[   62.351356] usb 2-1: USB disconnect, device number 4
[   62.421558] JDB: tcpm_state_machine_work state chage:
SRC_ATTACHED->SRC_ATTACHED
[   62.543282] usb 1-1: new high-speed USB device number 15 using xhci-hcd
[   62.696916] usb 1-1: New USB device found, idVendor=2109,
idProduct=2813, bcdDevice=90.11
[   62.705142] usb 1-1: New USB device strings: Mfr=1, Product=2, SerialNumber=0
[   62.712616] usb 1-1: Product: USB2.0 Hub
[   62.716595] usb 1-1: Manufacturer: VIA Labs, Inc.
[   62.784743] hub 1-1:1.0: USB hub found
[   62.788841] hub 1-1:1.0: 4 ports detected
[   62.911249] JDB: state change SRC_ATTACHED -> SRC_UNATTACHED [delayed 480 ms]
[   62.911598] JDB: rt1711h_set_vbus  source: 0 sink: 0
[   62.923769] JDB: rt1711h_set_vbus disabling regulator!
[   62.928940] type-c-vbus-current-regulator:  disabled successfully?!
JDB! (ret=0)
[   62.936711] rt1711h_set_vbus: vbus := Off
[   62.936714] rt1711h_set_vbus: charge is already Off
[   63.143272] usb 1-1.1: new low-speed USB device number 16 using xhci-hcd
[   63.154684] JDB: hub_usb_role_switch_set switching to ROLE_NONE!
[   63.160941] usb 1-1-port1: cannot reset (err = -71)
[   63.161185] usb 1-1: USB disconnect, device number 15
[   63.171398] JDB: tcpm_state_machine_work state chage:
SRC_UNATTACHED->TOGGLING
[   63.175823] usb 1-1-port1: attempt power cycle
[   63.181995] JDB: tcpm_state_machine_work state chage: TOGGLING->TOGGLING
[   63.182155] JDB: tcpm_state_machine_work state chage:
SRC_ATTACH_WAIT->SRC_ATTACH_WAIT
[   63.244450] JDB: tcpm_state_machine_work state chage:
SRC_ATTACH_WAIT->SRC_ATTACH_WAIT
[   63.447246] JDB: state change SRC_ATTACH_WAIT -> SNK_TRY [delayed 200 ms]
[   63.447391] JDB: tcpm_state_machine_work state chage: SNK_TRY->SNK_TRY
[   63.507335] usb 2-1: new SuperSpeed Gen 1 USB device number 5 using xhci-hcd
[   63.532130] usb 2-1: New USB device found, idVendor=0424,
idProduct=5734, bcdDevice= 2.02
[   63.542169] usb 2-1: New USB device strings: Mfr=2, Product=3, SerialNumber=0
[   63.549402] usb 2-1: Product: USB5734
[   63.551286] JDB: state change SNK_TRY -> SNK_TRY_WAIT [delayed 100 ms]
[   63.551313] JDB: tcpm_state_machine_work state chage:
SNK_TRY_WAIT->SRC_TRYWAIT
[   63.554868] usb 2-1: Manufacturer: Microchip Tech
[   63.571708] JDB: tcpm_state_machine_work state chage:
SRC_TRYWAIT->SRC_TRYWAIT
[   63.600310] hub 2-1:1.0: USB hub found
[   63.604194] hub 2-1:1.0: 5 ports detected
[   63.675303] JDB: state change SRC_TRYWAIT -> SRC_TRYWAIT_UNATTACHED
[delayed 100 ms]
[   63.675331] JDB: tcpm_state_machine_work state chage:
SRC_TRYWAIT_UNATTACHED->SNK_UNATTACHED
[   63.693027] JDB: tcpm_state_machine_work state chage:
SNK_UNATTACHED->TOGGLING
[   63.701676] JDB: tcpm_state_machine_work state chage: TOGGLING->TOGGLING
[   63.747517] JDB: tcpm_state_machine_work state chage:
SRC_ATTACH_WAIT->SRC_ATTACH_WAIT
[   63.827281] usb 1-1: new high-speed USB device number 20 using xhci-hcd
[   63.941498] JDB: tcpm_state_machine_work state chage:
SRC_UNATTACHED->TOGGLING
[   63.948872] JDB: tcpm_state_machine_work state chage: TOGGLING->TOGGLING
[   63.979728] usb 1-1: New USB device found, idVendor=0424,
idProduct=2734, bcdDevice= 2.02
[   63.988033] usb 1-1: New USB device strings: Mfr=1, Product=2, SerialNumber=0
[   63.995636] usb 1-1: Product: USB2734
[   63.999540] usb 1-1: Manufacturer: Microchip Tech
[   64.064532] hub 1-1:1.0: USB hub found
[   64.068557] hub 1-1:1.0: 5 ports detected
[   64.415290] usb 1-1.5: new high-speed USB device number 21 using xhci-hcd
[   64.520307] usb 1-1.5: New USB device found, idVendor=0424,
idProduct=2740, bcdDevice= 2.00
[   64.528969] usb 1-1.5: New USB device strings: Mfr=1, Product=2,
SerialNumber=0
[   64.536345] usb 1-1.5: Product: Hub Controller
[   64.540828] usb 1-1.5: Manufacturer: Microchip Tech

Hans de Goede Nov. 14, 2019, 10:11 a.m. UTC | #26

Hi John,

Sorry for being a bit slow to respond.

On 22-10-2019 07:58, John Stultz wrote:
> On Fri, Oct 18, 2019 at 2:05 PM Hans de Goede <hdegoede@redhat.com> wrote:
>> On 18-10-2019 22:37, John Stultz wrote:
>>> At least, with the current driver, the functionality is working as
>>> expected: remove the USB-C cable, and devices connected to the hub
>>> power on, plug something into the USB-C port and devices plugged into
>>> the hub shutdown.
>>>
>>> But maybe I'm missing what you mean?
>>
>> Ok, so double checking the schematic I do see separate Vbus-es for the
>> TypeC port and the TypeA ports, with the TypeC port one being controlled
>> by GPIO_202_VBUS_TYPEC. So ideally that gpio would be  controlled to
>> enable/disable vbus by the tcpm framework.
> 
> So I've given this a shot, adding a gpio regulator for the type-c
> vbus, and added a set_vbus hook to the tcpci_rt1711 with logic to
> enable and disable the regulator depending on the source state.  I've
> also added some debug logic to check the regulator disabling/enabling
> is working properly. However, doing the type-c vbus control via the
> tcpm logic doesn't seem to be working properly.
> 
> The issue seems to be when the USB-C cable is unplugged the device
> goes into ROLE_NONE, we switch to the on-board hub. Then when we
> connect a USB-C hub to the type-c port, we switch to ROLE_HOST, and
> power on the regulator, and that starts to power on the USB-C hub
> devices. However, since this disconnects/powers down the on-board hub,
> we see the on-board hub device disconnect. I'm guessing the hub
> disconnection causes some confusion in the state machine, as then I
> see the state change from state change SRC_ATTACHED -> SRC_UNATTACHED,
> and set_vbus is immediately called with source=0 and the regulator is
> disabled, and we switch back to ROLE_NONE (which powers on the onboard
> hub).  The system then seems to quickly oscillate between the
> ROLE_HOST and ROLE_NONE switching the regulator on and off fairly
> quickly (see log below for more details) and never really settling for
> one state or the other.
> 
> Any off-hand thoughts on what might be going wrong here?

Sorry no clue.

> I'm fine to
> continue digging and working on this approach, but I also don't want
> to have to pollute the core code too much for this oddball hardware
> (esp since doing the vbus control in the role-switch intermediary does
> work ok - or at least better then this approach so far).

Given the special nature of the hardware I'm fine with the OTG intermediary
approach here. IMHO it is fine to just stick with that and to not spend
too much time on this.

Regards,

Hans

> 
> thanks
> -john
> 
> 
> Starts in ROLE_NONE with nothing connected to type-c port, with the
> on-board hub powered on, then we connect a type-c usb hub.
> 
> [   57.828323] JDB: tcpm_state_machine_work state chage:
> SRC_ATTACH_WAIT->SRC_ATTACH_WAIT
> [   58.031325] JDB: state change SRC_ATTACH_WAIT -> SNK_TRY [delayed 200 ms]
> [   58.031525] JDB: tcpm_state_machine_work state chage: SNK_TRY->SNK_TRY
> [   58.135273] JDB: state change SNK_TRY -> SNK_TRY_WAIT [delayed 100 ms]
> [   58.135296] JDB: tcpm_state_machine_work state chage:
> SNK_TRY_WAIT->SRC_TRYWAIT
> [   58.149344] JDB: tcpm_state_machine_work state chage:
> SRC_TRYWAIT->SRC_TRYWAIT
> [   58.251273] JDB: state change SRC_TRYWAIT -> SRC_TRYWAIT_UNATTACHED
> [delayed 100 ms]
> [   58.251297] JDB: tcpm_state_machine_work state chage:
> SRC_TRYWAIT_UNATTACHED->SNK_UNATTACHED
> [   58.269076] JDB: tcpm_state_machine_work state chage:
> SNK_UNATTACHED->TOGGLING
> [   58.276789] JDB: tcpm_state_machine_work state chage: TOGGLING->TOGGLING
> [   58.323506] JDB: tcpm_state_machine_work state chage:
> SRC_ATTACH_WAIT->SRC_ATTACH_WAIT
> [   58.527310] JDB: state change SRC_ATTACH_WAIT -> SRC_ATTACHED
> [delayed 200 ms]
> [   58.527788] JDB: hub_usb_role_switch_set switching to ROLE_HOST!
> [   58.541555] usb usb1-port1: disabled by hub (EMI?), re-enabling...
> [   58.548654] usb 1-1: USB disconnect, device number 2
> [   58.554077] usb 1-1.5: USB disconnect, device number 3
> [   58.560133] JDB: rt1711h_set_vbus  source: 1 sink: 0
> [   58.565377] JDB: rt1711h_set_vbus enabling regulator!
> [   58.570495] type-c-vbus-current-regulator:  being enabled! JDB!
> [   58.586202] type-c-vbus-current-regulator:  enabled successfully?! JDB!
> [   58.602350] rt1711h_set_vbus: vbus := On
> [   58.602354] rt1711h_set_vbus: charge is already Off
> [   58.747321] usb 2-1: USB disconnect, device number 2
> [   58.819706] JDB: tcpm_state_machine_work state chage:
> SRC_ATTACHED->SRC_ATTACHED
> [   58.871270] usb 1-1: new high-speed USB device number 4 using xhci-hcd
> [   59.030402] usb 1-1: New USB device found, idVendor=2109,
> idProduct=2813, bcdDevice=90.11
> [   59.038677] usb 1-1: New USB device strings: Mfr=1, Product=2, SerialNumber=0
> [   59.045881] usb 1-1: Product: USB2.0 Hub
> [   59.049838] usb 1-1: Manufacturer: VIA Labs, Inc.
> [   59.104926] hub 1-1:1.0: USB hub found
> [   59.109112] hub 1-1:1.0: 4 ports detected
> [   59.327259] JDB: state change SRC_ATTACHED -> SRC_UNATTACHED [delayed 480 ms]
> [   59.327710] JDB: rt1711h_set_vbus  source: 0 sink: 0
> [   59.340022] JDB: rt1711h_set_vbus disabling regulator!
> [   59.345296] type-c-vbus-current-regulator:  disabled successfully?!
> JDB! (ret=0)
> [   59.353458] rt1711h_set_vbus: vbus := Off
> [   59.353465] rt1711h_set_vbus: charge is already Off
> [   59.483278] usb 1-1.1: new low-speed USB device number 5 using xhci-hcd
> [   59.571494] JDB: hub_usb_role_switch_set switching to ROLE_NONE!
> [   59.577810] usb 1-1: USB disconnect, device number 4
> [   59.586675] JDB: tcpm_state_machine_work state chage:
> SRC_UNATTACHED->TOGGLING
> [   59.593896] JDB: tcpm_state_machine_work state chage: TOGGLING->TOGGLING
> [   59.600757] JDB: tcpm_state_machine_work state chage:
> SRC_ATTACH_WAIT->SRC_ATTACH_WAIT
> [   59.627362] usb 1-1.1: Device not responding to setup address.
> [   59.661413] JDB: tcpm_state_machine_work state chage:
> SRC_ATTACH_WAIT->SRC_ATTACH_WAIT
> [   59.839438] usb 1-1.1: Device not responding to setup address.
> [   59.863252] JDB: state change SRC_ATTACH_WAIT -> SNK_TRY [delayed 200 ms]
> [   59.863428] JDB: tcpm_state_machine_work state chage: SNK_TRY->SNK_TRY
> [   59.967359] JDB: state change SNK_TRY -> SNK_TRY_WAIT [delayed 100 ms]
> [   59.967383] JDB: tcpm_state_machine_work state chage:
> SNK_TRY_WAIT->SRC_TRYWAIT
> [   59.981452] JDB: tcpm_state_machine_work state chage:
> SRC_TRYWAIT->SRC_TRYWAIT
> [   60.051272] usb 1-1.1: device not accepting address 5, error -71
> [   60.083337] JDB: state change SRC_TRYWAIT -> SRC_TRYWAIT_UNATTACHED
> [delayed 100 ms]
> [   60.083365] JDB: tcpm_state_machine_work state chage:
> SRC_TRYWAIT_UNATTACHED->SNK_UNATTACHED
> [   60.101151] JDB: tcpm_state_machine_work state chage:
> SNK_UNATTACHED->TOGGLING
> [   60.108462] JDB: tcpm_state_machine_work state chage: TOGGLING->TOGGLING
> [   60.155642] JDB: tcpm_state_machine_work state chage:
> SRC_ATTACH_WAIT->SRC_ATTACH_WAIT
> [   60.183338] usb 2-1: new SuperSpeed Gen 1 USB device number 3 using xhci-hcd
> [   60.207782] usb 1-1-port1: attempt power cycle
> [   60.212603] usb 2-1: New USB device found, idVendor=0424,
> idProduct=5734, bcdDevice= 2.02
> [   60.220923] usb 2-1: New USB device strings: Mfr=2, Product=3, SerialNumber=0
> [   60.228147] usb 2-1: Product: USB5734
> [   60.231883] usb 2-1: Manufacturer: Microchip Tech
> [   60.256450] hub 2-1:1.0: USB hub found
> [   60.260360] hub 2-1:1.0: 5 ports detected
> [   60.359385] JDB: state change SRC_ATTACH_WAIT -> SRC_ATTACHED
> [delayed 200 ms]
> [   60.359853] JDB: hub_usb_role_switch_set switching to ROLE_HOST!
> [   60.374310] JDB: rt1711h_set_vbus  source: 1 sink: 0
> [   60.379386] JDB: rt1711h_set_vbus enabling regulator!
> [   60.384485] type-c-vbus-current-regulator:  being enabled! JDB!
> [   60.390552] hub 2-1:1.0: hub_ext_port_status failed (err = -71)
> [   60.396544] type-c-vbus-current-regulator:  enabled successfully?! JDB!
> [   60.403538] usb 2-1: Failed to suspend device, error -71
> [   60.403694] usb 2-1: USB disconnect, device number 3
> [   60.413841] rt1711h_set_vbus: vbus := On
> [   60.413844] rt1711h_set_vbus: charge is already Off
> [   60.631357] JDB: tcpm_state_machine_work state chage:
> SRC_ATTACHED->SRC_ATTACHED
> [   60.815285] usb 1-1: new high-speed USB device number 9 using xhci-hcd
> [   60.969662] usb 1-1: New USB device found, idVendor=2109,
> idProduct=2813, bcdDevice=90.11
> [   60.977964] usb 1-1: New USB device strings: Mfr=1, Product=2, SerialNumber=0
> [   60.985156] usb 1-1: Product: USB2.0 Hub
> [   60.989419] usb 1-1: Manufacturer: VIA Labs, Inc.
> [   61.056894] hub 1-1:1.0: USB hub found
> [   61.061194] hub 1-1:1.0: 4 ports detected
> [   61.119310] JDB: state change SRC_ATTACHED -> SRC_UNATTACHED [delayed 480 ms]
> [   61.119759] JDB: rt1711h_set_vbus  source: 0 sink: 0
> [   61.131951] JDB: rt1711h_set_vbus disabling regulator!
> [   61.137141] type-c-vbus-current-regulator:  disabled successfully?!
> JDB! (ret=0)
> [   61.145007] rt1711h_set_vbus: vbus := Off
> [   61.145010] rt1711h_set_vbus: charge is already Off
> [   61.362956] JDB: hub_usb_role_switch_set switching to ROLE_NONE!
> [   61.374082] usb 1-1: USB disconnect, device number 9
> [   61.380600] JDB: tcpm_state_machine_work state chage:
> SRC_UNATTACHED->TOGGLING
> [   61.390394] JDB: tcpm_state_machine_work state chage: TOGGLING->TOGGLING
> [   61.397257] JDB: tcpm_state_machine_work state chage:
> SRC_ATTACH_WAIT->SRC_ATTACH_WAIT
> [   61.419292] usb 1-1.1: new low-speed USB device number 10 using xhci-hcd
> [   61.427378] usb 1-1-port1: attempt power cycle
> [   61.452874] JDB: tcpm_state_machine_work state chage:
> SRC_ATTACH_WAIT->SRC_ATTACH_WAIT
> [   61.655250] JDB: state change SRC_ATTACH_WAIT -> SNK_TRY [delayed 200 ms]
> [   61.655398] JDB: tcpm_state_machine_work state chage: SNK_TRY->SNK_TRY
> [   61.723383] usb 2-1: new SuperSpeed Gen 1 USB device number 4 using xhci-hcd
> [   61.748163] usb 2-1: New USB device found, idVendor=0424,
> idProduct=5734, bcdDevice= 2.02
> [   61.757846] usb 2-1: New USB device strings: Mfr=2, Product=3, SerialNumber=0
> [   61.763291] JDB: state change SNK_TRY -> SNK_TRY_WAIT [delayed 100 ms]
> [   61.763317] JDB: tcpm_state_machine_work state chage:
> SNK_TRY_WAIT->SRC_TRYWAIT
> [   61.766781] usb 2-1: Product: USB5734
> [   61.782560] JDB: tcpm_state_machine_work state chage:
> SRC_TRYWAIT->SRC_TRYWAIT
> [   61.790221] usb 2-1: Manufacturer: Microchip Tech
> [   61.824476] hub 2-1:1.0: USB hub found
> [   61.828701] hub 2-1:1.0: 5 ports detected
> [   61.883350] JDB: state change SRC_TRYWAIT -> SRC_TRYWAIT_UNATTACHED
> [delayed 100 ms]
> [   61.883378] JDB: tcpm_state_machine_work state chage:
> SRC_TRYWAIT_UNATTACHED->SNK_UNATTACHED
> [   61.901040] JDB: tcpm_state_machine_work state chage:
> SNK_UNATTACHED->TOGGLING
> [   61.909513] JDB: tcpm_state_machine_work state chage: TOGGLING->TOGGLING
> [   61.955483] JDB: tcpm_state_machine_work state chage:
> SRC_ATTACH_WAIT->SRC_ATTACH_WAIT
> [   62.035296] usb 1-1: new high-speed USB device number 14 using xhci-hcd
> [   62.159263] JDB: state change SRC_ATTACH_WAIT -> SRC_ATTACHED
> [delayed 200 ms]
> [   62.159750] JDB: hub_usb_role_switch_set switching to ROLE_HOST!
> [   62.174502] JDB: rt1711h_set_vbus  source: 1 sink: 0
> [   62.179534] JDB: rt1711h_set_vbus enabling regulator!
> [   62.185067] type-c-vbus-current-regulator:  being enabled! JDB!
> [   62.191039] type-c-vbus-current-regulator:  enabled successfully?! JDB!
> [   62.198180] usb 1-1: device descriptor read/all, error -71
> [   62.203769] rt1711h_set_vbus: vbus := On
> [   62.203775] rt1711h_set_vbus: charge is already Off
> [   62.351356] usb 2-1: USB disconnect, device number 4
> [   62.421558] JDB: tcpm_state_machine_work state chage:
> SRC_ATTACHED->SRC_ATTACHED
> [   62.543282] usb 1-1: new high-speed USB device number 15 using xhci-hcd
> [   62.696916] usb 1-1: New USB device found, idVendor=2109,
> idProduct=2813, bcdDevice=90.11
> [   62.705142] usb 1-1: New USB device strings: Mfr=1, Product=2, SerialNumber=0
> [   62.712616] usb 1-1: Product: USB2.0 Hub
> [   62.716595] usb 1-1: Manufacturer: VIA Labs, Inc.
> [   62.784743] hub 1-1:1.0: USB hub found
> [   62.788841] hub 1-1:1.0: 4 ports detected
> [   62.911249] JDB: state change SRC_ATTACHED -> SRC_UNATTACHED [delayed 480 ms]
> [   62.911598] JDB: rt1711h_set_vbus  source: 0 sink: 0
> [   62.923769] JDB: rt1711h_set_vbus disabling regulator!
> [   62.928940] type-c-vbus-current-regulator:  disabled successfully?!
> JDB! (ret=0)
> [   62.936711] rt1711h_set_vbus: vbus := Off
> [   62.936714] rt1711h_set_vbus: charge is already Off
> [   63.143272] usb 1-1.1: new low-speed USB device number 16 using xhci-hcd
> [   63.154684] JDB: hub_usb_role_switch_set switching to ROLE_NONE!
> [   63.160941] usb 1-1-port1: cannot reset (err = -71)
> [   63.161185] usb 1-1: USB disconnect, device number 15
> [   63.171398] JDB: tcpm_state_machine_work state chage:
> SRC_UNATTACHED->TOGGLING
> [   63.175823] usb 1-1-port1: attempt power cycle
> [   63.181995] JDB: tcpm_state_machine_work state chage: TOGGLING->TOGGLING
> [   63.182155] JDB: tcpm_state_machine_work state chage:
> SRC_ATTACH_WAIT->SRC_ATTACH_WAIT
> [   63.244450] JDB: tcpm_state_machine_work state chage:
> SRC_ATTACH_WAIT->SRC_ATTACH_WAIT
> [   63.447246] JDB: state change SRC_ATTACH_WAIT -> SNK_TRY [delayed 200 ms]
> [   63.447391] JDB: tcpm_state_machine_work state chage: SNK_TRY->SNK_TRY
> [   63.507335] usb 2-1: new SuperSpeed Gen 1 USB device number 5 using xhci-hcd
> [   63.532130] usb 2-1: New USB device found, idVendor=0424,
> idProduct=5734, bcdDevice= 2.02
> [   63.542169] usb 2-1: New USB device strings: Mfr=2, Product=3, SerialNumber=0
> [   63.549402] usb 2-1: Product: USB5734
> [   63.551286] JDB: state change SNK_TRY -> SNK_TRY_WAIT [delayed 100 ms]
> [   63.551313] JDB: tcpm_state_machine_work state chage:
> SNK_TRY_WAIT->SRC_TRYWAIT
> [   63.554868] usb 2-1: Manufacturer: Microchip Tech
> [   63.571708] JDB: tcpm_state_machine_work state chage:
> SRC_TRYWAIT->SRC_TRYWAIT
> [   63.600310] hub 2-1:1.0: USB hub found
> [   63.604194] hub 2-1:1.0: 5 ports detected
> [   63.675303] JDB: state change SRC_TRYWAIT -> SRC_TRYWAIT_UNATTACHED
> [delayed 100 ms]
> [   63.675331] JDB: tcpm_state_machine_work state chage:
> SRC_TRYWAIT_UNATTACHED->SNK_UNATTACHED
> [   63.693027] JDB: tcpm_state_machine_work state chage:
> SNK_UNATTACHED->TOGGLING
> [   63.701676] JDB: tcpm_state_machine_work state chage: TOGGLING->TOGGLING
> [   63.747517] JDB: tcpm_state_machine_work state chage:
> SRC_ATTACH_WAIT->SRC_ATTACH_WAIT
> [   63.827281] usb 1-1: new high-speed USB device number 20 using xhci-hcd
> [   63.941498] JDB: tcpm_state_machine_work state chage:
> SRC_UNATTACHED->TOGGLING
> [   63.948872] JDB: tcpm_state_machine_work state chage: TOGGLING->TOGGLING
> [   63.979728] usb 1-1: New USB device found, idVendor=0424,
> idProduct=2734, bcdDevice= 2.02
> [   63.988033] usb 1-1: New USB device strings: Mfr=1, Product=2, SerialNumber=0
> [   63.995636] usb 1-1: Product: USB2734
> [   63.999540] usb 1-1: Manufacturer: Microchip Tech
> [   64.064532] hub 1-1:1.0: USB hub found
> [   64.068557] hub 1-1:1.0: 5 ports detected
> [   64.415290] usb 1-1.5: new high-speed USB device number 21 using xhci-hcd
> [   64.520307] usb 1-1.5: New USB device found, idVendor=0424,
> idProduct=2740, bcdDevice= 2.00
> [   64.528969] usb 1-1.5: New USB device strings: Mfr=1, Product=2,
> SerialNumber=0
> [   64.536345] usb 1-1.5: Product: Hub Controller
> [   64.540828] usb 1-1.5: Manufacturer: Microchip Tech
>

John Stultz Nov. 15, 2019, 12:23 a.m. UTC | #27

On Thu, Nov 14, 2019 at 2:11 AM Hans de Goede <hdegoede@redhat.com> wrote:
> On 22-10-2019 07:58, John Stultz wrote:
> > I'm fine to
> > continue digging and working on this approach, but I also don't want
> > to have to pollute the core code too much for this oddball hardware
> > (esp since doing the vbus control in the role-switch intermediary does
> > work ok - or at least better then this approach so far).
>
> Given the special nature of the hardware I'm fine with the OTG intermediary
> approach here. IMHO it is fine to just stick with that and to not spend
> too much time on this.

Ok.  That was what I was leaning towards as well.
Thanks again for all the review and feedback here! I really appreciate it!
-john

[RFC,2/3] usb: roles: Add usb role switch notifier.

Commit Message

Comments

Patch