[PATCH v2 0/5] arm: sun9i: Support AXP808 PMIC and Sunchip CX-A99 board

[PATCH v2 0/5] arm: sun9i: Support AXP808 PMIC and Sunchip CX-A99 board

[Rask Ingemann Lambertsen]

This patch series add support for the Sunchip CX-A99 board, which is used in
a few media players, making it the third device with an Allwinner A80 SoC
to be supported. The board uses the X-Powers AXP808 PMIC which this patch
series also adds support for.

Patch 1 adds a documentation line which was forgotten when device tree
bindings for the axp20x family were updated for the AXP806 PMIC. A line
immediately below the missing line will be inserted by patch 2, so I'm
including patch 1 in this series to avoid conflicts between patches.

Patch 2 adds the AXP808 to the axp20x device tree bindings.

Patch 3 adds support for the AXP808 to the axp20x MFD driver.

Patch 4 adds support for the AXP808 to the axp20x regulator driver.

Patch 5 add the device tree for the Sunchip CX-A99 board. This patch is v6 of
the only remaining patch from v4 of the patch series posted back in August.
Support for the AXP808 has made major improvements possible, notably Wifi.

Before trying out this patch series, make sure your tree contains patch
"regulator: axp20x: AXP806: Fix dcdcb being set instead of dcdce" [1]
to avoid damage to the DRAM chips through over-voltage.

[1] https://patchwork.kernel.org/patch/9530319/

Rask Ingemann Lambertsen (5):
  dts: mfd: axp20x: Add AXP806 to list of current AXP20x family members
  dts: mfd: axp20x: Add binding for the AXP808
  mfd: axp20x: Add support for the AXP808 PMIC
  regulator: axp20x: Add support for the AXP808 PMIC
  ARM: dts: sun9i: Initial support for the Sunchip CX-A99 board

 Documentation/devicetree/bindings/mfd/axp20x.txt |   8 +-
 arch/arm/boot/dts/Makefile                       |   3 +-
 arch/arm/boot/dts/sun9i-a80-cx-a99.dts           | 409 +++++++++++++++++++++++
 drivers/mfd/axp20x-rsb.c                         |   1 +
 drivers/mfd/axp20x.c                             |  26 ++
 drivers/regulator/axp20x-regulator.c             |   7 +-
 include/linux/mfd/axp20x.h                       |   1 +
 7 files changed, 449 insertions(+), 6 deletions(-)
 create mode 100644 arch/arm/boot/dts/sun9i-a80-cx-a99.dts

-- 
2.10.2

[PATCH v2 4/5] regulator: axp20x: Add support for the AXP808 PMIC

[Rask Ingemann Lambertsen]

The regulators are the same as on the AXP806.

Signed-off-by: Rask Ingemann Lambertsen <rask@formelder.dk>
Acked-by: Mark Brown <broonie@kernel.org>
Acked-by: Chen-Yu Tsai <wens@csie.org>
---

No changes in v2.

 drivers/regulator/axp20x-regulator.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/drivers/regulator/axp20x-regulator.c b/drivers/regulator/axp20x-regulator.c
index a3ade9e..034dce8 100644
--- a/drivers/regulator/axp20x-regulator.c
+++ b/drivers/regulator/axp20x-regulator.c
@@ -370,6 +370,7 @@ static int axp20x_set_dcdc_freq(struct platform_device *pdev, u32 dcdcfreq)
 		step = 75;
 		break;
 	case AXP806_ID:
+	case AXP808_ID:
 		/*
 		 * AXP806 DCDC work frequency setting has the same range and
 		 * step as AXP22X, but at a different register.
@@ -459,6 +460,7 @@ static int axp20x_set_dcdc_workmode(struct regulator_dev *rdev, int id, u32 work
 		break;
 
 	case AXP806_ID:
+	case AXP808_ID:
 		reg = AXP806_DCDC_MODE_CTRL2;
 		/*
 		 * AXP806 DCDC regulator IDs have the same range as AXP22X.
@@ -492,8 +494,8 @@ static bool axp20x_is_polyphase_slave(struct axp20x_dev *axp20x, int id)
 {
 	u32 reg = 0;
 
-	/* Only AXP806 has poly-phase outputs */
-	if (axp20x->variant != AXP806_ID)
+	/* Only AXP806 and AXP808 have poly-phase outputs */
+	if (axp20x->variant != AXP806_ID && axp20x->variant != AXP808_ID)
 		return false;
 
 	regmap_read(axp20x->regmap, AXP806_DCDC_MODE_CTRL2, &reg);
@@ -541,6 +543,7 @@ static int axp20x_regulator_probe(struct platform_device *pdev)
 						  "x-powers,drive-vbus-en");
 		break;
 	case AXP806_ID:
+	case AXP808_ID:
 		regulators = axp806_regulators;
 		nregulators = AXP806_REG_ID_MAX;
 		break;
-- 
2.10.2

[PATCH v2 1/5] dts: mfd: axp20x: Add AXP806 to list of current AXP20x family members

[Rask Ingemann Lambertsen]

An entry for the AXP806 was forgotten, so add one.

Fixes: 204ae2963e10 ("mfd: axp20x: Add bindings for AXP806 PMIC")
Signed-off-by: Rask Ingemann Lambertsen <rask@formelder.dk>
Acked-by: Chen-Yu Tsai <wens@csie.org>
Acked-by: Rob Herring <robh@kernel.org>
---

No changes in v2.

 Documentation/devicetree/bindings/mfd/axp20x.txt | 1 +
 1 file changed, 1 insertion(+)

diff --git a/Documentation/devicetree/bindings/mfd/axp20x.txt b/Documentation/devicetree/bindings/mfd/axp20x.txt
index 8f3ad9a..86ae540 100644
--- a/Documentation/devicetree/bindings/mfd/axp20x.txt
+++ b/Documentation/devicetree/bindings/mfd/axp20x.txt
@@ -6,6 +6,7 @@ axp202 (X-Powers)
 axp209 (X-Powers)
 axp221 (X-Powers)
 axp223 (X-Powers)
+axp806 (X-Powers)
 axp809 (X-Powers)
 
 Required properties:
-- 
2.10.2

[PATCH v2 2/5] dts: mfd: axp20x: Add binding for the AXP808

[Rask Ingemann Lambertsen]

The AXP808 does not support address space extension, but is otherwise
identical to the AXP806, including the chip ID, so add a compatible
string for it to the binding.

Signed-off-by: Rask Ingemann Lambertsen <rask@formelder.dk>
Acked-by: Rob Herring <robh@kernel.org>
Acked-by: Chen-Yu Tsai <wens@csie.org>
---

No changes in v2.

 Documentation/devicetree/bindings/mfd/axp20x.txt | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/Documentation/devicetree/bindings/mfd/axp20x.txt b/Documentation/devicetree/bindings/mfd/axp20x.txt
index 86ae540..8195b06 100644
--- a/Documentation/devicetree/bindings/mfd/axp20x.txt
+++ b/Documentation/devicetree/bindings/mfd/axp20x.txt
@@ -7,12 +7,13 @@ axp209 (X-Powers)
 axp221 (X-Powers)
 axp223 (X-Powers)
 axp806 (X-Powers)
+axp808 (X-Powers)
 axp809 (X-Powers)
 
 Required properties:
 - compatible: "x-powers,axp152", "x-powers,axp202", "x-powers,axp209",
 	      "x-powers,axp221", "x-powers,axp223", "x-powers,axp806",
-	      "x-powers,axp809"
+	      "x-powers,axp808", "x-powers,axp809"
 - reg: The I2C slave address or RSB hardware address for the AXP chip
 - interrupt-parent: The parent interrupt controller
 - interrupts: SoC NMI / GPIO interrupt connected to the PMIC's IRQ pin
@@ -87,7 +88,7 @@ LDO_IO1		: LDO		: ips-supply		: GPIO 1
 RTC_LDO		: LDO		: ips-supply		: always on
 DRIVEVBUS	: Enable output	: drivevbus-supply	: external regulator
 
-AXP806 regulators, type, and corresponding input supply names:
+AXP806/AXP808 regulators, type, and corresponding input supply names:
 
 Regulator	  Type		  Supply Name		  Notes
 ---------	  ----		  -----------		  -----
@@ -108,7 +109,7 @@ CLDO2		: LDO		: cldoin-supply		: shared supply
 CLDO3		: LDO		: cldoin-supply		: shared supply
 SW		: On/Off Switch : swin-supply
 
-Additionally, the AXP806 DC-DC regulators support poly-phase arrangements
+Additionally, the AXP806/AXP808 DC-DC regulators support poly-phase arrangements
 for higher output current. The possible groupings are: A+B, A+B+C, D+E.
 
 AXP809 regulators, type, and corresponding input supply names:
-- 
2.10.2

[PATCH v2 3/5] mfd: axp20x: Add support for the AXP808 PMIC

[Rask Ingemann Lambertsen]

The X-Powers AXP808 is a PMIC which, like the very similar AXP806, is
used on boards featuring Allwinner's A80 SoC. Unlike the AXP806, it
doesn't support address space extension and its associated registers,
but the two are otherwise identical (including the chip ID). After
commit b101829a029a ("mfd: axp20x: Fix AXP806 access errors on cold boot")
those registers are now used, causing the driver to no longer work with
an AXP808.

This patch adds support for the interrupts on the AXP808 and enables
the regulator sub-device. The next patch in the series adds the actual
regulator support.

Signed-off-by: Rask Ingemann Lambertsen <rask@formelder.dk>
Acked-by: Chen-Yu Tsai <wens@csie.org>
---

Changes in v2:
- Noted in the commit message that another commit for better AXP806 support
  means the driver no longer works with an AXP808.
- axp808_writeable_table was changed to use the common part (between AXP806 and
  AXP808) of axp806_writeable_ranges instead of duplicating the entries.
  Suggested by Chen-Yu Tsai.

 drivers/mfd/axp20x-rsb.c   |  1 +
 drivers/mfd/axp20x.c       | 26 ++++++++++++++++++++++++++
 include/linux/mfd/axp20x.h |  1 +
 3 files changed, 28 insertions(+)

diff --git a/drivers/mfd/axp20x-rsb.c b/drivers/mfd/axp20x-rsb.c
index a732cb5..96ea2e9 100644
--- a/drivers/mfd/axp20x-rsb.c
+++ b/drivers/mfd/axp20x-rsb.c
@@ -62,6 +62,7 @@ static int axp20x_rsb_remove(struct sunxi_rsb_device *rdev)
 static const struct of_device_id axp20x_rsb_of_match[] = {
 	{ .compatible = "x-powers,axp223", .data = (void *)AXP223_ID },
 	{ .compatible = "x-powers,axp806", .data = (void *)AXP806_ID },
+	{ .compatible = "x-powers,axp808", .data = (void *)AXP808_ID },
 	{ .compatible = "x-powers,axp809", .data = (void *)AXP809_ID },
 	{ },
 };
diff --git a/drivers/mfd/axp20x.c b/drivers/mfd/axp20x.c
index 25115fe..125b470 100644
--- a/drivers/mfd/axp20x.c
+++ b/drivers/mfd/axp20x.c
@@ -41,6 +41,7 @@ static const char * const axp20x_model_names[] = {
 	"AXP223",
 	"AXP288",
 	"AXP806",
+	"AXP808",
 	"AXP809",
 };
 
@@ -140,6 +141,11 @@ static const struct regmap_access_table axp288_volatile_table = {
 	.n_yes_ranges	= ARRAY_SIZE(axp288_volatile_ranges),
 };
 
+/*
+ * These ranges are used by both axp806_writeable_table and
+ * axp808_writeable_table. This is possible as long as those not supported
+ * by the AXP808 (currently only the last one) are kept at an end.
+ */
 static const struct regmap_range axp806_writeable_ranges[] = {
 	regmap_reg_range(AXP20X_DATACACHE(0), AXP20X_DATACACHE(3)),
 	regmap_reg_range(AXP806_PWR_OUT_CTRL1, AXP806_CLDO3_V_CTRL),
@@ -162,6 +168,11 @@ static const struct regmap_access_table axp806_volatile_table = {
 	.n_yes_ranges	= ARRAY_SIZE(axp806_volatile_ranges),
 };
 
+static const struct regmap_access_table axp808_writeable_table = {
+	.yes_ranges	= axp806_writeable_ranges,
+	.n_yes_ranges	= ARRAY_SIZE(axp806_writeable_ranges) - 1,
+};
+
 static struct resource axp152_pek_resources[] = {
 	DEFINE_RES_IRQ_NAMED(AXP152_IRQ_PEK_RIS_EDGE, "PEK_DBR"),
 	DEFINE_RES_IRQ_NAMED(AXP152_IRQ_PEK_FAL_EDGE, "PEK_DBF"),
@@ -320,6 +331,15 @@ static const struct regmap_config axp806_regmap_config = {
 	.cache_type	= REGCACHE_RBTREE,
 };
 
+static const struct regmap_config axp808_regmap_config = {
+	.reg_bits	= 8,
+	.val_bits	= 8,
+	.wr_table	= &axp808_writeable_table,
+	.volatile_table	= &axp806_volatile_table,
+	.max_register	= AXP806_VREF_TEMP_WARN_L,
+	.cache_type	= REGCACHE_RBTREE,
+};
+
 #define INIT_REGMAP_IRQ(_variant, _irq, _off, _mask)			\
 	[_variant##_IRQ_##_irq] = { .reg_offset = (_off), .mask = BIT(_mask) }
 
@@ -842,6 +862,12 @@ int axp20x_match_device(struct axp20x_dev *axp20x)
 		axp20x->regmap_cfg = &axp806_regmap_config;
 		axp20x->regmap_irq_chip = &axp806_regmap_irq_chip;
 		break;
+	case AXP808_ID:
+		axp20x->nr_cells = ARRAY_SIZE(axp806_cells);
+		axp20x->cells = axp806_cells;
+		axp20x->regmap_cfg = &axp808_regmap_config;
+		axp20x->regmap_irq_chip = &axp806_regmap_irq_chip;
+		break;
 	case AXP809_ID:
 		axp20x->nr_cells = ARRAY_SIZE(axp809_cells);
 		axp20x->cells = axp809_cells;
diff --git a/include/linux/mfd/axp20x.h b/include/linux/mfd/axp20x.h
index 0d9a1ff..48c1788 100644
--- a/include/linux/mfd/axp20x.h
+++ b/include/linux/mfd/axp20x.h
@@ -21,6 +21,7 @@ enum axp20x_variants {
 	AXP223_ID,
 	AXP288_ID,
 	AXP806_ID,
+	AXP808_ID,
 	AXP809_ID,
 	NR_AXP20X_VARIANTS,
 };
-- 
2.10.2

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

[Rask Ingemann Lambertsen]

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>.

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.

So far unsupported features:
- Using any of the Cortex-A15 CPU cores
- USB 3.0 port (except for supplying 5 V power)
- IEEE 802.3 10/100/1000 megabit Ethernet
- HDMI connector
- S/PDIF audio output
- Jack socket with composite video and analog stereo audio
- Bluetooth
- FM radio receiver (assuming it is even wired on the board)

Signed-off-by: Rask Ingemann Lambertsen <rask@formelder.dk>
---

Changes in v6:
- Updated commit message description of SATA-to-USB bridge quirk and added
  note about experimental U-Boot PSCI support for up to four CPU cores.
- The blue LED is no longer on by default as its meaning is not documented.
- Removed "regulator-boot-on" from regulators having "regulator-always-on".
- Removed misleading mention of "OTG connector" which the device doesn't have.
- More detailed explanation for the need for "broken-cd" on mmc0.
- Several regulators have had their voltage range relaxed a little to match
  the permissible range according to the data sheets of the consumers. This
  is similar to what is used for the Cubieboard4 and Merrii A80 Optimus.
- Shortened regulator dcdce name as per v5 comments. A comment now lists the
  pin groups supplied by dcdce.

Changes in v5:
- Switched pinmux modes to generic properties and dropped 
  #include <dt-bindings/pinctrl/sun4i-a10.h> as a consequence.
- Dropped pinctrl properties from GPIO nodes and dropped the pinmux
  nodes for them.
- AXP808 regulators added.
- Dropped the now unused #include <sunxi-common-regulators.dtsi>.
- Ampak AP6335 SDIO-Wifi added.
- USB Vbus changes as per v4 comments.
- Added "broken-cd" to mmc0 because GPIO interrupts don't work.

Changes in v4:
- Node names had underscores changed to hyphens.
- Changed formatting of the ac100/rtc node's clock output name list to match
 that of the same node in the cubieboard4 and a80-optimus device trees.

Changes in v3:
None.

Changes in v2:
- Fixed formatting and style issues found by scripts/checkpatch.pl.

 arch/arm/boot/dts/Makefile             |   3 +-
 arch/arm/boot/dts/sun9i-a80-cx-a99.dts | 409 +++++++++++++++++++++++++++++++++
 2 files changed, 411 insertions(+), 1 deletion(-)
 create mode 100644 arch/arm/boot/dts/sun9i-a80-cx-a99.dts

diff --git a/arch/arm/boot/dts/Makefile b/arch/arm/boot/dts/Makefile
index 8553bd7..40546fa 100644
--- a/arch/arm/boot/dts/Makefile
+++ b/arch/arm/boot/dts/Makefile
@@ -862,7 +862,8 @@ dtb-$(CONFIG_MACH_SUN8I) += \
 	sun8i-v3s-licheepi-zero.dtb
 dtb-$(CONFIG_MACH_SUN9I) += \
 	sun9i-a80-optimus.dtb \
-	sun9i-a80-cubieboard4.dtb
+	sun9i-a80-cubieboard4.dtb \
+	sun9i-a80-cx-a99.dtb
 dtb-$(CONFIG_ARCH_TANGO) += \
 	tango4-vantage-1172.dtb
 dtb-$(CONFIG_ARCH_TEGRA_2x_SOC) += \
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
@@ -0,0 +1,409 @@
+/*
+ * sun9i-a80-cx-a99.dts - Device Tree file for the Sunchip CX-A99 board.
+ *
+ * Copyright (C) 2017 Rask Ingemann Lambertsen <rask@formelder.dk>
+ *
+ * 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.
+ */
+
+/*
+ * The Sunchip CX-A99 board is found in several similar Android media
+ * players, such as:
+ *
+ * Instabox Fantasy A8 (no external antenna)
+ * Jesurun CS-Q8 (ships with larger remote control)
+ * Jesurun Maxone
+ * Rikomagic (RKM) MK80/MK80LE
+ * Tronsmart Draco AW80 Meta/Telos
+ *
+ * See <URL:https://linux-sunxi.org/Sunchip_CX-A99> for more information.
+ */
+
+/dts-v1/;
+#include "sun9i-a80.dtsi"
+
+#include <dt-bindings/gpio/gpio.h>
+
+/ {
+	model = "Sunchip CX-A99";
+	compatible = "sunchip,cx-a99", "allwinner,sun9i-a80";
+
+	aliases {
+		serial0 = &uart0;
+	};
+
+	chosen {
+		stdout-path = "serial0:115200n8";
+	};
+
+	leds {
+		compatible = "gpio-leds";
+
+		blue {
+			gpios = <&pio 6 10 GPIO_ACTIVE_HIGH>;	/* PG10 */
+			label = "cx-a99:blue:status";
+		};
+
+		red {
+			gpios = <&pio 6 11 GPIO_ACTIVE_HIGH>;	/* PG11 */
+			label = "cx-a99:red:status";
+		};
+	};
+
+	powerseq_wifi: powerseq-wifi {
+		compatible = "mmc-pwrseq-simple";
+		clocks = <&ac100_rtc 1>;
+		clock-names = "ext_clock";
+		reset-gpios = <&r_pio 1 0 GPIO_ACTIVE_LOW>;	/* PM0 */
+		post-power-on-delay-ms = <1>;	/* Minimum 2 cycles. */
+	};
+
+	/* USB 3.0 standard-A receptacle. For now, only Vbus is supported. */
+	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;
+		regulator-always-on;
+	};
+
+	/*
+	 * 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.
+	 */
+	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;
+	};
+
+	/* 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>;
+	};
+};
+
+&ehci0 {
+	status = "okay";
+};
+
+&ehci2 {
+	status = "okay";
+};
+
+/*
+ * SD card slot. Although the GPIO pin for card detection is listed as capable
+ * of generating interrupts in the "A80 User Manual", this doesn't work for
+ * some unknown reason, so poll the GPIO for card detection. This is also what
+ * the vendor sys_config.fex file specifies.
+ */
+&mmc0 {
+	bus-width = <4>;
+	cd-gpios = <&pio 7 17 GPIO_ACTIVE_LOW>;	/* PH17 */
+	broken-cd;				/* Poll. */
+	pinctrl-names = "default";
+	pinctrl-0 = <&mmc0_pins>;
+	vmmc-supply = <&reg_dcdce>;
+	status = "okay";
+};
+
+/* Ampak AP6335 IEEE 802.11 a/b/g/n/ac "Wifi". */
+&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>;
+	mmc-pwrseq = <&powerseq_wifi>;
+	status = "okay";
+};
+
+/* On-board eMMC card. */
+&mmc2 {
+	bus-width = <8>;
+	non-removable;
+	pinctrl-names = "default";
+	pinctrl-0 = <&mmc2_8bit_pins>;
+	vmmc-supply = <&reg_dcdce>;
+	status = "okay";
+};
+
+&osc32k {
+	clocks = <&ac100_rtc 0>;
+};
+
+&r_ir {
+	status = "okay";
+};
+
+&r_rsb {
+	status = "okay";
+
+	ac100: codec@e89 {
+		compatible = "x-powers,ac100";
+		reg = <0xe89>;
+
+		ac100_codec: codec {
+			compatible = "x-powers,ac100-codec";
+			interrupt-parent = <&r_pio>;
+			interrupts = <0 9 IRQ_TYPE_LEVEL_LOW>;	/* PL9 */
+			#clock-cells = <0>;
+			clock-output-names = "4M_adda";
+		};
+
+		ac100_rtc: rtc {
+			compatible = "x-powers,ac100-rtc";
+			interrupt-parent = <&nmi_intc>;
+			interrupts = <0 IRQ_TYPE_LEVEL_LOW>;
+			clocks = <&ac100_codec>;
+			#clock-cells = <1>;
+			clock-output-names = "cko1_rtc",
+					     "cko2_rtc",
+					     "cko3_rtc";
+		};
+	};
+
+	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>;
+
+		/* In comments: Initial setup from vendor sys_config.fex. */
+		regulators {
+			/* 3.0 V (enabled). */
+			reg_aldo1: aldo1 {
+				regulator-boot-on;
+				regulator-min-microvolt = <3000000>;
+				regulator-max-microvolt = <3000000>;
+				regulator-name = "vcc-3v0";
+			};
+
+			/* 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";
+			};
+
+			/* 2.5 V (enabled). */
+			reg_aldo3: aldo3 {
+				regulator-boot-on;
+				regulator-min-microvolt = <2500000>;
+				regulator-max-microvolt = <2500000>;
+				regulator-name = "vcc-pa-gmac2v5";
+			};
+
+			/* 1.8 V (enabled). */
+			reg_bldo1: bldo1 {
+				regulator-always-on;	/* Hang if disabled */
+				regulator-min-microvolt = <1700000>;
+				regulator-max-microvolt = <1900000>;
+				regulator-name = "vdd18-dll-vcc18-pll";
+			};
+
+			/* 0.9 V (enabled). */
+			reg_bldo2: bldo2 {
+				regulator-always-on;	/* Hang if disabled */
+				regulator-min-microvolt = < 800000>;
+				regulator-max-microvolt = <1100000>;
+				regulator-name = "vdd-cpus";
+			};
+
+			/* 1.2 V (disabled). */
+			reg_bldo3: bldo3 {
+				regulator-min-microvolt = <1100000>;
+				regulator-max-microvolt = <1300000>;
+				regulator-name = "vcc12-hsic";
+			};
+
+			/* 1.1 V (enabled). */
+			reg_bldo4: bldo4 {
+				regulator-boot-on;
+				regulator-min-microvolt = < 800000>;
+				regulator-max-microvolt = <1100000>;
+				regulator-name = "vdd09-hdmi";
+			};
+
+			/* 3.3 V (enabled). PLx pins control some regulators. */
+			reg_cldo1: cldo1 {
+				regulator-always-on;
+				regulator-min-microvolt = <3300000>;
+				regulator-max-microvolt = <3300000>;
+				regulator-name = "vcc-pl-led";
+			};
+
+			/*
+			 * cldo2 and cldo3 are connected in parallel.
+			 * There is currently no way to express that.
+			 * For now, use regulator-always-on on cldo2 and lock
+			 * the voltage on both to 3.3 V.
+			 *
+			 * 3.3 V (disabled).
+			 */
+			reg_cldo2: cldo2 {
+				regulator-always-on;
+				regulator-min-microvolt = <3300000>;
+				regulator-max-microvolt = <3300000>;
+				regulator-name = "vbat2-wifi+bt";
+			};
+
+			/* 3.3 V (disabled). */
+			reg_cldo3: cldo3 {
+				regulator-min-microvolt = <3300000>;
+				regulator-max-microvolt = <3300000>;
+				regulator-name = "vbat1-wifi+bt";
+			};
+
+			/* 0.9 V (enabled). */
+			reg_dcdca: dcdca {
+				regulator-always-on;
+				regulator-min-microvolt = < 800000>;
+				regulator-max-microvolt = <1100000>;
+				regulator-name = "vdd-cpua";
+			};
+
+			/* 1.5 V (enabled). */
+			reg_dcdcb: dcdcb {
+				regulator-always-on;
+				regulator-min-microvolt = <1450000>;
+				regulator-max-microvolt = <1550000>;
+				regulator-name = "vcc-dram";
+			};
+
+			/* 0.9 V (disabled). */
+			reg_dcdcc: dcdcc {
+				regulator-min-microvolt = < 800000>;
+				regulator-max-microvolt = <1100000>;
+				regulator-name = "vdd-gpu";
+			};
+
+			/* 0.9 V (enabled). */
+			reg_dcdcd: dcdcd {
+				regulator-always-on;	/* Hang if disabled. */
+				regulator-min-microvolt = < 800000>;
+				regulator-max-microvolt = <1100000>;
+				regulator-name = "vdd-sys";
+			};
+
+			/* 3.3 V (enabled). Supplies pin groups B-F and H. */
+			reg_dcdce: dcdce {
+				regulator-always-on;
+				regulator-min-microvolt = <3300000>;
+				regulator-max-microvolt = <3300000>;
+				regulator-name = "vcc-io-mmc-spdif";
+			};
+
+			/* 3.3 V (disabled). */
+			reg_sw: sw {
+				regulator-min-microvolt = <3300000>;
+				regulator-max-microvolt = <3300000>;
+				regulator-name = "vcc-gmac3v3-audio";
+			};
+		};
+	};
+};
+
+/*
+ * 5-pin connector opposite of the SD card slot:
+ * 1 = GND (pointed to by small triangle), 2 = GND, 3 = 3.3 V, 4 = RX, 5 = TX.
+ */
+&uart0 {
+	pinctrl-names = "default";
+	pinctrl-0 = <&uart0_pins_a>;
+	status = "okay";
+};
+
+/* The port has two Vbus supplies. See workaround at regulator-usb1-1-vbus. */
+&usbphy1 {
+	phy-supply = <&reg_usb1_1_vbus>;
+	status = "okay";
+};
+
+&usbphy3 {
+	status = "okay";
+};
-- 
2.10.2

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

[Maxime Ripard]

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

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).

> 
> 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?

> So far unsupported features:
> - Using any of the Cortex-A15 CPU cores
> - USB 3.0 port (except for supplying 5 V power)
> - IEEE 802.3 10/100/1000 megabit Ethernet
> - HDMI connector
> - S/PDIF audio output
> - Jack socket with composite video and analog stereo audio
> - Bluetooth
> - FM radio receiver (assuming it is even wired on the board)

I guess that should be in your cover letter.

This is not found in your DT, so no one really expects it to work :)

> Signed-off-by: Rask Ingemann Lambertsen <rask@formelder.dk>
> ---
> 
> Changes in v6:
> - Updated commit message description of SATA-to-USB bridge quirk and added
>   note about experimental U-Boot PSCI support for up to four CPU cores.
> - The blue LED is no longer on by default as its meaning is not documented.
> - Removed "regulator-boot-on" from regulators having "regulator-always-on".
> - Removed misleading mention of "OTG connector" which the device doesn't have.
> - More detailed explanation for the need for "broken-cd" on mmc0.
> - Several regulators have had their voltage range relaxed a little to match
>   the permissible range according to the data sheets of the consumers. This
>   is similar to what is used for the Cubieboard4 and Merrii A80 Optimus.
> - Shortened regulator dcdce name as per v5 comments. A comment now lists the
>   pin groups supplied by dcdce.
> 
> Changes in v5:
> - Switched pinmux modes to generic properties and dropped 
>   #include <dt-bindings/pinctrl/sun4i-a10.h> as a consequence.
> - Dropped pinctrl properties from GPIO nodes and dropped the pinmux
>   nodes for them.
> - AXP808 regulators added.
> - Dropped the now unused #include <sunxi-common-regulators.dtsi>.
> - Ampak AP6335 SDIO-Wifi added.
> - USB Vbus changes as per v4 comments.
> - Added "broken-cd" to mmc0 because GPIO interrupts don't work.
> 
> Changes in v4:
> - Node names had underscores changed to hyphens.
> - Changed formatting of the ac100/rtc node's clock output name list to match
>  that of the same node in the cubieboard4 and a80-optimus device trees.
> 
> Changes in v3:
> None.
> 
> Changes in v2:
> - Fixed formatting and style issues found by scripts/checkpatch.pl.
> 
>  arch/arm/boot/dts/Makefile             |   3 +-
>  arch/arm/boot/dts/sun9i-a80-cx-a99.dts | 409 +++++++++++++++++++++++++++++++++
>  2 files changed, 411 insertions(+), 1 deletion(-)
>  create mode 100644 arch/arm/boot/dts/sun9i-a80-cx-a99.dts
> 
> diff --git a/arch/arm/boot/dts/Makefile b/arch/arm/boot/dts/Makefile
> index 8553bd7..40546fa 100644
> --- a/arch/arm/boot/dts/Makefile
> +++ b/arch/arm/boot/dts/Makefile
> @@ -862,7 +862,8 @@ dtb-$(CONFIG_MACH_SUN8I) += \
>  	sun8i-v3s-licheepi-zero.dtb
>  dtb-$(CONFIG_MACH_SUN9I) += \
>  	sun9i-a80-optimus.dtb \
> -	sun9i-a80-cubieboard4.dtb
> +	sun9i-a80-cubieboard4.dtb \
> +	sun9i-a80-cx-a99.dtb
>  dtb-$(CONFIG_ARCH_TANGO) += \
>  	tango4-vantage-1172.dtb
>  dtb-$(CONFIG_ARCH_TEGRA_2x_SOC) += \
> 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
> @@ -0,0 +1,409 @@
> +/*
> + * sun9i-a80-cx-a99.dts - Device Tree file for the Sunchip CX-A99 board.
> + *
> + * Copyright (C) 2017 Rask Ingemann Lambertsen <rask@formelder.dk>
> + *
> + * 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.
> + */
> +
> +/*
> + * The Sunchip CX-A99 board is found in several similar Android media
> + * players, such as:
> + *
> + * Instabox Fantasy A8 (no external antenna)
> + * Jesurun CS-Q8 (ships with larger remote control)
> + * Jesurun Maxone
> + * Rikomagic (RKM) MK80/MK80LE
> + * Tronsmart Draco AW80 Meta/Telos
> + *
> + * See <URL:https://linux-sunxi.org/Sunchip_CX-A99> for more information.
> + */
> +
> +/dts-v1/;
> +#include "sun9i-a80.dtsi"
> +
> +#include <dt-bindings/gpio/gpio.h>
> +
> +/ {
> +	model = "Sunchip CX-A99";
> +	compatible = "sunchip,cx-a99", "allwinner,sun9i-a80";
> +
> +	aliases {
> +		serial0 = &uart0;
> +	};
> +
> +	chosen {
> +		stdout-path = "serial0:115200n8";
> +	};
> +
> +	leds {
> +		compatible = "gpio-leds";
> +
> +		blue {
> +			gpios = <&pio 6 10 GPIO_ACTIVE_HIGH>;	/* PG10 */
> +			label = "cx-a99:blue:status";
> +		};
> +
> +		red {
> +			gpios = <&pio 6 11 GPIO_ACTIVE_HIGH>;	/* PG11 */
> +			label = "cx-a99:red:status";
> +		};
> +	};
> +
> +	powerseq_wifi: powerseq-wifi {
> +		compatible = "mmc-pwrseq-simple";
> +		clocks = <&ac100_rtc 1>;
> +		clock-names = "ext_clock";
> +		reset-gpios = <&r_pio 1 0 GPIO_ACTIVE_LOW>;	/* PM0 */
> +		post-power-on-delay-ms = <1>;	/* Minimum 2 cycles. */
> +	};
> +
> +	/* 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?

> +	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

> +		regulator-always-on;

And it shouldn't be always on. The USB driver will enable it if needs
be.

> +	};
> +
> +	/*
> +	 * 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?

> +	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.

> +	};
> +
> +	/* 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.

> +	};
> +};
> +
> +&ehci0 {
> +	status = "okay";
> +};
> +
> +&ehci2 {
> +	status = "okay";
> +};
> +
> +/*
> + * SD card slot. Although the GPIO pin for card detection is listed as capable
> + * of generating interrupts in the "A80 User Manual", this doesn't work for
> + * some unknown reason, so poll the GPIO for card detection. This is also what
> + * the vendor sys_config.fex file specifies.
> + */
> +&mmc0 {
> +	bus-width = <4>;
> +	cd-gpios = <&pio 7 17 GPIO_ACTIVE_LOW>;	/* PH17 */
> +	broken-cd;				/* Poll. */
> +	pinctrl-names = "default";
> +	pinctrl-0 = <&mmc0_pins>;
> +	vmmc-supply = <&reg_dcdce>;
> +	status = "okay";
> +};
> +
> +/* Ampak AP6335 IEEE 802.11 a/b/g/n/ac "Wifi". */

Why "wifi" ? It's not implementing true wifi?

> +&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.

> +	mmc-pwrseq = <&powerseq_wifi>;
> +	status = "okay";
> +};
> +
> +/* On-board eMMC card. */
> +&mmc2 {
> +	bus-width = <8>;
> +	non-removable;
> +	pinctrl-names = "default";
> +	pinctrl-0 = <&mmc2_8bit_pins>;
> +	vmmc-supply = <&reg_dcdce>;
> +	status = "okay";
> +};
> +
> +&osc32k {
> +	clocks = <&ac100_rtc 0>;
> +};
> +
> +&r_ir {
> +	status = "okay";
> +};
> +
> +&r_rsb {
> +	status = "okay";
> +
> +	ac100: codec@e89 {
> +		compatible = "x-powers,ac100";
> +		reg = <0xe89>;
> +
> +		ac100_codec: codec {
> +			compatible = "x-powers,ac100-codec";
> +			interrupt-parent = <&r_pio>;
> +			interrupts = <0 9 IRQ_TYPE_LEVEL_LOW>;	/* PL9 */
> +			#clock-cells = <0>;
> +			clock-output-names = "4M_adda";
> +		};
> +
> +		ac100_rtc: rtc {
> +			compatible = "x-powers,ac100-rtc";
> +			interrupt-parent = <&nmi_intc>;
> +			interrupts = <0 IRQ_TYPE_LEVEL_LOW>;
> +			clocks = <&ac100_codec>;
> +			#clock-cells = <1>;
> +			clock-output-names = "cko1_rtc",
> +					     "cko2_rtc",
> +					     "cko3_rtc";
> +		};
> +	};
> +
> +	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.

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

This might be disabled though.

> +			reg_aldo1: aldo1 {
> +				regulator-boot-on;
> +				regulator-min-microvolt = <3000000>;
> +				regulator-max-microvolt = <3000000>;
> +				regulator-name = "vcc-3v0";
> +			};
> +
> +			/* 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.

Something like vcc-wifi would be enough her.

> +			};
> +
> +			/* 2.5 V (enabled). */
> +			reg_aldo3: aldo3 {
> +				regulator-boot-on;
> +				regulator-min-microvolt = <2500000>;
> +				regulator-max-microvolt = <2500000>;
> +				regulator-name = "vcc-pa-gmac2v5";

vcc-gmac

> +			};
> +
> +			/* 1.8 V (enabled). */
> +			reg_bldo1: bldo1 {
> +				regulator-always-on;	/* Hang if disabled */
> +				regulator-min-microvolt = <1700000>;
> +				regulator-max-microvolt = <1900000>;
> +				regulator-name = "vdd18-dll-vcc18-pll";

vdd-dll

> +			};
> +
> +			/* 0.9 V (enabled). */
> +			reg_bldo2: bldo2 {
> +				regulator-always-on;	/* Hang if disabled */
> +				regulator-min-microvolt = < 800000>;
> +				regulator-max-microvolt = <1100000>;
> +				regulator-name = "vdd-cpus";
> +			};
> +
> +			/* 1.2 V (disabled). */
> +			reg_bldo3: bldo3 {
> +				regulator-min-microvolt = <1100000>;
> +				regulator-max-microvolt = <1300000>;
> +				regulator-name = "vcc12-hsic";

vcc-hsic

> +			};
> +
> +			/* 1.1 V (enabled). */
> +			reg_bldo4: bldo4 {
> +				regulator-boot-on;
> +				regulator-min-microvolt = < 800000>;
> +				regulator-max-microvolt = <1100000>;
> +				regulator-name = "vdd09-hdmi";

vdd-hdmi

> +			};
> +
> +			/* 3.3 V (enabled). PLx pins control some regulators. */
> +			reg_cldo1: cldo1 {
> +				regulator-always-on;
> +				regulator-min-microvolt = <3300000>;
> +				regulator-max-microvolt = <3300000>;
> +				regulator-name = "vcc-pl-led";

vcc-led, etc...

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 v6 5/5] ARM: dts: sun9i: Initial support for the Sunchip CX-A99 board

[Chen-Yu Tsai]

On Fri, Feb 10, 2017 at 4:59 PM, Maxime Ripard
<maxime.ripard@free-electrons.com> 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).
>
>>
>> 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?
>
>> So far unsupported features:
>> - Using any of the Cortex-A15 CPU cores
>> - USB 3.0 port (except for supplying 5 V power)
>> - IEEE 802.3 10/100/1000 megabit Ethernet
>> - HDMI connector
>> - S/PDIF audio output
>> - Jack socket with composite video and analog stereo audio
>> - Bluetooth
>> - FM radio receiver (assuming it is even wired on the board)
>
> I guess that should be in your cover letter.
>
> This is not found in your DT, so no one really expects it to work :)
>
>> Signed-off-by: Rask Ingemann Lambertsen <rask@formelder.dk>
>> ---
>>
>> Changes in v6:
>> - Updated commit message description of SATA-to-USB bridge quirk and added
>>   note about experimental U-Boot PSCI support for up to four CPU cores.
>> - The blue LED is no longer on by default as its meaning is not documented.
>> - Removed "regulator-boot-on" from regulators having "regulator-always-on".
>> - Removed misleading mention of "OTG connector" which the device doesn't have.
>> - More detailed explanation for the need for "broken-cd" on mmc0.
>> - Several regulators have had their voltage range relaxed a little to match
>>   the permissible range according to the data sheets of the consumers. This
>>   is similar to what is used for the Cubieboard4 and Merrii A80 Optimus.
>> - Shortened regulator dcdce name as per v5 comments. A comment now lists the
>>   pin groups supplied by dcdce.
>>
>> Changes in v5:
>> - Switched pinmux modes to generic properties and dropped
>>   #include <dt-bindings/pinctrl/sun4i-a10.h> as a consequence.
>> - Dropped pinctrl properties from GPIO nodes and dropped the pinmux
>>   nodes for them.
>> - AXP808 regulators added.
>> - Dropped the now unused #include <sunxi-common-regulators.dtsi>.
>> - Ampak AP6335 SDIO-Wifi added.
>> - USB Vbus changes as per v4 comments.
>> - Added "broken-cd" to mmc0 because GPIO interrupts don't work.
>>
>> Changes in v4:
>> - Node names had underscores changed to hyphens.
>> - Changed formatting of the ac100/rtc node's clock output name list to match
>>  that of the same node in the cubieboard4 and a80-optimus device trees.
>>
>> Changes in v3:
>> None.
>>
>> Changes in v2:
>> - Fixed formatting and style issues found by scripts/checkpatch.pl.
>>
>>  arch/arm/boot/dts/Makefile             |   3 +-
>>  arch/arm/boot/dts/sun9i-a80-cx-a99.dts | 409 +++++++++++++++++++++++++++++++++
>>  2 files changed, 411 insertions(+), 1 deletion(-)
>>  create mode 100644 arch/arm/boot/dts/sun9i-a80-cx-a99.dts
>>
>> diff --git a/arch/arm/boot/dts/Makefile b/arch/arm/boot/dts/Makefile
>> index 8553bd7..40546fa 100644
>> --- a/arch/arm/boot/dts/Makefile
>> +++ b/arch/arm/boot/dts/Makefile
>> @@ -862,7 +862,8 @@ dtb-$(CONFIG_MACH_SUN8I) += \
>>       sun8i-v3s-licheepi-zero.dtb
>>  dtb-$(CONFIG_MACH_SUN9I) += \
>>       sun9i-a80-optimus.dtb \
>> -     sun9i-a80-cubieboard4.dtb
>> +     sun9i-a80-cubieboard4.dtb \
>> +     sun9i-a80-cx-a99.dtb
>>  dtb-$(CONFIG_ARCH_TANGO) += \
>>       tango4-vantage-1172.dtb
>>  dtb-$(CONFIG_ARCH_TEGRA_2x_SOC) += \
>> 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
>> @@ -0,0 +1,409 @@
>> +/*
>> + * sun9i-a80-cx-a99.dts - Device Tree file for the Sunchip CX-A99 board.
>> + *
>> + * Copyright (C) 2017 Rask Ingemann Lambertsen <rask@formelder.dk>
>> + *
>> + * 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.
>> + */
>> +
>> +/*
>> + * The Sunchip CX-A99 board is found in several similar Android media
>> + * players, such as:
>> + *
>> + * Instabox Fantasy A8 (no external antenna)
>> + * Jesurun CS-Q8 (ships with larger remote control)
>> + * Jesurun Maxone
>> + * Rikomagic (RKM) MK80/MK80LE
>> + * Tronsmart Draco AW80 Meta/Telos
>> + *
>> + * See <URL:https://linux-sunxi.org/Sunchip_CX-A99> for more information.
>> + */
>> +
>> +/dts-v1/;
>> +#include "sun9i-a80.dtsi"
>> +
>> +#include <dt-bindings/gpio/gpio.h>
>> +
>> +/ {
>> +     model = "Sunchip CX-A99";
>> +     compatible = "sunchip,cx-a99", "allwinner,sun9i-a80";
>> +
>> +     aliases {
>> +             serial0 = &uart0;
>> +     };
>> +
>> +     chosen {
>> +             stdout-path = "serial0:115200n8";
>> +     };
>> +
>> +     leds {
>> +             compatible = "gpio-leds";
>> +
>> +             blue {
>> +                     gpios = <&pio 6 10 GPIO_ACTIVE_HIGH>;   /* PG10 */
>> +                     label = "cx-a99:blue:status";
>> +             };
>> +
>> +             red {
>> +                     gpios = <&pio 6 11 GPIO_ACTIVE_HIGH>;   /* PG11 */
>> +                     label = "cx-a99:red:status";
>> +             };
>> +     };
>> +
>> +     powerseq_wifi: powerseq-wifi {
>> +             compatible = "mmc-pwrseq-simple";
>> +             clocks = <&ac100_rtc 1>;
>> +             clock-names = "ext_clock";
>> +             reset-gpios = <&r_pio 1 0 GPIO_ACTIVE_LOW>;     /* PM0 */
>> +             post-power-on-delay-ms = <1>;   /* Minimum 2 cycles. */
>> +     };
>> +
>> +     /* 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?
>
>> +     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
>
>> +             regulator-always-on;
>
> And it shouldn't be always on. The USB driver will enable it if needs
> be.
>
>> +     };
>> +
>> +     /*
>> +      * 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?
>
>> +     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.
>
>> +     };
>> +
>> +     /* 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.
>
>> +     };
>> +};
>> +
>> +&ehci0 {
>> +     status = "okay";
>> +};
>> +
>> +&ehci2 {
>> +     status = "okay";
>> +};
>> +
>> +/*
>> + * SD card slot. Although the GPIO pin for card detection is listed as capable
>> + * of generating interrupts in the "A80 User Manual", this doesn't work for
>> + * some unknown reason, so poll the GPIO for card detection. This is also what
>> + * the vendor sys_config.fex file specifies.
>> + */
>> +&mmc0 {
>> +     bus-width = <4>;
>> +     cd-gpios = <&pio 7 17 GPIO_ACTIVE_LOW>; /* PH17 */
>> +     broken-cd;                              /* Poll. */
>> +     pinctrl-names = "default";
>> +     pinctrl-0 = <&mmc0_pins>;
>> +     vmmc-supply = <&reg_dcdce>;
>> +     status = "okay";
>> +};
>> +
>> +/* Ampak AP6335 IEEE 802.11 a/b/g/n/ac "Wifi". */
>
> Why "wifi" ? It's not implementing true wifi?
>
>> +&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.
>
>> +     mmc-pwrseq = <&powerseq_wifi>;
>> +     status = "okay";
>> +};
>> +
>> +/* On-board eMMC card. */
>> +&mmc2 {
>> +     bus-width = <8>;
>> +     non-removable;
>> +     pinctrl-names = "default";
>> +     pinctrl-0 = <&mmc2_8bit_pins>;
>> +     vmmc-supply = <&reg_dcdce>;
>> +     status = "okay";
>> +};
>> +
>> +&osc32k {
>> +     clocks = <&ac100_rtc 0>;
>> +};
>> +
>> +&r_ir {
>> +     status = "okay";
>> +};
>> +
>> +&r_rsb {
>> +     status = "okay";
>> +
>> +     ac100: codec@e89 {
>> +             compatible = "x-powers,ac100";
>> +             reg = <0xe89>;
>> +
>> +             ac100_codec: codec {
>> +                     compatible = "x-powers,ac100-codec";
>> +                     interrupt-parent = <&r_pio>;
>> +                     interrupts = <0 9 IRQ_TYPE_LEVEL_LOW>;  /* PL9 */
>> +                     #clock-cells = <0>;
>> +                     clock-output-names = "4M_adda";
>> +             };
>> +
>> +             ac100_rtc: rtc {
>> +                     compatible = "x-powers,ac100-rtc";
>> +                     interrupt-parent = <&nmi_intc>;
>> +                     interrupts = <0 IRQ_TYPE_LEVEL_LOW>;
>> +                     clocks = <&ac100_codec>;
>> +                     #clock-cells = <1>;
>> +                     clock-output-names = "cko1_rtc",
>> +                                          "cko2_rtc",
>> +                                          "cko3_rtc";
>> +             };
>> +     };
>> +
>> +     pmic@745 {
>> +             compatible = "x-powers,axp808", "x-powers,axp806";

As you mentioned elsewhere, they are not really compatible.
You should drop the latter compatible.

>> +             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.

Hmm, there isn't one for the AXP806 either.

Some more info about the regulator names:

>> +
>> +             /* In comments: Initial setup from vendor sys_config.fex. */
>> +             regulators {
>> +                     /* 3.0 V (enabled). */
>
> This might be disabled though.
>
>> +                     reg_aldo1: aldo1 {
>> +                             regulator-boot-on;
>> +                             regulator-min-microvolt = <3000000>;
>> +                             regulator-max-microvolt = <3000000>;
>> +                             regulator-name = "vcc-3v0";
>> +                     };
>> +
>> +                     /* 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.
>
> Something like vcc-wifi would be enough her.

It should be vddio-wifi. Looking at the pin groups it names, it likely
just drives the I/O pins on both ends.

>
>> +                     };
>> +
>> +                     /* 2.5 V (enabled). */
>> +                     reg_aldo3: aldo3 {
>> +                             regulator-boot-on;
>> +                             regulator-min-microvolt = <2500000>;
>> +                             regulator-max-microvolt = <2500000>;
>> +                             regulator-name = "vcc-pa-gmac2v5";
>
> vcc-gmac

vcc-pa or vddio-gmac. 2.5V is for RGMII I/O. vcc-gmac is the "sw"
regulator @ 3.3V.

>
>> +                     };
>> +
>> +                     /* 1.8 V (enabled). */
>> +                     reg_bldo1: bldo1 {
>> +                             regulator-always-on;    /* Hang if disabled */
>> +                             regulator-min-microvolt = <1700000>;
>> +                             regulator-max-microvolt = <1900000>;
>> +                             regulator-name = "vdd18-dll-vcc18-pll";
>
> vdd-dll

PLLs and DLLs are different though. Maybe vcc-pll-dll?

>
>> +                     };
>> +
>> +                     /* 0.9 V (enabled). */
>> +                     reg_bldo2: bldo2 {
>> +                             regulator-always-on;    /* Hang if disabled */
>> +                             regulator-min-microvolt = < 800000>;
>> +                             regulator-max-microvolt = <1100000>;
>> +                             regulator-name = "vdd-cpus";
>> +                     };
>> +
>> +                     /* 1.2 V (disabled). */
>> +                     reg_bldo3: bldo3 {
>> +                             regulator-min-microvolt = <1100000>;
>> +                             regulator-max-microvolt = <1300000>;
>> +                             regulator-name = "vcc12-hsic";
>
> vcc-hsic

vcc12-hsic is actually the name listed in the SoC datasheet.

>
>> +                     };
>> +
>> +                     /* 1.1 V (enabled). */
>> +                     reg_bldo4: bldo4 {
>> +                             regulator-boot-on;
>> +                             regulator-min-microvolt = < 800000>;
>> +                             regulator-max-microvolt = <1100000>;
>> +                             regulator-name = "vdd09-hdmi";
>
> vdd-hdmi

vdd09-hdmi is actually the name listed in the SoC datasheet.

>
>> +                     };
>> +
>> +                     /* 3.3 V (enabled). PLx pins control some regulators. */
>> +                     reg_cldo1: cldo1 {
>> +                             regulator-always-on;
>> +                             regulator-min-microvolt = <3300000>;
>> +                             regulator-max-microvolt = <3300000>;
>> +                             regulator-name = "vcc-pl-led";
>
> vcc-led, etc...

vcc-pl is probably better... One can figure out the LEDs are connected to the
PL group and maybe realize they are powered this way. Not that easy the other
way around.

I used really long names for the other 2 A80 boards though.


Regards
ChenYu

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

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

[Rask Ingemann Lambertsen]

Let me just answer the questions about the regulator names and get back to
you about your other points.

On Fri, Feb 10, 2017 at 05:22:21PM +0800, Chen-Yu Tsai wrote:
> On Fri, Feb 10, 2017 at 4:59 PM, Maxime Ripard
> <maxime.ripard@free-electrons.com> wrote:
> > Hi,
> >
> > On Thu, Feb 09, 2017 at 12:34:06AM +0100, Rask Ingemann Lambertsen wrote:
[...]
> >> +                     /* 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.
> >
> > Something like vcc-wifi would be enough her.
> 
> It should be vddio-wifi. Looking at the pin groups it names, it likely
> just drives the I/O pins on both ends.

Sys_config.fex lists is as supplying the audio codec as well, so
"vddio-wifi-codec" perhaps?

> >> +                     /* 2.5 V (enabled). */
> >> +                     reg_aldo3: aldo3 {
> >> +                             regulator-boot-on;
> >> +                             regulator-min-microvolt = <2500000>;
> >> +                             regulator-max-microvolt = <2500000>;
> >> +                             regulator-name = "vcc-pa-gmac2v5";
> >
> > vcc-gmac
> 
> vcc-pa or vddio-gmac. 2.5V is for RGMII I/O. vcc-gmac is the "sw"
> regulator @ 3.3V.

I think nothing else is connected to pin group A, so I'll call it
"vddio-gmac".

> >> +                     /* 1.8 V (enabled). */
> >> +                     reg_bldo1: bldo1 {
> >> +                             regulator-always-on;    /* Hang if disabled */
> >> +                             regulator-min-microvolt = <1700000>;
> >> +                             regulator-max-microvolt = <1900000>;
> >> +                             regulator-name = "vdd18-dll-vcc18-pll";
> >
> > vdd-dll
> 
> PLLs and DLLs are different though. Maybe vcc-pll-dll?

The SoC data sheet uses the names "VDD18-DLL" and "VCC18-PLL". However, it's
quite likely that more of the SoC's 1.8 V consumers are connected to bldo1,
as I see no other likely 1.8 V supply for them. I'll call it "vdd18".

> >> +                     /* 3.3 V (enabled). PLx pins control some regulators. */
> >> +                     reg_cldo1: cldo1 {
> >> +                             regulator-always-on;
> >> +                             regulator-min-microvolt = <3300000>;
> >> +                             regulator-max-microvolt = <3300000>;
> >> +                             regulator-name = "vcc-pl-led";
> >
> > vcc-led, etc...
> 
> vcc-pl is probably better... One can figure out the LEDs are connected to the
> PL group and maybe realize they are powered this way. Not that easy the other
> way around.

The LEDs are controlled by PG10 and PG11 but get their power from cldo1,
which is connected to the (common) anode of the red and blue LED. Cldo1 also
supplies the SoCs "VCC-PL", which powers pin group L that controls the USB
2.0 Vbus and vdd-cpub regulators. Pin group L has nothing to do with the
LEDs. Thus I think the regulator name is appropriate in this case.

-- 
Rask Ingemann Lambertsen

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

[Rask Ingemann Lambertsen]

On Fri, Feb 10, 2017 at 05:22:21PM +0800, Chen-Yu Tsai wrote:
> On Fri, Feb 10, 2017 at 4:59 PM, Maxime Ripard
> <maxime.ripard@free-electrons.com> wrote:
> > Hi,
> >
> > On Thu, Feb 09, 2017 at 12:34:06AM +0100, Rask Ingemann Lambertsen wrote:
[...]
> >> 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
[...]
> >> +     pmic@745 {
> >> +             compatible = "x-powers,axp808", "x-powers,axp806";
> 
> As you mentioned elsewhere, they are not really compatible.
> You should drop the latter compatible.

The compatibility is good enough that it works fine with the driver that
went into kernel 4.9 and 4.10. Using this device tree file, I have built and
booted 4.9.9 and 4.10-rc7 kernels as per the instructions here;
https://linux-sunxi.org/Sunchip_CX-A99#Linux_kernel
Likewise any other AXP806 driver which doesn't touch the register at address
0xff will work fine.

So, the disadvantage of removing the "x-powers,axp806" compatible is that
it breaks on two kernel versions where it works fine with the compatible.
What is the advantage of removing the "x-powers,axp806" compatible?

As to touching the register at 0xff, it would be interesting to know more
precisely what goes wrong. The symptom is these messages in the log:

[    3.209573] axp20x-rsb sunxi-rsb-745: AXP20x variant AXP806 found
[    3.210153] axp20x-rsb sunxi-rsb-745: Failed to set masks in 0x40: -5
[    3.210178] axp20x-rsb sunxi-rsb-745: failed to add irq chip: -5
[    3.210306] axp20x-rsb: probe of sunxi-rsb-745 failed with error -5

I haven't yet looked further into what happens at the RSB bus level.

-- 
Rask Ingemann Lambertsen

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

[Maxime Ripard]

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

On Wed, Feb 15, 2017 at 12:35:39AM +0100, Rask Ingemann Lambertsen wrote:
> On Fri, Feb 10, 2017 at 05:22:21PM +0800, Chen-Yu Tsai wrote:
> > On Fri, Feb 10, 2017 at 4:59 PM, Maxime Ripard
> > <maxime.ripard@free-electrons.com> wrote:
> > > Hi,
> > >
> > > On Thu, Feb 09, 2017 at 12:34:06AM +0100, Rask Ingemann Lambertsen wrote:
> [...]
> > >> 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
> [...]
> > >> +     pmic@745 {
> > >> +             compatible = "x-powers,axp808", "x-powers,axp806";
> > 
> > As you mentioned elsewhere, they are not really compatible.
> > You should drop the latter compatible.
> 
> The compatibility is good enough that it works fine with the driver that
> went into kernel 4.9 and 4.10. Using this device tree file, I have built and
> booted 4.9.9 and 4.10-rc7 kernels as per the instructions here;
> https://linux-sunxi.org/Sunchip_CX-A99#Linux_kernel
> Likewise any other AXP806 driver which doesn't touch the register at address
> 0xff will work fine.
>
> So, the disadvantage of removing the "x-powers,axp806" compatible is that
> it breaks on two kernel versions where it works fine with the compatible.
> What is the advantage of removing the "x-powers,axp806" compatible?

This works fine for the features you tested, which are the
regulators. All the other features of the PMIC are untested, and might
or might not change, so you really don't know about the overall
capability of the PMIC with another model.

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 --]

AXP808 vs. AXP806 debugged, no difference? (Was: [PATCH v6 5/5] ARM: dts: sun9i: Initial support for the Sunchip CX-A99 board)

[Rask Ingemann Lambertsen]

In summary: I'll propose a new property "extended-address" or so to the
existing "x-powers,axp806" compatible, because the axp808 does seem to have
the register at 0xff after all, it just needs a value of 0 instead of the 16
which drivers/mfd/axp20x.c as of recently is now writing unconditionally
to 0xff. A new "x-powers,axp808" compatible and new AXP808_ID is therefore
not needed. This should make for a simpler and smaller patch.

On Wed, Feb 15, 2017 at 12:35:39AM +0100, Rask Ingemann Lambertsen wrote:
> As to touching the register at 0xff, it would be interesting to know more
> precisely what goes wrong. The symptom is these messages in the log:
> 
> [    3.209573] axp20x-rsb sunxi-rsb-745: AXP20x variant AXP806 found
> [    3.210153] axp20x-rsb sunxi-rsb-745: Failed to set masks in 0x40: -5
> [    3.210178] axp20x-rsb sunxi-rsb-745: failed to add irq chip: -5
> [    3.210306] axp20x-rsb: probe of sunxi-rsb-745 failed with error -5
> 
> I haven't yet looked further into what happens at the RSB bus level.

With the patch below applied for debugging purposes and using the
"xpowers,axp806" compatible, I get:

[    3.209955] axp20x-rsb sunxi-rsb-745: AXP20x variant AXP806 found
[    3.210065] axp20x-rsb sunxi-rsb-745: Chip ID read 1 = 0x60
[    3.210119] axp20x-rsb sunxi-rsb-745: Reg addr ext = 0x0
[    3.210171] axp20x-rsb sunxi-rsb-745: Bus addr ext = 0x0
[    3.210381] axp20x-rsb sunxi-rsb-745: Chip ID read 2 = 0x60
(16 written to register 0xff here)
[    3.210570] sunxi-rsb 8003400.i2c: RSB transfer data error
[    3.210596] sunxi-rsb 8003400.i2c: RSB error reading dev 58 register 3 length 1.
[    3.210625] axp20x-rsb sunxi-rsb-745: Chip ID read 3 = 0xffffffff
(0 written to register 0xff here)
[    3.210828] axp20x-rsb sunxi-rsb-745: Chip ID read 4 = 0x60

I.e. register 0xff (Reg addr ext) and 0xfe (Bus addr ext) exist with a
default of 0. After writing 16 to register 0xff, the chip ID (register 3)
can not be read, but writing 0 to register 0xff makes the AXP808 respond
again and the board boots up as normal.

Also, can someone with an AXP806 confirm that it does use the same chip ID
of 0x60 as the AXP808?

diff --git a/drivers/mfd/axp20x.c b/drivers/mfd/axp20x.c
index 125b470..db557f3 100644
--- a/drivers/mfd/axp20x.c
+++ b/drivers/mfd/axp20x.c
@@ -910,9 +910,40 @@ int axp20x_device_probe(struct axp20x_dev *axp20x)
 	 * address.
 	 */
 	if (axp20x->variant == AXP806_ID)
+	{
+		int dummy;
+
+		dummy = -1;
+		regmap_read(axp20x->regmap, AXP806_CHIP_ID, &dummy);
+		dev_info(axp20x->dev, "Chip ID read 1 = 0x%x\n", dummy);
+
+		regmap_read(axp20x->regmap, AXP806_REG_ADDR_EXT, &dummy);
+		dev_info(axp20x->dev, "Reg addr ext = 0x%x\n", dummy);
+		regmap_read(axp20x->regmap, AXP806_BUS_ADDR_EXT, &dummy);
+		dev_info(axp20x->dev, "Bus addr ext = 0x%x\n", dummy);
+
+		regmap_reinit_cache(axp20x->regmap, axp20x->regmap_cfg);
+		dummy = -1;
+		regmap_read(axp20x->regmap, AXP806_CHIP_ID, &dummy);
+		dev_info(axp20x->dev, "Chip ID read 2 = 0x%x\n", dummy);
+
 		regmap_write(axp20x->regmap, AXP806_REG_ADDR_EXT,
 			     AXP806_REG_ADDR_EXT_ADDR_SLAVE_MODE);
 
+		regmap_reinit_cache(axp20x->regmap, axp20x->regmap_cfg);
+		dummy = -1;
+		regmap_read(axp20x->regmap, AXP806_CHIP_ID, &dummy);
+		dev_info(axp20x->dev, "Chip ID read 3 = 0x%x\n", dummy);
+
+		regmap_write(axp20x->regmap, AXP806_REG_ADDR_EXT,
+			     0);
+
+		regmap_reinit_cache(axp20x->regmap, axp20x->regmap_cfg);
+		dummy = -1;
+		regmap_read(axp20x->regmap, AXP806_CHIP_ID, &dummy);
+		dev_info(axp20x->dev, "Chip ID read 4 = 0x%x\n", dummy);
+	}
+
 	ret = regmap_add_irq_chip(axp20x->regmap, axp20x->irq,
 			  IRQF_ONESHOT | IRQF_SHARED | axp20x->irq_flags,
 			   -1, axp20x->regmap_irq_chip, &axp20x->regmap_irqc);
diff --git a/drivers/bus/sunxi-rsb.c b/drivers/bus/sunxi-rsb.c
index 795c9d9..85e4926 100644
--- a/drivers/bus/sunxi-rsb.c
+++ b/drivers/bus/sunxi-rsb.c
@@ -349,6 +349,10 @@ static int sunxi_rsb_read(struct sunxi_rsb *rsb, u8 rtaddr, u8 addr,
 unlock:
 	mutex_unlock(&rsb->lock);
 
+	if (ret)
+		dev_dbg(rsb->dev, "RSB error reading dev %u register %u length %u.\n",
+			rtaddr, addr, (unsigned int) len);
+
 	return ret;
 }
 
@@ -386,6 +390,10 @@ static int sunxi_rsb_write(struct sunxi_rsb *rsb, u8 rtaddr, u8 addr,
 
 	mutex_unlock(&rsb->lock);
 
+	if (ret)
+		dev_dbg(rsb->dev, "RSB error writing dev %u register %u length %u.\n",
+			rtaddr, addr, (unsigned int) len);
+
 	return ret;
 }
 
diff --git a/drivers/bus/Makefile b/drivers/bus/Makefile
index cc6364b..4b81226 100644
--- a/drivers/bus/Makefile
+++ b/drivers/bus/Makefile
@@ -24,3 +24,4 @@ obj-$(CONFIG_UNIPHIER_SYSTEM_BUS)	+= uniphier-system-bus.o
 obj-$(CONFIG_VEXPRESS_CONFIG)	+= vexpress-config.o
 
 obj-$(CONFIG_DA8XX_MSTPRI)	+= da8xx-mstpri.o
+CFLAGS_sunxi-rsb.o		+= -DDEBUG -DEBUG

-- 
Rask Ingemann Lambertsen

Re: AXP808 vs. AXP806 debugged, no difference? (Was: [PATCH v6 5/5] ARM: dts: sun9i: Initial support for the Sunchip CX-A99 board)

[Chen-Yu Tsai]

On Fri, Feb 17, 2017 at 5:16 AM, Rask Ingemann Lambertsen
<rask@formelder.dk> wrote:
> In summary: I'll propose a new property "extended-address" or so to the
> existing "x-powers,axp806" compatible, because the axp808 does seem to have
> the register at 0xff after all, it just needs a value of 0 instead of the 16
> which drivers/mfd/axp20x.c as of recently is now writing unconditionally
> to 0xff. A new "x-powers,axp808" compatible and new AXP808_ID is therefore
> not needed. This should make for a simpler and smaller patch.

It might be the same core as the AXP806, though without a datasheet to actually
confirm it, it might be best to stick to a new compatible.

However, we could add a new property "x-powers,master-mode". If the driver
sees it, it would clear bit 4, instead of setting it. The hardware bit is
latched from the external pin, so this would be per board.

Regards
ChenYu

> On Wed, Feb 15, 2017 at 12:35:39AM +0100, Rask Ingemann Lambertsen wrote:
>> As to touching the register at 0xff, it would be interesting to know more
>> precisely what goes wrong. The symptom is these messages in the log:
>>
>> [    3.209573] axp20x-rsb sunxi-rsb-745: AXP20x variant AXP806 found
>> [    3.210153] axp20x-rsb sunxi-rsb-745: Failed to set masks in 0x40: -5
>> [    3.210178] axp20x-rsb sunxi-rsb-745: failed to add irq chip: -5
>> [    3.210306] axp20x-rsb: probe of sunxi-rsb-745 failed with error -5
>>
>> I haven't yet looked further into what happens at the RSB bus level.
>
> With the patch below applied for debugging purposes and using the
> "xpowers,axp806" compatible, I get:
>
> [    3.209955] axp20x-rsb sunxi-rsb-745: AXP20x variant AXP806 found
> [    3.210065] axp20x-rsb sunxi-rsb-745: Chip ID read 1 = 0x60
> [    3.210119] axp20x-rsb sunxi-rsb-745: Reg addr ext = 0x0
> [    3.210171] axp20x-rsb sunxi-rsb-745: Bus addr ext = 0x0
> [    3.210381] axp20x-rsb sunxi-rsb-745: Chip ID read 2 = 0x60
> (16 written to register 0xff here)
> [    3.210570] sunxi-rsb 8003400.i2c: RSB transfer data error
> [    3.210596] sunxi-rsb 8003400.i2c: RSB error reading dev 58 register 3 length 1.
> [    3.210625] axp20x-rsb sunxi-rsb-745: Chip ID read 3 = 0xffffffff
> (0 written to register 0xff here)
> [    3.210828] axp20x-rsb sunxi-rsb-745: Chip ID read 4 = 0x60
>
> I.e. register 0xff (Reg addr ext) and 0xfe (Bus addr ext) exist with a
> default of 0. After writing 16 to register 0xff, the chip ID (register 3)
> can not be read, but writing 0 to register 0xff makes the AXP808 respond
> again and the board boots up as normal.
>
> Also, can someone with an AXP806 confirm that it does use the same chip ID
> of 0x60 as the AXP808?
>
> diff --git a/drivers/mfd/axp20x.c b/drivers/mfd/axp20x.c
> index 125b470..db557f3 100644
> --- a/drivers/mfd/axp20x.c
> +++ b/drivers/mfd/axp20x.c
> @@ -910,9 +910,40 @@ int axp20x_device_probe(struct axp20x_dev *axp20x)
>          * address.
>          */
>         if (axp20x->variant == AXP806_ID)
> +       {
> +               int dummy;
> +
> +               dummy = -1;
> +               regmap_read(axp20x->regmap, AXP806_CHIP_ID, &dummy);
> +               dev_info(axp20x->dev, "Chip ID read 1 = 0x%x\n", dummy);
> +
> +               regmap_read(axp20x->regmap, AXP806_REG_ADDR_EXT, &dummy);
> +               dev_info(axp20x->dev, "Reg addr ext = 0x%x\n", dummy);
> +               regmap_read(axp20x->regmap, AXP806_BUS_ADDR_EXT, &dummy);
> +               dev_info(axp20x->dev, "Bus addr ext = 0x%x\n", dummy);
> +
> +               regmap_reinit_cache(axp20x->regmap, axp20x->regmap_cfg);
> +               dummy = -1;
> +               regmap_read(axp20x->regmap, AXP806_CHIP_ID, &dummy);
> +               dev_info(axp20x->dev, "Chip ID read 2 = 0x%x\n", dummy);
> +
>                 regmap_write(axp20x->regmap, AXP806_REG_ADDR_EXT,
>                              AXP806_REG_ADDR_EXT_ADDR_SLAVE_MODE);
>
> +               regmap_reinit_cache(axp20x->regmap, axp20x->regmap_cfg);
> +               dummy = -1;
> +               regmap_read(axp20x->regmap, AXP806_CHIP_ID, &dummy);
> +               dev_info(axp20x->dev, "Chip ID read 3 = 0x%x\n", dummy);
> +
> +               regmap_write(axp20x->regmap, AXP806_REG_ADDR_EXT,
> +                            0);
> +
> +               regmap_reinit_cache(axp20x->regmap, axp20x->regmap_cfg);
> +               dummy = -1;
> +               regmap_read(axp20x->regmap, AXP806_CHIP_ID, &dummy);
> +               dev_info(axp20x->dev, "Chip ID read 4 = 0x%x\n", dummy);
> +       }
> +
>         ret = regmap_add_irq_chip(axp20x->regmap, axp20x->irq,
>                           IRQF_ONESHOT | IRQF_SHARED | axp20x->irq_flags,
>                            -1, axp20x->regmap_irq_chip, &axp20x->regmap_irqc);
> diff --git a/drivers/bus/sunxi-rsb.c b/drivers/bus/sunxi-rsb.c
> index 795c9d9..85e4926 100644
> --- a/drivers/bus/sunxi-rsb.c
> +++ b/drivers/bus/sunxi-rsb.c
> @@ -349,6 +349,10 @@ static int sunxi_rsb_read(struct sunxi_rsb *rsb, u8 rtaddr, u8 addr,
>  unlock:
>         mutex_unlock(&rsb->lock);
>
> +       if (ret)
> +               dev_dbg(rsb->dev, "RSB error reading dev %u register %u length %u.\n",
> +                       rtaddr, addr, (unsigned int) len);
> +
>         return ret;
>  }
>
> @@ -386,6 +390,10 @@ static int sunxi_rsb_write(struct sunxi_rsb *rsb, u8 rtaddr, u8 addr,
>
>         mutex_unlock(&rsb->lock);
>
> +       if (ret)
> +               dev_dbg(rsb->dev, "RSB error writing dev %u register %u length %u.\n",
> +                       rtaddr, addr, (unsigned int) len);
> +
>         return ret;
>  }
>
> diff --git a/drivers/bus/Makefile b/drivers/bus/Makefile
> index cc6364b..4b81226 100644
> --- a/drivers/bus/Makefile
> +++ b/drivers/bus/Makefile
> @@ -24,3 +24,4 @@ obj-$(CONFIG_UNIPHIER_SYSTEM_BUS)     += uniphier-system-bus.o
>  obj-$(CONFIG_VEXPRESS_CONFIG)  += vexpress-config.o
>
>  obj-$(CONFIG_DA8XX_MSTPRI)     += da8xx-mstpri.o
> +CFLAGS_sunxi-rsb.o             += -DDEBUG -DEBUG
>
> --
> Rask Ingemann Lambertsen

Re: AXP808 vs. AXP806 debugged, no difference? (Was: [PATCH v6 5/5] ARM: dts: sun9i: Initial support for the Sunchip CX-A99 board)

[Rask Ingemann Lambertsen]

On Fri, Feb 17, 2017 at 11:08:59AM +0800, Chen-Yu Tsai wrote:
> On Fri, Feb 17, 2017 at 5:16 AM, Rask Ingemann Lambertsen
> <rask@formelder.dk> wrote:
> > In summary: I'll propose a new property "extended-address" or so to the
> > existing "x-powers,axp806" compatible, because the axp808 does seem to have
> > the register at 0xff after all, it just needs a value of 0 instead of the 16
> > which drivers/mfd/axp20x.c as of recently is now writing unconditionally
> > to 0xff. A new "x-powers,axp808" compatible and new AXP808_ID is therefore
> > not needed. This should make for a simpler and smaller patch.
> 
> It might be the same core as the AXP806, though without a datasheet to actually
> confirm it, it might be best to stick to a new compatible.

Yes, an extra compatible is cheap. Also, it occurs to me that either could
turn out to have features that the other one doesn't.

FWIW, both are 56-pin devices going by board photos. I'd say they differ in
the power-on voltages. On the AXP808, I've measured (in FEL mode, assuming
that the BROM doesn't have any PMIC code at all):

aldo1	3.0 V
aldo2	off
aldo3	off
bldo1	1.8 V
bldo2	0.9 V	(here the AXP806 gives 1.8 V for DLL and PLL)
bldo3	off
bldo4	?	(haven't found it yet)
cldo1	3.3 V
cldo2	off
cldo3	off
dcdca	0.9 V
dcdcb	1.5 V
dcdcc	0.9 V
dcdcd	0.9 V
dcdce	3.3 V	(AXP806 set to 2.1 V in Optimus and Cubieboard4 dts)
sw0	?	(haven't found it yet)

It would also explain why they were given the same chip ID, as from driver
point of view, they behave in the same way. The Allwinner kernel tree also
uses the same driver (confusingly called axp15) for both.

> However, we could add a new property "x-powers,master-mode". If the driver
> sees it, it would clear bit 4, instead of setting it. The hardware bit is
> latched from the external pin, so this would be per board.

Patch coming.

-- 
Rask Ingemann Lambertsen

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

[Rask Ingemann Lambertsen]

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

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

[Chen-Yu Tsai]

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

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

[Maxime Ripard]

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

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

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

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

[Rask Ingemann Lambertsen]

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

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

[Maxime Ripard]

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

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

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