| Message ID | 20231113005503.2423-1-jszhang@kernel.org |
|---|---|
| Headers | show |
| Series | riscv: sophgo: add reset support for cv1800b | expand |
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 >
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
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
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
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.
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
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