| Message ID | 1415087402-26007-2-git-send-email-Peng.Fan@freescale.com |
|---|---|
| State | Changes Requested |
| Delegated to: | Stefano Babic |
| Headers | show |
On Tuesday, November 04, 2014 at 08:50:00 AM, Peng Fan wrote: > Include a weak function board_ehci_usb_mode to gives board code > a choice. What choice? > If the board want the otg port work in host mode but not > device mode, this should be handled. How? Also, isn't usb_phy_enable() supposed to do exactly this kind of selection between device and host mode ? > Signed-off-by: Peng Fan <Peng.Fan@freescale.com> > Signed-off-by: Ye Li <B37916@freescale.com> > --- > > Changes v2: > Introduce a new weak function to let board have a choice to decide which > mode to work at. > > drivers/usb/host/ehci-mx6.c | 7 +++++++ > 1 file changed, 7 insertions(+) > > diff --git a/drivers/usb/host/ehci-mx6.c b/drivers/usb/host/ehci-mx6.c > index 9ec5a0a..3662a80 100644 > --- a/drivers/usb/host/ehci-mx6.c > +++ b/drivers/usb/host/ehci-mx6.c > @@ -193,6 +193,11 @@ static void usb_oc_config(int index) > __raw_writel(val, ctrl); > } > > +int __weak board_ehci_usb_mode(int index, enum usb_init_type *type) > +{ > + return 0; > +} > + > int __weak board_ehci_hcd_init(int port) > { > return 0; > @@ -223,6 +228,8 @@ int ehci_hcd_init(int index, enum usb_init_type init, > usb_internal_phy_clock_gate(index, 1); > type = usb_phy_enable(index, ehci) ? USB_INIT_DEVICE : USB_INIT_HOST; > > + board_ehci_usb_mode(index, &type); > + > *hccr = (struct ehci_hccr *)((uint32_t)&ehci->caplength); > *hcor = (struct ehci_hcor *)((uint32_t)*hccr + > HC_LENGTH(ehci_readl(&(*hccr)->cr_capbase))); Best regards, Marek Vasut
在 11/4/2014 6:33 PM, Marek Vasut 写道: > On Tuesday, November 04, 2014 at 08:50:00 AM, Peng Fan wrote: >> Include a weak function board_ehci_usb_mode to gives board code >> a choice. > > What choice? > >> If the board want the otg port work in host mode but not >> device mode, this should be handled. > > How? > > Also, isn't usb_phy_enable() supposed to do exactly this kind of selection > between device and host mode ? In mx6sxsabresd board, there are two usb port, one used for otg, the other used for host. However they are connected to SOC USB controller otg1 core and otg2 core respectively. Like following: OTG1 CORE <----> board otg port OTG2 CORE <----> board host port However the board do not have ID pin set for board host port. If just use usb_phy_enable, the board host port will not work, because "type = usb_phy_enable(index, ehci) ? USB_INIT_DEVICE : USB_INIT_HOST;" will always set type with USB_INIT_DEVICE. Because i did not find way to handle this situation in board/freescale/mx6sxsabresd/mx6sxsabresd.c, add this function to let board level code handle handle 'type', if board level code want to set it's own 'type'. > >> Signed-off-by: Peng Fan <Peng.Fan@freescale.com> >> Signed-off-by: Ye Li <B37916@freescale.com> >> --- >> >> Changes v2: >> Introduce a new weak function to let board have a choice to decide which >> mode to work at. >> >> drivers/usb/host/ehci-mx6.c | 7 +++++++ >> 1 file changed, 7 insertions(+) >> >> diff --git a/drivers/usb/host/ehci-mx6.c b/drivers/usb/host/ehci-mx6.c >> index 9ec5a0a..3662a80 100644 >> --- a/drivers/usb/host/ehci-mx6.c >> +++ b/drivers/usb/host/ehci-mx6.c >> @@ -193,6 +193,11 @@ static void usb_oc_config(int index) >> __raw_writel(val, ctrl); >> } >> >> +int __weak board_ehci_usb_mode(int index, enum usb_init_type *type) >> +{ >> + return 0; >> +} >> + >> int __weak board_ehci_hcd_init(int port) >> { >> return 0; >> @@ -223,6 +228,8 @@ int ehci_hcd_init(int index, enum usb_init_type init, >> usb_internal_phy_clock_gate(index, 1); >> type = usb_phy_enable(index, ehci) ? USB_INIT_DEVICE : USB_INIT_HOST; >> >> + board_ehci_usb_mode(index, &type); >> + >> *hccr = (struct ehci_hccr *)((uint32_t)&ehci->caplength); >> *hcor = (struct ehci_hcor *)((uint32_t)*hccr + >> HC_LENGTH(ehci_readl(&(*hccr)->cr_capbase))); > > Best regards, > Marek Vasut > Regards, Peng.
On Tuesday, November 04, 2014 at 11:50:29 AM, Peng Fan wrote: > 在 11/4/2014 6:33 PM, Marek Vasut 写道: > > On Tuesday, November 04, 2014 at 08:50:00 AM, Peng Fan wrote: > >> Include a weak function board_ehci_usb_mode to gives board code > >> a choice. > > > > What choice? > > > >> If the board want the otg port work in host mode but not > >> device mode, this should be handled. > > > > How? > > > > Also, isn't usb_phy_enable() supposed to do exactly this kind of > > selection between device and host mode ? > > In mx6sxsabresd board, there are two usb port, one used for otg, the > other used for host. However they are connected to SOC USB controller > otg1 core and otg2 core respectively. Like following: > > OTG1 CORE <----> board otg port > OTG2 CORE <----> board host port > > However the board do not have ID pin set for board host port. If just > use usb_phy_enable, the board host port will not work, because > "type = usb_phy_enable(index, ehci) ? USB_INIT_DEVICE : USB_INIT_HOST;" > will always set type with USB_INIT_DEVICE. > > Because i did not find way to handle this situation in > board/freescale/mx6sxsabresd/mx6sxsabresd.c, add this function to let > board level code handle handle 'type', if board level code want to set > it's own 'type'. This part in usb_phy_enable() 163 return val & USBPHY_CTRL_OTG_ID; should be replaced by some kind of a board-specific callback then, with default implmentation being the above (reading the phy ctrl register). Best regards, Marek Vasut
Hello Peng, On 04-11-14 08:50, Peng Fan wrote: > Include a weak function board_ehci_usb_mode to gives board code > a choice. If the board want the otg port work in host mode but not > device mode, this should be handled. > > Signed-off-by: Peng Fan <Peng.Fan@freescale.com> > Signed-off-by: Ye Li <B37916@freescale.com> > --- > > Changes v2: > Introduce a new weak function to let board have a choice to decide which mode > to work at. > > drivers/usb/host/ehci-mx6.c | 7 +++++++ > 1 file changed, 7 insertions(+) > > diff --git a/drivers/usb/host/ehci-mx6.c b/drivers/usb/host/ehci-mx6.c > index 9ec5a0a..3662a80 100644 > --- a/drivers/usb/host/ehci-mx6.c > +++ b/drivers/usb/host/ehci-mx6.c > @@ -193,6 +193,11 @@ static void usb_oc_config(int index) > __raw_writel(val, ctrl); > } > > +int __weak board_ehci_usb_mode(int index, enum usb_init_type *type) > +{ > + return 0; > +} > + > int __weak board_ehci_hcd_init(int port) > { > return 0; > @@ -223,6 +228,8 @@ int ehci_hcd_init(int index, enum usb_init_type init, > usb_internal_phy_clock_gate(index, 1); > type = usb_phy_enable(index, ehci) ? USB_INIT_DEVICE : USB_INIT_HOST; > > + board_ehci_usb_mode(index, &type); > + > *hccr = (struct ehci_hccr *)((uint32_t)&ehci->caplength); > *hcor = (struct ehci_hcor *)((uint32_t)*hccr + > HC_LENGTH(ehci_readl(&(*hccr)->cr_capbase))); Can you add a prototype type as well and make sure it is included? Thanks, Jeroen
Hi Marek, 在 11/4/2014 7:01 PM, Marek Vasut 写道: > On Tuesday, November 04, 2014 at 11:50:29 AM, Peng Fan wrote: >> 在 11/4/2014 6:33 PM, Marek Vasut 写道: >>> On Tuesday, November 04, 2014 at 08:50:00 AM, Peng Fan wrote: >>>> Include a weak function board_ehci_usb_mode to gives board code >>>> a choice. >>> >>> What choice? >>> >>>> If the board want the otg port work in host mode but not >>>> device mode, this should be handled. >>> >>> How? >>> >>> Also, isn't usb_phy_enable() supposed to do exactly this kind of >>> selection between device and host mode ? >> >> In mx6sxsabresd board, there are two usb port, one used for otg, the >> other used for host. However they are connected to SOC USB controller >> otg1 core and otg2 core respectively. Like following: >> >> OTG1 CORE <----> board otg port >> OTG2 CORE <----> board host port >> >> However the board do not have ID pin set for board host port. If just >> use usb_phy_enable, the board host port will not work, because >> "type = usb_phy_enable(index, ehci) ? USB_INIT_DEVICE : USB_INIT_HOST;" >> will always set type with USB_INIT_DEVICE. >> >> Because i did not find way to handle this situation in >> board/freescale/mx6sxsabresd/mx6sxsabresd.c, add this function to let >> board level code handle handle 'type', if board level code want to set >> it's own 'type'. > > This part in usb_phy_enable() > > 163 return val & USBPHY_CTRL_OTG_ID; > > should be replaced by some kind of a board-specific callback then, with > default implmentation being the above (reading the phy ctrl register). > How about using the following piece of code? in ehci-mx6.c unsigned int __weak board_usb_phy_mode(int index, unsigned int val) { return val & USBPHY_CTRL_OTG_ID; } replace "return val & USBPHY_CTRL_OTG_ID;" using " return board_usb_phy_mode(index, val);" In board file, unsigned int board_usb_phy_mode(int index, unsigned int val) { if (index == 1) return 0; /* HOST */ else return 1; /* DEVICE */ } > Best regards, > Marek Vasut > Regards, Peng.
Hi Jeroen, 在 11/4/2014 7:40 PM, Jeroen Hofstee 写道: > Hello Peng, > > On 04-11-14 08:50, Peng Fan wrote: >> Include a weak function board_ehci_usb_mode to gives board code >> a choice. If the board want the otg port work in host mode but not >> device mode, this should be handled. >> >> Signed-off-by: Peng Fan <Peng.Fan@freescale.com> >> Signed-off-by: Ye Li <B37916@freescale.com> >> --- >> >> Changes v2: >> Introduce a new weak function to let board have a choice to decide which mode >> to work at. >> >> drivers/usb/host/ehci-mx6.c | 7 +++++++ >> 1 file changed, 7 insertions(+) >> >> diff --git a/drivers/usb/host/ehci-mx6.c b/drivers/usb/host/ehci-mx6.c >> index 9ec5a0a..3662a80 100644 >> --- a/drivers/usb/host/ehci-mx6.c >> +++ b/drivers/usb/host/ehci-mx6.c >> @@ -193,6 +193,11 @@ static void usb_oc_config(int index) >> __raw_writel(val, ctrl); >> } >> >> +int __weak board_ehci_usb_mode(int index, enum usb_init_type *type) >> +{ >> + return 0; >> +} >> + >> int __weak board_ehci_hcd_init(int port) >> { >> return 0; >> @@ -223,6 +228,8 @@ int ehci_hcd_init(int index, enum usb_init_type init, >> usb_internal_phy_clock_gate(index, 1); >> type = usb_phy_enable(index, ehci) ? USB_INIT_DEVICE : USB_INIT_HOST; >> >> + board_ehci_usb_mode(index, &type); >> + >> *hccr = (struct ehci_hccr *)((uint32_t)&ehci->caplength); >> *hcor = (struct ehci_hcor *)((uint32_t)*hccr + >> HC_LENGTH(ehci_readl(&(*hccr)->cr_capbase))); > > Can you add a prototype type as well and make sure it is included? I did not find a good place for the prototype type. I think ehci.h is not fine to include this prototype. Any suggestions? > > Thanks, > Jeroen > Regards, Peng.
On Tuesday, November 04, 2014 at 02:29:56 PM, Peng Fan wrote: > Hi Marek, > > 在 11/4/2014 7:01 PM, Marek Vasut 写道: > > On Tuesday, November 04, 2014 at 11:50:29 AM, Peng Fan wrote: > >> 在 11/4/2014 6:33 PM, Marek Vasut 写道: > >>> On Tuesday, November 04, 2014 at 08:50:00 AM, Peng Fan wrote: > >>>> Include a weak function board_ehci_usb_mode to gives board code > >>>> a choice. > >>> > >>> What choice? > >>> > >>>> If the board want the otg port work in host mode but not > >>>> device mode, this should be handled. > >>> > >>> How? > >>> > >>> Also, isn't usb_phy_enable() supposed to do exactly this kind of > >>> selection between device and host mode ? > >> > >> In mx6sxsabresd board, there are two usb port, one used for otg, the > >> other used for host. However they are connected to SOC USB controller > >> otg1 core and otg2 core respectively. Like following: > >> > >> OTG1 CORE <----> board otg port > >> OTG2 CORE <----> board host port > >> > >> However the board do not have ID pin set for board host port. If just > >> use usb_phy_enable, the board host port will not work, because > >> "type = usb_phy_enable(index, ehci) ? USB_INIT_DEVICE : USB_INIT_HOST;" > >> will always set type with USB_INIT_DEVICE. > >> > >> Because i did not find way to handle this situation in > >> board/freescale/mx6sxsabresd/mx6sxsabresd.c, add this function to let > >> board level code handle handle 'type', if board level code want to set > >> it's own 'type'. > > > > This part in usb_phy_enable() > > > > 163 return val & USBPHY_CTRL_OTG_ID; > > > > should be replaced by some kind of a board-specific callback then, with > > default implmentation being the above (reading the phy ctrl register). > > How about using the following piece of code? > in ehci-mx6.c > > unsigned int __weak board_usb_phy_mode(int index, unsigned int val) > { > return val & USBPHY_CTRL_OTG_ID; > } > > replace "return val & USBPHY_CTRL_OTG_ID;" using " > return board_usb_phy_mode(index, val);" > > In board file, > unsigned int board_usb_phy_mode(int index, unsigned int val) Why not pass in full struct usb_ehci * instead ? Passing some ad-hoc $val into the function doesn't seem like a scalable future-proof solution. [...] Best regards, Marek Vasut
Hello Peng, On 04-11-14 14:36, Peng Fan wrote: > > Hi Jeroen, > > 在 11/4/2014 7:40 PM, Jeroen Hofstee 写道: >> Hello Peng, >> >> On 04-11-14 08:50, Peng Fan wrote: >>> Include a weak function board_ehci_usb_mode to gives board code >>> a choice. If the board want the otg port work in host mode but not >>> device mode, this should be handled. >>> >>> Signed-off-by: Peng Fan <Peng.Fan@freescale.com> >>> Signed-off-by: Ye Li <B37916@freescale.com> >>> --- >>> >>> Changes v2: >>> Introduce a new weak function to let board have a choice to >>> decide which mode >>> to work at. >>> >>> drivers/usb/host/ehci-mx6.c | 7 +++++++ >>> 1 file changed, 7 insertions(+) >>> >>> diff --git a/drivers/usb/host/ehci-mx6.c b/drivers/usb/host/ehci-mx6.c >>> index 9ec5a0a..3662a80 100644 >>> --- a/drivers/usb/host/ehci-mx6.c >>> +++ b/drivers/usb/host/ehci-mx6.c >>> @@ -193,6 +193,11 @@ static void usb_oc_config(int index) >>> __raw_writel(val, ctrl); >>> } >>> >>> +int __weak board_ehci_usb_mode(int index, enum usb_init_type *type) >>> +{ >>> + return 0; >>> +} >>> + >>> int __weak board_ehci_hcd_init(int port) >>> { >>> return 0; >>> @@ -223,6 +228,8 @@ int ehci_hcd_init(int index, enum usb_init_type >>> init, >>> usb_internal_phy_clock_gate(index, 1); >>> type = usb_phy_enable(index, ehci) ? USB_INIT_DEVICE : >>> USB_INIT_HOST; >>> >>> + board_ehci_usb_mode(index, &type); >>> + >>> *hccr = (struct ehci_hccr *)((uint32_t)&ehci->caplength); >>> *hcor = (struct ehci_hcor *)((uint32_t)*hccr + >>> HC_LENGTH(ehci_readl(&(*hccr)->cr_capbase))); >> >> Can you add a prototype type as well and make sure it is included? > I did not find a good place for the prototype type. I think ehci.h is > not fine to include this prototype. Any suggestions? Ah, good point. I have reserved USB as well for the "not trivial" to fix warnings. Ideally there should be a header in drivers somewhere defining what different usb drivers can/should support when dealing with common usb code and one in include/... for the board interface. Since this is currently lacking, I am also fine checking it in without a valid prototype and fix it later. Regards, Jeroen
在 11/5/2014 1:33 AM, Marek Vasut 写道: > On Tuesday, November 04, 2014 at 02:29:56 PM, Peng Fan wrote: >> Hi Marek, >> >> 在 11/4/2014 7:01 PM, Marek Vasut 写道: >>> On Tuesday, November 04, 2014 at 11:50:29 AM, Peng Fan wrote: >>>> 在 11/4/2014 6:33 PM, Marek Vasut 写道: >>>>> On Tuesday, November 04, 2014 at 08:50:00 AM, Peng Fan wrote: >>>>>> Include a weak function board_ehci_usb_mode to gives board code >>>>>> a choice. >>>>> >>>>> What choice? >>>>> >>>>>> If the board want the otg port work in host mode but not >>>>>> device mode, this should be handled. >>>>> >>>>> How? >>>>> >>>>> Also, isn't usb_phy_enable() supposed to do exactly this kind of >>>>> selection between device and host mode ? >>>> >>>> In mx6sxsabresd board, there are two usb port, one used for otg, the >>>> other used for host. However they are connected to SOC USB controller >>>> otg1 core and otg2 core respectively. Like following: >>>> >>>> OTG1 CORE <----> board otg port >>>> OTG2 CORE <----> board host port >>>> >>>> However the board do not have ID pin set for board host port. If just >>>> use usb_phy_enable, the board host port will not work, because >>>> "type = usb_phy_enable(index, ehci) ? USB_INIT_DEVICE : USB_INIT_HOST;" >>>> will always set type with USB_INIT_DEVICE. >>>> >>>> Because i did not find way to handle this situation in >>>> board/freescale/mx6sxsabresd/mx6sxsabresd.c, add this function to let >>>> board level code handle handle 'type', if board level code want to set >>>> it's own 'type'. >>> >>> This part in usb_phy_enable() >>> >>> 163 return val & USBPHY_CTRL_OTG_ID; >>> >>> should be replaced by some kind of a board-specific callback then, with >>> default implmentation being the above (reading the phy ctrl register). >> >> How about using the following piece of code? >> in ehci-mx6.c >> >> unsigned int __weak board_usb_phy_mode(int index, unsigned int val) >> { >> return val & USBPHY_CTRL_OTG_ID; >> } >> >> replace "return val & USBPHY_CTRL_OTG_ID;" using " >> return board_usb_phy_mode(index, val);" >> >> In board file, >> unsigned int board_usb_phy_mode(int index, unsigned int val) > > Why not pass in full struct usb_ehci * instead ? Passing some ad-hoc $val into > the function doesn't seem like a scalable future-proof solution. > [...] Passing struct usb_ehci * to board code seems exports ehci register definition to board layer. How about just use "int board_usb_phy_mode(int index)" without using 'val' or 'struct usb_ehci *ehci'. > Best regards, > Marek Vasut > Regards, Peng.
On Wednesday, November 05, 2014 at 07:00:32 AM, Peng Fan wrote: > 在 11/5/2014 1:33 AM, Marek Vasut 写道: > > On Tuesday, November 04, 2014 at 02:29:56 PM, Peng Fan wrote: > >> Hi Marek, > >> > >> 在 11/4/2014 7:01 PM, Marek Vasut 写道: > >>> On Tuesday, November 04, 2014 at 11:50:29 AM, Peng Fan wrote: > >>>> 在 11/4/2014 6:33 PM, Marek Vasut 写道: > >>>>> On Tuesday, November 04, 2014 at 08:50:00 AM, Peng Fan wrote: > >>>>>> Include a weak function board_ehci_usb_mode to gives board code > >>>>>> a choice. > >>>>> > >>>>> What choice? > >>>>> > >>>>>> If the board want the otg port work in host mode but not > >>>>>> device mode, this should be handled. > >>>>> > >>>>> How? > >>>>> > >>>>> Also, isn't usb_phy_enable() supposed to do exactly this kind of > >>>>> selection between device and host mode ? > >>>> > >>>> In mx6sxsabresd board, there are two usb port, one used for otg, the > >>>> other used for host. However they are connected to SOC USB controller > >>>> otg1 core and otg2 core respectively. Like following: > >>>> > >>>> OTG1 CORE <----> board otg port > >>>> OTG2 CORE <----> board host port > >>>> > >>>> However the board do not have ID pin set for board host port. If just > >>>> use usb_phy_enable, the board host port will not work, because > >>>> "type = usb_phy_enable(index, ehci) ? USB_INIT_DEVICE : > >>>> USB_INIT_HOST;" will always set type with USB_INIT_DEVICE. > >>>> > >>>> Because i did not find way to handle this situation in > >>>> board/freescale/mx6sxsabresd/mx6sxsabresd.c, add this function to let > >>>> board level code handle handle 'type', if board level code want to set > >>>> it's own 'type'. > >>> > >>> This part in usb_phy_enable() > >>> > >>> 163 return val & USBPHY_CTRL_OTG_ID; > >>> > >>> should be replaced by some kind of a board-specific callback then, with > >>> default implmentation being the above (reading the phy ctrl register). > >> > >> How about using the following piece of code? > >> in ehci-mx6.c > >> > >> unsigned int __weak board_usb_phy_mode(int index, unsigned int val) > >> { > >> > >> return val & USBPHY_CTRL_OTG_ID; > >> > >> } > >> > >> replace "return val & USBPHY_CTRL_OTG_ID;" using " > >> return board_usb_phy_mode(index, val);" > >> > >> In board file, > >> unsigned int board_usb_phy_mode(int index, unsigned int val) > > > > Why not pass in full struct usb_ehci * instead ? Passing some ad-hoc $val > > into the function doesn't seem like a scalable future-proof solution. > > [...] > > Passing struct usb_ehci * to board code seems exports ehci register > definition to board layer. Yeah. > How about just use > "int board_usb_phy_mode(int index)" without using 'val' or 'struct > usb_ehci *ehci'. The board part might need to read the EHCI registers though. How would the board part be able to do it if you didn't pass the *ehci in ?
在 11/5/2014 5:03 PM, Marek Vasut 写道: > On Wednesday, November 05, 2014 at 07:00:32 AM, Peng Fan wrote: >> 在 11/5/2014 1:33 AM, Marek Vasut 写道: >>> On Tuesday, November 04, 2014 at 02:29:56 PM, Peng Fan wrote: >>>> Hi Marek, >>>> >>>> 在 11/4/2014 7:01 PM, Marek Vasut 写道: >>>>> On Tuesday, November 04, 2014 at 11:50:29 AM, Peng Fan wrote: >>>>>> 在 11/4/2014 6:33 PM, Marek Vasut 写道: >>>>>>> On Tuesday, November 04, 2014 at 08:50:00 AM, Peng Fan wrote: >>>>>>>> Include a weak function board_ehci_usb_mode to gives board code >>>>>>>> a choice. >>>>>>> >>>>>>> What choice? >>>>>>> >>>>>>>> If the board want the otg port work in host mode but not >>>>>>>> device mode, this should be handled. >>>>>>> >>>>>>> How? >>>>>>> >>>>>>> Also, isn't usb_phy_enable() supposed to do exactly this kind of >>>>>>> selection between device and host mode ? >>>>>> >>>>>> In mx6sxsabresd board, there are two usb port, one used for otg, the >>>>>> other used for host. However they are connected to SOC USB controller >>>>>> otg1 core and otg2 core respectively. Like following: >>>>>> >>>>>> OTG1 CORE <----> board otg port >>>>>> OTG2 CORE <----> board host port >>>>>> >>>>>> However the board do not have ID pin set for board host port. If just >>>>>> use usb_phy_enable, the board host port will not work, because >>>>>> "type = usb_phy_enable(index, ehci) ? USB_INIT_DEVICE : >>>>>> USB_INIT_HOST;" will always set type with USB_INIT_DEVICE. >>>>>> >>>>>> Because i did not find way to handle this situation in >>>>>> board/freescale/mx6sxsabresd/mx6sxsabresd.c, add this function to let >>>>>> board level code handle handle 'type', if board level code want to set >>>>>> it's own 'type'. >>>>> >>>>> This part in usb_phy_enable() >>>>> >>>>> 163 return val & USBPHY_CTRL_OTG_ID; >>>>> >>>>> should be replaced by some kind of a board-specific callback then, with >>>>> default implmentation being the above (reading the phy ctrl register). >>>> >>>> How about using the following piece of code? >>>> in ehci-mx6.c >>>> >>>> unsigned int __weak board_usb_phy_mode(int index, unsigned int val) >>>> { >>>> >>>> return val & USBPHY_CTRL_OTG_ID; >>>> >>>> } >>>> >>>> replace "return val & USBPHY_CTRL_OTG_ID;" using " >>>> return board_usb_phy_mode(index, val);" >>>> >>>> In board file, >>>> unsigned int board_usb_phy_mode(int index, unsigned int val) >>> >>> Why not pass in full struct usb_ehci * instead ? Passing some ad-hoc $val >>> into the function doesn't seem like a scalable future-proof solution. >>> [...] >> >> Passing struct usb_ehci * to board code seems exports ehci register >> definition to board layer. > > Yeah. > >> How about just use >> "int board_usb_phy_mode(int index)" without using 'val' or 'struct >> usb_ehci *ehci'. > > The board part might need to read the EHCI registers though. How would the > board part be able to do it if you didn't pass the *ehci in ? To imx6, the ID bit is in PHY ctrl reg 'USBPHYx_CTRLn', also the phy regs definition are not included in "struct usb_ehci". I just think expose the ehci register to board layer is not fine and board_usb_phy_mode does not need this. I define this just as "board_ehci_hcd_init" and "board_ehci_power". Their prototype are int __weak board_ehci_hcd_init(int port); int __weak board_ehci_power(int port, int on); My implementation is the following: replace "return val & USBPHY_CTRL_OTG_ID;" using "return board_usb_phy_mode(index);" in usb_phy_enable In drivers/usb/host/ehci-mx6.c: 116 int __weak board_usb_phy_mode(int index) 117 { 118 void __iomem *phy_reg; 119 void __iomem *phy_ctrl; 120 u32 val; 121 122 phy_reg = (void __iomem *)phy_bases[index]; 123 phy_ctrl = (void __iomem *)(phy_reg + USBPHY_CTRL); 124 125 val = __raw_readl(phy_ctrl); 126 127 return val & USBPHY_CTRL_OTG_ID; 128 } In board/freescale/mx6sxsabresd/mx6sxsabresd.c: 295 int board_usb_phy_mode(int port) 296 { 297 void __iomem *phy_reg; 298 void __iomem *phy_ctrl; 299 u32 val; 300 301 switch (port) { 302 case 0: 303 phy_reg = (void __iomem *)USB_PHY0_BASE_ADDR; 304 phy_ctrl = (void __iomem *)(phy_reg + USBPHY_CTRL); 305 val = __raw_readl(phy_ctrl); 306 return val & USBPHY_CTRL_OTG_ID; 307 case 1: 308 /* Work in HOST mode. */ 309 return 0; 310 } 311 312 /* suppress warning msg */ 313 return 0; 314 } Is this piece of code fine? > Regards, Peng.
On Wednesday, November 05, 2014 at 10:18:25 AM, Peng Fan wrote: > 在 11/5/2014 5:03 PM, Marek Vasut 写道: > > On Wednesday, November 05, 2014 at 07:00:32 AM, Peng Fan wrote: > >> 在 11/5/2014 1:33 AM, Marek Vasut 写道: > >>> On Tuesday, November 04, 2014 at 02:29:56 PM, Peng Fan wrote: > >>>> Hi Marek, > >>>> > >>>> 在 11/4/2014 7:01 PM, Marek Vasut 写道: > >>>>> On Tuesday, November 04, 2014 at 11:50:29 AM, Peng Fan wrote: > >>>>>> 在 11/4/2014 6:33 PM, Marek Vasut 写道: > >>>>>>> On Tuesday, November 04, 2014 at 08:50:00 AM, Peng Fan wrote: > >>>>>>>> Include a weak function board_ehci_usb_mode to gives board code > >>>>>>>> a choice. > >>>>>>> > >>>>>>> What choice? > >>>>>>> > >>>>>>>> If the board want the otg port work in host mode but not > >>>>>>>> device mode, this should be handled. > >>>>>>> > >>>>>>> How? > >>>>>>> > >>>>>>> Also, isn't usb_phy_enable() supposed to do exactly this kind of > >>>>>>> selection between device and host mode ? > >>>>>> > >>>>>> In mx6sxsabresd board, there are two usb port, one used for otg, the > >>>>>> other used for host. However they are connected to SOC USB > >>>>>> controller otg1 core and otg2 core respectively. Like following: > >>>>>> > >>>>>> OTG1 CORE <----> board otg port > >>>>>> OTG2 CORE <----> board host port > >>>>>> > >>>>>> However the board do not have ID pin set for board host port. If > >>>>>> just use usb_phy_enable, the board host port will not work, because > >>>>>> "type = usb_phy_enable(index, ehci) ? USB_INIT_DEVICE : > >>>>>> USB_INIT_HOST;" will always set type with USB_INIT_DEVICE. > >>>>>> > >>>>>> Because i did not find way to handle this situation in > >>>>>> board/freescale/mx6sxsabresd/mx6sxsabresd.c, add this function to > >>>>>> let board level code handle handle 'type', if board level code want > >>>>>> to set it's own 'type'. > >>>>> > >>>>> This part in usb_phy_enable() > >>>>> > >>>>> 163 return val & USBPHY_CTRL_OTG_ID; > >>>>> > >>>>> should be replaced by some kind of a board-specific callback then, > >>>>> with default implmentation being the above (reading the phy ctrl > >>>>> register). > >>>> > >>>> How about using the following piece of code? > >>>> in ehci-mx6.c > >>>> > >>>> unsigned int __weak board_usb_phy_mode(int index, unsigned int val) > >>>> { > >>>> > >>>> return val & USBPHY_CTRL_OTG_ID; > >>>> > >>>> } > >>>> > >>>> replace "return val & USBPHY_CTRL_OTG_ID;" using " > >>>> return board_usb_phy_mode(index, val);" > >>>> > >>>> In board file, > >>>> unsigned int board_usb_phy_mode(int index, unsigned int val) > >>> > >>> Why not pass in full struct usb_ehci * instead ? Passing some ad-hoc > >>> $val into the function doesn't seem like a scalable future-proof > >>> solution. [...] > >> > >> Passing struct usb_ehci * to board code seems exports ehci register > >> definition to board layer. > > > > Yeah. > > > >> How about just use > >> "int board_usb_phy_mode(int index)" without using 'val' or 'struct > >> usb_ehci *ehci'. > > > > The board part might need to read the EHCI registers though. How would > > the board part be able to do it if you didn't pass the *ehci in ? > > To imx6, the ID bit is in PHY ctrl reg 'USBPHYx_CTRLn', also the phy > regs definition are not included in "struct usb_ehci". I just think > expose the ehci register to board layer is not fine and > board_usb_phy_mode does not need this. I define this just as > "board_ehci_hcd_init" and "board_ehci_power". Their prototype are > int __weak board_ehci_hcd_init(int port); > int __weak board_ehci_power(int port, int on); > > My implementation is the following: > > replace "return val & USBPHY_CTRL_OTG_ID;" using "return > board_usb_phy_mode(index);" in usb_phy_enable > > In drivers/usb/host/ehci-mx6.c: > 116 int __weak board_usb_phy_mode(int index) > 117 { > 118 void __iomem *phy_reg; > 119 void __iomem *phy_ctrl; > 120 u32 val; > 121 > 122 phy_reg = (void __iomem *)phy_bases[index]; > 123 phy_ctrl = (void __iomem *)(phy_reg + USBPHY_CTRL); > 124 > 125 val = __raw_readl(phy_ctrl); > 126 > 127 return val & USBPHY_CTRL_OTG_ID; > 128 } > > In board/freescale/mx6sxsabresd/mx6sxsabresd.c: > 295 int board_usb_phy_mode(int port) > 296 { > 297 void __iomem *phy_reg; > 298 void __iomem *phy_ctrl; > 299 u32 val; > 300 > 301 switch (port) { > 302 case 0: > 303 phy_reg = (void __iomem *)USB_PHY0_BASE_ADDR; > 304 phy_ctrl = (void __iomem *)(phy_reg + USBPHY_CTRL); > 305 val = __raw_readl(phy_ctrl); > 306 return val & USBPHY_CTRL_OTG_ID; > 307 case 1: > 308 /* Work in HOST mode. */ > 309 return 0; > 310 } > 311 > 312 /* suppress warning msg */ > 313 return 0; > 314 } > > Is this piece of code fine? These ad-hoc hooks are starting to become absolute horror, but I guess this one (if properly documented) might just work. Let's see what will come out of this approach.
在 11/7/2014 4:20 AM, Marek Vasut 写道: > On Wednesday, November 05, 2014 at 10:18:25 AM, Peng Fan wrote: >> 在 11/5/2014 5:03 PM, Marek Vasut 写道: >>> On Wednesday, November 05, 2014 at 07:00:32 AM, Peng Fan wrote: >>>> 在 11/5/2014 1:33 AM, Marek Vasut 写道: >>>>> On Tuesday, November 04, 2014 at 02:29:56 PM, Peng Fan wrote: >>>>>> Hi Marek, >>>>>> >>>>>> 在 11/4/2014 7:01 PM, Marek Vasut 写道: >>>>>>> On Tuesday, November 04, 2014 at 11:50:29 AM, Peng Fan wrote: >>>>>>>> 在 11/4/2014 6:33 PM, Marek Vasut 写道: >>>>>>>>> On Tuesday, November 04, 2014 at 08:50:00 AM, Peng Fan wrote: >>>>>>>>>> Include a weak function board_ehci_usb_mode to gives board code >>>>>>>>>> a choice. >>>>>>>>> >>>>>>>>> What choice? >>>>>>>>> >>>>>>>>>> If the board want the otg port work in host mode but not >>>>>>>>>> device mode, this should be handled. >>>>>>>>> >>>>>>>>> How? >>>>>>>>> >>>>>>>>> Also, isn't usb_phy_enable() supposed to do exactly this kind of >>>>>>>>> selection between device and host mode ? >>>>>>>> >>>>>>>> In mx6sxsabresd board, there are two usb port, one used for otg, the >>>>>>>> other used for host. However they are connected to SOC USB >>>>>>>> controller otg1 core and otg2 core respectively. Like following: >>>>>>>> >>>>>>>> OTG1 CORE <----> board otg port >>>>>>>> OTG2 CORE <----> board host port >>>>>>>> >>>>>>>> However the board do not have ID pin set for board host port. If >>>>>>>> just use usb_phy_enable, the board host port will not work, because >>>>>>>> "type = usb_phy_enable(index, ehci) ? USB_INIT_DEVICE : >>>>>>>> USB_INIT_HOST;" will always set type with USB_INIT_DEVICE. >>>>>>>> >>>>>>>> Because i did not find way to handle this situation in >>>>>>>> board/freescale/mx6sxsabresd/mx6sxsabresd.c, add this function to >>>>>>>> let board level code handle handle 'type', if board level code want >>>>>>>> to set it's own 'type'. >>>>>>> >>>>>>> This part in usb_phy_enable() >>>>>>> >>>>>>> 163 return val & USBPHY_CTRL_OTG_ID; >>>>>>> >>>>>>> should be replaced by some kind of a board-specific callback then, >>>>>>> with default implmentation being the above (reading the phy ctrl >>>>>>> register). >>>>>> >>>>>> How about using the following piece of code? >>>>>> in ehci-mx6.c >>>>>> >>>>>> unsigned int __weak board_usb_phy_mode(int index, unsigned int val) >>>>>> { >>>>>> >>>>>> return val & USBPHY_CTRL_OTG_ID; >>>>>> >>>>>> } >>>>>> >>>>>> replace "return val & USBPHY_CTRL_OTG_ID;" using " >>>>>> return board_usb_phy_mode(index, val);" >>>>>> >>>>>> In board file, >>>>>> unsigned int board_usb_phy_mode(int index, unsigned int val) >>>>> >>>>> Why not pass in full struct usb_ehci * instead ? Passing some ad-hoc >>>>> $val into the function doesn't seem like a scalable future-proof >>>>> solution. [...] >>>> >>>> Passing struct usb_ehci * to board code seems exports ehci register >>>> definition to board layer. >>> >>> Yeah. >>> >>>> How about just use >>>> "int board_usb_phy_mode(int index)" without using 'val' or 'struct >>>> usb_ehci *ehci'. >>> >>> The board part might need to read the EHCI registers though. How would >>> the board part be able to do it if you didn't pass the *ehci in ? >> >> To imx6, the ID bit is in PHY ctrl reg 'USBPHYx_CTRLn', also the phy >> regs definition are not included in "struct usb_ehci". I just think >> expose the ehci register to board layer is not fine and >> board_usb_phy_mode does not need this. I define this just as >> "board_ehci_hcd_init" and "board_ehci_power". Their prototype are >> int __weak board_ehci_hcd_init(int port); >> int __weak board_ehci_power(int port, int on); >> >> My implementation is the following: >> >> replace "return val & USBPHY_CTRL_OTG_ID;" using "return >> board_usb_phy_mode(index);" in usb_phy_enable >> >> In drivers/usb/host/ehci-mx6.c: >> 116 int __weak board_usb_phy_mode(int index) >> 117 { >> 118 void __iomem *phy_reg; >> 119 void __iomem *phy_ctrl; >> 120 u32 val; >> 121 >> 122 phy_reg = (void __iomem *)phy_bases[index]; >> 123 phy_ctrl = (void __iomem *)(phy_reg + USBPHY_CTRL); >> 124 >> 125 val = __raw_readl(phy_ctrl); >> 126 >> 127 return val & USBPHY_CTRL_OTG_ID; >> 128 } >> >> In board/freescale/mx6sxsabresd/mx6sxsabresd.c: >> 295 int board_usb_phy_mode(int port) >> 296 { >> 297 void __iomem *phy_reg; >> 298 void __iomem *phy_ctrl; >> 299 u32 val; >> 300 >> 301 switch (port) { >> 302 case 0: >> 303 phy_reg = (void __iomem *)USB_PHY0_BASE_ADDR; >> 304 phy_ctrl = (void __iomem *)(phy_reg + USBPHY_CTRL); >> 305 val = __raw_readl(phy_ctrl); >> 306 return val & USBPHY_CTRL_OTG_ID; >> 307 case 1: >> 308 /* Work in HOST mode. */ >> 309 return 0; >> 310 } >> 311 >> 312 /* suppress warning msg */ >> 313 return 0; >> 314 } >> >> Is this piece of code fine? > > These ad-hoc hooks are starting to become absolute horror, but I guess > this one (if properly documented) might just work. Let's see what will > come out of this approach. > Sent out v3 patch set just now. Please review. Thanks, Peng.
On Friday, November 07, 2014 at 02:10:28 AM, Peng Fan wrote: [...] > >> In board/freescale/mx6sxsabresd/mx6sxsabresd.c: > >> 295 int board_usb_phy_mode(int port) > >> 296 { > >> 297 void __iomem *phy_reg; > >> 298 void __iomem *phy_ctrl; > >> 299 u32 val; > >> 300 > >> 301 switch (port) { > >> 302 case 0: > >> 303 phy_reg = (void __iomem *)USB_PHY0_BASE_ADDR; > >> 304 phy_ctrl = (void __iomem *)(phy_reg + USBPHY_CTRL); > >> 305 val = __raw_readl(phy_ctrl); > >> 306 return val & USBPHY_CTRL_OTG_ID; > >> 307 case 1: > >> 308 /* Work in HOST mode. */ > >> 309 return 0; > >> 310 } > >> 311 > >> 312 /* suppress warning msg */ > >> 313 return 0; > >> 314 } > >> > >> Is this piece of code fine? > > > > These ad-hoc hooks are starting to become absolute horror, but I guess > > this one (if properly documented) might just work. Let's see what will > > come out of this approach. > > Sent out v3 patch set just now. Please review. Done, there're only minor comments. Thanks! Best regards, Marek Vasut
diff --git a/drivers/usb/host/ehci-mx6.c b/drivers/usb/host/ehci-mx6.c index 9ec5a0a..3662a80 100644 --- a/drivers/usb/host/ehci-mx6.c +++ b/drivers/usb/host/ehci-mx6.c @@ -193,6 +193,11 @@ static void usb_oc_config(int index) __raw_writel(val, ctrl); } +int __weak board_ehci_usb_mode(int index, enum usb_init_type *type) +{ + return 0; +} + int __weak board_ehci_hcd_init(int port) { return 0; @@ -223,6 +228,8 @@ int ehci_hcd_init(int index, enum usb_init_type init, usb_internal_phy_clock_gate(index, 1); type = usb_phy_enable(index, ehci) ? USB_INIT_DEVICE : USB_INIT_HOST; + board_ehci_usb_mode(index, &type); + *hccr = (struct ehci_hccr *)((uint32_t)&ehci->caplength); *hcor = (struct ehci_hcor *)((uint32_t)*hccr + HC_LENGTH(ehci_readl(&(*hccr)->cr_capbase)));