Re: [PATCH 1/3] dt-bindings: reset: Add Amlogic T7 Reset Controller
From: Kelvin Zhang
Date: Thu Apr 18 2024 - 21:51:23 EST
On 2024/4/19 05:03, neil.armstrong@xxxxxxxxxx wrote:
[ EXTERNAL EMAIL ]
On 17/04/2024 21:08, Krzysztof Kozlowski wrote:
On 16/04/2024 01:30, neil.armstrong@xxxxxxxxxx wrote:
On 15/04/2024 12:31, Kelvin Zhang wrote:
On 2024/4/13 02:03, Krzysztof Kozlowski wrote:
[ EXTERNAL EMAIL ]
On 12/04/2024 19:57, Krzysztof Kozlowski wrote:
On 12/04/2024 15:12, Neil Armstrong wrote:
Hi,
On 29/03/2024 20:39, Krzysztof Kozlowski wrote:
On 29/03/2024 10:17, Kelvin Zhang via B4 Relay wrote:
From: Zelong Dong <zelong.dong@xxxxxxxxxxx>
Add a new compatible and the related header file
for Amlogic T7 Reset Controller.
Signed-off-by: Zelong Dong <zelong.dong@xxxxxxxxxxx>
Signed-off-by: Kelvin Zhang <kelvin.zhang@xxxxxxxxxxx>
---
.../bindings/reset/amlogic,meson-reset.yaml | 1 +
include/dt-bindings/reset/amlogic,t7-reset.h | 197
+++++++++++++++++++++
2 files changed, 198 insertions(+)
diff --git
a/Documentation/devicetree/bindings/reset/amlogic,meson-reset.yaml b/Documentation/devicetree/bindings/reset/amlogic,meson-reset.yaml
index f0c6c0df0ce3..fefe343e5afe 100644
---
a/Documentation/devicetree/bindings/reset/amlogic,meson-reset.yaml
+++
b/Documentation/devicetree/bindings/reset/amlogic,meson-reset.yaml
@@ -19,6 +19,7 @@ properties:
- amlogic,meson-a1-reset # Reset Controller on A1 and
compatible SoCs
- amlogic,meson-s4-reset # Reset Controller on S4 and
compatible SoCs
- amlogic,c3-reset # Reset Controller on C3 and
compatible SoCs
+ - amlogic,t7-reset # Reset Controller on T7 and
compatible SoCs
If there is going to be any resend, please drop the comment.
It's not
really helpful and makes it trickier to read.
reg:
maxItems: 1
diff --git a/include/dt-bindings/reset/amlogic,t7-reset.h
b/include/dt-bindings/reset/amlogic,t7-reset.h
new file mode 100644
index 000000000000..ca4a832eeeec
--- /dev/null
+++ b/include/dt-bindings/reset/amlogic,t7-reset.h
@@ -0,0 +1,197 @@
+/* SPDX-License-Identifier: (GPL-2.0-only OR MIT) */
+/*
+ * Copyright (c) 2024 Amlogic, Inc. All rights reserved.
+ */
+
+#ifndef _DT_BINDINGS_AMLOGIC_T7_RESET_H
+#define _DT_BINDINGS_AMLOGIC_T7_RESET_H
+
+/* RESET0 */
+/* 0-3 */
I assume this matches existing drivers which do not use IDs but
map the
binding to hardware value? I remember we talked about changing
it, so if
something happened about this and it could be changed: please
change.
I'm not aware of such discussion, and I don't really see the
issue...
thoses are IDs, and yes they match the Hardware offsets, and ?
Bindings are not for hardware offsets/values/addresses. It's just
not a
binding.
I quickly looked at your driver patch and it confirms: not a binding.
Binding constant is used by the driver and DTS consumer.
I am really sure we had this talk in the past, but could be I think
about different platform. Since this is not a binding, I do not think
claiming there is any ABI here is reasonable. Feel free to store them
with other hardware values, like in DTS headers etc. We already
moved to
DTS headers several such "non-binding" constants.
Un-acked.
I looked at my archives and we did talk about it and you were CCed:
https://lore.kernel.org/linux-devicetree/c088e01c-0714-82be-8347-6140daf56640@xxxxxxxxxx/
simple-reset is an exception.
So to recap:
That's not a binding. Don't add some real values to binding headers
because it is not a binding then.
So what's exactly a binding then?
Binding headers is interface needed (necessary) between implementation
(like Linux drivers) and DTS.
random linear numbers that means nothing can be a binding
but registers numbers can't be ? why ? I still don't understand, why
this suddenly gets problematic ?
There is no interface here. Drivers don't use them. It's not "suddenly"
problematic, I commented on this year or two years ago and we also
started moving such header-abusers out of bindings.
https://lore.kernel.org/linux-devicetree/CAK8P3a1APzs74YTcZ=m43G3zrmwJZKcYSTvV5eDDQX-37UY7Tw@xxxxxxxxxxxxxx/
https://lore.kernel.org/linux-devicetree/CAK8P3a0fDJQvGLEtG0fxLkG08Fh9V7LEMPsx4AaS+2Ldo_xWxw@xxxxxxxxxxxxxx/
https://lore.kernel.org/linux-devicetree/b60f5fd2-dc48-9375-da1c-ffcfe8292683@xxxxxxxxxx/
https://lore.kernel.org/linux-devicetree/418c5f0c-5279-41f5-3705-345ec9a97ea2@xxxxxxxxxx/
https://lore.kernel.org/all/201401111415.29395.arnd@xxxxxxxx/
Got it. Will delete amlogic,t7-reset.h and use the hardware numbers
directly in the DT. >
Hi Neil,
As you know, Amlogic reset controller is divided into several
groups: reset0, reset1, ..., resetN. I'd like to discuss the
rationality of splitting the one device node of reset controller
into device nodes according to the groups. Then we can use the bit
number within the 'resets' property.
reset0: reset-controller@2000 {
...
};
reset1: reset-controller@2004 {
...
};
...
What do you think?
No since you'll basically add a node per register, you need to add a
node for the while reset HW function, another
solution would be to split the phandle arguments in 2, the first
first would be the reset bank, and the second one
the reset line for the bank.
But still it's a regression in readability to drop the macros, until
gpios or pins the reset number doesn't mean anything per se.
What stops you from putting the header in the DTS? Just like others are
doing?
Ok so now I understand, Kelvin just keep the header but move it in
arch/arm64/boot/dts/amlogic along the DT patch and drop it from the
bindings patch.
Will do.
Thanks!
Thanks,
Neil
Best regards,
Krzysztof