From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751956AbdIAKxU (ORCPT ); Fri, 1 Sep 2017 06:53:20 -0400 Received: from lb2-smtp-cloud8.xs4all.net ([194.109.24.25]:36456 "EHLO lb2-smtp-cloud8.xs4all.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751725AbdIAKxS (ORCPT ); Fri, 1 Sep 2017 06:53:18 -0400 Date: Fri, 1 Sep 2017 12:53:13 +0200 From: Antony Antony To: Maxime Ripard Cc: Antony Antony , Chen-Yu Tsai , Icenowy Zheng , linux-sunxi@googlegroups.com, linux-arm-kernel@lists.infradead.org, devicetree@vger.kernel.org, Linux Kernel Mailing List Subject: Re: [PATCH v5] arm64: allwinner: h5: add support for NanoPi NEO Plus2 Message-ID: <20170901105313.m26y2re3ulskua43@AntonyAntony.local> References: <20170824231716.2623-1-antony@phenome.org> <20170830125057.38529-1-antony@phenome.org> <20170831145859.rief3fqo36ns23rm@flea> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20170831145859.rief3fqo36ns23rm@flea> User-Agent: NeoMutt/20170602 (1.8.3) X-CMAE-Envelope: MS4wfP9hTmZaNxMcuBtcabrMN8wEThlUb0MRtsj6wcgpICsPRfZVZE461oNDDlc9UW+8XLMb4tMAJmsuB2wAgJrFJylhwtmj39cvbBd022ctYEqmMfeOPXYO fGc39MF2ODMsqTzy9qFNII9YR81Y6u7MKPSl32toDIilvefibtiiZ5aBMAeDjgo5Go+oOT83eTNSU3/WfW+J9XsgEM62kDZoybxu7BV9gziOC13tkXozDhaG poFUXUvp9ncFVqFayObwzjfRiHA1PkytIRg2YhU7EQxMoCTueUdGcZbf3jHobU9XevLJV6zYZMRwljRUQ2dre8T+oooDjXqbvnDXigfQv6fOQl2OaWgrzZpl 8IAyC7rZJB4OBEi3w/sxO990GGLTiaevnFtd9lq6mBMVnas/hjDI6DryvtT6UXZfn+7YxrSZ Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org 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 = ; > > + allwinner,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 From mboxrd@z Thu Jan 1 00:00:00 1970 From: antony@phenome.org (Antony Antony) Date: Fri, 1 Sep 2017 12:53:13 +0200 Subject: [PATCH v5] arm64: allwinner: h5: add support for NanoPi NEO Plus2 In-Reply-To: <20170831145859.rief3fqo36ns23rm@flea> References: <20170824231716.2623-1-antony@phenome.org> <20170830125057.38529-1-antony@phenome.org> <20170831145859.rief3fqo36ns23rm@flea> Message-ID: <20170901105313.m26y2re3ulskua43@AntonyAntony.local> To: linux-arm-kernel@lists.infradead.org List-Id: linux-arm-kernel.lists.infradead.org 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 at 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 at 1c30000/mdio/ethernet-phy at 7"; ethernet at 1c30000 { mdio { .. ethernet-phy at 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 at 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 at 0 { > > + pins = "PA10"; > > + function = "gpio_out"; > > + }; > > + gmac_power_pin_nanopi: gmac_power_pin at 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 at 0 { > > + pins = "PL10"; > > + function = "gpio_out"; > > + }; > > + > > + vdd_cpux_r_npi: regulator_pins at 0 { > > + allwinner,pins = "PL6"; > > + allwinner,function = "gpio_out"; > > + allwinner,drive = ; > > + allwinner,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