| Message ID | 1361946799-29471-7-git-send-email-peter.chen@freescale.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi, On Wed, Feb 27, 2013 at 02:33:17PM +0800, Peter Chen wrote: > - For host, the vbus should always be on. > - For otg, the vbus is off defaultly, the vbus needs to be > turned on/off when usb role switches. > > Signed-off-by: Peter Chen <peter.chen@freescale.com> one quick question, if chipidea already has an imx glue layer, can I delete the broken imx_udc ? It doesn't even compile and it's including headers which don't exist.
On Wed, Feb 27, 2013 at 09:55:10AM +0200, Felipe Balbi wrote: > Hi, > > On Wed, Feb 27, 2013 at 02:33:17PM +0800, Peter Chen wrote: > > - For host, the vbus should always be on. > > - For otg, the vbus is off defaultly, the vbus needs to be > > turned on/off when usb role switches. > > > > Signed-off-by: Peter Chen <peter.chen@freescale.com> > > one quick question, if chipidea already has an imx glue layer, can I > delete the broken imx_udc ? It doesn't even compile and it's including > headers which don't exist. This driver handles i.MX1. This one has a gadget only core which is not chipidea compatible. You should ping Darius Augulis <augulis.darius@gmail.com> if he is willing to put some love into the driver. Sascha
On Wed, Feb 27, 2013 at 09:07:35AM +0100, Sascha Hauer wrote: > On Wed, Feb 27, 2013 at 09:55:10AM +0200, Felipe Balbi wrote: > > Hi, > > > > On Wed, Feb 27, 2013 at 02:33:17PM +0800, Peter Chen wrote: > > > - For host, the vbus should always be on. > > > - For otg, the vbus is off defaultly, the vbus needs to be > > > turned on/off when usb role switches. > > > > > > Signed-off-by: Peter Chen <peter.chen@freescale.com> > > > > one quick question, if chipidea already has an imx glue layer, can I > > delete the broken imx_udc ? It doesn't even compile and it's including > > headers which don't exist. > > This driver handles i.MX1. This one has a gadget only core which is not > chipidea compatible. You should ping Darius Augulis <augulis.darius@gmail.com> > if he is willing to put some love into the driver. Sascha is correct.
Hi Darius, On Wed, Feb 27, 2013 at 04:13:14PM +0800, Peter Chen wrote: > On Wed, Feb 27, 2013 at 09:07:35AM +0100, Sascha Hauer wrote: > > On Wed, Feb 27, 2013 at 09:55:10AM +0200, Felipe Balbi wrote: > > > Hi, > > > > > > On Wed, Feb 27, 2013 at 02:33:17PM +0800, Peter Chen wrote: > > > > - For host, the vbus should always be on. > > > > - For otg, the vbus is off defaultly, the vbus needs to be > > > > turned on/off when usb role switches. > > > > > > > > Signed-off-by: Peter Chen <peter.chen@freescale.com> > > > > > > one quick question, if chipidea already has an imx glue layer, can I > > > delete the broken imx_udc ? It doesn't even compile and it's including > > > headers which don't exist. > > > > This driver handles i.MX1. This one has a gadget only core which is not > > chipidea compatible. You should ping Darius Augulis <augulis.darius@gmail.com> > > if he is willing to put some love into the driver. > > Sascha is correct. do you plan to put any work on drivers/usb/gadget/imx_udc.c ? It doesn't even compile since it includes headers which don't exist (and no, simply removing the include doesn't help). If nobody is going to work on that driver, I'm thinking about scheduling it for removal. We can't keep such drivers that nobody cares (and just break tree compilation) around.
Hi balbi, actually no. I don't work with this hardware any more and don't have free time to support it. Please feel free to remove it if it breaks other things. Darius. On Wed, Feb 27, 2013 at 10:36 AM, Felipe Balbi <balbi@ti.com> wrote: > Hi Darius, > > On Wed, Feb 27, 2013 at 04:13:14PM +0800, Peter Chen wrote: >> On Wed, Feb 27, 2013 at 09:07:35AM +0100, Sascha Hauer wrote: >> > On Wed, Feb 27, 2013 at 09:55:10AM +0200, Felipe Balbi wrote: >> > > Hi, >> > > >> > > On Wed, Feb 27, 2013 at 02:33:17PM +0800, Peter Chen wrote: >> > > > - For host, the vbus should always be on. >> > > > - For otg, the vbus is off defaultly, the vbus needs to be >> > > > turned on/off when usb role switches. >> > > > >> > > > Signed-off-by: Peter Chen <peter.chen@freescale.com> >> > > >> > > one quick question, if chipidea already has an imx glue layer, can I >> > > delete the broken imx_udc ? It doesn't even compile and it's including >> > > headers which don't exist. >> > >> > This driver handles i.MX1. This one has a gadget only core which is not >> > chipidea compatible. You should ping Darius Augulis <augulis.darius@gmail.com> >> > if he is willing to put some love into the driver. >> >> Sascha is correct. > > do you plan to put any work on drivers/usb/gadget/imx_udc.c ? It doesn't > even compile since it includes headers which don't exist (and no, > simply removing the include doesn't help). > > If nobody is going to work on that driver, I'm thinking about scheduling > it for removal. We can't keep such drivers that nobody cares (and just > break tree compilation) around. > > -- > balbi
On Wed, Feb 27, 2013 at 09:08:11PM +0200, Alexander Shishkin wrote: > On Feb 27, 2013 8:34 AM, "Peter Chen" <peter.chen@freescale.com> wrote: > > > > - For host, the vbus should always be on. > > - For otg, the vbus is off defaultly, the vbus needs to be > > turned on/off when usb role switches. > > > > Signed-off-by: Peter Chen <peter.chen@freescale.com> > > --- > > drivers/usb/chipidea/ci.h | 2 + > > drivers/usb/chipidea/ci13xxx_imx.c | 76 > ++++++++++++++++++++++++++++-------- > > 2 files changed, 61 insertions(+), 17 deletions(-) > > > > diff --git a/drivers/usb/chipidea/ci.h b/drivers/usb/chipidea/ci.h > > index 0bd17a5..5663092 100644 > > --- a/drivers/usb/chipidea/ci.h > > +++ b/drivers/usb/chipidea/ci.h > > @@ -133,6 +133,7 @@ struct hw_bank { > > * @id_event: indicates there is a id event, and handled at ci_otg_work > > * @b_sess_valid_event: indicates there is a vbus event, and handled > > * at ci_otg_work > > + * @reg_vbus: used to control internal vbus regulator > > */ > > struct ci13xxx { > > struct device *dev; > > @@ -172,6 +173,7 @@ struct ci13xxx { > > struct usb_otg otg; > > bool id_event; > > bool b_sess_valid_event; > > + struct regulator *reg_vbus; > > }; > > I suggest that this regulator be taken care of by either the platform code > or phy layer, it doesn't look like it really belongs in chipidea core. The > easiest way I guess would be to do something similar to the notify callback > in msm platform code. I guess it would look better at the phy layer, but looks like we need another ->set_vbus() method to switch VBUS on/off.
On Wed, Feb 27, 2013 at 09:08:11PM +0200, Alexander Shishkin wrote: > On Feb 27, 2013 8:34 AM, "Peter Chen" <peter.chen@freescale.com> wrote: > > > > - For host, the vbus should always be on. > > - For otg, the vbus is off defaultly, the vbus needs to be > > turned on/off when usb role switches. > > > > + * @reg_vbus: used to control internal vbus regulator > > */ > > struct ci13xxx { > > struct device *dev; > > @@ -172,6 +173,7 @@ struct ci13xxx { > > struct usb_otg otg; > > bool id_event; > > bool b_sess_valid_event; > > + struct regulator *reg_vbus; > > }; > > I suggest that this regulator be taken care of by either the platform code > or phy layer, it doesn't look like it really belongs in chipidea core. The > easiest way I guess would be to do something similar to the notify > callback in msm platform code. VBUS operation is typical operation during the USB code, eg, we need to open vbus at Host mode, we need close/open vbus during host<->gadget switch. I suggest vbus regulator is specified at platform code, and assign it to ci core's pdata, like sasche's patch pdata->dr_mode and pdata->phy_mode. At core code, we can have a vbus operation API for on/off vbus if vbus regulator is existed.
On Thu, Feb 28, 2013 at 02:06:09PM +0800, Peter Chen wrote: > On Wed, Feb 27, 2013 at 09:08:11PM +0200, Alexander Shishkin wrote: > > On Feb 27, 2013 8:34 AM, "Peter Chen" <peter.chen@freescale.com> wrote: > > > > > > - For host, the vbus should always be on. > > > - For otg, the vbus is off defaultly, the vbus needs to be > > > turned on/off when usb role switches. > > > > > > + * @reg_vbus: used to control internal vbus regulator > > > */ > > > struct ci13xxx { > > > struct device *dev; > > > @@ -172,6 +173,7 @@ struct ci13xxx { > > > struct usb_otg otg; > > > bool id_event; > > > bool b_sess_valid_event; > > > + struct regulator *reg_vbus; > > > }; > > > > I suggest that this regulator be taken care of by either the platform code > > or phy layer, it doesn't look like it really belongs in chipidea core. The > > easiest way I guess would be to do something similar to the notify > > callback in msm platform code. > > VBUS operation is typical operation during the USB code, eg, we need > to open vbus at Host mode, we need close/open vbus during host<->gadget > switch. > > I suggest vbus regulator is specified at platform code, and assign it > to ci core's pdata, like sasche's patch pdata->dr_mode and pdata->phy_mode. you don't need platform_data for it. From ci core probe try to get the regulator, if it doesn't exist, ignore the error and continue anyways.
On Thu, Feb 28, 2013 at 09:29:46AM +0200, Felipe Balbi wrote: > On Thu, Feb 28, 2013 at 02:06:09PM +0800, Peter Chen wrote: > > On Wed, Feb 27, 2013 at 09:08:11PM +0200, Alexander Shishkin wrote: > > > On Feb 27, 2013 8:34 AM, "Peter Chen" <peter.chen@freescale.com> wrote: > > > > > > > > - For host, the vbus should always be on. > > > > - For otg, the vbus is off defaultly, the vbus needs to be > > > > turned on/off when usb role switches. > > > > > > > > + * @reg_vbus: used to control internal vbus regulator > > > > */ > > > > struct ci13xxx { > > > > struct device *dev; > > > > @@ -172,6 +173,7 @@ struct ci13xxx { > > > > struct usb_otg otg; > > > > bool id_event; > > > > bool b_sess_valid_event; > > > > + struct regulator *reg_vbus; > > > > }; > > > > > > I suggest that this regulator be taken care of by either the platform code > > > or phy layer, it doesn't look like it really belongs in chipidea core. The > > > easiest way I guess would be to do something similar to the notify > > > callback in msm platform code. > > > > VBUS operation is typical operation during the USB code, eg, we need > > to open vbus at Host mode, we need close/open vbus during host<->gadget > > switch. > > > > I suggest vbus regulator is specified at platform code, and assign it > > to ci core's pdata, like sasche's patch pdata->dr_mode and pdata->phy_mode. > > you don't need platform_data for it. From ci core probe try to get the > regulator, if it doesn't exist, ignore the error and continue anyways. The pdev of ci core is created by platform layer, at ci core probe, it can't get platform things unless it is passed from platform layer > > -- > balbi
Hi Greg, On Wed, Feb 27, 2013 at 04:43:22PM +0200, Darius Augulis wrote: > Hi balbi, > > actually no. I don't work with this hardware any more and don't have > free time to support it. > Please feel free to remove it if it breaks other things. since nobody seems to care about imx_udc and it's currently even including unexistent headers, I'll schedule it for removal on v3.11 (skipping v3.10 to give time for people to complain). Is that alright with you ?
diff --git a/drivers/usb/chipidea/ci.h b/drivers/usb/chipidea/ci.h index 0bd17a5..5663092 100644 --- a/drivers/usb/chipidea/ci.h +++ b/drivers/usb/chipidea/ci.h @@ -133,6 +133,7 @@ struct hw_bank { * @id_event: indicates there is a id event, and handled at ci_otg_work * @b_sess_valid_event: indicates there is a vbus event, and handled * at ci_otg_work + * @reg_vbus: used to control internal vbus regulator */ struct ci13xxx { struct device *dev; @@ -172,6 +173,7 @@ struct ci13xxx { struct usb_otg otg; bool id_event; bool b_sess_valid_event; + struct regulator *reg_vbus; }; static inline struct ci_role_driver *ci_role(struct ci13xxx *ci) diff --git a/drivers/usb/chipidea/ci13xxx_imx.c b/drivers/usb/chipidea/ci13xxx_imx.c index 3ed119e..5499cf8 100644 --- a/drivers/usb/chipidea/ci13xxx_imx.c +++ b/drivers/usb/chipidea/ci13xxx_imx.c @@ -85,11 +85,43 @@ EXPORT_SYMBOL_GPL(usbmisc_get_init_data); /* End of common functions shared by usbmisc drivers*/ +static int ci13xxx_otg_set_vbus(struct usb_otg *otg, bool enabled) +{ + struct ci13xxx *ci = container_of(otg, struct ci13xxx, otg); + struct regulator *reg_vbus = ci->reg_vbus; + int ret; + + WARN_ON(!reg_vbus); + + if (reg_vbus) { + if (enabled) { + ret = regulator_enable(reg_vbus); + if (ret) { + dev_err(ci->dev, + "Failed to enable vbus regulator, ret=%d\n", + ret); + return ret; + } + } else { + ret = regulator_disable(reg_vbus); + if (ret) { + dev_err(ci->dev, + "Failed to disable vbus regulator, ret=%d\n", + ret); + return ret; + } + } + } + + return 0; +} + static int ci13xxx_imx_probe(struct platform_device *pdev) { struct ci13xxx_imx_data *data; struct ci13xxx_platform_data *pdata; struct platform_device *plat_ci; + struct ci13xxx *ci; struct resource *res; struct regulator *reg_vbus; struct pinctrl *pinctrl; @@ -163,20 +195,11 @@ static int ci13xxx_imx_probe(struct platform_device *pdev) data->phy = phy; } - /* we only support host now, so enable vbus here */ reg_vbus = devm_regulator_get(&pdev->dev, "vbus"); - if (!IS_ERR(reg_vbus)) { - ret = regulator_enable(reg_vbus); - if (ret) { - dev_err(&pdev->dev, - "Failed to enable vbus regulator, err=%d\n", - ret); - goto err_clk; - } + if (!IS_ERR(reg_vbus)) data->reg_vbus = reg_vbus; - } else { + else reg_vbus = NULL; - } pdata->phy = data->phy; @@ -186,7 +209,7 @@ static int ci13xxx_imx_probe(struct platform_device *pdev) if (!pdev->dev.dma_mask) { ret = -ENOMEM; dev_err(&pdev->dev, "Failed to alloc dma_mask!\n"); - goto err; + goto err_clk; } *pdev->dev.dma_mask = DMA_BIT_MASK(32); dma_set_coherent_mask(&pdev->dev, *pdev->dev.dma_mask); @@ -197,7 +220,7 @@ static int ci13xxx_imx_probe(struct platform_device *pdev) if (ret) { dev_err(&pdev->dev, "usbmisc init failed, ret=%d\n", ret); - goto err; + goto err_clk; } } @@ -209,20 +232,39 @@ static int ci13xxx_imx_probe(struct platform_device *pdev) dev_err(&pdev->dev, "Can't register ci_hdrc platform device, err=%d\n", ret); - goto err; + goto err_clk; } data->ci_pdev = plat_ci; platform_set_drvdata(pdev, data); + ci = platform_get_drvdata(plat_ci); + /* + * Internal vbus on/off policy + * - Always on for host only function + * - Always off for gadget only function + * - call otg.set_vbus to control on/off according usb role + */ + + if (ci->roles[CI_ROLE_HOST] && !ci->roles[CI_ROLE_GADGET] + && reg_vbus) { + ret = regulator_enable(reg_vbus); + if (ret) { + dev_err(&pdev->dev, + "Failed to enable vbus regulator, ret=%d\n", + ret); + goto err_clk; + } + } else if (ci->is_otg) { + ci->otg.set_vbus = ci13xxx_otg_set_vbus; + ci->reg_vbus = data->reg_vbus; + } + pm_runtime_no_callbacks(&pdev->dev); pm_runtime_enable(&pdev->dev); return 0; -err: - if (reg_vbus) - regulator_disable(reg_vbus); err_clk: clk_disable_unprepare(data->clk); return ret;
- For host, the vbus should always be on. - For otg, the vbus is off defaultly, the vbus needs to be turned on/off when usb role switches. Signed-off-by: Peter Chen <peter.chen@freescale.com> --- drivers/usb/chipidea/ci.h | 2 + drivers/usb/chipidea/ci13xxx_imx.c | 76 ++++++++++++++++++++++++++++-------- 2 files changed, 61 insertions(+), 17 deletions(-)