From: Antony Antony <antony@phenome.org>
To: Maxime Ripard <maxime.ripard@free-electrons.com>
Cc: Antony Antony <antony@phenome.org>, Chen-Yu Tsai <wens@csie.org>,
Icenowy Zheng <icenowy@aosc.io>,
linux-sunxi@googlegroups.com,
linux-arm-kernel@lists.infradead.org, devicetree@vger.kernel.org,
Linux Kernel Mailing List <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH v5] arm64: allwinner: h5: add support for NanoPi NEO Plus2
Date: Fri, 1 Sep 2017 12:53:13 +0200 [thread overview]
Message-ID: <20170901105313.m26y2re3ulskua43@AntonyAntony.local> (raw)
In-Reply-To: <20170831145859.rief3fqo36ns23rm@flea>
Hi Maxime
Thanks for the review. I will send a PATCH v6 soon.
On Thu, Aug 31, 2017 at 04:58:59PM +0200, Maxime Ripard wrote:
> Hi,
>
> On Wed, Aug 30, 2017 at 02:50:57PM +0200, Antony Antony wrote:
> > Add initial DT support for NanoPi NEO Plus2 by FriendlyARM
> > Allwinner quad core H5 Cortex A53 with an ARM Mali-450MP GPU
> > 1 GB DDR3 RAM
> > 8GB eMMC flash (Samsung KLM8G1WEPD-B031)
> > micro SD card slot
> > Gigabit Ethernet (external RTL8211E-VB-CG chip)
> > 802.11 b/g/n WiFi, Bluetooth 4.0 (Ampak AP6212A module)
> > 2x USB 2.0 host ports & 2x USB via headers
>
> This indendation is weird
I will fix it.
> > The DTS is based on OrangePi PC 2, sun50i-h5-orangepi-pc2
> > Added dwmac-sun8i Gigabit Ethernet support based on
> > Nano Pi Neo2 DT and the schematics.
>
> And that's outdated.
Now I am glad to delete it.
> > + wifi_pwrseq: wifi_pwrseq {
> > + compatible = "mmc-pwrseq-simple";
> > + pinctrl-names = "default";
> > + pinctrl-0 = <&wifi_en_npi>;
> > + reset-gpios = <&r_pio 0 7 GPIO_ACTIVE_LOW>; /* PL7 */
> > + post-power-on-delay-ms = <200>;
> > + };
>
> You should order these nodes alphabetically.
good point I did.
> > +&codec {
> > + allwinner,audio-routing =
> > + "Line Out", "LINEOUT",
> > + "MIC1", "Mic",
> > + "Mic", "MBIAS";
> > + status = "okay";
> > +};
> > +
> > +&ehci0 {
> > + status = "okay";
> > +};
> > +
> > +&ehci3 {
> > + status = "okay";
> > +};
> > +
> > +&emac {
> > + pinctrl-names = "default";
> > + pinctrl-0 = <&emac_rgmii_pins>;
> > + phy-supply = <®_gmac_3v3>;
> > + phy-handle = <&ext_rgmii_phy>;
> > + phy-mode = "rgmii";
> > + status = "okay";
> > +};
> > +
> > +&mdio {
> > + ext_rgmii_phy: ethernet-phy@7 {
> > + compatible = "ethernet-phy-ieee802.3-c22";
> > + reg = <7>;
> > + };
> > +};
>
> This will not compile.
I don't understand you, because, v5 file compiled for me. Here is output
from running system, just the relevant part. using dtc -I fs
/proc/device-tree
ext_rgmii_phy = "/soc/ethernet@1c30000/mdio/ethernet-phy@7";
ethernet@1c30000 {
mdio {
..
ethernet-phy@7 {
compatible = "ethernet-phy-ieee802.3-c22";
phandle = <0x1c>;
reg = <0x7>;
linux,phandle = <0x1c>;
};
};
Is this what you expect?
> > +&mmc1 {
> > + pinctrl-names = "default";
> > + pinctrl-0 = <&mmc1_pins_a>;
> > + vmmc-supply = <®_vcc3v3>;
> > + vqmmc-supply = <®_vcc3v3>;
> > + mmc-pwrseq = <&wifi_pwrseq>;
> > + bus-width = <4>;
> > + non-removable;
> > + boot_device = <0>;
>
> This property is not documented anywhere, I'm not sure what it's here
> for.
boot_device is deleted. A u-boot property got mixed up in kernel DT.
> > + status = "okay";
> > +
> > + /*
> > + * AMPAK AP6212A WiFi module with BCM43430, rev=1 inside
> > + * sdio vendor ID: 0x02d0, sdio device ID: 0xa9a6
> > + * There is no specific Documentation: dt-binding for BCM43430
> > + * brcm,bcm4329-fmac compatible can initialize this module
> > + */
>
> This is not really relevant.
would you prefer no comment or a rewrite? How does this look?
/*
* AMPAK AP6212A WiFi module with BCM43430, rev=1 inside
* sdio vendor ID: 0x02d0, sdio device ID: 0xa9a6
*/
I am afraid a casual reader would think "brcm,bcm4329-fmac" is wrong,
because that is not the actual chip inside the module.
> > + brcmf: wifi@1 {
> > + reg = <1>;
> > + compatible = "brcm,bcm4329-fmac";
> > + };
> > +};
> > +
> > +&mmc2 {
> > + pinctrl-names = "default";
> > + pinctrl-0 = <&mmc2_8bit_pins>;
> > + vmmc-supply = <®_vcc3v3>;
> > + bus-width = <8>;
> > + non-removable;
> > + cap-mmc-hw-reset;
> > + boot_device = <0>;
> > + status = "okay";
> > +};
> > +
> > +&mmc2_8bit_pins {
> > + /* Increase drive strength for DDR modes */
> > + drive-strength = <40>;
>
> It's very likely that you actually don't need 40mA
drive-strength and the node mmc2_8bit_pins are gone. When I removed it
drive-strength = <0x1e>; seems the default. And eMMC seems to work when
booting from Micro SD.
NOTE: the 40mA came from a vresion of vendor's old dts file and I also
noticed the same value is used in other dts in kernel e.g
sun8i-h3-orangepi-plus.dts, sun9i-a80-cubieboard4.dts
It could be a copy paste error or those boards need it. Anyway I removed it.
> > + /* eMMC is missing pull-ups */
> > + bias-pull-up;
> > +};
>
> And that one is already here by default.
good, deleteed.
>
> > +&ohci0 {
> > + status = "okay";
> > +};
> > +
> > +&ohci3 {
> > + status = "okay";
> > +};
> > +
> > +&pio {
> > + leds_npi: led_pins@0 {
> > + pins = "PA10";
> > + function = "gpio_out";
> > + };
> > + gmac_power_pin_nanopi: gmac_power_pin@0 {
> > + pins = "PD6";
> > + function = "gpio_out";
> > + };
> > +};
>
> You don't need these nodes
&pio { } and gmac_power_pin_nanopi{} are deleted.
along with
pinctrl-0 = <&gmac_power_pin_nanopi>;
> > +
> > +&r_pio {
> > + leds_r_npi: led_pins@0 {
> > + pins = "PL10";
> > + function = "gpio_out";
> > + };
> > +
> > + vdd_cpux_r_npi: regulator_pins@0 {
> > + allwinner,pins = "PL6";
> > + allwinner,function = "gpio_out";
> > + allwinner,drive = <SUN4I_PINCTRL_10_MA>;
> > + allwinner,pull = <SUN4I_PINCTRL_NO_PULL>;
> > + };
> > +
> > + wifi_en_npi: wifi_en_pin {
> > + pins = "PL7";
> > + function = "gpio_out";
> > + };
> > +};
>
> Or those.
deleted wifi_en_npi.
>
> > +
> > +&uart0 {
> > + pinctrl-names = "default";
> > + pinctrl-0 = <&uart0_pins_a>;
> > + status = "okay";
> > +};
> > +
> > +&usb_otg {
> > + dr_mode = "host";
> > + status = "okay";
> > +};
> > +
> > +&usbphy {
> > + /* USB Type-A ports' VBUS is always on */
> > + usb0_id_det-gpios = <&pio 6 12 GPIO_ACTIVE_HIGH>; /* PG12 */
>
> If it has an ID-detect pin, then it's not a host-only USB OTG
> controller. dr_mode should be set to otga
good point. I don't see an ID-detect connected in the schematic. The
previous generation had.
I will leave
&usb_otg {
dr_mode = "host";
status = "okay";
};
&usbphy {
/* USB Type-A ports' VBUS is always on */
status = "okay";
};
Wow, a nice cleanup.
I am surprised defaults works well and thanks for pointing these out.
-antony
next prev parent reply other threads:[~2017-09-01 10:53 UTC|newest]
Thread overview: 22+ messages / expand[flat|nested] mbox.gz Atom feed top
[not found] <20170824231716.2623-1-antony@phenome.org>
2017-08-25 10:32 ` [PATCH v2] arm64: allwinner: h5: add support for NanoPi NEO Plus 2 Antony Antony
2017-08-25 12:05 ` Corentin Labbe
2017-08-25 17:47 ` Antony Antony
2017-08-25 13:28 ` Code Kipper
2017-08-25 17:42 ` Antony Antony
2017-08-28 12:16 ` Antony Antony
2017-08-28 12:18 ` Icenowy Zheng
2017-08-29 21:51 ` Antony Antony
2017-08-26 11:11 ` [PATCH v3] " Antony Antony
2017-08-28 12:19 ` [PATCH v4] arm64: allwinner: h5: add support for NanoPi NEO Plus2 Antony Antony
2017-08-30 12:50 ` [PATCH v5] " Antony Antony
2017-08-31 14:58 ` Maxime Ripard
2017-09-01 10:53 ` Antony Antony [this message]
2017-09-04 8:27 ` Maxime Ripard
2017-09-04 14:26 ` Antony Antony
2017-09-05 11:04 ` Maxime Ripard
2017-09-01 10:58 ` [PATCH] arm64: allwinner: h5: add NanoPi NEO Plus2 DT support Antony Antony
2017-09-01 20:45 ` [PATCH v7] " Antony Antony
2017-09-04 14:30 ` [PATCH v8] " Antony Antony
2017-09-05 11:09 ` Maxime Ripard
2017-09-07 16:42 ` [PATCH v9] " Antony Antony
2017-09-08 14:49 ` Maxime Ripard
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20170901105313.m26y2re3ulskua43@AntonyAntony.local \
--to=antony@phenome.org \
--cc=devicetree@vger.kernel.org \
--cc=icenowy@aosc.io \
--cc=linux-arm-kernel@lists.infradead.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-sunxi@googlegroups.com \
--cc=maxime.ripard@free-electrons.com \
--cc=wens@csie.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).