From: Quentin Schulz <quentin.schulz@cherry.de>
The RK806 PMIC allows to configure its reset/restart behavior whenever
the PMIC is reset either programmatically or via some external pins
(e.g. PWRCTRL or RESETB).
The following modes exist:
- 0 (RK806_RESTART) restart PMU,
- 1 (RK806_RESET) reset all power off reset registers and force
state to switch to ACTIVE mode,
- 2 (RK806_RESET_NOTIFY) same as RK806_RESET and also pull
RESETB pin down for 5ms,
For example, some hardware may require a full restart
(RK806_RESTART mode) in order to function properly as regulators
are shortly interrupted in this mode.
This is the case for RK3588 Jaguar and RK3588 Tiger which have a
companion microcontroller running on an independent power supply and
monitoring the PMIC power rail to know the state of the main system.
When it detects a restart, it resets its own IPs exposed to the main
system as if to simulate its own reset. Failing to perform this fake
reset of the microcontroller may break things (e.g. watchdog not
automatically disabled, buzzer still running until manually disabled,
leftover configuration from previous main system state, etc...).
Some other systems may be depending on the power rails to not be
interrupted even for a small amount of time[1].
This allows to specify how the PMIC should perform on the hardware level
and may differ between harwdare designs, so a DT property seems
warranted. I unfortunately do not see how this could be made generic
enough to make it a non-vendor property.
[1] https://lore.kernel.org/linux-rockchip/2577051.irdbgypaU6@workhorse/
Signed-off-by: Quentin Schulz <quentin.schulz@cherry.de>
---
.../devicetree/bindings/mfd/rockchip,rk806.yaml | 23 ++++++++++++++++++++++
include/dt-bindings/mfd/rockchip,rk8xx.h | 17 ++++++++++++++++
2 files changed, 40 insertions(+)
diff --git a/Documentation/devicetree/bindings/mfd/rockchip,rk806.yaml b/Documentation/devicetree/bindings/mfd/rockchip,rk806.yaml
index 3c2b06629b75ea94f90712470bf14ed7fc16d68d..c555b5956cea9f594d80ebd3b27e8489e520d97d 100644
--- a/Documentation/devicetree/bindings/mfd/rockchip,rk806.yaml
+++ b/Documentation/devicetree/bindings/mfd/rockchip,rk806.yaml
@@ -31,6 +31,29 @@ properties:
system-power-controller: true
+ rockchip,reset-mode:
+ $ref: /schemas/types.yaml#/definitions/uint32
+ enum: [0, 1, 2]
+ description:
+ Mode to use when a reset of the PMIC is triggered.
+
+ The reset can be triggered either programmatically, via one of
+ the PWRCTRL pins (provided additional configuration) or
+ asserting RESETB pin low.
+
+ The following modes are supported (see also
+ include/dt-bindings/mfd/rockchip,rk8xx.h)
+
+ - 0 (RK806_RESTART) restart PMU,
+ - 1 (RK806_RESET) reset all power off reset registers and force
+ state to switch to ACTIVE mode,
+ - 2 (RK806_RESET_NOTIFY) same as RK806_RESET and also pull
+ RESETB pin down for 5ms,
+
+ For example, some hardware may require a full restart
+ (RK806_RESTART mode) in order to function properly as regulators
+ are shortly interrupted in this mode.
+
vcc1-supply:
description:
The input supply for dcdc-reg1.
diff --git a/include/dt-bindings/mfd/rockchip,rk8xx.h b/include/dt-bindings/mfd/rockchip,rk8xx.h
new file mode 100644
index 0000000000000000000000000000000000000000..f058ed1ca661185f79738a358aa2d4f04539c590
--- /dev/null
+++ b/include/dt-bindings/mfd/rockchip,rk8xx.h
@@ -0,0 +1,17 @@
+/* SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause */
+/*
+ * Device Tree defines for Rockchip RK8xx PMICs
+ *
+ * Copyright 2025 Cherry Embedded Solutions GmbH
+ *
+ * Author: Quentin Schulz <quentin.schulz@cherry.de>
+ */
+
+#ifndef _DT_BINDINGS_MFD_ROCKCHIP_RK8XX_H
+#define _DT_BINDINGS_MFD_ROCKCHIP_RK8XX_H
+
+#define RK806_RESTART 0
+#define RK806_RESET 1
+#define RK806_RESET_NOTIFY 2
+
+#endif
--
2.49.0On Thu, Jun 05, 2025 at 05:41:06PM GMT, Quentin Schulz wrote: > + rockchip,reset-mode: > + $ref: /schemas/types.yaml#/definitions/uint32 > + enum: [0, 1, 2] > + description: > + Mode to use when a reset of the PMIC is triggered. > + > + The reset can be triggered either programmatically, via one of > + the PWRCTRL pins (provided additional configuration) or > + asserting RESETB pin low. > + > + The following modes are supported (see also > + include/dt-bindings/mfd/rockchip,rk8xx.h) > + > + - 0 (RK806_RESTART) restart PMU, > + - 1 (RK806_RESET) reset all power off reset registers and force > + state to switch to ACTIVE mode, > + - 2 (RK806_RESET_NOTIFY) same as RK806_RESET and also pull > + RESETB pin down for 5ms, > + > + For example, some hardware may require a full restart > + (RK806_RESTART mode) in order to function properly as regulators > + are shortly interrupted in this mode. > + This is fine, although now points to missing restart-handler schema and maybe this should be once made common property. But that's just digression, nothing needed here. > vcc1-supply: > description: > The input supply for dcdc-reg1. > diff --git a/include/dt-bindings/mfd/rockchip,rk8xx.h b/include/dt-bindings/mfd/rockchip,rk8xx.h > new file mode 100644 > index 0000000000000000000000000000000000000000..f058ed1ca661185f79738a358aa2d4f04539c590 > --- /dev/null > +++ b/include/dt-bindings/mfd/rockchip,rk8xx.h > @@ -0,0 +1,17 @@ > +/* SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause */ > +/* > + * Device Tree defines for Rockchip RK8xx PMICs > + * > + * Copyright 2025 Cherry Embedded Solutions GmbH > + * > + * Author: Quentin Schulz <quentin.schulz@cherry.de> > + */ > + > +#ifndef _DT_BINDINGS_MFD_ROCKCHIP_RK8XX_H > +#define _DT_BINDINGS_MFD_ROCKCHIP_RK8XX_H > + > +#define RK806_RESTART 0 > +#define RK806_RESET 1 > +#define RK806_RESET_NOTIFY 2 I do not see how this is a binding. Where do you use this in the driver (to be a binding because otherwise you just add unused ABI)? Best regards, Krzysztof
Hi Krzysztof, On 6/17/25 10:08 AM, Krzysztof Kozlowski wrote: > On Thu, Jun 05, 2025 at 05:41:06PM GMT, Quentin Schulz wrote: >> + rockchip,reset-mode: >> + $ref: /schemas/types.yaml#/definitions/uint32 >> + enum: [0, 1, 2] >> + description: >> + Mode to use when a reset of the PMIC is triggered. >> + >> + The reset can be triggered either programmatically, via one of >> + the PWRCTRL pins (provided additional configuration) or >> + asserting RESETB pin low. >> + >> + The following modes are supported (see also >> + include/dt-bindings/mfd/rockchip,rk8xx.h) >> + >> + - 0 (RK806_RESTART) restart PMU, >> + - 1 (RK806_RESET) reset all power off reset registers and force >> + state to switch to ACTIVE mode, >> + - 2 (RK806_RESET_NOTIFY) same as RK806_RESET and also pull >> + RESETB pin down for 5ms, >> + >> + For example, some hardware may require a full restart >> + (RK806_RESTART mode) in order to function properly as regulators >> + are shortly interrupted in this mode. >> + > > This is fine, although now points to missing restart-handler schema and > maybe this should be once made common property. But that's just > digression, nothing needed here. > >> vcc1-supply: >> description: >> The input supply for dcdc-reg1. >> diff --git a/include/dt-bindings/mfd/rockchip,rk8xx.h b/include/dt-bindings/mfd/rockchip,rk8xx.h >> new file mode 100644 >> index 0000000000000000000000000000000000000000..f058ed1ca661185f79738a358aa2d4f04539c590 >> --- /dev/null >> +++ b/include/dt-bindings/mfd/rockchip,rk8xx.h >> @@ -0,0 +1,17 @@ >> +/* SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause */ >> +/* >> + * Device Tree defines for Rockchip RK8xx PMICs >> + * >> + * Copyright 2025 Cherry Embedded Solutions GmbH >> + * >> + * Author: Quentin Schulz <quentin.schulz@cherry.de> >> + */ >> + >> +#ifndef _DT_BINDINGS_MFD_ROCKCHIP_RK8XX_H >> +#define _DT_BINDINGS_MFD_ROCKCHIP_RK8XX_H >> + >> +#define RK806_RESTART 0 >> +#define RK806_RESET 1 >> +#define RK806_RESET_NOTIFY 2 > > I do not see how this is a binding. Where do you use this in the driver > (to be a binding because otherwise you just add unused ABI)? > Explained in the commit log of the driver patch: """ This adds the appropriate logic in the driver to parse the new rockchip,reset-mode DT property to pass this information. It just happens that the values in the binding match the values to write in the bitfield so no mapping is necessary. """ I can add useless mapping in the driver if it's preferred. I had the impression that simply using a hardcoded value in the DT binding and then writing it to the register was not desired, so the constant is now here to make this less obscure from DT perspective though I'm still writing the value directly in the register. If hardcoded values are ok in the binding, then I can remove that header file. Cheers, Quentin
On 17/06/2025 11:38, Quentin Schulz wrote: > Hi Krzysztof, > > On 6/17/25 10:08 AM, Krzysztof Kozlowski wrote: >> On Thu, Jun 05, 2025 at 05:41:06PM GMT, Quentin Schulz wrote: >>> + rockchip,reset-mode: >>> + $ref: /schemas/types.yaml#/definitions/uint32 >>> + enum: [0, 1, 2] >>> + description: >>> + Mode to use when a reset of the PMIC is triggered. >>> + >>> + The reset can be triggered either programmatically, via one of >>> + the PWRCTRL pins (provided additional configuration) or >>> + asserting RESETB pin low. >>> + >>> + The following modes are supported (see also >>> + include/dt-bindings/mfd/rockchip,rk8xx.h) >>> + >>> + - 0 (RK806_RESTART) restart PMU, >>> + - 1 (RK806_RESET) reset all power off reset registers and force >>> + state to switch to ACTIVE mode, >>> + - 2 (RK806_RESET_NOTIFY) same as RK806_RESET and also pull >>> + RESETB pin down for 5ms, >>> + >>> + For example, some hardware may require a full restart >>> + (RK806_RESTART mode) in order to function properly as regulators >>> + are shortly interrupted in this mode. >>> + >> >> This is fine, although now points to missing restart-handler schema and >> maybe this should be once made common property. But that's just >> digression, nothing needed here. >> >>> vcc1-supply: >>> description: >>> The input supply for dcdc-reg1. >>> diff --git a/include/dt-bindings/mfd/rockchip,rk8xx.h b/include/dt-bindings/mfd/rockchip,rk8xx.h >>> new file mode 100644 >>> index 0000000000000000000000000000000000000000..f058ed1ca661185f79738a358aa2d4f04539c590 >>> --- /dev/null >>> +++ b/include/dt-bindings/mfd/rockchip,rk8xx.h >>> @@ -0,0 +1,17 @@ >>> +/* SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause */ >>> +/* >>> + * Device Tree defines for Rockchip RK8xx PMICs >>> + * >>> + * Copyright 2025 Cherry Embedded Solutions GmbH >>> + * >>> + * Author: Quentin Schulz <quentin.schulz@cherry.de> >>> + */ >>> + >>> +#ifndef _DT_BINDINGS_MFD_ROCKCHIP_RK8XX_H >>> +#define _DT_BINDINGS_MFD_ROCKCHIP_RK8XX_H >>> + >>> +#define RK806_RESTART 0 >>> +#define RK806_RESET 1 >>> +#define RK806_RESET_NOTIFY 2 >> >> I do not see how this is a binding. Where do you use this in the driver >> (to be a binding because otherwise you just add unused ABI)? >> > > Explained in the commit log of the driver patch: > > """ > This adds the appropriate logic in the driver to parse the new > rockchip,reset-mode DT property to pass this information. It just > happens that the values in the binding match the values to write in the > bitfield so no mapping is necessary. > """ > > I can add useless mapping in the driver if it's preferred. I had the No, I comment and raise questions when you add ABI which is neither ABI or should not be ABI. > impression that simply using a hardcoded value in the DT binding and > then writing it to the register was not desired, so the constant is now > here to make this less obscure from DT perspective though I'm still > writing the value directly in the register. If hardcoded values are ok > in the binding, then I can remove that header file. If you want something user readable, make it an enum string or keep the header within DTS. If I review it like that, it will be brought to me next time for some other patch saying that commit was reviewed so I can do the same. [1] Therefore since I object against unused binding headers in general (there is no user here technically), I need to object here as well. :( https://lore.kernel.org/all/0d381ad0-85d4-43de-a050-3b9ed03bf5d8@kernel.org/ Best regards, Krzysztof
On 6/17/25 12:21 PM, Krzysztof Kozlowski wrote: > On 17/06/2025 11:38, Quentin Schulz wrote: >> Hi Krzysztof, >> >> On 6/17/25 10:08 AM, Krzysztof Kozlowski wrote: >>> On Thu, Jun 05, 2025 at 05:41:06PM GMT, Quentin Schulz wrote: >>>> + rockchip,reset-mode: >>>> + $ref: /schemas/types.yaml#/definitions/uint32 >>>> + enum: [0, 1, 2] >>>> + description: >>>> + Mode to use when a reset of the PMIC is triggered. >>>> + >>>> + The reset can be triggered either programmatically, via one of >>>> + the PWRCTRL pins (provided additional configuration) or >>>> + asserting RESETB pin low. >>>> + >>>> + The following modes are supported (see also >>>> + include/dt-bindings/mfd/rockchip,rk8xx.h) >>>> + >>>> + - 0 (RK806_RESTART) restart PMU, >>>> + - 1 (RK806_RESET) reset all power off reset registers and force >>>> + state to switch to ACTIVE mode, >>>> + - 2 (RK806_RESET_NOTIFY) same as RK806_RESET and also pull >>>> + RESETB pin down for 5ms, >>>> + >>>> + For example, some hardware may require a full restart >>>> + (RK806_RESTART mode) in order to function properly as regulators >>>> + are shortly interrupted in this mode. >>>> + >>> >>> This is fine, although now points to missing restart-handler schema and >>> maybe this should be once made common property. But that's just >>> digression, nothing needed here. >>> >>>> vcc1-supply: >>>> description: >>>> The input supply for dcdc-reg1. >>>> diff --git a/include/dt-bindings/mfd/rockchip,rk8xx.h b/include/dt-bindings/mfd/rockchip,rk8xx.h >>>> new file mode 100644 >>>> index 0000000000000000000000000000000000000000..f058ed1ca661185f79738a358aa2d4f04539c590 >>>> --- /dev/null >>>> +++ b/include/dt-bindings/mfd/rockchip,rk8xx.h >>>> @@ -0,0 +1,17 @@ >>>> +/* SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause */ >>>> +/* >>>> + * Device Tree defines for Rockchip RK8xx PMICs >>>> + * >>>> + * Copyright 2025 Cherry Embedded Solutions GmbH >>>> + * >>>> + * Author: Quentin Schulz <quentin.schulz@cherry.de> >>>> + */ >>>> + >>>> +#ifndef _DT_BINDINGS_MFD_ROCKCHIP_RK8XX_H >>>> +#define _DT_BINDINGS_MFD_ROCKCHIP_RK8XX_H >>>> + >>>> +#define RK806_RESTART 0 >>>> +#define RK806_RESET 1 >>>> +#define RK806_RESET_NOTIFY 2 >>> >>> I do not see how this is a binding. Where do you use this in the driver >>> (to be a binding because otherwise you just add unused ABI)? >>> >> >> Explained in the commit log of the driver patch: >> >> """ >> This adds the appropriate logic in the driver to parse the new >> rockchip,reset-mode DT property to pass this information. It just >> happens that the values in the binding match the values to write in the >> bitfield so no mapping is necessary. >> """ >> >> I can add useless mapping in the driver if it's preferred. I had the > > No, I comment and raise questions when you add ABI which is neither ABI > or should not be ABI. > Not sure what would make something part of the ABI or not. I would assume the value in the DT property to be ABI anyway so this is just another name for the same value no? Trying to understand this from your perspective. >> impression that simply using a hardcoded value in the DT binding and >> then writing it to the register was not desired, so the constant is now >> here to make this less obscure from DT perspective though I'm still >> writing the value directly in the register. If hardcoded values are ok >> in the binding, then I can remove that header file. > > If you want something user readable, make it an enum string or keep the > header within DTS. > Just to be sure I understood correctly, moving that file to e.g. arch/arm64/boot/dts/rockchip/rk806.h (or rk8xx.h or whatever) with the file content unchanged from this v2 would be fine with you? Would I be able to point at this file from the DT binding (I assume not)? Of course, Heiko may have a different opinion on the location of this file as I don't see header files in arch/arm64/boot/dts/rockchip yet :) > If I review it like that, it will be brought to me next time for some > other patch saying that commit was reviewed so I can do the same. [1] Fair :) > Therefore since I object against unused binding headers in general > (there is no user here technically), I need to object here as well. :( > Laws do evolve too over time, same as how society view things. Something done decades ago could simply not be acceptable today, and vice-versa. It can be good, it can be bad :) Not here to judge, if there are new rules to contribute, there are new rules to follow :) (or discussed so they evolve to better work for maintainers, the community or project :) ). It's easier to follow rules if they are made explicit somewhere. Is there a public documentation on those "new" rules maybe we can read ahead of time to make this easier on you? For example I really like https://www.kernel.org/doc/html/latest/devicetree/bindings/dts-coding-style.html and point to it often (internally, sometimes on the ML too). Maybe something to add to https://www.kernel.org/doc/html/latest/devicetree/bindings/writing-bindings.html or https://www.kernel.org/doc/html/latest/devicetree/bindings/submitting-patches.html? Cheers, Quentin
On 17/06/2025 12:45, Quentin Schulz wrote: > On 6/17/25 12:21 PM, Krzysztof Kozlowski wrote: >> On 17/06/2025 11:38, Quentin Schulz wrote: >>> Hi Krzysztof, >>> >>> On 6/17/25 10:08 AM, Krzysztof Kozlowski wrote: >>>> On Thu, Jun 05, 2025 at 05:41:06PM GMT, Quentin Schulz wrote: >>>>> + rockchip,reset-mode: >>>>> + $ref: /schemas/types.yaml#/definitions/uint32 >>>>> + enum: [0, 1, 2] >>>>> + description: >>>>> + Mode to use when a reset of the PMIC is triggered. >>>>> + >>>>> + The reset can be triggered either programmatically, via one of >>>>> + the PWRCTRL pins (provided additional configuration) or >>>>> + asserting RESETB pin low. >>>>> + >>>>> + The following modes are supported (see also >>>>> + include/dt-bindings/mfd/rockchip,rk8xx.h) >>>>> + >>>>> + - 0 (RK806_RESTART) restart PMU, >>>>> + - 1 (RK806_RESET) reset all power off reset registers and force >>>>> + state to switch to ACTIVE mode, >>>>> + - 2 (RK806_RESET_NOTIFY) same as RK806_RESET and also pull >>>>> + RESETB pin down for 5ms, >>>>> + >>>>> + For example, some hardware may require a full restart >>>>> + (RK806_RESTART mode) in order to function properly as regulators >>>>> + are shortly interrupted in this mode. >>>>> + >>>> >>>> This is fine, although now points to missing restart-handler schema and >>>> maybe this should be once made common property. But that's just >>>> digression, nothing needed here. >>>> >>>>> vcc1-supply: >>>>> description: >>>>> The input supply for dcdc-reg1. >>>>> diff --git a/include/dt-bindings/mfd/rockchip,rk8xx.h b/include/dt-bindings/mfd/rockchip,rk8xx.h >>>>> new file mode 100644 >>>>> index 0000000000000000000000000000000000000000..f058ed1ca661185f79738a358aa2d4f04539c590 >>>>> --- /dev/null >>>>> +++ b/include/dt-bindings/mfd/rockchip,rk8xx.h >>>>> @@ -0,0 +1,17 @@ >>>>> +/* SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause */ >>>>> +/* >>>>> + * Device Tree defines for Rockchip RK8xx PMICs >>>>> + * >>>>> + * Copyright 2025 Cherry Embedded Solutions GmbH >>>>> + * >>>>> + * Author: Quentin Schulz <quentin.schulz@cherry.de> >>>>> + */ >>>>> + >>>>> +#ifndef _DT_BINDINGS_MFD_ROCKCHIP_RK8XX_H >>>>> +#define _DT_BINDINGS_MFD_ROCKCHIP_RK8XX_H >>>>> + >>>>> +#define RK806_RESTART 0 >>>>> +#define RK806_RESET 1 >>>>> +#define RK806_RESET_NOTIFY 2 >>>> >>>> I do not see how this is a binding. Where do you use this in the driver >>>> (to be a binding because otherwise you just add unused ABI)? >>>> >>> >>> Explained in the commit log of the driver patch: >>> >>> """ >>> This adds the appropriate logic in the driver to parse the new >>> rockchip,reset-mode DT property to pass this information. It just >>> happens that the values in the binding match the values to write in the >>> bitfield so no mapping is necessary. >>> """ >>> >>> I can add useless mapping in the driver if it's preferred. I had the >> >> No, I comment and raise questions when you add ABI which is neither ABI >> or should not be ABI. >> > > Not sure what would make something part of the ABI or not. I would > assume the value in the DT property to be ABI anyway so this is just > another name for the same value no? Trying to understand this from your > perspective. Drop the header, it's not an ABI. You just use register values. This is not a Linux ABI. The values are coming from the hardware. > >>> impression that simply using a hardcoded value in the DT binding and >>> then writing it to the register was not desired, so the constant is now >>> here to make this less obscure from DT perspective though I'm still >>> writing the value directly in the register. If hardcoded values are ok >>> in the binding, then I can remove that header file. >> >> If you want something user readable, make it an enum string or keep the >> header within DTS. >> > > Just to be sure I understood correctly, moving that file to e.g. > arch/arm64/boot/dts/rockchip/rk806.h (or rk8xx.h or whatever) with the Yes > file content unchanged from this v2 would be fine with you? Would I be > able to point at this file from the DT binding (I assume not)? No, because it is not a binding. Best regards, Krzysztof
© 2016 - 2025 Red Hat, Inc.