LinuxLists.cc - [PATCH v2 00/11] da8xx USB clocks

[permalink] [raw]

Subject: [PATCH v2 11/11] usb: musb: da8xx: Remove mach code

Use the new phy-da8xx-usb driver to take the place of the mach code that
pokes CFGCHIP2 in the da8xx musb glue driver.

Signed-off-by: David Lechner <[email protected]>
---

v2 changes: This is part of a previous patch that was split. This version uses
the new phy driver instead of a second clock. It also gets rid of more mach
code.

drivers/usb/musb/da8xx.c | 126 ++++++++++++-----------------------------------
1 file changed, 31 insertions(+), 95 deletions(-)

diff --git a/drivers/usb/musb/da8xx.c b/drivers/usb/musb/da8xx.c
index 50f07a5..1d51711 100644
--- a/drivers/usb/musb/da8xx.c
+++ b/drivers/usb/musb/da8xx.c
@@ -30,13 +30,11 @@
#include <linux/clk.h>
#include <linux/err.h>
#include <linux/io.h>
+#include <linux/phy/phy.h>
#include <linux/platform_device.h>
#include <linux/dma-mapping.h>
#include <linux/usb/usb_phy_generic.h>

-#include <mach/da8xx.h>
-#include <linux/platform_data/usb-davinci.h>
-
#include "musb_core.h"

/*
@@ -80,61 +78,15 @@

#define DA8XX_MENTOR_CORE_OFFSET 0x400

-#define CFGCHIP2 IO_ADDRESS(DA8XX_SYSCFG0_BASE + DA8XX_CFGCHIP2_REG)
-
struct da8xx_glue {
struct device *dev;
struct platform_device *musb;
- struct platform_device *phy;
+ struct platform_device *usb_phy;
struct clk *clk;
+ struct phy *phy;
};

/*
- * REVISIT (PM): we should be able to keep the PHY in low power mode most
- * of the time (24 MHz oscillator and PLL off, etc.) by setting POWER.D0
- * and, when in host mode, autosuspending idle root ports... PHY_PLLON
- * (overriding SUSPENDM?) then likely needs to stay off.
- */
-
-static inline void phy_on(void)
-{
- u32 cfgchip2 = __raw_readl(CFGCHIP2);
-
- /*
- * Start the on-chip PHY and its PLL.
- */
- cfgchip2 &= ~(CFGCHIP2_RESET | CFGCHIP2_PHYPWRDN | CFGCHIP2_OTGPWRDN);
- cfgchip2 |= CFGCHIP2_PHY_PLLON;
- __raw_writel(cfgchip2, CFGCHIP2);
-
- pr_info("Waiting for USB PHY clock good...\n");
- while (!(__raw_readl(CFGCHIP2) & CFGCHIP2_PHYCLKGD))
- cpu_relax();
-}
-
-static inline void phy_off(void)
-{
- u32 cfgchip2 = __raw_readl(CFGCHIP2);
-
- /*
- * Ensure that USB 1.1 reference clock is not being sourced from
- * USB 2.0 PHY. Otherwise do not power down the PHY.
- */
- if (!(cfgchip2 & CFGCHIP2_USB1PHYCLKMUX) &&
- (cfgchip2 & CFGCHIP2_USB1SUSPENDM)) {
- pr_warning("USB 1.1 clocked from USB 2.0 PHY -- "
- "can't power it down\n");
- return;
- }
-
- /*
- * Power down the on-chip PHY.
- */
- cfgchip2 |= CFGCHIP2_PHYPWRDN | CFGCHIP2_OTGPWRDN;
- __raw_writel(cfgchip2, CFGCHIP2);
-}
-
-/*
* Because we don't set CTRL.UINT, it's "important" to:
* - not read/write INTRUSB/INTRUSBE (except during
* initial setup, as a workaround);
@@ -383,31 +335,18 @@ static irqreturn_t da8xx_musb_interrupt(int irq, void *hci)
return ret;
}

+extern int da8xx_usb20_phy_set_mode(struct phy *phy, enum musb_mode mode);
+
static int da8xx_musb_set_mode(struct musb *musb, u8 musb_mode)
{
- u32 cfgchip2 = __raw_readl(CFGCHIP2);
+ struct da8xx_glue *glue = dev_get_drvdata(musb->controller->parent);

- cfgchip2 &= ~CFGCHIP2_OTGMODE;
- switch (musb_mode) {
- case MUSB_HOST: /* Force VBUS valid, ID = 0 */
- cfgchip2 |= CFGCHIP2_FORCE_HOST;
- break;
- case MUSB_PERIPHERAL: /* Force VBUS valid, ID = 1 */
- cfgchip2 |= CFGCHIP2_FORCE_DEVICE;
- break;
- case MUSB_OTG: /* Don't override the VBUS/ID comparators */
- cfgchip2 |= CFGCHIP2_NO_OVERRIDE;
- break;
- default:
- dev_dbg(musb->controller, "Trying to set unsupported mode %u\n", musb_mode);
- }
-
- __raw_writel(cfgchip2, CFGCHIP2);
- return 0;
+ return da8xx_usb20_phy_set_mode(glue->phy, musb_mode);
}

static int da8xx_musb_init(struct musb *musb)
{
+ struct da8xx_glue *glue = dev_get_drvdata(musb->controller->parent);
void __iomem *reg_base = musb->ctrl_base;
u32 rev;
int ret = -ENODEV;
@@ -425,19 +364,24 @@ static int da8xx_musb_init(struct musb *musb)
goto fail;
}

+ ret = clk_prepare_enable(glue->clk);
+ if (ret) {
+ dev_err(glue->dev, "failed to enable clock\n");
+ goto fail;
+ }
+
setup_timer(&otg_workaround, otg_timer, (unsigned long)musb);

/* Reset the controller */
musb_writel(reg_base, DA8XX_USB_CTRL_REG, DA8XX_SOFT_RESET_MASK);

/* Start the on-chip PHY and its PLL. */
- phy_on();
+ phy_power_on(glue->phy);

msleep(5);

/* NOTE: IRQs are in mixed mode, not bypass to pure MUSB */
- pr_debug("DA8xx OTG revision %08x, PHY %03x, control %02x\n",
- rev, __raw_readl(CFGCHIP2),
+ pr_debug("DA8xx OTG revision %08x, control %02x\n", rev,
musb_readb(reg_base, DA8XX_USB_CTRL_REG));

musb->isr = da8xx_musb_interrupt;
@@ -448,9 +392,12 @@ fail:

static int da8xx_musb_exit(struct musb *musb)
{
+ struct da8xx_glue *glue = dev_get_drvdata(musb->controller->parent);
+
del_timer_sync(&otg_workaround);

- phy_off();
+ phy_power_off(glue->phy);
+ clk_disable_unprepare(glue->clk);

usb_put_phy(musb->xceiv);

@@ -503,19 +450,19 @@ static int da8xx_probe(struct platform_device *pdev)
return PTR_ERR(glue->clk);
}

- ret = clk_enable(glue->clk);
- if (ret) {
- dev_err(&pdev->dev, "failed to enable clock\n");
- goto err4;
+ glue->phy = devm_phy_get(&pdev->dev, "usbphy");
+ if (IS_ERR(glue->phy)) {
+ dev_err(&pdev->dev, "failed to get phy\n");
+ return PTR_ERR(glue->phy);
}

glue->dev = &pdev->dev;
pdata->platform_ops = &da8xx_ops;

- glue->phy = usb_phy_generic_register();
- if (IS_ERR(glue->phy)) {
- ret = PTR_ERR(glue->phy);
- goto err5;
+ glue->usb_phy = usb_phy_generic_register();
+ if (IS_ERR(glue->usb_phy)) {
+ dev_err(&pdev->dev, "failed to register usb_phy\n");
+ return PTR_ERR(glue->usb_phy);
}
platform_set_drvdata(pdev, glue);

@@ -541,22 +488,12 @@ static int da8xx_probe(struct platform_device *pdev)

glue->musb = musb = platform_device_register_full(&pinfo);
if (IS_ERR(musb)) {
- ret = PTR_ERR(musb);
dev_err(&pdev->dev, "failed to register musb device: %d\n", ret);
- goto err6;
+ usb_phy_generic_unregister(glue->usb_phy);
+ return PTR_ERR(musb);
}

return 0;
-
-err6:
- usb_phy_generic_unregister(glue->phy);
-
-err5:
- clk_disable(glue->clk);
-
-err4:
-
- return ret;
}

static int da8xx_remove(struct platform_device *pdev)
@@ -564,8 +501,7 @@ static int da8xx_remove(struct platform_device *pdev)
struct da8xx_glue *glue = platform_get_drvdata(pdev);

platform_device_unregister(glue->musb);
- usb_phy_generic_unregister(glue->phy);
- clk_disable(glue->clk);
+ usb_phy_generic_unregister(glue->usb_phy);

return 0;
}
--
1.9.1

2016-03-17 03:38:41

[permalink] [raw]

Subject: [PATCH v2 05/11] dt-bindings: Add bindings for phy-da8xx-usb

Device tree binding for new phy-da8xx-usb driver.

Signed-off-by: David Lechner <[email protected]>
---

v2 changes: This is new patch in v2.

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

diff --git a/Documentation/devicetree/bindings/phy/phy-da8xx-usb.txt b/Documentation/devicetree/bindings/phy/phy-da8xx-usb.txt
new file mode 100644
index 0000000..ed6b710
--- /dev/null
+++ b/Documentation/devicetree/bindings/phy/phy-da8xx-usb.txt
@@ -0,0 +1,34 @@
+TI DaVinci DA8XX USB PHY
+
+Required properties:
+ - compatible: must be "ti,da830-usbphy".
+ - #phy-cells: must be 1.
+ - reg : Address and length of the CFGCHIP2 register.
+
+This device controls the PHY for both the USB 1.1 OHCI and USB 2.0 OTG
+controllers on DA8XX SoCs. Consumers of this device should use index 1 for
+the USB 1.1 phy device and index 2 for the USB 2.0 phy device.
+
+Example:
+
+ usbphy: usbphy@1c14184 {
+ compatible = "ti,da830-usbphy";
+ #phy-cells = <1>;
+ reg = <0x14184 4>;
+ };
+
+ usb11: usb11@1e25000 {
+ compatible = "ti,da830-ohci";
+ reg = <0x225000 0x1000>;
+ interrupts = <59>;
+ phys = <&usbphy 1>;
+ phy-names = "usbphy";
+ };
+
+ usb20: usb@1e00000 {
+ compatible = "ti,da830-musb";
+ reg = <0x200000 0x1000>;
+ interrupts = <58>;
+ phys = <&usbphy 2>;
+ phy-names = "usbphy";
+ };
--
1.9.1

2016-03-17 03:39:28

[permalink] [raw]

Subject: [PATCH v2 06/11] phy: da8xx-usb: new driver for DA8XX SoC USB PHY

This is a new phy driver for the SoC USB controllers on the TI DA8XX
family of microcontrollers. The USB 1.1 PHY is just a simple on/off.
The USB 2.0 PHY also allows overriding the VBUS and ID pins.

Signed-off-by: David Lechner <[email protected]>
---

v2 changes: This is new patch in v2.

drivers/phy/Kconfig | 9 ++
drivers/phy/Makefile | 1 +
drivers/phy/phy-da8xx-usb.c | 295 ++++++++++++++++++++++++++++++++++++++++++++
3 files changed, 305 insertions(+)
create mode 100644 drivers/phy/phy-da8xx-usb.c

diff --git a/drivers/phy/Kconfig b/drivers/phy/Kconfig
index 26566db..a6060ea 100644
--- a/drivers/phy/Kconfig
+++ b/drivers/phy/Kconfig
@@ -35,6 +35,15 @@ config ARMADA375_USBCLUSTER_PHY
depends on OF && HAS_IOMEM
select GENERIC_PHY

+config PHY_DA8XX_USB
+ tristate "TI DA8XX USB PHY Driver"
+ depends on ARCH_DAVINCI_DA8XX
+ select GENERIC_PHY
+ help
+ Enable this to support the USB PHY on DA8XX SoCs.
+
+ This driver controls both the USB 1.1 PHY and the USB 2.0 PHY.
+
config PHY_DM816X_USB
tristate "TI dm816x USB PHY driver"
depends on ARCH_OMAP2PLUS
diff --git a/drivers/phy/Makefile b/drivers/phy/Makefile
index 24596a9..722e01c 100644
--- a/drivers/phy/Makefile
+++ b/drivers/phy/Makefile
@@ -5,6 +5,7 @@
obj-$(CONFIG_GENERIC_PHY) += phy-core.o
obj-$(CONFIG_PHY_BERLIN_USB) += phy-berlin-usb.o
obj-$(CONFIG_PHY_BERLIN_SATA) += phy-berlin-sata.o
+obj-$(CONFIG_PHY_DA8XX_USB) += phy-da8xx-usb.o
obj-$(CONFIG_PHY_DM816X_USB) += phy-dm816x-usb.o
obj-$(CONFIG_ARMADA375_USBCLUSTER_PHY) += phy-armada375-usb2.o
obj-$(CONFIG_BCM_KONA_USB2_PHY) += phy-bcm-kona-usb2.o
diff --git a/drivers/phy/phy-da8xx-usb.c b/drivers/phy/phy-da8xx-usb.c
new file mode 100644
index 0000000..93a5f4d
--- /dev/null
+++ b/drivers/phy/phy-da8xx-usb.c
@@ -0,0 +1,295 @@
+/*
+ * phy-da8xx-usb - TI DaVinci DA8XX USB PHY driver
+ *
+ * Copyright (C) 2016 David Lechner <[email protected]>
+ *
+ * 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/io.h>
+#include <linux/of.h>
+#include <linux/module.h>
+#include <linux/phy/phy.h>
+#include <linux/platform_device.h>
+#include <linux/usb/gadget.h>
+#include <linux/usb/musb.h>
+#include <linux/usb/otg.h>
+
+/* DA8xx CFGCHIP2 (USB PHY Control) register bits */
+#define PHYCLKGD (1 << 17)
+#define VBUSSENSE (1 << 16)
+#define RESET (1 << 15)
+#define OTGMODE_MASK (3 << 13)
+#define NO_OVERRIDE (0 << 13)
+#define FORCE_HOST (1 << 13)
+#define FORCE_DEVICE (2 << 13)
+#define FORCE_HOST_VBUS_LOW (3 << 13)
+#define USB1PHYCLKMUX (1 << 12)
+#define USB2PHYCLKMUX (1 << 11)
+#define PHYPWRDN (1 << 10)
+#define OTGPWRDN (1 << 9)
+#define DATPOL (1 << 8)
+#define USB1SUSPENDM (1 << 7)
+#define PHY_PLLON (1 << 6)
+#define SESENDEN (1 << 5)
+#define VBDTCTEN (1 << 4)
+#define REFFREQ_MASK (0xf << 0)
+#define REFFREQ_12MHZ (1 << 0)
+#define REFFREQ_24MHZ (2 << 0)
+#define REFFREQ_48MHZ (3 << 0)
+#define REFFREQ_19_2MHZ (4 << 0)
+#define REFFREQ_38_4MHZ (5 << 0)
+#define REFFREQ_13MHZ (6 << 0)
+#define REFFREQ_26MHZ (7 << 0)
+#define REFFREQ_20MHZ (8 << 0)
+#define REFFREQ_40MHZ (9 << 0)
+
+struct da8xx_usbphy {
+ struct phy_provider *phy_provider;
+ struct phy *usb11_phy;
+ struct phy *usb20_phy;
+ struct clk *usb11_clk;
+ struct clk *usb20_clk;
+ void __iomem *phy_ctrl;
+};
+
+static inline u32 da8xx_usbphy_readl(void __iomem *base)
+{
+ return readl(base);
+}
+
+static inline void da8xx_usbphy_writel(void __iomem *base, u32 value)
+{
+ writel(value, base);
+}
+
+static int da8xx_usb11_phy_init(struct phy *phy)
+{
+ struct da8xx_usbphy *d_phy = phy_get_drvdata(phy);
+ int ret;
+ u32 val;
+
+ ret = clk_prepare_enable(d_phy->usb11_clk);
+ if (ret)
+ return ret;
+
+ val = da8xx_usbphy_readl(d_phy->phy_ctrl);
+ val |= USB1SUSPENDM;
+ da8xx_usbphy_writel(d_phy->phy_ctrl, val);
+
+ return 0;
+}
+
+static int da8xx_usb11_phy_shutdown(struct phy *phy)
+{
+ struct da8xx_usbphy *d_phy = phy_get_drvdata(phy);
+ u32 val;
+
+ val = da8xx_usbphy_readl(d_phy->phy_ctrl);
+ val &= ~USB1SUSPENDM;
+ da8xx_usbphy_writel(d_phy->phy_ctrl, val);
+
+ clk_disable_unprepare(d_phy->usb11_clk);
+
+ return 0;
+}
+
+static const struct phy_ops da8xx_usb11_phy_ops = {
+ .power_on = da8xx_usb11_phy_init,
+ .power_off = da8xx_usb11_phy_shutdown,
+ .owner = THIS_MODULE,
+};
+
+static int da8xx_usb20_phy_init(struct phy *phy)
+{
+ struct da8xx_usbphy *d_phy = phy_get_drvdata(phy);
+ int ret;
+ u32 val;
+
+ ret = clk_prepare_enable(d_phy->usb20_clk);
+ if (ret)
+ return ret;
+
+ val = da8xx_usbphy_readl(d_phy->phy_ctrl);
+ val &= ~OTGPWRDN;
+ da8xx_usbphy_writel(d_phy->phy_ctrl, val);
+
+ return 0;
+}
+
+static int da8xx_usb20_phy_shutdown(struct phy *phy)
+{
+ struct da8xx_usbphy *d_phy = phy_get_drvdata(phy);
+ u32 val;
+
+ val = da8xx_usbphy_readl(d_phy->phy_ctrl);
+ val |= OTGPWRDN;
+ da8xx_usbphy_writel(d_phy->phy_ctrl, val);
+
+ clk_disable_unprepare(d_phy->usb20_clk);
+
+ return 0;
+}
+
+static const struct phy_ops da8xx_usb20_phy_ops = {
+ .power_on = da8xx_usb20_phy_init,
+ .power_off = da8xx_usb20_phy_shutdown,
+ .owner = THIS_MODULE,
+};
+
+int da8xx_usb20_phy_set_mode(struct phy *phy, enum musb_mode mode)
+{
+ struct da8xx_usbphy *d_phy = phy_get_drvdata(phy);
+ u32 val;
+
+ val = da8xx_usbphy_readl(d_phy->phy_ctrl);
+
+ val &= ~OTGMODE_MASK;
+ switch (mode) {
+ case MUSB_HOST: /* Force VBUS valid, ID = 0 */
+ val |= FORCE_HOST;
+ break;
+ case MUSB_PERIPHERAL: /* Force VBUS valid, ID = 1 */
+ val |= FORCE_DEVICE;
+ break;
+ case MUSB_OTG: /* Don't override the VBUS/ID comparators */
+ val |= NO_OVERRIDE;
+ break;
+ default:
+ return -EINVAL;
+ }
+
+ da8xx_usbphy_writel(d_phy->phy_ctrl, val);
+
+ return 0;
+}
+EXPORT_SYMBOL_GPL(da8xx_usb20_phy_set_mode);
+
+static struct phy *da8xx_usbphy_of_xlate(struct device *dev,
+ struct of_phandle_args *args)
+{
+ struct da8xx_usbphy *d_phy = dev_get_drvdata(dev);
+
+ if (!d_phy)
+ return ERR_PTR(-ENODEV);
+
+ switch (args->args[0]) {
+ case 1:
+ return d_phy->usb11_phy;
+ case 2:
+ return d_phy->usb20_phy;
+ default:
+ return ERR_PTR(-EINVAL);
+ }
+}
+
+static int da8xx_usbphy_probe(struct platform_device *pdev)
+{
+ struct device *dev = &pdev->dev;
+ struct device_node *node = dev->of_node;
+ struct da8xx_usbphy *d_phy;
+ struct resource *res;
+
+ d_phy = devm_kzalloc(dev, sizeof(*d_phy), GFP_KERNEL);
+ if (!d_phy)
+ return -ENOMEM;
+
+ res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+ d_phy->phy_ctrl = devm_ioremap_resource(dev, res);
+ if (IS_ERR(d_phy->phy_ctrl)) {
+ dev_err(dev, "Failed to map resource.\n");
+ return PTR_ERR(d_phy->phy_ctrl);
+ }
+
+ d_phy->usb11_clk = devm_clk_get(dev, "usb11_phy");
+ if (IS_ERR(d_phy->usb11_clk)) {
+ dev_err(dev, "Failed to get usb11_phy clock.\n");
+ return PTR_ERR(d_phy->usb11_clk);
+ }
+
+ d_phy->usb20_clk = devm_clk_get(dev, "usb20_phy");
+ if (IS_ERR(d_phy->usb20_clk)) {
+ dev_err(dev, "Failed to get usb20_phy clock.\n");
+ return PTR_ERR(d_phy->usb20_clk);
+ }
+
+ d_phy->usb11_phy = devm_phy_create(dev, node, &da8xx_usb11_phy_ops);
+ if (IS_ERR(d_phy->usb11_phy)) {
+ dev_err(dev, "Failed to create usb11 phy.\n");
+ return PTR_ERR(d_phy->usb11_phy);
+ }
+
+ d_phy->usb20_phy = devm_phy_create(dev, node, &da8xx_usb20_phy_ops);
+ if (IS_ERR(d_phy->usb20_phy)) {
+ dev_err(dev, "Failed to create usb20 phy.\n");
+ return PTR_ERR(d_phy->usb20_phy);
+ }
+
+ platform_set_drvdata(pdev, d_phy);
+ phy_set_drvdata(d_phy->usb11_phy, d_phy);
+ phy_set_drvdata(d_phy->usb20_phy, d_phy);
+
+ if (node) {
+ d_phy->phy_provider = devm_of_phy_provider_register(dev,
+ da8xx_usbphy_of_xlate);
+ if (IS_ERR(d_phy->phy_provider)) {
+ dev_err(dev, "Failed to create phy provider.\n");
+ return PTR_ERR(d_phy->phy_provider);
+ }
+ } else {
+ int ret;
+
+ ret = phy_create_lookup(d_phy->usb11_phy, "usbphy", "ohci.0");
+ if (ret)
+ dev_warn(dev, "Failed to create usb11 phy lookup .\n");
+ ret = phy_create_lookup(d_phy->usb20_phy, "usbphy", "musb-da8xx");
+ if (ret)
+ dev_warn(dev, "Failed to create usb20 phy lookup .\n");
+ }
+
+ return 0;
+}
+
+static int da8xx_usbphy_remove(struct platform_device *pdev)
+{
+ struct da8xx_usbphy *d_phy = platform_get_drvdata(pdev);
+
+ if (!pdev->dev.of_node) {
+ phy_remove_lookup(d_phy->usb20_phy, "usbphy", "musb-da8xx");
+ phy_remove_lookup(d_phy->usb11_phy, "usbphy", "ohci.0");
+ }
+
+ return 0;
+}
+
+static const struct of_device_id da8xx_usbphy_ids[] = {
+ { .compatible = "ti,da830-usbphy" },
+ { }
+};
+MODULE_DEVICE_TABLE(of, da8xx_usbphy_ids);
+
+static struct platform_driver da8xx_usbphy_driver = {
+ .probe = da8xx_usbphy_probe,
+ .remove = da8xx_usbphy_remove,
+ .driver = {
+ .name = "da8xx-usbphy",
+ .of_match_table = da8xx_usbphy_ids,
+ },
+};
+
+module_platform_driver(da8xx_usbphy_driver);
+
+MODULE_ALIAS("platform:da8xx-usbphy");
+MODULE_AUTHOR("David Lechner <[email protected]>");
+MODULE_DESCRIPTION("TI DA8XX USB PHY driver");
+MODULE_LICENSE("GPL v2");
--
1.9.1

2016-03-17 03:39:50

[permalink] [raw]

Subject: [PATCH v2 08/11] ARM: dt: da850: Add usb phy node

Add a node for the new usb phy driver.

Signed-off-by: David Lechner <[email protected]>
---

v2 changes: This is new patch in v2.

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

diff --git a/arch/arm/boot/dts/da850.dtsi b/arch/arm/boot/dts/da850.dtsi
index 591660d..06f36cd 100644
--- a/arch/arm/boot/dts/da850.dtsi
+++ b/arch/arm/boot/dts/da850.dtsi
@@ -324,6 +324,12 @@
>;
status = "disabled";
};
+ usbphy: usbphy@1c14184 {
+ compatible = "ti,da830-usbphy";
+ #phy-cells = <1>;
+ reg = <0x14184 4>;
+ status = "disabled";
+ };
gpio: gpio@1e26000 {
compatible = "ti,dm6441-gpio";
gpio-controller;
--
1.9.1

2016-03-17 03:40:13

[permalink] [raw]

Subject: [PATCH v2 02/11] ARM: davinci: add set_parent callback for mux clocks

Introduce a set_parent callback that will be used for mux clocks, such as
the USB PHY muxes and the async3 clock domain mux.

Signed-off-by: David Lechner <[email protected]>
---

v2 changes: This is a new patch in v2.

arch/arm/mach-davinci/clock.c | 17 ++++++++++++++++-
arch/arm/mach-davinci/clock.h | 1 +
2 files changed, 17 insertions(+), 1 deletion(-)

diff --git a/arch/arm/mach-davinci/clock.c b/arch/arm/mach-davinci/clock.c
index 3424eac6..3143e8b 100644
--- a/arch/arm/mach-davinci/clock.c
+++ b/arch/arm/mach-davinci/clock.c
@@ -195,6 +195,13 @@ int clk_set_parent(struct clk *clk, struct clk *parent)
return -EINVAL;

mutex_lock(&clocks_mutex);
+ if (clk->set_parent) {
+ int ret = clk->set_parent(clk, parent);
+ if (ret) {
+ mutex_unlock(&clocks_mutex);
+ return ret;
+ }
+ }
clk->parent = parent;
list_del_init(&clk->childnode);
list_add(&clk->childnode, &clk->parent->children);
@@ -224,8 +231,16 @@ int clk_register(struct clk *clk)

mutex_lock(&clocks_mutex);
list_add_tail(&clk->node, &clocks);
- if (clk->parent)
+ if (clk->parent) {
+ if (clk->set_parent) {
+ int ret = clk->set_parent(clk, clk->parent);
+ if (ret) {
+ mutex_unlock(&clocks_mutex);
+ return ret;
+ }
+ }
list_add_tail(&clk->childnode, &clk->parent->children);
+ }
mutex_unlock(&clocks_mutex);

/* If rate is already set, use it */
diff --git a/arch/arm/mach-davinci/clock.h b/arch/arm/mach-davinci/clock.h
index 1e4e836..e2a5437 100644
--- a/arch/arm/mach-davinci/clock.h
+++ b/arch/arm/mach-davinci/clock.h
@@ -106,6 +106,7 @@ struct clk {
int (*reset) (struct clk *clk, bool reset);
void (*clk_enable) (struct clk *clk);
void (*clk_disable) (struct clk *clk);
+ int (*set_parent) (struct clk *clk, struct clk *parent);
};

/* Clock flags: SoC-specific flags start at BIT(16) */
--
1.9.1

2016-03-17 03:40:36

[permalink] [raw]

Subject: [PATCH v2 03/11] ARM: davinci: da850: use clk->set_parent for async3

The da850 family of processors has an async3 clock domain that can be
muxed to either pll0_sysclk2 or pll1_sysclk2. Now that the davinci clocks
have a set_parent callback, we can use this to control the async3 mux
instead of a stand-alone function.

This adds a new async3_clk and sets the appropriate child clocks. The
default is use to pll1_sysclk2 since it is not affected by processor
frequency scaling.

Signed-off-by: David Lechner <[email protected]>
---

v2 changes: This is a new patch in v2.

arch/arm/mach-davinci/da850.c | 88 ++++++++++++++++++++-----------------------
1 file changed, 40 insertions(+), 48 deletions(-)

diff --git a/arch/arm/mach-davinci/da850.c b/arch/arm/mach-davinci/da850.c
index 97d8779..8c8f31e 100644
--- a/arch/arm/mach-davinci/da850.c
+++ b/arch/arm/mach-davinci/da850.c
@@ -34,9 +34,6 @@
#include "clock.h"
#include "mux.h"

-/* SoC specific clock flags */
-#define DA850_CLK_ASYNC3 BIT(16)
-
#define DA850_PLL1_BASE 0x01e1a000
#define DA850_TIMER64P2_BASE 0x01f0c000
#define DA850_TIMER64P3_BASE 0x01f0d000
@@ -161,6 +158,39 @@ static struct clk pll1_sysclk3 = {
.div_reg = PLLDIV3,
};

+static int da850_async3_set_parent(struct clk *clk, struct clk *parent)
+{
+ u32 __iomem *cfgchip3;
+ u32 val;
+
+ /*
+ * Can't use DA8XX_SYSCFG0_VIRT() here since this can be called before
+ * da8xx_syscfg0_base is initialized.
+ */
+ cfgchip3 = ioremap(DA8XX_SYSCFG0_BASE + DA8XX_CFGCHIP3_REG, 4);
+ val = readl(cfgchip3);
+
+ /* Set the USB 1.1 PHY clock mux based on the parent clock. */
+ if (parent == &pll0_sysclk2)
+ val &= ~CFGCHIP3_ASYNC3_CLKSRC;
+ else if (parent == &pll1_sysclk2)
+ val |= CFGCHIP3_ASYNC3_CLKSRC;
+ else {
+ pr_err("Bad parent on async3 clock mux.\n");
+ return -EINVAL;
+ }
+
+ writel(val, cfgchip3);
+
+ return 0;
+}
+
+static struct clk async3_clk = {
+ .name = "async3",
+ .parent = &pll1_sysclk2,
+ .set_parent = da850_async3_set_parent,
+};
+
static struct clk i2c0_clk = {
.name = "i2c0",
.parent = &pll0_aux_clk,
@@ -234,18 +264,16 @@ static struct clk uart0_clk = {

static struct clk uart1_clk = {
.name = "uart1",
- .parent = &pll0_sysclk2,
+ .parent = &async3_clk,
.lpsc = DA8XX_LPSC1_UART1,
.gpsc = 1,
- .flags = DA850_CLK_ASYNC3,
};

static struct clk uart2_clk = {
.name = "uart2",
- .parent = &pll0_sysclk2,
+ .parent = &async3_clk,
.lpsc = DA8XX_LPSC1_UART2,
.gpsc = 1,
- .flags = DA850_CLK_ASYNC3,
};

static struct clk aintc_clk = {
@@ -300,10 +328,9 @@ static struct clk emac_clk = {

static struct clk mcasp_clk = {
.name = "mcasp",
- .parent = &pll0_sysclk2,
+ .parent = &async3_clk,
.lpsc = DA8XX_LPSC1_McASP0,
.gpsc = 1,
- .flags = DA850_CLK_ASYNC3,
};

static struct clk lcdc_clk = {
@@ -355,10 +382,9 @@ static struct clk spi0_clk = {

static struct clk spi1_clk = {
.name = "spi1",
- .parent = &pll0_sysclk2,
+ .parent = &async3_clk,
.lpsc = DA8XX_LPSC1_SPI1,
.gpsc = 1,
- .flags = DA850_CLK_ASYNC3,
};

static struct clk vpif_clk = {
@@ -386,10 +412,9 @@ static struct clk dsp_clk = {

static struct clk ehrpwm_clk = {
.name = "ehrpwm",
- .parent = &pll0_sysclk2,
+ .parent = &async3_clk,
.lpsc = DA8XX_LPSC1_PWM,
.gpsc = 1,
- .flags = DA850_CLK_ASYNC3,
};

#define DA8XX_EHRPWM_TBCLKSYNC BIT(12)
@@ -421,10 +446,9 @@ static struct clk ehrpwm_tbclk = {

static struct clk ecap_clk = {
.name = "ecap",
- .parent = &pll0_sysclk2,
+ .parent = &async3_clk,
.lpsc = DA8XX_LPSC1_ECAP,
.gpsc = 1,
- .flags = DA850_CLK_ASYNC3,
};

static struct clk_lookup da850_clks[] = {
@@ -442,6 +466,7 @@ static struct clk_lookup da850_clks[] = {
CLK(NULL, "pll1_aux", &pll1_aux_clk),
CLK(NULL, "pll1_sysclk2", &pll1_sysclk2),
CLK(NULL, "pll1_sysclk3", &pll1_sysclk3),
+ CLK(NULL, "async3", &async3_clk),
CLK("i2c_davinci.1", NULL, &i2c0_clk),
CLK(NULL, "timer0", &timerp64_0_clk),
CLK("davinci-wdt", NULL, &timerp64_1_clk),
@@ -909,30 +934,6 @@ static struct davinci_timer_info da850_timer_info = {
.clocksource_id = T0_TOP,
};

-static void da850_set_async3_src(int pllnum)
-{
- struct clk *clk, *newparent = pllnum ? &pll1_sysclk2 : &pll0_sysclk2;
- struct clk_lookup *c;
- unsigned int v;
- int ret;
-
- for (c = da850_clks; c->clk; c++) {
- clk = c->clk;
- if (clk->flags & DA850_CLK_ASYNC3) {
- ret = clk_set_parent(clk, newparent);
- WARN(ret, "DA850: unable to re-parent clock %s",
- clk->name);
- }
- }
-
- v = __raw_readl(DA8XX_SYSCFG0_VIRT(DA8XX_CFGCHIP3_REG));
- if (pllnum)
- v |= CFGCHIP3_ASYNC3_CLKSRC;
- else
- v &= ~CFGCHIP3_ASYNC3_CLKSRC;
- __raw_writel(v, DA8XX_SYSCFG0_VIRT(DA8XX_CFGCHIP3_REG));
-}
-
#ifdef CONFIG_CPU_FREQ
/*
* Notes:
@@ -1328,15 +1329,6 @@ void __init da850_init(void)
if (WARN(!da8xx_syscfg1_base, "Unable to map syscfg1 module"))
return;

- /*
- * Move the clock source of Async3 domain to PLL1 SYSCLK2.
- * This helps keeping the peripherals on this domain insulated
- * from CPU frequency changes caused by DVFS. The firmware sets
- * both PLL0 and PLL1 to the same frequency so, there should not
- * be any noticeable change even in non-DVFS use cases.
- */
- da850_set_async3_src(1);
-
/* Unlock writing to PLL0 registers */
v = __raw_readl(DA8XX_SYSCFG0_VIRT(DA8XX_CFGCHIP0_REG));
v &= ~CFGCHIP0_PLL_MASTER_LOCK;
--
1.9.1

2016-03-17 03:41:04

[permalink] [raw]

Subject: [PATCH v2 09/11] usb: ohci-da8xx: Remove code that references mach

Including mach/* is frowned upon in device drivers, so get rid of it.

This replaces usb20_clk and code that pokes CFGCHIP2 with a proper phy
driver.

Signed-off-by: David Lechner <[email protected]>
---

v2 changes: Uses the new phy driver instead of using a second clock.

drivers/usb/host/ohci-da8xx.c | 90 +++++++++++++++++++++----------------------
1 file changed, 44 insertions(+), 46 deletions(-)

diff --git a/drivers/usb/host/ohci-da8xx.c b/drivers/usb/host/ohci-da8xx.c
index e5c33bc..c648674 100644
--- a/drivers/usb/host/ohci-da8xx.c
+++ b/drivers/usb/host/ohci-da8xx.c
@@ -15,58 +15,40 @@
#include <linux/jiffies.h>
#include <linux/platform_device.h>
#include <linux/clk.h>
-
-#include <mach/da8xx.h>
+#include <linux/phy/phy.h>
#include <linux/platform_data/usb-davinci.h>

#ifndef CONFIG_ARCH_DAVINCI_DA8XX
#error "This file is DA8xx bus glue. Define CONFIG_ARCH_DAVINCI_DA8XX."
#endif

-#define CFGCHIP2 DA8XX_SYSCFG0_VIRT(DA8XX_CFGCHIP2_REG)
-
static struct clk *usb11_clk;
-static struct clk *usb20_clk;
+static struct phy *usb11_phy;

/* Over-current indicator change bitmask */
static volatile u16 ocic_mask;

-static void ohci_da8xx_clock(int on)
+static int ohci_da8xx_enable(void)
{
- u32 cfgchip2;
-
- cfgchip2 = __raw_readl(CFGCHIP2);
- if (on) {
- clk_enable(usb11_clk);
-
- /*
- * If USB 1.1 reference clock is sourced from USB 2.0 PHY, we
- * need to enable the USB 2.0 module clocking, start its PHY,
- * and not allow it to stop the clock during USB 2.0 suspend.
- */
- if (!(cfgchip2 & CFGCHIP2_USB1PHYCLKMUX)) {
- clk_enable(usb20_clk);
-
- cfgchip2 &= ~(CFGCHIP2_RESET | CFGCHIP2_PHYPWRDN);
- cfgchip2 |= CFGCHIP2_PHY_PLLON;
- __raw_writel(cfgchip2, CFGCHIP2);
-
- pr_info("Waiting for USB PHY clock good...\n");
- while (!(__raw_readl(CFGCHIP2) & CFGCHIP2_PHYCLKGD))
- cpu_relax();
- }
+ int ret;

- /* Enable USB 1.1 PHY */
- cfgchip2 |= CFGCHIP2_USB1SUSPENDM;
- } else {
- clk_disable(usb11_clk);
- if (!(cfgchip2 & CFGCHIP2_USB1PHYCLKMUX))
- clk_disable(usb20_clk);
+ ret = clk_prepare_enable(usb11_clk);
+ if (ret)
+ return ret;

- /* Disable USB 1.1 PHY */
- cfgchip2 &= ~CFGCHIP2_USB1SUSPENDM;
+ ret = phy_power_on(usb11_phy);
+ if (ret) {
+ clk_disable_unprepare(usb11_clk);
+ return ret;
}
- __raw_writel(cfgchip2, CFGCHIP2);
+
+ return 0;
+}
+
+static void ohci_da8xx_disable(void)
+{
+ phy_power_off(usb11_phy);
+ clk_disable_unprepare(usb11_clk);
}

/*
@@ -92,7 +74,9 @@ static int ohci_da8xx_init(struct usb_hcd *hcd)

dev_dbg(dev, "starting USB controller\n");

- ohci_da8xx_clock(1);
+ result = ohci_da8xx_enable();
+ if (result < 0)
+ return result;

/*
* DA8xx only have 1 port connected to the pins but the HC root hub
@@ -101,8 +85,10 @@ static int ohci_da8xx_init(struct usb_hcd *hcd)
ohci->num_ports = 1;

result = ohci_init(ohci);
- if (result < 0)
+ if (result < 0) {
+ ohci_da8xx_disable();
return result;
+ }

/*
* Since we're providing a board-specific root hub port power control
@@ -129,7 +115,7 @@ static int ohci_da8xx_init(struct usb_hcd *hcd)
static void ohci_da8xx_stop(struct usb_hcd *hcd)
{
ohci_stop(hcd);
- ohci_da8xx_clock(0);
+ ohci_da8xx_disable();
}

static int ohci_da8xx_start(struct usb_hcd *hcd)
@@ -301,12 +287,18 @@ static int usb_hcd_da8xx_probe(const struct hc_driver *driver,
return -ENODEV;

usb11_clk = devm_clk_get(&pdev->dev, "usb11");
- if (IS_ERR(usb11_clk))
+ if (IS_ERR(usb11_clk)) {
+ if (PTR_ERR(usb11_clk) != -EPROBE_DEFER)
+ dev_err(&pdev->dev, "Failed to get clock.\n");
return PTR_ERR(usb11_clk);
+ }

- usb20_clk = devm_clk_get(&pdev->dev, "usb20");
- if (IS_ERR(usb20_clk))
- return PTR_ERR(usb20_clk);
+ usb11_phy = devm_phy_get(&pdev->dev, "usbphy");
+ if (IS_ERR(usb11_phy)) {
+ if (PTR_ERR(usb11_phy) != -EPROBE_DEFER)
+ dev_err(&pdev->dev, "Failed to get phy.\n");
+ return PTR_ERR(usb11_phy);
+ }

hcd = usb_create_hcd(driver, &pdev->dev, dev_name(&pdev->dev));
if (!hcd)
@@ -316,6 +308,7 @@ static int usb_hcd_da8xx_probe(const struct hc_driver *driver,
hcd->regs = devm_ioremap_resource(&pdev->dev, mem);
if (IS_ERR(hcd->regs)) {
error = PTR_ERR(hcd->regs);
+ dev_err(&pdev->dev, "failed to map ohci.\n");
goto err;
}
hcd->rsrc_start = mem->start;
@@ -397,7 +390,7 @@ static int ohci_da8xx_suspend(struct platform_device *pdev,
if (ret)
return ret;

- ohci_da8xx_clock(0);
+ ohci_da8xx_disable();
hcd->state = HC_STATE_SUSPENDED;

return ret;
@@ -407,14 +400,19 @@ static int ohci_da8xx_resume(struct platform_device *dev)
{
struct usb_hcd *hcd = platform_get_drvdata(dev);
struct ohci_hcd *ohci = hcd_to_ohci(hcd);
+ int ret;

if (time_before(jiffies, ohci->next_statechange))
msleep(5);
ohci->next_statechange = jiffies;

- ohci_da8xx_clock(1);
+ ret = ohci_da8xx_enable();
+ if (ret)
+ return ret;
+
dev->dev.power.power_state = PMSG_ON;
usb_hcd_resume_root_hub(hcd);
+
return 0;
}
#endif
--
1.9.1

2016-03-17 03:41:33

[permalink] [raw]

Subject: [PATCH v2 01/11] ARM: davinci: defined missing CFGCHIP2_REFFREQ_* macros for MUSB PHY

From: Petr Kulhavy <[email protected]>

Only few MUSB PHY reference clock frequencies were defined.
This patch defines macros for the missing frequencies:
19.2MHz, 38.4MHz, 13MHz, 26MHz, 20MHz, 40MHz

Signed-off-by: Petr Kulhavy <[email protected]>.
Acked-by: Sergei Shtylyov <[email protected]>
Signed-off-by: Bin Liu <[email protected]>
---

v2 changes: None.

include/linux/platform_data/usb-davinci.h | 6 ++++++
1 file changed, 6 insertions(+)

diff --git a/include/linux/platform_data/usb-davinci.h b/include/linux/platform_data/usb-davinci.h
index e0bc4ab..961d3dc 100644
--- a/include/linux/platform_data/usb-davinci.h
+++ b/include/linux/platform_data/usb-davinci.h
@@ -33,6 +33,12 @@
#define CFGCHIP2_REFFREQ_12MHZ (1 << 0)
#define CFGCHIP2_REFFREQ_24MHZ (2 << 0)
#define CFGCHIP2_REFFREQ_48MHZ (3 << 0)
+#define CFGCHIP2_REFFREQ_19_2MHZ (4 << 0)
+#define CFGCHIP2_REFFREQ_38_4MHZ (5 << 0)
+#define CFGCHIP2_REFFREQ_13MHZ (6 << 0)
+#define CFGCHIP2_REFFREQ_26MHZ (7 << 0)
+#define CFGCHIP2_REFFREQ_20MHZ (8 << 0)
+#define CFGCHIP2_REFFREQ_40MHZ (9 << 0)

struct da8xx_ohci_root_hub;

--
1.9.1

2016-03-17 03:41:59

[permalink] [raw]

Subject: [PATCH v2 07/11] ARM: davinci: da8xx: Add USB PHY platform declaration

There is now a proper phy driver for the DA8XX SoC USB PHY. This adds the
platform device declarations needed to use it.

Signed-off-by: David Lechner <[email protected]>
---

v2 changes: This is new patch in v2.

arch/arm/mach-davinci/include/mach/da8xx.h | 1 +
arch/arm/mach-davinci/usb.c | 24 +++++++++++++++++++++++-
2 files changed, 24 insertions(+), 1 deletion(-)

diff --git a/arch/arm/mach-davinci/include/mach/da8xx.h b/arch/arm/mach-davinci/include/mach/da8xx.h
index f9f9713..049d26f 100644
--- a/arch/arm/mach-davinci/include/mach/da8xx.h
+++ b/arch/arm/mach-davinci/include/mach/da8xx.h
@@ -88,6 +88,7 @@ int da850_register_edma(struct edma_rsv_info *rsv[2]);
int da8xx_register_i2c(int instance, struct davinci_i2c_platform_data *pdata);
int da8xx_register_spi_bus(int instance, unsigned num_chipselect);
int da8xx_register_watchdog(void);
+int da8xx_register_usbphy(void);
int da8xx_register_usb20(unsigned mA, unsigned potpgt);
int da8xx_register_usb11(struct da8xx_ohci_root_hub *pdata);
int da8xx_register_emac(void);
diff --git a/arch/arm/mach-davinci/usb.c b/arch/arm/mach-davinci/usb.c
index b0a6b52..6a0802e 100644
--- a/arch/arm/mach-davinci/usb.c
+++ b/arch/arm/mach-davinci/usb.c
@@ -4,7 +4,7 @@
#include <linux/init.h>
#include <linux/platform_device.h>
#include <linux/dma-mapping.h>
-
+#include <linux/phy/phy.h>
#include <linux/usb/musb.h>

#include <mach/common.h>
@@ -18,6 +18,28 @@
#define DA8XX_USB0_BASE 0x01e00000
#define DA8XX_USB1_BASE 0x01e25000

+#ifdef CONFIG_ARCH_DAVINCI_DA8XX
+static struct resource da8xx_usbphy_resources[] = {
+ [0] = {
+ .start = DA8XX_SYSCFG0_BASE + DA8XX_CFGCHIP2_REG,
+ .end = DA8XX_SYSCFG0_BASE + DA8XX_CFGCHIP2_REG + 4 - 1,
+ .flags = IORESOURCE_MEM,
+ },
+};
+
+static struct platform_device da8xx_usbphy = {
+ .name = "da8xx-usbphy",
+ .id = 0,
+ .num_resources = ARRAY_SIZE(da8xx_usbphy_resources),
+ .resource = da8xx_usbphy_resources,
+};
+
+int __init da8xx_register_usbphy(void)
+{
+ return platform_device_register(&da8xx_usbphy);
+}
+#endif /* CONFIG_ARCH_DAVINCI_DA8XX */
+
#if IS_ENABLED(CONFIG_USB_MUSB_HDRC)
static struct musb_hdrc_eps_bits musb_eps[] = {
{ "ep1_tx", 8, },
--
1.9.1

2016-03-17 03:42:36

[permalink] [raw]

Subject: [PATCH v2 10/11] usb: musb: da8xx: Use devm in probe

Simplify things a bit by using devm functions where possible.

Signed-off-by: David Lechner <[email protected]>
---

v2 changes: This is part of a previous patch that was split. No changes from
previous version.

drivers/usb/musb/da8xx.c | 28 ++++++++--------------------
1 file changed, 8 insertions(+), 20 deletions(-)

diff --git a/drivers/usb/musb/da8xx.c b/drivers/usb/musb/da8xx.c
index b03d3b8..50f07a5 100644
--- a/drivers/usb/musb/da8xx.c
+++ b/drivers/usb/musb/da8xx.c
@@ -489,32 +489,27 @@ static int da8xx_probe(struct platform_device *pdev)
struct platform_device *musb;
struct da8xx_glue *glue;
struct platform_device_info pinfo;
- struct clk *clk;
+ int ret;

- int ret = -ENOMEM;
-
- glue = kzalloc(sizeof(*glue), GFP_KERNEL);
+ glue = devm_kzalloc(&pdev->dev, sizeof(*glue), GFP_KERNEL);
if (!glue) {
dev_err(&pdev->dev, "failed to allocate glue context\n");
- goto err0;
+ return -ENOMEM;
}

- clk = clk_get(&pdev->dev, "usb20");
- if (IS_ERR(clk)) {
+ glue->clk = devm_clk_get(&pdev->dev, "usb20");
+ if (IS_ERR(glue->clk)) {
dev_err(&pdev->dev, "failed to get clock\n");
- ret = PTR_ERR(clk);
- goto err3;
+ return PTR_ERR(glue->clk);
}

- ret = clk_enable(clk);
+ ret = clk_enable(glue->clk);
if (ret) {
dev_err(&pdev->dev, "failed to enable clock\n");
goto err4;
}

glue->dev = &pdev->dev;
- glue->clk = clk;
-
pdata->platform_ops = &da8xx_ops;

glue->phy = usb_phy_generic_register();
@@ -557,15 +552,10 @@ err6:
usb_phy_generic_unregister(glue->phy);

err5:
- clk_disable(clk);
+ clk_disable(glue->clk);

err4:
- clk_put(clk);
-
-err3:
- kfree(glue);

-err0:
return ret;
}

@@ -576,8 +566,6 @@ static int da8xx_remove(struct platform_device *pdev)
platform_device_unregister(glue->musb);
usb_phy_generic_unregister(glue->phy);
clk_disable(glue->clk);
- clk_put(glue->clk);
- kfree(glue);

return 0;
}
--
1.9.1

2016-03-17 03:43:15

[permalink] [raw]

Subject: [PATCH v2 04/11] ARM: davinci: da8xx: add usb phy clocks

Up to this point, the USB phy clock configuration was handled manually in
the board files and in the usb drivers. This adds proper clocks so that
the usb drivers can use clk_get and clk_enable and not have to worry about
the details. Also, the related code is removed from the board files.

Signed-off-by: David Lechner <[email protected]>
---

v2 changes: Move clock mux code to set_parent callback. Also fixed some other
issues from feedback on the previous patch.

arch/arm/mach-davinci/board-da830-evm.c | 12 ---
arch/arm/mach-davinci/board-omapl138-hawk.c | 7 --
arch/arm/mach-davinci/da830.c | 143 ++++++++++++++++++++++++++++
arch/arm/mach-davinci/da850.c | 143 ++++++++++++++++++++++++++++
4 files changed, 286 insertions(+), 19 deletions(-)

diff --git a/arch/arm/mach-davinci/board-da830-evm.c b/arch/arm/mach-davinci/board-da830-evm.c
index 3d8cf8c..f3a8cc9 100644
--- a/arch/arm/mach-davinci/board-da830-evm.c
+++ b/arch/arm/mach-davinci/board-da830-evm.c
@@ -115,18 +115,6 @@ static __init void da830_evm_usb_init(void)
*/
cfgchip2 = __raw_readl(DA8XX_SYSCFG0_VIRT(DA8XX_CFGCHIP2_REG));

- /* USB2.0 PHY reference clock is 24 MHz */
- cfgchip2 &= ~CFGCHIP2_REFFREQ;
- cfgchip2 |= CFGCHIP2_REFFREQ_24MHZ;
-
- /*
- * Select internal reference clock for USB 2.0 PHY
- * and use it as a clock source for USB 1.1 PHY
- * (this is the default setting anyway).
- */
- cfgchip2 &= ~CFGCHIP2_USB1PHYCLKMUX;
- cfgchip2 |= CFGCHIP2_USB2PHYCLKMUX;
-
/*
* We have to override VBUS/ID signals when MUSB is configured into the
* host-only mode -- ID pin will float if no cable is connected, so the
diff --git a/arch/arm/mach-davinci/board-omapl138-hawk.c b/arch/arm/mach-davinci/board-omapl138-hawk.c
index ee62486..d27e753 100644
--- a/arch/arm/mach-davinci/board-omapl138-hawk.c
+++ b/arch/arm/mach-davinci/board-omapl138-hawk.c
@@ -251,13 +251,6 @@ static __init void omapl138_hawk_usb_init(void)
return;
}

- /* Setup the Ref. clock frequency for the HAWK at 24 MHz. */
-
- cfgchip2 = __raw_readl(DA8XX_SYSCFG0_VIRT(DA8XX_CFGCHIP2_REG));
- cfgchip2 &= ~CFGCHIP2_REFFREQ;
- cfgchip2 |= CFGCHIP2_REFFREQ_24MHZ;
- __raw_writel(cfgchip2, DA8XX_SYSCFG0_VIRT(DA8XX_CFGCHIP2_REG));
-
ret = gpio_request_one(DA850_USB1_VBUS_PIN,
GPIOF_DIR_OUT, "USB1 VBUS");
if (ret < 0) {
diff --git a/arch/arm/mach-davinci/da830.c b/arch/arm/mach-davinci/da830.c
index 7187e7f..ee942b0 100644
--- a/arch/arm/mach-davinci/da830.c
+++ b/arch/arm/mach-davinci/da830.c
@@ -12,6 +12,7 @@
#include <linux/init.h>
#include <linux/clk.h>
#include <linux/platform_data/gpio-davinci.h>
+#include <linux/platform_data/usb-davinci.h>

#include <asm/mach/map.h>

@@ -346,6 +347,12 @@ static struct clk i2c1_clk = {
.gpsc = 1,
};

+static struct clk usb_ref_clk = {
+ .name = "usb_ref_clk",
+ .rate = 48000000,
+ .set_rate = davinci_simple_set_rate,
+};
+
static struct clk usb11_clk = {
.name = "usb11",
.parent = &pll0_sysclk4,
@@ -353,6 +360,139 @@ static struct clk usb11_clk = {
.gpsc = 1,
};

+static void usb20_phy_clk_enable(struct clk *clk)
+{
+ u32 val;
+
+ val = readl(DA8XX_SYSCFG0_VIRT(DA8XX_CFGCHIP2_REG));
+
+ /*
+ * Turn on the USB 2.0 PHY, but just the PLL, and not OTG. The USB 1.1
+ * host may use the PLL clock without USB 2.0 OTG being used.
+ */
+ val &= ~(CFGCHIP2_RESET | CFGCHIP2_PHYPWRDN);
+ val |= CFGCHIP2_PHY_PLLON;
+
+ writel(val, DA8XX_SYSCFG0_VIRT(DA8XX_CFGCHIP2_REG));
+
+ pr_info("Waiting for USB 2.0 PHY clock good...\n");
+ while (!(readl(DA8XX_SYSCFG0_VIRT(DA8XX_CFGCHIP2_REG))
+ & CFGCHIP2_PHYCLKGD))
+ cpu_relax();
+}
+
+static void usb20_phy_clk_disable(struct clk *clk)
+{
+ u32 val;
+
+ val = readl(DA8XX_SYSCFG0_VIRT(DA8XX_CFGCHIP2_REG));
+ val |= CFGCHIP2_PHYPWRDN;
+ writel(val, DA8XX_SYSCFG0_VIRT(DA8XX_CFGCHIP2_REG));
+}
+
+static int usb20_phy_clk_set_parent(struct clk *clk, struct clk *parent)
+{
+ u32 __iomem *cfgchip2;
+ u32 val;
+
+ /*
+ * Can't use DA8XX_SYSCFG0_VIRT() here since this can be called before
+ * da8xx_syscfg0_base is initialized.
+ */
+ cfgchip2 = ioremap(DA8XX_SYSCFG0_BASE + DA8XX_CFGCHIP2_REG, 4);
+ val = readl(cfgchip2);
+
+ /* Set the mux depending on the parent clock. */
+ if (parent == &pll0_aux_clk)
+ val |= CFGCHIP2_USB2PHYCLKMUX;
+ else if (parent == &usb_ref_clk)
+ val &= ~CFGCHIP2_USB2PHYCLKMUX;
+ else {
+ pr_err("Bad parent on USB 2.0 PHY clock.\n");
+ return -EINVAL;
+ }
+
+ /* reference frequency also comes from parent clock */
+ val &= ~CFGCHIP2_REFFREQ;
+ switch (clk_get_rate(parent)) {
+ case 12000000:
+ val |= CFGCHIP2_REFFREQ_12MHZ;
+ break;
+ case 13000000:
+ val |= CFGCHIP2_REFFREQ_13MHZ;
+ break;
+ case 19200000:
+ val |= CFGCHIP2_REFFREQ_19_2MHZ;
+ break;
+ case 20000000:
+ val |= CFGCHIP2_REFFREQ_20MHZ;
+ break;
+ case 24000000:
+ val |= CFGCHIP2_REFFREQ_24MHZ;
+ break;
+ case 26000000:
+ val |= CFGCHIP2_REFFREQ_26MHZ;
+ break;
+ case 38400000:
+ val |= CFGCHIP2_REFFREQ_38_4MHZ;
+ break;
+ case 40000000:
+ val |= CFGCHIP2_REFFREQ_40MHZ;
+ break;
+ case 48000000:
+ val |= CFGCHIP2_REFFREQ_48MHZ;
+ break;
+ default:
+ pr_err("Bad parent clock rate on USB 2.0 PHY clock.\n");
+ return -EINVAL;
+ }
+
+ writel(val, cfgchip2);
+
+ return 0;
+}
+
+static struct clk usb20_phy_clk = {
+ .name = "usb20_phy",
+ .parent = &pll0_aux_clk,
+ .clk_enable = usb20_phy_clk_enable,
+ .clk_disable = usb20_phy_clk_disable,
+ .set_parent = usb20_phy_clk_set_parent,
+};
+
+static int usb11_phy_clk_set_parent(struct clk *clk, struct clk *parent)
+{
+ u32 __iomem *cfgchip2;
+ u32 val;
+
+ /*
+ * Can't use DA8XX_SYSCFG0_VIRT() here since this can be called before
+ * da8xx_syscfg0_base is initialized.
+ */
+ cfgchip2 = ioremap(DA8XX_SYSCFG0_BASE + DA8XX_CFGCHIP2_REG, 4);
+ val = readl(cfgchip2);
+
+ /* Set the USB 1.1 PHY clock mux based on the parent clock. */
+ if (parent == &usb20_phy_clk)
+ val &= ~CFGCHIP2_USB1PHYCLKMUX;
+ else if (parent == &usb_ref_clk)
+ val |= CFGCHIP2_USB1PHYCLKMUX;
+ else {
+ pr_err("Bad parent on USB 1.1 PHY clock.\n");
+ return -EINVAL;
+ }
+
+ writel(val, cfgchip2);
+
+ return 0;
+}
+
+static struct clk usb11_phy_clk = {
+ .name = "usb11_phy",
+ .parent = &usb20_phy_clk,
+ .set_parent = usb11_phy_clk_set_parent,
+};
+
static struct clk emif3_clk = {
.name = "emif3",
.parent = &pll0_sysclk5,
@@ -420,7 +560,10 @@ static struct clk_lookup da830_clks[] = {
CLK("davinci_mdio.0", "fck", &emac_clk),
CLK(NULL, "gpio", &gpio_clk),
CLK("i2c_davinci.2", NULL, &i2c1_clk),
+ CLK(NULL, "usb_ref_clk", &usb_ref_clk),
CLK(NULL, "usb11", &usb11_clk),
+ CLK(NULL, "usb20_phy", &usb20_phy_clk),
+ CLK(NULL, "usb11_phy", &usb11_phy_clk),
CLK(NULL, "emif3", &emif3_clk),
CLK(NULL, "arm", &arm_clk),
CLK(NULL, "rmii", &rmii_clk),
diff --git a/arch/arm/mach-davinci/da850.c b/arch/arm/mach-davinci/da850.c
index 8c8f31e..8089a82 100644
--- a/arch/arm/mach-davinci/da850.c
+++ b/arch/arm/mach-davinci/da850.c
@@ -19,6 +19,7 @@
#include <linux/cpufreq.h>
#include <linux/regulator/consumer.h>
#include <linux/platform_data/gpio-davinci.h>
+#include <linux/platform_data/usb-davinci.h>

#include <asm/mach/map.h>

@@ -360,6 +361,12 @@ static struct clk aemif_clk = {
.flags = ALWAYS_ENABLED,
};

+static struct clk usb_ref_clk = {
+ .name = "usb_ref_clk",
+ .rate = 48000000,
+ .set_rate = davinci_simple_set_rate,
+};
+
static struct clk usb11_clk = {
.name = "usb11",
.parent = &pll0_sysclk4,
@@ -374,6 +381,139 @@ static struct clk usb20_clk = {
.gpsc = 1,
};

+static void usb20_phy_clk_enable(struct clk *clk)
+{
+ u32 val;
+
+ val = readl(DA8XX_SYSCFG0_VIRT(DA8XX_CFGCHIP2_REG));
+
+ /*
+ * Turn on the USB 2.0 PHY, but just the PLL, and not OTG. The USB 1.1
+ * host may use the PLL clock without USB 2.0 OTG being used.
+ */
+ val &= ~(CFGCHIP2_RESET | CFGCHIP2_PHYPWRDN);
+ val |= CFGCHIP2_PHY_PLLON;
+
+ writel(val, DA8XX_SYSCFG0_VIRT(DA8XX_CFGCHIP2_REG));
+
+ pr_info("Waiting for USB 2.0 PHY clock good...\n");
+ while (!(readl(DA8XX_SYSCFG0_VIRT(DA8XX_CFGCHIP2_REG))
+ & CFGCHIP2_PHYCLKGD))
+ cpu_relax();
+}
+
+static void usb20_phy_clk_disable(struct clk *clk)
+{
+ u32 val;
+
+ val = readl(DA8XX_SYSCFG0_VIRT(DA8XX_CFGCHIP2_REG));
+ val |= CFGCHIP2_PHYPWRDN;
+ writel(val, DA8XX_SYSCFG0_VIRT(DA8XX_CFGCHIP2_REG));
+}
+
+static int usb20_phy_clk_set_parent(struct clk *clk, struct clk *parent)
+{
+ u32 __iomem *cfgchip2;
+ u32 val;
+
+ /*
+ * Can't use DA8XX_SYSCFG0_VIRT() here since this can be called before
+ * da8xx_syscfg0_base is initialized.
+ */
+ cfgchip2 = ioremap(DA8XX_SYSCFG0_BASE + DA8XX_CFGCHIP2_REG, 4);
+ val = readl(cfgchip2);
+
+ /* Set the mux depending on the parent clock. */
+ if (parent == &pll0_aux_clk)
+ val |= CFGCHIP2_USB2PHYCLKMUX;
+ else if (parent == &usb_ref_clk)
+ val &= ~CFGCHIP2_USB2PHYCLKMUX;
+ else {
+ pr_err("Bad parent on USB 2.0 PHY clock.\n");
+ return -EINVAL;
+ }
+
+ /* reference frequency also comes from parent clock */
+ val &= ~CFGCHIP2_REFFREQ;
+ switch (clk_get_rate(parent)) {
+ case 12000000:
+ val |= CFGCHIP2_REFFREQ_12MHZ;
+ break;
+ case 13000000:
+ val |= CFGCHIP2_REFFREQ_13MHZ;
+ break;
+ case 19200000:
+ val |= CFGCHIP2_REFFREQ_19_2MHZ;
+ break;
+ case 20000000:
+ val |= CFGCHIP2_REFFREQ_20MHZ;
+ break;
+ case 24000000:
+ val |= CFGCHIP2_REFFREQ_24MHZ;
+ break;
+ case 26000000:
+ val |= CFGCHIP2_REFFREQ_26MHZ;
+ break;
+ case 38400000:
+ val |= CFGCHIP2_REFFREQ_38_4MHZ;
+ break;
+ case 40000000:
+ val |= CFGCHIP2_REFFREQ_40MHZ;
+ break;
+ case 48000000:
+ val |= CFGCHIP2_REFFREQ_48MHZ;
+ break;
+ default:
+ pr_err("Bad parent clock rate on USB 2.0 PHY clock.\n");
+ return -EINVAL;
+ }
+
+ writel(val, cfgchip2);
+
+ return 0;
+}
+
+static struct clk usb20_phy_clk = {
+ .name = "usb20_phy",
+ .parent = &pll0_aux_clk,
+ .clk_enable = usb20_phy_clk_enable,
+ .clk_disable = usb20_phy_clk_disable,
+ .set_parent = usb20_phy_clk_set_parent,
+};
+
+static int usb11_phy_clk_set_parent(struct clk *clk, struct clk *parent)
+{
+ u32 __iomem *cfgchip2;
+ u32 val;
+
+ /*
+ * Can't use DA8XX_SYSCFG0_VIRT() here since this can be called before
+ * da8xx_syscfg0_base is initialized.
+ */
+ cfgchip2 = ioremap(DA8XX_SYSCFG0_BASE + DA8XX_CFGCHIP2_REG, 4);
+ val = readl(cfgchip2);
+
+ /* Set the USB 1.1 PHY clock mux based on the parent clock. */
+ if (parent == &usb20_phy_clk)
+ val &= ~CFGCHIP2_USB1PHYCLKMUX;
+ else if (parent == &usb_ref_clk)
+ val |= CFGCHIP2_USB1PHYCLKMUX;
+ else {
+ pr_err("Bad parent on USB 1.1 PHY clock.\n");
+ return -EINVAL;
+ }
+
+ writel(val, cfgchip2);
+
+ return 0;
+}
+
+static struct clk usb11_phy_clk = {
+ .name = "usb11_phy",
+ .parent = &usb20_phy_clk,
+ .set_parent = usb11_phy_clk_set_parent,
+};
+
static struct clk spi0_clk = {
.name = "spi0",
.parent = &pll0_sysclk2,
@@ -493,8 +633,11 @@ static struct clk_lookup da850_clks[] = {
CLK("da830-mmc.0", NULL, &mmcsd0_clk),
CLK("da830-mmc.1", NULL, &mmcsd1_clk),
CLK(NULL, "aemif", &aemif_clk),
+ CLK(NULL, "usb_ref_clk", &usb_ref_clk),
CLK(NULL, "usb11", &usb11_clk),
CLK(NULL, "usb20", &usb20_clk),
+ CLK(NULL, "usb20_phy", &usb20_phy_clk),
+ CLK(NULL, "usb11_phy", &usb11_phy_clk),
CLK("spi_davinci.0", NULL, &spi0_clk),
CLK("spi_davinci.1", NULL, &spi1_clk),
CLK("vpif", NULL, &vpif_clk),
--
1.9.1

2016-03-17 11:07:49

[permalink] [raw]

Subject: Re: [PATCH v2 10/11] usb: musb: da8xx: Use devm in probe

Hello.

On 3/17/2016 5:26 AM, David Lechner wrote:

> Simplify things a bit by using devm functions where possible.
>
> Signed-off-by: David Lechner <[email protected]>
> ---
>
> v2 changes: This is part of a previous patch that was split. No changes from
> previous version.
>
>
> drivers/usb/musb/da8xx.c | 28 ++++++++--------------------
> 1 file changed, 8 insertions(+), 20 deletions(-)
>
> diff --git a/drivers/usb/musb/da8xx.c b/drivers/usb/musb/da8xx.c
> index b03d3b8..50f07a5 100644
> --- a/drivers/usb/musb/da8xx.c
> +++ b/drivers/usb/musb/da8xx.c
> @@ -489,32 +489,27 @@ static int da8xx_probe(struct platform_device *pdev)
> struct platform_device *musb;
> struct da8xx_glue *glue;
> struct platform_device_info pinfo;
> - struct clk *clk;

I wouldn't remove this variable to minimize the patch.

[...]

MBR, Sergei

2016-03-17 12:12:15

[permalink] [raw]

Subject: Re: [PATCH v2 04/11] ARM: davinci: da8xx: add usb phy clocks

On 3/17/2016 5:26 AM, David Lechner wrote:

> Up to this point, the USB phy clock configuration was handled manually in
> the board files and in the usb drivers. This adds proper clocks so that
> the usb drivers can use clk_get and clk_enable and not have to worry about
> the details. Also, the related code is removed from the board files.
>
> Signed-off-by: David Lechner <[email protected]>
[...]
> diff --git a/arch/arm/mach-davinci/da830.c b/arch/arm/mach-davinci/da830.c
> index 7187e7f..ee942b0 100644
> --- a/arch/arm/mach-davinci/da830.c
> +++ b/arch/arm/mach-davinci/da830.c
[...]
> @@ -346,6 +347,12 @@ static struct clk i2c1_clk = {
> .gpsc = 1,
> };
>
> +static struct clk usb_ref_clk = {
> + .name = "usb_ref_clk",

I'd leave out "_clk" for consistency.

[...]
> @@ -353,6 +360,139 @@ static struct clk usb11_clk = {
[...]
> +static int usb20_phy_clk_set_parent(struct clk *clk, struct clk *parent)
> +{
> + u32 __iomem *cfgchip2;
> + u32 val;
> +
> + /*
> + * Can't use DA8XX_SYSCFG0_VIRT() here since this can be called before
> + * da8xx_syscfg0_base is initialized.
> + */
> + cfgchip2 = ioremap(DA8XX_SYSCFG0_BASE + DA8XX_CFGCHIP2_REG, 4);

You should be able to use IO_ADDRESS(), no?

[...]
> @@ -420,7 +560,10 @@ static struct clk_lookup da830_clks[] = {
> CLK("davinci_mdio.0", "fck", &emac_clk),
> CLK(NULL, "gpio", &gpio_clk),
> CLK("i2c_davinci.2", NULL, &i2c1_clk),
> + CLK(NULL, "usb_ref_clk", &usb_ref_clk),

I'd leave out "_clk" agai.

> CLK(NULL, "usb11", &usb11_clk),
> + CLK(NULL, "usb20_phy", &usb20_phy_clk),
> + CLK(NULL, "usb11_phy", &usb11_phy_clk),
> CLK(NULL, "emif3", &emif3_clk),
> CLK(NULL, "arm", &arm_clk),
> CLK(NULL, "rmii", &rmii_clk),
> diff --git a/arch/arm/mach-davinci/da850.c b/arch/arm/mach-davinci/da850.c
> index 8c8f31e..8089a82 100644
> --- a/arch/arm/mach-davinci/da850.c
> +++ b/arch/arm/mach-davinci/da850.c

Same comments on this file...

MBR, Sergei

2016-03-17 12:39:07

[permalink] [raw]

Subject: Re: [PATCH v2 06/11] phy: da8xx-usb: new driver for DA8XX SoC USB PHY

On 3/17/2016 5:26 AM, David Lechner wrote:

> This is a new phy driver for the SoC USB controllers on the TI DA8XX

DA8xx, please.

> family of microcontrollers. The USB 1.1 PHY is just a simple on/off.
> The USB 2.0 PHY also allows overriding the VBUS and ID pins.
>
> Signed-off-by: David Lechner <[email protected]>
[...]

> diff --git a/drivers/phy/phy-da8xx-usb.c b/drivers/phy/phy-da8xx-usb.c
> new file mode 100644
> index 0000000..93a5f4d
> --- /dev/null
> +++ b/drivers/phy/phy-da8xx-usb.c
> @@ -0,0 +1,295 @@
[...]
> +static inline u32 da8xx_usbphy_readl(void __iomem *base)
> +{
> + return readl(base);
> +}
> +
> +static inline void da8xx_usbphy_writel(void __iomem *base, u32 value)
> +{
> + writel(value, base);

Why wrap them at all?

> +}
> +
> +static int da8xx_usb11_phy_init(struct phy *phy)
> +{
> + struct da8xx_usbphy *d_phy = phy_get_drvdata(phy);
> + int ret;
> + u32 val;
> +
> + ret = clk_prepare_enable(d_phy->usb11_clk);
> + if (ret)
> + return ret;
> +
> + val = da8xx_usbphy_readl(d_phy->phy_ctrl);
> + val |= USB1SUSPENDM;
> + da8xx_usbphy_writel(d_phy->phy_ctrl, val);

Hum, I'd think this needs to be done in the power_on() method...

> +
> + return 0;
> +}
> +
> +static int da8xx_usb11_phy_shutdown(struct phy *phy)
> +{
> + struct da8xx_usbphy *d_phy = phy_get_drvdata(phy);
> + u32 val;
> +
> + val = da8xx_usbphy_readl(d_phy->phy_ctrl);
> + val &= ~USB1SUSPENDM;
> + da8xx_usbphy_writel(d_phy->phy_ctrl, val);

And this in power_off()...

> +
> + clk_disable_unprepare(d_phy->usb11_clk);
> +
> + return 0;
> +}
> +
> +static const struct phy_ops da8xx_usb11_phy_ops = {
> + .power_on = da8xx_usb11_phy_init,
> + .power_off = da8xx_usb11_phy_shutdown,

Aha, it's just that the names don't match...
Please call the implementations da8xx_usb11_phy_power_{on|off}().
The same with USB 2.0 PHY.

[...]

Looks good otherwise on my superficial review. :-)

MBR, Sergei

2016-03-17 12:54:01

[permalink] [raw]

Subject: Re: [PATCH v2 09/11] usb: ohci-da8xx: Remove code that references mach

On 3/17/2016 5:26 AM, David Lechner wrote:

> Including mach/* is frowned upon in device drivers, so get rid of it.
>
> This replaces usb20_clk and code that pokes CFGCHIP2 with a proper phy
> driver.
>
> Signed-off-by: David Lechner <[email protected]>
> ---
>
> v2 changes: Uses the new phy driver instead of using a second clock.
>
>
> drivers/usb/host/ohci-da8xx.c | 90 +++++++++++++++++++++----------------------
> 1 file changed, 44 insertions(+), 46 deletions(-)
>
> diff --git a/drivers/usb/host/ohci-da8xx.c b/drivers/usb/host/ohci-da8xx.c
> index e5c33bc..c648674 100644
> --- a/drivers/usb/host/ohci-da8xx.c
> +++ b/drivers/usb/host/ohci-da8xx.c
> @@ -15,58 +15,40 @@
> #include <linux/jiffies.h>
> #include <linux/platform_device.h>
> #include <linux/clk.h>
> -
> -#include <mach/da8xx.h>
> +#include <linux/phy/phy.h>
> #include <linux/platform_data/usb-davinci.h>
>
> #ifndef CONFIG_ARCH_DAVINCI_DA8XX
> #error "This file is DA8xx bus glue. Define CONFIG_ARCH_DAVINCI_DA8XX."
> #endif
>
> -#define CFGCHIP2 DA8XX_SYSCFG0_VIRT(DA8XX_CFGCHIP2_REG)
> -
> static struct clk *usb11_clk;
> -static struct clk *usb20_clk;
> +static struct phy *usb11_phy;
>
> /* Over-current indicator change bitmask */
> static volatile u16 ocic_mask;
>
> -static void ohci_da8xx_clock(int on)
> +static int ohci_da8xx_enable(void)
> {
[...]
> + ret = clk_prepare_enable(usb11_clk);
> + if (ret)
> + return ret;
>
> - /* Disable USB 1.1 PHY */
> - cfgchip2 &= ~CFGCHIP2_USB1SUSPENDM;
> + ret = phy_power_on(usb11_phy);

Aren't you supposed to call phy_init() first?

> + if (ret) {
> + clk_disable_unprepare(usb11_clk);
> + return ret;
> }
> - __raw_writel(cfgchip2, CFGCHIP2);
> +
> + return 0;
> +}
> +
> +static void ohci_da8xx_disable(void)
> +{
> + phy_power_off(usb11_phy);

... and phy_exit() after that?

> + clk_disable_unprepare(usb11_clk);
> }
>
> /*
[...]

MBR, Sergei

2016-03-17 13:11:48

[permalink] [raw]

Subject: Re: [PATCH v2 11/11] usb: musb: da8xx: Remove mach code

On 3/17/2016 5:26 AM, David Lechner wrote:

> Use the new phy-da8xx-usb driver to take the place of the mach code that
> pokes CFGCHIP2 in the da8xx musb glue driver.
>
> Signed-off-by: David Lechner <[email protected]>
> ---
>
> v2 changes: This is part of a previous patch that was split. This version uses
> the new phy driver instead of a second clock. It also gets rid of more mach
> code.
>
>
> drivers/usb/musb/da8xx.c | 126 ++++++++++++-----------------------------------
> 1 file changed, 31 insertions(+), 95 deletions(-)
>
> diff --git a/drivers/usb/musb/da8xx.c b/drivers/usb/musb/da8xx.c
> index 50f07a5..1d51711 100644
> --- a/drivers/usb/musb/da8xx.c
> +++ b/drivers/usb/musb/da8xx.c
> @@ -30,13 +30,11 @@
> #include <linux/clk.h>
> #include <linux/err.h>
> #include <linux/io.h>
> +#include <linux/phy/phy.h>
> #include <linux/platform_device.h>
> #include <linux/dma-mapping.h>
> #include <linux/usb/usb_phy_generic.h>
>
> -#include <mach/da8xx.h>
> -#include <linux/platform_data/usb-davinci.h>

I guess we can move this header back into mach/ now?

[...]
> @@ -383,31 +335,18 @@ static irqreturn_t da8xx_musb_interrupt(int irq, void *hci)
> return ret;
> }
>
> +extern int da8xx_usb20_phy_set_mode(struct phy *phy, enum musb_mode mode);
> +

You now need to select your new PHY driver since otherwise the kernel
won't build.
You can remove "depends on BROKEN" as well since your patch is un-breaking
it. :-)

[...]
> @@ -425,19 +364,24 @@ static int da8xx_musb_init(struct musb *musb)
[...]
> /* Start the on-chip PHY and its PLL. */
> - phy_on();
> + phy_power_on(glue->phy);

What about phy_init()?

> @@ -448,9 +392,12 @@ fail:
>
> static int da8xx_musb_exit(struct musb *musb)
> {
> + struct da8xx_glue *glue = dev_get_drvdata(musb->controller->parent);
> +
> del_timer_sync(&otg_workaround);
>
> - phy_off();
> + phy_power_off(glue->phy);

phy_exit()?

[...]
> @@ -503,19 +450,19 @@ static int da8xx_probe(struct platform_device *pdev)
[...]
>
> glue->dev = &pdev->dev;
> pdata->platform_ops = &da8xx_ops;
>
> - glue->phy = usb_phy_generic_register();
> - if (IS_ERR(glue->phy)) {
> - ret = PTR_ERR(glue->phy);
> - goto err5;
> + glue->usb_phy = usb_phy_generic_register();

Hm, do we still need the USB PHY? It does nothing IIRC...

[...]

MBR, Sergei

2016-03-17 13:39:31

[permalink] [raw]

Subject: Re: [PATCH v2 00/11] da8xx USB clocks

On 3/17/2016 5:26 AM, David Lechner wrote:

> OK, ready for round two.

You're quick... :-)

> I've added a new callback in the davinci clocks so that they can properly
> handle clock muxing. The clock functions are pretty much the same as in the
> previous patch set other than clk_set_parent() now works.
>
> The next new thing is a phy driver for the CFGCHIP2 register that controls the
> SoC USB PHY (both USB 1.1 and USB 2.0). The ohci and musb drivers have been
> updated to use this new phy driver.

Fantastic! At least there's some breakthru on this front.

[...]

MBR, Sergei

2016-03-17 18:01:09

by Alan Stern

[permalink] [raw]

Subject: Re: [PATCH v2 09/11] usb: ohci-da8xx: Remove code that references mach

On Wed, 16 Mar 2016, David Lechner wrote:

> Including mach/* is frowned upon in device drivers, so get rid of it.
>
> This replaces usb20_clk and code that pokes CFGCHIP2 with a proper phy
> driver.
>
> Signed-off-by: David Lechner <[email protected]>
> ---
>
> v2 changes: Uses the new phy driver instead of using a second clock.

Acked-by: Alan Stern <[email protected]>

2016-03-17 21:06:26

[permalink] [raw]

Subject: Re: [PATCH v2 11/11] usb: musb: da8xx: Remove mach code

On 03/17/2016 08:11 AM, Sergei Shtylyov wrote:
> On 3/17/2016 5:26 AM, David Lechner wrote:
>>
>> glue->dev = &pdev->dev;
>> pdata->platform_ops = &da8xx_ops;
>>
>> - glue->phy = usb_phy_generic_register();
>> - if (IS_ERR(glue->phy)) {
>> - ret = PTR_ERR(glue->phy);
>> - goto err5;
>> + glue->usb_phy = usb_phy_generic_register();
>
> Hm, do we still need the USB PHY? It does nothing IIRC...
>

Unfortunately, yes. It is used in musb_core, etc., mainly for otg->state.

2016-03-19 23:56:53

by Rob Herring (Arm)

[permalink] [raw]

Subject: Re: [PATCH v2 05/11] dt-bindings: Add bindings for phy-da8xx-usb

On Wed, Mar 16, 2016 at 09:26:38PM -0500, David Lechner wrote:
> Device tree binding for new phy-da8xx-usb driver.
>
> Signed-off-by: David Lechner <[email protected]>
> ---
>
> v2 changes: This is new patch in v2.
>
>
> .../devicetree/bindings/phy/phy-da8xx-usb.txt | 34 ++++++++++++++++++++++
> 1 file changed, 34 insertions(+)
> create mode 100644 Documentation/devicetree/bindings/phy/phy-da8xx-usb.txt
>
> diff --git a/Documentation/devicetree/bindings/phy/phy-da8xx-usb.txt b/Documentation/devicetree/bindings/phy/phy-da8xx-usb.txt
> new file mode 100644
> index 0000000..ed6b710
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/phy/phy-da8xx-usb.txt
> @@ -0,0 +1,34 @@
> +TI DaVinci DA8XX USB PHY
> +
> +Required properties:
> + - compatible: must be "ti,da830-usbphy".
> + - #phy-cells: must be 1.
> + - reg : Address and length of the CFGCHIP2 register.
> +
> +This device controls the PHY for both the USB 1.1 OHCI and USB 2.0 OTG
> +controllers on DA8XX SoCs. Consumers of this device should use index 1 for

DA8xx

> +the USB 1.1 phy device and index 2 for the USB 2.0 phy device.
> +
> +Example:
> +
> + usbphy: usbphy@1c14184 {

TI has been doing the unit addresses wrong I've recently found. It
should match the reg property, not be the full physical address. Please
fix all these in the example.

> + compatible = "ti,da830-usbphy";
> + #phy-cells = <1>;
> + reg = <0x14184 4>;
> + };
> +
> + usb11: usb11@1e25000 {

usb@...

> + compatible = "ti,da830-ohci";
> + reg = <0x225000 0x1000>;
> + interrupts = <59>;
> + phys = <&usbphy 1>;
> + phy-names = "usbphy";
> + };
> +
> + usb20: usb@1e00000 {
> + compatible = "ti,da830-musb";
> + reg = <0x200000 0x1000>;
> + interrupts = <58>;
> + phys = <&usbphy 2>;
> + phy-names = "usbphy";
> + };
> --
> 1.9.1
>

2016-03-23 15:58:31

[permalink] [raw]

Subject: Re: [PATCH v2 03/11] ARM: davinci: da850: use clk->set_parent for async3

On Thursday 17 March 2016 07:56 AM, David Lechner wrote:
> The da850 family of processors has an async3 clock domain that can be
> muxed to either pll0_sysclk2 or pll1_sysclk2. Now that the davinci clocks
> have a set_parent callback, we can use this to control the async3 mux
> instead of a stand-alone function.
>
> This adds a new async3_clk and sets the appropriate child clocks. The
> default is use to pll1_sysclk2 since it is not affected by processor
> frequency scaling.
>
> Signed-off-by: David Lechner <[email protected]>
> ---

> +static int da850_async3_set_parent(struct clk *clk, struct clk *parent)
> +{
> + u32 __iomem *cfgchip3;
> + u32 val;
> +
> + /*
> + * Can't use DA8XX_SYSCFG0_VIRT() here since this can be called before
> + * da8xx_syscfg0_base is initialized.
> + */
> + cfgchip3 = ioremap(DA8XX_SYSCFG0_BASE + DA8XX_CFGCHIP3_REG, 4);

Is this just a theoretical possibility or have you seen this happen? I
would like to see if there are ways of avoiding this rather than throw
away usage of DA8XX_SYSCFG0_VIRT()

> + val = readl(cfgchip3);
> +
> + /* Set the USB 1.1 PHY clock mux based on the parent clock. */

Comment is wrong (copy-paste error?)

> + if (parent == &pll0_sysclk2)
> + val &= ~CFGCHIP3_ASYNC3_CLKSRC;
> + else if (parent == &pll1_sysclk2)
> + val |= CFGCHIP3_ASYNC3_CLKSRC;
> + else {
> + pr_err("Bad parent on async3 clock mux.\n");
> + return -EINVAL;
> + }
> +
> + writel(val, cfgchip3);
> +
> + return 0;
> +}

Thanks,
Sekhar

2016-03-23 16:58:29

[permalink] [raw]

Subject: Re: [PATCH v2 04/11] ARM: davinci: da8xx: add usb phy clocks

On Thursday 17 March 2016 07:56 AM, David Lechner wrote:
> Up to this point, the USB phy clock configuration was handled manually in
> the board files and in the usb drivers. This adds proper clocks so that
> the usb drivers can use clk_get and clk_enable and not have to worry about
> the details. Also, the related code is removed from the board files.
>
> Signed-off-by: David Lechner <[email protected]>
> ---
>
> v2 changes: Move clock mux code to set_parent callback. Also fixed some other
> issues from feedback on the previous patch.
>
>
> arch/arm/mach-davinci/board-da830-evm.c | 12 ---
> arch/arm/mach-davinci/board-omapl138-hawk.c | 7 --
> arch/arm/mach-davinci/da830.c | 143 ++++++++++++++++++++++++++++
> arch/arm/mach-davinci/da850.c | 143 ++++++++++++++++++++++++++++
> 4 files changed, 286 insertions(+), 19 deletions(-)
>
> diff --git a/arch/arm/mach-davinci/board-da830-evm.c b/arch/arm/mach-davinci/board-da830-evm.c
> index 3d8cf8c..f3a8cc9 100644
> --- a/arch/arm/mach-davinci/board-da830-evm.c
> +++ b/arch/arm/mach-davinci/board-da830-evm.c
> @@ -115,18 +115,6 @@ static __init void da830_evm_usb_init(void)
> */
> cfgchip2 = __raw_readl(DA8XX_SYSCFG0_VIRT(DA8XX_CFGCHIP2_REG));
>
> - /* USB2.0 PHY reference clock is 24 MHz */
> - cfgchip2 &= ~CFGCHIP2_REFFREQ;
> - cfgchip2 |= CFGCHIP2_REFFREQ_24MHZ;
> -
> - /*
> - * Select internal reference clock for USB 2.0 PHY
> - * and use it as a clock source for USB 1.1 PHY
> - * (this is the default setting anyway).
> - */
> - cfgchip2 &= ~CFGCHIP2_USB1PHYCLKMUX;
> - cfgchip2 |= CFGCHIP2_USB2PHYCLKMUX;
> -
> /*
> * We have to override VBUS/ID signals when MUSB is configured into the
> * host-only mode -- ID pin will float if no cable is connected, so the
> diff --git a/arch/arm/mach-davinci/board-omapl138-hawk.c b/arch/arm/mach-davinci/board-omapl138-hawk.c
> index ee62486..d27e753 100644
> --- a/arch/arm/mach-davinci/board-omapl138-hawk.c
> +++ b/arch/arm/mach-davinci/board-omapl138-hawk.c
> @@ -251,13 +251,6 @@ static __init void omapl138_hawk_usb_init(void)
> return;
> }
>
> - /* Setup the Ref. clock frequency for the HAWK at 24 MHz. */
> -
> - cfgchip2 = __raw_readl(DA8XX_SYSCFG0_VIRT(DA8XX_CFGCHIP2_REG));
> - cfgchip2 &= ~CFGCHIP2_REFFREQ;
> - cfgchip2 |= CFGCHIP2_REFFREQ_24MHZ;
> - __raw_writel(cfgchip2, DA8XX_SYSCFG0_VIRT(DA8XX_CFGCHIP2_REG));
> -
> ret = gpio_request_one(DA850_USB1_VBUS_PIN,
> GPIOF_DIR_OUT, "USB1 VBUS");
> if (ret < 0) {
> diff --git a/arch/arm/mach-davinci/da830.c b/arch/arm/mach-davinci/da830.c
> index 7187e7f..ee942b0 100644
> --- a/arch/arm/mach-davinci/da830.c
> +++ b/arch/arm/mach-davinci/da830.c
> @@ -12,6 +12,7 @@
> #include <linux/init.h>
> #include <linux/clk.h>
> #include <linux/platform_data/gpio-davinci.h>
> +#include <linux/platform_data/usb-davinci.h>
>
> #include <asm/mach/map.h>
>
> @@ -346,6 +347,12 @@ static struct clk i2c1_clk = {
> .gpsc = 1,
> };
>
> +static struct clk usb_ref_clk = {
> + .name = "usb_ref_clk",
> + .rate = 48000000,
> + .set_rate = davinci_simple_set_rate,
> +};

can we call this usb_refclkin so it matches the TRM name? Also, should
this node be not be coming through individual board files as the rate
depends on what is connected to the usb_refclkin pin? Or is the
expectation that boards will call clk_set_rate() on this clock to the
correct value? If yes, I think it is misleading to populate the .rate here.

> +
> static struct clk usb11_clk = {
> .name = "usb11",
> .parent = &pll0_sysclk4,
> @@ -353,6 +360,139 @@ static struct clk usb11_clk = {
> .gpsc = 1,
> };
>
> +static void usb20_phy_clk_enable(struct clk *clk)
> +{
> + u32 val;
> +
> + val = readl(DA8XX_SYSCFG0_VIRT(DA8XX_CFGCHIP2_REG));
> +
> + /*
> + * Turn on the USB 2.0 PHY, but just the PLL, and not OTG. The USB 1.1
> + * host may use the PLL clock without USB 2.0 OTG being used.
> + */
> + val &= ~(CFGCHIP2_RESET | CFGCHIP2_PHYPWRDN);
> + val |= CFGCHIP2_PHY_PLLON;
> +
> + writel(val, DA8XX_SYSCFG0_VIRT(DA8XX_CFGCHIP2_REG));
> +
> + pr_info("Waiting for USB 2.0 PHY clock good...\n");
> + while (!(readl(DA8XX_SYSCFG0_VIRT(DA8XX_CFGCHIP2_REG))
> + & CFGCHIP2_PHYCLKGD))
> + cpu_relax();

I guess this is copying some earlier code, but still, it will be nice to
see a timeout mechanism here, rather than loop endlessly.

> +}
> +
> +static void usb20_phy_clk_disable(struct clk *clk)
> +{
> + u32 val;
> +
> + val = readl(DA8XX_SYSCFG0_VIRT(DA8XX_CFGCHIP2_REG));
> + val |= CFGCHIP2_PHYPWRDN;
> + writel(val, DA8XX_SYSCFG0_VIRT(DA8XX_CFGCHIP2_REG));
> +}
> +
> +static int usb20_phy_clk_set_parent(struct clk *clk, struct clk *parent)
> +{
> + u32 __iomem *cfgchip2;
> + u32 val;
> +
> + /*
> + * Can't use DA8XX_SYSCFG0_VIRT() here since this can be called before
> + * da8xx_syscfg0_base is initialized.
> + */
> + cfgchip2 = ioremap(DA8XX_SYSCFG0_BASE + DA8XX_CFGCHIP2_REG, 4);

Again, not sure if this is juts a theoretical possibility. If yes, I
would rather see you bail out if syscfg0_base is not initialized by the
time you get here than do an ioremap() again.

> + val = readl(cfgchip2);
> +
> + /* Set the mux depending on the parent clock. */
> + if (parent == &pll0_aux_clk)
> + val |= CFGCHIP2_USB2PHYCLKMUX;
> + else if (parent == &usb_ref_clk)
> + val &= ~CFGCHIP2_USB2PHYCLKMUX;
> + else {
> + pr_err("Bad parent on USB 2.0 PHY clock.\n");
> + return -EINVAL;
> + }
> +
> + /* reference frequency also comes from parent clock */
> + val &= ~CFGCHIP2_REFFREQ;
> + switch (clk_get_rate(parent)) {
> + case 12000000:
> + val |= CFGCHIP2_REFFREQ_12MHZ;
> + break;
> + case 13000000:
> + val |= CFGCHIP2_REFFREQ_13MHZ;
> + break;
> + case 19200000:
> + val |= CFGCHIP2_REFFREQ_19_2MHZ;
> + break;
> + case 20000000:
> + val |= CFGCHIP2_REFFREQ_20MHZ;
> + break;
> + case 24000000:
> + val |= CFGCHIP2_REFFREQ_24MHZ;
> + break;
> + case 26000000:
> + val |= CFGCHIP2_REFFREQ_26MHZ;
> + break;
> + case 38400000:
> + val |= CFGCHIP2_REFFREQ_38_4MHZ;
> + break;
> + case 40000000:
> + val |= CFGCHIP2_REFFREQ_40MHZ;
> + break;
> + case 48000000:
> + val |= CFGCHIP2_REFFREQ_48MHZ;
> + break;
> + default:
> + pr_err("Bad parent clock rate on USB 2.0 PHY clock.\n");
> + return -EINVAL;
> + }
> +
> + writel(val, cfgchip2);
> +
> + return 0;
> +}
> +
> +static struct clk usb20_phy_clk = {
> + .name = "usb20_phy",
> + .parent = &pll0_aux_clk,
> + .clk_enable = usb20_phy_clk_enable,
> + .clk_disable = usb20_phy_clk_disable,
> + .set_parent = usb20_phy_clk_set_parent,
> +};

I hope you have checked that all boards in mainline use the AUXCLK as
the reference USB 2.0 frequency?

> +
> +static int usb11_phy_clk_set_parent(struct clk *clk, struct clk *parent)
> +{
> + u32 __iomem *cfgchip2;
> + u32 val;
> +
> + /*
> + * Can't use DA8XX_SYSCFG0_VIRT() here since this can be called before
> + * da8xx_syscfg0_base is initialized.
> + */
> + cfgchip2 = ioremap(DA8XX_SYSCFG0_BASE + DA8XX_CFGCHIP2_REG, 4);
> + val = readl(cfgchip2);
> +
> + /* Set the USB 1.1 PHY clock mux based on the parent clock. */
> + if (parent == &usb20_phy_clk)
> + val &= ~CFGCHIP2_USB1PHYCLKMUX;
> + else if (parent == &usb_ref_clk)
> + val |= CFGCHIP2_USB1PHYCLKMUX;
> + else {
> + pr_err("Bad parent on USB 1.1 PHY clock.\n");
> + return -EINVAL;
> + }
> +
> + writel(val, cfgchip2);
> +
> + return 0;
> +}
> +
> +static struct clk usb11_phy_clk = {
> + .name = "usb11_phy",
> + .parent = &usb20_phy_clk,
> + .set_parent = usb11_phy_clk_set_parent,
> +};

Same thing here. I hope all current boards use USB2.0 clk as reference
for USB 1.1 phy

> +
> static struct clk emif3_clk = {
> .name = "emif3",
> .parent = &pll0_sysclk5,
> @@ -420,7 +560,10 @@ static struct clk_lookup da830_clks[] = {
> CLK("davinci_mdio.0", "fck", &emac_clk),
> CLK(NULL, "gpio", &gpio_clk),
> CLK("i2c_davinci.2", NULL, &i2c1_clk),
> + CLK(NULL, "usb_ref_clk", &usb_ref_clk),
> CLK(NULL, "usb11", &usb11_clk),
> + CLK(NULL, "usb20_phy", &usb20_phy_clk),
> + CLK(NULL, "usb11_phy", &usb11_phy_clk),
> CLK(NULL, "emif3", &emif3_clk),
> CLK(NULL, "arm", &arm_clk),
> CLK(NULL, "rmii", &rmii_clk),
> diff --git a/arch/arm/mach-davinci/da850.c b/arch/arm/mach-davinci/da850.c
> index 8c8f31e..8089a82 100644
> --- a/arch/arm/mach-davinci/da850.c
> +++ b/arch/arm/mach-davinci/da850.c
> @@ -19,6 +19,7 @@
> #include <linux/cpufreq.h>
> #include <linux/regulator/consumer.h>
> #include <linux/platform_data/gpio-davinci.h>
> +#include <linux/platform_data/usb-davinci.h>
>
> #include <asm/mach/map.h>
>
> @@ -360,6 +361,12 @@ static struct clk aemif_clk = {
> .flags = ALWAYS_ENABLED,
> };
>
> +static struct clk usb_ref_clk = {
> + .name = "usb_ref_clk",
> + .rate = 48000000,
> + .set_rate = davinci_simple_set_rate,
> +};
> +
> static struct clk usb11_clk = {
> .name = "usb11",
> .parent = &pll0_sysclk4,
> @@ -374,6 +381,139 @@ static struct clk usb20_clk = {
> .gpsc = 1,
> };
>
> +static void usb20_phy_clk_enable(struct clk *clk)
> +{
> + u32 val;
> +
> + val = readl(DA8XX_SYSCFG0_VIRT(DA8XX_CFGCHIP2_REG));
> +
> + /*
> + * Turn on the USB 2.0 PHY, but just the PLL, and not OTG. The USB 1.1
> + * host may use the PLL clock without USB 2.0 OTG being used.
> + */
> + val &= ~(CFGCHIP2_RESET | CFGCHIP2_PHYPWRDN);
> + val |= CFGCHIP2_PHY_PLLON;
> +
> + writel(val, DA8XX_SYSCFG0_VIRT(DA8XX_CFGCHIP2_REG));
> +
> + pr_info("Waiting for USB 2.0 PHY clock good...\n");
> + while (!(readl(DA8XX_SYSCFG0_VIRT(DA8XX_CFGCHIP2_REG))
> + & CFGCHIP2_PHYCLKGD))
> + cpu_relax();
> +}

Looks like this is pretty much going to be the same code repeated for
DA850 and DA830. So can we move these to a common file like da8xx-usb.c?
You can even register these USB clocks from that file by using
clkdev_add() and clk_register(). This way they can remain to be file local.

Thanks,
Sekhar

2016-03-23 17:08:27

[permalink] [raw]

Subject: Re: [PATCH v2 05/11] dt-bindings: Add bindings for phy-da8xx-usb

On Thursday 17 March 2016 07:56 AM, David Lechner wrote:
> Device tree binding for new phy-da8xx-usb driver.
>
> Signed-off-by: David Lechner <[email protected]>
> ---
>
> v2 changes: This is new patch in v2.
>
>
> .../devicetree/bindings/phy/phy-da8xx-usb.txt | 34 ++++++++++++++++++++++
> 1 file changed, 34 insertions(+)
> create mode 100644 Documentation/devicetree/bindings/phy/phy-da8xx-usb.txt
>
> diff --git a/Documentation/devicetree/bindings/phy/phy-da8xx-usb.txt b/Documentation/devicetree/bindings/phy/phy-da8xx-usb.txt
> new file mode 100644
> index 0000000..ed6b710
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/phy/phy-da8xx-usb.txt
> @@ -0,0 +1,34 @@
> +TI DaVinci DA8XX USB PHY
> +
> +Required properties:
> + - compatible: must be "ti,da830-usbphy".
> + - #phy-cells: must be 1.
> + - reg : Address and length of the CFGCHIP2 register.

I am not sure passing CFGCHIP2 register as reg property to the phy is
future proof. At some point, we do want to move to common clock
framework and at that point USB clocks controlled by CFGCHIP2 will be a
separate driver needing access to the same register.

So I think the CFGCHIP2 access in USB phy driver should happen through a
syscon phandle. This needs to happen now, not later since we cannot
break DT backward-compatibility.

Thanks,
Sekhar

2016-03-23 17:20:37

[permalink] [raw]

Subject: Re: [PATCH v2 03/11] ARM: davinci: da850: use clk->set_parent for async3

On 03/23/2016 10:56 AM, Sekhar Nori wrote:
> On Thursday 17 March 2016 07:56 AM, David Lechner wrote:
>> The da850 family of processors has an async3 clock domain that can be
>> muxed to either pll0_sysclk2 or pll1_sysclk2. Now that the davinci clocks
>> have a set_parent callback, we can use this to control the async3 mux
>> instead of a stand-alone function.
>>
>> This adds a new async3_clk and sets the appropriate child clocks. The
>> default is use to pll1_sysclk2 since it is not affected by processor
>> frequency scaling.
>>
>> Signed-off-by: David Lechner <[email protected]>
>> ---
>
>> +static int da850_async3_set_parent(struct clk *clk, struct clk *parent)
>> +{
>> + u32 __iomem *cfgchip3;
>> + u32 val;
>> +
>> + /*
>> + * Can't use DA8XX_SYSCFG0_VIRT() here since this can be called before
>> + * da8xx_syscfg0_base is initialized.
>> + */
>> + cfgchip3 = ioremap(DA8XX_SYSCFG0_BASE + DA8XX_CFGCHIP3_REG, 4);
>
> Is this just a theoretical possibility or have you seen this happen? I
> would like to see if there are ways of avoiding this rather than throw
> away usage of DA8XX_SYSCFG0_VIRT()

Yes, it will not boot without this. The problem comes from the fact that
clocks are setup in davinci_common_init() which is called before
da8xx_syscfg0_base = ioremap(DA8XX_SYSCFG0_BASE, SZ_4K) in da850_init()
(and da830_init()). I also tried moving the ioremap() before
davinci_common_init(), but davinci_common_init() sets up the iomem, so
that doesn't work either.

So, if you want to use DA8XX_SYSCFG0_VIRT() here, the clock init would
have to be split out from davinci_common_init() which would affect all
davinci devices.

2016-03-23 17:23:38

[permalink] [raw]

Subject: Re: [PATCH v2 06/11] phy: da8xx-usb: new driver for DA8XX SoC USB PHY

On Thursday 17 March 2016 07:56 AM, David Lechner wrote:
> This is a new phy driver for the SoC USB controllers on the TI DA8XX
> family of microcontrollers. The USB 1.1 PHY is just a simple on/off.
> The USB 2.0 PHY also allows overriding the VBUS and ID pins.
>
> Signed-off-by: David Lechner <[email protected]>

> diff --git a/drivers/phy/phy-da8xx-usb.c b/drivers/phy/phy-da8xx-usb.c
> new file mode 100644
> index 0000000..93a5f4d
> --- /dev/null
> +++ b/drivers/phy/phy-da8xx-usb.c
> @@ -0,0 +1,295 @@
> +/*
> + * phy-da8xx-usb - TI DaVinci DA8XX USB PHY driver
> + *
> + * Copyright (C) 2016 David Lechner <[email protected]>
> + *
> + * 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/io.h>
> +#include <linux/of.h>
> +#include <linux/module.h>
> +#include <linux/phy/phy.h>
> +#include <linux/platform_device.h>
> +#include <linux/usb/gadget.h>
> +#include <linux/usb/musb.h>
> +#include <linux/usb/otg.h>
> +
> +/* DA8xx CFGCHIP2 (USB PHY Control) register bits */
> +#define PHYCLKGD (1 << 17)
> +#define VBUSSENSE (1 << 16)
> +#define RESET (1 << 15)
> +#define OTGMODE_MASK (3 << 13)
> +#define NO_OVERRIDE (0 << 13)
> +#define FORCE_HOST (1 << 13)
> +#define FORCE_DEVICE (2 << 13)
> +#define FORCE_HOST_VBUS_LOW (3 << 13)
> +#define USB1PHYCLKMUX (1 << 12)
> +#define USB2PHYCLKMUX (1 << 11)
> +#define PHYPWRDN (1 << 10)
> +#define OTGPWRDN (1 << 9)
> +#define DATPOL (1 << 8)
> +#define USB1SUSPENDM (1 << 7)
> +#define PHY_PLLON (1 << 6)
> +#define SESENDEN (1 << 5)
> +#define VBDTCTEN (1 << 4)
> +#define REFFREQ_MASK (0xf << 0)
> +#define REFFREQ_12MHZ (1 << 0)
> +#define REFFREQ_24MHZ (2 << 0)
> +#define REFFREQ_48MHZ (3 << 0)
> +#define REFFREQ_19_2MHZ (4 << 0)
> +#define REFFREQ_38_4MHZ (5 << 0)
> +#define REFFREQ_13MHZ (6 << 0)
> +#define REFFREQ_26MHZ (7 << 0)
> +#define REFFREQ_20MHZ (8 << 0)
> +#define REFFREQ_40MHZ (9 << 0)

Many of these register bits are unused. I guess opinion varies around
this, but I get confused with unnecessary bit definitions and register
offsets. I tend to search for it and its sort of disappointing to see
that its basically unused. Of course, you should wait for PHY
maintainers preference.

> +
> +struct da8xx_usbphy {
> + struct phy_provider *phy_provider;
> + struct phy *usb11_phy;
> + struct phy *usb20_phy;
> + struct clk *usb11_clk;
> + struct clk *usb20_clk;
> + void __iomem *phy_ctrl;
> +};
> +
> +static inline u32 da8xx_usbphy_readl(void __iomem *base)
> +{
> + return readl(base);
> +}
> +
> +static inline void da8xx_usbphy_writel(void __iomem *base, u32 value)
> +{
> + writel(value, base);
> +}

Agree with Sergei that these don't add much value.

> +int da8xx_usb20_phy_set_mode(struct phy *phy, enum musb_mode mode)
> +{
> + struct da8xx_usbphy *d_phy = phy_get_drvdata(phy);
> + u32 val;
> +
> + val = da8xx_usbphy_readl(d_phy->phy_ctrl);
> +
> + val &= ~OTGMODE_MASK;
> + switch (mode) {
> + case MUSB_HOST: /* Force VBUS valid, ID = 0 */
> + val |= FORCE_HOST;
> + break;
> + case MUSB_PERIPHERAL: /* Force VBUS valid, ID = 1 */
> + val |= FORCE_DEVICE;
> + break;
> + case MUSB_OTG: /* Don't override the VBUS/ID comparators */
> + val |= NO_OVERRIDE;
> + break;
> + default:
> + return -EINVAL;
> + }
> +
> + da8xx_usbphy_writel(d_phy->phy_ctrl, val);
> +
> + return 0;
> +}
> +EXPORT_SYMBOL_GPL(da8xx_usb20_phy_set_mode);

Hmm, since this driver exports this symbol, I think it should also
provide an include file in include/linux/phy for users of the symbol. Or
perhaps there should be a generic API around this since it looks like
most USB phys will need something similar?

Thanks,
Sekhar

2016-03-23 17:28:46

[permalink] [raw]

Subject: Re: [PATCH v2 00/11] da8xx USB clocks

Hi David,

On Thursday 17 March 2016 07:09 PM, Sergei Shtylyov wrote:
> On 3/17/2016 5:26 AM, David Lechner wrote:
>
>> OK, ready for round two.
>
> You're quick... :-)
>
>> I've added a new callback in the davinci clocks so that they can properly
>> handle clock muxing. The clock functions are pretty much the same as
>> in the
>> previous patch set other than clk_set_parent() now works.
>>
>> The next new thing is a phy driver for the CFGCHIP2 register that
>> controls the
>> SoC USB PHY (both USB 1.1 and USB 2.0). The ohci and musb drivers have
>> been
>> updated to use this new phy driver.
>
> Fantastic! At least there's some breakthru on this front.

+1 Thanks a bunch for resurrecting this code.

Regards,
Sekhar

2016-03-23 17:31:22

[permalink] [raw]

Subject: Re: [PATCH v2 03/11] ARM: davinci: da850: use clk->set_parent for async3

On Wednesday 23 March 2016 10:50 PM, David Lechner wrote:
> On 03/23/2016 10:56 AM, Sekhar Nori wrote:
>> On Thursday 17 March 2016 07:56 AM, David Lechner wrote:
>>> The da850 family of processors has an async3 clock domain that can be
>>> muxed to either pll0_sysclk2 or pll1_sysclk2. Now that the davinci
>>> clocks
>>> have a set_parent callback, we can use this to control the async3 mux
>>> instead of a stand-alone function.
>>>
>>> This adds a new async3_clk and sets the appropriate child clocks. The
>>> default is use to pll1_sysclk2 since it is not affected by processor
>>> frequency scaling.
>>>
>>> Signed-off-by: David Lechner <[email protected]>
>>> ---
>>
>>> +static int da850_async3_set_parent(struct clk *clk, struct clk *parent)
>>> +{
>>> + u32 __iomem *cfgchip3;
>>> + u32 val;
>>> +
>>> + /*
>>> + * Can't use DA8XX_SYSCFG0_VIRT() here since this can be called
>>> before
>>> + * da8xx_syscfg0_base is initialized.
>>> + */
>>> + cfgchip3 = ioremap(DA8XX_SYSCFG0_BASE + DA8XX_CFGCHIP3_REG, 4);
>>
>> Is this just a theoretical possibility or have you seen this happen? I
>> would like to see if there are ways of avoiding this rather than throw
>> away usage of DA8XX_SYSCFG0_VIRT()
>
> Yes, it will not boot without this. The problem comes from the fact that
> clocks are setup in davinci_common_init() which is called before
> da8xx_syscfg0_base = ioremap(DA8XX_SYSCFG0_BASE, SZ_4K) in da850_init()
> (and da830_init()). I also tried moving the ioremap() before
> davinci_common_init(), but davinci_common_init() sets up the iomem, so
> that doesn't work either.
>
> So, if you want to use DA8XX_SYSCFG0_VIRT() here, the clock init would
> have to be split out from davinci_common_init() which would affect all
> davinci devices.

Alright, I guess 'can be called' in the comment should have used
stronger language :) How about late registration of USB clocks as I
suggested. It should also help consolidate code across da830 and da850.

Thanks,
Sekhar

2016-03-23 17:45:10

[permalink] [raw]

Subject: Re: [PATCH v2 04/11] ARM: davinci: da8xx: add usb phy clocks

On 03/23/2016 11:56 AM, Sekhar Nori wrote:
>>
>> +static struct clk usb_ref_clk = {
>> + .name = "usb_ref_clk",
>> + .rate = 48000000,
>> + .set_rate = davinci_simple_set_rate,
>> +};
>
> can we call this usb_refclkin so it matches the TRM name? Also, should
> this node be not be coming through individual board files as the rate
> depends on what is connected to the usb_refclkin pin? Or is the
> expectation that boards will call clk_set_rate() on this clock to the
> correct value? If yes, I think it is misleading to populate the .rate here.

You are right. When I did this, I was looking at USB 1.1 only, which
MUST be 48MHz. However, this can be used for USB 2.0 which can accept a
number of rates.

However, even the main reference oscillator in da850.c has the rate hard
coded in da850.c (DA850_REF_FREQ).

The clock initialization will fail if a clock does not have a parent or
a rate, so we have to give it a default rate since it is an external
clock and has no parent. So, I think 48MHz makes sense for a default
value. Most boards will probably not be using this clock anyway, but
rather the PLL in the USB 2.0 PHY.

>> +
>> + pr_info("Waiting for USB 2.0 PHY clock good...\n");
>> + while (!(readl(DA8XX_SYSCFG0_VIRT(DA8XX_CFGCHIP2_REG))
>> + & CFGCHIP2_PHYCLKGD))
>> + cpu_relax();
>
> I guess this is copying some earlier code, but still, it will be nice to
> see a timeout mechanism here, rather than loop endlessly.

Do you have a suggestion on how to do this?

>> +
>> + /*
>> + * Can't use DA8XX_SYSCFG0_VIRT() here since this can be called before
>> + * da8xx_syscfg0_base is initialized.
>> + */
>> + cfgchip2 = ioremap(DA8XX_SYSCFG0_BASE + DA8XX_CFGCHIP2_REG, 4);
>
> Again, not sure if this is juts a theoretical possibility. If yes, I
> would rather see you bail out if syscfg0_base is not initialized by the
> time you get here than do an ioremap() again.
>

Will rework clock registration so that this is not necessary.

>> +
>> +static struct clk usb20_phy_clk = {
>> + .name = "usb20_phy",
>> + .parent = &pll0_aux_clk,
>> + .clk_enable = usb20_phy_clk_enable,
>> + .clk_disable = usb20_phy_clk_disable,
>> + .set_parent = usb20_phy_clk_set_parent,
>> +};
>
> I hope you have checked that all boards in mainline use the AUXCLK as
> the reference USB 2.0 frequency?
>

After sending this patch set, I realized that I missed updating existing
boards. Will include this in v3.

>> +
>> +static struct clk usb11_phy_clk = {
>> + .name = "usb11_phy",
>> + .parent = &usb20_phy_clk,
>> + .set_parent = usb11_phy_clk_set_parent,
>> +};
>
> Same thing here. I hope all current boards use USB2.0 clk as reference
> for USB 1.1 phy
>

Ditto. Will check on this.

>> +static void usb20_phy_clk_enable(struct clk *clk)
>> +{
>> + u32 val;
>> +
>> + val = readl(DA8XX_SYSCFG0_VIRT(DA8XX_CFGCHIP2_REG));
>> +
>> + /*
>> + * Turn on the USB 2.0 PHY, but just the PLL, and not OTG. The USB 1.1
>> + * host may use the PLL clock without USB 2.0 OTG being used.
>> + */
>> + val &= ~(CFGCHIP2_RESET | CFGCHIP2_PHYPWRDN);
>> + val |= CFGCHIP2_PHY_PLLON;
>> +
>> + writel(val, DA8XX_SYSCFG0_VIRT(DA8XX_CFGCHIP2_REG));
>> +
>> + pr_info("Waiting for USB 2.0 PHY clock good...\n");
>> + while (!(readl(DA8XX_SYSCFG0_VIRT(DA8XX_CFGCHIP2_REG))
>> + & CFGCHIP2_PHYCLKGD))
>> + cpu_relax();
>> +}
>
> Looks like this is pretty much going to be the same code repeated for
> DA850 and DA830. So can we move these to a common file like da8xx-usb.c?
> You can even register these USB clocks from that file by using
> clkdev_add() and clk_register(). This way they can remain to be file local.
>

I knew someone was going to say that. ;-) Thanks for the suggestion of
clkdev_add() and clk_register(), I had not considered that but it sounds
like a good idea and will take care of the ioremap problem too.

2016-03-23 17:56:20

[permalink] [raw]

Subject: Re: [PATCH v2 04/11] ARM: davinci: da8xx: add usb phy clocks

On Wednesday 23 March 2016 11:15 PM, David Lechner wrote:
> On 03/23/2016 11:56 AM, Sekhar Nori wrote:
>>>
>>> +static struct clk usb_ref_clk = {
>>> + .name = "usb_ref_clk",
>>> + .rate = 48000000,
>>> + .set_rate = davinci_simple_set_rate,
>>> +};
>>
>> can we call this usb_refclkin so it matches the TRM name? Also, should
>> this node be not be coming through individual board files as the rate
>> depends on what is connected to the usb_refclkin pin? Or is the
>> expectation that boards will call clk_set_rate() on this clock to the
>> correct value? If yes, I think it is misleading to populate the .rate
>> here.
>
> You are right. When I did this, I was looking at USB 1.1 only, which
> MUST be 48MHz. However, this can be used for USB 2.0 which can accept a
> number of rates.
>
> However, even the main reference oscillator in da850.c has the rate hard
> coded in da850.c (DA850_REF_FREQ).

:)

>
> The clock initialization will fail if a clock does not have a parent or
> a rate, so we have to give it a default rate since it is an external
> clock and has no parent. So, I think 48MHz makes sense for a default
> value. Most boards will probably not be using this clock anyway, but
> rather the PLL in the USB 2.0 PHY.

Alright, I guess the only change is to call it usb_refclkin

>
>
>>> +
>>> + pr_info("Waiting for USB 2.0 PHY clock good...\n");
>>> + while (!(readl(DA8XX_SYSCFG0_VIRT(DA8XX_CFGCHIP2_REG))
>>> + & CFGCHIP2_PHYCLKGD))
>>> + cpu_relax();
>>
>> I guess this is copying some earlier code, but still, it will be nice to
>> see a timeout mechanism here, rather than loop endlessly.
>
> Do you have a suggestion on how to do this?

Simplest would be to use a udelay(1) inside the loop and count a
specific number of times (sufficiently large to not cause false
negatives and sufficiently small so as to not appear that board is
frozen forever).

Thanks,
Sekhar

2016-03-23 17:56:45

[permalink] [raw]

Subject: Re: [PATCH v2 05/11] dt-bindings: Add bindings for phy-da8xx-usb

On 03/23/2016 12:06 PM, Sekhar Nori wrote:
> On Thursday 17 March 2016 07:56 AM, David Lechner wrote:
>> Device tree binding for new phy-da8xx-usb driver.
>>
>> Signed-off-by: David Lechner <[email protected]>
>> ---
>>
>> v2 changes: This is new patch in v2.
>>
>>
>> .../devicetree/bindings/phy/phy-da8xx-usb.txt | 34 ++++++++++++++++++++++
>> 1 file changed, 34 insertions(+)
>> create mode 100644 Documentation/devicetree/bindings/phy/phy-da8xx-usb.txt
>>
>> diff --git a/Documentation/devicetree/bindings/phy/phy-da8xx-usb.txt b/Documentation/devicetree/bindings/phy/phy-da8xx-usb.txt
>> new file mode 100644
>> index 0000000..ed6b710
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/phy/phy-da8xx-usb.txt
>> @@ -0,0 +1,34 @@
>> +TI DaVinci DA8XX USB PHY
>> +
>> +Required properties:
>> + - compatible: must be "ti,da830-usbphy".
>> + - #phy-cells: must be 1.
>> + - reg : Address and length of the CFGCHIP2 register.
>
> I am not sure passing CFGCHIP2 register as reg property to the phy is
> future proof. At some point, we do want to move to common clock
> framework and at that point USB clocks controlled by CFGCHIP2 will be a
> separate driver needing access to the same register.
>
> So I think the CFGCHIP2 access in USB phy driver should happen through a
> syscon phandle. This needs to happen now, not later since we cannot
> break DT backward-compatibility.
>

I think using "syscon" for the CFGCHIP registers makes sense (based on
my minimal experience). Would we want one "syscon" device node that
includes all of the CFGCHIP registers or one each?

Something like this?

cfgchip@1417C {
compatible = "ti,da830-cfgchip", "syscon";
reg = <1417C 20>;
}

or this?

cfgchip0@1417C {
compatible = "ti,da830-cfgchip0", "syscon";
reg = <1417C 4>;
}

cfgchip1@14180 {
compatible = "ti,da830-cfgchip1", "syscon";
reg = <14180 4>;
}

etc.

-or-

Would it be OK if the PHY driver registered clocks? I'm guessing this
falls into the category of "not such a good idea".

2016-03-23 18:07:06

[permalink] [raw]

Subject: Re: [PATCH v2 06/11] phy: da8xx-usb: new driver for DA8XX SoC USB PHY

On 03/23/2016 12:21 PM, Sekhar Nori wrote:
>> +/* DA8xx CFGCHIP2 (USB PHY Control) register bits */
>> +#define PHYCLKGD (1 << 17)
>> +#define VBUSSENSE (1 << 16)
>> +#define RESET (1 << 15)
>> +#define OTGMODE_MASK (3 << 13)
>> +#define NO_OVERRIDE (0 << 13)
>> +#define FORCE_HOST (1 << 13)
>> +#define FORCE_DEVICE (2 << 13)
>> +#define FORCE_HOST_VBUS_LOW (3 << 13)
>> +#define USB1PHYCLKMUX (1 << 12)
>> +#define USB2PHYCLKMUX (1 << 11)
>> +#define PHYPWRDN (1 << 10)
>> +#define OTGPWRDN (1 << 9)
>> +#define DATPOL (1 << 8)
>> +#define USB1SUSPENDM (1 << 7)
>> +#define PHY_PLLON (1 << 6)
>> +#define SESENDEN (1 << 5)
>> +#define VBDTCTEN (1 << 4)
>> +#define REFFREQ_MASK (0xf << 0)
>> +#define REFFREQ_12MHZ (1 << 0)
>> +#define REFFREQ_24MHZ (2 << 0)
>> +#define REFFREQ_48MHZ (3 << 0)
>> +#define REFFREQ_19_2MHZ (4 << 0)
>> +#define REFFREQ_38_4MHZ (5 << 0)
>> +#define REFFREQ_13MHZ (6 << 0)
>> +#define REFFREQ_26MHZ (7 << 0)
>> +#define REFFREQ_20MHZ (8 << 0)
>> +#define REFFREQ_40MHZ (9 << 0)
>
> Many of these register bits are unused. I guess opinion varies around
> this, but I get confused with unnecessary bit definitions and register
> offsets. I tend to search for it and its sort of disappointing to see
> that its basically unused. Of course, you should wait for PHY
> maintainers preference.

My thinking was that since this driver *owns* the CFGCHIP2 register that
is would eventually register clocks using the common clock framework, in
which case, it would use many of these registers.

But, based on feedback on another commit, if we go the syscon route,
then yes, I will remove the unused values.

>> +EXPORT_SYMBOL_GPL(da8xx_usb20_phy_set_mode);
>
> Hmm, since this driver exports this symbol, I think it should also
> provide an include file in include/linux/phy for users of the symbol. Or
> perhaps there should be a generic API around this since it looks like
> most USB phys will need something similar?
>

The reason I didn't make a header file is that this function is only
meant to be use in one place and would likely cause a crash if used
anywhere else.

I agree that it would be nice if the generic phy driver had a mechanism
for user-defined callbacks such as this, however, I think the scope of
this patch series has grown enough already.

2016-03-23 18:32:34

[permalink] [raw]

Subject: Re: [PATCH v2 03/11] ARM: davinci: da850: use clk->set_parent for async3

On 03/23/2016 12:29 PM, Sekhar Nori wrote:
>
> Alright, I guess 'can be called' in the comment should have used
> stronger language :) How about late registration of USB clocks as I
> suggested. It should also help consolidate code across da830 and da850.
>

What about the new async3 clock? It will require later registration too,
but it has many children that depend on it.

2016-03-24 13:45:57

[permalink] [raw]

Subject: Re: [PATCH v2 03/11] ARM: davinci: da850: use clk->set_parent for async3

On Thursday 24 March 2016 12:02 AM, David Lechner wrote:
> On 03/23/2016 12:29 PM, Sekhar Nori wrote:
>>
>> Alright, I guess 'can be called' in the comment should have used
>> stronger language :) How about late registration of USB clocks as I
>> suggested. It should also help consolidate code across da830 and da850.
>>
>
> What about the new async3 clock? It will require later registration too,
> but it has many children that depend on it.

I guess that was the reason why it was done the way it was. Can we leave
that alone for now (not use the new set_parent interface)? You don't
really need that to be changed for USB functionality, right?

Thanks,
sekhar

2016-03-24 14:02:23

by David Laight

[permalink] [raw]

Subject: RE: [PATCH v2 06/11] phy: da8xx-usb: new driver for DA8XX SoC USB PHY

From: David Lechner
> Sent: 23 March 2016 18:07
> On 03/23/2016 12:21 PM, Sekhar Nori wrote:
> >> +/* DA8xx CFGCHIP2 (USB PHY Control) register bits */
> >> +#define PHYCLKGD (1 << 17)
> >> +#define VBUSSENSE (1 << 16)
> >> +#define RESET (1 << 15)
> >> +#define OTGMODE_MASK (3 << 13)
> >> +#define NO_OVERRIDE (0 << 13)
> >> +#define FORCE_HOST (1 << 13)
> >> +#define FORCE_DEVICE (2 << 13)
> >> +#define FORCE_HOST_VBUS_LOW (3 << 13)

I think I'd define the above as:
#define OTG_MODE(x) ((x) << 13)
#define OTGMODE_MASK OTG_MODE(3)
#define NO_OVERRIDE OTG_MODE(0)
#define FORCE_HOST OTG_MODE(1)
#define FORCE_DEVICE OTG_MODE(2)
#define FORCE_HOST_VBUS_LOW OTG_MODE(3)
Then realise that all the names need changing (to include OTG).

> >> +#define USB1PHYCLKMUX (1 << 12)
> >> +#define USB2PHYCLKMUX (1 << 11)
> >> +#define PHYPWRDN (1 << 10)
> >> +#define OTGPWRDN (1 << 9)
> >> +#define DATPOL (1 << 8)
> >> +#define USB1SUSPENDM (1 << 7)
> >> +#define PHY_PLLON (1 << 6)
> >> +#define SESENDEN (1 << 5)
> >> +#define VBDTCTEN (1 << 4)
> >> +#define REFFREQ_MASK (0xf << 0)
> >> +#define REFFREQ_12MHZ (1 << 0)
> >> +#define REFFREQ_24MHZ (2 << 0)
> >> +#define REFFREQ_48MHZ (3 << 0)
> >> +#define REFFREQ_19_2MHZ (4 << 0)
> >> +#define REFFREQ_38_4MHZ (5 << 0)
> >> +#define REFFREQ_13MHZ (6 << 0)
> >> +#define REFFREQ_26MHZ (7 << 0)
> >> +#define REFFREQ_20MHZ (8 << 0)
> >> +#define REFFREQ_40MHZ (9 << 0)

I'd define these similarly to the OTGxxx values.

> > Many of these register bits are unused. I guess opinion varies around
> > this, but I get confused with unnecessary bit definitions and register
> > offsets. I tend to search for it and its sort of disappointing to see
> > that its basically unused. Of course, you should wait for PHY
> > maintainers preference.

It can be useful to see what isn't being set.
Sometimes this can be very useful when making changes to a driver.
For status values it is particularly important to know what all the bits mean.

> My thinking was that since this driver *owns* the CFGCHIP2 register that
> is would eventually register clocks using the common clock framework, in
> which case, it would use many of these registers.
>
> But, based on feedback on another commit, if we go the syscon route,
> then yes, I will remove the unused values.
>
>
> >> +EXPORT_SYMBOL_GPL(da8xx_usb20_phy_set_mode);
> >
> > Hmm, since this driver exports this symbol, I think it should also
> > provide an include file in include/linux/phy for users of the symbol. Or
> > perhaps there should be a generic API around this since it looks like
> > most USB phys will need something similar?
> >
>
> The reason I didn't make a header file is that this function is only
> meant to be use in one place and would likely cause a crash if used
> anywhere else.

And a compilation error if compiled with -Wmissing-prototypes.

Sounds like you need a header file just for this driver's 'private' exports.

IMHO .c files shouldn't contain 'extern' anywhere.
I removed a load from some local code and found quite a few lurking bugs
where the types didn't match.

David

2016-04-01 13:18:20

by Kishon Vijay Abraham I

[permalink] [raw]

Subject: Re: [PATCH v2 06/11] phy: da8xx-usb: new driver for DA8XX SoC USB PHY

Hi,

On Thursday 17 March 2016 07:56 AM, David Lechner wrote:
> This is a new phy driver for the SoC USB controllers on the TI DA8XX
> family of microcontrollers. The USB 1.1 PHY is just a simple on/off.
> The USB 2.0 PHY also allows overriding the VBUS and ID pins.
>
> Signed-off-by: David Lechner <[email protected]>
> ---
>
> v2 changes: This is new patch in v2.
>
>
>
> drivers/phy/Kconfig | 9 ++
> drivers/phy/Makefile | 1 +
> drivers/phy/phy-da8xx-usb.c | 295 ++++++++++++++++++++++++++++++++++++++++++++
> 3 files changed, 305 insertions(+)
> create mode 100644 drivers/phy/phy-da8xx-usb.c
>
> diff --git a/drivers/phy/Kconfig b/drivers/phy/Kconfig
> index 26566db..a6060ea 100644
> --- a/drivers/phy/Kconfig
> +++ b/drivers/phy/Kconfig
> @@ -35,6 +35,15 @@ config ARMADA375_USBCLUSTER_PHY
> depends on OF && HAS_IOMEM
> select GENERIC_PHY
>
> +config PHY_DA8XX_USB
> + tristate "TI DA8XX USB PHY Driver"
> + depends on ARCH_DAVINCI_DA8XX
> + select GENERIC_PHY
> + help
> + Enable this to support the USB PHY on DA8XX SoCs.
> +
> + This driver controls both the USB 1.1 PHY and the USB 2.0 PHY.
> +
> config PHY_DM816X_USB
> tristate "TI dm816x USB PHY driver"
> depends on ARCH_OMAP2PLUS
> diff --git a/drivers/phy/Makefile b/drivers/phy/Makefile
> index 24596a9..722e01c 100644
> --- a/drivers/phy/Makefile
> +++ b/drivers/phy/Makefile
> @@ -5,6 +5,7 @@
> obj-$(CONFIG_GENERIC_PHY) += phy-core.o
> obj-$(CONFIG_PHY_BERLIN_USB) += phy-berlin-usb.o
> obj-$(CONFIG_PHY_BERLIN_SATA) += phy-berlin-sata.o
> +obj-$(CONFIG_PHY_DA8XX_USB) += phy-da8xx-usb.o
> obj-$(CONFIG_PHY_DM816X_USB) += phy-dm816x-usb.o
> obj-$(CONFIG_ARMADA375_USBCLUSTER_PHY) += phy-armada375-usb2.o
> obj-$(CONFIG_BCM_KONA_USB2_PHY) += phy-bcm-kona-usb2.o
> diff --git a/drivers/phy/phy-da8xx-usb.c b/drivers/phy/phy-da8xx-usb.c
> new file mode 100644
> index 0000000..93a5f4d
> --- /dev/null
> +++ b/drivers/phy/phy-da8xx-usb.c
> @@ -0,0 +1,295 @@
> +/*
> + * phy-da8xx-usb - TI DaVinci DA8XX USB PHY driver
> + *
> + * Copyright (C) 2016 David Lechner <[email protected]>
> + *
> + * 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/io.h>
> +#include <linux/of.h>
> +#include <linux/module.h>
> +#include <linux/phy/phy.h>
> +#include <linux/platform_device.h>
> +#include <linux/usb/gadget.h>
> +#include <linux/usb/musb.h>
> +#include <linux/usb/otg.h>
> +
> +/* DA8xx CFGCHIP2 (USB PHY Control) register bits */
> +#define PHYCLKGD (1 << 17)
> +#define VBUSSENSE (1 << 16)
> +#define RESET (1 << 15)
> +#define OTGMODE_MASK (3 << 13)
> +#define NO_OVERRIDE (0 << 13)
> +#define FORCE_HOST (1 << 13)
> +#define FORCE_DEVICE (2 << 13)
> +#define FORCE_HOST_VBUS_LOW (3 << 13)
> +#define USB1PHYCLKMUX (1 << 12)
> +#define USB2PHYCLKMUX (1 << 11)
> +#define PHYPWRDN (1 << 10)
> +#define OTGPWRDN (1 << 9)
> +#define DATPOL (1 << 8)
> +#define USB1SUSPENDM (1 << 7)
> +#define PHY_PLLON (1 << 6)
> +#define SESENDEN (1 << 5)
> +#define VBDTCTEN (1 << 4)
> +#define REFFREQ_MASK (0xf << 0)
> +#define REFFREQ_12MHZ (1 << 0)
> +#define REFFREQ_24MHZ (2 << 0)
> +#define REFFREQ_48MHZ (3 << 0)
> +#define REFFREQ_19_2MHZ (4 << 0)
> +#define REFFREQ_38_4MHZ (5 << 0)
> +#define REFFREQ_13MHZ (6 << 0)
> +#define REFFREQ_26MHZ (7 << 0)
> +#define REFFREQ_20MHZ (8 << 0)
> +#define REFFREQ_40MHZ (9 << 0)
> +
> +struct da8xx_usbphy {
> + struct phy_provider *phy_provider;
> + struct phy *usb11_phy;
> + struct phy *usb20_phy;
> + struct clk *usb11_clk;
> + struct clk *usb20_clk;
> + void __iomem *phy_ctrl;
> +};
> +
> +static inline u32 da8xx_usbphy_readl(void __iomem *base)
> +{
> + return readl(base);
> +}
> +
> +static inline void da8xx_usbphy_writel(void __iomem *base, u32 value)
> +{
> + writel(value, base);
> +}
> +
> +static int da8xx_usb11_phy_init(struct phy *phy)
> +{
> + struct da8xx_usbphy *d_phy = phy_get_drvdata(phy);
> + int ret;
> + u32 val;
> +
> + ret = clk_prepare_enable(d_phy->usb11_clk);
> + if (ret)
> + return ret;
> +
> + val = da8xx_usbphy_readl(d_phy->phy_ctrl);
> + val |= USB1SUSPENDM;
> + da8xx_usbphy_writel(d_phy->phy_ctrl, val);
> +
> + return 0;
> +}
> +
> +static int da8xx_usb11_phy_shutdown(struct phy *phy)
> +{
> + struct da8xx_usbphy *d_phy = phy_get_drvdata(phy);
> + u32 val;
> +
> + val = da8xx_usbphy_readl(d_phy->phy_ctrl);
> + val &= ~USB1SUSPENDM;
> + da8xx_usbphy_writel(d_phy->phy_ctrl, val);
> +
> + clk_disable_unprepare(d_phy->usb11_clk);
> +
> + return 0;
> +}
> +
> +static const struct phy_ops da8xx_usb11_phy_ops = {
> + .power_on = da8xx_usb11_phy_init,
> + .power_off = da8xx_usb11_phy_shutdown,
> + .owner = THIS_MODULE,
> +};
> +
> +static int da8xx_usb20_phy_init(struct phy *phy)
> +{
> + struct da8xx_usbphy *d_phy = phy_get_drvdata(phy);
> + int ret;
> + u32 val;
> +
> + ret = clk_prepare_enable(d_phy->usb20_clk);
> + if (ret)
> + return ret;
> +
> + val = da8xx_usbphy_readl(d_phy->phy_ctrl);
> + val &= ~OTGPWRDN;
> + da8xx_usbphy_writel(d_phy->phy_ctrl, val);
> +
> + return 0;
> +}
> +
> +static int da8xx_usb20_phy_shutdown(struct phy *phy)
> +{
> + struct da8xx_usbphy *d_phy = phy_get_drvdata(phy);
> + u32 val;
> +
> + val = da8xx_usbphy_readl(d_phy->phy_ctrl);
> + val |= OTGPWRDN;
> + da8xx_usbphy_writel(d_phy->phy_ctrl, val);
> +
> + clk_disable_unprepare(d_phy->usb20_clk);
> +
> + return 0;
> +}
> +
> +static const struct phy_ops da8xx_usb20_phy_ops = {
> + .power_on = da8xx_usb20_phy_init,
> + .power_off = da8xx_usb20_phy_shutdown,
> + .owner = THIS_MODULE,
> +};
> +
> +int da8xx_usb20_phy_set_mode(struct phy *phy, enum musb_mode mode)
> +{
> + struct da8xx_usbphy *d_phy = phy_get_drvdata(phy);
> + u32 val;
> +
> + val = da8xx_usbphy_readl(d_phy->phy_ctrl);
> +
> + val &= ~OTGMODE_MASK;
> + switch (mode) {
> + case MUSB_HOST: /* Force VBUS valid, ID = 0 */
> + val |= FORCE_HOST;
> + break;
> + case MUSB_PERIPHERAL: /* Force VBUS valid, ID = 1 */
> + val |= FORCE_DEVICE;
> + break;
> + case MUSB_OTG: /* Don't override the VBUS/ID comparators */
> + val |= NO_OVERRIDE;
> + break;
> + default:
> + return -EINVAL;
> + }
> +
> + da8xx_usbphy_writel(d_phy->phy_ctrl, val);
> +
> + return 0;
> +}
> +EXPORT_SYMBOL_GPL(da8xx_usb20_phy_set_mode);

Don't prefer export symbols from PHY driver. That'll create unnecessary
dependencies between the controller and the PHY.

I think it'll be better to create a new attribute and use it?

8<============

diff --git a/include/linux/phy/phy.h b/include/linux/phy/phy.h
index 8cf05e3..3d92b9b 100644
--- a/include/linux/phy/phy.h
+++ b/include/linux/phy/phy.h
@@ -22,6 +22,13 @@

struct phy;

+enum phy_mode {
+ PHY_MODE_INVALID,
+ PHY_MODE_USB_HOST,
+ PHY_MODE_USB_DEVICE,
+ PHY_MODE_USB_OTG,
+};
+
/**
* struct phy_ops - set of function pointers for performing phy operations
* @init: operation to be performed for initializing phy
@@ -44,6 +51,7 @@ struct phy_ops {
*/
struct phy_attrs {
u32 bus_width;
+ enum phy_mode mode;
};

/**
@@ -127,6 +135,14 @@ static inline void phy_set_bus_width(struct phy *phy, int
bus_width)
{
phy->attrs.bus_width = bus_width;
}
+static inline enum phy_mode phy_get_mode(struct phy *phy)
+{
+ return phy->attrs.mode;
+}
+static inline void phy_set_mode(struct phy *phy, enum phy_mode mode)
+{
+ phy->attrs.mode = mode;
+}
struct phy *phy_get(struct device *dev, const char *string);
struct phy *phy_optional_get(struct device *dev, const char *string);
struct phy *devm_phy_get(struct device *dev, const char *string);
@@ -234,6 +250,16 @@ static inline void phy_set_bus_width(struct phy *phy, int
bus_width)
return;
}

+static inline enum phy_mode phy_get_mode(struct phy *phy)
+{
+ return PHY_MODE_INVALID;
+}
+
+static inline void phy_set_mode(struct phy *phy, enum phy_mode mode)
+{
+ return;
+}
+
static inline struct phy *phy_get(struct device *dev, const char *string)
{
return ERR_PTR(-ENOSYS);

=====================>8

Thanks
Kishon

> +
> +static struct phy *da8xx_usbphy_of_xlate(struct device *dev,
> + struct of_phandle_args *args)
> +{
> + struct da8xx_usbphy *d_phy = dev_get_drvdata(dev);
> +
> + if (!d_phy)
> + return ERR_PTR(-ENODEV);
> +
> + switch (args->args[0]) {
> + case 1:
> + return d_phy->usb11_phy;
> + case 2:
> + return d_phy->usb20_phy;
> + default:
> + return ERR_PTR(-EINVAL);
> + }
> +}
> +
> +static int da8xx_usbphy_probe(struct platform_device *pdev)
> +{
> + struct device *dev = &pdev->dev;
> + struct device_node *node = dev->of_node;
> + struct da8xx_usbphy *d_phy;
> + struct resource *res;
> +
> + d_phy = devm_kzalloc(dev, sizeof(*d_phy), GFP_KERNEL);
> + if (!d_phy)
> + return -ENOMEM;
> +
> + res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> + d_phy->phy_ctrl = devm_ioremap_resource(dev, res);
> + if (IS_ERR(d_phy->phy_ctrl)) {
> + dev_err(dev, "Failed to map resource.\n");
> + return PTR_ERR(d_phy->phy_ctrl);
> + }
> +
> + d_phy->usb11_clk = devm_clk_get(dev, "usb11_phy");
> + if (IS_ERR(d_phy->usb11_clk)) {
> + dev_err(dev, "Failed to get usb11_phy clock.\n");
> + return PTR_ERR(d_phy->usb11_clk);
> + }
> +
> + d_phy->usb20_clk = devm_clk_get(dev, "usb20_phy");
> + if (IS_ERR(d_phy->usb20_clk)) {
> + dev_err(dev, "Failed to get usb20_phy clock.\n");
> + return PTR_ERR(d_phy->usb20_clk);
> + }
> +
> + d_phy->usb11_phy = devm_phy_create(dev, node, &da8xx_usb11_phy_ops);
> + if (IS_ERR(d_phy->usb11_phy)) {
> + dev_err(dev, "Failed to create usb11 phy.\n");
> + return PTR_ERR(d_phy->usb11_phy);
> + }
> +
> + d_phy->usb20_phy = devm_phy_create(dev, node, &da8xx_usb20_phy_ops);
> + if (IS_ERR(d_phy->usb20_phy)) {
> + dev_err(dev, "Failed to create usb20 phy.\n");
> + return PTR_ERR(d_phy->usb20_phy);
> + }
> +
> + platform_set_drvdata(pdev, d_phy);
> + phy_set_drvdata(d_phy->usb11_phy, d_phy);
> + phy_set_drvdata(d_phy->usb20_phy, d_phy);
> +
> + if (node) {
> + d_phy->phy_provider = devm_of_phy_provider_register(dev,
> + da8xx_usbphy_of_xlate);
> + if (IS_ERR(d_phy->phy_provider)) {
> + dev_err(dev, "Failed to create phy provider.\n");
> + return PTR_ERR(d_phy->phy_provider);
> + }
> + } else {
> + int ret;
> +
> + ret = phy_create_lookup(d_phy->usb11_phy, "usbphy", "ohci.0");
> + if (ret)
> + dev_warn(dev, "Failed to create usb11 phy lookup .\n");
> + ret = phy_create_lookup(d_phy->usb20_phy, "usbphy", "musb-da8xx");
> + if (ret)
> + dev_warn(dev, "Failed to create usb20 phy lookup .\n");
> + }
> +
> + return 0;
> +}
> +
> +static int da8xx_usbphy_remove(struct platform_device *pdev)
> +{
> + struct da8xx_usbphy *d_phy = platform_get_drvdata(pdev);
> +
> + if (!pdev->dev.of_node) {
> + phy_remove_lookup(d_phy->usb20_phy, "usbphy", "musb-da8xx");
> + phy_remove_lookup(d_phy->usb11_phy, "usbphy", "ohci.0");
> + }
> +
> + return 0;
> +}
> +
> +static const struct of_device_id da8xx_usbphy_ids[] = {
> + { .compatible = "ti,da830-usbphy" },
> + { }
> +};
> +MODULE_DEVICE_TABLE(of, da8xx_usbphy_ids);
> +
> +static struct platform_driver da8xx_usbphy_driver = {
> + .probe = da8xx_usbphy_probe,
> + .remove = da8xx_usbphy_remove,
> + .driver = {
> + .name = "da8xx-usbphy",
> + .of_match_table = da8xx_usbphy_ids,
> + },
> +};
> +
> +module_platform_driver(da8xx_usbphy_driver);
> +
> +MODULE_ALIAS("platform:da8xx-usbphy");
> +MODULE_AUTHOR("David Lechner <[email protected]>");
> +MODULE_DESCRIPTION("TI DA8XX USB PHY driver");
> +MODULE_LICENSE("GPL v2");
>

2016-04-01 14:47:05

by Bin Liu

[permalink] [raw]

Subject: Re: [PATCH v2 06/11] phy: da8xx-usb: new driver for DA8XX SoC USB PHY

Hi,

On Fri, Apr 01, 2016 at 06:46:28PM +0530, Kishon Vijay Abraham I wrote:
> Hi,
>
> On Thursday 17 March 2016 07:56 AM, David Lechner wrote:
> > This is a new phy driver for the SoC USB controllers on the TI DA8XX
> > family of microcontrollers. The USB 1.1 PHY is just a simple on/off.
> > The USB 2.0 PHY also allows overriding the VBUS and ID pins.
> >
> > Signed-off-by: David Lechner <[email protected]>
> > ---
> >
> > v2 changes: This is new patch in v2.
> >
> >
> >
> > drivers/phy/Kconfig | 9 ++
> > drivers/phy/Makefile | 1 +
> > drivers/phy/phy-da8xx-usb.c | 295 ++++++++++++++++++++++++++++++++++++++++++++
> > 3 files changed, 305 insertions(+)
> > create mode 100644 drivers/phy/phy-da8xx-usb.c
> >
> > diff --git a/drivers/phy/Kconfig b/drivers/phy/Kconfig
> > index 26566db..a6060ea 100644
> > --- a/drivers/phy/Kconfig
> > +++ b/drivers/phy/Kconfig
> > @@ -35,6 +35,15 @@ config ARMADA375_USBCLUSTER_PHY
> > depends on OF && HAS_IOMEM
> > select GENERIC_PHY
> >
> > +config PHY_DA8XX_USB
> > + tristate "TI DA8XX USB PHY Driver"
> > + depends on ARCH_DAVINCI_DA8XX
> > + select GENERIC_PHY
> > + help
> > + Enable this to support the USB PHY on DA8XX SoCs.
> > +
> > + This driver controls both the USB 1.1 PHY and the USB 2.0 PHY.
> > +
> > config PHY_DM816X_USB
> > tristate "TI dm816x USB PHY driver"
> > depends on ARCH_OMAP2PLUS
> > diff --git a/drivers/phy/Makefile b/drivers/phy/Makefile
> > index 24596a9..722e01c 100644
> > --- a/drivers/phy/Makefile
> > +++ b/drivers/phy/Makefile
> > @@ -5,6 +5,7 @@
> > obj-$(CONFIG_GENERIC_PHY) += phy-core.o
> > obj-$(CONFIG_PHY_BERLIN_USB) += phy-berlin-usb.o
> > obj-$(CONFIG_PHY_BERLIN_SATA) += phy-berlin-sata.o
> > +obj-$(CONFIG_PHY_DA8XX_USB) += phy-da8xx-usb.o
> > obj-$(CONFIG_PHY_DM816X_USB) += phy-dm816x-usb.o
> > obj-$(CONFIG_ARMADA375_USBCLUSTER_PHY) += phy-armada375-usb2.o
> > obj-$(CONFIG_BCM_KONA_USB2_PHY) += phy-bcm-kona-usb2.o
> > diff --git a/drivers/phy/phy-da8xx-usb.c b/drivers/phy/phy-da8xx-usb.c
> > new file mode 100644
> > index 0000000..93a5f4d
> > --- /dev/null
> > +++ b/drivers/phy/phy-da8xx-usb.c
> > @@ -0,0 +1,295 @@
> > +/*
> > + * phy-da8xx-usb - TI DaVinci DA8XX USB PHY driver
> > + *
> > + * Copyright (C) 2016 David Lechner <[email protected]>
> > + *
> > + * 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/io.h>
> > +#include <linux/of.h>
> > +#include <linux/module.h>
> > +#include <linux/phy/phy.h>
> > +#include <linux/platform_device.h>
> > +#include <linux/usb/gadget.h>
> > +#include <linux/usb/musb.h>
> > +#include <linux/usb/otg.h>
> > +
> > +/* DA8xx CFGCHIP2 (USB PHY Control) register bits */
> > +#define PHYCLKGD (1 << 17)
> > +#define VBUSSENSE (1 << 16)
> > +#define RESET (1 << 15)
> > +#define OTGMODE_MASK (3 << 13)
> > +#define NO_OVERRIDE (0 << 13)
> > +#define FORCE_HOST (1 << 13)
> > +#define FORCE_DEVICE (2 << 13)
> > +#define FORCE_HOST_VBUS_LOW (3 << 13)
> > +#define USB1PHYCLKMUX (1 << 12)
> > +#define USB2PHYCLKMUX (1 << 11)
> > +#define PHYPWRDN (1 << 10)
> > +#define OTGPWRDN (1 << 9)
> > +#define DATPOL (1 << 8)
> > +#define USB1SUSPENDM (1 << 7)
> > +#define PHY_PLLON (1 << 6)
> > +#define SESENDEN (1 << 5)
> > +#define VBDTCTEN (1 << 4)
> > +#define REFFREQ_MASK (0xf << 0)
> > +#define REFFREQ_12MHZ (1 << 0)
> > +#define REFFREQ_24MHZ (2 << 0)
> > +#define REFFREQ_48MHZ (3 << 0)
> > +#define REFFREQ_19_2MHZ (4 << 0)
> > +#define REFFREQ_38_4MHZ (5 << 0)
> > +#define REFFREQ_13MHZ (6 << 0)
> > +#define REFFREQ_26MHZ (7 << 0)
> > +#define REFFREQ_20MHZ (8 << 0)
> > +#define REFFREQ_40MHZ (9 << 0)
> > +
> > +struct da8xx_usbphy {
> > + struct phy_provider *phy_provider;
> > + struct phy *usb11_phy;
> > + struct phy *usb20_phy;
> > + struct clk *usb11_clk;
> > + struct clk *usb20_clk;
> > + void __iomem *phy_ctrl;
> > +};
> > +
> > +static inline u32 da8xx_usbphy_readl(void __iomem *base)
> > +{
> > + return readl(base);
> > +}
> > +
> > +static inline void da8xx_usbphy_writel(void __iomem *base, u32 value)
> > +{
> > + writel(value, base);
> > +}
> > +
> > +static int da8xx_usb11_phy_init(struct phy *phy)
> > +{
> > + struct da8xx_usbphy *d_phy = phy_get_drvdata(phy);
> > + int ret;
> > + u32 val;
> > +
> > + ret = clk_prepare_enable(d_phy->usb11_clk);
> > + if (ret)
> > + return ret;
> > +
> > + val = da8xx_usbphy_readl(d_phy->phy_ctrl);
> > + val |= USB1SUSPENDM;
> > + da8xx_usbphy_writel(d_phy->phy_ctrl, val);
> > +
> > + return 0;
> > +}
> > +
> > +static int da8xx_usb11_phy_shutdown(struct phy *phy)
> > +{
> > + struct da8xx_usbphy *d_phy = phy_get_drvdata(phy);
> > + u32 val;
> > +
> > + val = da8xx_usbphy_readl(d_phy->phy_ctrl);
> > + val &= ~USB1SUSPENDM;
> > + da8xx_usbphy_writel(d_phy->phy_ctrl, val);
> > +
> > + clk_disable_unprepare(d_phy->usb11_clk);
> > +
> > + return 0;
> > +}
> > +
> > +static const struct phy_ops da8xx_usb11_phy_ops = {
> > + .power_on = da8xx_usb11_phy_init,
> > + .power_off = da8xx_usb11_phy_shutdown,
> > + .owner = THIS_MODULE,
> > +};
> > +
> > +static int da8xx_usb20_phy_init(struct phy *phy)
> > +{
> > + struct da8xx_usbphy *d_phy = phy_get_drvdata(phy);
> > + int ret;
> > + u32 val;
> > +
> > + ret = clk_prepare_enable(d_phy->usb20_clk);
> > + if (ret)
> > + return ret;
> > +
> > + val = da8xx_usbphy_readl(d_phy->phy_ctrl);
> > + val &= ~OTGPWRDN;
> > + da8xx_usbphy_writel(d_phy->phy_ctrl, val);
> > +
> > + return 0;
> > +}
> > +
> > +static int da8xx_usb20_phy_shutdown(struct phy *phy)
> > +{
> > + struct da8xx_usbphy *d_phy = phy_get_drvdata(phy);
> > + u32 val;
> > +
> > + val = da8xx_usbphy_readl(d_phy->phy_ctrl);
> > + val |= OTGPWRDN;
> > + da8xx_usbphy_writel(d_phy->phy_ctrl, val);
> > +
> > + clk_disable_unprepare(d_phy->usb20_clk);
> > +
> > + return 0;
> > +}
> > +
> > +static const struct phy_ops da8xx_usb20_phy_ops = {
> > + .power_on = da8xx_usb20_phy_init,
> > + .power_off = da8xx_usb20_phy_shutdown,
> > + .owner = THIS_MODULE,
> > +};
> > +
> > +int da8xx_usb20_phy_set_mode(struct phy *phy, enum musb_mode mode)
> > +{
> > + struct da8xx_usbphy *d_phy = phy_get_drvdata(phy);
> > + u32 val;
> > +
> > + val = da8xx_usbphy_readl(d_phy->phy_ctrl);
> > +
> > + val &= ~OTGMODE_MASK;
> > + switch (mode) {
> > + case MUSB_HOST: /* Force VBUS valid, ID = 0 */
> > + val |= FORCE_HOST;
> > + break;
> > + case MUSB_PERIPHERAL: /* Force VBUS valid, ID = 1 */
> > + val |= FORCE_DEVICE;
> > + break;
> > + case MUSB_OTG: /* Don't override the VBUS/ID comparators */
> > + val |= NO_OVERRIDE;
> > + break;
> > + default:
> > + return -EINVAL;
> > + }
> > +
> > + da8xx_usbphy_writel(d_phy->phy_ctrl, val);
> > +
> > + return 0;
> > +}
> > +EXPORT_SYMBOL_GPL(da8xx_usb20_phy_set_mode);
>
> Don't prefer export symbols from PHY driver. That'll create unnecessary
> dependencies between the controller and the PHY.

Agreed.

>
> I think it'll be better to create a new attribute and use it?

Another simpler option is to not support _set_mode() for DA8xx, and the
phy driver set the otgmode bit in probe() based on dr_mode of the
controller.

Regards,
-Bin.

2016-04-01 16:02:27

[permalink] [raw]

Subject: Re: [PATCH v2 06/11] phy: da8xx-usb: new driver for DA8XX SoC USB PHY

On 04/01/2016 09:45 AM, Bin Liu wrote:
>>> +EXPORT_SYMBOL_GPL(da8xx_usb20_phy_set_mode);
>>
>> Don't prefer export symbols from PHY driver. That'll create unnecessary
>> dependencies between the controller and the PHY.
>
> Agreed.
>
>>
>> I think it'll be better to create a new attribute and use it?
>
> Another simpler option is to not support _set_mode() for DA8xx, and the
> phy driver set the otgmode bit in probe() based on dr_mode of the
> controller.
>
> Regards,
> -Bin.
>

This certainly works for my particular use case, however, Sergei
Shtylyov, who wrote the da8xx musb glue layer originally, seemed to
recall in his review of the patch series that this really is necessary
for OTG mode to work properly.

By the way, there is v3 of this patch series already, although this
patch is nearly the same.

2016-04-01 16:21:15

by Bin Liu

[permalink] [raw]

Subject: Re: [PATCH v2 06/11] phy: da8xx-usb: new driver for DA8XX SoC USB PHY

Hi,

On Fri, Apr 01, 2016 at 11:02:23AM -0500, David Lechner wrote:
> On 04/01/2016 09:45 AM, Bin Liu wrote:
> >>>+EXPORT_SYMBOL_GPL(da8xx_usb20_phy_set_mode);
> >>
> >>Don't prefer export symbols from PHY driver. That'll create unnecessary
> >>dependencies between the controller and the PHY.
> >
> >Agreed.
> >
> >>
> >>I think it'll be better to create a new attribute and use it?
> >
> >Another simpler option is to not support _set_mode() for DA8xx, and the
> >phy driver set the otgmode bit in probe() based on dr_mode of the
> >controller.
> >
> >Regards,
> >-Bin.
> >
>
> This certainly works for my particular use case, however, Sergei
> Shtylyov, who wrote the da8xx musb glue layer originally, seemed to
> recall in his review of the patch series that this really is
> necessary for OTG mode to work properly.

I don't know much about the usb module on DA8xx, but it seems to me that
_set_mode() has nothing to do with OTG mode. musb_core only calls
_set_mode() in two places - 1) sysfs: musb_mode_store(), 2) init:
musb_init_controller(). None of them should be related to OTG mode.

Regards,
-Bin.

2016-04-01 19:45:54

[permalink] [raw]

Subject: Re: [PATCH v2 06/11] phy: da8xx-usb: new driver for DA8XX SoC USB PHY

On 04/01/2016 07:02 PM, David Lechner wrote:

>>>> +EXPORT_SYMBOL_GPL(da8xx_usb20_phy_set_mode);
>>>
>>> Don't prefer export symbols from PHY driver. That'll create unnecessary
>>> dependencies between the controller and the PHY.
>>
>> Agreed.
>>
>>> I think it'll be better to create a new attribute and use it?
>>
>> Another simpler option is to not support _set_mode() for DA8xx, and the
>> phy driver set the otgmode bit in probe() based on dr_mode of the
>> controller.
>>
>> Regards,
>> -Bin.
>
> This certainly works for my particular use case, however, Sergei Shtylyov, who
> wrote the da8xx musb glue layer originally, seemed to recall in his review of
> the patch series that this really is necessary for OTG mode to work properly.

No, it's still not necessary, however this method is now called from
musb_init_controller() (it wasn't in my time). ISTR however that you needed to
enforce the mode override in CFGCHIP2 for the host mode to function properly.

WBR, Sergei

2016-04-01 19:49:33

[permalink] [raw]

Subject: Re: [PATCH v2 06/11] phy: da8xx-usb: new driver for DA8XX SoC USB PHY

On 04/01/2016 07:19 PM, Bin Liu wrote:

>>>>> +EXPORT_SYMBOL_GPL(da8xx_usb20_phy_set_mode);
>>>>
>>>> Don't prefer export symbols from PHY driver. That'll create unnecessary
>>>> dependencies between the controller and the PHY.
>>>
>>> Agreed.
>>>
>>>>
>>>> I think it'll be better to create a new attribute and use it?
>>>
>>> Another simpler option is to not support _set_mode() for DA8xx, and the
>>> phy driver set the otgmode bit in probe() based on dr_mode of the
>>> controller.
>>>
>>> Regards,
>>> -Bin.
>>>
>>
>> This certainly works for my particular use case, however, Sergei
>> Shtylyov, who wrote the da8xx musb glue layer originally, seemed to
>> recall in his review of the patch series that this really is
>> necessary for OTG mode to work properly.
>
> I don't know much about the usb module on DA8xx, but it seems to me that
> _set_mode() has nothing to do with OTG mode.

No, it does.

> musb_core only calls
> _set_mode() in two places - 1) sysfs: musb_mode_store(), 2) init:
> musb_init_controller(). None of them should be related to OTG mode.

Both these places do select OTG mode.

> Regards,
> -Bin.

MBR, Sergei

2016-04-01 19:58:16

by Bin Liu

[permalink] [raw]

Subject: Re: [PATCH v2 06/11] phy: da8xx-usb: new driver for DA8XX SoC USB PHY

Hi,

On Fri, Apr 01, 2016 at 10:45:47PM +0300, Sergei Shtylyov wrote:
> On 04/01/2016 07:02 PM, David Lechner wrote:
>
> >>>>+EXPORT_SYMBOL_GPL(da8xx_usb20_phy_set_mode);
> >>>
> >>>Don't prefer export symbols from PHY driver. That'll create unnecessary
> >>>dependencies between the controller and the PHY.
> >>
> >>Agreed.
> >>
> >>>I think it'll be better to create a new attribute and use it?
> >>
> >>Another simpler option is to not support _set_mode() for DA8xx, and the
> >>phy driver set the otgmode bit in probe() based on dr_mode of the
> >>controller.
> >>
> >>Regards,
> >>-Bin.
> >
> >This certainly works for my particular use case, however, Sergei Shtylyov, who
> >wrote the da8xx musb glue layer originally, seemed to recall in his review of
> >the patch series that this really is necessary for OTG mode to work properly.
>
> No, it's still not necessary, however this method is now called
> from musb_init_controller() (it wasn't in my time). ISTR however
> that you needed to enforce the mode override in CFGCHIP2 for the
> host mode to function properly.

Yes, but I think we don't have to rely on musb_init_controller(), what
we can do is the phy probe() calls of_usb_get_dr_mode_by_phy() to get
the dr_mode of the controller then sets CFGCHIP2 bits accordingly, so
that we can drop _set_mode() in the phy driver.

Regards,
-Bin.

2016-04-13 20:51:22