[PATCH 0/4] Add MediaTek MUSB Controller Driver

[PATCH 0/4] Add MediaTek MUSB Controller Driver

[min.guo]

From: Min Guo <min.guo@mediatek.com>

These patches introduce the MediaTek MUSB controller driver.

The driver can be configured as Dual-Role Device (DRD),
Peripheral Only and Host Only modes. This has beed tested on
MT2701 with a variety of devices in host mode and with the 
f_mass gadget driver in peripheral mode, plugging otg cables
in/out a lot of times in all possible imaginable plug orders

Min Guo (4):
  dt-bindings: usb: musb: Add support for MediaTek musb controller
  arm: dts: mt2701: Add usb2 device nodes
  usb: musb: Move musbhsdma macro definition to musb_dma.h
  usb: musb: Add support for MediaTek musb controller

 .../devicetree/bindings/usb/mediatek,musb.txt      |  49 ++
 arch/arm/boot/dts/mt2701-evb.dts                   |  21 +
 arch/arm/boot/dts/mt2701.dtsi                      |  34 ++
 drivers/usb/musb/Kconfig                           |   8 +-
 drivers/usb/musb/Makefile                          |   1 +
 drivers/usb/musb/mediatek.c                        | 562 +++++++++++++++++++++
 drivers/usb/musb/musb_core.c                       |  10 +
 drivers/usb/musb/musb_core.h                       |   1 +
 drivers/usb/musb/musb_dma.h                        |   7 +
 drivers/usb/musb/musb_host.c                       |  79 ++-
 drivers/usb/musb/musb_regs.h                       |   6 +
 drivers/usb/musb/musbhsdma.c                       |  41 +-
 12 files changed, 782 insertions(+), 37 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/usb/mediatek,musb.txt
 create mode 100644 drivers/usb/musb/mediatek.c

-- 
1.9.1

[PATCH 1/4] dt-bindings: usb: musb: Add support for MediaTek musb controller

[min.guo]

From: Min Guo <min.guo@mediatek.com>

This adds support for MediaTek musb controller in
host, peripheral and otg mode

Signed-off-by: Min Guo <min.guo@mediatek.com>
---
 .../devicetree/bindings/usb/mediatek,musb.txt      | 49 ++++++++++++++++++++++
 1 file changed, 49 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/usb/mediatek,musb.txt

diff --git a/Documentation/devicetree/bindings/usb/mediatek,musb.txt b/Documentation/devicetree/bindings/usb/mediatek,musb.txt
new file mode 100644
index 0000000..e899c9b
--- /dev/null
+++ b/Documentation/devicetree/bindings/usb/mediatek,musb.txt
@@ -0,0 +1,49 @@
+MediaTek musb DRC/OTG controller
+-------------------------------------------
+
+Required properties:
+ - compatible      : should be "mediatek,<soc-model>-musb",
+   "mediatek,mtk-musb", soc-model is the name of SoC, such as
+   mt2701, when using "mediatek,mtk-musb" compatible string, you
+   need SoC specific ones in addition, one of:
+   - "mediatek,mt2701-musb"
+ - reg             : specifies physical base address and size of
+   the registers
+ - interrupts      : interrupt used by musb controller
+ - interrupt-names : must be "mc"
+ - phys            : PHY specifier for the OTG phy
+ - phy-names       : should be "usb2-phy"
+ - dr_mode         : should be one of "host", "peripheral" or "otg",
+   refer to usb/generic.txt
+ - clocks          : a list of phandle + clock-specifier pairs, one for
+   each entry in clock-names
+ - clock-names     : must contain "main","mcu","univpll"
+   for clocks of controller
+
+Optional properties:
+ - extcon : external connector for VBUS and IDPIN changes detection,
+   needed when supports dual-role mode.
+ - vbus-supply : reference to the VBUS regulator, needed when supports
+   dual-role mode.
+ - power-domains   : a phandle to USB power domain node to control USB's
+   MTCMOS
+
+Example:
+
+usb2: usb@11200000 {
+	compatible = "mediatek,mt2701-musb";
+		"mediatek,mtk-musb";
+	reg = <0 0x11200000 0 0x1000>;
+	interrupts = <GIC_SPI 32 IRQ_TYPE_LEVEL_LOW>;
+	interrupt-names = "mc";
+	phys = <&u2port2 PHY_TYPE_USB2>;
+	phy-names = "usb2-phy";
+	vbus-supply = <&usb_vbus>;
+	extcon = <&extcon_usb>;
+	dr_mode = "otg";
+	clocks = <&pericfg CLK_PERI_USB0>,
+		 <&pericfg CLK_PERI_USB0_MCU>,
+		 <&pericfg CLK_PERI_USB_SLV>;
+	clock-names = "main","mcu","univpll";
+	power-domains = <&scpsys MT2701_POWER_DOMAIN_IFR_MSC>;
+};
-- 
1.9.1

[PATCH 2/4] arm: dts: mt2701: Add usb2 device nodes

[min.guo]

From: Min Guo <min.guo@mediatek.com>

Add musb nodes and usb2 phy nodes for MT2701

Signed-off-by: Min Guo <min.guo@mediatek.com>
---
 arch/arm/boot/dts/mt2701-evb.dts | 21 +++++++++++++++++++++
 arch/arm/boot/dts/mt2701.dtsi    | 34 ++++++++++++++++++++++++++++++++++
 2 files changed, 55 insertions(+)

diff --git a/arch/arm/boot/dts/mt2701-evb.dts b/arch/arm/boot/dts/mt2701-evb.dts
index be0edb3..2635911 100644
--- a/arch/arm/boot/dts/mt2701-evb.dts
+++ b/arch/arm/boot/dts/mt2701-evb.dts
@@ -6,6 +6,7 @@
  */
 
 /dts-v1/;
+#include <dt-bindings/gpio/gpio.h>
 #include "mt2701.dtsi"
 
 / {
@@ -60,6 +61,20 @@
 		>;
 		default-brightness-level = <9>;
 	};
+
+	extcon_usb: extcon_iddig {
+		compatible = "linux,extcon-usb-gpio";
+		id-gpio = <&pio 44 GPIO_ACTIVE_HIGH>;
+	};
+
+	usb_vbus: regulator@0 {
+		compatible = "regulator-fixed";
+		regulator-name = "usb_vbus";
+		regulator-min-microvolt = <5000000>;
+		regulator-max-microvolt = <5000000>;
+		gpio = <&pio 45 GPIO_ACTIVE_HIGH>;
+		enable-active-high;
+	};
 };
 
 &auxadc {
@@ -229,3 +244,9 @@
 &uart0 {
 	status = "okay";
 };
+
+&usb2 {
+	vbus-supply = <&usb_vbus>;
+	extcon = <&extcon_usb>;
+	status = "okay";
+};
diff --git a/arch/arm/boot/dts/mt2701.dtsi b/arch/arm/boot/dts/mt2701.dtsi
index 180377e..495ece9 100644
--- a/arch/arm/boot/dts/mt2701.dtsi
+++ b/arch/arm/boot/dts/mt2701.dtsi
@@ -670,6 +670,40 @@
 		};
 	};
 
+	usb2: usb@11200000 {
+		compatible = "mediatek,mt2701-musb",
+				"mediatek,mtk-musb";
+		reg = <0 0x11200000 0 0x1000>;
+		interrupts = <GIC_SPI 32 IRQ_TYPE_LEVEL_LOW>;
+		interrupt-names = "mc";
+		phys = <&u2port2 PHY_TYPE_USB2>;
+		phy-names = "usb2-phy";
+		dr_mode = "otg";
+		clocks = <&pericfg CLK_PERI_USB0>,
+			 <&pericfg CLK_PERI_USB0_MCU>,
+			 <&pericfg CLK_PERI_USB_SLV>;
+		clock-names = "main","mcu","univpll";
+		power-domains = <&scpsys MT2701_POWER_DOMAIN_IFR_MSC>;
+		status = "disabled";
+	};
+
+	u2phy0: usb-phy@11210000 {
+		compatible = "mediatek,generic-tphy-v1";
+		reg = <0 0x11210000 0 0x0800>;
+		#address-cells = <2>;
+		#size-cells = <2>;
+		ranges;
+		status = "okay";
+
+		u2port2: usb-phy@1a1c4800 {
+			reg = <0 0x11210800 0 0x0100>;
+			clocks = <&topckgen CLK_TOP_USB_PHY48M>;
+			clock-names = "ref";
+			#phy-cells = <1>;
+			status = "okay";
+		};
+	};
+
 	ethsys: syscon@1b000000 {
 		compatible = "mediatek,mt2701-ethsys", "syscon";
 		reg = <0 0x1b000000 0 0x1000>;
-- 
1.9.1

[PATCH 3/4] usb: musb: Move musbhsdma macro definition to musb_dma.h

[min.guo]

From: Min Guo <min.guo@mediatek.com>

Because the MediaTek musb controller dose not have a
separate DMA interrupt, these macros are required to
access the dma registers

Signed-off-by: Min Guo <min.guo@mediatek.com>
---
 drivers/usb/musb/musb_dma.h  | 6 ++++++
 drivers/usb/musb/musbhsdma.c | 7 +------
 2 files changed, 7 insertions(+), 6 deletions(-)

diff --git a/drivers/usb/musb/musb_dma.h b/drivers/usb/musb/musb_dma.h
index 8f60271..281e75d3 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
diff --git a/drivers/usb/musb/musbhsdma.c b/drivers/usb/musb/musbhsdma.c
index a688f7f..824adcb 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)
-- 
1.9.1

[PATCH 4/4] usb: musb: Add support for MediaTek musb controller

[min.guo]

From: Min Guo <min.guo@mediatek.com>

This adds support for MediaTek musb controller in
host, peripheral and otg mode

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  | 562 +++++++++++++++++++++++++++++++++++++++++++
 drivers/usb/musb/musb_core.c |  10 +
 drivers/usb/musb/musb_core.h |   1 +
 drivers/usb/musb/musb_dma.h  |   1 +
 drivers/usb/musb/musb_host.c |  79 ++++--
 drivers/usb/musb/musb_regs.h |   6 +
 drivers/usb/musb/musbhsdma.c |  34 ++-
 9 files changed, 671 insertions(+), 31 deletions(-)
 create mode 100644 drivers/usb/musb/mediatek.c

diff --git a/drivers/usb/musb/Kconfig b/drivers/usb/musb/Kconfig
index ad08895..540fc9f 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
+	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..15a6460
--- /dev/null
+++ b/drivers/usb/musb/mediatek.c
@@ -0,0 +1,562 @@
+// 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
+
+#define TX_INT_STATUS		BIT(0)
+#define RX_INT_STATUS		BIT(1)
+#define USBCOM_INT_STATUS	BIT(2)
+#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)
+{
+	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_writew(musb->mregs, MUSB_INTRRX, musb->int_rx);
+	musb_writew(musb->mregs, MUSB_INTRTX, musb->int_tx);
+	musb_writeb(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 int mtk_musb_init(struct musb *musb)
+{
+	struct device *dev = musb->controller;
+	struct mtk_glue *glue = dev_get_drvdata(dev->parent);
+	int ret;
+
+	glue->musb = musb;
+	musb->phy = glue->phy;
+	musb->xceiv = glue->xceiv;
+	musb->is_host = false;
+	musb->isr = mtk_musb_interrupt;
+	ret = phy_init(glue->phy);
+	if (ret)
+		return ret;
+
+	ret = phy_power_on(glue->phy);
+	if (ret) {
+		phy_exit(glue->phy);
+		return ret;
+	}
+
+	phy_set_mode(glue->phy, glue->phy_mode);
+
+#if defined(CONFIG_USB_INVENTRA_DMA)
+	musb_writel(musb->mregs, MUSB_HSDMA_INTR,
+		    DMA_INTR_STATUS_MSK | DMA_INTR_UNMASK_SET_MSK);
+#endif
+	musb_writel(musb->mregs, USB_L1INTM, TX_INT_STATUS | RX_INT_STATUS |
+		    USBCOM_INT_STATUS | DMA_INT_STATUS);
+	return 0;
+}
+
+static int mtk_musb_set_mode(struct musb *musb, u8 mode)
+{
+	struct device *dev = musb->controller;
+	struct mtk_glue *glue = dev_get_drvdata(dev->parent);
+	enum phy_mode new_mode;
+
+	switch (mode) {
+	case MUSB_HOST:
+		new_mode = PHY_MODE_USB_HOST;
+		mtk_musb_set_vbus(musb, 1);
+		break;
+	case MUSB_PERIPHERAL:
+		new_mode = PHY_MODE_USB_DEVICE;
+		break;
+	case MUSB_OTG:
+		new_mode = PHY_MODE_USB_HOST;
+		break;
+	default:
+		dev_err(musb->controller->parent,
+			"Error requested mode not supported by this kernel\n");
+		return -EINVAL;
+	}
+	if (glue->phy_mode == new_mode)
+		return 0;
+
+	mtk_musb_set_mailbox(glue, MTK_ID_GROUND);
+	return 0;
+}
+
+static int mtk_musb_exit(struct musb *musb)
+{
+	struct device *dev = musb->controller;
+	struct mtk_glue *glue = dev_get_drvdata(dev->parent);
+
+	phy_power_off(glue->phy);
+	phy_exit(glue->phy);
+	mtk_musb_clks_disable(glue);
+
+	pm_runtime_put_sync(dev);
+	pm_runtime_disable(dev);
+	return 0;
+}
+
+static const struct musb_platform_ops mtk_musb_ops = {
+	.quirks = MUSB_DMA_INVENTRA | MUSB_MTK_QUIRKS,
+	.init = mtk_musb_init,
+	.exit = mtk_musb_exit,
+#ifdef CONFIG_USB_INVENTRA_DMA
+	.dma_init = musbhs_dma_controller_create,
+	.dma_exit = musbhs_dma_controller_destroy,
+#endif
+	.busctl_offset = mtk_musb_busctl_offset,
+	.set_mode = mtk_musb_set_mode,
+	.set_vbus = mtk_musb_set_vbus,
+};
+
+#define MTK_MUSB_MAX_EP_NUM	8
+#define MTK_MUSB_RAM_BITS	11
+
+static struct musb_fifo_cfg mtk_musb_mode_cfg[] = {
+	{ .hw_ep_num = 1, .style = FIFO_TX, .maxpacket = 512, },
+	{ .hw_ep_num = 1, .style = FIFO_RX, .maxpacket = 512, },
+	{ .hw_ep_num = 2, .style = FIFO_TX, .maxpacket = 512, },
+	{ .hw_ep_num = 2, .style = FIFO_RX, .maxpacket = 512, },
+	{ .hw_ep_num = 3, .style = FIFO_TX, .maxpacket = 512, },
+	{ .hw_ep_num = 3, .style = FIFO_RX, .maxpacket = 512, },
+	{ .hw_ep_num = 4, .style = FIFO_TX, .maxpacket = 512, },
+	{ .hw_ep_num = 4, .style = FIFO_RX, .maxpacket = 512, },
+	{ .hw_ep_num = 5, .style = FIFO_TX, .maxpacket = 512, },
+	{ .hw_ep_num = 5, .style = FIFO_RX, .maxpacket = 512, },
+	{ .hw_ep_num = 6, .style = FIFO_TX, .maxpacket = 1024, },
+	{ .hw_ep_num = 6, .style = FIFO_RX, .maxpacket = 1024, },
+	{ .hw_ep_num = 7, .style = FIFO_TX, .maxpacket = 512, },
+	{ .hw_ep_num = 7, .style = FIFO_RX, .maxpacket = 64, },
+};
+
+static const struct musb_hdrc_config mtk_musb_hdrc_config = {
+	.fifo_cfg = mtk_musb_mode_cfg,
+	.fifo_cfg_size = ARRAY_SIZE(mtk_musb_mode_cfg),
+	.multipoint = true,
+	.dyn_fifo = true,
+	.num_eps = MTK_MUSB_MAX_EP_NUM,
+	.ram_bits = MTK_MUSB_RAM_BITS,
+};
+
+static const struct platform_device_info mtk_dev_info = {
+	.name = "musb-hdrc",
+	.id = PLATFORM_DEVID_AUTO,
+	.dma_mask = DMA_BIT_MASK(32),
+};
+
+static int mtk_musb_probe(struct platform_device *pdev)
+{
+	struct musb_hdrc_platform_data *pdata;
+	struct mtk_glue *glue;
+	struct platform_device_info pinfo;
+	struct device *dev = &pdev->dev;
+	struct device_node *np = dev->of_node;
+	int ret = -ENOMEM;
+
+	glue = devm_kzalloc(dev, sizeof(*glue), GFP_KERNEL);
+	if (!glue)
+		return -ENOMEM;
+
+	glue->dev = dev;
+	pdata = devm_kzalloc(dev, sizeof(*pdata), GFP_KERNEL);
+	if (!pdata)
+		return -ENOMEM;
+
+	ret = mtk_musb_clks_get(glue);
+	if (ret)
+		return ret;
+
+	glue->vbus = devm_regulator_get(dev, "vbus");
+	if (IS_ERR(glue->vbus)) {
+		dev_err(dev, "fail to get vbus\n");
+		return PTR_ERR(glue->vbus);
+	}
+
+	pdata->config = &mtk_musb_hdrc_config;
+	pdata->platform_ops = &mtk_musb_ops;
+	if (of_property_read_bool(np, "extcon")) {
+		glue->edev = extcon_get_edev_by_phandle(dev, 0);
+		if (IS_ERR(glue->edev)) {
+			dev_err(dev, "fail to get extcon\n");
+			return PTR_ERR(glue->edev);
+		}
+	}
+
+	pdata->mode = usb_get_dr_mode(dev);
+	switch (pdata->mode) {
+	case USB_DR_MODE_HOST:
+		glue->phy_mode = PHY_MODE_USB_HOST;
+		break;
+	case USB_DR_MODE_PERIPHERAL:
+		glue->phy_mode = PHY_MODE_USB_DEVICE;
+		break;
+	default:
+		pdata->mode = USB_DR_MODE_OTG;
+		/* FALL THROUGH */
+	case USB_DR_MODE_OTG:
+		glue->phy_mode = PHY_MODE_USB_OTG;
+		break;
+	}
+
+	glue->phy = devm_phy_get(dev, "usb2-phy");
+	if (IS_ERR(glue->phy)) {
+		dev_err(dev, "fail to getting phy %ld\n",
+			PTR_ERR(glue->phy));
+		return PTR_ERR(glue->phy);
+	}
+
+	glue->usb_phy = usb_phy_generic_register();
+	if (IS_ERR(glue->usb_phy)) {
+		dev_err(dev, "fail to registering usb-phy %ld\n",
+			PTR_ERR(glue->usb_phy));
+		return PTR_ERR(glue->usb_phy);
+	}
+
+	glue->xceiv = devm_usb_get_phy(dev, USB_PHY_TYPE_USB2);
+	if (IS_ERR(glue->xceiv)) {
+		dev_err(dev, "fail to getting usb-phy %d\n", ret);
+		ret = PTR_ERR(glue->xceiv);
+		goto err_unregister_usb_phy;
+	}
+
+	platform_set_drvdata(pdev, glue);
+	pm_runtime_enable(dev);
+	pm_runtime_get_sync(dev);
+
+	ret = mtk_musb_clks_enable(glue);
+	if (ret)
+		goto err_enable_clk;
+
+	pinfo = mtk_dev_info;
+	pinfo.parent = dev;
+	pinfo.res = pdev->resource;
+	pinfo.num_res = pdev->num_resources;
+	pinfo.data = pdata;
+	pinfo.size_data = sizeof(*pdata);
+
+	glue->musb_pdev = platform_device_register_full(&pinfo);
+	if (IS_ERR(glue->musb_pdev)) {
+		ret = PTR_ERR(glue->musb_pdev);
+		dev_err(dev, "failed to register musb device: %d\n", ret);
+		goto err_device_register;
+	}
+
+	if (pdata->mode == USB_DR_MODE_OTG)
+		mtk_otg_switch_init(glue);
+
+	dev_info(dev, "USB probe done!\n");
+	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..d60f76f 100644
--- a/drivers/usb/musb/musb_core.c
+++ b/drivers/usb/musb/musb_core.c
@@ -1028,6 +1028,16 @@ 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);
+
+	/*  MediaTek controller interrupt status is W1C */
+	if (musb->ops->quirks & MUSB_MTK_QUIRKS) {
+		musb_writeb(mbase, MUSB_INTRUSB,
+			musb_readb(mbase, MUSB_INTRUSB));
+		musb_writew(mbase, MUSB_INTRRX,
+			musb_readw(mbase, MUSB_INTRRX));
+		musb_writew(mbase, MUSB_INTRTX,
+			musb_readw(mbase, MUSB_INTRTX));
+	}
 }
 
 static void musb_enable_interrupts(struct musb *musb)
diff --git a/drivers/usb/musb/musb_core.h b/drivers/usb/musb/musb_core.h
index 04203b7..1bf4e9a 100644
--- a/drivers/usb/musb/musb_core.h
+++ b/drivers/usb/musb/musb_core.h
@@ -138,6 +138,7 @@ enum musb_g_ep0_state {
  */
 struct musb_platform_ops {
 
+#define MUSB_MTK_QUIRKS	BIT(10)
 #define MUSB_G_NO_SKB_RESERVE	BIT(9)
 #define MUSB_DA8XX		BIT(8)
 #define MUSB_PRESERVE_SESSION	BIT(7)
diff --git a/drivers/usb/musb/musb_dma.h b/drivers/usb/musb/musb_dma.h
index 281e75d3..b218210 100644
--- a/drivers/usb/musb/musb_dma.h
+++ b/drivers/usb/musb/musb_dma.h
@@ -197,6 +197,7 @@ 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 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 b59ce9a..b1b0216 100644
--- a/drivers/usb/musb/musb_host.c
+++ b/drivers/usb/musb/musb_host.c
@@ -292,20 +292,73 @@ static inline void musb_save_toggle(struct musb_qh *qh, int is_in,
 {
 	void __iomem		*epio = qh->hw_ep->regs;
 	u16			csr;
+	struct musb *musb = qh->hw_ep->musb;
 
 	/*
 	 * 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;
+	/* MediaTek controller has private toggle register */
+	if (musb->ops->quirks & MUSB_MTK_QUIRKS) {
+		u16 toggle;
+		u8 epnum = qh->hw_ep->epnum;
+
+		if (is_in)
+			toggle = musb_readl(musb->mregs, MUSB_RXTOG);
+		else
+			toggle = musb_readl(musb->mregs, MUSB_TXTOG);
+
+		csr = toggle & (1 << epnum);
+	} else {
+		if (is_in)
+			csr = musb_readw(epio, MUSB_RXCSR)
+				& MUSB_RXCSR_H_DATATOGGLE;
+		else
+			csr = musb_readw(epio, MUSB_TXCSR)
+				& MUSB_TXCSR_H_DATATOGGLE;
+	}
 
 	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;
+	struct musb *musb = qh->hw_ep->musb;
+	u8 epnum = qh->hw_ep->epnum;
+
+	toggle = usb_gettoggle(urb->dev, qh->epnum, !is_in);
+
+	/* MediaTek controller has private toggle register */
+	if (musb->ops->quirks & MUSB_MTK_QUIRKS) {
+		if (is_in) {
+			musb_writel(musb->mregs, MUSB_RXTOGEN, (1 << epnum));
+			musb_writel(musb->mregs, MUSB_RXTOG, (toggle << epnum));
+		} else {
+			musb_writel(musb->mregs, MUSB_TXTOGEN, (1 << epnum));
+			musb_writel(musb->mregs, MUSB_TXTOG, (toggle << epnum));
+		}
+	} else {
+		if (is_in) {
+			if (toggle)
+				csr = MUSB_RXCSR_H_WR_DATATOGGLE
+						| MUSB_RXCSR_H_DATATOGGLE;
+			else
+				csr = 0;
+		} else {
+			if (toggle)
+				csr |= MUSB_TXCSR_H_WR_DATATOGGLE
+						| MUSB_TXCSR_H_DATATOGGLE;
+			else
+				csr |= MUSB_TXCSR_CLRDATATOG;
+		}
+	}
+	return csr;
+}
+
 /*
  * Advance this hardware endpoint's queue, completing the specified URB and
  * advancing to either the next URB queued to that qh, or else invalidating
@@ -772,13 +825,8 @@ static void musb_ep_program(struct musb *musb, u8 epnum,
 					);
 			csr |= MUSB_TXCSR_MODE;
 
-			if (!hw_ep->tx_double_buffered) {
-				if (usb_gettoggle(urb->dev, qh->epnum, 1))
-					csr |= MUSB_TXCSR_H_WR_DATATOGGLE
-						| MUSB_TXCSR_H_DATATOGGLE;
-				else
-					csr |= MUSB_TXCSR_CLRDATATOG;
-			}
+			if (!hw_ep->tx_double_buffered)
+				csr |= musb_set_toggle(qh, !is_out, urb);
 
 			musb_writew(epio, MUSB_TXCSR, csr);
 			/* REVISIT may need to clear FLUSHFIFO ... */
@@ -860,17 +908,12 @@ static void musb_ep_program(struct musb *musb, u8 epnum,
 
 	/* IN/receive */
 	} else {
-		u16	csr;
+		u16	csr = 0;
 
 		if (hw_ep->rx_reinit) {
 			musb_rx_reinit(musb, qh, epnum);
+			csr |= musb_set_toggle(qh, !is_out, urb);
 
-			/* init new state: toggle and NYET, maybe DMA later */
-			if (usb_gettoggle(urb->dev, qh->epnum, 0))
-				csr = MUSB_RXCSR_H_WR_DATATOGGLE
-					| MUSB_RXCSR_H_DATATOGGLE;
-			else
-				csr = 0;
 			if (qh->type == USB_ENDPOINT_XFER_INT)
 				csr |= MUSB_RXCSR_DISNYET;
 
diff --git a/drivers/usb/musb/musb_regs.h b/drivers/usb/musb/musb_regs.h
index 5cd7264..ffbe267 100644
--- a/drivers/usb/musb/musb_regs.h
+++ b/drivers/usb/musb/musb_regs.h
@@ -273,6 +273,12 @@
 #define MUSB_RXHUBADDR		0x06
 #define MUSB_RXHUBPORT		0x07
 
+/* MediaTek controller toggle enable and status reg */
+#define MUSB_RXTOG		0x80
+#define MUSB_RXTOGEN		0x82
+#define MUSB_TXTOG		0x84
+#define MUSB_TXTOGEN		0x86
+
 static inline u8 musb_read_configdata(void __iomem *mbase)
 {
 	musb_writeb(mbase, MUSB_INDEX, 0);
diff --git a/drivers/usb/musb/musbhsdma.c b/drivers/usb/musb/musbhsdma.c
index 824adcb..c545475 100644
--- a/drivers/usb/musb/musbhsdma.c
+++ b/drivers/usb/musb/musbhsdma.c
@@ -263,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;
@@ -285,6 +285,8 @@ static irqreturn_t dma_controller_irq(int irq, void *private_data)
 	spin_lock_irqsave(&musb->lock, flags);
 
 	int_hsdma = musb_readb(mbase, MUSB_HSDMA_INTR);
+	if (musb->ops->quirks & MUSB_MTK_QUIRKS)
+		musb_writeb(musb->mregs, MUSB_HSDMA_INTR, int_hsdma);
 
 	if (!int_hsdma) {
 		musb_dbg(musb, "spurious DMA irq");
@@ -377,15 +379,17 @@ 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)
 {
 	struct musb_dma_controller *controller = container_of(c,
 			struct musb_dma_controller, controller);
+	struct musb *musb = controller->private_data;
 
 	dma_controller_stop(controller);
 
-	if (controller->irq)
+	if (!(musb->ops->quirks & MUSB_MTK_QUIRKS) && controller->irq)
 		free_irq(controller->irq, c);
 
 	kfree(controller);
@@ -398,11 +402,15 @@ struct dma_controller *musbhs_dma_controller_create(struct musb *musb,
 	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");
+	int irq = -1;
 
-	if (irq <= 0) {
-		dev_err(dev, "No DMA interrupt line!\n");
-		return NULL;
+	if (!(musb->ops->quirks & MUSB_MTK_QUIRKS)) {
+		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);
@@ -418,15 +426,17 @@ struct dma_controller *musbhs_dma_controller_create(struct musb *musb,
 	controller->controller.channel_program = dma_channel_program;
 	controller->controller.channel_abort = dma_channel_abort;
 
-	if (request_irq(irq, dma_controller_irq, 0,
+	if (!(musb->ops->quirks & MUSB_MTK_QUIRKS)) {
+		if (request_irq(irq, dma_controller_irq, 0,
 			dev_name(musb->controller), &controller->controller)) {
-		dev_err(dev, "request_irq %d failed!\n", irq);
-		musb_dma_controller_destroy(&controller->controller);
+			dev_err(dev, "request_irq %d failed!\n", irq);
+			musb_dma_controller_destroy(&controller->controller);
 
-		return NULL;
-	}
+			return NULL;
+		}
 
-	controller->irq = irq;
+		controller->irq = irq;
+	}
 
 	return &controller->controller;
 }
-- 
1.9.1

Re: [PATCH 1/4] dt-bindings: usb: musb: Add support for MediaTek musb controller

[Rob Herring]

On Thu, Dec 27, 2018 at 03:34:23PM +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
> 
> Signed-off-by: Min Guo <min.guo@mediatek.com>
> ---
>  .../devicetree/bindings/usb/mediatek,musb.txt      | 49 ++++++++++++++++++++++
>  1 file changed, 49 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/usb/mediatek,musb.txt
> 
> diff --git a/Documentation/devicetree/bindings/usb/mediatek,musb.txt b/Documentation/devicetree/bindings/usb/mediatek,musb.txt
> new file mode 100644
> index 0000000..e899c9b
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/usb/mediatek,musb.txt
> @@ -0,0 +1,49 @@
> +MediaTek musb DRC/OTG controller
> +-------------------------------------------
> +
> +Required properties:
> + - compatible      : should be "mediatek,<soc-model>-musb",
> +   "mediatek,mtk-musb", soc-model is the name of SoC, such as
> +   mt2701, when using "mediatek,mtk-musb" compatible string, you
> +   need SoC specific ones in addition, one of:
> +   - "mediatek,mt2701-musb"

This isn't very clear. Just drop the <soc-model and enumerate the SoCs:

compatible: should be one of:
    "mediatek,mt2701-musb"
    ...
  followed by "mediatek,mtk-musb"

> + - reg             : specifies physical base address and size of
> +   the registers
> + - interrupts      : interrupt used by musb controller
> + - interrupt-names : must be "mc"

-names is pointless when there is only one.

> + - phys            : PHY specifier for the OTG phy
> + - phy-names       : should be "usb2-phy"

Same here.

> + - dr_mode         : should be one of "host", "peripheral" or "otg",
> +   refer to usb/generic.txt
> + - clocks          : a list of phandle + clock-specifier pairs, one for
> +   each entry in clock-names
> + - clock-names     : must contain "main","mcu","univpll"

space needed after each comma.

> +   for clocks of controller
> +
> +Optional properties:
> + - extcon : external connector for VBUS and IDPIN changes detection,
> +   needed when supports dual-role mode.

Don't use extcon for new bindings. The usb-connector binding should be 
used instead.

> + - vbus-supply : reference to the VBUS regulator, needed when supports
> +   dual-role mode.

The controller is powered from Vbus? Probably not. This belongs in the 
connector or maybe the phy (if the phy is powered from Vbus).

> + - power-domains   : a phandle to USB power domain node to control USB's
> +   MTCMOS
> +
> +Example:
> +
> +usb2: usb@11200000 {
> +	compatible = "mediatek,mt2701-musb";
> +		"mediatek,mtk-musb";
> +	reg = <0 0x11200000 0 0x1000>;
> +	interrupts = <GIC_SPI 32 IRQ_TYPE_LEVEL_LOW>;
> +	interrupt-names = "mc";
> +	phys = <&u2port2 PHY_TYPE_USB2>;
> +	phy-names = "usb2-phy";
> +	vbus-supply = <&usb_vbus>;
> +	extcon = <&extcon_usb>;
> +	dr_mode = "otg";
> +	clocks = <&pericfg CLK_PERI_USB0>,
> +		 <&pericfg CLK_PERI_USB0_MCU>,
> +		 <&pericfg CLK_PERI_USB_SLV>;
> +	clock-names = "main","mcu","univpll";
> +	power-domains = <&scpsys MT2701_POWER_DOMAIN_IFR_MSC>;
> +};
> -- 
> 1.9.1
> 

Re: [PATCH 1/4] dt-bindings: usb: musb: Add support for MediaTek musb controller

[Min Guo]

On Thu, 2019-01-03 at 16:14 -0600, Rob Herring wrote:
> On Thu, Dec 27, 2018 at 03:34:23PM +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
> > 
> > Signed-off-by: Min Guo <min.guo@mediatek.com>
> > ---
> >  .../devicetree/bindings/usb/mediatek,musb.txt      | 49 ++++++++++++++++++++++
> >  1 file changed, 49 insertions(+)
> >  create mode 100644 Documentation/devicetree/bindings/usb/mediatek,musb.txt
> > 
> > diff --git a/Documentation/devicetree/bindings/usb/mediatek,musb.txt b/Documentation/devicetree/bindings/usb/mediatek,musb.txt
> > new file mode 100644
> > index 0000000..e899c9b
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/usb/mediatek,musb.txt
> > @@ -0,0 +1,49 @@
> > +MediaTek musb DRC/OTG controller
> > +-------------------------------------------
> > +
> > +Required properties:
> > + - compatible      : should be "mediatek,<soc-model>-musb",
> > +   "mediatek,mtk-musb", soc-model is the name of SoC, such as
> > +   mt2701, when using "mediatek,mtk-musb" compatible string, you
> > +   need SoC specific ones in addition, one of:
> > +   - "mediatek,mt2701-musb"
> 
> This isn't very clear. Just drop the <soc-model and enumerate the SoCs:
> 
> compatible: should be one of:
>     "mediatek,mt2701-musb"
>     ...
>   followed by "mediatek,mtk-musb"
I will modify it in the next patch.

> > + - reg             : specifies physical base address and size of
> > +   the registers
> > + - interrupts      : interrupt used by musb controller
> > + - interrupt-names : must be "mc"
> 
> -names is pointless when there is only one.
The MUSB core driver has two interrupts, one is for MAC, another for DMA,
but on MTK platform, there is only a MAC interrupt, here following the binding
of MUSB core driver.

> > + - phys            : PHY specifier for the OTG phy
> > + - phy-names       : should be "usb2-phy"
> 
> Same here.
I will modify it in the next patch.

> > + - dr_mode         : should be one of "host", "peripheral" or "otg",
> > +   refer to usb/generic.txt
> > + - clocks          : a list of phandle + clock-specifier pairs, one for
> > +   each entry in clock-names
> > + - clock-names     : must contain "main","mcu","univpll"
> 
> space needed after each comma.
I will modify it in the next patch.

> > +   for clocks of controller
> > +
> > +Optional properties:
> > + - extcon : external connector for VBUS and IDPIN changes detection,
> > +   needed when supports dual-role mode.
> 
> Don't use extcon for new bindings. The usb-connector binding should be 
> used instead.
This is used to detect the changes of the IDPIN and VBUS, the change
events are provided by other drivers, such as extcon-usb-gpio.c, and
then switch MUSB controller to host or device mode, but the
usb-connector can't detect these changes.

> > + - vbus-supply : reference to the VBUS regulator, needed when supports
> > +   dual-role mode.
> 
> The controller is powered from Vbus? Probably not. This belongs in the 
> connector or maybe the phy (if the phy is powered from Vbus).
The Vbus is used to provide 5V voltage to the connected device when the
controller works as host mode.

> > + - power-domains   : a phandle to USB power domain node to control USB's
> > +   MTCMOS
> > +
> > +Example:
> > +
> > +usb2: usb@11200000 {
> > +	compatible = "mediatek,mt2701-musb";
> > +		"mediatek,mtk-musb";
> > +	reg = <0 0x11200000 0 0x1000>;
> > +	interrupts = <GIC_SPI 32 IRQ_TYPE_LEVEL_LOW>;
> > +	interrupt-names = "mc";
> > +	phys = <&u2port2 PHY_TYPE_USB2>;
> > +	phy-names = "usb2-phy";
> > +	vbus-supply = <&usb_vbus>;
> > +	extcon = <&extcon_usb>;
> > +	dr_mode = "otg";
> > +	clocks = <&pericfg CLK_PERI_USB0>,
> > +		 <&pericfg CLK_PERI_USB0_MCU>,
> > +		 <&pericfg CLK_PERI_USB_SLV>;
> > +	clock-names = "main","mcu","univpll";
> > +	power-domains = <&scpsys MT2701_POWER_DOMAIN_IFR_MSC>;
> > +};
> > -- 
> > 1.9.1
> > 

Re: [PATCH 1/4] dt-bindings: usb: musb: Add support for MediaTek musb controller

[Rob Herring]

On Thu, Jan 3, 2019 at 9:00 PM Min Guo <min.guo@mediatek.com> wrote:
>
> On Thu, 2019-01-03 at 16:14 -0600, Rob Herring wrote:
> > On Thu, Dec 27, 2018 at 03:34:23PM +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

[...]

> > > + - interrupts      : interrupt used by musb controller
> > > + - interrupt-names : must be "mc"
> >
> > -names is pointless when there is only one.
> The MUSB core driver has two interrupts, one is for MAC, another for DMA,
> but on MTK platform, there is only a MAC interrupt, here following the binding
> of MUSB core driver.

You should probably be listing the same interrupt number twice if 2
interrupts are combined.

> > > +Optional properties:
> > > + - extcon : external connector for VBUS and IDPIN changes detection,
> > > +   needed when supports dual-role mode.
> >
> > Don't use extcon for new bindings. The usb-connector binding should be
> > used instead.
> This is used to detect the changes of the IDPIN and VBUS, the change
> events are provided by other drivers, such as extcon-usb-gpio.c, and
> then switch MUSB controller to host or device mode, but the
> usb-connector can't detect these changes.

To repeat, do not use extcon binding for new bindings. It is poorly
designed as it reflects extcon driver needs, not a description of the
hardware. If you have ID on GPIO, then that belongs in a usb-connector
node because that GPIO goes to the connector. For Vbus, you should
have a vbus-supply in the connector and use a gpio-regulator if it is
GPIO controlled.

> > > + - vbus-supply : reference to the VBUS regulator, needed when supports
> > > +   dual-role mode.
> >
> > The controller is powered from Vbus? Probably not. This belongs in the
> > connector or maybe the phy (if the phy is powered from Vbus).
> The Vbus is used to provide 5V voltage to the connected device when the
> controller works as host mode.

I know what Vbus is. Unless Vbus is providing power to the host
controller, putting the Vbus supply in the controller node is not a
accurate representation of the hardware.

Rob

Re: [PATCH 1/4] dt-bindings: usb: musb: Add support for MediaTek musb controller

[Min Guo]

On Fri, 2019-01-04 at 10:10 -0600, Rob Herring wrote:
> On Thu, Jan 3, 2019 at 9:00 PM Min Guo <min.guo@mediatek.com> wrote:
> >
> > On Thu, 2019-01-03 at 16:14 -0600, Rob Herring wrote:
> > > On Thu, Dec 27, 2018 at 03:34:23PM +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
> 
> [...]
> 
> > > > + - interrupts      : interrupt used by musb controller
> > > > + - interrupt-names : must be "mc"
> > >
> > > -names is pointless when there is only one.
> > The MUSB core driver has two interrupts, one is for MAC, another for DMA,
> > but on MTK platform, there is only a MAC interrupt, here following the binding
> > of MUSB core driver.
> 
> You should probably be listing the same interrupt number twice if 2
> interrupts are combined.

If only one interrupt is regisetred in driver, dose still need list the
same interrupt number twice in dtsi?

> > > > +Optional properties:
> > > > + - extcon : external connector for VBUS and IDPIN changes detection,
> > > > +   needed when supports dual-role mode.
> > >
> > > Don't use extcon for new bindings. The usb-connector binding should be
> > > used instead.
> > This is used to detect the changes of the IDPIN and VBUS, the change
> > events are provided by other drivers, such as extcon-usb-gpio.c, and
> > then switch MUSB controller to host or device mode, but the
> > usb-connector can't detect these changes.
> 
> To repeat, do not use extcon binding for new bindings. It is poorly
> designed as it reflects extcon driver needs, not a description of the
> hardware. If you have ID on GPIO, then that belongs in a usb-connector
> node because that GPIO goes to the connector. For Vbus, you should
> have a vbus-supply in the connector and use a gpio-regulator if it is
> GPIO controlled.

Sorry, I didn't find a common driver describing the usb-connector. Is
there any driver that I can refer to, specially the way to switch MUSB
controller between host and device mode?

> > > > + - vbus-supply : reference to the VBUS regulator, needed when supports
> > > > +   dual-role mode.
> > >
> > > The controller is powered from Vbus? Probably not. This belongs in the
> > > connector or maybe the phy (if the phy is powered from Vbus).
> > The Vbus is used to provide 5V voltage to the connected device when the
> > controller works as host mode.
> 
> I know what Vbus is. Unless Vbus is providing power to the host
> controller, putting the Vbus supply in the controller node is not a
> accurate representation of the hardware.

I will put vbus-supply in usb-connector after implement it.

> Rob

Re: [PATCH 1/4] dt-bindings: usb: musb: Add support for MediaTek musb controller

[Bin Liu]

Hi,

On Thu, Dec 27, 2018 at 03:34:23PM +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
> 
> Signed-off-by: Min Guo <min.guo@mediatek.com>
> ---
>  .../devicetree/bindings/usb/mediatek,musb.txt      | 49 ++++++++++++++++++++++
>  1 file changed, 49 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/usb/mediatek,musb.txt
> 
> diff --git a/Documentation/devicetree/bindings/usb/mediatek,musb.txt b/Documentation/devicetree/bindings/usb/mediatek,musb.txt
> new file mode 100644
> index 0000000..e899c9b
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/usb/mediatek,musb.txt
> @@ -0,0 +1,49 @@
> +MediaTek musb DRC/OTG controller

s/DRC/DRD/

> +-------------------------------------------
> +
> +Required properties:

[snip]

> +Example:
> +
> +usb2: usb@11200000 {
> +	compatible = "mediatek,mt2701-musb";

s/;/,/

> +		"mediatek,mtk-musb";

Regards,
-Bin.

Re: [PATCH 1/4] dt-bindings: usb: musb: Add support for MediaTek musb controller

[Min Guo]

On Mon, 2019-01-07 at 14:40 -0600, Bin Liu wrote:
> Hi,
> 
> On Thu, Dec 27, 2018 at 03:34:23PM +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
> > 
> > Signed-off-by: Min Guo <min.guo@mediatek.com>
> > ---
> >  .../devicetree/bindings/usb/mediatek,musb.txt      | 49 ++++++++++++++++++++++
> >  1 file changed, 49 insertions(+)
> >  create mode 100644 Documentation/devicetree/bindings/usb/mediatek,musb.txt
> > 
> > diff --git a/Documentation/devicetree/bindings/usb/mediatek,musb.txt b/Documentation/devicetree/bindings/usb/mediatek,musb.txt
> > new file mode 100644
> > index 0000000..e899c9b
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/usb/mediatek,musb.txt
> > @@ -0,0 +1,49 @@
> > +MediaTek musb DRC/OTG controller
> 
> s/DRC/DRD/

I will modify it in next patch.

> > +-------------------------------------------
> > +
> > +Required properties:
> 
> [snip]
> 
> > +Example:
> > +
> > +usb2: usb@11200000 {
> > +	compatible = "mediatek,mt2701-musb";
> 
> s/;/,/

I will modify it in next patch.

> > +		"mediatek,mtk-musb";
> 
> Regards,
> -Bin.

Re: [PATCH 4/4] usb: musb: Add support for MediaTek musb controller

[Bin Liu]

Hi,

On Thu, Dec 27, 2018 at 03:34:26PM +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
> 
> 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  | 562 +++++++++++++++++++++++++++++++++++++++++++
>  drivers/usb/musb/musb_core.c |  10 +
>  drivers/usb/musb/musb_core.h |   1 +
>  drivers/usb/musb/musb_dma.h  |   1 +
>  drivers/usb/musb/musb_host.c |  79 ++++--
>  drivers/usb/musb/musb_regs.h |   6 +
>  drivers/usb/musb/musbhsdma.c |  34 ++-
>  9 files changed, 671 insertions(+), 31 deletions(-)
>  create mode 100644 drivers/usb/musb/mediatek.c
> 
> diff --git a/drivers/usb/musb/Kconfig b/drivers/usb/musb/Kconfig
> index ad08895..540fc9f 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

Please also add '|| COMPILE_TEST' to make testing easier.

> +	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..15a6460
> --- /dev/null
> +++ b/drivers/usb/musb/mediatek.c

[snip]
I will review this section later after we sorted out other things.

> diff --git a/drivers/usb/musb/musb_core.c b/drivers/usb/musb/musb_core.c
> index b7d5627..d60f76f 100644
> --- a/drivers/usb/musb/musb_core.c
> +++ b/drivers/usb/musb/musb_core.c
> @@ -1028,6 +1028,16 @@ 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);
> +
> +	/*  MediaTek controller interrupt status is W1C */

This W1C doesn't match to the MUSB Programming Guide that I have. Those
registers are read-only.
Is the difference due to the IP intergration in the mtk platforms? or is
it a new version of the MUSB controller? If latter, what is the version?

> +	if (musb->ops->quirks & MUSB_MTK_QUIRKS) {

Basically we don't want to use this type of platform specific quirks if
possible, so let's try to not use it.

> +		musb_writeb(mbase, MUSB_INTRUSB,
> +			musb_readb(mbase, MUSB_INTRUSB));

For this clearing register bit operation, please create platform hooks
musb_clearb() and musb_clearw() in struct musb_platform_ops instead,
then follow how musb_readb() pointer is assigned in
musb_init_controller() to use the W1C version for mtk platform.

> +		musb_writew(mbase, MUSB_INTRRX,
> +			musb_readw(mbase, MUSB_INTRRX));
> +		musb_writew(mbase, MUSB_INTRTX,
> +			musb_readw(mbase, MUSB_INTRTX));
> +	}
>  }
>  
>  static void musb_enable_interrupts(struct musb *musb)
> diff --git a/drivers/usb/musb/musb_core.h b/drivers/usb/musb/musb_core.h
> index 04203b7..1bf4e9a 100644
> --- a/drivers/usb/musb/musb_core.h
> +++ b/drivers/usb/musb/musb_core.h
> @@ -138,6 +138,7 @@ enum musb_g_ep0_state {
>   */
>  struct musb_platform_ops {
>  
> +#define MUSB_MTK_QUIRKS	BIT(10)
>  #define MUSB_G_NO_SKB_RESERVE	BIT(9)
>  #define MUSB_DA8XX		BIT(8)
>  #define MUSB_PRESERVE_SESSION	BIT(7)
> diff --git a/drivers/usb/musb/musb_dma.h b/drivers/usb/musb/musb_dma.h
> index 281e75d3..b218210 100644
> --- a/drivers/usb/musb/musb_dma.h
> +++ b/drivers/usb/musb/musb_dma.h
> @@ -197,6 +197,7 @@ 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 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 b59ce9a..b1b0216 100644
> --- a/drivers/usb/musb/musb_host.c
> +++ b/drivers/usb/musb/musb_host.c
> @@ -292,20 +292,73 @@ static inline void musb_save_toggle(struct musb_qh *qh, int is_in,
>  {
>  	void __iomem		*epio = qh->hw_ep->regs;
>  	u16			csr;
> +	struct musb *musb = qh->hw_ep->musb;
>  
>  	/*
>  	 * 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;
> +	/* MediaTek controller has private toggle register */

only one toggle register for all endpoints? how does it handle
difference toggle values for different endpoints?

> +	if (musb->ops->quirks & MUSB_MTK_QUIRKS) {
> +		u16 toggle;
> +		u8 epnum = qh->hw_ep->epnum;
> +
> +		if (is_in)
> +			toggle = musb_readl(musb->mregs, MUSB_RXTOG);

should use musb_readw() instead? MUSB_RXTOG seems to be 16bit.

> +		else
> +			toggle = musb_readl(musb->mregs, MUSB_TXTOG);
> +
> +		csr = toggle & (1 << epnum);
> +	} else {
> +		if (is_in)
> +			csr = musb_readw(epio, MUSB_RXCSR)
> +				& MUSB_RXCSR_H_DATATOGGLE;
> +		else
> +			csr = musb_readw(epio, MUSB_TXCSR)
> +				& MUSB_TXCSR_H_DATATOGGLE;

Does this logic still work for the mtk platform even if it has its own
private toggle register? If so, we don't need to change here.

If not, let's try to not use this quirk flag. Please create a hook
musb_platform_get_toggle() in struct musb_platform_ops.

> +	}
>  
>  	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;
> +	struct musb *musb = qh->hw_ep->musb;
> +	u8 epnum = qh->hw_ep->epnum;
> +
> +	toggle = usb_gettoggle(urb->dev, qh->epnum, !is_in);
> +
> +	/* MediaTek controller has private toggle register */
> +	if (musb->ops->quirks & MUSB_MTK_QUIRKS) {
> +		if (is_in) {
> +			musb_writel(musb->mregs, MUSB_RXTOGEN, (1 << epnum));
> +			musb_writel(musb->mregs, MUSB_RXTOG, (toggle << epnum));
> +		} else {
> +			musb_writel(musb->mregs, MUSB_TXTOGEN, (1 << epnum));
> +			musb_writel(musb->mregs, MUSB_TXTOG, (toggle << epnum));
> +		}
> +	} else {
> +		if (is_in) {
> +			if (toggle)
> +				csr = MUSB_RXCSR_H_WR_DATATOGGLE
> +						| MUSB_RXCSR_H_DATATOGGLE;
> +			else
> +				csr = 0;
> +		} else {
> +			if (toggle)
> +				csr |= MUSB_TXCSR_H_WR_DATATOGGLE
> +						| MUSB_TXCSR_H_DATATOGGLE;
> +			else
> +				csr |= MUSB_TXCSR_CLRDATATOG;
> +		}
> +	}
> +	return csr;

Please create a seperate patch for this musb_set_toggle() without adding
the mtk logic. It is a nice cleanup.

> +}
> +
>  /*
>   * Advance this hardware endpoint's queue, completing the specified URB and
>   * advancing to either the next URB queued to that qh, or else invalidating
> @@ -772,13 +825,8 @@ static void musb_ep_program(struct musb *musb, u8 epnum,
>  					);
>  			csr |= MUSB_TXCSR_MODE;
>  
> -			if (!hw_ep->tx_double_buffered) {
> -				if (usb_gettoggle(urb->dev, qh->epnum, 1))
> -					csr |= MUSB_TXCSR_H_WR_DATATOGGLE
> -						| MUSB_TXCSR_H_DATATOGGLE;
> -				else
> -					csr |= MUSB_TXCSR_CLRDATATOG;
> -			}
> +			if (!hw_ep->tx_double_buffered)
> +				csr |= musb_set_toggle(qh, !is_out, urb);
>  
>  			musb_writew(epio, MUSB_TXCSR, csr);
>  			/* REVISIT may need to clear FLUSHFIFO ... */
> @@ -860,17 +908,12 @@ static void musb_ep_program(struct musb *musb, u8 epnum,
>  
>  	/* IN/receive */
>  	} else {
> -		u16	csr;
> +		u16	csr = 0;
>  
>  		if (hw_ep->rx_reinit) {
>  			musb_rx_reinit(musb, qh, epnum);
> +			csr |= musb_set_toggle(qh, !is_out, urb);
>  
> -			/* init new state: toggle and NYET, maybe DMA later */
> -			if (usb_gettoggle(urb->dev, qh->epnum, 0))
> -				csr = MUSB_RXCSR_H_WR_DATATOGGLE
> -					| MUSB_RXCSR_H_DATATOGGLE;
> -			else
> -				csr = 0;
>  			if (qh->type == USB_ENDPOINT_XFER_INT)
>  				csr |= MUSB_RXCSR_DISNYET;
>  
> diff --git a/drivers/usb/musb/musb_regs.h b/drivers/usb/musb/musb_regs.h
> index 5cd7264..ffbe267 100644
> --- a/drivers/usb/musb/musb_regs.h
> +++ b/drivers/usb/musb/musb_regs.h
> @@ -273,6 +273,12 @@
>  #define MUSB_RXHUBADDR		0x06
>  #define MUSB_RXHUBPORT		0x07
>  
> +/* MediaTek controller toggle enable and status reg */
> +#define MUSB_RXTOG		0x80
> +#define MUSB_RXTOGEN		0x82
> +#define MUSB_TXTOG		0x84
> +#define MUSB_TXTOGEN		0x86

Again, these offsets are for different registers in the MUSB version I
have, please let me know if you have different version of the MUSB IP.

> +
>  static inline u8 musb_read_configdata(void __iomem *mbase)
>  {
>  	musb_writeb(mbase, MUSB_INDEX, 0);
> diff --git a/drivers/usb/musb/musbhsdma.c b/drivers/usb/musb/musbhsdma.c
> index 824adcb..c545475 100644
> --- a/drivers/usb/musb/musbhsdma.c
> +++ b/drivers/usb/musb/musbhsdma.c
> @@ -263,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;
> @@ -285,6 +285,8 @@ static irqreturn_t dma_controller_irq(int irq, void *private_data)
>  	spin_lock_irqsave(&musb->lock, flags);
>  
>  	int_hsdma = musb_readb(mbase, MUSB_HSDMA_INTR);
> +	if (musb->ops->quirks & MUSB_MTK_QUIRKS)
> +		musb_writeb(musb->mregs, MUSB_HSDMA_INTR, int_hsdma);

you can use musb_clearb() defined above to get rid of this quirk.

>  
>  	if (!int_hsdma) {
>  		musb_dbg(musb, "spurious DMA irq");
> @@ -377,15 +379,17 @@ 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)
>  {
>  	struct musb_dma_controller *controller = container_of(c,
>  			struct musb_dma_controller, controller);
> +	struct musb *musb = controller->private_data;
>  
>  	dma_controller_stop(controller);
>  
> -	if (controller->irq)
> +	if (!(musb->ops->quirks & MUSB_MTK_QUIRKS) && controller->irq)
>  		free_irq(controller->irq, c);
>  
>  	kfree(controller);
> @@ -398,11 +402,15 @@ struct dma_controller *musbhs_dma_controller_create(struct musb *musb,
>  	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");
> +	int irq = -1;
>  
> -	if (irq <= 0) {
> -		dev_err(dev, "No DMA interrupt line!\n");
> -		return NULL;
> +	if (!(musb->ops->quirks & MUSB_MTK_QUIRKS)) {
> +		irq = platform_get_irq_byname(pdev, "dma");
> +
> +		if (irq < 0) {
> +			dev_err(dev, "No DMA interrupt line!\n");
> +			return NULL;
> +		}
>  	}

Please create musbhs_dma_controller_destroy_noirq() for your platform to
not use the quirk.

>  
>  	controller = kzalloc(sizeof(*controller), GFP_KERNEL);
> @@ -418,15 +426,17 @@ struct dma_controller *musbhs_dma_controller_create(struct musb *musb,
>  	controller->controller.channel_program = dma_channel_program;
>  	controller->controller.channel_abort = dma_channel_abort;
>  
> -	if (request_irq(irq, dma_controller_irq, 0,
> +	if (!(musb->ops->quirks & MUSB_MTK_QUIRKS)) {
> +		if (request_irq(irq, dma_controller_irq, 0,
>  			dev_name(musb->controller), &controller->controller)) {
> -		dev_err(dev, "request_irq %d failed!\n", irq);
> -		musb_dma_controller_destroy(&controller->controller);
> +			dev_err(dev, "request_irq %d failed!\n", irq);
> +			musb_dma_controller_destroy(&controller->controller);
>  
> -		return NULL;
> -	}
> +			return NULL;
> +		}
>  
> -	controller->irq = irq;
> +		controller->irq = irq;
> +	}
>  
>  	return &controller->controller;
>  }

Same here, create musbhs_dma_controller_create_noirq(). Then use both
new API for the mtk glue driver.

Regards,
-Bin.

Re: [PATCH 4/4] usb: musb: Add support for MediaTek musb controller

[Min Guo]

Hi Bin,
On Tue, 2019-01-08 at 09:44 -0600, Bin Liu wrote:
> Hi,
> 
> On Thu, Dec 27, 2018 at 03:34:26PM +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
> > 
> > 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  | 562 +++++++++++++++++++++++++++++++++++++++++++
> >  drivers/usb/musb/musb_core.c |  10 +
> >  drivers/usb/musb/musb_core.h |   1 +
> >  drivers/usb/musb/musb_dma.h  |   1 +
> >  drivers/usb/musb/musb_host.c |  79 ++++--
> >  drivers/usb/musb/musb_regs.h |   6 +
> >  drivers/usb/musb/musbhsdma.c |  34 ++-
> >  9 files changed, 671 insertions(+), 31 deletions(-)
> >  create mode 100644 drivers/usb/musb/mediatek.c
> > 
> > diff --git a/drivers/usb/musb/Kconfig b/drivers/usb/musb/Kconfig
> > index ad08895..540fc9f 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
> 
> Please also add '|| COMPILE_TEST' to make testing easier.

Ok

> > +	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..15a6460
> > --- /dev/null
> > +++ b/drivers/usb/musb/mediatek.c
> 
> [snip]
> I will review this section later after we sorted out other things.
> 
> > diff --git a/drivers/usb/musb/musb_core.c b/drivers/usb/musb/musb_core.c
> > index b7d5627..d60f76f 100644
> > --- a/drivers/usb/musb/musb_core.c
> > +++ b/drivers/usb/musb/musb_core.c
> > @@ -1028,6 +1028,16 @@ 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);
> > +
> > +	/*  MediaTek controller interrupt status is W1C */
> 
> This W1C doesn't match to the MUSB Programming Guide that I have. Those
> registers are read-only.
> Is the difference due to the IP intergration in the mtk platforms? or is
> it a new version of the MUSB controller? If latter, what is the version?

This is difference due to the IP intergration in mtk platforms. W1C is
easy for CpdeViser debug.

> > +	if (musb->ops->quirks & MUSB_MTK_QUIRKS) {
> 
> Basically we don't want to use this type of platform specific quirks if
> possible, so let's try to not use it.

I will try my best to avoid using it.

> > +		musb_writeb(mbase, MUSB_INTRUSB,
> > +			musb_readb(mbase, MUSB_INTRUSB));
> 
> For this clearing register bit operation, please create platform hooks
> musb_clearb() and musb_clearw() in struct musb_platform_ops instead,
> then follow how musb_readb() pointer is assigned in
> musb_init_controller() to use the W1C version for mtk platform.

I have tried implementing musb_readb(), musb_readw() interface with
interrupt status W1C function in struct musb_platform_ops. But this
interface will require a global variable to hold MAC basic address for
judgment, and then special handling of the interrupt state. A global
variable will make the driver work with only a single instance, so it
can't work on some MTK platforms which have two instances.
How about creating musb_clearb/w() as following:
void (*clearb)(void __iomem *addr, unsigned offset, u8 data);
void (*clearw)(void __iomem *addr, unsigned offset, u16 data);


> > +		musb_writew(mbase, MUSB_INTRRX,
> > +			musb_readw(mbase, MUSB_INTRRX));
> > +		musb_writew(mbase, MUSB_INTRTX,
> > +			musb_readw(mbase, MUSB_INTRTX));
> > +	}
> >  }
> >  
> >  static void musb_enable_interrupts(struct musb *musb)
> > diff --git a/drivers/usb/musb/musb_core.h b/drivers/usb/musb/musb_core.h
> > index 04203b7..1bf4e9a 100644
> > --- a/drivers/usb/musb/musb_core.h
> > +++ b/drivers/usb/musb/musb_core.h
> > @@ -138,6 +138,7 @@ enum musb_g_ep0_state {
> >   */
> >  struct musb_platform_ops {
> >  
> > +#define MUSB_MTK_QUIRKS	BIT(10)
> >  #define MUSB_G_NO_SKB_RESERVE	BIT(9)
> >  #define MUSB_DA8XX		BIT(8)
> >  #define MUSB_PRESERVE_SESSION	BIT(7)
> > diff --git a/drivers/usb/musb/musb_dma.h b/drivers/usb/musb/musb_dma.h
> > index 281e75d3..b218210 100644
> > --- a/drivers/usb/musb/musb_dma.h
> > +++ b/drivers/usb/musb/musb_dma.h
> > @@ -197,6 +197,7 @@ 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 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 b59ce9a..b1b0216 100644
> > --- a/drivers/usb/musb/musb_host.c
> > +++ b/drivers/usb/musb/musb_host.c
> > @@ -292,20 +292,73 @@ static inline void musb_save_toggle(struct musb_qh *qh, int is_in,
> >  {
> >  	void __iomem		*epio = qh->hw_ep->regs;
> >  	u16			csr;
> > +	struct musb *musb = qh->hw_ep->musb;
> >  
> >  	/*
> >  	 * 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;
> > +	/* MediaTek controller has private toggle register */
> 
> only one toggle register for all endpoints? how does it handle
> difference toggle values for different endpoints?

MediaTek controller has separate registers to describe TX/RX toggle.

> > +	if (musb->ops->quirks & MUSB_MTK_QUIRKS) {
> > +		u16 toggle;
> > +		u8 epnum = qh->hw_ep->epnum;
> > +
> > +		if (is_in)
> > +			toggle = musb_readl(musb->mregs, MUSB_RXTOG);
> 
> should use musb_readw() instead? MUSB_RXTOG seems to be 16bit.

Ok

> > +		else
> > +			toggle = musb_readl(musb->mregs, MUSB_TXTOG);
> > +
> > +		csr = toggle & (1 << epnum);
> > +	} else {
> > +		if (is_in)
> > +			csr = musb_readw(epio, MUSB_RXCSR)
> > +				& MUSB_RXCSR_H_DATATOGGLE;
> > +		else
> > +			csr = musb_readw(epio, MUSB_TXCSR)
> > +				& MUSB_TXCSR_H_DATATOGGLE;
> 
> Does this logic still work for the mtk platform even if it has its own
> private toggle register? If so, we don't need to change here.

Sorry, this logic can not work on mtk platform, bit
MUSB_RXCSR_H_DATATOGGLE and MUSB_TXCSR_H_DATATOGGLE are used for other
function.

> If not, let's try to not use this quirk flag. Please create a hook
> musb_platform_get_toggle() in struct musb_platform_ops.

Does the method of implement musb_platform_get_toggle() is prepare
musb_default_get_toggle with common function, then follow how
musb_readb() pointer is assigned in musb_init_controller()?

How about creating musb_platform_get_toggle() as following:
u16 (*get_toggle)(struct musb* musb, struct musb_qh *qh, int 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;
> > +	struct musb *musb = qh->hw_ep->musb;
> > +	u8 epnum = qh->hw_ep->epnum;
> > +
> > +	toggle = usb_gettoggle(urb->dev, qh->epnum, !is_in);
> > +
> > +	/* MediaTek controller has private toggle register */
> > +	if (musb->ops->quirks & MUSB_MTK_QUIRKS) {
> > +		if (is_in) {
> > +			musb_writel(musb->mregs, MUSB_RXTOGEN, (1 << epnum));
> > +			musb_writel(musb->mregs, MUSB_RXTOG, (toggle << epnum));
> > +		} else {
> > +			musb_writel(musb->mregs, MUSB_TXTOGEN, (1 << epnum));
> > +			musb_writel(musb->mregs, MUSB_TXTOG, (toggle << epnum));
> > +		}
> > +	} else {
> > +		if (is_in) {
> > +			if (toggle)
> > +				csr = MUSB_RXCSR_H_WR_DATATOGGLE
> > +						| MUSB_RXCSR_H_DATATOGGLE;
> > +			else
> > +				csr = 0;
> > +		} else {
> > +			if (toggle)
> > +				csr |= MUSB_TXCSR_H_WR_DATATOGGLE
> > +						| MUSB_TXCSR_H_DATATOGGLE;
> > +			else
> > +				csr |= MUSB_TXCSR_CLRDATATOG;
> > +		}
> > +	}
> > +	return csr;
> 
> Please create a seperate patch for this musb_set_toggle() without adding
> the mtk logic. It is a nice cleanup.

Does this like get toggle implementation, create a hook
musb_platform_set_toggle() in struct musb_platform_ops?

> > +}
> > +
> >  /*
> >   * Advance this hardware endpoint's queue, completing the specified URB and
> >   * advancing to either the next URB queued to that qh, or else invalidating
> > @@ -772,13 +825,8 @@ static void musb_ep_program(struct musb *musb, u8 epnum,
> >  					);
> >  			csr |= MUSB_TXCSR_MODE;
> >  
> > -			if (!hw_ep->tx_double_buffered) {
> > -				if (usb_gettoggle(urb->dev, qh->epnum, 1))
> > -					csr |= MUSB_TXCSR_H_WR_DATATOGGLE
> > -						| MUSB_TXCSR_H_DATATOGGLE;
> > -				else
> > -					csr |= MUSB_TXCSR_CLRDATATOG;
> > -			}
> > +			if (!hw_ep->tx_double_buffered)
> > +				csr |= musb_set_toggle(qh, !is_out, urb);
> >  
> >  			musb_writew(epio, MUSB_TXCSR, csr);
> >  			/* REVISIT may need to clear FLUSHFIFO ... */
> > @@ -860,17 +908,12 @@ static void musb_ep_program(struct musb *musb, u8 epnum,
> >  
> >  	/* IN/receive */
> >  	} else {
> > -		u16	csr;
> > +		u16	csr = 0;
> >  
> >  		if (hw_ep->rx_reinit) {
> >  			musb_rx_reinit(musb, qh, epnum);
> > +			csr |= musb_set_toggle(qh, !is_out, urb);
> >  
> > -			/* init new state: toggle and NYET, maybe DMA later */
> > -			if (usb_gettoggle(urb->dev, qh->epnum, 0))
> > -				csr = MUSB_RXCSR_H_WR_DATATOGGLE
> > -					| MUSB_RXCSR_H_DATATOGGLE;
> > -			else
> > -				csr = 0;
> >  			if (qh->type == USB_ENDPOINT_XFER_INT)
> >  				csr |= MUSB_RXCSR_DISNYET;
> >  
> > diff --git a/drivers/usb/musb/musb_regs.h b/drivers/usb/musb/musb_regs.h
> > index 5cd7264..ffbe267 100644
> > --- a/drivers/usb/musb/musb_regs.h
> > +++ b/drivers/usb/musb/musb_regs.h
> > @@ -273,6 +273,12 @@
> >  #define MUSB_RXHUBADDR		0x06
> >  #define MUSB_RXHUBPORT		0x07
> >  
> > +/* MediaTek controller toggle enable and status reg */
> > +#define MUSB_RXTOG		0x80
> > +#define MUSB_RXTOGEN		0x82
> > +#define MUSB_TXTOG		0x84
> > +#define MUSB_TXTOGEN		0x86
> 
> Again, these offsets are for different registers in the MUSB version I
> have, please let me know if you have different version of the MUSB IP.

Sorry, these are MediaTek controller private registers used for control
toggle.

> > +
> >  static inline u8 musb_read_configdata(void __iomem *mbase)
> >  {
> >  	musb_writeb(mbase, MUSB_INDEX, 0);
> > diff --git a/drivers/usb/musb/musbhsdma.c b/drivers/usb/musb/musbhsdma.c
> > index 824adcb..c545475 100644
> > --- a/drivers/usb/musb/musbhsdma.c
> > +++ b/drivers/usb/musb/musbhsdma.c
> > @@ -263,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;
> > @@ -285,6 +285,8 @@ static irqreturn_t dma_controller_irq(int irq, void *private_data)
> >  	spin_lock_irqsave(&musb->lock, flags);
> >  
> >  	int_hsdma = musb_readb(mbase, MUSB_HSDMA_INTR);
> > +	if (musb->ops->quirks & MUSB_MTK_QUIRKS)
> > +		musb_writeb(musb->mregs, MUSB_HSDMA_INTR, int_hsdma);
> 
> you can use musb_clearb() defined above to get rid of this quirk.

OK. 

> >  
> >  	if (!int_hsdma) {
> >  		musb_dbg(musb, "spurious DMA irq");
> > @@ -377,15 +379,17 @@ 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)
> >  {
> >  	struct musb_dma_controller *controller = container_of(c,
> >  			struct musb_dma_controller, controller);
> > +	struct musb *musb = controller->private_data;
> >  
> >  	dma_controller_stop(controller);
> >  
> > -	if (controller->irq)
> > +	if (!(musb->ops->quirks & MUSB_MTK_QUIRKS) && controller->irq)
> >  		free_irq(controller->irq, c);
> >  
> >  	kfree(controller);
> > @@ -398,11 +402,15 @@ struct dma_controller *musbhs_dma_controller_create(struct musb *musb,
> >  	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");
> > +	int irq = -1;
> >  
> > -	if (irq <= 0) {
> > -		dev_err(dev, "No DMA interrupt line!\n");
> > -		return NULL;
> > +	if (!(musb->ops->quirks & MUSB_MTK_QUIRKS)) {
> > +		irq = platform_get_irq_byname(pdev, "dma");
> > +
> > +		if (irq < 0) {
> > +			dev_err(dev, "No DMA interrupt line!\n");
> > +			return NULL;
> > +		}
> >  	}
> 
> Please create musbhs_dma_controller_destroy_noirq() for your platform to
> not use the quirk.

OK.

> >  
> >  	controller = kzalloc(sizeof(*controller), GFP_KERNEL);
> > @@ -418,15 +426,17 @@ struct dma_controller *musbhs_dma_controller_create(struct musb *musb,
> >  	controller->controller.channel_program = dma_channel_program;
> >  	controller->controller.channel_abort = dma_channel_abort;
> >  
> > -	if (request_irq(irq, dma_controller_irq, 0,
> > +	if (!(musb->ops->quirks & MUSB_MTK_QUIRKS)) {
> > +		if (request_irq(irq, dma_controller_irq, 0,
> >  			dev_name(musb->controller), &controller->controller)) {
> > -		dev_err(dev, "request_irq %d failed!\n", irq);
> > -		musb_dma_controller_destroy(&controller->controller);
> > +			dev_err(dev, "request_irq %d failed!\n", irq);
> > +			musb_dma_controller_destroy(&controller->controller);
> >  
> > -		return NULL;
> > -	}
> > +			return NULL;
> > +		}
> >  
> > -	controller->irq = irq;
> > +		controller->irq = irq;
> > +	}
> >  
> >  	return &controller->controller;
> >  }
> 
> Same here, create musbhs_dma_controller_create_noirq(). Then use both
> new API for the mtk glue driver.

OK.

> Regards,
> -Bin.

Re: [PATCH 4/4] usb: musb: Add support for MediaTek musb controller

[Bin Liu]

Hi Min,

On Wed, Jan 09, 2019 at 08:31:08PM +0800, Min Guo wrote:
> Hi Bin,
> On Tue, 2019-01-08 at 09:44 -0600, Bin Liu wrote:
> > Hi,
> > 
> > On Thu, Dec 27, 2018 at 03:34:26PM +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
> > > 
> > > 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  | 562 +++++++++++++++++++++++++++++++++++++++++++
> > >  drivers/usb/musb/musb_core.c |  10 +
> > >  drivers/usb/musb/musb_core.h |   1 +
> > >  drivers/usb/musb/musb_dma.h  |   1 +
> > >  drivers/usb/musb/musb_host.c |  79 ++++--
> > >  drivers/usb/musb/musb_regs.h |   6 +
> > >  drivers/usb/musb/musbhsdma.c |  34 ++-
> > >  9 files changed, 671 insertions(+), 31 deletions(-)
> > >  create mode 100644 drivers/usb/musb/mediatek.c
> > > 
> > > diff --git a/drivers/usb/musb/Kconfig b/drivers/usb/musb/Kconfig
> > > index ad08895..540fc9f 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
> > 
> > Please also add '|| COMPILE_TEST' to make testing easier.
> 
> Ok
> 
> > > +	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..15a6460
> > > --- /dev/null
> > > +++ b/drivers/usb/musb/mediatek.c
> > 
> > [snip]
> > I will review this section later after we sorted out other things.
> > 
> > > diff --git a/drivers/usb/musb/musb_core.c b/drivers/usb/musb/musb_core.c
> > > index b7d5627..d60f76f 100644
> > > --- a/drivers/usb/musb/musb_core.c
> > > +++ b/drivers/usb/musb/musb_core.c
> > > @@ -1028,6 +1028,16 @@ 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);
> > > +
> > > +	/*  MediaTek controller interrupt status is W1C */
> > 
> > This W1C doesn't match to the MUSB Programming Guide that I have. Those
> > registers are read-only.
> > Is the difference due to the IP intergration in the mtk platforms? or is
> > it a new version of the MUSB controller? If latter, what is the version?
> 
> This is difference due to the IP intergration in mtk platforms. W1C is
> easy for CpdeViser debug.
> 
> > > +	if (musb->ops->quirks & MUSB_MTK_QUIRKS) {
> > 
> > Basically we don't want to use this type of platform specific quirks if
> > possible, so let's try to not use it.
> 
> I will try my best to avoid using it.

Thanks.

> 
> > > +		musb_writeb(mbase, MUSB_INTRUSB,
> > > +			musb_readb(mbase, MUSB_INTRUSB));
> > 
> > For this clearing register bit operation, please create platform hooks
> > musb_clearb() and musb_clearw() in struct musb_platform_ops instead,
> > then follow how musb_readb() pointer is assigned in
> > musb_init_controller() to use the W1C version for mtk platform.
> 
> I have tried implementing musb_readb(), musb_readw() interface with
> interrupt status W1C function in struct musb_platform_ops. But this
> interface will require a global variable to hold MAC basic address for
> judgment, and then special handling of the interrupt state. A global
> variable will make the driver work with only a single instance, so it
> can't work on some MTK platforms which have two instances.

I didn't mean to modify musb_read*(), but

> How about creating musb_clearb/w() as following:
> void (*clearb)(void __iomem *addr, unsigned offset, u8 data);
> void (*clearw)(void __iomem *addr, unsigned offset, u16 data);

this is what I was asking for, similar to what musb_readb/w() is
implemented.

> 
> 
> > > +		musb_writew(mbase, MUSB_INTRRX,
> > > +			musb_readw(mbase, MUSB_INTRRX));
> > > +		musb_writew(mbase, MUSB_INTRTX,
> > > +			musb_readw(mbase, MUSB_INTRTX));
> > > +	}
> > >  }
> > >  
> > >  static void musb_enable_interrupts(struct musb *musb)
> > > diff --git a/drivers/usb/musb/musb_core.h b/drivers/usb/musb/musb_core.h
> > > index 04203b7..1bf4e9a 100644
> > > --- a/drivers/usb/musb/musb_core.h
> > > +++ b/drivers/usb/musb/musb_core.h
> > > @@ -138,6 +138,7 @@ enum musb_g_ep0_state {
> > >   */
> > >  struct musb_platform_ops {
> > >  
> > > +#define MUSB_MTK_QUIRKS	BIT(10)
> > >  #define MUSB_G_NO_SKB_RESERVE	BIT(9)
> > >  #define MUSB_DA8XX		BIT(8)
> > >  #define MUSB_PRESERVE_SESSION	BIT(7)
> > > diff --git a/drivers/usb/musb/musb_dma.h b/drivers/usb/musb/musb_dma.h
> > > index 281e75d3..b218210 100644
> > > --- a/drivers/usb/musb/musb_dma.h
> > > +++ b/drivers/usb/musb/musb_dma.h
> > > @@ -197,6 +197,7 @@ 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 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 b59ce9a..b1b0216 100644
> > > --- a/drivers/usb/musb/musb_host.c
> > > +++ b/drivers/usb/musb/musb_host.c
> > > @@ -292,20 +292,73 @@ static inline void musb_save_toggle(struct musb_qh *qh, int is_in,
> > >  {
> > >  	void __iomem		*epio = qh->hw_ep->regs;
> > >  	u16			csr;
> > > +	struct musb *musb = qh->hw_ep->musb;
> > >  
> > >  	/*
> > >  	 * 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;
> > > +	/* MediaTek controller has private toggle register */
> > 
> > only one toggle register for all endpoints? how does it handle
> > difference toggle values for different endpoints?
> 
> MediaTek controller has separate registers to describe TX/RX toggle.

Is it one register per endpoint?

> 
> > > +	if (musb->ops->quirks & MUSB_MTK_QUIRKS) {
> > > +		u16 toggle;
> > > +		u8 epnum = qh->hw_ep->epnum;
> > > +
> > > +		if (is_in)
> > > +			toggle = musb_readl(musb->mregs, MUSB_RXTOG);

this line seems telling there is just *one* register for all endpoints.

> > 
> > should use musb_readw() instead? MUSB_RXTOG seems to be 16bit.
> 
> Ok
> 
> > > +		else
> > > +			toggle = musb_readl(musb->mregs, MUSB_TXTOG);
> > > +
> > > +		csr = toggle & (1 << epnum);
> > > +	} else {
> > > +		if (is_in)
> > > +			csr = musb_readw(epio, MUSB_RXCSR)
> > > +				& MUSB_RXCSR_H_DATATOGGLE;
> > > +		else
> > > +			csr = musb_readw(epio, MUSB_TXCSR)
> > > +				& MUSB_TXCSR_H_DATATOGGLE;
> > 
> > Does this logic still work for the mtk platform even if it has its own
> > private toggle register? If so, we don't need to change here.
> 
> Sorry, this logic can not work on mtk platform, bit
> MUSB_RXCSR_H_DATATOGGLE and MUSB_TXCSR_H_DATATOGGLE are used for other
> function.

Is there a different controller RTL version we can use to
differentiate?

> 
> > If not, let's try to not use this quirk flag. Please create a hook
> > musb_platform_get_toggle() in struct musb_platform_ops.
> 
> Does the method of implement musb_platform_get_toggle() is prepare
> musb_default_get_toggle with common function, then follow how
> musb_readb() pointer is assigned in musb_init_controller()?

Yes, similar to musb_readb() implementation.

> How about creating musb_platform_get_toggle() as following:
> u16 (*get_toggle)(struct musb* musb, struct musb_qh *qh, int is_in);

yes, it is part of the implementation, then add it in struct musb_io.

> 
> > > +	}
> > >  
> > >  	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;
> > > +	struct musb *musb = qh->hw_ep->musb;
> > > +	u8 epnum = qh->hw_ep->epnum;
> > > +
> > > +	toggle = usb_gettoggle(urb->dev, qh->epnum, !is_in);
> > > +
> > > +	/* MediaTek controller has private toggle register */
> > > +	if (musb->ops->quirks & MUSB_MTK_QUIRKS) {
> > > +		if (is_in) {
> > > +			musb_writel(musb->mregs, MUSB_RXTOGEN, (1 << epnum));
> > > +			musb_writel(musb->mregs, MUSB_RXTOG, (toggle << epnum));
> > > +		} else {
> > > +			musb_writel(musb->mregs, MUSB_TXTOGEN, (1 << epnum));
> > > +			musb_writel(musb->mregs, MUSB_TXTOG, (toggle << epnum));
> > > +		}
> > > +	} else {
> > > +		if (is_in) {
> > > +			if (toggle)
> > > +				csr = MUSB_RXCSR_H_WR_DATATOGGLE
> > > +						| MUSB_RXCSR_H_DATATOGGLE;
> > > +			else
> > > +				csr = 0;
> > > +		} else {
> > > +			if (toggle)
> > > +				csr |= MUSB_TXCSR_H_WR_DATATOGGLE
> > > +						| MUSB_TXCSR_H_DATATOGGLE;
> > > +			else
> > > +				csr |= MUSB_TXCSR_CLRDATATOG;

'? :' operation probably is better than 'if else' here.

> > > +		}
> > > +	}
> > > +	return csr;
> > 
> > Please create a seperate patch for this musb_set_toggle() without adding
> > the mtk logic. It is a nice cleanup.
> 
> Does this like get toggle implementation, create a hook
> musb_platform_set_toggle() in struct musb_platform_ops?

You did the code cleanup by creating musb_set_toggle(), please make it
in a separate patch, like

static u16 musb_set_toggle(struct musb_qh *qh, int is_in, struct urb *urb)
{
	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;
}

/* use musb_set_toggle() in the two instances */

then in this patch you add the mtk implementation similar as
*get_toggle() discussed above.

> 
> > > +}
> > > +
> > >  /*
> > >   * Advance this hardware endpoint's queue, completing the specified URB and
> > >   * advancing to either the next URB queued to that qh, or else invalidating
> > > @@ -772,13 +825,8 @@ static void musb_ep_program(struct musb *musb, u8 epnum,
> > >  					);
> > >  			csr |= MUSB_TXCSR_MODE;
> > >  
> > > -			if (!hw_ep->tx_double_buffered) {
> > > -				if (usb_gettoggle(urb->dev, qh->epnum, 1))
> > > -					csr |= MUSB_TXCSR_H_WR_DATATOGGLE
> > > -						| MUSB_TXCSR_H_DATATOGGLE;
> > > -				else
> > > -					csr |= MUSB_TXCSR_CLRDATATOG;
> > > -			}
> > > +			if (!hw_ep->tx_double_buffered)
> > > +				csr |= musb_set_toggle(qh, !is_out, urb);
> > >  
> > >  			musb_writew(epio, MUSB_TXCSR, csr);
> > >  			/* REVISIT may need to clear FLUSHFIFO ... */
> > > @@ -860,17 +908,12 @@ static void musb_ep_program(struct musb *musb, u8 epnum,
> > >  
> > >  	/* IN/receive */
> > >  	} else {
> > > -		u16	csr;
> > > +		u16	csr = 0;
> > >  
> > >  		if (hw_ep->rx_reinit) {
> > >  			musb_rx_reinit(musb, qh, epnum);
> > > +			csr |= musb_set_toggle(qh, !is_out, urb);
> > >  
> > > -			/* init new state: toggle and NYET, maybe DMA later */
> > > -			if (usb_gettoggle(urb->dev, qh->epnum, 0))
> > > -				csr = MUSB_RXCSR_H_WR_DATATOGGLE
> > > -					| MUSB_RXCSR_H_DATATOGGLE;
> > > -			else
> > > -				csr = 0;
> > >  			if (qh->type == USB_ENDPOINT_XFER_INT)
> > >  				csr |= MUSB_RXCSR_DISNYET;
> > >  
> > > diff --git a/drivers/usb/musb/musb_regs.h b/drivers/usb/musb/musb_regs.h
> > > index 5cd7264..ffbe267 100644
> > > --- a/drivers/usb/musb/musb_regs.h
> > > +++ b/drivers/usb/musb/musb_regs.h
> > > @@ -273,6 +273,12 @@
> > >  #define MUSB_RXHUBADDR		0x06
> > >  #define MUSB_RXHUBPORT		0x07
> > >  
> > > +/* MediaTek controller toggle enable and status reg */
> > > +#define MUSB_RXTOG		0x80
> > > +#define MUSB_RXTOGEN		0x82
> > > +#define MUSB_TXTOG		0x84
> > > +#define MUSB_TXTOGEN		0x86
> > 
> > Again, these offsets are for different registers in the MUSB version I
> > have, please let me know if you have different version of the MUSB IP.
> 
> Sorry, these are MediaTek controller private registers used for control
> toggle.

Okay. Once the platform get/set_toggle() are implemented, those
registers can be defined in the mtk glue driver instead of here.

Regards,
-Bin.

Re: [PATCH 4/4] usb: musb: Add support for MediaTek musb controller

[Min Guo]

Hi Bin,

On Wed, 2019-01-09 at 08:01 -0600, Bin Liu wrote:
> Hi Min,
> 
> On Wed, Jan 09, 2019 at 08:31:08PM +0800, Min Guo wrote:
> > Hi Bin,
> > On Tue, 2019-01-08 at 09:44 -0600, Bin Liu wrote:
> > > Hi,
> > > 
> > > On Thu, Dec 27, 2018 at 03:34:26PM +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
> > > > 
> > > > 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  | 562 +++++++++++++++++++++++++++++++++++++++++++
> > > >  drivers/usb/musb/musb_core.c |  10 +
> > > >  drivers/usb/musb/musb_core.h |   1 +
> > > >  drivers/usb/musb/musb_dma.h  |   1 +
> > > >  drivers/usb/musb/musb_host.c |  79 ++++--
> > > >  drivers/usb/musb/musb_regs.h |   6 +
> > > >  drivers/usb/musb/musbhsdma.c |  34 ++-
> > > >  9 files changed, 671 insertions(+), 31 deletions(-)
> > > >  create mode 100644 drivers/usb/musb/mediatek.c
> > > > 
> > > > diff --git a/drivers/usb/musb/Kconfig b/drivers/usb/musb/Kconfig
> > > > index ad08895..540fc9f 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
> > > 
> > > Please also add '|| COMPILE_TEST' to make testing easier.
> > 
> > Ok
> > 
> > > > +	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..15a6460
> > > > --- /dev/null
> > > > +++ b/drivers/usb/musb/mediatek.c
> > > 
> > > [snip]
> > > I will review this section later after we sorted out other things.
> > > 
> > > > diff --git a/drivers/usb/musb/musb_core.c b/drivers/usb/musb/musb_core.c
> > > > index b7d5627..d60f76f 100644
> > > > --- a/drivers/usb/musb/musb_core.c
> > > > +++ b/drivers/usb/musb/musb_core.c
> > > > @@ -1028,6 +1028,16 @@ 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);
> > > > +
> > > > +	/*  MediaTek controller interrupt status is W1C */
> > > 
> > > This W1C doesn't match to the MUSB Programming Guide that I have. Those
> > > registers are read-only.
> > > Is the difference due to the IP intergration in the mtk platforms? or is
> > > it a new version of the MUSB controller? If latter, what is the version?
> > 
> > This is difference due to the IP intergration in mtk platforms. W1C is
> > easy for CpdeViser debug.
> > 
> > > > +	if (musb->ops->quirks & MUSB_MTK_QUIRKS) {
> > > 
> > > Basically we don't want to use this type of platform specific quirks if
> > > possible, so let's try to not use it.
> > 
> > I will try my best to avoid using it.
> 
> Thanks.
> 
> > 
> > > > +		musb_writeb(mbase, MUSB_INTRUSB,
> > > > +			musb_readb(mbase, MUSB_INTRUSB));
> > > 
> > > For this clearing register bit operation, please create platform hooks
> > > musb_clearb() and musb_clearw() in struct musb_platform_ops instead,
> > > then follow how musb_readb() pointer is assigned in
> > > musb_init_controller() to use the W1C version for mtk platform.
> > 
> > I have tried implementing musb_readb(), musb_readw() interface with
> > interrupt status W1C function in struct musb_platform_ops. But this
> > interface will require a global variable to hold MAC basic address for
> > judgment, and then special handling of the interrupt state. A global
> > variable will make the driver work with only a single instance, so it
> > can't work on some MTK platforms which have two instances.
> 
> I didn't mean to modify musb_read*(), but
> 
> > How about creating musb_clearb/w() as following:
> > void (*clearb)(void __iomem *addr, unsigned offset, u8 data);
> > void (*clearw)(void __iomem *addr, unsigned offset, u16 data);
> 
> this is what I was asking for, similar to what musb_readb/w() is
> implemented.

I will prepare a patch for musb_clearb/w().

> > 
> > 
> > > > +		musb_writew(mbase, MUSB_INTRRX,
> > > > +			musb_readw(mbase, MUSB_INTRRX));
> > > > +		musb_writew(mbase, MUSB_INTRTX,
> > > > +			musb_readw(mbase, MUSB_INTRTX));
> > > > +	}
> > > >  }
> > > >  
> > > >  static void musb_enable_interrupts(struct musb *musb)
> > > > diff --git a/drivers/usb/musb/musb_core.h b/drivers/usb/musb/musb_core.h
> > > > index 04203b7..1bf4e9a 100644
> > > > --- a/drivers/usb/musb/musb_core.h
> > > > +++ b/drivers/usb/musb/musb_core.h
> > > > @@ -138,6 +138,7 @@ enum musb_g_ep0_state {
> > > >   */
> > > >  struct musb_platform_ops {
> > > >  
> > > > +#define MUSB_MTK_QUIRKS	BIT(10)
> > > >  #define MUSB_G_NO_SKB_RESERVE	BIT(9)
> > > >  #define MUSB_DA8XX		BIT(8)
> > > >  #define MUSB_PRESERVE_SESSION	BIT(7)
> > > > diff --git a/drivers/usb/musb/musb_dma.h b/drivers/usb/musb/musb_dma.h
> > > > index 281e75d3..b218210 100644
> > > > --- a/drivers/usb/musb/musb_dma.h
> > > > +++ b/drivers/usb/musb/musb_dma.h
> > > > @@ -197,6 +197,7 @@ 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 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 b59ce9a..b1b0216 100644
> > > > --- a/drivers/usb/musb/musb_host.c
> > > > +++ b/drivers/usb/musb/musb_host.c
> > > > @@ -292,20 +292,73 @@ static inline void musb_save_toggle(struct musb_qh *qh, int is_in,
> > > >  {
> > > >  	void __iomem		*epio = qh->hw_ep->regs;
> > > >  	u16			csr;
> > > > +	struct musb *musb = qh->hw_ep->musb;
> > > >  
> > > >  	/*
> > > >  	 * 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;
> > > > +	/* MediaTek controller has private toggle register */
> > > 
> > > only one toggle register for all endpoints? how does it handle
> > > difference toggle values for different endpoints?
> > 
> > MediaTek controller has separate registers to describe TX/RX toggle.
> 
> Is it one register per endpoint?

MUSB_RXTOG/MUSB_TXTOG is common register, each bit reflects the toggle
state of an endpoint. bit[0] not used,bit[1~8] corresponds to ep[1~8]

> > 
> > > > +	if (musb->ops->quirks & MUSB_MTK_QUIRKS) {
> > > > +		u16 toggle;
> > > > +		u8 epnum = qh->hw_ep->epnum;
> > > > +
> > > > +		if (is_in)
> > > > +			toggle = musb_readl(musb->mregs, MUSB_RXTOG);
> 
> this line seems telling there is just *one* register for all endpoints.

Yes, all endpoint share this register, endpoint and bit are one-to-one
correspondence.

> > > 
> > > should use musb_readw() instead? MUSB_RXTOG seems to be 16bit.
> > 
> > Ok
> > 
> > > > +		else
> > > > +			toggle = musb_readl(musb->mregs, MUSB_TXTOG);
> > > > +
> > > > +		csr = toggle & (1 << epnum);
> > > > +	} else {
> > > > +		if (is_in)
> > > > +			csr = musb_readw(epio, MUSB_RXCSR)
> > > > +				& MUSB_RXCSR_H_DATATOGGLE;
> > > > +		else
> > > > +			csr = musb_readw(epio, MUSB_TXCSR)
> > > > +				& MUSB_TXCSR_H_DATATOGGLE;
> > > 
> > > Does this logic still work for the mtk platform even if it has its own
> > > private toggle register? If so, we don't need to change here.
> > 
> > Sorry, this logic can not work on mtk platform, bit
> > MUSB_RXCSR_H_DATATOGGLE and MUSB_TXCSR_H_DATATOGGLE are used for other
> > function.
> 
> Is there a different controller RTL version we can use to
> differentiate?

Sorry, there is no controller RTL version can be used to indicate the
differences.

> > 
> > > If not, let's try to not use this quirk flag. Please create a hook
> > > musb_platform_get_toggle() in struct musb_platform_ops.
> > 
> > Does the method of implement musb_platform_get_toggle() is prepare
> > musb_default_get_toggle with common function, then follow how
> > musb_readb() pointer is assigned in musb_init_controller()?
> 
> Yes, similar to musb_readb() implementation.

OK.

> > How about creating musb_platform_get_toggle() as following:
> > u16 (*get_toggle)(struct musb* musb, struct musb_qh *qh, int is_in);
> 
> yes, it is part of the implementation, then add it in struct musb_io.

I will prepare a patch for (*get_toggle).

> > 
> > > > +	}
> > > >  
> > > >  	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;
> > > > +	struct musb *musb = qh->hw_ep->musb;
> > > > +	u8 epnum = qh->hw_ep->epnum;
> > > > +
> > > > +	toggle = usb_gettoggle(urb->dev, qh->epnum, !is_in);
> > > > +
> > > > +	/* MediaTek controller has private toggle register */
> > > > +	if (musb->ops->quirks & MUSB_MTK_QUIRKS) {
> > > > +		if (is_in) {
> > > > +			musb_writel(musb->mregs, MUSB_RXTOGEN, (1 << epnum));
> > > > +			musb_writel(musb->mregs, MUSB_RXTOG, (toggle << epnum));
> > > > +		} else {
> > > > +			musb_writel(musb->mregs, MUSB_TXTOGEN, (1 << epnum));
> > > > +			musb_writel(musb->mregs, MUSB_TXTOG, (toggle << epnum));
> > > > +		}
> > > > +	} else {
> > > > +		if (is_in) {
> > > > +			if (toggle)
> > > > +				csr = MUSB_RXCSR_H_WR_DATATOGGLE
> > > > +						| MUSB_RXCSR_H_DATATOGGLE;
> > > > +			else
> > > > +				csr = 0;
> > > > +		} else {
> > > > +			if (toggle)
> > > > +				csr |= MUSB_TXCSR_H_WR_DATATOGGLE
> > > > +						| MUSB_TXCSR_H_DATATOGGLE;
> > > > +			else
> > > > +				csr |= MUSB_TXCSR_CLRDATATOG;
> 
> '? :' operation probably is better than 'if else' here.

OK.

> > > > +		}
> > > > +	}
> > > > +	return csr;
> > > 
> > > Please create a seperate patch for this musb_set_toggle() without adding
> > > the mtk logic. It is a nice cleanup.
> > 
> > Does this like get toggle implementation, create a hook
> > musb_platform_set_toggle() in struct musb_platform_ops?
> 
> You did the code cleanup by creating musb_set_toggle(), please make it
> in a separate patch, like
> 
> static u16 musb_set_toggle(struct musb_qh *qh, int is_in, struct urb *urb)
> {
> 	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;
> }
> 
> /* use musb_set_toggle() in the two instances */
> 
> then in this patch you add the mtk implementation similar as
> *get_toggle() discussed above.

OK.

> > 
> > > > +}
> > > > +
> > > >  /*
> > > >   * Advance this hardware endpoint's queue, completing the specified URB and
> > > >   * advancing to either the next URB queued to that qh, or else invalidating
> > > > @@ -772,13 +825,8 @@ static void musb_ep_program(struct musb *musb, u8 epnum,
> > > >  					);
> > > >  			csr |= MUSB_TXCSR_MODE;
> > > >  
> > > > -			if (!hw_ep->tx_double_buffered) {
> > > > -				if (usb_gettoggle(urb->dev, qh->epnum, 1))
> > > > -					csr |= MUSB_TXCSR_H_WR_DATATOGGLE
> > > > -						| MUSB_TXCSR_H_DATATOGGLE;
> > > > -				else
> > > > -					csr |= MUSB_TXCSR_CLRDATATOG;
> > > > -			}
> > > > +			if (!hw_ep->tx_double_buffered)
> > > > +				csr |= musb_set_toggle(qh, !is_out, urb);
> > > >  
> > > >  			musb_writew(epio, MUSB_TXCSR, csr);
> > > >  			/* REVISIT may need to clear FLUSHFIFO ... */
> > > > @@ -860,17 +908,12 @@ static void musb_ep_program(struct musb *musb, u8 epnum,
> > > >  
> > > >  	/* IN/receive */
> > > >  	} else {
> > > > -		u16	csr;
> > > > +		u16	csr = 0;
> > > >  
> > > >  		if (hw_ep->rx_reinit) {
> > > >  			musb_rx_reinit(musb, qh, epnum);
> > > > +			csr |= musb_set_toggle(qh, !is_out, urb);
> > > >  
> > > > -			/* init new state: toggle and NYET, maybe DMA later */
> > > > -			if (usb_gettoggle(urb->dev, qh->epnum, 0))
> > > > -				csr = MUSB_RXCSR_H_WR_DATATOGGLE
> > > > -					| MUSB_RXCSR_H_DATATOGGLE;
> > > > -			else
> > > > -				csr = 0;
> > > >  			if (qh->type == USB_ENDPOINT_XFER_INT)
> > > >  				csr |= MUSB_RXCSR_DISNYET;
> > > >  
> > > > diff --git a/drivers/usb/musb/musb_regs.h b/drivers/usb/musb/musb_regs.h
> > > > index 5cd7264..ffbe267 100644
> > > > --- a/drivers/usb/musb/musb_regs.h
> > > > +++ b/drivers/usb/musb/musb_regs.h
> > > > @@ -273,6 +273,12 @@
> > > >  #define MUSB_RXHUBADDR		0x06
> > > >  #define MUSB_RXHUBPORT		0x07
> > > >  
> > > > +/* MediaTek controller toggle enable and status reg */
> > > > +#define MUSB_RXTOG		0x80
> > > > +#define MUSB_RXTOGEN		0x82
> > > > +#define MUSB_TXTOG		0x84
> > > > +#define MUSB_TXTOGEN		0x86
> > > 
> > > Again, these offsets are for different registers in the MUSB version I
> > > have, please let me know if you have different version of the MUSB IP.
> > 
> > Sorry, these are MediaTek controller private registers used for control
> > toggle.
> 
> Okay. Once the platform get/set_toggle() are implemented, those
> registers can be defined in the mtk glue driver instead of here.

OK.

> Regards,
> -Bin.

Re: [PATCH 4/4] usb: musb: Add support for MediaTek musb controller

[Bin Liu]

Hi Min,

Please briefly summarize the controller differences in the commit log,
such as

- WIC interrupt registers;
- data toggle bit;
- no dedicated DMA interrupt line;

so that we can quickly understand the core driver is modified
accordingly to handle the differences.

On Thu, Jan 10, 2019 at 03:24:22PM +0800, Min Guo wrote:
> Hi Bin,

[snip]

> > > > > +		musb_writeb(mbase, MUSB_INTRUSB,
> > > > > +			musb_readb(mbase, MUSB_INTRUSB));
> > > > 
> > > > For this clearing register bit operation, please create platform hooks
> > > > musb_clearb() and musb_clearw() in struct musb_platform_ops instead,
> > > > then follow how musb_readb() pointer is assigned in
> > > > musb_init_controller() to use the W1C version for mtk platform.
> > > 
> > > I have tried implementing musb_readb(), musb_readw() interface with
> > > interrupt status W1C function in struct musb_platform_ops. But this
> > > interface will require a global variable to hold MAC basic address for
> > > judgment, and then special handling of the interrupt state. A global
> > > variable will make the driver work with only a single instance, so it
> > > can't work on some MTK platforms which have two instances.
> > 
> > I didn't mean to modify musb_read*(), but
> > 
> > > How about creating musb_clearb/w() as following:
> > > void (*clearb)(void __iomem *addr, unsigned offset, u8 data);
> > > void (*clearw)(void __iomem *addr, unsigned offset, u16 data);
> > 
> > this is what I was asking for, similar to what musb_readb/w() is
> > implemented.
> 
> I will prepare a patch for musb_clearb/w().

This doesn't have to be a separate patch.

> > > > > +		musb_writew(mbase, MUSB_INTRRX,
> > > > > +			musb_readw(mbase, MUSB_INTRRX));
> > > > > +		musb_writew(mbase, MUSB_INTRTX,
> > > > > +			musb_readw(mbase, MUSB_INTRTX));
> > > > > +	}

[snip]

> > > > > +	/* MediaTek controller has private toggle register */
> > > > 
> > > > only one toggle register for all endpoints? how does it handle
> > > > difference toggle values for different endpoints?
> > > 
> > > MediaTek controller has separate registers to describe TX/RX toggle.
> > 
> > Is it one register per endpoint?
> 
> MUSB_RXTOG/MUSB_TXTOG is common register, each bit reflects the toggle
> state of an endpoint. bit[0] not used,bit[1~8] corresponds to ep[1~8]
> 
> > > 
> > > > > +	if (musb->ops->quirks & MUSB_MTK_QUIRKS) {
> > > > > +		u16 toggle;
> > > > > +		u8 epnum = qh->hw_ep->epnum;
> > > > > +
> > > > > +		if (is_in)
> > > > > +			toggle = musb_readl(musb->mregs, MUSB_RXTOG);
> > 
> > this line seems telling there is just *one* register for all endpoints.
> 
> Yes, all endpoint share this register, endpoint and bit are one-to-one
> correspondence.

Okay, thanks. Sorry I missed the bit operation in the code below.

> 
> > > > 
> > > > should use musb_readw() instead? MUSB_RXTOG seems to be 16bit.
> > > 
> > > Ok
> > > 
> > > > > +		else
> > > > > +			toggle = musb_readl(musb->mregs, MUSB_TXTOG);
> > > > > +
> > > > > +		csr = toggle & (1 << epnum);

Regards,
-Bin.

Re: [PATCH 4/4] usb: musb: Add support for MediaTek musb controller

[Min Guo]

Hi Bin,

On Thu, 2019-01-10 at 08:18 -0600, Bin Liu wrote:
> Hi Min,
> 
> Please briefly summarize the controller differences in the commit log,
> such as
> 
> - WIC interrupt registers;
> - data toggle bit;
> - no dedicated DMA interrupt line;
> 
> so that we can quickly understand the core driver is modified
> accordingly to handle the differences.

Okay, tkanks.

> On Thu, Jan 10, 2019 at 03:24:22PM +0800, Min Guo wrote:
> > Hi Bin,
> 
> [snip]
> 
> > > > > > +		musb_writeb(mbase, MUSB_INTRUSB,
> > > > > > +			musb_readb(mbase, MUSB_INTRUSB));
> > > > > 
> > > > > For this clearing register bit operation, please create platform hooks
> > > > > musb_clearb() and musb_clearw() in struct musb_platform_ops instead,
> > > > > then follow how musb_readb() pointer is assigned in
> > > > > musb_init_controller() to use the W1C version for mtk platform.
> > > > 
> > > > I have tried implementing musb_readb(), musb_readw() interface with
> > > > interrupt status W1C function in struct musb_platform_ops. But this
> > > > interface will require a global variable to hold MAC basic address for
> > > > judgment, and then special handling of the interrupt state. A global
> > > > variable will make the driver work with only a single instance, so it
> > > > can't work on some MTK platforms which have two instances.
> > > 
> > > I didn't mean to modify musb_read*(), but
> > > 
> > > > How about creating musb_clearb/w() as following:
> > > > void (*clearb)(void __iomem *addr, unsigned offset, u8 data);
> > > > void (*clearw)(void __iomem *addr, unsigned offset, u16 data);
> > > 
> > > this is what I was asking for, similar to what musb_readb/w() is
> > > implemented.
> > 
> > I will prepare a patch for musb_clearb/w().
> 
> This doesn't have to be a separate patch.

Okay.

> > > > > > +		musb_writew(mbase, MUSB_INTRRX,
> > > > > > +			musb_readw(mbase, MUSB_INTRRX));
> > > > > > +		musb_writew(mbase, MUSB_INTRTX,
> > > > > > +			musb_readw(mbase, MUSB_INTRTX));
> > > > > > +	}
> 
> [snip]
> 
> > > > > > +	/* MediaTek controller has private toggle register */
> > > > > 
> > > > > only one toggle register for all endpoints? how does it handle
> > > > > difference toggle values for different endpoints?
> > > > 
> > > > MediaTek controller has separate registers to describe TX/RX toggle.
> > > 
> > > Is it one register per endpoint?
> > 
> > MUSB_RXTOG/MUSB_TXTOG is common register, each bit reflects the toggle
> > state of an endpoint. bit[0] not used,bit[1~8] corresponds to ep[1~8]
> > 
> > > > 
> > > > > > +	if (musb->ops->quirks & MUSB_MTK_QUIRKS) {
> > > > > > +		u16 toggle;
> > > > > > +		u8 epnum = qh->hw_ep->epnum;
> > > > > > +
> > > > > > +		if (is_in)
> > > > > > +			toggle = musb_readl(musb->mregs, MUSB_RXTOG);
> > > 
> > > this line seems telling there is just *one* register for all endpoints.
> > 
> > Yes, all endpoint share this register, endpoint and bit are one-to-one
> > correspondence.
> 
> Okay, thanks. Sorry I missed the bit operation in the code below.
> 
> > 
> > > > > 
> > > > > should use musb_readw() instead? MUSB_RXTOG seems to be 16bit.
> > > > 
> > > > Ok
> > > > 
> > > > > > +		else
> > > > > > +			toggle = musb_readl(musb->mregs, MUSB_TXTOG);
> > > > > > +
> > > > > > +		csr = toggle & (1 << epnum);
> 
> Regards,
> -Bin.

Re: [PATCH 4/4] usb: musb: Add support for MediaTek musb controller

[Min Guo]

Hi Bin,

I have some questions about
musbhs_dma_controller_destroy/create_noirq().
1,Because of controller->irq=0 in noirq case, destroy_noirq can reuse
musbhs_dma_controller_destroy. Is it necessary to write a destroy_noirq
function?
2, How about creating a common function for create musb dma controller
as following:
static struct musb_dma_controller *dma_controller_alloc(struct musb
*musb, void __iomem *base)
then musbhs_dma_controller_create() and
musbhs_dma_controller_create_noirq() call it to alloc common resource.


On Tue, 2019-01-08 at 09:44 -0600, Bin Liu wrote:
> Hi,
> 
> On Thu, Dec 27, 2018 at 03:34:26PM +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
> > 

> > diff --git a/drivers/usb/musb/musbhsdma.c b/drivers/usb/musb/musbhsdma.c
> > index 824adcb..c545475 100644
> > --- a/drivers/usb/musb/musbhsdma.c
> > +++ b/drivers/usb/musb/musbhsdma.c

> >  
> >  void musbhs_dma_controller_destroy(struct dma_controller *c)
> >  {
> >  	struct musb_dma_controller *controller = container_of(c,
> >  			struct musb_dma_controller, controller);
> > +	struct musb *musb = controller->private_data;
> >  
> >  	dma_controller_stop(controller);
> >  
> > -	if (controller->irq)
> > +	if (!(musb->ops->quirks & MUSB_MTK_QUIRKS) && controller->irq)
> >  		free_irq(controller->irq, c);
> >  
> >  	kfree(controller);
> > @@ -398,11 +402,15 @@ struct dma_controller *musbhs_dma_controller_create(struct musb *musb,
> >  	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");
> > +	int irq = -1;
> >  
> > -	if (irq <= 0) {
> > -		dev_err(dev, "No DMA interrupt line!\n");
> > -		return NULL;
> > +	if (!(musb->ops->quirks & MUSB_MTK_QUIRKS)) {
> > +		irq = platform_get_irq_byname(pdev, "dma");
> > +
> > +		if (irq < 0) {
> > +			dev_err(dev, "No DMA interrupt line!\n");
> > +			return NULL;
> > +		}
> >  	}
> 
> Please create musbhs_dma_controller_destroy_noirq() for your platform to
> not use the quirk.
> 
> >  
> >  	controller = kzalloc(sizeof(*controller), GFP_KERNEL);
> > @@ -418,15 +426,17 @@ struct dma_controller *musbhs_dma_controller_create(struct musb *musb,
> >  	controller->controller.channel_program = dma_channel_program;
> >  	controller->controller.channel_abort = dma_channel_abort;
> >  
> > -	if (request_irq(irq, dma_controller_irq, 0,
> > +	if (!(musb->ops->quirks & MUSB_MTK_QUIRKS)) {
> > +		if (request_irq(irq, dma_controller_irq, 0,
> >  			dev_name(musb->controller), &controller->controller)) {
> > -		dev_err(dev, "request_irq %d failed!\n", irq);
> > -		musb_dma_controller_destroy(&controller->controller);
> > +			dev_err(dev, "request_irq %d failed!\n", irq);
> > +			musb_dma_controller_destroy(&controller->controller);
> >  
> > -		return NULL;
> > -	}
> > +			return NULL;
> > +		}
> >  
> > -	controller->irq = irq;
> > +		controller->irq = irq;
> > +	}
> >  
> >  	return &controller->controller;
> >  }
> 
> Same here, create musbhs_dma_controller_create_noirq(). Then use both
> new API for the mtk glue driver.



> Regards,
> -Bin.