linux-kernel - Re: [PATCH 2/2] ARM: dts: sun50i: h6: Introduce Beelink GS1 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: Windows password security audit tool. GUI, reports in PDF.

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

Message-ID: <CAJiuCcdGHgUgUg5xAVPjcxeCqJwnoVjf69_CJaWF22iGPNfniw@mail.gmail.com>
Date:   Fri, 5 Apr 2019 14:02:17 +0200
From:   Clément Péron <peron.clem@...il.com>
To:     Maxime Ripard <maxime.ripard@...tlin.com>
Cc:     Rob Herring <robh+dt@...nel.org>, Chen-Yu Tsai <wens@...e.org>,
        Icenowy Zheng <icenowy@...c.io>,
        Jagan Teki <jagan@...rulasolutions.com>,
        devicetree@...r.kernel.org, linux-kernel@...r.kernel.org,
        linux-arm-kernel@...ts.infradead.org
Subject: Re: [PATCH 2/2] ARM: dts: sun50i: h6: Introduce Beelink GS1 board

Hi,

On Fri, 5 Apr 2019 at 11:08, Maxime Ripard <maxime.ripard@...tlin.com> wrote:
>
> Hi,
>
> On Thu, Apr 04, 2019 at 02:45:04PM +0200, Clément Péron wrote:
> > Beelink GS1 is an Allwinner H6 based TV box,
> > which support:
> > - Allwinner H6 Quad-core 64-bit ARM Cortex-A53
> > - GPU Mali-T720
> > - 2GB LPDDR3 RAM
> > - AXP805 PMIC
> > - 1Gbps GMAC via RTL8211E
> > - FN-Link 6222B-SRB Wifi/BT
> > - 1x USB 2.0 Host and 1x USB 3.0 Host
> > - HDMI port
> > - S/PDIF Tx
> > - IR receiver
> > - 5V/2A DC power supply
> >
> > Signed-off-by: Clément Péron <peron.clem@...il.com>
> > ---
> >
> > Thanks to Jagan and Icenowy, most of this devicetree is taken from their works on
> > the OrangePi One Plus and Pine H64.
> >
> >  arch/arm64/boot/dts/allwinner/Makefile        |   1 +
> >  .../dts/allwinner/sun50i-h6-beelink-gs1.dts   | 252 ++++++++++++++++++
> >  2 files changed, 253 insertions(+)
> >  create mode 100644 arch/arm64/boot/dts/allwinner/sun50i-h6-beelink-gs1.dts
> >
> > diff --git a/arch/arm64/boot/dts/allwinner/Makefile b/arch/arm64/boot/dts/allwinner/Makefile
> > index e4dce2f6fa3a..2dd806a3d5c9 100644
> > --- a/arch/arm64/boot/dts/allwinner/Makefile
> > +++ b/arch/arm64/boot/dts/allwinner/Makefile
> > @@ -20,6 +20,7 @@ dtb-$(CONFIG_ARCH_SUNXI) += sun50i-h5-orangepi-pc2.dtb
> >  dtb-$(CONFIG_ARCH_SUNXI) += sun50i-h5-orangepi-prime.dtb
> >  dtb-$(CONFIG_ARCH_SUNXI) += sun50i-h5-orangepi-zero-plus.dtb
> >  dtb-$(CONFIG_ARCH_SUNXI) += sun50i-h5-orangepi-zero-plus2.dtb
> > +dtb-$(CONFIG_ARCH_SUNXI) += sun50i-h6-beelink-gs1.dtb
> >  dtb-$(CONFIG_ARCH_SUNXI) += sun50i-h6-orangepi-lite2.dtb
> >  dtb-$(CONFIG_ARCH_SUNXI) += sun50i-h6-orangepi-one-plus.dtb
> >  dtb-$(CONFIG_ARCH_SUNXI) += sun50i-h6-pine-h64.dtb
> > diff --git a/arch/arm64/boot/dts/allwinner/sun50i-h6-beelink-gs1.dts b/arch/arm64/boot/dts/allwinner/sun50i-h6-beelink-gs1.dts
> > new file mode 100644
> > index 000000000000..d927e7d33bf3
> > --- /dev/null
> > +++ b/arch/arm64/boot/dts/allwinner/sun50i-h6-beelink-gs1.dts
> > @@ -0,0 +1,252 @@
> > +// SPDX-License-Identifier: (GPL-2.0+ or MIT)
> > +/*
> > + * Copyright (C) 2019 Clément Péron <peron.clem@...il.com>
> > + */
> > +
> > +/dts-v1/;
> > +
> > +#include "sun50i-h6.dtsi"
> > +
> > +#include <dt-bindings/gpio/gpio.h>
> > +
> > +/ {
> > +     model = "Beelink GS1";
> > +     compatible = "azw,beelink-gs1", "allwinner,sun50i-h6";
>
> Please add that board to
> Documentation/devicetree/bindings/arm/sunxi.yaml, it's new and in
> linux-next.

OK didn't know about that will do it.

>
> > +&mmc0 {
> > +     pinctrl-names = "default";
> > +     pinctrl-0 = <&mmc0_pins>;
> > +     vmmc-supply = <&reg_cldo1>;
> > +     cd-gpios = <&pio 5 6 GPIO_ACTIVE_LOW>;
> > +     bus-width = <4>;
> > +     status = "okay";
> > +};
> > +
> > +&mmc2 {
> > +     pinctrl-names = "default";
> > +     pinctrl-0 = <&mmc2_pins>;
>
> If those (mmc0 and mmc2_pins) are the only muxing options as their
> names suggests, it should be set in the DTSI.
Yes, it's the only muxing. I will add a patch to move these in the SOC dtsi.

>
> If not, the name of the pin group should be changed.
>
> > +                     reg_cldo2: cldo2 {
> > +                             regulator-always-on;
> > +                             regulator-min-microvolt = <3300000>;
> > +                             regulator-max-microvolt = <3300000>;
> > +                             regulator-name = "vcc-wifi-1";
> > +                     };
> > +
> > +                     reg_cldo3: cldo3 {
> > +                             regulator-always-on;
> > +                             regulator-min-microvolt = <3300000>;
> > +                             regulator-max-microvolt = <3300000>;
> > +                             regulator-name = "vcc-wifi-2";
> > +                     };
>
> I don't see any SDIO bus declared, why do you need those regulators on?
You're right forget to remove. Will do.

>
> Can you also declare the PIO regulators?
Ok,

Thanks for the review.


>
> Thanks!
> Maxmie
>
> --
> Maxime Ripard, Bootlin
> Embedded Linux and Kernel engineering
> https://bootlin.com