Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1582350 > unrolled thread

Re: [PATCH v6 5/5] ARM: dts: sun9i: Initial support for the Sunchip CX-A99 board

Started byRask Ingemann Lambertsen <rask@formelder.dk>
First post2017-02-16 07:20 +0100
Last post2017-02-22 00:30 +0100
Articles 5 — 3 participants

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH v6 5/5] ARM: dts: sun9i: Initial support for the Sunchip  CX-A99 board Rask Ingemann Lambertsen <rask@formelder.dk> - 2017-02-16 07:20 +0100
    Re: [PATCH v6 5/5] ARM: dts: sun9i: Initial support for the Sunchip  CX-A99 board Chen-Yu Tsai <wens@csie.org> - 2017-02-16 07:40 +0100
    Re: [PATCH v6 5/5] ARM: dts: sun9i: Initial support for the Sunchip  CX-A99 board Maxime Ripard <maxime.ripard@free-electrons.com> - 2017-02-17 09:50 +0100
      Re: [PATCH v6 5/5] ARM: dts: sun9i: Initial support for the Sunchip  CX-A99 board Rask Ingemann Lambertsen <rask@formelder.dk> - 2017-02-19 21:20 +0100
        Re: [PATCH v6 5/5] ARM: dts: sun9i: Initial support for the Sunchip  CX-A99 board Maxime Ripard <maxime.ripard@free-electrons.com> - 2017-02-22 00:30 +0100

#1582350 — Re: [PATCH v6 5/5] ARM: dts: sun9i: Initial support for the Sunchip CX-A99 board

FromRask Ingemann Lambertsen <rask@formelder.dk>
Date2017-02-16 07:20 +0100
SubjectRe: [PATCH v6 5/5] ARM: dts: sun9i: Initial support for the Sunchip CX-A99 board
Message-ID<tbnA6-2bA-5@gated-at.bofh.it>
On Fri, Feb 10, 2017 at 09:59:20AM +0100, Maxime Ripard wrote:
> Hi,
> 
> On Thu, Feb 09, 2017 at 12:34:06AM +0100, Rask Ingemann Lambertsen wrote:
> > The Suncip CX-A99 board is found in at least four brands of media players.
> > It features an Allwinner A80 ARM SoC and is found in two models:
> > 
> > 1) 2 GiB DDR3 DRAM and 16 GB eMMC
> > 2) 4 GiB DDR3 DRAM and 32 GB eMMC
> > 
> > For details of the board, see the linux-sunxi page
> > <URL:https://linux-sunxi.org/Sunchip_CX-A99>.
> 
> Please don't put URLs in commit logs (and the DT).

OK.

> > Supported features (+ means tested):
> > + One Cortex-A7 CPU core (or four with experimental U-Boot PSCI patches)
> > + AXP808 power management chip
> > + OZ80120 voltage regulator
> > + Serial console port (internal)
> > + eMMC and SD card slot
> > + USB 2.0 host ports on on-board USB hub
> > + SATA port on on-board SATA-to-USB bridge *
> > + IEEE 802.11 a/b/g/n/ac SDIO Wifi
> > + Real-time clock
> > + LEDs
> > - IR receiver for remote control
> > 
> > * Only shows up when a SATA device is connected. Also, if a power source
> >   is connected to the USB 3.0 connector across power cycles (e.g. FEL
> >   boot), the bridge may not properly reset and not show up on the USB bus.
> >   The vendor U-Boot performs some unknown magic which resets the bridge.
> 
> Is that magic at the USB level, or specific to the bridge itself?

I don't know, but since the other USB port connected to the hub comes up
fine, I'm assuming it's something to do with the SATA-to-USB bridge.

> > 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..f5496d2
> > --- /dev/null
> > +++ b/arch/arm/boot/dts/sun9i-a80-cx-a99.dts
[...]
> > +	/* USB 3.0 standard-A receptacle. For now, only Vbus is supported. */
> 
> I'm not sure what you mean by "only VBUS is supported"? Is there any
> other signal?

I'm just trying to say that at the moment, all the connector will do is to
supply power to a connected device (for charging batteries or whatever).

> > +	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;
> 
> This is redundant with the GPIO flag

No. I have now tested without "enable-active-high" and then Vbus stays off.
This is also in line with the binding documentation:

"- enable-active-high: Polarity of GPIO is Active high
 If this property is missing, the default assumed is Active low."

It also seems to me that the way this is implemented in Linux, the GPIO
polarity flag is ignored.

> > +		regulator-always-on;
> 
> And it shouldn't be always on. The USB driver will enable it if needs
> be.

Yes, we just don't have a driver for the A80 USB 3.0 port yet.

> > +	/*
> > +	 * A GL850G hub with two external USB connectors is connected
> > +	 * to ehci0. Each has a Vbus regulator controlled by a GPIO:
> > +	 * PL7 for port 1, closest to the 12 V power connector, and
> > +	 * PL8 for port 2, next to the SD card slot.
> > +	 * Because regulator-fixed doesn't support a GPIO list, and
> > +	 * allwinner,sun9i-a80-usb-phy doesn't support more than one
> > +	 * supply, we have to use regulator-always-on on usb1-2-vbus.
> > +	 * Note that the GPIO pins also need cldo1 to be enabled.
> > +	 */
> 
> What is the source of those regulators connected then? Some PMIC
> regulator? AC-IN?

These two regulators are supplied from a fixed 12 V to 5 V DC-DC converter
which can't be controlled in any way.

> > +	reg_usb1_1_vbus: regulator-usb1-1-vbus {
> > +		compatible = "regulator-fixed";
> > +		regulator-name = "usb1-1-vbus";
> > +		regulator-min-microvolt = <5000000>;
> > +		regulator-max-microvolt = <5000000>;
> > +		gpio = <&r_pio 0 7 GPIO_ACTIVE_HIGH>;	/* PL7 */
> > +		enable-active-high;
> > +	};
> > +
> > +	reg_usb1_2_vbus: regulator-usb1-2-vbus {
> > +		compatible = "regulator-fixed";
> > +		regulator-name = "usb1-2-vbus";
> > +		regulator-min-microvolt = <5000000>;
> > +		regulator-max-microvolt = <5000000>;
> > +		gpio = <&r_pio 0 8 GPIO_ACTIVE_HIGH>;	/* PL8 */
> > +		enable-active-high;
> > +		regulator-always-on;
> 
> Same comment about always on. If the driver needs fixing to grab an
> additional regulator, fix it, but this shouldn't be left that way.

I agree, but a lot of work is missing to get to that point. To get the USB
ports up and running, you need to enable quite a few regulators:
1. VDD09-USB for the controller internals.
2. VCC33-USB for the PHY.
3. Vbus for each connector belonging to the controller.
4. Pin group supply for the GPIO pins controlling each of the Vbus supplies.

Nobody seems to have thought about the first and the last two supplies when
the bindings were written. In this case, 1, 2 and 4 will be connected to
always-on regulators anyway. Getting support for multiple Vbus supplies
probably needs something along the lines of what was discussed with your
coupled regulator patches:

https://lists.infradead.org/pipermail/linux-arm-kernel/2016-January/398862.html
https://lists.infradead.org/pipermail/linux-arm-kernel/2016-February/405401.html
https://lists.infradead.org/pipermail/linux-arm-kernel/2016-November/466207.html

> > +	/* OZ80120 voltage regulator for the four Cortex-A15 cores. */
> > +	reg_vdd_cpub: regulator-vdd-cpub {
> > +		compatible = "regulator-gpio";
> > +
> > +		regulator-always-on;
> > +		regulator-min-microvolt = < 800000>;
> > +		regulator-max-microvolt = <1100000>;
> > +		regulator-name = "vdd-cpub";
> > +
> > +		enable-gpio = <&r_pio 0 2 GPIO_ACTIVE_HIGH>;	/* PL2 */
> > +		enable-active-high;
> > +		gpios = <&r_pio 0 3 GPIO_ACTIVE_HIGH>,		/* PL3 */
> > +			<&r_pio 0 4 GPIO_ACTIVE_HIGH>,		/* PL4 */
> > +			<&r_pio 0 5 GPIO_ACTIVE_HIGH>;		/* PL5 */
> > +
> > +		gpios-states = <1 0 0>;
> > +		states = <	 750000 0x7
> > +				 800000 0x3
> > +				 850000 0x5
> > +				 900000 0x1
> > +				 950000 0x6
> > +				1000000 0x2
> > +				1100000 0x4
> > +				1200000 0x0>;
> 
> You're listing a minimum state of 750mv, yet your minimum voltage is
> 800mV.

Yes. The regulator is capable of outputting the listed voltages in the
750 mV to 1200 mV range, but the permissible range of the consumer is only
800 mV to 1100 mV according to the A80 data sheet. Likewise, the AXP806
regulators support a larger voltage range than that of the connected
consumers.

> > +/* Ampak AP6335 IEEE 802.11 a/b/g/n/ac "Wifi". */
> 
> Why "wifi" ? It's not implementing true wifi?

I put quotes around it because the proper spelling is Wi-Fi, but since that
is a trademark, most people use the term Wifi capitalised in one way or
another (or sometimes not). Grepping the other dts files, it seems customary
to leave out the quotes, so I'll do the same.

> > +&mmc1 {
> > +	bus-width = <4>;
> > +	non-removable;
> > +	pinctrl-names = "default";
> > +	pinctrl-0 = <&mmc1_pins>;
> > +	vmmc-supply = <&reg_cldo3>;	/* See cldo2,cldo3 note. */
> > +	vqmmc-supply = <&reg_aldo2>;
> 
> So it's able to support 1.2 or 1.8V IO modes? Surely you want to
> enable those modes here to.

Thanks for the hint. I thought the driver would automatically detect that.
The following modes are supported according to the data sheet:

* DS: Default speed up to 25MHz (3.3V signaling).
* HS: High speed up to 50MHz (3.3V signaling).
* SDR12: SDR up to 25MHz (1.8V signaling).
* SDR25: SDR up to 50MHz (1.8V signaling).
* SDR50: SDR up to 100MHz (1.8V signaling).
* SDR104: SDR up to 208MHz (1.8V signaling).
* DDR50: DDR up to 50MHz (1.8V signaling).

Unfortunately, when I enable the 1.8 V modes, or even just SDR-12, the
device fails to initialise with error -110 (-ETIMEDOUT). I will take a look
at that, but no promises.

> > +	pmic@745 {
> > +		compatible = "x-powers,axp808", "x-powers,axp806";
> > +		reg = <0x745>;
> > +		interrupt-parent = <&nmi_intc>;
> > +		interrupts = <0 IRQ_TYPE_LEVEL_LOW>;
> > +		interrupt-controller;
> > +		#interrupt-cells = <1>;
> > +
> > +		swin-supply = <&reg_dcdce>;
> 
> Please use an incude for that PMIC.

I don't understand. What would that include file contain? Very few lines
would be shared between .dts files. You could save < 8 lines total if you
#include the 5 lines that are common between the Cubieboard4, the Optimus
and this board.

> > +		/* In comments: Initial setup from vendor sys_config.fex. */
> > +		regulators {
> > +			/* 3.0 V (enabled). */
> 
> This might be disabled though.

Indeed it is currently disabled by the kernel during startup, as there are
no consumers listed in the device tree. I don't know what that regulator
supplies, and it is on my to-do list for probing the next time that I have
the heat sink off the board.

> > +			/* 1.8 V (enabled). */
> > +			reg_aldo2: aldo2 {
> > +				regulator-boot-on;
> > +				regulator-min-microvolt = <1800000>;
> > +				regulator-max-microvolt = <3600000>;
> > +				regulator-name = "vcc-pg-pm-wifi+btio-audio";
> 
> Usually, there is simpler names available on the schematics, or at
> least simpler names we can come up with.

I don't have schematics as it's not a developer board. V1.1 of the PCB,
which I have, doesn't even have the component names silkscreened onto it.

-- 
Rask Ingemann Lambertsen

[toc] | [next] | [standalone]

#1582354

FromChen-Yu Tsai <wens@csie.org>
Date2017-02-16 07:40 +0100
Message-ID<tbnTr-2k8-1@gated-at.bofh.it>
In reply to#1582350
On Thu, Feb 16, 2017 at 2:17 PM, Rask Ingemann Lambertsen
<rask@formelder.dk> wrote:
> On Fri, Feb 10, 2017 at 09:59:20AM +0100, Maxime Ripard wrote:
>> Hi,
>>
>> On Thu, Feb 09, 2017 at 12:34:06AM +0100, Rask Ingemann Lambertsen wrote:
>> > The Suncip CX-A99 board is found in at least four brands of media players.
>> > It features an Allwinner A80 ARM SoC and is found in two models:
>> >
>> > 1) 2 GiB DDR3 DRAM and 16 GB eMMC
>> > 2) 4 GiB DDR3 DRAM and 32 GB eMMC
>> >
>> > For details of the board, see the linux-sunxi page
>> > <URL:https://linux-sunxi.org/Sunchip_CX-A99>.
>>
>> Please don't put URLs in commit logs (and the DT).
>
> OK.
>
>> > Supported features (+ means tested):
>> > + One Cortex-A7 CPU core (or four with experimental U-Boot PSCI patches)
>> > + AXP808 power management chip
>> > + OZ80120 voltage regulator
>> > + Serial console port (internal)
>> > + eMMC and SD card slot
>> > + USB 2.0 host ports on on-board USB hub
>> > + SATA port on on-board SATA-to-USB bridge *
>> > + IEEE 802.11 a/b/g/n/ac SDIO Wifi
>> > + Real-time clock
>> > + LEDs
>> > - IR receiver for remote control
>> >
>> > * Only shows up when a SATA device is connected. Also, if a power source
>> >   is connected to the USB 3.0 connector across power cycles (e.g. FEL
>> >   boot), the bridge may not properly reset and not show up on the USB bus.
>> >   The vendor U-Boot performs some unknown magic which resets the bridge.
>>
>> Is that magic at the USB level, or specific to the bridge itself?
>
> I don't know, but since the other USB port connected to the hub comes up
> fine, I'm assuming it's something to do with the SATA-to-USB bridge.
>
>> > 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..f5496d2
>> > --- /dev/null
>> > +++ b/arch/arm/boot/dts/sun9i-a80-cx-a99.dts
> [...]
>> > +   /* USB 3.0 standard-A receptacle. For now, only Vbus is supported. */
>>
>> I'm not sure what you mean by "only VBUS is supported"? Is there any
>> other signal?
>
> I'm just trying to say that at the moment, all the connector will do is to
> supply power to a connected device (for charging batteries or whatever).
>
>> > +   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;
>>
>> This is redundant with the GPIO flag
>
> No. I have now tested without "enable-active-high" and then Vbus stays off.
> This is also in line with the binding documentation:
>
> "- enable-active-high: Polarity of GPIO is Active high
>  If this property is missing, the default assumed is Active low."
>
> It also seems to me that the way this is implemented in Linux, the GPIO
> polarity flag is ignored.
>
>> > +           regulator-always-on;
>>
>> And it shouldn't be always on. The USB driver will enable it if needs
>> be.
>
> Yes, we just don't have a driver for the A80 USB 3.0 port yet.
>
>> > +   /*
>> > +    * A GL850G hub with two external USB connectors is connected
>> > +    * to ehci0. Each has a Vbus regulator controlled by a GPIO:
>> > +    * PL7 for port 1, closest to the 12 V power connector, and
>> > +    * PL8 for port 2, next to the SD card slot.
>> > +    * Because regulator-fixed doesn't support a GPIO list, and
>> > +    * allwinner,sun9i-a80-usb-phy doesn't support more than one
>> > +    * supply, we have to use regulator-always-on on usb1-2-vbus.
>> > +    * Note that the GPIO pins also need cldo1 to be enabled.
>> > +    */
>>
>> What is the source of those regulators connected then? Some PMIC
>> regulator? AC-IN?
>
> These two regulators are supplied from a fixed 12 V to 5 V DC-DC converter
> which can't be controlled in any way.
>
>> > +   reg_usb1_1_vbus: regulator-usb1-1-vbus {
>> > +           compatible = "regulator-fixed";
>> > +           regulator-name = "usb1-1-vbus";
>> > +           regulator-min-microvolt = <5000000>;
>> > +           regulator-max-microvolt = <5000000>;
>> > +           gpio = <&r_pio 0 7 GPIO_ACTIVE_HIGH>;   /* PL7 */
>> > +           enable-active-high;
>> > +   };
>> > +
>> > +   reg_usb1_2_vbus: regulator-usb1-2-vbus {
>> > +           compatible = "regulator-fixed";
>> > +           regulator-name = "usb1-2-vbus";
>> > +           regulator-min-microvolt = <5000000>;
>> > +           regulator-max-microvolt = <5000000>;
>> > +           gpio = <&r_pio 0 8 GPIO_ACTIVE_HIGH>;   /* PL8 */
>> > +           enable-active-high;
>> > +           regulator-always-on;
>>
>> Same comment about always on. If the driver needs fixing to grab an
>> additional regulator, fix it, but this shouldn't be left that way.
>
> I agree, but a lot of work is missing to get to that point. To get the USB
> ports up and running, you need to enable quite a few regulators:
> 1. VDD09-USB for the controller internals.
> 2. VCC33-USB for the PHY.
> 3. Vbus for each connector belonging to the controller.
> 4. Pin group supply for the GPIO pins controlling each of the Vbus supplies.
>
> Nobody seems to have thought about the first and the last two supplies when
> the bindings were written. In this case, 1, 2 and 4 will be connected to
> always-on regulators anyway. Getting support for multiple Vbus supplies
> probably needs something along the lines of what was discussed with your
> coupled regulator patches:
>
> https://lists.infradead.org/pipermail/linux-arm-kernel/2016-January/398862.html
> https://lists.infradead.org/pipermail/linux-arm-kernel/2016-February/405401.html
> https://lists.infradead.org/pipermail/linux-arm-kernel/2016-November/466207.html
>
>> > +   /* OZ80120 voltage regulator for the four Cortex-A15 cores. */
>> > +   reg_vdd_cpub: regulator-vdd-cpub {
>> > +           compatible = "regulator-gpio";
>> > +
>> > +           regulator-always-on;
>> > +           regulator-min-microvolt = < 800000>;
>> > +           regulator-max-microvolt = <1100000>;
>> > +           regulator-name = "vdd-cpub";
>> > +
>> > +           enable-gpio = <&r_pio 0 2 GPIO_ACTIVE_HIGH>;    /* PL2 */
>> > +           enable-active-high;
>> > +           gpios = <&r_pio 0 3 GPIO_ACTIVE_HIGH>,          /* PL3 */
>> > +                   <&r_pio 0 4 GPIO_ACTIVE_HIGH>,          /* PL4 */
>> > +                   <&r_pio 0 5 GPIO_ACTIVE_HIGH>;          /* PL5 */
>> > +
>> > +           gpios-states = <1 0 0>;
>> > +           states = <       750000 0x7
>> > +                            800000 0x3
>> > +                            850000 0x5
>> > +                            900000 0x1
>> > +                            950000 0x6
>> > +                           1000000 0x2
>> > +                           1100000 0x4
>> > +                           1200000 0x0>;
>>
>> You're listing a minimum state of 750mv, yet your minimum voltage is
>> 800mV.
>
> Yes. The regulator is capable of outputting the listed voltages in the
> 750 mV to 1200 mV range, but the permissible range of the consumer is only
> 800 mV to 1100 mV according to the A80 data sheet. Likewise, the AXP806
> regulators support a larger voltage range than that of the connected
> consumers.
>
>> > +/* Ampak AP6335 IEEE 802.11 a/b/g/n/ac "Wifi". */
>>
>> Why "wifi" ? It's not implementing true wifi?
>
> I put quotes around it because the proper spelling is Wi-Fi, but since that
> is a trademark, most people use the term Wifi capitalised in one way or
> another (or sometimes not). Grepping the other dts files, it seems customary
> to leave out the quotes, so I'll do the same.
>
>> > +&mmc1 {
>> > +   bus-width = <4>;
>> > +   non-removable;
>> > +   pinctrl-names = "default";
>> > +   pinctrl-0 = <&mmc1_pins>;
>> > +   vmmc-supply = <&reg_cldo3>;     /* See cldo2,cldo3 note. */
>> > +   vqmmc-supply = <&reg_aldo2>;
>>
>> So it's able to support 1.2 or 1.8V IO modes? Surely you want to
>> enable those modes here to.
>
> Thanks for the hint. I thought the driver would automatically detect that.
> The following modes are supported according to the data sheet:
>
> * DS: Default speed up to 25MHz (3.3V signaling).
> * HS: High speed up to 50MHz (3.3V signaling).
> * SDR12: SDR up to 25MHz (1.8V signaling).
> * SDR25: SDR up to 50MHz (1.8V signaling).
> * SDR50: SDR up to 100MHz (1.8V signaling).
> * SDR104: SDR up to 208MHz (1.8V signaling).
> * DDR50: DDR up to 50MHz (1.8V signaling).
>
> Unfortunately, when I enable the 1.8 V modes, or even just SDR-12, the
> device fails to initialise with error -110 (-ETIMEDOUT). I will take a look
> at that, but no promises.

The kernel doesn't support lower I/O voltages for the A80 at the moment.
The pin controller has voltage level controls that need to match the
external regulators supplying it power.

Rockchip has a similar thing called "Rockchip IO Domain". Look for the
ROCKCHIP_IODOMAIN Kconfig symbol. My plan is to do something similar,
as a subdevice to the pin controller.

We cannot have it as part of the pin controller as that leads to a
circular dependency for the R_PIO, thus blocking device probing for
the rest of the system.

However this is somewhat low on my todo list as it works fine without.

Regards
ChenYu

>
>> > +   pmic@745 {
>> > +           compatible = "x-powers,axp808", "x-powers,axp806";
>> > +           reg = <0x745>;
>> > +           interrupt-parent = <&nmi_intc>;
>> > +           interrupts = <0 IRQ_TYPE_LEVEL_LOW>;
>> > +           interrupt-controller;
>> > +           #interrupt-cells = <1>;
>> > +
>> > +           swin-supply = <&reg_dcdce>;
>>
>> Please use an incude for that PMIC.
>
> I don't understand. What would that include file contain? Very few lines
> would be shared between .dts files. You could save < 8 lines total if you
> #include the 5 lines that are common between the Cubieboard4, the Optimus
> and this board.
>
>> > +           /* In comments: Initial setup from vendor sys_config.fex. */
>> > +           regulators {
>> > +                   /* 3.0 V (enabled). */
>>
>> This might be disabled though.
>
> Indeed it is currently disabled by the kernel during startup, as there are
> no consumers listed in the device tree. I don't know what that regulator
> supplies, and it is on my to-do list for probing the next time that I have
> the heat sink off the board.
>
>> > +                   /* 1.8 V (enabled). */
>> > +                   reg_aldo2: aldo2 {
>> > +                           regulator-boot-on;
>> > +                           regulator-min-microvolt = <1800000>;
>> > +                           regulator-max-microvolt = <3600000>;
>> > +                           regulator-name = "vcc-pg-pm-wifi+btio-audio";
>>
>> Usually, there is simpler names available on the schematics, or at
>> least simpler names we can come up with.
>
> I don't have schematics as it's not a developer board. V1.1 of the PCB,
> which I have, doesn't even have the component names silkscreened onto it.
>
> --
> Rask Ingemann Lambertsen

[toc] | [prev] | [next] | [standalone]

#1583214

FromMaxime Ripard <maxime.ripard@free-electrons.com>
Date2017-02-17 09:50 +0100
Message-ID<tbMoN-1Y8-15@gated-at.bofh.it>
In reply to#1582350

[Multipart message — attachments visible in raw view] — view raw

On Thu, Feb 16, 2017 at 07:17:54AM +0100, Rask Ingemann Lambertsen wrote:
> > > Supported features (+ means tested):
> > > + One Cortex-A7 CPU core (or four with experimental U-Boot PSCI patches)
> > > + AXP808 power management chip
> > > + OZ80120 voltage regulator
> > > + Serial console port (internal)
> > > + eMMC and SD card slot
> > > + USB 2.0 host ports on on-board USB hub
> > > + SATA port on on-board SATA-to-USB bridge *
> > > + IEEE 802.11 a/b/g/n/ac SDIO Wifi
> > > + Real-time clock
> > > + LEDs
> > > - IR receiver for remote control
> > > 
> > > * Only shows up when a SATA device is connected. Also, if a power source
> > >   is connected to the USB 3.0 connector across power cycles (e.g. FEL
> > >   boot), the bridge may not properly reset and not show up on the USB bus.
> > >   The vendor U-Boot performs some unknown magic which resets the bridge.
> > 
> > Is that magic at the USB level, or specific to the bridge itself?
> 
> I don't know, but since the other USB port connected to the hub comes up
> fine, I'm assuming it's something to do with the SATA-to-USB bridge.

But where is that magic written to then? the USB controller registers,
or is it an USB packet?

> > > 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..f5496d2
> > > --- /dev/null
> > > +++ b/arch/arm/boot/dts/sun9i-a80-cx-a99.dts
> [...]
> > > +	/* USB 3.0 standard-A receptacle. For now, only Vbus is supported. */
> > 
> > I'm not sure what you mean by "only VBUS is supported"? Is there any
> > other signal?
> 
> I'm just trying to say that at the moment, all the connector will do is to
> supply power to a connected device (for charging batteries or whatever).

So the USB controller itself is non-functionnal then?

> > > +	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;
> > 
> > This is redundant with the GPIO flag
> 
> No. I have now tested without "enable-active-high" and then Vbus stays off.
> This is also in line with the binding documentation:
> 
> "- enable-active-high: Polarity of GPIO is Active high
>  If this property is missing, the default assumed is Active low."
>
> It also seems to me that the way this is implemented in Linux, the GPIO
> polarity flag is ignored.

It works just fine if the driver uses the gpiod API. If that driver
doesn't, then that driver needs some fixing :)

> > > +		regulator-always-on;
> > 
> > And it shouldn't be always on. The USB driver will enable it if needs
> > be.
> 
> Yes, we just don't have a driver for the A80 USB 3.0 port yet.

Aaah, so that's what you meant. Leave it out of the DT then.

> > > +	 * A GL850G hub with two external USB connectors is connected
> > > +	 * to ehci0. Each has a Vbus regulator controlled by a GPIO:
> > > +	 * PL7 for port 1, closest to the 12 V power connector, and
> > > +	 * PL8 for port 2, next to the SD card slot.
> > > +	 * Because regulator-fixed doesn't support a GPIO list, and
> > > +	 * allwinner,sun9i-a80-usb-phy doesn't support more than one
> > > +	 * supply, we have to use regulator-always-on on usb1-2-vbus.
> > > +	 * Note that the GPIO pins also need cldo1 to be enabled.
> > > +	 */
> > 
> > What is the source of those regulators connected then? Some PMIC
> > regulator? AC-IN?
> 
> These two regulators are supplied from a fixed 12 V to 5 V DC-DC converter
> which can't be controlled in any way.

Ok.

> > > +	reg_usb1_1_vbus: regulator-usb1-1-vbus {
> > > +		compatible = "regulator-fixed";
> > > +		regulator-name = "usb1-1-vbus";
> > > +		regulator-min-microvolt = <5000000>;
> > > +		regulator-max-microvolt = <5000000>;
> > > +		gpio = <&r_pio 0 7 GPIO_ACTIVE_HIGH>;	/* PL7 */
> > > +		enable-active-high;
> > > +	};
> > > +
> > > +	reg_usb1_2_vbus: regulator-usb1-2-vbus {
> > > +		compatible = "regulator-fixed";
> > > +		regulator-name = "usb1-2-vbus";
> > > +		regulator-min-microvolt = <5000000>;
> > > +		regulator-max-microvolt = <5000000>;
> > > +		gpio = <&r_pio 0 8 GPIO_ACTIVE_HIGH>;	/* PL8 */
> > > +		enable-active-high;
> > > +		regulator-always-on;
> > 
> > Same comment about always on. If the driver needs fixing to grab an
> > additional regulator, fix it, but this shouldn't be left that way.
> 
> I agree, but a lot of work is missing to get to that point. To get the USB
> ports up and running, you need to enable quite a few regulators:
> 1. VDD09-USB for the controller internals.

This one has never been needed in any SoC before. You should
definitely add a regulator property to the USB controller node then,
and add a regulator_get to the driver. Ideally based on the
compatible.

> 2. VCC33-USB for the PHY.

This one will be taken by the PHY driver, through a new, optional
regulator.

> 3. Vbus for each connector belonging to the controller.

These should be taken through the usb*_vbus-supply properties

> 4. Pin group supply for the GPIO pins controlling each of the Vbus
> supplies.

And we haven't had any case of a board where those were tied to
regulators that were not in themselves essential to the system, which
is why no one really bothered to add them. If they have dedicated
regulators on your board, then it's time to add those extra
properties.

> Nobody seems to have thought about the first and the last two supplies when
> the bindings were written. In this case, 1, 2 and 4 will be connected to
> always-on regulators anyway. Getting support for multiple Vbus supplies
> probably needs something along the lines of what was discussed with your
> coupled regulator patches:
> 
> https://lists.infradead.org/pipermail/linux-arm-kernel/2016-January/398862.html
> https://lists.infradead.org/pipermail/linux-arm-kernel/2016-February/405401.html
> https://lists.infradead.org/pipermail/linux-arm-kernel/2016-November/466207.html

This is not really the point, always-on is a hack. We can use it if
needed to workaround weird cases like this one, but in trivial cases
like the one you exposed above, this really should be modelled
properly from the very start.

> > > +	/* OZ80120 voltage regulator for the four Cortex-A15 cores. */
> > > +	reg_vdd_cpub: regulator-vdd-cpub {
> > > +		compatible = "regulator-gpio";
> > > +
> > > +		regulator-always-on;
> > > +		regulator-min-microvolt = < 800000>;
> > > +		regulator-max-microvolt = <1100000>;
> > > +		regulator-name = "vdd-cpub";
> > > +
> > > +		enable-gpio = <&r_pio 0 2 GPIO_ACTIVE_HIGH>;	/* PL2 */
> > > +		enable-active-high;
> > > +		gpios = <&r_pio 0 3 GPIO_ACTIVE_HIGH>,		/* PL3 */
> > > +			<&r_pio 0 4 GPIO_ACTIVE_HIGH>,		/* PL4 */
> > > +			<&r_pio 0 5 GPIO_ACTIVE_HIGH>;		/* PL5 */
> > > +
> > > +		gpios-states = <1 0 0>;
> > > +		states = <	 750000 0x7
> > > +				 800000 0x3
> > > +				 850000 0x5
> > > +				 900000 0x1
> > > +				 950000 0x6
> > > +				1000000 0x2
> > > +				1100000 0x4
> > > +				1200000 0x0>;
> > 
> > You're listing a minimum state of 750mv, yet your minimum voltage is
> > 800mV.
> 
> Yes. The regulator is capable of outputting the listed voltages in the
> 750 mV to 1200 mV range, but the permissible range of the consumer is only
> 800 mV to 1100 mV according to the A80 data sheet. Likewise, the AXP806
> regulators support a larger voltage range than that of the connected
> consumers.

Adding a comment stating that would be nice then.

> > > +&mmc1 {
> > > +	bus-width = <4>;
> > > +	non-removable;
> > > +	pinctrl-names = "default";
> > > +	pinctrl-0 = <&mmc1_pins>;
> > > +	vmmc-supply = <&reg_cldo3>;	/* See cldo2,cldo3 note. */
> > > +	vqmmc-supply = <&reg_aldo2>;
> > 
> > So it's able to support 1.2 or 1.8V IO modes? Surely you want to
> > enable those modes here to.
> 
> Thanks for the hint. I thought the driver would automatically detect that.
> The following modes are supported according to the data sheet:
> 
> * DS: Default speed up to 25MHz (3.3V signaling).
> * HS: High speed up to 50MHz (3.3V signaling).
> * SDR12: SDR up to 25MHz (1.8V signaling).
> * SDR25: SDR up to 50MHz (1.8V signaling).
> * SDR50: SDR up to 100MHz (1.8V signaling).
> * SDR104: SDR up to 208MHz (1.8V signaling).
> * DDR50: DDR up to 50MHz (1.8V signaling).
> 
> Unfortunately, when I enable the 1.8 V modes, or even just SDR-12, the
> device fails to initialise with error -110 (-ETIMEDOUT). I will take a look
> at that, but no promises.

Ok.

> > > +	pmic@745 {
> > > +		compatible = "x-powers,axp808", "x-powers,axp806";
> > > +		reg = <0x745>;
> > > +		interrupt-parent = <&nmi_intc>;
> > > +		interrupts = <0 IRQ_TYPE_LEVEL_LOW>;
> > > +		interrupt-controller;
> > > +		#interrupt-cells = <1>;
> > > +
> > > +		swin-supply = <&reg_dcdce>;
> > 
> > Please use an incude for that PMIC.
> 
> I don't understand. What would that include file contain? Very few lines
> would be shared between .dts files. You could save < 8 lines total if you
> #include the 5 lines that are common between the Cubieboard4, the Optimus
> and this board.

You also have to take into account that the PMIC is barely supported
at the moment, you'll also need to define the GPIO controllers, the
various power supplies, adc, etc. nodes that are going (hopefully to
be added) in the future.

If the include is shared, once it is enabled, every one benefits from
it. Otherwise, you have to duplicate that change across all DTs. This
is much likely to error, harder to maintain, and you end up in a
situation where none of the boards really support the same feature set
while being based on the same SoC / PMIC combination.

> > > +		/* In comments: Initial setup from vendor sys_config.fex. */
> > > +		regulators {
> > > +			/* 3.0 V (enabled). */
> > 
> > This might be disabled though.
> 
> Indeed it is currently disabled by the kernel during startup, as there are
> no consumers listed in the device tree. I don't know what that regulator
> supplies, and it is on my to-do list for probing the next time that I have
> the heat sink off the board.

I don't think those comments are needed actually. some are misleading,
like the one above, and some are likely to change at some point, like
the one below.

> > > +			/* 1.8 V (enabled). */
> > > +			reg_aldo2: aldo2 {
> > > +				regulator-boot-on;
> > > +				regulator-min-microvolt = <1800000>;
> > > +				regulator-max-microvolt = <3600000>;
> > > +				regulator-name = "vcc-pg-pm-wifi+btio-audio";
> > 
> > Usually, there is simpler names available on the schematics, or at
> > least simpler names we can come up with.
> 
> I don't have schematics as it's not a developer board. V1.1 of the PCB,
> which I have, doesn't even have the component names silkscreened onto it.

Ok.

Thanks,
Maxime

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

[toc] | [prev] | [next] | [standalone]

#1584248

FromRask Ingemann Lambertsen <rask@formelder.dk>
Date2017-02-19 21:20 +0100
Message-ID<tcG7D-3r3-1@gated-at.bofh.it>
In reply to#1583214
On Thu, Feb 16, 2017 at 07:29:57PM +0100, Maxime Ripard wrote:
> On Thu, Feb 16, 2017 at 07:17:54AM +0100, Rask Ingemann Lambertsen wrote:
> > > > Supported features (+ means tested):
[...]
> > > > + SATA port on on-board SATA-to-USB bridge *
[...]
> > > > * Only shows up when a SATA device is connected. Also, if a power source
> > > >   is connected to the USB 3.0 connector across power cycles (e.g. FEL
> > > >   boot), the bridge may not properly reset and not show up on the USB bus.
> > > >   The vendor U-Boot performs some unknown magic which resets the bridge.
> > > 
> > > Is that magic at the USB level, or specific to the bridge itself?
> > 
> > I don't know, but since the other USB port connected to the hub comes up
> > fine, I'm assuming it's something to do with the SATA-to-USB bridge.
> 
> But where is that magic written to then? the USB controller registers,
> or is it an USB packet?

I don't know. Like I said, unknown magic. I haven't found the source code to
the U-Boot version that the board uses.

For all I know, it could be a bug in the Linux USB stack. I've always had
to unplug and replug my USB keyboards after Linux has booted, otherwise
they are not detected. If the keyboard was connected to a hub, Linux doesn't
detect that port on the hub, so the hub must be unplugged and replugged.
If it's an internal hub, tough luck.

> > > > +	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;
> > > 
> > > This is redundant with the GPIO flag

The GPIO flag is overridden by the regulator core, which makes it misleading.
I will remove the GPIO flag and add a comment there that GPIO flags are not
supported. This is to prevent someone from thinking that

gpio = <... GPIO_ACTIVE_HIGH>;
enable-active-high;

and

gpio = <... GPIO_ACTIVE_LOW>;

are equivalent. It would be an easy mistake to make, because in mmc nodes,

cd-gpios = <...  GPIO_ACTIVE_HIGH>;

and

cd-gpios = <...  GPIO_ACTIVE_LOW>;
cd-inverted;

_are_ equivalent.

> > No. I have now tested without "enable-active-high" and then Vbus stays off.
> > This is also in line with the binding documentation:
> > 
> > "- enable-active-high: Polarity of GPIO is Active high
> >  If this property is missing, the default assumed is Active low."
> >
> > It also seems to me that the way this is implemented in Linux, the GPIO
> > polarity flag is ignored.
> 
> It works just fine if the driver uses the gpiod API. If that driver
> doesn't, then that driver needs some fixing :)

Take it up with the maintainer of the regulator core, then. :)
From of_get_fixed_voltage_config() in drivers/regulator/fixed.c:
[...]
	config->gpio = of_get_named_gpio(np, "gpio", 0);
[...]
	config->enable_high = of_property_read_bool(np, "enable-active-high");
	config->gpio_is_open_drain = of_property_read_bool(np,
							   "gpio-open-drain");
[...]

From reg_fixed_voltage_probe() in drivers/regulator/fixed.c:
[...]
		config = of_get_fixed_voltage_config(&pdev->dev,
						     &drvdata->desc);
[...]
	if (gpio_is_valid(config->gpio)) {
		cfg.ena_gpio = config->gpio;
		if (pdev->dev.of_node)
			cfg.ena_gpio_initialized = true;
	}
	cfg.ena_gpio_invert = !config->enable_high;
	if (config->enabled_at_boot) {
		if (config->enable_high)
			cfg.ena_gpio_flags |= GPIOF_OUT_INIT_HIGH;
		else
			cfg.ena_gpio_flags |= GPIOF_OUT_INIT_LOW;
	} else {
		if (config->enable_high)
			cfg.ena_gpio_flags |= GPIOF_OUT_INIT_LOW;
		else
			cfg.ena_gpio_flags |= GPIOF_OUT_INIT_HIGH;
	}
	if (config->gpio_is_open_drain)
		cfg.ena_gpio_flags |= GPIOF_OPEN_DRAIN;
[...]

From regulator_ena_gpio_request() in drivers/regulator/core.c:
[...]
	gpiod = gpio_to_desc(config->ena_gpio);
[...]
	ret = gpio_request_one(config->ena_gpio,
				GPIOF_DIR_OUT | config->ena_gpio_flags,
				rdev_get_name(rdev));
[...]
	pin->gpiod = gpiod;
	pin->ena_gpio_invert = config->ena_gpio_invert;

And finally, gpio_request_one() in drivers/gpio/gpiolib-legacy.c:

int gpio_request_one(unsigned gpio, unsigned long flags, const char *label)
{
	struct gpio_desc *desc;
	int err;

	desc = gpio_to_desc(gpio);
[...]
	if (flags & GPIOF_OPEN_DRAIN)
		set_bit(FLAG_OPEN_DRAIN, &desc->flags);

	if (flags & GPIOF_OPEN_SOURCE)
		set_bit(FLAG_OPEN_SOURCE, &desc->flags);

	if (flags & GPIOF_ACTIVE_LOW)
		set_bit(FLAG_ACTIVE_LOW, &desc->flags);

	if (flags & GPIOF_DIR_IN)
		err = gpiod_direction_input(desc);
	else
		err = gpiod_direction_output_raw(desc,
				(flags & GPIOF_INIT_HIGH) ? 1 : 0);
[...]

> > > > +		regulator-always-on;
> > > 
> > > And it shouldn't be always on. The USB driver will enable it if needs
> > > be.
> > 
> > Yes, we just don't have a driver for the A80 USB 3.0 port yet.
> 
> Aaah, so that's what you meant. Leave it out of the DT then.

OK.

> > > You're listing a minimum state of 750mv, yet your minimum voltage is
> > > 800mV.
> > 
> > Yes. The regulator is capable of outputting the listed voltages in the
> > 750 mV to 1200 mV range, but the permissible range of the consumer is only
> > 800 mV to 1100 mV according to the A80 data sheet. Likewise, the AXP806
> > regulators support a larger voltage range than that of the connected
> > consumers.
> 
> Adding a comment stating that would be nice then.

OK.

> > > > +	pmic@745 {
> > > > +		compatible = "x-powers,axp808", "x-powers,axp806";
> > > > +		reg = <0x745>;
> > > > +		interrupt-parent = <&nmi_intc>;
> > > > +		interrupts = <0 IRQ_TYPE_LEVEL_LOW>;
> > > > +		interrupt-controller;
> > > > +		#interrupt-cells = <1>;
> > > > +
> > > > +		swin-supply = <&reg_dcdce>;
> > > 
> > > Please use an incude for that PMIC.
> > 
> > I don't understand. What would that include file contain? Very few lines
> > would be shared between .dts files. You could save < 8 lines total if you
> > #include the 5 lines that are common between the Cubieboard4, the Optimus
> > and this board.
> 
> You also have to take into account that the PMIC is barely supported
> at the moment, you'll also need to define the GPIO controllers, the
> various power supplies, adc, etc. nodes that are going (hopefully to
> be added) in the future.
> 
> If the include is shared, once it is enabled, every one benefits from
> it. Otherwise, you have to duplicate that change across all DTs. This
> is much likely to error, harder to maintain, and you end up in a
> situation where none of the boards really support the same feature set
> while being based on the same SoC / PMIC combination.

Fair enough, I'll add an include file the next time around. Like you
mentioned youself, though, we will only really know how compatible the two
PMICs are when we have the code to support the other features (or a data
sheet/user's manual is found). I'm hopeful based on what has been found so
far.

-- 
Rask Ingemann Lambertsen

[toc] | [prev] | [next] | [standalone]

#1585805

FromMaxime Ripard <maxime.ripard@free-electrons.com>
Date2017-02-22 00:30 +0100
Message-ID<tds2C-151-15@gated-at.bofh.it>
In reply to#1584248

[Multipart message — attachments visible in raw view] — view raw

On Sun, Feb 19, 2017 at 09:12:32PM +0100, Rask Ingemann Lambertsen wrote:
> On Thu, Feb 16, 2017 at 07:29:57PM +0100, Maxime Ripard wrote:
> > On Thu, Feb 16, 2017 at 07:17:54AM +0100, Rask Ingemann Lambertsen wrote:
> > > > > Supported features (+ means tested):
> [...]
> > > > > + SATA port on on-board SATA-to-USB bridge *
> [...]
> > > > > * Only shows up when a SATA device is connected. Also, if a power source
> > > > >   is connected to the USB 3.0 connector across power cycles (e.g. FEL
> > > > >   boot), the bridge may not properly reset and not show up on the USB bus.
> > > > >   The vendor U-Boot performs some unknown magic which resets the bridge.
> > > > 
> > > > Is that magic at the USB level, or specific to the bridge itself?
> > > 
> > > I don't know, but since the other USB port connected to the hub comes up
> > > fine, I'm assuming it's something to do with the SATA-to-USB bridge.
> > 
> > But where is that magic written to then? the USB controller registers,
> > or is it an USB packet?
> 
> I don't know. Like I said, unknown magic. I haven't found the source code to
> the U-Boot version that the board uses.

Then how do you know there's a magic in the first place?

> For all I know, it could be a bug in the Linux USB stack. I've always had
> to unplug and replug my USB keyboards after Linux has booted, otherwise
> they are not detected. If the keyboard was connected to a hub, Linux doesn't
> detect that port on the hub, so the hub must be unplugged and replugged.
> If it's an internal hub, tough luck.

I have a laptop that certainly doesn't show that behaviour, so a bug
in the USB stack seems weird.

> > > > > +	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;
> > > > 
> > > > This is redundant with the GPIO flag
> 
> The GPIO flag is overridden by the regulator core, which makes it misleading.
> I will remove the GPIO flag and add a comment there that GPIO flags are not
> supported. This is to prevent someone from thinking that
> 
> gpio = <... GPIO_ACTIVE_HIGH>;
> enable-active-high;
> 
> and
> 
> gpio = <... GPIO_ACTIVE_LOW>;
> 
> are equivalent. It would be an easy mistake to make, because in mmc nodes,
> 
> cd-gpios = <...  GPIO_ACTIVE_HIGH>;
> 
> and
> 
> cd-gpios = <...  GPIO_ACTIVE_LOW>;
> cd-inverted;
> 
> _are_ equivalent.

And your point is? The gpio flags are now properly passed through to
their respective consumers, which was not the case before. Everything
works, without the workaround that we had to use before. Why would you
want to use anything else than the property that just works without
any modification and with the minimal amount of "code" ?

> 
> > > No. I have now tested without "enable-active-high" and then Vbus stays off.
> > > This is also in line with the binding documentation:
> > > 
> > > "- enable-active-high: Polarity of GPIO is Active high
> > >  If this property is missing, the default assumed is Active low."
> > >
> > > It also seems to me that the way this is implemented in Linux, the GPIO
> > > polarity flag is ignored.
> > 
> > It works just fine if the driver uses the gpiod API. If that driver
> > doesn't, then that driver needs some fixing :)
> 
> Take it up with the maintainer of the regulator core, then. :)

Or you can also submit a patch fixing it.

> From of_get_fixed_voltage_config() in drivers/regulator/fixed.c:
> [...]
> 	config->gpio = of_get_named_gpio(np, "gpio", 0);
> [...]
> 	config->enable_high = of_property_read_bool(np, "enable-active-high");
> 	config->gpio_is_open_drain = of_property_read_bool(np,
> 							   "gpio-open-drain");
> [...]
> 
> From reg_fixed_voltage_probe() in drivers/regulator/fixed.c:
> [...]
> 		config = of_get_fixed_voltage_config(&pdev->dev,
> 						     &drvdata->desc);
> [...]
> 	if (gpio_is_valid(config->gpio)) {
> 		cfg.ena_gpio = config->gpio;
> 		if (pdev->dev.of_node)
> 			cfg.ena_gpio_initialized = true;
> 	}
> 	cfg.ena_gpio_invert = !config->enable_high;
> 	if (config->enabled_at_boot) {
> 		if (config->enable_high)
> 			cfg.ena_gpio_flags |= GPIOF_OUT_INIT_HIGH;
> 		else
> 			cfg.ena_gpio_flags |= GPIOF_OUT_INIT_LOW;
> 	} else {
> 		if (config->enable_high)
> 			cfg.ena_gpio_flags |= GPIOF_OUT_INIT_LOW;
> 		else
> 			cfg.ena_gpio_flags |= GPIOF_OUT_INIT_HIGH;
> 	}
> 	if (config->gpio_is_open_drain)
> 		cfg.ena_gpio_flags |= GPIOF_OPEN_DRAIN;
> [...]
> 
> From regulator_ena_gpio_request() in drivers/regulator/core.c:
> [...]
> 	gpiod = gpio_to_desc(config->ena_gpio);
> [...]
> 	ret = gpio_request_one(config->ena_gpio,
> 				GPIOF_DIR_OUT | config->ena_gpio_flags,
> 				rdev_get_name(rdev));
> [...]
> 	pin->gpiod = gpiod;
> 	pin->ena_gpio_invert = config->ena_gpio_invert;
> 
> And finally, gpio_request_one() in drivers/gpio/gpiolib-legacy.c:
> 
> int gpio_request_one(unsigned gpio, unsigned long flags, const char *label)
> {
> 	struct gpio_desc *desc;
> 	int err;
> 
> 	desc = gpio_to_desc(gpio);
> [...]
> 	if (flags & GPIOF_OPEN_DRAIN)
> 		set_bit(FLAG_OPEN_DRAIN, &desc->flags);
> 
> 	if (flags & GPIOF_OPEN_SOURCE)
> 		set_bit(FLAG_OPEN_SOURCE, &desc->flags);
> 
> 	if (flags & GPIOF_ACTIVE_LOW)
> 		set_bit(FLAG_ACTIVE_LOW, &desc->flags);
> 
> 	if (flags & GPIOF_DIR_IN)
> 		err = gpiod_direction_input(desc);
> 	else
> 		err = gpiod_direction_output_raw(desc,
> 				(flags & GPIOF_INIT_HIGH) ? 1 : 0);
> [...]
> 
> > > > > +		regulator-always-on;
> > > > 
> > > > And it shouldn't be always on. The USB driver will enable it if needs
> > > > be.
> > > 
> > > Yes, we just don't have a driver for the A80 USB 3.0 port yet.
> > 
> > Aaah, so that's what you meant. Leave it out of the DT then.
> 
> OK.
> 
> > > > You're listing a minimum state of 750mv, yet your minimum voltage is
> > > > 800mV.
> > > 
> > > Yes. The regulator is capable of outputting the listed voltages in the
> > > 750 mV to 1200 mV range, but the permissible range of the consumer is only
> > > 800 mV to 1100 mV according to the A80 data sheet. Likewise, the AXP806
> > > regulators support a larger voltage range than that of the connected
> > > consumers.
> > 
> > Adding a comment stating that would be nice then.
> 
> OK.
> 
> > > > > +	pmic@745 {
> > > > > +		compatible = "x-powers,axp808", "x-powers,axp806";
> > > > > +		reg = <0x745>;
> > > > > +		interrupt-parent = <&nmi_intc>;
> > > > > +		interrupts = <0 IRQ_TYPE_LEVEL_LOW>;
> > > > > +		interrupt-controller;
> > > > > +		#interrupt-cells = <1>;
> > > > > +
> > > > > +		swin-supply = <&reg_dcdce>;
> > > > 
> > > > Please use an incude for that PMIC.
> > > 
> > > I don't understand. What would that include file contain? Very few lines
> > > would be shared between .dts files. You could save < 8 lines total if you
> > > #include the 5 lines that are common between the Cubieboard4, the Optimus
> > > and this board.
> > 
> > You also have to take into account that the PMIC is barely supported
> > at the moment, you'll also need to define the GPIO controllers, the
> > various power supplies, adc, etc. nodes that are going (hopefully to
> > be added) in the future.
> > 
> > If the include is shared, once it is enabled, every one benefits from
> > it. Otherwise, you have to duplicate that change across all DTs. This
> > is much likely to error, harder to maintain, and you end up in a
> > situation where none of the boards really support the same feature set
> > while being based on the same SoC / PMIC combination.
> 
> Fair enough, I'll add an include file the next time around. Like you
> mentioned youself, though, we will only really know how compatible the two
> PMICs are when we have the code to support the other features (or a data
> sheet/user's manual is found). I'm hopeful based on what has been found so
> far.

Adding a new compatible early on is cheap. Dealing with a device that
was supposed to be fully compatible but turned out not to be is really
painful, so yeah, it's better to just add a new compatible from the
very beginning, even if we'll always behave in the exact same way.

Maxime

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

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web