* [PATCH 0/4] riscv: sophgo: add reset support for cv1800b
@ 2023-11-13 0:54 Jisheng Zhang
2023-11-13 0:55 ` [PATCH 1/4] dt-bindings: reset: Add binding for Sophgo CV1800B reset controller Jisheng Zhang
` (3 more replies)
0 siblings, 4 replies; 21+ messages in thread
From: Jisheng Zhang @ 2023-11-13 0:54 UTC (permalink / raw)
To: Philipp Zabel, Rob Herring, Krzysztof Kozlowski, Conor Dooley,
Paul Walmsley, Palmer Dabbelt, Albert Ou, Chao Wei, Chen Wang
Cc: devicetree, linux-kernel, linux-riscv
This series adds reset controller support for sophgo cv1800b using
reset-simple driver.
Jisheng Zhang (4):
dt-bindings: reset: Add binding for Sophgo CV1800B reset controller
reset: Add reset controller support for Sophgo CV1800B SoC
riscv: dts: sophgo: add reset dt node for cv1800b
riscv: dts: sophgo: add reset phandle to all uart nodes
.../bindings/reset/sophgo,cv1800b-reset.yaml | 38 ++++++++
arch/riscv/boot/dts/sophgo/cv1800b.dtsi | 12 +++
drivers/reset/Kconfig | 3 +-
drivers/reset/reset-simple.c | 2 +
.../dt-bindings/reset/sophgo,cv1800b-reset.h | 96 +++++++++++++++++++
5 files changed, 150 insertions(+), 1 deletion(-)
create mode 100644 Documentation/devicetree/bindings/reset/sophgo,cv1800b-reset.yaml
create mode 100644 include/dt-bindings/reset/sophgo,cv1800b-reset.h
--
2.42.0
^ permalink raw reply [flat|nested] 21+ messages in thread
* [PATCH 1/4] dt-bindings: reset: Add binding for Sophgo CV1800B reset controller
2023-11-13 0:54 [PATCH 0/4] riscv: sophgo: add reset support for cv1800b Jisheng Zhang
@ 2023-11-13 0:55 ` Jisheng Zhang
2023-11-13 13:36 ` Conor Dooley
2023-11-14 21:12 ` Krzysztof Kozlowski
2023-11-13 0:55 ` [PATCH 2/4] reset: Add reset controller support for Sophgo CV1800B SoC Jisheng Zhang
` (2 subsequent siblings)
3 siblings, 2 replies; 21+ messages in thread
From: Jisheng Zhang @ 2023-11-13 0:55 UTC (permalink / raw)
To: Philipp Zabel, Rob Herring, Krzysztof Kozlowski, Conor Dooley,
Paul Walmsley, Palmer Dabbelt, Albert Ou, Chao Wei, Chen Wang
Cc: devicetree, linux-kernel, linux-riscv
Add devicetree binding for Sophgo CV1800B SoC reset controller.
Signed-off-by: Jisheng Zhang <jszhang@kernel.org>
---
.../bindings/reset/sophgo,cv1800b-reset.yaml | 38 ++++++++
.../dt-bindings/reset/sophgo,cv1800b-reset.h | 96 +++++++++++++++++++
2 files changed, 134 insertions(+)
create mode 100644 Documentation/devicetree/bindings/reset/sophgo,cv1800b-reset.yaml
create mode 100644 include/dt-bindings/reset/sophgo,cv1800b-reset.h
diff --git a/Documentation/devicetree/bindings/reset/sophgo,cv1800b-reset.yaml b/Documentation/devicetree/bindings/reset/sophgo,cv1800b-reset.yaml
new file mode 100644
index 000000000000..20a525147490
--- /dev/null
+++ b/Documentation/devicetree/bindings/reset/sophgo,cv1800b-reset.yaml
@@ -0,0 +1,38 @@
+# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/reset/sophgo,cv1800b-reset.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Sophgo CV1800B SoC Reset Controller
+
+maintainers:
+ - Jisheng Zhang <jszhang@kernel.org>
+
+properties:
+ compatible:
+ enum:
+ - sophgo,cv1800b-reset
+
+ reg:
+ maxItems: 1
+
+ "#reset-cells":
+ const: 1
+
+required:
+ - compatible
+ - reg
+ - "#reset-cells"
+
+additionalProperties: false
+
+examples:
+ - |
+ reset-controller@3003000 {
+ compatible = "sophgo,cv1800b-reset";
+ reg = <0x03003000 0x1000>;
+ #reset-cells = <1>;
+ };
+
+...
diff --git a/include/dt-bindings/reset/sophgo,cv1800b-reset.h b/include/dt-bindings/reset/sophgo,cv1800b-reset.h
new file mode 100644
index 000000000000..28dda71369b4
--- /dev/null
+++ b/include/dt-bindings/reset/sophgo,cv1800b-reset.h
@@ -0,0 +1,96 @@
+/* SPDX-License-Identifier: GPL-2.0 OR MIT */
+/*
+ * Copyright (C) 2023 Sophgo Technology Inc. All rights reserved.
+ * Copyright (C) 2023 Jisheng Zhang <jszhang@kernel.org>
+ */
+
+#ifndef _DT_BINDINGS_CV1800B_RESET_H
+#define _DT_BINDINGS_CV1800B_RESET_H
+
+/* 0-1 */
+#define RST_DDR 2
+#define RST_H264C 3
+#define RST_JPEG 4
+#define RST_H265C 5
+#define RST_VIPSYS 6
+#define RST_TDMA 7
+#define RST_TPU 8
+#define RST_TPUSYS 9
+/* 10 */
+#define RST_USB 11
+#define RST_ETH0 12
+/* 13 */
+#define RST_NAND 14
+/* 15 */
+#define RST_SD0 16
+/* 17 */
+#define RST_SDMA 18
+#define RST_I2S0 19
+#define RST_I2S1 20
+#define RST_I2S2 21
+#define RST_I2S3 22
+#define RST_UART0 23
+#define RST_UART1 24
+#define RST_UART2 25
+#define RST_UART3 26
+#define RST_I2C0 27
+#define RST_I2C1 28
+#define RST_I2C2 29
+#define RST_I2C3 30
+#define RST_I2C4 31
+#define RST_PWM0 32
+#define RST_PWM1 33
+#define RST_PWM2 34
+#define RST_PWM3 35
+/* 36-39 */
+#define RST_SPI0 40
+#define RST_SPI1 41
+#define RST_SPI2 42
+#define RST_SPI3 43
+#define RST_GPIO0 44
+#define RST_GPIO1 45
+#define RST_GPIO2 46
+#define RST_EFUSE 47
+#define RST_WDT 48
+#define RST_AHBRST_ROM 49
+#define RST_SPIC 50
+#define RST_TEMPSEN 51
+#define RST_SARADC 52
+/* 53-57 */
+#define RST_COMBORST_PHY0 58
+/* 59-60 */
+#define RST_SPIRST_NAND 61
+#define RST_SE 62
+/* 63-73 */
+#define RST_UART4 74
+#define RST_GPIO3 75
+#define RST_SYSTEM 76
+#define RST_TIMER 77
+#define RST_TIMER0 78
+#define RST_TIMER1 79
+#define RST_TIMER2 80
+#define RST_TIMER3 81
+#define RST_TIMER4 82
+#define RST_TIMER5 83
+#define RST_TIMER6 84
+#define RST_TIMER7 85
+#define RST_WGN0 86
+#define RST_WGN1 87
+#define RST_WGN2 88
+#define RST_KEYSCAN 89
+/* 90 */
+#define RST_AUDDAC 91
+#define RST_AUDDACRST_APB 92
+#define RST_AUDADC 93
+/* 94 */
+#define RST_VCSYS 95
+#define RST_ETHPHY 96
+#define RST_ETHPHYRST_APB 97
+#define RST_AUDSRC 98
+#define RST_VIP_CAM0 99
+#define RST_WDT1 100
+#define RST_WDT2 101
+/* 102-292 */
+#define RST_CPUSYS1 293
+#define RST_CPUSYS2 294
+
+#endif /* _DT_BINDINGS_CV1800B_RESET_H */
--
2.42.0
^ permalink raw reply related [flat|nested] 21+ messages in thread
* [PATCH 2/4] reset: Add reset controller support for Sophgo CV1800B SoC
2023-11-13 0:54 [PATCH 0/4] riscv: sophgo: add reset support for cv1800b Jisheng Zhang
2023-11-13 0:55 ` [PATCH 1/4] dt-bindings: reset: Add binding for Sophgo CV1800B reset controller Jisheng Zhang
@ 2023-11-13 0:55 ` Jisheng Zhang
2023-11-13 0:55 ` [PATCH 3/4] riscv: dts: sophgo: add reset dt node for cv1800b Jisheng Zhang
2023-11-13 0:55 ` [PATCH 4/4] riscv: dts: sophgo: add reset phandle to all uart nodes Jisheng Zhang
3 siblings, 0 replies; 21+ messages in thread
From: Jisheng Zhang @ 2023-11-13 0:55 UTC (permalink / raw)
To: Philipp Zabel, Rob Herring, Krzysztof Kozlowski, Conor Dooley,
Paul Walmsley, Palmer Dabbelt, Albert Ou, Chao Wei, Chen Wang
Cc: devicetree, linux-kernel, linux-riscv
Add reset controller support for Sophgo CV1800B SoC reusing the
reset-simple driver.
Signed-off-by: Jisheng Zhang <jszhang@kernel.org>
---
drivers/reset/Kconfig | 3 ++-
drivers/reset/reset-simple.c | 2 ++
2 files changed, 4 insertions(+), 1 deletion(-)
diff --git a/drivers/reset/Kconfig b/drivers/reset/Kconfig
index ccd59ddd7610..2034f69d5953 100644
--- a/drivers/reset/Kconfig
+++ b/drivers/reset/Kconfig
@@ -213,7 +213,7 @@ config RESET_SCMI
config RESET_SIMPLE
bool "Simple Reset Controller Driver" if COMPILE_TEST || EXPERT
- default ARCH_ASPEED || ARCH_BCMBCA || ARCH_BITMAIN || ARCH_REALTEK || ARCH_STM32 || (ARCH_INTEL_SOCFPGA && ARM64) || ARCH_SUNXI || ARC
+ default ARCH_ASPEED || ARCH_BCMBCA || ARCH_BITMAIN || ARCH_REALTEK || ARCH_SOPHGO || ARCH_STM32 || (ARCH_INTEL_SOCFPGA && ARM64) || ARCH_SUNXI || ARC
depends on HAS_IOMEM
help
This enables a simple reset controller driver for reset lines that
@@ -228,6 +228,7 @@ config RESET_SIMPLE
- RCC reset controller in STM32 MCUs
- Allwinner SoCs
- SiFive FU740 SoCs
+ - Sophgo SoCs
config RESET_SOCFPGA
bool "SoCFPGA Reset Driver" if COMPILE_TEST && (!ARM || !ARCH_INTEL_SOCFPGA)
diff --git a/drivers/reset/reset-simple.c b/drivers/reset/reset-simple.c
index 7ea5adbf2097..573753ae3e08 100644
--- a/drivers/reset/reset-simple.c
+++ b/drivers/reset/reset-simple.c
@@ -151,6 +151,8 @@ static const struct of_device_id reset_simple_dt_ids[] = {
{ .compatible = "snps,dw-high-reset" },
{ .compatible = "snps,dw-low-reset",
.data = &reset_simple_active_low },
+ { .compatible = "sophgo,cv1800b-reset",
+ .data = &reset_simple_active_low },
{ /* sentinel */ },
};
--
2.42.0
^ permalink raw reply related [flat|nested] 21+ messages in thread
* [PATCH 3/4] riscv: dts: sophgo: add reset dt node for cv1800b
2023-11-13 0:54 [PATCH 0/4] riscv: sophgo: add reset support for cv1800b Jisheng Zhang
2023-11-13 0:55 ` [PATCH 1/4] dt-bindings: reset: Add binding for Sophgo CV1800B reset controller Jisheng Zhang
2023-11-13 0:55 ` [PATCH 2/4] reset: Add reset controller support for Sophgo CV1800B SoC Jisheng Zhang
@ 2023-11-13 0:55 ` Jisheng Zhang
2023-11-13 14:32 ` Yixun Lan
2023-11-13 0:55 ` [PATCH 4/4] riscv: dts: sophgo: add reset phandle to all uart nodes Jisheng Zhang
3 siblings, 1 reply; 21+ messages in thread
From: Jisheng Zhang @ 2023-11-13 0:55 UTC (permalink / raw)
To: Philipp Zabel, Rob Herring, Krzysztof Kozlowski, Conor Dooley,
Paul Walmsley, Palmer Dabbelt, Albert Ou, Chao Wei, Chen Wang
Cc: devicetree, linux-kernel, linux-riscv
Add the reset device tree node to cv1800b SoC.
Signed-off-by: Jisheng Zhang <jszhang@kernel.org>
---
arch/riscv/boot/dts/sophgo/cv1800b.dtsi | 6 ++++++
1 file changed, 6 insertions(+)
diff --git a/arch/riscv/boot/dts/sophgo/cv1800b.dtsi b/arch/riscv/boot/dts/sophgo/cv1800b.dtsi
index df40e87ee063..4032419486be 100644
--- a/arch/riscv/boot/dts/sophgo/cv1800b.dtsi
+++ b/arch/riscv/boot/dts/sophgo/cv1800b.dtsi
@@ -54,6 +54,12 @@ soc {
dma-noncoherent;
ranges;
+ rst: reset-controller@3003000 {
+ compatible = "sophgo,cv1800b-reset";
+ reg = <0x03003000 0x1000>;
+ #reset-cells = <1>;
+ };
+
uart0: serial@4140000 {
compatible = "snps,dw-apb-uart";
reg = <0x04140000 0x100>;
--
2.42.0
^ permalink raw reply related [flat|nested] 21+ messages in thread
* [PATCH 4/4] riscv: dts: sophgo: add reset phandle to all uart nodes
2023-11-13 0:54 [PATCH 0/4] riscv: sophgo: add reset support for cv1800b Jisheng Zhang
` (2 preceding siblings ...)
2023-11-13 0:55 ` [PATCH 3/4] riscv: dts: sophgo: add reset dt node for cv1800b Jisheng Zhang
@ 2023-11-13 0:55 ` Jisheng Zhang
2023-11-13 2:04 ` Samuel Holland
2023-11-13 4:57 ` kernel test robot
3 siblings, 2 replies; 21+ messages in thread
From: Jisheng Zhang @ 2023-11-13 0:55 UTC (permalink / raw)
To: Philipp Zabel, Rob Herring, Krzysztof Kozlowski, Conor Dooley,
Paul Walmsley, Palmer Dabbelt, Albert Ou, Chao Wei, Chen Wang
Cc: devicetree, linux-kernel, linux-riscv
Although, the resets are deasserted by default. Add them for
completeness.
Signed-off-by: Jisheng Zhang <jszhang@kernel.org>
---
arch/riscv/boot/dts/sophgo/cv1800b.dtsi | 6 ++++++
1 file changed, 6 insertions(+)
diff --git a/arch/riscv/boot/dts/sophgo/cv1800b.dtsi b/arch/riscv/boot/dts/sophgo/cv1800b.dtsi
index 4032419486be..e04df04a91c0 100644
--- a/arch/riscv/boot/dts/sophgo/cv1800b.dtsi
+++ b/arch/riscv/boot/dts/sophgo/cv1800b.dtsi
@@ -4,6 +4,7 @@
*/
#include <dt-bindings/interrupt-controller/irq.h>
+#include <dt-bindings/reset/sophgo,cv1800b-reset.h>
/ {
compatible = "sophgo,cv1800b";
@@ -65,6 +66,7 @@ uart0: serial@4140000 {
reg = <0x04140000 0x100>;
interrupts = <44 IRQ_TYPE_LEVEL_HIGH>;
clocks = <&osc>;
+ resets = <&rst RST_UART0>;
reg-shift = <2>;
reg-io-width = <4>;
status = "disabled";
@@ -75,6 +77,7 @@ uart1: serial@4150000 {
reg = <0x04150000 0x100>;
interrupts = <45 IRQ_TYPE_LEVEL_HIGH>;
clocks = <&osc>;
+ resets = <&rst RST_UART1>;
reg-shift = <2>;
reg-io-width = <4>;
status = "disabled";
@@ -85,6 +88,7 @@ uart2: serial@4160000 {
reg = <0x04160000 0x100>;
interrupts = <46 IRQ_TYPE_LEVEL_HIGH>;
clocks = <&osc>;
+ resets = <&rst RST_UART2>;
reg-shift = <2>;
reg-io-width = <4>;
status = "disabled";
@@ -95,6 +99,7 @@ uart3: serial@4170000 {
reg = <0x04170000 0x100>;
interrupts = <47 IRQ_TYPE_LEVEL_HIGH>;
clocks = <&osc>;
+ resets = <&rst RST_UART3>;
reg-shift = <2>;
reg-io-width = <4>;
status = "disabled";
@@ -105,6 +110,7 @@ uart4: serial@41c0000 {
reg = <0x041c0000 0x100>;
interrupts = <48 IRQ_TYPE_LEVEL_HIGH>;
clocks = <&osc>;
+ resets = <&rst RST_UART4>;
reg-shift = <2>;
reg-io-width = <4>;
status = "disabled";
--
2.42.0
^ permalink raw reply related [flat|nested] 21+ messages in thread
* Re: [PATCH 4/4] riscv: dts: sophgo: add reset phandle to all uart nodes
2023-11-13 0:55 ` [PATCH 4/4] riscv: dts: sophgo: add reset phandle to all uart nodes Jisheng Zhang
@ 2023-11-13 2:04 ` Samuel Holland
2023-11-13 13:09 ` Jisheng Zhang
2023-11-13 4:57 ` kernel test robot
1 sibling, 1 reply; 21+ messages in thread
From: Samuel Holland @ 2023-11-13 2:04 UTC (permalink / raw)
To: Jisheng Zhang, Philipp Zabel, Rob Herring, Krzysztof Kozlowski,
Conor Dooley
Cc: devicetree, linux-kernel, linux-riscv, Paul Walmsley,
Palmer Dabbelt, Albert Ou, Chao Wei, Chen Wang
Hi Jisheng,
On 2023-11-12 6:55 PM, Jisheng Zhang wrote:
> Although, the resets are deasserted by default. Add them for
> completeness.
>
> Signed-off-by: Jisheng Zhang <jszhang@kernel.org>
> ---
> arch/riscv/boot/dts/sophgo/cv1800b.dtsi | 6 ++++++
> 1 file changed, 6 insertions(+)
>
> diff --git a/arch/riscv/boot/dts/sophgo/cv1800b.dtsi b/arch/riscv/boot/dts/sophgo/cv1800b.dtsi
> index 4032419486be..e04df04a91c0 100644
> --- a/arch/riscv/boot/dts/sophgo/cv1800b.dtsi
> +++ b/arch/riscv/boot/dts/sophgo/cv1800b.dtsi
> @@ -4,6 +4,7 @@
> */
>
> #include <dt-bindings/interrupt-controller/irq.h>
> +#include <dt-bindings/reset/sophgo,cv1800b-reset.h>
>
> / {
> compatible = "sophgo,cv1800b";
> @@ -65,6 +66,7 @@ uart0: serial@4140000 {
> reg = <0x04140000 0x100>;
> interrupts = <44 IRQ_TYPE_LEVEL_HIGH>;
> clocks = <&osc>;
> + resets = <&rst RST_UART0>;
Since it's not obvious: this breaks devicetree forward compatibility. An
existing kernel will fail the devm_reset_control_get_optional_exclusive() in
8250_dw.c because it has no driver for the reset controller.
This may not be a concern yet, since the devicetree is still "in development".
But it is something to keep in mind for the future. To avoid this sort of
problem, it's best to fully model the clocks/resets/other dependencies as early
as possible, and not rely on the firmware setting anything up.
Regards,
Samuel
> reg-shift = <2>;
> reg-io-width = <4>;
> status = "disabled";
> @@ -75,6 +77,7 @@ uart1: serial@4150000 {
> reg = <0x04150000 0x100>;
> interrupts = <45 IRQ_TYPE_LEVEL_HIGH>;
> clocks = <&osc>;
> + resets = <&rst RST_UART1>;
> reg-shift = <2>;
> reg-io-width = <4>;
> status = "disabled";
> @@ -85,6 +88,7 @@ uart2: serial@4160000 {
> reg = <0x04160000 0x100>;
> interrupts = <46 IRQ_TYPE_LEVEL_HIGH>;
> clocks = <&osc>;
> + resets = <&rst RST_UART2>;
> reg-shift = <2>;
> reg-io-width = <4>;
> status = "disabled";
> @@ -95,6 +99,7 @@ uart3: serial@4170000 {
> reg = <0x04170000 0x100>;
> interrupts = <47 IRQ_TYPE_LEVEL_HIGH>;
> clocks = <&osc>;
> + resets = <&rst RST_UART3>;
> reg-shift = <2>;
> reg-io-width = <4>;
> status = "disabled";
> @@ -105,6 +110,7 @@ uart4: serial@41c0000 {
> reg = <0x041c0000 0x100>;
> interrupts = <48 IRQ_TYPE_LEVEL_HIGH>;
> clocks = <&osc>;
> + resets = <&rst RST_UART4>;
> reg-shift = <2>;
> reg-io-width = <4>;
> status = "disabled";
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH 4/4] riscv: dts: sophgo: add reset phandle to all uart nodes
2023-11-13 0:55 ` [PATCH 4/4] riscv: dts: sophgo: add reset phandle to all uart nodes Jisheng Zhang
2023-11-13 2:04 ` Samuel Holland
@ 2023-11-13 4:57 ` kernel test robot
1 sibling, 0 replies; 21+ messages in thread
From: kernel test robot @ 2023-11-13 4:57 UTC (permalink / raw)
To: Jisheng Zhang, Philipp Zabel, Rob Herring, Krzysztof Kozlowski,
Conor Dooley, Paul Walmsley, Palmer Dabbelt, Albert Ou, Chao Wei,
Chen Wang
Cc: oe-kbuild-all, devicetree, linux-kernel, linux-riscv
Hi Jisheng,
kernel test robot noticed the following build errors:
[auto build test ERROR on next-20231110]
[also build test ERROR on linus/master v6.7-rc1]
[cannot apply to robh/for-next krzk/for-next krzk-dt/for-next pza/reset/next pza/imx-drm/next v6.6 v6.6-rc7 v6.6-rc6]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]
url: https://github.com/intel-lab-lkp/linux/commits/Jisheng-Zhang/dt-bindings-reset-Add-binding-for-Sophgo-CV1800B-reset-controller/20231113-091129
base: next-20231110
patch link: https://lore.kernel.org/r/20231113005503.2423-5-jszhang%40kernel.org
patch subject: [PATCH 4/4] riscv: dts: sophgo: add reset phandle to all uart nodes
config: riscv-randconfig-001-20231113 (https://download.01.org/0day-ci/archive/20231113/202311131220.lnq9Gdut-lkp@intel.com/config)
compiler: riscv64-linux-gcc (GCC) 13.2.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20231113/202311131220.lnq9Gdut-lkp@intel.com/reproduce)
If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202311131220.lnq9Gdut-lkp@intel.com/
All errors (new ones prefixed by >>):
In file included from arch/riscv/boot/dts/sophgo/cv1800b.dtsi:7,
from arch/riscv/boot/dts/sophgo/cv1800b-milkv-duo.dts:8:
>> scripts/dtc/include-prefixes/dt-bindings/reset/sophgo,cv1800b-reset.h:7: error: unterminated #ifndef
7 | #ifndef _DT_BINDINGS_CV1800B_RESET_H
|
vim +7 scripts/dtc/include-prefixes/dt-bindings/reset/sophgo,cv1800b-reset.h
--
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH 4/4] riscv: dts: sophgo: add reset phandle to all uart nodes
2023-11-13 2:04 ` Samuel Holland
@ 2023-11-13 13:09 ` Jisheng Zhang
0 siblings, 0 replies; 21+ messages in thread
From: Jisheng Zhang @ 2023-11-13 13:09 UTC (permalink / raw)
To: Samuel Holland
Cc: Philipp Zabel, Rob Herring, Krzysztof Kozlowski, Conor Dooley,
devicetree, linux-kernel, linux-riscv, Paul Walmsley,
Palmer Dabbelt, Albert Ou, Chao Wei, Chen Wang
On Sun, Nov 12, 2023 at 09:04:55PM -0500, Samuel Holland wrote:
> Hi Jisheng,
>
> On 2023-11-12 6:55 PM, Jisheng Zhang wrote:
> > Although, the resets are deasserted by default. Add them for
> > completeness.
> >
> > Signed-off-by: Jisheng Zhang <jszhang@kernel.org>
> > ---
> > arch/riscv/boot/dts/sophgo/cv1800b.dtsi | 6 ++++++
> > 1 file changed, 6 insertions(+)
> >
> > diff --git a/arch/riscv/boot/dts/sophgo/cv1800b.dtsi b/arch/riscv/boot/dts/sophgo/cv1800b.dtsi
> > index 4032419486be..e04df04a91c0 100644
> > --- a/arch/riscv/boot/dts/sophgo/cv1800b.dtsi
> > +++ b/arch/riscv/boot/dts/sophgo/cv1800b.dtsi
> > @@ -4,6 +4,7 @@
> > */
> >
> > #include <dt-bindings/interrupt-controller/irq.h>
> > +#include <dt-bindings/reset/sophgo,cv1800b-reset.h>
> >
> > / {
> > compatible = "sophgo,cv1800b";
> > @@ -65,6 +66,7 @@ uart0: serial@4140000 {
> > reg = <0x04140000 0x100>;
> > interrupts = <44 IRQ_TYPE_LEVEL_HIGH>;
> > clocks = <&osc>;
> > + resets = <&rst RST_UART0>;
>
> Since it's not obvious: this breaks devicetree forward compatibility. An
> existing kernel will fail the devm_reset_control_get_optional_exclusive() in
> 8250_dw.c because it has no driver for the reset controller.
>
> This may not be a concern yet, since the devicetree is still "in development".
> But it is something to keep in mind for the future. To avoid this sort of
> problem, it's best to fully model the clocks/resets/other dependencies as early
> as possible, and not rely on the firmware setting anything up.
Thank you. This may be discussed before, "DT backward compatibility is a
must while forward compatibility is optional"? maybe I'm wrong.
And Indeed, it's better if we can have forward compatibility, will take
care this in future.
>
> Regards,
> Samuel
>
> > reg-shift = <2>;
> > reg-io-width = <4>;
> > status = "disabled";
> > @@ -75,6 +77,7 @@ uart1: serial@4150000 {
> > reg = <0x04150000 0x100>;
> > interrupts = <45 IRQ_TYPE_LEVEL_HIGH>;
> > clocks = <&osc>;
> > + resets = <&rst RST_UART1>;
> > reg-shift = <2>;
> > reg-io-width = <4>;
> > status = "disabled";
> > @@ -85,6 +88,7 @@ uart2: serial@4160000 {
> > reg = <0x04160000 0x100>;
> > interrupts = <46 IRQ_TYPE_LEVEL_HIGH>;
> > clocks = <&osc>;
> > + resets = <&rst RST_UART2>;
> > reg-shift = <2>;
> > reg-io-width = <4>;
> > status = "disabled";
> > @@ -95,6 +99,7 @@ uart3: serial@4170000 {
> > reg = <0x04170000 0x100>;
> > interrupts = <47 IRQ_TYPE_LEVEL_HIGH>;
> > clocks = <&osc>;
> > + resets = <&rst RST_UART3>;
> > reg-shift = <2>;
> > reg-io-width = <4>;
> > status = "disabled";
> > @@ -105,6 +110,7 @@ uart4: serial@41c0000 {
> > reg = <0x041c0000 0x100>;
> > interrupts = <48 IRQ_TYPE_LEVEL_HIGH>;
> > clocks = <&osc>;
> > + resets = <&rst RST_UART4>;
> > reg-shift = <2>;
> > reg-io-width = <4>;
> > status = "disabled";
>
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH 1/4] dt-bindings: reset: Add binding for Sophgo CV1800B reset controller
2023-11-13 0:55 ` [PATCH 1/4] dt-bindings: reset: Add binding for Sophgo CV1800B reset controller Jisheng Zhang
@ 2023-11-13 13:36 ` Conor Dooley
2023-11-13 14:00 ` Jisheng Zhang
2023-11-14 21:12 ` Krzysztof Kozlowski
1 sibling, 1 reply; 21+ messages in thread
From: Conor Dooley @ 2023-11-13 13:36 UTC (permalink / raw)
To: Jisheng Zhang
Cc: Philipp Zabel, Rob Herring, Krzysztof Kozlowski, Conor Dooley,
Paul Walmsley, Palmer Dabbelt, Albert Ou, Chao Wei, Chen Wang,
devicetree, linux-kernel, linux-riscv
[-- Attachment #1: Type: text/plain, Size: 613 bytes --]
On Mon, Nov 13, 2023 at 08:55:00AM +0800, Jisheng Zhang wrote:
> Add devicetree binding for Sophgo CV1800B SoC reset controller.
>
> Signed-off-by: Jisheng Zhang <jszhang@kernel.org>
With the unterminated ifndef that was pointed out by the robots fixed,
Reviewed-by: Conor Dooley <conor.dooley@microchip.com>
> +/* 0-1 */
> +/* 10 */
> +/* 13 */
> +/* 15 */
> +/* 17 */
> +/* 36-39 */
> +/* 53-57 */
> +/* 59-60 */
> +/* 63-73 */
> +/* 90 */
> +/* 94 */
> +/* 102-292 */
There are quite a lot of gaps here, do you know why that is?
Thanks,
Conor.
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH 1/4] dt-bindings: reset: Add binding for Sophgo CV1800B reset controller
2023-11-13 13:36 ` Conor Dooley
@ 2023-11-13 14:00 ` Jisheng Zhang
2023-11-14 21:13 ` Krzysztof Kozlowski
0 siblings, 1 reply; 21+ messages in thread
From: Jisheng Zhang @ 2023-11-13 14:00 UTC (permalink / raw)
To: Conor Dooley
Cc: Philipp Zabel, Rob Herring, Krzysztof Kozlowski, Conor Dooley,
Paul Walmsley, Palmer Dabbelt, Albert Ou, Chao Wei, Chen Wang,
devicetree, linux-kernel, linux-riscv
On Mon, Nov 13, 2023 at 01:36:54PM +0000, Conor Dooley wrote:
> On Mon, Nov 13, 2023 at 08:55:00AM +0800, Jisheng Zhang wrote:
> > Add devicetree binding for Sophgo CV1800B SoC reset controller.
> >
> > Signed-off-by: Jisheng Zhang <jszhang@kernel.org>
>
> With the unterminated ifndef that was pointed out by the robots fixed,
> Reviewed-by: Conor Dooley <conor.dooley@microchip.com>
>
> > +/* 0-1 */
> > +/* 10 */
> > +/* 13 */
> > +/* 15 */
> > +/* 17 */
> > +/* 36-39 */
> > +/* 53-57 */
> > +/* 59-60 */
> > +/* 63-73 */
> > +/* 90 */
> > +/* 94 */
> > +/* 102-292 */
>
> There are quite a lot of gaps here, do you know why that is?
The tail bits are for cpusys, so I guess the SoC designer want to
seperate them with guard? I'm not sure.
>
> Thanks,
> Conor.
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH 3/4] riscv: dts: sophgo: add reset dt node for cv1800b
2023-11-13 0:55 ` [PATCH 3/4] riscv: dts: sophgo: add reset dt node for cv1800b Jisheng Zhang
@ 2023-11-13 14:32 ` Yixun Lan
2023-11-13 15:14 ` Jisheng Zhang
0 siblings, 1 reply; 21+ messages in thread
From: Yixun Lan @ 2023-11-13 14:32 UTC (permalink / raw)
To: Jisheng Zhang
Cc: Philipp Zabel, Rob Herring, Krzysztof Kozlowski, Conor Dooley,
Paul Walmsley, Palmer Dabbelt, Albert Ou, Chao Wei, Chen Wang,
devicetree, linux-kernel, linux-riscv
Hi Jisheng:
On 08:55 Mon 13 Nov , Jisheng Zhang wrote:
> Add the reset device tree node to cv1800b SoC.
>
> Signed-off-by: Jisheng Zhang <jszhang@kernel.org>
> ---
> arch/riscv/boot/dts/sophgo/cv1800b.dtsi | 6 ++++++
> 1 file changed, 6 insertions(+)
>
> diff --git a/arch/riscv/boot/dts/sophgo/cv1800b.dtsi b/arch/riscv/boot/dts/sophgo/cv1800b.dtsi
> index df40e87ee063..4032419486be 100644
> --- a/arch/riscv/boot/dts/sophgo/cv1800b.dtsi
> +++ b/arch/riscv/boot/dts/sophgo/cv1800b.dtsi
> @@ -54,6 +54,12 @@ soc {
> dma-noncoherent;
> ranges;
>
> + rst: reset-controller@3003000 {
> + compatible = "sophgo,cv1800b-reset";
> + reg = <0x03003000 0x1000>;
~~~~~~~
it should be 0x28
while please also note the 0x24 == SOFT_CPUAC_RSTN, does not compatible
with the reset-simple driver, but as it's not implemented nor used in this driver,
so we should be fine with this?
> + #reset-cells = <1>;
> + };
> +
> uart0: serial@4140000 {
> compatible = "snps,dw-apb-uart";
> reg = <0x04140000 0x100>;
> --
> 2.42.0
>
--
Yixun Lan (dlan)
Gentoo Linux Developer
GPG Key ID AABEFD55
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH 3/4] riscv: dts: sophgo: add reset dt node for cv1800b
2023-11-13 14:32 ` Yixun Lan
@ 2023-11-13 15:14 ` Jisheng Zhang
2023-11-13 15:37 ` Samuel Holland
0 siblings, 1 reply; 21+ messages in thread
From: Jisheng Zhang @ 2023-11-13 15:14 UTC (permalink / raw)
To: Yixun Lan
Cc: Philipp Zabel, Rob Herring, Krzysztof Kozlowski, Conor Dooley,
Paul Walmsley, Palmer Dabbelt, Albert Ou, Chao Wei, Chen Wang,
devicetree, linux-kernel, linux-riscv
On Mon, Nov 13, 2023 at 02:32:24PM +0000, Yixun Lan wrote:
> Hi Jisheng:
Hi
>
> On 08:55 Mon 13 Nov , Jisheng Zhang wrote:
> > Add the reset device tree node to cv1800b SoC.
> >
> > Signed-off-by: Jisheng Zhang <jszhang@kernel.org>
> > ---
> > arch/riscv/boot/dts/sophgo/cv1800b.dtsi | 6 ++++++
> > 1 file changed, 6 insertions(+)
> >
> > diff --git a/arch/riscv/boot/dts/sophgo/cv1800b.dtsi b/arch/riscv/boot/dts/sophgo/cv1800b.dtsi
> > index df40e87ee063..4032419486be 100644
> > --- a/arch/riscv/boot/dts/sophgo/cv1800b.dtsi
> > +++ b/arch/riscv/boot/dts/sophgo/cv1800b.dtsi
> > @@ -54,6 +54,12 @@ soc {
> > dma-noncoherent;
> > ranges;
> >
> > + rst: reset-controller@3003000 {
> > + compatible = "sophgo,cv1800b-reset";
> > + reg = <0x03003000 0x1000>;
> ~~~~~~~
> it should be 0x28
The reg space is 4KB, but only 0x28 are used. I think 0x1000 or 0x28 are fine
since the ioremap granule is 4kB.
>
> while please also note the 0x24 == SOFT_CPUAC_RSTN, does not compatible
> with the reset-simple driver, but as it's not implemented nor used in this driver,
But the functionality of this "autoclear" reg isn't used at all since we also
have "sticky" reset to acchieve the same feature, I.E reset cpusys. And in the
usage case of reseting cpusys, I believe "sticky" reset is preferred.
And except the cpusys reset which has both autoclear and sticky, other
resets are sticky only. I'm not sure whether it's worth to write a new
driver for almost useless feature.
> so we should be fine with this?
>
> > + #reset-cells = <1>;
> > + };
> > +
> > uart0: serial@4140000 {
> > compatible = "snps,dw-apb-uart";
> > reg = <0x04140000 0x100>;
> > --
> > 2.42.0
> >
>
> --
> Yixun Lan (dlan)
> Gentoo Linux Developer
> GPG Key ID AABEFD55
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH 3/4] riscv: dts: sophgo: add reset dt node for cv1800b
2023-11-13 15:14 ` Jisheng Zhang
@ 2023-11-13 15:37 ` Samuel Holland
2023-11-14 14:55 ` Jisheng Zhang
0 siblings, 1 reply; 21+ messages in thread
From: Samuel Holland @ 2023-11-13 15:37 UTC (permalink / raw)
To: Jisheng Zhang, Yixun Lan
Cc: Philipp Zabel, Rob Herring, Krzysztof Kozlowski, Conor Dooley,
Paul Walmsley, Palmer Dabbelt, Albert Ou, Chao Wei, Chen Wang,
devicetree, linux-kernel, linux-riscv
Hi Jisheng,
On 2023-11-13 9:14 AM, Jisheng Zhang wrote:
> On Mon, Nov 13, 2023 at 02:32:24PM +0000, Yixun Lan wrote:
>> On 08:55 Mon 13 Nov , Jisheng Zhang wrote:
>>> Add the reset device tree node to cv1800b SoC.
>>>
>>> Signed-off-by: Jisheng Zhang <jszhang@kernel.org>
>>> ---
>>> arch/riscv/boot/dts/sophgo/cv1800b.dtsi | 6 ++++++
>>> 1 file changed, 6 insertions(+)
>>>
>>> diff --git a/arch/riscv/boot/dts/sophgo/cv1800b.dtsi b/arch/riscv/boot/dts/sophgo/cv1800b.dtsi
>>> index df40e87ee063..4032419486be 100644
>>> --- a/arch/riscv/boot/dts/sophgo/cv1800b.dtsi
>>> +++ b/arch/riscv/boot/dts/sophgo/cv1800b.dtsi
>>> @@ -54,6 +54,12 @@ soc {
>>> dma-noncoherent;
>>> ranges;
>>>
>>> + rst: reset-controller@3003000 {
>>> + compatible = "sophgo,cv1800b-reset";
>>> + reg = <0x03003000 0x1000>;
>> ~~~~~~~
>> it should be 0x28
>
> The reg space is 4KB, but only 0x28 are used. I think 0x1000 or 0x28 are fine
> since the ioremap granule is 4kB.
>>
>> while please also note the 0x24 == SOFT_CPUAC_RSTN, does not compatible
>> with the reset-simple driver, but as it's not implemented nor used in this driver,
>
> But the functionality of this "autoclear" reg isn't used at all since we also
> have "sticky" reset to acchieve the same feature, I.E reset cpusys. And in the
> usage case of reseting cpusys, I believe "sticky" reset is preferred.
>
> And except the cpusys reset which has both autoclear and sticky, other
> resets are sticky only. I'm not sure whether it's worth to write a new
> driver for almost useless feature.
As long as the device has its own binding/compatible string, it is always
possible to replace RESET_SIMPLE with a custom driver later if needed. (Or use a
more complicated driver in some other context, e.g. firmware).
Regards,
Samuel
>> so we should be fine with this?
>>
>>> + #reset-cells = <1>;
>>> + };
>>> +
>>> uart0: serial@4140000 {
>>> compatible = "snps,dw-apb-uart";
>>> reg = <0x04140000 0x100>;
>>> --
>>> 2.42.0
>>>
>>
>> --
>> Yixun Lan (dlan)
>> Gentoo Linux Developer
>> GPG Key ID AABEFD55
>
> _______________________________________________
> linux-riscv mailing list
> linux-riscv@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-riscv
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH 3/4] riscv: dts: sophgo: add reset dt node for cv1800b
2023-11-13 15:37 ` Samuel Holland
@ 2023-11-14 14:55 ` Jisheng Zhang
0 siblings, 0 replies; 21+ messages in thread
From: Jisheng Zhang @ 2023-11-14 14:55 UTC (permalink / raw)
To: Samuel Holland
Cc: Yixun Lan, Philipp Zabel, Rob Herring, Krzysztof Kozlowski,
Conor Dooley, Paul Walmsley, Palmer Dabbelt, Albert Ou, Chao Wei,
Chen Wang, devicetree, linux-kernel, linux-riscv
On Mon, Nov 13, 2023 at 10:37:35AM -0500, Samuel Holland wrote:
> Hi Jisheng,
Hi Samuel,
>
> On 2023-11-13 9:14 AM, Jisheng Zhang wrote:
> > On Mon, Nov 13, 2023 at 02:32:24PM +0000, Yixun Lan wrote:
> >> On 08:55 Mon 13 Nov , Jisheng Zhang wrote:
> >>> Add the reset device tree node to cv1800b SoC.
> >>>
> >>> Signed-off-by: Jisheng Zhang <jszhang@kernel.org>
> >>> ---
> >>> arch/riscv/boot/dts/sophgo/cv1800b.dtsi | 6 ++++++
> >>> 1 file changed, 6 insertions(+)
> >>>
> >>> diff --git a/arch/riscv/boot/dts/sophgo/cv1800b.dtsi b/arch/riscv/boot/dts/sophgo/cv1800b.dtsi
> >>> index df40e87ee063..4032419486be 100644
> >>> --- a/arch/riscv/boot/dts/sophgo/cv1800b.dtsi
> >>> +++ b/arch/riscv/boot/dts/sophgo/cv1800b.dtsi
> >>> @@ -54,6 +54,12 @@ soc {
> >>> dma-noncoherent;
> >>> ranges;
> >>>
> >>> + rst: reset-controller@3003000 {
> >>> + compatible = "sophgo,cv1800b-reset";
> >>> + reg = <0x03003000 0x1000>;
> >> ~~~~~~~
> >> it should be 0x28
> >
> > The reg space is 4KB, but only 0x28 are used. I think 0x1000 or 0x28 are fine
> > since the ioremap granule is 4kB.
> >>
> >> while please also note the 0x24 == SOFT_CPUAC_RSTN, does not compatible
> >> with the reset-simple driver, but as it's not implemented nor used in this driver,
> >
> > But the functionality of this "autoclear" reg isn't used at all since we also
> > have "sticky" reset to acchieve the same feature, I.E reset cpusys. And in the
> > usage case of reseting cpusys, I believe "sticky" reset is preferred.
> >
> > And except the cpusys reset which has both autoclear and sticky, other
> > resets are sticky only. I'm not sure whether it's worth to write a new
> > driver for almost useless feature.
>
> As long as the device has its own binding/compatible string, it is always
> possible to replace RESET_SIMPLE with a custom driver later if needed. (Or use a
> more complicated driver in some other context, e.g. firmware).
Good idea, indeed if needed we can implement a customized driver, and I think
this can acchieve both backward and forward DT compatbility ;)
As for firmware, I guess you mean the little c906 core os firmware. Here
is my draft plan:
a sophgo custom remoteproc driver which will do something like:
load the firmware;
reset_assert(rst);
prepara cpu entry address and so on;
reset_deassert(rst);
so sticky reset still works perfectly. While I believe autoclear reset
may not work if the reset clears something we have set.
>
> Regards,
> Samuel
>
> >> so we should be fine with this?
> >>
> >>> + #reset-cells = <1>;
> >>> + };
> >>> +
> >>> uart0: serial@4140000 {
> >>> compatible = "snps,dw-apb-uart";
> >>> reg = <0x04140000 0x100>;
> >>> --
> >>> 2.42.0
> >>>
> >>
> >> --
> >> Yixun Lan (dlan)
> >> Gentoo Linux Developer
> >> GPG Key ID AABEFD55
> >
> > _______________________________________________
> > linux-riscv mailing list
> > linux-riscv@lists.infradead.org
> > http://lists.infradead.org/mailman/listinfo/linux-riscv
>
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH 1/4] dt-bindings: reset: Add binding for Sophgo CV1800B reset controller
2023-11-13 0:55 ` [PATCH 1/4] dt-bindings: reset: Add binding for Sophgo CV1800B reset controller Jisheng Zhang
2023-11-13 13:36 ` Conor Dooley
@ 2023-11-14 21:12 ` Krzysztof Kozlowski
2023-11-15 13:27 ` Jisheng Zhang
1 sibling, 1 reply; 21+ messages in thread
From: Krzysztof Kozlowski @ 2023-11-14 21:12 UTC (permalink / raw)
To: Jisheng Zhang, Philipp Zabel, Rob Herring, Krzysztof Kozlowski,
Conor Dooley, Paul Walmsley, Palmer Dabbelt, Albert Ou, Chao Wei,
Chen Wang
Cc: devicetree, linux-kernel, linux-riscv
On 13/11/2023 01:55, Jisheng Zhang wrote:
...
> diff --git a/include/dt-bindings/reset/sophgo,cv1800b-reset.h b/include/dt-bindings/reset/sophgo,cv1800b-reset.h
> new file mode 100644
> index 000000000000..28dda71369b4
> --- /dev/null
> +++ b/include/dt-bindings/reset/sophgo,cv1800b-reset.h
> @@ -0,0 +1,96 @@
> +/* SPDX-License-Identifier: GPL-2.0 OR MIT */
> +/*
> + * Copyright (C) 2023 Sophgo Technology Inc. All rights reserved.
> + * Copyright (C) 2023 Jisheng Zhang <jszhang@kernel.org>
> + */
> +
> +#ifndef _DT_BINDINGS_CV1800B_RESET_H
> +#define _DT_BINDINGS_CV1800B_RESET_H
> +
> +/* 0-1 */
> +#define RST_DDR 2
> +#define RST_H264C 3
> +#define RST_JPEG 4
> +#define RST_H265C 5
> +#define RST_VIPSYS 6
> +#define RST_TDMA 7
> +#define RST_TPU 8
> +#define RST_TPUSYS 9
> +/* 10 */
Why do you have empty IDs? IDs start at 0 and are incremented by 1.
Best regards,
Krzysztof
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH 1/4] dt-bindings: reset: Add binding for Sophgo CV1800B reset controller
2023-11-13 14:00 ` Jisheng Zhang
@ 2023-11-14 21:13 ` Krzysztof Kozlowski
0 siblings, 0 replies; 21+ messages in thread
From: Krzysztof Kozlowski @ 2023-11-14 21:13 UTC (permalink / raw)
To: Jisheng Zhang, Conor Dooley
Cc: Philipp Zabel, Rob Herring, Krzysztof Kozlowski, Conor Dooley,
Paul Walmsley, Palmer Dabbelt, Albert Ou, Chao Wei, Chen Wang,
devicetree, linux-kernel, linux-riscv
On 13/11/2023 15:00, Jisheng Zhang wrote:
> On Mon, Nov 13, 2023 at 01:36:54PM +0000, Conor Dooley wrote:
>> On Mon, Nov 13, 2023 at 08:55:00AM +0800, Jisheng Zhang wrote:
>>> Add devicetree binding for Sophgo CV1800B SoC reset controller.
>>>
>>> Signed-off-by: Jisheng Zhang <jszhang@kernel.org>
>>
>> With the unterminated ifndef that was pointed out by the robots fixed,
>> Reviewed-by: Conor Dooley <conor.dooley@microchip.com>
>>
>>> +/* 0-1 */
>>> +/* 10 */
>>> +/* 13 */
>>> +/* 15 */
>>> +/* 17 */
>>> +/* 36-39 */
>>> +/* 53-57 */
>>> +/* 59-60 */
>>> +/* 63-73 */
>>> +/* 90 */
>>> +/* 94 */
>>> +/* 102-292 */
>>
>> There are quite a lot of gaps here, do you know why that is?
>
> The tail bits are for cpusys, so I guess the SoC designer want to
> seperate them with guard? I'm not sure.
>
There is misunderstanding here. You add here IDs, which are abstract.
Any gaps do not make any sense for bindings.
Best regards,
Krzysztof
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH 1/4] dt-bindings: reset: Add binding for Sophgo CV1800B reset controller
2023-11-14 21:12 ` Krzysztof Kozlowski
@ 2023-11-15 13:27 ` Jisheng Zhang
2023-11-15 14:56 ` Samuel Holland
0 siblings, 1 reply; 21+ messages in thread
From: Jisheng Zhang @ 2023-11-15 13:27 UTC (permalink / raw)
To: Krzysztof Kozlowski
Cc: Philipp Zabel, Rob Herring, Krzysztof Kozlowski, Conor Dooley,
Paul Walmsley, Palmer Dabbelt, Albert Ou, Chao Wei, Chen Wang,
devicetree, linux-kernel, linux-riscv
On Tue, Nov 14, 2023 at 10:12:35PM +0100, Krzysztof Kozlowski wrote:
> On 13/11/2023 01:55, Jisheng Zhang wrote:
> ...
>
> > diff --git a/include/dt-bindings/reset/sophgo,cv1800b-reset.h b/include/dt-bindings/reset/sophgo,cv1800b-reset.h
> > new file mode 100644
> > index 000000000000..28dda71369b4
> > --- /dev/null
> > +++ b/include/dt-bindings/reset/sophgo,cv1800b-reset.h
> > @@ -0,0 +1,96 @@
> > +/* SPDX-License-Identifier: GPL-2.0 OR MIT */
> > +/*
> > + * Copyright (C) 2023 Sophgo Technology Inc. All rights reserved.
> > + * Copyright (C) 2023 Jisheng Zhang <jszhang@kernel.org>
> > + */
> > +
> > +#ifndef _DT_BINDINGS_CV1800B_RESET_H
> > +#define _DT_BINDINGS_CV1800B_RESET_H
> > +
> > +/* 0-1 */
> > +#define RST_DDR 2
> > +#define RST_H264C 3
> > +#define RST_JPEG 4
> > +#define RST_H265C 5
> > +#define RST_VIPSYS 6
> > +#define RST_TDMA 7
> > +#define RST_TPU 8
> > +#define RST_TPUSYS 9
> > +/* 10 */
>
> Why do you have empty IDs? IDs start at 0 and are incremented by 1.
there's 1:1 mapping between the ID and bit. Some bits are reserved, I.E
no actions at all. Is "ID start at 0 and increment by 1" documented
in some docs? From another side, I also notice some SoCs especially
those which make use of reset-simple driver don't strictly follow
this rule, for example, amlogic,meson-a1-reset.h and so on. What
happened?
And I'd like to ask a question here before cooking 2nd version:
if the HW programming logic is the same as reset-simple, but some
or many bits are reserved, what's the can-be-accepted way to support
the reset controller? Use reset-simple? Obviously if we want the
"ID start at 0 and increment by 1" rule, then we have to write
a custom driver which almost use the reset-simple but with a
customized mapping.
Thanks
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH 1/4] dt-bindings: reset: Add binding for Sophgo CV1800B reset controller
2023-11-15 13:27 ` Jisheng Zhang
@ 2023-11-15 14:56 ` Samuel Holland
2023-11-15 15:02 ` Conor Dooley
0 siblings, 1 reply; 21+ messages in thread
From: Samuel Holland @ 2023-11-15 14:56 UTC (permalink / raw)
To: Jisheng Zhang, Krzysztof Kozlowski
Cc: Philipp Zabel, Rob Herring, Krzysztof Kozlowski, Conor Dooley,
Paul Walmsley, Palmer Dabbelt, Albert Ou, Chao Wei, Chen Wang,
devicetree, linux-kernel, linux-riscv
On 2023-11-15 7:27 AM, Jisheng Zhang wrote:
> On Tue, Nov 14, 2023 at 10:12:35PM +0100, Krzysztof Kozlowski wrote:
>> On 13/11/2023 01:55, Jisheng Zhang wrote:
>> ...
>>
>>> diff --git a/include/dt-bindings/reset/sophgo,cv1800b-reset.h b/include/dt-bindings/reset/sophgo,cv1800b-reset.h
>>> new file mode 100644
>>> index 000000000000..28dda71369b4
>>> --- /dev/null
>>> +++ b/include/dt-bindings/reset/sophgo,cv1800b-reset.h
>>> @@ -0,0 +1,96 @@
>>> +/* SPDX-License-Identifier: GPL-2.0 OR MIT */
>>> +/*
>>> + * Copyright (C) 2023 Sophgo Technology Inc. All rights reserved.
>>> + * Copyright (C) 2023 Jisheng Zhang <jszhang@kernel.org>
>>> + */
>>> +
>>> +#ifndef _DT_BINDINGS_CV1800B_RESET_H
>>> +#define _DT_BINDINGS_CV1800B_RESET_H
>>> +
>>> +/* 0-1 */
>>> +#define RST_DDR 2
>>> +#define RST_H264C 3
>>> +#define RST_JPEG 4
>>> +#define RST_H265C 5
>>> +#define RST_VIPSYS 6
>>> +#define RST_TDMA 7
>>> +#define RST_TPU 8
>>> +#define RST_TPUSYS 9
>>> +/* 10 */
>>
>> Why do you have empty IDs? IDs start at 0 and are incremented by 1.
>
> there's 1:1 mapping between the ID and bit. Some bits are reserved, I.E
> no actions at all. Is "ID start at 0 and increment by 1" documented
> in some docs? From another side, I also notice some SoCs especially
> those which make use of reset-simple driver don't strictly follow
> this rule, for example, amlogic,meson-a1-reset.h and so on. What
> happened?
>
> And I'd like to ask a question here before cooking 2nd version:
> if the HW programming logic is the same as reset-simple, but some
> or many bits are reserved, what's the can-be-accepted way to support
> the reset controller? Use reset-simple? Obviously if we want the
> "ID start at 0 and increment by 1" rule, then we have to write
> a custom driver which almost use the reset-simple but with a
> customized mapping.
There are two possible situations. Either the reset specifier maps directly to
something in the hardware, or you are inventing some brand new enumeration to
use as a specifier.
In the first situation, you do not need a header. We assume the user will look
to the SoC documentation if they want to know what the numbers mean. (You aren't
_creating_ an ABI, since the ABI is already defined by the hardware.)
In the second situation, since we are inventing something new, the numbers
should be contiguous. This is what Krzysztof's comment was about.
For this reset device, the numbers are hardware bit offsets, so you are in the
first situation. So I think the recommended solution here is to remove the
header entirely and use the bit numbers directly in the SoC devicetree.
It's still appropriate to use reset-simple. Adding some new mapping would make
things more complicated for no benefit.
Regards,
Samuel
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH 1/4] dt-bindings: reset: Add binding for Sophgo CV1800B reset controller
2023-11-15 14:56 ` Samuel Holland
@ 2023-11-15 15:02 ` Conor Dooley
2023-11-15 15:15 ` Jisheng Zhang
0 siblings, 1 reply; 21+ messages in thread
From: Conor Dooley @ 2023-11-15 15:02 UTC (permalink / raw)
To: Samuel Holland
Cc: Jisheng Zhang, Krzysztof Kozlowski, Philipp Zabel, Rob Herring,
Krzysztof Kozlowski, Conor Dooley, Paul Walmsley, Palmer Dabbelt,
Albert Ou, Chao Wei, Chen Wang, devicetree, linux-kernel,
linux-riscv
[-- Attachment #1: Type: text/plain, Size: 3156 bytes --]
On Wed, Nov 15, 2023 at 09:56:07AM -0500, Samuel Holland wrote:
> On 2023-11-15 7:27 AM, Jisheng Zhang wrote:
> > On Tue, Nov 14, 2023 at 10:12:35PM +0100, Krzysztof Kozlowski wrote:
> >> On 13/11/2023 01:55, Jisheng Zhang wrote:
> >> ...
> >>
> >>> diff --git a/include/dt-bindings/reset/sophgo,cv1800b-reset.h b/include/dt-bindings/reset/sophgo,cv1800b-reset.h
> >>> new file mode 100644
> >>> index 000000000000..28dda71369b4
> >>> --- /dev/null
> >>> +++ b/include/dt-bindings/reset/sophgo,cv1800b-reset.h
> >>> @@ -0,0 +1,96 @@
> >>> +/* SPDX-License-Identifier: GPL-2.0 OR MIT */
> >>> +/*
> >>> + * Copyright (C) 2023 Sophgo Technology Inc. All rights reserved.
> >>> + * Copyright (C) 2023 Jisheng Zhang <jszhang@kernel.org>
> >>> + */
> >>> +
> >>> +#ifndef _DT_BINDINGS_CV1800B_RESET_H
> >>> +#define _DT_BINDINGS_CV1800B_RESET_H
> >>> +
> >>> +/* 0-1 */
> >>> +#define RST_DDR 2
> >>> +#define RST_H264C 3
> >>> +#define RST_JPEG 4
> >>> +#define RST_H265C 5
> >>> +#define RST_VIPSYS 6
> >>> +#define RST_TDMA 7
> >>> +#define RST_TPU 8
> >>> +#define RST_TPUSYS 9
> >>> +/* 10 */
> >>
> >> Why do you have empty IDs? IDs start at 0 and are incremented by 1.
> >
> > there's 1:1 mapping between the ID and bit. Some bits are reserved, I.E
> > no actions at all. Is "ID start at 0 and increment by 1" documented
> > in some docs? From another side, I also notice some SoCs especially
> > those which make use of reset-simple driver don't strictly follow
> > this rule, for example, amlogic,meson-a1-reset.h and so on. What
> > happened?
> >
> > And I'd like to ask a question here before cooking 2nd version:
> > if the HW programming logic is the same as reset-simple, but some
> > or many bits are reserved, what's the can-be-accepted way to support
> > the reset controller? Use reset-simple? Obviously if we want the
> > "ID start at 0 and increment by 1" rule, then we have to write
> > a custom driver which almost use the reset-simple but with a
> > customized mapping.
>
> There are two possible situations. Either the reset specifier maps directly to
> something in the hardware, or you are inventing some brand new enumeration to
> use as a specifier.
>
> In the first situation, you do not need a header. We assume the user will look
> to the SoC documentation if they want to know what the numbers mean. (You aren't
> _creating_ an ABI, since the ABI is already defined by the hardware.)
>
> In the second situation, since we are inventing something new, the numbers
> should be contiguous. This is what Krzysztof's comment was about.
>
> For this reset device, the numbers are hardware bit offsets, so you are in the
> first situation. So I think the recommended solution here is to remove the
> header entirely and use the bit numbers directly in the SoC devicetree.
>
> It's still appropriate to use reset-simple. Adding some new mapping would make
> things more complicated for no benefit.
Further, I think it is fine in that case to have a header, just the
header doesn't belong as a binding, and can instead go in the dts
directory.
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH 1/4] dt-bindings: reset: Add binding for Sophgo CV1800B reset controller
2023-11-15 15:02 ` Conor Dooley
@ 2023-11-15 15:15 ` Jisheng Zhang
2023-11-15 21:00 ` Krzysztof Kozlowski
0 siblings, 1 reply; 21+ messages in thread
From: Jisheng Zhang @ 2023-11-15 15:15 UTC (permalink / raw)
To: Conor Dooley
Cc: Samuel Holland, Krzysztof Kozlowski, Philipp Zabel, Rob Herring,
Krzysztof Kozlowski, Conor Dooley, Paul Walmsley, Palmer Dabbelt,
Albert Ou, Chao Wei, Chen Wang, devicetree, linux-kernel,
linux-riscv
On Wed, Nov 15, 2023 at 03:02:21PM +0000, Conor Dooley wrote:
> On Wed, Nov 15, 2023 at 09:56:07AM -0500, Samuel Holland wrote:
> > On 2023-11-15 7:27 AM, Jisheng Zhang wrote:
> > > On Tue, Nov 14, 2023 at 10:12:35PM +0100, Krzysztof Kozlowski wrote:
> > >> On 13/11/2023 01:55, Jisheng Zhang wrote:
> > >> ...
> > >>
> > >>> diff --git a/include/dt-bindings/reset/sophgo,cv1800b-reset.h b/include/dt-bindings/reset/sophgo,cv1800b-reset.h
> > >>> new file mode 100644
> > >>> index 000000000000..28dda71369b4
> > >>> --- /dev/null
> > >>> +++ b/include/dt-bindings/reset/sophgo,cv1800b-reset.h
> > >>> @@ -0,0 +1,96 @@
> > >>> +/* SPDX-License-Identifier: GPL-2.0 OR MIT */
> > >>> +/*
> > >>> + * Copyright (C) 2023 Sophgo Technology Inc. All rights reserved.
> > >>> + * Copyright (C) 2023 Jisheng Zhang <jszhang@kernel.org>
> > >>> + */
> > >>> +
> > >>> +#ifndef _DT_BINDINGS_CV1800B_RESET_H
> > >>> +#define _DT_BINDINGS_CV1800B_RESET_H
> > >>> +
> > >>> +/* 0-1 */
> > >>> +#define RST_DDR 2
> > >>> +#define RST_H264C 3
> > >>> +#define RST_JPEG 4
> > >>> +#define RST_H265C 5
> > >>> +#define RST_VIPSYS 6
> > >>> +#define RST_TDMA 7
> > >>> +#define RST_TPU 8
> > >>> +#define RST_TPUSYS 9
> > >>> +/* 10 */
> > >>
> > >> Why do you have empty IDs? IDs start at 0 and are incremented by 1.
> > >
> > > there's 1:1 mapping between the ID and bit. Some bits are reserved, I.E
> > > no actions at all. Is "ID start at 0 and increment by 1" documented
> > > in some docs? From another side, I also notice some SoCs especially
> > > those which make use of reset-simple driver don't strictly follow
> > > this rule, for example, amlogic,meson-a1-reset.h and so on. What
> > > happened?
> > >
> > > And I'd like to ask a question here before cooking 2nd version:
> > > if the HW programming logic is the same as reset-simple, but some
> > > or many bits are reserved, what's the can-be-accepted way to support
> > > the reset controller? Use reset-simple? Obviously if we want the
> > > "ID start at 0 and increment by 1" rule, then we have to write
> > > a custom driver which almost use the reset-simple but with a
> > > customized mapping.
> >
> > There are two possible situations. Either the reset specifier maps directly to
> > something in the hardware, or you are inventing some brand new enumeration to
> > use as a specifier.
> >
> > In the first situation, you do not need a header. We assume the user will look
> > to the SoC documentation if they want to know what the numbers mean. (You aren't
> > _creating_ an ABI, since the ABI is already defined by the hardware.)
> >
> > In the second situation, since we are inventing something new, the numbers
> > should be contiguous. This is what Krzysztof's comment was about.
> >
> > For this reset device, the numbers are hardware bit offsets, so you are in the
> > first situation. So I think the recommended solution here is to remove the
> > header entirely and use the bit numbers directly in the SoC devicetree.
> >
> > It's still appropriate to use reset-simple. Adding some new mapping would make
> > things more complicated for no benefit.
>
> Further, I think it is fine in that case to have a header, just the
> header doesn't belong as a binding, and can instead go in the dts
> directory.
Hi Samuel, Conor,
thanks a lot for the suggestion!
Regards
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH 1/4] dt-bindings: reset: Add binding for Sophgo CV1800B reset controller
2023-11-15 15:15 ` Jisheng Zhang
@ 2023-11-15 21:00 ` Krzysztof Kozlowski
0 siblings, 0 replies; 21+ messages in thread
From: Krzysztof Kozlowski @ 2023-11-15 21:00 UTC (permalink / raw)
To: Jisheng Zhang, Conor Dooley
Cc: Samuel Holland, Philipp Zabel, Rob Herring, Krzysztof Kozlowski,
Conor Dooley, Paul Walmsley, Palmer Dabbelt, Albert Ou, Chao Wei,
Chen Wang, devicetree, linux-kernel, linux-riscv
On 15/11/2023 16:15, Jisheng Zhang wrote:
> On Wed, Nov 15, 2023 at 03:02:21PM +0000, Conor Dooley wrote:
>> On Wed, Nov 15, 2023 at 09:56:07AM -0500, Samuel Holland wrote:
>>> On 2023-11-15 7:27 AM, Jisheng Zhang wrote:
>>>> On Tue, Nov 14, 2023 at 10:12:35PM +0100, Krzysztof Kozlowski wrote:
>>>>> On 13/11/2023 01:55, Jisheng Zhang wrote:
>>>>> ...
>>>>>
>>>>>> diff --git a/include/dt-bindings/reset/sophgo,cv1800b-reset.h b/include/dt-bindings/reset/sophgo,cv1800b-reset.h
>>>>>> new file mode 100644
>>>>>> index 000000000000..28dda71369b4
>>>>>> --- /dev/null
>>>>>> +++ b/include/dt-bindings/reset/sophgo,cv1800b-reset.h
>>>>>> @@ -0,0 +1,96 @@
>>>>>> +/* SPDX-License-Identifier: GPL-2.0 OR MIT */
>>>>>> +/*
>>>>>> + * Copyright (C) 2023 Sophgo Technology Inc. All rights reserved.
>>>>>> + * Copyright (C) 2023 Jisheng Zhang <jszhang@kernel.org>
>>>>>> + */
>>>>>> +
>>>>>> +#ifndef _DT_BINDINGS_CV1800B_RESET_H
>>>>>> +#define _DT_BINDINGS_CV1800B_RESET_H
>>>>>> +
>>>>>> +/* 0-1 */
>>>>>> +#define RST_DDR 2
>>>>>> +#define RST_H264C 3
>>>>>> +#define RST_JPEG 4
>>>>>> +#define RST_H265C 5
>>>>>> +#define RST_VIPSYS 6
>>>>>> +#define RST_TDMA 7
>>>>>> +#define RST_TPU 8
>>>>>> +#define RST_TPUSYS 9
>>>>>> +/* 10 */
>>>>>
>>>>> Why do you have empty IDs? IDs start at 0 and are incremented by 1.
>>>>
>>>> there's 1:1 mapping between the ID and bit. Some bits are reserved, I.E
>>>> no actions at all. Is "ID start at 0 and increment by 1" documented
>>>> in some docs? From another side, I also notice some SoCs especially
>>>> those which make use of reset-simple driver don't strictly follow
>>>> this rule, for example, amlogic,meson-a1-reset.h and so on. What
>>>> happened?
>>>>
>>>> And I'd like to ask a question here before cooking 2nd version:
>>>> if the HW programming logic is the same as reset-simple, but some
>>>> or many bits are reserved, what's the can-be-accepted way to support
>>>> the reset controller? Use reset-simple? Obviously if we want the
>>>> "ID start at 0 and increment by 1" rule, then we have to write
>>>> a custom driver which almost use the reset-simple but with a
>>>> customized mapping.
>>>
>>> There are two possible situations. Either the reset specifier maps directly to
>>> something in the hardware, or you are inventing some brand new enumeration to
>>> use as a specifier.
>>>
>>> In the first situation, you do not need a header. We assume the user will look
>>> to the SoC documentation if they want to know what the numbers mean. (You aren't
>>> _creating_ an ABI, since the ABI is already defined by the hardware.)
>>>
>>> In the second situation, since we are inventing something new, the numbers
>>> should be contiguous. This is what Krzysztof's comment was about.
>>>
>>> For this reset device, the numbers are hardware bit offsets, so you are in the
>>> first situation. So I think the recommended solution here is to remove the
>>> header entirely and use the bit numbers directly in the SoC devicetree.
>>>
>>> It's still appropriate to use reset-simple. Adding some new mapping would make
>>> things more complicated for no benefit.
>>
>> Further, I think it is fine in that case to have a header, just the
>> header doesn't belong as a binding, and can instead go in the dts
>> directory.
>
> Hi Samuel, Conor,
>
> thanks a lot for the suggestion!
There is actually a thing here I missed. If this is a reset-simple
driver with dedicated SoC-specific compatible, you could want to have a
binding header to have IDs. This way later you can easily replace the
driver with another implementation, which does no rely on 1-1 mapping to
reset bits.
Therefore the reset-simple could be the exception where reset-bits could
have a meaning as binding header.
Best regards,
Krzysztof
^ permalink raw reply [flat|nested] 21+ messages in thread
end of thread, other threads:[~2023-11-15 21:00 UTC | newest]
Thread overview: 21+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2023-11-13 0:54 [PATCH 0/4] riscv: sophgo: add reset support for cv1800b Jisheng Zhang
2023-11-13 0:55 ` [PATCH 1/4] dt-bindings: reset: Add binding for Sophgo CV1800B reset controller Jisheng Zhang
2023-11-13 13:36 ` Conor Dooley
2023-11-13 14:00 ` Jisheng Zhang
2023-11-14 21:13 ` Krzysztof Kozlowski
2023-11-14 21:12 ` Krzysztof Kozlowski
2023-11-15 13:27 ` Jisheng Zhang
2023-11-15 14:56 ` Samuel Holland
2023-11-15 15:02 ` Conor Dooley
2023-11-15 15:15 ` Jisheng Zhang
2023-11-15 21:00 ` Krzysztof Kozlowski
2023-11-13 0:55 ` [PATCH 2/4] reset: Add reset controller support for Sophgo CV1800B SoC Jisheng Zhang
2023-11-13 0:55 ` [PATCH 3/4] riscv: dts: sophgo: add reset dt node for cv1800b Jisheng Zhang
2023-11-13 14:32 ` Yixun Lan
2023-11-13 15:14 ` Jisheng Zhang
2023-11-13 15:37 ` Samuel Holland
2023-11-14 14:55 ` Jisheng Zhang
2023-11-13 0:55 ` [PATCH 4/4] riscv: dts: sophgo: add reset phandle to all uart nodes Jisheng Zhang
2023-11-13 2:04 ` Samuel Holland
2023-11-13 13:09 ` Jisheng Zhang
2023-11-13 4:57 ` kernel test robot
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).