Re: [PATCH v2 1/3] ARM: sunxi: add support for H2+ SoC

Re: [PATCH v2 1/3] ARM: sunxi: add support for H2+ SoC

[Maxime Ripard]

[-- Attachment #1: Type: text/plain, Size: 1899 bytes --]

Hi,

On Tue, Nov 22, 2016 at 12:24:19AM +0800, Icenowy Zheng wrote:
> Allwinner H2+ is a quad-core Cortex-A7 SoC.
> 
> It is very like H3, that they share the same SoC ID (0x1680), and H3
> memory maps as well as drivers works well on the SoC.
> 
> Signed-off-by: Icenowy Zheng <icenowy@aosc.xyz>
> ---
>  Documentation/arm/sunxi/README                  | 4 ++++
>  Documentation/devicetree/bindings/arm/sunxi.txt | 1 +
>  arch/arm/mach-sunxi/sunxi.c                     | 1 +
>  3 files changed, 6 insertions(+)
> 
> diff --git a/Documentation/arm/sunxi/README b/Documentation/arm/sunxi/README
> index cd02433..1fe4d99c 100644
> --- a/Documentation/arm/sunxi/README
> +++ b/Documentation/arm/sunxi/README
> @@ -63,6 +63,10 @@ SunXi family
>          + User Manual
>            http://dl.linux-sunxi.org/A33/A33%20user%20manual%20release%201.1.pdf
>  
> +      - Allwinner H2+ (sun8i)
> +        + No document available now, but is known to be working properly with
> +          H3 drivers and memory map.
> +

I'm not sure the phrasing is right here. I would prefer something like:

"No document publicly available now, but looks very similar to the H3" 

>        - Allwinner H3 (sun8i)
>          + Datasheet
>            http://dl.linux-sunxi.org/H3/Allwinner_H3_Datasheet_V1.0.pdf
> diff --git a/Documentation/devicetree/bindings/arm/sunxi.txt b/Documentation/devicetree/bindings/arm/sunxi.txt
> index 4d6467c..26b35a7 100644
> --- a/Documentation/devicetree/bindings/arm/sunxi.txt
> +++ b/Documentation/devicetree/bindings/arm/sunxi.txt
> @@ -13,6 +13,7 @@ using one of the following compatible strings:
>    allwinner,sun8i-a33
>    allwinner,sun8i-a83t
>    allwinner,sun8i-h3
> +  allwinner,sun8i-h2plus

h2-plus please.

Thanks!
Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 801 bytes --]

Re: [PATCH v2 2/3] ARM: dts: sunxi: add support for Orange Pi Zero board

[Maxime Ripard]

[-- Attachment #1: Type: text/plain, Size: 5888 bytes --]

On Tue, Nov 22, 2016 at 12:24:20AM +0800, Icenowy Zheng wrote:
> Orange Pi Zero is a board that came with the new Allwinner H2+ SoC.
> 
> Add a device tree file for it.
> 
> Signed-off-by: Icenowy Zheng <icenowy@aosc.xyz>
> ---
> Changes since v2:
> - Use generic pinconf binding instead of legacy allwinner pinctrl binding.
> - removed uart3, which is not accessible on Orange Pi Zero.
> - Removed sun8i-h2plus.dtsi and make Orange Pi Zero dts directly include
>   sun8i-h3.dtsi.
> - Removed allwinner,sun8i-h3 compatible.
> 
>  arch/arm/boot/dts/Makefile                       |   1 +
>  arch/arm/boot/dts/sun8i-h2plus-orangepi-zero.dts | 137 +++++++++++++++++++++++

Ditto, h2-plus-orangepi-zero.

>  2 files changed, 138 insertions(+)
>  create mode 100644 arch/arm/boot/dts/sun8i-h2plus-orangepi-zero.dts
> 
> diff --git a/arch/arm/boot/dts/Makefile b/arch/arm/boot/dts/Makefile
> index 802a10d..51a1dd7 100644
> --- a/arch/arm/boot/dts/Makefile
> +++ b/arch/arm/boot/dts/Makefile
> @@ -834,6 +834,7 @@ dtb-$(CONFIG_MACH_SUN8I) += \
>  	sun8i-a33-sinlinx-sina33.dtb \
>  	sun8i-a83t-allwinner-h8homlet-v2.dtb \
>  	sun8i-a83t-cubietruck-plus.dtb \
> +	sun8i-h2plus-orangepi-zero.dtb \
>  	sun8i-h3-bananapi-m2-plus.dtb \
>  	sun8i-h3-nanopi-neo.dtb \
>  	sun8i-h3-orangepi-2.dtb \
> diff --git a/arch/arm/boot/dts/sun8i-h2plus-orangepi-zero.dts b/arch/arm/boot/dts/sun8i-h2plus-orangepi-zero.dts
> new file mode 100644
> index 0000000..b428e47
> --- /dev/null
> +++ b/arch/arm/boot/dts/sun8i-h2plus-orangepi-zero.dts
> @@ -0,0 +1,137 @@
> +/*
> + * Copyright (C) 2016 Icenowy Zheng <icenowy@aosc.xyz>
> + *
> + * Based on sun8i-h3-orangepi-one.dts, which is:
> + *   Copyright (C) 2016 Hans de Goede <hdegoede@redhat.com>
> + *
> + * This file is dual-licensed: you can use it either under the terms
> + * of the GPL or the X11 license, at your option. Note that this dual
> + * licensing only applies to this file, and not this project as a
> + * whole.
> + *
> + *  a) This file is free software; you can redistribute it and/or
> + *     modify it under the terms of the GNU General Public License as
> + *     published by the Free Software Foundation; either version 2 of the
> + *     License, or (at your option) any later version.
> + *
> + *     This file is distributed in the hope that it will be useful,
> + *     but WITHOUT ANY WARRANTY; without even the implied warranty of
> + *     MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + *     GNU General Public License for more details.
> + *
> + * Or, alternatively,
> + *
> + *  b) Permission is hereby granted, free of charge, to any person
> + *     obtaining a copy of this software and associated documentation
> + *     files (the "Software"), to deal in the Software without
> + *     restriction, including without limitation the rights to use,
> + *     copy, modify, merge, publish, distribute, sublicense, and/or
> + *     sell copies of the Software, and to permit persons to whom the
> + *     Software is furnished to do so, subject to the following
> + *     conditions:
> + *
> + *     The above copyright notice and this permission notice shall be
> + *     included in all copies or substantial portions of the Software.
> + *
> + *     THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND,
> + *     EXPRESS OR IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES
> + *     OF MERCHANTABILITY, FITNESS FOR A PARTICULAR PURPOSE AND
> + *     NONINFRINGEMENT. IN NO EVENT SHALL THE AUTHORS OR COPYRIGHT
> + *     HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER LIABILITY,
> + *     WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING
> + *     FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR
> + *     OTHER DEALINGS IN THE SOFTWARE.
> + */
> +
> +/dts-v1/;
> +#include "sun8i-h3.dtsi"
> +#include "sunxi-common-regulators.dtsi"
> +
> +#include <dt-bindings/gpio/gpio.h>
> +#include <dt-bindings/input/input.h>
> +#include <dt-bindings/pinctrl/sun4i-a10.h>
> +
> +/ {
> +	model = "Xunlong Orange Pi Zero";
> +	compatible = "xunlong,orangepi-zero", "allwinner,sun8i-h2plus";
> +
> +	aliases {
> +		serial0 = &uart0;
> +	};
> +
> +	chosen {
> +		stdout-path = "serial0:115200n8";
> +	};
> +
> +	leds {
> +		compatible = "gpio-leds";
> +		pinctrl-names = "default";
> +		pinctrl-0 = <&leds_opi0>, <&leds_r_opi0>;
> +
> +		pwr_led {
> +			label = "orangepi:green:pwr";
> +			gpios = <&r_pio 0 10 GPIO_ACTIVE_HIGH>;
> +			default-state = "on";
> +		};
> +
> +		status_led {
> +			label = "orangepi:red:status";
> +			gpios = <&pio 0 17 GPIO_ACTIVE_HIGH>;
> +		};
> +	};
> +};
> +
> +&ehci1 {
> +	status = "okay";
> +};
> +
> +&mmc0 {
> +	pinctrl-names = "default";
> +	pinctrl-0 = <&mmc0_pins_a>, <&mmc0_cd_pin>;
> +	vmmc-supply = <&reg_vcc3v3>;
> +	bus-width = <4>;
> +	cd-gpios = <&pio 5 6 GPIO_ACTIVE_HIGH>; /* PF6 */
> +	cd-inverted;
> +	status = "okay";
> +};
> +
> +&ohci1 {
> +	status = "okay";
> +};
> +
> +&pio {
> +	leds_opi0: led_pins@0 {
> +		pins = "PA17";
> +		function = "gpio_out";
> +	};
> +};
> +
> +&r_pio {
> +	leds_r_opi0: led_pins@0 {
> +		pins = "PL10";
> +		function = "gpio_out";
> +	};
> +};
> +
> +&uart0 {
> +	pinctrl-names = "default";
> +	pinctrl-0 = <&uart0_pins_a>;
> +	status = "okay";
> +};
> +
> +&uart1 {
> +	pinctrl-names = "default";
> +	pinctrl-0 = <&uart1_pins>;
> +	status = "disabled";
> +};
> +
> +&uart2 {
> +	pinctrl-names = "default";
> +	pinctrl-0 = <&uart2_pins>;
> +	status = "disabled";
> +};

I'm not sure you answered me on this one. Are those exposed on the
headers? why did you put them as disabled here?

Thanks!
Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 801 bytes --]

Re: [PATCH v2 2/3] ARM: dts: sunxi: add support for Orange Pi Zero board

[Andre Przywara]

Hi Maxime,

On 23/11/16 07:57, Maxime Ripard wrote:
> On Tue, Nov 22, 2016 at 12:24:20AM +0800, Icenowy Zheng wrote:
>> Orange Pi Zero is a board that came with the new Allwinner H2+ SoC.
>>
>> Add a device tree file for it.
>>
>> Signed-off-by: Icenowy Zheng <icenowy@aosc.xyz>
>> ---
>> Changes since v2:
>> - Use generic pinconf binding instead of legacy allwinner pinctrl binding.
>> - removed uart3, which is not accessible on Orange Pi Zero.
>> - Removed sun8i-h2plus.dtsi and make Orange Pi Zero dts directly include
>>   sun8i-h3.dtsi.
>> - Removed allwinner,sun8i-h3 compatible.
>>
>>  arch/arm/boot/dts/Makefile                       |   1 +
>>  arch/arm/boot/dts/sun8i-h2plus-orangepi-zero.dts | 137 +++++++++++++++++++++++
> 
> Ditto, h2-plus-orangepi-zero.
> 
>>  2 files changed, 138 insertions(+)
>>  create mode 100644 arch/arm/boot/dts/sun8i-h2plus-orangepi-zero.dts
>>
>> diff --git a/arch/arm/boot/dts/Makefile b/arch/arm/boot/dts/Makefile
>> index 802a10d..51a1dd7 100644
>> --- a/arch/arm/boot/dts/Makefile
>> +++ b/arch/arm/boot/dts/Makefile
>> @@ -834,6 +834,7 @@ dtb-$(CONFIG_MACH_SUN8I) += \
>>  	sun8i-a33-sinlinx-sina33.dtb \
>>  	sun8i-a83t-allwinner-h8homlet-v2.dtb \
>>  	sun8i-a83t-cubietruck-plus.dtb \
>> +	sun8i-h2plus-orangepi-zero.dtb \
>>  	sun8i-h3-bananapi-m2-plus.dtb \
>>  	sun8i-h3-nanopi-neo.dtb \
>>  	sun8i-h3-orangepi-2.dtb \
>> diff --git a/arch/arm/boot/dts/sun8i-h2plus-orangepi-zero.dts b/arch/arm/boot/dts/sun8i-h2plus-orangepi-zero.dts
>> new file mode 100644
>> index 0000000..b428e47
>> --- /dev/null
>> +++ b/arch/arm/boot/dts/sun8i-h2plus-orangepi-zero.dts
>> @@ -0,0 +1,137 @@
>> +/*
>> + * Copyright (C) 2016 Icenowy Zheng <icenowy@aosc.xyz>
>> + *
>> + * Based on sun8i-h3-orangepi-one.dts, which is:
>> + *   Copyright (C) 2016 Hans de Goede <hdegoede@redhat.com>
>> + *
>> + * This file is dual-licensed: you can use it either under the terms
>> + * of the GPL or the X11 license, at your option. Note that this dual
>> + * licensing only applies to this file, and not this project as a
>> + * whole.
>> + *
>> + *  a) This file is free software; you can redistribute it and/or
>> + *     modify it under the terms of the GNU General Public License as
>> + *     published by the Free Software Foundation; either version 2 of the
>> + *     License, or (at your option) any later version.
>> + *
>> + *     This file is distributed in the hope that it will be useful,
>> + *     but WITHOUT ANY WARRANTY; without even the implied warranty of
>> + *     MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
>> + *     GNU General Public License for more details.
>> + *
>> + * Or, alternatively,
>> + *
>> + *  b) Permission is hereby granted, free of charge, to any person
>> + *     obtaining a copy of this software and associated documentation
>> + *     files (the "Software"), to deal in the Software without
>> + *     restriction, including without limitation the rights to use,
>> + *     copy, modify, merge, publish, distribute, sublicense, and/or
>> + *     sell copies of the Software, and to permit persons to whom the
>> + *     Software is furnished to do so, subject to the following
>> + *     conditions:
>> + *
>> + *     The above copyright notice and this permission notice shall be
>> + *     included in all copies or substantial portions of the Software.
>> + *
>> + *     THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND,
>> + *     EXPRESS OR IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES
>> + *     OF MERCHANTABILITY, FITNESS FOR A PARTICULAR PURPOSE AND
>> + *     NONINFRINGEMENT. IN NO EVENT SHALL THE AUTHORS OR COPYRIGHT
>> + *     HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER LIABILITY,
>> + *     WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING
>> + *     FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR
>> + *     OTHER DEALINGS IN THE SOFTWARE.
>> + */
>> +
>> +/dts-v1/;
>> +#include "sun8i-h3.dtsi"
>> +#include "sunxi-common-regulators.dtsi"
>> +
>> +#include <dt-bindings/gpio/gpio.h>
>> +#include <dt-bindings/input/input.h>
>> +#include <dt-bindings/pinctrl/sun4i-a10.h>
>> +
>> +/ {
>> +	model = "Xunlong Orange Pi Zero";
>> +	compatible = "xunlong,orangepi-zero", "allwinner,sun8i-h2plus";
>> +
>> +	aliases {
>> +		serial0 = &uart0;
>> +	};
>> +
>> +	chosen {
>> +		stdout-path = "serial0:115200n8";
>> +	};
>> +
>> +	leds {
>> +		compatible = "gpio-leds";
>> +		pinctrl-names = "default";
>> +		pinctrl-0 = <&leds_opi0>, <&leds_r_opi0>;
>> +
>> +		pwr_led {
>> +			label = "orangepi:green:pwr";
>> +			gpios = <&r_pio 0 10 GPIO_ACTIVE_HIGH>;
>> +			default-state = "on";
>> +		};
>> +
>> +		status_led {
>> +			label = "orangepi:red:status";
>> +			gpios = <&pio 0 17 GPIO_ACTIVE_HIGH>;
>> +		};
>> +	};
>> +};
>> +
>> +&ehci1 {
>> +	status = "okay";
>> +};
>> +
>> +&mmc0 {
>> +	pinctrl-names = "default";
>> +	pinctrl-0 = <&mmc0_pins_a>, <&mmc0_cd_pin>;
>> +	vmmc-supply = <&reg_vcc3v3>;
>> +	bus-width = <4>;
>> +	cd-gpios = <&pio 5 6 GPIO_ACTIVE_HIGH>; /* PF6 */
>> +	cd-inverted;
>> +	status = "okay";
>> +};
>> +
>> +&ohci1 {
>> +	status = "okay";
>> +};
>> +
>> +&pio {
>> +	leds_opi0: led_pins@0 {
>> +		pins = "PA17";
>> +		function = "gpio_out";
>> +	};
>> +};
>> +
>> +&r_pio {
>> +	leds_r_opi0: led_pins@0 {
>> +		pins = "PL10";
>> +		function = "gpio_out";
>> +	};
>> +};
>> +
>> +&uart0 {
>> +	pinctrl-names = "default";
>> +	pinctrl-0 = <&uart0_pins_a>;
>> +	status = "okay";
>> +};
>> +
>> +&uart1 {
>> +	pinctrl-names = "default";
>> +	pinctrl-0 = <&uart1_pins>;
>> +	status = "disabled";
>> +};
>> +
>> +&uart2 {
>> +	pinctrl-names = "default";
>> +	pinctrl-0 = <&uart2_pins>;
>> +	status = "disabled";
>> +};
> 
> I'm not sure you answered me on this one. Are those exposed on the
> headers? why did you put them as disabled here?

So they are on headers, though you have to solder the actual header pins
yourself [1]. But also these are the normal pins multiplexed with GPIOs
and other peripherals, so keeping them disabled is in line with the
existing policy, if I got this correctly.

I agree that the status="disabled" is redundant, since we have that
exact line already in the .dtsi. But I saw it in other DTs as well, most
prominently in the sun8i-h3-orangepi-one.dts.

So I think we should remove the "status=" lines here, dtc will generate
an identical dtb out of it. But we should keep the uart descriptions in
to make it easier for users to see which SoC pins are used for these
pins labeled UART[012] in the board description and schematic. Also all
it takes to enable those is to overwrite the status property, which can
easily be done inline (without resizing the dtb).

Cheers,
Andre.

[1] http://linux-sunxi.org/Xunlong_Orange_Pi_Zero

Re: [PATCH v2 2/3] ARM: dts: sunxi: add support for Orange Pi Zero board

[Maxime Ripard]

[-- Attachment #1: Type: text/plain, Size: 7684 bytes --]

On Wed, Nov 23, 2016 at 09:23:49AM +0000, Andre Przywara wrote:
> Hi Maxime,
> 
> On 23/11/16 07:57, Maxime Ripard wrote:
> > On Tue, Nov 22, 2016 at 12:24:20AM +0800, Icenowy Zheng wrote:
> >> Orange Pi Zero is a board that came with the new Allwinner H2+ SoC.
> >>
> >> Add a device tree file for it.
> >>
> >> Signed-off-by: Icenowy Zheng <icenowy@aosc.xyz>
> >> ---
> >> Changes since v2:
> >> - Use generic pinconf binding instead of legacy allwinner pinctrl binding.
> >> - removed uart3, which is not accessible on Orange Pi Zero.
> >> - Removed sun8i-h2plus.dtsi and make Orange Pi Zero dts directly include
> >>   sun8i-h3.dtsi.
> >> - Removed allwinner,sun8i-h3 compatible.
> >>
> >>  arch/arm/boot/dts/Makefile                       |   1 +
> >>  arch/arm/boot/dts/sun8i-h2plus-orangepi-zero.dts | 137 +++++++++++++++++++++++
> > 
> > Ditto, h2-plus-orangepi-zero.
> > 
> >>  2 files changed, 138 insertions(+)
> >>  create mode 100644 arch/arm/boot/dts/sun8i-h2plus-orangepi-zero.dts
> >>
> >> diff --git a/arch/arm/boot/dts/Makefile b/arch/arm/boot/dts/Makefile
> >> index 802a10d..51a1dd7 100644
> >> --- a/arch/arm/boot/dts/Makefile
> >> +++ b/arch/arm/boot/dts/Makefile
> >> @@ -834,6 +834,7 @@ dtb-$(CONFIG_MACH_SUN8I) += \
> >>  	sun8i-a33-sinlinx-sina33.dtb \
> >>  	sun8i-a83t-allwinner-h8homlet-v2.dtb \
> >>  	sun8i-a83t-cubietruck-plus.dtb \
> >> +	sun8i-h2plus-orangepi-zero.dtb \
> >>  	sun8i-h3-bananapi-m2-plus.dtb \
> >>  	sun8i-h3-nanopi-neo.dtb \
> >>  	sun8i-h3-orangepi-2.dtb \
> >> diff --git a/arch/arm/boot/dts/sun8i-h2plus-orangepi-zero.dts b/arch/arm/boot/dts/sun8i-h2plus-orangepi-zero.dts
> >> new file mode 100644
> >> index 0000000..b428e47
> >> --- /dev/null
> >> +++ b/arch/arm/boot/dts/sun8i-h2plus-orangepi-zero.dts
> >> @@ -0,0 +1,137 @@
> >> +/*
> >> + * Copyright (C) 2016 Icenowy Zheng <icenowy@aosc.xyz>
> >> + *
> >> + * Based on sun8i-h3-orangepi-one.dts, which is:
> >> + *   Copyright (C) 2016 Hans de Goede <hdegoede@redhat.com>
> >> + *
> >> + * This file is dual-licensed: you can use it either under the terms
> >> + * of the GPL or the X11 license, at your option. Note that this dual
> >> + * licensing only applies to this file, and not this project as a
> >> + * whole.
> >> + *
> >> + *  a) This file is free software; you can redistribute it and/or
> >> + *     modify it under the terms of the GNU General Public License as
> >> + *     published by the Free Software Foundation; either version 2 of the
> >> + *     License, or (at your option) any later version.
> >> + *
> >> + *     This file is distributed in the hope that it will be useful,
> >> + *     but WITHOUT ANY WARRANTY; without even the implied warranty of
> >> + *     MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> >> + *     GNU General Public License for more details.
> >> + *
> >> + * Or, alternatively,
> >> + *
> >> + *  b) Permission is hereby granted, free of charge, to any person
> >> + *     obtaining a copy of this software and associated documentation
> >> + *     files (the "Software"), to deal in the Software without
> >> + *     restriction, including without limitation the rights to use,
> >> + *     copy, modify, merge, publish, distribute, sublicense, and/or
> >> + *     sell copies of the Software, and to permit persons to whom the
> >> + *     Software is furnished to do so, subject to the following
> >> + *     conditions:
> >> + *
> >> + *     The above copyright notice and this permission notice shall be
> >> + *     included in all copies or substantial portions of the Software.
> >> + *
> >> + *     THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND,
> >> + *     EXPRESS OR IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES
> >> + *     OF MERCHANTABILITY, FITNESS FOR A PARTICULAR PURPOSE AND
> >> + *     NONINFRINGEMENT. IN NO EVENT SHALL THE AUTHORS OR COPYRIGHT
> >> + *     HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER LIABILITY,
> >> + *     WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING
> >> + *     FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR
> >> + *     OTHER DEALINGS IN THE SOFTWARE.
> >> + */
> >> +
> >> +/dts-v1/;
> >> +#include "sun8i-h3.dtsi"
> >> +#include "sunxi-common-regulators.dtsi"
> >> +
> >> +#include <dt-bindings/gpio/gpio.h>
> >> +#include <dt-bindings/input/input.h>
> >> +#include <dt-bindings/pinctrl/sun4i-a10.h>
> >> +
> >> +/ {
> >> +	model = "Xunlong Orange Pi Zero";
> >> +	compatible = "xunlong,orangepi-zero", "allwinner,sun8i-h2plus";
> >> +
> >> +	aliases {
> >> +		serial0 = &uart0;
> >> +	};
> >> +
> >> +	chosen {
> >> +		stdout-path = "serial0:115200n8";
> >> +	};
> >> +
> >> +	leds {
> >> +		compatible = "gpio-leds";
> >> +		pinctrl-names = "default";
> >> +		pinctrl-0 = <&leds_opi0>, <&leds_r_opi0>;
> >> +
> >> +		pwr_led {
> >> +			label = "orangepi:green:pwr";
> >> +			gpios = <&r_pio 0 10 GPIO_ACTIVE_HIGH>;
> >> +			default-state = "on";
> >> +		};
> >> +
> >> +		status_led {
> >> +			label = "orangepi:red:status";
> >> +			gpios = <&pio 0 17 GPIO_ACTIVE_HIGH>;
> >> +		};
> >> +	};
> >> +};
> >> +
> >> +&ehci1 {
> >> +	status = "okay";
> >> +};
> >> +
> >> +&mmc0 {
> >> +	pinctrl-names = "default";
> >> +	pinctrl-0 = <&mmc0_pins_a>, <&mmc0_cd_pin>;
> >> +	vmmc-supply = <&reg_vcc3v3>;
> >> +	bus-width = <4>;
> >> +	cd-gpios = <&pio 5 6 GPIO_ACTIVE_HIGH>; /* PF6 */
> >> +	cd-inverted;
> >> +	status = "okay";
> >> +};
> >> +
> >> +&ohci1 {
> >> +	status = "okay";
> >> +};
> >> +
> >> +&pio {
> >> +	leds_opi0: led_pins@0 {
> >> +		pins = "PA17";
> >> +		function = "gpio_out";
> >> +	};
> >> +};
> >> +
> >> +&r_pio {
> >> +	leds_r_opi0: led_pins@0 {
> >> +		pins = "PL10";
> >> +		function = "gpio_out";
> >> +	};
> >> +};
> >> +
> >> +&uart0 {
> >> +	pinctrl-names = "default";
> >> +	pinctrl-0 = <&uart0_pins_a>;
> >> +	status = "okay";
> >> +};
> >> +
> >> +&uart1 {
> >> +	pinctrl-names = "default";
> >> +	pinctrl-0 = <&uart1_pins>;
> >> +	status = "disabled";
> >> +};
> >> +
> >> +&uart2 {
> >> +	pinctrl-names = "default";
> >> +	pinctrl-0 = <&uart2_pins>;
> >> +	status = "disabled";
> >> +};
> > 
> > I'm not sure you answered me on this one. Are those exposed on the
> > headers? why did you put them as disabled here?
> 
> So they are on headers, though you have to solder the actual header pins
> yourself [1]. But also these are the normal pins multiplexed with GPIOs
> and other peripherals, so keeping them disabled is in line with the
> existing policy, if I got this correctly.
> 
> I agree that the status="disabled" is redundant, since we have that
> exact line already in the .dtsi. But I saw it in other DTs as well, most
> prominently in the sun8i-h3-orangepi-one.dts.
> 
> So I think we should remove the "status=" lines here, dtc will generate
> an identical dtb out of it. But we should keep the uart descriptions in
> to make it easier for users to see which SoC pins are used for these
> pins labeled UART[012] in the board description and schematic. Also all
> it takes to enable those is to overwrite the status property, which can
> easily be done inline (without resizing the dtb).

I'd rather have the status still in the DTS. It's true that it's
redundant, but it's also explicit. A node without any status would
give the impression that it is actually enabled, especially since a
node without a status is going to be probed.

Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 801 bytes --]

Re: [PATCH v2 2/3] ARM: dts: sunxi: add support for Orange Pi Zero board

[André Przywara]

On 27/11/16 09:36, Icenowy Zheng wrote:

Hi,

> 22.11.2016, 00:26, "Icenowy Zheng" <icenowy@aosc.xyz>:
>> Orange Pi Zero is a board that came with the new Allwinner H2+ SoC.
>>
>> Add a device tree file for it.
>>
>> Signed-off-by: Icenowy Zheng <icenowy@aosc.xyz>
>> ---
>> Changes since v2:
>> - Use generic pinconf binding instead of legacy allwinner pinctrl binding.
>> - removed uart3, which is not accessible on Orange Pi Zero.
>> - Removed sun8i-h2plus.dtsi and make Orange Pi Zero dts directly include
>>   sun8i-h3.dtsi.
>> - Removed allwinner,sun8i-h3 compatible.
>>
>>  arch/arm/boot/dts/Makefile | 1 +
>>  arch/arm/boot/dts/sun8i-h2plus-orangepi-zero.dts | 137 +++++++++++++++++++++++
>>  2 files changed, 138 insertions(+)
>>  create mode 100644 arch/arm/boot/dts/sun8i-h2plus-orangepi-zero.dts
>>
>> diff --git a/arch/arm/boot/dts/Makefile b/arch/arm/boot/dts/Makefile
>> index 802a10d..51a1dd7 100644
>> --- a/arch/arm/boot/dts/Makefile
>> +++ b/arch/arm/boot/dts/Makefile
>> @@ -834,6 +834,7 @@ dtb-$(CONFIG_MACH_SUN8I) += \
>>          sun8i-a33-sinlinx-sina33.dtb \
>>          sun8i-a83t-allwinner-h8homlet-v2.dtb \
>>          sun8i-a83t-cubietruck-plus.dtb \
>> + sun8i-h2plus-orangepi-zero.dtb \
>>          sun8i-h3-bananapi-m2-plus.dtb \
>>          sun8i-h3-nanopi-neo.dtb \
>>          sun8i-h3-orangepi-2.dtb \
>> diff --git a/arch/arm/boot/dts/sun8i-h2plus-orangepi-zero.dts b/arch/arm/boot/dts/sun8i-h2plus-orangepi-zero.dts
>> new file mode 100644
>> index 0000000..b428e47
>> --- /dev/null
>> +++ b/arch/arm/boot/dts/sun8i-h2plus-orangepi-zero.dts
>> @@ -0,0 +1,137 @@
>> +/*
>> + * Copyright (C) 2016 Icenowy Zheng <icenowy@aosc.xyz>
>> + *
>> + * Based on sun8i-h3-orangepi-one.dts, which is:
>> + * Copyright (C) 2016 Hans de Goede <hdegoede@redhat.com>
>> + *
>> + * This file is dual-licensed: you can use it either under the terms
>> + * of the GPL or the X11 license, at your option. Note that this dual
>> + * licensing only applies to this file, and not this project as a
>> + * whole.
>> + *
>> + * a) This file is free software; you can redistribute it and/or
>> + * modify it under the terms of the GNU General Public License as
>> + * published by the Free Software Foundation; either version 2 of the
>> + * License, or (at your option) any later version.
>> + *
>> + * This file is distributed in the hope that it will be useful,
>> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
>> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
>> + * GNU General Public License for more details.
>> + *
>> + * Or, alternatively,
>> + *
>> + * b) Permission is hereby granted, free of charge, to any person
>> + * obtaining a copy of this software and associated documentation
>> + * files (the "Software"), to deal in the Software without
>> + * restriction, including without limitation the rights to use,
>> + * copy, modify, merge, publish, distribute, sublicense, and/or
>> + * sell copies of the Software, and to permit persons to whom the
>> + * Software is furnished to do so, subject to the following
>> + * conditions:
>> + *
>> + * The above copyright notice and this permission notice shall be
>> + * included in all copies or substantial portions of the Software.
>> + *
>> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND,
>> + * EXPRESS OR IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES
>> + * OF MERCHANTABILITY, FITNESS FOR A PARTICULAR PURPOSE AND
>> + * NONINFRINGEMENT. IN NO EVENT SHALL THE AUTHORS OR COPYRIGHT
>> + * HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER LIABILITY,
>> + * WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING
>> + * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR
>> + * OTHER DEALINGS IN THE SOFTWARE.
>> + */
>> +
>> +/dts-v1/;
>> +#include "sun8i-h3.dtsi"
>> +#include "sunxi-common-regulators.dtsi"
>> +
>> +#include <dt-bindings/gpio/gpio.h>
>> +#include <dt-bindings/input/input.h>
>> +#include <dt-bindings/pinctrl/sun4i-a10.h>
>> +
>> +/ {
>> + model = "Xunlong Orange Pi Zero";
>> + compatible = "xunlong,orangepi-zero", "allwinner,sun8i-h2plus";
>> +
>> + aliases {
>> + serial0 = &uart0;
>> + };
>> +
>> + chosen {
>> + stdout-path = "serial0:115200n8";
>> + };
>> +
>> + leds {
>> + compatible = "gpio-leds";
>> + pinctrl-names = "default";
>> + pinctrl-0 = <&leds_opi0>, <&leds_r_opi0>;
>> +
>> + pwr_led {
>> + label = "orangepi:green:pwr";
>> + gpios = <&r_pio 0 10 GPIO_ACTIVE_HIGH>;
>> + default-state = "on";
>> + };
>> +
>> + status_led {
>> + label = "orangepi:red:status";
>> + gpios = <&pio 0 17 GPIO_ACTIVE_HIGH>;
>> + };
>> + };
>> +};
>> +
>> +&ehci1 {
>> + status = "okay";
>> +};
>> +
>> +&mmc0 {
>> + pinctrl-names = "default";
>> + pinctrl-0 = <&mmc0_pins_a>, <&mmc0_cd_pin>;
>> + vmmc-supply = <&reg_vcc3v3>;
>> + bus-width = <4>;
>> + cd-gpios = <&pio 5 6 GPIO_ACTIVE_HIGH>; /* PF6 */
>> + cd-inverted;
>> + status = "okay";
>> +};
>> +
>> +&ohci1 {
>> + status = "okay";
>> +};
>> +
>> +&pio {
>> + leds_opi0: led_pins@0 {
>> + pins = "PA17";
>> + function = "gpio_out";
>> + };
>> +};
>> +
>> +&r_pio {
>> + leds_r_opi0: led_pins@0 {
>> + pins = "PL10";
>> + function = "gpio_out";
>> + };
>> +};
>> +
>> +&uart0 {
>> + pinctrl-names = "default";
>> + pinctrl-0 = <&uart0_pins_a>;
>> + status = "okay";
>> +};
>> +
>> +&uart1 {
>> + pinctrl-names = "default";
>> + pinctrl-0 = <&uart1_pins>;
>> + status = "disabled";
>> +};
>> +
>> +&uart2 {
>> + pinctrl-names = "default";
>> + pinctrl-0 = <&uart2_pins>;
>> + status = "disabled";
>> +};
>> +
>> +&usbphy {
>> + /* USB VBUS is always on */
>> + status = "okay";
>> +};
> 
> Something more interesting happened.
> 
> Xunlong made a add-on board for Orange Pi Zero, which exposes the two USB Controllers exported at expansion bus as USB Type-A connectors.
> 
> Also it exposes a analog A/V jack and a microphone.
> 
> Should I enable {e,o}hci{2.3} in the device tree?

Actually we should do this regardless of this extension board. The USB
pins are not multiplexed and are exposed on user accessible pins (just
not soldered, but that's a detail), so I think they qualify for DT
enablement. And even if a user can't use them, it doesn't hurt to have
them (since they are not multiplexed).

Cheers,
Andre.

Re: [PATCH v2 2/3] ARM: dts: sunxi: add support for Orange Pi Zero board

[Maxime Ripard]

[-- Attachment #1: Type: text/plain, Size: 1083 bytes --]

On Mon, Nov 28, 2016 at 12:29:07AM +0000, André Przywara wrote:
> > Something more interesting happened.
> > 
> > Xunlong made a add-on board for Orange Pi Zero, which exposes the
> > two USB Controllers exported at expansion bus as USB Type-A
> > connectors.
> > 
> > Also it exposes a analog A/V jack and a microphone.
> > 
> > Should I enable {e,o}hci{2.3} in the device tree?
> 
> Actually we should do this regardless of this extension board. The USB
> pins are not multiplexed and are exposed on user accessible pins (just
> not soldered, but that's a detail), so I think they qualify for DT
> enablement. And even if a user can't use them, it doesn't hurt to have
> them (since they are not multiplexed).

My main concern about this is that we'll leave regulators enabled by
default, for a minority of users. And that minority will prevent to do
a proper power management when the times come since we'll have to keep
that behaviour forever.

Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 801 bytes --]

Re: [PATCH v2 2/3] ARM: dts: sunxi: add support for Orange Pi Zero board

[Hans de Goede]

Hi,

On 02-12-16 15:22, Icenowy Zheng wrote:
>
>
> 01.12.2016, 17:36, "Maxime Ripard" <maxime.ripard@free-electrons.com>:
>> On Mon, Nov 28, 2016 at 12:29:07AM +0000, André Przywara wrote:
>>>  > Something more interesting happened.
>>>  >
>>>  > Xunlong made a add-on board for Orange Pi Zero, which exposes the
>>>  > two USB Controllers exported at expansion bus as USB Type-A
>>>  > connectors.
>>>  >
>>>  > Also it exposes a analog A/V jack and a microphone.
>>>  >
>>>  > Should I enable {e,o}hci{2.3} in the device tree?
>>>
>>>  Actually we should do this regardless of this extension board. The USB
>>>  pins are not multiplexed and are exposed on user accessible pins (just
>>>  not soldered, but that's a detail), so I think they qualify for DT
>>>  enablement. And even if a user can't use them, it doesn't hurt to have
>>>  them (since they are not multiplexed).
>>
>> My main concern about this is that we'll leave regulators enabled by
>> default, for a minority of users. And that minority will prevent to do
>> a proper power management when the times come since we'll have to keep
>> that behaviour forever.
>
> I think these users can add a 'fdt set /xxx/xxx status "disabled" ' .

I don't think that will be necessary I'm pretty sure these extra usb
ports do not have a regulator for the Vbus, they just hook directly
to the 5V rail, can someone with a schematic check ?

Regards,

Hans

Re: [PATCH v2 2/3] ARM: dts: sunxi: add support for Orange Pi Zero board

[Andre Przywara]

Hi,

On 02/12/16 14:32, Icenowy Zheng wrote:
> 
> 
> 02.12.2016, 22:30, "Hans de Goede" <hdegoede@redhat.com>:
>> Hi,
>>
>> On 02-12-16 15:22, Icenowy Zheng wrote:
>>>  01.12.2016, 17:36, "Maxime Ripard" <maxime.ripard@free-electrons.com>:
>>>>  On Mon, Nov 28, 2016 at 12:29:07AM +0000, André Przywara wrote:
>>>>>   > Something more interesting happened.
>>>>>   >
>>>>>   > Xunlong made a add-on board for Orange Pi Zero, which exposes the
>>>>>   > two USB Controllers exported at expansion bus as USB Type-A
>>>>>   > connectors.
>>>>>   >
>>>>>   > Also it exposes a analog A/V jack and a microphone.
>>>>>   >
>>>>>   > Should I enable {e,o}hci{2.3} in the device tree?
>>>>>
>>>>>   Actually we should do this regardless of this extension board. The USB
>>>>>   pins are not multiplexed and are exposed on user accessible pins (just
>>>>>   not soldered, but that's a detail), so I think they qualify for DT
>>>>>   enablement. And even if a user can't use them, it doesn't hurt to have
>>>>>   them (since they are not multiplexed).
>>>>
>>>>  My main concern about this is that we'll leave regulators enabled by
>>>>  default, for a minority of users. And that minority will prevent to do
>>>>  a proper power management when the times come since we'll have to keep
>>>>  that behaviour forever.
>>>
>>>  I think these users can add a 'fdt set /xxx/xxx status "disabled" ' .
>>
>> I don't think that will be necessary I'm pretty sure these extra usb
>> ports do not have a regulator for the Vbus, they just hook directly
>> to the 5V rail, can someone with a schematic check ?
> 
> We seems to have still no schematics for the add-on board.

>From looking at the picture of that expansion board on the Aliexpress
page and chasing the tracks, there is clearly no voltage regulator on
there, it's just passive components. The 5V pin from the headers is
routed forth and back between the two layers via some vias directly to
the 5V pins of the USB sockets.

> But something is sure is that there's no any regulator-related pins
> on the add-on pinout. There's only USB DM and DP pins.
> 
> So the Vbus must be directly connected to +5V.

So yes, it is.

But I think the question is moot anyways, since we don't provide DT
support for that add-on board at that point anyways.
One could imagine another board, though, which has regulators switched
by GPIOs, but that would be their problem and they would have regulators
specified in their specific DT snippet, then.

So to summarize:
- For that specific Orange Pi Zero board which we discuss the DT for
there is no regulator support for the additional USB ports. Thus nothing
we could turn off to save power.
- A user could just take these USB brackets with pin headers that are so
common in PCs to connect additional USB ports to the back of the box.
One just needs to re-sort the pins, which is a matter of a minute.
- As long as we don't provide any easy way of handling DT changes, we
should enable the USB ports for the sake of the users of either those
brackets or the expansion board. Any more sophisticated USB expansion
board with regulators would need to amend the DT anyway.

Does that make sense?

Cheers,
Andre.

Re: [PATCH v2 2/3] ARM: dts: sunxi: add support for Orange Pi Zero board

[Chen-Yu Tsai]

On Sat, Dec 3, 2016 at 12:10 AM, Andre Przywara <andre.przywara@arm.com> wrote:
> Hi,
>
> On 02/12/16 14:32, Icenowy Zheng wrote:
>>
>>
>> 02.12.2016, 22:30, "Hans de Goede" <hdegoede@redhat.com>:
>>> Hi,
>>>
>>> On 02-12-16 15:22, Icenowy Zheng wrote:
>>>>  01.12.2016, 17:36, "Maxime Ripard" <maxime.ripard@free-electrons.com>:
>>>>>  On Mon, Nov 28, 2016 at 12:29:07AM +0000, André Przywara wrote:
>>>>>>   > Something more interesting happened.
>>>>>>   >
>>>>>>   > Xunlong made a add-on board for Orange Pi Zero, which exposes the
>>>>>>   > two USB Controllers exported at expansion bus as USB Type-A
>>>>>>   > connectors.
>>>>>>   >
>>>>>>   > Also it exposes a analog A/V jack and a microphone.
>>>>>>   >
>>>>>>   > Should I enable {e,o}hci{2.3} in the device tree?
>>>>>>
>>>>>>   Actually we should do this regardless of this extension board. The USB
>>>>>>   pins are not multiplexed and are exposed on user accessible pins (just
>>>>>>   not soldered, but that's a detail), so I think they qualify for DT
>>>>>>   enablement. And even if a user can't use them, it doesn't hurt to have
>>>>>>   them (since they are not multiplexed).
>>>>>
>>>>>  My main concern about this is that we'll leave regulators enabled by
>>>>>  default, for a minority of users. And that minority will prevent to do
>>>>>  a proper power management when the times come since we'll have to keep
>>>>>  that behaviour forever.
>>>>
>>>>  I think these users can add a 'fdt set /xxx/xxx status "disabled" ' .
>>>
>>> I don't think that will be necessary I'm pretty sure these extra usb
>>> ports do not have a regulator for the Vbus, they just hook directly
>>> to the 5V rail, can someone with a schematic check ?
>>
>> We seems to have still no schematics for the add-on board.
>
> From looking at the picture of that expansion board on the Aliexpress
> page and chasing the tracks, there is clearly no voltage regulator on
> there, it's just passive components. The 5V pin from the headers is
> routed forth and back between the two layers via some vias directly to
> the 5V pins of the USB sockets.
>
>> But something is sure is that there's no any regulator-related pins
>> on the add-on pinout. There's only USB DM and DP pins.
>>
>> So the Vbus must be directly connected to +5V.
>
> So yes, it is.
>
> But I think the question is moot anyways, since we don't provide DT
> support for that add-on board at that point anyways.
> One could imagine another board, though, which has regulators switched
> by GPIOs, but that would be their problem and they would have regulators
> specified in their specific DT snippet, then.
>
> So to summarize:
> - For that specific Orange Pi Zero board which we discuss the DT for
> there is no regulator support for the additional USB ports. Thus nothing
> we could turn off to save power.
> - A user could just take these USB brackets with pin headers that are so
> common in PCs to connect additional USB ports to the back of the box.
> One just needs to re-sort the pins, which is a matter of a minute.
> - As long as we don't provide any easy way of handling DT changes, we
> should enable the USB ports for the sake of the users of either those
> brackets or the expansion board. Any more sophisticated USB expansion
> board with regulators would need to amend the DT anyway.
>
> Does that make sense?

Sounds good to me.

ChenYu

Re: [PATCH v2 2/3] ARM: dts: sunxi: add support for Orange Pi Zero board

[Maxime Ripard]

[-- Attachment #1: Type: text/plain, Size: 3972 bytes --]

On Fri, Dec 02, 2016 at 04:10:46PM +0000, Andre Przywara wrote:
> Hi,
> 
> On 02/12/16 14:32, Icenowy Zheng wrote:
> > 
> > 
> > 02.12.2016, 22:30, "Hans de Goede" <hdegoede@redhat.com>:
> >> Hi,
> >>
> >> On 02-12-16 15:22, Icenowy Zheng wrote:
> >>>  01.12.2016, 17:36, "Maxime Ripard" <maxime.ripard@free-electrons.com>:
> >>>>  On Mon, Nov 28, 2016 at 12:29:07AM +0000, André Przywara wrote:
> >>>>>   > Something more interesting happened.
> >>>>>   >
> >>>>>   > Xunlong made a add-on board for Orange Pi Zero, which exposes the
> >>>>>   > two USB Controllers exported at expansion bus as USB Type-A
> >>>>>   > connectors.
> >>>>>   >
> >>>>>   > Also it exposes a analog A/V jack and a microphone.
> >>>>>   >
> >>>>>   > Should I enable {e,o}hci{2.3} in the device tree?
> >>>>>
> >>>>>   Actually we should do this regardless of this extension board. The USB
> >>>>>   pins are not multiplexed and are exposed on user accessible pins (just
> >>>>>   not soldered, but that's a detail), so I think they qualify for DT
> >>>>>   enablement. And even if a user can't use them, it doesn't hurt to have
> >>>>>   them (since they are not multiplexed).
> >>>>
> >>>>  My main concern about this is that we'll leave regulators enabled by
> >>>>  default, for a minority of users. And that minority will prevent to do
> >>>>  a proper power management when the times come since we'll have to keep
> >>>>  that behaviour forever.
> >>>
> >>>  I think these users can add a 'fdt set /xxx/xxx status "disabled" ' .
> >>
> >> I don't think that will be necessary I'm pretty sure these extra usb
> >> ports do not have a regulator for the Vbus, they just hook directly
> >> to the 5V rail, can someone with a schematic check ?
> > 
> > We seems to have still no schematics for the add-on board.
> 
> From looking at the picture of that expansion board on the Aliexpress
> page and chasing the tracks, there is clearly no voltage regulator on
> there, it's just passive components. The 5V pin from the headers is
> routed forth and back between the two layers via some vias directly to
> the 5V pins of the USB sockets.
> 
> > But something is sure is that there's no any regulator-related pins
> > on the add-on pinout. There's only USB DM and DP pins.
> > 
> > So the Vbus must be directly connected to +5V.
> 
> So yes, it is.
> 
> But I think the question is moot anyways, since we don't provide DT
> support for that add-on board at that point anyways.
> One could imagine another board, though, which has regulators switched
> by GPIOs, but that would be their problem and they would have regulators
> specified in their specific DT snippet, then.
> 
> So to summarize:
> - For that specific Orange Pi Zero board which we discuss the DT for
> there is no regulator support for the additional USB ports. Thus nothing
> we could turn off to save power.
> - A user could just take these USB brackets with pin headers that are so
> common in PCs to connect additional USB ports to the back of the box.
> One just needs to re-sort the pins, which is a matter of a minute.
> - As long as we don't provide any easy way of handling DT changes, we
> should enable the USB ports for the sake of the users of either those
> brackets or the expansion board. Any more sophisticated USB expansion
> board with regulators would need to amend the DT anyway.

I disagree with this. We already have different ways of changing the
DT at runtime, and even if we didn't I'd still disagree. Once you add
that, there's simply no going back. Saying "let's enable it and we'll
figure it out later" doesn't work, and is essentially a "enable it".

So what you're suggesting is to have those regulators enabled forever,
which might be the case on that board anyway, but definitely shouldn't
be policy.

Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 801 bytes --]

Re: [PATCH v2 2/3] ARM: dts: sunxi: add support for Orange Pi Zero board

[Maxime Ripard]

[-- Attachment #1: Type: text/plain, Size: 1694 bytes --]

On Fri, Dec 02, 2016 at 10:22:30PM +0800, Icenowy Zheng wrote:
> 
> 
> 01.12.2016, 17:36, "Maxime Ripard" <maxime.ripard@free-electrons.com>:
> > On Mon, Nov 28, 2016 at 12:29:07AM +0000, André Przywara wrote:
> >>  > Something more interesting happened.
> >>  >
> >>  > Xunlong made a add-on board for Orange Pi Zero, which exposes the
> >>  > two USB Controllers exported at expansion bus as USB Type-A
> >>  > connectors.
> >>  >
> >>  > Also it exposes a analog A/V jack and a microphone.
> >>  >
> >>  > Should I enable {e,o}hci{2.3} in the device tree?
> >>
> >>  Actually we should do this regardless of this extension board. The USB
> >>  pins are not multiplexed and are exposed on user accessible pins (just
> >>  not soldered, but that's a detail), so I think they qualify for DT
> >>  enablement. And even if a user can't use them, it doesn't hurt to have
> >>  them (since they are not multiplexed).
> >
> > My main concern about this is that we'll leave regulators enabled by
> > default, for a minority of users. And that minority will prevent to do
> > a proper power management when the times come since we'll have to keep
> > that behaviour forever.
> 
> I think these users can add a 'fdt set /xxx/xxx status "disabled" ' .

You can't ask that from the majority of users. These users will take
debian or fedora, install it, and expect everything to work
properly. I would make the opposite argument actually. If someone is
knowledgeable enough to solder the USB pins a connector, then (s)he'll
be able to make that u-boot call.

Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 801 bytes --]

Re: [PATCH 3/3] ARM: dts: sunxi: enable SDIO Wi-Fi on Orange Pi Zero

[Maxime Ripard]

[-- Attachment #1: Type: text/plain, Size: 2865 bytes --]

Hi,

On Tue, Nov 22, 2016 at 12:24:21AM +0800, Icenowy Zheng wrote:
> There's a Allwinner's XR819 SDIO Wi-Fi module soldered on the board of
> Orange Pi Zero, which used a dedicated regulator to power.
> 
> Add the device tree node of the regulator, the enable gpio (with
> mmc-pwrseq) and the sdio controller.
> 
> There's a out-of-tree driver tested to work with this device tree.
> 
> Signed-off-by: Icenowy Zheng <icenowy@aosc.xyz>
> ---
> New patch in the patchset, since a out-of-tree working xradio driver is done.
> 
> If there is any problem in this patch, it can be omitted.

No particular problem with this one, however it can and should be
merged with the previous one.

Minor comments below though.

> 
>  arch/arm/boot/dts/sun8i-h2plus-orangepi-zero.dts | 42 ++++++++++++++++++++++++
>  1 file changed, 42 insertions(+)
> 
> diff --git a/arch/arm/boot/dts/sun8i-h2plus-orangepi-zero.dts b/arch/arm/boot/dts/sun8i-h2plus-orangepi-zero.dts
> index b428e47..39cac26 100644
> --- a/arch/arm/boot/dts/sun8i-h2plus-orangepi-zero.dts
> +++ b/arch/arm/boot/dts/sun8i-h2plus-orangepi-zero.dts
> @@ -79,6 +79,24 @@
>  			gpios = <&pio 0 17 GPIO_ACTIVE_HIGH>;
>  		};
>  	};
> +
> +	reg_vcc_wifi: reg_vcc_wifi {
> +		compatible = "regulator-fixed";
> +		pinctrl-names = "default";
> +		pinctrl-0 = <&vcc_wifi_pin_opi0>;
> +		regulator-min-microvolt = <3300000>;
> +		regulator-max-microvolt = <3300000>;
> +		regulator-name = "vcc-wifi";
> +		enable-active-high;
> +		gpio = <&pio 0 20 GPIO_ACTIVE_HIGH>;
> +	};
> +
> +	wifi_pwrseq: wifi_pwrseq {
> +		compatible = "mmc-pwrseq-simple";
> +		pinctrl-names = "default";
> +		pinctrl-0 = <&wifi_pwrseq_pin_opi0>;
> +		reset-gpios = <&r_pio 0 7 GPIO_ACTIVE_LOW>;
> +	};
>  };
>  
>  &ehci1 {
> @@ -95,6 +113,20 @@
>  	status = "okay";
>  };
>  
> +&mmc1 {
> +	pinctrl-names = "default";
> +	pinctrl-0 = <&mmc1_pins_a>;
> +	vmmc-supply = <&reg_vcc_wifi>;
> +	mmc-pwrseq = <&wifi_pwrseq>;
> +	bus-width = <4>;
> +	non-removable;
> +	status = "okay";
> +};
> +
> +&mmc1_pins_a {
> +	allwinner,pull = <SUN4I_PINCTRL_PULL_UP>;

This should be bias-pull-up.

> +};
> +
>  &ohci1 {
>  	status = "okay";
>  };
> @@ -104,6 +136,11 @@
>  		pins = "PA17";
>  		function = "gpio_out";
>  	};
> +
> +	vcc_wifi_pin_opi0: vcc_wifi_pin@0 {
> +		allwinner,pins = "PA20";

This should be pins

> +		allwinner,function = "gpio_out";

This should be function

> +	};
>  };
>  
>  &r_pio {
> @@ -111,6 +148,11 @@
>  		pins = "PL10";
>  		function = "gpio_out";
>  	};
> +
> +	wifi_pwrseq_pin_opi0: wifi_pwrseq_pin@0 {
> +		allwinner,pins = "PL7";
> +		allwinner,function = "gpio_out";

And same thing here.

Thanks!
Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 801 bytes --]

Re: [PATCH 3/3] ARM: dts: sunxi: enable SDIO Wi-Fi on Orange Pi Zero

[Chen-Yu Tsai]

On Wed, Nov 23, 2016 at 3:59 PM, Maxime Ripard
<maxime.ripard@free-electrons.com> wrote:
> Hi,
>
> On Tue, Nov 22, 2016 at 12:24:21AM +0800, Icenowy Zheng wrote:
>> There's a Allwinner's XR819 SDIO Wi-Fi module soldered on the board of
>> Orange Pi Zero, which used a dedicated regulator to power.
>>
>> Add the device tree node of the regulator, the enable gpio (with
>> mmc-pwrseq) and the sdio controller.
>>
>> There's a out-of-tree driver tested to work with this device tree.
>>
>> Signed-off-by: Icenowy Zheng <icenowy@aosc.xyz>
>> ---
>> New patch in the patchset, since a out-of-tree working xradio driver is done.
>>
>> If there is any problem in this patch, it can be omitted.
>
> No particular problem with this one, however it can and should be
> merged with the previous one.
>
> Minor comments below though.
>
>>
>>  arch/arm/boot/dts/sun8i-h2plus-orangepi-zero.dts | 42 ++++++++++++++++++++++++
>>  1 file changed, 42 insertions(+)
>>
>> diff --git a/arch/arm/boot/dts/sun8i-h2plus-orangepi-zero.dts b/arch/arm/boot/dts/sun8i-h2plus-orangepi-zero.dts
>> index b428e47..39cac26 100644
>> --- a/arch/arm/boot/dts/sun8i-h2plus-orangepi-zero.dts
>> +++ b/arch/arm/boot/dts/sun8i-h2plus-orangepi-zero.dts
>> @@ -79,6 +79,24 @@
>>                       gpios = <&pio 0 17 GPIO_ACTIVE_HIGH>;
>>               };
>>       };
>> +
>> +     reg_vcc_wifi: reg_vcc_wifi {
>> +             compatible = "regulator-fixed";
>> +             pinctrl-names = "default";
>> +             pinctrl-0 = <&vcc_wifi_pin_opi0>;
>> +             regulator-min-microvolt = <3300000>;
>> +             regulator-max-microvolt = <3300000>;
>> +             regulator-name = "vcc-wifi";
>> +             enable-active-high;
>> +             gpio = <&pio 0 20 GPIO_ACTIVE_HIGH>;
>> +     };
>> +
>> +     wifi_pwrseq: wifi_pwrseq {
>> +             compatible = "mmc-pwrseq-simple";
>> +             pinctrl-names = "default";
>> +             pinctrl-0 = <&wifi_pwrseq_pin_opi0>;
>> +             reset-gpios = <&r_pio 0 7 GPIO_ACTIVE_LOW>;
>> +     };
>>  };
>>
>>  &ehci1 {
>> @@ -95,6 +113,20 @@
>>       status = "okay";
>>  };
>>
>> +&mmc1 {
>> +     pinctrl-names = "default";
>> +     pinctrl-0 = <&mmc1_pins_a>;
>> +     vmmc-supply = <&reg_vcc_wifi>;
>> +     mmc-pwrseq = <&wifi_pwrseq>;
>> +     bus-width = <4>;
>> +     non-removable;
>> +     status = "okay";
>> +};
>> +
>> +&mmc1_pins_a {
>> +     allwinner,pull = <SUN4I_PINCTRL_PULL_UP>;
>
> This should be bias-pull-up.

IIRC I already added this for _all_ existing mmc pinmux settings?

>
>> +};
>> +
>>  &ohci1 {
>>       status = "okay";
>>  };
>> @@ -104,6 +136,11 @@
>>               pins = "PA17";
>>               function = "gpio_out";
>>       };
>> +
>> +     vcc_wifi_pin_opi0: vcc_wifi_pin@0 {
>> +             allwinner,pins = "PA20";
>
> This should be pins
>
>> +             allwinner,function = "gpio_out";
>
> This should be function
>
>> +     };
>>  };
>>
>>  &r_pio {
>> @@ -111,6 +148,11 @@
>>               pins = "PL10";
>>               function = "gpio_out";
>>       };
>> +
>> +     wifi_pwrseq_pin_opi0: wifi_pwrseq_pin@0 {
>> +             allwinner,pins = "PL7";
>> +             allwinner,function = "gpio_out";
>
> And same thing here.

Might we do away with the pinmux for gpio pins tradition?
Recent patches I've sent all omit them.

ChenYu

Re: [PATCH 3/3] ARM: dts: sunxi: enable SDIO Wi-Fi on Orange Pi Zero

[Hans de Goede]

Hi,

On 23-11-16 15:25, Chen-Yu Tsai wrote:
> On Wed, Nov 23, 2016 at 3:59 PM, Maxime Ripard
> <maxime.ripard@free-electrons.com> wrote:
>> Hi,
>>
>> On Tue, Nov 22, 2016 at 12:24:21AM +0800, Icenowy Zheng wrote:
>>> There's a Allwinner's XR819 SDIO Wi-Fi module soldered on the board of
>>> Orange Pi Zero, which used a dedicated regulator to power.
>>>
>>> Add the device tree node of the regulator, the enable gpio (with
>>> mmc-pwrseq) and the sdio controller.
>>>
>>> There's a out-of-tree driver tested to work with this device tree.
>>>
>>> Signed-off-by: Icenowy Zheng <icenowy@aosc.xyz>
>>> ---
>>> New patch in the patchset, since a out-of-tree working xradio driver is done.
>>>
>>> If there is any problem in this patch, it can be omitted.
>>
>> No particular problem with this one, however it can and should be
>> merged with the previous one.
>>
>> Minor comments below though.
>>
>>>
>>>  arch/arm/boot/dts/sun8i-h2plus-orangepi-zero.dts | 42 ++++++++++++++++++++++++
>>>  1 file changed, 42 insertions(+)
>>>
>>> diff --git a/arch/arm/boot/dts/sun8i-h2plus-orangepi-zero.dts b/arch/arm/boot/dts/sun8i-h2plus-orangepi-zero.dts
>>> index b428e47..39cac26 100644
>>> --- a/arch/arm/boot/dts/sun8i-h2plus-orangepi-zero.dts
>>> +++ b/arch/arm/boot/dts/sun8i-h2plus-orangepi-zero.dts
>>> @@ -79,6 +79,24 @@
>>>                       gpios = <&pio 0 17 GPIO_ACTIVE_HIGH>;
>>>               };
>>>       };
>>> +
>>> +     reg_vcc_wifi: reg_vcc_wifi {
>>> +             compatible = "regulator-fixed";
>>> +             pinctrl-names = "default";
>>> +             pinctrl-0 = <&vcc_wifi_pin_opi0>;
>>> +             regulator-min-microvolt = <3300000>;
>>> +             regulator-max-microvolt = <3300000>;
>>> +             regulator-name = "vcc-wifi";
>>> +             enable-active-high;
>>> +             gpio = <&pio 0 20 GPIO_ACTIVE_HIGH>;
>>> +     };
>>> +
>>> +     wifi_pwrseq: wifi_pwrseq {
>>> +             compatible = "mmc-pwrseq-simple";
>>> +             pinctrl-names = "default";
>>> +             pinctrl-0 = <&wifi_pwrseq_pin_opi0>;
>>> +             reset-gpios = <&r_pio 0 7 GPIO_ACTIVE_LOW>;
>>> +     };
>>>  };
>>>
>>>  &ehci1 {
>>> @@ -95,6 +113,20 @@
>>>       status = "okay";
>>>  };
>>>
>>> +&mmc1 {
>>> +     pinctrl-names = "default";
>>> +     pinctrl-0 = <&mmc1_pins_a>;
>>> +     vmmc-supply = <&reg_vcc_wifi>;
>>> +     mmc-pwrseq = <&wifi_pwrseq>;
>>> +     bus-width = <4>;
>>> +     non-removable;
>>> +     status = "okay";
>>> +};
>>> +
>>> +&mmc1_pins_a {
>>> +     allwinner,pull = <SUN4I_PINCTRL_PULL_UP>;
>>
>> This should be bias-pull-up.
>
> IIRC I already added this for _all_ existing mmc pinmux settings?
>
>>
>>> +};
>>> +
>>>  &ohci1 {
>>>       status = "okay";
>>>  };
>>> @@ -104,6 +136,11 @@
>>>               pins = "PA17";
>>>               function = "gpio_out";
>>>       };
>>> +
>>> +     vcc_wifi_pin_opi0: vcc_wifi_pin@0 {
>>> +             allwinner,pins = "PA20";
>>
>> This should be pins
>>
>>> +             allwinner,function = "gpio_out";
>>
>> This should be function
>>
>>> +     };
>>>  };
>>>
>>>  &r_pio {
>>> @@ -111,6 +148,11 @@
>>>               pins = "PL10";
>>>               function = "gpio_out";
>>>       };
>>> +
>>> +     wifi_pwrseq_pin_opi0: wifi_pwrseq_pin@0 {
>>> +             allwinner,pins = "PL7";
>>> +             allwinner,function = "gpio_out";
>>
>> And same thing here.
>
> Might we do away with the pinmux for gpio pins tradition?
> Recent patches I've sent all omit them.

I'm in favor of doing away with them, except there were
we need to configure bias / strength.

Regards,

Hans

Re: [PATCH 3/3] ARM: dts: sunxi: enable SDIO Wi-Fi on Orange Pi Zero

[Maxime Ripard]

[-- Attachment #1: Type: text/plain, Size: 645 bytes --]

On Wed, Nov 23, 2016 at 10:25:57PM +0800, Chen-Yu Tsai wrote:
> >>  &r_pio {
> >> @@ -111,6 +148,11 @@
> >>               pins = "PL10";
> >>               function = "gpio_out";
> >>       };
> >> +
> >> +     wifi_pwrseq_pin_opi0: wifi_pwrseq_pin@0 {
> >> +             allwinner,pins = "PL7";
> >> +             allwinner,function = "gpio_out";
> >
> > And same thing here.
> 
> Might we do away with the pinmux for gpio pins tradition?
> Recent patches I've sent all omit them.

Oh, yes, that's true.

Thanks,
Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 801 bytes --]

Re: [PATCH v2 2/3] ARM: dts: sunxi: add support for Orange Pi Zero board

[Maxime Ripard]

[-- Attachment #1: Type: text/plain, Size: 2277 bytes --]

On Mon, Dec 05, 2016 at 04:59:44PM +0800, Icenowy Zheng wrote:
> 
> 2016年12月5日 16:52于 Maxime Ripard <maxime.ripard@free-electrons.com>写道：
> >
> > On Fri, Dec 02, 2016 at 10:22:30PM +0800, Icenowy Zheng wrote: 
> > > 
> > > 
> > > 01.12.2016, 17:36, "Maxime Ripard" <maxime.ripard@free-electrons.com>: 
> > > > On Mon, Nov 28, 2016 at 12:29:07AM +0000, André Przywara wrote: 
> > > >>  > Something more interesting happened. 
> > > >>  > 
> > > >>  > Xunlong made a add-on board for Orange Pi Zero, which exposes the 
> > > >>  > two USB Controllers exported at expansion bus as USB Type-A 
> > > >>  > connectors. 
> > > >>  > 
> > > >>  > Also it exposes a analog A/V jack and a microphone. 
> > > >>  > 
> > > >>  > Should I enable {e,o}hci{2.3} in the device tree? 
> > > >> 
> > > >>  Actually we should do this regardless of this extension board. The USB 
> > > >>  pins are not multiplexed and are exposed on user accessible pins (just 
> > > >>  not soldered, but that's a detail), so I think they qualify for DT 
> > > >>  enablement. And even if a user can't use them, it doesn't hurt to have 
> > > >>  them (since they are not multiplexed). 
> > > > 
> > > > My main concern about this is that we'll leave regulators enabled by 
> > > > default, for a minority of users. And that minority will prevent to do 
> > > > a proper power management when the times come since we'll have to keep 
> > > > that behaviour forever. 
> > > 
> > > I think these users can add a 'fdt set /xxx/xxx status "disabled" ' . 
> >
> > You can't ask that from the majority of users. These users will take 
> > debian or fedora, install it, and expect everything to work 
> > properly. I would make the opposite argument actually. If someone is 
> > knowledgeable enough to solder the USB pins a connector, then (s)he'll 
> > be able to make that u-boot call. 
> 
> Now (s)he do not need soldering.
> 
> (S)he needs only paying $1.99 more to Xunlong to get the expansion
> board, and insert it on the OPi Zero.

Which is going to require an overlay anyway, so we could have the USB
bits in there too.

Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 801 bytes --]

Re: [PATCH v2 2/3] ARM: dts: sunxi: add support for Orange Pi Zero board

[Maxime Ripard]

[-- Attachment #1: Type: text/plain, Size: 2744 bytes --]

On Mon, Dec 05, 2016 at 07:01:46PM +0800, Icenowy Zheng wrote:
> 
> 
> 05.12.2016, 17:40, "Maxime Ripard" <maxime.ripard@free-electrons.com>:
> > On Mon, Dec 05, 2016 at 04:59:44PM +0800, Icenowy Zheng wrote:
> >>  2016年12月5日 16:52于 Maxime Ripard <maxime.ripard@free-electrons.com>写道：
> >>  >
> >>  > On Fri, Dec 02, 2016 at 10:22:30PM +0800, Icenowy Zheng wrote:
> >>  > >
> >>  > >
> >>  > > 01.12.2016, 17:36, "Maxime Ripard" <maxime.ripard@free-electrons.com>:
> >>  > > > On Mon, Nov 28, 2016 at 12:29:07AM +0000, André Przywara wrote:
> >>  > > >>  > Something more interesting happened.
> >>  > > >>  >
> >>  > > >>  > Xunlong made a add-on board for Orange Pi Zero, which exposes the
> >>  > > >>  > two USB Controllers exported at expansion bus as USB Type-A
> >>  > > >>  > connectors.
> >>  > > >>  >
> >>  > > >>  > Also it exposes a analog A/V jack and a microphone.
> >>  > > >>  >
> >>  > > >>  > Should I enable {e,o}hci{2.3} in the device tree?
> >>  > > >>
> >>  > > >>  Actually we should do this regardless of this extension board. The USB
> >>  > > >>  pins are not multiplexed and are exposed on user accessible pins (just
> >>  > > >>  not soldered, but that's a detail), so I think they qualify for DT
> >>  > > >>  enablement. And even if a user can't use them, it doesn't hurt to have
> >>  > > >>  them (since they are not multiplexed).
> >>  > > >
> >>  > > > My main concern about this is that we'll leave regulators enabled by
> >>  > > > default, for a minority of users. And that minority will prevent to do
> >>  > > > a proper power management when the times come since we'll have to keep
> >>  > > > that behaviour forever.
> >>  > >
> >>  > > I think these users can add a 'fdt set /xxx/xxx status "disabled" ' .
> >>  >
> >>  > You can't ask that from the majority of users. These users will take
> >>  > debian or fedora, install it, and expect everything to work
> >>  > properly. I would make the opposite argument actually. If someone is
> >>  > knowledgeable enough to solder the USB pins a connector, then (s)he'll
> >>  > be able to make that u-boot call.
> >>
> >>  Now (s)he do not need soldering.
> >>
> >>  (S)he needs only paying $1.99 more to Xunlong to get the expansion
> >>  board, and insert it on the OPi Zero.
> >
> > Which is going to require an overlay anyway, so we could have the USB
> > bits in there too.
> 
> If so, I think the [PATCH -next v3 2/2] is ready to be merged ;-)

I meant enabling the USB in the overlay, you enabled it in the base DT.

Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 801 bytes --]