| Message ID | 1381140752-312-3-git-send-email-matt.porter@linaro.org |
|---|---|
| State | Superseded |
| Headers | show |
On Mon, Oct 07, 2013 at 06:12:29AM -0400, Matt Porter wrote: > Extend dwc2 binding with an optional utmi phy width property. > Enable the s3c-hsotg.c driver to use standard dwc2 binding > and enable configuration of the UTMI phy width based on the > property. > > Signed-off-by: Matt Porter <matt.porter@linaro.org> > Reviewed-by: Markus Mayer <markus.mayer@linaro.org> > Reviewed-by: Tim Kryger <tim.kryger@linaro.org> > --- > Documentation/devicetree/bindings/staging/dwc2.txt | 4 ++++ > drivers/usb/gadget/s3c-hsotg.c | 18 +++++++++++++++++- > drivers/usb/gadget/s3c-hsotg.h | 1 + > 3 files changed, 22 insertions(+), 1 deletion(-) > > diff --git a/Documentation/devicetree/bindings/staging/dwc2.txt b/Documentation/devicetree/bindings/staging/dwc2.txt > index 1a1b7cf..fb6b8ee 100644 > --- a/Documentation/devicetree/bindings/staging/dwc2.txt > +++ b/Documentation/devicetree/bindings/staging/dwc2.txt > @@ -6,10 +6,14 @@ Required properties: > - reg : Should contain 1 register range (address and length) > - interrupts : Should contain 1 interrupt > > +Optional properties: > +- snps,phy-utmi-width: Must contain the UTMI data width (either 8 or 16) isn't this available in any of the configuration registers ? > Example: > > usb@101c0000 { > compatible = "ralink,rt3050-usb, snps,dwc2"; > reg = <0x101c0000 40000>; > interrupts = <18>; > + snps,phy-utmi-width = <8>; indentation.
On 10/10/2013 11:29 AM, Felipe Balbi wrote: > On Mon, Oct 07, 2013 at 06:12:29AM -0400, Matt Porter wrote: >> Extend dwc2 binding with an optional utmi phy width property. >> Enable the s3c-hsotg.c driver to use standard dwc2 binding >> and enable configuration of the UTMI phy width based on the >> property. >> >> Signed-off-by: Matt Porter <matt.porter@linaro.org> >> Reviewed-by: Markus Mayer <markus.mayer@linaro.org> >> Reviewed-by: Tim Kryger <tim.kryger@linaro.org> >> --- >> Documentation/devicetree/bindings/staging/dwc2.txt | 4 ++++ >> drivers/usb/gadget/s3c-hsotg.c | 18 +++++++++++++++++- >> drivers/usb/gadget/s3c-hsotg.h | 1 + >> 3 files changed, 22 insertions(+), 1 deletion(-) >> >> diff --git a/Documentation/devicetree/bindings/staging/dwc2.txt b/Documentation/devicetree/bindings/staging/dwc2.txt >> index 1a1b7cf..fb6b8ee 100644 >> --- a/Documentation/devicetree/bindings/staging/dwc2.txt >> +++ b/Documentation/devicetree/bindings/staging/dwc2.txt >> @@ -6,10 +6,14 @@ Required properties: >> - reg : Should contain 1 register range (address and length) >> - interrupts : Should contain 1 interrupt >> >> +Optional properties: >> +- snps,phy-utmi-width: Must contain the UTMI data width (either 8 or 16) > > isn't this available in any of the configuration registers ? Yes and no. HWCFG4 has a UTMI data width field. However, it has 3 valid states, "8", "16", or "8 or 16". The BCM281xx implementation is set to the latter and the attached phy is 8-bit. Looking at dwc2 prior to Matthijs Kooijman's patch [1] which starts validating the value of phy_utmi_width in that driver, the pci.c dwc2_module_params .phy_utmi_width field there even had the comment, "/* 16 bits - NOT DETECTABLE */". The autodetect code in dwc2_set_param_phy_utmi_width() will fail if HWCFG4 has the "8 or 16" option as it just decides to default to a phy width of 16 if nothing is configured by the platform glue. This property would also allow this issue to be addressed in that driver. >> Example: >> >> usb@101c0000 { >> compatible = "ralink,rt3050-usb, snps,dwc2"; >> reg = <0x101c0000 40000>; >> interrupts = <18>; >> + snps,phy-utmi-width = <8>; > > indentation. will fix. -Matt [1] de4a193 staging: dwc2: validate the value for phy_utmi_width
Hi, On Thu, Oct 10, 2013 at 12:54:40PM -0400, Matt Porter wrote: > On 10/10/2013 11:29 AM, Felipe Balbi wrote: > >On Mon, Oct 07, 2013 at 06:12:29AM -0400, Matt Porter wrote: > >>Extend dwc2 binding with an optional utmi phy width property. > >>Enable the s3c-hsotg.c driver to use standard dwc2 binding > >>and enable configuration of the UTMI phy width based on the > >>property. > >> > >>Signed-off-by: Matt Porter <matt.porter@linaro.org> > >>Reviewed-by: Markus Mayer <markus.mayer@linaro.org> > >>Reviewed-by: Tim Kryger <tim.kryger@linaro.org> > >>--- > >> Documentation/devicetree/bindings/staging/dwc2.txt | 4 ++++ > >> drivers/usb/gadget/s3c-hsotg.c | 18 +++++++++++++++++- > >> drivers/usb/gadget/s3c-hsotg.h | 1 + > >> 3 files changed, 22 insertions(+), 1 deletion(-) > >> > >>diff --git a/Documentation/devicetree/bindings/staging/dwc2.txt b/Documentation/devicetree/bindings/staging/dwc2.txt > >>index 1a1b7cf..fb6b8ee 100644 > >>--- a/Documentation/devicetree/bindings/staging/dwc2.txt > >>+++ b/Documentation/devicetree/bindings/staging/dwc2.txt > >>@@ -6,10 +6,14 @@ Required properties: > >> - reg : Should contain 1 register range (address and length) > >> - interrupts : Should contain 1 interrupt > >> > >>+Optional properties: > >>+- snps,phy-utmi-width: Must contain the UTMI data width (either 8 or 16) > > > >isn't this available in any of the configuration registers ? > > Yes and no. HWCFG4 has a UTMI data width field. However, it has 3 > valid states, "8", "16", or "8 or 16". The BCM281xx implementation is > set to the latter and the attached phy is 8-bit. > > Looking at dwc2 prior to Matthijs Kooijman's patch [1] which starts > validating the value of phy_utmi_width in that driver, the pci.c > dwc2_module_params .phy_utmi_width field there even had the comment, > "/* 16 bits - NOT DETECTABLE */". The autodetect code in > dwc2_set_param_phy_utmi_width() will fail if HWCFG4 has the "8 or 16" > option as it just decides to default to a phy width of 16 if nothing > is configured by the platform glue. This property would also allow > this issue to be addressed in that driver. fair enough, but I'd really like to hear from DT folks if your suggested binding is acceptable. It seems like we can equally argue that it's a SW configuration or HW description. cheers
> From: Felipe Balbi [mailto:balbi@ti.com] > Sent: Thursday, October 10, 2013 10:46 AM > > On Thu, Oct 10, 2013 at 12:54:40PM -0400, Matt Porter wrote: > > On 10/10/2013 11:29 AM, Felipe Balbi wrote: > > >On Mon, Oct 07, 2013 at 06:12:29AM -0400, Matt Porter wrote: > > >>Extend dwc2 binding with an optional utmi phy width property. > > >>Enable the s3c-hsotg.c driver to use standard dwc2 binding > > >>and enable configuration of the UTMI phy width based on the > > >>property. > > >> > > >>Signed-off-by: Matt Porter <matt.porter@linaro.org> > > >>Reviewed-by: Markus Mayer <markus.mayer@linaro.org> > > >>Reviewed-by: Tim Kryger <tim.kryger@linaro.org> > > >>--- > > >> Documentation/devicetree/bindings/staging/dwc2.txt | 4 ++++ > > >> drivers/usb/gadget/s3c-hsotg.c | 18 +++++++++++++++++- > > >> drivers/usb/gadget/s3c-hsotg.h | 1 + > > >> 3 files changed, 22 insertions(+), 1 deletion(-) > > >> > > >>diff --git a/Documentation/devicetree/bindings/staging/dwc2.txt > b/Documentation/devicetree/bindings/staging/dwc2.txt > > >>index 1a1b7cf..fb6b8ee 100644 > > >>--- a/Documentation/devicetree/bindings/staging/dwc2.txt > > >>+++ b/Documentation/devicetree/bindings/staging/dwc2.txt > > >>@@ -6,10 +6,14 @@ Required properties: > > >> - reg : Should contain 1 register range (address and length) > > >> - interrupts : Should contain 1 interrupt > > >> > > >>+Optional properties: > > >>+- snps,phy-utmi-width: Must contain the UTMI data width (either 8 or 16) > > > > > >isn't this available in any of the configuration registers ? > > > > Yes and no. HWCFG4 has a UTMI data width field. However, it has 3 > > valid states, "8", "16", or "8 or 16". The BCM281xx implementation is > > set to the latter and the attached phy is 8-bit. > > > > Looking at dwc2 prior to Matthijs Kooijman's patch [1] which starts > > validating the value of phy_utmi_width in that driver, the pci.c > > dwc2_module_params .phy_utmi_width field there even had the comment, > > "/* 16 bits - NOT DETECTABLE */". The autodetect code in > > dwc2_set_param_phy_utmi_width() will fail if HWCFG4 has the "8 or 16" > > option as it just decides to default to a phy width of 16 if nothing > > is configured by the platform glue. This property would also allow > > this issue to be addressed in that driver. > > fair enough, but I'd really like to hear from DT folks if your suggested > binding is acceptable. It seems like we can equally argue that it's a SW > configuration or HW description. It's definitely a HW description - the width of the UTMI data connection. But, which PHY is this? Does it have a register that could tell what the data width is? The dwc2 core has an (optional) PHY Vendor Control Register that allows reading the PHY registers.
Hi, On Thu, Oct 10, 2013 at 05:57:54PM +0000, Paul Zimmerman wrote: > > From: Felipe Balbi [mailto:balbi@ti.com] > > Sent: Thursday, October 10, 2013 10:46 AM > > > > On Thu, Oct 10, 2013 at 12:54:40PM -0400, Matt Porter wrote: > > > On 10/10/2013 11:29 AM, Felipe Balbi wrote: > > > >On Mon, Oct 07, 2013 at 06:12:29AM -0400, Matt Porter wrote: > > > >>Extend dwc2 binding with an optional utmi phy width property. > > > >>Enable the s3c-hsotg.c driver to use standard dwc2 binding > > > >>and enable configuration of the UTMI phy width based on the > > > >>property. > > > >> > > > >>Signed-off-by: Matt Porter <matt.porter@linaro.org> > > > >>Reviewed-by: Markus Mayer <markus.mayer@linaro.org> > > > >>Reviewed-by: Tim Kryger <tim.kryger@linaro.org> > > > >>--- > > > >> Documentation/devicetree/bindings/staging/dwc2.txt | 4 ++++ > > > >> drivers/usb/gadget/s3c-hsotg.c | 18 +++++++++++++++++- > > > >> drivers/usb/gadget/s3c-hsotg.h | 1 + > > > >> 3 files changed, 22 insertions(+), 1 deletion(-) > > > >> > > > >>diff --git a/Documentation/devicetree/bindings/staging/dwc2.txt > > b/Documentation/devicetree/bindings/staging/dwc2.txt > > > >>index 1a1b7cf..fb6b8ee 100644 > > > >>--- a/Documentation/devicetree/bindings/staging/dwc2.txt > > > >>+++ b/Documentation/devicetree/bindings/staging/dwc2.txt > > > >>@@ -6,10 +6,14 @@ Required properties: > > > >> - reg : Should contain 1 register range (address and length) > > > >> - interrupts : Should contain 1 interrupt > > > >> > > > >>+Optional properties: > > > >>+- snps,phy-utmi-width: Must contain the UTMI data width (either 8 or 16) > > > > > > > >isn't this available in any of the configuration registers ? > > > > > > Yes and no. HWCFG4 has a UTMI data width field. However, it has 3 > > > valid states, "8", "16", or "8 or 16". The BCM281xx implementation is > > > set to the latter and the attached phy is 8-bit. > > > > > > Looking at dwc2 prior to Matthijs Kooijman's patch [1] which starts > > > validating the value of phy_utmi_width in that driver, the pci.c > > > dwc2_module_params .phy_utmi_width field there even had the comment, > > > "/* 16 bits - NOT DETECTABLE */". The autodetect code in > > > dwc2_set_param_phy_utmi_width() will fail if HWCFG4 has the "8 or 16" > > > option as it just decides to default to a phy width of 16 if nothing > > > is configured by the platform glue. This property would also allow > > > this issue to be addressed in that driver. > > > > fair enough, but I'd really like to hear from DT folks if your suggested > > binding is acceptable. It seems like we can equally argue that it's a SW > > configuration or HW description. > > It's definitely a HW description - the width of the UTMI data connection. right, but that description should be passed to the PHY, right ? DWC2 works with both widths (8 or 16).
> From: Felipe Balbi [mailto:balbi@ti.com] > Sent: Thursday, October 10, 2013 11:07 AM > > On Thu, Oct 10, 2013 at 05:57:54PM +0000, Paul Zimmerman wrote: > > > From: Felipe Balbi [mailto:balbi@ti.com] > > > Sent: Thursday, October 10, 2013 10:46 AM > > > > > > On Thu, Oct 10, 2013 at 12:54:40PM -0400, Matt Porter wrote: > > > > On 10/10/2013 11:29 AM, Felipe Balbi wrote: > > > > >On Mon, Oct 07, 2013 at 06:12:29AM -0400, Matt Porter wrote: > > > > >>Extend dwc2 binding with an optional utmi phy width property. > > > > >>Enable the s3c-hsotg.c driver to use standard dwc2 binding > > > > >>and enable configuration of the UTMI phy width based on the > > > > >>property. > > > > >> > > > > >>Signed-off-by: Matt Porter <matt.porter@linaro.org> > > > > >>Reviewed-by: Markus Mayer <markus.mayer@linaro.org> > > > > >>Reviewed-by: Tim Kryger <tim.kryger@linaro.org> > > > > >>--- > > > > >> Documentation/devicetree/bindings/staging/dwc2.txt | 4 ++++ > > > > >> drivers/usb/gadget/s3c-hsotg.c | 18 +++++++++++++++++- > > > > >> drivers/usb/gadget/s3c-hsotg.h | 1 + > > > > >> 3 files changed, 22 insertions(+), 1 deletion(-) > > > > >> > > > > >>diff --git a/Documentation/devicetree/bindings/staging/dwc2.txt > > > b/Documentation/devicetree/bindings/staging/dwc2.txt > > > > >>index 1a1b7cf..fb6b8ee 100644 > > > > >>--- a/Documentation/devicetree/bindings/staging/dwc2.txt > > > > >>+++ b/Documentation/devicetree/bindings/staging/dwc2.txt > > > > >>@@ -6,10 +6,14 @@ Required properties: > > > > >> - reg : Should contain 1 register range (address and length) > > > > >> - interrupts : Should contain 1 interrupt > > > > >> > > > > >>+Optional properties: > > > > >>+- snps,phy-utmi-width: Must contain the UTMI data width (either 8 or 16) > > > > > > > > > >isn't this available in any of the configuration registers ? > > > > > > > > Yes and no. HWCFG4 has a UTMI data width field. However, it has 3 > > > > valid states, "8", "16", or "8 or 16". The BCM281xx implementation is > > > > set to the latter and the attached phy is 8-bit. > > > > > > > > Looking at dwc2 prior to Matthijs Kooijman's patch [1] which starts > > > > validating the value of phy_utmi_width in that driver, the pci.c > > > > dwc2_module_params .phy_utmi_width field there even had the comment, > > > > "/* 16 bits - NOT DETECTABLE */". The autodetect code in > > > > dwc2_set_param_phy_utmi_width() will fail if HWCFG4 has the "8 or 16" > > > > option as it just decides to default to a phy width of 16 if nothing > > > > is configured by the platform glue. This property would also allow > > > > this issue to be addressed in that driver. > > > > > > fair enough, but I'd really like to hear from DT folks if your suggested > > > binding is acceptable. It seems like we can equally argue that it's a SW > > > configuration or HW description. > > > > It's definitely a HW description - the width of the UTMI data connection. > > right, but that description should be passed to the PHY, right ? DWC2 > works with both widths (8 or 16). I don't know what you mean by "passed to the PHY". The data width is the number of physical wires connecting the PHY to the DWC2 controller. If that value can be read from the PHY, fine. Otherwise, I think a DT binding is the way to go.
Hi, On Thu, Oct 10, 2013 at 06:14:19PM +0000, Paul Zimmerman wrote: > > From: Felipe Balbi [mailto:balbi@ti.com] > > Sent: Thursday, October 10, 2013 11:07 AM > > > > On Thu, Oct 10, 2013 at 05:57:54PM +0000, Paul Zimmerman wrote: > > > > From: Felipe Balbi [mailto:balbi@ti.com] > > > > Sent: Thursday, October 10, 2013 10:46 AM > > > > > > > > On Thu, Oct 10, 2013 at 12:54:40PM -0400, Matt Porter wrote: > > > > > On 10/10/2013 11:29 AM, Felipe Balbi wrote: > > > > > >On Mon, Oct 07, 2013 at 06:12:29AM -0400, Matt Porter wrote: > > > > > >>Extend dwc2 binding with an optional utmi phy width property. > > > > > >>Enable the s3c-hsotg.c driver to use standard dwc2 binding > > > > > >>and enable configuration of the UTMI phy width based on the > > > > > >>property. > > > > > >> > > > > > >>Signed-off-by: Matt Porter <matt.porter@linaro.org> > > > > > >>Reviewed-by: Markus Mayer <markus.mayer@linaro.org> > > > > > >>Reviewed-by: Tim Kryger <tim.kryger@linaro.org> > > > > > >>--- > > > > > >> Documentation/devicetree/bindings/staging/dwc2.txt | 4 ++++ > > > > > >> drivers/usb/gadget/s3c-hsotg.c | 18 +++++++++++++++++- > > > > > >> drivers/usb/gadget/s3c-hsotg.h | 1 + > > > > > >> 3 files changed, 22 insertions(+), 1 deletion(-) > > > > > >> > > > > > >>diff --git a/Documentation/devicetree/bindings/staging/dwc2.txt > > > > b/Documentation/devicetree/bindings/staging/dwc2.txt > > > > > >>index 1a1b7cf..fb6b8ee 100644 > > > > > >>--- a/Documentation/devicetree/bindings/staging/dwc2.txt > > > > > >>+++ b/Documentation/devicetree/bindings/staging/dwc2.txt > > > > > >>@@ -6,10 +6,14 @@ Required properties: > > > > > >> - reg : Should contain 1 register range (address and length) > > > > > >> - interrupts : Should contain 1 interrupt > > > > > >> > > > > > >>+Optional properties: > > > > > >>+- snps,phy-utmi-width: Must contain the UTMI data width (either 8 or 16) > > > > > > > > > > > >isn't this available in any of the configuration registers ? > > > > > > > > > > Yes and no. HWCFG4 has a UTMI data width field. However, it has 3 > > > > > valid states, "8", "16", or "8 or 16". The BCM281xx implementation is > > > > > set to the latter and the attached phy is 8-bit. > > > > > > > > > > Looking at dwc2 prior to Matthijs Kooijman's patch [1] which starts > > > > > validating the value of phy_utmi_width in that driver, the pci.c > > > > > dwc2_module_params .phy_utmi_width field there even had the comment, > > > > > "/* 16 bits - NOT DETECTABLE */". The autodetect code in > > > > > dwc2_set_param_phy_utmi_width() will fail if HWCFG4 has the "8 or 16" > > > > > option as it just decides to default to a phy width of 16 if nothing > > > > > is configured by the platform glue. This property would also allow > > > > > this issue to be addressed in that driver. > > > > > > > > fair enough, but I'd really like to hear from DT folks if your suggested > > > > binding is acceptable. It seems like we can equally argue that it's a SW > > > > configuration or HW description. > > > > > > It's definitely a HW description - the width of the UTMI data connection. > > > > right, but that description should be passed to the PHY, right ? DWC2 > > works with both widths (8 or 16). > > I don't know what you mean by "passed to the PHY". The data width is the > number of physical wires connecting the PHY to the DWC2 controller. If right, that's well understood. > that value can be read from the PHY, fine. Otherwise, I think a DT > binding is the way to go. Let's see what the DT folks have to say.
On 10/10/2013 01:57 PM, Paul Zimmerman wrote: >> From: Felipe Balbi [mailto:balbi@ti.com] >> Sent: Thursday, October 10, 2013 10:46 AM >> >> On Thu, Oct 10, 2013 at 12:54:40PM -0400, Matt Porter wrote: >>> On 10/10/2013 11:29 AM, Felipe Balbi wrote: >>>> On Mon, Oct 07, 2013 at 06:12:29AM -0400, Matt Porter wrote: >>>>> Extend dwc2 binding with an optional utmi phy width property. >>>>> Enable the s3c-hsotg.c driver to use standard dwc2 binding >>>>> and enable configuration of the UTMI phy width based on the >>>>> property. >>>>> >>>>> Signed-off-by: Matt Porter <matt.porter@linaro.org> >>>>> Reviewed-by: Markus Mayer <markus.mayer@linaro.org> >>>>> Reviewed-by: Tim Kryger <tim.kryger@linaro.org> >>>>> --- >>>>> Documentation/devicetree/bindings/staging/dwc2.txt | 4 ++++ >>>>> drivers/usb/gadget/s3c-hsotg.c | 18 +++++++++++++++++- >>>>> drivers/usb/gadget/s3c-hsotg.h | 1 + >>>>> 3 files changed, 22 insertions(+), 1 deletion(-) >>>>> >>>>> diff --git a/Documentation/devicetree/bindings/staging/dwc2.txt >> b/Documentation/devicetree/bindings/staging/dwc2.txt >>>>> index 1a1b7cf..fb6b8ee 100644 >>>>> --- a/Documentation/devicetree/bindings/staging/dwc2.txt >>>>> +++ b/Documentation/devicetree/bindings/staging/dwc2.txt >>>>> @@ -6,10 +6,14 @@ Required properties: >>>>> - reg : Should contain 1 register range (address and length) >>>>> - interrupts : Should contain 1 interrupt >>>>> >>>>> +Optional properties: >>>>> +- snps,phy-utmi-width: Must contain the UTMI data width (either 8 or 16) >>>> >>>> isn't this available in any of the configuration registers ? >>> >>> Yes and no. HWCFG4 has a UTMI data width field. However, it has 3 >>> valid states, "8", "16", or "8 or 16". The BCM281xx implementation is >>> set to the latter and the attached phy is 8-bit. >>> >>> Looking at dwc2 prior to Matthijs Kooijman's patch [1] which starts >>> validating the value of phy_utmi_width in that driver, the pci.c >>> dwc2_module_params .phy_utmi_width field there even had the comment, >>> "/* 16 bits - NOT DETECTABLE */". The autodetect code in >>> dwc2_set_param_phy_utmi_width() will fail if HWCFG4 has the "8 or 16" >>> option as it just decides to default to a phy width of 16 if nothing >>> is configured by the platform glue. This property would also allow >>> this issue to be addressed in that driver. >> >> fair enough, but I'd really like to hear from DT folks if your suggested >> binding is acceptable. It seems like we can equally argue that it's a SW >> configuration or HW description. > > It's definitely a HW description - the width of the UTMI data connection. > > But, which PHY is this? Does it have a register that could tell what > the data width is? The dwc2 core has an (optional) PHY Vendor Control > Register that allows reading the PHY registers. This is the integrated PHY on the bcm28155 part (the entire bcm281xx/11351 family shares it). My register doc doesn't show the optional PHY vendor control register as being present in this dwc2 implementation. I'll track down with the design team if this is accurate...but it appears we can't go that route. -Matt
On 10/10/2013 03:07 PM, Matt Porter wrote: > On 10/10/2013 01:57 PM, Paul Zimmerman wrote: >>> From: Felipe Balbi [mailto:balbi@ti.com] >>> Sent: Thursday, October 10, 2013 10:46 AM >>> >>> On Thu, Oct 10, 2013 at 12:54:40PM -0400, Matt Porter wrote: >>>> On 10/10/2013 11:29 AM, Felipe Balbi wrote: >>>>> On Mon, Oct 07, 2013 at 06:12:29AM -0400, Matt Porter wrote: >>>>>> Extend dwc2 binding with an optional utmi phy width property. >>>>>> Enable the s3c-hsotg.c driver to use standard dwc2 binding >>>>>> and enable configuration of the UTMI phy width based on the >>>>>> property. >>>>>> >>>>>> Signed-off-by: Matt Porter <matt.porter@linaro.org> >>>>>> Reviewed-by: Markus Mayer <markus.mayer@linaro.org> >>>>>> Reviewed-by: Tim Kryger <tim.kryger@linaro.org> >>>>>> --- >>>>>> Documentation/devicetree/bindings/staging/dwc2.txt | 4 ++++ >>>>>> drivers/usb/gadget/s3c-hsotg.c | 18 >>>>>> +++++++++++++++++- >>>>>> drivers/usb/gadget/s3c-hsotg.h | 1 + >>>>>> 3 files changed, 22 insertions(+), 1 deletion(-) >>>>>> >>>>>> diff --git a/Documentation/devicetree/bindings/staging/dwc2.txt >>> b/Documentation/devicetree/bindings/staging/dwc2.txt >>>>>> index 1a1b7cf..fb6b8ee 100644 >>>>>> --- a/Documentation/devicetree/bindings/staging/dwc2.txt >>>>>> +++ b/Documentation/devicetree/bindings/staging/dwc2.txt >>>>>> @@ -6,10 +6,14 @@ Required properties: >>>>>> - reg : Should contain 1 register range (address and length) >>>>>> - interrupts : Should contain 1 interrupt >>>>>> >>>>>> +Optional properties: >>>>>> +- snps,phy-utmi-width: Must contain the UTMI data width (either 8 >>>>>> or 16) >>>>> >>>>> isn't this available in any of the configuration registers ? >>>> >>>> Yes and no. HWCFG4 has a UTMI data width field. However, it has 3 >>>> valid states, "8", "16", or "8 or 16". The BCM281xx implementation is >>>> set to the latter and the attached phy is 8-bit. >>>> >>>> Looking at dwc2 prior to Matthijs Kooijman's patch [1] which starts >>>> validating the value of phy_utmi_width in that driver, the pci.c >>>> dwc2_module_params .phy_utmi_width field there even had the comment, >>>> "/* 16 bits - NOT DETECTABLE */". The autodetect code in >>>> dwc2_set_param_phy_utmi_width() will fail if HWCFG4 has the "8 or 16" >>>> option as it just decides to default to a phy width of 16 if nothing >>>> is configured by the platform glue. This property would also allow >>>> this issue to be addressed in that driver. >>> >>> fair enough, but I'd really like to hear from DT folks if your suggested >>> binding is acceptable. It seems like we can equally argue that it's a SW >>> configuration or HW description. >> >> It's definitely a HW description - the width of the UTMI data connection. >> >> But, which PHY is this? Does it have a register that could tell what >> the data width is? The dwc2 core has an (optional) PHY Vendor Control >> Register that allows reading the PHY registers. > > This is the integrated PHY on the bcm28155 part (the entire > bcm281xx/11351 family shares it). My register doc doesn't show the > optional PHY vendor control register as being present in this dwc2 > implementation. I'll track down with the design team if this is > accurate...but it appears we can't go that route. The design team confirmed that we do not have any registers that can be accessed via the PHY Vendor Control Register. All PHY control registers are implemented in the MMIO control block which is supported in the PHY control driver later in this series.They also confirmed there's no other way to detect that it's an 8-bit data path. -Matt
Hi, On Thu, Oct 10, 2013 at 11:21:30PM -0400, Matt Porter wrote: > On 10/10/2013 03:07 PM, Matt Porter wrote: > >On 10/10/2013 01:57 PM, Paul Zimmerman wrote: > >>>From: Felipe Balbi [mailto:balbi@ti.com] > >>>Sent: Thursday, October 10, 2013 10:46 AM > >>> > >>>On Thu, Oct 10, 2013 at 12:54:40PM -0400, Matt Porter wrote: > >>>>On 10/10/2013 11:29 AM, Felipe Balbi wrote: > >>>>>On Mon, Oct 07, 2013 at 06:12:29AM -0400, Matt Porter wrote: > >>>>>>Extend dwc2 binding with an optional utmi phy width property. > >>>>>>Enable the s3c-hsotg.c driver to use standard dwc2 binding > >>>>>>and enable configuration of the UTMI phy width based on the > >>>>>>property. > >>>>>> > >>>>>>Signed-off-by: Matt Porter <matt.porter@linaro.org> > >>>>>>Reviewed-by: Markus Mayer <markus.mayer@linaro.org> > >>>>>>Reviewed-by: Tim Kryger <tim.kryger@linaro.org> > >>>>>>--- > >>>>>> Documentation/devicetree/bindings/staging/dwc2.txt | 4 ++++ > >>>>>> drivers/usb/gadget/s3c-hsotg.c | 18 > >>>>>>+++++++++++++++++- > >>>>>> drivers/usb/gadget/s3c-hsotg.h | 1 + > >>>>>> 3 files changed, 22 insertions(+), 1 deletion(-) > >>>>>> > >>>>>>diff --git a/Documentation/devicetree/bindings/staging/dwc2.txt > >>>b/Documentation/devicetree/bindings/staging/dwc2.txt > >>>>>>index 1a1b7cf..fb6b8ee 100644 > >>>>>>--- a/Documentation/devicetree/bindings/staging/dwc2.txt > >>>>>>+++ b/Documentation/devicetree/bindings/staging/dwc2.txt > >>>>>>@@ -6,10 +6,14 @@ Required properties: > >>>>>> - reg : Should contain 1 register range (address and length) > >>>>>> - interrupts : Should contain 1 interrupt > >>>>>> > >>>>>>+Optional properties: > >>>>>>+- snps,phy-utmi-width: Must contain the UTMI data width (either 8 > >>>>>>or 16) > >>>>> > >>>>>isn't this available in any of the configuration registers ? > >>>> > >>>>Yes and no. HWCFG4 has a UTMI data width field. However, it has 3 > >>>>valid states, "8", "16", or "8 or 16". The BCM281xx implementation is > >>>>set to the latter and the attached phy is 8-bit. > >>>> > >>>>Looking at dwc2 prior to Matthijs Kooijman's patch [1] which starts > >>>>validating the value of phy_utmi_width in that driver, the pci.c > >>>>dwc2_module_params .phy_utmi_width field there even had the comment, > >>>>"/* 16 bits - NOT DETECTABLE */". The autodetect code in > >>>>dwc2_set_param_phy_utmi_width() will fail if HWCFG4 has the "8 or 16" > >>>>option as it just decides to default to a phy width of 16 if nothing > >>>>is configured by the platform glue. This property would also allow > >>>>this issue to be addressed in that driver. > >>> > >>>fair enough, but I'd really like to hear from DT folks if your suggested > >>>binding is acceptable. It seems like we can equally argue that it's a SW > >>>configuration or HW description. > >> > >>It's definitely a HW description - the width of the UTMI data connection. > >> > >>But, which PHY is this? Does it have a register that could tell what > >>the data width is? The dwc2 core has an (optional) PHY Vendor Control > >>Register that allows reading the PHY registers. > > > >This is the integrated PHY on the bcm28155 part (the entire > >bcm281xx/11351 family shares it). My register doc doesn't show the > >optional PHY vendor control register as being present in this dwc2 > >implementation. I'll track down with the design team if this is > >accurate...but it appears we can't go that route. > > The design team confirmed that we do not have any registers that can > be accessed via the PHY Vendor Control Register. All PHY control > registers are implemented in the MMIO control block which is > supported in the PHY control driver later in this series.They also > confirmed there's no other way to detect that it's an 8-bit data > path. then we need the DT binding, let's see what DT maintainers say.
On Fri, Oct 11, 2013 at 08:37:31AM -0500, Felipe Balbi wrote: > Hi, > > On Thu, Oct 10, 2013 at 11:21:30PM -0400, Matt Porter wrote: > > On 10/10/2013 03:07 PM, Matt Porter wrote: > > >On 10/10/2013 01:57 PM, Paul Zimmerman wrote: > > >>>From: Felipe Balbi [mailto:balbi@ti.com] > > >>>Sent: Thursday, October 10, 2013 10:46 AM > > >>> > > >>>On Thu, Oct 10, 2013 at 12:54:40PM -0400, Matt Porter wrote: > > >>>>On 10/10/2013 11:29 AM, Felipe Balbi wrote: > > >>>>>On Mon, Oct 07, 2013 at 06:12:29AM -0400, Matt Porter wrote: > > >>>>>>Extend dwc2 binding with an optional utmi phy width property. > > >>>>>>Enable the s3c-hsotg.c driver to use standard dwc2 binding > > >>>>>>and enable configuration of the UTMI phy width based on the > > >>>>>>property. > > >>>>>> > > >>>>>>Signed-off-by: Matt Porter <matt.porter@linaro.org> > > >>>>>>Reviewed-by: Markus Mayer <markus.mayer@linaro.org> > > >>>>>>Reviewed-by: Tim Kryger <tim.kryger@linaro.org> > > >>>>>>--- > > >>>>>> Documentation/devicetree/bindings/staging/dwc2.txt | 4 ++++ > > >>>>>> drivers/usb/gadget/s3c-hsotg.c | 18 > > >>>>>>+++++++++++++++++- > > >>>>>> drivers/usb/gadget/s3c-hsotg.h | 1 + > > >>>>>> 3 files changed, 22 insertions(+), 1 deletion(-) > > >>>>>> > > >>>>>>diff --git a/Documentation/devicetree/bindings/staging/dwc2.txt > > >>>b/Documentation/devicetree/bindings/staging/dwc2.txt > > >>>>>>index 1a1b7cf..fb6b8ee 100644 > > >>>>>>--- a/Documentation/devicetree/bindings/staging/dwc2.txt > > >>>>>>+++ b/Documentation/devicetree/bindings/staging/dwc2.txt > > >>>>>>@@ -6,10 +6,14 @@ Required properties: > > >>>>>> - reg : Should contain 1 register range (address and length) > > >>>>>> - interrupts : Should contain 1 interrupt > > >>>>>> > > >>>>>>+Optional properties: > > >>>>>>+- snps,phy-utmi-width: Must contain the UTMI data width (either 8 > > >>>>>>or 16) > > >>>>> > > >>>>>isn't this available in any of the configuration registers ? > > >>>> > > >>>>Yes and no. HWCFG4 has a UTMI data width field. However, it has 3 > > >>>>valid states, "8", "16", or "8 or 16". The BCM281xx implementation is > > >>>>set to the latter and the attached phy is 8-bit. > > >>>> > > >>>>Looking at dwc2 prior to Matthijs Kooijman's patch [1] which starts > > >>>>validating the value of phy_utmi_width in that driver, the pci.c > > >>>>dwc2_module_params .phy_utmi_width field there even had the comment, > > >>>>"/* 16 bits - NOT DETECTABLE */". The autodetect code in > > >>>>dwc2_set_param_phy_utmi_width() will fail if HWCFG4 has the "8 or 16" > > >>>>option as it just decides to default to a phy width of 16 if nothing > > >>>>is configured by the platform glue. This property would also allow > > >>>>this issue to be addressed in that driver. > > >>> > > >>>fair enough, but I'd really like to hear from DT folks if your suggested > > >>>binding is acceptable. It seems like we can equally argue that it's a SW > > >>>configuration or HW description. > > >> > > >>It's definitely a HW description - the width of the UTMI data connection. > > >> > > >>But, which PHY is this? Does it have a register that could tell what > > >>the data width is? The dwc2 core has an (optional) PHY Vendor Control > > >>Register that allows reading the PHY registers. > > > > > >This is the integrated PHY on the bcm28155 part (the entire > > >bcm281xx/11351 family shares it). My register doc doesn't show the > > >optional PHY vendor control register as being present in this dwc2 > > >implementation. I'll track down with the design team if this is > > >accurate...but it appears we can't go that route. > > > > The design team confirmed that we do not have any registers that can > > be accessed via the PHY Vendor Control Register. All PHY control > > registers are implemented in the MMIO control block which is > > supported in the PHY control driver later in this series.They also > > confirmed there's no other way to detect that it's an 8-bit data > > path. > > then we need the DT binding, let's see what DT maintainers say. I've reworked all of this against the generic phy layer. Things are simplified, of course, but this needs to be clarified to move forward on either the phy driver or the s3c-hsotg support. DT maintainers: can you weigh in here please? Summary is that we have a 8-bit data path UTMI phy attached to a dwc2. The width is undetectable and a fixed h/w configuration. I would actually view this as a property of the phy itself. It's 8-bit only and thus puts the constraint on the controller it's attached to (internally) to operate in 8-bit UTMI mode. However, I see no way with the generic phy binding to provide this information to the controller h/w. The original approach above adds a phy-utmi-width property to the dwc2 binding which is simple, but in retrospect may not reflect where that constraint actually comes into play. -Matt
diff --git a/Documentation/devicetree/bindings/staging/dwc2.txt b/Documentation/devicetree/bindings/staging/dwc2.txt index 1a1b7cf..fb6b8ee 100644 --- a/Documentation/devicetree/bindings/staging/dwc2.txt +++ b/Documentation/devicetree/bindings/staging/dwc2.txt @@ -6,10 +6,14 @@ Required properties: - reg : Should contain 1 register range (address and length) - interrupts : Should contain 1 interrupt +Optional properties: +- snps,phy-utmi-width: Must contain the UTMI data width (either 8 or 16) + Example: usb@101c0000 { compatible = "ralink,rt3050-usb, snps,dwc2"; reg = <0x101c0000 40000>; interrupts = <18>; + snps,phy-utmi-width = <8>; }; diff --git a/drivers/usb/gadget/s3c-hsotg.c b/drivers/usb/gadget/s3c-hsotg.c index 34d80f3..396afb3 100644 --- a/drivers/usb/gadget/s3c-hsotg.c +++ b/drivers/usb/gadget/s3c-hsotg.c @@ -137,6 +137,7 @@ struct s3c_hsotg_ep { * @regs: The memory area mapped for accessing registers. * @irq: The IRQ number we are using * @supplies: Definition of USB power supplies + * @phyif: PHY interface width * @dedicated_fifos: Set if the hardware has dedicated IN-EP fifos. * @num_of_eps: Number of available EPs (excluding EP0) * @debug_root: root directrory for debugfs. @@ -164,6 +165,7 @@ struct s3c_hsotg { struct regulator_bulk_data supplies[ARRAY_SIZE(s3c_hsotg_supply_names)]; + u32 phyif; unsigned int dedicated_fifos:1; unsigned char num_of_eps; @@ -2212,7 +2214,7 @@ static void s3c_hsotg_core_init(struct s3c_hsotg *hsotg) */ /* set the PLL on, remove the HNP/SRP and set the PHY */ - writel(GUSBCFG_PHYIf16 | GUSBCFG_TOutCal(7) | + writel(hsotg->phyif | GUSBCFG_TOutCal(7) | (0x5 << 10), hsotg->regs + GUSBCFG); s3c_hsotg_init_fifo(hsotg); @@ -3447,9 +3449,11 @@ static int s3c_hsotg_probe(struct platform_device *pdev) struct s3c_hsotg_plat *plat = dev_get_platdata(&pdev->dev); struct usb_phy *phy; struct device *dev = &pdev->dev; + struct device_node *np = dev->of_node; struct s3c_hsotg_ep *eps; struct s3c_hsotg *hsotg; struct resource *res; + u32 phy_utmi_width = 16; int epnum; int ret; int i; @@ -3539,6 +3543,17 @@ static int s3c_hsotg_probe(struct platform_device *pdev) goto err_supplies; } + /* Set default UTMI width */ + hsotg->phyif = GUSBCFG_PHYIf16; + if (np) { + of_property_read_u32(np, "snps,phy-utmi-width", + &phy_utmi_width); + if (phy_utmi_width == 8) + hsotg->phyif = GUSBCFG_PHYIf8; + if ((phy_utmi_width != 8) && (phy_utmi_width != 16)) + dev_warn(dev, "invalid snps,phy-utmi-width value\n"); + } + /* usb phy enable */ s3c_hsotg_phy_enable(hsotg); @@ -3645,6 +3660,7 @@ static int s3c_hsotg_remove(struct platform_device *pdev) #ifdef CONFIG_OF static const struct of_device_id s3c_hsotg_of_ids[] = { { .compatible = "samsung,s3c6400-hsotg", }, + { .compatible = "snps,dwc2", }, { /* sentinel */ } }; MODULE_DEVICE_TABLE(of, s3c_hsotg_of_ids); diff --git a/drivers/usb/gadget/s3c-hsotg.h b/drivers/usb/gadget/s3c-hsotg.h index d650b12..85f549f 100644 --- a/drivers/usb/gadget/s3c-hsotg.h +++ b/drivers/usb/gadget/s3c-hsotg.h @@ -55,6 +55,7 @@ #define GUSBCFG_HNPCap (1 << 9) #define GUSBCFG_SRPCap (1 << 8) #define GUSBCFG_PHYIf16 (1 << 3) +#define GUSBCFG_PHYIf8 (0 << 3) #define GUSBCFG_TOutCal_MASK (0x7 << 0) #define GUSBCFG_TOutCal_SHIFT (0) #define GUSBCFG_TOutCal_LIMIT (0x7)