| 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
| ||
|
Message-Id: <DBE6TK1KDOTP.IIT72I1LUN5M@fairphone.com>
Date: Thu, 17 Jul 2025 10:29:35 +0200
From: "Luca Weiss" <luca.weiss@...rphone.com>
To: "Konrad Dybcio" <konrad.dybcio@....qualcomm.com>, "Will Deacon"
<will@...nel.org>, "Robin Murphy" <robin.murphy@....com>, "Joerg Roedel"
<joro@...tes.org>, "Rob Herring" <robh@...nel.org>, "Krzysztof Kozlowski"
<krzk+dt@...nel.org>, "Conor Dooley" <conor+dt@...nel.org>, "Rafael J.
Wysocki" <rafael@...nel.org>, "Viresh Kumar" <viresh.kumar@...aro.org>,
"Manivannan Sadhasivam" <mani@...nel.org>, "Herbert Xu"
<herbert@...dor.apana.org.au>, "David S. Miller" <davem@...emloft.net>,
"Vinod Koul" <vkoul@...nel.org>, "Bjorn Andersson" <andersson@...nel.org>,
"Konrad Dybcio" <konradybcio@...nel.org>, "Robert Marko"
<robimarko@...il.com>, "Das Srinagesh" <quic_gurus@...cinc.com>, "Thomas
Gleixner" <tglx@...utronix.de>, "Jassi Brar" <jassisinghbrar@...il.com>,
"Amit Kucheria" <amitk@...nel.org>, "Thara Gopinath"
<thara.gopinath@...il.com>, "Daniel Lezcano" <daniel.lezcano@...aro.org>,
"Zhang Rui" <rui.zhang@...el.com>, "Lukasz Luba" <lukasz.luba@....com>,
"Ulf Hansson" <ulf.hansson@...aro.org>
Cc: <~postmarketos/upstreaming@...ts.sr.ht>, <phone-devel@...r.kernel.org>,
<linux-arm-kernel@...ts.infradead.org>, <iommu@...ts.linux.dev>,
<devicetree@...r.kernel.org>, <linux-kernel@...r.kernel.org>,
<linux-pm@...r.kernel.org>, <linux-arm-msm@...r.kernel.org>,
<linux-crypto@...r.kernel.org>, <dmaengine@...r.kernel.org>,
<linux-mmc@...r.kernel.org>
Subject: Re: [PATCH v2 14/15] arm64: dts: qcom: Add initial Milos dtsi
On Mon Jul 14, 2025 at 1:06 PM CEST, Konrad Dybcio wrote:
> On 7/13/25 10:05 AM, Luca Weiss wrote:
>> Add a devicetree description for the Milos SoC, which is for example
>> Snapdragon 7s Gen 3 (SM7635).
>>
>> Signed-off-by: Luca Weiss <luca.weiss@...rphone.com>
>> ---
>
> [...]
>
>> + cpu-map {
>> + cluster0 {
>> + core0 {
>> + cpu = <&cpu0>;
>> + };
>> +
>> + core1 {
>> + cpu = <&cpu1>;
>> + };
>> +
>> + core2 {
>> + cpu = <&cpu2>;
>> + };
>> +
>> + core3 {
>> + cpu = <&cpu3>;
>> + };
>> + };
>> +
>> + cluster1 {
>> + core0 {
>> + cpu = <&cpu4>;
>> + };
>> +
>> + core1 {
>> + cpu = <&cpu5>;
>> + };
>> +
>> + core2 {
>> + cpu = <&cpu6>;
>> + };
>> + };
>> +
>> + cluster2 {
>> + core0 {
>> + cpu = <&cpu7>;
>> + };
>> + };
>> + };
>
> I'm getting mixed information about the core topology..
>
> What does dmesg say wrt this line?
>
> CPU%u: Booted secondary processor 0x%010lx [0x%08x]\n
[ 0.003570] CPU1: Booted secondary processor 0x0000000100 [0x410fd801]
[ 0.004738] CPU2: Booted secondary processor 0x0000000200 [0x410fd801]
[ 0.005783] CPU3: Booted secondary processor 0x0000000300 [0x410fd801]
[ 0.007206] CPU4: Booted secondary processor 0x0000000400 [0x410fd811]
[ 0.008206] CPU5: Booted secondary processor 0x0000000500 [0x410fd811]
[ 0.009073] CPU6: Booted secondary processor 0x0000000600 [0x410fd811]
[ 0.010406] CPU7: Booted secondary processor 0x0000000700 [0x410fd811]
>
>> + pmu-a520 {
>> + compatible = "arm,cortex-a520-pmu";
>> + interrupts = <GIC_PPI 7 IRQ_TYPE_LEVEL_LOW>;
>> + };
>> +
>> + pmu-a720 {
>> + compatible = "arm,cortex-a720-pmu";
>> + interrupts = <GIC_PPI 7 IRQ_TYPE_LEVEL_LOW>;
>> + };
>
> See:
>
> 9ce52e908bd5 ("arm64: dts: qcom: sm8650: switch to interrupt-cells 4 to add PPI partitions")
> 2c06e0797c32 ("arm64: dts: qcom: sm8650: add PPI interrupt partitions for the ARM PMUs")
Sure, will take a look.
>
> [...]
>
>> + gcc: clock-controller@...000 {
>> + compatible = "qcom,milos-gcc";
>> + reg = <0x0 0x00100000 0x0 0x1f4200>;
>> +
>> + clocks = <&rpmhcc RPMH_CXO_CLK>,
>> + <&sleep_clk>,
>> + <0>, /* pcie_0_pipe_clk */
>> + <0>, /* pcie_1_pipe_clk */
>> + <0>, /* ufs_phy_rx_symbol_0_clk */
>> + <0>, /* ufs_phy_rx_symbol_1_clk */
>> + <0>, /* ufs_phy_tx_symbol_0_clk */
>> + <0>; /* usb3_phy_wrapper_gcc_usb30_pipe_clk */
>> + protected-clocks = <GCC_PCIE_1_AUX_CLK>, <GCC_PCIE_1_AUX_CLK_SRC>,
>> + <GCC_PCIE_1_CFG_AHB_CLK>, <GCC_PCIE_1_MSTR_AXI_CLK>,
>> + <GCC_PCIE_1_PHY_RCHNG_CLK>, <GCC_PCIE_1_PHY_RCHNG_CLK_SRC>,
>> + <GCC_PCIE_1_PIPE_CLK>, <GCC_PCIE_1_PIPE_CLK_SRC>,
>> + <GCC_PCIE_1_PIPE_DIV2_CLK>, <GCC_PCIE_1_PIPE_DIV2_CLK_SRC>,
>> + <GCC_PCIE_1_SLV_AXI_CLK>, <GCC_PCIE_1_SLV_Q2A_AXI_CLK>;
>
> Does access control disallow accessing these on your prod-fused
> device?
Hm, taking another look, this property should probably be moved to
device dts.
Downstream has this in volcano.dtsi but volcano6i.dtsi (QCM6690?) and
volcano6ip.dtsi (QCS6690?) have a /delete-property/ for this, because
they have PCIe available.
I don't think this has anything to do with secure boot fuses, but I
don't think I have tried enabling these clocks on my SB-off prototype.
>
> [...]
>
>> + usb_1: usb@...0000 {
>> + compatible = "qcom,milos-dwc3", "qcom,snps-dwc3";
>> + reg = <0x0 0x0a600000 0x0 0x10000>;
>
> size = 0xfc_000
Ack
>
> [...]
>
>> +
>> + clocks = <&gcc GCC_CFG_NOC_USB3_PRIM_AXI_CLK>,
>> + <&gcc GCC_USB30_PRIM_MASTER_CLK>,
>> + <&gcc GCC_AGGRE_USB3_PRIM_AXI_CLK>,
>> + <&gcc GCC_USB30_PRIM_SLEEP_CLK>,
>> + <&gcc GCC_USB30_PRIM_MOCK_UTMI_CLK>,
>> + <&rpmhcc RPMH_CXO_CLK>;
>> + clock-names = "cfg_noc",
>> + "core",
>> + "iface",
>> + "sleep",
>> + "mock_utmi",
>> + "xo";
>> +
>> + assigned-clocks = <&gcc GCC_USB30_PRIM_MOCK_UTMI_CLK>,
>> + <&gcc GCC_USB30_PRIM_MASTER_CLK>;
>> + assigned-clock-rates = <19200000>, <133333333>;
>
> Set the latter to 200000000 - your device doesn't have USB3, but the
> next person may lose their hair about tracking down why it doesn't
> work on theirs
Ah, I think I only checked the downstream reference which was patched to
be qcom,core-clk-rate = <133333333>; for FP6. The original file does
have:
qcom,core-clk-rate = <200000000>;
qcom,core-clk-rate-disconnected = <133333333>;
>
> [...]
>
>> + pdc: interrupt-controller@...0000 {
>> + compatible = "qcom,milos-pdc", "qcom,pdc";
>> + reg = <0x0 0x0b220000 0x0 0x30000>, <0x0 0x174000f0 0x0 0x64>;
>
> 1 per line, please
Ack
>
>> + interrupt-parent = <&intc>;
>> +
>> + qcom,pdc-ranges = <0 480 40>, <40 140 11>, <51 527 47>,
>> + <98 609 31>, <129 63 1>, <130 716 12>,
>> + <142 251 5>;
>> +
>> + #interrupt-cells = <2>;
>> + interrupt-controller;
>> + };
>> +
>> + tsens0: thermal-sensor@...8000 {
>> + compatible = "qcom,milos-tsens", "qcom,tsens-v2";
>> + reg = <0x0 0x0c228000 0x0 0x1ff>, /* TM */
>> + <0x0 0x0c222000 0x0 0x1ff>; /* SROT */
>
> drop the comments
>
> the sizes are 0x1000 for both regions for both controllers
Ack
>
>> +
>> + interrupts = <GIC_SPI 506 IRQ_TYPE_LEVEL_HIGH>,
>
> pdc 26
You mean replace <GIC_SPI 506 IRQ_TYPE_LEVEL_HIGH> with
<&pdc 26 IRQ_TYPE_LEVEL_HIGH> (plus interrupts-extended)?
I assume you got this from internal docs, but just to mention,
volcano-thermal.dtsi contains GIC_SPI 506 (+ 507 for tsens1).
>
>> + <GIC_SPI 640 IRQ_TYPE_LEVEL_HIGH>;
>> + interrupt-names = "uplow",
>> + "critical";
>> +
>> + #qcom,sensors = <15>;
>> +
>> + #thermal-sensor-cells = <1>;
>> + };
>> +
>> + tsens1: thermal-sensor@...9000 {
>> + compatible = "qcom,milos-tsens", "qcom,tsens-v2";
>> + reg = <0x0 0x0c229000 0x0 0x1ff>, /* TM */
>> + <0x0 0x0c223000 0x0 0x1ff>; /* SROT */
>> +
>> + interrupts = <GIC_SPI 507 IRQ_TYPE_LEVEL_HIGH>,
>
> pdc 27
same as above
>
>> + <GIC_SPI 641 IRQ_TYPE_LEVEL_HIGH>;
>> + interrupt-names = "uplow",
>> + "critical";
>> +
>> + #qcom,sensors = <14>;
>> +
>> + #thermal-sensor-cells = <1>;
>> + };
>> +
>> + aoss_qmp: power-management@...0000 {
>> + compatible = "qcom,milos-aoss-qmp", "qcom,aoss-qmp";
>> + reg = <0x0 0x0c300000 0x0 0x400>;
>> +
>> + interrupt-parent = <&ipcc>;
>> + interrupts-extended = <&ipcc IPCC_CLIENT_AOP IPCC_MPROC_SIGNAL_GLINK_QMP
>> + IRQ_TYPE_EDGE_RISING>;
>> +
>> + mboxes = <&ipcc IPCC_CLIENT_AOP IPCC_MPROC_SIGNAL_GLINK_QMP>;
>> +
>> + #clock-cells = <0>;
>> + };
>> +
>> + sram@...0000 {
>> + compatible = "qcom,rpmh-stats";
>> + reg = <0x0 0x0c3f0000 0x0 0x400>;
>> + };
>> +
>> + spmi_bus: spmi@...0000 {
>> + compatible = "qcom,spmi-pmic-arb";
>
> There's two bus instances on this platform, check out the x1e binding
Will do
>
> [...]
>
>> + intc: interrupt-controller@...00000 {
>> + compatible = "arm,gic-v3";
>> + reg = <0x0 0x17100000 0x0 0x10000>, /* GICD */
>> + <0x0 0x17180000 0x0 0x200000>; /* GICR * 8 */
>
> drop the comments please
Ack
>
> [...]
>
>> + clocks = <&rpmhcc RPMH_CXO_CLK>, <&gcc GCC_GPLL0>;
>> + clock-names = "xo", "alternate";
>
> 1 a line, please
Ack
>
> [...]
>
>> + cpuss0-thermal {
>> + thermal-sensors = <&tsens0 1>;
>> +
>> + trips {
>> + cpuss0-hot {
>> + temperature = <110000>;
>> + hysteresis = <1000>;
>> + type = "hot";
>> + };
>> +
>> + cpuss0-critical {
>> + temperature = <115000>;
>> + hysteresis = <0>;
>> + type = "critical";
>> + };
>> + };
>> + };
>
> See:
>
> 06eadce93697 ("arm64: dts: qcom: x1e80100: Drop unused passive thermal trip points for CPU")
>
> (tldr drop non-critical trips for CPU)
Will take a look.
Regards
Luca
>
> Konrad
Powered by blists - more mailing lists