Message-ID: <22002c0e-ec4e-187a-69ce-d2dac9b554bb@gmail.com>
Date:   Mon, 13 Jul 2020 13:16:37 +0200
From:   Matthias Brugger <matthias.bgg@...il.com>
To:     Neal Liu <neal.liu@...iatek.com>
Cc:     Rob Herring <robh+dt@...nel.org>, devicetree@...r.kernel.org,
        linux-arm-kernel@...ts.infradead.org,
        linux-mediatek@...ts.infradead.org,
        lkml <linux-kernel@...r.kernel.org>, wsd_upstream@...iatek.com
Subject: Re: [PATCH v2 2/2] soc: mediatek: add mtk-devapc driver



On 13/07/2020 09:45, Neal Liu wrote:
> On Fri, 2020-07-10 at 14:14 +0200, Matthias Brugger wrote:
>>
> [snip]
>>> +
>>> +static int get_vio_slave_num(int slave_type)
>>
>> I have a hard time to understand the usefullness of this, can you please explain.
>>
> 
> The basic idea is to get total numbers of slaves. And we can use it to
> scan all slaves which has been triggered violation.
> I think I can pass it through DT data instead of using mtk_device_info
> array. I'll send another patches to change it.
> 
>>> +{
>>> +	if (slave_type == 0)
>>> +		return ARRAY_SIZE(mtk_devices_infra);
>>> +
>>> +	return 0;
>>> +}
>>> +
>>> +static u32 get_shift_group(struct mtk_devapc_context *devapc_ctx,
>>> +			   int slave_type, int vio_idx)
>>> +{
>>> +	u32 vio_shift_sta;
>>> +	void __iomem *reg;
>>> +	int bit;
>>> +
>>> +	reg = mtk_devapc_pd_get(devapc_ctx, slave_type, VIO_SHIFT_STA, 0);
>>> +	vio_shift_sta = readl(reg);
>>> +
>>> +	for (bit = 0; bit < 32; bit++) {
>>> +		if ((vio_shift_sta >> bit) & 0x1) > +			break;
>>> +	}
>>> +
>>> +	return bit;
>>
>> We return the first position (from the right) of the rigster with the bit set to
>> one. Correct?
>> Can't we use __ffs() for this?
> 
> Yes, thanks for your reminds to use __ffs().
> I'll revise it in next patches.
> 
>>
>>> +}
>>> +
>>> +static int check_vio_mask_sta(struct mtk_devapc_context *devapc_ctx,
>>> +			      int slave_type, u32 module, int pd_reg_type)
>>> +{
>>> +	u32 reg_index, reg_offset;
>>> +	void __iomem *reg;
>>> +	u32 value;
>>> +
>>> +	VIO_MASK_STA_REG_GET(module);
>>> +
>>> +	reg = mtk_devapc_pd_get(devapc_ctx, slave_type, pd_reg_type, reg_index);
>>
>> reg = mtk_devapc_pd_get(devapc_ctx, slave_type, pd_reg_type,
>> VIO_MOD_TO_REG_IND(module));
> 
> Okay, I'll revise it in next patches.
> 
>>
>>> +	value = readl(reg);
>>> +
>>> +	return ((value >> reg_offset) & 0x1);
>>
>> return ((value >> VIO_MOD_TO_REG_OFF(module)) & 0x1);
> 
> Okay, I'll revise it in next patches.
> 
>>
>>> +}
>>> +
>>> +static int check_vio_mask(struct mtk_devapc_context *devapc_ctx, int slave_type,
>>> +			  u32 module)
>>> +{
>>> +	return check_vio_mask_sta(devapc_ctx, slave_type, module, VIO_MASK);
>>> +}
>>> +
>>> +static int check_vio_status(struct mtk_devapc_context *devapc_ctx,
>>> +			    int slave_type, u32 module)
>>> +{
>>> +	return check_vio_mask_sta(devapc_ctx, slave_type, module, VIO_STA);
>>> +}
>>> +
>>> +static void clear_vio_status(struct mtk_devapc_context *devapc_ctx,
>>> +			     int slave_type, u32 module)
>>> +{
>>> +	u32 reg_index, reg_offset;
>>> +	void __iomem *reg;
>>> +
>>> +	VIO_MASK_STA_REG_GET(module);
>>> +
>>> +	reg = mtk_devapc_pd_get(devapc_ctx, slave_type, VIO_STA, reg_index);
>>> +	writel(0x1 << reg_offset, reg);
>>> +
>>> +	if (check_vio_status(devapc_ctx, slave_type, module))
>>> +		pr_err(PFX "%s: Clear failed, slave_type:0x%x, module_index:0x%x\n",
>>> +		       __func__, slave_type, module);
>>> +}
>>> +
>>> +static void mask_module_irq(struct mtk_devapc_context *devapc_ctx,
>>> +			    int slave_type, u32 module, bool mask)
>>> +{
>>> +	u32 reg_index, reg_offset;
>>> +	void __iomem *reg;
>>> +	u32 value;
>>> +
>>> +	VIO_MASK_STA_REG_GET(module);
>>> +
>>> +	reg = mtk_devapc_pd_get(devapc_ctx, slave_type, VIO_MASK, reg_index);
>>> +
>>> +	value = readl(reg);
>>> +	if (mask)
>>> +		value |= (0x1 << reg_offset);
>>> +	else
>>> +		value &= ~(0x1 << reg_offset);
>>> +
>>> +	writel(value, reg);
>>> +}
>>> +
>>> +#define TIMEOUT_MS		10000
>>> +
>>> +static int read_poll_timeout(void __iomem *addr, u32 mask)
>>
>> That function is defined in include/linux/iopoll.h
>>
>>> +{
>>> +	unsigned long timeout = jiffies + msecs_to_jiffies(TIMEOUT_MS);
>>> +
>>> +	do {
>>> +		if (readl_relaxed(addr) & mask)
>>
>> Please use a variable where you write your value to and then check for the mask.
>> That maks the code easier to read and I think is part of the coding style.
>>
> 
> Okay, I'll use the function in iopoll.h instead.
> Thanks for your reminds.
> 
>>> +			return 0;
>>> +
>>> +	} while (!time_after(jiffies, timeout));
>>> +
>>> +	return (readl_relaxed(addr) & mask) ? 0 : -ETIMEDOUT;
>>> +}
>>> +
>>> +/*
>>> + * sync_vio_dbg - start to get violation information by selecting violation
>>> + *		  group and enable violation shift.
>>> + *
>>> + * Returns sync done or not
>>> + */
>>> +static u32 sync_vio_dbg(struct mtk_devapc_context *devapc_ctx, int slave_type,
>>> +			u32 shift_bit)
>>> +{
>>> +	void __iomem *pd_vio_shift_sta_reg;
>>> +	void __iomem *pd_vio_shift_sel_reg;
>>> +	void __iomem *pd_vio_shift_con_reg;
>>> +	u32 sync_done = 0;
>>> +
>>> +	pd_vio_shift_sta_reg = mtk_devapc_pd_get(devapc_ctx, slave_type,
>>> +						 VIO_SHIFT_STA, 0);
>>> +	pd_vio_shift_sel_reg = mtk_devapc_pd_get(devapc_ctx, slave_type,
>>> +						 VIO_SHIFT_SEL, 0);
>>> +	pd_vio_shift_con_reg = mtk_devapc_pd_get(devapc_ctx, slave_type,
>>> +						 VIO_SHIFT_CON, 0);
>>> +
>>> +	writel(0x1 << shift_bit, pd_vio_shift_sel_reg);
>>> +	writel(0x1, pd_vio_shift_con_reg);
>>> +
>>> +	if (!read_poll_timeout(pd_vio_shift_con_reg, 0x2))
>>> +		sync_done = 1;
>>> +	else
>>> +		pr_err(PFX "%s: Shift violation info failed\n", __func__);
>>> +
>>> +	/* Disable shift mechanism */
>>
>> Please add a comment explaining what the shift mechanism is about.
> 
> Okay, I'll add a comment to explain it at the beginning of this
> function.
> 
>>
>>> +	writel(0x0, pd_vio_shift_con_reg);
>>> +	writel(0x0, pd_vio_shift_sel_reg);
>>> +	writel(0x1 << shift_bit, pd_vio_shift_sta_reg);
>>> +
>>> +	return sync_done;
>>> +}
>>> +
>>> +static void devapc_vio_info_print(struct mtk_devapc_context *devapc_ctx)
>>> +{
>>> +	struct mtk_devapc_vio_info *vio_info = devapc_ctx->vio_info;
>>> +
>>> +	/* Print violation information */
>>> +	if (vio_info->write)
>>> +		pr_info(PFX "Write Violation\n");
>>> +	else if (vio_info->read)
>>> +		pr_info(PFX "Read Violation\n");
>>> +
>>> +	pr_info(PFX "%s%x, %s%x, %s%x, %s%x\n",
>>> +		"Vio Addr:0x", vio_info->vio_addr,
>>> +		"High:0x", vio_info->vio_addr_high,
>>> +		"Bus ID:0x", vio_info->master_id,
>>> +		"Dom ID:0x", vio_info->domain_id);
>>> +}
>>> +
>>> +static void devapc_extract_vio_dbg(struct mtk_devapc_context *devapc_ctx,
>>> +				   int slave_type)
>>> +{
>>> +	void __iomem *vio_dbg0_reg, *vio_dbg1_reg;
>>> +	struct mtk_devapc_vio_dbgs_desc *vio_dbgs;
>>> +	struct mtk_devapc_vio_info *vio_info;
>>> +	u32 dbg0;
>>> +
>>> +	vio_dbg0_reg = mtk_devapc_pd_get(devapc_ctx, slave_type, VIO_DBG0, 0);
>>> +	vio_dbg1_reg = mtk_devapc_pd_get(devapc_ctx, slave_type, VIO_DBG1, 0);
>>> +
>>> +	vio_dbgs = devapc_ctx->vio_dbgs_desc;
>>> +	vio_info = devapc_ctx->vio_info;
>>> +
>>> +	/* Extract violation information */
>>> +	dbg0 = readl(vio_dbg0_reg);
>>> +	vio_info->vio_addr = readl(vio_dbg1_reg);
>>> +
>>> +	vio_info->master_id = (dbg0 & vio_dbgs[MSTID].mask) >>
>>> +			      vio_dbgs[MSTID].start_bit;
>>> +	vio_info->domain_id = (dbg0 & vio_dbgs[DMNID].mask) >>
>>> +			      vio_dbgs[DMNID].start_bit;
>>> +	vio_info->write = ((dbg0 & vio_dbgs[VIO_W].mask) >>
>>> +			   vio_dbgs[VIO_W].start_bit) == 1;
>>> +	vio_info->read = ((dbg0 & vio_dbgs[VIO_R].mask) >>
>>> +			  vio_dbgs[VIO_R].start_bit) == 1;
>>> +	vio_info->vio_addr_high = (dbg0 & vio_dbgs[ADDR_H].mask) >>
>>> +				  vio_dbgs[ADDR_H].start_bit;
>>> +
>>> +	devapc_vio_info_print(devapc_ctx);
>>> +}
>>> +
>>> +/*
>>> + * mtk_devapc_dump_vio_dbg - shift & dump the violation debug information.
>>> + */
>>> +static bool mtk_devapc_dump_vio_dbg(struct mtk_devapc_context *devapc_ctx,
>>> +				    int slave_type, int *vio_idx)
>>> +{
>>> +	const struct mtk_device_info **device_info;
>>> +	u32 shift_bit;
>>> +	int i;
>>> +
>>> +	device_info = devapc_ctx->device_info;
>>> +
>>> +	for (i = 0; i < get_vio_slave_num(slave_type); i++) {
>>> +		*vio_idx = device_info[slave_type][i].vio_index;
>>> +
>>> +		if (check_vio_mask(devapc_ctx, slave_type, *vio_idx))
>>> +			continue;
>>> +
>>> +		if (!check_vio_status(devapc_ctx, slave_type, *vio_idx))
>>> +			continue;
>>> +
>>> +		shift_bit = get_shift_group(devapc_ctx, slave_type, *vio_idx);
>>> +
>>> +		if (!sync_vio_dbg(devapc_ctx, slave_type, shift_bit))
>>> +			continue;
>>> +
>>> +		devapc_extract_vio_dbg(devapc_ctx, slave_type);
>>> +
>>> +		return true;
>>> +	}
>>> +
>>> +	return false;
>>> +}
>>> +
>>> +/*
>>> + * devapc_violation_irq - the devapc Interrupt Service Routine (ISR) will dump
>>> + *			  violation information including which master violates
>>> + *			  access slave.
>>> + */
>>> +static irqreturn_t devapc_violation_irq(int irq_number,
>>> +					struct mtk_devapc_context *devapc_ctx)
>>> +{
>>> +	const struct mtk_device_info **device_info;
>>> +	int slave_type_num;
>>> +	int vio_idx = -1;
>>> +	int slave_type;
>>> +
>>> +	slave_type_num = devapc_ctx->slave_type_num;
>>> +	device_info = devapc_ctx->device_info;
>>> +
>>> +	for (slave_type = 0; slave_type < slave_type_num; slave_type++) {
>>> +		if (!mtk_devapc_dump_vio_dbg(devapc_ctx, slave_type, &vio_idx))
>>> +			continue;
>>> +
>>> +		/* Ensure that violation info are written before
>>> +		 * further operations
>>> +		 */
>>> +		smp_mb();
>>> +
>>> +		mask_module_irq(devapc_ctx, slave_type, vio_idx, true);
>>> +
>>> +		clear_vio_status(devapc_ctx, slave_type, vio_idx);
>>> +
>>> +		mask_module_irq(devapc_ctx, slave_type, vio_idx, false);
>>> +	}
>>> +
>>> +	return IRQ_HANDLED;
>>> +}
>>> +
>>> +/*
>>> + * start_devapc - initialize devapc status and start receiving interrupt
>>> + *		  while devapc violation is triggered.
>>> + */
>>> +static void start_devapc(struct mtk_devapc_context *devapc_ctx)
>>> +{
>>> +	const struct mtk_device_info **device_info;
>>> +	void __iomem *pd_vio_shift_sta_reg;
>>> +	void __iomem *pd_apc_con_reg;
>>> +	u32 vio_shift_sta;
>>> +	int slave_type, slave_type_num;
>>> +	int i, vio_idx;
>>> +
>>> +	device_info = devapc_ctx->device_info;
>>> +	slave_type_num = devapc_ctx->slave_type_num;
>>> +
>>> +	for (slave_type = 0; slave_type < slave_type_num; slave_type++) {
>>> +		pd_apc_con_reg = mtk_devapc_pd_get(devapc_ctx, slave_type,
>>> +						   APC_CON, 0);
>>> +		pd_vio_shift_sta_reg = mtk_devapc_pd_get(devapc_ctx, slave_type,
>>> +							 VIO_SHIFT_STA, 0);
>>> +		if (!pd_apc_con_reg || !pd_vio_shift_sta_reg)
>>> +			return;
>>> +
>>> +		/* Clear devapc violation status */
>>> +		writel(BIT(31), pd_apc_con_reg);
>>> +
>>> +		/* Clear violation shift status */
>>> +		vio_shift_sta = readl(pd_vio_shift_sta_reg);
>>> +		if (vio_shift_sta)
>>> +			writel(vio_shift_sta, pd_vio_shift_sta_reg);
>>> +
>>> +		/* Clear slave violation status */
>>> +		for (i = 0; i < get_vio_slave_num(slave_type); i++) {
>>> +			vio_idx = device_info[slave_type][i].vio_index;
>>> +
>>> +			clear_vio_status(devapc_ctx, slave_type, vio_idx);
>>> +
>>> +			mask_module_irq(devapc_ctx, slave_type, vio_idx, false);
>>> +		}
>>> +	}
>>> +}
>>> +
>>> +static int mtk_devapc_probe(struct platform_device *pdev)
>>> +{
>>> +	struct device_node *node = pdev->dev.of_node;
>>> +	struct mtk_devapc_context *devapc_ctx;
>>> +	struct clk *devapc_infra_clk;
>>> +	u32 vio_dbgs_num, pds_num;
>>> +	u8 slave_type_num;
>>> +	u32 devapc_irq;
>>> +	size_t size;
>>> +	int i, ret;
>>> +
>>> +	if (IS_ERR(node))
>>> +		return -ENODEV;
>>> +
>>> +	devapc_ctx = devm_kzalloc(&pdev->dev, sizeof(struct mtk_devapc_context),
>>> +				  GFP_KERNEL);
>>> +	if (!devapc_ctx)
>>> +		return -ENOMEM;
>>> +
>>> +	if (of_property_read_u8(node, "mediatek-slv_type_num", &slave_type_num))
>>> +		return -ENXIO;
>>> +
>>> +	devapc_ctx->slave_type_num = slave_type_num;
>>> +
>>> +	size = slave_type_num * sizeof(void *);
>>> +	devapc_ctx->devapc_pd_base = devm_kzalloc(&pdev->dev, size, GFP_KERNEL);
>>> +	if (!devapc_ctx->devapc_pd_base)
>>> +		return -ENOMEM;
>>> +
>>> +	size = slave_type_num * sizeof(struct mtk_device_info *);
>>> +	devapc_ctx->device_info = devm_kzalloc(&pdev->dev, size, GFP_KERNEL);
>>> +	if (!devapc_ctx->device_info)
>>> +		return -ENOMEM;
>>> +
>>> +	for (i = 0; i < slave_type_num; i++) {
>>> +		devapc_ctx->devapc_pd_base[i] = of_iomap(node, i);
>>> +		if (!devapc_ctx->devapc_pd_base[i])
>>> +			return -EINVAL;
>>> +
>>> +		if (i == 0)
>>> +			devapc_ctx->device_info[i] = mtk_devices_infra;
>>> +	}
>>> +
>>> +	size = sizeof(struct mtk_devapc_vio_info);
>>> +	devapc_ctx->vio_info = devm_kzalloc(&pdev->dev, size, GFP_KERNEL);
>>> +	if (!devapc_ctx->vio_info)
>>> +		return -ENOMEM;
>>> +
>>> +	vio_dbgs_num = of_property_count_u32_elems(node, "mediatek-vio_dbgs");
>>> +	if (vio_dbgs_num <= 0)
>>> +		return -ENXIO;
>>> +
>>> +	size = (vio_dbgs_num / 2) * sizeof(struct mtk_devapc_vio_dbgs_desc);
>>> +	devapc_ctx->vio_dbgs_desc = devm_kzalloc(&pdev->dev, size, GFP_KERNEL);
>>> +	if (!devapc_ctx->vio_dbgs_desc)
>>> +		return -ENOMEM;
>>> +
>>> +	for (i = 0; i < vio_dbgs_num / 2; i++) {
>>> +		if (of_property_read_u32_index(node, "mediatek-vio_dbgs",
>>> +					       i * 2,
>>> +					       &devapc_ctx->vio_dbgs_desc[i].mask))
>>> +			return -ENXIO;
>>> +
>>> +		if (of_property_read_u32_index(node, "mediatek-vio_dbgs",
>>> +					       (i * 2) + 1,
>>> +					       &devapc_ctx->vio_dbgs_desc[i].start_bit))
>>> +			return -ENXIO;
>>> +	}
>>> +
>>> +	pds_num = of_property_count_u32_elems(node, "mediatek-pds_offset");
>>> +	if (pds_num <= 0)
>>> +		return -ENXIO;
>>> +
>>> +	size = pds_num * sizeof(u32);
>>> +	devapc_ctx->pds_offset = devm_kzalloc(&pdev->dev, size, GFP_KERNEL);
>>> +	if (!devapc_ctx->pds_offset)
>>> +		return -ENOMEM;
>>> +
>>> +	for (i = 0; i < pds_num; i++) {
>>> +		if (of_property_read_u32_index(node, "mediatek-pds_offset", i,
>>> +					       &devapc_ctx->pds_offset[i]))
>>> +			return -ENXIO;
>>> +	}
>>> +
>>> +	devapc_irq = irq_of_parse_and_map(node, 0);
>>> +	if (!devapc_irq)
>>> +		return -EINVAL;
>>> +
>>> +	devapc_infra_clk = devm_clk_get(&pdev->dev, "devapc-infra-clock");
>>> +	if (IS_ERR(devapc_infra_clk))
>>> +		return -EINVAL;
>>> +
>>> +	if (clk_prepare_enable(devapc_infra_clk))
>>> +		return -EINVAL;
>>> +
>>> +	start_devapc(devapc_ctx);
>>> +
>>> +	ret = devm_request_irq(&pdev->dev, devapc_irq,
>>> +			       (irq_handler_t)devapc_violation_irq,
>>> +			       IRQF_TRIGGER_NONE, "devapc", devapc_ctx);
>>> +	if (ret)
>>> +		return ret;
>>> +
>>> +	return 0;
>>> +}
>>> +
>>> +static int mtk_devapc_remove(struct platform_device *dev)
>>> +{
>>> +	return 0;
>>> +}
>>> +
>>> +static const struct of_device_id mtk_devapc_dt_match[] = {
>>> +	{ .compatible = "mediatek,mt6779-devapc" },
>>> +	{},
>>> +};
>>> +
>>> +static struct platform_driver mtk_devapc_driver = {
>>> +	.probe = mtk_devapc_probe,
>>> +	.remove = mtk_devapc_remove,
>>> +	.driver = {
>>> +		.name = KBUILD_MODNAME,
>>> +		.of_match_table = mtk_devapc_dt_match,
>>> +	},
>>> +};
>>> +
>>> +module_platform_driver(mtk_devapc_driver);
>>> +
>>> +MODULE_DESCRIPTION("Mediatek Device APC Driver");
>>> +MODULE_AUTHOR("Neal Liu <neal.liu@...iatek.com>");
>>> +MODULE_LICENSE("GPL");
>>> diff --git a/drivers/soc/mediatek/mtk-devapc.h b/drivers/soc/mediatek/mtk-devapc.h
>>> new file mode 100644
>>> index 0000000..ab2cb14
>>> --- /dev/null
>>> +++ b/drivers/soc/mediatek/mtk-devapc.h
>>> @@ -0,0 +1,670 @@
>>> +/* SPDX-License-Identifier: GPL-2.0 */
>>> +/*
>>> + * Copyright (C) 2020 MediaTek Inc.
>>> + */
>>> +
>>> +#ifndef __MTK_DEVAPC_H__
>>> +#define __MTK_DEVAPC_H__
>>> +
>>> +#define PFX			"[DEVAPC]: "
>>
>> use dev_err() and friends instead.
> 
> Okay, I'll remove it.
> 
>>
>>> +
>>> +#define VIO_MASK_STA_REG_GET(m) \
>>> +({ \
>>> +	typeof(m) (_m) = (m); \
>>> +	reg_index = _m / 32; \
>>> +	reg_offset = _m % 32; \
>>> +})
>>
>> don't do that. no explicit variable assingment in a macro, the macro should
>> return the value.
> 
> Okay, I'll revise it in next patches.
> 
>>
>>> +
>>> +enum DEVAPC_PD_REG_TYPE {
>>> +	VIO_MASK = 0,
>>> +	VIO_STA,
>>> +	VIO_DBG0,
>>> +	VIO_DBG1,
>>> +	APC_CON,
>>> +	VIO_SHIFT_STA,
>>> +	VIO_SHIFT_SEL,
>>> +	VIO_SHIFT_CON,
>>> +	PD_REG_TYPE_NUM,
>>> +};
>>> +
>>> +enum DEVAPC_VIO_DBGS_TYPE {
>>> +	MSTID = 0,
>>> +	DMNID,
>>> +	VIO_W,
>>> +	VIO_R,
>>> +	ADDR_H,
>>> +};
>>> +
>>> +struct mtk_device_info {
>>> +	int sys_index;
>>> +	int ctrl_index;
>>> +	int vio_index;
>>> +};
>>> +
>>> +static struct mtk_device_info mtk_devices_infra[] = {
>>
>> That's for mt6779, correct? Should be stated in the name.
> 
> Okay. I have another way to reach the goal without using this struct
> array. I'll send another patches.
> 

[...]

>>> +
>>> +struct mtk_devapc_vio_info {
>>> +	bool read;
>>> +	bool write;
>>> +	u32 vio_addr;
>>> +	u32 vio_addr_high;
>>> +	u32 master_id;
>>> +	u32 domain_id;
>>> +};
>>> +
>>> +struct mtk_devapc_vio_dbgs_desc {
>>> +	u32 mask;
>>> +	u32 start_bit;
>>> +};
>>> +
>>> +struct mtk_devapc_context {
>>> +	u8 slave_type_num;
>>> +	void __iomem **devapc_pd_base;
>>> +	const struct mtk_device_info **device_info;
>>> +	struct mtk_devapc_vio_info *vio_info;
>>> +	struct mtk_devapc_vio_dbgs_desc *vio_dbgs_desc;
>>> +	u32 *pds_offset;
>>> +};
>>> +
>>
>> Not sure if I get this right:
>>
>> struct mtk_devapc_offset {
>> 	u32 vio_mask;
>> 	u32 vio_sta;
>> 	u32 vio_dbg0;
>> 	u32 vio_dbg1;
>> 	...
>> }
>>
>> struct mtk_devapc_context {
>> 	u8 pd_base_num;
>> 	void __iomem **devapc_pd_base;
>> 	struct mtk_devapc_offset *offset;
>> 	const struct mtk_device_info **device_info;
>> 	struct mtk_devapc_vio_info *vio_info;
>> 	struct mtk_devapc_vio_dbgs_desc *vio_dbgs_desc;
>> };
>>
>> With this I think we can get rid of mtk_devapc_pd_get().
>>
> 
> mtk_devapc_pd_get() is used to calculate the vaddr of devapc pd
> register. It's based on different slave_type, pd_reg_type and reg_idx.
> I don't think it can be replaced with such simple data structures.
> 

How I understand the code:
Every slave_type has a base memory represented by the **devapc_pd_base array.
Inside each base memory chunk you have an offset depending on the pd_reg_type, 
but the offset is the same for all base memory chunks. This offset is 
represented by struct mtk_devapc_offset.
If pd_reg_type is VIO_MASK or VIO_STA we have to further read the value based on 
an index represented by reg_idx. So if we  add 0x4 for each reg_idx. So we have 
for example for:
int check_vio_mask(struct mtk_devapc_context *ctx, inst slave_type, u32 module)
{
	reg = ctx->devapc_pd_base[slave_type] + ctx->offset.vio_mask;
	reg += 0x4 * VIO_MOD_TO_REG_IND(module);

	value = readl(reg);
	return ((value >> VIO_TO_REG_OFF(module)) & 0x1);
}

similarly:
u32 get_shift_group(struct mtk_devapc_context *ctx, int slave_type, int vio_idx)
{
	reg = ctx->devapc_pd_base[slave_type] + ctx->offset.vio_shift_sta;

	value = readl(reg);
	bit = __ffs(...);
}

What does us buy that? When looking on the function we understand how the 
register layout in HW looks like. We have a base value with an offset and in 
case of VIO_MASK and VIO_STA we have to shift the value.

By the way, right now in mtk_devapc_pd_get you are doing pointer arithmetic with 
a void pointer. That's not a good approach, please define the pointer to point 
to the value you want to read. I understand that's a 32 bit register.

Regards
Matthias

> 
>> Sorry I'm not able to review the whole driver right now. Please also have a look
>> on my comments from v1.
>>
>> We will have to go little by little to get this into a good state. In case it
>> makes sense to have this in the kernel at all.
>>
>> Regards,
>> Matthias
> 
> I'm appreciated for your review. It helps me to write better code and
> get closer to the kernel.
> 
> 

Powered by blists - more mailing lists