[PATCH 1/2] dt-bindings: reset: document Broadcom's PMC binding

[PATCH 1/2] dt-bindings: reset: document Broadcom's PMC binding

[Rafał Miłecki]

From: Rafał Miłecki <rafal@milecki.pl>

Broadcom's PMC is Power Management Controller that is used for disabling
and enabling SoC devices.

Signed-off-by: Rafał Miłecki <rafal@milecki.pl>
---
 .../bindings/reset/brcm,bcm-pmc.yaml          | 69 +++++++++++++++++++
 1 file changed, 69 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/reset/brcm,bcm-pmc.yaml

diff --git a/Documentation/devicetree/bindings/reset/brcm,bcm-pmc.yaml b/Documentation/devicetree/bindings/reset/brcm,bcm-pmc.yaml
new file mode 100644
index 000000000000..2afc2048997f
--- /dev/null
+++ b/Documentation/devicetree/bindings/reset/brcm,bcm-pmc.yaml
@@ -0,0 +1,69 @@
+# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/reset/brcm,bcm-pmc.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Broadcom PMC (Power Management Controller) reset controller
+
+description: This document describes the Broadcom's PMC (Power Management
+  Controller). It supports resetting devices identified by the bus id and device
+  address.
+
+maintainers:
+  - Rafał Miłecki <rafal@milecki.pl>
+
+properties:
+  compatible:
+    enum:
+      - brcm,bcm4908-pmc # PMC on BCM4908 and compatible SoCs
+
+  reg:
+    maxItems: 1
+
+  syscon-misc:
+    $ref: /schemas/types.yaml#/definitions/phandle
+    description:
+      Phandle to the MISC system controller node.
+
+  syscon-procmon:
+    $ref: /schemas/types.yaml#/definitions/phandle
+    description:
+      Phandle to the PROC MON system controller node.
+
+  big-endian:
+    $ref: /schemas/types.yaml#/definitions/flag
+    description:
+      Flag to use for block working in big endian mode.
+
+  "#reset-cells":
+    const: 2
+
+required:
+  - compatible
+  - reg
+  - syscon-misc
+  - syscon-procmon
+  - "#reset-cells"
+
+additionalProperties: false
+
+examples:
+  - |
+    pmc: reset-controller@80200000 {
+        compatible = "brcm,bcm4908-pmc";
+        reg = <0x80200000 0x5000>;
+        syscon-misc = <&misc>;
+        syscon-procmon = <&procmon>;
+        #reset-cells = <2>;
+    };
+
+    procmon: syscon@80280000 {
+        compatible = "brcm,misc", "syscon";
+        reg = <0x80280000 0x1000>;
+    };
+
+    misc: syscon@ff802600 {
+        compatible = "brcm,misc", "syscon";
+        reg = <0xff802600 0xe4>;
+    };
-- 
2.27.0

[PATCH 2/2] reset: brcm-pmc: add driver for Broadcom's PMC

[Rafał Miłecki]

From: Rafał Miłecki <rafal@milecki.pl>

Power Management Controller can be found on BCM4908 and is needed to
power on SoC blocks. It can be controlled using:
1. Firmware calls
2. Direct regs access

Only the later method is supported by this initial driver. It was
successfully tested on BCM4908 using HCD controllers.

Signed-off-by: Rafał Miłecki <rafal@milecki.pl>
---
 drivers/reset/Kconfig          |   8 +
 drivers/reset/Makefile         |   1 +
 drivers/reset/reset-brcm-pmc.c | 415 +++++++++++++++++++++++++++++++++
 3 files changed, 424 insertions(+)
 create mode 100644 drivers/reset/reset-brcm-pmc.c

diff --git a/drivers/reset/Kconfig b/drivers/reset/Kconfig
index 84baec01aa30..d27104e7fa24 100644
--- a/drivers/reset/Kconfig
+++ b/drivers/reset/Kconfig
@@ -41,6 +41,14 @@ config RESET_BERLIN
 	help
 	  This enables the reset controller driver for Marvell Berlin SoCs.
 
+config RESET_BRCM_PMC
+	tristate "Broadcom PMC reset controller"
+	depends on ARCH_BCM4908 || COMPILE_TEST
+	default ARCH_BCM4908
+	help
+	  This enables the Broadcom PMC (Power Management Controller) reset
+	  controller driver.
+
 config RESET_BRCMSTB
 	tristate "Broadcom STB reset controller"
 	depends on ARCH_BRCMSTB || COMPILE_TEST
diff --git a/drivers/reset/Makefile b/drivers/reset/Makefile
index 16947610cc3b..bbab031e9f19 100644
--- a/drivers/reset/Makefile
+++ b/drivers/reset/Makefile
@@ -7,6 +7,7 @@ obj-$(CONFIG_RESET_A10SR) += reset-a10sr.o
 obj-$(CONFIG_RESET_ATH79) += reset-ath79.o
 obj-$(CONFIG_RESET_AXS10X) += reset-axs10x.o
 obj-$(CONFIG_RESET_BERLIN) += reset-berlin.o
+obj-$(CONFIG_RESET_BRCM_PMC) += reset-brcm-pmc.o
 obj-$(CONFIG_RESET_BRCMSTB) += reset-brcmstb.o
 obj-$(CONFIG_RESET_BRCMSTB_RESCAL) += reset-brcmstb-rescal.o
 obj-$(CONFIG_RESET_HSDK) += reset-hsdk.o
diff --git a/drivers/reset/reset-brcm-pmc.c b/drivers/reset/reset-brcm-pmc.c
new file mode 100644
index 000000000000..6ab31f5d026c
--- /dev/null
+++ b/drivers/reset/reset-brcm-pmc.c
@@ -0,0 +1,415 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Copyright (c) 2013 Broadcom
+ * Copyright (C) 2020 Rafał Miłecki <rafal@milecki.pl>
+ */
+
+#include <linux/mfd/syscon.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/of_device.h>
+#include <linux/platform_device.h>
+#include <linux/regmap.h>
+#include <linux/reset-controller.h>
+
+#define PROCMON_MONITORCTRL			0x00
+#define PROCMON_ROSC				0x20
+#define PROCMON_MISC				0x40
+#define PROCMON_SSBMASTER			0x60
+#define PROCMON_ECTR				0x80
+#define PROCMON_PMBM				0xc0	/* PMB Master bus 0 */
+#define  PROCMON_PMBM_SIZE			0x20
+#define PROCMON_PMBM1				0xe0	/* PMB Master bus 1 */
+
+#define PMBM_CTRL				0x00
+#define  PMC_PMBM_START				(1 << 31)
+#define  PMC_PMBM_TIMEOUT			(1 << 30)
+#define  PMC_PMBM_SLAVE_ERR			(1 << 29)
+#define  PMC_PMBM_BUSY				(1 << 28)
+#define  PMC_PMBM_READ				(0 << 20)
+#define  PMC_PMBM_WRITE				(1 << 20)
+#define PMBM_WR_DATA				0x04
+#define PMBM_TIMEOUT				0x08
+#define PMBM_RD_DATA				0x0c
+
+#define BPCM_ID_REG				0x00
+#define BPCM_CAPABILITIES			0x04
+#define BPCM_CONTROL				0x08
+#define BPCM_STATUS				0x0c
+#define BPCM_ROSC_CONTROL			0x10
+#define BPCM_ROSC_THRESH_H			0x14
+#define BPCM_ROSC_THRESHOLD_BCM6838		0x14
+#define BPCM_ROSC_THRESH_S			0x18
+#define BPCM_ROSC_COUNT_BCM6838			0x18
+#define BPCM_ROSC_COUNT				0x1c
+#define BPCM_PWD_CONTROL_BCM6838		0x1c
+#define BPCM_PWD_CONTROL			0x20
+#define BPCM_SR_CONTROL_BCM6838			0x20
+#define BPCM_PWD_ACCUM_CONTROL			0x24
+#define BPCM_SR_CONTROL				0x28
+#define BPCM_GLOBAL_CONTROL			0x2c
+#define BPCM_MISC_CONTROL			0x30
+#define BPCM_MISC_CONTROL2			0x34
+#define BPCM_SGPHY_CNTL				0x38
+#define BPCM_SGPHY_STATUS			0x3c
+#define BPCM_ZONE0				0x40
+#define  BPCM_ZONE_CONTROL			0x00
+#define  BPCM_ZONE_CONFIG1			0x04
+#define  BPCM_ZONE_CONFIG2			0x08
+#define  BPCM_ZONE_FREQ_SCALAR_CONTROL		0x0c
+#define  BPCM_ZONE_SIZE				0x10
+
+enum brcm_pmc_chipset {
+	BCM4908,
+	BCM6848,
+	BCM6858,
+};
+
+struct brcm_pmc_data {
+	enum brcm_pmc_chipset chipset;
+	int misc_strap_bus_reg;
+	int misc_strap_bus_pmc_rom_boot_bit;
+};
+
+static const struct brcm_pmc_data brcm_pmc_4908_data = {
+	.chipset				= BCM4908,
+	.misc_strap_bus_reg			= 0x00,
+	.misc_strap_bus_pmc_rom_boot_bit	= BIT(11),
+};
+
+union bpcm_capabilities {
+	struct {
+		int num_zones:8;
+		int sr_reg_bits:8;
+		int pllType:2;
+		int reserved0:1;
+		int ubus:1;
+		int reserved1:12;
+	} bits;
+	u32 raw32;
+} __attribute__((__packed__));
+
+union bpcm_zone_control {
+	struct {
+		u32 manual_clk_en:1;
+		u32 manual_reset_ctl:1;
+		u32 freq_scale_used:1;		/* R/O */
+		u32 dpg_capable:1;		/* R/O */
+		u32 manual_mem_pwr:2;
+		u32 manual_iso_ctl:1;
+		u32 manual_ctl:1;
+		u32 dpg_ctl_en:1;
+		u32 pwr_dn_req:1;
+		u32 pwr_up_req:1;
+		u32 mem_pwr_ctl_en:1;
+		u32 blk_reset_assert:1;
+		u32 mem_stby:1;
+		u32 reserved:5;
+		u32 pwr_cntl_state:5;
+		u32 freq_scalar_dyn_sel:1;	/* R/O */
+		u32 pwr_off_state:1;		/* R/O */
+		u32 pwr_on_state:1;		/* R/O */
+		u32 pwr_good:1;			/* R/O */
+		u32 dpg_pwr_state:1;		/* R/O */
+		u32 mem_pwr_state:1;		/* R/O */
+		u32 iso_state:1;		/* R/O */
+		u32 reset_state:1;		/* R/O */
+	} bits;
+	u32 raw32;
+} __attribute__((__packed__));
+
+struct brcm_pmc {
+	const struct brcm_pmc_data *data;
+	void __iomem *base;
+	struct regmap *misc;
+	struct regmap *procmon;
+	struct device *dev;
+	struct reset_controller_dev rcdev;
+	bool direct;
+	bool little_endian;
+};
+
+static DEFINE_SPINLOCK(lock);
+
+#define cpu_to_dev32(pmc, val) \
+	((pmc)->little_endian ? cpu_to_le32(val) : cpu_to_be32(val))
+
+#define dev32_to_cpu(pmc, val) \
+	((pmc)->little_endian ? le32_to_cpu(val) : be32_to_cpu(val))
+
+static int brcm_pmc_bpcm_wait(struct brcm_pmc *pmc, int bus)
+{
+	unsigned long deadline = jiffies + usecs_to_jiffies(1000);
+	int pmbm = PROCMON_PMBM + bus * PROCMON_PMBM_SIZE;
+	u32 val;
+
+	do {
+		regmap_read(pmc->procmon, pmbm + PMBM_CTRL, &val);
+		if (!(val & PMC_PMBM_START))
+			return 0;
+		cpu_relax();
+		udelay(10);
+	} while (!time_after_eq(jiffies, deadline));
+
+	dev_err(pmc->dev, "Timeout waiting for the BPCM\n");
+
+	return -EBUSY;
+}
+
+static int brcm_pmc_bpcm_read(struct brcm_pmc *pmc, u16 addr, int word_offset, u32 *val)
+{
+	int err = 0;
+
+	if (!pmc->direct) {
+		err = -EOPNOTSUPP;
+	} else {
+		u8 device = addr & 0xff;
+		u8 bus = addr >> 8;
+		unsigned long flags;
+		int pmbm;
+		u32 tmp;
+
+		pmbm = PROCMON_PMBM + bus * PROCMON_PMBM_SIZE;
+
+		spin_lock_irqsave(&lock, flags);
+
+		/* TODO: Make sure PMBM is not busy */
+
+		regmap_write(pmc->procmon, pmbm + PMBM_CTRL,
+			     PMC_PMBM_START | PMC_PMBM_READ | (device << 12) | word_offset);
+
+		err = brcm_pmc_bpcm_wait(pmc, bus);
+		if (!err)
+			err = regmap_read(pmc->procmon, pmbm + PMBM_RD_DATA, val);
+
+		spin_unlock_irqrestore(&lock, flags);
+	}
+
+	return err;
+}
+
+static int brcm_pmc_bpcm_write(struct brcm_pmc *pmc, u16 addr, int word_offset, u32 val)
+{
+	int err = 0;
+
+	if (!pmc->direct) {
+		err = -EOPNOTSUPP;
+	} else {
+		u8 device = addr & 0xff;
+		u8 bus = addr >> 8;
+		unsigned long flags;
+		int pmbm;
+		u32 tmp;
+		int err = 0;
+
+		pmbm = PROCMON_PMBM + bus * PROCMON_PMBM_SIZE;
+
+		spin_lock_irqsave(&lock, flags);
+
+		/* TODO: Make sure PMBM is not busy */
+
+		regmap_write(pmc->procmon, pmbm + PMBM_WR_DATA, val);
+		regmap_write(pmc->procmon, pmbm + PMBM_CTRL,
+			     PMC_PMBM_START | PMC_PMBM_WRITE | (device << 12) | word_offset);
+
+		err = brcm_pmc_bpcm_wait(pmc, bus);
+
+		spin_unlock_irqrestore(&lock, flags);
+	}
+
+	return err;
+}
+
+static int brcm_pmc_power_on_zone(struct brcm_pmc *pmc, u16 addr, int zone)
+{
+	int err;
+
+	if ((pmc->data->chipset == BCM6848 || pmc->data->chipset == BCM6858) && !pmc->direct) {
+		err = -EOPNOTSUPP;
+	} else {
+		union bpcm_zone_control control;
+		int offset;
+		int err;
+
+		offset = BPCM_ZONE0 + zone * BPCM_ZONE_SIZE + BPCM_ZONE_CONTROL;
+
+		err = brcm_pmc_bpcm_read(pmc, addr, offset >> 2, (u32 *)&control.raw32);
+		if (err)
+			return err;
+		control.raw32 = dev32_to_cpu(pmc, control.raw32);
+
+		if (control.bits.pwr_on_state == 0) {
+			control.bits.pwr_dn_req = 0;
+			control.bits.dpg_ctl_en = 1;
+			control.bits.pwr_up_req = 1;
+			control.bits.mem_pwr_ctl_en = 1;
+			control.bits.blk_reset_assert = 1;
+
+			control.raw32 = cpu_to_dev32(pmc, control.raw32);
+			err = brcm_pmc_bpcm_write(pmc, addr, offset >> 2, control.raw32);
+		}
+	}
+
+	return err;
+}
+
+static int brcm_pmc_power_off_device(struct brcm_pmc *pmc, u16 addr)
+{
+	int err;
+
+	if (!pmc->direct) {
+		err = -EOPNOTSUPP;
+	} else {
+		/* Entire device can be powered off by powering off the 0th zone */
+		union bpcm_zone_control control;
+		int offset;
+		int err;
+
+		offset = BPCM_ZONE0 + BPCM_ZONE_CONTROL;
+
+		err = brcm_pmc_bpcm_read(pmc, addr, offset >> 2, (u32 *)&control.raw32);
+		if (err)
+			return err;
+		control.raw32 = dev32_to_cpu(pmc, control.raw32);
+
+		if (!control.bits.pwr_off_state) {
+			control.bits.pwr_dn_req = 1;
+
+			control.raw32 = cpu_to_dev32(pmc, control.raw32);
+			err = brcm_pmc_bpcm_write(pmc, addr, offset >> 2, control.raw32);
+		}
+	}
+
+	return err;
+}
+
+static int brcm_pmc_power_on_device(struct brcm_pmc *pmc, u16 addr)
+{
+	int err;
+
+	if (!pmc->direct) {
+		err = -EOPNOTSUPP;
+	} else {
+		union bpcm_capabilities caps;
+		int offset;
+		int err;
+		int i;
+
+		offset = BPCM_CAPABILITIES;
+
+		err = brcm_pmc_bpcm_read(pmc, addr, offset >> 2, (u32 *)&caps.raw32);
+		if (err)
+			return err;
+		caps.raw32 = dev32_to_cpu(pmc, caps.raw32);
+
+		for (i = 0; i < caps.bits.num_zones; i++) {
+			err = brcm_pmc_power_on_zone(pmc, addr, i);
+			if (err)
+				return err;
+		}
+	}
+
+	return err;
+}
+
+static int brcm_pmc_assert(struct reset_controller_dev *rcdev, unsigned long id)
+{
+	struct brcm_pmc *pmc = container_of(rcdev, struct brcm_pmc, rcdev);
+
+	return brcm_pmc_power_off_device(pmc, id);
+}
+
+static int brcm_pmc_deassert(struct reset_controller_dev *rcdev, unsigned long id)
+{
+	struct brcm_pmc *pmc = container_of(rcdev, struct brcm_pmc, rcdev);
+
+	return brcm_pmc_power_on_device(pmc, id);
+}
+
+static int brcm_pmc_reset_xlate(struct reset_controller_dev *rcdev,
+				const struct of_phandle_args *reset_spec)
+{
+	u8 device = reset_spec->args[1];
+	u8 bus = reset_spec->args[0];
+
+	if (bus > 1)
+		return -EINVAL;
+
+	return (bus << 8) | device;
+}
+
+static const struct reset_control_ops brcm_pmc_reset_control_ops = {
+	.assert = brcm_pmc_assert,
+	.deassert = brcm_pmc_deassert,
+};
+
+static const struct of_device_id brcm_pmc_reset_of_match[] = {
+	{ .compatible = "brcm,bcm4908-pmc", .data = &brcm_pmc_4908_data, },
+	{ },
+};
+
+static int brcm_pmc_reset_probe(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	struct brcm_pmc *pmc;
+	struct resource *res;
+	unsigned int val;
+	int err;
+
+	pmc = devm_kzalloc(dev, sizeof(*pmc), GFP_KERNEL);
+	if (!pmc)
+		return -ENOMEM;
+
+	pmc->data = of_device_get_match_data(dev);
+	if (!pmc->data) {
+		dev_err(dev, "Failed to find chipset data\n");
+		return -EINVAL;
+	}
+
+	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	pmc->base = devm_ioremap_resource(dev, res);
+	if (IS_ERR(pmc->base))
+		return PTR_ERR(pmc->base);
+
+	pmc->misc = syscon_regmap_lookup_by_phandle(dev->of_node, "syscon-misc");
+	if (IS_ERR(pmc->misc))
+		return PTR_ERR(pmc->misc);
+
+	pmc->procmon = syscon_regmap_lookup_by_phandle(dev->of_node, "syscon-procmon");
+	if (IS_ERR(pmc->procmon))
+		return PTR_ERR(pmc->procmon);
+
+	pmc->dev = dev;
+
+	pmc->rcdev.ops = &brcm_pmc_reset_control_ops;
+	pmc->rcdev.owner = THIS_MODULE;
+	pmc->rcdev.of_node = dev->of_node;
+	pmc->rcdev.of_reset_n_cells = 2;
+	pmc->rcdev.of_xlate = brcm_pmc_reset_xlate;
+
+	err = regmap_read(pmc->misc, pmc->data->misc_strap_bus_reg, &val);
+	if (err)
+		return err;
+	pmc->direct = !(val & pmc->data->misc_strap_bus_pmc_rom_boot_bit);
+	if (!pmc->direct) {
+		dev_err(dev, "detected unsupported access mode, falling back to the direct one\n");
+		pmc->direct = 1;
+	}
+
+	pmc->little_endian = !of_device_is_big_endian(dev->of_node);
+
+	return devm_reset_controller_register(dev, &pmc->rcdev);
+}
+
+static struct platform_driver brcm_pmc_reset_driver = {
+	.probe = brcm_pmc_reset_probe,
+	.driver = {
+		.name	= "brcm-pmc",
+		.of_match_table	= brcm_pmc_reset_of_match,
+	}
+};
+module_platform_driver(brcm_pmc_reset_driver);
+
+MODULE_AUTHOR("Rafał Miłecki");
+MODULE_LICENSE("GPL v2");
+MODULE_DEVICE_TABLE(of, brcm_pmc_reset_of_match);
-- 
2.27.0

Re: [PATCH 2/2] reset: brcm-pmc: add driver for Broadcom's PMC

[Florian Fainelli]

On 11/9/2020 8:35 AM, Rafał Miłecki wrote:
> From: Rafał Miłecki <rafal@milecki.pl>
> 
> Power Management Controller can be found on BCM4908 and is needed to
> power on SoC blocks. It can be controlled using:
> 1. Firmware calls
> 2. Direct regs access
> 
> Only the later method is supported by this initial driver. It was
> successfully tested on BCM4908 using HCD controllers.
> 
> Signed-off-by: Rafał Miłecki <rafal@milecki.pl>
> ---

[snip]

> +#define PMBM_CTRL				0x00
> +#define  PMC_PMBM_START				(1 << 31)
> +#define  PMC_PMBM_TIMEOUT			(1 << 30)
> +#define  PMC_PMBM_SLAVE_ERR			(1 << 29)
> +#define  PMC_PMBM_BUSY				(1 << 28)
> +#define  PMC_PMBM_READ				(0 << 20)
> +#define  PMC_PMBM_WRITE				(1 << 20)
> +#define PMBM_WR_DATA				0x04
> +#define PMBM_TIMEOUT				0x08
> +#define PMBM_RD_DATA				0x0c

Can you re-use the definitions from include/linux/reset/bcm63xx_pmb.h,
you have the same constant names.

> +
> +#define BPCM_ID_REG				0x00
> +#define BPCM_CAPABILITIES			0x04
> +#define BPCM_CONTROL				0x08
> +#define BPCM_STATUS				0x0c
> +#define BPCM_ROSC_CONTROL			0x10
> +#define BPCM_ROSC_THRESH_H			0x14
> +#define BPCM_ROSC_THRESHOLD_BCM6838		0x14
> +#define BPCM_ROSC_THRESH_S			0x18
> +#define BPCM_ROSC_COUNT_BCM6838			0x18
> +#define BPCM_ROSC_COUNT				0x1c
> +#define BPCM_PWD_CONTROL_BCM6838		0x1c
> +#define BPCM_PWD_CONTROL			0x20
> +#define BPCM_SR_CONTROL_BCM6838			0x20
> +#define BPCM_PWD_ACCUM_CONTROL			0x24
> +#define BPCM_SR_CONTROL				0x28
> +#define BPCM_GLOBAL_CONTROL			0x2c
> +#define BPCM_MISC_CONTROL			0x30
> +#define BPCM_MISC_CONTROL2			0x34
> +#define BPCM_SGPHY_CNTL				0x38
> +#define BPCM_SGPHY_STATUS			0x3c
> +#define BPCM_ZONE0				0x40
> +#define  BPCM_ZONE_CONTROL			0x00
> +#define  BPCM_ZONE_CONFIG1			0x04
> +#define  BPCM_ZONE_CONFIG2			0x08
> +#define  BPCM_ZONE_FREQ_SCALAR_CONTROL		0x0c
> +#define  BPCM_ZONE_SIZE				0x10
> +
> +enum brcm_pmc_chipset {
> +	BCM4908,
> +	BCM6848,
> +	BCM6858,
> +};
> +
> +struct brcm_pmc_data {
> +	enum brcm_pmc_chipset chipset;
> +	int misc_strap_bus_reg;
> +	int misc_strap_bus_pmc_rom_boot_bit;
> +};
> +
> +static const struct brcm_pmc_data brcm_pmc_4908_data = {
> +	.chipset				= BCM4908,
> +	.misc_strap_bus_reg			= 0x00,
> +	.misc_strap_bus_pmc_rom_boot_bit	= BIT(11),
> +};
> +
> +union bpcm_capabilities {
> +	struct {
> +		int num_zones:8;
> +		int sr_reg_bits:8;
> +		int pllType:2;
> +		int reserved0:1;
> +		int ubus:1;
> +		int reserved1:12;
> +	} bits;
> +	u32 raw32;
> +} __attribute__((__packed__));

This is not going to work well if we extend this driver to support the
BCM63xx DSL SoCs which are done by the same engineering group within
Broadcom. Can you convert that to bits/masks instead such that this
becomes endian independent?

> +
> +union bpcm_zone_control {
> +	struct {
> +		u32 manual_clk_en:1;
> +		u32 manual_reset_ctl:1;
> +		u32 freq_scale_used:1;		/* R/O */
> +		u32 dpg_capable:1;		/* R/O */
> +		u32 manual_mem_pwr:2;
> +		u32 manual_iso_ctl:1;
> +		u32 manual_ctl:1;
> +		u32 dpg_ctl_en:1;
> +		u32 pwr_dn_req:1;
> +		u32 pwr_up_req:1;
> +		u32 mem_pwr_ctl_en:1;
> +		u32 blk_reset_assert:1;
> +		u32 mem_stby:1;
> +		u32 reserved:5;
> +		u32 pwr_cntl_state:5;
> +		u32 freq_scalar_dyn_sel:1;	/* R/O */
> +		u32 pwr_off_state:1;		/* R/O */
> +		u32 pwr_on_state:1;		/* R/O */
> +		u32 pwr_good:1;			/* R/O */
> +		u32 dpg_pwr_state:1;		/* R/O */
> +		u32 mem_pwr_state:1;		/* R/O */
> +		u32 iso_state:1;		/* R/O */
> +		u32 reset_state:1;		/* R/O */
> +	} bits;
> +	u32 raw32;
> +} __attribute__((__packed__));
> +
> +struct brcm_pmc {
> +	const struct brcm_pmc_data *data;
> +	void __iomem *base;
> +	struct regmap *misc;
> +	struct regmap *procmon;
> +	struct device *dev;
> +	struct reset_controller_dev rcdev;
> +	bool direct;
> +	bool little_endian;
> +};
> +
> +static DEFINE_SPINLOCK(lock);

You have a platform device, can you store the lock there instead of
having a global?

> +
> +#define cpu_to_dev32(pmc, val) \
> +	((pmc)->little_endian ? cpu_to_le32(val) : cpu_to_be32(val))

This would probably be better written as a static inline helper, and
maybe something like this:

static inline u32 brcm_pmc_readl(struct brcm_pmc *pmc, u8 offset)
{
	if (pmc->little_endian)
		return ioread32(pmc->base + offset);
	else
		return ioread32_be(pmc->base + offset);
}

> +
> +#define dev32_to_cpu(pmc, val) \
> +	((pmc)->little_endian ? le32_to_cpu(val) : be32_to_cpu(val))
> +
> +static int brcm_pmc_bpcm_wait(struct brcm_pmc *pmc, int bus)
> +{
> +	unsigned long deadline = jiffies + usecs_to_jiffies(1000);
> +	int pmbm = PROCMON_PMBM + bus * PROCMON_PMBM_SIZE;
> +	u32 val;
> +
> +	do {
> +		regmap_read(pmc->procmon, pmbm + PMBM_CTRL, &val);
> +		if (!(val & PMC_PMBM_START))
> +			return 0;
> +		cpu_relax();
> +		udelay(10);
> +	} while (!time_after_eq(jiffies, deadline));
> +
> +	dev_err(pmc->dev, "Timeout waiting for the BPCM\n");

Can you use include/linux/reset/bcm63xx_pmb.h again here and check
arch/arm/mach-bcm/bcm63xx_pmb.c for how it is used.

> +
> +	return -EBUSY;
> +}
> +
> +static int brcm_pmc_bpcm_read(struct brcm_pmc *pmc, u16 addr, int word_offset, u32 *val)
> +{
> +	int err = 0;
> +
> +	if (!pmc->direct) {
> +		err = -EOPNOTSUPP;

Do an early return here so you don't have to indent the block under by
one tab.

> +	} else {
> +		u8 device = addr & 0xff;
> +		u8 bus = addr >> 8;
> +		unsigned long flags;
> +		int pmbm;
> +		u32 tmp;
> +
> +		pmbm = PROCMON_PMBM + bus * PROCMON_PMBM_SIZE;
> +
> +		spin_lock_irqsave(&lock, flags);
> +
> +		/* TODO: Make sure PMBM is not busy */
> +
> +		regmap_write(pmc->procmon, pmbm + PMBM_CTRL,
> +			     PMC_PMBM_START | PMC_PMBM_READ | (device << 12) | word_offset);
> +
> +		err = brcm_pmc_bpcm_wait(pmc, bus);
> +		if (!err)
> +			err = regmap_read(pmc->procmon, pmbm + PMBM_RD_DATA, val);
> +
> +		spin_unlock_irqrestore(&lock, flags);
> +	}
> +
> +	return err;
> +}
> +
> +static int brcm_pmc_bpcm_write(struct brcm_pmc *pmc, u16 addr, int word_offset, u32 val)
> +{
> +	int err = 0;
> +
> +	if (!pmc->direct) {
> +		err = -EOPNOTSUPP;

Likewise.

[snip]

> +static int brcm_pmc_reset_xlate(struct reset_controller_dev *rcdev,
> +				const struct of_phandle_args *reset_spec)
> +{
> +	u8 device = reset_spec->args[1];
> +	u8 bus = reset_spec->args[0];
> +
> +	if (bus > 1)
> +		return -EINVAL;
> +
> +	return (bus << 8) | device;

IIRC the bus is limited to 8 bits, and the device ID certainly is
limited to 8 bits so you need to validate that before doing the
assignment otherwise you are going to see truncated values which could
be wrapped around and return some bogus reset ID combination.

You are using the same binding as
Documentation/devicetree/bindings/reset/brcm,bcm63138-pmb.txt, so maybe
you can add that compatible string into your binding document and
deprecate this old binding file.

Other than those, this looks great to me, thanks!
-- 
Florian

Re: [PATCH 2/2] reset: brcm-pmc: add driver for Broadcom's PMC

[Rafał Miłecki]

On 10.11.2020 05:52, Florian Fainelli wrote:
> On 11/9/2020 8:35 AM, Rafał Miłecki wrote:
>> From: Rafał Miłecki <rafal@milecki.pl>
>>
>> Power Management Controller can be found on BCM4908 and is needed to
>> power on SoC blocks. It can be controlled using:
>> 1. Firmware calls
>> 2. Direct regs access
>>
>> Only the later method is supported by this initial driver. It was
>> successfully tested on BCM4908 using HCD controllers.
>>
>> Signed-off-by: Rafał Miłecki <rafal@milecki.pl>
>> ---
> 
> [snip]
> 
>> +#define PMBM_CTRL				0x00
>> +#define  PMC_PMBM_START				(1 << 31)
>> +#define  PMC_PMBM_TIMEOUT			(1 << 30)
>> +#define  PMC_PMBM_SLAVE_ERR			(1 << 29)
>> +#define  PMC_PMBM_BUSY				(1 << 28)
>> +#define  PMC_PMBM_READ				(0 << 20)
>> +#define  PMC_PMBM_WRITE				(1 << 20)
>> +#define PMBM_WR_DATA				0x04
>> +#define PMBM_TIMEOUT				0x08
>> +#define PMBM_RD_DATA				0x0c
> 
> Can you re-use the definitions from include/linux/reset/bcm63xx_pmb.h,
> you have the same constant names.
> 
>> +
>> +#define BPCM_ID_REG				0x00
>> +#define BPCM_CAPABILITIES			0x04
>> +#define BPCM_CONTROL				0x08
>> +#define BPCM_STATUS				0x0c
>> +#define BPCM_ROSC_CONTROL			0x10
>> +#define BPCM_ROSC_THRESH_H			0x14
>> +#define BPCM_ROSC_THRESHOLD_BCM6838		0x14
>> +#define BPCM_ROSC_THRESH_S			0x18
>> +#define BPCM_ROSC_COUNT_BCM6838			0x18
>> +#define BPCM_ROSC_COUNT				0x1c
>> +#define BPCM_PWD_CONTROL_BCM6838		0x1c
>> +#define BPCM_PWD_CONTROL			0x20
>> +#define BPCM_SR_CONTROL_BCM6838			0x20
>> +#define BPCM_PWD_ACCUM_CONTROL			0x24
>> +#define BPCM_SR_CONTROL				0x28
>> +#define BPCM_GLOBAL_CONTROL			0x2c
>> +#define BPCM_MISC_CONTROL			0x30
>> +#define BPCM_MISC_CONTROL2			0x34
>> +#define BPCM_SGPHY_CNTL				0x38
>> +#define BPCM_SGPHY_STATUS			0x3c
>> +#define BPCM_ZONE0				0x40
>> +#define  BPCM_ZONE_CONTROL			0x00
>> +#define  BPCM_ZONE_CONFIG1			0x04
>> +#define  BPCM_ZONE_CONFIG2			0x08
>> +#define  BPCM_ZONE_FREQ_SCALAR_CONTROL		0x0c
>> +#define  BPCM_ZONE_SIZE				0x10
>> +
>> +enum brcm_pmc_chipset {
>> +	BCM4908,
>> +	BCM6848,
>> +	BCM6858,
>> +};
>> +
>> +struct brcm_pmc_data {
>> +	enum brcm_pmc_chipset chipset;
>> +	int misc_strap_bus_reg;
>> +	int misc_strap_bus_pmc_rom_boot_bit;
>> +};
>> +
>> +static const struct brcm_pmc_data brcm_pmc_4908_data = {
>> +	.chipset				= BCM4908,
>> +	.misc_strap_bus_reg			= 0x00,
>> +	.misc_strap_bus_pmc_rom_boot_bit	= BIT(11),
>> +};
>> +
>> +union bpcm_capabilities {
>> +	struct {
>> +		int num_zones:8;
>> +		int sr_reg_bits:8;
>> +		int pllType:2;
>> +		int reserved0:1;
>> +		int ubus:1;
>> +		int reserved1:12;
>> +	} bits;
>> +	u32 raw32;
>> +} __attribute__((__packed__));
> 
> This is not going to work well if we extend this driver to support the
> BCM63xx DSL SoCs which are done by the same engineering group within
> Broadcom. Can you convert that to bits/masks instead such that this
> becomes endian independent?
> 
>> +
>> +union bpcm_zone_control {
>> +	struct {
>> +		u32 manual_clk_en:1;
>> +		u32 manual_reset_ctl:1;
>> +		u32 freq_scale_used:1;		/* R/O */
>> +		u32 dpg_capable:1;		/* R/O */
>> +		u32 manual_mem_pwr:2;
>> +		u32 manual_iso_ctl:1;
>> +		u32 manual_ctl:1;
>> +		u32 dpg_ctl_en:1;
>> +		u32 pwr_dn_req:1;
>> +		u32 pwr_up_req:1;
>> +		u32 mem_pwr_ctl_en:1;
>> +		u32 blk_reset_assert:1;
>> +		u32 mem_stby:1;
>> +		u32 reserved:5;
>> +		u32 pwr_cntl_state:5;
>> +		u32 freq_scalar_dyn_sel:1;	/* R/O */
>> +		u32 pwr_off_state:1;		/* R/O */
>> +		u32 pwr_on_state:1;		/* R/O */
>> +		u32 pwr_good:1;			/* R/O */
>> +		u32 dpg_pwr_state:1;		/* R/O */
>> +		u32 mem_pwr_state:1;		/* R/O */
>> +		u32 iso_state:1;		/* R/O */
>> +		u32 reset_state:1;		/* R/O */
>> +	} bits;
>> +	u32 raw32;
>> +} __attribute__((__packed__));
>> +
>> +struct brcm_pmc {
>> +	const struct brcm_pmc_data *data;
>> +	void __iomem *base;
>> +	struct regmap *misc;
>> +	struct regmap *procmon;
>> +	struct device *dev;
>> +	struct reset_controller_dev rcdev;
>> +	bool direct;
>> +	bool little_endian;
>> +};
>> +
>> +static DEFINE_SPINLOCK(lock);
> 
> You have a platform device, can you store the lock there instead of
> having a global?
> 
>> +
>> +#define cpu_to_dev32(pmc, val) \
>> +	((pmc)->little_endian ? cpu_to_le32(val) : cpu_to_be32(val))
> 
> This would probably be better written as a static inline helper, and
> maybe something like this:
> 
> static inline u32 brcm_pmc_readl(struct brcm_pmc *pmc, u8 offset)
> {
> 	if (pmc->little_endian)
> 		return ioread32(pmc->base + offset);
> 	else
> 		return ioread32_be(pmc->base + offset);
> }
> 
>> +
>> +#define dev32_to_cpu(pmc, val) \
>> +	((pmc)->little_endian ? le32_to_cpu(val) : be32_to_cpu(val))
>> +
>> +static int brcm_pmc_bpcm_wait(struct brcm_pmc *pmc, int bus)
>> +{
>> +	unsigned long deadline = jiffies + usecs_to_jiffies(1000);
>> +	int pmbm = PROCMON_PMBM + bus * PROCMON_PMBM_SIZE;
>> +	u32 val;
>> +
>> +	do {
>> +		regmap_read(pmc->procmon, pmbm + PMBM_CTRL, &val);
>> +		if (!(val & PMC_PMBM_START))
>> +			return 0;
>> +		cpu_relax();
>> +		udelay(10);
>> +	} while (!time_after_eq(jiffies, deadline));
>> +
>> +	dev_err(pmc->dev, "Timeout waiting for the BPCM\n");
> 
> Can you use include/linux/reset/bcm63xx_pmb.h again here and check
> arch/arm/mach-bcm/bcm63xx_pmb.c for how it is used.

I use regmap while bcm63xx_pmb.h depends on raw IO access. See below though.


>> +	return -EBUSY;
>> +}
>> +
>> +static int brcm_pmc_bpcm_read(struct brcm_pmc *pmc, u16 addr, int word_offset, u32 *val)
>> +{
>> +	int err = 0;
>> +
>> +	if (!pmc->direct) {
>> +		err = -EOPNOTSUPP;
> 
> Do an early return here so you don't have to indent the block under by
> one tab.

In a long term I'll need a full if + else anyway. Having it this way now
will save me some code moving (indent) later.


>> +	} else {
>> +		u8 device = addr & 0xff;
>> +		u8 bus = addr >> 8;
>> +		unsigned long flags;
>> +		int pmbm;
>> +		u32 tmp;
>> +
>> +		pmbm = PROCMON_PMBM + bus * PROCMON_PMBM_SIZE;
>> +
>> +		spin_lock_irqsave(&lock, flags);
>> +
>> +		/* TODO: Make sure PMBM is not busy */
>> +
>> +		regmap_write(pmc->procmon, pmbm + PMBM_CTRL,
>> +			     PMC_PMBM_START | PMC_PMBM_READ | (device << 12) | word_offset);
>> +
>> +		err = brcm_pmc_bpcm_wait(pmc, bus);
>> +		if (!err)
>> +			err = regmap_read(pmc->procmon, pmbm + PMBM_RD_DATA, val);
>> +
>> +		spin_unlock_irqrestore(&lock, flags);
>> +	}
>> +
>> +	return err;
>> +}
>> +
>> +static int brcm_pmc_bpcm_write(struct brcm_pmc *pmc, u16 addr, int word_offset, u32 val)
>> +{
>> +	int err = 0;
>> +
>> +	if (!pmc->direct) {
>> +		err = -EOPNOTSUPP;
> 
> Likewise.
> 
> [snip]
> 
>> +static int brcm_pmc_reset_xlate(struct reset_controller_dev *rcdev,
>> +				const struct of_phandle_args *reset_spec)
>> +{
>> +	u8 device = reset_spec->args[1];
>> +	u8 bus = reset_spec->args[0];
>> +
>> +	if (bus > 1)
>> +		return -EINVAL;
>> +
>> +	return (bus << 8) | device;
> 
> IIRC the bus is limited to 8 bits, and the device ID certainly is
> limited to 8 bits so you need to validate that before doing the
> assignment otherwise you are going to see truncated values which could
> be wrapped around and return some bogus reset ID combination.
> 
> You are using the same binding as
> Documentation/devicetree/bindings/reset/brcm,bcm63138-pmb.txt, so maybe
> you can add that compatible string into your binding document and
> deprecate this old binding file.

Looking at arch/arm/mach-bcm/bcm63xx_pmb.c made me realize this design may be
actually more complex than that.


*** PMB ***

It seems like PMB Master is reset controller on its own. Even though we don't
have actual driver for the (documented) "brcm,bcm63138-pmb" binding, arch code
treates it like a reset.

So it seems that a single PMB is capable at least of:
1. Resetting ARM CPU core (id by 8b addr)
2. Resetting devices (id by 8b addr) by (en|dis)abling zones

Above operations are performed in a different (programming) ways.


*** PMC ***

PMC seems to be a reset controller with firmware capable at least of:
1. Resetting devices (id by 8b addr) by (en|dis)abling zones

that fallback to using PMB blocks when needed / required.


*** Bindings ***

So I started wondering if following bindings were not more applicable:

pmb0: reset-controller@4800c0 {
	compatible = "brcm,bcm4908-pmb";
	reg = <0x802800c0 0x10>;
	#reset-cells = <2>;
};

pmb1: reset-controller@4800e0 {
	compatible = "brcm,bcm4908-pmb";
	reg = <0x802800e0 0x10>;
	#reset-cells = <2>;
};

pmc: reset-controller@80200000 {
	compatible = "brcm,bcm4908-pmc";
	reg = <0x80200000 0x5000>;
	brcm,syscon-misc = <&misc>;
	pmb-resets = <BUS_ID_0 &pmb0>, <BUS_ID_1 &pmb1>;
	#reset-cells = <2>;
};

cpu@1 {
	resets = <&pmb0 PMB_ARM 4>; /* ARM CPU */
}

foo {
	resets = <&pmb1 PMB_DEVICE 17>; /* Reset USB using PMB directly */
}

foo {
	resets = <&pmc BUS_ID_1 17>; /* Reset USB using PMC */
}


Then we would have:

1. PMB driver handling PMB_ARM and PMB_DEVICE with 2 different code paths
2. PMC driver falling back to PMB when needed / required



Florian: can you have an extra look at it please and review proposed design,
comment on it?

Re: [PATCH 2/2] reset: brcm-pmc: add driver for Broadcom's PMC

[Rafał Miłecki]

On 12.11.2020 15:55, Rafał Miłecki wrote:
> *** PMB ***
> 
> It seems like PMB Master is reset controller on its own. Even though we don't
> have actual driver for the (documented) "brcm,bcm63138-pmb" binding, arch code
> treates it like a reset.
> 
> So it seems that a single PMB is capable at least of:
> 1. Resetting ARM CPU core (id by 8b addr)
> 2. Resetting devices (id by 8b addr) by (en|dis)abling zones
> 
> Above operations are performed in a different (programming) ways.
> 
> 
> *** PMC ***
> 
> PMC seems to be a reset controller with firmware capable at least of:
> 1. Resetting devices (id by 8b addr) by (en|dis)abling zones
> 
> that fallback to using PMB blocks when needed / required.

This just got more complex as I started playing with PMC / PMB and PCIe
controller.

1. BPCM_CAPABILITIES reg for PCIe controller reports 0 num_zones
2. Enabling PCIe requires powering on zone 0 manually
3. After powering on zone, PCIe requires setting SR_CONTROL

It means that PMB driver (and so PMC one as it fallback to the PMB) needs to
know what type of device we're addressing.

Anything simple like:

resets = <&pmb0 14>;
resets = <&pmc 1 14>;

won't work (unless we hardcode ids in driver - which is unwanted for DTS).


So I guess that after all we'll need something like:

cpu@1 {
     resets = <&pmb0 PMB_ARM 4>; /* ARM CPU */
}

foo {
     resets = <&pmb1 PMB_USB_DEVICE 17>; /* Reset USB using PMB directly */
}

bar {
     resets = <&pmb1 PMB_PCIE_DEVICE 15>; /* Reset PCIE using PMB directly */
}

and also

qux {
     resets = <&pmc PMB_PCIE_DEVICE 1 15>; /* Reset PCIe using PMC */
}

Re: [PATCH 2/2] reset: brcm-pmc: add driver for Broadcom's PMC

[Florian Fainelli]

On 11/17/20 10:44 AM, Rafał Miłecki wrote:
> On 12.11.2020 15:55, Rafał Miłecki wrote:
>> *** PMB ***
>>
>> It seems like PMB Master is reset controller on its own. Even though
>> we don't
>> have actual driver for the (documented) "brcm,bcm63138-pmb" binding,
>> arch code
>> treates it like a reset.
>>
>> So it seems that a single PMB is capable at least of:
>> 1. Resetting ARM CPU core (id by 8b addr)
>> 2. Resetting devices (id by 8b addr) by (en|dis)abling zones
>>
>> Above operations are performed in a different (programming) ways.
>>
>>
>> *** PMC ***
>>
>> PMC seems to be a reset controller with firmware capable at least of:
>> 1. Resetting devices (id by 8b addr) by (en|dis)abling zones
>>
>> that fallback to using PMB blocks when needed / required.
> 
> This just got more complex as I started playing with PMC / PMB and PCIe
> controller.
> 
> 1. BPCM_CAPABILITIES reg for PCIe controller reports 0 num_zones
> 2. Enabling PCIe requires powering on zone 0 manually
> 3. After powering on zone, PCIe requires setting SR_CONTROL
> 
> It means that PMB driver (and so PMC one as it fallback to the PMB)
> needs to
> know what type of device we're addressing.

From prior attempts at getting the 63138 supported which was a fairly
early silicon revision with a mix of PMB and PMC ultimately, the reset
driver does need to know what kind of device ID is being reset/powered
on. I stopped after seeing that SATA, USB, PCIe, Switch would all need
to be treated differently, and the board I had should have been upgraded
to a B0:

https://github.com/ffainelli/linux/commit/50f1dfb17b624ee41a11dda01bc899e6756869b7

There are way too many quirks for Device Tree to express all of them,
and the whole point of the BPCM was to design a re-usable and
self-discoverable  power/reset/clocking module. As you found out, if the
capabilities are wrong, it defeats the purpose.

> 
> Anything simple like:
> 
> resets = <&pmb0 14>;
> resets = <&pmc 1 14>;
> 
> won't work (unless we hardcode ids in driver - which is unwanted for DTS).
> 
> 
> So I guess that after all we'll need something like:
> 
> cpu@1 {
>     resets = <&pmb0 PMB_ARM 4>; /* ARM CPU */
> }
> 
> foo {
>     resets = <&pmb1 PMB_USB_DEVICE 17>; /* Reset USB using PMB directly */
> }
> 
> bar {
>     resets = <&pmb1 PMB_PCIE_DEVICE 15>; /* Reset PCIE using PMB
> directly */
> }

What is the second reset cell mapping to? Maybe you can define a C
preprocessor macro that expressed both the PMB bus + device ID?

> 
> and also
> 
> qux {
>     resets = <&pmc PMB_PCIE_DEVICE 1 15>; /* Reset PCIe using PMC */
> }


-- 
Florian

Re: [PATCH 2/2] reset: brcm-pmc: add driver for Broadcom's PMC

[Rafał Miłecki]

On 17.11.2020 19:55, Florian Fainelli wrote:
> On 11/17/20 10:44 AM, Rafał Miłecki wrote:
>> On 12.11.2020 15:55, Rafał Miłecki wrote:
>>> *** PMB ***
>>>
>>> It seems like PMB Master is reset controller on its own. Even though
>>> we don't
>>> have actual driver for the (documented) "brcm,bcm63138-pmb" binding,
>>> arch code
>>> treates it like a reset.
>>>
>>> So it seems that a single PMB is capable at least of:
>>> 1. Resetting ARM CPU core (id by 8b addr)
>>> 2. Resetting devices (id by 8b addr) by (en|dis)abling zones
>>>
>>> Above operations are performed in a different (programming) ways.
>>>
>>>
>>> *** PMC ***
>>>
>>> PMC seems to be a reset controller with firmware capable at least of:
>>> 1. Resetting devices (id by 8b addr) by (en|dis)abling zones
>>>
>>> that fallback to using PMB blocks when needed / required.
>>
>> This just got more complex as I started playing with PMC / PMB and PCIe
>> controller.
>>
>> 1. BPCM_CAPABILITIES reg for PCIe controller reports 0 num_zones
>> 2. Enabling PCIe requires powering on zone 0 manually
>> 3. After powering on zone, PCIe requires setting SR_CONTROL
>>
>> It means that PMB driver (and so PMC one as it fallback to the PMB)
>> needs to
>> know what type of device we're addressing.
> 
>  From prior attempts at getting the 63138 supported which was a fairly
> early silicon revision with a mix of PMB and PMC ultimately, the reset
> driver does need to know what kind of device ID is being reset/powered
> on. I stopped after seeing that SATA, USB, PCIe, Switch would all need
> to be treated differently, and the board I had should have been upgraded
> to a B0:
> 
> https://github.com/ffainelli/linux/commit/50f1dfb17b624ee41a11dda01bc899e6756869b7
> 
> There are way too many quirks for Device Tree to express all of them,
> and the whole point of the BPCM was to design a re-usable and
> self-discoverable  power/reset/clocking module. As you found out, if the
> capabilities are wrong, it defeats the purpose.

Good to know we share conclusions. The problem with PMC (except having to
support firmware / ucode / you call it) is it's still not capable of
handling everything.

As mentioned earlier, powering up PCIe controller can be done by PMC and
powering up zone 0 BUT it still requires setting that SR_CTRL.

I see you ended up hardcoding IDs in the bcm63138-pmb.c . I still hope I
can get something slightly more generic.


>> Anything simple like:
>>
>> resets = <&pmb0 14>;
>> resets = <&pmc 1 14>;
>>
>> won't work (unless we hardcode ids in driver - which is unwanted for DTS).
>>
>>
>> So I guess that after all we'll need something like:
>>
>> cpu@1 {
>>      resets = <&pmb0 PMB_ARM 4>; /* ARM CPU */
>> }
>>
>> foo {
>>      resets = <&pmb1 PMB_USB_DEVICE 17>; /* Reset USB using PMB directly */
>> }
>>
>> bar {
>>      resets = <&pmb1 PMB_PCIE_DEVICE 15>; /* Reset PCIE using PMB
>> directly */
>> }
> 
> What is the second reset cell mapping to? Maybe you can define a C
> preprocessor macro that expressed both the PMB bus + device ID?

Device ID. Just like 4 in the :
#define PMB_ADDR_AIP		(4 | PMB_BUS_AIP << PMB_BUS_ID_SHIFT)

So for BCM4908 I could have:
resets = <&pmb1 PMB_PCIE_DEVICE 14>; /* PCIe 0 */
resets = <&pmb1 PMB_PCIE_DEVICE 15>; /* PCIe 1 */

Do you mean C macro for combining two values? Sure, I can do that, but is it
worth it, just to save n * 32 b of .dtb data?


>> and also
>>
>> qux {
>>      resets = <&pmc PMB_PCIE_DEVICE 1 15>; /* Reset PCIe using PMC */
>> }
> 
> 

Re: [PATCH 1/2] dt-bindings: reset: document Broadcom's PMC binding

[Florian Fainelli]

On 11/9/2020 8:35 AM, Rafał Miłecki wrote:
> From: Rafał Miłecki <rafal@milecki.pl>
> 
> Broadcom's PMC is Power Management Controller that is used for disabling
> and enabling SoC devices.
> 
> Signed-off-by: Rafał Miłecki <rafal@milecki.pl>
> ---

[snip]

> +
> +  big-endian:
> +    $ref: /schemas/types.yaml#/definitions/flag
> +    description:
> +      Flag to use for block working in big endian mode.
> +
> +  "#reset-cells":
> +    const: 2

You would need to describe what these two cells are supposed to be.
-- 
Florian

Re: [PATCH 1/2] dt-bindings: reset: document Broadcom's PMC binding

[Rob Herring]

On Mon, Nov 09, 2020 at 05:35:18PM +0100, Rafał Miłecki wrote:
> From: Rafał Miłecki <rafal@milecki.pl>
> 
> Broadcom's PMC is Power Management Controller that is used for disabling
> and enabling SoC devices.
> 
> Signed-off-by: Rafał Miłecki <rafal@milecki.pl>
> ---
>  .../bindings/reset/brcm,bcm-pmc.yaml          | 69 +++++++++++++++++++
>  1 file changed, 69 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/reset/brcm,bcm-pmc.yaml
> 
> diff --git a/Documentation/devicetree/bindings/reset/brcm,bcm-pmc.yaml b/Documentation/devicetree/bindings/reset/brcm,bcm-pmc.yaml
> new file mode 100644
> index 000000000000..2afc2048997f
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/reset/brcm,bcm-pmc.yaml
> @@ -0,0 +1,69 @@
> +# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/reset/brcm,bcm-pmc.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Broadcom PMC (Power Management Controller) reset controller
> +
> +description: This document describes the Broadcom's PMC (Power Management
> +  Controller). It supports resetting devices identified by the bus id and device
> +  address.
> +
> +maintainers:
> +  - Rafał Miłecki <rafal@milecki.pl>
> +
> +properties:
> +  compatible:
> +    enum:
> +      - brcm,bcm4908-pmc # PMC on BCM4908 and compatible SoCs
> +
> +  reg:
> +    maxItems: 1
> +
> +  syscon-misc:

Needs a vendor prefix

> +    $ref: /schemas/types.yaml#/definitions/phandle
> +    description:
> +      Phandle to the MISC system controller node.
> +
> +  syscon-procmon:

Needs a vendor prefix

> +    $ref: /schemas/types.yaml#/definitions/phandle
> +    description:
> +      Phandle to the PROC MON system controller node.
> +
> +  big-endian:
> +    $ref: /schemas/types.yaml#/definitions/flag
> +    description:
> +      Flag to use for block working in big endian mode.
> +
> +  "#reset-cells":
> +    const: 2
> +
> +required:
> +  - compatible
> +  - reg
> +  - syscon-misc
> +  - syscon-procmon
> +  - "#reset-cells"
> +
> +additionalProperties: false
> +
> +examples:
> +  - |
> +    pmc: reset-controller@80200000 {
> +        compatible = "brcm,bcm4908-pmc";
> +        reg = <0x80200000 0x5000>;
> +        syscon-misc = <&misc>;
> +        syscon-procmon = <&procmon>;
> +        #reset-cells = <2>;
> +    };
> +
> +    procmon: syscon@80280000 {
> +        compatible = "brcm,misc", "syscon";

IIRC, not a documented compatible. Nor is it specific enough.

You can just drop these from the example.

> +        reg = <0x80280000 0x1000>;
> +    };
> +
> +    misc: syscon@ff802600 {
> +        compatible = "brcm,misc", "syscon";
> +        reg = <0xff802600 0xe4>;
> +    };
> -- 
> 2.27.0
>