From: Sam Protsenko <semen.protsenko@linaro.org> To: Krzysztof Kozlowski <krzysztof.kozlowski@canonical.com> Cc: David Virag <virag.david003@gmail.com>, Rob Herring <robh+dt@kernel.org>, Sylwester Nawrocki <s.nawrocki@samsung.com>, Tomasz Figa <tomasz.figa@gmail.com>, Chanwoo Choi <cw00.choi@samsung.com>, Michael Turquette <mturquette@baylibre.com>, Stephen Boyd <sboyd@kernel.org>, linux-arm-kernel@lists.infradead.org, linux-samsung-soc@vger.kernel.org, devicetree@vger.kernel.org, linux-kernel@vger.kernel.org, linux-clk@vger.kernel.org Subject: Re: [PATCH v4 7/7] arm64: dts: exynos: Add initial device tree support for Exynos7885 SoC Date: Wed, 8 Dec 2021 18:51:45 +0200 [thread overview] Message-ID: <CAPLW+4kmt1fEWG14jLJmPM0uHoyf017U7rigri47KT9Tamto=Q@mail.gmail.com> (raw) In-Reply-To: <a7d5f290-7992-b37c-fe2c-90bf3e5e83ce@canonical.com> On Wed, 8 Dec 2021 at 18:29, Krzysztof Kozlowski <krzysztof.kozlowski@canonical.com> wrote: > > On 08/12/2021 16:37, Sam Protsenko wrote: > > On Wed, 8 Dec 2021 at 11:05, Krzysztof Kozlowski > > <krzysztof.kozlowski@canonical.com> wrote: > >> > >> On 07/12/2021 21:19, Sam Protsenko wrote: > >>> On Mon, 6 Dec 2021 at 17:32, David Virag <virag.david003@gmail.com> wrote: > >>>> > >>>> Add initial Exynos7885 device tree nodes with dts for the Samsung Galaxy > >>>> A8 (2018), a.k.a. "jackpotlte", with model number "SM-A530F". > >>>> Currently this includes some clock support, UART support, and I2C nodes. > >>>> > >>>> Signed-off-by: David Virag <virag.david003@gmail.com> > >>>> --- > >>>> Changes in v2: > >>>> - Remove address-cells, and size-cells from dts, since they are > >>>> already in the dtsi. > >>>> - Lower case hex in memory node > >>>> - Fix node names with underscore instead of hyphen > >>>> - Fix line breaks > >>>> - Fix "-key" missing from gpio keys node names > >>>> - Use the form without "key" in gpio key labels on all keys > >>>> - Suffix pin configuration node names with "-pins" > >>>> - Remove "fimc_is_mclk" nodes from pinctrl dtsi for now > >>>> - Use macros for "samsung,pin-con-pdn", and "samsung,pin-con-pdn" > >>>> - Add comment about Arm PMU > >>>> - Rename "clock-oscclk" to "osc-clock" > >>>> - Include exynos-syscon-restart.dtsi instead of rewriting its contents > >>>> > >>>> Changes in v3: > >>>> - Fix typo (seperate -> separate) > >>>> > >>>> Changes in v4: > >>>> - Fixed leading 0x in clock-controller nodes > >>>> - Actually suffixed pin configuration node names with "-pins" > >>>> - Seperated Cortex-A53 and Cortex-A73 PMU > >>>> > >>>> arch/arm64/boot/dts/exynos/Makefile | 7 +- > >>>> .../boot/dts/exynos/exynos7885-jackpotlte.dts | 95 ++ > >>>> .../boot/dts/exynos/exynos7885-pinctrl.dtsi | 865 ++++++++++++++++++ > >>>> arch/arm64/boot/dts/exynos/exynos7885.dtsi | 438 +++++++++ > >>>> 4 files changed, 1402 insertions(+), 3 deletions(-) > >>>> create mode 100644 arch/arm64/boot/dts/exynos/exynos7885-jackpotlte.dts > >>> > >>> Shouldn't SoC and board files be sent as two separate patches? For > >>> example, I've checked exynos5433 and exynos7, SoC support > >> > >> Does not have to be. DTSI by itself cannot be even compiled, so keeping > >> it a separate commit does not bring that much benefits. Especially if it > >> is only one DTSI and one DTS. > >> > > > > Right, the only theoretical benefit I can see is reverting the board > > dts in future, if another board already uses SoC dtsi. Or > > cherry-picking in similar manner. Not my call though, for me it just > > seems easier to review it that way, and more atomic. > > > >>> > >>>> create mode 100644 arch/arm64/boot/dts/exynos/exynos7885-pinctrl.dtsi > >>>> create mode 100644 arch/arm64/boot/dts/exynos/exynos7885.dtsi > >>>> > >>>> diff --git a/arch/arm64/boot/dts/exynos/Makefile b/arch/arm64/boot/dts/exynos/Makefile > >>>> index b41e86df0a84..c68c4ad577ac 100644 > >>>> --- a/arch/arm64/boot/dts/exynos/Makefile > >>>> +++ b/arch/arm64/boot/dts/exynos/Makefile > >>>> @@ -1,6 +1,7 @@ > >>>> # SPDX-License-Identifier: GPL-2.0 > >>>> dtb-$(CONFIG_ARCH_EXYNOS) += \ > >>>> - exynos5433-tm2.dtb \ > >>>> - exynos5433-tm2e.dtb \ > >>>> - exynos7-espresso.dtb \ > >>>> + exynos5433-tm2.dtb \ > >>>> + exynos5433-tm2e.dtb \ > >>>> + exynos7-espresso.dtb \ > >>>> + exynos7885-jackpotlte.dtb \ > >>>> exynosautov9-sadk.dtb > >>>> diff --git a/arch/arm64/boot/dts/exynos/exynos7885-jackpotlte.dts b/arch/arm64/boot/dts/exynos/exynos7885-jackpotlte.dts > >>>> new file mode 100644 > >>>> index 000000000000..f5941dc4c374 > >>>> --- /dev/null > >>>> +++ b/arch/arm64/boot/dts/exynos/exynos7885-jackpotlte.dts > >>>> @@ -0,0 +1,95 @@ > >>>> +// SPDX-License-Identifier: GPL-2.0 > >>>> +/* > >>>> + * Samsung Galaxy A8 2018 (jackpotlte/SM-A530F) device tree source > >>>> + * > >>>> + * Copyright (c) 2021 Samsung Electronics Co., Ltd. > >>>> + * Copyright (c) 2021 Dávid Virág > >>>> + * > >>> > >>> This line is not needed. > >>> > >>>> + */ > >>>> + > >>>> +/dts-v1/; > >>> > >>> Suggest adding empty line here. > >>> > >>>> +#include "exynos7885.dtsi" > >>>> +#include <dt-bindings/gpio/gpio.h> > >>>> +#include <dt-bindings/input/input.h> > >>>> +#include <dt-bindings/interrupt-controller/irq.h> > >>>> + > >>>> +/ { > >>>> + model = "Samsung Galaxy A8 (2018)"; > >>>> + compatible = "samsung,jackpotlte", "samsung,exynos7885"; > >>>> + chassis-type = "handset"; > >>>> + > >>>> + aliases { > >>>> + serial0 = &serial_0; > >>>> + serial1 = &serial_1; > >>>> + serial2 = &serial_2; > >>> > >>> Suggestion: add aliases also for i2c nodes, to keep i2c instance > >>> numbers fixed in run-time (e.g. in "i2cdetect -l" output). > >>> > >>>> + }; > >>>> + > >>>> + chosen { > >>>> + stdout-path = &serial_2; > >>>> + }; > >>>> + > >>>> + memory@80000000 { > >>>> + device_type = "memory"; > >>>> + reg = <0x0 0x80000000 0x3da00000>, > >>>> + <0x0 0xc0000000 0x40000000>, > >>>> + <0x8 0x80000000 0x40000000>; > >>>> + }; > >>>> + > >>>> + gpio-keys { > >>>> + compatible = "gpio-keys"; > >>>> + pinctrl-names = "default"; > >>>> + pinctrl-0 = <&key_volup &key_voldown &key_power>; > >>>> + > >>>> + volup-key { > >>>> + label = "Volume Up"; > >>>> + interrupts = <5 IRQ_TYPE_LEVEL_HIGH 0>; > >>> > >>> Here and below: what is 0, why it's needed? Also, isn't it enough to > >>> have just "gpios", and remove interrupt*? Need to check "gpio-keys" > >>> driver and bindings doc, but AFAIR it should be enough to have just > >>> "gpios =" or just "interrupts =". > >> > >> "gpios" is enough, because the IRQ line is derived from it. However > >> explicitly describing interrupts seems like a more detailed hardware > >> description. > >> > > > > Frankly I don't think it's more detailed, it states the same thing > > (gpa1 controller, line=5). > > It states that interrupt is exactly the same as GPIO which not > explicitly coming from bindings. > > > Also not sure if level interrupt is needed > > for a key, maybe edge type would be better. Also, I still don't > > understand 0 in the end. > > Indeed this part looks not correct - the leve and 0 at the end. In such > case better to skip it then define misleading property. > > > Checking existing dts's, most of those only > > define "gpios". I'd say having only "gpios" is more obvious, and will > > work the same way. But that's not a strong preference on my side, just > > think it's a bit misleading right now. > > Yep. > > > > >>> > >>> > >>>> + interrupt-parent = <&gpa1>; > >>>> + linux,code = <KEY_VOLUMEUP>; > >>>> + gpios = <&gpa1 5 GPIO_ACTIVE_LOW>; > >>>> + }; > >>>> + > >>>> + voldown-key { > >>>> + label = "Volume Down"; > >>>> + interrupts = <6 IRQ_TYPE_LEVEL_HIGH 0>; > >>>> + interrupt-parent = <&gpa1>; > >>>> + linux,code = <KEY_VOLUMEDOWN>; > >>>> + gpios = <&gpa1 6 GPIO_ACTIVE_LOW>; > >>>> + }; > >>>> + > >>>> + power-key { > >>>> + label = "Power"; > >>>> + interrupts = <7 IRQ_TYPE_LEVEL_HIGH 0>; > >>>> + interrupt-parent = <&gpa1>; > >>>> + linux,code = <KEY_POWER>; > >>>> + gpios = <&gpa1 7 GPIO_ACTIVE_LOW>; > >>>> + wakeup-source; > >>>> + }; > >>>> + }; > >>>> +}; > >>>> + > >>> > >>> If there are some LEDs by chance on that board -- it might be useful > >>> to define those here with "gpio-leds" as well. Maybe even set some > >>> default trigger like "heartbeat". > >>> > >>>> +&serial_2 { > >>>> + status = "okay"; > >>>> +}; > >>>> + > >>>> +&pinctrl_alive { > >>>> + key_volup: key-volup-pins { > >>>> + samsung,pins = "gpa1-5"; > >>>> + samsung,pin-function = <EXYNOS_PIN_FUNC_F>; > >>> > >>> Maybe EXYNOS_PIN_FUNC_EINT is more self-explanatory? Just a suggestion though. > >>> > >>>> + samsung,pin-pud = <EXYNOS_PIN_PULL_NONE>; > >>>> + samsung,pin-drv = <0>; > >>> > >>> Here and below: please use EXYNOS5420_PIN_DRV_LV1 (means drive level = 1x). > >> > >> But are these drive level 1x? The Exynos Auto v9 has different values > >> than older ones. > >> > > > > It should be that. One way to implicitly figure that out is to look at > > nodes like "sd0_clk_fast_slew_rate_3x" and those pin-drv properties. > > Also, in Exynos850 for most of domains those constants are > > appropriate, that's why I mentioned that. > > Then I agree, use existing macros. The macros can be skipped for cases > when the meaning is different. > > > > >>> > >>>> + }; > >>>> + > >>>> + key_voldown: key-voldown-pins { > >>>> + samsung,pins = "gpa1-6"; > >>>> + samsung,pin-function = <EXYNOS_PIN_FUNC_F>; > >>>> + samsung,pin-pud = <EXYNOS_PIN_PULL_NONE>; > >>>> + samsung,pin-drv = <0>; > >>>> + }; > >>>> + > >>>> + key_power: key-power-pins { > >>>> + samsung,pins = "gpa1-7"; > >>>> + samsung,pin-function = <EXYNOS_PIN_FUNC_F>; > >>>> + samsung,pin-pud = <EXYNOS_PIN_PULL_NONE>; > >>>> + samsung,pin-drv = <0>; > >>>> + }; > >>>> +}; > >>>> diff --git a/arch/arm64/boot/dts/exynos/exynos7885-pinctrl.dtsi b/arch/arm64/boot/dts/exynos/exynos7885-pinctrl.dtsi > >>>> new file mode 100644 > >>>> index 000000000000..8336b2e48858 > >>>> --- /dev/null > >>>> +++ b/arch/arm64/boot/dts/exynos/exynos7885-pinctrl.dtsi > >>>> @@ -0,0 +1,865 @@ > >>>> +// SPDX-License-Identifier: GPL-2.0 > >>>> +/* > >>>> + * Samsung Exynos7885 SoC pin-mux and pin-config device tree source > >>>> + * > >>>> + * Copyright (c) 2017 Samsung Electronics Co., Ltd. > >>>> + * Copyright (c) 2021 Dávid Virág > >>>> + * > >>>> + * Samsung's Exynos7885 SoC pin-mux and pin-config options are listed as > >>>> + * device tree nodes in this file. > >>>> + */ > >>>> + > >>>> +#include <dt-bindings/pinctrl/samsung.h> > >>> > >>> You probably also need <dt-bindings/interrupt-controller/arm-gic.h> > >>> here for GIC_SPI definition. > >>> > >>>> + > >>>> +&pinctrl_alive { > >>>> + etc0: etc0 { > >>>> + gpio-controller; > >>>> + #gpio-cells = <2>; > >>>> + > >>>> + interrupt-controller; > >>>> + #interrupt-cells = <2>; > >>>> + }; > >>>> + > >>>> + etc1: etc1 { > >>>> + gpio-controller; > >>>> + #gpio-cells = <2>; > >>>> + > >>>> + interrupt-controller; > >>>> + #interrupt-cells = <2>; > >>>> + }; > >>> > >>> Hmm, what are these two? I can't find anything related in > >>> exynos7885.dtsi. If it's just some leftover from downstream vendor > >>> kernel -- please remove it. > >> > >> This is a pinctrl DTSI file. What do you expect to find in > >> exynos7885.dtsi for these? Why removing them? > > > > etc0 and etc1 nodes are defined as gpio-controller and > > interrupt-controller. So "compatible" should be provided somewhere for > > those nodes. For example, for "gpa0" node below you can find its > > compatible in exynos7885.dtsi. > > I am sorry, I still don't get it. gpa0 below does not have compatible. > I was probably groggy and missed the fact those etc* nodes are child nodes of pinctrl_alive :) And now I can see those are actually described in pinctrl-exynos-arm64.c (in linux-next, where 7885 pinctrl support is added). Please ignore my request w.r.t. etc* nodes, those should stay of course. > > Right now I don't understand how those > > etc0 and etc1 can be used at all. > > Exactly the same as gpa0, nothing changes here. > > > So maybe it's better to just remove > > those? Those are not used anywhere and we probably don't even know > > what those nodes represent. My point is, if those are actually some > > leftovers from vendor kernel and those are not going to be used (and I > > don't see how, without "compatible"), then we probablly better off > > without those. > > I don't have the manual but in other SoCs these are not left-overs, but > real GPIO banks. Their configurability depends on the SoCs. I agree that > usually they are not used (because one of the uses is debugging), but > they can be included for completness of HW description. Assuming they exist. > > (...) > > >>>> +#include "exynos7885-pinctrl.dtsi" > >>>> +#include "arm/exynos-syscon-restart.dtsi" > >>> > >>> Have you verified both reboot and power off functions from this file? > >>> I guess if some doesn't work, it's better to avoid including this, but > >>> instead add corresponding sub-nodes into your pmu_sytem_controller. > >> > >> Why open-coding same code work and including would not? Assuming that it > >> compiles, of course. > >> > > > > For example, in case of Exynos850 the "power off" node from this file > > wasn't suitable. In that case it's not worth including it. But David > > already confirmed both work fine for him, so it doesn't matter > > anymore. > > These nodes were here before and since they duplicated common syscon, I > asked to use DTSI. The boards which do not use the same syscon > registers/methods should not include it, obviously. :) > > > Best regards, > Krzysztof
WARNING: multiple messages have this Message-ID (diff)
From: Sam Protsenko <semen.protsenko@linaro.org> To: Krzysztof Kozlowski <krzysztof.kozlowski@canonical.com> Cc: David Virag <virag.david003@gmail.com>, Rob Herring <robh+dt@kernel.org>, Sylwester Nawrocki <s.nawrocki@samsung.com>, Tomasz Figa <tomasz.figa@gmail.com>, Chanwoo Choi <cw00.choi@samsung.com>, Michael Turquette <mturquette@baylibre.com>, Stephen Boyd <sboyd@kernel.org>, linux-arm-kernel@lists.infradead.org, linux-samsung-soc@vger.kernel.org, devicetree@vger.kernel.org, linux-kernel@vger.kernel.org, linux-clk@vger.kernel.org Subject: Re: [PATCH v4 7/7] arm64: dts: exynos: Add initial device tree support for Exynos7885 SoC Date: Wed, 8 Dec 2021 18:51:45 +0200 [thread overview] Message-ID: <CAPLW+4kmt1fEWG14jLJmPM0uHoyf017U7rigri47KT9Tamto=Q@mail.gmail.com> (raw) In-Reply-To: <a7d5f290-7992-b37c-fe2c-90bf3e5e83ce@canonical.com> On Wed, 8 Dec 2021 at 18:29, Krzysztof Kozlowski <krzysztof.kozlowski@canonical.com> wrote: > > On 08/12/2021 16:37, Sam Protsenko wrote: > > On Wed, 8 Dec 2021 at 11:05, Krzysztof Kozlowski > > <krzysztof.kozlowski@canonical.com> wrote: > >> > >> On 07/12/2021 21:19, Sam Protsenko wrote: > >>> On Mon, 6 Dec 2021 at 17:32, David Virag <virag.david003@gmail.com> wrote: > >>>> > >>>> Add initial Exynos7885 device tree nodes with dts for the Samsung Galaxy > >>>> A8 (2018), a.k.a. "jackpotlte", with model number "SM-A530F". > >>>> Currently this includes some clock support, UART support, and I2C nodes. > >>>> > >>>> Signed-off-by: David Virag <virag.david003@gmail.com> > >>>> --- > >>>> Changes in v2: > >>>> - Remove address-cells, and size-cells from dts, since they are > >>>> already in the dtsi. > >>>> - Lower case hex in memory node > >>>> - Fix node names with underscore instead of hyphen > >>>> - Fix line breaks > >>>> - Fix "-key" missing from gpio keys node names > >>>> - Use the form without "key" in gpio key labels on all keys > >>>> - Suffix pin configuration node names with "-pins" > >>>> - Remove "fimc_is_mclk" nodes from pinctrl dtsi for now > >>>> - Use macros for "samsung,pin-con-pdn", and "samsung,pin-con-pdn" > >>>> - Add comment about Arm PMU > >>>> - Rename "clock-oscclk" to "osc-clock" > >>>> - Include exynos-syscon-restart.dtsi instead of rewriting its contents > >>>> > >>>> Changes in v3: > >>>> - Fix typo (seperate -> separate) > >>>> > >>>> Changes in v4: > >>>> - Fixed leading 0x in clock-controller nodes > >>>> - Actually suffixed pin configuration node names with "-pins" > >>>> - Seperated Cortex-A53 and Cortex-A73 PMU > >>>> > >>>> arch/arm64/boot/dts/exynos/Makefile | 7 +- > >>>> .../boot/dts/exynos/exynos7885-jackpotlte.dts | 95 ++ > >>>> .../boot/dts/exynos/exynos7885-pinctrl.dtsi | 865 ++++++++++++++++++ > >>>> arch/arm64/boot/dts/exynos/exynos7885.dtsi | 438 +++++++++ > >>>> 4 files changed, 1402 insertions(+), 3 deletions(-) > >>>> create mode 100644 arch/arm64/boot/dts/exynos/exynos7885-jackpotlte.dts > >>> > >>> Shouldn't SoC and board files be sent as two separate patches? For > >>> example, I've checked exynos5433 and exynos7, SoC support > >> > >> Does not have to be. DTSI by itself cannot be even compiled, so keeping > >> it a separate commit does not bring that much benefits. Especially if it > >> is only one DTSI and one DTS. > >> > > > > Right, the only theoretical benefit I can see is reverting the board > > dts in future, if another board already uses SoC dtsi. Or > > cherry-picking in similar manner. Not my call though, for me it just > > seems easier to review it that way, and more atomic. > > > >>> > >>>> create mode 100644 arch/arm64/boot/dts/exynos/exynos7885-pinctrl.dtsi > >>>> create mode 100644 arch/arm64/boot/dts/exynos/exynos7885.dtsi > >>>> > >>>> diff --git a/arch/arm64/boot/dts/exynos/Makefile b/arch/arm64/boot/dts/exynos/Makefile > >>>> index b41e86df0a84..c68c4ad577ac 100644 > >>>> --- a/arch/arm64/boot/dts/exynos/Makefile > >>>> +++ b/arch/arm64/boot/dts/exynos/Makefile > >>>> @@ -1,6 +1,7 @@ > >>>> # SPDX-License-Identifier: GPL-2.0 > >>>> dtb-$(CONFIG_ARCH_EXYNOS) += \ > >>>> - exynos5433-tm2.dtb \ > >>>> - exynos5433-tm2e.dtb \ > >>>> - exynos7-espresso.dtb \ > >>>> + exynos5433-tm2.dtb \ > >>>> + exynos5433-tm2e.dtb \ > >>>> + exynos7-espresso.dtb \ > >>>> + exynos7885-jackpotlte.dtb \ > >>>> exynosautov9-sadk.dtb > >>>> diff --git a/arch/arm64/boot/dts/exynos/exynos7885-jackpotlte.dts b/arch/arm64/boot/dts/exynos/exynos7885-jackpotlte.dts > >>>> new file mode 100644 > >>>> index 000000000000..f5941dc4c374 > >>>> --- /dev/null > >>>> +++ b/arch/arm64/boot/dts/exynos/exynos7885-jackpotlte.dts > >>>> @@ -0,0 +1,95 @@ > >>>> +// SPDX-License-Identifier: GPL-2.0 > >>>> +/* > >>>> + * Samsung Galaxy A8 2018 (jackpotlte/SM-A530F) device tree source > >>>> + * > >>>> + * Copyright (c) 2021 Samsung Electronics Co., Ltd. > >>>> + * Copyright (c) 2021 Dávid Virág > >>>> + * > >>> > >>> This line is not needed. > >>> > >>>> + */ > >>>> + > >>>> +/dts-v1/; > >>> > >>> Suggest adding empty line here. > >>> > >>>> +#include "exynos7885.dtsi" > >>>> +#include <dt-bindings/gpio/gpio.h> > >>>> +#include <dt-bindings/input/input.h> > >>>> +#include <dt-bindings/interrupt-controller/irq.h> > >>>> + > >>>> +/ { > >>>> + model = "Samsung Galaxy A8 (2018)"; > >>>> + compatible = "samsung,jackpotlte", "samsung,exynos7885"; > >>>> + chassis-type = "handset"; > >>>> + > >>>> + aliases { > >>>> + serial0 = &serial_0; > >>>> + serial1 = &serial_1; > >>>> + serial2 = &serial_2; > >>> > >>> Suggestion: add aliases also for i2c nodes, to keep i2c instance > >>> numbers fixed in run-time (e.g. in "i2cdetect -l" output). > >>> > >>>> + }; > >>>> + > >>>> + chosen { > >>>> + stdout-path = &serial_2; > >>>> + }; > >>>> + > >>>> + memory@80000000 { > >>>> + device_type = "memory"; > >>>> + reg = <0x0 0x80000000 0x3da00000>, > >>>> + <0x0 0xc0000000 0x40000000>, > >>>> + <0x8 0x80000000 0x40000000>; > >>>> + }; > >>>> + > >>>> + gpio-keys { > >>>> + compatible = "gpio-keys"; > >>>> + pinctrl-names = "default"; > >>>> + pinctrl-0 = <&key_volup &key_voldown &key_power>; > >>>> + > >>>> + volup-key { > >>>> + label = "Volume Up"; > >>>> + interrupts = <5 IRQ_TYPE_LEVEL_HIGH 0>; > >>> > >>> Here and below: what is 0, why it's needed? Also, isn't it enough to > >>> have just "gpios", and remove interrupt*? Need to check "gpio-keys" > >>> driver and bindings doc, but AFAIR it should be enough to have just > >>> "gpios =" or just "interrupts =". > >> > >> "gpios" is enough, because the IRQ line is derived from it. However > >> explicitly describing interrupts seems like a more detailed hardware > >> description. > >> > > > > Frankly I don't think it's more detailed, it states the same thing > > (gpa1 controller, line=5). > > It states that interrupt is exactly the same as GPIO which not > explicitly coming from bindings. > > > Also not sure if level interrupt is needed > > for a key, maybe edge type would be better. Also, I still don't > > understand 0 in the end. > > Indeed this part looks not correct - the leve and 0 at the end. In such > case better to skip it then define misleading property. > > > Checking existing dts's, most of those only > > define "gpios". I'd say having only "gpios" is more obvious, and will > > work the same way. But that's not a strong preference on my side, just > > think it's a bit misleading right now. > > Yep. > > > > >>> > >>> > >>>> + interrupt-parent = <&gpa1>; > >>>> + linux,code = <KEY_VOLUMEUP>; > >>>> + gpios = <&gpa1 5 GPIO_ACTIVE_LOW>; > >>>> + }; > >>>> + > >>>> + voldown-key { > >>>> + label = "Volume Down"; > >>>> + interrupts = <6 IRQ_TYPE_LEVEL_HIGH 0>; > >>>> + interrupt-parent = <&gpa1>; > >>>> + linux,code = <KEY_VOLUMEDOWN>; > >>>> + gpios = <&gpa1 6 GPIO_ACTIVE_LOW>; > >>>> + }; > >>>> + > >>>> + power-key { > >>>> + label = "Power"; > >>>> + interrupts = <7 IRQ_TYPE_LEVEL_HIGH 0>; > >>>> + interrupt-parent = <&gpa1>; > >>>> + linux,code = <KEY_POWER>; > >>>> + gpios = <&gpa1 7 GPIO_ACTIVE_LOW>; > >>>> + wakeup-source; > >>>> + }; > >>>> + }; > >>>> +}; > >>>> + > >>> > >>> If there are some LEDs by chance on that board -- it might be useful > >>> to define those here with "gpio-leds" as well. Maybe even set some > >>> default trigger like "heartbeat". > >>> > >>>> +&serial_2 { > >>>> + status = "okay"; > >>>> +}; > >>>> + > >>>> +&pinctrl_alive { > >>>> + key_volup: key-volup-pins { > >>>> + samsung,pins = "gpa1-5"; > >>>> + samsung,pin-function = <EXYNOS_PIN_FUNC_F>; > >>> > >>> Maybe EXYNOS_PIN_FUNC_EINT is more self-explanatory? Just a suggestion though. > >>> > >>>> + samsung,pin-pud = <EXYNOS_PIN_PULL_NONE>; > >>>> + samsung,pin-drv = <0>; > >>> > >>> Here and below: please use EXYNOS5420_PIN_DRV_LV1 (means drive level = 1x). > >> > >> But are these drive level 1x? The Exynos Auto v9 has different values > >> than older ones. > >> > > > > It should be that. One way to implicitly figure that out is to look at > > nodes like "sd0_clk_fast_slew_rate_3x" and those pin-drv properties. > > Also, in Exynos850 for most of domains those constants are > > appropriate, that's why I mentioned that. > > Then I agree, use existing macros. The macros can be skipped for cases > when the meaning is different. > > > > >>> > >>>> + }; > >>>> + > >>>> + key_voldown: key-voldown-pins { > >>>> + samsung,pins = "gpa1-6"; > >>>> + samsung,pin-function = <EXYNOS_PIN_FUNC_F>; > >>>> + samsung,pin-pud = <EXYNOS_PIN_PULL_NONE>; > >>>> + samsung,pin-drv = <0>; > >>>> + }; > >>>> + > >>>> + key_power: key-power-pins { > >>>> + samsung,pins = "gpa1-7"; > >>>> + samsung,pin-function = <EXYNOS_PIN_FUNC_F>; > >>>> + samsung,pin-pud = <EXYNOS_PIN_PULL_NONE>; > >>>> + samsung,pin-drv = <0>; > >>>> + }; > >>>> +}; > >>>> diff --git a/arch/arm64/boot/dts/exynos/exynos7885-pinctrl.dtsi b/arch/arm64/boot/dts/exynos/exynos7885-pinctrl.dtsi > >>>> new file mode 100644 > >>>> index 000000000000..8336b2e48858 > >>>> --- /dev/null > >>>> +++ b/arch/arm64/boot/dts/exynos/exynos7885-pinctrl.dtsi > >>>> @@ -0,0 +1,865 @@ > >>>> +// SPDX-License-Identifier: GPL-2.0 > >>>> +/* > >>>> + * Samsung Exynos7885 SoC pin-mux and pin-config device tree source > >>>> + * > >>>> + * Copyright (c) 2017 Samsung Electronics Co., Ltd. > >>>> + * Copyright (c) 2021 Dávid Virág > >>>> + * > >>>> + * Samsung's Exynos7885 SoC pin-mux and pin-config options are listed as > >>>> + * device tree nodes in this file. > >>>> + */ > >>>> + > >>>> +#include <dt-bindings/pinctrl/samsung.h> > >>> > >>> You probably also need <dt-bindings/interrupt-controller/arm-gic.h> > >>> here for GIC_SPI definition. > >>> > >>>> + > >>>> +&pinctrl_alive { > >>>> + etc0: etc0 { > >>>> + gpio-controller; > >>>> + #gpio-cells = <2>; > >>>> + > >>>> + interrupt-controller; > >>>> + #interrupt-cells = <2>; > >>>> + }; > >>>> + > >>>> + etc1: etc1 { > >>>> + gpio-controller; > >>>> + #gpio-cells = <2>; > >>>> + > >>>> + interrupt-controller; > >>>> + #interrupt-cells = <2>; > >>>> + }; > >>> > >>> Hmm, what are these two? I can't find anything related in > >>> exynos7885.dtsi. If it's just some leftover from downstream vendor > >>> kernel -- please remove it. > >> > >> This is a pinctrl DTSI file. What do you expect to find in > >> exynos7885.dtsi for these? Why removing them? > > > > etc0 and etc1 nodes are defined as gpio-controller and > > interrupt-controller. So "compatible" should be provided somewhere for > > those nodes. For example, for "gpa0" node below you can find its > > compatible in exynos7885.dtsi. > > I am sorry, I still don't get it. gpa0 below does not have compatible. > I was probably groggy and missed the fact those etc* nodes are child nodes of pinctrl_alive :) And now I can see those are actually described in pinctrl-exynos-arm64.c (in linux-next, where 7885 pinctrl support is added). Please ignore my request w.r.t. etc* nodes, those should stay of course. > > Right now I don't understand how those > > etc0 and etc1 can be used at all. > > Exactly the same as gpa0, nothing changes here. > > > So maybe it's better to just remove > > those? Those are not used anywhere and we probably don't even know > > what those nodes represent. My point is, if those are actually some > > leftovers from vendor kernel and those are not going to be used (and I > > don't see how, without "compatible"), then we probablly better off > > without those. > > I don't have the manual but in other SoCs these are not left-overs, but > real GPIO banks. Their configurability depends on the SoCs. I agree that > usually they are not used (because one of the uses is debugging), but > they can be included for completness of HW description. Assuming they exist. > > (...) > > >>>> +#include "exynos7885-pinctrl.dtsi" > >>>> +#include "arm/exynos-syscon-restart.dtsi" > >>> > >>> Have you verified both reboot and power off functions from this file? > >>> I guess if some doesn't work, it's better to avoid including this, but > >>> instead add corresponding sub-nodes into your pmu_sytem_controller. > >> > >> Why open-coding same code work and including would not? Assuming that it > >> compiles, of course. > >> > > > > For example, in case of Exynos850 the "power off" node from this file > > wasn't suitable. In that case it's not worth including it. But David > > already confirmed both work fine for him, so it doesn't matter > > anymore. > > These nodes were here before and since they duplicated common syscon, I > asked to use DTSI. The boards which do not use the same syscon > registers/methods should not include it, obviously. :) > > > Best regards, > Krzysztof _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
next prev parent reply other threads:[~2021-12-08 16:52 UTC|newest] Thread overview: 78+ messages / expand[flat|nested] mbox.gz Atom feed top 2021-12-06 15:31 [PATCH v4 0/7] Initial Samsung Galaxy A8 (2018) support David Virag 2021-12-06 15:31 ` David Virag 2021-12-06 15:31 ` [PATCH v4 1/7] dt-bindings: clock: Add bindings definitions for Exynos7885 CMU David Virag 2021-12-06 15:31 ` David Virag 2021-12-07 18:15 ` Sam Protsenko 2021-12-07 18:15 ` Sam Protsenko 2021-12-10 21:26 ` Rob Herring 2021-12-10 21:26 ` Rob Herring 2021-12-12 18:39 ` Krzysztof Kozlowski 2021-12-12 18:39 ` Krzysztof Kozlowski 2021-12-20 9:40 ` Krzysztof Kozlowski 2021-12-20 9:40 ` Krzysztof Kozlowski 2021-12-19 22:52 ` Sylwester Nawrocki 2021-12-19 22:52 ` Sylwester Nawrocki 2021-12-06 15:31 ` [PATCH v4 2/7] dt-bindings: clock: Document Exynos7885 CMU bindings David Virag 2021-12-06 15:31 ` David Virag 2021-12-07 18:23 ` Sam Protsenko 2021-12-07 18:23 ` Sam Protsenko 2021-12-10 21:28 ` Rob Herring 2021-12-10 21:28 ` Rob Herring 2021-12-06 15:31 ` [PATCH v4 3/7] dt-bindings: arm: samsung: document jackpotlte board binding David Virag 2021-12-06 15:31 ` David Virag 2021-12-07 18:26 ` Sam Protsenko 2021-12-07 18:26 ` Sam Protsenko 2021-12-10 21:30 ` Rob Herring 2021-12-10 21:30 ` Rob Herring 2021-12-15 16:21 ` (subset) " Krzysztof Kozlowski 2021-12-15 16:21 ` Krzysztof Kozlowski 2021-12-19 14:53 ` David Virag 2021-12-19 14:53 ` David Virag 2021-12-20 9:38 ` Krzysztof Kozlowski 2021-12-20 9:38 ` Krzysztof Kozlowski 2021-12-06 15:31 ` [PATCH v4 4/7] clk: samsung: Make exynos850_register_cmu shared David Virag 2021-12-06 15:31 ` David Virag 2021-12-07 9:32 ` Krzysztof Kozlowski 2021-12-07 9:32 ` Krzysztof Kozlowski 2021-12-07 18:53 ` Sam Protsenko 2021-12-07 18:53 ` Sam Protsenko 2021-12-06 15:31 ` [PATCH v4 5/7] clk: samsung: clk-pll: Add support for pll1417x David Virag 2021-12-06 15:31 ` David Virag 2021-12-07 19:00 ` Sam Protsenko 2021-12-07 19:00 ` Sam Protsenko 2021-12-08 8:50 ` Krzysztof Kozlowski 2021-12-08 8:50 ` Krzysztof Kozlowski 2021-12-06 15:31 ` [PATCH v4 6/7] clk: samsung: Add initial Exynos7885 clock driver David Virag 2021-12-06 15:31 ` David Virag 2021-12-07 9:33 ` Krzysztof Kozlowski 2021-12-07 9:33 ` Krzysztof Kozlowski 2021-12-07 19:14 ` Sam Protsenko 2021-12-07 19:14 ` Sam Protsenko 2021-12-06 15:31 ` [PATCH v4 7/7] arm64: dts: exynos: Add initial device tree support for Exynos7885 SoC David Virag 2021-12-06 15:31 ` David Virag 2021-12-07 9:39 ` Krzysztof Kozlowski 2021-12-07 9:39 ` Krzysztof Kozlowski 2021-12-07 19:42 ` Marc Zyngier 2021-12-07 19:42 ` Marc Zyngier 2021-12-19 14:36 ` David Virag 2021-12-19 14:36 ` David Virag 2021-12-20 8:44 ` Marc Zyngier 2021-12-20 8:44 ` Marc Zyngier 2021-12-07 20:19 ` Sam Protsenko 2021-12-07 20:19 ` Sam Protsenko 2021-12-07 22:29 ` David Virag 2021-12-07 22:29 ` David Virag 2021-12-08 0:55 ` Chanho Park 2021-12-08 0:55 ` Chanho Park 2021-12-08 9:05 ` Krzysztof Kozlowski 2021-12-08 9:05 ` Krzysztof Kozlowski 2021-12-08 15:37 ` Sam Protsenko 2021-12-08 15:37 ` Sam Protsenko 2021-12-08 16:28 ` Krzysztof Kozlowski 2021-12-08 16:28 ` Krzysztof Kozlowski 2021-12-08 16:51 ` Sam Protsenko [this message] 2021-12-08 16:51 ` Sam Protsenko 2022-01-31 15:35 ` Krzysztof Kozlowski 2022-01-31 15:35 ` Krzysztof Kozlowski 2022-02-01 0:47 ` David Virag 2022-02-01 0:47 ` David Virag
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to='CAPLW+4kmt1fEWG14jLJmPM0uHoyf017U7rigri47KT9Tamto=Q@mail.gmail.com' \ --to=semen.protsenko@linaro.org \ --cc=cw00.choi@samsung.com \ --cc=devicetree@vger.kernel.org \ --cc=krzysztof.kozlowski@canonical.com \ --cc=linux-arm-kernel@lists.infradead.org \ --cc=linux-clk@vger.kernel.org \ --cc=linux-kernel@vger.kernel.org \ --cc=linux-samsung-soc@vger.kernel.org \ --cc=mturquette@baylibre.com \ --cc=robh+dt@kernel.org \ --cc=s.nawrocki@samsung.com \ --cc=sboyd@kernel.org \ --cc=tomasz.figa@gmail.com \ --cc=virag.david003@gmail.com \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.