dts: arm: STM32 boards use DT to configure I2C #663

ydamigos · 2017-07-01T12:08:15Z

This patches allow the configuration of I2C using DT for the following STM32 boards:

disco_l475_iot1
nucleo_f401re
96b_carbon
olimexino_stm32

galak

Looks pretty good, some minor things to tweak.

galak · 2017-07-01T15:55:07Z

drivers/i2c/i2c_ll_stm32.c

@@ -140,11 +140,11 @@ DEVICE_AND_API_INIT(i2c_stm32_1, CONFIG_I2C_1_NAME, &i2c_stm32_init,
 #ifdef CONFIG_I2C_STM32_INTERRUPT
 static void i2c_stm32_irq_config_func_1(struct device *dev)
 {
-	IRQ_CONNECT(I2C1_EV_IRQn, CONFIG_I2C_1_IRQ_PRI,
+	IRQ_CONNECT(I2C1_EV_IRQn, CONFIG_I2C_1_IRQ_EVENT_PRI,


we should get the IRQ# from the DT as well

galak · 2017-07-01T15:55:51Z

drivers/i2c/Kconfig

@@ -215,6 +215,8 @@ config I2C_1
 	bool "Enable I2C Port 1"
 	default n

+if !(I2C_STM32_V1 || I2C_STM32_V2)


Lets introduce a HAS_DTS_I2C and use that instead.

galak · 2017-07-01T16:04:22Z

dts/arm/96b_carbon.dts

@@ -27,3 +27,8 @@
 	current-speed = <115200>;
 	status = "ok";
 };
+
+&i2c1 {
+	default-configuration = <0x14>;


lets not deal with default-configuration right now. We should re-work how that's done in Zephyr anyways.

galak

Need to put I2C_n_DEFAULT_CFG back into the board Kconfig.defconfig files. Also, I forgot to mention but we need to tweak the compatible. I assume the whole _v1 & _v2 drivers are because the I2C HW differs (and so the compatible should kinda match that).

galak · 2017-07-01T19:18:55Z

boards/arm/96b_carbon/Kconfig.defconfig

@@ -10,14 +10,4 @@ if BOARD_96B_CARBON
 config BOARD
 	default 96b_carbon

-if I2C_1
-
-config I2C_1_DEFAULT_CFG


you need to leave all I2C_n_DEFAULT_CFG in here for now.

The default value for all I2C_n_DEFAULT_CFG is 0x00. Why should we change it in the board defconfig? Users should configure it in their applications. Please let me know if this change is necessary?

I left all I2C_n_DEFAULT_CFG in Kconfig.defconfig files for now.

ydamigos · 2017-07-02T10:53:31Z

I updated the compatibles to match _v1 & _v2 I2C HW.

mnkp · 2017-07-02T18:30:41Z

dts/common/yaml/i2c.yaml

+    This binding gives the base structures for all I2C devices
+
+properties:
+  - clock-frequency :


dts/common/yaml/uart.yaml is using "clock-frequency" to mean clock frequency driving the module, not the bus speed. We would better have "bus-speed" or "bus-clock-frequency". Bus speed or speed modes is also what I2C standard talks about. As it is the name "clock-frequency" is confusing.

I used the clock-frequency because I followed the linux kernel DT bindings for I2C. Moreover, I have read in a previous comment (on github or mailing list, I cannot remember) that ST plans to use the same dts files for linux and zephyr.

Indeed, aim is to converge to Linux Kernel DT.
For node properties, it's better to stick with Linux naming, as long as this is not in complete contradiction with driver behavior in Zephyr.

Makes sense. Thanks for clarification.

mnkp · 2017-07-02T19:09:14Z

drivers/i2c/Kconfig

@@ -218,7 +220,7 @@ config I2C_1
 config I2C_1_NAME
 	string "Port 1 device name"
 	default "I2C_1"
-	depends on I2C_1
+	depends on I2C_1 && !HAS_DTS_I2C

 config I2C_1_DEFAULT_CFG


I was under impression that the main point of using DTS is to get rid of things like this one from the Kconfig. Shouldn't default configuration be handled by DTS? I.e. shouldn't we add?
depends on !HAS_DTS_I2C

Initially the PR handled also the default configuration in the DTS. I was asked in a previous comment to leave it out.

Let's it it out for now..We'd need to add the parameters we want to configure rather than a generic and vague default configuration

I want to suggest to sort out such issues as default configuration now, not later. Introducing new DTS based I2C config is a good chance to get rid of this config option. It is quite ugly. It's also not clear what DTS based I2C drivers should do with it.

As @erwango suggested one way to go would be to add in DTS specific parameters we want to configure. The other approach, mentioned by @ydamigos, would be not to call i2c_configure() in I2C driver initialization function but relay on a user calling i2c_configure() in the application. In this case we would not need to define any additional parameters in DTS. I'm personally in favor of the second approach.

Once this PR is merged I can update Atmel I2C drivers to use DTS.

The latter approach sounds better to me. Keeps the DTS minimal, which it should be.

I'm not sure if we have already decided to go definitely for the latter approach but if we did we need to add
depends on !HAS_DTS_I2C
here to exclude this config option for DTS based I2C drivers. Also, we need to ensure that the driver initialization function does not call i2c_configure().

If we remove I2C_1_DEFAULT_CFG from Kconfig.defconfig but not from here and the driver initialization we will attempt to configure the driver with invalid parameters at boot, this should result in an error.

I left all I2C_n_DEFAULT_CFG in Kconfig.defconfig files for now. As we plan not to handle the default configuration in the DTS file, I believe we should address this issue in another PR.

That's also a good option. I'll open a Jira issue to give this matter more visibility.

I wanted to get the base I2C stuff in for DT, I have idea's on how we can we than use DT to replace I2C_n_DEFAULT_CFG for HAS_DTS_I2C enabled platforms

mnkp · 2017-07-02T19:13:52Z

dts/arm/st/stm32f103Xb.dtsi

+			compatible = "st,stm32-i2c-v1";
+			reg = <0x40005400 0x400>;
+			interrupts = <31 0>, <32 0>;
+			starus = "disable";


status = "disabled"

I will update it.

mnkp · 2017-07-02T19:26:09Z

dts/common/yaml/i2c.yaml

+  - clock-frequency :
+      type: int
+      category: optional
+      description: Desired I2C bus clock frequency in Hz


According to the I2C standard: "The bus speed is always constrained to the maximum communication rate of the slowest device attached to the bus". So this is the one important piece of information which needs to be provided by DTS. Otherwise I2C driver has no way of knowing what is the maximum speed it is allowed to run the bus at.

Maybe we should change the description to "Maximum I2C bus clock frequency in Hz"? Or do we need a second property?

Again here, I used part of the description in linux kernel DT bindings for I2C (https://github.com/torvalds/linux/blob/master/Documentation/devicetree/bindings/i2c/i2c-stm32.txt)

In properties description, I'm not opposed to clarifying driver behavior as it could diverge in some cases vs Linux's driver behavior and does not have impact on dts files (and won't cause any divergence on dts files themselves)

Ok, I will update the description to "Maximum I2C bus clock frequency in Hz".

erwango · 2017-07-03T07:03:56Z

dts/arm/st/stm32f103Xb.dtsi

+			compatible = "st,stm32-i2c-v1";
+			reg = <0x40005400 0x400>;
+			interrupts = <31 0>, <32 0>;
+			starus = "disable";


Add: interrupt-names = "event", "error";

I will update it.

erwango · 2017-07-03T07:04:27Z

dts/arm/st/stm32f103Xb.dtsi

+			compatible = "st,stm32-i2c-v1";
+			reg = <0x40005800 0x400>;
+			interrupts = <33 0>, <34 0>;
+			starus = "disable";


Add: interrupt-names = "event", "error";v

I will update it.

erwango · 2017-07-03T07:04:32Z

dts/arm/st/stm32f4.dtsi

+			compatible = "st,stm32-i2c-v1";
+			reg = <0x40005400 0x400>;
+			interrupts = <31 0>, <32 0>;
+			starus = "disable";


Add: interrupt-names = "event", "error";

I will update it.

erwango · 2017-07-03T07:04:36Z

dts/arm/st/stm32f4.dtsi

+		i2c2: i2c@40005800 {
+			compatible = "st,stm32-i2c-v1";
+			reg = <0x40005800 0x400>;
+			interrupts = <33 0>, <34 0>;


Add: interrupt-names = "event", "error";

I will update it.

erwango · 2017-07-03T07:04:40Z

dts/arm/st/stm32f4.dtsi

+		i2c3: i2c@40005C00 {
+			compatible = "st,stm32-i2c-v1";
+			reg = <0x40005C00 0x400>;
+			interrupts = <72 0>, <73 0>;


Add: interrupt-names = "event", "error";

I will update it.

erwango · 2017-07-03T07:04:47Z

dts/arm/st/stm32l475.dtsi

+		i2c1: i2c@40005400 {
+			compatible = "st,stm32-i2c-v2";
+			reg = <0x40005400 0x400>;
+			interrupts = <31 0>, <32 0>;


Add: interrupt-names = "event", "error";

I will update it.

erwango · 2017-07-03T07:04:53Z

dts/arm/st/stm32l475.dtsi

+		i2c2: i2c@40005800 {
+			compatible = "st,stm32-i2c-v2";
+			reg = <0x40005800 0x400>;
+			interrupts = <33 0>, <34 0>;


Add: interrupt-names = "event", "error";

I will update it.

erwango · 2017-07-03T07:04:57Z

dts/arm/st/stm32l475.dtsi

+		i2c3: i2c@40005C00 {
+			compatible = "st,stm32-i2c-v2";
+			reg = <0x40005C00 0x400>;
+			interrupts = <72 0>, <73 0>;


Add: interrupt-names = "event", "error";

I will update it.

ydamigos · 2017-07-03T17:26:13Z

Updated according to the review requests

ydamigos · 2017-07-03T19:11:45Z

I do not have prior experience with dts files but I think something is wrong with the generated dts board header when I added the interrupt-names:

/* i2c@40005400 */
#define ST_STM32_I2C_V1_40005400_BASE_ADDRESS_0		0x40005400
#define ST_STM32_I2C_V1_40005400_IRQ_0			31
#define ST_STM32_I2C_V1_40005400_IRQ_0_PRIORITY		0
#define ST_STM32_I2C_V1_40005400_IRQ_1			32
#define ST_STM32_I2C_V1_40005400_IRQ_1_PRIORITY		0
#define ST_STM32_I2C_V1_40005400_LABEL			"I2C_1"
#define ST_STM32_I2C_V1_40005400_SIZE_0			1024
#define ST_STM32_I2C_V1_40005400_BASE_ADDRESS		ST_STM32_I2C_V1_40005400_BASE_ADDRESS_0
#define ST_STM32_I2C_V1_40005400_ERROR_IRQ		ST_STM32_I2C_V1_40005400_IRQ_1_PRIORITY
#define ST_STM32_I2C_V1_40005400_EVENT_IRQ		ST_STM32_I2C_V1_40005400_IRQ_0_PRIORITY
#define ST_STM32_I2C_V1_40005400_SIZE			ST_STM32_I2C_V1_40005400_SIZE_0

The define ST_STM32_I2C_V1_40005400_EVENT_IRQ points to the priority instead of the interrupt and I believe that a define ST_STM32_I2C_V1_40005400_EVENT_IRQ_PRIORITY is missing. The same happens for the ERROR_IRQ.

@agross-linaro Is there a problem with the generated dts board header or this is the correct output?

agross-oss · 2017-07-03T20:29:35Z

The name should be applied to each of the cells. There are two cells in your irqs, so it should be doing the right thing. I'll test out your patches and see what is going on.

agross-oss · 2017-07-04T06:40:33Z

Found the issue. It was a bug in the script. Please see:
#680

@ydamigos

galak · 2017-07-04T15:06:49Z

#680 is now merged.

ydamigos · 2017-07-04T15:11:05Z

It was merged on master, do you plan to merge it also in arm branch?

galak · 2017-07-05T15:16:09Z

Yes, will update the arm branch

Add yaml files to DT for initial support of STM32 I2C Origin: original Signed-off-by: Yannis Damigos <[email protected]>

Configure I2C using DT for the following STM32 boards: disco_l475_iot1 nucleo_f401re 96b_carbon olimexino_stm32 Signed-off-by: Yannis Damigos <[email protected]>

ydamigos · 2017-07-06T15:29:03Z

The extract_dts_includes.py script was merged in arm branch, so I updated the PR.

…yrproject-rtos#663) Also fixing the return type to be int, since that's what zephyr returns. Signed-off-by: Geoff Gustafson <[email protected]>

ydamigos force-pushed the dts-i2c branch from 613bc0f to 40defb9 Compare July 1, 2017 13:04

galak self-assigned this Jul 1, 2017

galak requested review from erwango and agross-oss July 1, 2017 13:58

galak requested changes Jul 1, 2017

View reviewed changes

galak force-pushed the arm branch from 4c01e1a to 395daba Compare July 1, 2017 16:08

ydamigos force-pushed the dts-i2c branch from 40defb9 to 5617305 Compare July 1, 2017 19:14

galak requested changes Jul 1, 2017

View reviewed changes

ydamigos force-pushed the dts-i2c branch 3 times, most recently from 168eb2e to 6766b99 Compare July 2, 2017 10:06

mnkp reviewed Jul 2, 2017

View reviewed changes

erwango requested changes Jul 3, 2017

View reviewed changes

ydamigos force-pushed the dts-i2c branch from 6766b99 to 39df11e Compare July 3, 2017 17:22

ydamigos force-pushed the dts-i2c branch from 39df11e to b3f32f0 Compare July 3, 2017 18:59

ydamigos force-pushed the dts-i2c branch 2 times, most recently from dec8cf1 to 9454295 Compare July 4, 2017 15:42

ydamigos added 2 commits July 6, 2017 17:58

dts: yaml: Add yaml files for STM32 I2C support to DT

193715f

Add yaml files to DT for initial support of STM32 I2C Origin: original Signed-off-by: Yannis Damigos <[email protected]>

dts: arm: STM32 boards use DT to configure I2C

c7a54a0

Configure I2C using DT for the following STM32 boards: disco_l475_iot1 nucleo_f401re 96b_carbon olimexino_stm32 Signed-off-by: Yannis Damigos <[email protected]>

ydamigos force-pushed the dts-i2c branch from 9454295 to c7a54a0 Compare July 6, 2017 15:06

galak approved these changes Jul 6, 2017

View reviewed changes

galak merged commit 784814d into zephyrproject-rtos:arm Jul 6, 2017

ydamigos deleted the dts-i2c branch July 6, 2017 19:40

dts: arm: STM32 boards use DT to configure I2C #663

dts: arm: STM32 boards use DT to configure I2C #663

Uh oh!

Conversation

ydamigos commented Jul 1, 2017

Uh oh!

galak left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

galak left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ydamigos Jul 1, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ydamigos commented Jul 2, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ydamigos Jul 1, 2017 •

edited

Loading

ydamigos commented Jul 3, 2017 •

edited

Loading