[PATCH v4 0/4] add usb support for hi6220

[PATCH v4 0/4] add usb support for hi6220

[Zhangfei Gao]

v4:
Move drivers/usb/phy/phy-hi6220-usb.c to drivers/phy/phy-hi6220-usb.c, required by Balbi.
Modify dt bindings per comments from Mark and Sergei

v3:
fix typo and add -EPROBE_DEFER of regulator, pointed by Peter

v2:
address comments from Sergei and Peter
add hi6220_phy_setup(false) code

v1:
hi6220 usb controller is inherited from dwc2
add phy accordingly
support otg gadget/host

Zhangfei Gao (4):
  Documentation: dt-bindings: add dt binding info for hi6220 dwc2
  Documentation: dt-bindings: add dt binding info for hi6220
  usb: dwc2: platform: add hi6220 support
  phy: add phy-hi6220-usb

 Documentation/devicetree/bindings/usb/dwc2.txt     |   1 +
 .../devicetree/bindings/usb/hi6220-usb.txt         |  49 ++++
 drivers/phy/Kconfig                                |   9 +
 drivers/phy/Makefile                               |   1 +
 drivers/phy/phy-hi6220-usb.c                       | 306 +++++++++++++++++++++
 drivers/usb/dwc2/platform.c                        |  30 ++
 6 files changed, 396 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/usb/hi6220-usb.txt
 create mode 100644 drivers/phy/phy-hi6220-usb.c

-- 
1.9.1

[PATCH v4 1/4] Documentation: dt-bindings: add dt binding info for hi6220 dwc2

[Zhangfei Gao]

Add necessary dwc2 binding documentation for Hisilicon soc: hi6220

Signed-off-by: Zhangfei Gao <zhangfei.gao@linaro.org>
---
 Documentation/devicetree/bindings/usb/dwc2.txt | 1 +
 1 file changed, 1 insertion(+)

diff --git a/Documentation/devicetree/bindings/usb/dwc2.txt b/Documentation/devicetree/bindings/usb/dwc2.txt
index fd132cb..2213682 100644
--- a/Documentation/devicetree/bindings/usb/dwc2.txt
+++ b/Documentation/devicetree/bindings/usb/dwc2.txt
@@ -4,6 +4,7 @@ Platform DesignWare HS OTG USB 2.0 controller
 Required properties:
 - compatible : One of:
   - brcm,bcm2835-usb: The DWC2 USB controller instance in the BCM2835 SoC.
+  - hisilicon,hi6220-usb: The DWC2 USB controller instance in the hi6220 SoC.
   - rockchip,rk3066-usb: The DWC2 USB controller instance in the rk3066 Soc;
   - "rockchip,rk3188-usb", "rockchip,rk3066-usb", "snps,dwc2": for rk3188 Soc;
   - "rockchip,rk3288-usb", "rockchip,rk3066-usb", "snps,dwc2": for rk3288 Soc;
-- 
1.9.1

[PATCH v4 2/4] Documentation: dt-bindings: add dt binding info for hi6220

[Zhangfei Gao]

Signed-off-by: Zhangfei Gao <zhangfei.gao@linaro.org>
---
 .../devicetree/bindings/usb/hi6220-usb.txt         | 49 ++++++++++++++++++++++
 1 file changed, 49 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/usb/hi6220-usb.txt

diff --git a/Documentation/devicetree/bindings/usb/hi6220-usb.txt b/Documentation/devicetree/bindings/usb/hi6220-usb.txt
new file mode 100644
index 0000000..b3a7b5a
--- /dev/null
+++ b/Documentation/devicetree/bindings/usb/hi6220-usb.txt
@@ -0,0 +1,49 @@
+Hisilicon hi6220 SoC USB controller
+-----------------------------
+
+usb controller is inherited from dwc2, refer dwc2.txt
+-----
+
+Required properties:
+- compatible: "hisilicon,hi6220-usb"
+Refer to dwc2.txt for dwc2 usb properties
+
+
+PHY:
+-----
+
+Required properties:
+- compatible: "hisilicon,hi6220-usb-phy"
+- vcc-supply: phandle to the regulator that provides power to the PHY.
+- clocks: phandle and clock specifier of the PHY clock.
+- hisilicon,peripheral-syscon: phandle of syscon used to control peripheral.
+- hisilicon,vbus-gpios: gpio of detecting vbus.
+- hisilicon,id-gpios: gpio of detecting id.
+
+Example:
+
+	sys_ctrl: syscon at f7030000 {
+		compatible = "hisilicon,sysctrl", "syscon";
+	        reg = <0x0 0xf7030000 0x0 0x1000>;
+	};
+
+	usb_phy: usb-phy {
+		compatible = "hisilicon,hi6220-usb-phy";
+		vcc-supply = <&fixed_5v_hub>;
+		hisilicon,vbus-gpios = <&gpio2 6 0>;
+		hisilicon,id-gpios = <&gpio2 5 0>;
+		hisilicon,peripheral-syscon = <&sys_ctrl>;
+		clocks = <&clock_sys HI6220_USBOTG_HCLK>;
+	};
+
+	usb: usb at f72c0000 {
+		compatible = "hisilicon,hi6220-usb";
+		reg = <0x0 0xf72c0000 0x0 0x40000>;
+		phys = <&usb_phy>;
+		dr_mode = "otg";
+		g-use-dma;
+		g-rx-fifo-size = <512>;
+		g-np-tx-fifo-size = <128>;
+		g-tx-fifo-size = <128>;
+		interrupts = <0 77 0x4>;
+	};
-- 
1.9.1

[PATCH v4 3/4] usb: dwc2: platform: add hi6220 support

[Zhangfei Gao]

Signed-off-by: Zhangfei Gao <zhangfei.gao@linaro.org>
---
 drivers/usb/dwc2/platform.c | 30 ++++++++++++++++++++++++++++++
 1 file changed, 30 insertions(+)

diff --git a/drivers/usb/dwc2/platform.c b/drivers/usb/dwc2/platform.c
index ae095f0..f7c67db 100644
--- a/drivers/usb/dwc2/platform.c
+++ b/drivers/usb/dwc2/platform.c
@@ -50,6 +50,35 @@
 
 static const char dwc2_driver_name[] = "dwc2";
 
+static const struct dwc2_core_params params_hi6220 = {
+	.otg_cap			= 2,	/* No HNP/SRP capable */
+	.otg_ver			= 0,	/* 1.3 */
+	.dma_enable			= 1,
+	.dma_desc_enable		= 0,
+	.speed				= 0,	/* High Speed */
+	.enable_dynamic_fifo		= 1,
+	.en_multiple_tx_fifo		= 1,
+	.host_rx_fifo_size		= 512,
+	.host_nperio_tx_fifo_size	= 512,
+	.host_perio_tx_fifo_size	= 512,
+	.max_transfer_size		= 65535,
+	.max_packet_count		= 511,
+	.host_channels			= 16,
+	.phy_type			= 1,	/* UTMI */
+	.phy_utmi_width			= 8,
+	.phy_ulpi_ddr			= 0,	/* Single */
+	.phy_ulpi_ext_vbus		= 0,
+	.i2c_enable			= 0,
+	.ulpi_fs_ls			= 0,
+	.host_support_fs_ls_low_power	= 0,
+	.host_ls_low_power_phy_clk	= 0,	/* 48 MHz */
+	.ts_dline			= 0,
+	.reload_ctl			= 0,
+	.ahbcfg				= GAHBCFG_HBSTLEN_INCR16 <<
+					  GAHBCFG_HBSTLEN_SHIFT,
+	.uframe_sched			= 0,
+};
+
 static const struct dwc2_core_params params_bcm2835 = {
 	.otg_cap			= 0,	/* HNP/SRP capable */
 	.otg_ver			= 0,	/* 1.3 */
@@ -129,6 +158,7 @@ static int dwc2_driver_remove(struct platform_device *dev)
 
 static const struct of_device_id dwc2_of_match_table[] = {
 	{ .compatible = "brcm,bcm2835-usb", .data = &params_bcm2835 },
+	{ .compatible = "hisilicon,hi6220-usb", .data = &params_hi6220 },
 	{ .compatible = "rockchip,rk3066-usb", .data = &params_rk3066 },
 	{ .compatible = "snps,dwc2", .data = NULL },
 	{ .compatible = "samsung,s3c6400-hsotg", .data = NULL},
-- 
1.9.1

[PATCH v4 4/4] phy: add phy-hi6220-usb

[Zhangfei Gao]

Add usb phy controller for hi6220 platform

Signed-off-by: Zhangfei Gao <zhangfei.gao@linaro.org>
---
 drivers/phy/Kconfig          |   9 ++
 drivers/phy/Makefile         |   1 +
 drivers/phy/phy-hi6220-usb.c | 306 +++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 316 insertions(+)
 create mode 100644 drivers/phy/phy-hi6220-usb.c

diff --git a/drivers/phy/Kconfig b/drivers/phy/Kconfig
index ccad880..40a1ef1 100644
--- a/drivers/phy/Kconfig
+++ b/drivers/phy/Kconfig
@@ -162,6 +162,15 @@ config PHY_HIX5HD2_SATA
 	help
 	  Support for SATA PHY on Hisilicon hix5hd2 Soc.
 
+config PHY_HI6220_USB
+	tristate "hi6220 USB PHY support"
+	select USB_PHY
+	select MFD_SYSCON
+	help
+	  Enable this to support the HISILICON HI6220 USB PHY.
+
+	  To compile this driver as a module, choose M here.
+
 config PHY_SUN4I_USB
 	tristate "Allwinner sunxi SoC USB PHY driver"
 	depends on ARCH_SUNXI && HAS_IOMEM && OF
diff --git a/drivers/phy/Makefile b/drivers/phy/Makefile
index aa74f96..ec43c2d 100644
--- a/drivers/phy/Makefile
+++ b/drivers/phy/Makefile
@@ -19,6 +19,7 @@ obj-$(CONFIG_TI_PIPE3)			+= phy-ti-pipe3.o
 obj-$(CONFIG_TWL4030_USB)		+= phy-twl4030-usb.o
 obj-$(CONFIG_PHY_EXYNOS5250_SATA)	+= phy-exynos5250-sata.o
 obj-$(CONFIG_PHY_HIX5HD2_SATA)		+= phy-hix5hd2-sata.o
+obj-$(CONFIG_PHY_HI6220_USB)		+= phy-hi6220-usb.o
 obj-$(CONFIG_PHY_SUN4I_USB)		+= phy-sun4i-usb.o
 obj-$(CONFIG_PHY_SAMSUNG_USB2)		+= phy-exynos-usb2.o
 phy-exynos-usb2-y			+= phy-samsung-usb2.o
diff --git a/drivers/phy/phy-hi6220-usb.c b/drivers/phy/phy-hi6220-usb.c
new file mode 100644
index 0000000..0d9f5ac
--- /dev/null
+++ b/drivers/phy/phy-hi6220-usb.c
@@ -0,0 +1,306 @@
+/*
+ * Copyright (c) 2015 Linaro Ltd.
+ * Copyright (c) 2015 Hisilicon Limited.
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ */
+
+#include <linux/clk.h>
+#include <linux/mfd/syscon.h>
+#include <linux/of_gpio.h>
+#include <linux/platform_device.h>
+#include <linux/regmap.h>
+#include <linux/regulator/consumer.h>
+#include <linux/usb/gadget.h>
+#include <linux/usb/otg.h>
+
+#define SC_PERIPH_CTRL4			0x00c
+
+#define CTRL4_PICO_SIDDQ		BIT(6)
+#define CTRL4_PICO_OGDISABLE		BIT(8)
+#define CTRL4_PICO_VBUSVLDEXT		BIT(10)
+#define CTRL4_PICO_VBUSVLDEXTSEL	BIT(11)
+#define CTRL4_OTG_PHY_SEL		BIT(21)
+
+#define SC_PERIPH_CTRL5			0x010
+
+#define CTRL5_USBOTG_RES_SEL		BIT(3)
+#define CTRL5_PICOPHY_ACAENB		BIT(4)
+#define CTRL5_PICOPHY_BC_MODE		BIT(5)
+#define CTRL5_PICOPHY_CHRGSEL		BIT(6)
+#define CTRL5_PICOPHY_VDATSRCEND	BIT(7)
+#define CTRL5_PICOPHY_VDATDETENB	BIT(8)
+#define CTRL5_PICOPHY_DCDENB		BIT(9)
+#define CTRL5_PICOPHY_IDDIG		BIT(10)
+
+#define SC_PERIPH_CTRL8			0x018
+#define SC_PERIPH_RSTEN0		0x300
+#define SC_PERIPH_RSTDIS0		0x304
+
+#define RST0_USBOTG_BUS			BIT(4)
+#define RST0_POR_PICOPHY		BIT(5)
+#define RST0_USBOTG			BIT(6)
+#define RST0_USBOTG_32K			BIT(7)
+
+#define EYE_PATTERN_PARA		0x7053348c
+
+struct hi6220_priv {
+	struct usb_phy phy;
+	struct delayed_work work;
+	struct regmap *reg;
+	struct clk *clk;
+	struct regulator *vcc;
+	struct device *dev;
+	int gpio_vbus;
+	int gpio_id;
+	enum usb_otg_state state;
+};
+
+static void hi6220_start_peripheral(struct hi6220_priv *priv, bool on)
+{
+	struct usb_otg *otg = priv->phy.otg;
+
+	if (!otg->gadget)
+		return;
+
+	if (on)
+		usb_gadget_connect(otg->gadget);
+	else
+		usb_gadget_disconnect(otg->gadget);
+}
+
+static void hi6220_detect_work(struct work_struct *work)
+{
+	struct hi6220_priv *priv =
+		container_of(work, struct hi6220_priv, work.work);
+	int gpio_id, gpio_vbus;
+	enum usb_otg_state state;
+
+	if (!gpio_is_valid(priv->gpio_id) || !gpio_is_valid(priv->gpio_vbus))
+		return;
+
+	gpio_id = gpio_get_value_cansleep(priv->gpio_id);
+	gpio_vbus = gpio_get_value_cansleep(priv->gpio_vbus);
+
+	if (gpio_vbus == 0) {
+		if (gpio_id == 1)
+			state = OTG_STATE_B_PERIPHERAL;
+		else
+			state = OTG_STATE_A_HOST;
+	} else {
+		state = OTG_STATE_A_HOST;
+	}
+
+	if (priv->state != state) {
+		hi6220_start_peripheral(priv, state == OTG_STATE_B_PERIPHERAL);
+		priv->state = state;
+	}
+}
+
+static irqreturn_t hiusb_gpio_intr(int irq, void *data)
+{
+	struct hi6220_priv *priv = (struct hi6220_priv *)data;
+
+	/* add debounce time */
+	schedule_delayed_work(&priv->work, msecs_to_jiffies(100));
+	return IRQ_HANDLED;
+}
+
+static int hi6220_set_peripheral(struct usb_otg *otg, struct usb_gadget *gadget)
+{
+	otg->gadget = gadget;
+	return 0;
+}
+
+static int hi6220_phy_setup(struct hi6220_priv *priv, bool on)
+{
+	struct regmap *reg = priv->reg;
+	u32 val, mask;
+	int ret;
+
+	if (priv->reg == NULL)
+		return 0;
+
+	if (on) {
+		val = RST0_USBOTG_BUS | RST0_POR_PICOPHY |
+		      RST0_USBOTG | RST0_USBOTG_32K;
+		mask = val;
+		ret = regmap_update_bits(reg, SC_PERIPH_RSTDIS0, mask, val);
+		if (ret)
+			goto out;
+
+		val = CTRL5_USBOTG_RES_SEL | CTRL5_PICOPHY_ACAENB;
+		mask = val | CTRL5_PICOPHY_BC_MODE;
+		ret = regmap_update_bits(reg, SC_PERIPH_CTRL5, mask, val);
+		if (ret)
+			goto out;
+
+		val =  CTRL4_PICO_VBUSVLDEXT | CTRL4_PICO_VBUSVLDEXTSEL |
+		       CTRL4_OTG_PHY_SEL;
+		mask = val | CTRL4_PICO_SIDDQ | CTRL4_PICO_OGDISABLE;
+		ret = regmap_update_bits(reg, SC_PERIPH_CTRL4, mask, val);
+		if (ret)
+			goto out;
+
+		ret = regmap_write(reg, SC_PERIPH_CTRL8, EYE_PATTERN_PARA);
+		if (ret)
+			goto out;
+	} else {
+		val = CTRL4_PICO_SIDDQ;
+		mask = val;
+		ret = regmap_update_bits(reg, SC_PERIPH_CTRL4, mask, val);
+		if (ret)
+			goto out;
+
+		val = RST0_USBOTG_BUS | RST0_POR_PICOPHY |
+		      RST0_USBOTG | RST0_USBOTG_32K;
+		mask = val;
+		ret = regmap_update_bits(reg, SC_PERIPH_RSTEN0, mask, val);
+		if (ret)
+			goto out;
+	}
+
+	return 0;
+out:
+	dev_err(priv->dev, "failed to setup phy ret: %d\n", ret);
+	return ret;
+}
+
+static int hi6220_phy_probe(struct platform_device *pdev)
+{
+	struct hi6220_priv *priv;
+	struct usb_otg *otg;
+	struct device_node *np = pdev->dev.of_node;
+	struct device *dev = &pdev->dev;
+	int ret, irq;
+
+	priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
+	if (!priv)
+		return -ENOMEM;
+
+	otg = devm_kzalloc(dev, sizeof(*otg), GFP_KERNEL);
+	if (!otg)
+		return -ENOMEM;
+
+	priv->dev = dev;
+	priv->phy.dev = &pdev->dev;
+	priv->phy.otg = otg;
+	priv->phy.label = "hi6220";
+	priv->phy.type = USB_PHY_TYPE_USB2;
+	otg->set_peripheral = hi6220_set_peripheral;
+	platform_set_drvdata(pdev, priv);
+
+	priv->gpio_vbus = of_get_named_gpio(np, "hisilicon,vbus-gpios", 0);
+	if (priv->gpio_vbus == -EPROBE_DEFER)
+		return -EPROBE_DEFER;
+	if (!gpio_is_valid(priv->gpio_vbus)) {
+		dev_err(dev, "invalid gpio %d\n", priv->gpio_vbus);
+		return -ENODEV;
+	}
+
+	priv->gpio_id = of_get_named_gpio(np, "hisilicon,id-gpios", 0);
+	if (priv->gpio_id == -EPROBE_DEFER)
+		return -EPROBE_DEFER;
+	if (!gpio_is_valid(priv->gpio_id)) {
+		dev_err(dev, "invalid gpio %d\n", priv->gpio_id);
+		return -ENODEV;
+	}
+
+	priv->reg = syscon_regmap_lookup_by_phandle(pdev->dev.of_node,
+					"hisilicon,peripheral-syscon");
+	if (IS_ERR(priv->reg))
+		priv->reg = NULL;
+
+	ret = devm_gpio_request_one(dev, priv->gpio_vbus,
+				    GPIOF_IN, "gpio_vbus");
+	if (ret < 0) {
+		dev_err(dev, "gpio request failed for gpio_vbus\n");
+		return ret;
+	}
+
+	ret = devm_gpio_request_one(dev, priv->gpio_id, GPIOF_IN, "gpio_id");
+	if (ret < 0) {
+		dev_err(dev, "gpio request failed for gpio_id\n");
+		return ret;
+	}
+
+	priv->vcc = devm_regulator_get(dev, "vcc");
+	if (IS_ERR(priv->vcc)) {
+		if (PTR_ERR(priv->vcc) == -EPROBE_DEFER)
+			return -EPROBE_DEFER;
+		dev_info(dev, "No regulator found\n");
+	} else {
+		ret = regulator_enable(priv->vcc);
+		if (ret) {
+			dev_err(dev, "Failed to enable regulator\n");
+			return -ENODEV;
+		}
+	}
+
+	priv->clk = devm_clk_get(dev, NULL);
+	if (IS_ERR(priv->clk)) {
+		regulator_disable(priv->vcc);
+		return PTR_ERR(priv->clk);
+	}
+	clk_prepare_enable(priv->clk);
+	INIT_DELAYED_WORK(&priv->work, hi6220_detect_work);
+
+	irq = gpio_to_irq(priv->gpio_vbus);
+	ret = devm_request_irq(dev, gpio_to_irq(priv->gpio_vbus),
+			       hiusb_gpio_intr, IRQF_NO_SUSPEND |
+			       IRQF_TRIGGER_FALLING | IRQF_TRIGGER_RISING,
+			       "vbus_gpio_intr", priv);
+	if (ret) {
+		dev_err(dev, "request gpio irq failed.\n");
+		goto err_irq;
+	}
+
+	hi6220_phy_setup(priv, true);
+	ret = usb_add_phy_dev(&priv->phy);
+	if (ret) {
+		dev_err(dev, "Can't register transceiver\n");
+		goto err_irq;
+	}
+	schedule_delayed_work(&priv->work, 0);
+
+	return 0;
+err_irq:
+	cancel_delayed_work_sync(&priv->work);
+	clk_disable_unprepare(priv->clk);
+	regulator_disable(priv->vcc);
+	return ret;
+}
+
+static int hi6220_phy_remove(struct platform_device *pdev)
+{
+	struct hi6220_priv *priv = platform_get_drvdata(pdev);
+
+	cancel_delayed_work_sync(&priv->work);
+	hi6220_phy_setup(priv, false);
+	clk_disable_unprepare(priv->clk);
+	regulator_disable(priv->vcc);
+	return 0;
+}
+
+static const struct of_device_id hi6220_phy_of_match[] = {
+	{.compatible = "hisilicon,hi6220-usb-phy",},
+	{ },
+};
+MODULE_DEVICE_TABLE(of, hi6220_phy_of_match);
+
+static struct platform_driver hi6220_phy_driver = {
+	.probe	= hi6220_phy_probe,
+	.remove	= hi6220_phy_remove,
+	.driver = {
+		.name	= "hi6220-usb-phy",
+		.of_match_table	= hi6220_phy_of_match,
+	}
+};
+module_platform_driver(hi6220_phy_driver);
+
+MODULE_DESCRIPTION("HISILICON HI6220 USB PHY driver");
+MODULE_ALIAS("platform:hi6220-usb-phy");
+MODULE_LICENSE("GPL");
-- 
1.9.1

[PATCH v4 4/4] phy: add phy-hi6220-usb

[Kishon Vijay Abraham I]

Hi,

On Thursday 12 February 2015 01:07 PM, Zhangfei Gao wrote:
> Add usb phy controller for hi6220 platform
>
> Signed-off-by: Zhangfei Gao <zhangfei.gao@linaro.org>
> ---
>   drivers/phy/Kconfig          |   9 ++
>   drivers/phy/Makefile         |   1 +
>   drivers/phy/phy-hi6220-usb.c | 306 +++++++++++++++++++++++++++++++++++++++++++
>   3 files changed, 316 insertions(+)
>   create mode 100644 drivers/phy/phy-hi6220-usb.c

why is this driver in drivers/phy when it doesn't use the generic PHY framework 
at all?


-Kishon

>
> diff --git a/drivers/phy/Kconfig b/drivers/phy/Kconfig
> index ccad880..40a1ef1 100644
> --- a/drivers/phy/Kconfig
> +++ b/drivers/phy/Kconfig
> @@ -162,6 +162,15 @@ config PHY_HIX5HD2_SATA
>   	help
>   	  Support for SATA PHY on Hisilicon hix5hd2 Soc.
>
> +config PHY_HI6220_USB
> +	tristate "hi6220 USB PHY support"
> +	select USB_PHY
> +	select MFD_SYSCON
> +	help
> +	  Enable this to support the HISILICON HI6220 USB PHY.
> +
> +	  To compile this driver as a module, choose M here.
> +
>   config PHY_SUN4I_USB
>   	tristate "Allwinner sunxi SoC USB PHY driver"
>   	depends on ARCH_SUNXI && HAS_IOMEM && OF
> diff --git a/drivers/phy/Makefile b/drivers/phy/Makefile
> index aa74f96..ec43c2d 100644
> --- a/drivers/phy/Makefile
> +++ b/drivers/phy/Makefile
> @@ -19,6 +19,7 @@ obj-$(CONFIG_TI_PIPE3)			+= phy-ti-pipe3.o
>   obj-$(CONFIG_TWL4030_USB)		+= phy-twl4030-usb.o
>   obj-$(CONFIG_PHY_EXYNOS5250_SATA)	+= phy-exynos5250-sata.o
>   obj-$(CONFIG_PHY_HIX5HD2_SATA)		+= phy-hix5hd2-sata.o
> +obj-$(CONFIG_PHY_HI6220_USB)		+= phy-hi6220-usb.o
>   obj-$(CONFIG_PHY_SUN4I_USB)		+= phy-sun4i-usb.o
>   obj-$(CONFIG_PHY_SAMSUNG_USB2)		+= phy-exynos-usb2.o
>   phy-exynos-usb2-y			+= phy-samsung-usb2.o
> diff --git a/drivers/phy/phy-hi6220-usb.c b/drivers/phy/phy-hi6220-usb.c
> new file mode 100644
> index 0000000..0d9f5ac
> --- /dev/null
> +++ b/drivers/phy/phy-hi6220-usb.c
> @@ -0,0 +1,306 @@
> +/*
> + * Copyright (c) 2015 Linaro Ltd.
> + * Copyright (c) 2015 Hisilicon Limited.
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License as published by
> + * the Free Software Foundation; either version 2 of the License, or
> + * (at your option) any later version.
> + */
> +
> +#include <linux/clk.h>
> +#include <linux/mfd/syscon.h>
> +#include <linux/of_gpio.h>
> +#include <linux/platform_device.h>
> +#include <linux/regmap.h>
> +#include <linux/regulator/consumer.h>
> +#include <linux/usb/gadget.h>
> +#include <linux/usb/otg.h>
> +
> +#define SC_PERIPH_CTRL4			0x00c
> +
> +#define CTRL4_PICO_SIDDQ		BIT(6)
> +#define CTRL4_PICO_OGDISABLE		BIT(8)
> +#define CTRL4_PICO_VBUSVLDEXT		BIT(10)
> +#define CTRL4_PICO_VBUSVLDEXTSEL	BIT(11)
> +#define CTRL4_OTG_PHY_SEL		BIT(21)
> +
> +#define SC_PERIPH_CTRL5			0x010
> +
> +#define CTRL5_USBOTG_RES_SEL		BIT(3)
> +#define CTRL5_PICOPHY_ACAENB		BIT(4)
> +#define CTRL5_PICOPHY_BC_MODE		BIT(5)
> +#define CTRL5_PICOPHY_CHRGSEL		BIT(6)
> +#define CTRL5_PICOPHY_VDATSRCEND	BIT(7)
> +#define CTRL5_PICOPHY_VDATDETENB	BIT(8)
> +#define CTRL5_PICOPHY_DCDENB		BIT(9)
> +#define CTRL5_PICOPHY_IDDIG		BIT(10)
> +
> +#define SC_PERIPH_CTRL8			0x018
> +#define SC_PERIPH_RSTEN0		0x300
> +#define SC_PERIPH_RSTDIS0		0x304
> +
> +#define RST0_USBOTG_BUS			BIT(4)
> +#define RST0_POR_PICOPHY		BIT(5)
> +#define RST0_USBOTG			BIT(6)
> +#define RST0_USBOTG_32K			BIT(7)
> +
> +#define EYE_PATTERN_PARA		0x7053348c
> +
> +struct hi6220_priv {
> +	struct usb_phy phy;
> +	struct delayed_work work;
> +	struct regmap *reg;
> +	struct clk *clk;
> +	struct regulator *vcc;
> +	struct device *dev;
> +	int gpio_vbus;
> +	int gpio_id;
> +	enum usb_otg_state state;
> +};
> +
> +static void hi6220_start_peripheral(struct hi6220_priv *priv, bool on)
> +{
> +	struct usb_otg *otg = priv->phy.otg;
> +
> +	if (!otg->gadget)
> +		return;
> +
> +	if (on)
> +		usb_gadget_connect(otg->gadget);
> +	else
> +		usb_gadget_disconnect(otg->gadget);
> +}
> +
> +static void hi6220_detect_work(struct work_struct *work)
> +{
> +	struct hi6220_priv *priv =
> +		container_of(work, struct hi6220_priv, work.work);
> +	int gpio_id, gpio_vbus;
> +	enum usb_otg_state state;
> +
> +	if (!gpio_is_valid(priv->gpio_id) || !gpio_is_valid(priv->gpio_vbus))
> +		return;
> +
> +	gpio_id = gpio_get_value_cansleep(priv->gpio_id);
> +	gpio_vbus = gpio_get_value_cansleep(priv->gpio_vbus);
> +
> +	if (gpio_vbus == 0) {
> +		if (gpio_id == 1)
> +			state = OTG_STATE_B_PERIPHERAL;
> +		else
> +			state = OTG_STATE_A_HOST;
> +	} else {
> +		state = OTG_STATE_A_HOST;
> +	}
> +
> +	if (priv->state != state) {
> +		hi6220_start_peripheral(priv, state == OTG_STATE_B_PERIPHERAL);
> +		priv->state = state;
> +	}
> +}
> +
> +static irqreturn_t hiusb_gpio_intr(int irq, void *data)
> +{
> +	struct hi6220_priv *priv = (struct hi6220_priv *)data;
> +
> +	/* add debounce time */
> +	schedule_delayed_work(&priv->work, msecs_to_jiffies(100));
> +	return IRQ_HANDLED;
> +}
> +
> +static int hi6220_set_peripheral(struct usb_otg *otg, struct usb_gadget *gadget)
> +{
> +	otg->gadget = gadget;
> +	return 0;
> +}
> +
> +static int hi6220_phy_setup(struct hi6220_priv *priv, bool on)
> +{
> +	struct regmap *reg = priv->reg;
> +	u32 val, mask;
> +	int ret;
> +
> +	if (priv->reg == NULL)
> +		return 0;
> +
> +	if (on) {
> +		val = RST0_USBOTG_BUS | RST0_POR_PICOPHY |
> +		      RST0_USBOTG | RST0_USBOTG_32K;
> +		mask = val;
> +		ret = regmap_update_bits(reg, SC_PERIPH_RSTDIS0, mask, val);
> +		if (ret)
> +			goto out;
> +
> +		val = CTRL5_USBOTG_RES_SEL | CTRL5_PICOPHY_ACAENB;
> +		mask = val | CTRL5_PICOPHY_BC_MODE;
> +		ret = regmap_update_bits(reg, SC_PERIPH_CTRL5, mask, val);
> +		if (ret)
> +			goto out;
> +
> +		val =  CTRL4_PICO_VBUSVLDEXT | CTRL4_PICO_VBUSVLDEXTSEL |
> +		       CTRL4_OTG_PHY_SEL;
> +		mask = val | CTRL4_PICO_SIDDQ | CTRL4_PICO_OGDISABLE;
> +		ret = regmap_update_bits(reg, SC_PERIPH_CTRL4, mask, val);
> +		if (ret)
> +			goto out;
> +
> +		ret = regmap_write(reg, SC_PERIPH_CTRL8, EYE_PATTERN_PARA);
> +		if (ret)
> +			goto out;
> +	} else {
> +		val = CTRL4_PICO_SIDDQ;
> +		mask = val;
> +		ret = regmap_update_bits(reg, SC_PERIPH_CTRL4, mask, val);
> +		if (ret)
> +			goto out;
> +
> +		val = RST0_USBOTG_BUS | RST0_POR_PICOPHY |
> +		      RST0_USBOTG | RST0_USBOTG_32K;
> +		mask = val;
> +		ret = regmap_update_bits(reg, SC_PERIPH_RSTEN0, mask, val);
> +		if (ret)
> +			goto out;
> +	}
> +
> +	return 0;
> +out:
> +	dev_err(priv->dev, "failed to setup phy ret: %d\n", ret);
> +	return ret;
> +}
> +
> +static int hi6220_phy_probe(struct platform_device *pdev)
> +{
> +	struct hi6220_priv *priv;
> +	struct usb_otg *otg;
> +	struct device_node *np = pdev->dev.of_node;
> +	struct device *dev = &pdev->dev;
> +	int ret, irq;
> +
> +	priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
> +	if (!priv)
> +		return -ENOMEM;
> +
> +	otg = devm_kzalloc(dev, sizeof(*otg), GFP_KERNEL);
> +	if (!otg)
> +		return -ENOMEM;
> +
> +	priv->dev = dev;
> +	priv->phy.dev = &pdev->dev;
> +	priv->phy.otg = otg;
> +	priv->phy.label = "hi6220";
> +	priv->phy.type = USB_PHY_TYPE_USB2;
> +	otg->set_peripheral = hi6220_set_peripheral;
> +	platform_set_drvdata(pdev, priv);
> +
> +	priv->gpio_vbus = of_get_named_gpio(np, "hisilicon,vbus-gpios", 0);
> +	if (priv->gpio_vbus == -EPROBE_DEFER)
> +		return -EPROBE_DEFER;
> +	if (!gpio_is_valid(priv->gpio_vbus)) {
> +		dev_err(dev, "invalid gpio %d\n", priv->gpio_vbus);
> +		return -ENODEV;
> +	}
> +
> +	priv->gpio_id = of_get_named_gpio(np, "hisilicon,id-gpios", 0);
> +	if (priv->gpio_id == -EPROBE_DEFER)
> +		return -EPROBE_DEFER;
> +	if (!gpio_is_valid(priv->gpio_id)) {
> +		dev_err(dev, "invalid gpio %d\n", priv->gpio_id);
> +		return -ENODEV;
> +	}
> +
> +	priv->reg = syscon_regmap_lookup_by_phandle(pdev->dev.of_node,
> +					"hisilicon,peripheral-syscon");
> +	if (IS_ERR(priv->reg))
> +		priv->reg = NULL;
> +
> +	ret = devm_gpio_request_one(dev, priv->gpio_vbus,
> +				    GPIOF_IN, "gpio_vbus");
> +	if (ret < 0) {
> +		dev_err(dev, "gpio request failed for gpio_vbus\n");
> +		return ret;
> +	}
> +
> +	ret = devm_gpio_request_one(dev, priv->gpio_id, GPIOF_IN, "gpio_id");
> +	if (ret < 0) {
> +		dev_err(dev, "gpio request failed for gpio_id\n");
> +		return ret;
> +	}
> +
> +	priv->vcc = devm_regulator_get(dev, "vcc");
> +	if (IS_ERR(priv->vcc)) {
> +		if (PTR_ERR(priv->vcc) == -EPROBE_DEFER)
> +			return -EPROBE_DEFER;
> +		dev_info(dev, "No regulator found\n");
> +	} else {
> +		ret = regulator_enable(priv->vcc);
> +		if (ret) {
> +			dev_err(dev, "Failed to enable regulator\n");
> +			return -ENODEV;
> +		}
> +	}
> +
> +	priv->clk = devm_clk_get(dev, NULL);
> +	if (IS_ERR(priv->clk)) {
> +		regulator_disable(priv->vcc);
> +		return PTR_ERR(priv->clk);
> +	}
> +	clk_prepare_enable(priv->clk);
> +	INIT_DELAYED_WORK(&priv->work, hi6220_detect_work);
> +
> +	irq = gpio_to_irq(priv->gpio_vbus);
> +	ret = devm_request_irq(dev, gpio_to_irq(priv->gpio_vbus),
> +			       hiusb_gpio_intr, IRQF_NO_SUSPEND |
> +			       IRQF_TRIGGER_FALLING | IRQF_TRIGGER_RISING,
> +			       "vbus_gpio_intr", priv);
> +	if (ret) {
> +		dev_err(dev, "request gpio irq failed.\n");
> +		goto err_irq;
> +	}
> +
> +	hi6220_phy_setup(priv, true);
> +	ret = usb_add_phy_dev(&priv->phy);
> +	if (ret) {
> +		dev_err(dev, "Can't register transceiver\n");
> +		goto err_irq;
> +	}
> +	schedule_delayed_work(&priv->work, 0);
> +
> +	return 0;
> +err_irq:
> +	cancel_delayed_work_sync(&priv->work);
> +	clk_disable_unprepare(priv->clk);
> +	regulator_disable(priv->vcc);
> +	return ret;
> +}
> +
> +static int hi6220_phy_remove(struct platform_device *pdev)
> +{
> +	struct hi6220_priv *priv = platform_get_drvdata(pdev);
> +
> +	cancel_delayed_work_sync(&priv->work);
> +	hi6220_phy_setup(priv, false);
> +	clk_disable_unprepare(priv->clk);
> +	regulator_disable(priv->vcc);
> +	return 0;
> +}
> +
> +static const struct of_device_id hi6220_phy_of_match[] = {
> +	{.compatible = "hisilicon,hi6220-usb-phy",},
> +	{ },
> +};
> +MODULE_DEVICE_TABLE(of, hi6220_phy_of_match);
> +
> +static struct platform_driver hi6220_phy_driver = {
> +	.probe	= hi6220_phy_probe,
> +	.remove	= hi6220_phy_remove,
> +	.driver = {
> +		.name	= "hi6220-usb-phy",
> +		.of_match_table	= hi6220_phy_of_match,
> +	}
> +};
> +module_platform_driver(hi6220_phy_driver);
> +
> +MODULE_DESCRIPTION("HISILICON HI6220 USB PHY driver");
> +MODULE_ALIAS("platform:hi6220-usb-phy");
> +MODULE_LICENSE("GPL");
>

[PATCH v4 4/4] phy: add phy-hi6220-usb

[zhangfei]

Hi, Kishon

On 02/18/2015 01:35 PM, Kishon Vijay Abraham I wrote:
> Hi,
>
> On Thursday 12 February 2015 01:07 PM, Zhangfei Gao wrote:
>> Add usb phy controller for hi6220 platform
>>
>> Signed-off-by: Zhangfei Gao <zhangfei.gao@linaro.org>
>> ---
>>   drivers/phy/Kconfig          |   9 ++
>>   drivers/phy/Makefile         |   1 +
>>   drivers/phy/phy-hi6220-usb.c | 306
>> +++++++++++++++++++++++++++++++++++++++++++
>>   3 files changed, 316 insertions(+)
>>   create mode 100644 drivers/phy/phy-hi6220-usb.c
>
> why is this driver in drivers/phy when it doesn't use the generic PHY
> framework at all?
>

Balbi recommended "new drivers only on drivers/phy/", including usb phy.
So Move drivers/usb/phy/phy-hi6220-usb.c to 
drivers/phy/phy-hi6220-usb.c, required by Balbi.

Thanks

[PATCH v4 4/4] phy: add phy-hi6220-usb

[Felipe Balbi]

On Wed, Feb 18, 2015 at 01:44:21PM +0800, zhangfei wrote:
> Hi, Kishon
> 
> On 02/18/2015 01:35 PM, Kishon Vijay Abraham I wrote:
> >Hi,
> >
> >On Thursday 12 February 2015 01:07 PM, Zhangfei Gao wrote:
> >>Add usb phy controller for hi6220 platform
> >>
> >>Signed-off-by: Zhangfei Gao <zhangfei.gao@linaro.org>
> >>---
> >>  drivers/phy/Kconfig          |   9 ++
> >>  drivers/phy/Makefile         |   1 +
> >>  drivers/phy/phy-hi6220-usb.c | 306
> >>+++++++++++++++++++++++++++++++++++++++++++
> >>  3 files changed, 316 insertions(+)
> >>  create mode 100644 drivers/phy/phy-hi6220-usb.c
> >
> >why is this driver in drivers/phy when it doesn't use the generic PHY
> >framework at all?
> >
> 
> Balbi recommended "new drivers only on drivers/phy/", including usb
> phy.

but it should use the API too. It's not only about a directory, you need
to use the new API.

> So Move drivers/usb/phy/phy-hi6220-usb.c to
> drivers/phy/phy-hi6220-usb.c, required by Balbi.

you're reading what I stated the way you like.

-- 
balbi
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20150218/4efa4784/attachment.sig>

[PATCH v4 4/4] phy: add phy-hi6220-usb

[zhangfei]

Hi, Balbi

On 02/18/2015 10:35 PM, Felipe Balbi wrote:
> On Wed, Feb 18, 2015 at 01:44:21PM +0800, zhangfei wrote:
>> Hi, Kishon
>>
>> On 02/18/2015 01:35 PM, Kishon Vijay Abraham I wrote:
>>> Hi,
>>>
>>> On Thursday 12 February 2015 01:07 PM, Zhangfei Gao wrote:
>>>> Add usb phy controller for hi6220 platform
>>>>
>>>> Signed-off-by: Zhangfei Gao <zhangfei.gao@linaro.org>
>>>> ---
>>>>   drivers/phy/Kconfig          |   9 ++
>>>>   drivers/phy/Makefile         |   1 +
>>>>   drivers/phy/phy-hi6220-usb.c | 306
>>>> +++++++++++++++++++++++++++++++++++++++++++
>>>>   3 files changed, 316 insertions(+)
>>>>   create mode 100644 drivers/phy/phy-hi6220-usb.c
>>>
>>> why is this driver in drivers/phy when it doesn't use the generic PHY
>>> framework at all?
>>>
>>
>> Balbi recommended "new drivers only on drivers/phy/", including usb
>> phy.
>
> but it should use the API too. It's not only about a directory, you need
> to use the new API.
>
>> So Move drivers/usb/phy/phy-hi6220-usb.c to
>> drivers/phy/phy-hi6220-usb.c, required by Balbi.
>
> you're reading what I stated the way you like.

Sorry for my bad understanding.

Still not clear about the otg_set_peripheral, which is required in 
phy-hi6220-usb.c

1. drivers/usb/dwc2/gadget.c use
otg_set_peripheral(hsotg->uphy->otg, &hsotg->gadget);

2. include/linux/phy/phy.h
struct phy do not have member otg, while struct usb_phy has.

Could you give more hints?

Thanks

[PATCH v4 4/4] phy: add phy-hi6220-usb

[Felipe Balbi]

On Fri, Feb 20, 2015 at 11:07:21AM +0800, zhangfei wrote:
> Hi, Balbi
> 
> On 02/18/2015 10:35 PM, Felipe Balbi wrote:
> >On Wed, Feb 18, 2015 at 01:44:21PM +0800, zhangfei wrote:
> >>Hi, Kishon
> >>
> >>On 02/18/2015 01:35 PM, Kishon Vijay Abraham I wrote:
> >>>Hi,
> >>>
> >>>On Thursday 12 February 2015 01:07 PM, Zhangfei Gao wrote:
> >>>>Add usb phy controller for hi6220 platform
> >>>>
> >>>>Signed-off-by: Zhangfei Gao <zhangfei.gao@linaro.org>
> >>>>---
> >>>>  drivers/phy/Kconfig          |   9 ++
> >>>>  drivers/phy/Makefile         |   1 +
> >>>>  drivers/phy/phy-hi6220-usb.c | 306
> >>>>+++++++++++++++++++++++++++++++++++++++++++
> >>>>  3 files changed, 316 insertions(+)
> >>>>  create mode 100644 drivers/phy/phy-hi6220-usb.c
> >>>
> >>>why is this driver in drivers/phy when it doesn't use the generic PHY
> >>>framework at all?
> >>>
> >>
> >>Balbi recommended "new drivers only on drivers/phy/", including usb
> >>phy.
> >
> >but it should use the API too. It's not only about a directory, you need
> >to use the new API.
> >
> >>So Move drivers/usb/phy/phy-hi6220-usb.c to
> >>drivers/phy/phy-hi6220-usb.c, required by Balbi.
> >
> >you're reading what I stated the way you like.
> 
> Sorry for my bad understanding.
> 
> Still not clear about the otg_set_peripheral, which is required in
> phy-hi6220-usb.c
> 
> 1. drivers/usb/dwc2/gadget.c use
> otg_set_peripheral(hsotg->uphy->otg, &hsotg->gadget);
> 
> 2. include/linux/phy/phy.h
> struct phy do not have member otg, while struct usb_phy has.
> 
> Could you give more hints?

your set_peripheral doesn't do anything, just holds a pointer. Might as
well not implement it. I'll review your driver more fully tomorrow.

There a few things which must be changed.

-- 
balbi
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20150219/9aa797c1/attachment.sig>

[PATCH v4 4/4] phy: add phy-hi6220-usb

[zhangfei]

On 02/20/2015 12:38 PM, Felipe Balbi wrote:
> On Fri, Feb 20, 2015 at 11:07:21AM +0800, zhangfei wrote:
>> Hi, Balbi
>>
>> On 02/18/2015 10:35 PM, Felipe Balbi wrote:
>>> On Wed, Feb 18, 2015 at 01:44:21PM +0800, zhangfei wrote:
>>>> Hi, Kishon
>>>>
>>>> On 02/18/2015 01:35 PM, Kishon Vijay Abraham I wrote:
>>>>> Hi,
>>>>>
>>>>> On Thursday 12 February 2015 01:07 PM, Zhangfei Gao wrote:
>>>>>> Add usb phy controller for hi6220 platform
>>>>>>
>>>>>> Signed-off-by: Zhangfei Gao <zhangfei.gao@linaro.org>
>>>>>> ---
>>>>>>   drivers/phy/Kconfig          |   9 ++
>>>>>>   drivers/phy/Makefile         |   1 +
>>>>>>   drivers/phy/phy-hi6220-usb.c | 306
>>>>>> +++++++++++++++++++++++++++++++++++++++++++
>>>>>>   3 files changed, 316 insertions(+)
>>>>>>   create mode 100644 drivers/phy/phy-hi6220-usb.c
>>>>>
>>>>> why is this driver in drivers/phy when it doesn't use the generic PHY
>>>>> framework at all?
>>>>>
>>>>
>>>> Balbi recommended "new drivers only on drivers/phy/", including usb
>>>> phy.
>>>
>>> but it should use the API too. It's not only about a directory, you need
>>> to use the new API.
>>>
>>>> So Move drivers/usb/phy/phy-hi6220-usb.c to
>>>> drivers/phy/phy-hi6220-usb.c, required by Balbi.
>>>
>>> you're reading what I stated the way you like.
>>
>> Sorry for my bad understanding.
>>
>> Still not clear about the otg_set_peripheral, which is required in
>> phy-hi6220-usb.c
>>
>> 1. drivers/usb/dwc2/gadget.c use
>> otg_set_peripheral(hsotg->uphy->otg, &hsotg->gadget);
>>
>> 2. include/linux/phy/phy.h
>> struct phy do not have member otg, while struct usb_phy has.
>>
>> Could you give more hints?
>
> your set_peripheral doesn't do anything, just holds a pointer. Might as
> well not implement it. I'll review your driver more fully tomorrow.
>
> There a few things which must be changed.
>

Thanks in advance.

We need this call back set_peripheral setting otg->gadget, which used in 
usb_gadget_connect/disconnect(otg->gadget).

The workable method test here is not provide phy-names = "usb2-phy";
Then dwc2 will still use hsotg->uphy instead of hsotg->phy.
Though devm_phy_create is used in phy-hi6220-usb.c, phy_ops is not used 
in fact.

Thanks

[PATCH v4 4/4] phy: add phy-hi6220-usb

[Felipe Balbi]

Hi,

On Thu, Feb 12, 2015 at 03:37:26PM +0800, Zhangfei Gao wrote:
> Add usb phy controller for hi6220 platform
> 
> Signed-off-by: Zhangfei Gao <zhangfei.gao@linaro.org>
> ---
>  drivers/phy/Kconfig          |   9 ++
>  drivers/phy/Makefile         |   1 +
>  drivers/phy/phy-hi6220-usb.c | 306 +++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 316 insertions(+)
>  create mode 100644 drivers/phy/phy-hi6220-usb.c
> 
> diff --git a/drivers/phy/Kconfig b/drivers/phy/Kconfig
> index ccad880..40a1ef1 100644
> --- a/drivers/phy/Kconfig
> +++ b/drivers/phy/Kconfig
> @@ -162,6 +162,15 @@ config PHY_HIX5HD2_SATA
>  	help
>  	  Support for SATA PHY on Hisilicon hix5hd2 Soc.
>  
> +config PHY_HI6220_USB
> +	tristate "hi6220 USB PHY support"
> +	select USB_PHY
> +	select MFD_SYSCON
> +	help
> +	  Enable this to support the HISILICON HI6220 USB PHY.
> +
> +	  To compile this driver as a module, choose M here.
> +
>  config PHY_SUN4I_USB
>  	tristate "Allwinner sunxi SoC USB PHY driver"
>  	depends on ARCH_SUNXI && HAS_IOMEM && OF
> diff --git a/drivers/phy/Makefile b/drivers/phy/Makefile
> index aa74f96..ec43c2d 100644
> --- a/drivers/phy/Makefile
> +++ b/drivers/phy/Makefile
> @@ -19,6 +19,7 @@ obj-$(CONFIG_TI_PIPE3)			+= phy-ti-pipe3.o
>  obj-$(CONFIG_TWL4030_USB)		+= phy-twl4030-usb.o
>  obj-$(CONFIG_PHY_EXYNOS5250_SATA)	+= phy-exynos5250-sata.o
>  obj-$(CONFIG_PHY_HIX5HD2_SATA)		+= phy-hix5hd2-sata.o
> +obj-$(CONFIG_PHY_HI6220_USB)		+= phy-hi6220-usb.o
>  obj-$(CONFIG_PHY_SUN4I_USB)		+= phy-sun4i-usb.o
>  obj-$(CONFIG_PHY_SAMSUNG_USB2)		+= phy-exynos-usb2.o
>  phy-exynos-usb2-y			+= phy-samsung-usb2.o
> diff --git a/drivers/phy/phy-hi6220-usb.c b/drivers/phy/phy-hi6220-usb.c
> new file mode 100644
> index 0000000..0d9f5ac
> --- /dev/null
> +++ b/drivers/phy/phy-hi6220-usb.c
> @@ -0,0 +1,306 @@
> +/*
> + * Copyright (c) 2015 Linaro Ltd.
> + * Copyright (c) 2015 Hisilicon Limited.
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License as published by
> + * the Free Software Foundation; either version 2 of the License, or
> + * (at your option) any later version.
> + */
> +
> +#include <linux/clk.h>
> +#include <linux/mfd/syscon.h>
> +#include <linux/of_gpio.h>
> +#include <linux/platform_device.h>
> +#include <linux/regmap.h>
> +#include <linux/regulator/consumer.h>
> +#include <linux/usb/gadget.h>
> +#include <linux/usb/otg.h>
> +
> +#define SC_PERIPH_CTRL4			0x00c
> +
> +#define CTRL4_PICO_SIDDQ		BIT(6)
> +#define CTRL4_PICO_OGDISABLE		BIT(8)
> +#define CTRL4_PICO_VBUSVLDEXT		BIT(10)
> +#define CTRL4_PICO_VBUSVLDEXTSEL	BIT(11)
> +#define CTRL4_OTG_PHY_SEL		BIT(21)
> +
> +#define SC_PERIPH_CTRL5			0x010
> +
> +#define CTRL5_USBOTG_RES_SEL		BIT(3)
> +#define CTRL5_PICOPHY_ACAENB		BIT(4)
> +#define CTRL5_PICOPHY_BC_MODE		BIT(5)
> +#define CTRL5_PICOPHY_CHRGSEL		BIT(6)
> +#define CTRL5_PICOPHY_VDATSRCEND	BIT(7)
> +#define CTRL5_PICOPHY_VDATDETENB	BIT(8)
> +#define CTRL5_PICOPHY_DCDENB		BIT(9)
> +#define CTRL5_PICOPHY_IDDIG		BIT(10)
> +
> +#define SC_PERIPH_CTRL8			0x018
> +#define SC_PERIPH_RSTEN0		0x300
> +#define SC_PERIPH_RSTDIS0		0x304
> +
> +#define RST0_USBOTG_BUS			BIT(4)
> +#define RST0_POR_PICOPHY		BIT(5)
> +#define RST0_USBOTG			BIT(6)
> +#define RST0_USBOTG_32K			BIT(7)
> +
> +#define EYE_PATTERN_PARA		0x7053348c
> +
> +struct hi6220_priv {
> +	struct usb_phy phy;
> +	struct delayed_work work;
> +	struct regmap *reg;
> +	struct clk *clk;
> +	struct regulator *vcc;
> +	struct device *dev;
> +	int gpio_vbus;
> +	int gpio_id;
> +	enum usb_otg_state state;
> +};
> +
> +static void hi6220_start_peripheral(struct hi6220_priv *priv, bool on)
> +{
> +	struct usb_otg *otg = priv->phy.otg;
> +
> +	if (!otg->gadget)
> +		return;
> +
> +	if (on)
> +		usb_gadget_connect(otg->gadget);
> +	else
> +		usb_gadget_disconnect(otg->gadget);

why is the PHY fiddling with pullups ?

> +}
> +
> +static void hi6220_detect_work(struct work_struct *work)
> +{
> +	struct hi6220_priv *priv =
> +		container_of(work, struct hi6220_priv, work.work);
> +	int gpio_id, gpio_vbus;
> +	enum usb_otg_state state;
> +
> +	if (!gpio_is_valid(priv->gpio_id) || !gpio_is_valid(priv->gpio_vbus))
> +		return;
> +
> +	gpio_id = gpio_get_value_cansleep(priv->gpio_id);
> +	gpio_vbus = gpio_get_value_cansleep(priv->gpio_vbus);

looks like this should be using extcon

> +
> +	if (gpio_vbus == 0) {
> +		if (gpio_id == 1)
> +			state = OTG_STATE_B_PERIPHERAL;
> +		else
> +			state = OTG_STATE_A_HOST;
> +	} else {
> +		state = OTG_STATE_A_HOST;
> +	}
> +
> +	if (priv->state != state) {
> +		hi6220_start_peripheral(priv, state == OTG_STATE_B_PERIPHERAL);
> +		priv->state = state;
> +	}
> +}
> +
> +static irqreturn_t hiusb_gpio_intr(int irq, void *data)
> +{
> +	struct hi6220_priv *priv = (struct hi6220_priv *)data;
> +
> +	/* add debounce time */
> +	schedule_delayed_work(&priv->work, msecs_to_jiffies(100));

this is really bad. We have threaded interrupt support, right ?

-- 
balbi
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20150220/014b497e/attachment.sig>

[PATCH v4 4/4] phy: add phy-hi6220-usb

[zhangfei]

Hi, Balbi

On 02/20/2015 10:41 PM, Felipe Balbi wrote:

>> +static void hi6220_start_peripheral(struct hi6220_priv *priv, bool on)
>> +{
>> +	struct usb_otg *otg = priv->phy.otg;
>> +
>> +	if (!otg->gadget)
>> +		return;
>> +
>> +	if (on)
>> +		usb_gadget_connect(otg->gadget);
>> +	else
>> +		usb_gadget_disconnect(otg->gadget);
>
> why is the PHY fiddling with pullups ?

We use this to enable/disable otg gadget mode.

The gpio_id & gpio_vbus are used to distinguish otg gadget mode or host 
mode.
When micro usb or otg device attached to otg, gpio_vbus falling down.
And gpio_id = 1 is micro usb, gpio_id = 0 is otg device.

So when micro usb attached, we enable gadget mode; while micro usb 
detached, we disable gadget mode, and dwc2 will automatically set to 
host mode.

>
>> +}
>> +
>> +static void hi6220_detect_work(struct work_struct *work)
>> +{
>> +	struct hi6220_priv *priv =
>> +		container_of(work, struct hi6220_priv, work.work);
>> +	int gpio_id, gpio_vbus;
>> +	enum usb_otg_state state;
>> +
>> +	if (!gpio_is_valid(priv->gpio_id) || !gpio_is_valid(priv->gpio_vbus))
>> +		return;
>> +
>> +	gpio_id = gpio_get_value_cansleep(priv->gpio_id);
>> +	gpio_vbus = gpio_get_value_cansleep(priv->gpio_vbus);
>
> looks like this should be using extcon
Not used extcon before.
However, we need gpio_vbus interrupt.
Checked phy-tahvo.c and phy-omap-otg.c, not find extcon related with 
interrupt.
Will investigate tomorrow.

>
>> +
>> +	if (gpio_vbus == 0) {
>> +		if (gpio_id == 1)
>> +			state = OTG_STATE_B_PERIPHERAL;
>> +		else
>> +			state = OTG_STATE_A_HOST;
>> +	} else {
>> +		state = OTG_STATE_A_HOST;
>> +	}
>> +
>> +	if (priv->state != state) {
>> +		hi6220_start_peripheral(priv, state == OTG_STATE_B_PERIPHERAL);
>> +		priv->state = state;
>> +	}
>> +}
>> +
>> +static irqreturn_t hiusb_gpio_intr(int irq, void *data)
>> +{
>> +	struct hi6220_priv *priv = (struct hi6220_priv *)data;
>> +
>> +	/* add debounce time */
>> +	schedule_delayed_work(&priv->work, msecs_to_jiffies(100));
>
> this is really bad. We have threaded interrupt support, right ?

Since we use two gpio to distinguish gadget mode or host mode.
Debounce time can introduce more accuracy.
I think threaded interrupt can not be used for adding debounce time.
Here add debounce is just for safety.

Thanks

[PATCH v4 4/4] phy: add phy-hi6220-usb

[Felipe Balbi]

Hi,

On Fri, Feb 20, 2015 at 11:44:37PM +0800, zhangfei wrote:
> Hi, Balbi
> 
> On 02/20/2015 10:41 PM, Felipe Balbi wrote:
> 
> >>+static void hi6220_start_peripheral(struct hi6220_priv *priv, bool on)
> >>+{
> >>+	struct usb_otg *otg = priv->phy.otg;
> >>+
> >>+	if (!otg->gadget)
> >>+		return;
> >>+
> >>+	if (on)
> >>+		usb_gadget_connect(otg->gadget);
> >>+	else
> >>+		usb_gadget_disconnect(otg->gadget);
> >
> >why is the PHY fiddling with pullups ?
> 
> We use this to enable/disable otg gadget mode.

I got that, but the pullups don't belong to the PHY, they belong to the
gadget.

> The gpio_id & gpio_vbus are used to distinguish otg gadget mode or
> host mode.
> When micro usb or otg device attached to otg, gpio_vbus falling down.
> And gpio_id = 1 is micro usb, gpio_id = 0 is otg device.

all of that I understood clearly :-)

> So when micro usb attached, we enable gadget mode; while micro usb
> detached, we disable gadget mode, and dwc2 will automatically set to
> host mode.

that's all fine, I'm concerned about letting the PHY fiddle with
something it doesn't own. If I am to change pullups rules in udc-core,
this is likely to break down miserably and I don't want to have to go
through that.

> >>+static void hi6220_detect_work(struct work_struct *work)
> >>+{
> >>+	struct hi6220_priv *priv =
> >>+		container_of(work, struct hi6220_priv, work.work);
> >>+	int gpio_id, gpio_vbus;
> >>+	enum usb_otg_state state;
> >>+
> >>+	if (!gpio_is_valid(priv->gpio_id) || !gpio_is_valid(priv->gpio_vbus))
> >>+		return;
> >>+
> >>+	gpio_id = gpio_get_value_cansleep(priv->gpio_id);
> >>+	gpio_vbus = gpio_get_value_cansleep(priv->gpio_vbus);
> >
> >looks like this should be using extcon
> Not used extcon before.
> However, we need gpio_vbus interrupt.
> Checked phy-tahvo.c and phy-omap-otg.c, not find extcon related with
> interrupt.
> Will investigate tomorrow.

drivers/extcon/extcon-gpio.c

> >>+	if (gpio_vbus == 0) {
> >>+		if (gpio_id == 1)
> >>+			state = OTG_STATE_B_PERIPHERAL;
> >>+		else
> >>+			state = OTG_STATE_A_HOST;
> >>+	} else {
> >>+		state = OTG_STATE_A_HOST;
> >>+	}
> >>+
> >>+	if (priv->state != state) {
> >>+		hi6220_start_peripheral(priv, state == OTG_STATE_B_PERIPHERAL);
> >>+		priv->state = state;
> >>+	}
> >>+}
> >>+
> >>+static irqreturn_t hiusb_gpio_intr(int irq, void *data)
> >>+{
> >>+	struct hi6220_priv *priv = (struct hi6220_priv *)data;
> >>+
> >>+	/* add debounce time */
> >>+	schedule_delayed_work(&priv->work, msecs_to_jiffies(100));
> >
> >this is really bad. We have threaded interrupt support, right ?
> 
> Since we use two gpio to distinguish gadget mode or host mode.
> Debounce time can introduce more accuracy.

gpio_set_debounce() ?

> I think threaded interrupt can not be used for adding debounce time.
> Here add debounce is just for safety.

add the debounce to the gpio itself.

-- 
balbi
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20150220/355bfe35/attachment-0001.sig>

[PATCH v4 4/4] phy: add phy-hi6220-usb

[zhangfei]

Hi, Balbi

On 02/21/2015 12:06 AM, Felipe Balbi wrote:
> Hi,
>
> On Fri, Feb 20, 2015 at 11:44:37PM +0800, zhangfei wrote:
>> Hi, Balbi
>>
>> On 02/20/2015 10:41 PM, Felipe Balbi wrote:
>>
>>>> +static void hi6220_start_peripheral(struct hi6220_priv *priv, bool on)
>>>> +{
>>>> +	struct usb_otg *otg = priv->phy.otg;
>>>> +
>>>> +	if (!otg->gadget)
>>>> +		return;
>>>> +
>>>> +	if (on)
>>>> +		usb_gadget_connect(otg->gadget);
>>>> +	else
>>>> +		usb_gadget_disconnect(otg->gadget);
>>>
>>> why is the PHY fiddling with pullups ?
>>
>> We use this to enable/disable otg gadget mode.
>
> I got that, but the pullups don't belong to the PHY, they belong to the
> gadget.
>
>> The gpio_id & gpio_vbus are used to distinguish otg gadget mode or
>> host mode.
>> When micro usb or otg device attached to otg, gpio_vbus falling down.
>> And gpio_id = 1 is micro usb, gpio_id = 0 is otg device.
>
> all of that I understood clearly :-)
>
>> So when micro usb attached, we enable gadget mode; while micro usb
>> detached, we disable gadget mode, and dwc2 will automatically set to
>> host mode.
>
> that's all fine, I'm concerned about letting the PHY fiddle with
> something it doesn't own. If I am to change pullups rules in udc-core,
> this is likely to break down miserably and I don't want to have to go
> through that.

Thanks for the clarifying.

How about using usb_gadget_vbus_connect/disconnect, which are used in 
many files under drivers/usb/phy.
There is no vbus_session in dwc2/gadget.c, I thought it would be same as 
pullup.

However, usb_gadget_vbus_connect still need para gadget, where should we 
put this file, drivers/usb/phy or drivers/phy

>
>>>> +static void hi6220_detect_work(struct work_struct *work)
>>>> +{
>>>> +	struct hi6220_priv *priv =
>>>> +		container_of(work, struct hi6220_priv, work.work);
>>>> +	int gpio_id, gpio_vbus;
>>>> +	enum usb_otg_state state;
>>>> +
>>>> +	if (!gpio_is_valid(priv->gpio_id) || !gpio_is_valid(priv->gpio_vbus))
>>>> +		return;
>>>> +
>>>> +	gpio_id = gpio_get_value_cansleep(priv->gpio_id);
>>>> +	gpio_vbus = gpio_get_value_cansleep(priv->gpio_vbus);
>>>
>>> looks like this should be using extcon
>> Not used extcon before.
>> However, we need gpio_vbus interrupt.
>> Checked phy-tahvo.c and phy-omap-otg.c, not find extcon related with
>> interrupt.
>> Will investigate tomorrow.
>
> drivers/extcon/extcon-gpio.c
I think there is no need to use extcon, gpio is clear enough.
extcon-gpio.c even do not support dt.

>
>>>> +	if (gpio_vbus == 0) {
>>>> +		if (gpio_id == 1)
>>>> +			state = OTG_STATE_B_PERIPHERAL;
>>>> +		else
>>>> +			state = OTG_STATE_A_HOST;
>>>> +	} else {
>>>> +		state = OTG_STATE_A_HOST;
>>>> +	}
>>>> +
>>>> +	if (priv->state != state) {
>>>> +		hi6220_start_peripheral(priv, state == OTG_STATE_B_PERIPHERAL);
>>>> +		priv->state = state;
>>>> +	}
>>>> +}
>>>> +
>>>> +static irqreturn_t hiusb_gpio_intr(int irq, void *data)
>>>> +{
>>>> +	struct hi6220_priv *priv = (struct hi6220_priv *)data;
>>>> +
>>>> +	/* add debounce time */
>>>> +	schedule_delayed_work(&priv->work, msecs_to_jiffies(100));
>>>
>>> this is really bad. We have threaded interrupt support, right ?
>>
>> Since we use two gpio to distinguish gadget mode or host mode.
>> Debounce time can introduce more accuracy.
>
> gpio_set_debounce() ?
Not all gpio.c support set_debounce, including gpio-pl061.c.

>
>> I think threaded interrupt can not be used for adding debounce time.
>> Here add debounce is just for safety.
>
> add the debounce to the gpio itself.

Here the debounce added only for safety.
gpio_id may mis-report when unplug usb, but it is correct for plug usb & 
otg device.
So debounce can be omitted.
If you think using delayed work for debounce is ugly, it is fine switch 
to threaded_irq.

Thanks

[PATCH v4 4/4] phy: add phy-hi6220-usb

[Felipe Balbi]

Hi,

On Sat, Feb 21, 2015 at 11:03:05PM +0800, zhangfei wrote:
> >>>>+static void hi6220_start_peripheral(struct hi6220_priv *priv, bool on)
> >>>>+{
> >>>>+	struct usb_otg *otg = priv->phy.otg;
> >>>>+
> >>>>+	if (!otg->gadget)
> >>>>+		return;
> >>>>+
> >>>>+	if (on)
> >>>>+		usb_gadget_connect(otg->gadget);
> >>>>+	else
> >>>>+		usb_gadget_disconnect(otg->gadget);
> >>>
> >>>why is the PHY fiddling with pullups ?
> >>
> >>We use this to enable/disable otg gadget mode.
> >
> >I got that, but the pullups don't belong to the PHY, they belong to the
> >gadget.
> >
> >>The gpio_id & gpio_vbus are used to distinguish otg gadget mode or
> >>host mode.
> >>When micro usb or otg device attached to otg, gpio_vbus falling down.
> >>And gpio_id = 1 is micro usb, gpio_id = 0 is otg device.
> >
> >all of that I understood clearly :-)
> >
> >>So when micro usb attached, we enable gadget mode; while micro usb
> >>detached, we disable gadget mode, and dwc2 will automatically set to
> >>host mode.
> >
> >that's all fine, I'm concerned about letting the PHY fiddle with
> >something it doesn't own. If I am to change pullups rules in udc-core,
> >this is likely to break down miserably and I don't want to have to go
> >through that.
> 
> Thanks for the clarifying.

no problem.

> How about using usb_gadget_vbus_connect/disconnect, which are used in many
> files under drivers/usb/phy.
> There is no vbus_session in dwc2/gadget.c, I thought it would be same as
> pullup.
> 
> However, usb_gadget_vbus_connect still need para gadget, where should we put
> this file, drivers/usb/phy or drivers/phy

drivers/phy, if the framework misses anything you need, it's a great
opportunity to give back to the community by extending the framework.

Scratching one's own itch kinda thing...

> >>>>+static void hi6220_detect_work(struct work_struct *work)
> >>>>+{
> >>>>+	struct hi6220_priv *priv =
> >>>>+		container_of(work, struct hi6220_priv, work.work);
> >>>>+	int gpio_id, gpio_vbus;
> >>>>+	enum usb_otg_state state;
> >>>>+
> >>>>+	if (!gpio_is_valid(priv->gpio_id) || !gpio_is_valid(priv->gpio_vbus))
> >>>>+		return;
> >>>>+
> >>>>+	gpio_id = gpio_get_value_cansleep(priv->gpio_id);
> >>>>+	gpio_vbus = gpio_get_value_cansleep(priv->gpio_vbus);
> >>>
> >>>looks like this should be using extcon
> >>Not used extcon before.
> >>However, we need gpio_vbus interrupt.
> >>Checked phy-tahvo.c and phy-omap-otg.c, not find extcon related with
> >>interrupt.
> >>Will investigate tomorrow.
> >
> >drivers/extcon/extcon-gpio.c
> I think there is no need to use extcon, gpio is clear enough.
> extcon-gpio.c even do not support dt.

well, add DT. The whole idea of free software is that we improve on
things we already have. EXTCON is *the* API to handle such things.

Quite frankly, though, Roger Quadros (now in Cc) has been working to get
DT support on extcon-gpio, so perhaps wait for that and base your
patches on top of his.

Now your statement that GPIO is clear enough is completely bogus to me.

Why do we have fixed regulators with GPIO enable signals, right ? Might
as well just fiddle with the GPIO directly, right ? Wrong, the idea of
using these frameworks is to encapsulate implementation details and make
sure that if things change from one board to another, we can still use
our SW without major modifications.

> >>>>+	if (gpio_vbus == 0) {
> >>>>+		if (gpio_id == 1)
> >>>>+			state = OTG_STATE_B_PERIPHERAL;
> >>>>+		else
> >>>>+			state = OTG_STATE_A_HOST;
> >>>>+	} else {
> >>>>+		state = OTG_STATE_A_HOST;
> >>>>+	}
> >>>>+
> >>>>+	if (priv->state != state) {
> >>>>+		hi6220_start_peripheral(priv, state == OTG_STATE_B_PERIPHERAL);
> >>>>+		priv->state = state;
> >>>>+	}
> >>>>+}
> >>>>+
> >>>>+static irqreturn_t hiusb_gpio_intr(int irq, void *data)
> >>>>+{
> >>>>+	struct hi6220_priv *priv = (struct hi6220_priv *)data;
> >>>>+
> >>>>+	/* add debounce time */
> >>>>+	schedule_delayed_work(&priv->work, msecs_to_jiffies(100));
> >>>
> >>>this is really bad. We have threaded interrupt support, right ?
> >>
> >>Since we use two gpio to distinguish gadget mode or host mode.
> >>Debounce time can introduce more accuracy.
> >
> >gpio_set_debounce() ?
> Not all gpio.c support set_debounce, including gpio-pl061.c.

then the framework should implement it in SW. That was the whole idea of
adding set_debounce() to gpiolib. If the controller can't handle it,
gpiolib handles it in SW. Again, encapsulating details.

> >>I think threaded interrupt can not be used for adding debounce time.
> >>Here add debounce is just for safety.
> >
> >add the debounce to the gpio itself.
> 
> Here the debounce added only for safety.
> gpio_id may mis-report when unplug usb, but it is correct for plug usb & otg
> device.
> So debounce can be omitted.
> If you think using delayed work for debounce is ugly, it is fine switch to
> threaded_irq.

debounce might be very well needed. We *do* want to filter out the
transient period. I'm just telling you there are better ways of doing
so; and your response to that is "let's just remove it" and I'm not
really comfortable with that attitude.

That's the attitude of a lazy person which, I hope, you are not ;)
balbi
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20150221/9223c82f/attachment.sig>

[PATCH v4 4/4] phy: add phy-hi6220-usb

[zhangfei]

Hi, Balbi

On 02/22/2015 12:21 AM, Felipe Balbi wrote:
> Hi,
>
> On Sat, Feb 21, 2015 at 11:03:05PM +0800, zhangfei wrote:
>>>>>> +static void hi6220_start_peripheral(struct hi6220_priv *priv, bool on)
>>>>>> +{
>>>>>> +	struct usb_otg *otg = priv->phy.otg;
>>>>>> +
>>>>>> +	if (!otg->gadget)
>>>>>> +		return;
>>>>>> +
>>>>>> +	if (on)
>>>>>> +		usb_gadget_connect(otg->gadget);
>>>>>> +	else
>>>>>> +		usb_gadget_disconnect(otg->gadget);
>>>>>
>>>>> why is the PHY fiddling with pullups ?
>>>>
>>>> We use this to enable/disable otg gadget mode.
>>>
>>> I got that, but the pullups don't belong to the PHY, they belong to the
>>> gadget.
>>>
>>>> The gpio_id & gpio_vbus are used to distinguish otg gadget mode or
>>>> host mode.
>>>> When micro usb or otg device attached to otg, gpio_vbus falling down.
>>>> And gpio_id = 1 is micro usb, gpio_id = 0 is otg device.
>>>
>>> all of that I understood clearly :-)
>>>
>>>> So when micro usb attached, we enable gadget mode; while micro usb
>>>> detached, we disable gadget mode, and dwc2 will automatically set to
>>>> host mode.
>>>
>>> that's all fine, I'm concerned about letting the PHY fiddle with
>>> something it doesn't own. If I am to change pullups rules in udc-core,
>>> this is likely to break down miserably and I don't want to have to go
>>> through that.
>>
>> Thanks for the clarifying.
>
> no problem.
>
>> How about using usb_gadget_vbus_connect/disconnect, which are used in many
>> files under drivers/usb/phy.
>> There is no vbus_session in dwc2/gadget.c, I thought it would be same as
>> pullup.
>>
>> However, usb_gadget_vbus_connect still need para gadget, where should we put
>> this file, drivers/usb/phy or drivers/phy
>
> drivers/phy, if the framework misses anything you need, it's a great
> opportunity to give back to the community by extending the framework.

Sorry, I am a little confused.
I need some concrete suggestion for the next step of this patch, which 
is required for the community board, hikey board.

Do you mean in the future we need use hsotg->phy instead of hsotg->uphy.
         struct phy *phy;
         struct usb_phy *uphy;
usb_phy has many members that struct phy does not have, including otg.
struct usb_otg          *otg;
Is that mean we need port such member from usb_phy to phy.

Besides, are you ok with using usb_gadget_vbus_connect/disconnect.

>
> Scratching one's own itch kinda thing...
>
>>>>>> +static void hi6220_detect_work(struct work_struct *work)
>>>>>> +{
>>>>>> +	struct hi6220_priv *priv =
>>>>>> +		container_of(work, struct hi6220_priv, work.work);
>>>>>> +	int gpio_id, gpio_vbus;
>>>>>> +	enum usb_otg_state state;
>>>>>> +
>>>>>> +	if (!gpio_is_valid(priv->gpio_id) || !gpio_is_valid(priv->gpio_vbus))
>>>>>> +		return;
>>>>>> +
>>>>>> +	gpio_id = gpio_get_value_cansleep(priv->gpio_id);
>>>>>> +	gpio_vbus = gpio_get_value_cansleep(priv->gpio_vbus);
>>>>>
>>>>> looks like this should be using extcon
>>>> Not used extcon before.
>>>> However, we need gpio_vbus interrupt.
>>>> Checked phy-tahvo.c and phy-omap-otg.c, not find extcon related with
>>>> interrupt.
>>>> Will investigate tomorrow.
>>>
>>> drivers/extcon/extcon-gpio.c
>> I think there is no need to use extcon, gpio is clear enough.
>> extcon-gpio.c even do not support dt.
>
> well, add DT. The whole idea of free software is that we improve on
> things we already have. EXTCON is *the* API to handle such things.

I think I am still not understanding extcon-gpio, not sure why need use 
this API here.

Here two gpio requires, one gpio as interrupt, in the interrupt handler, 
we detect the gpio status judging the otg status.
extcon-gpio.c use the interrupt, then can we also use the gpio interrupt.
Using extcon-gpio is used for saving gpio_request?

>
> Quite frankly, though, Roger Quadros (now in Cc) has been working to get
> DT support on extcon-gpio, so perhaps wait for that and base your
> patches on top of his.
>
> Now your statement that GPIO is clear enough is completely bogus to me.
>
> Why do we have fixed regulators with GPIO enable signals, right ? Might
> as well just fiddle with the GPIO directly, right ? Wrong, the idea of
> using these frameworks is to encapsulate implementation details and make
> sure that if things change from one board to another, we can still use
> our SW without major modifications.
>
>>>>>> +	if (gpio_vbus == 0) {
>>>>>> +		if (gpio_id == 1)
>>>>>> +			state = OTG_STATE_B_PERIPHERAL;
>>>>>> +		else
>>>>>> +			state = OTG_STATE_A_HOST;
>>>>>> +	} else {
>>>>>> +		state = OTG_STATE_A_HOST;
>>>>>> +	}
>>>>>> +
>>>>>> +	if (priv->state != state) {
>>>>>> +		hi6220_start_peripheral(priv, state == OTG_STATE_B_PERIPHERAL);
>>>>>> +		priv->state = state;
>>>>>> +	}
>>>>>> +}
>>>>>> +
>>>>>> +static irqreturn_t hiusb_gpio_intr(int irq, void *data)
>>>>>> +{
>>>>>> +	struct hi6220_priv *priv = (struct hi6220_priv *)data;
>>>>>> +
>>>>>> +	/* add debounce time */
>>>>>> +	schedule_delayed_work(&priv->work, msecs_to_jiffies(100));
>>>>>
>>>>> this is really bad. We have threaded interrupt support, right ?
>>>>
>>>> Since we use two gpio to distinguish gadget mode or host mode.
>>>> Debounce time can introduce more accuracy.
>>>
>>> gpio_set_debounce() ?
>> Not all gpio.c support set_debounce, including gpio-pl061.c.
>
> then the framework should implement it in SW. That was the whole idea of
> adding set_debounce() to gpiolib. If the controller can't handle it,
> gpiolib handles it in SW. Again, encapsulating details.
>
>>>> I think threaded interrupt can not be used for adding debounce time.
>>>> Here add debounce is just for safety.
>>>
>>> add the debounce to the gpio itself.
>>
>> Here the debounce added only for safety.
>> gpio_id may mis-report when unplug usb, but it is correct for plug usb & otg
>> device.
>> So debounce can be omitted.
>> If you think using delayed work for debounce is ugly, it is fine switch to
>> threaded_irq.
>
> debounce might be very well needed. We *do* want to filter out the
> transient period. I'm just telling you there are better ways of doing
> so; and your response to that is "let's just remove it" and I'm not
> really comfortable with that attitude.
>
> That's the attitude of a lazy person which, I hope, you are not ;)

Understand.
What I mean here is gpio_id is not used when unplug, it is only used 
after plug.

Thanks

[PATCH v4 4/4] phy: add phy-hi6220-usb

[Felipe Balbi]

Hi,

On Sun, Feb 22, 2015 at 11:10:36AM +0800, zhangfei wrote:
> >>>>>>+static void hi6220_start_peripheral(struct hi6220_priv *priv, bool on)
> >>>>>>+{
> >>>>>>+	struct usb_otg *otg = priv->phy.otg;
> >>>>>>+
> >>>>>>+	if (!otg->gadget)
> >>>>>>+		return;
> >>>>>>+
> >>>>>>+	if (on)
> >>>>>>+		usb_gadget_connect(otg->gadget);
> >>>>>>+	else
> >>>>>>+		usb_gadget_disconnect(otg->gadget);
> >>>>>
> >>>>>why is the PHY fiddling with pullups ?
> >>>>
> >>>>We use this to enable/disable otg gadget mode.
> >>>
> >>>I got that, but the pullups don't belong to the PHY, they belong to the
> >>>gadget.
> >>>
> >>>>The gpio_id & gpio_vbus are used to distinguish otg gadget mode or
> >>>>host mode.
> >>>>When micro usb or otg device attached to otg, gpio_vbus falling down.
> >>>>And gpio_id = 1 is micro usb, gpio_id = 0 is otg device.
> >>>
> >>>all of that I understood clearly :-)
> >>>
> >>>>So when micro usb attached, we enable gadget mode; while micro usb
> >>>>detached, we disable gadget mode, and dwc2 will automatically set to
> >>>>host mode.
> >>>
> >>>that's all fine, I'm concerned about letting the PHY fiddle with
> >>>something it doesn't own. If I am to change pullups rules in udc-core,
> >>>this is likely to break down miserably and I don't want to have to go
> >>>through that.
> >>
> >>Thanks for the clarifying.
> >
> >no problem.
> >
> >>How about using usb_gadget_vbus_connect/disconnect, which are used in many
> >>files under drivers/usb/phy.
> >>There is no vbus_session in dwc2/gadget.c, I thought it would be same as
> >>pullup.
> >>
> >>However, usb_gadget_vbus_connect still need para gadget, where should we put
> >>this file, drivers/usb/phy or drivers/phy
> >
> >drivers/phy, if the framework misses anything you need, it's a great
> >opportunity to give back to the community by extending the framework.
> 
> Sorry, I am a little confused.
> I need some concrete suggestion for the next step of this patch, which is
> required for the community board, hikey board.
> 
> Do you mean in the future we need use hsotg->phy instead of hsotg->uphy.
>         struct phy *phy;
>         struct usb_phy *uphy;

yes, we need to move everybody to use struct phy, instead of struct
usb_phy.

> usb_phy has many members that struct phy does not have, including otg.
> struct usb_otg          *otg;
> Is that mean we need port such member from usb_phy to phy.

This means we have a little ground work to do before we can add your phy
driver to that framework, right ? As I said, if the framework is missing
anything, work with Kishon (generic phy maintainer) to add those missing
pieces generically.

> Besides, are you ok with using usb_gadget_vbus_connect/disconnect.
> 
> >
> >Scratching one's own itch kinda thing...
> >
> >>>>>>+static void hi6220_detect_work(struct work_struct *work)
> >>>>>>+{
> >>>>>>+	struct hi6220_priv *priv =
> >>>>>>+		container_of(work, struct hi6220_priv, work.work);
> >>>>>>+	int gpio_id, gpio_vbus;
> >>>>>>+	enum usb_otg_state state;
> >>>>>>+
> >>>>>>+	if (!gpio_is_valid(priv->gpio_id) || !gpio_is_valid(priv->gpio_vbus))
> >>>>>>+		return;
> >>>>>>+
> >>>>>>+	gpio_id = gpio_get_value_cansleep(priv->gpio_id);
> >>>>>>+	gpio_vbus = gpio_get_value_cansleep(priv->gpio_vbus);
> >>>>>
> >>>>>looks like this should be using extcon
> >>>>Not used extcon before.
> >>>>However, we need gpio_vbus interrupt.
> >>>>Checked phy-tahvo.c and phy-omap-otg.c, not find extcon related with
> >>>>interrupt.
> >>>>Will investigate tomorrow.
> >>>
> >>>drivers/extcon/extcon-gpio.c
> >>I think there is no need to use extcon, gpio is clear enough.
> >>extcon-gpio.c even do not support dt.
> >
> >well, add DT. The whole idea of free software is that we improve on
> >things we already have. EXTCON is *the* API to handle such things.
> 
> I think I am still not understanding extcon-gpio, not sure why need
> use this API here.

because extcon is the API to use for external connectors. The same way
you use regulator framework to control that single GPIO tied to an
enable signal of a fixed regulator, you use extcon when you need to read
that gpio signal tied to id pin of the USB connector.

> Here two gpio requires, one gpio as interrupt, in the interrupt
> handler, we detect the gpio status judging the otg status.
> extcon-gpio.c use the interrupt, then can we also use the gpio
> interrupt.  Using extcon-gpio is used for saving gpio_request?

extcon is used to hide gpio_request from dwc2. dwc2 only knows about
extcon, not gpios. extcon will request the gpio and use it as interrupt
source. When an IRQ fires, it will read the gpio state and decide if it
should broadcast a message to tell dwc2 to become host or peripheral.

> >Quite frankly, though, Roger Quadros (now in Cc) has been working to get
> >DT support on extcon-gpio, so perhaps wait for that and base your
> >patches on top of his.
> >
> >Now your statement that GPIO is clear enough is completely bogus to me.
> >
> >Why do we have fixed regulators with GPIO enable signals, right ? Might
> >as well just fiddle with the GPIO directly, right ? Wrong, the idea of
> >using these frameworks is to encapsulate implementation details and make
> >sure that if things change from one board to another, we can still use
> >our SW without major modifications.
> >
> >>>>>>+	if (gpio_vbus == 0) {
> >>>>>>+		if (gpio_id == 1)
> >>>>>>+			state = OTG_STATE_B_PERIPHERAL;
> >>>>>>+		else
> >>>>>>+			state = OTG_STATE_A_HOST;
> >>>>>>+	} else {
> >>>>>>+		state = OTG_STATE_A_HOST;
> >>>>>>+	}
> >>>>>>+
> >>>>>>+	if (priv->state != state) {
> >>>>>>+		hi6220_start_peripheral(priv, state == OTG_STATE_B_PERIPHERAL);
> >>>>>>+		priv->state = state;
> >>>>>>+	}
> >>>>>>+}
> >>>>>>+
> >>>>>>+static irqreturn_t hiusb_gpio_intr(int irq, void *data)
> >>>>>>+{
> >>>>>>+	struct hi6220_priv *priv = (struct hi6220_priv *)data;
> >>>>>>+
> >>>>>>+	/* add debounce time */
> >>>>>>+	schedule_delayed_work(&priv->work, msecs_to_jiffies(100));
> >>>>>
> >>>>>this is really bad. We have threaded interrupt support, right ?
> >>>>
> >>>>Since we use two gpio to distinguish gadget mode or host mode.
> >>>>Debounce time can introduce more accuracy.
> >>>
> >>>gpio_set_debounce() ?
> >>Not all gpio.c support set_debounce, including gpio-pl061.c.
> >
> >then the framework should implement it in SW. That was the whole idea of
> >adding set_debounce() to gpiolib. If the controller can't handle it,
> >gpiolib handles it in SW. Again, encapsulating details.
> >
> >>>>I think threaded interrupt can not be used for adding debounce time.
> >>>>Here add debounce is just for safety.
> >>>
> >>>add the debounce to the gpio itself.
> >>
> >>Here the debounce added only for safety.
> >>gpio_id may mis-report when unplug usb, but it is correct for plug usb & otg
> >>device.
> >>So debounce can be omitted.
> >>If you think using delayed work for debounce is ugly, it is fine switch to
> >>threaded_irq.
> >
> >debounce might be very well needed. We *do* want to filter out the
> >transient period. I'm just telling you there are better ways of doing
> >so; and your response to that is "let's just remove it" and I'm not
> >really comfortable with that attitude.
> >
> >That's the attitude of a lazy person which, I hope, you are not ;)
> 
> Understand.
> What I mean here is gpio_id is not used when unplug, it is only used after
> plug.

sure, but does it make a difference if you let gpiolib handle debouncing
for you ? It makes no difference at all, right ?

-- 
balbi
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20150223/58ed6b4d/attachment.sig>

[PATCH v4 4/4] phy: add phy-hi6220-usb

[zhangfei]

On 02/23/2015 11:36 PM, Felipe Balbi wrote:
> Hi,
>
> On Sun, Feb 22, 2015 at 11:10:36AM +0800, zhangfei wrote:
>>>>>>>> +static void hi6220_start_peripheral(struct hi6220_priv *priv, bool on)
>>>>>>>> +{
>>>>>>>> +	struct usb_otg *otg = priv->phy.otg;
>>>>>>>> +
>>>>>>>> +	if (!otg->gadget)
>>>>>>>> +		return;
>>>>>>>> +
>>>>>>>> +	if (on)
>>>>>>>> +		usb_gadget_connect(otg->gadget);
>>>>>>>> +	else
>>>>>>>> +		usb_gadget_disconnect(otg->gadget);
>>>>>>>
>>>>>>> why is the PHY fiddling with pullups ?
>>>>>>
>>>>>> We use this to enable/disable otg gadget mode.
>>>>>
>>>>> I got that, but the pullups don't belong to the PHY, they belong to the
>>>>> gadget.
>>>>>
>>>>>> The gpio_id & gpio_vbus are used to distinguish otg gadget mode or
>>>>>> host mode.
>>>>>> When micro usb or otg device attached to otg, gpio_vbus falling down.
>>>>>> And gpio_id = 1 is micro usb, gpio_id = 0 is otg device.
>>>>>
>>>>> all of that I understood clearly :-)
>>>>>
>>>>>> So when micro usb attached, we enable gadget mode; while micro usb
>>>>>> detached, we disable gadget mode, and dwc2 will automatically set to
>>>>>> host mode.
>>>>>
>>>>> that's all fine, I'm concerned about letting the PHY fiddle with
>>>>> something it doesn't own. If I am to change pullups rules in udc-core,
>>>>> this is likely to break down miserably and I don't want to have to go
>>>>> through that.
>>>>
>>>> Thanks for the clarifying.
>>>
>>> no problem.
>>>
>>>> How about using usb_gadget_vbus_connect/disconnect, which are used in many
>>>> files under drivers/usb/phy.
>>>> There is no vbus_session in dwc2/gadget.c, I thought it would be same as
>>>> pullup.
>>>>
>>>> However, usb_gadget_vbus_connect still need para gadget, where should we put
>>>> this file, drivers/usb/phy or drivers/phy
>>>
>>> drivers/phy, if the framework misses anything you need, it's a great
>>> opportunity to give back to the community by extending the framework.
>>
>> Sorry, I am a little confused.
>> I need some concrete suggestion for the next step of this patch, which is
>> required for the community board, hikey board.
>>
>> Do you mean in the future we need use hsotg->phy instead of hsotg->uphy.
>>          struct phy *phy;
>>          struct usb_phy *uphy;
>
> yes, we need to move everybody to use struct phy, instead of struct
> usb_phy.
>
>> usb_phy has many members that struct phy does not have, including otg.
>> struct usb_otg          *otg;
>> Is that mean we need port such member from usb_phy to phy.
>
> This means we have a little ground work to do before we can add your phy
> driver to that framework, right ? As I said, if the framework is missing
> anything, work with Kishon (generic phy maintainer) to add those missing
> pieces generically.

OK, got it.
>
>> Besides, are you ok with using usb_gadget_vbus_connect/disconnect.
>>
>>>
>>> Scratching one's own itch kinda thing...
>>>
>>>>>>>> +static void hi6220_detect_work(struct work_struct *work)
>>>>>>>> +{
>>>>>>>> +	struct hi6220_priv *priv =
>>>>>>>> +		container_of(work, struct hi6220_priv, work.work);
>>>>>>>> +	int gpio_id, gpio_vbus;
>>>>>>>> +	enum usb_otg_state state;
>>>>>>>> +
>>>>>>>> +	if (!gpio_is_valid(priv->gpio_id) || !gpio_is_valid(priv->gpio_vbus))
>>>>>>>> +		return;
>>>>>>>> +
>>>>>>>> +	gpio_id = gpio_get_value_cansleep(priv->gpio_id);
>>>>>>>> +	gpio_vbus = gpio_get_value_cansleep(priv->gpio_vbus);
>>>>>>>
>>>>>>> looks like this should be using extcon
>>>>>> Not used extcon before.
>>>>>> However, we need gpio_vbus interrupt.
>>>>>> Checked phy-tahvo.c and phy-omap-otg.c, not find extcon related with
>>>>>> interrupt.
>>>>>> Will investigate tomorrow.
>>>>>
>>>>> drivers/extcon/extcon-gpio.c
>>>> I think there is no need to use extcon, gpio is clear enough.
>>>> extcon-gpio.c even do not support dt.
>>>
>>> well, add DT. The whole idea of free software is that we improve on
>>> things we already have. EXTCON is *the* API to handle such things.
>>
>> I think I am still not understanding extcon-gpio, not sure why need
>> use this API here.
>
> because extcon is the API to use for external connectors. The same way
> you use regulator framework to control that single GPIO tied to an
> enable signal of a fixed regulator, you use extcon when you need to read
> that gpio signal tied to id pin of the USB connector.
>
>> Here two gpio requires, one gpio as interrupt, in the interrupt
>> handler, we detect the gpio status judging the otg status.
>> extcon-gpio.c use the interrupt, then can we also use the gpio
>> interrupt.  Using extcon-gpio is used for saving gpio_request?
>
> extcon is used to hide gpio_request from dwc2. dwc2 only knows about
> extcon, not gpios. extcon will request the gpio and use it as interrupt
> source. When an IRQ fires, it will read the gpio state and decide if it
> should broadcast a message to tell dwc2 to become host or peripheral.

Thanks for the kind education, understand now.

[PATCH v4 4/4] phy: add phy-hi6220-usb

[Felipe Balbi]

On Wed, Feb 25, 2015 at 09:28:36PM +0800, zhangfei wrote:
> >>>>>>>>+static void hi6220_detect_work(struct work_struct *work)
> >>>>>>>>+{
> >>>>>>>>+	struct hi6220_priv *priv =
> >>>>>>>>+		container_of(work, struct hi6220_priv, work.work);
> >>>>>>>>+	int gpio_id, gpio_vbus;
> >>>>>>>>+	enum usb_otg_state state;
> >>>>>>>>+
> >>>>>>>>+	if (!gpio_is_valid(priv->gpio_id) || !gpio_is_valid(priv->gpio_vbus))
> >>>>>>>>+		return;
> >>>>>>>>+
> >>>>>>>>+	gpio_id = gpio_get_value_cansleep(priv->gpio_id);
> >>>>>>>>+	gpio_vbus = gpio_get_value_cansleep(priv->gpio_vbus);
> >>>>>>>
> >>>>>>>looks like this should be using extcon
> >>>>>>Not used extcon before.
> >>>>>>However, we need gpio_vbus interrupt.
> >>>>>>Checked phy-tahvo.c and phy-omap-otg.c, not find extcon related with
> >>>>>>interrupt.
> >>>>>>Will investigate tomorrow.
> >>>>>
> >>>>>drivers/extcon/extcon-gpio.c
> >>>>I think there is no need to use extcon, gpio is clear enough.
> >>>>extcon-gpio.c even do not support dt.
> >>>
> >>>well, add DT. The whole idea of free software is that we improve on
> >>>things we already have. EXTCON is *the* API to handle such things.
> >>
> >>I think I am still not understanding extcon-gpio, not sure why need
> >>use this API here.
> >
> >because extcon is the API to use for external connectors. The same way
> >you use regulator framework to control that single GPIO tied to an
> >enable signal of a fixed regulator, you use extcon when you need to read
> >that gpio signal tied to id pin of the USB connector.
> >
> >>Here two gpio requires, one gpio as interrupt, in the interrupt
> >>handler, we detect the gpio status judging the otg status.
> >>extcon-gpio.c use the interrupt, then can we also use the gpio
> >>interrupt.  Using extcon-gpio is used for saving gpio_request?
> >
> >extcon is used to hide gpio_request from dwc2. dwc2 only knows about
> >extcon, not gpios. extcon will request the gpio and use it as interrupt
> >source. When an IRQ fires, it will read the gpio state and decide if it
> >should broadcast a message to tell dwc2 to become host or peripheral.
> 
> Thanks for the kind education, understand now.

hey, no problem ;-)

-- 
balbi
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20150225/dab57924/attachment.sig>

[PATCH v4 4/4] phy: add phy-hi6220-usb

[Roger Quadros]

Hi Zhangfei,

On 22/02/15 05:10, zhangfei wrote:
> Hi, Balbi
> 
> On 02/22/2015 12:21 AM, Felipe Balbi wrote:
>> Hi,
>>
>> On Sat, Feb 21, 2015 at 11:03:05PM +0800, zhangfei wrote:
>>>>>>> +static void hi6220_start_peripheral(struct hi6220_priv *priv, bool on)
>>>>>>> +{
>>>>>>> +    struct usb_otg *otg = priv->phy.otg;
>>>>>>> +
>>>>>>> +    if (!otg->gadget)
>>>>>>> +        return;
>>>>>>> +
>>>>>>> +    if (on)
>>>>>>> +        usb_gadget_connect(otg->gadget);
>>>>>>> +    else
>>>>>>> +        usb_gadget_disconnect(otg->gadget);
>>>>>>
>>>>>> why is the PHY fiddling with pullups ?
>>>>>
>>>>> We use this to enable/disable otg gadget mode.
>>>>
>>>> I got that, but the pullups don't belong to the PHY, they belong to the
>>>> gadget.
>>>>
>>>>> The gpio_id & gpio_vbus are used to distinguish otg gadget mode or
>>>>> host mode.
>>>>> When micro usb or otg device attached to otg, gpio_vbus falling down.
>>>>> And gpio_id = 1 is micro usb, gpio_id = 0 is otg device.
>>>>
>>>> all of that I understood clearly :-)
>>>>
>>>>> So when micro usb attached, we enable gadget mode; while micro usb
>>>>> detached, we disable gadget mode, and dwc2 will automatically set to
>>>>> host mode.
>>>>
>>>> that's all fine, I'm concerned about letting the PHY fiddle with
>>>> something it doesn't own. If I am to change pullups rules in udc-core,
>>>> this is likely to break down miserably and I don't want to have to go
>>>> through that.
>>>
>>> Thanks for the clarifying.
>>
>> no problem.
>>
>>> How about using usb_gadget_vbus_connect/disconnect, which are used in many
>>> files under drivers/usb/phy.
>>> There is no vbus_session in dwc2/gadget.c, I thought it would be same as
>>> pullup.
>>>
>>> However, usb_gadget_vbus_connect still need para gadget, where should we put
>>> this file, drivers/usb/phy or drivers/phy
>>
>> drivers/phy, if the framework misses anything you need, it's a great
>> opportunity to give back to the community by extending the framework.
> 
> Sorry, I am a little confused.
> I need some concrete suggestion for the next step of this patch, which is required for the community board, hikey board.
> 
> Do you mean in the future we need use hsotg->phy instead of hsotg->uphy.
>         struct phy *phy;
>         struct usb_phy *uphy;
> usb_phy has many members that struct phy does not have, including otg.
> struct usb_otg          *otg;
> Is that mean we need port such member from usb_phy to phy.

In my opinion otg structure should belong to the USB core part that takes care
of the OTG/DRD state machine. We still don't have a clear solution here and
I'm currently investigating this.
My current work is to get Dual role functionality working with DWC3 controller and TI
platforms.

Currently phy drivers take care of OTG operation themselves but there is an opportunity
to share code and centralize USB role switching.
The USB core should be the owner of the Host controller, Gadget controller and the OTG phy
and should take care of the that.

> 
> Besides, are you ok with using usb_gadget_vbus_connect/disconnect.

I don't think PHY is the right place for this even though older drivers seem to be doing so.
But at the same time there is nowhere else to add this at the moment.
The right place should be the USB core that is aware of host/gadget, phy and the state of the bus.

> 
>>
>> Scratching one's own itch kinda thing...
>>
>>>>>>> +static void hi6220_detect_work(struct work_struct *work)
>>>>>>> +{
>>>>>>> +    struct hi6220_priv *priv =
>>>>>>> +        container_of(work, struct hi6220_priv, work.work);
>>>>>>> +    int gpio_id, gpio_vbus;
>>>>>>> +    enum usb_otg_state state;
>>>>>>> +
>>>>>>> +    if (!gpio_is_valid(priv->gpio_id) || !gpio_is_valid(priv->gpio_vbus))
>>>>>>> +        return;
>>>>>>> +
>>>>>>> +    gpio_id = gpio_get_value_cansleep(priv->gpio_id);
>>>>>>> +    gpio_vbus = gpio_get_value_cansleep(priv->gpio_vbus);
>>>>>>
>>>>>> looks like this should be using extcon
>>>>> Not used extcon before.
>>>>> However, we need gpio_vbus interrupt.
>>>>> Checked phy-tahvo.c and phy-omap-otg.c, not find extcon related with
>>>>> interrupt.
>>>>> Will investigate tomorrow.
>>>>
>>>> drivers/extcon/extcon-gpio.c
>>> I think there is no need to use extcon, gpio is clear enough.
>>> extcon-gpio.c even do not support dt.
>>
>> well, add DT. The whole idea of free software is that we improve on
>> things we already have. EXTCON is *the* API to handle such things.
> 

I wrote the extcon-gpio-usb.c driver for exactly your use case. It is
queued for v4.1
https://lkml.org/lkml/2015/2/2/187

It takes care of debouncing for you. Although currently it supports only ID gpio,
it should be very easy to extend to VBUS sense GPIO.

> I think I am still not understanding extcon-gpio, not sure why need use this API here.

several reasons. Let me list a few.
1) Code reuse. Every PHY driver doesn't need to implement GPIO/interrupt handling and debouncing.
It just registers what cable events it wants to hear and gets a notification.
2) The events (ID/VBUS) are not only interesting for the PHY driver but also the controller
driver and the OTG state machine (whenever it exists at a common place) ;).
3) standardization because of common API.

> 
> Here two gpio requires, one gpio as interrupt, in the interrupt handler, we detect the gpio status judging the otg status.
> extcon-gpio.c use the interrupt, then can we also use the gpio interrupt.
> Using extcon-gpio is used for saving gpio_request?
> 
>>
>> Quite frankly, though, Roger Quadros (now in Cc) has been working to get
>> DT support on extcon-gpio, so perhaps wait for that and base your
>> patches on top of his.
>>
>> Now your statement that GPIO is clear enough is completely bogus to me.
>>
>> Why do we have fixed regulators with GPIO enable signals, right ? Might
>> as well just fiddle with the GPIO directly, right ? Wrong, the idea of
>> using these frameworks is to encapsulate implementation details and make
>> sure that if things change from one board to another, we can still use
>> our SW without major modifications.
>>
>>>>>>> +    if (gpio_vbus == 0) {
>>>>>>> +        if (gpio_id == 1)
>>>>>>> +            state = OTG_STATE_B_PERIPHERAL;
>>>>>>> +        else
>>>>>>> +            state = OTG_STATE_A_HOST;
>>>>>>> +    } else {
>>>>>>> +        state = OTG_STATE_A_HOST;
>>>>>>> +    }
>>>>>>> +
>>>>>>> +    if (priv->state != state) {
>>>>>>> +        hi6220_start_peripheral(priv, state == OTG_STATE_B_PERIPHERAL);
>>>>>>> +        priv->state = state;
>>>>>>> +    }
>>>>>>> +}
>>>>>>> +
>>>>>>> +static irqreturn_t hiusb_gpio_intr(int irq, void *data)
>>>>>>> +{
>>>>>>> +    struct hi6220_priv *priv = (struct hi6220_priv *)data;
>>>>>>> +
>>>>>>> +    /* add debounce time */
>>>>>>> +    schedule_delayed_work(&priv->work, msecs_to_jiffies(100));
>>>>>>
>>>>>> this is really bad. We have threaded interrupt support, right ?
>>>>>
>>>>> Since we use two gpio to distinguish gadget mode or host mode.
>>>>> Debounce time can introduce more accuracy.
>>>>
>>>> gpio_set_debounce() ?
>>> Not all gpio.c support set_debounce, including gpio-pl061.c.
>>
>> then the framework should implement it in SW. That was the whole idea of
>> adding set_debounce() to gpiolib. If the controller can't handle it,
>> gpiolib handles it in SW. Again, encapsulating details.
>>
>>>>> I think threaded interrupt can not be used for adding debounce time.
>>>>> Here add debounce is just for safety.
>>>>
>>>> add the debounce to the gpio itself.
>>>
>>> Here the debounce added only for safety.
>>> gpio_id may mis-report when unplug usb, but it is correct for plug usb & otg
>>> device.
>>> So debounce can be omitted.
>>> If you think using delayed work for debounce is ugly, it is fine switch to
>>> threaded_irq.
>>
>> debounce might be very well needed. We *do* want to filter out the
>> transient period. I'm just telling you there are better ways of doing
>> so; and your response to that is "let's just remove it" and I'm not
>> really comfortable with that attitude.
>>
>> That's the attitude of a lazy person which, I hope, you are not ;)
> 
> Understand.
> What I mean here is gpio_id is not used when unplug, it is only used after plug.
> 
> Thanks

cheers,
-roger

[PATCH v4 4/4] phy: add phy-hi6220-usb

[zhangfei]

Hi, Roger

On 02/24/2015 06:13 PM, Roger Quadros wrote:
>>> On Sat, Feb 21, 2015 at 11:03:05PM +0800, zhangfei wrote:
>>>>>>>> +static void hi6220_start_peripheral(struct hi6220_priv *priv, bool on)
>>>>>>>> +{
>>>>>>>> +    struct usb_otg *otg = priv->phy.otg;
>>>>>>>> +
>>>>>>>> +    if (!otg->gadget)
>>>>>>>> +        return;
>>>>>>>> +
>>>>>>>> +    if (on)
>>>>>>>> +        usb_gadget_connect(otg->gadget);
>>>>>>>> +    else
>>>>>>>> +        usb_gadget_disconnect(otg->gadget);
>>>>>>>
>>>>>>> why is the PHY fiddling with pullups ?
>>>>>>
>>>>>> We use this to enable/disable otg gadget mode.
>>>>>
>>>>> I got that, but the pullups don't belong to the PHY, they belong to the
>>>>> gadget.
>>>>>
>>>>>> The gpio_id & gpio_vbus are used to distinguish otg gadget mode or
>>>>>> host mode.
>>>>>> When micro usb or otg device attached to otg, gpio_vbus falling down.
>>>>>> And gpio_id = 1 is micro usb, gpio_id = 0 is otg device.
>>>>>
>>>>> all of that I understood clearly :-)
>>>>>
>>>>>> So when micro usb attached, we enable gadget mode; while micro usb
>>>>>> detached, we disable gadget mode, and dwc2 will automatically set to
>>>>>> host mode.
>>>>>
>>>>> that's all fine, I'm concerned about letting the PHY fiddle with
>>>>> something it doesn't own. If I am to change pullups rules in udc-core,
>>>>> this is likely to break down miserably and I don't want to have to go
>>>>> through that.
>>>>
>>>> Thanks for the clarifying.
>>>
>>> no problem.
>>>
>>>> How about using usb_gadget_vbus_connect/disconnect, which are used in many
>>>> files under drivers/usb/phy.
>>>> There is no vbus_session in dwc2/gadget.c, I thought it would be same as
>>>> pullup.
>>>>
>>>> However, usb_gadget_vbus_connect still need para gadget, where should we put
>>>> this file, drivers/usb/phy or drivers/phy
>>>
>>> drivers/phy, if the framework misses anything you need, it's a great
>>> opportunity to give back to the community by extending the framework.
>>
>> Sorry, I am a little confused.
>> I need some concrete suggestion for the next step of this patch, which is required for the community board, hikey board.
>>
>> Do you mean in the future we need use hsotg->phy instead of hsotg->uphy.
>>          struct phy *phy;
>>          struct usb_phy *uphy;
>> usb_phy has many members that struct phy does not have, including otg.
>> struct usb_otg          *otg;
>> Is that mean we need port such member from usb_phy to phy.
>
> In my opinion otg structure should belong to the USB core part that takes care
> of the OTG/DRD state machine. We still don't have a clear solution here and
> I'm currently investigating this.
> My current work is to get Dual role functionality working with DWC3 controller and TI
> platforms.
>
> Currently phy drivers take care of OTG operation themselves but there is an opportunity
> to share code and centralize USB role switching.
> The USB core should be the owner of the Host controller, Gadget controller and the OTG phy
> and should take care of the that.

Good idea.
If you have any patch, I will be very happy to verify.

How about adding these things in drivers/phy/phy-core.c, it is also 
sharable, though not in usb core.

Just tried adding one member struct usb_otg otg to struct phy, since not 
find any good member can hold usb_otg.
In drivers/phy/phy-core.c, adding extcon_register_interest, 
phy_vbus_notifier, phy_set_peripheral, it works for me, dwc2 on hikey board.

>
>>
>> Besides, are you ok with using usb_gadget_vbus_connect/disconnect.
>
> I don't think PHY is the right place for this even though older drivers seem to be doing so.
> But at the same time there is nowhere else to add this at the moment.
> The right place should be the USB core that is aware of host/gadget, phy and the state of the bus.

Understand.

>>> Scratching one's own itch kinda thing...
>>>
>>>>>>>> +static void hi6220_detect_work(struct work_struct *work)
>>>>>>>> +{
>>>>>>>> +    struct hi6220_priv *priv =
>>>>>>>> +        container_of(work, struct hi6220_priv, work.work);
>>>>>>>> +    int gpio_id, gpio_vbus;
>>>>>>>> +    enum usb_otg_state state;
>>>>>>>> +
>>>>>>>> +    if (!gpio_is_valid(priv->gpio_id) || !gpio_is_valid(priv->gpio_vbus))
>>>>>>>> +        return;
>>>>>>>> +
>>>>>>>> +    gpio_id = gpio_get_value_cansleep(priv->gpio_id);
>>>>>>>> +    gpio_vbus = gpio_get_value_cansleep(priv->gpio_vbus);
>>>>>>>
>>>>>>> looks like this should be using extcon
>>>>>> Not used extcon before.
>>>>>> However, we need gpio_vbus interrupt.
>>>>>> Checked phy-tahvo.c and phy-omap-otg.c, not find extcon related with
>>>>>> interrupt.
>>>>>> Will investigate tomorrow.
>>>>>
>>>>> drivers/extcon/extcon-gpio.c
>>>> I think there is no need to use extcon, gpio is clear enough.
>>>> extcon-gpio.c even do not support dt.
>>>
>>> well, add DT. The whole idea of free software is that we improve on
>>> things we already have. EXTCON is *the* API to handle such things.
>>
>
> I wrote the extcon-gpio-usb.c driver for exactly your use case. It is
> queued for v4.1
> https://lkml.org/lkml/2015/2/2/187

That's great, thanks.
>
> It takes care of debouncing for you. Although currently it supports only ID gpio,
> it should be very easy to extend to VBUS sense GPIO.
>
>> I think I am still not understanding extcon-gpio, not sure why need use this API here.
>
> several reasons. Let me list a few.
> 1) Code reuse. Every PHY driver doesn't need to implement GPIO/interrupt handling and debouncing.
> It just registers what cable events it wants to hear and gets a notification.
> 2) The events (ID/VBUS) are not only interesting for the PHY driver but also the controller
> driver and the OTG state machine (whenever it exists at a common place) ;).
> 3) standardization because of common API.

Thanks for the explanation.

Zhangfei

[PATCH v4 4/4] phy: add phy-hi6220-usb

[Roger Quadros]

Zhangfei,

On 26/02/15 10:48, zhangfei wrote:
> Hi, Roger
> 
> On 02/24/2015 06:13 PM, Roger Quadros wrote:
>>>> On Sat, Feb 21, 2015 at 11:03:05PM +0800, zhangfei wrote:
>>>>>>>>> +static void hi6220_start_peripheral(struct hi6220_priv *priv, bool on)
>>>>>>>>> +{
>>>>>>>>> +    struct usb_otg *otg = priv->phy.otg;
>>>>>>>>> +
>>>>>>>>> +    if (!otg->gadget)
>>>>>>>>> +        return;
>>>>>>>>> +
>>>>>>>>> +    if (on)
>>>>>>>>> +        usb_gadget_connect(otg->gadget);
>>>>>>>>> +    else
>>>>>>>>> +        usb_gadget_disconnect(otg->gadget);
>>>>>>>>
>>>>>>>> why is the PHY fiddling with pullups ?
>>>>>>>
>>>>>>> We use this to enable/disable otg gadget mode.
>>>>>>
>>>>>> I got that, but the pullups don't belong to the PHY, they belong to the
>>>>>> gadget.
>>>>>>
>>>>>>> The gpio_id & gpio_vbus are used to distinguish otg gadget mode or
>>>>>>> host mode.
>>>>>>> When micro usb or otg device attached to otg, gpio_vbus falling down.
>>>>>>> And gpio_id = 1 is micro usb, gpio_id = 0 is otg device.
>>>>>>
>>>>>> all of that I understood clearly :-)
>>>>>>
>>>>>>> So when micro usb attached, we enable gadget mode; while micro usb
>>>>>>> detached, we disable gadget mode, and dwc2 will automatically set to
>>>>>>> host mode.
>>>>>>
>>>>>> that's all fine, I'm concerned about letting the PHY fiddle with
>>>>>> something it doesn't own. If I am to change pullups rules in udc-core,
>>>>>> this is likely to break down miserably and I don't want to have to go
>>>>>> through that.
>>>>>
>>>>> Thanks for the clarifying.
>>>>
>>>> no problem.
>>>>
>>>>> How about using usb_gadget_vbus_connect/disconnect, which are used in many
>>>>> files under drivers/usb/phy.
>>>>> There is no vbus_session in dwc2/gadget.c, I thought it would be same as
>>>>> pullup.
>>>>>
>>>>> However, usb_gadget_vbus_connect still need para gadget, where should we put
>>>>> this file, drivers/usb/phy or drivers/phy
>>>>
>>>> drivers/phy, if the framework misses anything you need, it's a great
>>>> opportunity to give back to the community by extending the framework.
>>>
>>> Sorry, I am a little confused.
>>> I need some concrete suggestion for the next step of this patch, which is required for the community board, hikey board.
>>>
>>> Do you mean in the future we need use hsotg->phy instead of hsotg->uphy.
>>>          struct phy *phy;
>>>          struct usb_phy *uphy;
>>> usb_phy has many members that struct phy does not have, including otg.
>>> struct usb_otg          *otg;
>>> Is that mean we need port such member from usb_phy to phy.
>>
>> In my opinion otg structure should belong to the USB core part that takes care
>> of the OTG/DRD state machine. We still don't have a clear solution here and
>> I'm currently investigating this.
>> My current work is to get Dual role functionality working with DWC3 controller and TI
>> platforms.
>>
>> Currently phy drivers take care of OTG operation themselves but there is an opportunity
>> to share code and centralize USB role switching.
>> The USB core should be the owner of the Host controller, Gadget controller and the OTG phy
>> and should take care of the that.
> 
> Good idea.
> If you have any patch, I will be very happy to verify.

Not yet. but expect some RFC patches in a few days.

> 
> How about adding these things in drivers/phy/phy-core.c, it is also sharable, though not in usb core.

Not the best place IMO. Kishon can give his views if he sees it as a temporary solution.

> 
> Just tried adding one member struct usb_otg otg to struct phy, since not find any good member can hold usb_otg.
> In drivers/phy/phy-core.c, adding extcon_register_interest, phy_vbus_notifier, phy_set_peripheral, it works for me, dwc2 on hikey board.

cool.

> 
>>
>>>
>>> Besides, are you ok with using usb_gadget_vbus_connect/disconnect.
>>
>> I don't think PHY is the right place for this even though older drivers seem to be doing so.
>> But at the same time there is nowhere else to add this at the moment.
>> The right place should be the USB core that is aware of host/gadget, phy and the state of the bus.
> 
> Understand.
> 
>>>> Scratching one's own itch kinda thing...
>>>>
>>>>>>>>> +static void hi6220_detect_work(struct work_struct *work)
>>>>>>>>> +{
>>>>>>>>> +    struct hi6220_priv *priv =
>>>>>>>>> +        container_of(work, struct hi6220_priv, work.work);
>>>>>>>>> +    int gpio_id, gpio_vbus;
>>>>>>>>> +    enum usb_otg_state state;
>>>>>>>>> +
>>>>>>>>> +    if (!gpio_is_valid(priv->gpio_id) || !gpio_is_valid(priv->gpio_vbus))
>>>>>>>>> +        return;
>>>>>>>>> +
>>>>>>>>> +    gpio_id = gpio_get_value_cansleep(priv->gpio_id);
>>>>>>>>> +    gpio_vbus = gpio_get_value_cansleep(priv->gpio_vbus);
>>>>>>>>
>>>>>>>> looks like this should be using extcon
>>>>>>> Not used extcon before.
>>>>>>> However, we need gpio_vbus interrupt.
>>>>>>> Checked phy-tahvo.c and phy-omap-otg.c, not find extcon related with
>>>>>>> interrupt.
>>>>>>> Will investigate tomorrow.
>>>>>>
>>>>>> drivers/extcon/extcon-gpio.c
>>>>> I think there is no need to use extcon, gpio is clear enough.
>>>>> extcon-gpio.c even do not support dt.
>>>>
>>>> well, add DT. The whole idea of free software is that we improve on
>>>> things we already have. EXTCON is *the* API to handle such things.
>>>
>>
>> I wrote the extcon-gpio-usb.c driver for exactly your use case. It is
>> queued for v4.1
>> https://lkml.org/lkml/2015/2/2/187
> 
> That's great, thanks.
>>
>> It takes care of debouncing for you. Although currently it supports only ID gpio,
>> it should be very easy to extend to VBUS sense GPIO.
>>
>>> I think I am still not understanding extcon-gpio, not sure why need use this API here.
>>
>> several reasons. Let me list a few.
>> 1) Code reuse. Every PHY driver doesn't need to implement GPIO/interrupt handling and debouncing.
>> It just registers what cable events it wants to hear and gets a notification.
>> 2) The events (ID/VBUS) are not only interesting for the PHY driver but also the controller
>> driver and the OTG state machine (whenever it exists at a common place) ;).
>> 3) standardization because of common API.
> 
> Thanks for the explanation.

no problem :).

cheers,
-roger

[PATCH v4 4/4] phy: add phy-hi6220-usb

[Peter Chen]

On Thu, Feb 26, 2015 at 04:48:30PM +0800, zhangfei wrote:
> Hi, Roger
> 
> On 02/24/2015 06:13 PM, Roger Quadros wrote:
> >>>On Sat, Feb 21, 2015 at 11:03:05PM +0800, zhangfei wrote:
> >>>>>>>>+static void hi6220_start_peripheral(struct hi6220_priv *priv, bool on)
> >>>>>>>>+{
> >>>>>>>>+    struct usb_otg *otg = priv->phy.otg;
> >>>>>>>>+
> >>>>>>>>+    if (!otg->gadget)
> >>>>>>>>+        return;
> >>>>>>>>+
> >>>>>>>>+    if (on)
> >>>>>>>>+        usb_gadget_connect(otg->gadget);
> >>>>>>>>+    else
> >>>>>>>>+        usb_gadget_disconnect(otg->gadget);
> >>>>>>>
> >>>>>>>why is the PHY fiddling with pullups ?
> >>>>>>
> >>>>>>We use this to enable/disable otg gadget mode.
> >>>>>
> >>>>>I got that, but the pullups don't belong to the PHY, they belong to the
> >>>>>gadget.
> >>>>>
> >>>>>>The gpio_id & gpio_vbus are used to distinguish otg gadget mode or
> >>>>>>host mode.
> >>>>>>When micro usb or otg device attached to otg, gpio_vbus falling down.
> >>>>>>And gpio_id = 1 is micro usb, gpio_id = 0 is otg device.
> >>>>>
> >>>>>all of that I understood clearly :-)
> >>>>>
> >>>>>>So when micro usb attached, we enable gadget mode; while micro usb
> >>>>>>detached, we disable gadget mode, and dwc2 will automatically set to
> >>>>>>host mode.
> >>>>>
> >>>>>that's all fine, I'm concerned about letting the PHY fiddle with
> >>>>>something it doesn't own. If I am to change pullups rules in udc-core,
> >>>>>this is likely to break down miserably and I don't want to have to go
> >>>>>through that.
> >>>>
> >>>>Thanks for the clarifying.
> >>>
> >>>no problem.
> >>>
> >>>>How about using usb_gadget_vbus_connect/disconnect, which are used in many
> >>>>files under drivers/usb/phy.
> >>>>There is no vbus_session in dwc2/gadget.c, I thought it would be same as
> >>>>pullup.
> >>>>
> >>>>However, usb_gadget_vbus_connect still need para gadget, where should we put
> >>>>this file, drivers/usb/phy or drivers/phy
> >>>
> >>>drivers/phy, if the framework misses anything you need, it's a great
> >>>opportunity to give back to the community by extending the framework.
> >>
> >>Sorry, I am a little confused.
> >>I need some concrete suggestion for the next step of this patch, which is required for the community board, hikey board.
> >>
> >>Do you mean in the future we need use hsotg->phy instead of hsotg->uphy.
> >>         struct phy *phy;
> >>         struct usb_phy *uphy;
> >>usb_phy has many members that struct phy does not have, including otg.
> >>struct usb_otg          *otg;
> >>Is that mean we need port such member from usb_phy to phy.
> >
> >In my opinion otg structure should belong to the USB core part that takes care
> >of the OTG/DRD state machine. We still don't have a clear solution here and
> >I'm currently investigating this.
> >My current work is to get Dual role functionality working with DWC3 controller and TI
> >platforms.
> >
> >Currently phy drivers take care of OTG operation themselves but there is an opportunity
> >to share code and centralize USB role switching.
> >The USB core should be the owner of the Host controller, Gadget controller and the OTG phy
> >and should take care of the that.
> 
> Good idea.
> If you have any patch, I will be very happy to verify.
> 
> How about adding these things in drivers/phy/phy-core.c, it is also
> sharable, though not in usb core.
> 
> Just tried adding one member struct usb_otg otg to struct phy, since
> not find any good member can hold usb_otg.
> In drivers/phy/phy-core.c, adding extcon_register_interest,
> phy_vbus_notifier, phy_set_peripheral, it works for me, dwc2 on
> hikey board.

Just thinking if we can follow struct usb_hcd and struct ehci_hcd design
way, the generic phy just like hcd, and the usb phy like ehci hcd which
is a private data for hcd. zhangfei, maybe you can have a try.

-- 

Best Regards,
Peter Chen