Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Wed, 22 Jun 2022 14:41:22 -0500
From:   Bjorn Andersson <bjorn.andersson@linaro.org>
To:     Stephan Gerhold <stephan@gerhold.net>
Cc:     Andy Gross <agross@kernel.org>,
        Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org>,
        Rob Herring <robh+dt@kernel.org>,
        Manivannan Sadhasivam <mani@kernel.org>,
        Jassi Brar <jassisinghbrar@gmail.com>,
        linux-arm-msm@vger.kernel.org, devicetree@vger.kernel.org,
        linux-kernel@vger.kernel.org
Subject: Re: [PATCH v2 3/6] arm64: dts: qcom: add SC8280XP platform
Message-ID: <YrNwYsbKG+ai5vFT@builder.lan>
References: <20220622041224.627803-1-bjorn.andersson@linaro.org>
 <20220622041224.627803-4-bjorn.andersson@linaro.org>
 <YrNl2jS3Stcl2DP8@gerhold.net>
MIME-Version: 1.0
Content-Type: text/plain; charset=windows-1252
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <YrNl2jS3Stcl2DP8@gerhold.net>
Precedence: bulk

On Wed 22 Jun 13:56 CDT 2022, Stephan Gerhold wrote:

> On Tue, Jun 21, 2022 at 09:12:21PM -0700, Bjorn Andersson wrote:
> > Introduce initial support for the Qualcomm SC8280XP platform, aka 8cx
> > Gen 3. This initial contribution supports SMP, CPUfreq, CPU cluster
> > idling, GCC, TLMM, SMMU, RPMh regulators, power-domains and clocks,
> > interconnects, some QUPs, UFS, remoteprocs, USB, watchdog, LLCC and
> > tsens.
> > 
> > Signed-off-by: Bjorn Andersson <bjorn.andersson@linaro.org>
> > 
> >  arch/arm64/boot/dts/qcom/sc8280xp.dtsi | 2145 ++++++++++++++++++++++++
> >  1 file changed, 2145 insertions(+)
> >  create mode 100644 arch/arm64/boot/dts/qcom/sc8280xp.dtsi
> > 
> > diff --git a/arch/arm64/boot/dts/qcom/sc8280xp.dtsi b/arch/arm64/boot/dts/qcom/sc8280xp.dtsi
> > new file mode 100644
> > index 000000000000..ac13965a181e
> > --- /dev/null
> > +++ b/arch/arm64/boot/dts/qcom/sc8280xp.dtsi
> [...]
> > +	reserved-memory {
> > +		#address-cells = <2>;
> > +		#size-cells = <2>;
> > +		ranges;
> > +
> > +		memory@80000000 {
> > +			reg = <0 0x80000000 0 0x860000>;
> > +			no-map;
> > +		};
> > +
> > +		cmd_db: memory@80860000 {
> > +			compatible = "qcom,cmd-db";
> > +			reg = <0 0x80860000 0 0x20000>;
> > +			no-map;
> > +		};
> > +
> > +		memory@80880000 {
> > +			reg = <0 0x80880000 0 0x80000>;
> > +			no-map;
> > +		};
> > +
> > +		smem_mem: smem@80900000 {
> > +			compatible = "qcom,smem";
> > +			reg = <0 0x80900000 0 0x200000>;
> > +			no-map;
> > +			hwlocks = <&tcsr_mutex 3>;
> > +		};
> > +
> > +		memory@80b00000 {
> > +			reg = <0 0x80b00000 0 0x100000>;
> > +			no-map;
> > +		};
> > +
> > +		memory@83b00000 {
> > +			reg = <0 0x83b00000 0 0x1700000>;
> > +			no-map;
> > +		};
> > +
> > +		memory@85b00000 {
> > +			reg = <0 0x85b00000 0 0xc00000>;
> > +			no-map;
> > +		};
> > +
> > +		pil_adsp_mem: memory@86c00000 {
> > +			reg = <0 0x86c00000 0 0x2000000>;
> > +			no-map;
> > +		};
> > +
> > +		pil_nsp0_mem: memory@8a100000 {
> > +			reg = <0 0x8a100000 0 0x1e00000>;
> > +			no-map;
> > +		};
> > +
> > +		pil_nsp1_mem: memory@8c600000 {
> > +			reg = <0 0x8c600000 0 0x1e00000>;
> > +			no-map;
> > +		};
> > +
> > +		memory@aeb00000 {
> > +			reg = <0 0xaeb00000 0 0x16600000>;
> > +			no-map;
> > +		};
> 
> Doesn't memory@ still cause the dtbs_check warnings? Similar to
> 
> > soc/qcom/qcom,smem.example.dt.yaml: memory@fa00000: 'device_type' is a required property
> >         From schema: dtschema/schemas/memory.yaml
> 
> as in [1]. If I understood it correctly there Rob said that memory@
> shouldn't be used for reserved-memory. Perhaps even reserved-memory@
> might be better then.
> 
> The device tree specification on the other hand suggests using the
> purpose of the reserved memory, like we did on older SoCs:
> 
> > 3.5.2 /reserved-memory/ child nodes
> > Following the generic-names recommended practice, node names should
> > reflect the purpose of the node (ie. ?framebuffer? or ?dma-pool?).
> 
> [1]: https://lore.kernel.org/linux-arm-msm/CAL_Jsq+66j8Y5y+PQ+mezkaxN1pfHFKz524YUF4Lz_OU5E-mZQ@mail.gmail.com/
> 

Thanks for the pointer. I stared at these for a while given that we
"shouldn't use memory@", but like with the other platforms I figured we
could fix it later...

I'll update these accordingly.

> > +		timer@17c20000 {
> > +			compatible = "arm,armv7-timer-mem";
> > +			#address-cells = <2>;
> > +			#size-cells = <2>;
> > +			ranges;
> > +			reg = <0x0 0x17c20000 0x0 0x1000>;
> > +			clock-frequency = <19200000>;
> [...]
> > +		};
> > +	timer {
> > +		compatible = "arm,armv8-timer";
> > +		interrupts = <GIC_PPI 13 (GIC_CPU_MASK_SIMPLE(8) | IRQ_TYPE_LEVEL_LOW)>,
> > +			     <GIC_PPI 14 (GIC_CPU_MASK_SIMPLE(8) | IRQ_TYPE_LEVEL_LOW)>,
> > +			     <GIC_PPI 11 (GIC_CPU_MASK_SIMPLE(8) | IRQ_TYPE_LEVEL_LOW)>,
> > +			     <GIC_PPI 10 (GIC_CPU_MASK_SIMPLE(8) | IRQ_TYPE_LEVEL_LOW)>;
> > +		clock-frequency = <19200000>;
> > +	};
> 
> Is the "clock-frequency" really needed for these two?
> 
> The binding docs are pretty clear that this should be configured by the
> firmware instead:
> 
> > Should be present only where necessary to work around broken firmware
> > which does not configure CNTFRQ on all CPUs to a uniform correct
> > value. Use of this property is strongly discouraged; fix your firmware
> > unless absolutely impossible.
> 
> I hope Qualcomm's firmware is actually improving on newer platforms
> and not making big steps backwards. :-)
> 

I believe I inherited this from somewhere, will check if it's actually
needed.

Thanks,
Bjorn