Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
MIME-Version: 1.0
References: <20190307151342.7381-1-narmstrong@baylibre.com> <20190307151342.7381-7-narmstrong@baylibre.com>
In-Reply-To: <20190307151342.7381-7-narmstrong@baylibre.com>
From:   Martin Blumenstingl <martin.blumenstingl@googlemail.com>
Date:   Fri, 15 Mar 2019 22:24:42 +0100
Message-ID: <CAFBinCAa+ZjxJi491_dG+kvLFPLFoLXHFZToGP8W1ECeeqOYVQ@mail.gmail.com>
Subject: Re: [PATCH 6/9] arm64: dts: meson: g12a: Add UART A, B & C nodes and pins
To:     Neil Armstrong <narmstrong@baylibre.com>
Cc:     khilman@baylibre.com, linux-amlogic@lists.infradead.org,
        linux-kernel@vger.kernel.org, linux-arm-kernel@lists.infradead.org
Content-Type: text/plain; charset="UTF-8"
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

Hi Neil,

On Thu, Mar 7, 2019 at 4:15 PM Neil Armstrong <narmstrong@baylibre.com> wrote:
>
> This patch adds the 2 UART nodes in the EE power domain with the corresponding
> pinctrl nodes.
there are 3 UART controllers in the EE power domain

> Signed-off-by: Neil Armstrong <narmstrong@baylibre.com>
> ---
>  arch/arm64/boot/dts/amlogic/meson-g12a.dtsi | 82 +++++++++++++++++++++
>  1 file changed, 82 insertions(+)
>
> diff --git a/arch/arm64/boot/dts/amlogic/meson-g12a.dtsi b/arch/arm64/boot/dts/amlogic/meson-g12a.dtsi
> index 2a700bb45d04..50e2cd36e08b 100644
> --- a/arch/arm64/boot/dts/amlogic/meson-g12a.dtsi
> +++ b/arch/arm64/boot/dts/amlogic/meson-g12a.dtsi
> @@ -113,6 +113,61 @@
>                                                 #gpio-cells = <2>;
>                                                 gpio-ranges = <&periphs_pinctrl 0 0 86>;
>                                         };
> +
> +                                       uart_a_pins: uart_a {
(this applies to all new nodes)
we started using dashes for the node names for new pin definitions on
the GX SoCs.
I don't remember where it was discussed exactly but I think this was
requested from Rob

since G12A is a new SoC we should do it "right" from the beginning

[...]
> +                                       uart_ao_a_c_pins: uart_ao_a_c {
> +                                               mux {
> +                                                       groups = "uart_ao_a_rx_c",
> +                                                                "uart_ao_a_tx_c";
> +                                                       function = "uart_ao_a_c";
> +                                                       bias-disable;
> +                                               };
> +                                       };
I'm fine with this part if you mention it in the subject and/or the description
uart_ao_a_c routes two pins from bank C (from the EE domain) to the
uart_AO controller (from the AO domain)

> +                                       uart_b_pins: uart_b {
> +                                               mux {
> +                                                       groups = "uart_b_tx",
> +                                                                "uart_b_rx";
> +                                                       function = "uart_b";
> +                                                       bias-disable;
> +                                               };
> +                                       };
> +
> +                                       uart_c_pins: uart_c {
> +                                               mux {
> +                                                       groups = "uart_c_tx",
> +                                                                "uart_c_rx";
> +                                                       function = "uart_c";
> +                                                       bias-disable;
> +                                               };
> +                                       };
> +
> +                                       uart_c_cts_rts_pins: uart_c_cts_rts {
> +                                               mux {
> +                                                       groups = "uart_c_cts",
> +                                                                "uart_c_rts";
> +                                                       function = "uart_c";
> +                                                       bias-disable;
> +                                               };
> +                                       };
> +                               };
>                         };
>
>                         hiu: bus@3c000 {
> @@ -256,6 +311,33 @@
>                                 compatible = "amlogic,meson-g12a-clk-measure";
>                                 reg = <0x0 0x18000 0x0 0x10>;
>                         };
> +
> +                       uart_C: serial@22000 {
> +                               compatible = "amlogic,meson-gx-uart";
> +                               reg = <0x0 0x22000 0x0 0x18>;
> +                               interrupts = <GIC_SPI 93 IRQ_TYPE_EDGE_RISING>;
> +                               clocks = <&xtal>, <&clkc CLKID_UART1>, <&xtal>;
does uart_C really use CLKID_UART1? on GX uart_C uses CLKID_UART2


Regards
Martin