Reset Controller Driver #43073

andrei-edward-popa · 2022-02-22T14:56:51Z

This is my initial work and approach for a generic reset controller driver that can be used in Zephyr. I am open to any ideas for improvement of this driver, but this was my first thought.

drivers/reset/reset.c

carlocaione · 2022-02-23T13:24:55Z

two more points:

Do you have any hardware using a reset controller? It would be great to have at least a sample or test or hardware using this API
Please use the device_mmio framework (see https://github.com/zephyrproject-rtos/zephyr/blob/main/include/sys/device_mmio.h and https://github.com/zephyrproject-rtos/zephyr/blob/main/drivers/syscon/syscon.c as example)

drivers/serial/uart_rpi_pico.c

include/drivers/reset.h

drivers/reset/Kconfig

drivers/serial/uart_rpi_pico.c

include/devicetree/reset.h

include/drivers/reset.h

dts/bindings/reset/reset-controller.yaml

carlocaione · 2022-02-24T13:15:35Z

include/drivers/reset.h

+struct reset_dev_config {
+	/** Reset controller device. */
+	const struct device *dev;
+	/** Reset line. */
+	uint32_t id;
+};
+
+/**
+ * @brief Helper macro to initialize reset controller device config.
+ *
+ * @param inst Instance number.
+ * @param id Reset line.
+ */
+#define RESET_DEV_CONFIG_INIT(inst, id)						\
+	{									\
+		.dev = DEVICE_DT_GET(DT_INST_RESET_CTLR(inst)),			\
+		.id = DT_INST_RESET_CELL(inst, id),				\
+	}


I'm a bit puzzled about this. Who is going to use the struct reset_dev_config and how? Take for example

zephyr/include/drivers/mbox.h

Lines 92 to 98 in 8a8bc39

struct mbox_channel {

/** MBOX device pointer. */

const struct device *dev;

/** Channel ID. */

uint32_t id;

};

In this case we have added some helper functions (see the mbox_* functions) that leverage that struct. But in your case how you are supposed to use this struct and why is it there if there is no user?

I tried to create this struct for getting the reset controller device and its line into some device driver (you can see an example of this in drivers/serial/uart_rpi_pico.c) and if you want to reset the device you can say something like this:
reset_reset(reset->dev, reset->line);
In the line below, 'reset' is a pointer to a reset_dev_config.
I'm not sure how to do it in another way, but if you have any idea I will gladly receive it.

if you want to reset the device you can say something like this:
reset_reset(reset->dev, reset->line);

yes, but that makes sense if you have a wrapper at API level that can unpack that for you, for example reset((struct reset_dev_config *) conf). In your case that wrapper is not being defined at API level so this struct is simply unused (I mean, the same struct can be created by the driver if it is going to be used right?)

Okay, yes, I think I understand it, I will try to create some wrappers.

@carlocaione I have already a syscall called reset_reset as you saw, and this takes a dev pointer and an id and now I want to create another syscall that takes a reset_dev_config. How can I call it (I'm talking about its name)?

no, do not create more syscalls. All your syscalls are already taking a dev pointer and an id, why don't you simply change the syscalls you have already to take a struct reset_dev_config instead?

Okay, that's cool.

you should be using device * and not this struct. The reason is that struct device is a kernel object and because of this you can grant or revoke access per thread. You can also go additional validations like if the type of the device is right ...

@ceolin What about the MBOX device? It is using a structure like I did. Maybe @carlocaione or @gmarull can clarify this? I'm not sure how to do it in another way and some suggestions would be useful.

@ceolin you can still do that, see https://github.com/zephyrproject-rtos/zephyr/blob/main/drivers/mbox/mbox_handlers.c

tests/lib/devicetree/api/src/main.c

drivers/reset/Kconfig

carlescufi · 2022-03-22T11:09:28Z

@mbolivar-nordic can you please take a look?

carlescufi · 2022-03-22T12:58:18Z

include/drivers/reset.h

+ *
+ * @param dev Reset controller device.
+ * @param id Reset line.
+ * @param status Where to write the reset status.


@andrei-edward-popa what is the format of this "status"? I see there's no enum or bool, just a uint8_t, so it'd be good to document what 'status' means in this context? At least a comment in the PR.

Status is the current status of the reset, 0 if reset is deasserted and 1 if reset is asserted. This is only 1 bit of the status, the unused 7 bits can be used for other information about the reset or not used at all. Let me know if you think that we need to use bool instead of uint8_t.

No, this sounds good. Thanks for the clarification.

added API and syscalls for reset controller added reset controller devicetree macro public API header file Signed-off-by: Andrei-Edward Popa <[email protected]>

added bindings and compatible for reset controller nodes added bindings for devices that use the reset controller Signed-off-by: Andrei-Edward Popa <[email protected]>

added bindings for vnd,reset used for devicetree test reset added adc temp sensor as supporting reset Signed-off-by: Andrei-Edward Popa <[email protected]>

added test reset for reset public API macros Signed-off-by: Andrei-Edward Popa <[email protected]>

added needed files for reset controller driver Signed-off-by: Andrei-Edward Popa <[email protected]>

added documentation for Reset Controller Driver Signed-off-by: Andrei-Edward Popa <[email protected]>

added reset bindings for Raspberry Pi Pico board Signed-off-by: Andrei-Edward Popa <[email protected]>

added Reset Controller driver for Raspberry Pi Pico board Signed-off-by: Andrei-Edward Popa <[email protected]>

added reset controller bindings for Raspberry Pi Pico board Signed-off-by: Andrei-Edward Popa <[email protected]>

added reset controller node in dtsi added defines for all reset pins Signed-off-by: Andrei-Edward Popa <[email protected]>

added resets for Raspberry Pi Pico UART instances enabled reset controller in defconfig Signed-off-by: Andrei-Edward Popa <[email protected]>

added reset controller for Raspberry Pi Pico UART devices Signed-off-by: Andrei-Edward Popa <[email protected]>

mbolivar-nordic · 2022-03-22T21:19:42Z

Approved and rebased one more time to try to get green CI.

andrei-edward-popa requested review from nashif, carlescufi, galak and MaureenHelm as code owners February 22, 2022 14:56

github-actions bot added area: API Changes to public APIs area: Devicetree area: Devicetree Binding PR modifies or adds a Device Tree binding labels Feb 22, 2022

andrei-edward-popa force-pushed the reset_controller branch from 407953c to c075363 Compare February 22, 2022 15:04

carlocaione self-requested a review February 22, 2022 15:09

henrikbrixandersen self-requested a review February 22, 2022 20:03

carlescufi reviewed Feb 23, 2022

View reviewed changes

drivers/reset/reset.c Outdated Show resolved Hide resolved

carlocaione requested changes Feb 23, 2022

View reviewed changes

drivers/reset/reset.c Outdated Show resolved Hide resolved

drivers/reset/reset.c Outdated Show resolved Hide resolved

drivers/reset/reset.c Outdated Show resolved Hide resolved

drivers/reset/reset.c Outdated Show resolved Hide resolved

andrei-edward-popa force-pushed the reset_controller branch from c075363 to 32e70ba Compare February 23, 2022 15:38

github-actions bot added the area: Boards label Feb 23, 2022

andrei-edward-popa requested a review from carlocaione February 23, 2022 15:39

erwango reviewed Feb 23, 2022

View reviewed changes

drivers/serial/uart_rpi_pico.c Outdated Show resolved Hide resolved

carlocaione requested changes Feb 23, 2022

View reviewed changes

include/drivers/reset.h Outdated Show resolved Hide resolved

include/drivers/reset.h Outdated Show resolved Hide resolved

drivers/reset/Kconfig Outdated Show resolved Hide resolved

drivers/serial/uart_rpi_pico.c Outdated Show resolved Hide resolved

andrei-edward-popa force-pushed the reset_controller branch 2 times, most recently from 49eafe5 to e92b42d Compare February 23, 2022 23:07

andrei-edward-popa requested a review from carlocaione February 23, 2022 23:07

carlocaione requested changes Feb 24, 2022

View reviewed changes

andrei-edward-popa force-pushed the reset_controller branch from e92b42d to ff4e278 Compare February 24, 2022 11:51

github-actions bot added the area: Tests Issues related to a particular existing or missing test label Feb 24, 2022

andrei-edward-popa force-pushed the reset_controller branch from ff4e278 to b98817e Compare February 24, 2022 11:52

andrei-edward-popa requested a review from carlocaione February 24, 2022 11:53

andrei-edward-popa force-pushed the reset_controller branch 2 times, most recently from ea2fd9e to 6486fd3 Compare February 24, 2022 13:19

carlocaione requested changes Feb 24, 2022

View reviewed changes

andrei-edward-popa force-pushed the reset_controller branch from 6486fd3 to d53b5c5 Compare February 24, 2022 14:09

carlescufi requested review from erwango and ceolin March 22, 2022 11:09

carlescufi reviewed Mar 22, 2022

View reviewed changes

mbolivar-nordic previously approved these changes Mar 22, 2022

View reviewed changes

andrei-edward-popa added 12 commits March 22, 2022 14:19

include: drivers: added header file for Reset Controller Driver

5a97251

added API and syscalls for reset controller added reset controller devicetree macro public API header file Signed-off-by: Andrei-Edward Popa <[email protected]>

dts: bindings: added bindings for Reset Controller Driver

1b455d1

added bindings and compatible for reset controller nodes added bindings for devices that use the reset controller Signed-off-by: Andrei-Edward Popa <[email protected]>

dts: bindings: test: added bindings for devicetree test reset

fc7c0f6

added bindings for vnd,reset used for devicetree test reset added adc temp sensor as supporting reset Signed-off-by: Andrei-Edward Popa <[email protected]>

tests: lib: devicetree: api: added test for reset public API

cdbe341

added test reset for reset public API macros Signed-off-by: Andrei-Edward Popa <[email protected]>

drivers: added file structure for Reset Controller Driver

d9aeb65

added needed files for reset controller driver Signed-off-by: Andrei-Edward Popa <[email protected]>

doc: reference: added documentation for Reset Controller

1c53cb5

added documentation for Reset Controller Driver Signed-off-by: Andrei-Edward Popa <[email protected]>

dts: bindings: reset: added bindings for Raspberry Pi Pico

3156a9e

added reset bindings for Raspberry Pi Pico board Signed-off-by: Andrei-Edward Popa <[email protected]>

drivers: reset: added Reset Controller for Raspberry Pi Pico

ce45f7c

added Reset Controller driver for Raspberry Pi Pico board Signed-off-by: Andrei-Edward Popa <[email protected]>

dts: bindings: serial: added reset bindings for Raspberry Pi Pico

507a1cf

added reset controller bindings for Raspberry Pi Pico board Signed-off-by: Andrei-Edward Popa <[email protected]>

dts: arm: rpi_pico: added reset controller node

dd5e809

added reset controller node in dtsi added defines for all reset pins Signed-off-by: Andrei-Edward Popa <[email protected]>

dts: arm: rpi_pico: added resets for UART instances

8bf33dc

added resets for Raspberry Pi Pico UART instances enabled reset controller in defconfig Signed-off-by: Andrei-Edward Popa <[email protected]>

drivers: serial: rpi_pico: added reset support for UART devices

799bfee

added reset controller for Raspberry Pi Pico UART devices Signed-off-by: Andrei-Edward Popa <[email protected]>

mbolivar-nordic force-pushed the reset_controller branch from 7eeb57f to 799bfee Compare March 22, 2022 21:19

nashif dismissed stale reviews from mbolivar-nordic, gmarull, and carlocaione via 799bfee March 22, 2022 22:00

carlocaione approved these changes Mar 23, 2022

View reviewed changes

gmarull approved these changes Mar 23, 2022

View reviewed changes

carlescufi approved these changes Mar 23, 2022

View reviewed changes

carlescufi merged commit 12fa630 into zephyrproject-rtos:main Mar 23, 2022

andrei-edward-popa deleted the reset_controller branch March 23, 2022 12:19

mbolivar-nordic mentioned this pull request May 25, 2022

add support to store devicetree phandle+specifier as "foo_dt_spec" structures #23691

Open

soburi added the platform: Raspberry Pi Pico Raspberry Pi Pico (RPi Pico) label Jan 15, 2023

soburi mentioned this pull request Jan 16, 2023

TaskList for Supporting RaspberryPi Pico peripherals #53810

Open

52 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Reset Controller Driver #43073

Reset Controller Driver #43073

andrei-edward-popa commented Feb 22, 2022

carlocaione commented Feb 23, 2022

carlocaione Feb 24, 2022

andrei-edward-popa Feb 24, 2022

carlocaione Feb 24, 2022

andrei-edward-popa Feb 24, 2022

andrei-edward-popa Feb 24, 2022

carlocaione Feb 24, 2022

andrei-edward-popa Feb 24, 2022

ceolin Feb 25, 2022

andrei-edward-popa Feb 25, 2022

carlocaione Feb 25, 2022

carlescufi commented Mar 22, 2022

carlescufi Mar 22, 2022

andrei-edward-popa Mar 22, 2022

carlescufi Mar 22, 2022

mbolivar-nordic commented Mar 22, 2022

	struct mbox_channel {
	/** MBOX device pointer. */
	const struct device *dev;

	/** Channel ID. */
	uint32_t id;
	};

Reset Controller Driver #43073

Reset Controller Driver #43073

Conversation

andrei-edward-popa commented Feb 22, 2022

carlocaione commented Feb 23, 2022

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

carlescufi commented Mar 22, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mbolivar-nordic commented Mar 22, 2022