Re: [PATCH 2/2] soc: mediatek: devapc: add devapc-mt6873 driver

[Neal Liu]

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]