Messages in this thread | | | Date | Fri, 1 Sep 2017 12:53:13 +0200 | From | Antony Antony <> | Subject | Re: [PATCH v5] arm64: allwinner: h5: add support for NanoPi NEO Plus2 |
| |
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
| |