RE: [PATCH v2 2/2] usb: chipidea: add role switch class support

[Peter Chen <peter.chen@nxp.com>]

 
> > > USB role is fully controlled by usb role switch consumer(e.g.
> > > typec), usb port can be at host mode(USB_ROLE_HOST), device mode
> > > connected to host(USB_ROLE_DEVICE), or not connecting any
> parter(USB_ROLE_NONE).
> > >
> >
> > %s/parter/partner ?
> 
> Yes, I will update.
> 
> >
> > Are there any ways you could get external cable status from usb-switch
> > or type-c like external connector? If there are, you could update
> > otgsc value for otgsc_read, or change cable status, and avoid changing common
> handling, like ci_hdrc_probe and  ci_otg_work.
> > And it could benefit for other use cases, like booting with cable
> > connected and switch role through /sys.
> 
> The new role switch class has nothing to do with extcon, it's using graph bindings to
> link the connection, so there is no extcon_dev, change for ci_hdrc_probe() is
> required as property usb-role-switch has to be specified, there may be some code
> reuse if still touch the external cable even no extcon dev, but that will make existing
> external cable complicated and not clean.
> 

I don't mean using extcon directly.

At ci_usb_role_switch_set, you could know current "id" and "vbus" status.
as well as if "id" and "vbus" has changed. So, you could update ci->id_event
or ci->vbus_event. And at otgsc_read, you could return correct vbus and id value.

So, you may not need to change main function for ci_otg_work, for ci_hdrc_probe,
you could only need to register usb-switch class.

Surely, you may need to introduce some structures or variables for usb switch, but
it could keep main structure not changing, and just differentiate the way to get
id and vbus.


> On the other hand, a new GPIO based role switch driver is on review:
> https://patchwork.kernel.org/patch/11056379/
> Seems this is the direction to move out from extcon.
> 

If external connector is given up, we need to use that.

When you update the v3, please fix the build error reported by kbuild test robot.

Peter	

> Li Jun
> >
> > Peter
> >
> > > Signed-off-by: Li Jun <jun.li@nxp.com>
> > > ---
> > >
> > > Change for v2:
> > > - Support USB_ROLE_NONE, which for usb port not connecting any
> > >   device or host, and will be in low power mode.
> > >
> > >  drivers/usb/chipidea/ci.h   |   3 ++
> > >  drivers/usb/chipidea/core.c | 120
> > > +++++++++++++++++++++++++++++++++-------
> > > ----
> > >  drivers/usb/chipidea/otg.c  |  20 ++++++++
> > >  3 files changed, 114 insertions(+), 29 deletions(-)
> > >
> > > diff --git a/drivers/usb/chipidea/ci.h b/drivers/usb/chipidea/ci.h
> > > index 82b86cd..f0aec1d 100644
> > > --- a/drivers/usb/chipidea/ci.h
> > > +++ b/drivers/usb/chipidea/ci.h
> > > @@ -205,6 +205,7 @@ struct ci_hdrc {
> > >  	int				irq;
> > >  	struct ci_role_driver		*roles[USB_ROLE_DEVICE + 1];
> > >  	enum usb_role			role;
> > > +	enum usb_role			target_role;
> > >  	bool				is_otg;
> > >  	struct usb_otg			otg;
> > >  	struct otg_fsm			fsm;
> > > @@ -212,6 +213,7 @@ struct ci_hdrc {
> > >  	ktime_t
> > > 	hr_timeouts[NUM_OTG_FSM_TIMERS];
> > >  	unsigned			enabled_otg_timer_bits;
> > >  	enum otg_fsm_timer		next_otg_timer;
> > > +	struct usb_role_switch		*role_switch;
> > >  	struct work_struct		work;
> > >  	struct workqueue_struct		*wq;
> > >
> > > @@ -244,6 +246,7 @@ struct ci_hdrc {
> > >  	struct dentry			*debugfs;
> > >  	bool				id_event;
> > >  	bool				b_sess_valid_event;
> > > +	bool				role_switch_event;
> > >  	bool				imx28_write_fix;
> > >  	bool				supports_runtime_pm;
> > >  	bool				in_lpm;
> > > diff --git a/drivers/usb/chipidea/core.c
> > > b/drivers/usb/chipidea/core.c index
> > > bc24c5b..965ce17 100644
> > > --- a/drivers/usb/chipidea/core.c
> > > +++ b/drivers/usb/chipidea/core.c
> > > @@ -587,6 +587,42 @@ static irqreturn_t ci_irq(int irq, void *data)
> > >  	return ret;
> > >  }
> > >
> > > +static int ci_usb_role_switch_set(struct device *dev, enum usb_role
> > > +role) {
> > > +	struct ci_hdrc *ci = dev_get_drvdata(dev);
> > > +	unsigned long flags;
> > > +
> > > +	if (ci->role == role)
> > > +		return 0;
> > > +
> > > +	spin_lock_irqsave(&ci->lock, flags);
> > > +	ci->role_switch_event = true;
> > > +	ci->target_role = role;
> > > +	spin_unlock_irqrestore(&ci->lock, flags);
> > > +
> > > +	ci_otg_queue_work(ci);
> > > +
> > > +	return 0;
> > > +}
> > > +
> > > +static enum usb_role ci_usb_role_switch_get(struct device *dev) {
> > > +	struct ci_hdrc *ci = dev_get_drvdata(dev);
> > > +	unsigned long flags;
> > > +	enum usb_role role;
> > > +
> > > +	spin_lock_irqsave(&ci->lock, flags);
> > > +	role = ci->role;
> > > +	spin_unlock_irqrestore(&ci->lock, flags);
> > > +
> > > +	return role;
> > > +}
> > > +
> > > +static struct usb_role_switch_desc ci_role_switch = {
> > > +	.set = ci_usb_role_switch_set,
> > > +	.get = ci_usb_role_switch_get,
> > > +};
> > > +
> > >  static int ci_cable_notifier(struct notifier_block *nb, unsigned long event,
> > >  			     void *ptr)
> > >  {
> > > @@ -689,6 +725,9 @@ static int ci_get_platdata(struct device *dev,
> > >  	if (of_find_property(dev->of_node, "non-zero-ttctrl-ttha", NULL))
> > >  		platdata->flags |= CI_HDRC_SET_NON_ZERO_TTHA;
> > >
> > > +	if (device_property_read_bool(dev, "usb-role-switch"))
> > > +		ci_role_switch.fwnode = dev->fwnode;
> > > +
> > >  	ext_id = ERR_PTR(-ENODEV);
> > >  	ext_vbus = ERR_PTR(-ENODEV);
> > >  	if (of_property_read_bool(dev->of_node, "extcon")) { @@ -908,6
> > > +947,43 @@ static const struct attribute_group ci_attr_group = {
> > >  	.attrs = ci_attrs,
> > >  };
> > >
> > > +static int ci_start_initial_role(struct ci_hdrc *ci) {
> > > +	int ret = 0;
> > > +
> > > +	if (ci->roles[USB_ROLE_HOST] && ci->roles[USB_ROLE_DEVICE]) {
> > > +		if (ci->is_otg) {
> > > +			ci->role = ci_otg_role(ci);
> > > +			/* Enable ID change irq */
> > > +			hw_write_otgsc(ci, OTGSC_IDIE, OTGSC_IDIE);
> > > +		} else {
> > > +			/*
> > > +			 * If the controller is not OTG capable, but support
> > > +			 * role switch, the defalt role is gadget, and the
> > > +			 * user can switch it through debugfs.
> > > +			 */
> > > +			ci->role = USB_ROLE_DEVICE;
> > > +		}
> > > +	} else {
> > > +		ci->role = ci->roles[USB_ROLE_HOST]
> > > +			? USB_ROLE_HOST
> > > +			: USB_ROLE_DEVICE;
> > > +	}
> > > +
> > > +	if (!ci_otg_is_fsm_mode(ci)) {
> > > +		/* only update vbus status for peripheral */
> > > +		if (ci->role == USB_ROLE_DEVICE)
> > > +			ci_handle_vbus_change(ci);
> > > +
> > > +		ret = ci_role_start(ci, ci->role);
> > > +		if (ret)
> > > +			dev_err(ci->dev, "can't start %s role\n",
> > > +						ci_role(ci)->name);
> > > +	}
> > > +
> > > +	return ret;
> > > +}
> > > +
> > >  static int ci_hdrc_probe(struct platform_device *pdev)  {
> > >  	struct device	*dev = &pdev->dev;
> > > @@ -1051,36 +1127,10 @@ static int ci_hdrc_probe(struct platform_device
> *pdev)
> > >  		}
> > >  	}
> > >
> > > -	if (ci->roles[USB_ROLE_HOST] && ci->roles[USB_ROLE_DEVICE]) {
> > > -		if (ci->is_otg) {
> > > -			ci->role = ci_otg_role(ci);
> > > -			/* Enable ID change irq */
> > > -			hw_write_otgsc(ci, OTGSC_IDIE, OTGSC_IDIE);
> > > -		} else {
> > > -			/*
> > > -			 * If the controller is not OTG capable, but support
> > > -			 * role switch, the defalt role is gadget, and the
> > > -			 * user can switch it through debugfs.
> > > -			 */
> > > -			ci->role = USB_ROLE_DEVICE;
> > > -		}
> > > -	} else {
> > > -		ci->role = ci->roles[USB_ROLE_HOST]
> > > -			? USB_ROLE_HOST
> > > -			: USB_ROLE_DEVICE;
> > > -	}
> > > -
> > > -	if (!ci_otg_is_fsm_mode(ci)) {
> > > -		/* only update vbus status for peripheral */
> > > -		if (ci->role == USB_ROLE_DEVICE)
> > > -			ci_handle_vbus_change(ci);
> > > -
> > > -		ret = ci_role_start(ci, ci->role);
> > > -		if (ret) {
> > > -			dev_err(dev, "can't start %s role\n",
> > > -						ci_role(ci)->name);
> > > +	if (!ci_role_switch.fwnode) {
> > > +		ret = ci_start_initial_role(ci);
> > > +		if (ret)
> > >  			goto stop;
> > > -		}
> > >  	}
> > >
> > >  	ret = devm_request_irq(dev, ci->irq, ci_irq, IRQF_SHARED, @@
> > > -1092,6
> > > +1142,15 @@ static int ci_hdrc_probe(struct platform_device *pdev)
> > >  	if (ret)
> > >  		goto stop;
> > >
> > > +	if (ci_role_switch.fwnode) {
> > > +		ci->role_switch = usb_role_switch_register(dev,
> > > +					&ci_role_switch);
> > > +		if (IS_ERR(ci->role_switch)) {
> > > +			ret = PTR_ERR(ci->role_switch);
> > > +			goto stop;
> > > +		}
> > > +	}
> > > +
> > >  	if (ci->supports_runtime_pm) {
> > >  		pm_runtime_set_active(&pdev->dev);
> > >  		pm_runtime_enable(&pdev->dev);
> > > @@ -1133,6 +1192,9 @@ static int ci_hdrc_remove(struct
> > > platform_device
> > > *pdev) {
> > >  	struct ci_hdrc *ci = platform_get_drvdata(pdev);
> > >
> > > +	if (ci->role_switch)
> > > +		usb_role_switch_unregister(ci->role_switch);
> > > +
> > >  	if (ci->supports_runtime_pm) {
> > >  		pm_runtime_get_sync(&pdev->dev);
> > >  		pm_runtime_disable(&pdev->dev);
> > > diff --git a/drivers/usb/chipidea/otg.c b/drivers/usb/chipidea/otg.c
> > > index
> > > 5bde0b5..03675b6 100644
> > > --- a/drivers/usb/chipidea/otg.c
> > > +++ b/drivers/usb/chipidea/otg.c
> > > @@ -214,6 +214,26 @@ static void ci_otg_work(struct work_struct *work)
> > >  		ci_handle_vbus_change(ci);
> > >  	}
> > >
> > > +	if (ci->role_switch_event) {
> > > +		ci->role_switch_event = false;
> > > +
> > > +		/* Stop current role */
> > > +		if (ci->role == USB_ROLE_DEVICE) {
> > > +			usb_gadget_vbus_disconnect(&ci->gadget);
> > > +			ci->role = USB_ROLE_NONE;
> > > +		} else if (ci->role == USB_ROLE_HOST) {
> > > +			ci_role_stop(ci);
> > > +		}
> > > +
> > > +		/* Start target role */
> > > +		if (ci->target_role == USB_ROLE_DEVICE) {
> > > +			usb_gadget_vbus_connect(&ci->gadget);
> > > +			ci->role = USB_ROLE_DEVICE;
> > > +		} else if (ci->target_role == USB_ROLE_HOST) {
> > > +			ci_role_start(ci, USB_ROLE_HOST);
> > > +		}
> > > +	}
> > > +
> > >  	pm_runtime_put_sync(ci->dev);
> > >
> > >  	enable_irq(ci->irq);
> > > --
> > > 2.7.4