Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
Message-ID: <1547606593.4433.178.camel@mhfsdcap03>
Subject: Re: [PATCH v2 4/4] usb: musb: Add support for MediaTek musb
 controller
From:   Min Guo <min.guo@mediatek.com>
To:     Bin Liu <b-liu@ti.com>
CC:     Rob Herring <robh+dt@kernel.org>,
        Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
        Mark Rutland <mark.rutland@arm.com>,
        "Matthias Brugger" <matthias.bgg@gmail.com>,
        Alan Stern <stern@rowland.harvard.edu>,
        <chunfeng.yun@mediatek.com>, <linux-usb@vger.kernel.org>,
        <devicetree@vger.kernel.org>, <linux-kernel@vger.kernel.org>,
        <linux-arm-kernel@lists.infradead.org>,
        <linux-mediatek@lists.infradead.org>,
        Yonglong Wu <yonglong.wu@mediatek.com>
Date:   Wed, 16 Jan 2019 10:43:13 +0800
In-Reply-To: <20190115203815.GD18026@uda0271908>
References: <1547516626-5084-1-git-send-email-min.guo@mediatek.com>
         <1547516626-5084-5-git-send-email-min.guo@mediatek.com>
         <20190115203815.GD18026@uda0271908>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: 7bit
MIME-Version: 1.0
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

Hi Bin,

On Tue, 2019-01-15 at 14:38 -0600, Bin Liu wrote:
> Hi Min,
> 
> very close, thanks.
> Below I tried to explain a further cleanup in musb_clearb/w() and
> musb_get/set_toggle() implementation. Please let me know if it is not
> clear.
> 
> Basically, we don't need musb_default_clearb/w(), just assign the
> musb_io function pointers to musb_readb/w().
> 
> Then the mtk platform musb_clearb/w() calls musb_readb/w() and
> musb_writeb/w() to handle W1C.

Okay.

> On Tue, Jan 15, 2019 at 09:43:46AM +0800, min.guo@mediatek.com wrote:
> > From: Min Guo <min.guo@mediatek.com>
> > 
> > This adds support for MediaTek musb controller in
> > host, peripheral and otg mode.
> > There are some quirk of MediaTek musb controller, such as:
> >  -W1C interrupt status registers
> >  -Private data toggle registers
> >  -No dedicated DMA interrupt line
> > 
> > Signed-off-by: Min Guo <min.guo@mediatek.com>
> > Signed-off-by: Yonglong Wu <yonglong.wu@mediatek.com>
> > ---
> >  drivers/usb/musb/Kconfig     |   8 +-
> >  drivers/usb/musb/Makefile    |   1 +
> >  drivers/usb/musb/mediatek.c  | 617 +++++++++++++++++++++++++++++++++++++++++++
> >  drivers/usb/musb/musb_core.c |  69 +++++
> >  drivers/usb/musb/musb_core.h |   9 +
> >  drivers/usb/musb/musb_dma.h  |   9 +
> >  drivers/usb/musb/musb_host.c |  26 +-
> >  drivers/usb/musb/musb_io.h   |   6 +
> >  drivers/usb/musb/musbhsdma.c |  55 ++--
> >  9 files changed, 762 insertions(+), 38 deletions(-)
> >  create mode 100644 drivers/usb/musb/mediatek.c
> > 
> > diff --git a/drivers/usb/musb/Kconfig b/drivers/usb/musb/Kconfig
> > index ad08895..b72b7c1 100644
> > --- a/drivers/usb/musb/Kconfig
> > +++ b/drivers/usb/musb/Kconfig
> > @@ -115,6 +115,12 @@ config USB_MUSB_JZ4740
> >  	depends on USB_MUSB_GADGET
> >  	depends on USB_OTG_BLACKLIST_HUB
> >  
> > +config USB_MUSB_MEDIATEK
> > +	tristate "MediaTek platforms"
> > +	depends on ARCH_MEDIATEK || COMPILE_TEST
> > +	depends on NOP_USB_XCEIV
> > +	depends on GENERIC_PHY
> > +
> >  config USB_MUSB_AM335X_CHILD
> >  	tristate
> >  
> > @@ -141,7 +147,7 @@ config USB_UX500_DMA
> >  
> >  config USB_INVENTRA_DMA
> >  	bool 'Inventra'
> > -	depends on USB_MUSB_OMAP2PLUS
> > +	depends on USB_MUSB_OMAP2PLUS || USB_MUSB_MEDIATEK
> >  	help
> >  	  Enable DMA transfers using Mentor's engine.
> >  
> > diff --git a/drivers/usb/musb/Makefile b/drivers/usb/musb/Makefile
> > index 3a88c79..63d82d0 100644
> > --- a/drivers/usb/musb/Makefile
> > +++ b/drivers/usb/musb/Makefile
> > @@ -24,6 +24,7 @@ obj-$(CONFIG_USB_MUSB_DA8XX)			+= da8xx.o
> >  obj-$(CONFIG_USB_MUSB_UX500)			+= ux500.o
> >  obj-$(CONFIG_USB_MUSB_JZ4740)			+= jz4740.o
> >  obj-$(CONFIG_USB_MUSB_SUNXI)			+= sunxi.o
> > +obj-$(CONFIG_USB_MUSB_MEDIATEK)      		+= mediatek.o
> >  
> >  
> >  obj-$(CONFIG_USB_MUSB_AM335X_CHILD)		+= musb_am335x.o
> > diff --git a/drivers/usb/musb/mediatek.c b/drivers/usb/musb/mediatek.c
> > new file mode 100644
> > index 0000000..7221989
> > --- /dev/null
> > +++ b/drivers/usb/musb/mediatek.c
> > @@ -0,0 +1,617 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/*
> > + * Copyright (C) 2018 MediaTek Inc.
> > + *
> > + * Author:
> > + *  Min Guo <min.guo@mediatek.com>
> > + *  Yonglong Wu <yonglong.wu@mediatek.com>
> > + */
> > +
> > +#include <linux/clk.h>
> > +#include <linux/dma-mapping.h>
> > +#include <linux/module.h>
> > +#include <linux/platform_device.h>
> > +#include <linux/usb/usb_phy_generic.h>
> > +#include "musb_core.h"
> > +#include "musb_dma.h"
> > +
> > +#define USB_L1INTS	0x00a0
> > +#define USB_L1INTM	0x00a4
> > +#define MTK_MUSB_TXFUNCADDR	0x0480
> > +
> > +/* MediaTek controller toggle enable and status reg */
> > +#define MUSB_RXTOG		0x80
> > +#define MUSB_RXTOGEN		0x82
> > +#define MUSB_TXTOG		0x84
> > +#define MUSB_TXTOGEN		0x86
> > +
> > +#define TX_INT_STATUS		BIT(0)
> > +#define RX_INT_STATUS		BIT(1)
> > +#define USBCOM_INT_STATUS	BIT(2)
> 
> missing a TAB for the alignment?

Okay.

> > +#define DMA_INT_STATUS		BIT(3)
> > +
> > +#define DMA_INTR_STATUS_MSK		GENMASK(7, 0)
> > +#define DMA_INTR_UNMASK_SET_MSK	GENMASK(31, 24)
> > +
> > +enum mtk_vbus_id_state {
> > +	MTK_ID_FLOAT = 1,
> > +	MTK_ID_GROUND,
> > +	MTK_VBUS_OFF,
> > +	MTK_VBUS_VALID,
> > +};
> > +
> > +struct mtk_glue {
> > +	struct device *dev;
> > +	struct musb *musb;
> > +	struct platform_device *musb_pdev;
> > +	struct platform_device *usb_phy;
> > +	struct phy *phy;
> > +	struct usb_phy *xceiv;
> > +	enum phy_mode phy_mode;
> > +	struct clk *main;
> > +	struct clk *mcu;
> > +	struct clk *univpll;
> > +	struct regulator *vbus;
> > +	struct extcon_dev *edev;
> > +	struct notifier_block vbus_nb;
> > +	struct notifier_block id_nb;
> > +};
> > +
> > +static int mtk_musb_clks_get(struct mtk_glue *glue)
> > +{
> > +	struct device *dev = glue->dev;
> > +
> > +	glue->main = devm_clk_get(dev, "main");
> > +	if (IS_ERR(glue->main)) {
> > +		dev_err(dev, "fail to get main clock\n");
> > +		return PTR_ERR(glue->main);
> > +	}
> > +
> > +	glue->mcu = devm_clk_get(dev, "mcu");
> > +	if (IS_ERR(glue->mcu)) {
> > +		dev_err(dev, "fail to get mcu clock\n");
> > +		return PTR_ERR(glue->mcu);
> > +	}
> > +
> > +	glue->univpll = devm_clk_get(dev, "univpll");
> > +	if (IS_ERR(glue->univpll)) {
> > +		dev_err(dev, "fail to get univpll clock\n");
> > +		return PTR_ERR(glue->univpll);
> > +	}
> > +
> > +	return 0;
> > +}
> > +
> > +static int mtk_musb_clks_enable(struct mtk_glue *glue)
> > +{
> > +	int ret;
> > +
> > +	ret = clk_prepare_enable(glue->main);
> > +	if (ret) {
> > +		dev_err(glue->dev, "failed to enable main clock\n");
> > +		goto err_main_clk;
> > +	}
> > +
> > +	ret = clk_prepare_enable(glue->mcu);
> > +	if (ret) {
> > +		dev_err(glue->dev, "failed to enable mcu clock\n");
> > +		goto err_mcu_clk;
> > +	}
> > +
> > +	ret = clk_prepare_enable(glue->univpll);
> > +	if (ret) {
> > +		dev_err(glue->dev, "failed to enable univpll clock\n");
> > +		goto err_univpll_clk;
> > +	}
> > +
> > +	return 0;
> > +
> > +err_univpll_clk:
> > +	clk_disable_unprepare(glue->mcu);
> > +err_mcu_clk:
> > +	clk_disable_unprepare(glue->main);
> > +err_main_clk:
> > +	return ret;
> > +}
> > +
> > +static void mtk_musb_clks_disable(struct mtk_glue *glue)
> > +{
> > +	clk_disable_unprepare(glue->univpll);
> > +	clk_disable_unprepare(glue->mcu);
> > +	clk_disable_unprepare(glue->main);
> > +}
> > +
> > +static void mtk_musb_set_vbus(struct musb *musb, int is_on)
> > +{
> > +	struct device *dev = musb->controller;
> > +	struct mtk_glue *glue = dev_get_drvdata(dev->parent);
> > +	int ret;
> > +
> > +	/* vbus is optional */
> > +	if (!glue->vbus)
> > +		return;
> > +
> > +	dev_dbg(musb->controller, "%s, is_on=%d\r\n", __func__, is_on);
> > +	if (is_on) {
> > +		ret = regulator_enable(glue->vbus);
> > +		if (ret) {
> > +			dev_err(glue->dev, "fail to enable vbus regulator\n");
> > +			return;
> > +		}
> > +	} else {
> > +		regulator_disable(glue->vbus);
> > +	}
> > +}
> > +
> > +/*
> > + * switch to host: -> MTK_VBUS_OFF --> MTK_ID_GROUND
> > + * switch to device: -> MTK_ID_FLOAT --> MTK_VBUS_VALID
> > + */
> > +static void mtk_musb_set_mailbox(struct mtk_glue *glue,
> > +	enum mtk_vbus_id_state status)
> 
> add one more TAB for params.

Okay.

> > +{
> > +	struct musb *musb = glue->musb;
> > +	u8 devctl = 0;
> > +
> > +	dev_dbg(glue->dev, "mailbox state(%d)\n", status);
> > +	switch (status) {
> > +	case MTK_ID_GROUND:
> > +		phy_power_on(glue->phy);
> > +		devctl = readb(musb->mregs + MUSB_DEVCTL);
> > +		musb->xceiv->otg->state = OTG_STATE_A_WAIT_VRISE;
> > +		mtk_musb_set_vbus(musb, 1);
> > +		glue->phy_mode = PHY_MODE_USB_HOST;
> > +		phy_set_mode(glue->phy, glue->phy_mode);
> > +		devctl |= MUSB_DEVCTL_SESSION;
> > +		musb_writeb(musb->mregs, MUSB_DEVCTL, devctl);
> > +		MUSB_HST_MODE(musb);
> > +		break;
> > +	/*
> > +	 * MTK_ID_FLOAT process is the same as MTK_VBUS_VALID
> > +	 * except that turn off VBUS
> > +	 */
> > +	case MTK_ID_FLOAT:
> > +		mtk_musb_set_vbus(musb, 0);
> > +		/* fall through */
> > +	case MTK_VBUS_OFF:
> > +		musb->xceiv->otg->state = OTG_STATE_B_IDLE;
> > +		devctl &= ~MUSB_DEVCTL_SESSION;
> > +		musb_writeb(musb->mregs, MUSB_DEVCTL, devctl);
> > +		phy_power_off(glue->phy);
> > +		break;
> > +	case MTK_VBUS_VALID:
> > +		phy_power_on(glue->phy);
> > +		glue->phy_mode = PHY_MODE_USB_DEVICE;
> > +		phy_set_mode(glue->phy, glue->phy_mode);
> > +		MUSB_DEV_MODE(musb);
> > +		break;
> > +	default:
> > +		dev_err(glue->dev, "invalid state\n");
> > +	}
> > +}
> > +
> > +static int mtk_musb_id_notifier(struct notifier_block *nb,
> > +	unsigned long event, void *ptr)
> > +{
> > +	struct mtk_glue *glue = container_of(nb, struct mtk_glue, id_nb);
> > +
> > +	if (event)
> > +		mtk_musb_set_mailbox(glue, MTK_ID_GROUND);
> > +	else
> > +		mtk_musb_set_mailbox(glue, MTK_ID_FLOAT);
> > +
> > +	return NOTIFY_DONE;
> > +}
> > +
> > +static int mtk_musb_vbus_notifier(struct notifier_block *nb,
> > +	unsigned long event, void *ptr)
> > +{
> > +	struct mtk_glue *glue = container_of(nb, struct mtk_glue, vbus_nb);
> > +
> > +	if (event)
> > +		mtk_musb_set_mailbox(glue, MTK_VBUS_VALID);
> > +	else
> > +		mtk_musb_set_mailbox(glue, MTK_VBUS_OFF);
> > +
> > +	return NOTIFY_DONE;
> > +}
> > +
> > +static void mtk_otg_switch_init(struct mtk_glue *glue)
> > +{
> > +	int ret;
> > +
> > +	/* extcon is optional */
> > +	if (!glue->edev)
> > +		return;
> > +
> > +	glue->vbus_nb.notifier_call = mtk_musb_vbus_notifier;
> > +	ret = devm_extcon_register_notifier(glue->dev, glue->edev, EXTCON_USB,
> > +					&glue->vbus_nb);
> > +	if (ret < 0)
> > +		dev_err(glue->dev, "failed to register notifier for USB\n");
> > +
> > +	glue->id_nb.notifier_call = mtk_musb_id_notifier;
> > +	ret = devm_extcon_register_notifier(glue->dev, glue->edev,
> > +					EXTCON_USB_HOST, &glue->id_nb);
> > +	if (ret < 0)
> > +		dev_err(glue->dev, "failed to register notifier for USB-HOST\n");
> > +
> > +	dev_dbg(glue->dev, "EXTCON_USB: %d, EXTCON_USB_HOST: %d\n",
> > +		extcon_get_state(glue->edev, EXTCON_USB),
> > +		extcon_get_state(glue->edev, EXTCON_USB_HOST));
> > +
> > +	/* default as host, switch to device mode if needed */
> > +	if (extcon_get_state(glue->edev, EXTCON_USB_HOST) == false)
> > +		mtk_musb_set_mailbox(glue, MTK_ID_FLOAT);
> > +	if (extcon_get_state(glue->edev, EXTCON_USB) == true)
> > +		mtk_musb_set_mailbox(glue, MTK_VBUS_VALID);
> > +}
> > +
> > +static irqreturn_t generic_interrupt(int irq, void *__hci)
> > +{
> > +	unsigned long flags;
> > +	irqreturn_t retval = IRQ_NONE;
> > +	struct musb *musb = __hci;
> > +
> > +	spin_lock_irqsave(&musb->lock, flags);
> > +	musb->int_usb = musb_readb(musb->mregs, MUSB_INTRUSB) &
> > +	    musb_readb(musb->mregs, MUSB_INTRUSBE);
> > +	musb->int_tx = musb_readw(musb->mregs, MUSB_INTRTX)
> > +	    & musb_readw(musb->mregs, MUSB_INTRTXE);
> > +	musb->int_rx = musb_readw(musb->mregs, MUSB_INTRRX)
> > +	    & musb_readw(musb->mregs, MUSB_INTRRXE);
> > +	/* MediaTek controller interrupt status is W1C */
> > +	musb_clearw(musb->mregs, MUSB_INTRRX, musb->int_rx);
> > +	musb_clearw(musb->mregs, MUSB_INTRTX, musb->int_tx);
> > +	musb_clearb(musb->mregs, MUSB_INTRUSB, musb->int_usb);
> > +
> > +	if (musb->int_usb || musb->int_tx || musb->int_rx)
> > +		retval = musb_interrupt(musb);
> > +
> > +	spin_unlock_irqrestore(&musb->lock, flags);
> > +
> > +	return retval;
> > +}
> > +
> > +static irqreturn_t mtk_musb_interrupt(int irq, void *dev_id)
> > +{
> > +	irqreturn_t retval = IRQ_NONE;
> > +	struct musb *musb = (struct musb *)dev_id;
> > +	u32 l1_ints;
> > +
> > +	l1_ints = musb_readl(musb->mregs, USB_L1INTS) &
> > +			musb_readl(musb->mregs, USB_L1INTM);
> > +
> > +	if (l1_ints & (TX_INT_STATUS | RX_INT_STATUS | USBCOM_INT_STATUS))
> > +		retval = generic_interrupt(irq, musb);
> > +
> > +#if defined(CONFIG_USB_INVENTRA_DMA)
> > +	if (l1_ints & DMA_INT_STATUS)
> > +		retval = dma_controller_irq(irq, musb->dma_controller);
> > +#endif
> > +	return retval;
> > +}
> > +
> > +static u32 mtk_musb_busctl_offset(u8 epnum, u16 offset)
> > +{
> > +	return MTK_MUSB_TXFUNCADDR + offset + 8 * epnum;
> > +}
> > +
> > +static void mtk_musb_clearb(void __iomem *addr, unsigned int offset, u8 data)
> 
> remove 'u8 data' parameter, then add:

Okay.

> > +{
> 
> 	u8 data;
> 
> > +	/* W1C */
> 	data = musb_readb(addr, offset);
> > +	musb_writeb(addr, offset, data);
> > +}
> > +
> > +static void mtk_musb_clearw(void __iomem *addr, unsigned int offset, u16 data)
> > +{
> > +	/* W1C */
> > +	musb_writew(addr, offset, data);
> > +}
> 
> similar as mtk_musb_clearb() above.

Okay.

> > +
> > +static int mtk_musb_init(struct musb *musb)
> > +{
> > +	struct device *dev = musb->controller;
> > +	struct mtk_glue *glue = dev_get_drvdata(dev->parent);
> > +	int ret;
> 
> [snip]
> 
> > +	if (pdata->mode == USB_DR_MODE_OTG)
> > +		mtk_otg_switch_init(glue);
> > +
> > +	dev_info(dev, "USB probe done!\n");
> 
> not really useful, can be removed.

Okay.

> > +	return 0;
> > +
> > +err_device_register:
> > +	mtk_musb_clks_disable(glue);
> > +err_enable_clk:
> > +	pm_runtime_put_sync(dev);
> > +	pm_runtime_disable(dev);
> > +err_unregister_usb_phy:
> > +	usb_phy_generic_unregister(glue->usb_phy);
> > +	return ret;
> > +}
> > +
> > +static int mtk_musb_remove(struct platform_device *pdev)
> > +{
> > +	struct mtk_glue *glue = platform_get_drvdata(pdev);
> > +	struct platform_device *usb_phy = glue->usb_phy;
> > +
> > +	platform_device_unregister(glue->musb_pdev);
> > +	usb_phy_generic_unregister(usb_phy);
> > +
> > +	return 0;
> > +}
> > +
> > +#ifdef CONFIG_OF
> > +static const struct of_device_id mtk_musb_match[] = {
> > +	{.compatible = "mediatek,mtk-musb",},
> > +	{},
> > +};
> > +MODULE_DEVICE_TABLE(of, mtk_musb_match);
> > +#endif
> > +
> > +static struct platform_driver mtk_musb_driver = {
> > +	.probe = mtk_musb_probe,
> > +	.remove = mtk_musb_remove,
> > +	.driver = {
> > +		   .name = "musb-mtk",
> > +		   .of_match_table = of_match_ptr(mtk_musb_match),
> > +	},
> > +};
> > +
> > +module_platform_driver(mtk_musb_driver);
> > +
> > +MODULE_DESCRIPTION("MediaTek MUSB Glue Layer");
> > +MODULE_AUTHOR("Min Guo <min.guo@mediatek.com>");
> > +MODULE_LICENSE("GPL v2");
> > diff --git a/drivers/usb/musb/musb_core.c b/drivers/usb/musb/musb_core.c
> > index b7d5627..2c0d102 100644
> > --- a/drivers/usb/musb/musb_core.c
> > +++ b/drivers/usb/musb/musb_core.c
> > @@ -260,6 +260,11 @@ static void musb_default_writeb(void __iomem *addr, unsigned offset, u8 data)
> >  	__raw_writeb(data, addr + offset);
> >  }
> >  
> > +static void
> > +musb_default_clearb(void __iomem *addr, unsigned int offset, u8 data)
> > +{
> > +}
> 
> don't need this, use musb_readb() for the function pointer.

Okay.

> > +
> >  static u16 musb_default_readw(const void __iomem *addr, unsigned offset)
> >  {
> >  	u16 data = __raw_readw(addr + offset);
> > @@ -274,6 +279,43 @@ static void musb_default_writew(void __iomem *addr, unsigned offset, u16 data)
> >  	__raw_writew(data, addr + offset);
> >  }
> >  
> > +static void
> > +musb_default_clearw(void __iomem *addr, unsigned int offset, u16 data)
> > +{
> > +}
> 
> don't need this, use musb_readw() for the function pointer.

Okay.

> > +
> > +static u16 musb_default_get_toggle(struct musb_qh *qh, int is_in)
> > +{
> > +	void __iomem *epio = qh->hw_ep->regs;
> > +	u16 csr;
> > +
> > +	if (is_in)
> > +		csr = musb_readw(epio, MUSB_RXCSR) & MUSB_RXCSR_H_DATATOGGLE;
> > +	else
> > +		csr = musb_readw(epio, MUSB_TXCSR) & MUSB_TXCSR_H_DATATOGGLE;
> > +
> > +	return csr;
> > +}
> > +
> > +static u16 musb_default_set_toggle(struct musb_qh *qh, int is_in,
> > +	struct urb *urb)
> > +{
> > +	u16 csr = 0;
> > +	u16 toggle = 0;
> 
> no need to assign them 0.

Okay.

> > +
> > +	toggle = usb_gettoggle(urb->dev, qh->epnum, !is_in);
> > +
> > +	if (is_in)
> > +		csr = toggle ? (MUSB_RXCSR_H_WR_DATATOGGLE
> > +				| MUSB_RXCSR_H_DATATOGGLE) : 0;
> > +	else
> > +		csr |= toggle ? (MUSB_TXCSR_H_WR_DATATOGGLE
> > +				| MUSB_TXCSR_H_DATATOGGLE)
> > +				: MUSB_TXCSR_CLRDATATOG;
> > +
> > +	return csr;
> > +}
> > +
> >  /*
> >   * Load an endpoint's FIFO
> >   */
> > @@ -370,12 +412,18 @@ static void musb_default_read_fifo(struct musb_hw_ep *hw_ep, u16 len, u8 *dst)
> >  void (*musb_writeb)(void __iomem *addr, unsigned offset, u8 data);
> >  EXPORT_SYMBOL_GPL(musb_writeb);
> >  
> > +void (*musb_clearb)(void __iomem *addr, unsigned int offset, u8 data);
> > +EXPORT_SYMBOL_GPL(musb_clearb);
> > +
> >  u16 (*musb_readw)(const void __iomem *addr, unsigned offset);
> >  EXPORT_SYMBOL_GPL(musb_readw);
> >  
> >  void (*musb_writew)(void __iomem *addr, unsigned offset, u16 data);
> >  EXPORT_SYMBOL_GPL(musb_writew);
> >  
> > +void (*musb_clearw)(void __iomem *addr, unsigned int offset, u16 data);
> > +EXPORT_SYMBOL_GPL(musb_clearw);
> > +
> >  u32 musb_readl(const void __iomem *addr, unsigned offset)
> >  {
> >  	u32 data = __raw_readl(addr + offset);
> > @@ -1028,6 +1076,11 @@ static void musb_disable_interrupts(struct musb *musb)
> >  	temp = musb_readb(mbase, MUSB_INTRUSB);
> >  	temp = musb_readw(mbase, MUSB_INTRTX);
> >  	temp = musb_readw(mbase, MUSB_INTRRX);
> 
> replace the 3 line above with
> 	musb_clearb/w();

Okay.

> > +
> > +	/* some platform needs clear pending interrupts by manual */
> > +	musb_clearb(mbase, MUSB_INTRUSB, musb_readb(mbase, MUSB_INTRUSB));
> > +	musb_clearw(mbase, MUSB_INTRRX, musb_readw(mbase, MUSB_INTRRX));
> > +	musb_clearw(mbase, MUSB_INTRTX, musb_readw(mbase, MUSB_INTRTX));
> 
> then those are no longer needed.

Okay.

> >  }
> >  
> >  static void musb_enable_interrupts(struct musb *musb)
> > @@ -2192,6 +2245,8 @@ static void musb_deassert_reset(struct work_struct *work)
> >  	musb_writeb = musb_default_writeb;
> >  	musb_readw = musb_default_readw;
> >  	musb_writew = musb_default_writew;
> > +	musb_clearb = musb_default_clearb;
> > +	musb_clearw = musb_default_clearw;
> >  
> >  	/* The musb_platform_init() call:
> >  	 *   - adjusts musb->mregs
> > @@ -2252,10 +2307,14 @@ static void musb_deassert_reset(struct work_struct *work)
> >  		musb_readb = musb->ops->readb;
> >  	if (musb->ops->writeb)
> >  		musb_writeb = musb->ops->writeb;
> > +	if (musb->ops->clearb)
> > +		musb_clearb = musb->ops->clearb;
> 	else
> 		musb_clearb = musb_readb;

Okay.

> >  	if (musb->ops->readw)
> >  		musb_readw = musb->ops->readw;
> >  	if (musb->ops->writew)
> >  		musb_writew = musb->ops->writew;
> > +	if (musb->ops->clearw)
> > +		musb_clearw = musb->ops->clearw;
> 	else
> 		musb_clearw = musb_readw;

Okay.

> >  
> >  #ifndef CONFIG_MUSB_PIO_ONLY
> >  	if (!musb->ops->dma_init || !musb->ops->dma_exit) {
> > @@ -2277,6 +2336,16 @@ static void musb_deassert_reset(struct work_struct *work)
> >  	else
> >  		musb->io.write_fifo = musb_default_write_fifo;
> >  
> > +	if (musb->ops->get_toggle)
> > +		musb->io.get_toggle = musb->ops->get_toggle;
> > +	else
> > +		musb->io.get_toggle = musb_default_get_toggle;
> > +
> > +	if (musb->ops->set_toggle)
> > +		musb->io.set_toggle = musb->ops->set_toggle;
> > +	else
> > +		musb->io.set_toggle = musb_default_set_toggle;
> > +
> >  	if (!musb->xceiv->io_ops) {
> >  		musb->xceiv->io_dev = musb->controller;
> >  		musb->xceiv->io_priv = musb->mregs;
> > diff --git a/drivers/usb/musb/musb_core.h b/drivers/usb/musb/musb_core.h
> > index 04203b7..71dca80 100644
> > --- a/drivers/usb/musb/musb_core.h
> > +++ b/drivers/usb/musb/musb_core.h
> > @@ -27,6 +27,7 @@
> >  struct musb;
> >  struct musb_hw_ep;
> >  struct musb_ep;
> > +struct musb_qh;
> >  
> >  /* Helper defines for struct musb->hwvers */
> >  #define MUSB_HWVERS_MAJOR(x)	((x >> 10) & 0x1f)
> > @@ -119,10 +120,14 @@ enum musb_g_ep0_state {
> >   * @fifo_offset: returns the fifo offset
> >   * @readb:	read 8 bits
> >   * @writeb:	write 8 bits
> > + * @clearb:	clear 8 bits,
> 
> add "could be clear-on-read or W1C" to give more info.

Okay.

> >   * @readw:	read 16 bits
> >   * @writew:	write 16 bits
> > + * @clearw:	clear 16 bits
> >   * @read_fifo:	reads the fifo
> >   * @write_fifo:	writes to fifo
> > + * @get_toggle:	platform specific get toggle function
> > + * @set_toggle:	platform specific set toggle function
> >   * @dma_init:	platform specific dma init function
> >   * @dma_exit:	platform specific dma exit function
> >   * @init:	turns on clocks, sets up platform-specific registers, etc
> > @@ -163,10 +168,14 @@ struct musb_platform_ops {
> >  	u32	(*busctl_offset)(u8 epnum, u16 offset);
> >  	u8	(*readb)(const void __iomem *addr, unsigned offset);
> >  	void	(*writeb)(void __iomem *addr, unsigned offset, u8 data);
> > +	void	(*clearb)(void __iomem *addr, unsigned int offset, u8 data);
> >  	u16	(*readw)(const void __iomem *addr, unsigned offset);
> >  	void	(*writew)(void __iomem *addr, unsigned offset, u16 data);
> > +	void	(*clearw)(void __iomem *addr, unsigned int offset, u16 data);
> >  	void	(*read_fifo)(struct musb_hw_ep *hw_ep, u16 len, u8 *buf);
> >  	void	(*write_fifo)(struct musb_hw_ep *hw_ep, u16 len, const u8 *buf);
> > +	u16	(*get_toggle)(struct musb_qh *qh, int is_in);
> > +	u16	(*set_toggle)(struct musb_qh *qh, int is_in, struct urb *urb);
> >  	struct dma_controller *
> >  		(*dma_init) (struct musb *musb, void __iomem *base);
> >  	void	(*dma_exit)(struct dma_controller *c);
> > diff --git a/drivers/usb/musb/musb_dma.h b/drivers/usb/musb/musb_dma.h
> > index 8f60271..05103ea 100644
> > --- a/drivers/usb/musb/musb_dma.h
> > +++ b/drivers/usb/musb/musb_dma.h
> > @@ -35,6 +35,12 @@
> >   *    whether shared with the Inventra core or separate.
> >   */
> >  
> > +#define MUSB_HSDMA_BASE		0x200
> > +#define MUSB_HSDMA_INTR		(MUSB_HSDMA_BASE + 0)
> > +#define MUSB_HSDMA_CONTROL		0x4
> > +#define MUSB_HSDMA_ADDRESS		0x8
> > +#define MUSB_HSDMA_COUNT		0xc
> > +
> >  #define	DMA_ADDR_INVALID	(~(dma_addr_t)0)
> >  
> >  #ifdef CONFIG_MUSB_PIO_ONLY
> > @@ -191,6 +197,9 @@ static inline void musb_dma_controller_destroy(struct dma_controller *d) { }
> >  extern struct dma_controller *
> >  musbhs_dma_controller_create(struct musb *musb, void __iomem *base);
> >  extern void musbhs_dma_controller_destroy(struct dma_controller *c);
> > +extern struct dma_controller *
> > +musbhs_dma_controller_create_noirq(struct musb *musb, void __iomem *base);
> > +extern irqreturn_t dma_controller_irq(int irq, void *private_data);
> >  
> >  extern struct dma_controller *
> >  tusb_dma_controller_create(struct musb *musb, void __iomem *base);
> > diff --git a/drivers/usb/musb/musb_host.c b/drivers/usb/musb/musb_host.c
> > index 16d0ba4..ba66f44 100644
> > --- a/drivers/usb/musb/musb_host.c
> > +++ b/drivers/usb/musb/musb_host.c
> > @@ -290,39 +290,23 @@ static void musb_giveback(struct musb *musb, struct urb *urb, int status)
> >  static inline void musb_save_toggle(struct musb_qh *qh, int is_in,
> >  				    struct urb *urb)
> >  {
> > -	void __iomem		*epio = qh->hw_ep->regs;
> > -	u16			csr;
> > +	struct musb *musb = qh->hw_ep->musb;
> > +	u16 csr;
> >  
> >  	/*
> >  	 * FIXME: the current Mentor DMA code seems to have
> >  	 * problems getting toggle correct.
> >  	 */
> > -
> > -	if (is_in)
> > -		csr = musb_readw(epio, MUSB_RXCSR) & MUSB_RXCSR_H_DATATOGGLE;
> > -	else
> > -		csr = musb_readw(epio, MUSB_TXCSR) & MUSB_TXCSR_H_DATATOGGLE;
> > -
> > +	csr = musb->io.get_toggle(qh, is_in);
> >  	usb_settoggle(urb->dev, qh->epnum, !is_in, csr ? 1 : 0);
> >  }
> >  
> >  static inline u16 musb_set_toggle(struct musb_qh *qh, int is_in,
> >  	struct urb *urb)
> >  {
> > -	u16 csr = 0;
> > -	u16 toggle = 0;
> > -
> > -	toggle = usb_gettoggle(urb->dev, qh->epnum, !is_in);
> > -
> > -	if (is_in)
> > -		csr = toggle ? (MUSB_RXCSR_H_WR_DATATOGGLE
> > -				| MUSB_RXCSR_H_DATATOGGLE) : 0;
> > -	else
> > -		csr = toggle ? (MUSB_TXCSR_H_WR_DATATOGGLE
> > -				| MUSB_TXCSR_H_DATATOGGLE)
> > -				: MUSB_TXCSR_CLRDATATOG;
> > +	struct musb *musb = qh->hw_ep->musb;
> >  
> > -	return csr;
> > +	return musb->io.set_toggle(qh, is_in, urb);
> >  }
> 
> Those two functions - musb_save_toggle() and musb_set_toggle() are very
> short now, we can get rid of them, and directly use
> musb->io.get/set_toggle().

Okay.

> >  
> >  /*
> > diff --git a/drivers/usb/musb/musb_io.h b/drivers/usb/musb/musb_io.h
> > index 8058a58..9bae09b 100644
> > --- a/drivers/usb/musb/musb_io.h
> > +++ b/drivers/usb/musb/musb_io.h
> > @@ -22,6 +22,8 @@
> >   * @read_fifo:	platform specific function to read fifo
> >   * @write_fifo:	platform specific function to write fifo
> >   * @busctl_offset: platform specific function to get busctl offset
> > + * @get_toggle: platform specific function to get toggle
> > + * @set_toggle: platform specific function to set toggle
> >   */
> >  struct musb_io {
> >  	u32	(*ep_offset)(u8 epnum, u16 offset);
> > @@ -30,13 +32,17 @@ struct musb_io {
> >  	void	(*read_fifo)(struct musb_hw_ep *hw_ep, u16 len, u8 *buf);
> >  	void	(*write_fifo)(struct musb_hw_ep *hw_ep, u16 len, const u8 *buf);
> >  	u32	(*busctl_offset)(u8 epnum, u16 offset);
> > +	u16	(*get_toggle)(struct musb_qh *qh, int is_in);
> > +	u16	(*set_toggle)(struct musb_qh *qh, int is_in, struct urb *urb);
> >  };
> >  
> >  /* Do not add new entries here, add them the struct musb_io instead */
> >  extern u8 (*musb_readb)(const void __iomem *addr, unsigned offset);
> >  extern void (*musb_writeb)(void __iomem *addr, unsigned offset, u8 data);
> > +extern void (*musb_clearb)(void __iomem *addr, unsigned int offset, u8 data);
> >  extern u16 (*musb_readw)(const void __iomem *addr, unsigned offset);
> >  extern void (*musb_writew)(void __iomem *addr, unsigned offset, u16 data);
> > +extern void (*musb_clearw)(void __iomem *addr, unsigned int offset, u16 data);
> >  extern u32 musb_readl(const void __iomem *addr, unsigned offset);
> >  extern void musb_writel(void __iomem *addr, unsigned offset, u32 data);
> >  
> > diff --git a/drivers/usb/musb/musbhsdma.c b/drivers/usb/musb/musbhsdma.c
> > index a688f7f..b05fe68 100644
> > --- a/drivers/usb/musb/musbhsdma.c
> > +++ b/drivers/usb/musb/musbhsdma.c
> > @@ -10,12 +10,7 @@
> >  #include <linux/platform_device.h>
> >  #include <linux/slab.h>
> >  #include "musb_core.h"
> > -
> > -#define MUSB_HSDMA_BASE		0x200
> > -#define MUSB_HSDMA_INTR		(MUSB_HSDMA_BASE + 0)
> > -#define MUSB_HSDMA_CONTROL		0x4
> > -#define MUSB_HSDMA_ADDRESS		0x8
> > -#define MUSB_HSDMA_COUNT		0xc
> > +#include "musb_dma.h"
> >  
> >  #define MUSB_HSDMA_CHANNEL_OFFSET(_bchannel, _offset)		\
> >  		(MUSB_HSDMA_BASE + (_bchannel << 4) + _offset)
> > @@ -268,7 +263,7 @@ static int dma_channel_abort(struct dma_channel *channel)
> >  	return 0;
> >  }
> >  
> > -static irqreturn_t dma_controller_irq(int irq, void *private_data)
> > +irqreturn_t dma_controller_irq(int irq, void *private_data)
> >  {
> >  	struct musb_dma_controller *controller = private_data;
> >  	struct musb *musb = controller->private_data;
> > @@ -291,6 +286,9 @@ static irqreturn_t dma_controller_irq(int irq, void *private_data)
> >  
> >  	int_hsdma = musb_readb(mbase, MUSB_HSDMA_INTR);
> >  
> > +	/* some platform needs clear pending interrupts by manual */
> > +	musb_clearb(musb->mregs, MUSB_HSDMA_INTR, int_hsdma);
> > +
> >  	if (!int_hsdma) {
> >  		musb_dbg(musb, "spurious DMA irq");
> >  
> > @@ -382,6 +380,7 @@ static irqreturn_t dma_controller_irq(int irq, void *private_data)
> >  	spin_unlock_irqrestore(&musb->lock, flags);
> >  	return retval;
> >  }
> > +EXPORT_SYMBOL_GPL(dma_controller_irq);
> >  
> >  void musbhs_dma_controller_destroy(struct dma_controller *c)
> >  {
> > @@ -397,18 +396,10 @@ void musbhs_dma_controller_destroy(struct dma_controller *c)
> >  }
> >  EXPORT_SYMBOL_GPL(musbhs_dma_controller_destroy);
> >  
> > -struct dma_controller *musbhs_dma_controller_create(struct musb *musb,
> > +static struct musb_dma_controller *dma_controller_alloc(struct musb *musb,
> >  						    void __iomem *base)
> >  {
> >  	struct musb_dma_controller *controller;
> > -	struct device *dev = musb->controller;
> > -	struct platform_device *pdev = to_platform_device(dev);
> > -	int irq = platform_get_irq_byname(pdev, "dma");
> > -
> > -	if (irq <= 0) {
> > -		dev_err(dev, "No DMA interrupt line!\n");
> > -		return NULL;
> > -	}
> >  
> >  	controller = kzalloc(sizeof(*controller), GFP_KERNEL);
> >  	if (!controller)
> > @@ -422,6 +413,25 @@ struct dma_controller *musbhs_dma_controller_create(struct musb *musb,
> >  	controller->controller.channel_release = dma_channel_release;
> >  	controller->controller.channel_program = dma_channel_program;
> >  	controller->controller.channel_abort = dma_channel_abort;
> > +	return controller;
> > +}
> > +
> > +struct dma_controller *musbhs_dma_controller_create(struct musb *musb,
> > +						    void __iomem *base)
> > +{
> > +	struct musb_dma_controller *controller;
> > +	struct device *dev = musb->controller;
> > +	struct platform_device *pdev = to_platform_device(dev);
> > +	int irq = platform_get_irq_byname(pdev, "dma");
> > +
> > +	if (irq <= 0) {
> > +		dev_err(dev, "No DMA interrupt line!\n");
> > +		return NULL;
> > +	}
> > +
> > +	controller = dma_controller_alloc(musb, base);
> > +	if (!controller)
> > +		return NULL;
> >  
> >  	if (request_irq(irq, dma_controller_irq, 0,
> >  			dev_name(musb->controller), &controller->controller)) {
> > @@ -436,3 +446,16 @@ struct dma_controller *musbhs_dma_controller_create(struct musb *musb,
> >  	return &controller->controller;
> >  }
> >  EXPORT_SYMBOL_GPL(musbhs_dma_controller_create);
> > +
> > +struct dma_controller *musbhs_dma_controller_create_noirq(struct musb *musb,
> > +						    void __iomem *base)
> > +{
> > +	struct musb_dma_controller *controller;
> > +
> > +	controller = dma_controller_alloc(musb, base);
> > +	if (!controller)
> > +		return NULL;
> > +
> > +	return &controller->controller;
> > +}
> > +EXPORT_SYMBOL_GPL(musbhs_dma_controller_create_noirq);
> 
> regards,
> -Bin.