2014-12-05 12:53:10

by Yunzhi Li

[permalink] [raw]
Subject: [PATCH v2 0/5]

Patches to add support for Rockchip usb phys.Add a new Rockchip
usb phy driver and modify dwc2 controller driver to make dwc2
platform devices support a generic PHY framework driver. This
patch set has been tested on my rk3288-evb and power off the usb
phys would reduce about 60mW power budget in total during sustem
suspend.


Yunzhi Li (5):
phy: add a driver for the Rockchip SoC internal USB2.0 PHY.
Documentation: bindings: add doc for the Rockchip usb PHY
usb: dwc2: Add generic PHY framework support for dwc2 usb
controler platform driver.
ARM: dts: add rk3288 usb PHY
ARM: dts: Enable usb PHY on rk3288-evb board

.../devicetree/bindings/phy/rockchip-usb-phy.txt | 22 +++
arch/arm/boot/dts/rk3288-evb.dtsi | 4 +
arch/arm/boot/dts/rk3288.dtsi | 13 ++
drivers/phy/Kconfig | 7 +
drivers/phy/Makefile | 1 +
drivers/phy/phy-rockchip-usb.c | 179 +++++++++++++++++++++
drivers/usb/dwc2/gadget.c | 33 ++--
drivers/usb/dwc2/platform.c | 34 ++++
8 files changed, 272 insertions(+), 21 deletions(-)
create mode 100644 Documentation/devicetree/bindings/phy/rockchip-usb-phy.txt
create mode 100644 drivers/phy/phy-rockchip-usb.c

--
2.0.0


2014-12-05 13:38:30

by Romain Perier

[permalink] [raw]
Subject: Re: [PATCH v2 1/5] phy: add a driver for the Rockchip SoC internal USB2.0 PHY.

Hi,

Some quick comments

2014-12-05 13:52 GMT+01:00 Yunzhi Li <[email protected]>:
> This patche to add a generic PHY driver for ROCKCHIP usb PHYs,
> currently this driver can support RK3288. The RK3288 SoC have
> three independent USB PHY IPs which are all configured through a
> set of registers located in the GRF (general register files)
> module.
>
> Signed-off-by: Yunzhi Li <[email protected]>
> ---
>
> drivers/phy/Kconfig | 7 ++
> drivers/phy/Makefile | 1 +
> drivers/phy/phy-rockchip-usb.c | 179 +++++++++++++++++++++++++++++++++++++++++
> 3 files changed, 187 insertions(+)
> create mode 100644 drivers/phy/phy-rockchip-usb.c
>
> diff --git a/drivers/phy/Kconfig b/drivers/phy/Kconfig
> index ccad880..e3a5857 100644
> --- a/drivers/phy/Kconfig
> +++ b/drivers/phy/Kconfig
> @@ -239,6 +239,13 @@ config PHY_QCOM_IPQ806X_SATA
> depends on OF
> select GENERIC_PHY
>
> +config PHY_ROCKCHIP_RK3288_USB2
> + tristate "Rockchip USB2 RK3288 PHY Driver"
> + depends on ARCH_ROCKCHIP && OF
> + select GENERIC_PHY
> + help
> + Enable this to support the Rockchip USB 2.0 PHY.
> +


The C module is named phy-rockchip-usb.c, why did you call the config
entry PHY_ROCKCHIP_RK3288_USB2 ? why RK3288 ? this driver might be
ported to old SoCs later.
I think that PHY_ROCKCHIP_USB would be enough.

> +#include <linux/regmap.h>
> +#include <linux/mfd/syscon.h>
> +
> +#define ROCKCHIP_RK3288_UOC(n) (0x320 + n * 0x14)
> +
> +#define SIDDQ_MSK (1 << (13 + 16))
> +#define SIDDQ_ON (1 << 13)

You could probably use BIT(13) and BIT(13+16) here.

Thanks,
Romain

2014-12-05 13:47:45

by Romain Perier

[permalink] [raw]
Subject: Re: [PATCH v2 4/5] ARM: dts: add rk3288 usb PHY

Hi,

2014-12-05 13:52 GMT+01:00 Yunzhi Li <[email protected]>:
> Add dt nodes for rk3288 usb PHY.

Could you give more context about the changes in the long description ?
Usually, you need a short and a long description for a commit.

For example:

short: "ARM: dts: rockchip: add rk3288 usb PHY"
long: "This patch adds a device_node phy usb for RK3288 SoC. It also
defines the phy to be used by usbhost and usbotg"



Romain

2014-12-05 12:53:05

by Yunzhi Li

[permalink] [raw]
Subject: [PATCH v2 2/5] Documentation: bindings: add doc for the Rockchip usb PHY

Document the bindings of the Rockchip usb PHY driver.

Signed-off-by: Yunzhi Li <[email protected]>
---

.../devicetree/bindings/phy/rockchip-usb-phy.txt | 22 ++++++++++++++++++++++
1 file changed, 22 insertions(+)
create mode 100644 Documentation/devicetree/bindings/phy/rockchip-usb-phy.txt

diff --git a/Documentation/devicetree/bindings/phy/rockchip-usb-phy.txt b/Documentation/devicetree/bindings/phy/rockchip-usb-phy.txt
new file mode 100644
index 0000000..4574696
--- /dev/null
+++ b/Documentation/devicetree/bindings/phy/rockchip-usb-phy.txt
@@ -0,0 +1,22 @@
+ROCKCHIP USB2 PHY
+
+Required properties:
+ - compatible: rockchip,rk3288-usb-phy
+ - rockchip,grf : phandle to the syscon managing the "general
+ register files"
+ - #phy-cells: must be 1
+
+Optional Properties:
+ - clocks : phandle + clock specifier for the phy clocks
+ - clock-names :
+ "usbphy0" - PHY clock for OTG controller
+ "usbphy1" - PHY clock for HOST0 controller
+ "usbphy2" - PHY clock for HOST1 controller
+
+Example:
+
+usbphy: phy {
+ #phy-cells = <1>;
+ compatible = "rockchip,rk3288-usb-phy";
+ rockchip,grf = <&grf>;
+};
--
2.0.0

2014-12-05 12:54:30

by Yunzhi Li

[permalink] [raw]
Subject: [PATCH v2 1/5] phy: add a driver for the Rockchip SoC internal USB2.0 PHY.

This patche to add a generic PHY driver for ROCKCHIP usb PHYs,
currently this driver can support RK3288. The RK3288 SoC have
three independent USB PHY IPs which are all configured through a
set of registers located in the GRF (general register files)
module.

Signed-off-by: Yunzhi Li <[email protected]>
---

drivers/phy/Kconfig | 7 ++
drivers/phy/Makefile | 1 +
drivers/phy/phy-rockchip-usb.c | 179 +++++++++++++++++++++++++++++++++++++++++
3 files changed, 187 insertions(+)
create mode 100644 drivers/phy/phy-rockchip-usb.c

diff --git a/drivers/phy/Kconfig b/drivers/phy/Kconfig
index ccad880..e3a5857 100644
--- a/drivers/phy/Kconfig
+++ b/drivers/phy/Kconfig
@@ -239,6 +239,13 @@ config PHY_QCOM_IPQ806X_SATA
depends on OF
select GENERIC_PHY

+config PHY_ROCKCHIP_RK3288_USB2
+ tristate "Rockchip USB2 RK3288 PHY Driver"
+ depends on ARCH_ROCKCHIP && OF
+ select GENERIC_PHY
+ help
+ Enable this to support the Rockchip USB 2.0 PHY.
+
config PHY_ST_SPEAR1310_MIPHY
tristate "ST SPEAR1310-MIPHY driver"
select GENERIC_PHY
diff --git a/drivers/phy/Makefile b/drivers/phy/Makefile
index aa74f96..12c17f6 100644
--- a/drivers/phy/Makefile
+++ b/drivers/phy/Makefile
@@ -28,6 +28,7 @@ phy-exynos-usb2-$(CONFIG_PHY_EXYNOS5250_USB2) += phy-exynos5250-usb2.o
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_RK3288_USB2) += phy-rockchip-usb.o
obj-$(CONFIG_PHY_QCOM_IPQ806X_SATA) += phy-qcom-ipq806x-sata.o
obj-$(CONFIG_PHY_ST_SPEAR1310_MIPHY) += phy-spear1310-miphy.o
obj-$(CONFIG_PHY_ST_SPEAR1340_MIPHY) += phy-spear1340-miphy.o
diff --git a/drivers/phy/phy-rockchip-usb.c b/drivers/phy/phy-rockchip-usb.c
new file mode 100644
index 0000000..3132db9
--- /dev/null
+++ b/drivers/phy/phy-rockchip-usb.c
@@ -0,0 +1,179 @@
+/*
+ * Rockchip usb PHY driver
+ *
+ * Copyright (C) 2014 Yunzhi Li <[email protected]>
+ * Copyright (C) 2014 ROCKCHIP, Inc.
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License.
+ *
+ * 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/io.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/mutex.h>
+#include <linux/of.h>
+#include <linux/of_address.h>
+#include <linux/phy/phy.h>
+#include <linux/platform_device.h>
+#include <linux/regulator/consumer.h>
+#include <linux/reset.h>
+#include <linux/regmap.h>
+#include <linux/mfd/syscon.h>
+
+#define ROCKCHIP_RK3288_UOC(n) (0x320 + n * 0x14)
+
+#define SIDDQ_MSK (1 << (13 + 16))
+#define SIDDQ_ON (1 << 13)
+#define SIDDQ_OFF (0 << 13)
+
+enum rk3288_phy_id {
+ RK3288_OTG,
+ RK3288_HOST0,
+ RK3288_HOST1,
+ RK3288_NUM_PHYS,
+};
+
+struct rockchip_usb_phy {
+ struct regmap *reg_base;
+ unsigned int reg_offset;
+ struct clk *clk;
+ struct phy *phy;
+};
+
+static int rockchip_usb_phy_power(struct rockchip_usb_phy *phy,
+ bool siddq)
+{
+ return regmap_write(phy->reg_base, phy->reg_offset,
+ SIDDQ_MSK | (siddq ? SIDDQ_ON : SIDDQ_OFF));
+}
+
+static int rockchip_usb_phy_power_off(struct phy *_phy)
+{
+ struct rockchip_usb_phy *phy = phy_get_drvdata(_phy);
+ int ret = 0;
+
+ /* Power down usb phy analog blocks by set siddq 1*/
+ ret = rockchip_usb_phy_power(phy, 1);
+ if (ret)
+ return ret;
+
+ clk_disable_unprepare(phy->clk);
+ if (ret)
+ return ret;
+
+ return 0;
+}
+
+static int rockchip_usb_phy_power_on(struct phy *_phy)
+{
+ struct rockchip_usb_phy *phy = phy_get_drvdata(_phy);
+ int ret = 0;
+
+ ret = clk_prepare_enable(phy->clk);
+ if (ret)
+ return ret;
+
+ /* Power up usb phy analog blocks by set siddq 0*/
+ ret = rockchip_usb_phy_power(phy, 0);
+ if (ret)
+ return ret;
+
+ return 0;
+}
+
+static struct phy *rockchip_usb_phy_xlate(struct device *dev,
+ struct of_phandle_args *args)
+{
+ struct rockchip_usb_phy *phy_array = dev_get_drvdata(dev);
+
+ if (WARN_ON(args->args[0] == 0 || args->args[0] >= RK3288_NUM_PHYS))
+ return ERR_PTR(-ENODEV);
+
+ return (phy_array + args->args[0])->phy;
+}
+
+static struct phy_ops ops = {
+ .power_on = rockchip_usb_phy_power_on,
+ .power_off = rockchip_usb_phy_power_off,
+ .owner = THIS_MODULE,
+};
+
+static int rockchip_usb_phy_probe(struct platform_device *pdev)
+{
+ struct device *dev = &pdev->dev;
+ struct rockchip_usb_phy *rk_phy;
+ struct rockchip_usb_phy *phy_array;
+ struct phy_provider *phy_provider;
+ struct regmap *grf;
+ char clk_name[16];
+ int i;
+
+ grf = syscon_regmap_lookup_by_phandle(dev->of_node, "rockchip,grf");
+ if (IS_ERR(grf)) {
+ dev_err(&pdev->dev, "Missing rockchip,grf property\n");
+ return PTR_ERR(grf);
+ }
+
+ phy_array = devm_kzalloc(dev, RK3288_NUM_PHYS * sizeof(*rk_phy),
+ GFP_KERNEL);
+ if (!phy_array)
+ return -ENOMEM;
+
+ for (i = 0; i < RK3288_NUM_PHYS; i++) {
+ rk_phy = &phy_array[i];
+
+ rk_phy->reg_base = grf;
+
+ rk_phy->reg_offset = ROCKCHIP_RK3288_UOC(i);
+
+ snprintf(clk_name, sizeof(clk_name), "usbphy%d", i);
+ rk_phy->clk = devm_clk_get(dev, clk_name);
+ if (IS_ERR(rk_phy->clk)) {
+ dev_warn(dev, "failed to get clock %s\n", clk_name);
+ rk_phy->clk = NULL;
+ }
+
+ rk_phy->phy = devm_phy_create(dev, NULL, &ops);
+ if (IS_ERR(rk_phy->phy)) {
+ dev_err(dev, "failed to create PHY %d\n", i);
+ return PTR_ERR(rk_phy->phy);
+ }
+ phy_set_drvdata(rk_phy->phy, rk_phy);
+ }
+
+ platform_set_drvdata(pdev, phy_array);
+
+ phy_provider = devm_of_phy_provider_register(dev,
+ rockchip_usb_phy_xlate);
+ return PTR_ERR_OR_ZERO(phy_provider);
+}
+
+static const struct of_device_id rockchip_usb_phy_dt_ids[] = {
+ { .compatible = "rockchip,rk3288-usb-phy" },
+ {}
+};
+
+MODULE_DEVICE_TABLE(of, rockchip_usb_phy_dt_ids);
+
+static struct platform_driver rockchip_usb_driver = {
+ .probe = rockchip_usb_phy_probe,
+ .driver = {
+ .name = "rockchip-usb-phy",
+ .owner = THIS_MODULE,
+ .of_match_table = rockchip_usb_phy_dt_ids,
+ },
+};
+
+module_platform_driver(rockchip_usb_driver);
+
+MODULE_AUTHOR("Yunzhi Li <[email protected]>");
+MODULE_DESCRIPTION("Rockchip USB 2.0 PHY driver");
+MODULE_LICENSE("GPL v2");
--
2.0.0

2014-12-05 12:53:07

by Yunzhi Li

[permalink] [raw]
Subject: [PATCH v2 3/5] usb: dwc2: Add generic PHY framework support for dwc2 usb controler platform driver.

Get PHY parameters from devicetree and power off usb PHY during
system suspend.

Signed-off-by: Yunzhi Li <[email protected]>
---

drivers/usb/dwc2/gadget.c | 33 ++++++++++++---------------------
drivers/usb/dwc2/platform.c | 34 ++++++++++++++++++++++++++++++++++
2 files changed, 46 insertions(+), 21 deletions(-)

diff --git a/drivers/usb/dwc2/gadget.c b/drivers/usb/dwc2/gadget.c
index 200168e..2601c61 100644
--- a/drivers/usb/dwc2/gadget.c
+++ b/drivers/usb/dwc2/gadget.c
@@ -3410,8 +3410,6 @@ int dwc2_gadget_init(struct dwc2_hsotg *hsotg, int irq)
{
struct device *dev = hsotg->dev;
struct s3c_hsotg_plat *plat = dev->platform_data;
- struct phy *phy;
- struct usb_phy *uphy;
struct s3c_hsotg_ep *eps;
int epnum;
int ret;
@@ -3421,30 +3419,23 @@ int dwc2_gadget_init(struct dwc2_hsotg *hsotg, int irq)
hsotg->phyif = GUSBCFG_PHYIF16;

/*
- * Attempt to find a generic PHY, then look for an old style
- * USB PHY, finally fall back to pdata
+ * If platform probe couldn't find a generic PHY or an old style
+ * USB PHY, fall back to pdata
*/
- phy = devm_phy_get(dev, "usb2-phy");
- if (IS_ERR(phy)) {
- uphy = devm_usb_get_phy(dev, USB_PHY_TYPE_USB2);
- if (IS_ERR(uphy)) {
- /* Fallback for pdata */
- plat = dev_get_platdata(dev);
- if (!plat) {
- dev_err(dev,
- "no platform data or transceiver defined\n");
- return -EPROBE_DEFER;
- }
- hsotg->plat = plat;
- } else
- hsotg->uphy = uphy;
- } else {
- hsotg->phy = phy;
+ if (IS_ERR_OR_NULL(hsotg->phy) && IS_ERR_OR_NULL(hsotg->uphy)) {
+ plat = dev_get_platdata(dev);
+ if (!plat) {
+ dev_err(dev,
+ "no platform data or transceiver defined\n");
+ return -EPROBE_DEFER;
+ }
+ hsotg->plat = plat;
+ } else if (hsotg->phy) {
/*
* If using the generic PHY framework, check if the PHY bus
* width is 8-bit and set the phyif appropriately.
*/
- if (phy_get_bus_width(phy) == 8)
+ if (phy_get_bus_width(hsotg->phy) == 8)
hsotg->phyif = GUSBCFG_PHYIF8;
}

diff --git a/drivers/usb/dwc2/platform.c b/drivers/usb/dwc2/platform.c
index 6a795aa..739d14f 100644
--- a/drivers/usb/dwc2/platform.c
+++ b/drivers/usb/dwc2/platform.c
@@ -155,6 +155,8 @@ static int dwc2_driver_probe(struct platform_device *dev)
struct dwc2_core_params defparams;
struct dwc2_hsotg *hsotg;
struct resource *res;
+ struct phy *phy;
+ struct usb_phy *uphy;
int retval;
int irq;

@@ -212,6 +214,24 @@ static int dwc2_driver_probe(struct platform_device *dev)

hsotg->dr_mode = of_usb_get_dr_mode(dev->dev.of_node);

+ /*
+ * Attempt to find a generic PHY, then look for an old style
+ * USB PHY
+ */
+ phy = devm_phy_get(&dev->dev, "usb2-phy");
+ if (IS_ERR(phy)) {
+ hsotg->phy = NULL;
+ uphy = devm_usb_get_phy(&dev->dev, USB_PHY_TYPE_USB2);
+ if (IS_ERR(uphy))
+ hsotg->uphy = NULL;
+ else
+ hsotg->uphy = uphy;
+ } else {
+ hsotg->phy = phy;
+ phy_power_on(hsotg->phy);
+ phy_init(hsotg->phy);
+ }
+
spin_lock_init(&hsotg->lock);
mutex_init(&hsotg->init_mutex);
retval = dwc2_gadget_init(hsotg, irq);
@@ -233,6 +253,14 @@ static int __maybe_unused dwc2_suspend(struct device *dev)

if (dwc2_is_device_mode(dwc2))
ret = s3c_hsotg_suspend(dwc2);
+ else {
+ if (dwc2->lx_state == DWC2_L0)
+ return 0;
+ if (dwc2->phy) {
+ phy_exit(dwc2->phy);
+ phy_power_off(dwc2->phy);
+ }
+ }
return ret;
}

@@ -243,6 +271,12 @@ static int __maybe_unused dwc2_resume(struct device *dev)

if (dwc2_is_device_mode(dwc2))
ret = s3c_hsotg_resume(dwc2);
+ else {
+ if (dwc2->phy) {
+ phy_power_on(dwc2->phy);
+ phy_init(dwc2->phy);
+ }
+ }
return ret;
}

--
2.0.0

2014-12-05 12:53:13

by Yunzhi Li

[permalink] [raw]
Subject: [PATCH v2 4/5] ARM: dts: add rk3288 usb PHY

Add dt nodes for rk3288 usb PHY.

Signed-off-by: Yunzhi Li <[email protected]>
---

arch/arm/boot/dts/rk3288.dtsi | 13 +++++++++++++
1 file changed, 13 insertions(+)

diff --git a/arch/arm/boot/dts/rk3288.dtsi b/arch/arm/boot/dts/rk3288.dtsi
index 874e66d..a663c4c 100644
--- a/arch/arm/boot/dts/rk3288.dtsi
+++ b/arch/arm/boot/dts/rk3288.dtsi
@@ -329,12 +329,21 @@
status = "disabled";
};

+ usbphy: phy {
+ #phy-cells = <1>;
+ compatible = "rockchip,rk3288-usb-phy";
+ rockchip,grf = <&grf>;
+ status = "disabled";
+ };
+
usb_host0_ehci: usb@ff500000 {
compatible = "generic-ehci";
reg = <0xff500000 0x100>;
interrupts = <GIC_SPI 24 IRQ_TYPE_LEVEL_HIGH>;
clocks = <&cru HCLK_USBHOST0>;
clock-names = "usbhost";
+ phys = <&usbphy 1>;
+ phy-names = "usb";
status = "disabled";
};

@@ -347,6 +356,8 @@
interrupts = <GIC_SPI 25 IRQ_TYPE_LEVEL_HIGH>;
clocks = <&cru HCLK_USBHOST1>;
clock-names = "otg";
+ phys = <&usbphy 2>;
+ phy-names = "usb2-phy";
status = "disabled";
};

@@ -357,6 +368,8 @@
interrupts = <GIC_SPI 23 IRQ_TYPE_LEVEL_HIGH>;
clocks = <&cru HCLK_OTG0>;
clock-names = "otg";
+ phys = <&usbphy 0>;
+ phy-names = "usb2-phy";
status = "disabled";
};

--
2.0.0

2014-12-05 19:04:09

by Paul Zimmerman

[permalink] [raw]
Subject: RE: [PATCH v2 3/5] usb: dwc2: Add generic PHY framework support for dwc2 usb controler platform driver.

> From: Yunzhi Li [mailto:[email protected]]
> Sent: Friday, December 05, 2014 4:52 AM
>
> Get PHY parameters from devicetree and power off usb PHY during
> system suspend.
>
> Signed-off-by: Yunzhi Li <[email protected]>
> ---
>
> drivers/usb/dwc2/gadget.c | 33 ++++++++++++---------------------
> drivers/usb/dwc2/platform.c | 34 ++++++++++++++++++++++++++++++++++
> 2 files changed, 46 insertions(+), 21 deletions(-)
>
> diff --git a/drivers/usb/dwc2/gadget.c b/drivers/usb/dwc2/gadget.c
> index 200168e..2601c61 100644
> --- a/drivers/usb/dwc2/gadget.c
> +++ b/drivers/usb/dwc2/gadget.c
> @@ -3410,8 +3410,6 @@ int dwc2_gadget_init(struct dwc2_hsotg *hsotg, int irq)
> {
> struct device *dev = hsotg->dev;
> struct s3c_hsotg_plat *plat = dev->platform_data;
> - struct phy *phy;
> - struct usb_phy *uphy;
> struct s3c_hsotg_ep *eps;
> int epnum;
> int ret;
> @@ -3421,30 +3419,23 @@ int dwc2_gadget_init(struct dwc2_hsotg *hsotg, int irq)
> hsotg->phyif = GUSBCFG_PHYIF16;
>
> /*
> - * Attempt to find a generic PHY, then look for an old style
> - * USB PHY, finally fall back to pdata
> + * If platform probe couldn't find a generic PHY or an old style
> + * USB PHY, fall back to pdata
> */
> - phy = devm_phy_get(dev, "usb2-phy");
> - if (IS_ERR(phy)) {
> - uphy = devm_usb_get_phy(dev, USB_PHY_TYPE_USB2);
> - if (IS_ERR(uphy)) {
> - /* Fallback for pdata */
> - plat = dev_get_platdata(dev);
> - if (!plat) {
> - dev_err(dev,
> - "no platform data or transceiver defined\n");
> - return -EPROBE_DEFER;
> - }
> - hsotg->plat = plat;
> - } else
> - hsotg->uphy = uphy;
> - } else {
> - hsotg->phy = phy;
> + if (IS_ERR_OR_NULL(hsotg->phy) && IS_ERR_OR_NULL(hsotg->uphy)) {
> + plat = dev_get_platdata(dev);
> + if (!plat) {
> + dev_err(dev,
> + "no platform data or transceiver defined\n");
> + return -EPROBE_DEFER;
> + }
> + hsotg->plat = plat;
> + } else if (hsotg->phy) {

You have changed the behavior here. Previously, the driver would work
even if there were no phys or pdata defined. Now it will return
-EPROBE_DEFER instead. Are you sure that won't break any existing
platforms?

> /*
> * If using the generic PHY framework, check if the PHY bus
> * width is 8-bit and set the phyif appropriately.
> */
> - if (phy_get_bus_width(phy) == 8)
> + if (phy_get_bus_width(hsotg->phy) == 8)
> hsotg->phyif = GUSBCFG_PHYIF8;
> }
>
> diff --git a/drivers/usb/dwc2/platform.c b/drivers/usb/dwc2/platform.c
> index 6a795aa..739d14f 100644
> --- a/drivers/usb/dwc2/platform.c
> +++ b/drivers/usb/dwc2/platform.c
> @@ -155,6 +155,8 @@ static int dwc2_driver_probe(struct platform_device *dev)
> struct dwc2_core_params defparams;
> struct dwc2_hsotg *hsotg;
> struct resource *res;
> + struct phy *phy;
> + struct usb_phy *uphy;
> int retval;
> int irq;
>
> @@ -212,6 +214,24 @@ static int dwc2_driver_probe(struct platform_device *dev)
>
> hsotg->dr_mode = of_usb_get_dr_mode(dev->dev.of_node);
>
> + /*
> + * Attempt to find a generic PHY, then look for an old style
> + * USB PHY
> + */
> + phy = devm_phy_get(&dev->dev, "usb2-phy");
> + if (IS_ERR(phy)) {
> + hsotg->phy = NULL;
> + uphy = devm_usb_get_phy(&dev->dev, USB_PHY_TYPE_USB2);
> + if (IS_ERR(uphy))
> + hsotg->uphy = NULL;
> + else
> + hsotg->uphy = uphy;
> + } else {
> + hsotg->phy = phy;
> + phy_power_on(hsotg->phy);
> + phy_init(hsotg->phy);
> + }
> +
> spin_lock_init(&hsotg->lock);
> mutex_init(&hsotg->init_mutex);
> retval = dwc2_gadget_init(hsotg, irq);
> @@ -233,6 +253,14 @@ static int __maybe_unused dwc2_suspend(struct device *dev)
>
> if (dwc2_is_device_mode(dwc2))
> ret = s3c_hsotg_suspend(dwc2);
> + else {

Kernel style says that both branches of the if() should have braces
here.

> + if (dwc2->lx_state == DWC2_L0)
> + return 0;
> + if (dwc2->phy) {
> + phy_exit(dwc2->phy);
> + phy_power_off(dwc2->phy);
> + }

Minor nit: no need to test dwc2->phy for NULL here, the phy functions
handle a NULL pointer just fine.

> + }
> return ret;
> }
>
> @@ -243,6 +271,12 @@ static int __maybe_unused dwc2_resume(struct device *dev)
>
> if (dwc2_is_device_mode(dwc2))
> ret = s3c_hsotg_resume(dwc2);
> + else {

Braces.

> + if (dwc2->phy) {
> + phy_power_on(dwc2->phy);
> + phy_init(dwc2->phy);
> + }
> + }
> return ret;
> }
>

--
Paul

2014-12-08 03:58:27

by Yunzhi Li

[permalink] [raw]
Subject: Re: [PATCH v2 3/5] usb: dwc2: Add generic PHY framework support for dwc2 usb controler platform driver.

Hi Paul:

Thank you for replying.

On 2014/12/6 3:04, Paul Zimmerman wrote:
>> From: Yunzhi Li [mailto:[email protected]]
>> Sent: Friday, December 05, 2014 4:52 AM
>>
>> Get PHY parameters from devicetree and power off usb PHY during
>> system suspend.
>>
>> Signed-off-by: Yunzhi Li <[email protected]>
>> ---
>>
>> drivers/usb/dwc2/gadget.c | 33 ++++++++++++---------------------
>> drivers/usb/dwc2/platform.c | 34 ++++++++++++++++++++++++++++++++++
>> 2 files changed, 46 insertions(+), 21 deletions(-)
>>
>> diff --git a/drivers/usb/dwc2/gadget.c b/drivers/usb/dwc2/gadget.c
>> index 200168e..2601c61 100644
>> --- a/drivers/usb/dwc2/gadget.c
>> +++ b/drivers/usb/dwc2/gadget.c
>> [...]
>>
>> /*
>> - * Attempt to find a generic PHY, then look for an old style
>> - * USB PHY, finally fall back to pdata
>> + * If platform probe couldn't find a generic PHY or an old style
>> + * USB PHY, fall back to pdata
>> */
>> - phy = devm_phy_get(dev, "usb2-phy");
>> - if (IS_ERR(phy)) {
>> - uphy = devm_usb_get_phy(dev, USB_PHY_TYPE_USB2);
>> - if (IS_ERR(uphy)) {
>> - /* Fallback for pdata */
>> - plat = dev_get_platdata(dev);
>> - if (!plat) {
>> - dev_err(dev,
>> - "no platform data or transceiver defined\n");
>> - return -EPROBE_DEFER;
>> - }
>> - hsotg->plat = plat;
>> - } else
>> - hsotg->uphy = uphy;
>> - } else {
>> - hsotg->phy = phy;
>> + if (IS_ERR_OR_NULL(hsotg->phy) && IS_ERR_OR_NULL(hsotg->uphy)) {
>> + plat = dev_get_platdata(dev);
>> + if (!plat) {
>> + dev_err(dev,
>> + "no platform data or transceiver defined\n");
>> + return -EPROBE_DEFER;
>> + }
>> + hsotg->plat = plat;
>> + } else if (hsotg->phy) {
> You have changed the behavior here. Previously, the driver would work
> even if there were no phys or pdata defined. Now it will return
> -EPROBE_DEFER instead. Are you sure that won't break any existing
> platforms?
>
I don't really catch your meaning. Could you please point out where is
the difference? Thanks .

In previous version, if there were no phy or usb_phy defined,
devm_phy_get() and devm_usb_get_phy() both failed, then driver falls
back to find pdata, if no pdata defined dev_get_platdata() returns NULL,
then dwc2_gadget_init returns -EPROBE_DEFER and the dwc2_driver_probe
fail. So I think, for the dwc2 driver without my patch, when
CONFIG_USB_DWC2_PERIPHERAL or CONFIG_USB_DWC2_DUAL_ROLE selected, if
there were no phys or pdata defined, the driver probe will also return
-EPROBE_DEFER. And when only CONFIG_USB_DWC2_HOST selected, the previous
version driver didn't care any thing about phy, in my patch it will try
to find a phy when platform driver probe, but do not fail probe if there
isn't a phy node.

2014-12-08 04:12:41

by Yunzhi Li

[permalink] [raw]
Subject: Re: [PATCH v2 1/5] phy: add a driver for the Rockchip SoC internal USB2.0 PHY.

Hi Romain :

On 2014/12/5 21:38, Romain Perier wrote:
> Hi,
>
> Some quick comments
>
> 2014-12-05 13:52 GMT+01:00 Yunzhi Li <[email protected]>:
>> [...]
>>
>> diff --git a/drivers/phy/Kconfig b/drivers/phy/Kconfig
>> index ccad880..e3a5857 100644
>> --- a/drivers/phy/Kconfig
>> +++ b/drivers/phy/Kconfig
>> @@ -239,6 +239,13 @@ config PHY_QCOM_IPQ806X_SATA
>> depends on OF
>> select GENERIC_PHY
>>
>> +config PHY_ROCKCHIP_RK3288_USB2
>> + tristate "Rockchip USB2 RK3288 PHY Driver"
>> + depends on ARCH_ROCKCHIP && OF
>> + select GENERIC_PHY
>> + help
>> + Enable this to support the Rockchip USB 2.0 PHY.
>> +
>
> The C module is named phy-rockchip-usb.c, why did you call the config
> entry PHY_ROCKCHIP_RK3288_USB2 ? why RK3288 ? this driver might be
> ported to old SoCs later.
> I think that PHY_ROCKCHIP_USB would be enough.
Right, this driver will be ported to old SoCs like RK3188 or RK3066,
this will be renamed to PHY_ROCKCHIP_USB in the next version.

2014-12-08 19:47:05

by Paul Zimmerman

[permalink] [raw]
Subject: RE: [PATCH v2 3/5] usb: dwc2: Add generic PHY framework support for dwc2 usb controler platform driver.

> From: Yunzhi Li [mailto:[email protected]]
> Sent: Sunday, December 07, 2014 7:58 PM
>
> On 2014/12/6 3:04, Paul Zimmerman wrote:
> >> From: Yunzhi Li [mailto:[email protected]]
> >> Sent: Friday, December 05, 2014 4:52 AM
> >>
> >> Get PHY parameters from devicetree and power off usb PHY during
> >> system suspend.
> >>
> >> Signed-off-by: Yunzhi Li <[email protected]>
> >> ---
> >>
> >> drivers/usb/dwc2/gadget.c | 33 ++++++++++++---------------------
> >> drivers/usb/dwc2/platform.c | 34 ++++++++++++++++++++++++++++++++++
> >> 2 files changed, 46 insertions(+), 21 deletions(-)
> >>
> >> diff --git a/drivers/usb/dwc2/gadget.c b/drivers/usb/dwc2/gadget.c
> >> index 200168e..2601c61 100644
> >> --- a/drivers/usb/dwc2/gadget.c
> >> +++ b/drivers/usb/dwc2/gadget.c
> >> [...]
> >>
> >> /*
> >> - * Attempt to find a generic PHY, then look for an old style
> >> - * USB PHY, finally fall back to pdata
> >> + * If platform probe couldn't find a generic PHY or an old style
> >> + * USB PHY, fall back to pdata
> >> */
> >> - phy = devm_phy_get(dev, "usb2-phy");
> >> - if (IS_ERR(phy)) {
> >> - uphy = devm_usb_get_phy(dev, USB_PHY_TYPE_USB2);
> >> - if (IS_ERR(uphy)) {
> >> - /* Fallback for pdata */
> >> - plat = dev_get_platdata(dev);
> >> - if (!plat) {
> >> - dev_err(dev,
> >> - "no platform data or transceiver defined\n");
> >> - return -EPROBE_DEFER;
> >> - }
> >> - hsotg->plat = plat;
> >> - } else
> >> - hsotg->uphy = uphy;
> >> - } else {
> >> - hsotg->phy = phy;
> >> + if (IS_ERR_OR_NULL(hsotg->phy) && IS_ERR_OR_NULL(hsotg->uphy)) {
> >> + plat = dev_get_platdata(dev);
> >> + if (!plat) {
> >> + dev_err(dev,
> >> + "no platform data or transceiver defined\n");
> >> + return -EPROBE_DEFER;
> >> + }
> >> + hsotg->plat = plat;
> >> + } else if (hsotg->phy) {
> > You have changed the behavior here. Previously, the driver would work
> > even if there were no phys or pdata defined. Now it will return
> > -EPROBE_DEFER instead. Are you sure that won't break any existing
> > platforms?
> >
> I don't really catch your meaning. Could you please point out where is
> the difference? Thanks .

Yeah, sorry, I misread the patch. I think your new version is fine.

--
Paul

2020-10-27 09:05:01

by Mark Brown

[permalink] [raw]
Subject: Re: [PATCH v2 0/5]

On Fri, 23 Oct 2020 16:19:20 +0300, Dmitry Baryshkov wrote:
> On SM8250 MDSS_GDSC (and the rest of display clock controller) is
> supplied power by MMCX power domain. Handle this link in GDSC code by
> binding the power domain in dts file.
>
> This patchset depends on [1]
>
> Changes since v1:
> - Define fixed-regulator-domain regulator using power domain
> performance state for enabling/disabling.
> - Rework to use new fixed regulator type (fixed-regulator-domain)
> instead of controlling power domain directly from gdsc code.
>
> [...]

Applied to

https://git.kernel.org/pub/scm/linux/kernel/git/broonie/regulator.git for-next

Thanks!

[1/2] regulator: fixed: provide bindings using power domain
commit: d4189bc55d5c40251abaa1f341796aac84ddfb10
[2/2] regulator: fixed: support using power domain for enable/disable
commit: bf3a28cf42412c0a85631da94f198048bb37a8e5

All being well this means that it will be integrated into the linux-next
tree (usually sometime in the next 24 hours) and sent to Linus during
the next merge window (or sooner if it is a bug fix), however if
problems are discovered then the patch may be dropped or reverted.

You may get further e-mails resulting from automated or manual testing
and review of the tree, please engage with people reporting problems and
send followup patches addressing any issues that are reported if needed.

If any updates are required or you are submitting further changes they
should be sent as incremental updates against current git, existing
patches will not be replaced.

Please add any relevant lists and maintainers to the CCs when replying
to this mail.

Thanks,
Mark