Message-ID: <CAMuHMdV3Z9d8qsa_mnUgH50a=_aq9myf0xYVx+ZexKtkUXcMNw@mail.gmail.com>
Date: Wed, 29 Jan 2025 15:12:07 +0100
From: Geert Uytterhoeven <geert@...ux-m68k.org>
To: Julien Massot <julien.massot@...labora.com>
Cc: Magnus Damm <magnus.damm@...il.com>, Rob Herring <robh@...nel.org>, 
	Krzysztof Kozlowski <krzk+dt@...nel.org>, Conor Dooley <conor+dt@...nel.org>, 
	linux-renesas-soc@...r.kernel.org, devicetree@...r.kernel.org, 
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2 2/2] arm64: renesas: add initial support for MYIR Remi Pi

Hi Julien,

On Mon, 27 Jan 2025 at 10:39, Julien Massot <julien.massot@...labora.com> wrote:
> Add basic support for the MyIR Remi Pi (based on r9a07g044l2):
>  - UART
>  - i2c
>  - emmc
>  - USB host
>  - HDMI output
>  - Ethernet
>
> Signed-off-by: Julien Massot <julien.massot@...labora.com>

Thanks for your patch!

> --- /dev/null
> +++ b/arch/arm64/boot/dts/renesas/r9a07g044l2-remi-pi.dts
> @@ -0,0 +1,388 @@
> +// SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +/*
> + * Device Tree Source for the MYiR Remi Pi
> + *
> + * Copyright (C) 2022 MYiR Electronics Corp.
> + * Copyright (C) 2025 Collabora Ltd.
> + */
> +
> +/dts-v1/;
> +
> +#include <dt-bindings/gpio/gpio.h>
> +#include <dt-bindings/pinctrl/rzg2l-pinctrl.h>
> +
> +#include "r9a07g044l2.dtsi"
> +
> +/ {
> +       model = "MYIR Tech Limited Remi Pi MYB-YG2LX-REMI";
> +       compatible = "myir,remi-pi", "renesas,r9a07g044l2", "renesas,r9a07g044";
> +
> +       aliases {
> +               ethernet0 = &eth0;
> +               ethernet1 = &eth1;
> +
> +               serial0 = &scif0;

So serial0 is the CA55 console, OK.

> +               serial1 = &scif1;

Connected to BT/WiFi, but not yet enabled below.

> +               serial2 = &scif2;

This is wired to the Raspberri Pi expansion connector.
However, on Raspberri Pi, these pins are GPIOs.
While the Raspberri Pi UART_[TR]X pins are wired to SCIF4 here,
which is not yet enabled...

> +               serial3 = &scif3;

Serial3 is the CA33 debug console, OK.

> +
> +               i2c0 = &i2c0;
> +               i2c1 = &i2c1;
> +               i2c2 = &i2c2;
> +               i2c3 = &i2c3;
> +
> +               mmc0 = &sdhi0;
> +               mmc1 = &sdhi1;

sdhi1 is not yet enabled, so please drop it.

> +       };

> +       reg_5p0v: regulator-5p0v {
> +               compatible = "regulator-fixed";
> +               regulator-name = "fixed-5.0V";
> +               regulator-min-microvolt = <5000000>;
> +               regulator-max-microvolt = <5000000>;
> +       };
> +
> +       reg_3p3v: regulator-3p3v {
> +               compatible = "regulator-fixed";
> +               regulator-name = "fixed-3.3V";
> +               vin-supply = <&reg_5p0v>;
> +               regulator-min-microvolt = <3300000>;
> +               regulator-max-microvolt = <3300000>;
> +               regulator-always-on;
> +       };
> +
> +       reg_1p8v: regulator-1p8v {
> +               compatible = "regulator-fixed";
> +               regulator-name = "fixed-1.8V";
> +               vin-supply = <&reg_3p3v>;

reg_5p0v, as 1.8V is generated from 5.0V, according to the schematics.

> +               regulator-min-microvolt = <1800000>;
> +               regulator-max-microvolt = <1800000>;
> +               regulator-always-on;
> +       };
> +
> +       reg_1p1v: regulator-vdd-core {
> +               compatible = "regulator-fixed";
> +               regulator-name = "fixed-1.1V";
> +               regulator-min-microvolt = <1100000>;
> +               regulator-max-microvolt = <1100000>;
> +               regulator-always-on;
> +       };

> +&eth0 {
> +       pinctrl-0 = <&eth0_pins>;
> +       pinctrl-names = "default";
> +       phy-handle = <&phy0>;
> +       phy-mode = "rgmii-id";
> +       status = "okay";
> +
> +       phy0: ethernet-phy@4 {
> +               compatible = "ethernet-phy-ieee802.3-c22";
> +               reg = <4>;
> +               interrupts-extended = <&pinctrl RZG2L_GPIO(44, 2) IRQ_TYPE_LEVEL_LOW>;

Missing reset-gpios (P44_3).

> +       };
> +};
> +
> +&eth1 {
> +       pinctrl-0 = <&eth1_pins>;
> +       pinctrl-names = "default";
> +       phy-handle = <&phy1>;
> +       phy-mode = "rgmii-id";
> +       status = "okay";
> +
> +       phy1: ethernet-phy@6 {
> +               compatible = "ethernet-phy-ieee802.3-c22";
> +               reg = <6>;
> +               interrupts-extended = <&pinctrl RZG2L_GPIO(43, 2) IRQ_TYPE_LEVEL_LOW>;

Missing reset-gpios (P43_3).

> +       };
> +};

> +&pinctrl {

> +       spi0_pins: spi0 {
> +               pinmux = <RZG2L_PORT_PINMUX(47, 0, 5)>, /* CLK */
> +                        <RZG2L_PORT_PINMUX(47, 1, 5)>, /* MOSI */
> +                        <RZG2L_PORT_PINMUX(47, 2, 5)>, /* MISO */
> +                        <RZG2L_PORT_PINMUX(47, 3, 5)>; /* Chip Enable*/
> +       };

Unused.

> +
> +       eth0_pins: eth0 {

Please sort pinctrl subnodes (alphabetically by node name).

> +       sdhi0_pins: sd0 {
> +               sd0_data {
> +                       pins = "SD0_DATA0", "SD0_DATA1", "SD0_DATA2", "SD0_DATA3",
> +                              "SD0_DATA4", "SD0_DATA5", "SD0_DATA6", "SD0_DATA7";
> +                       power-source = <1800>;
> +               };
> +
> +               sd0_ctrl {
> +                       pins = "SD0_CLK", "SD0_CMD";
> +                       power-source = <1800>;
> +               };
> +
> +               sd0_rst {
> +                       pins = "SD0_RST#";
> +                       power-source = <1800>;
> +               };
> +       };
> +
> +       sdhi0_pins_uhs: sd0_uhs {
> +               sd0_data_uhs {
> +                       pins = "SD0_DATA0", "SD0_DATA1", "SD0_DATA2", "SD0_DATA3",
> +                              "SD0_DATA4", "SD0_DATA5", "SD0_DATA6", "SD0_DATA7";
> +                       power-source = <1800>;
> +               };
> +
> +               sd0_ctrl_uhs {
> +                       pins = "SD0_CLK", "SD0_CMD";
> +                       power-source = <1800>;
> +               };
> +
> +               sd0_rst_uhs {
> +                       pins = "SD0_RST#";
> +                       power-source = <1800>;
> +               };
> +       };

sd0 and sd0_uhs are identical, so you can just always use the former,
and drop the latter.

> +
> +       usb1_pins: usb1 {
> +               pinmux = <RZG2L_PORT_PINMUX(42, 0, 1)>, /* VBUS */
> +                        <RZG2L_PORT_PINMUX(42, 1, 1)>; /* OVC */

These two pins don't seem to be exposed on the SoM.
Perhaps this is wired on the SoM?

> +       };
> +
> +       scif0_pins: scif0 {
> +               pinmux = <RZG2L_PORT_PINMUX(38, 0, 1)>, /* TxD */
> +                        <RZG2L_PORT_PINMUX(38, 1, 1)>; /* RxD */
> +       };
> +
> +       scif1_pins: scif1 {
> +               pinmux = <RZG2L_PORT_PINMUX(40, 0, 1)>, /* TxD */
> +                        <RZG2L_PORT_PINMUX(40, 1, 1)>; /* RxD */
> +       };

SCIF1 is connected to BT/WiFi, and the CTS/RTS pins should be included.
However, for now SCIF1 is not yet enabled, so please everything related to it.

> +&usb2_phy1 {
> +       pinctrl-0 = <&usb1_pins>;
> +       pinctrl-names = "default";
> +
> +       status = "okay";
> +};
> +
> +&ehci1 {

Please sort references to nodes (alphabetically). More below...

> +&dsi {
> +       status = "okay";

Please insert a blank line.

> +       ports {

> +&i2c0 {
> +       pinctrl-0 = <&i2c0_pins>;
> +       pinctrl-names = "default";
> +
> +       clock-frequency = <400000>;
> +       status = "okay";
> +
> +       hdmi-bridge@48 {
> +               compatible = "lontium,lt8912b";
> +               reg = <0x48> ;
> +               reset-gpios = <&pinctrl RZG2L_GPIO(42, 2) GPIO_ACTIVE_LOW>;

Missing interrupts (P16_1), but that is not yet supported by the
bindings, so I guess it's fine to leave it out for now.

Gr{oetje,eeting}s,

                        Geert


--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@...ux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Powered by blists - more mailing lists