* [PATCH v5 0/2] Add a new Rockchip usb2 phy driver
@ 2016-06-13 2:10 Frank Wang
2016-06-13 2:10 ` [PATCH v5 1/2] Documentation: bindings: add DT documentation for Rockchip USB2PHY Frank Wang
2016-06-13 2:10 ` [PATCH v5 2/2] phy: rockchip-inno-usb2: add a new driver for Rockchip usb2phy Frank Wang
0 siblings, 2 replies; 25+ messages in thread
From: Frank Wang @ 2016-06-13 2:10 UTC (permalink / raw)
To: heiko, dianders, linux, groeck, jwerner, kishon, robh+dt,
pawel.moll, mark.rutland, ijc+devicetree, galak
Cc: linux-kernel, devicetree, linux-usb, linux-rockchip, xzy.xu,
kever.yang, huangtao, william.wu, frank.wang
The newer SoCs (rk3366, rk3399) of Rock-chip take a different usb-phy
IP block than rk3288 and before, and most of phy-related registers are
also different from the past, so a new phy driver is required necessarily.
These series patches add phy-rockchip-inno-usb2.c and the corresponding
documentation.
Changes in v5:
- Added 'reg' property both in devicetree bindings and driver to match the different phy-blocks.
Changes in v4:
- Used 'phy-supply' instead of 'vbus_*-supply'.
Changes in v3:
- Supplemented some hardware-description into the devicetree bindings.
- Resolved the mapping defect between fixed value in driver and the property
in devicetree.
- Code cleanup.
Changes in v2:
- Specified more hardware-description into the devicetree bindings.
- Optimized some process of driver.
Frank Wang (2):
Documentation: bindings: add DT documentation for Rockchip USB2PHY
phy: rockchip-inno-usb2: add a new driver for Rockchip usb2phy
.../bindings/phy/phy-rockchip-inno-usb2.txt | 64 ++
drivers/phy/Kconfig | 7 +
drivers/phy/Makefile | 1 +
drivers/phy/phy-rockchip-inno-usb2.c | 645 ++++++++++++++++++++
4 files changed, 717 insertions(+)
create mode 100644 Documentation/devicetree/bindings/phy/phy-rockchip-inno-usb2.txt
create mode 100644 drivers/phy/phy-rockchip-inno-usb2.c
--
1.7.9.5
^ permalink raw reply [flat|nested] 25+ messages in thread
* [PATCH v5 1/2] Documentation: bindings: add DT documentation for Rockchip USB2PHY
2016-06-13 2:10 [PATCH v5 0/2] Add a new Rockchip usb2 phy driver Frank Wang
@ 2016-06-13 2:10 ` Frank Wang
2016-06-13 8:38 ` Heiko Stübner
2016-06-14 22:26 ` Rob Herring
2016-06-13 2:10 ` [PATCH v5 2/2] phy: rockchip-inno-usb2: add a new driver for Rockchip usb2phy Frank Wang
1 sibling, 2 replies; 25+ messages in thread
From: Frank Wang @ 2016-06-13 2:10 UTC (permalink / raw)
To: heiko, dianders, linux, groeck, jwerner, kishon, robh+dt,
pawel.moll, mark.rutland, ijc+devicetree, galak
Cc: linux-kernel, devicetree, linux-usb, linux-rockchip, xzy.xu,
kever.yang, huangtao, william.wu, frank.wang
Signed-off-by: Frank Wang <frank.wang@rock-chips.com>
---
Changes in v5:
- Added 'reg' property to identify the different phy-blocks.
Changes in v4:
- Used 'phy-supply' instead of 'vbus_*-supply'.
Changes in v3:
- Added 'clocks' and 'clock-names' optional properties.
- Specified 'otg-port' and 'host-port' as the sub-node name.
Changes in v2:
- Changed vbus_host optional property from gpio to regulator.
- Specified vbus_otg-supply optional property.
- Specified otg_id and otg_bvalid property.
.../bindings/phy/phy-rockchip-inno-usb2.txt | 64 ++++++++++++++++++++
1 file changed, 64 insertions(+)
create mode 100644 Documentation/devicetree/bindings/phy/phy-rockchip-inno-usb2.txt
diff --git a/Documentation/devicetree/bindings/phy/phy-rockchip-inno-usb2.txt b/Documentation/devicetree/bindings/phy/phy-rockchip-inno-usb2.txt
new file mode 100644
index 0000000..48bb5de
--- /dev/null
+++ b/Documentation/devicetree/bindings/phy/phy-rockchip-inno-usb2.txt
@@ -0,0 +1,64 @@
+ROCKCHIP USB2.0 PHY WITH INNO IP BLOCK
+
+Required properties (phy (parent) node):
+ - compatible : should be one of the listed compatibles:
+ * "rockchip,rk3366-usb2phy"
+ * "rockchip,rk3399-usb2phy"
+ - reg : the address offset of grf for usb-phy configuration.
+ - #clock-cells : should be 0.
+ - clock-output-names : specify the 480m output clock name.
+
+Optional properties:
+ - clocks : phandle + phy specifier pair, for the input clock of phy.
+ - clock-names : input clock name of phy, must be "phyclk".
+
+Required nodes : a sub-node is required for each port the phy provides.
+ The sub-node name is used to identify host or otg port,
+ and shall be the following entries:
+ * "otg-port" : the name of otg port.
+ * "host-port" : the name of host port.
+
+Required properties (port (child) node):
+ - #phy-cells : must be 0. See ./phy-bindings.txt for details.
+ - interrupts : specify an interrupt for each entry in interrupt-names.
+ - interrupt-names : a list which shall be the following entries:
+ * "otg_id" : for the otg id interrupt.
+ * "otg_bvalid" : for the otg vbus interrupt.
+ * "linestate" : for the host/otg linestate interrupt.
+
+Optional properties:
+ - phy-supply : phandle to a regulator that provides power to VBUS.
+ See ./phy-bindings.txt for details.
+
+Example:
+
+grf: syscon@ff770000 {
+ compatible = "rockchip,rk3366-grf", "syscon", "simple-mfd";
+ #address-cells = <1>;
+ #size-cells = <1>;
+
+...
+
+ u2phy: usb2-phy@700 {
+ compatible = "rockchip,rk3366-usb2phy";
+ reg = <0x700 0x2c>;
+ #clock-cells = <0>;
+ clock-output-names = "sclk_otgphy0_480m";
+
+ u2phy_otg: otg-port {
+ #phy-cells = <0>;
+ interrupts = <GIC_SPI 93 IRQ_TYPE_LEVEL_HIGH>,
+ <GIC_SPI 94 IRQ_TYPE_LEVEL_HIGH>,
+ <GIC_SPI 95 IRQ_TYPE_LEVEL_HIGH>;
+ interrupt-names = "otg_id", "otg_bvalid", "linestate";
+ status = "okay";
+ };
+
+ u2phy_host: host-port {
+ #phy-cells = <0>;
+ interrupts = <GIC_SPI 96 IRQ_TYPE_LEVEL_HIGH>;
+ interrupt-names = "linestate";
+ status = "okay";
+ };
+ };
+};
--
1.7.9.5
^ permalink raw reply related [flat|nested] 25+ messages in thread
* [PATCH v5 2/2] phy: rockchip-inno-usb2: add a new driver for Rockchip usb2phy
2016-06-13 2:10 [PATCH v5 0/2] Add a new Rockchip usb2 phy driver Frank Wang
2016-06-13 2:10 ` [PATCH v5 1/2] Documentation: bindings: add DT documentation for Rockchip USB2PHY Frank Wang
@ 2016-06-13 2:10 ` Frank Wang
2016-06-14 13:27 ` Heiko Stübner
` (2 more replies)
1 sibling, 3 replies; 25+ messages in thread
From: Frank Wang @ 2016-06-13 2:10 UTC (permalink / raw)
To: heiko, dianders, linux, groeck, jwerner, kishon, robh+dt,
pawel.moll, mark.rutland, ijc+devicetree, galak
Cc: linux-kernel, devicetree, linux-usb, linux-rockchip, xzy.xu,
kever.yang, huangtao, william.wu, frank.wang
The newer SoCs (rk3366, rk3399) take a different usb-phy IP block
than rk3288 and before, and most of phy-related registers are also
different from the past, so a new phy driver is required necessarily.
Signed-off-by: Frank Wang <frank.wang@rock-chips.com>
---
Changes in v5:
- Added 'reg' in the data block to match the different phy-blocks in dt.
Changes in v4:
- Removed some processes related to 'vbus_host-supply'.
Changes in v3:
- Resolved the mapping defect between fixed value in driver and the property
in devicetree.
- Optimized 480m output clock register function.
- Code cleanup.
Changes in v2:
- Changed vbus_host operation from gpio to regulator in *_probe.
- Improved the fault treatment relate to 480m clock register.
- Cleaned up some meaningless codes in *_clk480m_disable.
- made more clear the comment of *_sm_work.
drivers/phy/Kconfig | 7 +
drivers/phy/Makefile | 1 +
drivers/phy/phy-rockchip-inno-usb2.c | 645 ++++++++++++++++++++++++++++++++++
3 files changed, 653 insertions(+)
create mode 100644 drivers/phy/phy-rockchip-inno-usb2.c
diff --git a/drivers/phy/Kconfig b/drivers/phy/Kconfig
index b869b98..29ef15c 100644
--- a/drivers/phy/Kconfig
+++ b/drivers/phy/Kconfig
@@ -347,6 +347,13 @@ config PHY_ROCKCHIP_USB
help
Enable this to support the Rockchip USB 2.0 PHY.
+config PHY_ROCKCHIP_INNO_USB2
+ tristate "Rockchip INNO USB2PHY Driver"
+ depends on ARCH_ROCKCHIP && OF
+ select GENERIC_PHY
+ help
+ Support for Rockchip USB2.0 PHY with Innosilicon IP block.
+
config PHY_ROCKCHIP_EMMC
tristate "Rockchip EMMC PHY Driver"
depends on ARCH_ROCKCHIP && OF
diff --git a/drivers/phy/Makefile b/drivers/phy/Makefile
index 9c3e73c..4963fbc 100644
--- a/drivers/phy/Makefile
+++ b/drivers/phy/Makefile
@@ -38,6 +38,7 @@ phy-exynos-usb2-$(CONFIG_PHY_S5PV210_USB2) += phy-s5pv210-usb2.o
obj-$(CONFIG_PHY_EXYNOS5_USBDRD) += phy-exynos5-usbdrd.o
obj-$(CONFIG_PHY_QCOM_APQ8064_SATA) += phy-qcom-apq8064-sata.o
obj-$(CONFIG_PHY_ROCKCHIP_USB) += phy-rockchip-usb.o
+obj-$(CONFIG_PHY_ROCKCHIP_INNO_USB2) += phy-rockchip-inno-usb2.o
obj-$(CONFIG_PHY_ROCKCHIP_EMMC) += phy-rockchip-emmc.o
obj-$(CONFIG_PHY_ROCKCHIP_DP) += phy-rockchip-dp.o
obj-$(CONFIG_PHY_QCOM_IPQ806X_SATA) += phy-qcom-ipq806x-sata.o
diff --git a/drivers/phy/phy-rockchip-inno-usb2.c b/drivers/phy/phy-rockchip-inno-usb2.c
new file mode 100644
index 0000000..fc599c7
--- /dev/null
+++ b/drivers/phy/phy-rockchip-inno-usb2.c
@@ -0,0 +1,645 @@
+/*
+ * Rockchip USB2.0 PHY with Innosilicon IP block driver
+ *
+ * Copyright (C) 2016 Fuzhou Rockchip Electronics Co., Ltd
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+ * GNU General Public License for more details.
+ */
+
+#include <linux/clk.h>
+#include <linux/clk-provider.h>
+#include <linux/delay.h>
+#include <linux/interrupt.h>
+#include <linux/io.h>
+#include <linux/gpio/consumer.h>
+#include <linux/jiffies.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/mutex.h>
+#include <linux/of.h>
+#include <linux/of_address.h>
+#include <linux/of_irq.h>
+#include <linux/of_platform.h>
+#include <linux/phy/phy.h>
+#include <linux/platform_device.h>
+#include <linux/regmap.h>
+#include <linux/mfd/syscon.h>
+
+#define BIT_WRITEABLE_SHIFT 16
+#define SCHEDULE_DELAY (60 * HZ)
+
+enum rockchip_usb2phy_port_id {
+ USB2PHY_PORT_OTG,
+ USB2PHY_PORT_HOST,
+ USB2PHY_NUM_PORTS,
+};
+
+enum rockchip_usb2phy_host_state {
+ PHY_STATE_HS_ONLINE = 0,
+ PHY_STATE_DISCONNECT = 1,
+ PHY_STATE_HS_CONNECT = 2,
+ PHY_STATE_FS_CONNECT = 4,
+};
+
+struct usb2phy_reg {
+ unsigned int offset;
+ unsigned int bitend;
+ unsigned int bitstart;
+ unsigned int disable;
+ unsigned int enable;
+};
+
+/**
+ * struct rockchip_usb2phy_port_cfg: usb-phy port configuration.
+ * @phy_sus: phy suspend register.
+ * @ls_det_en: linestate detection enable register.
+ * @ls_det_st: linestate detection state register.
+ * @ls_det_clr: linestate detection clear register.
+ * @utmi_ls: utmi linestate state register.
+ * @utmi_hstdet: utmi host disconnect register.
+ */
+struct rockchip_usb2phy_port_cfg {
+ struct usb2phy_reg phy_sus;
+ struct usb2phy_reg ls_det_en;
+ struct usb2phy_reg ls_det_st;
+ struct usb2phy_reg ls_det_clr;
+ struct usb2phy_reg utmi_ls;
+ struct usb2phy_reg utmi_hstdet;
+};
+
+/**
+ * struct rockchip_usb2phy_cfg: usb-phy configuration.
+ * @reg: the address offset of grf for usb-phy config.
+ * @num_ports: specify how many ports that the phy has.
+ * @clkout_ctl: keep on/turn off output clk of phy.
+ */
+struct rockchip_usb2phy_cfg {
+ unsigned int reg;
+ unsigned int num_ports;
+ struct usb2phy_reg clkout_ctl;
+ const struct rockchip_usb2phy_port_cfg port_cfgs[USB2PHY_NUM_PORTS];
+};
+
+/**
+ * struct rockchip_usb2phy_port: usb-phy port data.
+ * @port_id: flag for otg port or host port.
+ * @suspended: phy suspended flag.
+ * @ls_irq: IRQ number assigned for linestate detection.
+ * @mutex: for register updating in sm_work.
+ * @sm_work: OTG state machine work.
+ * @phy_cfg: port register configuration, assigned by driver data.
+ */
+struct rockchip_usb2phy_port {
+ struct phy *phy;
+ unsigned int port_id;
+ bool suspended;
+ int ls_irq;
+ struct mutex mutex;
+ struct delayed_work sm_work;
+ const struct rockchip_usb2phy_port_cfg *port_cfg;
+};
+
+/**
+ * struct rockchip_usb2phy: usb2.0 phy driver data.
+ * @grf: General Register Files regmap.
+ * @clk: clock struct of phy input clk.
+ * @clk480m: clock struct of phy output clk.
+ * @clk_hw: clock struct of phy output clk management.
+ * @phy_cfg: phy register configuration, assigned by driver data.
+ * @ports: phy port instance.
+ */
+struct rockchip_usb2phy {
+ struct device *dev;
+ struct regmap *grf;
+ struct clk *clk;
+ struct clk *clk480m;
+ struct clk_hw clk480m_hw;
+ const struct rockchip_usb2phy_cfg *phy_cfg;
+ struct rockchip_usb2phy_port ports[USB2PHY_NUM_PORTS];
+};
+
+static inline int property_enable(struct rockchip_usb2phy *rphy,
+ const struct usb2phy_reg *reg, bool en)
+{
+ unsigned int val, mask, tmp;
+
+ tmp = en ? reg->enable : reg->disable;
+ mask = GENMASK(reg->bitend, reg->bitstart);
+ val = (tmp << reg->bitstart) | (mask << BIT_WRITEABLE_SHIFT);
+
+ return regmap_write(rphy->grf, reg->offset, val);
+}
+
+static inline bool property_enabled(struct rockchip_usb2phy *rphy,
+ const struct usb2phy_reg *reg)
+{
+ int ret;
+ unsigned int tmp, orig;
+ unsigned int mask = GENMASK(reg->bitend, reg->bitstart);
+
+ ret = regmap_read(rphy->grf, reg->offset, &orig);
+ if (ret)
+ return false;
+
+ tmp = (orig & mask) >> reg->bitstart;
+ return tmp == reg->enable;
+}
+
+static int rockchip_usb2phy_clk480m_enable(struct clk_hw *hw)
+{
+ struct rockchip_usb2phy *rphy =
+ container_of(hw, struct rockchip_usb2phy, clk480m_hw);
+ int ret;
+
+ /* turn on 480m clk output if it is off */
+ if (!property_enabled(rphy, &rphy->phy_cfg->clkout_ctl)) {
+ ret = property_enable(rphy, &rphy->phy_cfg->clkout_ctl, true);
+ if (ret)
+ return ret;
+
+ /* waitting for the clk become stable */
+ mdelay(1);
+ }
+
+ return 0;
+}
+
+static void rockchip_usb2phy_clk480m_disable(struct clk_hw *hw)
+{
+ struct rockchip_usb2phy *rphy =
+ container_of(hw, struct rockchip_usb2phy, clk480m_hw);
+
+ /* turn off 480m clk output */
+ property_enable(rphy, &rphy->phy_cfg->clkout_ctl, false);
+}
+
+static int rockchip_usb2phy_clk480m_enabled(struct clk_hw *hw)
+{
+ struct rockchip_usb2phy *rphy =
+ container_of(hw, struct rockchip_usb2phy, clk480m_hw);
+
+ return property_enabled(rphy, &rphy->phy_cfg->clkout_ctl);
+}
+
+static unsigned long
+rockchip_usb2phy_clk480m_recalc_rate(struct clk_hw *hw,
+ unsigned long parent_rate)
+{
+ return 480000000;
+}
+
+static const struct clk_ops rockchip_usb2phy_clkout_ops = {
+ .enable = rockchip_usb2phy_clk480m_enable,
+ .disable = rockchip_usb2phy_clk480m_disable,
+ .is_enabled = rockchip_usb2phy_clk480m_enabled,
+ .recalc_rate = rockchip_usb2phy_clk480m_recalc_rate,
+};
+
+static void rockchip_usb2phy_clk480m_unregister(void *data)
+{
+ struct rockchip_usb2phy *rphy = data;
+
+ of_clk_del_provider(rphy->dev->of_node);
+ clk_unregister(rphy->clk480m);
+
+ if (rphy->clk)
+ clk_put(rphy->clk);
+}
+
+static int
+rockchip_usb2phy_clk480m_register(struct rockchip_usb2phy *rphy)
+{
+ struct device_node *node = rphy->dev->of_node;
+ struct clk_init_data init;
+ const char *clk_name;
+ int ret;
+
+ init.flags = 0;
+ init.name = "clk_usbphy_480m";
+ init.ops = &rockchip_usb2phy_clkout_ops;
+
+ /* optional override of the clockname */
+ of_property_read_string(node, "clock-output-names", &init.name);
+
+ rphy->clk = of_clk_get_by_name(node, "phyclk");
+ if (IS_ERR(rphy->clk)) {
+ rphy->clk = NULL;
+ init.parent_names = NULL;
+ init.num_parents = 0;
+ } else {
+ clk_name = __clk_get_name(rphy->clk);
+ init.parent_names = &clk_name;
+ init.num_parents = 1;
+ }
+
+ rphy->clk480m_hw.init = &init;
+
+ /* register the clock */
+ rphy->clk480m = clk_register(rphy->dev, &rphy->clk480m_hw);
+ if (IS_ERR(rphy->clk480m)) {
+ ret = PTR_ERR(rphy->clk480m);
+ goto err_register;
+ }
+
+ ret = of_clk_add_provider(node, of_clk_src_simple_get, rphy->clk480m);
+ if (ret < 0)
+ goto err_clk_provider;
+
+ ret = devm_add_action(rphy->dev, rockchip_usb2phy_clk480m_unregister,
+ rphy);
+ if (ret < 0)
+ goto err_unreg_action;
+
+ return 0;
+
+err_unreg_action:
+ of_clk_del_provider(node);
+err_clk_provider:
+ clk_unregister(rphy->clk480m);
+err_register:
+ if (rphy->clk)
+ clk_put(rphy->clk);
+ return ret;
+}
+
+static int rockchip_usb2phy_init(struct phy *phy)
+{
+ struct rockchip_usb2phy_port *rport = phy_get_drvdata(phy);
+ struct rockchip_usb2phy *rphy = dev_get_drvdata(phy->dev.parent);
+ int ret;
+
+ if (rport->port_id == USB2PHY_PORT_HOST) {
+ /* clear linestate and enable linestate detect irq */
+ mutex_lock(&rport->mutex);
+
+ ret = property_enable(rphy, &rport->port_cfg->ls_det_clr, true);
+ if (ret) {
+ mutex_unlock(&rport->mutex);
+ return ret;
+ }
+
+ ret = property_enable(rphy, &rport->port_cfg->ls_det_en, true);
+ if (ret) {
+ mutex_unlock(&rport->mutex);
+ return ret;
+ }
+
+ mutex_unlock(&rport->mutex);
+ schedule_delayed_work(&rport->sm_work, SCHEDULE_DELAY);
+ }
+
+ return 0;
+}
+
+static int rockchip_usb2phy_resume(struct phy *phy)
+{
+ struct rockchip_usb2phy_port *rport = phy_get_drvdata(phy);
+ struct rockchip_usb2phy *rphy = dev_get_drvdata(phy->dev.parent);
+ int ret;
+
+ dev_dbg(&rport->phy->dev, "port resume\n");
+
+ ret = clk_prepare_enable(rphy->clk480m);
+ if (ret)
+ return ret;
+
+ ret = property_enable(rphy, &rport->port_cfg->phy_sus, false);
+ if (ret)
+ return ret;
+
+ rport->suspended = false;
+ return 0;
+}
+
+static int rockchip_usb2phy_suspend(struct phy *phy)
+{
+ struct rockchip_usb2phy_port *rport = phy_get_drvdata(phy);
+ struct rockchip_usb2phy *rphy = dev_get_drvdata(phy->dev.parent);
+ int ret;
+
+ dev_dbg(&rport->phy->dev, "port suspend\n");
+
+ ret = property_enable(rphy, &rport->port_cfg->phy_sus, true);
+ if (ret)
+ return ret;
+
+ rport->suspended = true;
+ clk_disable_unprepare(rphy->clk480m);
+ return 0;
+}
+
+static int rockchip_usb2phy_exit(struct phy *phy)
+{
+ struct rockchip_usb2phy_port *rport = phy_get_drvdata(phy);
+
+ if (rport->port_id == USB2PHY_PORT_HOST)
+ cancel_delayed_work_sync(&rport->sm_work);
+
+ return 0;
+}
+
+static const struct phy_ops rockchip_usb2phy_ops = {
+ .init = rockchip_usb2phy_init,
+ .exit = rockchip_usb2phy_exit,
+ .power_on = rockchip_usb2phy_resume,
+ .power_off = rockchip_usb2phy_suspend,
+ .owner = THIS_MODULE,
+};
+
+/*
+ * The function manage host-phy port state and suspend/resume phy port
+ * to save power.
+ *
+ * we rely on utmi_linestate and utmi_hostdisconnect to identify whether
+ * FS/HS is disconnect or not. Besides, we do not need care it is FS
+ * disconnected or HS disconnected, actually, we just only need get the
+ * device is disconnected at last through rearm the delayed work,
+ * to suspend the phy port in _PHY_STATE_DISCONNECT_ case.
+ *
+ * NOTE: It may invoke *phy_suspend or *phy_resume which will invoke some
+ * clk related APIs, so do not invoke it from interrupt context directly.
+ */
+static void rockchip_usb2phy_sm_work(struct work_struct *work)
+{
+ struct rockchip_usb2phy_port *rport =
+ container_of(work, struct rockchip_usb2phy_port, sm_work.work);
+ struct rockchip_usb2phy *rphy = dev_get_drvdata(rport->phy->dev.parent);
+ unsigned int sh = rport->port_cfg->utmi_hstdet.bitend -
+ rport->port_cfg->utmi_hstdet.bitstart + 1;
+ unsigned int ul, uhd, state;
+ unsigned int ul_mask, uhd_mask;
+ int ret;
+
+ mutex_lock(&rport->mutex);
+
+ ret = regmap_read(rphy->grf, rport->port_cfg->utmi_ls.offset, &ul);
+ if (ret < 0)
+ goto next_schedule;
+
+ ret = regmap_read(rphy->grf, rport->port_cfg->utmi_hstdet.offset,
+ &uhd);
+ if (ret < 0)
+ goto next_schedule;
+
+ uhd_mask = GENMASK(rport->port_cfg->utmi_hstdet.bitend,
+ rport->port_cfg->utmi_hstdet.bitstart);
+ ul_mask = GENMASK(rport->port_cfg->utmi_ls.bitend,
+ rport->port_cfg->utmi_ls.bitstart);
+
+ /* stitch on utmi_ls and utmi_hstdet as phy state */
+ state = ((uhd & uhd_mask) >> rport->port_cfg->utmi_hstdet.bitstart) |
+ (((ul & ul_mask) >> rport->port_cfg->utmi_ls.bitstart) << sh);
+
+ switch (state) {
+ case PHY_STATE_HS_ONLINE:
+ dev_dbg(&rport->phy->dev, "HS online\n");
+ break;
+ case PHY_STATE_FS_CONNECT:
+ /*
+ * For FS device, the online state share with connect state
+ * from utmi_ls and utmi_hstdet register, so we distinguish
+ * them via suspended flag.
+ */
+ if (!rport->suspended) {
+ dev_dbg(&rport->phy->dev, "FS online\n");
+ break;
+ }
+ /* fall through */
+ case PHY_STATE_HS_CONNECT:
+ if (rport->suspended) {
+ dev_dbg(&rport->phy->dev, "HS/FS connected\n");
+ rockchip_usb2phy_resume(rport->phy);
+ rport->suspended = false;
+ }
+ break;
+ case PHY_STATE_DISCONNECT:
+ if (!rport->suspended) {
+ dev_dbg(&rport->phy->dev, "HS/FS disconnected\n");
+ rockchip_usb2phy_suspend(rport->phy);
+ rport->suspended = true;
+ }
+
+ /*
+ * activate the linestate detection to get the next device
+ * plug-in irq.
+ */
+ property_enable(rphy, &rport->port_cfg->ls_det_clr, true);
+ property_enable(rphy, &rport->port_cfg->ls_det_en, true);
+
+ /*
+ * we don't need to rearm the delayed work when the phy port
+ * is suspended.
+ */
+ mutex_unlock(&rport->mutex);
+ return;
+ default:
+ dev_dbg(&rport->phy->dev, "unknown phy state\n");
+ break;
+ }
+
+next_schedule:
+ mutex_unlock(&rport->mutex);
+ schedule_delayed_work(&rport->sm_work, SCHEDULE_DELAY);
+}
+
+static irqreturn_t rockchip_usb2phy_linestate_irq(int irq, void *data)
+{
+ struct rockchip_usb2phy_port *rport = data;
+ struct rockchip_usb2phy *rphy = dev_get_drvdata(rport->phy->dev.parent);
+
+ if (!property_enabled(rphy, &rport->port_cfg->ls_det_st))
+ return IRQ_NONE;
+
+ mutex_lock(&rport->mutex);
+
+ /* disable linestate detect irq and clear its status */
+ property_enable(rphy, &rport->port_cfg->ls_det_en, false);
+ property_enable(rphy, &rport->port_cfg->ls_det_clr, true);
+
+ mutex_unlock(&rport->mutex);
+
+ /*
+ * In this case for host phy port, a new device is plugged in,
+ * meanwhile, if the phy port is suspended, we need rearm the work to
+ * resume it and mange its states; otherwise, we do nothing about that.
+ */
+ if (rport->suspended && rport->port_id == USB2PHY_PORT_HOST)
+ rockchip_usb2phy_sm_work(&rport->sm_work.work);
+
+ return IRQ_HANDLED;
+}
+
+static int rockchip_usb2phy_host_port_init(struct rockchip_usb2phy *rphy,
+ struct rockchip_usb2phy_port *rport,
+ struct device_node *child_np)
+{
+ int ret;
+
+ rport->port_id = USB2PHY_PORT_HOST;
+ rport->port_cfg = &rphy->phy_cfg->port_cfgs[USB2PHY_PORT_HOST];
+
+ mutex_init(&rport->mutex);
+ INIT_DELAYED_WORK(&rport->sm_work, rockchip_usb2phy_sm_work);
+
+ rport->ls_irq = of_irq_get_byname(child_np, "linestate");
+ if (rport->ls_irq < 0) {
+ dev_err(rphy->dev, "no linestate irq provided\n");
+ return rport->ls_irq;
+ }
+
+ ret = devm_request_threaded_irq(rphy->dev, rport->ls_irq, NULL,
+ rockchip_usb2phy_linestate_irq,
+ IRQF_ONESHOT,
+ "rockchip_usb2phy", rport);
+ if (ret) {
+ dev_err(rphy->dev, "failed to request irq handle\n");
+ return ret;
+ }
+
+ return 0;
+}
+
+static int rockchip_usb2phy_probe(struct platform_device *pdev)
+{
+ struct device *dev = &pdev->dev;
+ struct device_node *np = dev->of_node;
+ struct device_node *child_np;
+ struct phy_provider *provider;
+ struct rockchip_usb2phy *rphy;
+ const struct rockchip_usb2phy_cfg *phy_cfgs;
+ const struct of_device_id *match;
+ unsigned int reg;
+ int index, ret;
+
+ rphy = devm_kzalloc(dev, sizeof(*rphy), GFP_KERNEL);
+ if (!rphy)
+ return -ENOMEM;
+
+ match = of_match_device(dev->driver->of_match_table, dev);
+ if (!match || !match->data) {
+ dev_err(dev, "phy configs are not assigned!\n");
+ return -EINVAL;
+ }
+
+ if (!dev->parent || !dev->parent->of_node)
+ return -EINVAL;
+
+ rphy->grf = syscon_node_to_regmap(dev->parent->of_node);
+ if (IS_ERR(rphy->grf))
+ return PTR_ERR(rphy->grf);
+
+ if (of_property_read_u32(np, "reg", ®)) {
+ dev_err(dev, "the reg property is not assigned in %s node\n",
+ np->name);
+ return -EINVAL;
+ }
+
+ rphy->dev = dev;
+ phy_cfgs = match->data;
+ platform_set_drvdata(pdev, rphy);
+
+ /* find out a proper config which can be matched with dt. */
+ index = 0;
+ while (phy_cfgs[index].reg) {
+ if (phy_cfgs[index].reg == reg) {
+ rphy->phy_cfg = &phy_cfgs[index];
+ break;
+ }
+
+ ++index;
+ }
+
+ if (!rphy->phy_cfg) {
+ dev_err(dev, "no phy-config can be matched with %s node\n",
+ np->name);
+ return -EINVAL;
+ }
+
+ ret = rockchip_usb2phy_clk480m_register(rphy);
+ if (ret) {
+ dev_err(dev, "failed to register 480m output clock\n");
+ return ret;
+ }
+
+ index = 0;
+ for_each_available_child_of_node(np, child_np) {
+ struct rockchip_usb2phy_port *rport = &rphy->ports[index];
+ struct phy *phy;
+
+ phy = devm_phy_create(dev, child_np, &rockchip_usb2phy_ops);
+ if (IS_ERR(phy)) {
+ dev_err(dev, "failed to create phy\n");
+ ret = PTR_ERR(phy);
+ goto put_child;
+ }
+
+ rport->phy = phy;
+
+ /* initialize otg/host port separately */
+ if (!of_node_cmp(child_np->name, "host-port")) {
+ ret = rockchip_usb2phy_host_port_init(rphy, rport,
+ child_np);
+ if (ret)
+ goto put_child;
+ }
+
+ phy_set_drvdata(rport->phy, rport);
+
+ /* to prevent out of boundary */
+ if (++index >= rphy->phy_cfg->num_ports)
+ break;
+ }
+
+ provider = devm_of_phy_provider_register(dev, of_phy_simple_xlate);
+ return PTR_ERR_OR_ZERO(provider);
+
+put_child:
+ of_node_put(child_np);
+ return ret;
+}
+
+static const struct rockchip_usb2phy_cfg rk3366_phy_cfgs[] = {
+ {
+ .reg = 0x700,
+ .num_ports = 2,
+ .clkout_ctl = { 0x0724, 15, 15, 1, 0 },
+ .port_cfgs = {
+ [USB2PHY_PORT_HOST] = {
+ .phy_sus = { 0x0728, 15, 0, 0, 0x1d1 },
+ .ls_det_en = { 0x0680, 4, 4, 0, 1 },
+ .ls_det_st = { 0x0690, 4, 4, 0, 1 },
+ .ls_det_clr = { 0x06a0, 4, 4, 0, 1 },
+ .utmi_ls = { 0x049c, 14, 13, 0, 1 },
+ .utmi_hstdet = { 0x049c, 12, 12, 0, 1 }
+ }
+ },
+ },
+ { /* sentinel */ }
+};
+
+static const struct of_device_id rockchip_usb2phy_dt_match[] = {
+ { .compatible = "rockchip,rk3366-usb2phy", .data = &rk3366_phy_cfgs },
+ {}
+};
+MODULE_DEVICE_TABLE(of, rockchip_usb2phy_dt_match);
+
+static struct platform_driver rockchip_usb2phy_driver = {
+ .probe = rockchip_usb2phy_probe,
+ .driver = {
+ .name = "rockchip-usb2phy",
+ .of_match_table = rockchip_usb2phy_dt_match,
+ },
+};
+module_platform_driver(rockchip_usb2phy_driver);
+
+MODULE_AUTHOR("Frank Wang <frank.wang@rock-chips.com>");
+MODULE_DESCRIPTION("Rockchip USB2.0 PHY driver");
+MODULE_LICENSE("GPL v2");
--
1.7.9.5
^ permalink raw reply related [flat|nested] 25+ messages in thread
* Re: [PATCH v5 1/2] Documentation: bindings: add DT documentation for Rockchip USB2PHY
2016-06-13 2:10 ` [PATCH v5 1/2] Documentation: bindings: add DT documentation for Rockchip USB2PHY Frank Wang
@ 2016-06-13 8:38 ` Heiko Stübner
2016-06-14 13:28 ` Heiko Stübner
2016-06-14 22:26 ` Rob Herring
1 sibling, 1 reply; 25+ messages in thread
From: Heiko Stübner @ 2016-06-13 8:38 UTC (permalink / raw)
To: Frank Wang
Cc: dianders, linux, groeck, jwerner, kishon, robh+dt, pawel.moll,
mark.rutland, ijc+devicetree, galak, linux-kernel, devicetree,
linux-usb, linux-rockchip, xzy.xu, kever.yang, huangtao,
william.wu
Am Montag, 13. Juni 2016, 10:10:09 schrieb Frank Wang:
> Signed-off-by: Frank Wang <frank.wang@rock-chips.com>
looks really cool now, thanks for addressing all the review comments
Reviewed-by: Heiko Stuebner <heiko@sntech.de>
> ---
>
> Changes in v5:
> - Added 'reg' property to identify the different phy-blocks.
>
> Changes in v4:
> - Used 'phy-supply' instead of 'vbus_*-supply'.
>
> Changes in v3:
> - Added 'clocks' and 'clock-names' optional properties.
> - Specified 'otg-port' and 'host-port' as the sub-node name.
>
> Changes in v2:
> - Changed vbus_host optional property from gpio to regulator.
> - Specified vbus_otg-supply optional property.
> - Specified otg_id and otg_bvalid property.
>
> .../bindings/phy/phy-rockchip-inno-usb2.txt | 64
> ++++++++++++++++++++ 1 file changed, 64 insertions(+)
> create mode 100644
> Documentation/devicetree/bindings/phy/phy-rockchip-inno-usb2.txt
>
> diff --git
> a/Documentation/devicetree/bindings/phy/phy-rockchip-inno-usb2.txt
> b/Documentation/devicetree/bindings/phy/phy-rockchip-inno-usb2.txt new file
> mode 100644
> index 0000000..48bb5de
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/phy/phy-rockchip-inno-usb2.txt
> @@ -0,0 +1,64 @@
> +ROCKCHIP USB2.0 PHY WITH INNO IP BLOCK
> +
> +Required properties (phy (parent) node):
> + - compatible : should be one of the listed compatibles:
> + * "rockchip,rk3366-usb2phy"
> + * "rockchip,rk3399-usb2phy"
> + - reg : the address offset of grf for usb-phy configuration.
> + - #clock-cells : should be 0.
> + - clock-output-names : specify the 480m output clock name.
> +
> +Optional properties:
> + - clocks : phandle + phy specifier pair, for the input clock of phy.
> + - clock-names : input clock name of phy, must be "phyclk".
> +
> +Required nodes : a sub-node is required for each port the phy provides.
> + The sub-node name is used to identify host or otg port,
> + and shall be the following entries:
> + * "otg-port" : the name of otg port.
> + * "host-port" : the name of host port.
> +
> +Required properties (port (child) node):
> + - #phy-cells : must be 0. See ./phy-bindings.txt for details.
> + - interrupts : specify an interrupt for each entry in interrupt-names.
> + - interrupt-names : a list which shall be the following entries:
> + * "otg_id" : for the otg id interrupt.
> + * "otg_bvalid" : for the otg vbus interrupt.
> + * "linestate" : for the host/otg linestate interrupt.
> +
> +Optional properties:
> + - phy-supply : phandle to a regulator that provides power to VBUS.
> + See ./phy-bindings.txt for details.
> +
> +Example:
> +
> +grf: syscon@ff770000 {
> + compatible = "rockchip,rk3366-grf", "syscon", "simple-mfd";
> + #address-cells = <1>;
> + #size-cells = <1>;
> +
> +...
> +
> + u2phy: usb2-phy@700 {
> + compatible = "rockchip,rk3366-usb2phy";
> + reg = <0x700 0x2c>;
> + #clock-cells = <0>;
> + clock-output-names = "sclk_otgphy0_480m";
> +
> + u2phy_otg: otg-port {
> + #phy-cells = <0>;
> + interrupts = <GIC_SPI 93 IRQ_TYPE_LEVEL_HIGH>,
> + <GIC_SPI 94 IRQ_TYPE_LEVEL_HIGH>,
> + <GIC_SPI 95 IRQ_TYPE_LEVEL_HIGH>;
> + interrupt-names = "otg_id", "otg_bvalid", "linestate";
> + status = "okay";
> + };
> +
> + u2phy_host: host-port {
> + #phy-cells = <0>;
> + interrupts = <GIC_SPI 96 IRQ_TYPE_LEVEL_HIGH>;
> + interrupt-names = "linestate";
> + status = "okay";
> + };
> + };
> +};
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH v5 2/2] phy: rockchip-inno-usb2: add a new driver for Rockchip usb2phy
2016-06-13 2:10 ` [PATCH v5 2/2] phy: rockchip-inno-usb2: add a new driver for Rockchip usb2phy Frank Wang
@ 2016-06-14 13:27 ` Heiko Stübner
2016-06-14 13:50 ` Guenter Roeck
2016-06-15 3:23 ` Frank Wang
2016-06-14 14:52 ` Guenter Roeck
2016-06-17 11:54 ` Kishon Vijay Abraham I
2 siblings, 2 replies; 25+ messages in thread
From: Heiko Stübner @ 2016-06-14 13:27 UTC (permalink / raw)
To: Frank Wang
Cc: dianders, linux, groeck, jwerner, kishon, robh+dt, pawel.moll,
mark.rutland, ijc+devicetree, galak, linux-kernel, devicetree,
linux-usb, linux-rockchip, xzy.xu, kever.yang, huangtao,
william.wu
Am Montag, 13. Juni 2016, 10:10:10 schrieb Frank Wang:
> The newer SoCs (rk3366, rk3399) take a different usb-phy IP block
> than rk3288 and before, and most of phy-related registers are also
> different from the past, so a new phy driver is required necessarily.
>
> Signed-off-by: Frank Wang <frank.wang@rock-chips.com>
> ---
>
> Changes in v5:
> - Added 'reg' in the data block to match the different phy-blocks in dt.
>
> Changes in v4:
> - Removed some processes related to 'vbus_host-supply'.
>
> Changes in v3:
> - Resolved the mapping defect between fixed value in driver and the
> property in devicetree.
> - Optimized 480m output clock register function.
> - Code cleanup.
>
> Changes in v2:
> - Changed vbus_host operation from gpio to regulator in *_probe.
> - Improved the fault treatment relate to 480m clock register.
> - Cleaned up some meaningless codes in *_clk480m_disable.
> - made more clear the comment of *_sm_work.
>
> drivers/phy/Kconfig | 7 +
> drivers/phy/Makefile | 1 +
> drivers/phy/phy-rockchip-inno-usb2.c | 645
> ++++++++++++++++++++++++++++++++++ 3 files changed, 653 insertions(+)
> create mode 100644 drivers/phy/phy-rockchip-inno-usb2.c
>
> diff --git a/drivers/phy/Kconfig b/drivers/phy/Kconfig
> index b869b98..29ef15c 100644
> --- a/drivers/phy/Kconfig
> +++ b/drivers/phy/Kconfig
> @@ -347,6 +347,13 @@ config PHY_ROCKCHIP_USB
> help
> Enable this to support the Rockchip USB 2.0 PHY.
>
> +config PHY_ROCKCHIP_INNO_USB2
> + tristate "Rockchip INNO USB2PHY Driver"
> + depends on ARCH_ROCKCHIP && OF
> + select GENERIC_PHY
> + help
> + Support for Rockchip USB2.0 PHY with Innosilicon IP block.
> +
> config PHY_ROCKCHIP_EMMC
> tristate "Rockchip EMMC PHY Driver"
> depends on ARCH_ROCKCHIP && OF
> diff --git a/drivers/phy/Makefile b/drivers/phy/Makefile
> index 9c3e73c..4963fbc 100644
> --- a/drivers/phy/Makefile
> +++ b/drivers/phy/Makefile
> @@ -38,6 +38,7 @@ phy-exynos-usb2-$(CONFIG_PHY_S5PV210_USB2) +=
> phy-s5pv210-usb2.o obj-$(CONFIG_PHY_EXYNOS5_USBDRD) += phy-exynos5-usbdrd.o
> obj-$(CONFIG_PHY_QCOM_APQ8064_SATA) += phy-qcom-apq8064-sata.o
> obj-$(CONFIG_PHY_ROCKCHIP_USB) += phy-rockchip-usb.o
> +obj-$(CONFIG_PHY_ROCKCHIP_INNO_USB2) += phy-rockchip-inno-usb2.o
> obj-$(CONFIG_PHY_ROCKCHIP_EMMC) += phy-rockchip-emmc.o
> obj-$(CONFIG_PHY_ROCKCHIP_DP) += phy-rockchip-dp.o
> obj-$(CONFIG_PHY_QCOM_IPQ806X_SATA) += phy-qcom-ipq806x-sata.o
> diff --git a/drivers/phy/phy-rockchip-inno-usb2.c
> b/drivers/phy/phy-rockchip-inno-usb2.c new file mode 100644
> index 0000000..fc599c7
> --- /dev/null
> +++ b/drivers/phy/phy-rockchip-inno-usb2.c
> @@ -0,0 +1,645 @@
> +/*
> + * Rockchip USB2.0 PHY with Innosilicon IP block driver
> + *
> + * Copyright (C) 2016 Fuzhou Rockchip Electronics Co., Ltd
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License as published by
> + * the Free Software Foundation; either version 2 of the License, or
> + * (at your option) any later version.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> + * GNU General Public License for more details.
> + */
> +
> +#include <linux/clk.h>
> +#include <linux/clk-provider.h>
> +#include <linux/delay.h>
> +#include <linux/interrupt.h>
> +#include <linux/io.h>
> +#include <linux/gpio/consumer.h>
> +#include <linux/jiffies.h>
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/mutex.h>
> +#include <linux/of.h>
> +#include <linux/of_address.h>
> +#include <linux/of_irq.h>
> +#include <linux/of_platform.h>
> +#include <linux/phy/phy.h>
> +#include <linux/platform_device.h>
> +#include <linux/regmap.h>
> +#include <linux/mfd/syscon.h>
> +
> +#define BIT_WRITEABLE_SHIFT 16
> +#define SCHEDULE_DELAY (60 * HZ)
> +
> +enum rockchip_usb2phy_port_id {
> + USB2PHY_PORT_OTG,
> + USB2PHY_PORT_HOST,
> + USB2PHY_NUM_PORTS,
> +};
> +
> +enum rockchip_usb2phy_host_state {
> + PHY_STATE_HS_ONLINE = 0,
> + PHY_STATE_DISCONNECT = 1,
> + PHY_STATE_HS_CONNECT = 2,
> + PHY_STATE_FS_CONNECT = 4,
> +};
> +
> +struct usb2phy_reg {
> + unsigned int offset;
> + unsigned int bitend;
> + unsigned int bitstart;
> + unsigned int disable;
> + unsigned int enable;
> +};
> +
> +/**
> + * struct rockchip_usb2phy_port_cfg: usb-phy port configuration.
> + * @phy_sus: phy suspend register.
> + * @ls_det_en: linestate detection enable register.
> + * @ls_det_st: linestate detection state register.
> + * @ls_det_clr: linestate detection clear register.
> + * @utmi_ls: utmi linestate state register.
> + * @utmi_hstdet: utmi host disconnect register.
> + */
> +struct rockchip_usb2phy_port_cfg {
> + struct usb2phy_reg phy_sus;
> + struct usb2phy_reg ls_det_en;
> + struct usb2phy_reg ls_det_st;
> + struct usb2phy_reg ls_det_clr;
> + struct usb2phy_reg utmi_ls;
> + struct usb2phy_reg utmi_hstdet;
> +};
> +
> +/**
> + * struct rockchip_usb2phy_cfg: usb-phy configuration.
> + * @reg: the address offset of grf for usb-phy config.
> + * @num_ports: specify how many ports that the phy has.
> + * @clkout_ctl: keep on/turn off output clk of phy.
> + */
> +struct rockchip_usb2phy_cfg {
> + unsigned int reg;
> + unsigned int num_ports;
> + struct usb2phy_reg clkout_ctl;
> + const struct rockchip_usb2phy_port_cfg port_cfgs[USB2PHY_NUM_PORTS];
> +};
> +
> +/**
> + * struct rockchip_usb2phy_port: usb-phy port data.
> + * @port_id: flag for otg port or host port.
> + * @suspended: phy suspended flag.
> + * @ls_irq: IRQ number assigned for linestate detection.
> + * @mutex: for register updating in sm_work.
> + * @sm_work: OTG state machine work.
> + * @phy_cfg: port register configuration, assigned by driver data.
> + */
> +struct rockchip_usb2phy_port {
> + struct phy *phy;
> + unsigned int port_id;
> + bool suspended;
> + int ls_irq;
> + struct mutex mutex;
> + struct delayed_work sm_work;
> + const struct rockchip_usb2phy_port_cfg *port_cfg;
> +};
> +
> +/**
> + * struct rockchip_usb2phy: usb2.0 phy driver data.
> + * @grf: General Register Files regmap.
> + * @clk: clock struct of phy input clk.
> + * @clk480m: clock struct of phy output clk.
> + * @clk_hw: clock struct of phy output clk management.
> + * @phy_cfg: phy register configuration, assigned by driver data.
> + * @ports: phy port instance.
> + */
> +struct rockchip_usb2phy {
> + struct device *dev;
> + struct regmap *grf;
> + struct clk *clk;
> + struct clk *clk480m;
> + struct clk_hw clk480m_hw;
> + const struct rockchip_usb2phy_cfg *phy_cfg;
> + struct rockchip_usb2phy_port ports[USB2PHY_NUM_PORTS];
> +};
> +
> +static inline int property_enable(struct rockchip_usb2phy *rphy,
> + const struct usb2phy_reg *reg, bool en)
> +{
> + unsigned int val, mask, tmp;
> +
> + tmp = en ? reg->enable : reg->disable;
> + mask = GENMASK(reg->bitend, reg->bitstart);
> + val = (tmp << reg->bitstart) | (mask << BIT_WRITEABLE_SHIFT);
> +
> + return regmap_write(rphy->grf, reg->offset, val);
> +}
> +
> +static inline bool property_enabled(struct rockchip_usb2phy *rphy,
> + const struct usb2phy_reg *reg)
> +{
> + int ret;
> + unsigned int tmp, orig;
> + unsigned int mask = GENMASK(reg->bitend, reg->bitstart);
> +
> + ret = regmap_read(rphy->grf, reg->offset, &orig);
> + if (ret)
> + return false;
> +
> + tmp = (orig & mask) >> reg->bitstart;
> + return tmp == reg->enable;
> +}
> +
> +static int rockchip_usb2phy_clk480m_enable(struct clk_hw *hw)
> +{
> + struct rockchip_usb2phy *rphy =
> + container_of(hw, struct rockchip_usb2phy, clk480m_hw);
> + int ret;
> +
> + /* turn on 480m clk output if it is off */
> + if (!property_enabled(rphy, &rphy->phy_cfg->clkout_ctl)) {
> + ret = property_enable(rphy, &rphy->phy_cfg->clkout_ctl, true);
> + if (ret)
> + return ret;
> +
> + /* waitting for the clk become stable */
> + mdelay(1);
> + }
> +
> + return 0;
> +}
> +
> +static void rockchip_usb2phy_clk480m_disable(struct clk_hw *hw)
> +{
> + struct rockchip_usb2phy *rphy =
> + container_of(hw, struct rockchip_usb2phy, clk480m_hw);
> +
> + /* turn off 480m clk output */
> + property_enable(rphy, &rphy->phy_cfg->clkout_ctl, false);
> +}
> +
> +static int rockchip_usb2phy_clk480m_enabled(struct clk_hw *hw)
> +{
> + struct rockchip_usb2phy *rphy =
> + container_of(hw, struct rockchip_usb2phy, clk480m_hw);
> +
> + return property_enabled(rphy, &rphy->phy_cfg->clkout_ctl);
> +}
> +
> +static unsigned long
> +rockchip_usb2phy_clk480m_recalc_rate(struct clk_hw *hw,
> + unsigned long parent_rate)
> +{
> + return 480000000;
> +}
> +
> +static const struct clk_ops rockchip_usb2phy_clkout_ops = {
> + .enable = rockchip_usb2phy_clk480m_enable,
> + .disable = rockchip_usb2phy_clk480m_disable,
> + .is_enabled = rockchip_usb2phy_clk480m_enabled,
> + .recalc_rate = rockchip_usb2phy_clk480m_recalc_rate,
> +};
> +
> +static void rockchip_usb2phy_clk480m_unregister(void *data)
> +{
> + struct rockchip_usb2phy *rphy = data;
> +
> + of_clk_del_provider(rphy->dev->of_node);
> + clk_unregister(rphy->clk480m);
> +
> + if (rphy->clk)
> + clk_put(rphy->clk);
> +}
> +
> +static int
> +rockchip_usb2phy_clk480m_register(struct rockchip_usb2phy *rphy)
> +{
> + struct device_node *node = rphy->dev->of_node;
> + struct clk_init_data init;
> + const char *clk_name;
> + int ret;
> +
> + init.flags = 0;
> + init.name = "clk_usbphy_480m";
> + init.ops = &rockchip_usb2phy_clkout_ops;
> +
> + /* optional override of the clockname */
> + of_property_read_string(node, "clock-output-names", &init.name);
> +
> + rphy->clk = of_clk_get_by_name(node, "phyclk");
> + if (IS_ERR(rphy->clk)) {
> + rphy->clk = NULL;
> + init.parent_names = NULL;
> + init.num_parents = 0;
> + } else {
> + clk_name = __clk_get_name(rphy->clk);
> + init.parent_names = &clk_name;
> + init.num_parents = 1;
> + }
> +
> + rphy->clk480m_hw.init = &init;
> +
> + /* register the clock */
> + rphy->clk480m = clk_register(rphy->dev, &rphy->clk480m_hw);
> + if (IS_ERR(rphy->clk480m)) {
> + ret = PTR_ERR(rphy->clk480m);
> + goto err_register;
> + }
> +
> + ret = of_clk_add_provider(node, of_clk_src_simple_get, rphy->clk480m);
> + if (ret < 0)
> + goto err_clk_provider;
> +
> + ret = devm_add_action(rphy->dev, rockchip_usb2phy_clk480m_unregister,
> + rphy);
> + if (ret < 0)
> + goto err_unreg_action;
> +
> + return 0;
> +
> +err_unreg_action:
> + of_clk_del_provider(node);
> +err_clk_provider:
> + clk_unregister(rphy->clk480m);
> +err_register:
> + if (rphy->clk)
> + clk_put(rphy->clk);
> + return ret;
> +}
> +
> +static int rockchip_usb2phy_init(struct phy *phy)
> +{
> + struct rockchip_usb2phy_port *rport = phy_get_drvdata(phy);
> + struct rockchip_usb2phy *rphy = dev_get_drvdata(phy->dev.parent);
> + int ret;
> +
if (!rport->port_cfg)
return 0;
Otherwise the currently empty otg-port will cause null-pointer dereferences
when it gets assigned in the devicetree already.
> + if (rport->port_id == USB2PHY_PORT_HOST) {
> + /* clear linestate and enable linestate detect irq */
> + mutex_lock(&rport->mutex);
> +
> + ret = property_enable(rphy, &rport->port_cfg->ls_det_clr, true);
> + if (ret) {
> + mutex_unlock(&rport->mutex);
> + return ret;
> + }
> +
> + ret = property_enable(rphy, &rport->port_cfg->ls_det_en, true);
> + if (ret) {
> + mutex_unlock(&rport->mutex);
> + return ret;
> + }
> +
> + mutex_unlock(&rport->mutex);
> + schedule_delayed_work(&rport->sm_work, SCHEDULE_DELAY);
> + }
> +
> + return 0;
> +}
> +
> +static int rockchip_usb2phy_resume(struct phy *phy)
> +{
> + struct rockchip_usb2phy_port *rport = phy_get_drvdata(phy);
> + struct rockchip_usb2phy *rphy = dev_get_drvdata(phy->dev.parent);
> + int ret;
> +
> + dev_dbg(&rport->phy->dev, "port resume\n");
if (!rport->port_cfg)
return 0;
> +
> + ret = clk_prepare_enable(rphy->clk480m);
> + if (ret)
> + return ret;
> +
> + ret = property_enable(rphy, &rport->port_cfg->phy_sus, false);
> + if (ret)
> + return ret;
> +
> + rport->suspended = false;
> + return 0;
> +}
> +
> +static int rockchip_usb2phy_suspend(struct phy *phy)
> +{
> + struct rockchip_usb2phy_port *rport = phy_get_drvdata(phy);
> + struct rockchip_usb2phy *rphy = dev_get_drvdata(phy->dev.parent);
> + int ret;
> +
if (!rport->port_cfg)
return 0;
> + dev_dbg(&rport->phy->dev, "port suspend\n");
> +
> + ret = property_enable(rphy, &rport->port_cfg->phy_sus, true);
> + if (ret)
> + return ret;
> +
> + rport->suspended = true;
> + clk_disable_unprepare(rphy->clk480m);
> + return 0;
> +}
> +
> +static int rockchip_usb2phy_exit(struct phy *phy)
> +{
> + struct rockchip_usb2phy_port *rport = phy_get_drvdata(phy);
> +
if (!rport->port_cfg)
return 0;
> + if (rport->port_id == USB2PHY_PORT_HOST)
> + cancel_delayed_work_sync(&rport->sm_work);
> +
you will also need to resume the port here, if it is suspended at this point,
as phy_power_off gets called after phy_exit and would probably produce clk
enable/disable mismatches otherwise.
> + return 0;
> +}
> +
> +static const struct phy_ops rockchip_usb2phy_ops = {
> + .init = rockchip_usb2phy_init,
> + .exit = rockchip_usb2phy_exit,
> + .power_on = rockchip_usb2phy_resume,
> + .power_off = rockchip_usb2phy_suspend,
> + .owner = THIS_MODULE,
> +};
> +
> +/*
> + * The function manage host-phy port state and suspend/resume phy port
> + * to save power.
> + *
> + * we rely on utmi_linestate and utmi_hostdisconnect to identify whether
> + * FS/HS is disconnect or not. Besides, we do not need care it is FS
> + * disconnected or HS disconnected, actually, we just only need get the
> + * device is disconnected at last through rearm the delayed work,
> + * to suspend the phy port in _PHY_STATE_DISCONNECT_ case.
> + *
> + * NOTE: It may invoke *phy_suspend or *phy_resume which will invoke some
> + * clk related APIs, so do not invoke it from interrupt context directly.
> + */
> +static void rockchip_usb2phy_sm_work(struct work_struct *work)
> +{
> + struct rockchip_usb2phy_port *rport =
> + container_of(work, struct rockchip_usb2phy_port, sm_work.work);
> + struct rockchip_usb2phy *rphy = dev_get_drvdata(rport->phy->dev.parent);
> + unsigned int sh = rport->port_cfg->utmi_hstdet.bitend -
> + rport->port_cfg->utmi_hstdet.bitstart + 1;
> + unsigned int ul, uhd, state;
> + unsigned int ul_mask, uhd_mask;
> + int ret;
> +
> + mutex_lock(&rport->mutex);
> +
> + ret = regmap_read(rphy->grf, rport->port_cfg->utmi_ls.offset, &ul);
> + if (ret < 0)
> + goto next_schedule;
> +
> + ret = regmap_read(rphy->grf, rport->port_cfg->utmi_hstdet.offset,
> + &uhd);
> + if (ret < 0)
> + goto next_schedule;
> +
> + uhd_mask = GENMASK(rport->port_cfg->utmi_hstdet.bitend,
> + rport->port_cfg->utmi_hstdet.bitstart);
> + ul_mask = GENMASK(rport->port_cfg->utmi_ls.bitend,
> + rport->port_cfg->utmi_ls.bitstart);
> +
> + /* stitch on utmi_ls and utmi_hstdet as phy state */
> + state = ((uhd & uhd_mask) >> rport->port_cfg->utmi_hstdet.bitstart) |
> + (((ul & ul_mask) >> rport->port_cfg->utmi_ls.bitstart) << sh);
> +
> + switch (state) {
> + case PHY_STATE_HS_ONLINE:
> + dev_dbg(&rport->phy->dev, "HS online\n");
> + break;
> + case PHY_STATE_FS_CONNECT:
> + /*
> + * For FS device, the online state share with connect state
> + * from utmi_ls and utmi_hstdet register, so we distinguish
> + * them via suspended flag.
> + */
> + if (!rport->suspended) {
> + dev_dbg(&rport->phy->dev, "FS online\n");
> + break;
> + }
> + /* fall through */
> + case PHY_STATE_HS_CONNECT:
> + if (rport->suspended) {
> + dev_dbg(&rport->phy->dev, "HS/FS connected\n");
> + rockchip_usb2phy_resume(rport->phy);
> + rport->suspended = false;
> + }
> + break;
> + case PHY_STATE_DISCONNECT:
> + if (!rport->suspended) {
> + dev_dbg(&rport->phy->dev, "HS/FS disconnected\n");
> + rockchip_usb2phy_suspend(rport->phy);
> + rport->suspended = true;
> + }
> +
> + /*
> + * activate the linestate detection to get the next device
> + * plug-in irq.
> + */
> + property_enable(rphy, &rport->port_cfg->ls_det_clr, true);
> + property_enable(rphy, &rport->port_cfg->ls_det_en, true);
> +
> + /*
> + * we don't need to rearm the delayed work when the phy port
> + * is suspended.
> + */
> + mutex_unlock(&rport->mutex);
> + return;
> + default:
> + dev_dbg(&rport->phy->dev, "unknown phy state\n");
> + break;
> + }
> +
> +next_schedule:
> + mutex_unlock(&rport->mutex);
> + schedule_delayed_work(&rport->sm_work, SCHEDULE_DELAY);
> +}
> +
> +static irqreturn_t rockchip_usb2phy_linestate_irq(int irq, void *data)
> +{
> + struct rockchip_usb2phy_port *rport = data;
> + struct rockchip_usb2phy *rphy = dev_get_drvdata(rport->phy->dev.parent);
> +
> + if (!property_enabled(rphy, &rport->port_cfg->ls_det_st))
> + return IRQ_NONE;
> +
> + mutex_lock(&rport->mutex);
> +
> + /* disable linestate detect irq and clear its status */
> + property_enable(rphy, &rport->port_cfg->ls_det_en, false);
> + property_enable(rphy, &rport->port_cfg->ls_det_clr, true);
> +
> + mutex_unlock(&rport->mutex);
> +
> + /*
> + * In this case for host phy port, a new device is plugged in,
> + * meanwhile, if the phy port is suspended, we need rearm the work to
> + * resume it and mange its states; otherwise, we do nothing about that.
> + */
> + if (rport->suspended && rport->port_id == USB2PHY_PORT_HOST)
> + rockchip_usb2phy_sm_work(&rport->sm_work.work);
> +
> + return IRQ_HANDLED;
> +}
> +
> +static int rockchip_usb2phy_host_port_init(struct rockchip_usb2phy *rphy,
> + struct rockchip_usb2phy_port *rport,
> + struct device_node *child_np)
> +{
> + int ret;
> +
> + rport->port_id = USB2PHY_PORT_HOST;
> + rport->port_cfg = &rphy->phy_cfg->port_cfgs[USB2PHY_PORT_HOST];
> +
> + mutex_init(&rport->mutex);
> + INIT_DELAYED_WORK(&rport->sm_work, rockchip_usb2phy_sm_work);
> +
> + rport->ls_irq = of_irq_get_byname(child_np, "linestate");
> + if (rport->ls_irq < 0) {
> + dev_err(rphy->dev, "no linestate irq provided\n");
> + return rport->ls_irq;
> + }
> +
> + ret = devm_request_threaded_irq(rphy->dev, rport->ls_irq, NULL,
> + rockchip_usb2phy_linestate_irq,
> + IRQF_ONESHOT,
> + "rockchip_usb2phy", rport);
> + if (ret) {
> + dev_err(rphy->dev, "failed to request irq handle\n");
> + return ret;
> + }
> +
> + return 0;
> +}
> +
> +static int rockchip_usb2phy_probe(struct platform_device *pdev)
> +{
> + struct device *dev = &pdev->dev;
> + struct device_node *np = dev->of_node;
> + struct device_node *child_np;
> + struct phy_provider *provider;
> + struct rockchip_usb2phy *rphy;
> + const struct rockchip_usb2phy_cfg *phy_cfgs;
> + const struct of_device_id *match;
> + unsigned int reg;
> + int index, ret;
> +
> + rphy = devm_kzalloc(dev, sizeof(*rphy), GFP_KERNEL);
> + if (!rphy)
> + return -ENOMEM;
> +
> + match = of_match_device(dev->driver->of_match_table, dev);
> + if (!match || !match->data) {
> + dev_err(dev, "phy configs are not assigned!\n");
> + return -EINVAL;
> + }
> +
> + if (!dev->parent || !dev->parent->of_node)
> + return -EINVAL;
> +
> + rphy->grf = syscon_node_to_regmap(dev->parent->of_node);
> + if (IS_ERR(rphy->grf))
> + return PTR_ERR(rphy->grf);
> +
> + if (of_property_read_u32(np, "reg", ®)) {
> + dev_err(dev, "the reg property is not assigned in %s node\n",
> + np->name);
> + return -EINVAL;
> + }
> +
> + rphy->dev = dev;
> + phy_cfgs = match->data;
> + platform_set_drvdata(pdev, rphy);
> +
> + /* find out a proper config which can be matched with dt. */
> + index = 0;
> + while (phy_cfgs[index].reg) {
> + if (phy_cfgs[index].reg == reg) {
> + rphy->phy_cfg = &phy_cfgs[index];
> + break;
> + }
> +
> + ++index;
> + }
> +
> + if (!rphy->phy_cfg) {
> + dev_err(dev, "no phy-config can be matched with %s node\n",
> + np->name);
> + return -EINVAL;
> + }
> +
> + ret = rockchip_usb2phy_clk480m_register(rphy);
> + if (ret) {
> + dev_err(dev, "failed to register 480m output clock\n");
> + return ret;
> + }
> +
> + index = 0;
> + for_each_available_child_of_node(np, child_np) {
> + struct rockchip_usb2phy_port *rport = &rphy->ports[index];
> + struct phy *phy;
> +
> + phy = devm_phy_create(dev, child_np, &rockchip_usb2phy_ops);
> + if (IS_ERR(phy)) {
> + dev_err(dev, "failed to create phy\n");
> + ret = PTR_ERR(phy);
> + goto put_child;
> + }
> +
> + rport->phy = phy;
phy_set_drvdata(rport->phy, rport);
> +
> + /* initialize otg/host port separately */
> + if (!of_node_cmp(child_np->name, "host-port")) {
> + ret = rockchip_usb2phy_host_port_init(rphy, rport,
> + child_np);
> + if (ret)
> + goto put_child;
> + }
> +
> + phy_set_drvdata(rport->phy, rport);
move this to the location above to prevent null-pointer dereferences with
devices plugged in on boot.
I've tested this a bit on a rk3036 (which is lacking the disconnect-detection
it seems), so in general (apart from the stuff mentioned above) this looks
nice now. So with the stuff above fixed:
Reviewed-by: Heiko Stuebner <heiko@sntech.de>
Tested-by: Heiko Stuebner <heiko@sntech.de>
Heiko
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH v5 1/2] Documentation: bindings: add DT documentation for Rockchip USB2PHY
2016-06-13 8:38 ` Heiko Stübner
@ 2016-06-14 13:28 ` Heiko Stübner
2016-06-15 1:24 ` Frank Wang
0 siblings, 1 reply; 25+ messages in thread
From: Heiko Stübner @ 2016-06-14 13:28 UTC (permalink / raw)
To: Frank Wang
Cc: dianders, linux, groeck, jwerner, kishon, robh+dt, pawel.moll,
mark.rutland, ijc+devicetree, galak, linux-kernel, devicetree,
linux-usb, linux-rockchip, xzy.xu, kever.yang, huangtao,
william.wu
Am Montag, 13. Juni 2016, 10:38:39 schrieb Heiko Stübner:
> Am Montag, 13. Juni 2016, 10:10:09 schrieb Frank Wang:
> > Signed-off-by: Frank Wang <frank.wang@rock-chips.com>
>
> looks really cool now, thanks for addressing all the review comments
>
> Reviewed-by: Heiko Stuebner <heiko@sntech.de>
You've only added the very common "reg" property, so I guess it should be just
fine to keep Rob's Ack on the binding in that case.
I noticed one thing below though.
As you'll probably need to do another version (see comments to patch2), you
could change that as well in both here and in the second patch - see below:
> > ---
> >
> > Changes in v5:
> > - Added 'reg' property to identify the different phy-blocks.
> >
> > Changes in v4:
> > - Used 'phy-supply' instead of 'vbus_*-supply'.
> >
> > Changes in v3:
> > - Added 'clocks' and 'clock-names' optional properties.
> > - Specified 'otg-port' and 'host-port' as the sub-node name.
> >
> > Changes in v2:
> > - Changed vbus_host optional property from gpio to regulator.
> > - Specified vbus_otg-supply optional property.
> > - Specified otg_id and otg_bvalid property.
> >
> > .../bindings/phy/phy-rockchip-inno-usb2.txt | 64
> >
> > ++++++++++++++++++++ 1 file changed, 64 insertions(+)
> >
> > create mode 100644
> >
> > Documentation/devicetree/bindings/phy/phy-rockchip-inno-usb2.txt
> >
> > diff --git
> > a/Documentation/devicetree/bindings/phy/phy-rockchip-inno-usb2.txt
> > b/Documentation/devicetree/bindings/phy/phy-rockchip-inno-usb2.txt new
> > file
> > mode 100644
> > index 0000000..48bb5de
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/phy/phy-rockchip-inno-usb2.txt
> > @@ -0,0 +1,64 @@
> > +ROCKCHIP USB2.0 PHY WITH INNO IP BLOCK
> > +
> > +Required properties (phy (parent) node):
> > + - compatible : should be one of the listed compatibles:
> > + * "rockchip,rk3366-usb2phy"
> > + * "rockchip,rk3399-usb2phy"
> > + - reg : the address offset of grf for usb-phy configuration.
> > + - #clock-cells : should be 0.
> > + - clock-output-names : specify the 480m output clock name.
> > +
> > +Optional properties:
> > + - clocks : phandle + phy specifier pair, for the input clock of phy.
> > + - clock-names : input clock name of phy, must be "phyclk".
> > +
> > +Required nodes : a sub-node is required for each port the phy provides.
> > + The sub-node name is used to identify host or otg port,
> > + and shall be the following entries:
> > + * "otg-port" : the name of otg port.
> > + * "host-port" : the name of host port.
> > +
> > +Required properties (port (child) node):
> > + - #phy-cells : must be 0. See ./phy-bindings.txt for details.
> > + - interrupts : specify an interrupt for each entry in interrupt-names.
> > + - interrupt-names : a list which shall be the following entries:
> > + * "otg_id" : for the otg id interrupt.
> > + * "otg_bvalid" : for the otg vbus interrupt.
please use "-" not underscores in dt-bindings, so "otg-id", "otg-bvalid".
> > + * "linestate" : for the host/otg linestate interrupt.
> > +
> > +Optional properties:
> > + - phy-supply : phandle to a regulator that provides power to VBUS.
> > + See ./phy-bindings.txt for details.
> > +
> > +Example:
> > +
> > +grf: syscon@ff770000 {
> > + compatible = "rockchip,rk3366-grf", "syscon", "simple-mfd";
> > + #address-cells = <1>;
> > + #size-cells = <1>;
> > +
> > +...
> > +
> > + u2phy: usb2-phy@700 {
> > + compatible = "rockchip,rk3366-usb2phy";
> > + reg = <0x700 0x2c>;
> > + #clock-cells = <0>;
> > + clock-output-names = "sclk_otgphy0_480m";
> > +
> > + u2phy_otg: otg-port {
> > + #phy-cells = <0>;
> > + interrupts = <GIC_SPI 93 IRQ_TYPE_LEVEL_HIGH>,
> > + <GIC_SPI 94 IRQ_TYPE_LEVEL_HIGH>,
> > + <GIC_SPI 95 IRQ_TYPE_LEVEL_HIGH>;
> > + interrupt-names = "otg_id", "otg_bvalid", "linestate";
again "-" not "_".
> > + status = "okay";
> > + };
> > +
> > + u2phy_host: host-port {
> > + #phy-cells = <0>;
> > + interrupts = <GIC_SPI 96 IRQ_TYPE_LEVEL_HIGH>;
> > + interrupt-names = "linestate";
> > + status = "okay";
> > + };
> > + };
> > +};
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH v5 2/2] phy: rockchip-inno-usb2: add a new driver for Rockchip usb2phy
2016-06-14 13:27 ` Heiko Stübner
@ 2016-06-14 13:50 ` Guenter Roeck
2016-06-14 14:00 ` Heiko Stübner
2016-06-15 3:23 ` Frank Wang
1 sibling, 1 reply; 25+ messages in thread
From: Guenter Roeck @ 2016-06-14 13:50 UTC (permalink / raw)
To: Heiko Stübner
Cc: Frank Wang, Douglas Anderson, Guenter Roeck, Guenter Roeck,
jwerner, kishon, robh+dt, pawel.moll, mark.rutland,
ijc+devicetree, Kumar Gala, linux-kernel, devicetree, linux-usb,
linux-rockchip, Ziyuan Xu, Kever Yang, Tao Huang, william.wu
On Tue, Jun 14, 2016 at 6:27 AM, Heiko Stübner <heiko@sntech.de> wrote:
> Am Montag, 13. Juni 2016, 10:10:10 schrieb Frank Wang:
>> The newer SoCs (rk3366, rk3399) take a different usb-phy IP block
>> than rk3288 and before, and most of phy-related registers are also
>> different from the past, so a new phy driver is required necessarily.
>>
>> Signed-off-by: Frank Wang <frank.wang@rock-chips.com>
>> ---
>>
>> Changes in v5:
>> - Added 'reg' in the data block to match the different phy-blocks in dt.
>>
>> Changes in v4:
>> - Removed some processes related to 'vbus_host-supply'.
>>
>> Changes in v3:
>> - Resolved the mapping defect between fixed value in driver and the
>> property in devicetree.
>> - Optimized 480m output clock register function.
>> - Code cleanup.
>>
>> Changes in v2:
>> - Changed vbus_host operation from gpio to regulator in *_probe.
>> - Improved the fault treatment relate to 480m clock register.
>> - Cleaned up some meaningless codes in *_clk480m_disable.
>> - made more clear the comment of *_sm_work.
>>
>> drivers/phy/Kconfig | 7 +
>> drivers/phy/Makefile | 1 +
>> drivers/phy/phy-rockchip-inno-usb2.c | 645
>> ++++++++++++++++++++++++++++++++++ 3 files changed, 653 insertions(+)
>> create mode 100644 drivers/phy/phy-rockchip-inno-usb2.c
>>
>> diff --git a/drivers/phy/Kconfig b/drivers/phy/Kconfig
>> index b869b98..29ef15c 100644
>> --- a/drivers/phy/Kconfig
>> +++ b/drivers/phy/Kconfig
>> @@ -347,6 +347,13 @@ config PHY_ROCKCHIP_USB
>> help
>> Enable this to support the Rockchip USB 2.0 PHY.
>>
>> +config PHY_ROCKCHIP_INNO_USB2
>> + tristate "Rockchip INNO USB2PHY Driver"
>> + depends on ARCH_ROCKCHIP && OF
>> + select GENERIC_PHY
>> + help
>> + Support for Rockchip USB2.0 PHY with Innosilicon IP block.
>> +
>> config PHY_ROCKCHIP_EMMC
>> tristate "Rockchip EMMC PHY Driver"
>> depends on ARCH_ROCKCHIP && OF
>> diff --git a/drivers/phy/Makefile b/drivers/phy/Makefile
>> index 9c3e73c..4963fbc 100644
>> --- a/drivers/phy/Makefile
>> +++ b/drivers/phy/Makefile
>> @@ -38,6 +38,7 @@ phy-exynos-usb2-$(CONFIG_PHY_S5PV210_USB2) +=
>> phy-s5pv210-usb2.o obj-$(CONFIG_PHY_EXYNOS5_USBDRD) += phy-exynos5-usbdrd.o
>> obj-$(CONFIG_PHY_QCOM_APQ8064_SATA) += phy-qcom-apq8064-sata.o
>> obj-$(CONFIG_PHY_ROCKCHIP_USB) += phy-rockchip-usb.o
>> +obj-$(CONFIG_PHY_ROCKCHIP_INNO_USB2) += phy-rockchip-inno-usb2.o
>> obj-$(CONFIG_PHY_ROCKCHIP_EMMC) += phy-rockchip-emmc.o
>> obj-$(CONFIG_PHY_ROCKCHIP_DP) += phy-rockchip-dp.o
>> obj-$(CONFIG_PHY_QCOM_IPQ806X_SATA) += phy-qcom-ipq806x-sata.o
>> diff --git a/drivers/phy/phy-rockchip-inno-usb2.c
>> b/drivers/phy/phy-rockchip-inno-usb2.c new file mode 100644
>> index 0000000..fc599c7
>> --- /dev/null
>> +++ b/drivers/phy/phy-rockchip-inno-usb2.c
>> @@ -0,0 +1,645 @@
>> +/*
>> + * Rockchip USB2.0 PHY with Innosilicon IP block driver
>> + *
>> + * Copyright (C) 2016 Fuzhou Rockchip Electronics Co., Ltd
>> + *
>> + * This program is free software; you can redistribute it and/or modify
>> + * it under the terms of the GNU General Public License as published by
>> + * the Free Software Foundation; either version 2 of the License, or
>> + * (at your option) any later version.
>> + *
>> + * This program is distributed in the hope that it will be useful,
>> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
>> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
>> + * GNU General Public License for more details.
>> + */
>> +
>> +#include <linux/clk.h>
>> +#include <linux/clk-provider.h>
>> +#include <linux/delay.h>
>> +#include <linux/interrupt.h>
>> +#include <linux/io.h>
>> +#include <linux/gpio/consumer.h>
>> +#include <linux/jiffies.h>
>> +#include <linux/kernel.h>
>> +#include <linux/module.h>
>> +#include <linux/mutex.h>
>> +#include <linux/of.h>
>> +#include <linux/of_address.h>
>> +#include <linux/of_irq.h>
>> +#include <linux/of_platform.h>
>> +#include <linux/phy/phy.h>
>> +#include <linux/platform_device.h>
>> +#include <linux/regmap.h>
>> +#include <linux/mfd/syscon.h>
>> +
>> +#define BIT_WRITEABLE_SHIFT 16
>> +#define SCHEDULE_DELAY (60 * HZ)
>> +
>> +enum rockchip_usb2phy_port_id {
>> + USB2PHY_PORT_OTG,
>> + USB2PHY_PORT_HOST,
>> + USB2PHY_NUM_PORTS,
>> +};
>> +
>> +enum rockchip_usb2phy_host_state {
>> + PHY_STATE_HS_ONLINE = 0,
>> + PHY_STATE_DISCONNECT = 1,
>> + PHY_STATE_HS_CONNECT = 2,
>> + PHY_STATE_FS_CONNECT = 4,
>> +};
>> +
>> +struct usb2phy_reg {
>> + unsigned int offset;
>> + unsigned int bitend;
>> + unsigned int bitstart;
>> + unsigned int disable;
>> + unsigned int enable;
>> +};
>> +
>> +/**
>> + * struct rockchip_usb2phy_port_cfg: usb-phy port configuration.
>> + * @phy_sus: phy suspend register.
>> + * @ls_det_en: linestate detection enable register.
>> + * @ls_det_st: linestate detection state register.
>> + * @ls_det_clr: linestate detection clear register.
>> + * @utmi_ls: utmi linestate state register.
>> + * @utmi_hstdet: utmi host disconnect register.
>> + */
>> +struct rockchip_usb2phy_port_cfg {
>> + struct usb2phy_reg phy_sus;
>> + struct usb2phy_reg ls_det_en;
>> + struct usb2phy_reg ls_det_st;
>> + struct usb2phy_reg ls_det_clr;
>> + struct usb2phy_reg utmi_ls;
>> + struct usb2phy_reg utmi_hstdet;
>> +};
>> +
>> +/**
>> + * struct rockchip_usb2phy_cfg: usb-phy configuration.
>> + * @reg: the address offset of grf for usb-phy config.
>> + * @num_ports: specify how many ports that the phy has.
>> + * @clkout_ctl: keep on/turn off output clk of phy.
>> + */
>> +struct rockchip_usb2phy_cfg {
>> + unsigned int reg;
>> + unsigned int num_ports;
>> + struct usb2phy_reg clkout_ctl;
>> + const struct rockchip_usb2phy_port_cfg port_cfgs[USB2PHY_NUM_PORTS];
>> +};
>> +
>> +/**
>> + * struct rockchip_usb2phy_port: usb-phy port data.
>> + * @port_id: flag for otg port or host port.
>> + * @suspended: phy suspended flag.
>> + * @ls_irq: IRQ number assigned for linestate detection.
>> + * @mutex: for register updating in sm_work.
>> + * @sm_work: OTG state machine work.
>> + * @phy_cfg: port register configuration, assigned by driver data.
>> + */
>> +struct rockchip_usb2phy_port {
>> + struct phy *phy;
>> + unsigned int port_id;
>> + bool suspended;
>> + int ls_irq;
>> + struct mutex mutex;
>> + struct delayed_work sm_work;
>> + const struct rockchip_usb2phy_port_cfg *port_cfg;
>> +};
>> +
>> +/**
>> + * struct rockchip_usb2phy: usb2.0 phy driver data.
>> + * @grf: General Register Files regmap.
>> + * @clk: clock struct of phy input clk.
>> + * @clk480m: clock struct of phy output clk.
>> + * @clk_hw: clock struct of phy output clk management.
>> + * @phy_cfg: phy register configuration, assigned by driver data.
>> + * @ports: phy port instance.
>> + */
>> +struct rockchip_usb2phy {
>> + struct device *dev;
>> + struct regmap *grf;
>> + struct clk *clk;
>> + struct clk *clk480m;
>> + struct clk_hw clk480m_hw;
>> + const struct rockchip_usb2phy_cfg *phy_cfg;
>> + struct rockchip_usb2phy_port ports[USB2PHY_NUM_PORTS];
>> +};
>> +
>> +static inline int property_enable(struct rockchip_usb2phy *rphy,
>> + const struct usb2phy_reg *reg, bool en)
>> +{
>> + unsigned int val, mask, tmp;
>> +
>> + tmp = en ? reg->enable : reg->disable;
>> + mask = GENMASK(reg->bitend, reg->bitstart);
>> + val = (tmp << reg->bitstart) | (mask << BIT_WRITEABLE_SHIFT);
>> +
>> + return regmap_write(rphy->grf, reg->offset, val);
>> +}
>> +
>> +static inline bool property_enabled(struct rockchip_usb2phy *rphy,
>> + const struct usb2phy_reg *reg)
>> +{
>> + int ret;
>> + unsigned int tmp, orig;
>> + unsigned int mask = GENMASK(reg->bitend, reg->bitstart);
>> +
>> + ret = regmap_read(rphy->grf, reg->offset, &orig);
>> + if (ret)
>> + return false;
>> +
>> + tmp = (orig & mask) >> reg->bitstart;
>> + return tmp == reg->enable;
>> +}
>> +
>> +static int rockchip_usb2phy_clk480m_enable(struct clk_hw *hw)
>> +{
>> + struct rockchip_usb2phy *rphy =
>> + container_of(hw, struct rockchip_usb2phy, clk480m_hw);
>> + int ret;
>> +
>> + /* turn on 480m clk output if it is off */
>> + if (!property_enabled(rphy, &rphy->phy_cfg->clkout_ctl)) {
>> + ret = property_enable(rphy, &rphy->phy_cfg->clkout_ctl, true);
>> + if (ret)
>> + return ret;
>> +
>> + /* waitting for the clk become stable */
>> + mdelay(1);
>> + }
>> +
>> + return 0;
>> +}
>> +
>> +static void rockchip_usb2phy_clk480m_disable(struct clk_hw *hw)
>> +{
>> + struct rockchip_usb2phy *rphy =
>> + container_of(hw, struct rockchip_usb2phy, clk480m_hw);
>> +
>> + /* turn off 480m clk output */
>> + property_enable(rphy, &rphy->phy_cfg->clkout_ctl, false);
>> +}
>> +
>> +static int rockchip_usb2phy_clk480m_enabled(struct clk_hw *hw)
>> +{
>> + struct rockchip_usb2phy *rphy =
>> + container_of(hw, struct rockchip_usb2phy, clk480m_hw);
>> +
>> + return property_enabled(rphy, &rphy->phy_cfg->clkout_ctl);
>> +}
>> +
>> +static unsigned long
>> +rockchip_usb2phy_clk480m_recalc_rate(struct clk_hw *hw,
>> + unsigned long parent_rate)
>> +{
>> + return 480000000;
>> +}
>> +
>> +static const struct clk_ops rockchip_usb2phy_clkout_ops = {
>> + .enable = rockchip_usb2phy_clk480m_enable,
>> + .disable = rockchip_usb2phy_clk480m_disable,
>> + .is_enabled = rockchip_usb2phy_clk480m_enabled,
>> + .recalc_rate = rockchip_usb2phy_clk480m_recalc_rate,
>> +};
>> +
>> +static void rockchip_usb2phy_clk480m_unregister(void *data)
>> +{
>> + struct rockchip_usb2phy *rphy = data;
>> +
>> + of_clk_del_provider(rphy->dev->of_node);
>> + clk_unregister(rphy->clk480m);
>> +
>> + if (rphy->clk)
>> + clk_put(rphy->clk);
>> +}
>> +
>> +static int
>> +rockchip_usb2phy_clk480m_register(struct rockchip_usb2phy *rphy)
>> +{
>> + struct device_node *node = rphy->dev->of_node;
>> + struct clk_init_data init;
>> + const char *clk_name;
>> + int ret;
>> +
>> + init.flags = 0;
>> + init.name = "clk_usbphy_480m";
>> + init.ops = &rockchip_usb2phy_clkout_ops;
>> +
>> + /* optional override of the clockname */
>> + of_property_read_string(node, "clock-output-names", &init.name);
>> +
>> + rphy->clk = of_clk_get_by_name(node, "phyclk");
>> + if (IS_ERR(rphy->clk)) {
>> + rphy->clk = NULL;
>> + init.parent_names = NULL;
>> + init.num_parents = 0;
>> + } else {
>> + clk_name = __clk_get_name(rphy->clk);
>> + init.parent_names = &clk_name;
>> + init.num_parents = 1;
>> + }
>> +
>> + rphy->clk480m_hw.init = &init;
>> +
>> + /* register the clock */
>> + rphy->clk480m = clk_register(rphy->dev, &rphy->clk480m_hw);
>> + if (IS_ERR(rphy->clk480m)) {
>> + ret = PTR_ERR(rphy->clk480m);
>> + goto err_register;
>> + }
>> +
>> + ret = of_clk_add_provider(node, of_clk_src_simple_get, rphy->clk480m);
>> + if (ret < 0)
>> + goto err_clk_provider;
>> +
>> + ret = devm_add_action(rphy->dev, rockchip_usb2phy_clk480m_unregister,
>> + rphy);
>> + if (ret < 0)
>> + goto err_unreg_action;
>> +
>> + return 0;
>> +
>> +err_unreg_action:
>> + of_clk_del_provider(node);
>> +err_clk_provider:
>> + clk_unregister(rphy->clk480m);
>> +err_register:
>> + if (rphy->clk)
>> + clk_put(rphy->clk);
>> + return ret;
>> +}
>> +
>> +static int rockchip_usb2phy_init(struct phy *phy)
>> +{
>> + struct rockchip_usb2phy_port *rport = phy_get_drvdata(phy);
>> + struct rockchip_usb2phy *rphy = dev_get_drvdata(phy->dev.parent);
>> + int ret;
>> +
>
> if (!rport->port_cfg)
> return 0;
>
> Otherwise the currently empty otg-port will cause null-pointer dereferences
> when it gets assigned in the devicetree already.
>
Not really, at least not here - that port should not have port_id set
to USB2PHY_PORT_HOST.
Does it even make sense to instantiate the otg port ? Is it going to
do anything without port configuration ?
>
>> + if (rport->port_id == USB2PHY_PORT_HOST) {
>> + /* clear linestate and enable linestate detect irq */
>> + mutex_lock(&rport->mutex);
>> +
>> + ret = property_enable(rphy, &rport->port_cfg->ls_det_clr, true);
>> + if (ret) {
>> + mutex_unlock(&rport->mutex);
>> + return ret;
>> + }
>> +
>> + ret = property_enable(rphy, &rport->port_cfg->ls_det_en, true);
>> + if (ret) {
>> + mutex_unlock(&rport->mutex);
>> + return ret;
>> + }
>> +
>> + mutex_unlock(&rport->mutex);
>> + schedule_delayed_work(&rport->sm_work, SCHEDULE_DELAY);
>> + }
>> +
>> + return 0;
>> +}
>> +
>> +static int rockchip_usb2phy_resume(struct phy *phy)
>> +{
>> + struct rockchip_usb2phy_port *rport = phy_get_drvdata(phy);
>> + struct rockchip_usb2phy *rphy = dev_get_drvdata(phy->dev.parent);
>> + int ret;
>> +
>> + dev_dbg(&rport->phy->dev, "port resume\n");
>
> if (!rport->port_cfg)
> return 0;
>
>> +
>> + ret = clk_prepare_enable(rphy->clk480m);
>> + if (ret)
>> + return ret;
>> +
>> + ret = property_enable(rphy, &rport->port_cfg->phy_sus, false);
>> + if (ret)
>> + return ret;
>> +
>> + rport->suspended = false;
>> + return 0;
>> +}
>> +
>> +static int rockchip_usb2phy_suspend(struct phy *phy)
>> +{
>> + struct rockchip_usb2phy_port *rport = phy_get_drvdata(phy);
>> + struct rockchip_usb2phy *rphy = dev_get_drvdata(phy->dev.parent);
>> + int ret;
>> +
>
> if (!rport->port_cfg)
> return 0;
>
>> + dev_dbg(&rport->phy->dev, "port suspend\n");
>> +
>> + ret = property_enable(rphy, &rport->port_cfg->phy_sus, true);
>> + if (ret)
>> + return ret;
>> +
>> + rport->suspended = true;
>> + clk_disable_unprepare(rphy->clk480m);
>> + return 0;
>> +}
>> +
>> +static int rockchip_usb2phy_exit(struct phy *phy)
>> +{
>> + struct rockchip_usb2phy_port *rport = phy_get_drvdata(phy);
>> +
>
> if (!rport->port_cfg)
> return 0;
>
No access to port_cfg here ?
>> + if (rport->port_id == USB2PHY_PORT_HOST)
>> + cancel_delayed_work_sync(&rport->sm_work);
>> +
>
> you will also need to resume the port here, if it is suspended at this point,
> as phy_power_off gets called after phy_exit and would probably produce clk
> enable/disable mismatches otherwise.
>
>
>> + return 0;
>> +}
>> +
>> +static const struct phy_ops rockchip_usb2phy_ops = {
>> + .init = rockchip_usb2phy_init,
>> + .exit = rockchip_usb2phy_exit,
>> + .power_on = rockchip_usb2phy_resume,
>> + .power_off = rockchip_usb2phy_suspend,
>> + .owner = THIS_MODULE,
>> +};
>> +
>> +/*
>> + * The function manage host-phy port state and suspend/resume phy port
>> + * to save power.
>> + *
>> + * we rely on utmi_linestate and utmi_hostdisconnect to identify whether
>> + * FS/HS is disconnect or not. Besides, we do not need care it is FS
>> + * disconnected or HS disconnected, actually, we just only need get the
>> + * device is disconnected at last through rearm the delayed work,
>> + * to suspend the phy port in _PHY_STATE_DISCONNECT_ case.
>> + *
>> + * NOTE: It may invoke *phy_suspend or *phy_resume which will invoke some
>> + * clk related APIs, so do not invoke it from interrupt context directly.
>> + */
>> +static void rockchip_usb2phy_sm_work(struct work_struct *work)
>> +{
>> + struct rockchip_usb2phy_port *rport =
>> + container_of(work, struct rockchip_usb2phy_port, sm_work.work);
>> + struct rockchip_usb2phy *rphy = dev_get_drvdata(rport->phy->dev.parent);
>> + unsigned int sh = rport->port_cfg->utmi_hstdet.bitend -
>> + rport->port_cfg->utmi_hstdet.bitstart + 1;
>> + unsigned int ul, uhd, state;
>> + unsigned int ul_mask, uhd_mask;
>> + int ret;
>> +
>> + mutex_lock(&rport->mutex);
>> +
>> + ret = regmap_read(rphy->grf, rport->port_cfg->utmi_ls.offset, &ul);
>> + if (ret < 0)
>> + goto next_schedule;
>> +
>> + ret = regmap_read(rphy->grf, rport->port_cfg->utmi_hstdet.offset,
>> + &uhd);
>> + if (ret < 0)
>> + goto next_schedule;
>> +
>> + uhd_mask = GENMASK(rport->port_cfg->utmi_hstdet.bitend,
>> + rport->port_cfg->utmi_hstdet.bitstart);
>> + ul_mask = GENMASK(rport->port_cfg->utmi_ls.bitend,
>> + rport->port_cfg->utmi_ls.bitstart);
>> +
>> + /* stitch on utmi_ls and utmi_hstdet as phy state */
>> + state = ((uhd & uhd_mask) >> rport->port_cfg->utmi_hstdet.bitstart) |
>> + (((ul & ul_mask) >> rport->port_cfg->utmi_ls.bitstart) << sh);
>> +
>> + switch (state) {
>> + case PHY_STATE_HS_ONLINE:
>> + dev_dbg(&rport->phy->dev, "HS online\n");
>> + break;
>> + case PHY_STATE_FS_CONNECT:
>> + /*
>> + * For FS device, the online state share with connect state
>> + * from utmi_ls and utmi_hstdet register, so we distinguish
>> + * them via suspended flag.
>> + */
>> + if (!rport->suspended) {
>> + dev_dbg(&rport->phy->dev, "FS online\n");
>> + break;
>> + }
>> + /* fall through */
>> + case PHY_STATE_HS_CONNECT:
>> + if (rport->suspended) {
>> + dev_dbg(&rport->phy->dev, "HS/FS connected\n");
>> + rockchip_usb2phy_resume(rport->phy);
>> + rport->suspended = false;
>> + }
>> + break;
>> + case PHY_STATE_DISCONNECT:
>> + if (!rport->suspended) {
>> + dev_dbg(&rport->phy->dev, "HS/FS disconnected\n");
>> + rockchip_usb2phy_suspend(rport->phy);
>> + rport->suspended = true;
>> + }
>> +
>> + /*
>> + * activate the linestate detection to get the next device
>> + * plug-in irq.
>> + */
>> + property_enable(rphy, &rport->port_cfg->ls_det_clr, true);
>> + property_enable(rphy, &rport->port_cfg->ls_det_en, true);
>> +
>> + /*
>> + * we don't need to rearm the delayed work when the phy port
>> + * is suspended.
>> + */
>> + mutex_unlock(&rport->mutex);
>> + return;
>> + default:
>> + dev_dbg(&rport->phy->dev, "unknown phy state\n");
>> + break;
>> + }
>> +
>> +next_schedule:
>> + mutex_unlock(&rport->mutex);
>> + schedule_delayed_work(&rport->sm_work, SCHEDULE_DELAY);
>> +}
>> +
>> +static irqreturn_t rockchip_usb2phy_linestate_irq(int irq, void *data)
>> +{
>> + struct rockchip_usb2phy_port *rport = data;
>> + struct rockchip_usb2phy *rphy = dev_get_drvdata(rport->phy->dev.parent);
>> +
>> + if (!property_enabled(rphy, &rport->port_cfg->ls_det_st))
>> + return IRQ_NONE;
>> +
>> + mutex_lock(&rport->mutex);
>> +
>> + /* disable linestate detect irq and clear its status */
>> + property_enable(rphy, &rport->port_cfg->ls_det_en, false);
>> + property_enable(rphy, &rport->port_cfg->ls_det_clr, true);
>> +
>> + mutex_unlock(&rport->mutex);
>> +
>> + /*
>> + * In this case for host phy port, a new device is plugged in,
>> + * meanwhile, if the phy port is suspended, we need rearm the work to
>> + * resume it and mange its states; otherwise, we do nothing about that.
>> + */
>> + if (rport->suspended && rport->port_id == USB2PHY_PORT_HOST)
>> + rockchip_usb2phy_sm_work(&rport->sm_work.work);
>> +
>> + return IRQ_HANDLED;
>> +}
>> +
>> +static int rockchip_usb2phy_host_port_init(struct rockchip_usb2phy *rphy,
>> + struct rockchip_usb2phy_port *rport,
>> + struct device_node *child_np)
>> +{
>> + int ret;
>> +
>> + rport->port_id = USB2PHY_PORT_HOST;
>> + rport->port_cfg = &rphy->phy_cfg->port_cfgs[USB2PHY_PORT_HOST];
>> +
>> + mutex_init(&rport->mutex);
>> + INIT_DELAYED_WORK(&rport->sm_work, rockchip_usb2phy_sm_work);
>> +
>> + rport->ls_irq = of_irq_get_byname(child_np, "linestate");
>> + if (rport->ls_irq < 0) {
>> + dev_err(rphy->dev, "no linestate irq provided\n");
>> + return rport->ls_irq;
>> + }
>> +
>> + ret = devm_request_threaded_irq(rphy->dev, rport->ls_irq, NULL,
>> + rockchip_usb2phy_linestate_irq,
>> + IRQF_ONESHOT,
>> + "rockchip_usb2phy", rport);
>> + if (ret) {
>> + dev_err(rphy->dev, "failed to request irq handle\n");
>> + return ret;
>> + }
>> +
>> + return 0;
>> +}
>> +
>> +static int rockchip_usb2phy_probe(struct platform_device *pdev)
>> +{
>> + struct device *dev = &pdev->dev;
>> + struct device_node *np = dev->of_node;
>> + struct device_node *child_np;
>> + struct phy_provider *provider;
>> + struct rockchip_usb2phy *rphy;
>> + const struct rockchip_usb2phy_cfg *phy_cfgs;
>> + const struct of_device_id *match;
>> + unsigned int reg;
>> + int index, ret;
>> +
>> + rphy = devm_kzalloc(dev, sizeof(*rphy), GFP_KERNEL);
>> + if (!rphy)
>> + return -ENOMEM;
>> +
>> + match = of_match_device(dev->driver->of_match_table, dev);
>> + if (!match || !match->data) {
>> + dev_err(dev, "phy configs are not assigned!\n");
>> + return -EINVAL;
>> + }
>> +
>> + if (!dev->parent || !dev->parent->of_node)
>> + return -EINVAL;
>> +
>> + rphy->grf = syscon_node_to_regmap(dev->parent->of_node);
>> + if (IS_ERR(rphy->grf))
>> + return PTR_ERR(rphy->grf);
>> +
>> + if (of_property_read_u32(np, "reg", ®)) {
>> + dev_err(dev, "the reg property is not assigned in %s node\n",
>> + np->name);
>> + return -EINVAL;
>> + }
>> +
>> + rphy->dev = dev;
>> + phy_cfgs = match->data;
>> + platform_set_drvdata(pdev, rphy);
>> +
>> + /* find out a proper config which can be matched with dt. */
>> + index = 0;
>> + while (phy_cfgs[index].reg) {
>> + if (phy_cfgs[index].reg == reg) {
>> + rphy->phy_cfg = &phy_cfgs[index];
>> + break;
>> + }
>> +
>> + ++index;
>> + }
>> +
>> + if (!rphy->phy_cfg) {
>> + dev_err(dev, "no phy-config can be matched with %s node\n",
>> + np->name);
>> + return -EINVAL;
>> + }
>> +
>> + ret = rockchip_usb2phy_clk480m_register(rphy);
>> + if (ret) {
>> + dev_err(dev, "failed to register 480m output clock\n");
>> + return ret;
>> + }
>> +
>> + index = 0;
>> + for_each_available_child_of_node(np, child_np) {
>> + struct rockchip_usb2phy_port *rport = &rphy->ports[index];
>> + struct phy *phy;
>> +
>> + phy = devm_phy_create(dev, child_np, &rockchip_usb2phy_ops);
>> + if (IS_ERR(phy)) {
>> + dev_err(dev, "failed to create phy\n");
>> + ret = PTR_ERR(phy);
>> + goto put_child;
>> + }
>> +
>> + rport->phy = phy;
>
> phy_set_drvdata(rport->phy, rport);
>
>> +
>> + /* initialize otg/host port separately */
>> + if (!of_node_cmp(child_np->name, "host-port")) {
>> + ret = rockchip_usb2phy_host_port_init(rphy, rport,
>> + child_np);
>> + if (ret)
>> + goto put_child;
>> + }
>> +
>> + phy_set_drvdata(rport->phy, rport);
>
> move this to the location above to prevent null-pointer dereferences with
> devices plugged in on boot.
>
> I've tested this a bit on a rk3036 (which is lacking the disconnect-detection
> it seems), so in general (apart from the stuff mentioned above) this looks
> nice now. So with the stuff above fixed:
>
> Reviewed-by: Heiko Stuebner <heiko@sntech.de>
> Tested-by: Heiko Stuebner <heiko@sntech.de>
>
>
> Heiko
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH v5 2/2] phy: rockchip-inno-usb2: add a new driver for Rockchip usb2phy
2016-06-14 13:50 ` Guenter Roeck
@ 2016-06-14 14:00 ` Heiko Stübner
2016-06-15 1:14 ` Frank Wang
0 siblings, 1 reply; 25+ messages in thread
From: Heiko Stübner @ 2016-06-14 14:00 UTC (permalink / raw)
To: Guenter Roeck
Cc: Frank Wang, Douglas Anderson, Guenter Roeck, Guenter Roeck,
jwerner, kishon, robh+dt, pawel.moll, mark.rutland,
ijc+devicetree, Kumar Gala, linux-kernel, devicetree, linux-usb,
linux-rockchip, Ziyuan Xu, Kever Yang, Tao Huang, william.wu
Am Dienstag, 14. Juni 2016, 06:50:31 schrieb Guenter Roeck:
> On Tue, Jun 14, 2016 at 6:27 AM, Heiko Stübner <heiko@sntech.de> wrote:
> > Am Montag, 13. Juni 2016, 10:10:10 schrieb Frank Wang:
> >> The newer SoCs (rk3366, rk3399) take a different usb-phy IP block
> >> than rk3288 and before, and most of phy-related registers are also
> >> different from the past, so a new phy driver is required necessarily.
> >>
> >> Signed-off-by: Frank Wang <frank.wang@rock-chips.com>
> >> ---
[...]
> >> +static int rockchip_usb2phy_init(struct phy *phy)
> >> +{
> >> + struct rockchip_usb2phy_port *rport = phy_get_drvdata(phy);
> >> + struct rockchip_usb2phy *rphy = dev_get_drvdata(phy->dev.parent);
> >> + int ret;
> >> +
> >>
> > if (!rport->port_cfg)
> >
> > return 0;
> >
> > Otherwise the currently empty otg-port will cause null-pointer
> > dereferences
> > when it gets assigned in the devicetree already.
>
> Not really, at least not here - that port should not have port_id set
> to USB2PHY_PORT_HOST.
>
> Does it even make sense to instantiate the otg port ? Is it going to
> do anything without port configuration ?
Ok, that would be the other option - not creating the phy in the driver.
Or from what I've seen, handling it as similar to the host-port should work
initially as well most likely, supplying the additional otg-parts later on.
[...]
> >> +static int rockchip_usb2phy_exit(struct phy *phy)
> >> +{
> >> + struct rockchip_usb2phy_port *rport = phy_get_drvdata(phy);
> >> +
> >>
> > if (!rport->port_cfg)
> >
> > return 0;
>
> No access to port_cfg here ?
sorry, one copy'n'paste to many :-)
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH v5 2/2] phy: rockchip-inno-usb2: add a new driver for Rockchip usb2phy
2016-06-13 2:10 ` [PATCH v5 2/2] phy: rockchip-inno-usb2: add a new driver for Rockchip usb2phy Frank Wang
2016-06-14 13:27 ` Heiko Stübner
@ 2016-06-14 14:52 ` Guenter Roeck
2016-06-14 15:20 ` Heiko Stübner
2016-06-17 11:54 ` Kishon Vijay Abraham I
2 siblings, 1 reply; 25+ messages in thread
From: Guenter Roeck @ 2016-06-14 14:52 UTC (permalink / raw)
To: Frank Wang, heiko, dianders, groeck, jwerner, kishon, robh+dt,
pawel.moll, mark.rutland, ijc+devicetree, galak
Cc: linux-kernel, devicetree, linux-usb, linux-rockchip, xzy.xu,
kever.yang, huangtao, william.wu
On 06/12/2016 07:10 PM, Frank Wang wrote:
> The newer SoCs (rk3366, rk3399) take a different usb-phy IP block
> than rk3288 and before, and most of phy-related registers are also
> different from the past, so a new phy driver is required necessarily.
>
> Signed-off-by: Frank Wang <frank.wang@rock-chips.com>
> ---
>
[ ... ]
> +
> +static int rockchip_usb2phy_resume(struct phy *phy)
> +{
> + struct rockchip_usb2phy_port *rport = phy_get_drvdata(phy);
> + struct rockchip_usb2phy *rphy = dev_get_drvdata(phy->dev.parent);
> + int ret;
> +
> + dev_dbg(&rport->phy->dev, "port resume\n");
> +
> + ret = clk_prepare_enable(rphy->clk480m);
> + if (ret)
> + return ret;
> +
> + ret = property_enable(rphy, &rport->port_cfg->phy_sus, false);
> + if (ret)
> + return ret;
> +
> + rport->suspended = false;
> + return 0;
> +}
> +
> +static int rockchip_usb2phy_suspend(struct phy *phy)
> +{
> + struct rockchip_usb2phy_port *rport = phy_get_drvdata(phy);
> + struct rockchip_usb2phy *rphy = dev_get_drvdata(phy->dev.parent);
> + int ret;
> +
> + dev_dbg(&rport->phy->dev, "port suspend\n");
> +
> + ret = property_enable(rphy, &rport->port_cfg->phy_sus, true);
> + if (ret)
> + return ret;
> +
> + rport->suspended = true;
> + clk_disable_unprepare(rphy->clk480m);
> + return 0;
> +}
> +
I am still quite confused by the clock handling.
The above will be called for each instantiated phy (user, otg).
Each time, clk_disable_unprepare() will be called. Yet, there
is no matching clk_prepare_enable() call during initialization.
How does this work ?
Thanks,
Guenter
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH v5 2/2] phy: rockchip-inno-usb2: add a new driver for Rockchip usb2phy
2016-06-14 14:52 ` Guenter Roeck
@ 2016-06-14 15:20 ` Heiko Stübner
0 siblings, 0 replies; 25+ messages in thread
From: Heiko Stübner @ 2016-06-14 15:20 UTC (permalink / raw)
To: Guenter Roeck
Cc: Frank Wang, dianders, groeck, jwerner, kishon, robh+dt,
pawel.moll, mark.rutland, ijc+devicetree, galak, linux-kernel,
devicetree, linux-usb, linux-rockchip, xzy.xu, kever.yang,
huangtao, william.wu
Am Dienstag, 14. Juni 2016, 07:52:13 schrieb Guenter Roeck:
> On 06/12/2016 07:10 PM, Frank Wang wrote:
> > The newer SoCs (rk3366, rk3399) take a different usb-phy IP block
> > than rk3288 and before, and most of phy-related registers are also
> > different from the past, so a new phy driver is required necessarily.
> >
> > Signed-off-by: Frank Wang <frank.wang@rock-chips.com>
> > ---
>
> [ ... ]
>
> > +
> > +static int rockchip_usb2phy_resume(struct phy *phy)
> > +{
> > + struct rockchip_usb2phy_port *rport = phy_get_drvdata(phy);
> > + struct rockchip_usb2phy *rphy = dev_get_drvdata(phy->dev.parent);
> > + int ret;
> > +
> > + dev_dbg(&rport->phy->dev, "port resume\n");
> > +
> > + ret = clk_prepare_enable(rphy->clk480m);
> > + if (ret)
> > + return ret;
> > +
> > + ret = property_enable(rphy, &rport->port_cfg->phy_sus, false);
> > + if (ret)
> > + return ret;
> > +
> > + rport->suspended = false;
> > + return 0;
> > +}
> > +
> > +static int rockchip_usb2phy_suspend(struct phy *phy)
> > +{
> > + struct rockchip_usb2phy_port *rport = phy_get_drvdata(phy);
> > + struct rockchip_usb2phy *rphy = dev_get_drvdata(phy->dev.parent);
> > + int ret;
> > +
> > + dev_dbg(&rport->phy->dev, "port suspend\n");
> > +
> > + ret = property_enable(rphy, &rport->port_cfg->phy_sus, true);
> > + if (ret)
> > + return ret;
> > +
> > + rport->suspended = true;
> > + clk_disable_unprepare(rphy->clk480m);
> > + return 0;
> > +}
> > +
>
> I am still quite confused by the clock handling.
>
> The above will be called for each instantiated phy (user, otg).
> Each time, clk_disable_unprepare() will be called. Yet, there
> is no matching clk_prepare_enable() call during initialization.
>
> How does this work ?
the created clock gets the supplying clock as parent, see
+ rphy->clk = of_clk_get_by_name(node, "phyclk");
+ if (IS_ERR(rphy->clk)) {
+ rphy->clk = NULL;
+ init.parent_names = NULL;
+ init.num_parents = 0;
+ } else {
+ clk_name = __clk_get_name(rphy->clk);
+ init.parent_names = &clk_name;
+ init.num_parents = 1;
+ }
that way when you enable the 480m clock from the phy, its parent will
automatically get enabled as well. And of course due to refcounting in the
clock framework, the 480m clock (and thus also its parent) will only get
disabled once both the host and otg port have disabled it.
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH v5 1/2] Documentation: bindings: add DT documentation for Rockchip USB2PHY
2016-06-13 2:10 ` [PATCH v5 1/2] Documentation: bindings: add DT documentation for Rockchip USB2PHY Frank Wang
2016-06-13 8:38 ` Heiko Stübner
@ 2016-06-14 22:26 ` Rob Herring
1 sibling, 0 replies; 25+ messages in thread
From: Rob Herring @ 2016-06-14 22:26 UTC (permalink / raw)
To: Frank Wang
Cc: heiko, dianders, linux, groeck, jwerner, kishon, pawel.moll,
mark.rutland, ijc+devicetree, galak, linux-kernel, devicetree,
linux-usb, linux-rockchip, xzy.xu, kever.yang, huangtao,
william.wu
On Mon, Jun 13, 2016 at 10:10:09AM +0800, Frank Wang wrote:
> Signed-off-by: Frank Wang <frank.wang@rock-chips.com>
> ---
>
> Changes in v5:
> - Added 'reg' property to identify the different phy-blocks.
>
> Changes in v4:
> - Used 'phy-supply' instead of 'vbus_*-supply'.
>
> Changes in v3:
> - Added 'clocks' and 'clock-names' optional properties.
> - Specified 'otg-port' and 'host-port' as the sub-node name.
>
> Changes in v2:
> - Changed vbus_host optional property from gpio to regulator.
> - Specified vbus_otg-supply optional property.
> - Specified otg_id and otg_bvalid property.
>
> .../bindings/phy/phy-rockchip-inno-usb2.txt | 64 ++++++++++++++++++++
> 1 file changed, 64 insertions(+)
> create mode 100644 Documentation/devicetree/bindings/phy/phy-rockchip-inno-usb2.txt
Acked-by: Rob Herring <robh@kernel.org>
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH v5 2/2] phy: rockchip-inno-usb2: add a new driver for Rockchip usb2phy
2016-06-14 14:00 ` Heiko Stübner
@ 2016-06-15 1:14 ` Frank Wang
2016-06-15 15:47 ` Guenter Roeck
0 siblings, 1 reply; 25+ messages in thread
From: Frank Wang @ 2016-06-15 1:14 UTC (permalink / raw)
To: Heiko Stübner, Guenter Roeck
Cc: Douglas Anderson, Guenter Roeck, Guenter Roeck, jwerner, kishon,
robh+dt, pawel.moll, mark.rutland, ijc+devicetree, Kumar Gala,
linux-kernel, devicetree, linux-usb, linux-rockchip, Ziyuan Xu,
Kever Yang, Tao Huang, william.wu, frank.wang
Hi Heiko & Guenter,
On 2016/6/14 22:00, Heiko Stübner wrote:
> Am Dienstag, 14. Juni 2016, 06:50:31 schrieb Guenter Roeck:
>> On Tue, Jun 14, 2016 at 6:27 AM, Heiko Stübner <heiko@sntech.de> wrote:
>>> Am Montag, 13. Juni 2016, 10:10:10 schrieb Frank Wang:
>>>> The newer SoCs (rk3366, rk3399) take a different usb-phy IP block
>>>> than rk3288 and before, and most of phy-related registers are also
>>>> different from the past, so a new phy driver is required necessarily.
>>>>
>>>> Signed-off-by: Frank Wang <frank.wang@rock-chips.com>
>>>> ---
> [...]
>
>>>> +static int rockchip_usb2phy_init(struct phy *phy)
>>>> +{
>>>> + struct rockchip_usb2phy_port *rport = phy_get_drvdata(phy);
>>>> + struct rockchip_usb2phy *rphy = dev_get_drvdata(phy->dev.parent);
>>>> + int ret;
>>>> +
>>>>
>>> if (!rport->port_cfg)
>>>
>>> return 0;
>>>
>>> Otherwise the currently empty otg-port will cause null-pointer
>>> dereferences
>>> when it gets assigned in the devicetree already.
>> Not really, at least not here - that port should not have port_id set
>> to USB2PHY_PORT_HOST.
>>
>> Does it even make sense to instantiate the otg port ? Is it going to
>> do anything without port configuration ?
> Ok, that would be the other option - not creating the phy in the driver.
Well, I will put this conditional inside *_host_port_init(), if it is an
empty, the phy-device should not be created.
Something like the following:
--- a/drivers/phy/phy-rockchip-inno-usb2.c
+++ b/drivers/phy/phy-rockchip-inno-usb2.c
@@ -483,9 +483,13 @@ static int rockchip_usb2phy_host_port_init(struct
rockchip_usb2phy *rphy,
{
int ret;
- rport->port_id = USB2PHY_PORT_HOST;
rport->port_cfg = &rphy->phy_cfg->port_cfgs[USB2PHY_PORT_HOST];
+ if (!rport->port_cfg) {
+ dev_err(rphy->dev, "no host port-config provided.\n");
+ return -EINVAL;
+ }
+ rport->port_id = USB2PHY_PORT_HOST;
> Or from what I've seen, handling it as similar to the host-port should work
> initially as well most likely, supplying the additional otg-parts later on.
@Guenter, just as Heiko said, the otg-parts is not ready now, it will be
supplied later.
BR.
Frank
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH v5 1/2] Documentation: bindings: add DT documentation for Rockchip USB2PHY
2016-06-14 13:28 ` Heiko Stübner
@ 2016-06-15 1:24 ` Frank Wang
0 siblings, 0 replies; 25+ messages in thread
From: Frank Wang @ 2016-06-15 1:24 UTC (permalink / raw)
To: Heiko Stübner
Cc: dianders, linux, groeck, jwerner, kishon, robh+dt, pawel.moll,
mark.rutland, ijc+devicetree, galak, linux-kernel, devicetree,
linux-usb, linux-rockchip, xzy.xu, kever.yang, huangtao,
william.wu, frank.wang
Hi Heiko,
On 2016/6/14 21:28, Heiko Stübner wrote:
> Am Montag, 13. Juni 2016, 10:38:39 schrieb Heiko Stübner:
>> Am Montag, 13. Juni 2016, 10:10:09 schrieb Frank Wang:
>>> Signed-off-by: Frank Wang <frank.wang@rock-chips.com>
>> looks really cool now, thanks for addressing all the review comments
>>
>> Reviewed-by: Heiko Stuebner <heiko@sntech.de>
> You've only added the very common "reg" property, so I guess it should be just
> fine to keep Rob's Ack on the binding in that case.
OK, I will keep Rob's Ack in the next patch.
> I noticed one thing below though.
> As you'll probably need to do another version (see comments to patch2), you
> could change that as well in both here and in the second patch - see below:
Well, I will correct them and send out with a new version later.
>>> ---
>>>
>>> Changes in v5:
>>> - Added 'reg' property to identify the different phy-blocks.
>>>
>>> Changes in v4:
>>> - Used 'phy-supply' instead of 'vbus_*-supply'.
>>>
>>> Changes in v3:
>>> - Added 'clocks' and 'clock-names' optional properties.
>>> - Specified 'otg-port' and 'host-port' as the sub-node name.
>>>
>>> Changes in v2:
>>> - Changed vbus_host optional property from gpio to regulator.
>>> - Specified vbus_otg-supply optional property.
>>> - Specified otg_id and otg_bvalid property.
>>>
>>> .../bindings/phy/phy-rockchip-inno-usb2.txt | 64
>>>
>>> ++++++++++++++++++++ 1 file changed, 64 insertions(+)
>>>
>>> create mode 100644
>>>
>>> Documentation/devicetree/bindings/phy/phy-rockchip-inno-usb2.txt
>>>
>>> diff --git
>>> a/Documentation/devicetree/bindings/phy/phy-rockchip-inno-usb2.txt
>>> b/Documentation/devicetree/bindings/phy/phy-rockchip-inno-usb2.txt new
>>> file
>>> mode 100644
>>> index 0000000..48bb5de
>>> --- /dev/null
>>> +++ b/Documentation/devicetree/bindings/phy/phy-rockchip-inno-usb2.txt
>>> @@ -0,0 +1,64 @@
>>> +ROCKCHIP USB2.0 PHY WITH INNO IP BLOCK
>>> +
>>> +Required properties (phy (parent) node):
>>> + - compatible : should be one of the listed compatibles:
>>> + * "rockchip,rk3366-usb2phy"
>>> + * "rockchip,rk3399-usb2phy"
>>> + - reg : the address offset of grf for usb-phy configuration.
>>> + - #clock-cells : should be 0.
>>> + - clock-output-names : specify the 480m output clock name.
>>> +
>>> +Optional properties:
>>> + - clocks : phandle + phy specifier pair, for the input clock of phy.
>>> + - clock-names : input clock name of phy, must be "phyclk".
>>> +
>>> +Required nodes : a sub-node is required for each port the phy provides.
>>> + The sub-node name is used to identify host or otg port,
>>> + and shall be the following entries:
>>> + * "otg-port" : the name of otg port.
>>> + * "host-port" : the name of host port.
>>> +
>>> +Required properties (port (child) node):
>>> + - #phy-cells : must be 0. See ./phy-bindings.txt for details.
>>> + - interrupts : specify an interrupt for each entry in interrupt-names.
>>> + - interrupt-names : a list which shall be the following entries:
>>> + * "otg_id" : for the otg id interrupt.
>>> + * "otg_bvalid" : for the otg vbus interrupt.
> please use "-" not underscores in dt-bindings, so "otg-id", "otg-bvalid".
>
>
>>> + * "linestate" : for the host/otg linestate interrupt.
>>> +
>>> +Optional properties:
>>> + - phy-supply : phandle to a regulator that provides power to VBUS.
>>> + See ./phy-bindings.txt for details.
>>> +
>>> +Example:
>>> +
>>> +grf: syscon@ff770000 {
>>> + compatible = "rockchip,rk3366-grf", "syscon", "simple-mfd";
>>> + #address-cells = <1>;
>>> + #size-cells = <1>;
>>> +
>>> +...
>>> +
>>> + u2phy: usb2-phy@700 {
>>> + compatible = "rockchip,rk3366-usb2phy";
>>> + reg = <0x700 0x2c>;
>>> + #clock-cells = <0>;
>>> + clock-output-names = "sclk_otgphy0_480m";
>>> +
>>> + u2phy_otg: otg-port {
>>> + #phy-cells = <0>;
>>> + interrupts = <GIC_SPI 93 IRQ_TYPE_LEVEL_HIGH>,
>>> + <GIC_SPI 94 IRQ_TYPE_LEVEL_HIGH>,
>>> + <GIC_SPI 95 IRQ_TYPE_LEVEL_HIGH>;
>>> + interrupt-names = "otg_id", "otg_bvalid", "linestate";
> again "-" not "_".
>
>
>>> + status = "okay";
>>> + };
>>> +
>>> + u2phy_host: host-port {
>>> + #phy-cells = <0>;
>>> + interrupts = <GIC_SPI 96 IRQ_TYPE_LEVEL_HIGH>;
>>> + interrupt-names = "linestate";
>>> + status = "okay";
>>> + };
>>> + };
>>> +};
>
BR.
Frank
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH v5 2/2] phy: rockchip-inno-usb2: add a new driver for Rockchip usb2phy
2016-06-14 13:27 ` Heiko Stübner
2016-06-14 13:50 ` Guenter Roeck
@ 2016-06-15 3:23 ` Frank Wang
2016-06-15 9:04 ` Heiko Stübner
1 sibling, 1 reply; 25+ messages in thread
From: Frank Wang @ 2016-06-15 3:23 UTC (permalink / raw)
To: Heiko Stübner
Cc: dianders, linux, groeck, jwerner, kishon, robh+dt, pawel.moll,
mark.rutland, ijc+devicetree, galak, linux-kernel, devicetree,
linux-usb, linux-rockchip, xzy.xu, kever.yang, huangtao,
william.wu, frank.wang
Hi Heiko,
On 2016/6/14 21:27, Heiko Stübner wrote:
> Am Montag, 13. Juni 2016, 10:10:10 schrieb Frank Wang:
>> The newer SoCs (rk3366, rk3399) take a different usb-phy IP block
>> than rk3288 and before, and most of phy-related registers are also
>> different from the past, so a new phy driver is required necessarily.
>>
>> Signed-off-by: Frank Wang <frank.wang@rock-chips.com>
>> ---
>>
>> Changes in v5:
>> - Added 'reg' in the data block to match the different phy-blocks in dt.
>>
>> Changes in v4:
>> - Removed some processes related to 'vbus_host-supply'.
>>
>> Changes in v3:
>> - Resolved the mapping defect between fixed value in driver and the
>> property in devicetree.
>> - Optimized 480m output clock register function.
>> - Code cleanup.
>>
>> Changes in v2:
>> - Changed vbus_host operation from gpio to regulator in *_probe.
>> - Improved the fault treatment relate to 480m clock register.
>> - Cleaned up some meaningless codes in *_clk480m_disable.
>> - made more clear the comment of *_sm_work.
>>
>> drivers/phy/Kconfig | 7 +
>> drivers/phy/Makefile | 1 +
>> drivers/phy/phy-rockchip-inno-usb2.c | 645
>> ++++++++++++++++++++++++++++++++++ 3 files changed, 653 insertions(+)
>>
>> [...]
>>
>> +
>> +static int rockchip_usb2phy_exit(struct phy *phy)
>> +{
>> + struct rockchip_usb2phy_port *rport = phy_get_drvdata(phy);
>> +
> if (!rport->port_cfg)
> return 0;
>
>> + if (rport->port_id == USB2PHY_PORT_HOST)
>> + cancel_delayed_work_sync(&rport->sm_work);
>> +
> you will also need to resume the port here, if it is suspended at this point,
> as phy_power_off gets called after phy_exit and would probably produce clk
> enable/disable mismatches otherwise.
>
Hmm, from my personal point of view, when canceling sm_work here, it may
not cause the port goes to suspend, isn't it? besides, clk only prepared
in *_usb2phy_resume(), and unprepared in *_usb2phy_suspend(), so if we
resume port here, the prepare_count of clk will be increased again, I
am afraid this is not correct, and am I wrong? would you like to tell me
more details?
>> + return 0;
>> +}
>>
>> [...]
>>
>> +static int rockchip_usb2phy_probe(struct platform_device *pdev)
>> +{
>> + struct device *dev = &pdev->dev;
>> + struct device_node *np = dev->of_node;
>> + struct device_node *child_np;
>>
>> [...]
>>
>> + index = 0;
>> + for_each_available_child_of_node(np, child_np) {
>> + struct rockchip_usb2phy_port *rport = &rphy->ports[index];
>> + struct phy *phy;
>> +
>> + phy = devm_phy_create(dev, child_np, &rockchip_usb2phy_ops);
>> + if (IS_ERR(phy)) {
>> + dev_err(dev, "failed to create phy\n");
>> + ret = PTR_ERR(phy);
>> + goto put_child;
>> + }
>> +
>> + rport->phy = phy;
> phy_set_drvdata(rport->phy, rport);
>
>> +
>> + /* initialize otg/host port separately */
>> + if (!of_node_cmp(child_np->name, "host-port")) {
>> + ret = rockchip_usb2phy_host_port_init(rphy, rport,
>> + child_np);
>> + if (ret)
>> + goto put_child;
>> + }
>> +
>> + phy_set_drvdata(rport->phy, rport);
> move this to the location above to prevent null-pointer dereferences with
> devices plugged in on boot.
OK, I will fix it.
>
> I've tested this a bit on a rk3036 (which is lacking the disconnect-detection
> it seems), so in general (apart from the stuff mentioned above) this looks
> nice now. So with the stuff above fixed:
>
> Reviewed-by: Heiko Stuebner <heiko@sntech.de>
> Tested-by: Heiko Stuebner <heiko@sntech.de>
BR.
Frank
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH v5 2/2] phy: rockchip-inno-usb2: add a new driver for Rockchip usb2phy
2016-06-15 3:23 ` Frank Wang
@ 2016-06-15 9:04 ` Heiko Stübner
2016-06-15 10:58 ` Frank Wang
0 siblings, 1 reply; 25+ messages in thread
From: Heiko Stübner @ 2016-06-15 9:04 UTC (permalink / raw)
To: Frank Wang
Cc: dianders, linux, groeck, jwerner, kishon, robh+dt, pawel.moll,
mark.rutland, ijc+devicetree, galak, linux-kernel, devicetree,
linux-usb, linux-rockchip, xzy.xu, kever.yang, huangtao,
william.wu
Hi Frank,
Am Mittwoch, 15. Juni 2016, 11:23:26 schrieb Frank Wang:
> On 2016/6/14 21:27, Heiko Stübner wrote:
> > Am Montag, 13. Juni 2016, 10:10:10 schrieb Frank Wang:
> >> The newer SoCs (rk3366, rk3399) take a different usb-phy IP block
> >> than rk3288 and before, and most of phy-related registers are also
> >> different from the past, so a new phy driver is required necessarily.
> >>
> >> Signed-off-by: Frank Wang <frank.wang@rock-chips.com>
> >> ---
> >>
> >> Changes in v5:
> >> - Added 'reg' in the data block to match the different phy-blocks in
> >> dt.
> >>
> >> Changes in v4:
> >> - Removed some processes related to 'vbus_host-supply'.
> >>
> >> Changes in v3:
> >> - Resolved the mapping defect between fixed value in driver and the
> >>
> >> property in devicetree.
> >>
> >> - Optimized 480m output clock register function.
> >> - Code cleanup.
> >>
> >> Changes in v2:
> >> - Changed vbus_host operation from gpio to regulator in *_probe.
> >> - Improved the fault treatment relate to 480m clock register.
> >> - Cleaned up some meaningless codes in *_clk480m_disable.
> >> - made more clear the comment of *_sm_work.
> >>
> >> drivers/phy/Kconfig | 7 +
> >> drivers/phy/Makefile | 1 +
> >> drivers/phy/phy-rockchip-inno-usb2.c | 645
> >>
> >> ++++++++++++++++++++++++++++++++++ 3 files changed, 653 insertions(+)
> >>
> >> [...]
> >>
> >> +
> >> +static int rockchip_usb2phy_exit(struct phy *phy)
> >> +{
> >> + struct rockchip_usb2phy_port *rport = phy_get_drvdata(phy);
> >> +
> >>
> > if (!rport->port_cfg)
> >
> > return 0;
> >>
> >> + if (rport->port_id == USB2PHY_PORT_HOST)
> >> + cancel_delayed_work_sync(&rport->sm_work);
> >> +
> >
> > you will also need to resume the port here, if it is suspended at this
> > point, as phy_power_off gets called after phy_exit and would probably
> > produce clk enable/disable mismatches otherwise.
>
> Hmm, from my personal point of view, when canceling sm_work here, it may
> not cause the port goes to suspend, isn't it? besides, clk only prepared
> in *_usb2phy_resume(), and unprepared in *_usb2phy_suspend(), so if we
> resume port here, the prepare_count of clk will be increased again, I
> am afraid this is not correct, and am I wrong? would you like to tell me
> more details?
usb2phy_resume gets called both initially through phy_power_on as well.
So it's on but through the first scheduled work call, might get suspended when
nothing is connected. (clk_enable and clk_disable will run).
If nothing is connected on unload phy_power_off will get called while the
clock actually is still disabled.
So I think it's either resuming on exit, or at least making sure to do nothing
in that case in the phy_power_off callback of the driver.
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH v5 2/2] phy: rockchip-inno-usb2: add a new driver for Rockchip usb2phy
2016-06-15 9:04 ` Heiko Stübner
@ 2016-06-15 10:58 ` Frank Wang
2016-06-15 18:49 ` Heiko Stübner
0 siblings, 1 reply; 25+ messages in thread
From: Frank Wang @ 2016-06-15 10:58 UTC (permalink / raw)
To: Heiko Stübner
Cc: dianders, linux, groeck, jwerner, kishon, robh+dt, pawel.moll,
mark.rutland, ijc+devicetree, galak, linux-kernel, devicetree,
linux-usb, linux-rockchip, xzy.xu, kever.yang, huangtao,
william.wu, frank.wang
Hi Heiko,
On 2016/6/15 17:04, Heiko Stübner wrote:
> Hi Frank,
>
> Am Mittwoch, 15. Juni 2016, 11:23:26 schrieb Frank Wang:
>> On 2016/6/14 21:27, Heiko Stübner wrote:
>>> Am Montag, 13. Juni 2016, 10:10:10 schrieb Frank Wang:
>>>> The newer SoCs (rk3366, rk3399) take a different usb-phy IP block
>>>> than rk3288 and before, and most of phy-related registers are also
>>>> different from the past, so a new phy driver is required necessarily.
>>>>
>>>> Signed-off-by: Frank Wang <frank.wang@rock-chips.com>
>>>> ---
>>>>
>>>> Changes in v5:
>>>> - Added 'reg' in the data block to match the different phy-blocks in
>>>> dt.
>>>>
>>>> Changes in v4:
>>>> - Removed some processes related to 'vbus_host-supply'.
>>>>
>>>> Changes in v3:
>>>> - Resolved the mapping defect between fixed value in driver and the
>>>>
>>>> property in devicetree.
>>>>
>>>> - Optimized 480m output clock register function.
>>>> - Code cleanup.
>>>>
>>>> Changes in v2:
>>>> - Changed vbus_host operation from gpio to regulator in *_probe.
>>>> - Improved the fault treatment relate to 480m clock register.
>>>> - Cleaned up some meaningless codes in *_clk480m_disable.
>>>> - made more clear the comment of *_sm_work.
>>>>
>>>> drivers/phy/Kconfig | 7 +
>>>> drivers/phy/Makefile | 1 +
>>>> drivers/phy/phy-rockchip-inno-usb2.c | 645
>>>>
>>>> ++++++++++++++++++++++++++++++++++ 3 files changed, 653 insertions(+)
>>>>
>>>> [...]
>>>>
>>>> +
>>>> +static int rockchip_usb2phy_exit(struct phy *phy)
>>>> +{
>>>> + struct rockchip_usb2phy_port *rport = phy_get_drvdata(phy);
>>>> +
>>>>
>>> if (!rport->port_cfg)
>>>
>>> return 0;
>>>> + if (rport->port_id == USB2PHY_PORT_HOST)
>>>> + cancel_delayed_work_sync(&rport->sm_work);
>>>> +
>>> you will also need to resume the port here, if it is suspended at this
>>> point, as phy_power_off gets called after phy_exit and would probably
>>> produce clk enable/disable mismatches otherwise.
>> Hmm, from my personal point of view, when canceling sm_work here, it may
>> not cause the port goes to suspend, isn't it? besides, clk only prepared
>> in *_usb2phy_resume(), and unprepared in *_usb2phy_suspend(), so if we
>> resume port here, the prepare_count of clk will be increased again, I
>> am afraid this is not correct, and am I wrong? would you like to tell me
>> more details?
> usb2phy_resume gets called both initially through phy_power_on as well.
> So it's on but through the first scheduled work call, might get suspended when
> nothing is connected. (clk_enable and clk_disable will run).
> If nothing is connected on unload phy_power_off will get called while the
> clock actually is still disabled.
>
> So I think it's either resuming on exit, or at least making sure to do nothing
> in that case in the phy_power_off callback of the driver.
>
>
Well, I got what you mean. I saw phy_power_on() gets called twice at
ehci/ohci driver probe time, one is at pdata->power_on(); another is at
usb_add_hcd(), so after that, the power_count of phy increases to two.
however, when ehci/ohci driver goes to suspend, there is only once to
call phy_power_off(), and the power_count of phy decreases to 1, so our
usb2phy_resume() could not be invoked :-) .
If so, is it still need to care it? How about use suspended member of
rockchip_usb2phy_port as a conditional to check in *_usb2phy_suspend(),
if necessary.
BR.
Frank
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH v5 2/2] phy: rockchip-inno-usb2: add a new driver for Rockchip usb2phy
2016-06-15 1:14 ` Frank Wang
@ 2016-06-15 15:47 ` Guenter Roeck
2016-06-16 1:47 ` Frank Wang
0 siblings, 1 reply; 25+ messages in thread
From: Guenter Roeck @ 2016-06-15 15:47 UTC (permalink / raw)
To: Frank Wang
Cc: Heiko Stübner, Douglas Anderson, Guenter Roeck,
Guenter Roeck, jwerner, kishon, robh+dt, pawel.moll,
mark.rutland, ijc+devicetree, Kumar Gala, linux-kernel,
devicetree, linux-usb, linux-rockchip, Ziyuan Xu, Kever Yang,
Tao Huang, william.wu
On Tue, Jun 14, 2016 at 6:14 PM, Frank Wang <frank.wang@rock-chips.com> wrote:
> Hi Heiko & Guenter,
>
>
> On 2016/6/14 22:00, Heiko Stübner wrote:
>>
>> Am Dienstag, 14. Juni 2016, 06:50:31 schrieb Guenter Roeck:
>>>
>>> On Tue, Jun 14, 2016 at 6:27 AM, Heiko Stübner <heiko@sntech.de> wrote:
>>>>
>>>> Am Montag, 13. Juni 2016, 10:10:10 schrieb Frank Wang:
>>>>>
>>>>> The newer SoCs (rk3366, rk3399) take a different usb-phy IP block
>>>>> than rk3288 and before, and most of phy-related registers are also
>>>>> different from the past, so a new phy driver is required necessarily.
>>>>>
>>>>> Signed-off-by: Frank Wang <frank.wang@rock-chips.com>
>>>>> ---
>>
>> [...]
>>
>>>>> +static int rockchip_usb2phy_init(struct phy *phy)
>>>>> +{
>>>>> + struct rockchip_usb2phy_port *rport = phy_get_drvdata(phy);
>>>>> + struct rockchip_usb2phy *rphy = dev_get_drvdata(phy->dev.parent);
>>>>> + int ret;
>>>>> +
>>>>>
>>>> if (!rport->port_cfg)
>>>> return 0;
>>>>
>>>> Otherwise the currently empty otg-port will cause null-pointer
>>>> dereferences
>>>> when it gets assigned in the devicetree already.
>>>
>>> Not really, at least not here - that port should not have port_id set
>>> to USB2PHY_PORT_HOST.
>>>
>>> Does it even make sense to instantiate the otg port ? Is it going to
>>> do anything without port configuration ?
>>
>> Ok, that would be the other option - not creating the phy in the driver.
>
>
> Well, I will put this conditional inside *_host_port_init(), if it is an
> empty, the phy-device should not be created.
> Something like the following:
>
> --- a/drivers/phy/phy-rockchip-inno-usb2.c
> +++ b/drivers/phy/phy-rockchip-inno-usb2.c
> @@ -483,9 +483,13 @@ static int rockchip_usb2phy_host_port_init(struct
> rockchip_usb2phy *rphy,
> {
> int ret;
>
> - rport->port_id = USB2PHY_PORT_HOST;
> rport->port_cfg = &rphy->phy_cfg->port_cfgs[USB2PHY_PORT_HOST];
> + if (!rport->port_cfg) {
> + dev_err(rphy->dev, "no host port-config provided.\n");
> + return -EINVAL;
> + }
This would never be NULL. At issue is that you don't assign port_cfg
if the port is _not_ a host port.
Guenter
>
> + rport->port_id = USB2PHY_PORT_HOST;
>
>> Or from what I've seen, handling it as similar to the host-port should
>> work
>> initially as well most likely, supplying the additional otg-parts later
>> on.
>
>
> @Guenter, just as Heiko said, the otg-parts is not ready now, it will be
> supplied later.
>
>
> BR.
> Frank
>
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH v5 2/2] phy: rockchip-inno-usb2: add a new driver for Rockchip usb2phy
2016-06-15 10:58 ` Frank Wang
@ 2016-06-15 18:49 ` Heiko Stübner
0 siblings, 0 replies; 25+ messages in thread
From: Heiko Stübner @ 2016-06-15 18:49 UTC (permalink / raw)
To: Frank Wang
Cc: dianders, linux, groeck, jwerner, kishon, robh+dt, pawel.moll,
mark.rutland, ijc+devicetree, galak, linux-kernel, devicetree,
linux-usb, linux-rockchip, xzy.xu, kever.yang, huangtao,
william.wu
Hi Frank,
Am Mittwoch, 15. Juni 2016, 18:58:43 schrieb Frank Wang:
> On 2016/6/15 17:04, Heiko Stübner wrote:
> > Am Mittwoch, 15. Juni 2016, 11:23:26 schrieb Frank Wang:
> >> On 2016/6/14 21:27, Heiko Stübner wrote:
> >>> Am Montag, 13. Juni 2016, 10:10:10 schrieb Frank Wang:
> >>>> The newer SoCs (rk3366, rk3399) take a different usb-phy IP block
> >>>> than rk3288 and before, and most of phy-related registers are also
> >>>> different from the past, so a new phy driver is required necessarily.
> >>>>
> >>>> Signed-off-by: Frank Wang <frank.wang@rock-chips.com>
> >>>> ---
> >>>>
> >>>> Changes in v5:
> >>>> - Added 'reg' in the data block to match the different phy-blocks in
> >>>> dt.
> >>>>
> >>>> Changes in v4:
> >>>> - Removed some processes related to 'vbus_host-supply'.
> >>>>
> >>>> Changes in v3:
> >>>> - Resolved the mapping defect between fixed value in driver and the
> >>>>
> >>>> property in devicetree.
> >>>>
> >>>> - Optimized 480m output clock register function.
> >>>> - Code cleanup.
> >>>>
> >>>> Changes in v2:
> >>>> - Changed vbus_host operation from gpio to regulator in *_probe.
> >>>> - Improved the fault treatment relate to 480m clock register.
> >>>> - Cleaned up some meaningless codes in *_clk480m_disable.
> >>>> - made more clear the comment of *_sm_work.
> >>>>
> >>>> drivers/phy/Kconfig | 7 +
> >>>> drivers/phy/Makefile | 1 +
> >>>> drivers/phy/phy-rockchip-inno-usb2.c | 645
> >>>>
> >>>> ++++++++++++++++++++++++++++++++++ 3 files changed, 653 insertions(+)
> >>>>
> >>>> [...]
> >>>>
> >>>> +
> >>>> +static int rockchip_usb2phy_exit(struct phy *phy)
> >>>> +{
> >>>> + struct rockchip_usb2phy_port *rport = phy_get_drvdata(phy);
> >>>> +
> >>>>
> >>> if (!rport->port_cfg)
> >>>
> >>> return 0;
> >>>>
> >>>> + if (rport->port_id == USB2PHY_PORT_HOST)
> >>>> + cancel_delayed_work_sync(&rport->sm_work);
> >>>> +
> >>>
> >>> you will also need to resume the port here, if it is suspended at this
> >>> point, as phy_power_off gets called after phy_exit and would probably
> >>> produce clk enable/disable mismatches otherwise.
> >>
> >> Hmm, from my personal point of view, when canceling sm_work here, it may
> >> not cause the port goes to suspend, isn't it? besides, clk only prepared
> >> in *_usb2phy_resume(), and unprepared in *_usb2phy_suspend(), so if we
> >> resume port here, the prepare_count of clk will be increased again, I
> >> am afraid this is not correct, and am I wrong? would you like to tell me
> >> more details?
> >
> > usb2phy_resume gets called both initially through phy_power_on as well.
> > So it's on but through the first scheduled work call, might get suspended
> > when nothing is connected. (clk_enable and clk_disable will run).
> > If nothing is connected on unload phy_power_off will get called while the
> > clock actually is still disabled.
> >
> > So I think it's either resuming on exit, or at least making sure to do
> > nothing in that case in the phy_power_off callback of the driver.
>
> Well, I got what you mean. I saw phy_power_on() gets called twice at
> ehci/ohci driver probe time, one is at pdata->power_on(); another is at
> usb_add_hcd(), so after that, the power_count of phy increases to two.
> however, when ehci/ohci driver goes to suspend, there is only once to
> call phy_power_off(), and the power_count of phy decreases to 1, so our
> usb2phy_resume() could not be invoked :-) .
>
> If so, is it still need to care it?
You should never have to rely on oddities in other drivers :-)
> How about use suspended member of
> rockchip_usb2phy_port as a conditional to check in *_usb2phy_suspend(),
> if necessary.
That looks also sane and should work.
Heiko
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH v5 2/2] phy: rockchip-inno-usb2: add a new driver for Rockchip usb2phy
2016-06-15 15:47 ` Guenter Roeck
@ 2016-06-16 1:47 ` Frank Wang
2016-06-16 13:12 ` Guenter Roeck
0 siblings, 1 reply; 25+ messages in thread
From: Frank Wang @ 2016-06-16 1:47 UTC (permalink / raw)
To: Guenter Roeck
Cc: Heiko Stübner, Douglas Anderson, Guenter Roeck,
Guenter Roeck, jwerner, kishon, robh+dt, pawel.moll,
mark.rutland, ijc+devicetree, Kumar Gala, linux-kernel,
devicetree, linux-usb, linux-rockchip, Ziyuan Xu, Kever Yang,
Tao Huang, william.wu, frank.wang
Hi Guenter & Heiko,
On 2016/6/15 23:47, Guenter Roeck wrote:
> On Tue, Jun 14, 2016 at 6:14 PM, Frank Wang <frank.wang@rock-chips.com> wrote:
>> Hi Heiko & Guenter,
>>
>>
>> On 2016/6/14 22:00, Heiko Stübner wrote:
>>> Am Dienstag, 14. Juni 2016, 06:50:31 schrieb Guenter Roeck:
>>>> On Tue, Jun 14, 2016 at 6:27 AM, Heiko Stübner <heiko@sntech.de> wrote:
>>>>> Am Montag, 13. Juni 2016, 10:10:10 schrieb Frank Wang:
>>>>>> The newer SoCs (rk3366, rk3399) take a different usb-phy IP block
>>>>>> than rk3288 and before, and most of phy-related registers are also
>>>>>> different from the past, so a new phy driver is required necessarily.
>>>>>>
>>>>>> Signed-off-by: Frank Wang <frank.wang@rock-chips.com>
>>>>>> ---
>>> [...]
>>>
>>>>>> +static int rockchip_usb2phy_init(struct phy *phy)
>>>>>> +{
>>>>>> + struct rockchip_usb2phy_port *rport = phy_get_drvdata(phy);
>>>>>> + struct rockchip_usb2phy *rphy = dev_get_drvdata(phy->dev.parent);
>>>>>> + int ret;
>>>>>> +
>>>>>>
>>>>> if (!rport->port_cfg)
>>>>> return 0;
>>>>>
>>>>> Otherwise the currently empty otg-port will cause null-pointer
>>>>> dereferences
>>>>> when it gets assigned in the devicetree already.
>>>> Not really, at least not here - that port should not have port_id set
>>>> to USB2PHY_PORT_HOST.
>>>>
>>>> Does it even make sense to instantiate the otg port ? Is it going to
>>>> do anything without port configuration ?
>>> Ok, that would be the other option - not creating the phy in the driver.
>>
>> Well, I will put this conditional inside *_host_port_init(), if it is an
>> empty, the phy-device should not be created.
>> Something like the following:
>>
>> --- a/drivers/phy/phy-rockchip-inno-usb2.c
>> +++ b/drivers/phy/phy-rockchip-inno-usb2.c
>> @@ -483,9 +483,13 @@ static int rockchip_usb2phy_host_port_init(struct
>> rockchip_usb2phy *rphy,
>> {
>> int ret;
>>
>> - rport->port_id = USB2PHY_PORT_HOST;
>> rport->port_cfg = &rphy->phy_cfg->port_cfgs[USB2PHY_PORT_HOST];
>> + if (!rport->port_cfg) {
>> + dev_err(rphy->dev, "no host port-config provided.\n");
>> + return -EINVAL;
>> + }
> This would never be NULL. At issue is that you don't assign port_cfg
> if the port is _not_ a host port.
Sorry, I made a mistake. How about something like the following:
@@ -574,6 +579,15 @@ static int rockchip_usb2phy_probe(struct
platform_device *pdev)
struct rockchip_usb2phy_port *rport = &rphy->ports[index];
struct phy *phy;
+ /*
+ * This driver aim to support both otg-port and host-port,
+ * but unfortunately, the otg part is not ready in current,
+ * so this comments and below codes are interim, which
should
+ * be removed after otg-port is supplied soon.
+ */
+ if (of_node_cmp(child_np->name, "host-port"))
+ goto next_child;
+
phy = devm_phy_create(dev, child_np,
&rockchip_usb2phy_ops);
if (IS_ERR(phy)) {
dev_err(dev, "failed to create phy\n");
@@ -582,17 +596,13 @@ static int rockchip_usb2phy_probe(struct
platform_device *pdev)
}
rport->phy = phy;
-
- /* initialize otg/host port separately */
- if (!of_node_cmp(child_np->name, "host-port")) {
- ret = rockchip_usb2phy_host_port_init(rphy, rport,
- child_np);
- if (ret)
- goto put_child;
- }
-
phy_set_drvdata(rport->phy, rport);
+ ret = rockchip_usb2phy_host_port_init(rphy, rport,
child_np);
+ if (ret)
+ goto put_child;
+
+next_child:
/* to prevent out of boundary */
if (++index >= rphy->phy_cfg->num_ports)
break;
BR.
Frank
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH v5 2/2] phy: rockchip-inno-usb2: add a new driver for Rockchip usb2phy
2016-06-16 1:47 ` Frank Wang
@ 2016-06-16 13:12 ` Guenter Roeck
2016-06-17 0:57 ` Frank Wang
0 siblings, 1 reply; 25+ messages in thread
From: Guenter Roeck @ 2016-06-16 13:12 UTC (permalink / raw)
To: Frank Wang, Guenter Roeck
Cc: Heiko Stübner, Douglas Anderson, Guenter Roeck, jwerner,
kishon, robh+dt, pawel.moll, mark.rutland, ijc+devicetree,
Kumar Gala, linux-kernel, devicetree, linux-usb, linux-rockchip,
Ziyuan Xu, Kever Yang, Tao Huang, william.wu
On 06/15/2016 06:47 PM, Frank Wang wrote:
> Hi Guenter & Heiko,
>
> On 2016/6/15 23:47, Guenter Roeck wrote:
>> On Tue, Jun 14, 2016 at 6:14 PM, Frank Wang <frank.wang@rock-chips.com> wrote:
>>> Hi Heiko & Guenter,
>>>
>>>
>>> On 2016/6/14 22:00, Heiko Stübner wrote:
>>>> Am Dienstag, 14. Juni 2016, 06:50:31 schrieb Guenter Roeck:
>>>>> On Tue, Jun 14, 2016 at 6:27 AM, Heiko Stübner <heiko@sntech.de> wrote:
>>>>>> Am Montag, 13. Juni 2016, 10:10:10 schrieb Frank Wang:
>>>>>>> The newer SoCs (rk3366, rk3399) take a different usb-phy IP block
>>>>>>> than rk3288 and before, and most of phy-related registers are also
>>>>>>> different from the past, so a new phy driver is required necessarily.
>>>>>>>
>>>>>>> Signed-off-by: Frank Wang <frank.wang@rock-chips.com>
>>>>>>> ---
>>>> [...]
>>>>
>>>>>>> +static int rockchip_usb2phy_init(struct phy *phy)
>>>>>>> +{
>>>>>>> + struct rockchip_usb2phy_port *rport = phy_get_drvdata(phy);
>>>>>>> + struct rockchip_usb2phy *rphy = dev_get_drvdata(phy->dev.parent);
>>>>>>> + int ret;
>>>>>>> +
>>>>>>>
>>>>>> if (!rport->port_cfg)
>>>>>> return 0;
>>>>>>
>>>>>> Otherwise the currently empty otg-port will cause null-pointer
>>>>>> dereferences
>>>>>> when it gets assigned in the devicetree already.
>>>>> Not really, at least not here - that port should not have port_id set
>>>>> to USB2PHY_PORT_HOST.
>>>>>
>>>>> Does it even make sense to instantiate the otg port ? Is it going to
>>>>> do anything without port configuration ?
>>>> Ok, that would be the other option - not creating the phy in the driver.
>>>
>>> Well, I will put this conditional inside *_host_port_init(), if it is an
>>> empty, the phy-device should not be created.
>>> Something like the following:
>>>
>>> --- a/drivers/phy/phy-rockchip-inno-usb2.c
>>> +++ b/drivers/phy/phy-rockchip-inno-usb2.c
>>> @@ -483,9 +483,13 @@ static int rockchip_usb2phy_host_port_init(struct
>>> rockchip_usb2phy *rphy,
>>> {
>>> int ret;
>>>
>>> - rport->port_id = USB2PHY_PORT_HOST;
>>> rport->port_cfg = &rphy->phy_cfg->port_cfgs[USB2PHY_PORT_HOST];
>>> + if (!rport->port_cfg) {
>>> + dev_err(rphy->dev, "no host port-config provided.\n");
>>> + return -EINVAL;
>>> + }
>> This would never be NULL. At issue is that you don't assign port_cfg
>> if the port is _not_ a host port.
>
> Sorry, I made a mistake. How about something like the following:
>
Yes, that should work. Just keep in mind that there could always be
a port named "something-port", so you'll always need some kind of check
(and possibly return an error if a port with a wrong name is provided).
Thanks,
Guenter
> @@ -574,6 +579,15 @@ static int rockchip_usb2phy_probe(struct platform_device *pdev)
> struct rockchip_usb2phy_port *rport = &rphy->ports[index];
> struct phy *phy;
>
> + /*
> + * This driver aim to support both otg-port and host-port,
> + * but unfortunately, the otg part is not ready in current,
> + * so this comments and below codes are interim, which should
> + * be removed after otg-port is supplied soon.
> + */
> + if (of_node_cmp(child_np->name, "host-port"))
> + goto next_child;
> +
> phy = devm_phy_create(dev, child_np, &rockchip_usb2phy_ops);
> if (IS_ERR(phy)) {
> dev_err(dev, "failed to create phy\n");
> @@ -582,17 +596,13 @@ static int rockchip_usb2phy_probe(struct platform_device *pdev)
> }
>
> rport->phy = phy;
> -
> - /* initialize otg/host port separately */
> - if (!of_node_cmp(child_np->name, "host-port")) {
> - ret = rockchip_usb2phy_host_port_init(rphy, rport,
> - child_np);
> - if (ret)
> - goto put_child;
> - }
> -
> phy_set_drvdata(rport->phy, rport);
>
> + ret = rockchip_usb2phy_host_port_init(rphy, rport, child_np);
> + if (ret)
> + goto put_child;
> +
> +next_child:
> /* to prevent out of boundary */
> if (++index >= rphy->phy_cfg->num_ports)
> break;
>
>
> BR.
> Frank
>
>
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH v5 2/2] phy: rockchip-inno-usb2: add a new driver for Rockchip usb2phy
2016-06-16 13:12 ` Guenter Roeck
@ 2016-06-17 0:57 ` Frank Wang
0 siblings, 0 replies; 25+ messages in thread
From: Frank Wang @ 2016-06-17 0:57 UTC (permalink / raw)
To: Guenter Roeck
Cc: Heiko Stübner, Douglas Anderson, Guenter Roeck, jwerner,
kishon, robh+dt, pawel.moll, mark.rutland, ijc+devicetree,
Kumar Gala, linux-kernel, devicetree, linux-usb, linux-rockchip,
Ziyuan Xu, Kever Yang, Tao Huang, william.wu, frank.wang
Hi Guenter,
On 2016/6/16 21:12, Guenter Roeck wrote:
> On 06/15/2016 06:47 PM, Frank Wang wrote:
>> Hi Guenter & Heiko,
>>
>> On 2016/6/15 23:47, Guenter Roeck wrote:
>>> On Tue, Jun 14, 2016 at 6:14 PM, Frank Wang
>>> <frank.wang@rock-chips.com> wrote:
>>>> Hi Heiko & Guenter,
>>>>
>>>>
>>>> On 2016/6/14 22:00, Heiko Stübner wrote:
>>>>> Am Dienstag, 14. Juni 2016, 06:50:31 schrieb Guenter Roeck:
>>>>>> On Tue, Jun 14, 2016 at 6:27 AM, Heiko Stübner <heiko@sntech.de>
>>>>>> wrote:
>>>>>>> Am Montag, 13. Juni 2016, 10:10:10 schrieb Frank Wang:
>>>>>>>> The newer SoCs (rk3366, rk3399) take a different usb-phy IP block
>>>>>>>> than rk3288 and before, and most of phy-related registers are also
>>>>>>>> different from the past, so a new phy driver is required
>>>>>>>> necessarily.
>>>>>>>>
>>>>>>>> Signed-off-by: Frank Wang <frank.wang@rock-chips.com>
>>>>>>>> ---
>>>>> [...]
>>>>>
>>>>>>>> +static int rockchip_usb2phy_init(struct phy *phy)
>>>>>>>> +{
>>>>>>>> + struct rockchip_usb2phy_port *rport = phy_get_drvdata(phy);
>>>>>>>> + struct rockchip_usb2phy *rphy =
>>>>>>>> dev_get_drvdata(phy->dev.parent);
>>>>>>>> + int ret;
>>>>>>>> +
>>>>>>>>
>>>>>>> if (!rport->port_cfg)
>>>>>>> return 0;
>>>>>>>
>>>>>>> Otherwise the currently empty otg-port will cause null-pointer
>>>>>>> dereferences
>>>>>>> when it gets assigned in the devicetree already.
>>>>>> Not really, at least not here - that port should not have port_id
>>>>>> set
>>>>>> to USB2PHY_PORT_HOST.
>>>>>>
>>>>>> Does it even make sense to instantiate the otg port ? Is it going to
>>>>>> do anything without port configuration ?
>>>>> Ok, that would be the other option - not creating the phy in the
>>>>> driver.
>>>>
>>>> Well, I will put this conditional inside *_host_port_init(), if it
>>>> is an
>>>> empty, the phy-device should not be created.
>>>> Something like the following:
>>>>
>>>> --- a/drivers/phy/phy-rockchip-inno-usb2.c
>>>> +++ b/drivers/phy/phy-rockchip-inno-usb2.c
>>>> @@ -483,9 +483,13 @@ static int rockchip_usb2phy_host_port_init(struct
>>>> rockchip_usb2phy *rphy,
>>>> {
>>>> int ret;
>>>>
>>>> - rport->port_id = USB2PHY_PORT_HOST;
>>>> rport->port_cfg =
>>>> &rphy->phy_cfg->port_cfgs[USB2PHY_PORT_HOST];
>>>> + if (!rport->port_cfg) {
>>>> + dev_err(rphy->dev, "no host port-config provided.\n");
>>>> + return -EINVAL;
>>>> + }
>>> This would never be NULL. At issue is that you don't assign port_cfg
>>> if the port is _not_ a host port.
>>
>> Sorry, I made a mistake. How about something like the following:
>>
> Yes, that should work. Just keep in mind that there could always be
> a port named "something-port", so you'll always need some kind of check
> (and possibly return an error if a port with a wrong name is provided).
>
>
OK, thanks for your reminding, I am going to send out it with a new
version later.
BR.
Frank
>
>> @@ -574,6 +579,15 @@ static int rockchip_usb2phy_probe(struct
>> platform_device *pdev)
>> struct rockchip_usb2phy_port *rport =
>> &rphy->ports[index];
>> struct phy *phy;
>>
>> + /*
>> + * This driver aim to support both otg-port and
>> host-port,
>> + * but unfortunately, the otg part is not ready in
>> current,
>> + * so this comments and below codes are interim,
>> which should
>> + * be removed after otg-port is supplied soon.
>> + */
>> + if (of_node_cmp(child_np->name, "host-port"))
>> + goto next_child;
>> +
>> phy = devm_phy_create(dev, child_np,
>> &rockchip_usb2phy_ops);
>> if (IS_ERR(phy)) {
>> dev_err(dev, "failed to create phy\n");
>> @@ -582,17 +596,13 @@ static int rockchip_usb2phy_probe(struct
>> platform_device *pdev)
>> }
>>
>> rport->phy = phy;
>> -
>> - /* initialize otg/host port separately */
>> - if (!of_node_cmp(child_np->name, "host-port")) {
>> - ret = rockchip_usb2phy_host_port_init(rphy,
>> rport,
>> - child_np);
>> - if (ret)
>> - goto put_child;
>> - }
>> -
>> phy_set_drvdata(rport->phy, rport);
>>
>> + ret = rockchip_usb2phy_host_port_init(rphy, rport,
>> child_np);
>> + if (ret)
>> + goto put_child;
>> +
>> +next_child:
>> /* to prevent out of boundary */
>> if (++index >= rphy->phy_cfg->num_ports)
>> break;
>>
>>
>>
>
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH v5 2/2] phy: rockchip-inno-usb2: add a new driver for Rockchip usb2phy
2016-06-13 2:10 ` [PATCH v5 2/2] phy: rockchip-inno-usb2: add a new driver for Rockchip usb2phy Frank Wang
2016-06-14 13:27 ` Heiko Stübner
2016-06-14 14:52 ` Guenter Roeck
@ 2016-06-17 11:54 ` Kishon Vijay Abraham I
2016-06-17 16:46 ` Heiko Stübner
2 siblings, 1 reply; 25+ messages in thread
From: Kishon Vijay Abraham I @ 2016-06-17 11:54 UTC (permalink / raw)
To: Frank Wang, heiko, dianders, linux, groeck, jwerner, robh+dt,
pawel.moll, mark.rutland, ijc+devicetree, galak
Cc: linux-kernel, devicetree, linux-usb, linux-rockchip, xzy.xu,
kever.yang, huangtao, william.wu
Hi,
On Monday 13 June 2016 07:40 AM, Frank Wang wrote:
> The newer SoCs (rk3366, rk3399) take a different usb-phy IP block
> than rk3288 and before, and most of phy-related registers are also
> different from the past, so a new phy driver is required necessarily.
>
> Signed-off-by: Frank Wang <frank.wang@rock-chips.com>
> ---
>
> Changes in v5:
> - Added 'reg' in the data block to match the different phy-blocks in dt.
>
> Changes in v4:
> - Removed some processes related to 'vbus_host-supply'.
>
> Changes in v3:
> - Resolved the mapping defect between fixed value in driver and the property
> in devicetree.
> - Optimized 480m output clock register function.
> - Code cleanup.
>
> Changes in v2:
> - Changed vbus_host operation from gpio to regulator in *_probe.
> - Improved the fault treatment relate to 480m clock register.
> - Cleaned up some meaningless codes in *_clk480m_disable.
> - made more clear the comment of *_sm_work.
>
> drivers/phy/Kconfig | 7 +
> drivers/phy/Makefile | 1 +
> drivers/phy/phy-rockchip-inno-usb2.c | 645 ++++++++++++++++++++++++++++++++++
> 3 files changed, 653 insertions(+)
> create mode 100644 drivers/phy/phy-rockchip-inno-usb2.c
>
> diff --git a/drivers/phy/Kconfig b/drivers/phy/Kconfig
> index b869b98..29ef15c 100644
> --- a/drivers/phy/Kconfig
> +++ b/drivers/phy/Kconfig
> @@ -347,6 +347,13 @@ config PHY_ROCKCHIP_USB
> help
> Enable this to support the Rockchip USB 2.0 PHY.
>
> +config PHY_ROCKCHIP_INNO_USB2
> + tristate "Rockchip INNO USB2PHY Driver"
> + depends on ARCH_ROCKCHIP && OF
depends on COMMON_CLK, since this is also a clock provider.
> + select GENERIC_PHY
> + help
> + Support for Rockchip USB2.0 PHY with Innosilicon IP block.
> +
> config PHY_ROCKCHIP_EMMC
> tristate "Rockchip EMMC PHY Driver"
> depends on ARCH_ROCKCHIP && OF
> diff --git a/drivers/phy/Makefile b/drivers/phy/Makefile
> index 9c3e73c..4963fbc 100644
> --- a/drivers/phy/Makefile
> +++ b/drivers/phy/Makefile
> @@ -38,6 +38,7 @@ phy-exynos-usb2-$(CONFIG_PHY_S5PV210_USB2) += phy-s5pv210-usb2.o
> obj-$(CONFIG_PHY_EXYNOS5_USBDRD) += phy-exynos5-usbdrd.o
> obj-$(CONFIG_PHY_QCOM_APQ8064_SATA) += phy-qcom-apq8064-sata.o
> obj-$(CONFIG_PHY_ROCKCHIP_USB) += phy-rockchip-usb.o
> +obj-$(CONFIG_PHY_ROCKCHIP_INNO_USB2) += phy-rockchip-inno-usb2.o
> obj-$(CONFIG_PHY_ROCKCHIP_EMMC) += phy-rockchip-emmc.o
> obj-$(CONFIG_PHY_ROCKCHIP_DP) += phy-rockchip-dp.o
> obj-$(CONFIG_PHY_QCOM_IPQ806X_SATA) += phy-qcom-ipq806x-sata.o
> diff --git a/drivers/phy/phy-rockchip-inno-usb2.c b/drivers/phy/phy-rockchip-inno-usb2.c
> new file mode 100644
> index 0000000..fc599c7
> --- /dev/null
> +++ b/drivers/phy/phy-rockchip-inno-usb2.c
> @@ -0,0 +1,645 @@
> +/*
> + * Rockchip USB2.0 PHY with Innosilicon IP block driver
> + *
> + * Copyright (C) 2016 Fuzhou Rockchip Electronics Co., Ltd
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License as published by
> + * the Free Software Foundation; either version 2 of the License, or
> + * (at your option) any later version.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> + * GNU General Public License for more details.
> + */
> +
> +#include <linux/clk.h>
> +#include <linux/clk-provider.h>
> +#include <linux/delay.h>
> +#include <linux/interrupt.h>
> +#include <linux/io.h>
> +#include <linux/gpio/consumer.h>
> +#include <linux/jiffies.h>
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/mutex.h>
> +#include <linux/of.h>
> +#include <linux/of_address.h>
> +#include <linux/of_irq.h>
> +#include <linux/of_platform.h>
> +#include <linux/phy/phy.h>
> +#include <linux/platform_device.h>
> +#include <linux/regmap.h>
> +#include <linux/mfd/syscon.h>
> +
> +#define BIT_WRITEABLE_SHIFT 16
> +#define SCHEDULE_DELAY (60 * HZ)
> +
> +enum rockchip_usb2phy_port_id {
> + USB2PHY_PORT_OTG,
> + USB2PHY_PORT_HOST,
> + USB2PHY_NUM_PORTS,
> +};
> +
> +enum rockchip_usb2phy_host_state {
> + PHY_STATE_HS_ONLINE = 0,
> + PHY_STATE_DISCONNECT = 1,
> + PHY_STATE_HS_CONNECT = 2,
> + PHY_STATE_FS_CONNECT = 4,
> +};
> +
> +struct usb2phy_reg {
> + unsigned int offset;
> + unsigned int bitend;
> + unsigned int bitstart;
> + unsigned int disable;
> + unsigned int enable;
> +};
> +
> +/**
> + * struct rockchip_usb2phy_port_cfg: usb-phy port configuration.
> + * @phy_sus: phy suspend register.
> + * @ls_det_en: linestate detection enable register.
> + * @ls_det_st: linestate detection state register.
> + * @ls_det_clr: linestate detection clear register.
> + * @utmi_ls: utmi linestate state register.
> + * @utmi_hstdet: utmi host disconnect register.
> + */
> +struct rockchip_usb2phy_port_cfg {
> + struct usb2phy_reg phy_sus;
> + struct usb2phy_reg ls_det_en;
> + struct usb2phy_reg ls_det_st;
> + struct usb2phy_reg ls_det_clr;
> + struct usb2phy_reg utmi_ls;
> + struct usb2phy_reg utmi_hstdet;
> +};
> +
> +/**
> + * struct rockchip_usb2phy_cfg: usb-phy configuration.
> + * @reg: the address offset of grf for usb-phy config.
> + * @num_ports: specify how many ports that the phy has.
> + * @clkout_ctl: keep on/turn off output clk of phy.
> + */
> +struct rockchip_usb2phy_cfg {
> + unsigned int reg;
> + unsigned int num_ports;
> + struct usb2phy_reg clkout_ctl;
> + const struct rockchip_usb2phy_port_cfg port_cfgs[USB2PHY_NUM_PORTS];
> +};
> +
> +/**
> + * struct rockchip_usb2phy_port: usb-phy port data.
> + * @port_id: flag for otg port or host port.
> + * @suspended: phy suspended flag.
> + * @ls_irq: IRQ number assigned for linestate detection.
> + * @mutex: for register updating in sm_work.
> + * @sm_work: OTG state machine work.
> + * @phy_cfg: port register configuration, assigned by driver data.
> + */
> +struct rockchip_usb2phy_port {
> + struct phy *phy;
> + unsigned int port_id;
> + bool suspended;
> + int ls_irq;
> + struct mutex mutex;
> + struct delayed_work sm_work;
> + const struct rockchip_usb2phy_port_cfg *port_cfg;
> +};
> +
> +/**
> + * struct rockchip_usb2phy: usb2.0 phy driver data.
> + * @grf: General Register Files regmap.
> + * @clk: clock struct of phy input clk.
> + * @clk480m: clock struct of phy output clk.
> + * @clk_hw: clock struct of phy output clk management.
> + * @phy_cfg: phy register configuration, assigned by driver data.
> + * @ports: phy port instance.
> + */
> +struct rockchip_usb2phy {
> + struct device *dev;
> + struct regmap *grf;
> + struct clk *clk;
> + struct clk *clk480m;
> + struct clk_hw clk480m_hw;
> + const struct rockchip_usb2phy_cfg *phy_cfg;
> + struct rockchip_usb2phy_port ports[USB2PHY_NUM_PORTS];
> +};
> +
> +static inline int property_enable(struct rockchip_usb2phy *rphy,
> + const struct usb2phy_reg *reg, bool en)
> +{
> + unsigned int val, mask, tmp;
> +
> + tmp = en ? reg->enable : reg->disable;
> + mask = GENMASK(reg->bitend, reg->bitstart);
> + val = (tmp << reg->bitstart) | (mask << BIT_WRITEABLE_SHIFT);
> +
> + return regmap_write(rphy->grf, reg->offset, val);
> +}
> +
> +static inline bool property_enabled(struct rockchip_usb2phy *rphy,
> + const struct usb2phy_reg *reg)
> +{
> + int ret;
> + unsigned int tmp, orig;
> + unsigned int mask = GENMASK(reg->bitend, reg->bitstart);
> +
> + ret = regmap_read(rphy->grf, reg->offset, &orig);
> + if (ret)
> + return false;
> +
> + tmp = (orig & mask) >> reg->bitstart;
> + return tmp == reg->enable;
> +}
> +
> +static int rockchip_usb2phy_clk480m_enable(struct clk_hw *hw)
> +{
> + struct rockchip_usb2phy *rphy =
> + container_of(hw, struct rockchip_usb2phy, clk480m_hw);
> + int ret;
> +
> + /* turn on 480m clk output if it is off */
> + if (!property_enabled(rphy, &rphy->phy_cfg->clkout_ctl)) {
> + ret = property_enable(rphy, &rphy->phy_cfg->clkout_ctl, true);
> + if (ret)
> + return ret;
> +
> + /* waitting for the clk become stable */
> + mdelay(1);
> + }
> +
> + return 0;
> +}
> +
> +static void rockchip_usb2phy_clk480m_disable(struct clk_hw *hw)
> +{
> + struct rockchip_usb2phy *rphy =
> + container_of(hw, struct rockchip_usb2phy, clk480m_hw);
> +
> + /* turn off 480m clk output */
> + property_enable(rphy, &rphy->phy_cfg->clkout_ctl, false);
> +}
> +
> +static int rockchip_usb2phy_clk480m_enabled(struct clk_hw *hw)
> +{
> + struct rockchip_usb2phy *rphy =
> + container_of(hw, struct rockchip_usb2phy, clk480m_hw);
> +
> + return property_enabled(rphy, &rphy->phy_cfg->clkout_ctl);
> +}
> +
> +static unsigned long
> +rockchip_usb2phy_clk480m_recalc_rate(struct clk_hw *hw,
> + unsigned long parent_rate)
> +{
> + return 480000000;
> +}
> +
> +static const struct clk_ops rockchip_usb2phy_clkout_ops = {
> + .enable = rockchip_usb2phy_clk480m_enable,
> + .disable = rockchip_usb2phy_clk480m_disable,
> + .is_enabled = rockchip_usb2phy_clk480m_enabled,
> + .recalc_rate = rockchip_usb2phy_clk480m_recalc_rate,
> +};
> +
> +static void rockchip_usb2phy_clk480m_unregister(void *data)
> +{
> + struct rockchip_usb2phy *rphy = data;
> +
> + of_clk_del_provider(rphy->dev->of_node);
> + clk_unregister(rphy->clk480m);
> +
> + if (rphy->clk)
> + clk_put(rphy->clk);
> +}
> +
> +static int
> +rockchip_usb2phy_clk480m_register(struct rockchip_usb2phy *rphy)
> +{
> + struct device_node *node = rphy->dev->of_node;
> + struct clk_init_data init;
> + const char *clk_name;
> + int ret;
> +
> + init.flags = 0;
> + init.name = "clk_usbphy_480m";
> + init.ops = &rockchip_usb2phy_clkout_ops;
> +
> + /* optional override of the clockname */
> + of_property_read_string(node, "clock-output-names", &init.name);
> +
> + rphy->clk = of_clk_get_by_name(node, "phyclk");
> + if (IS_ERR(rphy->clk)) {
> + rphy->clk = NULL;
> + init.parent_names = NULL;
> + init.num_parents = 0;
> + } else {
> + clk_name = __clk_get_name(rphy->clk);
> + init.parent_names = &clk_name;
> + init.num_parents = 1;
> + }
> +
> + rphy->clk480m_hw.init = &init;
> +
> + /* register the clock */
> + rphy->clk480m = clk_register(rphy->dev, &rphy->clk480m_hw);
> + if (IS_ERR(rphy->clk480m)) {
> + ret = PTR_ERR(rphy->clk480m);
> + goto err_register;
> + }
> +
> + ret = of_clk_add_provider(node, of_clk_src_simple_get, rphy->clk480m);
> + if (ret < 0)
> + goto err_clk_provider;
> +
> + ret = devm_add_action(rphy->dev, rockchip_usb2phy_clk480m_unregister,
> + rphy);
> + if (ret < 0)
> + goto err_unreg_action;
> +
> + return 0;
> +
> +err_unreg_action:
> + of_clk_del_provider(node);
> +err_clk_provider:
> + clk_unregister(rphy->clk480m);
> +err_register:
> + if (rphy->clk)
> + clk_put(rphy->clk);
> + return ret;
> +}
I'm seeing lot of similarities specifically w.r.t to clock handling part in
drivers/phy/phy-rockchip-usb.c. Why not just re-use that driver?
> +
> +static int rockchip_usb2phy_init(struct phy *phy)
> +{
> + struct rockchip_usb2phy_port *rport = phy_get_drvdata(phy);
> + struct rockchip_usb2phy *rphy = dev_get_drvdata(phy->dev.parent);
> + int ret;
> +
> + if (rport->port_id == USB2PHY_PORT_HOST) {
> + /* clear linestate and enable linestate detect irq */
> + mutex_lock(&rport->mutex);
> +
> + ret = property_enable(rphy, &rport->port_cfg->ls_det_clr, true);
> + if (ret) {
> + mutex_unlock(&rport->mutex);
> + return ret;
> + }
> +
> + ret = property_enable(rphy, &rport->port_cfg->ls_det_en, true);
> + if (ret) {
> + mutex_unlock(&rport->mutex);
> + return ret;
> + }
> +
> + mutex_unlock(&rport->mutex);
> + schedule_delayed_work(&rport->sm_work, SCHEDULE_DELAY);
> + }
> +
> + return 0;
> +}
> +
> +static int rockchip_usb2phy_resume(struct phy *phy)
> +{
> + struct rockchip_usb2phy_port *rport = phy_get_drvdata(phy);
> + struct rockchip_usb2phy *rphy = dev_get_drvdata(phy->dev.parent);
> + int ret;
> +
> + dev_dbg(&rport->phy->dev, "port resume\n");
> +
> + ret = clk_prepare_enable(rphy->clk480m);
> + if (ret)
> + return ret;
> +
> + ret = property_enable(rphy, &rport->port_cfg->phy_sus, false);
> + if (ret)
> + return ret;
> +
> + rport->suspended = false;
> + return 0;
> +}
> +
> +static int rockchip_usb2phy_suspend(struct phy *phy)
> +{
> + struct rockchip_usb2phy_port *rport = phy_get_drvdata(phy);
> + struct rockchip_usb2phy *rphy = dev_get_drvdata(phy->dev.parent);
> + int ret;
> +
> + dev_dbg(&rport->phy->dev, "port suspend\n");
> +
> + ret = property_enable(rphy, &rport->port_cfg->phy_sus, true);
> + if (ret)
> + return ret;
> +
> + rport->suspended = true;
> + clk_disable_unprepare(rphy->clk480m);
> + return 0;
> +}
> +
> +static int rockchip_usb2phy_exit(struct phy *phy)
> +{
> + struct rockchip_usb2phy_port *rport = phy_get_drvdata(phy);
> +
> + if (rport->port_id == USB2PHY_PORT_HOST)
> + cancel_delayed_work_sync(&rport->sm_work);
> +
> + return 0;
> +}
> +
> +static const struct phy_ops rockchip_usb2phy_ops = {
> + .init = rockchip_usb2phy_init,
> + .exit = rockchip_usb2phy_exit,
> + .power_on = rockchip_usb2phy_resume,
> + .power_off = rockchip_usb2phy_suspend,
> + .owner = THIS_MODULE,
> +};
> +
> +/*
> + * The function manage host-phy port state and suspend/resume phy port
> + * to save power.
> + *
> + * we rely on utmi_linestate and utmi_hostdisconnect to identify whether
> + * FS/HS is disconnect or not. Besides, we do not need care it is FS
> + * disconnected or HS disconnected, actually, we just only need get the
> + * device is disconnected at last through rearm the delayed work,
> + * to suspend the phy port in _PHY_STATE_DISCONNECT_ case.
> + *
> + * NOTE: It may invoke *phy_suspend or *phy_resume which will invoke some
> + * clk related APIs, so do not invoke it from interrupt context directly.
> + */
> +static void rockchip_usb2phy_sm_work(struct work_struct *work)
> +{
> + struct rockchip_usb2phy_port *rport =
> + container_of(work, struct rockchip_usb2phy_port, sm_work.work);
> + struct rockchip_usb2phy *rphy = dev_get_drvdata(rport->phy->dev.parent);
> + unsigned int sh = rport->port_cfg->utmi_hstdet.bitend -
> + rport->port_cfg->utmi_hstdet.bitstart + 1;
> + unsigned int ul, uhd, state;
> + unsigned int ul_mask, uhd_mask;
> + int ret;
> +
> + mutex_lock(&rport->mutex);
> +
> + ret = regmap_read(rphy->grf, rport->port_cfg->utmi_ls.offset, &ul);
> + if (ret < 0)
> + goto next_schedule;
> +
> + ret = regmap_read(rphy->grf, rport->port_cfg->utmi_hstdet.offset,
> + &uhd);
> + if (ret < 0)
> + goto next_schedule;
> +
> + uhd_mask = GENMASK(rport->port_cfg->utmi_hstdet.bitend,
> + rport->port_cfg->utmi_hstdet.bitstart);
> + ul_mask = GENMASK(rport->port_cfg->utmi_ls.bitend,
> + rport->port_cfg->utmi_ls.bitstart);
> +
> + /* stitch on utmi_ls and utmi_hstdet as phy state */
> + state = ((uhd & uhd_mask) >> rport->port_cfg->utmi_hstdet.bitstart) |
> + (((ul & ul_mask) >> rport->port_cfg->utmi_ls.bitstart) << sh);
> +
> + switch (state) {
> + case PHY_STATE_HS_ONLINE:
> + dev_dbg(&rport->phy->dev, "HS online\n");
> + break;
> + case PHY_STATE_FS_CONNECT:
> + /*
> + * For FS device, the online state share with connect state
> + * from utmi_ls and utmi_hstdet register, so we distinguish
> + * them via suspended flag.
> + */
> + if (!rport->suspended) {
> + dev_dbg(&rport->phy->dev, "FS online\n");
> + break;
> + }
> + /* fall through */
> + case PHY_STATE_HS_CONNECT:
> + if (rport->suspended) {
> + dev_dbg(&rport->phy->dev, "HS/FS connected\n");
> + rockchip_usb2phy_resume(rport->phy);
> + rport->suspended = false;
> + }
> + break;
> + case PHY_STATE_DISCONNECT:
> + if (!rport->suspended) {
> + dev_dbg(&rport->phy->dev, "HS/FS disconnected\n");
> + rockchip_usb2phy_suspend(rport->phy);
> + rport->suspended = true;
> + }
> +
> + /*
> + * activate the linestate detection to get the next device
> + * plug-in irq.
> + */
> + property_enable(rphy, &rport->port_cfg->ls_det_clr, true);
> + property_enable(rphy, &rport->port_cfg->ls_det_en, true);
> +
> + /*
> + * we don't need to rearm the delayed work when the phy port
> + * is suspended.
> + */
> + mutex_unlock(&rport->mutex);
> + return;
> + default:
> + dev_dbg(&rport->phy->dev, "unknown phy state\n");
> + break;
> + }
> +
> +next_schedule:
> + mutex_unlock(&rport->mutex);
> + schedule_delayed_work(&rport->sm_work, SCHEDULE_DELAY);
Why are you scheduling the work again? Interrupt handlers can invoke this right?
> +}
> +
> +static irqreturn_t rockchip_usb2phy_linestate_irq(int irq, void *data)
> +{
> + struct rockchip_usb2phy_port *rport = data;
> + struct rockchip_usb2phy *rphy = dev_get_drvdata(rport->phy->dev.parent);
> +
> + if (!property_enabled(rphy, &rport->port_cfg->ls_det_st))
> + return IRQ_NONE;
> +
> + mutex_lock(&rport->mutex);
> +
> + /* disable linestate detect irq and clear its status */
> + property_enable(rphy, &rport->port_cfg->ls_det_en, false);
> + property_enable(rphy, &rport->port_cfg->ls_det_clr, true);
> +
> + mutex_unlock(&rport->mutex);
> +
> + /*
> + * In this case for host phy port, a new device is plugged in,
> + * meanwhile, if the phy port is suspended, we need rearm the work to
> + * resume it and mange its states; otherwise, we do nothing about that.
> + */
> + if (rport->suspended && rport->port_id == USB2PHY_PORT_HOST)
> + rockchip_usb2phy_sm_work(&rport->sm_work.work);
> +
> + return IRQ_HANDLED;
> +}
> +
> +static int rockchip_usb2phy_host_port_init(struct rockchip_usb2phy *rphy,
> + struct rockchip_usb2phy_port *rport,
> + struct device_node *child_np)
> +{
> + int ret;
> +
> + rport->port_id = USB2PHY_PORT_HOST;
> + rport->port_cfg = &rphy->phy_cfg->port_cfgs[USB2PHY_PORT_HOST];
> +
> + mutex_init(&rport->mutex);
> + INIT_DELAYED_WORK(&rport->sm_work, rockchip_usb2phy_sm_work);
> +
> + rport->ls_irq = of_irq_get_byname(child_np, "linestate");
> + if (rport->ls_irq < 0) {
> + dev_err(rphy->dev, "no linestate irq provided\n");
> + return rport->ls_irq;
> + }
> +
> + ret = devm_request_threaded_irq(rphy->dev, rport->ls_irq, NULL,
> + rockchip_usb2phy_linestate_irq,
> + IRQF_ONESHOT,
> + "rockchip_usb2phy", rport);
> + if (ret) {
> + dev_err(rphy->dev, "failed to request irq handle\n");
> + return ret;
> + }
> +
> + return 0;
> +}
> +
> +static int rockchip_usb2phy_probe(struct platform_device *pdev)
> +{
> + struct device *dev = &pdev->dev;
> + struct device_node *np = dev->of_node;
> + struct device_node *child_np;
> + struct phy_provider *provider;
> + struct rockchip_usb2phy *rphy;
> + const struct rockchip_usb2phy_cfg *phy_cfgs;
> + const struct of_device_id *match;
> + unsigned int reg;
> + int index, ret;
> +
> + rphy = devm_kzalloc(dev, sizeof(*rphy), GFP_KERNEL);
> + if (!rphy)
> + return -ENOMEM;
> +
> + match = of_match_device(dev->driver->of_match_table, dev);
> + if (!match || !match->data) {
> + dev_err(dev, "phy configs are not assigned!\n");
> + return -EINVAL;
> + }
> +
> + if (!dev->parent || !dev->parent->of_node)
> + return -EINVAL;
> +
> + rphy->grf = syscon_node_to_regmap(dev->parent->of_node);
> + if (IS_ERR(rphy->grf))
> + return PTR_ERR(rphy->grf);
> +
> + if (of_property_read_u32(np, "reg", ®)) {
> + dev_err(dev, "the reg property is not assigned in %s node\n",
> + np->name);
> + return -EINVAL;
> + }
> +
> + rphy->dev = dev;
> + phy_cfgs = match->data;
> + platform_set_drvdata(pdev, rphy);
> +
> + /* find out a proper config which can be matched with dt. */
> + index = 0;
> + while (phy_cfgs[index].reg) {
> + if (phy_cfgs[index].reg == reg) {
Why not pass these config values from dt? Moreover finding the config using
register offset is bound to break.
Thanks
Kishon
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH v5 2/2] phy: rockchip-inno-usb2: add a new driver for Rockchip usb2phy
2016-06-17 11:54 ` Kishon Vijay Abraham I
@ 2016-06-17 16:46 ` Heiko Stübner
2016-06-29 14:14 ` Kishon Vijay Abraham I
0 siblings, 1 reply; 25+ messages in thread
From: Heiko Stübner @ 2016-06-17 16:46 UTC (permalink / raw)
To: Kishon Vijay Abraham I
Cc: Frank Wang, dianders, linux, groeck, jwerner, robh+dt,
pawel.moll, mark.rutland, ijc+devicetree, galak, linux-kernel,
devicetree, linux-usb, linux-rockchip, xzy.xu, kever.yang,
huangtao, william.wu
Hi Kishon,
Am Freitag, 17. Juni 2016, 17:24:46 schrieb Kishon Vijay Abraham I:
> > + ret = of_clk_add_provider(node, of_clk_src_simple_get, rphy->clk480m);
> > + if (ret < 0)
> > + goto err_clk_provider;
> > +
> > + ret = devm_add_action(rphy->dev, rockchip_usb2phy_clk480m_unregister,
> > + rphy);
> > + if (ret < 0)
> > + goto err_unreg_action;
> > +
> > + return 0;
> > +
> > +err_unreg_action:
> > + of_clk_del_provider(node);
> > +err_clk_provider:
> > + clk_unregister(rphy->clk480m);
> > +err_register:
> > + if (rphy->clk)
> > + clk_put(rphy->clk);
> > + return ret;
> > +}
>
> I'm seeing lot of similarities specifically w.r.t to clock handling part in
> drivers/phy/phy-rockchip-usb.c. Why not just re-use that driver?
It's a completely different phy block (Designware vs. Innosilicon) and a lot
of stuff also is handled differently.
For one on the old block, each phy was somewhat independent and had for examle
its own clock-supply, while on this one there is only one for both ports of
the phy. Similarly with the clock getting fed back to the clock-controller
(one clock per port on the old block, now one clock for the whole phy).
Then as you can see, the handling for power-up and down is a bit different and
I guess one big block might be the still missing special otg handling, Frank
wrote about.
[...]
> > + /*
> > + * we don't need to rearm the delayed work when the phy port
> > + * is suspended.
> > + */
> > + mutex_unlock(&rport->mutex);
> > + return;
> > + default:
> > + dev_dbg(&rport->phy->dev, "unknown phy state\n");
> > + break;
> > + }
> > +
> > +next_schedule:
> > + mutex_unlock(&rport->mutex);
> > + schedule_delayed_work(&rport->sm_work, SCHEDULE_DELAY);
>
> Why are you scheduling the work again? Interrupt handlers can invoke this
> right?
Frank said, that the phy is only able to detect the plug-in event via
interrupts, not the removal, so once a plugged device is detected, this gets
rescheduled until the device gets removed.
[...]
> > + /* find out a proper config which can be matched with dt. */
> > + index = 0;
> > + while (phy_cfgs[index].reg) {
> > + if (phy_cfgs[index].reg == reg) {
>
> Why not pass these config values from dt? Moreover finding the config using
> register offset is bound to break.
As you have probably seen, this phy block is no stand-alone (mmio-)device, but
gets controlled through special register/bits in the so called "General
Register Files" syscon.
The values stored and accessed here, are the location and layout of those
control registers. Bits in those phy control registers at times move between
phy-versions in different socs (rk3036, rk3228, rk3366, rk3368, rk3399) and
some are even missing. So I don't really see a nice way to describe that in dt
without describing the register and offset of each of those 22 used bits
individually.
I'm also not sure where you expect it to break? The reg-offset is the offset
of the phy inside the GRF and the Designware-phy also already does something
similar to select some appropriate values.
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH v5 2/2] phy: rockchip-inno-usb2: add a new driver for Rockchip usb2phy
2016-06-17 16:46 ` Heiko Stübner
@ 2016-06-29 14:14 ` Kishon Vijay Abraham I
2016-06-29 14:27 ` Heiko Stuebner
0 siblings, 1 reply; 25+ messages in thread
From: Kishon Vijay Abraham I @ 2016-06-29 14:14 UTC (permalink / raw)
To: Heiko Stübner
Cc: Frank Wang, dianders, linux, groeck, jwerner, robh+dt,
pawel.moll, mark.rutland, ijc+devicetree, galak, linux-kernel,
devicetree, linux-usb, linux-rockchip, xzy.xu, kever.yang,
huangtao, william.wu
Hi,
On Friday 17 June 2016 10:16 PM, Heiko Stübner wrote:
> Hi Kishon,
>
> Am Freitag, 17. Juni 2016, 17:24:46 schrieb Kishon Vijay Abraham I:
>
>>> + ret = of_clk_add_provider(node, of_clk_src_simple_get, rphy->clk480m);
>>> + if (ret < 0)
>>> + goto err_clk_provider;
>>> +
>>> + ret = devm_add_action(rphy->dev, rockchip_usb2phy_clk480m_unregister,
>>> + rphy);
>>> + if (ret < 0)
>>> + goto err_unreg_action;
>>> +
>>> + return 0;
>>> +
>>> +err_unreg_action:
>>> + of_clk_del_provider(node);
>>> +err_clk_provider:
>>> + clk_unregister(rphy->clk480m);
>>> +err_register:
>>> + if (rphy->clk)
>>> + clk_put(rphy->clk);
>>> + return ret;
>>> +}
>>
>> I'm seeing lot of similarities specifically w.r.t to clock handling part in
>> drivers/phy/phy-rockchip-usb.c. Why not just re-use that driver?
>
> It's a completely different phy block (Designware vs. Innosilicon) and a lot
> of stuff also is handled differently.
>
> For one on the old block, each phy was somewhat independent and had for examle
> its own clock-supply, while on this one there is only one for both ports of
> the phy. Similarly with the clock getting fed back to the clock-controller
> (one clock per port on the old block, now one clock for the whole phy).
>
> Then as you can see, the handling for power-up and down is a bit different and
> I guess one big block might be the still missing special otg handling, Frank
> wrote about.
All right then.
>
>
> [...]
>
>>> + /*
>>> + * we don't need to rearm the delayed work when the phy port
>>> + * is suspended.
>>> + */
>>> + mutex_unlock(&rport->mutex);
>>> + return;
>>> + default:
>>> + dev_dbg(&rport->phy->dev, "unknown phy state\n");
>>> + break;
>>> + }
>>> +
>>> +next_schedule:
>>> + mutex_unlock(&rport->mutex);
>>> + schedule_delayed_work(&rport->sm_work, SCHEDULE_DELAY);
>>
>> Why are you scheduling the work again? Interrupt handlers can invoke this
>> right?
>
> Frank said, that the phy is only able to detect the plug-in event via
> interrupts, not the removal, so once a plugged device is detected, this gets
> rescheduled until the device gets removed.
okay.
>
> [...]
>
>>> + /* find out a proper config which can be matched with dt. */
>>> + index = 0;
>>> + while (phy_cfgs[index].reg) {
>>> + if (phy_cfgs[index].reg == reg) {
>>
>> Why not pass these config values from dt? Moreover finding the config using
>> register offset is bound to break.
>
> As you have probably seen, this phy block is no stand-alone (mmio-)device, but
> gets controlled through special register/bits in the so called "General
> Register Files" syscon.
>
> The values stored and accessed here, are the location and layout of those
> control registers. Bits in those phy control registers at times move between
> phy-versions in different socs (rk3036, rk3228, rk3366, rk3368, rk3399) and
> some are even missing. So I don't really see a nice way to describe that in dt
> without describing the register and offset of each of those 22 used bits
> individually.
>
>
> I'm also not sure where you expect it to break? The reg-offset is the offset
> of the phy inside the GRF and the Designware-phy also already does something
> similar to select some appropriate values.
I'm concerned the phy can be placed in the different reg-offset within GRF
(like in the next silicon revision) and this driver can't be used.
Thanks
Kishon
>
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH v5 2/2] phy: rockchip-inno-usb2: add a new driver for Rockchip usb2phy
2016-06-29 14:14 ` Kishon Vijay Abraham I
@ 2016-06-29 14:27 ` Heiko Stuebner
0 siblings, 0 replies; 25+ messages in thread
From: Heiko Stuebner @ 2016-06-29 14:27 UTC (permalink / raw)
To: Kishon Vijay Abraham I
Cc: Frank Wang, dianders, linux, groeck, jwerner, robh+dt,
pawel.moll, mark.rutland, ijc+devicetree, galak, linux-kernel,
devicetree, linux-usb, linux-rockchip, xzy.xu, kever.yang,
huangtao, william.wu
Hi Kishon,
Am Mittwoch, 29. Juni 2016, 19:44:52 schrieb Kishon Vijay Abraham I:
> On Friday 17 June 2016 10:16 PM, Heiko Stübner wrote:
> > Am Freitag, 17. Juni 2016, 17:24:46 schrieb Kishon Vijay Abraham I:
> > [...]
> >
> >>> + /* find out a proper config which can be matched with dt. */
> >>> + index = 0;
> >>> + while (phy_cfgs[index].reg) {
> >>> + if (phy_cfgs[index].reg == reg) {
> >>
> >> Why not pass these config values from dt? Moreover finding the config
> >> using register offset is bound to break.
> >
> > As you have probably seen, this phy block is no stand-alone
> > (mmio-)device, but gets controlled through special register/bits in the
> > so called "General Register Files" syscon.
> >
> > The values stored and accessed here, are the location and layout of
> > those
> > control registers. Bits in those phy control registers at times move
> > between phy-versions in different socs (rk3036, rk3228, rk3366, rk3368,
> > rk3399) and some are even missing. So I don't really see a nice way to
> > describe that in dt without describing the register and offset of each
> > of those 22 used bits individually.
> >
> >
> > I'm also not sure where you expect it to break? The reg-offset is the
> > offset of the phy inside the GRF and the Designware-phy also already
> > does something similar to select some appropriate values.
>
> I'm concerned the phy can be placed in the different reg-offset within GRF
> (like in the next silicon revision) and this driver can't be used.
That is something that has not happened with all Rockchip SoCs I have had in
my hands so far.
While the GRF is some sort-of wild-west area with settings for vastly
different blocks in there and always changes between different socs (rk3288
[4-core A17] <-> rk3366 <-> rk3368 [8-core A53] <-> rk3399 etc) there
haven't been such changes (different register offsets) between soc-revisions
of the same soc that I know off.
The GRF also includes such important things like the whole pinctrl handling,
so if there were such offset changes in the GRF a lot of drivers (and
maintainers) would get very unhappy - me included :-) .
Heiko
^ permalink raw reply [flat|nested] 25+ messages in thread
end of thread, other threads:[~2016-06-29 14:52 UTC | newest]
Thread overview: 25+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-06-13 2:10 [PATCH v5 0/2] Add a new Rockchip usb2 phy driver Frank Wang
2016-06-13 2:10 ` [PATCH v5 1/2] Documentation: bindings: add DT documentation for Rockchip USB2PHY Frank Wang
2016-06-13 8:38 ` Heiko Stübner
2016-06-14 13:28 ` Heiko Stübner
2016-06-15 1:24 ` Frank Wang
2016-06-14 22:26 ` Rob Herring
2016-06-13 2:10 ` [PATCH v5 2/2] phy: rockchip-inno-usb2: add a new driver for Rockchip usb2phy Frank Wang
2016-06-14 13:27 ` Heiko Stübner
2016-06-14 13:50 ` Guenter Roeck
2016-06-14 14:00 ` Heiko Stübner
2016-06-15 1:14 ` Frank Wang
2016-06-15 15:47 ` Guenter Roeck
2016-06-16 1:47 ` Frank Wang
2016-06-16 13:12 ` Guenter Roeck
2016-06-17 0:57 ` Frank Wang
2016-06-15 3:23 ` Frank Wang
2016-06-15 9:04 ` Heiko Stübner
2016-06-15 10:58 ` Frank Wang
2016-06-15 18:49 ` Heiko Stübner
2016-06-14 14:52 ` Guenter Roeck
2016-06-14 15:20 ` Heiko Stübner
2016-06-17 11:54 ` Kishon Vijay Abraham I
2016-06-17 16:46 ` Heiko Stübner
2016-06-29 14:14 ` Kishon Vijay Abraham I
2016-06-29 14:27 ` Heiko Stuebner
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).