diff mbox series

[v2,4/7] dt-bindings: clock: Add binding constants for BLZP1600

Message ID 20240424133241.19210-1-nikolaos.pasaloukos@blaize.com (mailing list archive)
State New, archived
Headers show
Series Add support for Blaize BLZP1600 SoC | expand

Commit Message

Nikolaos Pasaloukos April 24, 2024, 1:32 p.m. UTC
Add SCMI clock numbers according to the Blaize BLZP1600
SoC hardware specifications.

Reviewed-by: James Cowgill <james.cowgill@blaize.com>
Reviewed-by: Matt Redfearn <matt.redfearn@blaize.com>
Reviewed-by: Neil Jones <neil.jones@blaize.com>
Signed-off-by: Nikolaos Pasaloukos <nikolaos.pasaloukos@blaize.com>
---
 .../dt-bindings/clock/blaize,blzp1600-clk.h   | 77 +++++++++++++++++++
 1 file changed, 77 insertions(+)
 create mode 100644 include/dt-bindings/clock/blaize,blzp1600-clk.h

Comments

Krzysztof Kozlowski April 24, 2024, 2:13 p.m. UTC | #1
On 24/04/2024 15:32, Niko Pasaloukos wrote:
> Add SCMI clock numbers according to the Blaize BLZP1600
> SoC hardware specifications.
> 
> Reviewed-by: James Cowgill <james.cowgill@blaize.com>
> Reviewed-by: Matt Redfearn <matt.redfearn@blaize.com>
> Reviewed-by: Neil Jones <neil.jones@blaize.com>
> Signed-off-by: Nikolaos Pasaloukos <nikolaos.pasaloukos@blaize.com>

This goes to the patch introducing binding doc.

> ---
>  .../dt-bindings/clock/blaize,blzp1600-clk.h   | 77 +++++++++++++++++++
>  1 file changed, 77 insertions(+)
>  create mode 100644 include/dt-bindings/clock/blaize,blzp1600-clk.h
> 
> diff --git a/include/dt-bindings/clock/blaize,blzp1600-clk.h b/include/dt-bindings/clock/blaize,blzp1600-clk.h
> new file mode 100644
> index 000000000000..f1d59849a6e5
> --- /dev/null
> +++ b/include/dt-bindings/clock/blaize,blzp1600-clk.h
> @@ -0,0 +1,77 @@
> +/* SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause */
> +/*
> + * Copyright (C) 2023, Blaize, Inc.
> + */
> +
> +#ifndef DT_BINDING_CLK_BLZP1600_H
> +#define DT_BINDING_CLK_BLZP1600_H
> +
> +/* ARM SCMI clocks */
> +
> +/* BLZP1600 clock-gate numbers as defined in the hardware architecture */
> +
> +#define BLZP1600_CPU_CLK 0
> +/* clock-gates 1-6 invalid */

No, they cannot be invalid. IDs start from 0 and are incremented by one.
If you have holes, it is not a binding.

Drop the header or use it properly, so as virtual IDs.

Best regards,
Krzysztof
Nikolaos Pasaloukos April 24, 2024, 4:14 p.m. UTC | #2
On 24/04/2024 15:13, Krzysztof Kozlowski wrote:
> On 24/04/2024 15:32, Niko Pasaloukos wrote:
>> Add SCMI clock numbers according to the Blaize BLZP1600
>> SoC hardware specifications.
>>
>> Reviewed-by: James Cowgill <james.cowgill@blaize.com>
>> Reviewed-by: Matt Redfearn <matt.redfearn@blaize.com>
>> Reviewed-by: Neil Jones <neil.jones@blaize.com>
>> Signed-off-by: Nikolaos Pasaloukos <nikolaos.pasaloukos@blaize.com>
> This goes to the patch introducing binding doc.

Apologies for the broken threading.

>
>> ---
>>  .../dt-bindings/clock/blaize,blzp1600-clk.h   | 77 +++++++++++++++++++
>>  1 file changed, 77 insertions(+)
>>  create mode 100644 include/dt-bindings/clock/blaize,blzp1600-clk.h
>>
>> diff --git a/include/dt-bindings/clock/blaize,blzp1600-clk.h b/include/dt-bindings/clock/blaize,blzp1600-clk.h
>> new file mode 100644
>> index 000000000000..f1d59849a6e5
>> --- /dev/null
>> +++ b/include/dt-bindings/clock/blaize,blzp1600-clk.h
>> @@ -0,0 +1,77 @@
>> +/* SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause */
>> +/*
>> + * Copyright (C) 2023, Blaize, Inc.
>> + */
>> +
>> +#ifndef DT_BINDING_CLK_BLZP1600_H
>> +#define DT_BINDING_CLK_BLZP1600_H
>> +
>> +/* ARM SCMI clocks */
>> +
>> +/* BLZP1600 clock-gate numbers as defined in the hardware architecture */
>> +
>> +#define BLZP1600_CPU_CLK 0
>> +/* clock-gates 1-6 invalid */
> No, they cannot be invalid. IDs start from 0 and are incremented by one.
> If you have holes, it is not a binding.
>
> Drop the header or use it properly, so as virtual IDs.
>
> Best regards,
> Krzysztof
>
My intention was to avoid using magic numbers on the DeviceTree. That's why I added them here.
Also, we have some custom drivers which we plan to upload and their schemas need those files.
The alternative would be to use magic numbers for our clocks and resets.

In the commit message and on the header file I have mentioned that these are numbers matching
the hardware specification (1 to 1) of the chip not just enums.
Some IDs are invalid because of a hardware gap, some others are invalid because the SCMI
service will return an error that the number is invalid.

Is there another way to prevent the magic numbers in the schemas and device-tree.

Thank you very much for your fast and detailed review.

Best regards,
Niko
Krzysztof Kozlowski April 25, 2024, 6:56 a.m. UTC | #3
On 24/04/2024 18:14, Nikolaos Pasaloukos wrote:
>> No, they cannot be invalid. IDs start from 0 and are incremented by one.
>> If you have holes, it is not a binding.
>>
>> Drop the header or use it properly, so as virtual IDs.
>>
>> Best regards,
>> Krzysztof
>>
> My intention was to avoid using magic numbers on the DeviceTree. That's why I added them here.
> Also, we have some custom drivers which we plan to upload and their schemas need those files.
> The alternative would be to use magic numbers for our clocks and resets.
> 
> In the commit message and on the header file I have mentioned that these are numbers matching
> the hardware specification (1 to 1) of the chip not just enums.
> Some IDs are invalid because of a hardware gap, some others are invalid because the SCMI
> service will return an error that the number is invalid.
> 
> Is there another way to prevent the magic numbers in the schemas and device-tree.
> 
> Thank you very much for your fast and detailed review.

Bindings describe the interface between DTS and drivers (OS or some sort
of other software). The purpose of binding headers is to document the
constants which are used by both, because they are part of that
interface. Therefore constants are pure abstraction.

Let me rephrase the question: Why you do not have headers for interrupt
numbers? All addresses? GPIO pin numbers?

Best regards,
Krzysztof
Nikolaos Pasaloukos April 25, 2024, 7:18 a.m. UTC | #4
On 25/04/2024 07:56, Krzysztof Kozlowski wrote:
> On 24/04/2024 18:14, Nikolaos Pasaloukos wrote:
>>> No, they cannot be invalid. IDs start from 0 and are incremented by one.
>>> If you have holes, it is not a binding.
>>>
>>> Drop the header or use it properly, so as virtual IDs.
>>>
>>> Best regards,
>>> Krzysztof
>>>
>> My intention was to avoid using magic numbers on the DeviceTree. That's why I added them here.
>> Also, we have some custom drivers which we plan to upload and their schemas need those files.
>> The alternative would be to use magic numbers for our clocks and resets.
>>
>> In the commit message and on the header file I have mentioned that these are numbers matching
>> the hardware specification (1 to 1) of the chip not just enums.
>> Some IDs are invalid because of a hardware gap, some others are invalid because the SCMI
>> service will return an error that the number is invalid.
>>
>> Is there another way to prevent the magic numbers in the schemas and device-tree.
>>
>> Thank you very much for your fast and detailed review.
> Bindings describe the interface between DTS and drivers (OS or some sort
> of other software). The purpose of binding headers is to document the
> constants which are used by both, because they are part of that
> interface. Therefore constants are pure abstraction.
>
> Let me rephrase the question: Why you do not have headers for interrupt
> numbers? All addresses? GPIO pin numbers?
>
> Best regards,
> Krzysztof
>
Thank you very much for your feedback Krzysztof, I'll prepare a v3 with proper
threading this time, removing the dt-bindings for the clock & reset.

Best regards,
Niko
Krzysztof Kozlowski April 25, 2024, 7:25 a.m. UTC | #5
On 25/04/2024 09:18, Nikolaos Pasaloukos wrote:
>>
>> Let me rephrase the question: Why you do not have headers for interrupt
>> numbers? All addresses? GPIO pin numbers?
>>
>> Best regards,
>> Krzysztof
>>
> Thank you very much for your feedback Krzysztof, I'll prepare a v3 with proper
> threading this time, removing the dt-bindings for the clock & reset.

BTW, this is purely about bindings. I don't oppose DTS headers to avoid
certain magic numbers, like we do for several platforms already.

Best regards,
Krzysztof
diff mbox series

Patch

diff --git a/include/dt-bindings/clock/blaize,blzp1600-clk.h b/include/dt-bindings/clock/blaize,blzp1600-clk.h
new file mode 100644
index 000000000000..f1d59849a6e5
--- /dev/null
+++ b/include/dt-bindings/clock/blaize,blzp1600-clk.h
@@ -0,0 +1,77 @@ 
+/* SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause */
+/*
+ * Copyright (C) 2023, Blaize, Inc.
+ */
+
+#ifndef DT_BINDING_CLK_BLZP1600_H
+#define DT_BINDING_CLK_BLZP1600_H
+
+/* ARM SCMI clocks */
+
+/* BLZP1600 clock-gate numbers as defined in the hardware architecture */
+
+#define BLZP1600_CPU_CLK 0
+/* clock-gates 1-6 invalid */
+#define BLZP1600_CRYPTO_CLK 7
+/* clock-gate 8 invalid */
+#define BLZP1600_GSP_CLK 9
+/* clock-gates 10-23 invalid */
+#define BLZP1600_USB_CLK 24
+#define BLZP1600_USB_PHY_CLK 25
+#define BLZP1600_CAN0_CLK 26
+#define BLZP1600_CAN1_CLK 27
+#define BLZP1600_CAN2_CLK 28
+#define BLZP1600_ETH_MAC_CLK 29
+#define BLZP1600_SDIO0_CLK 30
+#define BLZP1600_SDIO1_CLK 31
+#define BLZP1600_SDIO2_CLK 32
+/* clock-gate 33 invalid */
+#define BLZP1600_SD_CARD_CLK 34
+#define BLZP1600_CSI0_CTRL_CLK 35
+#define BLZP1600_CSI0_VDMA_CLK 36
+#define BLZP1600_CSI1_CTRL_CLK 37
+#define BLZP1600_CSI1_VDMA_CLK 38
+#define BLZP1600_CSI2_CTRL_CLK 39
+#define BLZP1600_CSI2_VDMA_CLK 40
+#define BLZP1600_CSI3_CTRL_CLK 41
+#define BLZP1600_CSI3_VDMA_CLK 42
+#define BLZP1600_CSID_CTRL_CLK 43
+#define BLZP1600_CSID_VDMA_CLK 44
+#define BLZP1600_DSI_CTRL_CLK 45
+#define BLZP1600_DSI_VDMA_CLK 46
+/* clock-gate 47 invalid */
+#define BLZP1600_I2S_CODEC_CLK 48
+#define BLZP1600_DMA_CLK 49
+#define BLZP1600_QSPI_PCLK 50
+#define BLZP1600_QSPI_CLK 51
+#define BLZP1600_I2S_TX_CLK 52
+#define BLZP1600_I2S_RX_CLK 53
+#define BLZP1600_I2C0_CLK 54
+#define BLZP1600_I2C1_CLK 55
+#define BLZP1600_I2C2_CLK 56
+#define BLZP1600_I2C3_CLK 57
+#define BLZP1600_I2C4_CLK 58
+#define BLZP1600_UART0_CLK 59
+#define BLZP1600_UART1_CLK 60
+#define BLZP1600_SPIS_PCLK 61
+#define BLZP1600_SPIS_CLK 62
+/* clock-gate 63 invalid */
+#define BLZP1600_TSENSOR_CLK 64
+#define BLZP1600_VIDEO_E_CLK 65
+/* clock-gates 66-67 invalid */
+#define BLZP1600_VIDEO_D_CLK 68
+/* Special clock-gates */
+#define BLZP1600_NIC_CLK 69
+#define BLZP1600_NIC_HALF_CLK 70
+#define BLZP1600_ETH_MAC_M_CLK 71
+#define BLZP1600_I2S_MASTER_CLK 72
+/* Clock sources */
+#define BLZP1600_SRC_XTAL_CLK 100
+#define BLZP1600_SRC_PLL0_CLK 101
+#define BLZP1600_SRC_PLL1_CLK 102
+#define BLZP1600_SRC_PLL2_CLK 103
+#define BLZP1600_SRC_I2S_CLK 104
+#define BLZP1600_SRC_CSID_CLK 105
+#define BLZP1600_SRC_DSI_CLK 106
+
+#endif