da8xx USB PHY platform devices and clocks

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.

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 <[email protected]>

Signed-off-by Axel Haslam <[email protected]>

---

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:

Quoted 3 lines

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.

Quoted 15 lines · 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 <[email protected]>

Signed-off-by Axel Haslam <[email protected]>

---

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:

Quoted 7 lines

Quoted 3 lines

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

Quoted 36 lines · Sekhar Nori

Quoted 24 lines · 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 <[email protected]>

Signed-off-by Axel Haslam <[email protected]>

---

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

Quoted 5 lines · Sekhar

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.

Quoted 5 lines

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.

Quoted 3 lines

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?

arch/arm/mach-davinci/usb-da8xx.c +2 -1

index f141f51..71a6d85 100644
@@ -1,20 +1,248 @@
 /*
  * DA8xx USB
  */
-#include <linux/dma-mapping.h>
+#include <linux/clk.h>
+#include <linux/delay.h>
 #include <linux/init.h>

arch/arm/mach-davinci/usb-da8xx.c +7 -0

+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);

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 <[email protected]>

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

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

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

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

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

arch/arm/boot/dts/da850.dtsi +0 -0

index f79e1b9..6bbf20d 100644

arch/arm/boot/dts/da850.dtsi +3 -0

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

Quoted 15 lines · 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 <[email protected]>

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

arch/arm/boot/dts/da850.dtsi +0 -0

index f79e1b9..6bbf20d 100644
@@ -188,6 +188,10 @@
             };
 

arch/arm/boot/dts/da850.dtsi line 191

Quoted 2 lines

arch/arm/boot/dts/da850.dtsi +1 -0

         };
+        cfgchip: cfgchip at 1417c {

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

arch/arm/boot/dts/da850.dtsi line 192

Quoted 2 lines

arch/arm/boot/dts/da850.dtsi +1 -0

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

Quoted 16 lines · David Lechner

Quoted 15 lines · 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 <[email protected]>

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

arch/arm/boot/dts/da850.dtsi +0 -0

index f79e1b9..6bbf20d 100644
@@ -188,6 +188,10 @@
             };
 

Quoted 5 lines

arch/arm/boot/dts/da850.dtsi line 191

Quoted 2 lines

arch/arm/boot/dts/da850.dtsi +1 -0

         };
+        cfgchip: cfgchip at 1417c {

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

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

Quoted 2 lines

arch/arm/boot/dts/da850.dtsi line 192

Quoted 2 lines

arch/arm/boot/dts/da850.dtsi +1 -0

+            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.

Quoted 29 lines · Sekhar Nori

Quoted 16 lines · David Lechner

Quoted 15 lines · 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 <[email protected]>

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

arch/arm/boot/dts/da850.dtsi +0 -0

index f79e1b9..6bbf20d 100644
@@ -188,6 +188,10 @@
             };
 

Quoted 5 lines

arch/arm/boot/dts/da850.dtsi line 191

Quoted 2 lines

arch/arm/boot/dts/da850.dtsi +1 -0

         };
+        cfgchip: cfgchip at 1417c {

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

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

Quoted 2 lines

arch/arm/boot/dts/da850.dtsi line 192

Quoted 2 lines

arch/arm/boot/dts/da850.dtsi +1 -0

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

Quoted 7 lines

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

Quoted 42 lines · Kevin Hilman

Quoted 29 lines · Sekhar Nori

Quoted 16 lines · David Lechner

Quoted 15 lines · 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 <[email protected]>

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

arch/arm/boot/dts/da850.dtsi +0 -0

index f79e1b9..6bbf20d 100644
@@ -188,6 +188,10 @@
             };
 

Quoted 5 lines

arch/arm/boot/dts/da850.dtsi line 191

Quoted 2 lines

arch/arm/boot/dts/da850.dtsi +1 -0

         };
+        cfgchip: cfgchip at 1417c {

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

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

Quoted 2 lines

arch/arm/boot/dts/da850.dtsi line 192

Quoted 2 lines

arch/arm/boot/dts/da850.dtsi +1 -0

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

Quoted 7 lines

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.

Quoted 3 lines

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?

Quoted 4 lines

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

Quoted 4 lines

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.

Add a node for the new usb phy driver.

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

Quoted 1 line · David Lechner

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.

Quoted 1 line

Quoted 1 line

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

Applied to v4.10/dt

Thanks,
Sekhar