| Message ID | 1362563800-16673-7-git-send-email-peter.chen@freescale.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi, On Wed, Mar 06, 2013 at 05:56:37PM +0800, Peter Chen wrote: > For boards which have board level vbus control (eg, gpio), we > need to operation vbus according to below rules: > - For host, the vbus should always be on. > - For otg, the vbus needs to be turned on/off when usb role switches. > > We put vbus operation to host as host is the only vbus user, > When we are at host mode, the vbus is on, when we are not at > host mode, vbus should be off. > > Signed-off-by: Peter Chen <peter.chen@freescale.com> > --- > drivers/usb/chipidea/ci.h | 2 ++ > drivers/usb/chipidea/core.c | 24 ++++++++---------------- > drivers/usb/chipidea/host.c | 23 ++++++++++++++++++++++- > include/linux/usb/chipidea.h | 1 + > 4 files changed, 33 insertions(+), 17 deletions(-) > > diff --git a/drivers/usb/chipidea/ci.h b/drivers/usb/chipidea/ci.h > index a3777a1..8826cdb 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/core.c b/drivers/usb/chipidea/core.c > index c563e17..e0ff335 100644 > --- a/drivers/usb/chipidea/core.c > +++ b/drivers/usb/chipidea/core.c > @@ -63,6 +63,7 @@ > #include <linux/kernel.h> > #include <linux/slab.h> > #include <linux/pm_runtime.h> > +#include <linux/regulator/consumer.h> > #include <linux/usb/ch9.h> > #include <linux/usb/gadget.h> > #include <linux/usb/otg.h> > @@ -364,17 +365,10 @@ static void ci_handle_id_switch(struct ci13xxx *ci) > hw_device_reset(ci, USBMODE_CM_IDLE); > > /* 2. Turn on/off vbus according to coming role */ > - if (role == CI_ROLE_GADGET) { > - otg_set_vbus(&ci->otg, false); > + if (role == CI_ROLE_GADGET) > /* wait vbus lower than OTGSC_BSV */ > hw_wait_reg(ci, OP_OTGSC, OTGSC_BSV, 0, > CI_VBUS_STABLE_TIMEOUT); > - } else if (role == CI_ROLE_HOST) { > - otg_set_vbus(&ci->otg, true); > - /* wait vbus higher than OTGSC_AVV */ > - hw_wait_reg(ci, OP_OTGSC, OTGSC_AVV, OTGSC_AVV, > - CI_VBUS_STABLE_TIMEOUT); > - } > > /* 3. Begin the new role */ > ci_role_start(ci, role); > @@ -416,17 +410,14 @@ static void ci_delayed_work(struct work_struct *work) > struct delayed_work *dwork = to_delayed_work(work); > struct ci13xxx *ci = container_of(dwork, struct ci13xxx, dwork); > > + /* > + * If it is gadget mode, the vbus operation should be done like below: > + * 1. Enable vbus detect > + * 2. If it has already connected to host, notify udc > + */ > if (ci->role == CI_ROLE_GADGET) { > - /* > - * if it is device mode: > - * - Enable vbus detect > - * - If it has already connected to host, notify udc > - */ > ci_enable_otg_interrupt(ci, OTGSC_BSVIE); > ci_handle_vbus_change(ci); > - } else if (ci->is_otg && (ci->role == CI_ROLE_HOST)) { > - /* USB Device at the MicroB to A cable */ > - otg_set_vbus(&ci->otg, true); > } > } > > @@ -603,6 +594,7 @@ static int ci_hdrc_probe(struct platform_device *pdev) > > ci->dev = dev; > ci->platdata = dev->platform_data; > + ci->reg_vbus = ci->platdata->reg_vbus; nak, teach ci_hdrc_probe() how to get its own regulator. > if (ci->platdata->phy) > ci->transceiver = ci->platdata->phy; this should happen for PHYs as well btw.
On Wed, Mar 06, 2013 at 01:29:16PM +0200, Felipe Balbi wrote: > Hi, > > On Wed, Mar 06, 2013 at 05:56:37PM +0800, Peter Chen wrote: > > For boards which have board level vbus control (eg, gpio), we > > need to operation vbus according to below rules: > > - For host, the vbus should always be on. > > - For otg, the vbus needs to be turned on/off when usb role switches. > > > > We put vbus operation to host as host is the only vbus user, > > When we are at host mode, the vbus is on, when we are not at > > host mode, vbus should be off. > > > > Signed-off-by: Peter Chen <peter.chen@freescale.com> > > --- > > > > @@ -603,6 +594,7 @@ static int ci_hdrc_probe(struct platform_device *pdev) > > > > ci->dev = dev; > > ci->platdata = dev->platform_data; > > + ci->reg_vbus = ci->platdata->reg_vbus; > > nak, teach ci_hdrc_probe() how to get its own regulator. > > > if (ci->platdata->phy) > > ci->transceiver = ci->platdata->phy; > > this should happen for PHYs as well btw. Like I said at previous email, core code has NO DTS entry. > > -- > balbi
On Thu, Mar 07, 2013 at 10:41:03AM +0800, Peter Chen wrote: > On Wed, Mar 06, 2013 at 01:29:16PM +0200, Felipe Balbi wrote: > > Hi, > > > > On Wed, Mar 06, 2013 at 05:56:37PM +0800, Peter Chen wrote: > > > For boards which have board level vbus control (eg, gpio), we > > > need to operation vbus according to below rules: > > > - For host, the vbus should always be on. > > > - For otg, the vbus needs to be turned on/off when usb role switches. > > > > > > We put vbus operation to host as host is the only vbus user, > > > When we are at host mode, the vbus is on, when we are not at > > > host mode, vbus should be off. > > > > > > Signed-off-by: Peter Chen <peter.chen@freescale.com> > > > --- > > > > > > @@ -603,6 +594,7 @@ static int ci_hdrc_probe(struct platform_device *pdev) > > > > > > ci->dev = dev; > > > ci->platdata = dev->platform_data; > > > + ci->reg_vbus = ci->platdata->reg_vbus; > > > > nak, teach ci_hdrc_probe() how to get its own regulator. > > > > > if (ci->platdata->phy) > > > ci->transceiver = ci->platdata->phy; > > > > this should happen for PHYs as well btw. > > Like I said at previous email, core code has NO DTS entry. that's your problem :-) You can just as well create both nodes in DTS file: ci13xx_fsl@foo { compatible = "fsl,chipidea"; reg = <foo size>; blablabla ranges; chipidea_core@bar { compatible = "chipidea"; reg <bar size>; blablabla }; }; then assign the regulator to chipidea itself, not to the glue.
On Thu, Mar 07, 2013 at 11:52:48AM +0200, Felipe Balbi wrote: > On Thu, Mar 07, 2013 at 10:41:03AM +0800, Peter Chen wrote: > > On Wed, Mar 06, 2013 at 01:29:16PM +0200, Felipe Balbi wrote: > > > Hi, > > > > > > On Wed, Mar 06, 2013 at 05:56:37PM +0800, Peter Chen wrote: > > > > For boards which have board level vbus control (eg, gpio), we > > > > need to operation vbus according to below rules: > > > > - For host, the vbus should always be on. > > > > - For otg, the vbus needs to be turned on/off when usb role switches. > > > > > > > > We put vbus operation to host as host is the only vbus user, > > > > When we are at host mode, the vbus is on, when we are not at > > > > host mode, vbus should be off. > > > > > > > > Signed-off-by: Peter Chen <peter.chen@freescale.com> > > > > --- > > > > > > > > @@ -603,6 +594,7 @@ static int ci_hdrc_probe(struct platform_device *pdev) > > > > > > > > ci->dev = dev; > > > > ci->platdata = dev->platform_data; > > > > + ci->reg_vbus = ci->platdata->reg_vbus; > > > > > > nak, teach ci_hdrc_probe() how to get its own regulator. > > > > > > > if (ci->platdata->phy) > > > > ci->transceiver = ci->platdata->phy; > > > > > > this should happen for PHYs as well btw. > > > > Like I said at previous email, core code has NO DTS entry. > > that's your problem :-) > > You can just as well create both nodes in DTS file: > > ci13xx_fsl@foo { > compatible = "fsl,chipidea"; > reg = <foo size>; > blablabla > ranges; > > chipidea_core@bar { > compatible = "chipidea"; > reg <bar size>; > blablabla > }; > }; > > then assign the regulator to chipidea itself, not to the glue. No, that will make things more complicated at current chipidea driver. Differ with dwc3, chipidea created its platform data at glue layer, and chipidea core owing this data when platform device is created. If we also want get things from DT node, we need to get node from glue layer as this node is NOT belonged to chipidea core's pdev. (We can't get it through compatible string as compatible string is the same from every chipidea device) Convert platform data to DT for chipidea core is a big job, it is out of scope of this patch. If Alex also thinks we are deserved to do it, I can do it later. > > -- > balbi
Hi, On Fri, Mar 08, 2013 at 02:27:58PM +0800, Peter Chen wrote: > > > > > For boards which have board level vbus control (eg, gpio), we > > > > > need to operation vbus according to below rules: > > > > > - For host, the vbus should always be on. > > > > > - For otg, the vbus needs to be turned on/off when usb role switches. > > > > > > > > > > We put vbus operation to host as host is the only vbus user, > > > > > When we are at host mode, the vbus is on, when we are not at > > > > > host mode, vbus should be off. > > > > > > > > > > Signed-off-by: Peter Chen <peter.chen@freescale.com> > > > > > --- > > > > > > > > > > @@ -603,6 +594,7 @@ static int ci_hdrc_probe(struct platform_device *pdev) > > > > > > > > > > ci->dev = dev; > > > > > ci->platdata = dev->platform_data; > > > > > + ci->reg_vbus = ci->platdata->reg_vbus; > > > > > > > > nak, teach ci_hdrc_probe() how to get its own regulator. > > > > > > > > > if (ci->platdata->phy) > > > > > ci->transceiver = ci->platdata->phy; > > > > > > > > this should happen for PHYs as well btw. > > > > > > Like I said at previous email, core code has NO DTS entry. > > > > that's your problem :-) > > > > You can just as well create both nodes in DTS file: > > > > ci13xx_fsl@foo { > > compatible = "fsl,chipidea"; > > reg = <foo size>; > > blablabla > > ranges; > > > > chipidea_core@bar { > > compatible = "chipidea"; > > reg <bar size>; > > blablabla > > }; > > }; > > > > then assign the regulator to chipidea itself, not to the glue. > > No, that will make things more complicated at current chipidea driver. so what ? We're not here to choose the easy approach, we're here to choose the 'right' approach. If that means rewritting a bunch of chipidea core code, so be it. > Differ with dwc3, chipidea created its platform data at glue layer, > and chipidea core owing this data when platform device is created. yeah yeah, but that's just because chipidea core doesn't understand DT. It should take you no more than a day to teach it about DT, though. Not sure what you're complaining about. > If we also want get things from DT node, we need to get node from > glue layer as this node is NOT belonged to chipidea core's pdev. which DT node are you talking about ? you want chipidea core to have access to glue's DT node ? Are you insane ? Currently your regulator only belongs to glue because chipidea core doesn't know about DT, if you teach it about DT, then you move the regulator to chipidea core and it will not need access to glue's DT node. > (We can't get it through compatible string as compatible string > is the same from every chipidea device) you're not making sense. If you wanna add DT support for chipidea core, you need to come up with a compatible that makes sense. It won't be fsl,* because chipidea core doesn't belong to fsl. > Convert platform data to DT for chipidea core is a big job, no it's not. > it is out of scope of this patch. If Alex also thinks we yeah, but this patch makes no sense. The right thing (TM) to do is, as I have said multiple times before, to teach chipidea core about DT. Please stop trying to find excuses for not doing the work you need to do; You would've already done it if you had spent so much of everybody's time trying to find excuses why not to do it and why Alex should accept your broken patches. Have you already forgotten how many issues were found in each and every version of your patchset ? There's a reason why we're revieweing v11, right ?
On Fri, Mar 08, 2013 at 09:26:24AM +0200, Felipe Balbi wrote: > > > Differ with dwc3, chipidea created its platform data at glue layer, > > and chipidea core owing this data when platform device is created. > > yeah yeah, but that's just because chipidea core doesn't understand DT. > > It should take you no more than a day to teach it about DT, though. Not > sure what you're complaining about. I have not complained about it, it is out of scope of this patchset. This patchset is just adds ID/vbus support for chipidea. For other enhancements, it needs another patchset. > > > If we also want get things from DT node, we need to get node from > > glue layer as this node is NOT belonged to chipidea core's pdev. > > which DT node are you talking about ? you want chipidea core to have > access to glue's DT node ? Are you insane ? Please read chipidea code, the chipidea core (pdev & platform data) is not created by DT, it is created by glue layer. If we added dts entry like you showed me, we can't get node by pdev->dev.of_node. > > If you wanna add DT support for chipidea core, you need to come up with > a compatible that makes sense. It won't be fsl,* because chipidea core > doesn't belong to fsl. I know it. > > > Convert platform data to DT for chipidea core is a big job, > > no it's not. I need to change all msm and pci board related files. > > yeah, but this patch makes no sense. The right thing (TM) to do is, as I > have said multiple times before, to teach chipidea core about DT. Please > stop trying to find excuses for not doing the work you need to do; You > would've already done it if you had spent so much of everybody's time > trying to find excuses why not to do it and why Alex should accept your > broken patches. First let chipidea core know about DT is out of this patchset. Second, why you think it is a broken patch, it just follows current framework. Besides, if you have concerns about DT stuff for chipidea core, why not mention it when sasche added dr_mode and phy_mode at that time? > > Have you already forgotten how many issues were found in each and every > version of your patchset ? There's a reason why we're revieweing v11, > right ? I just want to limit this patchset to id/vbus enable feature, I admit I am less upstream experience, but if new requirements new features and new bugs always coming, things can't be ended up easily.
Hi, On Fri, Mar 08, 2013 at 04:32:46PM +0800, Peter Chen wrote: > > > If we also want get things from DT node, we need to get node from > > > glue layer as this node is NOT belonged to chipidea core's pdev. > > > > which DT node are you talking about ? you want chipidea core to have > > access to glue's DT node ? Are you insane ? > > Please read chipidea code, the chipidea core (pdev & platform data) > is not created by DT, it is created by glue layer. I know that, what I'm arguing is that instead of going through all loops and hoops as you're doing, make DTS create the pdev. > > > Convert platform data to DT for chipidea core is a big job, > > > > no it's not. > > I need to change all msm and pci board related files. that's just life. Or, you could meanwhile use OF_DEV_AUX_DATA() > > yeah, but this patch makes no sense. The right thing (TM) to do is, as I > > have said multiple times before, to teach chipidea core about DT. Please > > stop trying to find excuses for not doing the work you need to do; You > > would've already done it if you had spent so much of everybody's time > > trying to find excuses why not to do it and why Alex should accept your > > broken patches. > > First let chipidea core know about DT is out of this patchset. not entirely, since you need access to the regulator from chipidea core and your regulator bindings are all passed via DT. There's a clear dependency there. > Second, why you think it is a broken patch, it just follows current > framework. Besides, if you have concerns about DT stuff for chipidea > core, why not mention it when sasche added dr_mode and phy_mode at > that time? Well, people make mistakes, right ? That was my mistake, but it doesn't we have to let the mistake proliferate.
On Fri, Mar 08, 2013 at 10:42:35AM +0200, Felipe Balbi wrote: > > > Second, why you think it is a broken patch, it just follows current > > framework. Besides, if you have concerns about DT stuff for chipidea > > core, why not mention it when sasche added dr_mode and phy_mode at > > that time? > > Well, people make mistakes, right ? That was my mistake, but it doesn't > we have to let the mistake proliferate. > That means I need to post below two patches before this patchset can be accepted. - Let udc-core know vbus - Convert platform data to DT for chipidea core driver
On Fri, Mar 08, 2013 at 04:52:02PM +0800, Peter Chen wrote: > On Fri, Mar 08, 2013 at 10:42:35AM +0200, Felipe Balbi wrote: > > > > > Second, why you think it is a broken patch, it just follows current > > > framework. Besides, if you have concerns about DT stuff for chipidea > > > core, why not mention it when sasche added dr_mode and phy_mode at > > > that time? > > > > Well, people make mistakes, right ? That was my mistake, but it doesn't > > we have to let the mistake proliferate. > > > > That means I need to post below two patches before this patchset can > be accepted. > > - Let udc-core know vbus > - Convert platform data to DT for chipidea core driver right, ask for help if you need but don't try to deliver patches which are conceptually wrong.
diff --git a/drivers/usb/chipidea/ci.h b/drivers/usb/chipidea/ci.h index a3777a1..8826cdb 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/core.c b/drivers/usb/chipidea/core.c index c563e17..e0ff335 100644 --- a/drivers/usb/chipidea/core.c +++ b/drivers/usb/chipidea/core.c @@ -63,6 +63,7 @@ #include <linux/kernel.h> #include <linux/slab.h> #include <linux/pm_runtime.h> +#include <linux/regulator/consumer.h> #include <linux/usb/ch9.h> #include <linux/usb/gadget.h> #include <linux/usb/otg.h> @@ -364,17 +365,10 @@ static void ci_handle_id_switch(struct ci13xxx *ci) hw_device_reset(ci, USBMODE_CM_IDLE); /* 2. Turn on/off vbus according to coming role */ - if (role == CI_ROLE_GADGET) { - otg_set_vbus(&ci->otg, false); + if (role == CI_ROLE_GADGET) /* wait vbus lower than OTGSC_BSV */ hw_wait_reg(ci, OP_OTGSC, OTGSC_BSV, 0, CI_VBUS_STABLE_TIMEOUT); - } else if (role == CI_ROLE_HOST) { - otg_set_vbus(&ci->otg, true); - /* wait vbus higher than OTGSC_AVV */ - hw_wait_reg(ci, OP_OTGSC, OTGSC_AVV, OTGSC_AVV, - CI_VBUS_STABLE_TIMEOUT); - } /* 3. Begin the new role */ ci_role_start(ci, role); @@ -416,17 +410,14 @@ static void ci_delayed_work(struct work_struct *work) struct delayed_work *dwork = to_delayed_work(work); struct ci13xxx *ci = container_of(dwork, struct ci13xxx, dwork); + /* + * If it is gadget mode, the vbus operation should be done like below: + * 1. Enable vbus detect + * 2. If it has already connected to host, notify udc + */ if (ci->role == CI_ROLE_GADGET) { - /* - * if it is device mode: - * - Enable vbus detect - * - If it has already connected to host, notify udc - */ ci_enable_otg_interrupt(ci, OTGSC_BSVIE); ci_handle_vbus_change(ci); - } else if (ci->is_otg && (ci->role == CI_ROLE_HOST)) { - /* USB Device at the MicroB to A cable */ - otg_set_vbus(&ci->otg, true); } } @@ -603,6 +594,7 @@ static int ci_hdrc_probe(struct platform_device *pdev) ci->dev = dev; ci->platdata = dev->platform_data; + ci->reg_vbus = ci->platdata->reg_vbus; if (ci->platdata->phy) ci->transceiver = ci->platdata->phy; else diff --git a/drivers/usb/chipidea/host.c b/drivers/usb/chipidea/host.c index ead3158..8efbd44 100644 --- a/drivers/usb/chipidea/host.c +++ b/drivers/usb/chipidea/host.c @@ -21,6 +21,7 @@ #include <linux/kernel.h> #include <linux/io.h> +#include <linux/regulator/consumer.h> #include <linux/usb.h> #include <linux/usb/hcd.h> #include <linux/usb/chipidea.h> @@ -64,9 +65,19 @@ static int host_start(struct ci13xxx *ci) ehci->caps = ci->hw_bank.cap; ehci->has_hostpc = ci->hw_bank.lpm; + if (ci->reg_vbus) { + ret = regulator_enable(ci->reg_vbus); + if (ret) { + dev_err(ci->dev, + "Failed to enable vbus regulator, ret=%d\n", + ret); + goto put_hcd; + } + } + ret = usb_add_hcd(hcd, 0, 0); if (ret) - usb_put_hcd(hcd); + goto disable_reg; else ci->hcd = hcd; @@ -74,6 +85,14 @@ static int host_start(struct ci13xxx *ci) hw_write(ci, OP_USBMODE, USBMODE_CI_SDIS, USBMODE_CI_SDIS); return ret; + +disable_reg: + if (ci->reg_vbus) + regulator_disable(ci->reg_vbus); +put_hcd: + usb_put_hcd(hcd); + + return ret; } static void host_stop(struct ci13xxx *ci) @@ -82,6 +101,8 @@ static void host_stop(struct ci13xxx *ci) usb_remove_hcd(hcd); usb_put_hcd(hcd); + if (ci->reg_vbus) + regulator_disable(ci->reg_vbus); } int ci_hdrc_host_init(struct ci13xxx *ci) diff --git a/include/linux/usb/chipidea.h b/include/linux/usb/chipidea.h index d543e4a..1430403 100644 --- a/include/linux/usb/chipidea.h +++ b/include/linux/usb/chipidea.h @@ -23,6 +23,7 @@ struct ci13xxx_platform_data { #define CI13XXX_CONTROLLER_RESET_EVENT 0 #define CI13XXX_CONTROLLER_STOPPED_EVENT 1 void (*notify_event) (struct ci13xxx *ci, unsigned event); + struct regulator *reg_vbus; }; /* Default offset of capability registers */
For boards which have board level vbus control (eg, gpio), we need to operation vbus according to below rules: - For host, the vbus should always be on. - For otg, the vbus needs to be turned on/off when usb role switches. We put vbus operation to host as host is the only vbus user, When we are at host mode, the vbus is on, when we are not at host mode, vbus should be off. Signed-off-by: Peter Chen <peter.chen@freescale.com> --- drivers/usb/chipidea/ci.h | 2 ++ drivers/usb/chipidea/core.c | 24 ++++++++---------------- drivers/usb/chipidea/host.c | 23 ++++++++++++++++++++++- include/linux/usb/chipidea.h | 1 + 4 files changed, 33 insertions(+), 17 deletions(-)