From: Krzysztof Kozlowski <krzysztof.kozlowski@canonical.com>
To: Sam Protsenko <semen.protsenko@linaro.org>
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 17:28:55 +0100 [thread overview]
Message-ID: <a7d5f290-7992-b37c-fe2c-90bf3e5e83ce@canonical.com> (raw)
In-Reply-To: <CAPLW+4=ioLewf5Hv5FO1jXjxaLrgedrzyKNtNtevTGADF9rBmw@mail.gmail.com>
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.
> 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:30 UTC|newest]
Thread overview: 39+ 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 ` [PATCH v4 1/7] dt-bindings: clock: Add bindings definitions for Exynos7885 CMU David Virag
2021-12-07 18:15 ` Sam Protsenko
2021-12-10 21:26 ` Rob Herring
2021-12-12 18:39 ` Krzysztof Kozlowski
2021-12-20 9:40 ` Krzysztof Kozlowski
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-07 18:23 ` Sam Protsenko
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-07 18:26 ` Sam Protsenko
2021-12-10 21:30 ` Rob Herring
2021-12-15 16:21 ` (subset) " Krzysztof Kozlowski
2021-12-19 14:53 ` David Virag
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-07 9:32 ` Krzysztof Kozlowski
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-07 19:00 ` Sam Protsenko
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-07 9:33 ` Krzysztof Kozlowski
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-07 9:39 ` Krzysztof Kozlowski
2021-12-07 19:42 ` Marc Zyngier
2021-12-19 14:36 ` David Virag
2021-12-20 8:44 ` Marc Zyngier
2021-12-07 20:19 ` Sam Protsenko
2021-12-07 22:29 ` David Virag
2021-12-08 0:55 ` Chanho Park
2021-12-08 9:05 ` Krzysztof Kozlowski
2021-12-08 15:37 ` Sam Protsenko
2021-12-08 16:28 ` Krzysztof Kozlowski [this message]
2021-12-08 16:51 ` Sam Protsenko
2022-01-31 15:35 ` Krzysztof Kozlowski
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=a7d5f290-7992-b37c-fe2c-90bf3e5e83ce@canonical.com \
--to=krzysztof.kozlowski@canonical.com \
--cc=cw00.choi@samsung.com \
--cc=devicetree@vger.kernel.org \
--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=semen.protsenko@linaro.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: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).