Received-SPF: pass (google.com: domain of linux-kernel+bounces-31390-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.199.223 as permitted sender) client-ip=147.75.199.223;
Date: Fri, 19 Jan 2024 17:04:36 +0000
From: Conor Dooley <conor@kernel.org>
To: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
Cc: Frank Wunderlich <frank-w@public-files.de>,
	Frank Wunderlich <linux@fw-web.de>,
	Michael Turquette <mturquette@baylibre.com>,
	Stephen Boyd <sboyd@kernel.org>,
	Matthias Brugger <matthias.bgg@gmail.com>,
	Philipp Zabel <p.zabel@pengutronix.de>,
	Rob Herring <robh+dt@kernel.org>,
	Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org>,
	Conor Dooley <conor+dt@kernel.org>,
	Sam Shih <sam.shih@mediatek.com>,
	Daniel Golle <daniel@makrotopia.org>, linux-clk@vger.kernel.org,
	linux-kernel@vger.kernel.org, linux-arm-kernel@lists.infradead.org,
	devicetree@vger.kernel.org, linux-mediatek@lists.infradead.org
Subject: Re: Aw: Re: [PATCH v3 1/2] dt-bindings: reset: mediatek: add MT7988
 reset IDs
Message-ID: <20240119-dupe-obligate-707b3a01b356@spud>
References: <20240117184111.62371-1-linux@fw-web.de>
 <20240117184111.62371-2-linux@fw-web.de>
 <20240118-calcium-krypton-3c787b8d1912@spud>
 <trinity-afc4f48e-65e1-46ee-a78b-1d670cc0f310-1705615200900@3c-app-gmx-bap21>
 <43f946cc-07e1-48c5-9b31-40fc9bc93037@collabora.com>
Precedence: bulk
MIME-Version: 1.0
Content-Type: multipart/signed; micalg=pgp-sha256;
	protocol="application/pgp-signature"; boundary="hqUNFAuri8IfMYKy"
Content-Disposition: inline
In-Reply-To: <43f946cc-07e1-48c5-9b31-40fc9bc93037@collabora.com>


--hqUNFAuri8IfMYKy
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
Content-Transfer-Encoding: quoted-printable

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@kernel.org>
> > > On Wed, Jan 17, 2024 at 07:41:10PM +0100, Frank Wunderlich wrote:
> > > > From: Frank Wunderlich <frank-w@public-files.de>
> > > >=20
> > > > Add reset constants for using as index in driver and dts.
> > > >=20
> > > > Signed-off-by: Frank Wunderlich <frank-w@public-files.de>
> > > > ---
> > > > v3:
> > > > - add pcie reset id as suggested by angelo
> > > >=20
> > > > 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(+)
> > > >=20
> > > > diff --git a/include/dt-bindings/reset/mediatek,mt7988-resets.h b/i=
nclude/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
> > > >=20
> > > > +/* INFRA resets */
> > > > +#define MT7988_INFRA_RST0_PEXTP_MAC_SWRST	0
> > > > +#define MT7988_INFRA_RST1_THERM_CTRL_SWRST	1
> > >=20
> > > These are just "random" numbers, why not continue the numbering from =
the
> > > ETHWARP?
> >=20
> > i can do...basicly these consts are used in DTS and driver only as inde=
x.
> >=20
> > @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.
> >=20
> > so e.g. let RST0 start at 20 and RST1 at 40 (or even higher, because RS=
T0 and RST1 can have up to 32 resets).
> > That will allow adding more reset constants between my values and havin=
g raising numbers.
>=20
> 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_somet=
hing_else,
> etc. while the first reset of the INFRA reset controller is PEXTP_MAC_SWR=
ST.
>=20
> 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.
>=20
> This is one header, but it should "in theory" be more than one... so we w=
ould have
> one for each hardware block - but that'd make the reset directory over-cr=
owded, as
> other MediaTek SoCs have got even more resets in even more hardware block=
s than the
> MT7988. That'd be something like ~4 reset headers per SoC (and will incre=
ase 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 ho=
les.

Agreed.

--hqUNFAuri8IfMYKy
Content-Type: application/pgp-signature; name="signature.asc"

-----BEGIN PGP SIGNATURE-----

iHUEABYIAB0WIQRh246EGq/8RLhDjO14tDGHoIJi0gUCZaqrpAAKCRB4tDGHoIJi
0kpNAP0fXJ5CKQv/dcdDfDwM6QxpaHDnXco2vvD9zatS35PP3QEAyXeJjQ1mosMC
HzNIVUEk3x7CKsrxFYlJvszPHJz8fAs=
=leay
-----END PGP SIGNATURE-----

--hqUNFAuri8IfMYKy--