Re: [PATCH v2 1/2] usb: samsung: Update Exynos EHCI/OHCI bindings documentation

[Mark Rutland <mark.rutland@arm.com>]

Hi,

On Thu, Aug 29, 2013 at 12:33:25PM +0100, Sachin Kamat wrote:
> Updated the document as per the latest implementation.
> While at it also fixed some trivial typos.
> 
> Signed-off-by: Sachin Kamat <sachin.kamat@linaro.org>
> ---
> Changes since v1:
> * Updated review comments from Stephen Warren regarding the wording
> and some styling.
> ---
>  .../devicetree/bindings/usb/exynos-usb.txt         |   29 ++++++++++----------
>  1 file changed, 15 insertions(+), 14 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/usb/exynos-usb.txt b/Documentation/devicetree/bindings/usb/exynos-usb.txt
> index d967ba1..56468f7 100644
> --- a/Documentation/devicetree/bindings/usb/exynos-usb.txt
> +++ b/Documentation/devicetree/bindings/usb/exynos-usb.txt
> @@ -5,13 +5,14 @@ The device node has following properties.
>  
>  EHCI
>  Required properties:
> - - compatible: should be "samsung,exynos4210-ehci" for USB 2.0
> -   EHCI controller in host mode.
> + - compatible: should be one of the following for USB 2.0 EHCI controller:
> +	(a) "samsung,exynos5440-ehci" for Exynos5440 SoC
> +	(b) "samsung,exynos4210-ehci" for all other Exynos4 and 5 SoCs
>   - reg: physical base address of the controller and length of memory mapped
>     region.
> - - interrupts: interrupt number to the cpu.
> - - clocks: from common clock binding: handle to usb clock.
> - - clock-names: from common clock binding: Shall be "usbhost".
> + - interrupts: interrupt number to the CPU.

If this is going to be reorganised, can we fix the terminology?
Interrupts are defined by interrupt-specifiers:

 - interrupts: a single interrupt-specifier for the sole interrupt
               generated by the device.

> + - clocks: from common clock binding: handle to USB clock.
> + - clock-names: shall be "usbhost".

Similarly:

 - clocks: phandle amd clock-specifier pair for the clocks listed in
   clock-names.
 - clock-names: shall contain "usbhost" for the USB clock.

>  
>  Optional properties:
>   - samsung,vbus-gpio:  if present, specifies the GPIO that
> @@ -23,7 +24,7 @@ Example:
>  		compatible = "samsung,exynos4210-ehci";
>  		reg = <0x12110000 0x100>;
>  		interrupts = <0 71 0>;
> -		samsung,vbus-gpio = <&gpx2 6 1 3 3>;
> +		samsung,vbus-gpio = <&gpx2 6 0>;

Why does the gpio in the example need to be changed in this way?

>  
>  		clocks = <&clock 285>;
>  		clock-names = "usbhost";
> @@ -31,13 +32,14 @@ Example:
>  
>  OHCI
>  Required properties:
> - - compatible: should be "samsung,exynos4210-ohci" for USB 2.0
> -   OHCI companion controller in host mode.
> + - compatible: should be one of the following for USB 2.0 OHCI controller:
> +	(a) "samsung,exynos5440-ohci" for Exynos5440 SoC
> +	(b) "samsung,exynos4210-ohci" for all other Exynos4 and 5 SoCs
>   - reg: physical base address of the controller and length of memory mapped
>     region.
> - - interrupts: interrupt number to the cpu.
> - - clocks: from common clock binding: handle to usb clock.
> - - clock-names: from common clock binding: Shall be "usbhost".
> + - interrupts: interrupt number to the CPU.
> + - clocks: from common clock binding: handle to USB clock.
> + - clock-names: shall be "usbhost".

Similarly it would be nice to fix up the terminology here.

>  
>  Example:
>  	usb@12120000 {
> @@ -53,12 +55,11 @@ DWC3
>  Required properties:
>   - compatible: should be "samsung,exynos5250-dwusb3" for USB 3.0 DWC3
>  	       controller.
> - - #address-cells, #size-cells : should be '1' if the device has sub-nodes
> -				 with 'reg' property.
> + - #address-cells, #size-cells: should be '1'.
>   - ranges: allows valid 1:1 translation between child's address space and
>  	   parent's address space

Huh? What if I'm on an LPAE system. I can't necessarily have both an
empty ranges property (for 1:1 translation) and #address-cells = <1>,
#size-cells = <1>. That's just broken.

Is the driver relying on this, or does it just require some valid
ranges, #address-cells, and #size-cells that the Linux infrastructure
can already handle?

As far as I an see, this should be something like:

- #address-cells: as required to describe any child nodes.

- #size-cells: as required to describe any child nodes.

- ranges: as required to map any child nodes to the parent address space.

>   - clocks: Clock IDs array as required by the controller.
> - - clock-names: names of clocks correseponding to IDs in the clock property
> + - clock-names: shall be "usbdrd30".

It would be nice to fix up the terminology here too.

Thanks,
Mark.