[PATCH 3/4] USB: add USB OTG support for MPC5121 SoC

[Grant Likely]

Hi Anatolij,

I haven't looked deeply, but I've written a couple of minor comments below.

g.

On Tue, Apr 27, 2010 at 10:11 AM, Anatolij Gustschin <agust at denx.de> wrote:
> Adds Freescale USB OTG driver and extends Freescale
> USB SoC Device Controller driver and FSL EHCI driver
> to support OTG operation on MPC5121.
>
> Signed-off-by: Li Yang <leoli at freescale.com>
> Signed-off-by: Bruce Schmid <duck at freescale.com>
> Signed-off-by: Anatolij Gustschin <agust at denx.de>
> Cc: Greg Kroah-Hartman <gregkh at suse.de>
> Cc: David Brownell <dbrownell at users.sourceforge.net>
> Cc: Grant Likely <grant.likely at secretlab.ca>
> ---
>  arch/powerpc/include/asm/fsl_usb_io.h |  106 +++
>  drivers/usb/gadget/fsl_udc_core.c     |  357 ++++++++--
>  drivers/usb/gadget/fsl_usb2_udc.h     |   14 +
>  drivers/usb/host/ehci-fsl.c           |  261 +++++++-
>  drivers/usb/host/ehci-fsl.h           |    3 +
>  drivers/usb/host/ehci-hub.c           |   39 ++
>  drivers/usb/host/ehci.h               |    5 +
>  drivers/usb/otg/Kconfig               |    8 +
>  drivers/usb/otg/Makefile              |    2 +
>  drivers/usb/otg/fsl_otg.c             | 1199 +++++++++++++++++++++++++++++++++
>  drivers/usb/otg/fsl_otg.h             |  418 ++++++++++++
>  drivers/usb/otg/otg.c                 |   17 +
>  drivers/usb/otg/otg_fsm.c             |  368 ++++++++++
>  drivers/usb/otg/otg_fsm.h             |  154 +++++
>  include/linux/fsl_devices.h           |   15 +
>  15 files changed, 2867 insertions(+), 99 deletions(-)
>  create mode 100644 arch/powerpc/include/asm/fsl_usb_io.h
>  create mode 100644 drivers/usb/otg/fsl_otg.c
>  create mode 100644 drivers/usb/otg/fsl_otg.h
>  create mode 100644 drivers/usb/otg/otg_fsm.c
>  create mode 100644 drivers/usb/otg/otg_fsm.h
>
> diff --git a/arch/powerpc/include/asm/fsl_usb_io.h b/arch/powerpc/include/asm/fsl_usb_io.h
> new file mode 100644
> index 0000000..20c42ef
> --- /dev/null
> +++ b/arch/powerpc/include/asm/fsl_usb_io.h
> @@ -0,0 +1,106 @@
> +/* Copyright (c) 2008 Freescale Semiconductor Inc.
> + *
> + * This program is free software; you can redistribute  it and/or modify it
> + * under  the terms of  the GNU General  Public License as published by the
> + * Free Software Foundation;  either version 2 of the  License, or (at your
> + * option) any later version.
> + *
> + * This program is distributed in the hope that it will be useful, but
> + * WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> + * General Public License for more details.
> + *
> + * You should have received a copy of the  GNU General Public License along
> + * with this program; if not, write  to the Free Software Foundation, Inc.,
> + * 675 Mass Ave, Cambridge, MA 02139, USA.
> + */
> +#ifndef _FSL_USB_IO_H
> +#define _FSL_USB_IO_H
> +
> +/*
> + * On some SoCs, the USB controller registers can be big or little endian,
> + * depending on the version of the chip.  For these SoCs, the kernel
> + * should be configured with CONFIG_USB_FSL_BIG_ENDIAN_MMIO enabled.
> + *
> + * Platform code for SoCs that have BE USB registers should set
> + * pdata->big_endian_mmio flag.
> + *
> + * In order to be able to run the same kernel binary on 2 different
> + * versions of an SoC, the BE/LE decision must be made at run time.
> + * _fsl_readl and fsl_writel are pointers to the BE or LE readl()
> + * and writel() functions, and fsl_readl() and fsl_writel() call through
> + * those pointers.
> + *
> + * For SoCs with the usual LE USB registers, don't enable
> + * CONFIG_USB_FSL_BIG_ENDIAN_MMIO, and then fsl_readl() and fsl_writel()
> + * are just macro wrappers for in_le32() and out_le32().
> + *
> + * In either (LE or mixed) case, the function fsl_set_usb_accessors()
> + * should be called at probe time, to either set up the readl/writel
> + * function pointers (mixed case), or do nothing (LE case).
> + *
> + * The host USB drivers already have a mechanism to handle BE/LE
> + * registers.  The functionality here is intended to be used by the
> + * gadget and OTG transceiver drivers.
> + *
> + * This file also contains controller-to-cpu accessors for the
> + * USB descriptors, since their endianess is also SoC dependant.
> + * The kernel option CONFIG_USB_FSL_BIG_ENDIAN_DESC configures
> + * which way to go.
> + */
> +
> +#ifdef CONFIG_USB_FSL_BIG_ENDIAN_MMIO
> +static u32 __maybe_unused _fsl_readl_be(const unsigned __iomem *p)
> +{
> +       return in_be32(p);
> +}
> +static u32 __maybe_unused _fsl_readl_le(const unsigned __iomem *p)
> +{
> +       return in_le32(p);
> +}
> +
> +static void __maybe_unused _fsl_writel_be(u32 v, unsigned __iomem *p)
> +{
> +       out_be32(p, v);
> +}
> +static void __maybe_unused _fsl_writel_le(u32 v, unsigned __iomem *p)
> +{
> +       out_le32(p, v);
> +}
> +
> +static u32 (*_fsl_readl)(const unsigned __iomem *p);
> +static void (*_fsl_writel)(u32 v, unsigned __iomem *p);

statics defined in a header file?  That doesn't look right.  These
dynamic accessors probably need to be defined in a .c file and linked
to the rest of the driver code.

It is also better practise to put ops like these into the drivers
private instance data structure, but I understand that there is a
large driver impact associated with making that change.

> +
> +#define fsl_readl(p)           (*_fsl_readl)((p))
> +#define fsl_writel(v, p)       (*_fsl_writel)((v), (p))
> +
> +static inline void fsl_set_usb_accessors(struct fsl_usb2_platform_data *pdata)
> +{
> +       if (pdata->big_endian_mmio) {
> +               _fsl_readl = _fsl_readl_be;
> +               _fsl_writel = _fsl_writel_be;
> +       } else {
> +               _fsl_readl = _fsl_readl_le;
> +               _fsl_writel = _fsl_writel_le;
> +       }
> +}
> +
> +#else /* CONFIG_USB_FSL_BIG_ENDIAN_MMIO */
> +
> +#define fsl_readl(addr)                in_le32((addr))
> +#define fsl_writel(val32, addr) out_le32((addr), (val32))
> +
> +static inline void fsl_set_usb_accessors(struct fsl_usb2_platform_data *pdata)
> +{
> +}
> +#endif /* CONFIG_USB_FSL_BIG_ENDIAN_MMIO */
> +
> +#ifdef CONFIG_USB_FSL_BIG_ENDIAN_DESC
> +#define cpu_to_hc32(x) (x)
> +#define hc32_to_cpu(x) (x)
> +#else
> +#define cpu_to_hc32(x) cpu_to_le32((x))
> +#define hc32_to_cpu(x) le32_to_cpu((x))
> +#endif
> +
> +#endif /* _FSL_USB_IO_H */
> diff --git a/drivers/usb/gadget/fsl_udc_core.c b/drivers/usb/gadget/fsl_udc_core.c
> index fa3d142..b5361a8 100644
> --- a/drivers/usb/gadget/fsl_udc_core.c
> +++ b/drivers/usb/gadget/fsl_udc_core.c
> @@ -45,6 +45,7 @@
>  #include <asm/system.h>
>  #include <asm/unaligned.h>
>  #include <asm/dma.h>
> +#include <asm/cacheflush.h>
>
>  #include "fsl_usb2_udc.h"
>
> @@ -76,10 +77,7 @@ fsl_ep0_desc = {
>
>  static void fsl_ep_fifo_flush(struct usb_ep *_ep);
>
> -#ifdef CONFIG_PPC32
> -#define fsl_readl(addr)                in_le32(addr)
> -#define fsl_writel(val32, addr) out_le32(addr, val32)
> -#else
> +#ifndef CONFIG_PPC32
>  #define fsl_readl(addr)                readl(addr)
>  #define fsl_writel(val32, addr) writel(val32, addr)
>  #endif

I would think these !CONFIG_PPC32 defines should also move into the
header file just so everything is defined in the same place.

Cheers,
g.