| 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: <00aa901b-5de9-4b72-a157-c54ad29df458@gmail.com>
Date: Tue, 4 Feb 2025 20:22:09 +0200
From: Ivaylo Ivanov <ivo.ivanov.ivanov1@...il.com>
To: Kaustabh Chakraborty <kauschluss@...root.org>,
Rob Herring <robh@...nel.org>, Krzysztof Kozlowski <krzk@...nel.org>,
Conor Dooley <conor@...nel.org>, Alim Akhtar <alim.akhtar@...sung.com>,
Kees Cook <kees@...nel.org>, Tony Luck <tony.luck@...el.com>,
"Guilherme G. Piccoli" <gpiccoli@...lia.com>
Cc: Sergey Lisov <sleirsgoevy@...il.com>, devicetree@...r.kernel.org,
linux-arm-kernel@...ts.infradead.org, linux-samsung-soc@...r.kernel.org,
linux-kernel@...r.kernel.org, linux-hardening@...r.kernel.org
Subject: Re: [PATCH v2 2/5] arm64: dts: exynos: add initial devicetree support
for exynos7870
On 2/3/25 22:46, Kaustabh Chakraborty wrote:
> Exynos7870 is an arm64 SoC manufactured by Samsung and announced in
> 2016. It is present in multiple mid-range Samsung phones and tablets.
>
> Add basic devicetree support for the SoC, which includes CMUs, pin
> controllers, I2C, UART, DW-MMC, and USB-DRD.
>
> Co-developed-by: Sergey Lisov <sleirsgoevy@...il.com>
> Signed-off-by: Sergey Lisov <sleirsgoevy@...il.com>
> Signed-off-by: Kaustabh Chakraborty <kauschluss@...root.org>
> ---
> arch/arm64/boot/dts/exynos/exynos7870-pinctrl.dtsi | 1035 ++++++++++++++++++++
> arch/arm64/boot/dts/exynos/exynos7870.dtsi | 722 ++++++++++++++
> 2 files changed, 1757 insertions(+)
>
> diff --git a/arch/arm64/boot/dts/exynos/exynos7870-pinctrl.dtsi b/arch/arm64/boot/dts/exynos/exynos7870-pinctrl.dtsi
> new file mode 100644
> index 0000000000000000000000000000000000000000..28ff409c4fdc5f766d92617ea2df7be2112c28d1
> --- /dev/null
> +++ b/arch/arm64/boot/dts/exynos/exynos7870-pinctrl.dtsi
> @@ -0,0 +1,1035 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Samsung Exynos7870 SoC pin-mux and pin-config device tree source
> + *
> + * Copyright (c) 2015 Samsung Electronics Co., Ltd.
> + */
> +
> +#include <dt-bindings/interrupt-controller/arm-gic.h>
> +#include "exynos-pinctrl.h"
> +
> +&pinctrl0 {
I haven't had enough time to look deeper, but these are my 2 cents:
Can you label them according to their block name rather than numbers?
For example, pinctrl_abox (make sure to keep them alphabetically sorted
as well).
> + etc0: etc0-gpio-bank {
> + gpio-controller;
> + #gpio-cells = <2>;
> +
> + interrupt-controller;
> + #interrupt-cells = <2>;
> + };
> +
> + etc1: etc1-gpio-bank {
> + gpio-controller;
> + #gpio-cells = <2>;
> +
> + interrupt-controller;
> + #interrupt-cells = <2>;
> + };
> +
> + gpa0: gpa0-gpio-bank {
> + gpio-controller;
> + #gpio-cells = <2>;
> +
> + interrupt-controller;
> + #interrupt-cells = <2>;
> + interrupt-parent = <&gic>;
> + interrupts = <GIC_SPI 0 IRQ_TYPE_LEVEL_HIGH>,
> + <GIC_SPI 1 IRQ_TYPE_LEVEL_HIGH>,
> + <GIC_SPI 2 IRQ_TYPE_LEVEL_HIGH>,
> + <GIC_SPI 3 IRQ_TYPE_LEVEL_HIGH>,
> + <GIC_SPI 4 IRQ_TYPE_LEVEL_HIGH>,
> + <GIC_SPI 5 IRQ_TYPE_LEVEL_HIGH>,
> + <GIC_SPI 6 IRQ_TYPE_LEVEL_HIGH>,
> + <GIC_SPI 7 IRQ_TYPE_LEVEL_HIGH>;
> + };
> +
> + gpa1: gpa1-gpio-bank {
> + gpio-controller;
> + #gpio-cells = <2>;
> +
> + interrupt-controller;
> + #interrupt-cells = <2>;
> + interrupt-parent = <&gic>;
> + interrupts = <GIC_SPI 8 IRQ_TYPE_LEVEL_HIGH>,
> + <GIC_SPI 9 IRQ_TYPE_LEVEL_HIGH>,
> + <GIC_SPI 10 IRQ_TYPE_LEVEL_HIGH>,
> + <GIC_SPI 11 IRQ_TYPE_LEVEL_HIGH>,
> + <GIC_SPI 12 IRQ_TYPE_LEVEL_HIGH>,
> + <GIC_SPI 13 IRQ_TYPE_LEVEL_HIGH>,
> + <GIC_SPI 14 IRQ_TYPE_LEVEL_HIGH>,
> + <GIC_SPI 15 IRQ_TYPE_LEVEL_HIGH>;
> + };
> +
> + gpa2: gpa2-gpio-bank {
> + gpio-controller;
> + #gpio-cells = <2>;
> +
> + interrupt-controller;
> + #interrupt-cells = <2>;
> + };
> +
> + gpq0: gpq0-gpio-bank {
> + gpio-controller;
> + #gpio-cells = <2>;
> +
> + interrupt-controller;
> + #interrupt-cells = <2>;
> + };
> +
> + uart2_bus: uart2-bus-pins {
> + samsung,pins = "gpa1-1", "gpa1-0";
> + samsung,pin-function = <EXYNOS_PIN_FUNC_2>;
> + samsung,pin-pud = <EXYNOS_PIN_PULL_NONE>;
> + };
> +
> + uart2_sleep: uart2-sleep-pins {
> + samsung,pins = "gpa1-1";
> + samsung,pin-function = <EXYNOS_PIN_FUNC_INPUT>;
> + samsung,pin-pud = <EXYNOS_PIN_PULL_DOWN>;
> + };
> +
> + bt_btwake: bt-btwake-pins {
> + samsung,pins = "gpa1-2";
> + samsung,pin-function = <EXYNOS_PIN_FUNC_OUTPUT>;
> + samsung,pin-con-pdn = <EXYNOS_PIN_PDN_PREV>;
> + samsung,pin-pud-pdn = <EXYNOS_PIN_PULL_NONE>;
> + samsung,pin-pud = <EXYNOS_PIN_PULL_NONE>;
> + };
> +
> + bt_hostwake: bt-hostwake-pins {
Sort these alphabetically, so you have:
all a-z-gpio-bank, after which all a-z-pin.
In this case bt_* can be first after all gpio bank nodes.
> + samsung,pins = "gpa1-6";
> + samsung,pin-function = <EXYNOS_PIN_FUNC_INPUT>;
> + samsung,pin-con-pdn = <EXYNOS_PIN_PDN_INPUT>;
> + samsung,pin-pud-pdn = <EXYNOS_PIN_PULL_NONE>;
> + samsung,pin-pud = <EXYNOS_PIN_PULL_NONE>;
> + };
> +
> + gnss_sensor_irq: gnss-sensor-irq-pins {
> + samsung,pins = "gpa2-3";
> + samsung,pin-function = <EXYNOS_PIN_FUNC_6>;
> + samsung,pin-pud = <EXYNOS_PIN_PULL_UP>;
> + };
> +
> + gnss_sensor_i2c: gnss-sensor-i2c-pins {
> + samsung,pins = "gpa2-5", "gpa2-4";
> + samsung,pin-function = <EXYNOS_PIN_FUNC_6>;
> + samsung,pin-pud = <EXYNOS_PIN_PULL_NONE>;
> + };
> +
...
> + #interrupt-cells = <2>;
> + };
> +
> + hs_i2c0_bus: hs-i2c0-bus-pins {
s/hs_i2c/hsi2c/g
> + samsung,pins = "gpm0-1", "gpm0-0";
> + samsung,pin-function = <EXYNOS_PIN_FUNC_2>;
> + samsung,pin-con-pdn = <EXYNOS_PIN_PDN_PREV>;
> + samsung,pin-pud = <EXYNOS_PIN_PULL_NONE>;
> + samsung,pin-drv = <EXYNOS5433_PIN_DRV_FAST_SR1>;
> + };
...
> diff --git a/arch/arm64/boot/dts/exynos/exynos7870.dtsi b/arch/arm64/boot/dts/exynos/exynos7870.dtsi
> new file mode 100644
> index 0000000000000000000000000000000000000000..11129e37fc86ebaee01684ed6841c932dd6cbc8a
> --- /dev/null
> +++ b/arch/arm64/boot/dts/exynos/exynos7870.dtsi
> @@ -0,0 +1,722 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Samsung Exynos7870 SoC device tree source
> + *
> + * Copyright (c) 2015 Samsung Electronics Co., Ltd.
> + */
> +
> +#include <dt-bindings/clock/exynos7870.h>
> +#include <dt-bindings/interrupt-controller/arm-gic.h>
> +#include <dt-bindings/soc/samsung,boot-mode.h>
> +
> +/ {
> + compatible = "samsung,exynos7870";
> + #address-cells = <2>;
> + #size-cells = <1>;
> +
> + /* Samsung's bootloader (S-BOOT) checks for these DT properties */
> + model_info-hw_rev = <0>;
> + model_info-hw_rev_end = <255>;
> + model_info-chip = <7870>;
You could skip the properties with a shim bootloader like uniLoader. Up to
Krzysztof to decide if you should keep these.
> +
...
> + };
> +
> + oscclk: oscclk {
> + compatible = "fixed-clock";
> + #clock-cells = <0>;
> +
> + clock-frequency = <26000000>;
Frequency must be defined in the board device tree.
> + };
> +
> + timer {
S is before T, so this goes after soc {}
> + compatible = "arm,armv8-timer";
> + /* Hypervisor Virtual Timer interrupt is not wired to GIC */
> + 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)>;
> +
> + /* Samsung's bootloader (S-BOOT) doesn't set CNTFRQ_EL0 */
> + clock-frequency = <26000000>;
> + };
> +
> + gic: interrupt-controller@...e0000 {
> + compatible = "arm,cortex-a15-gic";
> + reg = <0x0 0x104e1000 0x1000>,
So it should be @104e1000 and not @104e0000
> + <0x0 0x104e2000 0x1000>,
> + <0x0 0x104e4000 0x2000>,
> + <0x0 0x104e6000 0x2000>;
> + interrupts = <GIC_PPI 9 (GIC_CPU_MASK_SIMPLE(8) | IRQ_TYPE_LEVEL_HIGH)>;
> + interrupt-controller;
> + #address-cells = <0>;
> + #interrupt-cells = <3>;
> + };
> +
> + soc: soc@0 {
> + compatible = "simple-bus";
> + ranges = <0x0 0x0 0x0 0x20000000>;
> + #address-cells = <1>;
> + #size-cells = <1>;
> +
> + chipid@...00000 {
> + compatible = "samsung,exynos7870-chipid",
> + "samsung,exynos4210-chipid";
> + reg = <0x10100000 0x100>;
> + };
> +
> + pmu_system_controller: system-controller@...80000 {
> + compatible = "samsung,exynos7870-pmu",
> + "samsung,exynos7-pmu", "syscon";
> + reg = <0x10480000 0x10000>;
> + #clock-cells = <1>;
> +
> + clock-names = "clkout8",
> + "clkout9",
Could fit more than one per line.
> + "clkout10",
> + "clkout11",
> + "clkout12";
> + clocks = <&oscclk>,
> + <&oscclk>,
> + <&oscclk>,
> + <&oscclk>,
> + <&oscclk>;
> +
> + reboot-mode {
> + compatible = "syscon-reboot-mode";
> + offset = <0x080c>;
> + mode-bootloader = <EXYNOS7870_BOOT_BOOTLOADER>;
> + mode-download = <EXYNOS7870_BOOT_DOWNLOAD>;
> + mode-recovery = <EXYNOS7870_BOOT_RECOVERY>;
> + };
> + };
> +
> + cmu_mif: clock-controller@...60000 {
Sort all of these by address.
> + compatible = "samsung,exynos7870-cmu-mif";
> + reg = <0x10460000 0x1000>;
> + #clock-cells = <1>;
> +
> + clock-names = "oscclk";
> + clocks = <&oscclk>;
> + };
> +
> + cmu_dispaud: clock-controller@...d0000 {
> + compatible = "samsung,exynos7870-cmu-dispaud";
> + reg = <0x148d0000 0x1000>;
> + #clock-cells = <1>;
> +
> + clock-names = "oscclk",
> + "gout_mif_cmu_dispaud_bus",
bus
> + "gout_mif_cmu_dispaud_decon_eclk",
decon_eclk
> + "gout_mif_cmu_dispaud_decon_vclk";
decon_vclk
Clock names here shouldn't be long. You'll still describe them in the
clock YAML either way. Check other SoCs like 8895, autov920, 990.
> + clocks = <&oscclk>,
> + <&cmu_mif CLK_GOUT_MIF_CMU_DISPAUD_BUS>,
> + <&cmu_mif CLK_GOUT_MIF_CMU_DISPAUD_DECON_ECLK>,
> + <&cmu_mif CLK_GOUT_MIF_CMU_DISPAUD_DECON_VCLK>;
> + };
> +
...
> +
> + i2c1: i2c@...80000 {
> + compatible = "samsung,exynos7870-i2c",
> + "samsung,s3c2440-i2c";
> + reg = <0x13880000 0x100>;
> + interrupts = <GIC_SPI 429 IRQ_TYPE_LEVEL_HIGH>;
> + status = "disabled";
Status always comes last. Make sure to sort all properties as per the DT
Coding Style [1].
[1] https://docs.kernel.org/devicetree/bindings/dts-coding-style.html
> +
> + pinctrl-names = "default";
> + pinctrl-0 = <&i2c1_bus>;
> +
> + clock-names = "i2c";
> + clocks = <&cmu_peri CLK_GOUT_PERI_I2C1_PCLK>;
> + };
...
> + };
> + };
> +};
> +
> +#include "exynos7870-pinctrl.dtsi"
> +#include "arm/samsung/exynos-syscon-restart.dtsi"
Didn't this already include a reboot node?
Best regards,
Ivaylo
>
Powered by blists - more mailing lists