[PATCH 0/3] Add support for Amlogic T7 Reset

[PATCH 0/3] Add support for Amlogic T7 Reset

[Kelvin Zhang via B4 Relay]

Add a new compatible and device node for Amlogic T7 Reset.

Signed-off-by: Zelong Dong <zelong.dong@amlogic.com>
Signed-off-by: Kelvin Zhang <kelvin.zhang@amlogic.com>
---
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 <kelvin.zhang@amlogic.com>

[PATCH 1/3] dt-bindings: reset: Add Amlogic T7 Reset Controller

[Kelvin Zhang via B4 Relay]

From: Zelong Dong <zelong.dong@amlogic.com>

Add a new compatible and the related header file
for Amlogic T7 Reset Controller.

Signed-off-by: Zelong Dong <zelong.dong@amlogic.com>
Signed-off-by: Kelvin Zhang <kelvin.zhang@amlogic.com>
---
 .../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

Re: [PATCH 1/3] dt-bindings: reset: Add Amlogic T7 Reset Controller

[Krzysztof Kozlowski]

On 29/03/2024 10:17, Kelvin Zhang via B4 Relay wrote:
> From: Zelong Dong <zelong.dong@amlogic.com>
> 
> Add a new compatible and the related header file
> for Amlogic T7 Reset Controller.
> 
> Signed-off-by: Zelong Dong <zelong.dong@amlogic.com>
> Signed-off-by: Kelvin Zhang <kelvin.zhang@amlogic.com>
> ---
>  .../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 <krzysztof.kozlowski@linaro.org>


Best regards,
Krzysztof

Re: [PATCH 1/3] dt-bindings: reset: Add Amlogic T7 Reset Controller

[Neil Armstrong]

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@amlogic.com>
>>
>> Add a new compatible and the related header file
>> for Amlogic T7 Reset Controller.
>>
>> Signed-off-by: Zelong Dong <zelong.dong@amlogic.com>
>> Signed-off-by: Kelvin Zhang <kelvin.zhang@amlogic.com>
>> ---
>>   .../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 <krzysztof.kozlowski@linaro.org>

Thanks,
Neil

> 
> 
> Best regards,
> Krzysztof
> 

Re: [PATCH 1/3] dt-bindings: reset: Add Amlogic T7 Reset Controller

[Krzysztof Kozlowski]

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@amlogic.com>
>>>
>>> Add a new compatible and the related header file
>>> for Amlogic T7 Reset Controller.
>>>
>>> Signed-off-by: Zelong Dong <zelong.dong@amlogic.com>
>>> Signed-off-by: Kelvin Zhang <kelvin.zhang@amlogic.com>
>>> ---
>>>   .../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

Re: [PATCH 1/3] dt-bindings: reset: Add Amlogic T7 Reset Controller

[Krzysztof Kozlowski]

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@amlogic.com>
>>>>
>>>> Add a new compatible and the related header file
>>>> for Amlogic T7 Reset Controller.
>>>>
>>>> Signed-off-by: Zelong Dong <zelong.dong@amlogic.com>
>>>> Signed-off-by: Kelvin Zhang <kelvin.zhang@amlogic.com>
>>>> ---
>>>>   .../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@linaro.org/
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/b60f5fd2-dc48-9375-da1c-ffcfe8292683@linaro.org/
https://lore.kernel.org/linux-devicetree/418c5f0c-5279-41f5-3705-345ec9a97ea2@linaro.org/
https://lore.kernel.org/all/201401111415.29395.arnd@arndb.de/


Best regards,
Krzysztof

Re: [PATCH 1/3] dt-bindings: reset: Add Amlogic T7 Reset Controller

[Kelvin Zhang]

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@amlogic.com>
>>>>>
>>>>> Add a new compatible and the related header file
>>>>> for Amlogic T7 Reset Controller.
>>>>>
>>>>> Signed-off-by: Zelong Dong <zelong.dong@amlogic.com>
>>>>> Signed-off-by: Kelvin Zhang <kelvin.zhang@amlogic.com>
>>>>> ---
>>>>>    .../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@linaro.org/
> 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/b60f5fd2-dc48-9375-da1c-ffcfe8292683@linaro.org/
> https://lore.kernel.org/linux-devicetree/418c5f0c-5279-41f5-3705-345ec9a97ea2@linaro.org/
> https://lore.kernel.org/all/201401111415.29395.arnd@arndb.de/
> 
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
> 

Re: [PATCH 1/3] dt-bindings: reset: Add Amlogic T7 Reset Controller

[neil.armstrong]

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@amlogic.com>
>>>>>>
>>>>>> Add a new compatible and the related header file
>>>>>> for Amlogic T7 Reset Controller.
>>>>>>
>>>>>> Signed-off-by: Zelong Dong <zelong.dong@amlogic.com>
>>>>>> Signed-off-by: Kelvin Zhang <kelvin.zhang@amlogic.com>
>>>>>> ---
>>>>>>    .../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@linaro.org/
>> 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/b60f5fd2-dc48-9375-da1c-ffcfe8292683@linaro.org/
>> https://lore.kernel.org/linux-devicetree/418c5f0c-5279-41f5-3705-345ec9a97ea2@linaro.org/
>> https://lore.kernel.org/all/201401111415.29395.arnd@arndb.de/
>>
> 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
>>

Re: [PATCH 1/3] dt-bindings: reset: Add Amlogic T7 Reset Controller

[Krzysztof Kozlowski]

On 16/04/2024 01:30, neil.armstrong@linaro.org 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@amlogic.com>
>>>>>>>
>>>>>>> Add a new compatible and the related header file
>>>>>>> for Amlogic T7 Reset Controller.
>>>>>>>
>>>>>>> Signed-off-by: Zelong Dong <zelong.dong@amlogic.com>
>>>>>>> Signed-off-by: Kelvin Zhang <kelvin.zhang@amlogic.com>
>>>>>>> ---
>>>>>>>    .../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@linaro.org/
>>> 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/b60f5fd2-dc48-9375-da1c-ffcfe8292683@linaro.org/
>>> https://lore.kernel.org/linux-devicetree/418c5f0c-5279-41f5-3705-345ec9a97ea2@linaro.org/
>>> https://lore.kernel.org/all/201401111415.29395.arnd@arndb.de/
>>>
>> 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

Re: [PATCH 1/3] dt-bindings: reset: Add Amlogic T7 Reset Controller

[neil.armstrong]

On 17/04/2024 21:08, Krzysztof Kozlowski wrote:
> On 16/04/2024 01:30, neil.armstrong@linaro.org 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@amlogic.com>
>>>>>>>>
>>>>>>>> Add a new compatible and the related header file
>>>>>>>> for Amlogic T7 Reset Controller.
>>>>>>>>
>>>>>>>> Signed-off-by: Zelong Dong <zelong.dong@amlogic.com>
>>>>>>>> Signed-off-by: Kelvin Zhang <kelvin.zhang@amlogic.com>
>>>>>>>> ---
>>>>>>>>     .../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@linaro.org/
>>>> 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/b60f5fd2-dc48-9375-da1c-ffcfe8292683@linaro.org/
>>>> https://lore.kernel.org/linux-devicetree/418c5f0c-5279-41f5-3705-345ec9a97ea2@linaro.org/
>>>> https://lore.kernel.org/all/201401111415.29395.arnd@arndb.de/
>>>>
>>> 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
> 

Re: [PATCH 1/3] dt-bindings: reset: Add Amlogic T7 Reset Controller

[Kelvin Zhang]

On 2024/4/19 05:03, neil.armstrong@linaro.org wrote:
> [ EXTERNAL EMAIL ]
> 
> On 17/04/2024 21:08, Krzysztof Kozlowski wrote:
>> On 16/04/2024 01:30, neil.armstrong@linaro.org 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@amlogic.com>
>>>>>>>>>
>>>>>>>>> Add a new compatible and the related header file
>>>>>>>>> for Amlogic T7 Reset Controller.
>>>>>>>>>
>>>>>>>>> Signed-off-by: Zelong Dong <zelong.dong@amlogic.com>
>>>>>>>>> Signed-off-by: Kelvin Zhang <kelvin.zhang@amlogic.com>
>>>>>>>>> ---
>>>>>>>>>     .../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@linaro.org/
>>>>> 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/b60f5fd2-dc48-9375-da1c-ffcfe8292683@linaro.org/
>>>>> https://lore.kernel.org/linux-devicetree/418c5f0c-5279-41f5-3705-345ec9a97ea2@linaro.org/
>>>>> https://lore.kernel.org/all/201401111415.29395.arnd@arndb.de/
>>>>>
>>>> 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
>>
> 

[PATCH 2/3] reset: reset-meson: add support for Amlogic T7 SoC Reset Controller

[Kelvin Zhang via B4 Relay]

From: Zelong Dong <zelong.dong@amlogic.com>

There are 7 sets of Reset Source in Amlogic T7 SoC Reset Controller,
and the offset between base and level registers is 0x40.
Add a new compatible string and struct meson_reset_param to support
the reset controller on T7 SoC.

Signed-off-by: Zelong Dong <zelong.dong@amlogic.com>
Signed-off-by: Kelvin Zhang <kelvin.zhang@amlogic.com>
---
 drivers/reset/reset-meson.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/drivers/reset/reset-meson.c b/drivers/reset/reset-meson.c
index f78be97898bc..1e9fca3e30e8 100644
--- a/drivers/reset/reset-meson.c
+++ b/drivers/reset/reset-meson.c
@@ -102,6 +102,11 @@ static const struct meson_reset_param meson_s4_param = {
 	.level_offset	= 0x40,
 };
 
+static const struct meson_reset_param t7_param = {
+	.reg_count      = 7,
+	.level_offset   = 0x40,
+};
+
 static const struct of_device_id meson_reset_dt_ids[] = {
 	 { .compatible = "amlogic,meson8b-reset",    .data = &meson8b_param},
 	 { .compatible = "amlogic,meson-gxbb-reset", .data = &meson8b_param},
@@ -109,6 +114,7 @@ static const struct of_device_id meson_reset_dt_ids[] = {
 	 { .compatible = "amlogic,meson-a1-reset",   .data = &meson_a1_param},
 	 { .compatible = "amlogic,meson-s4-reset",   .data = &meson_s4_param},
 	 { .compatible = "amlogic,c3-reset",   .data = &meson_s4_param},
+	 { .compatible = "amlogic,t7-reset",   .data = &t7_param},
 	 { /* sentinel */ },
 };
 MODULE_DEVICE_TABLE(of, meson_reset_dt_ids);

-- 
2.37.1

Re: [PATCH 2/3] reset: reset-meson: add support for Amlogic T7 SoC Reset Controller

[Neil Armstrong]

On 29/03/2024 10:17, Kelvin Zhang via B4 Relay wrote:
> From: Zelong Dong <zelong.dong@amlogic.com>
> 
> There are 7 sets of Reset Source in Amlogic T7 SoC Reset Controller,
> and the offset between base and level registers is 0x40.
> Add a new compatible string and struct meson_reset_param to support
> the reset controller on T7 SoC.
> 
> Signed-off-by: Zelong Dong <zelong.dong@amlogic.com>
> Signed-off-by: Kelvin Zhang <kelvin.zhang@amlogic.com>
> ---
>   drivers/reset/reset-meson.c | 6 ++++++
>   1 file changed, 6 insertions(+)
> 
> diff --git a/drivers/reset/reset-meson.c b/drivers/reset/reset-meson.c
> index f78be97898bc..1e9fca3e30e8 100644
> --- a/drivers/reset/reset-meson.c
> +++ b/drivers/reset/reset-meson.c
> @@ -102,6 +102,11 @@ static const struct meson_reset_param meson_s4_param = {
>   	.level_offset	= 0x40,
>   };
>   
> +static const struct meson_reset_param t7_param = {
> +	.reg_count      = 7,
> +	.level_offset   = 0x40,
> +};
> +
>   static const struct of_device_id meson_reset_dt_ids[] = {
>   	 { .compatible = "amlogic,meson8b-reset",    .data = &meson8b_param},
>   	 { .compatible = "amlogic,meson-gxbb-reset", .data = &meson8b_param},
> @@ -109,6 +114,7 @@ static const struct of_device_id meson_reset_dt_ids[] = {
>   	 { .compatible = "amlogic,meson-a1-reset",   .data = &meson_a1_param},
>   	 { .compatible = "amlogic,meson-s4-reset",   .data = &meson_s4_param},
>   	 { .compatible = "amlogic,c3-reset",   .data = &meson_s4_param},
> +	 { .compatible = "amlogic,t7-reset",   .data = &t7_param},
>   	 { /* sentinel */ },
>   };
>   MODULE_DEVICE_TABLE(of, meson_reset_dt_ids);
> 

Reviewed-by: Neil Armstrong <neil.armstrong@linaro.org>

Re: [PATCH 2/3] reset: reset-meson: add support for Amlogic T7 SoC Reset Controller

[Neil Armstrong]

On 29/03/2024 10:17, Kelvin Zhang via B4 Relay wrote:
> From: Zelong Dong <zelong.dong@amlogic.com>
> 
> There are 7 sets of Reset Source in Amlogic T7 SoC Reset Controller,
> and the offset between base and level registers is 0x40.
> Add a new compatible string and struct meson_reset_param to support
> the reset controller on T7 SoC.
> 
> Signed-off-by: Zelong Dong <zelong.dong@amlogic.com>
> Signed-off-by: Kelvin Zhang <kelvin.zhang@amlogic.com>
> ---
>   drivers/reset/reset-meson.c | 6 ++++++
>   1 file changed, 6 insertions(+)
> 
> diff --git a/drivers/reset/reset-meson.c b/drivers/reset/reset-meson.c
> index f78be97898bc..1e9fca3e30e8 100644
> --- a/drivers/reset/reset-meson.c
> +++ b/drivers/reset/reset-meson.c
> @@ -102,6 +102,11 @@ static const struct meson_reset_param meson_s4_param = {
>   	.level_offset	= 0x40,
>   };
>   
> +static const struct meson_reset_param t7_param = {
> +	.reg_count      = 7,
> +	.level_offset   = 0x40,
> +};
> +
>   static const struct of_device_id meson_reset_dt_ids[] = {
>   	 { .compatible = "amlogic,meson8b-reset",    .data = &meson8b_param},
>   	 { .compatible = "amlogic,meson-gxbb-reset", .data = &meson8b_param},
> @@ -109,6 +114,7 @@ static const struct of_device_id meson_reset_dt_ids[] = {
>   	 { .compatible = "amlogic,meson-a1-reset",   .data = &meson_a1_param},
>   	 { .compatible = "amlogic,meson-s4-reset",   .data = &meson_s4_param},
>   	 { .compatible = "amlogic,c3-reset",   .data = &meson_s4_param},
> +	 { .compatible = "amlogic,t7-reset",   .data = &t7_param},
>   	 { /* sentinel */ },
>   };
>   MODULE_DEVICE_TABLE(of, meson_reset_dt_ids);
> 

Reviewed-by: Neil Armstrong <neil.armstrong@linaro.org>

[PATCH 3/3] arm64: dts: amlogic: add reset controller for Amlogic T7 SoC

[Kelvin Zhang via B4 Relay]

From: Zelong Dong <zelong.dong@amlogic.com>

Add the reset controller device of Amlogic T7 SoC family

Signed-off-by: Zelong Dong <zelong.dong@amlogic.com>
Signed-off-by: Kelvin Zhang <kelvin.zhang@amlogic.com>
---
 arch/arm64/boot/dts/amlogic/amlogic-t7.dtsi | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/arch/arm64/boot/dts/amlogic/amlogic-t7.dtsi b/arch/arm64/boot/dts/amlogic/amlogic-t7.dtsi
index 5248bdf824ea..e94bb85b5292 100644
--- a/arch/arm64/boot/dts/amlogic/amlogic-t7.dtsi
+++ b/arch/arm64/boot/dts/amlogic/amlogic-t7.dtsi
@@ -5,6 +5,7 @@
 
 #include <dt-bindings/interrupt-controller/arm-gic.h>
 #include <dt-bindings/power/amlogic,t7-pwrc.h>
+#include <dt-bindings/reset/amlogic,t7-reset.h>
 
 / {
 	interrupt-parent = <&gic>;
@@ -149,6 +150,12 @@ apb4: bus@fe000000 {
 			#size-cells = <2>;
 			ranges = <0x0 0x0 0x0 0xfe000000 0x0 0x480000>;
 
+			reset: reset-controller@2000 {
+				compatible = "amlogic,t7-reset";
+				reg = <0x0 0x2000 0x0 0x98>;
+				#reset-cells = <1>;
+			};
+
 			watchdog@2100 {
 				compatible = "amlogic,t7-wdt";
 				reg = <0x0 0x2100 0x0 0x10>;

-- 
2.37.1

Re: [PATCH 3/3] arm64: dts: amlogic: add reset controller for Amlogic T7 SoC

[Neil Armstrong]

On 29/03/2024 10:17, Kelvin Zhang via B4 Relay wrote:
> From: Zelong Dong <zelong.dong@amlogic.com>
> 
> Add the reset controller device of Amlogic T7 SoC family
> 
> Signed-off-by: Zelong Dong <zelong.dong@amlogic.com>
> Signed-off-by: Kelvin Zhang <kelvin.zhang@amlogic.com>
> ---
>   arch/arm64/boot/dts/amlogic/amlogic-t7.dtsi | 7 +++++++
>   1 file changed, 7 insertions(+)
> 
> diff --git a/arch/arm64/boot/dts/amlogic/amlogic-t7.dtsi b/arch/arm64/boot/dts/amlogic/amlogic-t7.dtsi
> index 5248bdf824ea..e94bb85b5292 100644
> --- a/arch/arm64/boot/dts/amlogic/amlogic-t7.dtsi
> +++ b/arch/arm64/boot/dts/amlogic/amlogic-t7.dtsi
> @@ -5,6 +5,7 @@
>   
>   #include <dt-bindings/interrupt-controller/arm-gic.h>
>   #include <dt-bindings/power/amlogic,t7-pwrc.h>
> +#include <dt-bindings/reset/amlogic,t7-reset.h>
>   
>   / {
>   	interrupt-parent = <&gic>;
> @@ -149,6 +150,12 @@ apb4: bus@fe000000 {
>   			#size-cells = <2>;
>   			ranges = <0x0 0x0 0x0 0xfe000000 0x0 0x480000>;
>   
> +			reset: reset-controller@2000 {
> +				compatible = "amlogic,t7-reset";
> +				reg = <0x0 0x2000 0x0 0x98>;
> +				#reset-cells = <1>;
> +			};
> +
>   			watchdog@2100 {
>   				compatible = "amlogic,t7-wdt";
>   				reg = <0x0 0x2100 0x0 0x10>;
> 

Reviewed-by: Neil Armstrong <neil.armstrong@linaro.org>

Re: [PATCH 0/3] Add support for Amlogic T7 Reset

[Neil Armstrong]

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 <zelong.dong@amlogic.com>
> Signed-off-by: Kelvin Zhang <kelvin.zhang@amlogic.com>
> ---
> 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,