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@xxxxxxx> 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@xxxxxxxxxxxxx>
> Signed-off-by: Bruce Schmid <duck@xxxxxxxxxxxxx>
> Signed-off-by: Anatolij Gustschin <agust@xxxxxxx>
> Cc: Greg Kroah-Hartman <gregkh@xxxxxxx>
> Cc: David Brownell <dbrownell@xxxxxxxxxxxxxxxxxxxxx>
> Cc: Grant Likely <grant.likely@xxxxxxxxxxxx>
> ---
> 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.
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
