Hi David,
On Mon, Jan 31, 2011 at 11:25 AM, David Cohen <david.cohen@xxxxxxxxx> wrote:
> IOMMU error messages are duplicated. They're printed on IOMMU specific
> layer for OMAP2,3 and once again on the above layer. With this patch,
> the error message is printed on the above layer only.
So, you say they are duplicated, previously the type of fault was
printed in the omap2,3 code and the addresses involving the error
printed on plat code... with this patch both messages are printed on
plat code, I don't see where the duplication/fix is about.
AFAIK, your patch removes this line:
- dev_err(obj->dev, "%s:\tda:%08x ", __func__, da);
Which I assume is the one being printed again in plat code, right?
> Signed-off-by: David Cohen <david.cohen@xxxxxxxxx>
> ---
> arch/arm/mach-omap2/iommu2.c | 33 +++++++++++++--------------
> arch/arm/plat-omap/include/plat/iommu.h | 2 +-
> arch/arm/plat-omap/iommu.c | 36 ++++++++++++++++++++----------
> 3 files changed, 41 insertions(+), 30 deletions(-)
>
> diff --git a/arch/arm/mach-omap2/iommu2.c b/arch/arm/mach-omap2/iommu2.c
> index 14ee686..bb3d75b 100644
> --- a/arch/arm/mach-omap2/iommu2.c
> +++ b/arch/arm/mach-omap2/iommu2.c
> @@ -143,33 +143,32 @@ static void omap2_iommu_set_twl(struct iommu *obj, bool on)
> __iommu_set_twl(obj, false);
> }
>
> -static u32 omap2_iommu_fault_isr(struct iommu *obj, u32 *ra)
> +static u32 omap2_iommu_fault_isr(struct iommu *obj, u32 *ra, u32 *iommu_errs)
> {
> - int i;
> + u32 errs = 0;
> u32 stat, da;
> - const char *err_msg[] = {
> - "tlb miss",
> - "translation fault",
> - "emulation miss",
> - "table walk fault",
> - "multi hit fault",
> - };
AFAIU, this code living in mach-omap2, means that this errors are
common for omap2,3,4 which they are. Does moving them to plat code
implies that all omap platforms expect these errors?
> stat = iommu_read_reg(obj, MMU_IRQSTATUS);
> stat &= MMU_IRQ_MASK;
> - if (!stat)
> + if (!stat) {
> + *iommu_errs = 0;
> return 0;
> + }
>
> da = iommu_read_reg(obj, MMU_FAULT_AD);
> *ra = da;
>
> - dev_err(obj->dev, "%s:\tda:%08x ", __func__, da);
> -
> - for (i = 0; i < ARRAY_SIZE(err_msg); i++) {
> - if (stat & (1 << i))
> - printk("%s ", err_msg[i]);
> - }
> - printk("\n");
> + if (stat & MMU_IRQ_TLBMISS)
> + errs |= IOMMU_ERR_TLB_MISS;
> + if (stat & MMU_IRQ_TRANSLATIONFAULT)
> + errs |= IOMMU_ERR_TRANS_FAULT;
> + if (stat & MMU_IRQ_EMUMISS)
> + errs |= IOMMU_ERR_EMU_MISS;
> + if (stat & MMU_IRQ_TABLEWALKFAULT)
> + errs |= IOMMU_ERR_TBLWALK_FAULT;
> + if (stat & MMU_IRQ_MULTIHITFAULT)
> + errs |= IOMMU_ERR_MULTIHIT_FAULT;
> + *iommu_errs = errs;
Is there any point in doing this?
Basically: *iommu_errs = stat, given that the mask values and the
error defines are the same for each case.
Besides I haven't seen two errors trigger a single interrupt.
...
>
> - iopte = iopte_offset(iopgd, da);
> -
> - dev_err(obj->dev, "%s: da:%08x pgd:%p *pgd:%08x pte:%p *pte:%08x\n",
> - __func__, da, iopgd, *iopgd, iopte, *iopte);
> + for (i = 0; i < ARRAY_SIZE(err_msg); i++) {
> + if (errs & (1 << i))
> + printk(KERN_CONT " %s", err_msg[i]);
> + }
> + printk("\n");
I think we should get rid of the "printks".
Regards,
Omar
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
