[PATCH v6 0/5] da8xx USB PHY platform devices and clocks

[PATCH v6 0/5] da8xx USB PHY platform devices and clocks

[David Lechner]

It has been almost 6 months since the v5 submission, so here is a recap:

* There were a number of phy and usb dependencies that were submitted
  separately.
* The last of the usb dependencies has finally made its way into linux-next
  today.
* This series was recently included in "[PATCH/RFT v2 00/17] Add DT support for
  ohci-da8xx". I am breaking it back out again as a standalone series.


v6 changes:

* Combine "ARM: davinci: da8xx: Enable the usb20 "per" clk on phy_clk_enable"
  from the "[PATCH/RFT v2 00/17] Add DT support for ohci-da8xx" series with
  the "ARM: davinci: da8xx: add usb phy clocks" patch in this series.
* Change the syscon and da8xx-usb-phy device ids to -1.

v5 changes: renamed "usbphy" to "usb_phy" or "usb-phy" as appropriate

v4 changes: fix strict checkpatch complaint

v3 changes:

* Fixed the davinci device tree declarations to use the preferred DT address
  convention so that the items I have added can be correct too.
* Moved that davinci clock init so that we don't have to call ioremap in the
  clock mux functions.
* Added a new "syscon" device for the CFGCHIP registers. This is used by the
  USB PHY driver and will be used in the future in common clock framework
  drivers.
* USB clocks are moved to a common file instead of having duplicated code.
* PHY driver uses syscon for CFGCHIP registers instead of using them directly.

David Lechner (5):
  ARM: davinci: da8xx: add usb phy clocks
  ARM: davinci: da8xx: Add CFGCHIP syscon platform declaration.
  ARM: davinci: da8xx: Add USB PHY platform declaration
  ARM: DTS: da850: Add cfgchip syscon node
  ARM: DTS: da850: Add usb phy node

 arch/arm/boot/dts/da850.dtsi                |   9 ++
 arch/arm/mach-davinci/board-da830-evm.c     |  52 +++---
 arch/arm/mach-davinci/board-da850-evm.c     |   4 +
 arch/arm/mach-davinci/board-mityomapl138.c  |   4 +
 arch/arm/mach-davinci/board-omapl138-hawk.c |  23 ++-
 arch/arm/mach-davinci/devices-da8xx.c       |  28 ++++
 arch/arm/mach-davinci/include/mach/da8xx.h  |   6 +
 arch/arm/mach-davinci/usb-da8xx.c           | 243 +++++++++++++++++++++++++++-
 8 files changed, 327 insertions(+), 42 deletions(-)

-- 
2.7.4

[PATCH v6 1/5] ARM: davinci: da8xx: add usb phy clocks

[David Lechner]

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 and
replaced with the new clock registration functions.

Signed-off-by: David Lechner <david@lechnology.com>
Signed-off-by: Axel Haslam <ahaslam@baylibre.com>
---

I have added "ARM: davinci: da8xx: Enable the usb20 "per" clk on phy_clk_enable"
from Axel Haslam to this patch.

In the review of Axel's patch, Sekhar said:

> We should not be using a NULL device pointer here. Can you pass the musb
> device pointer available in the same file? Also, da850_clks[] in da850.c
> needs to be fixed to add the matching device name.

However, the musb device may not be registered. The usb20_clk can be used to
supply a 48MHz clock to USB 1.1 (ohci) without using the musb device. So, I am
inclined to leave this as NULL.


 arch/arm/mach-davinci/board-da830-evm.c     |  22 ++-
 arch/arm/mach-davinci/board-omapl138-hawk.c |  16 +-
 arch/arm/mach-davinci/include/mach/da8xx.h  |   3 +
 arch/arm/mach-davinci/usb-da8xx.c           | 232 +++++++++++++++++++++++++++-
 4 files changed, 252 insertions(+), 21 deletions(-)

diff --git a/arch/arm/mach-davinci/board-da830-evm.c b/arch/arm/mach-davinci/board-da830-evm.c
index 3d8cf8c..605d444 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
@@ -143,6 +131,16 @@ static __init void da830_evm_usb_init(void)
 	__raw_writel(cfgchip2, DA8XX_SYSCFG0_VIRT(DA8XX_CFGCHIP2_REG));
 
 	/* USB_REFCLKIN is not used. */
+	ret = da8xx_register_usb20_phy_clk(false);
+	if (ret)
+		pr_warn("%s: USB 2.0 PHY CLK registration failed: %d\n",
+			__func__, ret);
+
+	ret = da8xx_register_usb11_phy_clk(false);
+	if (ret)
+		pr_warn("%s: USB 1.1 PHY CLK registration failed: %d\n",
+			__func__, ret);
+
 	ret = davinci_cfg_reg(DA830_USB0_DRVVBUS);
 	if (ret)
 		pr_warn("%s: USB 2.0 PinMux setup failed: %d\n", __func__, ret);
diff --git a/arch/arm/mach-davinci/board-omapl138-hawk.c b/arch/arm/mach-davinci/board-omapl138-hawk.c
index ee62486..d4930b6 100644
--- a/arch/arm/mach-davinci/board-omapl138-hawk.c
+++ b/arch/arm/mach-davinci/board-omapl138-hawk.c
@@ -243,7 +243,6 @@ static irqreturn_t omapl138_hawk_usb_ocic_irq(int irq, void *dev_id)
 static __init void omapl138_hawk_usb_init(void)
 {
 	int ret;
-	u32 cfgchip2;
 
 	ret = davinci_cfg_reg_list(da850_hawk_usb11_pins);
 	if (ret) {
@@ -251,12 +250,15 @@ 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));
+	/* USB_REFCLKIN is not used. */
+	ret = da8xx_register_usb20_phy_clk(false);
+	if (ret)
+		pr_warn("%s: USB 2.0 PHY CLK registration failed: %d\n",
+			__func__, ret);
+	ret = da8xx_register_usb11_phy_clk(false);
+	if (ret)
+		pr_warn("%s: USB 1.1 PHY CLK registration failed: %d\n",
+			__func__, ret);
 
 	ret = gpio_request_one(DA850_USB1_VBUS_PIN,
 			GPIOF_DIR_OUT, "USB1 VBUS");
diff --git a/arch/arm/mach-davinci/include/mach/da8xx.h b/arch/arm/mach-davinci/include/mach/da8xx.h
index f9f9713..c367530 100644
--- a/arch/arm/mach-davinci/include/mach/da8xx.h
+++ b/arch/arm/mach-davinci/include/mach/da8xx.h
@@ -88,6 +88,9 @@ 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_usb_refclkin(int rate);
+int da8xx_register_usb20_phy_clk(bool use_usb_refclkin);
+int da8xx_register_usb11_phy_clk(bool use_usb_refclkin);
 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-da8xx.c b/arch/arm/mach-davinci/usb-da8xx.c
index f141f51..71a6d85 100644
--- a/arch/arm/mach-davinci/usb-da8xx.c
+++ b/arch/arm/mach-davinci/usb-da8xx.c
@@ -1,20 +1,248 @@
 /*
  * DA8xx USB
  */
-#include <linux/dma-mapping.h>
+#include <linux/clk.h>
+#include <linux/delay.h>
 #include <linux/init.h>
 #include <linux/platform_data/usb-davinci.h>
 #include <linux/platform_device.h>
+#include <linux/mfd/da8xx-cfgchip.h>
 #include <linux/usb/musb.h>
 
+#include <mach/clock.h>
 #include <mach/common.h>
 #include <mach/cputype.h>
 #include <mach/da8xx.h>
-#include <mach/irqs.h>
+
+#include "clock.h"
 
 #define DA8XX_USB0_BASE		0x01e00000
 #define DA8XX_USB1_BASE		0x01e25000
 
+static struct clk usb_refclkin = {
+	.name		= "usb_refclkin",
+	.set_rate	= davinci_simple_set_rate,
+};
+
+static struct clk_lookup usb_refclkin_lookup =
+	CLK(NULL, "usb_refclkin", &usb_refclkin);
+
+/**
+ * da8xx_register_usb_refclkin - register USB_REFCLKIN clock
+ *
+ * @rate: The clock rate in Hz
+ *
+ * This clock is only needed if the board provides an external USB_REFCLKIN
+ * signal, in which case it will be used as the parent of usb20_phy_clk and/or
+ * usb11_phy_clk.
+ */
+int __init da8xx_register_usb_refclkin(int rate)
+{
+	int ret;
+
+	usb_refclkin.rate = rate;
+	ret = clk_register(&usb_refclkin);
+	if (ret)
+		return ret;
+
+	clkdev_add(&usb_refclkin_lookup);
+
+	return 0;
+}
+
+static void usb20_phy_clk_enable(struct clk *clk)
+{
+	struct clk *usb20_clk;
+	u32 val;
+	u32 timeout = 500000; /* 500 msec */
+
+	val = readl(DA8XX_SYSCFG0_VIRT(DA8XX_CFGCHIP2_REG));
+
+	usb20_clk = clk_get(NULL, "usb20");
+	if (IS_ERR(usb20_clk)) {
+		pr_err("could not get usb20 clk\n");
+		return;
+	}
+
+	/* The USB 2.O PLL requires that the USB 2.O PSC is enabled as well. */
+	clk_prepare_enable(usb20_clk);
+
+	/*
+	 * 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));
+
+	while (--timeout) {
+		val = readl(DA8XX_SYSCFG0_VIRT(DA8XX_CFGCHIP2_REG));
+		if (val & CFGCHIP2_PHYCLKGD)
+			goto done;
+		udelay(1);
+	}
+
+	pr_err("Timeout waiting for USB 2.0 PHY clock good.\n");
+done:
+	clk_disable_unprepare(usb20_clk);
+	clk_put(usb20_clk);
+}
+
+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 val;
+
+	val = readl(DA8XX_SYSCFG0_VIRT(DA8XX_CFGCHIP2_REG));
+
+	/* Set the mux depending on the parent clock. */
+	if (parent == &usb_refclkin) {
+		val &= ~CFGCHIP2_USB2PHYCLKMUX;
+	} else if (strcmp(parent->name, "pll0_aux_clk") == 0) {
+		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_MASK;
+	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, DA8XX_SYSCFG0_VIRT(DA8XX_CFGCHIP2_REG));
+
+	return 0;
+}
+
+static struct clk usb20_phy_clk = {
+	.name		= "usb20_phy",
+	.clk_enable	= usb20_phy_clk_enable,
+	.clk_disable	= usb20_phy_clk_disable,
+	.set_parent	= usb20_phy_clk_set_parent,
+};
+
+static struct clk_lookup usb20_phy_clk_lookup =
+	CLK(NULL, "usb20_phy", &usb20_phy_clk);
+
+/**
+ * da8xx_register_usb20_phy_clk - register USB0PHYCLKMUX clock
+ *
+ * @use_usb_refclkin: Selects the parent clock - either "usb_refclkin" if true
+ *	or "pll0_aux" if false.
+ */
+int __init da8xx_register_usb20_phy_clk(bool use_usb_refclkin)
+{
+	struct clk *parent;
+	int ret = 0;
+
+	parent = clk_get(NULL, use_usb_refclkin ? "usb_refclkin" : "pll0_aux");
+	if (IS_ERR(parent))
+		return PTR_ERR(parent);
+
+	usb20_phy_clk.parent = parent;
+	ret = clk_register(&usb20_phy_clk);
+	if (!ret)
+		clkdev_add(&usb20_phy_clk_lookup);
+
+	clk_put(parent);
+
+	return ret;
+}
+
+static int usb11_phy_clk_set_parent(struct clk *clk, struct clk *parent)
+{
+	u32 val;
+
+	val = readl(DA8XX_SYSCFG0_VIRT(DA8XX_CFGCHIP2_REG));
+
+	/* 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_refclkin) {
+		val |= CFGCHIP2_USB1PHYCLKMUX;
+	} else {
+		pr_err("Bad parent on USB 1.1 PHY clock.\n");
+		return -EINVAL;
+	}
+
+	writel(val, DA8XX_SYSCFG0_VIRT(DA8XX_CFGCHIP2_REG));
+
+	return 0;
+}
+
+static struct clk usb11_phy_clk = {
+	.name		= "usb11_phy",
+	.set_parent	= usb11_phy_clk_set_parent,
+};
+
+static struct clk_lookup usb11_phy_clk_lookup =
+	CLK(NULL, "usb11_phy", &usb11_phy_clk);
+
+/**
+ * da8xx_register_usb11_phy_clk - register USB1PHYCLKMUX clock
+ *
+ * @use_usb_refclkin: Selects the parent clock - either "usb_refclkin" if true
+ *	or "usb20_phy" if false.
+ */
+int __init da8xx_register_usb11_phy_clk(bool use_usb_refclkin)
+{
+	struct clk *parent;
+	int ret = 0;
+
+	parent = clk_get(NULL, use_usb_refclkin ? "usb_refclkin" : "usb20_phy");
+	if (IS_ERR(parent))
+		return PTR_ERR(parent);
+
+	usb11_phy_clk.parent = parent;
+	ret = clk_register(&usb11_phy_clk);
+	if (!ret)
+		clkdev_add(&usb11_phy_clk_lookup);
+
+	clk_put(parent);
+
+	return ret;
+}
+
 #if IS_ENABLED(CONFIG_USB_MUSB_HDRC)
 
 static struct musb_hdrc_config musb_config = {
-- 
2.7.4

Re: [PATCH v6 1/5] ARM: davinci: da8xx: add usb phy clocks

[Sekhar Nori]

On Wednesday 26 October 2016 08:36 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 and
> replaced with the new clock registration functions.
> 
> Signed-off-by: David Lechner <david@lechnology.com>
> Signed-off-by: Axel Haslam <ahaslam@baylibre.com>
> ---
> 
> I have added "ARM: davinci: da8xx: Enable the usb20 "per" clk on phy_clk_enable"
> from Axel Haslam to this patch.
> 
> In the review of Axel's patch, Sekhar said:
> 
>> We should not be using a NULL device pointer here. Can you pass the musb
>> device pointer available in the same file? Also, da850_clks[] in da850.c
>> needs to be fixed to add the matching device name.
> 
> However, the musb device may not be registered. The usb20_clk can be used to
> supply a 48MHz clock to USB 1.1 (ohci) without using the musb device. So, I am
> inclined to leave this as NULL.

But clock look-up has nothing to do with device being registered AFAICT.
It is used to identify the clock consumer. Passing NULL there means the
clock is not associated with any device. Which is not correct as we are
specifically looking at MUSB module clock.

Thanks,
Sekhar

Re: [PATCH v6 1/5] ARM: davinci: da8xx: add usb phy clocks

[David Lechner]

On 10/26/2016 02:59 AM, Sekhar Nori wrote:
> On Wednesday 26 October 2016 08:36 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 and
>> replaced with the new clock registration functions.
>>
>> Signed-off-by: David Lechner <david@lechnology.com>
>> Signed-off-by: Axel Haslam <ahaslam@baylibre.com>
>> ---
>>
>> I have added "ARM: davinci: da8xx: Enable the usb20 "per" clk on phy_clk_enable"
>> from Axel Haslam to this patch.
>>
>> In the review of Axel's patch, Sekhar said:
>>
>>> We should not be using a NULL device pointer here. Can you pass the musb
>>> device pointer available in the same file? Also, da850_clks[] in da850.c
>>> needs to be fixed to add the matching device name.
>>
>> However, the musb device may not be registered. The usb20_clk can be used to
>> supply a 48MHz clock to USB 1.1 (ohci) without using the musb device. So, I am
>> inclined to leave this as NULL.
>
> But clock look-up has nothing to do with device being registered AFAICT.
> It is used to identify the clock consumer. Passing NULL there means the
> clock is not associated with any device. Which is not correct as we are
> specifically looking at MUSB module clock.
>
> Thanks,
> Sekhar
>

FWIW, clk_get() uses dev_name() to get the device name, which will 
return NULL until after the platform device is registered.

I can add the device references anyway. However, this is complicated by 
the fact that the musb platform device declaration is inside of an #if 
IS_ENABLED(CONFIG_USB_MUSB_HDRC). I can either remove the #if or add 
more #if's. Do you have a preference on this?

Re: [PATCH v6 1/5] ARM: davinci: da8xx: add usb phy clocks

[Sekhar Nori]

On Wednesday 26 October 2016 10:07 PM, David Lechner wrote:
> On 10/26/2016 02:59 AM, Sekhar Nori wrote:
>> On Wednesday 26 October 2016 08:36 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 and
>>> replaced with the new clock registration functions.
>>>
>>> Signed-off-by: David Lechner <david@lechnology.com>
>>> Signed-off-by: Axel Haslam <ahaslam@baylibre.com>
>>> ---
>>>
>>> I have added "ARM: davinci: da8xx: Enable the usb20 "per" clk on
>>> phy_clk_enable"
>>> from Axel Haslam to this patch.
>>>
>>> In the review of Axel's patch, Sekhar said:
>>>
>>>> We should not be using a NULL device pointer here. Can you pass the
>>>> musb
>>>> device pointer available in the same file? Also, da850_clks[] in
>>>> da850.c
>>>> needs to be fixed to add the matching device name.
>>>
>>> However, the musb device may not be registered. The usb20_clk can be
>>> used to
>>> supply a 48MHz clock to USB 1.1 (ohci) without using the musb device.
>>> So, I am
>>> inclined to leave this as NULL.
>>
>> But clock look-up has nothing to do with device being registered AFAICT.
>> It is used to identify the clock consumer. Passing NULL there means the
>> clock is not associated with any device. Which is not correct as we are
>> specifically looking at MUSB module clock.
>>
>> Thanks,
>> Sekhar
>>
> 
> FWIW, clk_get() uses dev_name() to get the device name, which will
> return NULL until after the platform device is registered.

I believe you can set init_name in the device to setup an initial name
until registration.

> 
> I can add the device references anyway. However, this is complicated by
> the fact that the musb platform device declaration is inside of an #if
> IS_ENABLED(CONFIG_USB_MUSB_HDRC). I can either remove the #if or add
> more #if's. Do you have a preference on this?

Please remove the #if's. Usually device registration is never done
conditionally based on whether the driver is built or not. So this seems
incorrect anyway.

Thanks,
Sekhar

Re: [PATCH v6 1/5] ARM: davinci: da8xx: add usb phy clocks

[Sekhar Nori]

On Wednesday 26 October 2016 08:36 AM, David Lechner wrote:

> diff --git a/arch/arm/mach-davinci/usb-da8xx.c b/arch/arm/mach-davinci/usb-da8xx.c
> index f141f51..71a6d85 100644
> --- a/arch/arm/mach-davinci/usb-da8xx.c
> +++ b/arch/arm/mach-davinci/usb-da8xx.c
> @@ -1,20 +1,248 @@
>  /*
>   * DA8xx USB
>   */
> -#include <linux/dma-mapping.h>
> +#include <linux/clk.h>
> +#include <linux/delay.h>
>  #include <linux/init.h>
>  #include <linux/platform_data/usb-davinci.h>
>  #include <linux/platform_device.h>
> +#include <linux/mfd/da8xx-cfgchip.h>
>  #include <linux/usb/musb.h>
>  
> +#include <mach/clock.h>
>  #include <mach/common.h>
>  #include <mach/cputype.h>
>  #include <mach/da8xx.h>
> -#include <mach/irqs.h>

Looks like you have cleaned up some unneeded includes. Thats fine, just
mention it in the commit description too, so its clear that it is intended.

Also, when adding new include files, please do it in alphabetical order.

> +static struct clk usb_refclkin = {
> +	.name		= "usb_refclkin",
> +	.set_rate	= davinci_simple_set_rate,
> +};
> +
> +static struct clk_lookup usb_refclkin_lookup =
> +	CLK(NULL, "usb_refclkin", &usb_refclkin);
> +
> +static struct clk usb20_phy_clk = {
> +	.name		= "usb20_phy",
> +	.clk_enable	= usb20_phy_clk_enable,
> +	.clk_disable	= usb20_phy_clk_disable,
> +	.set_parent	= usb20_phy_clk_set_parent,
> +};
> +
> +static struct clk_lookup usb20_phy_clk_lookup =
> +	CLK(NULL, "usb20_phy", &usb20_phy_clk);

NULL device name here is wrong. There is a PHY device that gets added
3/5. The name of that device should appear here. Since that phy device
is the consumer of this clock. You can change the order of patches so
the device appears before the associated clocks.

> +static struct clk usb11_phy_clk = {
> +	.name		= "usb11_phy",
> +	.set_parent	= usb11_phy_clk_set_parent,
> +};
> +
> +static struct clk_lookup usb11_phy_clk_lookup =
> +	CLK(NULL, "usb11_phy", &usb11_phy_clk);

here too, please have the phy device name as the consumer of this clock.

It is okay to have NULL device name for the usb_refclkin since that is a
root clock not associated with any device.

Thanks,
Sekhar

[PATCH v6 2/5] ARM: davinci: da8xx: Add CFGCHIP syscon platform declaration.

[David Lechner]

The CFGCHIP registers are used by a number of devices, so using a syscon
device to share them. The first consumer of this will by the phy-da8xx-usb
driver.

Signed-off-by: David Lechner <david@lechnology.com>
---

syscon device id is changed to -1 since there is only one syscon device.

 arch/arm/mach-davinci/board-da830-evm.c     |  4 ++++
 arch/arm/mach-davinci/board-da850-evm.c     |  4 ++++
 arch/arm/mach-davinci/board-mityomapl138.c  |  4 ++++
 arch/arm/mach-davinci/board-omapl138-hawk.c |  4 ++++
 arch/arm/mach-davinci/devices-da8xx.c       | 28 ++++++++++++++++++++++++++++
 arch/arm/mach-davinci/include/mach/da8xx.h  |  2 ++
 6 files changed, 46 insertions(+)

diff --git a/arch/arm/mach-davinci/board-da830-evm.c b/arch/arm/mach-davinci/board-da830-evm.c
index 605d444..3051cb6 100644
--- a/arch/arm/mach-davinci/board-da830-evm.c
+++ b/arch/arm/mach-davinci/board-da830-evm.c
@@ -586,6 +586,10 @@ static __init void da830_evm_init(void)
 	struct davinci_soc_info *soc_info = &davinci_soc_info;
 	int ret;
 
+	ret = da8xx_register_cfgchip();
+	if (ret)
+		pr_warn("%s: CFGCHIP registration failed: %d\n", __func__, ret);
+
 	ret = da830_register_gpio();
 	if (ret)
 		pr_warn("%s: GPIO init failed: %d\n", __func__, ret);
diff --git a/arch/arm/mach-davinci/board-da850-evm.c b/arch/arm/mach-davinci/board-da850-evm.c
index 8e4539f..ec5cb10 100644
--- a/arch/arm/mach-davinci/board-da850-evm.c
+++ b/arch/arm/mach-davinci/board-da850-evm.c
@@ -1345,6 +1345,10 @@ static __init void da850_evm_init(void)
 {
 	int ret;
 
+	ret = da8xx_register_cfgchip();
+	if (ret)
+		pr_warn("%s: CFGCHIP registration failed: %d\n", __func__, ret);
+
 	ret = da850_register_gpio();
 	if (ret)
 		pr_warn("%s: GPIO init failed: %d\n", __func__, ret);
diff --git a/arch/arm/mach-davinci/board-mityomapl138.c b/arch/arm/mach-davinci/board-mityomapl138.c
index bc4e63f..1a6d430 100644
--- a/arch/arm/mach-davinci/board-mityomapl138.c
+++ b/arch/arm/mach-davinci/board-mityomapl138.c
@@ -514,6 +514,10 @@ static void __init mityomapl138_init(void)
 {
 	int ret;
 
+	ret = da8xx_register_cfgchip();
+	if (ret)
+		pr_warn("%s: CFGCHIP registration failed: %d\n", __func__, ret);
+
 	/* for now, no special EDMA channels are reserved */
 	ret = da850_register_edma(NULL);
 	if (ret)
diff --git a/arch/arm/mach-davinci/board-omapl138-hawk.c b/arch/arm/mach-davinci/board-omapl138-hawk.c
index d4930b6..8691a25 100644
--- a/arch/arm/mach-davinci/board-omapl138-hawk.c
+++ b/arch/arm/mach-davinci/board-omapl138-hawk.c
@@ -294,6 +294,10 @@ static __init void omapl138_hawk_init(void)
 {
 	int ret;
 
+	ret = da8xx_register_cfgchip();
+	if (ret)
+		pr_warn("%s: CFGCHIP registration failed: %d\n", __func__, ret);
+
 	ret = da850_register_gpio();
 	if (ret)
 		pr_warn("%s: GPIO init failed: %d\n", __func__, ret);
diff --git a/arch/arm/mach-davinci/devices-da8xx.c b/arch/arm/mach-davinci/devices-da8xx.c
index add3771..31a99db 100644
--- a/arch/arm/mach-davinci/devices-da8xx.c
+++ b/arch/arm/mach-davinci/devices-da8xx.c
@@ -11,6 +11,7 @@
  * (at your option) any later version.
  */
 #include <linux/init.h>
+#include <linux/platform_data/syscon.h>
 #include <linux/platform_device.h>
 #include <linux/dma-contiguous.h>
 #include <linux/serial_8250.h>
@@ -1089,3 +1090,30 @@ int __init da850_register_sata(unsigned long refclkpn)
 	return platform_device_register(&da850_sata_device);
 }
 #endif
+
+static struct syscon_platform_data da8xx_cfgchip_platform_data = {
+	.label	= "cfgchip",
+};
+
+static struct resource da8xx_cfgchip_resources[] = {
+	{
+		.start	= DA8XX_SYSCFG0_BASE + DA8XX_CFGCHIP0_REG,
+		.end	= DA8XX_SYSCFG0_BASE + DA8XX_CFGCHIP4_REG + 3,
+		.flags	= IORESOURCE_MEM,
+	},
+};
+
+static struct platform_device da8xx_cfgchip_device = {
+	.name	= "syscon",
+	.id	= -1,
+	.dev	= {
+		.platform_data	= &da8xx_cfgchip_platform_data,
+	},
+	.num_resources	= ARRAY_SIZE(da8xx_cfgchip_resources),
+	.resource	= da8xx_cfgchip_resources,
+};
+
+int __init da8xx_register_cfgchip(void)
+{
+	return platform_device_register(&da8xx_cfgchip_device);
+}
diff --git a/arch/arm/mach-davinci/include/mach/da8xx.h b/arch/arm/mach-davinci/include/mach/da8xx.h
index c367530..c32444b 100644
--- a/arch/arm/mach-davinci/include/mach/da8xx.h
+++ b/arch/arm/mach-davinci/include/mach/da8xx.h
@@ -61,6 +61,7 @@ extern unsigned int da850_max_speed;
 #define DA8XX_CFGCHIP1_REG	0x180
 #define DA8XX_CFGCHIP2_REG	0x184
 #define DA8XX_CFGCHIP3_REG	0x188
+#define DA8XX_CFGCHIP4_REG	0x18c
 
 #define DA8XX_SYSCFG1_BASE	(IO_PHYS + 0x22C000)
 #define DA8XX_SYSCFG1_VIRT(x)	(da8xx_syscfg1_base + (x))
@@ -116,6 +117,7 @@ void da8xx_rproc_reserve_cma(void);
 int da8xx_register_rproc(void);
 int da850_register_gpio(void);
 int da830_register_gpio(void);
+int da8xx_register_cfgchip(void);
 
 extern struct platform_device da8xx_serial_device[];
 extern struct emac_platform_data da8xx_emac_pdata;
-- 
2.7.4

Re: [PATCH v6 2/5] ARM: davinci: da8xx: Add CFGCHIP syscon platform declaration.

[Sekhar Nori]

On Wednesday 26 October 2016 08:36 AM, David Lechner wrote:
> The CFGCHIP registers are used by a number of devices, so using a syscon
> device to share them. The first consumer of this will by the phy-da8xx-usb

"will be the .." 

Also, in subject line, "platform device" instead of "platform 
declaration". Also lose the period at at the end of subject line

> driver.
> 
> Signed-off-by: David Lechner <david@lechnology.com>

Applied to v4.10/soc. Here is the final commit text.

---
ARM: davinci: da8xx: Add CFGCHIP syscon platform device

The CFGCHIP registers are used by a number of devices, so use a syscon
device to share them. The first consumer of this will by the phy-da8xx-usb
driver.

Add the syscon device and register it.

Signed-off-by: David Lechner <david@lechnology.com>
[nsekhar@ti.com: minor commit message fixes]
Signed-off-by: Sekhar Nori <nsekhar@ti.com>
---

Thanks,
Sekhar

[PATCH v6 3/5] ARM: davinci: da8xx: Add USB PHY platform declaration

[David Lechner]

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 <david@lechnology.com>
---

da8xx-usb-phy device id is changed to -1 since there is only one da8xx-usb-phy
device.

 arch/arm/mach-davinci/board-da830-evm.c     | 28 +++++-----------------------
 arch/arm/mach-davinci/board-omapl138-hawk.c |  5 +++++
 arch/arm/mach-davinci/include/mach/da8xx.h  |  1 +
 arch/arm/mach-davinci/usb-da8xx.c           | 11 +++++++++++
 4 files changed, 22 insertions(+), 23 deletions(-)

diff --git a/arch/arm/mach-davinci/board-da830-evm.c b/arch/arm/mach-davinci/board-da830-evm.c
index 3051cb6..c62766e 100644
--- a/arch/arm/mach-davinci/board-da830-evm.c
+++ b/arch/arm/mach-davinci/board-da830-evm.c
@@ -26,7 +26,6 @@
 #include <linux/platform_data/mtd-davinci.h>
 #include <linux/platform_data/mtd-davinci-aemif.h>
 #include <linux/platform_data/spi-davinci.h>
-#include <linux/platform_data/usb-davinci.h>
 
 #include <asm/mach-types.h>
 #include <asm/mach/arch.h>
@@ -106,30 +105,8 @@ static irqreturn_t da830_evm_usb_ocic_irq(int irq, void *dev_id)
 
 static __init void da830_evm_usb_init(void)
 {
-	u32 cfgchip2;
 	int ret;
 
-	/*
-	 * Set up USB clock/mode in the CFGCHIP2 register.
-	 * FYI:  CFGCHIP2 is 0x0000ef00 initially.
-	 */
-	cfgchip2 = __raw_readl(DA8XX_SYSCFG0_VIRT(DA8XX_CFGCHIP2_REG));
-
-	/*
-	 * 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
-	 * controller won't be able to drive VBUS thinking that it's a B-device.
-	 * Otherwise, we want to use the OTG mode and enable VBUS comparators.
-	 */
-	cfgchip2 &= ~CFGCHIP2_OTGMODE;
-#ifdef	CONFIG_USB_MUSB_HOST
-	cfgchip2 |=  CFGCHIP2_FORCE_HOST;
-#else
-	cfgchip2 |=  CFGCHIP2_SESENDEN | CFGCHIP2_VBDTCTEN;
-#endif
-
-	__raw_writel(cfgchip2, DA8XX_SYSCFG0_VIRT(DA8XX_CFGCHIP2_REG));
-
 	/* USB_REFCLKIN is not used. */
 	ret = da8xx_register_usb20_phy_clk(false);
 	if (ret)
@@ -141,6 +118,11 @@ static __init void da830_evm_usb_init(void)
 		pr_warn("%s: USB 1.1 PHY CLK registration failed: %d\n",
 			__func__, ret);
 
+	ret = da8xx_register_usb_phy();
+	if (ret)
+		pr_warn("%s: USB PHY registration failed: %d\n",
+			__func__, ret);
+
 	ret = davinci_cfg_reg(DA830_USB0_DRVVBUS);
 	if (ret)
 		pr_warn("%s: USB 2.0 PinMux setup failed: %d\n", __func__, ret);
diff --git a/arch/arm/mach-davinci/board-omapl138-hawk.c b/arch/arm/mach-davinci/board-omapl138-hawk.c
index 8691a25..c5cb8d9 100644
--- a/arch/arm/mach-davinci/board-omapl138-hawk.c
+++ b/arch/arm/mach-davinci/board-omapl138-hawk.c
@@ -260,6 +260,11 @@ static __init void omapl138_hawk_usb_init(void)
 		pr_warn("%s: USB 1.1 PHY CLK registration failed: %d\n",
 			__func__, ret);
 
+	ret = da8xx_register_usb_phy();
+	if (ret)
+		pr_warn("%s: USB PHY registration failed: %d\n",
+			__func__, ret);
+
 	ret = gpio_request_one(DA850_USB1_VBUS_PIN,
 			GPIOF_DIR_OUT, "USB1 VBUS");
 	if (ret < 0) {
diff --git a/arch/arm/mach-davinci/include/mach/da8xx.h b/arch/arm/mach-davinci/include/mach/da8xx.h
index c32444b..38d932e 100644
--- a/arch/arm/mach-davinci/include/mach/da8xx.h
+++ b/arch/arm/mach-davinci/include/mach/da8xx.h
@@ -92,6 +92,7 @@ int da8xx_register_watchdog(void);
 int da8xx_register_usb_refclkin(int rate);
 int da8xx_register_usb20_phy_clk(bool use_usb_refclkin);
 int da8xx_register_usb11_phy_clk(bool use_usb_refclkin);
+int da8xx_register_usb_phy(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-da8xx.c b/arch/arm/mach-davinci/usb-da8xx.c
index 71a6d85..9c30bff 100644
--- a/arch/arm/mach-davinci/usb-da8xx.c
+++ b/arch/arm/mach-davinci/usb-da8xx.c
@@ -7,6 +7,7 @@
 #include <linux/platform_data/usb-davinci.h>
 #include <linux/platform_device.h>
 #include <linux/mfd/da8xx-cfgchip.h>
+#include <linux/phy/phy.h>
 #include <linux/usb/musb.h>
 
 #include <mach/clock.h>
@@ -243,6 +244,16 @@ int __init da8xx_register_usb11_phy_clk(bool use_usb_refclkin)
 	return ret;
 }
 
+static struct platform_device da8xx_usb_phy = {
+	.name		= "da8xx-usb-phy",
+	.id		= -1,
+};
+
+int __init da8xx_register_usb_phy(void)
+{
+	return platform_device_register(&da8xx_usb_phy);
+}
+
 #if IS_ENABLED(CONFIG_USB_MUSB_HDRC)
 
 static struct musb_hdrc_config musb_config = {
-- 
2.7.4

[PATCH v6 4/5] ARM: DTS: da850: Add cfgchip syscon node

[David Lechner]

Add a syscon node for the SoC CFGCHIPn registers. This is needed for
the new usb phy driver.

Signed-off-by: David Lechner <david@lechnology.com>
---
 arch/arm/boot/dts/da850.dtsi | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/arch/arm/boot/dts/da850.dtsi b/arch/arm/boot/dts/da850.dtsi
index f79e1b9..6bbf20d 100644
--- a/arch/arm/boot/dts/da850.dtsi
+++ b/arch/arm/boot/dts/da850.dtsi
@@ -188,6 +188,10 @@
 			};
 
 		};
+		cfgchip: cfgchip@1417c {
+			compatible = "ti,da830-cfgchip", "syscon";
+			reg = <0x1417c 0x14>;
+		};
 		edma0: edma@0 {
 			compatible = "ti,edma3-tpcc";
 			/* eDMA3 CC0: 0x01c0 0000 - 0x01c0 7fff */
-- 
2.7.4

Re: [PATCH v6 4/5] ARM: DTS: da850: Add cfgchip syscon node

[Sekhar Nori]

On Wednesday 26 October 2016 08:36 AM, David Lechner wrote:
> Add a syscon node for the SoC CFGCHIPn registers. This is needed for
> the new usb phy driver.
> 
> Signed-off-by: David Lechner <david@lechnology.com>

Applied to v4.10/dt. Changed "DTS" in subject line to small case. ARM
device-tree patches have been following that.

Thanks
Sekhar

Re: [PATCH v6 4/5] ARM: DTS: da850: Add cfgchip syscon node

[David Lechner]

On 10/25/2016 10:06 PM, David Lechner wrote:
> Add a syscon node for the SoC CFGCHIPn registers. This is needed for
> the new usb phy driver.
>
> Signed-off-by: David Lechner <david@lechnology.com>
> ---
>  arch/arm/boot/dts/da850.dtsi | 4 ++++
>  1 file changed, 4 insertions(+)
>
> diff --git a/arch/arm/boot/dts/da850.dtsi b/arch/arm/boot/dts/da850.dtsi
> index f79e1b9..6bbf20d 100644
> --- a/arch/arm/boot/dts/da850.dtsi
> +++ b/arch/arm/boot/dts/da850.dtsi
> @@ -188,6 +188,10 @@
>  			};
>
>  		};
> +		cfgchip: cfgchip@1417c {

I wonder if there is a more generic name instead of cfgchip@. Is there a 
preferred generic name for syscon nodes?

> +			compatible = "ti,da830-cfgchip", "syscon";
> +			reg = <0x1417c 0x14>;
> +		};
>  		edma0: edma@0 {
>  			compatible = "ti,edma3-tpcc";
>  			/* eDMA3 CC0: 0x01c0 0000 - 0x01c0 7fff */
>

Re: [PATCH v6 4/5] ARM: DTS: da850: Add cfgchip syscon node

[Sekhar Nori]

On Wednesday 26 October 2016 09:38 PM, David Lechner wrote:
> On 10/25/2016 10:06 PM, David Lechner wrote:
>> Add a syscon node for the SoC CFGCHIPn registers. This is needed for
>> the new usb phy driver.
>>
>> Signed-off-by: David Lechner <david@lechnology.com>
>> ---
>>  arch/arm/boot/dts/da850.dtsi | 4 ++++
>>  1 file changed, 4 insertions(+)
>>
>> diff --git a/arch/arm/boot/dts/da850.dtsi b/arch/arm/boot/dts/da850.dtsi
>> index f79e1b9..6bbf20d 100644
>> --- a/arch/arm/boot/dts/da850.dtsi
>> +++ b/arch/arm/boot/dts/da850.dtsi
>> @@ -188,6 +188,10 @@
>>              };
>>
>>          };
>> +        cfgchip: cfgchip@1417c {
> 
> I wonder if there is a more generic name instead of cfgchip@. Is there a
> preferred generic name for syscon nodes?

I did not find anything in ePAPR, but chip-controller might be more
appropriate.

> 
>> +            compatible = "ti,da830-cfgchip", "syscon";

Looks like we need "simple-mfd" too in the compatible list?

I think we can also fold patch 5/5 into this patch and add the cfgchip
along with USB phy child node included.

If you respin the patch, I can drop 4/5 and 5/5 that I have queued and
included the updated patch instead.

Thanks,
Sekhar

Re: [PATCH v6 4/5] ARM: DTS: da850: Add cfgchip syscon node

[Kevin Hilman]

Sekhar Nori <nsekhar@ti.com> writes:

> On Wednesday 26 October 2016 09:38 PM, David Lechner wrote:
>> On 10/25/2016 10:06 PM, David Lechner wrote:
>>> Add a syscon node for the SoC CFGCHIPn registers. This is needed for
>>> the new usb phy driver.
>>>
>>> Signed-off-by: David Lechner <david@lechnology.com>
>>> ---
>>>  arch/arm/boot/dts/da850.dtsi | 4 ++++
>>>  1 file changed, 4 insertions(+)
>>>
>>> diff --git a/arch/arm/boot/dts/da850.dtsi b/arch/arm/boot/dts/da850.dtsi
>>> index f79e1b9..6bbf20d 100644
>>> --- a/arch/arm/boot/dts/da850.dtsi
>>> +++ b/arch/arm/boot/dts/da850.dtsi
>>> @@ -188,6 +188,10 @@
>>>              };
>>>
>>>          };
>>> +        cfgchip: cfgchip@1417c {
>> 
>> I wonder if there is a more generic name instead of cfgchip@. Is there a
>> preferred generic name for syscon nodes?
>
> I did not find anything in ePAPR, but chip-controller might be more
> appropriate.
>
>> 
>>> +            compatible = "ti,da830-cfgchip", "syscon";
>
> Looks like we need "simple-mfd" too in the compatible list?
>
> I think we can also fold patch 5/5 into this patch and add the cfgchip
> along with USB phy child node included.
>
> If you respin the patch, I can drop 4/5 and 5/5 that I have queued and
> included the updated patch instead.

Sekhar, what's your opinion of having this syscon just for CFGCHIP* vs 
a single syscon for the whole SYSCFG0 region.

The drivers/bus driver from Bartosz is also using SYSCFG0 registers, and
proposing a sysconf ro this region, but it will need to exclude the
CFGCHIPn registers if we also have this syscon.

I tend to think we should just have one for the whole SYSCFG0 which
this series could use.

Unfortunately, the PHY driver is already merged and it references the
syscon by compatible.  The PHY driver should probably be fixed to find
its syscon by phandle, and then maybe we could move to a single syscon
for SYSCFG0?

Let us know your preference, I don't have a very strong feeling either
way, but since we're already part way down the path of the CFGCHIP
syscon, we should keep it and later migrate it to one for all of
SYSCFG0.

Kevin

Re: [PATCH v6 4/5] ARM: DTS: da850: Add cfgchip syscon node

[David Lechner]

On 10/28/2016 12:08 PM, Kevin Hilman wrote:
> Sekhar Nori <nsekhar@ti.com> writes:
>
>> On Wednesday 26 October 2016 09:38 PM, David Lechner wrote:
>>> On 10/25/2016 10:06 PM, David Lechner wrote:
>>>> Add a syscon node for the SoC CFGCHIPn registers. This is needed for
>>>> the new usb phy driver.
>>>>
>>>> Signed-off-by: David Lechner <david@lechnology.com>
>>>> ---
>>>>  arch/arm/boot/dts/da850.dtsi | 4 ++++
>>>>  1 file changed, 4 insertions(+)
>>>>
>>>> diff --git a/arch/arm/boot/dts/da850.dtsi b/arch/arm/boot/dts/da850.dtsi
>>>> index f79e1b9..6bbf20d 100644
>>>> --- a/arch/arm/boot/dts/da850.dtsi
>>>> +++ b/arch/arm/boot/dts/da850.dtsi
>>>> @@ -188,6 +188,10 @@
>>>>              };
>>>>
>>>>          };
>>>> +        cfgchip: cfgchip@1417c {
>>>
>>> I wonder if there is a more generic name instead of cfgchip@. Is there a
>>> preferred generic name for syscon nodes?
>>
>> I did not find anything in ePAPR, but chip-controller might be more
>> appropriate.
>>
>>>
>>>> +            compatible = "ti,da830-cfgchip", "syscon";
>>
>> Looks like we need "simple-mfd" too in the compatible list?
>>
>> I think we can also fold patch 5/5 into this patch and add the cfgchip
>> along with USB phy child node included.
>>
>> If you respin the patch, I can drop 4/5 and 5/5 that I have queued and
>> included the updated patch instead.
>
> Sekhar, what's your opinion of having this syscon just for CFGCHIP* vs
> a single syscon for the whole SYSCFG0 region.
>
> The drivers/bus driver from Bartosz is also using SYSCFG0 registers, and
> proposing a sysconf ro this region, but it will need to exclude the
> CFGCHIPn registers if we also have this syscon.

What about the pinmux registers, which are already being used separately 
too?

>
> I tend to think we should just have one for the whole SYSCFG0 which
> this series could use.
>
> Unfortunately, the PHY driver is already merged and it references the
> syscon by compatible.  The PHY driver should probably be fixed to find
> its syscon by phandle, and then maybe we could move to a single syscon
> for SYSCFG0?

I agree that this should be change, but I was thinking we should use 
syscon_node_to_regmap(np->parent) since the phy node should be a child 
of the syscon node.

>
> Let us know your preference, I don't have a very strong feeling either
> way, but since we're already part way down the path of the CFGCHIP
> syscon, we should keep it and later migrate it to one for all of
> SYSCFG0.
>
> Kevin
>

Re: [PATCH v6 4/5] ARM: DTS: da850: Add cfgchip syscon node

[Kevin Hilman]

David Lechner <david@lechnology.com> writes:

> On 10/28/2016 12:08 PM, Kevin Hilman wrote:
>> Sekhar Nori <nsekhar@ti.com> writes:
>>
>>> On Wednesday 26 October 2016 09:38 PM, David Lechner wrote:
>>>> On 10/25/2016 10:06 PM, David Lechner wrote:
>>>>> Add a syscon node for the SoC CFGCHIPn registers. This is needed for
>>>>> the new usb phy driver.
>>>>>
>>>>> Signed-off-by: David Lechner <david@lechnology.com>
>>>>> ---
>>>>>  arch/arm/boot/dts/da850.dtsi | 4 ++++
>>>>>  1 file changed, 4 insertions(+)
>>>>>
>>>>> diff --git a/arch/arm/boot/dts/da850.dtsi b/arch/arm/boot/dts/da850.dtsi
>>>>> index f79e1b9..6bbf20d 100644
>>>>> --- a/arch/arm/boot/dts/da850.dtsi
>>>>> +++ b/arch/arm/boot/dts/da850.dtsi
>>>>> @@ -188,6 +188,10 @@
>>>>>              };
>>>>>
>>>>>          };
>>>>> +        cfgchip: cfgchip@1417c {
>>>>
>>>> I wonder if there is a more generic name instead of cfgchip@. Is there a
>>>> preferred generic name for syscon nodes?
>>>
>>> I did not find anything in ePAPR, but chip-controller might be more
>>> appropriate.
>>>
>>>>
>>>>> +            compatible = "ti,da830-cfgchip", "syscon";
>>>
>>> Looks like we need "simple-mfd" too in the compatible list?
>>>
>>> I think we can also fold patch 5/5 into this patch and add the cfgchip
>>> along with USB phy child node included.
>>>
>>> If you respin the patch, I can drop 4/5 and 5/5 that I have queued and
>>> included the updated patch instead.
>>
>> Sekhar, what's your opinion of having this syscon just for CFGCHIP* vs
>> a single syscon for the whole SYSCFG0 region.
>>
>> The drivers/bus driver from Bartosz is also using SYSCFG0 registers, and
>> proposing a sysconf ro this region, but it will need to exclude the
>> CFGCHIPn registers if we also have this syscon.
>
> What about the pinmux registers, which are already being used
> separately too?

I guess I/we need to have a closer look at how these are actually shared
and decide if there are really overlapping users.  here are several
different registers packed into this region, but it may be the case that
there really isn't any overlapping usage (e.g. pinmux vs. PHY vs bus
priorities, etc.)

>>
>> I tend to think we should just have one for the whole SYSCFG0 which
>> this series could use.
>>
>> Unfortunately, the PHY driver is already merged and it references the
>> syscon by compatible.  The PHY driver should probably be fixed to find
>> its syscon by phandle, and then maybe we could move to a single syscon
>> for SYSCFG0?
>
> I agree that this should be change, but I was thinking we should use
> syscon_node_to_regmap(np->parent) since the phy node should be a child
> of the syscon node.

Even better.

Kevin

[PATCH v6 5/5] ARM: DTS: da850: Add usb phy node

[David Lechner]

Add a node for the new usb phy driver.

Signed-off-by: David Lechner <david@lechnology.com>
---
 arch/arm/boot/dts/da850.dtsi | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/arch/arm/boot/dts/da850.dtsi b/arch/arm/boot/dts/da850.dtsi
index 6bbf20d..33fcdce 100644
--- a/arch/arm/boot/dts/da850.dtsi
+++ b/arch/arm/boot/dts/da850.dtsi
@@ -376,6 +376,11 @@
 					>;
 			status = "disabled";
 		};
+		usb_phy: usb-phy {
+			compatible = "ti,da830-usb-phy";
+			#phy-cells = <1>;
+			status = "disabled";
+		};
 		gpio: gpio@226000 {
 			compatible = "ti,dm6441-gpio";
 			gpio-controller;
-- 
2.7.4

Re: [PATCH v6 5/5] ARM: DTS: da850: Add usb phy node

[Sekhar Nori]

On Wednesday 26 October 2016 08:36 AM, David Lechner wrote:
> Add a node for the new usb phy driver.

changed this to:

    Add a node for usb phy device. This device
    controls both the USB 1.1 and USB 2.0 PHYs.

mainly because the node is for the device, not the driver.

> 
> Signed-off-by: David Lechner <david@lechnology.com>

Applied to v4.10/dt

Thanks,
Sekhar

Re: [PATCH v6 5/5] ARM: DTS: da850: Add usb phy node

[David Lechner]

On 10/26/2016 05:26 AM, Sekhar Nori wrote:
> On Wednesday 26 October 2016 08:36 AM, David Lechner wrote:
>> Add a node for the new usb phy driver.
>
> changed this to:
>
>     Add a node for usb phy device. This device
>     controls both the USB 1.1 and USB 2.0 PHYs.
>
> mainly because the node is for the device, not the driver.
>
>>
>> Signed-off-by: David Lechner <david@lechnology.com>
>
> Applied to v4.10/dt
>
> Thanks,
> Sekhar
>

I found a better way to represent this device as a child of the syscon 
node. How should we handle the change? Should I submit a new patch that 
applies on top of this one or will you drop this patch and I should send 
a new one to take it's place?

Assuming that you agree that this is better:

	cfgchip: cfgchip@1417c {
		compatible = "ti,da830-cfgchip", "syscon", "simple-mfd";
		reg = <0x1417c 0x14>;

		usb_phy: usb-phy {
			compatible = "ti,da830-usb-phy";
			#phy-cells = <1>;
			status = "disabled";
		};
	};

Since the phy consists entirely as registers in the syscon device, we 
should make it a child of the syscon device instead of a child of the 
soc node.