drivers: add imx7d pwm driver #8794

diegosueiro · 2018-07-07T17:02:42Z

This pull request is to add PWM driver for mcimx7_m4 arch and colibri_imx7d_m4 board.

The implementation was tested on the colibri_imx7d_m4 board by compiling and running the samples/basic/blink_led and tests/drivers/pwm/pwm_api/ applications (PWM characteristics were verified using a Saleae Logic Analyzer) using the PWM_1 interface.

codecov-io · 2018-07-07T17:26:34Z

Codecov Report

Merging #8794 into master will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master    #8794   +/-   ##
=======================================
  Coverage   52.25%   52.25%           
=======================================
  Files         196      196           
  Lines       24802    24802           
  Branches     5155     5155           
=======================================
  Hits        12961    12961           
  Misses       9762     9762           
  Partials     2079     2079

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 5b8e4ae...9896fbd. Read the comment docs.

dbkinder

+1 for doc changes

MaureenHelm

Please update samples/basic/blink_led/sample.yaml and tests/drivers/pwm/pwm_api/testcase.yaml to include the colibri board. This will ensure that we build the driver in CI

MaureenHelm · 2018-07-10T21:16:06Z

drivers/pwm/pwm_imx.c

+	} else {
+		PWM_PWMCR_REG(base) = PWM_PWMCR_SWR(1);
+		do {
+			k_busy_wait(500);


Do we have to busy-wait? Can we k_sleep instead?

Would put 500 along with PWM_PWMSWR_LOOP define, so the values are written close together.

MaureenHelm · 2018-07-10T21:17:23Z

drivers/pwm/pwm_imx.c

+			k_busy_wait(500);
+			cr = PWM_PWMCR_REG(base);
+		} while ((PWM_PWMCR_SWR(cr)) &&
+			 (wait_count++ < PWM_PWMSWR_LOOP));


PWM_PWMSWR_LOOP should be a Kconfig option

MaureenHelm · 2018-07-10T21:26:53Z

ext/hal/nxp/imx/devices/MCIMX7D/clock_freq.c

+ * Description   : Get clock frequency applys to the PWM module
+ *
+ *END**************************************************************************/
+uint32_t get_pwm_clock_freq(PWM_Type *base)


I can see why you did this, but I still want to avoid modifying ext/ files. Can you implement in soc.c instead?

@MaureenHelm,

The clock_freq.c came originally from "FreeRTOS_BSP_1.0.1_iMX7D/examples/imx7d_val_m4/clock_freq.c" and I put it inside "ext/hal/nxp/imx/devices/MCIMX7D/" path following yours and @stanislav-poboril suggestion.

What should I do in this case? Move all get_pwm_clock_freq related stuff to "arch/arm/soc/nxp_imx/mcimx7_m4/" folder?

Should I have to isolate the changes for "ext/hal/nxp/imx/drivers/ccm_imx7d.h" in another header with other enums inside the "arch/arm/soc/nxp_imx/mcimx7_m4/" folder as well?

The clock_freq.c should stay in the ext folder, unmodified. The get_pwm_clock_freq() function and related changes should be either moved to soc.c (or similar soc related location) or Zephyr has a mechanism for this which is probably preferred but would require a little more effort - see drivers/clock_control/ folder and the file clock_control_mcux_ccm.c.

@MaureenHelm and @stanislav-poboril ,

This should be addressed now.

Just to be clear, did you write get_pwm_clock_freq() yourself following the pattern from clock_freq.c, or did you copy the function from somewhere? I assumed the former in my previous comment.

@MaureenHelm,

Yes, I wrote the function by myself following the same pattern from the other functions.

diegosueiro · 2018-07-16T09:23:12Z

@MaureenHelm,

I found some issues with the pwm_api test related to asserts in the i.MX UART Driver. This PR depends on the PR #8938.

stanislav-poboril · 2018-07-16T10:51:08Z

drivers/pwm/pwm_imx.c

+		return -EINVAL;
+	}
+
+	duty_cycle = 100 * pulse_cycles / period_cycles;


The duty_cycle variable is used for logging only. Maybe the formula can be written directly in the logging statement. But compiler would probably optimize it anyway if log not used.

Another reason to move the formula is to avoid a compiler warning when you build with logging turned off.

stanislav-poboril · 2018-07-16T10:52:46Z

drivers/pwm/pwm_imx.c

+	struct imx_pwm_data *data = DEV_DATA(dev);
+	u8_t duty_cycle;
+	unsigned int period_ms;
+	bool enable = imx_pwm_is_enabled(base);


Would rename to enabled or is_enabled. At the end of the function, PWM should be enabled anyway (unless there is an error in parameters etc).

stanislav-poboril · 2018-07-16T10:55:28Z

drivers/pwm/pwm_imx.c

+	} else {
+		PWM_PWMCR_REG(base) = PWM_PWMCR_SWR(1);
+		do {
+			k_busy_wait(500);


Would put 500 along with PWM_PWMSWR_LOOP define, so the values are written close together.

stanislav-poboril · 2018-07-16T10:56:13Z

drivers/pwm/pwm_imx.c

+	if (period_cycles > 2) {
+		period_cycles -= 2;
+	} else {
+		period_cycles = 0;


If the period_cycles < 2, should the function return error?

stanislav-poboril · 2018-07-16T11:07:59Z

drivers/pwm/pwm_imx.c

+
+	if (data->period_cycles != period_cycles) {
+		data->period_cycles = period_cycles;
+		PWM_PWMPR_REG(base) = period_cycles;


If there are still some data in the sample register FIFO, say 1-3, could this happen:

We change the period to another value.

Samples in the FIFO were meant to be output with the previous period value.

But they will be output with the new period value, it may even be shorter than the sample value.

This is probably not very usual to change the period and I know the hardware does not allow to handle it better, though (unless we maintain our own fifo with both the cycle and period and set the HW FIFO low watermark to 1 and change the period in interrupt at the last moment).

@stanislav-poboril ,

Maybe in the "if (enable)" block it can wait for a full cycle if there is still 1,2,3 or 4 words of data in the FIFO. I think this way it will only configure a new period cycle if the FIFO is empty.
What do you think?

The best would be to try it. But then it could be perhaps too late to set the new period - there could be delays between stream of output with different cycles, which means FIFO would be emptied and the last cycle would repeat once more before changing the period.

I think the peripheral was created with the following use case in mind where period is set once and then the FIFO is filled ahead of time to keep it from underflowing (or let it underflow and repeat the same cycle until stopped or changing the cycle).

My previous comment would be probably relevant in a scenario, where the exact number of cycles has to be output (some sort of audio output?), but then I think it probably does not makes a sense to change the period duration.

Well I am not sure at the end if we should do anything about this.

Perhaps we can leave as is (I used the same approach as the Linux kernel driver) for now and add a comment in the source code about this specific scenario that is not covered?

@MaureenHelm,
What do you think?

I handled a similar case in pwm_mcux_ftm.c by logging a warning. You can do the same here for now.

zephyr/drivers/pwm/pwm_mcux_ftm.c

Line 61 in 16ff8ca

SYS_LOG_WRN("Changing period cycles from %d to %d"

MaureenHelm · 2018-07-16T17:30:12Z

drivers/pwm/pwm_imx.c

+		return -EINVAL;
+	}
+
+	duty_cycle = 100 * pulse_cycles / period_cycles;


Another reason to move the formula is to avoid a compiler warning when you build with logging turned off.

MaureenHelm · 2018-07-16T17:50:37Z

drivers/pwm/pwm_imx.c

+
+	if (data->period_cycles != period_cycles) {
+		data->period_cycles = period_cycles;
+		PWM_PWMPR_REG(base) = period_cycles;


I handled a similar case in pwm_mcux_ftm.c by logging a warning. You can do the same here for now.

zephyr/drivers/pwm/pwm_mcux_ftm.c

Line 61 in 16ff8ca

SYS_LOG_WRN("Changing period cycles from %d to %d"

MaureenHelm · 2018-07-16T17:53:59Z

boards/arm/colibri_imx7d_m4/Kconfig.defconfig

+	def_bool y
+
+config PWM_2
+	def_bool n


The def_bool n are redundant. I think all but PWM_1 should be removed.

@ulfalizer Will you please confirm?

diegosueiro · 2018-07-17T06:26:19Z

@MaureenHelm and @stanislav-poboril ,

All comments should be addressed now.

stanislav-poboril · 2018-07-17T07:18:24Z

drivers/pwm/pwm_imx.c

+	} else {
+		PWM_PWMCR_REG(base) = PWM_PWMCR_SWR(1);
+		do {
+			k_sleep(1);


I didn't realize before that the k_busy_wait(500) is 500 microseconds, vs k_sleep(1) is one millisecond, but due to the default quantum is 10 milliseconds in Zephyr/ARM, it will sleep 10 milliseconds or more (unless kernel parameters are changed, you have maybe changed the system tick to 1 ms). Is this ok?

MaureenHelm

Just two small nits

MaureenHelm · 2018-07-17T15:32:30Z

drivers/pwm/pwm_imx.c

+			k_sleep(1);
+			cr = PWM_PWMCR_REG(base);
+		} while ((PWM_PWMCR_SWR(cr)) &&
+			 (wait_count++ < CONFIG_PWM_PWMSWR_LOOP));


This should be ++wait_count

MaureenHelm · 2018-07-17T15:34:58Z

drivers/pwm/pwm_imx.c

+			 (wait_count++ < CONFIG_PWM_PWMSWR_LOOP));
+
+		if (PWM_PWMCR_SWR(cr))
+			SYS_LOG_WRN("software reset timeout\n");


Need to wrap in braces

This patch adds the get_pwm_clock_freq function to be used by the i.MX PWM driver. Signed-off-by: Diego Sueiro <[email protected]>

Adds the PWM shim driver for i.MX Signed-off-by: Diego Sueiro <[email protected]>

Adds definitions, devicetree entries and clock controller configurations for PWM peripheral. Signed-off-by: Diego Sueiro <[email protected]>

Adds configurations and pinmux to support PWM on colibri_imx7d_m4 Signed-off-by: Diego Sueiro <[email protected]>

Adds the appropriated configuration for colibri_imx7d_m4 board in blink_led sample app. Signed-off-by: Diego Sueiro <[email protected]>

Adds the appropriated configuration for colibri_imx7d_m4 board in pwm_api tests. Signed-off-by: Diego Sueiro <[email protected]>

diegosueiro requested review from dbkinder and MaureenHelm as code owners July 7, 2018 17:02

dbkinder approved these changes Jul 9, 2018

View reviewed changes

MaureenHelm requested changes Jul 10, 2018

View reviewed changes

MaureenHelm requested a review from stanislav-poboril July 10, 2018 22:56

MaureenHelm added area: PWM Pulse Width Modulation platform: NXP NXP labels Jul 10, 2018

diegosueiro force-pushed the imx7d_pwm branch from 2f10b6c to 608e73d Compare July 16, 2018 10:58

stanislav-poboril suggested changes Jul 16, 2018

View reviewed changes

stanislav-poboril reviewed Jul 16, 2018

View reviewed changes

MaureenHelm requested changes Jul 16, 2018

View reviewed changes

diegosueiro force-pushed the imx7d_pwm branch 3 times, most recently from 7eb88c0 to 276a17f Compare July 17, 2018 05:45

stanislav-poboril reviewed Jul 17, 2018

View reviewed changes

stanislav-poboril closed this Jul 17, 2018

stanislav-poboril reopened this Jul 17, 2018

stanislav-poboril approved these changes Jul 17, 2018

View reviewed changes

MaureenHelm requested changes Jul 17, 2018

View reviewed changes

diegosueiro added 6 commits July 17, 2018 17:24

arch: Add i.MX7 PWM get clock frequency function

cff307b

This patch adds the get_pwm_clock_freq function to be used by the i.MX PWM driver. Signed-off-by: Diego Sueiro <[email protected]>

drivers: add i.MX PWM driver

7f8f498

Adds the PWM shim driver for i.MX Signed-off-by: Diego Sueiro <[email protected]>

arch: Add support for i.MX PWM

aa71bb6

Adds definitions, devicetree entries and clock controller configurations for PWM peripheral. Signed-off-by: Diego Sueiro <[email protected]>

board: Add PWM support for colibri_imx7d_m4

cc13755

Adds configurations and pinmux to support PWM on colibri_imx7d_m4 Signed-off-by: Diego Sueiro <[email protected]>

samples: Add colibri_imx7d_m4 config in blink_led

9f95d62

Adds the appropriated configuration for colibri_imx7d_m4 board in blink_led sample app. Signed-off-by: Diego Sueiro <[email protected]>

tests: Add colibri_imx7d_m4 config in pwm_api

9896fbd

Adds the appropriated configuration for colibri_imx7d_m4 board in pwm_api tests. Signed-off-by: Diego Sueiro <[email protected]>

diegosueiro force-pushed the imx7d_pwm branch from 276a17f to 9896fbd Compare July 17, 2018 16:24

MaureenHelm approved these changes Jul 17, 2018

View reviewed changes

MaureenHelm merged commit 5d1b57f into zephyrproject-rtos:master Jul 17, 2018

diegosueiro deleted the imx7d_pwm branch July 19, 2018 03:08

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

drivers: add imx7d pwm driver #8794

drivers: add imx7d pwm driver #8794

diegosueiro commented Jul 7, 2018

codecov-io commented Jul 7, 2018 •

edited

Loading

dbkinder left a comment

MaureenHelm left a comment

MaureenHelm Jul 10, 2018

stanislav-poboril Jul 16, 2018

MaureenHelm Jul 10, 2018

MaureenHelm Jul 10, 2018

diegosueiro Jul 16, 2018 •

edited

Loading

stanislav-poboril Jul 16, 2018 •

edited

Loading

diegosueiro Jul 16, 2018

MaureenHelm Jul 16, 2018

diegosueiro Jul 16, 2018

diegosueiro commented Jul 16, 2018

stanislav-poboril Jul 16, 2018

MaureenHelm Jul 16, 2018

stanislav-poboril Jul 16, 2018

stanislav-poboril Jul 16, 2018

stanislav-poboril Jul 16, 2018

stanislav-poboril Jul 16, 2018

diegosueiro Jul 16, 2018

stanislav-poboril Jul 16, 2018

diegosueiro Jul 16, 2018

MaureenHelm Jul 16, 2018

MaureenHelm Jul 16, 2018

MaureenHelm Jul 16, 2018

MaureenHelm Jul 16, 2018

diegosueiro commented Jul 17, 2018

stanislav-poboril Jul 17, 2018

MaureenHelm left a comment

MaureenHelm Jul 17, 2018

MaureenHelm Jul 17, 2018

drivers: add imx7d pwm driver #8794

drivers: add imx7d pwm driver #8794

Conversation

diegosueiro commented Jul 7, 2018

codecov-io commented Jul 7, 2018 • edited Loading

Codecov Report

dbkinder left a comment

Choose a reason for hiding this comment

MaureenHelm left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

diegosueiro Jul 16, 2018 • edited Loading

Choose a reason for hiding this comment

stanislav-poboril Jul 16, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

diegosueiro commented Jul 16, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

diegosueiro commented Jul 17, 2018

Choose a reason for hiding this comment

MaureenHelm left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov-io commented Jul 7, 2018 •

edited

Loading

diegosueiro Jul 16, 2018 •

edited

Loading

stanislav-poboril Jul 16, 2018 •

edited

Loading