On Wed, 2020-06-10 at 00:01 +0800, Chun-Kuang Hu wrote:
Hi Chun-Kuang,
[snip]
> > +
> > +/*
> > + * mtk_devapc_pd_get - get devapc pd_types of register address.
> > + *
> > + * Returns the value of reg addr
> > + */
> > +static void __iomem *mtk_devapc_pd_get(int slave_type,
> > + enum DEVAPC_PD_REG_TYPE pd_reg_type,
> > + u32 index)
> > +{
> > + struct mtk_devapc_vio_info *vio_info = mtk_devapc_ctx->soc->vio_info;
> > + u32 slave_type_num = mtk_devapc_ctx->soc->slave_type_num;
> > + const u32 *devapc_pds = mtk_devapc_ctx->soc->devapc_pds;
> > + void __iomem *reg;
> > +
> > + if (!devapc_pds)
> > + return NULL;
> > +
> > + if ((slave_type < slave_type_num &&
> > + index < vio_info->vio_mask_sta_num[slave_type]) &&
> > + pd_reg_type < PD_REG_TYPE_NUM) {
> > + reg = mtk_devapc_ctx->devapc_pd_base[slave_type] +
> > + devapc_pds[pd_reg_type];
> > +
> > + if (pd_reg_type == VIO_MASK || pd_reg_type == VIO_STA)
> > + reg += 0x4 * index;
> > +
> > + } else {
> > + pr_err(PFX "%s:0x%x or %s:0x%x or %s:0x%x is out of boundary\n",
> > + "slave_type", slave_type,
>
> Move "slave_type" into format string.
Why is this necessary? Is there any benefit for moving this?
Since the line length is almost over 80 chars.
>
> > + "pd_reg_type", pd_reg_type,
> > + "index", index);
> > + return NULL;
> > + }
> > +
> > + return reg;
> > +}
> > +
>
[snip]
>
> > +
> > +/*
> > + * 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, void *dev_id)
> > +{
> > + u32 slave_type_num = mtk_devapc_ctx->soc->slave_type_num;
> > + const struct mtk_device_info **device_info;
> > + struct mtk_devapc_vio_info *vio_info;
> > + int slave_type, vio_idx, index;
> > + const char *vio_master;
> > + unsigned long flags;
> > + bool normal;
> > + u8 perm;
> > +
> > + spin_lock_irqsave(&devapc_lock, flags);
> > +
> > + device_info = mtk_devapc_ctx->soc->device_info;
> > + vio_info = mtk_devapc_ctx->soc->vio_info;
> > + normal = false;
> > + vio_idx = -1;
> > + index = -1;
> > +
> > + /* There are multiple DEVAPC_PD */
> > + for (slave_type = 0; slave_type < slave_type_num; slave_type++) {
> > + if (!check_type2_vio_status(slave_type, &vio_idx, &index))
> > + if (!mtk_devapc_dump_vio_dbg(slave_type, &vio_idx,
> > + &index))
> > + continue;
> > +
> > + /* Ensure that violation info are written before
> > + * further operations
> > + */
> > + smp_mb();
> > + normal = true;
> > +
> > + mask_module_irq(slave_type, vio_idx, true);
> > +
> > + if (clear_vio_status(slave_type, vio_idx))
> > + pr_warn(PFX "%s, %s:0x%x, %s:0x%x\n",
> > + "clear vio status failed",
> > + "slave_type", slave_type,
> > + "vio_index", vio_idx);
> > +
> > + perm = get_permission(slave_type, index, vio_info->domain_id);
> > +
> > + vio_master = mtk_devapc_ctx->soc->master_get
> > + (vio_info->master_id,
> > + vio_info->vio_addr,
> > + slave_type,
> > + vio_info->shift_sta_bit,
> > + vio_info->domain_id);
>
> Call mt6873_bus_id_to_master() directly. For first patch, make things
> as simple as possible.
In devapc_violation_irq() function, we use common flow to handle each
devapc violation on different platforms. The master_get() has different
implementation on different platforms, that why it called indirectly.
Once we have new platform, we only have to update devapc-mtxxxx.c
instead of common handler flow.
>
> > +
> > + if (!vio_master) {
> > + pr_warn(PFX "master_get failed\n");
> > + vio_master = "UNKNOWN_MASTER";
> > + }
> > +
> > + pr_info(PFX "%s - %s:0x%x, %s:0x%x, %s:0x%x, %s:0x%x\n",
> > + "Violation", "slave_type", slave_type,
> > + "sys_index",
> > + device_info[slave_type][index].sys_index,
> > + "ctrl_index",
> > + device_info[slave_type][index].ctrl_index,
> > + "vio_index",
> > + device_info[slave_type][index].vio_index);
> > +
> > + pr_info(PFX "%s %s %s %s\n",
> > + "Violation - master:", vio_master,
> > + "access violation slave:",
> > + device_info[slave_type][index].device);
> > +
> > + devapc_vio_reason(perm);
> > +
> > + devapc_extra_handler(slave_type, vio_master, vio_idx,
> > + vio_info->vio_addr);
> > +
> > + mask_module_irq(slave_type, vio_idx, false);
> > + }
> > +
> > + if (normal) {
> > + spin_unlock_irqrestore(&devapc_lock, flags);
> > + return IRQ_HANDLED;
> > + }
> > +
> > + spin_unlock_irqrestore(&devapc_lock, flags);
> > + return IRQ_HANDLED;
> > +}
> > +
[snip]
