On Tue, Feb 15, 2011 at 5:10 PM, David Cohen <dacohen@xxxxxxxxx> wrote:
> On Tue, Feb 15, 2011 at 4:48 PM, Hiroshi DOYU <Hiroshi.DOYU@xxxxxxxxx> wrote:
>> Hi David,
>
> Hi Hiroshi,
>
>>
>> Sorry for a bit late reply....
>
> You're just in time. :)
>
>>
>> From: David Cohen <dacohen@xxxxxxxxx>
>> Subject: [PATCH v2 1/1] OMAP: IOMMU: add support to callback during fault handling
>> Date: Tue, 15 Feb 2011 16:36:31 +0200
>>
>>> Add support to register a callback for IOMMU fault situations. Drivers using
>>> IOMMU module might want to be informed when such errors happen in order to
>>> debug it or react.
>>>
>>> Signed-off-by: David Cohen <dacohen@xxxxxxxxx>
>>> ---
>>> Âarch/arm/mach-omap2/iommu2.c      Â|  21 +++++++++++++--
>>> Âarch/arm/plat-omap/include/plat/iommu.h | Â 15 ++++++++++-
>>> Âarch/arm/plat-omap/iommu.c       Â|  41 ++++++++++++++++++++++++++++---
>>> Â3 files changed, 69 insertions(+), 8 deletions(-)
>>>
>>> diff --git a/arch/arm/mach-omap2/iommu2.c b/arch/arm/mach-omap2/iommu2.c
>>> index 14ee686..504310d 100644
>>> --- a/arch/arm/mach-omap2/iommu2.c
>>> +++ b/arch/arm/mach-omap2/iommu2.c
>>> @@ -143,10 +143,10 @@ 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 stat, da;
>>> + Â Â u32 stat, da, errs;
>>> Â Â Â const char *err_msg[] = {
>>> Â Â Â Â Â Â Â "tlb miss",
>>> Â Â Â Â Â Â Â "translation fault",
>>> @@ -157,8 +157,10 @@ static u32 omap2_iommu_fault_isr(struct iommu *obj, u32 *ra)
>>>
>>> Â Â Â 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;
>>> @@ -171,6 +173,19 @@ static u32 omap2_iommu_fault_isr(struct iommu *obj, u32 *ra)
>>> Â Â Â }
>>> Â Â Â printk("\n");
>>>
>>> + Â Â errs = 0;
>>> + Â Â if (stat & MMU_IRQ_TLBMISS)
>>> + Â Â Â Â Â Â errs |= OMAP_IOMMU_ERR_TLB_MISS;
>>> + Â Â if (stat & MMU_IRQ_TRANSLATIONFAULT)
>>> + Â Â Â Â Â Â errs |= OMAP_IOMMU_ERR_TRANS_FAULT;
>>> + Â Â if (stat & MMU_IRQ_EMUMISS)
>>> + Â Â Â Â Â Â errs |= OMAP_IOMMU_ERR_EMU_MISS;
>>> + Â Â if (stat & MMU_IRQ_TABLEWALKFAULT)
>>> + Â Â Â Â Â Â errs |= OMAP_IOMMU_ERR_TBLWALK_FAULT;
>>> + Â Â if (stat & MMU_IRQ_MULTIHITFAULT)
>>> + Â Â Â Â Â Â errs |= OMAP_IOMMU_ERR_MULTIHIT_FAULT;
>>> + Â Â *iommu_errs = errs;
>>> +
>>> Â Â Â iommu_write_reg(obj, stat, MMU_IRQSTATUS);
>>>
>>> Â Â Â return stat;
>>> diff --git a/arch/arm/plat-omap/include/plat/iommu.h b/arch/arm/plat-omap/include/plat/iommu.h
>>> index 19cbb5e..5a2475f 100644
>>> --- a/arch/arm/plat-omap/include/plat/iommu.h
>>> +++ b/arch/arm/plat-omap/include/plat/iommu.h
>>> @@ -31,6 +31,7 @@ struct iommu {
>>>    struct clk   Â*clk;
>>>    void __iomem  Â*regbase;
>>>    struct device  *dev;
>>> +   void      Â*fault_cb_priv;
>>>
>>>    unsigned int  Ârefcount;
>>>    struct mutex  Âiommu_lock;   /* global for this whole object */
>>> @@ -48,6 +49,7 @@ struct iommu {
>>>    struct mutex      Âmmap_lock; /* protect mmap */
>>>
>>> Â Â Â int (*isr)(struct iommu *obj);
>>> + Â Â void (*fault_cb)(struct iommu *obj, u32 da, u32 iommu_errs, void *priv);
>>
>> What about making use of (*isr)() for fault call back as well?
>>
>> Basic concept is that, client can decide how to deal with iommu
>> fault. For example, for advanced case, client wants daynamic loading of
>> TLB(or PTE), or for ISP case, clients just want more appropriate fault
>> reporting. This (*isr)() could be used in such flexibility.
>
> In this case, it seems we can re-use it.
>
>>
>> Â 785 Â * Â Â ÂDevice IOMMU generic operations
>> Â 786 Â */
>> Â 787 Âstatic irqreturn_t iommu_fault_handler(int irq, void *data)
>> Â 788 Â{
>> Â 789 Â Â Â Â Âu32 stat, da;
>> Â 790 Â Â Â Â Âu32 *iopgd, *iopte;
>> Â 791 Â Â Â Â Âint err = -EIO;
>> Â 792 Â Â Â Â Âstruct iommu *obj = data;
>> Â 793
>> Â 794 Â Â Â Â Âif (!obj->refcount)
>> Â 795 Â Â Â Â Â Â Â Â Âreturn IRQ_NONE;
>> Â 796
>> Â 797 Â Â Â Â Â/* Dynamic loading TLB or PTE */
>> Â 798 Â Â Â Â Âif (obj->isr)
>> Â 799 Â Â Â Â Â Â Â Â Âerr = obj->isr(obj);
>> Â 800
>> Â 801 Â Â Â Â Âif (!err)
>> Â 802 Â Â Â Â Â Â Â Â Âreturn IRQ_HANDLED;
>> Â 803
>> Â 804 Â Â Â Â Âclk_enable(obj->clk);
>> Â 805 Â Â Â Â Âstat = iommu_report_fault(obj, &da);
>> Â 806 Â Â Â Â Âclk_disable(obj->clk);
>> Â 807 Â Â Â Â Âif (!stat)
>> Â 808 Â Â Â Â Â Â Â Â Âreturn IRQ_HANDLED;
>> Â 809
>> Â 810 Â Â Â Â Âiommu_disable(obj);
>>
>> I guess that this modifying the above code could make (*isr)()
>> flexible to be used for any purpose for clients? For me, having both
>> following may be a bit residual.
>
There are few possible issues in this case. (*isr)() is meant to
replace the generic fault handler on iommu, but the fault callback
isn't. Basically fault callback needs to call specific iommu fault
report to know faulty 'da' and iommu errors, so part of the generic
fault handler must be called.
One possible solution is always call specific fault report before
(*isr)(). IMO it makes sense as (*isr)() might want to know at least
the current faults.
But in this case, specific fault report should not print anything else
as only the upper layer and user will know when a fault is actually an
error or not. So, they're responsible for error messages.
I'm sending a v3 with these changes. Comments will be welcome.
Br,
David
> I agree. We need to add 'void *isr_priv' to iommu struct and the
> function to register isr.
> I'll send a v3 soon.
>
> Regards,
>
> David
>
>>
>>> Â Â Â int (*isr)(struct iommu *obj);
>>> + Â Â void (*fault_cb)(struct iommu *obj, u32 da, u32 iommu_errs, void *priv);
>>
>
--
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
