Re: [PATCH 3/6] Documentation: devicetree: dwc3: Add interrupt moderation

[Mark Rutland]

On Tue, Nov 01, 2016 at 01:18:17PM +0200, Felipe Balbi wrote:
> 
> Hi,
> 
> John Youn  writes:
>  John Youn  writes:
> > Add interrupt moderation interval binding for dwc3.
> >
> > Signed-off-by: John Youn 
> > ---
> >  Documentation/devicetree/bindings/usb/dwc3.txt | 1 +
> >  1 file changed, 1 insertion(+)
> >
> > diff --git a/Documentation/devicetree/bindings/usb/dwc3.txt 
> > b/Documentation/devicetree/bindings/usb/dwc3.txt
> > index e3e6983..17de9fc 100644
> > --- a/Documentation/devicetree/bindings/usb/dwc3.txt
> > +++ b/Documentation/devicetree/bindings/usb/dwc3.txt
> > @@ -53,6 +53,7 @@ Optional properties:
> >   - snps,quirk-frame-length-adjustment: Value for GFLADJ_30MHZ field of 
> > GFLADJ
> > register for post-silicon frame length adjustment when the
> > fladj_30mhz_sdbnd signal is invalid or incorrect.
> > + - snps,imod_interval: the interrupt moderation interval.
> 
>  on top of all other comments, what's the unit here? nanoseconds? clock 
>  cycles?
> 
> >>>
> >>> Number of 250 ns intervals. I'll update the description to clarify.
> >> 
> >> it's probably better to add it in nanoseconds itself, then let driver
> >> compute register value with DIV_ROUND_UP()
> >> 
> >
> > I'm fine with it either way, but I think "increments of 250 ns" is
> > slightly cleaner in that it reflects the exact settings that are
> > possible and documented, and also fits neatly into a u16.
> 
> I don't know, I'll leave this to Mark and the other devicetree folks,
> but I remember there was a preference of not passing register values via
> devicetree. What if a following HW revision decides to change 250ns
> increments to 125ns increments?
> 
> Mark?

Generally, directly human-readable units are preferable, with some
validation/reporting left to the driver for cases the HW cannot support.
In this case, some *-interval-ns property would be better.

That said, it sounds like we're all agreed that we don't need this
property for now. :)

Thanks,
Mark.
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 3/6] Documentation: devicetree: dwc3: Add interrupt moderation

[Mark Rutland]

On Fri, Oct 28, 2016 at 01:30:07PM +0300, Felipe Balbi wrote:
> Mark Rutland <mark.rutl...@arm.com> writes:
> > On Thu, Oct 27, 2016 at 02:08:25PM -0700, John Youn wrote:
> >> On 10/26/2016 3:57 AM, Mark Rutland wrote:
> >> > On Tue, Oct 25, 2016 at 12:42:46PM -0700, John Youn wrote:
> >> >> Add interrupt moderation interval binding for dwc3.
> >
> >> >> + - snps,imod_interval: the interrupt moderation interval.

> >> This series implements the feature and enables it as a workaround for
> >> a particular version of the controller.
> >
> > ... as a workaround for *what*? Is there a bug in that IP version, or an
> 
> you didn't receive the entire series, I guess. Here's last patch in the
> series:

No, I did not. Thanks for forwarding this.

>  This is a workaround for STAR 9000961433 which affects only version
>  3.00a of the DWC_usb3 core. This prevents the controller interrupt from
>  being masked while handling events. Enabling interrupt moderation allows
>  us to work around this issue because once the GEVNTCOUNT.count is
>  written the IRQ is immediately deasserted and won't be asserted again
>  until GEVNTCOUNT.EHB is cleared.
> 
>  Signed-off-by: John Youn <johny...@synopsys.com>
>  ---
>   drivers/usb/dwc3/core.c | 12 
>   1 file changed, 12 insertions(+)
> 
>  diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
>  index 6733838..7fa0832 100644
>  --- a/drivers/usb/dwc3/core.c
>  +++ b/drivers/usb/dwc3/core.c
>  @@ -1050,6 +1050,18 @@ static void dwc3_check_params(struct dwc3 *dwc)
>dwc->imod_interval = 0;
>}
> 
>  +/*
>  + * Workaround for STAR 9000961433 which affects only version
>  + * 3.00a of the DWC_usb3 core. This prevents the controller
>  + * interrupt from being masked while handling events. IMOD
>  + * allows us to work around this issue. Enable it for the
>  + * affected version.
>  + */
>  +if (!dwc->imod_interval &&
>  +(dwc->revision == DWC3_REVISION_300A)) {
>  +dwc->imod_interval = 1;
>  +}
>  +
>/* Check the maximum_speed parameter */
>switch (dwc->maximum_speed) {
>case USB_SPEED_LOW:
> 
> > integration issue? Does the problem vary per-board?
> >
> > Generally, if there's a problem that needs to be worked around, we
> > describe the problem in the DT (perhaps implicitly in the compatible
> > string), and then the kernel chooses the workaround.
> 
> Regardless of the silicon erratum, interrupt moderation is a *feature*
> of the IP, common to all instances since revision v3.00a (IIRC). John is
> just using interrupt moderation in the context of implementing this
> workaround. But the actual feature is valid also without the erratum.

Sure, I understand this.

> Another thing to remember is that different applications (i.e. boards)
> might want to moderate the interrupt for different periods. That's,
> again, not related to the erratum at all.

... again, the question is *why*?

If this varies per use-case, then it would be better to handle this
dynamically -- people can run wildly different use-cases on the same
hardware.

I'm not sure that it makes sense for this to be in the DT, though I may
have misunderstood.

Thanks,
Mark.
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 3/6] Documentation: devicetree: dwc3: Add interrupt moderation

[Mark Rutland]

On Thu, Oct 27, 2016 at 02:08:25PM -0700, John Youn wrote:
> On 10/26/2016 3:57 AM, Mark Rutland wrote:
> > On Tue, Oct 25, 2016 at 12:42:46PM -0700, John Youn wrote:
> >> Add interrupt moderation interval binding for dwc3.

> >> + - snps,imod_interval: the interrupt moderation interval.

> > What is "interrupt moderation"? The cover mentions that this is to be
> > used for some kind of workaround, but it's not clear to me what this is,
> > and as such, whether it makes sense to describe it in this manner.
> > 
> 
> Interrupt moderation throttles the interrupt rate to be no faster than
> a specified interval. It's an optional feature of the controller.

Ok.

> This series implements the feature and enables it as a workaround for
> a particular version of the controller.

... as a workaround for *what*? Is there a bug in that IP version, or an
integration issue? Does the problem vary per-board?

Generally, if there's a problem that needs to be worked around, we
describe the problem in the DT (perhaps implicitly in the compatible
string), and then the kernel chooses the workaround.

Thanks,
Mark.
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 3/6] Documentation: devicetree: dwc3: Add interrupt moderation

[Mark Rutland]

On Tue, Oct 25, 2016 at 12:42:46PM -0700, John Youn wrote:
> Add interrupt moderation interval binding for dwc3.
> 
> Signed-off-by: John Youn 
> ---
>  Documentation/devicetree/bindings/usb/dwc3.txt | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/Documentation/devicetree/bindings/usb/dwc3.txt 
> b/Documentation/devicetree/bindings/usb/dwc3.txt
> index e3e6983..17de9fc 100644
> --- a/Documentation/devicetree/bindings/usb/dwc3.txt
> +++ b/Documentation/devicetree/bindings/usb/dwc3.txt
> @@ -53,6 +53,7 @@ Optional properties:
>   - snps,quirk-frame-length-adjustment: Value for GFLADJ_30MHZ field of GFLADJ
>   register for post-silicon frame length adjustment when the
>   fladj_30mhz_sdbnd signal is invalid or incorrect.
> + - snps,imod_interval: the interrupt moderation interval.

As otherwise commented, s/_/-/

What is "interrupt moderation"? The cover mentions that this is to be
used for some kind of workaround, but it's not clear to me what this is,
and as such, whether it makes sense to describe it in this manner.

Thanks,
Mark.

>  
>   -  tx-fifo-resize: determines if the FIFO *has* to be 
> reallocated.
>  
> -- 
> 2.10.0
> 
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/1] Add driver for smsc usb251x i2c configuration

[Mark Rutland]

Hi,

On Tue, Aug 02, 2016 at 04:31:54PM +0200, Fabien Lahoudere wrote:
> This driver copy the configuration of the controller EEPROM via i2c.
> Configuration information is available in Documentation/usb/usb251x.txt
> 
> Signed-off-by: Fabien Lahoudere 
> ---
>  Documentation/devicetree/bindings/usb/usb251x.txt |  27 +++
>  drivers/usb/misc/Kconfig  |   9 +
>  drivers/usb/misc/Makefile |   1 +
>  drivers/usb/misc/usb251x.c| 265 
> ++
>  4 files changed, 302 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/usb/usb251x.txt
>  create mode 100644 drivers/usb/misc/usb251x.c
> 
> diff --git a/Documentation/devicetree/bindings/usb/usb251x.txt 
> b/Documentation/devicetree/bindings/usb/usb251x.txt
> new file mode 100644
> index 000..2b0863a3
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/usb/usb251x.txt
> @@ -0,0 +1,27 @@
> +SMSC USB 2.0 Hi-Speed Hub Controller
> +
> +Required properties:
> +- compatible = "smsc,usb251x";

Please us specific compatible strings rather than wildcards.

> +- reg = <0x2c>;
> +
> +Optional properties:
> +- smsc,usb251x-cfg-data1 : u8, set configuration data 1 (byte 0x06)
> +- smsc,usb251x-cfg-data2 : u8, set configuration data 2 (byte 0x07)
> +- smsc,usb251x-cfg-data3 : u8, set configuration data 3 (byte 0x08)
> +- smsc,usb251x-portmap12 : u8, set port mapping for ports 1 and 2 (byte 0xfb)
> +- smsc,usb251x-portmap34 : u8, set port mapping for ports 3 and 4 (byte 0xfc)
> +- smsc,usb251x-portmap56 : u8, set port mapping for ports 5 and 6 (byte 0xfd)
> +- smsc,usb251x-portmap7 : u8, set port mapping for port 7 (byte 0xfe)
> +- smsc,usb251x-status-command : u8, configure smbus behaviour (byte 0xff)

For device tree bindings we generally shy away from encoding raw values
in this manner. I'm very much not keen on this as-is.

What exactly do these values represent?

Why must these be configured through DT? When should a dts author
provide them?

I have more comments on the representation below.

> +
> +Example:
> +
> + usb251x: usb251x@2c {
> + compatible = "smsc,usb251x";
> + reg = <0x2c>;
> + smsc,usb251x-cfg-data3 = <0x09>;
> + smsc,usb251x-portmap12 = <0x21>;
> + smsc,usb251x-portmap12 = <0x43>;
> + smsc,usb251x-status-command = <0x1>;
> + };

Above these were describes as u8 values, but here they're treated as u32
due to the lack of a /bits/ 8 prefix on the values. Trying to store them
as u8 saves no space whatsoever, given values are always padded to 32
bits.

[...]

> +static unsigned char default_init_table[USB251X_ADDR_SZ] = {
> + 0x24, 0x04, 0x14, 0x25, 0xa0, 0x0b, 0x9b, 0x20, /* 00 - 07 */
> + 0x02, 0x00, 0x00, 0x00, 0x01, 0x32, 0x01, 0x32, /* 08 - 0F */
> + 0x32, 0x00, 0x00, 4, 30, 0x00, 'S', 0x00,   /* 10 - 17 */
> + 'M', 0x00, 'S', 0x00, 'C', 0x00, 0x00, 0x00,/* 18 - 1F */
> + 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, /* 20 - 27 */
> + 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, /* 28 - 2F */
> + 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, /* 30 - 37 */
> + 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, /* 38 - 3F */
> + 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, /* 40 - 47 */
> + 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, /* 48 - 4F */
> + 0x00, 0x00, 0x00, 0x00, 'U', 0x00, 'S', 0x00,   /* 50 - 57 */
> + 'B', 0x00, ' ', 0x00, '2', 0x00, '.', 0x00, /* 58 - 5F */
> + '0', 0x00, ' ', 0x00, 'H', 0x00, 'i', 0x00, /* 60 - 67 */
> + '-', 0x00, 'S', 0x00, 'p', 0x00, 'e', 0x00, /* 68 - 6F */
> + 'e', 0x00, 'd', 0x00, ' ', 0x00, 'H', 0x00, /* 70 - 77 */
> + 'u', 0x00, 'b', 0x00, ' ', 0x00, 'C', 0x00, /* 78 - 7F */
> + 'o', 0x00, 'n', 0x00, 't', 0x00, 'r', 0x00, /* 80 - 87 */
> + 'o', 0x00, 'l', 0x00, 'l', 0x00, 'e', 0x00, /* 88 - 8F */
> + 'r', 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,  /* 90 - 97 */
> + 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, /* 98 - 9F */
> + 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, /* A0 - A7 */
> + 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, /* A8 - AF */
> + 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, /* B0 - B7 */
> + 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, /* B8 - BF */
> + 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, /* C0 - C7 */
> + 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, /* C8 - CF */
> + 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, /* D0 - D7 */
> + 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, /* D8 - DF */
> + 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, /* E0 - E7 */
> + 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, /* E8 - EF */
> + 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, /* F0 - F7 */
> + 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00  /* F8 - FF */
> +};

What exactly 

Re: [PATCH v3 1/2] Documentation: bindings: add DT documentation for Rockchip USB2PHY

[Mark Rutland]

On Mon, Jun 06, 2016 at 05:20:03PM +0800, Frank Wang wrote:
> Signed-off-by: Frank Wang 
> ---
> 
> Changes in v3:
>  - Added 'clocks' and 'clock-names' optional properties.
>  - Specified 'otg-port' and 'host-port' as the sub-node name.
> 
> Changes in v2:
>  - Changed vbus_host optional property from gpio to regulator.
>  - Specified vbus_otg-supply optional property.
>  - Specified otg_id and otg_bvalid property.
> 
>  .../bindings/phy/phy-rockchip-inno-usb2.txt|   60 
> 
>  1 file changed, 60 insertions(+)
>  create mode 100644 
> Documentation/devicetree/bindings/phy/phy-rockchip-inno-usb2.txt
> 
> diff --git a/Documentation/devicetree/bindings/phy/phy-rockchip-inno-usb2.txt 
> b/Documentation/devicetree/bindings/phy/phy-rockchip-inno-usb2.txt
> new file mode 100644
> index 000..0b4
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/phy/phy-rockchip-inno-usb2.txt
> @@ -0,0 +1,60 @@
> +ROCKCHIP USB2.0 PHY WITH INNO IP BLOCK
> +
> +Required properties (phy (parent) node):
> + - compatible : should be one of the listed compatibles:
> + * "rockchip,rk3366-usb2phy"
> + * "rockchip,rk3399-usb2phy"
> + - #clock-cells : should be 0.
> + - clock-output-names : specify the 480m output clock name.
> +
> +Optional properties:
> + - clocks : phandle + phy specifier pair, for the input clock of phy.
> + - clock-names : input clock name of phy, must be "phyclk".
> + - vbus_host-supply : phandle to a regulator that supplies host vbus.
> + - vbus_otg-supply : phandle to a regulator that supplies otg vbus.

Nit: s/_/-/ here.

Otherwise the rest of this looks generally fine, though I'm confused as
to how you address the programming interface(s), given none are
described.

Thanks,
Mark.

> +
> +Required nodes : a sub-node is required for each port the phy provides.
> +  The sub-node name is used to identify host or otg port,
> +  and shall be the following entries:
> + * "otg-port" : the name of otg port.
> + * "host-port" : the name of host port.
> +
> +Required properties (port (child) node):
> + - #phy-cells : must be 0. See ./phy-bindings.txt for details.
> + - interrupts : specify an interrupt for each entry in interrupt-names.
> + - interrupt-names : a list which shall be the following entries:
> + * "otg_id" : for the otg id interrupt.
> + * "otg_bvalid" : for the otg vbus interrupt.
> + * "linestate" : for the host/otg linestate interrupt.
> +
> +Example:
> +
> +grf: syscon@ff77 {
> + compatible = "rockchip,rk3366-grf", "syscon", "simple-mfd";
> + #address-cells = <1>;
> + #size-cells = <1>;
> +
> +...
> +
> + u2phy: usb2-phy {
> + compatible = "rockchip,rk3366-usb2phy";
> + #clock-cells = <0>;
> + clock-output-names = "sclk_otgphy0_480m";
> +
> + u2phy_otg: otg-port {
> + #phy-cells = <0>;
> + interrupts = ,
> +  ,
> +  ;
> + interrupt-names = "otg_id", "otg_bvalid", "linestate";
> + status = "okay";
> + };
> +
> + u2phy_host: host-port {
> + #phy-cells = <0>;
> + interrupts = ;
> + interrupt-names = "linestate";
> + status = "okay";
> + };
> + };
> +};
> -- 
> 1.7.9.5
> 
> 
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v4 2/5] dt-bindings: Add a binding for Mediatek xHCI host controller

[Mark Rutland]

Hi,

Sorry for my late reply to a prior version of this series, but I still
have concerns with some of the properties.

I'll repeat those below.

On Fri, Jul 31, 2015 at 02:03:53PM +0100, Chunfeng Yun wrote:
 add a DT binding documentation of xHCI host controller for the
 MT8173 SoC from Mediatek.
 
 Signed-off-by: Chunfeng Yun chunfeng@mediatek.com
 ---
  .../devicetree/bindings/usb/mt8173-xhci.txt| 51 
 ++
  1 file changed, 51 insertions(+)
  create mode 100644 Documentation/devicetree/bindings/usb/mt8173-xhci.txt
 
 diff --git a/Documentation/devicetree/bindings/usb/mt8173-xhci.txt 
 b/Documentation/devicetree/bindings/usb/mt8173-xhci.txt
 new file mode 100644
 index 000..364be5a
 --- /dev/null
 +++ b/Documentation/devicetree/bindings/usb/mt8173-xhci.txt
 @@ -0,0 +1,51 @@
 +MT65XX xhci
 +
 +The device node for Mediatek SOC usb3.0 host controller
 +
 +Required properties:
 + - compatible : Supports mediatek,mt8173-xhci

s/Supports/should contain/

 + - reg : Specifies physical base address and size of the registers,
 + the first one for MAC, the second for IPPC
 + - interrupts : Interrupt mode, number and trigger mode

Just specify what this logically corresponds to; the format of the
interrupts proeprty depends on the interrupt controller.

 + - power-domains : To enable usb's mtcmos

Please describe what this contains. I assume you exped a single power
domain to be described, covering the whole xHCI controller?

 + - vusb33-supply : Regulator of usb avdd3.3v
 + - clocks : Must support all clocks that xhci needs

- clocks: a list of phandle + clock-specifier pairs, one for each entry
  in clock-names.

 + - clock-names : Should be sys_mac for sys and mac clocks, and
 + wakeup_deb_p0, wakeup_deb_p1 for wakeup debounce control
 + clocks

This would be better as a list, e.g.

- clock-names: should contain:
  * sys_mac description here if necessary
  * wakeup_deb_p0 description here if necessary
  * wakeup_deb_p1 description here if necessary

Is sys_mac a single clock input line? Or is it jsut the case that a
single line feeds two inputs currently? if the latter they should be
separate entries.

 + - phys : List of PHY specifiers (used by generic PHY framework).

The bracketed part should go. The bidnigns should know nothing of Linux
internals.

 + - phy-names : Must be usb-phy0, usb-phy1,.., usb-phyN, based on
 + the number of PHYs as specified in @phys property.

This is useless.

You either don't need names, and can acquire the phys by index, or you
need names which correspond to those on the data sheet for the xHCI
controller.

 + - mediatek,usb-wakeup : To access usb wakeup control register

Please describe what this points to (I guess it's a syscon)/

 + - mediatek,wakeup-src : 1: Ip sleep wakeup mode; 2: line state wakeup
 + mode; others means don't enable wakeup source of usb

This sounds like a runtime decision.

_why_ do you think this needs to be in the DT?

 + - mediatek,u2port-num : The number should not greater than the number
 + of phys

This is useless. Either this is implied by the entries in the phys
property, or it should be a runtime decision.

Thanks,
Mark.

 +
 +Optional properties:
 + - vbus-supply : Reference to the VBUS regulator;
 +
 +Example:
 +usb: usb@1127 {
 + compatible = mediatek,mt8173-xhci;
 + reg = 0 0x1127 0 0x4000,
 +   0 0x1128 0 0x0800;
 + interrupts = GIC_SPI 115 IRQ_TYPE_LEVEL_LOW;
 + power-domains = scpsys MT8173_POWER_DOMAIN_USB;
 + clocks = topckgen CLK_TOP_USB30_SEL,
 +  pericfg CLK_PERI_USB0,
 +  pericfg CLK_PERI_USB1;
 + clock-names = sys_mac,
 +   wakeup_deb_p0,
 +   wakeup_deb_p1;
 + phys = u3phy 0, u3phy 1;
 + phy-names = usb-phy0, usb-phy1;
 + vusb33-supply = mt6397_vusb_reg;
 + vbus-supply = usb_p1_vbus;
 + usb3-lpm-capable;
 + mediatek,usb-wakeup = pericfg;
 + mediatek,wakeup-src = 1;
 + mediatek,u2port-num = 2;
 + status = okay;
 +};
 -- 
 1.8.1.1.dirty
 
--
To unsubscribe from this list: send the line unsubscribe linux-usb in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v3 2/5] dt-bindings: Add a binding for Mediatek xHCI host controller

[Mark Rutland]

Hi,

   + - mediatek,usb-wakeup: to access usb wakeup control register
  
  What exactly does this property imply?
  
 There are some control registers for usb wakeup which are put in another
 module, here to get the node of that module, and then use regmap and
 syscon to operate it.

Ok. You need to specify the type of this property (i.e. that it is a
phandle to a syscon node). The description makes it sound like a boolean.

 
   + - mediatek,wakeup-src: 1: ip sleep wakeup mode; 2: line state wakeup
   + mode; others means don't enable wakeup source of usb
  
  This sounds like configuration rather than a hardware property. Why do
  you think this needs to be in the DT?
  
 Yes, it's better to put it in the DT. 

That doesn't answer my question.

_why_ do you think this needs to be in the DT? What do you think is
better for it being there?

 
   + - mediatek,u2port-num: the number should not greater than the number
   + of phys
  
  What exactly does this property imply?
  
 On some platform, it only makes use of partial usb ports, so disable
 others to save power.

What exactly do you mean by partial USB ports?

If a phy isn't wired up, it won't be listed in the phys property, if it
is then disabling it sounds like a run-time decision.

Thanks,
Mark.
--
To unsubscribe from this list: send the line unsubscribe linux-usb in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v3 2/5] dt-bindings: Add a binding for Mediatek xHCI host controller

[Mark Rutland]

On Wed, Jul 22, 2015 at 03:05:42PM +0100, Chunfeng Yun wrote:
 add a DT binding documentation of xHCI host controller for the
 MT8173 SoC from Mediatek.
 
 Signed-off-by: Chunfeng Yun chunfeng@mediatek.com
 ---
  .../devicetree/bindings/usb/mt8173-xhci.txt| 50 
 ++
  1 file changed, 50 insertions(+)
  create mode 100644 Documentation/devicetree/bindings/usb/mt8173-xhci.txt
 
 diff --git a/Documentation/devicetree/bindings/usb/mt8173-xhci.txt 
 b/Documentation/devicetree/bindings/usb/mt8173-xhci.txt
 new file mode 100644
 index 000..94d
 --- /dev/null
 +++ b/Documentation/devicetree/bindings/usb/mt8173-xhci.txt
 @@ -0,0 +1,50 @@
 +MT65XX xhci
 +
 +The device node for Mediatek SOC usb3.0 host controller
 +
 +Required properties:
 + - compatible : supports mediatek,mt8173-xhci
 + - reg: Offset and length of registers

Your example has multiple reg entries.

Please list what each entry is, and the order you expect them in.

 + - interrupts : Interrupt mode, number and trigger mode
 + - power-domains: to enable usb's mtcmos
 + - vusb33-supply:  regulator of usb avdd3.3v
 + - clocks : must support all clocks that xhci needs
 + - clock-names: should be sys_mac for sys and mac clocks, and
 + wakeup_deb_p0, wakeup_deb_p1 for wakeup debounce control
 + clocks
 + - phys  : the phys that xhci will bind, currently supports up to two
 + phys, so phy index should not greater than one.
 + - phy-names : should be phy-X format, X equals to 0 or 1

This seems somewhat pointless.

 + - usb3-lpm-capable: supports USB3 LPM
 + - mediatek,usb-wakeup: to access usb wakeup control register

What exactly does this property imply?

 + - mediatek,wakeup-src: 1: ip sleep wakeup mode; 2: line state wakeup
 + mode; others means don't enable wakeup source of usb

This sounds like configuration rather than a hardware property. Why do
you think this needs to be in the DT?

 + - mediatek,u2port-num: the number should not greater than the number
 + of phys

What exactly does this property imply?

Mark.

 +
 +Optional properties:
 + - vbus-supply : reference to the VBUS regulator;
 +
 +Example:
 +usb: usb30@1127 {
 + compatible = mediatek,mt8173-xhci;
 + reg = 0 0x1127 0 0x4000,
 +   0 0x1128 0 0x0800;
 + interrupts = GIC_SPI 115 IRQ_TYPE_LEVEL_LOW;
 + power-domains = scpsys MT8173_POWER_DOMAIN_USB;
 + clocks = topckgen CLK_TOP_USB30_SEL,
 +  pericfg CLK_PERI_USB0,
 +  pericfg CLK_PERI_USB1;
 + clock-names = sys_mac,
 +   wakeup_deb_p0,
 +   wakeup_deb_p1;
 + phys = u3phy 0, u3phy 1;
 + phy-names = phy-0, phy-1;
 + vusb33-supply = mt6397_vusb_reg;
 + vbus-supply = usb_p1_vbus;
 + usb3-lpm-capable;
 + mediatek,usb-wakeup = pericfg;
 + mediatek,wakeup-src = 1;
 + mediatek,u2port-num = 2;
 + status = okay;
 +};
 -- 
 1.8.1.1.dirty
 
--
To unsubscribe from this list: send the line unsubscribe linux-usb in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] usb: renesas_usbhs: Revise the binding document about the dma-names

[Mark Rutland]

On Thu, Apr 09, 2015 at 01:17:44AM +0100, Yoshihiro Shimoda wrote:
 Hi Mark,
 
  
  On Wed, Apr 08, 2015 at 11:42:24AM +0100, Yoshihiro Shimoda wrote:
   Since the DT should describe the hardware (not the driver limitation),
   This patch revises the binding document about the dma-names to change
   simple numbering as ch%d instead of txn and rxn.
  
  The naming given in this patch looks more sensible to me.
 
 Thank you for your comment!
 
   Also this patch fixes the actual code of renesas_usbhs driver to handle
   the new dma-names.
  
   Signed-off-by: Yoshihiro Shimoda yoshihiro.shimoda...@renesas.com
   ---
This patch is based on Felipe's usb.bit / testing/next branch.
(commit id = bbc78c07a51f6fd29c227b1220a9016e585358ba)
  
  I take it the existing driver and binding haven't hit mainline, and
  therefore there are no users yet?
 
 That's correct. At the moment, nobody uses the dma-names in the node of
 renesas_usbhs yet.

Given that:

Acked-by: Mark Rutland mark.rutl...@arm.com

Thanks,
Mark.
--
To unsubscribe from this list: send the line unsubscribe linux-usb in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] usb: renesas_usbhs: Revise the binding document about the dma-names

[Mark Rutland]

On Wed, Apr 08, 2015 at 11:42:24AM +0100, Yoshihiro Shimoda wrote:
 Since the DT should describe the hardware (not the driver limitation),
 This patch revises the binding document about the dma-names to change
 simple numbering as ch%d instead of txn and rxn.

The naming given in this patch looks more sensible to me.

 Also this patch fixes the actual code of renesas_usbhs driver to handle
 the new dma-names.
 
 Signed-off-by: Yoshihiro Shimoda yoshihiro.shimoda...@renesas.com
 ---
  This patch is based on Felipe's usb.bit / testing/next branch.
  (commit id = bbc78c07a51f6fd29c227b1220a9016e585358ba)

I take it the existing driver and binding haven't hit mainline, and
therefore there are no users yet?

Mark.

 
  Geert is pointed out about this issue:
  https://www.mail-archive.com/devicetree@vger.kernel.org/msg68401.html
 
  .../devicetree/bindings/usb/renesas_usbhs.txt  |  6 ++
  drivers/usb/renesas_usbhs/fifo.c   | 24 
 ++
  2 files changed, 17 insertions(+), 13 deletions(-)
 
 diff --git a/Documentation/devicetree/bindings/usb/renesas_usbhs.txt 
 b/Documentation/devicetree/bindings/usb/renesas_usbhs.txt
 index dc2a18f..ddbe304 100644
 --- a/Documentation/devicetree/bindings/usb/renesas_usbhs.txt
 +++ b/Documentation/devicetree/bindings/usb/renesas_usbhs.txt
 @@ -15,10 +15,8 @@ Optional properties:
- phys: phandle + phy specifier pair
- phy-names: must be usb
- dmas: Must contain a list of references to DMA specifiers.
 -  - dma-names : Must contain a list of DMA names:
 -   - tx0 ... txn
 -   - rx0 ... rxn
 -- This n means DnFIFO in USBHS module.
 +  - dma-names : named ch%d, where %d is the channel number ranging from 
 zero
 +to the number of channels (DnFIFOs) minus one.
  
  Example:
   usbhs: usb@e659 {
 diff --git a/drivers/usb/renesas_usbhs/fifo.c 
 b/drivers/usb/renesas_usbhs/fifo.c
 index 8597cf9..bc23b4a 100644
 --- a/drivers/usb/renesas_usbhs/fifo.c
 +++ b/drivers/usb/renesas_usbhs/fifo.c
 @@ -1227,15 +1227,21 @@ static void usbhsf_dma_init_dt(struct device *dev, 
 struct usbhs_fifo *fifo,
  {
   char name[16];
  
 - snprintf(name, sizeof(name), tx%d, channel);
 - fifo-tx_chan = dma_request_slave_channel_reason(dev, name);
 - if (IS_ERR(fifo-tx_chan))
 - fifo-tx_chan = NULL;
 -
 - snprintf(name, sizeof(name), rx%d, channel);
 - fifo-rx_chan = dma_request_slave_channel_reason(dev, name);
 - if (IS_ERR(fifo-rx_chan))
 - fifo-rx_chan = NULL;
 + /*
 +  * To avoid complex handing for DnFIFOs, the driver uses each
 +  * DnFIFO as TX or RX direction (not bi-direction).
 +  * So, the driver uses odd channels for TX, even channels for RX.
 +  */
 + snprintf(name, sizeof(name), ch%d, channel);
 + if (channel  1) {
 + fifo-tx_chan = dma_request_slave_channel_reason(dev, name);
 + if (IS_ERR(fifo-tx_chan))
 + fifo-tx_chan = NULL;
 + } else {
 + fifo-rx_chan = dma_request_slave_channel_reason(dev, name);
 + if (IS_ERR(fifo-rx_chan))
 + fifo-rx_chan = NULL;
 + }
  }
  
  static void usbhsf_dma_init(struct usbhs_priv *priv, struct usbhs_fifo *fifo,
 -- 
 1.9.1
 
--
To unsubscribe from this list: send the line unsubscribe linux-usb in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v3 2/5] Documentation: dt-bindings: add dt binding info for hi6220

[Mark Rutland]

On Tue, Feb 10, 2015 at 07:50:24AM +, Zhangfei Gao wrote:
 Signed-off-by: Zhangfei Gao zhangfei@linaro.org
 ---
  .../devicetree/bindings/usb/hi6220-usb.txt | 49 
 ++
  1 file changed, 49 insertions(+)
  create mode 100644 Documentation/devicetree/bindings/usb/hi6220-usb.txt
 
 diff --git a/Documentation/devicetree/bindings/usb/hi6220-usb.txt 
 b/Documentation/devicetree/bindings/usb/hi6220-usb.txt
 new file mode 100644
 index 000..b8278de
 --- /dev/null
 +++ b/Documentation/devicetree/bindings/usb/hi6220-usb.txt
 @@ -0,0 +1,49 @@
 +Hisilicon hi6220 SoC USB controller
 +-
 +
 +usb controller is inherited from dwc2, refer dwc2.txt
 +-
 +
 +Required properties:
 +- compatible: hisilicon,hi6220-usb
 +Refer to dwc2.txt for dwc2 usb properties
 +
 +
 +PHY:
 +-
 +
 +Required properties:
 +- compatible: hisilicon,hi6220-usb-phy
 +- vcc-supply: phandle to the regulator that provides power to the PHY.
 +- clocks: phandle and clock specifier of the PHY clock.
 +- hisilicon,peripheral-syscon: phandle of syscon used to control peripheral.
 +- hisilicon,gpio-vbus: gpio of detecting vbus.
 +- hisilicon,gpio-id: gpio of detecting id.

These should be vbus-gpios and id-gpios respectively.

 +
 +Example:
 +
 + peripheral_ctrl: syscon@f703 {
 + compatible = syscon;
 + reg = 0x0 0xf703 0x0 0x1000;
 + };

We should have a real string for this in addition to syscon.

Thanks,
Mark.
--
To unsubscribe from this list: send the line unsubscribe linux-usb in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/5] usb: dwc3: exynos: Add support for SCLK present on Exynos7

[Mark Rutland]

On Tue, Sep 02, 2014 at 11:39:08AM +0100, Vivek Gautam wrote:
 Hi,
 
 
 On Fri, Aug 29, 2014 at 12:18 AM, Mark Rutland mark.rutl...@arm.com wrote:
  On Thu, Aug 28, 2014 at 09:01:56AM +0100, Vivek Gautam wrote:
  Exynos7 also has a separate special gate clock going to the IP
  apart from the usual AHB clock. So add support for the same.
 
  Signed-off-by: Vivek Gautam gautam.vi...@samsung.com
  ---
   drivers/usb/dwc3/dwc3-exynos.c |   16 
   1 file changed, 16 insertions(+)
 
  diff --git a/drivers/usb/dwc3/dwc3-exynos.c 
  b/drivers/usb/dwc3/dwc3-exynos.c
  index f9fb8ad..bab6395 100644
  --- a/drivers/usb/dwc3/dwc3-exynos.c
  +++ b/drivers/usb/dwc3/dwc3-exynos.c
  @@ -35,6 +35,7 @@ struct dwc3_exynos {
struct device   *dev;
 
struct clk  *clk;
  + struct clk  *sclk;
struct regulator*vdd33;
struct regulator*vdd10;
   };
  @@ -141,10 +142,17 @@ static int dwc3_exynos_probe(struct platform_device 
  *pdev)
return -EINVAL;
}
 
  + /* Exynos7 has a special gate clock going to this IP */
  + exynos-sclk = devm_clk_get(dev, usbdrd30_sclk);
  + if (IS_ERR(exynos-sclk))
  + dev_warn(dev, couldn't get sclk\n);
 
  Doesn't this introduce a pointless warning for Exynos SoCs other than
  Exynos7?
 
 True, it will introduce an unnecessary warning for non-Exynos7 systems.
 I initially thought of introducing a compatible check for Exynos7-dwc3, but 
 that
 way we may end up adding such checks for future SoCs which have similar
 controller but have some clock difference or some other small change, no ?

Perhaps. I don't know what your future hardware will look like.

Is the usbdrd30_sclk input unique to Exynos7, or was it previously there
but just without an input?

If the latter you could just reduce this to:

dev_info(dev, no sclk specified);

Mark.
--
To unsubscribe from this list: send the line unsubscribe linux-usb in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/5] usb: dwc3: exynos: Add support for SCLK present on Exynos7

[Mark Rutland]

On Thu, Aug 28, 2014 at 09:01:56AM +0100, Vivek Gautam wrote:
 Exynos7 also has a separate special gate clock going to the IP
 apart from the usual AHB clock. So add support for the same.
 
 Signed-off-by: Vivek Gautam gautam.vi...@samsung.com
 ---
  drivers/usb/dwc3/dwc3-exynos.c |   16 
  1 file changed, 16 insertions(+)
 
 diff --git a/drivers/usb/dwc3/dwc3-exynos.c b/drivers/usb/dwc3/dwc3-exynos.c
 index f9fb8ad..bab6395 100644
 --- a/drivers/usb/dwc3/dwc3-exynos.c
 +++ b/drivers/usb/dwc3/dwc3-exynos.c
 @@ -35,6 +35,7 @@ struct dwc3_exynos {
   struct device   *dev;
  
   struct clk  *clk;
 + struct clk  *sclk;
   struct regulator*vdd33;
   struct regulator*vdd10;
  };
 @@ -141,10 +142,17 @@ static int dwc3_exynos_probe(struct platform_device 
 *pdev)
   return -EINVAL;
   }
  
 + /* Exynos7 has a special gate clock going to this IP */
 + exynos-sclk = devm_clk_get(dev, usbdrd30_sclk);
 + if (IS_ERR(exynos-sclk))
 + dev_warn(dev, couldn't get sclk\n);

Doesn't this introduce a pointless warning for Exynos SoCs other than
Exynos7?

 +
   exynos-dev = dev;
   exynos-clk = clk;
  
   clk_prepare_enable(exynos-clk);
 + if (!IS_ERR(exynos-sclk))
 + clk_prepare_enable(exynos-sclk);

If you replaced the returned err value with NULL you could avoid these
IS_ERR cases.

Mark.

  
   exynos-vdd33 = devm_regulator_get(dev, vdd33);
   if (IS_ERR(exynos-vdd33)) {
 @@ -187,6 +195,8 @@ err4:
  err3:
   regulator_disable(exynos-vdd33);
  err2:
 + if (!IS_ERR(exynos-sclk))
 + clk_disable_unprepare(exynos-sclk);
   clk_disable_unprepare(clk);
   return ret;
  }
 @@ -199,6 +209,8 @@ static int dwc3_exynos_remove(struct platform_device 
 *pdev)
   platform_device_unregister(exynos-usb2_phy);
   platform_device_unregister(exynos-usb3_phy);
  
 + if (!IS_ERR(exynos-sclk))
 + clk_disable_unprepare(exynos-sclk);
   clk_disable_unprepare(exynos-clk);
  
   regulator_disable(exynos-vdd33);
 @@ -220,6 +232,8 @@ static int dwc3_exynos_suspend(struct device *dev)
  {
   struct dwc3_exynos *exynos = dev_get_drvdata(dev);
  
 + if (!IS_ERR(exynos-sclk))
 + clk_disable(exynos-sclk);
   clk_disable(exynos-clk);
  
   regulator_disable(exynos-vdd33);
 @@ -245,6 +259,8 @@ static int dwc3_exynos_resume(struct device *dev)
   }
  
   clk_enable(exynos-clk);
 + if (!IS_ERR(exynos-sclk))
 + clk_enable(exynos-sclk);
  
   /* runtime set active to reflect active state. */
   pm_runtime_disable(dev);
 -- 
 1.7.10.4
 
 --
 To unsubscribe from this list: send the line unsubscribe devicetree in
 the body of a message to majord...@vger.kernel.org
 More majordomo info at  http://vger.kernel.org/majordomo-info.html
 
--
To unsubscribe from this list: send the line unsubscribe linux-usb in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 2/5] phy: exynos5-usbdrd: Add pipe-clk and utmi-clk support

[Mark Rutland]

On Thu, Aug 28, 2014 at 09:01:57AM +0100, Vivek Gautam wrote:
 Exynos7 SoC has now separate gate control for 125MHz pipe3 phy
 clock, as well as 60MHz utmi phy clock.
 So get the same and control in the phy-exynos5-usbdrd driver.
 
 Signed-off-by: Vivek Gautam gautam.vi...@samsung.com
 ---
  .../devicetree/bindings/phy/samsung-phy.txt|4 
  drivers/phy/phy-exynos5-usbdrd.c   |   24 
 
  2 files changed, 28 insertions(+)
 
 diff --git a/Documentation/devicetree/bindings/phy/samsung-phy.txt 
 b/Documentation/devicetree/bindings/phy/samsung-phy.txt
 index 7a6feea..b64d616 100644
 --- a/Documentation/devicetree/bindings/phy/samsung-phy.txt
 +++ b/Documentation/devicetree/bindings/phy/samsung-phy.txt
 @@ -135,6 +135,10 @@ Required properties:
  PHY operations, associated by phy name. It is used to
  determine bit values for clock settings register.
  For Exynos5420 this is given as 'sclk_usbphy30' in CMU.
 + - optional clocks: Next gen Exynos SoCs have following additional

It's not going to be 'Next gen' for long...

 +gate clocks available:
 +- phy_pipe: for PIPE3 phy
 +- phy_utmi: for UTMI+ phy
  - samsung,pmu-syscon: phandle for PMU system controller interface, used to
 control pmu registers for power isolation.
  - #phy-cells : from the generic PHY bindings, must be 1;
 diff --git a/drivers/phy/phy-exynos5-usbdrd.c 
 b/drivers/phy/phy-exynos5-usbdrd.c
 index b05302b..685c108 100644
 --- a/drivers/phy/phy-exynos5-usbdrd.c
 +++ b/drivers/phy/phy-exynos5-usbdrd.c
 @@ -148,6 +148,8 @@ struct exynos5_usbdrd_phy_drvdata {
   * @dev: pointer to device instance of this platform device
   * @reg_phy: usb phy controller register memory base
   * @clk: phy clock for register access
 + * @pipeclk: clock for pipe3 phy
 + * @utmiclk: clock for utmi+ phy
   * @drv_data: pointer to SoC level driver data structure
   * @phys[]: array for 'EXYNOS5_DRDPHYS_NUM' number of PHY
   *   instances each with its 'phy' and 'phy_cfg'.
 @@ -161,6 +163,8 @@ struct exynos5_usbdrd_phy {
   struct device *dev;
   void __iomem *reg_phy;
   struct clk *clk;
 + struct clk *pipeclk;
 + struct clk *utmiclk;
   const struct exynos5_usbdrd_phy_drvdata *drv_data;
   struct phy_usb_instance {
   struct phy *phy;
 @@ -446,6 +450,10 @@ static int exynos5_usbdrd_phy_power_on(struct phy *phy)
  
   dev_dbg(phy_drd-dev, Request to power_on usbdrd_phy phy\n);
  
 + if (!IS_ERR(phy_drd-utmiclk))
 + clk_prepare_enable(phy_drd-utmiclk);
 + if (!IS_ERR(phy_drd-pipeclk))
 + clk_prepare_enable(phy_drd-pipeclk);
   clk_prepare_enable(phy_drd-ref_clk);
  
   /* Enable VBUS supply */
 @@ -464,6 +472,10 @@ static int exynos5_usbdrd_phy_power_on(struct phy *phy)
  
  fail_vbus:
   clk_disable_unprepare(phy_drd-ref_clk);
 + if (!IS_ERR(phy_drd-pipeclk))
 + clk_disable_unprepare(phy_drd-pipeclk);
 + if (!IS_ERR(phy_drd-utmiclk))
 + clk_disable_unprepare(phy_drd-utmiclk);
  
   return ret;
  }
 @@ -483,6 +495,10 @@ static int exynos5_usbdrd_phy_power_off(struct phy *phy)
   regulator_disable(phy_drd-vbus);
  
   clk_disable_unprepare(phy_drd-ref_clk);
 + if (!IS_ERR(phy_drd-pipeclk))
 + clk_disable_unprepare(phy_drd-pipeclk);
 + if (!IS_ERR(phy_drd-utmiclk))
 + clk_disable_unprepare(phy_drd-utmiclk);
  
   return 0;
  }
 @@ -581,6 +597,14 @@ static int exynos5_usbdrd_phy_probe(struct 
 platform_device *pdev)
   return PTR_ERR(phy_drd-clk);
   }
  
 + phy_drd-pipeclk = devm_clk_get(dev, phy_pipe);
 + if (IS_ERR(phy_drd-pipeclk))
 + dev_warn(dev, Failed to get pipe3 phy operational clock\n);
 +
 + phy_drd-utmiclk = devm_clk_get(dev, phy_utmi);
 + if (IS_ERR(phy_drd-utmiclk))
 + dev_warn(dev, Failed to get utmi phy operational clock\n);
 +

Pointless warnings for !Exynos7?

Would it not be better to set these to NULL and not litter the code with
IS_ERR checks?

Mark.
--
To unsubscribe from this list: send the line unsubscribe linux-usb in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 0/4] Tegra USB probe order issue fix

[Mark Rutland]

On Fri, Jul 04, 2014 at 02:09:35AM +0100, Tuomas Tynkkynen wrote:
 Hi all,
 
 Here's a second version of the probe order issue series. This time I've
 added the USB1 resets to the PHYs, thus replacing the really ugly parts
 with something slightly better. Old device trees will still probe
 successfully, but instead of this bugfix they'll get a dev_warn().

Given the binding looks sane and doesn't unnecessarily break existing
DTBs, for patches 1-3:

Acked-by: Mark Rutland mark.rutl...@arm.com

Thanks,
Mark.
--
To unsubscribe from this list: send the line unsubscribe linux-usb in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v3 1/2] usb: doc: udc-xilinx: Add devicetree bindings

[Mark Rutland]

On Tue, Jun 24, 2014 at 07:44:10AM +0100, sundeep subbaraya wrote:
 Ping
 
 Thanks,
 Sundeep.B.S.
 
 On Tue, Jun 10, 2014 at 5:34 PM,  subbaraya.sundeep.bha...@xilinx.com wrote:
  From: Subbaraya Sundeep Bhatta sbha...@xilinx.com
 
  Add devicetree bindings for Xilinx axi udc driver.
 
  Signed-off-by: Subbaraya Sundeep Bhatta sbha...@xilinx.com
  ---
  Changes for v3:
  - None
  Changes for v2:
  - replaced xlnx,include-dma with xlnx,has-builtin-dma
 
   .../devicetree/bindings/usb/udc-xilinx.txt |   20 
  
   1 files changed, 20 insertions(+), 0 deletions(-)
   create mode 100644 Documentation/devicetree/bindings/usb/udc-xilinx.txt
 
  diff --git a/Documentation/devicetree/bindings/usb/udc-xilinx.txt 
  b/Documentation/devicetree/bindings/usb/udc-xilinx.txt
  new file mode 100644
  index 000..7c24fac
  --- /dev/null
  +++ b/Documentation/devicetree/bindings/usb/udc-xilinx.txt
  @@ -0,0 +1,20 @@
  +Xilinx AXI USB2 device controller
  +
  +Required properties:
  +- compatible   : Should be xlnx,axi-usb2-device-4.00.a

That's an interesting name. What's the product name? Is the a manual?

  +- reg  : Physical base address and size of the Axi USB2
  + device registers map.
  +- interrupts   : Property with a value describing the interrupt
  + number.

Describe what this logically is. Everyone knows that this is a property
with a value, and everyone knows that an interrupts property describes
interrupts.

The ket point is _which_ interrupts this describes.

  +- interrupt-parent : Must be core interrupt controller

This isn't strictly necessary anyway.

  +- xlnx,include-dma : if DMA is included

You appear to have forgotten to fix this name, judging by the changelog
and example.

  +
  +Example:
  +   axi-usb2-device@42e0 {
  +compatible = xlnx,axi-usb2-device-4.00.a;
  +interrupt-parent = 0x1;

Huh? Who writes a raw phandle value?

  +interrupts = 0x0 0x39 0x1;
  +reg = 0x42e0 0x1;
  +xlnx,has-builtin-dma;

As mentioned above, this conflicts with the binding documentation.
Please ensure the example is aligned with the documentation.

Thanks,
Mark.
--
To unsubscribe from this list: send the line unsubscribe linux-usb in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 2/3] usb: gadget: pxa27x_udc device-tree documentation

[Mark Rutland]

On Sun, Jun 29, 2014 at 10:29:49AM +0100, Robert Jarzmik wrote:
 Mark Rutland mark.rutl...@arm.com writes:
 
  On Wed, Jun 25, 2014 at 08:54:01PM +0100, Robert Jarzmik wrote:
   The name of the clock input doesn't make sense.
  I don't understand. With [1] does it make any more sense ? If not you'll 
  have to
  expand a bit more the doesn't make sense.
 
  My concern is that clock-names is supposed to describe the name of the
  input clock line from the view of the IP block. pxa27x-udc doesn't
  sound like the name of a clock input line from the view of the UDC
  block.
 
  I assume the clock input line you care about has a more specific name
  than pxa27x-udc?
 Not as far as I know. The technical reference manual call it udc clock, so
 it's even less specific ...

Not from the point of view of the device. The clock-names are namespaced
to the particular binding, so they're equally specific.

Given the above I'd recommend naming the clock udc or udc_clk. That
doesn't pretend to be overly specific, and matches the TRM.

 Marvell engineers have probably the internal schematics and the name of the
 clock, but outsiders like me only have udc ...

Sure, not having the full specs is always a pain.

Mark.
--
To unsubscribe from this list: send the line unsubscribe linux-usb in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 2/3] usb: gadget: pxa27x_udc device-tree documentation

[Mark Rutland]

On Wed, Jun 25, 2014 at 08:54:01PM +0100, Robert Jarzmik wrote:
 Mark Rutland mark.rutl...@arm.com writes:
 
  On Sun, Jun 22, 2014 at 10:04:58AM +0100, Robert Jarzmik wrote:
  index 79729a9..0e4863f 100644
  --- a/Documentation/devicetree/bindings/usb/pxa-usb.txt
  +++ b/Documentation/devicetree/bindings/usb/pxa-usb.txt
  @@ -29,3 +29,23 @@ Example:
 marvell,port-mode = 2; /* PMM_GLOBAL_MODE */
 };
   
  +UDC
  +
  +Required properties:
  + - compatible: Should be marvell,pxa27x-udc for USB controllers
  +   used in device mode.
 
  We typically don't like wildcard compatible strings in DT.
 Same as in the other patch, marvell,pxa270-udc.
 
  +
  +Optional properties:
  + - udc-pullup-gpio - gpio activated to control the USB D+ pullup.
  + - udc-pullup-gpio-inverted - boolean inverting gpio pullup logic.
 
  Use the GPIO bindings.
 OK. I'm presuming in this case you think of something like :
   udc-pullup-gpio = gpio 22 GPIO_ACTIVE_LOW
 Which translates into:
   udc-pullup-gpio - gpio activated to control the USB D+ pullup (see
 gpio.txt)

Something like that, yes.

 
  +
  +Example:
  +
  +  pxa27x_udc: udc@4060 {
  +  compatible = marvell,pxa27x-udc;
  +  reg = 0x4060 0x1;
  +  interrupts = 11;
  +  clocks = pxa2xx_clks 11;
  +  clock-names = pxa27x-udc;
 
  Neither of these were mentioned above.
 Ah you mean I shall describe the standard reg, interrupts as mandatory 
 standard
 options I take it. OK.

Yes. While the properties are standard, their precise meanings (e.g.
which interrupt or address space region), and whether a particular
binding uses them is not.

  The name of the clock input doesn't make sense.
 I don't understand. With [1] does it make any more sense ? If not you'll have 
 to
 expand a bit more the doesn't make sense.

My concern is that clock-names is supposed to describe the name of the
input clock line from the view of the IP block. pxa27x-udc doesn't
sound like the name of a clock input line from the view of the UDC
block.

I assume the clock input line you care about has a more specific name
than pxa27x-udc?

Cheers,
Mark.
--
To unsubscribe from this list: send the line unsubscribe linux-usb in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 2/3] usb: gadget: pxa27x_udc device-tree documentation

[Mark Rutland]

On Sun, Jun 22, 2014 at 10:04:58AM +0100, Robert Jarzmik wrote:
 Add documentation for device-tree binding of arm PXA 27x udc (usb
 device) driver.
 
 Signed-off-by: Robert Jarzmik robert.jarz...@free.fr
 Cc: devicet...@vger.kernel.org
 
 ---
 Since V1: change OF id mrvl,pxa27x_udc - marvell,pxa27x-udc
   This is a consequence of other DT reviews on the marvell
   namings.
 ---
  Documentation/devicetree/bindings/usb/pxa-usb.txt | 20 
  1 file changed, 20 insertions(+)
 
 diff --git a/Documentation/devicetree/bindings/usb/pxa-usb.txt 
 b/Documentation/devicetree/bindings/usb/pxa-usb.txt
 index 79729a9..0e4863f 100644
 --- a/Documentation/devicetree/bindings/usb/pxa-usb.txt
 +++ b/Documentation/devicetree/bindings/usb/pxa-usb.txt
 @@ -29,3 +29,23 @@ Example:
   marvell,port-mode = 2; /* PMM_GLOBAL_MODE */
   };
  
 +UDC
 +
 +Required properties:
 + - compatible: Should be marvell,pxa27x-udc for USB controllers
 +   used in device mode.

We typically don't like wildcard compatible strings in DT.

 +
 +Optional properties:
 + - udc-pullup-gpio - gpio activated to control the USB D+ pullup.
 + - udc-pullup-gpio-inverted - boolean inverting gpio pullup logic.

Use the GPIO bindings.

 +
 +Example:
 +
 + pxa27x_udc: udc@4060 {
 + compatible = marvell,pxa27x-udc;
 + reg = 0x4060 0x1;
 + interrupts = 11;
 + clocks = pxa2xx_clks 11;
 + clock-names = pxa27x-udc;

Neither of these were mentioned above.

The name of the clock input doesn't make sense.

Thanks,
Mark.
--
To unsubscribe from this list: send the line unsubscribe linux-usb in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 1/3] usb: gadget: pxa27x_udc: add devicetree support

[Mark Rutland]

On Sun, Jun 22, 2014 at 10:04:57AM +0100, Robert Jarzmik wrote:
 Add support for device-tree device discovery. If devicetree is not
 provided, fallback to legacy platform data discovery.
 
 Signed-off-by: Robert Jarzmik robert.jarz...@free.fr
 Cc: devicet...@vger.kernel.org
 
 ---
 Since V1: change OF id mrvl,pxa27x_udc - marvell,pxa27x-udc
   This is a consequence of other DT reviews on the marvell
   namings.
 ---
  drivers/usb/gadget/pxa27x_udc.c | 105 
 
  drivers/usb/gadget/pxa27x_udc.h |   4 ++
  2 files changed, 90 insertions(+), 19 deletions(-)

[...]

 +/**
 + * pxa_udc_probe_dt - device tree specific probe
 + * @pdev: platform data
 + * @udc: pxa_udc structure to fill
 + *
 + * Fills udc from platform data out of device tree.
 + *
 + * Returns 0 if DT found, 1 if DT not found, and 0 on error

That's impossible as this function is marked as returning bool.

Make this an int and return negative error codes.

 + */
 +bool pxa_udc_probe_dt(struct platform_device *pdev, struct pxa_udc *udc)
 +{
 + struct device_node *np = pdev-dev.of_node;
 + const struct of_device_id *of_id =
 + of_match_device(udc_pxa_dt_ids, pdev-dev);
 + int ret;
 + u32 gpio_pullup;
 +
 + if (!np || !of_id)
 + return 1;
 + ret = of_alias_get_id(np, udc);
 + pdev-id = (ret = 0) ? ret : -1;

The alias name wasn't mentioned in the binding...

 +
 + if (!of_property_read_u32(np, udc-pullup-gpio, gpio_pullup))
 + udc-gpio_pullup = gpio_pullup;

This number is a Linux internal detail. Use the GPIO bindings.

 + udc-gpio_pullup_inverted =
 + of_property_read_bool(np, udc-pullup-gpio-inverted);

The GPIO bindings can describe this.

 @@ -2415,7 +2469,13 @@ static int pxa_udc_probe(struct platform_device *pdev)
  {
   struct resource *regs;
   struct pxa_udc *udc = memory;
 - int retval = 0, gpio;
 + int retval = 0;
 +
 + retval = pxa_udc_probe_dt(pdev, udc);
 + if (retval  0)
 + return retval;

This case can never happen given the prototype of pxa_udc_probe_dt.

 @@ -2446,6 +2504,9 @@ static int pxa_udc_probe(struct platform_device *pdev)
   retval = PTR_ERR(udc-clk);
   goto err_clk;
   }
 + retval = clk_prepare(udc-clk);
 + if (retval)
 + goto err_clk_prepare;
  
   retval = -ENOMEM;
   udc-regs = ioremap(regs-start, resource_size(regs));
 @@ -2483,9 +2544,13 @@ err_add_udc:
  err_irq:
   iounmap(udc-regs);
  err_map:
 + clk_unprepare(udc-clk);
 +err_clk_prepare:
   clk_put(udc-clk);
   udc-clk = NULL;
  err_clk:
 + if (gpio_is_valid(udc-gpio_pullup))
 + gpio_free(udc-gpio_pullup);
   return retval;
  }
  
 @@ -2509,6 +2574,7 @@ static int pxa_udc_remove(struct platform_device *_dev)
  
   udc-transceiver = NULL;
   the_controller = NULL;
 + clk_unprepare(udc-clk);
   clk_put(udc-clk);

I don't see why these clock changes should be in the same patch as the
DT support. They might be a prerequisite, but as far as I can tell they
are required even without DT probing.

Mark.
--
To unsubscribe from this list: send the line unsubscribe linux-usb in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] phy-rcar-gen2-usb: add device tree support

[Mark Rutland]

On Thu, Feb 27, 2014 at 12:12:50AM +, Sergei Shtylyov wrote:
 Add support of the device tree probing for the Renesas R-Car generation 2 SoCs
 documenting the device tree binding as necessary.
 
 Signed-off-by: Sergei Shtylyov sergei.shtyl...@cogentembedded.com
 
 ---
 This patch is against the 'next' branch of Felipe Balbi's 'usb.git' repo.
 
  Documentation/devicetree/bindings/usb/rcar-gen2-phy.txt |   29 +++
  drivers/usb/phy/phy-rcar-gen2-usb.c |   64 
 ++--
  2 files changed, 85 insertions(+), 8 deletions(-)
 
 Index: usb/Documentation/devicetree/bindings/usb/rcar-gen2-phy.txt
 ===
 --- /dev/null
 +++ usb/Documentation/devicetree/bindings/usb/rcar-gen2-phy.txt
 @@ -0,0 +1,29 @@
 +* Renesas R-Car generation 2 USB PHY
 +
 +This file provides information on what the device node for the R-Car 
 generation
 +2 USB PHY contains.
 +
 +Required properties:
 +- compatible: renesas,usb-phy-r8a7790 if the device is a part of R8A7790 
 SoC.
 +   renesas,usb-phy-r8a7791 if the device is a part of R8A7791 SoC.

Is the r8a7791's USB PHY known to be different to that of the r8a7790?

If this is just to possibly handle the two differently in future, why
not have renesas,usb-phy-r8a7790 as a fallback in the compatible list?
That was you only need it in the driver for now.

 +- reg: offset and length of the register block.
 +- clocks: clock phandle and specifier pair.
 +- clock-names: string, clock input name, must be usbhs.
 +
 +Optional properties:
 +- renesas,channel0-pci: boolean, specify when USB channel 0 should be 
 connected
 + to PCI EHCI/OHCI; otherwise, it will be connected to the
 + USBHS controller.
 +- renesas,channel2-pci: boolean, specify when USB channel 2 should be 
 connected
 + to PCI EHCI/OHCI; otherwise, it will be connected to the
 + USBSS controller (xHCI).

When would you want this to connect to PCI, and when would you not? Why
is this not a run-time decision?

 +
 +Example (Lager board):
 +
 + usb-phy@e6590100 {
 + compatible = renesas,usb-phy-r8a7790;
 + reg = 0 0xe6590100 0 0x100;
 + clocks = mstp7_clks R8A7790_CLK_HSUSB;
 + clock-names = usbhs;
 + renesas,channel2-pci;
 + };

We're not using the generic phy bindings? How is the linkage to the host
controller expressed?

[...]

 - clk = devm_clk_get(dev, usbhs);
 + if (np)
 + clk = of_clk_get_by_name(np, usbhs);
 + else
 + clk = clk_get(dev, usbhs);

Doesn't clk_get (and hence devm_clk_get) call of_clk_get_by_name?

Cheers,
Mark.
--
To unsubscribe from this list: send the line unsubscribe linux-usb in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v10 01/15] usb: doc: phy-mxs: Add more compatible strings

[Mark Rutland]

On Thu, Feb 20, 2014 at 05:14:30AM +, Peter Chen wrote:
 Add fsl,imx6q-usbphy for imx6dq and imx6dl, add
 fsl,imx6sl-usbphy for imx6sl.
 
 Signed-off-by: Peter Chen peter.c...@freescale.com
 ---
  Documentation/devicetree/bindings/usb/mxs-phy.txt |3 ++-
  1 files changed, 2 insertions(+), 1 deletions(-)
 
 diff --git a/Documentation/devicetree/bindings/usb/mxs-phy.txt 
 b/Documentation/devicetree/bindings/usb/mxs-phy.txt
 index 5835b27..b43d4c9e 100644
 --- a/Documentation/devicetree/bindings/usb/mxs-phy.txt
 +++ b/Documentation/devicetree/bindings/usb/mxs-phy.txt
 @@ -1,7 +1,8 @@
  * Freescale MXS USB Phy Device
  
  Required properties:
 -- compatible: Should be fsl,imx23-usbphy
 +- compatible: fsl,imx23-usbphy for imx23 and imx28, fsl,imx6q-usbphy
 +  for imx6dq and imx6dl, fsl,imx6sl-usbphy for imx6sl

Minor nit, but could we restructure this as something like the
following, with each string on a new line:

- compatible: should contain:
* fsl,imx23-usbphy for imx23 and imx28
* fsl,imx6q-usbphy for imx6dq and imx6dl
* fsl,imx6sl-usbphy for imx6sl

It makes it a bit easier to read.

I see the existing fsl,imx23-usbphy is used as a fallback for
fsl,imx28-usbphy, fsl,imx6q-usbphy, and fsl,imx6sl-usbphy in
existing DTs.

Is this expected going forward? It might be worth mentioning.

Otherwise this looks fine to me.

Thanks,
Mark.
--
To unsubscribe from this list: send the line unsubscribe linux-usb in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v10 04/15] usb: doc: phy-mxs: update binding for adding anatop phandle

[Mark Rutland]

On Thu, Feb 20, 2014 at 05:14:33AM +, Peter Chen wrote:
 Add anatop phandle which is used to access anatop registers to
 control PHY's power and other USB operations.
 
 Signed-off-by: Peter Chen peter.c...@freescale.com
 ---
  Documentation/devicetree/bindings/usb/mxs-phy.txt |2 ++
  1 files changed, 2 insertions(+), 0 deletions(-)
 
 diff --git a/Documentation/devicetree/bindings/usb/mxs-phy.txt 
 b/Documentation/devicetree/bindings/usb/mxs-phy.txt
 index b43d4c9e..fc6659d 100644
 --- a/Documentation/devicetree/bindings/usb/mxs-phy.txt
 +++ b/Documentation/devicetree/bindings/usb/mxs-phy.txt
 @@ -5,10 +5,12 @@ Required properties:
for imx6dq and imx6dl, fsl,imx6sl-usbphy for imx6sl
  - reg: Should contain registers location and length
  - interrupts: Should contain phy interrupt
 +- fsl,anatop: phandle for anatop register, it is only for imx6 SoC series

This is now required?

What happens to those existing DTBs that claim compatibility with
fsl,imx6q-usbphy but don't have an fsl,anatop property?

Thanks,
Mark.

  
  Example:
  usbphy1: usbphy@020c9000 {
   compatible = fsl,imx6q-usbphy, fsl,imx23-usbphy;
   reg = 0x020c9000 0x1000;
   interrupts = 0 44 0x04;
 + fsl,anatop = anatop;
  };
 -- 
 1.7.8
 
 
 
--
To unsubscribe from this list: send the line unsubscribe linux-usb in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v10 10/15] usb: phy-mxs: Add implementation of set_wakeup

[Mark Rutland]

On Thu, Feb 20, 2014 at 05:14:39AM +, Peter Chen wrote:
 When we need the PHY can be waken up by external signals,
 we can call this API. Besides, we call mxs_phy_disconnect_line
 at this API to close the connection between USB PHY and
 controller, after that, the line state from controller is SE0.
 Once the PHY is out of power, without calling mxs_phy_disconnect_line,
 there are unknown wakeups due to dp/dm floating at device mode.
 
 Signed-off-by: Peter Chen peter.c...@freescale.com
 ---
  drivers/usb/phy/phy-mxs-usb.c |  116 
 +
  1 files changed, 116 insertions(+), 0 deletions(-)

[...]

 +static void mxs_phy_disconnect_line(struct mxs_phy *mxs_phy, bool on)
 +{
 + bool vbus_is_on = false;
 +
 + /* If the SoCs don't need to disconnect line without vbus, quit */
 + if (!(mxs_phy-data-flags  MXS_PHY_DISCONNECT_LINE_WITHOUT_VBUS))
 + return;
 +
 + /* If the SoCs don't have anatop, quit */
 + if (!mxs_phy-regmap_anatop)
 + return;

So it looks like fsl,anatop is truly optional.

Thanks,
Mark.
--
To unsubscribe from this list: send the line unsubscribe linux-usb in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v10 01/15] usb: doc: phy-mxs: Add more compatible strings

[Mark Rutland]

On Fri, Feb 21, 2014 at 09:40:29AM +, Peter Chen wrote:
  
  
Required properties:
   -- compatible: Should be fsl,imx23-usbphy
   +- compatible: fsl,imx23-usbphy for imx23 and imx28, fsl,imx6q-
  usbphy
   +  for imx6dq and imx6dl, fsl,imx6sl-usbphy for imx6sl
  
  Minor nit, but could we restructure this as something like the following,
  with each string on a new line:
  
  - compatible: should contain:
  * fsl,imx23-usbphy for imx23 and imx28
  * fsl,imx6q-usbphy for imx6dq and imx6dl
  * fsl,imx6sl-usbphy for imx6sl
  
  It makes it a bit easier to read.
 
 Thanks, will change like above.
 
  
  I see the existing fsl,imx23-usbphy is used as a fallback for
  fsl,imx28-usbphy, fsl,imx6q-usbphy, and fsl,imx6sl-usbphy in
  existing DTs.
  
  Is this expected going forward? It might be worth mentioning.
  
 
 These SoCs used the same FSL imx PHY, but different versions.
 imx23/imx28 are the first version, more improvements are at
 later SoCs (like imx6x) version. Keep fsl,imx23-usbphy at
 imx6 dts will be user know it is from imx23's. If you think
 it does not need, I can delete fsl,imx23-usbphy from imx6 dts.

I'm not arguing to remove it, I'm suggesting it might be worth
mentioning that it's not mutually exclusive, and can be a fallback for
the other strings.

Cheers,
Mark.
--
To unsubscribe from this list: send the line unsubscribe linux-usb in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH RFC] usb: gadget: Add xilinx axi usb2 device support

[Mark Rutland]

On Fri, Feb 21, 2014 at 01:41:03PM +, Michal Simek wrote:
 Hi Mark,
 
 On 02/21/2014 01:04 PM, Mark Rutland wrote:
  
  On Thu, Feb 20, 2014 at 06:23:13PM +, Felipe Balbi wrote:
  Hi,
 
  On Wed, Feb 19, 2014 at 03:10:45PM +0530, Subbaraya Sundeep Bhatta wrote:
  This patch adds xilinx axi usb2 device driver support
 
  Signed-off-by: Subbaraya Sundeep Bhatta sbha...@xilinx.com
  ---
   .../devicetree/bindings/usb/xilinx_usb.txt |   21 +
   drivers/usb/gadget/Kconfig |   14 +
   drivers/usb/gadget/Makefile|1 +
   drivers/usb/gadget/xilinx_udc.c| 2045 
  
   4 files changed, 2081 insertions(+), 0 deletions(-)
   create mode 100644 Documentation/devicetree/bindings/usb/xilinx_usb.txt
   create mode 100644 drivers/usb/gadget/xilinx_udc.c
 
  diff --git a/Documentation/devicetree/bindings/usb/xilinx_usb.txt 
  b/Documentation/devicetree/bindings/usb/xilinx_usb.txt
  new file mode 100644
  index 000..acf03ab
  --- /dev/null
  +++ b/Documentation/devicetree/bindings/usb/xilinx_usb.txt
  @@ -0,0 +1,21 @@
  +Xilinx AXI USB2 device controller
  +
  +Required properties:
  +- compatible : Should be xlnx,axi-usb2-device-4.00.a
  
  Is axi-usb2-device the official device name?
 
 It is the name of IP which Xilinx have and we are using names in this format.
 
 
  +- reg: Physical base address and size of the Axi USB2
  +   device registers map.
  +- interrupts : Property with a value describing the interrupt
  +   number.
  
  Does the device only have a single interrupt?
 
 I believe so.
 
 
  +- interrupt-parent   : Must be core interrupt controller
  
  Is this strictly necessary?
 
 I am not sure what do you mean by that. If you mean that interrupt-parent
 can be written to the root of DTS file then we can setup system with more
 interrupt controllers that's why it is required.

At which point it's not a required property of the node...

 If we can point to standard interrupt description then please point me to
 exact description you would like to see here and we can change it.

Unfortunately I'm not aware of a generic interrupts document. I just
don't see the point in each document listing interrupt-parent as a
requiredp roeprty when it's not. That said this is a trivial detail and
not really a blocker.

Cheers,
Mark
--
To unsubscribe from this list: send the line unsubscribe linux-usb in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH RFC] usb: gadget: Add xilinx axi usb2 device support

[Mark Rutland]

On Fri, Feb 21, 2014 at 03:41:03PM +, Felipe Balbi wrote:
 Hi,
 
 On Fri, Feb 21, 2014 at 12:04:54PM +, Mark Rutland wrote:
+Example:
+   axi-usb2-device@42e0 {
+compatible = xlnx,axi-usb2-device-4.00.a;
+interrupt-parent = 0x1;
+interrupts = 0x0 0x39 0x1;
+reg = 0x42e0 0x1;
+xlnx,include-dma = 0x1;
+};
+
   
   you need to Cc devicet...@vger.kernel.org for this.
  
  Cheers Filipe; thanks for adding us to Cc :)
 
 sure, but it's Felipe ;-)

Whoops; sorry Felipe :)

 
+   /* Map the registers */
+   res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+   udc-base_address = devm_ioremap_nocache(pdev-dev, res-start,
+resource_size(res));
   
   use devm_ioremap_resource() instead.
  
  Also, res might be NULL. You should check that before dereferencing it.
 
 not needed when using devm_ioremap_resource(), see the implementation.

Ah, good to know.

Cheers,
Mark.
--
To unsubscribe from this list: send the line unsubscribe linux-usb in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] ARM-i.MX6Q-dts : Added USB_OTG Support

[Mark Rutland]

On Fri, Jan 24, 2014 at 09:28:44AM +, Ashutosh singh wrote:
 This patch adds support for USB_OTG on Phytec phyFLEX-i.MX6 Quad module.
 
 Signed-off-by: Ashutosh singh ashutos...@phytec.in
 ---
  arch/arm/boot/dts/imx6q-phytec-pbab01.dts  |4 
  arch/arm/boot/dts/imx6q-phytec-pfla02.dtsi |   22 ++
  2 files changed, 26 insertions(+)
 
 diff --git a/arch/arm/boot/dts/imx6q-phytec-pbab01.dts 
 b/arch/arm/boot/dts/imx6q-phytec-pbab01.dts
 index 7d37ec6..39e69bd 100644
 --- a/arch/arm/boot/dts/imx6q-phytec-pbab01.dts
 +++ b/arch/arm/boot/dts/imx6q-phytec-pbab01.dts
 @@ -32,3 +32,7 @@
  usdhc3 {
   status = okay;
  };
 +
 +usbotg {
 + status = okay;
 +};
 diff --git a/arch/arm/boot/dts/imx6q-phytec-pfla02.dtsi 
 b/arch/arm/boot/dts/imx6q-phytec-pfla02.dtsi
 index 1a3b50d..dcb1d59 100644
 --- a/arch/arm/boot/dts/imx6q-phytec-pfla02.dtsi
 +++ b/arch/arm/boot/dts/imx6q-phytec-pfla02.dtsi
 @@ -18,6 +18,19 @@
   memory {
   reg = 0x1000 0x8000;
   };
 +
 + regulators {
 + compatible = simple-bus;

This is _not_ a simple bus. It doesn't have the required ranges
property.

Why do these need to be in a regulators container node? We don't group
dma controllers under a dmas node, or uarts under a uarts node.

 +
 + reg_usb_otg_vbus: usb_otg_vbus {
 + compatible = regulator-fixed;
 + regulator-name = usb_otg_vbus;
 + regulator-min-microvolt = 500;
 + regulator-max-microvolt = 500;
 + gpio = gpio4 15 0;
 + enable-active-low;
 + };
 + };
  };

Thanks,
Mark.
--
To unsubscribe from this list: send the line unsubscribe linux-usb in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] ARM-i.MX6Q-dts : Added USB_OTG Support

[Mark Rutland]

On Fri, Jan 24, 2014 at 12:15:08PM +, Fabio Estevam wrote:
 Hi Mark,
 
 On Fri, Jan 24, 2014 at 9:50 AM, Mark Rutland mark.rutl...@arm.com wrote:
 
  +
  + regulators {
  + compatible = simple-bus;
 
  This is _not_ a simple bus. It doesn't have the required ranges
  property.
 
  Why do these need to be in a regulators container node? We don't group
  dma controllers under a dmas node, or uarts under a uarts node.
 
 It seems we have this same issue on several imx6 dts files.
 
 Would the below address your suggestion?

The below patch looks good to me.

In general there seems to be a misunderstanding of the purpose of
simple-bus, as an annotation that children of a node should be probed,
rather than as a representation of a simple bus (which is of the same
type as the parent-bus, which requires no programming, where children
can be used without knowledge of the simple-bus).

There seem to be a lot of cases where simple-bus is used as a fallback
compatible string, when in reality the node's children make no sense
without information from and/or programming of the node with the
simple-bus property. Those cases are completely wrong, and a complete
abuse of simple-bus.

There are other cases where simple-bus nodes are used to group nodes
from a commonly reused block. As long as these have the requisite
ranges, #address-cells, and #size-cells, then they are valid, and make
sense for translating / widening addresses for common groups of
peripherals even if they're not on a different physical bus.

I don't see the point in grouping together nodes in an artificial
container because of their functionality rather than their topology, it
seems to breed confusion. 

Cheers,
Mark.

 
 diff --git a/arch/arm/boot/dts/imx6qdl-sabresd.dtsi 
 b/arch/arm/boot/dts/imx6qdl-
 index e75e11b..ba35560 100644
 --- a/arch/arm/boot/dts/imx6qdl-sabresd.dtsi
 +++ b/arch/arm/boot/dts/imx6qdl-sabresd.dtsi
 @@ -15,33 +15,29 @@
 reg = 0x1000 0x4000;
 };
 
 -   regulators {
 -   compatible = simple-bus;
 -
 -   reg_usb_otg_vbus: usb_otg_vbus {
 -   compatible = regulator-fixed;
 -   regulator-name = usb_otg_vbus;
 -   regulator-min-microvolt = 500;
 -   regulator-max-microvolt = 500;
 -   gpio = gpio3 22 0;
 -   enable-active-high;
 -   };
 +   reg_usb_otg_vbus: regulator@0 {
 +   compatible = regulator-fixed;
 +   regulator-name = usb_otg_vbus;
 +   regulator-min-microvolt = 500;
 +   regulator-max-microvolt = 500;
 +   gpio = gpio3 22 0;
 +   enable-active-high;
 +   };
 
 -   reg_usb_h1_vbus: usb_h1_vbus {
 -   compatible = regulator-fixed;
 -   regulator-name = usb_h1_vbus;
 -   regulator-min-microvolt = 500;
 -   regulator-max-microvolt = 500;
 -   gpio = gpio1 29 0;
 -   enable-active-high;
 -   };
 +   reg_usb_h1_vbus: regulator@1 {
 +   compatible = regulator-fixed;
 +   regulator-name = usb_h1_vbus;
 +   regulator-min-microvolt = 500;
 +   regulator-max-microvolt = 500;
 +   gpio = gpio1 29 0;
 +   enable-active-high;
 +   };
 
 -   reg_audio: wm8962_supply {
 -   compatible = regulator-fixed;
 -   regulator-name = wm8962-supply;
 -   gpio = gpio4 10 0;
 -   enable-active-high;
 -   };
 +   reg_audio: regulator@2 {
 +   compatible = regulator-fixed;
 +   regulator-name = wm8962-supply;
 +   gpio = gpio4 10 0;
 +   enable-active-high;
 };
 
 gpio-keys {
 
 If so, I will prepare some patches to update other dts files.
 
 Thanks,
 
 Fabio Estevam
 
--
To unsubscribe from this list: send the line unsubscribe linux-usb in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v4 1/5] mfd: omap-usb-host: Update DT clock binding information

[Mark Rutland]

On Wed, Jan 08, 2014 at 06:15:38AM +, Roger Quadros wrote:
 The omap-usb-host driver expects the 60MHz functional clock
 of the USB host module to be named as init_60m_fclk.
 Add this information in the DT binding document.
 
 CC: Lee Jones lee.jo...@linaro.org
 CC: Samuel Ortiz sa...@linux.intel.com
 Signed-off-by: Roger Quadros rog...@ti.com
 ---
  Documentation/devicetree/bindings/mfd/omap-usb-host.txt | 4 
  1 file changed, 4 insertions(+)
 
 diff --git a/Documentation/devicetree/bindings/mfd/omap-usb-host.txt 
 b/Documentation/devicetree/bindings/mfd/omap-usb-host.txt
 index b381fa6..5635202 100644
 --- a/Documentation/devicetree/bindings/mfd/omap-usb-host.txt
 +++ b/Documentation/devicetree/bindings/mfd/omap-usb-host.txt
 @@ -32,6 +32,10 @@ Optional properties:
  - single-ulpi-bypass: Must be present if the controller contains a single
ULPI bypass control bit. e.g. OMAP3 silicon = ES2.1
  
 +- clocks: phandle to 60MHz functional clock to the USB Host module.
 +
 +- clock-names: must be init_60m_fclk
 +

Nit: clocks aren't just phandles, there's a clock-specifier too.

Also, it would be nicer if clocks was defined in terms of clock-names,
as it makes the relationship between clocks and clock-names clear, and
makes it easier to extend the list in future. Something like:

- clocks: a list of phandles + clock-specifiers, one for each entry in
  clock-names

- clock-names: should include:
  * init_60m_fclk - the 60MHz functional clock to the USB host module.

Thanks,
Mark.
--
To unsubscribe from this list: send the line unsubscribe linux-usb in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v6] usb: gadget: Add UDC driver for Aeroflex Gaisler GRUSBDC

[Mark Rutland]

On Wed, Jan 08, 2014 at 01:23:04PM +, Andreas Larsson wrote:
 On 2014-01-06 17:22, Mark Rutland wrote:
  Hi,
 
  Apologies for the late reply, I wasn't able to access my mail much over
  the Christmas break.
 
 The patch is already applied to both the next branch of Felipe Balbi's 
 usb/next branch and merged from there into Greg Kroah-Hartman's 
 usb/usb-next branch, so it might be too late for including in the 
 initial patch.
 
 I'll be happy to send a patch series to improve things as indicated 
 inline below. There are no functional bugs running on SPARC which is the 
 ordinary environment for this core, so adding patches to do these 
 improvements should work fine.
 
 
  On Mon, Dec 23, 2013 at 08:25:49PM +, Andreas Larsson wrote:
  This adds an UDC driver for GRUSBDC USB Device Controller cores available 
  in the
  GRLIB VHDL IP core library. The driver only supports DMA mode.
 
  Signed-off-by: Andreas Larsson andr...@gaisler.com
  ---
 
  Differences since v1:
  - Use hexprint for debug printoutes instead of homemade equivalent
  - Use the dev_* printk variants over the board
  - Get rid of unnecessary casts and includes
  - Use USB_STATE_NOTATTACHED instead of USB_STATE_ATTACHED for clarity
  - Get rid of commented out VERBOSE_DEBUG definition
  - Do not devm-allocate any requests
  - Get rid of unnecessary resqueduling of the workqueue handler
  - Make sure that gadget setup function is called with interrupts disabled
  Differences since v2:
  - Fixed an error printout
  - Got rid of the work queue in favor of threaded interrupts
  Differences since v3:
  - Disable interrupts when locking spinlocks instead of using
 local_irq_save deeper within critical section
  Differences since v4:
  - Set quirk_ep_out_aligned_size
  - Use usb_ep_set_maxpacket_limit
  - Add devicetree documentation
  Differences since v5:
  - Declare unexpected spin_unlock and spin_lock for sparse
  - Fix a bad comment
  - Use ACCESS_ONCE instead of gr_read32 when checking for update of dma 
  descriptor
 
Documentation/devicetree/bindings/usb/gr-udc.txt |   28 +
drivers/usb/gadget/Kconfig   |7 +
drivers/usb/gadget/Makefile  |1 +
drivers/usb/gadget/gr_udc.c  | 2242 
  ++
drivers/usb/gadget/gr_udc.h  |  220 +++
5 files changed, 2498 insertions(+)
create mode 100644 Documentation/devicetree/bindings/usb/gr-udc.txt
create mode 100644 drivers/usb/gadget/gr_udc.c
create mode 100644 drivers/usb/gadget/gr_udc.h
 
  diff --git a/Documentation/devicetree/bindings/usb/gr-udc.txt 
  b/Documentation/devicetree/bindings/usb/gr-udc.txt
  new file mode 100644
  index 000..0c5118f
  --- /dev/null
  +++ b/Documentation/devicetree/bindings/usb/gr-udc.txt
  @@ -0,0 +1,28 @@
  +USB Peripheral Controller driver for Aeroflex Gaisler GRUSBDC.
  +
  +The GRUSBDC USB Device Controller core is available in the GRLIB VHDL
  +IP core library.
  +
  +Note: In the ordinary environment for the core, a Leon SPARC system,
  +these properties are built from information in the AMBA plugplay.
  +
  +Required properties:
  +
  +- name : Should be GAISLER_USBDC or 01_021
 
  What's this for?
 
  Why is this not matched using a compatible string?
 
  What does 01_021 mean?
 
 Regarding using name, this is standard for SPARC. The names in the
 device tree originates from the PROM.
 
 The name field is the actually the first field checked for a match in
 of_match_node, followed by type then compatible. See
 http://lxr.free-electrons.com/source/drivers/of/base.c?v=3.12#L723
 
 Examples can be found among others in:
 - drivers/watchdog/riowd.c
 - drivers/watchdog/cpwd.c
 - drivers/mtd/maps/sun_uflash.c
 - drivers/input/misc/sparcspkr.c
 - drivers/input/serio/i8042-sparcio.h
 - drivers/hwmon/ultra45_env.c
 
 As for the 01_021, the LEON SPARC systems uses a plugplay to identify
 different IP cores in the system. When the PROM is unaware of the name
 of a certain core, the name field presented from the PROM will be on
 this form. This is standard handling for LEON SPARC drivers.
 Examples of this can be found in:
 - drivers/gpio/gpio-grgpio.c
 - drivers/usb/host/uhci-grlib.c
 - drivers/usb/host/ehci-grlib.c
 - drivers/video/grvga.c
 - drivers/net/can/grcan.c
 - drivers/net/ethernet/aeroflex/greth.c
 - drivers/tty/serial/apbuart.c
 - drivers/gpio/gpio-grgpio.c

OK. Sorry for the confusion there.

 
 
  +
  +- reg : Address and length of the register set for the device
  +
  +- interrupts : Interrupt numbers for this device
 
  How many, and what do they correspond to?
 
 I'll add text on that
 
 
  +
  +Optional properties:
  +
  +- epobufsizes : An array of buffer sizes for OUT endpoints. If the 
  property is
  +   not present, or for endpoints outside of the array, 1024 is 
  assumed by
  +   the driver.
  +
  +- epibufsizes : An array of buffer sizes for IN endpoints. If the 
  property

Re: [PATCH 1/2] ohci-platform: Add support for devicetree instantiation

[Mark Rutland]

On Sun, Jan 05, 2014 at 11:04:39PM +, Hans de Goede wrote:
 Add support for ohci-platform instantiation from devicetree, including
 optionally getting clks and a phy from devicetree, and enabling / disabling
 those on power_on / off.
 
 This should allow using ohci-platform from devicetree in various cases.
 Specifically after this commit it can be used for the ohci controller found
 on Allwinner sunxi SoCs.
 
 Signed-off-by: Hans de Goede hdego...@redhat.com
 ---
  .../devicetree/bindings/usb/platform-ohci.txt  |  25 
  drivers/usb/host/ohci-platform.c   | 146 
 ++---
  2 files changed, 151 insertions(+), 20 deletions(-)
  create mode 100644 Documentation/devicetree/bindings/usb/platform-ohci.txt
 
 diff --git a/Documentation/devicetree/bindings/usb/platform-ohci.txt 
 b/Documentation/devicetree/bindings/usb/platform-ohci.txt
 new file mode 100644
 index 000..6846f1c
 --- /dev/null
 +++ b/Documentation/devicetree/bindings/usb/platform-ohci.txt
 @@ -0,0 +1,25 @@
 +Generic Platform OHCI controller
 +
 +Required properties:
 + - compatible: Should be platform-ohci

To me, platform-ohci seems rather Linux-specific. Platform devices are
a Linux abstraction that don't correspond to any particular bus type in
reality.

We have some *-generic bindings. A name of that form might be more
appropriate. Or we could choose an arbitrary OHCI implementation's
vendor,ochi string and everything else can declare compatibility with
that.

 + - reg: Address range of the ohci registers.
 + - interrupts: Should contain the ohci interrupt.
 +
 +Optional properties:
 + - clocks: array of clocks
 + - clock-names: clock names ahb and/or ohci

A description of what the clocks are might be helpful.

How about something like:

- clocks: a list of phandle + clock specifier pairs, one for each entry
  in clock-names.

- clock-names: Should contain:
   * ahb - some description here.
   * ohci - some description here.

Thanks,
Mark.
--
To unsubscribe from this list: send the line unsubscribe linux-usb in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v6] usb: gadget: Add UDC driver for Aeroflex Gaisler GRUSBDC

[Mark Rutland]

Hi,

Apologies for the late reply, I wasn't able to access my mail much over
the Christmas break.

On Mon, Dec 23, 2013 at 08:25:49PM +, Andreas Larsson wrote:
 This adds an UDC driver for GRUSBDC USB Device Controller cores available in 
 the
 GRLIB VHDL IP core library. The driver only supports DMA mode.

 Signed-off-by: Andreas Larsson andr...@gaisler.com
 ---

 Differences since v1:
 - Use hexprint for debug printoutes instead of homemade equivalent
 - Use the dev_* printk variants over the board
 - Get rid of unnecessary casts and includes
 - Use USB_STATE_NOTATTACHED instead of USB_STATE_ATTACHED for clarity
 - Get rid of commented out VERBOSE_DEBUG definition
 - Do not devm-allocate any requests
 - Get rid of unnecessary resqueduling of the workqueue handler
 - Make sure that gadget setup function is called with interrupts disabled
 Differences since v2:
 - Fixed an error printout
 - Got rid of the work queue in favor of threaded interrupts
 Differences since v3:
 - Disable interrupts when locking spinlocks instead of using
   local_irq_save deeper within critical section
 Differences since v4:
 - Set quirk_ep_out_aligned_size
 - Use usb_ep_set_maxpacket_limit
 - Add devicetree documentation
 Differences since v5:
 - Declare unexpected spin_unlock and spin_lock for sparse
 - Fix a bad comment
 - Use ACCESS_ONCE instead of gr_read32 when checking for update of dma 
 descriptor

  Documentation/devicetree/bindings/usb/gr-udc.txt |   28 +
  drivers/usb/gadget/Kconfig   |7 +
  drivers/usb/gadget/Makefile  |1 +
  drivers/usb/gadget/gr_udc.c  | 2242 
 ++
  drivers/usb/gadget/gr_udc.h  |  220 +++
  5 files changed, 2498 insertions(+)
  create mode 100644 Documentation/devicetree/bindings/usb/gr-udc.txt
  create mode 100644 drivers/usb/gadget/gr_udc.c
  create mode 100644 drivers/usb/gadget/gr_udc.h

 diff --git a/Documentation/devicetree/bindings/usb/gr-udc.txt 
 b/Documentation/devicetree/bindings/usb/gr-udc.txt
 new file mode 100644
 index 000..0c5118f
 --- /dev/null
 +++ b/Documentation/devicetree/bindings/usb/gr-udc.txt
 @@ -0,0 +1,28 @@
 +USB Peripheral Controller driver for Aeroflex Gaisler GRUSBDC.
 +
 +The GRUSBDC USB Device Controller core is available in the GRLIB VHDL
 +IP core library.
 +
 +Note: In the ordinary environment for the core, a Leon SPARC system,
 +these properties are built from information in the AMBA plugplay.
 +
 +Required properties:
 +
 +- name : Should be GAISLER_USBDC or 01_021

What's this for?

Why is this not matched using a compatible string?

What does 01_021 mean?

 +
 +- reg : Address and length of the register set for the device
 +
 +- interrupts : Interrupt numbers for this device

How many, and what do they correspond to?

 +
 +Optional properties:
 +
 +- epobufsizes : An array of buffer sizes for OUT endpoints. If the property 
 is
 +   not present, or for endpoints outside of the array, 1024 is assumed by
 +   the driver.
 +
 +- epibufsizes : An array of buffer sizes for IN endpoints. If the property is
 +   not present, or for endpoints outside of the array, 1024 is assumed by
 +   the driver.

These names are rather opaque. Something like {input,output}-buf-lens
would be far clearer.

How many entries are expected?

A specific driver should have no relevance to the binding. Just say if
not 1024.

[...]

 +/* Must be called with dev-lock held */
 +static int gr_udc_init(struct gr_udc *dev)
 +{
 +   struct device_node *np = dev-dev-of_node;
 +   u32 epctrl_val;
 +   u32 dmactrl_val;
 +   int i;
 +   int ret = 0;
 +   u32 *bufsizes;
 +   u32 bufsize;
 +   int len;
 +
 +   gr_set_address(dev, 0);
 +
 +   INIT_LIST_HEAD(dev-gadget.ep_list);
 +   dev-gadget.speed = USB_SPEED_UNKNOWN;
 +   dev-gadget.ep0 = dev-epi[0].ep;
 +
 +   INIT_LIST_HEAD(dev-ep_list);
 +   gr_set_ep0state(dev, GR_EP0_DISCONNECT);
 +
 +   bufsizes = (u32 *)of_get_property(np, epobufsizes, len);

of_get_property gives you the raw property value bye string, which is
_not_ a u32 pointer -- it's not necessarily the same endianness as the
kernel. Please use an appropriate accessor.

 +   len /= sizeof(u32);
 +   for (i = 0; i  dev-nepo; i++) {
 +   bufsize = (bufsizes  i  len) ? bufsizes[i] : 1024;

You can use of_property_read_u32_index within the loop for this:

if (of_property_read_u32_index(np, epobufsizes, bufsize)
bufsize = 1024;

 +   ret = gr_ep_init(dev, i, 0, bufsize);
 +   if (ret)
 +   return ret;
 +   }
 +
 +   bufsizes = (u32 *)of_get_property(np, epibufsizes, len);
 +   len /= sizeof(u32);
 +   for (i = 0; i  dev-nepi; i++) {
 +   bufsize = (bufsizes  i  len) ? bufsizes[i] : 1024;

Likewise here.

[...]

 +static int gr_probe(struct platform_device *ofdev)
 +{
 +  

Re: [PATCH v4 12/15] usb: phy: msm: Add support for secondary PHY control

[Mark Rutland]

On Mon, Nov 18, 2013 at 12:57:42PM +, Ivan T. Ivanov wrote:
 
 Hi Mark,
 
 On Fri, 2013-11-15 at 16:42 +, Mark Rutland wrote: 
  On Tue, Nov 12, 2013 at 02:51:47PM +, Ivan T. Ivanov wrote:
   From: Ivan T. Ivanov iiva...@mm-sol.com
   
   Allow support to use 2nd HSPHY with USB2 Core.
   Some platforms may have configuration to allow USB controller
   work with any of the two HSPHYs present. By default driver
   configures USB core to use primary HSPHY. Add support to allow
   user select 2nd HSPHY using DT parameter.
   
   Signed-off-by: Ivan T. Ivanov iiva...@mm-sol.com
   Cc: Manu Gautam mgau...@codeaurora.org
   Cc: devicet...@vger.kernel.org
   ---
.../devicetree/bindings/usb/msm-hsusb.txt  |6 +
drivers/usb/phy/phy-msm-usb.c  |   24 
   ++--
include/linux/usb/msm_hsusb.h  |1 +
include/linux/usb/msm_hsusb_hw.h   |1 +
4 files changed, 30 insertions(+), 2 deletions(-)
   
   diff --git a/Documentation/devicetree/bindings/usb/msm-hsusb.txt 
   b/Documentation/devicetree/bindings/usb/msm-hsusb.txt
   index 3f21204..d105ba9 100644
   --- a/Documentation/devicetree/bindings/usb/msm-hsusb.txt
   +++ b/Documentation/devicetree/bindings/usb/msm-hsusb.txt
   @@ -72,6 +72,12 @@ Optional properties:
- qcom,phy-init-sequence: PHY configuration sequence. val, reg pairs
 terminate with -1

   +- qcom,phy-num:  Select number of pyco-phy to use, can be one of
   + 0 - PHY one, default
   + 1 - Second PHY
   + Some platforms may have configuration to allow 
   USB
   + controller work with any of the two HSPHYs 
   present.
   +
  
  Only one can be used at a time?
 
 Yes only one of them. Selected at driver init time. 

Ok. For a given platform, is it likely that both are wired up and
_possibly_ usable?

Thanks
Mark.
--
To unsubscribe from this list: send the line unsubscribe linux-usb in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/2] usb: chipidea: fix mistake in device tree binding of nspire-usb to use vendor name 'lsi' instead of SoC name 'zevio'

[Mark Rutland]

On Wed, Dec 04, 2013 at 09:20:07AM +, dt.ta...@gmail.com wrote:
 From: Daniel Tang dt.ta...@gmail.com
 
 The SoC name was mistakenly used instead of the vendor name in the
 device tree binding for nspire-usb.
 
 This patch fixes this before the driver becomes widely adopted.

How widely adopted is it so far?

I can't see the binding in mainline yet.

 
 Signed-off-by: Daniel Tang dt.ta...@gmail.com
 ---
  Documentation/devicetree/bindings/usb/ci-hdrc-nspire.txt |4 ++--
  drivers/usb/chipidea/ci_hdrc_nspire.c|2 +-
  2 files changed, 3 insertions(+), 3 deletions(-)
 
 diff --git a/Documentation/devicetree/bindings/usb/ci-hdrc-nspire.txt 
 b/Documentation/devicetree/bindings/usb/ci-hdrc-nspire.txt
 index 5ba8e90..ef1fcbf 100644
 --- a/Documentation/devicetree/bindings/usb/ci-hdrc-nspire.txt
 +++ b/Documentation/devicetree/bindings/usb/ci-hdrc-nspire.txt
 @@ -1,7 +1,7 @@
  * TI-Nspire USB OTG Controller
  
  Required properties:
 -- compatible: Should be zevio,nspire-usb
 +- compatible: Should be lsi,nspire-usb

Surely this should be lsi,zevio-usb, matching the lsi,zevio-timer
binding?

Thanks,
Mark.
--
To unsubscribe from this list: send the line unsubscribe linux-usb in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/2] chipidea: ci_hdrc_imx: Allow handling the clock for an USB phy/hub

[Mark Rutland]

On Thu, Nov 14, 2013 at 02:09:46AM +, Fabio Estevam wrote:
 From: Fabio Estevam fabio.este...@freescale.com
 
 When using external USB PHY or USB hub, it is common that they require a clock
 input.
 
 Add a 'clk_phy' clock, so that it can be retrieved from the device tree and 
 enabled in the driver, so that the clock can properly drive the external 
 USB phy/hub.

As this clock feeds the PHY, it should be described on the PHY node.
It's a property of the PHY itself.

 
 Tested on a imx6q-udoo board, that connects via USBH1 to a USB2514 hub.
 
 In this board the USB2514 is clocked from a 24MHz clock that comes from the 
 imx6q CLKO2 pin.
 
 Signed-off-by: Fabio Estevam fabio.este...@freescale.com
 ---
  .../devicetree/bindings/usb/ci13xxx-imx.txt  |  2 ++
  drivers/usb/chipidea/ci_hdrc_imx.c   | 20 
 +++-
  2 files changed, 21 insertions(+), 1 deletion(-)
 
 diff --git a/Documentation/devicetree/bindings/usb/ci13xxx-imx.txt 
 b/Documentation/devicetree/bindings/usb/ci13xxx-imx.txt
 index b4b5b79..07ba38c 100644
 --- a/Documentation/devicetree/bindings/usb/ci13xxx-imx.txt
 +++ b/Documentation/devicetree/bindings/usb/ci13xxx-imx.txt
 @@ -18,6 +18,8 @@ Optional properties:
  - vbus-supply: regulator for vbus
  - disable-over-current: disable over current detect
  - external-vbus-divider: enables off-chip resistor divider for Vbus
 +- clocks: phandle to the clock that drives the USB hub

If you're using clock-names, define clocks in terms of it:

- clocks: a list of phandle + clock-specifier pairs corresponding to
  entries in clock-names.

 +- clock-names: must be phy

s/must be/should contain/

Thanks,
Mark.
--
To unsubscribe from this list: send the line unsubscribe linux-usb in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 2/2] ARM: dts: imx6q-udoo: Add USB host support

[Mark Rutland]

On Thu, Nov 14, 2013 at 02:09:47AM +, Fabio Estevam wrote:
 From: Fabio Estevam fabio.este...@freescale.com
 
 Udoo board has USBH1 port connected to a USB2514 hub.
 
 Add support for it.
 
 Signed-off-by: Fabio Estevam fabio.este...@freescale.com
 ---
  arch/arm/boot/dts/imx6q-udoo.dts | 34 ++
  1 file changed, 34 insertions(+)
 
 diff --git a/arch/arm/boot/dts/imx6q-udoo.dts 
 b/arch/arm/boot/dts/imx6q-udoo.dts
 index 1c7f7a1..109b997 100644
 --- a/arch/arm/boot/dts/imx6q-udoo.dts
 +++ b/arch/arm/boot/dts/imx6q-udoo.dts
 @@ -19,6 +19,23 @@
   memory {
   reg = 0x1000 0x4000;
   };
 +
 + regulators {
 + compatible = simple-bus;
 + #address-cells = 1;
 + #size-cells = 0;
 +

This should have a ranges property, or it's not a simple-bus.

Why do you even need this anyway? The regulators can live under the root
quite happily.

Thanks,
Mark.
--
To unsubscribe from this list: send the line unsubscribe linux-usb in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 3/3] usb: phy-generic: Add ULPI VBUS support

[Mark Rutland]

On Mon, Dec 02, 2013 at 07:05:19AM +, Chris Ruehl wrote:
 usb: phy-generic: Add ULPI VBUS support
 
 Some platforms need to set the VBUS parameters of the ULPI
 like ISP1504 which interact with overcurrent protection and
 power switch MIC2575. Therefore it requires to set
 * DRVVBUS
 * DRVVBUS_EXT
 * EXTVBUSIND
 * CHRGVBUS
 of the ULPI.
 This patch add support for it.

Could you elaborate on when we need to do this and why?

 
 Devicetree configuration example:
 
 usbphy0: usbphy@0x10024170 {
  compatible = usb-nop-xceiv;
  reg = 0x10024170 0x4; /* ULPI Viewport OTG */
  clocks = clks 75;
  clock-names = main_clk;
 };
 
 usbphy0 {
 reset-gpios = gpio1 31 1;
 pinctrl-names = default;
 pinctrl-0 = pinctrl_usbphy0 pinctrl_usbotg1;
 ulpi_set_vbus = 0x0f;
 };
 
 Please refer to the phy-bindings.txt for the value of ulpi_set_vbus commit
 with this patch.
 
 Signed-off-by: Chris Ruehl chris.ru...@gtsys.com.hk
 ---
  .../devicetree/bindings/phy/phy-bindings.txt   |   15 ++
  drivers/usb/phy/phy-generic.c  |   50 
 +++-
  drivers/usb/phy/phy-generic.h  |3 ++
  3 files changed, 67 insertions(+), 1 deletion(-)
 
 diff --git a/Documentation/devicetree/bindings/phy/phy-bindings.txt 
 b/Documentation/devicetree/bindings/phy/phy-bindings.txt
 index 8ae844f..b109b2f 100644
 --- a/Documentation/devicetree/bindings/phy/phy-bindings.txt
 +++ b/Documentation/devicetree/bindings/phy/phy-bindings.txt
 @@ -34,6 +34,18 @@ phys : the phandle for the PHY device (used by the PHY 
 subsystem)
  phy-names : the names of the PHY corresponding to the PHYs present in the
   *phys* phandle
  
 +Optional Properties:
 +reset-gpios :GPIO used to reset ULPI like ISP1504 with
 + 0 = reset active high ; 1 = reset active low.

The format of the gpio-specifier doesn't matter here.

Why do you need to mention the ISP1504? Either this is generic, or it
doesn't belong here.

Perhaps we need a ulpi-phy binding document. This and the rest of the
patch is strongly tied to ULPI.

 +cs-gpios :   GPIO used to activate a ULPI like ISP1504 with
 + 0 = reset acitive high; 1 = reset active low.

Again, the format of the gpio-specifier is not a concern here. I'm also
a little confused as to the name cs-gpios for something that activates
the ULPI.

 +ulpi_set_vbus :  ULPI configuation parameter to program the VBUS 
 signaling of 
 + ISP1504 or similar chipsets.

Could you elaborate on this? Is this applicable to any ULPI PHY? The
description implies that it's somewhat tied to ISP1504 (if it's not,
drop the mention of ISP1504).

Why exactly do we need this, and why should it live in the DT?

Why can se not figure this out automatically, or have defaults that work
in more places?

Also, s/_/-/ on property names please.

 + Set the parameter:
 + DRVVBUS = (1) DRVVBUS_EXT = (11) EXTVBUSIND = (12) CHRGVBUS 
 = (13)
 + eg: DRVVBUS | DRVVBUS_EXT = 0x03
 + ulpi_set_vbus = 0x03

I don't like putting arbitrary bitfields like this in DT. They're
illegible and easy to abuse.

Boolean properties are nicer for flags.

What exactly do these flags mean?

[...]

 @@ -253,14 +284,15 @@ static int usb_phy_gen_xceiv_probe(struct 
 platform_device *pdev)
  
   if (dev-of_node) {
   struct device_node *node = dev-of_node;
 + struct resource *res;
   enum of_gpio_flags flags;
   enum of_gpio_flags csflags;
 + u32 ulpi_vbus;
  
   if (of_property_read_u32(node, clock-frequency, clk_rate))
   clk_rate = 0;
  
   needs_vcc = of_property_read_bool(node, vcc-supply);
 -
   nop-gpio_reset = of_get_named_gpio_flags(node, reset-gpios,
   0, flags);

Why the random line deletion?

  
 @@ -274,6 +306,22 @@ static int usb_phy_gen_xceiv_probe(struct 
 platform_device *pdev)
   if (gpio_is_valid(nop-gpio_chipselect))
   nop-cs_active_low = csflags  OF_GPIO_ACTIVE_LOW;
  
 + err = of_property_read_u32(node, ulpi_set_vbus,ulpi_vbus);
 + if (err) {
 + nop-ulpi_vbus = -1;
 + nop-viewport = NULL;
 + ulpi_vbus = 0;
 + } else {
 + dev_dbg(dev,ULPI ulpi_set_vbus 0x%02x,ulpi_vbus);
 + nop-ulpi_vbus = ulpi_vbus;
 + res = platform_get_resource(pdev, IORESOURCE_MEM, 0);

This wasn't described in the binding document, and it's ULPI specific. I
think we need a separate ulpi-phy binding that builds upon the generic
one.

Thanks,
Mark.
--
To unsubscribe from this list: send the line unsubscribe linux-usb in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v4 10/15] usb: phy: msm: Add device tree support and binding information

[Mark Rutland]

On Tue, Nov 12, 2013 at 02:51:45PM +, Ivan T. Ivanov wrote:
 From: Ivan T. Ivanov iiva...@mm-sol.com
 
 Allows MSM OTG controller to be specified via device tree.
 
 Signed-off-by: Ivan T. Ivanov iiva...@mm-sol.com
 Cc: devicet...@vger.kernel.org
 ---
  .../devicetree/bindings/usb/msm-hsusb.txt  |   57 +-
  drivers/usb/phy/phy-msm-usb.c  |   79 
 +---
  2 files changed, 124 insertions(+), 12 deletions(-)
 
 diff --git a/Documentation/devicetree/bindings/usb/msm-hsusb.txt 
 b/Documentation/devicetree/bindings/usb/msm-hsusb.txt
 index 0a85eba..f1045e3 100644
 --- a/Documentation/devicetree/bindings/usb/msm-hsusb.txt
 +++ b/Documentation/devicetree/bindings/usb/msm-hsusb.txt
 @@ -30,4 +30,59 @@ Required properties:
   dr_mode = peripheral;
   interrupts = 0 134 0;
   usb-phy = usb_otg;
 - };
 \ No newline at end of file
 + };
 +
 +USB PHY with optional OTG:
 +
 +Required properties:
 +- compatible:should contain qcom,usb-otg-ci for chipsets with
 + Chipidea 45nm PHY or qcom,usb-otg-snps for 
 chipsets
 + with Synopsys 28nm PHY

To make this easier to read and extend in future, I'd reorganise this
like so:

compatible: should contain:
 * qcom,usb-otg-ci for chipsets with Chipidea 45nm PHY
 * qcom,usb-otg-snps for chipsets with Synopsys 28nm PHY

 +- regs:  offset and length of the register set in the 
 memory map
 +- interrupts:interrupt-specifier for the OTG interrupt.
 +
 +- clocks:A list of phandle + clock-specifier pairs for the
 + clocks listed in clock-names
 +- clock-names:   Should contain the following:
 +  phy  USB PHY reference clock
 +  core Protocol engine clock
 +  ifaceInterface bus clock
 +  alt_core Optional: Protocol engine clock for targets with asynchronous
 + reset methodology.

I'd rearrange that last entry:

alt_core: Protocol engine clock for targets with asynchronous
reset methodology. (optional)

 +
 +- dr_mode:   One of host, peripheral or otg. Defaults to otg

If this has a default and thus isn't required, it should be listed as
optional.

 +
 +- vdccx-supply:  phandle to the regulator for the vdd supply for
 + digital circuit operation.
 +- v1p8-supply:   phandle to the regulator for the 1.8V supply
 +- v3p3-supply:   phandle to the regulator for the 3.3V supply
 +
 +- qcom,otg-control: OTG control (VBUS and ID notifications) can be one of
 + 1 - PHY control
 + 2 - PMIC control
 + 3 - User control (via debugfs)

NAK. This does not seem like a description of the hardware, and given
the debugfs comment is fundamentally tied to the way Linux functions
today.

 +
 +Optional properties:
 +- qcom,phy-init-sequence: PHY configuration sequence. val, reg pairs
 + terminate with -1

What is this? I'm really not keen on having arbitrary register/memory
poking sequences in the dt.

Why does it need to be terminated? Surely the size of the property tells
you that it's terminated.

 +
 +Example HSUSB OTG controller device node:
 +
 + usb@f9a55000 {
 + compatible = qcom,usb-otg-snps;
 + reg = 0xf9a55000 0x400;
 + interrupts = 0 134 0;;

s/;;/;/

 + dr_mode = peripheral;
 +
 + clocks = gcc GCC_XO_CLK, gcc GCC_USB_HS_SYSTEM_CLK,
 + gcc GCC_USB_HS_AHB_CLK;
 +
 + clock-names = phy, core, iface;
 +
 + vddcx-supply = pm8841_s2_corner;
 + v1p8-supply = pm8941_l6;
 + v3p3-supply = pm8941_l24;
 +
 + qcom,otg-control = 1;
 + qcom,phy-init-sequence = 0x01 0x90 0x;

I believe modern dtc versions can handle negative numbers directly.

[...]

 +static int msm_otg_read_dt(struct platform_device *pdev, struct msm_otg 
 *motg)
 +{
 + struct msm_otg_platform_data *pdata;
 + const struct of_device_id *id;
 + struct device_node *node = pdev-dev.of_node;
 + int len = 0;
 + u32 val;
 +
 + pdata = devm_kzalloc(pdev-dev, sizeof(*pdata), GFP_KERNEL);
 + if (!pdata)
 + return -ENOMEM;
 +
 + motg-pdata = pdata;
 +
 + id = of_match_device(msm_otg_dt_match, pdev-dev);
 + pdata-phy_type = (int) id-data;
 +
 + pdata-mode = of_usb_get_dr_mode(node);
 + if (pdata-mode == USB_DR_MODE_UNKNOWN)
 + pdata-mode = USB_DR_MODE_OTG;
 +
 + pdata-otg_control = OTG_PHY_CONTROL;
 + if (!of_property_read_u32(node, qcom,otg-control, val))
 + pdata-otg_control = val;

Is this validated elsewhere?

Thanks,
Mark.
--
To unsubscribe from this list: send the line unsubscribe linux-usb in
the body of a message to 

Re: [PATCH v4 12/15] usb: phy: msm: Add support for secondary PHY control

[Mark Rutland]

On Tue, Nov 12, 2013 at 02:51:47PM +, Ivan T. Ivanov wrote:
 From: Ivan T. Ivanov iiva...@mm-sol.com
 
 Allow support to use 2nd HSPHY with USB2 Core.
 Some platforms may have configuration to allow USB controller
 work with any of the two HSPHYs present. By default driver
 configures USB core to use primary HSPHY. Add support to allow
 user select 2nd HSPHY using DT parameter.
 
 Signed-off-by: Ivan T. Ivanov iiva...@mm-sol.com
 Cc: Manu Gautam mgau...@codeaurora.org
 Cc: devicet...@vger.kernel.org
 ---
  .../devicetree/bindings/usb/msm-hsusb.txt  |6 +
  drivers/usb/phy/phy-msm-usb.c  |   24 
 ++--
  include/linux/usb/msm_hsusb.h  |1 +
  include/linux/usb/msm_hsusb_hw.h   |1 +
  4 files changed, 30 insertions(+), 2 deletions(-)
 
 diff --git a/Documentation/devicetree/bindings/usb/msm-hsusb.txt 
 b/Documentation/devicetree/bindings/usb/msm-hsusb.txt
 index 3f21204..d105ba9 100644
 --- a/Documentation/devicetree/bindings/usb/msm-hsusb.txt
 +++ b/Documentation/devicetree/bindings/usb/msm-hsusb.txt
 @@ -72,6 +72,12 @@ Optional properties:
  - qcom,phy-init-sequence: PHY configuration sequence. val, reg pairs
   terminate with -1
  
 +- qcom,phy-num:  Select number of pyco-phy to use, can be one of
 + 0 - PHY one, default
 + 1 - Second PHY
 + Some platforms may have configuration to allow 
 USB
 + controller work with any of the two HSPHYs 
 present.
 +

Only one can be used at a time?

[...]

 @@ -1395,6 +1412,9 @@ static int msm_otg_read_dt(struct platform_device 
 *pdev, struct msm_otg *motg)
   if (!of_property_read_u32(node, qcom,otg-control, val))
   pdata-otg_control = val;
  
 + if (!of_property_read_u32(node, qcom,phy-num, val))
 + motg-phy_number = val;

No sanity checking? If not all of your tests on the value of
motg-phy_number are the same form, you may get undepected results...

Mark.
--
To unsubscribe from this list: send the line unsubscribe linux-usb in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v6 2/3] usb: phy: Add Qualcomm SS-USB and HS-USB drivers for DW PHY's

[Mark Rutland]

[...]

   +   phy-sleep_a_clk = devm_clk_get(phy-dev, sleep_a);
 
  What means the _a in clock name?
 
 They are 2 PHY's on 8074 chip. This drivers is supposed to
 operate on PHY 0, thus sleep_a. PHY 1 is using sleep_b. Take a look
 here http://www.spinics.net/lists/arm-kernel/msg276945.html

When you say two PHYs, do you mean the HS PHY and the SS PHY?

Or are there two HS PHYs? If so, would the other HS PHY have a sleep_b clock?

The clock-names property should describe the clocks from the view of the device
itself. As we're describing the PHY in isolation, rather than a big block that
contains the PHY, the presesnce or absence of other PHYs should not affect the
name. If the _a suffix is only to differentiate the instance of PHY, it
should be dropped.

Thanks,
Mark.
--
To unsubscribe from this list: send the line unsubscribe linux-usb in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v4 1/3] usb: dwc3: msm: Add device tree binding information

[Mark Rutland]

On Mon, Sep 23, 2013 at 08:31:48PM +0100, Felipe Balbi wrote:
 Hi,
 
 On Tue, Aug 20, 2013 at 12:56:03PM +0300, Ivan T. Ivanov wrote:
  From: Ivan T. Ivanov iiva...@mm-sol.com
  
  MSM USB3.0 core wrapper consist of USB3.0 IP from Synopsys
  (SNPS) and HS, SS PHY's control and configuration registers.
  
  It could operate in device mode (SS, HS, FS) and host
  mode (SS, HS, FS, LS).
  
  Signed-off-by: Ivan T. Ivanov iiva...@mm-sol.com
 
 Any acks for the DT part ? This patch has been pending forever.

Apologies for the delay. I have a couple of comments that would be nice
to fix up now.

 
  ---
   .../devicetree/bindings/usb/msm-ssusb.txt  |  104 
  
   1 file changed, 104 insertions(+)
   create mode 100644 Documentation/devicetree/bindings/usb/msm-ssusb.txt
  
  diff --git a/Documentation/devicetree/bindings/usb/msm-ssusb.txt 
  b/Documentation/devicetree/bindings/usb/msm-ssusb.txt
  new file mode 100644
  index 000..cacbd3b
  --- /dev/null
  +++ b/Documentation/devicetree/bindings/usb/msm-ssusb.txt
  @@ -0,0 +1,104 @@
  +MSM SuperSpeed DWC3 USB SoC controller
  +
  +
  +DWC3 Highspeed USB PHY
  +==
  +Required properities :
  +- compatible : sould be qcom,dwc3-hsphy;

s/sould/should/

In general, compatible properties are should contain rather than
should be, as we might have backwards compatible hardware in future.

  +- reg : offset and length of the register set in the memory map
  +- clocks : phandles to clock instances of the device tree nodes

Clocks are referred to be phandle + clock-specifier pairs rather than
phandles, it would be nice to fix the terminology here

  +- clock-names :
  +   xo : External reference clock 19 MHz
  +   sleep_a : Sleep clock, used when USB3 core goes into low
  +   power mode (U3).

I think it would be nicer to say:
- clocks : A list of phandle + clock-specifier pairs for the clocks
   listed in clock-names
- clock-names: Should contain the following:
   xo  - External reference clock (19MHz)
   sleep_a - Sleep clock used when USB3 core goes into low
   power mode (U3).

I'm not sure we need to describe the frequency of the xo clock -- either
it's a requriement of the IP and thus doesn't need to be a part of the
binding, or it's configurable by the driver and thus doesn't need to be
documented.

  +supply-name-supply : phandle to the regulator device tree node
  +Required supply-name are:
  +   v1p8 : 1.8v supply for HSPHY
  +   v3p3 : 3.3v supply for HSPHY
  +   vbus : vbus supply for host mode
  +   vddcx : vdd supply for HS-PHY digital circuit operation

Any reason for the HSPHY/HS-PHY difference?

I'd list these separately with their full names:

- v1p8-supply: phandle to the regulator for the 1.8v supply to HSPHY.
- v3p3-supply: phandle to the regulator for the 3.3v supply to HSPHY.
- vbus-supply: phandle to the regulator for the vbus supply for host
   mode.
- vddcx-supply: phandle to the regulator for the vdd supply for HSPHY
  digital circuit operation.

  +
  +DWC3 Superspeed USB PHY
  +===
  +Required properities :
  +- compatible : sould be qcom,dwc3-ssphy;
  +- reg : offset and length of the register set in the memory map
  +- clocks : phandles to clock instances of the device tree nodes
  +- clock-names :
  +   xo : External reference clock 19 MHz
  +   ref : Reference clock - used in host mode.
  +supply-name-supply : phandle to the regulator device tree node
  +Required supply-name are:
  +   v1p8 : 1.8v supply for SS-PHY
  +   vddcx : vdd supply for SS-PHY digital circuit operation

The commments on compatible, clocks, clock-names and the regulators
apply here.

  +
  +DWC3 controller wrapper
  +===
  +Required properties :
  +- compatible : should be qcom,dwc3
  +- reg : offset and length of the register set in the memory map
  +   offset and length of the TCSR register for routing USB
  +   signals to either picoPHY0 or picoPHY1.
  +- clocks : phandles to clock instances of the device tree nodes
  +- clock-names :
  +   core : Master/Core clock, have to be = 125 MHz for SS
  +   operation and = 60MHz for HS operation
  +   iface : System bus AXI clock
  +   sleep : Sleep clock, used when USB3 core goes into low
  +   power mode (U3).
  +   utmi : Generated by HS-PHY. Used to clock the low power
  +   parts of thr HS Link layer.
  +Optional properties :
  +- gdsc-supply : phandle to the globally distributed switch controller
  +  regulator node to the USB controller.

The commments on compatible, clocks, and clock-names apply here too. I
see the regulator is defined individually :)

I'm fine with the binding itself, I'd just like the documentation fixed
up.

Cheers,
Mark.

  +Required child node:
  +A child node must exist to represent the core DWC3 IP block. The name of
  +the node is not important. The content of the node is defined in dwc3.txt.
  +
  +Example device nodes:
  +
  +   

Re: [PATCH] usb: dwc3: core: clarify usb-phy array binding

[Mark Rutland]

On Wed, Sep 18, 2013 at 03:21:18PM +0100, Felipe Balbi wrote:
 Hi,
 
 On Wed, Aug 28, 2013 at 05:01:51PM +0100, Mark Rutland wrote:
So it's not physically possible for someone to just wire up a single phy
to the device, either USB2-only or USB3?
   
   of course it is :-) In fact, TI has done it. But it causes a whole bunch
   of other problems to support that sort of model. For one, in some
   systems, a clock generated by the USB3 PHY is backfed into the IP and if
   USB3 PHY isn't running, the dwc3 IP won't start.
  
  That sounds like a mess. But unless I've misunderstood, that doesn't
 
 well, it is :-)
 
  sound like a general problem with having one phy, but rather an
  integration issue on a specific system? Presumably in that case as long
  as the phy was brought up before poking the rest of the IP, the unit
  would function correctly.
 
 right, but what 'brings up' the PHY is usb_phy_init(). If we don't add
 usb3phy to DTS or skip usb3phy in case maximum-speed  superspeed, then
 we're screwed :-)

:(

 
   I even wrote a patch making USB3 PHY optional, but didn't push it
   exactly because it broke some other systems and I can't guarantee users
   won't mess up their DTS/pdata.
  
  Does that mean that their dts or pdata are wrong at the moment, but
  they're spared because the driver bails out due to a missing phy? If
  their pdata's wrong, that should be fixable relatively easily, but if
  the dt is wrong then I'm not sure how we can support those systems
  sensibly. That sounds like an ideal candidate for a dt quirks
  mechanism...
 
 hmm, the idea of the patch was:
 
   switch (maximum-speed) {
   case SUPER:
   grab_and_initialize_usb3_phy();
   grab_and_initialize_usb2_phy();
   break;
   case HIGH:
   case FULL:
   case LOW:
   grab_and_initialize_usb2_phy();
   break;
   }
 
 now, imagine someone wants to run his dwc3 in highspeed mode, we
 wouldn't initialize USB3 PHY which could cause the whole IP to break.

When you say wants to run it in highspeed mode, you mean that they want
this configured at run-time, or they deliberately omit a phy when
describing the hardware in the DT?

For the former I appreciate that it's a problem, but for the latter I'd
argue that's their fault. As far as I can see we initialise both PHYs in
the probe path and never uninitialise them, so the only problem would be
if someone's trying to be too clever. As we never check the return value
of usb_phy_init, they can still attempt to work around our best
efforts...

I appreciate that we should not break existing DTs. More on that below.

 
 I tried poking around on device's registers to see if there was any way
 to detect that the IP needs somethin back from USB3 PHY, but couldn't
 find anything.
 
 Since we can't know how the IP was integrated, it's best to leave it
 alone and require NOP xceiv to be used.

Agreed for the existing systems, but I still think we can work around
this for new DTs...

 
You can describe the USB2-only case in the dt by only listing the first
phy (though the driver won't support it as it expects both to be
present), but it's impossible to describe that you've wired up a single
phy that's USB3 capable.
   
   you might be right there...
  
  Would it be possible to have something like (an optional) usb-phy-names?
  If not present, we assume the current situation, if present then we use
  it to figure out which phys are present. Maybe I'm missing something
  that makes that impossible?
 
 you're missing the point regarding the IP possibly needing something
 back from the PHY (e.g. a clock which PHY generates).

I'm not sure why that detracts from the usb-phy-names idea -- if not
present, we stick with the current behaviour and require both PHYs or
fail early. No existing dts suddently explode, though none gain new
functionality either.

If someone's explicitly placed usb-phy-names, we know that they're
up-to-date on the lastest binding, something like:

 - usb-phy: A list of phandles to PHYs. If usb-phy-names is not
present, the USB2/HS PHY should be first, followed by the
USB3/SS PHY. If usb-phy-names is present, it defines the
order of PHYs.

 - usb-phy-names: The names of PHYs described in the usb-phy property.
  Valid values are usb2 and usb3. Should be used iff
  a subset of PHYs are connected.

  Compatibility note: The DWC3 IP can be integrated in
  such a way as to require outputs from particular PHYs
  for *any* level of operation. This cannot be detected
  by the OS, and on such systems all required PHYs must
  be described.

Given that, if they list fewer PHYs than present and their system really
requires a particular PHY, we can quite happily allow their system to
explode in the knowledge it's their fault, not ours

Re: [PATCH 04/11] usb: usb: dsps: update device tree bindings

[Mark Rutland]

On Tue, Aug 20, 2013 at 05:35:46PM +0100, Sebastian Andrzej Siewior wrote:
 The support for both am335x-USB instances required changes to the device
 tree bindings. This patch reflects these changes in the bindings
 document.
 
 v3…v4:
 - remove the child node for USB. This is driver specific on won't be
   reflected in the device tree
 - use the mentor prefix instead of mg.
 - use dr_mode istead of mg,port-mode for the port mode. The former
   is used by a few other drivers.
 
 v2…v3:
 - use proper usb-phy nodes in evm, bone and evmsk device tree.
 
 v1…v2:
 - use mg prefix for the Metor Graphics specific attributes
 - use power in mA not in mA/2 as specifed in the USB2.0 specification
 - use usbX-phy instead of usbX_phy
 - use dma-controller instead of dma
 
 Cc: Rob Herring rob.herr...@calxeda.com
 Cc: Pawel Moll pawel.m...@arm.com
 Cc: Mark Rutland mark.rutl...@arm.com
 Cc: Stephen Warren swar...@wwwdotorg.org
 Cc: Ian Campbell ian.campb...@citrix.com
 Cc: devicet...@vger.kernel.org
 Signed-off-by: Sebastian Andrzej Siewior bige...@linutronix.de
 ---
  .../devicetree/bindings/usb/am33xx-usb.txt | 222 
 ++---
  1 file changed, 192 insertions(+), 30 deletions(-)
 
 diff --git a/Documentation/devicetree/bindings/usb/am33xx-usb.txt 
 b/Documentation/devicetree/bindings/usb/am33xx-usb.txt
 index dc9dc8c..20c2ff2 100644
 --- a/Documentation/devicetree/bindings/usb/am33xx-usb.txt
 +++ b/Documentation/devicetree/bindings/usb/am33xx-usb.txt
 @@ -1,35 +1,197 @@
 -AM33XX MUSB GLUE
 - - compatible : Should be ti,musb-am33xx
 - - reg : offset and length of register sets, first usbss, then for musb 
 instances
 - - interrupts : usbss, musb instance interrupts in order
 - - ti,hwmods : must be usb_otg_hs
 - - multipoint : Should be 1 indicating the musb controller supports
 -   multipoint. This is a MUSB configuration-specific setting.
 - - num-eps : Specifies the number of endpoints. This is also a
 -   MUSB configuration-specific setting. Should be set to 16
 - - ram-bits : Specifies the ram address size. Should be set to 12
 - - port0-mode : Should be 3 to represent OTG. 1 signifies HOST and 2
 -   represents PERIPHERAL.
 - - port1-mode : Should be 1 to represent HOST. 3 signifies OTG and 2
 -   represents PERIPHERAL.
 - - power : Should be 250. This signifies the controller can supply up to
 -   500mA when operating in host mode.
 +  AM33xx MUSB
 +~~~
 +- compatible: ti,am33xx-usb
 +- reg: offset and length of the usbss register sets
 +- ti,hwmods : must be usb_otg_hs
 +
 +The glue layer contains multiple child nodes. It is required the have
 +at least a control module node, USB node and a PHY node. The second USB
 +node and its PHY node is optional. The DMA node is also optional.
 +
 +Reset module
 +
 +- compatible: ti,am335x-usb-ctrl-module
 +- reg: offset and length of the USB control registers in the Control
 +  Module block. A second offset and length for the USB wake up control
 +  in the same memory block.
 +- reg-names: phy_ctrl for the USB control registers and wakeup for
 +  the USB wake up control register.
 +
 +USB PHY
 +~~~
 +compatible: ti,am335x-usb-phy
 +reg: offset and length of the USB PHY register space
 +ti,ctrl_mod: reference to the reset module node
 +reg-names: phy

For consistency, these should be bullet bpoints (as with the Reset
module properties above).

Also, preferably:

- reg-names: should contain phy.

 +The PHY should have a phy alias numbered properly in the alias
 +node.

Why?

 +
 +USB
 +~~~
 +- compatible: ti,musb-am33xx
 +- reg: offset and length of USB Controller Registers, and offset and
 +  length of USB Core register space.
 +- reg-names: control for the USB Controller Registers and mc for
 +  USB Core register space

Consistent quoting please.

 +- interrupts: USB interrupt number

- interrupts: should contain an interrupt-specifier for the USB
  interrupt.

Is this the only interrupt? Is there a particular name for it in any
data sheet?

 +- interrupt-names: mc

String property values quoted please:

- interrupt-names: should contain mc for the USB interrupt.

Is mc the name of the interrupt on the data sheet?

 +- dr_mode: Should be one of host, peripheral or otg.
 +- mentor,multipoint: Should be 1 indicating the musb controller supports
 +  multipoint. This is a MUSB configuration-specific setting.

Is this a boolean value?

Why is this not an empty property?

 +- mentor,num-eps: Specifies the number of endpoints. This is also a
 +  MUSB configuration-specific setting. Should be set to 16
 +- mentor,ram-bits: Specifies the ram address size. Should be set to 12

Is this to define the address space the controller can see? What if it's
offset from the CPU's view? This sounds like something better described
via another means (like dma-ranges).

 +- mentor,power: Should be 500. This signifies the controller can supply up 
 to
 +  500mA when operating in host mode.

Why do we nee a particular value in a binding

Re: [PATCH] usb: dwc3: core: clarify usb-phy array binding

[Mark Rutland]

On Tue, Aug 27, 2013 at 07:53:29PM +0100, Felipe Balbi wrote:
 Hi,
 
 On Tue, Aug 13, 2013 at 02:34:10PM +0100, Mark Rutland wrote:
  On Mon, Aug 12, 2013 at 07:05:53PM +0100, Felipe Balbi wrote:
   On Fri, Aug 09, 2013 at 01:42:15PM -0500, Kumar Gala wrote:

On Aug 9, 2013, at 11:28 AM, Mark Rutland wrote:

 On Fri, Aug 09, 2013 at 04:40:32PM +0100, Kumar Gala wrote:
 The binding spec wasn't clear that the order of the phandles in the
 usb-phy array has meaning.  Clarify this point in the binding that
 it should be USB2-HS-PHY, USB3-SS-PHY.
 
 Signed-off-by: Kumar Gala ga...@codeaurora.org
 ---
 Documentation/devicetree/bindings/usb/dwc3.txt | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)
 
 diff --git a/Documentation/devicetree/bindings/usb/dwc3.txt 
 b/Documentation/devicetree/bindings/usb/dwc3.txt
 index 7a95c65..8a9770b 100644
 --- a/Documentation/devicetree/bindings/usb/dwc3.txt
 +++ b/Documentation/devicetree/bindings/usb/dwc3.txt
 @@ -6,7 +6,9 @@ Required properties:
  - compatible: must be synopsys,dwc3
  - reg : Address and length of the register set for the device
  - interrupts: Interrupts used by the dwc3 controller.
 - - usb-phy : array of phandle for the PHY device
 + - usb-phy : array of phandle for the PHY device.  The first element
 +   in the array is expected to be a handle to the USB2/HS PHY and
 +   the second element is expected to be a handle to the USB3/SS PHY
 
 Just to check - it's not valid to have a USB3 phy without a USB2 phy?

Don't know, hopefully Felipe will chime in on that.
   
   that'd be a really non-standard implementation. Per-spec, USB3 is
   *always* backwards compatible with USB2.
  
  I'm slightly confused here. From a quick look at the driver, it expects
  two separate phys to be present -- one handling only USB2, and one
  handling USB3 (with USB2 backwards compatibility).
  
  So it's not physically possible for someone to just wire up a single phy
  to the device, either USB2-only or USB3?
 
 of course it is :-) In fact, TI has done it. But it causes a whole bunch
 of other problems to support that sort of model. For one, in some
 systems, a clock generated by the USB3 PHY is backfed into the IP and if
 USB3 PHY isn't running, the dwc3 IP won't start.

That sounds like a mess. But unless I've misunderstood, that doesn't
sound like a general problem with having one phy, but rather an
integration issue on a specific system? Presumably in that case as long
as the phy was brought up before poking the rest of the IP, the unit
would function correctly.

 
 I even wrote a patch making USB3 PHY optional, but didn't push it
 exactly because it broke some other systems and I can't guarantee users
 won't mess up their DTS/pdata.

Does that mean that their dts or pdata are wrong at the moment, but
they're spared because the driver bails out due to a missing phy? If
their pdata's wrong, that should be fixable relatively easily, but if
the dt is wrong then I'm not sure how we can support those systems
sensibly. That sounds like an ideal candidate for a dt quirks
mechanism...

 
  You can describe the USB2-only case in the dt by only listing the first
  phy (though the driver won't support it as it expects both to be
  present), but it's impossible to describe that you've wired up a single
  phy that's USB3 capable.
 
 you might be right there...

Would it be possible to have something like (an optional) usb-phy-names?
If not present, we assume the current situation, if present then we use
it to figure out which phys are present. Maybe I'm missing something
that makes that impossible?

Thanks,
Mark.
--
To unsubscribe from this list: send the line unsubscribe linux-usb in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] usb: dwc3: core: clarify usb-phy array binding

[Mark Rutland]

On Mon, Aug 12, 2013 at 07:05:53PM +0100, Felipe Balbi wrote:
 On Fri, Aug 09, 2013 at 01:42:15PM -0500, Kumar Gala wrote:
  
  On Aug 9, 2013, at 11:28 AM, Mark Rutland wrote:
  
   On Fri, Aug 09, 2013 at 04:40:32PM +0100, Kumar Gala wrote:
   The binding spec wasn't clear that the order of the phandles in the
   usb-phy array has meaning.  Clarify this point in the binding that
   it should be USB2-HS-PHY, USB3-SS-PHY.
   
   Signed-off-by: Kumar Gala ga...@codeaurora.org
   ---
   Documentation/devicetree/bindings/usb/dwc3.txt | 4 +++-
   1 file changed, 3 insertions(+), 1 deletion(-)
   
   diff --git a/Documentation/devicetree/bindings/usb/dwc3.txt 
   b/Documentation/devicetree/bindings/usb/dwc3.txt
   index 7a95c65..8a9770b 100644
   --- a/Documentation/devicetree/bindings/usb/dwc3.txt
   +++ b/Documentation/devicetree/bindings/usb/dwc3.txt
   @@ -6,7 +6,9 @@ Required properties:
- compatible: must be synopsys,dwc3
- reg : Address and length of the register set for the device
- interrupts: Interrupts used by the dwc3 controller.
   - - usb-phy : array of phandle for the PHY device
   + - usb-phy : array of phandle for the PHY device.  The first element
   +   in the array is expected to be a handle to the USB2/HS PHY and
   +   the second element is expected to be a handle to the USB3/SS PHY
   
   Just to check - it's not valid to have a USB3 phy without a USB2 phy?
  
  Don't know, hopefully Felipe will chime in on that.
 
 that'd be a really non-standard implementation. Per-spec, USB3 is
 *always* backwards compatible with USB2.

I'm slightly confused here. From a quick look at the driver, it expects
two separate phys to be present -- one handling only USB2, and one
handling USB3 (with USB2 backwards compatibility).

So it's not physically possible for someone to just wire up a single phy
to the device, either USB2-only or USB3?

You can describe the USB2-only case in the dt by only listing the first
phy (though the driver won't support it as it expects both to be
present), but it's impossible to describe that you've wired up a single
phy that's USB3 capable.

Thanks,
Mark.
--
To unsubscribe from this list: send the line unsubscribe linux-usb in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: Non-enumerable devices on USB and other enumerable buses

[Mark Rutland]

[Adding Olof]

On Mon, Aug 12, 2013 at 10:51:36AM +0100, Mark Brown wrote:
 On Sun, Aug 11, 2013 at 09:53:01PM -0400, Alan Stern wrote:
  On Sun, 11 Aug 2013, Mark Brown wrote:
 
   One example that's bugging me right now is that on the Insignal Arndale
   platform there's a USB hub connected to one of the USB ports on the SoC
   (not as a PHY, it seems we also need the internal PHY running to talk to
   the device).  The hub needs to be plugged into the SoC after the SoC
   USB controller has started with some GPIOs so we need to tell the system
   that the hub exists and needs to be synchronised with the USB controller.
 
  On the surface, this seems like a particularly simple case.  Why wait 
  until the SoC's USB controller has started?  Why not plug in the hub 
  via the GPIOs right from the beginning?
 
 I tried that, it doesn't seem to work - for some reason it seems that
 the hub is only successfully enumerated if it starts after its parent is
 running.  I don't know enough about USB to speculate on why that might
 be, the GPIOs are brining the device out of reset not applying power or
 anything.
 
   Another case that's going to be problematic once it's in mainline is
   Slimbus - this is a bus used in some embedded audio subsystems which is
   enumerable in a similar manner to USB but where the devices on the bus
   are normally powered up only on demand (causing them to hotplug when
   used and unplug when idle) and have at least interrupt lines wired to
   the SoC using a normal interrupt outside the enumerable bus.
 
  That is indeed more difficult, because it requires geniune cooperation 
  between the bus and platform subsystems.
 
 Yeah.  You might want to do the same with for example a USB network
 controller when you're in flight only mode, that seems to be one of the
 more common reasons for doing this sort of thing with USB.

As I understand it, the wifi chip on the Snow Chromebook has a similar
issue -- it hangs off of a probeable SDIO bus, but needs a regulator
poked for it to turn on and become probeable (see
exynos_wifi_bt_set_power in [1]).

Thanks,
Mark.

[1] 
http://git.chromium.org/gitweb/?p=chromiumos/third_party/kernel.git;a=blob;f=arch/arm/mach-exynos/mach-exynos5-dt.c;h=d131241ea78b7d89df21e676e284ddfd369b4da0;hb=refs/heads/chromeos-3.4
--
To unsubscribe from this list: send the line unsubscribe linux-usb in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] usb: dwc3: core: clarify usb-phy array binding

[Mark Rutland]

On Fri, Aug 09, 2013 at 04:40:32PM +0100, Kumar Gala wrote:
 The binding spec wasn't clear that the order of the phandles in the
 usb-phy array has meaning.  Clarify this point in the binding that
 it should be USB2-HS-PHY, USB3-SS-PHY.
 
 Signed-off-by: Kumar Gala ga...@codeaurora.org
 ---
  Documentation/devicetree/bindings/usb/dwc3.txt | 4 +++-
  1 file changed, 3 insertions(+), 1 deletion(-)
 
 diff --git a/Documentation/devicetree/bindings/usb/dwc3.txt 
 b/Documentation/devicetree/bindings/usb/dwc3.txt
 index 7a95c65..8a9770b 100644
 --- a/Documentation/devicetree/bindings/usb/dwc3.txt
 +++ b/Documentation/devicetree/bindings/usb/dwc3.txt
 @@ -6,7 +6,9 @@ Required properties:
   - compatible: must be synopsys,dwc3
   - reg : Address and length of the register set for the device
   - interrupts: Interrupts used by the dwc3 controller.
 - - usb-phy : array of phandle for the PHY device
 + - usb-phy : array of phandle for the PHY device.  The first element
 +   in the array is expected to be a handle to the USB2/HS PHY and
 +   the second element is expected to be a handle to the USB3/SS PHY

Just to check - it's not valid to have a USB3 phy without a USB2 phy?

Mark.
--
To unsubscribe from this list: send the line unsubscribe linux-usb in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [RFC 1/2] usb: phy: Add Qualcomm SS-USB and HS-USB drivers for DWC3 core

[Mark Rutland]

On Tue, Aug 06, 2013 at 03:36:33PM +0100, Ivan T. Ivanov wrote:
 Hi,
 
 On Tue, 2013-08-06 at 15:03 +0100, Mark Rutland wrote:
  On Tue, Aug 06, 2013 at 12:53:10PM +0100, Ivan T. Ivanov wrote:
   From: Ivan T. Ivanov iiva...@mm-sol.com
  
   Signed-off-by: Ivan T. Ivanov iiva...@mm-sol.com
   ---
.../devicetree/bindings/usb/msm-ssusb.txt  |   49 +++
drivers/usb/phy/Kconfig|   11 +
drivers/usb/phy/Makefile   |2 +
drivers/usb/phy/phy-msm-dwc3-usb2.c|  342 
   +
drivers/usb/phy/phy-msm-dwc3-usb3.c|  389 
   
5 files changed, 793 insertions(+)
create mode 100644 Documentation/devicetree/bindings/usb/msm-ssusb.txt
create mode 100644 drivers/usb/phy/phy-msm-dwc3-usb2.c
create mode 100644 drivers/usb/phy/phy-msm-dwc3-usb3.c
  
   diff --git a/Documentation/devicetree/bindings/usb/msm-ssusb.txt 
   b/Documentation/devicetree/bindings/usb/msm-ssusb.txt
   new file mode 100644
   index 000..550b496
   --- /dev/null
   +++ b/Documentation/devicetree/bindings/usb/msm-ssusb.txt
   @@ -0,0 +1,49 @@
   +MSM SuperSpeed USB3.0 SoC controllers
   +
   +Required properities :
   +- compatible sould be qcom,dwc3-usb2;
   +- reg : offset and length of the register set in the memory map
   +- clocks: cxo, usb2a_phy_sleep_cxc;
  
  Huh? That doesn't describe what these are. These would be better
  explained with a reference to clock-names and a basic description as to
  what the input's called, what it drives, etc, as you've done done for
  the *-supply properties.
 
 Ok, I will fix this.
 
  
   +- clock-names: xo, sleep_a_clk;
   +supply-name-supply: phandle to the regulator device tree node
   +Required supply-name examples are:
   +   v1p8 : 1.8v supply for HSPHY
   +   v3p3 : 3.3v supply for HSPHY
   +   vbus : vbus supply for host mode
   +   vddcx : vdd supply for HS-PHY digital circuit operation
   +
   +Required properities :
   +- compatible sould be qcom,dwc3-usb3;
   +- reg : offset and length of the register set in the memory map
   +- clocks: cxo, usb30_mock_utmi_cxc;
  
  Similarly, this doesn't describe what the clocks are.
 
 Understood.
 
  
   +- clock-names: xo, ref_clk;
   +supply-name-supply: phandle to the regulator device tree node
   +Required supply-name examples are:
   +   v1p8 : 1.8v supply for SS-PHY
   +   vddcx : vdd supply for SS-PHY digital circuit operation
   +
   +Example device nodes:
   +
   +   dwc3_usb2: phy@f92f8800 {
   +   compatible = qcom,dwc3-usb2;
   +   reg = 0xf92f8800 0x30;
   +
   +   clocks = cxo, usb2a_phy_sleep_cxc;
   +   clock-names = xo, sleep_a_clk;
   +
   +   vbus-supply = supply;
   +   vddcx-supply = supply;
   +   v1p8-supply = supply;
   +   v3p3-supply = supply;
   +   };
   +
   +   dwc3_usb3: phy@f92f8830 {
   +   compatible = qcom,dwc3-usb3;
   +   reg = 0xf92f8830 0x30;
   +
   +   clocks = cxo, usb30_mock_utmi_cxc;
   +   clock-names = xo, ref_clk;
   +
   +   vddcx-supply = supply;
   +   v1p8-supply = supply;
   +   };
  
  
  Those regster banks look suspiciously close. Are these the same IP
  block? Can they ever appear separately?
  
 
 They are part of the wrapper Qualcomm logic around Synopsys USB3 core.
 In this sense they are part of the one IP, I believe. Manage them from
 separate drivers simplify code.

Hmmm. I'm not entirely certain on this. On the one hand, they're
separate IP blocks, and have lgoically separate drivers, so describing
them as two devices makes sense. On the other hand, they've been fused
into one IP block with shared resources. Describing them as two devices
probably makes sense given you have the wrapper driver.

 
  Do the drivers not trample each other when messing with shared clocks
  and regulators?
  
 
 Regulators and clocks have reference counting, right?, so this should
 be safe. Even if they are part of the one driver, clocks and regulators
 could be switched off only if both PHY's do not use them.

Ok, I just wanted to be sure this had been considered :)

Thanks,
Mark.
--
To unsubscribe from this list: send the line unsubscribe linux-usb in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/3 v5] usb: phy-samsung-usb: Simplify PMU register handling

[Mark Rutland]

On Wed, Aug 07, 2013 at 06:06:05PM +0100, Julius Werner wrote:
  This breaks compatibility, both for an old kernel and a new dt and a new
  kernel with an old dt. Is anyone using these bindings?
 
 They only affect Samsung SoCs and have only been upstream for half a
 year, so I doubt it's heavily used.

I'm not sure everyone will be happy with that line.

 
  Why are we describing fewer registers now? Are they described elsewhere?
 
  The dt should describe the device, not only the portion of it Linux
  wants to use right now.
 
 This only ever described a small section of the huge set of PMU
 registers anyway. Before it described up to three registers
 controlling different PHYs (using hardcoded offsets in the code to
 later find the right one)... with my patch every PHY's DT entry only
 describes the one register concerning itself, which makes more sense
 in my opinion. It will also prevent the register descriptions in
 different DT entries from overlapping.
 

I'm not sure I understand. The old documentation referred to the
USBDEVICE_PHY_CONTROL and USBHOST_PHY_CONTROL registers for a phy, and
your new version only refers to (usb device) PHY_CONTROL. Regardless of
multiple phys, you're suggesting that we describe less of each phy.
That seems like taking away usable information. Unless I've
misunderstood?

Ideally, we'd describe the whole set of registers and linkages to phys,
even if Linux doesn't ahppen to use that information right now.

Thanks,
Mark.
--
To unsubscribe from this list: send the line unsubscribe linux-usb in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/3 v5] usb: phy-samsung-usb: Simplify PMU register handling

[Mark Rutland]

On Tue, Aug 06, 2013 at 07:00:17PM +0100, Julius Werner wrote:
 This patch simplifies the way the phy-samsung-usb code finds the correct
 power management register to enable PHY clock gating. Previously, the
 code would calculate the register address from a device tree supplied
 base address and add an offset based on the PHY type.
 
 Since every PHY has its own device tree entry and needs only one
 register, we can just encode the address itself in the device tree and
 remove the diffentiation in the code. The bitmask needed to specify the
 bit within that register stays in place, allowing support for platforms
 like s3c64xx that use different bits within the same register.

This breaks compatibility, both for an old kernel and a new dt and a new
kernel with an old dt. Is anyone using these bindings?


 
 Signed-off-by: Julius Werner jwer...@chromium.org
 ---
  .../devicetree/bindings/usb/samsung-usbphy.txt | 26 
 +-
  arch/arm/boot/dts/exynos5250.dtsi  |  4 ++--
  drivers/usb/phy/phy-samsung-usb.c  | 18 ---
  drivers/usb/phy/phy-samsung-usb.h  | 23 +--
  drivers/usb/phy/phy-samsung-usb2.c | 11 -
  drivers/usb/phy/phy-samsung-usb3.c |  2 +-
  6 files changed, 22 insertions(+), 62 deletions(-)
 
 diff --git a/Documentation/devicetree/bindings/usb/samsung-usbphy.txt 
 b/Documentation/devicetree/bindings/usb/samsung-usbphy.txt
 index 33fd354..1cf9b68 100644
 --- a/Documentation/devicetree/bindings/usb/samsung-usbphy.txt
 +++ b/Documentation/devicetree/bindings/usb/samsung-usbphy.txt
 @@ -34,14 +34,7 @@ Optional properties:
  - The child node 'usbphy-sys' to the node 'usbphy' is for the system 
 controller
interface for usb-phy. It should provide the following information 
 required by
usb-phy controller to control phy.
 -  - reg : base physical address of PHY_CONTROL registers.
 -   The size of this register is the total sum of size of all PHY_CONTROL
 -   registers that the SoC has. For example, the size will be
 -   '0x4' in case we have only one PHY_CONTROL register (e.g.
 -   OTHERS register in S3C64XX or USB_PHY_CONTROL register in S5PV210)
 -   and, '0x8' in case we have two PHY_CONTROL registers (e.g.
 -   USBDEVICE_PHY_CONTROL and USBHOST_PHY_CONTROL registers in exynos4x).
 -   and so on.
 +  - reg : address of PHY_CONTROL register for this PHY.
  
  Example:
   - Exynos4210
 @@ -57,8 +50,8 @@ Example:
   clock-names = xusbxti, otg;
  
   usbphy-sys {
 - /* USB device and host PHY_CONTROL registers */
 - reg = 0x10020704 0x8;
 + /* USB device PHY_CONTROL register */
 + reg = 0x10020704 0x4;
   };
   };
  

Why are we describing fewer registers now? Are they described elsewhere?

The dt should describe the device, not only the portion of it Linux
wants to use right now.

Thanks,
Mark.
--
To unsubscribe from this list: send the line unsubscribe linux-usb in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [RFC 1/2] usb: phy: Add Qualcomm SS-USB and HS-USB drivers for DWC3 core

[Mark Rutland]

On Tue, Aug 06, 2013 at 12:53:10PM +0100, Ivan T. Ivanov wrote:
 From: Ivan T. Ivanov iiva...@mm-sol.com

 Signed-off-by: Ivan T. Ivanov iiva...@mm-sol.com
 ---
  .../devicetree/bindings/usb/msm-ssusb.txt  |   49 +++
  drivers/usb/phy/Kconfig|   11 +
  drivers/usb/phy/Makefile   |2 +
  drivers/usb/phy/phy-msm-dwc3-usb2.c|  342 +
  drivers/usb/phy/phy-msm-dwc3-usb3.c|  389 
 
  5 files changed, 793 insertions(+)
  create mode 100644 Documentation/devicetree/bindings/usb/msm-ssusb.txt
  create mode 100644 drivers/usb/phy/phy-msm-dwc3-usb2.c
  create mode 100644 drivers/usb/phy/phy-msm-dwc3-usb3.c

 diff --git a/Documentation/devicetree/bindings/usb/msm-ssusb.txt 
 b/Documentation/devicetree/bindings/usb/msm-ssusb.txt
 new file mode 100644
 index 000..550b496
 --- /dev/null
 +++ b/Documentation/devicetree/bindings/usb/msm-ssusb.txt
 @@ -0,0 +1,49 @@
 +MSM SuperSpeed USB3.0 SoC controllers
 +
 +Required properities :
 +- compatible sould be qcom,dwc3-usb2;
 +- reg : offset and length of the register set in the memory map
 +- clocks: cxo, usb2a_phy_sleep_cxc;

Huh? That doesn't describe what these are. These would be better
explained with a reference to clock-names and a basic description as to
what the input's called, what it drives, etc, as you've done done for
the *-supply properties.

 +- clock-names: xo, sleep_a_clk;
 +supply-name-supply: phandle to the regulator device tree node
 +Required supply-name examples are:
 +   v1p8 : 1.8v supply for HSPHY
 +   v3p3 : 3.3v supply for HSPHY
 +   vbus : vbus supply for host mode
 +   vddcx : vdd supply for HS-PHY digital circuit operation
 +
 +Required properities :
 +- compatible sould be qcom,dwc3-usb3;
 +- reg : offset and length of the register set in the memory map
 +- clocks: cxo, usb30_mock_utmi_cxc;

Similarly, this doesn't describe what the clocks are.

 +- clock-names: xo, ref_clk;
 +supply-name-supply: phandle to the regulator device tree node
 +Required supply-name examples are:
 +   v1p8 : 1.8v supply for SS-PHY
 +   vddcx : vdd supply for SS-PHY digital circuit operation
 +
 +Example device nodes:
 +
 +   dwc3_usb2: phy@f92f8800 {
 +   compatible = qcom,dwc3-usb2;
 +   reg = 0xf92f8800 0x30;
 +
 +   clocks = cxo, usb2a_phy_sleep_cxc;
 +   clock-names = xo, sleep_a_clk;
 +
 +   vbus-supply = supply;
 +   vddcx-supply = supply;
 +   v1p8-supply = supply;
 +   v3p3-supply = supply;
 +   };
 +
 +   dwc3_usb3: phy@f92f8830 {
 +   compatible = qcom,dwc3-usb3;
 +   reg = 0xf92f8830 0x30;
 +
 +   clocks = cxo, usb30_mock_utmi_cxc;
 +   clock-names = xo, ref_clk;
 +
 +   vddcx-supply = supply;
 +   v1p8-supply = supply;
 +   };


Those regster banks look suspiciously close. Are these the same IP
block? Can they ever appear separately?

Do the drivers not trample each other when messing with shared clocks
and regulators?

Thanks,
Mark.
--
To unsubscribe from this list: send the line unsubscribe linux-usb in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [RFC 2/2] usb: dwc3: Add Qualcomm DWC3 glue layer driver

[Mark Rutland]

On Tue, Aug 06, 2013 at 12:53:11PM +0100, Ivan T. Ivanov wrote:
 From: Ivan T. Ivanov iiva...@mm-sol.com

What does the glue layer do? Is it an actual piece of hardware, or
just some platform-specific code?

 
 Signed-off-by: Ivan T. Ivanov iiva...@mm-sol.com
 ---
  .../devicetree/bindings/usb/msm-ssusb.txt  |   39 +
  drivers/usb/dwc3/Kconfig   |8 +
  drivers/usb/dwc3/Makefile  |1 +
  drivers/usb/dwc3/dwc3-msm.c|  175 
 
  4 files changed, 223 insertions(+)
  create mode 100644 drivers/usb/dwc3/dwc3-msm.c
 
 diff --git a/Documentation/devicetree/bindings/usb/msm-ssusb.txt 
 b/Documentation/devicetree/bindings/usb/msm-ssusb.txt
 index 550b496..313ae0d 100644
 --- a/Documentation/devicetree/bindings/usb/msm-ssusb.txt
 +++ b/Documentation/devicetree/bindings/usb/msm-ssusb.txt
 @@ -22,6 +22,23 @@ Required supply-name examples are:
   v1p8 : 1.8v supply for SS-PHY
   vddcx : vdd supply for SS-PHY digital circuit operation
  
 +Required properties :
 +- compatible : should be qcom,dwc-usb3-msm
 +- reg : offset and length of the register set in the memory map
 + offset and length of the TCSR register for routing USB
 + signals to either picoPHY0 or picoPHY1.
 +- clocks = usb30_master_cxc, sys_noc_usb3_axi_cxc, usb30_sleep_cxc, 
 usb30_mock_utmi_cxc;

Similarly to my comment on patch 1, these need to be described better.

Thanks,
Mark.
--
To unsubscribe from this list: send the line unsubscribe linux-usb in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/8] USB: EHCI: make ehci-spear a separate driver

[Mark Rutland]

Hello,

[...]

 -static struct of_device_id spear_ehci_id_table[] = {
 -   { .compatible = st,spear600-ehci, },
 +static struct of_device_id SPEAr_ehci_id_table[] = {
 +   { .compatible = st,SPEAr600-ehci, },
 { },
  };

This will break anyone using the documented binding, and anyone with an
existing dt will suddenly find their usb support broken.

Please don't change this.

Thanks,
Mark.

--
To unsubscribe from this list: send the line unsubscribe linux-usb in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [V6 PATCH 01/16] usb: phy: mv_usb2: add PHY driver for marvell usb2 controller

[Mark Rutland]

Hi,

I have a few comments on the devicetree binding and the way it's parsed.

I note many are similar to those I commented on for the mv_udc and ehci-mv
devicetree code.

On Wed, Feb 06, 2013 at 07:23:36AM +, Chao Xie wrote:
 The PHY is seperated from usb controller.
 The usb controller used in marvell pxa168/pxa910/mmp2 are same,
 but PHY initialization may be different.
 the usb controller can support udc/otg/ehci, and for each of
 the mode, it need PHY to initialized before use the controller.
 Direclty writing the phy driver will make the usb controller
 driver to be simple and portable.
 The PHY driver will be used by marvell udc/otg/ehci.
 
 Signed-off-by: Chao Xie chao@marvell.com
 ---
  drivers/usb/phy/Kconfig  |7 +
  drivers/usb/phy/Makefile |1 +
  drivers/usb/phy/mv_usb2_phy.c|  454 
 ++
  include/linux/platform_data/mv_usb.h |9 +-
  include/linux/usb/mv_usb2.h  |   43 
  5 files changed, 511 insertions(+), 3 deletions(-)
  create mode 100644 drivers/usb/phy/mv_usb2_phy.c
  create mode 100644 include/linux/usb/mv_usb2.h


[...]

 +static struct of_device_id usb_phy_dt_ids[] = {
 +   { .compatible = mrvl,pxa168-usb-phy,  .data = (void *)PXA168_USB},
 +   { .compatible = mrvl,pxa910-usb-phy,  .data = (void *)PXA910_USB},
 +   { .compatible = mrvl,mmp2-usb-phy,.data = (void *)MMP2_USB},
 +   {}
 +};
 +MODULE_DEVICE_TABLE(of, usb_phy_dt_ids);

The binding (including these compatible string) needs to be documented.

 +
 +static int usb_phy_parse_dt(struct platform_device *pdev,
 +   struct mv_usb2_phy *mv_phy)
 +{
 +   struct device_node *np = pdev-dev.of_node;
 +   const struct of_device_id *of_id =
 +   of_match_device(usb_phy_dt_ids, pdev-dev);
 +   unsigned int clks_num;
 +   int i, ret;
 +   const char *clk_name;
 +
 +   if (!np)
 +   return 1;

An actual error code please.

-ENODEV? -EINVAL?

 +
 +   clks_num = of_property_count_strings(np, clocks);
 +   if (clks_num  0) {
 +   dev_err(pdev-dev, failed to get clock number\n);
 +   return clks_num;
 +   }

The common clock bindings use clocks as a list of phandle and clock-specifier
pairs. It seems bad to reuse that name in a different sense for this binding.

I'd recommend you use the common clock binding. There doesn't seem to be an
of_clk_count, but it should be a relatively simple addition, and it'd make this
code clearer and more consistent with other drivers.

See Documentation/devicetree/bindings/clock/clock-bindings.txt

 +
 +   mv_phy-clks = devm_kzalloc(pdev-dev,
 +   sizeof(struct clk *) * clks_num, GFP_KERNEL);
 +   if (mv_phy-clks == NULL) {
 +   dev_err(pdev-dev,
 +   failed to allocate mempory for clocks);

s/mempory/memory/

 +   return -ENOMEM;
 +   }
 +
 +   for (i = 0; i  clks_num; i++) {
 +   ret = of_property_read_string_index(np, clocks, i,
 +   clk_name);
 +   if (ret) {
 +   dev_err(pdev-dev, failed to read clocks\n);
 +   return ret;
 +   }
 +   mv_phy-clks[i] = devm_clk_get(pdev-dev, clk_name);
 +   if (IS_ERR(mv_phy-clks[i])) {
 +   dev_err(pdev-dev, failed to get clock %s\n,
 +   clk_name);
 +   return PTR_ERR(mv_phy-clks[i]);
 +   }
 +   }
 +
 +   mv_phy-clks_num = clks_num;
 +   mv_phy-type = (enum mv_usb2_phy_type)(of_id-data);
 +
 +   return 0;
 +}

There's probably a need for something like devm_of_clk_get to make it easier to
write of-backed drivers.

 +
 +static int usb_phy_probe(struct platform_device *pdev)
 +{
 +   struct mv_usb2_phy *mv_phy;
 +   struct resource *r;
 +   int ret, i;
 +
 +   mv_phy = devm_kzalloc(pdev-dev, sizeof(*mv_phy), GFP_KERNEL);
 +   if (mv_phy == NULL) {
 +   dev_err(pdev-dev, failed to allocate memory\n);
 +   return -ENOMEM;
 +   }
 +   mutex_init(mv_phy-phy_lock);
 +
 +   ret = usb_phy_parse_dt(pdev, mv_phy);
 +   /* no CONFIG_OF */
 +   if (ret  0) {

Reorganise this so you test for pdev-dev.of_node, and based of that you either
call usb_phy_parse_dt or do this stuff in the block below. That way you don't 
need
the positive return code from usb_phy_parse_dt.

 +   struct mv_usb_phy_platform_data *pdata
 +   = pdev-dev.platform_data;
 +   const struct platform_device_id *id
 +   = platform_get_device_id(pdev);
 +
 +   if (pdata == NULL || id == NULL) {
 +   dev_err(pdev-dev,
 +   missing platform_data or id_entry\n);
 +   return -ENODEV;
 +   

Re: [PATCH 08/13] USB: ehci-omap: Add device tree support and binding information

[Mark Rutland]

On Mon, Feb 04, 2013 at 03:58:55PM +, Roger Quadros wrote:
 Allows the OMAP EHCI controller to be specified via device tree.
 
 Signed-off-by: Roger Quadros rog...@ti.com
 ---
  .../devicetree/bindings/usb/omap-ehci.txt  |   34 ++
  drivers/usb/host/ehci-omap.c   |   36 
 +++-
  2 files changed, 69 insertions(+), 1 deletions(-)
  create mode 100644 Documentation/devicetree/bindings/usb/omap-ehci.txt
 
 diff --git a/Documentation/devicetree/bindings/usb/omap-ehci.txt 
 b/Documentation/devicetree/bindings/usb/omap-ehci.txt
 new file mode 100644
 index 000..90e6e3a
 --- /dev/null
 +++ b/Documentation/devicetree/bindings/usb/omap-ehci.txt
 @@ -0,0 +1,34 @@
 +OMAP HS USB EHCI controller
 +
 +This device is usually the child of the omap-usb-host
 +Documentation/devicetree/bindings/mfd/omap-usb-host.txt
 +
 +Required properties:
 +
 +- compatible: should be ti,omap-ehci
 +- reg: should contain one register range i.e. start and length
 +- interrupt-parent: phandle to the interrupt controller
 +- interrupts: description of the interrupt line
 +
 +Optional properties:
 +
 +- phy: list of phandles to PHY nodes.
 +  This property is required if at least one of the ports are in
 +  PHY mode i.e. OMAP_EHCI_PORT_MODE_PHY

Any reason for not calling this phys, given it's a list?

[...]

Thanks,
Mark.

--
To unsubscribe from this list: send the line unsubscribe linux-usb in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 09/13] mfd: omap-usb-host: Add device tree support and binding information

[Mark Rutland]

Hi,

I have a few comments on the binding and the way it's parsed.

On Mon, Feb 04, 2013 at 03:58:56PM +, Roger Quadros wrote:
 Allows the OMAP HS USB host controller to be specified
 via device tree.
 
 CC: Samuel Ortiz sa...@linux.intel.com
 Signed-off-by: Roger Quadros rog...@ti.com
 ---
  .../devicetree/bindings/mfd/omap-usb-host.txt  |   68 
  drivers/mfd/omap-usb-host.c|   83 
 ++--
  2 files changed, 145 insertions(+), 6 deletions(-)
  create mode 100644 Documentation/devicetree/bindings/mfd/omap-usb-host.txt
 
 diff --git a/Documentation/devicetree/bindings/mfd/omap-usb-host.txt 
 b/Documentation/devicetree/bindings/mfd/omap-usb-host.txt
 new file mode 100644
 index 000..2196893
 --- /dev/null
 +++ b/Documentation/devicetree/bindings/mfd/omap-usb-host.txt
 @@ -0,0 +1,68 @@
 +OMAP HS USB Host
 +
 +Required properties:
 +
 +- compatible: should be ti,usbhs-host
 +- reg: should contain one register range i.e. start and length
 +- ti,hwmods: must contain usb_host_hs
 +
 +Optional properties:
 +
 +- nports: number of USB ports. Usually this is automatically detected
 +  from the IP's revision register but can be overridden by specifying
 +  this property.

It would be nice if this were num-ports, as atmel-usb is already using that,
and it's clear that it's a number of ports rather than some other meaning of
'n'.

From a quick grep of binding documents, out of nTHING(s), nr-THINGs, and
num-THINGs, num-THINGs seems to be the most common. It would be nice if new
bindings could standardise this.

 +
 +- portN_mode: Integer specifying the port mode for port N, where N can be
 +  from 1 to nports. The port mode must be as per enum usbhs_omap_port_mode
 +  in include/linux/platform_data/usb-omap.h
 +  If the port mode is not specified, that port is treated as unused.

I'm against devicetree bindings refering to Linux internals. It makes a poorly
documented ABI that someone might change in future without realising the
implications, and it makes it stupidly difficult to read a dts.

Everything required should be specified in the binding document (or another
linked binding document). It might be better to describe this with a string
property that gets mapped by your dt parsing code to whatever internal
representation you need. That way it's far easier for a human to verify the dts
is correct, and you know by construction that the parsed value is something you
can handle in the driver.

It would be nicer is you used '-' rather than '_' for consistency with
devicetree bindings in general.

 +
 +- single_ulpi_bypass: Must be present if the controller contains a single
 +  ULPI bypass control bit. e.g. OMAP3 silicon = ES2.1

Again it would be nicer to have '-' rather than '_' here. It might be worth
prefixing this ti,.

 +
 +Required properties if child node exists:
 +
 +- #address-cells: Must be 1
 +- #size-cells: Must be 1
 +- ranges: must be present
 +
 +Properties for children:
 +
 +The OMAP HS USB Host subsystem contains EHCI and OHCI controllers.
 +See Documentation/devicetree/bindings/usb/omap-ehci.txt and
 +omap3-ohci.txt
 +
 +Example for OMAP4:
 +
 +usbhshost: usbhshost@0x4a064000 {
 + compatible = ti,usbhs-host;
 + reg = 0x4a064000 0x800;
 + ti,hwmods = usb_host_hs;
 + #address-cells = 1;
 + #size-cells = 1;
 + ranges;
 +
 + usbhsohci: ohci@0x4a064800 {
 + compatible = ti,omap3-ohci, usb-ohci;
 + reg = 0x4a064800 0x400;
 + interrupt-parent = gic;
 + interrupts = 0 76 0x4;
 + };
 +
 + usbhsehci: ehci@0x4a064c00 {
 + compatible = ti,omap-ehci, usb-ehci;
 + reg = 0x4a064c00 0x400;
 + interrupt-parent = gic;
 + interrupts = 0 77 0x4;
 + };
 +};
 +
 +usbhshost {
 + port1_mode = 1; /* OMAP_EHCI_PORT_MODE_PHY */
 + port2_mode = 2; /* OMAP_EHCI_PORT_MODE_TLL */
 + port3_mode = 1; /* OMAP_EHCI_PORT_MODE_PHY */

With a string property, these values would be self-documenting.

 +};
 +
 +usbhsehci {
 + phy = hsusb1_phy 0 hsusb3_phy;
 +};
 diff --git a/drivers/mfd/omap-usb-host.c b/drivers/mfd/omap-usb-host.c
 index f8ed08e..0f67856 100644
 --- a/drivers/mfd/omap-usb-host.c
 +++ b/drivers/mfd/omap-usb-host.c
 @@ -1,8 +1,9 @@
  /**
   * omap-usb-host.c - The USBHS core driver for OMAP EHCI  OHCI
   *
 - * Copyright (C) 2011 Texas Instruments Incorporated - http://www.ti.com
 + * Copyright (C) 2011-2013 Texas Instruments Incorporated - http://www.ti.com
   * Author: Keshava Munegowda keshava_mgo...@ti.com
 + * Author: Roger Quadros rog...@ti.com
   *
   * This program is free software: you can redistribute it and/or modify
   * it under the terms of the GNU General Public License version 2  of
 @@ -27,6 +28,8 @@
  #include linux/platform_device.h
  #include linux/platform_data/usb-omap.h
  #include linux/pm_runtime.h
 +#include linux/of.h
 +#include linux/of_platform.h
  
  #include omap-usb.h
  
 @@ 

Re: [PATCH 09/13] mfd: omap-usb-host: Add device tree support and binding information

[Mark Rutland]

[...]

  +
  +- single_ulpi_bypass: Must be present if the controller contains a single
  +  ULPI bypass control bit. e.g. OMAP3 silicon = ES2.1
  
  Again it would be nicer to have '-' rather than '_' here. It might be worth
  prefixing this ti,.
 
 Is prefixing with ti really required? how does it better?

I thought single-ulpi-bypass sounded rather generic, but it probably is
specific enough as-is. I don't know enough about USB hardware to have strong
feelings either way.

[...]

 Thanks for the in-depth review :).

You're welcome.

Thanks,
Mark.

--
To unsubscribe from this list: send the line unsubscribe linux-usb in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [V5 PATCH 24/26] usb: gadget: mv_udc: add device tree support

[Mark Rutland]

On Fri, Jan 25, 2013 at 02:13:54AM +, chao xie wrote:
 2013/1/24 Mark Rutland mark.rutl...@arm.com:
  Hello,
 
  On Thu, Jan 24, 2013 at 06:38:48AM +, Chao Xie wrote:
  In original driver, we have callbacks in platform data, and some
  dynamically allocations variables in platform data.
  Now, these blocks are removed, the device tree support is easier
  now.
 
  Please could you also add a binding document? See
  Documentation/devicetree/bindings/usb for examples of existing bindings.
 
  Also, please Cc devicetree-discuss when introducing a new binding as you are
  doing here.
 
 
  Signed-off-by: Chao Xie chao@marvell.com
  ---
   drivers/usb/gadget/mv_udc.h  |5 +-
   drivers/usb/gadget/mv_udc_core.c |  106 
  ++
   2 files changed, 86 insertions(+), 25 deletions(-)
 
  diff --git a/drivers/usb/gadget/mv_udc.h b/drivers/usb/gadget/mv_udc.h
  index 50ae7c7..de84722 100644
  --- a/drivers/usb/gadget/mv_udc.h
  +++ b/drivers/usb/gadget/mv_udc.h
  @@ -179,6 +179,7 @@ struct mv_udc {
int irq;
 
unsigned intextern_attr;
  + unsigned intmode;
struct notifier_block   notifier;
 
struct mv_cap_regs __iomem  *cap_regs;
  @@ -222,11 +223,9 @@ struct mv_udc {
struct mv_usb2_phy  *phy;
struct usb_phy  *transceiver;
 
  - struct mv_usb_platform_data *pdata;
  -
/* some SOC has mutiple clock sources for USB*/
unsigned intclknum;
  - struct clk  *clk[0];
  + struct clk  **clk;
   };
 
   /* endpoint data structure */
  diff --git a/drivers/usb/gadget/mv_udc_core.c 
  b/drivers/usb/gadget/mv_udc_core.c
  index 2e5907f..a4ee9a1 100644
  --- a/drivers/usb/gadget/mv_udc_core.c
  +++ b/drivers/usb/gadget/mv_udc_core.c
  @@ -34,6 +34,7 @@
   #include linux/irq.h
   #include linux/platform_device.h
   #include linux/clk.h
  +#include linux/of.h
   #include linux/platform_data/mv_usb.h
   #include linux/usb/mv_usb2.h
   #include asm/unaligned.h
  @@ -2153,21 +2154,57 @@ static int mv_udc_remove(struct platform_device 
  *pdev)
return 0;
   }
 
  +static int mv_udc_parse_dt(struct platform_device *pdev,
  + struct mv_udc *udc)
  +{
  + struct device_node *np = pdev-dev.of_node;
  + unsigned int clks_num;
  + int i, ret;
  + const char *clk_name;
  +
  + if (!np)
  + return 1;
  +
  + clks_num = of_property_count_strings(np, clocks);
  + if (clks_num  0)
  + return clks_num;
  +
  + udc-clk = devm_kzalloc(pdev-dev,
  + sizeof(struct clk *) * clks_num, GFP_KERNEL);
  + if (udc-clk == NULL)
  + return -ENOMEM;
  +
  + for (i = 0; i  clks_num; i++) {
  + ret = of_property_read_string_index(np, clocks, i,
  + clk_name);
  + if (ret)
  + return ret;
  + udc-clk[i] = devm_clk_get(pdev-dev, clk_name);
  + if (IS_ERR(udc-clk[i]))
  + return PTR_ERR(udc-clk[i]);
 
  I was going to ask if you couldn't use of_clk_get, but I see you want to use
  the devm_* functions to handle cleanup. It seems a shame there's not a 
  standard
  devm_of_clk_get.
 
 It is nice to have someone to review the device tree part patches.
 In fact main target of the modification is to remove the
 pointers/callbacks in the platform_data, so
 i can easly to add device tree support.
 
 of_clk_get is is based on CONFIG_COMMON_CLK. of_clk_get is not simple.
 The orginal way we use to
 get a clock used two arguments: dev_id and con_id. The dev_id is the
 name of the device.
 of_clk_get need clock framework changes. It means that clock driver
 need add device tree support. Now
 we are doing the clock tree support for Marvell MMP SOCes, but it will
 not be done in a short time.
 As i think, of_clk_get still have some problems. It parses the
 property's name as clocks, but it does not support
 mutipile clocks. If the device driver original has two clocks, the old
 way can do it as
 clk_get(dev, clock 1);
 clk_get(dev, clock 2);
 of_clk_get can not do it. I have not asked this question in the
 mailist, and i will do it soon.

I may have misunderstood here, but as far as I can see, of_clk_get supports
multiple clocks -- it even takes an index parameter:

struct clk *of_clk_get(struct device_node *np, int index)

I can see several dts files which have a clocks property with multiple clocks.
In arch/arm/boot/dts/vexpress-v2m.dtsi there's a arm,sp810 node with
multiple clocks, which I believe is dealt with by vexpress_clk_of_init in
drivers/clk/versatile/clk-vexpress.c (though this uses of_clk_get_by_name).

This also raises the issue that you expect the clocks property to be a list of
strings, while other bindings expect the clocks property to be list of
phandle/clock-specifier pairs.

It's worth taking

Re: [V5 PATCH 26/26] usb: ehci: ehci-mv: add device tree support

[Mark Rutland]

[...]

  @@ -170,19 +202,36 @@ static int mv_ehci_probe(struct platform_device 
  *pdev)
}
 
platform_set_drvdata(pdev, ehci_mv);
  - ehci_mv-pdata = pdata;
ehci_mv-hcd = hcd;
 
  - ehci_mv-clknum = pdata-clknum;
  - for (clk_i = 0; clk_i  ehci_mv-clknum; clk_i++) {
  - ehci_mv-clk[clk_i] =
  - devm_clk_get(pdev-dev, pdata-clkname[clk_i]);
  - if (IS_ERR(ehci_mv-clk[clk_i])) {
  - dev_err(pdev-dev, error get clck \%s\\n,
  - pdata-clkname[clk_i]);
  - retval = PTR_ERR(ehci_mv-clk[clk_i]);
  - goto err_clear_drvdata;
  + retval = mv_ehci_parse_dt(pdev, ehci_mv);
  + if (retval  0) {
 
  Is this why you returned 1 from mv_ehci_parse_dt? So you only do this if you
  don't have a dt node?
 
  If so, why not rip the check (and positive error code) out of 
  mv_ehci_parse_dt,
  and then here:
 
  if (pdev-dev.of_node) {
  retval = mv_ehci_parse_dt(pdev, ehci_mv);
  } else
  fall back to mv_usb_platform_data ...
  }
 
  That makes it obvious that one side depends on dt and the other's a 
  fallback,
  and doesn't rely on nonstandard return code behaviour.
 
 
 I will change it.
 
  Also, why not return immediately if mv_ehci_parse_dt fails?
 
 I do not understand. if mv_ehci_parse_dt returns negative value, the
 probe will be finished immediately.

Yes, you're right. I got myself confused about the logic.

Thanks,
Mark.


--
To unsubscribe from this list: send the line unsubscribe linux-usb in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v4 1/4] drivers: usb: phy: add a new driver for usb part of control module

[Mark Rutland]

On Fri, Jan 25, 2013 at 10:23:57AM +, Kishon Vijay Abraham I wrote:
 Added a new driver for the usb part of control module. This has an API
 to power on the USB2 phy and an API to write to the mailbox depending on
 whether MUSB has to act in host mode or in device mode.
 
 Writing to control module registers for doing the above task which was
 previously done in omap glue and in omap-usb2 phy will be removed.
 
 Signed-off-by: Kishon Vijay Abraham I kis...@ti.com
 ---
  Documentation/devicetree/bindings/usb/omap-usb.txt |   30 +-
  Documentation/devicetree/bindings/usb/usb-phy.txt  |5 +
  drivers/usb/phy/Kconfig|   10 +
  drivers/usb/phy/Makefile   |1 +
  drivers/usb/phy/omap-control-usb.c |  295 
 
  include/linux/usb/omap_control_usb.h   |   92 ++
  6 files changed, 432 insertions(+), 1 deletion(-)
  create mode 100644 drivers/usb/phy/omap-control-usb.c
  create mode 100644 include/linux/usb/omap_control_usb.h
 
 diff --git a/Documentation/devicetree/bindings/usb/omap-usb.txt 
 b/Documentation/devicetree/bindings/usb/omap-usb.txt
 index 29a043e..2d8c6c4 100644
 --- a/Documentation/devicetree/bindings/usb/omap-usb.txt
 +++ b/Documentation/devicetree/bindings/usb/omap-usb.txt
 @@ -1,4 +1,4 @@
 -OMAP GLUE
 +OMAP GLUE AND OTHER OMAP SPECIFIC COMPONENTS
 
  OMAP MUSB GLUE
   - compatible : Should be ti,omap4-musb or ti,omap3-musb
 @@ -16,6 +16,10 @@ OMAP MUSB GLUE
   - power : Should be 50. This signifies the controller can supply upto
 100mA when operating in host mode.
 
 +Optional properties:
 + - ctrl-module : phandle of the control module this glue uses to write to
 +   mailbox
 +
  SOC specific device node entry
  usb_otg_hs: usb_otg_hs@4a0ab000 {
 compatible = ti,omap4-musb;
 @@ -23,6 +27,7 @@ usb_otg_hs: usb_otg_hs@4a0ab000 {
 multipoint = 1;
 num_eps = 16;
 ram_bits = 12;
 +   ctrl-module = omap_control_usb;
  };
 
  Board specific device node entry
 @@ -31,3 +36,26 @@ Board specific device node entry
 mode = 3;
 power = 50;
  };
 +
 +OMAP CONTROL USB
 +
 +Required properties:
 + - compatible: Should be ti,omap-control-usb
 + - reg : Address and length of the register set for the device. It contains
 +   the address of control_dev_conf and otghs_control or phy_power_usb

Could you not use '-' over '_' here?

 +   depending upon omap4 or omap5.
 + - reg-names: The names of the register addresses corresponding to the 
 registers
 +   filled in reg.
 + - ti,type: This is used to differentiate whether the control module has
 +   usb mailbox or usb3 phy power. omap4 has usb mailbox in control module to
 +   notify events to the musb core and omap5 has usb3 phy power register to
 +   power on usb3 phy. Should be 1 if it has mailbox and 2 if it has usb3
 +   phy power.

Why not make this a string property, perhaps values mailbox or register?

That way it's easy for humans and code to verify the dts, and easy to expand
arbitrarily in future if necessary. It also means you're not leaking
kernel-side constants as an ABI.

 +
 +omap_control_usb: omap-control-usb@4a002300 {
 +   compatible = ti,omap-control-usb;
 +   reg = 0x4a002300 0x4,
 + 0x4a00233c 0x4;
 +   reg-names = control_dev_conf, otghs_control;
 +   ti,type = 1;
 +};

[...]

 +static int omap_control_usb_probe(struct platform_device *pdev)
 +{
 +   struct resource *res;
 +   struct device_node *np = pdev-dev.of_node;
 +   struct omap_control_usb_platform_data *pdata = 
 pdev-dev.platform_data;
 +
 +   control_usb = devm_kzalloc(pdev-dev, sizeof(*control_usb),
 +   GFP_KERNEL);
 +   if (!control_usb) {
 +   dev_err(pdev-dev, unable to alloc memory for control 
 usb\n);
 +   return -ENOMEM;
 +   }
 +
 +   if (np) {
 +   of_property_read_u32(np, ti,type, control_usb-type);
 +   } else if (pdata) {
 +   control_usb-type = pdata-type;
 +   } else {
 +   dev_err(pdev-dev, no pdata present\n);
 +   return -EINVAL;
 +   }

Please do some sanity checking here on type. What if it's not
OMAP_CTRL_DEV_TYPE1 or OMAP_CTRL_DEV_TYPE2?

What if the values for OMAP_CTRL_DEV_TYPE{1,2} change? The binding will be
broken.

If you change ti,type to a string, the parsing also becomes sanity checking:

if (np) {
char *type;
if (of_property_read_string(np, ti,type, type) != 0)
return -EINVAL;

if (strcmp(type, mailbox) == 0)
control_usb-type = OMAP_CTRL_DEV_TYPE1;
else if (strcmp(type, register) == 0)
control_usb-type = OMAP_CTRL_DEV_TYPE2;
else
return -EINVAL;
} else {
... pdata case ...
}

Also, TYPE1 and TYPE2 aren't very descriptive. These could probably be better
named.

 +
 +   control_usb-dev= pdev-dev;
 +
 +   res = 

Re: [PATCH v4 1/4] drivers: usb: phy: add a new driver for usb part of control module

[Mark Rutland]

[...]

   +OMAP CONTROL USB
   +
   +Required properties:
   + - compatible: Should be ti,omap-control-usb
   + - reg : Address and length of the register set for the device. It 
   contains
   +   the address of control_dev_conf and otghs_control or 
   phy_power_usb
  
  Could you not use '-' over '_' here?
 
 that's part of omap hwmod.

Aha, that makes sense (unlike my original comment, now I reread it). Sorry for
the noise.

   +   depending upon omap4 or omap5.
   + - reg-names: The names of the register addresses corresponding to the 
   registers
   +   filled in reg.
   + - ti,type: This is used to differentiate whether the control module has
   +   usb mailbox or usb3 phy power. omap4 has usb mailbox in control 
   module to
   +   notify events to the musb core and omap5 has usb3 phy power register 
   to
   +   power on usb3 phy. Should be 1 if it has mailbox and 2 if it has 
   usb3
   +   phy power.
  
  Why not make this a string property, perhaps values mailbox or register?
 
 NAK.

Can I ask what your objection to using a string property is?

As far as I can see, ti,type is only used by this driver, so there's no
common convention to stick to. Using a string makes the binding easier for
humans to read, and thus harder to mess up in a dts, and it decouples the
binding from kernel-side constants.

Thanks,
Mark.

--
To unsubscribe from this list: send the line unsubscribe linux-usb in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v4 1/4] drivers: usb: phy: add a new driver for usb part of control module

[Mark Rutland]

On Fri, Jan 25, 2013 at 02:59:28PM +, Felipe Balbi wrote:
 Hi,
 
 On Fri, Jan 25, 2013 at 12:29:43PM +, Mark Rutland wrote:
 +   depending upon omap4 or omap5.
 + - reg-names: The names of the register addresses corresponding to 
 the registers
 +   filled in reg.
 + - ti,type: This is used to differentiate whether the control module 
 has
 +   usb mailbox or usb3 phy power. omap4 has usb mailbox in control 
 module to
 +   notify events to the musb core and omap5 has usb3 phy power 
 register to
 +   power on usb3 phy. Should be 1 if it has mailbox and 2 if it 
 has usb3
 +   phy power.

Why not make this a string property, perhaps values mailbox or 
register?
   
   NAK.
  
  Can I ask what your objection to using a string property is?
  
  As far as I can see, ti,type is only used by this driver, so there's no
  common convention to stick to. Using a string makes the binding easier for
  humans to read, and thus harder to mess up in a dts, and it decouples the
  binding from kernel-side constants.
 
 IIRC there is some work going on to add #define-like support for DT,
 which would allow us to match against integers while still having
 meaningful symbolic representations.

I was under the impression that the motivation for using the preprocessor on
the DT was to allow symbolic names for device/soc-specific values like
addresses, rather than what amounts to ABI values for the binding.

I don't see the point in building a binding that depends on future
functionality to be legible, especially as we can make it more readable,
robust, and just as extensible today, with a simple change to the proposed
binding.

Even ignoring the above, the driver isn't doing appropriate sanity checking.
If you use a string property, this sanity check is implicit in the parsing --
you've either matched a value you can handle or you haven't.

Thanks,
Mark.

--
To unsubscribe from this list: send the line unsubscribe linux-usb in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v4 1/4] drivers: usb: phy: add a new driver for usb part of control module

[Mark Rutland]

On Fri, Jan 25, 2013 at 04:23:46PM +, Felipe Balbi wrote:
 Hi,
 
 On Fri, Jan 25, 2013 at 04:14:02PM +, Mark Rutland wrote:
  On Fri, Jan 25, 2013 at 02:59:28PM +, Felipe Balbi wrote:
   Hi,
   
   On Fri, Jan 25, 2013 at 12:29:43PM +, Mark Rutland wrote:
   +   depending upon omap4 or omap5.
   + - reg-names: The names of the register addresses corresponding 
   to the registers
   +   filled in reg.
   + - ti,type: This is used to differentiate whether the control 
   module has
   +   usb mailbox or usb3 phy power. omap4 has usb mailbox in 
   control module to
   +   notify events to the musb core and omap5 has usb3 phy power 
   register to
   +   power on usb3 phy. Should be 1 if it has mailbox and 2 if 
   it has usb3
   +   phy power.
  
  Why not make this a string property, perhaps values mailbox or 
  register?
 
 NAK.

Can I ask what your objection to using a string property is?

As far as I can see, ti,type is only used by this driver, so there's 
no
common convention to stick to. Using a string makes the binding easier 
for
humans to read, and thus harder to mess up in a dts, and it decouples 
the
binding from kernel-side constants.
   
   IIRC there is some work going on to add #define-like support for DT,
   which would allow us to match against integers while still having
   meaningful symbolic representations.
  
  I was under the impression that the motivation for using the preprocessor on
  the DT was to allow symbolic names for device/soc-specific values like
  addresses, rather than what amounts to ABI values for the binding.
  
  I don't see the point in building a binding that depends on future
  functionality to be legible, especially as we can make it more readable,
  robust, and just as extensible today, with a simple change to the proposed
  binding.
  
  Even ignoring the above, the driver isn't doing appropriate sanity checking.
  If you use a string property, this sanity check is implicit in the parsing 
  --
  you've either matched a value you can handle or you haven't.
 
 Even IRQs use numbers (not talking about the IRQ number, but the IRQ
 flags), why would we depend on string comparisson ? It doesn't make
 sense.

When describing interrupts, the flags are associated with the interrupt number,
and don't really make sense on their own. devicetree does not allow you to mix
strings and integers in a value, so you'd never be able to encode irq flags
with strings without completely breaking away from the way ePAPR describes how
to encode interrupts.

By using a string property, the binding is self-describing and easy for humans
to read and verify. It doesn't add a large overhead to either the driver or the
dts, and is no worse (possibly better) for future extension of bindings to
support new device variants while retaining backwards compatibility.

See the standard status property, which is string encoded. This would still
work were it an integer-encoded enumeration. Having it as a string makes the
meaning clear to a reader of the dts, and it's trivial for code to deal with.

I don't understand why you think encoding a more legible value in the dt does
not make sense.

Thanks,
Mark.

--
To unsubscribe from this list: send the line unsubscribe linux-usb in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [V5 PATCH 24/26] usb: gadget: mv_udc: add device tree support

[Mark Rutland]

Hello,

On Thu, Jan 24, 2013 at 06:38:48AM +, Chao Xie wrote:
 In original driver, we have callbacks in platform data, and some
 dynamically allocations variables in platform data.
 Now, these blocks are removed, the device tree support is easier
 now.

Please could you also add a binding document? See
Documentation/devicetree/bindings/usb for examples of existing bindings.

Also, please Cc devicetree-discuss when introducing a new binding as you are
doing here.

 
 Signed-off-by: Chao Xie chao@marvell.com
 ---
  drivers/usb/gadget/mv_udc.h  |5 +-
  drivers/usb/gadget/mv_udc_core.c |  106 
 ++
  2 files changed, 86 insertions(+), 25 deletions(-)
 
 diff --git a/drivers/usb/gadget/mv_udc.h b/drivers/usb/gadget/mv_udc.h
 index 50ae7c7..de84722 100644
 --- a/drivers/usb/gadget/mv_udc.h
 +++ b/drivers/usb/gadget/mv_udc.h
 @@ -179,6 +179,7 @@ struct mv_udc {
   int irq;
  
   unsigned intextern_attr;
 + unsigned intmode;
   struct notifier_block   notifier;
  
   struct mv_cap_regs __iomem  *cap_regs;
 @@ -222,11 +223,9 @@ struct mv_udc {
   struct mv_usb2_phy  *phy;
   struct usb_phy  *transceiver;
  
 - struct mv_usb_platform_data *pdata;
 -
   /* some SOC has mutiple clock sources for USB*/
   unsigned intclknum;
 - struct clk  *clk[0];
 + struct clk  **clk;
  };
  
  /* endpoint data structure */
 diff --git a/drivers/usb/gadget/mv_udc_core.c 
 b/drivers/usb/gadget/mv_udc_core.c
 index 2e5907f..a4ee9a1 100644
 --- a/drivers/usb/gadget/mv_udc_core.c
 +++ b/drivers/usb/gadget/mv_udc_core.c
 @@ -34,6 +34,7 @@
  #include linux/irq.h
  #include linux/platform_device.h
  #include linux/clk.h
 +#include linux/of.h
  #include linux/platform_data/mv_usb.h
  #include linux/usb/mv_usb2.h
  #include asm/unaligned.h
 @@ -2153,21 +2154,57 @@ static int mv_udc_remove(struct platform_device *pdev)
   return 0;
  }
  
 +static int mv_udc_parse_dt(struct platform_device *pdev,
 + struct mv_udc *udc)
 +{
 + struct device_node *np = pdev-dev.of_node;
 + unsigned int clks_num;
 + int i, ret;
 + const char *clk_name;
 +
 + if (!np)
 + return 1;
 +
 + clks_num = of_property_count_strings(np, clocks);
 + if (clks_num  0)
 + return clks_num;
 +
 + udc-clk = devm_kzalloc(pdev-dev,
 + sizeof(struct clk *) * clks_num, GFP_KERNEL);
 + if (udc-clk == NULL)
 + return -ENOMEM;
 +
 + for (i = 0; i  clks_num; i++) {
 + ret = of_property_read_string_index(np, clocks, i,
 + clk_name);
 + if (ret)
 + return ret;
 + udc-clk[i] = devm_clk_get(pdev-dev, clk_name);
 + if (IS_ERR(udc-clk[i]))
 + return PTR_ERR(udc-clk[i]);

I was going to ask if you couldn't use of_clk_get, but I see you want to use
the devm_* functions to handle cleanup. It seems a shame there's not a standard
devm_of_clk_get.

 + }
 +
 + udc-clknum = clks_num;
 +
 + ret = of_property_read_u32(np, extern_attr, udc-extern_attr);
 + if (ret)
 + return ret;

This looks like a *very* bad idea. The udc::extern_attr field seems to be a set
of flags, which this could reads in directly (without any sanity checking),
effectively making it an undocumented ABI. This might be better as a set of
valueless properties.

Will unsigned int be the same as u32 on all platforms this could be used on?
If not, you're passing the wrong type into of_property_read_u32.

Additionally, in devicetree, '-' is used in preference of '_' in compatible
and property names.

 +
 + ret = of_property_read_u32(np, mode, udc-mode);
 + if (ret)
 + return ret;

If I've understood correctly, this property will either be MV_USB_MODE_OTG (0)
or MV_USB_MODE_OTG (1). Again, I don't think this is good to be exported as an
ABI, especially as nothing in the enum definition points out that this affects
anything outside the kernel.

If this isn't something that wants to be changed at runtime, this should
probably be a string property, with host and otg as valid values. Looking
in Documentation/devicetree, the nvidia,tegra20-ehci binding uses similar
strings in its dr_mode property. There may be other (undocumented) precedents.

 +
 + return 0;
 +}
 +
  static int mv_udc_probe(struct platform_device *pdev)
  {
 - struct mv_usb_platform_data *pdata = pdev-dev.platform_data;
   struct mv_udc *udc;
   int retval = 0;
 - int clk_i = 0;
   struct resource *r;
   size_t size;
  
 - if (pdata == NULL) {
 - dev_err(pdev-dev, missing platform_data\n);
 - return -ENODEV;
 - }
 -
 - size = sizeof(*udc) + sizeof(struct clk *) * pdata-clknum;
 + size = sizeof(*udc);
   udc = devm_kzalloc(pdev-dev, 

Re: [V5 PATCH 26/26] usb: ehci: ehci-mv: add device tree support

[Mark Rutland]

On Thu, Jan 24, 2013 at 06:38:50AM +, Chao Xie wrote:
 All blocks are removed. Add the device tree support for ehci.

Similarly to the last two patches, could you please add a binding document?

 Signed-off-by: Chao Xie chao@marvell.com
 Acked-by: Alan Stern st...@rowland.harvard.edu
 ---
  drivers/usb/host/ehci-mv.c |  105 +--
  1 files changed, 80 insertions(+), 25 deletions(-)
 
 diff --git a/drivers/usb/host/ehci-mv.c b/drivers/usb/host/ehci-mv.c
 index 171e145..79eca0c 100644
 --- a/drivers/usb/host/ehci-mv.c
 +++ b/drivers/usb/host/ehci-mv.c
 @@ -13,6 +13,7 @@
  #include linux/module.h
  #include linux/platform_device.h
  #include linux/clk.h
 +#include linux/of.h
  #include linux/err.h
  #include linux/usb/otg.h
  #include linux/usb/mv_usb2.h
 @@ -32,11 +33,9 @@ struct ehci_hcd_mv {
  
   struct usb_phy *otg;
  
 - struct mv_usb_platform_data *pdata;
 -
   /* clock source and total clock number */
   unsigned int clknum;
 - struct clk *clk[0];
 + struct clk **clk;
  };
  
  static void ehci_clock_enable(struct ehci_hcd_mv *ehci_mv)
 @@ -138,22 +137,55 @@ static const struct hc_driver mv_ehci_hc_driver = {
   .bus_resume = ehci_bus_resume,
  };
  
 +static int mv_ehci_parse_dt(struct platform_device *pdev,
 + struct ehci_hcd_mv *ehci_mv)
 +{
 + struct device_node *np = pdev-dev.of_node;
 + unsigned int clks_num;
 + int i, ret;
 + const char *clk_name;
 +
 + if (!np)
 + return 1;

Perhaps -ENODEV?

 +
 + clks_num = of_property_count_strings(np, clocks);
 + if (clks_num  0)
 + return clks_num;
 +
 + ehci_mv-clk = devm_kzalloc(pdev-dev,
 + sizeof(struct clk *) * clks_num, GFP_KERNEL);
 + if (ehci_mv-clk == NULL)
 + return -ENOMEM;
 +
 + for (i = 0; i  clks_num; i++) {
 + ret = of_property_read_string_index(np, clocks, i,
 + clk_name);
 + if (ret)
 + return ret;
 + ehci_mv-clk[i] = clk_get(NULL, clk_name);
 + if (IS_ERR(ehci_mv-clk[i]))
 + return PTR_ERR(ehci_mv-clk[i]);
 + }
 +
 + ehci_mv-clknum = clks_num;
 +
 + ret = of_property_read_u32(np, mode, ehci_mv-mode);
 + if (ret)
 + return ret;

Again, this is a bad idea.

 +
 + return 0;
 +}
 +
  static int mv_ehci_probe(struct platform_device *pdev)
  {
 - struct mv_usb_platform_data *pdata = pdev-dev.platform_data;
   struct usb_hcd *hcd;
   struct ehci_hcd *ehci;
   struct ehci_hcd_mv *ehci_mv;
   struct resource *r;
 - int clk_i, retval = -ENODEV;
 + int retval = -ENODEV;
   u32 offset;
   size_t size;
  
 - if (!pdata) {
 - dev_err(pdev-dev, missing platform_data\n);
 - return -ENODEV;
 - }
 -
   if (usb_disabled())
   return -ENODEV;
  
 @@ -161,7 +193,7 @@ static int mv_ehci_probe(struct platform_device *pdev)
   if (!hcd)
   return -ENOMEM;
  
 - size = sizeof(*ehci_mv) + sizeof(struct clk *) * pdata-clknum;
 + size = sizeof(*ehci_mv);
   ehci_mv = devm_kzalloc(pdev-dev, size, GFP_KERNEL);
   if (ehci_mv == NULL) {
   dev_err(pdev-dev, cannot allocate ehci_hcd_mv\n);
 @@ -170,19 +202,36 @@ static int mv_ehci_probe(struct platform_device *pdev)
   }
  
   platform_set_drvdata(pdev, ehci_mv);
 - ehci_mv-pdata = pdata;
   ehci_mv-hcd = hcd;
  
 - ehci_mv-clknum = pdata-clknum;
 - for (clk_i = 0; clk_i  ehci_mv-clknum; clk_i++) {
 - ehci_mv-clk[clk_i] =
 - devm_clk_get(pdev-dev, pdata-clkname[clk_i]);
 - if (IS_ERR(ehci_mv-clk[clk_i])) {
 - dev_err(pdev-dev, error get clck \%s\\n,
 - pdata-clkname[clk_i]);
 - retval = PTR_ERR(ehci_mv-clk[clk_i]);
 - goto err_clear_drvdata;
 + retval = mv_ehci_parse_dt(pdev, ehci_mv);
 + if (retval  0) {

Is this why you returned 1 from mv_ehci_parse_dt? So you only do this if you
don't have a dt node?

If so, why not rip the check (and positive error code) out of mv_ehci_parse_dt,
and then here:

if (pdev-dev.of_node) {
retval = mv_ehci_parse_dt(pdev, ehci_mv);
} else 
fall back to mv_usb_platform_data ...
}

That makes it obvious that one side depends on dt and the other's a fallback,
and doesn't rely on nonstandard return code behaviour.

Also, why not return immediately if mv_ehci_parse_dt fails?

[...]

Thanks,
Mark.

--
To unsubscribe from this list: send the line unsubscribe linux-usb in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v1 5/6] usb: otg: add device tree support to otg library

[Mark Rutland]

 +struct usb_phy *devm_usb_get_phy_by_phandle(struct device *dev,
 + const char *phandle, u8 index)
 +{
 + struct usb_phy  *phy = NULL, **ptr;
 + unsigned long   flags;
 + struct device_node *node;
 +
 + if (!dev-of_node) {
 + dev_dbg(dev, device does not have a device node entry\n);
 + return ERR_PTR(-EINVAL);
 + }
 +
 + node = of_parse_phandle(dev-of_node, phandle, index);
 + if (!node) {
 + dev_dbg(dev, failed to get %s phandle in %s node\n, phandle,
 + dev-of_node-full_name);
 + return ERR_PTR(-ENODEV);
 + }

At any point past this, node's refcount is incremented (done automatically by
of_parse_phandle = of_find_node_by_phandle).

 +
 + ptr = devres_alloc(devm_usb_phy_release, sizeof(*ptr), GFP_KERNEL);
 + if (!ptr) {
 + dev_dbg(dev, failed to allocate memory for devres\n);
 + return ERR_PTR(-ENOMEM);
 + }
 +
 + spin_lock_irqsave(phy_lock, flags);
 +
 + phy = __of_usb_find_phy(node);
 + if (IS_ERR(phy) || !try_module_get(phy-dev-driver-owner)) {
 + phy = ERR_PTR(-EPROBE_DEFER);
 + devres_free(ptr);
 + goto err0;
 + }
 +
 + *ptr = phy;
 + devres_add(dev, ptr);
 +
 + get_device(phy-dev);
 +
 +err0:
 + spin_unlock_irqrestore(phy_lock, flags);
 +
 + return phy;
 +}

This means that on all of the exit paths, node's refcount is left incremented
incorrectly. You'll need an of_node_put(node) on each exit path.

Thanks,
Mark.

--
To unsubscribe from this list: send the line unsubscribe linux-usb in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html