Re: [PATCH 1/4] dt-bindings: reset: Add binding for Sophgo CV1800B reset controller

[Samuel Holland]

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.

Regards,
Samuel