[PATCH v6] usb: common: add API to set usb otg capabilities by device tree

[PATCH v6] usb: common: add API to set usb otg capabilities by device tree

[Li Jun]

Change for v6:
UPdate otg caps by disable flags even if otg_rev is not passed, this is
needed in case user wants to disable whole OTG(or just want to ID pin
detect).

Check property of usb hardware to get otg version and if SRP, HNP and ADP
are disabled.

Signed-off-by: Li Jun <jun.li-KZfg59tc24xl57MIdRCFDg@public.gmane.org>
---
 drivers/usb/common/common.c | 27 +++++++++++++++++++++++++++
 include/linux/usb/of.h      |  7 +++++++
 2 files changed, 34 insertions(+)

diff --git a/drivers/usb/common/common.c b/drivers/usb/common/common.c
index b530fd4..12f5d28 100644
--- a/drivers/usb/common/common.c
+++ b/drivers/usb/common/common.c
@@ -154,6 +154,33 @@ bool of_usb_host_tpl_support(struct device_node *np)
 	return false;
 }
 EXPORT_SYMBOL_GPL(of_usb_host_tpl_support);
+
+/**
+ * of_usb_get_set_caps - to set usb otg capabilities according to
+ * the passed properties in DT.
+ * @np: Pointer to the given device_node
+ * @otg_caps: Pointer to the target usb_otg_caps to be set
+ *
+ * The function gets and sets the otg capabilities
+ */
+void of_usb_set_otg_caps(struct device_node *np, struct usb_otg_caps *otg_caps)
+{
+	u32 otg_rev;
+
+	if (!otg_caps)
+		return;
+
+	if (!of_property_read_u32(np, "otg-rev", &otg_rev))
+		otg_caps->otg_rev = otg_rev;
+	if (of_find_property(np, "hnp-disable", NULL))
+		otg_caps->hnp_support = false;
+	if (of_find_property(np, "srp-disable", NULL))
+		otg_caps->srp_support = false;
+	if (of_find_property(np, "adp-disable", NULL))
+		otg_caps->adp_support = false;
+}
+EXPORT_SYMBOL_GPL(of_usb_set_otg_caps);
+
 #endif
 
 MODULE_LICENSE("GPL");
diff --git a/include/linux/usb/of.h b/include/linux/usb/of.h
index cfe0528..6339799 100644
--- a/include/linux/usb/of.h
+++ b/include/linux/usb/of.h
@@ -15,6 +15,8 @@
 enum usb_dr_mode of_usb_get_dr_mode(struct device_node *np);
 enum usb_device_speed of_usb_get_maximum_speed(struct device_node *np);
 bool of_usb_host_tpl_support(struct device_node *np);
+void of_usb_set_otg_caps(struct device_node *np,
+			struct usb_otg_caps *otg_caps);
 #else
 static inline enum usb_dr_mode of_usb_get_dr_mode(struct device_node *np)
 {
@@ -30,6 +32,11 @@ static inline bool of_usb_host_tpl_support(struct device_node *np)
 {
 	return false;
 }
+static inline void of_usb_set_otg_caps(struct device_node *np,
+				struct usb_otg_caps *otg_caps)
+{
+
+}
 #endif
 
 #if IS_ENABLED(CONFIG_OF) && IS_ENABLED(CONFIG_USB_SUPPORT)
-- 
1.9.1

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v6] usb: common: add API to set usb otg capabilities by device tree

[Roger Quadros]

Lin,

You can use --in-reply-to "message id of v5 of this path" so that it appears together
with the other patches in peoples mailboxes.

On Thu, 18 Jun 2015 09:18:22 +0800
Li Jun <jun.li-KZfg59tc24xl57MIdRCFDg@public.gmane.org> wrote:

> Change for v6:
> UPdate otg caps by disable flags even if otg_rev is not passed, this is
> needed in case user wants to disable whole OTG(or just want to ID pin
> detect).
> 
> Check property of usb hardware to get otg version and if SRP, HNP and ADP
> are disabled.
> 
> Signed-off-by: Li Jun <jun.li-KZfg59tc24xl57MIdRCFDg@public.gmane.org>
> ---
>  drivers/usb/common/common.c | 27 +++++++++++++++++++++++++++
>  include/linux/usb/of.h      |  7 +++++++
>  2 files changed, 34 insertions(+)
> 
> diff --git a/drivers/usb/common/common.c b/drivers/usb/common/common.c
> index b530fd4..12f5d28 100644
> --- a/drivers/usb/common/common.c
> +++ b/drivers/usb/common/common.c
> @@ -154,6 +154,33 @@ bool of_usb_host_tpl_support(struct device_node *np)
>  	return false;
>  }
>  EXPORT_SYMBOL_GPL(of_usb_host_tpl_support);
> +
> +/**
> + * of_usb_get_set_caps - to set usb otg capabilities according to

get or set?
how about update instead?

> + * the passed properties in DT.
> + * @np: Pointer to the given device_node
> + * @otg_caps: Pointer to the target usb_otg_caps to be set
> + *
> + * The function gets and sets the otg capabilities
> + */
> +void of_usb_set_otg_caps(struct device_node *np, struct usb_otg_caps *otg_caps)
> +{
> +	u32 otg_rev;
> +
> +	if (!otg_caps)
> +		return;
> +
> +	if (!of_property_read_u32(np, "otg-rev", &otg_rev))
> +		otg_caps->otg_rev = otg_rev;

should we check if otg_rev is in correct format?
anything non BCD and greater than 0x9999 is invalid.

Also if otg-rev is not passed then we need to treat it as legacy
platform right? How is this taken care of?

> +	if (of_find_property(np, "hnp-disable", NULL))
> +		otg_caps->hnp_support = false;
> +	if (of_find_property(np, "srp-disable", NULL))
> +		otg_caps->srp_support = false;
> +	if (of_find_property(np, "adp-disable", NULL))
> +		otg_caps->adp_support = false;

Don't we expect it to do basic sanity checks?
e.g. if otg_rev < 0x200 then adp_support can't be true?

> +}
> +EXPORT_SYMBOL_GPL(of_usb_set_otg_caps);
> +
>  #endif
>  
>  MODULE_LICENSE("GPL");
> diff --git a/include/linux/usb/of.h b/include/linux/usb/of.h
> index cfe0528..6339799 100644
> --- a/include/linux/usb/of.h
> +++ b/include/linux/usb/of.h
> @@ -15,6 +15,8 @@
>  enum usb_dr_mode of_usb_get_dr_mode(struct device_node *np);
>  enum usb_device_speed of_usb_get_maximum_speed(struct device_node *np);
>  bool of_usb_host_tpl_support(struct device_node *np);
> +void of_usb_set_otg_caps(struct device_node *np,
> +			struct usb_otg_caps *otg_caps);
>  #else
>  static inline enum usb_dr_mode of_usb_get_dr_mode(struct device_node *np)
>  {
> @@ -30,6 +32,11 @@ static inline bool of_usb_host_tpl_support(struct device_node *np)
>  {
>  	return false;
>  }
> +static inline void of_usb_set_otg_caps(struct device_node *np,
> +				struct usb_otg_caps *otg_caps)
> +{
> +
> +}
>  #endif
>  
>  #if IS_ENABLED(CONFIG_OF) && IS_ENABLED(CONFIG_USB_SUPPORT)
> -- 
> 1.9.1
> 

cheers,
-roger
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v6] usb: common: add API to set usb otg capabilities by device tree

[Li Jun]

On Thu, Jun 18, 2015 at 10:36:50AM +0300, Roger Quadros wrote:
> Lin,
> 
> You can use --in-reply-to "message id of v5 of this path" so that it appears together
> with the other patches in peoples mailboxes.
> 
okay, I will try that next time, thanks.

Li Jun
> On Thu, 18 Jun 2015 09:18:22 +0800
> Li Jun <jun.li-KZfg59tc24xl57MIdRCFDg@public.gmane.org> wrote:
> 
> > Change for v6:
> > UPdate otg caps by disable flags even if otg_rev is not passed, this is
> > needed in case user wants to disable whole OTG(or just want to ID pin
> > detect).
> > 
> > Check property of usb hardware to get otg version and if SRP, HNP and ADP
> > are disabled.
> > 
> > Signed-off-by: Li Jun <jun.li-KZfg59tc24xl57MIdRCFDg@public.gmane.org>
> > ---
> >  drivers/usb/common/common.c | 27 +++++++++++++++++++++++++++
> >  include/linux/usb/of.h      |  7 +++++++
> >  2 files changed, 34 insertions(+)
> > 
> > diff --git a/drivers/usb/common/common.c b/drivers/usb/common/common.c
> > index b530fd4..12f5d28 100644
> > --- a/drivers/usb/common/common.c
> > +++ b/drivers/usb/common/common.c
> > @@ -154,6 +154,33 @@ bool of_usb_host_tpl_support(struct device_node *np)
> >  	return false;
> >  }
> >  EXPORT_SYMBOL_GPL(of_usb_host_tpl_support);
> > +
> > +/**
> > + * of_usb_get_set_caps - to set usb otg capabilities according to
> 
> get or set?
> how about update instead?
> 
Should be set, update? okay.

Li Jun
> > + * the passed properties in DT.
> > + * @np: Pointer to the given device_node
> > + * @otg_caps: Pointer to the target usb_otg_caps to be set
> > + *
> > + * The function gets and sets the otg capabilities
> > + */
> > +void of_usb_set_otg_caps(struct device_node *np, struct usb_otg_caps *otg_caps)
> > +{
> > +	u32 otg_rev;
> > +
> > +	if (!otg_caps)
> > +		return;
> > +
> > +	if (!of_property_read_u32(np, "otg-rev", &otg_rev))
> > +		otg_caps->otg_rev = otg_rev;
> 
> should we check if otg_rev is in correct format?
> anything non BCD and greater than 0x9999 is invalid.
> 
Then that's not so simple, every number should be checked,
if ((otg_caps->otg_rev % 10) > 9) ||
	((otg_caps->otg_rev >> 4) % 10) > 9) ||
	((otg_caps->otg_rev >> 8) % 10) > 9) ||
	((otg_caps->otg_rev >> 12) % 10) > 9))
	return error;
We really need this kind of format check? seems no this check
for bcdUSB.
	
> Also if otg-rev is not passed then we need to treat it as legacy
> platform right? How is this taken care of?
> 
> > +	if (of_find_property(np, "hnp-disable", NULL))
> > +		otg_caps->hnp_support = false;
> > +	if (of_find_property(np, "srp-disable", NULL))
> > +		otg_caps->srp_support = false;
> > +	if (of_find_property(np, "adp-disable", NULL))
> > +		otg_caps->adp_support = false;
> 
> Don't we expect it to do basic sanity checks?
> e.g. if otg_rev < 0x200 then adp_support can't be true?
> 
Okay, it's better we add a sanity check here.

> > +}
> > +EXPORT_SYMBOL_GPL(of_usb_set_otg_caps);
> > +
> >  #endif
> >  
> >  MODULE_LICENSE("GPL");
> > diff --git a/include/linux/usb/of.h b/include/linux/usb/of.h
> > index cfe0528..6339799 100644
> > --- a/include/linux/usb/of.h
> > +++ b/include/linux/usb/of.h
> > @@ -15,6 +15,8 @@
> >  enum usb_dr_mode of_usb_get_dr_mode(struct device_node *np);
> >  enum usb_device_speed of_usb_get_maximum_speed(struct device_node *np);
> >  bool of_usb_host_tpl_support(struct device_node *np);
> > +void of_usb_set_otg_caps(struct device_node *np,
> > +			struct usb_otg_caps *otg_caps);
> >  #else
> >  static inline enum usb_dr_mode of_usb_get_dr_mode(struct device_node *np)
> >  {
> > @@ -30,6 +32,11 @@ static inline bool of_usb_host_tpl_support(struct device_node *np)
> >  {
> >  	return false;
> >  }
> > +static inline void of_usb_set_otg_caps(struct device_node *np,
> > +				struct usb_otg_caps *otg_caps)
> > +{
> > +
> > +}
> >  #endif
> >  
> >  #if IS_ENABLED(CONFIG_OF) && IS_ENABLED(CONFIG_USB_SUPPORT)
> > -- 
> > 1.9.1
> > 
> 
> cheers,
> -roger
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v6] usb: common: add API to set usb otg capabilities by device tree

[Roger Quadros]

On Thu, 18 Jun 2015 15:55:28 +0800
Li Jun <b47624-KZfg59tc24xl57MIdRCFDg@public.gmane.org> wrote:

> On Thu, Jun 18, 2015 at 10:36:50AM +0300, Roger Quadros wrote:
> > Lin,
> > 
> > You can use --in-reply-to "message id of v5 of this path" so that it appears together
> > with the other patches in peoples mailboxes.
> > 
> okay, I will try that next time, thanks.
> 
> Li Jun
> > On Thu, 18 Jun 2015 09:18:22 +0800
> > Li Jun <jun.li-KZfg59tc24xl57MIdRCFDg@public.gmane.org> wrote:
> > 
> > > Change for v6:
> > > UPdate otg caps by disable flags even if otg_rev is not passed, this is
> > > needed in case user wants to disable whole OTG(or just want to ID pin
> > > detect).
> > > 
> > > Check property of usb hardware to get otg version and if SRP, HNP and ADP
> > > are disabled.
> > > 
> > > Signed-off-by: Li Jun <jun.li-KZfg59tc24xl57MIdRCFDg@public.gmane.org>
> > > ---
> > >  drivers/usb/common/common.c | 27 +++++++++++++++++++++++++++
> > >  include/linux/usb/of.h      |  7 +++++++
> > >  2 files changed, 34 insertions(+)
> > > 
> > > diff --git a/drivers/usb/common/common.c b/drivers/usb/common/common.c
> > > index b530fd4..12f5d28 100644
> > > --- a/drivers/usb/common/common.c
> > > +++ b/drivers/usb/common/common.c
> > > @@ -154,6 +154,33 @@ bool of_usb_host_tpl_support(struct device_node *np)
> > >  	return false;
> > >  }
> > >  EXPORT_SYMBOL_GPL(of_usb_host_tpl_support);
> > > +
> > > +/**
> > > + * of_usb_get_set_caps - to set usb otg capabilities according to
> > 
> > get or set?
> > how about update instead?
> > 
> Should be set, update? okay.
> 
> Li Jun
> > > + * the passed properties in DT.
> > > + * @np: Pointer to the given device_node
> > > + * @otg_caps: Pointer to the target usb_otg_caps to be set
> > > + *
> > > + * The function gets and sets the otg capabilities
> > > + */
> > > +void of_usb_set_otg_caps(struct device_node *np, struct usb_otg_caps *otg_caps)
> > > +{
> > > +	u32 otg_rev;
> > > +
> > > +	if (!otg_caps)
> > > +		return;
> > > +
> > > +	if (!of_property_read_u32(np, "otg-rev", &otg_rev))
> > > +		otg_caps->otg_rev = otg_rev;
> > 
> > should we check if otg_rev is in correct format?
> > anything non BCD and greater than 0x9999 is invalid.
> > 
> Then that's not so simple, every number should be checked,
> if ((otg_caps->otg_rev % 10) > 9) ||
> 	((otg_caps->otg_rev >> 4) % 10) > 9) ||
> 	((otg_caps->otg_rev >> 8) % 10) > 9) ||
> 	((otg_caps->otg_rev >> 12) % 10) > 9))
> 	return error;
> We really need this kind of format check? seems no this check
> for bcdUSB.

How about checking for released spec versions instead?

	switch (otg_rev) {
	case 0x0100:
	case 0x0110:
	case 0x0200:
	case 0x0300:
		break;
	default:
		dev_err(dev, "unsupported otg-rev: 0x%x\n", otg_rev);
		return error;
	}

cheers,
-roger

> 	
> > Also if otg-rev is not passed then we need to treat it as legacy
> > platform right? How is this taken care of?
> > 
> > > +	if (of_find_property(np, "hnp-disable", NULL))
> > > +		otg_caps->hnp_support = false;
> > > +	if (of_find_property(np, "srp-disable", NULL))
> > > +		otg_caps->srp_support = false;
> > > +	if (of_find_property(np, "adp-disable", NULL))
> > > +		otg_caps->adp_support = false;
> > 
> > Don't we expect it to do basic sanity checks?
> > e.g. if otg_rev < 0x200 then adp_support can't be true?
> > 
> Okay, it's better we add a sanity check here.
> 
> > > +}
> > > +EXPORT_SYMBOL_GPL(of_usb_set_otg_caps);
> > > +
> > >  #endif
> > >  
> > >  MODULE_LICENSE("GPL");
> > > diff --git a/include/linux/usb/of.h b/include/linux/usb/of.h
> > > index cfe0528..6339799 100644
> > > --- a/include/linux/usb/of.h
> > > +++ b/include/linux/usb/of.h
> > > @@ -15,6 +15,8 @@
> > >  enum usb_dr_mode of_usb_get_dr_mode(struct device_node *np);
> > >  enum usb_device_speed of_usb_get_maximum_speed(struct device_node *np);
> > >  bool of_usb_host_tpl_support(struct device_node *np);
> > > +void of_usb_set_otg_caps(struct device_node *np,
> > > +			struct usb_otg_caps *otg_caps);
> > >  #else
> > >  static inline enum usb_dr_mode of_usb_get_dr_mode(struct device_node *np)
> > >  {
> > > @@ -30,6 +32,11 @@ static inline bool of_usb_host_tpl_support(struct device_node *np)
> > >  {
> > >  	return false;
> > >  }
> > > +static inline void of_usb_set_otg_caps(struct device_node *np,
> > > +				struct usb_otg_caps *otg_caps)
> > > +{
> > > +
> > > +}
> > >  #endif
> > >  
> > >  #if IS_ENABLED(CONFIG_OF) && IS_ENABLED(CONFIG_USB_SUPPORT)
> > > -- 
> > > 1.9.1
> > > 
> > 
> > cheers,
> > -roger
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v6] usb: common: add API to set usb otg capabilities by device tree

[Li Jun]

On Thu, Jun 18, 2015 at 03:18:24PM +0300, Roger Quadros wrote:
> On Thu, 18 Jun 2015 15:55:28 +0800
> Li Jun <b47624-KZfg59tc24xl57MIdRCFDg@public.gmane.org> wrote:
> 
> > On Thu, Jun 18, 2015 at 10:36:50AM +0300, Roger Quadros wrote:
> > > Lin,
> > > 
> > > You can use --in-reply-to "message id of v5 of this path" so that it appears together
> > > with the other patches in peoples mailboxes.
> > > 
> > okay, I will try that next time, thanks.
> > 
> > Li Jun
> > > On Thu, 18 Jun 2015 09:18:22 +0800
> > > Li Jun <jun.li-KZfg59tc24xl57MIdRCFDg@public.gmane.org> wrote:
> > > 
> > > 
> > Then that's not so simple, every number should be checked,
> > if ((otg_caps->otg_rev % 10) > 9) ||
> > 	((otg_caps->otg_rev >> 4) % 10) > 9) ||
> > 	((otg_caps->otg_rev >> 8) % 10) > 9) ||
> > 	((otg_caps->otg_rev >> 12) % 10) > 9))
> > 	return error;
> > We really need this kind of format check? seems no this check
> > for bcdUSB.
> 
> How about checking for released spec versions instead?
> 
> 	switch (otg_rev) {
> 	case 0x0100:
> 	case 0x0110:
> 	case 0x0200:
> 	case 0x0300:
> 		break;
> 	default:
> 		dev_err(dev, "unsupported otg-rev: 0x%x\n", otg_rev);
> 		return error;
> 	}
> 
Currently only 1.x and 2.0 can make sense, but this could be done.

> cheers,
> -roger
> 
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v6] usb: common: add API to set usb otg capabilities by device tree

[Li Jun]

On Thu, Jun 18, 2015 at 10:36:50AM +0300, Roger Quadros wrote:
> Lin,
> 
> You can use --in-reply-to "message id of v5 of this path" so that it appears together
> with the other patches in peoples mailboxes.
> 
> > + * the passed properties in DT.
> > + * @np: Pointer to the given device_node
> > + * @otg_caps: Pointer to the target usb_otg_caps to be set
> > + *
> > + * The function gets and sets the otg capabilities
> > + */
> > +void of_usb_set_otg_caps(struct device_node *np, struct usb_otg_caps *otg_caps)
> > +{
> > +	u32 otg_rev;
> > +
> > +	if (!otg_caps)
> > +		return;
> > +
> > +	if (!of_property_read_u32(np, "otg-rev", &otg_rev))
> > +		otg_caps->otg_rev = otg_rev;
> 
> should we check if otg_rev is in correct format?
> anything non BCD and greater than 0x9999 is invalid.
> 
> Also if otg-rev is not passed then we need to treat it as legacy
> platform right? How is this taken care of?
> 
Missed this comment
This handling rely on controller driver, cannot decided here.
There are several cases we need take care:
1) otg-rev is not passed, but all 3 disable flags passed, this is
  valid, means user want to disable whole OTG, so only "otg-rev"
  not passed, cannot treat as legacy platform.
2) Legacy platform means: none of 4 properties is present.
3) Some controller drivers already support OTG HNP/SRP, then change
  to utilize those new flags, still should support OTG HNP/SRP w/o
  any dt change, so OTG caps should be enabled for legacy platforms.
4) Some controller drivers does not support any OTG, after add OTG
  functions and utilize those new flags, should keep OTG disabled
  for legacy platforms.

Li Jun 
> > -- 
> > 1.9.1
> > 
> 
> cheers,
> -roger
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v6] usb: common: add API to set usb otg capabilities by device tree

[Roger Quadros]

On Thu, 18 Jun 2015 16:47:48 +0800
Li Jun <b47624-KZfg59tc24xl57MIdRCFDg@public.gmane.org> wrote:

> On Thu, Jun 18, 2015 at 10:36:50AM +0300, Roger Quadros wrote:
> > Lin,
> > 
> > You can use --in-reply-to "message id of v5 of this path" so that it appears together
> > with the other patches in peoples mailboxes.
> > 
> > > + * the passed properties in DT.
> > > + * @np: Pointer to the given device_node
> > > + * @otg_caps: Pointer to the target usb_otg_caps to be set
> > > + *
> > > + * The function gets and sets the otg capabilities
> > > + */
> > > +void of_usb_set_otg_caps(struct device_node *np, struct usb_otg_caps *otg_caps)
> > > +{
> > > +	u32 otg_rev;
> > > +
> > > +	if (!otg_caps)
> > > +		return;
> > > +
> > > +	if (!of_property_read_u32(np, "otg-rev", &otg_rev))
> > > +		otg_caps->otg_rev = otg_rev;
> > 
> > should we check if otg_rev is in correct format?
> > anything non BCD and greater than 0x9999 is invalid.
> > 
> > Also if otg-rev is not passed then we need to treat it as legacy
> > platform right? How is this taken care of?
> > 
> Missed this comment
> This handling rely on controller driver, cannot decided here.
> There are several cases we need take care:
> 1) otg-rev is not passed, but all 3 disable flags passed, this is
>   valid, means user want to disable whole OTG, so only "otg-rev"
>   not passed, cannot treat as legacy platform.
> 2) Legacy platform means: none of 4 properties is present.

Right. Plus controller drivers that are not updated to use these otg_caps
are also legacy.

> 3) Some controller drivers already support OTG HNP/SRP, then change
>   to utilize those new flags, still should support OTG HNP/SRP w/o
>   any dt change, so OTG caps should be enabled for legacy platforms.

I didn't understand this point. If a controller driver is not updated
to use otg_caps it is legacy and gadget->otg_caps will be NULL.

> 4) Some controller drivers does not support any OTG, after add OTG
>   functions and utilize those new flags, should keep OTG disabled
>   for legacy platforms.

If controller drivers don't support OTG. gadget->is_otg and
gadget->otg_caps will not be set by them and they don't have to use
of_usb_set_otg_caps().

So I didn't understand why the decision can't be taken here
for non-legacy controller drivers with legacy DT.
They will set gadget->otg_caps and gadget->is_otg.

If none of the 4 DT flags are passed then we know it is legacy DT
and we can limit to legacy behaviour.

cheers,
-roger
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v6] usb: common: add API to set usb otg capabilities by device tree

[Li Jun]

On Thu, Jun 18, 2015 at 03:07:48PM +0300, Roger Quadros wrote:
> On Thu, 18 Jun 2015 16:47:48 +0800
> Li Jun <b47624-KZfg59tc24xl57MIdRCFDg@public.gmane.org> wrote:
> 
> > On Thu, Jun 18, 2015 at 10:36:50AM +0300, Roger Quadros wrote:
> > > Lin,
> > > 
> > > You can use --in-reply-to "message id of v5 of this path" so that it appears together
> > > with the other patches in peoples mailboxes.
> > > 
> > > > + * the passed properties in DT.
> > > > + * @np: Pointer to the given device_node
> > > > + * @otg_caps: Pointer to the target usb_otg_caps to be set
> > > > + *
> > > > + * The function gets and sets the otg capabilities
> > > > + */
> > > > +void of_usb_set_otg_caps(struct device_node *np, struct usb_otg_caps *otg_caps)
> > > > +{
> > > > +	u32 otg_rev;
> > > > +
> > > > +	if (!otg_caps)
> > > > +		return;
> > > > +
> > > > +	if (!of_property_read_u32(np, "otg-rev", &otg_rev))
> > > > +		otg_caps->otg_rev = otg_rev;
> > > 
> > > should we check if otg_rev is in correct format?
> > > anything non BCD and greater than 0x9999 is invalid.
> > > 
> > > Also if otg-rev is not passed then we need to treat it as legacy
> > > platform right? How is this taken care of?
> > > 
> > Missed this comment
> > This handling rely on controller driver, cannot decided here.
> > There are several cases we need take care:
> > 1) otg-rev is not passed, but all 3 disable flags passed, this is
> >   valid, means user want to disable whole OTG, so only "otg-rev"
> >   not passed, cannot treat as legacy platform.
> > 2) Legacy platform means: none of 4 properties is present.
> 
> Right. Plus controller drivers that are not updated to use these otg_caps
> are also legacy.
>
Did not understand the "Plus" case.
Some of 4 properties is present, but its driver dose not use otg_caps?

> > 3) Some controller drivers already support OTG HNP/SRP, then change
> >   to utilize those new flags, still should support OTG HNP/SRP w/o
> >   any dt change, so OTG caps should be enabled for legacy platforms.
> 
> I didn't understand this point. If a controller driver is not updated
> to use otg_caps it is legacy and gadget->otg_caps will be NULL.
> 
Some of existing chipdea platforms work fine on HNP and SRP , which did
not use any new flags and properties, after my patch, those platform should
still enable HNP and SRP without any DT change.

> > 4) Some controller drivers does not support any OTG, after add OTG
> >   functions and utilize those new flags, should keep OTG disabled
> >   for legacy platforms.
> 
> If controller drivers don't support OTG. gadget->is_otg and
> gadget->otg_caps will not be set by them and they don't have to use
> of_usb_set_otg_caps().
> 
But after my patch, some time later, this driver adds OTG functions on
it new platform, it should disable any OTG feature for its legacy
platforms (none of properties is passed).
 
> So I didn't understand why the decision can't be taken here
> for non-legacy controller drivers with legacy DT.
> They will set gadget->otg_caps and gadget->is_otg.
> 
> If none of the 4 DT flags are passed then we know it is legacy DT
> and we can limit to legacy behaviour.
> 
legacy behaviour number is 2, not only one legacy behaviour.
That's why I leave the otg caps decided by controller drivers.

> cheers,
> -roger
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v6] usb: common: add API to set usb otg capabilities by device tree

[Roger Quadros]

On Thu, 18 Jun 2015 21:37:04 +0800
Li Jun <b47624-KZfg59tc24xl57MIdRCFDg@public.gmane.org> wrote:

> On Thu, Jun 18, 2015 at 03:07:48PM +0300, Roger Quadros wrote:
> > On Thu, 18 Jun 2015 16:47:48 +0800
> > Li Jun <b47624-KZfg59tc24xl57MIdRCFDg@public.gmane.org> wrote:
> > 
> > > On Thu, Jun 18, 2015 at 10:36:50AM +0300, Roger Quadros wrote:
> > > > Lin,
> > > > 
> > > > You can use --in-reply-to "message id of v5 of this path" so that it appears together
> > > > with the other patches in peoples mailboxes.
> > > > 
> > > > > + * the passed properties in DT.
> > > > > + * @np: Pointer to the given device_node
> > > > > + * @otg_caps: Pointer to the target usb_otg_caps to be set
> > > > > + *
> > > > > + * The function gets and sets the otg capabilities
> > > > > + */
> > > > > +void of_usb_set_otg_caps(struct device_node *np, struct usb_otg_caps *otg_caps)
> > > > > +{
> > > > > +	u32 otg_rev;
> > > > > +
> > > > > +	if (!otg_caps)
> > > > > +		return;
> > > > > +
> > > > > +	if (!of_property_read_u32(np, "otg-rev", &otg_rev))
> > > > > +		otg_caps->otg_rev = otg_rev;
> > > > 
> > > > should we check if otg_rev is in correct format?
> > > > anything non BCD and greater than 0x9999 is invalid.
> > > > 
> > > > Also if otg-rev is not passed then we need to treat it as legacy
> > > > platform right? How is this taken care of?
> > > > 
> > > Missed this comment
> > > This handling rely on controller driver, cannot decided here.
> > > There are several cases we need take care:
> > > 1) otg-rev is not passed, but all 3 disable flags passed, this is
> > >   valid, means user want to disable whole OTG, so only "otg-rev"
> > >   not passed, cannot treat as legacy platform.
> > > 2) Legacy platform means: none of 4 properties is present.
> > 
> > Right. Plus controller drivers that are not updated to use these otg_caps
> > are also legacy.
> >
> Did not understand the "Plus" case.
> Some of 4 properties is present, but its driver dose not use otg_caps?

yes that is what I meant.

> 
> > > 3) Some controller drivers already support OTG HNP/SRP, then change
> > >   to utilize those new flags, still should support OTG HNP/SRP w/o
> > >   any dt change, so OTG caps should be enabled for legacy platforms.
> > 
> > I didn't understand this point. If a controller driver is not updated
> > to use otg_caps it is legacy and gadget->otg_caps will be NULL.
> > 
> Some of existing chipdea platforms work fine on HNP and SRP , which did
> not use any new flags and properties, after my patch, those platform should
> still enable HNP and SRP without any DT change.

Agreed.
> 
> > > 4) Some controller drivers does not support any OTG, after add OTG
> > >   functions and utilize those new flags, should keep OTG disabled
> > >   for legacy platforms.
> > 
> > If controller drivers don't support OTG. gadget->is_otg and
> > gadget->otg_caps will not be set by them and they don't have to use
> > of_usb_set_otg_caps().
> > 
> But after my patch, some time later, this driver adds OTG functions on
> it new platform, it should disable any OTG feature for its legacy
> platforms (none of properties is passed).

That can be decided in of_usb_update_otg_caps().
Controller sets whatever it supports in otg_caps.
of_usb_updade_otg_caps() checks if it is legacy DT (i.e. no otg-rev) and then
overrides to legacy otg_caps.

>  
> > So I didn't understand why the decision can't be taken here
> > for non-legacy controller drivers with legacy DT.
> > They will set gadget->otg_caps and gadget->is_otg.
> > 
> > If none of the 4 DT flags are passed then we know it is legacy DT
> > and we can limit to legacy behaviour.
> > 
> legacy behaviour number is 2, not only one legacy behaviour.
> That's why I leave the otg caps decided by controller drivers.

I'm only suggesting that it be decided at one place i.e. in 
of_usb_updade_otg_caps() instead of every controller.

Do you see any problems with that approach?

cheers,
-roger
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in

Re: [PATCH v6] usb: common: add API to set usb otg capabilities by device tree

[Li Jun]

On Mon, Jun 22, 2015 at 12:41:22PM +0300, Roger Quadros wrote:
> On Thu, 18 Jun 2015 21:37:04 +0800
> Li Jun <b47624-KZfg59tc24xl57MIdRCFDg@public.gmane.org> wrote:
> 
> > On Thu, Jun 18, 2015 at 03:07:48PM +0300, Roger Quadros wrote:
> > > On Thu, 18 Jun 2015 16:47:48 +0800
> > > Li Jun <b47624-KZfg59tc24xl57MIdRCFDg@public.gmane.org> wrote:
> > > 
> > > > On Thu, Jun 18, 2015 at 10:36:50AM +0300, Roger Quadros wrote:
> > > > > Lin,
> > > > > 
> > > > > You can use --in-reply-to "message id of v5 of this path" so that it appears together
> > > > > with the other patches in peoples mailboxes.
> > > > > 
> > > > > > + * the passed properties in DT.
> > > > > > + * @np: Pointer to the given device_node
> > > > > > + * @otg_caps: Pointer to the target usb_otg_caps to be set
> > > > > > + *
> > > > > > + * The function gets and sets the otg capabilities
> > > > > > + */
> > > > > > +void of_usb_set_otg_caps(struct device_node *np, struct usb_otg_caps *otg_caps)
> > > > > > +{
> > > > > > +	u32 otg_rev;
> > > > > > +
> > > > > > +	if (!otg_caps)
> > > > > > +		return;
> > > > > > +
> > > > > > +	if (!of_property_read_u32(np, "otg-rev", &otg_rev))
> > > > > > +		otg_caps->otg_rev = otg_rev;
> > > > > 
> > > > > should we check if otg_rev is in correct format?
> > > > > anything non BCD and greater than 0x9999 is invalid.
> > > > > 
> > > > > Also if otg-rev is not passed then we need to treat it as legacy
> > > > > platform right? How is this taken care of?
> > > > > 
> > > > Missed this comment
> > > > This handling rely on controller driver, cannot decided here.
> > > > There are several cases we need take care:
> > > > 1) otg-rev is not passed, but all 3 disable flags passed, this is
> > > >   valid, means user want to disable whole OTG, so only "otg-rev"
> > > >   not passed, cannot treat as legacy platform.
> > > > 2) Legacy platform means: none of 4 properties is present.
> > > 
> > > Right. Plus controller drivers that are not updated to use these otg_caps
> > > are also legacy.
> > >
> > Did not understand the "Plus" case.
> > Some of 4 properties is present, but its driver dose not use otg_caps?
> 
> yes that is what I meant.
> 
> > 
> > > > 3) Some controller drivers already support OTG HNP/SRP, then change
> > > >   to utilize those new flags, still should support OTG HNP/SRP w/o
> > > >   any dt change, so OTG caps should be enabled for legacy platforms.
> > > 
> > > I didn't understand this point. If a controller driver is not updated
> > > to use otg_caps it is legacy and gadget->otg_caps will be NULL.
> > > 
> > Some of existing chipdea platforms work fine on HNP and SRP , which did
> > not use any new flags and properties, after my patch, those platform should
> > still enable HNP and SRP without any DT change.
> 
> Agreed.
> > 
> > > > 4) Some controller drivers does not support any OTG, after add OTG
> > > >   functions and utilize those new flags, should keep OTG disabled
> > > >   for legacy platforms.
> > > 
> > > If controller drivers don't support OTG. gadget->is_otg and
> > > gadget->otg_caps will not be set by them and they don't have to use
> > > of_usb_set_otg_caps().
> > > 
> > But after my patch, some time later, this driver adds OTG functions on
> > it new platform, it should disable any OTG feature for its legacy
> > platforms (none of properties is passed).
> 
> That can be decided in of_usb_update_otg_caps().
> Controller sets whatever it supports in otg_caps.
> of_usb_updade_otg_caps() checks if it is legacy DT (i.e. no otg-rev) and then
> overrides to legacy otg_caps.
> 
> >  
> > > So I didn't understand why the decision can't be taken here
> > > for non-legacy controller drivers with legacy DT.
> > > They will set gadget->otg_caps and gadget->is_otg.
> > > 
> > > If none of the 4 DT flags are passed then we know it is legacy DT
> > > and we can limit to legacy behaviour.
> > > 
> > legacy behaviour number is 2, not only one legacy behaviour.
> > That's why I leave the otg caps decided by controller drivers.
> 
> I'm only suggesting that it be decided at one place i.e. in 
> of_usb_updade_otg_caps() instead of every controller.
> 
> Do you see any problems with that approach?
> 
The problem is the override policy for legacy platforms. 
For case 3) above, we should enable HNP and SRP,
For case 4) above, after add OTG HNP&SRP support, it should disable HNP and SRP.

How I make one decision in of_usb_updade_otg_caps()
for above 2 cases?(the otg-rev is 0 for both).

Li Jun
> cheers,
> -roger
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in

Re: [PATCH v6] usb: common: add API to set usb otg capabilities by device tree

[Roger Quadros]

On Mon, 22 Jun 2015 18:45:37 +0800
Li Jun <b47624-KZfg59tc24xl57MIdRCFDg@public.gmane.org> wrote:

> On Mon, Jun 22, 2015 at 12:41:22PM +0300, Roger Quadros wrote:
> > On Thu, 18 Jun 2015 21:37:04 +0800
> > Li Jun <b47624-KZfg59tc24xl57MIdRCFDg@public.gmane.org> wrote:
> > 
> > > On Thu, Jun 18, 2015 at 03:07:48PM +0300, Roger Quadros wrote:
> > > > On Thu, 18 Jun 2015 16:47:48 +0800
> > > > Li Jun <b47624-KZfg59tc24xl57MIdRCFDg@public.gmane.org> wrote:
> > > > 
> > > > > On Thu, Jun 18, 2015 at 10:36:50AM +0300, Roger Quadros wrote:
> > > > > > Lin,
> > > > > > 
> > > > > > You can use --in-reply-to "message id of v5 of this path" so that it appears together
> > > > > > with the other patches in peoples mailboxes.
> > > > > > 
> > > > > > > + * the passed properties in DT.
> > > > > > > + * @np: Pointer to the given device_node
> > > > > > > + * @otg_caps: Pointer to the target usb_otg_caps to be set
> > > > > > > + *
> > > > > > > + * The function gets and sets the otg capabilities
> > > > > > > + */
> > > > > > > +void of_usb_set_otg_caps(struct device_node *np, struct usb_otg_caps *otg_caps)
> > > > > > > +{
> > > > > > > +	u32 otg_rev;
> > > > > > > +
> > > > > > > +	if (!otg_caps)
> > > > > > > +		return;
> > > > > > > +
> > > > > > > +	if (!of_property_read_u32(np, "otg-rev", &otg_rev))
> > > > > > > +		otg_caps->otg_rev = otg_rev;
> > > > > > 
> > > > > > should we check if otg_rev is in correct format?
> > > > > > anything non BCD and greater than 0x9999 is invalid.
> > > > > > 
> > > > > > Also if otg-rev is not passed then we need to treat it as legacy
> > > > > > platform right? How is this taken care of?
> > > > > > 
> > > > > Missed this comment
> > > > > This handling rely on controller driver, cannot decided here.
> > > > > There are several cases we need take care:
> > > > > 1) otg-rev is not passed, but all 3 disable flags passed, this is
> > > > >   valid, means user want to disable whole OTG, so only "otg-rev"
> > > > >   not passed, cannot treat as legacy platform.
> > > > > 2) Legacy platform means: none of 4 properties is present.
> > > > 
> > > > Right. Plus controller drivers that are not updated to use these otg_caps
> > > > are also legacy.
> > > >
> > > Did not understand the "Plus" case.
> > > Some of 4 properties is present, but its driver dose not use otg_caps?
> > 
> > yes that is what I meant.
> > 
> > > 
> > > > > 3) Some controller drivers already support OTG HNP/SRP, then change
> > > > >   to utilize those new flags, still should support OTG HNP/SRP w/o
> > > > >   any dt change, so OTG caps should be enabled for legacy platforms.
> > > > 
> > > > I didn't understand this point. If a controller driver is not updated
> > > > to use otg_caps it is legacy and gadget->otg_caps will be NULL.
> > > > 
> > > Some of existing chipdea platforms work fine on HNP and SRP , which did
> > > not use any new flags and properties, after my patch, those platform should
> > > still enable HNP and SRP without any DT change.
> > 
> > Agreed.
> > > 
> > > > > 4) Some controller drivers does not support any OTG, after add OTG
> > > > >   functions and utilize those new flags, should keep OTG disabled
> > > > >   for legacy platforms.
> > > > 
> > > > If controller drivers don't support OTG. gadget->is_otg and
> > > > gadget->otg_caps will not be set by them and they don't have to use
> > > > of_usb_set_otg_caps().
> > > > 
> > > But after my patch, some time later, this driver adds OTG functions on
> > > it new platform, it should disable any OTG feature for its legacy
> > > platforms (none of properties is passed).
> > 
> > That can be decided in of_usb_update_otg_caps().
> > Controller sets whatever it supports in otg_caps.
> > of_usb_updade_otg_caps() checks if it is legacy DT (i.e. no otg-rev) and then
> > overrides to legacy otg_caps.
> > 
> > >  
> > > > So I didn't understand why the decision can't be taken here
> > > > for non-legacy controller drivers with legacy DT.
> > > > They will set gadget->otg_caps and gadget->is_otg.
> > > > 
> > > > If none of the 4 DT flags are passed then we know it is legacy DT
> > > > and we can limit to legacy behaviour.
> > > > 
> > > legacy behaviour number is 2, not only one legacy behaviour.
> > > That's why I leave the otg caps decided by controller drivers.
> > 
> > I'm only suggesting that it be decided at one place i.e. in 
> > of_usb_updade_otg_caps() instead of every controller.
> > 
> > Do you see any problems with that approach?
> > 
> The problem is the override policy for legacy platforms. 
> For case 3) above, we should enable HNP and SRP,

case 3 is controller is supporting new flags but DT is not and we want
legacy OTG enabled, right?
Can't we detect if none of the new otg flags are present then it is legacy DT
so use legacy OTG mode?

> For case 4) above, after add OTG HNP&SRP support, it should disable HNP and SRP.

case 4 is controller was not supporting OTG at all before and now is updated to support
OTG. But for such cases isn't dr_mode = "peripheral" in the DT?
one example is dwc3 controller.

If the dr_mode was "otg" for such case and we want OTG disabled then it is really the DT fault.
We don't need to tackle this case. Just fix up the DT to dr_mode = "peripheral" if
OTG behaviour is not needed.

> 
> How I make one decision in of_usb_updade_otg_caps()
> for above 2 cases?(the otg-rev is 0 for both).
> 

For case 3. otg-rev passed by controller is not 0. otg-rev is just not present in DT.

cheers,
-roger
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in

Re: [PATCH v6] usb: common: add API to set usb otg capabilities by device tree

[Li Jun]

On Mon, Jun 22, 2015 at 04:32:56PM +0300, Roger Quadros wrote:
> On Mon, 22 Jun 2015 18:45:37 +0800
> Li Jun <b47624-KZfg59tc24xl57MIdRCFDg@public.gmane.org> wrote:
> 
> > On Mon, Jun 22, 2015 at 12:41:22PM +0300, Roger Quadros wrote:
> > > On Thu, 18 Jun 2015 21:37:04 +0800
> > > Li Jun <b47624-KZfg59tc24xl57MIdRCFDg@public.gmane.org> wrote:
> > > 
> > > > On Thu, Jun 18, 2015 at 03:07:48PM +0300, Roger Quadros wrote:
> > > > > On Thu, 18 Jun 2015 16:47:48 +0800
> > > > > Li Jun <b47624-KZfg59tc24xl57MIdRCFDg@public.gmane.org> wrote:
> > > > > 
> > > > > > On Thu, Jun 18, 2015 at 10:36:50AM +0300, Roger Quadros wrote:
> > > > > > > Lin,
> > > > > > > 
> > > > > > > You can use --in-reply-to "message id of v5 of this path" so that it appears together
> > > > > > > with the other patches in peoples mailboxes.
> > > > > > > 
> > > > > > > > + * the passed properties in DT.
> > > > > > > > + * @np: Pointer to the given device_node
> > > > > > > > + * @otg_caps: Pointer to the target usb_otg_caps to be set
> > > > > > > > + *
> > > > > > > > + * The function gets and sets the otg capabilities
> > > > > > > > + */
> > > > > > > > +void of_usb_set_otg_caps(struct device_node *np, struct usb_otg_caps *otg_caps)
> > > > > > > > +{
> > > > > > > > +	u32 otg_rev;
> > > > > > > > +
> > > > > > > > +	if (!otg_caps)
> > > > > > > > +		return;
> > > > > > > > +
> > > > > > > > +	if (!of_property_read_u32(np, "otg-rev", &otg_rev))
> > > > > > > > +		otg_caps->otg_rev = otg_rev;
> > > > > > > 
> > > > > > > should we check if otg_rev is in correct format?
> > > > > > > anything non BCD and greater than 0x9999 is invalid.
> > > > > > > 
> > > > > > > Also if otg-rev is not passed then we need to treat it as legacy
> > > > > > > platform right? How is this taken care of?
> > > > > > > 
> > > > > > Missed this comment
> > > > > > This handling rely on controller driver, cannot decided here.
> > > > > > There are several cases we need take care:
> > > > > > 1) otg-rev is not passed, but all 3 disable flags passed, this is
> > > > > >   valid, means user want to disable whole OTG, so only "otg-rev"
> > > > > >   not passed, cannot treat as legacy platform.
> > > > > > 2) Legacy platform means: none of 4 properties is present.
> > > > > 
> > > > > Right. Plus controller drivers that are not updated to use these otg_caps
> > > > > are also legacy.
> > > > >
> > > > Did not understand the "Plus" case.
> > > > Some of 4 properties is present, but its driver dose not use otg_caps?
> > > 
> > > yes that is what I meant.
> > > 
> > > > 
> > > > > > 3) Some controller drivers already support OTG HNP/SRP, then change
> > > > > >   to utilize those new flags, still should support OTG HNP/SRP w/o
> > > > > >   any dt change, so OTG caps should be enabled for legacy platforms.
> > > > > 
> > > > > I didn't understand this point. If a controller driver is not updated
> > > > > to use otg_caps it is legacy and gadget->otg_caps will be NULL.
> > > > > 
> > > > Some of existing chipdea platforms work fine on HNP and SRP , which did
> > > > not use any new flags and properties, after my patch, those platform should
> > > > still enable HNP and SRP without any DT change.
> > > 
> > > Agreed.
> > > > 
> > > > > > 4) Some controller drivers does not support any OTG, after add OTG
> > > > > >   functions and utilize those new flags, should keep OTG disabled
> > > > > >   for legacy platforms.
> > > > > 
> > > > > If controller drivers don't support OTG. gadget->is_otg and
> > > > > gadget->otg_caps will not be set by them and they don't have to use
> > > > > of_usb_set_otg_caps().
> > > > > 
> > > > But after my patch, some time later, this driver adds OTG functions on
> > > > it new platform, it should disable any OTG feature for its legacy
> > > > platforms (none of properties is passed).
> > > 
> > > That can be decided in of_usb_update_otg_caps().
> > > Controller sets whatever it supports in otg_caps.
> > > of_usb_updade_otg_caps() checks if it is legacy DT (i.e. no otg-rev) and then
> > > overrides to legacy otg_caps.
> > > 
> > > >  
> > > > > So I didn't understand why the decision can't be taken here
> > > > > for non-legacy controller drivers with legacy DT.
> > > > > They will set gadget->otg_caps and gadget->is_otg.
> > > > > 
> > > > > If none of the 4 DT flags are passed then we know it is legacy DT
> > > > > and we can limit to legacy behaviour.
> > > > > 
> > > > legacy behaviour number is 2, not only one legacy behaviour.
> > > > That's why I leave the otg caps decided by controller drivers.
> > > 
> > > I'm only suggesting that it be decided at one place i.e. in 
> > > of_usb_updade_otg_caps() instead of every controller.
> > > 
> > > Do you see any problems with that approach?
> > > 
> > The problem is the override policy for legacy platforms. 
> > For case 3) above, we should enable HNP and SRP,
> 
> case 3 is controller is supporting new flags but DT is not and we want
> legacy OTG enabled, right?
> Can't we detect if none of the new otg flags are present then it is legacy DT
> so use legacy OTG mode?
> 
Yes, we can. The legacy OTG mode is HNP&SRP is enabled.

> > For case 4) above, after add OTG HNP&SRP support, it should disable HNP and SRP.
> 
> case 4 is controller was not supporting OTG at all before and now is updated to support
> OTG. But for such cases isn't dr_mode = "peripheral" in the DT?
> one example is dwc3 controller.
> 
> If the dr_mode was "otg" for such case and we want OTG disabled then it is really the DT fault.

It's ID pin detect for dual role switch as many current OTG controllers have.
not DT fault, its dt only has a dr_mode = "otg".

> We don't need to tackle this case. Just fix up the DT to dr_mode = "peripheral" if
> OTG behaviour is not needed.
> 
OTG behaviour is not needed, so we need disable HNP/SRP/ADP. but dr_mode =
"otg" as it already works fine with ID pin detect.

> > 
> > How I make one decision in of_usb_updade_otg_caps()
> > for above 2 cases?(the otg-rev is 0 for both).
> > 
> 
> For case 3. otg-rev passed by controller is not 0. otg-rev is just not present in DT.
> 

Current OTG situation is not so simple, so that we can not use one simple
rule to handle legacy cases in a common place, but for a particular controller
driver, make decision on this is a simple work, I think.

Li Jun

> cheers,
> -roger
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in

Re: [PATCH v6] usb: common: add API to set usb otg capabilities by device tree

[Roger Quadros]

On Mon, 22 Jun 2015 22:36:40 +0800
Li Jun <b47624-KZfg59tc24xl57MIdRCFDg@public.gmane.org> wrote:

> On Mon, Jun 22, 2015 at 04:32:56PM +0300, Roger Quadros wrote:
> > On Mon, 22 Jun 2015 18:45:37 +0800
> > Li Jun <b47624-KZfg59tc24xl57MIdRCFDg@public.gmane.org> wrote:
> > 
> > > On Mon, Jun 22, 2015 at 12:41:22PM +0300, Roger Quadros wrote:
> > > > On Thu, 18 Jun 2015 21:37:04 +0800
> > > > Li Jun <b47624-KZfg59tc24xl57MIdRCFDg@public.gmane.org> wrote:
> > > > 
> > > > > On Thu, Jun 18, 2015 at 03:07:48PM +0300, Roger Quadros wrote:
> > > > > > On Thu, 18 Jun 2015 16:47:48 +0800
> > > > > > Li Jun <b47624-KZfg59tc24xl57MIdRCFDg@public.gmane.org> wrote:
> > > > > > 
> > > > > > > On Thu, Jun 18, 2015 at 10:36:50AM +0300, Roger Quadros wrote:
> > > > > > > > Lin,
> > > > > > > > 
> > > > > > > > You can use --in-reply-to "message id of v5 of this path" so that it appears together
> > > > > > > > with the other patches in peoples mailboxes.
> > > > > > > > 
> > > > > > > > > + * the passed properties in DT.
> > > > > > > > > + * @np: Pointer to the given device_node
> > > > > > > > > + * @otg_caps: Pointer to the target usb_otg_caps to be set
> > > > > > > > > + *
> > > > > > > > > + * The function gets and sets the otg capabilities
> > > > > > > > > + */
> > > > > > > > > +void of_usb_set_otg_caps(struct device_node *np, struct usb_otg_caps *otg_caps)
> > > > > > > > > +{
> > > > > > > > > +	u32 otg_rev;
> > > > > > > > > +
> > > > > > > > > +	if (!otg_caps)
> > > > > > > > > +		return;
> > > > > > > > > +
> > > > > > > > > +	if (!of_property_read_u32(np, "otg-rev", &otg_rev))
> > > > > > > > > +		otg_caps->otg_rev = otg_rev;
> > > > > > > > 
> > > > > > > > should we check if otg_rev is in correct format?
> > > > > > > > anything non BCD and greater than 0x9999 is invalid.
> > > > > > > > 
> > > > > > > > Also if otg-rev is not passed then we need to treat it as legacy
> > > > > > > > platform right? How is this taken care of?
> > > > > > > > 
> > > > > > > Missed this comment
> > > > > > > This handling rely on controller driver, cannot decided here.
> > > > > > > There are several cases we need take care:
> > > > > > > 1) otg-rev is not passed, but all 3 disable flags passed, this is
> > > > > > >   valid, means user want to disable whole OTG, so only "otg-rev"
> > > > > > >   not passed, cannot treat as legacy platform.
> > > > > > > 2) Legacy platform means: none of 4 properties is present.
> > > > > > 
> > > > > > Right. Plus controller drivers that are not updated to use these otg_caps
> > > > > > are also legacy.
> > > > > >
> > > > > Did not understand the "Plus" case.
> > > > > Some of 4 properties is present, but its driver dose not use otg_caps?
> > > > 
> > > > yes that is what I meant.
> > > > 
> > > > > 
> > > > > > > 3) Some controller drivers already support OTG HNP/SRP, then change
> > > > > > >   to utilize those new flags, still should support OTG HNP/SRP w/o
> > > > > > >   any dt change, so OTG caps should be enabled for legacy platforms.
> > > > > > 
> > > > > > I didn't understand this point. If a controller driver is not updated
> > > > > > to use otg_caps it is legacy and gadget->otg_caps will be NULL.
> > > > > > 
> > > > > Some of existing chipdea platforms work fine on HNP and SRP , which did
> > > > > not use any new flags and properties, after my patch, those platform should
> > > > > still enable HNP and SRP without any DT change.
> > > > 
> > > > Agreed.
> > > > > 
> > > > > > > 4) Some controller drivers does not support any OTG, after add OTG
> > > > > > >   functions and utilize those new flags, should keep OTG disabled
> > > > > > >   for legacy platforms.
> > > > > > 
> > > > > > If controller drivers don't support OTG. gadget->is_otg and
> > > > > > gadget->otg_caps will not be set by them and they don't have to use
> > > > > > of_usb_set_otg_caps().
> > > > > > 
> > > > > But after my patch, some time later, this driver adds OTG functions on
> > > > > it new platform, it should disable any OTG feature for its legacy
> > > > > platforms (none of properties is passed).
> > > > 
> > > > That can be decided in of_usb_update_otg_caps().
> > > > Controller sets whatever it supports in otg_caps.
> > > > of_usb_updade_otg_caps() checks if it is legacy DT (i.e. no otg-rev) and then
> > > > overrides to legacy otg_caps.
> > > > 
> > > > >  
> > > > > > So I didn't understand why the decision can't be taken here
> > > > > > for non-legacy controller drivers with legacy DT.
> > > > > > They will set gadget->otg_caps and gadget->is_otg.
> > > > > > 
> > > > > > If none of the 4 DT flags are passed then we know it is legacy DT
> > > > > > and we can limit to legacy behaviour.
> > > > > > 
> > > > > legacy behaviour number is 2, not only one legacy behaviour.
> > > > > That's why I leave the otg caps decided by controller drivers.
> > > > 
> > > > I'm only suggesting that it be decided at one place i.e. in 
> > > > of_usb_updade_otg_caps() instead of every controller.
> > > > 
> > > > Do you see any problems with that approach?
> > > > 
> > > The problem is the override policy for legacy platforms. 
> > > For case 3) above, we should enable HNP and SRP,
> > 
> > case 3 is controller is supporting new flags but DT is not and we want
> > legacy OTG enabled, right?
> > Can't we detect if none of the new otg flags are present then it is legacy DT
> > so use legacy OTG mode?
> > 
> Yes, we can. The legacy OTG mode is HNP&SRP is enabled.
> 
> > > For case 4) above, after add OTG HNP&SRP support, it should disable HNP and SRP.
> > 
> > case 4 is controller was not supporting OTG at all before and now is updated to support
> > OTG. But for such cases isn't dr_mode = "peripheral" in the DT?
> > one example is dwc3 controller.
> > 
> > If the dr_mode was "otg" for such case and we want OTG disabled then it is really the DT fault.
> 
> It's ID pin detect for dual role switch as many current OTG controllers have.
> not DT fault, its dt only has a dr_mode = "otg".
> 
> > We don't need to tackle this case. Just fix up the DT to dr_mode = "peripheral" if
> > OTG behaviour is not needed.
> > 
> OTG behaviour is not needed, so we need disable HNP/SRP/ADP. but dr_mode =
> "otg" as it already works fine with ID pin detect.

Do you know which platform has plain non-otg dual-role working as of today
with dr_mode set to "otg"?
For TI platforms none of them have it working currently.

> 
> > > 
> > > How I make one decision in of_usb_updade_otg_caps()
> > > for above 2 cases?(the otg-rev is 0 for both).
> > > 
> > 
> > For case 3. otg-rev passed by controller is not 0. otg-rev is just not present in DT.
> > 
> 
> Current OTG situation is not so simple, so that we can not use one simple
> rule to handle legacy cases in a common place, but for a particular controller
> driver, make decision on this is a simple work, I think.

OK. Let's continue with your apporach. Maybe USB maintainers can give their opinion
as well.

Later if more controllers are trying to do the same thing then we can introduce a helper
function for those controllers.

cheers,
-roger
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v6] usb: common: add API to set usb otg capabilities by device tree

[Li Jun]

On Tue, Jun 23, 2015 at 10:43:28AM +0300, Roger Quadros wrote:
> > > 
> > > If the dr_mode was "otg" for such case and we want OTG disabled then it is really the DT fault.
> > 
> > It's ID pin detect for dual role switch as many current OTG controllers have.
> > not DT fault, its dt only has a dr_mode = "otg".
> > 
> > > We don't need to tackle this case. Just fix up the DT to dr_mode = "peripheral" if
> > > OTG behaviour is not needed.
> > > 
> > OTG behaviour is not needed, so we need disable HNP/SRP/ADP. but dr_mode =
> > "otg" as it already works fine with ID pin detect.
> 
> Do you know which platform has plain non-otg dual-role working as of today
> with dr_mode set to "otg"?

I don't know.
The dt property dr_mode is already there, and currently there are some platforms
as its user, so I assume those platforms either are non-otg dual-role, or real OTG
with HNP supported, I guess most are the former cases; chipidea is the later case.

> For TI platforms none of them have it working currently.
> 
So for Ti platforms, some enables non-otg dual-role function but do not use
dr_mode = "otg"?

For those only have non-otg dual-role platforms, no matter using dr_mode or not,
we need keep it's real OTG disabled for legacy users after it's controller
driver adds real OTG later.

> > 
> > > > 
> > > > How I make one decision in of_usb_updade_otg_caps()
> > > > for above 2 cases?(the otg-rev is 0 for both).
> > > > 
> > > 
> > > For case 3. otg-rev passed by controller is not 0. otg-rev is just not present in DT.
> > > 
> > 
> > Current OTG situation is not so simple, so that we can not use one simple
> > rule to handle legacy cases in a common place, but for a particular controller
> > driver, make decision on this is a simple work, I think.
> 
> OK. Let's continue with your apporach. Maybe USB maintainers can give their opinion
> as well.
> 
If some simple handling for all legacy platforms can be accepted, I am happy
to follow it in of_usb_update_otg_caps().

Felipe, Peter, any comments on this?

Li Jun

> Later if more controllers are trying to do the same thing then we can introduce a helper
> function for those controllers.
> 
Agree.

> cheers,
> -roger
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v6] usb: common: add API to set usb otg capabilities by device tree

[Roger Quadros]

+ Kukjin, Stephen,

for board specific USB question.

On Tue, 23 Jun 2015 16:35:49 +0800
Li Jun <b47624-KZfg59tc24xl57MIdRCFDg@public.gmane.org> wrote:

> On Tue, Jun 23, 2015 at 10:43:28AM +0300, Roger Quadros wrote:
> > > > 
> > > > If the dr_mode was "otg" for such case and we want OTG disabled then it is really the DT fault.
> > > 
> > > It's ID pin detect for dual role switch as many current OTG controllers have.
> > > not DT fault, its dt only has a dr_mode = "otg".
> > > 
> > > > We don't need to tackle this case. Just fix up the DT to dr_mode = "peripheral" if
> > > > OTG behaviour is not needed.
> > > > 
> > > OTG behaviour is not needed, so we need disable HNP/SRP/ADP. but dr_mode =
> > > "otg" as it already works fine with ID pin detect.
> > 
> > Do you know which platform has plain non-otg dual-role working as of today
> > with dr_mode set to "otg"?
> 
> I don't know.
> The dt property dr_mode is already there, and currently there are some platforms
> as its user, so I assume those platforms either are non-otg dual-role, or real OTG
> with HNP supported, I guess most are the former cases; chipidea is the later case.
> 
> > For TI platforms none of them have it working currently.
> > 
> So for Ti platforms, some enables non-otg dual-role function but do not use
> dr_mode = "otg"?

for TI we have only musb based and dwc3 based platforms. MUSB based are OTG.
dwc3 doesn't support dual-role or OTG yet so we use either "peripheral" or "Host"

> 
> For those only have non-otg dual-role platforms, no matter using dr_mode or not,
> we need keep it's real OTG disabled for legacy users after it's controller
> driver adds real OTG later.

I wouldn't even bother about platforms expecting dual-role behaviour
with dr_mode not set to "otg". That is just plain wrong and can't be supported.

These are the dts files having dr_mode == "otg".
I've sorted them as per Soc vendor and USB controller

Freescale:
----------
powerpc/boot/dts/mpc5121.dtsi:			dr_mode = "otg";
	compatible = "fsl,mpc5121-usb2-dr";
powerpc/boot/dts/asp834x-redboot.dts:			dr_mode = "otg";
	compatible = "fsl-usb2-dr"
powerpc/boot/dts/mpc834x_mds.dts:			dr_mode = "otg";
	compatible = "fsl-usb2-dr";
powerpc/boot/dts/mpc8349emitxgp.dts:			dr_mode = "otg";
	compatible = "fsl-usb2-dr";

arm/boot/dts/imx25-pdk.dts:	dr_mode = "otg";
	compatible = "fsl,imx25-usb", "fsl,imx27-usb";
arm/boot/dts/imx25-eukrea-mbimxsd25-baseboard.dts:	dr_mode = "otg";
	compatible = "fsl,imx25-usb", "fsl,imx27-usb";
arm/boot/dts/imx27-eukrea-cpuimx27.dtsi:	dr_mode = "otg";
	compatible = "fsl,imx27-usb";
arm/boot/dts/imx27-phytec-phycore-som.dtsi:	dr_mode = "otg";
	compatible = "fsl,imx27-usb";
arm/boot/dts/imx27-pdk.dts:	dr_mode = "otg";
	compatible = "fsl,imx27-usb";
arm/boot/dts/imx35-eukrea-mbimxsd35-baseboard.dts:	dr_mode = "otg";
	compatible = "fsl,imx35-usb", "fsl,imx27-usb";
arm/boot/dts/imx51-eukrea-mbimxsd51-baseboard.dts:	dr_mode = "otg";
	compatible = "fsl,imx51-usb", "fsl,imx27-usb";
arm/boot/dts/imx51-babbage.dts:	dr_mode = "otg";
	compatible = "fsl,imx51-usb", "fsl,imx27-usb";
arm/boot/dts/imx51-digi-connectcore-jsk.dts:	dr_mode = "otg";
	compatible = "fsl,imx51-usb", "fsl,imx27-usb";
arm/boot/dts/imx6dl-riotboard.dts:	dr_mode = "otg";
	compatible = "fsl,imx6q-usb", "fsl,imx27-usb";

Li, can you please comment on all the above Freesacle platforms?

STE:
---
arm/boot/dts/ste-dbx5x0.dtsi:			dr_mode = "otg";
	compatible = "stericsson,db8500-musb";

MUSB so legacy OTG.

TI:
---
arm/boot/dts/am33xx.dtsi:				dr_mode = "otg";
	compatible = "ti,musb-am33xx";

MUSB so legacy OTG.

arm/boot/dts/dra7.dtsi:				dr_mode = "otg";
	compatible = "snps,dwc3";	/* board dts overrides to dr_mode = "peripheral" or "host" */
arm/boot/dts/dra74x.dtsi:				dr_mode = "otg";
	compatible = "snps,dwc3";	/* board dts overrides to dr_mode = "peripheral" or "host" */
arm/boot/dts/am4372.dtsi:				dr_mode = "otg";
	compatible = "snps,dwc3";	/* board dts overrides to dr_mode = "peripheral" or "host" */
arm/boot/dts/am4372.dtsi:				dr_mode = "otg";
	compatible = "snps,dwc3";	/* board dts overrides to dr_mode = "peripheral" or "host" */

All these will eventually use dr_mode = "otg" with all 3 disable flags for simple dual-role operation.

Samsung
-------
arm/boot/dts/exynos5422-odroidxu3.dts:	dr_mode = "otg";
	compatible = "snps,dwc3";

I think that this board expects dual-role only behaviour. But that is not functional for DWC3.
Kukjin/Felipe, any comment?

Nvidia
------
arm/boot/dts/tegra20-seaboard.dts:		dr_mode = "otg";
	compatible = "nvidia,tegra20-ehci", "usb-ehci";
arm/boot/dts/tegra30-colibri-eval-v3.dts:		dr_mode = "otg";
	compatible = "nvidia,tegra30-ehci", "usb-ehci";

I couldn't find gadget side implementation for this.
Stephen, any comments on whether this board supports true OTG operation or just dual-role operation?

Cambridge Silicon
-----------------
arm/boot/dts/atlas7.dtsi:				dr_mode = "otg";
	compatible = "sirf,atlas7-usb";

I couldn't find the USB controller driver for this.

cheers,
-roger
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v6] usb: common: add API to set usb otg capabilities by device tree

[Li Jun]

On Tue, Jun 23, 2015 at 02:56:06PM +0300, Roger Quadros wrote:
> + Kukjin, Stephen,
> 
> for board specific USB question.
> 
> On Tue, 23 Jun 2015 16:35:49 +0800
> Li Jun <b47624-KZfg59tc24xl57MIdRCFDg@public.gmane.org> wrote:
> 
> > On Tue, Jun 23, 2015 at 10:43:28AM +0300, Roger Quadros wrote:
> > > > > 
> > > > > If the dr_mode was "otg" for such case and we want OTG disabled then it is really the DT fault.
> > > > 
> > > > It's ID pin detect for dual role switch as many current OTG controllers have.
> > > > not DT fault, its dt only has a dr_mode = "otg".
> > > > 
> > > > > We don't need to tackle this case. Just fix up the DT to dr_mode = "peripheral" if
> > > > > OTG behaviour is not needed.
> > > > > 
> > > > OTG behaviour is not needed, so we need disable HNP/SRP/ADP. but dr_mode =
> > > > "otg" as it already works fine with ID pin detect.
> > > 
> > > Do you know which platform has plain non-otg dual-role working as of today
> > > with dr_mode set to "otg"?
> > 
> > I don't know.
> > The dt property dr_mode is already there, and currently there are some platforms
> > as its user, so I assume those platforms either are non-otg dual-role, or real OTG
> > with HNP supported, I guess most are the former cases; chipidea is the later case.
> > 
> > > For TI platforms none of them have it working currently.
> > > 
> > So for Ti platforms, some enables non-otg dual-role function but do not use
> > dr_mode = "otg"?
> 
> for TI we have only musb based and dwc3 based platforms. MUSB based are OTG.
> dwc3 doesn't support dual-role or OTG yet so we use either "peripheral" or "Host"
> 
> > 
> > For those only have non-otg dual-role platforms, no matter using dr_mode or not,
> > we need keep it's real OTG disabled for legacy users after it's controller
> > driver adds real OTG later.
> 
> I wouldn't even bother about platforms expecting dual-role behaviour
> with dr_mode not set to "otg". That is just plain wrong and can't be supported.
> 
Some platforms may not pass dr_mode in it DT, and treat that means
otg controller will work at otg mode by default, at least I know
current chipidea platforms do so. Therefore we may not list all otg
controller drivers just by grep "dr_mode = "otg"" in its dts files.

> These are the dts files having dr_mode == "otg".
> I've sorted them as per Soc vendor and USB controller
> 
> Freescale:
> ----------
> powerpc/boot/dts/mpc5121.dtsi:			dr_mode = "otg";
> 	compatible = "fsl,mpc5121-usb2-dr";
> powerpc/boot/dts/asp834x-redboot.dts:			dr_mode = "otg";
> 	compatible = "fsl-usb2-dr"
> powerpc/boot/dts/mpc834x_mds.dts:			dr_mode = "otg";
> 	compatible = "fsl-usb2-dr";
> powerpc/boot/dts/mpc8349emitxgp.dts:			dr_mode = "otg";
> 	compatible = "fsl-usb2-dr";
> 
> arm/boot/dts/imx25-pdk.dts:	dr_mode = "otg";
> 	compatible = "fsl,imx25-usb", "fsl,imx27-usb";
> arm/boot/dts/imx25-eukrea-mbimxsd25-baseboard.dts:	dr_mode = "otg";
> 	compatible = "fsl,imx25-usb", "fsl,imx27-usb";
> arm/boot/dts/imx27-eukrea-cpuimx27.dtsi:	dr_mode = "otg";
> 	compatible = "fsl,imx27-usb";
> arm/boot/dts/imx27-phytec-phycore-som.dtsi:	dr_mode = "otg";
> 	compatible = "fsl,imx27-usb";
> arm/boot/dts/imx27-pdk.dts:	dr_mode = "otg";
> 	compatible = "fsl,imx27-usb";
> arm/boot/dts/imx35-eukrea-mbimxsd35-baseboard.dts:	dr_mode = "otg";
> 	compatible = "fsl,imx35-usb", "fsl,imx27-usb";
> arm/boot/dts/imx51-eukrea-mbimxsd51-baseboard.dts:	dr_mode = "otg";
> 	compatible = "fsl,imx51-usb", "fsl,imx27-usb";
> arm/boot/dts/imx51-babbage.dts:	dr_mode = "otg";
> 	compatible = "fsl,imx51-usb", "fsl,imx27-usb";
> arm/boot/dts/imx51-digi-connectcore-jsk.dts:	dr_mode = "otg";
> 	compatible = "fsl,imx51-usb", "fsl,imx27-usb";
> arm/boot/dts/imx6dl-riotboard.dts:	dr_mode = "otg";
> 	compatible = "fsl,imx6q-usb", "fsl,imx27-usb";
> 
> Li, can you please comment on all the above Freesacle platforms?
> 

AFAIK, those FSL platforms are legacy OTG.

Li Jun
 
> STE:
> ---
> arm/boot/dts/ste-dbx5x0.dtsi:			dr_mode = "otg";
> 	compatible = "stericsson,db8500-musb";
> 
> MUSB so legacy OTG.
> 
> TI:
> ---
> arm/boot/dts/am33xx.dtsi:				dr_mode = "otg";
> 	compatible = "ti,musb-am33xx";
> 
> MUSB so legacy OTG.
> 
> arm/boot/dts/dra7.dtsi:				dr_mode = "otg";
> 	compatible = "snps,dwc3";	/* board dts overrides to dr_mode = "peripheral" or "host" */
> arm/boot/dts/dra74x.dtsi:				dr_mode = "otg";
> 	compatible = "snps,dwc3";	/* board dts overrides to dr_mode = "peripheral" or "host" */
> arm/boot/dts/am4372.dtsi:				dr_mode = "otg";
> 	compatible = "snps,dwc3";	/* board dts overrides to dr_mode = "peripheral" or "host" */
> arm/boot/dts/am4372.dtsi:				dr_mode = "otg";
> 	compatible = "snps,dwc3";	/* board dts overrides to dr_mode = "peripheral" or "host" */
> 
> All these will eventually use dr_mode = "otg" with all 3 disable flags for simple dual-role operation.
> 
> Samsung
> -------
> arm/boot/dts/exynos5422-odroidxu3.dts:	dr_mode = "otg";
> 	compatible = "snps,dwc3";
> 
> I think that this board expects dual-role only behaviour. But that is not functional for DWC3.
> Kukjin/Felipe, any comment?
> 
> Nvidia
> ------
> arm/boot/dts/tegra20-seaboard.dts:		dr_mode = "otg";
> 	compatible = "nvidia,tegra20-ehci", "usb-ehci";
> arm/boot/dts/tegra30-colibri-eval-v3.dts:		dr_mode = "otg";
> 	compatible = "nvidia,tegra30-ehci", "usb-ehci";
> 
> I couldn't find gadget side implementation for this.
> Stephen, any comments on whether this board supports true OTG operation or just dual-role operation?
> 
> Cambridge Silicon
> -----------------
> arm/boot/dts/atlas7.dtsi:				dr_mode = "otg";
> 	compatible = "sirf,atlas7-usb";
> 
> I couldn't find the USB controller driver for this.
> 
> cheers,
> -roger
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v6] usb: common: add API to set usb otg capabilities by device tree

[Stephen Warren]

On 06/23/2015 05:56 AM, Roger Quadros wrote:
> + Kukjin, Stephen,
>
> for board specific USB question.
>
> On Tue, 23 Jun 2015 16:35:49 +0800
> Li Jun <b47624-KZfg59tc24xl57MIdRCFDg@public.gmane.org> wrote:
>
>> On Tue, Jun 23, 2015 at 10:43:28AM +0300, Roger Quadros wrote:
>>>>>
>>>>> If the dr_mode was "otg" for such case and we want OTG disabled then it is really the DT fault.
>>>>
>>>> It's ID pin detect for dual role switch as many current OTG controllers have.
>>>> not DT fault, its dt only has a dr_mode = "otg".
>>>>
>>>>> We don't need to tackle this case. Just fix up the DT to dr_mode = "peripheral" if
>>>>> OTG behaviour is not needed.
>>>>>
>>>> OTG behaviour is not needed, so we need disable HNP/SRP/ADP. but dr_mode =
>>>> "otg" as it already works fine with ID pin detect.
>>>
>>> Do you know which platform has plain non-otg dual-role working as of today
>>> with dr_mode set to "otg"?
>>
>> I don't know.
>> The dt property dr_mode is already there, and currently there are some platforms
>> as its user, so I assume those platforms either are non-otg dual-role, or real OTG
>> with HNP supported, I guess most are the former cases; chipidea is the later case.
>>
>>> For TI platforms none of them have it working currently.
>>>
>> So for Ti platforms, some enables non-otg dual-role function but do not use
>> dr_mode = "otg"?
>
> for TI we have only musb based and dwc3 based platforms. MUSB based are OTG.
> dwc3 doesn't support dual-role or OTG yet so we use either "peripheral" or "Host"
>
>>
>> For those only have non-otg dual-role platforms, no matter using dr_mode or not,
>> we need keep it's real OTG disabled for legacy users after it's controller
>> driver adds real OTG later.
>
> I wouldn't even bother about platforms expecting dual-role behaviour
> with dr_mode not set to "otg". That is just plain wrong and can't be supported.
>
> These are the dts files having dr_mode == "otg".
> I've sorted them as per Soc vendor and USB controller
...
> Nvidia
> ------
> arm/boot/dts/tegra20-seaboard.dts:		dr_mode = "otg";
> 	compatible = "nvidia,tegra20-ehci", "usb-ehci";
> arm/boot/dts/tegra30-colibri-eval-v3.dts:		dr_mode = "otg";
> 	compatible = "nvidia,tegra30-ehci", "usb-ehci";
>
> I couldn't find gadget side implementation for this.
> Stephen, any comments on whether this board supports true OTG operation or just dual-role operation?

Sorry for the slow reply; I was on vacation.

I'm not sure what the difference is?

I won't be able to answer for the Colibri board since I'm not familiar 
with the HW. I have the schematics for Seaboard, so I should be able to 
answer once I understand the question:-)
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v6] usb: common: add API to set usb otg capabilities by device tree

[Roger Quadros]

On Thu, 18 Jun 2015 16:47:48 +0800
Li Jun <b47624-KZfg59tc24xl57MIdRCFDg@public.gmane.org> wrote:

> On Thu, Jun 18, 2015 at 10:36:50AM +0300, Roger Quadros wrote:
> > Lin,
> > 
> > You can use --in-reply-to "message id of v5 of this path" so that it appears together
> > with the other patches in peoples mailboxes.
> > 
> > > + * the passed properties in DT.
> > > + * @np: Pointer to the given device_node
> > > + * @otg_caps: Pointer to the target usb_otg_caps to be set
> > > + *
> > > + * The function gets and sets the otg capabilities
> > > + */
> > > +void of_usb_set_otg_caps(struct device_node *np, struct usb_otg_caps *otg_caps)
> > > +{
> > > +	u32 otg_rev;
> > > +
> > > +	if (!otg_caps)
> > > +		return;
> > > +
> > > +	if (!of_property_read_u32(np, "otg-rev", &otg_rev))
> > > +		otg_caps->otg_rev = otg_rev;
> > 
> > should we check if otg_rev is in correct format?
> > anything non BCD and greater than 0x9999 is invalid.
> > 
> > Also if otg-rev is not passed then we need to treat it as legacy
> > platform right? How is this taken care of?
> > 
> Missed this comment
> This handling rely on controller driver, cannot decided here.
> There are several cases we need take care:
> 1) otg-rev is not passed, but all 3 disable flags passed, this is
>   valid, means user want to disable whole OTG, so only "otg-rev"
>   not passed, cannot treat as legacy platform.
> 2) Legacy platform means: none of 4 properties is present.

OK this was our difference in understanding. I was thinking that for non
legacy code otg-rev _must_ be passed. without otg-rev the disable flags
will be ignored. It makes life much easier no?

why do you want otg-rev to be optional for non-legacy DT?

> 3) Some controller drivers already support OTG HNP/SRP, then change
>   to utilize those new flags, still should support OTG HNP/SRP w/o
>   any dt change, so OTG caps should be enabled for legacy platforms.
> 4) Some controller drivers does not support any OTG, after add OTG
>   functions and utilize those new flags, should keep OTG disabled
>   for legacy platforms.
> 

cheers,
-roger
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in

Re: [PATCH v6] usb: common: add API to set usb otg capabilities by device tree

[Li Jun]

On Mon, Jun 22, 2015 at 12:43:37PM +0300, Roger Quadros wrote:
> On Thu, 18 Jun 2015 16:47:48 +0800
> Li Jun <b47624-KZfg59tc24xl57MIdRCFDg@public.gmane.org> wrote:
> 
> > On Thu, Jun 18, 2015 at 10:36:50AM +0300, Roger Quadros wrote:
> > > Lin,
> > > 
> > > You can use --in-reply-to "message id of v5 of this path" so that it appears together
> > > with the other patches in peoples mailboxes.
> > > 
> > > > + * the passed properties in DT.
> > > > + * @np: Pointer to the given device_node
> > > > + * @otg_caps: Pointer to the target usb_otg_caps to be set
> > > > + *
> > > > + * The function gets and sets the otg capabilities
> > > > + */
> > > > +void of_usb_set_otg_caps(struct device_node *np, struct usb_otg_caps *otg_caps)
> > > > +{
> > > > +	u32 otg_rev;
> > > > +
> > > > +	if (!otg_caps)
> > > > +		return;
> > > > +
> > > > +	if (!of_property_read_u32(np, "otg-rev", &otg_rev))
> > > > +		otg_caps->otg_rev = otg_rev;
> > > 
> > > should we check if otg_rev is in correct format?
> > > anything non BCD and greater than 0x9999 is invalid.
> > > 
> > > Also if otg-rev is not passed then we need to treat it as legacy
> > > platform right? How is this taken care of?
> > > 
> > Missed this comment
> > This handling rely on controller driver, cannot decided here.
> > There are several cases we need take care:
> > 1) otg-rev is not passed, but all 3 disable flags passed, this is
> >   valid, means user want to disable whole OTG, so only "otg-rev"
> >   not passed, cannot treat as legacy platform.
> > 2) Legacy platform means: none of 4 properties is present.
> 
> OK this was our difference in understanding. I was thinking that for non
> legacy code otg-rev _must_ be passed. without otg-rev the disable flags
> will be ignored. It makes life much easier no?
> 

I have to consider ID pint detect case, this is a most common usage,
after controller driver use otg_caps and enable OTG HNP/SRP, some
platform still just want ID pin detect, no more otg feature required,
it need disable OTG HNP/SRP/ADP, the dt should be:
dr_mode = "otg";
adp-disable;
hnp-disable;
srp-didable;

in this case, we cannot require DT user still pass a otg-rev, right?

> why do you want otg-rev to be optional for non-legacy DT?
> 

For above ID pin detect case.

> > 3) Some controller drivers already support OTG HNP/SRP, then change
> >   to utilize those new flags, still should support OTG HNP/SRP w/o
> >   any dt change, so OTG caps should be enabled for legacy platforms.
> > 4) Some controller drivers does not support any OTG, after add OTG
> >   functions and utilize those new flags, should keep OTG disabled
> >   for legacy platforms.
> > 
> 
> cheers,
> -roger
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in

Re: [PATCH v6] usb: common: add API to set usb otg capabilities by device tree

[Roger Quadros]

On Mon, 22 Jun 2015 18:56:18 +0800
Li Jun <b47624-KZfg59tc24xl57MIdRCFDg@public.gmane.org> wrote:

> On Mon, Jun 22, 2015 at 12:43:37PM +0300, Roger Quadros wrote:
> > On Thu, 18 Jun 2015 16:47:48 +0800
> > Li Jun <b47624-KZfg59tc24xl57MIdRCFDg@public.gmane.org> wrote:
> > 
> > > On Thu, Jun 18, 2015 at 10:36:50AM +0300, Roger Quadros wrote:
> > > > Lin,
> > > > 
> > > > You can use --in-reply-to "message id of v5 of this path" so that it appears together
> > > > with the other patches in peoples mailboxes.
> > > > 
> > > > > + * the passed properties in DT.
> > > > > + * @np: Pointer to the given device_node
> > > > > + * @otg_caps: Pointer to the target usb_otg_caps to be set
> > > > > + *
> > > > > + * The function gets and sets the otg capabilities
> > > > > + */
> > > > > +void of_usb_set_otg_caps(struct device_node *np, struct usb_otg_caps *otg_caps)
> > > > > +{
> > > > > +	u32 otg_rev;
> > > > > +
> > > > > +	if (!otg_caps)
> > > > > +		return;
> > > > > +
> > > > > +	if (!of_property_read_u32(np, "otg-rev", &otg_rev))
> > > > > +		otg_caps->otg_rev = otg_rev;
> > > > 
> > > > should we check if otg_rev is in correct format?
> > > > anything non BCD and greater than 0x9999 is invalid.
> > > > 
> > > > Also if otg-rev is not passed then we need to treat it as legacy
> > > > platform right? How is this taken care of?
> > > > 
> > > Missed this comment
> > > This handling rely on controller driver, cannot decided here.
> > > There are several cases we need take care:
> > > 1) otg-rev is not passed, but all 3 disable flags passed, this is
> > >   valid, means user want to disable whole OTG, so only "otg-rev"
> > >   not passed, cannot treat as legacy platform.
> > > 2) Legacy platform means: none of 4 properties is present.
> > 
> > OK this was our difference in understanding. I was thinking that for non
> > legacy code otg-rev _must_ be passed. without otg-rev the disable flags
> > will be ignored. It makes life much easier no?
> > 
> 
> I have to consider ID pint detect case, this is a most common usage,
> after controller driver use otg_caps and enable OTG HNP/SRP, some
> platform still just want ID pin detect, no more otg feature required,
> it need disable OTG HNP/SRP/ADP, the dt should be:
> dr_mode = "otg";
> adp-disable;
> hnp-disable;
> srp-didable;
> 
> in this case, we cannot require DT user still pass a otg-rev, right?

Right. Although I'm beginning to think if we should add "drd-only" flag
to explicitly state DRD feature as it might be more common than OTG.

But for current patches I think we can safely assume that
if the 3 flags are not passed and otg-rev is not passed then it is
legacy DT requiring OTG with HNP/SRP.

> 
> > why do you want otg-rev to be optional for non-legacy DT?
> > 
> 
> For above ID pin detect case.

Got it.

cheers,
-roger

> 
> > > 3) Some controller drivers already support OTG HNP/SRP, then change
> > >   to utilize those new flags, still should support OTG HNP/SRP w/o
> > >   any dt change, so OTG caps should be enabled for legacy platforms.
> > > 4) Some controller drivers does not support any OTG, after add OTG
> > >   functions and utilize those new flags, should keep OTG disabled
> > >   for legacy platforms.
> > > 
> > 
> > cheers,
> > -roger
> --
> To unsubscribe from this list: send the line "unsubscribe linux-usb" in
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in