Re: [PATCH 1/3] usb: orion-echi: Add support for the Armada 3700

[Andrew Lunn]

Hi Gregory

> - Add a new compatoble string for the Armada 3700 SoCs

compatible

> 
> - add sbuscfg support for orion usb controller driver. For the SoCs
>   without hlock, need to program BAWR/BARD/AHBBRST fields in the sbuscfg
>   register to guarantee the AHB master's burst would not overrun or
>   underrun the FIFO.
> 
> - the sbuscfg register has to be set after the usb controller reset,
>   otherwise the value would be overridden to 0. In order to do this, the
>   reset callback is registered.
> 
> [gregory.clement@xxxxxxxxxxxxxxxxxx: - reword commit and comments
> 				     - fix checkpatch warning]
> Signed-off-by: jinghua <jinghua@xxxxxxxxxxx>
> Signed-off-by: Gregory CLEMENT <gregory.clement@xxxxxxxxxxxxxxxxxx>
> ---
>  .../devicetree/bindings/usb/ehci-orion.txt         |  4 ++-
>  drivers/usb/host/ehci-orion.c                      | 39 ++++++++++++++++++++++
>  2 files changed, 42 insertions(+), 1 deletion(-)
> 
> diff --git a/Documentation/devicetree/bindings/usb/ehci-orion.txt b/Documentation/devicetree/bindings/usb/ehci-orion.txt
> index 17c3bc858b86..9dfffc9dffec 100644
> --- a/Documentation/devicetree/bindings/usb/ehci-orion.txt
> +++ b/Documentation/devicetree/bindings/usb/ehci-orion.txt
> @@ -1,7 +1,9 @@
>  * EHCI controller, Orion Marvell variants
>  
>  Required properties:
> -- compatible: must be "marvell,orion-ehci"
> +- compatible: could be one of the following

must, not could.

> +	"marvell,orion-ehci"
> +	"marvell,armada-3700-ehci"
>  - reg: physical base address of the controller and length of memory mapped
>    region.
>  - interrupts: The EHCI interrupt
> diff --git a/drivers/usb/host/ehci-orion.c b/drivers/usb/host/ehci-orion.c
> index ee8d5faa0194..cf778e166b90 100644
> --- a/drivers/usb/host/ehci-orion.c
> +++ b/drivers/usb/host/ehci-orion.c
> @@ -47,6 +47,21 @@
>  #define USB_PHY_IVREF_CTRL	0x440
>  #define USB_PHY_TST_GRP_CTRL	0x450
>  
> +#define USB_SBUSCFG		0x90
> +#define	    USB_SBUSCFG_BAWR	    0x6
> +#define	    USB_SBUSCFG_BARD	    0x3
> +#define	    USB_SBUSCFG_AHBBRST	    0x0

These three are all shifts. So i would suggest adding _SHIFT to the
end.

> +
> +/* BAWR = BARD = 3 : Align read/write bursts packets larger than 128 bytes */
> +#define USB_SBUSCFG_BAWR_ALIGN_128B	0x3
> +#define USB_SBUSCFG_BARD_ALIGN_128B	0x3
> +/* AHBBRST = 3	   : Align AHB Burst to INCR16 (64 bytes) */
> +#define USB_SBUSCFG_AHBBRST_INCR16	0x3

You can then apply the shift here.

> +
> +#define USB_SBUSCFG_DEF_VAL ((USB_SBUSCFG_BAWR_ALIGN_128B << USB_SBUSCFG_BAWR) \
> +		     | (USB_SBUSCFG_BARD_ALIGN_128B << USB_SBUSCFG_BARD) \
> +		     | (USB_SBUSCFG_AHBBRST_INCR16 << USB_SBUSCFG_AHBBRST))

and this is then shorted.

    Andrew