pinctrl: add NPCX implementation.

[MulinChao]

This pull request contains the following changes:

• Provide NPCX pinctrl implementation based on Road from pinmux to pinctrl #39740.
  Add pinctrl implementation in pinctrl_npcx.c.
  Merge npcx pinctrl configuration into pinctrl_soc_pin_t structure in soc/arm/nuvoton_npcx/common/pinctrl_soc.h
  Keep part of the existing but non-generic NPCX soc pinctrl utilities still. (Will remove them in the following PRs.)
• Implement pinctrl mechanism for npcx peripheral drivers such as uart, pwm, espi and so on.
  Inherit pinctrl-device in peripheral drivers' yaml files
  Add required 'pinctrl-names' properties on related peripheral device-tree nodes.
  Replace npcx utilities with pinctrl APIs for generic pinctrl functionalities in the driver.

Verified devices: (ongoing,)
npcx7m6fb_evb, npcx9m6f_evb, and volteer/lazer chromebooks.

Since Nuvton ec has been applied on notebooks for decades, the needs such as firmware backward compatibility are to be considered. Hence, there are three consideration factors during pinctrl driver refactoring.

1. The pinctrl functionalities are scatted in different modules such as scfg, glue, gpio, and peripheral devices themselves. Not all IO pads have all pinctrl properties from the cost perspective.
2. The IO-pads' functionalities such as open-drain, pull-up/down are different from gpio and peripheral devices in npcx series. For example, users can select pull-up/down, push-pull and open-drain properties of gpiob5/b4 via gpio module. But it will be open-drain bias type automatically if pin-mux is selected to the i2c module. The settings of gpiob5/b4 don't influence i2c's pad functionalities even though they share the same pins. (Besides low-voltage input feature. It influences both gpio and peripheral devices.)
3. The layout for corresponding peripheral/io and control reg/bit is irregular. The mapping tables for pin-muxing, 1.8v support, and so on are introduced and needed in the current implementation. But it is not one-on-one mapping between all pinctrl functionalities. For example, users only need to set one control bit to switch 8 pins from gpio to espi module, but they need to configure these pins one by one if they are selected to gpio. And few pins need two or more control bits for pin-muxing if they share two or more peripheral devices.

This PR is the first attempt for npcx pinctrl driver and adds pinctrl nodes for each npcx series. According to the above points, it introduces pinctrl nodes for peripheral devices. And wraps all configurations no matter general pin-muxing or soc-specific features such as pinmux-locked in pinctrl_soc.h. Users set pin configuration via these nodes in the board layout DT file by the following ways:

1. Pinmuxing for peripheral devices

&uart1 {
        status = "okay";
        pinctrl-0 = <&pad_uart1_2_sin_sout>; /* Use UART1_SL2 ie. PIN64.65 */
        pinctrl-names = "default";
};

2. Configure peripheral devices' pad property by overwriting prebuild nodes in pinctrl nodes directly. (Not all of them are fully configurable.)

&pad_i2c0_0_sda_scl {
	bias-pull-up; /* Enable internal pull-up for i2c0_0 */
	pinmux-locked; /* Lock pinmuxing */
};

&i2c0_0 {
	status = "okay";
	pinctrl-0 = <&pad_i2c0_0_sda_scl>;
	pinctrl-names = "default";
	clock-frequency = <I2C_BITRATE_FAST>;
};

Regarding gpio and psl pads configurations via pinctrl driver, they will be introduced in the following PR.

[MulinChao]

Hi all, this PR has been verified on all devices on my side and marked as ready. Any comments are highly appreciated.

[keith-zephyr]

Some general comments:

Does this pinctrl driver provide any mechanism for setting the pullup/pulldown, and open drain properties? I believe these are set using the GPIO module in the NPCX, while the alternate function support is set through the system configuration DEVALTn registers.

It might be useful to add an NPCX specific pinctrl state called "locked". This state would be used set the corresponding DEVALTn_LK register.

Right now, as gerard notes, you have setup the default pinctrl for each driver. The I2C ports support is currently modeled as an I2C port/I2C controller pair. With the pinctrl support, this could be refactored so that the board uses the pinctrl-0 property to select the correct I2C port.

For example, boards using I2C controller 4, port 1, are configured with the current implementation like this:

&i2c6_1 {
	status = "okay";
	clock-frequency = <I2C_BITRATE_FAST>;
};

This could be refactored as:

/* Use I2C 6, port 0 */
&i2c6 {
	status = "okay";
	clock-frequency = <I2C_BITRATE_FAST>;
	pinctrl-0 = <&alt2_i2c6_0_sl>; /* PINC2.C1 */
};
/* Use I2C 6, port 1 */
&i2c6 {
	status = "okay";
	clock-frequency = <I2C_BITRATE_FAST>;
	pinctrl-0 = <&alt6_i2c6_1_sl>; /* PINE4.E3 */
};

[MulinChao]

Thanks for your comments. Please see my reply below.

Some general comments:
Does this pinctrl driver provide any mechanism for setting the pullup/pulldown, and open drain properties? I believe these are set using the GPIO module in the NPCX, while the alternate function support is set through the system configuration DEVALTn registers.

Not only NPCX, the other soc series also use gpios property to configure the pullup/pulldown, and open drain functionalities of IO pads. For alternative functions, they are usually configured by pinctrl-0/1/2 of the peripheral device DT node.

It might be useful to add an NPCX specific pinctrl state called "locked". This state would be used set the corresponding DEVALTn_LK register.

Indeed, it is a good idea. I will add a new pinctrl state, locked, and its mechanism in this PR. Then, will submit it for code review after passing verification on my side.

Right now, as gerard notes, you have setup the default pinctrl for each driver. The I2C ports support is currently modeled as an I2C port/I2C controller pair. With the pinctrl support, this could be refactored so that the board uses the pinctrl-0 property to select the correct I2C port.

Here is the chart which demonstrates npcx I2C port and controller pair.

              DEVALTn   Port SEL
                 |        |
                 |      |\|
 SCL/SDA Port0 x-+------| \     +--------------+
                        |  |----|   SMB/I2C    |
                        |  |----| Controller N |
 SCL/SDA Port1 x-+------|  |    +--------------+
                 |      | /
                 |      |/
              DEVALTn

If users use both i2c6_0 and i2c6_1 ports for the application, all relevant selection bits in DEVALTn must be set, and switch pin-mux from GPIO to I2C. By setting the i2c-port DT node's status to okay. Or users need to set pinctrl-0 explicitly as below:

/* Use I2C 6, port 0  only */
&i2c6 {
	status = "okay";
	clock-frequency = <I2C_BITRATE_FAST>;
	pinctrl-0 = <&alt2_i2c6_0_sl>; /* PINC2.C1 */
};

/* Use I2C 6, port 1 only */
&i2c6 {
	status = "okay";
	clock-frequency = <I2C_BITRATE_FAST>;
	pinctrl-0 = <&alt6_i2c6_1_sl>; /* PINE4.E3 */
};

/* Use I2C 6, port 0 and 1 */
&i2c6 {
	status = "okay";
	clock-frequency = <I2C_BITRATE_FAST>;
	pinctrl-0 = <&alt2_i2c6_0_sl &alt6_i2c6_1_sl>; /* PINC2.C1 PINE4.E3 */
};

In my opinion, I still think putting pinctrl-0 in i2c-port is more suitable since it fits our design and prevents users set wrong configurations in this property.

[keith-zephyr]

In my opinion, I still think putting pinctrl-0 in i2c-port is more suitable since it fits our design and prevents users set wrong configurations in this property.

This was my thought as well. This will also simplify the devicetree for the user. Today the user needs to enable both the I2C port (&i2c0_0) and the I2C controller (&i2c_ctrl0).

[gmarull]

lgtm, a couple of minor issues

[gmarull]

this property should be part of the pwm6_gpc0 node

[MulinChao]

Done. Setting bit 7 in PWMCTLEX selects the type of the PWM_n output buffer to push-pull or open-drain. I don't know it's a good idea to move it to pinctrl_npcx.c. Hence, I still keep it in the pwm driver.

[MulinChao]

Please review the latest PR.

[gmarull]

this needs to be handled by a pin control driver, not like this. If some pin properties are handled directly by certain IP blocks (like e.g. Nordic), you can either encode address/bit in the state node or make use of the PINCTRL_STORE_REG, so that you also get the peripheral address when being called.

[MulinChao]

Got it. Will modify it for code review later.

[MulinChao]

Done. The latest PR has moved sources that configure drive-open-drain from pwm_npcx.c to pinctrl_npcx.c. In order to make sure static casting of reg is reliable, a new property, drive-supported, is introduced in pwm pinctrl rebuild nodes.

[gmarull]

Thanks for the continued effort

[MulinChao]

Thanks for the continued effort

Thanks for the detailed explanation. It helps a lot.

Hi @keith-zephyr, do you have any comments for the latest version?

[keith-zephyr]

Thanks for the continued effort

Thanks for the detailed explanation. It helps a lot.

Hi @keith-zephyr, do you have any comments for the latest version?

I have been on vacation. But I will review this tomorrow (May 11).

[MulinChao]

In order to resolve the conflicts, I rebase the branch onto ToT. Sorry for dismissing @gmarull's approval. Could you review it again?

[keith-zephyr]

Non-blocking comment - this adds 4 bytes to every pinctrl node correct? But this is only needed by the PWM nodes.

If you can repack the bitfields of npcx_periph to use 3 bits to store the PWM instance number. You could then store the PWM base addresses into npcx_pinctrl_cfg similar to the base_scfg field.

Using PINCTRL_STORE_REG is a cleaner approach, so you would need to evaluate how much flash space is saved by encoding the PWM instance in place of using reg.

[MulinChao]

Non-blocking comment - this adds 4 bytes to every pinctrl node correct? But this is only needed by the PWM nodes.

ML> If you mean each given device that applies pinctrl mechanism will increase 4 bytes, yes, you're correct.

If you can repack the bitfields of npcx_periph to use 3 bits to store the PWM instance number. You could then store the PWM base addresses into npcx_pinctrl_cfg similar to the base_scfg field.
Using PINCTRL_STORE_REG is a cleaner approach, so you would need to evaluate how much flash space is saved by encoding the PWM instance in place of using reg.

ML> I have thought about that before. But if we have a chance to add drive mode support for the other peripheral devices, PINCTRL_STORE_REG is a clearer way to maintain pinctrl mechanism. As I mentioned above, the flash space increases by 4 bytes for every device using pinctl-names and proctrl-0/1/2 properties for pinctrl mechanism. If we need to save flash spaces in the furture, I prefer to create the other PR that includes:

1. Restore device's label name in npcx_periph
2. Use device_get_binding to get the correct register address.

But the cost is the latency of selecting drive mode is longer.

[gmarull]

Note that device_get_binding is rarely needed these days, DEVICE_DT_GET can be used for most situations. It is sometimes not necessary to store a device pointer either, see e.g. how GD32 pinctrl driver handles selected port.

[MulinChao]

Hi @gmarull & @keith-zephyr, sorry for the noise. The latest changes only rename variables for the recent comments. Please review it and give your comment freely.
BTW, I also submitted a new PR adding a helper macro to prevent build error if the target device hasn't reg property.