Re: [PATCH V14 2/7] soc: qcom: dcc: Add driver support for Data Capture and Compare unit(DCC)

[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;
> > +}