Re: [PATCH v2 2/5] arm64: dts: exynos: add initial devicetree support for exynos7870

[Kaustabh Chakraborty <kauschluss@disroot.org>]

On 2025-02-04 18:22, Ivaylo Ivanov wrote:
> 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@gmail.com>
>> Signed-off-by: Sergey Lisov <sleirsgoevy@gmail.com>
>> Signed-off-by: Kaustabh Chakraborty <kauschluss@disroot.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).

Can do. The rationale behind keeping it numeric was that the names were
kind of misleading in some cases.

> 
>> +	etc0: etc0-gpio-bank {
>> +		gpio-controller;

...

>> 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>
>> +

...

>> +
>> +			reboot-mode {
>> +				compatible = "syscon-reboot-mode";
>> +				offset = <0x080c>;
>> +				mode-bootloader = <EXYNOS7870_BOOT_BOOTLOADER>;
>> +				mode-download = <EXYNOS7870_BOOT_DOWNLOAD>;
>> +				mode-recovery = <EXYNOS7870_BOOT_RECOVERY>;
>> +			};
>> +		};
>> +

...

>> +
>> +#include "exynos7870-pinctrl.dtsi"
>> +#include "arm/samsung/exynos-syscon-restart.dtsi"
> 
> Didn't this already include a reboot node?

Yes, I believe you've confused it with the *reboot-mode* node as defined
above.

> 
> Best regards,
> Ivaylo
> 
>>