On 9/29/2022 9:53 PM, Bjorn Andersson wrote:
On Wed, Sep 28, 2022 at 10:41:12PM +0530, Souradeep Chowdhury wrote:
[..]
diff --git a/drivers/soc/qcom/dcc.c b/drivers/soc/qcom/dcc.c
[..]
+/**
+ * struct dcc_drvdata - configuration information related to a dcc device
+ * @base: Base Address of the dcc device
+ * @dev: The device attached to the driver data
+ * @mutex: Lock to protect access and manipulation of dcc_drvdata
+ * @ram_base: Base address for the SRAM dedicated for the dcc device
+ * @ram_size: Total size of the SRAM dedicated for the dcc device
+ * @ram_offset: Offset to the SRAM dedicated for dcc device
+ * @mem_map_ver: Memory map version of DCC hardware
+ * @ram_cfg: Used for address limit calculation for dcc
+ * @ram_start: Starting address of DCC SRAM
+ * @sram_dev: Miscellaneous device equivalent of dcc SRAM
+ * @cfg_head: Points to the head of the linked list of addresses
+ * @dbg_dir: The dcc debugfs directory under which all the debugfs files are placed
+ * @nr_link_list: Total number of linkedlists supported by the DCC configuration
+ * @loopoff: Loop offset bits range for the addresses
+ * @enable; This contains an array of linkedlist enable flags
+ */
+struct dcc_drvdata {
+ void __iomem *base;
+ void *ram_base;
+ struct device *dev;
+ struct mutex mutex;
+ size_t ram_size;
+ size_t ram_offset;
+ int mem_map_ver;
+ phys_addr_t ram_cfg;
+ phys_addr_t ram_start;
+ struct miscdevice sram_dev;
+ struct list_head *cfg_head;
+ struct dentry *dbg_dir;
+ size_t nr_link_list;
+ u8 loopoff;
+ bool *enable;
It's idiomatic to carry such information in a bitmap, and if
DCC_MAX_LINK_LIST applies to all versions (not obvious from the code
below) then replacing this with just a fixed unsigned long would be a
good move.
Use set_bit(), clear_bit() and test_bit() as needed to access the
individual bits.
Ack
+};
+
+struct dcc_cfg_attr {
+ u32 addr;
+ u32 prev_addr;
+ u32 prev_off;
+ u32 link;
+ u32 sram_offset;
+};
+
+struct dcc_cfg_loop_attr {
+ u32 loop;
+ u32 loop_cnt;
+ u32 loop_len;
+ u32 loop_off;
+ bool loop_start;
+};
+
[..]
+static bool is_dcc_enabled(struct dcc_drvdata *drvdata)
+{
+ int list;
+
+ for (list = 0; list < DCC_MAX_LINK_LIST; list++)
It's possible that there's only dcc->nr_link_list entries in the enable
array.
Ack
+ if (drvdata->enable[list])
+ return true;
+
+ return false;
+}
[..]
+static int dcc_probe(struct platform_device *pdev)
+{
+ u32 val;
+ int ret = 0, i, size;
+ struct device *dev = &pdev->dev;
+ struct dcc_drvdata *dcc;
+ struct resource *res;
+
+ dcc = devm_kzalloc(dev, sizeof(*dcc), GFP_KERNEL);
+ if (!dcc)
+ return -ENOMEM;
+
+ dcc->dev = &pdev->dev;
+ platform_set_drvdata(pdev, dcc);
+
+ dcc->base = devm_platform_ioremap_resource(pdev, 0);
+ if (IS_ERR(dcc->base))
+ return PTR_ERR(dcc->base);
+
+ res = platform_get_resource(pdev, IORESOURCE_MEM, 1);
+ if (!res)
+ return -ENODEV;
+
+ dcc->ram_base = memremap(res->start, resource_size(res), MEMREMAP_WB);
+ if (!dcc->ram_base)
+ return -ENODEV;
+
+ dcc->ram_size = resource_size(res);
+
+ dcc->ram_offset = (size_t)of_device_get_match_data(&pdev->dev);
+
+ val = dcc_readl(dcc, DCC_HW_INFO);
+
+ if (FIELD_GET(DCC_VER_INFO_MASK, val)) {
+ dcc->mem_map_ver = 3;
+ dcc->nr_link_list = dcc_readl(dcc, DCC_LL_NUM_INFO);
+ if (dcc->nr_link_list == 0)
+ return -EINVAL;
+ } else if ((val & DCC_VER_MASK2) == DCC_VER_MASK2) {
+ dcc->mem_map_ver = 2;
+ dcc->nr_link_list = dcc_readl(dcc, DCC_LL_NUM_INFO);
+ if (dcc->nr_link_list == 0)
+ return -EINVAL;
+ } else {
+ dcc->mem_map_ver = 1;
+ dcc->nr_link_list = DCC_MAX_LINK_LIST;
+ }
+
+ /* Either set the fixed loop offset or calculate it
+ * from ram_size. Max consecutive addresses the
+ * dcc can loop is equivalent to the ram size
+ */
+ if (val & DCC_LOOP_OFFSET_MASK)
+ dcc->loopoff = DCC_FIX_LOOP_OFFSET;
+ else
+ dcc->loopoff = get_bitmask_order((dcc->ram_size +
+ dcc->ram_offset) / 4 - 1);
+
+ mutex_init(&dcc->mutex);
+ /* Allocate space for all entries at once */
+ size = sizeof(*dcc->enable) + sizeof(*dcc->cfg_head);
+
+ dcc->enable = devm_kcalloc(dev, dcc->nr_link_list, size, GFP_KERNEL);
+ if (!dcc->enable)
+ return -ENOMEM;
+
+ dcc->cfg_head = (struct list_head *)(dcc->enable + dcc->nr_link_list);
Turning enable into a unsigned long bitmap, will clean this stuff up as
well, as you won't have the need/urge to allocate the two components at
once and then do pointer math on them...
Ack
Regards,
Bjorn
+
+ for (i = 0; i < dcc->nr_link_list; i++)
+ INIT_LIST_HEAD(&dcc->cfg_head[i]);
+
+ ret = dcc_sram_dev_init(dcc);
+ if (ret) {
+ dev_err(dcc->dev, "DCC: sram node not registered.\n");
+ return ret;
+ }
+
+ ret = dcc_create_debug_dir(dcc);
+ if (ret) {
+ dev_err(dcc->dev, "DCC: debugfs files not created.\n");
+ dcc_sram_dev_exit(dcc);
+ return ret;
+ }
+
+ return 0;
+}
