linux-kernel - Re: [PATCH 5/5] ARM: dts: sun9i: Initial support for the Sunchip CX-A99 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: <20170205192313.iik7fdcygltpjfwt@localhost>
Date:   Sun, 5 Feb 2017 20:23:14 +0100
From:   Rask Ingemann Lambertsen <rask@...melder.dk>
To:     Chen-Yu Tsai <wens@...e.org>
Cc:     Mark Rutland <mark.rutland@....com>,
        devicetree <devicetree@...r.kernel.org>,
        Liam Girdwood <lgirdwood@...il.com>,
        Rob Herring <robh+dt@...nel.org>,
        linux-kernel <linux-kernel@...r.kernel.org>,
        Mark Brown <broonie@...nel.org>,
        Maxime Ripard <maxime.ripard@...e-electrons.com>,
        Lee Jones <lee.jones@...aro.org>,
        linux-arm-kernel <linux-arm-kernel@...ts.infradead.org>
Subject: Re: [PATCH 5/5] ARM: dts: sun9i: Initial support for the Sunchip
 CX-A99 board

On Thu, Feb 02, 2017 at 03:36:41PM +0800, Chen-Yu Tsai wrote:
> Hi,
> 
> On Fri, Jan 27, 2017 at 5:27 AM, Rask Ingemann Lambertsen
> <rask@...melder.dk> wrote:
[...]
> > diff --git a/arch/arm/boot/dts/sun9i-a80-cx-a99.dts b/arch/arm/boot/dts/sun9i-a80-cx-a99.dts
> > new file mode 100644
> > index 0000000..8925093
> > --- /dev/null
> > +++ b/arch/arm/boot/dts/sun9i-a80-cx-a99.dts
[...]
> > +       leds {
> > +               compatible = "gpio-leds";
> > +
> > +               blue {
> > +                       gpios = <&pio 6 10 GPIO_ACTIVE_HIGH>;   /* PG10 */
> > +                       label = "cx-a99:blue:status";
> > +                       default-state = "on";
> 
> Please add a note justifying default-state = "on".

I think I'll remove it instead. The device ships with a U-Boot and Android
installation where blue means "on" and red means "stand-by" (which is simply
implemented by blanking the screen, it seems), but I doubt many people want
to run the rather old Android version available for this device. Instead,
I was going to point to the quick setup guide, but it has no mention of the
LEDs at all, so I guess the user won't expect the blue LED to turn on.

> > +       /* USB 3.0 OTG connector. For now, only Vbus is supported. */
> > +       reg_usb0_vbus: regulator-usb0-vbus {
> > +               compatible = "regulator-fixed";
> > +               regulator-name = "usb0-vbus";
> > +               regulator-min-microvolt = <5000000>;
> > +               regulator-max-microvolt = <5000000>;
> > +               gpio = <&pio 7 15 GPIO_ACTIVE_HIGH>;    /* PH15 */
> > +               enable-active-high;
> > +               regulator-always-on;
> 
> It's bad to leave VBUS for OTG connectors on. You never know
> what is plugged in.

The comment is misleading as the connector is not an OTG one but just a
standard-A receptacle wired to the SoC's OTG port. I'll change the comment.

> > +/* SD card slot. */
> > +&mmc0 {
> > +       bus-width = <4>;
> > +       cd-gpios = <&pio 7 17 GPIO_ACTIVE_LOW>; /* PH17 */
> > +       broken-cd;                              /* Interrupts don't work. */
> 
> This should be handled by the driver.

The MMC core, without broken-cd, requests the GPIO driver to enable
interrupt delivery on the pin and will fall back to polling the GPIO pin
if the GPIO driver refuses to enable interrupt delivery. What I see is
that the GPIO driver accepts to deliver interrupts, but only the first
interrupt is triggered (as evident from /proc/interrupts showing only 1).
The vendor sys_config.fex also sets the GPIO pin to polled operation, so
maybe there is a hardware bug.

The sunxi-mmc driver doesn't really support operation with broken-cd only
because it spams the console and dmesg with useless error messages, see
https://www.mail-archive.com/linux-sunxi@googlegroups.com/msg19963.html
for an example.

The comment should probably read something like "GPIO pin delivers no
interrupts, poll it".

> > +                       /* 1.8 V (enabled). */
> > +                       reg_bldo1: bldo1 {
> > +                               regulator-boot-on;
> > +                               regulator-always-on;    /* Hang if disabled */
> 
> If you specify always-on, you don't need to add boot-on.

Ack.

> > +                       /* 3.3 V (enabled). */
> > +                       reg_dcdce: dcdce {
> > +                               regulator-boot-on;
> > +                               regulator-always-on;
> > +                               regulator-min-microvolt = <3300000>;
> > +                               regulator-max-microvolt = <3300000>;
> > +                               regulator-name = "vcc-pb-pc-pd-pe-pf-ph-mmc-spdif";
> 
> This is probably vcc-io or something shorter.

I'm verbose with the names of the regulators for documentation purposes
because unlike some of the boards aimed at developers, documentation is on
this device is scarse. I see that with -mmc now added after my latest round
of probing with a multimeter, the line went above 80 characters. Will it be
OK with vcc-pb,c,d,e,f,h-mmc-spdif?

-- 
Rask Ingemann Lambertsen