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@xxxxxxxxxx> >>>>>> + */ >>>>>> + >>>>>> +#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
