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@xxxxxxxxxxxxx> 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@xxxxxxxxxxxxx> 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@xxxxxxxxxxxxx> 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 devicetree" in
