Date:   Thu, 11 Jun 2020 19:01:39 +0800
From:   Chun-Kuang Hu <chunkuang.hu@...nel.org>
To:     Neal Liu <neal.liu@...iatek.com>
Cc:     Chun-Kuang Hu <chunkuang.hu@...nel.org>,
        "devicetree@...r.kernel.org" <devicetree@...r.kernel.org>,
        wsd_upstream <wsd_upstream@...iatek.com>,
        linux-kernel <linux-kernel@...r.kernel.org>,
        Rob Herring <robh+dt@...nel.org>,
        "moderated list:ARM/Mediatek SoC support" 
        <linux-mediatek@...ts.infradead.org>,
        Matthias Brugger <matthias.bgg@...il.com>,
        Linux ARM <linux-arm-kernel@...ts.infradead.org>
Subject: Re: [PATCH 2/2] soc: mediatek: devapc: add devapc-mt6873 driver

Hi, Neal:

Neal Liu <neal.liu@...iatek.com> 於 2020年6月11日 週四 下午5:26寫道：
>
> 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?

Smaller code size, simple, intuition.

> Since the line length is almost over 80 chars.

Single string could be 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.

You just upstream one SoC now, so I have no information of 2nd SoC.
Without the 2nd SoC, how do we know what is common and what is SoC special?
So the first patch should not consider the things which does not exist yet.

Regards,
Chun-Kuang.

>
> >
> > > +
> > > +               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]
>
>

Powered by blists - more mailing lists