Skip to content

Conversation

@gmarull
Copy link
Member

@gmarull gmarull commented Nov 1, 2021

This PR adds a new pinctrl driver for GD32 SoCs for both AF and AFIO pinmux models.

Tested on:

It depends on:

@github-actions
Copy link

github-actions bot commented Nov 1, 2021

The following west manifest projects have been modified in this Pull Request:

Name Old Revision New Revision Diff
hal_gigadevice zephyrproject-rtos/hal_gigadevice@85f10a6 zephyrproject-rtos/hal_gigadevice@cb4cc10 (main) zephyrproject-rtos/[email protected]

Note: This message is automatically posted and updated by the Manifest GitHub Action.

@gmarull
Copy link
Member Author

gmarull commented Nov 7, 2021

@nandojve Support for AFIO (like GD32F403) should now be ready. However, I haven't been able to test it as I don't have that board (will try on VF103 using #34970)

@nandojve
Copy link
Member

nandojve commented Nov 8, 2021

@nandojve Support for AFIO (like GD32F403) should now be ready. However, I haven't been able to test it as I don't have that board (will try on VF103 using #34970)

Great ! I'll try soon and report to you.

@nandojve
Copy link
Member

Hi @gmarull ,

I tested using gd32f403 without sucess, no characters at serial port.
I copy usart0_default resulting node here to help you check if something is missing.

usart0_default: usart0_default {
	phandle = < 0x2 >;
	pins1 {
		pinmux = < 0x11290 >, < 0x111a0 >;
	};
};

Copy link
Member

@nandojve nandojve left a comment

Choose a reason for hiding this comment

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

Hi @gmarull ,

Excellent work!

I start to look from manifest update and have only small comments.

Copy link
Member

@nandojve nandojve left a comment

Choose a reason for hiding this comment

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

Hi @gmarull ,

After some fixes AFIO is working:

*** Booting Zephyr OS build v2.7.99-1382-g31b26bab517c  ***
Running test suite pinctrl_gd32
===================================================================
START - test_dt_extract
 PASS - test_dt_extract in 0.1 seconds
===================================================================
Test suite pinctrl_gd32 succeeded
===================================================================
PROJECT EXECUTION SUCCESSFUL

Comment on lines +10 to +14
Copy link
Member

Choose a reason for hiding this comment

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

bias-high-impedance is missing

Copy link
Member Author

Choose a reason for hiding this comment

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

This option is not required, same as STM32 (see https://github.com/zephyrproject-rtos/zephyr/blob/main/dts/bindings/pinctrl/st%2Cstm32-pinctrl.yaml#L18). Not setting any bias and operating as input would be sufficient as far as I understand.

Copy link
Member

Choose a reason for hiding this comment

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

Not setting any bias and operating as input would be sufficient as far as I understand.

I understand not setting any bias, which means bias-disable (default state), equals Analog. We should not work with a hidden state because people can made confusion. The configuration table have 4 options for Input and there are 3 options defined. If you look at

bias-high-impedance:
required: false
type: boolean
description: high impedance mode ("third-state", "floating")
this is the option that Linux define for the Input Floating.

Copy link
Member Author

@gmarull gmarull Nov 15, 2021

Choose a reason for hiding this comment

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

The problem is that analog is treated via the pinmux value as a separate mode, not as an input mode. In case of DAC (an output signal), ANALOG still has to be used. For low-power mode, what is usually done is to set the analog mode (default config) on a pin, this is why I added the analog option for all pins in the generator:

/* ANALOG */
#define ANALOG_PA0 \
	GD32_PINMUX_AFIO('A', 0, ANALOG, NORMP)
#define ANALOG_PA1 \
	GD32_PINMUX_AFIO('A', 1, ANALOG, NORMP)
...

I know it can be a bit confusing, it's really STM32 inheritance (it's done this way, see for example https://elixir.bootlin.com/linux/latest/source/arch/arm/boot/dts/stm32mp15-pinctrl.dtsi#L62). I think that treating analog in a special way makes things more understandable when it comes to the analog signals, but not that clear for low-power modes. In such case, it would probably make more sense to configure the pin as an input and set bias-high-impedance. But familiarity with STM32 has value for users experienced in that platform.

Copy link
Member

Choose a reason for hiding this comment

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

In this case, it is important give some background for users. The information you presented at last comment is relevant to users understand how this should be configured. Other important thing is that no one can determine from where user will start to read the documentation. I think it could be very valuable improve documentation and try connect those things. For instance, correlate the GPIO configuration table with the options available can be very valuable to users. The same way, explain to then the why Analog is configured differently will help. Maybe provide pointer that exists in Zephyr can help to. Take in consideration that a lot of users don't know Linux and devicetree.

Copy link
Member Author

Choose a reason for hiding this comment

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

I have updated the bindings description explaining more details on low power modes, please check.

Comment on lines +39 to +73
Copy link
Member

Choose a reason for hiding this comment

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

bias-high-impedance is missing

Copy link
Member Author

Choose a reason for hiding this comment

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

ditto

Copy link
Member

Choose a reason for hiding this comment

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

bias-disable: Disable pull-up/down (default, not required).

What is this? Analog or High-Impedance? Again, it is not clear the meaning and it is possible improve here to a very clear pin state.

Comment on lines +60 to +68
Copy link
Member

Choose a reason for hiding this comment

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

bias-high-impedance is missing

Copy link
Member Author

Choose a reason for hiding this comment

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

ditto

@gmarull gmarull force-pushed the gd32-pinctrl branch 2 times, most recently from e4690d7 to 4799cd9 Compare November 13, 2021 17:30
@gmarull gmarull requested a review from nandojve November 13, 2021 17:30
@gmarull gmarull force-pushed the gd32-pinctrl branch 2 times, most recently from 3b01287 to c8f784d Compare November 14, 2021 18:06
@github-actions github-actions bot added the DNM This PR should not be merged (Do Not Merge) label Nov 14, 2021
@gmarull
Copy link
Member Author

gmarull commented Nov 14, 2021

@cameled reviews welcome

@github-actions github-actions bot removed the DNM This PR should not be merged (Do Not Merge) label Nov 18, 2021
Copy link
Member

@nandojve nandojve left a comment

Choose a reason for hiding this comment

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

Hi @gmarull ,

Thank you for this PR!

Copy link
Contributor

@mbolivar-nordic mbolivar-nordic left a comment

Choose a reason for hiding this comment

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

I didn't review this very closely, but it looks like it's following all the latest practices from a quick skim.

gmarull and others added 13 commits November 22, 2021 16:46
Include the module `include` folder to allow access to the dt-bindings
helpers.

Signed-off-by: Gerard Marull-Paretas <[email protected]>
Add a pin control driver for GD32 SoCs using the AF model.

Signed-off-by: Gerard Marull-Paretas <[email protected]>
Add definitions for the pinctrl and gpio nodes.

Signed-off-by: Gerard Marull-Paretas <[email protected]>
GD32F4XX series have AF based pinmux.

Signed-off-by: Gerard Marull-Paretas <[email protected]>
Add a pin control driver for GD32 SoCs using the AFIO model.

Thanks to Gerson Fernando Budke for testing and implementation
suggestions.

Co-authored-by: Gerson Fernando Budke <[email protected]>
Signed-off-by: Gerard Marull-Paretas <[email protected]>
Add afio, pinctrl and gpioa...g entries for the GD32F403 series.

Signed-off-by: Gerard Marull-Paretas <[email protected]>
GD32F403 series use AFIO pinmux model.

Signed-off-by: Gerard Marull-Paretas <[email protected]>
Enable pinctrl by default, since it is an essential component on almost
every firmware. Inclusion of series defconfig has also been guarded with
SoC availability (was missing).

Signed-off-by: Gerard Marull-Paretas <[email protected]>
Use the pinctrl API to configure peripheral pins.

Signed-off-by: Gerard Marull-Paretas <[email protected]>
Enable usage of the pinctrl driver.

Signed-off-by: Gerard Marull-Paretas <[email protected]>
Enable usage of pinctrl.

Signed-off-by: Gerard Marull-Paretas <[email protected]>
Add a test to check that DT information for AF model is extracted
correctly.

Signed-off-by: Gerard Marull-Paretas <[email protected]>
Add a test to check that DT information for the AFIO model is extracted
correctly.

Signed-off-by: Gerard Marull-Paretas <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants