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@xxxxxxxxxxxx>
Signed-off-by: Rayagonda Kokatanur <rayagonda.kokatanur@xxxxxxxxxxxx>
---
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
