riscv: sophgo: add reset support for cv1800b
cover riscv: sophgo: add reset support for cv1800b
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
Signature · 1 line
2.42.0
PATCH 1/4 dt-bindings: reset: Add binding for Sophgo CV1800B reset controller
Add devicetree binding for Sophgo CV1800B SoC reset controller.
--- .../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
Documentation/devicetree/bindings/reset/sophgo,cv1800b-reset.yaml new file +38 -0
@@ -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 <[email protected]> + +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>; + }; + +...
include/dt-bindings/reset/sophgo,cv1800b-reset.h new file +96 -0
96 lines not on this page · raw message
+#endif /* _DT_BINDINGS_CV1800B_RESET_H */
Signature · 1 line
2.42.0
Re: [PATCH 1/4] dt-bindings: reset: Add binding for Sophgo CV1800B reset controller
Quoted 3 lines · Jisheng Zhang
Add devicetree binding for Sophgo CV1800B SoC reset controller.
Signed-off-by Jisheng Zhang <[email protected]>
With the unterminated ifndef that was pointed out by the robots fixed,
include/dt-bindings/reset/sophgo,cv1800b-reset.h line 93
Quoted 12 lines
include/dt-bindings/reset/sophgo,cv1800b-reset.h new file +12
-0+/* 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.
Re: [PATCH 1/4] dt-bindings: reset: Add binding for Sophgo CV1800B reset controller
Quoted 8 lines · Conor Dooley
Quoted 3 lines · Jisheng Zhang
Add devicetree binding for Sophgo CV1800B SoC reset controller.
Signed-off-by Jisheng Zhang <[email protected]>With the unterminated ifndef that was pointed out by the robots fixed,
Reviewed-by Conor Dooley <[email protected]>
Quoted 14 lines
include/dt-bindings/reset/sophgo,cv1800b-reset.h line 93
Quoted 12 lines
include/dt-bindings/reset/sophgo,cv1800b-reset.h new file +12
-0+/* 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.
Quoted 3 lines
Thanks,
Conor.
Re: [PATCH 1/4] dt-bindings: reset: Add binding for Sophgo CV1800B reset controller
Quoted 24 lines · Jisheng Zhang
Quoted 8 lines · Conor Dooley
Quoted 3 lines · Jisheng Zhang
Add devicetree binding for Sophgo CV1800B SoC reset controller.
Signed-off-by Jisheng Zhang <[email protected]>With the unterminated ifndef that was pointed out by the robots fixed,
Reviewed-by Conor Dooley <[email protected]>Quoted 14 lines
include/dt-bindings/reset/sophgo,cv1800b-reset.h line 93
Quoted 12 lines
include/dt-bindings/reset/sophgo,cv1800b-reset.h new file +12
-0+/* 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?
Quoted 3 lines
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
Re: [PATCH 1/4] dt-bindings: reset: Add binding for Sophgo CV1800B reset controller
In reply to Jisheng ZhangOn 13/11/2023 01:55, Jisheng Zhang wrote:
...
Quoted 15 lines
include/dt-bindings/reset/sophgo,cv1800b-reset.h new file +9
-0@@ -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 <[email protected]> +*/ + +#ifndef _DT_BINDINGS_CV1800B_RESET_H +#define _DT_BINDINGS_CV1800B_RESET_H +
include/dt-bindings/reset/sophgo,cv1800b-reset.h line 19
Quoted 10 lines
include/dt-bindings/reset/sophgo,cv1800b-reset.h new file +10
-0+/* 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
Re: [PATCH 1/4] dt-bindings: reset: Add binding for Sophgo CV1800B reset controller
Quoted 22 lines · Krzysztof Kozlowski
On 13/11/2023 01:55, Jisheng Zhang wrote:
...Quoted 19 lines
include/dt-bindings/reset/sophgo,cv1800b-reset.h new file +13
-0@@ -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 <[email protected]> +*/ + +#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
Quoted 8 lines
include/dt-bindings/reset/sophgo,cv1800b-reset.h line 19
Quoted 6 lines
include/dt-bindings/reset/sophgo,cv1800b-reset.h new file +6
-0+#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
Re: [PATCH 1/4] dt-bindings: reset: Add binding for Sophgo CV1800B reset controller
Quoted 39 lines · Jisheng Zhang
Quoted 22 lines · Krzysztof Kozlowski
On 13/11/2023 01:55, Jisheng Zhang wrote:
...Quoted 19 lines
include/dt-bindings/reset/sophgo,cv1800b-reset.h new file +13
-0@@ -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 <[email protected]> +*/ + +#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 4Quoted 8 lines
include/dt-bindings/reset/sophgo,cv1800b-reset.h line 19
Quoted 6 lines
include/dt-bindings/reset/sophgo,cv1800b-reset.h new file +6
-0+#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?
Quoted 7 lines
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
Re: [PATCH 1/4] dt-bindings: reset: Add binding for Sophgo CV1800B reset controller
Quoted 59 lines · Samuel Holland
Quoted 39 lines · Jisheng Zhang
Quoted 22 lines · Krzysztof Kozlowski
On 13/11/2023 01:55, Jisheng Zhang wrote:
...Quoted 19 lines
include/dt-bindings/reset/sophgo,cv1800b-reset.h new file +13
-0@@ -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 <[email protected]> +*/ + +#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 4Quoted 8 lines
include/dt-bindings/reset/sophgo,cv1800b-reset.h line 19
Quoted 6 lines
include/dt-bindings/reset/sophgo,cv1800b-reset.h new file +6
-0+#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?
Quoted 7 lines
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.
Quoted 6 lines
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.
Re: [PATCH 1/4] dt-bindings: reset: Add binding for Sophgo CV1800B reset controller
Quoted 64 lines · Conor Dooley
Quoted 63 lines · Samuel Holland
Quoted 39 lines · Jisheng Zhang
Quoted 22 lines · Krzysztof Kozlowski
On 13/11/2023 01:55, Jisheng Zhang wrote:
...Quoted 19 lines
include/dt-bindings/reset/sophgo,cv1800b-reset.h new file +13
-0@@ -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 <[email protected]> +*/ + +#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 4Quoted 8 lines
include/dt-bindings/reset/sophgo,cv1800b-reset.h line 19
Quoted 6 lines
include/dt-bindings/reset/sophgo,cv1800b-reset.h new file +6
-0+#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?
Quoted 7 lines
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.
Quoted 6 lines
Quoted 2 lines
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
Re: [PATCH 1/4] dt-bindings: reset: Add binding for Sophgo CV1800B reset controller
Quoted 68 lines · Jisheng Zhang
Quoted 67 lines · Conor Dooley
Quoted 65 lines · Samuel Holland
Quoted 39 lines · Jisheng Zhang
Quoted 22 lines · Krzysztof Kozlowski
On 13/11/2023 01:55, Jisheng Zhang wrote:
...Quoted 19 lines
include/dt-bindings/reset/sophgo,cv1800b-reset.h new file +13
-0@@ -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 <[email protected]> +*/ + +#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 4Quoted 8 lines
include/dt-bindings/reset/sophgo,cv1800b-reset.h line 19
Quoted 6 lines
include/dt-bindings/reset/sophgo,cv1800b-reset.h new file +6
-0+#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?
Quoted 7 lines
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.
Quoted 7 lines
Quoted 3 lines
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
PATCH 2/4 reset: Add reset controller support for Sophgo CV1800B SoC
In reply to Jisheng ZhangAdd reset controller support for Sophgo CV1800B SoC reusing the reset-simple driver.
--- drivers/reset/Kconfig | 3 ++- drivers/reset/reset-simple.c | 2 ++ 2 files changed, 4 insertions(+), 1 deletion(-)
drivers/reset/Kconfig +2 -1
@@ -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)
drivers/reset/reset-simple.c +2 -0
@@ -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 */ }, };
Signature · 1 line
2.42.0
PATCH 3/4 riscv: dts: sophgo: add reset dt node for cv1800b
In reply to Jisheng ZhangAdd the reset device tree node to cv1800b SoC.
--- arch/riscv/boot/dts/sophgo/cv1800b.dtsi | 6 ++++++ 1 file changed, 6 insertions(+)
arch/riscv/boot/dts/sophgo/cv1800b.dtsi +6 -0
@@ -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>;
Signature · 1 line
2.42.0
Re: [PATCH 3/4] riscv: dts: sophgo: add reset dt node for cv1800b
Hi Jisheng:
Quoted 12 lines · Jisheng Zhang
Add the reset device tree node to cv1800b SoC.
Signed-off-by Jisheng Zhang <[email protected]>--- arch/riscv/boot/dts/sophgo/cv1800b.dtsi | 6 ++++++ 1 file changed, 6 insertions(+)arch/riscv/boot/dts/sophgo/cv1800b.dtsi +0
-0
arch/riscv/boot/dts/sophgo/cv1800b.dtsi soc { · line 59
Quoted 6 lines
arch/riscv/boot/dts/sophgo/cv1800b.dtsi +3
-0@@ -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?
Quoted 9 lines
arch/riscv/boot/dts/sophgo/cv1800b.dtsi +3
-0+ #reset-cells = <1>; + }; + uart0: serial@4140000 { compatible = "snps,dw-apb-uart"; reg = <0x04140000 0x100>;Signature · 1 line
2.42.0
Signature · 3 lines
Yixun Lan (dlan) Gentoo Linux Developer GPG Key ID AABEFD55
Re: [PATCH 3/4] riscv: dts: sophgo: add reset dt node for cv1800b
Quoted 1 line · Yixun Lan
Hi Jisheng:
Hi
Quoted 17 lines · Jisheng Zhang
Quoted 15 lines
Add the reset device tree node to cv1800b SoC.
Signed-off-by Jisheng Zhang <[email protected]>--- arch/riscv/boot/dts/sophgo/cv1800b.dtsi | 6 ++++++ 1 file changed, 6 insertions(+)arch/riscv/boot/dts/sophgo/cv1800b.dtsi +0
-0@@ -54,6 +54,12 @@ soc { dma-noncoherent; ranges;
Quoted 5 lines
arch/riscv/boot/dts/sophgo/cv1800b.dtsi soc { · line 59
Quoted 3 lines
arch/riscv/boot/dts/sophgo/cv1800b.dtsi +3
-0+ 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.
Quoted 3 lines
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.
Quoted 16 lines
so we should be fine with this?
Quoted 9 lines
arch/riscv/boot/dts/sophgo/cv1800b.dtsi +3
-0+ #reset-cells = <1>; + }; + uart0: serial@4140000 { compatible = "snps,dw-apb-uart"; reg = <0x04140000 0x100>;Signature · 1 line
2.42.0Signature · 3 lines
Yixun Lan (dlan) Gentoo Linux Developer GPG Key ID AABEFD55
Re: [PATCH 3/4] riscv: dts: sophgo: add reset dt node for cv1800b
Hi Jisheng,
Quoted 29 lines · Jisheng Zhang
Quoted 16 lines · Yixun Lan
Quoted 15 lines · Jisheng Zhang
Add the reset device tree node to cv1800b SoC.
Signed-off-by Jisheng Zhang <[email protected]>--- arch/riscv/boot/dts/sophgo/cv1800b.dtsi | 6 ++++++ 1 file changed, 6 insertions(+)arch/riscv/boot/dts/sophgo/cv1800b.dtsi +0
-0@@ -54,6 +54,12 @@ soc { dma-noncoherent; ranges;Quoted 5 lines
arch/riscv/boot/dts/sophgo/cv1800b.dtsi soc { · line 59
Quoted 3 lines
arch/riscv/boot/dts/sophgo/cv1800b.dtsi +3
-0+ rst: reset-controller@3003000 { + compatible = "sophgo,cv1800b-reset"; + reg = <0x03003000 0x1000>;~~~~~~~ it should be 0x28The reg space is 4KB, but only
0x28are used. I think0x1000or0x28are fine since the ioremap granule is 4kB.Quoted 3 lines
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,
Quoted 7 lines
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
Quoted 21 lines
Quoted 16 lines
so we should be fine with this?
Quoted 9 lines
arch/riscv/boot/dts/sophgo/cv1800b.dtsi +3
-0+ #reset-cells = <1>; + }; + uart0: serial@4140000 { compatible = "snps,dw-apb-uart"; reg = <0x04140000 0x100>;Signature · 1 line
2.42.0Signature · 3 lines
Yixun Lan (dlan) Gentoo Linux Developer GPG Key ID AABEFD55_______________________________________________
linux-riscv mailing list
[email protected]
http://lists.infradead.org/mailman/listinfo/linux-riscv
Re: [PATCH 3/4] riscv: dts: sophgo: add reset dt node for cv1800b
Quoted 1 line · Samuel Holland
Hi Jisheng,
Hi Samuel,
Quoted 35 lines · Jisheng Zhang
Quoted 33 lines · Yixun Lan
Quoted 16 lines · Jisheng Zhang
Quoted 15 lines
Add the reset device tree node to cv1800b SoC.
Signed-off-by Jisheng Zhang <[email protected]>--- arch/riscv/boot/dts/sophgo/cv1800b.dtsi | 6 ++++++ 1 file changed, 6 insertions(+)arch/riscv/boot/dts/sophgo/cv1800b.dtsi +0
-0@@ -54,6 +54,12 @@ soc { dma-noncoherent; ranges;Quoted 5 lines
arch/riscv/boot/dts/sophgo/cv1800b.dtsi soc { · line 59
Quoted 3 lines
arch/riscv/boot/dts/sophgo/cv1800b.dtsi +3
-0+ rst: reset-controller@3003000 { + compatible = "sophgo,cv1800b-reset"; + reg = <0x03003000 0x1000>;~~~~~~~ it should be 0x28The reg space is 4KB, but only
0x28are used. I think0x1000or0x28are fine since the ioremap granule is 4kB.Quoted 3 lines
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.
Quoted 7 lines
Quoted 3 lines
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_SIMPLEwith 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.
Quoted 26 lines
Regards,
SamuelQuoted 21 lines
Quoted 16 lines
so we should be fine with this?
Quoted 9 lines
arch/riscv/boot/dts/sophgo/cv1800b.dtsi +3
-0+ #reset-cells = <1>; + }; + uart0: serial@4140000 { compatible = "snps,dw-apb-uart"; reg = <0x04140000 0x100>;Signature · 1 line
2.42.0Signature · 3 lines
Yixun Lan (dlan) Gentoo Linux Developer GPG Key ID AABEFD55_______________________________________________
linux-riscv mailing list
[email protected]
http://lists.infradead.org/mailman/listinfo/linux-riscv
PATCH 4/4 riscv: dts: sophgo: add reset phandle to all uart nodes
In reply to Jisheng ZhangAlthough, the resets are deasserted by default. Add them for completeness.
--- arch/riscv/boot/dts/sophgo/cv1800b.dtsi | 6 ++++++ 1 file changed, 6 insertions(+)
arch/riscv/boot/dts/sophgo/cv1800b.dtsi +6 -0
@@ -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";
Signature · 1 line
2.42.0
Re: [PATCH 4/4] riscv: dts: sophgo: add reset phandle to all uart nodes
Hi Jisheng,
Quoted 21 lines · Jisheng Zhang
Although, the resets are deasserted by default. Add them for completeness.
Signed-off-by Jisheng Zhang <[email protected]>--- arch/riscv/boot/dts/sophgo/cv1800b.dtsi | 6 ++++++ 1 file changed, 6 insertions(+)arch/riscv/boot/dts/sophgo/cv1800b.dtsi +1
-0@@ -4,6 +4,7 @@ */ #include <dt-bindings/interrupt-controller/irq.h> +#include <dt-bindings/reset/sophgo,cv1800b-reset.h> / { compatible = "sophgo,cv1800b";
arch/riscv/boot/dts/sophgo/cv1800b.dtsi uart0: serial@4140000 { · line 69
Quoted 4 lines
arch/riscv/boot/dts/sophgo/cv1800b.dtsi +1
-0@@ -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
Quoted 35 lines
arch/riscv/boot/dts/sophgo/cv1800b.dtsi +4
-0reg-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";
Re: [PATCH 4/4] riscv: dts: sophgo: add reset phandle to all uart nodes
Quoted 33 lines · Samuel Holland
Hi Jisheng,
Quoted 21 lines · Jisheng Zhang
Although, the resets are deasserted by default. Add them for completeness.
Signed-off-by Jisheng Zhang <[email protected]>--- arch/riscv/boot/dts/sophgo/cv1800b.dtsi | 6 ++++++ 1 file changed, 6 insertions(+)arch/riscv/boot/dts/sophgo/cv1800b.dtsi +1
-0@@ -4,6 +4,7 @@ */ #include <dt-bindings/interrupt-controller/irq.h> +#include <dt-bindings/reset/sophgo,cv1800b-reset.h> / { compatible = "sophgo,cv1800b";arch/riscv/boot/dts/sophgo/cv1800b.dtsi uart0: serial@4140000 { · line 69
Quoted 4 lines
arch/riscv/boot/dts/sophgo/cv1800b.dtsi +1
-0@@ -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()in8250_dw.cbecause it has no driver for the reset controller.
Quoted 4 lines
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.
Quoted 40 lines
Regards,
SamuelQuoted 35 lines
arch/riscv/boot/dts/sophgo/cv1800b.dtsi +4
-0reg-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";
kernel test robot
build botRe: [PATCH 4/4] riscv: dts: sophgo: add reset phandle to all uart nodes
In reply to Jisheng Zhangkernel test robot noticed the following build errors:
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/[email protected]/config) compiler: riscv64-linux-gcc (GCC) 13.2.0 reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20231113/[email protected]/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 <[email protected]>
| Closes: https://lore.kernel.org/oe-kbuild-all/[email protected]/
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
Signature · 2 lines
0-DAY CI Kernel Test Service https://github.com/intel/lkp-tests/wiki