Re: [PATCH 2/2] arm64: renesas: add initial support for MYIR Remi Pi

[Julien Massot <julien.massot@xxxxxxxxxxxxx>]

Hi Krzysztof,

Thanks for the review,

On Wed, 2025-01-22 at 14:01 +0100, Krzysztof Kozlowski wrote:
> On 22/01/2025 13:56, Julien Massot 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@xxxxxxxxxxxxx>
> > ---
> >  arch/arm64/boot/dts/renesas/Makefile               |   1 +
> >  .../arm64/boot/dts/renesas/r9a07g044l2-remi-pi.dts | 420 +++++++++++++++++++++
> >  2 files changed, 421 insertions(+)
> > 
> > diff --git a/arch/arm64/boot/dts/renesas/Makefile b/arch/arm64/boot/dts/renesas/Makefile
> > index 97228a3cb99c163d299b508ee7653aafea3d1a3a..0b69bcfa405b69c26e8072d9b62be98dc621f89a 100644
> > --- a/arch/arm64/boot/dts/renesas/Makefile
> > +++ b/arch/arm64/boot/dts/renesas/Makefile
> > @@ -130,6 +130,7 @@ dtb-$(CONFIG_ARCH_R9A07G044) += r9a07g044l2-smarc.dtb
> >  dtb-$(CONFIG_ARCH_R9A07G044) += r9a07g044l2-smarc-cru-csi-ov5645.dtbo
> >  r9a07g044l2-smarc-cru-csi-ov5645-dtbs := r9a07g044l2-smarc.dtb r9a07g044l2-smarc-cru-csi-
> > ov5645.dtbo
> >  dtb-$(CONFIG_ARCH_R9A07G044) += r9a07g044l2-smarc-cru-csi-ov5645.dtb
> > +dtb-$(CONFIG_ARCH_R9A07G044) += r9a07g044l2-remi-pi.dtb
> 
> Why not keeping the order? Or is there no order at all?
> 
> 
> My mistake there is an order and I will fix it in the V2.
> 
> 
> >  
> >  dtb-$(CONFIG_ARCH_R9A07G054) += r9a07g054l2-smarc.dtb
> >  dtb-$(CONFIG_ARCH_R9A07G054) += r9a07g054l2-smarc-cru-csi-ov5645.dtbo
> > diff --git a/arch/arm64/boot/dts/renesas/r9a07g044l2-remi-pi.dts
> > b/arch/arm64/boot/dts/renesas/r9a07g044l2-remi-pi.dts
> > new file mode 100644
> > index 0000000000000000000000000000000000000000..e6e00afc5f5b2347f139ec4dc145fac6fd39e75d
> > --- /dev/null
> > +++ b/arch/arm64/boot/dts/renesas/r9a07g044l2-remi-pi.dts
> > @@ -0,0 +1,420 @@
> > +// 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;
> > +		serial1 = &scif1;
> > +		serial2 = &scif2;
> > +		serial3 = &scif3;
> > +
> > +		i2c0 = &i2c0;
> > +		i2c1 = &i2c1;
> > +		i2c2 = &i2c2;
> > +		i2c3 = &i2c3;
> > +
> > +		mmc0 = &sdhi0;
> > +		mmc1 = &sdhi1;
> > +	};
> > +
> > +	chosen {
> > +		stdout-path = "serial0:115200n8";
> > +	};
> > +
> > +	memory@48000000 {
> > +		device_type = "memory";
> > +		/* first 128MB is reserved for secure area. */
> > +		reg = <0x0 0x48000000 0x0 0x38000000>;
> > +	};
> > +
> > +	reg_5p0v: regulator-reg_5p0v {
> 
> No underscores in node names.
Ok.

> 
> > +		compatible = "regulator-fixed";
> > +		regulator-name = "fixed-5.0V";
> > +		regulator-min-microvolt = <5000000>;
> > +		regulator-max-microvolt = <5000000>;
> > +	};
> > +
> 
> 
> ...
> 
> > +
> > +&eth0 {
> > +	pinctrl-0 = <&eth0_pins>;
> > +	pinctrl-names = "default";
> > +	phy-handle = <&phy0>;
> > +	phy-mode = "rgmii-id";
> > +	status = "okay";
> > +
> > +	phy0: ethernet-phy@4 {
> > +		compatible = "ethernet-phy-id0022.1640",
> > +			     "ethernet-phy-ieee802.3-c22";
> > +		reg = <4>;
> > +		interrupts-extended = <&pinctrl RZG2L_GPIO(44, 2) IRQ_TYPE_LEVEL_LOW>;
> > +		rxc-skew-psec = <2400>;
> > +		txc-skew-psec = <2400>;
> > +		rxdv-skew-psec = <0>;
> > +		txdv-skew-psec = <0>;
> > +		rxd0-skew-psec = <0>;
> > +		rxd1-skew-psec = <0>;
> > +		rxd2-skew-psec = <0>;
> > +		rxd3-skew-psec = <0>;
> > +		txd0-skew-psec = <0>;
> > +		txd1-skew-psec = <0>;
> > +		txd2-skew-psec = <0>;
> > +		txd3-skew-psec = <0>;
> > +	};
> > +};
> > +
> > +&eth1 {
> > +	pinctrl-0 = <&eth1_pins>;
> > +	pinctrl-names = "default";
> > +	phy-handle = <&phy1>;
> > +	phy-mode = "rgmii-id";
> > +	status = "okay";
> > +
> > +	phy1: ethernet-phy@6 {
> > +		compatible = "ethernet-phy-id0022.1640",
> > +			     "ethernet-phy-ieee802.3-c22";
> > +		reg = <6>;
> > +		interrupts-extended = <&pinctrl RZG2L_GPIO(43, 2) IRQ_TYPE_LEVEL_LOW>;
> > +		rxc-skew-psec = <2400>;
> > +		txc-skew-psec = <2400>;
> > +		rxdv-skew-psec = <0>;
> > +		txdv-skew-psec = <0>;
> > +		rxd0-skew-psec = <0>;
> > +		rxd1-skew-psec = <0>;
> > +		rxd2-skew-psec = <0>;
> > +		rxd3-skew-psec = <0>;
> > +		txd0-skew-psec = <0>;
> > +		txd1-skew-psec = <0>;
> > +		txd2-skew-psec = <0>;
> > +		txd3-skew-psec = <0>;
> 
> 
> At least some properties above do not exist. You cannot use them.
And, there is something wrong with the compatible here. I have a Motorcomm phy
that correctly reports the PHY ID, so I will drop the ethernet-phy-id property.

> 
> It does not look like you tested the DTS against bindings. Please run
> `make dtbs_check W=1` (see
> Documentation/devicetree/bindings/writing-schema.rst or
> https://www.linaro.org/blog/tips-and-tricks-for-validating-devicetree-sources-with-the-devicetree-schema/
> for instructions).
> Maybe you need to update your dtschema and yamllint. Don't rely on
> distro packages for dtschema and be sure you are using the latest
> released dtschema.
I will make sure to have no warnings in V2 with up to date yamllint and dtschema with 
$ make CHECK_DTBS=y renesas/r9a07g044l2-remi-pi.dtb W=1

Best regards,
Julien