Add pinctrl driver for Quicklogic EOS S3 #60095

wsipak · 2023-07-06T12:51:34Z

This PR adds a new driver for the Quicklogic EOS S3 SoC.
Currently, there are two boards that do pinmuxing in their board.c files: quick_feather and qomu.
The legacy code is removed and now the driver does all the pinmuxing.

Fixes #59186.

gmarull · 2023-07-12T08:42:31Z

dts/bindings/pinctrl/quicklogic,eos-s3-pinctrl.yaml

+        pinmux = <QUICKLOGIC_EOS_S3_PINMUX_UART_RX_PAD QUICKLOGIC_EOS_S3_PINMUX_UART_RX_PAD_CFG
+                  QUICKLOGIC_EOS_S3_PINMUX_FUNC_SEL_UART_RX>;


why do we need 3 macros here? can't a single one be used to specify the UART RX functionality?

In order to select an input function, one must first program IOMUX_PAD_x_CTRL register,
and then route the signal to functional block using IOMUX_<function>_SEL.

The second element in the DTS is used to set proper function to a physical pad.
The third element in the DTS is used for selecting proper IOMUX_<function_SEL register, where the pad number must be written.
These values are independent and one cannot be deduced using the other.

I mean, can't this all be encoded in 32-bit + dt props?

By looking at e.g. the RX case we have:

#define QUICKLOGIC_EOS_S3_PINMUX_UART_RX_PAD 45 #define QUICKLOGIC_EOS_S3_PINMUX_FUNC_SEL_UART_RX 77 #define QUICKLOGIC_EOS_S3_PINMUX_UART_RX_PAD_CFG \ (EOS_S3_UART_RXD_SEL_PAD45 | EOS_S3_PAD_OEN_DISABLE \ | EOS_S3_PAD_E_4MA | EOS_S3_PAD_REN_ENABLE)

PAD 45 is duplicated, for example. And what do all flags like OEN, REN, etc mean?

The reason it's done this way is related to the code that uses the HAL and was used for IO MUX:

eos_s3_io_mux(UART_TX_PAD, UART_TX_PAD_CFG); eos_s3_io_mux(UART_RX_PAD, UART_RX_PAD_CFG); IO_MUX->UART_rxd_SEL = UART_RX_SEL;

In order to do the same thing in the driver as depicted in the third line, one must deduce that the input function for UART_rxd is being set.
However, for different pads that can serve for the same function, the function values differ:

HAL/inc/eoss3_dev.h 1451:#define UART_RXD_SEL_PAD16 ((uint32_t) (0x00000002)) 1452:#define UART_RXD_SEL_PAD45 ((uint32_t) (0x00000004))

Knowing that the function is 0x4 is not enough to conclude that the function of UART_RX is being set here.
Therefore, to ascertain this, we would need to map each (PAD, FUNCTION) pair to a register in IO_MUX.
The value assigned would correspond to the shift to this field in the struct:
https://github.com/zephyrproject-rtos/hal_quicklogic/blob/ba7e35f175fcd28199de11f955cb995e1ef20c73/HAL/inc/eoss3_dev.h#L469

The flags used in the code here are utilized to configure active low output, drive strength and receive enable respectively.
The second value provided in the DTS configures the pad as input using these parameters.
While it's a 32-bit value, based on the S3 TRM (pp. 204-205), it appears that only bits [12:0] are actually used. We could pass some information on the more siginifant bits to the driver.

I can see that passing 3 32-bit values seems a bit much as for an IO multiplexer, and I'm sure I can add a mapping of (PAD, FUNCTION) -> register (Is this the preferred way?).

Considering the perspecive of both the TRM of EOS S3 and the HAL, it seemed logical to me that the configuration of inputs would require the pad number and two other values. Also, the current solution doesn't add any layer of mapping values between Zephyr and the HAL.

Note: Devicetree can’t be influenced by HALs.

All right, I've addressed this issue.

gmarull · 2023-07-12T13:14:11Z

dts/bindings/pinctrl/quicklogic,eos-s3-pinctrl.yaml

+        pinmux = <QUICKLOGIC_EOS_S3_PINMUX_UART_RX_PAD QUICKLOGIC_EOS_S3_PINMUX_UART_RX_PAD_CFG
+                  QUICKLOGIC_EOS_S3_PINMUX_FUNC_SEL_UART_RX>;


I mean, can't this all be encoded in 32-bit + dt props?

By looking at e.g. the RX case we have:

#define QUICKLOGIC_EOS_S3_PINMUX_UART_RX_PAD 45 #define QUICKLOGIC_EOS_S3_PINMUX_FUNC_SEL_UART_RX 77 #define QUICKLOGIC_EOS_S3_PINMUX_UART_RX_PAD_CFG \ (EOS_S3_UART_RXD_SEL_PAD45 | EOS_S3_PAD_OEN_DISABLE \ | EOS_S3_PAD_E_4MA | EOS_S3_PAD_REN_ENABLE)

PAD 45 is duplicated, for example. And what do all flags like OEN, REN, etc mean?

gmarull · 2023-07-17T14:20:16Z

include/zephyr/dt-bindings/pinctrl/quicklogic-eos-s3-pinctrl.h

+#define QUICKLOGIC_EOS_S3_PINMUX_UART_RX_PAD		45
+
+#define UART_RX_SEL	EOS_S3_UART_RXD_SEL_PAD45
+
+#define QUICKLOGIC_EOS_S3_PINMUX_FUNC_SEL_NONE 0
+#define QUICKLOGIC_EOS_S3_PINMUX_FUNC_SEL_UART_RX	(77 << 13)
+
+#define QUICKLOGIC_EOS_S3_PINMUX_UART_RX_PAD_CFG \
+	(QUICKLOGIC_EOS_S3_PINMUX_FUNC_SEL_UART_RX \
+	 | EOS_S3_UART_RXD_SEL_PAD45 | EOS_S3_PAD_OEN_DISABLE \
+	 | EOS_S3_PAD_E_4MA | EOS_S3_PAD_REN_ENABLE)


this continues to duplicate info, PAD45 is in both places.

I've reworked the driver, this is done.

gmarull · 2023-07-17T14:24:18Z

include/zephyr/dt-bindings/pinctrl/quicklogic-eos-s3-pinctrl.h

+#define QUICKLOGIC_EOS_S3_PINMUX_USB_PAD_CFG (EOS_S3_PAD_E_4MA | EOS_S3_PAD_P_Z \
+					      | EOS_S3_PAD_OEN_NORMAL | EOS_S3_PAD_SMT_DISABLE \
+					      | EOS_S3_PAD_REN_DISABLE \
+					      | EOS_S3_PAD_SR_SLOW | EOS_S3_PAD_CTRL_SEL_FPGA)


please use standard properties, 4MA looks like a drive strength...

I've reworked the driver so that it utilizes properties from the DTS. Could you take a look at the current implementation? Thanks!

gmarull · 2023-07-21T09:49:15Z

include/zephyr/dt-bindings/pinctrl/quicklogic-eos-s3-pinctrl.h

+#define QUICKLOGIC_EOS_S3_PINMUX_UART_RX_PAD		45
+
+#define QUICKLOGIC_EOS_S3_PINMUX_FUNC_SEL_UART_RX	(77 << 13)
+
+#define QUICKLOGIC_EOS_S3_PINMUX_UART_RX_PAD_CFG	(QUICKLOGIC_EOS_S3_PINMUX_FUNC_SEL_UART_RX \
+							 | QUICKLOGIC_EOS_S3_UART_RXD_SEL_PAD45)


is selection related to pad?

why not something like #define UART_TX_PAD44 QUICKLOGIC_EOS_S3_PINMUX(44, 0x3)

*_FUNC_SEL* is related to the function and the function is UART_rxd in this case.
I've implemented the macro you suggested and reworked the driver. I think the DTS looks much simpler now. Thank you.

gmarull · 2023-07-26T07:57:58Z

drivers/pinctrl/pinctrl_eos_s3.c

+#include <zephyr/logging/log.h>
+#include <zephyr/dt-bindings/pinctrl/quicklogic-eos-s3-pinctrl.h>
+#include <soc.h>
+
+LOG_MODULE_REGISTER(pinctrl_eos_s3, CONFIG_PINCTRL_LOG_LEVEL);


logging not used

There's LOG_ERR:
https://github.com/zephyrproject-rtos/zephyr/pull/60095/files#diff-267a39e481fb4ee81f32cbd5cb0d7ec45c2cf27a440217d473d451d9dcbb6ea7R98

gmarull · 2023-07-26T07:58:07Z

drivers/pinctrl/pinctrl_eos_s3.c

+LOG_MODULE_REGISTER(pinctrl_eos_s3, CONFIG_PINCTRL_LOG_LEVEL);
+
+#define FUNCTION_REGISTER(func)	(func >> 13)
+#define PAD_FUNC_SEL_MASK	(BIT(0) | BIT(1) | BIT(2))


maybe use GENMASK?

I've modified this to use GENMASK.

gmarull · 2023-07-26T07:58:38Z

dts/bindings/pinctrl/quicklogic,eos-s3-pinctrl.yaml

+        pinmux = <QUICKLOGIC_EOS_S3_PINMUX_UART_RX_PAD QUICKLOGIC_EOS_S3_PINMUX_UART_RX_PAD_CFG>;
+        input-enable;
+      };
+      uart0_tx_default: uart0_tx_default {
+        pinmux = <QUICKLOGIC_EOS_S3_PINMUX_UART_TX_PAD QUICKLOGIC_EOS_S3_PINMUX_UART_TX_PAD_CFG>;


needs update

I've updated the bindings file.

gmarull · 2023-07-26T07:59:34Z

dts/bindings/pinctrl/quicklogic,eos-s3-pinctrl.yaml

+      enum:
+        - "slow"
+        - "fast"
+    control-selection:


nit: quicklogic,control-selection (vendor specific prop)

gmarull · 2023-07-26T07:59:50Z

dts/bindings/pinctrl/quicklogic,eos-s3-pinctrl.yaml

+        - "slow"
+        - "fast"
+    control-selection:
+      default: "a0registers"


default needs justification, per guidelines

I've added proper descriptions.

gmarull · 2023-07-26T07:59:58Z

dts/bindings/pinctrl/quicklogic,eos-s3-pinctrl.yaml

+      description: |
+        Quicklogic EOS S3 pin's configuration (pin, IO function).
+    slew-rate:
+      default: "slow"


ditto for default

This adds a new pinctrl driver for Quicklogic EOS S3 SoC Signed-off-by: Wojciech Sipak <wsipak@antmicro.com>

Pinmuxing was previously done in the board.c file. Now it is done by the pinctrl driver. Signed-off-by: Wojciech Sipak <wsipak@antmicro.com>

Pinmuxing is now done by a pinctrl driver, not by board.c, so the code used previously for pinmuxing can be removed. Fixes zephyrproject-rtos#59186. Signed-off-by: Wojciech Sipak <wsipak@antmicro.com>

gmarull

lgtm, thanks for the continued effort!

wsipak requested review from nashif, carlescufi, galak, MaureenHelm, fkokosinski, kgugala and gmarull as code owners July 6, 2023 12:51

zephyrbot added area: Devicetree Binding PR modifies or adds a Device Tree binding area: Pinctrl area: UART Universal Asynchronous Receiver-Transmitter labels Jul 6, 2023

zephyrbot requested a review from dcpleung July 6, 2023 12:52

zephyrbot assigned gmarull Jul 6, 2023

wsipak force-pushed the wsip/eos_s3_pinctrl branch from 2949b33 to 0032226 Compare July 6, 2023 13:03

gmarull reviewed Jul 12, 2023

View reviewed changes

gmarull requested changes Jul 12, 2023

View reviewed changes

wsipak force-pushed the wsip/eos_s3_pinctrl branch 4 times, most recently from f48b282 to d3363f3 Compare July 14, 2023 09:37

wsipak requested a review from gmarull July 14, 2023 14:59

gmarull requested changes Jul 17, 2023

View reviewed changes

wsipak force-pushed the wsip/eos_s3_pinctrl branch 3 times, most recently from ede1061 to b6a53a8 Compare July 20, 2023 15:22

wsipak requested a review from gmarull July 20, 2023 16:35

gmarull reviewed Jul 21, 2023

View reviewed changes

wsipak force-pushed the wsip/eos_s3_pinctrl branch 2 times, most recently from 4dc348a to 587d867 Compare July 24, 2023 15:14

wsipak requested a review from gmarull July 24, 2023 18:31

gmarull reviewed Jul 26, 2023

View reviewed changes

wsipak force-pushed the wsip/eos_s3_pinctrl branch 2 times, most recently from 1fe4949 to b103e74 Compare July 26, 2023 11:18

wsipak added 4 commits July 26, 2023 13:40

drivers: pinctrl: add driver for EOS S3

8e10b42

This adds a new pinctrl driver for Quicklogic EOS S3 SoC Signed-off-by: Wojciech Sipak <wsipak@antmicro.com>

boards: quick_feather: use pinctrl driver

8f67d14

Pinmuxing was previously done in the board.c file. Now it is done by the pinctrl driver. Signed-off-by: Wojciech Sipak <wsipak@antmicro.com>

boards: qomu: use pinctrl driver

aeaf851

Pinmuxing was previously done in the board.c file. Now it is done by the pinctrl driver. Signed-off-by: Wojciech Sipak <wsipak@antmicro.com>

soc: quicklogic_eos_s3: remove unneeded code

6f775f6

Pinmuxing is now done by a pinctrl driver, not by board.c, so the code used previously for pinmuxing can be removed. Fixes zephyrproject-rtos#59186. Signed-off-by: Wojciech Sipak <wsipak@antmicro.com>

wsipak force-pushed the wsip/eos_s3_pinctrl branch from b103e74 to 6f775f6 Compare July 26, 2023 11:41

wsipak requested a review from gmarull July 26, 2023 12:17

gmarull approved these changes Jul 26, 2023

View reviewed changes

carlescufi approved these changes Jul 26, 2023

View reviewed changes

carlescufi merged commit 69d0f03 into zephyrproject-rtos:main Jul 26, 2023
18 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add pinctrl driver for Quicklogic EOS S3 #60095

Add pinctrl driver for Quicklogic EOS S3 #60095

wsipak commented Jul 6, 2023

gmarull Jul 12, 2023

wsipak Jul 12, 2023

gmarull Jul 12, 2023

wsipak Jul 12, 2023

gmarull Jul 12, 2023

wsipak Jul 14, 2023

gmarull Jul 12, 2023

gmarull Jul 17, 2023

wsipak Jul 20, 2023

gmarull Jul 17, 2023

wsipak Jul 20, 2023

gmarull Jul 21, 2023

wsipak Jul 24, 2023

gmarull Jul 26, 2023

wsipak Jul 26, 2023

gmarull Jul 26, 2023

wsipak Jul 26, 2023

gmarull Jul 26, 2023

wsipak Jul 26, 2023

gmarull Jul 26, 2023

wsipak Jul 26, 2023

gmarull Jul 26, 2023

wsipak Jul 26, 2023

gmarull Jul 26, 2023

wsipak Jul 26, 2023

gmarull left a comment

		pinmux = <QUICKLOGIC_EOS_S3_PINMUX_UART_RX_PAD QUICKLOGIC_EOS_S3_PINMUX_UART_RX_PAD_CFG
		QUICKLOGIC_EOS_S3_PINMUX_FUNC_SEL_UART_RX>;

Add pinctrl driver for Quicklogic EOS S3 #60095

Add pinctrl driver for Quicklogic EOS S3 #60095

Conversation

wsipak commented Jul 6, 2023

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

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

gmarull left a comment

Choose a reason for hiding this comment