linux-kernel - Re: [PATCH v2 3/6] arm64: dts: qcom: add SC8280XP platform

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:   Wed, 22 Jun 2022 20:56:26 +0200
From:   Stephan Gerhold <stephan@...hold.net>
To:     Bjorn Andersson <bjorn.andersson@...aro.org>
Cc:     Andy Gross <agross@...nel.org>,
        Krzysztof Kozlowski <krzysztof.kozlowski+dt@...aro.org>,
        Rob Herring <robh+dt@...nel.org>,
        Manivannan Sadhasivam <mani@...nel.org>,
        Jassi Brar <jassisinghbrar@...il.com>,
        linux-arm-msm@...r.kernel.org, devicetree@...r.kernel.org,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2 3/6] arm64: dts: qcom: add SC8280XP platform

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@...aro.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@...00000 {
> +			reg = <0 0x80000000 0 0x860000>;
> +			no-map;
> +		};
> +
> +		cmd_db: memory@...60000 {
> +			compatible = "qcom,cmd-db";
> +			reg = <0 0x80860000 0 0x20000>;
> +			no-map;
> +		};
> +
> +		memory@...80000 {
> +			reg = <0 0x80880000 0 0x80000>;
> +			no-map;
> +		};
> +
> +		smem_mem: smem@...00000 {
> +			compatible = "qcom,smem";
> +			reg = <0 0x80900000 0 0x200000>;
> +			no-map;
> +			hwlocks = <&tcsr_mutex 3>;
> +		};
> +
> +		memory@...00000 {
> +			reg = <0 0x80b00000 0 0x100000>;
> +			no-map;
> +		};
> +
> +		memory@...00000 {
> +			reg = <0 0x83b00000 0 0x1700000>;
> +			no-map;
> +		};
> +
> +		memory@...00000 {
> +			reg = <0 0x85b00000 0 0xc00000>;
> +			no-map;
> +		};
> +
> +		pil_adsp_mem: memory@...00000 {
> +			reg = <0 0x86c00000 0 0x2000000>;
> +			no-map;
> +		};
> +
> +		pil_nsp0_mem: memory@...00000 {
> +			reg = <0 0x8a100000 0 0x1e00000>;
> +			no-map;
> +		};
> +
> +		pil_nsp1_mem: memory@...00000 {
> +			reg = <0 0x8c600000 0 0x1e00000>;
> +			no-map;
> +		};
> +
> +		memory@...00000 {
> +			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@...0000: '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/

> +		timer@...20000 {
> +			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. :-)

Thanks,
Stephan