[PATCH v2 2/2] soc: mediatek: add mtk-devapc driver
Matthias Brugger
matthias.bgg at gmail.com
Mon Jul 13 07:16:37 EDT 2020
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 at mediatek.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.
>
>
More information about the linux-arm-kernel
mailing list