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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date: Fri, 19 Jan 2024 17:04:36 +0000
From: Conor Dooley <conor@...nel.org>
To: AngeloGioacchino Del Regno <angelogioacchino.delregno@...labora.com>
Cc: Frank Wunderlich <frank-w@...lic-files.de>,
	Frank Wunderlich <linux@...web.de>,
	Michael Turquette <mturquette@...libre.com>,
	Stephen Boyd <sboyd@...nel.org>,
	Matthias Brugger <matthias.bgg@...il.com>,
	Philipp Zabel <p.zabel@...gutronix.de>,
	Rob Herring <robh+dt@...nel.org>,
	Krzysztof Kozlowski <krzysztof.kozlowski+dt@...aro.org>,
	Conor Dooley <conor+dt@...nel.org>,
	Sam Shih <sam.shih@...iatek.com>,
	Daniel Golle <daniel@...rotopia.org>, linux-clk@...r.kernel.org,
	linux-kernel@...r.kernel.org, linux-arm-kernel@...ts.infradead.org,
	devicetree@...r.kernel.org, linux-mediatek@...ts.infradead.org
Subject: Re: Aw: Re: [PATCH v3 1/2] dt-bindings: reset: mediatek: add MT7988
 reset IDs

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@...nel.org>
> > > On Wed, Jan 17, 2024 at 07:41:10PM +0100, Frank Wunderlich wrote:
> > > > From: Frank Wunderlich <frank-w@...lic-files.de>
> > > > 
> > > > Add reset constants for using as index in driver and dts.
> > > > 
> > > > Signed-off-by: Frank Wunderlich <frank-w@...lic-files.de>
> > > > ---
> > > > 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.

Download attachment "signature.asc" of type "application/pgp-signature" (229 bytes)