Added documentation and Versal-NET reset indices to describe about
Versal-Net reset driver bindings.
In Versal-NET all reset indices includes Class, SubClass, Type, Index
information whereas class refers to clock, reset, power etc.,
Underlying firmware in Versal have such classification and expects
the ID to be this way.
[13:0] - Index bits
[19:14] - Type bits
[25:20] - SubClass bits
[31:26] - Class bits.
Signed-off-by: Piyush Mehta <[email protected]>
---
.../bindings/reset/xlnx,zynqmp-reset.yaml | 4 +
.../reset/xlnx-versal-net-resets.h | 127 ++++++++++++++++++
2 files changed, 131 insertions(+)
create mode 100644 include/dt-bindings/reset/xlnx-versal-net-resets.h
diff --git a/Documentation/devicetree/bindings/reset/xlnx,zynqmp-reset.yaml b/Documentation/devicetree/bindings/reset/xlnx,zynqmp-reset.yaml
index 0d50f6a54af3..b996fc1d4e53 100644
--- a/Documentation/devicetree/bindings/reset/xlnx,zynqmp-reset.yaml
+++ b/Documentation/devicetree/bindings/reset/xlnx,zynqmp-reset.yaml
@@ -27,11 +27,15 @@ description: |
For list of all valid reset indices for Versal
<dt-bindings/reset/xlnx-versal-resets.h>
+ For list of all valid reset indices for Versal-NET
+ <dt-bindings/reset/xlnx-versal-net-resets.h>
+
properties:
compatible:
enum:
- xlnx,zynqmp-reset
- xlnx,versal-reset
+ - xlnx,versal-net-reset
"#reset-cells":
const: 1
diff --git a/include/dt-bindings/reset/xlnx-versal-net-resets.h b/include/dt-bindings/reset/xlnx-versal-net-resets.h
new file mode 100644
index 000000000000..b3e7d5e4c33e
--- /dev/null
+++ b/include/dt-bindings/reset/xlnx-versal-net-resets.h
@@ -0,0 +1,127 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+/*
+ * Copyright (C) 2023 Advanced Micro Devices, Inc. All Rights Reserved.
+ */
+
+#ifndef _DT_BINDINGS_VERSAL_NET_RESETS_H
+#define _DT_BINDINGS_VERSAL_NET_RESETS_H
+
+#define PM_RST_PMC_POR (0xc30c001U)
+#define PM_RST_PMC (0xc410002U)
+#define PM_RST_PS_POR (0xc30c003U)
+#define PM_RST_PL_POR (0xc30c004U)
+#define PM_RST_NOC_POR (0xc30c005U)
+#define PM_RST_PS_SRST (0xc41000aU)
+#define PM_RST_PL_SRST (0xc41000bU)
+#define PM_RST_NOC (0xc41000cU)
+#define PM_RST_NPI (0xc41000dU)
+#define PM_RST_SYS_RST_1 (0xc41000eU)
+#define PM_RST_SYS_RST_2 (0xc41000fU)
+#define PM_RST_SYS_RST_3 (0xc410010U)
+#define PM_RST_PL0 (0xc410012U)
+#define PM_RST_PL1 (0xc410013U)
+#define PM_RST_PL2 (0xc410014U)
+#define PM_RST_PL3 (0xc410015U)
+#define PM_RST_ACPU_GIC (0xc41001aU)
+#define PM_RST_ADMA (0xc410026U)
+#define PM_RST_OCM (0xc410028U)
+#define PM_RST_IPI (0xc41002aU)
+#define PM_RST_QSPI (0xc10402dU)
+#define PM_RST_OSPI (0xc10402eU)
+#define PM_RST_SDIO_0 (0xc10402fU)
+#define PM_RST_SDIO_1 (0xc104030U)
+#define PM_RST_GPIO_PMC (0xc104032U)
+#define PM_RST_GEM_0 (0xc104033U)
+#define PM_RST_GEM_1 (0xc104034U)
+#define PM_RST_USB_0 (0xc104036U)
+#define PM_RST_UART_0 (0xc104037U)
+#define PM_RST_UART_1 (0xc104038U)
+#define PM_RST_SPI_0 (0xc104039U)
+#define PM_RST_SPI_1 (0xc10403aU)
+#define PM_RST_CAN_FD_0 (0xc10403bU)
+#define PM_RST_CAN_FD_1 (0xc10403cU)
+#define PM_RST_GPIO_LPD (0xc10403fU)
+#define PM_RST_TTC_0 (0xc104040U)
+#define PM_RST_GPIO_LPD (0xc10403fU)
+#define PM_RST_TTC_0 (0xc104040U)
+#define PM_RST_TTC_1 (0xc104041U)
+#define PM_RST_TTC_2 (0xc104042U)
+#define PM_RST_TTC_3 (0xc104043U)
+#define PM_RST_WWDT (0xC410079U)
+#define PM_RST_SYS_1 (0xC41007AU)
+#define PM_RST_SYS_3 (0xC41007BU)
+#define PM_RST_SYS_2 (0xC41007CU)
+#define PM_RST_I2C (0xC410085U)
+#define PM_RST_FPD_SWDT_2 (0xC41008AU)
+#define PM_RST_FPD_SWDT_1 (0xC41008CU)
+#define PM_RST_APU3_CORE1_WARM (0xC514099U)
+#define PM_RST_APU3_CORE3_COLD (0xC61809AU)
+#define PM_RST_APU3_CORE0_COLD (0xC61809BU)
+#define PM_RST_APU3_CORE1_COLD (0xC61809CU)
+#define PM_RST_APU3_CLUSTER_COLD (0xC61809DU)
+#define PM_RST_APU3_CORE0_WARM (0xC51409EU)
+#define PM_RST_APU3_CLUSTER_COLD (0xC61809DU)
+#define PM_RST_APU3_CORE0_WARM (0xC51409EU)
+#define PM_RST_APU3_CORE2_COLD (0xC61809FU)
+#define PM_RST_APU3_CORE2_WARM (0xC5140A0U)
+#define PM_RST_APU3_CORE3_WARM (0xC5140A1U)
+#define PM_RST_APU3_CLUSTER_WARM (0xC5140A2U)
+#define PM_RST_FPD_SWDT_3 (0xC4100A3U)
+#define PM_RST_APU1_CORE1_WARM (0xC5140A4U)
+#define PM_RST_APU1_CORE3_COLD (0xC6180A5U)
+#define PM_RST_APU1_CORE0_COLD (0xC6180A6U)
+#define PM_RST_APU1_CORE1_COLD (0xC6180A7U)
+#define PM_RST_APU1_CLUSTER_COLD (0xC6180A8U)
+#define PM_RST_APU1_CORE0_WARM (0xC5140A9U)
+#define PM_RST_APU1_CORE2_COLD (0xC6180AAU)
+#define PM_RST_APU1_CORE2_WARM (0xC5140ABU)
+#define PM_RST_APU1_CORE3_WARM (0xC5140ACU)
+#define PM_RST_APU1_CLUSTER_WARM (0xC5140ADU)
+#define PM_RST_APU0_CORE1_WARM (0xC5140AFU)
+#define PM_RST_APU0_CORE3_COLD (0xC6180B0U)
+#define PM_RST_APU0_CORE0_COLD (0xC6180B1U)
+#define PM_RST_APU0_CORE1_COLD (0xC6180B2U)
+#define PM_RST_APU0_CLUSTER_COLD (0xC6180B3U)
+#define PM_RST_APU0_CORE0_WARM (0xC5140B4U)
+#define PM_RST_APU0_CORE2_COLD (0xC6180B5U)
+#define PM_RST_APU0_CORE2_WARM (0xC5140B6U)
+#define PM_RST_APU0_CORE3_WARM (0xC5140B7U)
+#define PM_RST_APU0_CLUSTER_WARM (0xC5140B8U)
+#define PM_RST_FPD_SWDT_0 (0xC4100B9U)
+#define PM_RST_APU2_CORE1_WARM (0xC5140BAU)
+#define PM_RST_APU2_CORE3_COLD (0xC6180BBU)
+#define PM_RST_APU2_CORE0_COLD (0xC6180BCU)
+#define PM_RST_APU2_CORE1_COLD (0xC6180BDU)
+#define PM_RST_APU2_CORE0_COLD (0xC6180BCU)
+#define PM_RST_APU2_CORE1_COLD (0xC6180BDU)
+#define PM_RST_APU2_CLUSTER_COLD (0xC6180BEU)
+#define PM_RST_APU2_CORE0_WARM (0xC5140BFU)
+#define PM_RST_APU2_CORE2_COLD (0xC6180C0U)
+#define PM_RST_APU2_CORE2_WARM (0xC5140C1U)
+#define PM_RST_APU2_CORE3_WARM (0xC5140C2U)
+#define PM_RST_APU2_CLUSTER_WARM (0xC5140C3U)
+#define PM_RST_USB_1 (0xC1040C6U)
+#define PM_RST_SWDT_1 (0xC4100C7U)
+#define PM_RST_SWDT_0 (0xC4100C8U)
+#define PM_RST_RPU_A_GD (0xC4100C9U)
+#define PM_RST_RPU_B_GD (0xC4100CAU)
+#define PM_RST_RPU_CORE0A (0xC4100CBU)
+#define PM_RST_RPU_CORE0A_POR (0xC30C0CCU)
+#define PM_RST_RPU_CORE0B_POR (0xC30C0CDU)
+#define PM_RST_RPU_A_GD_TOP (0xC4100CEU)
+#define PM_RST_RPU_CORE1B (0xC4100CFU)
+#define PM_RST_RPU_B_TOPRESET (0xC4100D0U)
+#define PM_RST_RPU_CORE1B_POR (0xC30C0D1U)
+#define PM_RST_RPU_CORE1A (0xC4100D2U)
+#define PM_RST_RPU_B_GD_TOP (0xC4100D3U)
+#define PM_RST_RPU_A_TOPRESET (0xC4100D4U)
+#define PM_RST_RPU_B_DBGRST (0xC2080D5U)
+#define PM_RST_RPU_A_DCLS_TOPRESET (0xC4100D6U)
+#define PM_RST_RPU_CORE1A_POR (0xC30C0D7U)
+#define PM_RST_RPU_B_DCLS_TOPRESET (0xC4100D8U)
+#define PM_RST_RPU_A_DBGRST (0xC2080D9U)
+#define PM_RST_RPU_CORE0B (0xC4100DAU)
+#define PM_RST_I3C_1 (0xC4100DFU)
+#define PM_RST_I3C_0 (0xC4100E0U)
+
+#endif
--
2.25.1
On Mon, Jul 17, 2023 at 04:53:47PM +0530, Piyush Mehta wrote:
> Added documentation and Versal-NET reset indices to describe about
> Versal-Net reset driver bindings.
>
> In Versal-NET all reset indices includes Class, SubClass, Type, Index
> information whereas class refers to clock, reset, power etc.,
> Underlying firmware in Versal have such classification and expects
> the ID to be this way.
> [13:0] - Index bits
> [19:14] - Type bits
> [25:20] - SubClass bits
> [31:26] - Class bits.
Riight.. I'm not sure that describing these as "indices" is really all
that valid, given only 13:0 are actually the index.
I'd be inclined to say that the type/class/subclass stuff should not be
part of the dt-bindings, and instead looked up inside the driver
depending on the index.
Hopefully Rob or Krzysztof can comment further.
Thanks,
Conor.
>
> Signed-off-by: Piyush Mehta <[email protected]>
> ---
> .../bindings/reset/xlnx,zynqmp-reset.yaml | 4 +
> .../reset/xlnx-versal-net-resets.h | 127 ++++++++++++++++++
> 2 files changed, 131 insertions(+)
> create mode 100644 include/dt-bindings/reset/xlnx-versal-net-resets.h
>
> diff --git a/Documentation/devicetree/bindings/reset/xlnx,zynqmp-reset.yaml b/Documentation/devicetree/bindings/reset/xlnx,zynqmp-reset.yaml
> index 0d50f6a54af3..b996fc1d4e53 100644
> --- a/Documentation/devicetree/bindings/reset/xlnx,zynqmp-reset.yaml
> +++ b/Documentation/devicetree/bindings/reset/xlnx,zynqmp-reset.yaml
> @@ -27,11 +27,15 @@ description: |
> For list of all valid reset indices for Versal
> <dt-bindings/reset/xlnx-versal-resets.h>
>
> + For list of all valid reset indices for Versal-NET
> + <dt-bindings/reset/xlnx-versal-net-resets.h>
> +
> properties:
> compatible:
> enum:
> - xlnx,zynqmp-reset
> - xlnx,versal-reset
> + - xlnx,versal-net-reset
>
> "#reset-cells":
> const: 1
> diff --git a/include/dt-bindings/reset/xlnx-versal-net-resets.h b/include/dt-bindings/reset/xlnx-versal-net-resets.h
> new file mode 100644
> index 000000000000..b3e7d5e4c33e
> --- /dev/null
> +++ b/include/dt-bindings/reset/xlnx-versal-net-resets.h
> @@ -0,0 +1,127 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +/*
> + * Copyright (C) 2023 Advanced Micro Devices, Inc. All Rights Reserved.
> + */
> +
> +#ifndef _DT_BINDINGS_VERSAL_NET_RESETS_H
> +#define _DT_BINDINGS_VERSAL_NET_RESETS_H
> +
> +#define PM_RST_PMC_POR (0xc30c001U)
> +#define PM_RST_PMC (0xc410002U)
> +#define PM_RST_PS_POR (0xc30c003U)
> +#define PM_RST_PL_POR (0xc30c004U)
> +#define PM_RST_NOC_POR (0xc30c005U)
> +#define PM_RST_PS_SRST (0xc41000aU)
> +#define PM_RST_PL_SRST (0xc41000bU)
> +#define PM_RST_NOC (0xc41000cU)
> +#define PM_RST_NPI (0xc41000dU)
> +#define PM_RST_SYS_RST_1 (0xc41000eU)
> +#define PM_RST_SYS_RST_2 (0xc41000fU)
> +#define PM_RST_SYS_RST_3 (0xc410010U)
> +#define PM_RST_PL0 (0xc410012U)
> +#define PM_RST_PL1 (0xc410013U)
> +#define PM_RST_PL2 (0xc410014U)
> +#define PM_RST_PL3 (0xc410015U)
> +#define PM_RST_ACPU_GIC (0xc41001aU)
> +#define PM_RST_ADMA (0xc410026U)
> +#define PM_RST_OCM (0xc410028U)
> +#define PM_RST_IPI (0xc41002aU)
> +#define PM_RST_QSPI (0xc10402dU)
> +#define PM_RST_OSPI (0xc10402eU)
> +#define PM_RST_SDIO_0 (0xc10402fU)
> +#define PM_RST_SDIO_1 (0xc104030U)
> +#define PM_RST_GPIO_PMC (0xc104032U)
> +#define PM_RST_GEM_0 (0xc104033U)
> +#define PM_RST_GEM_1 (0xc104034U)
> +#define PM_RST_USB_0 (0xc104036U)
> +#define PM_RST_UART_0 (0xc104037U)
> +#define PM_RST_UART_1 (0xc104038U)
> +#define PM_RST_SPI_0 (0xc104039U)
> +#define PM_RST_SPI_1 (0xc10403aU)
> +#define PM_RST_CAN_FD_0 (0xc10403bU)
> +#define PM_RST_CAN_FD_1 (0xc10403cU)
> +#define PM_RST_GPIO_LPD (0xc10403fU)
> +#define PM_RST_TTC_0 (0xc104040U)
> +#define PM_RST_GPIO_LPD (0xc10403fU)
> +#define PM_RST_TTC_0 (0xc104040U)
> +#define PM_RST_TTC_1 (0xc104041U)
> +#define PM_RST_TTC_2 (0xc104042U)
> +#define PM_RST_TTC_3 (0xc104043U)
> +#define PM_RST_WWDT (0xC410079U)
> +#define PM_RST_SYS_1 (0xC41007AU)
> +#define PM_RST_SYS_3 (0xC41007BU)
> +#define PM_RST_SYS_2 (0xC41007CU)
> +#define PM_RST_I2C (0xC410085U)
> +#define PM_RST_FPD_SWDT_2 (0xC41008AU)
> +#define PM_RST_FPD_SWDT_1 (0xC41008CU)
> +#define PM_RST_APU3_CORE1_WARM (0xC514099U)
> +#define PM_RST_APU3_CORE3_COLD (0xC61809AU)
> +#define PM_RST_APU3_CORE0_COLD (0xC61809BU)
> +#define PM_RST_APU3_CORE1_COLD (0xC61809CU)
> +#define PM_RST_APU3_CLUSTER_COLD (0xC61809DU)
> +#define PM_RST_APU3_CORE0_WARM (0xC51409EU)
> +#define PM_RST_APU3_CLUSTER_COLD (0xC61809DU)
> +#define PM_RST_APU3_CORE0_WARM (0xC51409EU)
> +#define PM_RST_APU3_CORE2_COLD (0xC61809FU)
> +#define PM_RST_APU3_CORE2_WARM (0xC5140A0U)
> +#define PM_RST_APU3_CORE3_WARM (0xC5140A1U)
> +#define PM_RST_APU3_CLUSTER_WARM (0xC5140A2U)
> +#define PM_RST_FPD_SWDT_3 (0xC4100A3U)
> +#define PM_RST_APU1_CORE1_WARM (0xC5140A4U)
> +#define PM_RST_APU1_CORE3_COLD (0xC6180A5U)
> +#define PM_RST_APU1_CORE0_COLD (0xC6180A6U)
> +#define PM_RST_APU1_CORE1_COLD (0xC6180A7U)
> +#define PM_RST_APU1_CLUSTER_COLD (0xC6180A8U)
> +#define PM_RST_APU1_CORE0_WARM (0xC5140A9U)
> +#define PM_RST_APU1_CORE2_COLD (0xC6180AAU)
> +#define PM_RST_APU1_CORE2_WARM (0xC5140ABU)
> +#define PM_RST_APU1_CORE3_WARM (0xC5140ACU)
> +#define PM_RST_APU1_CLUSTER_WARM (0xC5140ADU)
> +#define PM_RST_APU0_CORE1_WARM (0xC5140AFU)
> +#define PM_RST_APU0_CORE3_COLD (0xC6180B0U)
> +#define PM_RST_APU0_CORE0_COLD (0xC6180B1U)
> +#define PM_RST_APU0_CORE1_COLD (0xC6180B2U)
> +#define PM_RST_APU0_CLUSTER_COLD (0xC6180B3U)
> +#define PM_RST_APU0_CORE0_WARM (0xC5140B4U)
> +#define PM_RST_APU0_CORE2_COLD (0xC6180B5U)
> +#define PM_RST_APU0_CORE2_WARM (0xC5140B6U)
> +#define PM_RST_APU0_CORE3_WARM (0xC5140B7U)
> +#define PM_RST_APU0_CLUSTER_WARM (0xC5140B8U)
> +#define PM_RST_FPD_SWDT_0 (0xC4100B9U)
> +#define PM_RST_APU2_CORE1_WARM (0xC5140BAU)
> +#define PM_RST_APU2_CORE3_COLD (0xC6180BBU)
> +#define PM_RST_APU2_CORE0_COLD (0xC6180BCU)
> +#define PM_RST_APU2_CORE1_COLD (0xC6180BDU)
> +#define PM_RST_APU2_CORE0_COLD (0xC6180BCU)
> +#define PM_RST_APU2_CORE1_COLD (0xC6180BDU)
> +#define PM_RST_APU2_CLUSTER_COLD (0xC6180BEU)
> +#define PM_RST_APU2_CORE0_WARM (0xC5140BFU)
> +#define PM_RST_APU2_CORE2_COLD (0xC6180C0U)
> +#define PM_RST_APU2_CORE2_WARM (0xC5140C1U)
> +#define PM_RST_APU2_CORE3_WARM (0xC5140C2U)
> +#define PM_RST_APU2_CLUSTER_WARM (0xC5140C3U)
> +#define PM_RST_USB_1 (0xC1040C6U)
> +#define PM_RST_SWDT_1 (0xC4100C7U)
> +#define PM_RST_SWDT_0 (0xC4100C8U)
> +#define PM_RST_RPU_A_GD (0xC4100C9U)
> +#define PM_RST_RPU_B_GD (0xC4100CAU)
> +#define PM_RST_RPU_CORE0A (0xC4100CBU)
> +#define PM_RST_RPU_CORE0A_POR (0xC30C0CCU)
> +#define PM_RST_RPU_CORE0B_POR (0xC30C0CDU)
> +#define PM_RST_RPU_A_GD_TOP (0xC4100CEU)
> +#define PM_RST_RPU_CORE1B (0xC4100CFU)
> +#define PM_RST_RPU_B_TOPRESET (0xC4100D0U)
> +#define PM_RST_RPU_CORE1B_POR (0xC30C0D1U)
> +#define PM_RST_RPU_CORE1A (0xC4100D2U)
> +#define PM_RST_RPU_B_GD_TOP (0xC4100D3U)
> +#define PM_RST_RPU_A_TOPRESET (0xC4100D4U)
> +#define PM_RST_RPU_B_DBGRST (0xC2080D5U)
> +#define PM_RST_RPU_A_DCLS_TOPRESET (0xC4100D6U)
> +#define PM_RST_RPU_CORE1A_POR (0xC30C0D7U)
> +#define PM_RST_RPU_B_DCLS_TOPRESET (0xC4100D8U)
> +#define PM_RST_RPU_A_DBGRST (0xC2080D9U)
> +#define PM_RST_RPU_CORE0B (0xC4100DAU)
> +#define PM_RST_I3C_1 (0xC4100DFU)
> +#define PM_RST_I3C_0 (0xC4100E0U)
> +
> +#endif
> --
> 2.25.1
>
On 17/07/2023 20:40, Conor Dooley wrote:
> On Mon, Jul 17, 2023 at 04:53:47PM +0530, Piyush Mehta wrote:
>> Added documentation and Versal-NET reset indices to describe about
>> Versal-Net reset driver bindings.
>>
>> In Versal-NET all reset indices includes Class, SubClass, Type, Index
>> information whereas class refers to clock, reset, power etc.,
>> Underlying firmware in Versal have such classification and expects
>> the ID to be this way.
>> [13:0] - Index bits
>> [19:14] - Type bits
>> [25:20] - SubClass bits
>> [31:26] - Class bits.
>
> Riight.. I'm not sure that describing these as "indices" is really all
> that valid, given only 13:0 are actually the index.
> I'd be inclined to say that the type/class/subclass stuff should not be
> part of the dt-bindings, and instead looked up inside the driver
> depending on the index.
>
> Hopefully Rob or Krzysztof can comment further.
This confuses me as well. I don't understand why do you need it in the
bindings. Nothing uses these values, so storing them as bindings seems
pointless.
>
> Thanks,
> Conor.
>
>>
>> Signed-off-by: Piyush Mehta <[email protected]>
>> ---
>> .../bindings/reset/xlnx,zynqmp-reset.yaml | 4 +
>> .../reset/xlnx-versal-net-resets.h | 127 ++++++++++++++++++
>> 2 files changed, 131 insertions(+)
>> create mode 100644 include/dt-bindings/reset/xlnx-versal-net-resets.h
>>
>> diff --git a/Documentation/devicetree/bindings/reset/xlnx,zynqmp-reset.yaml b/Documentation/devicetree/bindings/reset/xlnx,zynqmp-reset.yaml
>> index 0d50f6a54af3..b996fc1d4e53 100644
>> --- a/Documentation/devicetree/bindings/reset/xlnx,zynqmp-reset.yaml
>> +++ b/Documentation/devicetree/bindings/reset/xlnx,zynqmp-reset.yaml
>> @@ -27,11 +27,15 @@ description: |
>> For list of all valid reset indices for Versal
>> <dt-bindings/reset/xlnx-versal-resets.h>
>>
>> + For list of all valid reset indices for Versal-NET
>> + <dt-bindings/reset/xlnx-versal-net-resets.h>
>> +
>> properties:
>> compatible:
>> enum:
>> - xlnx,zynqmp-reset
>> - xlnx,versal-reset
>> + - xlnx,versal-net-reset
>>
>> "#reset-cells":
>> const: 1
>> diff --git a/include/dt-bindings/reset/xlnx-versal-net-resets.h b/include/dt-bindings/reset/xlnx-versal-net-resets.h
>> new file mode 100644
>> index 000000000000..b3e7d5e4c33e
>> --- /dev/null
>> +++ b/include/dt-bindings/reset/xlnx-versal-net-resets.h
>> @@ -0,0 +1,127 @@
>> +/* SPDX-License-Identifier: GPL-2.0 */
>> +/*
>> + * Copyright (C) 2023 Advanced Micro Devices, Inc. All Rights Reserved.
>> + */
>> +
>> +#ifndef _DT_BINDINGS_VERSAL_NET_RESETS_H
>> +#define _DT_BINDINGS_VERSAL_NET_RESETS_H
>> +
>> +#define PM_RST_PMC_POR (0xc30c001U)
IDs start from 0 and are incremented by 1.
Best regards,
Krzysztof
On 7/17/23 22:47, Krzysztof Kozlowski wrote:
> On 17/07/2023 20:40, Conor Dooley wrote:
>> On Mon, Jul 17, 2023 at 04:53:47PM +0530, Piyush Mehta wrote:
>>> Added documentation and Versal-NET reset indices to describe about
>>> Versal-Net reset driver bindings.
>>>
>>> In Versal-NET all reset indices includes Class, SubClass, Type, Index
>>> information whereas class refers to clock, reset, power etc.,
>>> Underlying firmware in Versal have such classification and expects
>>> the ID to be this way.
>>> [13:0] - Index bits
>>> [19:14] - Type bits
>>> [25:20] - SubClass bits
>>> [31:26] - Class bits.
>>
>> Riight.. I'm not sure that describing these as "indices" is really all
>> that valid, given only 13:0 are actually the index.
>> I'd be inclined to say that the type/class/subclass stuff should not be
>> part of the dt-bindings, and instead looked up inside the driver
>> depending on the index.
>>
>> Hopefully Rob or Krzysztof can comment further.
>
> This confuses me as well. I don't understand why do you need it in the
> bindings. Nothing uses these values, so storing them as bindings seems
> pointless.
Power Management team wants to use these NodeID with format describe above to
identify that elements. And I already told them that ID (0-max) translation to
internal NodeID format should be done in firmware but they don't pretty much
agree with it.
From DT binding perspective I think it doesn't really matter if numbers are
from 0 to max or they are from random high number to another random number
without step equal to 1.
And it is driver implementation detail if driver itself is checking that
requested ID is bigger than number of pins passed.
In connection to reset driver in Linux.
Core has:
static int of_reset_simple_xlate(struct reset_controller_dev *rcdev,
const struct of_phandle_args *reset_spec)
{
if (reset_spec->args[0] >= rcdev->nr_resets)
return -EINVAL;
return reset_spec->args[0];
}
/**
* reset_controller_register - register a reset controller device
* @rcdev: a pointer to the initialized reset controller device
*/
int reset_controller_register(struct reset_controller_dev *rcdev)
{
if (!rcdev->of_xlate) {
rcdev->of_reset_n_cells = 1;
rcdev->of_xlate = of_reset_simple_xlate;
}
...
And zynqmp reset driver already defines of_xlate function that's why checking
against nr_resets is not done as is visible from code below.
static int zynqmp_reset_of_xlate(struct reset_controller_dev *rcdev,
const struct of_phandle_args *reset_spec)
{
return reset_spec->args[0];
}
And actually Xilinx Versal platform is using this for a while and you can find
IDs description here.
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/include/dt-bindings/reset/xlnx-versal-resets.h?h=v6.5-rc2
Xilinx ZynqMP is using IDs from 0 to 119
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/include/dt-bindings/reset/xlnx-zynqmp-resets.h?h=v6.5-rc2
but IDs itself are shifted by 1000:
include/linux/firmware/xlnx-zynqmp.h:217: ZYNQMP_PM_RESET_START = 1000,
#define ZYNQMP_RESET_ID ZYNQMP_PM_RESET_START
static const struct zynqmp_reset_soc_data zynqmp_reset_data = {
.reset_id = ZYNQMP_RESET_ID,
.num_resets = ZYNQMP_NR_RESETS,
};
static int zynqmp_reset_assert(struct reset_controller_dev *rcdev,
unsigned long id)
{
struct zynqmp_reset_data *priv = to_zynqmp_reset_data(rcdev);
return zynqmp_pm_reset_assert(priv->data->reset_id + id,
PM_RESET_ACTION_ASSERT);
}
That numbers in DT are virtual no matter if you use ID from 0 to max or random
values it is up to code to handle them. Checking nr_pins against ID is done in
core but it is up to drivers.
In our case that IDs are coming from firmware and driver itself is just matching
them.
At the end of day pretty much IDs in header can be from 0 to max and conversion
to IDs can be done in the driver itself or in driver probe firmware can be
requested to provide IDs with specific call.
Thanks,
Michal
On 18/07/2023 09:10, Michal Simek wrote:
>
>
> On 7/17/23 22:47, Krzysztof Kozlowski wrote:
>> On 17/07/2023 20:40, Conor Dooley wrote:
>>> On Mon, Jul 17, 2023 at 04:53:47PM +0530, Piyush Mehta wrote:
>>>> Added documentation and Versal-NET reset indices to describe about
>>>> Versal-Net reset driver bindings.
>>>>
>>>> In Versal-NET all reset indices includes Class, SubClass, Type, Index
>>>> information whereas class refers to clock, reset, power etc.,
>>>> Underlying firmware in Versal have such classification and expects
>>>> the ID to be this way.
>>>> [13:0] - Index bits
>>>> [19:14] - Type bits
>>>> [25:20] - SubClass bits
>>>> [31:26] - Class bits.
>>>
>>> Riight.. I'm not sure that describing these as "indices" is really all
>>> that valid, given only 13:0 are actually the index.
>>> I'd be inclined to say that the type/class/subclass stuff should not be
>>> part of the dt-bindings, and instead looked up inside the driver
>>> depending on the index.
>>>
>>> Hopefully Rob or Krzysztof can comment further.
>>
>> This confuses me as well. I don't understand why do you need it in the
>> bindings. Nothing uses these values, so storing them as bindings seems
>> pointless.
>
> Power Management team wants to use these NodeID with format describe above to
> identify that elements. And I already told them that ID (0-max) translation to
> internal NodeID format should be done in firmware but they don't pretty much
> agree with it.
Too bad for them. They can join the discussion, though :)
>
> From DT binding perspective I think it doesn't really matter if numbers are
> from 0 to max or they are from random high number to another random number
> without step equal to 1.
> And it is driver implementation detail if driver itself is checking that
> requested ID is bigger than number of pins passed.
>
> In connection to reset driver in Linux.
>
> Core has:
> static int of_reset_simple_xlate(struct reset_controller_dev *rcdev,
> const struct of_phandle_args *reset_spec)
> {
> if (reset_spec->args[0] >= rcdev->nr_resets)
> return -EINVAL;
>
> return reset_spec->args[0];
> }
>
> /**
> * reset_controller_register - register a reset controller device
> * @rcdev: a pointer to the initialized reset controller device
> */
> int reset_controller_register(struct reset_controller_dev *rcdev)
> {
> if (!rcdev->of_xlate) {
> rcdev->of_reset_n_cells = 1;
> rcdev->of_xlate = of_reset_simple_xlate;
> }
> ...
If you use both, it means you don't need bindings.
>
> And zynqmp reset driver already defines of_xlate function that's why checking
> against nr_resets is not done as is visible from code below.
>
>
> static int zynqmp_reset_of_xlate(struct reset_controller_dev *rcdev,
> const struct of_phandle_args *reset_spec)
> {
> return reset_spec->args[0];
> }
Exactly, there is no xlate. These IDs are not suitable nor needed for
bindings.
>
>
> And actually Xilinx Versal platform is using this for a while and you can find
> IDs description here.
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/include/dt-bindings/reset/xlnx-versal-resets.h?h=v6.5-rc2
We cannot fix existing bindings, but we can fix future ones.
>
> Xilinx ZynqMP is using IDs from 0 to 119
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/include/dt-bindings/reset/xlnx-zynqmp-resets.h?h=v6.5-rc2
>
>
> but IDs itself are shifted by 1000:
> include/linux/firmware/xlnx-zynqmp.h:217: ZYNQMP_PM_RESET_START = 1000,
> #define ZYNQMP_RESET_ID ZYNQMP_PM_RESET_START
>
> static const struct zynqmp_reset_soc_data zynqmp_reset_data = {
> .reset_id = ZYNQMP_RESET_ID,
> .num_resets = ZYNQMP_NR_RESETS,
> };
>
> static int zynqmp_reset_assert(struct reset_controller_dev *rcdev,
> unsigned long id)
> {
> struct zynqmp_reset_data *priv = to_zynqmp_reset_data(rcdev);
>
> return zynqmp_pm_reset_assert(priv->data->reset_id + id,
> PM_RESET_ACTION_ASSERT);
> }
>
>
> That numbers in DT are virtual no matter if you use ID from 0 to max or random
> values it is up to code to handle them. Checking nr_pins against ID is done in
> core but it is up to drivers.
No, you confuse "virtual" and "ID". IDs are not virtual. IDs are real
and have representation in Linux driver. You do not need to define
anything virtual in the bindings.
> In our case that IDs are coming from firmware and driver itself is just matching
> them.
So they are the same as if coming from hardware - no need for IDs.
> At the end of day pretty much IDs in header can be from 0 to max and conversion
> to IDs can be done in the driver itself or in driver probe firmware can be
> requested to provide IDs with specific call.
Best regards,
Krzysztof
On 7/18/23 09:39, Krzysztof Kozlowski wrote:
> On 18/07/2023 09:10, Michal Simek wrote:
>>
>>
>> On 7/17/23 22:47, Krzysztof Kozlowski wrote:
>>> On 17/07/2023 20:40, Conor Dooley wrote:
>>>> On Mon, Jul 17, 2023 at 04:53:47PM +0530, Piyush Mehta wrote:
>>>>> Added documentation and Versal-NET reset indices to describe about
>>>>> Versal-Net reset driver bindings.
>>>>>
>>>>> In Versal-NET all reset indices includes Class, SubClass, Type, Index
>>>>> information whereas class refers to clock, reset, power etc.,
>>>>> Underlying firmware in Versal have such classification and expects
>>>>> the ID to be this way.
>>>>> [13:0] - Index bits
>>>>> [19:14] - Type bits
>>>>> [25:20] - SubClass bits
>>>>> [31:26] - Class bits.
>>>>
>>>> Riight.. I'm not sure that describing these as "indices" is really all
>>>> that valid, given only 13:0 are actually the index.
>>>> I'd be inclined to say that the type/class/subclass stuff should not be
>>>> part of the dt-bindings, and instead looked up inside the driver
>>>> depending on the index.
>>>>
>>>> Hopefully Rob or Krzysztof can comment further.
>>>
>>> This confuses me as well. I don't understand why do you need it in the
>>> bindings. Nothing uses these values, so storing them as bindings seems
>>> pointless.
>>
>> Power Management team wants to use these NodeID with format describe above to
>> identify that elements. And I already told them that ID (0-max) translation to
>> internal NodeID format should be done in firmware but they don't pretty much
>> agree with it.
>
> Too bad for them. They can join the discussion, though :)
>
>>
>> From DT binding perspective I think it doesn't really matter if numbers are
>> from 0 to max or they are from random high number to another random number
>> without step equal to 1.
>> And it is driver implementation detail if driver itself is checking that
>> requested ID is bigger than number of pins passed.
>>
>> In connection to reset driver in Linux.
>>
>> Core has:
>> static int of_reset_simple_xlate(struct reset_controller_dev *rcdev,
>> const struct of_phandle_args *reset_spec)
>> {
>> if (reset_spec->args[0] >= rcdev->nr_resets)
>> return -EINVAL;
>>
>> return reset_spec->args[0];
>> }
>>
>> /**
>> * reset_controller_register - register a reset controller device
>> * @rcdev: a pointer to the initialized reset controller device
>> */
>> int reset_controller_register(struct reset_controller_dev *rcdev)
>> {
>> if (!rcdev->of_xlate) {
>> rcdev->of_reset_n_cells = 1;
>> rcdev->of_xlate = of_reset_simple_xlate;
>> }
>> ...
>
> If you use both, it means you don't need bindings.
>
>>
>> And zynqmp reset driver already defines of_xlate function that's why checking
>> against nr_resets is not done as is visible from code below.
>>
>>
>> static int zynqmp_reset_of_xlate(struct reset_controller_dev *rcdev,
>> const struct of_phandle_args *reset_spec)
>> {
>> return reset_spec->args[0];
>> }
>
> Exactly, there is no xlate. These IDs are not suitable nor needed for
> bindings.
>
>>
>>
>> And actually Xilinx Versal platform is using this for a while and you can find
>> IDs description here.
>> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/include/dt-bindings/reset/xlnx-versal-resets.h?h=v6.5-rc2
>
> We cannot fix existing bindings, but we can fix future ones.
>
>>
>> Xilinx ZynqMP is using IDs from 0 to 119
>> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/include/dt-bindings/reset/xlnx-zynqmp-resets.h?h=v6.5-rc2
>>
>>
>> but IDs itself are shifted by 1000:
>> include/linux/firmware/xlnx-zynqmp.h:217: ZYNQMP_PM_RESET_START = 1000,
>> #define ZYNQMP_RESET_ID ZYNQMP_PM_RESET_START
>>
>> static const struct zynqmp_reset_soc_data zynqmp_reset_data = {
>> .reset_id = ZYNQMP_RESET_ID,
>> .num_resets = ZYNQMP_NR_RESETS,
>> };
>>
>> static int zynqmp_reset_assert(struct reset_controller_dev *rcdev,
>> unsigned long id)
>> {
>> struct zynqmp_reset_data *priv = to_zynqmp_reset_data(rcdev);
>>
>> return zynqmp_pm_reset_assert(priv->data->reset_id + id,
>> PM_RESET_ACTION_ASSERT);
>> }
>>
>>
>> That numbers in DT are virtual no matter if you use ID from 0 to max or random
>> values it is up to code to handle them. Checking nr_pins against ID is done in
>> core but it is up to drivers.
>
> No, you confuse "virtual" and "ID". IDs are not virtual. IDs are real
> and have representation in Linux driver. You do not need to define
> anything virtual in the bindings.
Not sure how you define ID itself. But HW doesn't know ID. HW knows only
register which you can use to perform the reset. It is not really 128bit
register where every bit targets to different IP.
And this is SW-firmware interface like SCMI reset driver.
Firmware is saying that ID 0 is QSPI, ID 1 is MMC.
Their Linux driver is asking for nr_reset via firmware call which can be
different for different SOC and that's fine and I have no problem with it.
But only SCMI server is dictating that ID 0 is QSPI and ID 1 is MMC. Different
SCMI server implementation can map it differently.
>> In our case that IDs are coming from firmware and driver itself is just matching
>> them.
>
> So they are the same as if coming from hardware - no need for IDs.
It is hard to say what hardware here exactly is. From my perspective and I am
not advocating not using IDs from 0 to max, it is just a number.
If my firmware knows that QSPI reset is 0xc10402dU then I will just pass it to
reach my goal which is reset QSPI IP.
If you think that we should use IDs from 0 to max NR I am happy to pass this
message to PM team and we should extend any SW to do translation between.
Thanks,
Michal
On 18/07/2023 15:20, Krzysztof Kozlowski wrote:
> On 18/07/2023 15:11, Michal Simek wrote:
>>>>
>>>> That numbers in DT are virtual no matter if you use ID from 0 to max or random
>>>> values it is up to code to handle them. Checking nr_pins against ID is done in
>>>> core but it is up to drivers.
>>>
>>> No, you confuse "virtual" and "ID". IDs are not virtual. IDs are real
>>> and have representation in Linux driver. You do not need to define
>>> anything virtual in the bindings.
>>
>> Not sure how you define ID itself. But HW doesn't know ID. HW knows only
>> register which you can use to perform the reset. It is not really 128bit
>> register where every bit targets to different IP.
>>
>> And this is SW-firmware interface like SCMI reset driver.
>>
>> Firmware is saying that ID 0 is QSPI, ID 1 is MMC.
>> Their Linux driver is asking for nr_reset via firmware call which can be
>> different for different SOC and that's fine and I have no problem with it.
>> But only SCMI server is dictating that ID 0 is QSPI and ID 1 is MMC. Different
>> SCMI server implementation can map it differently.
>
> Sure, and all this points to: no need for bindings.
>
>>
>>
>>>> In our case that IDs are coming from firmware and driver itself is just matching
>>>> them.
>>>
>>> So they are the same as if coming from hardware - no need for IDs.
>>
>> It is hard to say what hardware here exactly is. From my perspective and I am
>> not advocating not using IDs from 0 to max, it is just a number.
>>
>> If my firmware knows that QSPI reset is 0xc10402dU then I will just pass it to
>> reach my goal which is reset QSPI IP.
>>
>> If you think that we should use IDs from 0 to max NR I am happy to pass this
>> message to PM team and we should extend any SW to do translation between.
>
> When we talk about IDs and bindings, we mean IDs meaningful to Linux.
> Whatever is ignored by Linux and passed to anyone else - hardware or
> firmware - is not a ID anymore from bindings point of view. It's just
> some value.
And just some proofs:
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/include/dt-bindings/reset/xlnx-versal-resets.h?h=v6.5-rc2
$ git grep VERSAL_RST
Results: No users.
Best regards,
Krzysztof
On 18/07/2023 15:11, Michal Simek wrote:
>>>
>>> That numbers in DT are virtual no matter if you use ID from 0 to max or random
>>> values it is up to code to handle them. Checking nr_pins against ID is done in
>>> core but it is up to drivers.
>>
>> No, you confuse "virtual" and "ID". IDs are not virtual. IDs are real
>> and have representation in Linux driver. You do not need to define
>> anything virtual in the bindings.
>
> Not sure how you define ID itself. But HW doesn't know ID. HW knows only
> register which you can use to perform the reset. It is not really 128bit
> register where every bit targets to different IP.
>
> And this is SW-firmware interface like SCMI reset driver.
>
> Firmware is saying that ID 0 is QSPI, ID 1 is MMC.
> Their Linux driver is asking for nr_reset via firmware call which can be
> different for different SOC and that's fine and I have no problem with it.
> But only SCMI server is dictating that ID 0 is QSPI and ID 1 is MMC. Different
> SCMI server implementation can map it differently.
Sure, and all this points to: no need for bindings.
>
>
>>> In our case that IDs are coming from firmware and driver itself is just matching
>>> them.
>>
>> So they are the same as if coming from hardware - no need for IDs.
>
> It is hard to say what hardware here exactly is. From my perspective and I am
> not advocating not using IDs from 0 to max, it is just a number.
>
> If my firmware knows that QSPI reset is 0xc10402dU then I will just pass it to
> reach my goal which is reset QSPI IP.
>
> If you think that we should use IDs from 0 to max NR I am happy to pass this
> message to PM team and we should extend any SW to do translation between.
When we talk about IDs and bindings, we mean IDs meaningful to Linux.
Whatever is ignored by Linux and passed to anyone else - hardware or
firmware - is not a ID anymore from bindings point of view. It's just
some value.
Best regards,
Krzysztof
On 7/18/23 15:20, Krzysztof Kozlowski wrote:
> On 18/07/2023 15:11, Michal Simek wrote:
>>>>
>>>> That numbers in DT are virtual no matter if you use ID from 0 to max or random
>>>> values it is up to code to handle them. Checking nr_pins against ID is done in
>>>> core but it is up to drivers.
>>>
>>> No, you confuse "virtual" and "ID". IDs are not virtual. IDs are real
>>> and have representation in Linux driver. You do not need to define
>>> anything virtual in the bindings.
>>
>> Not sure how you define ID itself. But HW doesn't know ID. HW knows only
>> register which you can use to perform the reset. It is not really 128bit
>> register where every bit targets to different IP.
>>
>> And this is SW-firmware interface like SCMI reset driver.
>>
>> Firmware is saying that ID 0 is QSPI, ID 1 is MMC.
>> Their Linux driver is asking for nr_reset via firmware call which can be
>> different for different SOC and that's fine and I have no problem with it.
>> But only SCMI server is dictating that ID 0 is QSPI and ID 1 is MMC. Different
>> SCMI server implementation can map it differently.
>
> Sure, and all this points to: no need for bindings.
>
>>
>>
>>>> In our case that IDs are coming from firmware and driver itself is just matching
>>>> them.
>>>
>>> So they are the same as if coming from hardware - no need for IDs.
>>
>> It is hard to say what hardware here exactly is. From my perspective and I am
>> not advocating not using IDs from 0 to max, it is just a number.
>>
>> If my firmware knows that QSPI reset is 0xc10402dU then I will just pass it to
>> reach my goal which is reset QSPI IP.
>>
>> If you think that we should use IDs from 0 to max NR I am happy to pass this
>> message to PM team and we should extend any SW to do translation between.
>
> When we talk about IDs and bindings, we mean IDs meaningful to Linux.
> Whatever is ignored by Linux and passed to anyone else - hardware or
> firmware - is not a ID anymore from bindings point of view. It's just
> some value.
Please correct me if I am wrong. Description about ID should be removed from
commit message because it is not necessary. And
include/dt-bindings/reset/xlnx-versal-net-resets.h
should be added when we merge also DT for versal-net SOC.
Thanks,
Michal
On 18/07/2023 16:01, Michal Simek wrote:
>
>
> On 7/18/23 15:20, Krzysztof Kozlowski wrote:
>> On 18/07/2023 15:11, Michal Simek wrote:
>>>>>
>>>>> That numbers in DT are virtual no matter if you use ID from 0 to max or random
>>>>> values it is up to code to handle them. Checking nr_pins against ID is done in
>>>>> core but it is up to drivers.
>>>>
>>>> No, you confuse "virtual" and "ID". IDs are not virtual. IDs are real
>>>> and have representation in Linux driver. You do not need to define
>>>> anything virtual in the bindings.
>>>
>>> Not sure how you define ID itself. But HW doesn't know ID. HW knows only
>>> register which you can use to perform the reset. It is not really 128bit
>>> register where every bit targets to different IP.
>>>
>>> And this is SW-firmware interface like SCMI reset driver.
>>>
>>> Firmware is saying that ID 0 is QSPI, ID 1 is MMC.
>>> Their Linux driver is asking for nr_reset via firmware call which can be
>>> different for different SOC and that's fine and I have no problem with it.
>>> But only SCMI server is dictating that ID 0 is QSPI and ID 1 is MMC. Different
>>> SCMI server implementation can map it differently.
>>
>> Sure, and all this points to: no need for bindings.
>>
>>>
>>>
>>>>> In our case that IDs are coming from firmware and driver itself is just matching
>>>>> them.
>>>>
>>>> So they are the same as if coming from hardware - no need for IDs.
>>>
>>> It is hard to say what hardware here exactly is. From my perspective and I am
>>> not advocating not using IDs from 0 to max, it is just a number.
>>>
>>> If my firmware knows that QSPI reset is 0xc10402dU then I will just pass it to
>>> reach my goal which is reset QSPI IP.
>>>
>>> If you think that we should use IDs from 0 to max NR I am happy to pass this
>>> message to PM team and we should extend any SW to do translation between.
>>
>> When we talk about IDs and bindings, we mean IDs meaningful to Linux.
>> Whatever is ignored by Linux and passed to anyone else - hardware or
>> firmware - is not a ID anymore from bindings point of view. It's just
>> some value.
>
> Please correct me if I am wrong. Description about ID should be removed from
> commit message because it is not necessary. And
> include/dt-bindings/reset/xlnx-versal-net-resets.h
> should be added when we merge also DT for versal-net SOC.
No, the binding header is needed only if driver is using it. Adding DTS
will not change that - still no kernel driver users. No benefits of such
binding. If there are no users and no benefits - don't make it a binding.
Best regards,
Krzysztof
On 7/18/23 15:21, Krzysztof Kozlowski wrote:
> On 18/07/2023 15:20, Krzysztof Kozlowski wrote:
>> On 18/07/2023 15:11, Michal Simek wrote:
>>>>>
>>>>> That numbers in DT are virtual no matter if you use ID from 0 to max or random
>>>>> values it is up to code to handle them. Checking nr_pins against ID is done in
>>>>> core but it is up to drivers.
>>>>
>>>> No, you confuse "virtual" and "ID". IDs are not virtual. IDs are real
>>>> and have representation in Linux driver. You do not need to define
>>>> anything virtual in the bindings.
>>>
>>> Not sure how you define ID itself. But HW doesn't know ID. HW knows only
>>> register which you can use to perform the reset. It is not really 128bit
>>> register where every bit targets to different IP.
>>>
>>> And this is SW-firmware interface like SCMI reset driver.
>>>
>>> Firmware is saying that ID 0 is QSPI, ID 1 is MMC.
>>> Their Linux driver is asking for nr_reset via firmware call which can be
>>> different for different SOC and that's fine and I have no problem with it.
>>> But only SCMI server is dictating that ID 0 is QSPI and ID 1 is MMC. Different
>>> SCMI server implementation can map it differently.
>>
>> Sure, and all this points to: no need for bindings.
>>
>>>
>>>
>>>>> In our case that IDs are coming from firmware and driver itself is just matching
>>>>> them.
>>>>
>>>> So they are the same as if coming from hardware - no need for IDs.
>>>
>>> It is hard to say what hardware here exactly is. From my perspective and I am
>>> not advocating not using IDs from 0 to max, it is just a number.
>>>
>>> If my firmware knows that QSPI reset is 0xc10402dU then I will just pass it to
>>> reach my goal which is reset QSPI IP.
>>>
>>> If you think that we should use IDs from 0 to max NR I am happy to pass this
>>> message to PM team and we should extend any SW to do translation between.
>>
>> When we talk about IDs and bindings, we mean IDs meaningful to Linux.
>> Whatever is ignored by Linux and passed to anyone else - hardware or
>> firmware - is not a ID anymore from bindings point of view. It's just
>> some value.
>
> And just some proofs:
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/include/dt-bindings/reset/xlnx-versal-resets.h?h=v6.5-rc2
> $ git grep VERSAL_RST
> Results: No users.
I know this. I should likely submit one DT for Versal to start to use it.
Thanks,
Michal
On 7/18/23 16:04, Krzysztof Kozlowski wrote:
> On 18/07/2023 16:01, Michal Simek wrote:
>>
>>
>> On 7/18/23 15:20, Krzysztof Kozlowski wrote:
>>> On 18/07/2023 15:11, Michal Simek wrote:
>>>>>>
>>>>>> That numbers in DT are virtual no matter if you use ID from 0 to max or random
>>>>>> values it is up to code to handle them. Checking nr_pins against ID is done in
>>>>>> core but it is up to drivers.
>>>>>
>>>>> No, you confuse "virtual" and "ID". IDs are not virtual. IDs are real
>>>>> and have representation in Linux driver. You do not need to define
>>>>> anything virtual in the bindings.
>>>>
>>>> Not sure how you define ID itself. But HW doesn't know ID. HW knows only
>>>> register which you can use to perform the reset. It is not really 128bit
>>>> register where every bit targets to different IP.
>>>>
>>>> And this is SW-firmware interface like SCMI reset driver.
>>>>
>>>> Firmware is saying that ID 0 is QSPI, ID 1 is MMC.
>>>> Their Linux driver is asking for nr_reset via firmware call which can be
>>>> different for different SOC and that's fine and I have no problem with it.
>>>> But only SCMI server is dictating that ID 0 is QSPI and ID 1 is MMC. Different
>>>> SCMI server implementation can map it differently.
>>>
>>> Sure, and all this points to: no need for bindings.
>>>
>>>>
>>>>
>>>>>> In our case that IDs are coming from firmware and driver itself is just matching
>>>>>> them.
>>>>>
>>>>> So they are the same as if coming from hardware - no need for IDs.
>>>>
>>>> It is hard to say what hardware here exactly is. From my perspective and I am
>>>> not advocating not using IDs from 0 to max, it is just a number.
>>>>
>>>> If my firmware knows that QSPI reset is 0xc10402dU then I will just pass it to
>>>> reach my goal which is reset QSPI IP.
>>>>
>>>> If you think that we should use IDs from 0 to max NR I am happy to pass this
>>>> message to PM team and we should extend any SW to do translation between.
>>>
>>> When we talk about IDs and bindings, we mean IDs meaningful to Linux.
>>> Whatever is ignored by Linux and passed to anyone else - hardware or
>>> firmware - is not a ID anymore from bindings point of view. It's just
>>> some value.
>>
>> Please correct me if I am wrong. Description about ID should be removed from
>> commit message because it is not necessary. And
>> include/dt-bindings/reset/xlnx-versal-net-resets.h
>> should be added when we merge also DT for versal-net SOC.
>
> No, the binding header is needed only if driver is using it. Adding DTS
> will not change that - still no kernel driver users. No benefits of such
> binding. If there are no users and no benefits - don't make it a binding.
But header in board folder should be fine like these.
arch/arm64/boot/dts/mediatek/mt2712-pinfunc.h
arch/arm64/boot/dts/mediatek/mt8516-pinfunc.h
arch/arm64/boot/dts/mediatek/mt8167-pinfunc.h
arch/arm64/boot/dts/mediatek/mt8173-pinfunc.h
arch/arm64/boot/dts/exynos/exynos-pinctrl.h
arch/arm64/boot/dts/freescale/imx8mq-pinfunc.h
arch/arm64/boot/dts/freescale/imx8mp-pinfunc.h
arch/arm64/boot/dts/freescale/imx8mn-pinfunc.h
arch/arm64/boot/dts/freescale/imx93-pinfunc.h
arch/arm64/boot/dts/freescale/imx8ulp-pinfunc.h
arch/arm64/boot/dts/freescale/imx8mm-pinfunc.h
arch/arm64/boot/dts/tesla/fsd-pinctrl.h
Thanks,
Michal
On 18/07/2023 16:30, Michal Simek wrote:
>>>
>>> Please correct me if I am wrong. Description about ID should be removed from
>>> commit message because it is not necessary. And
>>> include/dt-bindings/reset/xlnx-versal-net-resets.h
>>> should be added when we merge also DT for versal-net SOC.
>>
>> No, the binding header is needed only if driver is using it. Adding DTS
>> will not change that - still no kernel driver users. No benefits of such
>> binding. If there are no users and no benefits - don't make it a binding.
>
> But header in board folder should be fine like these.
> arch/arm64/boot/dts/mediatek/mt2712-pinfunc.h
> arch/arm64/boot/dts/mediatek/mt8516-pinfunc.h
> arch/arm64/boot/dts/mediatek/mt8167-pinfunc.h
> arch/arm64/boot/dts/mediatek/mt8173-pinfunc.h
> arch/arm64/boot/dts/exynos/exynos-pinctrl.h
> arch/arm64/boot/dts/freescale/imx8mq-pinfunc.h
> arch/arm64/boot/dts/freescale/imx8mp-pinfunc.h
> arch/arm64/boot/dts/freescale/imx8mn-pinfunc.h
> arch/arm64/boot/dts/freescale/imx93-pinfunc.h
> arch/arm64/boot/dts/freescale/imx8ulp-pinfunc.h
> arch/arm64/boot/dts/freescale/imx8mm-pinfunc.h
> arch/arm64/boot/dts/tesla/fsd-pinctrl.h
Yes. If you want to store some constants (register values, firmware
magic numbers) and use in DTS, this is the way to go. Most (or all) of
examples above are for register values.
Best regards,
Krzysztof
On 7/18/23 20:01, Krzysztof Kozlowski wrote:
> On 18/07/2023 16:30, Michal Simek wrote:
>>>>
>>>> Please correct me if I am wrong. Description about ID should be removed from
>>>> commit message because it is not necessary. And
>>>> include/dt-bindings/reset/xlnx-versal-net-resets.h
>>>> should be added when we merge also DT for versal-net SOC.
>>>
>>> No, the binding header is needed only if driver is using it. Adding DTS
>>> will not change that - still no kernel driver users. No benefits of such
>>> binding. If there are no users and no benefits - don't make it a binding.
>>
>> But header in board folder should be fine like these.
>> arch/arm64/boot/dts/mediatek/mt2712-pinfunc.h
>> arch/arm64/boot/dts/mediatek/mt8516-pinfunc.h
>> arch/arm64/boot/dts/mediatek/mt8167-pinfunc.h
>> arch/arm64/boot/dts/mediatek/mt8173-pinfunc.h
>> arch/arm64/boot/dts/exynos/exynos-pinctrl.h
>> arch/arm64/boot/dts/freescale/imx8mq-pinfunc.h
>> arch/arm64/boot/dts/freescale/imx8mp-pinfunc.h
>> arch/arm64/boot/dts/freescale/imx8mn-pinfunc.h
>> arch/arm64/boot/dts/freescale/imx93-pinfunc.h
>> arch/arm64/boot/dts/freescale/imx8ulp-pinfunc.h
>> arch/arm64/boot/dts/freescale/imx8mm-pinfunc.h
>> arch/arm64/boot/dts/tesla/fsd-pinctrl.h
>
> Yes. If you want to store some constants (register values, firmware
> magic numbers) and use in DTS, this is the way to go. Most (or all) of
> examples above are for register values.
I did small grepping over Linux (reset only) and I found that all of these files
are not used by any driver/code. They are included in binding document or dt files.
Based on your description above they all are candidates for removing from
include/dt-bindings/reset/.
On the other hand that files could be used in different projects out of Linux
where that values could be used by a driver/code.
What to do with it? Should we remove it, deprecate it or just keep it and not to
add new one? I just want to know how to properly handle it.
Thanks,
Michal
altr,rst-mgr-a10.h
altr,rst-mgr.h
altr,rst-mgr-s10.h
amlogic,meson8b-reset.h
amlogic,meson-a1-reset.h
amlogic,meson-axg-reset.h
amlogic,meson-g12a-audio-reset.h
amlogic,meson-g12a-reset.h
amlogic,meson-gxbb-reset.h
amlogic,meson-s4-reset.h
bcm6318-reset.h
bcm63268-reset.h
bcm6328-reset.h
bcm6358-reset.h
bcm6362-reset.h
bcm6368-reset.h
bitmain,bm1880-reset.h
hisi,hi6220-resets.h
imx8ulp-pcc-reset.h
mediatek,mt6735-wdt.h
mt2701-resets.h
mt7622-reset.h
mt7629-resets.h
mt8135-resets.h
mt8173-resets.h
nuvoton,npcm7xx-reset.h
oxsemi,ox810se.h
oxsemi,ox820.h
realtek,rtd1195.h
realtek,rtd1295.h
snps,hsdk-reset.h
stericsson,db8500-prcc-reset.h
stm32mp13-resets.h
stm32mp1-resets.h
sunplus,sp7021-reset.h
tegra186-reset.h
tegra194-reset.h
tegra234-reset.h
xlnx-versal-resets.h
xlnx-zynqmp-resets.h
On 19/07/2023 08:23, Michal Simek wrote:
>> Yes. If you want to store some constants (register values, firmware
>> magic numbers) and use in DTS, this is the way to go. Most (or all) of
>> examples above are for register values.
>
> I did small grepping over Linux (reset only) and I found that all of these files
> are not used by any driver/code. They are included in binding document or dt files.
> Based on your description above they all are candidates for removing from
> include/dt-bindings/reset/.
> On the other hand that files could be used in different projects out of Linux
> where that values could be used by a driver/code.
Yes, therefore this should be case-by-case decision.
>
> What to do with it? Should we remove it, deprecate it or just keep it and not to
> add new one? I just want to know how to properly handle it.
They cannot be removed. They could be copied to DTS and deprecated in
the bindings. But it is not that important that we clean it up... or it
is rather to the platform maintainer. I did it some time ago for Samsung
and recently TI is doing for serdes mux bindings.
Best regards,
Krzysztof