Re: [PATCH V2 1/3] dma: add Qualcomm Technologies HIDMA management driver

[Sinan Kaya]

On 11/3/2015 5:22 AM, Andy Shevchenko wrote:
> On Mon, Nov 2, 2015 at 8:07 AM, Sinan Kaya <okaya@xxxxxxxxxxxxxx> wrote:
> > The Qualcomm Technologies HIDMA device has been designed
> > to support virtualization technology. The driver has been
> > divided into two to follow the hardware design. The management
> > driver is executed in hypervisor context and is the main
> > management entity for all channels provided by the device.
> > The channel driver is executed in the hypervisor/guest OS
> > context.
> >
> > All channel devices get probed in the hypervisor
> > context during power up. They show up as DMA engine
> > channels. Then, before starting the virtualization; each
> > channel device is unbound from the hypervisor by VFIO
> > and assigned to the guest machine for control.
> >
> > This management driver will be used by the system
> > admin to monitor/reset the execution state of the DMA
> > channels. This will be the management interface.
>
>
> > +static ssize_t qcom_hidma_mgmt_evtena(struct file *file,
> > +       const char __user *user_buf, size_t count, loff_t *ppos)
> > +{
> > +       char temp_buf[16+1];
>
> 16 is a magic number. Make it defined constant.

removed it

> > +       struct qcom_hidma_mgmt_dev *mgmtdev = file->f_inode->i_private;
> > +       u32 event;
> > +       ssize_t ret;
> > +       unsigned long val;
> > +
> > +       temp_buf[16] = '\0';
> > +       if (copy_from_user(temp_buf, user_buf, min_t(int, count, 16)))
> > +               goto out;
> > +
> > +       ret = kstrtoul(temp_buf, 16, &val);
>
> kstrtoul_from_user?

nice, simpler.

> > +       if (ret) {
> > +               dev_warn(&mgmtdev->pdev->dev, "unknown event\n");
> > +               goto out;
> > +       }
> > +
> > +       event = (u32)val & HW_EVENTS_CFG_MASK;
> > +
> > +       pm_runtime_get_sync(&mgmtdev->pdev->dev);
> > +       writel(event, mgmtdev->dev_virtaddr + HW_EVENTS_CFG_OFFSET);
> > +       pm_runtime_mark_last_busy(&mgmtdev->pdev->dev);
> > +       pm_runtime_put_autosuspend(&mgmtdev->pdev->dev);
> > +out:
> > +       return count;
> > +}
> > +
> > +static const struct file_operations qcom_hidma_mgmt_evtena_fops = {
> > +       .write = qcom_hidma_mgmt_evtena,
> > +};
> > +
> > +struct fileinfo {
> > +       char *name;
> > +       int mode;
> > +       const struct file_operations *ops;
> > +};
> > +
> > +static struct fileinfo files[] = {
> > +       {"info", S_IRUGO, &qcom_hidma_mgmt_fops},
> > +       {"err", S_IRUGO,  &qcom_hidma_mgmt_err_fops},
> > +       {"mhiderrclr", S_IWUSR, &qcom_hidma_mgmt_mhiderr_clrfops},
> > +       {"evterrclr", S_IWUSR, &qcom_hidma_mgmt_evterr_clrfops},
> > +       {"ideerrclr", S_IWUSR, &qcom_hidma_mgmt_ideerr_clrfops},
> > +       {"odeerrclr", S_IWUSR, &qcom_hidma_mgmt_odeerr_clrfops},
> > +       {"msierrclr", S_IWUSR, &qcom_hidma_mgmt_msierr_clrfops},
> > +       {"treerrclr", S_IWUSR, &qcom_hidma_mgmt_treerr_clrfops},
> > +       {"evtena", S_IWUSR, &qcom_hidma_mgmt_evtena_fops},
> > +};
> > +
> > +static void qcom_hidma_mgmt_debug_uninit(struct qcom_hidma_mgmt_dev *mgmtdev)
> > +{
> > +       debugfs_remove_recursive(mgmtdev->debugfs);
> > +}
> > +
> > +static int qcom_hidma_mgmt_debug_init(struct qcom_hidma_mgmt_dev *mgmtdev)
> > +{
> > +       int rc = 0;
> > +       u32 i;
> > +       struct dentry   *fs_entry;
> > +
> > +       mgmtdev->debugfs = debugfs_create_dir(dev_name(&mgmtdev->pdev->dev),
> > +                                               NULL);
> > +       if (!mgmtdev->debugfs) {
> > +               rc = -ENODEV;
> > +               return rc;
> > +       }
> > +
> > +       for (i = 0; i < ARRAY_SIZE(files); i++) {
> > +               fs_entry = debugfs_create_file(files[i].name,
> > +                                       files[i].mode, mgmtdev->debugfs,
> > +                                       mgmtdev, files[i].ops);
> > +               if (!fs_entry) {
> > +                       rc = -ENOMEM;
> > +                       goto cleanup;
> > +               }
> > +       }
> > +
> > +       return 0;
> > +cleanup:
> > +       qcom_hidma_mgmt_debug_uninit(mgmtdev);
> > +       return rc;
> > +}
> > +#else
> > +static void qcom_hidma_mgmt_debug_uninit(struct qcom_hidma_mgmt_dev *mgmtdev)
> > +{
> > +}
> > +static int qcom_hidma_mgmt_debug_init(struct qcom_hidma_mgmt_dev *mgmtdev)
> > +{
> > +       return 0;
> > +}
> > +#endif
> > +
> > +static int qcom_hidma_mgmt_setup(struct qcom_hidma_mgmt_dev *mgmtdev)
> > +{
> > +       u32 val;
> > +       u32 i;
> > +
> > +       val = readl(mgmtdev->dev_virtaddr + MAX_BUS_REQ_LEN_OFFSET);
> > +       val = val &
> > +               ~(MAX_BUS_REQ_LEN_MASK << MAX_BUS_WR_REQ_BIT_POS);
> > +       val = val |
> > +               (mgmtdev->max_write_request << MAX_BUS_WR_REQ_BIT_POS);
> > +       val = val & ~(MAX_BUS_REQ_LEN_MASK);
> > +       val = val | (mgmtdev->max_read_request);
> > +       writel(val, mgmtdev->dev_virtaddr + MAX_BUS_REQ_LEN_OFFSET);
> > +
> > +       val = readl(mgmtdev->dev_virtaddr + MAX_XACTIONS_OFFSET);
> > +       val = val &
> > +               ~(MAX_WR_XACTIONS_MASK << MAX_WR_XACTIONS_BIT_POS);
> > +       val = val |
> > +               (mgmtdev->max_wr_xactions << MAX_WR_XACTIONS_BIT_POS);
> > +       val = val & ~(MAX_RD_XACTIONS_MASK);
> > +       val = val | (mgmtdev->max_rd_xactions);
> > +       writel(val, mgmtdev->dev_virtaddr + MAX_XACTIONS_OFFSET);
> > +
> > +       mgmtdev->sw_version = readl(mgmtdev->dev_virtaddr + SW_VERSION_OFFSET);
> > +
> > +       for (i = 0; i < mgmtdev->dma_channels; i++) {
> > +               val = readl(mgmtdev->dev_virtaddr + QOS_N_OFFSET + (4 * i));
> > +               val = val & ~(1 << PRIORITY_BIT_POS);
> > +               val = val |
> > +                       ((mgmtdev->priority[i] & 0x1) << PRIORITY_BIT_POS);
> > +               val = val & ~(WEIGHT_MASK << WRR_BIT_POS);
> > +               val = val
> > +                       | ((mgmtdev->weight[i] & WEIGHT_MASK) << WRR_BIT_POS);
> > +               writel(val, mgmtdev->dev_virtaddr + QOS_N_OFFSET + (4 * i));
> > +       }
> > +
> > +       val = readl(mgmtdev->dev_virtaddr + CHRESET_TIMEOUT_OFFSET);
> > +       val = val & ~CHRESET_TIMEOUUT_MASK;
> > +       val = val | (mgmtdev->chreset_timeout & CHRESET_TIMEOUUT_MASK);
> > +       writel(val, mgmtdev->dev_virtaddr + CHRESET_TIMEOUT_OFFSET);
> > +
> > +       val = readl(mgmtdev->dev_virtaddr + CFG_OFFSET);
> > +       val = val | 1;
> > +       writel(val, mgmtdev->dev_virtaddr + CFG_OFFSET);
> > +
> > +       return 0;
> > +}
> > +
> > +static int qcom_hidma_mgmt_probe(struct platform_device *pdev)
> > +{
> > +       struct resource *dma_resource;
> > +       int irq;
> > +       int rc;
> > +       u32 i;
> > +       struct qcom_hidma_mgmt_dev *mgmtdev;
>
> Better move this line to the top of definition block.
done

> > +
> > +       pm_runtime_set_autosuspend_delay(&pdev->dev, AUTOSUSPEND_TIMEOUT);
> > +       pm_runtime_use_autosuspend(&pdev->dev);
> > +       pm_runtime_set_active(&pdev->dev);
> > +       pm_runtime_enable(&pdev->dev);
> > +       dma_resource = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> > +       if (!dma_resource) {
> > +               dev_err(&pdev->dev, "No memory resources found\n");
> > +               rc = -ENODEV;
> > +               goto out;
> > +       }
>
> Consolidate with devm_ioremap_resource()

OK

> > +
> > +       irq = platform_get_irq(pdev, 0);
> > +       if (irq < 0) {
> > +               dev_err(&pdev->dev, "irq resources not found\n");
> > +               rc = -ENODEV;
> > +               goto out;
> > +       }
> > +
> > +       mgmtdev = devm_kzalloc(&pdev->dev, sizeof(*mgmtdev), GFP_KERNEL);
> > +       if (!mgmtdev) {
> > +               rc = -ENOMEM;
> > +               goto out;
> > +       }
> > +
> > +       mgmtdev->pdev = pdev;
> > +
> > +       pm_runtime_get_sync(&mgmtdev->pdev->dev);
> > +       mgmtdev->dev_addrsize = resource_size(dma_resource);
> > +       mgmtdev->dev_virtaddr = devm_ioremap_resource(&pdev->dev,
> > +                                                       dma_resource);
> > +       if (IS_ERR(mgmtdev->dev_virtaddr)) {
> > +               dev_err(&pdev->dev, "can't map i/o memory at %pa\n",
> > +                       &dma_resource->start);
> > +               rc = -ENOMEM;
> > +               goto out;
> > +       }
> > +
> > +       if (device_property_read_u32(&pdev->dev, "dma-channels",
> > +               &mgmtdev->dma_channels)) {
> > +               dev_err(&pdev->dev, "number of channels missing\n");
> > +               rc = -EINVAL;
> > +               goto out;
> > +       }
> > +
> > +       if (device_property_read_u32(&pdev->dev, "channel-reset-timeout",
> > +               &mgmtdev->chreset_timeout)) {
> > +               dev_err(&pdev->dev, "channel reset timeout missing\n");
> > +               rc = -EINVAL;
> > +               goto out;
> > +       }
> > +
> > +       if (device_property_read_u32(&pdev->dev, "max-write-burst-bytes",
> > +               &mgmtdev->max_write_request)) {
> > +               dev_err(&pdev->dev, "max-write-burst-bytes missing\n");
> > +               rc = -EINVAL;
> > +               goto out;
> > +       }
> > +       if ((mgmtdev->max_write_request != 128) &&
> > +               (mgmtdev->max_write_request != 256) &&
> > +               (mgmtdev->max_write_request != 512) &&
> > +               (mgmtdev->max_write_request != 1024)) {
>
> is_power_of_2()  && min/max ?
ok

> > +               dev_err(&pdev->dev, "invalid write request %d\n",
> > +                       mgmtdev->max_write_request);
> > +               rc = -EINVAL;
> > +               goto out;
> > +       }
> > +
> > +       if (device_property_read_u32(&pdev->dev, "max-read-burst-bytes",
> > +               &mgmtdev->max_read_request)) {
> > +               dev_err(&pdev->dev, "max-read-burst-bytes missing\n");
> > +               rc = -EINVAL;
> > +               goto out;
> > +       }
> > +
> > +       if ((mgmtdev->max_read_request != 128) &&
> > +               (mgmtdev->max_read_request != 256) &&
> > +               (mgmtdev->max_read_request != 512) &&
> > +               (mgmtdev->max_read_request != 1024)) {
>
> Ditto.
done

> > +               dev_err(&pdev->dev, "invalid read request %d\n",
> > +                       mgmtdev->max_read_request);
> > +               rc = -EINVAL;
> > +               goto out;
> > +       }
> > +
> > +       if (device_property_read_u32(&pdev->dev, "max-write-transactions",
> > +               &mgmtdev->max_wr_xactions)) {
> > +               dev_err(&pdev->dev, "max-write-transactions missing\n");
> > +               rc = -EINVAL;
> > +               goto out;
> > +       }
> > +
> > +       if (device_property_read_u32(&pdev->dev, "max-read-transactions",
> > +               &mgmtdev->max_rd_xactions)) {
> > +               dev_err(&pdev->dev, "max-read-transactions missing\n");
> > +               rc = -EINVAL;
> > +               goto out;
> > +       }
> > +
> > +       mgmtdev->priority = devm_kcalloc(&pdev->dev,
> > +               mgmtdev->dma_channels, sizeof(*mgmtdev->priority), GFP_KERNEL);
> > +       if (!mgmtdev->priority) {
> > +               rc = -ENOMEM;
> > +               goto out;
> > +       }
> > +
> > +       mgmtdev->weight = devm_kcalloc(&pdev->dev,
> > +               mgmtdev->dma_channels, sizeof(*mgmtdev->weight), GFP_KERNEL);
> > +       if (!mgmtdev->weight) {
> > +               rc = -ENOMEM;
> > +               goto out;
> > +       }
> > +
> > +       if (device_property_read_u32_array(&pdev->dev, "channel-priority",
> > +                               mgmtdev->priority, mgmtdev->dma_channels)) {
> > +               dev_err(&pdev->dev, "channel-priority missing\n");
> > +               rc = -EINVAL;
> > +               goto out;
> > +       }
> > +
> > +       if (device_property_read_u32_array(&pdev->dev, "channel-weight",
> > +                               mgmtdev->weight, mgmtdev->dma_channels)) {
> > +               dev_err(&pdev->dev, "channel-weight missing\n");
> > +               rc = -EINVAL;
> > +               goto out;
> > +       }
> > +
> > +       for (i = 0; i < mgmtdev->dma_channels; i++) {
> > +               if (mgmtdev->weight[i] > 15) {
>
> 15 is magic.

I created a new macro called MAX_CHANNEL_WEIGHT.

> > +                       dev_err(&pdev->dev,
> > +                               "max value of weight can be 15.\n");
> > +                       rc = -EINVAL;
> > +                       goto out;
> > +               }
> > +
> > +               /* weight needs to be at least one */
> > +               if (mgmtdev->weight[i] == 0)
> > +                       mgmtdev->weight[i] = 1;
> > +       }
> > +
> > +       rc = qcom_hidma_mgmt_setup(mgmtdev);
> > +       if (rc) {
> > +               dev_err(&pdev->dev, "setup failed\n");
> > +               goto out;
> > +       }
> > +
> > +       rc = qcom_hidma_mgmt_debug_init(mgmtdev);
> > +       if (rc) {
> > +               dev_err(&pdev->dev, "debugfs init failed\n");
> > +               goto out;
> > +       }
> > +
> > +       dev_info(&pdev->dev,
> > +               "HI-DMA engine management driver registration complete\n");
>
> You may put some useful information here, otherwise looks like a debug message.
ok, I did now. I copied the syntax from another driver to here.

> > +       platform_set_drvdata(pdev, mgmtdev);
> > +       pm_runtime_mark_last_busy(&mgmtdev->pdev->dev);
> > +       pm_runtime_put_autosuspend(&mgmtdev->pdev->dev);
> > +       return 0;
> > +out:
> > +       pm_runtime_disable(&pdev->dev);
> > +       pm_runtime_put_sync_suspend(&pdev->dev);
> > +       return rc;
> > +}
> > +
> > +static int qcom_hidma_mgmt_remove(struct platform_device *pdev)
> > +{
> > +       struct qcom_hidma_mgmt_dev *mgmtdev = platform_get_drvdata(pdev);
> > +
> > +       pm_runtime_get_sync(&mgmtdev->pdev->dev);
> > +       qcom_hidma_mgmt_debug_uninit(mgmtdev);
> > +       pm_runtime_put_sync_suspend(&pdev->dev);
> > +       pm_runtime_disable(&pdev->dev);
> > +
> > +       dev_info(&pdev->dev, "HI-DMA engine management driver removed\n");
>
> Useless message.

removed .

> > +       return 0;
> > +}
> > +
> > +#if IS_ENABLED(CONFIG_ACPI)
> > +static const struct acpi_device_id qcom_hidma_mgmt_acpi_ids[] = {
> > +       {"QCOM8060"},
> > +       {},
> > +};
> > +#endif
> > +
> > +static const struct of_device_id qcom_hidma_mgmt_match[] = {
> > +       { .compatible = "qcom,hidma-mgmt-1.0", },
> > +       {},
> > +};
> > +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 = "hidma-mgmt",
> > +               .of_match_table = qcom_hidma_mgmt_match,
> > +               .acpi_match_table = ACPI_PTR(qcom_hidma_mgmt_acpi_ids),
> > +       },
> > +};
> > +module_platform_driver(qcom_hidma_mgmt_driver);
> > +MODULE_LICENSE("GPL v2");
> > --
> > 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@xxxxxxxxxxxxxxx
> > More majordomo info at  http://vger.kernel.org/majordomo-info.html
> > Please read the FAQ at  http://www.tux.org/lkml/

--
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@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/