From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1756167AbbKDArP (ORCPT ); Tue, 3 Nov 2015 19:47:15 -0500 Received: from smtp.codeaurora.org ([198.145.29.96]:57700 "EHLO smtp.codeaurora.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1756107AbbKDArL (ORCPT ); Tue, 3 Nov 2015 19:47:11 -0500 Subject: Re: [PATCH V2 1/3] dma: add Qualcomm Technologies HIDMA management driver To: Andy Shevchenko References: <1446444460-21600-1-git-send-email-okaya@codeaurora.org> <1446444460-21600-2-git-send-email-okaya@codeaurora.org> Cc: dmaengine , timur@codeaurora.org, cov@codeaurora.org, jcm@redhat.com, Rob Herring , Pawel Moll , Mark Rutland , Ian Campbell , Kumar Gala , Vinod Koul , Dan Williams , devicetree , "linux-kernel@vger.kernel.org" From: Sinan Kaya Message-ID: <5639558A.6030308@codeaurora.org> Date: Tue, 3 Nov 2015 19:47:06 -0500 User-Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:38.0) Gecko/20100101 Thunderbird/38.3.0 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On 11/3/2015 5:22 AM, Andy Shevchenko wrote: > On Mon, Nov 2, 2015 at 8:07 AM, Sinan Kaya 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@vger.kernel.org >> 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