On Sun, Nov 22, 2015 at 6:37 PM, Sinan Kaya [off-list ref] 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.

1. HIDMA Management driver
2. HIDMA Channel driver

Each HIDMA HW consists of multiple channels. These channels
share some set of common parameters. These parameters are
initialized by the management driver during power up.
Same management driver is used for monitoring the execution
of the channels. Management driver can change the performance
behavior dynamically such as bandwidth allocation and
prioritization.

The management driver is executed in hypervisor context and
is the main management entity for all channels provided by
the device.

+What:          /sys/devices/platform/hidma-mgmt*/channel*_weight
+               /sys/devices/platform/QCOM8060:*/channel*__weight

Extra _ ?

+Each HIDMA HW consists of multiple channels. These channels
+share some set of common parameters. These parameters are
+initialized by the management driver during power up.
+Same management driver is used for monitoring the execution
+of the channels. Management driver can change the performance
+behavior dynamically such as bandwidth allocation and
+prioritization.
+
+All channel devices get probed in the hypervisor
+context during power up. They show up as DMA engine
+DMA channels. Then, before starting the virtualization; each
+channel device is unbound from the hypervisor by VFIO
+and assign 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.

Hmm? Can lines be longer up to 76-78 characters?

+#define MAX_WR_XACTIONS_MASK           0x1F
+#define MAX_RD_XACTIONS_MASK           0x1F
+#define WEIGHT_MASK                    0x7F
+#define MAX_BUS_REQ_LEN_MASK           0xFFFF
+#define CHRESET_TIMEOUUT_MASK          0xFFFFF

GENMASK?

+int hidma_mgmt_setup(struct hidma_mgmt_dev *mgmtdev)
+{
+       unsigned int i;
+       u32 val;
+
+       if (!is_power_of_2(mgmtdev->max_write_request) ||
+               (mgmtdev->max_write_request < 128) ||

Someone likes parens.
I might agree with these cases, but below in assignments and combined
operations the parens are indeed redundant.

+               (mgmtdev->max_write_request > 1024)) {
+               dev_err(&mgmtdev->pdev->dev, "invalid write request %d\n",
+                       mgmtdev->max_write_request);
+               return -EINVAL;
+       }
+
+       if (!is_power_of_2(mgmtdev->max_read_request) ||
+               (mgmtdev->max_read_request < 128) ||
+               (mgmtdev->max_read_request > 1024)) {

Ditto.

+               dev_err(&mgmtdev->pdev->dev, "invalid read request %d\n",
+                       mgmtdev->max_read_request);
+               return  -EINVAL;
+       }
+
+       if (mgmtdev->max_wr_xactions > MAX_WR_XACTIONS_MASK) {
+               dev_err(&mgmtdev->pdev->dev,
+                       "max_wr_xactions cannot be bigger than %d\n",
+                       MAX_WR_XACTIONS_MASK);
+               return -EINVAL;
+       }
+
+       if (mgmtdev->max_rd_xactions > MAX_RD_XACTIONS_MASK) {
+               dev_err(&mgmtdev->pdev->dev,
+                       "max_rd_xactions cannot be bigger than %d\n",
+                       MAX_RD_XACTIONS_MASK);
+               return -EINVAL;
+       }
+
+       for (i = 0; i < mgmtdev->dma_channels; i++) {
+               if (mgmtdev->priority[i] > 1) {
+                       dev_err(&mgmtdev->pdev->dev, "priority can be 0 or 1\n");
+                       return -EINVAL;
+               }
+
+               if (mgmtdev->weight[i] > MAX_CHANNEL_WEIGHT) {
+                       dev_err(&mgmtdev->pdev->dev,
+                               "max value of weight can be %d.\n",
+                               MAX_CHANNEL_WEIGHT);
+                       return -EINVAL;
+               }
+
+               /* weight needs to be at least one */
+               if (mgmtdev->weight[i] == 0)
+                       mgmtdev->weight[i] = 1;
+       }
+
+       pm_runtime_get_sync(&mgmtdev->pdev->dev);
+       val = readl(mgmtdev->virtaddr + MAX_BUS_REQ_LEN_OFFSET);
+       val &= ~(MAX_BUS_REQ_LEN_MASK << MAX_BUS_WR_REQ_BIT_POS);

+       val |= (mgmtdev->max_write_request << MAX_BUS_WR_REQ_BIT_POS);

Ditto.

+       val &= ~(MAX_BUS_REQ_LEN_MASK);

Ditto.

+       val |= (mgmtdev->max_read_request);

Ditto.

+       writel(val, mgmtdev->virtaddr + MAX_BUS_REQ_LEN_OFFSET);
+
+       val = readl(mgmtdev->virtaddr + MAX_XACTIONS_OFFSET);
+       val &= ~(MAX_WR_XACTIONS_MASK << MAX_WR_XACTIONS_BIT_POS);

+       val |= (mgmtdev->max_wr_xactions << MAX_WR_XACTIONS_BIT_POS);
+       val &= ~(MAX_RD_XACTIONS_MASK);
+       val |= (mgmtdev->max_rd_xactions);

Ditto for all 3 above.

+       writel(val, mgmtdev->virtaddr + MAX_XACTIONS_OFFSET);
+
+       mgmtdev->hw_version = readl(mgmtdev->virtaddr + HW_VERSION_OFFSET);
+       mgmtdev->hw_version_major = (mgmtdev->hw_version >> 28) & 0xF;
+       mgmtdev->hw_version_minor = (mgmtdev->hw_version >> 16) & 0xF;
+
+       for (i = 0; i < mgmtdev->dma_channels; i++) {
+               val = readl(mgmtdev->virtaddr + QOS_N_OFFSET + (4 * i));

Ditto.

+               val &= ~(1 << PRIORITY_BIT_POS);
+               val |= ((mgmtdev->priority[i] & 0x1) << PRIORITY_BIT_POS);
+               val &= ~(WEIGHT_MASK << WRR_BIT_POS);
+               val |= ((mgmtdev->weight[i] & WEIGHT_MASK) << WRR_BIT_POS);
+               writel(val, mgmtdev->virtaddr + QOS_N_OFFSET + (4 * i));

Ditto.

+       }
+
+       val = readl(mgmtdev->virtaddr + CHRESET_TIMEOUT_OFFSET);
+       val &= ~CHRESET_TIMEOUUT_MASK;

Look here, you use it like it intuitively feels.

+       val |= (mgmtdev->chreset_timeout_cycles & CHRESET_TIMEOUUT_MASK);

But here?

+       writel(val, mgmtdev->virtaddr + CHRESET_TIMEOUT_OFFSET);
+
+       pm_runtime_mark_last_busy(&mgmtdev->pdev->dev);
+       pm_runtime_put_autosuspend(&mgmtdev->pdev->dev);
+       return 0;
+}
+EXPORT_SYMBOL_GPL(hidma_mgmt_setup);
+
+static int hidma_mgmt_probe(struct platform_device *pdev)
+{
+       struct hidma_mgmt_dev *mgmtdev;
+       struct resource *res;
+       void __iomem *virtaddr;
+       int irq;
+       int rc;
+       u32 val;
+
+       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);
+       pm_runtime_get_sync(&pdev->dev);

+empty line

+       res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+       virtaddr = devm_ioremap_resource(&pdev->dev, res);
+       if (IS_ERR(virtaddr)) {
+               rc = -ENOMEM;
+               goto out;
+       }
+
+       irq = platform_get_irq(pdev, 0);
+       if (irq < 0) {
+               dev_err(&pdev->dev, "irq resources not found\n");
+               rc = irq;
+               goto out;
+       }
+
+       mgmtdev = devm_kzalloc(&pdev->dev, sizeof(*mgmtdev), GFP_KERNEL);
+       if (!mgmtdev) {
+               rc = -ENOMEM;
+               goto out;
+       }
+
+       mgmtdev->pdev = pdev;
+       mgmtdev->addrsize = resource_size(res);
+       mgmtdev->virtaddr = virtaddr;
+
+       rc = device_property_read_u32(&pdev->dev, "dma-channels",
+                               &mgmtdev->dma_channels);
+       if (rc) {
+               dev_err(&pdev->dev, "number of channels missing\n");
+               goto out;
+       }
+
+       rc = device_property_read_u32(&pdev->dev,
+                               "channel-reset-timeout-cycles",
+                               &mgmtdev->chreset_timeout_cycles);
+       if (rc) {
+               dev_err(&pdev->dev, "channel reset timeout missing\n");
+               goto out;
+       }
+
+       rc = device_property_read_u32(&pdev->dev, "max-write-burst-bytes",
+                               &mgmtdev->max_write_request);
+       if (rc) {
+               dev_err(&pdev->dev, "max-write-burst-bytes missing\n");
+               goto out;
+       }
+
+       rc = device_property_read_u32(&pdev->dev, "max-read-burst-bytes",
+                               &mgmtdev->max_read_request);
+       if (rc) {
+               dev_err(&pdev->dev, "max-read-burst-bytes missing\n");
+               goto out;
+       }
+
+       rc = device_property_read_u32(&pdev->dev, "max-write-transactions",
+                               &mgmtdev->max_wr_xactions);
+       if (rc) {
+               dev_err(&pdev->dev, "max-write-transactions missing\n");
+               goto out;
+       }
+
+       rc = device_property_read_u32(&pdev->dev, "max-read-transactions",
+                               &mgmtdev->max_rd_xactions);
+       if (rc) {
+               dev_err(&pdev->dev, "max-read-transactions missing\n");
+               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;
+       }
+
+       rc = device_property_read_u32_array(&pdev->dev, "channel-priority",
+                               mgmtdev->priority, mgmtdev->dma_channels);
+       if (rc) {
+               dev_err(&pdev->dev, "channel-priority missing\n");
+               goto out;
+       }
+
+       rc = device_property_read_u32_array(&pdev->dev, "channel-weight",
+                               mgmtdev->weight, mgmtdev->dma_channels);
+       if (rc) {
+               dev_err(&pdev->dev, "channel-weight missing\n");
+               goto out;
+       }
+
+       rc = hidma_mgmt_setup(mgmtdev);
+       if (rc) {
+               dev_err(&pdev->dev, "setup failed\n");
+               goto out;
+       }
+
+       /* start the HW */
+       val = readl(mgmtdev->virtaddr + CFG_OFFSET);
+       val |= 1;
+       writel(val, mgmtdev->virtaddr + CFG_OFFSET);
+
+       rc = hidma_mgmt_init_sys(mgmtdev);
+       if (rc) {
+               dev_err(&pdev->dev, "sysfs setup failed\n");
+               goto out;
+       }
+
+       dev_info(&pdev->dev,
+                "HW rev: %d.%d @ %pa with %d physical channels\n",
+                mgmtdev->hw_version_major, mgmtdev->hw_version_minor,
+                &res->start, mgmtdev->dma_channels);
+
+       platform_set_drvdata(pdev, mgmtdev);
+       pm_runtime_mark_last_busy(&pdev->dev);
+       pm_runtime_put_autosuspend(&pdev->dev);
+       return 0;

+out:
+       pm_runtime_disable(&pdev->dev);
+       pm_runtime_put_sync_suspend(&pdev->dev);

I'm not sure the sequence is logically correct, though it might work.

Perhaps hidma_mgmt_files ?

+       DECLARE_ATTRIBUTE(hw_version_major, S_IRUGO),
+       DECLARE_ATTRIBUTE(hw_version_minor, S_IRUGO),
+       DECLARE_ATTRIBUTE(dma_channels, S_IRUGO),
+       DECLARE_ATTRIBUTE(chreset_timeout_cycles, S_IRUGO),
+       DECLARE_ATTRIBUTE(max_wr_xactions, (S_IRUGO|S_IWUGO)),
+       DECLARE_ATTRIBUTE(max_rd_xactions, (S_IRUGO|S_IWUGO)),
+       DECLARE_ATTRIBUTE(max_write_request, (S_IRUGO|S_IWUGO)),
+       DECLARE_ATTRIBUTE(max_read_request, (S_IRUGO|S_IWUGO)),
+};
+
+static ssize_t show_values(struct device *dev, struct device_attribute *attr,
+                               char *buf)
+{
+       struct platform_device *pdev = to_platform_device(dev);
+       struct hidma_mgmt_dev *mdev = platform_get_drvdata(pdev);
+       unsigned int i;
+
+       for (i = 0; i < ARRAY_SIZE(files); i++) {
+               if (strcmp(attr->attr.name, files[i].name) == 0) {
+                       sprintf(buf, "%d\n", files[i].get(mdev));
+                       goto done;
+               }
+       }
+
+       for (i = 0; i < mdev->dma_channels; i++) {
+               char name[30];
+
+               sprintf(name, "channel%d_priority", i);
+               if (strcmp(attr->attr.name, name) == 0) {
+                       sprintf(buf, "%d\n", mdev->priority[i]);
+                       goto done;
+               }
+
+               sprintf(name, "channel%d_weight", i);
+               if (strcmp(attr->attr.name, name) == 0) {
+                       sprintf(buf, "%d\n", mdev->weight[i]);
+                       goto done;
+               }
+       }
+
+done:
+       return strlen(buf);

buf might be uninitialized here. Have you got any warning from compiler?

+}
+
+static ssize_t set_values(struct device *dev, struct device_attribute *attr,
+                               const char *buf, size_t count)
+{
+       struct platform_device *pdev = to_platform_device(dev);
+       struct hidma_mgmt_dev *mdev = platform_get_drvdata(pdev);
+       unsigned long tmp;
+       unsigned int i;
+       int rc;
+
+       rc = kstrtoul(buf, 0, &tmp);
+       if (rc)
+               return rc;
+
+       for (i = 0; i < ARRAY_SIZE(files); i++) {
+               if (strcmp(attr->attr.name, files[i].name) == 0) {
+                       rc = files[i].set(mdev, tmp);
+                       if (rc)
+                               return rc;
+
+                       goto done;
+               }
+       }
+
+       for (i = 0; i < mdev->dma_channels; i++) {
+               char name[30];
+
+               sprintf(name, "channel%d_priority", i);
+               if (strcmp(attr->attr.name, name) == 0) {
+                       rc = set_priority(mdev, i, tmp);
+                       if (rc)
+                               return rc;
+                       goto done;
+               }
+
+               sprintf(name, "channel%d_weight", i);
+               if (strcmp(attr->attr.name, name) == 0) {
+                       rc = set_weight(mdev, i, tmp);
+                       if (rc)
+                               return rc;
+               }
+       }
+done:
+       return count;
+}
+
+static int create_sysfs_entry(struct hidma_mgmt_dev *dev, char *name, int mode)
+{
+       struct device_attribute *attrs;
+       char *name_copy;
+
+       attrs = devm_kmalloc(&dev->pdev->dev,
+                       sizeof(struct device_attribute), GFP_KERNEL);
+       if (!attrs)
+               return -ENOMEM;
+
+       name_copy = devm_kstrdup(&dev->pdev->dev, name, GFP_KERNEL);
+       if (!name_copy)
+               return -ENOMEM;
+
+       attrs->attr.name = name_copy;
+       attrs->attr.mode = mode;
+       attrs->show      = show_values;
+       attrs->store     = set_values;
+       sysfs_attr_init(&attrs->attr);
+
+       return device_create_file(&dev->pdev->dev, attrs);
+}
+
+

Extra empty line.

+int hidma_mgmt_init_sys(struct hidma_mgmt_dev *dev)
+{
+       unsigned int i;
+       int rc;
+
+       for (i = 0; i < ARRAY_SIZE(files); i++) {
+               rc = create_sysfs_entry(dev, files[i].name, files[i].mode);
+               if (rc)
+                       return rc;
+       }
+

Can it be like

/sys/?/DEVICExx/
 channelYY/
 attr1
 attr2
 ?

?

I think it will be easier to handle in code and from user. (Similar
way DMAEngine API does for slave DMA devices)

+       for (i = 0; i < dev->dma_channels; i++) {
+               char name[30];
+
+               sprintf(name, "channel%d_priority", i);
+               rc = create_sysfs_entry(dev, name, (S_IRUGO|S_IWUGO));
+               if (rc)
+                       return rc;
+
+               sprintf(name, "channel%d_weight", i);
+               rc = create_sysfs_entry(dev, name, (S_IRUGO|S_IWUGO));
+               if (rc)
+                       return rc;
+       }
+
+       return 0;
+}
+EXPORT_SYMBOL_GPL(hidma_mgmt_init_sys);

-- 
With Best Regards,
Andy Shevchenko