dspbridge: add mmufault support
in [Kernel]
|
Prev: [GIT PULL net-2.6] vhost-net: more error handling fixes
Next: [PATCH v2]kernel.h Move preprocessor #warning about using kernel headers in userpsace to types.h
From: Kanigeri, Hari on 1 Jul 2010 13:20 Hi Fernando, > +int mmu_fault_isr(struct iommu *mmu) > > -/* > - * ======== mmu_check_if_fault ======= > - * Check to see if MMU Fault is valid TLB miss from DSP > - * Note: This function is called from an ISR > - */ > -static bool mmu_check_if_fault(struct bridge_dev_context *dev_context) > { > + struct deh_mgr *dm; > + u32 da; > + > + dev_get_deh_mgr(dev_get_first(), &dm); > + > + if (!dm) > + return -EPERM; > + > + da = iommu_read_reg(mmu, MMU_FAULT_AD); > + iommu_write_reg(mmu, 0, MMU_IRQENABLE); -- Isn't the MMU already enabled at this point when the function callback is called by iommu ? > + dm->err_info.dw_val1 = da; > + tasklet_schedule(&dm->dpc_tasklet); -- The iommu fault isr disables the IOMMU at the end of the fault handler, so by the time your tasklet is scheduled you might have the MMU in a disabled state. Looks to me either this requires change in iommu to remove the disable part or enable the MMU in the tasklet instead of doing it early in mmu_fault_isr. Thank you, Best regards, Hari -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo(a)vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
From: Guzman Lugo, Fernando on 1 Jul 2010 14:00 Hi Hari, > -----Original Message----- > From: Kanigeri, Hari > Sent: Thursday, July 01, 2010 12:17 PM > To: Guzman Lugo, Fernando; linux-omap(a)vger.kernel.org; linux- > kernel(a)vger.kernel.org > Cc: ohad(a)wizery.com; hiroshi.doyo(a)nokia.com; ameya.palande(a)nokia.com; > felipe.contreras(a)nokia.com; Guzman Lugo, Fernando > Subject: RE: [PATCHv3 5/9] dspbridge: add mmufault support > > Hi Fernando, > > > +int mmu_fault_isr(struct iommu *mmu) > > > > -/* > > - * ======== mmu_check_if_fault ======= > > - * Check to see if MMU Fault is valid TLB miss from DSP > > - * Note: This function is called from an ISR > > - */ > > -static bool mmu_check_if_fault(struct bridge_dev_context *dev_context) > > { > > + struct deh_mgr *dm; > > + u32 da; > > + > > + dev_get_deh_mgr(dev_get_first(), &dm); > > + > > + if (!dm) > > + return -EPERM; > > + > > + da = iommu_read_reg(mmu, MMU_FAULT_AD); > > + iommu_write_reg(mmu, 0, MMU_IRQENABLE); > > -- Isn't the MMU already enabled at this point when the function callback > is called by iommu ? This line is actually disabling the interrupts. I am writing "0x0" in the MMU_IRQENABLE. > > > + dm->err_info.dw_val1 = da; > > + tasklet_schedule(&dm->dpc_tasklet); > > -- The iommu fault isr disables the IOMMU at the end of the fault handler, > so by the time your tasklet is scheduled you might have the MMU in a > disabled state. Looks to me either this requires change in iommu to remove > the disable part or enable the MMU in the tasklet instead of doing it > early in mmu_fault_isr. I am returning 0 in the callback function, that means the callback function has managed the fault and the mmu_fault_isr does not do anything else if (obj->isr) err = obj->isr(obj); if (!err) return IRQ_HANDLED; it is working for me without not modifications in the iommu_fault_handler function. Thanks for the comments, Fernando. > > Thank you, > Best regards, > Hari -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo(a)vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
From: Kanigeri, Hari on 1 Jul 2010 14:10 > > > + da = iommu_read_reg(mmu, MMU_FAULT_AD); > > > + iommu_write_reg(mmu, 0, MMU_IRQENABLE); > > > > -- Isn't the MMU already enabled at this point when the function > callback > > is called by iommu ? > > This line is actually disabling the interrupts. I am writing "0x0" in the > MMU_IRQENABLE. -- oops ! sorry about that. Didn't pay attention to 0x0. Yes, this should work. Best regards, Hari -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo(a)vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
From: Guzman Lugo, Fernando on 1 Jul 2010 14:30 > -----Original Message----- > From: Kanigeri, Hari > Sent: Thursday, July 01, 2010 1:04 PM > To: Guzman Lugo, Fernando; linux-omap(a)vger.kernel.org; linux- > kernel(a)vger.kernel.org > Cc: ohad(a)wizery.com; Hiroshi DOYU; ameya.palande(a)nokia.com; > felipe.contreras(a)nokia.com > Subject: RE: [PATCHv3 5/9] dspbridge: add mmufault support > > > > > + da = iommu_read_reg(mmu, MMU_FAULT_AD); > > > > + iommu_write_reg(mmu, 0, MMU_IRQENABLE); > > > > > > -- Isn't the MMU already enabled at this point when the function > > callback > > > is called by iommu ? > > > > This line is actually disabling the interrupts. I am writing "0x0" in > the > > MMU_IRQENABLE. > > -- oops ! sorry about that. Didn't pay attention to 0x0. Yes, this should > work. Not problem :). Could you please comment in the second set of patches I sent? The previous ones have version 3 which is not correct, that was a version I was using internally, but the patches shouldn't have sent to mailing list with that version. Because it is the first version posted. Thanks and regards, Fernando. > > Best regards, > Hari -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo(a)vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
From: Hiroshi DOYU on 2 Jul 2010 02:30
Hi Fernando, From: ext Fernando Guzman Lugo <x0095840(a)ti.com> Subject: [PATCH 5/9] dspbridge: add mmufault support Date: Thu, 1 Jul 2010 02:20:56 +0200 > With changes for iommu migration mmu fault report and dsp track > dump is broken, this patch fixes that. > > Signed-off-by: Fernando Guzman Lugo <x0095840(a)ti.com> > --- > drivers/dsp/bridge/core/mmu_fault.c | 93 ++++++--------------------------- > drivers/dsp/bridge/core/mmu_fault.h | 5 +- > drivers/dsp/bridge/core/tiomap3430.c | 2 + > drivers/dsp/bridge/core/ue_deh.c | 31 +++++------- > 4 files changed, 34 insertions(+), 97 deletions(-) > > diff --git a/drivers/dsp/bridge/core/mmu_fault.c b/drivers/dsp/bridge/core/mmu_fault.c > index 5c0124f..d991c6a 100644 > --- a/drivers/dsp/bridge/core/mmu_fault.c > +++ b/drivers/dsp/bridge/core/mmu_fault.c > @@ -23,9 +23,12 @@ > /* ----------------------------------- Trace & Debug */ > #include <dspbridge/host_os.h> > #include <dspbridge/dbc.h> > +#include <plat/iommu.h> > > /* ----------------------------------- OS Adaptation Layer */ > #include <dspbridge/drv.h> > +#include <dspbridge/dev.h> > + > > /* ----------------------------------- Link Driver */ > #include <dspbridge/dspdeh.h> > @@ -40,11 +43,6 @@ > #include "_tiomap.h" > #include "mmu_fault.h" > > -static u32 dmmu_event_mask; > -u32 fault_addr; > - > -static bool mmu_check_if_fault(struct bridge_dev_context *dev_context); > - > /* > * ======== mmu_fault_dpc ======== > * Deferred procedure call to handle DSP MMU fault. > @@ -62,78 +60,21 @@ void mmu_fault_dpc(IN unsigned long pRefData) > * ======== mmu_fault_isr ======== > * ISR to be triggered by a DSP MMU fault interrupt. > */ > -irqreturn_t mmu_fault_isr(int irq, IN void *pRefData) > -{ > - struct deh_mgr *deh_mgr_obj = (struct deh_mgr *)pRefData; > - struct bridge_dev_context *dev_context; > - struct cfg_hostres *resources; > - > - DBC_REQUIRE(irq == INT_DSP_MMU_IRQ); > - DBC_REQUIRE(deh_mgr_obj); > - > - if (deh_mgr_obj) { > - > - dev_context = > - (struct bridge_dev_context *)deh_mgr_obj->hbridge_context; > - > - resources = dev_context->resources; > - > - if (!resources) { > - dev_dbg(bridge, "%s: Failed to get Host Resources\n", > - __func__); > - return IRQ_HANDLED; > - } > - if (mmu_check_if_fault(dev_context)) { > - printk(KERN_INFO "***** DSPMMU FAULT ***** IRQStatus " > - "0x%x\n", dmmu_event_mask); > - printk(KERN_INFO "***** DSPMMU FAULT ***** fault_addr " > - "0x%x\n", fault_addr); > - /* > - * Schedule a DPC directly. In the future, it may be > - * necessary to check if DSP MMU fault is intended for > - * Bridge. > - */ > - tasklet_schedule(&deh_mgr_obj->dpc_tasklet); > - > - /* Reset err_info structure before use. */ > - deh_mgr_obj->err_info.dw_err_mask = DSP_MMUFAULT; > - deh_mgr_obj->err_info.dw_val1 = fault_addr >> 16; > - deh_mgr_obj->err_info.dw_val2 = fault_addr & 0xFFFF; > - deh_mgr_obj->err_info.dw_val3 = 0L; > - /* Disable the MMU events, else once we clear it will > - * start to raise INTs again */ > - hw_mmu_event_disable(resources->dw_dmmu_base, > - HW_MMU_TRANSLATION_FAULT); > - } else { > - hw_mmu_event_disable(resources->dw_dmmu_base, > - HW_MMU_ALL_INTERRUPTS); > - } > - } > - return IRQ_HANDLED; > -} > +int mmu_fault_isr(struct iommu *mmu) > > -/* > - * ======== mmu_check_if_fault ======== > - * Check to see if MMU Fault is valid TLB miss from DSP > - * Note: This function is called from an ISR > - */ > -static bool mmu_check_if_fault(struct bridge_dev_context *dev_context) > { > + struct deh_mgr *dm; > + u32 da; > + > + dev_get_deh_mgr(dev_get_first(), &dm); > + > + if (!dm) > + return -EPERM; > + > + da = iommu_read_reg(mmu, MMU_FAULT_AD); > + iommu_write_reg(mmu, 0, MMU_IRQENABLE); > + dm->err_info.dw_val1 = da; > + tasklet_schedule(&dm->dpc_tasklet); I think that the following "iommu_disable()" does disabling MMU. Modified arch/arm/plat-omap/iommu.c diff --git a/arch/arm/plat-omap/iommu.c b/arch/arm/plat-omap/iommu.c index a202a2c..17407f1 100644 --- a/arch/arm/plat-omap/iommu.c +++ b/arch/arm/plat-omap/iommu.c @@ -800,7 +800,7 @@ static irqreturn_t iommu_fault_handler(int irq, void *data) if (!stat) return IRQ_HANDLED; - iommu_disable(obj); + iommu_disable(obj); <- HERE! iopgd = iopgd_offset(obj, da); You can find the latest omap iommu code from: git://gitorious.org/~doyu/lk/mainline.git v2.6.35-rc3-iommu-for-next Also I just thought that, passing 'da' to user callback might be cleaner as below. Then you can avoid the direct access to IOMMU registers. Basically we should avoid the direct access to IOMMU registers from client. If you have to, then it would be better to modify omap iommu code itself. Modified arch/arm/plat-omap/iommu.c diff --git a/arch/arm/plat-omap/iommu.c b/arch/arm/plat-omap/iommu.c index a202a2c..64d918e 100644 --- a/arch/arm/plat-omap/iommu.c +++ b/arch/arm/plat-omap/iommu.c @@ -787,13 +787,6 @@ static irqreturn_t iommu_fault_handler(int irq, void *data) if (!obj->refcount) return IRQ_NONE; - /* Dynamic loading TLB or PTE */ - if (obj->isr) - err = obj->isr(obj); - - if (!err) - return IRQ_HANDLED; - clk_enable(obj->clk); stat = iommu_report_fault(obj, &da); clk_disable(obj->clk); @@ -802,6 +795,13 @@ static irqreturn_t iommu_fault_handler(int irq, void *data) iommu_disable(obj); + /* user registered callback */ + if (obj->isr) + err = obj->isr(obj, da); + + if (!err) + return IRQ_HANDLED; + iopgd = iopgd_offset(obj, da); > > - bool ret = false; > - hw_status hw_status_obj; > - struct cfg_hostres *resources = dev_context->resources; > - > - if (!resources) { > - dev_dbg(bridge, "%s: Failed to get Host Resources in\n", > - __func__); > - return ret; > - } > - hw_status_obj = > - hw_mmu_event_status(resources->dw_dmmu_base, &dmmu_event_mask); > - if (dmmu_event_mask == HW_MMU_TRANSLATION_FAULT) { > - hw_mmu_fault_addr_read(resources->dw_dmmu_base, &fault_addr); > - ret = true; > - } > - return ret; > + return 0; > } > diff --git a/drivers/dsp/bridge/core/mmu_fault.h b/drivers/dsp/bridge/core/mmu_fault.h > index 74db489..df3fba6 100644 > --- a/drivers/dsp/bridge/core/mmu_fault.h > +++ b/drivers/dsp/bridge/core/mmu_fault.h > @@ -19,8 +19,6 @@ > #ifndef MMU_FAULT_ > #define MMU_FAULT_ > > -extern u32 fault_addr; > - > /* > * ======== mmu_fault_dpc ======== > * Deferred procedure call to handle DSP MMU fault. > @@ -31,6 +29,7 @@ void mmu_fault_dpc(IN unsigned long pRefData); > * ======== mmu_fault_isr ======== > * ISR to be triggered by a DSP MMU fault interrupt. > */ > -irqreturn_t mmu_fault_isr(int irq, IN void *pRefData); > +int mmu_fault_isr(struct iommu *mmu); > + > > #endif /* MMU_FAULT_ */ > diff --git a/drivers/dsp/bridge/core/tiomap3430.c b/drivers/dsp/bridge/core/tiomap3430.c > index 96cceea..89867e7 100644 > --- a/drivers/dsp/bridge/core/tiomap3430.c > +++ b/drivers/dsp/bridge/core/tiomap3430.c > @@ -57,6 +57,7 @@ > #include "_tiomap.h" > #include "_tiomap_pwr.h" > #include "tiomap_io.h" > +#include "mmu_fault.h" > > /* Offset in shared mem to write to in order to synchronize start with DSP */ > #define SHMSYNCOFFSET 4 /* GPP byte offset */ > @@ -382,6 +383,7 @@ static int bridge_brd_start(struct bridge_dev_context *hDevContext, > goto end; > } > dev_context->dsp_mmu = mmu; > + mmu->isr = mmu_fault_isr; > sm_sg = dev_context->sh_s; > > sm_sg->seg0_da = iommu_kmap(mmu, sm_sg->seg0_da, sm_sg->seg0_pa, > diff --git a/drivers/dsp/bridge/core/ue_deh.c b/drivers/dsp/bridge/core/ue_deh.c > index ce13e6c..a03d172 100644 > --- a/drivers/dsp/bridge/core/ue_deh.c > +++ b/drivers/dsp/bridge/core/ue_deh.c > @@ -18,6 +18,7 @@ > > /* ----------------------------------- Host OS */ > #include <dspbridge/host_os.h> > +#include <plat/iommu.h> > > /* ----------------------------------- DSP/BIOS Bridge */ > #include <dspbridge/std.h> > @@ -51,12 +52,6 @@ > #include "_tiomap_pwr.h" > #include <dspbridge/io_sm.h> > > - > -static struct hw_mmu_map_attrs_t map_attrs = { HW_LITTLE_ENDIAN, > - HW_ELEM_SIZE16BIT, > - HW_MMU_CPUES > -}; > - > static void *dummy_va_addr; > > int bridge_deh_create(struct deh_mgr **ret_deh_mgr, > @@ -154,10 +149,10 @@ int bridge_deh_register_notify(struct deh_mgr *deh_mgr, u32 event_mask, > void bridge_deh_notify(struct deh_mgr *deh_mgr, u32 ulEventMask, u32 dwErrInfo) > { > struct bridge_dev_context *dev_context; > - int status = 0; > u32 hw_mmu_max_tlb_count = 31; > struct cfg_hostres *resources; > - hw_status hw_status_obj; > + u32 fault_addr, tmp; > + struct iotlb_entry e; > > if (!deh_mgr) > return; > @@ -181,6 +176,9 @@ void bridge_deh_notify(struct deh_mgr *deh_mgr, u32 ulEventMask, u32 dwErrInfo) > break; > case DSP_MMUFAULT: > /* MMU fault routine should have set err info structure. */ > + fault_addr = iommu_read_reg(dev_context->dsp_mmu, > + MMU_FAULT_AD); > + > deh_mgr->err_info.dw_err_mask = DSP_MMUFAULT; > dev_err(bridge, "%s: %s, err_info = 0x%x\n", > __func__, "DSP_MMUFAULT", dwErrInfo); > @@ -206,21 +204,18 @@ void bridge_deh_notify(struct deh_mgr *deh_mgr, u32 ulEventMask, u32 dwErrInfo) > dev_context->num_tlb_entries = > dev_context->fixed_tlb_entries; > } > - if (DSP_SUCCEEDED(status)) { > - hw_status_obj = > - hw_mmu_tlb_add(resources->dw_dmmu_base, > - virt_to_phys(dummy_va_addr), fault_addr, > - HW_PAGE_SIZE4KB, 1, > - &map_attrs, HW_SET, HW_SET); > - } > + dsp_iotlb_init(&e, fault_addr & PAGE_MASK, > + virt_to_phys(dummy_va_addr), IOVMF_PGSZ_4K); > + load_iotlb_entry(dev_context->dsp_mmu, &e); > + > > dsp_clk_enable(DSP_CLK_GPT8); > > dsp_gpt_wait_overflow(DSP_CLK_GPT8, 0xfffffffe); > > - /* Clear MMU interrupt */ > - hw_mmu_event_ack(resources->dw_dmmu_base, > - HW_MMU_TRANSLATION_FAULT); > + tmp = iommu_read_reg(dev_context->dsp_mmu, MMU_IRQSTATUS); > + iommu_write_reg(dev_context->dsp_mmu, tmp, MMU_IRQSTATUS); > + Why is the above necessary? > dump_dsp_stack(deh_mgr->hbridge_context); > dsp_clk_disable(DSP_CLK_GPT8); > break; > -- > 1.6.3.3 > -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo(a)vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/ |