Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

dts: add HRTIM pinctrl #87

Merged
merged 2 commits into from
Mar 19, 2021
Merged

Conversation

martinjaeger
Copy link
Member

Add options for high-resolution timer HRTIM to pinctrl-config and generate new pinctrl DTS files.

I know that the HRTIM peripheral is not yet supported by Zephyr, but I'm working on a driver and would like to use the pinctrl properly already.

Copy link
Member

@gmarull gmarull left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@martinjaeger We usually add the config changes in one commit and the autogenerated content into another one to ease review process.

@martinjaeger
Copy link
Member Author

Ah, makes sense. Will fix that tomorrow.

Add signals generation for high-resolution timer (HRTIM).

HRTIM is not available on F1 targets.

Signed-off-by: Martin Jäger <[email protected]>
New batch of pinctrl.dtsi files including HRTIM signals

Signed-off-by: Martin Jäger <[email protected]>
@luizvilla
Copy link

luizvilla commented Feb 4, 2021

Wonderful work @martinjaeger.
We were looking for this. We'll implement and leave a comment here.

@martinjaeger
Copy link
Member Author

Wonderful work @martinjaeger.
We were looking for this. We'll implement and leave a comment here.

Great. Zephyr has feature freeze at the moment until v2.5 is released (scheduled for 12 February). After that we should probably get this PR in rather quickly.

In the meantime you can already add something like this to your board.dts as a workaround:

	soc {
		hrtim1: timers@40016800 {
			compatible = "st,stm32-timers";
			reg = <0x40016800 0x400>;
			clocks = <&rcc STM32_CLOCK_BUS_APB2 0x04000000>;
			interrupts = <67 0>, <68 0>, <69 0>, <70 0>, <71 0>, <72 0>, <73 0>, <74 0>;
			interrupt-names = "master", "tima", "timb", "timc", "timd", "time", "flt", "timf";
			status = "okay";
			label = "HRTIM_1";
		};

		/* required until hal_stm32 PR #87 is merged */
		pinctrl: pin-controller@48000000 {
			hrtim1_cha1_pa8: hrtim1_cha1_pa8 {
				pinmux = <STM32_PINMUX('A', 8, AF13)>;
			};

			hrtim1_cha2_pa9: hrtim1_cha2_pa9 {
				pinmux = <STM32_PINMUX('A', 9, AF13)>;
			};

			// ...
		};
	};

@galak
Copy link
Contributor

galak commented Feb 16, 2021

Is there a Zephyr side PR related to this?

@martinjaeger
Copy link
Member Author

Is there a Zephyr side PR related to this?

Not yet. The driver is still work in progress.

It doesn't really make sense to use the HRTIM with the existing "simple" PWM API. The HRTIM is meant for motor driver and power converter applications with complementary PWM outputs. So we'll probably have to come up with a new API for that (half_bridge, sync_pwm, complementary_pwm or how ever it may be called).

@erwango erwango added the DNM label Mar 17, 2021
@erwango
Copy link
Member

erwango commented Mar 17, 2021

@martinjaeger Set as DNM waiting for a PR to use these in main Zephyr repo. Any objections ?

@gmarull
Copy link
Member

gmarull commented Mar 17, 2021

@erwango I think it is still fine to have definitions for peripherals that are currently not used in-tree. It can be helpful for people developing custom drivers.

@martinjaeger
Copy link
Member Author

martinjaeger commented Mar 17, 2021

@erwango I think it is still fine to have definitions for peripherals that are currently not used in-tree. It can be helpful for people developing custom drivers.

I agree with @gmarull. The pinctrl driver is fully supported upstream and this PR only adds features to the pinctrl driver.

IMO we could even say that we generate all pinctrl devicetree nodes that are defined in the original ST cube XML files in order to make out-of-tree driver development on top of pinctrl easier.

We don't need a dedicated commit upstream to update west.yml. But I think it would not harm anyone to just merge this here and get it in together with the next update.

@erwango
Copy link
Member

erwango commented Mar 17, 2021

@gmarull, @martinjaeger Ok, it doesn't harm indeed and I don't have strong opinion otherwise.

IMO we could even say that we generate all pinctrl devicetree nodes that are defined in the original ST cube XML files in order to make out-of-tree driver development on top of pinctrl easier.

I'm not particularly of favor of this as I prefer to have users and reviewers for each peripherals (as it seems to be that case here), and avoid generating blindly configurations that could be potentially broken. So I prefer this remains a case by case operation.

@erwango erwango removed the DNM label Mar 17, 2021
@erwango erwango requested a review from galak March 17, 2021 08:26
@erwango
Copy link
Member

erwango commented Mar 17, 2021

@galak, this one is ready for merge.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants