[PATCH 00/11] ARM: berlin: refactor chip and system controllers

[PATCH 00/11] ARM: berlin: refactor chip and system controllers

[Antoine Tenart]

Hi,

Marvell Berlin SoCs have a chip control register set providing several
individual registers dealing with various controllers (pinctrl, reset,
clk). This chip controller is described by a single DT node since the
individual registers are spread among the chip control register bank.

Marvell Berlin also have a system control register set providing several
individual registers for pinctrl or adc.

Prior to this series, there was no proper way of probing properly the
devices using one of the chip and system controller nodes. The pinctrl
driver was probed using the DT and the reset controller had an initcall.

This series aims to handle these two nodes correctly, by introducing an
MFD driver, the Berlin controller, to register all the devices described
by the chip and system controller nodes. Syscon is also used to provide
a regmap to the sub-node drivers, so that they can access the registers
of the controllers safely. In addition, the clock driver will need the
regmap early during the boot, so syscon is a good choice here.

Reworks were done in the Berlin pin controller and in the Berlin reset
driver, to take the changes into account (proper compatibles, syscon
helpers, regmap use).

The actual clock driver still works with these modifications, and a
dedicated series will follow to convert the Berlin clock drivers to use
the regmap provided by syscon.

Tested on the Marvell BG2Q DMP.

Thanks!

Antoine

Antoine Tenart (11):
  mfd: add the Berlin controller driver
  Documentation: bindings: add the Berlin controller documentation
  ARM: berlin: select MFD_BERLIN_CTRL
  reset: berlin: convert to a platform driver
  Documentation: bindings: move the Berlin reset documentation
  pinctrl: berlin: use the regmap provided by syscon
  pinctrl: berlin: use proper compatibles
  Documentation: bindings: move the Berlin pinctrl documentation
  ARM: berlin: rework chip and system controller nodes for BG2
  ARM: berlin: rework chip and system controller nodes for BG2CD
  ARM: berlin: rework chip and system controller nodes for BG2Q

 .../devicetree/bindings/arm/marvell,berlin.txt     |  76 ---------
 .../devicetree/bindings/mfd/berlin-ctrl.txt        |  43 +++++
 .../devicetree/bindings/pinctrl/berlin,pinctrl.txt |  43 +++++
 .../devicetree/bindings/reset/berlin,reset.txt     |  23 +++
 arch/arm/boot/dts/berlin2.dtsi                     |  50 +++---
 arch/arm/boot/dts/berlin2cd.dtsi                   |  32 ++--
 arch/arm/boot/dts/berlin2q.dtsi                    |  70 ++++----
 arch/arm/mach-berlin/Kconfig                       |   1 +
 drivers/mfd/Kconfig                                |   5 +
 drivers/mfd/Makefile                               |   2 +
 drivers/mfd/berlin-ctrl.c                          | 180 +++++++++++++++++++++
 drivers/pinctrl/berlin/berlin-bg2.c                |  26 +--
 drivers/pinctrl/berlin/berlin-bg2cd.c              |  26 +--
 drivers/pinctrl/berlin/berlin-bg2q.c               |  26 +--
 drivers/pinctrl/berlin/berlin.c                    |   9 +-
 drivers/reset/reset-berlin.c                       |  69 +++-----
 16 files changed, 429 insertions(+), 252 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/mfd/berlin-ctrl.txt
 create mode 100644 Documentation/devicetree/bindings/pinctrl/berlin,pinctrl.txt
 create mode 100644 Documentation/devicetree/bindings/reset/berlin,reset.txt
 create mode 100644 drivers/mfd/berlin-ctrl.c

-- 
2.3.0

[PATCH 00/11] ARM: berlin: refactor chip and system controllers

[Antoine Tenart]

Hi,

Marvell Berlin SoCs have a chip control register set providing several
individual registers dealing with various controllers (pinctrl, reset,
clk). This chip controller is described by a single DT node since the
individual registers are spread among the chip control register bank.

Marvell Berlin also have a system control register set providing several
individual registers for pinctrl or adc.

Prior to this series, there was no proper way of probing properly the
devices using one of the chip and system controller nodes. The pinctrl
driver was probed using the DT and the reset controller had an initcall.

This series aims to handle these two nodes correctly, by introducing an
MFD driver, the Berlin controller, to register all the devices described
by the chip and system controller nodes. Syscon is also used to provide
a regmap to the sub-node drivers, so that they can access the registers
of the controllers safely. In addition, the clock driver will need the
regmap early during the boot, so syscon is a good choice here.

Reworks were done in the Berlin pin controller and in the Berlin reset
driver, to take the changes into account (proper compatibles, syscon
helpers, regmap use).

The actual clock driver still works with these modifications, and a
dedicated series will follow to convert the Berlin clock drivers to use
the regmap provided by syscon.

Tested on the Marvell BG2Q DMP.

Thanks!

Antoine

Antoine Tenart (11):
  mfd: add the Berlin controller driver
  Documentation: bindings: add the Berlin controller documentation
  ARM: berlin: select MFD_BERLIN_CTRL
  reset: berlin: convert to a platform driver
  Documentation: bindings: move the Berlin reset documentation
  pinctrl: berlin: use the regmap provided by syscon
  pinctrl: berlin: use proper compatibles
  Documentation: bindings: move the Berlin pinctrl documentation
  ARM: berlin: rework chip and system controller nodes for BG2
  ARM: berlin: rework chip and system controller nodes for BG2CD
  ARM: berlin: rework chip and system controller nodes for BG2Q

 .../devicetree/bindings/arm/marvell,berlin.txt     |  76 ---------
 .../devicetree/bindings/mfd/berlin-ctrl.txt        |  43 +++++
 .../devicetree/bindings/pinctrl/berlin,pinctrl.txt |  43 +++++
 .../devicetree/bindings/reset/berlin,reset.txt     |  23 +++
 arch/arm/boot/dts/berlin2.dtsi                     |  50 +++---
 arch/arm/boot/dts/berlin2cd.dtsi                   |  32 ++--
 arch/arm/boot/dts/berlin2q.dtsi                    |  70 ++++----
 arch/arm/mach-berlin/Kconfig                       |   1 +
 drivers/mfd/Kconfig                                |   5 +
 drivers/mfd/Makefile                               |   2 +
 drivers/mfd/berlin-ctrl.c                          | 180 +++++++++++++++++++++
 drivers/pinctrl/berlin/berlin-bg2.c                |  26 +--
 drivers/pinctrl/berlin/berlin-bg2cd.c              |  26 +--
 drivers/pinctrl/berlin/berlin-bg2q.c               |  26 +--
 drivers/pinctrl/berlin/berlin.c                    |   9 +-
 drivers/reset/reset-berlin.c                       |  69 +++-----
 16 files changed, 429 insertions(+), 252 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/mfd/berlin-ctrl.txt
 create mode 100644 Documentation/devicetree/bindings/pinctrl/berlin,pinctrl.txt
 create mode 100644 Documentation/devicetree/bindings/reset/berlin,reset.txt
 create mode 100644 drivers/mfd/berlin-ctrl.c

-- 
2.3.0

[PATCH 01/11] mfd: add the Berlin controller driver

[Antoine Tenart]

Marvell Berlin SoC have two nodes providing multiple devices (clk,
pinctrl, reset). While until now these drivers were initialized using
initcalls, this wasn't a proper solution. This mfd driver will be
responsible of adding these devices, to be probed properly.

It currently registers the pin controllers and the reset drivers for
BG2, BG2CD and BG2Q in the soc and system controller nodes.

Signed-off-by: Antoine Tenart <antoine.tenart@free-electrons.com>
---
 drivers/mfd/Kconfig       |   5 ++
 drivers/mfd/Makefile      |   2 +
 drivers/mfd/berlin-ctrl.c | 180 ++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 187 insertions(+)
 create mode 100644 drivers/mfd/berlin-ctrl.c

diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig
index 2e6b7311fabc..eda6dbec02ff 100644
--- a/drivers/mfd/Kconfig
+++ b/drivers/mfd/Kconfig
@@ -840,6 +840,11 @@ config STMPE_SPI
 	  This is used to enable SPI interface of STMPE
 endmenu
 
+config MFD_BERLIN_CTRL
+	bool
+	select MFD_CORE
+	select MFD_SYSCON
+
 config MFD_STA2X11
 	bool "STMicroelectronics STA2X11"
 	depends on STA2X11
diff --git a/drivers/mfd/Makefile b/drivers/mfd/Makefile
index 53467e211381..adf60e85df20 100644
--- a/drivers/mfd/Makefile
+++ b/drivers/mfd/Makefile
@@ -179,3 +179,5 @@ obj-$(CONFIG_MFD_DLN2)		+= dln2.o
 
 intel-soc-pmic-objs		:= intel_soc_pmic_core.o intel_soc_pmic_crc.o
 obj-$(CONFIG_INTEL_SOC_PMIC)	+= intel-soc-pmic.o
+
+obj-$(CONFIG_MFD_BERLIN_CTRL)	+= berlin-ctrl.o
diff --git a/drivers/mfd/berlin-ctrl.c b/drivers/mfd/berlin-ctrl.c
new file mode 100644
index 000000000000..e3ce6f069f16
--- /dev/null
+++ b/drivers/mfd/berlin-ctrl.c
@@ -0,0 +1,180 @@
+/*
+ * Copyright (C) 2015 Marvell Technology Group Ltd.
+ *
+ * Antoine Tenart <antoine.tenart@free-electrons.com>
+ *
+ * This file is licensed under the terms of the GNU General Public
+ * License version 2. This program is licensed "as is" without any
+ * warranty of any kind, whether express or implied.
+ */
+
+#include <linux/mfd/core.h>
+#include <linux/module.h>
+#include <linux/of.h>
+
+struct berlin_ctrl_priv {
+	const struct mfd_cell	*devs;
+	unsigned		ndevs;
+};
+
+/*
+ * BG2 devices
+ */
+static const struct mfd_cell berlin2_ctrl_chip_ctrl_subdevs[] = {
+	{
+		.name		= "berlin2-soc-pinctrl",
+		.of_compatible	= "marvell,berlin2-soc-pinctrl",
+	},
+	{
+		.name		= "berlin2-reset",
+		.of_compatible	= "marvell,berlin2-reset",
+	},
+};
+
+static const struct mfd_cell berlin2_ctrl_system_ctrl_subdevs[] = {
+	{
+		.name		= "berlin2-system-pinctrl",
+		.of_compatible	= "marvell,berlin2-system-pinctrl",
+	},
+};
+
+static const struct berlin_ctrl_priv berlin2_ctrl_chip_ctrl_data = {
+	.devs	= berlin2_ctrl_chip_ctrl_subdevs,
+	.ndevs	= ARRAY_SIZE(berlin2_ctrl_chip_ctrl_subdevs),
+};
+
+static const struct berlin_ctrl_priv berlin2_ctrl_system_data = {
+	.devs	= berlin2_ctrl_system_ctrl_subdevs,
+	.ndevs	= ARRAY_SIZE(berlin2_ctrl_system_ctrl_subdevs),
+};
+
+
+/*
+ * BG2CD devices
+ */
+static const struct mfd_cell berlin2cd_ctrl_chip_ctrl_subdevs[] = {
+	{
+		.name		= "berlin2cd-soc-pinctrl",
+		.of_compatible	= "marvell,berlin2cd-soc-pinctrl",
+	},
+	{
+		.name		= "berlin2-reset",
+		.of_compatible	= "marvell,berlin2-reset",
+	},
+};
+
+static const struct mfd_cell berlin2cd_ctrl_system_ctrl_subdevs[] = {
+	{
+		.name		= "berlin2cd-system-pinctrl",
+		.of_compatible	= "marvell,berlin2cd-system-pinctrl",
+	},
+};
+
+static const struct berlin_ctrl_priv berlin2cd_ctrl_chip_ctrl_data = {
+	.devs	= berlin2cd_ctrl_chip_ctrl_subdevs,
+	.ndevs	= ARRAY_SIZE(berlin2cd_ctrl_chip_ctrl_subdevs),
+};
+
+static const struct berlin_ctrl_priv berlin2cd_ctrl_system_data = {
+	.devs	= berlin2cd_ctrl_system_ctrl_subdevs,
+	.ndevs	= ARRAY_SIZE(berlin2cd_ctrl_system_ctrl_subdevs),
+};
+
+/*
+ * BG2Q devices
+ */
+static const struct mfd_cell berlin2q_ctrl_chip_ctrl_subdevs[] = {
+	{
+		.name		= "berlin2q-soc-pinctrl",
+		.of_compatible	= "marvell,berlin2q-soc-pinctrl",
+	},
+	{
+		.name		= "berlin2-reset",
+		.of_compatible	= "marvell,berlin2-reset",
+	},
+};
+
+static const struct mfd_cell berlin2q_ctrl_system_ctrl_subdevs[] = {
+	{
+		.name		= "berlin2q-system-pinctrl",
+		.of_compatible	= "marvell,berlin2q-system-pinctrl",
+	},
+};
+
+static const struct berlin_ctrl_priv berlin2q_ctrl_chip_ctrl_data = {
+	.devs	= berlin2q_ctrl_chip_ctrl_subdevs,
+	.ndevs	= ARRAY_SIZE(berlin2q_ctrl_chip_ctrl_subdevs),
+};
+
+static const struct berlin_ctrl_priv berlin2q_ctrl_system_data = {
+	.devs	= berlin2q_ctrl_system_ctrl_subdevs,
+	.ndevs	= ARRAY_SIZE(berlin2q_ctrl_system_ctrl_subdevs),
+};
+
+
+static const struct of_device_id berlin_ctrl_of_match[] = {
+	/* BG2 */
+	{
+		.compatible	= "marvell,berlin2-chip-ctrl",
+		.data		= &berlin2_ctrl_chip_ctrl_data,
+	},
+	{
+		.compatible	= "marvell,berlin2-system-ctrl",
+		.data		= &berlin2_ctrl_system_data,
+	},
+	/* BG2CD */
+	{
+		.compatible	= "marvell,berlin2cd-chip-ctrl",
+		.data		= &berlin2cd_ctrl_chip_ctrl_data,
+	},
+	{
+		.compatible	= "marvell,berlin2cd-system-ctrl",
+		.data		= &berlin2cd_ctrl_system_data,
+	},
+	/* BG2Q */
+	{
+		.compatible	= "marvell,berlin2q-chip-ctrl",
+		.data		= &berlin2q_ctrl_chip_ctrl_data,
+	},
+	{
+		.compatible	= "marvell,berlin2q-system-ctrl",
+		.data		= &berlin2q_ctrl_system_data,
+	},
+	{ },
+};
+MODULE_DEVICE_TABLE(of, berlin_ctrl_of_match);
+
+static int berlin_ctrl_probe(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	const struct of_device_id *match;
+	const struct berlin_ctrl_priv *priv;
+	int ret;
+
+	match = of_match_node(berlin_ctrl_of_match, dev->of_node);
+	if (!match)
+		return -EINVAL;
+
+	priv = match->data;
+
+	ret = mfd_add_devices(dev, 0, priv->devs, priv->ndevs, NULL, -1, NULL);
+	if (ret) {
+		dev_err(dev, "Failed to add devices: %d\n", ret);
+		return ret;
+	}
+
+	return 0;
+}
+
+static struct platform_driver berlin_ctrl_driver = {
+	.probe	= berlin_ctrl_probe,
+	.driver	= {
+		.name = "berlin-ctrl",
+		.of_match_table = berlin_ctrl_of_match,
+	},
+};
+module_platform_driver(berlin_ctrl_driver);
+
+MODULE_AUTHOR("Antoine Tenart <antoine.tenart@free-electrons.com>");
+MODULE_DESCRIPTION("Marvell Berlin controller driver (mfd)");
+MODULE_LICENSE("GPL");
-- 
2.3.0

[PATCH 01/11] mfd: add the Berlin controller driver

[Antoine Tenart]

Marvell Berlin SoC have two nodes providing multiple devices (clk,
pinctrl, reset). While until now these drivers were initialized using
initcalls, this wasn't a proper solution. This mfd driver will be
responsible of adding these devices, to be probed properly.

It currently registers the pin controllers and the reset drivers for
BG2, BG2CD and BG2Q in the soc and system controller nodes.

Signed-off-by: Antoine Tenart <antoine.tenart@free-electrons.com>
---
 drivers/mfd/Kconfig       |   5 ++
 drivers/mfd/Makefile      |   2 +
 drivers/mfd/berlin-ctrl.c | 180 ++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 187 insertions(+)
 create mode 100644 drivers/mfd/berlin-ctrl.c

diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig
index 2e6b7311fabc..eda6dbec02ff 100644
--- a/drivers/mfd/Kconfig
+++ b/drivers/mfd/Kconfig
@@ -840,6 +840,11 @@ config STMPE_SPI
 	  This is used to enable SPI interface of STMPE
 endmenu
 
+config MFD_BERLIN_CTRL
+	bool
+	select MFD_CORE
+	select MFD_SYSCON
+
 config MFD_STA2X11
 	bool "STMicroelectronics STA2X11"
 	depends on STA2X11
diff --git a/drivers/mfd/Makefile b/drivers/mfd/Makefile
index 53467e211381..adf60e85df20 100644
--- a/drivers/mfd/Makefile
+++ b/drivers/mfd/Makefile
@@ -179,3 +179,5 @@ obj-$(CONFIG_MFD_DLN2)		+= dln2.o
 
 intel-soc-pmic-objs		:= intel_soc_pmic_core.o intel_soc_pmic_crc.o
 obj-$(CONFIG_INTEL_SOC_PMIC)	+= intel-soc-pmic.o
+
+obj-$(CONFIG_MFD_BERLIN_CTRL)	+= berlin-ctrl.o
diff --git a/drivers/mfd/berlin-ctrl.c b/drivers/mfd/berlin-ctrl.c
new file mode 100644
index 000000000000..e3ce6f069f16
--- /dev/null
+++ b/drivers/mfd/berlin-ctrl.c
@@ -0,0 +1,180 @@
+/*
+ * Copyright (C) 2015 Marvell Technology Group Ltd.
+ *
+ * Antoine Tenart <antoine.tenart@free-electrons.com>
+ *
+ * This file is licensed under the terms of the GNU General Public
+ * License version 2. This program is licensed "as is" without any
+ * warranty of any kind, whether express or implied.
+ */
+
+#include <linux/mfd/core.h>
+#include <linux/module.h>
+#include <linux/of.h>
+
+struct berlin_ctrl_priv {
+	const struct mfd_cell	*devs;
+	unsigned		ndevs;
+};
+
+/*
+ * BG2 devices
+ */
+static const struct mfd_cell berlin2_ctrl_chip_ctrl_subdevs[] = {
+	{
+		.name		= "berlin2-soc-pinctrl",
+		.of_compatible	= "marvell,berlin2-soc-pinctrl",
+	},
+	{
+		.name		= "berlin2-reset",
+		.of_compatible	= "marvell,berlin2-reset",
+	},
+};
+
+static const struct mfd_cell berlin2_ctrl_system_ctrl_subdevs[] = {
+	{
+		.name		= "berlin2-system-pinctrl",
+		.of_compatible	= "marvell,berlin2-system-pinctrl",
+	},
+};
+
+static const struct berlin_ctrl_priv berlin2_ctrl_chip_ctrl_data = {
+	.devs	= berlin2_ctrl_chip_ctrl_subdevs,
+	.ndevs	= ARRAY_SIZE(berlin2_ctrl_chip_ctrl_subdevs),
+};
+
+static const struct berlin_ctrl_priv berlin2_ctrl_system_data = {
+	.devs	= berlin2_ctrl_system_ctrl_subdevs,
+	.ndevs	= ARRAY_SIZE(berlin2_ctrl_system_ctrl_subdevs),
+};
+
+
+/*
+ * BG2CD devices
+ */
+static const struct mfd_cell berlin2cd_ctrl_chip_ctrl_subdevs[] = {
+	{
+		.name		= "berlin2cd-soc-pinctrl",
+		.of_compatible	= "marvell,berlin2cd-soc-pinctrl",
+	},
+	{
+		.name		= "berlin2-reset",
+		.of_compatible	= "marvell,berlin2-reset",
+	},
+};
+
+static const struct mfd_cell berlin2cd_ctrl_system_ctrl_subdevs[] = {
+	{
+		.name		= "berlin2cd-system-pinctrl",
+		.of_compatible	= "marvell,berlin2cd-system-pinctrl",
+	},
+};
+
+static const struct berlin_ctrl_priv berlin2cd_ctrl_chip_ctrl_data = {
+	.devs	= berlin2cd_ctrl_chip_ctrl_subdevs,
+	.ndevs	= ARRAY_SIZE(berlin2cd_ctrl_chip_ctrl_subdevs),
+};
+
+static const struct berlin_ctrl_priv berlin2cd_ctrl_system_data = {
+	.devs	= berlin2cd_ctrl_system_ctrl_subdevs,
+	.ndevs	= ARRAY_SIZE(berlin2cd_ctrl_system_ctrl_subdevs),
+};
+
+/*
+ * BG2Q devices
+ */
+static const struct mfd_cell berlin2q_ctrl_chip_ctrl_subdevs[] = {
+	{
+		.name		= "berlin2q-soc-pinctrl",
+		.of_compatible	= "marvell,berlin2q-soc-pinctrl",
+	},
+	{
+		.name		= "berlin2-reset",
+		.of_compatible	= "marvell,berlin2-reset",
+	},
+};
+
+static const struct mfd_cell berlin2q_ctrl_system_ctrl_subdevs[] = {
+	{
+		.name		= "berlin2q-system-pinctrl",
+		.of_compatible	= "marvell,berlin2q-system-pinctrl",
+	},
+};
+
+static const struct berlin_ctrl_priv berlin2q_ctrl_chip_ctrl_data = {
+	.devs	= berlin2q_ctrl_chip_ctrl_subdevs,
+	.ndevs	= ARRAY_SIZE(berlin2q_ctrl_chip_ctrl_subdevs),
+};
+
+static const struct berlin_ctrl_priv berlin2q_ctrl_system_data = {
+	.devs	= berlin2q_ctrl_system_ctrl_subdevs,
+	.ndevs	= ARRAY_SIZE(berlin2q_ctrl_system_ctrl_subdevs),
+};
+
+
+static const struct of_device_id berlin_ctrl_of_match[] = {
+	/* BG2 */
+	{
+		.compatible	= "marvell,berlin2-chip-ctrl",
+		.data		= &berlin2_ctrl_chip_ctrl_data,
+	},
+	{
+		.compatible	= "marvell,berlin2-system-ctrl",
+		.data		= &berlin2_ctrl_system_data,
+	},
+	/* BG2CD */
+	{
+		.compatible	= "marvell,berlin2cd-chip-ctrl",
+		.data		= &berlin2cd_ctrl_chip_ctrl_data,
+	},
+	{
+		.compatible	= "marvell,berlin2cd-system-ctrl",
+		.data		= &berlin2cd_ctrl_system_data,
+	},
+	/* BG2Q */
+	{
+		.compatible	= "marvell,berlin2q-chip-ctrl",
+		.data		= &berlin2q_ctrl_chip_ctrl_data,
+	},
+	{
+		.compatible	= "marvell,berlin2q-system-ctrl",
+		.data		= &berlin2q_ctrl_system_data,
+	},
+	{ },
+};
+MODULE_DEVICE_TABLE(of, berlin_ctrl_of_match);
+
+static int berlin_ctrl_probe(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	const struct of_device_id *match;
+	const struct berlin_ctrl_priv *priv;
+	int ret;
+
+	match = of_match_node(berlin_ctrl_of_match, dev->of_node);
+	if (!match)
+		return -EINVAL;
+
+	priv = match->data;
+
+	ret = mfd_add_devices(dev, 0, priv->devs, priv->ndevs, NULL, -1, NULL);
+	if (ret) {
+		dev_err(dev, "Failed to add devices: %d\n", ret);
+		return ret;
+	}
+
+	return 0;
+}
+
+static struct platform_driver berlin_ctrl_driver = {
+	.probe	= berlin_ctrl_probe,
+	.driver	= {
+		.name = "berlin-ctrl",
+		.of_match_table = berlin_ctrl_of_match,
+	},
+};
+module_platform_driver(berlin_ctrl_driver);
+
+MODULE_AUTHOR("Antoine Tenart <antoine.tenart@free-electrons.com>");
+MODULE_DESCRIPTION("Marvell Berlin controller driver (mfd)");
+MODULE_LICENSE("GPL");
-- 
2.3.0

[PATCH 02/11] Documentation: bindings: add the Berlin controller documentation

[Antoine Tenart]

A Berlin controller mfd driver was added to handle correctly the chip
and system controller DT nodes. Its purpose is to register devices
configured by one of the controller nodes. This adds the corresponding
bindings documentation.

Signed-off-by: Antoine Tenart <antoine.tenart@free-electrons.com>
---
 .../devicetree/bindings/arm/marvell,berlin.txt     | 29 ---------------
 .../devicetree/bindings/mfd/berlin-ctrl.txt        | 43 ++++++++++++++++++++++
 2 files changed, 43 insertions(+), 29 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/mfd/berlin-ctrl.txt

diff --git a/Documentation/devicetree/bindings/arm/marvell,berlin.txt b/Documentation/devicetree/bindings/arm/marvell,berlin.txt
index a99eb9eb14c0..cb280bc8de80 100644
--- a/Documentation/devicetree/bindings/arm/marvell,berlin.txt
+++ b/Documentation/devicetree/bindings/arm/marvell,berlin.txt
@@ -40,35 +40,6 @@ cpu-ctrl@f7dd0000 {
 	reg = <0xf7dd0000 0x10000>;
 };
 
-* Marvell Berlin2 chip control binding
-
-Marvell Berlin SoCs have a chip control register set providing several
-individual registers dealing with pinmux, padmux, clock, reset, and secondary
-CPU boot address. Unfortunately, the individual registers are spread among the
-chip control registers, so there should be a single DT node only providing the
-different functions which are described below.
-
-Required properties:
-- compatible: shall be one of
-	"marvell,berlin2-chip-ctrl" for BG2
-	"marvell,berlin2cd-chip-ctrl" for BG2CD
-	"marvell,berlin2q-chip-ctrl" for BG2Q
-- reg: address and length of following register sets for
-  BG2/BG2CD: chip control register set
-  BG2Q: chip control register set and cpu pll registers
-
-* Marvell Berlin2 system control binding
-
-Marvell Berlin SoCs have a system control register set providing several
-individual registers dealing with pinmux, padmux, and reset.
-
-Required properties:
-- compatible: should be one of
-	"marvell,berlin2-system-ctrl" for BG2
-	"marvell,berlin2cd-system-ctrl" for BG2CD
-	"marvell,berlin2q-system-ctrl" for BG2Q
-- reg: address and length of the system control register set
-
 * Clock provider binding
 
 As clock related registers are spread among the chip control registers, the
diff --git a/Documentation/devicetree/bindings/mfd/berlin-ctrl.txt b/Documentation/devicetree/bindings/mfd/berlin-ctrl.txt
new file mode 100644
index 000000000000..eed8ed437a1c
--- /dev/null
+++ b/Documentation/devicetree/bindings/mfd/berlin-ctrl.txt
@@ -0,0 +1,43 @@
+* Marvell Berlin Multi-Functional Device controller
+
+Marvell Berlin SoCs have a chip control register set providing several
+individual registers dealing with pinmux, padmux, clock, reset, and secondary
+CPU boot address. Unfortunately, the individual registers are spread among the
+chip control registers, so there should be a single DT node only providing the
+different functions which are described below.
+
+Marvell Berlin SoCs also have a system control register set providing several
+individual registers dealing with pinmux, padmux, and reset.
+
+The Berlin controller mfd handles the chip and the system controllers on Berlin
+SoCs. It exposes various devices:
+- pinctrl, see Documentation/devicetree/bindings/pinctrl/berlin,pinctrl.txt
+- reset, see Documentation/devicetree/bindings/reset/berlin,reset.txt
+
+Required properties:
+- compatible:
+	* the first value should be one of:
+		"marvell,berlin2-chip-ctrl",
+		"marvell,berlin2-system-ctrl",
+		"marvell,berlin2cd-chip-ctrl",
+		"marvell,berlin2cd-system-ctrl",
+		"marvell,berlin2q-chip-ctrl",
+		"marvell,berlin2q-system-ctrl"
+	* the second value must be "syscon"
+- reg: the controller register range
+
+Example:
+
+chip: chip-controller@f7ea0000 {
+	compatible = "marvell,berlin2q-chip-ctrl", "syscon";
+	reg = <0xf7ea0000 0x400>;
+
+	/* sub-device nodes */
+};
+
+sysctrl: system-controller@f7fcd000 {
+	compatible = "marvell,berlin2q-system-ctrl", "syscon";
+	reg = <0xf7fcd000 0x100>;
+
+	/* sub-device nodes */
+};
-- 
2.3.0

[PATCH 02/11] Documentation: bindings: add the Berlin controller documentation

[Antoine Tenart]

A Berlin controller mfd driver was added to handle correctly the chip
and system controller DT nodes. Its purpose is to register devices
configured by one of the controller nodes. This adds the corresponding
bindings documentation.

Signed-off-by: Antoine Tenart <antoine.tenart@free-electrons.com>
---
 .../devicetree/bindings/arm/marvell,berlin.txt     | 29 ---------------
 .../devicetree/bindings/mfd/berlin-ctrl.txt        | 43 ++++++++++++++++++++++
 2 files changed, 43 insertions(+), 29 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/mfd/berlin-ctrl.txt

diff --git a/Documentation/devicetree/bindings/arm/marvell,berlin.txt b/Documentation/devicetree/bindings/arm/marvell,berlin.txt
index a99eb9eb14c0..cb280bc8de80 100644
--- a/Documentation/devicetree/bindings/arm/marvell,berlin.txt
+++ b/Documentation/devicetree/bindings/arm/marvell,berlin.txt
@@ -40,35 +40,6 @@ cpu-ctrl at f7dd0000 {
 	reg = <0xf7dd0000 0x10000>;
 };
 
-* Marvell Berlin2 chip control binding
-
-Marvell Berlin SoCs have a chip control register set providing several
-individual registers dealing with pinmux, padmux, clock, reset, and secondary
-CPU boot address. Unfortunately, the individual registers are spread among the
-chip control registers, so there should be a single DT node only providing the
-different functions which are described below.
-
-Required properties:
-- compatible: shall be one of
-	"marvell,berlin2-chip-ctrl" for BG2
-	"marvell,berlin2cd-chip-ctrl" for BG2CD
-	"marvell,berlin2q-chip-ctrl" for BG2Q
-- reg: address and length of following register sets for
-  BG2/BG2CD: chip control register set
-  BG2Q: chip control register set and cpu pll registers
-
-* Marvell Berlin2 system control binding
-
-Marvell Berlin SoCs have a system control register set providing several
-individual registers dealing with pinmux, padmux, and reset.
-
-Required properties:
-- compatible: should be one of
-	"marvell,berlin2-system-ctrl" for BG2
-	"marvell,berlin2cd-system-ctrl" for BG2CD
-	"marvell,berlin2q-system-ctrl" for BG2Q
-- reg: address and length of the system control register set
-
 * Clock provider binding
 
 As clock related registers are spread among the chip control registers, the
diff --git a/Documentation/devicetree/bindings/mfd/berlin-ctrl.txt b/Documentation/devicetree/bindings/mfd/berlin-ctrl.txt
new file mode 100644
index 000000000000..eed8ed437a1c
--- /dev/null
+++ b/Documentation/devicetree/bindings/mfd/berlin-ctrl.txt
@@ -0,0 +1,43 @@
+* Marvell Berlin Multi-Functional Device controller
+
+Marvell Berlin SoCs have a chip control register set providing several
+individual registers dealing with pinmux, padmux, clock, reset, and secondary
+CPU boot address. Unfortunately, the individual registers are spread among the
+chip control registers, so there should be a single DT node only providing the
+different functions which are described below.
+
+Marvell Berlin SoCs also have a system control register set providing several
+individual registers dealing with pinmux, padmux, and reset.
+
+The Berlin controller mfd handles the chip and the system controllers on Berlin
+SoCs. It exposes various devices:
+- pinctrl, see Documentation/devicetree/bindings/pinctrl/berlin,pinctrl.txt
+- reset, see Documentation/devicetree/bindings/reset/berlin,reset.txt
+
+Required properties:
+- compatible:
+	* the first value should be one of:
+		"marvell,berlin2-chip-ctrl",
+		"marvell,berlin2-system-ctrl",
+		"marvell,berlin2cd-chip-ctrl",
+		"marvell,berlin2cd-system-ctrl",
+		"marvell,berlin2q-chip-ctrl",
+		"marvell,berlin2q-system-ctrl"
+	* the second value must be "syscon"
+- reg: the controller register range
+
+Example:
+
+chip: chip-controller at f7ea0000 {
+	compatible = "marvell,berlin2q-chip-ctrl", "syscon";
+	reg = <0xf7ea0000 0x400>;
+
+	/* sub-device nodes */
+};
+
+sysctrl: system-controller at f7fcd000 {
+	compatible = "marvell,berlin2q-system-ctrl", "syscon";
+	reg = <0xf7fcd000 0x100>;
+
+	/* sub-device nodes */
+};
-- 
2.3.0

[PATCH 03/11] ARM: berlin: select MFD_BERLIN_CTRL

[Antoine Tenart]

The Berlin controller mfd driver is responsible to probe multiple
sub-devices in both the soc and system controller nodes. It currently
registers the pin controller and the reset controller.

Select it for all Berlin SoCs.

Signed-off-by: Antoine Tenart <antoine.tenart@free-electrons.com>
---
 arch/arm/mach-berlin/Kconfig | 1 +
 1 file changed, 1 insertion(+)

diff --git a/arch/arm/mach-berlin/Kconfig b/arch/arm/mach-berlin/Kconfig
index 3e40a947f3ea..b6131b9889c1 100644
--- a/arch/arm/mach-berlin/Kconfig
+++ b/arch/arm/mach-berlin/Kconfig
@@ -6,6 +6,7 @@ menuconfig ARCH_BERLIN
 	select DW_APB_ICTL
 	select DW_APB_TIMER_OF
 	select GENERIC_IRQ_CHIP
+	select MFD_BERLIN_CTRL
 	select PINCTRL
 
 if ARCH_BERLIN
-- 
2.3.0

[PATCH 03/11] ARM: berlin: select MFD_BERLIN_CTRL

[Antoine Tenart]

The Berlin controller mfd driver is responsible to probe multiple
sub-devices in both the soc and system controller nodes. It currently
registers the pin controller and the reset controller.

Select it for all Berlin SoCs.

Signed-off-by: Antoine Tenart <antoine.tenart@free-electrons.com>
---
 arch/arm/mach-berlin/Kconfig | 1 +
 1 file changed, 1 insertion(+)

diff --git a/arch/arm/mach-berlin/Kconfig b/arch/arm/mach-berlin/Kconfig
index 3e40a947f3ea..b6131b9889c1 100644
--- a/arch/arm/mach-berlin/Kconfig
+++ b/arch/arm/mach-berlin/Kconfig
@@ -6,6 +6,7 @@ menuconfig ARCH_BERLIN
 	select DW_APB_ICTL
 	select DW_APB_TIMER_OF
 	select GENERIC_IRQ_CHIP
+	select MFD_BERLIN_CTRL
 	select PINCTRL
 
 if ARCH_BERLIN
-- 
2.3.0

[PATCH 04/11] reset: berlin: convert to a platform driver

[Antoine Tenart]

The Berlin reset controller was introduced without being a platform
driver because of a needed DT rework: the node describing the reset
controller also describes the pinctrl and clk controllers...

The DT issue being solved thanks to the addition of the Berlin
controller mfd driver, it is now possible to convert the Berlin reset
driver to a plaftorm driver.

Signed-off-by: Antoine Tenart <antoine.tenart@free-electrons.com>
---
 drivers/reset/reset-berlin.c | 69 +++++++++++++++++---------------------------
 1 file changed, 26 insertions(+), 43 deletions(-)

diff --git a/drivers/reset/reset-berlin.c b/drivers/reset/reset-berlin.c
index f8b48a13cf0b..fd44f872a212 100644
--- a/drivers/reset/reset-berlin.c
+++ b/drivers/reset/reset-berlin.c
@@ -12,10 +12,12 @@
 #include <linux/delay.h>
 #include <linux/io.h>
 #include <linux/module.h>
+#include <linux/mfd/syscon.h>
 #include <linux/of.h>
 #include <linux/of_address.h>
 #include <linux/platform_device.h>
 #include <linux/reset-controller.h>
+#include <linux/regmap.h>
 #include <linux/slab.h>
 #include <linux/types.h>
 
@@ -25,8 +27,7 @@
 	container_of((p), struct berlin_reset_priv, rcdev)
 
 struct berlin_reset_priv {
-	void __iomem			*base;
-	unsigned int			size;
+	struct regmap			*regmap;
 	struct reset_controller_dev	rcdev;
 };
 
@@ -37,7 +38,7 @@ static int berlin_reset_reset(struct reset_controller_dev *rcdev,
 	int offset = id >> 8;
 	int mask = BIT(id & 0x1f);
 
-	writel(mask, priv->base + offset);
+	regmap_write(priv->regmap, offset, mask);
 
 	/* let the reset be effective */
 	udelay(10);
@@ -52,7 +53,6 @@ static struct reset_control_ops berlin_reset_ops = {
 static int berlin_reset_xlate(struct reset_controller_dev *rcdev,
 			      const struct of_phandle_args *reset_spec)
 {
-	struct berlin_reset_priv *priv = to_berlin_reset_priv(rcdev);
 	unsigned offset, bit;
 
 	if (WARN_ON(reset_spec->args_count != rcdev->of_reset_n_cells))
@@ -61,71 +61,54 @@ static int berlin_reset_xlate(struct reset_controller_dev *rcdev,
 	offset = reset_spec->args[0];
 	bit = reset_spec->args[1];
 
-	if (offset >= priv->size)
-		return -EINVAL;
-
 	if (bit >= BERLIN_MAX_RESETS)
 		return -EINVAL;
 
 	return (offset << 8) | bit;
 }
 
-static int __berlin_reset_init(struct device_node *np)
+static int berlin2_reset_probe(struct platform_device *pdev)
 {
+	struct device_node *parent_np = of_get_parent(pdev->dev.of_node);
 	struct berlin_reset_priv *priv;
-	struct resource res;
-	resource_size_t size;
-	int ret;
 
-	priv = kzalloc(sizeof(*priv), GFP_KERNEL);
+	priv = devm_kzalloc(&pdev->dev, sizeof(*priv), GFP_KERNEL);
 	if (!priv)
 		return -ENOMEM;
 
-	ret = of_address_to_resource(np, 0, &res);
-	if (ret)
-		goto err;
-
-	size = resource_size(&res);
-	priv->base = ioremap(res.start, size);
-	if (!priv->base) {
-		ret = -ENOMEM;
-		goto err;
-	}
-	priv->size = size;
+	priv->regmap = syscon_node_to_regmap(parent_np);
+	of_node_put(parent_np);
+	if (IS_ERR(priv->regmap))
+		return PTR_ERR(priv->regmap);
 
 	priv->rcdev.owner = THIS_MODULE;
 	priv->rcdev.ops = &berlin_reset_ops;
-	priv->rcdev.of_node = np;
+	priv->rcdev.of_node = pdev->dev.of_node;
 	priv->rcdev.of_reset_n_cells = 2;
 	priv->rcdev.of_xlate = berlin_reset_xlate;
 
 	reset_controller_register(&priv->rcdev);
 
 	return 0;
-
-err:
-	kfree(priv);
-	return ret;
 }
 
 static const struct of_device_id berlin_reset_of_match[] __initconst = {
-	{ .compatible = "marvell,berlin2-chip-ctrl" },
-	{ .compatible = "marvell,berlin2cd-chip-ctrl" },
-	{ .compatible = "marvell,berlin2q-chip-ctrl" },
+	{ .compatible = "marvell,berlin2-reset" },
 	{ },
 };
+MODULE_DEVICE_TABLE(of, berlin_reset_of_match);
 
-static int __init berlin_reset_init(void)
-{
-	struct device_node *np;
-	int ret;
+static struct platform_driver berlin_reset_driver = {
+	.probe	= berlin2_reset_probe,
+	.driver	= {
+		.name = "berlin2-reset",
+		.of_match_table = berlin_reset_of_match,
+	},
 
-	for_each_matching_node(np, berlin_reset_of_match) {
-		ret = __berlin_reset_init(np);
-		if (ret)
-			return ret;
-	}
+};
+module_platform_driver(berlin_reset_driver);
 
-	return 0;
-}
-arch_initcall(berlin_reset_init);
+MODULE_AUTHOR("Antoine Tenart <antoine.tenart@free-electrons.com>");
+MODULE_AUTHOR("Sebastian Hesselbarth <sebastian.hesselbarth@gmail.com>");
+MODULE_DESCRIPTION("Marvell Berlin reset driver");
+MODULE_LICENSE("GPL");
-- 
2.3.0

[PATCH 04/11] reset: berlin: convert to a platform driver

[Antoine Tenart]

The Berlin reset controller was introduced without being a platform
driver because of a needed DT rework: the node describing the reset
controller also describes the pinctrl and clk controllers...

The DT issue being solved thanks to the addition of the Berlin
controller mfd driver, it is now possible to convert the Berlin reset
driver to a plaftorm driver.

Signed-off-by: Antoine Tenart <antoine.tenart@free-electrons.com>
---
 drivers/reset/reset-berlin.c | 69 +++++++++++++++++---------------------------
 1 file changed, 26 insertions(+), 43 deletions(-)

diff --git a/drivers/reset/reset-berlin.c b/drivers/reset/reset-berlin.c
index f8b48a13cf0b..fd44f872a212 100644
--- a/drivers/reset/reset-berlin.c
+++ b/drivers/reset/reset-berlin.c
@@ -12,10 +12,12 @@
 #include <linux/delay.h>
 #include <linux/io.h>
 #include <linux/module.h>
+#include <linux/mfd/syscon.h>
 #include <linux/of.h>
 #include <linux/of_address.h>
 #include <linux/platform_device.h>
 #include <linux/reset-controller.h>
+#include <linux/regmap.h>
 #include <linux/slab.h>
 #include <linux/types.h>
 
@@ -25,8 +27,7 @@
 	container_of((p), struct berlin_reset_priv, rcdev)
 
 struct berlin_reset_priv {
-	void __iomem			*base;
-	unsigned int			size;
+	struct regmap			*regmap;
 	struct reset_controller_dev	rcdev;
 };
 
@@ -37,7 +38,7 @@ static int berlin_reset_reset(struct reset_controller_dev *rcdev,
 	int offset = id >> 8;
 	int mask = BIT(id & 0x1f);
 
-	writel(mask, priv->base + offset);
+	regmap_write(priv->regmap, offset, mask);
 
 	/* let the reset be effective */
 	udelay(10);
@@ -52,7 +53,6 @@ static struct reset_control_ops berlin_reset_ops = {
 static int berlin_reset_xlate(struct reset_controller_dev *rcdev,
 			      const struct of_phandle_args *reset_spec)
 {
-	struct berlin_reset_priv *priv = to_berlin_reset_priv(rcdev);
 	unsigned offset, bit;
 
 	if (WARN_ON(reset_spec->args_count != rcdev->of_reset_n_cells))
@@ -61,71 +61,54 @@ static int berlin_reset_xlate(struct reset_controller_dev *rcdev,
 	offset = reset_spec->args[0];
 	bit = reset_spec->args[1];
 
-	if (offset >= priv->size)
-		return -EINVAL;
-
 	if (bit >= BERLIN_MAX_RESETS)
 		return -EINVAL;
 
 	return (offset << 8) | bit;
 }
 
-static int __berlin_reset_init(struct device_node *np)
+static int berlin2_reset_probe(struct platform_device *pdev)
 {
+	struct device_node *parent_np = of_get_parent(pdev->dev.of_node);
 	struct berlin_reset_priv *priv;
-	struct resource res;
-	resource_size_t size;
-	int ret;
 
-	priv = kzalloc(sizeof(*priv), GFP_KERNEL);
+	priv = devm_kzalloc(&pdev->dev, sizeof(*priv), GFP_KERNEL);
 	if (!priv)
 		return -ENOMEM;
 
-	ret = of_address_to_resource(np, 0, &res);
-	if (ret)
-		goto err;
-
-	size = resource_size(&res);
-	priv->base = ioremap(res.start, size);
-	if (!priv->base) {
-		ret = -ENOMEM;
-		goto err;
-	}
-	priv->size = size;
+	priv->regmap = syscon_node_to_regmap(parent_np);
+	of_node_put(parent_np);
+	if (IS_ERR(priv->regmap))
+		return PTR_ERR(priv->regmap);
 
 	priv->rcdev.owner = THIS_MODULE;
 	priv->rcdev.ops = &berlin_reset_ops;
-	priv->rcdev.of_node = np;
+	priv->rcdev.of_node = pdev->dev.of_node;
 	priv->rcdev.of_reset_n_cells = 2;
 	priv->rcdev.of_xlate = berlin_reset_xlate;
 
 	reset_controller_register(&priv->rcdev);
 
 	return 0;
-
-err:
-	kfree(priv);
-	return ret;
 }
 
 static const struct of_device_id berlin_reset_of_match[] __initconst = {
-	{ .compatible = "marvell,berlin2-chip-ctrl" },
-	{ .compatible = "marvell,berlin2cd-chip-ctrl" },
-	{ .compatible = "marvell,berlin2q-chip-ctrl" },
+	{ .compatible = "marvell,berlin2-reset" },
 	{ },
 };
+MODULE_DEVICE_TABLE(of, berlin_reset_of_match);
 
-static int __init berlin_reset_init(void)
-{
-	struct device_node *np;
-	int ret;
+static struct platform_driver berlin_reset_driver = {
+	.probe	= berlin2_reset_probe,
+	.driver	= {
+		.name = "berlin2-reset",
+		.of_match_table = berlin_reset_of_match,
+	},
 
-	for_each_matching_node(np, berlin_reset_of_match) {
-		ret = __berlin_reset_init(np);
-		if (ret)
-			return ret;
-	}
+};
+module_platform_driver(berlin_reset_driver);
 
-	return 0;
-}
-arch_initcall(berlin_reset_init);
+MODULE_AUTHOR("Antoine Tenart <antoine.tenart@free-electrons.com>");
+MODULE_AUTHOR("Sebastian Hesselbarth <sebastian.hesselbarth@gmail.com>");
+MODULE_DESCRIPTION("Marvell Berlin reset driver");
+MODULE_LICENSE("GPL");
-- 
2.3.0

[PATCH 05/11] Documentation: bindings: move the Berlin reset documentation

[Antoine Tenart]

The Berlin reset documentation was part of the Marvell Berlin SoC
documentation because the Berlin reset configuration was inside the chip
controller. With the recent rework of the chip and system controller
handling (now an MFD driver registers all sub-devices of the two soc and
system controller nodes and each device has its own sub-node), the
documentation of the Berlin reset driver can be moved to the generic
reset documentation directory.

Signed-off-by: Antoine Tenart <antoine.tenart@free-electrons.com>
---
 .../devicetree/bindings/arm/marvell,berlin.txt     | 10 ----------
 .../devicetree/bindings/reset/berlin,reset.txt     | 23 ++++++++++++++++++++++
 2 files changed, 23 insertions(+), 10 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/reset/berlin,reset.txt

diff --git a/Documentation/devicetree/bindings/arm/marvell,berlin.txt b/Documentation/devicetree/bindings/arm/marvell,berlin.txt
index cb280bc8de80..6413ce1fa485 100644
--- a/Documentation/devicetree/bindings/arm/marvell,berlin.txt
+++ b/Documentation/devicetree/bindings/arm/marvell,berlin.txt
@@ -77,21 +77,11 @@ Required subnode-properties:
 - groups: a list of strings describing the group names.
 - function: a string describing the function used to mux the groups.
 
-* Reset controller binding
-
-A reset controller is part of the chip control registers set. The chip control
-node also provides the reset. The register set is not at the same offset between
-Berlin SoCs.
-
-Required property:
-- #reset-cells: must be set to 2
-
 Example:
 
 chip: chip-control@ea0000 {
 	compatible = "marvell,berlin2-chip-ctrl";
 	#clock-cells = <1>;
-	#reset-cells = <2>;
 	reg = <0xea0000 0x400>;
 	clocks = <&refclk>, <&externaldev 0>;
 	clock-names = "refclk", "video_ext0";
diff --git a/Documentation/devicetree/bindings/reset/berlin,reset.txt b/Documentation/devicetree/bindings/reset/berlin,reset.txt
new file mode 100644
index 000000000000..514fee098b4b
--- /dev/null
+++ b/Documentation/devicetree/bindings/reset/berlin,reset.txt
@@ -0,0 +1,23 @@
+Marvell Berlin reset controller
+===============================
+
+Please also refer to reset.txt in this directory for common reset
+controller binding usage.
+
+The reset controller node must be a sub-node of the chip controller
+node on Berlin SoCs.
+
+Required properties:
+- compatible: should be "marvell,berlin2-reset"
+- #reset-cells: must be set to 2
+
+Example:
+
+chip_rst: reset {
+	compatible = "marvell,berlin2-reset";
+	#reset-cells = <2>;
+};
+
+&usb_phy0 {
+	resets = <&chip_rst 0x104 12>;
+};
-- 
2.3.0

[PATCH 05/11] Documentation: bindings: move the Berlin reset documentation

[Antoine Tenart]

The Berlin reset documentation was part of the Marvell Berlin SoC
documentation because the Berlin reset configuration was inside the chip
controller. With the recent rework of the chip and system controller
handling (now an MFD driver registers all sub-devices of the two soc and
system controller nodes and each device has its own sub-node), the
documentation of the Berlin reset driver can be moved to the generic
reset documentation directory.

Signed-off-by: Antoine Tenart <antoine.tenart@free-electrons.com>
---
 .../devicetree/bindings/arm/marvell,berlin.txt     | 10 ----------
 .../devicetree/bindings/reset/berlin,reset.txt     | 23 ++++++++++++++++++++++
 2 files changed, 23 insertions(+), 10 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/reset/berlin,reset.txt

diff --git a/Documentation/devicetree/bindings/arm/marvell,berlin.txt b/Documentation/devicetree/bindings/arm/marvell,berlin.txt
index cb280bc8de80..6413ce1fa485 100644
--- a/Documentation/devicetree/bindings/arm/marvell,berlin.txt
+++ b/Documentation/devicetree/bindings/arm/marvell,berlin.txt
@@ -77,21 +77,11 @@ Required subnode-properties:
 - groups: a list of strings describing the group names.
 - function: a string describing the function used to mux the groups.
 
-* Reset controller binding
-
-A reset controller is part of the chip control registers set. The chip control
-node also provides the reset. The register set is not at the same offset between
-Berlin SoCs.
-
-Required property:
-- #reset-cells: must be set to 2
-
 Example:
 
 chip: chip-control at ea0000 {
 	compatible = "marvell,berlin2-chip-ctrl";
 	#clock-cells = <1>;
-	#reset-cells = <2>;
 	reg = <0xea0000 0x400>;
 	clocks = <&refclk>, <&externaldev 0>;
 	clock-names = "refclk", "video_ext0";
diff --git a/Documentation/devicetree/bindings/reset/berlin,reset.txt b/Documentation/devicetree/bindings/reset/berlin,reset.txt
new file mode 100644
index 000000000000..514fee098b4b
--- /dev/null
+++ b/Documentation/devicetree/bindings/reset/berlin,reset.txt
@@ -0,0 +1,23 @@
+Marvell Berlin reset controller
+===============================
+
+Please also refer to reset.txt in this directory for common reset
+controller binding usage.
+
+The reset controller node must be a sub-node of the chip controller
+node on Berlin SoCs.
+
+Required properties:
+- compatible: should be "marvell,berlin2-reset"
+- #reset-cells: must be set to 2
+
+Example:
+
+chip_rst: reset {
+	compatible = "marvell,berlin2-reset";
+	#reset-cells = <2>;
+};
+
+&usb_phy0 {
+	resets = <&chip_rst 0x104 12>;
+};
-- 
2.3.0

[PATCH 06/11] pinctrl: berlin: use the regmap provided by syscon

[Antoine Tenart]

The Berlin pin controller nodes are now sub-nodes of the soc-controller
and the system-controller nodes. The register bank is managed by syscon,
which provides a regmap.

Remove the regmap setup from the Berlin pinctrl driver and use the one
provided by syscon.

Signed-off-by: Antoine Tenart <antoine.tenart@free-electrons.com>
---
 drivers/pinctrl/berlin/berlin-bg2.c   | 22 ----------------------
 drivers/pinctrl/berlin/berlin-bg2cd.c | 22 ----------------------
 drivers/pinctrl/berlin/berlin-bg2q.c  | 22 ----------------------
 drivers/pinctrl/berlin/berlin.c       |  9 ++++++---
 4 files changed, 6 insertions(+), 69 deletions(-)

diff --git a/drivers/pinctrl/berlin/berlin-bg2.c b/drivers/pinctrl/berlin/berlin-bg2.c
index b71a6fffef1b..368ec0b9b8ba 100644
--- a/drivers/pinctrl/berlin/berlin-bg2.c
+++ b/drivers/pinctrl/berlin/berlin-bg2.c
@@ -233,28 +233,6 @@ static int berlin2_pinctrl_probe(struct platform_device *pdev)
 {
 	const struct of_device_id *match =
 		of_match_device(berlin2_pinctrl_match, &pdev->dev);
-	struct regmap_config *rmconfig;
-	struct regmap *regmap;
-	struct resource *res;
-	void __iomem *base;
-
-	rmconfig = devm_kzalloc(&pdev->dev, sizeof(*rmconfig), GFP_KERNEL);
-	if (!rmconfig)
-		return -ENOMEM;
-
-	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
-	base = devm_ioremap_resource(&pdev->dev, res);
-	if (IS_ERR(base))
-		return PTR_ERR(base);
-
-	rmconfig->reg_bits = 32,
-	rmconfig->val_bits = 32,
-	rmconfig->reg_stride = 4,
-	rmconfig->max_register = resource_size(res);
-
-	regmap = devm_regmap_init_mmio(&pdev->dev, base, rmconfig);
-	if (IS_ERR(regmap))
-		return PTR_ERR(regmap);
 
 	return berlin_pinctrl_probe(pdev, match->data);
 }
diff --git a/drivers/pinctrl/berlin/berlin-bg2cd.c b/drivers/pinctrl/berlin/berlin-bg2cd.c
index 19ac5a22c947..6b9cae029ef7 100644
--- a/drivers/pinctrl/berlin/berlin-bg2cd.c
+++ b/drivers/pinctrl/berlin/berlin-bg2cd.c
@@ -176,28 +176,6 @@ static int berlin2cd_pinctrl_probe(struct platform_device *pdev)
 {
 	const struct of_device_id *match =
 		of_match_device(berlin2cd_pinctrl_match, &pdev->dev);
-	struct regmap_config *rmconfig;
-	struct regmap *regmap;
-	struct resource *res;
-	void __iomem *base;
-
-	rmconfig = devm_kzalloc(&pdev->dev, sizeof(*rmconfig), GFP_KERNEL);
-	if (!rmconfig)
-		return -ENOMEM;
-
-	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
-	base = devm_ioremap_resource(&pdev->dev, res);
-	if (IS_ERR(base))
-		return PTR_ERR(base);
-
-	rmconfig->reg_bits = 32,
-	rmconfig->val_bits = 32,
-	rmconfig->reg_stride = 4,
-	rmconfig->max_register = resource_size(res);
-
-	regmap = devm_regmap_init_mmio(&pdev->dev, base, rmconfig);
-	if (IS_ERR(regmap))
-		return PTR_ERR(regmap);
 
 	return berlin_pinctrl_probe(pdev, match->data);
 }
diff --git a/drivers/pinctrl/berlin/berlin-bg2q.c b/drivers/pinctrl/berlin/berlin-bg2q.c
index bd9662e57ad3..11aa10cc0e3e 100644
--- a/drivers/pinctrl/berlin/berlin-bg2q.c
+++ b/drivers/pinctrl/berlin/berlin-bg2q.c
@@ -395,28 +395,6 @@ static int berlin2q_pinctrl_probe(struct platform_device *pdev)
 {
 	const struct of_device_id *match =
 		of_match_device(berlin2q_pinctrl_match, &pdev->dev);
-	struct regmap_config *rmconfig;
-	struct regmap *regmap;
-	struct resource *res;
-	void __iomem *base;
-
-	rmconfig = devm_kzalloc(&pdev->dev, sizeof(*rmconfig), GFP_KERNEL);
-	if (!rmconfig)
-		return -ENOMEM;
-
-	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
-	base = devm_ioremap_resource(&pdev->dev, res);
-	if (IS_ERR(base))
-		return PTR_ERR(base);
-
-	rmconfig->reg_bits = 32,
-	rmconfig->val_bits = 32,
-	rmconfig->reg_stride = 4,
-	rmconfig->max_register = resource_size(res);
-
-	regmap = devm_regmap_init_mmio(&pdev->dev, base, rmconfig);
-	if (IS_ERR(regmap))
-		return PTR_ERR(regmap);
 
 	return berlin_pinctrl_probe(pdev, match->data);
 }
diff --git a/drivers/pinctrl/berlin/berlin.c b/drivers/pinctrl/berlin/berlin.c
index 7f0b0f93242b..2e3a8b8858ec 100644
--- a/drivers/pinctrl/berlin/berlin.c
+++ b/drivers/pinctrl/berlin/berlin.c
@@ -12,6 +12,7 @@
 
 #include <linux/io.h>
 #include <linux/module.h>
+#include <linux/mfd/syscon.h>
 #include <linux/of.h>
 #include <linux/of_address.h>
 #include <linux/of_device.h>
@@ -295,13 +296,15 @@ int berlin_pinctrl_probe(struct platform_device *pdev,
 			 const struct berlin_pinctrl_desc *desc)
 {
 	struct device *dev = &pdev->dev;
+	struct device_node *parent_np = of_get_parent(dev->of_node);
 	struct berlin_pinctrl *pctrl;
 	struct regmap *regmap;
 	int ret;
 
-	regmap = dev_get_regmap(&pdev->dev, NULL);
-	if (!regmap)
-		return -ENODEV;
+	regmap = syscon_node_to_regmap(parent_np);
+	of_node_put(parent_np);
+	if (IS_ERR(regmap))
+		return PTR_ERR(regmap);
 
 	pctrl = devm_kzalloc(dev, sizeof(*pctrl), GFP_KERNEL);
 	if (!pctrl)
-- 
2.3.0

[PATCH 06/11] pinctrl: berlin: use the regmap provided by syscon

[Antoine Tenart]

The Berlin pin controller nodes are now sub-nodes of the soc-controller
and the system-controller nodes. The register bank is managed by syscon,
which provides a regmap.

Remove the regmap setup from the Berlin pinctrl driver and use the one
provided by syscon.

Signed-off-by: Antoine Tenart <antoine.tenart@free-electrons.com>
---
 drivers/pinctrl/berlin/berlin-bg2.c   | 22 ----------------------
 drivers/pinctrl/berlin/berlin-bg2cd.c | 22 ----------------------
 drivers/pinctrl/berlin/berlin-bg2q.c  | 22 ----------------------
 drivers/pinctrl/berlin/berlin.c       |  9 ++++++---
 4 files changed, 6 insertions(+), 69 deletions(-)

diff --git a/drivers/pinctrl/berlin/berlin-bg2.c b/drivers/pinctrl/berlin/berlin-bg2.c
index b71a6fffef1b..368ec0b9b8ba 100644
--- a/drivers/pinctrl/berlin/berlin-bg2.c
+++ b/drivers/pinctrl/berlin/berlin-bg2.c
@@ -233,28 +233,6 @@ static int berlin2_pinctrl_probe(struct platform_device *pdev)
 {
 	const struct of_device_id *match =
 		of_match_device(berlin2_pinctrl_match, &pdev->dev);
-	struct regmap_config *rmconfig;
-	struct regmap *regmap;
-	struct resource *res;
-	void __iomem *base;
-
-	rmconfig = devm_kzalloc(&pdev->dev, sizeof(*rmconfig), GFP_KERNEL);
-	if (!rmconfig)
-		return -ENOMEM;
-
-	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
-	base = devm_ioremap_resource(&pdev->dev, res);
-	if (IS_ERR(base))
-		return PTR_ERR(base);
-
-	rmconfig->reg_bits = 32,
-	rmconfig->val_bits = 32,
-	rmconfig->reg_stride = 4,
-	rmconfig->max_register = resource_size(res);
-
-	regmap = devm_regmap_init_mmio(&pdev->dev, base, rmconfig);
-	if (IS_ERR(regmap))
-		return PTR_ERR(regmap);
 
 	return berlin_pinctrl_probe(pdev, match->data);
 }
diff --git a/drivers/pinctrl/berlin/berlin-bg2cd.c b/drivers/pinctrl/berlin/berlin-bg2cd.c
index 19ac5a22c947..6b9cae029ef7 100644
--- a/drivers/pinctrl/berlin/berlin-bg2cd.c
+++ b/drivers/pinctrl/berlin/berlin-bg2cd.c
@@ -176,28 +176,6 @@ static int berlin2cd_pinctrl_probe(struct platform_device *pdev)
 {
 	const struct of_device_id *match =
 		of_match_device(berlin2cd_pinctrl_match, &pdev->dev);
-	struct regmap_config *rmconfig;
-	struct regmap *regmap;
-	struct resource *res;
-	void __iomem *base;
-
-	rmconfig = devm_kzalloc(&pdev->dev, sizeof(*rmconfig), GFP_KERNEL);
-	if (!rmconfig)
-		return -ENOMEM;
-
-	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
-	base = devm_ioremap_resource(&pdev->dev, res);
-	if (IS_ERR(base))
-		return PTR_ERR(base);
-
-	rmconfig->reg_bits = 32,
-	rmconfig->val_bits = 32,
-	rmconfig->reg_stride = 4,
-	rmconfig->max_register = resource_size(res);
-
-	regmap = devm_regmap_init_mmio(&pdev->dev, base, rmconfig);
-	if (IS_ERR(regmap))
-		return PTR_ERR(regmap);
 
 	return berlin_pinctrl_probe(pdev, match->data);
 }
diff --git a/drivers/pinctrl/berlin/berlin-bg2q.c b/drivers/pinctrl/berlin/berlin-bg2q.c
index bd9662e57ad3..11aa10cc0e3e 100644
--- a/drivers/pinctrl/berlin/berlin-bg2q.c
+++ b/drivers/pinctrl/berlin/berlin-bg2q.c
@@ -395,28 +395,6 @@ static int berlin2q_pinctrl_probe(struct platform_device *pdev)
 {
 	const struct of_device_id *match =
 		of_match_device(berlin2q_pinctrl_match, &pdev->dev);
-	struct regmap_config *rmconfig;
-	struct regmap *regmap;
-	struct resource *res;
-	void __iomem *base;
-
-	rmconfig = devm_kzalloc(&pdev->dev, sizeof(*rmconfig), GFP_KERNEL);
-	if (!rmconfig)
-		return -ENOMEM;
-
-	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
-	base = devm_ioremap_resource(&pdev->dev, res);
-	if (IS_ERR(base))
-		return PTR_ERR(base);
-
-	rmconfig->reg_bits = 32,
-	rmconfig->val_bits = 32,
-	rmconfig->reg_stride = 4,
-	rmconfig->max_register = resource_size(res);
-
-	regmap = devm_regmap_init_mmio(&pdev->dev, base, rmconfig);
-	if (IS_ERR(regmap))
-		return PTR_ERR(regmap);
 
 	return berlin_pinctrl_probe(pdev, match->data);
 }
diff --git a/drivers/pinctrl/berlin/berlin.c b/drivers/pinctrl/berlin/berlin.c
index 7f0b0f93242b..2e3a8b8858ec 100644
--- a/drivers/pinctrl/berlin/berlin.c
+++ b/drivers/pinctrl/berlin/berlin.c
@@ -12,6 +12,7 @@
 
 #include <linux/io.h>
 #include <linux/module.h>
+#include <linux/mfd/syscon.h>
 #include <linux/of.h>
 #include <linux/of_address.h>
 #include <linux/of_device.h>
@@ -295,13 +296,15 @@ int berlin_pinctrl_probe(struct platform_device *pdev,
 			 const struct berlin_pinctrl_desc *desc)
 {
 	struct device *dev = &pdev->dev;
+	struct device_node *parent_np = of_get_parent(dev->of_node);
 	struct berlin_pinctrl *pctrl;
 	struct regmap *regmap;
 	int ret;
 
-	regmap = dev_get_regmap(&pdev->dev, NULL);
-	if (!regmap)
-		return -ENODEV;
+	regmap = syscon_node_to_regmap(parent_np);
+	of_node_put(parent_np);
+	if (IS_ERR(regmap))
+		return PTR_ERR(regmap);
 
 	pctrl = devm_kzalloc(dev, sizeof(*pctrl), GFP_KERNEL);
 	if (!pctrl)
-- 
2.3.0

[PATCH 07/11] pinctrl: berlin: use proper compatibles

[Antoine Tenart]

The Berlin pin-controller driver was sharing the chip and system
controller nodes with the clock and the reset drivers. They all shared
the same compatible. With the introduction of the Marvell Berlin MFD
controller, the Berlin pin-controller driver has now its own node.
Update its compatibles to not share anymore the ones of the chip and
system controllers.

Signed-off-by: Antoine Tenart <antoine.tenart@free-electrons.com>
---
 drivers/pinctrl/berlin/berlin-bg2.c   | 4 ++--
 drivers/pinctrl/berlin/berlin-bg2cd.c | 4 ++--
 drivers/pinctrl/berlin/berlin-bg2q.c  | 4 ++--
 3 files changed, 6 insertions(+), 6 deletions(-)

diff --git a/drivers/pinctrl/berlin/berlin-bg2.c b/drivers/pinctrl/berlin/berlin-bg2.c
index 368ec0b9b8ba..3769eaedf519 100644
--- a/drivers/pinctrl/berlin/berlin-bg2.c
+++ b/drivers/pinctrl/berlin/berlin-bg2.c
@@ -218,11 +218,11 @@ static const struct berlin_pinctrl_desc berlin2_sysmgr_pinctrl_data = {
 
 static const struct of_device_id berlin2_pinctrl_match[] = {
 	{
-		.compatible = "marvell,berlin2-chip-ctrl",
+		.compatible = "marvell,berlin2-soc-pinctrl",
 		.data = &berlin2_soc_pinctrl_data
 	},
 	{
-		.compatible = "marvell,berlin2-system-ctrl",
+		.compatible = "marvell,berlin2-system-pinctrl",
 		.data = &berlin2_sysmgr_pinctrl_data
 	},
 	{}
diff --git a/drivers/pinctrl/berlin/berlin-bg2cd.c b/drivers/pinctrl/berlin/berlin-bg2cd.c
index 6b9cae029ef7..9e11f191d643 100644
--- a/drivers/pinctrl/berlin/berlin-bg2cd.c
+++ b/drivers/pinctrl/berlin/berlin-bg2cd.c
@@ -161,11 +161,11 @@ static const struct berlin_pinctrl_desc berlin2cd_sysmgr_pinctrl_data = {
 
 static const struct of_device_id berlin2cd_pinctrl_match[] = {
 	{
-		.compatible = "marvell,berlin2cd-chip-ctrl",
+		.compatible = "marvell,berlin2cd-soc-pinctrl",
 		.data = &berlin2cd_soc_pinctrl_data
 	},
 	{
-		.compatible = "marvell,berlin2cd-system-ctrl",
+		.compatible = "marvell,berlin2cd-system-pinctrl",
 		.data = &berlin2cd_sysmgr_pinctrl_data
 	},
 	{}
diff --git a/drivers/pinctrl/berlin/berlin-bg2q.c b/drivers/pinctrl/berlin/berlin-bg2q.c
index 11aa10cc0e3e..ba7a8a8ad010 100644
--- a/drivers/pinctrl/berlin/berlin-bg2q.c
+++ b/drivers/pinctrl/berlin/berlin-bg2q.c
@@ -380,11 +380,11 @@ static const struct berlin_pinctrl_desc berlin2q_sysmgr_pinctrl_data = {
 
 static const struct of_device_id berlin2q_pinctrl_match[] = {
 	{
-		.compatible = "marvell,berlin2q-chip-ctrl",
+		.compatible = "marvell,berlin2q-soc-pinctrl",
 		.data = &berlin2q_soc_pinctrl_data,
 	},
 	{
-		.compatible = "marvell,berlin2q-system-ctrl",
+		.compatible = "marvell,berlin2q-system-pinctrl",
 		.data = &berlin2q_sysmgr_pinctrl_data,
 	},
 	{}
-- 
2.3.0

[PATCH 07/11] pinctrl: berlin: use proper compatibles

[Antoine Tenart]

The Berlin pin-controller driver was sharing the chip and system
controller nodes with the clock and the reset drivers. They all shared
the same compatible. With the introduction of the Marvell Berlin MFD
controller, the Berlin pin-controller driver has now its own node.
Update its compatibles to not share anymore the ones of the chip and
system controllers.

Signed-off-by: Antoine Tenart <antoine.tenart@free-electrons.com>
---
 drivers/pinctrl/berlin/berlin-bg2.c   | 4 ++--
 drivers/pinctrl/berlin/berlin-bg2cd.c | 4 ++--
 drivers/pinctrl/berlin/berlin-bg2q.c  | 4 ++--
 3 files changed, 6 insertions(+), 6 deletions(-)

diff --git a/drivers/pinctrl/berlin/berlin-bg2.c b/drivers/pinctrl/berlin/berlin-bg2.c
index 368ec0b9b8ba..3769eaedf519 100644
--- a/drivers/pinctrl/berlin/berlin-bg2.c
+++ b/drivers/pinctrl/berlin/berlin-bg2.c
@@ -218,11 +218,11 @@ static const struct berlin_pinctrl_desc berlin2_sysmgr_pinctrl_data = {
 
 static const struct of_device_id berlin2_pinctrl_match[] = {
 	{
-		.compatible = "marvell,berlin2-chip-ctrl",
+		.compatible = "marvell,berlin2-soc-pinctrl",
 		.data = &berlin2_soc_pinctrl_data
 	},
 	{
-		.compatible = "marvell,berlin2-system-ctrl",
+		.compatible = "marvell,berlin2-system-pinctrl",
 		.data = &berlin2_sysmgr_pinctrl_data
 	},
 	{}
diff --git a/drivers/pinctrl/berlin/berlin-bg2cd.c b/drivers/pinctrl/berlin/berlin-bg2cd.c
index 6b9cae029ef7..9e11f191d643 100644
--- a/drivers/pinctrl/berlin/berlin-bg2cd.c
+++ b/drivers/pinctrl/berlin/berlin-bg2cd.c
@@ -161,11 +161,11 @@ static const struct berlin_pinctrl_desc berlin2cd_sysmgr_pinctrl_data = {
 
 static const struct of_device_id berlin2cd_pinctrl_match[] = {
 	{
-		.compatible = "marvell,berlin2cd-chip-ctrl",
+		.compatible = "marvell,berlin2cd-soc-pinctrl",
 		.data = &berlin2cd_soc_pinctrl_data
 	},
 	{
-		.compatible = "marvell,berlin2cd-system-ctrl",
+		.compatible = "marvell,berlin2cd-system-pinctrl",
 		.data = &berlin2cd_sysmgr_pinctrl_data
 	},
 	{}
diff --git a/drivers/pinctrl/berlin/berlin-bg2q.c b/drivers/pinctrl/berlin/berlin-bg2q.c
index 11aa10cc0e3e..ba7a8a8ad010 100644
--- a/drivers/pinctrl/berlin/berlin-bg2q.c
+++ b/drivers/pinctrl/berlin/berlin-bg2q.c
@@ -380,11 +380,11 @@ static const struct berlin_pinctrl_desc berlin2q_sysmgr_pinctrl_data = {
 
 static const struct of_device_id berlin2q_pinctrl_match[] = {
 	{
-		.compatible = "marvell,berlin2q-chip-ctrl",
+		.compatible = "marvell,berlin2q-soc-pinctrl",
 		.data = &berlin2q_soc_pinctrl_data,
 	},
 	{
-		.compatible = "marvell,berlin2q-system-ctrl",
+		.compatible = "marvell,berlin2q-system-pinctrl",
 		.data = &berlin2q_sysmgr_pinctrl_data,
 	},
 	{}
-- 
2.3.0

[PATCH 08/11] Documentation: bindings: move the Berlin pinctrl documentation

[Antoine Tenart]

The Berlin pinctrl documentation was part of the Marvell Berlin SoC
documentation because the Berlin pinctrl configuration was inside the
chip and the system controllers. With the recent rework of the chip and
system controller handling (now an MFD driver registers all sub-devices
of the two soc and system controller nodes and each device has its own
sub-node), the documentation of the Berlin pinctrl driver can be moved
to the generic pinctrl documentation directory.

Signed-off-by: Antoine Tenart <antoine.tenart@free-electrons.com>
---
 .../devicetree/bindings/arm/marvell,berlin.txt     | 37 -------------------
 .../devicetree/bindings/pinctrl/berlin,pinctrl.txt | 43 ++++++++++++++++++++++
 2 files changed, 43 insertions(+), 37 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/pinctrl/berlin,pinctrl.txt

diff --git a/Documentation/devicetree/bindings/arm/marvell,berlin.txt b/Documentation/devicetree/bindings/arm/marvell,berlin.txt
index 6413ce1fa485..080953daf9f5 100644
--- a/Documentation/devicetree/bindings/arm/marvell,berlin.txt
+++ b/Documentation/devicetree/bindings/arm/marvell,berlin.txt
@@ -60,23 +60,6 @@ Clocks provided by core clocks shall be referenced by a clock specifier
 indexing one of the provided clocks. Refer to dt-bindings/clock/berlin<soc>.h
 for the corresponding index mapping.
 
-* Pin controller binding
-
-Pin control registers are part of both register sets, chip control and system
-control. The pins controlled are organized in groups, so no actual pin
-information is needed.
-
-A pin-controller node should contain subnodes representing the pin group
-configurations, one per function. Each subnode has the group name and the muxing
-function used.
-
-Be aware the Marvell Berlin datasheets use the keyword 'mode' for what is called
-a 'function' in the pin-controller subsystem.
-
-Required subnode-properties:
-- groups: a list of strings describing the group names.
-- function: a string describing the function used to mux the groups.
-
 Example:
 
 chip: chip-control@ea0000 {
@@ -85,29 +68,9 @@ chip: chip-control@ea0000 {
 	reg = <0xea0000 0x400>;
 	clocks = <&refclk>, <&externaldev 0>;
 	clock-names = "refclk", "video_ext0";
-
-	spi1_pmux: spi1-pmux {
-		groups = "G0";
-		function = "spi1";
-	};
 };
 
 sysctrl: system-controller@d000 {
 	compatible = "marvell,berlin2-system-ctrl";
 	reg = <0xd000 0x100>;
-
-	uart0_pmux: uart0-pmux {
-		groups = "GSM4";
-		function = "uart0";
-	};
-
-	uart1_pmux: uart1-pmux {
-		groups = "GSM5";
-		function = "uart1";
-	};
-
-	uart2_pmux: uart2-pmux {
-		groups = "GSM3";
-		function = "uart2";
-	};
 };
diff --git a/Documentation/devicetree/bindings/pinctrl/berlin,pinctrl.txt b/Documentation/devicetree/bindings/pinctrl/berlin,pinctrl.txt
new file mode 100644
index 000000000000..a8bb5e26019c
--- /dev/null
+++ b/Documentation/devicetree/bindings/pinctrl/berlin,pinctrl.txt
@@ -0,0 +1,43 @@
+* Pin-controller driver for the Marvell Berlin SoCs
+
+Pin control registers are part of both chip controller and system
+controller register sets. Pin controller nodes should be a sub-node of
+either the chip controller or system controller node. The pins
+controlled are organized in groups, so no actual pin information is
+needed.
+
+A pin-controller node should contain subnodes representing the pin group
+configurations, one per function. Each subnode has the group name and
+the muxing function used.
+
+Be aware the Marvell Berlin datasheets use the keyword 'mode' for what
+is called a 'function' in the pin-controller subsystem.
+
+Required properties:
+- compatible: should be one of:
+	"marvell,berlin2-soc-pinctrl",
+	"marvell,berlin2-system-pinctrl",
+	"marvell,berlin2cd-soc-pinctrl",
+	"marvell,berlin2cd-system-pinctrl",
+	"marvell,berlin2q-soc-pinctrl",
+	"marvell,berlin2q-system-pinctrl"
+
+Required subnode-properties:
+- groups: a list of strings describing the group names.
+- function: a string describing the function used to mux the groups.
+
+Example:
+
+sys_pinctrl: pin-controller {
+	compatible = "marvell,berlin2q-system-pinctrl";
+
+	uart0_pmux: uart0-pmux {
+		groups = "GSM12";
+		function = "uart0";
+	};
+};
+
+&uart0 {
+	pinctrl-0 = <&uart0_pmux>;
+	pinctrl-names = "default";
+};
-- 
2.3.0

[PATCH 08/11] Documentation: bindings: move the Berlin pinctrl documentation

[Antoine Tenart]

The Berlin pinctrl documentation was part of the Marvell Berlin SoC
documentation because the Berlin pinctrl configuration was inside the
chip and the system controllers. With the recent rework of the chip and
system controller handling (now an MFD driver registers all sub-devices
of the two soc and system controller nodes and each device has its own
sub-node), the documentation of the Berlin pinctrl driver can be moved
to the generic pinctrl documentation directory.

Signed-off-by: Antoine Tenart <antoine.tenart@free-electrons.com>
---
 .../devicetree/bindings/arm/marvell,berlin.txt     | 37 -------------------
 .../devicetree/bindings/pinctrl/berlin,pinctrl.txt | 43 ++++++++++++++++++++++
 2 files changed, 43 insertions(+), 37 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/pinctrl/berlin,pinctrl.txt

diff --git a/Documentation/devicetree/bindings/arm/marvell,berlin.txt b/Documentation/devicetree/bindings/arm/marvell,berlin.txt
index 6413ce1fa485..080953daf9f5 100644
--- a/Documentation/devicetree/bindings/arm/marvell,berlin.txt
+++ b/Documentation/devicetree/bindings/arm/marvell,berlin.txt
@@ -60,23 +60,6 @@ Clocks provided by core clocks shall be referenced by a clock specifier
 indexing one of the provided clocks. Refer to dt-bindings/clock/berlin<soc>.h
 for the corresponding index mapping.
 
-* Pin controller binding
-
-Pin control registers are part of both register sets, chip control and system
-control. The pins controlled are organized in groups, so no actual pin
-information is needed.
-
-A pin-controller node should contain subnodes representing the pin group
-configurations, one per function. Each subnode has the group name and the muxing
-function used.
-
-Be aware the Marvell Berlin datasheets use the keyword 'mode' for what is called
-a 'function' in the pin-controller subsystem.
-
-Required subnode-properties:
-- groups: a list of strings describing the group names.
-- function: a string describing the function used to mux the groups.
-
 Example:
 
 chip: chip-control at ea0000 {
@@ -85,29 +68,9 @@ chip: chip-control at ea0000 {
 	reg = <0xea0000 0x400>;
 	clocks = <&refclk>, <&externaldev 0>;
 	clock-names = "refclk", "video_ext0";
-
-	spi1_pmux: spi1-pmux {
-		groups = "G0";
-		function = "spi1";
-	};
 };
 
 sysctrl: system-controller at d000 {
 	compatible = "marvell,berlin2-system-ctrl";
 	reg = <0xd000 0x100>;
-
-	uart0_pmux: uart0-pmux {
-		groups = "GSM4";
-		function = "uart0";
-	};
-
-	uart1_pmux: uart1-pmux {
-		groups = "GSM5";
-		function = "uart1";
-	};
-
-	uart2_pmux: uart2-pmux {
-		groups = "GSM3";
-		function = "uart2";
-	};
 };
diff --git a/Documentation/devicetree/bindings/pinctrl/berlin,pinctrl.txt b/Documentation/devicetree/bindings/pinctrl/berlin,pinctrl.txt
new file mode 100644
index 000000000000..a8bb5e26019c
--- /dev/null
+++ b/Documentation/devicetree/bindings/pinctrl/berlin,pinctrl.txt
@@ -0,0 +1,43 @@
+* Pin-controller driver for the Marvell Berlin SoCs
+
+Pin control registers are part of both chip controller and system
+controller register sets. Pin controller nodes should be a sub-node of
+either the chip controller or system controller node. The pins
+controlled are organized in groups, so no actual pin information is
+needed.
+
+A pin-controller node should contain subnodes representing the pin group
+configurations, one per function. Each subnode has the group name and
+the muxing function used.
+
+Be aware the Marvell Berlin datasheets use the keyword 'mode' for what
+is called a 'function' in the pin-controller subsystem.
+
+Required properties:
+- compatible: should be one of:
+	"marvell,berlin2-soc-pinctrl",
+	"marvell,berlin2-system-pinctrl",
+	"marvell,berlin2cd-soc-pinctrl",
+	"marvell,berlin2cd-system-pinctrl",
+	"marvell,berlin2q-soc-pinctrl",
+	"marvell,berlin2q-system-pinctrl"
+
+Required subnode-properties:
+- groups: a list of strings describing the group names.
+- function: a string describing the function used to mux the groups.
+
+Example:
+
+sys_pinctrl: pin-controller {
+	compatible = "marvell,berlin2q-system-pinctrl";
+
+	uart0_pmux: uart0-pmux {
+		groups = "GSM12";
+		function = "uart0";
+	};
+};
+
+&uart0 {
+	pinctrl-0 = <&uart0_pmux>;
+	pinctrl-names = "default";
+};
-- 
2.3.0

[PATCH 09/11] ARM: berlin: rework chip and system controller nodes for BG2

[Antoine Tenart]

The chip and system controller nodes are now handled by the Berlin
controller mfd driver. Its sub-devices are then registered by the mfd
driver and let the drivers be probed properly, using their own
sub-nodes.

Rework the device tree to take this changes into account.

Signed-off-by: Antoine Tenart <antoine.tenart@free-electrons.com>
---
 arch/arm/boot/dts/berlin2.dtsi | 50 ++++++++++++++++++++++++++----------------
 1 file changed, 31 insertions(+), 19 deletions(-)

diff --git a/arch/arm/boot/dts/berlin2.dtsi b/arch/arm/boot/dts/berlin2.dtsi
index 015a06c67c91..498ffd5b0e05 100644
--- a/arch/arm/boot/dts/berlin2.dtsi
+++ b/arch/arm/boot/dts/berlin2.dtsi
@@ -350,17 +350,25 @@
 			};
 		};
 
-		chip: chip-control@ea0000 {
-			compatible = "marvell,berlin2-chip-ctrl";
-			#clock-cells = <1>;
-			#reset-cells = <2>;
+		chip: chip-controller@ea0000 {
+			compatible = "marvell,berlin2-chip-ctrl", "syscon";
 			reg = <0xea0000 0x400>;
+			#clock-cells = <1>;
 			clocks = <&refclk>;
 			clock-names = "refclk";
 
-			emmc_pmux: emmc-pmux {
-				groups = "G26";
-				function = "emmc";
+			soc_pinctrl: pin-controller {
+				compatible = "marvell,berlin2-soc-pinctrl";
+
+				emmc_pmux: emmc-pmux {
+					groups = "G26";
+					function = "emmc";
+				};
+			};
+
+			chip_rst: reset {
+				compatible = "marvell,berlin2-reset";
+				#reset-cells = <2>;
 			};
 		};
 
@@ -442,22 +450,26 @@
 			};
 
 			sysctrl: system-controller@d000 {
-				compatible = "marvell,berlin2-system-ctrl";
+				compatible = "marvell,berlin2-system-ctrl", "syscon";
 				reg = <0xd000 0x100>;
 
-				uart0_pmux: uart0-pmux {
-					groups = "GSM4";
-					function = "uart0";
-				};
+				sys_pinctrl: pin-controller {
+					compatible = "marvell,berlin2-system-pinctrl";
 
-				uart1_pmux: uart1-pmux {
-					groups = "GSM5";
-					function = "uart1";
-				};
+					uart0_pmux: uart0-pmux {
+						groups = "GSM4";
+						function = "uart0";
+					};
+
+					uart1_pmux: uart1-pmux {
+						groups = "GSM5";
+						function = "uart1";
+					};
 
-				uart2_pmux: uart2-pmux {
-					groups = "GSM3";
-					function = "uart2";
+					uart2_pmux: uart2-pmux {
+						groups = "GSM3";
+						function = "uart2";
+					};
 				};
 			};
 
-- 
2.3.0

[PATCH 09/11] ARM: berlin: rework chip and system controller nodes for BG2

[Antoine Tenart]

The chip and system controller nodes are now handled by the Berlin
controller mfd driver. Its sub-devices are then registered by the mfd
driver and let the drivers be probed properly, using their own
sub-nodes.

Rework the device tree to take this changes into account.

Signed-off-by: Antoine Tenart <antoine.tenart@free-electrons.com>
---
 arch/arm/boot/dts/berlin2.dtsi | 50 ++++++++++++++++++++++++++----------------
 1 file changed, 31 insertions(+), 19 deletions(-)

diff --git a/arch/arm/boot/dts/berlin2.dtsi b/arch/arm/boot/dts/berlin2.dtsi
index 015a06c67c91..498ffd5b0e05 100644
--- a/arch/arm/boot/dts/berlin2.dtsi
+++ b/arch/arm/boot/dts/berlin2.dtsi
@@ -350,17 +350,25 @@
 			};
 		};
 
-		chip: chip-control at ea0000 {
-			compatible = "marvell,berlin2-chip-ctrl";
-			#clock-cells = <1>;
-			#reset-cells = <2>;
+		chip: chip-controller at ea0000 {
+			compatible = "marvell,berlin2-chip-ctrl", "syscon";
 			reg = <0xea0000 0x400>;
+			#clock-cells = <1>;
 			clocks = <&refclk>;
 			clock-names = "refclk";
 
-			emmc_pmux: emmc-pmux {
-				groups = "G26";
-				function = "emmc";
+			soc_pinctrl: pin-controller {
+				compatible = "marvell,berlin2-soc-pinctrl";
+
+				emmc_pmux: emmc-pmux {
+					groups = "G26";
+					function = "emmc";
+				};
+			};
+
+			chip_rst: reset {
+				compatible = "marvell,berlin2-reset";
+				#reset-cells = <2>;
 			};
 		};
 
@@ -442,22 +450,26 @@
 			};
 
 			sysctrl: system-controller at d000 {
-				compatible = "marvell,berlin2-system-ctrl";
+				compatible = "marvell,berlin2-system-ctrl", "syscon";
 				reg = <0xd000 0x100>;
 
-				uart0_pmux: uart0-pmux {
-					groups = "GSM4";
-					function = "uart0";
-				};
+				sys_pinctrl: pin-controller {
+					compatible = "marvell,berlin2-system-pinctrl";
 
-				uart1_pmux: uart1-pmux {
-					groups = "GSM5";
-					function = "uart1";
-				};
+					uart0_pmux: uart0-pmux {
+						groups = "GSM4";
+						function = "uart0";
+					};
+
+					uart1_pmux: uart1-pmux {
+						groups = "GSM5";
+						function = "uart1";
+					};
 
-				uart2_pmux: uart2-pmux {
-					groups = "GSM3";
-					function = "uart2";
+					uart2_pmux: uart2-pmux {
+						groups = "GSM3";
+						function = "uart2";
+					};
 				};
 			};
 
-- 
2.3.0

[PATCH 10/11] ARM: berlin: rework chip and system controller nodes for BG2CD

[Antoine Tenart]

The chip and system controller nodes are now handled by the Berlin
controller mfd driver. Its sub-devices are then registered by the mfd
driver and let the drivers be probed properly, using their own
sub-nodes.

Rework the device tree to take this changes into account.

Signed-off-by: Antoine Tenart <antoine.tenart@free-electrons.com>
---
 arch/arm/boot/dts/berlin2cd.dtsi | 32 ++++++++++++++++++++++----------
 1 file changed, 22 insertions(+), 10 deletions(-)

diff --git a/arch/arm/boot/dts/berlin2cd.dtsi b/arch/arm/boot/dts/berlin2cd.dtsi
index 230df3b1770e..314cde70a3ba 100644
--- a/arch/arm/boot/dts/berlin2cd.dtsi
+++ b/arch/arm/boot/dts/berlin2cd.dtsi
@@ -79,7 +79,7 @@
 			compatible = "marvell,berlin2cd-usb-phy";
 			reg = <0xb74000 0x128>;
 			#phy-cells = <0>;
-			resets = <&chip 0x178 23>;
+			resets = <&chip_rst 0x178 23>;
 			status = "disabled";
 		};
 
@@ -87,7 +87,7 @@
 			compatible = "marvell,berlin2cd-usb-phy";
 			reg = <0xb78000 0x128>;
 			#phy-cells = <0>;
-			resets = <&chip 0x178 24>;
+			resets = <&chip_rst 0x178 24>;
 			status = "disabled";
 		};
 
@@ -289,17 +289,25 @@
 			};
 		};
 
-		chip: chip-control@ea0000 {
-			compatible = "marvell,berlin2cd-chip-ctrl";
-			#clock-cells = <1>;
-			#reset-cells = <2>;
+		chip: chip-controller@ea0000 {
+			compatible = "marvell,berlin2cd-chip-ctrl", "syscon";
 			reg = <0xea0000 0x400>;
+			#clock-cells = <1>;
 			clocks = <&refclk>;
 			clock-names = "refclk";
 
-			uart0_pmux: uart0-pmux {
-				groups = "G6";
-				function = "uart0";
+			soc_pinctrl: pin-controller {
+				compatible = "marvell,berlin2cd-soc-pinctrl";
+
+				uart0_pmux: uart0-pmux {
+					groups = "G6";
+					function = "uart0";
+				};
+			};
+
+			chip_rst: reset {
+				compatible = "marvell,berlin2-reset";
+				#reset-cells = <2>;
 			};
 		};
 
@@ -384,8 +392,12 @@
 			};
 
 			sysctrl: system-controller@d000 {
-				compatible = "marvell,berlin2cd-system-ctrl";
+				compatible = "marvell,berlin2cd-system-ctrl", "syscon";
 				reg = <0xd000 0x100>;
+
+				sys_pinctrl: pin-controller {
+					compatible = "marvell,berlin2cd-system-pinctrl";
+				};
 			};
 
 			sic: interrupt-controller@e000 {
-- 
2.3.0

[PATCH 10/11] ARM: berlin: rework chip and system controller nodes for BG2CD

[Antoine Tenart]

The chip and system controller nodes are now handled by the Berlin
controller mfd driver. Its sub-devices are then registered by the mfd
driver and let the drivers be probed properly, using their own
sub-nodes.

Rework the device tree to take this changes into account.

Signed-off-by: Antoine Tenart <antoine.tenart@free-electrons.com>
---
 arch/arm/boot/dts/berlin2cd.dtsi | 32 ++++++++++++++++++++++----------
 1 file changed, 22 insertions(+), 10 deletions(-)

diff --git a/arch/arm/boot/dts/berlin2cd.dtsi b/arch/arm/boot/dts/berlin2cd.dtsi
index 230df3b1770e..314cde70a3ba 100644
--- a/arch/arm/boot/dts/berlin2cd.dtsi
+++ b/arch/arm/boot/dts/berlin2cd.dtsi
@@ -79,7 +79,7 @@
 			compatible = "marvell,berlin2cd-usb-phy";
 			reg = <0xb74000 0x128>;
 			#phy-cells = <0>;
-			resets = <&chip 0x178 23>;
+			resets = <&chip_rst 0x178 23>;
 			status = "disabled";
 		};
 
@@ -87,7 +87,7 @@
 			compatible = "marvell,berlin2cd-usb-phy";
 			reg = <0xb78000 0x128>;
 			#phy-cells = <0>;
-			resets = <&chip 0x178 24>;
+			resets = <&chip_rst 0x178 24>;
 			status = "disabled";
 		};
 
@@ -289,17 +289,25 @@
 			};
 		};
 
-		chip: chip-control at ea0000 {
-			compatible = "marvell,berlin2cd-chip-ctrl";
-			#clock-cells = <1>;
-			#reset-cells = <2>;
+		chip: chip-controller at ea0000 {
+			compatible = "marvell,berlin2cd-chip-ctrl", "syscon";
 			reg = <0xea0000 0x400>;
+			#clock-cells = <1>;
 			clocks = <&refclk>;
 			clock-names = "refclk";
 
-			uart0_pmux: uart0-pmux {
-				groups = "G6";
-				function = "uart0";
+			soc_pinctrl: pin-controller {
+				compatible = "marvell,berlin2cd-soc-pinctrl";
+
+				uart0_pmux: uart0-pmux {
+					groups = "G6";
+					function = "uart0";
+				};
+			};
+
+			chip_rst: reset {
+				compatible = "marvell,berlin2-reset";
+				#reset-cells = <2>;
 			};
 		};
 
@@ -384,8 +392,12 @@
 			};
 
 			sysctrl: system-controller at d000 {
-				compatible = "marvell,berlin2cd-system-ctrl";
+				compatible = "marvell,berlin2cd-system-ctrl", "syscon";
 				reg = <0xd000 0x100>;
+
+				sys_pinctrl: pin-controller {
+					compatible = "marvell,berlin2cd-system-pinctrl";
+				};
 			};
 
 			sic: interrupt-controller at e000 {
-- 
2.3.0

[PATCH 11/11] ARM: berlin: rework chip and system controller nodes for BG2Q

[Antoine Tenart]

The chip and system controller nodes are now handled by the Berlin
controller mfd driver. Its sub-devices are then registered by the mfd
driver and let the drivers be probed properly, using their own
sub-nodes.

Rework the device tree to take this changes into account.

Signed-off-by: Antoine Tenart <antoine.tenart@free-electrons.com>
---
 arch/arm/boot/dts/berlin2q.dtsi | 70 ++++++++++++++++++++++++-----------------
 1 file changed, 41 insertions(+), 29 deletions(-)

diff --git a/arch/arm/boot/dts/berlin2q.dtsi b/arch/arm/boot/dts/berlin2q.dtsi
index e2f61f27944e..bd86d4121ec8 100644
--- a/arch/arm/boot/dts/berlin2q.dtsi
+++ b/arch/arm/boot/dts/berlin2q.dtsi
@@ -119,7 +119,7 @@
 			compatible = "marvell,berlin2-usb-phy";
 			reg = <0xa2f400 0x128>;
 			#phy-cells = <0>;
-			resets = <&chip 0x104 14>;
+			resets = <&chip_rst 0x104 14>;
 			status = "disabled";
 		};
 
@@ -137,7 +137,7 @@
 			compatible = "marvell,berlin2-usb-phy";
 			reg = <0xb74000 0x128>;
 			#phy-cells = <0>;
-			resets = <&chip 0x104 12>;
+			resets = <&chip_rst 0x104 12>;
 			status = "disabled";
 		};
 
@@ -351,22 +351,30 @@
 			};
 		};
 
-		chip: chip-control@ea0000 {
-			compatible = "marvell,berlin2q-chip-ctrl";
-			#clock-cells = <1>;
-			#reset-cells = <2>;
+		chip: chip-controller@ea0000 {
+			compatible = "marvell,berlin2q-chip-ctrl", "syscon";
 			reg = <0xea0000 0x400>, <0xdd0170 0x10>;
+			#clock-cells = <1>;
 			clocks = <&refclk>;
 			clock-names = "refclk";
 
-			twsi0_pmux: twsi0-pmux {
-				groups = "G6";
-				function = "twsi0";
+			soc_pinctrl: pin-controller {
+				compatible = "marvell,berlin2q-soc-pinctrl";
+
+				twsi0_pmux: twsi0-pmux {
+					groups = "G6";
+					function = "twsi0";
+				};
+
+				twsi1_pmux: twsi1-pmux {
+					groups = "G7";
+					function = "twsi1";
+				};
 			};
 
-			twsi1_pmux: twsi1-pmux {
-				groups = "G7";
-				function = "twsi1";
+			chip_rst: reset {
+				compatible = "marvell,berlin2-reset";
+				#reset-cells = <2>;
 			};
 		};
 
@@ -517,28 +525,32 @@
 				};
 			};
 
-			sysctrl: pin-controller@d000 {
-				compatible = "marvell,berlin2q-system-ctrl";
+			sysctrl: system-controller@d000 {
+				compatible = "marvell,berlin2q-system-ctrl", "syscon";
 				reg = <0xd000 0x100>;
 
-				uart0_pmux: uart0-pmux {
-					groups = "GSM12";
-					function = "uart0";
-				};
+				sys_pinctrl: pin-controller {
+					compatible = "marvell,berlin2q-system-pinctrl";
 
-				uart1_pmux: uart1-pmux {
-					groups = "GSM14";
-					function = "uart1";
-				};
+					uart0_pmux: uart0-pmux {
+						groups = "GSM12";
+						function = "uart0";
+					};
 
-				twsi2_pmux: twsi2-pmux {
-					groups = "GSM13";
-					function = "twsi2";
-				};
+					uart1_pmux: uart1-pmux {
+						groups = "GSM14";
+						function = "uart1";
+					};
+
+					twsi2_pmux: twsi2-pmux {
+						groups = "GSM13";
+						function = "twsi2";
+					};
 
-				twsi3_pmux: twsi3-pmux {
-					groups = "GSM14";
-					function = "twsi3";
+					twsi3_pmux: twsi3-pmux {
+						groups = "GSM14";
+						function = "twsi3";
+					};
 				};
 			};
 
-- 
2.3.0

[PATCH 11/11] ARM: berlin: rework chip and system controller nodes for BG2Q

[Antoine Tenart]

The chip and system controller nodes are now handled by the Berlin
controller mfd driver. Its sub-devices are then registered by the mfd
driver and let the drivers be probed properly, using their own
sub-nodes.

Rework the device tree to take this changes into account.

Signed-off-by: Antoine Tenart <antoine.tenart@free-electrons.com>
---
 arch/arm/boot/dts/berlin2q.dtsi | 70 ++++++++++++++++++++++++-----------------
 1 file changed, 41 insertions(+), 29 deletions(-)

diff --git a/arch/arm/boot/dts/berlin2q.dtsi b/arch/arm/boot/dts/berlin2q.dtsi
index e2f61f27944e..bd86d4121ec8 100644
--- a/arch/arm/boot/dts/berlin2q.dtsi
+++ b/arch/arm/boot/dts/berlin2q.dtsi
@@ -119,7 +119,7 @@
 			compatible = "marvell,berlin2-usb-phy";
 			reg = <0xa2f400 0x128>;
 			#phy-cells = <0>;
-			resets = <&chip 0x104 14>;
+			resets = <&chip_rst 0x104 14>;
 			status = "disabled";
 		};
 
@@ -137,7 +137,7 @@
 			compatible = "marvell,berlin2-usb-phy";
 			reg = <0xb74000 0x128>;
 			#phy-cells = <0>;
-			resets = <&chip 0x104 12>;
+			resets = <&chip_rst 0x104 12>;
 			status = "disabled";
 		};
 
@@ -351,22 +351,30 @@
 			};
 		};
 
-		chip: chip-control at ea0000 {
-			compatible = "marvell,berlin2q-chip-ctrl";
-			#clock-cells = <1>;
-			#reset-cells = <2>;
+		chip: chip-controller at ea0000 {
+			compatible = "marvell,berlin2q-chip-ctrl", "syscon";
 			reg = <0xea0000 0x400>, <0xdd0170 0x10>;
+			#clock-cells = <1>;
 			clocks = <&refclk>;
 			clock-names = "refclk";
 
-			twsi0_pmux: twsi0-pmux {
-				groups = "G6";
-				function = "twsi0";
+			soc_pinctrl: pin-controller {
+				compatible = "marvell,berlin2q-soc-pinctrl";
+
+				twsi0_pmux: twsi0-pmux {
+					groups = "G6";
+					function = "twsi0";
+				};
+
+				twsi1_pmux: twsi1-pmux {
+					groups = "G7";
+					function = "twsi1";
+				};
 			};
 
-			twsi1_pmux: twsi1-pmux {
-				groups = "G7";
-				function = "twsi1";
+			chip_rst: reset {
+				compatible = "marvell,berlin2-reset";
+				#reset-cells = <2>;
 			};
 		};
 
@@ -517,28 +525,32 @@
 				};
 			};
 
-			sysctrl: pin-controller at d000 {
-				compatible = "marvell,berlin2q-system-ctrl";
+			sysctrl: system-controller at d000 {
+				compatible = "marvell,berlin2q-system-ctrl", "syscon";
 				reg = <0xd000 0x100>;
 
-				uart0_pmux: uart0-pmux {
-					groups = "GSM12";
-					function = "uart0";
-				};
+				sys_pinctrl: pin-controller {
+					compatible = "marvell,berlin2q-system-pinctrl";
 
-				uart1_pmux: uart1-pmux {
-					groups = "GSM14";
-					function = "uart1";
-				};
+					uart0_pmux: uart0-pmux {
+						groups = "GSM12";
+						function = "uart0";
+					};
 
-				twsi2_pmux: twsi2-pmux {
-					groups = "GSM13";
-					function = "twsi2";
-				};
+					uart1_pmux: uart1-pmux {
+						groups = "GSM14";
+						function = "uart1";
+					};
+
+					twsi2_pmux: twsi2-pmux {
+						groups = "GSM13";
+						function = "twsi2";
+					};
 
-				twsi3_pmux: twsi3-pmux {
-					groups = "GSM14";
-					function = "twsi3";
+					twsi3_pmux: twsi3-pmux {
+						groups = "GSM14";
+						function = "twsi3";
+					};
 				};
 			};
 
-- 
2.3.0

Re: [PATCH 01/11] mfd: add the Berlin controller driver

[Lee Jones]

On Wed, 11 Feb 2015, Antoine Tenart wrote:

> Marvell Berlin SoC have two nodes providing multiple devices (clk,
> pinctrl, reset). While until now these drivers were initialized using
> initcalls, this wasn't a proper solution. This mfd driver will be
> responsible of adding these devices, to be probed properly.
> 
> It currently registers the pin controllers and the reset drivers for
> BG2, BG2CD and BG2Q in the soc and system controller nodes.
> 
> Signed-off-by: Antoine Tenart <antoine.tenart@free-electrons.com>
> ---
>  drivers/mfd/Kconfig       |   5 ++
>  drivers/mfd/Makefile      |   2 +
>  drivers/mfd/berlin-ctrl.c | 180 ++++++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 187 insertions(+)
>  create mode 100644 drivers/mfd/berlin-ctrl.c
> 
> diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig
> index 2e6b7311fabc..eda6dbec02ff 100644
> --- a/drivers/mfd/Kconfig
> +++ b/drivers/mfd/Kconfig
> @@ -840,6 +840,11 @@ config STMPE_SPI
>  	  This is used to enable SPI interface of STMPE
>  endmenu
>  
> +config MFD_BERLIN_CTRL
> +	bool

Missing description.

Why can't this driver be built as a module?

> +	select MFD_CORE
> +	select MFD_SYSCON

Missing help.

>  config MFD_STA2X11
>  	bool "STMicroelectronics STA2X11"
>  	depends on STA2X11
> diff --git a/drivers/mfd/Makefile b/drivers/mfd/Makefile
> index 53467e211381..adf60e85df20 100644
> --- a/drivers/mfd/Makefile
> +++ b/drivers/mfd/Makefile
> @@ -179,3 +179,5 @@ obj-$(CONFIG_MFD_DLN2)		+= dln2.o
>  
>  intel-soc-pmic-objs		:= intel_soc_pmic_core.o intel_soc_pmic_crc.o
>  obj-$(CONFIG_INTEL_SOC_PMIC)	+= intel-soc-pmic.o
> +
> +obj-$(CONFIG_MFD_BERLIN_CTRL)	+= berlin-ctrl.o
> diff --git a/drivers/mfd/berlin-ctrl.c b/drivers/mfd/berlin-ctrl.c
> new file mode 100644
> index 000000000000..e3ce6f069f16
> --- /dev/null
> +++ b/drivers/mfd/berlin-ctrl.c
> @@ -0,0 +1,180 @@
> +/*
> + * Copyright (C) 2015 Marvell Technology Group Ltd.
> + *
> + * Antoine Tenart <antoine.tenart@free-electrons.com>

Author: Antoine Tenart <antoine.tenart@free-electrons.com>

> + * This file is licensed under the terms of the GNU General Public
> + * License version 2. This program is licensed "as is" without any
> + * warranty of any kind, whether express or implied.
> + */
> +
> +#include <linux/mfd/core.h>
> +#include <linux/module.h>
> +#include <linux/of.h>

kernel.h?

> +struct berlin_ctrl_priv {
> +	const struct mfd_cell	*devs;
> +	unsigned		ndevs;
> +};
> +
> +/*
> + * BG2 devices
> + */
> +static const struct mfd_cell berlin2_ctrl_chip_ctrl_subdevs[] = {
> +	{
> +		.name		= "berlin2-soc-pinctrl",
> +		.of_compatible	= "marvell,berlin2-soc-pinctrl",
> +	},
> +	{
> +		.name		= "berlin2-reset",
> +		.of_compatible	= "marvell,berlin2-reset",
> +	},
> +};
> +
> +static const struct mfd_cell berlin2_ctrl_system_ctrl_subdevs[] = {
> +	{
> +		.name		= "berlin2-system-pinctrl",
> +		.of_compatible	= "marvell,berlin2-system-pinctrl",
> +	},
> +};
> +
> +static const struct berlin_ctrl_priv berlin2_ctrl_chip_ctrl_data = {
> +	.devs	= berlin2_ctrl_chip_ctrl_subdevs,
> +	.ndevs	= ARRAY_SIZE(berlin2_ctrl_chip_ctrl_subdevs),
> +};
> +
> +static const struct berlin_ctrl_priv berlin2_ctrl_system_data = {
> +	.devs	= berlin2_ctrl_system_ctrl_subdevs,
> +	.ndevs	= ARRAY_SIZE(berlin2_ctrl_system_ctrl_subdevs),
> +};
> +
> +

Superfluous '\n'

> +/*
> + * BG2CD devices
> + */
> +static const struct mfd_cell berlin2cd_ctrl_chip_ctrl_subdevs[] = {
> +	{
> +		.name		= "berlin2cd-soc-pinctrl",
> +		.of_compatible	= "marvell,berlin2cd-soc-pinctrl",
> +	},
> +	{
> +		.name		= "berlin2-reset",
> +		.of_compatible	= "marvell,berlin2-reset",
> +	},
> +};
> +
> +static const struct mfd_cell berlin2cd_ctrl_system_ctrl_subdevs[] = {
> +	{
> +		.name		= "berlin2cd-system-pinctrl",
> +		.of_compatible	= "marvell,berlin2cd-system-pinctrl",
> +	},
> +};
> +
> +static const struct berlin_ctrl_priv berlin2cd_ctrl_chip_ctrl_data = {
> +	.devs	= berlin2cd_ctrl_chip_ctrl_subdevs,
> +	.ndevs	= ARRAY_SIZE(berlin2cd_ctrl_chip_ctrl_subdevs),
> +};
> +
> +static const struct berlin_ctrl_priv berlin2cd_ctrl_system_data = {
> +	.devs	= berlin2cd_ctrl_system_ctrl_subdevs,
> +	.ndevs	= ARRAY_SIZE(berlin2cd_ctrl_system_ctrl_subdevs),
> +};
> +
> +/*
> + * BG2Q devices
> + */
> +static const struct mfd_cell berlin2q_ctrl_chip_ctrl_subdevs[] = {
> +	{
> +		.name		= "berlin2q-soc-pinctrl",
> +		.of_compatible	= "marvell,berlin2q-soc-pinctrl",
> +	},
> +	{
> +		.name		= "berlin2-reset",
> +		.of_compatible	= "marvell,berlin2-reset",
> +	},
> +};
> +
> +static const struct mfd_cell berlin2q_ctrl_system_ctrl_subdevs[] = {
> +	{
> +		.name		= "berlin2q-system-pinctrl",
> +		.of_compatible	= "marvell,berlin2q-system-pinctrl",
> +	},
> +};
> +
> +static const struct berlin_ctrl_priv berlin2q_ctrl_chip_ctrl_data = {
> +	.devs	= berlin2q_ctrl_chip_ctrl_subdevs,
> +	.ndevs	= ARRAY_SIZE(berlin2q_ctrl_chip_ctrl_subdevs),
> +};
> +
> +static const struct berlin_ctrl_priv berlin2q_ctrl_system_data = {
> +	.devs	= berlin2q_ctrl_system_ctrl_subdevs,
> +	.ndevs	= ARRAY_SIZE(berlin2q_ctrl_system_ctrl_subdevs),
> +};
> +
> +
> +static const struct of_device_id berlin_ctrl_of_match[] = {
> +	/* BG2 */
> +	{
> +		.compatible	= "marvell,berlin2-chip-ctrl",
> +		.data		= &berlin2_ctrl_chip_ctrl_data,
> +	},
> +	{
> +		.compatible	= "marvell,berlin2-system-ctrl",
> +		.data		= &berlin2_ctrl_system_data,
> +	},
> +	/* BG2CD */
> +	{
> +		.compatible	= "marvell,berlin2cd-chip-ctrl",
> +		.data		= &berlin2cd_ctrl_chip_ctrl_data,
> +	},
> +	{
> +		.compatible	= "marvell,berlin2cd-system-ctrl",
> +		.data		= &berlin2cd_ctrl_system_data,
> +	},
> +	/* BG2Q */
> +	{
> +		.compatible	= "marvell,berlin2q-chip-ctrl",
> +		.data		= &berlin2q_ctrl_chip_ctrl_data,
> +	},
> +	{
> +		.compatible	= "marvell,berlin2q-system-ctrl",
> +		.data		= &berlin2q_ctrl_system_data,
> +	},
> +	{ },
> +};
> +MODULE_DEVICE_TABLE(of, berlin_ctrl_of_match);
> +
> +static int berlin_ctrl_probe(struct platform_device *pdev)
> +{
> +	struct device *dev = &pdev->dev;
> +	const struct of_device_id *match;
> +	const struct berlin_ctrl_priv *priv;
> +	int ret;
> +
> +	match = of_match_node(berlin_ctrl_of_match, dev->of_node);
> +	if (!match)
> +		return -EINVAL;
> +
> +	priv = match->data;
> +
> +	ret = mfd_add_devices(dev, 0, priv->devs, priv->ndevs, NULL, -1, NULL);
> +	if (ret) {
> +		dev_err(dev, "Failed to add devices: %d\n", ret);
> +		return ret;
> +	}
> +
> +	return 0;
> +}

I'm not sure I see the point in this driver.  Why can't you just
register these devices directly from DT?

> +static struct platform_driver berlin_ctrl_driver = {
> +	.probe	= berlin_ctrl_probe,
> +	.driver	= {
> +		.name = "berlin-ctrl",
> +		.of_match_table = berlin_ctrl_of_match,
> +	},
> +};
> +module_platform_driver(berlin_ctrl_driver);
> +
> +MODULE_AUTHOR("Antoine Tenart <antoine.tenart@free-electrons.com>");
> +MODULE_DESCRIPTION("Marvell Berlin controller driver (mfd)");
> +MODULE_LICENSE("GPL");

v2

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

[PATCH 01/11] mfd: add the Berlin controller driver

[Lee Jones]

On Wed, 11 Feb 2015, Antoine Tenart wrote:

> Marvell Berlin SoC have two nodes providing multiple devices (clk,
> pinctrl, reset). While until now these drivers were initialized using
> initcalls, this wasn't a proper solution. This mfd driver will be
> responsible of adding these devices, to be probed properly.
> 
> It currently registers the pin controllers and the reset drivers for
> BG2, BG2CD and BG2Q in the soc and system controller nodes.
> 
> Signed-off-by: Antoine Tenart <antoine.tenart@free-electrons.com>
> ---
>  drivers/mfd/Kconfig       |   5 ++
>  drivers/mfd/Makefile      |   2 +
>  drivers/mfd/berlin-ctrl.c | 180 ++++++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 187 insertions(+)
>  create mode 100644 drivers/mfd/berlin-ctrl.c
> 
> diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig
> index 2e6b7311fabc..eda6dbec02ff 100644
> --- a/drivers/mfd/Kconfig
> +++ b/drivers/mfd/Kconfig
> @@ -840,6 +840,11 @@ config STMPE_SPI
>  	  This is used to enable SPI interface of STMPE
>  endmenu
>  
> +config MFD_BERLIN_CTRL
> +	bool

Missing description.

Why can't this driver be built as a module?

> +	select MFD_CORE
> +	select MFD_SYSCON

Missing help.

>  config MFD_STA2X11
>  	bool "STMicroelectronics STA2X11"
>  	depends on STA2X11
> diff --git a/drivers/mfd/Makefile b/drivers/mfd/Makefile
> index 53467e211381..adf60e85df20 100644
> --- a/drivers/mfd/Makefile
> +++ b/drivers/mfd/Makefile
> @@ -179,3 +179,5 @@ obj-$(CONFIG_MFD_DLN2)		+= dln2.o
>  
>  intel-soc-pmic-objs		:= intel_soc_pmic_core.o intel_soc_pmic_crc.o
>  obj-$(CONFIG_INTEL_SOC_PMIC)	+= intel-soc-pmic.o
> +
> +obj-$(CONFIG_MFD_BERLIN_CTRL)	+= berlin-ctrl.o
> diff --git a/drivers/mfd/berlin-ctrl.c b/drivers/mfd/berlin-ctrl.c
> new file mode 100644
> index 000000000000..e3ce6f069f16
> --- /dev/null
> +++ b/drivers/mfd/berlin-ctrl.c
> @@ -0,0 +1,180 @@
> +/*
> + * Copyright (C) 2015 Marvell Technology Group Ltd.
> + *
> + * Antoine Tenart <antoine.tenart@free-electrons.com>

Author: Antoine Tenart <antoine.tenart@free-electrons.com>

> + * This file is licensed under the terms of the GNU General Public
> + * License version 2. This program is licensed "as is" without any
> + * warranty of any kind, whether express or implied.
> + */
> +
> +#include <linux/mfd/core.h>
> +#include <linux/module.h>
> +#include <linux/of.h>

kernel.h?

> +struct berlin_ctrl_priv {
> +	const struct mfd_cell	*devs;
> +	unsigned		ndevs;
> +};
> +
> +/*
> + * BG2 devices
> + */
> +static const struct mfd_cell berlin2_ctrl_chip_ctrl_subdevs[] = {
> +	{
> +		.name		= "berlin2-soc-pinctrl",
> +		.of_compatible	= "marvell,berlin2-soc-pinctrl",
> +	},
> +	{
> +		.name		= "berlin2-reset",
> +		.of_compatible	= "marvell,berlin2-reset",
> +	},
> +};
> +
> +static const struct mfd_cell berlin2_ctrl_system_ctrl_subdevs[] = {
> +	{
> +		.name		= "berlin2-system-pinctrl",
> +		.of_compatible	= "marvell,berlin2-system-pinctrl",
> +	},
> +};
> +
> +static const struct berlin_ctrl_priv berlin2_ctrl_chip_ctrl_data = {
> +	.devs	= berlin2_ctrl_chip_ctrl_subdevs,
> +	.ndevs	= ARRAY_SIZE(berlin2_ctrl_chip_ctrl_subdevs),
> +};
> +
> +static const struct berlin_ctrl_priv berlin2_ctrl_system_data = {
> +	.devs	= berlin2_ctrl_system_ctrl_subdevs,
> +	.ndevs	= ARRAY_SIZE(berlin2_ctrl_system_ctrl_subdevs),
> +};
> +
> +

Superfluous '\n'

> +/*
> + * BG2CD devices
> + */
> +static const struct mfd_cell berlin2cd_ctrl_chip_ctrl_subdevs[] = {
> +	{
> +		.name		= "berlin2cd-soc-pinctrl",
> +		.of_compatible	= "marvell,berlin2cd-soc-pinctrl",
> +	},
> +	{
> +		.name		= "berlin2-reset",
> +		.of_compatible	= "marvell,berlin2-reset",
> +	},
> +};
> +
> +static const struct mfd_cell berlin2cd_ctrl_system_ctrl_subdevs[] = {
> +	{
> +		.name		= "berlin2cd-system-pinctrl",
> +		.of_compatible	= "marvell,berlin2cd-system-pinctrl",
> +	},
> +};
> +
> +static const struct berlin_ctrl_priv berlin2cd_ctrl_chip_ctrl_data = {
> +	.devs	= berlin2cd_ctrl_chip_ctrl_subdevs,
> +	.ndevs	= ARRAY_SIZE(berlin2cd_ctrl_chip_ctrl_subdevs),
> +};
> +
> +static const struct berlin_ctrl_priv berlin2cd_ctrl_system_data = {
> +	.devs	= berlin2cd_ctrl_system_ctrl_subdevs,
> +	.ndevs	= ARRAY_SIZE(berlin2cd_ctrl_system_ctrl_subdevs),
> +};
> +
> +/*
> + * BG2Q devices
> + */
> +static const struct mfd_cell berlin2q_ctrl_chip_ctrl_subdevs[] = {
> +	{
> +		.name		= "berlin2q-soc-pinctrl",
> +		.of_compatible	= "marvell,berlin2q-soc-pinctrl",
> +	},
> +	{
> +		.name		= "berlin2-reset",
> +		.of_compatible	= "marvell,berlin2-reset",
> +	},
> +};
> +
> +static const struct mfd_cell berlin2q_ctrl_system_ctrl_subdevs[] = {
> +	{
> +		.name		= "berlin2q-system-pinctrl",
> +		.of_compatible	= "marvell,berlin2q-system-pinctrl",
> +	},
> +};
> +
> +static const struct berlin_ctrl_priv berlin2q_ctrl_chip_ctrl_data = {
> +	.devs	= berlin2q_ctrl_chip_ctrl_subdevs,
> +	.ndevs	= ARRAY_SIZE(berlin2q_ctrl_chip_ctrl_subdevs),
> +};
> +
> +static const struct berlin_ctrl_priv berlin2q_ctrl_system_data = {
> +	.devs	= berlin2q_ctrl_system_ctrl_subdevs,
> +	.ndevs	= ARRAY_SIZE(berlin2q_ctrl_system_ctrl_subdevs),
> +};
> +
> +
> +static const struct of_device_id berlin_ctrl_of_match[] = {
> +	/* BG2 */
> +	{
> +		.compatible	= "marvell,berlin2-chip-ctrl",
> +		.data		= &berlin2_ctrl_chip_ctrl_data,
> +	},
> +	{
> +		.compatible	= "marvell,berlin2-system-ctrl",
> +		.data		= &berlin2_ctrl_system_data,
> +	},
> +	/* BG2CD */
> +	{
> +		.compatible	= "marvell,berlin2cd-chip-ctrl",
> +		.data		= &berlin2cd_ctrl_chip_ctrl_data,
> +	},
> +	{
> +		.compatible	= "marvell,berlin2cd-system-ctrl",
> +		.data		= &berlin2cd_ctrl_system_data,
> +	},
> +	/* BG2Q */
> +	{
> +		.compatible	= "marvell,berlin2q-chip-ctrl",
> +		.data		= &berlin2q_ctrl_chip_ctrl_data,
> +	},
> +	{
> +		.compatible	= "marvell,berlin2q-system-ctrl",
> +		.data		= &berlin2q_ctrl_system_data,
> +	},
> +	{ },
> +};
> +MODULE_DEVICE_TABLE(of, berlin_ctrl_of_match);
> +
> +static int berlin_ctrl_probe(struct platform_device *pdev)
> +{
> +	struct device *dev = &pdev->dev;
> +	const struct of_device_id *match;
> +	const struct berlin_ctrl_priv *priv;
> +	int ret;
> +
> +	match = of_match_node(berlin_ctrl_of_match, dev->of_node);
> +	if (!match)
> +		return -EINVAL;
> +
> +	priv = match->data;
> +
> +	ret = mfd_add_devices(dev, 0, priv->devs, priv->ndevs, NULL, -1, NULL);
> +	if (ret) {
> +		dev_err(dev, "Failed to add devices: %d\n", ret);
> +		return ret;
> +	}
> +
> +	return 0;
> +}

I'm not sure I see the point in this driver.  Why can't you just
register these devices directly from DT?

> +static struct platform_driver berlin_ctrl_driver = {
> +	.probe	= berlin_ctrl_probe,
> +	.driver	= {
> +		.name = "berlin-ctrl",
> +		.of_match_table = berlin_ctrl_of_match,
> +	},
> +};
> +module_platform_driver(berlin_ctrl_driver);
> +
> +MODULE_AUTHOR("Antoine Tenart <antoine.tenart@free-electrons.com>");
> +MODULE_DESCRIPTION("Marvell Berlin controller driver (mfd)");
> +MODULE_LICENSE("GPL");

v2

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org ? Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

Re: [PATCH 01/11] mfd: add the Berlin controller driver

[Antoine Tenart]

Lee,

On Mon, Feb 16, 2015 at 12:48:08PM +0000, Lee Jones wrote:
> On Wed, 11 Feb 2015, Antoine Tenart wrote:
> 
> > --- a/drivers/mfd/Kconfig
> > +++ b/drivers/mfd/Kconfig
> > @@ -840,6 +840,11 @@ config STMPE_SPI
> >  	  This is used to enable SPI interface of STMPE
> >  endmenu
> >  
> > +config MFD_BERLIN_CTRL
> > +	bool
> 
> Missing description.
> Why can't this driver be built as a module?

Well, this mfd driver registers various devices as the pinctrl and the
reset driver. I think we want the pinctrl driver to always be
registered.

IMHO we want this driver to always be selected when using a Berlin SoC.

> 
> > +	select MFD_CORE
> > +	select MFD_SYSCON
> 
> Missing help.
> 
> >  config MFD_STA2X11
> >  	bool "STMicroelectronics STA2X11"
> >  	depends on STA2X11
> > diff --git a/drivers/mfd/Makefile b/drivers/mfd/Makefile
> > index 53467e211381..adf60e85df20 100644
> > --- a/drivers/mfd/Makefile
> > +++ b/drivers/mfd/Makefile
> > @@ -179,3 +179,5 @@ obj-$(CONFIG_MFD_DLN2)		+= dln2.o
> >  
> >  intel-soc-pmic-objs		:= intel_soc_pmic_core.o intel_soc_pmic_crc.o
> >  obj-$(CONFIG_INTEL_SOC_PMIC)	+= intel-soc-pmic.o
> > +
> > +obj-$(CONFIG_MFD_BERLIN_CTRL)	+= berlin-ctrl.o
> > diff --git a/drivers/mfd/berlin-ctrl.c b/drivers/mfd/berlin-ctrl.c
> > new file mode 100644
> > index 000000000000..e3ce6f069f16
> > --- /dev/null
> > +++ b/drivers/mfd/berlin-ctrl.c
> > @@ -0,0 +1,180 @@
> > +/*
> > + * Copyright (C) 2015 Marvell Technology Group Ltd.
> > + *
> > + * Antoine Tenart <antoine.tenart@free-electrons.com>
> 
> Author: Antoine Tenart <antoine.tenart@free-electrons.com>

Hmmm, okay.

> 
> > +
> > +#include <linux/mfd/core.h>
> > +#include <linux/module.h>
> > +#include <linux/of.h>
> 
> kernel.h?

Is there a reason to add this header here?

> > +/*
> > + * BG2 devices
> > + */
> > +static const struct mfd_cell berlin2_ctrl_chip_ctrl_subdevs[] = {
> > +	{
> > +		.name		= "berlin2-soc-pinctrl",
> > +		.of_compatible	= "marvell,berlin2-soc-pinctrl",
> > +	},
> > +	{
> > +		.name		= "berlin2-reset",
> > +		.of_compatible	= "marvell,berlin2-reset",
> > +	},
> > +};
> > +
> > +static const struct mfd_cell berlin2_ctrl_system_ctrl_subdevs[] = {
> > +	{
> > +		.name		= "berlin2-system-pinctrl",
> > +		.of_compatible	= "marvell,berlin2-system-pinctrl",
> > +	},
> > +};
> > +
> > +static const struct berlin_ctrl_priv berlin2_ctrl_chip_ctrl_data = {
> > +	.devs	= berlin2_ctrl_chip_ctrl_subdevs,
> > +	.ndevs	= ARRAY_SIZE(berlin2_ctrl_chip_ctrl_subdevs),
> > +};
> > +
> > +static const struct berlin_ctrl_priv berlin2_ctrl_system_data = {
> > +	.devs	= berlin2_ctrl_system_ctrl_subdevs,
> > +	.ndevs	= ARRAY_SIZE(berlin2_ctrl_system_ctrl_subdevs),
> > +};
> > +
> > +
> 
> Superfluous '\n'

Sure.

> 
> > +
> > +static int berlin_ctrl_probe(struct platform_device *pdev)
> > +{
> > +	struct device *dev = &pdev->dev;
> > +	const struct of_device_id *match;
> > +	const struct berlin_ctrl_priv *priv;
> > +	int ret;
> > +
> > +	match = of_match_node(berlin_ctrl_of_match, dev->of_node);
> > +	if (!match)
> > +		return -EINVAL;
> > +
> > +	priv = match->data;
> > +
> > +	ret = mfd_add_devices(dev, 0, priv->devs, priv->ndevs, NULL, -1, NULL);
> > +	if (ret) {
> > +		dev_err(dev, "Failed to add devices: %d\n", ret);
> > +		return ret;
> > +	}
> > +
> > +	return 0;
> > +}
> 
> I'm not sure I see the point in this driver.  Why can't you just
> register these devices directly from DT?

All these devices share the same bank of registers and we previously
used a single node. But with many devices sharing a single node, this is
problematic to register all the devices from DT. Using this MFD driver
to do it is a proper solution in this case.

To provide a regmap to the devices' drivers we also use syscon on the
chip/system controller nodes.

> > +MODULE_LICENSE("GPL");
> 
> v2

"GPL" is a valid choice, quoting include/linux.module.h:

	"GPL"              [GNU Public License v2 or later]
	"GPL v2"           [GNU Public License v2]

Is there a reason you explicitly want to use GPLv2, and only GPLv2?

Antoine

-- 
Antoine Ténart, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com

[PATCH 01/11] mfd: add the Berlin controller driver

[Antoine Tenart]

Lee,

On Mon, Feb 16, 2015 at 12:48:08PM +0000, Lee Jones wrote:
> On Wed, 11 Feb 2015, Antoine Tenart wrote:
> 
> > --- a/drivers/mfd/Kconfig
> > +++ b/drivers/mfd/Kconfig
> > @@ -840,6 +840,11 @@ config STMPE_SPI
> >  	  This is used to enable SPI interface of STMPE
> >  endmenu
> >  
> > +config MFD_BERLIN_CTRL
> > +	bool
> 
> Missing description.
> Why can't this driver be built as a module?

Well, this mfd driver registers various devices as the pinctrl and the
reset driver. I think we want the pinctrl driver to always be
registered.

IMHO we want this driver to always be selected when using a Berlin SoC.

> 
> > +	select MFD_CORE
> > +	select MFD_SYSCON
> 
> Missing help.
> 
> >  config MFD_STA2X11
> >  	bool "STMicroelectronics STA2X11"
> >  	depends on STA2X11
> > diff --git a/drivers/mfd/Makefile b/drivers/mfd/Makefile
> > index 53467e211381..adf60e85df20 100644
> > --- a/drivers/mfd/Makefile
> > +++ b/drivers/mfd/Makefile
> > @@ -179,3 +179,5 @@ obj-$(CONFIG_MFD_DLN2)		+= dln2.o
> >  
> >  intel-soc-pmic-objs		:= intel_soc_pmic_core.o intel_soc_pmic_crc.o
> >  obj-$(CONFIG_INTEL_SOC_PMIC)	+= intel-soc-pmic.o
> > +
> > +obj-$(CONFIG_MFD_BERLIN_CTRL)	+= berlin-ctrl.o
> > diff --git a/drivers/mfd/berlin-ctrl.c b/drivers/mfd/berlin-ctrl.c
> > new file mode 100644
> > index 000000000000..e3ce6f069f16
> > --- /dev/null
> > +++ b/drivers/mfd/berlin-ctrl.c
> > @@ -0,0 +1,180 @@
> > +/*
> > + * Copyright (C) 2015 Marvell Technology Group Ltd.
> > + *
> > + * Antoine Tenart <antoine.tenart@free-electrons.com>
> 
> Author: Antoine Tenart <antoine.tenart@free-electrons.com>

Hmmm, okay.

> 
> > +
> > +#include <linux/mfd/core.h>
> > +#include <linux/module.h>
> > +#include <linux/of.h>
> 
> kernel.h?

Is there a reason to add this header here?

> > +/*
> > + * BG2 devices
> > + */
> > +static const struct mfd_cell berlin2_ctrl_chip_ctrl_subdevs[] = {
> > +	{
> > +		.name		= "berlin2-soc-pinctrl",
> > +		.of_compatible	= "marvell,berlin2-soc-pinctrl",
> > +	},
> > +	{
> > +		.name		= "berlin2-reset",
> > +		.of_compatible	= "marvell,berlin2-reset",
> > +	},
> > +};
> > +
> > +static const struct mfd_cell berlin2_ctrl_system_ctrl_subdevs[] = {
> > +	{
> > +		.name		= "berlin2-system-pinctrl",
> > +		.of_compatible	= "marvell,berlin2-system-pinctrl",
> > +	},
> > +};
> > +
> > +static const struct berlin_ctrl_priv berlin2_ctrl_chip_ctrl_data = {
> > +	.devs	= berlin2_ctrl_chip_ctrl_subdevs,
> > +	.ndevs	= ARRAY_SIZE(berlin2_ctrl_chip_ctrl_subdevs),
> > +};
> > +
> > +static const struct berlin_ctrl_priv berlin2_ctrl_system_data = {
> > +	.devs	= berlin2_ctrl_system_ctrl_subdevs,
> > +	.ndevs	= ARRAY_SIZE(berlin2_ctrl_system_ctrl_subdevs),
> > +};
> > +
> > +
> 
> Superfluous '\n'

Sure.

> 
> > +
> > +static int berlin_ctrl_probe(struct platform_device *pdev)
> > +{
> > +	struct device *dev = &pdev->dev;
> > +	const struct of_device_id *match;
> > +	const struct berlin_ctrl_priv *priv;
> > +	int ret;
> > +
> > +	match = of_match_node(berlin_ctrl_of_match, dev->of_node);
> > +	if (!match)
> > +		return -EINVAL;
> > +
> > +	priv = match->data;
> > +
> > +	ret = mfd_add_devices(dev, 0, priv->devs, priv->ndevs, NULL, -1, NULL);
> > +	if (ret) {
> > +		dev_err(dev, "Failed to add devices: %d\n", ret);
> > +		return ret;
> > +	}
> > +
> > +	return 0;
> > +}
> 
> I'm not sure I see the point in this driver.  Why can't you just
> register these devices directly from DT?

All these devices share the same bank of registers and we previously
used a single node. But with many devices sharing a single node, this is
problematic to register all the devices from DT. Using this MFD driver
to do it is a proper solution in this case.

To provide a regmap to the devices' drivers we also use syscon on the
chip/system controller nodes.

> > +MODULE_LICENSE("GPL");
> 
> v2

"GPL" is a valid choice, quoting include/linux.module.h:

	"GPL"              [GNU Public License v2 or later]
	"GPL v2"           [GNU Public License v2]

Is there a reason you explicitly want to use GPLv2, and only GPLv2?

Antoine

-- 
Antoine T?nart, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com

Re: [PATCH 01/11] mfd: add the Berlin controller driver

[Lee Jones]

On Tue, 17 Feb 2015, Antoine Tenart wrote:
> On Mon, Feb 16, 2015 at 12:48:08PM +0000, Lee Jones wrote:
> > On Wed, 11 Feb 2015, Antoine Tenart wrote:
> > 
> > > --- a/drivers/mfd/Kconfig
> > > +++ b/drivers/mfd/Kconfig
> > > @@ -840,6 +840,11 @@ config STMPE_SPI
> > >  	  This is used to enable SPI interface of STMPE
> > >  endmenu
> > >  
> > > +config MFD_BERLIN_CTRL
> > > +	bool
> > 
> > Missing description.
> > Why can't this driver be built as a module?
> 
> Well, this mfd driver registers various devices as the pinctrl and the
> reset driver. I think we want the pinctrl driver to always be
> registered.
> 
> IMHO we want this driver to always be selected when using a Berlin SoC.

[...]

> > > +#include <linux/mfd/core.h>
> > > +#include <linux/module.h>
> > > +#include <linux/of.h>
> > 
> > kernel.h?
> 
> Is there a reason to add this header here?

I guess not if you're not using any of its macros.  Although, I don't
recall every seeing s driver without it.  I guess you learn something
every day.

[...]

> > > +static int berlin_ctrl_probe(struct platform_device *pdev)
> > > +{
> > > +	struct device *dev = &pdev->dev;
> > > +	const struct of_device_id *match;
> > > +	const struct berlin_ctrl_priv *priv;
> > > +	int ret;
> > > +
> > > +	match = of_match_node(berlin_ctrl_of_match, dev->of_node);
> > > +	if (!match)
> > > +		return -EINVAL;
> > > +
> > > +	priv = match->data;
> > > +
> > > +	ret = mfd_add_devices(dev, 0, priv->devs, priv->ndevs, NULL, -1, NULL);
> > > +	if (ret) {
> > > +		dev_err(dev, "Failed to add devices: %d\n", ret);
> > > +		return ret;
> > > +	}
> > > +
> > > +	return 0;
> > > +}
> > 
> > I'm not sure I see the point in this driver.  Why can't you just
> > register these devices directly from DT?
> 
> All these devices share the same bank of registers and we previously
> used a single node. But with many devices sharing a single node, this is
> problematic to register all the devices from DT. Using this MFD driver
> to do it is a proper solution in this case.

Tell me more.  What are the problems you encountered?

> To provide a regmap to the devices' drivers we also use syscon on the
> chip/system controller nodes.
> 
> > > +MODULE_LICENSE("GPL");

I wonder if these are actually useful if you're always going to be
built-in? 

> > > +MODULE_LICENSE("GPL");
> > 
> > v2
> 
> "GPL" is a valid choice, quoting include/linux.module.h:
> 
> 	"GPL"              [GNU Public License v2 or later]
> 	"GPL v2"           [GNU Public License v2]
> 
> Is there a reason you explicitly want to use GPLv2, and only GPLv2?

Yes, there is disparity between this and the comment in the file
header.  I don't mind which one you amend, but a change is required.

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

[PATCH 01/11] mfd: add the Berlin controller driver

[Lee Jones]

On Tue, 17 Feb 2015, Antoine Tenart wrote:
> On Mon, Feb 16, 2015 at 12:48:08PM +0000, Lee Jones wrote:
> > On Wed, 11 Feb 2015, Antoine Tenart wrote:
> > 
> > > --- a/drivers/mfd/Kconfig
> > > +++ b/drivers/mfd/Kconfig
> > > @@ -840,6 +840,11 @@ config STMPE_SPI
> > >  	  This is used to enable SPI interface of STMPE
> > >  endmenu
> > >  
> > > +config MFD_BERLIN_CTRL
> > > +	bool
> > 
> > Missing description.
> > Why can't this driver be built as a module?
> 
> Well, this mfd driver registers various devices as the pinctrl and the
> reset driver. I think we want the pinctrl driver to always be
> registered.
> 
> IMHO we want this driver to always be selected when using a Berlin SoC.

[...]

> > > +#include <linux/mfd/core.h>
> > > +#include <linux/module.h>
> > > +#include <linux/of.h>
> > 
> > kernel.h?
> 
> Is there a reason to add this header here?

I guess not if you're not using any of its macros.  Although, I don't
recall every seeing s driver without it.  I guess you learn something
every day.

[...]

> > > +static int berlin_ctrl_probe(struct platform_device *pdev)
> > > +{
> > > +	struct device *dev = &pdev->dev;
> > > +	const struct of_device_id *match;
> > > +	const struct berlin_ctrl_priv *priv;
> > > +	int ret;
> > > +
> > > +	match = of_match_node(berlin_ctrl_of_match, dev->of_node);
> > > +	if (!match)
> > > +		return -EINVAL;
> > > +
> > > +	priv = match->data;
> > > +
> > > +	ret = mfd_add_devices(dev, 0, priv->devs, priv->ndevs, NULL, -1, NULL);
> > > +	if (ret) {
> > > +		dev_err(dev, "Failed to add devices: %d\n", ret);
> > > +		return ret;
> > > +	}
> > > +
> > > +	return 0;
> > > +}
> > 
> > I'm not sure I see the point in this driver.  Why can't you just
> > register these devices directly from DT?
> 
> All these devices share the same bank of registers and we previously
> used a single node. But with many devices sharing a single node, this is
> problematic to register all the devices from DT. Using this MFD driver
> to do it is a proper solution in this case.

Tell me more.  What are the problems you encountered?

> To provide a regmap to the devices' drivers we also use syscon on the
> chip/system controller nodes.
> 
> > > +MODULE_LICENSE("GPL");

I wonder if these are actually useful if you're always going to be
built-in? 

> > > +MODULE_LICENSE("GPL");
> > 
> > v2
> 
> "GPL" is a valid choice, quoting include/linux.module.h:
> 
> 	"GPL"              [GNU Public License v2 or later]
> 	"GPL v2"           [GNU Public License v2]
> 
> Is there a reason you explicitly want to use GPLv2, and only GPLv2?

Yes, there is disparity between this and the comment in the file
header.  I don't mind which one you amend, but a change is required.

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org ? Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

Re: [PATCH 01/11] mfd: add the Berlin controller driver

[Antoine Tenart]

On Tue, Feb 17, 2015 at 11:54:48AM +0000, Lee Jones wrote:
> On Tue, 17 Feb 2015, Antoine Tenart wrote:
> > On Mon, Feb 16, 2015 at 12:48:08PM +0000, Lee Jones wrote:
> > > On Wed, 11 Feb 2015, Antoine Tenart wrote:
> 
> > > > +static int berlin_ctrl_probe(struct platform_device *pdev)
> > > > +{
> > > > +	struct device *dev = &pdev->dev;
> > > > +	const struct of_device_id *match;
> > > > +	const struct berlin_ctrl_priv *priv;
> > > > +	int ret;
> > > > +
> > > > +	match = of_match_node(berlin_ctrl_of_match, dev->of_node);
> > > > +	if (!match)
> > > > +		return -EINVAL;
> > > > +
> > > > +	priv = match->data;
> > > > +
> > > > +	ret = mfd_add_devices(dev, 0, priv->devs, priv->ndevs, NULL, -1, NULL);
> > > > +	if (ret) {
> > > > +		dev_err(dev, "Failed to add devices: %d\n", ret);
> > > > +		return ret;
> > > > +	}
> > > > +
> > > > +	return 0;
> > > > +}
> > > 
> > > I'm not sure I see the point in this driver.  Why can't you just
> > > register these devices directly from DT?
> > 
> > All these devices share the same bank of registers and we previously
> > used a single node. But with many devices sharing a single node, this is
> > problematic to register all the devices from DT. Using this MFD driver
> > to do it is a proper solution in this case.
> 
> Tell me more.  What are the problems you encountered?

So we had a single node, chip-controller, accessed by multiple
devices -and drivers-. We ended up with:

chip: chip-control@ea0000 {
	compatible = "marvell,berlin2q-chip-ctrl";
	reg = <0xea0000 0x400>, <0xdd0170 0x10>;
	#clock-cells = <1>;
	#reset-cells = <2>;
	clocks = <&refclk>;
	clock-names = "refclk";

	[pinmux nodes]
};

In addition to being a mess, how can you probe various drivers with this
single node? We had to probe a clock driver in addition to the
pin-controller and reset drivers. We ended up using arch_initcall() in
the reset driver, which was *not* acceptable.

These chip and system controllers are not an IP, but helps not spreading
this bank of registers all over the DT.

The solution to this problem is to introduce an mtd driver which
registers all the sub-devices described by these chip and system
controller nodes.

Antoine

-- 
Antoine Ténart, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com

[PATCH 01/11] mfd: add the Berlin controller driver

[Antoine Tenart]

On Tue, Feb 17, 2015 at 11:54:48AM +0000, Lee Jones wrote:
> On Tue, 17 Feb 2015, Antoine Tenart wrote:
> > On Mon, Feb 16, 2015 at 12:48:08PM +0000, Lee Jones wrote:
> > > On Wed, 11 Feb 2015, Antoine Tenart wrote:
> 
> > > > +static int berlin_ctrl_probe(struct platform_device *pdev)
> > > > +{
> > > > +	struct device *dev = &pdev->dev;
> > > > +	const struct of_device_id *match;
> > > > +	const struct berlin_ctrl_priv *priv;
> > > > +	int ret;
> > > > +
> > > > +	match = of_match_node(berlin_ctrl_of_match, dev->of_node);
> > > > +	if (!match)
> > > > +		return -EINVAL;
> > > > +
> > > > +	priv = match->data;
> > > > +
> > > > +	ret = mfd_add_devices(dev, 0, priv->devs, priv->ndevs, NULL, -1, NULL);
> > > > +	if (ret) {
> > > > +		dev_err(dev, "Failed to add devices: %d\n", ret);
> > > > +		return ret;
> > > > +	}
> > > > +
> > > > +	return 0;
> > > > +}
> > > 
> > > I'm not sure I see the point in this driver.  Why can't you just
> > > register these devices directly from DT?
> > 
> > All these devices share the same bank of registers and we previously
> > used a single node. But with many devices sharing a single node, this is
> > problematic to register all the devices from DT. Using this MFD driver
> > to do it is a proper solution in this case.
> 
> Tell me more.  What are the problems you encountered?

So we had a single node, chip-controller, accessed by multiple
devices -and drivers-. We ended up with:

chip: chip-control at ea0000 {
	compatible = "marvell,berlin2q-chip-ctrl";
	reg = <0xea0000 0x400>, <0xdd0170 0x10>;
	#clock-cells = <1>;
	#reset-cells = <2>;
	clocks = <&refclk>;
	clock-names = "refclk";

	[pinmux nodes]
};

In addition to being a mess, how can you probe various drivers with this
single node? We had to probe a clock driver in addition to the
pin-controller and reset drivers. We ended up using arch_initcall() in
the reset driver, which was *not* acceptable.

These chip and system controllers are not an IP, but helps not spreading
this bank of registers all over the DT.

The solution to this problem is to introduce an mtd driver which
registers all the sub-devices described by these chip and system
controller nodes.

Antoine

-- 
Antoine T?nart, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com

Re: [PATCH 01/11] mfd: add the Berlin controller driver

[Lee Jones]

On Wed, 18 Feb 2015, Antoine Tenart wrote:

> On Tue, Feb 17, 2015 at 11:54:48AM +0000, Lee Jones wrote:
> > On Tue, 17 Feb 2015, Antoine Tenart wrote:
> > > On Mon, Feb 16, 2015 at 12:48:08PM +0000, Lee Jones wrote:
> > > > On Wed, 11 Feb 2015, Antoine Tenart wrote:
> > 
> > > > > +static int berlin_ctrl_probe(struct platform_device *pdev)
> > > > > +{
> > > > > +	struct device *dev = &pdev->dev;
> > > > > +	const struct of_device_id *match;
> > > > > +	const struct berlin_ctrl_priv *priv;
> > > > > +	int ret;
> > > > > +
> > > > > +	match = of_match_node(berlin_ctrl_of_match, dev->of_node);
> > > > > +	if (!match)
> > > > > +		return -EINVAL;
> > > > > +
> > > > > +	priv = match->data;
> > > > > +
> > > > > +	ret = mfd_add_devices(dev, 0, priv->devs, priv->ndevs, NULL, -1, NULL);
> > > > > +	if (ret) {
> > > > > +		dev_err(dev, "Failed to add devices: %d\n", ret);
> > > > > +		return ret;
> > > > > +	}
> > > > > +
> > > > > +	return 0;
> > > > > +}
> > > > 
> > > > I'm not sure I see the point in this driver.  Why can't you just
> > > > register these devices directly from DT?
> > > 
> > > All these devices share the same bank of registers and we previously
> > > used a single node. But with many devices sharing a single node, this is
> > > problematic to register all the devices from DT. Using this MFD driver
> > > to do it is a proper solution in this case.
> > 
> > Tell me more.  What are the problems you encountered?
> 
> So we had a single node, chip-controller, accessed by multiple
> devices -and drivers-. We ended up with:
> 
> chip: chip-control@ea0000 {
> 	compatible = "marvell,berlin2q-chip-ctrl";
> 	reg = <0xea0000 0x400>, <0xdd0170 0x10>;
> 	#clock-cells = <1>;
> 	#reset-cells = <2>;
> 	clocks = <&refclk>;
> 	clock-names = "refclk";
> 
> 	[pinmux nodes]
> };
> 
> In addition to being a mess, how can you probe various drivers with this
> single node? We had to probe a clock driver in addition to the
> pin-controller and reset drivers. We ended up using arch_initcall() in
> the reset driver, which was *not* acceptable.
> 
> These chip and system controllers are not an IP, but helps not spreading
> this bank of registers all over the DT.
> 
> The solution to this problem is to introduce an mtd driver which
> registers all the sub-devices described by these chip and system
> controller nodes.

I'm still not convinced that your problem can't be solved in DT, but
creating a single psudo-hardware node is not correct either.  What
does the h/w _really_ look like?  Is all of this stuff on a single
chip?  If so, I would expect to see something like:

control@ea0000 {
	compatible = "marvel,control";

	pinctrl@xxxxx {
		compatible = "marvel,pinctrl";
	};

	reset@xxxxx {
		compatible = "marvel,reset";
	};
};

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

[PATCH 01/11] mfd: add the Berlin controller driver

[Lee Jones]

On Wed, 18 Feb 2015, Antoine Tenart wrote:

> On Tue, Feb 17, 2015 at 11:54:48AM +0000, Lee Jones wrote:
> > On Tue, 17 Feb 2015, Antoine Tenart wrote:
> > > On Mon, Feb 16, 2015 at 12:48:08PM +0000, Lee Jones wrote:
> > > > On Wed, 11 Feb 2015, Antoine Tenart wrote:
> > 
> > > > > +static int berlin_ctrl_probe(struct platform_device *pdev)
> > > > > +{
> > > > > +	struct device *dev = &pdev->dev;
> > > > > +	const struct of_device_id *match;
> > > > > +	const struct berlin_ctrl_priv *priv;
> > > > > +	int ret;
> > > > > +
> > > > > +	match = of_match_node(berlin_ctrl_of_match, dev->of_node);
> > > > > +	if (!match)
> > > > > +		return -EINVAL;
> > > > > +
> > > > > +	priv = match->data;
> > > > > +
> > > > > +	ret = mfd_add_devices(dev, 0, priv->devs, priv->ndevs, NULL, -1, NULL);
> > > > > +	if (ret) {
> > > > > +		dev_err(dev, "Failed to add devices: %d\n", ret);
> > > > > +		return ret;
> > > > > +	}
> > > > > +
> > > > > +	return 0;
> > > > > +}
> > > > 
> > > > I'm not sure I see the point in this driver.  Why can't you just
> > > > register these devices directly from DT?
> > > 
> > > All these devices share the same bank of registers and we previously
> > > used a single node. But with many devices sharing a single node, this is
> > > problematic to register all the devices from DT. Using this MFD driver
> > > to do it is a proper solution in this case.
> > 
> > Tell me more.  What are the problems you encountered?
> 
> So we had a single node, chip-controller, accessed by multiple
> devices -and drivers-. We ended up with:
> 
> chip: chip-control at ea0000 {
> 	compatible = "marvell,berlin2q-chip-ctrl";
> 	reg = <0xea0000 0x400>, <0xdd0170 0x10>;
> 	#clock-cells = <1>;
> 	#reset-cells = <2>;
> 	clocks = <&refclk>;
> 	clock-names = "refclk";
> 
> 	[pinmux nodes]
> };
> 
> In addition to being a mess, how can you probe various drivers with this
> single node? We had to probe a clock driver in addition to the
> pin-controller and reset drivers. We ended up using arch_initcall() in
> the reset driver, which was *not* acceptable.
> 
> These chip and system controllers are not an IP, but helps not spreading
> this bank of registers all over the DT.
> 
> The solution to this problem is to introduce an mtd driver which
> registers all the sub-devices described by these chip and system
> controller nodes.

I'm still not convinced that your problem can't be solved in DT, but
creating a single psudo-hardware node is not correct either.  What
does the h/w _really_ look like?  Is all of this stuff on a single
chip?  If so, I would expect to see something like:

control at ea0000 {
	compatible = "marvel,control";

	pinctrl at xxxxx {
		compatible = "marvel,pinctrl";
	};

	reset at xxxxx {
		compatible = "marvel,reset";
	};
};

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org ? Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

Re: [PATCH 01/11] mfd: add the Berlin controller driver

[Antoine Tenart]

On Wed, Feb 18, 2015 at 09:09:58AM +0000, Lee Jones wrote:
> On Wed, 18 Feb 2015, Antoine Tenart wrote:
> 
> > On Tue, Feb 17, 2015 at 11:54:48AM +0000, Lee Jones wrote:
> > > On Tue, 17 Feb 2015, Antoine Tenart wrote:
> > > > On Mon, Feb 16, 2015 at 12:48:08PM +0000, Lee Jones wrote:
> > > > > On Wed, 11 Feb 2015, Antoine Tenart wrote:
> > > 
> > > > > > +static int berlin_ctrl_probe(struct platform_device *pdev)
> > > > > > +{
> > > > > > +	struct device *dev = &pdev->dev;
> > > > > > +	const struct of_device_id *match;
> > > > > > +	const struct berlin_ctrl_priv *priv;
> > > > > > +	int ret;
> > > > > > +
> > > > > > +	match = of_match_node(berlin_ctrl_of_match, dev->of_node);
> > > > > > +	if (!match)
> > > > > > +		return -EINVAL;
> > > > > > +
> > > > > > +	priv = match->data;
> > > > > > +
> > > > > > +	ret = mfd_add_devices(dev, 0, priv->devs, priv->ndevs, NULL, -1, NULL);
> > > > > > +	if (ret) {
> > > > > > +		dev_err(dev, "Failed to add devices: %d\n", ret);
> > > > > > +		return ret;
> > > > > > +	}
> > > > > > +
> > > > > > +	return 0;
> > > > > > +}
> > > > > 
> > > > > I'm not sure I see the point in this driver.  Why can't you just
> > > > > register these devices directly from DT?
> > > > 
> > > > All these devices share the same bank of registers and we previously
> > > > used a single node. But with many devices sharing a single node, this is
> > > > problematic to register all the devices from DT. Using this MFD driver
> > > > to do it is a proper solution in this case.
> > > 
> > > Tell me more.  What are the problems you encountered?
> > 
> > So we had a single node, chip-controller, accessed by multiple
> > devices -and drivers-. We ended up with:
> > 
> > chip: chip-control@ea0000 {
> > 	compatible = "marvell,berlin2q-chip-ctrl";
> > 	reg = <0xea0000 0x400>, <0xdd0170 0x10>;
> > 	#clock-cells = <1>;
> > 	#reset-cells = <2>;
> > 	clocks = <&refclk>;
> > 	clock-names = "refclk";
> > 
> > 	[pinmux nodes]
> > };
> > 
> > In addition to being a mess, how can you probe various drivers with this
> > single node? We had to probe a clock driver in addition to the
> > pin-controller and reset drivers. We ended up using arch_initcall() in
> > the reset driver, which was *not* acceptable.
> > 
> > These chip and system controllers are not an IP, but helps not spreading
> > this bank of registers all over the DT.
> > 
> > The solution to this problem is to introduce an mtd driver which
> > registers all the sub-devices described by these chip and system
> > controller nodes.
> 
> I'm still not convinced that your problem can't be solved in DT, but
> creating a single psudo-hardware node is not correct either.  What
> does the h/w _really_ look like?  Is all of this stuff on a single
> chip? 

There is no specific IP for these registers, but we do not want them
spread all over the DT nodes so they're summed up into this chip node.

SO we have all those registers into a chip/system node and sub-nodes for
the devices using them.

> If so, I would expect to see something like:
> 
> control@ea0000 {
> 	compatible = "marvel,control";
> 
> 	pinctrl@xxxxx {
> 		compatible = "marvel,pinctrl";
> 	};
> 
> 	reset@xxxxx {
> 		compatible = "marvel,reset";
> 	};
> };

That's exactly the point of this series: having one sub-node per device.

With this series applied, we have (the clock being a sub-node of the
chip-controller node is part of another series following this one):

chip: chip-controller@ea0000 {
        compatible = "marvell,berlin2q-chip-ctrl", "syscon";
        reg = <0xea0000 0x400>, <0xdd0170 0x10>;
        #clock-cells = <1>;
        clocks = <&refclk>;
        clock-names = "refclk";

        soc_pinctrl: pin-controller {
                compatible = "marvell,berlin2q-soc-pinctrl";

                twsi0_pmux: twsi0-pmux {
                        groups = "G6";
                        function = "twsi0";
                };

                twsi1_pmux: twsi1-pmux {
                        groups = "G7";
                        function = "twsi1";
                };
        };

        chip_rst: reset {
                compatible = "marvell,berlin2-reset";
                #reset-cells = <2>;
        };
};

Antoine

-- 
Antoine Ténart, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com

[PATCH 01/11] mfd: add the Berlin controller driver

[Antoine Tenart]

On Wed, Feb 18, 2015 at 09:09:58AM +0000, Lee Jones wrote:
> On Wed, 18 Feb 2015, Antoine Tenart wrote:
> 
> > On Tue, Feb 17, 2015 at 11:54:48AM +0000, Lee Jones wrote:
> > > On Tue, 17 Feb 2015, Antoine Tenart wrote:
> > > > On Mon, Feb 16, 2015 at 12:48:08PM +0000, Lee Jones wrote:
> > > > > On Wed, 11 Feb 2015, Antoine Tenart wrote:
> > > 
> > > > > > +static int berlin_ctrl_probe(struct platform_device *pdev)
> > > > > > +{
> > > > > > +	struct device *dev = &pdev->dev;
> > > > > > +	const struct of_device_id *match;
> > > > > > +	const struct berlin_ctrl_priv *priv;
> > > > > > +	int ret;
> > > > > > +
> > > > > > +	match = of_match_node(berlin_ctrl_of_match, dev->of_node);
> > > > > > +	if (!match)
> > > > > > +		return -EINVAL;
> > > > > > +
> > > > > > +	priv = match->data;
> > > > > > +
> > > > > > +	ret = mfd_add_devices(dev, 0, priv->devs, priv->ndevs, NULL, -1, NULL);
> > > > > > +	if (ret) {
> > > > > > +		dev_err(dev, "Failed to add devices: %d\n", ret);
> > > > > > +		return ret;
> > > > > > +	}
> > > > > > +
> > > > > > +	return 0;
> > > > > > +}
> > > > > 
> > > > > I'm not sure I see the point in this driver.  Why can't you just
> > > > > register these devices directly from DT?
> > > > 
> > > > All these devices share the same bank of registers and we previously
> > > > used a single node. But with many devices sharing a single node, this is
> > > > problematic to register all the devices from DT. Using this MFD driver
> > > > to do it is a proper solution in this case.
> > > 
> > > Tell me more.  What are the problems you encountered?
> > 
> > So we had a single node, chip-controller, accessed by multiple
> > devices -and drivers-. We ended up with:
> > 
> > chip: chip-control at ea0000 {
> > 	compatible = "marvell,berlin2q-chip-ctrl";
> > 	reg = <0xea0000 0x400>, <0xdd0170 0x10>;
> > 	#clock-cells = <1>;
> > 	#reset-cells = <2>;
> > 	clocks = <&refclk>;
> > 	clock-names = "refclk";
> > 
> > 	[pinmux nodes]
> > };
> > 
> > In addition to being a mess, how can you probe various drivers with this
> > single node? We had to probe a clock driver in addition to the
> > pin-controller and reset drivers. We ended up using arch_initcall() in
> > the reset driver, which was *not* acceptable.
> > 
> > These chip and system controllers are not an IP, but helps not spreading
> > this bank of registers all over the DT.
> > 
> > The solution to this problem is to introduce an mtd driver which
> > registers all the sub-devices described by these chip and system
> > controller nodes.
> 
> I'm still not convinced that your problem can't be solved in DT, but
> creating a single psudo-hardware node is not correct either.  What
> does the h/w _really_ look like?  Is all of this stuff on a single
> chip? 

There is no specific IP for these registers, but we do not want them
spread all over the DT nodes so they're summed up into this chip node.

SO we have all those registers into a chip/system node and sub-nodes for
the devices using them.

> If so, I would expect to see something like:
> 
> control at ea0000 {
> 	compatible = "marvel,control";
> 
> 	pinctrl at xxxxx {
> 		compatible = "marvel,pinctrl";
> 	};
> 
> 	reset at xxxxx {
> 		compatible = "marvel,reset";
> 	};
> };

That's exactly the point of this series: having one sub-node per device.

With this series applied, we have (the clock being a sub-node of the
chip-controller node is part of another series following this one):

chip: chip-controller at ea0000 {
        compatible = "marvell,berlin2q-chip-ctrl", "syscon";
        reg = <0xea0000 0x400>, <0xdd0170 0x10>;
        #clock-cells = <1>;
        clocks = <&refclk>;
        clock-names = "refclk";

        soc_pinctrl: pin-controller {
                compatible = "marvell,berlin2q-soc-pinctrl";

                twsi0_pmux: twsi0-pmux {
                        groups = "G6";
                        function = "twsi0";
                };

                twsi1_pmux: twsi1-pmux {
                        groups = "G7";
                        function = "twsi1";
                };
        };

        chip_rst: reset {
                compatible = "marvell,berlin2-reset";
                #reset-cells = <2>;
        };
};

Antoine

-- 
Antoine T?nart, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com

Re: [PATCH 01/11] mfd: add the Berlin controller driver

[Sebastian Hesselbarth]

On 18.02.2015 10:09, Lee Jones wrote:
> On Wed, 18 Feb 2015, Antoine Tenart wrote:
[...]
>> So we had a single node, chip-controller, accessed by multiple
>> devices -and drivers-. We ended up with:
>>
>> chip: chip-control@ea0000 {
>> 	compatible = "marvell,berlin2q-chip-ctrl";
>> 	reg = <0xea0000 0x400>, <0xdd0170 0x10>;
>> 	#clock-cells = <1>;
>> 	#reset-cells = <2>;
>> 	clocks = <&refclk>;
>> 	clock-names = "refclk";
>>
>> 	[pinmux nodes]
>> };
>>
>> In addition to being a mess, how can you probe various drivers with this
>> single node? We had to probe a clock driver in addition to the
>> pin-controller and reset drivers. We ended up using arch_initcall() in
>> the reset driver, which was *not* acceptable.
>>
>> These chip and system controllers are not an IP, but helps not spreading
>> this bank of registers all over the DT.
>>
>> The solution to this problem is to introduce an mtd driver which
>> registers all the sub-devices described by these chip and system
>> controller nodes.
>
> I'm still not convinced that your problem can't be solved in DT, but
> creating a single psudo-hardware node is not correct either.  What
> does the h/w _really_ look like?  Is all of this stuff on a single
> chip?  If so, I would expect to see something like:
>
> control@ea0000 {
> 	compatible = "marvel,control";
>
> 	pinctrl@xxxxx {
> 		compatible = "marvel,pinctrl";
> 	};
>
> 	reset@xxxxx {
> 		compatible = "marvel,reset";
> 	};
> };

Lee,

I guess you never get what you expect ;) Honestly, Antoine is right.

The structure you describe above is a bus and that is definitely not
true for chip control registers. Also, clock, reset, and pinctrl
"units" are SW concepts that HW engineers don't care much about.
Each of the "units" is heavily spread among the chip control registers
and even within a single register.

We simply give up on carving out every single register range to squeeze
them into SW driven units. I think Marvell understood that this kind of
chip control dumpster puts a high burden on the SW guys and newer SoCs
will have a cleaner register set - but for the current SoCs we are stuck
with the situation.

So, what _sane_ options do we have in DT that is also sane for SW?

(a) We could follow your suggestion of a chipctrl bus or move the
nodes back to the SoC bus. Now with that we would end up with
reg = <0x000 0x4>, <0x024 0x10>, <0x78 0x8>, ...
for each of the nodes. Plus, we'll certainly have to overlap some of
the reg ranges because bit0 of one register is reset but bits 31:1 is
clock. This not only exposes the mess in DT but still we have to deal
with it in the driver. Also as soon as we overlap, we loose devm_ for
the ioremap.

(b) We follow Antoine's proposal and have a single chip-ctrl node with
a single reg property spanning over the whole register set. Then have
clock, pinctrl, reset as sub-nodes. Looks sane, but we need some SW
that hooks up to each of the nodes, i.e. as it is not a bus we have to
register devices for each sub-node ourselves. That is why mfd is used
here (and for sunxi SoCs BTW too).

So, back when we wrote the clock driver and tried to find a sane
representation of the corresponding registers, Mike finally suggested
to just have a single node and deal with the register mess in the
driver. This series goes a little further and provides a single regmap
to all sub-drivers that we can think of that have to deal with chip
ctrl registers.

We have looked at the register layout and corresponding driver concepts
over and over again - there is no way to chop this up.

Sebastian

[PATCH 01/11] mfd: add the Berlin controller driver

[Sebastian Hesselbarth]

On 18.02.2015 10:09, Lee Jones wrote:
> On Wed, 18 Feb 2015, Antoine Tenart wrote:
[...]
>> So we had a single node, chip-controller, accessed by multiple
>> devices -and drivers-. We ended up with:
>>
>> chip: chip-control at ea0000 {
>> 	compatible = "marvell,berlin2q-chip-ctrl";
>> 	reg = <0xea0000 0x400>, <0xdd0170 0x10>;
>> 	#clock-cells = <1>;
>> 	#reset-cells = <2>;
>> 	clocks = <&refclk>;
>> 	clock-names = "refclk";
>>
>> 	[pinmux nodes]
>> };
>>
>> In addition to being a mess, how can you probe various drivers with this
>> single node? We had to probe a clock driver in addition to the
>> pin-controller and reset drivers. We ended up using arch_initcall() in
>> the reset driver, which was *not* acceptable.
>>
>> These chip and system controllers are not an IP, but helps not spreading
>> this bank of registers all over the DT.
>>
>> The solution to this problem is to introduce an mtd driver which
>> registers all the sub-devices described by these chip and system
>> controller nodes.
>
> I'm still not convinced that your problem can't be solved in DT, but
> creating a single psudo-hardware node is not correct either.  What
> does the h/w _really_ look like?  Is all of this stuff on a single
> chip?  If so, I would expect to see something like:
>
> control at ea0000 {
> 	compatible = "marvel,control";
>
> 	pinctrl at xxxxx {
> 		compatible = "marvel,pinctrl";
> 	};
>
> 	reset at xxxxx {
> 		compatible = "marvel,reset";
> 	};
> };

Lee,

I guess you never get what you expect ;) Honestly, Antoine is right.

The structure you describe above is a bus and that is definitely not
true for chip control registers. Also, clock, reset, and pinctrl
"units" are SW concepts that HW engineers don't care much about.
Each of the "units" is heavily spread among the chip control registers
and even within a single register.

We simply give up on carving out every single register range to squeeze
them into SW driven units. I think Marvell understood that this kind of
chip control dumpster puts a high burden on the SW guys and newer SoCs
will have a cleaner register set - but for the current SoCs we are stuck
with the situation.

So, what _sane_ options do we have in DT that is also sane for SW?

(a) We could follow your suggestion of a chipctrl bus or move the
nodes back to the SoC bus. Now with that we would end up with
reg = <0x000 0x4>, <0x024 0x10>, <0x78 0x8>, ...
for each of the nodes. Plus, we'll certainly have to overlap some of
the reg ranges because bit0 of one register is reset but bits 31:1 is
clock. This not only exposes the mess in DT but still we have to deal
with it in the driver. Also as soon as we overlap, we loose devm_ for
the ioremap.

(b) We follow Antoine's proposal and have a single chip-ctrl node with
a single reg property spanning over the whole register set. Then have
clock, pinctrl, reset as sub-nodes. Looks sane, but we need some SW
that hooks up to each of the nodes, i.e. as it is not a bus we have to
register devices for each sub-node ourselves. That is why mfd is used
here (and for sunxi SoCs BTW too).

So, back when we wrote the clock driver and tried to find a sane
representation of the corresponding registers, Mike finally suggested
to just have a single node and deal with the register mess in the
driver. This series goes a little further and provides a single regmap
to all sub-drivers that we can think of that have to deal with chip
ctrl registers.

We have looked at the register layout and corresponding driver concepts
over and over again - there is no way to chop this up.

Sebastian

Re: [PATCH 09/11] ARM: berlin: rework chip and system controller nodes for BG2

[Sebastian Hesselbarth]

On 11.02.2015 17:15, Antoine Tenart wrote:
> The chip and system controller nodes are now handled by the Berlin
> controller mfd driver. Its sub-devices are then registered by the mfd
> driver and let the drivers be probed properly, using their own
> sub-nodes.
>
> Rework the device tree to take this changes into account.
>
> Signed-off-by: Antoine Tenart <antoine.tenart@free-electrons.com>
> ---
[...]
> -		chip: chip-control@ea0000 {
> -			compatible = "marvell,berlin2-chip-ctrl";
> -			#clock-cells = <1>;
> -			#reset-cells = <2>;
> +		chip: chip-controller@ea0000 {
> +			compatible = "marvell,berlin2-chip-ctrl", "syscon";
>   			reg = <0xea0000 0x400>;
> +			#clock-cells = <1>;

Antoine,

I noticed just now, but we should either have all of clock, reset,
pinctrl as sub-nodes or none. Currently, this has pinctrl and reset
as sub-nodes but clock hooked up to chip-controller.

Sebastian

>   			clocks = <&refclk>;
>   			clock-names = "refclk";
>
> -			emmc_pmux: emmc-pmux {
> -				groups = "G26";
> -				function = "emmc";
> +			soc_pinctrl: pin-controller {
> +				compatible = "marvell,berlin2-soc-pinctrl";
> +
> +				emmc_pmux: emmc-pmux {
> +					groups = "G26";
> +					function = "emmc";
> +				};
> +			};
> +
> +			chip_rst: reset {
> +				compatible = "marvell,berlin2-reset";
> +				#reset-cells = <2>;
>   			};
>   		};
>
> @@ -442,22 +450,26 @@
>   			};
>
>   			sysctrl: system-controller@d000 {
> -				compatible = "marvell,berlin2-system-ctrl";
> +				compatible = "marvell,berlin2-system-ctrl", "syscon";
>   				reg = <0xd000 0x100>;
>
> -				uart0_pmux: uart0-pmux {
> -					groups = "GSM4";
> -					function = "uart0";
> -				};
> +				sys_pinctrl: pin-controller {
> +					compatible = "marvell,berlin2-system-pinctrl";
>
> -				uart1_pmux: uart1-pmux {
> -					groups = "GSM5";
> -					function = "uart1";
> -				};
> +					uart0_pmux: uart0-pmux {
> +						groups = "GSM4";
> +						function = "uart0";
> +					};
> +
> +					uart1_pmux: uart1-pmux {
> +						groups = "GSM5";
> +						function = "uart1";
> +					};
>
> -				uart2_pmux: uart2-pmux {
> -					groups = "GSM3";
> -					function = "uart2";
> +					uart2_pmux: uart2-pmux {
> +						groups = "GSM3";
> +						function = "uart2";
> +					};
>   				};
>   			};
>
>

[PATCH 09/11] ARM: berlin: rework chip and system controller nodes for BG2

[Sebastian Hesselbarth]

On 11.02.2015 17:15, Antoine Tenart wrote:
> The chip and system controller nodes are now handled by the Berlin
> controller mfd driver. Its sub-devices are then registered by the mfd
> driver and let the drivers be probed properly, using their own
> sub-nodes.
>
> Rework the device tree to take this changes into account.
>
> Signed-off-by: Antoine Tenart <antoine.tenart@free-electrons.com>
> ---
[...]
> -		chip: chip-control at ea0000 {
> -			compatible = "marvell,berlin2-chip-ctrl";
> -			#clock-cells = <1>;
> -			#reset-cells = <2>;
> +		chip: chip-controller at ea0000 {
> +			compatible = "marvell,berlin2-chip-ctrl", "syscon";
>   			reg = <0xea0000 0x400>;
> +			#clock-cells = <1>;

Antoine,

I noticed just now, but we should either have all of clock, reset,
pinctrl as sub-nodes or none. Currently, this has pinctrl and reset
as sub-nodes but clock hooked up to chip-controller.

Sebastian

>   			clocks = <&refclk>;
>   			clock-names = "refclk";
>
> -			emmc_pmux: emmc-pmux {
> -				groups = "G26";
> -				function = "emmc";
> +			soc_pinctrl: pin-controller {
> +				compatible = "marvell,berlin2-soc-pinctrl";
> +
> +				emmc_pmux: emmc-pmux {
> +					groups = "G26";
> +					function = "emmc";
> +				};
> +			};
> +
> +			chip_rst: reset {
> +				compatible = "marvell,berlin2-reset";
> +				#reset-cells = <2>;
>   			};
>   		};
>
> @@ -442,22 +450,26 @@
>   			};
>
>   			sysctrl: system-controller at d000 {
> -				compatible = "marvell,berlin2-system-ctrl";
> +				compatible = "marvell,berlin2-system-ctrl", "syscon";
>   				reg = <0xd000 0x100>;
>
> -				uart0_pmux: uart0-pmux {
> -					groups = "GSM4";
> -					function = "uart0";
> -				};
> +				sys_pinctrl: pin-controller {
> +					compatible = "marvell,berlin2-system-pinctrl";
>
> -				uart1_pmux: uart1-pmux {
> -					groups = "GSM5";
> -					function = "uart1";
> -				};
> +					uart0_pmux: uart0-pmux {
> +						groups = "GSM4";
> +						function = "uart0";
> +					};
> +
> +					uart1_pmux: uart1-pmux {
> +						groups = "GSM5";
> +						function = "uart1";
> +					};
>
> -				uart2_pmux: uart2-pmux {
> -					groups = "GSM3";
> -					function = "uart2";
> +					uart2_pmux: uart2-pmux {
> +						groups = "GSM3";
> +						function = "uart2";
> +					};
>   				};
>   			};
>
>

Re: [PATCH 09/11] ARM: berlin: rework chip and system controller nodes for BG2

[Antoine Tenart]

Hi Sebastian!

On Wed, Feb 18, 2015 at 11:29:50AM +0100, Sebastian Hesselbarth wrote:
> On 11.02.2015 17:15, Antoine Tenart wrote:
> [...]
> >-		chip: chip-control@ea0000 {
> >-			compatible = "marvell,berlin2-chip-ctrl";
> >-			#clock-cells = <1>;
> >-			#reset-cells = <2>;
> >+		chip: chip-controller@ea0000 {
> >+			compatible = "marvell,berlin2-chip-ctrl", "syscon";
> >  			reg = <0xea0000 0x400>;
> >+			#clock-cells = <1>;
> 
> I noticed just now, but we should either have all of clock, reset,
> pinctrl as sub-nodes or none. Currently, this has pinctrl and reset
> as sub-nodes but clock hooked up to chip-controller.

Of course. The clock rework is part of the other series[1] which
modifies the clock driver to use regmap but also move the clock into
its own sub-node.

Antoine

[1] https://lkml.org/lkml/2015/2/13/252

-- 
Antoine Ténart, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com

[PATCH 09/11] ARM: berlin: rework chip and system controller nodes for BG2

[Antoine Tenart]

Hi Sebastian!

On Wed, Feb 18, 2015 at 11:29:50AM +0100, Sebastian Hesselbarth wrote:
> On 11.02.2015 17:15, Antoine Tenart wrote:
> [...]
> >-		chip: chip-control at ea0000 {
> >-			compatible = "marvell,berlin2-chip-ctrl";
> >-			#clock-cells = <1>;
> >-			#reset-cells = <2>;
> >+		chip: chip-controller at ea0000 {
> >+			compatible = "marvell,berlin2-chip-ctrl", "syscon";
> >  			reg = <0xea0000 0x400>;
> >+			#clock-cells = <1>;
> 
> I noticed just now, but we should either have all of clock, reset,
> pinctrl as sub-nodes or none. Currently, this has pinctrl and reset
> as sub-nodes but clock hooked up to chip-controller.

Of course. The clock rework is part of the other series[1] which
modifies the clock driver to use regmap but also move the clock into
its own sub-node.

Antoine

[1] https://lkml.org/lkml/2015/2/13/252

-- 
Antoine T?nart, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com

Re: [PATCH 09/11] ARM: berlin: rework chip and system controller nodes for BG2

[Sebastian Hesselbarth]

On 18.02.2015 11:33, Antoine Tenart wrote:
> On Wed, Feb 18, 2015 at 11:29:50AM +0100, Sebastian Hesselbarth wrote:
>> On 11.02.2015 17:15, Antoine Tenart wrote:
>> [...]
>>> -		chip: chip-control@ea0000 {
>>> -			compatible = "marvell,berlin2-chip-ctrl";
>>> -			#clock-cells = <1>;
>>> -			#reset-cells = <2>;
>>> +		chip: chip-controller@ea0000 {
>>> +			compatible = "marvell,berlin2-chip-ctrl", "syscon";
>>>   			reg = <0xea0000 0x400>;
>>> +			#clock-cells = <1>;
>>
>> I noticed just now, but we should either have all of clock, reset,
>> pinctrl as sub-nodes or none. Currently, this has pinctrl and reset
>> as sub-nodes but clock hooked up to chip-controller.
>
> Of course. The clock rework is part of the other series[1] which
> modifies the clock driver to use regmap but also move the clock into
> its own sub-node.

Ah, ok. I misinterpreted the addition of #clock-cells above, but it
gets removed and re-added again.

Sebastian

[PATCH 09/11] ARM: berlin: rework chip and system controller nodes for BG2

[Sebastian Hesselbarth]

On 18.02.2015 11:33, Antoine Tenart wrote:
> On Wed, Feb 18, 2015 at 11:29:50AM +0100, Sebastian Hesselbarth wrote:
>> On 11.02.2015 17:15, Antoine Tenart wrote:
>> [...]
>>> -		chip: chip-control at ea0000 {
>>> -			compatible = "marvell,berlin2-chip-ctrl";
>>> -			#clock-cells = <1>;
>>> -			#reset-cells = <2>;
>>> +		chip: chip-controller at ea0000 {
>>> +			compatible = "marvell,berlin2-chip-ctrl", "syscon";
>>>   			reg = <0xea0000 0x400>;
>>> +			#clock-cells = <1>;
>>
>> I noticed just now, but we should either have all of clock, reset,
>> pinctrl as sub-nodes or none. Currently, this has pinctrl and reset
>> as sub-nodes but clock hooked up to chip-controller.
>
> Of course. The clock rework is part of the other series[1] which
> modifies the clock driver to use regmap but also move the clock into
> its own sub-node.

Ah, ok. I misinterpreted the addition of #clock-cells above, but it
gets removed and re-added again.

Sebastian

Re: [PATCH 01/11] mfd: add the Berlin controller driver

[Lee Jones]

On Wed, 18 Feb 2015, Antoine Tenart wrote:

> On Wed, Feb 18, 2015 at 09:09:58AM +0000, Lee Jones wrote:
> > On Wed, 18 Feb 2015, Antoine Tenart wrote:
> > 
> > > On Tue, Feb 17, 2015 at 11:54:48AM +0000, Lee Jones wrote:
> > > > On Tue, 17 Feb 2015, Antoine Tenart wrote:
> > > > > On Mon, Feb 16, 2015 at 12:48:08PM +0000, Lee Jones wrote:
> > > > > > On Wed, 11 Feb 2015, Antoine Tenart wrote:
> > > > 
> > > > > > > +static int berlin_ctrl_probe(struct platform_device *pdev)
> > > > > > > +{
> > > > > > > +	struct device *dev = &pdev->dev;
> > > > > > > +	const struct of_device_id *match;
> > > > > > > +	const struct berlin_ctrl_priv *priv;
> > > > > > > +	int ret;
> > > > > > > +
> > > > > > > +	match = of_match_node(berlin_ctrl_of_match, dev->of_node);
> > > > > > > +	if (!match)
> > > > > > > +		return -EINVAL;
> > > > > > > +
> > > > > > > +	priv = match->data;
> > > > > > > +
> > > > > > > +	ret = mfd_add_devices(dev, 0, priv->devs, priv->ndevs, NULL, -1, NULL);
> > > > > > > +	if (ret) {
> > > > > > > +		dev_err(dev, "Failed to add devices: %d\n", ret);
> > > > > > > +		return ret;
> > > > > > > +	}
> > > > > > > +
> > > > > > > +	return 0;
> > > > > > > +}
> > > > > > 
> > > > > > I'm not sure I see the point in this driver.  Why can't you just
> > > > > > register these devices directly from DT?
> > > > > 
> > > > > All these devices share the same bank of registers and we previously
> > > > > used a single node. But with many devices sharing a single node, this is
> > > > > problematic to register all the devices from DT. Using this MFD driver
> > > > > to do it is a proper solution in this case.
> > > > 
> > > > Tell me more.  What are the problems you encountered?
> > > 
> > > So we had a single node, chip-controller, accessed by multiple
> > > devices -and drivers-. We ended up with:
> > > 
> > > chip: chip-control@ea0000 {
> > > 	compatible = "marvell,berlin2q-chip-ctrl";
> > > 	reg = <0xea0000 0x400>, <0xdd0170 0x10>;
> > > 	#clock-cells = <1>;
> > > 	#reset-cells = <2>;
> > > 	clocks = <&refclk>;
> > > 	clock-names = "refclk";
> > > 
> > > 	[pinmux nodes]
> > > };
> > > 
> > > In addition to being a mess, how can you probe various drivers with this
> > > single node? We had to probe a clock driver in addition to the
> > > pin-controller and reset drivers. We ended up using arch_initcall() in
> > > the reset driver, which was *not* acceptable.
> > > 
> > > These chip and system controllers are not an IP, but helps not spreading
> > > this bank of registers all over the DT.
> > > 
> > > The solution to this problem is to introduce an mtd driver which
> > > registers all the sub-devices described by these chip and system
> > > controller nodes.
> > 
> > I'm still not convinced that your problem can't be solved in DT, but
> > creating a single psudo-hardware node is not correct either.  What
> > does the h/w _really_ look like?  Is all of this stuff on a single
> > chip? 
> 
> There is no specific IP for these registers, but we do not want them
> spread all over the DT nodes so they're summed up into this chip node.
> 
> SO we have all those registers into a chip/system node and sub-nodes for
> the devices using them.
> 
> > If so, I would expect to see something like:
> > 
> > control@ea0000 {
> > 	compatible = "marvel,control";
> > 
> > 	pinctrl@xxxxx {
> > 		compatible = "marvel,pinctrl";
> > 	};
> > 
> > 	reset@xxxxx {
> > 		compatible = "marvel,reset";
> > 	};
> > };
> 
> That's exactly the point of this series: having one sub-node per device.
> 
> With this series applied, we have (the clock being a sub-node of the
> chip-controller node is part of another series following this one):
> 
> chip: chip-controller@ea0000 {
>         compatible = "marvell,berlin2q-chip-ctrl", "syscon";
>         reg = <0xea0000 0x400>, <0xdd0170 0x10>;
>         #clock-cells = <1>;
>         clocks = <&refclk>;
>         clock-names = "refclk";
> 
>         soc_pinctrl: pin-controller {
>                 compatible = "marvell,berlin2q-soc-pinctrl";
> 
>                 twsi0_pmux: twsi0-pmux {
>                         groups = "G6";
>                         function = "twsi0";
>                 };
> 
>                 twsi1_pmux: twsi1-pmux {
>                         groups = "G7";
>                         function = "twsi1";
>                 };
>         };
> 
>         chip_rst: reset {
>                 compatible = "marvell,berlin2-reset";
>                 #reset-cells = <2>;
>         };
> };

This is what I'd expect to see in DT, so we're heading in the right
direction.  So make to my original question, what's the point of this
MFD driver, and why don't you just let DT framework register these
devices for you?

You issue a compatible string here, then duplicate it in the driver,
why do you think this is necessary?

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

[PATCH 01/11] mfd: add the Berlin controller driver

[Lee Jones]

On Wed, 18 Feb 2015, Antoine Tenart wrote:

> On Wed, Feb 18, 2015 at 09:09:58AM +0000, Lee Jones wrote:
> > On Wed, 18 Feb 2015, Antoine Tenart wrote:
> > 
> > > On Tue, Feb 17, 2015 at 11:54:48AM +0000, Lee Jones wrote:
> > > > On Tue, 17 Feb 2015, Antoine Tenart wrote:
> > > > > On Mon, Feb 16, 2015 at 12:48:08PM +0000, Lee Jones wrote:
> > > > > > On Wed, 11 Feb 2015, Antoine Tenart wrote:
> > > > 
> > > > > > > +static int berlin_ctrl_probe(struct platform_device *pdev)
> > > > > > > +{
> > > > > > > +	struct device *dev = &pdev->dev;
> > > > > > > +	const struct of_device_id *match;
> > > > > > > +	const struct berlin_ctrl_priv *priv;
> > > > > > > +	int ret;
> > > > > > > +
> > > > > > > +	match = of_match_node(berlin_ctrl_of_match, dev->of_node);
> > > > > > > +	if (!match)
> > > > > > > +		return -EINVAL;
> > > > > > > +
> > > > > > > +	priv = match->data;
> > > > > > > +
> > > > > > > +	ret = mfd_add_devices(dev, 0, priv->devs, priv->ndevs, NULL, -1, NULL);
> > > > > > > +	if (ret) {
> > > > > > > +		dev_err(dev, "Failed to add devices: %d\n", ret);
> > > > > > > +		return ret;
> > > > > > > +	}
> > > > > > > +
> > > > > > > +	return 0;
> > > > > > > +}
> > > > > > 
> > > > > > I'm not sure I see the point in this driver.  Why can't you just
> > > > > > register these devices directly from DT?
> > > > > 
> > > > > All these devices share the same bank of registers and we previously
> > > > > used a single node. But with many devices sharing a single node, this is
> > > > > problematic to register all the devices from DT. Using this MFD driver
> > > > > to do it is a proper solution in this case.
> > > > 
> > > > Tell me more.  What are the problems you encountered?
> > > 
> > > So we had a single node, chip-controller, accessed by multiple
> > > devices -and drivers-. We ended up with:
> > > 
> > > chip: chip-control at ea0000 {
> > > 	compatible = "marvell,berlin2q-chip-ctrl";
> > > 	reg = <0xea0000 0x400>, <0xdd0170 0x10>;
> > > 	#clock-cells = <1>;
> > > 	#reset-cells = <2>;
> > > 	clocks = <&refclk>;
> > > 	clock-names = "refclk";
> > > 
> > > 	[pinmux nodes]
> > > };
> > > 
> > > In addition to being a mess, how can you probe various drivers with this
> > > single node? We had to probe a clock driver in addition to the
> > > pin-controller and reset drivers. We ended up using arch_initcall() in
> > > the reset driver, which was *not* acceptable.
> > > 
> > > These chip and system controllers are not an IP, but helps not spreading
> > > this bank of registers all over the DT.
> > > 
> > > The solution to this problem is to introduce an mtd driver which
> > > registers all the sub-devices described by these chip and system
> > > controller nodes.
> > 
> > I'm still not convinced that your problem can't be solved in DT, but
> > creating a single psudo-hardware node is not correct either.  What
> > does the h/w _really_ look like?  Is all of this stuff on a single
> > chip? 
> 
> There is no specific IP for these registers, but we do not want them
> spread all over the DT nodes so they're summed up into this chip node.
> 
> SO we have all those registers into a chip/system node and sub-nodes for
> the devices using them.
> 
> > If so, I would expect to see something like:
> > 
> > control at ea0000 {
> > 	compatible = "marvel,control";
> > 
> > 	pinctrl at xxxxx {
> > 		compatible = "marvel,pinctrl";
> > 	};
> > 
> > 	reset at xxxxx {
> > 		compatible = "marvel,reset";
> > 	};
> > };
> 
> That's exactly the point of this series: having one sub-node per device.
> 
> With this series applied, we have (the clock being a sub-node of the
> chip-controller node is part of another series following this one):
> 
> chip: chip-controller at ea0000 {
>         compatible = "marvell,berlin2q-chip-ctrl", "syscon";
>         reg = <0xea0000 0x400>, <0xdd0170 0x10>;
>         #clock-cells = <1>;
>         clocks = <&refclk>;
>         clock-names = "refclk";
> 
>         soc_pinctrl: pin-controller {
>                 compatible = "marvell,berlin2q-soc-pinctrl";
> 
>                 twsi0_pmux: twsi0-pmux {
>                         groups = "G6";
>                         function = "twsi0";
>                 };
> 
>                 twsi1_pmux: twsi1-pmux {
>                         groups = "G7";
>                         function = "twsi1";
>                 };
>         };
> 
>         chip_rst: reset {
>                 compatible = "marvell,berlin2-reset";
>                 #reset-cells = <2>;
>         };
> };

This is what I'd expect to see in DT, so we're heading in the right
direction.  So make to my original question, what's the point of this
MFD driver, and why don't you just let DT framework register these
devices for you?

You issue a compatible string here, then duplicate it in the driver,
why do you think this is necessary?

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org ? Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

Re: [PATCH 01/11] mfd: add the Berlin controller driver

[Antoine Tenart]

On Wed, Feb 18, 2015 at 10:40:23AM +0000, Lee Jones wrote:
> On Wed, 18 Feb 2015, Antoine Tenart wrote:
> 
> > On Wed, Feb 18, 2015 at 09:09:58AM +0000, Lee Jones wrote:
> > > On Wed, 18 Feb 2015, Antoine Tenart wrote:
> > > 
> > > > On Tue, Feb 17, 2015 at 11:54:48AM +0000, Lee Jones wrote:
> > > > > On Tue, 17 Feb 2015, Antoine Tenart wrote:
> > > > > > On Mon, Feb 16, 2015 at 12:48:08PM +0000, Lee Jones wrote:
> > > > > > > On Wed, 11 Feb 2015, Antoine Tenart wrote:
> > > > > 
> > > > > > > > +static int berlin_ctrl_probe(struct platform_device *pdev)
> > > > > > > > +{
> > > > > > > > +	struct device *dev = &pdev->dev;
> > > > > > > > +	const struct of_device_id *match;
> > > > > > > > +	const struct berlin_ctrl_priv *priv;
> > > > > > > > +	int ret;
> > > > > > > > +
> > > > > > > > +	match = of_match_node(berlin_ctrl_of_match, dev->of_node);
> > > > > > > > +	if (!match)
> > > > > > > > +		return -EINVAL;
> > > > > > > > +
> > > > > > > > +	priv = match->data;
> > > > > > > > +
> > > > > > > > +	ret = mfd_add_devices(dev, 0, priv->devs, priv->ndevs, NULL, -1, NULL);
> > > > > > > > +	if (ret) {
> > > > > > > > +		dev_err(dev, "Failed to add devices: %d\n", ret);
> > > > > > > > +		return ret;
> > > > > > > > +	}
> > > > > > > > +
> > > > > > > > +	return 0;
> > > > > > > > +}
> > > > > > > 
> > > > > > > I'm not sure I see the point in this driver.  Why can't you just
> > > > > > > register these devices directly from DT?
> > > > > > 
> > > > > > All these devices share the same bank of registers and we previously
> > > > > > used a single node. But with many devices sharing a single node, this is
> > > > > > problematic to register all the devices from DT. Using this MFD driver
> > > > > > to do it is a proper solution in this case.
> > > > > 
> > > > > Tell me more.  What are the problems you encountered?
> > > > 
> > > > So we had a single node, chip-controller, accessed by multiple
> > > > devices -and drivers-. We ended up with:
> > > > 
> > > > chip: chip-control@ea0000 {
> > > > 	compatible = "marvell,berlin2q-chip-ctrl";
> > > > 	reg = <0xea0000 0x400>, <0xdd0170 0x10>;
> > > > 	#clock-cells = <1>;
> > > > 	#reset-cells = <2>;
> > > > 	clocks = <&refclk>;
> > > > 	clock-names = "refclk";
> > > > 
> > > > 	[pinmux nodes]
> > > > };
> > > > 
> > > > In addition to being a mess, how can you probe various drivers with this
> > > > single node? We had to probe a clock driver in addition to the
> > > > pin-controller and reset drivers. We ended up using arch_initcall() in
> > > > the reset driver, which was *not* acceptable.
> > > > 
> > > > These chip and system controllers are not an IP, but helps not spreading
> > > > this bank of registers all over the DT.
> > > > 
> > > > The solution to this problem is to introduce an mtd driver which
> > > > registers all the sub-devices described by these chip and system
> > > > controller nodes.
> > > 
> > > I'm still not convinced that your problem can't be solved in DT, but
> > > creating a single psudo-hardware node is not correct either.  What
> > > does the h/w _really_ look like?  Is all of this stuff on a single
> > > chip? 
> > 
> > There is no specific IP for these registers, but we do not want them
> > spread all over the DT nodes so they're summed up into this chip node.
> > 
> > SO we have all those registers into a chip/system node and sub-nodes for
> > the devices using them.
> > 
> > > If so, I would expect to see something like:
> > > 
> > > control@ea0000 {
> > > 	compatible = "marvel,control";
> > > 
> > > 	pinctrl@xxxxx {
> > > 		compatible = "marvel,pinctrl";
> > > 	};
> > > 
> > > 	reset@xxxxx {
> > > 		compatible = "marvel,reset";
> > > 	};
> > > };
> > 
> > That's exactly the point of this series: having one sub-node per device.
> > 
> > With this series applied, we have (the clock being a sub-node of the
> > chip-controller node is part of another series following this one):
> > 
> > chip: chip-controller@ea0000 {
> >         compatible = "marvell,berlin2q-chip-ctrl", "syscon";
> >         reg = <0xea0000 0x400>, <0xdd0170 0x10>;
> >         #clock-cells = <1>;
> >         clocks = <&refclk>;
> >         clock-names = "refclk";
> > 
> >         soc_pinctrl: pin-controller {
> >                 compatible = "marvell,berlin2q-soc-pinctrl";
> > 
> >                 twsi0_pmux: twsi0-pmux {
> >                         groups = "G6";
> >                         function = "twsi0";
> >                 };
> > 
> >                 twsi1_pmux: twsi1-pmux {
> >                         groups = "G7";
> >                         function = "twsi1";
> >                 };
> >         };
> > 
> >         chip_rst: reset {
> >                 compatible = "marvell,berlin2-reset";
> >                 #reset-cells = <2>;
> >         };
> > };
> 
> This is what I'd expect to see in DT, so we're heading in the right
> direction.  So make to my original question, what's the point of this
> MFD driver, and why don't you just let DT framework register these
> devices for you?
> 
> You issue a compatible string here, then duplicate it in the driver,
> why do you think this is necessary?

The chip-controller node is *not* a bus. Please have a look on
Sebastian's answer.

Antoine

-- 
Antoine Ténart, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com

[PATCH 01/11] mfd: add the Berlin controller driver

[Antoine Tenart]

On Wed, Feb 18, 2015 at 10:40:23AM +0000, Lee Jones wrote:
> On Wed, 18 Feb 2015, Antoine Tenart wrote:
> 
> > On Wed, Feb 18, 2015 at 09:09:58AM +0000, Lee Jones wrote:
> > > On Wed, 18 Feb 2015, Antoine Tenart wrote:
> > > 
> > > > On Tue, Feb 17, 2015 at 11:54:48AM +0000, Lee Jones wrote:
> > > > > On Tue, 17 Feb 2015, Antoine Tenart wrote:
> > > > > > On Mon, Feb 16, 2015 at 12:48:08PM +0000, Lee Jones wrote:
> > > > > > > On Wed, 11 Feb 2015, Antoine Tenart wrote:
> > > > > 
> > > > > > > > +static int berlin_ctrl_probe(struct platform_device *pdev)
> > > > > > > > +{
> > > > > > > > +	struct device *dev = &pdev->dev;
> > > > > > > > +	const struct of_device_id *match;
> > > > > > > > +	const struct berlin_ctrl_priv *priv;
> > > > > > > > +	int ret;
> > > > > > > > +
> > > > > > > > +	match = of_match_node(berlin_ctrl_of_match, dev->of_node);
> > > > > > > > +	if (!match)
> > > > > > > > +		return -EINVAL;
> > > > > > > > +
> > > > > > > > +	priv = match->data;
> > > > > > > > +
> > > > > > > > +	ret = mfd_add_devices(dev, 0, priv->devs, priv->ndevs, NULL, -1, NULL);
> > > > > > > > +	if (ret) {
> > > > > > > > +		dev_err(dev, "Failed to add devices: %d\n", ret);
> > > > > > > > +		return ret;
> > > > > > > > +	}
> > > > > > > > +
> > > > > > > > +	return 0;
> > > > > > > > +}
> > > > > > > 
> > > > > > > I'm not sure I see the point in this driver.  Why can't you just
> > > > > > > register these devices directly from DT?
> > > > > > 
> > > > > > All these devices share the same bank of registers and we previously
> > > > > > used a single node. But with many devices sharing a single node, this is
> > > > > > problematic to register all the devices from DT. Using this MFD driver
> > > > > > to do it is a proper solution in this case.
> > > > > 
> > > > > Tell me more.  What are the problems you encountered?
> > > > 
> > > > So we had a single node, chip-controller, accessed by multiple
> > > > devices -and drivers-. We ended up with:
> > > > 
> > > > chip: chip-control at ea0000 {
> > > > 	compatible = "marvell,berlin2q-chip-ctrl";
> > > > 	reg = <0xea0000 0x400>, <0xdd0170 0x10>;
> > > > 	#clock-cells = <1>;
> > > > 	#reset-cells = <2>;
> > > > 	clocks = <&refclk>;
> > > > 	clock-names = "refclk";
> > > > 
> > > > 	[pinmux nodes]
> > > > };
> > > > 
> > > > In addition to being a mess, how can you probe various drivers with this
> > > > single node? We had to probe a clock driver in addition to the
> > > > pin-controller and reset drivers. We ended up using arch_initcall() in
> > > > the reset driver, which was *not* acceptable.
> > > > 
> > > > These chip and system controllers are not an IP, but helps not spreading
> > > > this bank of registers all over the DT.
> > > > 
> > > > The solution to this problem is to introduce an mtd driver which
> > > > registers all the sub-devices described by these chip and system
> > > > controller nodes.
> > > 
> > > I'm still not convinced that your problem can't be solved in DT, but
> > > creating a single psudo-hardware node is not correct either.  What
> > > does the h/w _really_ look like?  Is all of this stuff on a single
> > > chip? 
> > 
> > There is no specific IP for these registers, but we do not want them
> > spread all over the DT nodes so they're summed up into this chip node.
> > 
> > SO we have all those registers into a chip/system node and sub-nodes for
> > the devices using them.
> > 
> > > If so, I would expect to see something like:
> > > 
> > > control at ea0000 {
> > > 	compatible = "marvel,control";
> > > 
> > > 	pinctrl at xxxxx {
> > > 		compatible = "marvel,pinctrl";
> > > 	};
> > > 
> > > 	reset at xxxxx {
> > > 		compatible = "marvel,reset";
> > > 	};
> > > };
> > 
> > That's exactly the point of this series: having one sub-node per device.
> > 
> > With this series applied, we have (the clock being a sub-node of the
> > chip-controller node is part of another series following this one):
> > 
> > chip: chip-controller at ea0000 {
> >         compatible = "marvell,berlin2q-chip-ctrl", "syscon";
> >         reg = <0xea0000 0x400>, <0xdd0170 0x10>;
> >         #clock-cells = <1>;
> >         clocks = <&refclk>;
> >         clock-names = "refclk";
> > 
> >         soc_pinctrl: pin-controller {
> >                 compatible = "marvell,berlin2q-soc-pinctrl";
> > 
> >                 twsi0_pmux: twsi0-pmux {
> >                         groups = "G6";
> >                         function = "twsi0";
> >                 };
> > 
> >                 twsi1_pmux: twsi1-pmux {
> >                         groups = "G7";
> >                         function = "twsi1";
> >                 };
> >         };
> > 
> >         chip_rst: reset {
> >                 compatible = "marvell,berlin2-reset";
> >                 #reset-cells = <2>;
> >         };
> > };
> 
> This is what I'd expect to see in DT, so we're heading in the right
> direction.  So make to my original question, what's the point of this
> MFD driver, and why don't you just let DT framework register these
> devices for you?
> 
> You issue a compatible string here, then duplicate it in the driver,
> why do you think this is necessary?

The chip-controller node is *not* a bus. Please have a look on
Sebastian's answer.

Antoine

-- 
Antoine T?nart, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com

Re: [PATCH 01/11] mfd: add the Berlin controller driver

[Sebastian Hesselbarth]

On 18.02.2015 11:40, Lee Jones wrote:
> On Wed, 18 Feb 2015, Antoine Tenart wrote:
[...]
>> chip: chip-controller@ea0000 {
>>          compatible = "marvell,berlin2q-chip-ctrl", "syscon";
>>          reg = <0xea0000 0x400>, <0xdd0170 0x10>;
>>          #clock-cells = <1>;
>>          clocks = <&refclk>;
>>          clock-names = "refclk";
>>
>>          soc_pinctrl: pin-controller {
>>                  compatible = "marvell,berlin2q-soc-pinctrl";
>>
>>                  twsi0_pmux: twsi0-pmux {
>>                          groups = "G6";
>>                          function = "twsi0";
>>                  };
>>
>>                  twsi1_pmux: twsi1-pmux {
>>                          groups = "G7";
>>                          function = "twsi1";
>>                  };
>>          };
>>
>>          chip_rst: reset {
>>                  compatible = "marvell,berlin2-reset";
>>                  #reset-cells = <2>;
>>          };
>> };
>
> This is what I'd expect to see in DT, so we're heading in the right
> direction.  So make to my original question, what's the point of this
> MFD driver, and why don't you just let DT framework register these
> devices for you?
>
> You issue a compatible string here, then duplicate it in the driver,
> why do you think this is necessary?

Lee,

there is no DT framework that automatically probes for
compatible<->driver matches. You either make it "simple-bus" compatible
which will call of_foo_populate() or you have to register each of the
devices yourself. It clearly is not a bus, so if we use this as a
workaround, we'll get yelled at by others.

Sebastian

[PATCH 01/11] mfd: add the Berlin controller driver

[Sebastian Hesselbarth]

On 18.02.2015 11:40, Lee Jones wrote:
> On Wed, 18 Feb 2015, Antoine Tenart wrote:
[...]
>> chip: chip-controller at ea0000 {
>>          compatible = "marvell,berlin2q-chip-ctrl", "syscon";
>>          reg = <0xea0000 0x400>, <0xdd0170 0x10>;
>>          #clock-cells = <1>;
>>          clocks = <&refclk>;
>>          clock-names = "refclk";
>>
>>          soc_pinctrl: pin-controller {
>>                  compatible = "marvell,berlin2q-soc-pinctrl";
>>
>>                  twsi0_pmux: twsi0-pmux {
>>                          groups = "G6";
>>                          function = "twsi0";
>>                  };
>>
>>                  twsi1_pmux: twsi1-pmux {
>>                          groups = "G7";
>>                          function = "twsi1";
>>                  };
>>          };
>>
>>          chip_rst: reset {
>>                  compatible = "marvell,berlin2-reset";
>>                  #reset-cells = <2>;
>>          };
>> };
>
> This is what I'd expect to see in DT, so we're heading in the right
> direction.  So make to my original question, what's the point of this
> MFD driver, and why don't you just let DT framework register these
> devices for you?
>
> You issue a compatible string here, then duplicate it in the driver,
> why do you think this is necessary?

Lee,

there is no DT framework that automatically probes for
compatible<->driver matches. You either make it "simple-bus" compatible
which will call of_foo_populate() or you have to register each of the
devices yourself. It clearly is not a bus, so if we use this as a
workaround, we'll get yelled at by others.

Sebastian

Re: [PATCH 01/11] mfd: add the Berlin controller driver

[Lee Jones]

On Wed, 18 Feb 2015, Sebastian Hesselbarth wrote:

> On 18.02.2015 11:40, Lee Jones wrote:
> >On Wed, 18 Feb 2015, Antoine Tenart wrote:
> [...]
> >>chip: chip-controller@ea0000 {
> >>         compatible = "marvell,berlin2q-chip-ctrl", "syscon";
> >>         reg = <0xea0000 0x400>, <0xdd0170 0x10>;
> >>         #clock-cells = <1>;
> >>         clocks = <&refclk>;
> >>         clock-names = "refclk";
> >>
> >>         soc_pinctrl: pin-controller {
> >>                 compatible = "marvell,berlin2q-soc-pinctrl";
> >>
> >>                 twsi0_pmux: twsi0-pmux {
> >>                         groups = "G6";
> >>                         function = "twsi0";
> >>                 };
> >>
> >>                 twsi1_pmux: twsi1-pmux {
> >>                         groups = "G7";
> >>                         function = "twsi1";
> >>                 };
> >>         };
> >>
> >>         chip_rst: reset {
> >>                 compatible = "marvell,berlin2-reset";
> >>                 #reset-cells = <2>;
> >>         };
> >>};
> >
> >This is what I'd expect to see in DT, so we're heading in the right
> >direction.  So make to my original question, what's the point of this
> >MFD driver, and why don't you just let DT framework register these
> >devices for you?
> >
> >You issue a compatible string here, then duplicate it in the driver,
> >why do you think this is necessary?
> 
> there is no DT framework that automatically probes for
> compatible<->driver matches. You either make it "simple-bus" compatible
> which will call of_foo_populate() or you have to register each of the
> devices yourself. It clearly is not a bus, so if we use this as a
> workaround, we'll get yelled at by others.

I do agree that using 'simple-bus' to describe only this IP would be
an abuse.  However, my foundation thought/argument is unchanged.  This
'driver' is a hack.  It has no functional use besides to work around a
problem of semantics and as such has no place in MFD.

Back onto the simple-bus theme, as this is a syscon device it is a bus
of sorts.  Have you thought about making it a child of your its syscon
node, then using simple-bus to get the OF framework to register the
child devices?

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

[PATCH 01/11] mfd: add the Berlin controller driver

[Lee Jones]

On Wed, 18 Feb 2015, Sebastian Hesselbarth wrote:

> On 18.02.2015 11:40, Lee Jones wrote:
> >On Wed, 18 Feb 2015, Antoine Tenart wrote:
> [...]
> >>chip: chip-controller at ea0000 {
> >>         compatible = "marvell,berlin2q-chip-ctrl", "syscon";
> >>         reg = <0xea0000 0x400>, <0xdd0170 0x10>;
> >>         #clock-cells = <1>;
> >>         clocks = <&refclk>;
> >>         clock-names = "refclk";
> >>
> >>         soc_pinctrl: pin-controller {
> >>                 compatible = "marvell,berlin2q-soc-pinctrl";
> >>
> >>                 twsi0_pmux: twsi0-pmux {
> >>                         groups = "G6";
> >>                         function = "twsi0";
> >>                 };
> >>
> >>                 twsi1_pmux: twsi1-pmux {
> >>                         groups = "G7";
> >>                         function = "twsi1";
> >>                 };
> >>         };
> >>
> >>         chip_rst: reset {
> >>                 compatible = "marvell,berlin2-reset";
> >>                 #reset-cells = <2>;
> >>         };
> >>};
> >
> >This is what I'd expect to see in DT, so we're heading in the right
> >direction.  So make to my original question, what's the point of this
> >MFD driver, and why don't you just let DT framework register these
> >devices for you?
> >
> >You issue a compatible string here, then duplicate it in the driver,
> >why do you think this is necessary?
> 
> there is no DT framework that automatically probes for
> compatible<->driver matches. You either make it "simple-bus" compatible
> which will call of_foo_populate() or you have to register each of the
> devices yourself. It clearly is not a bus, so if we use this as a
> workaround, we'll get yelled at by others.

I do agree that using 'simple-bus' to describe only this IP would be
an abuse.  However, my foundation thought/argument is unchanged.  This
'driver' is a hack.  It has no functional use besides to work around a
problem of semantics and as such has no place in MFD.

Back onto the simple-bus theme, as this is a syscon device it is a bus
of sorts.  Have you thought about making it a child of your its syscon
node, then using simple-bus to get the OF framework to register the
child devices?

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org ? Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

Re: [PATCH 01/11] mfd: add the Berlin controller driver

[Sebastian Hesselbarth]

On 02/18/2015 12:58 PM, Lee Jones wrote:
> I do agree that using 'simple-bus' to describe only this IP would be
> an abuse.  However, my foundation thought/argument is unchanged.  This
> 'driver' is a hack.  It has no functional use besides to work around a
> problem of semantics and as such has no place in MFD.

Lee,

sorry I don't get it. Here you say that using simple-bus is an abuse...

> Back onto the simple-bus theme, as this is a syscon device it is a bus
> of sorts.  Have you thought about making it a child of your its syscon
> node, then using simple-bus to get the OF framework to register the
> child devices?

... and here you suggest to use simple-bus to register the child
devices?

I fundamentally disagree that either this registers or syscon in general
should in any way be seen as a bus. The chip control registers is an
highly unsorted bunch of bits that we try to match with cleanly
separated subsystems. This makes it a resource but no bus of any sort.

The problem that we try to solve here is not a DT problem but solely
driven by the fact that we need something to register platform_devices
for pinctrl and reset. The unit we describe in DT is a pinctrl-clock-
power-reset-unit - or short chip control.

If you argue that mfd is not the right place for this "driver" we'll
have to find a different place for it. I remember Mike has no problem
with extending early probed clock drivers to register additional
platform_devices - so I guess we end up putting it in there ignoring
mfd's ability to do it for us.

Do we agree on that?

Sebastian

[PATCH 01/11] mfd: add the Berlin controller driver

[Sebastian Hesselbarth]

On 02/18/2015 12:58 PM, Lee Jones wrote:
> I do agree that using 'simple-bus' to describe only this IP would be
> an abuse.  However, my foundation thought/argument is unchanged.  This
> 'driver' is a hack.  It has no functional use besides to work around a
> problem of semantics and as such has no place in MFD.

Lee,

sorry I don't get it. Here you say that using simple-bus is an abuse...

> Back onto the simple-bus theme, as this is a syscon device it is a bus
> of sorts.  Have you thought about making it a child of your its syscon
> node, then using simple-bus to get the OF framework to register the
> child devices?

... and here you suggest to use simple-bus to register the child
devices?

I fundamentally disagree that either this registers or syscon in general
should in any way be seen as a bus. The chip control registers is an
highly unsorted bunch of bits that we try to match with cleanly
separated subsystems. This makes it a resource but no bus of any sort.

The problem that we try to solve here is not a DT problem but solely
driven by the fact that we need something to register platform_devices
for pinctrl and reset. The unit we describe in DT is a pinctrl-clock-
power-reset-unit - or short chip control.

If you argue that mfd is not the right place for this "driver" we'll
have to find a different place for it. I remember Mike has no problem
with extending early probed clock drivers to register additional
platform_devices - so I guess we end up putting it in there ignoring
mfd's ability to do it for us.

Do we agree on that?

Sebastian

Re: [PATCH 01/11] mfd: add the Berlin controller driver

[Lee Jones]

On Wed, 18 Feb 2015, Sebastian Hesselbarth wrote:

> On 02/18/2015 12:58 PM, Lee Jones wrote:
> >I do agree that using 'simple-bus' to describe only this IP would be
> >an abuse.  However, my foundation thought/argument is unchanged.  This
> >'driver' is a hack.  It has no functional use besides to work around a
> >problem of semantics and as such has no place in MFD.
> 
> Lee,
> 
> sorry I don't get it. Here you say that using simple-bus is an abuse...
> 
> >Back onto the simple-bus theme, as this is a syscon device it is a bus
> >of sorts.  Have you thought about making it a child of your its syscon
> >node, then using simple-bus to get the OF framework to register the
> >child devices?
> 
> ... and here you suggest to use simple-bus to register the child
> devices?

Nope, that's not what I said:

  "I do agree that using 'simple-bus' to describe *ONLY THIS IP* would
  be an abuse."

... although I believe there is a need to treat syscon devices as
simple buses.  There are examples of devices doing this already:

git grep -El 'syscon.*simple-bus' next/master
  next/master:arch/arm/boot/dts/imx6qdl.dtsi
  next/master:arch/arm/boot/dts/imx6sl.dtsi
  next/master:arch/arm/boot/dts/imx6sx.dtsi
  next/master:arch/arm/boot/dts/zynq-7000.dtsi

> I fundamentally disagree that either this registers or syscon in general
> should in any way be seen as a bus. The chip control registers is an
> highly unsorted bunch of bits that we try to match with cleanly
> separated subsystems. This makes it a resource but no bus of any sort.

This is where my comment about semantics comes into play.  syscon may
not be a bus is the truest sense; however, this is clearly a
requirement for sub devices to be probed in the same way a simple-bus
is currently.  So we're just missing a framework somewhere.  We can
fix that.

> The problem that we try to solve here is not a DT problem but solely
> driven by the fact that we need something to register platform_devices
> for pinctrl and reset. The unit we describe in DT is a pinctrl-clock-
> power-reset-unit - or short chip control.

I agree with the last part, but this is a DT problem.  It lacks the
functionality to be able to cleanly register these types of
(sub-)devices.  Devices which, in my opinion should be described
inside the parent syscon node.

> If you argue that mfd is not the right place for this "driver" we'll
> have to find a different place for it. I remember Mike has no problem
> with extending early probed clock drivers to register additional
> platform_devices - so I guess we end up putting it in there ignoring
> mfd's ability to do it for us.

My argument is not that this fake driver doesn't belong in MFD, it's
that it doesn't belong.  That includes shoving it in drivers/clk.  I
will be only too happy to have a chat with Mike and provide him with
my reasons why.

What I think we should do however, it write some framework code which
can neatly handle these use-cases, which may just be a case of:

  s/of_platform_bus_probe/of_platform_subdevice_probe/

... obviously I'm oversimplifying by quite some margin, but I'm sure
you catch my drift.

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

[PATCH 01/11] mfd: add the Berlin controller driver

[Lee Jones]

On Wed, 18 Feb 2015, Sebastian Hesselbarth wrote:

> On 02/18/2015 12:58 PM, Lee Jones wrote:
> >I do agree that using 'simple-bus' to describe only this IP would be
> >an abuse.  However, my foundation thought/argument is unchanged.  This
> >'driver' is a hack.  It has no functional use besides to work around a
> >problem of semantics and as such has no place in MFD.
> 
> Lee,
> 
> sorry I don't get it. Here you say that using simple-bus is an abuse...
> 
> >Back onto the simple-bus theme, as this is a syscon device it is a bus
> >of sorts.  Have you thought about making it a child of your its syscon
> >node, then using simple-bus to get the OF framework to register the
> >child devices?
> 
> ... and here you suggest to use simple-bus to register the child
> devices?

Nope, that's not what I said:

  "I do agree that using 'simple-bus' to describe *ONLY THIS IP* would
  be an abuse."

... although I believe there is a need to treat syscon devices as
simple buses.  There are examples of devices doing this already:

git grep -El 'syscon.*simple-bus' next/master
  next/master:arch/arm/boot/dts/imx6qdl.dtsi
  next/master:arch/arm/boot/dts/imx6sl.dtsi
  next/master:arch/arm/boot/dts/imx6sx.dtsi
  next/master:arch/arm/boot/dts/zynq-7000.dtsi

> I fundamentally disagree that either this registers or syscon in general
> should in any way be seen as a bus. The chip control registers is an
> highly unsorted bunch of bits that we try to match with cleanly
> separated subsystems. This makes it a resource but no bus of any sort.

This is where my comment about semantics comes into play.  syscon may
not be a bus is the truest sense; however, this is clearly a
requirement for sub devices to be probed in the same way a simple-bus
is currently.  So we're just missing a framework somewhere.  We can
fix that.

> The problem that we try to solve here is not a DT problem but solely
> driven by the fact that we need something to register platform_devices
> for pinctrl and reset. The unit we describe in DT is a pinctrl-clock-
> power-reset-unit - or short chip control.

I agree with the last part, but this is a DT problem.  It lacks the
functionality to be able to cleanly register these types of
(sub-)devices.  Devices which, in my opinion should be described
inside the parent syscon node.

> If you argue that mfd is not the right place for this "driver" we'll
> have to find a different place for it. I remember Mike has no problem
> with extending early probed clock drivers to register additional
> platform_devices - so I guess we end up putting it in there ignoring
> mfd's ability to do it for us.

My argument is not that this fake driver doesn't belong in MFD, it's
that it doesn't belong.  That includes shoving it in drivers/clk.  I
will be only too happy to have a chat with Mike and provide him with
my reasons why.

What I think we should do however, it write some framework code which
can neatly handle these use-cases, which may just be a case of:

  s/of_platform_bus_probe/of_platform_subdevice_probe/

... obviously I'm oversimplifying by quite some margin, but I'm sure
you catch my drift.

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org ? Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

Re: [PATCH 01/11] mfd: add the Berlin controller driver

[Arnd Bergmann]

On Wednesday 18 February 2015 14:09:04 Sebastian Hesselbarth wrote:
> On 02/18/2015 12:58 PM, Lee Jones wrote:
> > I do agree that using 'simple-bus' to describe only this IP would be
> > an abuse.  However, my foundation thought/argument is unchanged.  This
> > 'driver' is a hack.  It has no functional use besides to work around a
> > problem of semantics and as such has no place in MFD.
> 
> Lee,
> 
> sorry I don't get it. Here you say that using simple-bus is an abuse...
> 
> > Back onto the simple-bus theme, as this is a syscon device it is a bus
> > of sorts.  Have you thought about making it a child of your its syscon
> > node, then using simple-bus to get the OF framework to register the
> > child devices?
> 
> ... and here you suggest to use simple-bus to register the child
> devices?
> 
> I fundamentally disagree that either this registers or syscon in general
> should in any way be seen as a bus. The chip control registers is an
> highly unsorted bunch of bits that we try to match with cleanly
> separated subsystems. This makes it a resource but no bus of any sort.

It really depends on what you mean by 'bus'. It's certainly not a bus_type
in Linux, but if you have a node in DT with multiple children, we
sometimes think of that as a bus. I believe it makes sense to have
the child devices under the syscon node here, at least the ones that
have no other registers.

> The problem that we try to solve here is not a DT problem but solely
> driven by the fact that we need something to register platform_devices
> for pinctrl and reset. The unit we describe in DT is a pinctrl-clock-
> power-reset-unit - or short chip control.

There are two problems here that we need to look at separately,
even though there is some interaction:

* For the DT representation, we need to make it something that
corresponds to the hardware. We could either have a single device
node for the set of registers, or we could have one node for each
child. With syscon, we could also put the functional device nodes
somewhere else, which we have to do if any device uses multiple
syscon parents.

* For the driver code, we need a way to fit into the kernel model
while at the same time using the information that is in DT.
I agree with Lee that your current driver is not a good solution
here: you create a driver for the parent device that knows what
child devices there are, which is not a good abstraction. Instead
we should have a way for the child devices to get probed automatically,
just like we do for simple-bus, whether we use simple-bus or not.

> If you argue that mfd is not the right place for this "driver" we'll
> have to find a different place for it. I remember Mike has no problem
> with extending early probed clock drivers to register additional
> platform_devices - so I guess we end up putting it in there ignoring
> mfd's ability to do it for us.

If you have a driver that is responsible for the entire register
area, I think it would be best to make that driver just register
to all the subsystems itself rather than creating child devices.

The alternative is to come up with a way to probe all the child
devices automatically, but then we should make that parent device
have a generic driver that does not need to know about the children
and that can work on any platform with similar requirements.

	Arnd

[PATCH 01/11] mfd: add the Berlin controller driver

[Arnd Bergmann]

On Wednesday 18 February 2015 14:09:04 Sebastian Hesselbarth wrote:
> On 02/18/2015 12:58 PM, Lee Jones wrote:
> > I do agree that using 'simple-bus' to describe only this IP would be
> > an abuse.  However, my foundation thought/argument is unchanged.  This
> > 'driver' is a hack.  It has no functional use besides to work around a
> > problem of semantics and as such has no place in MFD.
> 
> Lee,
> 
> sorry I don't get it. Here you say that using simple-bus is an abuse...
> 
> > Back onto the simple-bus theme, as this is a syscon device it is a bus
> > of sorts.  Have you thought about making it a child of your its syscon
> > node, then using simple-bus to get the OF framework to register the
> > child devices?
> 
> ... and here you suggest to use simple-bus to register the child
> devices?
> 
> I fundamentally disagree that either this registers or syscon in general
> should in any way be seen as a bus. The chip control registers is an
> highly unsorted bunch of bits that we try to match with cleanly
> separated subsystems. This makes it a resource but no bus of any sort.

It really depends on what you mean by 'bus'. It's certainly not a bus_type
in Linux, but if you have a node in DT with multiple children, we
sometimes think of that as a bus. I believe it makes sense to have
the child devices under the syscon node here, at least the ones that
have no other registers.

> The problem that we try to solve here is not a DT problem but solely
> driven by the fact that we need something to register platform_devices
> for pinctrl and reset. The unit we describe in DT is a pinctrl-clock-
> power-reset-unit - or short chip control.

There are two problems here that we need to look at separately,
even though there is some interaction:

* For the DT representation, we need to make it something that
corresponds to the hardware. We could either have a single device
node for the set of registers, or we could have one node for each
child. With syscon, we could also put the functional device nodes
somewhere else, which we have to do if any device uses multiple
syscon parents.

* For the driver code, we need a way to fit into the kernel model
while at the same time using the information that is in DT.
I agree with Lee that your current driver is not a good solution
here: you create a driver for the parent device that knows what
child devices there are, which is not a good abstraction. Instead
we should have a way for the child devices to get probed automatically,
just like we do for simple-bus, whether we use simple-bus or not.

> If you argue that mfd is not the right place for this "driver" we'll
> have to find a different place for it. I remember Mike has no problem
> with extending early probed clock drivers to register additional
> platform_devices - so I guess we end up putting it in there ignoring
> mfd's ability to do it for us.

If you have a driver that is responsible for the entire register
area, I think it would be best to make that driver just register
to all the subsystems itself rather than creating child devices.

The alternative is to come up with a way to probe all the child
devices automatically, but then we should make that parent device
have a generic driver that does not need to know about the children
and that can work on any platform with similar requirements.

	Arnd

Re: [PATCH 01/11] mfd: add the Berlin controller driver

[Lee Jones]

On Wed, 18 Feb 2015, Lee Jones wrote:

> On Wed, 18 Feb 2015, Sebastian Hesselbarth wrote:
> 
> > On 02/18/2015 12:58 PM, Lee Jones wrote:
> > >I do agree that using 'simple-bus' to describe only this IP would be
> > >an abuse.  However, my foundation thought/argument is unchanged.  This
> > >'driver' is a hack.  It has no functional use besides to work around a
> > >problem of semantics and as such has no place in MFD.
> > 
> > Lee,
> > 
> > sorry I don't get it. Here you say that using simple-bus is an abuse...
> > 
> > >Back onto the simple-bus theme, as this is a syscon device it is a bus
> > >of sorts.  Have you thought about making it a child of your its syscon
> > >node, then using simple-bus to get the OF framework to register the
> > >child devices?
> > 
> > ... and here you suggest to use simple-bus to register the child
> > devices?
> 
> Nope, that's not what I said:
> 
>   "I do agree that using 'simple-bus' to describe *ONLY THIS IP* would
>   be an abuse."
> 
> ... although I believe there is a need to treat syscon devices as
> simple buses.  There are examples of devices doing this already:
> 
> git grep -El 'syscon.*simple-bus' next/master
>   next/master:arch/arm/boot/dts/imx6qdl.dtsi
>   next/master:arch/arm/boot/dts/imx6sl.dtsi
>   next/master:arch/arm/boot/dts/imx6sx.dtsi
>   next/master:arch/arm/boot/dts/zynq-7000.dtsi
> 
> > I fundamentally disagree that either this registers or syscon in general
> > should in any way be seen as a bus. The chip control registers is an
> > highly unsorted bunch of bits that we try to match with cleanly
> > separated subsystems. This makes it a resource but no bus of any sort.
> 
> This is where my comment about semantics comes into play.  syscon may
> not be a bus is the truest sense; however, this is clearly a
> requirement for sub devices to be probed in the same way a simple-bus
> is currently.  So we're just missing a framework somewhere.  We can
> fix that.
> 
> > The problem that we try to solve here is not a DT problem but solely
> > driven by the fact that we need something to register platform_devices
> > for pinctrl and reset. The unit we describe in DT is a pinctrl-clock-
> > power-reset-unit - or short chip control.
> 
> I agree with the last part, but this is a DT problem.  It lacks the
> functionality to be able to cleanly register these types of
> (sub-)devices.  Devices which, in my opinion should be described
> inside the parent syscon node.
> 
> > If you argue that mfd is not the right place for this "driver" we'll
> > have to find a different place for it. I remember Mike has no problem
> > with extending early probed clock drivers to register additional
> > platform_devices - so I guess we end up putting it in there ignoring
> > mfd's ability to do it for us.
> 
> My argument is not that this fake driver doesn't belong in MFD, it's
> that it doesn't belong.  That includes shoving it in drivers/clk.  I
> will be only too happy to have a chat with Mike and provide him with
> my reasons why.
> 
> What I think we should do however, it write some framework code which
> can neatly handle these use-cases, which may just be a case of:
> 
>   s/of_platform_bus_probe/of_platform_subdevice_probe/
> 
> ... obviously I'm oversimplifying by quite some margin, but I'm sure
> you catch my drift.

I should extend that a little.

In the meantime I certainly wouldn't have a problem with you using the
"syscon", "simple-bus" approach as others have done already.

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

[PATCH 01/11] mfd: add the Berlin controller driver

[Lee Jones]

On Wed, 18 Feb 2015, Lee Jones wrote:

> On Wed, 18 Feb 2015, Sebastian Hesselbarth wrote:
> 
> > On 02/18/2015 12:58 PM, Lee Jones wrote:
> > >I do agree that using 'simple-bus' to describe only this IP would be
> > >an abuse.  However, my foundation thought/argument is unchanged.  This
> > >'driver' is a hack.  It has no functional use besides to work around a
> > >problem of semantics and as such has no place in MFD.
> > 
> > Lee,
> > 
> > sorry I don't get it. Here you say that using simple-bus is an abuse...
> > 
> > >Back onto the simple-bus theme, as this is a syscon device it is a bus
> > >of sorts.  Have you thought about making it a child of your its syscon
> > >node, then using simple-bus to get the OF framework to register the
> > >child devices?
> > 
> > ... and here you suggest to use simple-bus to register the child
> > devices?
> 
> Nope, that's not what I said:
> 
>   "I do agree that using 'simple-bus' to describe *ONLY THIS IP* would
>   be an abuse."
> 
> ... although I believe there is a need to treat syscon devices as
> simple buses.  There are examples of devices doing this already:
> 
> git grep -El 'syscon.*simple-bus' next/master
>   next/master:arch/arm/boot/dts/imx6qdl.dtsi
>   next/master:arch/arm/boot/dts/imx6sl.dtsi
>   next/master:arch/arm/boot/dts/imx6sx.dtsi
>   next/master:arch/arm/boot/dts/zynq-7000.dtsi
> 
> > I fundamentally disagree that either this registers or syscon in general
> > should in any way be seen as a bus. The chip control registers is an
> > highly unsorted bunch of bits that we try to match with cleanly
> > separated subsystems. This makes it a resource but no bus of any sort.
> 
> This is where my comment about semantics comes into play.  syscon may
> not be a bus is the truest sense; however, this is clearly a
> requirement for sub devices to be probed in the same way a simple-bus
> is currently.  So we're just missing a framework somewhere.  We can
> fix that.
> 
> > The problem that we try to solve here is not a DT problem but solely
> > driven by the fact that we need something to register platform_devices
> > for pinctrl and reset. The unit we describe in DT is a pinctrl-clock-
> > power-reset-unit - or short chip control.
> 
> I agree with the last part, but this is a DT problem.  It lacks the
> functionality to be able to cleanly register these types of
> (sub-)devices.  Devices which, in my opinion should be described
> inside the parent syscon node.
> 
> > If you argue that mfd is not the right place for this "driver" we'll
> > have to find a different place for it. I remember Mike has no problem
> > with extending early probed clock drivers to register additional
> > platform_devices - so I guess we end up putting it in there ignoring
> > mfd's ability to do it for us.
> 
> My argument is not that this fake driver doesn't belong in MFD, it's
> that it doesn't belong.  That includes shoving it in drivers/clk.  I
> will be only too happy to have a chat with Mike and provide him with
> my reasons why.
> 
> What I think we should do however, it write some framework code which
> can neatly handle these use-cases, which may just be a case of:
> 
>   s/of_platform_bus_probe/of_platform_subdevice_probe/
> 
> ... obviously I'm oversimplifying by quite some margin, but I'm sure
> you catch my drift.

I should extend that a little.

In the meantime I certainly wouldn't have a problem with you using the
"syscon", "simple-bus" approach as others have done already.

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org ? Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

Re: [PATCH 01/11] mfd: add the Berlin controller driver

[Sebastian Hesselbarth]

On 02/18/2015 04:06 PM, Arnd Bergmann wrote:
> On Wednesday 18 February 2015 14:09:04 Sebastian Hesselbarth wrote:
>> I fundamentally disagree that either this registers or syscon in general
>> should in any way be seen as a bus. The chip control registers is an
>> highly unsorted bunch of bits that we try to match with cleanly
>> separated subsystems. This makes it a resource but no bus of any sort.
>
> It really depends on what you mean by 'bus'. It's certainly not a bus_type
> in Linux, but if you have a node in DT with multiple children, we
> sometimes think of that as a bus. I believe it makes sense to have
> the child devices under the syscon node here, at least the ones that
> have no other registers.

Arnd, Lee,

First of all, I think I get the points of both of you.

With 'bus' I was referring to anything that implies a fixed number to
address any of the sub-units. As the register is more or less unsorted
and unordered, I'd see it as a resource - but that isn't important
really.

>> The problem that we try to solve here is not a DT problem but solely
>> driven by the fact that we need something to register platform_devices
>> for pinctrl and reset. The unit we describe in DT is a pinctrl-clock-
>> power-reset-unit - or short chip control.
>
> There are two problems here that we need to look at separately,
> even though there is some interaction:
>
> * For the DT representation, we need to make it something that
> corresponds to the hardware. We could either have a single device
> node for the set of registers, or we could have one node for each
> child. With syscon, we could also put the functional device nodes
> somewhere else, which we have to do if any device uses multiple
> syscon parents.

Well, during the discussion, I think we can also get along with a
single node for the whole chip-registers - even in DT. Clock and
reset just require corresponding #foo-cells and pinctrl will have
its pinmux sub-nodes right in that single node. If we have separate
sub-nodes for each of the subsystems is mainly a matter of taste,
right?

> * For the driver code, we need a way to fit into the kernel model
> while at the same time using the information that is in DT.
> I agree with Lee that your current driver is not a good solution
> here: you create a driver for the parent device that knows what
> child devices there are, which is not a good abstraction. Instead
> we should have a way for the child devices to get probed automatically,
> just like we do for simple-bus, whether we use simple-bus or not.

With no sub-nodes, we'd have to have a driver that knows the linux
subsystem platform_devices. With sub-nodes, you are proposing a
"syscon-bus"-like compatible hook to populate sub-devices. Ok, I
get this.

>> If you argue that mfd is not the right place for this "driver" we'll
>> have to find a different place for it. I remember Mike has no problem
>> with extending early probed clock drivers to register additional
>> platform_devices - so I guess we end up putting it in there ignoring
>> mfd's ability to do it for us.
>
> If you have a driver that is responsible for the entire register
> area, I think it would be best to make that driver just register
> to all the subsystems itself rather than creating child devices.

Hmm. That would create a beast of a driver wouldn't it? We could
get along with a library-like structure we each of foo-related
code resides in drivers/foo but still we'd need some common include
to reference to the sub-driver.

> The alternative is to come up with a way to probe all the child
> devices automatically, but then we should make that parent device
> have a generic driver that does not need to know about the children
> and that can work on any platform with similar requirements.

Ok, this is most likely the part that Lee doesn't like on the current
driver: a platform_device for registering platform_devices *only* and
only for Berlin.

So, out of the two options:

(a) Go for syscon_of_populate_devices() with a new compatible (I guess)
     and having sub-nodes for each Linux subsystem that we want to have
     a platform_device for. I fear that this will clash with early
     registration of clk and we still have to find a way, i.e. device
     naming policy, to match the drivers with their devices.

(b) Join clk, pinctrl, reset into a single chip/soc-control node and
     rewrite the sub-drivers to not directly rely on DT compatible.
     With this, joining all sub-drivers into drivers/soc/berlin would
     be a sane approach, right? Also, I have the strong feeling, that
     we will encounter situations later that will require the clk driver
     to pull a reset before changing a specific clk rate, e.g. for GPU.

Anyway, I appreciate your discussion but still I am a little lost
between the two options. I thought that Antoine's mfd approach is
good, but I understand your concerns.

Any direction we should go for?

Sebastian

[PATCH 01/11] mfd: add the Berlin controller driver

[Sebastian Hesselbarth]

On 02/18/2015 04:06 PM, Arnd Bergmann wrote:
> On Wednesday 18 February 2015 14:09:04 Sebastian Hesselbarth wrote:
>> I fundamentally disagree that either this registers or syscon in general
>> should in any way be seen as a bus. The chip control registers is an
>> highly unsorted bunch of bits that we try to match with cleanly
>> separated subsystems. This makes it a resource but no bus of any sort.
>
> It really depends on what you mean by 'bus'. It's certainly not a bus_type
> in Linux, but if you have a node in DT with multiple children, we
> sometimes think of that as a bus. I believe it makes sense to have
> the child devices under the syscon node here, at least the ones that
> have no other registers.

Arnd, Lee,

First of all, I think I get the points of both of you.

With 'bus' I was referring to anything that implies a fixed number to
address any of the sub-units. As the register is more or less unsorted
and unordered, I'd see it as a resource - but that isn't important
really.

>> The problem that we try to solve here is not a DT problem but solely
>> driven by the fact that we need something to register platform_devices
>> for pinctrl and reset. The unit we describe in DT is a pinctrl-clock-
>> power-reset-unit - or short chip control.
>
> There are two problems here that we need to look at separately,
> even though there is some interaction:
>
> * For the DT representation, we need to make it something that
> corresponds to the hardware. We could either have a single device
> node for the set of registers, or we could have one node for each
> child. With syscon, we could also put the functional device nodes
> somewhere else, which we have to do if any device uses multiple
> syscon parents.

Well, during the discussion, I think we can also get along with a
single node for the whole chip-registers - even in DT. Clock and
reset just require corresponding #foo-cells and pinctrl will have
its pinmux sub-nodes right in that single node. If we have separate
sub-nodes for each of the subsystems is mainly a matter of taste,
right?

> * For the driver code, we need a way to fit into the kernel model
> while at the same time using the information that is in DT.
> I agree with Lee that your current driver is not a good solution
> here: you create a driver for the parent device that knows what
> child devices there are, which is not a good abstraction. Instead
> we should have a way for the child devices to get probed automatically,
> just like we do for simple-bus, whether we use simple-bus or not.

With no sub-nodes, we'd have to have a driver that knows the linux
subsystem platform_devices. With sub-nodes, you are proposing a
"syscon-bus"-like compatible hook to populate sub-devices. Ok, I
get this.

>> If you argue that mfd is not the right place for this "driver" we'll
>> have to find a different place for it. I remember Mike has no problem
>> with extending early probed clock drivers to register additional
>> platform_devices - so I guess we end up putting it in there ignoring
>> mfd's ability to do it for us.
>
> If you have a driver that is responsible for the entire register
> area, I think it would be best to make that driver just register
> to all the subsystems itself rather than creating child devices.

Hmm. That would create a beast of a driver wouldn't it? We could
get along with a library-like structure we each of foo-related
code resides in drivers/foo but still we'd need some common include
to reference to the sub-driver.

> The alternative is to come up with a way to probe all the child
> devices automatically, but then we should make that parent device
> have a generic driver that does not need to know about the children
> and that can work on any platform with similar requirements.

Ok, this is most likely the part that Lee doesn't like on the current
driver: a platform_device for registering platform_devices *only* and
only for Berlin.

So, out of the two options:

(a) Go for syscon_of_populate_devices() with a new compatible (I guess)
     and having sub-nodes for each Linux subsystem that we want to have
     a platform_device for. I fear that this will clash with early
     registration of clk and we still have to find a way, i.e. device
     naming policy, to match the drivers with their devices.

(b) Join clk, pinctrl, reset into a single chip/soc-control node and
     rewrite the sub-drivers to not directly rely on DT compatible.
     With this, joining all sub-drivers into drivers/soc/berlin would
     be a sane approach, right? Also, I have the strong feeling, that
     we will encounter situations later that will require the clk driver
     to pull a reset before changing a specific clk rate, e.g. for GPU.

Anyway, I appreciate your discussion but still I am a little lost
between the two options. I thought that Antoine's mfd approach is
good, but I understand your concerns.

Any direction we should go for?

Sebastian

Re: [PATCH 01/11] mfd: add the Berlin controller driver

[Arnd Bergmann]

On Wednesday 18 February 2015 16:59:42 Sebastian Hesselbarth wrote:
> 
> > The alternative is to come up with a way to probe all the child
> > devices automatically, but then we should make that parent device
> > have a generic driver that does not need to know about the children
> > and that can work on any platform with similar requirements.
> 
> Ok, this is most likely the part that Lee doesn't like on the current
> driver: a platform_device for registering platform_devices *only* and
> only for Berlin.
> 
> So, out of the two options:
> 
> (a) Go for syscon_of_populate_devices() with a new compatible (I guess)
>      and having sub-nodes for each Linux subsystem that we want to have
>      a platform_device for. I fear that this will clash with early
>      registration of clk and we still have to find a way, i.e. device
>      naming policy, to match the drivers with their devices.

I don't see the problem with early clk registration, AFAICT it should
just work as expected, you just end up with an extra platform_device
for the clocks that does not get bound to a driver later because the
device node is already in use by the clock driver.

> (b) Join clk, pinctrl, reset into a single chip/soc-control node and
>      rewrite the sub-drivers to not directly rely on DT compatible.
>      With this, joining all sub-drivers into drivers/soc/berlin would
>      be a sane approach, right? Also, I have the strong feeling, that
>      we will encounter situations later that will require the clk driver
>      to pull a reset before changing a specific clk rate, e.g. for GPU.

If we do this, I think it should be a single driver as well, without
subdrivers. We should probably just do this if there is a small number
of subsystems to bind to, so the driver doesn't get out of hand.

This driver could live in drivers/soc then. 

If you want to have subdrivers after all, that would be a classic
MFD and should live in drivers/mfd. The binding would be the same
for both approaches.

	Arnd

[PATCH 01/11] mfd: add the Berlin controller driver

[Arnd Bergmann]

On Wednesday 18 February 2015 16:59:42 Sebastian Hesselbarth wrote:
> 
> > The alternative is to come up with a way to probe all the child
> > devices automatically, but then we should make that parent device
> > have a generic driver that does not need to know about the children
> > and that can work on any platform with similar requirements.
> 
> Ok, this is most likely the part that Lee doesn't like on the current
> driver: a platform_device for registering platform_devices *only* and
> only for Berlin.
> 
> So, out of the two options:
> 
> (a) Go for syscon_of_populate_devices() with a new compatible (I guess)
>      and having sub-nodes for each Linux subsystem that we want to have
>      a platform_device for. I fear that this will clash with early
>      registration of clk and we still have to find a way, i.e. device
>      naming policy, to match the drivers with their devices.

I don't see the problem with early clk registration, AFAICT it should
just work as expected, you just end up with an extra platform_device
for the clocks that does not get bound to a driver later because the
device node is already in use by the clock driver.

> (b) Join clk, pinctrl, reset into a single chip/soc-control node and
>      rewrite the sub-drivers to not directly rely on DT compatible.
>      With this, joining all sub-drivers into drivers/soc/berlin would
>      be a sane approach, right? Also, I have the strong feeling, that
>      we will encounter situations later that will require the clk driver
>      to pull a reset before changing a specific clk rate, e.g. for GPU.

If we do this, I think it should be a single driver as well, without
subdrivers. We should probably just do this if there is a small number
of subsystems to bind to, so the driver doesn't get out of hand.

This driver could live in drivers/soc then. 

If you want to have subdrivers after all, that would be a classic
MFD and should live in drivers/mfd. The binding would be the same
for both approaches.

	Arnd

Re: [PATCH 01/11] mfd: add the Berlin controller driver

[Lee Jones]

On Wed, 18 Feb 2015, Sebastian Hesselbarth wrote:

> On 02/18/2015 04:06 PM, Arnd Bergmann wrote:
> >On Wednesday 18 February 2015 14:09:04 Sebastian Hesselbarth wrote:
> >>I fundamentally disagree that either this registers or syscon in general
> >>should in any way be seen as a bus. The chip control registers is an
> >>highly unsorted bunch of bits that we try to match with cleanly
> >>separated subsystems. This makes it a resource but no bus of any sort.
> >
> >It really depends on what you mean by 'bus'. It's certainly not a bus_type
> >in Linux, but if you have a node in DT with multiple children, we
> >sometimes think of that as a bus. I believe it makes sense to have
> >the child devices under the syscon node here, at least the ones that
> >have no other registers.
> 
> Arnd, Lee,
> 
> First of all, I think I get the points of both of you.
> 
> With 'bus' I was referring to anything that implies a fixed number to
> address any of the sub-units. As the register is more or less unsorted
> and unordered, I'd see it as a resource - but that isn't important
> really.
> 
> >>The problem that we try to solve here is not a DT problem but solely
> >>driven by the fact that we need something to register platform_devices
> >>for pinctrl and reset. The unit we describe in DT is a pinctrl-clock-
> >>power-reset-unit - or short chip control.
> >
> >There are two problems here that we need to look at separately,
> >even though there is some interaction:
> >
> >* For the DT representation, we need to make it something that
> >corresponds to the hardware. We could either have a single device
> >node for the set of registers, or we could have one node for each
> >child. With syscon, we could also put the functional device nodes
> >somewhere else, which we have to do if any device uses multiple
> >syscon parents.
> 
> Well, during the discussion, I think we can also get along with a
> single node for the whole chip-registers - even in DT. Clock and
> reset just require corresponding #foo-cells and pinctrl will have
> its pinmux sub-nodes right in that single node. If we have separate
> sub-nodes for each of the subsystems is mainly a matter of taste,
> right?
> 
> >* For the driver code, we need a way to fit into the kernel model
> >while at the same time using the information that is in DT.
> >I agree with Lee that your current driver is not a good solution
> >here: you create a driver for the parent device that knows what
> >child devices there are, which is not a good abstraction. Instead
> >we should have a way for the child devices to get probed automatically,
> >just like we do for simple-bus, whether we use simple-bus or not.
> 
> With no sub-nodes, we'd have to have a driver that knows the linux
> subsystem platform_devices. With sub-nodes, you are proposing a
> "syscon-bus"-like compatible hook to populate sub-devices. Ok, I
> get this.
> 
> >>If you argue that mfd is not the right place for this "driver" we'll
> >>have to find a different place for it. I remember Mike has no problem
> >>with extending early probed clock drivers to register additional
> >>platform_devices - so I guess we end up putting it in there ignoring
> >>mfd's ability to do it for us.
> >
> >If you have a driver that is responsible for the entire register
> >area, I think it would be best to make that driver just register
> >to all the subsystems itself rather than creating child devices.
> 
> Hmm. That would create a beast of a driver wouldn't it? We could
> get along with a library-like structure we each of foo-related
> code resides in drivers/foo but still we'd need some common include
> to reference to the sub-driver.
> 
> >The alternative is to come up with a way to probe all the child
> >devices automatically, but then we should make that parent device
> >have a generic driver that does not need to know about the children
> >and that can work on any platform with similar requirements.
> 
> Ok, this is most likely the part that Lee doesn't like on the current
> driver: a platform_device for registering platform_devices *only* and
> only for Berlin.
> 
> So, out of the two options:
> 
> (a) Go for syscon_of_populate_devices() with a new compatible (I guess)
>     and having sub-nodes for each Linux subsystem that we want to have
>     a platform_device for. I fear that this will clash with early
>     registration of clk and we still have to find a way, i.e. device
>     naming policy, to match the drivers with their devices.
> 
> (b) Join clk, pinctrl, reset into a single chip/soc-control node and
>     rewrite the sub-drivers to not directly rely on DT compatible.
>     With this, joining all sub-drivers into drivers/soc/berlin would
>     be a sane approach, right? Also, I have the strong feeling, that
>     we will encounter situations later that will require the clk driver
>     to pull a reset before changing a specific clk rate, e.g. for GPU.
> 
> Anyway, I appreciate your discussion but still I am a little lost
> between the two options. I thought that Antoine's mfd approach is
> good, but I understand your concerns.
> 
> Any direction we should go for?

FWIW, my person opinion would be to keep the drivers separate (not
least to keep drivers/soc from becoming the next dumping ground after
drivers/mfd and drivers/misc had had enough) and place them inside
their own subsystems, keep the device tree node hierarchy and place the
parent node under syscon.  After all, that is what this device
represents.

Then, as you say, create a nice framework which elegantly probes each
of the platform device drivers in turn.  That way each of the drivers
get to keep their own compatible string which you can use to match on
using current framework helpers.

simple-bus already does what you want.  The only issue here is
semantics (naming).  As I've alluded to already, people are currently
using (abusing?) this stuff, therefore it must be suitable, at least
on a functional (rather than religious) level.

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

[PATCH 01/11] mfd: add the Berlin controller driver

[Lee Jones]

On Wed, 18 Feb 2015, Sebastian Hesselbarth wrote:

> On 02/18/2015 04:06 PM, Arnd Bergmann wrote:
> >On Wednesday 18 February 2015 14:09:04 Sebastian Hesselbarth wrote:
> >>I fundamentally disagree that either this registers or syscon in general
> >>should in any way be seen as a bus. The chip control registers is an
> >>highly unsorted bunch of bits that we try to match with cleanly
> >>separated subsystems. This makes it a resource but no bus of any sort.
> >
> >It really depends on what you mean by 'bus'. It's certainly not a bus_type
> >in Linux, but if you have a node in DT with multiple children, we
> >sometimes think of that as a bus. I believe it makes sense to have
> >the child devices under the syscon node here, at least the ones that
> >have no other registers.
> 
> Arnd, Lee,
> 
> First of all, I think I get the points of both of you.
> 
> With 'bus' I was referring to anything that implies a fixed number to
> address any of the sub-units. As the register is more or less unsorted
> and unordered, I'd see it as a resource - but that isn't important
> really.
> 
> >>The problem that we try to solve here is not a DT problem but solely
> >>driven by the fact that we need something to register platform_devices
> >>for pinctrl and reset. The unit we describe in DT is a pinctrl-clock-
> >>power-reset-unit - or short chip control.
> >
> >There are two problems here that we need to look at separately,
> >even though there is some interaction:
> >
> >* For the DT representation, we need to make it something that
> >corresponds to the hardware. We could either have a single device
> >node for the set of registers, or we could have one node for each
> >child. With syscon, we could also put the functional device nodes
> >somewhere else, which we have to do if any device uses multiple
> >syscon parents.
> 
> Well, during the discussion, I think we can also get along with a
> single node for the whole chip-registers - even in DT. Clock and
> reset just require corresponding #foo-cells and pinctrl will have
> its pinmux sub-nodes right in that single node. If we have separate
> sub-nodes for each of the subsystems is mainly a matter of taste,
> right?
> 
> >* For the driver code, we need a way to fit into the kernel model
> >while at the same time using the information that is in DT.
> >I agree with Lee that your current driver is not a good solution
> >here: you create a driver for the parent device that knows what
> >child devices there are, which is not a good abstraction. Instead
> >we should have a way for the child devices to get probed automatically,
> >just like we do for simple-bus, whether we use simple-bus or not.
> 
> With no sub-nodes, we'd have to have a driver that knows the linux
> subsystem platform_devices. With sub-nodes, you are proposing a
> "syscon-bus"-like compatible hook to populate sub-devices. Ok, I
> get this.
> 
> >>If you argue that mfd is not the right place for this "driver" we'll
> >>have to find a different place for it. I remember Mike has no problem
> >>with extending early probed clock drivers to register additional
> >>platform_devices - so I guess we end up putting it in there ignoring
> >>mfd's ability to do it for us.
> >
> >If you have a driver that is responsible for the entire register
> >area, I think it would be best to make that driver just register
> >to all the subsystems itself rather than creating child devices.
> 
> Hmm. That would create a beast of a driver wouldn't it? We could
> get along with a library-like structure we each of foo-related
> code resides in drivers/foo but still we'd need some common include
> to reference to the sub-driver.
> 
> >The alternative is to come up with a way to probe all the child
> >devices automatically, but then we should make that parent device
> >have a generic driver that does not need to know about the children
> >and that can work on any platform with similar requirements.
> 
> Ok, this is most likely the part that Lee doesn't like on the current
> driver: a platform_device for registering platform_devices *only* and
> only for Berlin.
> 
> So, out of the two options:
> 
> (a) Go for syscon_of_populate_devices() with a new compatible (I guess)
>     and having sub-nodes for each Linux subsystem that we want to have
>     a platform_device for. I fear that this will clash with early
>     registration of clk and we still have to find a way, i.e. device
>     naming policy, to match the drivers with their devices.
> 
> (b) Join clk, pinctrl, reset into a single chip/soc-control node and
>     rewrite the sub-drivers to not directly rely on DT compatible.
>     With this, joining all sub-drivers into drivers/soc/berlin would
>     be a sane approach, right? Also, I have the strong feeling, that
>     we will encounter situations later that will require the clk driver
>     to pull a reset before changing a specific clk rate, e.g. for GPU.
> 
> Anyway, I appreciate your discussion but still I am a little lost
> between the two options. I thought that Antoine's mfd approach is
> good, but I understand your concerns.
> 
> Any direction we should go for?

FWIW, my person opinion would be to keep the drivers separate (not
least to keep drivers/soc from becoming the next dumping ground after
drivers/mfd and drivers/misc had had enough) and place them inside
their own subsystems, keep the device tree node hierarchy and place the
parent node under syscon.  After all, that is what this device
represents.

Then, as you say, create a nice framework which elegantly probes each
of the platform device drivers in turn.  That way each of the drivers
get to keep their own compatible string which you can use to match on
using current framework helpers.

simple-bus already does what you want.  The only issue here is
semantics (naming).  As I've alluded to already, people are currently
using (abusing?) this stuff, therefore it must be suitable, at least
on a functional (rather than religious) level.

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org ? Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

Re: [PATCH 06/11] pinctrl: berlin: use the regmap provided by syscon

[Linus Walleij]

On Wed, Feb 11, 2015 at 5:15 PM, Antoine Tenart
<antoine.tenart@free-electrons.com> wrote:

> The Berlin pin controller nodes are now sub-nodes of the soc-controller
> and the system-controller nodes. The register bank is managed by syscon,
> which provides a regmap.
>
> Remove the regmap setup from the Berlin pinctrl driver and use the one
> provided by syscon.
>
> Signed-off-by: Antoine Tenart <antoine.tenart@free-electrons.com>

Sweet:
Acked-by: Linus Walleij <linus.walleij@linaro.org>

I guess you'll merge this through ARM SoC?

Else tell me if you prefer that I merge it into the pinctrl tree.

Yours,
Linus Walleij

Re: [PATCH 06/11] pinctrl: berlin: use the regmap provided by syscon

[Linus Walleij]

On Wed, Feb 11, 2015 at 5:15 PM, Antoine Tenart
<antoine.tenart@free-electrons.com> wrote:

> The Berlin pin controller nodes are now sub-nodes of the soc-controller
> and the system-controller nodes. The register bank is managed by syscon,
> which provides a regmap.
>
> Remove the regmap setup from the Berlin pinctrl driver and use the one
> provided by syscon.
>
> Signed-off-by: Antoine Tenart <antoine.tenart@free-electrons.com>

Sweet:
Acked-by: Linus Walleij <linus.walleij@linaro.org>

I guess you'll merge this through ARM SoC?

Else tell me if you prefer that I merge it into the pinctrl tree.

Yours,
Linus Walleij

[PATCH 06/11] pinctrl: berlin: use the regmap provided by syscon

[Linus Walleij]

On Wed, Feb 11, 2015 at 5:15 PM, Antoine Tenart
<antoine.tenart@free-electrons.com> wrote:

> The Berlin pin controller nodes are now sub-nodes of the soc-controller
> and the system-controller nodes. The register bank is managed by syscon,
> which provides a regmap.
>
> Remove the regmap setup from the Berlin pinctrl driver and use the one
> provided by syscon.
>
> Signed-off-by: Antoine Tenart <antoine.tenart@free-electrons.com>

Sweet:
Acked-by: Linus Walleij <linus.walleij@linaro.org>

I guess you'll merge this through ARM SoC?

Else tell me if you prefer that I merge it into the pinctrl tree.

Yours,
Linus Walleij

Re: [PATCH 07/11] pinctrl: berlin: use proper compatibles

[Linus Walleij]

On Wed, Feb 11, 2015 at 5:15 PM, Antoine Tenart
<antoine.tenart@free-electrons.com> wrote:

> The Berlin pin-controller driver was sharing the chip and system
> controller nodes with the clock and the reset drivers. They all shared
> the same compatible. With the introduction of the Marvell Berlin MFD
> controller, the Berlin pin-controller driver has now its own node.
> Update its compatibles to not share anymore the ones of the chip and
> system controllers.
>
> Signed-off-by: Antoine Tenart <antoine.tenart@free-electrons.com>

Acked-by: Linus Walleij <linus.walleij@linaro.org>

Tell me if you prefer that I merge this patch directly to the
pin control subsystem.

Yours,
Linus Walleij

Re: [PATCH 07/11] pinctrl: berlin: use proper compatibles

[Linus Walleij]

On Wed, Feb 11, 2015 at 5:15 PM, Antoine Tenart
<antoine.tenart@free-electrons.com> wrote:

> The Berlin pin-controller driver was sharing the chip and system
> controller nodes with the clock and the reset drivers. They all shared
> the same compatible. With the introduction of the Marvell Berlin MFD
> controller, the Berlin pin-controller driver has now its own node.
> Update its compatibles to not share anymore the ones of the chip and
> system controllers.
>
> Signed-off-by: Antoine Tenart <antoine.tenart@free-electrons.com>

Acked-by: Linus Walleij <linus.walleij@linaro.org>

Tell me if you prefer that I merge this patch directly to the
pin control subsystem.

Yours,
Linus Walleij

[PATCH 07/11] pinctrl: berlin: use proper compatibles

[Linus Walleij]

On Wed, Feb 11, 2015 at 5:15 PM, Antoine Tenart
<antoine.tenart@free-electrons.com> wrote:

> The Berlin pin-controller driver was sharing the chip and system
> controller nodes with the clock and the reset drivers. They all shared
> the same compatible. With the introduction of the Marvell Berlin MFD
> controller, the Berlin pin-controller driver has now its own node.
> Update its compatibles to not share anymore the ones of the chip and
> system controllers.
>
> Signed-off-by: Antoine Tenart <antoine.tenart@free-electrons.com>

Acked-by: Linus Walleij <linus.walleij@linaro.org>

Tell me if you prefer that I merge this patch directly to the
pin control subsystem.

Yours,
Linus Walleij

Re: [PATCH 08/11] Documentation: bindings: move the Berlin pinctrl documentation

[Linus Walleij]

On Wed, Feb 11, 2015 at 5:15 PM, Antoine Tenart
<antoine.tenart@free-electrons.com> wrote:

> The Berlin pinctrl documentation was part of the Marvell Berlin SoC
> documentation because the Berlin pinctrl configuration was inside the
> chip and the system controllers. With the recent rework of the chip and
> system controller handling (now an MFD driver registers all sub-devices
> of the two soc and system controller nodes and each device has its own
> sub-node), the documentation of the Berlin pinctrl driver can be moved
> to the generic pinctrl documentation directory.
>
> Signed-off-by: Antoine Tenart <antoine.tenart@free-electrons.com>

Acked-by: Linus Walleij <linus.walleij@linaro.org>

I tried applying the patch but it doesn't work. Rebase on v4.0-rc1 and
resend if you want me to apply it to the pinctrl tree.

Yours,
Linus Walleij

Re: [PATCH 08/11] Documentation: bindings: move the Berlin pinctrl documentation

[Linus Walleij]

On Wed, Feb 11, 2015 at 5:15 PM, Antoine Tenart
<antoine.tenart@free-electrons.com> wrote:

> The Berlin pinctrl documentation was part of the Marvell Berlin SoC
> documentation because the Berlin pinctrl configuration was inside the
> chip and the system controllers. With the recent rework of the chip and
> system controller handling (now an MFD driver registers all sub-devices
> of the two soc and system controller nodes and each device has its own
> sub-node), the documentation of the Berlin pinctrl driver can be moved
> to the generic pinctrl documentation directory.
>
> Signed-off-by: Antoine Tenart <antoine.tenart@free-electrons.com>

Acked-by: Linus Walleij <linus.walleij@linaro.org>

I tried applying the patch but it doesn't work. Rebase on v4.0-rc1 and
resend if you want me to apply it to the pinctrl tree.

Yours,
Linus Walleij

[PATCH 08/11] Documentation: bindings: move the Berlin pinctrl documentation

[Linus Walleij]

On Wed, Feb 11, 2015 at 5:15 PM, Antoine Tenart
<antoine.tenart@free-electrons.com> wrote:

> The Berlin pinctrl documentation was part of the Marvell Berlin SoC
> documentation because the Berlin pinctrl configuration was inside the
> chip and the system controllers. With the recent rework of the chip and
> system controller handling (now an MFD driver registers all sub-devices
> of the two soc and system controller nodes and each device has its own
> sub-node), the documentation of the Berlin pinctrl driver can be moved
> to the generic pinctrl documentation directory.
>
> Signed-off-by: Antoine Tenart <antoine.tenart@free-electrons.com>

Acked-by: Linus Walleij <linus.walleij@linaro.org>

I tried applying the patch but it doesn't work. Rebase on v4.0-rc1 and
resend if you want me to apply it to the pinctrl tree.

Yours,
Linus Walleij