linux-kernel - Re: [PATCH 1/1] ARM: dts: sunxi: Add Olimex A20-SOM204-EVB board

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Thu, 18 Jan 2018 11:07:45 +0100
From:   Maxime Ripard <maxime.ripard@...e-electrons.com>
To:     Stefan Mavrodiev <stefan@...mex.com>
Cc:     Rob Herring <robh+dt@...nel.org>,
        Mark Rutland <mark.rutland@....com>,
        Russell King <linux@...linux.org.uk>,
        Chen-Yu Tsai <wens@...e.org>,
        "open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS" 
        <devicetree@...r.kernel.org>,
        "moderated list:ARM PORT" <linux-arm-kernel@...ts.infradead.org>,
        open list <linux-kernel@...r.kernel.org>,
        linux-sunxi@...glegroups.com
Subject: Re: [PATCH 1/1] ARM: dts: sunxi: Add Olimex A20-SOM204-EVB board

Hi!

On Mon, Jan 15, 2018 at 12:07:34PM +0200, Stefan Mavrodiev wrote:
> > > +/dts-v1/;
> > > +#include "sun7i-a20.dtsi"
> > > +#include "sunxi-common-regulators.dtsi"
> > > +
> > > +
> > > +#include <dt-bindings/gpio/gpio.h>
> > > +#include <dt-bindings/interrupt-controller/irq.h>
> > > +#include <dt-bindings/pwm/pwm.h>
> > > +
> > > +/ {
> > > +	model = "Olimex A20-SOM204-EVB";
> > > +	compatible = "olimex,a20-olimex-som204-evb", "allwinner,sun7i-a20";
> > > +
> > > +	aliases {
> > > +		serial0 = &uart0;
> > > +		serial1 = &uart4;
> > > +		serial2 = &uart7;
> > > +		spi0 = &spi1;
> > > +		spi1 = &spi2;
> > > +		ethernet1 = &rtl8723bs;
> >
> > ethernet1? if there's a single network interface, it should be
> > ethernet0.
>
> I think this will conflict the gmac alias defined in sun7i-a20.dtsi:
> 
> aliases {
>     ethernet0 = &gmac;
> };

We have that? That's bad, but you're right :)

> > > +		stat {
> > > +			label = "a20-som204:green:stat";
> > > +			gpios = <&pio 8 0 GPIO_ACTIVE_HIGH>;
> > > +			default-state = "on";
> > > +		};
> > > +
> > > +		led1 {
> > > +			label = "a20-som204-evb:green:led1";
> > > +			gpios = <&pio 8 10 GPIO_ACTIVE_HIGH>;
> > > +			default-state = "on";
> > > +		};
> > > +
> > > +		led2 {
> > > +			label = "a20-som204-evb:yellow:led2";
> > > +			gpios = <&pio 8 11 GPIO_ACTIVE_HIGH>;
> > > +			default-state = "on";
> > > +		};
> >
> > You don't have the same prefix between stat and led1/led2. I'm fine
> > with both, but you should be consistent :)
>
> STAT led is on the SOM204 module, while led1/2 on the EVB. Thats why
> they have different prefix.

Still, the user and the system will see it as a single board, and the
documentation states that it should be the board name. I'm not quite
sure what a good rule would be here. Have you looked at how other
boards dealt with it? Chen-Yu, any opinion on this?

> > 
> > > +	};
> > > +
> > > +	mmc2_pwrseq: mmc2_pwrseq {
> > > +		compatible = "mmc-pwrseq-emmc";
> > > +		reset-gpios = <&pio 2 16 GPIO_ACTIVE_LOW>;
> > > +	};
> > This is already declared in the emmc variant, isn't it?
> > 
> > > +	rtl_pwrseq: rtl_pwrseq {
> > > +		compatible = "mmc-pwrseq-simple";
> > > +		reset-gpios = <&pio 6 9 GPIO_ACTIVE_LOW>,
> > > +			      <&pio 1 11 GPIO_ACTIVE_LOW>;
> > > +	};
> >
> > It looks suspicious that you have two reset lines.
>
> RTL8723BS is comblo WiFI/BT module. There is separate reset control
> for each of the systems.

You should tie the reset line to their associated device then. In this
case, you're linking the BT reset line to the wifi device, which in
turn means that if your MMC driver is not loaded / enabled, the BT
part will not work. That's obviously not ideal.

> > > +&spi1 {
> > > +	pinctrl-names = "default";
> > > +	pinctrl-0 = <&spi1_pins_a>,
> > > +		    <&spi1_cs0_pins_a>;
> > > +	status = "okay";
> > > +};
> > > +
> > > +&spi2 {
> > > +	pinctrl-names = "default";
> > > +	pinctrl-0 = <&spi2_pins_a>,
> > > +		    <&spi2_cs0_pins_a>;
> > > +	status = "okay";
> > > +};
> > What is connected on those buses
>
> As mentioned SPI1/2 are exposed to UEXT1/2.

Ok, please make that a comment.

> > 
> > > +&uart0 {
> > > +	pinctrl-names = "default";
> > > +	pinctrl-0 = <&uart0_pins_a>;
> > > +	status = "okay";
> > > +};
> > > +
> > > +&uart3 {
> > > +	pinctrl-names = "default";
> > > +	pinctrl-0 = <&bt_uart_pins>;
> > > +	status = "okay";
> > > +};
> > > +
> > > +&uart4 {
> > > +	pinctrl-names = "default";
> > > +	pinctrl-0 = <&uart4_pins_a>;
> > > +	status = "okay";
> > > +};
> > > +
> > > +&uart7 {
> > > +	pinctrl-names = "default";
> > > +	pinctrl-0 = <&uart7_pins_a>;
> > > +	status = "okay";
> > > +};
> > Same thing for these three UARTs
>
> Uart3 is used for H5 BT protocol. UART4/7 are exposed to UEXT.

Ok, comments for those as well then :)

yyThanks!
maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com

Download attachment "signature.asc" of type "application/pgp-signature" (834 bytes)