Messages in this thread | | | Date | Fri, 30 Sep 2022 12:37:10 +0530 | Subject | Re: [PATCH V14 2/7] soc: qcom: dcc: Add driver support for Data Capture and Compare unit(DCC) | From | Souradeep Chowdhury <> |
| |
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; >> +}
| |