Date:	Sat, 31 Oct 2015 02:51:46 -0400
From:	Sinan Kaya <okaya@...eaurora.org>
To:	Arnd Bergmann <arnd@...db.de>
Cc:	dmaengine@...r.kernel.org, timur@...eaurora.org,
	cov@...eaurora.org, jcm@...hat.com,
	Rob Herring <robh+dt@...nel.org>,
	Pawel Moll <pawel.moll@....com>,
	Mark Rutland <mark.rutland@....com>,
	Ian Campbell <ijc+devicetree@...lion.org.uk>,
	Kumar Gala <galak@...eaurora.org>,
	Vinod Koul <vinod.koul@...el.com>,
	Dan Williams <dan.j.williams@...el.com>,
	devicetree@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH 1/2] dma: add Qualcomm Technologies HIDMA management
 driver



On 10/30/2015 5:34 AM, Arnd Bergmann wrote:
> On Thursday 29 October 2015 23:08:12 Sinan Kaya wrote:
>> diff --git a/Documentation/devicetree/bindings/dma/qcom_hidma_mgmt.txt b/Documentation/devicetree/bindings/dma/qcom_hidma_mgmt.txt
>> new file mode 100644
>> index 0000000..81674ab
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/dma/qcom_hidma_mgmt.txt
>> @@ -0,0 +1,42 @@
>> +Qualcomm Technologies HIDMA Management interface
>> +
>> +Required properties:
>> +- compatible: must contain "qcom,hidma_mgmt"
>
> No underscores in the name please. Also try to be more specific, in case this
> device shows up in more than one SoC and they end up not being 100% compatible
> we want to be able to tell them apart.

ok

>
>> +- reg: Address range for DMA device
>> +- interrupts: Should contain the one interrupt shared by all channels
>> +- nr-channels: Number of channels supported by this DMA controller.
>> +- max-write: Maximum write burst in bytes. A memcpy requested is
>> +  fragmented to multiples of this amount.
>> +- max-read: Maximum read burst in bytes. A memcpy request is
>> +  fragmented to multiples of this amount.
>> +- max-wxactions: Maximum write transactions to perform in a burst
>> +- max-rdactions: Maximum read transactions to perform in a burst
>> +- max-memset-limit: Maximum memset limit
>> +- ch-priority-#n: Priority of the channel
>> +- ch-weight-#n: Round robin weight of the channel
>
> For the last two, you can just use a multi-cell property, using one
> cell per channel.

will look
>
>> +
>> +#define MODULE_NAME			"hidma-mgmt"
>> +#define PREFIX				MODULE_NAME ": "
>
> These can be removed, just use dev_info() etc for messages, to include
> a unique identifier.

got rid of them

>
>> +#define AUTOSUSPEND_TIMEOUT		2000
>> +
>> +#define HIDMA_RUNTIME_GET(dmadev)				\
>> +do {								\
>> +	atomic_inc(&(dmadev)->pm_counter);			\
>> +	TRC_PM(&(dmadev)->pdev->dev,				\
>> +		"%s:%d pm_runtime_get %d\n", __func__, __LINE__,\
>> +		atomic_read(&(dmadev)->pm_counter));		\
>> +	pm_runtime_get_sync(&(dmadev)->pdev->dev);		\
>> +} while (0)
>> +
>> +#define HIDMA_RUNTIME_SET(dmadev)				\
>> +do {								\
>> +	atomic_dec(&(dmadev)->pm_counter);			\
>> +	TRC_PM(&(dmadev)->pdev->dev,				\
>> +		"%s:%d pm_runtime_put_autosuspend:%d\n",	\
>> +		__func__, __LINE__,				\
>> +		atomic_read(&(dmadev)->pm_counter));		\
>> +	pm_runtime_mark_last_busy(&(dmadev)->pdev->dev);	\
>> +	pm_runtime_put_autosuspend(&(dmadev)->pdev->dev);	\
>> +} while (0)
>
> Better use inline functions.

removed these

>
>> +struct qcom_hidma_mgmt_dev {
>> +	u8 max_wr_xactions;
>> +	u8 max_rd_xactions;
>> +	u8 max_memset_limit;
>> +	u16 max_write_request;
>> +	u16 max_read_request;
>> +	u16 nr_channels;
>> +	u32 chreset_timeout;
>> +	u32 sw_version;
>> +	u8 *priority;
>> +	u8 *weight;
>> +
>> +	atomic_t	pm_counter;
>> +	/* Hardware device constants */
>> +	dma_addr_t dev_physaddr;
>
> No need to store the physaddr, it's write-only here.
ok
>
>> +
>> +static unsigned int debug_pm;
>> +module_param(debug_pm, uint, 0644);
>> +MODULE_PARM_DESC(debug_pm,
>> +		 "debug runtime power management transitions (default: 0)");
>> +
>> +#define TRC_PM(...) do {			\
>> +		if (debug_pm)			\
>> +			dev_info(__VA_ARGS__);	\
>> +	} while (0)
>> +
>
> Once you are done debugging the driver and have a final version, please
> remove these.

removed TRC_PM

>
>> +#if IS_ENABLED(CONFIG_DEBUG_FS)
>> +
>> +#define HIDMA_SHOW(dma, name) \
>> +		seq_printf(s, #name "=0x%x\n", dma->name)
>
> Only used once, just remove.
ok

>
>> +#define HIDMA_READ_SHOW(dma, name, offset) \
>> +	do { \
>> +		u32 val; \
>> +		val = readl(dma->dev_virtaddr + offset); \
>> +		seq_printf(s, name "=0x%x\n", val); \
>> +	} while (0)
>
> Inline function.

ok

>
>> +/**
>> + * qcom_hidma_mgmt_info: display HIDMA device info
>> + *
>> + * Display the info for the current HIDMA device.
>> + */
>> +static int qcom_hidma_mgmt_info(struct seq_file *s, void *unused)
>> +{
>> +	struct qcom_hidma_mgmt_dev *mgmtdev = s->private;
>> +	u32 val;
>> +	int i;
>> +
>> +	HIDMA_RUNTIME_GET(mgmtdev);
>> +	HIDMA_SHOW(mgmtdev, sw_version);
>> +
>> +	val = readl(mgmtdev->dev_virtaddr + CFG_OFFSET);
>> +	seq_printf(s, "ENABLE=%d\n", val & 0x1);
>> +
>> +	val = readl(mgmtdev->dev_virtaddr + CHRESET_TIMEOUUT_OFFSET);
>> +	seq_printf(s, "reset_timeout=%d\n", val & CHRESET_TIMEOUUT_MASK);
>> +
>> +	val = readl(mgmtdev->dev_virtaddr + MHICFG_OFFSET);
>> +	seq_printf(s, "nr_event_channel=%d\n",
>> +		(val >> NR_EV_CHANNEL_BIT_POS) & NR_CHANNEL_MASK);
>> +	seq_printf(s, "nr_tr_channel=%d\n", (val & NR_CHANNEL_MASK));
>> +	seq_printf(s, "nr_virt_tr_channel=%d\n", mgmtdev->nr_channels);
>> +	seq_printf(s, "dev_virtaddr=%p\n", &mgmtdev->dev_virtaddr);
>> +	seq_printf(s, "dev_physaddr=%pap\n", &mgmtdev->dev_physaddr);
>> +	seq_printf(s, "dev_addrsize=%pap\n", &mgmtdev->dev_addrsize);
>
> printing pointers is a security risk, just remove them if they are
> not essential here.
ok

>> +
>> +static int qcom_hidma_mgmt_err_open(struct inode *inode, struct file *file)
>> +{
>> +	return single_open(file, qcom_hidma_mgmt_err, inode->i_private);
>> +}
>> +
>> +static const struct file_operations qcom_hidma_mgmt_err_fops = {
>> +	.open = qcom_hidma_mgmt_err_open,
>> +	.read = seq_read,
>> +	.llseek = seq_lseek,
>> +	.release = single_release,
>> +};
>> +
>> +static ssize_t qcom_hidma_mgmt_mhiderr_clr(struct file *file,
>> +	const char __user *user_buf, size_t count, loff_t *ppos)
>> +{
>> +	struct qcom_hidma_mgmt_dev *mgmtdev = file->f_inode->i_private;
>> +
>> +	HIDMA_RUNTIME_GET(mgmtdev);
>> +	writel(1, mgmtdev->dev_virtaddr + MHID_BUS_ERR_CLR_OFFSET);
>> +	HIDMA_RUNTIME_SET(mgmtdev);
>> +	return count;
>> +}
>> +
>> +static const struct file_operations qcom_hidma_mgmt_mhiderr_clrfops = {
>> +	.write = qcom_hidma_mgmt_mhiderr_clr,
>> +};
>
> Is this really just a debugging interface? If anyone would do this
> for normal operation, it needs to be a proper API.
>
This will be used by the system admin to monitor/reset the execution 
state of the DMA channels. This will be the management interface. 
Debugfs is probably not the right choice. I originally had sysfs but 
than had some doubts. I'm open to suggestions.

>> +
>> +static void qcom_hidma_mgmt_debug_uninit(struct qcom_hidma_mgmt_dev *mgmtdev)
>> +{
>> +	debugfs_remove(mgmtdev->evt_ena);
>> +	debugfs_remove(mgmtdev->tre_errclr);
>> +	debugfs_remove(mgmtdev->msi_errclr);
>> +	debugfs_remove(mgmtdev->ode_errclr);
>> +	debugfs_remove(mgmtdev->ide_errclr);
>> +	debugfs_remove(mgmtdev->evt_errclr);
>> +	debugfs_remove(mgmtdev->mhid_errclr);
>> +	debugfs_remove(mgmtdev->err);
>> +	debugfs_remove(mgmtdev->info);
>> +	debugfs_remove(mgmtdev->debugfs);
>> +}
>>
> I guess debugfs_remove_recursive() would do the job as well.

ok

>
>> +static int qcom_hidma_mgmt_debug_init(struct qcom_hidma_mgmt_dev *mgmtdev)
>> +{
>> +	int rc = 0;
>> +
>> +	mgmtdev->debugfs = debugfs_create_dir(dev_name(&mgmtdev->pdev->dev),
>> +						NULL);
>> +	if (!mgmtdev->debugfs) {
>> +		rc = -ENODEV;
>> +		return rc;
>> +	}
>> +
>> +	mgmtdev->info = debugfs_create_file("info", S_IRUGO,
>> +			mgmtdev->debugfs, mgmtdev, &qcom_hidma_mgmt_fops);
>> +	if (!mgmtdev->info) {
>> +		rc = -ENOMEM;
>> +		goto cleanup;
>> +	}
>> +
>> +	mgmtdev->err = debugfs_create_file("err", S_IRUGO,
>> +			mgmtdev->debugfs, mgmtdev,
>> +			&qcom_hidma_mgmt_err_fops);
>> +	if (!mgmtdev->err) {
>> +		rc = -ENOMEM;
>> +		goto cleanup;
>> +	}
>> +
>> +	mgmtdev->mhid_errclr = debugfs_create_file("mhiderrclr", S_IWUSR,
>> +			mgmtdev->debugfs, mgmtdev,
>> +			&qcom_hidma_mgmt_mhiderr_clrfops);
>> +	if (!mgmtdev->mhid_errclr) {
>> +		rc = -ENOMEM;
>> +		goto cleanup;
>> +	}
>
> Maybe use a loop around an array here to avoid writing the same thing 10 times?
>
will do

> Also, you could move the debugging code into a separate file and have a separate
> Kconfig symbol so users can disable this if they do not debug your driver
> but still want to use debugfs for other things.
>
I need these to be accessible all the time, maybe via sysfs or ioctl.

>> +static irqreturn_t qcom_hidma_mgmt_irq_handler(int irq, void *arg)
>> +{
>> +	/* TODO: handle irq here */
>> +	return IRQ_HANDLED;
>> +}
>
>
>> +	rc = devm_request_irq(&pdev->dev, irq, qcom_hidma_mgmt_irq_handler,
>> +		IRQF_SHARED, "qcom-hidmamgmt", mgmtdev);
>> +	if (rc) {
>> +		dev_err(&pdev->dev, "irq registration failed: %d\n", irq);
>> +		goto out;
>> +	}
>
> If you create a shared handler, you must check whether there was an
> interrupt pending, otherwise you will lose interrupts for all other devices
> that share the same IRQ.

ok, I'll remove it for now. The interrupt handler is a to-do.

>
> Better remove the handler completely here so you don't need to check it.
>
>> +
>> +	if (device_property_read_u16(&pdev->dev, "nr-channels",
>> +		&mgmtdev->nr_channels)) {
>> +		dev_err(&pdev->dev, "number of channels missing\n");
>> +		rc = -EINVAL;
>> +		goto out;
>> +	}
>
> This should be a u32 name 'dma-channels' for consistency with the
> generic DMA binding.
>
ok

> Also, try to avoid using u8 and u16 properties everywhere and just
> us u32 for consistency.

will do

>
>> +
>> +	for (i = 0; i < mgmtdev->nr_channels; i++) {
>> +		char name[30];
>> +
>> +		sprintf(name, "ch-priority-%d", i);
>> +		device_property_read_u8(&pdev->dev, name,
>> +			&mgmtdev->priority[i]);
>> +
>> +		sprintf(name, "ch-weight-%d", i);
>> +		device_property_read_u8(&pdev->dev, name,
>> +			&mgmtdev->weight[i]);
>
> Per comment above, just read this as an array.

will look. While device tree syntax for arrays is easy, describing an 
array in DSM package looks really ugly. I tried to keep things simple. 
I'll spend some time on it.

>
>> +	dev_info(&pdev->dev,
>> +		"HI-DMA engine management driver registration complete\n");
>> +	platform_set_drvdata(pdev, mgmtdev);
>> +	HIDMA_RUNTIME_SET(mgmtdev);
>> +	return 0;
>> +out:
>> +	pm_runtime_disable(&pdev->dev);
>> +	pm_runtime_put_sync_suspend(&pdev->dev);
>> +	return rc;
>> +}
>
> The rest of the probe function does not register any user interface aside from
> the debugging stuff. Can you explain in the changelog how you expect the
> driver to be used in a real system? Is there another driver coming?

I expect this driver to grow in functionality over time. Right now, it 
does the global init for the DMA. After that all channels execute on 
their own without depending on each other. Global init has to be done 
first before attempting to do any channel initialization.

There is also implied startup ordering requirements. I was doing this by 
using channel driver with the late binding to guarantee that.

As soon as I use module_platform_driver, the ordering gets reversed for 
some reason.

>
>> +
>> +static const struct of_device_id qcom_hidma_mgmt_match[] = {
>> +	{ .compatible = "qcom,hidma_mgmt", },
>> +	{},
>> +};
>> +MODULE_DEVICE_TABLE(of, qcom_hidma_mgmt_match);
>> +
>> +static struct platform_driver qcom_hidma_mgmt_driver = {
>> +	.probe = qcom_hidma_mgmt_probe,
>> +	.remove = qcom_hidma_mgmt_remove,
>> +	.driver = {
>> +		.name = MODULE_NAME,
>> +		.owner = THIS_MODULE,
>> +		.of_match_table = of_match_ptr(qcom_hidma_mgmt_match),
>
> drop the .owner field and the of_match_ptr() macro to avoid warnings here.
>
>> +static int __init qcom_hidma_mgmt_init(void)
>> +{
>> +	return platform_driver_register(&qcom_hidma_mgmt_driver);
>> +}
>> +device_initcall(qcom_hidma_mgmt_init);
>> +
>> +static void __exit qcom_hidma_mgmt_exit(void)
>> +{
>> +	platform_driver_unregister(&qcom_hidma_mgmt_driver);
>> +}
>> +module_exit(qcom_hidma_mgmt_exit);
>
> module_platform_driver()
>
> 	Arnd
>
thanks for the feedback.

-- 
Sinan Kaya
Qualcomm Technologies, Inc. on behalf of Qualcomm Innovation Center, Inc.
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a 
Linux Foundation Collaborative Project
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists