| Message ID | 1373524041-10482-7-git-send-email-peter.chen@freescale.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Dear Peter Chen, > Since we need otgsc to know vbus's status at some chipidea > controllers even it is peripheral-only mode. Besides, some > SoCs (eg, AR9331 SoC) don't have otgsc register even > the DCCPARAMS_DC and DCCPARAMS_HC are both 1 at CAP_DCCPARAMS. > We inroduce otg_cap attribute to indicate if the controller > is otg capable, defaultly, we follow the rule that if DCCPARAMS_DC > and DCCPARAMS_HC are both 1 at CAP_DCCPARAMS are otg capable, but if there > is exception, the platform can override it by device tree or platform data. > > Signed-off-by: Peter Chen <peter.chen@freescale.com> > --- > drivers/usb/chipidea/core.c | 35 ++++++++++++++++++++++++++++------- > include/linux/usb/chipidea.h | 13 +++++++++++++ > 2 files changed, 41 insertions(+), 7 deletions(-) > > diff --git a/drivers/usb/chipidea/core.c b/drivers/usb/chipidea/core.c > index 93961ff..e8ceb04 100644 > --- a/drivers/usb/chipidea/core.c > +++ b/drivers/usb/chipidea/core.c > @@ -405,6 +405,18 @@ static inline void ci_role_destroy(struct ci_hdrc *ci) > ci_hdrc_host_destroy(ci); > } > > +static void ci_get_otg_capable(struct ci_hdrc *ci) > +{ > + if (ci->platdata->otg_cap != OTG_CAP_ATTR_IS_NOT_EXISTED) IS_NONEXISTENT , no? [..] Best regards, Marek Vasut
On Thu, Jul 11, 2013 at 05:36:10PM +0200, Marek Vasut wrote: > Dear Peter Chen, > > > Since we need otgsc to know vbus's status at some chipidea > > controllers even it is peripheral-only mode. Besides, some > > SoCs (eg, AR9331 SoC) don't have otgsc register even > > the DCCPARAMS_DC and DCCPARAMS_HC are both 1 at CAP_DCCPARAMS. > > We inroduce otg_cap attribute to indicate if the controller > > is otg capable, defaultly, we follow the rule that if DCCPARAMS_DC > > and DCCPARAMS_HC are both 1 at CAP_DCCPARAMS are otg capable, but if there > > is exception, the platform can override it by device tree or platform data. > > > > Signed-off-by: Peter Chen <peter.chen@freescale.com> > > --- > > drivers/usb/chipidea/core.c | 35 ++++++++++++++++++++++++++++------- > > include/linux/usb/chipidea.h | 13 +++++++++++++ > > 2 files changed, 41 insertions(+), 7 deletions(-) > > > > diff --git a/drivers/usb/chipidea/core.c b/drivers/usb/chipidea/core.c > > index 93961ff..e8ceb04 100644 > > --- a/drivers/usb/chipidea/core.c > > +++ b/drivers/usb/chipidea/core.c > > @@ -405,6 +405,18 @@ static inline void ci_role_destroy(struct ci_hdrc *ci) > > ci_hdrc_host_destroy(ci); > > } > > > > +static void ci_get_otg_capable(struct ci_hdrc *ci) > > +{ > > + if (ci->platdata->otg_cap != OTG_CAP_ATTR_IS_NOT_EXISTED) > > IS_NONEXISTENT , no? > You mean the English for this string? ok, I can change. It means if the "otg_cap" is not existed at platform data, then, the "otg_cap" can be determined by DCCPARAMS
Dear Peter Chen, > On Thu, Jul 11, 2013 at 05:36:10PM +0200, Marek Vasut wrote: > > Dear Peter Chen, > > > > > Since we need otgsc to know vbus's status at some chipidea > > > controllers even it is peripheral-only mode. Besides, some > > > SoCs (eg, AR9331 SoC) don't have otgsc register even > > > the DCCPARAMS_DC and DCCPARAMS_HC are both 1 at CAP_DCCPARAMS. > > > We inroduce otg_cap attribute to indicate if the controller > > > is otg capable, defaultly, we follow the rule that if DCCPARAMS_DC > > > and DCCPARAMS_HC are both 1 at CAP_DCCPARAMS are otg capable, but if > > > there is exception, the platform can override it by device tree or > > > platform data. > > > > > > Signed-off-by: Peter Chen <peter.chen@freescale.com> > > > --- > > > > > > drivers/usb/chipidea/core.c | 35 > > > ++++++++++++++++++++++++++++------- include/linux/usb/chipidea.h | > > > 13 +++++++++++++ > > > 2 files changed, 41 insertions(+), 7 deletions(-) > > > > > > diff --git a/drivers/usb/chipidea/core.c b/drivers/usb/chipidea/core.c > > > index 93961ff..e8ceb04 100644 > > > --- a/drivers/usb/chipidea/core.c > > > +++ b/drivers/usb/chipidea/core.c > > > @@ -405,6 +405,18 @@ static inline void ci_role_destroy(struct ci_hdrc > > > *ci) > > > > > > ci_hdrc_host_destroy(ci); > > > > > > } > > > > > > +static void ci_get_otg_capable(struct ci_hdrc *ci) > > > +{ > > > + if (ci->platdata->otg_cap != OTG_CAP_ATTR_IS_NOT_EXISTED) > > > > IS_NONEXISTENT , no? > > You mean the English for this string? ok, I can change. Yes, IS_NOT_EXISTED doesn't make much sense. Sorry, don't mean to nitpick. > It means if the "otg_cap" is not existed at platform data, > then, the "otg_cap" can be determined by DCCPARAMS Then maybe just call it OTG_CAP_ATTR_NOT_SET or something . Best regards, Marek Vasut
Peter Chen <peter.chen@freescale.com> writes: > Since we need otgsc to know vbus's status at some chipidea > controllers even it is peripheral-only mode. Besides, some > SoCs (eg, AR9331 SoC) don't have otgsc register even > the DCCPARAMS_DC and DCCPARAMS_HC are both 1 at CAP_DCCPARAMS. > We inroduce otg_cap attribute to indicate if the controller > is otg capable, defaultly, we follow the rule that if DCCPARAMS_DC > and DCCPARAMS_HC are both 1 at CAP_DCCPARAMS are otg capable, but if there > is exception, the platform can override it by device tree or platform data. > > Signed-off-by: Peter Chen <peter.chen@freescale.com> > --- [...] > diff --git a/include/linux/usb/chipidea.h b/include/linux/usb/chipidea.h > index 118bf66..0a906b4 100644 > --- a/include/linux/usb/chipidea.h > +++ b/include/linux/usb/chipidea.h > @@ -7,6 +7,12 @@ > > #include <linux/usb/otg.h> > > +enum usb_otg_cap { > + OTG_CAP_ATTR_IS_NOT_EXISTED = 0, > + OTG_CAP_ATTR_IS_TRUE, > + OTG_CAP_ATTR_IS_FALSE, > +}; We don't really need all three, do we? We only need to know if the controller *can't* do otg. The rest we can infer from dr_mode and DCCPARAMS. So it can be another bit in flags rather than a separate field in platdata. Regards, -- Alex
On Fri, Jul 12, 2013 at 11:12:01AM +0300, Alexander Shishkin wrote: > Peter Chen <peter.chen@freescale.com> writes: > > > Since we need otgsc to know vbus's status at some chipidea > > controllers even it is peripheral-only mode. Besides, some > > SoCs (eg, AR9331 SoC) don't have otgsc register even > > the DCCPARAMS_DC and DCCPARAMS_HC are both 1 at CAP_DCCPARAMS. > > We inroduce otg_cap attribute to indicate if the controller > > is otg capable, defaultly, we follow the rule that if DCCPARAMS_DC > > and DCCPARAMS_HC are both 1 at CAP_DCCPARAMS are otg capable, but if there > > is exception, the platform can override it by device tree or platform data. > > > > Signed-off-by: Peter Chen <peter.chen@freescale.com> > > --- > > [...] > > > diff --git a/include/linux/usb/chipidea.h b/include/linux/usb/chipidea.h > > index 118bf66..0a906b4 100644 > > --- a/include/linux/usb/chipidea.h > > +++ b/include/linux/usb/chipidea.h > > @@ -7,6 +7,12 @@ > > > > #include <linux/usb/otg.h> > > > > +enum usb_otg_cap { > > + OTG_CAP_ATTR_IS_NOT_EXISTED = 0, > > + OTG_CAP_ATTR_IS_TRUE, > > + OTG_CAP_ATTR_IS_FALSE, > > +}; > > We don't really need all three, do we? We only need to know if the > controller *can't* do otg. The rest we can infer from dr_mode and > DCCPARAMS. So it can be another bit in flags rather than a separate > field in platdata. > In order to cover below two cases, I have no other idea. 1. For mips Soc (ARxxx), if dr_mode = otg and it is 1 for both mode at DCCPARAMS, It can switch role through proc or whatever, but NO otgsc register, it is NOT otg-capable. 2. For peripheral-only case at most chipidea controllers, the dr_mode = peripheral and it is 1 for both modes at DCCPARAMS, the otgsc can be visited, it is otg-capable.
Peter Chen <peter.chen@freescale.com> writes: > On Fri, Jul 12, 2013 at 11:12:01AM +0300, Alexander Shishkin wrote: >> Peter Chen <peter.chen@freescale.com> writes: >> >> > Since we need otgsc to know vbus's status at some chipidea >> > controllers even it is peripheral-only mode. Besides, some >> > SoCs (eg, AR9331 SoC) don't have otgsc register even >> > the DCCPARAMS_DC and DCCPARAMS_HC are both 1 at CAP_DCCPARAMS. >> > We inroduce otg_cap attribute to indicate if the controller >> > is otg capable, defaultly, we follow the rule that if DCCPARAMS_DC >> > and DCCPARAMS_HC are both 1 at CAP_DCCPARAMS are otg capable, but if there >> > is exception, the platform can override it by device tree or platform data. >> > >> > Signed-off-by: Peter Chen <peter.chen@freescale.com> >> > --- >> >> [...] >> >> > diff --git a/include/linux/usb/chipidea.h b/include/linux/usb/chipidea.h >> > index 118bf66..0a906b4 100644 >> > --- a/include/linux/usb/chipidea.h >> > +++ b/include/linux/usb/chipidea.h >> > @@ -7,6 +7,12 @@ >> > >> > #include <linux/usb/otg.h> >> > >> > +enum usb_otg_cap { >> > + OTG_CAP_ATTR_IS_NOT_EXISTED = 0, >> > + OTG_CAP_ATTR_IS_TRUE, >> > + OTG_CAP_ATTR_IS_FALSE, >> > +}; >> >> We don't really need all three, do we? We only need to know if the >> controller *can't* do otg. The rest we can infer from dr_mode and >> DCCPARAMS. So it can be another bit in flags rather than a separate >> field in platdata. >> > > In order to cover below two cases, I have no other idea. > > 1. For mips Soc (ARxxx), if dr_mode = otg and it is 1 for both > mode at DCCPARAMS, It can switch role through proc or whatever, > but NO otgsc register, it is NOT otg-capable. For this case, platform should set platdata->flags &= CI_HDRC_NO_OTG , which will set ci->is_otg = false. No ci->is_otg => no touching OTGSC. > 2. For peripheral-only case at most chipidea controllers, > the dr_mode = peripheral and it is 1 for both modes at DCCPARAMS, > the otgsc can be visited, it is otg-capable. Then platform *doesn't* set CI_HDRC_NO_OTG and ci->is_otg is set to true, because DCCPARAMS.DC==1 and DCCPARAMS.HC==1, but we only have one role initialized, because of dr_mode==peripheral, so no role switching happens, but OTGSC accesses are fine. Makes sense? Regards, -- Alex
On Fri, Jul 12, 2013 at 12:42:10PM +0300, Alexander Shishkin wrote: > Peter Chen <peter.chen@freescale.com> writes: > > > On Fri, Jul 12, 2013 at 11:12:01AM +0300, Alexander Shishkin wrote: > >> Peter Chen <peter.chen@freescale.com> writes: > >> > >> > Since we need otgsc to know vbus's status at some chipidea > >> > controllers even it is peripheral-only mode. Besides, some > >> > SoCs (eg, AR9331 SoC) don't have otgsc register even > >> > the DCCPARAMS_DC and DCCPARAMS_HC are both 1 at CAP_DCCPARAMS. > >> > We inroduce otg_cap attribute to indicate if the controller > >> > is otg capable, defaultly, we follow the rule that if DCCPARAMS_DC > >> > and DCCPARAMS_HC are both 1 at CAP_DCCPARAMS are otg capable, but if there > >> > is exception, the platform can override it by device tree or platform data. > >> > > >> > Signed-off-by: Peter Chen <peter.chen@freescale.com> > >> > --- > >> > >> [...] > >> > >> > diff --git a/include/linux/usb/chipidea.h b/include/linux/usb/chipidea.h > >> > index 118bf66..0a906b4 100644 > >> > --- a/include/linux/usb/chipidea.h > >> > +++ b/include/linux/usb/chipidea.h > >> > @@ -7,6 +7,12 @@ > >> > > >> > #include <linux/usb/otg.h> > >> > > >> > +enum usb_otg_cap { > >> > + OTG_CAP_ATTR_IS_NOT_EXISTED = 0, > >> > + OTG_CAP_ATTR_IS_TRUE, > >> > + OTG_CAP_ATTR_IS_FALSE, > >> > +}; > >> > >> We don't really need all three, do we? We only need to know if the > >> controller *can't* do otg. The rest we can infer from dr_mode and > >> DCCPARAMS. So it can be another bit in flags rather than a separate > >> field in platdata. > >> > > > > In order to cover below two cases, I have no other idea. > > > > 1. For mips Soc (ARxxx), if dr_mode = otg and it is 1 for both > > mode at DCCPARAMS, It can switch role through proc or whatever, > > but NO otgsc register, it is NOT otg-capable. > > For this case, platform should set > > platdata->flags &= CI_HDRC_NO_OTG > > , which will set ci->is_otg = false. No ci->is_otg => no touching > OTGSC. > > > 2. For peripheral-only case at most chipidea controllers, > > the dr_mode = peripheral and it is 1 for both modes at DCCPARAMS, > > the otgsc can be visited, it is otg-capable. > > Then platform *doesn't* set CI_HDRC_NO_OTG and ci->is_otg is set to > true, because DCCPARAMS.DC==1 and DCCPARAMS.HC==1, but we only have one > role initialized, because of dr_mode==peripheral, so no role switching > happens, but OTGSC accesses are fine. > > Makes sense? > Agree, I will add the define of CI_HDRC_NO_OTG to struct ci_hdrc_platform_data, and add below comments /* Only set it when DCCPARAMS.DC==1 and DCCPARAMS.HC==1, * but otg is not supported (no register otgsc). */
On Fri, Jul 12, 2013 at 12:42:10PM +0300, Alexander Shishkin wrote: > Peter Chen <peter.chen@freescale.com> writes: > Hi Alex, do you have any other comments for this patchset? If you haven't, I will send the v13 patchset with current comments.
diff --git a/drivers/usb/chipidea/core.c b/drivers/usb/chipidea/core.c index 93961ff..e8ceb04 100644 --- a/drivers/usb/chipidea/core.c +++ b/drivers/usb/chipidea/core.c @@ -405,6 +405,18 @@ static inline void ci_role_destroy(struct ci_hdrc *ci) ci_hdrc_host_destroy(ci); } +static void ci_get_otg_capable(struct ci_hdrc *ci) +{ + if (ci->platdata->otg_cap != OTG_CAP_ATTR_IS_NOT_EXISTED) + ci->is_otg = (ci->platdata->otg_cap == OTG_CAP_ATTR_IS_TRUE); + else + ci->is_otg = (hw_read(ci, CAP_DCCPARAMS, + DCCPARAMS_DC | DCCPARAMS_HC) + == (DCCPARAMS_DC | DCCPARAMS_HC)); + if (ci->is_otg) + dev_dbg(ci->dev, "It is OTG capable controller\n"); +} + static int ci_hdrc_probe(struct platform_device *pdev) { struct device *dev = &pdev->dev; @@ -461,6 +473,9 @@ static int ci_hdrc_probe(struct platform_device *pdev) return -ENODEV; } + /* To know if controller is OTG capable or not */ + ci_get_otg_capable(ci); + if (!ci->platdata->phy_mode) ci->platdata->phy_mode = of_usb_get_phy_mode(dev->of_node); @@ -491,10 +506,19 @@ static int ci_hdrc_probe(struct platform_device *pdev) } if (ci->roles[CI_ROLE_HOST] && ci->roles[CI_ROLE_GADGET]) { - ci->is_otg = true; - /* ID pin needs 1ms debouce time, we delay 2ms for safe */ - mdelay(2); - ci->role = ci_otg_role(ci); + if (ci->is_otg) { + /* ID pin needs 1ms debouce time, we delay 2ms for safe */ + mdelay(2); + ci->role = ci_otg_role(ci); + ci_hdrc_otg_init(ci); + } else { + /* + * If the controller is not OTG capable, but support + * role switch, the defalt role is gadget, and the + * user can switch it through debugfs (proc in future?) + */ + ci->role = CI_ROLE_GADGET; + } } else { ci->role = ci->roles[CI_ROLE_HOST] ? CI_ROLE_HOST @@ -514,9 +538,6 @@ static int ci_hdrc_probe(struct platform_device *pdev) if (ret) goto stop; - if (ci->is_otg) - ci_hdrc_otg_init(ci); - ret = dbg_create_files(ci); if (!ret) return 0; diff --git a/include/linux/usb/chipidea.h b/include/linux/usb/chipidea.h index 118bf66..0a906b4 100644 --- a/include/linux/usb/chipidea.h +++ b/include/linux/usb/chipidea.h @@ -7,6 +7,12 @@ #include <linux/usb/otg.h> +enum usb_otg_cap { + OTG_CAP_ATTR_IS_NOT_EXISTED = 0, + OTG_CAP_ATTR_IS_TRUE, + OTG_CAP_ATTR_IS_FALSE, +}; + struct ci_hdrc; struct ci_hdrc_platform_data { const char *name; @@ -25,6 +31,13 @@ struct ci_hdrc_platform_data { #define CI_HDRC_CONTROLLER_STOPPED_EVENT 1 void (*notify_event) (struct ci_hdrc *ci, unsigned event); struct regulator *reg_vbus; + /* + * Please only set otg_cap when the otg capability can't be + * judged by CAP_DCCPARAMS, eg, the DCCPARAMS_DC and DCCPARAMS_HC + * are both 1 at CAP_DCCPARAMS, but the controller doesn't have + * OTGSC register (eg, AR9331 SoC). + */ + enum usb_otg_cap otg_cap; }; /* Default offset of capability registers */
Since we need otgsc to know vbus's status at some chipidea controllers even it is peripheral-only mode. Besides, some SoCs (eg, AR9331 SoC) don't have otgsc register even the DCCPARAMS_DC and DCCPARAMS_HC are both 1 at CAP_DCCPARAMS. We inroduce otg_cap attribute to indicate if the controller is otg capable, defaultly, we follow the rule that if DCCPARAMS_DC and DCCPARAMS_HC are both 1 at CAP_DCCPARAMS are otg capable, but if there is exception, the platform can override it by device tree or platform data. Signed-off-by: Peter Chen <peter.chen@freescale.com> --- drivers/usb/chipidea/core.c | 35 ++++++++++++++++++++++++++++------- include/linux/usb/chipidea.h | 13 +++++++++++++ 2 files changed, 41 insertions(+), 7 deletions(-)