From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from foss.arm.com (foss.arm.com [217.140.110.172]) by smtp.subspace.kernel.org (Postfix) with ESMTP id B8A6B1C04 for ; Tue, 16 Aug 2022 11:01:01 +0000 (UTC) Received: from usa-sjc-imap-foss1.foss.arm.com (unknown [10.121.207.14]) by usa-sjc-mx-foss1.foss.arm.com (Postfix) with ESMTP id C0A2E113E; Tue, 16 Aug 2022 04:01:01 -0700 (PDT) Received: from donnerap.cambridge.arm.com (usa-sjc-imap-foss1.foss.arm.com [10.121.207.14]) by usa-sjc-imap-foss1.foss.arm.com (Postfix) with ESMTPSA id 2960F3F70D; Tue, 16 Aug 2022 04:00:59 -0700 (PDT) Date: Tue, 16 Aug 2022 12:00:50 +0100 From: Andre Przywara To: Krzysztof Kozlowski Cc: Jernej =?UTF-8?B?xaBrcmFiZWM=?= , Samuel Holland , Chen-Yu Tsai , linux-sunxi@lists.linux.dev, Palmer Dabbelt , Paul Walmsley , Albert Ou , linux-riscv@lists.infradead.org, Heiko =?UTF-8?B?U3TDvGJuZXI=?= , Rob Herring , devicetree@vger.kernel.org, linux-kernel@vger.kernel.org, Krzysztof Kozlowski Subject: Re: [PATCH 06/12] riscv: dts: allwinner: Add the D1 SoC base devicetree Message-ID: <20220816120050.07dc2416@donnerap.cambridge.arm.com> In-Reply-To: References: <20220815050815.22340-1-samuel@sholland.org> <5593349.DvuYhMxLoT@jernej-laptop> <3881930.ZaRXLXkqSa@diego> <2249129.ElGaqSPkdT@jernej-laptop> Organization: ARM X-Mailer: Claws Mail 3.18.0 (GTK+ 2.24.32; aarch64-unknown-linux-gnu) Precedence: bulk X-Mailing-List: linux-sunxi@lists.linux.dev List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: quoted-printable On Tue, 16 Aug 2022 12:42:39 +0300 Krzysztof Kozlowski wrote: Hi, > On 16/08/2022 12:25, Jernej =C5=A0krabec wrote: > > Dne torek, 16. avgust 2022 ob 11:12:05 CEST je Heiko St=C3=BCbner napis= al(a): =20 > >> Am Dienstag, 16. August 2022, 09:49:58 CEST schrieb Jernej =C5=A0krabe= c: =20 > >>> Dne torek, 16. avgust 2022 ob 09:41:45 CEST je Krzysztof Kozlowski = =20 > > napisal(a): =20 > >>>> On 15/08/2022 08:08, Samuel Holland wrote: =20 > >>>>> + > >>>>> + de: display-engine { > >>>>> + compatible =3D "allwinner,sun20i-d1-display-engine"; > >>>>> + allwinner,pipelines =3D <&mixer0>, <&mixer1>; > >>>>> + status =3D "disabled"; > >>>>> + }; > >>>>> + > >>>>> + osc24M: osc24M-clk { =20 > >>>> > >>>> lowercase > >>>> =20 > >>>>> + compatible =3D "fixed-clock"; > >>>>> + clock-frequency =3D <24000000>; =20 > >>>> > >>>> This is a property of the board, not SoC. =20 > >>> > >>> SoC needs 24 MHz oscillator for correct operation, so each and every = board > >>> has it. Having it here simplifies board DT files. =20 > >> > >> I guess the oscillator is a separate component on each board, right? = =20 > >=20 > > Correct. > > =20 > >> And DT obvious is meant to describe the hardware - independently from > >> implementation-specific choices. =20 > >=20 > > There is no choice in this case. 24 MHz crystal has to be present. > >=20 > > FWIW, including crystal node in SoC specific DTSI is already common pat= tern in=20 > > Allwinner ARM SoC DTSI files. > > =20 > >> > >> Starting to discuss which exceptions to allow then might lead to even = more > >> exceptions. > >> > >> Also having to look for a board-component in the soc dtsi also is surp= rising > >> if one gets to the party later on :-) . =20 > >=20 > > As I said, if one is accustomed to Allwinner ARM DT development, it wou= ld be=20 > > more surprising to include 24 MHz crystal node in each and every board = DT. =20 >=20 > It's same everywhere. Allwinner, Exynos, iMX, Qualcomm. Everywhere this > is a part of the board, so even if oscillator frequency is fixed (as in > 99% of cases although some SoCs I think might just allow to implement > one of few), still this is a property of the board. Because: > 1. DTSI describes the SoC part, not board. > 2. So the DTS developer is a bit more conscious about his design. 1) is certainly true, but indeed most platforms put the base crystal oscillator in the SoC .dtsi: I just sampled Rockchip (rk3399.dtsi, rk356x.dtsi, rk3328.dtsi), Amlogic (meson-g12-common.dtsi), ActionSemi (s[7= 9]00.dtsi), Qualcomm (msm8916.dtsi, sm8450.dtsi, sc7180.dtsi), Freescale (imx8mm.dtsi, imx8qxp.dtsi), Realtek (rtd129x.dtsi), Broadcom (bcm283x.dtsi), Mediatek (mt8183.dtsi, mt8516.dtsi). The list probably goes on (I just stopped here). I think one reason might be that this is so central to the whole SoC operation, that it's already referenced multiple times in the base .dtsi. And having a yet unresolved reference in the .dtsi looks dodgy. NVidia seems to omit a base oscillator (maybe it's implicit in their binding design), Marvell doesn't use a fixed-clock (but still puts their base clock in armada-37xx.dtsi). Exynos and Renesas put a *stub* fixed-clock in the .dtsi, and set the frequency in the board .dts files. Would this be a compromise? Cheers, Andre > Keeping things in SoC DTSI just because it simplifies DTS is not correct > IMHO. So again - like in several other cases - minimum the frequency is > property of the board, not the SoC DTSI. >=20 > Everywhere. Allwinner is not special to receive exceptions. >=20 > Best regards, > Krzysztof >=20