drivers: gpio: miwu: Add miwu and gpio drivers in npcx7 series. #27578

MulinChao · 2020-08-14T09:32:16Z

Add Multi-Input Wake-Up Unit (MIWU) and GPIO driver implementations in Nuvoton npcx7 series..

It is separated into three commits. First, commit introduces MIWU which provides signal conditioning such as 'Level' and 'Edge' trigger type and grouping of NVIC's external interrupt sources. It's crucial since many devices such as gpio, timer, eSPI VW signals relies on it for interrupt handling. Beside gpio driver implementation, the other commits also demonstrate how we handle wake-up signals of gpio and uart by MIWU api functions.

MulinChao · 2020-08-17T12:01:02Z

From the build error report, it shows "gen_isr_tables.py: error: multiple registrations at table_index 61 for irq 61 (0x3d)". Dig the source of gen_isr_table, the root cause is that the test suit also uses IRQ61 (https://github.com/zephyrproject-rtos/zephyr/blob/master/tests/kernel/gen_isr_table/src/main.c#L283) which is the same as miwu2 group2 interrupt.
Hi @ioannisg
Might I know what is the purpose of the test suite? Is the workaround moving TEST_NUM_IRQS to unused IRQ? Thanks.

drivers/interrupt_controller/intc_miwu.c

dts/arm/nuvoton/npcx7m6fb.dtsi

soc/arm/nuvoton_npcx/common/reg/reg_def.h

soc/arm/nuvoton_npcx/common/soc_gpio.h

drivers/serial/uart_npcx.c

tests/kernel/gen_isr_table/src/main.c

MulinChao · 2020-08-24T10:24:04Z

Hi @sjg20 :
I left two comments for further discussion and resolved the others in this PR. Putting a comment at the top of miwu driver is a good idea since it's not a popular hardware device. Thanks for reviewing it.

pabigot

Looks interesting, but there's no public information I can find on the board or its MCU, or where to buy the board.

jettr

Sorry, I didn't have a change to finish reviewing. I just a few minor comments. Looks good to me so far though

drivers/gpio/gpio_npcx.c

MulinChao · 2020-08-26T02:29:28Z

Looks interesting, but there's no public information I can find on the board or its MCU, or where to buy the board.

Hi @pabigot:
Thanks for your comments in this PR. Regarding the npcx board, our PM will contact you privately.

Hi @michalsch:
Could you help with this? Thanks.

pabigot · 2020-08-26T10:55:13Z

Looks interesting, but there's no public information I can find on the board or its MCU, or where to buy the board.

Hi @pabigot:
Thanks for your comments in this PR. Regarding the npcx board, our PM will contact you privately.

Thanks. But my main point is an ongoing concern with adopting into mainline Zephyr hardware (boards, SoCs, shields) that are not generally available. This introduces a near-permanent maintenance burden as tree-wide refactoring often requires their code be changed, but the people making those changes can't easily verify functionality. I really hate changing something when I can't verify I didn't break it; so I have 26 Zephyr target boards spanning almost all SoC families. More than half of those are not of interest to the people who fund most of my Zephyr work.

When the documentation is also not available it becomes a bigger problem.

Note that this is my personal view, which others may or may not share. There is no documented policy in Zephyr providing non-technical criteria for adding new content. Since this target has already been accepted clearly the new peripheral implementations must go in. But the level of review I can give is seriously impeded by lack of technical documentation and the ability to test.

jettr · 2020-08-26T16:53:42Z

Thanks. But my main point is an ongoing concern with adopting into mainline Zephyr hardware (boards, SoCs, shields) that are not generally available. This introduces a near-permanent maintenance burden as tree-wide refactoring often requires their code be changed, but the people making those changes can't easily verify functionality. I really hate changing something when I can't verify I didn't break it; so I have 26 Zephyr target boards spanning almost all SoC families. More than half of those are not of interest to the people who fund most of my Zephyr work.

When the documentation is also not available it becomes a bigger problem.

Note that this is my personal view, which others may or may not share. There is no documented policy in Zephyr providing non-technical criteria for adding new content. Since this target has already been accepted clearly the new peripheral implementations must go in. But the level of review I can give is seriously impeded by lack of technical documentation and the ability to test.

I think your concern about testing wide-scale changes/refactors is valid. I know I have heard something about a hardware test matrix that we can run tests on that is distributed out to different chip vendors; would adding more testing for the npcx product line address some of your concerns here? @nashif for more details on test matix.

MulinChao · 2020-08-27T02:18:25Z

@pabigot and @jett:
Regarding test suites, boards, and documentation during refactoring, I fully understand what you concern. Each time I did changes for comments from code review, passing the related driver test suite is necessary before I submitting them. I think it's OK to provide npcx7m6fb evaluation boards.
Regarding the document or the datasheet of Nuvoton EC, I need further discussion with our PM. (Basically, they are provided under NDA.)

nashif · 2020-08-28T14:30:21Z

@jettr @pabigot

Thanks. But my main point is an ongoing concern with adopting into mainline Zephyr hardware (boards, SoCs, shields) that are not generally available.

This was already discussed in multiple occasions and the consensus was that support for unreleased hardware and missing documentation of the hardware is acceptable as long ad the drivers, SoCs and boards are being actively maintained with someone available to answer any technical questions. If this changes over time and some hardware becomes unmaintained and it prevents us from moving forward because of lack of response or broken support, we will then reserve the right to disable or remove such support.

Supporting hardware that is still unreleased is often related to product development and is an indicator that the project is doing well in general :) Linux is full of drivers and hardware support that is still not released, there is no way we can make progress only with release hardware (that is maybe how we started 5 years ago, but we are way beyond that).

jettr

The remaining comments are relatively minor. This PR looks good to me overall! Can you make a sweep through all of the remaining open comments and address them, please?

drivers/gpio/gpio_npcx.c

jettr · 2020-08-28T18:37:32Z

drivers/interrupt_controller/intc_miwu.c

+	callback->source = source;
+}
+
+int soc_miwu_manage_gpio_callback(struct miwu_io_callback *cb, bool set)


Why can't we use gpio_manage_callback in gpio_utils.h here instead

First, the parameter is different between soc_miwu_manage_gpio_callback() and gpio_manage_callback() although 'struct miwu_io_callback *cb' and struct gpio_callback *callback share the same memory space. In this PR, it won't increase code size since we don't call any gpio_manage_callback() and replace it with soc_miwu_manage_gpio_callback().

Of course, we can cast 'struct miwu_io_callback *' back to 'struct gpio_callback * and call gpio_manage_callback() in MIWU driver directly. But I thought the current one is more straightforward.

drivers/interrupt_controller/intc_miwu.c

soc/arm/nuvoton_npcx/common/reg/reg_def.h

jettr · 2020-08-28T18:52:02Z

drivers/gpio/gpio_npcx.c

+
+/* Driver config */
+struct gpio_npcx_config {
+	/* gpio_driver_config needs to be first */


+1; there do seem to still be /* instead of /** here

MulinChao · 2020-08-31T10:12:03Z

Can you make a sweep through all of the remaining open comments and address them, please?

@jettr I have swept all remaining comments. Most of them are resolved and related changes were updated. But a few comments are still open and I gave replies for further discussions. Please check them, thanks.

jettr

Looks good!

soc/arm/nuvoton_npcx/common/reg/reg_def.h

drivers/gpio/gpio_npcx.c

drivers/interrupt_controller/intc_miwu.c

The device Multi-Input Wake-Up Unit (MIWU) supports the embedded controller (EC) to exit 'Sleep' or 'Deep Sleep' power state which allows chip has better power consumption. Also, it provides signal conditioning such as 'Level' and 'Edge' trigger type and grouping of external interrupt sources of NVIC. The NPCX series has three identical MIWU modules: MIWU0, MIWU1, MIWU2. Together, they support a total of over 140 internal and/or external wake-up sources. In this CL, we use device tree files to present the relationship bewteen MIWU and the other devices in different npcx series. For npcx7 series, it include: 1. npcx7-miwus-int-map.dtsi: it presents relationship between MIWU group and NVIC interrupt in npcx7. Please notice it isn't 1-to-1 mapping. 2. npcx7-miwus-wui-map.dtsi: it presents relationship between input of MIWU and its source device such as gpio, timer, eSPI VWs and so on. This CL also includes: 1. Add MIWU device tree declarations. 2. MIWU api function declarations and implementation to configure signal conditions and callback function mechanism. They can be be classified into two types. One is for GPIO which connects original gpio callback implemetation and the other is for generic devices such as timer, eSPI, and so on. Signed-off-by: Mulin Chao <[email protected]>

Add gpio support for Nuvoton NPCX series. This CL includes: 1. Add GPIO device tree declarations. 2. Introduce wui_maps property in yaml file to present relationship between Wake-Up Input (WUI) and 8 IOs belong to the device. 3. Zephyr GPIO api implementation. 4. GPIO callback functions implementation with MIWU api functions. 5. Overlay file for gpio basic tests Signed-off-by: Mulin Chao <[email protected]>

This CL configures the UART wake-up event triggered from a falling edge (START condition) on CR_SIN pin. It also includes: 1. Introduce wui_maps property in yaml file to present relationship between Wake-Up Input (WUI) and UART device. 2. Implement wake-up mechanism by MIWU api functions. Signed-off-by: Mulin Chao <[email protected]>

In NPCX7M6FB, it uses some the IRQs at the end of the vector table, for example, the irq 60 and 61 used for Multi-Input Wake-Up Unit (MIWU) device by default, and conflicts with isr used for testing. Moving IRQs for this test suite to solve the issue. Signed-off-by: Mulin Chao <[email protected]>

Remove the '_t' suffix of device register structure since it is used mainly with typedefs. Signed-off-by: Mulin Chao <[email protected]>

Replace npcx register base address type, uint32_t, with uintptr_t. It is easier to know what type of base address and for linear addresses treated as integral values. This CL also modified IS_BIT_SET() macro function to fit MISRA code guidelines. Signed-off-by: Mulin Chao <[email protected]>

Michal-Sch · 2020-09-06T09:04:14Z

@jettr @pabigot

Thanks. But my main point is an ongoing concern with adopting into mainline Zephyr hardware (boards, SoCs, shields) that are not generally available.

This was already discussed in multiple occasions and the consensus was that support for unreleased hardware and missing documentation of the hardware is acceptable as long ad the drivers, SoCs and boards are being actively maintained with someone available to answer any technical questions. If this changes over time and some hardware becomes unmaintained and it prevents us from moving forward because of lack of response or broken support, we will then reserve the right to disable or remove such support.

Supporting hardware that is still unreleased is often related to product development and is an indicator that the project is doing well in general :) Linux is full of drivers and hardware support that is still not released, there is no way we can make progress only with release hardware (that is maybe how we started 5 years ago, but we are way beyond that).

Sorry for the delay. Was OOO and had to catch-up.
Datasheet can be provided under NDA as ML mentioned.
We can provide boards but I assume they are meaningless without the datasheet.

MulinChao requested review from andrewboie, galak, MaureenHelm, mnkp and pabigot as code owners August 14, 2020 09:32

MulinChao requested review from sjg20 and jettr August 14, 2020 09:34

github-actions bot added area: Boards area: Devicetree area: Devicetree Binding PR modifies or adds a Device Tree binding area: Tests Issues related to a particular existing or missing test labels Aug 14, 2020

MulinChao force-pushed the driver_npcx_miwu_gpio branch 4 times, most recently from 56dcb8d to f002b96 Compare August 17, 2020 05:23

MulinChao requested review from andyross and nashif as code owners August 18, 2020 02:00

github-actions bot added the area: Kernel label Aug 18, 2020

MulinChao force-pushed the driver_npcx_miwu_gpio branch from 0cfe454 to b62ff1f Compare August 19, 2020 04:06

MulinChao added this to the v2.4.0 milestone Aug 19, 2020

MulinChao force-pushed the driver_npcx_miwu_gpio branch from b62ff1f to 98495c2 Compare August 21, 2020 09:34

sjg20 requested changes Aug 21, 2020

View reviewed changes

MulinChao force-pushed the driver_npcx_miwu_gpio branch 5 times, most recently from d1e65b2 to 5daa9f2 Compare August 24, 2020 09:33

pabigot reviewed Aug 25, 2020

View reviewed changes

jettr reviewed Aug 25, 2020

View reviewed changes

drivers/gpio/gpio_npcx.c Outdated Show resolved Hide resolved

drivers/gpio/gpio_npcx.c Outdated Show resolved Hide resolved

drivers/gpio/gpio_npcx.c Outdated Show resolved Hide resolved

MulinChao force-pushed the driver_npcx_miwu_gpio branch 2 times, most recently from 0f21a1b to 5073b17 Compare August 26, 2020 05:45

andrewboie approved these changes Aug 26, 2020

View reviewed changes

MulinChao force-pushed the driver_npcx_miwu_gpio branch from 5073b17 to c158bf3 Compare August 27, 2020 01:26

duda-patryk approved these changes Aug 27, 2020

View reviewed changes

jettr reviewed Aug 28, 2020

View reviewed changes

MulinChao force-pushed the driver_npcx_miwu_gpio branch from c158bf3 to c627610 Compare August 31, 2020 09:44

jettr approved these changes Aug 31, 2020

View reviewed changes

soc/arm/nuvoton_npcx/common/reg/reg_def.h Outdated Show resolved Hide resolved

drivers/gpio/gpio_npcx.c Outdated Show resolved Hide resolved

drivers/interrupt_controller/intc_miwu.c Show resolved Hide resolved

MulinChao added 5 commits September 1, 2020 09:03

drivers: npcx: remove suffix '_t' of device register structure.

224eeee

Remove the '_t' suffix of device register structure since it is used mainly with typedefs. Signed-off-by: Mulin Chao <[email protected]>

MulinChao force-pushed the driver_npcx_miwu_gpio branch 2 times, most recently from fe6a228 to 8f5aa76 Compare September 1, 2020 01:49

MulinChao force-pushed the driver_npcx_miwu_gpio branch from 8f5aa76 to a9e6a8f Compare September 1, 2020 01:54

carlescufi merged commit b3188f1 into zephyrproject-rtos:master Sep 1, 2020

MulinChao mentioned this pull request Sep 11, 2020

drivers: pwm: add pwm driver support in NPCX7 series #28044

Merged

MulinChao mentioned this pull request Oct 15, 2020

Port i2c driver for NPCX7 series. #29144

Merged

MulinChao mentioned this pull request Feb 25, 2021

driver: hwinfo: npcx: Add hwinfo driver support in npcx family. #32602

Closed

MulinChao mentioned this pull request Jun 15, 2021

gpio: Add GPIO voltage overrides #36179

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

drivers: gpio: miwu: Add miwu and gpio drivers in npcx7 series. #27578

drivers: gpio: miwu: Add miwu and gpio drivers in npcx7 series. #27578

MulinChao commented Aug 14, 2020

MulinChao commented Aug 17, 2020

MulinChao commented Aug 24, 2020

pabigot left a comment

jettr left a comment

MulinChao commented Aug 26, 2020

pabigot commented Aug 26, 2020

jettr commented Aug 26, 2020

MulinChao commented Aug 27, 2020

nashif commented Aug 28, 2020

jettr left a comment

jettr Aug 28, 2020

MulinChao Aug 31, 2020

jettr Aug 28, 2020 •

edited

Loading

MulinChao commented Aug 31, 2020

jettr left a comment

Michal-Sch commented Sep 6, 2020

drivers: gpio: miwu: Add miwu and gpio drivers in npcx7 series. #27578

drivers: gpio: miwu: Add miwu and gpio drivers in npcx7 series. #27578

Conversation

MulinChao commented Aug 14, 2020

MulinChao commented Aug 17, 2020

MulinChao commented Aug 24, 2020

pabigot left a comment

Choose a reason for hiding this comment

jettr left a comment

Choose a reason for hiding this comment

MulinChao commented Aug 26, 2020

pabigot commented Aug 26, 2020

jettr commented Aug 26, 2020

MulinChao commented Aug 27, 2020

nashif commented Aug 28, 2020

jettr left a comment

Choose a reason for hiding this comment

jettr Aug 28, 2020

Choose a reason for hiding this comment

MulinChao Aug 31, 2020

Choose a reason for hiding this comment

jettr Aug 28, 2020 • edited Loading

Choose a reason for hiding this comment

MulinChao commented Aug 31, 2020

jettr left a comment

Choose a reason for hiding this comment

Michal-Sch commented Sep 6, 2020

jettr Aug 28, 2020 •

edited

Loading