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 <[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
> > +#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 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 <[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
>>> +#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
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 <[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
> >>> +#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 <[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
> > >>> +#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 <[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
>>>>>> +#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