[PATCH 0/2] Add iProc IDM device support

[PATCH 0/2] Add iProc IDM device support

[Ray Jui]

The Broadcom iProc IDM device allows control and monitoring of ASIC internal
bus transactions. Most importantly, it can be configured to detect bus
transaction timeout. In such case, critical information such as transaction
address that caused the error, bus master ID of the transaction that caused
the error, and etc., are made available from the IDM device.

This patch series adds the binding document and driver support for the
iProc IDM devices.

The patch series is based off v5.4 and was tested on Broadcom
Stingray combo SVK board. The patch series is available at:
Repo: https://github.com/Broadcom/arm64-linux.git
Branch: iproc-idm-v1

Ray Jui (2):
  dt-bindings: soc: Add binding doc for iProc IDM device
  soc: bcm: iproc: Add Broadcom iProc IDM driver

 .../bindings/soc/bcm/brcm,iproc-idm.txt       |  44 ++
 drivers/soc/bcm/Kconfig                       |  10 +
 drivers/soc/bcm/Makefile                      |   1 +
 drivers/soc/bcm/iproc/Kconfig                 |   6 +
 drivers/soc/bcm/iproc/Makefile                |   1 +
 drivers/soc/bcm/iproc/iproc-idm.c             | 390 ++++++++++++++++++
 6 files changed, 452 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/soc/bcm/brcm,iproc-idm.txt
 create mode 100644 drivers/soc/bcm/iproc/Kconfig
 create mode 100644 drivers/soc/bcm/iproc/Makefile
 create mode 100644 drivers/soc/bcm/iproc/iproc-idm.c

-- 
2.17.1

[PATCH 1/2] dt-bindings: soc: Add binding doc for iProc IDM device

[Ray Jui]

Add binding document for iProc based IDM devices.

Signed-off-by: Ray Jui <ray.jui@broadcom.com>
---
 .../bindings/soc/bcm/brcm,iproc-idm.txt       | 44 +++++++++++++++++++
 1 file changed, 44 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/soc/bcm/brcm,iproc-idm.txt

diff --git a/Documentation/devicetree/bindings/soc/bcm/brcm,iproc-idm.txt b/Documentation/devicetree/bindings/soc/bcm/brcm,iproc-idm.txt
new file mode 100644
index 000000000000..388c6b036d7e
--- /dev/null
+++ b/Documentation/devicetree/bindings/soc/bcm/brcm,iproc-idm.txt
@@ -0,0 +1,44 @@
+Broadcom iProc Interconnect Device Management (IDM) device
+
+The Broadcom iProc IDM device allows control and monitoring of ASIC internal
+bus transactions. Most importantly, it can be configured to detect bus
+transaction timeout. In such case, critical information such as transaction
+address that caused the error, bus master ID of the transaction that caused
+the error, and etc., are made available from the IDM device.
+
+-------------------------------------------------------------------------------
+
+Required properties for IDM device node:
+- compatible: must be "brcm,iproc-idm"
+- reg: base address and length of the IDM register space
+- interrupt: IDM interrupt number
+- brcm,iproc-idm-bus: IDM bus string
+
+Optional properties for IDM device node:
+- brcm,iproc-idm-elog: phandle to the device node of the IDM logging device
+
+-------------------------------------------------------------------------------
+
+Required properties for IDM error logging device node:
+- compatible: must be "brcm,iproc-idm-elog";
+- reg: base address and length of reserved memory location where IDM error
+  events can be saved
+
+-------------------------------------------------------------------------------
+
+Example:
+
+idm {
+	idm-elog {
+		compatible = "brcm,iproc-idm-elog";
+		reg = <0x8f221000 0x1000>;
+	};
+
+	idm-mhb-paxc-axi {
+		compatible = "brcm,iproc-idm";
+		reg = <0x60406900 0x200>;
+		interrupt = <GIC_SPI 516 IRQ_TYPE_LEVEL_HIGH>;
+		brcm,iproc-idm-bus = "idm-mhb-paxc-axi";
+		brcm,iproc-idm-elog = <&idm-elog>;
+	};
+};
-- 
2.17.1

[PATCH 2/2] soc: bcm: iproc: Add Broadcom iProc IDM driver

[Ray Jui]

Add Broadcom iProc IDM driver that controls that IDM devices available
on various iProc based SoCs for bus transaction timeout monitoring and
error logging.

Signed-off-by: Ray Jui <ray.jui@broadcom.com>
Signed-off-by: Rayagonda Kokatanur <rayagonda.kokatanur@broadcom.com>
---
 drivers/soc/bcm/Kconfig           |  10 +
 drivers/soc/bcm/Makefile          |   1 +
 drivers/soc/bcm/iproc/Kconfig     |   6 +
 drivers/soc/bcm/iproc/Makefile    |   1 +
 drivers/soc/bcm/iproc/iproc-idm.c | 390 ++++++++++++++++++++++++++++++
 5 files changed, 408 insertions(+)
 create mode 100644 drivers/soc/bcm/iproc/Kconfig
 create mode 100644 drivers/soc/bcm/iproc/Makefile
 create mode 100644 drivers/soc/bcm/iproc/iproc-idm.c

diff --git a/drivers/soc/bcm/Kconfig b/drivers/soc/bcm/Kconfig
index 648e32693b7e..30cf0c390c4e 100644
--- a/drivers/soc/bcm/Kconfig
+++ b/drivers/soc/bcm/Kconfig
@@ -33,6 +33,16 @@ config SOC_BRCMSTB
 
 	  If unsure, say N.
 
+config SOC_BRCM_IPROC
+	bool "Broadcom iProc SoC drivers"
+	depends on ARCH_BCM_IPROC || COMPILE_TEST
+	default ARCH_BCM_IPROC
+	help
+	  Enable SoC drivers for Broadcom iProc based chipsets
+
+	  If unsure, say N.
+
 source "drivers/soc/bcm/brcmstb/Kconfig"
+source "drivers/soc/bcm/iproc/Kconfig"
 
 endmenu
diff --git a/drivers/soc/bcm/Makefile b/drivers/soc/bcm/Makefile
index d92268a829a9..9db23ab5dacc 100644
--- a/drivers/soc/bcm/Makefile
+++ b/drivers/soc/bcm/Makefile
@@ -2,3 +2,4 @@
 obj-$(CONFIG_BCM2835_POWER)	+= bcm2835-power.o
 obj-$(CONFIG_RASPBERRYPI_POWER)	+= raspberrypi-power.o
 obj-$(CONFIG_SOC_BRCMSTB)	+= brcmstb/
+obj-$(CONFIG_SOC_BRCM_IPROC)	+= iproc/
diff --git a/drivers/soc/bcm/iproc/Kconfig b/drivers/soc/bcm/iproc/Kconfig
new file mode 100644
index 000000000000..205e0ebbf99c
--- /dev/null
+++ b/drivers/soc/bcm/iproc/Kconfig
@@ -0,0 +1,6 @@
+config IPROC_IDM
+	bool "Broadcom iProc IDM driver"
+	depends on (ARCH_BCM_IPROC || COMPILE_TEST) && OF
+	default ARCH_BCM_IPROC
+	help
+	  Enables support for iProc Interconnect and Device Management (IDM) control and monitoring
diff --git a/drivers/soc/bcm/iproc/Makefile b/drivers/soc/bcm/iproc/Makefile
new file mode 100644
index 000000000000..de54aef66097
--- /dev/null
+++ b/drivers/soc/bcm/iproc/Makefile
@@ -0,0 +1 @@
+obj-$(CONFIG_IPROC_IDM)	+= iproc-idm.o
diff --git a/drivers/soc/bcm/iproc/iproc-idm.c b/drivers/soc/bcm/iproc/iproc-idm.c
new file mode 100644
index 000000000000..5f3b04dbe80a
--- /dev/null
+++ b/drivers/soc/bcm/iproc/iproc-idm.c
@@ -0,0 +1,390 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Copyright (C) 2019 Broadcom
+ */
+
+#include <linux/interrupt.h>
+#include <linux/io.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/of_address.h>
+#include <linux/of_irq.h>
+#include <linux/of_platform.h>
+#include <linux/platform_device.h>
+#include <linux/slab.h>
+#include <linux/types.h>
+
+#define IDM_CTRL_OFFSET              0x000
+#define IDM_CTRL_TIMEOUT_ENABLE      BIT(9)
+#define IDM_CTRL_TIMEOUT_EXP_SHIFT   4
+#define IDM_CTRL_TIMEOUT_EXP_MASK    (0x1f << 4)
+#define IDM_CTRL_TIMEOUT_IRQ         BIT(3)
+#define IDM_CTRL_TIMEOUT_RESET       BIT(2)
+#define IDM_CTRL_BUS_ERR_IRQ         BIT(1)
+#define IDM_CTRL_BUS_ERR_RESET       BIT(0)
+
+#define IDM_COMP_OFFSET              0x004
+#define IDM_COMP_OVERFLOW            BIT(1)
+#define IDM_COMP_ERR                 BIT(0)
+
+#define IDM_STATUS_OFFSET            0x008
+#define IDM_STATUS_OVERFLOW          BIT(2)
+#define IDM_STATUS_CAUSE_MASK        0x03
+
+#define IDM_ADDR_LSB_OFFSET          0x00c
+#define IDM_ADDR_MSB_OFFSET          0x010
+#define IDM_ID_OFFSET                0x014
+#define IDM_FLAGS_OFFSET             0x01c
+
+#define IDM_ISR_STATUS_OFFSET        0x100
+#define IDM_ISR_STATUS_TIMEOUT       BIT(1)
+#define IDM_ISR_STATUS_ERR_LOG       BIT(0)
+
+#define ELOG_SIG_OFFSET              0x00
+#define ELOG_SIG_VAL                 0x49444d45
+
+#define ELOG_CUR_OFFSET              0x04
+#define ELOG_LEN_OFFSET              0x08
+#define ELOG_HEADER_LEN              12
+#define ELOG_EVENT_LEN               64
+
+#define ELOG_IDM_NAME_OFFSET         0x00
+#define ELOG_IDM_ADDR_LSB_OFFSET     0x10
+#define ELOG_IDM_ADDR_MSB_OFFSET     0x14
+#define ELOG_IDM_ID_OFFSET           0x18
+#define ELOG_IDM_CAUSE_OFFSET        0x20
+#define ELOG_IDM_FLAG_OFFSET         0x28
+
+#define ELOG_IDM_MAX_NAME_LEN        16
+
+#define ELOG_IDM_COMPAT_STR          "brcm,iproc-idm-elog"
+
+struct iproc_idm_elog {
+	struct device *dev;
+	void __iomem *buf;
+	u32 len;
+	spinlock_t lock;
+
+	int (*idm_event_log)(struct iproc_idm_elog *elog, const char *name,
+			     u32 cause, u32 addr_lsb, u32 addr_msb, u32 id,
+			     u32 flag);
+};
+
+struct iproc_idm {
+	struct device *dev;
+	struct iproc_idm_elog *elog;
+	void __iomem *base;
+	const char *name;
+	bool no_panic;
+};
+
+static ssize_t no_panic_store(struct device *dev, struct device_attribute *attr,
+			      const char *buf, size_t count)
+{
+	struct platform_device *pdev = to_platform_device(dev);
+	struct iproc_idm *idm = platform_get_drvdata(pdev);
+	unsigned int no_panic;
+	int ret;
+
+	ret = kstrtouint(buf, 0, &no_panic);
+	if (ret)
+		return ret;
+
+	idm->no_panic = no_panic ? true : false;
+
+	return count;
+}
+
+static ssize_t no_panic_show(struct device *dev, struct device_attribute *attr,
+			     char *buf)
+{
+	struct platform_device *pdev = to_platform_device(dev);
+	struct iproc_idm *idm = platform_get_drvdata(pdev);
+
+	return sprintf(buf, "%u\n", idm->no_panic ? 1 : 0);
+}
+
+static DEVICE_ATTR_RW(no_panic);
+
+static int iproc_idm_event_log(struct iproc_idm_elog *elog, const char *name,
+			       u32 cause, u32 addr_lsb, u32 addr_msb, u32 id,
+			       u32 flag)
+{
+	u32 val, cur, len;
+	void *event;
+	unsigned long flags;
+
+	spin_lock_irqsave(&elog->lock, flags);
+
+	/*
+	 * Check if signature is already there. If not, clear and restart
+	 * everything
+	 */
+	val = readl(elog->buf + ELOG_SIG_OFFSET);
+	if (val != ELOG_SIG_VAL) {
+		memset_io(elog->buf, 0, elog->len);
+		writel(ELOG_SIG_VAL, elog->buf + ELOG_SIG_OFFSET);
+		writel(ELOG_HEADER_LEN, elog->buf + ELOG_CUR_OFFSET);
+		writel(0, elog->buf + ELOG_LEN_OFFSET);
+	}
+
+	/* determine offset and length */
+	cur = readl(elog->buf + ELOG_CUR_OFFSET);
+	len = readl(elog->buf + ELOG_LEN_OFFSET);
+
+	/*
+	 * Based on the design and how kernel panic is triggered after an IDM
+	 * event, it's practically impossible for the storage to be full. In
+	 * case if it does happen, we can simply bail out since it's likely
+	 * the same category of events that have already been logged
+	 */
+	if (cur + ELOG_EVENT_LEN > elog->len) {
+		dev_warn(elog->dev, "IDM ELOG buffer is now full\n");
+		spin_unlock_irqrestore(&elog->lock, flags);
+		return -ENOMEM;
+	}
+
+	/* now log the IDM event */
+	event = elog->buf + cur;
+	memcpy_toio(event + ELOG_IDM_NAME_OFFSET, name, ELOG_IDM_MAX_NAME_LEN);
+	writel(addr_lsb, event + ELOG_IDM_ADDR_LSB_OFFSET);
+	writel(addr_msb, event + ELOG_IDM_ADDR_MSB_OFFSET);
+	writel(id, event + ELOG_IDM_ID_OFFSET);
+	writel(cause, event + ELOG_IDM_CAUSE_OFFSET);
+	writel(flag, event + ELOG_IDM_FLAG_OFFSET);
+
+	cur += ELOG_EVENT_LEN;
+	len += ELOG_EVENT_LEN;
+
+	/* update offset and length */
+	writel(cur, elog->buf + ELOG_CUR_OFFSET);
+	writel(len, elog->buf + ELOG_LEN_OFFSET);
+
+	spin_unlock_irqrestore(&elog->lock, flags);
+
+	return 0;
+}
+
+static irqreturn_t iproc_idm_irq_handler(int irq, void *data)
+{
+	struct iproc_idm *idm = data;
+	struct device *dev = idm->dev;
+	const char *name = idm->name;
+	u32 isr_status, log_status, lsb, msb, id, flag;
+	struct iproc_idm_elog *elog = idm->elog;
+
+	isr_status = readl(idm->base + IDM_ISR_STATUS_OFFSET);
+	log_status = readl(idm->base + IDM_STATUS_OFFSET);
+
+	/* quit if the interrupt is not for IDM */
+	if (!isr_status)
+		return IRQ_NONE;
+
+	/* ACK the interrupt */
+	if (log_status & IDM_STATUS_OVERFLOW)
+		writel(IDM_COMP_OVERFLOW, idm->base + IDM_COMP_OFFSET);
+
+	if (log_status & IDM_STATUS_CAUSE_MASK)
+		writel(IDM_COMP_ERR, idm->base + IDM_COMP_OFFSET);
+
+	/* dump critical IDM information */
+	if (isr_status & IDM_ISR_STATUS_TIMEOUT)
+		dev_err(dev, "[%s] IDM timeout\n", name);
+
+	if (isr_status & IDM_ISR_STATUS_ERR_LOG)
+		dev_err(dev, "[%s] IDM error log\n", name);
+
+	lsb = readl(idm->base + IDM_ADDR_LSB_OFFSET);
+	msb = readl(idm->base + IDM_ADDR_MSB_OFFSET);
+	id = readl(idm->base + IDM_ID_OFFSET);
+	flag = readl(idm->base + IDM_FLAGS_OFFSET);
+
+	dev_err(dev, "Cause: 0x%08x; Address LSB: 0x%08x; Address MSB: 0x%08x; Master ID: 0x%08x; Flag: 0x%08x\n",
+		log_status, lsb, msb, id, flag);
+
+	/* if elog service is available, log the event */
+	if (elog) {
+		elog->idm_event_log(elog, name, log_status, lsb, msb, id, flag);
+		dev_err(dev, "IDM event logged\n\n");
+	}
+
+	/* IDM timeout is fatal and non-recoverable. Panic the kernel */
+	if (!idm->no_panic)
+		panic("Fatal bus error detected by IDM");
+
+	return IRQ_HANDLED;
+}
+
+static int iproc_idm_dev_probe(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	struct device_node *np = dev->of_node;
+	struct platform_device *elog_pdev;
+	struct device_node *elog_np;
+	struct iproc_idm *idm;
+	const char *name;
+	int ret;
+	u32 val;
+
+	idm = devm_kzalloc(dev, sizeof(*idm), GFP_KERNEL);
+	if (!idm)
+		return -ENOMEM;
+
+	ret = of_property_read_string(np, "brcm,iproc-idm-bus", &name);
+	if (ret) {
+		dev_err(dev, "Unable to parse IDM bus name\n");
+		return ret;
+	}
+	idm->name = name;
+
+	platform_set_drvdata(pdev, idm);
+	idm->dev = dev;
+
+	idm->base = of_iomap(np, 0);
+	if (!idm->base) {
+		dev_err(dev, "Unable to map I/O\n");
+		ret = -ENOMEM;
+		goto err_exit;
+	}
+
+	ret = of_irq_get(np, 0);
+	if (ret <= 0) {
+		dev_err(dev, "Unable to find IRQ number. ret=%d\n", ret);
+		goto err_iounmap;
+	}
+
+	ret = devm_request_irq(dev, ret, iproc_idm_irq_handler, IRQF_SHARED,
+			       idm->name, idm);
+	if (ret < 0) {
+		dev_err(dev, "Unable to request irq. ret=%d\n", ret);
+		goto err_iounmap;
+	}
+
+	/*
+	 * ELOG phandle is optional. If ELOG phandle is specified, it indicates
+	 * ELOG logging needs to be enabled
+	 */
+	elog_np = of_parse_phandle(dev->of_node, ELOG_IDM_COMPAT_STR, 0);
+	if (elog_np) {
+		elog_pdev = of_find_device_by_node(elog_np);
+		if (!elog_pdev) {
+			dev_err(dev, "Unable to find IDM ELOG device\n");
+			ret = -ENODEV;
+			goto err_iounmap;
+		}
+
+		idm->elog = platform_get_drvdata(elog_pdev);
+		if (!idm->elog) {
+			dev_err(dev, "Unable to get IDM ELOG driver data\n");
+			ret = -EINVAL;
+			goto err_iounmap;
+		}
+	}
+
+	/* enable IDM timeout and its interrupt */
+	val = readl(idm->base + IDM_CTRL_OFFSET);
+	val |= IDM_CTRL_TIMEOUT_EXP_MASK | IDM_CTRL_TIMEOUT_ENABLE |
+	       IDM_CTRL_TIMEOUT_IRQ;
+	writel(val, idm->base + IDM_CTRL_OFFSET);
+
+	ret = device_create_file(dev, &dev_attr_no_panic);
+	if (ret < 0)
+		goto err_iounmap;
+
+	of_node_put(np);
+
+	pr_info("iProc IDM device %s registered\n", idm->name);
+
+	return 0;
+
+err_iounmap:
+	iounmap(idm->base);
+
+err_exit:
+	of_node_put(np);
+	return ret;
+}
+
+static int iproc_idm_elog_probe(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	struct iproc_idm_elog *elog;
+	struct resource *res;
+	u32 val;
+
+	elog = devm_kzalloc(dev, sizeof(*elog), GFP_KERNEL);
+	if (!elog)
+		return -ENOMEM;
+
+	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	elog->buf = (void __iomem *)devm_memremap(dev, res->start,
+						  resource_size(res),
+						  MEMREMAP_WB);
+	if (IS_ERR(elog->buf)) {
+		dev_err(dev, "Unable to map ELOG buffer\n");
+		return PTR_ERR(elog->buf);
+	}
+
+	elog->dev = dev;
+	elog->len = resource_size(res);
+	elog->idm_event_log = iproc_idm_event_log;
+
+	/*
+	 * Check if signature is already there. Only clear memory if there's
+	 * no signature detected
+	 */
+	val = readl(elog->buf + ELOG_SIG_OFFSET);
+	if (val != ELOG_SIG_VAL)
+		memset_io(elog->buf, 0, elog->len);
+
+	spin_lock_init(&elog->lock);
+	platform_set_drvdata(pdev, elog);
+
+	dev_info(dev, "iProc IDM ELOG registered\n");
+
+	return 0;
+}
+
+static int iproc_idm_probe(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	struct device_node *np = dev->of_node;
+	int ret;
+
+	if (of_device_is_compatible(np, ELOG_IDM_COMPAT_STR))
+		ret = iproc_idm_elog_probe(pdev);
+	else
+		ret = iproc_idm_dev_probe(pdev);
+
+	return ret;
+}
+
+static const struct of_device_id iproc_idm_of_match[] = {
+	{ .compatible = "brcm,iproc-idm", },
+	{ .compatible = ELOG_IDM_COMPAT_STR, },
+	{ }
+};
+
+static struct platform_driver iproc_idm_driver = {
+	.probe = iproc_idm_probe,
+	.driver = {
+		.name = "iproc-idm",
+		.of_match_table = of_match_ptr(iproc_idm_of_match),
+	},
+};
+
+static int __init iproc_idm_init(void)
+{
+	return platform_driver_register(&iproc_idm_driver);
+}
+arch_initcall(iproc_idm_init);
+
+static void __exit iproc_idm_exit(void)
+{
+	platform_driver_unregister(&iproc_idm_driver);
+}
+module_exit(iproc_idm_exit);
+
+MODULE_LICENSE("GPL v2");
+MODULE_AUTHOR("Broadcom");
+MODULE_DESCRIPTION("iProc IDM driver");
-- 
2.17.1

Re: [PATCH 1/2] dt-bindings: soc: Add binding doc for iProc IDM device

[Florian Fainelli]

On 12/2/19 3:31 PM, Ray Jui wrote:
> Add binding document for iProc based IDM devices.
> 
> Signed-off-by: Ray Jui <ray.jui@broadcom.com>

Looks good to me, it's 2019, nearly 2020, maybe make this a YAML
compatible binding since it is a new one?

> ---
>  .../bindings/soc/bcm/brcm,iproc-idm.txt       | 44 +++++++++++++++++++
>  1 file changed, 44 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/soc/bcm/brcm,iproc-idm.txt
> 
> diff --git a/Documentation/devicetree/bindings/soc/bcm/brcm,iproc-idm.txt b/Documentation/devicetree/bindings/soc/bcm/brcm,iproc-idm.txt
> new file mode 100644
> index 000000000000..388c6b036d7e
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/soc/bcm/brcm,iproc-idm.txt
> @@ -0,0 +1,44 @@
> +Broadcom iProc Interconnect Device Management (IDM) device
> +
> +The Broadcom iProc IDM device allows control and monitoring of ASIC internal
> +bus transactions. Most importantly, it can be configured to detect bus
> +transaction timeout. In such case, critical information such as transaction
> +address that caused the error, bus master ID of the transaction that caused
> +the error, and etc., are made available from the IDM device.
> +
> +-------------------------------------------------------------------------------
> +
> +Required properties for IDM device node:
> +- compatible: must be "brcm,iproc-idm"
> +- reg: base address and length of the IDM register space
> +- interrupt: IDM interrupt number
> +- brcm,iproc-idm-bus: IDM bus string
> +
> +Optional properties for IDM device node:
> +- brcm,iproc-idm-elog: phandle to the device node of the IDM logging device
> +
> +-------------------------------------------------------------------------------
> +
> +Required properties for IDM error logging device node:
> +- compatible: must be "brcm,iproc-idm-elog";
> +- reg: base address and length of reserved memory location where IDM error
> +  events can be saved
> +
> +-------------------------------------------------------------------------------
> +
> +Example:
> +
> +idm {
> +	idm-elog {
> +		compatible = "brcm,iproc-idm-elog";
> +		reg = <0x8f221000 0x1000>;
> +	};
> +
> +	idm-mhb-paxc-axi {
> +		compatible = "brcm,iproc-idm";
> +		reg = <0x60406900 0x200>;
> +		interrupt = <GIC_SPI 516 IRQ_TYPE_LEVEL_HIGH>;
> +		brcm,iproc-idm-bus = "idm-mhb-paxc-axi";
> +		brcm,iproc-idm-elog = <&idm-elog>;
> +	};
> +};
> 


-- 
Florian

Re: [PATCH 2/2] soc: bcm: iproc: Add Broadcom iProc IDM driver

[Florian Fainelli]

On 12/2/19 3:31 PM, Ray Jui wrote:
> Add Broadcom iProc IDM driver that controls that IDM devices available
> on various iProc based SoCs for bus transaction timeout monitoring and
> error logging.
> 
> Signed-off-by: Ray Jui <ray.jui@broadcom.com>
> Signed-off-by: Rayagonda Kokatanur <rayagonda.kokatanur@broadcom.com>
> ---

Looks good to me, just a few suggestions below

[snip]

> --- /dev/null
> +++ b/drivers/soc/bcm/iproc/Kconfig
> @@ -0,0 +1,6 @@

You would want an

if SOC_BRCM_IPROC

> +config IPROC_IDM
> +	bool "Broadcom iProc IDM driver"
> +	depends on (ARCH_BCM_IPROC || COMPILE_TEST) && OF
> +	default ARCH_BCM_IPROC
> +	help
> +	  Enables support for iProc Interconnect and Device Management (IDM) control and monitoring

and endif here to make this a nice menu.

[snip]

> +
> +static int iproc_idm_dev_probe(struct platform_device *pdev)
> +{
> +	struct device *dev = &pdev->dev;
> +	struct device_node *np = dev->of_node;
> +	struct platform_device *elog_pdev;
> +	struct device_node *elog_np;
> +	struct iproc_idm *idm;
> +	const char *name;
> +	int ret;
> +	u32 val;
> +
> +	idm = devm_kzalloc(dev, sizeof(*idm), GFP_KERNEL);
> +	if (!idm)
> +		return -ENOMEM;
> +
> +	ret = of_property_read_string(np, "brcm,iproc-idm-bus", &name);
> +	if (ret) {
> +		dev_err(dev, "Unable to parse IDM bus name\n");
> +		return ret;
> +	}
> +	idm->name = name;
> +
> +	platform_set_drvdata(pdev, idm);
> +	idm->dev = dev;
> +
> +	idm->base = of_iomap(np, 0);
> +	if (!idm->base) {
> +		dev_err(dev, "Unable to map I/O\n");
> +		ret = -ENOMEM;
> +		goto err_exit;
> +	}
> +
> +	ret = of_irq_get(np, 0);
> +	if (ret <= 0) {
> +		dev_err(dev, "Unable to find IRQ number. ret=%d\n", ret);
> +		goto err_iounmap;
> +	}

Since this is a standard platform device, you can use the standard
platform_get_resource() and platform_get_irq(). If you ever needed to
support ACPI in the future, that would make it transparent and almost
already ready.

> +
> +	ret = devm_request_irq(dev, ret, iproc_idm_irq_handler, IRQF_SHARED,
> +			       idm->name, idm);
> +	if (ret < 0) {
> +		dev_err(dev, "Unable to request irq. ret=%d\n", ret);
> +		goto err_iounmap;
> +	}
> +
> +	/*
> +	 * ELOG phandle is optional. If ELOG phandle is specified, it indicates
> +	 * ELOG logging needs to be enabled
> +	 */
> +	elog_np = of_parse_phandle(dev->of_node, ELOG_IDM_COMPAT_STR, 0);
> +	if (elog_np) {
> +		elog_pdev = of_find_device_by_node(elog_np);
> +		if (!elog_pdev) {
> +			dev_err(dev, "Unable to find IDM ELOG device\n");
> +			ret = -ENODEV;
> +			goto err_iounmap;
> +		}
> +
> +		idm->elog = platform_get_drvdata(elog_pdev);
> +		if (!idm->elog) {
> +			dev_err(dev, "Unable to get IDM ELOG driver data\n");
> +			ret = -EINVAL;
> +			goto err_iounmap;
> +		}
> +	}
> +
> +	/* enable IDM timeout and its interrupt */
> +	val = readl(idm->base + IDM_CTRL_OFFSET);
> +	val |= IDM_CTRL_TIMEOUT_EXP_MASK | IDM_CTRL_TIMEOUT_ENABLE |
> +	       IDM_CTRL_TIMEOUT_IRQ;
> +	writel(val, idm->base + IDM_CTRL_OFFSET);
> +
> +	ret = device_create_file(dev, &dev_attr_no_panic);
> +	if (ret < 0)
> +		goto err_iounmap;
> +
> +	of_node_put(np);

Did not you intend to drop the reference count on elog_np here?

[snip]

> +static struct platform_driver iproc_idm_driver = {
> +	.probe = iproc_idm_probe,

Do not you need a remove function in order to unregister the sysfs file
that you created in iproc_idm_dev_probe() to avoid bind/unbind (or
rmmod/modprobe) to spit out an existing sysfs entry warning?
-- 
Florian

Re: [PATCH 1/2] dt-bindings: soc: Add binding doc for iProc IDM device

[Ray Jui]

On 12/5/19 4:09 PM, Florian Fainelli wrote:
> On 12/2/19 3:31 PM, Ray Jui wrote:
>> Add binding document for iProc based IDM devices.
>>
>> Signed-off-by: Ray Jui <ray.jui@broadcom.com>
> 
> Looks good to me, it's 2019, nearly 2020, maybe make this a YAML
> compatible binding since it is a new one?
> 

Sorry I am not aware of this YAML requirement until now.

Is this a new requirement that new DT binding document should be made 
with YAML format?

Thanks,

Ray


>> ---
>>   .../bindings/soc/bcm/brcm,iproc-idm.txt       | 44 +++++++++++++++++++
>>   1 file changed, 44 insertions(+)
>>   create mode 100644 Documentation/devicetree/bindings/soc/bcm/brcm,iproc-idm.txt
>>
>> diff --git a/Documentation/devicetree/bindings/soc/bcm/brcm,iproc-idm.txt b/Documentation/devicetree/bindings/soc/bcm/brcm,iproc-idm.txt
>> new file mode 100644
>> index 000000000000..388c6b036d7e
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/soc/bcm/brcm,iproc-idm.txt
>> @@ -0,0 +1,44 @@
>> +Broadcom iProc Interconnect Device Management (IDM) device
>> +
>> +The Broadcom iProc IDM device allows control and monitoring of ASIC internal
>> +bus transactions. Most importantly, it can be configured to detect bus
>> +transaction timeout. In such case, critical information such as transaction
>> +address that caused the error, bus master ID of the transaction that caused
>> +the error, and etc., are made available from the IDM device.
>> +
>> +-------------------------------------------------------------------------------
>> +
>> +Required properties for IDM device node:
>> +- compatible: must be "brcm,iproc-idm"
>> +- reg: base address and length of the IDM register space
>> +- interrupt: IDM interrupt number
>> +- brcm,iproc-idm-bus: IDM bus string
>> +
>> +Optional properties for IDM device node:
>> +- brcm,iproc-idm-elog: phandle to the device node of the IDM logging device
>> +
>> +-------------------------------------------------------------------------------
>> +
>> +Required properties for IDM error logging device node:
>> +- compatible: must be "brcm,iproc-idm-elog";
>> +- reg: base address and length of reserved memory location where IDM error
>> +  events can be saved
>> +
>> +-------------------------------------------------------------------------------
>> +
>> +Example:
>> +
>> +idm {
>> +	idm-elog {
>> +		compatible = "brcm,iproc-idm-elog";
>> +		reg = <0x8f221000 0x1000>;
>> +	};
>> +
>> +	idm-mhb-paxc-axi {
>> +		compatible = "brcm,iproc-idm";
>> +		reg = <0x60406900 0x200>;
>> +		interrupt = <GIC_SPI 516 IRQ_TYPE_LEVEL_HIGH>;
>> +		brcm,iproc-idm-bus = "idm-mhb-paxc-axi";
>> +		brcm,iproc-idm-elog = <&idm-elog>;
>> +	};
>> +};
>>
> 
> 

Re: [PATCH 2/2] soc: bcm: iproc: Add Broadcom iProc IDM driver

[Ray Jui]

On 12/5/19 4:22 PM, Florian Fainelli wrote:
> On 12/2/19 3:31 PM, Ray Jui wrote:
>> Add Broadcom iProc IDM driver that controls that IDM devices available
>> on various iProc based SoCs for bus transaction timeout monitoring and
>> error logging.
>>
>> Signed-off-by: Ray Jui <ray.jui@broadcom.com>
>> Signed-off-by: Rayagonda Kokatanur <rayagonda.kokatanur@broadcom.com>
>> ---
> 
> Looks good to me, just a few suggestions below
> 
> [snip]
> 
>> --- /dev/null
>> +++ b/drivers/soc/bcm/iproc/Kconfig
>> @@ -0,0 +1,6 @@
> 
> You would want an
> 
> if SOC_BRCM_IPROC
> 
>> +config IPROC_IDM
>> +	bool "Broadcom iProc IDM driver"
>> +	depends on (ARCH_BCM_IPROC || COMPILE_TEST) && OF
>> +	default ARCH_BCM_IPROC
>> +	help
>> +	  Enables support for iProc Interconnect and Device Management (IDM) control and monitoring
> 
> and endif here to make this a nice menu.
> 

Sure I'll add SOC_BRCM_IPROC at one layer higher and use the if here.

> [snip]
> 
>> +
>> +static int iproc_idm_dev_probe(struct platform_device *pdev)
>> +{
>> +	struct device *dev = &pdev->dev;
>> +	struct device_node *np = dev->of_node;
>> +	struct platform_device *elog_pdev;
>> +	struct device_node *elog_np;
>> +	struct iproc_idm *idm;
>> +	const char *name;
>> +	int ret;
>> +	u32 val;
>> +
>> +	idm = devm_kzalloc(dev, sizeof(*idm), GFP_KERNEL);
>> +	if (!idm)
>> +		return -ENOMEM;
>> +
>> +	ret = of_property_read_string(np, "brcm,iproc-idm-bus", &name);
>> +	if (ret) {
>> +		dev_err(dev, "Unable to parse IDM bus name\n");
>> +		return ret;
>> +	}
>> +	idm->name = name;
>> +
>> +	platform_set_drvdata(pdev, idm);
>> +	idm->dev = dev;
>> +
>> +	idm->base = of_iomap(np, 0);
>> +	if (!idm->base) {
>> +		dev_err(dev, "Unable to map I/O\n");
>> +		ret = -ENOMEM;
>> +		goto err_exit;
>> +	}
>> +
>> +	ret = of_irq_get(np, 0);
>> +	if (ret <= 0) {
>> +		dev_err(dev, "Unable to find IRQ number. ret=%d\n", ret);
>> +		goto err_iounmap;
>> +	}
> 
> Since this is a standard platform device, you can use the standard
> platform_get_resource() and platform_get_irq(). If you ever needed to
> support ACPI in the future, that would make it transparent and almost
> already ready.
> 

Will do, thanks!

>> +
>> +	ret = devm_request_irq(dev, ret, iproc_idm_irq_handler, IRQF_SHARED,
>> +			       idm->name, idm);
>> +	if (ret < 0) {
>> +		dev_err(dev, "Unable to request irq. ret=%d\n", ret);
>> +		goto err_iounmap;
>> +	}
>> +
>> +	/*
>> +	 * ELOG phandle is optional. If ELOG phandle is specified, it indicates
>> +	 * ELOG logging needs to be enabled
>> +	 */
>> +	elog_np = of_parse_phandle(dev->of_node, ELOG_IDM_COMPAT_STR, 0);
>> +	if (elog_np) {
>> +		elog_pdev = of_find_device_by_node(elog_np);
>> +		if (!elog_pdev) {
>> +			dev_err(dev, "Unable to find IDM ELOG device\n");
>> +			ret = -ENODEV;
>> +			goto err_iounmap;
>> +		}
>> +
>> +		idm->elog = platform_get_drvdata(elog_pdev);
>> +		if (!idm->elog) {
>> +			dev_err(dev, "Unable to get IDM ELOG driver data\n");
>> +			ret = -EINVAL;
>> +			goto err_iounmap;
>> +		}
>> +	}
>> +
>> +	/* enable IDM timeout and its interrupt */
>> +	val = readl(idm->base + IDM_CTRL_OFFSET);
>> +	val |= IDM_CTRL_TIMEOUT_EXP_MASK | IDM_CTRL_TIMEOUT_ENABLE |
>> +	       IDM_CTRL_TIMEOUT_IRQ;
>> +	writel(val, idm->base + IDM_CTRL_OFFSET);
>> +
>> +	ret = device_create_file(dev, &dev_attr_no_panic);
>> +	if (ret < 0)
>> +		goto err_iounmap;
>> +
>> +	of_node_put(np);
> 
> Did not you intend to drop the reference count on elog_np here?
> 

Sorry, I'm not following this comment. Could you please help to clarify 
for me a bit more? Thanks!

> [snip]
> 
>> +static struct platform_driver iproc_idm_driver = {
>> +	.probe = iproc_idm_probe,
> 
> Do not you need a remove function in order to unregister the sysfs file
> that you created in iproc_idm_dev_probe() to avoid bind/unbind (or
> rmmod/modprobe) to spit out an existing sysfs entry warning?
> 

This driver should never be compiled as a module. It's meant to be 
always there to capture IDM bus timeouts.

But you are right that I cannot prevent user from trying to unbind it 
for whatever reason. I'll add a remove routine to take care of this.

Thanks!

Ray

Re: [PATCH 0/2] Add iProc IDM device support

[Marc Zyngier]

On Mon,  2 Dec 2019 15:31:25 -0800
Ray Jui <ray.jui@broadcom.com> wrote:

> The Broadcom iProc IDM device allows control and monitoring of ASIC internal
> bus transactions. Most importantly, it can be configured to detect bus
> transaction timeout. In such case, critical information such as transaction
> address that caused the error, bus master ID of the transaction that caused
> the error, and etc., are made available from the IDM device.

This seems to have many of the features of an EDAC device reporting
uncorrectable errors.

Is there any reason why it is not implemented as such?

Thanks,

	M.
-- 
Jazz is not dead. It just smells funny...

Re: [PATCH 2/2] soc: bcm: iproc: Add Broadcom iProc IDM driver

[Florian Fainelli]

On 12/6/2019 5:15 PM, Ray Jui wrote:
>>
>> Did not you intend to drop the reference count on elog_np here?
>>
> 
> Sorry, I'm not following this comment. Could you please help to clarify
> for me a bit more? Thanks!

I meant that you drop the reference count on 'np' but you called
functions that incremented the reference count on 'elog_np', so maybe
you are not doing the of_node_put() on the appropriate device_node
reference?

> 
>> [snip]
>>
>>> +static struct platform_driver iproc_idm_driver = {
>>> +    .probe = iproc_idm_probe,
>>
>> Do not you need a remove function in order to unregister the sysfs file
>> that you created in iproc_idm_dev_probe() to avoid bind/unbind (or
>> rmmod/modprobe) to spit out an existing sysfs entry warning?
>>
> 
> This driver should never be compiled as a module. It's meant to be
> always there to capture IDM bus timeouts.
> 
> But you are right that I cannot prevent user from trying to unbind it
> for whatever reason. I'll add a remove routine to take care of this.

You can also set suppress_bind_attrs to your platform_driver structure
to prevent unbind/bind from happening.
-- 
Florian

Re: [PATCH 0/2] Add iProc IDM device support

[Ray Jui]

On 12/7/19 9:39 AM, Marc Zyngier wrote:
> On Mon,  2 Dec 2019 15:31:25 -0800
> Ray Jui <ray.jui@broadcom.com> wrote:
> 
>> The Broadcom iProc IDM device allows control and monitoring of ASIC internal
>> bus transactions. Most importantly, it can be configured to detect bus
>> transaction timeout. In such case, critical information such as transaction
>> address that caused the error, bus master ID of the transaction that caused
>> the error, and etc., are made available from the IDM device.
> 
> This seems to have many of the features of an EDAC device reporting
> uncorrectable errors.
> 
> Is there any reason why it is not implemented as such?
> 
> Thanks,
> 
> 	M.
> 

I thought EDAC errors (in fact, in our case, that's fatal rather than 
uncorrectable) are mostly for DDR. Is my understanding incorrect?

Thanks,

Ray

Re: [PATCH 2/2] soc: bcm: iproc: Add Broadcom iProc IDM driver

[Ray Jui]

On 12/7/19 9:52 AM, Florian Fainelli wrote:
> 
> 
> On 12/6/2019 5:15 PM, Ray Jui wrote:
>>>
>>> Did not you intend to drop the reference count on elog_np here?
>>>
>>
>> Sorry, I'm not following this comment. Could you please help to clarify
>> for me a bit more? Thanks!
> 
> I meant that you drop the reference count on 'np' but you called
> functions that incremented the reference count on 'elog_np', so maybe
> you are not doing the of_node_put() on the appropriate device_node
> reference?
> 

Okay thanks. I'll look into this in more details and make corrections if 
needed.

>>
>>> [snip]
>>>
>>>> +static struct platform_driver iproc_idm_driver = {
>>>> +    .probe = iproc_idm_probe,
>>>
>>> Do not you need a remove function in order to unregister the sysfs file
>>> that you created in iproc_idm_dev_probe() to avoid bind/unbind (or
>>> rmmod/modprobe) to spit out an existing sysfs entry warning?
>>>
>>
>> This driver should never be compiled as a module. It's meant to be
>> always there to capture IDM bus timeouts.
>>
>> But you are right that I cannot prevent user from trying to unbind it
>> for whatever reason. I'll add a remove routine to take care of this.
> 
> You can also set suppress_bind_attrs to your platform_driver structure
> to prevent unbind/bind from happening.
> 

Great. This is what I'll do then. I meant to have this driver stays 
loaded/binded all the time once probed.

Thanks,

Ray

Re: [PATCH 0/2] Add iProc IDM device support

[Marc Zyngier]

On Mon, 9 Dec 2019 10:02:53 -0800
Ray Jui <ray.jui@broadcom.com> wrote:

> On 12/7/19 9:39 AM, Marc Zyngier wrote:
> > On Mon,  2 Dec 2019 15:31:25 -0800
> > Ray Jui <ray.jui@broadcom.com> wrote:
> >   
> >> The Broadcom iProc IDM device allows control and monitoring of ASIC internal
> >> bus transactions. Most importantly, it can be configured to detect bus
> >> transaction timeout. In such case, critical information such as transaction
> >> address that caused the error, bus master ID of the transaction that caused
> >> the error, and etc., are made available from the IDM device.  
> > 
> > This seems to have many of the features of an EDAC device reporting
> > uncorrectable errors.
> > 
> > Is there any reason why it is not implemented as such?
> > 
> > Thanks,
> > 
> > 	M.
> >   
> 
> I thought EDAC errors (in fact, in our case, that's fatal rather than
> uncorrectable) are mostly for DDR. Is my understanding incorrect?

No, they are for HW errors in general. There is no real limitation of
scope, as far as I understand. Recently, the Annapurna guys came up
with a similar HW block, and were convinced to make it an EDAC device.

See [1] for details.

Thanks,

	M.

[1] https://lore.kernel.org/linux-devicetree/1570707681-865-1-git-send-email-talel@amazon.com/
-- 
Jazz is not dead. It just smells funny...

Re: [PATCH 0/2] Add iProc IDM device support

[Ray Jui]

On 12/9/19 10:36 AM, Marc Zyngier wrote:
> On Mon, 9 Dec 2019 10:02:53 -0800
> Ray Jui <ray.jui@broadcom.com> wrote:
> 
>> On 12/7/19 9:39 AM, Marc Zyngier wrote:
>>> On Mon,  2 Dec 2019 15:31:25 -0800
>>> Ray Jui <ray.jui@broadcom.com> wrote:
>>>    
>>>> The Broadcom iProc IDM device allows control and monitoring of ASIC internal
>>>> bus transactions. Most importantly, it can be configured to detect bus
>>>> transaction timeout. In such case, critical information such as transaction
>>>> address that caused the error, bus master ID of the transaction that caused
>>>> the error, and etc., are made available from the IDM device.
>>>
>>> This seems to have many of the features of an EDAC device reporting
>>> uncorrectable errors.
>>>
>>> Is there any reason why it is not implemented as such?
>>>
>>> Thanks,
>>>
>>> 	M.
>>>    
>>
>> I thought EDAC errors (in fact, in our case, that's fatal rather than
>> uncorrectable) are mostly for DDR. Is my understanding incorrect?
> 
> No, they are for HW errors in general. There is no real limitation of
> scope, as far as I understand. Recently, the Annapurna guys came up
> with a similar HW block, and were convinced to make it an EDAC device.
> 
> See [1] for details.
> 
> Thanks,
> 
> 	M.
> 
> [1] https://lore.kernel.org/linux-devicetree/1570707681-865-1-git-send-email-talel@amazon.com/
> 

Ah I see. It looks like memory controllers are the primary devices 
supported by EDAC. In addition to that, EDAC also does seem to provide a 
generic data structure to support other types of HW devices and error 
events. I'll look into this and get back.

Thanks,

Ray

Re: [PATCH 1/2] dt-bindings: soc: Add binding doc for iProc IDM device

[Rob Herring]

On Fri, Dec 06, 2019 at 05:09:34PM -0800, Ray Jui wrote:
> 
> 
> On 12/5/19 4:09 PM, Florian Fainelli wrote:
> > On 12/2/19 3:31 PM, Ray Jui wrote:
> > > Add binding document for iProc based IDM devices.
> > > 
> > > Signed-off-by: Ray Jui <ray.jui@broadcom.com>
> > 
> > Looks good to me, it's 2019, nearly 2020, maybe make this a YAML
> > compatible binding since it is a new one?
> > 
> 
> Sorry I am not aware of this YAML requirement until now.
> 
> Is this a new requirement that new DT binding document should be made with
> YAML format?

The format has been in place in the kernel for a year now and we've 
moved slowly towards it being required. If you're paying that little 
attention to upstream, then yes it's definitely required so someone else 
doesn't get stuck converting your binding later.

BTW, I think all but RPi chips still need their SoC/board bindings 
converted. One of the few not yet converted...

> Thanks,
> 
> Ray
> 
> 
> > > ---
> > >   .../bindings/soc/bcm/brcm,iproc-idm.txt       | 44 +++++++++++++++++++
> > >   1 file changed, 44 insertions(+)
> > >   create mode 100644 Documentation/devicetree/bindings/soc/bcm/brcm,iproc-idm.txt
> > > 
> > > diff --git a/Documentation/devicetree/bindings/soc/bcm/brcm,iproc-idm.txt b/Documentation/devicetree/bindings/soc/bcm/brcm,iproc-idm.txt
> > > new file mode 100644
> > > index 000000000000..388c6b036d7e
> > > --- /dev/null
> > > +++ b/Documentation/devicetree/bindings/soc/bcm/brcm,iproc-idm.txt
> > > @@ -0,0 +1,44 @@
> > > +Broadcom iProc Interconnect Device Management (IDM) device
> > > +
> > > +The Broadcom iProc IDM device allows control and monitoring of ASIC internal
> > > +bus transactions. Most importantly, it can be configured to detect bus
> > > +transaction timeout. In such case, critical information such as transaction
> > > +address that caused the error, bus master ID of the transaction that caused
> > > +the error, and etc., are made available from the IDM device.
> > > +
> > > +-------------------------------------------------------------------------------
> > > +
> > > +Required properties for IDM device node:
> > > +- compatible: must be "brcm,iproc-idm"
> > > +- reg: base address and length of the IDM register space
> > > +- interrupt: IDM interrupt number
> > > +- brcm,iproc-idm-bus: IDM bus string

What are possible values?

> > > +
> > > +Optional properties for IDM device node:
> > > +- brcm,iproc-idm-elog: phandle to the device node of the IDM logging device
> > > +
> > > +-------------------------------------------------------------------------------
> > > +
> > > +Required properties for IDM error logging device node:
> > > +- compatible: must be "brcm,iproc-idm-elog";
> > > +- reg: base address and length of reserved memory location where IDM error
> > > +  events can be saved
> > > +
> > > +-------------------------------------------------------------------------------
> > > +
> > > +Example:
> > > +
> > > +idm {
> > > +	idm-elog {
> > > +		compatible = "brcm,iproc-idm-elog";
> > > +		reg = <0x8f221000 0x1000>;
> > > +	};
> > > +
> > > +	idm-mhb-paxc-axi {

Needs a unit-address.

> > > +		compatible = "brcm,iproc-idm";
> > > +		reg = <0x60406900 0x200>;
> > > +		interrupt = <GIC_SPI 516 IRQ_TYPE_LEVEL_HIGH>;
> > > +		brcm,iproc-idm-bus = "idm-mhb-paxc-axi";

Can't you just use the node name? Though if this is something that can 
be a standard class of h/w (i.e. EDAC), then we'd want the node name to 
be generic.

> > > +		brcm,iproc-idm-elog = <&idm-elog>;
> > > +	};
> > > +};
> > > 
> > 
> > 

Re: [PATCH 1/2] dt-bindings: soc: Add binding doc for iProc IDM device

[Florian Fainelli]

On 12/13/2019 3:50 PM, Rob Herring wrote:
> On Fri, Dec 06, 2019 at 05:09:34PM -0800, Ray Jui wrote:
>>
>>
>> On 12/5/19 4:09 PM, Florian Fainelli wrote:
>>> On 12/2/19 3:31 PM, Ray Jui wrote:
>>>> Add binding document for iProc based IDM devices.
>>>>
>>>> Signed-off-by: Ray Jui <ray.jui@broadcom.com>
>>>
>>> Looks good to me, it's 2019, nearly 2020, maybe make this a YAML
>>> compatible binding since it is a new one?
>>>
>>
>> Sorry I am not aware of this YAML requirement until now.
>>
>> Is this a new requirement that new DT binding document should be made with
>> YAML format?
> 
> The format has been in place in the kernel for a year now and we've 
> moved slowly towards it being required. If you're paying that little 
> attention to upstream, then yes it's definitely required so someone else 
> doesn't get stuck converting your binding later.
> 
> BTW, I think all but RPi chips still need their SoC/board bindings 
> converted. One of the few not yet converted...

Is there something more to do than what Stefan did here:

https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=ab06837dd269b600396b298e9f4678d02b11b71d

we could convert other Broadcom SoCs, and there, just found another
weekend project!
-- 
Florian

Re: [PATCH 1/2] dt-bindings: soc: Add binding doc for iProc IDM device

[Rob Herring]

On Fri, Dec 13, 2019 at 6:00 PM Florian Fainelli <f.fainelli@gmail.com> wrote:
>
>
>
> On 12/13/2019 3:50 PM, Rob Herring wrote:
> > On Fri, Dec 06, 2019 at 05:09:34PM -0800, Ray Jui wrote:
> >>
> >>
> >> On 12/5/19 4:09 PM, Florian Fainelli wrote:
> >>> On 12/2/19 3:31 PM, Ray Jui wrote:
> >>>> Add binding document for iProc based IDM devices.
> >>>>
> >>>> Signed-off-by: Ray Jui <ray.jui@broadcom.com>
> >>>
> >>> Looks good to me, it's 2019, nearly 2020, maybe make this a YAML
> >>> compatible binding since it is a new one?
> >>>
> >>
> >> Sorry I am not aware of this YAML requirement until now.
> >>
> >> Is this a new requirement that new DT binding document should be made with
> >> YAML format?
> >
> > The format has been in place in the kernel for a year now and we've
> > moved slowly towards it being required. If you're paying that little
> > attention to upstream, then yes it's definitely required so someone else
> > doesn't get stuck converting your binding later.
> >
> > BTW, I think all but RPi chips still need their SoC/board bindings
> > converted. One of the few not yet converted...
>
> Is there something more to do than what Stefan did here:
>
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=ab06837dd269b600396b298e9f4678d02b11b71d

No, that's it.

> we could convert other Broadcom SoCs, and there, just found another
> weekend project!
> --
> Florian