Re: Aw: Re: [PATCH v3 1/2] dt-bindings: reset: mediatek: add MT7988 reset IDs

[Conor Dooley]

On Fri, Jan 19, 2024 at 10:28:30AM +0100, AngeloGioacchino Del Regno wrote:
> Il 18/01/24 23:00, Frank Wunderlich ha scritto:
> > > Gesendet: Donnerstag, 18. Januar 2024 um 17:49 Uhr
> > > Von: "Conor Dooley" <conor@xxxxxxxxxx>
> > > On Wed, Jan 17, 2024 at 07:41:10PM +0100, Frank Wunderlich wrote:
> > > > From: Frank Wunderlich <frank-w@xxxxxxxxxxxxxxx>
> > > > 
> > > > Add reset constants for using as index in driver and dts.
> > > > 
> > > > Signed-off-by: Frank Wunderlich <frank-w@xxxxxxxxxxxxxxx>
> > > > ---
> > > > v3:
> > > > - add pcie reset id as suggested by angelo
> > > > 
> > > > v2:
> > > >   - add missing commit message and SoB
> > > >   - change value of infrareset to 0
> > > > ---
> > > >   include/dt-bindings/reset/mediatek,mt7988-resets.h | 6 ++++++
> > > >   1 file changed, 6 insertions(+)
> > > > 
> > > > diff --git a/include/dt-bindings/reset/mediatek,mt7988-resets.h b/include/dt-bindings/reset/mediatek,mt7988-resets.h
> > > > index 493301971367..0eb152889a89 100644
> > > > --- a/include/dt-bindings/reset/mediatek,mt7988-resets.h
> > > > +++ b/include/dt-bindings/reset/mediatek,mt7988-resets.h
> > > > @@ -10,4 +10,10 @@
> > > >   /* ETHWARP resets */
> > > >   #define MT7988_ETHWARP_RST_SWITCH		0
> > > > 
> > > > +/* INFRA resets */
> > > > +#define MT7988_INFRA_RST0_PEXTP_MAC_SWRST	0
> > > > +#define MT7988_INFRA_RST1_THERM_CTRL_SWRST	1
> > > 
> > > These are just "random" numbers, why not continue the numbering from the
> > > ETHWARP?
> > 
> > i can do...basicly these consts are used in DTS and driver only as index.
> > 
> > @angelo what do you think? I though also in leaving some space to allow grouping RST0 and RST1
> > when more consts are added, else the numbers are mixed up.
> > 
> > so e.g. let RST0 start at 20 and RST1 at 40 (or even higher, because RST0 and RST1 can have up to 32 resets).
> > That will allow adding more reset constants between my values and having raising numbers.
> 
> The resets are organized on a per-reset-controller basis, so, the ETHWARP
> reset controller's first reset is RST_SWITCH, the second one is RST_something_else,
> etc. while the first reset of the INFRA reset controller is PEXTP_MAC_SWRST.
> 
> That's why ETHWARP has a reset index 0 and INFRA also starts at 0.
> I think that the numbering is good as it is, and having one driver start at index 5
> while the other starts at index 12 would only overcomplicate registering the resets
> in each driver, or waste bytes by making unnecessarily large arrays, for (imo) no
> good reason.
> 
> This is one header, but it should "in theory" be more than one... so we would have
> one for each hardware block - but that'd make the reset directory over-crowded, as
> other MediaTek SoCs have got even more resets in even more hardware blocks than the
> MT7988. That'd be something like ~4 reset headers per SoC (and will increase with
> newer ones)...
> ...and this is why we have one binding header for resets.

That's okay. The commit message leaves me, who clearly isn't a mediatek
guy, with no information as to why these are not one contiguous set.
IMO being for different reset controllers entirely is fine.

> On the topic of leaving space to allow grouping RST0/RST1: -> No. <-
> The indices have to start from zero and have to be sequential, with no holes.

Agreed.

Attachment: signature.asc
Description: PGP signature