Message ID | 1423726646-30336-5-git-send-email-zhangfei.gao@linaro.org |
---|---|
State | New |
Headers | show |
Hi, Kishon On 02/18/2015 01:35 PM, Kishon Vijay Abraham I wrote: > Hi, > > On Thursday 12 February 2015 01:07 PM, Zhangfei Gao wrote: >> Add usb phy controller for hi6220 platform >> >> Signed-off-by: Zhangfei Gao <zhangfei.gao@linaro.org> >> --- >> drivers/phy/Kconfig | 9 ++ >> drivers/phy/Makefile | 1 + >> drivers/phy/phy-hi6220-usb.c | 306 >> +++++++++++++++++++++++++++++++++++++++++++ >> 3 files changed, 316 insertions(+) >> create mode 100644 drivers/phy/phy-hi6220-usb.c > > why is this driver in drivers/phy when it doesn't use the generic PHY > framework at all? > Balbi recommended "new drivers only on drivers/phy/", including usb phy. So Move drivers/usb/phy/phy-hi6220-usb.c to drivers/phy/phy-hi6220-usb.c, required by Balbi. Thanks -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi, Balbi On 02/18/2015 10:35 PM, Felipe Balbi wrote: > On Wed, Feb 18, 2015 at 01:44:21PM +0800, zhangfei wrote: >> Hi, Kishon >> >> On 02/18/2015 01:35 PM, Kishon Vijay Abraham I wrote: >>> Hi, >>> >>> On Thursday 12 February 2015 01:07 PM, Zhangfei Gao wrote: >>>> Add usb phy controller for hi6220 platform >>>> >>>> Signed-off-by: Zhangfei Gao <zhangfei.gao@linaro.org> >>>> --- >>>> drivers/phy/Kconfig | 9 ++ >>>> drivers/phy/Makefile | 1 + >>>> drivers/phy/phy-hi6220-usb.c | 306 >>>> +++++++++++++++++++++++++++++++++++++++++++ >>>> 3 files changed, 316 insertions(+) >>>> create mode 100644 drivers/phy/phy-hi6220-usb.c >>> >>> why is this driver in drivers/phy when it doesn't use the generic PHY >>> framework at all? >>> >> >> Balbi recommended "new drivers only on drivers/phy/", including usb >> phy. > > but it should use the API too. It's not only about a directory, you need > to use the new API. > >> So Move drivers/usb/phy/phy-hi6220-usb.c to >> drivers/phy/phy-hi6220-usb.c, required by Balbi. > > you're reading what I stated the way you like. Sorry for my bad understanding. Still not clear about the otg_set_peripheral, which is required in phy-hi6220-usb.c 1. drivers/usb/dwc2/gadget.c use otg_set_peripheral(hsotg->uphy->otg, &hsotg->gadget); 2. include/linux/phy/phy.h struct phy do not have member otg, while struct usb_phy has. Could you give more hints? Thanks -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 02/20/2015 12:38 PM, Felipe Balbi wrote: > On Fri, Feb 20, 2015 at 11:07:21AM +0800, zhangfei wrote: >> Hi, Balbi >> >> On 02/18/2015 10:35 PM, Felipe Balbi wrote: >>> On Wed, Feb 18, 2015 at 01:44:21PM +0800, zhangfei wrote: >>>> Hi, Kishon >>>> >>>> On 02/18/2015 01:35 PM, Kishon Vijay Abraham I wrote: >>>>> Hi, >>>>> >>>>> On Thursday 12 February 2015 01:07 PM, Zhangfei Gao wrote: >>>>>> Add usb phy controller for hi6220 platform >>>>>> >>>>>> Signed-off-by: Zhangfei Gao <zhangfei.gao@linaro.org> >>>>>> --- >>>>>> drivers/phy/Kconfig | 9 ++ >>>>>> drivers/phy/Makefile | 1 + >>>>>> drivers/phy/phy-hi6220-usb.c | 306 >>>>>> +++++++++++++++++++++++++++++++++++++++++++ >>>>>> 3 files changed, 316 insertions(+) >>>>>> create mode 100644 drivers/phy/phy-hi6220-usb.c >>>>> >>>>> why is this driver in drivers/phy when it doesn't use the generic PHY >>>>> framework at all? >>>>> >>>> >>>> Balbi recommended "new drivers only on drivers/phy/", including usb >>>> phy. >>> >>> but it should use the API too. It's not only about a directory, you need >>> to use the new API. >>> >>>> So Move drivers/usb/phy/phy-hi6220-usb.c to >>>> drivers/phy/phy-hi6220-usb.c, required by Balbi. >>> >>> you're reading what I stated the way you like. >> >> Sorry for my bad understanding. >> >> Still not clear about the otg_set_peripheral, which is required in >> phy-hi6220-usb.c >> >> 1. drivers/usb/dwc2/gadget.c use >> otg_set_peripheral(hsotg->uphy->otg, &hsotg->gadget); >> >> 2. include/linux/phy/phy.h >> struct phy do not have member otg, while struct usb_phy has. >> >> Could you give more hints? > > your set_peripheral doesn't do anything, just holds a pointer. Might as > well not implement it. I'll review your driver more fully tomorrow. > > There a few things which must be changed. > Thanks in advance. We need this call back set_peripheral setting otg->gadget, which used in usb_gadget_connect/disconnect(otg->gadget). The workable method test here is not provide phy-names = "usb2-phy"; Then dwc2 will still use hsotg->uphy instead of hsotg->phy. Though devm_phy_create is used in phy-hi6220-usb.c, phy_ops is not used in fact. Thanks -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi, Balbi On 02/20/2015 10:41 PM, Felipe Balbi 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. 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. 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. > >> +} >> + >> +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. > >> + >> + if (gpio_vbus == 0) { >> + if (gpio_id == 1) >> + state = OTG_STATE_B_PERIPHERAL; >> + else >> + state = OTG_STATE_A_HOST; >> + } else { >> + state = OTG_STATE_A_HOST; >> + } >> + >> + if (priv->state != state) { >> + hi6220_start_peripheral(priv, state == OTG_STATE_B_PERIPHERAL); >> + priv->state = state; >> + } >> +} >> + >> +static irqreturn_t hiusb_gpio_intr(int irq, void *data) >> +{ >> + struct hi6220_priv *priv = (struct hi6220_priv *)data; >> + >> + /* add debounce time */ >> + schedule_delayed_work(&priv->work, msecs_to_jiffies(100)); > > this is really bad. We have threaded interrupt support, right ? Since we use two gpio to distinguish gadget mode or host mode. Debounce time can introduce more accuracy. I think threaded interrupt can not be used for adding debounce time. Here add debounce is just for safety. Thanks -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi, Balbi On 02/21/2015 12:06 AM, Felipe Balbi wrote: > Hi, > > On Fri, Feb 20, 2015 at 11:44:37PM +0800, zhangfei wrote: >> Hi, Balbi >> >> On 02/20/2015 10:41 PM, Felipe Balbi 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. 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 > >>>> +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. > >>>> + if (gpio_vbus == 0) { >>>> + if (gpio_id == 1) >>>> + state = OTG_STATE_B_PERIPHERAL; >>>> + else >>>> + state = OTG_STATE_A_HOST; >>>> + } else { >>>> + state = OTG_STATE_A_HOST; >>>> + } >>>> + >>>> + if (priv->state != state) { >>>> + hi6220_start_peripheral(priv, state == OTG_STATE_B_PERIPHERAL); >>>> + priv->state = state; >>>> + } >>>> +} >>>> + >>>> +static irqreturn_t hiusb_gpio_intr(int irq, void *data) >>>> +{ >>>> + struct hi6220_priv *priv = (struct hi6220_priv *)data; >>>> + >>>> + /* add debounce time */ >>>> + schedule_delayed_work(&priv->work, msecs_to_jiffies(100)); >>> >>> this is really bad. We have threaded interrupt support, right ? >> >> Since we use two gpio to distinguish gadget mode or host mode. >> Debounce time can introduce more accuracy. > > gpio_set_debounce() ? Not all gpio.c support set_debounce, including gpio-pl061.c. > >> I think threaded interrupt can not be used for adding debounce time. >> Here add debounce is just for safety. > > add the debounce to the gpio itself. Here the debounce added only for safety. gpio_id may mis-report when unplug usb, but it is correct for plug usb & otg device. So debounce can be omitted. If you think using delayed work for debounce is ugly, it is fine switch to threaded_irq. Thanks -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi, Balbi On 02/22/2015 12:21 AM, Felipe Balbi wrote: > Hi, > > 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. Besides, are you ok with using usb_gadget_vbus_connect/disconnect. > > 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 think I am still not understanding extcon-gpio, not sure why need use this API here. Here two gpio requires, one gpio as interrupt, in the interrupt handler, we detect the gpio status judging the otg status. extcon-gpio.c use the interrupt, then can we also use the gpio interrupt. Using extcon-gpio is used for saving gpio_request? > > Quite frankly, though, Roger Quadros (now in Cc) has been working to get > DT support on extcon-gpio, so perhaps wait for that and base your > patches on top of his. > > Now your statement that GPIO is clear enough is completely bogus to me. > > Why do we have fixed regulators with GPIO enable signals, right ? Might > as well just fiddle with the GPIO directly, right ? Wrong, the idea of > using these frameworks is to encapsulate implementation details and make > sure that if things change from one board to another, we can still use > our SW without major modifications. > >>>>>> + if (gpio_vbus == 0) { >>>>>> + if (gpio_id == 1) >>>>>> + state = OTG_STATE_B_PERIPHERAL; >>>>>> + else >>>>>> + state = OTG_STATE_A_HOST; >>>>>> + } else { >>>>>> + state = OTG_STATE_A_HOST; >>>>>> + } >>>>>> + >>>>>> + if (priv->state != state) { >>>>>> + hi6220_start_peripheral(priv, state == OTG_STATE_B_PERIPHERAL); >>>>>> + priv->state = state; >>>>>> + } >>>>>> +} >>>>>> + >>>>>> +static irqreturn_t hiusb_gpio_intr(int irq, void *data) >>>>>> +{ >>>>>> + struct hi6220_priv *priv = (struct hi6220_priv *)data; >>>>>> + >>>>>> + /* add debounce time */ >>>>>> + schedule_delayed_work(&priv->work, msecs_to_jiffies(100)); >>>>> >>>>> this is really bad. We have threaded interrupt support, right ? >>>> >>>> Since we use two gpio to distinguish gadget mode or host mode. >>>> Debounce time can introduce more accuracy. >>> >>> gpio_set_debounce() ? >> Not all gpio.c support set_debounce, including gpio-pl061.c. > > then the framework should implement it in SW. That was the whole idea of > adding set_debounce() to gpiolib. If the controller can't handle it, > gpiolib handles it in SW. Again, encapsulating details. > >>>> I think threaded interrupt can not be used for adding debounce time. >>>> Here add debounce is just for safety. >>> >>> add the debounce to the gpio itself. >> >> Here the debounce added only for safety. >> gpio_id may mis-report when unplug usb, but it is correct for plug usb & otg >> device. >> So debounce can be omitted. >> If you think using delayed work for debounce is ugly, it is fine switch to >> threaded_irq. > > debounce might be very well needed. We *do* want to filter out the > transient period. I'm just telling you there are better ways of doing > so; and your response to that is "let's just remove it" and I'm not > really comfortable with that attitude. > > That's the attitude of a lazy person which, I hope, you are not ;) Understand. What I mean here is gpio_id is not used when unplug, it is only used after plug. Thanks -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 02/23/2015 11:36 PM, Felipe Balbi wrote: > Hi, > > On Sun, Feb 22, 2015 at 11:10:36AM +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; > > yes, we need to move everybody to use struct phy, instead of struct > usb_phy. > >> 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. > > This means we have a little ground work to do before we can add your phy > driver to that framework, right ? As I said, if the framework is missing > anything, work with Kishon (generic phy maintainer) to add those missing > pieces generically. OK, got it. > >> Besides, are you ok with using usb_gadget_vbus_connect/disconnect. >> >>> >>> 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 think I am still not understanding extcon-gpio, not sure why need >> use this API here. > > because extcon is the API to use for external connectors. The same way > you use regulator framework to control that single GPIO tied to an > enable signal of a fixed regulator, you use extcon when you need to read > that gpio signal tied to id pin of the USB connector. > >> Here two gpio requires, one gpio as interrupt, in the interrupt >> handler, we detect the gpio status judging the otg status. >> extcon-gpio.c use the interrupt, then can we also use the gpio >> interrupt. Using extcon-gpio is used for saving gpio_request? > > extcon is used to hide gpio_request from dwc2. dwc2 only knows about > extcon, not gpios. extcon will request the gpio and use it as interrupt > source. When an IRQ fires, it will read the gpio state and decide if it > should broadcast a message to tell dwc2 to become host or peripheral. Thanks for the kind education, understand now. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
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 devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/phy/Kconfig b/drivers/phy/Kconfig index ccad880..40a1ef1 100644 --- a/drivers/phy/Kconfig +++ b/drivers/phy/Kconfig @@ -162,6 +162,15 @@ config PHY_HIX5HD2_SATA help Support for SATA PHY on Hisilicon hix5hd2 Soc. +config PHY_HI6220_USB + tristate "hi6220 USB PHY support" + select USB_PHY + select MFD_SYSCON + help + Enable this to support the HISILICON HI6220 USB PHY. + + To compile this driver as a module, choose M here. + config PHY_SUN4I_USB tristate "Allwinner sunxi SoC USB PHY driver" depends on ARCH_SUNXI && HAS_IOMEM && OF diff --git a/drivers/phy/Makefile b/drivers/phy/Makefile index aa74f96..ec43c2d 100644 --- a/drivers/phy/Makefile +++ b/drivers/phy/Makefile @@ -19,6 +19,7 @@ obj-$(CONFIG_TI_PIPE3) += phy-ti-pipe3.o obj-$(CONFIG_TWL4030_USB) += phy-twl4030-usb.o obj-$(CONFIG_PHY_EXYNOS5250_SATA) += phy-exynos5250-sata.o obj-$(CONFIG_PHY_HIX5HD2_SATA) += phy-hix5hd2-sata.o +obj-$(CONFIG_PHY_HI6220_USB) += phy-hi6220-usb.o obj-$(CONFIG_PHY_SUN4I_USB) += phy-sun4i-usb.o obj-$(CONFIG_PHY_SAMSUNG_USB2) += phy-exynos-usb2.o phy-exynos-usb2-y += phy-samsung-usb2.o diff --git a/drivers/phy/phy-hi6220-usb.c b/drivers/phy/phy-hi6220-usb.c new file mode 100644 index 0000000..0d9f5ac --- /dev/null +++ b/drivers/phy/phy-hi6220-usb.c @@ -0,0 +1,306 @@ +/* + * Copyright (c) 2015 Linaro Ltd. + * Copyright (c) 2015 Hisilicon Limited. + * + * This program is free software; you can redistribute it and/or modify + * it under the terms of the GNU General Public License as published by + * the Free Software Foundation; either version 2 of the License, or + * (at your option) any later version. + */ + +#include <linux/clk.h> +#include <linux/mfd/syscon.h> +#include <linux/of_gpio.h> +#include <linux/platform_device.h> +#include <linux/regmap.h> +#include <linux/regulator/consumer.h> +#include <linux/usb/gadget.h> +#include <linux/usb/otg.h> + +#define SC_PERIPH_CTRL4 0x00c + +#define CTRL4_PICO_SIDDQ BIT(6) +#define CTRL4_PICO_OGDISABLE BIT(8) +#define CTRL4_PICO_VBUSVLDEXT BIT(10) +#define CTRL4_PICO_VBUSVLDEXTSEL BIT(11) +#define CTRL4_OTG_PHY_SEL BIT(21) + +#define SC_PERIPH_CTRL5 0x010 + +#define CTRL5_USBOTG_RES_SEL BIT(3) +#define CTRL5_PICOPHY_ACAENB BIT(4) +#define CTRL5_PICOPHY_BC_MODE BIT(5) +#define CTRL5_PICOPHY_CHRGSEL BIT(6) +#define CTRL5_PICOPHY_VDATSRCEND BIT(7) +#define CTRL5_PICOPHY_VDATDETENB BIT(8) +#define CTRL5_PICOPHY_DCDENB BIT(9) +#define CTRL5_PICOPHY_IDDIG BIT(10) + +#define SC_PERIPH_CTRL8 0x018 +#define SC_PERIPH_RSTEN0 0x300 +#define SC_PERIPH_RSTDIS0 0x304 + +#define RST0_USBOTG_BUS BIT(4) +#define RST0_POR_PICOPHY BIT(5) +#define RST0_USBOTG BIT(6) +#define RST0_USBOTG_32K BIT(7) + +#define EYE_PATTERN_PARA 0x7053348c + +struct hi6220_priv { + struct usb_phy phy; + struct delayed_work work; + struct regmap *reg; + struct clk *clk; + struct regulator *vcc; + struct device *dev; + int gpio_vbus; + int gpio_id; + enum usb_otg_state state; +}; + +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); +} + +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); + + if (gpio_vbus == 0) { + if (gpio_id == 1) + state = OTG_STATE_B_PERIPHERAL; + else + state = OTG_STATE_A_HOST; + } else { + state = OTG_STATE_A_HOST; + } + + if (priv->state != state) { + hi6220_start_peripheral(priv, state == OTG_STATE_B_PERIPHERAL); + priv->state = state; + } +} + +static irqreturn_t hiusb_gpio_intr(int irq, void *data) +{ + struct hi6220_priv *priv = (struct hi6220_priv *)data; + + /* add debounce time */ + schedule_delayed_work(&priv->work, msecs_to_jiffies(100)); + return IRQ_HANDLED; +} + +static int hi6220_set_peripheral(struct usb_otg *otg, struct usb_gadget *gadget) +{ + otg->gadget = gadget; + return 0; +} + +static int hi6220_phy_setup(struct hi6220_priv *priv, bool on) +{ + struct regmap *reg = priv->reg; + u32 val, mask; + int ret; + + if (priv->reg == NULL) + return 0; + + if (on) { + val = RST0_USBOTG_BUS | RST0_POR_PICOPHY | + RST0_USBOTG | RST0_USBOTG_32K; + mask = val; + ret = regmap_update_bits(reg, SC_PERIPH_RSTDIS0, mask, val); + if (ret) + goto out; + + val = CTRL5_USBOTG_RES_SEL | CTRL5_PICOPHY_ACAENB; + mask = val | CTRL5_PICOPHY_BC_MODE; + ret = regmap_update_bits(reg, SC_PERIPH_CTRL5, mask, val); + if (ret) + goto out; + + val = CTRL4_PICO_VBUSVLDEXT | CTRL4_PICO_VBUSVLDEXTSEL | + CTRL4_OTG_PHY_SEL; + mask = val | CTRL4_PICO_SIDDQ | CTRL4_PICO_OGDISABLE; + ret = regmap_update_bits(reg, SC_PERIPH_CTRL4, mask, val); + if (ret) + goto out; + + ret = regmap_write(reg, SC_PERIPH_CTRL8, EYE_PATTERN_PARA); + if (ret) + goto out; + } else { + val = CTRL4_PICO_SIDDQ; + mask = val; + ret = regmap_update_bits(reg, SC_PERIPH_CTRL4, mask, val); + if (ret) + goto out; + + val = RST0_USBOTG_BUS | RST0_POR_PICOPHY | + RST0_USBOTG | RST0_USBOTG_32K; + mask = val; + ret = regmap_update_bits(reg, SC_PERIPH_RSTEN0, mask, val); + if (ret) + goto out; + } + + return 0; +out: + dev_err(priv->dev, "failed to setup phy ret: %d\n", ret); + return ret; +} + +static int hi6220_phy_probe(struct platform_device *pdev) +{ + struct hi6220_priv *priv; + struct usb_otg *otg; + struct device_node *np = pdev->dev.of_node; + struct device *dev = &pdev->dev; + int ret, irq; + + priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL); + if (!priv) + return -ENOMEM; + + otg = devm_kzalloc(dev, sizeof(*otg), GFP_KERNEL); + if (!otg) + return -ENOMEM; + + priv->dev = dev; + priv->phy.dev = &pdev->dev; + priv->phy.otg = otg; + priv->phy.label = "hi6220"; + priv->phy.type = USB_PHY_TYPE_USB2; + otg->set_peripheral = hi6220_set_peripheral; + platform_set_drvdata(pdev, priv); + + priv->gpio_vbus = of_get_named_gpio(np, "hisilicon,vbus-gpios", 0); + if (priv->gpio_vbus == -EPROBE_DEFER) + return -EPROBE_DEFER; + if (!gpio_is_valid(priv->gpio_vbus)) { + dev_err(dev, "invalid gpio %d\n", priv->gpio_vbus); + return -ENODEV; + } + + priv->gpio_id = of_get_named_gpio(np, "hisilicon,id-gpios", 0); + if (priv->gpio_id == -EPROBE_DEFER) + return -EPROBE_DEFER; + if (!gpio_is_valid(priv->gpio_id)) { + dev_err(dev, "invalid gpio %d\n", priv->gpio_id); + return -ENODEV; + } + + priv->reg = syscon_regmap_lookup_by_phandle(pdev->dev.of_node, + "hisilicon,peripheral-syscon"); + if (IS_ERR(priv->reg)) + priv->reg = NULL; + + ret = devm_gpio_request_one(dev, priv->gpio_vbus, + GPIOF_IN, "gpio_vbus"); + if (ret < 0) { + dev_err(dev, "gpio request failed for gpio_vbus\n"); + return ret; + } + + ret = devm_gpio_request_one(dev, priv->gpio_id, GPIOF_IN, "gpio_id"); + if (ret < 0) { + dev_err(dev, "gpio request failed for gpio_id\n"); + return ret; + } + + priv->vcc = devm_regulator_get(dev, "vcc"); + if (IS_ERR(priv->vcc)) { + if (PTR_ERR(priv->vcc) == -EPROBE_DEFER) + return -EPROBE_DEFER; + dev_info(dev, "No regulator found\n"); + } else { + ret = regulator_enable(priv->vcc); + if (ret) { + dev_err(dev, "Failed to enable regulator\n"); + return -ENODEV; + } + } + + priv->clk = devm_clk_get(dev, NULL); + if (IS_ERR(priv->clk)) { + regulator_disable(priv->vcc); + return PTR_ERR(priv->clk); + } + clk_prepare_enable(priv->clk); + INIT_DELAYED_WORK(&priv->work, hi6220_detect_work); + + irq = gpio_to_irq(priv->gpio_vbus); + ret = devm_request_irq(dev, gpio_to_irq(priv->gpio_vbus), + hiusb_gpio_intr, IRQF_NO_SUSPEND | + IRQF_TRIGGER_FALLING | IRQF_TRIGGER_RISING, + "vbus_gpio_intr", priv); + if (ret) { + dev_err(dev, "request gpio irq failed.\n"); + goto err_irq; + } + + hi6220_phy_setup(priv, true); + ret = usb_add_phy_dev(&priv->phy); + if (ret) { + dev_err(dev, "Can't register transceiver\n"); + goto err_irq; + } + schedule_delayed_work(&priv->work, 0); + + return 0; +err_irq: + cancel_delayed_work_sync(&priv->work); + clk_disable_unprepare(priv->clk); + regulator_disable(priv->vcc); + return ret; +} + +static int hi6220_phy_remove(struct platform_device *pdev) +{ + struct hi6220_priv *priv = platform_get_drvdata(pdev); + + cancel_delayed_work_sync(&priv->work); + hi6220_phy_setup(priv, false); + clk_disable_unprepare(priv->clk); + regulator_disable(priv->vcc); + return 0; +} + +static const struct of_device_id hi6220_phy_of_match[] = { + {.compatible = "hisilicon,hi6220-usb-phy",}, + { }, +}; +MODULE_DEVICE_TABLE(of, hi6220_phy_of_match); + +static struct platform_driver hi6220_phy_driver = { + .probe = hi6220_phy_probe, + .remove = hi6220_phy_remove, + .driver = { + .name = "hi6220-usb-phy", + .of_match_table = hi6220_phy_of_match, + } +}; +module_platform_driver(hi6220_phy_driver); + +MODULE_DESCRIPTION("HISILICON HI6220 USB PHY driver"); +MODULE_ALIAS("platform:hi6220-usb-phy"); +MODULE_LICENSE("GPL");
Add usb phy controller for hi6220 platform Signed-off-by: Zhangfei Gao <zhangfei.gao@linaro.org> --- drivers/phy/Kconfig | 9 ++ drivers/phy/Makefile | 1 + drivers/phy/phy-hi6220-usb.c | 306 +++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 316 insertions(+) create mode 100644 drivers/phy/phy-hi6220-usb.c