Re: [PATCH v2 2/5] dt-bindings: reset: thead,th1520-reset: Add controllers for more subsys

[Yao Zi <ziyao@disroot.org>]

On Fri, Oct 03, 2025 at 03:46:17PM -0700, Drew Fustini wrote:
> On Mon, Sep 15, 2025 at 09:53:28AM +0000, Yao Zi wrote:
> > TH1520 SoC is divided into several subsystems, most of them have
> > distinct reset controllers. Let's document reset controllers other than
> > the one for VO subsystem and IDs for their reset signals.
> > 
> > Signed-off-by: Yao Zi <ziyao@disroot.org>
> 
> Thanks for sending these patches.
> 
> > ---
> >  .../bindings/reset/thead,th1520-reset.yaml    |   8 +-
> >  .../dt-bindings/reset/thead,th1520-reset.h    | 216 ++++++++++++++++++
> >  2 files changed, 223 insertions(+), 1 deletion(-)
> > 
> [snip]
> > diff --git a/include/dt-bindings/reset/thead,th1520-reset.h b/include/dt-bindings/reset/thead,th1520-reset.h
> > index e51d6314d131..68ac52ed69de 100644
> > --- a/include/dt-bindings/reset/thead,th1520-reset.h
> > +++ b/include/dt-bindings/reset/thead,th1520-reset.h
> > @@ -7,6 +7,200 @@
> >  #ifndef _DT_BINDINGS_TH1520_RESET_H
> >  #define _DT_BINDINGS_TH1520_RESET_H
> [snip]
> > +/* DSP Subsystem */
> > +#define TH1520_RESET_ID_X2X_DSP1	0
> > +#define TH1520_RESET_ID_X2X_DSP0	1
> > +#define TH1520_RESET_ID_X2X_SLAVE_DSP1	2
> > +#define TH1520_RESET_ID_X2X_SLAVE_DSP0	3
> > +#define TH1520_RESET_ID_DSP0_CORE	4
> > +#define TH1520_RESET_ID_DSP0_DEBUG	5
> > +#define TH1520_RESET_ID_DSP0_APB	6
> > +#define TH1520_RESET_ID_DSP1_CORE	4
> > +#define TH1520_RESET_ID_DSP1_DEBUG	5
> > +#define TH1520_RESET_ID_DSP1_APB	6
> > +#define TH1520_RESET_ID_DSPSYS_APB	7
> > +#define TH1520_RESET_ID_AXI4_DSPSYS_SLV	8
> > +#define TH1520_RESET_ID_AXI4_DSPSYS	9
> > +#define TH1520_RESET_ID_AXI4_DSP_RS	10
> 
> This doesn't seem right. The numbers for each subsystem should not
> repeat. Here the DSP0 and DSP1 items have the same numbers: 4, 5, 6.
> 
> This causes both clang and sparse to complain. I think you can just
> change this so that TH1520_RESET_ID_DSP1_CORE is 7 and so on. The
> indexes don't really have any concrete meaning other than how they are
> used as unique keys.

You're correct, it's a copy-paste error, just like the one spotted in v1
of the series...

I'm not sure why either my GCC or sparse yielded no warning about them.
Will figure it out and send v3 with this fixed. Much sorry for these
stupid mistakes.

Best regards,
Yao Zi

> The warnings:
> 
>   AR      kernel/built-in.a
>   CC      drivers/reset/reset-th1520.o
> drivers/reset/reset-th1520.c:655:32: warning: initializer overrides prior initialization of this subobject [-Winitializer-overrides]
>   655 |         [TH1520_RESET_ID_DSP1_CORE] = {
>       |                                       ^
>   656 |                 .bit = BIT(12),
>       |                 ~~~~~~~~~~~~~~~
>   657 |                 .reg = TH1520_DSPSYS_RST_CFG,
>       |                 ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>   658 |         },
>       |         ~
> drivers/reset/reset-th1520.c:643:32: note: previous initialization is here
>   643 |         [TH1520_RESET_ID_DSP0_CORE] = {
>       |                                       ^
>   644 |                 .bit = BIT(8),
>       |                 ~~~~~~~~~~~~~~
>   645 |                 .reg = TH1520_DSPSYS_RST_CFG,
>       |                 ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>   646 |         },
>       |         ~
> drivers/reset/reset-th1520.c:659:33: warning: initializer overrides prior initialization of this subobject [-Winitializer-overrides]
>   659 |         [TH1520_RESET_ID_DSP1_DEBUG] = {
>       |                                        ^
>   660 |                 .bit = BIT(13),
>       |                 ~~~~~~~~~~~~~~~
>   661 |                 .reg = TH1520_DSPSYS_RST_CFG,
>       |                 ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>   662 |         },
>       |         ~
> drivers/reset/reset-th1520.c:647:33: note: previous initialization is here
>   647 |         [TH1520_RESET_ID_DSP0_DEBUG] = {
>       |                                        ^
>   648 |                 .bit = BIT(9),
>       |                 ~~~~~~~~~~~~~~
>   649 |                 .reg = TH1520_DSPSYS_RST_CFG,
>       |                 ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>   650 |         },
>       |         ~
> drivers/reset/reset-th1520.c:663:31: warning: initializer overrides prior initialization of this subobject [-Winitializer-overrides]
>   663 |         [TH1520_RESET_ID_DSP1_APB] = {
>       |                                      ^
>   664 |                 .bit = BIT(14),
>       |                 ~~~~~~~~~~~~~~~
>   665 |                 .reg = TH1520_DSPSYS_RST_CFG,
>       |                 ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>   666 |         },
>       |         ~
> drivers/reset/reset-th1520.c:651:31: note: previous initialization is here
>   651 |         [TH1520_RESET_ID_DSP0_APB] = {
>       |                                      ^
>   652 |                 .bit = BIT(10),
>       |                 ~~~~~~~~~~~~~~~
>   653 |                 .reg = TH1520_DSPSYS_RST_CFG,
>       |                 ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>   654 |         },
>       |         ~
> 3 warnings generated.
>   CHECK   drivers/reset/reset-th1520.c
> drivers/reset/reset-th1520.c:643:10: warning: Initializer entry defined twice
> drivers/reset/reset-th1520.c:655:10:   also defined here
> 
> 
> Thanks,
> Drew
> 
> _______________________________________________
> linux-riscv mailing list
> linux-riscv@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-riscv