Re: [PATCH v4 2/2] soc: mediatek: add mtk-devapc driver
Neal Liu 於 2020年8月4日 週二 上午10:08寫道：
>
> On Tue, 2020-08-04 at 00:04 +0800, Chun-Kuang Hu wrote:
> > Hi, Neal:
> >
> > Neal Liu 於 2020年8月3日 週一 下午12:01寫道：
> > >
> > > Hi Chun-Kuang,
> > >
> > > On Sat, 2020-08-01 at 08:12 +0800, Chun-Kuang Hu wrote:
> > > > Hi, Neal:
> > > >
> > > > This patch is for "mediatek,mt6779-devapc", so I think commit title
> > > > should show the SoC ID.
> > >
> > > Okay, I'll change title to 'soc:mediatek: add mt6779 devapc driver'.
> > >
> > > >
> > > > Neal Liu 於 2020年7月29日 週三 下午4:29寫道：
> > > > >
> > > > > MediaTek bus fabric provides TrustZone security support and data
> > > > > protection to prevent slaves from being accessed by unexpected
> > > > > masters.
> > > > > The security violation is logged and sent to the processor for
> > > > > further analysis or countermeasures.
> > > > >
> > > > > Any occurrence of security violation would raise an interrupt, and
> > > > > it will be handled by mtk-devapc driver. The violation
> > > > > information is printed in order to find the murderer.
> > > > >
> > > > > Signed-off-by: Neal Liu
> > > > > ---
> > > >
> > > > [snip]
> > > >
> > > > > +
> > > > > +struct mtk_devapc_context {
> > > > > + struct device *dev;
> > > > > + u32 vio_idx_num;
> > > > > + void __iomem *devapc_pd_base;
> > > > > + struct mtk_devapc_vio_info *vio_info;
> > > > > + const struct mtk_devapc_pd_offset *offset;
> > > > > + const struct mtk_devapc_vio_dbgs *vio_dbgs;
> > > > > +};
> > > >
> > > > I think this structure should separate the constant part. The constant
> > > > part is:
> > > >
> > > > struct mtk_devapc_data {
> > > > const u32 vio_idx_num;
> > > > const struct mtk_devapc_pd_offset *offset; /* I would like to
> > > > remove struct mtk_devapc_pd_offset and directly put its member into
> > > > this structure */
> > > > const struct mtk_devapc_vio_dbgs *vio_dbgs; /* This may disappear */
> > > > };
> > > >
> > > > And the context is:
> > > >
> > > > struct mtk_devapc_context {
> > > > struct device *dev;
> > > > void __iomem *devapc_pd_base;
> > > > const struct mtk_devapc_data *data;
> > > > };
> > > >
> > > > So when you define this, you would not waste memory to store
> > > > non-constant data.
> > > >
> > > > static const struct mtk_devapc_data devapc_mt6779 = {
> > > > .vio_idx_num = 510,
> > > > .offset = _pd_offset,
> > > > .vio_dbgs = _vio_dbgs,
> > > > };
> > > >
> > >
> > > Sorry, I still don't understand why this refactoring will not waste
> > > memory to store non-constant data. Could you explain more details?
> > > To my understanding, we still also have to allocate memory to store dev
> > > & devapc_pd_base.
> >
> > In some situation, it is. You make the non-constant data a global
> > variable. I think the context data should be dynamic allocated. If
> > this driver is not probed, the non-constant data occupy the memory.
> >
>
> I got your point! In this case, we can save these 2 data structure
> space, right?
>
> struct device *dev;
> void __iomem *devapc_pd_base;
Right.
>
> I'll refactoring this data structures on next patch. Thanks !
>
> > Regards,
> > Chun-Kuang.
> >
> > >
> > > > Regards,
> > > > Chun-Kuang.
> > > >
> > > > > +
> > > > > +#endif /* __MTK_DEVAPC_H__ */
> > > > > --
> > > > > 1.7.9.5
> > > > > ___
> > > > > Linux-mediatek mailing list
> > > > > linux-media...@lists.infradead.org
> > > > > http://lists.infradead.org/mailman/listinfo/linux-mediatek
> > >
>
Re: [PATCH v4 2/2] soc: mediatek: add mtk-devapc driver
Neal Liu 於 2020年8月4日 週二 上午10:19寫道：
>
>
> On Tue, 2020-08-04 at 00:13 +0800, Chun-Kuang Hu wrote:
> > Hi, Neal:
> >
> > Neal Liu 於 2020年8月3日 週一 上午11:32寫道：
> > >
> > > Hi Chun-Kuang,
> > >
> > > On Fri, 2020-07-31 at 23:03 +0800, Chun-Kuang Hu wrote:
> > > > Hi, Neal:
> > > >
> > > > Neal Liu 於 2020年7月31日 週五 上午10:44寫道：
> > > > >
> > > > > Hi Chun-Kuang,
> > > > >
> > > > >
> > > > > On Thu, 2020-07-30 at 00:38 +0800, Chun-Kuang Hu wrote:
> > > > > > Hi, Neal:
> > > > > >
> > > > > > Neal Liu 於 2020年7月29日 週三 下午4:29寫道：
> > > > > > >
> > > > > > > MediaTek bus fabric provides TrustZone security support and data
> > > > > > > protection to prevent slaves from being accessed by unexpected
> > > > > > > masters.
> > > > > > > The security violation is logged and sent to the processor for
> > > > > > > further analysis or countermeasures.
> > > > > > >
> > > > > > > Any occurrence of security violation would raise an interrupt, and
> > > > > > > it will be handled by mtk-devapc driver. The violation
> > > > > > > information is printed in order to find the murderer.
> > > > > > >
> > > > > > > Signed-off-by: Neal Liu
> > > > > > > ---
> > > > > >
> > > > > > [snip]
> > > > > >
> > > > > > > +
> > > > > > > +/*
> > > > > > > + * devapc_extract_vio_dbg - extract full violation information
> > > > > > > after doing
> > > > > > > + * shift mechanism.
> > > > > > > + */
> > > > > > > +static void devapc_extract_vio_dbg(struct mtk_devapc_context
> > > > > > > *ctx)
> > > > > > > +{
> > > > > > > + const struct mtk_devapc_vio_dbgs *vio_dbgs;
> > > > > > > + struct mtk_devapc_vio_info *vio_info;
> > > > > > > + void __iomem *vio_dbg0_reg;
> > > > > > > + void __iomem *vio_dbg1_reg;
> > > > > > > + u32 dbg0;
> > > > > > > +
> > > > > > > + vio_dbg0_reg = ctx->devapc_pd_base +
> > > > > > > ctx->offset->vio_dbg0;
> > > > > > > + vio_dbg1_reg = ctx->devapc_pd_base +
> > > > > > > ctx->offset->vio_dbg1;
> > > > > > > +
> > > > > > > + vio_dbgs = ctx->vio_dbgs;
> > > > > > > + vio_info = ctx->vio_info;
> > > > > > > +
> > > > > > > + /* Starts to 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;
> > > > > >
> > > > > > What is master_id? How could we use it to debug? For example, if we
> > > > > > get a master_id = 1, what should we do for this?
> > > > > >
> > > > > > > + vio_info->domain_id = (dbg0 & vio_dbgs->dmnid.mask) >>
> > > > > > > + vio_dbgs->dmnid.start;
> > > > > >
> > > > > > What is domain_id? How could we use it to debug? For example, if we
> > > > > > get a domain_id = 2, what should we do for this?
> > > > > >
> > > > >
> > > > > master_id and domain_id belongs our bus side-band signal info. It can
> > > > > help us to find the violation master.
> > > >
> > > > Does 'violation master' means the hardware could access the protected
> > > > register? (ex. CPU, GCE, ...) If so, I think it's better to add
> > > > comment to explain how to map (master_id, domain_id) to a hardware
> > > > (maybe the device in device tree) because every body does not know
> > > > what the number means. Don't try to translate the number to a string
> > > > because this would cost much time to do this. Just print a number and
> > > > we could find out the master by the comment.
> > >
> > > 'violation master' means the master which violates the permission
> > > control. For example, if we set permission 'Secure R/W only' as CPU to
> > > spi register. When violation is triggered, it means CPU access spi
> > > register through normal world instead of secure world, which is not
> > > allowed.
> > >
> > > 'master_id' cannot use the simple comments to describe which master it
> > > is. It depends on violation slaves. For example, if there are two
> > > violations:
> > > 1. CPU access spi reg
> > > 2. CPU access timer reg
> > > It might be different 'master_id' for CPU on these two cases.
> > > I would prefer to remain the id number if translate to a string is a bad
> > > idea.
> > > Thanks !
> >
> > It seams that master_id and domain_id does not help for debug. When we
> > get master_id = 1 and domain_id = 2, we don't know what it mean. I
> > think we just need violation address because we could find the driver
> > that write this address and the bug would be inside this driver. So
> > need not to process master_id and domain_id.
> >
>
> Actually, it does help us for debug. violation master is not CPU only.
> It might be any other master in our SoC. So the bug might not be inside
> the kernel driver.
> I'll prefer to remain this information.
> Thanks !
Let maintainer to make decision. Maybe he like to print magic number
and wait for someone to hack it.
>
> >
Re: [PATCH v4 2/2] soc: mediatek: add mtk-devapc driver
On Tue, 2020-08-04 at 00:13 +0800, Chun-Kuang Hu wrote:
> Hi, Neal:
>
> Neal Liu 於 2020年8月3日 週一 上午11:32寫道：
> >
> > Hi Chun-Kuang,
> >
> > On Fri, 2020-07-31 at 23:03 +0800, Chun-Kuang Hu wrote:
> > > Hi, Neal:
> > >
> > > Neal Liu 於 2020年7月31日 週五 上午10:44寫道：
> > > >
> > > > Hi Chun-Kuang,
> > > >
> > > >
> > > > On Thu, 2020-07-30 at 00:38 +0800, Chun-Kuang Hu wrote:
> > > > > Hi, Neal:
> > > > >
> > > > > Neal Liu 於 2020年7月29日 週三 下午4:29寫道：
> > > > > >
> > > > > > MediaTek bus fabric provides TrustZone security support and data
> > > > > > protection to prevent slaves from being accessed by unexpected
> > > > > > masters.
> > > > > > The security violation is logged and sent to the processor for
> > > > > > further analysis or countermeasures.
> > > > > >
> > > > > > Any occurrence of security violation would raise an interrupt, and
> > > > > > it will be handled by mtk-devapc driver. The violation
> > > > > > information is printed in order to find the murderer.
> > > > > >
> > > > > > Signed-off-by: Neal Liu
> > > > > > ---
> > > > >
> > > > > [snip]
> > > > >
> > > > > > +
> > > > > > +/*
> > > > > > + * devapc_extract_vio_dbg - extract full violation information
> > > > > > after doing
> > > > > > + * shift mechanism.
> > > > > > + */
> > > > > > +static void devapc_extract_vio_dbg(struct mtk_devapc_context *ctx)
> > > > > > +{
> > > > > > + const struct mtk_devapc_vio_dbgs *vio_dbgs;
> > > > > > + struct mtk_devapc_vio_info *vio_info;
> > > > > > + void __iomem *vio_dbg0_reg;
> > > > > > + void __iomem *vio_dbg1_reg;
> > > > > > + u32 dbg0;
> > > > > > +
> > > > > > + vio_dbg0_reg = ctx->devapc_pd_base + ctx->offset->vio_dbg0;
> > > > > > + vio_dbg1_reg = ctx->devapc_pd_base + ctx->offset->vio_dbg1;
> > > > > > +
> > > > > > + vio_dbgs = ctx->vio_dbgs;
> > > > > > + vio_info = ctx->vio_info;
> > > > > > +
> > > > > > + /* Starts to 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;
> > > > >
> > > > > What is master_id? How could we use it to debug? For example, if we
> > > > > get a master_id = 1, what should we do for this?
> > > > >
> > > > > > + vio_info->domain_id = (dbg0 & vio_dbgs->dmnid.mask) >>
> > > > > > + vio_dbgs->dmnid.start;
> > > > >
> > > > > What is domain_id? How could we use it to debug? For example, if we
> > > > > get a domain_id = 2, what should we do for this?
> > > > >
> > > >
> > > > master_id and domain_id belongs our bus side-band signal info. It can
> > > > help us to find the violation master.
> > >
> > > Does 'violation master' means the hardware could access the protected
> > > register? (ex. CPU, GCE, ...) If so, I think it's better to add
> > > comment to explain how to map (master_id, domain_id) to a hardware
> > > (maybe the device in device tree) because every body does not know
> > > what the number means. Don't try to translate the number to a string
> > > because this would cost much time to do this. Just print a number and
> > > we could find out the master by the comment.
> >
> > 'violation master' means the master which violates the permission
> > control. For example, if we set permission 'Secure R/W only' as CPU to
> > spi register. When violation is triggered, it means CPU access spi
> > register through normal world instead of secure world, which is not
> > allowed.
> >
> > 'master_id' cannot use the simple comments to describe which master it
> > is. It depends on violation slaves. For example, if there are two
> > violations:
> > 1. CPU access spi reg
> > 2. CPU access timer reg
> > It might be different 'master_id' for CPU on these two cases.
> > I would prefer to remain the id number if translate to a string is a bad
> > idea.
> > Thanks !
>
> It seams that master_id and domain_id does not help for debug. When we
> get master_id = 1 and domain_id = 2, we don't know what it mean. I
> think we just need violation address because we could find the driver
> that write this address and the bug would be inside this driver. So
> need not to process master_id and domain_id.
>
Actually, it does help us for debug. violation master is not CPU only.
It might be any other master in our SoC. So the bug might not be inside
the kernel driver.
I'll prefer to remain this information.
Thanks !
> Regards,
> Chun-Kuang.
>
> >
> > >
> > > >
> > > > > > + vio_info->write = ((dbg0 & vio_dbgs->vio_w.mask) >>
> > > > > > + vio_dbgs->vio_w.start) == 1;
> > > > > > + vio_info->read = ((dbg0 & vio_dbgs->vio_r.mask) >>
> > > > > > + vio_dbgs->vio_r.start) == 1;
> > > > > > + vio_info->vio_addr_high = (dbg0 & vio_dbgs->addr_h.mask) >>
> > > > >
Re: [PATCH v4 2/2] soc: mediatek: add mtk-devapc driver
On Tue, 2020-08-04 at 00:04 +0800, Chun-Kuang Hu wrote:
> Hi, Neal:
>
> Neal Liu 於 2020年8月3日 週一 下午12:01寫道：
> >
> > Hi Chun-Kuang,
> >
> > On Sat, 2020-08-01 at 08:12 +0800, Chun-Kuang Hu wrote:
> > > Hi, Neal:
> > >
> > > This patch is for "mediatek,mt6779-devapc", so I think commit title
> > > should show the SoC ID.
> >
> > Okay, I'll change title to 'soc:mediatek: add mt6779 devapc driver'.
> >
> > >
> > > Neal Liu 於 2020年7月29日 週三 下午4:29寫道：
> > > >
> > > > MediaTek bus fabric provides TrustZone security support and data
> > > > protection to prevent slaves from being accessed by unexpected
> > > > masters.
> > > > The security violation is logged and sent to the processor for
> > > > further analysis or countermeasures.
> > > >
> > > > Any occurrence of security violation would raise an interrupt, and
> > > > it will be handled by mtk-devapc driver. The violation
> > > > information is printed in order to find the murderer.
> > > >
> > > > Signed-off-by: Neal Liu
> > > > ---
> > >
> > > [snip]
> > >
> > > > +
> > > > +struct mtk_devapc_context {
> > > > + struct device *dev;
> > > > + u32 vio_idx_num;
> > > > + void __iomem *devapc_pd_base;
> > > > + struct mtk_devapc_vio_info *vio_info;
> > > > + const struct mtk_devapc_pd_offset *offset;
> > > > + const struct mtk_devapc_vio_dbgs *vio_dbgs;
> > > > +};
> > >
> > > I think this structure should separate the constant part. The constant
> > > part is:
> > >
> > > struct mtk_devapc_data {
> > > const u32 vio_idx_num;
> > > const struct mtk_devapc_pd_offset *offset; /* I would like to
> > > remove struct mtk_devapc_pd_offset and directly put its member into
> > > this structure */
> > > const struct mtk_devapc_vio_dbgs *vio_dbgs; /* This may disappear */
> > > };
> > >
> > > And the context is:
> > >
> > > struct mtk_devapc_context {
> > > struct device *dev;
> > > void __iomem *devapc_pd_base;
> > > const struct mtk_devapc_data *data;
> > > };
> > >
> > > So when you define this, you would not waste memory to store non-constant
> > > data.
> > >
> > > static const struct mtk_devapc_data devapc_mt6779 = {
> > > .vio_idx_num = 510,
> > > .offset = _pd_offset,
> > > .vio_dbgs = _vio_dbgs,
> > > };
> > >
> >
> > Sorry, I still don't understand why this refactoring will not waste
> > memory to store non-constant data. Could you explain more details?
> > To my understanding, we still also have to allocate memory to store dev
> > & devapc_pd_base.
>
> In some situation, it is. You make the non-constant data a global
> variable. I think the context data should be dynamic allocated. If
> this driver is not probed, the non-constant data occupy the memory.
>
I got your point! In this case, we can save these 2 data structure
space, right?
struct device *dev;
void __iomem *devapc_pd_base;
I'll refactoring this data structures on next patch. Thanks !
> Regards,
> Chun-Kuang.
>
> >
> > > Regards,
> > > Chun-Kuang.
> > >
> > > > +
> > > > +#endif /* __MTK_DEVAPC_H__ */
> > > > --
> > > > 1.7.9.5
> > > > ___
> > > > Linux-mediatek mailing list
> > > > linux-media...@lists.infradead.org
> > > > http://lists.infradead.org/mailman/listinfo/linux-mediatek
> >
Re: [PATCH v4 2/2] soc: mediatek: add mtk-devapc driver
Hi, Neal:
Neal Liu 於 2020年8月3日 週一 上午11:32寫道：
>
> Hi Chun-Kuang,
>
> On Fri, 2020-07-31 at 23:03 +0800, Chun-Kuang Hu wrote:
> > Hi, Neal:
> >
> > Neal Liu 於 2020年7月31日 週五 上午10:44寫道：
> > >
> > > Hi Chun-Kuang,
> > >
> > >
> > > On Thu, 2020-07-30 at 00:38 +0800, Chun-Kuang Hu wrote:
> > > > Hi, Neal:
> > > >
> > > > Neal Liu 於 2020年7月29日 週三 下午4:29寫道：
> > > > >
> > > > > MediaTek bus fabric provides TrustZone security support and data
> > > > > protection to prevent slaves from being accessed by unexpected
> > > > > masters.
> > > > > The security violation is logged and sent to the processor for
> > > > > further analysis or countermeasures.
> > > > >
> > > > > Any occurrence of security violation would raise an interrupt, and
> > > > > it will be handled by mtk-devapc driver. The violation
> > > > > information is printed in order to find the murderer.
> > > > >
> > > > > Signed-off-by: Neal Liu
> > > > > ---
> > > >
> > > > [snip]
> > > >
> > > > > +
> > > > > +/*
> > > > > + * devapc_extract_vio_dbg - extract full violation information after
> > > > > doing
> > > > > + * shift mechanism.
> > > > > + */
> > > > > +static void devapc_extract_vio_dbg(struct mtk_devapc_context *ctx)
> > > > > +{
> > > > > + const struct mtk_devapc_vio_dbgs *vio_dbgs;
> > > > > + struct mtk_devapc_vio_info *vio_info;
> > > > > + void __iomem *vio_dbg0_reg;
> > > > > + void __iomem *vio_dbg1_reg;
> > > > > + u32 dbg0;
> > > > > +
> > > > > + vio_dbg0_reg = ctx->devapc_pd_base + ctx->offset->vio_dbg0;
> > > > > + vio_dbg1_reg = ctx->devapc_pd_base + ctx->offset->vio_dbg1;
> > > > > +
> > > > > + vio_dbgs = ctx->vio_dbgs;
> > > > > + vio_info = ctx->vio_info;
> > > > > +
> > > > > + /* Starts to 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;
> > > >
> > > > What is master_id? How could we use it to debug? For example, if we
> > > > get a master_id = 1, what should we do for this?
> > > >
> > > > > + vio_info->domain_id = (dbg0 & vio_dbgs->dmnid.mask) >>
> > > > > + vio_dbgs->dmnid.start;
> > > >
> > > > What is domain_id? How could we use it to debug? For example, if we
> > > > get a domain_id = 2, what should we do for this?
> > > >
> > >
> > > master_id and domain_id belongs our bus side-band signal info. It can
> > > help us to find the violation master.
> >
> > Does 'violation master' means the hardware could access the protected
> > register? (ex. CPU, GCE, ...) If so, I think it's better to add
> > comment to explain how to map (master_id, domain_id) to a hardware
> > (maybe the device in device tree) because every body does not know
> > what the number means. Don't try to translate the number to a string
> > because this would cost much time to do this. Just print a number and
> > we could find out the master by the comment.
>
> 'violation master' means the master which violates the permission
> control. For example, if we set permission 'Secure R/W only' as CPU to
> spi register. When violation is triggered, it means CPU access spi
> register through normal world instead of secure world, which is not
> allowed.
>
> 'master_id' cannot use the simple comments to describe which master it
> is. It depends on violation slaves. For example, if there are two
> violations:
> 1. CPU access spi reg
> 2. CPU access timer reg
> It might be different 'master_id' for CPU on these two cases.
> I would prefer to remain the id number if translate to a string is a bad
> idea.
> Thanks !
It seams that master_id and domain_id does not help for debug. When we
get master_id = 1 and domain_id = 2, we don't know what it mean. I
think we just need violation address because we could find the driver
that write this address and the bug would be inside this driver. So
need not to process master_id and domain_id.
Regards,
Chun-Kuang.
>
> >
> > >
> > > > > + vio_info->write = ((dbg0 & vio_dbgs->vio_w.mask) >>
> > > > > + vio_dbgs->vio_w.start) == 1;
> > > > > + vio_info->read = ((dbg0 & vio_dbgs->vio_r.mask) >>
> > > > > + vio_dbgs->vio_r.start) == 1;
> > > > > + vio_info->vio_addr_high = (dbg0 & vio_dbgs->addr_h.mask) >>
> > > > > + vio_dbgs->addr_h.start;
> > > >
> > > > What is vio_addr_high? As I know all register address are 32 bits, is
> > > > vio_addr_high the address above 32 bits?
> > >
> > > Yes, you are right. In MT6779, all register base are 32 bits. We can
> > > ignore this info for current driver. I'll update on next patch.
> > > Thanks !
> >
> > Such a strange hardware, all register is 32 bits but it has a
> > vio_addr_high in its register. OK, just drop this.
> >
> > >
> > >
Re: [PATCH v4 2/2] soc: mediatek: add mtk-devapc driver
Hi, Neal:
Neal Liu 於 2020年8月3日 週一 下午12:01寫道：
>
> Hi Chun-Kuang,
>
> On Sat, 2020-08-01 at 08:12 +0800, Chun-Kuang Hu wrote:
> > Hi, Neal:
> >
> > This patch is for "mediatek,mt6779-devapc", so I think commit title
> > should show the SoC ID.
>
> Okay, I'll change title to 'soc:mediatek: add mt6779 devapc driver'.
>
> >
> > Neal Liu 於 2020年7月29日 週三 下午4:29寫道：
> > >
> > > MediaTek bus fabric provides TrustZone security support and data
> > > protection to prevent slaves from being accessed by unexpected
> > > masters.
> > > The security violation is logged and sent to the processor for
> > > further analysis or countermeasures.
> > >
> > > Any occurrence of security violation would raise an interrupt, and
> > > it will be handled by mtk-devapc driver. The violation
> > > information is printed in order to find the murderer.
> > >
> > > Signed-off-by: Neal Liu
> > > ---
> >
> > [snip]
> >
> > > +
> > > +struct mtk_devapc_context {
> > > + struct device *dev;
> > > + u32 vio_idx_num;
> > > + void __iomem *devapc_pd_base;
> > > + struct mtk_devapc_vio_info *vio_info;
> > > + const struct mtk_devapc_pd_offset *offset;
> > > + const struct mtk_devapc_vio_dbgs *vio_dbgs;
> > > +};
> >
> > I think this structure should separate the constant part. The constant part
> > is:
> >
> > struct mtk_devapc_data {
> > const u32 vio_idx_num;
> > const struct mtk_devapc_pd_offset *offset; /* I would like to
> > remove struct mtk_devapc_pd_offset and directly put its member into
> > this structure */
> > const struct mtk_devapc_vio_dbgs *vio_dbgs; /* This may disappear */
> > };
> >
> > And the context is:
> >
> > struct mtk_devapc_context {
> > struct device *dev;
> > void __iomem *devapc_pd_base;
> > const struct mtk_devapc_data *data;
> > };
> >
> > So when you define this, you would not waste memory to store non-constant
> > data.
> >
> > static const struct mtk_devapc_data devapc_mt6779 = {
> > .vio_idx_num = 510,
> > .offset = _pd_offset,
> > .vio_dbgs = _vio_dbgs,
> > };
> >
>
> Sorry, I still don't understand why this refactoring will not waste
> memory to store non-constant data. Could you explain more details?
> To my understanding, we still also have to allocate memory to store dev
> & devapc_pd_base.
In some situation, it is. You make the non-constant data a global
variable. I think the context data should be dynamic allocated. If
this driver is not probed, the non-constant data occupy the memory.
Regards,
Chun-Kuang.
>
> > Regards,
> > Chun-Kuang.
> >
> > > +
> > > +#endif /* __MTK_DEVAPC_H__ */
> > > --
> > > 1.7.9.5
> > > ___
> > > Linux-mediatek mailing list
> > > linux-media...@lists.infradead.org
> > > http://lists.infradead.org/mailman/listinfo/linux-mediatek
>
Re: [PATCH v4 2/2] soc: mediatek: add mtk-devapc driver
Neal Liu 於 2020年8月3日 週一 下午12:05寫道：
>
> Hi Chun-Kuang,
>
> On Sun, 2020-08-02 at 07:50 +0800, Chun-Kuang Hu wrote:
> > Hi, Neal:
> >
> > Neal Liu 於 2020年7月29日 週三 下午4:29寫道：
> > >
> > > MediaTek bus fabric provides TrustZone security support and data
> > > protection to prevent slaves from being accessed by unexpected
> > > masters.
> > > The security violation is logged and sent to the processor for
> > > further analysis or countermeasures.
> > >
> > > Any occurrence of security violation would raise an interrupt, and
> > > it will be handled by mtk-devapc driver. The violation
> > > information is printed in order to find the murderer.
> > >
> > > Signed-off-by: Neal Liu
> > > ---
> >
> > [snip]
> >
> > > +
> > > +struct mtk_devapc_context {
> > > + struct device *dev;
> > > + u32 vio_idx_num;
> > > + void __iomem *devapc_pd_base;
> >
> > This is devapc context, so prefix 'devapc' is redundant.
> > And, what does 'pd' mean?
>
> 'pd' means power down. Of course we would also remove it as well.
> I suggest to change it as 'infra_base'.
This is OK for me.
>
> >
> > Regards,
> > Chun-Kuang.
> >
> > > + struct mtk_devapc_vio_info *vio_info;
> > > + const struct mtk_devapc_pd_offset *offset;
> > > + const struct mtk_devapc_vio_dbgs *vio_dbgs;
> > > +};
> > > +
>
Re: [PATCH v4 2/2] soc: mediatek: add mtk-devapc driver
Hi Chun-Kuang,
On Sun, 2020-08-02 at 07:50 +0800, Chun-Kuang Hu wrote:
> Hi, Neal:
>
> Neal Liu 於 2020年7月29日 週三 下午4:29寫道：
> >
> > MediaTek bus fabric provides TrustZone security support and data
> > protection to prevent slaves from being accessed by unexpected
> > masters.
> > The security violation is logged and sent to the processor for
> > further analysis or countermeasures.
> >
> > Any occurrence of security violation would raise an interrupt, and
> > it will be handled by mtk-devapc driver. The violation
> > information is printed in order to find the murderer.
> >
> > Signed-off-by: Neal Liu
> > ---
>
> [snip]
>
> > +
> > +struct mtk_devapc_context {
> > + struct device *dev;
> > + u32 vio_idx_num;
> > + void __iomem *devapc_pd_base;
>
> This is devapc context, so prefix 'devapc' is redundant.
> And, what does 'pd' mean?
'pd' means power down. Of course we would also remove it as well.
I suggest to change it as 'infra_base'.
>
> Regards,
> Chun-Kuang.
>
> > + struct mtk_devapc_vio_info *vio_info;
> > + const struct mtk_devapc_pd_offset *offset;
> > + const struct mtk_devapc_vio_dbgs *vio_dbgs;
> > +};
> > +
Re: [PATCH v4 2/2] soc: mediatek: add mtk-devapc driver
Hi Chun-Kuang,
On Sat, 2020-08-01 at 08:12 +0800, Chun-Kuang Hu wrote:
> Hi, Neal:
>
> This patch is for "mediatek,mt6779-devapc", so I think commit title
> should show the SoC ID.
Okay, I'll change title to 'soc:mediatek: add mt6779 devapc driver'.
>
> Neal Liu 於 2020年7月29日 週三 下午4:29寫道：
> >
> > MediaTek bus fabric provides TrustZone security support and data
> > protection to prevent slaves from being accessed by unexpected
> > masters.
> > The security violation is logged and sent to the processor for
> > further analysis or countermeasures.
> >
> > Any occurrence of security violation would raise an interrupt, and
> > it will be handled by mtk-devapc driver. The violation
> > information is printed in order to find the murderer.
> >
> > Signed-off-by: Neal Liu
> > ---
>
> [snip]
>
> > +
> > +struct mtk_devapc_context {
> > + struct device *dev;
> > + u32 vio_idx_num;
> > + void __iomem *devapc_pd_base;
> > + struct mtk_devapc_vio_info *vio_info;
> > + const struct mtk_devapc_pd_offset *offset;
> > + const struct mtk_devapc_vio_dbgs *vio_dbgs;
> > +};
>
> I think this structure should separate the constant part. The constant part
> is:
>
> struct mtk_devapc_data {
> const u32 vio_idx_num;
> const struct mtk_devapc_pd_offset *offset; /* I would like to
> remove struct mtk_devapc_pd_offset and directly put its member into
> this structure */
> const struct mtk_devapc_vio_dbgs *vio_dbgs; /* This may disappear */
> };
>
> And the context is:
>
> struct mtk_devapc_context {
> struct device *dev;
> void __iomem *devapc_pd_base;
> const struct mtk_devapc_data *data;
> };
>
> So when you define this, you would not waste memory to store non-constant
> data.
>
> static const struct mtk_devapc_data devapc_mt6779 = {
> .vio_idx_num = 510,
> .offset = _pd_offset,
> .vio_dbgs = _vio_dbgs,
> };
>
Sorry, I still don't understand why this refactoring will not waste
memory to store non-constant data. Could you explain more details?
To my understanding, we still also have to allocate memory to store dev
& devapc_pd_base.
> Regards,
> Chun-Kuang.
>
> > +
> > +#endif /* __MTK_DEVAPC_H__ */
> > --
> > 1.7.9.5
> > ___
> > Linux-mediatek mailing list
> > linux-media...@lists.infradead.org
> > http://lists.infradead.org/mailman/listinfo/linux-mediatek
Re: [PATCH v4 2/2] soc: mediatek: add mtk-devapc driver
Hi Chun-Kuang,
On Fri, 2020-07-31 at 23:55 +0800, Chun-Kuang Hu wrote:
> Hi, Neal:
>
> Neal Liu 於 2020年7月31日 週五 上午10:52寫道：
> >
> > Hi Chun-Kuang,
> >
> > On Fri, 2020-07-31 at 00:14 +0800, Chun-Kuang Hu wrote:
> > > Hi, Neal:
> > >
> > > Neal Liu 於 2020年7月29日 週三 下午4:29寫道：
> > > >
> > > > MediaTek bus fabric provides TrustZone security support and data
> > > > protection to prevent slaves from being accessed by unexpected
> > > > masters.
> > > > The security violation is logged and sent to the processor for
> > > > further analysis or countermeasures.
> > > >
> > > > Any occurrence of security violation would raise an interrupt, and
> > > > it will be handled by mtk-devapc driver. The violation
> > > > information is printed in order to find the murderer.
> > > >
> > > > Signed-off-by: Neal Liu
> > > > ---
> > >
> > > [snip]
> > >
> > > > +
> > > > +/*
> > > > + * devapc_extract_vio_dbg - extract full violation information after
> > > > doing
> > > > + * shift mechanism.
> > > > + */
> > > > +static void devapc_extract_vio_dbg(struct mtk_devapc_context *ctx)
> > > > +{
> > > > + const struct mtk_devapc_vio_dbgs *vio_dbgs;
> > > > + struct mtk_devapc_vio_info *vio_info;
> > > > + void __iomem *vio_dbg0_reg;
> > > > + void __iomem *vio_dbg1_reg;
> > > > + u32 dbg0;
> > > > +
> > > > + vio_dbg0_reg = ctx->devapc_pd_base + ctx->offset->vio_dbg0;
> > > > + vio_dbg1_reg = ctx->devapc_pd_base + ctx->offset->vio_dbg1;
> > > > +
> > > > + vio_dbgs = ctx->vio_dbgs;
> > > > + vio_info = ctx->vio_info;
> > > > +
> > > > + /* Starts to 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;
> > > > + vio_info->domain_id = (dbg0 & vio_dbgs->dmnid.mask) >>
> > > > + vio_dbgs->dmnid.start;
> > > > + vio_info->write = ((dbg0 & vio_dbgs->vio_w.mask) >>
> > > > + vio_dbgs->vio_w.start) == 1;
> > > > + vio_info->read = ((dbg0 & vio_dbgs->vio_r.mask) >>
> > > > + vio_dbgs->vio_r.start) == 1;
> > > > + vio_info->vio_addr_high = (dbg0 & vio_dbgs->addr_h.mask) >>
> > > > + vio_dbgs->addr_h.start;
> > >
> > >
> > > I would like to define the type of ctx->vio_info to be
> > >
> > > struct mtk_devapc_vio_dbgs {
> > > u32 mstid:16;
> > > u32 dmnid:6;
> > > u32 vio_w:1;
> > > u32 vio_r:1;
> > > u32 addr_h:4;
> > > u32 resv:4;
> > > };
> > >
> > > so the code would like the simple way
> > >
> > > ctx->vio_info = (struct mtk_devapc_vio_dbgs)readl(vio_dbg1_reg);
> > >
> >
> > This idea looks great! Is there any possible to pass the bit layout by
> > DT data, and still make this operation simple?
> > Why am I asking this question is because this bit layout is platform
> > dependent.
>
> I doubt these info would be in a single 32-bits register for all
> future SoC. If they are not in single 32-bits register, you may create
> a vio_dbgs_type in DT data, and the code may be
>
> if (ctx->vio_dbgs_type == VIO_DBGS_TYPE_MT) {
> ctx->vio_info = (struct mtk_devapc_vio_dbgs)readl(vio_dbg1_reg);
> } else if (ctx->vio_dbgs_type == VIO_DBGS_TYPE_MT) {
> ctx->vio_info->mstid = readl(vio_mstid_reg);
> ctx->vio_info->dmnid = readl(vio_dmnid_reg);
> ctx->vio_info->vio_w = readl(vio_vio_w_reg);
> ctx->vio_info->vio_r = readl(vio_vio_r_reg);
> }
>
> I think we need not to consider how the future would be. Once the
> second SoC driver is upstreaming, we could find out the best solution
> for it.
>
Okay, I'll apply this on next patch.
Thanks !
> Regards,
> Chun-Kuang.
>
> >
> > > Regards,
> > > Chun-Kuang.
> > >
> > > > +
> > > > + devapc_vio_info_print(ctx);
> > > > +}
> > > > +
> >
Re: [PATCH v4 2/2] soc: mediatek: add mtk-devapc driver
Hi Chun-Kuang,
On Fri, 2020-07-31 at 23:03 +0800, Chun-Kuang Hu wrote:
> Hi, Neal:
>
> Neal Liu 於 2020年7月31日 週五 上午10:44寫道：
> >
> > Hi Chun-Kuang,
> >
> >
> > On Thu, 2020-07-30 at 00:38 +0800, Chun-Kuang Hu wrote:
> > > Hi, Neal:
> > >
> > > Neal Liu 於 2020年7月29日 週三 下午4:29寫道：
> > > >
> > > > MediaTek bus fabric provides TrustZone security support and data
> > > > protection to prevent slaves from being accessed by unexpected
> > > > masters.
> > > > The security violation is logged and sent to the processor for
> > > > further analysis or countermeasures.
> > > >
> > > > Any occurrence of security violation would raise an interrupt, and
> > > > it will be handled by mtk-devapc driver. The violation
> > > > information is printed in order to find the murderer.
> > > >
> > > > Signed-off-by: Neal Liu
> > > > ---
> > >
> > > [snip]
> > >
> > > > +
> > > > +static int get_shift_group(struct mtk_devapc_context *ctx)
> > > > +{
> > > > + u32 vio_shift_sta;
> > > > + void __iomem *reg;
> > > > +
> > > > + reg = ctx->devapc_pd_base + ctx->offset->vio_shift_sta;
> > > > + vio_shift_sta = readl(reg);
> > > > +
> > > > + if (vio_shift_sta)
> > > > + return __ffs(vio_shift_sta);
> > > > +
> > > > + return -EIO;
> > > > +}
> > >
> > > get_shift_group() is a small function, I would like to merge this
> > > function into sync_vio_dbg() to make code more simple.
> >
> > This function have a specific functionality. And it would make this
> > driver more readability. I would like to keep it as a function. Is that
> > okay for you?
>
> After merge, the function would be:
>
> static bool sync_min_shift_group_vio_dbg(struct mtk_devapc_context *ctx)
> {
> int min_shift_group;
> int ret;
> u32 val;
>
> /* find the minimum shift group which has violation */
> val = readl(ctx->devapc_pd_base + ctx->offset->vio_shift_sta);
> if (!val)
> return false;
>
> min_shift_group = __ffs(val);
>
> /* Assign the group to sync */
> writel(0x1 << min_shift_group, ctx->devapc_pd_base +
> ctx->offset->vio_shift_sel);
>
> /* Start syncing */
> writel(0x1, ctx->devapc_pd_base + ctx->offset->vio_shift_con);
>
> ret = readl_poll_timeout(pd_vio_shift_con_reg, val, val == 0x3, 0,
> PHY_DEVAPC_TIMEOUT);
> if (ret) {
> dev_err(ctx->dev, "%s: Shift violation info failed\n", __func__);
> return false;
> }
>
> /* Stop syncing */
> writel(0x0, ctx->devapc_pd_base + ctx->offset->vio_shift_con);
>
> /* ? */
> writel(0x1 << min_shift_group, ctx->devapc_pd_base +
> ctx->offset->vio_shift_sta);
>
> return true;
> }
>
> The whole function is to sync min_shift_group violation info, I don't
> know why separate any part to an independent function? Any function
> call would cause penalty on CPU performance, so I does not like to
> break this function. After good comment, I think every body could
> understand the function of each register.
> After the merge, the code would be so simple as:
>
> while(sync_min_shift_group_vio_dbg(ctx))
> devapc_extract_vio_dbg(ctx);
>
Okay, this looks good to me. I'll apply this on next patch.
Thanks !
> >
> > >
> > > > +
> > >
> > > [snip]
> > >
> > > > +
> > > > +#define PHY_DEVAPC_TIMEOUT 0x1
> > > > +
> > > > +/*
> > > > + * sync_vio_dbg - do "shift" mechansim" to get full violation
> > > > information.
> > > > + *shift mechanism is depends on devapc hardware design.
> > > > + *Mediatek devapc set multiple slaves as a group. When
> > > > violation
> > > > + *is triggered, violation info is kept inside devapc
> > > > hardware.
> > > > + *Driver should do shift mechansim to "shift" full
> > > > violation
> > > > + *info to VIO_DBGs registers.
> > > > + *
> > > > + */
> > > > +static int sync_vio_dbg(struct mtk_devapc_context *ctx, u32 shift_bit)
> > > > +{
> > > > + void __iomem *pd_vio_shift_sta_reg;
> > > > + void __iomem *pd_vio_shift_sel_reg;
> > > > + void __iomem *pd_vio_shift_con_reg;
> > > > + int ret;
> > > > + u32 val;
> > > > +
> > > > + pd_vio_shift_sta_reg = ctx->devapc_pd_base +
> > > > ctx->offset->vio_shift_sta;
> > > > + pd_vio_shift_sel_reg = ctx->devapc_pd_base +
> > > > ctx->offset->vio_shift_sel;
> > > > + pd_vio_shift_con_reg = ctx->devapc_pd_base +
> > > > ctx->offset->vio_shift_con;
> > > > +
> > > > + /* Enable shift mechansim */
> > > > + writel(0x1 << shift_bit, pd_vio_shift_sel_reg);
> > > > + writel(0x1, pd_vio_shift_con_reg);
> > > > +
> > > > + ret = readl_poll_timeout(pd_vio_shift_con_reg, val, val == 0x3,
> > > > 0,
> > > > +PHY_DEVAPC_TIMEOUT);
> > > > + if (ret)
> > > > + dev_err(ctx->dev, "%s: Shift violation info failed\n",
> > > > __func__);
> > > > +
> > > > + /* Disable shift mechanism */
> > > > + writel(0x0, pd_vio_shift_con_reg);
> > > > + writel(0x0,
Re: [PATCH v4 2/2] soc: mediatek: add mtk-devapc driver
Hi, Neal:
Neal Liu 於 2020年7月29日 週三 下午4:29寫道：
>
> MediaTek bus fabric provides TrustZone security support and data
> protection to prevent slaves from being accessed by unexpected
> masters.
> The security violation is logged and sent to the processor for
> further analysis or countermeasures.
>
> Any occurrence of security violation would raise an interrupt, and
> it will be handled by mtk-devapc driver. The violation
> information is printed in order to find the murderer.
>
> Signed-off-by: Neal Liu
> ---
[snip]
> +
> +struct mtk_devapc_context {
> + struct device *dev;
> + u32 vio_idx_num;
> + void __iomem *devapc_pd_base;
This is devapc context, so prefix 'devapc' is redundant.
And, what does 'pd' mean?
Regards,
Chun-Kuang.
> + struct mtk_devapc_vio_info *vio_info;
> + const struct mtk_devapc_pd_offset *offset;
> + const struct mtk_devapc_vio_dbgs *vio_dbgs;
> +};
> +
Re: [PATCH v4 2/2] soc: mediatek: add mtk-devapc driver
Hi, Neal:
This patch is for "mediatek,mt6779-devapc", so I think commit title
should show the SoC ID.
Neal Liu 於 2020年7月29日 週三 下午4:29寫道：
>
> MediaTek bus fabric provides TrustZone security support and data
> protection to prevent slaves from being accessed by unexpected
> masters.
> The security violation is logged and sent to the processor for
> further analysis or countermeasures.
>
> Any occurrence of security violation would raise an interrupt, and
> it will be handled by mtk-devapc driver. The violation
> information is printed in order to find the murderer.
>
> Signed-off-by: Neal Liu
> ---
[snip]
> +
> +struct mtk_devapc_context {
> + struct device *dev;
> + u32 vio_idx_num;
> + void __iomem *devapc_pd_base;
> + struct mtk_devapc_vio_info *vio_info;
> + const struct mtk_devapc_pd_offset *offset;
> + const struct mtk_devapc_vio_dbgs *vio_dbgs;
> +};
I think this structure should separate the constant part. The constant part is:
struct mtk_devapc_data {
const u32 vio_idx_num;
const struct mtk_devapc_pd_offset *offset; /* I would like to
remove struct mtk_devapc_pd_offset and directly put its member into
this structure */
const struct mtk_devapc_vio_dbgs *vio_dbgs; /* This may disappear */
};
And the context is:
struct mtk_devapc_context {
struct device *dev;
void __iomem *devapc_pd_base;
const struct mtk_devapc_data *data;
};
So when you define this, you would not waste memory to store non-constant data.
static const struct mtk_devapc_data devapc_mt6779 = {
.vio_idx_num = 510,
.offset = _pd_offset,
.vio_dbgs = _vio_dbgs,
};
Regards,
Chun-Kuang.
> +
> +#endif /* __MTK_DEVAPC_H__ */
> --
> 1.7.9.5
> ___
> Linux-mediatek mailing list
> linux-media...@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-mediatek
Re: [PATCH v4 2/2] soc: mediatek: add mtk-devapc driver
Hi, Neal:
Neal Liu 於 2020年7月31日 週五 上午10:52寫道：
>
> Hi Chun-Kuang,
>
> On Fri, 2020-07-31 at 00:14 +0800, Chun-Kuang Hu wrote:
> > Hi, Neal:
> >
> > Neal Liu 於 2020年7月29日 週三 下午4:29寫道：
> > >
> > > MediaTek bus fabric provides TrustZone security support and data
> > > protection to prevent slaves from being accessed by unexpected
> > > masters.
> > > The security violation is logged and sent to the processor for
> > > further analysis or countermeasures.
> > >
> > > Any occurrence of security violation would raise an interrupt, and
> > > it will be handled by mtk-devapc driver. The violation
> > > information is printed in order to find the murderer.
> > >
> > > Signed-off-by: Neal Liu
> > > ---
> >
> > [snip]
> >
> > > +
> > > +/*
> > > + * devapc_extract_vio_dbg - extract full violation information after
> > > doing
> > > + * shift mechanism.
> > > + */
> > > +static void devapc_extract_vio_dbg(struct mtk_devapc_context *ctx)
> > > +{
> > > + const struct mtk_devapc_vio_dbgs *vio_dbgs;
> > > + struct mtk_devapc_vio_info *vio_info;
> > > + void __iomem *vio_dbg0_reg;
> > > + void __iomem *vio_dbg1_reg;
> > > + u32 dbg0;
> > > +
> > > + vio_dbg0_reg = ctx->devapc_pd_base + ctx->offset->vio_dbg0;
> > > + vio_dbg1_reg = ctx->devapc_pd_base + ctx->offset->vio_dbg1;
> > > +
> > > + vio_dbgs = ctx->vio_dbgs;
> > > + vio_info = ctx->vio_info;
> > > +
> > > + /* Starts to 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;
> > > + vio_info->domain_id = (dbg0 & vio_dbgs->dmnid.mask) >>
> > > + vio_dbgs->dmnid.start;
> > > + vio_info->write = ((dbg0 & vio_dbgs->vio_w.mask) >>
> > > + vio_dbgs->vio_w.start) == 1;
> > > + vio_info->read = ((dbg0 & vio_dbgs->vio_r.mask) >>
> > > + vio_dbgs->vio_r.start) == 1;
> > > + vio_info->vio_addr_high = (dbg0 & vio_dbgs->addr_h.mask) >>
> > > + vio_dbgs->addr_h.start;
> >
> >
> > I would like to define the type of ctx->vio_info to be
> >
> > struct mtk_devapc_vio_dbgs {
> > u32 mstid:16;
> > u32 dmnid:6;
> > u32 vio_w:1;
> > u32 vio_r:1;
> > u32 addr_h:4;
> > u32 resv:4;
> > };
> >
> > so the code would like the simple way
> >
> > ctx->vio_info = (struct mtk_devapc_vio_dbgs)readl(vio_dbg1_reg);
> >
>
> This idea looks great! Is there any possible to pass the bit layout by
> DT data, and still make this operation simple?
> Why am I asking this question is because this bit layout is platform
> dependent.
I doubt these info would be in a single 32-bits register for all
future SoC. If they are not in single 32-bits register, you may create
a vio_dbgs_type in DT data, and the code may be
if (ctx->vio_dbgs_type == VIO_DBGS_TYPE_MT) {
ctx->vio_info = (struct mtk_devapc_vio_dbgs)readl(vio_dbg1_reg);
} else if (ctx->vio_dbgs_type == VIO_DBGS_TYPE_MT) {
ctx->vio_info->mstid = readl(vio_mstid_reg);
ctx->vio_info->dmnid = readl(vio_dmnid_reg);
ctx->vio_info->vio_w = readl(vio_vio_w_reg);
ctx->vio_info->vio_r = readl(vio_vio_r_reg);
}
I think we need not to consider how the future would be. Once the
second SoC driver is upstreaming, we could find out the best solution
for it.
Regards,
Chun-Kuang.
>
> > Regards,
> > Chun-Kuang.
> >
> > > +
> > > + devapc_vio_info_print(ctx);
> > > +}
> > > +
>
Re: [PATCH v4 2/2] soc: mediatek: add mtk-devapc driver
Hi, Neal:
Neal Liu 於 2020年7月31日 週五 上午10:44寫道：
>
> Hi Chun-Kuang,
>
>
> On Thu, 2020-07-30 at 00:38 +0800, Chun-Kuang Hu wrote:
> > Hi, Neal:
> >
> > Neal Liu 於 2020年7月29日 週三 下午4:29寫道：
> > >
> > > MediaTek bus fabric provides TrustZone security support and data
> > > protection to prevent slaves from being accessed by unexpected
> > > masters.
> > > The security violation is logged and sent to the processor for
> > > further analysis or countermeasures.
> > >
> > > Any occurrence of security violation would raise an interrupt, and
> > > it will be handled by mtk-devapc driver. The violation
> > > information is printed in order to find the murderer.
> > >
> > > Signed-off-by: Neal Liu
> > > ---
> >
> > [snip]
> >
> > > +
> > > +static int get_shift_group(struct mtk_devapc_context *ctx)
> > > +{
> > > + u32 vio_shift_sta;
> > > + void __iomem *reg;
> > > +
> > > + reg = ctx->devapc_pd_base + ctx->offset->vio_shift_sta;
> > > + vio_shift_sta = readl(reg);
> > > +
> > > + if (vio_shift_sta)
> > > + return __ffs(vio_shift_sta);
> > > +
> > > + return -EIO;
> > > +}
> >
> > get_shift_group() is a small function, I would like to merge this
> > function into sync_vio_dbg() to make code more simple.
>
> This function have a specific functionality. And it would make this
> driver more readability. I would like to keep it as a function. Is that
> okay for you?
After merge, the function would be:
static bool sync_min_shift_group_vio_dbg(struct mtk_devapc_context *ctx)
{
int min_shift_group;
int ret;
u32 val;
/* find the minimum shift group which has violation */
val = readl(ctx->devapc_pd_base + ctx->offset->vio_shift_sta);
if (!val)
return false;
min_shift_group = __ffs(val);
/* Assign the group to sync */
writel(0x1 << min_shift_group, ctx->devapc_pd_base +
ctx->offset->vio_shift_sel);
/* Start syncing */
writel(0x1, ctx->devapc_pd_base + ctx->offset->vio_shift_con);
ret = readl_poll_timeout(pd_vio_shift_con_reg, val, val == 0x3, 0,
PHY_DEVAPC_TIMEOUT);
if (ret) {
dev_err(ctx->dev, "%s: Shift violation info failed\n", __func__);
return false;
}
/* Stop syncing */
writel(0x0, ctx->devapc_pd_base + ctx->offset->vio_shift_con);
/* ? */
writel(0x1 << min_shift_group, ctx->devapc_pd_base +
ctx->offset->vio_shift_sta);
return true;
}
The whole function is to sync min_shift_group violation info, I don't
know why separate any part to an independent function? Any function
call would cause penalty on CPU performance, so I does not like to
break this function. After good comment, I think every body could
understand the function of each register.
After the merge, the code would be so simple as:
while(sync_min_shift_group_vio_dbg(ctx))
devapc_extract_vio_dbg(ctx);
>
> >
> > > +
> >
> > [snip]
> >
> > > +
> > > +#define PHY_DEVAPC_TIMEOUT 0x1
> > > +
> > > +/*
> > > + * sync_vio_dbg - do "shift" mechansim" to get full violation
> > > information.
> > > + *shift mechanism is depends on devapc hardware design.
> > > + *Mediatek devapc set multiple slaves as a group. When
> > > violation
> > > + *is triggered, violation info is kept inside devapc
> > > hardware.
> > > + *Driver should do shift mechansim to "shift" full
> > > violation
> > > + *info to VIO_DBGs registers.
> > > + *
> > > + */
> > > +static int sync_vio_dbg(struct mtk_devapc_context *ctx, u32 shift_bit)
> > > +{
> > > + void __iomem *pd_vio_shift_sta_reg;
> > > + void __iomem *pd_vio_shift_sel_reg;
> > > + void __iomem *pd_vio_shift_con_reg;
> > > + int ret;
> > > + u32 val;
> > > +
> > > + pd_vio_shift_sta_reg = ctx->devapc_pd_base +
> > > ctx->offset->vio_shift_sta;
> > > + pd_vio_shift_sel_reg = ctx->devapc_pd_base +
> > > ctx->offset->vio_shift_sel;
> > > + pd_vio_shift_con_reg = ctx->devapc_pd_base +
> > > ctx->offset->vio_shift_con;
> > > +
> > > + /* Enable shift mechansim */
> > > + writel(0x1 << shift_bit, pd_vio_shift_sel_reg);
> > > + writel(0x1, pd_vio_shift_con_reg);
> > > +
> > > + ret = readl_poll_timeout(pd_vio_shift_con_reg, val, val == 0x3, 0,
> > > +PHY_DEVAPC_TIMEOUT);
> > > + if (ret)
> > > + dev_err(ctx->dev, "%s: Shift violation info failed\n",
> > > __func__);
> > > +
> > > + /* Disable shift mechanism */
> > > + writel(0x0, pd_vio_shift_con_reg);
> > > + writel(0x0, pd_vio_shift_sel_reg);
> > > + writel(0x1 << shift_bit, pd_vio_shift_sta_reg);
> > > +
> > > + return ret;
> > > +}
> > > +
> >
> > [snip]
> >
> > > +
> > > +/*
> > > + * devapc_extract_vio_dbg - extract full violation information after
> > > doing
> > > + * shift mechanism.
> > > + */
> > > +static void devapc_extract_vio_dbg(struct mtk_devapc_context *ctx)
> > > +{
> > > + const struct
Re: [PATCH v4 2/2] soc: mediatek: add mtk-devapc driver
Hi Chun-Kuang,
On Fri, 2020-07-31 at 00:14 +0800, Chun-Kuang Hu wrote:
> Hi, Neal:
>
> Neal Liu 於 2020年7月29日 週三 下午4:29寫道：
> >
> > MediaTek bus fabric provides TrustZone security support and data
> > protection to prevent slaves from being accessed by unexpected
> > masters.
> > The security violation is logged and sent to the processor for
> > further analysis or countermeasures.
> >
> > Any occurrence of security violation would raise an interrupt, and
> > it will be handled by mtk-devapc driver. The violation
> > information is printed in order to find the murderer.
> >
> > Signed-off-by: Neal Liu
> > ---
>
> [snip]
>
> > +
> > +/*
> > + * devapc_extract_vio_dbg - extract full violation information after doing
> > + * shift mechanism.
> > + */
> > +static void devapc_extract_vio_dbg(struct mtk_devapc_context *ctx)
> > +{
> > + const struct mtk_devapc_vio_dbgs *vio_dbgs;
> > + struct mtk_devapc_vio_info *vio_info;
> > + void __iomem *vio_dbg0_reg;
> > + void __iomem *vio_dbg1_reg;
> > + u32 dbg0;
> > +
> > + vio_dbg0_reg = ctx->devapc_pd_base + ctx->offset->vio_dbg0;
> > + vio_dbg1_reg = ctx->devapc_pd_base + ctx->offset->vio_dbg1;
> > +
> > + vio_dbgs = ctx->vio_dbgs;
> > + vio_info = ctx->vio_info;
> > +
> > + /* Starts to 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;
> > + vio_info->domain_id = (dbg0 & vio_dbgs->dmnid.mask) >>
> > + vio_dbgs->dmnid.start;
> > + vio_info->write = ((dbg0 & vio_dbgs->vio_w.mask) >>
> > + vio_dbgs->vio_w.start) == 1;
> > + vio_info->read = ((dbg0 & vio_dbgs->vio_r.mask) >>
> > + vio_dbgs->vio_r.start) == 1;
> > + vio_info->vio_addr_high = (dbg0 & vio_dbgs->addr_h.mask) >>
> > + vio_dbgs->addr_h.start;
>
>
> I would like to define the type of ctx->vio_info to be
>
> struct mtk_devapc_vio_dbgs {
> u32 mstid:16;
> u32 dmnid:6;
> u32 vio_w:1;
> u32 vio_r:1;
> u32 addr_h:4;
> u32 resv:4;
> };
>
> so the code would like the simple way
>
> ctx->vio_info = (struct mtk_devapc_vio_dbgs)readl(vio_dbg1_reg);
>
This idea looks great! Is there any possible to pass the bit layout by
DT data, and still make this operation simple?
Why am I asking this question is because this bit layout is platform
dependent.
> Regards,
> Chun-Kuang.
>
> > +
> > + devapc_vio_info_print(ctx);
> > +}
> > +
Re: [PATCH v4 2/2] soc: mediatek: add mtk-devapc driver
Hi Chun-Kuang,
On Thu, 2020-07-30 at 06:47 +0800, Chun-Kuang Hu wrote:
> Hi, Neal:
>
> Neal Liu 於 2020年7月29日 週三 下午4:29寫道：
> >
> > MediaTek bus fabric provides TrustZone security support and data
> > protection to prevent slaves from being accessed by unexpected
> > masters.
> > The security violation is logged and sent to the processor for
> > further analysis or countermeasures.
> >
> > Any occurrence of security violation would raise an interrupt, and
> > it will be handled by mtk-devapc driver. The violation
> > information is printed in order to find the murderer.
> >
> > Signed-off-by: Neal Liu
>
> [snip]
>
> > +
> > +static void devapc_vio_info_print(struct mtk_devapc_context *ctx)
> > +{
> > + struct mtk_devapc_vio_info *vio_info = ctx->vio_info;
> > +
> > + /* Print violation information */
> > + if (vio_info->write)
> > + dev_info(ctx->dev, "Write Violation\n");
> > + else if (vio_info->read)
> > + dev_info(ctx->dev, "Read Violation\n");
> > +
> > + dev_info(ctx->dev, "Vio Addr:0x%x, High:0x%x, Bus ID:0x%x, Dom
> > ID:%x\n",
> > +vio_info->vio_addr, vio_info->vio_addr_high,
> > +vio_info->master_id, vio_info->domain_id);
> > +}
>
> devapc_vio_info_print() is small function and only called by
> devapc_extract_vio_dbg(), so I would like to merge this function into
> devapc_extract_vio_dbg() and you could drop struct mtk_devapc_vio_info
> because its member are all local variable.
This idea is okay for me. I'll update on next patch.
Thanks !
>
> > +
> > +/*
> > + * devapc_extract_vio_dbg - extract full violation information after doing
> > + * shift mechanism.
> > + */
> > +static void devapc_extract_vio_dbg(struct mtk_devapc_context *ctx)
> > +{
> > + const struct mtk_devapc_vio_dbgs *vio_dbgs;
> > + struct mtk_devapc_vio_info *vio_info;
> > + void __iomem *vio_dbg0_reg;
> > + void __iomem *vio_dbg1_reg;
> > + u32 dbg0;
> > +
> > + vio_dbg0_reg = ctx->devapc_pd_base + ctx->offset->vio_dbg0;
> > + vio_dbg1_reg = ctx->devapc_pd_base + ctx->offset->vio_dbg1;
> > +
> > + vio_dbgs = ctx->vio_dbgs;
> > + vio_info = ctx->vio_info;
> > +
> > + /* Starts to 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;
> > + vio_info->domain_id = (dbg0 & vio_dbgs->dmnid.mask) >>
> > + vio_dbgs->dmnid.start;
> > + vio_info->write = ((dbg0 & vio_dbgs->vio_w.mask) >>
> > + vio_dbgs->vio_w.start) == 1;
> > + vio_info->read = ((dbg0 & vio_dbgs->vio_r.mask) >>
> > + vio_dbgs->vio_r.start) == 1;
> > + vio_info->vio_addr_high = (dbg0 & vio_dbgs->addr_h.mask) >>
> > + vio_dbgs->addr_h.start;
> > +
> > + devapc_vio_info_print(ctx);
> > +}
> > +
>
> [snip]
>
> > +
> > +/*
> > + * start_devapc - unmask slave's irq to start receiving devapc violation.
> > + */
> > +static void start_devapc(struct mtk_devapc_context *ctx)
> > +{
> > + u32 vio_idx;
> > +
> > + for (vio_idx = 0; vio_idx < ctx->vio_idx_num; vio_idx++)
> > + mask_module_irq(ctx, vio_idx, false);
>
> Are these bits default true? If they are default false, you need not
> to setup it to false again.
It's default value is true, which is mask.
We try to unmask it to start service.
>
> > +}
> > +
>
> [snip]
>
> > diff --git a/drivers/soc/mediatek/mtk-devapc.h
> > b/drivers/soc/mediatek/mtk-devapc.h
> > new file mode 100644
> > index 000..7bd7e66
> > --- /dev/null
> > +++ b/drivers/soc/mediatek/mtk-devapc.h
> > @@ -0,0 +1,54 @@
> > +/* SPDX-License-Identifier: GPL-2.0 */
> > +/*
> > + * Copyright (C) 2020 MediaTek Inc.
> > + */
> > +
> > +#ifndef __MTK_DEVAPC_H__
> > +#define __MTK_DEVAPC_H__
> > +
> > +#define VIO_MOD_TO_REG_IND(m) ((m) / 32)
> > +#define VIO_MOD_TO_REG_OFF(m) ((m) % 32)
> > +
> > +struct mtk_devapc_pd_offset {
> > + u32 vio_mask;
> > + u32 vio_sta;
> > + u32 vio_dbg0;
> > + u32 vio_dbg1;
> > + u32 apc_con;
> > + u32 vio_shift_sta;
> > + u32 vio_shift_sel;
> > + u32 vio_shift_con;
> > +};
> > +
> > +struct mtk_devapc_vio_dbgs_desc {
> > + u32 mask;
> > + u32 start;
> > +};
> > +
> > +struct mtk_devapc_vio_dbgs {
> > + struct mtk_devapc_vio_dbgs_desc mstid;
> > + struct mtk_devapc_vio_dbgs_desc dmnid;
> > + struct mtk_devapc_vio_dbgs_desc vio_w;
> > + struct mtk_devapc_vio_dbgs_desc vio_r;
> > + struct mtk_devapc_vio_dbgs_desc addr_h;
> > +};
> > +
> > +struct mtk_devapc_vio_info {
> > + bool read;
> > + bool write;
> > + u32 vio_addr;
> > + u32 vio_addr_high;
> > + u32
Re: [PATCH v4 2/2] soc: mediatek: add mtk-devapc driver
Hi Chun-Kuang,
On Thu, 2020-07-30 at 00:38 +0800, Chun-Kuang Hu wrote:
> Hi, Neal:
>
> Neal Liu 於 2020年7月29日 週三 下午4:29寫道：
> >
> > MediaTek bus fabric provides TrustZone security support and data
> > protection to prevent slaves from being accessed by unexpected
> > masters.
> > The security violation is logged and sent to the processor for
> > further analysis or countermeasures.
> >
> > Any occurrence of security violation would raise an interrupt, and
> > it will be handled by mtk-devapc driver. The violation
> > information is printed in order to find the murderer.
> >
> > Signed-off-by: Neal Liu
> > ---
>
> [snip]
>
> > +
> > +static int get_shift_group(struct mtk_devapc_context *ctx)
> > +{
> > + u32 vio_shift_sta;
> > + void __iomem *reg;
> > +
> > + reg = ctx->devapc_pd_base + ctx->offset->vio_shift_sta;
> > + vio_shift_sta = readl(reg);
> > +
> > + if (vio_shift_sta)
> > + return __ffs(vio_shift_sta);
> > +
> > + return -EIO;
> > +}
>
> get_shift_group() is a small function, I would like to merge this
> function into sync_vio_dbg() to make code more simple.
This function have a specific functionality. And it would make this
driver more readability. I would like to keep it as a function. Is that
okay for you?
>
> > +
>
> [snip]
>
> > +
> > +#define PHY_DEVAPC_TIMEOUT 0x1
> > +
> > +/*
> > + * sync_vio_dbg - do "shift" mechansim" to get full violation information.
> > + *shift mechanism is depends on devapc hardware design.
> > + *Mediatek devapc set multiple slaves as a group. When
> > violation
> > + *is triggered, violation info is kept inside devapc
> > hardware.
> > + *Driver should do shift mechansim to "shift" full
> > violation
> > + *info to VIO_DBGs registers.
> > + *
> > + */
> > +static int sync_vio_dbg(struct mtk_devapc_context *ctx, u32 shift_bit)
> > +{
> > + void __iomem *pd_vio_shift_sta_reg;
> > + void __iomem *pd_vio_shift_sel_reg;
> > + void __iomem *pd_vio_shift_con_reg;
> > + int ret;
> > + u32 val;
> > +
> > + pd_vio_shift_sta_reg = ctx->devapc_pd_base +
> > ctx->offset->vio_shift_sta;
> > + pd_vio_shift_sel_reg = ctx->devapc_pd_base +
> > ctx->offset->vio_shift_sel;
> > + pd_vio_shift_con_reg = ctx->devapc_pd_base +
> > ctx->offset->vio_shift_con;
> > +
> > + /* Enable shift mechansim */
> > + writel(0x1 << shift_bit, pd_vio_shift_sel_reg);
> > + writel(0x1, pd_vio_shift_con_reg);
> > +
> > + ret = readl_poll_timeout(pd_vio_shift_con_reg, val, val == 0x3, 0,
> > +PHY_DEVAPC_TIMEOUT);
> > + if (ret)
> > + dev_err(ctx->dev, "%s: Shift violation info failed\n",
> > __func__);
> > +
> > + /* Disable shift mechanism */
> > + writel(0x0, pd_vio_shift_con_reg);
> > + writel(0x0, pd_vio_shift_sel_reg);
> > + writel(0x1 << shift_bit, pd_vio_shift_sta_reg);
> > +
> > + return ret;
> > +}
> > +
>
> [snip]
>
> > +
> > +/*
> > + * devapc_extract_vio_dbg - extract full violation information after doing
> > + * shift mechanism.
> > + */
> > +static void devapc_extract_vio_dbg(struct mtk_devapc_context *ctx)
> > +{
> > + const struct mtk_devapc_vio_dbgs *vio_dbgs;
> > + struct mtk_devapc_vio_info *vio_info;
> > + void __iomem *vio_dbg0_reg;
> > + void __iomem *vio_dbg1_reg;
> > + u32 dbg0;
> > +
> > + vio_dbg0_reg = ctx->devapc_pd_base + ctx->offset->vio_dbg0;
> > + vio_dbg1_reg = ctx->devapc_pd_base + ctx->offset->vio_dbg1;
> > +
> > + vio_dbgs = ctx->vio_dbgs;
> > + vio_info = ctx->vio_info;
> > +
> > + /* Starts to 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;
>
> What is master_id? How could we use it to debug? For example, if we
> get a master_id = 1, what should we do for this?
>
> > + vio_info->domain_id = (dbg0 & vio_dbgs->dmnid.mask) >>
> > + vio_dbgs->dmnid.start;
>
> What is domain_id? How could we use it to debug? For example, if we
> get a domain_id = 2, what should we do for this?
>
master_id and domain_id belongs our bus side-band signal info. It can
help us to find the violation master.
> > + vio_info->write = ((dbg0 & vio_dbgs->vio_w.mask) >>
> > + vio_dbgs->vio_w.start) == 1;
> > + vio_info->read = ((dbg0 & vio_dbgs->vio_r.mask) >>
> > + vio_dbgs->vio_r.start) == 1;
> > + vio_info->vio_addr_high = (dbg0 & vio_dbgs->addr_h.mask) >>
> > + vio_dbgs->addr_h.start;
>
> What is vio_addr_high? As I know all register address are 32 bits, is
>
Re: [PATCH v4 2/2] soc: mediatek: add mtk-devapc driver
Hi, Neal:
Neal Liu 於 2020年7月29日 週三 下午4:29寫道：
>
> MediaTek bus fabric provides TrustZone security support and data
> protection to prevent slaves from being accessed by unexpected
> masters.
> The security violation is logged and sent to the processor for
> further analysis or countermeasures.
>
> Any occurrence of security violation would raise an interrupt, and
> it will be handled by mtk-devapc driver. The violation
> information is printed in order to find the murderer.
>
> Signed-off-by: Neal Liu
> ---
[snip]
> +
> +/*
> + * devapc_extract_vio_dbg - extract full violation information after doing
> + * shift mechanism.
> + */
> +static void devapc_extract_vio_dbg(struct mtk_devapc_context *ctx)
> +{
> + const struct mtk_devapc_vio_dbgs *vio_dbgs;
> + struct mtk_devapc_vio_info *vio_info;
> + void __iomem *vio_dbg0_reg;
> + void __iomem *vio_dbg1_reg;
> + u32 dbg0;
> +
> + vio_dbg0_reg = ctx->devapc_pd_base + ctx->offset->vio_dbg0;
> + vio_dbg1_reg = ctx->devapc_pd_base + ctx->offset->vio_dbg1;
> +
> + vio_dbgs = ctx->vio_dbgs;
> + vio_info = ctx->vio_info;
> +
> + /* Starts to 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;
> + vio_info->domain_id = (dbg0 & vio_dbgs->dmnid.mask) >>
> + vio_dbgs->dmnid.start;
> + vio_info->write = ((dbg0 & vio_dbgs->vio_w.mask) >>
> + vio_dbgs->vio_w.start) == 1;
> + vio_info->read = ((dbg0 & vio_dbgs->vio_r.mask) >>
> + vio_dbgs->vio_r.start) == 1;
> + vio_info->vio_addr_high = (dbg0 & vio_dbgs->addr_h.mask) >>
> + vio_dbgs->addr_h.start;
I would like to define the type of ctx->vio_info to be
struct mtk_devapc_vio_dbgs {
u32 mstid:16;
u32 dmnid:6;
u32 vio_w:1;
u32 vio_r:1;
u32 addr_h:4;
u32 resv:4;
};
so the code would like the simple way
ctx->vio_info = (struct mtk_devapc_vio_dbgs)readl(vio_dbg1_reg);
Regards,
Chun-Kuang.
> +
> + devapc_vio_info_print(ctx);
> +}
> +
Re: [PATCH v4 2/2] soc: mediatek: add mtk-devapc driver
Hi, Neal:
Neal Liu 於 2020年7月29日 週三 下午4:29寫道：
>
> MediaTek bus fabric provides TrustZone security support and data
> protection to prevent slaves from being accessed by unexpected
> masters.
> The security violation is logged and sent to the processor for
> further analysis or countermeasures.
>
> Any occurrence of security violation would raise an interrupt, and
> it will be handled by mtk-devapc driver. The violation
> information is printed in order to find the murderer.
>
> Signed-off-by: Neal Liu
[snip]
> +
> +static void devapc_vio_info_print(struct mtk_devapc_context *ctx)
> +{
> + struct mtk_devapc_vio_info *vio_info = ctx->vio_info;
> +
> + /* Print violation information */
> + if (vio_info->write)
> + dev_info(ctx->dev, "Write Violation\n");
> + else if (vio_info->read)
> + dev_info(ctx->dev, "Read Violation\n");
> +
> + dev_info(ctx->dev, "Vio Addr:0x%x, High:0x%x, Bus ID:0x%x, Dom
> ID:%x\n",
> +vio_info->vio_addr, vio_info->vio_addr_high,
> +vio_info->master_id, vio_info->domain_id);
> +}
devapc_vio_info_print() is small function and only called by
devapc_extract_vio_dbg(), so I would like to merge this function into
devapc_extract_vio_dbg() and you could drop struct mtk_devapc_vio_info
because its member are all local variable.
> +
> +/*
> + * devapc_extract_vio_dbg - extract full violation information after doing
> + * shift mechanism.
> + */
> +static void devapc_extract_vio_dbg(struct mtk_devapc_context *ctx)
> +{
> + const struct mtk_devapc_vio_dbgs *vio_dbgs;
> + struct mtk_devapc_vio_info *vio_info;
> + void __iomem *vio_dbg0_reg;
> + void __iomem *vio_dbg1_reg;
> + u32 dbg0;
> +
> + vio_dbg0_reg = ctx->devapc_pd_base + ctx->offset->vio_dbg0;
> + vio_dbg1_reg = ctx->devapc_pd_base + ctx->offset->vio_dbg1;
> +
> + vio_dbgs = ctx->vio_dbgs;
> + vio_info = ctx->vio_info;
> +
> + /* Starts to 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;
> + vio_info->domain_id = (dbg0 & vio_dbgs->dmnid.mask) >>
> + vio_dbgs->dmnid.start;
> + vio_info->write = ((dbg0 & vio_dbgs->vio_w.mask) >>
> + vio_dbgs->vio_w.start) == 1;
> + vio_info->read = ((dbg0 & vio_dbgs->vio_r.mask) >>
> + vio_dbgs->vio_r.start) == 1;
> + vio_info->vio_addr_high = (dbg0 & vio_dbgs->addr_h.mask) >>
> + vio_dbgs->addr_h.start;
> +
> + devapc_vio_info_print(ctx);
> +}
> +
[snip]
> +
> +/*
> + * start_devapc - unmask slave's irq to start receiving devapc violation.
> + */
> +static void start_devapc(struct mtk_devapc_context *ctx)
> +{
> + u32 vio_idx;
> +
> + for (vio_idx = 0; vio_idx < ctx->vio_idx_num; vio_idx++)
> + mask_module_irq(ctx, vio_idx, false);
Are these bits default true? If they are default false, you need not
to setup it to false again.
> +}
> +
[snip]
> diff --git a/drivers/soc/mediatek/mtk-devapc.h
> b/drivers/soc/mediatek/mtk-devapc.h
> new file mode 100644
> index 000..7bd7e66
> --- /dev/null
> +++ b/drivers/soc/mediatek/mtk-devapc.h
> @@ -0,0 +1,54 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +/*
> + * Copyright (C) 2020 MediaTek Inc.
> + */
> +
> +#ifndef __MTK_DEVAPC_H__
> +#define __MTK_DEVAPC_H__
> +
> +#define VIO_MOD_TO_REG_IND(m) ((m) / 32)
> +#define VIO_MOD_TO_REG_OFF(m) ((m) % 32)
> +
> +struct mtk_devapc_pd_offset {
> + u32 vio_mask;
> + u32 vio_sta;
> + u32 vio_dbg0;
> + u32 vio_dbg1;
> + u32 apc_con;
> + u32 vio_shift_sta;
> + u32 vio_shift_sel;
> + u32 vio_shift_con;
> +};
> +
> +struct mtk_devapc_vio_dbgs_desc {
> + u32 mask;
> + u32 start;
> +};
> +
> +struct mtk_devapc_vio_dbgs {
> + struct mtk_devapc_vio_dbgs_desc mstid;
> + struct mtk_devapc_vio_dbgs_desc dmnid;
> + struct mtk_devapc_vio_dbgs_desc vio_w;
> + struct mtk_devapc_vio_dbgs_desc vio_r;
> + struct mtk_devapc_vio_dbgs_desc addr_h;
> +};
> +
> +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_context {
> + struct device *dev;
> + u32 vio_idx_num;
> + void __iomem *devapc_pd_base;
> + struct mtk_devapc_vio_info *vio_info;
> + const struct mtk_devapc_pd_offset *offset;
> + const struct mtk_devapc_vio_dbgs *vio_dbgs;
> +};
> +
> +#endif /* __MTK_DEVAPC_H__ */
Data in this header file is only used in mtk-devapc.c and mtk-devapc.c
is a small file, so I think it's better to move data in header file
into .c
Re: [PATCH v4 2/2] soc: mediatek: add mtk-devapc driver
Hi, Neal:
Neal Liu 於 2020年7月29日 週三 下午4:29寫道：
>
> MediaTek bus fabric provides TrustZone security support and data
> protection to prevent slaves from being accessed by unexpected
> masters.
> The security violation is logged and sent to the processor for
> further analysis or countermeasures.
>
> Any occurrence of security violation would raise an interrupt, and
> it will be handled by mtk-devapc driver. The violation
> information is printed in order to find the murderer.
>
> Signed-off-by: Neal Liu
> ---
[snip]
> +
> +static int get_shift_group(struct mtk_devapc_context *ctx)
> +{
> + u32 vio_shift_sta;
> + void __iomem *reg;
> +
> + reg = ctx->devapc_pd_base + ctx->offset->vio_shift_sta;
> + vio_shift_sta = readl(reg);
> +
> + if (vio_shift_sta)
> + return __ffs(vio_shift_sta);
> +
> + return -EIO;
> +}
get_shift_group() is a small function, I would like to merge this
function into sync_vio_dbg() to make code more simple.
> +
[snip]
> +
> +#define PHY_DEVAPC_TIMEOUT 0x1
> +
> +/*
> + * sync_vio_dbg - do "shift" mechansim" to get full violation information.
> + *shift mechanism is depends on devapc hardware design.
> + *Mediatek devapc set multiple slaves as a group. When
> violation
> + *is triggered, violation info is kept inside devapc
> hardware.
> + *Driver should do shift mechansim to "shift" full violation
> + *info to VIO_DBGs registers.
> + *
> + */
> +static int sync_vio_dbg(struct mtk_devapc_context *ctx, u32 shift_bit)
> +{
> + void __iomem *pd_vio_shift_sta_reg;
> + void __iomem *pd_vio_shift_sel_reg;
> + void __iomem *pd_vio_shift_con_reg;
> + int ret;
> + u32 val;
> +
> + pd_vio_shift_sta_reg = ctx->devapc_pd_base +
> ctx->offset->vio_shift_sta;
> + pd_vio_shift_sel_reg = ctx->devapc_pd_base +
> ctx->offset->vio_shift_sel;
> + pd_vio_shift_con_reg = ctx->devapc_pd_base +
> ctx->offset->vio_shift_con;
> +
> + /* Enable shift mechansim */
> + writel(0x1 << shift_bit, pd_vio_shift_sel_reg);
> + writel(0x1, pd_vio_shift_con_reg);
> +
> + ret = readl_poll_timeout(pd_vio_shift_con_reg, val, val == 0x3, 0,
> +PHY_DEVAPC_TIMEOUT);
> + if (ret)
> + dev_err(ctx->dev, "%s: Shift violation info failed\n",
> __func__);
> +
> + /* Disable shift mechanism */
> + writel(0x0, pd_vio_shift_con_reg);
> + writel(0x0, pd_vio_shift_sel_reg);
> + writel(0x1 << shift_bit, pd_vio_shift_sta_reg);
> +
> + return ret;
> +}
> +
[snip]
> +
> +/*
> + * devapc_extract_vio_dbg - extract full violation information after doing
> + * shift mechanism.
> + */
> +static void devapc_extract_vio_dbg(struct mtk_devapc_context *ctx)
> +{
> + const struct mtk_devapc_vio_dbgs *vio_dbgs;
> + struct mtk_devapc_vio_info *vio_info;
> + void __iomem *vio_dbg0_reg;
> + void __iomem *vio_dbg1_reg;
> + u32 dbg0;
> +
> + vio_dbg0_reg = ctx->devapc_pd_base + ctx->offset->vio_dbg0;
> + vio_dbg1_reg = ctx->devapc_pd_base + ctx->offset->vio_dbg1;
> +
> + vio_dbgs = ctx->vio_dbgs;
> + vio_info = ctx->vio_info;
> +
> + /* Starts to 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;
What is master_id? How could we use it to debug? For example, if we
get a master_id = 1, what should we do for this?
> + vio_info->domain_id = (dbg0 & vio_dbgs->dmnid.mask) >>
> + vio_dbgs->dmnid.start;
What is domain_id? How could we use it to debug? For example, if we
get a domain_id = 2, what should we do for this?
> + vio_info->write = ((dbg0 & vio_dbgs->vio_w.mask) >>
> + vio_dbgs->vio_w.start) == 1;
> + vio_info->read = ((dbg0 & vio_dbgs->vio_r.mask) >>
> + vio_dbgs->vio_r.start) == 1;
> + vio_info->vio_addr_high = (dbg0 & vio_dbgs->addr_h.mask) >>
> + vio_dbgs->addr_h.start;
What is vio_addr_high? As I know all register address are 32 bits, is
vio_addr_high the address above 32 bits?
> +
> + devapc_vio_info_print(ctx);
> +}
> +
[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,
> + struct mtk_devapc_context *ctx)
> +{
> + u32 vio_idx;
> +
> + /*
> +* Mask slave's irq before clearing vio status.
> +* Must do it to avoid nested
