Add a new compatible and device node for Amlogic T7 Reset.
Signed-off-by: Zelong Dong <[email protected]>
Signed-off-by: Kelvin Zhang <[email protected]>
---
Zelong Dong (3):
dt-bindings: reset: Add Amlogic T7 Reset Controller
reset: reset-meson: add support for Amlogic T7 SoC Reset Controller
arm64: dts: amlogic: add reset controller for Amlogic T7 SoC
.../bindings/reset/amlogic,meson-reset.yaml | 1 +
arch/arm64/boot/dts/amlogic/amlogic-t7.dtsi | 7 +
drivers/reset/reset-meson.c | 6 +
include/dt-bindings/reset/amlogic,t7-reset.h | 197 +++++++++++++++++++++
4 files changed, 211 insertions(+)
---
base-commit: a6bd6c9333397f5a0e2667d4d82fef8c970108f2
change-id: 20240329-t7-reset-f87e8346fadb
Best regards,
--
Kelvin Zhang <[email protected]>
From: Zelong Dong <[email protected]>
Add a new compatible and the related header file
for Amlogic T7 Reset Controller.
Signed-off-by: Zelong Dong <[email protected]>
Signed-off-by: Kelvin Zhang <[email protected]>
---
.../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
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 */
+#define RESET_USB 4
+#define RESET_U2DRD 5
+#define RESET_U3DRD 6
+#define RESET_U3DRD_PIPE0 7
+#define RESET_U2PHY20 8
+#define RESET_U2PHY21 9
+#define RESET_GDC 10
+#define RESET_HDMI20_AES 11
+#define RESET_HDMIRX 12
+#define RESET_HDMIRX_APB 13
+#define RESET_DEWARP 14
+/* 15 */
+#define RESET_HDMITX_CAPB3 16
+#define RESET_BRG_VCBUG_DEC 17
+#define RESET_VCBUS 18
+#define RESET_VID_PLL_DIV 19
+#define RESET_VDI6 20
+#define RESET_GE2D 21
+#define RESET_HDMITXPHY 22
+#define RESET_VID_LOCK 23
+#define RESET_VENC0 24
+#define RESET_VDAC 25
+#define RESET_VENC2 26
+#define RESET_VENC1 27
+#define RESET_RDMA 28
+#define RESET_HDMITX 29
+#define RESET_VIU 30
+#define RESET_VENC 31
+
+/* RESET1 */
+#define RESET_AUDIO 32
+#define RESET_MALI_CAPB3 33
+#define RESET_MALI 34
+#define RESET_DDR_APB 35
+#define RESET_DDR 36
+#define RESET_DOS_CAPB3 37
+#define RESET_DOS 38
+#define RESET_COMBO_DPHY_CHAN2 39
+#define RESET_DEBUG_B 40
+#define RESET_DEBUG_A 41
+#define RESET_DSP_B 42
+#define RESET_DSP_A 43
+#define RESET_PCIE_A 44
+#define RESET_PCIE_PHY 45
+#define RESET_PCIE_APB 46
+#define RESET_ANAKIN 47
+#define RESET_ETH 48
+#define RESET_EDP0_CTRL 49
+#define RESET_EDP1_CTRL 50
+#define RESET_COMBO_DPHY_CHAN0 51
+#define RESET_COMBO_DPHY_CHAN1 52
+#define RESET_DSI_LVDS_EDP_TOP 53
+#define RESET_PCIE1_PHY 54
+#define RESET_PCIE1_APB 55
+#define RESET_DDR_1 56
+/* 57 */
+#define RESET_EDP1_PIPELINE 58
+#define RESET_EDP0_PIPELINE 59
+#define RESET_MIPI_DSI1_PHY 60
+#define RESET_MIPI_DSI0_PHY 61
+#define RESET_MIPI_DSI_A_HOST 62
+#define RESET_MIPI_DSI_B_HOST 63
+
+/* RESET2 */
+#define RESET_DEVICE_MMC_ARB 64
+#define RESET_IR_CTRL 65
+#define RESET_TS_A73 66
+#define RESET_TS_A53 67
+#define RESET_SPICC_2 68
+#define RESET_SPICC_3 69
+#define RESET_SPICC_4 70
+#define RESET_SPICC_5 71
+#define RESET_SMART_CARD 72
+#define RESET_SPICC_0 73
+#define RESET_SPICC_1 74
+#define RESET_RSA 75
+/* 76-79 */
+#define RESET_MSR_CLK 80
+#define RESET_SPIFC 81
+#define RESET_SAR_ADC 82
+#define RESET_BT 83
+/* 84-87 */
+#define RESET_ACODEC 88
+#define RESET_CEC 89
+#define RESET_AFIFO 90
+#define RESET_WATCHDOG 91
+/* 92-95 */
+
+/* RESET3 */
+#define RESET_BRG_NIC1_GPV 96
+#define RESET_BRG_NIC2_GPV 97
+#define RESET_BRG_NIC3_GPV 98
+#define RESET_BRG_NIC4_GPV 99
+#define RESET_BRG_NIC5_GPV 100
+/* 101-121 */
+#define RESET_MIPI_ISP 122
+#define RESET_BRG_ADB_MALI_1 123
+#define RESET_BRG_ADB_MALI_0 124
+#define RESET_BRG_ADB_A73 125
+#define RESET_BRG_ADB_A53 126
+#define RESET_BRG_CCI 127
+
+/* RESET4 */
+#define RESET_PWM_AO_AB 128
+#define RESET_PWM_AO_CD 129
+#define RESET_PWM_AO_EF 130
+#define RESET_PWM_AO_GH 131
+#define RESET_PWM_AB 132
+#define RESET_PWM_CD 133
+#define RESET_PWM_EF 134
+/* 135-137 */
+#define RESET_UART_A 138
+#define RESET_UART_B 139
+#define RESET_UART_C 140
+#define RESET_UART_D 141
+#define RESET_UART_E 142
+#define RESET_UART_F 143
+#define RESET_I2C_S_A 144
+#define RESET_I2C_M_A 145
+#define RESET_I2C_M_B 146
+#define RESET_I2C_M_C 147
+#define RESET_I2C_M_D 148
+#define RESET_I2C_M_E 149
+#define RESET_I2C_M_F 150
+#define RESET_I2C_M_AO_A 151
+#define RESET_SD_EMMC_A 152
+#define RESET_SD_EMMC_B 153
+#define RESET_SD_EMMC_C 154
+#define RESET_I2C_M_AO_B 155
+#define RESET_TS_GPU 156
+#define RESET_TS_NNA 157
+#define RESET_TS_VPN 158
+#define RESET_TS_HEVC 159
+
+/* RESET5 */
+#define RESET_BRG_NOC_DDR_1 160
+#define RESET_BRG_NOC_DDR_0 161
+#define RESET_BRG_NOC_MAIN 162
+#define RESET_BRG_NOC_ALL 163
+/* 164-167 */
+#define RESET_BRG_NIC2_SYS 168
+#define RESET_BRG_NIC2_MAIN 169
+#define RESET_BRG_NIC2_HDMI 170
+#define RESET_BRG_NIC2_ALL 171
+#define RESET_BRG_NIC3_WAVE 172
+#define RESET_BRG_NIC3_VDEC 173
+#define RESET_BRG_NIC3_HEVCF 174
+#define RESET_BRG_NIC3_HEVCB 175
+#define RESET_BRG_NIC3_HCODEC 176
+#define RESET_BRG_NIC3_GE2D 177
+#define RESET_BRG_NIC3_GDC 178
+#define RESET_BRG_NIC3_AMLOGIC 179
+#define RESET_BRG_NIC3_MAIN 180
+#define RESET_BRG_NIC3_ALL 181
+#define RESET_BRG_NIC5_VPU 182
+/* 183-185 */
+#define RESET_BRG_NIC4_DSPB 186
+#define RESET_BRG_NIC4_DSPA 187
+#define RESET_BRG_NIC4_VAPB 188
+#define RESET_BRG_NIC4_CLK81 189
+#define RESET_BRG_NIC4_MAIN 190
+#define RESET_BRG_NIC4_ALL 191
+
+/* RESET6 */
+#define RESET_BRG_VDEC_PIPEL 192
+#define RESET_BRG_HEVCF_DMC_PIPEL 193
+#define RESET_BRG_NIC2TONIC4_PIPEL 194
+#define RESET_BRG_HDMIRXTONIC2_PIPEL 195
+#define RESET_BRG_SECTONIC4_PIPEL 196
+#define RESET_BRG_VPUTONOC_PIPEL 197
+#define RESET_BRG_NIC4TONOC_PIPEL 198
+#define RESET_BRG_NIC3TONOC_PIPEL 199
+#define RESET_BRG_NIC2TONOC_PIPEL 200
+#define RESET_BRG_NNATONOC_PIPEL 201
+#define RESET_BRG_FRISP3_PIPEL 202
+#define RESET_BRG_FRISP2_PIPEL 203
+#define RESET_BRG_FRISP1_PIPEL 204
+#define RESET_BRG_FRISP0_PIPEL 205
+/* 206-217 */
+#define RESET_BRG_AMPIPE_NAND 218
+#define RESET_BRG_AMPIPE_ETH 219
+/* 220 */
+#define RESET_BRG_AM2AXI0 221
+#define RESET_BRG_AM2AXI1 222
+#define RESET_BRG_AM2AXI2 223
+
+#endif
--
2.37.1
On 29/03/2024 10:17, Kelvin Zhang via B4 Relay wrote:
> From: Zelong Dong <[email protected]>
>
> Add a new compatible and the related header file
> for Amlogic T7 Reset Controller.
>
> Signed-off-by: Zelong Dong <[email protected]>
> Signed-off-by: Kelvin Zhang <[email protected]>
> ---
> .../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.
Otherwise, it's fine:
Acked-by: Krzysztof Kozlowski <[email protected]>
Best regards,
Krzysztof
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 <[email protected]>
>>
>> Add a new compatible and the related header file
>> for Amlogic T7 Reset Controller.
>>
>> Signed-off-by: Zelong Dong <[email protected]>
>> Signed-off-by: Kelvin Zhang <[email protected]>
>> ---
>> .../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 ?
>
> Otherwise, it's fine:
>
> Acked-by: Krzysztof Kozlowski <[email protected]>
Thanks,
Neil
>
>
> Best regards,
> Krzysztof
>
Hi Philipp,
On 29/03/2024 10:17, Kelvin Zhang via B4 Relay wrote:
> Add a new compatible and device node for Amlogic T7 Reset.
>
> Signed-off-by: Zelong Dong <[email protected]>
> Signed-off-by: Kelvin Zhang <[email protected]>
> ---
> Zelong Dong (3):
> dt-bindings: reset: Add Amlogic T7 Reset Controller
> reset: reset-meson: add support for Amlogic T7 SoC Reset Controller
> arm64: dts: amlogic: add reset controller for Amlogic T7 SoC
If you could apply patches 1 & 2 and prepare me an immutable branch, I could
merge the DT part for v6.10.
Thanks,
Neil
>
> .../bindings/reset/amlogic,meson-reset.yaml | 1 +
> arch/arm64/boot/dts/amlogic/amlogic-t7.dtsi | 7 +
> drivers/reset/reset-meson.c | 6 +
> include/dt-bindings/reset/amlogic,t7-reset.h | 197 +++++++++++++++++++++
> 4 files changed, 211 insertions(+)
> ---
> base-commit: a6bd6c9333397f5a0e2667d4d82fef8c970108f2
> change-id: 20240329-t7-reset-f87e8346fadb
>
> Best regards,
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 <[email protected]>
>>>
>>> Add a new compatible and the related header file
>>> for Amlogic T7 Reset Controller.
>>>
>>> Signed-off-by: Zelong Dong <[email protected]>
>>> Signed-off-by: Kelvin Zhang <[email protected]>
>>> ---
>>> .../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.
Best regards,
Krzysztof
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 <[email protected]>
>>>>
>>>> Add a new compatible and the related header file
>>>> for Amlogic T7 Reset Controller.
>>>>
>>>> Signed-off-by: Zelong Dong <[email protected]>
>>>> Signed-off-by: Kelvin Zhang <[email protected]>
>>>> ---
>>>> .../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/[email protected]/
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.
https://lore.kernel.org/linux-devicetree/CAK8P3a1APzs74YTcZ=m43G3zrmwJZKcYSTvV5eDDQX-37UY7Tw@mail.gmail.com/
https://lore.kernel.org/linux-devicetree/CAK8P3a0fDJQvGLEtG0fxLkG08Fh9V7LEMPsx4AaS+2Ldo_xWxw@mail.gmail.com/
https://lore.kernel.org/linux-devicetree/[email protected]/
https://lore.kernel.org/linux-devicetree/[email protected]/
https://lore.kernel.org/all/[email protected]/
Best regards,
Krzysztof
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 <[email protected]>
>>>>>
>>>>> Add a new compatible and the related header file
>>>>> for Amlogic T7 Reset Controller.
>>>>>
>>>>> Signed-off-by: Zelong Dong <[email protected]>
>>>>> Signed-off-by: Kelvin Zhang <[email protected]>
>>>>> ---
>>>>> .../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/[email protected]/
> 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.
>
> https://lore.kernel.org/linux-devicetree/CAK8P3a1APzs74YTcZ=m43G3zrmwJZKcYSTvV5eDDQX-37UY7Tw@mail.gmail.com/
> https://lore.kernel.org/linux-devicetree/CAK8P3a0fDJQvGLEtG0fxLkG08Fh9V7LEMPsx4AaS+2Ldo_xWxw@mail.gmail.com/
> https://lore.kernel.org/linux-devicetree/[email protected]/
> https://lore.kernel.org/linux-devicetree/[email protected]/
> https://lore.kernel.org/all/[email protected]/
>
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?
Thanks!
>
> Best regards,
> Krzysztof
>
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 <[email protected]>
>>>>>>
>>>>>> Add a new compatible and the related header file
>>>>>> for Amlogic T7 Reset Controller.
>>>>>>
>>>>>> Signed-off-by: Zelong Dong <[email protected]>
>>>>>> Signed-off-by: Kelvin Zhang <[email protected]>
>>>>>> ---
>>>>>> .../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/[email protected]/
>> 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? 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 ?
>>
>> https://lore.kernel.org/linux-devicetree/CAK8P3a1APzs74YTcZ=m43G3zrmwJZKcYSTvV5eDDQX-37UY7Tw@mail.gmail.com/
>> https://lore.kernel.org/linux-devicetree/CAK8P3a0fDJQvGLEtG0fxLkG08Fh9V7LEMPsx4AaS+2Ldo_xWxw@mail.gmail.com/
>> https://lore.kernel.org/linux-devicetree/[email protected]/
>> https://lore.kernel.org/linux-devicetree/[email protected]/
>> https://lore.kernel.org/all/[email protected]/
>>
> 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.
Neil
Neil
> Thanks!
>>
>> Best regards,
>> Krzysztof
>>
On 16/04/2024 01:30, [email protected] 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 <[email protected]>
>>>>>>>
>>>>>>> Add a new compatible and the related header file
>>>>>>> for Amlogic T7 Reset Controller.
>>>>>>>
>>>>>>> Signed-off-by: Zelong Dong <[email protected]>
>>>>>>> Signed-off-by: Kelvin Zhang <[email protected]>
>>>>>>> ---
>>>>>>> .../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/[email protected]/
>>> 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@mail.gmail.com/
>>> https://lore.kernel.org/linux-devicetree/CAK8P3a0fDJQvGLEtG0fxLkG08Fh9V7LEMPsx4AaS+2Ldo_xWxw@mail.gmail.com/
>>> https://lore.kernel.org/linux-devicetree/[email protected]/
>>> https://lore.kernel.org/linux-devicetree/[email protected]/
>>> https://lore.kernel.org/all/[email protected]/
>>>
>> 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?
Best regards,
Krzysztof
On 17/04/2024 21:08, Krzysztof Kozlowski wrote:
> On 16/04/2024 01:30, [email protected] 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 <[email protected]>
>>>>>>>>
>>>>>>>> Add a new compatible and the related header file
>>>>>>>> for Amlogic T7 Reset Controller.
>>>>>>>>
>>>>>>>> Signed-off-by: Zelong Dong <[email protected]>
>>>>>>>> Signed-off-by: Kelvin Zhang <[email protected]>
>>>>>>>> ---
>>>>>>>> .../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/[email protected]/
>>>> 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@mail.gmail.com/
>>>> https://lore.kernel.org/linux-devicetree/CAK8P3a0fDJQvGLEtG0fxLkG08Fh9V7LEMPsx4AaS+2Ldo_xWxw@mail.gmail.com/
>>>> https://lore.kernel.org/linux-devicetree/[email protected]/
>>>> https://lore.kernel.org/linux-devicetree/[email protected]/
>>>> https://lore.kernel.org/all/[email protected]/
>>>>
>>> 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.
Thanks,
Neil
>
> Best regards,
> Krzysztof
>
On 2024/4/19 05:03, [email protected] wrote:
> [ EXTERNAL EMAIL ]
>
> On 17/04/2024 21:08, Krzysztof Kozlowski wrote:
>> On 16/04/2024 01:30, [email protected] 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 <[email protected]>
>>>>>>>>>
>>>>>>>>> Add a new compatible and the related header file
>>>>>>>>> for Amlogic T7 Reset Controller.
>>>>>>>>>
>>>>>>>>> Signed-off-by: Zelong Dong <[email protected]>
>>>>>>>>> Signed-off-by: Kelvin Zhang <[email protected]>
>>>>>>>>> ---
>>>>>>>>> .../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/[email protected]/
>>>>> 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@mail.gmail.com/
>>>>> https://lore.kernel.org/linux-devicetree/CAK8P3a0fDJQvGLEtG0fxLkG08Fh9V7LEMPsx4AaS+2Ldo_xWxw@mail.gmail.com/
>>>>> https://lore.kernel.org/linux-devicetree/[email protected]/
>>>>> https://lore.kernel.org/linux-devicetree/[email protected]/
>>>>> https://lore.kernel.org/all/[email protected]/
>>>>>
>>>> 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
>>
>