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

[Ray Jui <ray.jui@broadcom.com>]

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