Re: [PATCH v4 4/4] phy: add phy-hi6220-usb

[zhangfei]

Hi, Roger

On 02/24/2015 06:13 PM, Roger Quadros wrote:
> > > On Sat, Feb 21, 2015 at 11:03:05PM +0800, zhangfei wrote:
> > > > > > > > +static void hi6220_start_peripheral(struct hi6220_priv *priv, bool on)
> > > > > > > > +{
> > > > > > > > +    struct usb_otg *otg = priv->phy.otg;
> > > > > > > > +
> > > > > > > > +    if (!otg->gadget)
> > > > > > > > +        return;
> > > > > > > > +
> > > > > > > > +    if (on)
> > > > > > > > +        usb_gadget_connect(otg->gadget);
> > > > > > > > +    else
> > > > > > > > +        usb_gadget_disconnect(otg->gadget);
> > > > > > >
> > > > > > > why is the PHY fiddling with pullups ?
> > > > > >
> > > > > > We use this to enable/disable otg gadget mode.
> > > > >
> > > > > I got that, but the pullups don't belong to the PHY, they belong to the
> > > > > gadget.
> > > > >
> > > > > > The gpio_id & gpio_vbus are used to distinguish otg gadget mode or
> > > > > > host mode.
> > > > > > When micro usb or otg device attached to otg, gpio_vbus falling down.
> > > > > > And gpio_id = 1 is micro usb, gpio_id = 0 is otg device.
> > > > >
> > > > > all of that I understood clearly :-)
> > > > >
> > > > > > So when micro usb attached, we enable gadget mode; while micro usb
> > > > > > detached, we disable gadget mode, and dwc2 will automatically set to
> > > > > > host mode.
> > > > >
> > > > > that's all fine, I'm concerned about letting the PHY fiddle with
> > > > > something it doesn't own. If I am to change pullups rules in udc-core,
> > > > > this is likely to break down miserably and I don't want to have to go
> > > > > through that.
> > > >
> > > > Thanks for the clarifying.
> > >
> > > no problem.
> > >
> > > > How about using usb_gadget_vbus_connect/disconnect, which are used in many
> > > > files under drivers/usb/phy.
> > > > There is no vbus_session in dwc2/gadget.c, I thought it would be same as
> > > > pullup.
> > > >
> > > > However, usb_gadget_vbus_connect still need para gadget, where should we put
> > > > this file, drivers/usb/phy or drivers/phy
> > >
> > > drivers/phy, if the framework misses anything you need, it's a great
> > > opportunity to give back to the community by extending the framework.
> >
> > Sorry, I am a little confused.
> > I need some concrete suggestion for the next step of this patch, which is required for the community board, hikey board.
> >
> > Do you mean in the future we need use hsotg->phy instead of hsotg->uphy.
> >          struct phy *phy;
> >          struct usb_phy *uphy;
> > usb_phy has many members that struct phy does not have, including otg.
> > struct usb_otg          *otg;
> > Is that mean we need port such member from usb_phy to phy.
>
> In my opinion otg structure should belong to the USB core part that takes care
> of the OTG/DRD state machine. We still don't have a clear solution here and
> I'm currently investigating this.
> My current work is to get Dual role functionality working with DWC3 controller and TI
> platforms.
>
> Currently phy drivers take care of OTG operation themselves but there is an opportunity
> to share code and centralize USB role switching.
> The USB core should be the owner of the Host controller, Gadget controller and the OTG phy
> and should take care of the that.

Good idea.
If you have any patch, I will be very happy to verify.

How about adding these things in drivers/phy/phy-core.c, it is also 
sharable, though not in usb core.

Just tried adding one member struct usb_otg otg to struct phy, since not 
find any good member can hold usb_otg.
In drivers/phy/phy-core.c, adding extcon_register_interest, 
phy_vbus_notifier, phy_set_peripheral, it works for me, dwc2 on hikey board.

> > Besides, are you ok with using usb_gadget_vbus_connect/disconnect.
>
> I don't think PHY is the right place for this even though older drivers seem to be doing so.
> But at the same time there is nowhere else to add this at the moment.
> The right place should be the USB core that is aware of host/gadget, phy and the state of the bus.

Understand.

> > > Scratching one's own itch kinda thing...
> > >
> > > > > > > > +static void hi6220_detect_work(struct work_struct *work)
> > > > > > > > +{
> > > > > > > > +    struct hi6220_priv *priv =
> > > > > > > > +        container_of(work, struct hi6220_priv, work.work);
> > > > > > > > +    int gpio_id, gpio_vbus;
> > > > > > > > +    enum usb_otg_state state;
> > > > > > > > +
> > > > > > > > +    if (!gpio_is_valid(priv->gpio_id) || !gpio_is_valid(priv->gpio_vbus))
> > > > > > > > +        return;
> > > > > > > > +
> > > > > > > > +    gpio_id = gpio_get_value_cansleep(priv->gpio_id);
> > > > > > > > +    gpio_vbus = gpio_get_value_cansleep(priv->gpio_vbus);
> > > > > > >
> > > > > > > looks like this should be using extcon
> > > > > > Not used extcon before.
> > > > > > However, we need gpio_vbus interrupt.
> > > > > > Checked phy-tahvo.c and phy-omap-otg.c, not find extcon related with
> > > > > > interrupt.
> > > > > > Will investigate tomorrow.
> > > > >
> > > > > drivers/extcon/extcon-gpio.c
> > > > I think there is no need to use extcon, gpio is clear enough.
> > > > extcon-gpio.c even do not support dt.
> > >
> > > well, add DT. The whole idea of free software is that we improve on
> > > things we already have. EXTCON is *the* API to handle such things.
>
> I wrote the extcon-gpio-usb.c driver for exactly your use case. It is
> queued for v4.1
> https://lkml.org/lkml/2015/2/2/187

That's great, thanks.
> It takes care of debouncing for you. Although currently it supports only ID gpio,
> it should be very easy to extend to VBUS sense GPIO.
>
> > I think I am still not understanding extcon-gpio, not sure why need use this API here.
>
> several reasons. Let me list a few.
> 1) Code reuse. Every PHY driver doesn't need to implement GPIO/interrupt handling and debouncing.
> It just registers what cable events it wants to hear and gets a notification.
> 2) The events (ID/VBUS) are not only interesting for the PHY driver but also the controller
> driver and the OTG state machine (whenever it exists at a common place) ;).
> 3) standardization because of common API.

Thanks for the explanation.

Zhangfei
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/