Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
MIME-Version: 1.0
References: <20230807191952.2019208-1-contact@jookia.org>
In-Reply-To: <20230807191952.2019208-1-contact@jookia.org>
Reply-To: wens@csie.org
From:   Chen-Yu Tsai <wens@csie.org>
Date:   Sun, 13 Aug 2023 12:17:27 +0800
Message-ID: <CAGb2v66vqrNZ-ga-1O_bQ16BWXH=RmSHo2Kb+WHJyw-Nm1kWVg@mail.gmail.com>
Subject: Re: [PATCH v3] riscv: dts: allwinner: d1: Add CAN controller nodes
To:     John Watts <contact@jookia.org>
Cc:     linux-sunxi@lists.linux.dev, Rob Herring <robh+dt@kernel.org>,
        Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org>,
        Conor Dooley <conor+dt@kernel.org>,
        Paul Walmsley <paul.walmsley@sifive.com>,
        Palmer Dabbelt <palmer@dabbelt.com>,
        Albert Ou <aou@eecs.berkeley.edu>,
        Jernej Skrabec <jernej.skrabec@gmail.com>,
        Samuel Holland <samuel@sholland.org>,
        Maksim Kiselev <bigunclemax@gmail.com>,
        Marc Kleine-Budde <mkl@pengutronix.de>,
        Cristian Ciocaltea <cristian.ciocaltea@collabora.com>,
        devicetree@vger.kernel.org, linux-riscv@lists.infradead.org,
        linux-arm-kernel@lists.infradead.org, linux-kernel@vger.kernel.org
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: 8BIT
Precedence: bulk

On Tue, Aug 8, 2023 at 3:20 AM John Watts <contact@jookia.org> wrote:
>
> The Allwinner D1, T113 provide two CAN controllers that are variants
> of the R40 controller.
>
> I have tested support for these controllers on two boards:
>
> - A Lichee Panel RV 86 Panel running a D1 chip
> - A Mango Pi MQ Dual running a T113-s3 chip
>
> Both of these fully support both CAN controllers.
>
> Signed-off-by: John Watts <contact@jookia.org>
> ---
> Changes in v3:
> - Set default pinctrl for can controller
> - Moved can nodes to proper location
> - Moved can pins to proper location
>
>  .../boot/dts/allwinner/sunxi-d1s-t113.dtsi    | 34 +++++++++++++++++++
>  1 file changed, 34 insertions(+)
>
> diff --git a/arch/riscv/boot/dts/allwinner/sunxi-d1s-t113.dtsi b/arch/riscv/boot/dts/allwinner/sunxi-d1s-t113.dtsi
> index d59b4acf183a..24f2e70d5886 100644
> --- a/arch/riscv/boot/dts/allwinner/sunxi-d1s-t113.dtsi
> +++ b/arch/riscv/boot/dts/allwinner/sunxi-d1s-t113.dtsi
> @@ -52,6 +52,18 @@ pio: pinctrl@2000000 {
>                         #gpio-cells = <3>;
>                         #interrupt-cells = <3>;
>
> +                       /omit-if-no-ref/

Just FYI this likely ends up doing nothing if you also have them
referenced through a default pinctrl setting. They end up always
referenced and always included. For the D1 series it looks like no
peripheral has default pinctrl setting given.

We can still keep it though. It would help when future chip variants
specify different pinmuxes.

> +                       can0_pins: can0-pins {
> +                               pins = "PB2", "PB3";
> +                               function = "can0";
> +                       };
> +
> +                       /omit-if-no-ref/
> +                       can1_pins: can1-pins {
> +                               pins = "PB4", "PB5";
> +                               function = "can1";
> +                       };
> +
>                         /omit-if-no-ref/
>                         clk_pg11_pin: clk-pg11-pin {
>                                 pins = "PG11";
> @@ -356,6 +368,28 @@ i2c3: i2c@2502c00 {
>                         #size-cells = <0>;
>                 };
>
> +               can0: can@2504000 {
> +                       pinctrl-names = "default";
> +                       pinctrl-0 = <&can0_pins>;

The compatible string should be the first property. In other sunxi SoC dtsi
files, we put the pinctrl just before the "status" property if it's present
to specify a default pin muxing.

I can fix it up while applying.

ChenYu

> +                       compatible = "allwinner,sun20i-d1-can";
> +                       reg = <0x02504000 0x400>;
> +                       interrupts = <SOC_PERIPHERAL_IRQ(21) IRQ_TYPE_LEVEL_HIGH>;
> +                       clocks = <&ccu CLK_BUS_CAN0>;
> +                       resets = <&ccu RST_BUS_CAN0>;
> +                       status = "disabled";
> +               };
> +
> +               can1: can@2504400 {
> +                       pinctrl-names = "default";
> +                       pinctrl-0 = <&can1_pins>;
> +                       compatible = "allwinner,sun20i-d1-can";
> +                       reg = <0x02504400 0x400>;
> +                       interrupts = <SOC_PERIPHERAL_IRQ(22) IRQ_TYPE_LEVEL_HIGH>;
> +                       clocks = <&ccu CLK_BUS_CAN1>;
> +                       resets = <&ccu RST_BUS_CAN1>;
> +                       status = "disabled";
> +               };
> +
>                 syscon: syscon@3000000 {
>                         compatible = "allwinner,sun20i-d1-system-control";
>                         reg = <0x3000000 0x1000>;
> --
> 2.41.0
>
>