2019-12-02 23:32:33

by Ray Jui

[permalink] [raw]
Subject: [PATCH 0/2] Add iProc IDM device support

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


2019-12-02 23:32:44

by Ray Jui

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

Add binding document for iProc based IDM devices.

Signed-off-by: Ray Jui <[email protected]>
---
.../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

2019-12-02 23:34:01

by Ray Jui

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

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 <[email protected]>
Signed-off-by: Rayagonda Kokatanur <[email protected]>
---
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

2019-12-06 00:12:46

by Florian Fainelli

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

On 12/2/19 3:31 PM, Ray Jui wrote:
> Add binding document for iProc based IDM devices.
>
> Signed-off-by: Ray Jui <[email protected]>

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

2019-12-06 00:23:30

by Florian Fainelli

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

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 <[email protected]>
> Signed-off-by: Rayagonda Kokatanur <[email protected]>
> ---

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

2019-12-07 01:11:37

by Ray Jui

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



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 <[email protected]>
>
> 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>;
>> + };
>> +};
>>
>
>

2019-12-07 01:16:26

by Ray Jui

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



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 <[email protected]>
>> Signed-off-by: Rayagonda Kokatanur <[email protected]>
>> ---
>
> 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

2019-12-07 17:40:08

by Marc Zyngier

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

On Mon, 2 Dec 2019 15:31:25 -0800
Ray Jui <[email protected]> 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...

2019-12-07 17:53:31

by Florian Fainelli

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



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

2019-12-09 18:04:24

by Ray Jui

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



On 12/7/19 9:39 AM, Marc Zyngier wrote:
> On Mon, 2 Dec 2019 15:31:25 -0800
> Ray Jui <[email protected]> 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

2019-12-09 18:07:17

by Ray Jui

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



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

2019-12-09 18:37:37

by Marc Zyngier

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

On Mon, 9 Dec 2019 10:02:53 -0800
Ray Jui <[email protected]> wrote:

> On 12/7/19 9:39 AM, Marc Zyngier wrote:
> > On Mon, 2 Dec 2019 15:31:25 -0800
> > Ray Jui <[email protected]> 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/[email protected]/
--
Jazz is not dead. It just smells funny...

2019-12-10 00:20:58

by Ray Jui

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



On 12/9/19 10:36 AM, Marc Zyngier wrote:
> On Mon, 9 Dec 2019 10:02:53 -0800
> Ray Jui <[email protected]> wrote:
>
>> On 12/7/19 9:39 AM, Marc Zyngier wrote:
>>> On Mon, 2 Dec 2019 15:31:25 -0800
>>> Ray Jui <[email protected]> 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/[email protected]/
>

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

2019-12-13 23:51:10

by Rob Herring

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

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 <[email protected]>
> >
> > 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>;
> > > + };
> > > +};
> > >
> >
> >

2019-12-14 00:01:25

by Florian Fainelli

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



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 <[email protected]>
>>>
>>> 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