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

[Yao Zi <ziyao@disroot.org>]

On Sat, Oct 04, 2025 at 03:15:45PM -0700, Drew Fustini wrote:
> On Sat, Oct 04, 2025 at 02:21:43AM +0000, Yao Zi wrote:
> > 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
> 
> Have you tried using W=1?
> 
> I do see the warning in gcc with that:
> 
> pdp7@thelio:~/linux$ rm drivers/reset/reset-th1520.o
> pdp7@thelio:~/linux$ make W=1 CROSS_COMPILE=riscv64-linux-gnu- ARCH=riscv C=1 -j16
>   CALL    scripts/checksyscalls.sh
> Documentation/.renames.txt: warning: ignored by one of the .gitignore files
>   CC      drivers/reset/reset-th1520.o
> drivers/reset/reset-th1520.c:655:39: warning: initialized field overwritten [-Woverride-init]
>   655 |         [TH1520_RESET_ID_DSP1_CORE] = {
>       |                                       ^
> drivers/reset/reset-th1520.c:655:39: note: (near initialization for ‘th1520_dsp_resets[4]’)
> drivers/reset/reset-th1520.c:659:40: warning: initialized field overwritten [-Woverride-init]
>   659 |         [TH1520_RESET_ID_DSP1_DEBUG] = {
>       |                                        ^
> drivers/reset/reset-th1520.c:659:40: note: (near initialization for ‘th1520_dsp_resets[5]’)
> drivers/reset/reset-th1520.c:663:38: warning: initialized field overwritten [-Woverride-init]
>   663 |         [TH1520_RESET_ID_DSP1_APB] = {
>       |                                      ^
> drivers/reset/reset-th1520.c:663:38: note: (near initialization for ‘th1520_dsp_resets[6]’)
>   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, building with W=1 does spot the issue. Somehow my local setup of
sparse cannot find the duplicated IDs for either v1 or v2 of this
series, but GCC with W=1 could.

Will send v3 of the series with these duplicated IDs fixed. Really
thanks for finding the issue.

Best regards
Yao Zi